feat(review-fix-loop): define invocation, checkpoint, and terminal-result contracts - #107
Merged
Conversation
…sult contracts ## Summary - Add the skill-local `review-fix-loop` schemas (invocation, checkpoint, terminal-result) plus a dependency-free `scripts/validate.py` and its cross-field semantics documented in `references/CONTRACT.md`. - Model fix-cycle budgets as invocation-scoped state reconstructed from checkpoint `cycle_attempts` history rather than stored denormalized counts. - Give every terminal state (`converged`, `changes_remaining`, `blocked`) an explicit, validator-enforced publication/retained-commit/operator-action contract, covering both `local_commit` and `update_pr`. - Reject zero-cycle and unsupported review-only invocations, and ambiguous source-binding/publication-policy combinations, with actionable diagnostics. - Add complete valid examples for both publication policies (including `changes_remaining` and `blocked` terminal results), each already in canonical serialized form, plus 62 unit tests covering valid, invalid, boundary, cross-document consistency, and round-trip/determinism cases. ## Why - Issue #96 is the first child of epic #95 ("Build a standalone review-fix-loop skill") and is the foundation the execution (#97/#99/#100) and reviewer-orchestration (#98) leaves depend on. This child intentionally implements no reviewer, fix, lock, worktree, or publication behavior — only the contracts those leaves will consume. Refs #96 Supports #95
Backfill the full SHA onto the prior verification-sufficiency-removal bullet and add today's new review-fix-loop contracts entry, per this repository's changelog convention (AGENTS.md).
…d evidence ## Summary - `validate_terminal_result` now rejects `terminal_state: converged` unless its own embedded evidence actually supports it: every `validation_summary` entry must be `passed`, and `review_records` must contain an entry bound to the exact final head and comparison base whose `aggregate_verdict` is `clean` and `write_isolation` is `enforced`. - Add five negative tests reproducing the exact gap review-code-change found: a failed or unavailable required validation entry, a non-clean or write-isolation-violated final-head review record, and a missing final-head review record all now fail closed instead of silently validating. - Document the new check in `references/CONTRACT.md`. ## Why - The initial review-code-change pass found this a blocking correctness gap: CONTRACT.md already claimed "the validator enforces the combination so a result cannot claim a terminal state without the evidence that state requires," but `validate_terminal_result` never actually inspected `review_records` or `validation_summary`, so a document could claim `converged` while its own embedded evidence showed a failing check, an open finding, or a compromised reviewer boundary. This mirrors the sibling review-suite validator's existing "clean cannot pair with failed validation" rule. Refs #96 Supports #95
Backfill SHAs onto the two prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
…l-result cross-check ## Summary - `validate_terminal_against_checkpoint` now also compares the checkpoint's current `comparison_base.sha` against the terminal result's `comparison_base.final.sha`, matching CONTRACT.md's existing documented claim that this function confirms "head/base identities" match. - Add a negative test proving a terminal result with a fabricated comparison base, otherwise internally self-consistent, is now rejected against the real checkpoint. ## Why - The second review-code-change pass found this a blocking correctness gap: the function only ever compared head SHAs and budget, silently omitting the base-identity leg CONTRACT.md already documented it as covering. A terminal result could report a comparison base its own checkpoint never recorded and still pass this integrity check. Refs #96 Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
…se history match ## Summary - `_check_converged_requires_clean_evidence` now rejects `converged` when `validation_summary` is empty or missing its required `focused` or `full` scope, not only when a present entry has failed. - `validate_terminal_against_checkpoint` now also compares the terminal result's `comparison_base.initial.sha` against the checkpoint's `base_revision_history[0].sha`, closing the remaining half of the head/base reconciliation CONTRACT.md documents. - Add four negative tests reproducing both gaps: an empty `validation_summary`, a `validation_summary` missing its `full` scope, and a terminal result with a fabricated initial comparison base. ## Why - The third review-code-change pass found both blocking: a `converged` result with an empty or scope-incomplete `validation_summary` still claimed "required validation passed" with zero errors, and the checkpoint/terminal-result cross-check compared only the final comparison base, never the initial one, despite CONTRACT.md already documenting it as reconciling "head/base identities" and "history." Refs #96 Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
…s-check ## Summary - Add `validate_checkpoint_against_invocation(invocation, checkpoint)`, symmetric to the existing `validate_terminal_against_checkpoint`: it cross-checks `initial_head`, `base_revision_history[0].sha`, `original_cycle_budget`, `invocation_id`, `repository`, and `publication.policy` against the invocation the checkpoint derives from. - Correct CONTRACT.md's prior overclaim that `validate_checkpoint` alone enforces `base_revision_history[0]` against "the invocation's original comparison-base SHA" — nothing inside a checkpoint document alone can prove that, since it has no other field to compare against; only the new cross-document function can. - Add 8 tests: two positive (both example invocation/checkpoint pairs) and six negative, one per cross-checked field. - Document the new function in SKILL.md's importable-functions list. ## Why - Authorized by the coordinator to exceed the default three-cycle fix/re-review budget by exactly one additional cycle for this ticket only, to close the gap the fourth review-code-change pass found: `validate_checkpoint` never verified `base_revision_history[0]` against anything but itself, even though CONTRACT.md already documented that as an enforced rule. The terminal-result side already had the symmetric check (`validate_terminal_against_checkpoint`); the checkpoint side did not. Refs #96 Supports #95
…elog Backfill the SHA onto the prior review-fix-loop entry and add today's new fix entry, per this repository's changelog convention.
…erated fields ## Summary - `validate_terminal_against_checkpoint` now also cross-checks `repository`, `branch`, and `publication.policy` between a terminal result and its checkpoint, mirroring the equivalent checks already present in `validate_checkpoint_against_invocation`. - Add `change_contract.allowed_remediation_scope` (required) to `invocation.schema.json`, and `worktree` and `validation_outcomes` (both required) to `checkpoint.schema.json`, matching the exact fields `design/review-fix-loop.md` enumerates for the change contract and the durable checkpoint. `validation_outcomes` follows the established status/result/reason cross-field shape used by every other validation collection in this contract family. - Update all invocation and checkpoint examples with the new fields, and add positive/negative tests for every new check. - Update `references/CONTRACT.md` to document the new fields and checks. ## Why - The fifth review-code-change pass on #96 found: (1) the checkpoint/terminal-result cross-check omitted repository, branch, and publication policy, the same drift class four prior fixes already closed for the invocation/checkpoint pair; and (2) two schemas omitted fields `design/review-fix-loop.md` explicitly enumerates for the exact sections this ticket implements, which would otherwise require a breaking schema revision in #97/#98. - Authorized by the coordinator as one further fix/re-review cycle for this ticket, explicitly bundling in the deferred design-field gap so it does not need its own future breaking revision. Refs #96 Supports #95
Correct a misattributed SHA from an earlier entry (it described commit 3d240c6 but cited 86c7df2, a different docs-only commit), backfill the missing entry for that docs-only commit, backfill the SHA onto the prior fix entry, and add today's new fix entry, per this repository's changelog convention.
…commit provenance ## Summary - Enumerate the complete, closed set of invariant identity fields that must agree across invocation, checkpoint, and terminal-result (invocation_id, repository, branch, original fix-cycle budget, publication.policy, initial head, initial comparison base) in a new `references/CONTRACT.md` table, and verify both cross-document functions (`validate_checkpoint_against_invocation` and `validate_terminal_against_checkpoint`) check the exact same set. - Close the one remaining gap the table surfaced: `validate_checkpoint_against_invocation` was still missing the `branch` check `validate_terminal_against_checkpoint` already had. - Close the commit-provenance gap in `validate_terminal_result`: `created_commits` must equal `head_history[1:]` in order; a `selected` finding disposition requires `fix_commit_sha`, a `declined` one must not carry one, and any present `fix_commit_sha` must appear in `created_commits`. - Document, rather than silently omit, the one part of that recommendation deliberately not enforced: `unpushed_commits` is not constrained to be a subset of `created_commits`/`head_history`, because a resumed or already-diverged invocation can legitimately report local commits ahead of its recorded source that predate this invocation's own `head.initial` and were never created by it — recovery/resume semantics are this ticket's stated non-goal, so this boundary is recorded rather than guessed at. - Add 8 new tests (2 for the branch cross-check, 4 for commit provenance, plus the field-set enumeration is exercised transitively by the existing positive/negative cross-document tests). ## Why - The sixth review-code-change pass found the checkpoint↔invocation branch gap; the coordinator asked for a systematic sweep (enumerate the complete invariant field set, check it symmetrically in both cross-document functions, rather than patching one field per cycle) plus the still-open commit-provenance recommendation from the prior cycle, in one pass. Refs #96 Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
…ntity closure
## Summary
- Add the two remaining design-required checkpoint fields:
`preserved_failed_attempts` (mirroring terminal-result's shape) and
`pull_request` ({repository, number}, distinct from
`publication.pull_request`'s push-mechanics fields).
- `validate_checkpoint` now requires `preserved_failed_attempts` to have
exactly one entry per `cycle_attempts` entry whose outcome is `failed`
or `interrupted`.
- Add a shared `_pull_request_identity_mismatch` helper and wire it into
both `validate_checkpoint_against_invocation` and
`validate_terminal_against_checkpoint`, so the optional pull-request
identity is cross-checked wherever two adjacent documents both carry it
— closing the same drift class the last several cycles have been
closing for the required invariant fields.
- Update all three checkpoint examples and add 12 new tests (positive and
negative, for both the preserved-attempts count rule and the
pull-request cross-checks in both directions).
- Document all of this in `references/CONTRACT.md`.
## Why
- The seventh review-code-change pass found `checkpoint.schema.json`
still missing two fields `design/review-fix-loop.md` explicitly
requires as durable-checkpoint content, mirroring a completeness gap
the fifth pass already closed for two other fields. Rather than patch
one field per cycle again, this closes the checkpoint schema
completely against the design's enumerated content and extends the
cross-document invariant enforcement to cover the newly added optional
pull-request identity too, so it does not become the next cycle's
single-field gap.
Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
…e ahead/behind ## Summary - Add the three remaining design-required terminal-result fields: `worktree`, `resume_status` (`not_resumed`/`resumed`/`resume_unavailable`), and `unresolved_or_deferred_findings`. - Require `ahead_by` and `behind_by` alongside the head fields whenever `source.status` is `bound`, in both `validate_checkpoint` and `validate_terminal_result` — a bound source that reports a head but omits the ahead/behind counts does not satisfy the design's "local ahead/behind state" requirement. - Update all four terminal-result examples with the new fields and add 8 new tests (schema-presence and bound-source rejection, both directions). - Document all of this in `references/CONTRACT.md`. ## Why - The eighth review-code-change pass found two blocking gaps (`terminal-result.schema.json` missing `worktree` and `resume_status`, both explicitly enumerated in `design/review-fix-loop.md`'s Terminal result contract section) and one strong recommendation (bound sources silently omitting ahead/behind counts). While closing those, a systematic re-read of the same design section surfaced one more explicitly named field this candidate had not yet modeled (`unresolved_or_deferred_findings`), addressed in the same pass rather than deferring it to a ninth cycle. Refs #96 Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
… cleanliness ## Summary - Add a required `reviewer_identity` (non-empty string) field to `review_records` in both `checkpoint.schema.json` and `terminal-result.schema.json`, distinct from the invocation-invariant `review_independence` enum, so a fresh-subagent reviewer's actual per-pass identity can be recorded and distinguished across heads. - Add a `validate_invocation` check rejecting non-empty `candidate.worktree.staged`/`unstaged`/`untracked`, leaving `ignored` unconstrained, matching the design's "every invocation requires a dedicated globally clean worktree" hard precondition. - Update all 7 affected examples with distinct per-pass `reviewer_identity` values, and add 8 new tests (missing-field rejection and a distinguishing-identities test for both schemas; one rejection test per dirty worktree category plus a passing test proving `ignored` is unconstrained). - Document both additions in `references/CONTRACT.md`. ## Why - The ninth review-code-change pass — an intentionally maximal, final re-derivation of the design's complete field lists rather than trusting prior passes were exhaustive — found both gaps: the design's Terminal result contract names "reviewer identities" as required content and its Validation strategy requires a test for "different reviewer identities per head," which the two-valued `review_independence` enum alone cannot satisfy; and `validate_invocation` captured full worktree state but never used it to reject a dirty worktree, despite the design stating that blocks startup. Refs #96 Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
…a mutation attempt ## Summary - `_check_converged_requires_clean_evidence` now rejects `converged` when *any* `review_records` entry has a non-empty `mutation_attempts`, not only the final-head-bound entry. - Add two negative tests: a mutation attempt on the final record, and one on a non-final (earlier-head) record, both rejected. - Document the rule in `references/CONTRACT.md`. ## Why - The tenth review-code-change pass found that an attempted reviewer mutation on an earlier pass did not block `converged`, as long as a later pass on a different head came back clean and enforced. Design's "Reviewer write prevention" section states an attempted prohibited mutation invalidates that pass even if the runtime blocked it, regardless of a later clean pass — so this closes the last gap in the converged-evidence check. Refs #96 Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention.
6 tasks
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defines the public
review-fix-loopinvocation, checkpoint, grant, cycle-budget,review-record, and terminal-result contracts, per
design/review-fix-loop.md,under a new
skills/review-fix-loop/skill directory:invocation,checkpoint,terminal-result) coveringevery field named in the design's "Invocation contract," "Durable invocation
checkpoint," and "Terminal result contract" sections, including candidate
worktree-cleanliness enforcement,
allowed_remediation_scope, checkpointworktree/validation_outcomes/preserved_failed_attempts/pull_request,terminal-result
worktree/resume_status/unresolved_or_deferred_findings,and a per-pass
reviewer_identityonreview_records.scripts/validate.py, mirroring the repository'sestablished
review-code-change/review-suite validator convention, enforcingboth the JSON Schemas and the cross-field semantics documented in
references/CONTRACT.md— including a "Cross-document identity invariants"table enumerating the complete closed field set that must agree across
invocation/checkpoint/terminal-result, enforced symmetrically by
validate_checkpoint_against_invocationandvalidate_terminal_against_checkpoint.local_commitandupdate_pr, includingchanges_remainingandblockedterminal states), each in canonicalserialized form.
round-trip/determinism cases.
This is the first child of epic #95 and the foundation the execution (#97,
#99, #100) and reviewer-orchestration (#98) leaves depend on. Per its scope,
it does not run reviewers, apply fixes, acquire locks, manage worktrees,
recover from interruption, or publish anything.
Acceptance criteria
local_commitandupdate_prpass strict validation.Validation
python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py'— 115 tests, pass.just format— pass, no reformatting needed at head.just lint— pass, includingskills-ref validate skills/review-fix-loop.just test— full repository suite (implement-epic, implement-ticket, babysit-pr,carve-changesets, all four review skills, review-fix-loop's own 115 tests, and
review-suite's 310 tests) — pass, no failures.
Review history
This candidate went through 11 full
review-code-changepasses (each a fresh,isolated, read-only reviewer given only raw evidence) across 10 fix cycles,
converging on a systematic completeness effort against
design/review-fix-loop.md's enumerated field lists:validate_terminal_result(embeddedreview_records/validation_summarynot checked) — fixed.comparison_baseomitted from the checkpoint/terminal-result cross-check — fixed.validation_summaryand initial-base cross-check gap — fixed.validate_checkpoint_against_invocationadded (base/budget/repository/policy) — fixed.branch/repository/policy checkpoint cross-check,allowed_remediation_scope,checkpoint
worktree/validation_outcomes, misattributed changelog SHA — fixed.(
created_commits/fix_commit_shalinkage) — fixed.preserved_failed_attempts/pull_request— fixed.worktree/resume_status/unresolved_or_deferred_findings,bound-source
ahead_by/behind_bynot enforced — fixed.reviewer_identity, invocation worktree-cleanliness never enforced — fixed.mutation_attemptson a non-final review record — fixed.The eleventh (final authorized) pass found one more item, recorded below as a
disclosed follow-up rather than triggering a twelfth cycle.
Known follow-up
The eleventh review-code-change pass found one blocking gap, reproduced
directly against the checked-in examples, that this PR intentionally does not
fix (final authorized review cycle for this round):
Guardrail evidence
Diff is ~4,160 insertions across 17 files — well above the "few hundred lines"
preferred range in
carve-changesets' cognitive-load guardrails. Classified aswithin guardrails rather than oversized:
terminal-result contracts for a single new skill — not a mix of unrelated
concerns; the three schemas and their two cross-document validator functions
are inherently interdependent (the "Cross-document identity invariants"
table only makes sense reviewed together), so splitting by schema would
actively harm reviewability rather than help it.
8 JSON example fixtures and ~1,165 lines of largely formulaic unit tests.
validate.pyitself is ~500 lines of validation logic.successfully reasoned about the complete diff as one unit, reproduced
specific claims against it, and gave precise line-level citations —
empirical evidence the diff is within a single reviewer's cognitive
capacity despite its raw size.
instead requires reporting guardrail evidence rather than carving, which
this section does.
Refs #96
Supports #95