Skip to content

Say in the file that these five scripts have never run (#410) - #461

Merged
realmarcin merged 2 commits into
mainfrom
literature-enhanced-imports-410
Aug 7, 2026
Merged

Say in the file that these five scripts have never run (#410)#461
realmarcin merged 2 commits into
mainfrom
literature-enhanced-imports-410

Conversation

@realmarcin

Copy link
Copy Markdown
Contributor

Addresses #410. Does not close it — the delete-or-port decision stays open, deliberately.

What was already true

#413 pinned the five in tests/test_scripts_import.py::_KNOWN_BROKEN. That tells the suite. It tells nobody who opens curate_evidence_with_pdfs.py and finds 586 lines of plausible code.

What this adds

A docstring warning on each, and a test asserting every entry in _KNOWN_BROKEN carries one — keyed on that set rather than a fixed list, so a script joining later can't arrive undocumented.

Everything the warning states, verified:

  • communitymech.literature_enhanced has never existed in any commit. The five arrived together with the phantom module in the repo's first commit (7c658e6), so there is no revision at which they worked.
  • Porting is not an import swap. They call fetch_paper(ref, download_pdf=...) and subscript the result (paper["abstract"]); the real LiteratureFetcher.fetch_paper(reference, email=...) returns an (abstract, pdf_url) tuple and has no PDF download. Every call site needs rewriting, across 1787 lines.
  • Working equivalents exist, which is what the warning points at.

Why not just delete them

That's #410's option 1 and the history supports it — but it's ~1800 lines I didn't write, and the issue frames the choice as a decision to be made. Left for you. The warning means the cost of leaving them is now near zero: nobody will mistake one for a working tool.

A finding that fell out of it

Writing the test that asserts the replacement named in those docstrings exists is what caught that it doesn't. src/communitymech/validators/reference_validator.py was deleted in 4dd299a ("Replace custom validators with official LinkML validators"); only an untracked .pyc remains, so a naive find appears to succeed.

CLAUDE.md still named it as the sole occupant of validators/ — omitting the seven that are there (gtdb_coherence, gtdb_lineage_tree, ncbi_domain, prokaryotic_lineage, shared_taxon_ids, yaml_scalars, cross_repo_ids) and that scripts/validate_strict.py, the CI gate, runs. So the one directory the gate depends on was the one described entirely wrongly. Corrected here; filed as #460 with the broader point that nothing checks that file (it also claims kb/communities/ has 60 files, where it has 333).

just qc green · 129 passed, 5 skipped in the scripts-import suite.

🤖 Generated with Claude Code

realmarcin and others added 2 commits August 6, 2026 20:49
#413 pinned the five as broken in tests/test_scripts_import.py, which tells the
suite. It tells nobody who opens curate_evidence_with_pdfs.py and finds 586
lines of plausible code. Each now carries a docstring warning saying so, and a
test asserts every entry in _KNOWN_BROKEN carries one - keyed on that set rather
than a fixed list, so a script joining later cannot arrive undocumented.

What the warning records, all of it verified: literature_enhanced has never
existed in any commit, and the five arrived together with the phantom module in
the repo's first commit, so there is no revision at which they worked. Porting
is not an import swap - they call fetch_paper(ref, download_pdf=...) and
subscript the result, while LiteratureFetcher.fetch_paper(reference, email=...)
returns a tuple and has no PDF download, so every call site needs rewriting
across 1787 lines. And working equivalents exist, which is what the warning
points at.

Deleting them is #410's option 1 and the history supports it, but that is ~1800
lines I did not write and the issue frames it as a decision, so it stays open.

Writing the test that asserts the pointed-at replacement exists is what caught
that it does not. reference_validator.py was deleted in 4dd299a when the custom
validators were replaced by the official LinkML ones; only an untracked .pyc
remains. CLAUDE.md still named it as the sole occupant of validators/, omitting
the seven that are there and that validate_strict.py - the CI gate - runs.
Corrected, and filed as #460 along with the broader point that nothing checks
that file.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing right

The review found three, and the first is the worst kind - a fresh instance of
the error class this PR exists to fix. CLAUDE.md said validate_strict.py runs
cross_repo_ids. It does not: that validator has its own script and recipe, and
validate_strict imports five, one of which (ncbi_domain) arrives transitively
through prokaryotic_lineage. Corrected to say which runs what.

Second, all five docstrings said the scripts arrived "in the first commit
(7c658e6)". 7c658e6 is the seventh; the first is 79f5196 - and the sentence
contradicted itself, claiming the module never existed in any commit and then
naming the commit they arrived in. tests/test_scripts_import.py, the file each
docstring points at, already had this right. Reworded to say only what is
checkable: the file was added in 7c658e6 already importing the phantom module,
so no revision exists where it worked.

Third, the API paragraph was pasted verbatim into all five, but
test_pdf_fetching.py never calls fetch_paper - it calls fetch_pdf_url(doi),
which LiteratureFetcher has no equivalent of at all. Its warning now says that,
including that the PDF downloading it is built around is implemented nowhere.

On the review's fourth point, that docs still advertise these as working: it is
mostly wrong. docs/AUTOMATION_TOOLS.md carries the NOT FUNCTIONAL marker
directly above the invocation, and docs/pdf_fetching_capability.md opens with
one. The real residue is that the latter's banner sits 140 lines above the
commands, so a reader landing mid-document misses it - inline markers added at
both command sections.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@realmarcin

Copy link
Copy Markdown
Contributor Author

Review round 1 — three wrong facts, in prose whose entire point is accuracy

The worst one is a fresh instance of the error this PR exists to fix

CLAUDE.md claimed validate_strict.py runs cross_repo_ids. It doesn't — that validator has its own script and recipe (just validate-cross-repo-ids). validate_strict imports five, one of which (ncbi_domain) arrives transitively via prokaryotic_lineage. Corrected to say which runs what.

Provenance

All five docstrings said the scripts arrived "in the first commit (7c658e6)". 7c658e6 is the seventh; the first is 79f5196. Worse, the sentence contradicted itself — "has never existed in any commit" followed by naming the commit they arrived in. tests/test_scripts_import.py, the file each docstring points at, already had this right. Reworded to only what's checkable: the file was added in 7c658e6 already importing the phantom module, so no revision exists where it worked.

Wrong API described

The API paragraph was pasted verbatim into all five, but test_pdf_fetching.py never calls fetch_paper — it calls fetch_pdf_url(doi), which LiteratureFetcher has no equivalent of at all. Its warning now says that, including that the PDF downloading it's built around is implemented nowhere.

On the fourth finding — mostly wrong, and worth saying so

The review said docs "still advertise four of the five as working". They don't: docs/AUTOMATION_TOOLS.md carries the NOT FUNCTIONAL marker directly above the invocation (L201→204), and docs/pdf_fetching_capability.md opens with one at L9.

The real residue is narrower and genuine: that banner sits 140 lines above the commands (L149–204), so a reader landing mid-document via search or a section link misses it. Inline markers added at both command sections, one pointing at just validate-references as the thing that works.

Confirmed by the review, and worth recording

The phantom module was checked exhaustively — no path matching literature_enhanced was ever added across all 515 commits and both roots, and git log --all -S finds nothing. The API mismatch, the 1787 lines, and the linkml-reference-validatorconf/reference_validator.yaml pointer all verified. Both new tests genuinely fail when their subject is removed, and keying on _KNOWN_BROKEN rather than a literal list means a script joining later can't arrive undocumented.

just qc green · 1597 passed, 16 skipped.

@realmarcin
realmarcin merged commit e5e0ca0 into main Aug 7, 2026
3 checks passed
@realmarcin
realmarcin deleted the literature-enhanced-imports-410 branch August 7, 2026 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant