diff --git a/CHANGELOG.md b/CHANGELOG.md index 29c120d..7b3b03e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,14 +4,24 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-30 — Implemented the review-fix-loop reviewer isolation and complete-review orchestration and local execution substrate (common-directory locking, isolated attempts, checkpoint persistence, and recovery), 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 +## 2026-07-30 — Delivered and evaluated the standalone review-fix-loop `local_commit` workflow, implemented the review-fix-loop reviewer isolation and complete-review orchestration and local execution substrate (common-directory locking, isolated attempts, checkpoint persistence, and recovery), 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): compose the contract, local-execution, and + reviewer-orchestration leaves into the end-to-end standalone `local_commit` + workflow (`scripts/local_commit.py`'s `run_local_commit`), enforcing the + fix-cycle budget, committing selected fixes in isolated attempts, promoting + only a converged candidate, and reporting explicit retained-commit and + operator-action evidence for every non-converged stop, with end-to-end + fixtures for immediate convergence, one and multiple fix cycles, budget + exhaustion, validation failure (unavailable/untractable/tractable), operator + input (declined finding and scope expansion), expanding/oscillating finding + sets, repeated failed attempts, and interrupted-attempt recovery - fix(review-fix-loop): remove three subsumed/redundant tests and cut `reviewer_orchestration.py`'s docstring/comment density from roughly one line of prose per line of code down to sibling-module levels, replacing restated rationale with single pointers to `references/reviewer-orchestration.md`, closing the two code-simplicity gaps the sixth review-code-change pass on #98 - found + found (`fd690248670c6acecfb2d335e70e347d4d4390de`) - fix(review-fix-loop): correct an off-by-one changelog SHA attribution left by the previous fix cycle's own rebase cleanup — the duplicate-test entry and the `ignored`-comparison entry each carried the other's identity, closing the gap diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 1649f31..0e5ad07 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -1,6 +1,6 @@ --- name: review-fix-loop -description: Validate the review-fix-loop invocation, checkpoint, and terminal-result contracts; run the complete-review orchestration that later children of epic #95 build a fix loop on top of; and provide the local execution substrate (common-directory locking, isolated attempt worktrees, durable checkpoint persistence, verified fast-forward-only canonical promotion, and interrupted-attempt recovery). Use when authoring, resuming, or terminating a review-fix-loop invocation to check its documents against the shared schemas, when running one complete review pass (fresh reviewer subagent by default, explicit in-agent override otherwise) and recording its result, or when acquiring a candidate lock, running an isolated remediation attempt, or recovering an interrupted one. This skill does not yet select or apply a fix's content or publish anything; see design/review-fix-loop.md, references/CONTRACT.md, and references/reviewer-orchestration.md for the full behavioral design and current implementation status. +description: Validate the review-fix-loop invocation, checkpoint, and terminal-result contracts; run the complete-review orchestration and local execution substrate (locking, isolated attempt worktrees, checkpointing, fast-forward-only promotion, interrupted-attempt recovery); and run the standalone end-to-end `local_commit` workflow composing all of the above into one candidate-bound converge-or-stop loop. Use to check a review-fix-loop document against the shared schemas, to run and record one complete review pass, to acquire a candidate lock or run/recover an isolated remediation attempt, or to run the complete standalone `local_commit` loop end to end (review, decide, fix, validate, commit, repeat, converge or stop) with no remote publication. Does not yet publish anything (`update_pr`); see design/review-fix-loop.md, references/CONTRACT.md, references/reviewer-orchestration.md, and references/local-commit.md for the full design and implementation status. allowed-tools: Read, Grep, Glob, Bash, Agent, Task, Skill --- @@ -12,7 +12,7 @@ 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). -Three of its children are implemented so far: +Four of its children are implemented so far: - [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 @@ -34,13 +34,22 @@ Three of its children are implemented so far: locking, isolated attempt worktrees, durable checkpoint persistence and resume reconciliation, verified fast-forward-only canonical promotion, and recovery of an interrupted attempt. See [Local execution](#local-execution) below. - -This skill still does not select or apply a fix's content, or publish anything — -those behaviors belong to #99 (`local_commit`) and #100 (`update_pr`). Do not -invoke it expecting a complete, end-to-end review-fix loop yet; use it to -validate a document you or a later child produced against the shared schemas, to -run and record one complete review pass, or to acquire a lock, run an isolated -attempt, and recover an interrupted one. +- [Issue #99](https://github.com/shaug/agent-scripts/issues/99) composes all + three of the above into the actual standalone `local_commit` workflow: the + full Resolve/Establish evidence/Review/Decide/Fix/Validate and + commit/Invalidate and repeat/Publish/Return loop, fix-cycle budget + enforcement, the automatic non-convergence stops, and terminal-result assembly + — with no remote write in any path. See + [Run the standalone `local_commit` workflow](#run-the-standalone-local_commit-workflow) + below. + +This skill still does not publish anything to a remote — `update_pr` (#100) is +the only remaining behavior out of scope. Use +[`scripts/local_commit.py`](scripts/local_commit.py)'s `run_local_commit(...)` +to run a complete standalone `local_commit` invocation end to end; use the +lower-level modules directly only when you need just one of their individual +behaviors (validating a document, running one review pass, or acquiring a lock +and running one isolated attempt) outside the full loop. ## Load the contracts @@ -174,10 +183,38 @@ promotion-race, and cleanup-safety coverage. Selecting which finding to fix, writing the fix's content, and publishing to a remote remain out of scope here; a caller supplies the fix content and invokes these primitives around it. +## Run the standalone `local_commit` workflow + +Read [`references/local-commit.md`](references/local-commit.md) in full before +running an end-to-end invocation; it implements every remaining "Workflow" step +design describes plus "Convergence and stop conditions" and the `local_commit` +half of "Publication policy". In summary: +[`scripts/local_commit.py`](scripts/local_commit.py)'s `run_local_commit(...)` +composes `validate.py`, `local_execution.py`, and `reviewer_orchestration.py` +into the complete loop: resolve the invocation and acquire the candidate lock; +establish and classify validation evidence; run a complete review and detect any +reviewer mutation; decide each selected finding's disposition; fix, validate, +and commit an accepted finding in an isolated attempt, promoting only on +success; repeat with a fresh review after every promotion; and return one +schema-valid, candidate-bound terminal result (`converged`, `changes_remaining`, +or `blocked`) — never performing a remote write. The three genuinely +host-boundary actions (running one review pass, deciding a finding's +disposition, and writing a fix's content) are supplied by the caller as small +callables; every other action — locking, Git, checkpointing, promotion, +review-result evaluation — happens for real, matching this skill's existing "no +mocked Git state" testing convention. + +See [`scripts/tests/test_local_commit.py`](scripts/tests/test_local_commit.py) +for complete end-to-end coverage: immediate convergence, one and multiple fix +cycles, budget exhaustion, a validation failure that is unavailable, +untractable, and tractable, a declined finding and a scope-expanding fix (both +surfaced as operator input), a fix that introduces a new finding, an oscillating +finding set, two consecutive failed attempts, recovery from an interrupted +attempt, an already-held candidate lock, an attempted reviewer mutation, and a +host without fresh-subagent support. + ## Non-goals -- Deciding which finding to accept, reject, or defer, and applying the resulting - fix (a later child's "Decide"/"Fix" workflow steps). - Publishing anything, including the `update_pr` expected-old fast-forward update (issue #100). - Migrating `implement-ticket`, `babysit-pr`, `carve-changesets`, or any other diff --git a/skills/review-fix-loop/references/local-commit.md b/skills/review-fix-loop/references/local-commit.md new file mode 100644 index 0000000..1b2da77 --- /dev/null +++ b/skills/review-fix-loop/references/local-commit.md @@ -0,0 +1,220 @@ +# Standalone `local_commit` workflow + +This document implements +[`design/review-fix-loop.md`](../../../design/review-fix-loop.md)'s "Workflow" +section (steps 1 "Resolve" through 9 "Return"), "Convergence and stop +conditions", "Terminal result contract", and the `local_commit` parts of +"Publication policy" — for the executing agent that follows +[`SKILL.md`](../SKILL.md). It does not redefine any contract, locking, +checkpoint, or reviewer-orchestration behavior already owned by +[`references/CONTRACT.md`](CONTRACT.md) and +[`references/reviewer-orchestration.md`](reviewer-orchestration.md); it composes +them into the one entry point issue #99 delivers: +[`scripts/local_commit.py`](../scripts/local_commit.py)'s +`run_local_commit(...)`. + +`update_pr` (issue #100) and remote publication of any kind are out of scope +here; every path in this module ends without a remote write, per this +invocation's `publication.policy: local_commit`. + +## What this module composes, and what it does not reimplement + +`run_local_commit` sequences calls into the three already-merged children of +epic #95: + +- `validate.py` (#96) validates the invocation up front and every checkpoint and + terminal result this module produces, so a caller never receives a document + that would fail its own contract. +- `local_execution.py` (#97) owns the candidate lock, isolated attempt + worktrees, durable checkpoint persistence, verified fast-forward-only + promotion, and interrupted-attempt recovery. This module never reimplements + Git locking, promotion, or recovery logic — it calls the existing functions. +- `reviewer_orchestration.py` (#98) owns fixed lens resolution, raw-result + evaluation and candidate binding, mutation detection, reviewer identity, and + deterministic finding ordering. This module never reimplements + `evaluate_review_result`, `detect_worktree_mutation`, or `select_next_finding` + — it calls them once per review pass. + +What issue #99 itself owns, and what did not exist before this module: + +- the loop's control flow across the design's nine workflow steps; +- fix-cycle budget enforcement (reserving a cycle before the first mutation of + an attempt, whether it commits, fails, or is interrupted); +- the "Decide" step's disposition/scope/operator-input branching; +- the automatic non-convergence stops that are safely detectable from + candidate-visible evidence alone (`expanding_findings`, `oscillation`, + `repeated_failed_attempt`, `cycle_budget_exhausted`); and +- assembling and validating the checkpoint and terminal-result documents this + invocation produces. + +## Host boundary: the three ports + +A dependency-free module cannot itself spawn a review subagent, write a fix's +actual content, or decide whether a finding is genuinely tractable and in scope. +`run_local_commit` accepts these as small callables ("ports") so the executing +agent supplies exactly the judgment the design assigns to it, nothing more: + +- `reviewer(*, packet, briefing, head_sha, comparison_base_sha, independence, sequence) -> ReviewPass` + — run one complete `review-code-change` pass. In Claude Code, this means + spawning a fresh subagent restricted to + `Read, Grep, Glob, Bash, Agent, Task, Skill` per + [`references/reviewer-orchestration.md`](reviewer-orchestration.md) and + returning its raw aggregate result. `run_local_commit` still performs the + before/after mutation snapshot, `evaluate_review_result` binding, and + `write_isolation` recording around this call — the port only needs to return + the raw result (plus any tool-trace mutation evidence the runtime observed). +- `decide(*, finding, change_contract, attempt_number) -> FixDecision` — verify + the selected finding's evidence against the candidate, confirm it is within + `change_contract.allowed_remediation_scope`, and return `accepted`, + `rejected`, or `deferred` with a rationale. Set `expands_scope=True` when a + correct fix would exceed the allowed scope, or `operator_input_required=True` + when the finding itself needs a human decision before any fix is attempted. +- `apply_fix(*, finding, attempt_path, change_contract, attempt_number) -> str` + — write the smallest coherent remediation directly into the isolated attempt + worktree at `attempt_path` and return a commit message. Called only after + `decide` returns `accepted` with neither `expands_scope` nor + `operator_input_required` set, and only when budget remains. +- `run_validation(*, name, command, scope, cwd) -> ValidationOutcome` (optional; + defaults to a real `subprocess` runner) — run one recorded validation command. + Never interpolate untrusted text into `command`; it must already be one of the + invocation's own recorded commands. +- `classify_validation_failure(*, outcome, invocation) -> dict | None` + (optional; defaults to always returning `None`) — called only when validation + fails for a head no fix cycle just produced. Returning `None` means the + failure has no tractable correction, per the design; returning a + finding-shaped dict routes it through the same decide/fix pipeline as an + ordinary review finding, consuming a fix cycle like one. + +Every other action — acquiring the candidate lock, running +`git diff`/`git show`, building the raw review packet, creating and promoting +attempts, writing checkpoints — happens for real against the repository at +`repo`. There is no mocked Git state anywhere in this module or its tests, +matching `local_execution.py`'s own testing convention. + +## The loop, step by step + +1. **Resolve.** Validate the invocation against `validate.validate_invocation` + and reject anything but `publication.policy: local_commit`. Acquire the local + candidate lock (`local_execution.acquire_candidate_locks`); a busy lock + returns `blocked/candidate_busy` without touching the repository. Reconcile a + supplied `resume_checkpoint` through + `local_execution.reconcile_checkpoint_for_resume`, then recover any + interrupted attempt left behind by a prior crashed run through + `local_execution.recover_interrupted_attempts` — each uniquely identifiable + leftover is discarded (preserved for inspection) and recorded as one + `interrupted` cycle attempt, consuming budget exactly as the design requires + ("the reserved cycle is spent regardless of outcome"). +2. **Establish evidence.** Capture the live head and comparison base and verify + them against the invocation's recorded candidate identity (a mismatch is + `blocked/candidate_integrity_failure`). Run the invocation's recorded focused + and full validation against the current head. An `unavailable` scope is + `blocked/validation_unavailable`. A `failed` scope is classified via + `classify_validation_failure`: untractable is + `changes_remaining/current_candidate_validation_failure` (no cycle consumed); + tractable routes the returned finding through Decide/Fix like any other + finding. +3. **Review.** Resolve the review-execution mode + (`reviewer_orchestration.resolve_review_execution_mode`); an unsupported + `fresh_subagent` host with no explicit override is + `blocked/missing_capability`. Build the raw packet, snapshot the worktree + before and after the reviewer port runs, and feed both snapshots plus the raw + result into `reviewer_orchestration.build_review_record`. Any detected + mutation attempt is `blocked/reviewer_integrity_failure` immediately — this + cycle is never "fixed up" by continuing to iterate on it. A `clean` verdict + ends the loop as `converged`. A `blocked` verdict from the review pass itself + is `blocked/missing_capability`. +4. **Decide.** `reviewer_orchestration.select_next_finding` picks the next + gating finding in the fixed severity/lens/id order. `None` (only deferred + findings remain) is `blocked/operator_input_required`. Otherwise the `decide` + port runs once per finding; its disposition is recorded on the owning review + pass immediately, so a declined finding's rationale is visible in the + terminal result's `review_records` even though it was never fixed. + `rejected`/`deferred` is `blocked/operator_input_required`; `expands_scope` + is `blocked/scope_decision_required`; a generic `operator_input_required` + flag is `blocked/operator_input_required`. +5. **Fix.** If budget remains, reserve one cycle, create an isolated attempt + from the exact current head (`local_execution.create_attempt`), and call + `apply_fix`. If budget is already exhausted, stop as + `changes_remaining/cycle_budget_exhausted` before creating an attempt — the + same accepted finding remains visible as unresolved. +6. **Validate and commit.** Run the recorded validation inside the attempt + worktree. On success, commit and promote through the verified + fast-forward-only path (`local_execution.commit_attempt` + + `local_execution.promote_attempt`); the canonical worktree, checkpoint, and + in-memory state all advance together. On failure, discard the attempt + (preserved for inspection) and retry the same accepted finding from the same + head — no fresh review is needed since nothing changed. Two consecutive + failures from the same head stop early as + `changes_remaining/repeated_failed_attempt`, preserving remaining budget. +7. **Invalidate and repeat.** A successful promotion always triggers a fresh + review at the new head before anything else. Two additional automatic, + evidence-based stops fire here, both preserving budget: a fix that leaves + every prior gating finding present and adds new ones is + `changes_remaining/expanding_findings`; a finding set that returns to exactly + a set seen two reviews ago after having actually changed in between is + `changes_remaining/oscillation`. A finding that simply survives unchanged, + cycle after cycle, is deliberately **not** an automatic early stop — the + design tolerates that until the budget itself is exhausted (see + `references/examples/local-commit-terminal-changes-remaining.json`, which + consumes all three cycles on one recurring finding rather than stopping + early). A `decide` port sophisticated enough to notice unproductive + repetition may still request `operator_input_required` itself; the engine + does not second-guess that judgment call on its behalf. +8. **Publish.** Never, under `local_commit`. Every terminal result reports + `publication: {policy: "local_commit", status: "not_applicable", non_converged_exposure: false}` + and lists every created commit in `unpushed_commits` — the expected, + non-error shape of a converged (or non-converged) `local_commit` result. + `operator_action` always names how the operator publishes those commits + through their own workflow. +9. **Return.** Every return path writes a durable checkpoint + (`local_execution.write_checkpoint_atomic`) before releasing the candidate + lock, then assembles and validates (`validate.validate_terminal_result`) + exactly one terminal-result document. `run_local_commit` never returns a + document that would fail its own schema; an internal inconsistency raises + `LocalCommitError` instead of silently shipping bad evidence. + +## Terminal states this module actually returns + +- `converged` — the final head's review is `clean`, required validation passed, + and `write_isolation` was `enforced` throughout. +- `changes_remaining` — `cycle_budget_exhausted`, + `current_candidate_validation_failure`, `repeated_failed_attempt`, + `expanding_findings`, or `oscillation`. +- `blocked` — `candidate_busy`, `candidate_integrity_failure`, + `checkpoint_mismatch`, `missing_capability`, `reviewer_integrity_failure`, + `validation_unavailable`, `operator_input_required`, or + `scope_decision_required`. + +`repeated_finding` (a finding surviving one fix without a stronger pattern) is +deliberately not one of this module's own automatic stop reasons — see step 7 +above. `base_drift`, `remote_advanced`, and `publication_failed` never apply to +`local_commit`, since this module never reads a remote or a shared base outside +its own repository, and never touches `update_pr`'s publication path. + +## Recovery + +`resume_checkpoint` reconciles a prior invocation's durable checkpoint against +live Git state before doing anything else. A lock still held by another process, +or any cross-document identity mismatch, returns `blocked/candidate_busy` or +`blocked/checkpoint_mismatch` without mutation. On a clean reconciliation, any +attempt branch left behind by an interrupted run is recovered, discarded, and +recorded as one `interrupted` cycle attempt — visible in both the checkpoint and +the terminal result's `preserved_failed_attempts` — before the loop resumes +forward exactly as a fresh invocation would. + +## Tests + +[`scripts/tests/test_local_commit.py`](../scripts/tests/test_local_commit.py) +drives `run_local_commit` against real temporary Git repositories (no mocked Git +state) with small deterministic reviewer/decide/apply_fix fakes, covering: +immediate convergence with no fix cycle; one and multiple successful fix cycles; +budget exhaustion on a recurring finding; a validation failure that is +`unavailable`, untractable, and tractable (fixed through the synthetic-finding +path); a declined finding and a scope-expanding fix, both surfaced as operator +input; a fix that introduces a new finding (`expanding_findings`); an +oscillating finding set; two consecutive failed attempts +(`repeated_failed_attempt`); an interrupted attempt recovered and resumed to +convergence; an already-held candidate lock (`candidate_busy`); an attempted +reviewer mutation (`reviewer_integrity_failure`); a host without fresh-subagent +support and no override (`missing_capability`); and rejection of an invalid +invocation or an `update_pr` invocation at the API boundary. diff --git a/skills/review-fix-loop/scripts/local_commit.py b/skills/review-fix-loop/scripts/local_commit.py new file mode 100644 index 0000000..6ce2367 --- /dev/null +++ b/skills/review-fix-loop/scripts/local_commit.py @@ -0,0 +1,1248 @@ +#!/usr/bin/env python3 +"""End-to-end standalone `local_commit` workflow for `review-fix-loop` (issue #99). + +Composes the three already-merged children of epic #95 into the actual loop +`design/review-fix-loop.md`'s "Workflow" section describes (steps 1 "Resolve" +through 9 "Return"), restricted to the `local_commit` publication policy: + +- `validate.py` (#96) — the invocation/checkpoint/terminal-result contract and + its cross-field semantics. +- `local_execution.py` (#97) — common-directory locking, isolated attempt + worktrees, durable checkpointing, verified fast-forward-only promotion, and + interrupted-attempt recovery. +- `reviewer_orchestration.py` (#98) — fixed lens resolution, review-result + evaluation/binding, mutation detection, and deterministic finding ordering. + +This module does not reimplement any of that logic; it sequences calls into +the three modules above and owns only what issue #99 itself is responsible +for: the loop's control flow (Resolve, Establish evidence, Review, Decide, +Fix, Validate and commit, Invalidate and repeat, Publish, Return), fix-cycle +budget enforcement, and terminal-result assembly. + +## Host boundary + +Three actions in the design are inherently host/runtime actions that this +dependency-free module cannot perform itself — spawning a review subagent, +writing a fix's actual content, and running an arbitrary validation command — +so they are supplied by the caller as small callables ("ports"): + +- `reviewer(*, packet, briefing, head_sha, comparison_base_sha, independence, + sequence) -> ReviewPass` — run one complete `review-code-change` pass + (fresh subagent by default) and return its raw result. +- `decide(*, finding, change_contract, attempt_number) -> FixDecision` — + verify a selected finding's evidence, confirm it is within + `allowed_remediation_scope`, and accept, reject, or defer it. +- `apply_fix(*, finding, attempt_path, change_contract, attempt_number) -> + str` — write the smallest coherent remediation into the isolated attempt + worktree and return a commit message. Only called after `decide` accepts. +- `run_validation(*, name, command, scope, cwd) -> ValidationOutcome` — run one + recorded validation command. Defaults to a real subprocess runner. +- `classify_validation_failure(*, outcome, invocation) -> SyntheticFinding | + None` — called only when validation fails for a head no fix cycle just + produced (i.e., the failure was not itself just re-checked by a promotion). + Returning `None` (the default) means the failure is not tractable per the + design ("no tractable correction remains"); returning a finding-shaped dict + routes it through the same decide/fix pipeline as a review finding. + +Every other action (git, locking, checkpointing, promotion, review-result +evaluation) happens for real against the repository at `repo`, matching the +testing convention already used by `local_execution.py` and its tests: no +mocked Git state. +""" + +from __future__ import annotations + +import dataclasses +import subprocess +import sys +from importlib import util as importlib_util +from pathlib import Path +from typing import Any, Callable, Mapping, Sequence + +HERE = Path(__file__).resolve().parent + + +def _load_bundled_module(name: str, path: Path): + spec = importlib_util.spec_from_file_location(name, path) + assert spec and spec.loader + module = importlib_util.module_from_spec(spec) + # Registered before exec_module: local_execution.py's dataclasses use + # `from __future__ import annotations`, and this mirrors the same + # precaution `scripts/tests/test_local_execution.py` takes so Python's + # dataclass machinery can resolve string type hints via + # `sys.modules[cls.__module__]` if it ever needs to. + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +VALIDATE = _load_bundled_module("review_fix_loop_validate", HERE / "validate.py") +LE = _load_bundled_module( + "review_fix_loop_local_execution", HERE / "local_execution.py" +) +ORCH = _load_bundled_module( + "review_fix_loop_reviewer_orchestration", HERE / "reviewer_orchestration.py" +) + + +class LocalCommitError(RuntimeError): + """A precondition this module requires of its caller was not met. + + Raised only for a caller/programming error (an invalid invocation, or an + invocation whose `publication.policy` is not `local_commit`) — never for + an ordinary runtime stop condition, which is always a structured terminal + result instead. + """ + + +# --------------------------------------------------------------------------- +# Ports (host-boundary callables the caller supplies) +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass +class ReviewPass: + """One raw review-code-change pass and its isolation evidence.""" + + result: Mapping[str, Any] + reviewer_identity: str | None = None + mutation_attempts: Sequence[str] = () + + +@dataclasses.dataclass +class FixDecision: + """The result of the design's "Decide" workflow step for one finding.""" + + disposition: str # "accepted" | "rejected" | "deferred" + rationale: str + expands_scope: bool = False + operator_input_required: bool = False + + +@dataclasses.dataclass +class ValidationOutcome: + status: str # "passed" | "failed" | "unavailable" + result: str | None = None + reason: str | None = None + + +ReviewerPort = Callable[..., ReviewPass] +DeciderPort = Callable[..., FixDecision] +FixerPort = Callable[..., str] +ValidationRunnerPort = Callable[..., ValidationOutcome] +ValidationClassifierPort = Callable[..., dict[str, Any] | None] + + +def default_run_validation( + *, name: str, command: str, scope: str, cwd: Path +) -> ValidationOutcome: + """Real subprocess validation runner: the default `run_validation` port. + + A caller invoking this against a repository-instructed command (never + interpolating untrusted text into it) may use this directly; tests + supply their own deterministic port instead, per this module's + docstring. + """ + del name, scope # only the command and cwd matter to execute it + try: + completed = subprocess.run( + command, shell=True, cwd=str(cwd), text=True, capture_output=True + ) + except OSError as exc: + return ValidationOutcome(status="unavailable", reason=str(exc)) + output = (completed.stdout or "") + (completed.stderr or "") + if completed.returncode == 0: + return ValidationOutcome(status="passed", result=output.strip() or "OK") + return ValidationOutcome( + status="failed", result=output.strip() or f"exit code {completed.returncode}" + ) + + +def _default_classify_validation_failure( + *, outcome: ValidationOutcome, invocation: Mapping[str, Any] +) -> dict[str, Any] | None: + del outcome, invocation + return None + + +# --------------------------------------------------------------------------- +# Internal helpers +# --------------------------------------------------------------------------- + + +def _worktree_snapshot(repo: Path) -> dict[str, Any]: + """Capture the `REQUIRED_SNAPSHOT_KEYS`-shaped before/after snapshot + `detect_worktree_mutation` compares.""" + status = LE.worktree_status(repo) + refs_output = LE.git( + "for-each-ref", "--format=%(refname) %(objectname)", cwd=repo + ).stdout + refs: dict[str, str] = {} + for line in refs_output.splitlines(): + if not line: + continue + ref_name, _, object_id = line.partition(" ") + refs[ref_name] = object_id + return {"head_sha": LE.current_head(repo), "refs": refs, **status} + + +def _run_validation_suite( + invocation: Mapping[str, Any], *, cwd: Path, run_validation: ValidationRunnerPort +) -> list[dict[str, Any]]: + outcomes: list[dict[str, Any]] = [] + for entry in invocation["validation"]: + outcome = run_validation( + name=entry["name"], command=entry["command"], scope=entry["scope"], cwd=cwd + ) + record: dict[str, Any] = { + "name": entry["name"], + "command": entry["command"], + "scope": entry["scope"], + "status": outcome.status, + } + if outcome.result is not None: + record["result"] = outcome.result + if outcome.reason is not None: + record["reason"] = outcome.reason + outcomes.append(record) + return outcomes + + +def _unavailable_scope(outcomes: Sequence[Mapping[str, Any]]) -> str | None: + for outcome in outcomes: + if outcome["status"] == "unavailable": + return outcome["scope"] + return None + + +def _failed_outcome( + outcomes: Sequence[Mapping[str, Any]], +) -> Mapping[str, Any] | None: + for outcome in outcomes: + if outcome["status"] == "failed": + return outcome + return None + + +def _build_packet( + invocation: Mapping[str, Any], + *, + head_sha: str, + comparison_base_sha: str, + diff: Mapping[str, Any], + validation_outcomes: Sequence[Mapping[str, Any]], +) -> dict[str, Any]: + change_contract = invocation["change_contract"] + return { + "schema_version": "1.0", + "repository": { + "identity": invocation["repository"]["identity"], + "base_branch": invocation["candidate"]["comparison_base"]["ref"], + }, + "candidate": { + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "diff": dict(diff), + }, + "change_contract": { + "goal": change_contract["goal"], + "acceptance_criteria": list(change_contract["acceptance_criteria"]), + "non_goals": list(change_contract["non_goals"]), + "preserved_behaviors": list(change_contract["preserved_behaviors"]), + }, + "sources": { + "repository_instructions": list( + change_contract["sources"]["repository_instructions"] + ), + "named_documents": list(change_contract["sources"]["named_documents"]), + "nearby_patterns": list(change_contract["sources"]["nearby_patterns"]), + }, + "validation": list(validation_outcomes), + } + + +def _current_diff(repo: Path, base_sha: str, head_sha: str) -> dict[str, Any]: + content = LE.git("diff", base_sha, head_sha, cwd=repo).stdout + return {"format": "unified_diff", "complete": True, "content": content} + + +def _commits_between(repo: Path, old: str, new: str) -> int: + return int(LE.git("rev-list", "--count", f"{old}..{new}", cwd=repo).stdout.strip()) + + +def _checkpoint_source_status( + candidate: Mapping[str, Any], *, repo: Path, current_head: str +) -> dict[str, Any]: + """Build this checkpoint's `source` status. + + A `local_commit` invocation may still carry an optional read-only + `source_binding` purely for comparison (design: "An optional read-only + source binding ... required when the candidate has a known pushable + source"; "A read-only source binding grants comparison authority only; + it never implies permission to push"). When one is present, report + `bound` with the exact ahead/behind counts relative to its + `observed_object_id`; otherwise report the invocation's own + `source_unavailable_reason` verbatim. + """ + source_binding = candidate.get("source_binding") + if source_binding is None: + return { + "status": "unavailable", + "unavailable_reason": candidate.get( + "source_unavailable_reason", "no recorded pushable source" + ), + } + source_sha = source_binding["observed_object_id"] + return { + "status": "bound", + "last_verified_head": source_sha, + "ahead_by": _commits_between(repo, source_sha, current_head), + "behind_by": _commits_between(repo, current_head, source_sha), + } + + +def _terminal_source_status( + candidate: Mapping[str, Any], *, repo: Path, current_head: str +) -> dict[str, Any]: + """Terminal-result equivalent of `_checkpoint_source_status`. + + `local_commit` never re-fetches or advances a bound source (that + remains `update_pr`-only, per this ticket's non-goals); `initial_head` + and `final_head` are therefore both this invocation's one observed + source object, and only the local ahead/behind counts relative to the + final candidate head can change. + """ + source_binding = candidate.get("source_binding") + if source_binding is None: + return { + "status": "unavailable", + "unavailable_reason": candidate.get( + "source_unavailable_reason", "no recorded pushable source" + ), + } + source_sha = source_binding["observed_object_id"] + return { + "status": "bound", + "initial_head": source_sha, + "final_head": source_sha, + "ahead_by": _commits_between(repo, source_sha, current_head), + "behind_by": _commits_between(repo, current_head, source_sha), + } + + +def _checkpoint_preserved(preserved: Mapping[str, Any]) -> dict[str, str]: + """Narrow `discard_attempt`'s `{attempt_ref, patch_path, reason}` return + to the `{attempt_ref, reason}` shape `checkpoint.schema.json` and + `terminal-result.schema.json` both actually declare (`additionalProperties: + false`); `patch_path` is on-disk-only operator-inspection detail, not part + of either document contract.""" + return {"attempt_ref": preserved["attempt_ref"], "reason": preserved["reason"]} + + +# --------------------------------------------------------------------------- +# The loop +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass +class _State: + invocation: Mapping[str, Any] + repo: Path + common_dir: Path + branch: str + initial_head: str + current_head: str + base_ref: str + current_base_sha: str + original_budget: int + cycle_attempts: list[dict[str, Any]] = dataclasses.field(default_factory=list) + head_history: list[str] = dataclasses.field(default_factory=list) + base_revision_history: list[dict[str, str]] = dataclasses.field( + default_factory=list + ) + review_records: list[dict[str, Any]] = dataclasses.field(default_factory=list) + validation_outcomes: list[dict[str, Any]] = dataclasses.field(default_factory=list) + preserved_failed_attempts: list[dict[str, str]] = dataclasses.field( + default_factory=list + ) + finding_dispositions: list[dict[str, Any]] = dataclasses.field(default_factory=list) + unresolved_or_deferred: list[str] = dataclasses.field(default_factory=list) + resume_status: str = "not_resumed" + + def consumed_cycles(self) -> int: + return len(self.cycle_attempts) + + def remaining_cycles(self) -> int: + return self.original_budget - self.consumed_cycles() + + def record_finding_disposition( + self, + *, + finding_id: str, + disposition: str, + rationale: str, + fix_commit_sha: str | None = None, + ) -> None: + """Record (or replace) this finding's one current top-level + disposition (`selected` or `declined`). + + A `finding_id` can legitimately gain more than one disposition + across an invocation's lifetime — declined on one pass, then + accepted and fixed once resumed with a different decision, or + reconstructed from an older checkpoint entry and then superseded by + a fresh live decision in this same run. Only the most recent one is + meaningful evidence, so this always removes any prior entry for the + same `finding_id` from both `finding_dispositions` and + `unresolved_or_deferred_findings` before appending the new one, + rather than letting a `converged` result carry stale, contradictory + history for a finding that was in fact fixed (or vice versa). + """ + self.finding_dispositions = [ + entry + for entry in self.finding_dispositions + if entry["finding_id"] != finding_id + ] + self.unresolved_or_deferred = [ + entry + for entry in self.unresolved_or_deferred + if not entry.startswith(f"{finding_id}:") + ] + entry: dict[str, Any] = { + "finding_id": finding_id, + "disposition": disposition, + "rationale": rationale, + } + if fix_commit_sha is not None: + entry["fix_commit_sha"] = fix_commit_sha + self.finding_dispositions.append(entry) + if disposition == "declined": + self.unresolved_or_deferred.append(f"{finding_id}: {rationale}") + + +def _checkpoint_document( + state: _State, *, phase: str, next_action: str +) -> dict[str, Any]: + invocation = state.invocation + publication: dict[str, Any] = {"policy": "local_commit"} + document = { + "schema_version": "1.0", + "invocation_id": invocation["invocation_id"], + "repository": dict(invocation["repository"]), + "branch": state.branch, + "worktree": LE.worktree_status(state.repo), + "initial_head": state.initial_head, + "current_head": state.current_head, + "comparison_base": {"ref": state.base_ref, "sha": state.current_base_sha}, + "publication": publication, + "original_cycle_budget": state.original_budget, + "cycle_attempts": list(state.cycle_attempts), + "head_history": list(state.head_history), + "base_revision_history": list(state.base_revision_history), + "review_records": list(state.review_records), + "validation_outcomes": list(state.validation_outcomes), + "preserved_failed_attempts": list(state.preserved_failed_attempts), + "source": _checkpoint_source_status( + invocation["candidate"], repo=state.repo, current_head=state.current_head + ), + "current_phase": phase, + "expected_next_action": next_action, + } + if "pull_request" in invocation["candidate"]: + document["pull_request"] = invocation["candidate"]["pull_request"] + return document + + +def _persist_checkpoint( + state: _State, *, phase: str, next_action: str +) -> dict[str, Any]: + document = _checkpoint_document(state, phase=phase, next_action=next_action) + path = LE.checkpoint_path(state.common_dir, state.invocation["invocation_id"]) + LE.write_checkpoint_atomic(path, document) + return document + + +def _terminal_result( + state: _State, + *, + terminal_state: str, + reason: str | None, + operator_action: str, +) -> dict[str, Any]: + invocation = state.invocation + consumed = state.consumed_cycles() + remaining = state.remaining_cycles() + created_commits = state.head_history[1:] + identity_changed = ( + state.current_head != state.initial_head + or state.current_base_sha != state.base_revision_history[0]["sha"] + ) + document: dict[str, Any] = { + "schema_version": "1.0", + "invocation_id": invocation["invocation_id"], + "terminal_state": terminal_state, + "budget": { + "original_max_fix_cycles": state.original_budget, + "consumed_cycles": consumed, + "remaining_cycles": remaining, + }, + "resume_status": state.resume_status, + "repository": dict(invocation["repository"]), + "branch": state.branch, + "worktree": LE.worktree_status(state.repo), + "head": {"initial": state.initial_head, "final": state.current_head}, + "comparison_base": { + "initial": dict(state.base_revision_history[0]), + "final": {"ref": state.base_ref, "sha": state.current_base_sha}, + }, + "head_history": list(state.head_history), + "base_revision_history": list(state.base_revision_history), + "review_records": list(state.review_records), + "validation_summary": list(state.validation_outcomes), + "finding_dispositions": list(state.finding_dispositions), + "created_commits": list(created_commits), + "preserved_failed_attempts": list(state.preserved_failed_attempts), + "source": _terminal_source_status( + invocation["candidate"], repo=state.repo, current_head=state.current_head + ), + "unpushed_commits": list(created_commits), + "publication": { + "policy": "local_commit", + "status": "not_applicable", + "non_converged_exposure": False, + }, + "acceptance_reconciliation_required": bool(identity_changed), + "unresolved_or_deferred_findings": list(state.unresolved_or_deferred), + "operator_action": operator_action, + } + if reason is not None: + document["reason"] = reason + if "pull_request" in invocation["candidate"]: + document["pull_request"] = invocation["candidate"]["pull_request"] + return document + + +def _finalize( + state: _State, + *, + terminal_state: str, + reason: str | None, + operator_action: str, + phase: str, +) -> dict[str, Any]: + _persist_checkpoint(state, phase=phase, next_action=operator_action) + result = _terminal_result( + state, + terminal_state=terminal_state, + reason=reason, + operator_action=operator_action, + ) + errors = VALIDATE.validate_terminal_result(result) + if errors: + raise LocalCommitError( + "internal error: assembled an invalid terminal result: " + "; ".join(errors) + ) + return result + + +def _minimal_blocked_result( + invocation: Mapping[str, Any], *, repo: Path, reason: str, operator_action: str +) -> dict[str, Any]: + """Build a `blocked` terminal result for a stop that occurs before this + invocation could acquire its candidate lock — nothing has been + checkpointed or mutated yet, so this reports only the invocation's own + static candidate identity rather than any live-derived progress. A + read-only source-binding ahead/behind comparison is still safe and + meaningful here (it never touches the candidate lock or worktree).""" + candidate = invocation["candidate"] + base = dict(candidate["comparison_base"]) + head = candidate["head_sha"] + budget = invocation["fix_cycle_budget"]["max_fix_cycles"] + document: dict[str, Any] = { + "schema_version": "1.0", + "invocation_id": invocation["invocation_id"], + "terminal_state": "blocked", + "reason": reason, + "budget": { + "original_max_fix_cycles": budget, + "consumed_cycles": 0, + "remaining_cycles": budget, + }, + "resume_status": "not_resumed", + "repository": dict(invocation["repository"]), + "branch": candidate["branch"], + "worktree": dict(candidate["worktree"]), + "head": {"initial": head, "final": head}, + "comparison_base": {"initial": base, "final": base}, + "head_history": [head], + "base_revision_history": [base], + "review_records": [], + "validation_summary": [], + "finding_dispositions": [], + "created_commits": [], + "preserved_failed_attempts": [], + "source": _terminal_source_status(candidate, repo=repo, current_head=head), + "unpushed_commits": [], + "publication": { + "policy": "local_commit", + "status": "not_applicable", + "non_converged_exposure": False, + }, + "acceptance_reconciliation_required": False, + "unresolved_or_deferred_findings": [], + "operator_action": operator_action, + } + if "pull_request" in candidate: + document["pull_request"] = candidate["pull_request"] + errors = VALIDATE.validate_terminal_result(document) + if errors: + raise LocalCommitError( + "internal error: assembled an invalid terminal result: " + "; ".join(errors) + ) + return document + + +def run_local_commit( + invocation: Mapping[str, Any], + *, + repo: Path, + reviewer: ReviewerPort, + decide: DeciderPort, + apply_fix: FixerPort, + run_validation: ValidationRunnerPort = default_run_validation, + classify_validation_failure: ValidationClassifierPort = _default_classify_validation_failure, + host_supports_fresh_subagent: bool = True, + attempts_root: Path | None = None, + resume_checkpoint: Mapping[str, Any] | None = None, +) -> dict[str, Any]: + """Run (or resume) one standalone `local_commit` review-fix-loop invocation. + + Returns a schema-valid terminal-result document (`converged`, + `changes_remaining`, or `blocked`). Every successful fix is committed + locally and promoted through the canonical worktree at `repo`; no remote + write ever occurs, per this invocation's `local_commit` policy. + """ + errors = VALIDATE.validate_invocation(dict(invocation)) + if errors: + raise LocalCommitError( + "invocation failed contract validation: " + "; ".join(errors) + ) + policy = invocation["publication"]["policy"] + if policy != "local_commit": + raise LocalCommitError( + f"run_local_commit only handles publication.policy 'local_commit', got {policy!r}" + ) + + candidate = invocation["candidate"] + branch = candidate["branch"] + common_dir = LE.git_common_dir(repo) + local_ref = f"refs/heads/{branch}" + attempts_root = attempts_root or LE.default_attempts_root(common_dir) + + lock_cm = LE.acquire_candidate_locks(common_dir, local_ref) + try: + lock_cm.__enter__() + except LE.CandidateBusyError as exc: + return _minimal_blocked_result( + invocation, + repo=repo, + reason="candidate_busy", + operator_action=f"the candidate lock is already held: {exc}", + ) + + try: + live_head = LE.current_head(repo) + live_status = LE.worktree_status(repo) + if not LE.is_clean(live_status): + raise LocalCommitError( + "worktree must be clean before a local_commit invocation" + ) + + state = _State( + invocation=invocation, + repo=repo, + common_dir=common_dir, + branch=branch, + initial_head=candidate["head_sha"], + current_head=live_head, + base_ref=candidate["comparison_base"]["ref"], + current_base_sha=candidate["comparison_base"]["sha"], + original_budget=invocation["fix_cycle_budget"]["max_fix_cycles"], + ) + + if resume_checkpoint is not None: + try: + LE.reconcile_checkpoint_for_resume( + invocation=dict(invocation), + checkpoint=dict(resume_checkpoint), + live_head=live_head, + live_base_sha=state.current_base_sha, + live_worktree_status=live_status, + lock_busy=False, + ) + except LE.LocalExecutionError as exc: + reason = ( + "candidate_busy" + if isinstance(exc, LE.CandidateBusyError) + else "checkpoint_mismatch" + ) + state.resume_status = "resume_unavailable" + state.head_history = [state.initial_head] + state.base_revision_history = [ + {"ref": state.base_ref, "sha": state.current_base_sha} + ] + return _finalize( + state, + terminal_state="blocked", + reason=reason, + operator_action=f"resume failed: {exc}", + phase="resolve", + ) + state.resume_status = "resumed" + state.current_head = resume_checkpoint["current_head"] + state.cycle_attempts = list(resume_checkpoint["cycle_attempts"]) + state.head_history = list(resume_checkpoint["head_history"]) + state.base_revision_history = list( + resume_checkpoint["base_revision_history"] + ) + state.review_records = list(resume_checkpoint["review_records"]) + state.validation_outcomes = list(resume_checkpoint["validation_outcomes"]) + state.preserved_failed_attempts = list( + resume_checkpoint["preserved_failed_attempts"] + ) + # Reconstruct the terminal result's top-level `finding_dispositions` + # from checkpoint history, oldest review pass first. An "accepted" + # review-record disposition only becomes a reportable "selected" + # entry once a matching `committed` cycle attempt actually + # produced a fix commit for it (schema requires `selected` to + # carry `fix_commit_sha`); an accepted-but-not-yet-committed + # finding was interrupted before it resolved, so this run will + # decide and attempt it again rather than reporting it + # prematurely. `record_finding_disposition` always replaces any + # prior entry for the same finding_id, so iterating oldest first + # naturally leaves the most recent disposition per finding as the + # final reconstructed state — a finding declined on an earlier + # pass and later accepted-and-fixed is never left carrying both a + # stale `declined` entry and its real `selected` one. + committed_fix_by_finding = { + attempt["finding_id"]: attempt["resulting_head"] + for attempt in state.cycle_attempts + if attempt.get("outcome") == "committed" and attempt.get("finding_id") + } + for record in state.review_records: + for disposition in record["finding_dispositions"]: + finding_id = disposition["finding_id"] + if disposition["disposition"] == "accepted": + fix_commit_sha = committed_fix_by_finding.get(finding_id) + if fix_commit_sha is None: + continue + state.record_finding_disposition( + finding_id=finding_id, + disposition="selected", + rationale=disposition["rationale"], + fix_commit_sha=fix_commit_sha, + ) + else: + state.record_finding_disposition( + finding_id=finding_id, + disposition="declined", + rationale=disposition["rationale"], + ) + + recovered = LE.recover_interrupted_attempts( + repo=repo, + invocation_id=invocation["invocation_id"], + checkpoint=dict(resume_checkpoint), + ) + for leftover in recovered: + if leftover.already_promoted: + continue + handle = LE.AttemptHandle( + path=leftover.worktree_path or (attempts_root / leftover.branch), + branch=leftover.branch, + base_sha=leftover.base_sha, + ) + preserved = LE.discard_attempt( + common_dir=common_dir, + handle=handle, + attempt_sha=leftover.attempt_sha, + reason="recovered interrupted attempt from a prior invocation run", + ) + state.preserved_failed_attempts.append(_checkpoint_preserved(preserved)) + state.cycle_attempts.append( + { + "sequence": len(state.cycle_attempts) + 1, + "started_from_head": leftover.base_sha, + "outcome": "interrupted", + } + ) + else: + if state.current_head != state.initial_head: + return _finalize( + state, + terminal_state="blocked", + reason="candidate_integrity_failure", + operator_action=( + f"live head {state.current_head} does not match invocation " + f"candidate.head_sha {state.initial_head}" + ), + phase="resolve", + ) + state.head_history = [state.initial_head] + state.base_revision_history = [ + {"ref": state.base_ref, "sha": state.current_base_sha} + ] + + _persist_checkpoint( + state, + phase="establish_evidence", + next_action="run the next complete review", + ) + + review_sequence = len(state.review_records) + 1 + attempt_sequence = state.consumed_cycles() + 1 + # Only reconstructable from checkpoint `finding_dispositions` on resume + # (the checkpoint schema does not retain a pass's full raw finding + # set), so expanding/oscillation detection is best-effort across a + # resume boundary and exact within one continuous run. + previous_gating_ids: list[frozenset[str]] = [ + frozenset( + disposition["finding_id"] + for disposition in record["finding_dispositions"] + ) + for record in state.review_records + ] + + pending_finding: dict[str, Any] | None = None + pending_from_review = False + pending_decision: FixDecision | None = None + + while True: + if pending_finding is None: + validation_outcomes = _run_validation_suite( + invocation, cwd=state.repo, run_validation=run_validation + ) + unavailable_scope = _unavailable_scope(validation_outcomes) + if unavailable_scope is not None: + state.validation_outcomes = validation_outcomes + return _finalize( + state, + terminal_state="blocked", + reason="validation_unavailable", + operator_action=( + f"the {unavailable_scope} validation command is unavailable; " + "an operator must repair the environment before this " + "invocation can continue" + ), + phase="establish_evidence", + ) + + failed = _failed_outcome(validation_outcomes) + if failed is not None: + synthetic = classify_validation_failure( + outcome=ValidationOutcome( + status="failed", result=failed.get("result") + ), + invocation=invocation, + ) + state.validation_outcomes = validation_outcomes + if synthetic is None: + return _finalize( + state, + terminal_state="changes_remaining", + reason="current_candidate_validation_failure", + operator_action=( + f"the {failed['scope']} validation command " + f"{failed['command']!r} fails and no tractable " + "correction was identified; an operator must repair it" + ), + phase="decide", + ) + pending_finding = synthetic + pending_from_review = False + else: + state.validation_outcomes = validation_outcomes + diff = _current_diff( + state.repo, state.current_base_sha, state.current_head + ) + packet = _build_packet( + invocation, + head_sha=state.current_head, + comparison_base_sha=state.current_base_sha, + diff=diff, + validation_outcomes=validation_outcomes, + ) + mode = ORCH.resolve_review_execution_mode( + invocation["review_execution"]["mode"], + override_authorization=invocation["review_execution"].get( + "override_authorization" + ), + host_supports_fresh_subagent=host_supports_fresh_subagent, + ) + if mode["blocked_reason"] is not None: + return _finalize( + state, + terminal_state="blocked", + reason="missing_capability", + operator_action=( + "this host cannot run a fresh read-only reviewer " + "subagent and the invocation carries no explicit " + "in_agent_override" + ), + phase="review", + ) + independence = mode["independence"] + briefing = ORCH.build_reviewer_briefing( + independence=independence, + head_sha=state.current_head, + comparison_base_sha=state.current_base_sha, + ) + before = _worktree_snapshot(state.repo) + review_pass = reviewer( + packet=packet, + briefing=briefing, + head_sha=state.current_head, + comparison_base_sha=state.current_base_sha, + independence=independence, + sequence=review_sequence, + ) + after = _worktree_snapshot(state.repo) + mutation_attempts = list( + ORCH.detect_worktree_mutation(before, after) + ) + list(review_pass.mutation_attempts) + reviewer_identity = ORCH.generate_reviewer_identity( + independence, + review_sequence, + explicit=review_pass.reviewer_identity, + ) + try: + record = ORCH.build_review_record( + sequence=review_sequence, + packet=packet, + result=review_pass.result, + expected_head=state.current_head, + expected_base=state.current_base_sha, + independence=independence, + reviewer_identity=reviewer_identity, + mutation_attempts=mutation_attempts, + ) + except ORCH.ReviewIntegrityError as exc: + return _finalize( + state, + terminal_state="blocked", + reason="reviewer_integrity_failure", + operator_action=f"the review pass could not be trusted: {exc}", + phase="review", + ) + + if mutation_attempts: + state.review_records.append(record) + return _finalize( + state, + terminal_state="blocked", + reason="reviewer_integrity_failure", + operator_action=( + "the reviewer attempted a prohibited mutation: " + + "; ".join(mutation_attempts) + ), + phase="review", + ) + + result = review_pass.result + if record["aggregate_verdict"] == "blocked": + state.review_records.append(record) + return _finalize( + state, + terminal_state="blocked", + reason="missing_capability", + operator_action=( + "the review pass itself returned blocked: " + f"{result.get('next_action', '')}" + ), + phase="review", + ) + + if record["aggregate_verdict"] == "clean": + state.review_records.append(record) + _persist_checkpoint( + state, phase="return", next_action="none; converged" + ) + converged_created_commits = state.head_history[1:] + return _finalize( + state, + terminal_state="converged", + reason=None, + operator_action=( + "publish the retained local commit(s) through your " + "existing PR or merge workflow; review-fix-loop " + "performs no remote write under local_commit" + if converged_created_commits + else "no changes were required; nothing to publish" + ), + phase="return", + ) + + gating_ids = frozenset( + finding["id"] + for finding in ORCH.normalize_findings(result["findings"]) + if finding["severity"] in ORCH.GATING_SEVERITIES + ) + if ( + previous_gating_ids + and len(gating_ids) > len(previous_gating_ids[-1]) + and previous_gating_ids[-1] <= gating_ids + ): + state.review_records.append(record) + return _finalize( + state, + terminal_state="changes_remaining", + reason="expanding_findings", + operator_action=( + "a prior fix introduced additional gating findings; " + "an operator must review the remaining scope before " + "further automatic remediation" + ), + phase="decide", + ) + # A genuine A,B,A oscillation: the set changed from the + # immediately preceding pass but matches the pass before + # that. A finding set that simply survives unchanged + # (A,A,A) is not oscillation — it is the ordinary + # "repeated finding" case the design tolerates within + # budget, not an early stop. + if ( + len(previous_gating_ids) >= 2 + and previous_gating_ids[-1] != gating_ids + and previous_gating_ids[-2] == gating_ids + ): + state.review_records.append(record) + return _finalize( + state, + terminal_state="changes_remaining", + reason="oscillation", + operator_action=( + "the same finding set is oscillating across review " + "passes without converging; an operator must break " + "the cycle" + ), + phase="decide", + ) + previous_gating_ids.append(gating_ids) + + selected = ORCH.select_next_finding(result["findings"]) + state.review_records.append(record) + review_sequence += 1 + + if selected is None: + _persist_checkpoint( + state, phase="decide", next_action="operator input required" + ) + return _finalize( + state, + terminal_state="blocked", + reason="operator_input_required", + operator_action=( + "only deferred findings remain; an operator must " + "decide whether to accept, escalate, or authorize " + "further work" + ), + phase="decide", + ) + pending_finding = selected + pending_from_review = True + + finding = pending_finding + + if pending_decision is None: + decision = decide( + finding=finding, + change_contract=invocation["change_contract"], + attempt_number=attempt_sequence, + ) + disposition_record = { + "finding_id": finding["id"], + "disposition": decision.disposition, + "rationale": decision.rationale, + } + if pending_from_review and state.review_records: + state.review_records[-1]["finding_dispositions"].append( + disposition_record + ) + + if decision.disposition != "accepted": + state.record_finding_disposition( + finding_id=finding["id"], + disposition="declined", + rationale=decision.rationale, + ) + _persist_checkpoint( + state, phase="decide", next_action="operator input required" + ) + return _finalize( + state, + terminal_state="blocked", + reason="operator_input_required", + operator_action=( + f"finding {finding['id']} was {decision.disposition} " + f"({decision.rationale}); an operator must decide how " + "to proceed since it was not fixed" + ), + phase="decide", + ) + + if decision.expands_scope: + _persist_checkpoint( + state, phase="decide", next_action="scope decision required" + ) + return _finalize( + state, + terminal_state="blocked", + reason="scope_decision_required", + operator_action=( + f"fixing {finding['id']} would expand beyond " + f"{invocation['change_contract']['allowed_remediation_scope']!r}; " + "an operator must authorize the expanded scope" + ), + phase="decide", + ) + if decision.operator_input_required: + _persist_checkpoint( + state, phase="decide", next_action="operator input required" + ) + return _finalize( + state, + terminal_state="blocked", + reason="operator_input_required", + operator_action=( + f"finding {finding['id']} requires operator input " + "before a fix can be attempted" + ), + phase="decide", + ) + pending_decision = decision + + decision = pending_decision + + if state.remaining_cycles() <= 0: + state.unresolved_or_deferred.append( + f"{finding['id']}: accepted but the fix-cycle budget is exhausted" + ) + _persist_checkpoint( + state, + phase="fix", + next_action="operator must authorize a new budget", + ) + return _finalize( + state, + terminal_state="changes_remaining", + reason="cycle_budget_exhausted", + operator_action=( + f"{finding['id']} remains after consuming every fix cycle; " + "an operator must resolve it directly or authorize a new " + "invocation with a fresh budget" + ), + phase="fix", + ) + + started_from_head = state.current_head + sequence = attempt_sequence + attempt_sequence += 1 + attempt = LE.create_attempt( + repo=state.repo, + attempts_root=attempts_root, + base_sha=started_from_head, + invocation_id=invocation["invocation_id"], + sequence=sequence, + ) + commit_message = apply_fix( + finding=finding, + attempt_path=attempt.path, + change_contract=invocation["change_contract"], + attempt_number=sequence, + ) + attempt_validation = _run_validation_suite( + invocation, cwd=attempt.path, run_validation=run_validation + ) + attempt_failed = bool( + _failed_outcome(attempt_validation) + or _unavailable_scope(attempt_validation) + ) + + if attempt_failed: + preserved = LE.discard_attempt( + common_dir=common_dir, + handle=attempt, + attempt_sha=None, + reason=f"validation failed while fixing {finding['id']}", + ) + state.preserved_failed_attempts.append(_checkpoint_preserved(preserved)) + previous_attempt = ( + state.cycle_attempts[-1] if state.cycle_attempts else None + ) + consecutive_failure = ( + previous_attempt is not None + and previous_attempt["outcome"] == "failed" + and previous_attempt["started_from_head"] == started_from_head + ) + state.cycle_attempts.append( + { + "sequence": sequence, + "started_from_head": started_from_head, + "outcome": "failed", + "finding_id": finding["id"], + } + ) + if consecutive_failure: + _persist_checkpoint( + state, phase="fix", next_action="operator input required" + ) + return _finalize( + state, + terminal_state="changes_remaining", + reason="repeated_failed_attempt", + operator_action=( + f"two consecutive fix attempts for {finding['id']} both " + "failed validation without any progress; an operator " + "must intervene" + ), + phase="fix", + ) + _persist_checkpoint( + state, phase="fix", next_action="retry the fix from the same head" + ) + # Retry the same accepted finding from the same head: no head + # change occurred, so no fresh review is needed before retrying. + continue + + new_sha = LE.commit_attempt(attempt, commit_message) + new_head = LE.promote_attempt( + canonical_worktree=state.repo, + canonical_branch=state.branch, + attempt_sha=new_sha, + expected_old_head=started_from_head, + ) + state.cycle_attempts.append( + { + "sequence": sequence, + "started_from_head": started_from_head, + "outcome": "committed", + "resulting_head": new_head, + "finding_id": finding["id"], + } + ) + state.record_finding_disposition( + finding_id=finding["id"], + disposition="selected", + rationale=decision.rationale, + fix_commit_sha=new_head, + ) + state.current_head = new_head + state.head_history.append(new_head) + pending_finding = None + pending_decision = None + _persist_checkpoint( + state, + phase="invalidate_and_repeat", + next_action="run a fresh complete review", + ) + finally: + lock_cm.__exit__(None, None, None) diff --git a/skills/review-fix-loop/scripts/tests/test_local_commit.py b/skills/review-fix-loop/scripts/tests/test_local_commit.py new file mode 100644 index 0000000..0dfdb21 --- /dev/null +++ b/skills/review-fix-loop/scripts/tests/test_local_commit.py @@ -0,0 +1,1102 @@ +"""End-to-end tests for the standalone `local_commit` workflow (issue #99). + +Every test drives `local_commit.run_local_commit` against a real temporary Git +repository (matching `test_local_execution.py`'s "no mocked Git state" +convention) with small, deterministic fakes for the two genuinely host-boundary +actions the design assigns to the executing agent: running one +`review-code-change` pass and writing a fix's content. The fake reviewer +inspects the real repository content at the exact head it is asked to review +(`marker.txt`) rather than counting calls, so the same fake works across every +scenario and the review verdict is always a real function of real repository +state. + +Covers the ticket's required end-to-end fixtures: immediate convergence, one +or more fix cycles, budget exhaustion, validation failure (both the +"unavailable" and "no tractable correction" shapes), operator input +(rejected/deferred disposition and scope expansion), and recovery from an +interrupted attempt. +""" + +from __future__ import annotations + +import importlib.util +import sys +import tempfile +import unittest +from pathlib import Path +from typing import Any + +SKILL_ROOT = Path(__file__).resolve().parents[2] + + +def _load(name: str, filename: str): + spec = importlib.util.spec_from_file_location( + name, SKILL_ROOT / "scripts" / filename + ) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +LC = _load("review_fix_loop_local_commit", "local_commit.py") +LE = _load("review_fix_loop_local_execution_for_tests", "local_execution.py") +VALIDATE = _load("review_fix_loop_validate_for_tests", "validate.py") + + +# --------------------------------------------------------------------------- +# Repository fixtures +# --------------------------------------------------------------------------- + + +def init_repo(path: Path) -> None: + path.mkdir(parents=True, exist_ok=True) + LE.git("init", "-q", "-b", "main", cwd=path) + LE.git("config", "user.email", "test@example.com", cwd=path) + LE.git("config", "user.name", "Test", cwd=path) + (path / "README.md").write_text("initial\n") + LE.git("add", "-A", cwd=path) + LE.git("commit", "-q", "-m", "initial commit", cwd=path) + + +def start_candidate( + repo: Path, + *, + branch: str = "fix/99-example", + marker: str = "broken", + validation_flag: str = "pass", +) -> tuple[str, str]: + """Create `branch` off `main` with one commit adding the two control + files this test suite's fakes read. Returns `(base_sha, head_sha)`.""" + base_sha = LE.current_head(repo) + LE.git("checkout", "-q", "-b", branch, cwd=repo) + (repo / "marker.txt").write_text(marker + "\n") + (repo / "validation_flag.txt").write_text(validation_flag + "\n") + LE.git("add", "-A", cwd=repo) + LE.git("commit", "-q", "-m", "start candidate", cwd=repo) + head_sha = LE.current_head(repo) + return base_sha, head_sha + + +ALWAYS_PASS_VALIDATION = [ + {"name": "focused unit test", "command": "true", "scope": "focused"}, + {"name": "full repository gate", "command": "true", "scope": "full"}, +] + +FLAG_GATED_VALIDATION = [ + { + "name": "focused unit test", + "command": ( + 'python3 -c "import pathlib,sys; ' + "sys.exit(0 if pathlib.Path('validation_flag.txt').read_text().strip()" + "=='pass' else 1)\"" + ), + "scope": "focused", + }, + {"name": "full repository gate", "command": "true", "scope": "full"}, +] + +# `marker.txt` also drives the fake reviewer (below); this command only fails +# for the literal sentinel `trigger-fail`, so it never fires for the ordinary +# 'broken'/'fixed' content the reviewer itself reacts to. +NOT_TRIGGER_FAIL_VALIDATION = [ + { + "name": "focused unit test", + "command": ( + 'python3 -c "import pathlib,sys; ' + "sys.exit(1 if pathlib.Path('marker.txt').read_text().strip()" + "=='trigger-fail' else 0)\"" + ), + "scope": "focused", + }, + {"name": "full repository gate", "command": "true", "scope": "full"}, +] + + +def make_invocation( + repo: Path, + *, + branch: str, + base_sha: str, + head_sha: str, + invocation_id: str = "local-commit-test", + max_fix_cycles: int = 3, + validation: list[dict[str, str]] | None = None, +) -> dict[str, Any]: + common_dir = LE.git_common_dir(repo) + diff = LE.git("diff", base_sha, head_sha, cwd=repo).stdout + worktree = LE.worktree_status(repo) + return { + "schema_version": "1.0", + "invocation_id": invocation_id, + "repository": { + "identity": "shaug/agent-scripts", + "git_common_directory": str(common_dir), + }, + "candidate": { + "branch": branch, + "head_sha": head_sha, + "comparison_base": {"ref": "main", "sha": base_sha}, + "diff": {"format": "unified_diff", "complete": True, "content": diff}, + "worktree": worktree, + "all_changes_committed": True, + "source_unavailable_reason": "standalone invocation has no recorded pushable source", + }, + "change_contract": { + "goal": "Fix the example.", + "acceptance_criteria": ["marker.txt reads 'fixed'"], + "non_goals": ["Unrelated refactors"], + "preserved_behaviors": ["Existing README content"], + "allowed_remediation_scope": "marker.txt only", + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [], + }, + }, + "review_execution": {"mode": "fresh_subagent"}, + "fix_cycle_budget": {"max_fix_cycles": max_fix_cycles}, + "validation": validation or ALWAYS_PASS_VALIDATION, + "publication": {"policy": "local_commit"}, + } + + +CLEAN_TEMPLATE = { + "schema_version": "1.4", + "lens": "aggregate", + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "validation_limitations": [], + "next_action": "No changes are required.", +} + +FINDING_ID = "correctness-001" + + +def _finding() -> dict[str, Any]: + return { + "id": FINDING_ID, + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "example rule", + "evidence": [ + {"location": "marker.txt:1", "detail": "marker.txt is not 'fixed'"} + ], + "concern": "marker.txt does not read 'fixed'", + "impact": "the candidate is incomplete", + "proposed_change": "write 'fixed' into marker.txt", + "expected_effect": "marker.txt reads 'fixed'", + } + + +def make_marker_reviewer(repo: Path): + """A fake reviewer whose verdict is a real function of `marker.txt`'s + content at the exact head it is asked to review: `clean` when it reads + 'fixed', `changes_required` with one `correctness-001` finding otherwise. + """ + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ) -> LC.ReviewPass: + del packet, briefing, independence, sequence + content = LE.git("show", f"{head_sha}:marker.txt", cwd=repo).stdout.strip() + candidate = {"head_sha": head_sha, "comparison_base_sha": comparison_base_sha} + if content == "fixed": + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "lens_executions": [ + { + "lens": lens, + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "clean", + "freshly_executed": True, + } + for lens in ( + "solution_simplicity", + "correctness", + "code_simplicity", + ) + ], + } + else: + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "verdict": "changes_required", + "findings": [_finding()], + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "clean", + "freshly_executed": True, + } + ], + "next_action": f"Fix {FINDING_ID}.", + } + return LC.ReviewPass(result=result) + + return reviewer + + +def fixing_apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract, attempt_number + (attempt_path / "marker.txt").write_text("fixed\n") + return f"fix: resolve {FINDING_ID}" + + +def ineffective_apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract + (attempt_path / "marker.txt").write_text(f"still-broken-{attempt_number}\n") + return f"fix attempt {attempt_number} for {FINDING_ID}" + + +def accepting_decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="accepted", rationale=f"{finding['id']} is tractable" + ) + + +class LocalCommitRepoTestCase(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.repo = Path(self.tmp.name) / "repo" + init_repo(self.repo) + + +class ImmediateConvergenceTests(LocalCommitRepoTestCase): + def test_clean_candidate_converges_without_any_fix_cycle(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertNotIn("reason", result) + self.assertEqual(result["head"]["initial"], head_sha) + self.assertEqual(result["head"]["final"], head_sha) + self.assertEqual(result["created_commits"], []) + self.assertEqual(result["budget"]["consumed_cycles"], 0) + self.assertFalse(result["acceptance_reconciliation_required"]) + self.assertEqual( + result["publication"], + { + "policy": "local_commit", + "status": "not_applicable", + "non_converged_exposure": False, + }, + ) + self.assertEqual(len(result["review_records"]), 1) + self.assertEqual(result["review_records"][0]["aggregate_verdict"], "clean") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +class FixCycleTests(LocalCommitRepoTestCase): + def test_one_fix_cycle_converges(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(result["head"]["initial"], head_sha) + self.assertNotEqual(result["head"]["final"], head_sha) + self.assertEqual(len(result["created_commits"]), 1) + self.assertEqual(result["budget"]["consumed_cycles"], 1) + self.assertEqual(result["budget"]["remaining_cycles"], 2) + self.assertTrue(result["acceptance_reconciliation_required"]) + self.assertEqual(result["unpushed_commits"], result["created_commits"]) + self.assertEqual( + result["finding_dispositions"], + [ + { + "finding_id": FINDING_ID, + "disposition": "selected", + "rationale": f"{FINDING_ID} is tractable", + "fix_commit_sha": result["created_commits"][0], + } + ], + ) + # Two review passes: the initial non-clean one and the post-fix clean one. + self.assertEqual(len(result["review_records"]), 2) + self.assertEqual( + result["review_records"][0]["aggregate_verdict"], "changes_required" + ) + self.assertEqual(result["review_records"][1]["aggregate_verdict"], "clean") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + # The canonical worktree actually advanced to the promoted commit. + self.assertEqual(LE.current_head(self.repo), result["head"]["final"]) + self.assertEqual((self.repo / "marker.txt").read_text().strip(), "fixed") + self.assertTrue(LE.is_clean(LE.worktree_status(self.repo))) + + def test_multiple_fix_cycles_before_convergence(self): + """The fake reviewer only reports `clean` once marker.txt reads + 'fixed'; a fixer that takes two attempts to actually write that + content demonstrates more than one committed fix cycle.""" + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + max_fix_cycles=5, + ) + + attempts_seen: list[int] = [] + + def two_step_apply_fix( + *, finding, attempt_path, change_contract, attempt_number + ): + del finding, change_contract + attempts_seen.append(attempt_number) + content = "fixed" if len(attempts_seen) >= 2 else "getting-there" + (attempt_path / "marker.txt").write_text(content + "\n") + return f"fix attempt {attempt_number}" + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=two_step_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(len(result["created_commits"]), 2) + self.assertEqual(result["budget"]["consumed_cycles"], 2) + self.assertEqual(len(result["review_records"]), 3) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +class BudgetExhaustionTests(LocalCommitRepoTestCase): + def test_ineffective_fixes_exhaust_the_budget(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + max_fix_cycles=2, + ) + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=ineffective_apply_fix, + ) + self.assertEqual(result["terminal_state"], "changes_remaining") + self.assertEqual(result["reason"], "cycle_budget_exhausted") + self.assertEqual(result["budget"]["consumed_cycles"], 2) + self.assertEqual(result["budget"]["remaining_cycles"], 0) + self.assertEqual(len(result["created_commits"]), 2) + self.assertEqual(result["unpushed_commits"], result["created_commits"]) + self.assertEqual(result["publication"]["status"], "not_applicable") + self.assertIn(FINDING_ID, result["unresolved_or_deferred_findings"][0]) + self.assertTrue(result["operator_action"]) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + # The candidate itself is preserved at its last committed head, not lost. + self.assertEqual(LE.current_head(self.repo), result["head"]["final"]) + + +FINDING_ID_2 = "correctness-002" + + +def _finding2() -> dict[str, Any]: + return { + "id": FINDING_ID_2, + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "example rule", + "evidence": [ + {"location": "other_file.txt:1", "detail": "other_file.txt reads 'broken'"} + ], + "concern": "other_file.txt does not read 'ok'", + "impact": "a second defect is present", + "proposed_change": "write 'ok' into other_file.txt", + "expected_effect": "other_file.txt reads 'ok'", + } + + +def make_two_finding_reviewer(repo: Path): + """A fake reviewer gating on two independent tracked files: `marker.txt` + (finding `correctness-001`) and `other_file.txt` (finding + `correctness-002`, treated as absent/resolved when the file does not yet + exist at the reviewed head). Lets a test drive expanding or oscillating + finding sets across cycles by controlling both files independently. + """ + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ) -> LC.ReviewPass: + del packet, briefing, independence, sequence + marker = LE.git("show", f"{head_sha}:marker.txt", cwd=repo).stdout.strip() + other = LE.git("show", f"{head_sha}:other_file.txt", cwd=repo, check=False) + other_content = other.stdout.strip() if other.returncode == 0 else "ok" + findings = [] + if marker != "fixed": + findings.append(_finding()) + if other_content == "broken": + findings.append(_finding2()) + candidate = {"head_sha": head_sha, "comparison_base_sha": comparison_base_sha} + if not findings: + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "lens_executions": [ + { + "lens": lens, + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "clean", + "freshly_executed": True, + } + for lens in ( + "solution_simplicity", + "correctness", + "code_simplicity", + ) + ], + } + else: + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "verdict": "changes_required", + "findings": findings, + "lens_executions": [ + { + "lens": "solution_simplicity", + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "clean", + "freshly_executed": True, + } + ], + "next_action": "Fix the reported findings.", + } + return LC.ReviewPass(result=result) + + return reviewer + + +class StopConditionTests(LocalCommitRepoTestCase): + def test_a_fix_that_introduces_a_new_finding_stops_as_expanding_findings(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + max_fix_cycles=3, + ) + + def apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract, attempt_number + # Leave marker.txt (correctness-001) unresolved and additionally + # introduce a second, independent defect. + (attempt_path / "other_file.txt").write_text("broken\n") + return "fix: (ineffectively) touch other_file.txt too" + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_two_finding_reviewer(self.repo), + decide=accepting_decide, + apply_fix=apply_fix, + ) + self.assertEqual(result["terminal_state"], "changes_remaining") + self.assertEqual(result["reason"], "expanding_findings") + self.assertEqual(result["budget"]["consumed_cycles"], 1) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_oscillating_finding_sets_stop_as_oscillation(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + max_fix_cycles=5, + ) + + def apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract + if attempt_number % 2 == 1: + (attempt_path / "marker.txt").write_text("fixed\n") + (attempt_path / "other_file.txt").write_text("broken\n") + else: + (attempt_path / "marker.txt").write_text("broken\n") + (attempt_path / "other_file.txt").write_text("ok\n") + return f"fix attempt {attempt_number}: swap which defect is present" + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_two_finding_reviewer(self.repo), + decide=accepting_decide, + apply_fix=apply_fix, + ) + self.assertEqual(result["terminal_state"], "changes_remaining") + self.assertEqual(result["reason"], "oscillation") + self.assertEqual(result["budget"]["consumed_cycles"], 2) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_two_consecutive_failed_attempts_stop_as_repeated_failed_attempt(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + max_fix_cycles=3, + validation=NOT_TRIGGER_FAIL_VALIDATION, + ) + + def always_fails_validation_apply_fix( + *, finding, attempt_path, change_contract, attempt_number + ): + del finding, change_contract, attempt_number + (attempt_path / "marker.txt").write_text("trigger-fail\n") + return "fix attempt that never actually validates" + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=always_fails_validation_apply_fix, + ) + self.assertEqual(result["terminal_state"], "changes_remaining") + self.assertEqual(result["reason"], "repeated_failed_attempt") + self.assertEqual(result["budget"]["consumed_cycles"], 2) + self.assertEqual(result["created_commits"], []) + self.assertEqual(len(result["preserved_failed_attempts"]), 2) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + # Canonical candidate untouched: both attempts failed validation and + # were discarded rather than promoted. + self.assertEqual(LE.current_head(self.repo), head_sha) + self.assertTrue(LE.is_clean(LE.worktree_status(self.repo))) + + +class RegressionTests(LocalCommitRepoTestCase): + def test_resumed_acceptance_replaces_a_stale_declined_disposition(self): + """A finding declined on one pass and later accepted-and-fixed after + a resumed run with a different decision must not leave both a stale + `declined` entry and the real `selected` entry in the same result.""" + base_sha, head_sha = start_candidate(self.repo, marker="broken") + branch = "fix/99-example" + invocation_id = "local-commit-regression-test" + invocation = make_invocation( + self.repo, + branch=branch, + base_sha=base_sha, + head_sha=head_sha, + invocation_id=invocation_id, + ) + + def declining_decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="rejected", + rationale=f"{finding['id']} looked out of scope on first review", + ) + + first = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=declining_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(first["terminal_state"], "blocked") + self.assertEqual(first["reason"], "operator_input_required") + + common_dir = LE.git_common_dir(self.repo) + checkpoint_path = LE.checkpoint_path(common_dir, invocation_id) + checkpoint = LE.read_checkpoint(checkpoint_path) + + second = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + resume_checkpoint=checkpoint, + ) + self.assertEqual(second["terminal_state"], "converged") + self.assertEqual(second["resume_status"], "resumed") + # Exactly one disposition for this finding_id, and it is the real, + # current one — not both a stale `declined` and the new `selected`. + matching = [ + entry + for entry in second["finding_dispositions"] + if entry["finding_id"] == FINDING_ID + ] + self.assertEqual(len(matching), 1) + self.assertEqual(matching[0]["disposition"], "selected") + self.assertIn("fix_commit_sha", matching[0]) + # No longer reported as unresolved/deferred now that it was fixed. + self.assertEqual(second["unresolved_or_deferred_findings"], []) + self.assertEqual(VALIDATE.validate_terminal_result(second), []) + + +class SourceBindingTests(LocalCommitRepoTestCase): + def test_bound_source_reports_ahead_behind_counts_instead_of_unavailable(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + invocation["candidate"]["source_binding"] = { + "repository": "shaug/agent-scripts", + "remote_url": "git@github.com:shaug/agent-scripts.git", + "ref": "refs/heads/fix/99-example", + "observed_object_id": base_sha, + } + del invocation["candidate"]["source_unavailable_reason"] + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(result["source"]["status"], "bound") + self.assertEqual(result["source"]["initial_head"], base_sha) + self.assertEqual(result["source"]["final_head"], base_sha) + # The candidate head is exactly one commit ahead of the source + # (the "start candidate" commit) and zero behind. + self.assertEqual(result["source"]["ahead_by"], 1) + self.assertEqual(result["source"]["behind_by"], 0) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_source_unavailable_reason_preserved_without_a_binding(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["source"]["status"], "unavailable") + self.assertEqual( + result["source"]["unavailable_reason"], + "standalone invocation has no recorded pushable source", + ) + + +class ValidationFailureTests(LocalCommitRepoTestCase): + def test_untractable_validation_failure_reports_changes_remaining(self): + base_sha, head_sha = start_candidate( + self.repo, marker="fixed", validation_flag="fail" + ) + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + validation=FLAG_GATED_VALIDATION, + ) + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "changes_remaining") + self.assertEqual(result["reason"], "current_candidate_validation_failure") + self.assertEqual(result["budget"]["consumed_cycles"], 0) + self.assertEqual(result["review_records"], []) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_unavailable_validation_command_blocks(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + + def unavailable_run_validation(*, name, command, scope, cwd): + if scope == "full": + return LC.ValidationOutcome( + status="unavailable", reason="the full-gate tool is not installed" + ) + return LC.default_run_validation( + name=name, command=command, scope=scope, cwd=cwd + ) + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + run_validation=unavailable_run_validation, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "validation_unavailable") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_tractable_validation_failure_is_fixed_via_synthetic_finding(self): + base_sha, head_sha = start_candidate( + self.repo, marker="fixed", validation_flag="fail" + ) + invocation = make_invocation( + self.repo, + branch="fix/99-example", + base_sha=base_sha, + head_sha=head_sha, + validation=FLAG_GATED_VALIDATION, + ) + + def classify(*, outcome, invocation): + del outcome, invocation + return { + "id": "validation-flag-001", + "lens": "validation", + "severity": "blocking", + "confidence": "high", + "rule": "validation must pass", + "evidence": [ + {"location": "validation_flag.txt:1", "detail": "reads 'fail'"} + ], + "concern": "validation_flag.txt disables the focused check", + "impact": "the focused validation command fails", + "proposed_change": "write 'pass' into validation_flag.txt", + "expected_effect": "the focused validation command passes", + } + + def apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract, attempt_number + (attempt_path / "validation_flag.txt").write_text("pass\n") + return "fix: repair validation_flag.txt" + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=apply_fix, + classify_validation_failure=classify, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(result["budget"]["consumed_cycles"], 1) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +class OperatorInputTests(LocalCommitRepoTestCase): + def test_declined_finding_blocks_on_operator_input_and_stays_visible(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + + def declining_decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="rejected", + rationale=f"{finding['id']} is already addressed upstream", + ) + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=declining_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "operator_input_required") + self.assertEqual(result["budget"]["consumed_cycles"], 0) + self.assertEqual( + result["unresolved_or_deferred_findings"], + [f"{FINDING_ID}: {FINDING_ID} is already addressed upstream"], + ) + # The decline and its rationale are visible in the per-review record too. + self.assertEqual( + result["review_records"][0]["finding_dispositions"], + [ + { + "finding_id": FINDING_ID, + "disposition": "rejected", + "rationale": f"{FINDING_ID} is already addressed upstream", + } + ], + ) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_scope_expanding_fix_blocks_on_scope_decision(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + + def expanding_decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="accepted", + rationale=f"{finding['id']} needs a broader change", + expands_scope=True, + ) + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=expanding_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "scope_decision_required") + self.assertEqual(result["budget"]["consumed_cycles"], 0) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +class RecoveryTests(LocalCommitRepoTestCase): + def test_resumes_and_discards_an_interrupted_attempt_then_converges(self): + base_sha, head_sha = start_candidate(self.repo, marker="broken") + branch = "fix/99-example" + invocation_id = "local-commit-recovery-test" + invocation = make_invocation( + self.repo, + branch=branch, + base_sha=base_sha, + head_sha=head_sha, + invocation_id=invocation_id, + ) + common_dir = LE.git_common_dir(self.repo) + attempts_root = LE.default_attempts_root(common_dir) + + # Simulate a crash: an isolated attempt was created and committed but + # never promoted, and no checkpoint ever recorded its reservation. + interrupted = LE.create_attempt( + repo=self.repo, + attempts_root=attempts_root, + base_sha=head_sha, + invocation_id=invocation_id, + sequence=1, + ) + (interrupted.path / "marker.txt").write_text("half-fixed\n") + LE.commit_attempt(interrupted, "in-flight fix, never promoted") + self.assertEqual(LE.current_head(self.repo), head_sha) # canonical untouched + + checkpoint = { + "schema_version": "1.0", + "invocation_id": invocation_id, + "repository": invocation["repository"], + "branch": branch, + "worktree": LE.worktree_status(self.repo), + "initial_head": head_sha, + "current_head": head_sha, + "comparison_base": {"ref": "main", "sha": base_sha}, + "publication": {"policy": "local_commit"}, + "original_cycle_budget": 3, + "cycle_attempts": [], + "head_history": [head_sha], + "base_revision_history": [{"ref": "main", "sha": base_sha}], + "review_records": [], + "validation_outcomes": [], + "preserved_failed_attempts": [], + "source": { + "status": "unavailable", + "unavailable_reason": "standalone invocation has no recorded pushable source", + }, + "current_phase": "fix", + "expected_next_action": "recover the interrupted attempt", + } + self.assertEqual(VALIDATE.validate_checkpoint(checkpoint), []) + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + resume_checkpoint=checkpoint, + ) + self.assertEqual(result["resume_status"], "resumed") + self.assertEqual(result["terminal_state"], "converged") + # The leftover attempt was discarded (preserved for inspection) and + # recorded as one interrupted cycle attempt, then a fresh committed + # attempt fixed the candidate for real. + self.assertEqual(len(result["preserved_failed_attempts"]), 1) + self.assertEqual( + result["budget"]["consumed_cycles"], 2 + ) # interrupted + committed + self.assertEqual(len(result["created_commits"]), 1) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + # The interrupted attempt's branch and worktree were cleaned up by + # `discard_attempt`'s caller responsibility is NOT implied — only the + # canonical worktree's cleanliness matters here. + self.assertTrue(LE.is_clean(LE.worktree_status(self.repo))) + + +class InputValidationTests(LocalCommitRepoTestCase): + def test_rejects_invalid_invocation(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + del invocation["fix_cycle_budget"] + with self.assertRaises(LC.LocalCommitError): + LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + + def test_rejects_update_pr_policy(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + invocation["publication"] = { + "policy": "update_pr", + "pull_request": { + "head_repository": "shaug/agent-scripts", + "head_ref": "refs/heads/fix/99-example", + "expected_old_head_sha": head_sha, + "base_ref": "main", + "base_sha": base_sha, + }, + } + invocation["candidate"]["source_binding"] = { + "repository": "shaug/agent-scripts", + "remote_url": "git@github.com:shaug/agent-scripts.git", + "ref": "refs/heads/fix/99-example", + "observed_object_id": head_sha, + } + del invocation["candidate"]["source_unavailable_reason"] + with self.assertRaises(LC.LocalCommitError): + LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + + def test_candidate_busy_when_lock_already_held(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + common_dir = LE.git_common_dir(self.repo) + with LE.acquire_candidate_locks(common_dir, "refs/heads/fix/99-example"): + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "candidate_busy") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +class ReviewerMutationTests(LocalCommitRepoTestCase): + def test_reviewer_mutation_attempt_blocks_with_reviewer_integrity_failure(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + + def mutating_reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + del packet, briefing, independence, sequence + (self.repo / "sneaky.txt").write_text( + "a reviewer should never write this\n" + ) + LE.git("add", "-A", cwd=self.repo) + candidate = { + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + } + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "lens_executions": [ + { + "lens": lens, + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "clean", + "freshly_executed": True, + } + for lens in ( + "solution_simplicity", + "correctness", + "code_simplicity", + ) + ], + } + return LC.ReviewPass(result=result) + + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=mutating_reviewer, + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "reviewer_integrity_failure") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + # Preserved for operator inspection, not silently cleaned up. + LE.git("reset", "-q", "--hard", "HEAD", cwd=self.repo) + LE.git("clean", "-fdq", cwd=self.repo) + + +class MissingCapabilityTests(LocalCommitRepoTestCase): + def test_no_fresh_subagent_and_no_override_blocks_with_missing_capability(self): + base_sha, head_sha = start_candidate(self.repo, marker="fixed") + invocation = make_invocation( + self.repo, branch="fix/99-example", base_sha=base_sha, head_sha=head_sha + ) + result = LC.run_local_commit( + invocation, + repo=self.repo, + reviewer=make_marker_reviewer(self.repo), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + host_supports_fresh_subagent=False, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "missing_capability") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +if __name__ == "__main__": + unittest.main()