From b91e12b063ea6d7ed49f152ee359f1f0eb326363 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 06:40:37 -0700 Subject: [PATCH 1/2] feat(review-suite)!: remove the unproven verification-sufficiency pass - Remove the verification-sufficiency pass and its mandatory `verification_sufficiency_evidence` field from `skills/review-correctness/SKILL.md` and the shared review-result contract; advance `schema_version` `1.3 -> 1.4`. - Update `review-suite/CONTRACT.md` and `review-suite/scripts/validate.py` to match: a `clean` verdict no longer requires or accepts this evidence, and a stale `1.3` result now fails with a clear migration-to-`1.4` error. - Migrate every canonical fixture, standalone expected result, and orchestration case to `schema_version` `1.4`, then refresh all six bundled `references/review-suite/` copies with `just sync-contracts`. - Retire the `verification-sufficiency-guard` fixture (its sole purpose was the removed field's clean-pairing behavior); adapt, not delete, the `standalone-verification-sufficiency-gap` eval case, since its underlying defect pattern (a claimed test that misses the actual triggering condition) is still a genuine correctness catch expected from ordinary review, per `review-suite/fixtures/missing-test/`. - Add `review-suite/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md`, the decision record citing `S1-ABLATION-MATRIX.md` and `discriminating-case-validation.md` directly, and describe the removal in `README.md` and `CHANGELOG.md`. - Two independent, honestly-sourced measurements found no demonstrated value for this pass: `stale-claim-release-guard` (#57's ablation matrix) and `audit-log-flush-keyword-probe` (#89's harder-case validation) both resolve identically at `mean_combined_recall 1.0` whether the pass is enabled or not. - It also carries a confirmed, twice-reproduced cost: run in isolation (traversal pass disabled), it regresses `session-continuation-summary` - a clean control - to 3/5 false positives. - The traversal (consumer/impact) pass and `consumer_impact_evidence` showed a real, reproducible gap in the same evidence (`artifact-promotion-environment-shortcut`, 1.0 vs 0.4) and are explicitly out of scope for this ticket; both are verified byte-identical to their #52/#53-shipped state. - Approved directly by the repository owner (#93), executing a removal decision already made rather than re-litigating it. Fixes #93 --- CHANGELOG.md | 10 +- README.md | 12 ++ review-suite/CONTRACT.md | 33 ---- .../contracts/review-result.schema.json | 19 +-- .../v2/VERIFICATION-SUFFICIENCY-REMOVAL.md | 144 ++++++++++++++++++ .../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 | 132 +++++----------- .../scripts/tests/test_eval_protocol.py | 4 +- .../scripts/tests/test_eval_runner.py | 6 +- .../scripts/tests/test_review_gate.py | 6 +- review-suite/scripts/validate.py | 44 +----- skills/babysit-pr/evals/cases.json | 2 +- .../references/review-suite/CONTRACT.md | 33 ---- .../review-suite/review-result.schema.json | 19 +-- .../references/review-suite/validate.py | 44 +----- .../scripts/tests/test_review_gate.py | 6 +- skills/implement-ticket/evals/cases.json | 2 +- .../references/review-and-merge-gates.md | 4 +- .../references/review-suite/CONTRACT.md | 33 ---- .../review-suite/review-result.schema.json | 19 +-- .../references/review-suite/validate.py | 44 +----- .../tests/test_implement_ticket_contract.py | 2 +- .../scripts/tests/test_review_gate.py | 6 +- 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 | 40 +++-- skills/review-correctness/evals/README.md | 12 +- ...ne-sibling-call-site-traversal.result.json | 2 +- .../standalone-ticket-regression.result.json | 2 +- ...e-verification-sufficiency-gap.result.json | 9 +- .../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 | 38 +++-- .../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 +----- 70 files changed, 347 insertions(+), 938 deletions(-) create mode 100644 review-suite/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md delete mode 100644 review-suite/fixtures/verification-sufficiency-guard/expected.json delete mode 100644 review-suite/fixtures/verification-sufficiency-guard/packet.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 897d2fc..75c9b64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,17 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-30 — Simplified the review-fix-loop design around local coordination and Git-native publication safety +## 2026-07-30 — Removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- feat(review-suite)!: remove the verification-sufficiency pass and its + mandatory `verification_sufficiency_evidence` field from `review-correctness` + and the shared review-result contract, advancing `schema_version` `1.3 → 1.4`; + the traversal (consumer/impact) pass and `consumer_impact_evidence` are + unchanged, per #57's ablation matrix and #89's harder-case validation finding + no demonstrated value for the removed pass plus a confirmed, twice-reproduced + false-positive regression when it ran without the traversal pass (#93) - docs: simplify the review-fix-loop design + (`2e7a8cd93af9f2c8cec36d6c393694f7849adedb`) ## 2026-07-29 — Sourced two harder discriminating cases for the traversal and verification-sufficiency passes, designed the review-fix-loop skill, migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, added connector-outcome curation and promotion tooling, added a skill-root override for mechanism ablation runs, ran the preregistered v2 ablation and integration closeout, and confirmed the session-continuation-summary verification-only regression with an independent rerun diff --git a/README.md b/README.md index 67532ca..654751a 100644 --- a/README.md +++ b/README.md @@ -257,6 +257,18 @@ causal contribution — it does not, on the evidence recorded there), and a recommendation, not a removal decision, per mechanism. See [its README](review-suite/evals/v2/README.md) for the full document set. +Following that evidence, the repository owner removed the verification- +sufficiency pass (`#93`): neither `#57`'s ablation matrix nor `#89`'s harder- +case validation (`discriminating-case-validation.md`) found demonstrated value +for it, and it carried a confirmed, twice-reproduced false-positive regression +on `session-continuation-summary` when run without the traversal pass. The +traversal (consumer/impact) pass and `consumer_impact_evidence` were unaffected +— that pass showed a real, reproducible gap in the same validation. The shared +review-result contract advanced `1.3 → 1.4` to drop +`verification_sufficiency_evidence`; see +[`review-suite/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md`](review-suite/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md) +for the full decision record. + ### Connector-outcome curation and promotion `review-suite/evals/curation/` turns a newly adjudicated connector finding into diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index dfefe01..9c2a2a6 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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 22a22e1..1bde7d2 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.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md b/review-suite/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md new file mode 100644 index 0000000..52c6e2e --- /dev/null +++ b/review-suite/evals/v2/VERIFICATION-SUFFICIENCY-REMOVAL.md @@ -0,0 +1,144 @@ +# Verification-sufficiency pass removal (#93) + +This records the repository owner's removal decision for the verification- +sufficiency pass and its mandatory `verification_sufficiency_evidence` schema +field, both added by #52/#53, and the evidentiary basis the owner approved it +against on 2026-07-29. It does not modify `S1-ABLATION-MATRIX.md`, +`discriminating-case-validation.md`, `FROZEN-V2-CONFIGURATION.md`, +`CLOSEOUT-REPORT.md`, `gate-manifest.json`, `DECISION-RECORD.md`, +`FAILURE-TAXONOMY.md`, `audits/`, or `review-suite/evals/baseline/v1/` - all +remain exactly as prior tickets delivered them. This is the sole new file this +ticket adds under `review-suite/evals/v2/`. + +## Decision + +Remove the verification-sufficiency pass from +`skills/review-correctness/ SKILL.md` and remove +`verification_sufficiency_evidence` entirely from the shared review-result +contract, advancing `schema_version` `1.3 -> 1.4`. The traversal +(consumer/impact) pass and `consumer_impact_evidence` are unchanged, +byte-for-byte, from their #52/#53-shipped state - they are not in scope for this +decision and were not touched by this ticket. + +**Full removal, not an optional non-gating field.** #93's body states full +removal is preferred unless a concrete reason favors keeping an optional, +non-gating field. No such reason exists here: the field never had demonstrated +value (below), and it carried a confirmed, twice-reproduced cost. Retaining it +as optional would keep exactly the mechanism responsible for that regression +available for a reviewer to invoke, for no offsetting benefit ever measured in +this corpus. + +## Evidentiary basis + +Two independent, honestly-sourced measurements found no demonstrated value for +this pass: + +- **`stale-claim-release-guard`** (the original justifying case, #53's own + target): `S1-ABLATION-MATRIX.md`'s target-case gate table shows + `mean_recall 0.0`, `combined 1.0` -> **PASS** identically across all three + configurations (both passes together, traversal-only, verification-only). The + case resolves whether or not the verification-sufficiency pass runs at all - + its own "Settled verdict per mechanism" table states this plainly: "**No** - + case also passes with this pass disabled." +- **`audit-log-flush-keyword-probe`** (a new, deliberately harder case sourced + specifically to test this, #89): `discriminating-case-validation.md`'s + per-case results show `mean_combined_recall 1.0` in both the "both passes + together" and "traversal-only" (verification-sufficiency disabled) + configurations, 5 runs each, zero false positives or evaluation failures + either way. Its own conclusion: "This candidate does not discriminate, even + after a genuine, disguised-mock construction and a full validation pass." + +On top of the absence of demonstrated value, the pass carries a confirmed, +twice-reproduced cost: `S1-ABLATION-MATRIX.md`'s non-regression floor table +shows `session-continuation-summary` - a previously clean control with +`expected_root_cause_ids: []` - regressing to **3/5 false positives** under the +verification-only configuration (traversal pass disabled), a result +independently reconfirmed in `session-continuation-summary-confirming-rerun.md`. +No comparable regression appears under traversal-only, and none appears in the +"both passes together" (as-shipped) configuration - so this is evidence against +running the verification-sufficiency pass *in isolation*, not evidence against +whatever the already-shipped combined configuration produced. + +`discriminating-case-validation.md`'s own summary states the resulting position +directly: "the evidentiary case for this specific pass, as currently instructed, +is weaker after this validation than before it - a candidate built to be more +likely to need it did not need it." Both documents explicitly defer the +keep/remove decision itself to the repository owner; this record reflects that +the owner made it, approving removal on 2026-07-29. + +By contrast, and precisely because this decision must not blur the two +mechanisms together: the traversal pass **does** have demonstrated value in the +same body of evidence. `discriminating-case-validation.md` reports +`artifact-promotion-environment-shortcut` at `mean_combined_recall` 1.0 with the +traversal pass enabled vs. 0.4 disabled - a real, reproducible gap, "the first +evidence anywhere in this epic that the traversal (consumer/impact) pass changes +reviewer behavior on a case built to need it." That pass and its +`consumer_impact_evidence` schema field are unaffected by this removal. + +## What changed + +- `skills/review-correctness/SKILL.md`: removed the verification-sufficiency + pass instruction and its result-field requirement. The underlying correctness + question - whether a claimed test actually exercises the triggering condition + a risky change addresses, rather than merely passing - remains part of + ordinary correctness review (see "Review in priority order" item 6), just + without a separately mandated pass or required evidence field. The traversal + pass instruction is preserved exactly as shipped. +- `review-suite/contracts/review-result.schema.json`: `schema_version` + `1.3 -> 1.4`; the `verification_sufficiency_evidence` property is removed from + the schema entirely (not merely made optional - it was already optional at the + top level; the removal eliminates the property and its dedicated cross-field + validation). +- `review-suite/scripts/validate.py`: removed + `_check_verification_sufficiency_evidence` and its call site; extended + `STALE_RESULT_SCHEMA_VERSIONS` so a `1.3` result is rejected with a clear + migration-to-`1.4` error rather than silently accepted or misread. +- `review-suite/CONTRACT.md`: removed the "Verification-sufficiency evidence" + section. The "Consumer/impact evidence" section is unchanged. +- Canonical fixtures (`review-suite/fixtures/*/expected.json`): migrated + `schema_version` `1.3 -> 1.4` across every fixture. The + `verification-sufficiency-guard` fixture, whose entire purpose was exercising + the removed field's `clean`-pairing behavior, was retired (removed from + `review-suite/fixtures/manifest.json` and deleted) rather than adapted, since + nothing about its schema-conformance purpose survives the field's removal. +- `review-suite/scripts/tests/test_contracts.py`: removed + `VerificationSufficiencyEvidenceTests`; added a test proving the field is no + longer part of the schema and is rejected as an unknown property, and added + `test_stale_v1_3_aggregate_result_is_rejected_with_a_useful_error` alongside + the existing `1.0`/`1.1`/`1.2` stale-version regression tests. +- `skills/review-correctness/evals/standalone-verification-sufficiency-gap/` and + its expected result: **adapted, not deleted**, per #93's own instruction. Its + diff and ticket describe a genuine correctness defect pattern (a claimed test + that exercises an already-safe branch instead of the actual owner-absent + triggering condition) independent of the removed mechanism - + `review-suite/fixtures/missing-test/` already demonstrates that ordinary + correctness review catches an analogous insufficient-test-coverage gap without + any dedicated pass or evidence field. The expected result keeps its + `changes_required` verdict and blocking finding; only + `verification_sufficiency_evidence` and the stale `schema_version` were + removed/migrated. +- Every other schema-`1.3` expected result across the bundled review skills + (`review-code-change`, `review-code-simplicity`, + `review-solution- simplicity`, `babysit-pr`, `implement-ticket`, in addition + to `review-correctness`) and the shared eval/protocol test harnesses were + migrated to `1.4` atomically, since `schema_version` is one contract shared + across every lens and caller. +- `just sync-contracts` was run after every canonical edit so all six bundled + `references/review-suite/` copies (and, for `implement-ticket`/`babysit-pr`, + `scripts/review_gate.py` and its test) stay byte-identical to the canonical + source. + +## What did not change + +- The consumer/impact-traversal pass instruction in + `skills/review-correctness/SKILL.md`. +- `consumer_impact_evidence` anywhere in the schema, `CONTRACT.md`, or + `validate.py`. +- The `stale-claim-release-guard` and `audit-log-flush-keyword-probe` corpus + cases in `review-suite/evals/strata/s1-correctness-orchestrator/` - both + remain in the corpus as general correctness cases; only the requirement that a + `clean` verdict include `verification_sufficiency_evidence` is gone. +- `review-suite/evals/baseline/v1/`, `gate-manifest.json`, `DECISION-RECORD.md`, + `FAILURE-TAXONOMY.md`, `audits/`, and every existing `review-suite/evals/v2/` + measurement document, including the ablation skill-root snapshots under + `ablation-skill-roots/` used to reproduce the measurements cited above. diff --git a/review-suite/fixtures/auth-regression/expected.json b/review-suite/fixtures/auth-regression/expected.json index 96c3442..32d1e7f 100644 --- a/review-suite/fixtures/auth-regression/expected.json +++ b/review-suite/fixtures/auth-regression/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 a75df53..06a41b5 100644 --- a/review-suite/fixtures/behavior-bug/expected.json +++ b/review-suite/fixtures/behavior-bug/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 16f4ba6..52b8169 100644 --- a/review-suite/fixtures/clean-change/expected.json +++ b/review-suite/fixtures/clean-change/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 daa5580..70e1d1a 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.3", + "schema_version": "1.4", "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 99f6be7..c1b1986 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.3", + "schema_version": "1.4", "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 4653923..acd81f3 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.3", + "schema_version": "1.4", "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 d841b85..18b100d 100644 --- a/review-suite/fixtures/duplicated-policy/expected.json +++ b/review-suite/fixtures/duplicated-policy/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 3c357f8..e4627f4 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.3", + "schema_version": "1.4", "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 59d484b..06b4290 100644 --- a/review-suite/fixtures/imagined-machinery/expected.json +++ b/review-suite/fixtures/imagined-machinery/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "solution_simplicity", "candidate": { "head_sha": "3333333333333333333333333333333333333333", diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json index bb081e2..d4e27ff 100644 --- a/review-suite/fixtures/manifest.json +++ b/review-suite/fixtures/manifest.json @@ -15,7 +15,6 @@ {"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 8e87593..add22c1 100644 --- a/review-suite/fixtures/missing-evidence/expected.json +++ b/review-suite/fixtures/missing-evidence/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 efd404f..235b650 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.3", + "schema_version": "1.4", "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 b1e482b..faa4ff4 100644 --- a/review-suite/fixtures/missing-test/expected.json +++ b/review-suite/fixtures/missing-test/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 a153012..43f9b8b 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.3", + "schema_version": "1.4", "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 6cb74f0..13edf7b 100644 --- a/review-suite/fixtures/necessary-complexity/expected.json +++ b/review-suite/fixtures/necessary-complexity/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 1c39f3e..56a986c 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.3", + "schema_version": "1.4", "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 a016a80..f259abf 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.3", + "schema_version": "1.4", "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 3c1d257..a441188 100644 --- a/review-suite/fixtures/speculative-backfill/expected.json +++ b/review-suite/fixtures/speculative-backfill/expected.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "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 07f9756..d1d9e1b 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.3", + "schema_version": "1.4", "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 deleted file mode 100644 index 5bb30b1..0000000 --- a/review-suite/fixtures/verification-sufficiency-guard/expected.json +++ /dev/null @@ -1,19 +0,0 @@ -{ - "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 deleted file mode 100644 index 2da32c5..0000000 --- a/review-suite/fixtures/verification-sufficiency-guard/packet.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "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 7be1a44..fbcd173 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.3", + "schema_version": "1.4", "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 f498c5c..15377d5 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.3", + "schema_version": "1.4", "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 8a4ea9e..37bb343 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -416,8 +416,12 @@ 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, and - by #53 to reject a stale v1.2 aggregate now that schema 1.3 exists.""" + #52 to also reject a stale v1.1 aggregate now that schema 1.2 exists, by + #53 to reject a stale v1.2 aggregate now that schema 1.3 exists, and by + #93 to reject a stale v1.3 aggregate now that schema 1.4 exists (the + verification-sufficiency pass and its required-evidence field were + removed at 1.4; a 1.3 result relying on that field must not be silently + reinterpreted as 1.4 evidence).""" def test_stale_v1_0_aggregate_result_is_rejected_with_a_useful_error(self): result = load(ROOT / "fixtures" / "clean-change" / "expected.json") @@ -443,6 +447,14 @@ def test_stale_v1_2_aggregate_result_is_rejected_with_a_useful_error(self): self.assertTrue(any("stale v1.2" in error for error in errors)) self.assertTrue(any("1.3" in error for error in errors)) + def test_stale_v1_3_aggregate_result_is_rejected_with_a_useful_error(self): + result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + result["schema_version"] = "1.3" + errors = VALIDATOR.validate_result(result) + self.assertTrue(errors) + self.assertTrue(any("stale v1.3" in error for error in errors)) + self.assertTrue(any("1.4" in error for error in errors)) + class ConsumerImpactEvidenceTests(unittest.TestCase): """#52: `consumer_impact_evidence` records a reviewer's traversal to other @@ -536,111 +548,35 @@ 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)) +class RemovedVerificationSufficiencyEvidenceTests(unittest.TestCase): + """#93: `verification_sufficiency_evidence` and its mandated pass were + removed at schema 1.4 (no demonstrated value in #57's ablation matrix or + #89's harder-case validation, plus a confirmed, twice-reproduced + `session-continuation-summary` false-positive regression when the pass ran + without the traversal pass). The field is no longer part of the schema at + all -- unlike `consumer_impact_evidence`, which stays required-when-used + and unaffected by this removal.""" - 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_field_is_no_longer_part_of_the_schema(self): + schema = json.loads( + (ROOT / "contracts" / "review-result.schema.json").read_text() ) + self.assertNotIn("verification_sufficiency_evidence", schema["properties"]) - 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"] = [ + def test_a_supplied_field_is_rejected_as_an_unknown_property(self): + result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + result["verification_sufficiency_evidence"] = [ { - "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", + "claimed_test_or_command": "pytest", + "exercises_material_risk": "yes", + "reasoning": "no longer a recognized field", } ] - 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) + errors = VALIDATOR.validate_result(result) self.assertTrue(errors) self.assertTrue( any( - "only correctness or aggregate results may include " - "verification-sufficiency evidence" in error + "verification_sufficiency_evidence: unknown property" in error for error in errors ) ) diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index e875295..17cf800 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.3", + "schema_version": "1.4", "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.3", + "schema_version": "1.4", "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 3d62e1f..1185a63 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -256,7 +256,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.3", "lens": "aggregate",\n' + ' "schema_version": "1.4", "lens": "aggregate",\n' ' "candidate": request["run"]["candidate"],\n' ' "verdict": "changes_required", "findings": [finding],\n' ' "blocking_reasons": [],\n' @@ -589,7 +589,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.3", "lens": "aggregate",\n' + ' "schema_version": "1.4", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' ' "lens_executions": lens_executions,\n' @@ -652,7 +652,7 @@ def _envelope_stub(self, envelope_body: str) -> Path: ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' "]\n" "result = {\n" - ' "schema_version": "1.3", "lens": "aggregate",\n' + ' "schema_version": "1.4", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' ' "lens_executions": lens_executions,\n' diff --git a/review-suite/scripts/tests/test_review_gate.py b/review-suite/scripts/tests/test_review_gate.py index 04e226a..ddd510c 100644 --- a/review-suite/scripts/tests/test_review_gate.py +++ b/review-suite/scripts/tests/test_review_gate.py @@ -27,7 +27,7 @@ BASE = "abababababababababababababababababababab" CLEAN_AGGREGATE = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE}, "verdict": "clean", @@ -75,7 +75,7 @@ def test_malformed_result_is_rejected(self): def test_blocked_verdict_is_rejected(self): blocked = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": {}, "verdict": "blocked", @@ -147,7 +147,7 @@ def test_result_bound_to_a_different_base_is_rejected(self): def test_non_aggregate_lens_result_is_rejected(self): single_lens = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE}, "verdict": "clean", diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index 4edecec..c88e760 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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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/babysit-pr/evals/cases.json b/skills/babysit-pr/evals/cases.json index 7a9d1ef..29e02b7 100644 --- a/skills/babysit-pr/evals/cases.json +++ b/skills/babysit-pr/evals/cases.json @@ -117,7 +117,7 @@ "request": "Watch PR 205 until it is ready to merge.", "policy": "ready_to_merge", "authority": "read_and_ticket_scoped_fix_only", - "candidate_state": "CI and connector review are clean, but the supplied review-code-change result carries schema_version 1.1 instead of the bundled contract's current version 1.3." + "candidate_state": "CI and connector review are clean, but the supplied review-code-change result carries schema_version 1.1 instead of the bundled contract's current version 1.4." }, { "id": "malformed-review-result-blocks-final-gate", diff --git a/skills/babysit-pr/references/review-suite/CONTRACT.md b/skills/babysit-pr/references/review-suite/CONTRACT.md index dfefe01..9c2a2a6 100644 --- a/skills/babysit-pr/references/review-suite/CONTRACT.md +++ b/skills/babysit-pr/references/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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/babysit-pr/references/review-suite/review-result.schema.json b/skills/babysit-pr/references/review-suite/review-result.schema.json index 22a22e1..1bde7d2 100644 --- a/skills/babysit-pr/references/review-suite/review-result.schema.json +++ b/skills/babysit-pr/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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/babysit-pr/references/review-suite/validate.py b/skills/babysit-pr/references/review-suite/validate.py index 4edecec..c88e760 100644 --- a/skills/babysit-pr/references/review-suite/validate.py +++ b/skills/babysit-pr/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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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/babysit-pr/scripts/tests/test_review_gate.py b/skills/babysit-pr/scripts/tests/test_review_gate.py index 04e226a..ddd510c 100644 --- a/skills/babysit-pr/scripts/tests/test_review_gate.py +++ b/skills/babysit-pr/scripts/tests/test_review_gate.py @@ -27,7 +27,7 @@ BASE = "abababababababababababababababababababab" CLEAN_AGGREGATE = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE}, "verdict": "clean", @@ -75,7 +75,7 @@ def test_malformed_result_is_rejected(self): def test_blocked_verdict_is_rejected(self): blocked = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": {}, "verdict": "blocked", @@ -147,7 +147,7 @@ def test_result_bound_to_a_different_base_is_rejected(self): def test_non_aggregate_lens_result_is_rejected(self): single_lens = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE}, "verdict": "clean", diff --git a/skills/implement-ticket/evals/cases.json b/skills/implement-ticket/evals/cases.json index 23ffb26..4104eb4 100644 --- a/skills/implement-ticket/evals/cases.json +++ b/skills/implement-ticket/evals/cases.json @@ -217,7 +217,7 @@ "request": "Implement GitHub issue G-88 to a ready PR.", "tracker_state": "G-88 is otherwise ready and independently shippable.", "authority": "ready_pr_only", - "candidate_state": "The initial review-code-change invocation returns an aggregate result carrying schema_version 1.2 instead of the bundled review-result contract's current version 1.3." + "candidate_state": "The initial review-code-change invocation returns an aggregate result carrying schema_version 1.2 instead of the bundled review-result contract's current version 1.4." }, { "id": "malformed-review-result-blocks-publication", diff --git a/skills/implement-ticket/references/review-and-merge-gates.md b/skills/implement-ticket/references/review-and-merge-gates.md index 42b6c70..1bb2ef2 100644 --- a/skills/implement-ticket/references/review-and-merge-gates.md +++ b/skills/implement-ticket/references/review-and-merge-gates.md @@ -20,8 +20,8 @@ beside it. Before consuming any returned result, validate it with `scripts/review_gate.py`, which also binds the result to this run's exact current head and comparison-base SHA. Reject a result that fails schema validation, carries a `schema_version` other than the bundled contract's current -version (`1.3`; a stale version such as `1.0`, `1.1`, or `1.2` fails with its -own migration message and is never accepted as current evidence), is not an +version (`1.4`; a stale version such as `1.0`, `1.1`, `1.2`, or `1.3` fails with +its own migration message and is never accepted as current evidence), is not an `aggregate` result, has a `verdict` other than `clean`, or is missing a complete, current, freshly executed `lens_executions` entry for every required lens. Treat any such rejection, exactly like a missing dependency or a `blocked` diff --git a/skills/implement-ticket/references/review-suite/CONTRACT.md b/skills/implement-ticket/references/review-suite/CONTRACT.md index dfefe01..9c2a2a6 100644 --- a/skills/implement-ticket/references/review-suite/CONTRACT.md +++ b/skills/implement-ticket/references/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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/implement-ticket/references/review-suite/review-result.schema.json b/skills/implement-ticket/references/review-suite/review-result.schema.json index 22a22e1..1bde7d2 100644 --- a/skills/implement-ticket/references/review-suite/review-result.schema.json +++ b/skills/implement-ticket/references/review-suite/review-result.schema.json @@ -13,7 +13,7 @@ "blocking_reasons" ], "properties": { - "schema_version": {"const": "1.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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/implement-ticket/references/review-suite/validate.py b/skills/implement-ticket/references/review-suite/validate.py index 4edecec..c88e760 100644 --- a/skills/implement-ticket/references/review-suite/validate.py +++ b/skills/implement-ticket/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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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/implement-ticket/scripts/tests/test_implement_ticket_contract.py b/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py index 447f85f..f897afb 100644 --- a/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py +++ b/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py @@ -250,7 +250,7 @@ def test_review_result_contract_violations_block_publication(self): self.assertIn("references/review-suite/validate.py", self.gates) self.assertIn("scripts/review_gate.py", self.gates) self.assertIn("schema_version", self.gates) - self.assertIn("1.3", self.gates) + self.assertIn("1.4", self.gates) self.assertIn("lens_executions", self.gates) def test_acceptance_evidence_is_criterion_specific_and_fail_closed(self): diff --git a/skills/implement-ticket/scripts/tests/test_review_gate.py b/skills/implement-ticket/scripts/tests/test_review_gate.py index 04e226a..ddd510c 100644 --- a/skills/implement-ticket/scripts/tests/test_review_gate.py +++ b/skills/implement-ticket/scripts/tests/test_review_gate.py @@ -27,7 +27,7 @@ BASE = "abababababababababababababababababababab" CLEAN_AGGREGATE = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE}, "verdict": "clean", @@ -75,7 +75,7 @@ def test_malformed_result_is_rejected(self): def test_blocked_verdict_is_rejected(self): blocked = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": {}, "verdict": "blocked", @@ -147,7 +147,7 @@ def test_result_bound_to_a_different_base_is_rejected(self): def test_non_aggregate_lens_result_is_rejected(self): single_lens = { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE}, "verdict": "clean", diff --git a/skills/review-code-change/evals/cases.json b/skills/review-code-change/evals/cases.json index 4236396..b8fed75 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.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": []} + "solution_simplicity": {"schema_version": "1.4", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "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.4", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "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.4", "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.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "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.4", "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.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "lens": "solution_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "lens": "solution_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "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.4", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.4", "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.4", "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.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": []} + "solution_simplicity": {"schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.4", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.4", "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 77be817..1755cdc 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.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "1010101010101010101010101010101010101010", @@ -50,7 +50,7 @@ ], "stopped_reason": "solution_strategy_replacement_required", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "2020202020202020202020202020202020202020", @@ -89,7 +89,7 @@ ], "stopped_reason": "unsafe_simplification_rejected_and_remaining_lenses_clean", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "3030303030303030303030303030303030303030", @@ -147,7 +147,7 @@ ], "stopped_reason": "code_simplicity_gating_finding", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "4040404040404040404040404040404040404040", @@ -207,7 +207,7 @@ ], "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "5050505050505050505050505050505050505050", @@ -247,7 +247,7 @@ "observed_sequence": [], "stopped_reason": "missing_required_dependency", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "6060606060606060606060606060606060606060", @@ -266,7 +266,7 @@ "observed_sequence": [], "stopped_reason": "evidence_packet_incomplete", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "7070707070707070707070707070707070707070", @@ -293,7 +293,7 @@ ], "stopped_reason": "all_required_lenses_clean_with_deferred_finding", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "8080808080808080808080808080808080808080", @@ -354,7 +354,7 @@ ], "stopped_reason": "cycle_budget_exhausted_with_correctness_finding", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "9090909090909090909090909090909090909090", @@ -391,7 +391,7 @@ ], "stopped_reason": "untrustworthy_lens_result", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "aggregate", "candidate": { "head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", @@ -416,7 +416,7 @@ ], "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.3", + "schema_version": "1.4", "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 5e0693b..545f417 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.3", + "schema_version": "1.4", "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 dfefe01..9c2a2a6 100644 --- a/skills/review-code-change/references/review-suite/CONTRACT.md +++ b/skills/review-code-change/references/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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 22a22e1..1bde7d2 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.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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 4edecec..c88e760 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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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 b441c02..778df6b 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.3", + "schema_version": "1.4", "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 dfefe01..9c2a2a6 100644 --- a/skills/review-code-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-code-simplicity/references/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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 22a22e1..1bde7d2 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.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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 4edecec..c88e760 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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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 9f3621a..8528183 100644 --- a/skills/review-correctness/SKILL.md +++ b/skills/review-correctness/SKILL.md @@ -59,11 +59,11 @@ 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 the required traversal pass -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, +Perform this pass on every review and record its evidence in the shared result. +It is a dimension of this single correctness lens, not a routed specialist +module; 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 @@ -79,20 +79,17 @@ concurrency-as-a-context, compatibility/migration, operations, or UI module. 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. + +Whether a claimed validation command or test actually exercises the specific +triggering condition a materially risky change addresses — not merely whether it +passes — remains part of ordinary correctness review (see "Check that tests and +exact validation evidence prove success, failure, regression, and preserved +behavior required by the change contract" above). Detect mocks, fixtures, stubs, +happy-path assertions, or partial-suite selection that bypass the risky +behavior, and raise a blocking finding when a claimed test exercises an +already-safe branch instead of the actual triggering condition. This is a +routine application of the finding threshold below, not a separate mandated pass +with its own required evidence field. ## Apply the finding threshold @@ -139,10 +136,9 @@ 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. +- Include a `consumer_impact_evidence` entry for every changed symbol the + required traversal pass 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 ea46850..f23f1b4 100644 --- a/skills/review-correctness/evals/README.md +++ b/skills/review-correctness/evals/README.md @@ -11,7 +11,11 @@ directory cannot read the answer key. 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. +- `standalone-verification-sufficiency-gap/`: ordinary correctness review's + test- sufficiency judgment (the mandated verification-sufficiency pass this + case originally modeled was removed by #93 for lack of demonstrated value; the + underlying defect pattern -- a release guard whose only claimed test exercises + an already-safe branch instead of the actual owner-absent triggering condition + the change addresses -- remains a genuine correctness catch expected from the + ordinary review dimensions in `references/correctness-rubric.md`, without a + separate mandated pass or required evidence field). 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 index 8ebb210..5e6f4d9 100644 --- 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 @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "correctness", "candidate": { "head_sha": "6767676767676767676767676767676767676767", 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 266f720..e23f5bf 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.3", + "schema_version": "1.4", "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 index 940ca5f..923ce17 100644 --- a/skills/review-correctness/evals/expected/standalone-verification-sufficiency-gap.result.json +++ b/skills/review-correctness/evals/expected/standalone-verification-sufficiency-gap.result.json @@ -1,5 +1,5 @@ { - "schema_version": "1.3", + "schema_version": "1.4", "lens": "correctness", "candidate": { "head_sha": "5959595959595959595959595959595959595959", @@ -31,12 +31,5 @@ } ], "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/references/review-suite/CONTRACT.md b/skills/review-correctness/references/review-suite/CONTRACT.md index dfefe01..9c2a2a6 100644 --- a/skills/review-correctness/references/review-suite/CONTRACT.md +++ b/skills/review-correctness/references/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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 22a22e1..1bde7d2 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.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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 4edecec..c88e760 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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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 9de4a0d..de6ef3c 100644 --- a/skills/review-correctness/scripts/tests/test_skill_contract.py +++ b/skills/review-correctness/scripts/tests/test_skill_contract.py @@ -41,16 +41,20 @@ 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. + def test_skill_requires_the_traversal_pass(self): + # #53 required the traversal pass be part of this single lens, not a + # routed specialist module. #93 removed the verification-sufficiency + # pass and its `verification_sufficiency_evidence` field entirely -- + # neither #57's ablation matrix nor #89's harder-case validation found + # demonstrated value, and it carried a confirmed, twice-reproduced + # false-positive regression when run without the traversal pass. 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.assertNotIn("Verification-sufficiency pass", normalized) + self.assertNotIn("verification_sufficiency_evidence", normalized) + self.assertIn("not a routed specialist module", normalized) self.assertIn( "do not build or delegate to a separate security, " "concurrency-as-a-context, compatibility/migration, operations, " @@ -75,7 +79,6 @@ def test_correctness_fixture_results_conform(self): "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): @@ -153,17 +156,22 @@ def test_standalone_sibling_call_site_traversal_finds_the_missed_consumer(self): 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. + def test_standalone_insufficient_test_coverage_is_not_a_silent_clean(self): + # Originally #53's required fixture 2, exercising the now-removed + # verification-sufficiency pass (#93). The underlying defect pattern + # -- the only claimed test exercises the already-safe branch, not the + # actual owner-absent triggering condition the change addresses -- + # remains a genuine correctness catch expected from ordinary review, + # without a separate mandated pass or its required evidence field. 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"], + self.assertNotIn("verification_sufficiency_evidence", result) + self.assertTrue( + any( + "claim-release-guard-unverified" in finding["id"] + for finding in result["findings"] + ) ) 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 efdc369..6fe027b 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.3", + "schema_version": "1.4", "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 add6a1f..a7dd98a 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.3", + "schema_version": "1.4", "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 aa169fc..0a31551 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.3", + "schema_version": "1.4", "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 dfefe01..9c2a2a6 100644 --- a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md @@ -206,39 +206,6 @@ 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 22a22e1..1bde7d2 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.3"}, + "schema_version": {"const": "1.4"}, "lens": { "enum": [ "correctness", @@ -145,23 +145,6 @@ } } }, - "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 4edecec..c88e760 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", "1.2": "1.3"} +STALE_RESULT_SCHEMA_VERSIONS = {"1.0": "1.1", "1.1": "1.2", "1.2": "1.3", "1.3": "1.4"} BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( @@ -241,7 +241,6 @@ 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)) @@ -300,47 +299,6 @@ 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. From 4511535a5cb2ad845c2d7a5641853775e0027720 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 07:01:08 -0700 Subject: [PATCH 2/2] docs(review-correctness): fix a formatter-introduced hyphen-wrap typo in evals README --- skills/review-correctness/evals/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/review-correctness/evals/README.md b/skills/review-correctness/evals/README.md index f23f1b4..c59bfe4 100644 --- a/skills/review-correctness/evals/README.md +++ b/skills/review-correctness/evals/README.md @@ -12,7 +12,7 @@ directory cannot read the answer key. existing sibling call site keeps an explicit permissive override that the diff never touches. - `standalone-verification-sufficiency-gap/`: ordinary correctness review's - test- sufficiency judgment (the mandated verification-sufficiency pass this + test-sufficiency judgment (the mandated verification-sufficiency pass this case originally modeled was removed by #93 for lack of demonstrated value; the underlying defect pattern -- a release guard whose only claimed test exercises an already-safe branch instead of the actual owner-absent triggering condition