Repo review: a silent captcha bypass, three doubling generators, and 246 lines that lied

[adamjohnwright]

An adversarial pass over the whole repo. Four findings, all verified, each pinned by a test that fails when the bug is put back.

1. A deployment that mounts the captcha secret gets no captcha

get_secret prefers a mounted Docker secret file over the environment, and says why: "a deployment that has gone to the trouble of mounting a secret means it". bin/chat-fastapi.py loads CLOUDFLARE_SECRET_KEY that way — then asked os.environ twice, once to decide whether the captcha middleware runs at all and once to pick the secret sent to Cloudflare.

So mounting the key instead of exporting it silently switched the captcha off: the middleware saw an empty environment variable and returned early. If it were reached anyway, Cloudflare got a None secret and the failure would read as a captcha problem rather than a configuration one.

This matters now rather than later because init-docker-secrets on the security-improvements branch mounts secrets exactly this way — the bug would have arrived with that work. Audited the rest: this was the only such divergence.

2. Three of five generators doubled their collections

Chroma.from_documents appends. Running a build twice stores every document twice, with no error.

That isn't hypothetical — the shipped Release95 bundle had 33,498 reaction documents for 16,749 CSV rows, exactly 2.00x, unnoticed until the Release 97 rebuild. The vector retriever over-fetches then de-duplicates on st_id, so half that over-fetch was spent on duplicates.

generator	before
uniprot	no guard at all
userguide	removed only under force — but force means re-fetch the HTML, not rebuild the collection
reactome, alliance, disease_variant	fixed earlier, separately

The real fix is one shared persist helper, and this PR deliberately does not do it. reactome, uniprot and userguide have no executing tests, and alliance's only test parses the AST. Extracting across five untested modules is how a fault like this gets introduced rather than removed. So there's now a source-level guard over all five, and the extraction becomes a change something would catch.

3. The Alliance generator was 246 lines and most of it was misleading

It listed the CSV directory and printed every filename, using none of them. Five stray debug prints, one printing a loop variable after the loop. And it iterated seven filetypes while an if filetype == "genes" inside the loop meant only one was ever built.

56 lines now, with the seven column schemas lifted to module scope, because they are data — 203 lines of them inside the body made a 40-line routine read as a 246-line one.

What is deliberately kept: the six unused schemas. They're curated MITAB column lists, and test_alliance_columns.py exists because a missing comma once concatenated string literals and collapsed seven molecular_interaction columns into three. I started to delete them and that test stopped me.

Recorded rather than assumed: they can't just be switched on. The downloader writes variants_c_elegans.tsv, variants_zebrafish.tsv…, while the schema key is "variants" — so that entry could never have matched a file even without the genes-only guard. Two hand-maintained lists of the same thing, already disagreeing, with nothing to notice.

4. Alliance was the only module using working-directory-relative paths

It wrote and read "./csv_files/alliance/<version>/". Both halves were relative in the same way, so it worked from the repo root and silently found nothing anywhere else. Now derived from embeddings_dir like every other generator, and passed to the downloader rather than assumed at both ends.

---

326 tests, ruff and mypy clean.

🤖 Generated with Claude Code

[adamjohnwright]

Adversarial review of this PR — two faults, both mine

The guard test asserted shape, not behaviour. It checked that rmtree and clear_system_cache appear in each generator's source. I mutated uniprot to delete a completely unrelated directory and the test passed. So the detector for a silent-doubling bug could itself be silently wrong — the exact failure mode this project keeps finding, committed in the detector.

Replaced with tests that build each collection twice with a fake embedding and assert the count doesn't double. Verified against both mutations:

mutation	old test	new test
guard removed entirely	caught	caught
guard deletes the wrong directory	passed	caught

userguide stays source-level because generating it fetches pages over the network — now stated in the test rather than left to be discovered.

parent_dir: str = "." defeated its own fix. The point of that change was to stop writing relative to the working directory, and a default of "." silently restores exactly that for any caller who forgets the argument — failing quietly, which is what Principle IV is about. It's required now, so forgetting it is a TypeError rather than files appearing where nobody looks.

Also added: a new generator without a case in this file fails a test. Five hand-written persist blocks is why the same fault appeared three times.

What held up: the captcha fix (traced again — behaviour is identical when the secret is in the environment or absent, and differs only in the mounted-file case it exists to fix), the Alliance refactor, and the finding that Alliance held the only working-directory-relative paths in src/.

326 tests, ruff and mypy clean.

🤖 Generated with Claude Code