From 3262f1a050f73a263238e0b1291f78383100bd89 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 13:45:20 +1000 Subject: [PATCH] Give ingestion a role that can write ai-ingest built its DSN from PGUSER/PGPASSWORD and takes its environment from ~/ai/api.env, where PGUSER=agent_ro - SELECT and nothing else, deliberately, because it is what the answer path runs as. So docker compose -f ~/ai-compose.yml run --rm ai-ingest --all connected as a role that cannot INSERT INTO doc_chunks, and Phase 3 was unrunnable exactly as the README documents it. Nothing had reached Phase 3 yet, so nobody had hit it. The failure would also have landed at the worst possible moment: at the final INSERT, after the Docling parse, after a person had typed the header confirmations for every file, and after a billed embeddings call - with a permission error naming no cause. - ingest_rw moves to 003_roles.sql, at Phase 1 with the other roles. It is not a Phase 9 concept; ingestion has needed a writing role since Phase 3 and never had one. 004 keeps only its grants on the upload queue, and its idempotent role creation so it still applies to an older database. - ingest.py connects through INGEST_DB_USER / INGEST_DB_PASSWORD, falling back to PGUSER only for a local shell where one pair is set. - require_write_access() checks INSERT, UPDATE and DELETE on doc_chunks before anything is parsed or embedded, and fails with the fix in the message. Falling back to PGUSER cannot smuggle agent_ro past it. - Keyword connection parameters rather than a URL: a generated password containing @ or / breaks a DSN string silently. - A missing doc_chunks now says "apply 001_schema.sql" instead of raising UndefinedTable. Phase 1's gate gains the check that would have caught this: ingest_rw must be able to write doc_chunks. An ingestion role that cannot write is the same class of failure as an API role that can - it just surfaces two phases later. Co-Authored-By: Claude Opus 5 --- BUILD-AI-CONTAINERS.md | 5 ++-- README.md | 8 ++++- db/003_roles.sql | 47 +++++++++++++++++++++++++++-- db/004_doc_uploads.sql | 17 +++++------ ingest/ingest.py | 68 ++++++++++++++++++++++++++++++++++++++---- 5 files changed, 124 insertions(+), 21 deletions(-) diff --git a/BUILD-AI-CONTAINERS.md b/BUILD-AI-CONTAINERS.md index 97974bf..0449f16 100644 --- a/BUILD-AI-CONTAINERS.md +++ b/BUILD-AI-CONTAINERS.md @@ -424,6 +424,7 @@ Each phase ends in a gate. Gates are not suggestions. - [ ] `docker ps` shows `pg-ai` healthy; `docker logs pg-ai` clean - [ ] `vector` extension present - [ ] As `agent_ro`: `SELECT` works, `INSERT` is rejected +- [ ] As `ingest_rw`: `INSERT`, `UPDATE` and `DELETE` on `doc_chunks` all work, and `INSERT` on `equipment` is rejected. **An ingestion role that cannot write is the same failure as an API role that can** — it just surfaces two phases later - [ ] Every equipment item and tag has at least one human-friendly alias - [ ] `pg-ai` is **not** reachable from the `proxy` network and publishes no host port - [ ] `df -h /` unchanged — nothing landed on the root disk @@ -586,7 +587,7 @@ Deployed early, deliberately: from here on, every experiment is traced. 3. `ai-api`: the `/docs/*` router — upload, list, detail, approve, reject, withdraw, restore, purge. Identity comes from Authelia's forwarded headers, never from the request body. Approval requires the publisher group and is refused without it, whatever Authelia allowed through. 4. `ai-docs-worker` — the `ingest` image with `worker.py` as its entrypoint. Long-running, `ai-internal` only, no published port. Pre-scans uploads for a header proposal, ingests approved ones, and completes withdrawals, restores and purges. `/datadisk/ai-docs-withdrawn` is created and mounted with the other two. - **While you are in `ingest.py`: make `mark_superseded()` move the superseded file out of the tree as well.** Without that, `--all` re-ingests it as live — see the defect note in section 16.10. -5. `ingest.py`: extract `ingest_file(path, header=...)` so a header confirmed in the UI is passed in. `confirm_header()` stays the CLI path. Neither one gets a way to ingest an unconfirmed header. **Also switch its DSN to `INGEST_DB_USER`/`INGEST_DB_PASSWORD`** — see the defect note in section 16.1; the CLI path is currently connecting as `agent_ro` and cannot write. +5. `ingest.py`: extract `ingest_file(path, header=...)` so a header confirmed in the UI is passed in. `confirm_header()` stays the CLI path. Neither one gets a way to ingest an unconfirmed header. The DSN already resolves through `INGEST_DB_USER`, so the worker inherits the right role by construction. 6. `db/006_doc_pool.sql` — `pool_enabled`, the profile tables, `pool_status` / `pool_documents`. Then `tools/retrieval.py` gains the `pool_enabled` predicate and an optional per-request profile, `contracts.py` gains `pool_scope` on `BaseAnswer`, and the UI gains the banner. **Read the HNSW note at the top of 006 before trimming the pool for a demo.** 7. `ai-web`: a **Documents** view — upload form, review queue, review screen, the published list, and the pool screen. The nav entry is hidden without the publisher group; the hiding is cosmetic, the API check is the control. 8. Caddyfile: `copy_headers` on the `api.yokogawa.tech` block and a `request_body max_size`. Authelia: the `/docs/.*` resource rule for `AI_DocPublishers`, **placed before** the general `api.yokogawa.tech` rule. @@ -706,7 +707,7 @@ Built at Phase 9. The operational need: when the PLC logic or the SCADA program So: **no**, nothing in the current setup allows an operator to add a document, and none of the missing pieces is a small one. The design below adds them without moving any of the human decisions. -> **Defect found while writing this, and fixed by it.** `ingest.py` builds its DSN from `PGUSER`/`PGPASSWORD`, and `ai-ingest` takes its environment from `~/ai/api.env`, where `PGUSER=agent_ro` — a role deliberately granted `SELECT` and nothing else. As configured, `docker compose run --rm ai-ingest --all` connects as a role that cannot `INSERT INTO doc_chunks`, so the Phase 3 command in the README fails at the write unless someone overrides `PGUSER` on the command line. `ingest_rw` in `db/004_doc_uploads.sql` is the role that path should have been using; `ingest.py` should read `INGEST_DB_USER`/`INGEST_DB_PASSWORD` and fall back to `PGUSER` only when they are unset. Fix it with Phase 9, or sooner if Phase 3 is being run before then. +> **Defect found while writing this. Now fixed — recorded because the shape of it is worth keeping.** `ingest.py` built its DSN from `PGUSER`/`PGPASSWORD`, and `ai-ingest` takes its environment from `~/ai/api.env`, where `PGUSER=agent_ro` — a role deliberately granted `SELECT` and nothing else, because it is what the answer path runs as. So `docker compose run --rm ai-ingest --all` connected as a role that cannot `INSERT INTO doc_chunks`, and **Phase 3 was unrunnable exactly as the README documents it**. Two things hid it: nothing had reached Phase 3 yet, and the failure would have landed at the very end of a run, after the Docling parse, after a person had typed the header confirmations, and after a billed embeddings call. `ingest_rw` now lives in `db/003_roles.sql` — Phase 1, with the other roles, because ingestion has needed a writing role since Phase 3 and simply never had one — and `ingest.py` connects through `INGEST_DB_USER`, refusing at startup if the role it lands on cannot write. ### 16.2 What does not change diff --git a/README.md b/README.md index 50df30c..46a33a5 100644 --- a/README.md +++ b/README.md @@ -146,8 +146,14 @@ Creates `/datadisk/pg-ai`, starts `pg-ai`, applies the schema and roles, loads `equipment.csv` and `tags.csv` with their alias arrays, and — while `USE_FIXTURES=true` — loads the fixture stand-in for `imh`. +Three roles come out of this, and the split matters: `agent_ro` for the answer +path (SELECT only, everywhere), `cube_rw` for pre-aggregations, and `ingest_rw` +— the only role that writes `doc_chunks`. Set all three passwords in the `0600` +env files; `INGEST_DB_USER` / `INGEST_DB_PASSWORD` are what Phase 3 connects +with, and Phase 3 refuses to start without them. + **Gate:** `pg-ai` healthy, `vector` present, `agent_ro` can SELECT and cannot -INSERT, every equipment item and tag has an alias, `pg-ai` publishes no host +INSERT, `ingest_rw` can write `doc_chunks` and nothing else, every equipment item and tag has an alias, `pg-ai` publishes no host port and is not on the `proxy` network, and `df -h /` is unchanged. `./scripts/verify.sh` checks all of it. diff --git a/db/003_roles.sql b/db/003_roles.sql index 2e59ef1..6f4f7bc 100644 --- a/db/003_roles.sql +++ b/db/003_roles.sql @@ -7,10 +7,17 @@ -- \set agent_pw `echo "$AGENT_DB_PASSWORD"` -- or ALTER ROLE ... PASSWORD after creation, from a shell that reads ~/ai/*.env. -- --- Two roles, deliberately different: +-- Three roles, deliberately different: -- agent_ro the API. SELECT only, everywhere. It must not be able to write. -- cube_rw Cube. SELECT on reference data, full rights on cube_preagg only, -- because pre-aggregation refresh creates and drops tables there. +-- ingest_rw ingestion. The ONLY role that writes doc_chunks. +-- +-- ingest_rw is created here, at Phase 1, and not with the Phase 9 upload tables +-- that also use it. Ingestion has needed a writing role since Phase 3; it just +-- never had one. ai-ingest took PGUSER from api.env, which is agent_ro, so +-- `ai-ingest --all` connected as a SELECT-only role and could not insert a +-- single chunk. Phase 3 was unrunnable as documented. -- ============================================================================= -- --- agent_ro — the application role. SELECT and nothing else. --------------- @@ -68,6 +75,33 @@ GRANT USAGE ON SCHEMA cube_preagg TO agent_ro; ALTER DEFAULT PRIVILEGES FOR ROLE cube_rw IN SCHEMA cube_preagg GRANT SELECT ON TABLES TO agent_ro; +-- --- ingest_rw — ingestion. The only role that writes doc_chunks. ----------- +-- Used by `ai-ingest` from the command line and, from Phase 9, by +-- ai-docs-worker. Never used by ai-api: the component reachable from the +-- internet is not the component that can put a document chunk in front of an +-- operator. +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'ingest_rw') THEN + CREATE ROLE ingest_rw LOGIN; + END IF; +END +$$; + +GRANT CONNECT ON DATABASE plant TO ingest_rw; +GRANT USAGE ON SCHEMA public TO ingest_rw; +GRANT SELECT ON ALL TABLES IN SCHEMA public TO ingest_rw; +ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT ON TABLES TO ingest_rw; + +-- doc_chunks only. DELETE is required: re-ingesting a file deletes its chunks +-- and reinserts them in one transaction, which is what stops re-runs +-- duplicating. It writes no other table in this schema. +GRANT INSERT, UPDATE, DELETE ON doc_chunks TO ingest_rw; +GRANT USAGE, SELECT ON SEQUENCE doc_chunks_id_seq TO ingest_rw; + +REVOKE CREATE ON SCHEMA public FROM ingest_rw; +REVOKE TEMPORARY ON DATABASE plant FROM ingest_rw; + -- ============================================================================= -- Phase 1 gate — verify, do not assume. As agent_ro: -- @@ -75,7 +109,16 @@ ALTER DEFAULT PRIVILEGES FOR ROLE cube_rw IN SCHEMA cube_preagg -- INSERT INTO equipment VALUES ('X'); -- must be REJECTED -- CREATE TABLE t (i int); -- must be REJECTED -- --- An INSERT that succeeds here is a Phase 1 failure, not a detail to fix later. +-- And as ingest_rw, because Phase 3 depends on it: +-- +-- INSERT INTO doc_chunks (source_file, doc_type, chunk_text) +-- VALUES ('gate','manual','gate'); -- must SUCCEED +-- DELETE FROM doc_chunks WHERE source_file = 'gate'; -- must SUCCEED +-- INSERT INTO equipment VALUES ('X'); -- must be REJECTED +-- +-- An INSERT that succeeds as agent_ro is a Phase 1 failure, not a detail to fix +-- later. An INSERT that FAILS as ingest_rw is the same failure in reverse: it +-- means Phase 3 will not run. -- The API's SQL allow-list in guardrails.py is the second line of defence, not -- the first; this role is the first. -- ============================================================================= diff --git a/db/004_doc_uploads.sql b/db/004_doc_uploads.sql index f5952dd..ae79f66 100644 --- a/db/004_doc_uploads.sql +++ b/db/004_doc_uploads.sql @@ -197,8 +197,10 @@ SELECT doc_number, -- uploads_rw ai-api. Writes the QUEUE and nothing else. It cannot write -- doc_chunks, so no defect in an HTTP endpoint can put a chunk in -- front of an operator without a human approval in between. --- ingest_rw ai-docs-worker. Writes doc_chunks. Has no HTTP surface at all --- and is not on the proxy network. +-- ingest_rw ai-docs-worker, and the ai-ingest CLI. Writes doc_chunks. Has +-- no HTTP surface at all and is not on the proxy network. CREATED +-- IN 003_roles.sql, because ingestion has needed it since Phase 3 +-- - this file only adds its grants on the queue. -- -- ai-api therefore holds two connections: the existing agent_ro one for -- answering, and a uploads_rw one for the document screens. Do not collapse @@ -210,6 +212,8 @@ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'uploads_rw') THEN CREATE ROLE uploads_rw LOGIN; END IF; + -- ingest_rw belongs to 003_roles.sql. Kept idempotently here so this file + -- still applies cleanly against a database that predates that change. IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'ingest_rw') THEN CREATE ROLE ingest_rw LOGIN; END IF; @@ -231,15 +235,8 @@ REVOKE INSERT, UPDATE, DELETE ON doc_chunks FROM uploads_rw; REVOKE CREATE ON SCHEMA public FROM uploads_rw; REVOKE TEMPORARY ON DATABASE plant FROM uploads_rw; --- --- ingest_rw — the worker ------------------------------------------------- -GRANT CONNECT ON DATABASE plant TO ingest_rw; -GRANT USAGE ON SCHEMA public TO ingest_rw; -GRANT SELECT ON ALL TABLES IN SCHEMA public TO ingest_rw; -GRANT INSERT, UPDATE, DELETE ON doc_chunks TO ingest_rw; -GRANT USAGE, SELECT ON SEQUENCE doc_chunks_id_seq TO ingest_rw; +-- --- ingest_rw — the worker. Its doc_chunks grants are in 003_roles.sql. ---- GRANT INSERT, UPDATE ON doc_uploads TO ingest_rw; -REVOKE CREATE ON SCHEMA public FROM ingest_rw; -REVOKE TEMPORARY ON DATABASE plant FROM ingest_rw; -- The answer path reads the queue — so the UI can tell an operator a document -- is pending rather than silently not finding it — and writes nothing. diff --git a/ingest/ingest.py b/ingest/ingest.py index e282b26..6bc6ee2 100644 --- a/ingest/ingest.py +++ b/ingest/ingest.py @@ -304,6 +304,66 @@ def mark_superseded(conn: psycopg.Connection, doc_number: str, keep_revision: st return cur.rowcount +def connection_params() -> dict[str, str | int]: + """Connect as the role that can WRITE, not the one the API uses. + + PGUSER in ~/ai/api.env is agent_ro - SELECT and nothing else, deliberately, + because it is the role the answer path runs as. ai-ingest reads the same + env file, so it inherited that role and could not insert a chunk. Ingestion + connects as ingest_rw (db/003_roles.sql) via INGEST_DB_USER. + + The fallback to PGUSER exists for a local shell where only the one pair is + set. It is not a way to run ingestion as agent_ro - require_write_access() + below refuses that, whichever variable it came from. + + Keyword parameters rather than a URL: a password containing @ or / breaks a + DSN string silently, and these are generated passwords. + """ + return { + "host": os.environ["PGHOST"], + "port": int(os.environ.get("PGPORT", "5432")), + "dbname": os.environ["PGDATABASE"], + "user": os.environ.get("INGEST_DB_USER") or os.environ["PGUSER"], + "password": os.environ.get("INGEST_DB_PASSWORD") or os.environ["PGPASSWORD"], + } + + +def require_write_access(conn: psycopg.Connection) -> None: + """Refuse early, before anything is parsed, embedded or paid for. + + Without this the run does the whole job - Docling parse, header + confirmation typed by a person, an embeddings call that is billed - and + then fails on the INSERT with a permission error that names no cause. The + check is one round trip and it fails with the fix in it. + """ + try: + with conn.cursor() as cur: + cur.execute( + "SELECT current_user," + " has_table_privilege('doc_chunks', 'INSERT')," + " has_table_privilege('doc_chunks', 'UPDATE')," + " has_table_privilege('doc_chunks', 'DELETE')" + ) + user, may_insert, may_update, may_delete = cur.fetchone() + except psycopg.errors.UndefinedTable as missing: + raise SystemExit( + "doc_chunks does not exist in this database. Apply db/001_schema.sql " + "and db/003_roles.sql first - see README.md, Phase 1." + ) from missing + + if may_insert and may_update and may_delete: + log.info("connected as %s", user) + return + + raise SystemExit( + f"connected to pg-ai as {user!r}, which cannot write doc_chunks. " + "Ingestion must connect as ingest_rw. Set INGEST_DB_USER and " + "INGEST_DB_PASSWORD in ~/ai/api.env - PGUSER there is agent_ro, which " + "is SELECT-only on purpose and must stay that way. " + "If the role does not exist yet, apply db/003_roles.sql." + ) + + def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--all", action="store_true", help="ingest every document") @@ -321,18 +381,14 @@ def main() -> int: ) args = parser.parse_args() - dsn = ( - f"postgresql://{os.environ['PGUSER']}:{os.environ['PGPASSWORD']}" - f"@{os.environ['PGHOST']}:{os.environ.get('PGPORT','5432')}" - f"/{os.environ['PGDATABASE']}" - ) client = AzureOpenAI( azure_endpoint=os.environ["AZURE_OPENAI_ENDPOINT"], api_key=os.environ["AZURE_OPENAI_API_KEY"], api_version=os.environ["AZURE_OPENAI_API_VERSION"], ) - with psycopg.connect(dsn, application_name="ai-ingest") as conn: + with psycopg.connect(**connection_params(), application_name="ai-ingest") as conn: + require_write_access(conn) if args.supersede: count = mark_superseded(conn, *args.supersede) conn.commit()