CAP-1b: make an excluded source file visible to packet sufficiency - #1830
Merged
Conversation
A packet could rest a proof-bearing claim on a file the index had deliberately refused and still report `Sufficient`. Nothing under `agent/` referenced a coverage reason or an exclusion, so the fact never reached the verdict. The route that was actually open is not the one #1802 named. An explicitly probed excluded path already caps: `exact_path_probe_citation` mints it `eligible_for_sufficiency: Some(false)`, so it lands in `missing_exact_path_claims` and the packet is already `Partial`. The uncapped route is a *required* file-scoped citation — `orchestrator.rs` mints those `Some(!explicit)` — read straight off disk and never probed. So this is driven by cited paths, not probe paths; a probe-side fix leaves the hole untouched. Shaped after EV-78, including its two-enum split: a producer cause for what the lookup did, a consumer cause for what a reader may conclude, joined by one total mapping in contracts so packet and readiness cannot drift. With one deliberate inversion, which is the whole asymmetry. Freshness treats a *missing* observation as unknown-and-capping, because a check that did not run proves nothing. Coverage must not: an empty observation list means no path was checked, which is legitimate and caps nothing. A failed lookup arrives as a per-path `NotEstablished` instead of as an absence. Copying freshness wholesale would make every packet that cites nothing `Partial`; a test pins it, because that simplification looks right. Two things that would have made this silently do nothing: * **Path matching.** Exclusion rows store workspace-relative paths, citations carry absolute ones. A string compare, or `normalize_path_key`, matches on some platforms and never on others — plumbing complete, tests green, nothing ever capping. It goes through `same_workspace_path`. * **Filtering instead of mapping.** The observer returns exactly one observation per requested path, including on lookup failure. A producer that dropped unknown paths would be indistinguishable from one reporting them covered — the defect moved one layer up. The follow-up filter is not optional. Capping flips `terminally_sufficient` false, which is exactly what opens follow-up generation, so the cap alone turns a packet that answered and stopped into one that re-probes a permanently unindexable file every round. Leads arrive from three sources, so they are filtered once after all three have contributed. Mutation-checked: removing the cap clause makes the packet report `Sufficient` over the excluded file while its own gap text says the index never read it. Refs #1802
… causes My own tests set `source_coverage` directly, so nothing exercised the lookup itself — which is exactly where this change could most easily do nothing at all. Exclusion rows store a workspace-relative `normalized_path` while a citation carries whatever the retrieval layer produced, so a string comparison matches on some platforms and silently never matches on others: plumbing complete, every test green, and no packet ever capped. `coverage_observation_matches_an_exclusion_by_path_identity` indexes a real project with an oversized structural source and asks about the same file three ways — workspace-relative, absolute, and `./`-prefixed. Mutation-checked: replacing `same_workspace_path` with an `OsStr` comparison makes `./docs/api.json` report `Indexed`, an excluded file looking covered. It also asserts a file the index *did* cover reports `Indexed`, since a matcher that over-matched would cap every packet in the repository. Separately, "there is no published core to ask" and "the query failed" were both reported as `LookupUnavailable`. They are not the same judgement, and EV-78's split is the reason to keep them apart: the first is a deliberate, recoverable state, the second establishes nothing about anything. `project_unavailable` now maps to `PublicationIncomplete`, which is the variant that existed for it. Refs #1802
…e files Adversarial review found the half of this contract that can actually occur was shipped without a producer. Both refresh gates refuse to publish on any coverage reason except `ParserPartial` — they filter exactly that one out — so `ParserPartial` is the only recorded defect a *served* packet can rest on, and the producer reported such a file `Indexed`. That contradicts this contract's own wording: `Indexed` is defined as "no recorded coverage defect" and `Incomplete` as "the file was indexed but a coverage reason was recorded against it". The `reason` field existed to carry it and was always `None`. The reviewer reproduced it end to end; the fix consults `stored_file_coverage_diagnostics` alongside the exclusion rows. Mutation-checked: stubbing the diagnostics lookup makes a parser-partial file report `Indexed` again. Also removes `tmp_cap_combo_probe.rs` and `tmp_stat_cost_probe.rs`, which were scratch files written into this working tree by concurrent review agents and swept into the previous commit by `git add -A`. They were never mine and are not on `dev`. That is also the explanation for two test failures I could not reproduce earlier: `cargo test --workspace -- --list` shows no test named `probe_live_context_tool_structured_content_vs_published_output_schema` anywhere, because it was one of those agents' temporary probes, running in my tree while my own full-suite run was in flight. Refs #1802
`zz_tmp_structural_cap_probe.rs` is scratch, self-labelled "TEMPORARY probe (delete after)". It was written into this working tree by an adversarial-review agent verifying a claim about stale structural exclusion rows, and `git add -A` carried it into b22a4bb — which merged to `dev` in #1821. It is dead weight in CI and asserts nothing anyone owns. The behaviour it was probing is covered properly by `the_structural_bound_holds_when_the_parser_headroom_is_raised` in `codestory-workspace` and by the store-side publication tests. The general lesson, since this is the second and third instance in one session: `git add -A` is unsafe while other agents can write to the tree. Stage named paths.
`probe_answer_json`, `probe_context_packet_emits_fields_the_schema_forbids`, `tmp_experiment_sufficiency`, `tmp_layout_leaks` and `tmp_experiment_layouts` were written into existing files by adversarial-review agents and carried in by `git add -A`. One is literally commented "TEMPORARY INVESTIGATION TEST - remove before commit." The earlier sweep only caught whole scratch *files*; these live inside files I was legitimately editing, so a file-level check could not see them. Diffing the branch against dev for added `probe_`/`tmp_` functions is what found them.
`CONTEXT_PACKET_SCHEMA` is `additionalProperties: false`, and `context_packet_json` serializes the whole `AgentAnswerDto` — so every optional field on that DTO becomes a violation of the tool's own published output schema the moment it is populated. Nothing compared the two, so it had already happened once: `freshness` has been emitted undeclared since EV-78. `source_coverage` made it twice. Both are declared now, and `the_context_packet_emits_only_fields_its_schema_declares` compares emitted keys against the declared set. It populates the optional fields on purpose — they are `skip_serializing_if`, so a default fixture would pass while a real packet failed, which is exactly how this stayed invisible. Mutation-checked: removing the `source_coverage` declaration fails with "the packet emits [\"source_coverage\"], which its published output schema forbids". Found by adversarial review. Fixing `freshness` here is out of this change's nominal scope, but it is the same one-line defect in the same list, and leaving a known-false published contract in place to keep a diff tidy is the wrong trade. Refs #1802
Two more from adversarial review, both real on an ordinary repository. **The cap keyed on every citation.** `covered_paths` took every `citation.file_path` with no eligibility filter, unlike every other sufficiency consumer. The filesystem appenders mint citations with `eligible_for_sufficiency: false` and a real path — and `.sql` is a structural format, so its effective cap is 1 MiB while `collect_sql_schema_file_candidates` cites any `.sql` up to 1.5 MB. A 1.2 MB `db/structure.sql` is therefore both policy-excluded and cited, and would have capped packets over evidence they were never allowed to rest on. It now filters through `citation_sufficiency_eligible`. Route B, the hole this closes, is eligible by construction, so nothing intended is lost. **The gap sentence asserted a byte overrun that had not happened.** Two disjoint record kinds qualify as exclusions: byte-bound (`observed_size > byte_cap`) and structural-unit-bound (`observed_size <= byte_cap`, unit count over its own cap). The producer populated the sizes for both, so a 300 KB JSON with 3,000 structural units rendered "307200 bytes exceeds the 1048576 byte cap, so the index never read it" — false twice over, since 307200 does not exceed 1048576 and the index did read it. The sizes are now carried only for a genuinely byte-bound row; the rest get the plain sentence, which is true. A number that is wrong is worse than no number, particularly in a gap sentence whose whole job is to tell an operator what to change. Refs #1802
… lead filter CI caught the contract-surface consequence of declaring the two packet fields: `plugins/codestory/generated-mcp-catalog.json` is generated from `CONTEXT_PACKET_SCHEMA` and went stale. Regenerated. Three more from adversarial review: **The lead filter missed repo-cache leads.** `open_next_paths` entries are `packet_display_path` output, which strips a named repository root — `target/repo-cache/repos/axios/lib/core/Axios.js` becomes `lib/core/Axios.js`. Joining the project root back onto that yields a path that does not exist, so the identity comparison said "different file" and the lead survived. The filter was therefore inert for exactly the packets it matters most for. It now compares display form to display form, keeping the identity comparison as the fallback for leads that were never stripped. **Dedup compared strings while everything else compared identity.** `exact_packet_probe_paths` yields a project-relative spelling and a citation carries an absolute one, so both survived and the packet shipped the same gap twice for one file. Deduped by resolved identity now. The one branch that runs before the project root is known keeps a raw-string dedup, because there is nothing to resolve against there. **Restores the `PublicationIncomplete` cause split**, which 9b59b43's message claimed but which is in no commit — a concurrent review agent writing to this tree appears to have reverted the edit between my check and my commit. "No published core to ask" and "the query failed" are not the same judgement. The path-identity test grew two cases the dedup change demands: two spellings of one file must produce one observation, and two distinct files must still produce two. The second is the guard that the dedup did not quietly turn the producer into a filter. Refs #1802
The generalization lint rejected the comment I wrote explaining the display-path filter: I illustrated it with a real path from the evaluation corpus. The lint exists to stop eval-corpus literals reaching product code, and a comment is still product text — a later reader tuning behaviour to the named repository is exactly the over-fitting it guards against. Same explanation, no corpus path. Caught by CI, not by me: I had run the lint before this comment existed and not after.
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.
Closes #1829. #1802 stays open — the polish items (report field, citation
loss_reason, readiness, MCP schema) remain, none of which change a verdict.A packet could rest a proof-bearing claim on a file the index had deliberately
refused and still report
Sufficient.agent/contained zero references tocoverage_reasonor exclusions.The route that was open is not the one the issue named
An explicitly probed excluded path already caps:
exact_path_probe_citationmints
eligible_for_sufficiency: Some(false), so it reachesmissing_exact_path_claimsand the packet is alreadyPartial.The uncapped route is a required file-scoped citation, minted
Some(!explicit)atorchestrator.rs:3084, read straight off disk and neverprobed. This is therefore driven by cited paths, not probe paths — the
probe-side fix, which is where I would have started, leaves it untouched.
Shaped after EV-78, with one deliberate inversion
Two cause enums joined by one total mapping in contracts — a producer cause for
what the lookup did, a consumer cause for what a reader may conclude — so packet
and readiness cannot drift, exactly as
FreshnessUnknownCauseDtodoes.The inversion is the whole asymmetry. Freshness treats a missing
observation as unknown-and-capping, because a check that did not run proves
nothing. Coverage must not: an empty observation list means no path was checked,
which is legitimate and must cap nothing. A failed lookup arrives as a per-path
NotEstablishedinstead of as an absence.Copying freshness wholesale would make every packet that cites nothing
Partial. A reviewer pattern-matching on the precedent will ask for thefresh_index_observation()-style opt-in helper and be wrong;no_observations_cap_nothingpins it.Two ways this could have silently done nothing
carry absolute ones. A string compare — or
normalize_path_key— matches onsome platforms and never on others: plumbing complete, every test green, and
nothing ever capping. It goes through
same_workspace_path.per requested path, including on lookup failure. A producer that dropped
unknown paths would be indistinguishable from one reporting them covered —
the defect moved one layer up.
The follow-up filter is not optional
Capping flips
terminally_sufficientfalse, which is precisely what opensfollow-up generation. The cap alone turns a packet that answered and stopped
into one that re-probes a permanently unindexable file every round. Leads arrive
from three sources, so they are filtered once after all three contribute — my
first attempt filtered only the first source and the test caught it.
Verification
Ten tests. The load-bearing ones:
a_packet_resting_on_an_excluded_file_cannot_be_sufficient— asserts thecontrol is
Sufficientfirst, so it cannot pass vacuously. Removing the capclause reproduces
Sufficientover the excluded file while the gap textalready says the index never read it.
no_observations_cap_nothing— the EV-78 asymmetry.every_status_reaches_a_definite_verdict— fails if a status variant isadded and absorbed into a permissive arm.
an_unnamed_defect_is_still_unprovable— anIncompletewith no reason staystyped rather than defaulting to covered.
an_excluded_file_is_never_offered_as_a_follow_up_lead.cargo test --workspace,cargo clippy --workspace --all-targets,cargo fmt --check, and the generalization lint are clean.🤖 Generated with Claude Code