Repository navigation
Land the stack on main: the container, the BM25 migration and the CD workflow - #36
Merged
Merged
Conversation
`READ_STATUSES` has been ("ok", "partial") since ADR 0017, and the plan card,
`search_plans` and `plan_coverage` all honour it: a `partial` document's text
is stored, searched and quoted. Six other places still said `== "ok"`.
`make sbc-report` therefore reported 1,918 of 3,276 plans covered where the
rest of the app counts 1,940 — the number Phase 4 was signed off with was this
bug's. The same literal listed partly-read documents among the reasons a
document could not be read, skipped their kept PDFs under VERIFY=1, and hid
them from the outdated-parser count even though `_needs_reading` re-reads
them. Ingestion counted a current `partial` as a recorded failure and pointed
the operator at `make refresh-sbc` for something that had not failed.
All six derive from `READ_STATUSES` now, and the run's non-failure outcomes
are derived from it too rather than listed by hand a fourth time.
`fetch.py`'s two `!= "ok"` checks are left: a fetch is never `partial`.
Two things the API process got wrong, both invisible until something went looking. `setup_logging()` is called by every command-line entry point and by all four eval scripts, but never by `create_app()`. The root logger therefore kept Python's default in the Flask process: WARNING, with no handler. Every logger.info in the request path — retrieval's hybrid-search diagnostics, plan search, tool outcomes, the rewrite decisions, answer stats — went nowhere, and `logs/backend/app.log` held only what the ingestion and eval runs wrote. The per-library WARNING pins went uninstalled too; they exist to keep the CMS api key out of urllib3 and a user's ZIP and age out of httpx and openai, and an absent safeguard is worth fixing before this runs behind a WSGI server that configures logging itself. `get_current_user()` is annotated `-> User` and guarded neither case that breaks it. A subject that is not one of our IDs raised ValueError out of `uuid.UUID`; an account deleted since its token was issued left `db.get` returning None, which every caller dereferences. Both surfaced as a generic 500. A token that names nobody is a dead session, so both are 401 now and the client signs out instead of seeing a server error.
An audit of the finished project found three documents stating counts the database contradicts. For a project whose Phase 4 thesis is saying honestly what was read, that is the wrong kind of error to leave. ADR 0018 owns parser decisions and stopped at version 6: its status line said "parser versions 3 to 6" and its body said "`PARSER_VERSION` is 3", while extract.py has been at 9 since the live run. Versions 7, 8 and 9 — the chart ruled across but not down, the header printed in pieces, and the template's closing sentence read as a chart row — were documented nowhere but the findings. They have sections now, and the ADR says where the number lives rather than restating it. ADR 0017 and the CHANGELOG still said 62 of 1,189 documents are partial. The parser work those 62 prompted brought it to 22 within the same phase, and the refresh took the corpus to 1,246. ADR 0017 also gains the point the audit turned up the hard way: `READ_STATUSES` is the single definition of "there is text for this plan", and spelling it `== "ok"` anywhere else reintroduces the silent disagreement that section exists to stop. The CHANGELOG had no entry for either of the two largest runs in the project — the eighteen-state ingest and the first full refresh — though Phase 3's much smaller run has one. Both are recorded now, with the settled OCR answer and the eval floor raised to full marks, which also shipped without an entry. README and CLAUDE.md called the plan comparison "in progress" when all five phases are complete; they also omitted docs/findings, docs/runbooks and eval_sbc_ranking.py from the structure, and never named the `partial` status. ADR 0005 said follow-up latency was "the thing to revisit first if generation moves behind an API". ADR 0008 moved it. Recorded as still open rather than quietly left.
The trigger filtered `pull_request: branches: [main]`, so a PR based on another branch got no backend, frontend or secret-scan job at all — only the advisory Claude review, which is `continue-on-error`. A stack of related changes is exactly where a missed regression is easiest to inherit, and the filter made those the only PRs with no gate.
ADR 0013 recorded a site that would not serve /robots.txt as `blocked`, on the grounds that "a refused robots.txt counts as disallowing everything". That was written from two hosts in a two-state slice. At eighteen states it governs 482 documents, and it is stricter than RFC 9309, which puts every 400-499 in one "unavailable" class where a crawler may fetch. Only 5xx requires assuming a disallow. The cost of the rule had never been measured, so it was. All 482 documents come from seven hosts, so this is a census rather than a sample: one request each, two seconds apart, status and content type only, nothing stored. Five of the seven serve the document at 200 with application/pdf — Medica, Anthem, Wellpoint, AvMed and Mending. The prediction going in was the opposite, that a 403 on robots.txt meant a WAF refusing the client outright. That holds for UnitedHealthcare and Mercy, and they refuse on their own account: their 403 on the PDF still records the document as blocked, and is still never worked around. Nothing else moves. HTTPS only, public hosts re-checked at every redirect, two seconds between requests to one host, the 15 MB cap, and a real Disallow rule all stand. A 5xx robots.txt now blocks where it used to fall through to allow_all, which is the safer direction. A network error still fails open, as it did, but says so in the log instead of doing it silently.
The eight affected states were refreshed, 1,496 documents re-checked. The estimate held: coverage went from 1,940 plans (59.2%) to 2,274 (69.4%). Two things worth saying plainly rather than reporting the headline alone. The documents this reaches parse less completely than the ones already read: `partial` went from 22 to 138, so 69.4% is 2,136 whole documents and 138 partial ones. They are honestly marked and their text is searched, which is what ADR 0017 is for, but the number is not 2,274 whole documents. UnitedHealthcare and Mercy now fail on their own status — www.uhc.com returns 403 for the document itself, so those records carry that reason instead of one inferred from a different file. Same blocked status, same sentence to the user, accurate cause. ADR 0016 verified: 1,264 PDFs before, 1,593 after, every earlier file still present, and the archive grew by exactly the 77 documents issuers had replaced.
The robots.txt change moved them the moment it ran: 2,274 plans covered rather than 1,940, and 138 partly-read documents rather than 22.
`_bm25_index()` read `data/corpus/indices/bm25.pkl` from local disk and raised FileNotFoundError when it was absent. Nothing catches that, so the chat route's safety net turns it into a 500: an app running anywhere but the machine that built the corpus answers no questions at all — not degraded, not sparse-only, every question fails. That never showed because the app had only ever run where the file was. It cannot be fixed where it stands: `data/` is gitignored and stays that way, so no image can be built containing it. The index is derived data — every byte of it comes from the `chunks` table, which already holds each chunk's text and id. So it moves into the database next to them, as one row of `search_indexes`, and `make build-index` rebuilds it from the chunks already stored with no fetching, chunking or embedding. Seeding a fresh database is a restore plus one command. A missing index now names that command instead of naming a path that will not exist wherever the error was raised. The payload also stops carrying a second copy of the corpus: `texts` was read by nothing but a test, and dropping it took the stored index from 3.4 MB to 1.8 MB for the same 1,568 chunks. The orchestrator's tests now check what reaches the indexer at the call rather than by reading back a pickle, which is what they were really asserting; storing it has its own tests.
Closes the first of ADR 0002's two deferred items. Multi-stage, non-root, and verified by running rather than by reading. Two properties of this app shaped it. It carries a model runtime — torch and two transformers are in the request path — and it must never carry the documents, which are 983 MB of issuers' PDFs kept strictly local (ADR 0016). So torch comes from PyTorch's CPU index on linux, which dropped every nvidia package and triton from the lock, and both models are baked in at the revisions config.py pins, with HF_HUB_OFFLINE set at runtime: a cold start loads them from the image, and a HuggingFace outage cannot take the API down. CI builds the image, runs it, and fails if /app/data is not empty. A .dockerignore regression is otherwise invisible until an image is gigabytes too large, and this is the rule that must not break quietly. Trivy fails the build on fixable HIGH or CRITICAL findings. One gunicorn worker, eight threads. Flask-Limiter has no shared storage, so a second worker would keep its own counters and double the 5/min on register and 10/min on login. A deliberate ceiling, recorded as one. Production also logs JSON to stdout, because a container's filesystem is ephemeral and nobody reads it; development is unchanged so an ingestion run's lines do not interleave with its progress output. Also here because the image made them matter: .env.example, which did not exist though three settings have no default; `make test`, `lint` and `check` so passing locally means what CI means; and tiktoken and anyio dropped, both imported nowhere, tiktoken in particular sitting next to core/text.py's deliberate approximation looking like an unfinished intention. Measured: 2.48 GB, torch 2.14.0+cpu, zero nvidia packages, no dev tools, and a container with no data/ directory answering "What is a deductible?" from the Postgres-stored index at 0.999.
The action's setup step runs an install script that exits 1 on the runner with nothing on stderr, so the scan never started and the job failed without saying why. Its published image is pinned instead, the same way gitleaks is installed by checksum rather than by an action. The image is scanned from a `docker save` tar with `--input`, so the scanner is not handed the docker socket.
A tag can be moved to different bytes, so pinning by tag is not pinning. The repository already SHA-pins its GitHub Actions and installs gitleaks by checksum; images were the gap. python:3.12-slim-bookworm, ghcr.io/astral-sh/uv and aquasec/trivy now carry name:tag@sha256, the tag kept beside the digest so the version is still readable.
Closes the second of ADR 0002's two deferred items, which waited on a subscription that now exists. Container Apps for the API, Static Web Apps for the browser app, and a PostgreSQL Flexible Server. The choices that are not obvious: Migrations run as a Container Apps job on the same image digest that is about to serve traffic. Running Alembic from the GitHub runner instead would mean opening the database to GitHub's IP ranges for the sake of a deploy step; this keeps the server private and guarantees the schema is migrated by exactly the code that will then use it. The image is tagged with the commit and deployed by digest. A tag can be moved, a digest cannot, so what the reviewer approves is what runs. The trigger is `workflow_run` on a successful CI run, which makes green CI a precondition rather than a convention, and `environment: production` with a required reviewer is the approval. A post-deploy check polls the API until it answers 200, so a revision that starts but cannot serve fails the run. Sign-in is OIDC federated credentials, so no Azure secret is stored in GitHub at all. The commit a manual run asks for is checked against a 40-hex pattern before it reaches `ref:` or an image tag. One replica, deliberately: Flask-Limiter keeps its counters in process, so a second would double the 5/min on register and 10/min on login. Raising it and adding a shared store are the same piece of work. The database is seeded once by hand from a 75 MB dump, not by CI. Ingestion stays on the machine that holds data/ (ADR 0016, 0019), and nothing here uploads a PDF. docs/runbooks/deploy.md covers the one-time setup, the deploy path, rollback, and the failures worth expecting. What is still missing — staging, alerting, a shared rate-limit store — is named rather than left to be discovered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PRs #28–#35 were stacked, each based on the one below it. They were all merged
within about two minutes, and only #28 reached
main:bugfix_chat_load_errormainbugfix_partial_status_consistencybugfix_chat_load_errorbugfix_api_logging_and_auth_guardbugfix_partial_status_consistencypatch_docs_truth_upbugfix_api_logging_and_auth_guardfeature_robots_unavailablepatch_docs_truth_upfeature_bm25_in_postgresfeature_robots_unavailablefeature_container_imagefeature_bm25_in_postgresfeature_azure_cdfeature_container_image#28merged tomainfirst, carrying only its own commit. Every later mergelanded on a branch
mainhad already copied from. GitHub retargets a child PRonly when its base is merged and deleted, one at a time; merging the stack in
a burst outran that.
The result: twelve commits are on branches but not on
main— theDockerfile, the BM25 migration, the robots.txt correction, the documentation
truth-up and the entire CD workflow.
cd.ymlis absent from the defaultbranch, so GitHub reports "workflow cd.yml not found" and it can never run.
Implementation
No new work. This is the existing
feature_azure_cd— the top of the stack,which already contains all twelve commits in their reviewed order — merged
directly into
main.Validation evidence
The merge is provably lossless, not merely conflict-free:
git merge-tree --write-tree origin/main origin/feature_azure_cdexits 0 withno conflict output.
bade2d71010248680a48fe41de084ae7e1d8ca60, which isexactly
origin/feature_azure_cd^{tree}.maincontributes no contentthe branch lacks.
git statusclean;git status --short data/reports 0 changes.Each of the twelve commits was already CI-green on its own PR. CI re-runs here
against
mainand is the gate — this PR asserts nothing about green that thechecks have not yet shown.
Security / DB / API impact
f6e50c4aec26, creatingsearch_indexes.It must run before the new
retrieval.pyserves a request;make ingestormake build-indexpopulates the row.get_current_user()now returns 401 instead of 500 for a tokennaming no account.
create_app()configures logging, which installs theurllib3/httpx/openaiWARNING pins that keep the CMS?apikey=and theuser's ZIP and age out of logs.
uvand Trivy images pinned by@sha256:digest.Trivy scans the built image on every PR and fails on fixable HIGH/CRITICAL.
.dockerignoreexcludesdata/, and CI runs the built imageand asserts
/app/datais empty. No issuer PDF is in the image, and none isuploaded anywhere (ADR 0016, ADR 0019).
.envcommitted;.env.examplecarries keys and no values.CD signs in with OIDC, so no Azure credential is stored in the repo.
After this merges
cd.ymlreaches the default branch and becomes runnable, but the deploy jobwill stop at its environment gate until three things exist, per
docs/runbooks/deploy.md:App, PostgreSQL Flexible Server with
vectorallow-listed.productionGitHub environment with a required reviewer, plus the OIDCfederated credential. The only environment on this repo today is
copilot.pg_dump/pg_restoreseed (~75 MB). The SBC PDFs stay local.