From 0689cda71833751249ebc5e65b766d231cf2c093 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 10:43:58 -0700 Subject: [PATCH 01/22] feat(review-fix-loop): define invocation, checkpoint, and terminal-result contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/SKILL.md | 84 +++ skills/review-fix-loop/references/CONTRACT.md | 149 +++++ .../references/checkpoint.schema.json | 170 +++++ ...l-commit-checkpoint-changes-remaining.json | 108 +++ .../examples/local-commit-checkpoint.json | 72 ++ .../examples/local-commit-invocation.json | 70 ++ ...cal-commit-terminal-changes-remaining.json | 140 ++++ .../local-commit-terminal-converged.json | 109 +++ .../examples/update-pr-checkpoint.json | 79 +++ .../examples/update-pr-invocation.json | 87 +++ ...e-pr-terminal-blocked-remote-advanced.json | 119 ++++ .../update-pr-terminal-converged.json | 116 ++++ .../references/invocation.schema.json | 242 +++++++ .../references/terminal-result.schema.json | 257 ++++++++ .../scripts/tests/test_validate.py | 622 ++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 472 +++++++++++++ 16 files changed, 2896 insertions(+) create mode 100644 skills/review-fix-loop/SKILL.md create mode 100644 skills/review-fix-loop/references/CONTRACT.md create mode 100644 skills/review-fix-loop/references/checkpoint.schema.json create mode 100644 skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json create mode 100644 skills/review-fix-loop/references/examples/local-commit-checkpoint.json create mode 100644 skills/review-fix-loop/references/examples/local-commit-invocation.json create mode 100644 skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json create mode 100644 skills/review-fix-loop/references/examples/local-commit-terminal-converged.json create mode 100644 skills/review-fix-loop/references/examples/update-pr-checkpoint.json create mode 100644 skills/review-fix-loop/references/examples/update-pr-invocation.json create mode 100644 skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json create mode 100644 skills/review-fix-loop/references/examples/update-pr-terminal-converged.json create mode 100644 skills/review-fix-loop/references/invocation.schema.json create mode 100644 skills/review-fix-loop/references/terminal-result.schema.json create mode 100644 skills/review-fix-loop/scripts/tests/test_validate.py create mode 100644 skills/review-fix-loop/scripts/validate.py diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md new file mode 100644 index 0000000..82cb0aa --- /dev/null +++ b/skills/review-fix-loop/SKILL.md @@ -0,0 +1,84 @@ +--- +name: review-fix-loop +description: Validate the review-fix-loop invocation, checkpoint, and terminal-result contracts that later children of epic #95 use to run the standalone review-remediation workflow. Use when authoring, resuming, or terminating a review-fix-loop invocation to check its documents against the shared schemas before trusting or acting on them. This child does not yet run reviewers, apply fixes, acquire locks, manage worktrees, or publish anything; see design/review-fix-loop.md and references/CONTRACT.md for the full behavioral design and current implementation status. +allowed-tools: Read, Bash +--- + +# Review Fix Loop + +`review-fix-loop` will become a repository-owned skill that takes cooperative +ownership of an existing committed candidate, runs the complete repository +review suite, applies material ticket-scoped fixes, and repeats until review +converges or a bounded stop condition is reached. The full design is +[`design/review-fix-loop.md`](../../design/review-fix-loop.md). + +This child ([issue #96](https://github.com/shaug/agent-scripts/issues/96), the +first of epic [#95](https://github.com/shaug/agent-scripts/issues/95)) defines +and validates only the contracts later children build on: + +- the **invocation** a caller or standalone operator supplies to start or resume + a loop; +- the **durable checkpoint** the loop would record between phases; and +- the **terminal result** the loop returns. + +It does not run a reviewer, apply a fix, acquire a lock, manage a worktree, +recover from interruption, or publish anything — those behaviors belong to +issues #97 (local locking, isolated attempts, and recovery), #98 (reviewer +isolation and orchestration), #99 (`local_commit`), and #100 (`update_pr`). Do +not invoke this skill expecting an executable review-fix loop yet; use it to +validate a document you or a later child produced against the shared schemas. + +## Load the contracts + +Read [`references/CONTRACT.md`](references/CONTRACT.md) and the three schemas +beside it before authoring or trusting any invocation, checkpoint, or +terminal-result document: + +- [`references/invocation.schema.json`](references/invocation.schema.json) +- [`references/checkpoint.schema.json`](references/checkpoint.schema.json) +- [`references/terminal-result.schema.json`](references/terminal-result.schema.json) + +[`references/examples/`](references/examples) contains complete, valid documents +for both the `local_commit` and `update_pr` publication policies, including a +`changes_remaining` and a `blocked` terminal result, each already in the +canonical serialized form `scripts/validate.py` produces. + +## Validate a document + +`scripts/validate.py` is dependency-free, matching the convention used by the +bundled `review-code-change` review-suite contract, so it works wherever this +skill is installed: + +```bash +python3 skills/review-fix-loop/scripts/validate.py invocation path/to/invocation.json +python3 skills/review-fix-loop/scripts/validate.py checkpoint path/to/checkpoint.json +python3 skills/review-fix-loop/scripts/validate.py terminal-result path/to/result.json +``` + +Each prints `valid : ` and exits `0` on success, or prints one +diagnostic per violation to stderr and exits `1`. A malformed document exits +`2`. + +The module also exposes importable functions for a caller that already holds +parsed documents in memory: `validate_invocation`, `validate_checkpoint`, +`validate_terminal_result`, `reconstruct_cycle_accounting` (derive consumed and +remaining fix cycles from a checkpoint's recorded attempts), and +`validate_terminal_against_checkpoint` (confirm a terminal result's budget and +head/base identities match the checkpoint it derives from). See +[`references/CONTRACT.md`](references/CONTRACT.md) for the cross-field semantics +these functions enforce beyond plain JSON Schema, and +`scripts/tests/test_validate.py` for the complete valid, invalid, boundary, and +round-trip case coverage. + +## Non-goals of this child + +- Running a reviewer or applying a fix. +- Acquiring a local or remote-target lock, managing a worktree, or recovering an + interrupted attempt. +- Publishing anything, including the `update_pr` expected-old fast-forward + update. +- Migrating `implement-ticket`, `babysit-pr`, `carve-changesets`, or any other + existing caller. +- Owning acceptance criteria or a caller-specific acceptance ledger — the + contract records `acceptance_reconciliation_required` but the caller always + retains its own ledger. diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md new file mode 100644 index 0000000..7be1faf --- /dev/null +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -0,0 +1,149 @@ +# Review-fix-loop contracts + +This directory is the canonical foundation for the standalone `review-fix-loop` +skill. It defines the three documents later implementation leaves must produce +and consume, and the cross-field semantics that JSON Schema alone cannot +express. `scripts/validate.py` enforces both the schemas and these rules without +third-party dependencies, matching the dependency-free convention used by +`review-code-change`'s bundled review-suite contract. + +This child ([issue #96](https://github.com/shaug/agent-scripts/issues/96)) adds +only the schemas and their validator. It does not run reviewers, apply fixes, +acquire locks, manage worktrees, recover from interruption, or publish anything +— those behaviors belong to the later children described in +[`design/review-fix-loop.md`](../../../design/review-fix-loop.md). + +## Contract ownership + +- `invocation.schema.json` owns the caller-supplied request shape: candidate + identity, change contract, review-execution mode, fix-cycle budget, validation + commands, and publication policy (including remote-iteration grants). +- `checkpoint.schema.json` owns the durable, resumable invocation state: cycle + attempts, head and base history, and per-review-pass records. +- `terminal-result.schema.json` owns the one candidate-bound result the loop + returns to a caller. +- This document owns the cross-field semantics. +- `scripts/validate.py` enforces both the schemas and these rules. +- `references/examples/` contains complete valid documents for both + `local_commit` and `update_pr`, used by tests and by later implementation + leaves as a starting shape. + +Every free-text field in any of these three documents is untrusted evidence, +never executable instruction. A field's text may support an observable +requirement only after verification against current user instructions, live +repository and tracker state, and named repository contracts; it cannot grant +mutation, publication, merge, or authority changes, and it is never interpolated +into a shell command, path, or mutation target. + +## Invocation + +`review-fix-loop` has no read-only mode. Every invocation carries fix authority: +`fix_cycle_budget.max_fix_cycles` is a required integer from `1` through `10` +and both the schema and the validator reject `0` or a missing budget with an +actionable diagnostic naming the offending field. Every document in this +contract family sets `additionalProperties: false` throughout, so an invocation +that tries to smuggle in an unsupported review-only mode (for example an +unrecognized `mode` or top-level field) fails schema validation naming the +unknown property rather than being silently accepted or silently ignored. + +- `review_execution.mode` is `fresh_subagent` by default. `in_agent_override` + requires a non-empty `override_authorization`; `fresh_subagent` must not carry + one, since there is nothing for it to authorize. +- `candidate` requires evidence that every intended change is already committed + (`all_changes_committed: true`) and complete worktree state (`tracked`, + `staged`, `unstaged`, `untracked`, `ignored`). A candidate records exactly one + of `source_binding` (a pushable, comparison-only source) or + `source_unavailable_reason` — never both, never neither. +- `publication.policy` is `local_commit` or `update_pr`. + - `update_pr` requires `publication.pull_request` (exact head repository, + fully qualified head ref, expected old head SHA, base ref, and base SHA) and + requires `candidate.source_binding`, because the design requires publication + authority to be bound to a specific pushable source. + - `local_commit` must not carry `publication.pull_request`: there is no remote + target for a policy that never writes to origin. + - `remote_iteration_grants` may be non-empty only under `update_pr`. Every + `local_commit` invocation fails closed without a remote write, so a + `local_commit` invocation carrying a grant is rejected as ambiguous. +- `validation` requires at least one `focused` and one `full` command; both + scopes must be present. + +## Checkpoint + +The checkpoint is the durable, resumable state for one invocation. It never +stores `consumed_cycles` or `remaining_cycles` directly: cycle accounting is +reconstructed from `cycle_attempts`, so the derived numbers can never drift from +the history that produced them. `scripts/validate.py` exposes +`reconstruct_cycle_accounting(checkpoint)` for this purpose. A cycle is consumed +by starting an attempt, whether it is later `committed`, `failed`, or +`interrupted` — the design's rule that the reserved cycle is spent regardless of +outcome. The validator therefore rejects a checkpoint whose attempt count +exceeds `original_cycle_budget`. + +- `head_history[0]` must equal `initial_head` and `head_history[-1]` must equal + `current_head`. Only a `committed` cycle attempt may advance the head; the + number of `committed` attempts must equal `len(head_history) - 1`, and each + committed attempt's `resulting_head` must equal the corresponding subsequent + `head_history` entry in order. +- `base_revision_history[0].sha` must equal the invocation's original + comparison-base SHA and the last entry is the live current base. +- `review_records` bind every review pass to the exact head and base it + reviewed. `write_isolation: violated` records an attempted or unattributed + reviewer mutation; it does not by itself imply which terminal `blocked` reason + applies — that judgment belongs to the phase that observed it. + +## Terminal result + +Every terminal state has an explicit publication, retained-commit, and +operator-action contract. The validator enforces the combination so a result +cannot claim a terminal state without the evidence that state requires: + +- `converged`: the aggregate review is `clean` for the final head and base, + required validation passed, and the selected publication policy completed. + `reason` must be absent. Under `update_pr`, `publication.status` must be + `published` and `unpushed_commits` must be empty. Under `local_commit`, + `publication.status` must be `not_applicable` (local_commit never writes to + origin), and any created commits remain in `unpushed_commits` — that is the + expected, non-error shape of a converged `local_commit` result, and + `operator_action` must describe how the operator publishes them through their + own workflow. +- `changes_remaining`: `reason` must be one of `cycle_budget_exhausted`, + `repeated_finding`, `oscillation`, `expanding_findings`, + `repeated_failed_attempt`, or `current_candidate_validation_failure`. + `publication.status` must be `not_applicable` under `local_commit` or + `withheld` under `update_pr` — remediation stopped before convergence, so + nothing is published. `operator_action` names the concrete remaining work. +- `blocked`: `reason` must be one of `candidate_busy`, + `candidate_integrity_failure`, `checkpoint_mismatch`, `missing_capability`, + `missing_authority`, `insufficient_change_contract`, + `reviewer_integrity_failure`, `validation_unavailable`, `base_drift`, + `remote_advanced`, `publication_failed`, `scope_decision_required`, or + `operator_input_required`. `publication.status` must be `not_applicable` under + `local_commit`; under `update_pr` it is `withheld` unless the block reason is + `remote_advanced` or `publication_failed`, in which case it is `failed`. + `operator_action` names the concrete decision or repair the loop cannot make + on its own. + +Independent of terminal state: whenever `head.final` differs from `head.initial` +and `publication.status` is not `published`, `unpushed_commits` must be +non-empty — every unconverged or local-only result reports the exact retained +commits rather than silently dropping them. `acceptance_reconciliation_required` +must be `true` whenever `head.final != head.initial` or +`comparison_base.final != comparison_base.initial`; the loop never implies +ticket or PR acceptance merely by converging. +`budget.consumed_cycles + budget.remaining_cycles` must equal +`budget.original_max_fix_cycles`. + +`scripts/validate.py` also exposes +`validate_terminal_against_checkpoint(checkpoint, terminal_result)` to confirm a +terminal result's budget and head/base identities are the ones actually recorded +by its checkpoint, so a result cannot report cycle accounting or history that +its own checkpoint does not support. + +## Determinism + +`scripts/validate.py` exposes `canonical_json(document)`, which serializes with +sorted keys and a trailing newline. Every example under `references/examples/` +round-trips: parsing, validating, and re-serializing an example produces +byte-identical output to the checked-in file, and parsing that output again +produces an equal Python object. This is the same guarantee `just format` and +`just lint` already expect from every other repository-owned schema. diff --git a/skills/review-fix-loop/references/checkpoint.schema.json b/skills/review-fix-loop/references/checkpoint.schema.json new file mode 100644 index 0000000..ca28f07 --- /dev/null +++ b/skills/review-fix-loop/references/checkpoint.schema.json @@ -0,0 +1,170 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-fix-loop/checkpoint.schema.json", + "title": "Review-fix-loop durable checkpoint", + "type": "object", + "additionalProperties": false, + "required": [ + "schema_version", + "invocation_id", + "repository", + "branch", + "initial_head", + "current_head", + "comparison_base", + "publication", + "original_cycle_budget", + "cycle_attempts", + "head_history", + "base_revision_history", + "review_records", + "source", + "current_phase", + "expected_next_action" + ], + "properties": { + "schema_version": {"const": "1.0"}, + "invocation_id": {"type": "string", "minLength": 1}, + "repository": { + "type": "object", + "additionalProperties": false, + "required": ["identity", "git_common_directory"], + "properties": { + "identity": {"type": "string", "minLength": 1}, + "git_common_directory": {"type": "string", "minLength": 1} + } + }, + "branch": {"type": "string", "minLength": 1}, + "initial_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "current_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base": { + "type": "object", + "additionalProperties": false, + "required": ["ref", "sha"], + "properties": { + "ref": {"type": "string", "minLength": 1}, + "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "publication": { + "type": "object", + "additionalProperties": false, + "required": ["policy"], + "properties": { + "policy": {"enum": ["local_commit", "update_pr"]}, + "pull_request": { + "type": "object", + "additionalProperties": false, + "required": ["head_repository", "head_ref", "base_ref"], + "properties": { + "head_repository": {"type": "string", "minLength": 1}, + "head_ref": {"type": "string", "minLength": 1}, + "base_ref": {"type": "string", "minLength": 1} + } + } + } + }, + "original_cycle_budget": {"type": "integer", "minimum": 1, "maximum": 10}, + "cycle_attempts": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["sequence", "started_from_head", "outcome"], + "properties": { + "sequence": {"type": "integer", "minimum": 1}, + "started_from_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "outcome": {"enum": ["committed", "failed", "interrupted"]}, + "resulting_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "finding_id": {"type": "string", "minLength": 1} + } + } + }, + "head_history": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + }, + "base_revision_history": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["ref", "sha"], + "properties": { + "ref": {"type": "string", "minLength": 1}, + "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + } + }, + "review_records": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "sequence", + "head_sha", + "comparison_base_sha", + "review_independence", + "write_isolation", + "aggregate_verdict", + "finding_dispositions", + "mutation_attempts" + ], + "properties": { + "sequence": {"type": "integer", "minimum": 1}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "review_independence": {"enum": ["fresh_subagent", "in_agent_override"]}, + "write_isolation": {"enum": ["enforced", "violated"]}, + "aggregate_verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "finding_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["finding_id", "disposition", "rationale"], + "properties": { + "finding_id": {"type": "string", "minLength": 1}, + "disposition": {"enum": ["accepted", "rejected", "deferred"]}, + "rationale": {"type": "string", "minLength": 1} + } + } + }, + "mutation_attempts": { + "type": "array", + "items": {"type": "string", "minLength": 1} + } + } + } + }, + "source": { + "type": "object", + "additionalProperties": false, + "required": ["status"], + "properties": { + "status": {"enum": ["bound", "unavailable"]}, + "last_verified_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "ahead_by": {"type": "integer", "minimum": 0}, + "behind_by": {"type": "integer", "minimum": 0}, + "unavailable_reason": {"type": "string", "minLength": 1} + } + }, + "current_phase": { + "enum": [ + "resolve", + "establish_evidence", + "review", + "decide", + "fix", + "validate_and_commit", + "invalidate_and_repeat", + "publish", + "return" + ] + }, + "expected_next_action": {"type": "string", "minLength": 1} + } +} diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json new file mode 100644 index 0000000..c5b8214 --- /dev/null +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json @@ -0,0 +1,108 @@ +{ + "base_revision_history": [ + { + "ref": "main", + "sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912" + } + ], + "branch": "fix/96-budget-example", + "comparison_base": { + "ref": "main", + "sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912" + }, + "current_head": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", + "current_phase": "return", + "cycle_attempts": [ + { + "finding_id": "correctness-001", + "outcome": "committed", + "resulting_head": "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "sequence": 1, + "started_from_head": "98676d2fa265321c8399de22ee6607c84a4f2e46" + }, + { + "finding_id": "correctness-001", + "outcome": "failed", + "sequence": 2, + "started_from_head": "8fbc9b399aa14299d694e4beca34dc371baa7e58" + }, + { + "finding_id": "correctness-001", + "outcome": "committed", + "resulting_head": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", + "sequence": 3, + "started_from_head": "8fbc9b399aa14299d694e4beca34dc371baa7e58" + } + ], + "expected_next_action": "operator input required; the same finding survived three fix cycles", + "head_history": [ + "98676d2fa265321c8399de22ee6607c84a4f2e46", + "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af" + ], + "initial_head": "98676d2fa265321c8399de22ee6607c84a4f2e46", + "invocation_id": "review-fix-loop-96-budget-exhausted-example", + "original_cycle_budget": 3, + "publication": { + "policy": "local_commit" + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "off-by-one boundary defect" + } + ], + "head_sha": "98676d2fa265321c8399de22ee6607c84a4f2e46", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "same off-by-one boundary defect survived the first attempted fix" + } + ], + "head_sha": "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "same off-by-one boundary defect survived the second attempted fix" + } + ], + "head_sha": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 3, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "status": "unavailable", + "unavailable_reason": "standalone invocation has no recorded pushable source" + } +} diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json new file mode 100644 index 0000000..a281dc1 --- /dev/null +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json @@ -0,0 +1,72 @@ +{ + "base_revision_history": [ + { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd" + } + ], + "branch": "fix/96-example", + "comparison_base": { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd" + }, + "current_head": "b13de54b94608d3918bacdc14b4836332d8e0630", + "current_phase": "return", + "cycle_attempts": [ + { + "finding_id": "correctness-001", + "outcome": "committed", + "resulting_head": "b13de54b94608d3918bacdc14b4836332d8e0630", + "sequence": 1, + "started_from_head": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1" + } + ], + "expected_next_action": "none; converged result ready to return", + "head_history": [ + "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "b13de54b94608d3918bacdc14b4836332d8e0630" + ], + "initial_head": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "invocation_id": "review-fix-loop-96-local-commit-example", + "original_cycle_budget": 3, + "publication": { + "policy": "local_commit" + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "6d4e80a7e96351b471797a093ebc111917c516bd", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "add() returns None instead of the sum; ticket-scoped and tractable" + } + ], + "head_sha": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "clean", + "comparison_base_sha": "6d4e80a7e96351b471797a093ebc111917c516bd", + "finding_dispositions": [], + "head_sha": "b13de54b94608d3918bacdc14b4836332d8e0630", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "status": "unavailable", + "unavailable_reason": "standalone invocation has no recorded pushable source" + } +} diff --git a/skills/review-fix-loop/references/examples/local-commit-invocation.json b/skills/review-fix-loop/references/examples/local-commit-invocation.json new file mode 100644 index 0000000..8b4663f --- /dev/null +++ b/skills/review-fix-loop/references/examples/local-commit-invocation.json @@ -0,0 +1,70 @@ +{ + "candidate": { + "all_changes_committed": true, + "branch": "fix/96-example", + "comparison_base": { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd" + }, + "diff": { + "complete": true, + "content": "diff --git a/example.py b/example.py\n@@ -1 +1,2 @@\n def add(a, b):\n+ return a + b\n", + "format": "unified_diff" + }, + "head_sha": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "source_unavailable_reason": "standalone invocation has no recorded pushable source", + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] + } + }, + "change_contract": { + "acceptance_criteria": [ + "add(a, b) returns the sum of its arguments" + ], + "goal": "Implement the missing add() body.", + "non_goals": [ + "Refactoring unrelated arithmetic helpers" + ], + "preserved_behaviors": [ + "Existing call sites of add() keep their current signature" + ], + "sources": { + "named_documents": [], + "nearby_patterns": [], + "repository_instructions": [] + } + }, + "fix_cycle_budget": { + "max_fix_cycles": 3 + }, + "invocation_id": "review-fix-loop-96-local-commit-example", + "publication": { + "policy": "local_commit" + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_execution": { + "mode": "fresh_subagent" + }, + "schema_version": "1.0", + "validation": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "scope": "focused" + }, + { + "command": "just test", + "name": "full repository gate", + "scope": "full" + } + ] +} diff --git a/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json new file mode 100644 index 0000000..4af4976 --- /dev/null +++ b/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json @@ -0,0 +1,140 @@ +{ + "acceptance_reconciliation_required": true, + "base_revision_history": [ + { + "ref": "main", + "sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912" + } + ], + "branch": "fix/96-budget-example", + "budget": { + "consumed_cycles": 3, + "original_max_fix_cycles": 3, + "remaining_cycles": 0 + }, + "comparison_base": { + "final": { + "ref": "main", + "sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912" + }, + "initial": { + "ref": "main", + "sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912" + } + }, + "created_commits": [ + "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af" + ], + "finding_dispositions": [ + { + "disposition": "selected", + "finding_id": "correctness-001", + "fix_commit_sha": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", + "rationale": "off-by-one boundary defect survived two prior attempted fixes" + } + ], + "head": { + "final": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", + "initial": "98676d2fa265321c8399de22ee6607c84a4f2e46" + }, + "head_history": [ + "98676d2fa265321c8399de22ee6607c84a4f2e46", + "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af" + ], + "invocation_id": "review-fix-loop-96-budget-exhausted-example", + "operator_action": "The off-by-one boundary defect (correctness-001) survived three fix cycles; an operator must resolve it directly or authorize a new invocation with a fresh budget.", + "preserved_failed_attempts": [ + { + "attempt_ref": "attempt/correctness-001-cycle-2", + "reason": "the second remediation attempt did not clear the finding and was discarded" + } + ], + "publication": { + "non_converged_exposure": false, + "policy": "local_commit", + "status": "not_applicable" + }, + "reason": "cycle_budget_exhausted", + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "off-by-one boundary defect" + } + ], + "head_sha": "98676d2fa265321c8399de22ee6607c84a4f2e46", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "same off-by-one boundary defect survived the first attempted fix" + } + ], + "head_sha": "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "413b8b2d6e46f8d784b080e8ca62fc3af6979912", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "same off-by-one boundary defect survived the second attempted fix" + } + ], + "head_sha": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 3, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "status": "unavailable", + "unavailable_reason": "standalone invocation has no recorded pushable source" + }, + "terminal_state": "changes_remaining", + "unpushed_commits": [ + "8fbc9b399aa14299d694e4beca34dc371baa7e58", + "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af" + ], + "validation_summary": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ] +} diff --git a/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json b/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json new file mode 100644 index 0000000..4aaae4d --- /dev/null +++ b/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json @@ -0,0 +1,109 @@ +{ + "acceptance_reconciliation_required": true, + "base_revision_history": [ + { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd" + } + ], + "branch": "fix/96-example", + "budget": { + "consumed_cycles": 1, + "original_max_fix_cycles": 3, + "remaining_cycles": 2 + }, + "comparison_base": { + "final": { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd" + }, + "initial": { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd" + } + }, + "created_commits": [ + "b13de54b94608d3918bacdc14b4836332d8e0630" + ], + "finding_dispositions": [ + { + "disposition": "selected", + "finding_id": "correctness-001", + "fix_commit_sha": "b13de54b94608d3918bacdc14b4836332d8e0630", + "rationale": "add() returns None instead of the sum; ticket-scoped and tractable" + } + ], + "head": { + "final": "b13de54b94608d3918bacdc14b4836332d8e0630", + "initial": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1" + }, + "head_history": [ + "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "b13de54b94608d3918bacdc14b4836332d8e0630" + ], + "invocation_id": "review-fix-loop-96-local-commit-example", + "operator_action": "Publish retained local commit b13de54b94608d3918bacdc14b4836332d8e0630 through your existing PR or merge workflow; review-fix-loop performs no remote write under local_commit.", + "preserved_failed_attempts": [], + "publication": { + "non_converged_exposure": false, + "policy": "local_commit", + "status": "not_applicable" + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "6d4e80a7e96351b471797a093ebc111917c516bd", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "correctness-001", + "rationale": "add() returns None instead of the sum; ticket-scoped and tractable" + } + ], + "head_sha": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "clean", + "comparison_base_sha": "6d4e80a7e96351b471797a093ebc111917c516bd", + "finding_dispositions": [], + "head_sha": "b13de54b94608d3918bacdc14b4836332d8e0630", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "status": "unavailable", + "unavailable_reason": "standalone invocation has no recorded pushable source" + }, + "terminal_state": "converged", + "unpushed_commits": [ + "b13de54b94608d3918bacdc14b4836332d8e0630" + ], + "validation_summary": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ] +} diff --git a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json new file mode 100644 index 0000000..bc589ff --- /dev/null +++ b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json @@ -0,0 +1,79 @@ +{ + "base_revision_history": [ + { + "ref": "main", + "sha": "f8943028b30c974048d55ced0d55368b1781d91b" + } + ], + "branch": "fix/100-example", + "comparison_base": { + "ref": "main", + "sha": "f8943028b30c974048d55ced0d55368b1781d91b" + }, + "current_head": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "current_phase": "return", + "cycle_attempts": [ + { + "finding_id": "code-simplicity-001", + "outcome": "committed", + "resulting_head": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "sequence": 1, + "started_from_head": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895" + } + ], + "expected_next_action": "none; converged and published", + "head_history": [ + "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7" + ], + "initial_head": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "invocation_id": "review-fix-loop-100-update-pr-example", + "original_cycle_budget": 3, + "publication": { + "policy": "update_pr", + "pull_request": { + "base_ref": "refs/heads/main", + "head_ref": "refs/heads/fix/100-example", + "head_repository": "shaug/agent-scripts" + } + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "f8943028b30c974048d55ced0d55368b1781d91b", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "code-simplicity-001", + "rationale": "list comprehension only rebuilds range(); list(range(n)) is equivalent and simpler" + } + ], + "head_sha": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "clean", + "comparison_base_sha": "f8943028b30c974048d55ced0d55368b1781d91b", + "finding_dispositions": [], + "head_sha": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "ahead_by": 0, + "behind_by": 0, + "last_verified_head": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "status": "bound" + } +} diff --git a/skills/review-fix-loop/references/examples/update-pr-invocation.json b/skills/review-fix-loop/references/examples/update-pr-invocation.json new file mode 100644 index 0000000..9fcc605 --- /dev/null +++ b/skills/review-fix-loop/references/examples/update-pr-invocation.json @@ -0,0 +1,87 @@ +{ + "candidate": { + "all_changes_committed": true, + "branch": "fix/100-example", + "comparison_base": { + "ref": "main", + "sha": "f8943028b30c974048d55ced0d55368b1781d91b" + }, + "diff": { + "complete": true, + "content": "diff --git a/example.py b/example.py\n@@ -3,3 +3,2 @@\n- values = [x for x in range(n)]\n- return values\n+ return list(range(n))\n", + "format": "unified_diff" + }, + "head_sha": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "pull_request": { + "number": 123, + "repository": "shaug/agent-scripts" + }, + "source_binding": { + "observed_object_id": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "ref": "refs/heads/fix/100-example", + "remote_url": "https://github.com/shaug/agent-scripts.git", + "repository": "shaug/agent-scripts" + }, + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] + } + }, + "change_contract": { + "acceptance_criteria": [ + "range_list(n) returns the same values without the intermediate comprehension" + ], + "goal": "Simplify range_list() to a direct list(range(n)) call.", + "non_goals": [ + "Changing range_list()'s public signature" + ], + "preserved_behaviors": [ + "range_list(n) still returns a list, not a generator" + ], + "sources": { + "named_documents": [], + "nearby_patterns": [], + "repository_instructions": [] + } + }, + "fix_cycle_budget": { + "max_fix_cycles": 3 + }, + "invocation_id": "review-fix-loop-100-update-pr-example", + "publication": { + "policy": "update_pr", + "pull_request": { + "base_ref": "refs/heads/main", + "base_sha": "f8943028b30c974048d55ced0d55368b1781d91b", + "expected_old_head_sha": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "head_ref": "refs/heads/fix/100-example", + "head_repository": "shaug/agent-scripts" + }, + "remote_iteration_grants": [] + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_execution": { + "mode": "fresh_subagent" + }, + "schema_version": "1.0", + "validation": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "scope": "focused" + }, + { + "command": "just test", + "name": "full repository gate", + "scope": "full" + } + ] +} diff --git a/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json b/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json new file mode 100644 index 0000000..c2534a3 --- /dev/null +++ b/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json @@ -0,0 +1,119 @@ +{ + "acceptance_reconciliation_required": true, + "base_revision_history": [ + { + "ref": "main", + "sha": "1f555dddc6e6eec43e9bd72d0eed6102423a5f41" + } + ], + "branch": "fix/100-race-example", + "budget": { + "consumed_cycles": 1, + "original_max_fix_cycles": 3, + "remaining_cycles": 2 + }, + "comparison_base": { + "final": { + "ref": "main", + "sha": "1f555dddc6e6eec43e9bd72d0eed6102423a5f41" + }, + "initial": { + "ref": "main", + "sha": "1f555dddc6e6eec43e9bd72d0eed6102423a5f41" + } + }, + "created_commits": [ + "f995245310784433fa433d41c49128327c53213d" + ], + "finding_dispositions": [ + { + "disposition": "selected", + "finding_id": "code-simplicity-001", + "fix_commit_sha": "f995245310784433fa433d41c49128327c53213d", + "rationale": "duplicated validation helper collapsed into the shared one" + } + ], + "head": { + "final": "f995245310784433fa433d41c49128327c53213d", + "initial": "d1002fc2fef33f70e0105266daf29486d5c98a28" + }, + "head_history": [ + "d1002fc2fef33f70e0105266daf29486d5c98a28", + "f995245310784433fa433d41c49128327c53213d" + ], + "invocation_id": "review-fix-loop-100-remote-race-example", + "operator_action": "Another clone published to refs/heads/fix/100-race-example before this invocation converged. Reconcile the two candidates manually; review-fix-loop will not merge, rebase, or force-update over the winning push.", + "preserved_failed_attempts": [], + "publication": { + "non_converged_exposure": false, + "policy": "update_pr", + "remote_head_after": "4e6d91372b61d731d7b678c9071ab13ae1b56d91", + "remote_head_before": "d1002fc2fef33f70e0105266daf29486d5c98a28", + "status": "failed" + }, + "pull_request": { + "number": 456, + "repository": "shaug/agent-scripts" + }, + "reason": "remote_advanced", + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "1f555dddc6e6eec43e9bd72d0eed6102423a5f41", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "code-simplicity-001", + "rationale": "duplicated validation helper" + } + ], + "head_sha": "d1002fc2fef33f70e0105266daf29486d5c98a28", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "clean", + "comparison_base_sha": "1f555dddc6e6eec43e9bd72d0eed6102423a5f41", + "finding_dispositions": [], + "head_sha": "f995245310784433fa433d41c49128327c53213d", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "ahead_by": 1, + "behind_by": 1, + "final_head": "f995245310784433fa433d41c49128327c53213d", + "initial_head": "d1002fc2fef33f70e0105266daf29486d5c98a28", + "status": "bound" + }, + "terminal_state": "blocked", + "unpushed_commits": [ + "f995245310784433fa433d41c49128327c53213d" + ], + "validation_summary": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ] +} diff --git a/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json b/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json new file mode 100644 index 0000000..69d0f00 --- /dev/null +++ b/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json @@ -0,0 +1,116 @@ +{ + "acceptance_reconciliation_required": true, + "base_revision_history": [ + { + "ref": "main", + "sha": "f8943028b30c974048d55ced0d55368b1781d91b" + } + ], + "branch": "fix/100-example", + "budget": { + "consumed_cycles": 1, + "original_max_fix_cycles": 3, + "remaining_cycles": 2 + }, + "comparison_base": { + "final": { + "ref": "main", + "sha": "f8943028b30c974048d55ced0d55368b1781d91b" + }, + "initial": { + "ref": "main", + "sha": "f8943028b30c974048d55ced0d55368b1781d91b" + } + }, + "created_commits": [ + "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7" + ], + "finding_dispositions": [ + { + "disposition": "selected", + "finding_id": "code-simplicity-001", + "fix_commit_sha": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "rationale": "list comprehension only rebuilds range(); list(range(n)) is equivalent and simpler" + } + ], + "head": { + "final": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "initial": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895" + }, + "head_history": [ + "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7" + ], + "invocation_id": "review-fix-loop-100-update-pr-example", + "operator_action": "None required; the converged fix is already published as the exact expected-old fast-forward update to pull request #123.", + "preserved_failed_attempts": [], + "publication": { + "non_converged_exposure": false, + "policy": "update_pr", + "remote_head_after": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "remote_head_before": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "status": "published" + }, + "pull_request": { + "number": 123, + "repository": "shaug/agent-scripts" + }, + "repository": { + "git_common_directory": "/work/agent-scripts/.git", + "identity": "shaug/agent-scripts" + }, + "review_records": [ + { + "aggregate_verdict": "changes_required", + "comparison_base_sha": "f8943028b30c974048d55ced0d55368b1781d91b", + "finding_dispositions": [ + { + "disposition": "accepted", + "finding_id": "code-simplicity-001", + "rationale": "list comprehension only rebuilds range(); list(range(n)) is equivalent and simpler" + } + ], + "head_sha": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 1, + "write_isolation": "enforced" + }, + { + "aggregate_verdict": "clean", + "comparison_base_sha": "f8943028b30c974048d55ced0d55368b1781d91b", + "finding_dispositions": [], + "head_sha": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "mutation_attempts": [], + "review_independence": "fresh_subagent", + "sequence": 2, + "write_isolation": "enforced" + } + ], + "schema_version": "1.0", + "source": { + "ahead_by": 0, + "behind_by": 0, + "final_head": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", + "initial_head": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", + "status": "bound" + }, + "terminal_state": "converged", + "unpushed_commits": [], + "validation_summary": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ] +} diff --git a/skills/review-fix-loop/references/invocation.schema.json b/skills/review-fix-loop/references/invocation.schema.json new file mode 100644 index 0000000..bb7d2f0 --- /dev/null +++ b/skills/review-fix-loop/references/invocation.schema.json @@ -0,0 +1,242 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-fix-loop/invocation.schema.json", + "title": "Review-fix-loop invocation", + "type": "object", + "additionalProperties": false, + "required": [ + "schema_version", + "invocation_id", + "repository", + "candidate", + "change_contract", + "review_execution", + "fix_cycle_budget", + "validation", + "publication" + ], + "properties": { + "schema_version": {"const": "1.0"}, + "invocation_id": {"type": "string", "minLength": 1}, + "repository": { + "type": "object", + "additionalProperties": false, + "required": ["identity", "git_common_directory"], + "properties": { + "identity": {"type": "string", "minLength": 1}, + "git_common_directory": {"type": "string", "minLength": 1} + } + }, + "candidate": { + "type": "object", + "additionalProperties": false, + "required": [ + "branch", + "head_sha", + "comparison_base", + "diff", + "worktree", + "all_changes_committed" + ], + "properties": { + "branch": {"type": "string", "minLength": 1}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base": { + "type": "object", + "additionalProperties": false, + "required": ["ref", "sha"], + "properties": { + "ref": {"type": "string", "minLength": 1}, + "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "diff": { + "type": "object", + "additionalProperties": false, + "required": ["format", "complete", "content"], + "properties": { + "format": {"enum": ["unified_diff"]}, + "complete": {"const": true}, + "content": {"type": "string", "minLength": 1} + } + }, + "worktree": { + "type": "object", + "additionalProperties": false, + "required": ["tracked", "staged", "unstaged", "untracked", "ignored"], + "properties": { + "tracked": {"type": "array", "items": {"type": "string"}}, + "staged": {"type": "array", "items": {"type": "string"}}, + "unstaged": {"type": "array", "items": {"type": "string"}}, + "untracked": {"type": "array", "items": {"type": "string"}}, + "ignored": {"type": "array", "items": {"type": "string"}} + } + }, + "all_changes_committed": {"const": true}, + "pull_request": { + "type": "object", + "additionalProperties": false, + "required": ["repository", "number"], + "properties": { + "repository": {"type": "string", "minLength": 1}, + "number": {"type": "integer", "minimum": 1} + } + }, + "source_binding": { + "type": "object", + "additionalProperties": false, + "required": ["repository", "remote_url", "ref", "observed_object_id"], + "properties": { + "repository": {"type": "string", "minLength": 1}, + "remote_url": {"type": "string", "minLength": 1}, + "ref": {"type": "string", "minLength": 1}, + "observed_object_id": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "source_unavailable_reason": {"type": "string", "minLength": 1} + } + }, + "change_contract": { + "type": "object", + "additionalProperties": false, + "required": [ + "goal", + "acceptance_criteria", + "non_goals", + "preserved_behaviors", + "sources" + ], + "properties": { + "goal": {"type": "string", "minLength": 1}, + "acceptance_criteria": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "minLength": 1} + }, + "non_goals": {"type": "array", "items": {"type": "string", "minLength": 1}}, + "preserved_behaviors": { + "type": "array", + "items": {"type": "string", "minLength": 1} + }, + "sources": { + "type": "object", + "additionalProperties": false, + "required": ["repository_instructions", "named_documents", "nearby_patterns"], + "properties": { + "repository_instructions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["label", "location"], + "properties": { + "label": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "summary": {"type": "string"} + } + } + }, + "named_documents": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["label", "location"], + "properties": { + "label": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "summary": {"type": "string"} + } + } + }, + "nearby_patterns": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["label", "location"], + "properties": { + "label": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "summary": {"type": "string"} + } + } + } + } + } + } + }, + "review_execution": { + "type": "object", + "additionalProperties": false, + "required": ["mode"], + "properties": { + "mode": {"enum": ["fresh_subagent", "in_agent_override"]}, + "override_authorization": {"type": "string", "minLength": 1} + } + }, + "fix_cycle_budget": { + "type": "object", + "additionalProperties": false, + "required": ["max_fix_cycles"], + "properties": { + "max_fix_cycles": {"type": "integer", "minimum": 1, "maximum": 10} + } + }, + "validation": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["name", "command", "scope"], + "properties": { + "name": {"type": "string", "minLength": 1}, + "command": {"type": "string", "minLength": 1}, + "scope": {"enum": ["focused", "full"]} + } + } + }, + "publication": { + "type": "object", + "additionalProperties": false, + "required": ["policy"], + "properties": { + "policy": {"enum": ["local_commit", "update_pr"]}, + "pull_request": { + "type": "object", + "additionalProperties": false, + "required": [ + "head_repository", + "head_ref", + "expected_old_head_sha", + "base_ref", + "base_sha" + ], + "properties": { + "head_repository": {"type": "string", "minLength": 1}, + "head_ref": {"type": "string", "minLength": 1}, + "expected_old_head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "base_ref": {"type": "string", "minLength": 1}, + "base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "remote_iteration_grants": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["mechanism_id", "kind", "repository", "ref", "origin_only_evidence"], + "properties": { + "mechanism_id": {"type": "string", "minLength": 1}, + "kind": {"type": "string", "minLength": 1}, + "repository": {"type": "string", "minLength": 1}, + "ref": {"type": "string", "minLength": 1}, + "origin_only_evidence": {"type": "string", "minLength": 1} + } + } + } + } + } + } +} diff --git a/skills/review-fix-loop/references/terminal-result.schema.json b/skills/review-fix-loop/references/terminal-result.schema.json new file mode 100644 index 0000000..f154dd2 --- /dev/null +++ b/skills/review-fix-loop/references/terminal-result.schema.json @@ -0,0 +1,257 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-fix-loop/terminal-result.schema.json", + "title": "Review-fix-loop terminal result", + "type": "object", + "additionalProperties": false, + "required": [ + "schema_version", + "invocation_id", + "terminal_state", + "budget", + "repository", + "branch", + "head", + "comparison_base", + "head_history", + "base_revision_history", + "review_records", + "validation_summary", + "finding_dispositions", + "created_commits", + "preserved_failed_attempts", + "source", + "unpushed_commits", + "publication", + "acceptance_reconciliation_required", + "operator_action" + ], + "properties": { + "schema_version": {"const": "1.0"}, + "invocation_id": {"type": "string", "minLength": 1}, + "terminal_state": {"enum": ["converged", "changes_remaining", "blocked"]}, + "reason": { + "enum": [ + "cycle_budget_exhausted", + "repeated_finding", + "oscillation", + "expanding_findings", + "repeated_failed_attempt", + "current_candidate_validation_failure", + "candidate_busy", + "candidate_integrity_failure", + "checkpoint_mismatch", + "missing_capability", + "missing_authority", + "insufficient_change_contract", + "reviewer_integrity_failure", + "validation_unavailable", + "base_drift", + "remote_advanced", + "publication_failed", + "scope_decision_required", + "operator_input_required" + ] + }, + "budget": { + "type": "object", + "additionalProperties": false, + "required": ["original_max_fix_cycles", "consumed_cycles", "remaining_cycles"], + "properties": { + "original_max_fix_cycles": {"type": "integer", "minimum": 1, "maximum": 10}, + "consumed_cycles": {"type": "integer", "minimum": 0}, + "remaining_cycles": {"type": "integer", "minimum": 0} + } + }, + "repository": { + "type": "object", + "additionalProperties": false, + "required": ["identity", "git_common_directory"], + "properties": { + "identity": {"type": "string", "minLength": 1}, + "git_common_directory": {"type": "string", "minLength": 1} + } + }, + "branch": {"type": "string", "minLength": 1}, + "pull_request": { + "type": "object", + "additionalProperties": false, + "required": ["repository", "number"], + "properties": { + "repository": {"type": "string", "minLength": 1}, + "number": {"type": "integer", "minimum": 1} + } + }, + "head": { + "type": "object", + "additionalProperties": false, + "required": ["initial", "final"], + "properties": { + "initial": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "final": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "comparison_base": { + "type": "object", + "additionalProperties": false, + "required": ["initial", "final"], + "properties": { + "initial": { + "type": "object", + "additionalProperties": false, + "required": ["ref", "sha"], + "properties": { + "ref": {"type": "string", "minLength": 1}, + "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "final": { + "type": "object", + "additionalProperties": false, + "required": ["ref", "sha"], + "properties": { + "ref": {"type": "string", "minLength": 1}, + "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + } + } + }, + "head_history": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + }, + "base_revision_history": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["ref", "sha"], + "properties": { + "ref": {"type": "string", "minLength": 1}, + "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + } + }, + "review_records": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "sequence", + "head_sha", + "comparison_base_sha", + "review_independence", + "write_isolation", + "aggregate_verdict", + "finding_dispositions", + "mutation_attempts" + ], + "properties": { + "sequence": {"type": "integer", "minimum": 1}, + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "review_independence": {"enum": ["fresh_subagent", "in_agent_override"]}, + "write_isolation": {"enum": ["enforced", "violated"]}, + "aggregate_verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "finding_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["finding_id", "disposition", "rationale"], + "properties": { + "finding_id": {"type": "string", "minLength": 1}, + "disposition": {"enum": ["accepted", "rejected", "deferred"]}, + "rationale": {"type": "string", "minLength": 1} + } + } + }, + "mutation_attempts": { + "type": "array", + "items": {"type": "string", "minLength": 1} + } + } + } + }, + "validation_summary": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["name", "command", "scope", "status"], + "properties": { + "name": {"type": "string", "minLength": 1}, + "command": {"type": "string", "minLength": 1}, + "scope": {"enum": ["focused", "full"]}, + "status": {"enum": ["passed", "failed", "unavailable"]}, + "result": {"type": "string", "minLength": 1}, + "reason": {"type": "string", "minLength": 1} + } + } + }, + "finding_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["finding_id", "disposition", "rationale"], + "properties": { + "finding_id": {"type": "string", "minLength": 1}, + "disposition": {"enum": ["selected", "declined"]}, + "rationale": {"type": "string", "minLength": 1}, + "fix_commit_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + } + }, + "created_commits": { + "type": "array", + "items": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + }, + "preserved_failed_attempts": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["attempt_ref", "reason"], + "properties": { + "attempt_ref": {"type": "string", "minLength": 1}, + "reason": {"type": "string", "minLength": 1} + } + } + }, + "source": { + "type": "object", + "additionalProperties": false, + "required": ["status"], + "properties": { + "status": {"enum": ["bound", "unavailable"]}, + "initial_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "final_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "ahead_by": {"type": "integer", "minimum": 0}, + "behind_by": {"type": "integer", "minimum": 0}, + "unavailable_reason": {"type": "string", "minLength": 1} + } + }, + "unpushed_commits": { + "type": "array", + "items": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + }, + "publication": { + "type": "object", + "additionalProperties": false, + "required": ["policy", "status", "non_converged_exposure"], + "properties": { + "policy": {"enum": ["local_commit", "update_pr"]}, + "status": {"enum": ["not_applicable", "withheld", "published", "failed"]}, + "remote_head_before": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "remote_head_after": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "non_converged_exposure": {"type": "boolean"} + } + }, + "acceptance_reconciliation_required": {"type": "boolean"}, + "operator_action": {"type": "string", "minLength": 1} + } +} diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py new file mode 100644 index 0000000..d21386e --- /dev/null +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -0,0 +1,622 @@ +"""Validate the review-fix-loop invocation, checkpoint, and terminal-result +contracts: valid examples for both `local_commit` and `update_pr`, invalid and +ambiguous inputs failing closed with actionable diagnostics, rejection of +zero-cycle and review-only invocations, checkpoint-reconstructed cycle +accounting, the per-terminal-state publication/retained-commit/operator-action +contract, and deterministic round-trip serialization. +""" + +from __future__ import annotations + +import copy +import importlib.util +import json +import unittest +from pathlib import Path + +SKILL_ROOT = Path(__file__).resolve().parents[2] +EXAMPLES = SKILL_ROOT / "references" / "examples" + +SPEC = importlib.util.spec_from_file_location( + "review_fix_loop_validator", SKILL_ROOT / "scripts" / "validate.py" +) +assert SPEC and SPEC.loader +VALIDATE = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(VALIDATE) + + +def load(name: str) -> dict: + return json.loads((EXAMPLES / name).read_text()) + + +class ValidExampleTests(unittest.TestCase): + """Acceptance: valid examples for both local_commit and update_pr pass + strict validation, across all three document kinds.""" + + def test_local_commit_invocation_valid(self): + self.assertEqual( + VALIDATE.validate_invocation(load("local-commit-invocation.json")), [] + ) + + def test_local_commit_checkpoint_valid(self): + self.assertEqual( + VALIDATE.validate_checkpoint(load("local-commit-checkpoint.json")), [] + ) + + def test_local_commit_terminal_converged_valid(self): + self.assertEqual( + VALIDATE.validate_terminal_result( + load("local-commit-terminal-converged.json") + ), + [], + ) + + def test_update_pr_invocation_valid(self): + self.assertEqual( + VALIDATE.validate_invocation(load("update-pr-invocation.json")), [] + ) + + def test_update_pr_checkpoint_valid(self): + self.assertEqual( + VALIDATE.validate_checkpoint(load("update-pr-checkpoint.json")), [] + ) + + def test_update_pr_terminal_converged_valid(self): + self.assertEqual( + VALIDATE.validate_terminal_result( + load("update-pr-terminal-converged.json") + ), + [], + ) + + def test_local_commit_changes_remaining_examples_valid(self): + self.assertEqual( + VALIDATE.validate_checkpoint( + load("local-commit-checkpoint-changes-remaining.json") + ), + [], + ) + self.assertEqual( + VALIDATE.validate_terminal_result( + load("local-commit-terminal-changes-remaining.json") + ), + [], + ) + + def test_update_pr_blocked_remote_advanced_example_valid(self): + self.assertEqual( + VALIDATE.validate_terminal_result( + load("update-pr-terminal-blocked-remote-advanced.json") + ), + [], + ) + + +class InvocationRejectionTests(unittest.TestCase): + """Invalid and ambiguous invocations fail closed with actionable diagnostics.""" + + def setUp(self): + self.local_commit = load("local-commit-invocation.json") + self.update_pr = load("update-pr-invocation.json") + + def test_zero_cycle_budget_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["fix_cycle_budget"]["max_fix_cycles"] = 0 + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue( + any("must be >= 1" in error for error in errors), + errors, + ) + + def test_negative_cycle_budget_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["fix_cycle_budget"]["max_fix_cycles"] = -1 + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue(any("must be >= 1" in error for error in errors), errors) + + def test_missing_fix_cycle_budget_rejected(self): + invocation = copy.deepcopy(self.local_commit) + del invocation["fix_cycle_budget"] + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue( + any( + "missing required property 'fix_cycle_budget'" in error + for error in errors + ), + errors, + ) + + def test_over_maximum_cycle_budget_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["fix_cycle_budget"]["max_fix_cycles"] = 11 + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue(any("must be <= 10" in error for error in errors), errors) + + def test_unsupported_review_only_top_level_field_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["mode"] = "review_only" + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue(any("unknown property" in error for error in errors), errors) + + def test_unsupported_review_execution_mode_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["review_execution"]["mode"] = "review_only" + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue(any("expected one of" in error for error in errors), errors) + + def test_in_agent_override_requires_authorization(self): + invocation = copy.deepcopy(self.local_commit) + invocation["review_execution"]["mode"] = "in_agent_override" + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.review_execution: in_agent_override requires override_authorization", + errors, + ) + + def test_fresh_subagent_rejects_override_authorization(self): + invocation = copy.deepcopy(self.local_commit) + invocation["review_execution"]["override_authorization"] = "operator note" + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.review_execution: fresh_subagent must not carry override_authorization", + errors, + ) + + def test_source_binding_and_unavailable_reason_are_mutually_exclusive(self): + invocation = copy.deepcopy(self.update_pr) + invocation["candidate"]["source_unavailable_reason"] = "also unavailable" + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue(any("mutually exclusive" in error for error in errors), errors) + + def test_source_binding_and_unavailable_reason_both_absent_rejected(self): + invocation = copy.deepcopy(self.update_pr) + del invocation["candidate"]["source_binding"] + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue( + any("requires source_binding or an explicit" in error for error in errors), + errors, + ) + + def test_update_pr_requires_pull_request_target(self): + invocation = copy.deepcopy(self.update_pr) + del invocation["publication"]["pull_request"] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn("$.publication: update_pr requires pull_request", errors) + + def test_update_pr_requires_source_binding(self): + invocation = copy.deepcopy(self.update_pr) + del invocation["candidate"]["source_binding"] + invocation["candidate"]["source_unavailable_reason"] = "no source recorded" + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.publication: update_pr requires candidate.source_binding", errors + ) + + def test_local_commit_rejects_pull_request_target(self): + invocation = copy.deepcopy(self.local_commit) + invocation["publication"]["pull_request"] = self.update_pr["publication"][ + "pull_request" + ] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.publication: local_commit must not carry a pull_request target", + errors, + ) + + def test_local_commit_rejects_remote_iteration_grants(self): + invocation = copy.deepcopy(self.local_commit) + invocation["publication"]["remote_iteration_grants"] = [ + { + "mechanism_id": "external-linter", + "kind": "hosted_check", + "repository": "shaug/agent-scripts", + "ref": "refs/heads/fix/96-example", + "origin_only_evidence": "the checker only reads the origin ref", + } + ] + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue( + any("cannot carry a grant" in error for error in errors), errors + ) + + def test_validation_requires_focused_scope(self): + invocation = copy.deepcopy(self.local_commit) + invocation["validation"] = [ + entry for entry in invocation["validation"] if entry["scope"] != "focused" + ] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn("$.validation: missing focused validation", errors) + + def test_validation_requires_full_scope(self): + invocation = copy.deepcopy(self.local_commit) + invocation["validation"] = [ + entry for entry in invocation["validation"] if entry["scope"] != "full" + ] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn("$.validation: missing full validation", errors) + + def test_missing_candidate_head_sha_rejected(self): + invocation = copy.deepcopy(self.local_commit) + del invocation["candidate"]["head_sha"] + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue( + any("missing required property 'head_sha'" in error for error in errors), + errors, + ) + + def test_malformed_head_sha_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["candidate"]["head_sha"] = "not-a-sha" + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue(any("does not match" in error for error in errors), errors) + + +class CheckpointBudgetReconstructionTests(unittest.TestCase): + """Cycle consumption and remaining budget reconstruct from checkpoint history.""" + + def test_single_committed_cycle(self): + checkpoint = load("local-commit-checkpoint.json") + self.assertEqual( + VALIDATE.reconstruct_cycle_accounting(checkpoint), + { + "original_max_fix_cycles": 3, + "consumed_cycles": 1, + "remaining_cycles": 2, + }, + ) + + def test_exhausted_budget_with_a_failed_attempt(self): + checkpoint = load("local-commit-checkpoint-changes-remaining.json") + self.assertEqual( + VALIDATE.reconstruct_cycle_accounting(checkpoint), + { + "original_max_fix_cycles": 3, + "consumed_cycles": 3, + "remaining_cycles": 0, + }, + ) + + def test_no_attempts_yet(self): + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["cycle_attempts"] = [] + self.assertEqual( + VALIDATE.reconstruct_cycle_accounting(checkpoint), + { + "original_max_fix_cycles": 3, + "consumed_cycles": 0, + "remaining_cycles": 3, + }, + ) + + +class CheckpointRejectionTests(unittest.TestCase): + def setUp(self): + self.checkpoint = load("local-commit-checkpoint.json") + + def test_attempts_cannot_exceed_original_budget(self): + checkpoint = copy.deepcopy(self.checkpoint) + for sequence in range(2, 5): + checkpoint["cycle_attempts"].append( + { + "sequence": sequence, + "started_from_head": checkpoint["current_head"], + "outcome": "failed", + } + ) + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertTrue( + any( + "consumed more cycles than original_cycle_budget" in error + for error in errors + ), + errors, + ) + + def test_head_history_must_start_at_initial_head(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["head_history"][0] = checkpoint["current_head"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn("$.head_history[0]: must equal initial_head", errors) + + def test_head_history_must_end_at_current_head(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["head_history"][-1] = checkpoint["initial_head"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn("$.head_history[-1]: must equal current_head", errors) + + def test_committed_attempt_requires_resulting_head(self): + checkpoint = copy.deepcopy(self.checkpoint) + del checkpoint["cycle_attempts"][0]["resulting_head"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertTrue( + any( + "committed outcome requires resulting_head" in error for error in errors + ), + errors, + ) + + def test_committed_count_must_match_head_history_advances(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["cycle_attempts"][0]["outcome"] = "failed" + del checkpoint["cycle_attempts"][0]["resulting_head"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertTrue( + any("does not match head_history advances" in error for error in errors), + errors, + ) + + def test_source_unavailable_requires_reason(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["source"] = {"status": "unavailable"} + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn( + "$.source: unavailable status requires unavailable_reason", errors + ) + + def test_source_bound_requires_last_verified_head(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["source"] = {"status": "bound"} + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn("$.source: bound status requires last_verified_head", errors) + + def test_base_revision_history_must_match_current_base(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["base_revision_history"][-1]["sha"] = checkpoint["current_head"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn( + "$.base_revision_history[-1]: must equal the current comparison_base", + errors, + ) + + +class TerminalResultContractTests(unittest.TestCase): + """Every terminal state has an explicit publication/retained-commit/ + operator-action contract, enforced across converged, changes_remaining, + and blocked results.""" + + def setUp(self): + self.converged_local = load("local-commit-terminal-converged.json") + self.converged_pr = load("update-pr-terminal-converged.json") + self.changes_remaining = load("local-commit-terminal-changes-remaining.json") + self.blocked = load("update-pr-terminal-blocked-remote-advanced.json") + + def test_converged_must_not_carry_a_reason(self): + result = copy.deepcopy(self.converged_local) + result["reason"] = "cycle_budget_exhausted" + errors = VALIDATE.validate_terminal_result(result) + self.assertIn("$.reason: converged must not carry a reason", errors) + + def test_changes_remaining_rejects_a_blocked_only_reason(self): + result = copy.deepcopy(self.changes_remaining) + result["reason"] = "remote_advanced" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("changes_remaining requires one of" in error for error in errors), + errors, + ) + + def test_changes_remaining_requires_a_reason(self): + result = copy.deepcopy(self.changes_remaining) + result.pop("reason", None) + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("changes_remaining requires one of" in error for error in errors), + errors, + ) + + def test_blocked_rejects_a_changes_remaining_only_reason(self): + result = copy.deepcopy(self.blocked) + result["reason"] = "oscillation" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("blocked requires one of" in error for error in errors), errors + ) + + def test_local_commit_publication_status_must_be_not_applicable(self): + result = copy.deepcopy(self.converged_local) + result["publication"]["status"] = "published" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("local_commit never publishes" in error for error in errors), errors + ) + + def test_converged_update_pr_requires_published_status(self): + result = copy.deepcopy(self.converged_pr) + result["publication"]["status"] = "withheld" + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.publication.status: a converged update_pr result must be published", + errors, + ) + + def test_changes_remaining_update_pr_requires_withheld_status(self): + result = copy.deepcopy(self.converged_pr) + result["terminal_state"] = "changes_remaining" + result["reason"] = "cycle_budget_exhausted" + # publication.status is still "published" from the converged fixture. + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.publication.status: changes_remaining must withhold publication", + errors, + ) + + def test_blocked_remote_advanced_requires_failed_status(self): + result = copy.deepcopy(self.blocked) + result["publication"]["status"] = "withheld" + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.publication.status: blocked/remote_advanced must be 'failed'", + errors, + ) + + def test_blocked_non_publication_reason_requires_withheld_status(self): + result = copy.deepcopy(self.blocked) + result["reason"] = "missing_capability" + result["publication"]["status"] = "failed" + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.publication.status: blocked/missing_capability must be 'withheld'", + errors, + ) + + def test_unpushed_commits_required_when_head_changed_and_not_published(self): + result = copy.deepcopy(self.changes_remaining) + result["unpushed_commits"] = [] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "no unpushed commits were reported for a non-published result" in error + for error in errors + ), + errors, + ) + + def test_unpushed_commits_must_be_empty_once_published(self): + result = copy.deepcopy(self.converged_pr) + result["unpushed_commits"] = [result["head"]["final"]] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.unpushed_commits: must be empty once publication.status is published", + errors, + ) + + def test_acceptance_reconciliation_required_when_head_changed(self): + result = copy.deepcopy(self.converged_local) + result["acceptance_reconciliation_required"] = False + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "must be true when head or comparison_base identity changed" in error + for error in errors + ), + errors, + ) + + def test_budget_arithmetic_must_be_consistent(self): + result = copy.deepcopy(self.converged_local) + result["budget"]["remaining_cycles"] = 999 + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "consumed_cycles + remaining_cycles must equal" in error + for error in errors + ), + errors, + ) + + def test_head_history_must_match_head_initial_and_final(self): + result = copy.deepcopy(self.converged_local) + result["head_history"][0] = result["head"]["final"] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn("$.head_history[0]: must equal head.initial", errors) + + def test_base_revision_history_must_match_comparison_base(self): + result = copy.deepcopy(self.converged_local) + result["base_revision_history"][0]["sha"] = result["head"]["final"] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.base_revision_history[0]: must equal comparison_base.initial", errors + ) + + def test_source_unavailable_requires_reason(self): + result = copy.deepcopy(self.converged_local) + result["source"] = {"status": "unavailable"} + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.source: unavailable status requires unavailable_reason", errors + ) + + def test_source_bound_requires_initial_and_final_head(self): + result = copy.deepcopy(self.converged_pr) + result["source"] = {"status": "bound", "ahead_by": 0, "behind_by": 0} + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.source: bound status requires initial_head and final_head", errors + ) + + +class CrossDocumentConsistencyTests(unittest.TestCase): + """A terminal result must match the checkpoint it derives from.""" + + def test_local_commit_terminal_matches_its_checkpoint(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = load("local-commit-terminal-converged.json") + self.assertEqual( + VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal), [] + ) + + def test_update_pr_terminal_matches_its_checkpoint(self): + checkpoint = load("update-pr-checkpoint.json") + terminal = load("update-pr-terminal-converged.json") + self.assertEqual( + VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal), [] + ) + + def test_mismatched_budget_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["budget"]["consumed_cycles"] = 2 + terminal["budget"]["remaining_cycles"] = 1 + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertTrue( + any( + "does not match checkpoint-reconstructed value" in error + for error in errors + ), + errors, + ) + + def test_mismatched_invocation_id_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["invocation_id"] = "some-other-invocation" + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn( + "$.invocation_id: does not match checkpoint invocation_id", errors + ) + + def test_mismatched_final_head_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["head"]["final"] = terminal["head"]["initial"] + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn("$.head.final: does not match checkpoint current_head", errors) + + +class DeterministicSerializationTests(unittest.TestCase): + """Contract serialization is deterministic and every example is a complete, + byte-stable snapshot.""" + + def test_every_example_is_already_canonical(self): + for path in sorted(EXAMPLES.glob("*.json")): + with self.subTest(example=path.name): + original_text = path.read_text() + document = json.loads(original_text) + self.assertEqual( + VALIDATE.canonical_json(document), + original_text, + f"{path.name} is not in canonical form; re-run the " + "canonicalization step", + ) + + def test_canonical_serialization_round_trips(self): + for path in sorted(EXAMPLES.glob("*.json")): + with self.subTest(example=path.name): + document = json.loads(path.read_text()) + serialized = VALIDATE.canonical_json(document) + reparsed = json.loads(serialized) + self.assertEqual(document, reparsed) + # Serializing twice must be byte-identical: no hidden ordering + # or float-formatting nondeterminism. + self.assertEqual(serialized, VALIDATE.canonical_json(reparsed)) + + def test_key_order_is_independent_of_input_order(self): + document = load("local-commit-invocation.json") + reordered = dict(reversed(list(document.items()))) + self.assertEqual( + VALIDATE.canonical_json(document), VALIDATE.canonical_json(reordered) + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py new file mode 100644 index 0000000..d30b1fe --- /dev/null +++ b/skills/review-fix-loop/scripts/validate.py @@ -0,0 +1,472 @@ +#!/usr/bin/env python3 +"""Validate review-fix-loop invocation, checkpoint, and terminal-result documents. + +This module intentionally has no third-party dependencies, matching the +convention used by the repository's review-suite contract +(`skills/review-code-change/references/review-suite/validate.py`): a skill +folder is the unit of distribution, so its validator must work standalone +wherever the skill is installed. +""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +from pathlib import Path +from typing import Any + +HERE = Path(__file__).resolve().parent +SCHEMA_DIR = HERE.parent / "references" + +SCHEMAS = { + "invocation": SCHEMA_DIR / "invocation.schema.json", + "checkpoint": SCHEMA_DIR / "checkpoint.schema.json", + "terminal-result": SCHEMA_DIR / "terminal-result.schema.json", +} + +CHANGES_REMAINING_REASONS = { + "cycle_budget_exhausted", + "repeated_finding", + "oscillation", + "expanding_findings", + "repeated_failed_attempt", + "current_candidate_validation_failure", +} + +BLOCKED_REASONS = { + "candidate_busy", + "candidate_integrity_failure", + "checkpoint_mismatch", + "missing_capability", + "missing_authority", + "insufficient_change_contract", + "reviewer_integrity_failure", + "validation_unavailable", + "base_drift", + "remote_advanced", + "publication_failed", + "scope_decision_required", + "operator_input_required", +} + +# Blocked reasons whose publication status is `failed` rather than `withheld` +# under `update_pr`: only these observed a publication attempt that did not +# land cleanly. +BLOCKED_REASONS_IMPLYING_PUBLICATION_FAILED = {"remote_advanced", "publication_failed"} + + +# --------------------------------------------------------------------------- +# Minimal JSON Schema subset (shared shape with the review-suite validator) +# --------------------------------------------------------------------------- + + +def _path(parent: str, key: object) -> str: + if isinstance(key, int): + return f"{parent}[{key}]" + return f"{parent}.{key}" if parent else str(key) + + +def _is_type(value: Any, expected: str) -> bool: + if expected == "object": + return isinstance(value, dict) + if expected == "array": + return isinstance(value, list) + if expected == "string": + return isinstance(value, str) + if expected == "boolean": + return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) + return False + + +def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[str]: + """Validate the JSON Schema subset used by this repository.""" + errors: list[str] = [] + expected_type = schema.get("type") + if expected_type and not _is_type(value, expected_type): + return [f"{at}: expected {expected_type}"] + + if "const" in schema: + const = schema["const"] + # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. + if value != const or isinstance(const, bool) != isinstance(value, bool): + errors.append(f"{at}: expected constant {const!r}") + if "enum" in schema and value not in schema["enum"]: + errors.append(f"{at}: expected one of {schema['enum']!r}") + if isinstance(value, str): + if len(value) < schema.get("minLength", 0): + errors.append(f"{at}: string is too short") + if pattern := schema.get("pattern"): + if re.fullmatch(pattern, value) is None: + errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") + if isinstance(value, list): + if len(value) < schema.get("minItems", 0): + errors.append(f"{at}: expected at least {schema['minItems']} item(s)") + if item_schema := schema.get("items"): + for index, item in enumerate(value): + errors.extend(validate_schema(item, item_schema, _path(at, index))) + if isinstance(value, dict): + properties = schema.get("properties", {}) + for key in schema.get("required", []): + if key not in value: + errors.append(f"{at}: missing required property {key!r}") + if schema.get("additionalProperties") is False: + for key in value.keys() - properties.keys(): + errors.append(f"{_path(at, key)}: unknown property") + for key, child in value.items(): + if key in properties: + errors.extend(validate_schema(child, properties[key], _path(at, key))) + return errors + + +def _load_schema(name: str) -> dict[str, Any]: + return json.loads(SCHEMAS[name].read_text()) + + +# --------------------------------------------------------------------------- +# Invocation +# --------------------------------------------------------------------------- + + +def validate_invocation(document: dict[str, Any]) -> list[str]: + errors = validate_schema(document, _load_schema("invocation")) + if errors: + return errors + + review_execution = document.get("review_execution", {}) + mode = review_execution.get("mode") + override = review_execution.get("override_authorization") + if mode == "in_agent_override" and not override: + errors.append( + "$.review_execution: in_agent_override requires override_authorization" + ) + if mode == "fresh_subagent" and override: + errors.append( + "$.review_execution: fresh_subagent must not carry override_authorization" + ) + + candidate = document.get("candidate", {}) + has_binding = "source_binding" in candidate + has_unavailable = "source_unavailable_reason" in candidate + if has_binding and has_unavailable: + errors.append( + "$.candidate: source_binding and source_unavailable_reason are " + "mutually exclusive" + ) + elif not has_binding and not has_unavailable: + errors.append( + "$.candidate: requires source_binding or an explicit " + "source_unavailable_reason" + ) + + publication = document.get("publication", {}) + policy = publication.get("policy") + grants = publication.get("remote_iteration_grants", []) + if policy == "update_pr": + if "pull_request" not in publication: + errors.append("$.publication: update_pr requires pull_request") + if not has_binding: + errors.append("$.publication: update_pr requires candidate.source_binding") + elif policy == "local_commit": + if "pull_request" in publication: + errors.append( + "$.publication: local_commit must not carry a pull_request target" + ) + if grants: + errors.append( + "$.publication.remote_iteration_grants: local_commit never " + "performs a remote write and cannot carry a grant" + ) + + scopes = {entry.get("scope") for entry in document.get("validation", [])} + for required_scope in ("focused", "full"): + if required_scope not in scopes: + errors.append(f"$.validation: missing {required_scope} validation") + + return errors + + +# --------------------------------------------------------------------------- +# Checkpoint +# --------------------------------------------------------------------------- + + +def reconstruct_cycle_accounting(checkpoint: dict[str, Any]) -> dict[str, int]: + """Reconstruct cycle consumption from checkpoint history alone. + + A cycle is reserved and consumed the moment a remediation attempt starts, + whatever its outcome (`committed`, `failed`, or `interrupted`). Consumed + and remaining counts are therefore never stored directly; they are always + derived from `cycle_attempts` so they cannot drift from the history that + produced them. + """ + original = checkpoint.get("original_cycle_budget", 0) + consumed = len(checkpoint.get("cycle_attempts", [])) + return { + "original_max_fix_cycles": original, + "consumed_cycles": consumed, + "remaining_cycles": original - consumed, + } + + +def validate_checkpoint(document: dict[str, Any]) -> list[str]: + errors = validate_schema(document, _load_schema("checkpoint")) + if errors: + return errors + + initial_head = document.get("initial_head") + current_head = document.get("current_head") + head_history = document.get("head_history", []) + if head_history and head_history[0] != initial_head: + errors.append("$.head_history[0]: must equal initial_head") + if head_history and head_history[-1] != current_head: + errors.append("$.head_history[-1]: must equal current_head") + + attempts = document.get("cycle_attempts", []) + budget = document.get("original_cycle_budget", 0) + if len(attempts) > budget: + errors.append( + "$.cycle_attempts: consumed more cycles than original_cycle_budget " + f"({len(attempts)} > {budget})" + ) + + committed_heads = [ + attempt.get("resulting_head") + for attempt in attempts + if attempt.get("outcome") == "committed" + ] + for index, attempt in enumerate(attempts): + if attempt.get("outcome") == "committed" and not attempt.get("resulting_head"): + errors.append( + f"$.cycle_attempts[{index}]: committed outcome requires resulting_head" + ) + if len(committed_heads) != max(len(head_history) - 1, 0): + errors.append( + "$.cycle_attempts: committed attempt count " + f"({len(committed_heads)}) does not match head_history advances " + f"({max(len(head_history) - 1, 0)})" + ) + elif committed_heads != head_history[1:]: + errors.append( + "$.cycle_attempts: committed resulting_head sequence does not " + "match head_history" + ) + + base_history = document.get("base_revision_history", []) + current_base = document.get("comparison_base", {}).get("sha") + if base_history and base_history[-1].get("sha") != current_base: + errors.append( + "$.base_revision_history[-1]: must equal the current comparison_base" + ) + + source = document.get("source", {}) + if source.get("status") == "unavailable" and not source.get("unavailable_reason"): + errors.append("$.source: unavailable status requires unavailable_reason") + if source.get("status") == "bound" and "last_verified_head" not in source: + errors.append("$.source: bound status requires last_verified_head") + + return errors + + +# --------------------------------------------------------------------------- +# Terminal result +# --------------------------------------------------------------------------- + + +def validate_terminal_result(document: dict[str, Any]) -> list[str]: + errors = validate_schema(document, _load_schema("terminal-result")) + if errors: + return errors + + state = document.get("terminal_state") + reason = document.get("reason") + + if state == "converged" and reason is not None: + errors.append("$.reason: converged must not carry a reason") + if state == "changes_remaining" and reason not in CHANGES_REMAINING_REASONS: + errors.append( + "$.reason: changes_remaining requires one of " + + ", ".join(sorted(CHANGES_REMAINING_REASONS)) + ) + if state == "blocked" and reason not in BLOCKED_REASONS: + errors.append( + "$.reason: blocked requires one of " + ", ".join(sorted(BLOCKED_REASONS)) + ) + + budget = document.get("budget", {}) + original = budget.get("original_max_fix_cycles", 0) + consumed = budget.get("consumed_cycles", 0) + remaining = budget.get("remaining_cycles", 0) + if consumed + remaining != original: + errors.append( + "$.budget: consumed_cycles + remaining_cycles must equal " + "original_max_fix_cycles" + ) + + publication = document.get("publication", {}) + policy = publication.get("policy") + status = publication.get("status") + if policy == "local_commit" and status != "not_applicable": + errors.append( + "$.publication.status: local_commit never publishes; must be not_applicable" + ) + elif policy == "update_pr": + if state == "converged" and status != "published": + errors.append( + "$.publication.status: a converged update_pr result must be published" + ) + elif state == "changes_remaining" and status != "withheld": + errors.append( + "$.publication.status: changes_remaining must withhold publication" + ) + elif state == "blocked": + expected = ( + "failed" + if reason in BLOCKED_REASONS_IMPLYING_PUBLICATION_FAILED + else "withheld" + ) + if status != expected: + errors.append( + f"$.publication.status: blocked/{reason} must be {expected!r}" + ) + + head = document.get("head", {}) + unpushed = document.get("unpushed_commits", []) + if ( + head.get("initial") != head.get("final") + and status != "published" + and not unpushed + ): + errors.append( + "$.unpushed_commits: candidate changed but no unpushed commits were " + "reported for a non-published result" + ) + if status == "published" and unpushed: + errors.append( + "$.unpushed_commits: must be empty once publication.status is published" + ) + + base = document.get("comparison_base", {}) + identity_changed = head.get("initial") != head.get("final") or base.get( + "initial" + ) != base.get("final") + if ( + identity_changed + and document.get("acceptance_reconciliation_required") is not True + ): + errors.append( + "$.acceptance_reconciliation_required: must be true when head or " + "comparison_base identity changed" + ) + + head_history = document.get("head_history", []) + if head_history and head_history[0] != head.get("initial"): + errors.append("$.head_history[0]: must equal head.initial") + if head_history and head_history[-1] != head.get("final"): + errors.append("$.head_history[-1]: must equal head.final") + + base_history = document.get("base_revision_history", []) + if base_history and base_history[0].get("sha") != base.get("initial", {}).get( + "sha" + ): + errors.append("$.base_revision_history[0]: must equal comparison_base.initial") + if base_history and base_history[-1].get("sha") != base.get("final", {}).get("sha"): + errors.append("$.base_revision_history[-1]: must equal comparison_base.final") + + source = document.get("source", {}) + if source.get("status") == "unavailable" and not source.get("unavailable_reason"): + errors.append("$.source: unavailable status requires unavailable_reason") + if source.get("status") == "bound" and ( + "initial_head" not in source or "final_head" not in source + ): + errors.append("$.source: bound status requires initial_head and final_head") + + return errors + + +def validate_terminal_against_checkpoint( + checkpoint: dict[str, Any], terminal_result: dict[str, Any] +) -> list[str]: + """Cross-check that a terminal result matches the checkpoint it derives from.""" + errors: list[str] = [] + accounting = reconstruct_cycle_accounting(checkpoint) + budget = terminal_result.get("budget", {}) + for field in ("original_max_fix_cycles", "consumed_cycles", "remaining_cycles"): + if budget.get(field) != accounting[field]: + errors.append( + f"$.budget.{field}: {budget.get(field)!r} does not match " + f"checkpoint-reconstructed value {accounting[field]!r}" + ) + if checkpoint.get("invocation_id") != terminal_result.get("invocation_id"): + errors.append("$.invocation_id: does not match checkpoint invocation_id") + if checkpoint.get("initial_head") != terminal_result.get("head", {}).get("initial"): + errors.append("$.head.initial: does not match checkpoint initial_head") + if checkpoint.get("current_head") != terminal_result.get("head", {}).get("final"): + errors.append("$.head.final: does not match checkpoint current_head") + return errors + + +# --------------------------------------------------------------------------- +# Determinism +# --------------------------------------------------------------------------- + + +def canonical_json(document: dict[str, Any]) -> str: + """Serialize a document deterministically: sorted keys, trailing newline.""" + return json.dumps(document, sort_keys=True, indent=2) + "\n" + + +# --------------------------------------------------------------------------- +# CLI +# --------------------------------------------------------------------------- + + +def validate_document(kind: str, document: dict[str, Any]) -> list[str]: + if kind == "invocation": + return validate_invocation(document) + if kind == "checkpoint": + return validate_checkpoint(document) + return validate_terminal_result(document) + + +def _load_json(path: Path) -> dict[str, Any] | None: + try: + document = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError) as error: + print(f"{path}: {error}", file=sys.stderr) + return None + if not isinstance(document, dict): + print(f"{path}: top-level JSON value must be an object", file=sys.stderr) + return None + return document + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument("kind", choices=["invocation", "checkpoint", "terminal-result"]) + parser.add_argument("document", type=Path) + args = parser.parse_args() + + document = _load_json(args.document) + if document is None: + return 2 + + errors = validate_document(args.kind, document) + if errors: + for error in errors: + print(error, file=sys.stderr) + return 1 + print(f"valid {args.kind}: {args.document}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 461db19fdd89e65afdf1c13fb870c5c427c00b67 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 10:44:53 -0700 Subject: [PATCH 02/22] docs: record the review-fix-loop contracts changelog entry 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). --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75c9b64..c35ddf5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,12 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-30 — Removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- feat(review-fix-loop): define invocation, checkpoint, and terminal-result + contracts, adding the skill-local schemas, a dependency-free validator, and 62 + unit tests covering valid, invalid, boundary, cross-document, and + round-trip/determinism cases for both `local_commit` and `update_pr` (#96) - feat(review-suite)!: remove the verification-sufficiency pass and its mandatory `verification_sufficiency_evidence` field from `review-correctness` and the shared review-result contract, advancing `schema_version` `1.3 → 1.4`; @@ -13,6 +17,7 @@ summary: Chronological history of repository and skill changes. unchanged, per #57's ablation matrix and #89's harder-case validation finding no demonstrated value for the removed pass plus a confirmed, twice-reproduced false-positive regression when it ran without the traversal pass (#93) + (`b91e12b063ea6d7ed49f152ee359f1f0eb326363`) - docs: simplify the review-fix-loop design (`2e7a8cd93af9f2c8cec36d6c393694f7849adedb`) From fc701b0bb29047af7b2ad24f25fb1db739718f89 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 11:04:26 -0700 Subject: [PATCH 03/22] fix(review-fix-loop): reject converged results with non-clean embedded 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 --- skills/review-fix-loop/references/CONTRACT.md | 19 ++++--- .../scripts/tests/test_validate.py | 55 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 55 +++++++++++++++++++ 3 files changed, 122 insertions(+), 7 deletions(-) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 7be1faf..8f99b9f 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -99,13 +99,18 @@ cannot claim a terminal state without the evidence that state requires: - `converged`: the aggregate review is `clean` for the final head and base, required validation passed, and the selected publication policy completed. - `reason` must be absent. Under `update_pr`, `publication.status` must be - `published` and `unpushed_commits` must be empty. Under `local_commit`, - `publication.status` must be `not_applicable` (local_commit never writes to - origin), and any created commits remain in `unpushed_commits` — that is the - expected, non-error shape of a converged `local_commit` result, and - `operator_action` must describe how the operator publishes them through their - own workflow. + `reason` must be absent. `scripts/validate.py` enforces this directly, + mirroring the review-suite validator's "clean cannot pair with failed + validation" rule: it rejects `converged` when any `validation_summary` entry + is not `passed`, when `review_records` has no entry bound to the exact final + head and comparison base, when that final-head record's `aggregate_verdict` is + not `clean`, or when its `write_isolation` is not `enforced`. Under + `update_pr`, `publication.status` must be `published` and `unpushed_commits` + must be empty. Under `local_commit`, `publication.status` must be + `not_applicable` (local_commit never writes to origin), and any created + commits remain in `unpushed_commits` — that is the expected, non-error shape + of a converged `local_commit` result, and `operator_action` must describe how + the operator publishes them through their own workflow. - `changes_remaining`: `reason` must be one of `cycle_budget_exhausted`, `repeated_finding`, `oscillation`, `expanding_findings`, `repeated_failed_attempt`, or `current_candidate_validation_failure`. diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index d21386e..a8db9c9 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -412,6 +412,61 @@ def test_blocked_rejects_a_changes_remaining_only_reason(self): any("blocked requires one of" in error for error in errors), errors ) + def test_converged_rejects_a_failed_required_validation_entry(self): + result = copy.deepcopy(self.converged_local) + result["validation_summary"][1]["status"] = "failed" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "converged cannot pair with 'failed' required validation" in error + for error in errors + ), + errors, + ) + + def test_converged_rejects_an_unavailable_required_validation_entry(self): + result = copy.deepcopy(self.converged_pr) + result["validation_summary"][0]["status"] = "unavailable" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "converged cannot pair with 'unavailable' required validation" in error + for error in errors + ), + errors, + ) + + def test_converged_rejects_a_non_clean_final_head_review_record(self): + result = copy.deepcopy(self.converged_local) + result["review_records"][-1]["aggregate_verdict"] = "changes_required" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("aggregate_verdict to be 'clean'" in error for error in errors), + errors, + ) + + def test_converged_rejects_violated_write_isolation_on_final_head_record(self): + result = copy.deepcopy(self.converged_pr) + result["review_records"][-1]["write_isolation"] = "violated" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("write_isolation to be 'enforced'" in error for error in errors), + errors, + ) + + def test_converged_rejects_missing_final_head_review_record(self): + result = copy.deepcopy(self.converged_local) + result["review_records"] = [] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "converged requires a review record bound to the exact final " + "head and comparison base" in error + for error in errors + ), + errors, + ) + def test_local_commit_publication_status_must_be_not_applicable(self): result = copy.deepcopy(self.converged_local) result["publication"]["status"] = "published" diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index d30b1fe..a290a45 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -389,6 +389,61 @@ def validate_terminal_result(document: dict[str, Any]) -> list[str]: ): errors.append("$.source: bound status requires initial_head and final_head") + if state == "converged": + errors.extend(_check_converged_requires_clean_evidence(document)) + + return errors + + +def _check_converged_requires_clean_evidence(document: dict[str, Any]) -> list[str]: + """Reject `converged` unless its own embedded evidence actually supports it. + + CONTRACT.md states that a `converged` result requires "the aggregate + review is clean for the final head and base, required validation passed, + and the selected publication policy completed" — mirroring review-suite's + rule that a `clean` verdict cannot pair with failed or unavailable + validation. Without this check, a document could claim `converged` while + its own `validation_summary` records a failure or its own `review_records` + show a non-clean or write-isolation-violated final review, and schema + validation alone would not catch it. + """ + errors: list[str] = [] + for index, validation in enumerate(document.get("validation_summary", [])): + if validation.get("status") != "passed": + errors.append( + f"$.validation_summary[{index}]: converged cannot pair with " + f"{validation.get('status')!r} required validation" + ) + + head = document.get("head", {}) + base = document.get("comparison_base", {}) + final_head = head.get("final") + final_base = base.get("final", {}).get("sha") + final_records = [ + record + for record in document.get("review_records", []) + if record.get("head_sha") == final_head + and record.get("comparison_base_sha") == final_base + ] + if not final_records: + errors.append( + "$.review_records: converged requires a review record bound to " + "the exact final head and comparison base" + ) + return errors + for record in final_records: + if record.get("aggregate_verdict") != "clean": + errors.append( + "$.review_records: converged requires the final-head review " + f"record's aggregate_verdict to be 'clean', not " + f"{record.get('aggregate_verdict')!r}" + ) + if record.get("write_isolation") != "enforced": + errors.append( + "$.review_records: converged requires the final-head review " + f"record's write_isolation to be 'enforced', not " + f"{record.get('write_isolation')!r}" + ) return errors From 1c9fcdc5f4ae5765414f4b25839c07122c3bb151 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 11:05:20 -0700 Subject: [PATCH 04/22] docs: record the converged-evidence fix in the changelog Backfill SHAs onto the two prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c35ddf5..15a595a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,17 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): reject converged results with non-clean embedded + evidence, closing a gap where `validate_terminal_result` never inspected + `review_records` or `validation_summary`, found by the initial + review-code-change pass on #96 +- docs: record the review-fix-loop contracts changelog entry + (`461db19fdd89e65afdf1c13fb870c5c427c00b67`) - feat(review-fix-loop): define invocation, checkpoint, and terminal-result contracts, adding the skill-local schemas, a dependency-free validator, and 62 unit tests covering valid, invalid, boundary, cross-document, and round-trip/determinism cases for both `local_commit` and `update_pr` (#96) + (`0689cda71833751249ebc5e65b766d231cf2c093`) - feat(review-suite)!: remove the verification-sufficiency pass and its mandatory `verification_sufficiency_evidence` field from `review-correctness` and the shared review-result contract, advancing `schema_version` `1.3 → 1.4`; From e18f1469cf6d5f5cff1d99045dd041cdc5e77b71 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 11:21:59 -0700 Subject: [PATCH 05/22] fix(review-fix-loop): check comparison_base in the checkpoint/terminal-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 --- skills/review-fix-loop/scripts/tests/test_validate.py | 10 ++++++++++ skills/review-fix-loop/scripts/validate.py | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index a8db9c9..9fe797d 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -637,6 +637,16 @@ def test_mismatched_final_head_is_rejected(self): errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) self.assertIn("$.head.final: does not match checkpoint current_head", errors) + def test_mismatched_final_comparison_base_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["comparison_base"]["final"]["sha"] = terminal["head"]["final"] + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn( + "$.comparison_base.final: does not match checkpoint comparison_base", + errors, + ) + class DeterministicSerializationTests(unittest.TestCase): """Contract serialization is deterministic and every example is a complete, diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index a290a45..feb6efc 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -466,6 +466,14 @@ def validate_terminal_against_checkpoint( errors.append("$.head.initial: does not match checkpoint initial_head") if checkpoint.get("current_head") != terminal_result.get("head", {}).get("final"): errors.append("$.head.final: does not match checkpoint current_head") + checkpoint_base_sha = checkpoint.get("comparison_base", {}).get("sha") + result_final_base_sha = ( + terminal_result.get("comparison_base", {}).get("final", {}).get("sha") + ) + if checkpoint_base_sha != result_final_base_sha: + errors.append( + "$.comparison_base.final: does not match checkpoint comparison_base" + ) return errors From 7d206acd32151405865c4ade4e9e7399ea739f57 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 11:22:42 -0700 Subject: [PATCH 06/22] docs: record the comparison_base cross-check fix in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15a595a..7fe5839 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,16 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): check comparison_base in the checkpoint/terminal-result + cross-check, closing a gap where `validate_terminal_against_checkpoint` + silently omitted the base-identity leg CONTRACT.md already documented it as + covering, found by the second review-code-change pass on #96 +- docs: record the converged-evidence fix in the changelog + (`1c9fcdc5f4ae5765414f4b25839c07122c3bb151`) - fix(review-fix-loop): reject converged results with non-clean embedded evidence, closing a gap where `validate_terminal_result` never inspected `review_records` or `validation_summary`, found by the initial - review-code-change pass on #96 + review-code-change pass on #96 (`fc701b0bb29047af7b2ad24f25fb1db739718f89`) - docs: record the review-fix-loop contracts changelog entry (`461db19fdd89e65afdf1c13fb870c5c427c00b67`) - feat(review-fix-loop): define invocation, checkpoint, and terminal-result From 3d240c6be6a33bf131aa7ccee2544c59a36614c1 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 11:39:20 -0700 Subject: [PATCH 07/22] fix(review-fix-loop): require non-empty scoped validation and full base 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 --- skills/review-fix-loop/references/CONTRACT.md | 9 ++-- .../scripts/tests/test_validate.py | 41 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 22 +++++++++- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 8f99b9f..681f586 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -101,10 +101,11 @@ cannot claim a terminal state without the evidence that state requires: required validation passed, and the selected publication policy completed. `reason` must be absent. `scripts/validate.py` enforces this directly, mirroring the review-suite validator's "clean cannot pair with failed - validation" rule: it rejects `converged` when any `validation_summary` entry - is not `passed`, when `review_records` has no entry bound to the exact final - head and comparison base, when that final-head record's `aggregate_verdict` is - not `clean`, or when its `write_isolation` is not `enforced`. Under + validation" rule: it rejects `converged` when `validation_summary` is empty, + is missing its required `focused` or `full` scope, or contains any entry that + is not `passed`; when `review_records` has no entry bound to the exact final + head and comparison base; when that final-head record's `aggregate_verdict` is + not `clean`; or when its `write_isolation` is not `enforced`. Under `update_pr`, `publication.status` must be `published` and `unpushed_commits` must be empty. Under `local_commit`, `publication.status` must be `not_applicable` (local_commit never writes to origin), and any created diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index 9fe797d..aeb0784 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -467,6 +467,36 @@ def test_converged_rejects_missing_final_head_review_record(self): errors, ) + def test_converged_rejects_empty_validation_summary(self): + result = copy.deepcopy(self.converged_local) + result["validation_summary"] = [] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "converged requires a passed focused validation entry" in error + for error in errors + ) + and any( + "converged requires a passed full validation entry" in error + for error in errors + ), + errors, + ) + + def test_converged_rejects_a_missing_validation_scope(self): + result = copy.deepcopy(self.converged_pr) + result["validation_summary"] = [ + entry for entry in result["validation_summary"] if entry["scope"] != "full" + ] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "converged requires a passed full validation entry" in error + for error in errors + ), + errors, + ) + def test_local_commit_publication_status_must_be_not_applicable(self): result = copy.deepcopy(self.converged_local) result["publication"]["status"] = "published" @@ -647,6 +677,17 @@ def test_mismatched_final_comparison_base_is_rejected(self): errors, ) + def test_mismatched_initial_comparison_base_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["comparison_base"]["initial"]["sha"] = terminal["head"]["final"] + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn( + "$.comparison_base.initial: does not match checkpoint " + "base_revision_history[0]", + errors, + ) + class DeterministicSerializationTests(unittest.TestCase): """Contract serialization is deterministic and every example is a complete, diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index feb6efc..c51ff21 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -408,12 +408,20 @@ def _check_converged_requires_clean_evidence(document: dict[str, Any]) -> list[s validation alone would not catch it. """ errors: list[str] = [] - for index, validation in enumerate(document.get("validation_summary", [])): + validation_summary = document.get("validation_summary", []) + for index, validation in enumerate(validation_summary): if validation.get("status") != "passed": errors.append( f"$.validation_summary[{index}]: converged cannot pair with " f"{validation.get('status')!r} required validation" ) + validated_scopes = {entry.get("scope") for entry in validation_summary} + for required_scope in ("focused", "full"): + if required_scope not in validated_scopes: + errors.append( + f"$.validation_summary: converged requires a passed {required_scope} " + "validation entry" + ) head = document.get("head", {}) base = document.get("comparison_base", {}) @@ -474,6 +482,18 @@ def validate_terminal_against_checkpoint( errors.append( "$.comparison_base.final: does not match checkpoint comparison_base" ) + checkpoint_base_history = checkpoint.get("base_revision_history", []) + checkpoint_initial_base_sha = ( + checkpoint_base_history[0].get("sha") if checkpoint_base_history else None + ) + result_initial_base_sha = ( + terminal_result.get("comparison_base", {}).get("initial", {}).get("sha") + ) + if checkpoint_initial_base_sha != result_initial_base_sha: + errors.append( + "$.comparison_base.initial: does not match checkpoint " + "base_revision_history[0]" + ) return errors From 86c7df20d44d0c398c34bee5b8272dca4b239cf7 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 11:39:41 -0700 Subject: [PATCH 08/22] docs: record the third review-fix-loop fix cycle in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fe5839..cab75f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,18 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): require non-empty scoped validation and full base + history match, closing two remaining gaps the third review-code-change pass on + #96 found: an empty or scope-incomplete `validation_summary` could still claim + `converged`, and the checkpoint/terminal-result cross-check compared only the + final comparison base, never the initial one +- docs: record the comparison_base cross-check fix in the changelog + (`7d206acd32151405865c4ade4e9e7399ea739f57`) - fix(review-fix-loop): check comparison_base in the checkpoint/terminal-result cross-check, closing a gap where `validate_terminal_against_checkpoint` silently omitted the base-identity leg CONTRACT.md already documented it as covering, found by the second review-code-change pass on #96 + (`e18f1469cf6d5f5cff1d99045dd041cdc5e77b71`) - docs: record the converged-evidence fix in the changelog (`1c9fcdc5f4ae5765414f4b25839c07122c3bb151`) - fix(review-fix-loop): reject converged results with non-clean embedded From c625bb87b7aefb2371b992b20e4ce07b12b1c270 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 12:08:38 -0700 Subject: [PATCH 09/22] fix(review-fix-loop): add validate_checkpoint_against_invocation cross-check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/SKILL.md | 5 +- skills/review-fix-loop/references/CONTRACT.md | 11 ++- .../scripts/tests/test_validate.py | 84 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 55 ++++++++++++ 4 files changed, 152 insertions(+), 3 deletions(-) diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 82cb0aa..caf9f31 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -62,7 +62,10 @@ diagnostic per violation to stderr and exits `1`. A malformed document exits The module also exposes importable functions for a caller that already holds parsed documents in memory: `validate_invocation`, `validate_checkpoint`, `validate_terminal_result`, `reconstruct_cycle_accounting` (derive consumed and -remaining fix cycles from a checkpoint's recorded attempts), and +remaining fix cycles from a checkpoint's recorded attempts), +`validate_checkpoint_against_invocation` (confirm a checkpoint's initial head, +original base, cycle budget, invocation ID, repository, and publication policy +match the invocation it derives from), and `validate_terminal_against_checkpoint` (confirm a terminal result's budget and head/base identities match the checkpoint it derives from). See [`references/CONTRACT.md`](references/CONTRACT.md) for the cross-field semantics diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 681f586..0cc7b1b 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -84,8 +84,15 @@ exceeds `original_cycle_budget`. number of `committed` attempts must equal `len(head_history) - 1`, and each committed attempt's `resulting_head` must equal the corresponding subsequent `head_history` entry in order. -- `base_revision_history[0].sha` must equal the invocation's original - comparison-base SHA and the last entry is the live current base. +- `base_revision_history[-1].sha` must equal the live current + `comparison_base.sha`. `validate_checkpoint` checks only this internal + consistency; nothing inside a checkpoint document alone can prove + `base_revision_history[0]` is the invocation's *real* original comparison + base, since a checkpoint has no other field to compare it against. Use + `validate_checkpoint_against_invocation(invocation, checkpoint)` for that: it + also cross-checks `initial_head`, `original_cycle_budget`, `invocation_id`, + `repository`, and `publication.policy` against the invocation, mirroring + `validate_terminal_against_checkpoint` one level up the same chain. - `review_records` bind every review pass to the exact head and base it reviewed. `write_isolation: violated` records an attempted or unattributed reviewer mutation; it does not by itself imply which terminal `blocked` reason diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index aeb0784..a1d91dc 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -620,6 +620,90 @@ def test_source_bound_requires_initial_and_final_head(self): ) +class CheckpointInvocationConsistencyTests(unittest.TestCase): + """A checkpoint must match the invocation it derives from. + + `validate_checkpoint` alone cannot prove `base_revision_history[0]` is the + invocation's real original comparison base: a checkpoint document has no + other field to compare it against, so a corrupted first entry would + otherwise validate as long as the rest of the checkpoint is internally + consistent. This is the invocation-side counterpart of + `validate_terminal_against_checkpoint`. + """ + + def test_local_commit_checkpoint_matches_its_invocation(self): + invocation = load("local-commit-invocation.json") + checkpoint = load("local-commit-checkpoint.json") + self.assertEqual( + VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint), [] + ) + + def test_update_pr_checkpoint_matches_its_invocation(self): + invocation = load("update-pr-invocation.json") + checkpoint = load("update-pr-checkpoint.json") + self.assertEqual( + VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint), [] + ) + + def test_mismatched_invocation_id_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["invocation_id"] = "some-other-invocation" + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn( + "$.invocation_id: does not match invocation invocation_id", errors + ) + + def test_mismatched_initial_head_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["initial_head"] = checkpoint["current_head"] + checkpoint["head_history"][0] = checkpoint["current_head"] + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn( + "$.initial_head: does not match invocation candidate.head_sha", errors + ) + + def test_mismatched_original_comparison_base_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["base_revision_history"][0]["sha"] = checkpoint["current_head"] + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn( + "$.base_revision_history[0]: does not match invocation " + "candidate.comparison_base", + errors, + ) + + def test_mismatched_original_cycle_budget_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["original_cycle_budget"] = 7 + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn( + "$.original_cycle_budget: does not match invocation " + "fix_cycle_budget.max_fix_cycles", + errors, + ) + + def test_mismatched_repository_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["repository"]["identity"] = "someone-else/other-repo" + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn("$.repository: does not match invocation repository", errors) + + def test_mismatched_publication_policy_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["publication"]["policy"] = "update_pr" + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn( + "$.publication.policy: does not match invocation publication.policy", + errors, + ) + + class CrossDocumentConsistencyTests(unittest.TestCase): """A terminal result must match the checkpoint it derives from.""" diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index c51ff21..15c945e 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -276,6 +276,61 @@ def validate_checkpoint(document: dict[str, Any]) -> list[str]: return errors +def validate_checkpoint_against_invocation( + invocation: dict[str, Any], checkpoint: dict[str, Any] +) -> list[str]: + """Cross-check that a checkpoint matches the invocation it derives from. + + `validate_checkpoint` only checks a checkpoint's own internal + consistency: `base_revision_history[0]` trivially equals itself, so + nothing inside the checkpoint alone can prove it actually started from + the invocation's real original comparison base. This mirrors + `validate_terminal_against_checkpoint`, one level up the same chain. + """ + errors: list[str] = [] + invocation_candidate = invocation.get("candidate", {}) + if invocation.get("invocation_id") != checkpoint.get("invocation_id"): + errors.append("$.invocation_id: does not match invocation invocation_id") + if invocation_candidate.get("head_sha") != checkpoint.get("initial_head"): + errors.append("$.initial_head: does not match invocation candidate.head_sha") + + invocation_base_sha = invocation_candidate.get("comparison_base", {}).get("sha") + checkpoint_base_history = checkpoint.get("base_revision_history", []) + checkpoint_initial_base_sha = ( + checkpoint_base_history[0].get("sha") if checkpoint_base_history else None + ) + if invocation_base_sha != checkpoint_initial_base_sha: + errors.append( + "$.base_revision_history[0]: does not match invocation " + "candidate.comparison_base" + ) + + invocation_budget = invocation.get("fix_cycle_budget", {}).get("max_fix_cycles") + if invocation_budget != checkpoint.get("original_cycle_budget"): + errors.append( + "$.original_cycle_budget: does not match invocation " + "fix_cycle_budget.max_fix_cycles" + ) + + invocation_repository = invocation.get("repository", {}) + checkpoint_repository = checkpoint.get("repository", {}) + if invocation_repository.get("identity") != checkpoint_repository.get( + "identity" + ) or invocation_repository.get("git_common_directory") != checkpoint_repository.get( + "git_common_directory" + ): + errors.append("$.repository: does not match invocation repository") + + invocation_policy = invocation.get("publication", {}).get("policy") + checkpoint_policy = checkpoint.get("publication", {}).get("policy") + if invocation_policy != checkpoint_policy: + errors.append( + "$.publication.policy: does not match invocation publication.policy" + ) + + return errors + + # --------------------------------------------------------------------------- # Terminal result # --------------------------------------------------------------------------- From 2f3addbdceefb0a952fa1fb035475d0a9d31ebfb Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 12:08:57 -0700 Subject: [PATCH 10/22] docs: record the coordinator-authorized fourth fix cycle in the changelog Backfill the SHA onto the prior review-fix-loop entry and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cab75f3..a91377b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,17 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): add validate_checkpoint_against_invocation cross-check, + symmetric to the existing `validate_terminal_against_checkpoint`, closing the + gap the fourth review-code-change pass on #96 found: nothing inside a + checkpoint document alone could prove `base_revision_history[0]` was the + invocation's real original comparison base - fix(review-fix-loop): require non-empty scoped validation and full base history match, closing two remaining gaps the third review-code-change pass on #96 found: an empty or scope-incomplete `validation_summary` could still claim `converged`, and the checkpoint/terminal-result cross-check compared only the final comparison base, never the initial one + (`86c7df20d44d0c398c34bee5b8272dca4b239cf7`) - docs: record the comparison_base cross-check fix in the changelog (`7d206acd32151405865c4ade4e9e7399ea739f57`) - fix(review-fix-loop): check comparison_base in the checkpoint/terminal-result From a6178b8da086fab80bd52596babb0208304163a1 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 12:32:54 -0700 Subject: [PATCH 11/22] fix(review-fix-loop): checkpoint identity cross-check and design-enumerated 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 --- skills/review-fix-loop/references/CONTRACT.md | 26 +++++-- .../references/checkpoint.schema.json | 30 ++++++++ ...l-commit-checkpoint-changes-remaining.json | 25 +++++++ .../examples/local-commit-checkpoint.json | 25 +++++++ .../examples/local-commit-invocation.json | 1 + .../examples/update-pr-checkpoint.json | 25 +++++++ .../examples/update-pr-invocation.json | 1 + .../references/invocation.schema.json | 4 +- .../scripts/tests/test_validate.py | 71 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 27 +++++++ 10 files changed, 228 insertions(+), 7 deletions(-) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 0cc7b1b..f454b73 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -16,10 +16,12 @@ acquire locks, manage worktrees, recover from interruption, or publish anything ## Contract ownership - `invocation.schema.json` owns the caller-supplied request shape: candidate - identity, change contract, review-execution mode, fix-cycle budget, validation - commands, and publication policy (including remote-iteration grants). + identity, a change contract including its allowed remediation scope, + review-execution mode, fix-cycle budget, validation commands, and publication + policy (including remote-iteration grants). - `checkpoint.schema.json` owns the durable, resumable invocation state: cycle - attempts, head and base history, and per-review-pass records. + attempts, head and base history, worktree state, validation outcomes, and + per-review-pass records. - `terminal-result.schema.json` owns the one candidate-bound result the loop returns to a caller. - This document owns the cross-field semantics. @@ -54,6 +56,11 @@ unknown property rather than being silently accepted or silently ignored. `staged`, `unstaged`, `untracked`, `ignored`). A candidate records exactly one of `source_binding` (a pushable, comparison-only source) or `source_unavailable_reason` — never both, never neither. +- `change_contract.allowed_remediation_scope` is required alongside the goal, + acceptance criteria, non-goals, and preserved behaviors: the design's change + contract explicitly enumerates "allowed remediation scope" as the boundary a + fix cycle's edits must stay inside, distinct from the ticket's own change + contract. - `publication.policy` is `local_commit` or `update_pr`. - `update_pr` requires `publication.pull_request` (exact head repository, fully qualified head ref, expected old head SHA, base ref, and base SHA) and @@ -97,6 +104,12 @@ exceeds `original_cycle_budget`. reviewed. `write_isolation: violated` records an attempted or unattributed reviewer mutation; it does not by itself imply which terminal `blocked` reason applies — that judgment belongs to the phase that observed it. +- `worktree` (`tracked`, `staged`, `unstaged`, `untracked`, `ignored`) and + `validation_outcomes` are required checkpoint content, matching the design's + durable-checkpoint field list. `validation_outcomes` follows the same + `status`/`result`/`reason` shape as every other validation collection in this + contract family: a `passed` or `failed` entry requires `result`, and an + `unavailable` entry requires `reason`. ## Terminal result @@ -148,9 +161,10 @@ ticket or PR acceptance merely by converging. `scripts/validate.py` also exposes `validate_terminal_against_checkpoint(checkpoint, terminal_result)` to confirm a -terminal result's budget and head/base identities are the ones actually recorded -by its checkpoint, so a result cannot report cycle accounting or history that -its own checkpoint does not support. +terminal result's budget, head/base identities, repository, branch, and +publication policy are the ones actually recorded by its checkpoint, so a result +cannot report cycle accounting, history, or a candidate identity that its own +checkpoint does not support. ## Determinism diff --git a/skills/review-fix-loop/references/checkpoint.schema.json b/skills/review-fix-loop/references/checkpoint.schema.json index ca28f07..9447b18 100644 --- a/skills/review-fix-loop/references/checkpoint.schema.json +++ b/skills/review-fix-loop/references/checkpoint.schema.json @@ -9,6 +9,7 @@ "invocation_id", "repository", "branch", + "worktree", "initial_head", "current_head", "comparison_base", @@ -18,6 +19,7 @@ "head_history", "base_revision_history", "review_records", + "validation_outcomes", "source", "current_phase", "expected_next_action" @@ -35,6 +37,18 @@ } }, "branch": {"type": "string", "minLength": 1}, + "worktree": { + "type": "object", + "additionalProperties": false, + "required": ["tracked", "staged", "unstaged", "untracked", "ignored"], + "properties": { + "tracked": {"type": "array", "items": {"type": "string"}}, + "staged": {"type": "array", "items": {"type": "string"}}, + "unstaged": {"type": "array", "items": {"type": "string"}}, + "untracked": {"type": "array", "items": {"type": "string"}}, + "ignored": {"type": "array", "items": {"type": "string"}} + } + }, "initial_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, "current_head": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, "comparison_base": { @@ -140,6 +154,22 @@ } } }, + "validation_outcomes": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["name", "command", "scope", "status"], + "properties": { + "name": {"type": "string", "minLength": 1}, + "command": {"type": "string", "minLength": 1}, + "scope": {"enum": ["focused", "full"]}, + "status": {"enum": ["passed", "failed", "unavailable"]}, + "result": {"type": "string", "minLength": 1}, + "reason": {"type": "string", "minLength": 1} + } + } + }, "source": { "type": "object", "additionalProperties": false, diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json index c5b8214..3fd59a6 100644 --- a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json @@ -104,5 +104,30 @@ "source": { "status": "unavailable", "unavailable_reason": "standalone invocation has no recorded pushable source" + }, + "validation_outcomes": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] } } diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json index a281dc1..9ae6771 100644 --- a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json @@ -68,5 +68,30 @@ "source": { "status": "unavailable", "unavailable_reason": "standalone invocation has no recorded pushable source" + }, + "validation_outcomes": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] } } diff --git a/skills/review-fix-loop/references/examples/local-commit-invocation.json b/skills/review-fix-loop/references/examples/local-commit-invocation.json index 8b4663f..060ec6e 100644 --- a/skills/review-fix-loop/references/examples/local-commit-invocation.json +++ b/skills/review-fix-loop/references/examples/local-commit-invocation.json @@ -27,6 +27,7 @@ "acceptance_criteria": [ "add(a, b) returns the sum of its arguments" ], + "allowed_remediation_scope": "example.py's add() function body only", "goal": "Implement the missing add() body.", "non_goals": [ "Refactoring unrelated arithmetic helpers" diff --git a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json index bc589ff..0d15d86 100644 --- a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json +++ b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json @@ -75,5 +75,30 @@ "behind_by": 0, "last_verified_head": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", "status": "bound" + }, + "validation_outcomes": [ + { + "command": "python3 -m unittest tests.test_example", + "name": "focused unit test", + "result": "OK", + "scope": "focused", + "status": "passed" + }, + { + "command": "just test", + "name": "full repository gate", + "result": "OK", + "scope": "full", + "status": "passed" + } + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] } } diff --git a/skills/review-fix-loop/references/examples/update-pr-invocation.json b/skills/review-fix-loop/references/examples/update-pr-invocation.json index 9fcc605..e22575b 100644 --- a/skills/review-fix-loop/references/examples/update-pr-invocation.json +++ b/skills/review-fix-loop/references/examples/update-pr-invocation.json @@ -36,6 +36,7 @@ "acceptance_criteria": [ "range_list(n) returns the same values without the intermediate comprehension" ], + "allowed_remediation_scope": "example.py's range_list() function body only", "goal": "Simplify range_list() to a direct list(range(n)) call.", "non_goals": [ "Changing range_list()'s public signature" diff --git a/skills/review-fix-loop/references/invocation.schema.json b/skills/review-fix-loop/references/invocation.schema.json index bb7d2f0..6b1afde 100644 --- a/skills/review-fix-loop/references/invocation.schema.json +++ b/skills/review-fix-loop/references/invocation.schema.json @@ -104,7 +104,8 @@ "acceptance_criteria", "non_goals", "preserved_behaviors", - "sources" + "sources", + "allowed_remediation_scope" ], "properties": { "goal": {"type": "string", "minLength": 1}, @@ -118,6 +119,7 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "allowed_remediation_scope": {"type": "string", "minLength": 1}, "sources": { "type": "object", "additionalProperties": false, diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index a1d91dc..a9e4a06 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -250,6 +250,18 @@ def test_malformed_head_sha_rejected(self): errors = VALIDATE.validate_invocation(invocation) self.assertTrue(any("does not match" in error for error in errors), errors) + def test_missing_allowed_remediation_scope_rejected(self): + invocation = copy.deepcopy(self.local_commit) + del invocation["change_contract"]["allowed_remediation_scope"] + errors = VALIDATE.validate_invocation(invocation) + self.assertTrue( + any( + "missing required property 'allowed_remediation_scope'" in error + for error in errors + ), + errors, + ) + class CheckpointBudgetReconstructionTests(unittest.TestCase): """Cycle consumption and remaining budget reconstruct from checkpoint history.""" @@ -368,6 +380,40 @@ def test_base_revision_history_must_match_current_base(self): errors, ) + def test_missing_worktree_rejected(self): + checkpoint = copy.deepcopy(self.checkpoint) + del checkpoint["worktree"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertTrue( + any("missing required property 'worktree'" in error for error in errors), + errors, + ) + + def test_missing_validation_outcomes_rejected(self): + checkpoint = copy.deepcopy(self.checkpoint) + del checkpoint["validation_outcomes"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertTrue( + any( + "missing required property 'validation_outcomes'" in error + for error in errors + ), + errors, + ) + + def test_passed_validation_outcome_requires_result(self): + checkpoint = copy.deepcopy(self.checkpoint) + del checkpoint["validation_outcomes"][0]["result"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn("$.validation_outcomes[0]: passed requires result", errors) + + def test_unavailable_validation_outcome_requires_reason(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["validation_outcomes"][0]["status"] = "unavailable" + del checkpoint["validation_outcomes"][0]["result"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn("$.validation_outcomes[0]: unavailable requires reason", errors) + class TerminalResultContractTests(unittest.TestCase): """Every terminal state has an explicit publication/retained-commit/ @@ -772,6 +818,31 @@ def test_mismatched_initial_comparison_base_is_rejected(self): errors, ) + def test_mismatched_repository_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["repository"]["identity"] = "someone-else/other-repo" + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn("$.repository: does not match checkpoint repository", errors) + + def test_mismatched_branch_is_rejected(self): + checkpoint = load("local-commit-checkpoint.json") + terminal = copy.deepcopy(load("local-commit-terminal-converged.json")) + terminal["branch"] = "some-other-branch" + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn("$.branch: does not match checkpoint branch", errors) + + def test_mismatched_publication_policy_is_rejected(self): + checkpoint = load("update-pr-checkpoint.json") + terminal = copy.deepcopy(load("update-pr-terminal-converged.json")) + terminal["publication"]["policy"] = "local_commit" + terminal["publication"]["status"] = "not_applicable" + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn( + "$.publication.policy: does not match checkpoint publication.policy", + errors, + ) + class DeterministicSerializationTests(unittest.TestCase): """Contract serialization is deterministic and every example is a complete, diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index 15c945e..ea40098 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -267,6 +267,15 @@ def validate_checkpoint(document: dict[str, Any]) -> list[str]: "$.base_revision_history[-1]: must equal the current comparison_base" ) + for index, outcome in enumerate(document.get("validation_outcomes", [])): + status = outcome.get("status") + if status in {"passed", "failed"} and not outcome.get("result"): + errors.append(f"$.validation_outcomes[{index}]: {status} requires result") + if status == "unavailable" and not outcome.get("reason"): + errors.append( + f"$.validation_outcomes[{index}]: unavailable requires reason" + ) + source = document.get("source", {}) if source.get("status") == "unavailable" and not source.get("unavailable_reason"): errors.append("$.source: unavailable status requires unavailable_reason") @@ -549,6 +558,24 @@ def validate_terminal_against_checkpoint( "$.comparison_base.initial: does not match checkpoint " "base_revision_history[0]" ) + + checkpoint_repository = checkpoint.get("repository", {}) + result_repository = terminal_result.get("repository", {}) + if checkpoint_repository.get("identity") != result_repository.get( + "identity" + ) or checkpoint_repository.get("git_common_directory") != result_repository.get( + "git_common_directory" + ): + errors.append("$.repository: does not match checkpoint repository") + if checkpoint.get("branch") != terminal_result.get("branch"): + errors.append("$.branch: does not match checkpoint branch") + checkpoint_policy = checkpoint.get("publication", {}).get("policy") + result_policy = terminal_result.get("publication", {}).get("policy") + if checkpoint_policy != result_policy: + errors.append( + "$.publication.policy: does not match checkpoint publication.policy" + ) + return errors From e71ea93ad98beec7b39cf6bb6c2a123743e820cf Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 12:33:17 -0700 Subject: [PATCH 12/22] docs: record the fifth review-fix-loop fix cycle in the changelog 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. --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a91377b..d5e50ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,16 +6,26 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): add the missing repository/branch/publication.policy + checkpoint cross-check, the design-enumerated `allowed_remediation_scope`, + `worktree`, and `validation_outcomes` schema fields, and correct a + misattributed changelog SHA, closing all three items the fifth + review-code-change pass on #96 found - fix(review-fix-loop): add validate_checkpoint_against_invocation cross-check, symmetric to the existing `validate_terminal_against_checkpoint`, closing the gap the fourth review-code-change pass on #96 found: nothing inside a checkpoint document alone could prove `base_revision_history[0]` was the invocation's real original comparison base + (`c625bb87b7aefb2371b992b20e4ce07b12b1c270`) +- docs: record the coordinator-authorized fourth fix cycle in the changelog + (`2f3addbdceefb0a952fa1fb035475d0a9d31ebfb`) - fix(review-fix-loop): require non-empty scoped validation and full base history match, closing two remaining gaps the third review-code-change pass on #96 found: an empty or scope-incomplete `validation_summary` could still claim `converged`, and the checkpoint/terminal-result cross-check compared only the final comparison base, never the initial one + (`3d240c6be6a33bf131aa7ccee2544c59a36614c1`) +- docs: record the third review-fix-loop fix cycle in the changelog (`86c7df20d44d0c398c34bee5b8272dca4b239cf7`) - docs: record the comparison_base cross-check fix in the changelog (`7d206acd32151405865c4ade4e9e7399ea739f57`) From 040ae824fae708efe46ca772f378c30378c9c695 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 13:17:47 -0700 Subject: [PATCH 13/22] fix(review-fix-loop): systematic cross-document identity closure and commit provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/references/CONTRACT.md | 79 +++++++++++++++++-- .../scripts/tests/test_validate.py | 44 +++++++++++ skills/review-fix-loop/scripts/validate.py | 36 +++++++++ 3 files changed, 151 insertions(+), 8 deletions(-) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index f454b73..9b5973f 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -74,6 +74,49 @@ unknown property rather than being silently accepted or silently ignored. - `validation` requires at least one `focused` and one `full` command; both scopes must be present. +## Cross-document identity invariants + +One invocation's `invocation_id`, `repository` (`identity` and +`git_common_directory`), candidate `branch`, original fix-cycle budget, +`publication.policy`, initial head, and initial comparison base never change for +the life of that invocation. This is the complete, closed set: every other field +either evolves over the invocation (`current_head`, the live `comparison_base`, +`cycle_attempts`, `review_records`, `validation_outcomes`, `current_phase`) or +is checkpoint/terminal-result-only bookkeeping with no invocation-side +counterpart to compare against. + +`validate_checkpoint_against_invocation(invocation, checkpoint)` and +`validate_terminal_against_checkpoint(checkpoint, terminal_result)` each check +this complete invariant set against their adjacent document, using each +document's own field names for it: + +| Invariant | invocation | checkpoint | terminal-result | +| ------------------------- | --------------------------------- | ------------------------------ | -------------------------------- | +| invocation ID | `invocation_id` | `invocation_id` | `invocation_id` | +| repository | `repository` | `repository` | `repository` | +| branch | `candidate.branch` | `branch` | `branch` | +| original fix-cycle budget | `fix_cycle_budget.max_fix_cycles` | `original_cycle_budget` | `budget.original_max_fix_cycles` | +| publication policy | `publication.policy` | `publication.policy` | `publication.policy` | +| initial head | `candidate.head_sha` | `initial_head` | `head.initial` | +| initial comparison base | `candidate.comparison_base.sha` | `base_revision_history[0].sha` | `comparison_base.initial.sha` | + +Because both adjacent pairs enforce the same complete set, invocation and +terminal-result identity agree transitively through the checkpoint without a +third direct `validate_terminal_against_invocation` function. An earlier version +of `validate_checkpoint_against_invocation` omitted `branch` even though +`validate_terminal_against_checkpoint` already checked it — that asymmetry is +the reason this table exists: any future field added to one side of this +invariant set must be added to both cross-document functions, not just the one a +particular fix cycle happened to be looking at. + +`validate_terminal_against_checkpoint` additionally reconstructs and compares +`consumed_cycles` and `remaining_cycles` (not just the original budget) via +`reconstruct_cycle_accounting`, and compares the *current* head and comparison +base (`head.final`, `comparison_base.final`) against the checkpoint's live +`current_head` and `comparison_base` — those are evolving values, not part of +the closed invariant set above, and are checked only between checkpoint and +terminal-result because only that pair shares a live notion of "current." + ## Checkpoint The checkpoint is the durable, resumable state for one invocation. It never @@ -96,10 +139,8 @@ exceeds `original_cycle_budget`. consistency; nothing inside a checkpoint document alone can prove `base_revision_history[0]` is the invocation's *real* original comparison base, since a checkpoint has no other field to compare it against. Use - `validate_checkpoint_against_invocation(invocation, checkpoint)` for that: it - also cross-checks `initial_head`, `original_cycle_budget`, `invocation_id`, - `repository`, and `publication.policy` against the invocation, mirroring - `validate_terminal_against_checkpoint` one level up the same chain. + `validate_checkpoint_against_invocation(invocation, checkpoint)` for the + complete cross-document invariant set described above. - `review_records` bind every review pass to the exact head and base it reviewed. `write_isolation: violated` records an attempted or unattributed reviewer mutation; it does not by itself imply which terminal `blocked` reason @@ -159,12 +200,34 @@ ticket or PR acceptance merely by converging. `budget.consumed_cycles + budget.remaining_cycles` must equal `budget.original_max_fix_cycles`. +### Commit provenance + +`created_commits` must equal `head_history[1:]` in order — one commit per head +advance, the terminal-result mirror of the equivalent +`cycle_attempts`-to-`head_history` rule already enforced for checkpoints. Every +`finding_dispositions` entry's `disposition` governs `fix_commit_sha`: +`selected` requires it, `declined` must not carry one, and any present +`fix_commit_sha` must appear in `created_commits` — a disposition cannot point +at a commit the result does not otherwise claim to have created. + +This deliberately does not constrain `unpushed_commits` to be a subset of +`created_commits` or `head_history`. Design's own local-ahead/behind reporting +is relative to the invocation's *recorded source*, and a resumed or +already-diverged invocation can legitimately have local commits ahead of that +source that predate `head.initial` and were never created by this invocation at +all — `head_history` only records head snapshots from `initial_head` onward, not +the individual pre-invocation commits that got a candidate to that initial head. +Treating `unpushed_commits` as bounded by this invocation's own commit history +would incorrectly reject that legitimate case; recovery and resume semantics are +this ticket's stated non-goals, so this boundary is recorded here rather than +guessed at. + `scripts/validate.py` also exposes `validate_terminal_against_checkpoint(checkpoint, terminal_result)` to confirm a -terminal result's budget, head/base identities, repository, branch, and -publication policy are the ones actually recorded by its checkpoint, so a result -cannot report cycle accounting, history, or a candidate identity that its own -checkpoint does not support. +terminal result's complete cross-document invariant set (see above), current +head, and current comparison base are the ones actually recorded by its +checkpoint, so a result cannot report cycle accounting, history, or a candidate +identity that its own checkpoint does not support. ## Determinism diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index a9e4a06..9d89ad4 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -641,6 +641,43 @@ def test_head_history_must_match_head_initial_and_final(self): errors = VALIDATE.validate_terminal_result(result) self.assertIn("$.head_history[0]: must equal head.initial", errors) + def test_created_commits_must_match_head_history_advances(self): + result = copy.deepcopy(self.converged_local) + result["created_commits"] = [result["head"]["final"], result["head"]["final"]] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.created_commits: must equal head_history[1:] — one commit per " + "head advance, in order", + errors, + ) + + def test_selected_finding_disposition_requires_fix_commit_sha(self): + result = copy.deepcopy(self.converged_local) + del result["finding_dispositions"][0]["fix_commit_sha"] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.finding_dispositions[0]: selected requires fix_commit_sha", errors + ) + + def test_declined_finding_disposition_forbids_fix_commit_sha(self): + result = copy.deepcopy(self.converged_local) + result["finding_dispositions"][0]["disposition"] = "declined" + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.finding_dispositions[0]: declined must not carry fix_commit_sha", + errors, + ) + + def test_fix_commit_sha_must_appear_in_created_commits(self): + result = copy.deepcopy(self.converged_local) + result["finding_dispositions"][0]["fix_commit_sha"] = result["head"]["initial"] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.finding_dispositions[0].fix_commit_sha: does not appear in " + "created_commits", + errors, + ) + def test_base_revision_history_must_match_comparison_base(self): result = copy.deepcopy(self.converged_local) result["base_revision_history"][0]["sha"] = result["head"]["final"] @@ -710,6 +747,13 @@ def test_mismatched_initial_head_is_rejected(self): "$.initial_head: does not match invocation candidate.head_sha", errors ) + def test_mismatched_branch_is_rejected(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["branch"] = "some-other-branch" + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn("$.branch: does not match invocation candidate.branch", errors) + def test_mismatched_original_comparison_base_is_rejected(self): invocation = load("local-commit-invocation.json") checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index ea40098..b7fa2b0 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -295,6 +295,16 @@ def validate_checkpoint_against_invocation( nothing inside the checkpoint alone can prove it actually started from the invocation's real original comparison base. This mirrors `validate_terminal_against_checkpoint`, one level up the same chain. + + The complete invariant identity set checked here (kept identical to what + `validate_terminal_against_checkpoint` checks one level up the same + chain, so transitive invocation-to-terminal-result identity holds without + a third direct function) is: `invocation_id`, `repository`, `branch`, + the original fix-cycle budget, `publication.policy`, the initial head, + and the initial comparison base. An earlier version of this function + omitted `branch` even though the sibling function already checked it; + see `references/CONTRACT.md` for why these two field sets must be kept + identical. """ errors: list[str] = [] invocation_candidate = invocation.get("candidate", {}) @@ -302,6 +312,8 @@ def validate_checkpoint_against_invocation( errors.append("$.invocation_id: does not match invocation invocation_id") if invocation_candidate.get("head_sha") != checkpoint.get("initial_head"): errors.append("$.initial_head: does not match invocation candidate.head_sha") + if invocation_candidate.get("branch") != checkpoint.get("branch"): + errors.append("$.branch: does not match invocation candidate.branch") invocation_base_sha = invocation_candidate.get("comparison_base", {}).get("sha") checkpoint_base_history = checkpoint.get("base_revision_history", []) @@ -437,6 +449,30 @@ def validate_terminal_result(document: dict[str, Any]) -> list[str]: if head_history and head_history[-1] != head.get("final"): errors.append("$.head_history[-1]: must equal head.final") + created_commits = document.get("created_commits", []) + if head_history and created_commits != head_history[1:]: + errors.append( + "$.created_commits: must equal head_history[1:] — one commit per " + "head advance, in order" + ) + + for index, disposition in enumerate(document.get("finding_dispositions", [])): + has_fix_commit = "fix_commit_sha" in disposition + if disposition.get("disposition") == "selected" and not has_fix_commit: + errors.append( + f"$.finding_dispositions[{index}]: selected requires fix_commit_sha" + ) + if disposition.get("disposition") == "declined" and has_fix_commit: + errors.append( + f"$.finding_dispositions[{index}]: declined must not carry " + "fix_commit_sha" + ) + if has_fix_commit and disposition["fix_commit_sha"] not in created_commits: + errors.append( + f"$.finding_dispositions[{index}].fix_commit_sha: does not " + "appear in created_commits" + ) + base_history = document.get("base_revision_history", []) if base_history and base_history[0].get("sha") != base.get("initial", {}).get( "sha" From 31789481f000567a4b69cb0a1d5ba77b8d8c4dba Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 13:18:08 -0700 Subject: [PATCH 14/22] docs: record the sixth review-fix-loop fix cycle in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5e50ec..ee72c72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,22 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): systematically close cross-document identity checks and + commit provenance, enumerating the complete invariant field set + (`invocation_id`, `repository`, `branch`, original fix-cycle budget, + `publication.policy`, initial head, initial comparison base) that must agree + across invocation, checkpoint, and terminal-result, closing the one remaining + gap (`branch` in `validate_checkpoint_against_invocation`) plus the + commit-provenance gap (`created_commits`/`fix_commit_sha` linkage) the sixth + review-code-change pass on #96 found - fix(review-fix-loop): add the missing repository/branch/publication.policy checkpoint cross-check, the design-enumerated `allowed_remediation_scope`, `worktree`, and `validation_outcomes` schema fields, and correct a misattributed changelog SHA, closing all three items the fifth review-code-change pass on #96 found + (`a6178b8da086fab80bd52596babb0208304163a1`) +- docs: record the fifth review-fix-loop fix cycle in the changelog + (`e71ea93ad98beec7b39cf6bb6c2a123743e820cf`) - fix(review-fix-loop): add validate_checkpoint_against_invocation cross-check, symmetric to the existing `validate_terminal_against_checkpoint`, closing the gap the fourth review-code-change pass on #96 found: nothing inside a From e7a955a321bafab1cd6f20b77758aa26670567bc Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 13:40:40 -0700 Subject: [PATCH 15/22] fix(review-fix-loop): complete checkpoint schema and pull-request identity closure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/references/CONTRACT.md | 22 +++++++ .../references/checkpoint.schema.json | 22 +++++++ ...l-commit-checkpoint-changes-remaining.json | 6 ++ .../examples/local-commit-checkpoint.json | 1 + .../examples/update-pr-checkpoint.json | 5 ++ .../scripts/tests/test_validate.py | 61 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 41 +++++++++++++ 7 files changed, 158 insertions(+) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 9b5973f..db91f63 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -117,6 +117,11 @@ base (`head.final`, `comparison_base.final`) against the checkpoint's live the closed invariant set above, and are checked only between checkpoint and terminal-result because only that pair shares a live notion of "current." +Both functions also check the optional pull-request identity +(`{repository, number}`) wherever both adjacent documents carry it — see the +"Checkpoint" section below for why this is checked like an invariant even though +the field itself is optional everywhere. + ## Checkpoint The checkpoint is the durable, resumable state for one invocation. It never @@ -151,6 +156,23 @@ exceeds `original_cycle_budget`. `status`/`result`/`reason` shape as every other validation collection in this contract family: a `passed` or `failed` entry requires `result`, and an `unavailable` entry requires `reason`. +- `preserved_failed_attempts` (required, possibly empty) is the checkpoint's own + record of "preserved failed-attempt artifacts," the other piece of required + durable-checkpoint content the design names alongside "committed fixes" + (`cycle_attempts`). Its count must equal the number of `cycle_attempts` + entries whose `outcome` is `failed` or `interrupted` — every unresolved + attempt has exactly one preserved artifact reference, mirroring the shape + `terminal-result.preserved_failed_attempts` already uses. +- `pull_request` (optional `{repository, number}`) is the checkpoint's own + record of the design's "optional pull-request identity," distinct from + `publication.pull_request`'s push-mechanics fields + (`head_repository`/`head_ref`/`base_ref`). It is part of the closed + cross-document invariant set: whenever both + `invocation.candidate.pull_request` and `checkpoint.pull_request`, or both + `checkpoint.pull_request` and `terminal_result.pull_request`, are present, + they must agree. Neither side is required to carry it — an invocation, + checkpoint, or terminal result may omit the identity wherever it is not yet + known. ## Terminal result diff --git a/skills/review-fix-loop/references/checkpoint.schema.json b/skills/review-fix-loop/references/checkpoint.schema.json index 9447b18..a05b08b 100644 --- a/skills/review-fix-loop/references/checkpoint.schema.json +++ b/skills/review-fix-loop/references/checkpoint.schema.json @@ -20,6 +20,7 @@ "base_revision_history", "review_records", "validation_outcomes", + "preserved_failed_attempts", "source", "current_phase", "expected_next_action" @@ -60,6 +61,15 @@ "sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} } }, + "pull_request": { + "type": "object", + "additionalProperties": false, + "required": ["repository", "number"], + "properties": { + "repository": {"type": "string", "minLength": 1}, + "number": {"type": "integer", "minimum": 1} + } + }, "publication": { "type": "object", "additionalProperties": false, @@ -170,6 +180,18 @@ } } }, + "preserved_failed_attempts": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["attempt_ref", "reason"], + "properties": { + "attempt_ref": {"type": "string", "minLength": 1}, + "reason": {"type": "string", "minLength": 1} + } + } + }, "source": { "type": "object", "additionalProperties": false, diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json index 3fd59a6..65015e4 100644 --- a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json @@ -43,6 +43,12 @@ "initial_head": "98676d2fa265321c8399de22ee6607c84a4f2e46", "invocation_id": "review-fix-loop-96-budget-exhausted-example", "original_cycle_budget": 3, + "preserved_failed_attempts": [ + { + "attempt_ref": "attempt/correctness-001-cycle-2", + "reason": "the second remediation attempt did not clear the finding and was discarded" + } + ], "publication": { "policy": "local_commit" }, diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json index 9ae6771..614133e 100644 --- a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json @@ -29,6 +29,7 @@ "initial_head": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", "invocation_id": "review-fix-loop-96-local-commit-example", "original_cycle_budget": 3, + "preserved_failed_attempts": [], "publication": { "policy": "local_commit" }, diff --git a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json index 0d15d86..fbcaa4c 100644 --- a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json +++ b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json @@ -29,6 +29,7 @@ "initial_head": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", "invocation_id": "review-fix-loop-100-update-pr-example", "original_cycle_budget": 3, + "preserved_failed_attempts": [], "publication": { "policy": "update_pr", "pull_request": { @@ -37,6 +38,10 @@ "head_repository": "shaug/agent-scripts" } }, + "pull_request": { + "number": 123, + "repository": "shaug/agent-scripts" + }, "repository": { "git_common_directory": "/work/agent-scripts/.git", "identity": "shaug/agent-scripts" diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index 9d89ad4..6f35be4 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -414,6 +414,26 @@ def test_unavailable_validation_outcome_requires_reason(self): errors = VALIDATE.validate_checkpoint(checkpoint) self.assertIn("$.validation_outcomes[0]: unavailable requires reason", errors) + def test_preserved_failed_attempts_must_match_unresolved_attempt_count(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["cycle_attempts"].append( + { + "sequence": 2, + "started_from_head": checkpoint["current_head"], + "outcome": "failed", + } + ) + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn( + "$.preserved_failed_attempts: count (0) does not match the number " + "of failed or interrupted cycle_attempts (1)", + errors, + ) + + def test_changes_remaining_checkpoint_preserved_attempts_are_valid(self): + checkpoint = load("local-commit-checkpoint-changes-remaining.json") + self.assertEqual(VALIDATE.validate_checkpoint(checkpoint), []) + class TerminalResultContractTests(unittest.TestCase): """Every terminal state has an explicit publication/retained-commit/ @@ -793,6 +813,33 @@ def test_mismatched_publication_policy_is_rejected(self): errors, ) + def test_pull_request_matches_between_invocation_and_checkpoint(self): + invocation = load("update-pr-invocation.json") + checkpoint = load("update-pr-checkpoint.json") + self.assertEqual( + VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint), [] + ) + + def test_mismatched_pull_request_is_rejected(self): + invocation = load("update-pr-invocation.json") + checkpoint = copy.deepcopy(load("update-pr-checkpoint.json")) + checkpoint["pull_request"]["number"] = 999 + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertIn( + "$.pull_request: does not match invocation candidate.pull_request", + errors, + ) + + def test_pull_request_absent_on_either_side_is_not_a_mismatch(self): + invocation = load("local-commit-invocation.json") + checkpoint = copy.deepcopy(load("local-commit-checkpoint.json")) + checkpoint["pull_request"] = {"number": 7, "repository": "some/other-repo"} + errors = VALIDATE.validate_checkpoint_against_invocation(invocation, checkpoint) + self.assertNotIn( + "$.pull_request: does not match invocation candidate.pull_request", + errors, + ) + class CrossDocumentConsistencyTests(unittest.TestCase): """A terminal result must match the checkpoint it derives from.""" @@ -887,6 +934,20 @@ def test_mismatched_publication_policy_is_rejected(self): errors, ) + def test_pull_request_matches_between_checkpoint_and_terminal(self): + checkpoint = load("update-pr-checkpoint.json") + terminal = load("update-pr-terminal-converged.json") + self.assertEqual( + VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal), [] + ) + + def test_mismatched_pull_request_between_checkpoint_and_terminal_is_rejected(self): + checkpoint = load("update-pr-checkpoint.json") + terminal = copy.deepcopy(load("update-pr-terminal-converged.json")) + terminal["pull_request"]["number"] = 999 + errors = VALIDATE.validate_terminal_against_checkpoint(checkpoint, terminal) + self.assertIn("$.pull_request: does not match checkpoint pull_request", errors) + class DeterministicSerializationTests(unittest.TestCase): """Contract serialization is deterministic and every example is a complete, diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index b7fa2b0..5800c20 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -276,6 +276,17 @@ def validate_checkpoint(document: dict[str, Any]) -> list[str]: f"$.validation_outcomes[{index}]: unavailable requires reason" ) + unresolved_attempts = [ + attempt for attempt in attempts if attempt.get("outcome") != "committed" + ] + preserved = document.get("preserved_failed_attempts", []) + if len(preserved) != len(unresolved_attempts): + errors.append( + "$.preserved_failed_attempts: count " + f"({len(preserved)}) does not match the number of failed or " + f"interrupted cycle_attempts ({len(unresolved_attempts)})" + ) + source = document.get("source", {}) if source.get("status") == "unavailable" and not source.get("unavailable_reason"): errors.append("$.source: unavailable status requires unavailable_reason") @@ -285,6 +296,25 @@ def validate_checkpoint(document: dict[str, Any]) -> list[str]: return errors +def _pull_request_identity_mismatch( + left: dict[str, Any] | None, right: dict[str, Any] | None +) -> bool: + """Whether two optional `{repository, number}` pull-request identities disagree. + + A pull request's identity does not change during one invocation, so + whichever documents carry it (`invocation.candidate.pull_request`, + `checkpoint.pull_request`, `terminal_result.pull_request`) must agree + whenever more than one of them is present. Neither side is required to + carry it at all: a caller may omit the optional identity anywhere it + is not yet known. + """ + if not left or not right: + return False + return left.get("repository") != right.get("repository") or left.get( + "number" + ) != right.get("number") + + def validate_checkpoint_against_invocation( invocation: dict[str, Any], checkpoint: dict[str, Any] ) -> list[str]: @@ -349,6 +379,13 @@ def validate_checkpoint_against_invocation( "$.publication.policy: does not match invocation publication.policy" ) + if _pull_request_identity_mismatch( + invocation_candidate.get("pull_request"), checkpoint.get("pull_request") + ): + errors.append( + "$.pull_request: does not match invocation candidate.pull_request" + ) + return errors @@ -611,6 +648,10 @@ def validate_terminal_against_checkpoint( errors.append( "$.publication.policy: does not match checkpoint publication.policy" ) + if _pull_request_identity_mismatch( + checkpoint.get("pull_request"), terminal_result.get("pull_request") + ): + errors.append("$.pull_request: does not match checkpoint pull_request") return errors From f6d1adccea4ee1e6571c911b10053aa4c27e00ba Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 13:42:46 -0700 Subject: [PATCH 16/22] docs: record the seventh review-fix-loop fix cycle in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee72c72..2fccac6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): complete the checkpoint schema against the design's + durable-checkpoint field list (`preserved_failed_attempts`, `pull_request`) + and extend the optional pull-request identity cross-check to both + cross-document functions, closing the gap the seventh review-code-change pass + on #96 found +- docs: record the sixth review-fix-loop fix cycle in the changelog + (`31789481f000567a4b69cb0a1d5ba77b8d8c4dba`) - fix(review-fix-loop): systematically close cross-document identity checks and commit provenance, enumerating the complete invariant field set (`invocation_id`, `repository`, `branch`, original fix-cycle budget, @@ -14,6 +21,7 @@ summary: Chronological history of repository and skill changes. gap (`branch` in `validate_checkpoint_against_invocation`) plus the commit-provenance gap (`created_commits`/`fix_commit_sha` linkage) the sixth review-code-change pass on #96 found + (`040ae824fae708efe46ca772f378c30378c9c695`) - fix(review-fix-loop): add the missing repository/branch/publication.policy checkpoint cross-check, the design-enumerated `allowed_remediation_scope`, `worktree`, and `validation_outcomes` schema fields, and correct a From adf2ef5062038836d750b01e19f1549373ce1aad Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 14:07:11 -0700 Subject: [PATCH 17/22] fix(review-fix-loop): complete terminal-result schema and bound-source ahead/behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/references/CONTRACT.md | 15 +++++ ...cal-commit-terminal-changes-remaining.json | 15 ++++- .../local-commit-terminal-converged.json | 13 +++- ...e-pr-terminal-blocked-remote-advanced.json | 13 +++- .../update-pr-terminal-converged.json | 13 +++- .../references/terminal-result.schema.json | 20 +++++++ .../scripts/tests/test_validate.py | 59 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 16 +++-- 8 files changed, 154 insertions(+), 10 deletions(-) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index db91f63..044ebc7 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -173,9 +173,24 @@ exceeds `original_cycle_budget`. they must agree. Neither side is required to carry it — an invocation, checkpoint, or terminal result may omit the identity wherever it is not yet known. +- `source.status: bound` requires `last_verified_head`, `ahead_by`, and + `behind_by` — the design's "local ahead/behind state" is not satisfiable by a + bound source that reports a head but omits the counts, so the validator + requires all three together rather than only the head. ## Terminal result +`worktree` (the same `tracked`/`staged`/`unstaged`/`untracked`/`ignored` shape +used elsewhere in this contract family), `resume_status` +(`not_resumed`/`resumed`/`resume_unavailable`), and +`unresolved_or_deferred_findings` (a possibly-empty array of free-text +descriptions) are required terminal-result content, matching the exact field +list `design/review-fix-loop.md`'s Terminal result contract section enumerates +alongside repository/branch/pull-request identity and +invocation-ID/budget/resume-status. `source.status: bound` requires `ahead_by` +and `behind_by` in addition to `initial_head`/`final_head`, for the same reason +as the checkpoint's equivalent rule above. + Every terminal state has an explicit publication, retained-commit, and operator-action contract. The validator enforces the combination so a result cannot claim a terminal state without the evidence that state requires: diff --git a/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json index 4af4976..6f7d527 100644 --- a/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json +++ b/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json @@ -61,6 +61,7 @@ "git_common_directory": "/work/agent-scripts/.git", "identity": "shaug/agent-scripts" }, + "resume_status": "not_resumed", "review_records": [ { "aggregate_verdict": "changes_required", @@ -121,6 +122,9 @@ "8fbc9b399aa14299d694e4beca34dc371baa7e58", "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af" ], + "unresolved_or_deferred_findings": [ + "correctness-001: off-by-one boundary defect survived three fix cycles and remains unresolved" + ], "validation_summary": [ { "command": "python3 -m unittest tests.test_example", @@ -136,5 +140,14 @@ "scope": "full", "status": "passed" } - ] + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] + } } diff --git a/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json b/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json index 4aaae4d..5efee4e 100644 --- a/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json +++ b/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json @@ -53,6 +53,7 @@ "git_common_directory": "/work/agent-scripts/.git", "identity": "shaug/agent-scripts" }, + "resume_status": "not_resumed", "review_records": [ { "aggregate_verdict": "changes_required", @@ -90,6 +91,7 @@ "unpushed_commits": [ "b13de54b94608d3918bacdc14b4836332d8e0630" ], + "unresolved_or_deferred_findings": [], "validation_summary": [ { "command": "python3 -m unittest tests.test_example", @@ -105,5 +107,14 @@ "scope": "full", "status": "passed" } - ] + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] + } } diff --git a/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json b/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json index c2534a3..d13b658 100644 --- a/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json +++ b/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json @@ -60,6 +60,7 @@ "git_common_directory": "/work/agent-scripts/.git", "identity": "shaug/agent-scripts" }, + "resume_status": "not_resumed", "review_records": [ { "aggregate_verdict": "changes_required", @@ -100,6 +101,7 @@ "unpushed_commits": [ "f995245310784433fa433d41c49128327c53213d" ], + "unresolved_or_deferred_findings": [], "validation_summary": [ { "command": "python3 -m unittest tests.test_example", @@ -115,5 +117,14 @@ "scope": "full", "status": "passed" } - ] + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] + } } diff --git a/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json b/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json index 69d0f00..0d49a8a 100644 --- a/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json +++ b/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json @@ -59,6 +59,7 @@ "git_common_directory": "/work/agent-scripts/.git", "identity": "shaug/agent-scripts" }, + "resume_status": "not_resumed", "review_records": [ { "aggregate_verdict": "changes_required", @@ -97,6 +98,7 @@ }, "terminal_state": "converged", "unpushed_commits": [], + "unresolved_or_deferred_findings": [], "validation_summary": [ { "command": "python3 -m unittest tests.test_example", @@ -112,5 +114,14 @@ "scope": "full", "status": "passed" } - ] + ], + "worktree": { + "ignored": [], + "staged": [], + "tracked": [ + "example.py" + ], + "unstaged": [], + "untracked": [] + } } diff --git a/skills/review-fix-loop/references/terminal-result.schema.json b/skills/review-fix-loop/references/terminal-result.schema.json index f154dd2..2498d70 100644 --- a/skills/review-fix-loop/references/terminal-result.schema.json +++ b/skills/review-fix-loop/references/terminal-result.schema.json @@ -9,8 +9,10 @@ "invocation_id", "terminal_state", "budget", + "resume_status", "repository", "branch", + "worktree", "head", "comparison_base", "head_history", @@ -24,6 +26,7 @@ "unpushed_commits", "publication", "acceptance_reconciliation_required", + "unresolved_or_deferred_findings", "operator_action" ], "properties": { @@ -63,6 +66,7 @@ "remaining_cycles": {"type": "integer", "minimum": 0} } }, + "resume_status": {"enum": ["not_resumed", "resumed", "resume_unavailable"]}, "repository": { "type": "object", "additionalProperties": false, @@ -73,6 +77,18 @@ } }, "branch": {"type": "string", "minLength": 1}, + "worktree": { + "type": "object", + "additionalProperties": false, + "required": ["tracked", "staged", "unstaged", "untracked", "ignored"], + "properties": { + "tracked": {"type": "array", "items": {"type": "string"}}, + "staged": {"type": "array", "items": {"type": "string"}}, + "unstaged": {"type": "array", "items": {"type": "string"}}, + "untracked": {"type": "array", "items": {"type": "string"}}, + "ignored": {"type": "array", "items": {"type": "string"}} + } + }, "pull_request": { "type": "object", "additionalProperties": false, @@ -252,6 +268,10 @@ } }, "acceptance_reconciliation_required": {"type": "boolean"}, + "unresolved_or_deferred_findings": { + "type": "array", + "items": {"type": "string", "minLength": 1} + }, "operator_action": {"type": "string", "minLength": 1} } } diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index 6f35be4..be61fd8 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -371,6 +371,16 @@ def test_source_bound_requires_last_verified_head(self): errors = VALIDATE.validate_checkpoint(checkpoint) self.assertIn("$.source: bound status requires last_verified_head", errors) + def test_source_bound_requires_ahead_by_and_behind_by(self): + checkpoint = copy.deepcopy(self.checkpoint) + checkpoint["source"] = { + "status": "bound", + "last_verified_head": checkpoint["current_head"], + } + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertIn("$.source: bound status requires ahead_by", errors) + self.assertIn("$.source: bound status requires behind_by", errors) + def test_base_revision_history_must_match_current_base(self): checkpoint = copy.deepcopy(self.checkpoint) checkpoint["base_revision_history"][-1]["sha"] = checkpoint["current_head"] @@ -722,6 +732,55 @@ def test_source_bound_requires_initial_and_final_head(self): "$.source: bound status requires initial_head and final_head", errors ) + def test_source_bound_requires_ahead_by_and_behind_by(self): + result = copy.deepcopy(self.converged_pr) + result["source"] = { + "status": "bound", + "initial_head": result["head"]["initial"], + "final_head": result["head"]["final"], + } + errors = VALIDATE.validate_terminal_result(result) + self.assertIn("$.source: bound status requires ahead_by", errors) + self.assertIn("$.source: bound status requires behind_by", errors) + + def test_missing_worktree_rejected(self): + result = copy.deepcopy(self.converged_local) + del result["worktree"] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any("missing required property 'worktree'" in error for error in errors), + errors, + ) + + def test_missing_resume_status_rejected(self): + result = copy.deepcopy(self.converged_local) + del result["resume_status"] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "missing required property 'resume_status'" in error for error in errors + ), + errors, + ) + + def test_missing_unresolved_or_deferred_findings_rejected(self): + result = copy.deepcopy(self.converged_local) + del result["unresolved_or_deferred_findings"] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "missing required property 'unresolved_or_deferred_findings'" in error + for error in errors + ), + errors, + ) + + def test_invalid_resume_status_rejected(self): + result = copy.deepcopy(self.converged_local) + result["resume_status"] = "sort_of" + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue(any("expected one of" in error for error in errors), errors) + class CheckpointInvocationConsistencyTests(unittest.TestCase): """A checkpoint must match the invocation it derives from. diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index 5800c20..d933640 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -290,8 +290,10 @@ def validate_checkpoint(document: dict[str, Any]) -> list[str]: source = document.get("source", {}) if source.get("status") == "unavailable" and not source.get("unavailable_reason"): errors.append("$.source: unavailable status requires unavailable_reason") - if source.get("status") == "bound" and "last_verified_head" not in source: - errors.append("$.source: bound status requires last_verified_head") + if source.get("status") == "bound": + for field in ("last_verified_head", "ahead_by", "behind_by"): + if field not in source: + errors.append(f"$.source: bound status requires {field}") return errors @@ -521,10 +523,12 @@ def validate_terminal_result(document: dict[str, Any]) -> list[str]: source = document.get("source", {}) if source.get("status") == "unavailable" and not source.get("unavailable_reason"): errors.append("$.source: unavailable status requires unavailable_reason") - if source.get("status") == "bound" and ( - "initial_head" not in source or "final_head" not in source - ): - errors.append("$.source: bound status requires initial_head and final_head") + if source.get("status") == "bound": + if "initial_head" not in source or "final_head" not in source: + errors.append("$.source: bound status requires initial_head and final_head") + for field in ("ahead_by", "behind_by"): + if field not in source: + errors.append(f"$.source: bound status requires {field}") if state == "converged": errors.extend(_check_converged_requires_clean_evidence(document)) From 65a256a32b3087d064efb5e4a24725cfb1762467 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 14:07:33 -0700 Subject: [PATCH 18/22] docs: record the eighth review-fix-loop fix cycle in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fccac6..bc6d9a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,18 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): complete the terminal-result schema against the design's + Terminal result contract field list (`worktree`, `resume_status`, + `unresolved_or_deferred_findings`) and require `ahead_by`/`behind_by` + alongside the head fields whenever a source is `bound`, closing the gaps the + eighth review-code-change pass on #96 found - fix(review-fix-loop): complete the checkpoint schema against the design's durable-checkpoint field list (`preserved_failed_attempts`, `pull_request`) and extend the optional pull-request identity cross-check to both cross-document functions, closing the gap the seventh review-code-change pass - on #96 found + on #96 found (`e7a955a321bafab1cd6f20b77758aa26670567bc`) +- docs: record the seventh review-fix-loop fix cycle in the changelog + (`f6d1adccea4ee1e6571c911b10053aa4c27e00ba`) - docs: record the sixth review-fix-loop fix cycle in the changelog (`31789481f000567a4b69cb0a1d5ba77b8d8c4dba`) - fix(review-fix-loop): systematically close cross-document identity checks and From 4daa2a67a38be3baa7741380eb689581ce31a1db Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 14:30:11 -0700 Subject: [PATCH 19/22] fix(review-fix-loop): reviewer identity field and invocation worktree cleanliness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/references/CONTRACT.md | 11 +++ .../references/checkpoint.schema.json | 2 + ...l-commit-checkpoint-changes-remaining.json | 3 + .../examples/local-commit-checkpoint.json | 2 + ...cal-commit-terminal-changes-remaining.json | 3 + .../local-commit-terminal-converged.json | 2 + .../examples/update-pr-checkpoint.json | 2 + ...e-pr-terminal-blocked-remote-advanced.json | 2 + .../update-pr-terminal-converged.json | 2 + .../references/terminal-result.schema.json | 2 + .../scripts/tests/test_validate.py | 90 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 8 ++ 12 files changed, 129 insertions(+) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 044ebc7..54db0d7 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -56,6 +56,11 @@ unknown property rather than being silently accepted or silently ignored. `staged`, `unstaged`, `untracked`, `ignored`). A candidate records exactly one of `source_binding` (a pushable, comparison-only source) or `source_unavailable_reason` — never both, never neither. +- `candidate.worktree.staged`, `.unstaged`, and `.untracked` must each be empty: + the design requires "every invocation requires a dedicated globally clean + worktree" and blocks startup on dirty state. `ignored` is deliberately left + unconstrained — ignored files (build output, local environment files) do not + represent an uncommitted change and are not part of what "clean" means here. - `change_contract.allowed_remediation_scope` is required alongside the goal, acceptance criteria, non-goals, and preserved behaviors: the design's change contract explicitly enumerates "allowed remediation scope" as the boundary a @@ -150,6 +155,12 @@ exceeds `original_cycle_budget`. reviewed. `write_isolation: violated` records an attempted or unattributed reviewer mutation; it does not by itself imply which terminal `blocked` reason applies — that judgment belongs to the phase that observed it. + `reviewer_identity` (required, non-empty) is the design's "reviewer + identities" content: a per-pass identifier distinct from the + invocation-invariant `review_independence` enum, so a fresh-subagent + reviewer's identity can actually differ from one head's review pass to the + next, matching the design's validation-strategy requirement for "different + reviewer identities per head." - `worktree` (`tracked`, `staged`, `unstaged`, `untracked`, `ignored`) and `validation_outcomes` are required checkpoint content, matching the design's durable-checkpoint field list. `validation_outcomes` follows the same diff --git a/skills/review-fix-loop/references/checkpoint.schema.json b/skills/review-fix-loop/references/checkpoint.schema.json index a05b08b..6234aa0 100644 --- a/skills/review-fix-loop/references/checkpoint.schema.json +++ b/skills/review-fix-loop/references/checkpoint.schema.json @@ -132,6 +132,7 @@ "head_sha", "comparison_base_sha", "review_independence", + "reviewer_identity", "write_isolation", "aggregate_verdict", "finding_dispositions", @@ -142,6 +143,7 @@ "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, "review_independence": {"enum": ["fresh_subagent", "in_agent_override"]}, + "reviewer_identity": {"type": "string", "minLength": 1}, "write_isolation": {"enum": ["enforced", "violated"]}, "aggregate_verdict": {"enum": ["clean", "changes_required", "blocked"]}, "finding_dispositions": { diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json index 65015e4..a85a92d 100644 --- a/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint-changes-remaining.json @@ -70,6 +70,7 @@ "head_sha": "98676d2fa265321c8399de22ee6607c84a4f2e46", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -86,6 +87,7 @@ "head_sha": "8fbc9b399aa14299d694e4beca34dc371baa7e58", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" }, @@ -102,6 +104,7 @@ "head_sha": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-3", "sequence": 3, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json index 614133e..e63388c 100644 --- a/skills/review-fix-loop/references/examples/local-commit-checkpoint.json +++ b/skills/review-fix-loop/references/examples/local-commit-checkpoint.json @@ -51,6 +51,7 @@ "head_sha": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -61,6 +62,7 @@ "head_sha": "b13de54b94608d3918bacdc14b4836332d8e0630", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json b/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json index 6f7d527..0923774 100644 --- a/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json +++ b/skills/review-fix-loop/references/examples/local-commit-terminal-changes-remaining.json @@ -76,6 +76,7 @@ "head_sha": "98676d2fa265321c8399de22ee6607c84a4f2e46", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -92,6 +93,7 @@ "head_sha": "8fbc9b399aa14299d694e4beca34dc371baa7e58", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" }, @@ -108,6 +110,7 @@ "head_sha": "fb4a0f78cac42d90e26e025f7bf4b1233f3c49af", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-3", "sequence": 3, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json b/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json index 5efee4e..c1dc847 100644 --- a/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json +++ b/skills/review-fix-loop/references/examples/local-commit-terminal-converged.json @@ -68,6 +68,7 @@ "head_sha": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -78,6 +79,7 @@ "head_sha": "b13de54b94608d3918bacdc14b4836332d8e0630", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json index fbcaa4c..0de5069 100644 --- a/skills/review-fix-loop/references/examples/update-pr-checkpoint.json +++ b/skills/review-fix-loop/references/examples/update-pr-checkpoint.json @@ -60,6 +60,7 @@ "head_sha": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -70,6 +71,7 @@ "head_sha": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json b/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json index d13b658..739b9b6 100644 --- a/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json +++ b/skills/review-fix-loop/references/examples/update-pr-terminal-blocked-remote-advanced.json @@ -75,6 +75,7 @@ "head_sha": "d1002fc2fef33f70e0105266daf29486d5c98a28", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -85,6 +86,7 @@ "head_sha": "f995245310784433fa433d41c49128327c53213d", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json b/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json index 0d49a8a..81d0598 100644 --- a/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json +++ b/skills/review-fix-loop/references/examples/update-pr-terminal-converged.json @@ -74,6 +74,7 @@ "head_sha": "d2ecc3a15ad743b8361f465097dfe1dfbb39f895", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-1", "sequence": 1, "write_isolation": "enforced" }, @@ -84,6 +85,7 @@ "head_sha": "1d72d0fb75af38c8096e6dc0f4488eb00bbc98b7", "mutation_attempts": [], "review_independence": "fresh_subagent", + "reviewer_identity": "fresh-subagent-review-2", "sequence": 2, "write_isolation": "enforced" } diff --git a/skills/review-fix-loop/references/terminal-result.schema.json b/skills/review-fix-loop/references/terminal-result.schema.json index 2498d70..af43781 100644 --- a/skills/review-fix-loop/references/terminal-result.schema.json +++ b/skills/review-fix-loop/references/terminal-result.schema.json @@ -160,6 +160,7 @@ "head_sha", "comparison_base_sha", "review_independence", + "reviewer_identity", "write_isolation", "aggregate_verdict", "finding_dispositions", @@ -170,6 +171,7 @@ "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, "review_independence": {"enum": ["fresh_subagent", "in_agent_override"]}, + "reviewer_identity": {"type": "string", "minLength": 1}, "write_isolation": {"enum": ["enforced", "violated"]}, "aggregate_verdict": {"enum": ["clean", "changes_required", "blocked"]}, "finding_dispositions": { diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index be61fd8..8e116d4 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -250,6 +250,42 @@ def test_malformed_head_sha_rejected(self): errors = VALIDATE.validate_invocation(invocation) self.assertTrue(any("does not match" in error for error in errors), errors) + def test_staged_worktree_state_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["candidate"]["worktree"]["staged"] = ["example.py"] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.candidate.worktree.staged: must be empty; every invocation " + "requires a dedicated globally clean worktree", + errors, + ) + + def test_unstaged_worktree_state_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["candidate"]["worktree"]["unstaged"] = ["example.py"] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.candidate.worktree.unstaged: must be empty; every invocation " + "requires a dedicated globally clean worktree", + errors, + ) + + def test_untracked_worktree_state_rejected(self): + invocation = copy.deepcopy(self.local_commit) + invocation["candidate"]["worktree"]["untracked"] = ["stray-file.txt"] + errors = VALIDATE.validate_invocation(invocation) + self.assertIn( + "$.candidate.worktree.untracked: must be empty; every invocation " + "requires a dedicated globally clean worktree", + errors, + ) + + def test_ignored_worktree_state_is_not_constrained(self): + invocation = copy.deepcopy(self.local_commit) + invocation["candidate"]["worktree"]["ignored"] = [".env"] + errors = VALIDATE.validate_invocation(invocation) + self.assertEqual(errors, []) + def test_missing_allowed_remediation_scope_rejected(self): invocation = copy.deepcopy(self.local_commit) del invocation["change_contract"]["allowed_remediation_scope"] @@ -305,6 +341,33 @@ class CheckpointRejectionTests(unittest.TestCase): def setUp(self): self.checkpoint = load("local-commit-checkpoint.json") + def test_missing_reviewer_identity_rejected(self): + checkpoint = copy.deepcopy(self.checkpoint) + del checkpoint["review_records"][0]["reviewer_identity"] + errors = VALIDATE.validate_checkpoint(checkpoint) + self.assertTrue( + any( + "missing required property 'reviewer_identity'" in error + for error in errors + ), + errors, + ) + + def test_review_records_distinguish_passes_by_reviewer_identity(self): + # design/review-fix-loop.md's validation strategy requires "default + # fresh reviewer selection and different reviewer identities per + # head" — each fresh-subagent review pass gets its own reviewer + # identity, and the checkpoint records that identity per pass. + checkpoint = load("local-commit-checkpoint.json") + identities = [ + record["reviewer_identity"] for record in checkpoint["review_records"] + ] + self.assertEqual(len(identities), 2) + self.assertEqual( + len(set(identities)), 2, "each pass must have its own identity" + ) + self.assertEqual(VALIDATE.validate_checkpoint(checkpoint), []) + def test_attempts_cannot_exceed_original_budget(self): checkpoint = copy.deepcopy(self.checkpoint) for sequence in range(2, 5): @@ -456,6 +519,33 @@ def setUp(self): self.changes_remaining = load("local-commit-terminal-changes-remaining.json") self.blocked = load("update-pr-terminal-blocked-remote-advanced.json") + def test_missing_reviewer_identity_rejected(self): + result = copy.deepcopy(self.converged_local) + del result["review_records"][0]["reviewer_identity"] + errors = VALIDATE.validate_terminal_result(result) + self.assertTrue( + any( + "missing required property 'reviewer_identity'" in error + for error in errors + ), + errors, + ) + + def test_review_records_distinguish_passes_by_reviewer_identity(self): + # design/review-fix-loop.md's validation strategy requires "default + # fresh reviewer selection and different reviewer identities per + # head" — each fresh-subagent review pass gets its own reviewer + # identity, and the terminal result records that identity per pass. + result = load("local-commit-terminal-converged.json") + identities = [ + record["reviewer_identity"] for record in result["review_records"] + ] + self.assertEqual(len(identities), 2) + self.assertEqual( + len(set(identities)), 2, "each pass must have its own identity" + ) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + def test_converged_must_not_carry_a_reason(self): result = copy.deepcopy(self.converged_local) result["reason"] = "cycle_budget_exhausted" diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index d933640..4a33544 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -154,6 +154,14 @@ def validate_invocation(document: dict[str, Any]) -> list[str]: ) candidate = document.get("candidate", {}) + worktree = candidate.get("worktree", {}) + for dirty_state in ("staged", "unstaged", "untracked"): + if worktree.get(dirty_state): + errors.append( + f"$.candidate.worktree.{dirty_state}: must be empty; every " + "invocation requires a dedicated globally clean worktree" + ) + has_binding = "source_binding" in candidate has_unavailable = "source_unavailable_reason" in candidate if has_binding and has_unavailable: From 80690173571678bb14d14703451a8ab6d29b2cba Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 14:30:32 -0700 Subject: [PATCH 20/22] docs: record the ninth review-fix-loop fix cycle in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc6d9a6..39e6e6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,19 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): add a per-pass `reviewer_identity` field to + `review_records` in both checkpoint and terminal-result, and reject a dirty + `candidate.worktree` (`staged`/`unstaged`/`untracked`) in + `validate_invocation`, closing the two gaps the ninth (final) + review-code-change pass on #96 found - fix(review-fix-loop): complete the terminal-result schema against the design's Terminal result contract field list (`worktree`, `resume_status`, `unresolved_or_deferred_findings`) and require `ahead_by`/`behind_by` alongside the head fields whenever a source is `bound`, closing the gaps the eighth review-code-change pass on #96 found + (`adf2ef5062038836d750b01e19f1549373ce1aad`) +- docs: record the eighth review-fix-loop fix cycle in the changelog + (`65a256a32b3087d064efb5e4a24725cfb1762467`) - fix(review-fix-loop): complete the checkpoint schema against the design's durable-checkpoint field list (`preserved_failed_attempts`, `pull_request`) and extend the optional pull-request identity cross-check to both From 0187dfc77444fbf410b5ed86a42a12e4d088e7b3 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 14:58:02 -0700 Subject: [PATCH 21/22] fix(review-fix-loop): reject converged when any review pass recorded a mutation attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- skills/review-fix-loop/references/CONTRACT.md | 19 ++++++++----- .../scripts/tests/test_validate.py | 28 +++++++++++++++++++ skills/review-fix-loop/scripts/validate.py | 12 +++++++- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/skills/review-fix-loop/references/CONTRACT.md b/skills/review-fix-loop/references/CONTRACT.md index 54db0d7..efaf024 100644 --- a/skills/review-fix-loop/references/CONTRACT.md +++ b/skills/review-fix-loop/references/CONTRACT.md @@ -214,13 +214,18 @@ cannot claim a terminal state without the evidence that state requires: is missing its required `focused` or `full` scope, or contains any entry that is not `passed`; when `review_records` has no entry bound to the exact final head and comparison base; when that final-head record's `aggregate_verdict` is - not `clean`; or when its `write_isolation` is not `enforced`. Under - `update_pr`, `publication.status` must be `published` and `unpushed_commits` - must be empty. Under `local_commit`, `publication.status` must be - `not_applicable` (local_commit never writes to origin), and any created - commits remain in `unpushed_commits` — that is the expected, non-error shape - of a converged `local_commit` result, and `operator_action` must describe how - the operator publishes them through their own workflow. + not `clean`; or when its `write_isolation` is not `enforced`. It also rejects + `converged` when *any* `review_records` entry — not only the final-head-bound + one — has a non-empty `mutation_attempts`: design's "Reviewer write + prevention" section states an attempted prohibited mutation invalidates that + pass even if the runtime blocked it, regardless of whether a later pass on a + different head came back clean and enforced. Under `update_pr`, + `publication.status` must be `published` and `unpushed_commits` must be empty. + Under `local_commit`, `publication.status` must be `not_applicable` + (local_commit never writes to origin), and any created commits remain in + `unpushed_commits` — that is the expected, non-error shape of a converged + `local_commit` result, and `operator_action` must describe how the operator + publishes them through their own workflow. - `changes_remaining`: `reason` must be one of `cycle_budget_exhausted`, `repeated_finding`, `oscillation`, `expanding_findings`, `repeated_failed_attempt`, or `current_candidate_validation_failure`. diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index 8e116d4..ff3583c 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -620,6 +620,34 @@ def test_converged_rejects_violated_write_isolation_on_final_head_record(self): errors, ) + def test_converged_rejects_mutation_attempt_on_final_record(self): + result = copy.deepcopy(self.converged_local) + result["review_records"][-1]["mutation_attempts"] = [ + 'attempted: git commit -am "reviewer edit"' + ] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.review_records[1]: converged cannot pair with a review record " + "that recorded a mutation attempt — an attempted reviewer " + "mutation invalidates that pass regardless of a later clean pass", + errors, + ) + + def test_converged_rejects_mutation_attempt_on_a_non_final_record(self): + # An attempted mutation on an earlier pass invalidates that pass even + # though a later pass came back clean and enforced. + result = copy.deepcopy(self.converged_local) + result["review_records"][0]["mutation_attempts"] = [ + "attempted: rm -rf .git/hooks" + ] + errors = VALIDATE.validate_terminal_result(result) + self.assertIn( + "$.review_records[0]: converged cannot pair with a review record " + "that recorded a mutation attempt — an attempted reviewer " + "mutation invalidates that pass regardless of a later clean pass", + errors, + ) + def test_converged_rejects_missing_final_head_review_record(self): result = copy.deepcopy(self.converged_local) result["review_records"] = [] diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index 4a33544..ad64283 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -572,13 +572,23 @@ def _check_converged_requires_clean_evidence(document: dict[str, Any]) -> list[s "validation entry" ) + all_records = document.get("review_records", []) + for index, record in enumerate(all_records): + if record.get("mutation_attempts"): + errors.append( + f"$.review_records[{index}]: converged cannot pair with a " + "review record that recorded a mutation attempt — an " + "attempted reviewer mutation invalidates that pass " + "regardless of a later clean pass" + ) + head = document.get("head", {}) base = document.get("comparison_base", {}) final_head = head.get("final") final_base = base.get("final", {}).get("sha") final_records = [ record - for record in document.get("review_records", []) + for record in all_records if record.get("head_sha") == final_head and record.get("comparison_base_sha") == final_base ] From 99094f87d31f018ce162c4a08ab78fe5f0bb6bc1 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 14:58:27 -0700 Subject: [PATCH 22/22] docs: record the tenth review-fix-loop fix cycle in the changelog Backfill SHAs onto the prior review-fix-loop entries and add today's new fix entry, per this repository's changelog convention. --- CHANGELOG.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39e6e6a..cf12948 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,16 @@ summary: Chronological history of repository and skill changes. ## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +- fix(review-fix-loop): reject `converged` when any `review_records` entry — not + only the final-head-bound one — recorded a mutation attempt, closing the gap + the tenth (final) review-code-change pass on #96 found - fix(review-fix-loop): add a per-pass `reviewer_identity` field to `review_records` in both checkpoint and terminal-result, and reject a dirty `candidate.worktree` (`staged`/`unstaged`/`untracked`) in - `validate_invocation`, closing the two gaps the ninth (final) - review-code-change pass on #96 found + `validate_invocation`, closing the two gaps the ninth review-code-change pass + on #96 found (`4daa2a67a38be3baa7741380eb689581ce31a1db`) +- docs: record the ninth review-fix-loop fix cycle in the changelog + (`80690173571678bb14d14703451a8ab6d29b2cba`) - fix(review-fix-loop): complete the terminal-result schema against the design's Terminal result contract field list (`worktree`, `resume_status`, `unresolved_or_deferred_findings`) and require `ahead_by`/`behind_by`