plugin check --repo reports a file whose extraction died as a successful extraction — the repo leg drops diagnostics entirely #230
Labels
No labels
code-review
correctness
dos
performance
security
severity/high
severity/low
severity/medium
tech-debt
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
h-dv/code-index#230
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found by an external package author while answering a question I asked them about something else. Verified here:
crates/cli/src/plugin.rscontains zero occurrences of the stringdiagnostic. The repo leg reportswalked,extractedandrefused, and never the extraction diagnostics the guest put on the wire.The reproduction
A directory with two
.lgdfiles — one healthy, one a malformed fixture that provably emitsextract.parse_errorand contributes zero facts (graded exhaustively in its own.expected, so this is measured and not inferred):ran clean.2 of 2 extracted. One of those two files did not parse, produced nothing, and said so on the wire — and the probe presents it as a fully successful extraction.Why this is worse than it looks
An empty result is a legitimate state in the reporter's format family: 46 of 272
.lgdfiles carry an empty<sqlstmt>because the caller supplies rows at runtime. A definition that declares nothing is normal there.So "0 facts" and "0 facts because the parse died" are genuinely different answers, and this output renders them identically. The operator cannot tell a package working correctly on sparse input from a package failing on their input.
And it lands on the command whose entire purpose is answering that question.
--repois not "are this package's own fixtures right" — it is what would this package do to MY repository. That is the one place a silent extraction failure must not be silent.The asymmetry that makes this a clear defect rather than a design choice
The fixture leg grades diagnostics exhaustively and reports them well —
plugin checkwill tell you "diagnostic extract.node_limit_reached: emitted, not expected — the table is present, so it is graded exhaustively". The reporter confirmed that path works by mutation (they set a budget to 200, rebuilt, and watched it fire on all three graded fixtures).The repo leg drops them. Same command, same guest, same wire format, opposite treatment. There is no argument in the tree for the difference; it reads as an omission rather than a decision.
The fix
A per-code census line in the same shape as the existing
refusedline, which already proves the reporting shape exists (plugin.rs:2096):Emitted at zero as a measurement, not omitted when empty — the distinction this whole issue is about is between "none reported" and "not looked at", and an absent line cannot express the first.
What a fix must prove
extracted 2 of 2was not wrong, it was incomplete.Mutations
Provenance
The reporter went looking for the diagnostic census after I asked them an unrelated question about whether a corpus count was measured before or after a budget change. They could not find one, tested it directly, and found it absent. Their words: "I would not have looked without your question. That is twice today."
Worth recording as method: this is the second defect this week found not by a review pass but by someone checking whether a number they already believed was actually measured.
Fixed in
31cad14, onmaster.Premise confirmed.
probe_repo'sOk(facts)arm readsymbols/refs/importsoff theValidatedFactsand discardedfacts.diagnostics()— the failure arm of the guest's own report was thrown away at the boundary.crates/cli/src/plugin.rshad zero occurrences ofdiagnostic, so there was nothing to print even if it had been carried.The fix
A
DiagnosticCensusin which the count and the basis it was measured against are one value, so no surface can render one without the other — the same disciplineref_count/name_fallbackalready uses:DiagnosticTally(code + count + first witness)crates/indexer/src/conform.rs:1187DiagnosticCensus— count and basis as one valueconform.rs:1219report_lines()— the only rendering, never emptyconform.rs:1242RepoProbe.diagnostics, unconditionalconform.rs:1307repo_diagnostics_unreadconform.rs:1328Ok(facts)armconform.rs:1507conform.rs:1615crates/cli/src/plugin.rs:2117Design notes worth keeping: the basis is
extracted, notclaimed— a refused file's frame never validated, so its diagnostics died with it, andrepo_diagnostics_unreadstates that whenever the two differ. This is not a reclassification: a file with facts and a diagnostic is stillextracted, and its outcome is unmoved. The existingpackages::facts_diagnosticsis reused so the repo leg and the indexing path share one derivation.Mutations, every one RUN
no \diagnostics` line in:overprobe ran clean/extracted 2 of 2 claimed file(s)` — the issue's reproduction verbatimreport_linesreturns empty whenper_codeis emptyextract.parse_errorrepo_diagnostics_unreadmade unconditionalM4 is the arm that matters: without it, "report everything as failed" would pass.
M0 re-run independently before merge, not taken from the report:
EXIT=101, four tests red with that exact message, source restored bycpand verified identical by md5.A separate finding this turned up
Gutting
grade_diagnostics— the fixture-leg diagnostic grading that #230's whole argument rests on — survives the entirecode-index-packagecrate (42 lib tests) and-p code-index-indexer -p code-index-cli(108okblocks). Exactly one assertion in the tree notices, a 21-second daemon e2e atcrates/daemon/tests/xaml_package_e2e.rs:1644. Nothing in the crate that owns the function did, because every case there asserts that a correct expectation passes — and a comparison that compares nothing passes those too.A unit-level control was added at
crates/package/src/expect.rs:1785(0.00s, no worker binary).Gates: fmt, clippy (host and
x86_64-pc-windows-gnu),cargo test --workspace(326 suites, 3504 passed, 0 failed),cargo doc -D warnings, corpus ratchet (9 pinned repos,baseline.jsonunmoved and never blessed), precision gate 7/7 withphantoms=0and unshrunken POPULATION lines — all exit 0.Manifest.grammaris notOption#227