From 360e84eee995d4109fcb650383fa5570edf8e15c Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 12:52:21 +0000 Subject: [PATCH 1/5] Read the captcha secret from where every other secret comes from `get_secret` prefers a mounted Docker secret file over the environment, and its docstring 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 at import, and then asked `os.environ` twice -- once to decide whether the captcha middleware runs at all, once to pick the secret sent to Cloudflare. So a deployment that mounts the key instead of exporting it got the captcha silently switched off: the middleware saw an empty environment variable and returned early, skipping a protection the operator had configured. If it were reached anyway, Cloudflare would be sent a null secret and the failure would read as a captcha problem rather than a configuration one. Both sites now use the resolved value. This matters now rather than later because init-docker-secrets on the security-improvements branch mounts secrets exactly this way, so the bug would have arrived with that work. Tests pin both halves: that get_secret really does prefer the mounted file and that os.environ cannot see it, and a source-level guard that the script never reaches around get_secret for this secret -- checked by reintroducing the bug, which fails the guard. The guard is source-level because the script is hyphenated and therefore not importable. Co-Authored-By: Claude Opus 5 --- bin/chat-fastapi.py | 12 ++++- tests/util/test_captcha_secret_source.py | 67 ++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 tests/util/test_captcha_secret_source.py diff --git a/bin/chat-fastapi.py b/bin/chat-fastapi.py index c49a9f1f..8c78539b 100644 --- a/bin/chat-fastapi.py +++ b/bin/chat-fastapi.py @@ -87,7 +87,12 @@ async def verify_captcha_middleware( f"{CHAINLIT_URI}/static", ] or path.startswith("/static") - or not os.getenv("CLOUDFLARE_SECRET_KEY") + # The value resolved through get_secret, not os.environ. get_secret + # prefers a mounted Docker secret file, so a deployment that mounts the + # key rather than exporting it used to land here with the env var unset + # and skip the captcha entirely -- switching off a protection the + # operator had configured, silently. + or not CLOUDFLARE_SECRET_KEY or (CHAINLIT_URI and not path.startswith(CHAINLIT_URI)) ): return await call_next(request) @@ -170,7 +175,10 @@ async def verify_captcha(request: Request) -> Response: # Verify the CAPTCHA with Cloudflare url = "https://challenges.cloudflare.com/turnstile/v0/siteverify" data = { - "secret": os.getenv("CLOUDFLARE_SECRET_KEY"), + # Same reason: os.environ can be empty while the secret is mounted, and + # sending Cloudflare a null secret fails in a way that reads as a captcha + # problem rather than a configuration one. + "secret": CLOUDFLARE_SECRET_KEY, "response": cf_turnstile_response, "remoteip": client_ip, } diff --git a/tests/util/test_captcha_secret_source.py b/tests/util/test_captcha_secret_source.py new file mode 100644 index 00000000..9f061a91 --- /dev/null +++ b/tests/util/test_captcha_secret_source.py @@ -0,0 +1,67 @@ +"""The captcha must read its secret from the same place everything else does. + +`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 -- and then decides +whether the captcha is enabled, and what secret to send to Cloudflare, by reading +`os.environ` directly. + +So a deployment that mounts the secret rather than exporting it gets the captcha +silently switched off. That is Principle IV: configuration that cannot be honoured +must stop the process, not substitute something plausible. +""" + +from pathlib import Path + +import pytest + +from util.secrets import get_secret + + +def test_get_secret_prefers_a_mounted_file_over_the_environment( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr("util.secrets.DOCKER_SECRETS", tmp_path) + (tmp_path / "CLOUDFLARE_SECRET_KEY").write_text("from-the-mounted-file\n") + monkeypatch.setenv("CLOUDFLARE_SECRET_KEY", "from-the-environment") + assert get_secret("CLOUDFLARE_SECRET_KEY") == "from-the-mounted-file" + + +def test_a_mounted_secret_is_invisible_to_os_environ( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The divergence itself, stated plainly. + + With the secret mounted and not exported, `get_secret` finds it and + `os.environ` does not. Any code that asks os.environ whether the captcha is + configured concludes it is not, and skips it. + """ + import os + + monkeypatch.setattr("util.secrets.DOCKER_SECRETS", tmp_path) + (tmp_path / "CLOUDFLARE_SECRET_KEY").write_text("mounted-only\n") + monkeypatch.delenv("CLOUDFLARE_SECRET_KEY", raising=False) + + assert get_secret("CLOUDFLARE_SECRET_KEY") == "mounted-only" + assert os.getenv("CLOUDFLARE_SECRET_KEY") is None + + +def test_chat_fastapi_never_reads_the_captcha_secret_from_os_environ() -> None: + """A source-level guard, because the script cannot be imported. + + `bin/chat-fastapi.py` is hyphenated, so it is not importable as a module and + the middleware cannot be exercised directly here. What can be checked is that + it never reaches around `get_secret` for this particular secret, which is the + mistake being prevented. + """ + source = Path("bin/chat-fastapi.py").read_text() + offenders = [ + line.strip() + for line in source.splitlines() + if "CLOUDFLARE_SECRET_KEY" in line + and ("os.getenv" in line or "os.environ" in line) + ] + assert not offenders, ( + "the captcha secret must come from get_secret, which prefers a mounted " + f"Docker secret file over the environment: {offenders}" + ) From 853db960467f705a57d0640f63f2584deb8d391b Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 12:58:13 +0000 Subject: [PATCH 2/5] Make the Alliance generator say what it does `upload_to_chromadb` was 246 lines and most of it was misleading. It listed the CSV directory and printed each filename, doing nothing with them. It carried five stray debug prints, one of which printed a loop variable after the loop. It built its path as the literal "./csv_files/alliance//", so it worked from the repo root and nowhere else. And it iterated seven filetypes while an `if filetype == "genes"` inside the loop meant only one was ever built. The function is 56 lines now. The seven column schemas move to module scope as COLUMN_SCHEMAS, 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 NOT removed: those six unused schemas. They are curated MITAB and Alliance column lists, and tests/data_generation/test_alliance_columns.py exists because a missing comma once silently concatenated string literals and collapsed seven molecular_interaction columns into three. Deleting them would have thrown away that work and its guard -- which I started to do before the test caught me. Recorded instead of assumed: the schemas cannot simply be switched on. The downloader writes variants_c_elegans.tsv, variants_zebrafish.tsv and so on, 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 -- the comment now says so. Also: a missing file is now logged and skipped rather than raising from inside Chroma, and regeneration removes the collection first, since Chroma.from_documents appends and silently doubled the reactome bundle. Co-Authored-By: Claude Opus 5 --- src/data_generation/alliance/__init__.py | 490 ++++++++++++----------- 1 file changed, 256 insertions(+), 234 deletions(-) diff --git a/src/data_generation/alliance/__init__.py b/src/data_generation/alliance/__init__.py index aed6b7db..51fc20d6 100644 --- a/src/data_generation/alliance/__init__.py +++ b/src/data_generation/alliance/__init__.py @@ -12,9 +12,12 @@ its behaviour is unverified even where the code still type-checks. """ -import os +import logging +import shutil +from pathlib import Path import requests +from chromadb.api.shared_system_client import SharedSystemClient from langchain_community.vectorstores import Chroma from data_generation.alliance.csv_generator import generate_all_csvs @@ -36,6 +39,216 @@ def get_release_version() -> str: ) +# The Alliance column schemas, at module scope because they are data: 203 lines +# of them inside the function made a 40-line routine read as a 246-line one. +# tests/data_generation/test_alliance_columns.py parses these and guards them +# against the missing-comma bug that once collapsed seven molecular_interaction +# columns into three. +COLUMN_SCHEMAS: dict[str, list[str]] = { + "genes": [ + "Your Input", + "Gene ID", + "Gene Symbol", + "Gene Name", + "Description", + "Species", + "NCBI ID", + "ENSEMBL ID", + "UniProtKB ID", + "PANTHER ID", + "RefSeq ID", + "Synonym", + "Disease Association", + "Expression Location", + "Expression Stage", + "Variants", + "Genetic Interaction", + "Molecular/Physical Interaction", + "Homo sapiens Ortholog", + "Mus musculus Ortholog", + "Rattus norvegicus Ortholog", + "Danio rerio Ortholog", + "Drosophila melanogaster Ortholog", + "Caenorhabditis elegans Ortholog", + "Saccharomyces cerevisiae Ortholog", + "Xenopus laevis Ortholog", + "Xenopus tropicalis Ortholog", + ], + "disease": [ + "Taxon", + "SpeciesName", + "DBobjectType", + "DBObjectID", + "DBObjectSymbol", + "AssociationType", + "DOID", + "DOtermName", + "WithOrtholog", + "InferredFromID", + "InferredFromSymbol", + "ExperimentalCondition", + "Modifier", + "EvidenceCode", + "EvidenceCodeName", + "Reference", + "Date", + "Source", + ], + "expression": [ + "Species", + "SpeciesID", + "GeneID", + "GeneSymbol", + "Location", + "StageTerm", + "AssayID", + "AssayTermName", + "CellularComponentID", + "CellularComponentTerm", + "CellularComponentQualifierIDs", + "CellularComponentQualifierTermNames", + "SubStructureID", + "SubStructureName", + "SubStructureQualifierIDs", + "SubStructureQualifierTermNames", + "AnatomyTermID", + "AnatomyTermName", + "AnatomyTermQualifierIDs", + "AnatomyTermQualifierTermNames", + "SourceURL", + "Source,Reference", + ], + "molecular_interaction": [ + "ID(s) interactor A", + "ID(s) interactor B", + "Alt. ID(s) interactor A", + "Alt. ID(s) interactor B", + "Alias(es) interactor A", + "Alias(es) interactor B", + "Interaction detection method(s)", + "Publication 1st author(s)", + "Publication Identifier(s)", + "Taxid interactor A", + "Taxid interactor B", + "Interaction type(s)", + "Source database(s)", + "Interaction identifier(s)", + "Confidence value(s)", + "Expansion method(s)", + "Biological role(s) interactor A", + "Biological role(s) interactor B", + "Experimental role(s) interactor A", + "Experimental role(s) interactor B", + "Type(s) interactor A", + "Type(s) interactor B", + "Xref(s) interactor A", + "Xref(s) interactor B", + "Interaction Xref(s)", + "Annotation(s) interactor A", + "Annotation(s) interactor B", + "Interaction annotation(s)", + "Host organism(s)", + "Interaction parameter(s)", + "Creation date", + "Update date", + "Checksum(s) interactor A", + "Checksum(s) interactor B", + "Interaction Checksum(s) Negative", + "Feature(s) interactor A", + "Feature(s) interactor B", + "Stoichiometry(s) interactor A", + "Stoichiometry(s) interactor B", + "Identification method participant A", + "Identification method participant B", + ], + "genetic_interaction": [ + "ID(s) interactor A", + "ID(s) interactor B", + "Alt. ID(s) interactor A", + "Alt. ID(s) interactor B", + "Alias(es) interactor A", + "Alias(es) interactor B", + "Interaction detection method(s)", + "Publication 1st author(s)", + "Publication Identifier(s)", + "Taxid interactor A", + "Taxid interactor B", + "Interaction type(s)", + "Source database(s)", + "Interaction identifier(s)", + "Confidence value(s)", + "Expansion method(s)", + "Biological role(s) interactor A", + "Biological role(s) interactor B", + "Experimental role(s) interactor A", + "Experimental role(s) interactor B", + "Type(s) interactor A", + "Type(s) interactor B", + "Xref(s) interactor A", + "Xref(s) interactor B", + "Interaction Xref(s)", + "Annotation(s) interactor A", + "Annotation(s) interactor B", + "Interaction annotation(s)", + "Host organism(s)", + "Interaction parameter(s)", + "Creation date", + "Update date", + "Checksum(s) interactor A", + "Checksum(s) interactor B", + "Interaction Checksum(s) Negative", + "Feature(s) interactor A", + "Feature(s) interactor B", + "Stoichiometry(s) interactor A", + "Stoichiometry(s) interactor B", + "Identification method participant A", + "Identification method participant B", + ], + "orthology": [ + "Gene1ID Gene1Symbol", + "Gene1SpeciesTaxonID", + "Gene1SpeciesName", + "Gene2ID Gene2Symbol", + "Gene2SpeciesTaxonID", + "Gene2SpeciesName", + "Algorithms", + "AlgorithmsMatch", + "OutOfAlgorithms", + "IsBestScore", + "IsBestRevScore", + ], + "variants": [ + "Taxon", + "SpeciesName", + "AlleleId", + "AlleleSymbol", + "AlleleSynonyms", + "VariantId", + "VariantSymbol", + "VariantSynonyms", + "VariantCrossReferences", + "AlleleAssociatedGeneId", + "AlleleAssociatedGeneSymbol", + "VariantAffectedGeneId", + "VariantAffectedGeneSymbol", + "Category", + "VariantsTypeId", + "VariantsTypeName", + "VariantsHgvsNames", + "Assembly", + "Chromosome", + "StartPosition", + "EndPosition", + "SequenceOfReference", + "SequenceOfVariant", + "MostSevereConsequenceName", + "VariantInformationReference", + "HasDiseaseAnnotations", + "HasPhenotypeAnnotations", + ], +} + + def upload_to_chromadb( embeddings_dir: str, version: str, @@ -43,246 +256,55 @@ def upload_to_chromadb( hf_model: str | None = None, device: str | None = None, ) -> Chroma | None: - metadata_columns: dict[str, list[str]] = { - "genes": [ - "Your Input", - "Gene ID", - "Gene Symbol", - "Gene Name", - "Description", - "Species", - "NCBI ID", - "ENSEMBL ID", - "UniProtKB ID", - "PANTHER ID", - "RefSeq ID", - "Synonym", - "Disease Association", - "Expression Location", - "Expression Stage", - "Variants", - "Genetic Interaction", - "Molecular/Physical Interaction", - "Homo sapiens Ortholog", - "Mus musculus Ortholog", - "Rattus norvegicus Ortholog", - "Danio rerio Ortholog", - "Drosophila melanogaster Ortholog", - "Caenorhabditis elegans Ortholog", - "Saccharomyces cerevisiae Ortholog", - "Xenopus laevis Ortholog", - "Xenopus tropicalis Ortholog", - ], - "disease": [ - "Taxon", - "SpeciesName", - "DBobjectType", - "DBObjectID", - "DBObjectSymbol", - "AssociationType", - "DOID", - "DOtermName", - "WithOrtholog", - "InferredFromID", - "InferredFromSymbol", - "ExperimentalCondition", - "Modifier", - "EvidenceCode", - "EvidenceCodeName", - "Reference", - "Date", - "Source", - ], - "expression": [ - "Species", - "SpeciesID", - "GeneID", - "GeneSymbol", - "Location", - "StageTerm", - "AssayID", - "AssayTermName", - "CellularComponentID", - "CellularComponentTerm", - "CellularComponentQualifierIDs", - "CellularComponentQualifierTermNames", - "SubStructureID", - "SubStructureName", - "SubStructureQualifierIDs", - "SubStructureQualifierTermNames", - "AnatomyTermID", - "AnatomyTermName", - "AnatomyTermQualifierIDs", - "AnatomyTermQualifierTermNames", - "SourceURL", - "Source,Reference", - ], - "molecular_interaction": [ - "ID(s) interactor A", - "ID(s) interactor B", - "Alt. ID(s) interactor A", - "Alt. ID(s) interactor B", - "Alias(es) interactor A", - "Alias(es) interactor B", - "Interaction detection method(s)", - "Publication 1st author(s)", - "Publication Identifier(s)", - "Taxid interactor A", - "Taxid interactor B", - "Interaction type(s)", - "Source database(s)", - "Interaction identifier(s)", - "Confidence value(s)", - "Expansion method(s)", - "Biological role(s) interactor A", - "Biological role(s) interactor B", - "Experimental role(s) interactor A", - "Experimental role(s) interactor B", - "Type(s) interactor A", - "Type(s) interactor B", - "Xref(s) interactor A", - "Xref(s) interactor B", - "Interaction Xref(s)", - "Annotation(s) interactor A", - "Annotation(s) interactor B", - "Interaction annotation(s)", - "Host organism(s)", - "Interaction parameter(s)", - "Creation date", - "Update date", - "Checksum(s) interactor A", - "Checksum(s) interactor B", - "Interaction Checksum(s) Negative", - "Feature(s) interactor A", - "Feature(s) interactor B", - "Stoichiometry(s) interactor A", - "Stoichiometry(s) interactor B", - "Identification method participant A", - "Identification method participant B", - ], - "genetic_interaction": [ - "ID(s) interactor A", - "ID(s) interactor B", - "Alt. ID(s) interactor A", - "Alt. ID(s) interactor B", - "Alias(es) interactor A", - "Alias(es) interactor B", - "Interaction detection method(s)", - "Publication 1st author(s)", - "Publication Identifier(s)", - "Taxid interactor A", - "Taxid interactor B", - "Interaction type(s)", - "Source database(s)", - "Interaction identifier(s)", - "Confidence value(s)", - "Expansion method(s)", - "Biological role(s) interactor A", - "Biological role(s) interactor B", - "Experimental role(s) interactor A", - "Experimental role(s) interactor B", - "Type(s) interactor A", - "Type(s) interactor B", - "Xref(s) interactor A", - "Xref(s) interactor B", - "Interaction Xref(s)", - "Annotation(s) interactor A", - "Annotation(s) interactor B", - "Interaction annotation(s)", - "Host organism(s)", - "Interaction parameter(s)", - "Creation date", - "Update date", - "Checksum(s) interactor A", - "Checksum(s) interactor B", - "Interaction Checksum(s) Negative", - "Feature(s) interactor A", - "Feature(s) interactor B", - "Stoichiometry(s) interactor A", - "Stoichiometry(s) interactor B", - "Identification method participant A", - "Identification method participant B", - ], - "orthology": [ - "Gene1ID Gene1Symbol", - "Gene1SpeciesTaxonID", - "Gene1SpeciesName", - "Gene2ID Gene2Symbol", - "Gene2SpeciesTaxonID", - "Gene2SpeciesName", - "Algorithms", - "AlgorithmsMatch", - "OutOfAlgorithms", - "IsBestScore", - "IsBestRevScore", - ], - "variants": [ - "Taxon", - "SpeciesName", - "AlleleId", - "AlleleSymbol", - "AlleleSynonyms", - "VariantId", - "VariantSymbol", - "VariantSynonyms", - "VariantCrossReferences", - "AlleleAssociatedGeneId", - "AlleleAssociatedGeneSymbol", - "VariantAffectedGeneId", - "VariantAffectedGeneSymbol", - "Category", - "VariantsTypeId", - "VariantsTypeName", - "VariantsHgvsNames", - "Assembly", - "Chromosome", - "StartPosition", - "EndPosition", - "SequenceOfReference", - "SequenceOfVariant", - "MostSevereConsequenceName", - "VariantInformationReference", - "HasDiseaseAnnotations", - "HasPhenotypeAnnotations", - ], - } - csv_dir = "./csv_files/alliance/" + version + "/" - for filename in os.listdir(csv_dir): - file_path = os.path.join(csv_dir, filename) + # Only `genes` is embedded. The other six lists above are curated MITAB and + # Alliance schemas kept for when they are, and `tests/data_generation/ + # test_alliance_columns.py` guards them against the missing-comma bug that + # once collapsed seven molecular_interaction columns into three. They are + # data waiting on code, not dead code. + # + # They are not reachable as written: csv_generator downloads variants as + # variants_c_elegans.tsv, variants_zebrafish.tsv and so on, so the + # "variants" key below could never match a file. Anything that starts + # embedding them must reconcile the two lists rather than assume they agree. + embedded = ("genes",) - if os.path.isfile(file_path): - # Get the basename (filename without path) - base_name = os.path.basename(file_path) - print(base_name) + db = None + for filetype, column_names in COLUMN_SCHEMAS.items(): + if filetype not in embedded: + continue + file = Path(csv_dir) / f"{filetype}.tsv" + if not file.is_file(): + # Returned rather than raised from inside Chroma: `force` governs + # whether the CSVs were downloaded at all, and a caller that skipped + # that should get a clear message, not a FileNotFoundError. + logging.warning("No Alliance %s file at %s; skipping.", filetype, file) + continue - db = None # Initialize db variable - - for filetype, column_names in metadata_columns.items(): - file = csv_dir + filetype + ".tsv" - if filetype == "genes": - print(column_names) - loader = MetaDataCSVLoader( - file_path=file, - metadata_columns=column_names, - encoding="utf-8", - csv_args={"delimiter": "\t"}, - ) - docs = loader.load() - embeddings = build_embeddings(hf_model, device) + loader = MetaDataCSVLoader( + file_path=str(file), + metadata_columns=column_names, + encoding="utf-8", + csv_args={"delimiter": "\t"}, + ) + docs = loader.load() + logging.info("Embedding %d Alliance %s documents.", len(docs), filetype) - db = Chroma.from_documents( - documents=docs, - embedding=embeddings, - persist_directory=os.path.join(embeddings_dir, filetype), - ) - print(db) + persist = Path(embeddings_dir) / filetype + if persist.exists(): + # Chroma.from_documents appends; a second run would double the + # collection silently, as it did to the reactome bundle. + shutil.rmtree(persist) + SharedSystemClient.clear_system_cache() - print("filetype") - print(filetype) + db = Chroma.from_documents( + documents=docs, + embedding=build_embeddings(hf_model, device), + persist_directory=str(persist), + ) - return db # Ensure to return the db at the end + return db def generate_alliance_embeddings( From d55060d9c68e839eebd72aa9da53dd887f86bfde Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 13:00:53 +0000 Subject: [PATCH 3/5] Stop two more generators doubling their collections uniprot had no guard at all, and userguide removed its collection only under `force` -- but `force` governs whether the HTML pages are re-fetched, which is a different question from whether the collection is rebuilt from them. Either way an ordinary second run appended, storing every document twice with no error and nothing in the output to suggest it. That makes three of the five generators, each written separately and each carrying the same fault. The shipped Release95 reactome bundle is what it looks like in practice: 33,498 reaction documents for 16,749 CSV rows, unnoticed until the bundle was rebuilt, with the vector retriever spending half its over-fetch on duplicates of what it already had. The real fix is one shared persist helper instead of five hand-written copies. That is deliberately not done here: reactome, uniprot and userguide have no executing tests at all, 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 this adds a source-level guard over all five first -- checked by reintroducing the bug in uniprot, which fails it -- and the extraction becomes a change something would catch. Co-Authored-By: Claude Opus 5 --- src/data_generation/uniprot/__init__.py | 10 +++ src/data_generation/userguide/__init__.py | 10 ++- .../test_collections_replace_not_append.py | 68 +++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tests/data_generation/test_collections_replace_not_append.py diff --git a/src/data_generation/uniprot/__init__.py b/src/data_generation/uniprot/__init__.py index 37de1254..42e4330b 100644 --- a/src/data_generation/uniprot/__init__.py +++ b/src/data_generation/uniprot/__init__.py @@ -1,6 +1,8 @@ import os +import shutil from pathlib import Path +from chromadb.api.shared_system_client import SharedSystemClient from langchain_community.vectorstores import Chroma from data_generation.embeddings import build_embeddings @@ -36,6 +38,14 @@ def upload_to_chromadb( embeddings_instance = build_embeddings(hf_model, device, chunk_size=500) + # Chroma.from_documents appends, so a second run doubles the collection + # silently. The shipped reactome bundle carried every reaction twice for + # exactly this reason, and nothing reported it. + persist = Path(embeddings_dir) / embedding_table + if persist.exists(): + shutil.rmtree(persist) + SharedSystemClient.clear_system_cache() + return Chroma.from_documents( documents=docs, embedding=embeddings_instance, diff --git a/src/data_generation/userguide/__init__.py b/src/data_generation/userguide/__init__.py index 9b0b4c46..70ccb093 100644 --- a/src/data_generation/userguide/__init__.py +++ b/src/data_generation/userguide/__init__.py @@ -2,6 +2,7 @@ from pathlib import Path from shutil import rmtree +from chromadb.api.shared_system_client import SharedSystemClient from langchain_community.vectorstores import Chroma from langchain_core.documents import Document @@ -39,8 +40,15 @@ def generate_userguide_embeddings( ) -> None: embeddings_path = Path(embeddings_dir) chroma_dir = embeddings_path / CHROMA_COLLECTION - if force and chroma_dir.exists(): + # Unconditionally, not just under `force`. Chroma.from_documents appends, so + # a plain rebuild used to add a second copy of every section to the existing + # collection -- the same fault that left the shipped reactome bundle with + # 33,498 documents for 16,749 rows. `force` governs whether the HTML pages + # are re-fetched, which is a separate question from whether this collection + # is rebuilt from them. + if chroma_dir.exists(): rmtree(chroma_dir) + SharedSystemClient.clear_system_cache() cache_dir = embeddings_path / HTML_CACHE_DIR html_paths = fetch_userguide_pages( diff --git a/tests/data_generation/test_collections_replace_not_append.py b/tests/data_generation/test_collections_replace_not_append.py new file mode 100644 index 00000000..8960e56a --- /dev/null +++ b/tests/data_generation/test_collections_replace_not_append.py @@ -0,0 +1,68 @@ +"""Every generator must replace its collection, never add to it. + +`Chroma.from_documents` appends to whatever is already in the persist directory. +Running a build twice therefore stores every document twice, with no error and +nothing in the output to suggest it. That is not hypothetical: the Release95 +reactome bundle shipped with 33,498 reaction documents for 16,749 CSV rows -- +exactly 2.00x -- and it went unnoticed until the bundle was rebuilt for +Release 97. The vector retriever over-fetches and de-duplicates on `st_id`, so +half of that over-fetch was being spent on duplicates of what it already had. + +The same fault was present in three of the five generators, each written +separately. This is a source-level guard over all of them, because four of the +five have no executing tests at all and the real fix -- one shared persist +helper -- should not be attempted until something would catch it going wrong. +""" + +import ast +from pathlib import Path + +import pytest + +SRC = Path(__file__).resolve().parents[2] / "src/data_generation" + +GENERATORS = [ + "reactome/__init__.py", + "alliance/__init__.py", + "uniprot/__init__.py", + "userguide/__init__.py", + "disease_variant/__init__.py", +] + + +def _calls_from_documents(tree: ast.AST) -> bool: + return any( + isinstance(n, ast.Call) + and isinstance(n.func, ast.Attribute) + and n.func.attr == "from_documents" + for n in ast.walk(tree) + ) + + +@pytest.mark.parametrize("relative", GENERATORS) +def test_generator_removes_the_collection_before_writing_it(relative: str) -> None: + source = (SRC / relative).read_text() + tree = ast.parse(source) + if not _calls_from_documents(tree): + pytest.skip(f"{relative} does not persist a Chroma collection") + + assert "rmtree" in source, ( + f"{relative} calls Chroma.from_documents without removing the existing " + "collection first, so a second run doubles it silently" + ) + # Removing the directory leaves chromadb's cached system client pointing at + # a deleted sqlite file; the next write then fails with "attempt to write a + # readonly database", which reads as corruption rather than as this. + assert "clear_system_cache" in source, ( + f"{relative} removes the collection directory without clearing " + "chromadb's cached client" + ) + + +def test_userguide_does_not_gate_the_removal_on_force() -> None: + """`force` means re-fetch the HTML, not "rebuild the collection". + + Gating the removal on it meant an ordinary rebuild appended. + """ + source = (SRC / "userguide/__init__.py").read_text() + assert "if force and chroma_dir.exists():" not in source From 0a799a51d8a1cf7ed8db9fcc61b3976b7dd577b2 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 13:02:15 +0000 Subject: [PATCH 4/5] Give the Alliance generator its bundle directory instead of the CWD It wrote and read "./csv_files/alliance//", relative to the process's working directory. The two halves agreed only because both were relative in the same way, so it worked from the repo root and silently found nothing anywhere else. Every other generator derives its paths from embeddings_dir; this one now does too, and the directory is passed to the downloader rather than assumed by both ends. Checked across src/: alliance held the only working-directory-relative paths. Co-Authored-By: Claude Opus 5 --- src/data_generation/alliance/__init__.py | 9 +++++++-- src/data_generation/alliance/csv_generator.py | 15 +++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/data_generation/alliance/__init__.py b/src/data_generation/alliance/__init__.py index 51fc20d6..30af651d 100644 --- a/src/data_generation/alliance/__init__.py +++ b/src/data_generation/alliance/__init__.py @@ -256,7 +256,10 @@ def upload_to_chromadb( hf_model: str | None = None, device: str | None = None, ) -> Chroma | None: - csv_dir = "./csv_files/alliance/" + version + "/" + # Derived from embeddings_dir, as every other generator does. It was the + # literal "./csv_files/alliance//", so generation worked from the + # repo root and silently found nothing anywhere else. + csv_dir = str(Path(embeddings_dir) / "csv_files" / "alliance" / version) # Only `genes` is embedded. The other six lists above are curated MITAB and # Alliance schemas kept for when they are, and `tests/data_generation/ @@ -325,5 +328,7 @@ def generate_alliance_embeddings( ) exit() - generate_all_csvs(release_version, force) + # Same directory the loader reads from, passed rather than assumed: the two + # used to agree only because both were relative to the working directory. + generate_all_csvs(release_version, force, embeddings_dir) upload_to_chromadb(embeddings_dir, release_version, force, hf_model, device) diff --git a/src/data_generation/alliance/csv_generator.py b/src/data_generation/alliance/csv_generator.py index c92263a2..62fb1bab 100644 --- a/src/data_generation/alliance/csv_generator.py +++ b/src/data_generation/alliance/csv_generator.py @@ -39,9 +39,9 @@ def download_file(url: str, dest: str, force: bool) -> str | None: return None -def get_genes(version: str, force: bool) -> str: +def get_genes(version: str, force: bool, parent_dir: str = ".") -> str: # Define the file path - directory = f"csv_files/alliance/{version}" + directory = f"{parent_dir}/csv_files/alliance/{version}" os.makedirs(directory, exist_ok=True) gene_csv = f"{directory}/genes.tsv" @@ -106,11 +106,13 @@ def get_genes(version: str, force: bool) -> str: return gene_csv -def generate_all_csvs(version: str, force: bool) -> tuple[str, ...]: +def generate_all_csvs( + version: str, force: bool, parent_dir: str = "." +) -> tuple[str, ...]: files = [] # Download gene file - gene_csv = get_genes(version, force) + gene_csv = get_genes(version, force, parent_dir) files.append(gene_csv) # Define other files to download @@ -129,8 +131,9 @@ def generate_all_csvs(version: str, force: bool) -> tuple[str, ...]: } for name, url in other_files.items(): - gz_dest = f"csv_files/alliance/{version}/{name}.tsv.gz" - csv_dest = f"csv_files/alliance/{version}/{name}.tsv" + base = f"{parent_dir}/csv_files/alliance/{version}" + gz_dest = f"{base}/{name}.tsv.gz" + csv_dest = f"{base}/{name}.tsv" if not os.path.exists(csv_dest) or force: unzipped_dest = download_file(url, gz_dest, force) From 217dc2e0bbe04fc831351b16b5f2295381af7d6a Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 17 Sep 2026 13:18:55 +0000 Subject: [PATCH 5/5] Test what the generators do, not what their source looks like Two faults in yesterday's review, found by attacking it. The guard I wrote asserted that `rmtree` and `clear_system_cache` appear in each generator's source. That passes when the code removes a completely unrelated directory -- checked, and it did pass. It was asserting the shape of the code rather than its behaviour, which is the thing this project keeps catching elsewhere and I did it in the detector itself. The tests now build each collection twice with a fake embedding and assert the count does not double. Checked against both mutations: removing the guard entirely, and removing the *wrong* directory. The old test caught only the first; this catches both. userguide stays source-level because generating it fetches pages over the network, and that is said in the test rather than left to be discovered. Second fault: `parent_dir: str = "."` on the Alliance CSV functions. The whole 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. It is required now, so forgetting it is a TypeError rather than files appearing somewhere nobody looks. A new generator without a case in this file now fails a test, because five hand-written persist blocks is why the same fault appeared three times. Co-Authored-By: Claude Opus 5 --- src/data_generation/alliance/csv_generator.py | 8 +- .../test_collections_replace_not_append.py | 165 +++++++++++++----- 2 files changed, 120 insertions(+), 53 deletions(-) diff --git a/src/data_generation/alliance/csv_generator.py b/src/data_generation/alliance/csv_generator.py index 62fb1bab..c6dc2972 100644 --- a/src/data_generation/alliance/csv_generator.py +++ b/src/data_generation/alliance/csv_generator.py @@ -39,7 +39,7 @@ def download_file(url: str, dest: str, force: bool) -> str | None: return None -def get_genes(version: str, force: bool, parent_dir: str = ".") -> str: +def get_genes(version: str, force: bool, parent_dir: str) -> str: # Define the file path directory = f"{parent_dir}/csv_files/alliance/{version}" os.makedirs(directory, exist_ok=True) @@ -106,9 +106,7 @@ def get_genes(version: str, force: bool, parent_dir: str = ".") -> str: return gene_csv -def generate_all_csvs( - version: str, force: bool, parent_dir: str = "." -) -> tuple[str, ...]: +def generate_all_csvs(version: str, force: bool, parent_dir: str) -> tuple[str, ...]: files = [] # Download gene file @@ -130,8 +128,8 @@ def generate_all_csvs( "variants_yeast": "https://fms.alliancegenome.org/download/VARIANT-ALLELE_NCBITaxon559292.tsv.gz", } + base = f"{parent_dir}/csv_files/alliance/{version}" for name, url in other_files.items(): - base = f"{parent_dir}/csv_files/alliance/{version}" gz_dest = f"{base}/{name}.tsv.gz" csv_dest = f"{base}/{name}.tsv" diff --git a/tests/data_generation/test_collections_replace_not_append.py b/tests/data_generation/test_collections_replace_not_append.py index 8960e56a..e3e4321b 100644 --- a/tests/data_generation/test_collections_replace_not_append.py +++ b/tests/data_generation/test_collections_replace_not_append.py @@ -1,68 +1,137 @@ -"""Every generator must replace its collection, never add to it. - -`Chroma.from_documents` appends to whatever is already in the persist directory. -Running a build twice therefore stores every document twice, with no error and -nothing in the output to suggest it. That is not hypothetical: the Release95 -reactome bundle shipped with 33,498 reaction documents for 16,749 CSV rows -- -exactly 2.00x -- and it went unnoticed until the bundle was rebuilt for -Release 97. The vector retriever over-fetches and de-duplicates on `st_id`, so -half of that over-fetch was being spent on duplicates of what it already had. - -The same fault was present in three of the five generators, each written -separately. This is a source-level guard over all of them, because four of the -five have no executing tests at all and the real fix -- one shared persist -helper -- should not be attempted until something would catch it going wrong. +"""Building a collection twice must not store everything twice. + +`Chroma.from_documents` appends to whatever is already in the persist directory, +so a second build silently doubles the collection. The shipped Release95 bundle +is what that looks like: 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 of +what it already had. + +Three of the five generators had this, each written separately. + +These tests *run* the generators rather than inspecting their source. The first +version of this file grepped for `rmtree`, and an adversarial check showed it +passed even when the code deleted a completely unrelated directory -- it was +asserting the shape of the code, not what it does. """ -import ast +import csv from pathlib import Path +from typing import Any import pytest +from langchain_core.embeddings import FakeEmbeddings -SRC = Path(__file__).resolve().parents[2] / "src/data_generation" +import data_generation.alliance as alliance_mod +import data_generation.reactome as reactome_mod +import data_generation.uniprot as uniprot_mod -GENERATORS = [ - "reactome/__init__.py", - "alliance/__init__.py", - "uniprot/__init__.py", - "userguide/__init__.py", - "disease_variant/__init__.py", -] +@pytest.fixture(autouse=True) +def _fake_embeddings(monkeypatch: pytest.MonkeyPatch) -> None: + """No API calls, and no dependence on an embedding model being reachable.""" + for module in (reactome_mod, uniprot_mod, alliance_mod): + monkeypatch.setattr( + module, "build_embeddings", lambda *a, **k: FakeEmbeddings(size=8) + ) -def _calls_from_documents(tree: ast.AST) -> bool: - return any( - isinstance(n, ast.Call) - and isinstance(n.func, ast.Attribute) - and n.func.attr == "from_documents" - for n in ast.walk(tree) - ) + +def _write_csv(path: Path, columns: list[str], rows: int, delimiter: str = ",") -> None: + path.parent.mkdir(parents=True, exist_ok=True) + with open(path, "w", newline="", encoding="utf-8") as handle: + writer = csv.DictWriter(handle, fieldnames=columns, delimiter=delimiter) + writer.writeheader() + for i in range(rows): + writer.writerow({c: f"{c}-{i}" for c in columns}) -@pytest.mark.parametrize("relative", GENERATORS) -def test_generator_removes_the_collection_before_writing_it(relative: str) -> None: - source = (SRC / relative).read_text() - tree = ast.parse(source) - if not _calls_from_documents(tree): - pytest.skip(f"{relative} does not persist a Chroma collection") +def test_reactome_replaces_its_collection(tmp_path: Path) -> None: + columns = [ + "st_id", + "display_name", + "canonical_gene_name", + "synonyms_gene_name", + "uniprot_link", + ] + csv_path = tmp_path / "ewas.csv" + _write_csv(csv_path, columns, rows=3) - assert "rmtree" in source, ( - f"{relative} calls Chroma.from_documents without removing the existing " - "collection first, so a second run doubles it silently" + first = reactome_mod.upload_to_chromadb(str(tmp_path), str(csv_path), "ewas", "m") + assert first._collection.count() == 3 + second = reactome_mod.upload_to_chromadb(str(tmp_path), str(csv_path), "ewas", "m") + assert second._collection.count() == 3, "a second build must not append" + + +def test_uniprot_replaces_its_collection(tmp_path: Path) -> None: + columns = [ + "gene_names", + "short_protein_name", + "full_protein_name", + "protein_family", + "biological_pathways", + ] + csv_path = tmp_path / "uniprot_data.csv" + _write_csv(csv_path, columns, rows=4) + + first = uniprot_mod.upload_to_chromadb( + str(tmp_path), str(csv_path), "uniprot_data", "m" + ) + assert first._collection.count() == 4 + second = uniprot_mod.upload_to_chromadb( + str(tmp_path), str(csv_path), "uniprot_data", "m" ) - # Removing the directory leaves chromadb's cached system client pointing at - # a deleted sqlite file; the next write then fails with "attempt to write a - # readonly database", which reads as corruption rather than as this. - assert "clear_system_cache" in source, ( - f"{relative} removes the collection directory without clearing " - "chromadb's cached client" + assert second._collection.count() == 4, "a second build must not append" + + +def test_alliance_replaces_its_collection(tmp_path: Path) -> None: + version = "1.0.0" + _write_csv( + tmp_path / "csv_files" / "alliance" / version / "genes.tsv", + alliance_mod.COLUMN_SCHEMAS["genes"], + rows=2, + delimiter="\t", ) + first = alliance_mod.upload_to_chromadb(str(tmp_path), version, False, "m") + assert first is not None + assert first._collection.count() == 2 + second = alliance_mod.upload_to_chromadb(str(tmp_path), version, False, "m") + assert second is not None + assert second._collection.count() == 2, "a second build must not append" + + +def test_alliance_reads_from_the_directory_it_is_given(tmp_path: Path) -> None: + """Not from the working directory, which is where it used to look.""" + assert alliance_mod.upload_to_chromadb(str(tmp_path), "9.9.9", False, "m") is None + def test_userguide_does_not_gate_the_removal_on_force() -> None: - """`force` means re-fetch the HTML, not "rebuild the collection". + """Source-level, because generating it fetches pages over the network. - Gating the removal on it meant an ordinary rebuild appended. + `force` means re-fetch the HTML, not "rebuild the collection"; gating the + removal on it meant an ordinary rebuild appended. """ - source = (SRC / "userguide/__init__.py").read_text() + source = ( + Path(__file__).resolve().parents[2] + / "src/data_generation/userguide/__init__.py" + ).read_text() assert "if force and chroma_dir.exists():" not in source + assert "if chroma_dir.exists():" in source + + +def test_every_generator_is_covered_here(tmp_path: Path, request: Any) -> None: + """A new generator must arrive with a case in this file. + + Five modules, each with its own hand-written persist block, is why the same + fault appeared three times. + """ + src = Path(__file__).resolve().parents[2] / "src/data_generation" + generators = { + p.parent.name + for p in src.rglob("__init__.py") + if "from_documents" in p.read_text() + } + covered = {"reactome", "uniprot", "alliance", "userguide", "disease_variant"} + assert ( + generators <= covered + ), f"no replace-not-append test for: {generators - covered}"