Stop treating msbench-cli extract's exit code as proof it extracted - #1790
Merged
Conversation
`msbench-cli extract` refuses to write into a destination that already exists and is nonempty -- it prints "Quitting to avoid overwriting" on stderr and then exits 0, the same status as a successful extraction. Measured on one run id with only the destination varying: fresh dir -> exit 0, output written; existing nonempty dir -> exit 0, nothing written. Both consumers checked only `result.status !== 0`, so both read a refusal as a success and returned a stale cache as though it were a fresh download. gate-health: a single audit taken while run 2026090413313337 was still in flight cached a partial extraction (run_metadata.json, no instance output, because none existed yet). Every later audit then found no instances, re-invoked extract, got the silent refusal, and reported the run as a permanent READER FAULT -- dropping a real run from the corpus in the direction of "never executed", the loudest verdict the tool has. --refresh could not clear it either, because it extracts into the same nonempty directory and hits the identical refusal. extraction.ts: the same refusal made regrade's --refresh a silent no-op. Verified by pointing resolveExtraction at a nonempty directory with refresh: true -- it returned the directory, the stale marker file survived, and no instance output appeared, with nothing reported. Both now clear their own cache before extracting, which is what makes --refresh mean what it says. A caller-supplied --extract-dir is deliberately not deleted; that case is detected after the call and reported with recovery instructions, since it may point somewhere that must not be removed. The post-extraction check counts `incomplete` alongside `instances` so it fires only when extraction wrote nothing at all. An instance tree that arrives without its session.sqlite is a different failure with its own reporting -- on Windows the 260-character path limit truncates extractions under the repo-nested cache, observed here at 254 characters and 35 files -- and folding that into this message would trade one misdiagnosis for another. Falsification: with the same poisoned cache and the same run id, reverting only the gate-health change reproduces "READER FAULT ... yielded no instances" and loads 0 instances; restoring it loads 1. Across the full corpus the audit goes from 67 runs/67 instances with one dropped to 68/68, while the three runs that genuinely have no instance output are still reported as corpus facts rather than reader faults. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
msbench-cli extractrefuses to write into a destination that already exists and is nonempty -- it printsQuitting to avoid overwritingon stderr and then exits 0, the same status as a successful extraction.Measured on one run id, same CLI, only the destination varying:
Both consumers checked only
result.status !== 0, so both read a refusal as a success and handed back a stale cache as though it were a fresh download.Why it mattered
gate-healthdropped a real run from the corpus. One audit taken while run2026090413313337was still in flight cached a partial extraction --run_metadata.json, no instance output, because none existed yet. Every later audit then found no instances, re-invoked extract, got the silent refusal, and reported the run as a permanentREADER FAULT.--refreshcould not clear it: it re-extracts into the same nonempty directory and hits the identical refusal.That fails in the worst available direction. A dropped run makes gates look never executed, which is this tool's loudest verdict -- so the instrument built to find vacuous passes was itself under-reporting coverage invisibly.
regrade --refreshwas a silent no-op. PointingresolveExtractionat a nonempty directory withrefresh: truereturned the directory, left the stale marker file in place, produced no instance output, and reported nothing:The fix
Both now clear their own cache before extracting, which is what makes
--refreshmean what it says. A caller-supplied--extract-diris deliberately not deleted -- it may point somewhere that must not be removed -- so that case is detected after the call and reported with recovery instructions.The post-extraction check counts
incompletealongsideinstances, so it fires only when extraction wrote nothing at all. An instance tree arriving without itssession.sqliteis a different failure with its own reporting: on Windows the 260-character path limit truncates extractions under the repo-nested cache (observed here at 254 characters and 35 files). Folding that into this message would trade one misdiagnosis for another.All five
resolveExtractionpaths verified against the real CLI:refresh: true--extract-dirnonemptyFalsification
Same poisoned cache, same run id, only the
gate-healthchange reverted:READER FAULTAcross the full corpus the audit goes from 67 runs / 67 instances with one dropped to 68 / 68, while the three runs that genuinely have no instance output are still reported as corpus facts rather than reader faults -- so the fix does not paper over the real cases.
Validation
tsc --noEmitclean -eval:drift16/16 contracts -eval:cor:graders:certify151/151 - eslint cleanNote
The underlying
msbench-clibehaviour -- reportingERRORon stderr while exiting 0 -- is an upstream defect worth fixing there too. This PR defends against it locally and records it in the README, alongside the general rule it is an instance of: an exit code is a claim, not a verification.