Skip to content

feat: require passing validation and current-head lens evidence for a clean review verdict - #81

Merged
shaug merged 2 commits into
mainfrom
51-clean-verdict-validation-lens
Jul 29, 2026
Merged

feat: require passing validation and current-head lens evidence for a clean review verdict#81
shaug merged 2 commits into
mainfrom
51-clean-verdict-validation-lens

Conversation

@shaug

@shaug shaug commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

Repairs two settled semantic contradictions in the shared review contract and
review-code-change orchestration (per #59's decision record, disposition
"retain, unchanged"), and bumps the review result schema 1.0 -> 1.1,
additive only:

  • Validation/verdict hole: pair validation now cross-checks packet
    validation status against verdict — clean can no longer pair with a
    packet whose required focused or full validation entry is failed or
    unavailable.
  • New-head evidence contradiction: the aggregate result schema gains a
    lens_executions array recording, per required lens, its head SHA,
    comparison-base SHA, verdict, and freshness. Aggregate clean now requires
    exactly one fresh, current-head/current-base, clean execution for each of
    solution simplicity, correctness, and code simplicity — no missing,
    duplicate, or stale-head/base entry. The orchestration re-review matrix is
    corrected so any head-changing fix (solution redesign, correctness fix, or
    code-simplicity fix) restarts the complete three-lens sequence, closing the
    gap where a correctness-only or code-simplicity-only rerun could reach a
    new-head clean aggregate without a fresh solution-simplicity result.
  • A stale schema_version: "1.0" result is now rejected with a clear
    migration error rather than silently reinterpreted as v1.1.

Why

An aggregate clean result was previously reachable even when every required
validation command had failed, and the old re-review matrix could reach a new
head clean without re-running all three required lenses on that head. Both
are deterministic protocol/validator contradictions identified and settled by
#59's decision record — this child implements exactly the repair it
specifies, nothing broader (no coverage ledgers, impact maps, risk profiles,
independent explorers, connector learning, or caller integration).

Scope

  • Canonical: review-suite/CONTRACT.md, review-suite/contracts/review-result.schema.json,
    review-suite/scripts/validate.py, canonical fixtures, and 13 new
    regression tests in review-suite/scripts/tests/test_contracts.py.
  • review-code-change: SKILL.md, references/orchestration-protocol.md,
    its own orchestration case/expectation corpus.
  • All four bundled review-suite copies refreshed via just sync-contracts
    (byte-identity independently verified).
  • Every skill's own standalone eval fixtures migrated to schema 1.1.
  • CHANGELOG.md.

Does not touch implement-ticket, babysit-pr, or any of #58/#59's
frozen artifacts (review-suite/evals/baseline/v1/, review-suite/evals/v2/).

Test plan

  • just format, just lint, just test all pass (227 tests in
    review-suite/scripts/tests, up from 214; 9 in
    review-code-change/scripts/tests).
  • All 11 required regression tests from Make clean verdicts require passing validation and current-head lens evidence #51's body present and passing
    (as 13 direct test cases, covering failed/unavailable focused and full
    validation, missing/duplicate/stale lens executions, both new-head
    full-restart scenarios, unchanged-head base drift, and stale v1.0
    rejection).
  • The four bundled review-suite copies are byte-identical to canonical
    after just sync-contracts.
  • 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 validate with zero errors, and
    the new clean-requires-passing-validation rule cannot fire against any
    recorded verdict (the one non-passed validation entry across all 15
    packets belongs to a case with false_clean_attempts: 0). The baseline
    protocol issues exactly one request per attempt, so the new
    lens-execution/full-restart requirement is structurally unreachable in
    this baseline. Verdicts and findings replay unchanged.
  • Initial review-code-change cycle found one stale fixture
    inconsistency (a leftover pre-Make clean verdicts require passing validation and current-head lens evidence #51 partial-rerun next_action string);
    fixed, re-reviewed on the new head, clean.

Fixes #51

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

shaug and others added 2 commits July 28, 2026 19:10
@shaug @claude
feat: require passing validation and current-head lens evidence for a…
23d7c59 
… clean review verdict

## 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 <noreply@anthropic.com>
@shaug @claude
fix: correct a stale partial-rerun next_action in the deduplicate-ove…
0f26df6 
…rlap 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 <noreply@anthropic.com>
@shaug
shaug merged commit b1e5197 into main Jul 29, 2026
1 check passed
@shaug
shaug deleted the 51-clean-verdict-validation-lens branch July 29, 2026 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make clean verdicts require passing validation and current-head lens evidence

1 participant

@shaug