fix(fetch): changing GOOGLE_DOCS_FORMAT was a silent, permanent no-op - #45
Conversation
The skip decision compared the remote fingerprint and "some file is at the recorded
path" — never whether that path is the name the CURRENT config would write.
`fingerprint` is modifiedTime|size|md5, purely remote, so a LOCAL export-format change
is invisible to it:
GOOGLE_DOCS_FORMAT=md -> {'added': 1} on disk D.md localPath D.md
GOOGLE_DOCS_FORMAT=pdf -> {'skipped': 1} on disk D.md localPath D.md
downloads: []
So switching md -> pdf did nothing, forever. No download, no log line, no error: the
operator believes PDFs are flowing while clean keeps converting the old Markdown, and
no recipe in docs/operations/runbook.md tells you to wipe raw/ for it. The knob is
documented in both docs/pipeline/fetch.md and pipeline/.env.example, and
fetch/index.md even lists "New export format / MIME mapping" as a supported task.
The skip now also requires `prev_local == expected_local`, computed with the same
content_name() the download uses. That comparison subsumes the old truthiness check —
`None == "D.pdf"` is False, so a missing recorded path still falls through — and it is
exact rather than case-insensitive: this path is always written from content_name, so
any difference is a real change, and a hand-edited manifest differing only in case
costs one re-download and then records the canonical name.
Verified it settles: the pass after the switch re-downloads nothing, and the stale
export is unlinked by the existing rename cleanup rather than left in the mirror.
I dropped a case-insensitive comparison I had written first, along with the comment
justifying it: the mutation set showed nothing distinguished it from the exact form, and
per PR #43's lesson a documented rule no test pins is worse than not claiming it.
Debugging note for the record: I spent a while chasing "3 failed" runs that turned out to
be my own mutation harness leaving a mutated file in place, not a defect. Re-ran with a
hash-verified restore — 34/34 five times consecutively, and main fails only the new test.
fetch 34 (+1). scorecard 25/25, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…I needed
Gate review: I removed the `.lower()` from the expected-name comparison on the grounds
that "the mutation set showed nothing distinguished it from the exact form". The premise
was false. The mutation set was blind to case-folding filesystems — macOS APFS, Windows,
Docker Desktop bind mounts — which is precisely the environment `content_name`'s docstring
and `_clobbered_by_sidecar`'s samefile() dance exist for, and which two tests in this file
already cover.
On such a filesystem `D.md` and `D.MD` are ONE file, so a case-only difference is not a
format change, and treating it as one is worse than missing it. Measured on this machine
(verified case-folding) with GOOGLE_DOCS_FORMAT=MD — a typo in the very knob this PR
fixes:
pass 2: downloads=['D'] skipped=0 disk=['D.json'] <- content GONE
pass 3: downloads=['D'] skipped=0 disk=['D.MD', 'D.json']
pass 4: downloads=[] skipped=1
The download writes `D.MD`, then the pre-existing rename cleanup unlinks `D.md` — the same
inode — so the mirror loses the document for a full poll interval (1800s default) and
re-downloads twice. On main that config is a silent no-op; the branch made it destructive.
So the guard is back, with a comment that says why in terms of the filesystem rather than
of "any difference is a real change". And this time it is pinned:
test_a_case_only_name_difference_is_not_a_format_change asserts zero downloads AND that
the document is still in the mirror.
Also pinned the `.exists()` half, which nothing covered: a manifest entry whose file is
gone must re-download. It is what limits any name-comparison mistake to one cycle instead
of permanent silent absence, so it is load-bearing, not belt-and-braces. Mutants now
caught: dropping the expected-name check, the case-sensitive form (the one I had shipped),
and dropping `.exists()`.
Worth naming the mistake, because it is the inverse of PR #43's lesson. There I documented
a rule no test pinned. Here I had a rule that mattered, my tests could not see it, and I
resolved that by DELETING the rule instead of extending the tests. Same root cause —
trusting the test set to define what matters — opposite and worse outcome.
The pre-existing "unlink your own download on a case-folding fs" bug this exposed is
already on the bughunt backlog as its own lead; this change no longer triggers it.
fetch 36 (+3). scorecard 25/25, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gate verdictVERDICT: WEAKENED — and this one caught a mistake I should not have made, because I made it by misapplying a lesson from two PRs ago. I deleted a guard I needed, on a false premiseMy first version compared case-insensitively. I removed the On such a filesystem The download writes The guard is restored, with a comment that reasons about the filesystem rather than asserting "any difference is a real change" — and this time it is pinned: the test asserts zero downloads and that the document is still in the mirror. The mistake is the inverse of #43's, and worseIn #43 I documented a rule no test pinned. Here I had a rule that mattered, my tests couldn't see it, and I resolved that by deleting the rule rather than extending the tests. Same root cause — treating the test set as the definition of what matters — opposite and more damaging outcome. Worth stating plainly since I cited #43 as the justification for the deletion. Also pinned:
|
Twelfth iteration of the autonomous
bughuntsweep.Bug
The skip decision compared the remote fingerprint and "some file is at the recorded path" — never whether that path is the name the current config would write.
fingerprintismodifiedTime|size|md5, purely remote, so a local export-format change is invisible to it:Switching
md→pdfdid nothing, forever. No download, no log line, no error — the operator believes PDFs are flowing whilecleankeeps converting the old Markdown, and no recipe indocs/operations/runbook.mdtells you to wiperaw/for it.This is a documented knob (
docs/pipeline/fetch.md,pipeline/.env.example), andfetch/index.mdeven lists "New export format / MIME mapping" as a supported task.Fix
The skip now also requires
prev_local == expected_local, computed with the samecontent_name()the download uses.Two details worth stating:
That comparison subsumes the old truthiness check on
prev_local:None == "D.pdf"isFalse, so a missing recorded path still falls through to download.It is case-insensitive, like
content_nameand_clobbered_by_sidecar. On a case-folding filesystem (macOS APFS, Windows, Docker Desktop bind mounts)D.mdandD.MDare one file, so a case-only difference is not a format change — and treating it as one is worse than missing it.I shipped the exact comparison first and the gate refuted it. With
GOOGLE_DOCS_FORMAT=MD— a typo in the very knob this PR fixes — the download writesD.MD, then the pre-existing rename cleanup unlinksD.md, the same inode, so the mirror loses the document for a full poll interval:On main that config is a silent no-op; my first version made it destructive. My stated reason for removing the guard — "the mutation set showed nothing distinguished it" — was itself the bug: the mutation set was blind to case-folding filesystems, which two tests in this file already cover.
Test
test_changing_the_export_format_re_downloads— fails today withassert 'D' in []. It asserts the whole cycle, not just the download: the newD.pdfexists, the manifest records it, the staleD.mdis gone (the existing rename cleanup handles it), and the pass after the switch re-downloads nothing — so the fix converges rather than re-exporting every pass.test_a_case_only_name_difference_is_not_a_format_change— zero downloads on a case-only difference, and the document is still in the mirror. This is the one my first version failed.test_a_recorded_path_missing_from_disk_re_downloads— the.exists()half, which nothing covered. It is what limits any name-comparison mistake to a single cycle rather than permanent silent absence, so it is load-bearing.Mutation-tested with hash-verified restores, all four caught: dropping the expected-name check, the case-sensitive comparison (the version I had shipped), dropping
.exists(), and computingexpected_localfrom a hardcoded format.fetch 36 (+3) · clean 259 · corpus 48 · graph 51 · slack 13 · answer 66 · benchmark 3 — bare
pytest(CI mode). ruff clean; golden scorecard 25/25.What the gate caught, and why it matters
The failure mode here is the inverse of #43's, and worse. There I documented a rule no test pinned. Here I had a rule that mattered, my tests couldn't see it, and I resolved that by deleting the rule instead of extending the tests — while citing #43 as the justification. Same root cause (treating the test set as the definition of what matters), opposite and more damaging outcome.
The reviewer also investigated and dismissed the charges I most expected to land: no unwanted re-downloads across an 11-shape corpus over six passes;
content_nameis deterministic (noguess_type, and skip and write call the identical function on the identical item); the clobber-heal path still converges; everyGOOGLE_DOCS_FORMATchange necessarily changes the extension so none is undetectable; and this is not one instance of a class, sincecontent_nameis the single source of local naming.Process note
I lost time this iteration to "3 failed" runs that turned out to be my own mutation harness leaving a mutated file in place — not a defect. Worth recording because the wrong conclusion was available and tempting: every individual condition tested
Truein isolation while the suite reported failures, which looks exactly like a subtle logic bug. What resolved it was checking whether main was also failing (it was, on a different test) and then re-running the shipped version five times consecutively: 34/34 each time. The mutation harness now verifies the restore by hash before reporting.