From 23d7c59234ab20b305261c339d4de9ed6b6e4e12 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 19:10:07 -0700 Subject: [PATCH 1/2] feat: require passing validation and current-head lens evidence for a clean review verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Bump the shared review result contract to schema `1.1` (additive only): add a `lens_executions` array recording, per required lens, its head SHA, comparison-base SHA, verdict, and whether it was freshly executed. - `review-suite/scripts/validate.py`: reject any `clean` verdict paired with a packet whose focused or full validation entry is `failed` or `unavailable`; require an aggregate `clean` to carry exactly one fresh, current-head, clean `lens_executions` entry for each of solution simplicity, correctness, and code simplicity, with no missing, duplicate, or stale-head/base entry; reject a stale `schema_version: "1.0"` result with a clear migration error instead of silently reinterpreting it as v1.1. - `review-code-change`'s orchestration protocol and SKILL.md: any head-changing fix (solution redesign, correctness fix, or code-simplicity fix) now restarts the complete three-lens sequence, closing the re-review-matrix gap where a correctness-only or code-simplicity-only rerun could reach a new-head `clean` aggregate without a fresh solution-simplicity result for that head. - Migrate every canonical fixture, the review-code-change orchestration case corpus, and each bundled skill's standalone eval fixtures to schema 1.1; refresh the four bundled `review-suite` copies via `just sync-contracts`. - Add 13 direct regression tests in `review-suite/scripts/tests/test_contracts.py` covering the 11 required cases from #51: failed/unavailable focused and full validation paired with `clean`, missing/duplicate/stale lens executions, the two new-head full-restart scenarios, unchanged-head base drift, and stale v1.0 rejection. ## Why - #51: an aggregate `clean` result was reachable even when every required validation command had failed, and the orchestrator's old re-review matrix could reach a new-head `clean` aggregate without re-running all three required lenses on that head. Both are deterministic protocol/validator contradictions identified and settled by #59's decision record, which this child repairs without adding coverage ledgers, impact maps, or other scope reserved for later children. ## Verification - Replayed the frozen v1 baseline's 15 scored cases (`review-suite/evals/baseline/v1/`) against the v1.1 canonical validator: all 15 real corpus packets still validate with zero errors, and only one of 32 validation entries across all 15 packets is non-`passed` (`stale-claim-release-guard`'s `unavailable` concurrency-soak entry) — but that case's frozen report records `false_clean_attempts: 0` across all 5 scored attempts, so the new clean-requires-passing-validation rule never fires against any recorded verdict. The baseline protocol issues exactly one request per attempt (no multi-cycle fix/re-review), so the new lens-execution/full-restart requirement is structurally unreachable in this baseline. Verdicts and findings replay unchanged. Raw per-attempt artifacts are retained outside git (`in_git: false`) and are not present in this worktree, so this is a structural/packet-level verification rather than a literal raw-output byte-diff. - `just format`, `just lint`, and `just test` all pass (227 tests in `review-suite/scripts/tests`, up from 214). - The four bundled `review-suite` copies are byte-identical to the canonical source after `just sync-contracts`. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 6 + review-suite/CONTRACT.md | 49 +- .../contracts/review-result.schema.json | 23 +- .../fixtures/auth-regression/expected.json | 40 +- .../fixtures/behavior-bug/expected.json | 40 +- .../fixtures/clean-change/expected.json | 30 +- .../code-simplicity-clean/expected.json | 7 +- .../expected.json | 44 +- .../fixtures/duplicated-policy/expected.json | 44 +- .../explicit-tests-preserved/expected.json | 7 +- .../fixtures/imagined-machinery/expected.json | 40 +- .../fixtures/missing-evidence/expected.json | 16 +- .../expected.json | 15 +- .../fixtures/missing-test/expected.json | 40 +- .../multi-pass-control-flow/expected.json | 44 +- .../necessary-complexity/expected.json | 7 +- .../repository-convention-clean/expected.json | 7 +- .../fixtures/shared-test-setup/expected.json | 44 +- .../speculative-backfill/expected.json | 47 +- .../unrelated-base-drift/expected.json | 30 +- review-suite/scripts/evals/calibration.py | 2 +- .../scripts/evals/fixture_executor.py | 24 +- review-suite/scripts/tests/test_contracts.py | 159 ++++++ .../scripts/tests/test_eval_protocol.py | 33 +- .../scripts/tests/test_eval_runner.py | 20 +- review-suite/scripts/validate.py | 98 ++++ skills/review-code-change/SKILL.md | 21 +- skills/review-code-change/evals/cases.json | 62 ++- .../evals/expectations.json | 467 ++++++++++++++---- .../expected/standalone-clean.result.json | 25 +- .../references/orchestration-protocol.md | 48 +- .../references/review-suite/CONTRACT.md | 49 +- .../review-suite/review-result.schema.json | 23 +- .../references/review-suite/validate.py | 98 ++++ .../tests/test_orchestration_contract.py | 8 +- .../standalone-duplicated-policy.result.json | 2 +- .../references/review-suite/CONTRACT.md | 49 +- .../review-suite/review-result.schema.json | 23 +- .../references/review-suite/validate.py | 98 ++++ .../standalone-ticket-regression.result.json | 2 +- .../references/review-suite/CONTRACT.md | 49 +- .../review-suite/review-result.schema.json | 23 +- .../references/review-suite/validate.py | 98 ++++ .../standalone-provider-framework.result.json | 2 +- .../untrusted-packet-instruction.result.json | 2 +- .../verified-native-relationship.result.json | 2 +- .../references/review-suite/CONTRACT.md | 49 +- .../review-suite/review-result.schema.json | 23 +- .../references/review-suite/validate.py | 98 ++++ 49 files changed, 1897 insertions(+), 340 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2c5b46..2a327b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,15 @@ summary: Chronological history of repository and skill changes. # Changelog +## 2026-07-28 — Required passing validation and current-head lens evidence for a clean review verdict + +- feat: require passing validation and current-head lens evidence for a clean + review verdict + ## 2026-07-27 — Made database comparison output ephemeral, enforced untrusted-content boundaries, bound epic delegation, hardened command execution, populated the solution-simplicity and code-simplicity strata, enforced acceptance-gated closeout, populated the correctness stratum, recovered carved suffixes, folded owner adjudications, and ran the frozen v1 baseline - fix: keep database comparison output ephemeral by default + (`2f13a2d6c27fda2ced66558460a72c11c4d43c26`) - feat: enforce untrusted content boundaries (`ff3f4b9cca9b062a7113b95ab08bd1d36331a27c`) - docs: record the small-sample caveat the frozen protocol's step 6 requires diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index 87d0908..bdced9f 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -104,8 +104,11 @@ that merely move complexity behind another name. ## Verdict semantics -- `clean`: no `blocking` or `strong_recommendation` finding remains. Deferred - findings may be retained without failing the gate. +- `clean`: no `blocking` or `strong_recommendation` finding remains, every + packet validation entry supplied as required evidence passed, and — for an + aggregate result — every required lens has a fresh, current-head execution + (see "Lens execution evidence" below). Deferred findings may be retained + without failing the gate. - `changes_required`: at least one actionable `blocking` or `strong_recommendation` finding remains. - `blocked`: essential evidence or a product or architecture decision is @@ -118,6 +121,48 @@ fields that the caller could not establish and may preserve already-demonstrated findings, but those findings do not convert the blocked review into a merge verdict. +### Validation must back a `clean` verdict + +A packet's `validation` array is required evidence, not optional context: a +`clean` verdict claims that evidence is trustworthy, so a result must not +declare `clean` while that same packet records a required focused or full +validation entry as `failed` or `unavailable`. Pair validation rejects any +`clean` result paired with such a packet. + +- A `failed` command with a demonstrated candidate-caused failure is a gating + correctness/validation finding and yields `changes_required`, never `clean`. +- A `failed` or `unavailable` command whose attribution or result is + insufficient for a trustworthy verdict yields `blocked` with a concrete reason + and a recorded `validation_limitations` entry, never `clean`. +- Do not invent infrastructure attribution from an exit code alone, and do not + omit a failed or unavailable command from validation evidence to hide it. + +### Lens execution evidence (aggregate results) + +An aggregate result records `lens_executions`: one entry per required lens +(`solution_simplicity`, `correctness`, `code_simplicity`), each naming its +`lens`, `head_sha`, `comparison_base_sha`, `verdict`, and whether it was +`freshly_executed` for this exact aggregate. + +For aggregate `clean`: + +- all three required lenses must be present exactly once, with no missing and no + duplicate entry; +- every entry's `head_sha` and `comparison_base_sha` must equal the aggregate + result's own candidate — a stale-head or stale-base entry cannot contribute to + a new-head aggregate; +- every entry's `verdict` must be `clean`; and +- every entry must be `freshly_executed`; no old-head or reused result may count + toward a new aggregate. + +Any edit, rebase, conflict resolution, or update that changes the head +invalidates every existing lens execution for that head. Restart the complete +three-lens sequence — solution simplicity, correctness, then code simplicity — +after any such head-changing fix; a partial rerun (for example, only correctness +after a correctness fix, or only code simplicity and correctness after a +code-simplicity fix) cannot produce a valid `clean` aggregate. This child +defines no selective-reuse exception across different heads. + ## 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 7dab07c..47e9a38 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.0"}, + "schema_version": {"const": "1.1"}, "lens": { "enum": [ "correctness", @@ -83,6 +83,27 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "lens_executions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "lens", + "head_sha", + "comparison_base_sha", + "verdict", + "freshly_executed" + ], + "properties": { + "lens": {"enum": ["solution_simplicity", "correctness", "code_simplicity"]}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "freshly_executed": {"type": "boolean"} + } + } + }, "validation_limitations": { "type": "array", "items": {"type": "string", "minLength": 1} diff --git a/review-suite/fixtures/auth-regression/expected.json b/review-suite/fixtures/auth-regression/expected.json index cc9584b..73cec56 100644 --- a/review-suite/fixtures/auth-regression/expected.json +++ b/review-suite/fixtures/auth-regression/expected.json @@ -1,21 +1,31 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "correctness", - "candidate": {"head_sha": "8181818181818181818181818181818181818181", "comparison_base_sha": "9191919191919191919191919191919191919191"}, + "candidate": { + "head_sha": "8181818181818181818181818181818181818181", + "comparison_base_sha": "9191919191919191919191919191919191919191" + }, "verdict": "changes_required", - "findings": [{ - "id": "correctness.export-authorization", - "lens": "correctness", - "severity": "blocking", - "confidence": "high", - "rule": "Non-administrators remain forbidden from exporting customer records.", - "evidence": [{"location": "export.py:2", "detail": "The candidate checks only that an actor exists, so any authenticated role can export the record."}], - "concern": "The change removes the role check at the data-export trust boundary.", - "impact": "Ordinary users can access tenant-private customer exports.", - "proposed_change": "Restore the administrator role check and cover a non-administrator denial.", - "expected_effect": "Preserve the required export authorization boundary.", - "location": "export.py:2" - }], + "findings": [ + { + "id": "correctness.export-authorization", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "Non-administrators remain forbidden from exporting customer records.", + "evidence": [ + { + "location": "export.py:2", + "detail": "The candidate checks only that an actor exists, so any authenticated role can export the record." + } + ], + "concern": "The change removes the role check at the data-export trust boundary.", + "impact": "Ordinary users can access tenant-private customer exports.", + "proposed_change": "Restore the administrator role check and cover a non-administrator denial.", + "expected_effect": "Preserve the required export authorization boundary.", + "location": "export.py:2" + } + ], "blocking_reasons": [], "next_action": "Restore role-based authorization and add the missing denial test." } diff --git a/review-suite/fixtures/behavior-bug/expected.json b/review-suite/fixtures/behavior-bug/expected.json index f706168..a45c16f 100644 --- a/review-suite/fixtures/behavior-bug/expected.json +++ b/review-suite/fixtures/behavior-bug/expected.json @@ -1,21 +1,31 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "correctness", - "candidate": {"head_sha": "1111111111111111111111111111111111111111", "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "candidate": { + "head_sha": "1111111111111111111111111111111111111111", + "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + }, "verdict": "changes_required", - "findings": [{ - "id": "correctness.equal-balance", - "lens": "correctness", - "severity": "blocking", - "confidence": "high", - "rule": "A charge equal to the available balance succeeds.", - "evidence": [{"location": "limits.py:2", "detail": "The strict comparison rejects amount equal to balance."}], - "concern": "The new boundary condition contradicts the acceptance criterion.", - "impact": "Customers cannot spend their full available balance.", - "proposed_change": "Use a less-than-or-equal balance comparison and add the equality regression test.", - "expected_effect": "Restore the required boundary behavior without changing overdraft policy.", - "location": "limits.py:2" - }], + "findings": [ + { + "id": "correctness.equal-balance", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "A charge equal to the available balance succeeds.", + "evidence": [ + { + "location": "limits.py:2", + "detail": "The strict comparison rejects amount equal to balance." + } + ], + "concern": "The new boundary condition contradicts the acceptance criterion.", + "impact": "Customers cannot spend their full available balance.", + "proposed_change": "Use a less-than-or-equal balance comparison and add the equality regression test.", + "expected_effect": "Restore the required boundary behavior without changing overdraft policy.", + "location": "limits.py:2" + } + ], "blocking_reasons": [], "next_action": "Correct the comparison and add the missed boundary test." } diff --git a/review-suite/fixtures/clean-change/expected.json b/review-suite/fixtures/clean-change/expected.json index 3491568..859ae77 100644 --- a/review-suite/fixtures/clean-change/expected.json +++ b/review-suite/fixtures/clean-change/expected.json @@ -1,9 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "aggregate", - "candidate": {"head_sha": "5555555555555555555555555555555555555555", "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"}, + "candidate": { + "head_sha": "5555555555555555555555555555555555555555", + "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" + }, "verdict": "clean", "findings": [], "blocking_reasons": [], + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "5555555555555555555555555555555555555555", + "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "5555555555555555555555555555555555555555", + "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "5555555555555555555555555555555555555555", + "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "verdict": "clean", + "freshly_executed": true + } + ], "next_action": "No material review action is required." } diff --git a/review-suite/fixtures/code-simplicity-clean/expected.json b/review-suite/fixtures/code-simplicity-clean/expected.json index 0271b4c..677abca 100644 --- a/review-suite/fixtures/code-simplicity-clean/expected.json +++ b/review-suite/fixtures/code-simplicity-clean/expected.json @@ -1,7 +1,10 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "code_simplicity", - "candidate": {"head_sha": "8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c", "comparison_base_sha": "9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c"}, + "candidate": { + "head_sha": "8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c", + "comparison_base_sha": "9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c" + }, "verdict": "clean", "findings": [], "blocking_reasons": [], diff --git a/review-suite/fixtures/complexity-relocating-wrapper/expected.json b/review-suite/fixtures/complexity-relocating-wrapper/expected.json index 45ab831..b496507 100644 --- a/review-suite/fixtures/complexity-relocating-wrapper/expected.json +++ b/review-suite/fixtures/complexity-relocating-wrapper/expected.json @@ -1,21 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "code_simplicity", - "candidate": {"head_sha": "8989898989898989898989898989898989898989", "comparison_base_sha": "9999999999999999999999999999999999999999"}, + "candidate": { + "head_sha": "8989898989898989898989898989898989898989", + "comparison_base_sha": "9999999999999999999999999999999999999999" + }, "verdict": "changes_required", - "findings": [{ - "id": "code-simplicity.remove-policy-adapter", - "lens": "code_simplicity", - "severity": "strong_recommendation", - "confidence": "high", - "rule": "Prefer direct use of the existing policy owner when a wrapper does not adapt semantics or remove caller knowledge.", - "evidence": [{"location": "access.py:1", "detail": "AccessPolicyAdapter forwards the same actor and resource directly to is_allowed and has one caller."}, {"location": "edit.py:can_edit", "detail": "Nearby code calls policy functions directly when no semantic adaptation exists."}], - "concern": "The adapter moves one call behind a class without removing any branch, parameter, or policy concept.", - "impact": "It adds a public concept, allocation, method, and call-path hop while leaving the caller's knowledge unchanged.", - "proposed_change": "Remove AccessPolicyAdapter and return is_allowed(actor, resource) directly from can_view.", - "expected_effect": "Remove one class, one method, one allocation, and one call hop without changing policy behavior.", - "location": "access.py:1" - }], + "findings": [ + { + "id": "code-simplicity.remove-policy-adapter", + "lens": "code_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Prefer direct use of the existing policy owner when a wrapper does not adapt semantics or remove caller knowledge.", + "evidence": [ + { + "location": "access.py:1", + "detail": "AccessPolicyAdapter forwards the same actor and resource directly to is_allowed and has one caller." + }, + { + "location": "edit.py:can_edit", + "detail": "Nearby code calls policy functions directly when no semantic adaptation exists." + } + ], + "concern": "The adapter moves one call behind a class without removing any branch, parameter, or policy concept.", + "impact": "It adds a public concept, allocation, method, and call-path hop while leaving the caller's knowledge unchanged.", + "proposed_change": "Remove AccessPolicyAdapter and return is_allowed(actor, resource) directly from can_view.", + "expected_effect": "Remove one class, one method, one allocation, and one call hop without changing policy behavior.", + "location": "access.py:1" + } + ], "blocking_reasons": [], "next_action": "Remove the pass-through adapter and retain direct use of is_allowed." } diff --git a/review-suite/fixtures/duplicated-policy/expected.json b/review-suite/fixtures/duplicated-policy/expected.json index 8d57565..e03bda3 100644 --- a/review-suite/fixtures/duplicated-policy/expected.json +++ b/review-suite/fixtures/duplicated-policy/expected.json @@ -1,21 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "code_simplicity", - "candidate": {"head_sha": "2222222222222222222222222222222222222222", "comparison_base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}, + "candidate": { + "head_sha": "2222222222222222222222222222222222222222", + "comparison_base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + }, "verdict": "changes_required", - "findings": [{ - "id": "code-simplicity.reuse-auth-policy", - "lens": "code_simplicity", - "severity": "strong_recommendation", - "confidence": "high", - "rule": "Use the repository's existing active-administrator policy helper.", - "evidence": [{"location": "actions.py:2", "detail": "Both actions repeat the same policy."}, {"location": "auth.py:require_active_admin", "detail": "The repository already centralizes this exact rule."}], - "concern": "The change creates three independently editable definitions of one authorization policy.", - "impact": "Future policy changes can leave these actions inconsistent and increase reviewer cognitive load.", - "proposed_change": "Call require_active_admin from both actions.", - "expected_effect": "Remove duplicated policy while preserving behavior.", - "location": "actions.py:2" - }], + "findings": [ + { + "id": "code-simplicity.reuse-auth-policy", + "lens": "code_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Use the repository's existing active-administrator policy helper.", + "evidence": [ + { + "location": "actions.py:2", + "detail": "Both actions repeat the same policy." + }, + { + "location": "auth.py:require_active_admin", + "detail": "The repository already centralizes this exact rule." + } + ], + "concern": "The change creates three independently editable definitions of one authorization policy.", + "impact": "Future policy changes can leave these actions inconsistent and increase reviewer cognitive load.", + "proposed_change": "Call require_active_admin from both actions.", + "expected_effect": "Remove duplicated policy while preserving behavior.", + "location": "actions.py:2" + } + ], "blocking_reasons": [], "next_action": "Reuse the existing authorization helper and rerun focused tests." } diff --git a/review-suite/fixtures/explicit-tests-preserved/expected.json b/review-suite/fixtures/explicit-tests-preserved/expected.json index 8db8357..060cad4 100644 --- a/review-suite/fixtures/explicit-tests-preserved/expected.json +++ b/review-suite/fixtures/explicit-tests-preserved/expected.json @@ -1,7 +1,10 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "code_simplicity", - "candidate": {"head_sha": "8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b", "comparison_base_sha": "9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b"}, + "candidate": { + "head_sha": "8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b", + "comparison_base_sha": "9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b" + }, "verdict": "clean", "findings": [], "blocking_reasons": [], diff --git a/review-suite/fixtures/imagined-machinery/expected.json b/review-suite/fixtures/imagined-machinery/expected.json index cdee87b..d438342 100644 --- a/review-suite/fixtures/imagined-machinery/expected.json +++ b/review-suite/fixtures/imagined-machinery/expected.json @@ -1,21 +1,31 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "solution_simplicity", - "candidate": {"head_sha": "3333333333333333333333333333333333333333", "comparison_base_sha": "cccccccccccccccccccccccccccccccccccccccc"}, + "candidate": { + "head_sha": "3333333333333333333333333333333333333333", + "comparison_base_sha": "cccccccccccccccccccccccccccccccccccccccc" + }, "verdict": "changes_required", - "findings": [{ - "id": "solution-simplicity.remove-provider-framework", - "lens": "solution_simplicity", - "severity": "blocking", - "confidence": "high", - "rule": "Remote providers and an extension API are explicit non-goals.", - "evidence": [{"location": "export.py:1", "detail": "A registry, factory, provider interface, and selection parameter exist only for unspecified providers."}], - "concern": "The overall design solves imagined provider requirements rather than the requested local-file outcome.", - "impact": "The ticket carries avoidable concepts, configuration, and failure modes.", - "proposed_change": "Replace the provider framework with one direct local-file write at the existing export boundary.", - "expected_effect": "Meet the complete requirement with fewer concepts and no lost capability.", - "location": "export.py:1" - }], + "findings": [ + { + "id": "solution-simplicity.remove-provider-framework", + "lens": "solution_simplicity", + "severity": "blocking", + "confidence": "high", + "rule": "Remote providers and an extension API are explicit non-goals.", + "evidence": [ + { + "location": "export.py:1", + "detail": "A registry, factory, provider interface, and selection parameter exist only for unspecified providers." + } + ], + "concern": "The overall design solves imagined provider requirements rather than the requested local-file outcome.", + "impact": "The ticket carries avoidable concepts, configuration, and failure modes.", + "proposed_change": "Replace the provider framework with one direct local-file write at the existing export boundary.", + "expected_effect": "Meet the complete requirement with fewer concepts and no lost capability.", + "location": "export.py:1" + } + ], "blocking_reasons": [], "next_action": "Redesign around the required local-file operation before detailed review continues." } diff --git a/review-suite/fixtures/missing-evidence/expected.json b/review-suite/fixtures/missing-evidence/expected.json index 255fc93..42a35ba 100644 --- a/review-suite/fixtures/missing-evidence/expected.json +++ b/review-suite/fixtures/missing-evidence/expected.json @@ -1,10 +1,18 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "aggregate", - "candidate": {"head_sha": "6666666666666666666666666666666666666666", "comparison_base_sha": "ffffffffffffffffffffffffffffffffffffffff"}, + "candidate": { + "head_sha": "6666666666666666666666666666666666666666", + "comparison_base_sha": "ffffffffffffffffffffffffffffffffffffffff" + }, "verdict": "blocked", "findings": [], - "blocking_reasons": ["Acceptance criteria are missing.", "Required validation is unavailable without a reason."], - "validation_limitations": ["The required test result cannot be evaluated."], + "blocking_reasons": [ + "Acceptance criteria are missing.", + "Required validation is unavailable without a reason." + ], + "validation_limitations": [ + "The required test result cannot be evaluated." + ], "next_action": "Supply acceptance criteria and the validation result or unavailable reason." } diff --git a/review-suite/fixtures/missing-simplification-requirements/expected.json b/review-suite/fixtures/missing-simplification-requirements/expected.json index 1617b4c..b4a9d19 100644 --- a/review-suite/fixtures/missing-simplification-requirements/expected.json +++ b/review-suite/fixtures/missing-simplification-requirements/expected.json @@ -1,10 +1,17 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "solution_simplicity", - "candidate": {"head_sha": "8686868686868686868686868686868686868686", "comparison_base_sha": "9696969696969696969696969696969696969696"}, + "candidate": { + "head_sha": "8686868686868686868686868686868686868686", + "comparison_base_sha": "9696969696969696969696969696969696969696" + }, "verdict": "blocked", "findings": [], - "blocking_reasons": ["The deployment topology, available durable storage primitive, and required retention and availability are missing, so no smaller design can be shown to satisfy retrieval after worker exit."], - "validation_limitations": ["Passing fake-backend tests do not resolve which persistence behavior is required in the real deployment."], + "blocking_reasons": [ + "The deployment topology, available durable storage primitive, and required retention and availability are missing, so no smaller design can be shown to satisfy retrieval after worker exit." + ], + "validation_limitations": [ + "Passing fake-backend tests do not resolve which persistence behavior is required in the real deployment." + ], "next_action": "Decide the deployment storage boundary and durability requirements, then compare the candidate with the smallest repository-supported implementation." } diff --git a/review-suite/fixtures/missing-test/expected.json b/review-suite/fixtures/missing-test/expected.json index 6ca1053..d392f15 100644 --- a/review-suite/fixtures/missing-test/expected.json +++ b/review-suite/fixtures/missing-test/expected.json @@ -1,21 +1,31 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "correctness", - "candidate": {"head_sha": "8282828282828282828282828282828282828282", "comparison_base_sha": "9292929292929292929292929292929292929292"}, + "candidate": { + "head_sha": "8282828282828282828282828282828282828282", + "comparison_base_sha": "9292929292929292929292929292929292929292" + }, "verdict": "changes_required", - "findings": [{ - "id": "correctness.invalid-count-test", - "lens": "correctness", - "severity": "blocking", - "confidence": "high", - "rule": "Non-numeric input raises InvalidCount.", - "evidence": [{"location": "test_parser.py:1", "detail": "Both tests cover numeric success; neither executes the ticket's required failure behavior."}], - "concern": "The only new acceptance behavior is not exercised by validation.", - "impact": "The domain-error contract can regress while all supplied tests remain green.", - "proposed_change": "Replace the redundant larger-count test with a non-numeric input assertion for InvalidCount and its input.", - "expected_effect": "Make the required error conversion executable and regression-protected.", - "location": "test_parser.py:4" - }], + "findings": [ + { + "id": "correctness.invalid-count-test", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "Non-numeric input raises InvalidCount.", + "evidence": [ + { + "location": "test_parser.py:1", + "detail": "Both tests cover numeric success; neither executes the ticket's required failure behavior." + } + ], + "concern": "The only new acceptance behavior is not exercised by validation.", + "impact": "The domain-error contract can regress while all supplied tests remain green.", + "proposed_change": "Replace the redundant larger-count test with a non-numeric input assertion for InvalidCount and its input.", + "expected_effect": "Make the required error conversion executable and regression-protected.", + "location": "test_parser.py:4" + } + ], "blocking_reasons": [], "next_action": "Add the acceptance-path failure test and rerun focused and full validation." } diff --git a/review-suite/fixtures/multi-pass-control-flow/expected.json b/review-suite/fixtures/multi-pass-control-flow/expected.json index 9b70b3b..59eba66 100644 --- a/review-suite/fixtures/multi-pass-control-flow/expected.json +++ b/review-suite/fixtures/multi-pass-control-flow/expected.json @@ -1,21 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "code_simplicity", - "candidate": {"head_sha": "8888888888888888888888888888888888888888", "comparison_base_sha": "9898989898989898989898989898989898989898"}, + "candidate": { + "head_sha": "8888888888888888888888888888888888888888", + "comparison_base_sha": "9898989898989898989898989898989898989898" + }, "verdict": "changes_required", - "findings": [{ - "id": "code-simplicity.use-one-pass-total", - "lens": "code_simplicity", - "severity": "strong_recommendation", - "confidence": "high", - "rule": "Use the repository's one-pass accumulation pattern when intermediate collections have no required meaning.", - "evidence": [{"location": "totals.py:2", "detail": "The candidate materializes four sequential collections used only by the next phase."}, {"location": "credits.py:credit_total", "detail": "The nearby primitive normalizes once and accumulates qualifying rounded values directly."}], - "concern": "Four named phases and collections obscure one local filter-and-sum operation.", - "impact": "Reviewers must track five representations of the same rows and the runtime allocates four unnecessary collections.", - "proposed_change": "Use one direct loop that normalizes each raw row once, skips non-billable rows, and adds round_cents(price(row)) to the total.", - "expected_effect": "Reduce four passes and four intermediate representations to one pass and one accumulator while preserving normalization and rounding behavior.", - "location": "totals.py:2" - }], + "findings": [ + { + "id": "code-simplicity.use-one-pass-total", + "lens": "code_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Use the repository's one-pass accumulation pattern when intermediate collections have no required meaning.", + "evidence": [ + { + "location": "totals.py:2", + "detail": "The candidate materializes four sequential collections used only by the next phase." + }, + { + "location": "credits.py:credit_total", + "detail": "The nearby primitive normalizes once and accumulates qualifying rounded values directly." + } + ], + "concern": "Four named phases and collections obscure one local filter-and-sum operation.", + "impact": "Reviewers must track five representations of the same rows and the runtime allocates four unnecessary collections.", + "proposed_change": "Use one direct loop that normalizes each raw row once, skips non-billable rows, and adds round_cents(price(row)) to the total.", + "expected_effect": "Reduce four passes and four intermediate representations to one pass and one accumulator while preserving normalization and rounding behavior.", + "location": "totals.py:2" + } + ], "blocking_reasons": [], "next_action": "Replace the phase pipeline with the repository's direct accumulation pattern and rerun total tests." } diff --git a/review-suite/fixtures/necessary-complexity/expected.json b/review-suite/fixtures/necessary-complexity/expected.json index 8c9e752..b8fee7b 100644 --- a/review-suite/fixtures/necessary-complexity/expected.json +++ b/review-suite/fixtures/necessary-complexity/expected.json @@ -1,7 +1,10 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "solution_simplicity", - "candidate": {"head_sha": "4444444444444444444444444444444444444444", "comparison_base_sha": "dddddddddddddddddddddddddddddddddddddddd"}, + "candidate": { + "head_sha": "4444444444444444444444444444444444444444", + "comparison_base_sha": "dddddddddddddddddddddddddddddddddddddddd" + }, "verdict": "clean", "findings": [], "blocking_reasons": [], diff --git a/review-suite/fixtures/repository-convention-clean/expected.json b/review-suite/fixtures/repository-convention-clean/expected.json index fc6adfe..aaea4c2 100644 --- a/review-suite/fixtures/repository-convention-clean/expected.json +++ b/review-suite/fixtures/repository-convention-clean/expected.json @@ -1,7 +1,10 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "correctness", - "candidate": {"head_sha": "8383838383838383838383838383838383838383", "comparison_base_sha": "9393939393939393939393939393939393939393"}, + "candidate": { + "head_sha": "8383838383838383838383838383838383838383", + "comparison_base_sha": "9393939393939393939393939393939393939393" + }, "verdict": "clean", "findings": [], "blocking_reasons": [], diff --git a/review-suite/fixtures/shared-test-setup/expected.json b/review-suite/fixtures/shared-test-setup/expected.json index bf541d5..f9ecb40 100644 --- a/review-suite/fixtures/shared-test-setup/expected.json +++ b/review-suite/fixtures/shared-test-setup/expected.json @@ -1,21 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "code_simplicity", - "candidate": {"head_sha": "8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a", "comparison_base_sha": "9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a"}, + "candidate": { + "head_sha": "8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a", + "comparison_base_sha": "9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a" + }, "verdict": "changes_required", - "findings": [{ - "id": "code-simplicity.reuse-uploader-builder", - "lens": "code_simplicity", - "severity": "strong_recommendation", - "confidence": "high", - "rule": "Reuse the repository's uploader builder for irrelevant construction while keeping materially different boundary scenarios explicit.", - "evidence": [{"location": "test_uploads.py", "detail": "All three tests repeat the same FakeStore, FixedClock, and Uploader construction."}, {"location": "tests/helpers.py:make_uploader", "detail": "The existing builder supplies exactly that setup with a configurable byte limit."}], - "concern": "Repeated irrelevant construction obscures the below, equal, and above-limit scenarios.", - "impact": "The same setup policy appears three times and must be reviewed independently without adding scenario meaning.", - "proposed_change": "Call make_uploader(max_bytes=10) in each named test while retaining the three separate actions and expectations.", - "expected_effect": "Reduce three copies of construction policy to one shared helper while preserving three explicit boundary tests.", - "location": "test_uploads.py" - }], + "findings": [ + { + "id": "code-simplicity.reuse-uploader-builder", + "lens": "code_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Reuse the repository's uploader builder for irrelevant construction while keeping materially different boundary scenarios explicit.", + "evidence": [ + { + "location": "test_uploads.py", + "detail": "All three tests repeat the same FakeStore, FixedClock, and Uploader construction." + }, + { + "location": "tests/helpers.py:make_uploader", + "detail": "The existing builder supplies exactly that setup with a configurable byte limit." + } + ], + "concern": "Repeated irrelevant construction obscures the below, equal, and above-limit scenarios.", + "impact": "The same setup policy appears three times and must be reviewed independently without adding scenario meaning.", + "proposed_change": "Call make_uploader(max_bytes=10) in each named test while retaining the three separate actions and expectations.", + "expected_effect": "Reduce three copies of construction policy to one shared helper while preserving three explicit boundary tests.", + "location": "test_uploads.py" + } + ], "blocking_reasons": [], "next_action": "Reuse make_uploader for setup and keep the boundary tests separate." } diff --git a/review-suite/fixtures/speculative-backfill/expected.json b/review-suite/fixtures/speculative-backfill/expected.json index da9eadc..1a335b3 100644 --- a/review-suite/fixtures/speculative-backfill/expected.json +++ b/review-suite/fixtures/speculative-backfill/expected.json @@ -1,24 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "solution_simplicity", - "candidate": {"head_sha": "8585858585858585858585858585858585858585", "comparison_base_sha": "9595959595959595959595959595959595959595"}, + "candidate": { + "head_sha": "8585858585858585858585858585858585858585", + "comparison_base_sha": "9595959595959595959595959595959595959595" + }, "verdict": "changes_required", - "findings": [{ - "id": "solution-simplicity.remove-speculative-timezone-migration", - "lens": "solution_simplicity", - "severity": "blocking", - "confidence": "high", - "rule": "Historical migration and legacy-field compatibility are explicit non-goals, and no deployed data or callers exist.", - "evidence": [ - {"location": "timezones.py:1", "detail": "The candidate adds a feature flag, dual reads, dual writes, and a backfill solely for legacy timezone state."}, - {"location": "README.md", "detail": "The settings service is unreleased and has no production records to migrate."} - ], - "concern": "The solution carries a migration protocol for historical state that cannot exist.", - "impact": "It adds configuration, duplicate fields, a backfill lifecycle, and divergent read paths without satisfying any current requirement.", - "proposed_change": "Store one timezone field directly on new settings records and default it to UTC when absent; remove the flag, legacy field path, dual write, and backfill.", - "expected_effect": "Preserve all required timezone behavior while eliminating the migration state, compatibility branch, and duplicate source of truth.", - "location": "timezones.py:1" - }], + "findings": [ + { + "id": "solution-simplicity.remove-speculative-timezone-migration", + "lens": "solution_simplicity", + "severity": "blocking", + "confidence": "high", + "rule": "Historical migration and legacy-field compatibility are explicit non-goals, and no deployed data or callers exist.", + "evidence": [ + { + "location": "timezones.py:1", + "detail": "The candidate adds a feature flag, dual reads, dual writes, and a backfill solely for legacy timezone state." + }, + { + "location": "README.md", + "detail": "The settings service is unreleased and has no production records to migrate." + } + ], + "concern": "The solution carries a migration protocol for historical state that cannot exist.", + "impact": "It adds configuration, duplicate fields, a backfill lifecycle, and divergent read paths without satisfying any current requirement.", + "proposed_change": "Store one timezone field directly on new settings records and default it to UTC when absent; remove the flag, legacy field path, dual write, and backfill.", + "expected_effect": "Preserve all required timezone behavior while eliminating the migration state, compatibility branch, and duplicate source of truth.", + "location": "timezones.py:1" + } + ], "blocking_reasons": [], "next_action": "Replace the speculative migration layer with the direct timezone field before detailed review continues." } diff --git a/review-suite/fixtures/unrelated-base-drift/expected.json b/review-suite/fixtures/unrelated-base-drift/expected.json index 93b9959..4b9f523 100644 --- a/review-suite/fixtures/unrelated-base-drift/expected.json +++ b/review-suite/fixtures/unrelated-base-drift/expected.json @@ -1,9 +1,35 @@ { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "aggregate", - "candidate": {"head_sha": "7777777777777777777777777777777777777777", "comparison_base_sha": "1212121212121212121212121212121212121212"}, + "candidate": { + "head_sha": "7777777777777777777777777777777777777777", + "comparison_base_sha": "1212121212121212121212121212121212121212" + }, "verdict": "clean", "findings": [], "blocking_reasons": [], + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "7777777777777777777777777777777777777777", + "comparison_base_sha": "1212121212121212121212121212121212121212", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "7777777777777777777777777777777777777777", + "comparison_base_sha": "1212121212121212121212121212121212121212", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "7777777777777777777777777777777777777777", + "comparison_base_sha": "1212121212121212121212121212121212121212", + "verdict": "clean", + "freshly_executed": true + } + ], "next_action": "Retain the head-bound evidence and record the unrelated-base-drift justification." } diff --git a/review-suite/scripts/evals/calibration.py b/review-suite/scripts/evals/calibration.py index e18d245..3a39b95 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.0", + "schema_version": "1.1", "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 666b80d..ade073b 100644 --- a/review-suite/scripts/evals/fixture_executor.py +++ b/review-suite/scripts/evals/fixture_executor.py @@ -69,15 +69,35 @@ def _finding( } +REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") + + +def _clean_lens_executions(candidate: dict[str, Any]) -> list[dict[str, Any]]: + """Fresh current-head/current-base clean evidence for every required lens.""" + return [ + { + "lens": lens, + "head_sha": candidate["head_sha"], + "comparison_base_sha": candidate["comparison_base_sha"], + "verdict": "clean", + "freshly_executed": True, + } + for lens in REQUIRED_AGGREGATE_LENSES + ] + + def _review(candidate: dict[str, Any], verdict: str, findings: list[dict[str, Any]]): - return { - "schema_version": "1.0", + result = { + "schema_version": "1.1", "lens": "aggregate", "candidate": dict(candidate), "verdict": verdict, "findings": findings, "blocking_reasons": [], } + if verdict == "clean": + result["lens_executions"] = _clean_lens_executions(candidate) + return result def _ledger_review(candidate: dict[str, Any]) -> dict[str, Any]: diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index 77198bf..2e80c1c 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -267,5 +267,164 @@ def test_malformed_pair_candidate_returns_errors(self): self.assertTrue(VALIDATOR.validate_pair(packet, result)) +class CleanRequiresPassingValidationTests(unittest.TestCase): + """#51: a `clean` verdict must prove its own packet validation passed. + + Pair validation previously never cross-checked packet validation status + against verdict: a schema-valid `clean` result could pair with every + focused and full validation entry set to `failed`. + """ + + def setUp(self): + self.packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") + self.result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + + def _index(self, scope: str) -> int: + return next( + index + for index, validation in enumerate(self.packet["validation"]) + if validation["scope"] == scope + ) + + def test_failed_focused_validation_invalidates_a_clean_pair(self): + index = self._index("focused") + self.packet["validation"][index]["status"] = "failed" + self.packet["validation"][index]["result"] = "1 failed" + errors = VALIDATOR.validate_pair(self.packet, self.result) + self.assertTrue(errors) + self.assertTrue( + any("clean cannot pair with failed" in error for error in errors) + ) + + def test_failed_full_validation_invalidates_a_clean_pair(self): + index = self._index("full") + self.packet["validation"][index]["status"] = "failed" + self.packet["validation"][index]["result"] = "1 failed" + errors = VALIDATOR.validate_pair(self.packet, self.result) + self.assertTrue(errors) + self.assertTrue( + any("clean cannot pair with failed" in error for error in errors) + ) + + def test_unavailable_validation_invalidates_a_clean_pair(self): + index = self._index("full") + self.packet["validation"][index] = { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "unavailable", + "reason": "The sandboxed runtime has no network access.", + } + errors = VALIDATOR.validate_pair(self.packet, self.result) + self.assertTrue(errors) + self.assertTrue( + any("clean cannot pair with unavailable" in error for error in errors) + ) + + def test_all_required_validation_passed_produces_a_valid_clean_pair(self): + self.assertEqual([], VALIDATOR.validate_pair(self.packet, self.result)) + + +class AggregateLensExecutionEvidenceTests(unittest.TestCase): + """#51: aggregate `clean` requires one fresh, current-head lens execution + for every required lens, so a new-head aggregate cannot smuggle in an + old-head or missing lens result.""" + + def setUp(self): + self.result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + + def test_missing_required_lens_execution_invalidates_clean_aggregate(self): + self.result["lens_executions"] = [ + execution + for execution in self.result["lens_executions"] + if execution["lens"] != "solution_simplicity" + ] + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue( + any("missing required lens execution" in error for error in errors) + ) + + def test_duplicate_required_lens_execution_invalidates_clean_aggregate(self): + self.result["lens_executions"].append( + copy.deepcopy(self.result["lens_executions"][0]) + ) + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue(any("duplicate lens execution" in error for error in errors)) + + def test_stale_head_lens_execution_invalidates_clean_aggregate(self): + self.result["lens_executions"][0]["head_sha"] = "9" * 40 + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue(any("stale head or base" in error for error in errors)) + + def test_stale_base_lens_execution_invalidates_clean_aggregate(self): + self.result["lens_executions"][0]["comparison_base_sha"] = "9" * 40 + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue(any("stale head or base" in error for error in errors)) + + def test_fresh_current_head_clean_executions_produce_a_valid_clean_aggregate(self): + self.assertEqual([], VALIDATOR.validate_result(self.result)) + + +class NewHeadFullRestartTests(unittest.TestCase): + """#51 items 8-9: any head-changing fix restarts the full three-lens + sequence. The pre-#51 re-review matrix let a correctness fix or a + code-simplicity fix reach a new-head aggregate with only two of the three + required lenses re-executed; both must now fail validation.""" + + def setUp(self): + self.result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + + def _drop_solution_simplicity(self): + self.result["lens_executions"] = [ + execution + for execution in self.result["lens_executions"] + if execution["lens"] != "solution_simplicity" + ] + + def test_new_head_after_correctness_fix_requires_all_three_executions(self): + # Pre-#51 matrix: "correctness fix -> correctness and downstream + # lenses", which never re-ran solution simplicity on the new head. + self._drop_solution_simplicity() + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue(any("solution_simplicity" in error for error in errors)) + + def test_new_head_after_code_simplicity_fix_requires_all_three_executions(self): + # Pre-#51 matrix: "code-simplicity fix -> code, then targeted + # correctness", which never re-ran solution simplicity on the new head. + self._drop_solution_simplicity() + errors = VALIDATOR.validate_result(self.result) + self.assertTrue(errors) + self.assertTrue(any("solution_simplicity" in error for error in errors)) + + +class UnchangedHeadBaseDriftRegressionTest(unittest.TestCase): + """#51 item 10: unchanged-head base drift keeps the existing retain rules, + unaffected by the validation and lens-execution repairs above.""" + + def test_unrelated_base_drift_remains_a_valid_clean_pair_at_v1_1(self): + packet = load(ROOT / "fixtures" / "unrelated-base-drift" / "packet.json") + result = load(ROOT / "fixtures" / "unrelated-base-drift" / "expected.json") + self.assertEqual("retain", packet["base_drift"]["decision"]) + self.assertEqual([], VALIDATOR.validate_pair(packet, result)) + + +class StaleSchemaVersionTests(unittest.TestCase): + """#51 item 11: a stale v1.0 result is rejected with a useful error rather + than silently reinterpreted as v1.1 evidence.""" + + def test_stale_v1_0_aggregate_result_is_rejected_with_a_useful_error(self): + result = load(ROOT / "fixtures" / "clean-change" / "expected.json") + result["schema_version"] = "1.0" + errors = VALIDATOR.validate_result(result) + self.assertTrue(errors) + self.assertTrue(any("stale v1.0" in error for error in errors)) + self.assertTrue(any("1.1" in error for error in errors)) + + if __name__ == "__main__": unittest.main() diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index 2e46371..5570408 100644 --- a/review-suite/scripts/tests/test_eval_protocol.py +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -369,19 +369,31 @@ def _valid_response(self, verdict="changes_required"): "proposed_change": "Meet the requirement.", "expected_effect": "The requirement is met.", } + result = { + "schema_version": "1.1", + "lens": "aggregate", + "candidate": candidate, + "verdict": verdict, + "findings": [finding] if verdict == "changes_required" else [], + "blocking_reasons": [], + } + if verdict == "clean": + result["lens_executions"] = [ + { + "lens": lens, + "head_sha": candidate["head_sha"], + "comparison_base_sha": candidate["comparison_base_sha"], + "verdict": "clean", + "freshly_executed": True, + } + for lens in ("solution_simplicity", "correctness", "code_simplicity") + ] return { "protocol_version": protocol.PROTOCOL_VERSION, "outcome": "review_result", "simulation": False, "executor": {"name": "test"}, - "result": { - "schema_version": "1.0", - "lens": "aggregate", - "candidate": candidate, - "verdict": verdict, - "findings": [finding] if verdict == "changes_required" else [], - "blocking_reasons": [], - }, + "result": result, } def classify(self, response): @@ -447,7 +459,7 @@ def test_valid_blocked_result_is_its_own_status(self): "simulation": False, "executor": {"name": "test"}, "result": { - "schema_version": "1.0", + "schema_version": "1.1", "lens": "aggregate", "candidate": { "head_sha": blocked_case.packet["candidate"]["head_sha"], @@ -488,6 +500,9 @@ def test_a_merge_verdict_on_an_incomplete_packet_is_a_review_result(self): with self.subTest(verdict=verdict): response = self._valid_response(verdict) response["result"]["candidate"] = candidate + for execution in response["result"].get("lens_executions", []): + execution["head_sha"] = candidate["head_sha"] + execution["comparison_base_sha"] = candidate["comparison_base_sha"] status, _, detail = protocol.classify_response( blocked_case.packet, json.dumps(response) ) diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index 1acf6f6..0e99ed9 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -223,7 +223,7 @@ def test_a_merge_verdict_on_an_incomplete_packet_is_graded_not_failed(self): ' "outcome": "review_result", "simulation": False,\n' ' "executor": {"name": "always-gating"},\n' ' "result": {\n' - ' "schema_version": "1.0", "lens": "aggregate",\n' + ' "schema_version": "1.1", "lens": "aggregate",\n' ' "candidate": request["run"]["candidate"],\n' ' "verdict": "changes_required", "findings": [finding],\n' ' "blocking_reasons": [],\n' @@ -549,10 +549,17 @@ def test_the_adapter_completes_an_evaluation_through_the_protocol(self): "import json, sys\n" "prompt = sys.stdin.read()\n" "candidate = json.loads(prompt.strip().splitlines()[-1])\n" + "lens_executions = [\n" + ' {"lens": lens, "head_sha": candidate["head_sha"],\n' + ' "comparison_base_sha": candidate["comparison_base_sha"],\n' + ' "verdict": "clean", "freshly_executed": True}\n' + ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' + "]\n" "result = {\n" - ' "schema_version": "1.0", "lens": "aggregate",\n' + ' "schema_version": "1.1", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' + ' "lens_executions": lens_executions,\n' "}\n" "json.dump({\n" ' "result": json.dumps(result),\n' @@ -605,10 +612,17 @@ def _envelope_stub(self, envelope_body: str) -> Path: "import json, sys\n" "prompt = sys.stdin.read()\n" "candidate = json.loads(prompt.strip().splitlines()[-1])\n" + "lens_executions = [\n" + ' {"lens": lens, "head_sha": candidate["head_sha"],\n' + ' "comparison_base_sha": candidate["comparison_base_sha"],\n' + ' "verdict": "clean", "freshly_executed": True}\n' + ' for lens in ("solution_simplicity", "correctness", "code_simplicity")\n' + "]\n" "result = {\n" - ' "schema_version": "1.0", "lens": "aggregate",\n' + ' "schema_version": "1.1", "lens": "aggregate",\n' ' "candidate": candidate, "verdict": "clean",\n' ' "findings": [], "blocking_reasons": [],\n' + ' "lens_executions": lens_executions,\n' "}\n" f"envelope = {envelope_body}\n" 'envelope["result"] = json.dumps(result)\n' diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index 50e0b12..033b586 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -33,6 +33,10 @@ def _schema_file(name: str) -> Path: "result": _schema_file("review-result.schema.json"), } +REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") + +STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} + BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( r"^\$: missing required property " @@ -161,6 +165,14 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: + if ( + isinstance(result, dict) + and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS + ): + return [ + "$.schema_version: stale v1.0 result rejected; v1.0 results are not " + "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -217,6 +229,63 @@ def validate_result(result: dict[str, Any]) -> list[str]: "$.proposal_dispositions: duplicate finding id(s): " + ", ".join(duplicate_dispositions) ) + + if result.get("lens") == "aggregate" and verdict == "clean": + errors.extend(_check_aggregate_clean_lens_executions(result)) + 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. + + An aggregate `clean` is only trustworthy when every required lens actually + completed against the exact aggregate candidate. This closes the gap where + a new-head aggregate could be reached without a fresh solution-simplicity, + correctness, or code-simplicity execution for that exact head, and rejects + any old-head or old-base execution smuggled into a new aggregate. + """ + errors: list[str] = [] + candidate = result.get("candidate") + head = candidate.get("head_sha") if isinstance(candidate, dict) else None + base = candidate.get("comparison_base_sha") if isinstance(candidate, dict) else None + executions = result.get("lens_executions") + if not isinstance(executions, list) or not executions: + return ["$.lens_executions: aggregate clean requires lens execution evidence"] + + seen: list[str] = [] + for index, execution in enumerate(executions): + if not isinstance(execution, dict): + continue + lens_name = execution.get("lens") + seen.append(lens_name) + at = f"$.lens_executions[{index}]" + if ( + execution.get("head_sha") != head + or execution.get("comparison_base_sha") != base + ): + errors.append( + f"{at}: stale head or base cannot contribute to a new-head " + "aggregate clean" + ) + if execution.get("verdict") != "clean": + errors.append(f"{at}: aggregate clean requires a clean lens execution") + if execution.get("freshly_executed") is not True: + errors.append( + f"{at}: aggregate clean requires a freshly executed lens result" + ) + + missing = [lens for lens in REQUIRED_AGGREGATE_LENSES if lens not in seen] + if missing: + errors.append( + "$.lens_executions: aggregate clean is missing required lens " + "execution(s): " + ", ".join(missing) + ) + duplicates = sorted({lens for lens in seen if lens and seen.count(lens) > 1}) + if duplicates: + errors.append( + "$.lens_executions: aggregate clean has duplicate lens execution(s): " + + ", ".join(duplicates) + ) return errors @@ -253,6 +322,35 @@ def validate_pair(packet: dict[str, Any], result: dict[str, Any]) -> list[str]: errors.append(f"candidate.{field}: result omits identity present in packet") elif packet_has_field and packet_candidate[field] != result_candidate[field]: errors.append(f"candidate.{field}: result does not match packet") + errors.extend(_check_clean_requires_passing_validation(packet, result)) + return errors + + +def _check_clean_requires_passing_validation( + packet: dict[str, Any], result: dict[str, Any] +) -> list[str]: + """Reject a `clean` verdict paired with failed or unavailable validation. + + A schema-valid `clean` result previously did not prove that the packet's + own required focused and full validation actually passed: every entry + could be `failed` and pair validation raised no error. `clean` must not + hide a failed or unavailable required command. + """ + if result.get("verdict") != "clean": + return [] + validations = packet.get("validation") + if not isinstance(validations, list): + return [] + errors: list[str] = [] + for index, validation in enumerate(validations): + if not isinstance(validation, dict): + continue + status = validation.get("status") + if status in {"failed", "unavailable"}: + errors.append( + f"validation[{index}]: clean cannot pair with {status} " + "required validation" + ) return errors diff --git a/skills/review-code-change/SKILL.md b/skills/review-code-change/SKILL.md index b9401c7..43ff04a 100644 --- a/skills/review-code-change/SKILL.md +++ b/skills/review-code-change/SKILL.md @@ -65,11 +65,11 @@ lenses ends the review. The orchestrator is read-only. Return the required fix and next lens; the caller applies changes and supplies a new packet bound to the new head. -- After solution redesign, restart the full sequence. -- After a correctness fix, rerun correctness and every downstream lens whose - assumptions changed. -- After a code-simplicity fix, rerun code simplicity and then targeted - correctness. +- After solution redesign, a correctness fix, or a code-simplicity fix, restart + the complete three-lens sequence — solution simplicity, correctness, then code + simplicity — on the new head. No old-head lens result may contribute to the + new-head aggregate; a partial rerun cannot produce a valid `clean` aggregate + (see the shared contract's lens execution evidence requirement). - Use at most three full fix/re-review cycles by default. On the final cycle, return unresolved material findings without requesting another automatic cycle. @@ -90,7 +90,16 @@ limitations, and the next required action. - `changes_required`: a blocking or strong-recommendation finding remains. - `blocked`: evidence, a required dependency, or a lens verdict is untrustworthy. -- `clean`: every required lens completed and no gating finding remains. +- `clean`: every required lens completed and no gating finding remains, every + required packet validation entry passed, and `lens_executions` records one + fresh, current-head, `clean` execution for each of solution simplicity, + correctness, and code simplicity. + +For every `clean` aggregate, populate `lens_executions` from the exact lens +results just validated: each entry names its lens, the aggregate's own head and +comparison-base SHA, its verdict, and `freshly_executed: true`. Never count an +unavailable, skipped, or old-head lens result as clean, and never reuse a prior +aggregate's `lens_executions` for a new head. Never count an unavailable or skipped required lens as clean. diff --git a/skills/review-code-change/evals/cases.json b/skills/review-code-change/evals/cases.json index 00786b3..5d30aed 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.0", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.1", "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.0", "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.0", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "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.1", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.1", "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.0", "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.0", "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.0", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "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.1", "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.1", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -38,19 +38,31 @@ "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": { - "solution_simplicity": {"schema_version": "1.0", "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.0", "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.0", "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.1", "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.1", "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.1", "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": []} } }, { - "id": "targeted-correctness-recheck", + "id": "full-restart-after-code-simplicity-fix", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "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", + "evidence": {"complete": true, "new_packet_after_code_simplification": true}, "cycle": 2, "phase": "post_code_simplification_full_restart", "harness_outcomes": { - "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "full-restart-after-correctness-fix", + "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, + "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.1", "lens": "solution_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "5151515151515151515151515151515151515151", "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -72,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.0", "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.0", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "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.1", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.1", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -83,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.0", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.0", "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.0", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.1", "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.1", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} } }, { @@ -94,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.0", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, - "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + "solution_simplicity": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.1", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.1", "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 8da0af4..18e83da 100644 --- a/skills/review-code-change/evals/expectations.json +++ b/skills/review-code-change/evals/expectations.json @@ -1,94 +1,245 @@ [ { "case_id": "ordered-clean", - "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "observed_sequence": [ + "solution_simplicity", + "correctness", + "code_simplicity" + ], "stopped_reason": "all_required_lenses_clean", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, - "verdict": "clean", "findings": [], "blocking_reasons": [], - "next_action": "No further review action is required." + "schema_version": "1.1", + "lens": "aggregate", + "candidate": { + "head_sha": "1010101010101010101010101010101010101010", + "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0" + }, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "next_action": "No further review action is required.", + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "1010101010101010101010101010101010101010", + "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "1010101010101010101010101010101010101010", + "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "1010101010101010101010101010101010101010", + "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0", + "verdict": "clean", + "freshly_executed": true + } + ] } }, { "case_id": "early-solution-redesign", - "observed_sequence": ["solution_simplicity"], + "observed_sequence": [ + "solution_simplicity" + ], "stopped_reason": "solution_strategy_replacement_required", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, + "schema_version": "1.1", + "lens": "aggregate", + "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 implementation introduces a provider platform even though the scoped behavior requires only one local write.", - "impact": "The unnecessary strategy adds configuration, dispatch, and provider failure modes outside the change contract.", - "proposed_change": "Replace the provider platform with one direct local write.", - "expected_effect": "Remove the platform, configuration, and provider failure modes." - }], + "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 implementation introduces a provider platform even though the scoped behavior requires only one local write.", + "impact": "The unnecessary strategy adds configuration, dispatch, and provider failure modes outside the change contract.", + "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, capture a new head-bound evidence packet, and restart the full review sequence." } }, { "case_id": "correctness-overrides-simplification", - "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "observed_sequence": [ + "solution_simplicity", + "correctness", + "code_simplicity" + ], "stopped_reason": "unsafe_simplification_rejected_and_remaining_lenses_clean", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, - "verdict": "clean", "findings": [], + "schema_version": "1.1", + "lens": "aggregate", + "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": "Keep the existing claim fencing; no candidate change or further review action is required." + "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": "Keep the existing claim fencing; no candidate change or further review action is required.", + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "3030303030303030303030303030303030303030", + "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "3030303030303030303030303030303030303030", + "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "3030303030303030303030303030303030303030", + "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0", + "verdict": "clean", + "freshly_executed": true + } + ] } }, { "case_id": "deduplicate-overlap", - "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "observed_sequence": [ + "solution_simplicity", + "correctness", + "code_simplicity" + ], "stopped_reason": "code_simplicity_gating_finding", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, + "schema_version": "1.1", + "lens": "aggregate", + "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": "solution_simplicity result", "detail": "A new service duplicates the active-admin rule."}, - {"location": "code_simplicity result", "detail": "Two new actions repeat the same active-admin condition."} - ], - "concern": "The new service and actions duplicate the existing active-admin authorization policy.", - "impact": "The candidate maintains three policy definitions and adds an unnecessary service.", - "proposed_change": "Call require_active_admin directly and remove the duplicate local policy service and conditions.", - "expected_effect": "Reduce three policy definitions to one and remove the duplicate service.", - "related_finding_ids": ["code-simplicity.reuse-active-admin"] - }], + "findings": [ + { + "id": "solution-simplicity.remove-local-policy-service", + "lens": "solution_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Use the existing authorization boundary.", + "evidence": [ + { + "location": "solution_simplicity result", + "detail": "A new service duplicates the active-admin rule." + }, + { + "location": "code_simplicity result", + "detail": "Two new actions repeat the same active-admin condition." + } + ], + "concern": "The new service and actions duplicate the existing active-admin authorization policy.", + "impact": "The candidate maintains three policy definitions and adds an unnecessary service.", + "proposed_change": "Call require_active_admin directly and remove the duplicate local policy service and conditions.", + "expected_effect": "Reduce three policy definitions to one and remove the duplicate service.", + "related_finding_ids": [ + "code-simplicity.reuse-active-admin" + ] + } + ], "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."}] - }], + "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." + } + ] + } + ], "next_action": "Apply the deduplicated policy reuse change, build a new packet, rerun code simplicity, and then run targeted correctness." } }, { - "case_id": "targeted-correctness-recheck", - "observed_sequence": ["code_simplicity", "correctness"], - "stopped_reason": "all_required_results_clean", + "case_id": "full-restart-after-code-simplicity-fix", + "observed_sequence": [ + "solution_simplicity", + "correctness", + "code_simplicity" + ], + "stopped_reason": "all_required_results_clean_after_full_restart", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, - "verdict": "clean", "findings": [], "blocking_reasons": [], - "next_action": "No further review action is required." + "schema_version": "1.1", + "lens": "aggregate", + "candidate": { + "head_sha": "5050505050505050505050505050505050505050", + "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0" + }, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "next_action": "No further review action is required; the code-simplicity fix restarted and cleanly completed the full three-lens sequence on the new head.", + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "5050505050505050505050505050505050505050", + "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "5050505050505050505050505050505050505050", + "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "5050505050505050505050505050505050505050", + "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0", + "verdict": "clean", + "freshly_executed": true + } + ] } }, { @@ -96,10 +247,17 @@ "observed_sequence": [], "stopped_reason": "missing_required_dependency", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "6060606060606060606060606060606060606060", "comparison_base_sha": "f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0"}, - "verdict": "blocked", "findings": [], - "blocking_reasons": ["Required review skill review-code-simplicity is unavailable."], + "schema_version": "1.1", + "lens": "aggregate", + "candidate": { + "head_sha": "6060606060606060606060606060606060606060", + "comparison_base_sha": "f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0" + }, + "verdict": "blocked", + "findings": [], + "blocking_reasons": [ + "Required review skill review-code-simplicity is unavailable." + ], "next_action": "Make review-code-simplicity available and readable, then restart the full review." } }, @@ -108,64 +266,139 @@ "observed_sequence": [], "stopped_reason": "evidence_packet_incomplete", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "7070707070707070707070707070707070707070", "comparison_base_sha": "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1"}, - "verdict": "blocked", "findings": [], - "blocking_reasons": ["Acceptance criteria are missing.", "Full validation has no result or unavailable reason."], - "validation_limitations": ["Full validation has no result or unavailable reason."], + "schema_version": "1.1", + "lens": "aggregate", + "candidate": { + "head_sha": "7070707070707070707070707070707070707070", + "comparison_base_sha": "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1" + }, + "verdict": "blocked", + "findings": [], + "blocking_reasons": [ + "Acceptance criteria are missing.", + "Full validation has no result or unavailable reason." + ], + "validation_limitations": [ + "Full validation has no result or unavailable reason." + ], "next_action": "Recover the missing acceptance criteria and full-validation result or unavailable reason, rebuild and validate the evidence packet, then restart the full review sequence." } }, { "case_id": "deferred-only-clean", - "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "observed_sequence": [ + "solution_simplicity", + "correctness", + "code_simplicity" + ], "stopped_reason": "all_required_lenses_clean_with_deferred_finding", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, + "schema_version": "1.1", + "lens": "aggregate", + "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": "The ticket explicitly defers migration cleanup to issue #90."}], - "concern": "A legacy compatibility path remains, but its removal is explicitly outside the active ticket.", - "impact": "The compatibility branch remains until its callers migrate.", - "proposed_change": "Remove the legacy path in issue #90.", - "expected_effect": "Retire one compatibility branch after its callers migrate." - }], + "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": "The ticket explicitly defers migration cleanup to issue #90." + } + ], + "concern": "A legacy compatibility path remains, but its removal is explicitly outside the active ticket.", + "impact": "The compatibility branch remains until its callers migrate.", + "proposed_change": "Remove the legacy path in issue #90.", + "expected_effect": "Retire one compatibility branch after its callers migrate." + } + ], "blocking_reasons": [], - "next_action": "No candidate change is required; track the deferred legacy-path cleanup in issue #90." + "next_action": "No candidate change is required; track the deferred legacy-path cleanup in issue #90.", + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "8080808080808080808080808080808080808080", + "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "8080808080808080808080808080808080808080", + "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "8080808080808080808080808080808080808080", + "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1", + "verdict": "clean", + "freshly_executed": true + } + ] } }, { "case_id": "cycle-budget-exhausted", - "observed_sequence": ["solution_simplicity", "correctness"], + "observed_sequence": [ + "solution_simplicity", + "correctness" + ], "stopped_reason": "cycle_budget_exhausted_with_correctness_finding", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, + "schema_version": "1.1", + "lens": "aggregate", + "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 increments and schedules after the maximum."}], - "concern": "The retry path continues scheduling work after reaching the configured maximum.", - "impact": "Repeated work remains unbounded and violates the required retry limit.", - "proposed_change": "Stop scheduling when retry_count reaches max_retries.", - "expected_effect": "Bound repeated work at the required limit." - }], + "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 increments and schedules after the maximum." + } + ], + "concern": "The retry path continues scheduling work after reaching the configured maximum.", + "impact": "Repeated work remains unbounded and violates the required retry limit.", + "proposed_change": "Stop scheduling when retry_count reaches max_retries.", + "expected_effect": "Bound repeated work at the required limit." + } + ], "blocking_reasons": [], "next_action": "The caller must correct the retry bound before merge; the three-cycle automatic review budget is exhausted, so do not request another automatic cycle." } }, { "case_id": "mismatched-lens-result", - "observed_sequence": ["solution_simplicity"], + "observed_sequence": [ + "solution_simplicity" + ], "stopped_reason": "untrustworthy_lens_result", "result": { - "schema_version": "1.0", "lens": "aggregate", - "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, - "verdict": "blocked", "findings": [], + "schema_version": "1.1", + "lens": "aggregate", + "candidate": { + "head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", + "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2" + }, + "verdict": "blocked", + "findings": [], "blocking_reasons": [ "The invoked solution-simplicity lens returned lens correctness instead of solution_simplicity.", "The invoked solution-simplicity lens returned head deadbeefdeadbeefdeadbeefdeadbeefdeadbeef instead of the packet head a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2." @@ -173,5 +406,49 @@ "validation_limitations": [], "next_action": "Rerun review-solution-simplicity with the validated packet for the exact captured candidate before continuing." } + }, + { + "case_id": "full-restart-after-correctness-fix", + "observed_sequence": [ + "solution_simplicity", + "correctness", + "code_simplicity" + ], + "stopped_reason": "all_required_results_clean_after_full_restart", + "result": { + "schema_version": "1.1", + "lens": "aggregate", + "candidate": { + "head_sha": "5151515151515151515151515151515151515151", + "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1" + }, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "5151515151515151515151515151515151515151", + "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "5151515151515151515151515151515151515151", + "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "5151515151515151515151515151515151515151", + "comparison_base_sha": "e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1", + "verdict": "clean", + "freshly_executed": true + } + ], + "next_action": "No further review action is required; the correctness fix restarted and cleanly completed the full three-lens sequence on the new head." + } } ] 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 ebca30f..4eed3be 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.0", + "schema_version": "1.1", "lens": "aggregate", "candidate": { "head_sha": "1212121212121212121212121212121212121212", @@ -14,6 +14,29 @@ "verdict": "clean", "findings": [], "blocking_reasons": [], + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": "1212121212121212121212121212121212121212", + "comparison_base_sha": "abababababababababababababababababababab", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "correctness", + "head_sha": "1212121212121212121212121212121212121212", + "comparison_base_sha": "abababababababababababababababababababab", + "verdict": "clean", + "freshly_executed": true + }, + { + "lens": "code_simplicity", + "head_sha": "1212121212121212121212121212121212121212", + "comparison_base_sha": "abababababababababababababababababababab", + "verdict": "clean", + "freshly_executed": true + } + ], "validation_limitations": [], "next_action": "No changes are required." } diff --git a/skills/review-code-change/references/orchestration-protocol.md b/skills/review-code-change/references/orchestration-protocol.md index ac4615f..81f8727 100644 --- a/skills/review-code-change/references/orchestration-protocol.md +++ b/skills/review-code-change/references/orchestration-protocol.md @@ -21,18 +21,18 @@ the implementation transcript. Use this decision table after validating each lens result. -| Current result | Next action | -| --------------------------------------------- | -------------------------------------------------------------------- | -| solution `blocked` | aggregate `blocked`; stop | -| solution requires strategy replacement | aggregate findings; caller redesigns; stop | -| solution has a tractable in-strategy proposal | run correctness | -| solution `clean` | run correctness | -| correctness `blocked` | aggregate `blocked`; stop | -| correctness has a gating finding | aggregate findings; caller fixes; stop | -| correctness `clean` | run code simplicity | -| code simplicity `blocked` | aggregate `blocked`; stop | -| code simplicity has a gating finding | aggregate findings; caller fixes, then rechecks code and correctness | -| all required results `clean` | aggregate `clean`; stop | +| Current result | Next action | +| --------------------------------------------- | ----------------------------------------------------------------- | +| solution `blocked` | aggregate `blocked`; stop | +| solution requires strategy replacement | aggregate findings; caller redesigns; stop | +| solution has a tractable in-strategy proposal | run correctness | +| solution `clean` | run correctness | +| correctness `blocked` | aggregate `blocked`; stop | +| correctness has a gating finding | aggregate findings; caller fixes; stop | +| correctness `clean` | run code simplicity | +| code simplicity `blocked` | aggregate `blocked`; stop | +| code simplicity has a gating finding | aggregate findings; caller fixes, then restarts the full sequence | +| all required results `clean` | aggregate `clean`; stop | A solution result requires strategy replacement when its proposed change replaces major mechanisms, ownership boundaries, storage, state, or operational @@ -111,15 +111,27 @@ scope. | Change applied by caller | New-head sequence | | ------------------------ | ----------------------------------------------------------- | -| solution redesign | solution, correctness, code | -| correctness fix | correctness, then affected downstream lenses | -| code-simplicity fix | code, then targeted correctness | +| solution redesign | solution, correctness, code (full restart) | +| correctness fix | solution, correctness, code (full restart) | +| code-simplicity fix | solution, correctness, code (full restart) | | base-only drift | apply shared drift rules, then reset only affected evidence | +Any change that produces a new head — a solution redesign, a correctness fix, or +a code-simplicity fix — restarts the complete three-lens sequence on that new +head: solution simplicity, then correctness, then code simplicity. No old-head +lens result may contribute to the new-head aggregate, and no new-head aggregate +`clean` is valid without a fresh execution from all three lenses (see the shared +contract's "Lens execution evidence" section). This closes the contradiction +where a correctness-only or code-simplicity-only rerun could reach a new-head +`clean` aggregate without a fresh solution-simplicity result for that exact +head, or could miss a correctness fix that introduced new strategy-level +machinery. This protocol defines no selective-reuse exception across different +heads; only base-only drift, where the head itself is unchanged, uses the +existing effective-candidate retain or reset rules. + Count a full cycle when a candidate reaches a material finding and the caller -supplies a new head for another full or required downstream pass. At cycle -three, return unresolved findings and tell the caller that the automatic cycle -budget is exhausted. +supplies a new head for another full restart. At cycle three, return unresolved +findings and tell the caller that the automatic cycle budget is exhausted. ## Preserve read-only operation diff --git a/skills/review-code-change/references/review-suite/CONTRACT.md b/skills/review-code-change/references/review-suite/CONTRACT.md index 87d0908..bdced9f 100644 --- a/skills/review-code-change/references/review-suite/CONTRACT.md +++ b/skills/review-code-change/references/review-suite/CONTRACT.md @@ -104,8 +104,11 @@ that merely move complexity behind another name. ## Verdict semantics -- `clean`: no `blocking` or `strong_recommendation` finding remains. Deferred - findings may be retained without failing the gate. +- `clean`: no `blocking` or `strong_recommendation` finding remains, every + packet validation entry supplied as required evidence passed, and — for an + aggregate result — every required lens has a fresh, current-head execution + (see "Lens execution evidence" below). Deferred findings may be retained + without failing the gate. - `changes_required`: at least one actionable `blocking` or `strong_recommendation` finding remains. - `blocked`: essential evidence or a product or architecture decision is @@ -118,6 +121,48 @@ fields that the caller could not establish and may preserve already-demonstrated findings, but those findings do not convert the blocked review into a merge verdict. +### Validation must back a `clean` verdict + +A packet's `validation` array is required evidence, not optional context: a +`clean` verdict claims that evidence is trustworthy, so a result must not +declare `clean` while that same packet records a required focused or full +validation entry as `failed` or `unavailable`. Pair validation rejects any +`clean` result paired with such a packet. + +- A `failed` command with a demonstrated candidate-caused failure is a gating + correctness/validation finding and yields `changes_required`, never `clean`. +- A `failed` or `unavailable` command whose attribution or result is + insufficient for a trustworthy verdict yields `blocked` with a concrete reason + and a recorded `validation_limitations` entry, never `clean`. +- Do not invent infrastructure attribution from an exit code alone, and do not + omit a failed or unavailable command from validation evidence to hide it. + +### Lens execution evidence (aggregate results) + +An aggregate result records `lens_executions`: one entry per required lens +(`solution_simplicity`, `correctness`, `code_simplicity`), each naming its +`lens`, `head_sha`, `comparison_base_sha`, `verdict`, and whether it was +`freshly_executed` for this exact aggregate. + +For aggregate `clean`: + +- all three required lenses must be present exactly once, with no missing and no + duplicate entry; +- every entry's `head_sha` and `comparison_base_sha` must equal the aggregate + result's own candidate — a stale-head or stale-base entry cannot contribute to + a new-head aggregate; +- every entry's `verdict` must be `clean`; and +- every entry must be `freshly_executed`; no old-head or reused result may count + toward a new aggregate. + +Any edit, rebase, conflict resolution, or update that changes the head +invalidates every existing lens execution for that head. Restart the complete +three-lens sequence — solution simplicity, correctness, then code simplicity — +after any such head-changing fix; a partial rerun (for example, only correctness +after a correctness fix, or only code simplicity and correctness after a +code-simplicity fix) cannot produce a valid `clean` aggregate. This child +defines no selective-reuse exception across different heads. + ## 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 7dab07c..47e9a38 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.0"}, + "schema_version": {"const": "1.1"}, "lens": { "enum": [ "correctness", @@ -83,6 +83,27 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "lens_executions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "lens", + "head_sha", + "comparison_base_sha", + "verdict", + "freshly_executed" + ], + "properties": { + "lens": {"enum": ["solution_simplicity", "correctness", "code_simplicity"]}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "freshly_executed": {"type": "boolean"} + } + } + }, "validation_limitations": { "type": "array", "items": {"type": "string", "minLength": 1} diff --git a/skills/review-code-change/references/review-suite/validate.py b/skills/review-code-change/references/review-suite/validate.py index 50e0b12..033b586 100644 --- a/skills/review-code-change/references/review-suite/validate.py +++ b/skills/review-code-change/references/review-suite/validate.py @@ -33,6 +33,10 @@ def _schema_file(name: str) -> Path: "result": _schema_file("review-result.schema.json"), } +REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") + +STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} + BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( r"^\$: missing required property " @@ -161,6 +165,14 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: + if ( + isinstance(result, dict) + and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS + ): + return [ + "$.schema_version: stale v1.0 result rejected; v1.0 results are not " + "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -217,6 +229,63 @@ def validate_result(result: dict[str, Any]) -> list[str]: "$.proposal_dispositions: duplicate finding id(s): " + ", ".join(duplicate_dispositions) ) + + if result.get("lens") == "aggregate" and verdict == "clean": + errors.extend(_check_aggregate_clean_lens_executions(result)) + 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. + + An aggregate `clean` is only trustworthy when every required lens actually + completed against the exact aggregate candidate. This closes the gap where + a new-head aggregate could be reached without a fresh solution-simplicity, + correctness, or code-simplicity execution for that exact head, and rejects + any old-head or old-base execution smuggled into a new aggregate. + """ + errors: list[str] = [] + candidate = result.get("candidate") + head = candidate.get("head_sha") if isinstance(candidate, dict) else None + base = candidate.get("comparison_base_sha") if isinstance(candidate, dict) else None + executions = result.get("lens_executions") + if not isinstance(executions, list) or not executions: + return ["$.lens_executions: aggregate clean requires lens execution evidence"] + + seen: list[str] = [] + for index, execution in enumerate(executions): + if not isinstance(execution, dict): + continue + lens_name = execution.get("lens") + seen.append(lens_name) + at = f"$.lens_executions[{index}]" + if ( + execution.get("head_sha") != head + or execution.get("comparison_base_sha") != base + ): + errors.append( + f"{at}: stale head or base cannot contribute to a new-head " + "aggregate clean" + ) + if execution.get("verdict") != "clean": + errors.append(f"{at}: aggregate clean requires a clean lens execution") + if execution.get("freshly_executed") is not True: + errors.append( + f"{at}: aggregate clean requires a freshly executed lens result" + ) + + missing = [lens for lens in REQUIRED_AGGREGATE_LENSES if lens not in seen] + if missing: + errors.append( + "$.lens_executions: aggregate clean is missing required lens " + "execution(s): " + ", ".join(missing) + ) + duplicates = sorted({lens for lens in seen if lens and seen.count(lens) > 1}) + if duplicates: + errors.append( + "$.lens_executions: aggregate clean has duplicate lens execution(s): " + + ", ".join(duplicates) + ) return errors @@ -253,6 +322,35 @@ def validate_pair(packet: dict[str, Any], result: dict[str, Any]) -> list[str]: errors.append(f"candidate.{field}: result omits identity present in packet") elif packet_has_field and packet_candidate[field] != result_candidate[field]: errors.append(f"candidate.{field}: result does not match packet") + errors.extend(_check_clean_requires_passing_validation(packet, result)) + return errors + + +def _check_clean_requires_passing_validation( + packet: dict[str, Any], result: dict[str, Any] +) -> list[str]: + """Reject a `clean` verdict paired with failed or unavailable validation. + + A schema-valid `clean` result previously did not prove that the packet's + own required focused and full validation actually passed: every entry + could be `failed` and pair validation raised no error. `clean` must not + hide a failed or unavailable required command. + """ + if result.get("verdict") != "clean": + return [] + validations = packet.get("validation") + if not isinstance(validations, list): + return [] + errors: list[str] = [] + for index, validation in enumerate(validations): + if not isinstance(validation, dict): + continue + status = validation.get("status") + if status in {"failed", "unavailable"}: + errors.append( + f"validation[{index}]: clean cannot pair with {status} " + "required validation" + ) return errors diff --git a/skills/review-code-change/scripts/tests/test_orchestration_contract.py b/skills/review-code-change/scripts/tests/test_orchestration_contract.py index bd06a56..2f64a92 100644 --- a/skills/review-code-change/scripts/tests/test_orchestration_contract.py +++ b/skills/review-code-change/scripts/tests/test_orchestration_contract.py @@ -111,8 +111,12 @@ def test_sequence_and_verdict_boundaries(self): ["solution_simplicity", "correctness", "code_simplicity"], "changes_required", ), - "targeted-correctness-recheck": ( - ["code_simplicity", "correctness"], + "full-restart-after-code-simplicity-fix": ( + ["solution_simplicity", "correctness", "code_simplicity"], + "clean", + ), + "full-restart-after-correctness-fix": ( + ["solution_simplicity", "correctness", "code_simplicity"], "clean", ), "missing-dependency": ([], "blocked"), 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 6119c5a..7d6b5f7 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.0", + "schema_version": "1.1", "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 87d0908..bdced9f 100644 --- a/skills/review-code-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-code-simplicity/references/review-suite/CONTRACT.md @@ -104,8 +104,11 @@ that merely move complexity behind another name. ## Verdict semantics -- `clean`: no `blocking` or `strong_recommendation` finding remains. Deferred - findings may be retained without failing the gate. +- `clean`: no `blocking` or `strong_recommendation` finding remains, every + packet validation entry supplied as required evidence passed, and — for an + aggregate result — every required lens has a fresh, current-head execution + (see "Lens execution evidence" below). Deferred findings may be retained + without failing the gate. - `changes_required`: at least one actionable `blocking` or `strong_recommendation` finding remains. - `blocked`: essential evidence or a product or architecture decision is @@ -118,6 +121,48 @@ fields that the caller could not establish and may preserve already-demonstrated findings, but those findings do not convert the blocked review into a merge verdict. +### Validation must back a `clean` verdict + +A packet's `validation` array is required evidence, not optional context: a +`clean` verdict claims that evidence is trustworthy, so a result must not +declare `clean` while that same packet records a required focused or full +validation entry as `failed` or `unavailable`. Pair validation rejects any +`clean` result paired with such a packet. + +- A `failed` command with a demonstrated candidate-caused failure is a gating + correctness/validation finding and yields `changes_required`, never `clean`. +- A `failed` or `unavailable` command whose attribution or result is + insufficient for a trustworthy verdict yields `blocked` with a concrete reason + and a recorded `validation_limitations` entry, never `clean`. +- Do not invent infrastructure attribution from an exit code alone, and do not + omit a failed or unavailable command from validation evidence to hide it. + +### Lens execution evidence (aggregate results) + +An aggregate result records `lens_executions`: one entry per required lens +(`solution_simplicity`, `correctness`, `code_simplicity`), each naming its +`lens`, `head_sha`, `comparison_base_sha`, `verdict`, and whether it was +`freshly_executed` for this exact aggregate. + +For aggregate `clean`: + +- all three required lenses must be present exactly once, with no missing and no + duplicate entry; +- every entry's `head_sha` and `comparison_base_sha` must equal the aggregate + result's own candidate — a stale-head or stale-base entry cannot contribute to + a new-head aggregate; +- every entry's `verdict` must be `clean`; and +- every entry must be `freshly_executed`; no old-head or reused result may count + toward a new aggregate. + +Any edit, rebase, conflict resolution, or update that changes the head +invalidates every existing lens execution for that head. Restart the complete +three-lens sequence — solution simplicity, correctness, then code simplicity — +after any such head-changing fix; a partial rerun (for example, only correctness +after a correctness fix, or only code simplicity and correctness after a +code-simplicity fix) cannot produce a valid `clean` aggregate. This child +defines no selective-reuse exception across different heads. + ## 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 7dab07c..47e9a38 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.0"}, + "schema_version": {"const": "1.1"}, "lens": { "enum": [ "correctness", @@ -83,6 +83,27 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "lens_executions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "lens", + "head_sha", + "comparison_base_sha", + "verdict", + "freshly_executed" + ], + "properties": { + "lens": {"enum": ["solution_simplicity", "correctness", "code_simplicity"]}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "freshly_executed": {"type": "boolean"} + } + } + }, "validation_limitations": { "type": "array", "items": {"type": "string", "minLength": 1} diff --git a/skills/review-code-simplicity/references/review-suite/validate.py b/skills/review-code-simplicity/references/review-suite/validate.py index 50e0b12..033b586 100644 --- a/skills/review-code-simplicity/references/review-suite/validate.py +++ b/skills/review-code-simplicity/references/review-suite/validate.py @@ -33,6 +33,10 @@ def _schema_file(name: str) -> Path: "result": _schema_file("review-result.schema.json"), } +REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") + +STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} + BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( r"^\$: missing required property " @@ -161,6 +165,14 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: + if ( + isinstance(result, dict) + and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS + ): + return [ + "$.schema_version: stale v1.0 result rejected; v1.0 results are not " + "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -217,6 +229,63 @@ def validate_result(result: dict[str, Any]) -> list[str]: "$.proposal_dispositions: duplicate finding id(s): " + ", ".join(duplicate_dispositions) ) + + if result.get("lens") == "aggregate" and verdict == "clean": + errors.extend(_check_aggregate_clean_lens_executions(result)) + 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. + + An aggregate `clean` is only trustworthy when every required lens actually + completed against the exact aggregate candidate. This closes the gap where + a new-head aggregate could be reached without a fresh solution-simplicity, + correctness, or code-simplicity execution for that exact head, and rejects + any old-head or old-base execution smuggled into a new aggregate. + """ + errors: list[str] = [] + candidate = result.get("candidate") + head = candidate.get("head_sha") if isinstance(candidate, dict) else None + base = candidate.get("comparison_base_sha") if isinstance(candidate, dict) else None + executions = result.get("lens_executions") + if not isinstance(executions, list) or not executions: + return ["$.lens_executions: aggregate clean requires lens execution evidence"] + + seen: list[str] = [] + for index, execution in enumerate(executions): + if not isinstance(execution, dict): + continue + lens_name = execution.get("lens") + seen.append(lens_name) + at = f"$.lens_executions[{index}]" + if ( + execution.get("head_sha") != head + or execution.get("comparison_base_sha") != base + ): + errors.append( + f"{at}: stale head or base cannot contribute to a new-head " + "aggregate clean" + ) + if execution.get("verdict") != "clean": + errors.append(f"{at}: aggregate clean requires a clean lens execution") + if execution.get("freshly_executed") is not True: + errors.append( + f"{at}: aggregate clean requires a freshly executed lens result" + ) + + missing = [lens for lens in REQUIRED_AGGREGATE_LENSES if lens not in seen] + if missing: + errors.append( + "$.lens_executions: aggregate clean is missing required lens " + "execution(s): " + ", ".join(missing) + ) + duplicates = sorted({lens for lens in seen if lens and seen.count(lens) > 1}) + if duplicates: + errors.append( + "$.lens_executions: aggregate clean has duplicate lens execution(s): " + + ", ".join(duplicates) + ) return errors @@ -253,6 +322,35 @@ def validate_pair(packet: dict[str, Any], result: dict[str, Any]) -> list[str]: errors.append(f"candidate.{field}: result omits identity present in packet") elif packet_has_field and packet_candidate[field] != result_candidate[field]: errors.append(f"candidate.{field}: result does not match packet") + errors.extend(_check_clean_requires_passing_validation(packet, result)) + return errors + + +def _check_clean_requires_passing_validation( + packet: dict[str, Any], result: dict[str, Any] +) -> list[str]: + """Reject a `clean` verdict paired with failed or unavailable validation. + + A schema-valid `clean` result previously did not prove that the packet's + own required focused and full validation actually passed: every entry + could be `failed` and pair validation raised no error. `clean` must not + hide a failed or unavailable required command. + """ + if result.get("verdict") != "clean": + return [] + validations = packet.get("validation") + if not isinstance(validations, list): + return [] + errors: list[str] = [] + for index, validation in enumerate(validations): + if not isinstance(validation, dict): + continue + status = validation.get("status") + if status in {"failed", "unavailable"}: + errors.append( + f"validation[{index}]: clean cannot pair with {status} " + "required validation" + ) return errors 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 113c73c..0929f20 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.0", + "schema_version": "1.1", "lens": "correctness", "candidate": { "head_sha": "8484848484848484848484848484848484848484", diff --git a/skills/review-correctness/references/review-suite/CONTRACT.md b/skills/review-correctness/references/review-suite/CONTRACT.md index 87d0908..bdced9f 100644 --- a/skills/review-correctness/references/review-suite/CONTRACT.md +++ b/skills/review-correctness/references/review-suite/CONTRACT.md @@ -104,8 +104,11 @@ that merely move complexity behind another name. ## Verdict semantics -- `clean`: no `blocking` or `strong_recommendation` finding remains. Deferred - findings may be retained without failing the gate. +- `clean`: no `blocking` or `strong_recommendation` finding remains, every + packet validation entry supplied as required evidence passed, and — for an + aggregate result — every required lens has a fresh, current-head execution + (see "Lens execution evidence" below). Deferred findings may be retained + without failing the gate. - `changes_required`: at least one actionable `blocking` or `strong_recommendation` finding remains. - `blocked`: essential evidence or a product or architecture decision is @@ -118,6 +121,48 @@ fields that the caller could not establish and may preserve already-demonstrated findings, but those findings do not convert the blocked review into a merge verdict. +### Validation must back a `clean` verdict + +A packet's `validation` array is required evidence, not optional context: a +`clean` verdict claims that evidence is trustworthy, so a result must not +declare `clean` while that same packet records a required focused or full +validation entry as `failed` or `unavailable`. Pair validation rejects any +`clean` result paired with such a packet. + +- A `failed` command with a demonstrated candidate-caused failure is a gating + correctness/validation finding and yields `changes_required`, never `clean`. +- A `failed` or `unavailable` command whose attribution or result is + insufficient for a trustworthy verdict yields `blocked` with a concrete reason + and a recorded `validation_limitations` entry, never `clean`. +- Do not invent infrastructure attribution from an exit code alone, and do not + omit a failed or unavailable command from validation evidence to hide it. + +### Lens execution evidence (aggregate results) + +An aggregate result records `lens_executions`: one entry per required lens +(`solution_simplicity`, `correctness`, `code_simplicity`), each naming its +`lens`, `head_sha`, `comparison_base_sha`, `verdict`, and whether it was +`freshly_executed` for this exact aggregate. + +For aggregate `clean`: + +- all three required lenses must be present exactly once, with no missing and no + duplicate entry; +- every entry's `head_sha` and `comparison_base_sha` must equal the aggregate + result's own candidate — a stale-head or stale-base entry cannot contribute to + a new-head aggregate; +- every entry's `verdict` must be `clean`; and +- every entry must be `freshly_executed`; no old-head or reused result may count + toward a new aggregate. + +Any edit, rebase, conflict resolution, or update that changes the head +invalidates every existing lens execution for that head. Restart the complete +three-lens sequence — solution simplicity, correctness, then code simplicity — +after any such head-changing fix; a partial rerun (for example, only correctness +after a correctness fix, or only code simplicity and correctness after a +code-simplicity fix) cannot produce a valid `clean` aggregate. This child +defines no selective-reuse exception across different heads. + ## 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 7dab07c..47e9a38 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.0"}, + "schema_version": {"const": "1.1"}, "lens": { "enum": [ "correctness", @@ -83,6 +83,27 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "lens_executions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "lens", + "head_sha", + "comparison_base_sha", + "verdict", + "freshly_executed" + ], + "properties": { + "lens": {"enum": ["solution_simplicity", "correctness", "code_simplicity"]}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "freshly_executed": {"type": "boolean"} + } + } + }, "validation_limitations": { "type": "array", "items": {"type": "string", "minLength": 1} diff --git a/skills/review-correctness/references/review-suite/validate.py b/skills/review-correctness/references/review-suite/validate.py index 50e0b12..033b586 100644 --- a/skills/review-correctness/references/review-suite/validate.py +++ b/skills/review-correctness/references/review-suite/validate.py @@ -33,6 +33,10 @@ def _schema_file(name: str) -> Path: "result": _schema_file("review-result.schema.json"), } +REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") + +STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} + BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( r"^\$: missing required property " @@ -161,6 +165,14 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: + if ( + isinstance(result, dict) + and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS + ): + return [ + "$.schema_version: stale v1.0 result rejected; v1.0 results are not " + "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -217,6 +229,63 @@ def validate_result(result: dict[str, Any]) -> list[str]: "$.proposal_dispositions: duplicate finding id(s): " + ", ".join(duplicate_dispositions) ) + + if result.get("lens") == "aggregate" and verdict == "clean": + errors.extend(_check_aggregate_clean_lens_executions(result)) + 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. + + An aggregate `clean` is only trustworthy when every required lens actually + completed against the exact aggregate candidate. This closes the gap where + a new-head aggregate could be reached without a fresh solution-simplicity, + correctness, or code-simplicity execution for that exact head, and rejects + any old-head or old-base execution smuggled into a new aggregate. + """ + errors: list[str] = [] + candidate = result.get("candidate") + head = candidate.get("head_sha") if isinstance(candidate, dict) else None + base = candidate.get("comparison_base_sha") if isinstance(candidate, dict) else None + executions = result.get("lens_executions") + if not isinstance(executions, list) or not executions: + return ["$.lens_executions: aggregate clean requires lens execution evidence"] + + seen: list[str] = [] + for index, execution in enumerate(executions): + if not isinstance(execution, dict): + continue + lens_name = execution.get("lens") + seen.append(lens_name) + at = f"$.lens_executions[{index}]" + if ( + execution.get("head_sha") != head + or execution.get("comparison_base_sha") != base + ): + errors.append( + f"{at}: stale head or base cannot contribute to a new-head " + "aggregate clean" + ) + if execution.get("verdict") != "clean": + errors.append(f"{at}: aggregate clean requires a clean lens execution") + if execution.get("freshly_executed") is not True: + errors.append( + f"{at}: aggregate clean requires a freshly executed lens result" + ) + + missing = [lens for lens in REQUIRED_AGGREGATE_LENSES if lens not in seen] + if missing: + errors.append( + "$.lens_executions: aggregate clean is missing required lens " + "execution(s): " + ", ".join(missing) + ) + duplicates = sorted({lens for lens in seen if lens and seen.count(lens) > 1}) + if duplicates: + errors.append( + "$.lens_executions: aggregate clean has duplicate lens execution(s): " + + ", ".join(duplicates) + ) return errors @@ -253,6 +322,35 @@ def validate_pair(packet: dict[str, Any], result: dict[str, Any]) -> list[str]: errors.append(f"candidate.{field}: result omits identity present in packet") elif packet_has_field and packet_candidate[field] != result_candidate[field]: errors.append(f"candidate.{field}: result does not match packet") + errors.extend(_check_clean_requires_passing_validation(packet, result)) + return errors + + +def _check_clean_requires_passing_validation( + packet: dict[str, Any], result: dict[str, Any] +) -> list[str]: + """Reject a `clean` verdict paired with failed or unavailable validation. + + A schema-valid `clean` result previously did not prove that the packet's + own required focused and full validation actually passed: every entry + could be `failed` and pair validation raised no error. `clean` must not + hide a failed or unavailable required command. + """ + if result.get("verdict") != "clean": + return [] + validations = packet.get("validation") + if not isinstance(validations, list): + return [] + errors: list[str] = [] + for index, validation in enumerate(validations): + if not isinstance(validation, dict): + continue + status = validation.get("status") + if status in {"failed", "unavailable"}: + errors.append( + f"validation[{index}]: clean cannot pair with {status} " + "required validation" + ) return errors 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 1dd454b..7f267f6 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.0", + "schema_version": "1.1", "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 89ab243..05394cf 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.0", + "schema_version": "1.1", "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 9a0dea6..8b3a7b5 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.0", + "schema_version": "1.1", "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 87d0908..bdced9f 100644 --- a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md +++ b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md @@ -104,8 +104,11 @@ that merely move complexity behind another name. ## Verdict semantics -- `clean`: no `blocking` or `strong_recommendation` finding remains. Deferred - findings may be retained without failing the gate. +- `clean`: no `blocking` or `strong_recommendation` finding remains, every + packet validation entry supplied as required evidence passed, and — for an + aggregate result — every required lens has a fresh, current-head execution + (see "Lens execution evidence" below). Deferred findings may be retained + without failing the gate. - `changes_required`: at least one actionable `blocking` or `strong_recommendation` finding remains. - `blocked`: essential evidence or a product or architecture decision is @@ -118,6 +121,48 @@ fields that the caller could not establish and may preserve already-demonstrated findings, but those findings do not convert the blocked review into a merge verdict. +### Validation must back a `clean` verdict + +A packet's `validation` array is required evidence, not optional context: a +`clean` verdict claims that evidence is trustworthy, so a result must not +declare `clean` while that same packet records a required focused or full +validation entry as `failed` or `unavailable`. Pair validation rejects any +`clean` result paired with such a packet. + +- A `failed` command with a demonstrated candidate-caused failure is a gating + correctness/validation finding and yields `changes_required`, never `clean`. +- A `failed` or `unavailable` command whose attribution or result is + insufficient for a trustworthy verdict yields `blocked` with a concrete reason + and a recorded `validation_limitations` entry, never `clean`. +- Do not invent infrastructure attribution from an exit code alone, and do not + omit a failed or unavailable command from validation evidence to hide it. + +### Lens execution evidence (aggregate results) + +An aggregate result records `lens_executions`: one entry per required lens +(`solution_simplicity`, `correctness`, `code_simplicity`), each naming its +`lens`, `head_sha`, `comparison_base_sha`, `verdict`, and whether it was +`freshly_executed` for this exact aggregate. + +For aggregate `clean`: + +- all three required lenses must be present exactly once, with no missing and no + duplicate entry; +- every entry's `head_sha` and `comparison_base_sha` must equal the aggregate + result's own candidate — a stale-head or stale-base entry cannot contribute to + a new-head aggregate; +- every entry's `verdict` must be `clean`; and +- every entry must be `freshly_executed`; no old-head or reused result may count + toward a new aggregate. + +Any edit, rebase, conflict resolution, or update that changes the head +invalidates every existing lens execution for that head. Restart the complete +three-lens sequence — solution simplicity, correctness, then code simplicity — +after any such head-changing fix; a partial rerun (for example, only correctness +after a correctness fix, or only code simplicity and correctness after a +code-simplicity fix) cannot produce a valid `clean` aggregate. This child +defines no selective-reuse exception across different heads. + ## 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 7dab07c..47e9a38 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.0"}, + "schema_version": {"const": "1.1"}, "lens": { "enum": [ "correctness", @@ -83,6 +83,27 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "lens_executions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "lens", + "head_sha", + "comparison_base_sha", + "verdict", + "freshly_executed" + ], + "properties": { + "lens": {"enum": ["solution_simplicity", "correctness", "code_simplicity"]}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "freshly_executed": {"type": "boolean"} + } + } + }, "validation_limitations": { "type": "array", "items": {"type": "string", "minLength": 1} diff --git a/skills/review-solution-simplicity/references/review-suite/validate.py b/skills/review-solution-simplicity/references/review-suite/validate.py index 50e0b12..033b586 100644 --- a/skills/review-solution-simplicity/references/review-suite/validate.py +++ b/skills/review-solution-simplicity/references/review-suite/validate.py @@ -33,6 +33,10 @@ def _schema_file(name: str) -> Path: "result": _schema_file("review-result.schema.json"), } +REQUIRED_AGGREGATE_LENSES = ("solution_simplicity", "correctness", "code_simplicity") + +STALE_RESULT_SCHEMA_VERSIONS = {"1.0"} + BLOCKABLE_PACKET_ERROR_PATTERNS = ( re.compile( r"^\$: missing required property " @@ -161,6 +165,14 @@ def validate_packet(packet: dict[str, Any]) -> list[str]: def validate_result(result: dict[str, Any]) -> list[str]: + if ( + isinstance(result, dict) + and result.get("schema_version") in STALE_RESULT_SCHEMA_VERSIONS + ): + return [ + "$.schema_version: stale v1.0 result rejected; v1.0 results are not " + "accepted as v1.1 evidence, rebuild review evidence at schema 1.1" + ] schema = json.loads(SCHEMAS["result"].read_text()) errors = validate_schema(result, schema) if errors: @@ -217,6 +229,63 @@ def validate_result(result: dict[str, Any]) -> list[str]: "$.proposal_dispositions: duplicate finding id(s): " + ", ".join(duplicate_dispositions) ) + + if result.get("lens") == "aggregate" and verdict == "clean": + errors.extend(_check_aggregate_clean_lens_executions(result)) + 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. + + An aggregate `clean` is only trustworthy when every required lens actually + completed against the exact aggregate candidate. This closes the gap where + a new-head aggregate could be reached without a fresh solution-simplicity, + correctness, or code-simplicity execution for that exact head, and rejects + any old-head or old-base execution smuggled into a new aggregate. + """ + errors: list[str] = [] + candidate = result.get("candidate") + head = candidate.get("head_sha") if isinstance(candidate, dict) else None + base = candidate.get("comparison_base_sha") if isinstance(candidate, dict) else None + executions = result.get("lens_executions") + if not isinstance(executions, list) or not executions: + return ["$.lens_executions: aggregate clean requires lens execution evidence"] + + seen: list[str] = [] + for index, execution in enumerate(executions): + if not isinstance(execution, dict): + continue + lens_name = execution.get("lens") + seen.append(lens_name) + at = f"$.lens_executions[{index}]" + if ( + execution.get("head_sha") != head + or execution.get("comparison_base_sha") != base + ): + errors.append( + f"{at}: stale head or base cannot contribute to a new-head " + "aggregate clean" + ) + if execution.get("verdict") != "clean": + errors.append(f"{at}: aggregate clean requires a clean lens execution") + if execution.get("freshly_executed") is not True: + errors.append( + f"{at}: aggregate clean requires a freshly executed lens result" + ) + + missing = [lens for lens in REQUIRED_AGGREGATE_LENSES if lens not in seen] + if missing: + errors.append( + "$.lens_executions: aggregate clean is missing required lens " + "execution(s): " + ", ".join(missing) + ) + duplicates = sorted({lens for lens in seen if lens and seen.count(lens) > 1}) + if duplicates: + errors.append( + "$.lens_executions: aggregate clean has duplicate lens execution(s): " + + ", ".join(duplicates) + ) return errors @@ -253,6 +322,35 @@ def validate_pair(packet: dict[str, Any], result: dict[str, Any]) -> list[str]: errors.append(f"candidate.{field}: result omits identity present in packet") elif packet_has_field and packet_candidate[field] != result_candidate[field]: errors.append(f"candidate.{field}: result does not match packet") + errors.extend(_check_clean_requires_passing_validation(packet, result)) + return errors + + +def _check_clean_requires_passing_validation( + packet: dict[str, Any], result: dict[str, Any] +) -> list[str]: + """Reject a `clean` verdict paired with failed or unavailable validation. + + A schema-valid `clean` result previously did not prove that the packet's + own required focused and full validation actually passed: every entry + could be `failed` and pair validation raised no error. `clean` must not + hide a failed or unavailable required command. + """ + if result.get("verdict") != "clean": + return [] + validations = packet.get("validation") + if not isinstance(validations, list): + return [] + errors: list[str] = [] + for index, validation in enumerate(validations): + if not isinstance(validation, dict): + continue + status = validation.get("status") + if status in {"failed", "unavailable"}: + errors.append( + f"validation[{index}]: clean cannot pair with {status} " + "required validation" + ) return errors From 0f26df6efe01d65385efb549d7dce6797c445303 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 19:23:42 -0700 Subject: [PATCH 2/2] fix: correct a stale partial-rerun next_action in the deduplicate-overlap fixture ## Summary - `skills/review-code-change/evals/expectations.json`'s "deduplicate-overlap" case still described the pre-#51 partial-rerun pattern ("rerun code simplicity, and then run targeted correctness"). Update it to the corrected full three-lens restart, matching the orchestration protocol's decision table and the sibling full-restart cases. ## Why - Found by an initial `review-code-change` pass on this candidate: a repository-owned fixture that models correct orchestrator output still taught the exact partial-rerun sequence this ticket makes invalid. Co-Authored-By: Claude Sonnet 5 --- skills/review-code-change/evals/expectations.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/review-code-change/evals/expectations.json b/skills/review-code-change/evals/expectations.json index 18e83da..54a91e1 100644 --- a/skills/review-code-change/evals/expectations.json +++ b/skills/review-code-change/evals/expectations.json @@ -195,7 +195,7 @@ ] } ], - "next_action": "Apply the deduplicated policy reuse change, build a new packet, rerun code simplicity, and then run targeted correctness." + "next_action": "Apply the deduplicated policy reuse change, build a new packet, and restart the full three-lens sequence (solution simplicity, correctness, then code simplicity) on the new head." } }, {