From 559cbcb63655fdd4f4ba577f2dc21888a6a7a675 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 21:03:57 -0700 Subject: [PATCH] feat: add correctness traversal and verification-sufficiency passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- CHANGELOG.md | 4 +- review-suite/CONTRACT.md | 33 +++++ .../contracts/review-result.schema.json | 19 ++- .../fixtures/auth-regression/expected.json | 2 +- .../fixtures/behavior-bug/expected.json | 2 +- .../fixtures/clean-change/expected.json | 2 +- .../code-simplicity-clean/expected.json | 2 +- .../expected.json | 2 +- .../consumer-impact-traversal/expected.json | 2 +- .../fixtures/duplicated-policy/expected.json | 2 +- .../explicit-tests-preserved/expected.json | 2 +- .../fixtures/imagined-machinery/expected.json | 2 +- review-suite/fixtures/manifest.json | 1 + .../fixtures/missing-evidence/expected.json | 2 +- .../expected.json | 2 +- .../fixtures/missing-test/expected.json | 2 +- .../multi-pass-control-flow/expected.json | 2 +- .../necessary-complexity/expected.json | 2 +- .../repository-convention-clean/expected.json | 2 +- .../fixtures/shared-test-setup/expected.json | 2 +- .../speculative-backfill/expected.json | 2 +- .../unrelated-base-drift/expected.json | 2 +- .../expected.json | 19 +++ .../packet.json | 28 ++++ review-suite/scripts/evals/calibration.py | 2 +- .../scripts/evals/fixture_executor.py | 2 +- review-suite/scripts/tests/test_contracts.py | 121 +++++++++++++++++- .../scripts/tests/test_eval_protocol.py | 4 +- .../scripts/tests/test_eval_runner.py | 6 +- review-suite/scripts/validate.py | 44 ++++++- skills/review-code-change/evals/cases.json | 54 ++++---- .../evals/expectations.json | 22 ++-- .../expected/standalone-clean.result.json | 2 +- .../references/review-suite/CONTRACT.md | 33 +++++ .../review-suite/review-result.schema.json | 19 ++- .../references/review-suite/validate.py | 44 ++++++- .../standalone-duplicated-policy.result.json | 2 +- .../references/review-suite/CONTRACT.md | 33 +++++ .../review-suite/review-result.schema.json | 19 ++- .../references/review-suite/validate.py | 44 ++++++- skills/review-correctness/SKILL.md | 39 ++++++ skills/review-correctness/evals/README.md | 17 ++- ...ne-sibling-call-site-traversal.result.json | 52 ++++++++ .../standalone-ticket-regression.result.json | 2 +- ...e-verification-sufficiency-gap.result.json | 42 ++++++ .../candidate.diff | 16 +++ .../prompt.md | 8 ++ .../repository-evidence.md | 16 +++ .../ticket.md | 11 ++ .../validation.md | 7 + .../candidate.diff | 12 ++ .../prompt.md | 8 ++ .../repository-evidence.md | 14 ++ .../ticket.md | 9 ++ .../validation.md | 6 + .../references/review-suite/CONTRACT.md | 33 +++++ .../review-suite/review-result.schema.json | 19 ++- .../references/review-suite/validate.py | 44 ++++++- .../scripts/tests/test_skill_contract.py | 93 ++++++++++++-- .../standalone-provider-framework.result.json | 2 +- .../untrusted-packet-instruction.result.json | 2 +- .../verified-native-relationship.result.json | 2 +- .../references/review-suite/CONTRACT.md | 33 +++++ .../review-suite/review-result.schema.json | 19 ++- .../references/review-suite/validate.py | 44 ++++++- 65 files changed, 1047 insertions(+), 94 deletions(-) create mode 100644 review-suite/fixtures/verification-sufficiency-guard/expected.json create mode 100644 review-suite/fixtures/verification-sufficiency-guard/packet.json create mode 100644 skills/review-correctness/evals/expected/standalone-sibling-call-site-traversal.result.json create mode 100644 skills/review-correctness/evals/expected/standalone-verification-sufficiency-gap.result.json create mode 100644 skills/review-correctness/evals/standalone-sibling-call-site-traversal/candidate.diff create mode 100644 skills/review-correctness/evals/standalone-sibling-call-site-traversal/prompt.md create mode 100644 skills/review-correctness/evals/standalone-sibling-call-site-traversal/repository-evidence.md create mode 100644 skills/review-correctness/evals/standalone-sibling-call-site-traversal/ticket.md create mode 100644 skills/review-correctness/evals/standalone-sibling-call-site-traversal/validation.md create mode 100644 skills/review-correctness/evals/standalone-verification-sufficiency-gap/candidate.diff create mode 100644 skills/review-correctness/evals/standalone-verification-sufficiency-gap/prompt.md create mode 100644 skills/review-correctness/evals/standalone-verification-sufficiency-gap/repository-evidence.md create mode 100644 skills/review-correctness/evals/standalone-verification-sufficiency-gap/ticket.md create mode 100644 skills/review-correctness/evals/standalone-verification-sufficiency-gap/validation.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 74ceeef..835e6d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,11 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-28 — Added consumer/impact-traversal evidence and required passing validation and current-head lens evidence for a clean review verdict +## 2026-07-28 — Added correctness traversal and verification-sufficiency passes, consumer/impact-traversal evidence, and required passing validation and current-head lens evidence for a clean review verdict +- feat: add correctness traversal and verification-sufficiency passes - feat: add consumer/impact-traversal evidence to the shared review contract + (`8e4fdbdaad8f70751d45f8c2ca87e88288f8ba5b`) - feat: require passing validation and current-head lens evidence for a clean review verdict (`b1e51979628652e4ef60adad44089bf54f4551e7`) diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index 9c2a2a6..dfefe01 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -206,6 +206,39 @@ the same way this contract family already judges any other lens-specific finding (a duplicated-policy or behavior-bug miss is likewise never something this schema-and-structure validator can detect on its own). +### Verification-sufficiency evidence + +A `correctness` or `aggregate` result may record +`verification_sufficiency_evidence`: one entry per claimed validation command or +test that touches a materially risky change, each naming the +`claimed_test_or_command`, whether it `exercises_material_risk` (`yes`, `no`, or +`not_applicable`), and `reasoning` describing what specific triggering condition +was or was not exercised. + +This makes a reviewer's verification-sufficiency judgment machine-checkable: +asking whether a claimed test would actually fail for the specific triggering +condition a change addresses, not merely whether it passes. It closes a baseline +verification-sufficiency miss, where the added test exercised an already-safe +branch (an owned entry) rather than the actual risk (an owner-absent +interleaving), so the passing test proved nothing about the risk it was meant to +cover. + +Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- an entry exists only because a claimed test or command touches a materially + risky change, so `exercises_material_risk: "no"` is itself the gating fact — a + `clean` verdict must not pair with such an entry, because that would silently + hide exactly the gap this evidence exists to surface; and +- `exercises_material_risk: "yes"` or `"not_applicable"` may pair with `clean` + when no other gating finding remains. + +As with consumer/impact evidence, this validator cannot itself decide which +claimed tests required an entry — that judgment belongs to the lens performing +the pass. An omitted `verification_sufficiency_evidence` array remains +schema-valid; completeness of a given pass is judged by forward-testing the +lens's actual output against a fixture's expected result. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/review-suite/contracts/review-result.schema.json b/review-suite/contracts/review-result.schema.json index 0af0e91..22a22e1 100644 --- a/review-suite/contracts/review-result.schema.json +++ b/review-suite/contracts/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.2"}, + "schema_version": {"const": "1.3"}, "lens": { "enum": [ "correctness", @@ -145,6 +145,23 @@ } } }, + "verification_sufficiency_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "claimed_test_or_command", + "exercises_material_risk", + "reasoning" + ], + "properties": { + "claimed_test_or_command": {"type": "string", "minLength": 1}, + "exercises_material_risk": {"enum": ["yes", "no", "not_applicable"]}, + "reasoning": {"type": "string", "minLength": 1} + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/review-suite/fixtures/auth-regression/expected.json b/review-suite/fixtures/auth-regression/expected.json index f7224d8..96c3442 100644 --- a/review-suite/fixtures/auth-regression/expected.json +++ b/review-suite/fixtures/auth-regression/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "correctness", "candidate": { "head_sha": "8181818181818181818181818181818181818181", diff --git a/review-suite/fixtures/behavior-bug/expected.json b/review-suite/fixtures/behavior-bug/expected.json index 4bf4ca6..a75df53 100644 --- a/review-suite/fixtures/behavior-bug/expected.json +++ b/review-suite/fixtures/behavior-bug/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "correctness", "candidate": { "head_sha": "1111111111111111111111111111111111111111", diff --git a/review-suite/fixtures/clean-change/expected.json b/review-suite/fixtures/clean-change/expected.json index ad7c9c1..16f4ba6 100644 --- a/review-suite/fixtures/clean-change/expected.json +++ b/review-suite/fixtures/clean-change/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "5555555555555555555555555555555555555555", diff --git a/review-suite/fixtures/code-simplicity-clean/expected.json b/review-suite/fixtures/code-simplicity-clean/expected.json index 348cfb7..daa5580 100644 --- a/review-suite/fixtures/code-simplicity-clean/expected.json +++ b/review-suite/fixtures/code-simplicity-clean/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c", diff --git a/review-suite/fixtures/complexity-relocating-wrapper/expected.json b/review-suite/fixtures/complexity-relocating-wrapper/expected.json index 5867980..99f6be7 100644 --- a/review-suite/fixtures/complexity-relocating-wrapper/expected.json +++ b/review-suite/fixtures/complexity-relocating-wrapper/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "8989898989898989898989898989898989898989", diff --git a/review-suite/fixtures/consumer-impact-traversal/expected.json b/review-suite/fixtures/consumer-impact-traversal/expected.json index b6f6674..4653923 100644 --- a/review-suite/fixtures/consumer-impact-traversal/expected.json +++ b/review-suite/fixtures/consumer-impact-traversal/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "6262626262626262626262626262626262626262", diff --git a/review-suite/fixtures/duplicated-policy/expected.json b/review-suite/fixtures/duplicated-policy/expected.json index 112261e..d841b85 100644 --- a/review-suite/fixtures/duplicated-policy/expected.json +++ b/review-suite/fixtures/duplicated-policy/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "2222222222222222222222222222222222222222", diff --git a/review-suite/fixtures/explicit-tests-preserved/expected.json b/review-suite/fixtures/explicit-tests-preserved/expected.json index bb0a1f3..3c357f8 100644 --- a/review-suite/fixtures/explicit-tests-preserved/expected.json +++ b/review-suite/fixtures/explicit-tests-preserved/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b", diff --git a/review-suite/fixtures/imagined-machinery/expected.json b/review-suite/fixtures/imagined-machinery/expected.json index 0903583..59d484b 100644 --- a/review-suite/fixtures/imagined-machinery/expected.json +++ b/review-suite/fixtures/imagined-machinery/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "3333333333333333333333333333333333333333", diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json index d4e27ff..bb081e2 100644 --- a/review-suite/fixtures/manifest.json +++ b/review-suite/fixtures/manifest.json @@ -15,6 +15,7 @@ {"name": "missing-simplification-requirements", "packet_valid": true}, {"name": "clean-change", "packet_valid": true}, {"name": "consumer-impact-traversal", "packet_valid": true}, + {"name": "verification-sufficiency-guard", "packet_valid": true}, {"name": "missing-evidence", "packet_valid": false}, {"name": "unrelated-base-drift", "packet_valid": true} ] diff --git a/review-suite/fixtures/missing-evidence/expected.json b/review-suite/fixtures/missing-evidence/expected.json index 0dbeecf..8e87593 100644 --- a/review-suite/fixtures/missing-evidence/expected.json +++ b/review-suite/fixtures/missing-evidence/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "6666666666666666666666666666666666666666", diff --git a/review-suite/fixtures/missing-simplification-requirements/expected.json b/review-suite/fixtures/missing-simplification-requirements/expected.json index 88e5436..efd404f 100644 --- a/review-suite/fixtures/missing-simplification-requirements/expected.json +++ b/review-suite/fixtures/missing-simplification-requirements/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "8686868686868686868686868686868686868686", diff --git a/review-suite/fixtures/missing-test/expected.json b/review-suite/fixtures/missing-test/expected.json index c37956b..b1e482b 100644 --- a/review-suite/fixtures/missing-test/expected.json +++ b/review-suite/fixtures/missing-test/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "correctness", "candidate": { "head_sha": "8282828282828282828282828282828282828282", diff --git a/review-suite/fixtures/multi-pass-control-flow/expected.json b/review-suite/fixtures/multi-pass-control-flow/expected.json index 783d2b2..a153012 100644 --- a/review-suite/fixtures/multi-pass-control-flow/expected.json +++ b/review-suite/fixtures/multi-pass-control-flow/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "8888888888888888888888888888888888888888", diff --git a/review-suite/fixtures/necessary-complexity/expected.json b/review-suite/fixtures/necessary-complexity/expected.json index b8fc73e..6cb74f0 100644 --- a/review-suite/fixtures/necessary-complexity/expected.json +++ b/review-suite/fixtures/necessary-complexity/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "4444444444444444444444444444444444444444", diff --git a/review-suite/fixtures/repository-convention-clean/expected.json b/review-suite/fixtures/repository-convention-clean/expected.json index 336d1bf..1c39f3e 100644 --- a/review-suite/fixtures/repository-convention-clean/expected.json +++ b/review-suite/fixtures/repository-convention-clean/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "correctness", "candidate": { "head_sha": "8383838383838383838383838383838383838383", diff --git a/review-suite/fixtures/shared-test-setup/expected.json b/review-suite/fixtures/shared-test-setup/expected.json index 1ffb9d8..a016a80 100644 --- a/review-suite/fixtures/shared-test-setup/expected.json +++ b/review-suite/fixtures/shared-test-setup/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a", diff --git a/review-suite/fixtures/speculative-backfill/expected.json b/review-suite/fixtures/speculative-backfill/expected.json index c717a9b..3c1d257 100644 --- a/review-suite/fixtures/speculative-backfill/expected.json +++ b/review-suite/fixtures/speculative-backfill/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "8585858585858585858585858585858585858585", diff --git a/review-suite/fixtures/unrelated-base-drift/expected.json b/review-suite/fixtures/unrelated-base-drift/expected.json index 95fabfa..07f9756 100644 --- a/review-suite/fixtures/unrelated-base-drift/expected.json +++ b/review-suite/fixtures/unrelated-base-drift/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "7777777777777777777777777777777777777777", diff --git a/review-suite/fixtures/verification-sufficiency-guard/expected.json b/review-suite/fixtures/verification-sufficiency-guard/expected.json new file mode 100644 index 0000000..5bb30b1 --- /dev/null +++ b/review-suite/fixtures/verification-sufficiency-guard/expected.json @@ -0,0 +1,19 @@ +{ + "schema_version": "1.3", + "lens": "correctness", + "candidate": { + "head_sha": "7373737373737373737373737373737373737373", + "comparison_base_sha": "3737373737373737373737373737373737373737" + }, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "verification_sufficiency_evidence": [ + { + "claimed_test_or_command": "tests/test_claims.py::test_release_denied_when_owner_absent_and_not_expired", + "exercises_material_risk": "yes", + "reasoning": "Sets snapshot owners to {} (owner absent) and constructs the claim with expired=False, exactly the triggering condition the new `owner is None and not claim.is_expired()` guard clause addresses, and asserts release is still denied." + } + ], + "next_action": "No material review action is required." +} diff --git a/review-suite/fixtures/verification-sufficiency-guard/packet.json b/review-suite/fixtures/verification-sufficiency-guard/packet.json new file mode 100644 index 0000000..2da32c5 --- /dev/null +++ b/review-suite/fixtures/verification-sufficiency-guard/packet.json @@ -0,0 +1,28 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/coordinator", "base_branch": "main"}, + "candidate": { + "head_sha": "7373737373737373737373737373737373737373", + "comparison_base_sha": "3737373737373737373737373737373737373737", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/lib/claims.py b/lib/claims.py\n--- a/lib/claims.py\n+++ b/lib/claims.py\n@@ -1,6 +1,8 @@\n def release_if_stale(claim, snapshot):\n owner = snapshot.get_owner(claim.resource_id)\n if owner is not None and owner != claim.holder:\n return False\n+ if owner is None and not claim.is_expired():\n+ return False\n claim.release()\n return True\ndiff --git a/tests/test_claims.py b/tests/test_claims.py\n--- a/tests/test_claims.py\n+++ b/tests/test_claims.py\n@@ -1,4 +1,10 @@\n def test_release_denied_for_mismatched_owner():\n claim = Claim(holder=\"worker-a\", resource_id=\"r1\")\n snapshot = FakeSnapshot(owners={\"r1\": \"worker-b\"})\n assert release_if_stale(claim, snapshot) is False\n+\n+\n+def test_release_denied_when_owner_absent_and_not_expired():\n+ claim = Claim(holder=\"worker-a\", resource_id=\"r1\", expired=False)\n+ snapshot = FakeSnapshot(owners={})\n+ assert release_if_stale(claim, snapshot) is False\n" + } + }, + "change_contract": { + "goal": "Prevent release_if_stale from releasing a claim whose snapshot owner is transiently absent unless the claim has actually expired.", + "acceptance_criteria": ["release_if_stale denies release when the snapshot owner is absent and the claim has not expired.", "release_if_stale still denies release when a different owner holds the claim."], + "non_goals": ["Change how or when a claim expires."], + "preserved_behaviors": ["release_if_stale still denies release when the snapshot owner does not match the claim holder."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [{"label": "Claim release guard", "location": "docs/CLAIMS.md", "summary": "A stale claim may only be released once its snapshot ownership is confirmed absent or the claim itself has expired."}], + "nearby_patterns": [] + }, + "validation": [ + {"name": "claims tests", "command": "pytest tests/test_claims.py", "scope": "focused", "status": "passed", "result": "2 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "41 passed"} + ] +} diff --git a/review-suite/scripts/evals/calibration.py b/review-suite/scripts/evals/calibration.py index 0fb2a3b..7be1a44 100644 --- a/review-suite/scripts/evals/calibration.py +++ b/review-suite/scripts/evals/calibration.py @@ -88,7 +88,7 @@ def kinds(self) -> frozenset[str]: def probe_result(probe: dict[str, Any], candidate: dict[str, Any]) -> dict[str, Any]: """Render one probe as a review result bound to the case's candidate.""" return { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": candidate, "verdict": probe["verdict"], diff --git a/review-suite/scripts/evals/fixture_executor.py b/review-suite/scripts/evals/fixture_executor.py index 15a30b3..f498c5c 100644 --- a/review-suite/scripts/evals/fixture_executor.py +++ b/review-suite/scripts/evals/fixture_executor.py @@ -88,7 +88,7 @@ def _clean_lens_executions(candidate: dict[str, Any]) -> list[dict[str, Any]]: def _review(candidate: dict[str, Any], verdict: str, findings: list[dict[str, Any]]): result = { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": dict(candidate), "verdict": verdict, diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index fa53b96..8a4ea9e 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -416,7 +416,8 @@ def test_unrelated_base_drift_remains_a_valid_clean_pair_at_v1_1(self): class StaleSchemaVersionTests(unittest.TestCase): """#51 item 11 and #52 item 5: a stale result is rejected with a useful error rather than silently reinterpreted as newer evidence. Extended by - #52 to also reject a stale v1.1 aggregate now that schema 1.2 exists.""" + #52 to also reject a stale v1.1 aggregate now that schema 1.2 exists, and + by #53 to reject a stale v1.2 aggregate now that schema 1.3 exists.""" def test_stale_v1_0_aggregate_result_is_rejected_with_a_useful_error(self): result = load(ROOT / "fixtures" / "clean-change" / "expected.json") @@ -434,6 +435,14 @@ def test_stale_v1_1_aggregate_result_is_rejected_with_a_useful_error(self): self.assertTrue(any("stale v1.1" in error for error in errors)) self.assertTrue(any("1.2" in error for error in errors)) + def test_stale_v1_2_aggregate_result_is_rejected_with_a_useful_error(self): + result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + result["schema_version"] = "1.2" + errors = VALIDATOR.validate_result(result) + self.assertTrue(errors) + self.assertTrue(any("stale v1.2" in error for error in errors)) + self.assertTrue(any("1.3" in error for error in errors)) + class ConsumerImpactEvidenceTests(unittest.TestCase): """#52: `consumer_impact_evidence` records a reviewer's traversal to other @@ -527,5 +536,115 @@ def test_only_correctness_or_aggregate_results_may_include_the_evidence(self): ) +class VerificationSufficiencyEvidenceTests(unittest.TestCase): + """#53: `verification_sufficiency_evidence` records whether a claimed test + or command actually exercises the specific triggering condition a change + addresses, not merely whether it passes. Modeled on a baseline + verification-sufficiency miss, where the added test exercised an + already-safe branch (a present, matching owner) instead of the actual risk + (an absent snapshot owner paired with a not-yet-expired claim).""" + + def setUp(self): + self.packet = load( + ROOT / "fixtures" / "verification-sufficiency-guard" / "packet.json" + ) + self.result = load( + ROOT / "fixtures" / "verification-sufficiency-guard" / "expected.json" + ) + + def test_test_exercising_the_triggering_condition_is_valid_clean(self): + # Required fixture: the claimed test sets the snapshot owner absent + # and the claim unexpired -- the actual triggering condition -- and + # `verification_sufficiency_evidence` records `exercises_material_risk: + # yes`. + self.assertEqual([], VALIDATOR.validate_pair(self.packet, self.result)) + + def test_no_risk_exercised_cannot_pair_with_a_silent_clean(self): + # Required fixture: the same guard clause, but the claimed test only + # exercises the already-safe owned-entry branch, so + # `verification_sufficiency_evidence` correctly records + # `exercises_material_risk: no`. That fact must gate the verdict, not + # disappear into a silent `clean`. + entry = self.result["verification_sufficiency_evidence"][0] + entry["claimed_test_or_command"] = ( + "tests/test_claims.py::test_release_denied_for_mismatched_owner" + ) + entry["exercises_material_risk"] = "no" + entry["reasoning"] = ( + "Only sets a mismatched present owner; no test sets the snapshot " + "owner absent, so the new `owner is None and not " + "claim.is_expired()` guard clause is never exercised." + ) + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue( + any( + "exercises_material_risk 'no' contradicts a clean verdict" in error + for error in errors + ) + ) + + def test_no_risk_exercised_is_valid_as_a_gating_finding(self): + # The same unexercised-risk evidence is valid once paired with a + # gating verdict and finding, proving the rule targets the silent + # `clean`, not the evidence shape itself. + entry = self.result["verification_sufficiency_evidence"][0] + entry["claimed_test_or_command"] = ( + "tests/test_claims.py::test_release_denied_for_mismatched_owner" + ) + entry["exercises_material_risk"] = "no" + entry["reasoning"] = ( + "Only sets a mismatched present owner; no test sets the snapshot " + "owner absent, so the new `owner is None and not " + "claim.is_expired()` guard clause is never exercised." + ) + self.result["verdict"] = "changes_required" + self.result["findings"] = [ + { + "id": "correctness.claim-release-guard-unverified", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "A claimed test covering a materially risky change must exercise the actual triggering condition it addresses.", + "evidence": [ + { + "location": "lib/claims.py:5", + "detail": "The new guard clause only applies when `owner is None`, but no test constructs a snapshot with an absent owner.", + } + ], + "concern": "The added test exercises the pre-existing owner-mismatch branch, not the new owner-absent interleaving.", + "impact": "A claim could be released while its snapshot owner is transiently absent and the claim has not expired, with no test proving the guard denies it.", + "proposed_change": "Add a test that sets the snapshot owner absent and the claim unexpired, and assert release_if_stale still denies release.", + "expected_effect": "The guard clause addressing the owner-absent interleaving is proven by a test that actually exercises it.", + "location": "lib/claims.py:5", + } + ] + self.assertEqual([], VALIDATOR.validate_pair(self.packet, self.result)) + + def test_not_applicable_risk_may_pair_with_clean(self): + entry = self.result["verification_sufficiency_evidence"][0] + entry["exercises_material_risk"] = "not_applicable" + entry["reasoning"] = ( + "This claimed command does not touch the materially risky branch." + ) + self.assertEqual([], VALIDATOR.validate_result(self.result)) + + def test_an_omitted_evidence_array_is_a_lens_judgment_gap_not_a_schema_gap(self): + del self.result["verification_sufficiency_evidence"] + self.assertEqual([], VALIDATOR.validate_result(self.result)) + + def test_only_correctness_or_aggregate_results_may_include_the_evidence(self): + self.result["lens"] = "code_simplicity" + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue( + any( + "only correctness or aggregate results may include " + "verification-sufficiency evidence" in error + for error in errors + ) + ) + + if __name__ == "__main__": unittest.main() diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index 3bbd314..e875295 100644 --- a/review-suite/scripts/tests/test_eval_protocol.py +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -370,7 +370,7 @@ def _valid_response(self, verdict="changes_required"): "expected_effect": "The requirement is met.", } result = { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": candidate, "verdict": verdict, @@ -459,7 +459,7 @@ def test_valid_blocked_result_is_its_own_status(self): "simulation": False, "executor": {"name": "test"}, "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": blocked_case.packet["candidate"]["head_sha"], diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index e2683e8..9176050 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -223,7 +223,7 @@ def test_a_merge_verdict_on_an_incomplete_packet_is_graded_not_failed(self): ' "outcome": "review_result", "simulation": False,\n' ' "executor": {"name": "always-gating"},\n' ' "result": {\n' - ' "schema_version": "1.2", "lens": "aggregate",\n' + ' "schema_version": "1.3", "lens": "aggregate",\n' ' "candidate": request["run"]["candidate"],\n' ' "verdict": "changes_required", "findings": [finding],\n' ' "blocking_reasons": [],\n' @@ -556,7 +556,7 @@ def test_the_adapter_completes_an_evaluation_through_the_protocol(self): ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' "]\n" "result = {\n" - ' "schema_version": "1.2", "lens": "aggregate",\n' + ' "schema_version": "1.3", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' ' "lens_executions": lens_executions,\n' @@ -619,7 +619,7 @@ def _envelope_stub(self, envelope_body: str) -> Path: ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' "]\n" "result = {\n" - ' "schema_version": "1.2", "lens": "aggregate",\n' + ' "schema_version": "1.3", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' ' "lens_executions": lens_executions,\n' diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index 6b054e1..4edecec 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -44,7 +44,7 @@ def _schema_file(name: str) -> Path: # migrated to. Extend this mapping, never overwrite it, on the next additive # schema bump so every prior stale version keeps failing with its own useful # migration error. -STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,6 +241,7 @@ def validate_result(result: dict[str, Any]) -> list[str]: ) errors.extend(_check_consumer_impact_evidence(result)) + errors.extend(_check_verification_sufficiency_evidence(result)) if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) @@ -299,6 +300,47 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: return errors +def _check_verification_sufficiency_evidence(result: dict[str, Any]) -> list[str]: + """Check verification-sufficiency evidence structure and clean consistency. + + #53: `verification_sufficiency_evidence` records, per claimed validation + command or test touching a materially risky change, whether it actually + exercises the specific triggering condition the change addresses + (`exercises_material_risk`) rather than merely whether it passes. This + closes a baseline verification-sufficiency miss, where the added test + exercised an already-safe branch instead of the actual risk. + + Unlike `consumer_impact_evidence`, this validator does enforce one + cross-field rule: an entry only exists because a claimed test or command + touches a materially risky change, so `exercises_material_risk: "no"` is + itself the gating fact — the claimed validation does not prove the risk + is handled. A `clean` verdict paired with such an entry would silently + hide exactly the gap this evidence exists to surface, so it is rejected + here rather than left to lens judgment alone. + """ + errors: list[str] = [] + entries = result.get("verification_sufficiency_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.verification_sufficiency_evidence: only correctness or aggregate " + "results may include verification-sufficiency evidence" + ) + if result.get("verdict") == "clean": + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + if entry.get("exercises_material_risk") == "no": + errors.append( + f"$.verification_sufficiency_evidence[{index}]: " + "exercises_material_risk 'no' contradicts a clean verdict; " + "the claimed test or command does not exercise the material " + "risk the change addresses" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-code-change/evals/cases.json b/skills/review-code-change/evals/cases.json index d876c8e..4236396 100644 --- a/skills/review-code-change/evals/cases.json +++ b/skills/review-code-change/evals/cases.json @@ -5,9 +5,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -16,9 +16,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", "rule": "Remote providers are out of scope.", "evidence": [{"location": "candidate diff", "detail": "A registry, provider API, configuration, and dispatch exist for one local write."}], "concern": "The candidate implements an out-of-scope provider platform.", "impact": "It adds configuration, dispatch, and provider failure modes.", "proposed_change": "Replace the provider platform with one direct local write.", "expected_effect": "Remove the platform, configuration, and provider failure modes."}], "blocking_reasons": [], "next_action": "Redesign the implementation and restart the full review on a new head."}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", "rule": "Remote providers are out of scope.", "evidence": [{"location": "candidate diff", "detail": "A registry, provider API, configuration, and dispatch exist for one local write."}], "concern": "The candidate implements an out-of-scope provider platform.", "impact": "It adds configuration, dispatch, and provider failure modes.", "proposed_change": "Replace the provider platform with one direct local write.", "expected_effect": "Remove the platform, configuration, and provider failure modes."}], "blocking_reasons": [], "next_action": "Redesign the implementation and restart the full review on a new head."}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -27,9 +27,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-conditional-write", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "medium", "rule": "Prefer a direct status update when extra predicates are unnecessary.", "evidence": [{"location": "candidate update", "detail": "The conditional update has more predicates than a direct update."}], "concern": "The predicates may be unnecessary local machinery.", "impact": "They add conditions and a failure path.", "proposed_change": "Remove claim-token and running-state predicates.", "expected_effect": "Fewer predicates and one less failure path."}], "blocking_reasons": [], "next_action": "Continue to correctness before accepting this in-strategy simplification."}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-conditional-write", "source_lens": "solution_simplicity", "disposition": "unsafe", "reason": "The proposed removal would discard the required atomic claim fence; the current candidate already preserves it.", "evidence": [{"location": "candidate conditional update", "detail": "The claim-token and running-state predicates prevent an expired worker from completing another worker's claim."}]}], "next_action": "Reject the unsafe simplification and continue reviewing the unchanged candidate."}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-conditional-write", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "medium", "rule": "Prefer a direct status update when extra predicates are unnecessary.", "evidence": [{"location": "candidate update", "detail": "The conditional update has more predicates than a direct update."}], "concern": "The predicates may be unnecessary local machinery.", "impact": "They add conditions and a failure path.", "proposed_change": "Remove claim-token and running-state predicates.", "expected_effect": "Fewer predicates and one less failure path."}], "blocking_reasons": [], "next_action": "Continue to correctness before accepting this in-strategy simplification."}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-conditional-write", "source_lens": "solution_simplicity", "disposition": "unsafe", "reason": "The proposed removal would discard the required atomic claim fence; the current candidate already preserves it.", "evidence": [{"location": "candidate conditional update", "detail": "The claim-token and running-state predicates prevent an expired worker from completing another worker's claim."}]}], "next_action": "Reject the unsafe simplification and continue reviewing the unchanged candidate."}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -38,9 +38,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-local-policy-service", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Use the existing authorization boundary.", "evidence": [{"location": "policy service", "detail": "A new service duplicates the active-admin rule."}], "concern": "The service duplicates repository policy.", "impact": "It adds a service and another policy owner.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Remove one service and one duplicate policy."}], "blocking_reasons": [], "next_action": "Continue through correctness because this is a bounded in-strategy removal."}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-local-policy-service", "source_lens": "solution_simplicity", "disposition": "compatible", "reason": "Reusing the existing active-admin helper preserves the repository's authorization boundary.", "evidence": [{"location": "repository policy helper", "detail": "require_active_admin implements the same required authorization check."}]}]}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "code-simplicity.reuse-active-admin", "lens": "code_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Reuse repository policy helpers.", "evidence": [{"location": "new actions", "detail": "Two actions repeat the active-admin condition."}], "concern": "The actions duplicate the same repository policy.", "impact": "Three policy definitions can drift.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Reduce three policy definitions to one."}], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-local-policy-service", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Use the existing authorization boundary.", "evidence": [{"location": "policy service", "detail": "A new service duplicates the active-admin rule."}], "concern": "The service duplicates repository policy.", "impact": "It adds a service and another policy owner.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Remove one service and one duplicate policy."}], "blocking_reasons": [], "next_action": "Continue through correctness because this is a bounded in-strategy removal."}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-local-policy-service", "source_lens": "solution_simplicity", "disposition": "compatible", "reason": "Reusing the existing active-admin helper preserves the repository's authorization boundary.", "evidence": [{"location": "repository policy helper", "detail": "require_active_admin implements the same required authorization check."}]}]}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "code-simplicity.reuse-active-admin", "lens": "code_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Reuse repository policy helpers.", "evidence": [{"location": "new actions", "detail": "Two actions repeat the active-admin condition."}], "concern": "The actions duplicate the same repository policy.", "impact": "Three policy definitions can drift.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Reduce three policy definitions to one."}], "blocking_reasons": []} } }, { @@ -49,9 +49,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true, "new_packet_after_code_simplification": true}, "cycle": 2, "phase": "post_code_simplification_full_restart", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -60,9 +60,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true, "new_packet_after_correctness_fix": true}, "cycle": 2, "phase": "post_correctness_fix_full_restart", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -84,9 +84,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [{"id": "solution-simplicity.future-cleanup", "lens": "solution_simplicity", "severity": "defer", "confidence": "high", "rule": "A real legacy path remains outside this ticket.", "evidence": [{"location": "ticket", "detail": "Migration cleanup is explicitly deferred to issue #90."}], "concern": "A compatibility path remains outside active scope.", "impact": "The branch remains until callers migrate.", "proposed_change": "Remove the path in issue #90.", "expected_effect": "Retire one compatibility branch after migration."}], "blocking_reasons": []}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [{"id": "solution-simplicity.future-cleanup", "lens": "solution_simplicity", "severity": "defer", "confidence": "high", "rule": "A real legacy path remains outside this ticket.", "evidence": [{"location": "ticket", "detail": "Migration cleanup is explicitly deferred to issue #90."}], "concern": "A compatibility path remains outside active scope.", "impact": "The branch remains until callers migrate.", "proposed_change": "Remove the path in issue #90.", "expected_effect": "Retire one compatibility branch after migration."}], "blocking_reasons": []}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -95,9 +95,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 3, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "changes_required", "findings": [{"id": "correctness.retry-still-unbounded", "lens": "correctness", "severity": "blocking", "confidence": "high", "rule": "Retries must stop at the configured maximum.", "evidence": [{"location": "candidate diff", "detail": "The third candidate still schedules after the maximum."}], "concern": "Retry scheduling remains unbounded.", "impact": "The required retry limit is violated.", "proposed_change": "Stop scheduling at max_retries.", "expected_effect": "Bound repeated work at the required limit."}], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "changes_required", "findings": [{"id": "correctness.retry-still-unbounded", "lens": "correctness", "severity": "blocking", "confidence": "high", "rule": "Retries must stop at the configured maximum.", "evidence": [{"location": "candidate diff", "detail": "The third candidate still schedules after the maximum."}], "concern": "Retry scheduling remains unbounded.", "impact": "The required retry limit is violated.", "proposed_change": "Stop scheduling at max_retries.", "expected_effect": "Bound repeated work at the required limit."}], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -106,9 +106,9 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.2", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.2", "lens": "code_simplicity", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.3", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.3", "lens": "code_simplicity", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } } ] diff --git a/skills/review-code-change/evals/expectations.json b/skills/review-code-change/evals/expectations.json index 17d917e..77be817 100644 --- a/skills/review-code-change/evals/expectations.json +++ b/skills/review-code-change/evals/expectations.json @@ -8,7 +8,7 @@ ], "stopped_reason": "all_required_lenses_clean", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "1010101010101010101010101010101010101010", @@ -50,7 +50,7 @@ ], "stopped_reason": "solution_strategy_replacement_required", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "2020202020202020202020202020202020202020", @@ -89,7 +89,7 @@ ], "stopped_reason": "unsafe_simplification_rejected_and_remaining_lenses_clean", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "3030303030303030303030303030303030303030", @@ -147,7 +147,7 @@ ], "stopped_reason": "code_simplicity_gating_finding", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "4040404040404040404040404040404040404040", @@ -207,7 +207,7 @@ ], "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "5050505050505050505050505050505050505050", @@ -247,7 +247,7 @@ "observed_sequence": [], "stopped_reason": "missing_required_dependency", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "6060606060606060606060606060606060606060", @@ -266,7 +266,7 @@ "observed_sequence": [], "stopped_reason": "evidence_packet_incomplete", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "7070707070707070707070707070707070707070", @@ -293,7 +293,7 @@ ], "stopped_reason": "all_required_lenses_clean_with_deferred_finding", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "8080808080808080808080808080808080808080", @@ -354,7 +354,7 @@ ], "stopped_reason": "cycle_budget_exhausted_with_correctness_finding", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "9090909090909090909090909090909090909090", @@ -391,7 +391,7 @@ ], "stopped_reason": "untrustworthy_lens_result", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", @@ -416,7 +416,7 @@ ], "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "5151515151515151515151515151515151515151", diff --git a/skills/review-code-change/evals/expected/standalone-clean.result.json b/skills/review-code-change/evals/expected/standalone-clean.result.json index 7265a8d..5e0693b 100644 --- a/skills/review-code-change/evals/expected/standalone-clean.result.json +++ b/skills/review-code-change/evals/expected/standalone-clean.result.json @@ -5,7 +5,7 @@ "code_simplicity" ], "result": { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "aggregate", "candidate": { "head_sha": "1212121212121212121212121212121212121212", diff --git a/skills/review-code-change/references/review-suite/CONTRACT.md b/skills/review-code-change/references/review-suite/CONTRACT.md index 9c2a2a6..dfefe01 100644 --- a/skills/review-code-change/references/review-suite/CONTRACT.md +++ b/skills/review-code-change/references/review-suite/CONTRACT.md @@ -206,6 +206,39 @@ the same way this contract family already judges any other lens-specific finding (a duplicated-policy or behavior-bug miss is likewise never something this schema-and-structure validator can detect on its own). +### Verification-sufficiency evidence + +A `correctness` or `aggregate` result may record +`verification_sufficiency_evidence`: one entry per claimed validation command or +test that touches a materially risky change, each naming the +`claimed_test_or_command`, whether it `exercises_material_risk` (`yes`, `no`, or +`not_applicable`), and `reasoning` describing what specific triggering condition +was or was not exercised. + +This makes a reviewer's verification-sufficiency judgment machine-checkable: +asking whether a claimed test would actually fail for the specific triggering +condition a change addresses, not merely whether it passes. It closes a baseline +verification-sufficiency miss, where the added test exercised an already-safe +branch (an owned entry) rather than the actual risk (an owner-absent +interleaving), so the passing test proved nothing about the risk it was meant to +cover. + +Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- an entry exists only because a claimed test or command touches a materially + risky change, so `exercises_material_risk: "no"` is itself the gating fact — a + `clean` verdict must not pair with such an entry, because that would silently + hide exactly the gap this evidence exists to surface; and +- `exercises_material_risk: "yes"` or `"not_applicable"` may pair with `clean` + when no other gating finding remains. + +As with consumer/impact evidence, this validator cannot itself decide which +claimed tests required an entry — that judgment belongs to the lens performing +the pass. An omitted `verification_sufficiency_evidence` array remains +schema-valid; completeness of a given pass is judged by forward-testing the +lens's actual output against a fixture's expected result. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-code-change/references/review-suite/review-result.schema.json b/skills/review-code-change/references/review-suite/review-result.schema.json index 0af0e91..22a22e1 100644 --- a/skills/review-code-change/references/review-suite/review-result.schema.json +++ b/skills/review-code-change/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.2"}, + "schema_version": {"const": "1.3"}, "lens": { "enum": [ "correctness", @@ -145,6 +145,23 @@ } } }, + "verification_sufficiency_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "claimed_test_or_command", + "exercises_material_risk", + "reasoning" + ], + "properties": { + "claimed_test_or_command": {"type": "string", "minLength": 1}, + "exercises_material_risk": {"enum": ["yes", "no", "not_applicable"]}, + "reasoning": {"type": "string", "minLength": 1} + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-code-change/references/review-suite/validate.py b/skills/review-code-change/references/review-suite/validate.py index 6b054e1..4edecec 100644 --- a/skills/review-code-change/references/review-suite/validate.py +++ b/skills/review-code-change/references/review-suite/validate.py @@ -44,7 +44,7 @@ def _schema_file(name: str) -> Path: # migrated to. Extend this mapping, never overwrite it, on the next additive # schema bump so every prior stale version keeps failing with its own useful # migration error. -STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,6 +241,7 @@ def validate_result(result: dict[str, Any]) -> list[str]: ) errors.extend(_check_consumer_impact_evidence(result)) + errors.extend(_check_verification_sufficiency_evidence(result)) if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) @@ -299,6 +300,47 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: return errors +def _check_verification_sufficiency_evidence(result: dict[str, Any]) -> list[str]: + """Check verification-sufficiency evidence structure and clean consistency. + + #53: `verification_sufficiency_evidence` records, per claimed validation + command or test touching a materially risky change, whether it actually + exercises the specific triggering condition the change addresses + (`exercises_material_risk`) rather than merely whether it passes. This + closes a baseline verification-sufficiency miss, where the added test + exercised an already-safe branch instead of the actual risk. + + Unlike `consumer_impact_evidence`, this validator does enforce one + cross-field rule: an entry only exists because a claimed test or command + touches a materially risky change, so `exercises_material_risk: "no"` is + itself the gating fact — the claimed validation does not prove the risk + is handled. A `clean` verdict paired with such an entry would silently + hide exactly the gap this evidence exists to surface, so it is rejected + here rather than left to lens judgment alone. + """ + errors: list[str] = [] + entries = result.get("verification_sufficiency_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.verification_sufficiency_evidence: only correctness or aggregate " + "results may include verification-sufficiency evidence" + ) + if result.get("verdict") == "clean": + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + if entry.get("exercises_material_risk") == "no": + errors.append( + f"$.verification_sufficiency_evidence[{index}]: " + "exercises_material_risk 'no' contradicts a clean verdict; " + "the claimed test or command does not exercise the material " + "risk the change addresses" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json b/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json index 8197552..b441c02 100644 --- a/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json +++ b/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "code_simplicity", "candidate": { "head_sha": "8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d", diff --git a/skills/review-code-simplicity/references/review-suite/CONTRACT.md b/skills/review-code-simplicity/references/review-suite/CONTRACT.md index 9c2a2a6..dfefe01 100644 --- a/skills/review-code-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-code-simplicity/references/review-suite/CONTRACT.md @@ -206,6 +206,39 @@ the same way this contract family already judges any other lens-specific finding (a duplicated-policy or behavior-bug miss is likewise never something this schema-and-structure validator can detect on its own). +### Verification-sufficiency evidence + +A `correctness` or `aggregate` result may record +`verification_sufficiency_evidence`: one entry per claimed validation command or +test that touches a materially risky change, each naming the +`claimed_test_or_command`, whether it `exercises_material_risk` (`yes`, `no`, or +`not_applicable`), and `reasoning` describing what specific triggering condition +was or was not exercised. + +This makes a reviewer's verification-sufficiency judgment machine-checkable: +asking whether a claimed test would actually fail for the specific triggering +condition a change addresses, not merely whether it passes. It closes a baseline +verification-sufficiency miss, where the added test exercised an already-safe +branch (an owned entry) rather than the actual risk (an owner-absent +interleaving), so the passing test proved nothing about the risk it was meant to +cover. + +Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- an entry exists only because a claimed test or command touches a materially + risky change, so `exercises_material_risk: "no"` is itself the gating fact — a + `clean` verdict must not pair with such an entry, because that would silently + hide exactly the gap this evidence exists to surface; and +- `exercises_material_risk: "yes"` or `"not_applicable"` may pair with `clean` + when no other gating finding remains. + +As with consumer/impact evidence, this validator cannot itself decide which +claimed tests required an entry — that judgment belongs to the lens performing +the pass. An omitted `verification_sufficiency_evidence` array remains +schema-valid; completeness of a given pass is judged by forward-testing the +lens's actual output against a fixture's expected result. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-code-simplicity/references/review-suite/review-result.schema.json b/skills/review-code-simplicity/references/review-suite/review-result.schema.json index 0af0e91..22a22e1 100644 --- a/skills/review-code-simplicity/references/review-suite/review-result.schema.json +++ b/skills/review-code-simplicity/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.2"}, + "schema_version": {"const": "1.3"}, "lens": { "enum": [ "correctness", @@ -145,6 +145,23 @@ } } }, + "verification_sufficiency_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "claimed_test_or_command", + "exercises_material_risk", + "reasoning" + ], + "properties": { + "claimed_test_or_command": {"type": "string", "minLength": 1}, + "exercises_material_risk": {"enum": ["yes", "no", "not_applicable"]}, + "reasoning": {"type": "string", "minLength": 1} + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-code-simplicity/references/review-suite/validate.py b/skills/review-code-simplicity/references/review-suite/validate.py index 6b054e1..4edecec 100644 --- a/skills/review-code-simplicity/references/review-suite/validate.py +++ b/skills/review-code-simplicity/references/review-suite/validate.py @@ -44,7 +44,7 @@ def _schema_file(name: str) -> Path: # migrated to. Extend this mapping, never overwrite it, on the next additive # schema bump so every prior stale version keeps failing with its own useful # migration error. -STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,6 +241,7 @@ def validate_result(result: dict[str, Any]) -> list[str]: ) errors.extend(_check_consumer_impact_evidence(result)) + errors.extend(_check_verification_sufficiency_evidence(result)) if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) @@ -299,6 +300,47 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: return errors +def _check_verification_sufficiency_evidence(result: dict[str, Any]) -> list[str]: + """Check verification-sufficiency evidence structure and clean consistency. + + #53: `verification_sufficiency_evidence` records, per claimed validation + command or test touching a materially risky change, whether it actually + exercises the specific triggering condition the change addresses + (`exercises_material_risk`) rather than merely whether it passes. This + closes a baseline verification-sufficiency miss, where the added test + exercised an already-safe branch instead of the actual risk. + + Unlike `consumer_impact_evidence`, this validator does enforce one + cross-field rule: an entry only exists because a claimed test or command + touches a materially risky change, so `exercises_material_risk: "no"` is + itself the gating fact — the claimed validation does not prove the risk + is handled. A `clean` verdict paired with such an entry would silently + hide exactly the gap this evidence exists to surface, so it is rejected + here rather than left to lens judgment alone. + """ + errors: list[str] = [] + entries = result.get("verification_sufficiency_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.verification_sufficiency_evidence: only correctness or aggregate " + "results may include verification-sufficiency evidence" + ) + if result.get("verdict") == "clean": + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + if entry.get("exercises_material_risk") == "no": + errors.append( + f"$.verification_sufficiency_evidence[{index}]: " + "exercises_material_risk 'no' contradicts a clean verdict; " + "the claimed test or command does not exercise the material " + "risk the change addresses" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-correctness/SKILL.md b/skills/review-correctness/SKILL.md index 2f21984..9f3621a 100644 --- a/skills/review-correctness/SKILL.md +++ b/skills/review-correctness/SKILL.md @@ -59,6 +59,41 @@ to the caller. Do not mechanically emit every category. Follow evidence into the dimensions that can materially affect this candidate. +## Perform the required traversal and verification-sufficiency passes + +Perform both of these passes on every review and record their evidence in the +shared result. They are dimensions of this single correctness lens, not routed +specialist modules; do not build or delegate to a separate security, +concurrency-as-a-context, compatibility/migration, operations, or UI module. + +1. **Consumer/impact-traversal pass.** For each changed shared symbol, public + contract, or behavior toggle, search the repository for other call sites, + importers, or consumers — including ones the candidate diff does not touch. + Inspect material negative space: unchanged consumers or paths that must still + honor the changed contract. Record one `consumer_impact_evidence` entry per + changed symbol that requires one: the `changed_symbol`, its defining + `location`, one or more concrete `consumer_search_evidence` items describing + what the search actually found, and a `disposition` of + `all_consumers_consistent`, `inconsistency_found`, or `no_other_consumers`. A + disposition claiming other consumers exist requires evidence covering more + than the changed symbol's own location. Treat a genuine inconsistency as a + blocking correctness finding; recording the disposition alone does not + satisfy the review. +2. **Verification-sufficiency pass.** For each claimed validation command or + test that touches a materially risky change, ask whether it would actually + fail for the specific triggering condition the change addresses — not merely + whether it passes. Restate the relevant acceptance or preserved-behavior + contract and inspect the changed or added tests before concluding. Detect + mocks, fixtures, stubs, happy-path assertions, or partial-suite selection + that bypass the risky behavior, including a test that exercises an + already-safe branch instead of the actual triggering condition. Record one + `verification_sufficiency_evidence` entry per claimed test or command + inspected under this pass: the `claimed_test_or_command`, whether it + `exercises_material_risk` (`yes`, `no`, or `not_applicable`), and `reasoning` + naming the specific condition exercised or missed. Treat a claimed test that + does not exercise the material risk as a blocking finding; never let it back + a `clean` verdict. + ## Apply the finding threshold Raise a finding only when concrete ticket, code, test, repository, or runtime @@ -104,6 +139,10 @@ lens `correctness`. - Keep deferred findings non-gating. - Include proposal dispositions when the orchestrator supplied simplification proposals, using the shared contract shape. +- Include `consumer_impact_evidence` and `verification_sufficiency_evidence` + entries for every changed symbol or claimed test the two required passes above + examined. Omit an entry only when the pass genuinely found nothing applicable + to record. - Do not add praise, a scorecard, generic resources, or prose outside the shared result. diff --git a/skills/review-correctness/evals/README.md b/skills/review-correctness/evals/README.md index 7ceed8f..ea46850 100644 --- a/skills/review-correctness/evals/README.md +++ b/skills/review-correctness/evals/README.md @@ -1,6 +1,17 @@ # review-correctness evaluations -`standalone-ticket-regression/` holds raw forward-evaluation inputs. The -expected outcome lives in `expected/standalone-ticket-regression.result.json`, -outside the input directory, so a forward-testing reviewer pointed at the input +Each `standalone-*/` directory holds raw forward-evaluation inputs. Every +expected outcome lives outside its input directory, under +`expected/.result.json`, so a forward-testing reviewer pointed at an input directory cannot read the answer key. + +- `standalone-ticket-regression/`: a preserved-behavior regression (duplicate + webhook delivery reapplying an event). +- `standalone-sibling-call-site-traversal/`: #53's consumer/impact-traversal + pass, modeled on a changed shared helper whose default tightens while an + existing sibling call site keeps an explicit permissive override that the diff + never touches. +- `standalone-verification-sufficiency-gap/`: #53's verification-sufficiency + pass, modeled on a release guard whose only claimed test exercises an + already-safe branch instead of the actual owner-absent triggering condition + the change addresses. diff --git a/skills/review-correctness/evals/expected/standalone-sibling-call-site-traversal.result.json b/skills/review-correctness/evals/expected/standalone-sibling-call-site-traversal.result.json new file mode 100644 index 0000000..8ebb210 --- /dev/null +++ b/skills/review-correctness/evals/expected/standalone-sibling-call-site-traversal.result.json @@ -0,0 +1,52 @@ +{ + "schema_version": "1.3", + "lens": "correctness", + "candidate": { + "head_sha": "6767676767676767676767676767676767676767", + "comparison_base_sha": "7676767676767676767676767676767676767676" + }, + "verdict": "changes_required", + "findings": [ + { + "id": "correctness.legacy-importer-keeps-permissive-mode", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "AGENTS.md requires every caller of check_dependency to run under strict verification unless AGENTS.md itself records a dated, reasoned exception for that caller.", + "evidence": [ + { + "location": "repository-evidence.md", + "detail": "lib/legacy_importer.py calls check_dependency(status, mode=\"permissive\") explicitly, and this call site predates the change, is not touched by this diff, and has no exception recorded anywhere in AGENTS.md." + }, + { + "location": "candidate.diff:lib/policy.py", + "detail": "The shared default flips from permissive to strict, but only lib/orchestrator.py's call site is updated to name mode explicitly in this diff." + } + ], + "concern": "lib/legacy_importer.py keeps its own explicit permissive override, so it is unaffected by the tightened shared default.", + "impact": "A dependency status considered unready under the new strict policy still passes silently through lib/legacy_importer.py, defeating the ticket's goal for that caller.", + "proposed_change": "Update lib/legacy_importer.py's call site to pass mode=\"strict\" (or record and justify an explicit, documented opt-out) to match lib/orchestrator.py and the AGENTS.md convention.", + "expected_effect": "Every caller of check_dependency is verified under the new strict default or explicitly and intentionally opted out.", + "location": "lib/legacy_importer.py" + } + ], + "blocking_reasons": [], + "consumer_impact_evidence": [ + { + "changed_symbol": "check_dependency", + "location": "lib/policy.py:1", + "consumer_search_evidence": [ + { + "location": "lib/orchestrator.py:9", + "detail": "Call site updated in this diff to pass mode=\"strict\" explicitly, matching the new default and the AGENTS.md convention." + }, + { + "location": "lib/legacy_importer.py", + "detail": "Repository evidence names this as the other caller of check_dependency, still passing mode=\"permissive\" explicitly; not touched by this diff." + } + ], + "disposition": "inconsistency_found" + } + ], + "next_action": "Update lib/legacy_importer.py's call site to pass mode explicitly before merging." +} diff --git a/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json b/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json index eacaeb0..266f720 100644 --- a/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json +++ b/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "correctness", "candidate": { "head_sha": "8484848484848484848484848484848484848484", diff --git a/skills/review-correctness/evals/expected/standalone-verification-sufficiency-gap.result.json b/skills/review-correctness/evals/expected/standalone-verification-sufficiency-gap.result.json new file mode 100644 index 0000000..940ca5f --- /dev/null +++ b/skills/review-correctness/evals/expected/standalone-verification-sufficiency-gap.result.json @@ -0,0 +1,42 @@ +{ + "schema_version": "1.3", + "lens": "correctness", + "candidate": { + "head_sha": "5959595959595959595959595959595959595959", + "comparison_base_sha": "9595959595959595959595959595959595959595" + }, + "verdict": "changes_required", + "findings": [ + { + "id": "correctness.claim-release-guard-unverified", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "A claimed test covering a materially risky change must exercise the actual triggering condition it addresses, not merely a passing suite.", + "evidence": [ + { + "location": "candidate.diff:lib/claims.py", + "detail": "The new guard clause only applies when owner is None, but repository-evidence.md states no test constructs a snapshot with an absent owner." + }, + { + "location": "repository-evidence.md", + "detail": "The only existing test, test_release_denied_for_mismatched_owner, constructs a snapshot with a present but mismatched owner -- the pre-existing branch, not the new owner-absent branch." + } + ], + "concern": "The claimed passing test suite exercises only the already-safe owner-mismatch branch, never the owner-absent interleaving this change addresses.", + "impact": "A claim could be released while its snapshot owner is transiently absent and the claim has not expired, with no test proving the new guard denies it.", + "proposed_change": "Add a test that constructs a snapshot with an absent owner and an unexpired claim, and assert release_if_stale still denies release.", + "expected_effect": "The guard clause addressing the owner-absent interleaving is proven by a test that actually exercises it.", + "location": "lib/claims.py" + } + ], + "blocking_reasons": [], + "verification_sufficiency_evidence": [ + { + "claimed_test_or_command": "tests/test_claims.py::test_release_denied_for_mismatched_owner", + "exercises_material_risk": "no", + "reasoning": "This test sets a present but mismatched owner; it never sets the snapshot owner absent, so the new `owner is None and not claim.is_expired()` guard clause is never exercised." + } + ], + "next_action": "Add a test that exercises the owner-absent-and-unexpired branch before merging." +} diff --git a/skills/review-correctness/evals/standalone-sibling-call-site-traversal/candidate.diff b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/candidate.diff new file mode 100644 index 0000000..1f27814 --- /dev/null +++ b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/candidate.diff @@ -0,0 +1,16 @@ +diff --git a/lib/policy.py b/lib/policy.py +--- a/lib/policy.py ++++ b/lib/policy.py +@@ -1,4 +1,4 @@ +-def check_dependency(status, mode="permissive"): ++def check_dependency(status, mode="strict"): + if mode == "strict" and status != "ready": + raise DependencyError(status) + return True +diff --git a/lib/orchestrator.py b/lib/orchestrator.py +--- a/lib/orchestrator.py ++++ b/lib/orchestrator.py +@@ -8,2 +8,2 @@ + def finalize_stage(status): +- return check_dependency(status) ++ return check_dependency(status, mode="strict") diff --git a/skills/review-correctness/evals/standalone-sibling-call-site-traversal/prompt.md b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/prompt.md new file mode 100644 index 0000000..a66016f --- /dev/null +++ b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/prompt.md @@ -0,0 +1,8 @@ +Use the `review-correctness` skill to review the candidate described by the raw +evidence in this directory. Read `ticket.md`, `repository-evidence.md`, +`candidate.diff`, and `validation.md`; do not inspect answer keys (stored +outside this directory) or use any prior review conclusion. Reconstruct the +observable change contract from those sources before inspecting the +implementation, including searching the supplied repository evidence for other +consumers of any changed shared symbol. Return only the shared review result +JSON and do not modify any files or repository state. diff --git a/skills/review-correctness/evals/standalone-sibling-call-site-traversal/repository-evidence.md b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/repository-evidence.md new file mode 100644 index 0000000..28302db --- /dev/null +++ b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/repository-evidence.md @@ -0,0 +1,16 @@ +# Repository evidence + +Repository: `example/pipeline` Base branch: `main` Candidate head: +`6767676767676767676767676767676767676767` Comparison base: +`7676767676767676767676767676767676767676` + +`AGENTS.md` requires every caller of `check_dependency` to run under strict +verification; a caller may remain in permissive mode only if AGENTS.md itself +records a dated, reasoned exception for that caller. `lib/policy.py` defines +`check_dependency`. Repository-wide search shows exactly two callers: +`lib/orchestrator.py` and `lib/legacy_importer.py`. `lib/legacy_importer.py` +calls `check_dependency(status, mode="permissive")`, an explicit override that +predates this change, is not touched by this diff, and has no exception recorded +anywhere in `AGENTS.md` — it is simply an old call site nobody has revisited. +Nearby tests in `tests/test_policy.py` assert `check_dependency` raises +`DependencyError` for a non-ready status only when `mode="strict"`. diff --git a/skills/review-correctness/evals/standalone-sibling-call-site-traversal/ticket.md b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/ticket.md new file mode 100644 index 0000000..64db82c --- /dev/null +++ b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/ticket.md @@ -0,0 +1,11 @@ +# Tighten the dependency-check default to strict + +Our shared dependency-check helper defaults to permissive verification, which +lets a caller silently skip readiness checks. Change the shared default to +strict so every caller is checked. A caller may keep permissive verification +only if that exception is explicitly recorded in `AGENTS.md`; do not leave an +unrecorded permissive caller in place. + +Update the orchestrator's call site to pass `mode` explicitly now that the +default is changing, matching the repository's existing convention of naming the +mode at every call site. diff --git a/skills/review-correctness/evals/standalone-sibling-call-site-traversal/validation.md b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/validation.md new file mode 100644 index 0000000..4e2fe0d --- /dev/null +++ b/skills/review-correctness/evals/standalone-sibling-call-site-traversal/validation.md @@ -0,0 +1,7 @@ +# Validation evidence + +- Focused: `pytest tests/test_policy.py` passed, 5 tests. The suite checks + `check_dependency` under both `mode="strict"` and `mode="permissive"`. +- Full: `pytest` passed, 52 tests. + +The candidate diff is complete. The test commands did not modify tracked files. diff --git a/skills/review-correctness/evals/standalone-verification-sufficiency-gap/candidate.diff b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/candidate.diff new file mode 100644 index 0000000..f3aa516 --- /dev/null +++ b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/candidate.diff @@ -0,0 +1,12 @@ +diff --git a/lib/claims.py b/lib/claims.py +--- a/lib/claims.py ++++ b/lib/claims.py +@@ -1,6 +1,8 @@ + def release_if_stale(claim, snapshot): + owner = snapshot.get_owner(claim.resource_id) + if owner is not None and owner != claim.holder: + return False ++ if owner is None and not claim.is_expired(): ++ return False + claim.release() + return True diff --git a/skills/review-correctness/evals/standalone-verification-sufficiency-gap/prompt.md b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/prompt.md new file mode 100644 index 0000000..529c08a --- /dev/null +++ b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/prompt.md @@ -0,0 +1,8 @@ +Use the `review-correctness` skill to review the candidate described by the raw +evidence in this directory. Read `ticket.md`, `repository-evidence.md`, +`candidate.diff`, and `validation.md`; do not inspect answer keys (stored +outside this directory) or use any prior review conclusion. Reconstruct the +observable change contract from those sources before inspecting the +implementation, including asking whether each claimed passing test actually +exercises the specific triggering condition this change addresses. Return only +the shared review result JSON and do not modify any files or repository state. diff --git a/skills/review-correctness/evals/standalone-verification-sufficiency-gap/repository-evidence.md b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/repository-evidence.md new file mode 100644 index 0000000..11b960f --- /dev/null +++ b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/repository-evidence.md @@ -0,0 +1,14 @@ +# Repository evidence + +Repository: `example/coordinator` Base branch: `main` Candidate head: +`5959595959595959595959595959595959595959` Comparison base: +`9595959595959595959595959595959595959595` + +`docs/CLAIMS.md` states that a stale claim may only be released once its +snapshot ownership is confirmed absent or the claim itself has expired. +`lib/claims.py` is the only place `release_if_stale` is defined or called. +`tests/test_claims.py` previously contained one test, +`test_release_denied_for_mismatched_owner`, which constructs a snapshot with a +present owner that does not match the claim's holder. This candidate adds the +new guard clause but does not add or change any other test in +`tests/test_claims.py`. diff --git a/skills/review-correctness/evals/standalone-verification-sufficiency-gap/ticket.md b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/ticket.md new file mode 100644 index 0000000..e6abf9c --- /dev/null +++ b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/ticket.md @@ -0,0 +1,9 @@ +# Stop releasing a claim while its snapshot owner is transiently absent + +`release_if_stale` currently skips its holder-mismatch guard whenever the +snapshot has no recorded owner for a resource, treating an absent owner as proof +the claim is safe to release. That is wrong when the snapshot is simply +incomplete: the claim may still be legitimately held. + +Add a guard clause so that, when the snapshot owner is absent, the claim is +released only if it has actually expired. diff --git a/skills/review-correctness/evals/standalone-verification-sufficiency-gap/validation.md b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/validation.md new file mode 100644 index 0000000..ab028cd --- /dev/null +++ b/skills/review-correctness/evals/standalone-verification-sufficiency-gap/validation.md @@ -0,0 +1,6 @@ +# Validation evidence + +- Focused: `pytest tests/test_claims.py` passed, 1 test. +- Full: `pytest` passed, 41 tests. + +The candidate diff is complete. The test commands did not modify tracked files. diff --git a/skills/review-correctness/references/review-suite/CONTRACT.md b/skills/review-correctness/references/review-suite/CONTRACT.md index 9c2a2a6..dfefe01 100644 --- a/skills/review-correctness/references/review-suite/CONTRACT.md +++ b/skills/review-correctness/references/review-suite/CONTRACT.md @@ -206,6 +206,39 @@ the same way this contract family already judges any other lens-specific finding (a duplicated-policy or behavior-bug miss is likewise never something this schema-and-structure validator can detect on its own). +### Verification-sufficiency evidence + +A `correctness` or `aggregate` result may record +`verification_sufficiency_evidence`: one entry per claimed validation command or +test that touches a materially risky change, each naming the +`claimed_test_or_command`, whether it `exercises_material_risk` (`yes`, `no`, or +`not_applicable`), and `reasoning` describing what specific triggering condition +was or was not exercised. + +This makes a reviewer's verification-sufficiency judgment machine-checkable: +asking whether a claimed test would actually fail for the specific triggering +condition a change addresses, not merely whether it passes. It closes a baseline +verification-sufficiency miss, where the added test exercised an already-safe +branch (an owned entry) rather than the actual risk (an owner-absent +interleaving), so the passing test proved nothing about the risk it was meant to +cover. + +Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- an entry exists only because a claimed test or command touches a materially + risky change, so `exercises_material_risk: "no"` is itself the gating fact — a + `clean` verdict must not pair with such an entry, because that would silently + hide exactly the gap this evidence exists to surface; and +- `exercises_material_risk: "yes"` or `"not_applicable"` may pair with `clean` + when no other gating finding remains. + +As with consumer/impact evidence, this validator cannot itself decide which +claimed tests required an entry — that judgment belongs to the lens performing +the pass. An omitted `verification_sufficiency_evidence` array remains +schema-valid; completeness of a given pass is judged by forward-testing the +lens's actual output against a fixture's expected result. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-correctness/references/review-suite/review-result.schema.json b/skills/review-correctness/references/review-suite/review-result.schema.json index 0af0e91..22a22e1 100644 --- a/skills/review-correctness/references/review-suite/review-result.schema.json +++ b/skills/review-correctness/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.2"}, + "schema_version": {"const": "1.3"}, "lens": { "enum": [ "correctness", @@ -145,6 +145,23 @@ } } }, + "verification_sufficiency_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "claimed_test_or_command", + "exercises_material_risk", + "reasoning" + ], + "properties": { + "claimed_test_or_command": {"type": "string", "minLength": 1}, + "exercises_material_risk": {"enum": ["yes", "no", "not_applicable"]}, + "reasoning": {"type": "string", "minLength": 1} + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-correctness/references/review-suite/validate.py b/skills/review-correctness/references/review-suite/validate.py index 6b054e1..4edecec 100644 --- a/skills/review-correctness/references/review-suite/validate.py +++ b/skills/review-correctness/references/review-suite/validate.py @@ -44,7 +44,7 @@ def _schema_file(name: str) -> Path: # migrated to. Extend this mapping, never overwrite it, on the next additive # schema bump so every prior stale version keeps failing with its own useful # migration error. -STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,6 +241,7 @@ def validate_result(result: dict[str, Any]) -> list[str]: ) errors.extend(_check_consumer_impact_evidence(result)) + errors.extend(_check_verification_sufficiency_evidence(result)) if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) @@ -299,6 +300,47 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: return errors +def _check_verification_sufficiency_evidence(result: dict[str, Any]) -> list[str]: + """Check verification-sufficiency evidence structure and clean consistency. + + #53: `verification_sufficiency_evidence` records, per claimed validation + command or test touching a materially risky change, whether it actually + exercises the specific triggering condition the change addresses + (`exercises_material_risk`) rather than merely whether it passes. This + closes a baseline verification-sufficiency miss, where the added test + exercised an already-safe branch instead of the actual risk. + + Unlike `consumer_impact_evidence`, this validator does enforce one + cross-field rule: an entry only exists because a claimed test or command + touches a materially risky change, so `exercises_material_risk: "no"` is + itself the gating fact — the claimed validation does not prove the risk + is handled. A `clean` verdict paired with such an entry would silently + hide exactly the gap this evidence exists to surface, so it is rejected + here rather than left to lens judgment alone. + """ + errors: list[str] = [] + entries = result.get("verification_sufficiency_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.verification_sufficiency_evidence: only correctness or aggregate " + "results may include verification-sufficiency evidence" + ) + if result.get("verdict") == "clean": + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + if entry.get("exercises_material_risk") == "no": + errors.append( + f"$.verification_sufficiency_evidence[{index}]: " + "exercises_material_risk 'no' contradicts a clean verdict; " + "the claimed test or command does not exercise the material " + "risk the change addresses" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens. diff --git a/skills/review-correctness/scripts/tests/test_skill_contract.py b/skills/review-correctness/scripts/tests/test_skill_contract.py index a13b28f..9de4a0d 100644 --- a/skills/review-correctness/scripts/tests/test_skill_contract.py +++ b/skills/review-correctness/scripts/tests/test_skill_contract.py @@ -41,12 +41,41 @@ def test_skill_uses_shared_contract_and_is_read_only(self): self.assertIn("Do not turn a rejected hypothetical edit", skill) self.assertNotIn("code-review-pro", skill) + def test_skill_requires_the_traversal_and_verification_sufficiency_passes(self): + # #53: the two required passes must be part of this single lens, not + # a routed specialist module. + skill = (SKILL_ROOT / "SKILL.md").read_text() + normalized = " ".join(skill.split()) + self.assertIn("Consumer/impact-traversal pass", normalized) + self.assertIn("Verification-sufficiency pass", normalized) + self.assertIn("consumer_impact_evidence", normalized) + self.assertIn("verification_sufficiency_evidence", normalized) + self.assertIn("not routed specialist modules", normalized) + self.assertIn( + "do not build or delegate to a separate security, " + "concurrency-as-a-context, compatibility/migration, operations, " + "or ui module", + normalized.lower(), + ) + + def test_no_specialist_module_exists(self): + # #53 acceptance criterion: no specialist module (security, + # concurrency-as-a-context, migration, operational, or UI) exists + # anywhere in review-correctness. + for name in ("specialists", "specialist"): + self.assertFalse((SKILL_ROOT / name).exists()) + self.assertFalse((SKILL_ROOT / "references" / name).exists()) + for path in SKILL_ROOT.rglob("*"): + if path.is_dir(): + self.assertNotIn("specialist", path.name.lower(), str(path)) + def test_correctness_fixture_results_conform(self): expectations = { "behavior-bug": "changes_required", "auth-regression": "changes_required", "missing-test": "changes_required", "repository-convention-clean": "clean", + "verification-sufficiency-guard": "clean", } for fixture_name, verdict in expectations.items(): with self.subTest(fixture=fixture_name): @@ -57,24 +86,31 @@ def test_correctness_fixture_results_conform(self): self.assertEqual("correctness", result["lens"]) self.assertEqual(verdict, result["verdict"]) - def test_standalone_eval_uses_raw_evidence(self): - evaluation = SKILL_ROOT / "evals" / "standalone-ticket-regression" + def test_consumer_impact_traversal_fixture_conforms(self): + # This aggregate-lens fixture (from #52) models the sibling-call-site + # traversal this lens must now perform; kept separate from the + # correctness-lens-only expectations above. + fixture = REVIEW_SUITE / "fixtures" / "consumer-impact-traversal" + packet = load(fixture / "packet.json") + result = load(fixture / "expected.json") + self.assertEqual([], VALIDATOR.validate_pair(packet, result)) + self.assertEqual("aggregate", result["lens"]) + self.assertEqual("clean", result["verdict"]) + self.assertTrue(result["consumer_impact_evidence"]) + + def _load_standalone_eval(self, name: str): + evaluation = SKILL_ROOT / "evals" / name prompt = (evaluation / "prompt.md").read_text() evidence = "\n".join( - (evaluation / name).read_text() - for name in ( + (evaluation / filename).read_text() + for filename in ( "ticket.md", "repository-evidence.md", "candidate.diff", "validation.md", ) ) - result = load( - SKILL_ROOT - / "evals" - / "expected" - / "standalone-ticket-regression.result.json" - ) + result = load(SKILL_ROOT / "evals" / "expected" / f"{name}.result.json") # The reviewer-visible input directory must not contain the answer key. self.assertEqual( @@ -83,6 +119,10 @@ def test_standalone_eval_uses_raw_evidence(self): self.assertNotIn("expected", prompt.lower()) self.assertNotIn("change_contract", evidence) self.assertEqual([], VALIDATOR.validate_result(result)) + return result + + def test_standalone_eval_uses_raw_evidence(self): + result = self._load_standalone_eval("standalone-ticket-regression") self.assertEqual("correctness", result["lens"]) self.assertEqual("changes_required", result["verdict"]) self.assertEqual( @@ -93,6 +133,39 @@ def test_standalone_eval_uses_raw_evidence(self): any("idempotency" in finding["id"] for finding in result["findings"]) ) + def test_standalone_sibling_call_site_traversal_finds_the_missed_consumer(self): + # #53 required fixture 1 (real-runtime shape): a changed shared + # helper's default tightens while a sibling call site the diff never + # touches keeps an explicit permissive override. + result = self._load_standalone_eval("standalone-sibling-call-site-traversal") + self.assertEqual("correctness", result["lens"]) + self.assertEqual("changes_required", result["verdict"]) + self.assertTrue( + any( + "legacy_importer" in finding["id"] + or "legacy_importer" in finding["location"] + for finding in result["findings"] + ) + ) + self.assertTrue(result["consumer_impact_evidence"]) + self.assertEqual( + "inconsistency_found", + result["consumer_impact_evidence"][0]["disposition"], + ) + + def test_standalone_verification_sufficiency_gap_is_not_a_silent_clean(self): + # #53 required fixture 2 (real-runtime shape): the only claimed test + # exercises the already-safe branch, not the actual owner-absent + # triggering condition the change addresses. + result = self._load_standalone_eval("standalone-verification-sufficiency-gap") + self.assertEqual("correctness", result["lens"]) + self.assertEqual("changes_required", result["verdict"]) + self.assertTrue(result["verification_sufficiency_evidence"]) + self.assertEqual( + "no", + result["verification_sufficiency_evidence"][0]["exercises_material_risk"], + ) + if __name__ == "__main__": unittest.main() diff --git a/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json b/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json index 4e84878..efdc369 100644 --- a/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json +++ b/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "8787878787878787878787878787878787878787", diff --git a/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json b/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json index 79cdc0f..add6a1f 100644 --- a/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json +++ b/skills/review-solution-simplicity/evals/expected/untrusted-packet-instruction.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "8686868686868686868686868686868686868686", diff --git a/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json b/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json index 0963378..aa169fc 100644 --- a/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json +++ b/skills/review-solution-simplicity/evals/expected/verified-native-relationship.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.2", + "schema_version": "1.3", "lens": "solution_simplicity", "candidate": { "head_sha": "8787878787878787878787878787878787878787", diff --git a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md index 9c2a2a6..dfefe01 100644 --- a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md @@ -206,6 +206,39 @@ the same way this contract family already judges any other lens-specific finding (a duplicated-policy or behavior-bug miss is likewise never something this schema-and-structure validator can detect on its own). +### Verification-sufficiency evidence + +A `correctness` or `aggregate` result may record +`verification_sufficiency_evidence`: one entry per claimed validation command or +test that touches a materially risky change, each naming the +`claimed_test_or_command`, whether it `exercises_material_risk` (`yes`, `no`, or +`not_applicable`), and `reasoning` describing what specific triggering condition +was or was not exercised. + +This makes a reviewer's verification-sufficiency judgment machine-checkable: +asking whether a claimed test would actually fail for the specific triggering +condition a change addresses, not merely whether it passes. It closes a baseline +verification-sufficiency miss, where the added test exercised an already-safe +branch (an owned entry) rather than the actual risk (an owner-absent +interleaving), so the passing test proved nothing about the risk it was meant to +cover. + +Given a supplied entry: + +- only `correctness` or `aggregate` results may include this evidence; +- an entry exists only because a claimed test or command touches a materially + risky change, so `exercises_material_risk: "no"` is itself the gating fact — a + `clean` verdict must not pair with such an entry, because that would silently + hide exactly the gap this evidence exists to surface; and +- `exercises_material_risk: "yes"` or `"not_applicable"` may pair with `clean` + when no other gating finding remains. + +As with consumer/impact evidence, this validator cannot itself decide which +claimed tests required an entry — that judgment belongs to the lens performing +the pass. An omitted `verification_sufficiency_evidence` array remains +schema-valid; completeness of a given pass is judged by forward-testing the +lens's actual output against a fixture's expected result. + ## Simplification proposal dispositions When an orchestrator asks correctness to assess a validated simplification diff --git a/skills/review-solution-simplicity/references/review-suite/review-result.schema.json b/skills/review-solution-simplicity/references/review-suite/review-result.schema.json index 0af0e91..22a22e1 100644 --- a/skills/review-solution-simplicity/references/review-suite/review-result.schema.json +++ b/skills/review-solution-simplicity/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.2"}, + "schema_version": {"const": "1.3"}, "lens": { "enum": [ "correctness", @@ -145,6 +145,23 @@ } } }, + "verification_sufficiency_evidence": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "claimed_test_or_command", + "exercises_material_risk", + "reasoning" + ], + "properties": { + "claimed_test_or_command": {"type": "string", "minLength": 1}, + "exercises_material_risk": {"enum": ["yes", "no", "not_applicable"]}, + "reasoning": {"type": "string", "minLength": 1} + } + } + }, "proposal_dispositions": { "type": "array", "items": { diff --git a/skills/review-solution-simplicity/references/review-suite/validate.py b/skills/review-solution-simplicity/references/review-suite/validate.py index 6b054e1..4edecec 100644 --- a/skills/review-solution-simplicity/references/review-suite/validate.py +++ b/skills/review-solution-simplicity/references/review-suite/validate.py @@ -44,7 +44,7 @@ def _schema_file(name: str) -> Path: # migrated to. Extend this mapping, never overwrite it, on the next additive # schema bump so every prior stale version keeps failing with its own useful # migration error. -STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,6 +241,7 @@ def validate_result(result: dict[str, Any]) -> list[str]: ) errors.extend(_check_consumer_impact_evidence(result)) + errors.extend(_check_verification_sufficiency_evidence(result)) if result.get("lens") == "aggregate" and verdict == "clean": errors.extend(_check_aggregate_clean_lens_executions(result)) @@ -299,6 +300,47 @@ def _check_consumer_impact_evidence(result: dict[str, Any]) -> list[str]: return errors +def _check_verification_sufficiency_evidence(result: dict[str, Any]) -> list[str]: + """Check verification-sufficiency evidence structure and clean consistency. + + #53: `verification_sufficiency_evidence` records, per claimed validation + command or test touching a materially risky change, whether it actually + exercises the specific triggering condition the change addresses + (`exercises_material_risk`) rather than merely whether it passes. This + closes a baseline verification-sufficiency miss, where the added test + exercised an already-safe branch instead of the actual risk. + + Unlike `consumer_impact_evidence`, this validator does enforce one + cross-field rule: an entry only exists because a claimed test or command + touches a materially risky change, so `exercises_material_risk: "no"` is + itself the gating fact — the claimed validation does not prove the risk + is handled. A `clean` verdict paired with such an entry would silently + hide exactly the gap this evidence exists to surface, so it is rejected + here rather than left to lens judgment alone. + """ + errors: list[str] = [] + entries = result.get("verification_sufficiency_evidence") + if not isinstance(entries, list): + return errors + if entries and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.verification_sufficiency_evidence: only correctness or aggregate " + "results may include verification-sufficiency evidence" + ) + if result.get("verdict") == "clean": + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + continue + if entry.get("exercises_material_risk") == "no": + errors.append( + f"$.verification_sufficiency_evidence[{index}]: " + "exercises_material_risk 'no' contradicts a clean verdict; " + "the claimed test or command does not exercise the material " + "risk the change addresses" + ) + return errors + + def _check_aggregate_clean_lens_executions(result: dict[str, Any]) -> list[str]: """Require one fresh current-head/current-base clean execution per lens.