feat: add correctness traversal and verification-sufficiency passes - #83
Merged
Merged
Conversation
## Summary - Bump the shared review result contract to schema `1.3` (additive only): add a `verification_sufficiency_evidence` array recording, per claimed validation command or test touching a materially risky change, the `claimed_test_or_command`, whether it `exercises_material_risk` (`yes`, `no`, or `not_applicable`), and `reasoning`. - `review-suite/scripts/validate.py`: enforce structure and a clean-verdict consistency rule — unlike `consumer_impact_evidence`, an entry recording `exercises_material_risk: "no"` cannot pair with a `clean` verdict, since that would silently hide the exact gap this evidence exists to surface. Extend the stale-schema-version rejection so a v1.2 result now fails with a useful migration error, mirroring the existing v1.0/v1.1 rejections. - `skills/review-correctness/SKILL.md`: add the two required passes from #53's body as dimensions of this single lens — a consumer/impact-traversal pass that populates #52's `consumer_impact_evidence`, and a verification-sufficiency pass that populates the new evidence array — and explicitly rule out routing to any specialist module. - Add a new canonical fixture, `verification-sufficiency-guard`, modeled on the baseline's `stale-claim-release-guard` miss (a release guard whose only claimed test exercises an already-safe branch instead of the actual owner-absent triggering condition), covering the required fixture/consistency scenarios in `review-suite/scripts/tests/test_contracts.py`. - Add two real-runtime forward-evaluation scenarios under `skills/review-correctness/evals/` (`standalone-sibling-call-site-traversal` and `standalone-verification-sufficiency-gap`), modeled on both frozen v1 baseline misses, plus fixture-conformance and no-specialist-module tests in `skills/review-correctness/scripts/tests/test_skill_contract.py`. - Migrate every canonical fixture, eval helper, and each bundled skill's standalone eval fixtures to schema 1.3; refresh the four bundled `review-suite` copies via `just sync-contracts`. ## Why - #59's decision record found both baseline misses in `s1-correctness-orchestrator` confident (zero ambiguity, 5-for-5 across independent attempts): a missed sibling call site of a changed permissive/strict helper, and a verification-sufficiency gap where the added test exercised an already-safe branch. #52 supplied the `consumer_impact_evidence` schema shape; this ticket supplies the lens behavior that populates and reasons from it, plus the new verification-sufficiency evidence array and pass. Specialist routing is dropped entirely per #59's decision record — no baseline evidence supports it, and reintroducing it later requires its own decision-record entry. ## Verification - `just format`, `just lint`, and `just test` all pass (241 tests in `review-suite/scripts/tests`, unchanged in count from the prior commit; 8 tests in `skills/review-correctness/scripts/tests`, up from 3). - `just sync-contracts` leaves all four bundled `review-suite` copies byte-identical to the canonical source. - `python3 review-suite/scripts/evals/audit_corpus.py` passes for all 7 corpora (no case-identifier leakage into reviewer-visible contract text). - Real-runtime replay: a fresh, isolated agent given only the bundled skill/contract text and each eval's raw evidence (no access to the expected-result answer key) correctly reproduced the required verdict on both new standalone evals, twice each, and left an existing clean fixture (`repository-convention-clean`) clean under both new passes with no spurious findings. - No file under `review-suite/evals/baseline/v1/` or `review-suite/evals/v2/` was touched; #51's `lens_executions` and #52's `consumer_impact_evidence` behavior are unchanged and covered by their existing tests, which still pass. No specialist module (security, concurrency-as-a-context, compatibility/migration, operations, or UI) exists anywhere in `review-correctness`. 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.
Fixes #53
Summary
1.3(additive only): adda
verification_sufficiency_evidencearray recording, per claimedvalidation command or test touching a materially risky change, the
claimed_test_or_command, whether itexercises_material_risk(yes,no, ornot_applicable), andreasoning.review-suite/scripts/validate.py: enforce structure and a clean-verdictconsistency rule — an entry recording
exercises_material_risk: "no"cannot pair with a
cleanverdict, since that would silently hide the exactgap this evidence exists to surface. Extend the stale-schema-version
rejection so a v1.2 result now fails with a useful migration error.
skills/review-correctness/SKILL.md: add the two required passes fromAdd correctness traversal and verification-sufficiency passes #53's body as dimensions of this single lens — a consumer/impact-traversal
pass that populates Add consumer/impact-traversal evidence to the shared review contract #52's
consumer_impact_evidence, and averification-sufficiency pass that populates the new evidence array — and
explicitly rule out routing to any specialist module.
verification-sufficiency-guard, modeled onthe frozen baseline's verification-sufficiency miss (a release guard whose
only claimed test exercises an already-safe branch instead of the actual
owner-absent triggering condition).
skills/review-correctness/evals/modeled on both frozen v1 baselinemisses, plus fixture-conformance and no-specialist-module tests.
standalone eval fixtures to schema 1.3; refresh the four bundled
review-suitecopies viajust sync-contracts.Why
s1-correctness-orchestratorconfident (zero ambiguity, 5-for-5 acrossindependent attempts): a missed sibling call site of a changed
permissive/strict helper, and a verification-sufficiency gap where the
added test exercised an already-safe branch. Add consumer/impact-traversal evidence to the shared review contract #52 supplied the
consumer_impact_evidenceschema shape; this ticket supplies the lensbehavior that populates and reasons from it, plus the new
verification-sufficiency evidence array and pass. Specialist routing is
dropped entirely per Use the v1 baseline to finalize and preregister the review v2 graph #59's decision record — no baseline evidence supports
it.
Acceptance criteria (from #53's body)
correct but an unchanged consumer remains incompatible. — covered by
Add consumer/impact-traversal evidence to the shared review contract #52's existing
consumer-impact-traversalfixture/tests plus newreal-runtime eval
standalone-sibling-call-site-traversal.or bypass the material behavior the change addresses. — new
verification-sufficiency-guardfixture plus real-runtime evalstandalone-verification-sufficiency-gap.operational, or UI) exists anywhere in
review-correctnessafter thischange. — verified by
test_no_specialist_module_existsand manualrepository search.
consumer_impact_evidenceandverification_sufficiency_evidencewhere applicable. — SKILL.md nowrequires both; demonstrated by real-runtime replay below.
fixtures migrated and still pass; a fresh agent given only
repository-convention-clean's packet stayed clean with no spuriousfindings under both new passes.
behavior on both target cases. — see Verification below.
context for a later, separate re-scoring ticket, not a gate on this
merge (per the ticket body's own framing). No scoring harness was built.
just format,just lint, andjust testpass.Non-goals honored
review-correctnesswas not turned into an exhaustive checklist.Verification
just format,just lint, andjust testall pass (241 tests inreview-suite/scripts/tests, unchanged in count from the prior commit; 8tests in
skills/review-correctness/scripts/tests, up from 3).just sync-contractsleaves all four bundledreview-suitecopiesbyte-identical to the canonical source.
python3 review-suite/scripts/evals/audit_corpus.pypasses for all 7corpora (no case-identifier leakage into reviewer-visible contract text).
skill/contract text and each eval's raw evidence (no access to the
expected-result answer key) correctly reproduced the required verdict on
both new standalone evals, twice each, and left the existing
repository-convention-cleanfixture clean under both new passes with nospurious findings.
review-suite/evals/baseline/v1/orreview-suite/evals/v2/was touched; Make clean verdicts require passing validation and current-head lens evidence #51's
lens_executionsand Add consumer/impact-traversal evidence to the shared review contract #52'sconsumer_impact_evidencebehavior are unchanged and covered by their existing tests, which still
pass.
review-code-changepass (fresh, read-only, raw-evidence-onlycontext) returned a clean aggregate across all three lenses
(solution-simplicity, correctness, code-simplicity) bound to this exact
head/base.
🤖 Generated with Claude Code