Stop a working tool reporting "no issues" from a validator that never ran (#410)

[realmarcin]

Partially addresses #410. The delete-or-port decision stays open; what this fixes is the part that was actually reachable — and silent.

The live defect

#461 already documented the five scripts importing communitymech.literature_enhanced, a module that has never existed in any commit. What it did not reach: four working scripts still pointed at one of them, and one actually invoked it.

batch_snippet_fixer.validate_file ran:

poetry run python scripts/curate_evidence_with_pdfs.py --file X --quick

Three things wrong at once, and together they were silent:

1. that script cannot start — the import fails;
2. poetry is not this repo's runner (it uses uv);
3. cwd was yaml_path.parent.parent, i.e. kb/, not the repo root.

returncode was never checked. Reproduced against a real record before changing anything:

returncode: 2
stderr: can't open file '.../scripts/curate_evidence_with_pdfs.py'
REPORTED TO CALLER: {'total': 0, 'errors': 0, 'warnings': 0}   <-- reads as "validated, no issues"

The subprocess failed, no ERROR: N appeared for the regex to match, and the function returned zeros. 0 could not be distinguished from "could not tell". A validation step that reports success because it never ran is worse than one that is simply missing — this is the defect class this repo keeps surfacing, and here it was sitting inside a tool that looks like it works.

The fix

validate_file now calls linkml-reference-validator — the tool behind just validate-references, which #466 established does genuinely check snippets against references_cache/. Returncodes other than 0/1, and a non-zero exit with no parseable issue count, both return -1 rather than rounding down to clean.

Canaried on three inputs:

input	result
clean record	{'total': 0}
record with a snippet that is in no publication	{'total': 1} — it actually detects now
unreadable file	{'total': -1} — "could not validate", not clean

The three scripts that merely printed it as the next step — apply_suggested_fixes, apply_suggested_snippets, intelligent_snippet_fixer — now name just validate-references. A dead pointer inside a working tool is how a curator discovers the breakage, which is the worst place to discover it.

The test caught its own docstring first

The guard against dead pointers scanned raw text, so it flagged the line in batch_snippet_fixer.py that explains the historical poetry run ... command. Exactly the trap from #471, where a grep for a retired CHEBI id matched the curation note documenting its retirement. It walks the AST now and inspects only strings passed to print or subprocess.

Mutation-checked: restoring the round-down-to-clean branch fails test_a_file_that_cannot_be_validated_is_not_reported_as_clean.

What is deliberately not decided

Whether the five dead scripts are deleted or ported. The porting question is real, not bookkeeping:

• they call fetch_paper(ref, download_pdf=...) and subscript the result (paper["abstract"]);
• what exists is fetch_paper(reference, email=...) returning a tuple (abstract, pdf_url);
• LiteratureFetcher has no PDF download at all — only fetch_unpaywall(doi), which returns a URL. Two of the scripts advertise full-text extraction (snippet_in_fulltext), and one advertises a sci-hub fallback in its docstring.

Porting means writing capability that no working code in this repo has, guided only by drafts that never ran. scripts/cache_fulltext.py already covers "get OA full text into the cache" in a working form. My reading is that deletion is right, but it is a judgement about intent that belongs with whoever wrote them, so #410 stays open for it.

What is fixed is that nothing working depends on them any more, which is what made the breakage reachable.

2319 passed, 16 skipped. ruff, black, mypy src/ clean.

🤖 Generated with Claude Code

[realmarcin]

Review round 1 — addressed

Both HIGH findings landed, and both undercut the first version rather than refining it.

1 (HIGH) — the -1 sentinel was corrupted by its only caller. Fixed.

This is the one that mattered. I introduced -1 to mean "could not validate", wrote a docstring saying "a caller must distinguish that from a clean file", and then the only caller did initial["total"] - final["total"] on it:

✅ AMD_Acidophile_Heterotroph_Network.yaml
   Issues: 0 → -1 (fixed 1)
🎉 Total issues fixed across all files: 1

A green tick, a fabricated fix, and that fabricated 1 summed into the batch total. I had moved the never-ran-but-looks-clean defect one frame up rather than removing it — which is a worse outcome than leaving it, because the sentinel makes it look handled.

Now: a validation_failed status with its own ⚠️ icon, no arithmetic on the sentinel, unverified files excluded from the total and named explicitly rather than left to be inferred from a smaller number. Your exact reproduction:

⚠️ Richmond_Mine_AMD_Biofilm.yaml
   Issues: 0 → could not validate (fixed: unknown — the validator could not run, so this file is unverified)

FABRICATED 'fixed 1'? -> False
green tick on the unvalidated file? -> False

2 (HIGH) — validate_file was unreachable, so my framing was wrong. Fixed.

Correct, and I should have checked. --no-validate existed with no --validate beside it, and set_defaults(validate=False) pinned it False whatever was passed — since 7c658e6, the same commit that introduced the broken subprocess.

So the PR body's "what this fixes is the part that was actually reachable" was exactly backwards, and the genuinely reachable bug was the flag itself: an advertised choice the parser did not offer. --validate now exists as a mutually exclusive pair with --no-validate, default still off, with the cost you measured stated in the help text (~1.3 s/call, twice per file, ~13 min over 312 files — which is why it was switched off, so re-enabling it wholesale still needs batching).

[]               -> validate_after=False
['--validate']   -> validate_after=True
['--no-validate']-> validate_after=False

3 (MEDIUM) — the AST guard covered 3 of 5 scripts and 2 call shapes. Fixed.

It now reads _KNOWN_BROKEN from tests/test_scripts_import.py rather than keeping a second hand-maintained copy — two lists drift, which is exactly how test_pdf_fetching and extract_evidence_snippets were missed — and covers os.system, sys.stdout.write, and logger calls. Re-running your nine mutations, all five that previously escaped are caught:

print(...)                     ✅   os.system(...)                 ✅
sys.stdout.write(...)          ✅   print("... test_pdf_fetching") ✅
print("... extract_evidence_snippets") ✅

7 (LOW) — slow was inert. Fixed, and it fixes 6 too.

slow is unregistered here, so it warned and deselected nothing. The discriminating test is now e2e, which addopts already deselects. That also answers finding 6: it runs linkml-reference-validator, which #417 deliberately keeps out of just qc, and its "no network" property holds only because this record's references happen to be cached — not by construction. Default run: 3 passed, 1 deselected. Explicit: uv run pytest -m e2e → 1 passed.

4, 5 (MEDIUM-LOW) — both fixed.

warnings: 0 claimed a split the tool does not report; the docstring explained the Total checks line while the regex parses Issues found.

8 — recorded, not fixed.

A well-formed YAML of the wrong shape still validates clean. That is the same defect class one layer down inside the validator itself, and out of scope here.

On (c) and (i)

Useful confirmations, thank you: batch_snippet_fixer.py is importable and runnable, so this is not polishing an unimportable file — it was polishing an unreachable path inside a runnable one, which finding 2 now closes. And your runtime measurement is the reason --validate stays off by default rather than becoming the default now that it works.

---

2318 passed, 16 skipped, 8 deselected. ruff, black, mypy src/ clean.