feat: add consumer/impact-traversal evidence to the shared review contract - #82
Merged
Conversation
…tract ## Summary - Bump the shared review result contract to schema `1.2` (additive only): add a `consumer_impact_evidence` array recording, per changed shared symbol whose other call sites/consumers were traversed, the symbol, its defining location, one or more concrete `consumer_search_evidence` entries, and a `disposition` of `all_consumers_consistent`, `inconsistency_found`, or `no_other_consumers`. - `review-suite/scripts/validate.py`: enforce structure and non-emptiness only — the validator does not determine which changed symbols require an entry (that is lens judgment, owned by a later child). A disposition claiming other consumers were found (`all_consumers_consistent`, `inconsistency_found`) requires search evidence covering more than the changed symbol's own location; `no_other_consumers` requires at least one concrete search. Only `correctness` or `aggregate` results may carry this evidence. Extend the stale-schema-version rejection so a v1.1 result now fails with a useful migration error, mirroring the existing v1.0 rejection. - Add a new canonical fixture, `consumer-impact-traversal`, modeling a sanitized version of the baseline's `dependency-strictness-propagation` miss (a changed shared helper with a sibling call site, plus a second changed private helper used from exactly one call site), covering all 5 required fixture/test scenarios in `review-suite/scripts/tests/test_contracts.py`. - Migrate every canonical fixture, eval helper, and each bundled skill's standalone eval fixtures to schema 1.2; refresh the four bundled `review-suite` copies via `just sync-contracts`. ## Why - #52 (narrowed by #59's decision record): the baseline's `dependency-strictness-propagation` case shows 5 of 5 attempts missed the same sibling call site of a changed shared helper, because nothing in the contract required a reviewer to record whether other call sites were traversed. This ticket owns only the schema and validator for that evidence; it does not add the lens behavior that populates it (#53), and it does not add the changed-surface ledger, acceptance trace, or risk profile #59 explicitly dropped as unsupported by baseline evidence. ## Verification - `just format`, `just lint`, and `just test` all pass (234 tests in `review-suite/scripts/tests`, up from 227; 6 new consumer-impact-evidence tests plus one new stale-v1.1 rejection test). - `just sync-contracts` leaves all four bundled `review-suite` copies byte-identical to the canonical source (verified by `test_bundled_contracts.py` and a direct `cmp` check). - No file under `review-suite/evals/baseline/v1/` or `review-suite/evals/v2/` was touched; `review-code-change/SKILL.md`'s orchestration matrix, the correctness rubric, and every lens prompt are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
## Summary - Strengthen `review-suite/CONTRACT.md`, `validate.py`'s `_check_consumer_impact_evidence` docstring, and its regression test to explain, with concrete reasoning, why the validator never inspects the packet diff to decide whether a changed symbol needed a `consumer_impact_evidence` entry: this validator receives only a packet and a result with no repository checkout to search, so it cannot itself determine whether a changed symbol has other call sites, and adding that determination here would be the independent correctness explorer and static call-graph tooling #52's non-goals rule out. ## Why - A fresh `review-code-change` pass on the prior candidate raised a blocking correctness finding proposing that `validate_pair` be extended to detect, from the diff, whether a changed symbol's sibling call site was also covered. That proposed mechanism is exactly the independent correctness-explorer / static-call-graph tooling #52's own non-goals forbid, and #52's own text assigns "which changed symbols require an entry" to "the correctness lens's own traversal pass (#53)" — a mechanism that does not exist yet in this candidate — not to this validator. The real baseline miss this evidence exists to surface (`dependency-strictness-propagation`) involved a sibling call site the diff never touched, which only live repository access (available to the reviewing agent, never to this validator) can find; a diff-only heuristic would not even correctly model that failure mode. Declining to build that mechanism is therefore correct, but the prior candidate's reasoning for the boundary was implicit; this revision makes it explicit and self-evident from the artifacts themselves. ## Verification - `just format`, `just lint`, and `just test` all pass (234 tests in `review-suite/scripts/tests`, unchanged in count from the prior commit — this revision only strengthens documentation and comments, no schema, validator behavior, or fixture changed). - `just sync-contracts` leaves all four bundled `review-suite` copies byte-identical to the canonical source. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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.
Summary
1.2(additive only): adda
consumer_impact_evidencearray recording, per changed shared symbolwhose other call sites/consumers were traversed, the symbol, its defining
location, one or more concrete
consumer_search_evidenceentries, and adispositionofall_consumers_consistent,inconsistency_found, orno_other_consumers.review-suite/scripts/validate.py: enforce structure and non-emptinessonly. Only
correctnessoraggregateresults may carry this evidence. Adisposition claiming other consumers were found
(
all_consumers_consistent,inconsistency_found) requires searchevidence covering more than the changed symbol's own location;
no_other_consumersrequires at least one concrete search. The validatordeliberately does not determine which changed symbols require an entry —
it receives only a packet and a result, with no repository checkout to
search, so it cannot itself decide whether a changed symbol has other call
sites; that judgment belongs to the lens performing the traversal (Add correctness traversal and verification-sufficiency passes #53,
not yet implemented). Extend the stale-schema-version rejection so a v1.1
result now fails with a useful migration error, mirroring the existing
v1.0 rejection.
consumer-impact-traversal, modeling asanitized version of the baseline's
dependency-strictness-propagationmiss, covering all 5 required fixture/test scenarios in
review-suite/scripts/tests/test_contracts.py.standalone eval fixtures to schema 1.2; refresh the four bundled
review-suitecopies viajust sync-contracts.CONTRACT.md, thevalidator docstring, and a dedicated regression test, after a first
review-code-changepass raised — and a second, clean pass confirmedresolved — the question of whether the validator should itself detect
missing traversal from the diff (it should not: that would be exactly the
independent correctness explorer / static call-graph tooling this
ticket's non-goals rule out).
Why
dependency-strictness-propagationcase shows 5 of 5 attempts missed thesame sibling call site of a changed shared helper, because nothing in the
contract required a reviewer to record whether other call sites were
traversed. This ticket owns only the schema and validator for that
evidence; it does not add the lens behavior that populates it (Add correctness traversal and verification-sufficiency passes #53), and
it does not add the changed-surface ledger, acceptance trace, or risk
profile Use the v1 baseline to finalize and preregister the review v2 graph #59 explicitly dropped as unsupported by baseline evidence.
Acceptance criteria (from #52's body)
consumer_impact_evidencearraydescribed in the ticket; no changed-surface ledger, acceptance trace,
or risk profile is added.
accepts a concretely-evidenced
no_other_consumersdispositionotherwise (structure and non-emptiness only — see the CONTRACT.md
"Consumer/impact evidence" section for the validator's scope boundary
and why determining which symbols require an entry is lens
judgment, not validator judgment).
covered by
ConsumerImpactEvidenceTestsintest_contracts.py).just sync-contracts.documents (both 1.0 and now 1.1) fail with a useful error.
just format,just lint, andjust testpass.transition (CHANGELOG updated; root README does not document schema
versions, matching Make clean verdicts require passing validation and current-head lens evidence #51's own precedent).
ticket.
Test plan
just format— all checks passed, no reformatting neededjust lint— all checks passed (skills-ref validation of all 7skills, plugin packaging validation)
just test— 234 tests inreview-suite/scripts/tests(up from 227),plus every skill's own tests, all OK
just sync-contracts— all four bundledreview-suitecopiesbyte-identical to canonical source (verified by
test_bundled_contracts.pyand a directcmpcheck)review-code-changesuite (2 rounds;round 1 raised one correctness finding proposing the validator detect
missing traversal from the diff — declined with evidence as exactly
the independent-explorer/static-call-graph machinery this ticket's
non-goals forbid, and strengthened documentation instead; round 2
clean across solution-simplicity, correctness, and code-simplicity)
Scope note
This PR does not touch
review-suite/evals/baseline/v1/orreview-suite/evals/v2/(frozen artifacts from #58/#59), does not touchreview-code-change/SKILL.md's orchestration matrix, the correctnessrubric, or any lens prompt, and does not add lens behavior that populates
consumer_impact_evidence— that is #53's scope.Fixes #52