diff --git a/CHANGELOG.md b/CHANGELOG.md index cf12948..ae72258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,25 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-30 — Defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety +## 2026-07-30 — Implemented the review-fix-loop 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): add `scripts/local_execution.py` implementing #97's + local execution substrate — non-blocking common-Git-common-directory candidate + locking (local-ref lock before the optional `update_pr` remote-target lock, + released in reverse order), isolated attempt worktrees created from the exact + canonical head, verified fast-forward-only canonical promotion that fails + closed and preserves the candidate on a dirty or advanced canonical worktree, + atomic schema-validated checkpoint persistence and resume reconciliation, + preserved failed-attempt artifacts, cleanup that only ever removes the + `review-fix-loop/attempt/` namespace it created, and recovery of an + interrupted attempt against a checkpoint's own history — with deterministic + tests against real temporary Git repositories covering contention, + interruption, stale state, dirty worktrees, promotion races, and cleanup + safety - fix(review-fix-loop): reject `converged` when any `review_records` entry — not only the final-head-bound one — recorded a mutation attempt, closing the gap the tenth (final) review-code-change pass on #96 found + (`0187dfc77444fbf410b5ed86a42a12e4d088e7b3`) - fix(review-fix-loop): add a per-pass `reviewer_identity` field to `review_records` in both checkpoint and terminal-result, and reject a dirty `candidate.worktree` (`staged`/`unstaged`/`untracked`) in diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index caf9f31..30d23cf 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 that later children of epic #95 use to run the standalone review-remediation workflow. Use when authoring, resuming, or terminating a review-fix-loop invocation to check its documents against the shared schemas before trusting or acting on them. This child does not yet run reviewers, apply fixes, acquire locks, manage worktrees, or publish anything; see design/review-fix-loop.md and references/CONTRACT.md for the full behavioral design and current implementation status. +description: Validate the review-fix-loop invocation, checkpoint, and terminal-result contracts, 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) that later children of epic #95 use to run the standalone review-remediation workflow. Use when authoring, resuming, or terminating a review-fix-loop invocation to check its documents against the shared schemas, or when acquiring a candidate lock, running an isolated remediation attempt, or recovering an interrupted one. This skill does not yet run reviewers, select or apply a fix's content, or publish anything; see design/review-fix-loop.md and references/CONTRACT.md for the full behavioral design and current implementation status. allowed-tools: Read, Bash --- @@ -12,21 +12,27 @@ review suite, applies material ticket-scoped fixes, and repeats until review converges or a bounded stop condition is reached. The full design is [`design/review-fix-loop.md`](../../design/review-fix-loop.md). -This child ([issue #96](https://github.com/shaug/agent-scripts/issues/96), the -first of epic [#95](https://github.com/shaug/agent-scripts/issues/95)) defines -and validates only the contracts later children build on: +Issue [#96](https://github.com/shaug/agent-scripts/issues/96) (the first child +of epic [#95](https://github.com/shaug/agent-scripts/issues/95)) defined and +validates the contracts every other child builds on: - the **invocation** a caller or standalone operator supplies to start or resume a loop; - the **durable checkpoint** the loop would record between phases; and - the **terminal result** the loop returns. -It does not run a reviewer, apply a fix, acquire a lock, manage a worktree, -recover from interruption, or publish anything — those behaviors belong to -issues #97 (local locking, isolated attempts, and recovery), #98 (reviewer -isolation and orchestration), #99 (`local_commit`), and #100 (`update_pr`). Do -not invoke this skill expecting an executable review-fix loop yet; use it to -validate a document you or a later child produced against the shared schemas. +Issue [#97](https://github.com/shaug/agent-scripts/issues/97) adds the local +execution substrate those contracts describe: common-Git-common-directory +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 run a reviewer, select or apply a fix's content, or +publish anything — those behaviors belong to issue #98 (reviewer isolation and +orchestration), #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, or to acquire +a lock, run an isolated attempt, and recover an interrupted one. ## Load the contracts @@ -73,11 +79,45 @@ these functions enforce beyond plain JSON Schema, and `scripts/tests/test_validate.py` for the complete valid, invalid, boundary, and round-trip case coverage. -## Non-goals of this child - -- Running a reviewer or applying a fix. -- Acquiring a local or remote-target lock, managing a worktree, or recovering an - interrupted attempt. +## Local execution + +`scripts/local_execution.py` is dependency-free and loads `scripts/validate.py` +from this same directory via `importlib` rather than duplicating any schema or +cross-field check, so a caller always resumes and promotes against the exact +contract `references/CONTRACT.md` defines. It implements the parts of +`design/review-fix-loop.md`'s "Local ownership and checkpointing" section this +repository can exercise without a reviewer or a selected fix: + +- `acquire_candidate_locks` — the non-blocking, common-Git-common-directory + local-ref lock plus the optional `update_pr` remote-target lock, acquired in + that fixed order and released in reverse, so conflicting local invocations can + never both own the same target and lock ordering cannot self-deadlock. +- `write_checkpoint_atomic`, `read_checkpoint`, and + `reconcile_checkpoint_for_resume` — durable, schema-validated checkpoint + persistence and the complete design-required resume precondition set (no + active lock holder, matching cross-document identity, a clean candidate, and + live head/base agreement). +- `create_attempt`, `commit_attempt`, `promote_attempt`, `discard_attempt`, and + `cleanup_attempt` — an isolated attempt worktree and branch created from the + exact canonical head, a verified fast-forward-only promotion that leaves the + canonical candidate untouched on any failure, and cleanup that only ever acts + on the `review-fix-loop/attempt/` namespace it created. +- `recover_interrupted_attempts` — reconciles attempt branches an interrupted + invocation left behind against a checkpoint's own history, returning each + uniquely identifiable leftover for the caller to retry or discard, and raising + rather than guessing when reconciliation is ambiguous. + +See the module's own docstrings and +[`scripts/tests/test_local_execution.py`](scripts/tests/test_local_execution.py) +for the complete contention, interruption, stale-state, dirty-worktree, +promotion-race, and cleanup-safety coverage. Selecting which finding to fix, +writing the fix's content, running a reviewer, and publishing to a remote remain +out of scope here; a caller supplies the fix content and invokes these +primitives around it. + +## Non-goals + +- Running a reviewer, or selecting or writing a fix's content. - Publishing anything, including the `update_pr` expected-old fast-forward update. - Migrating `implement-ticket`, `babysit-pr`, `carve-changesets`, or any other diff --git a/skills/review-fix-loop/scripts/local_execution.py b/skills/review-fix-loop/scripts/local_execution.py new file mode 100644 index 0000000..10eae63 --- /dev/null +++ b/skills/review-fix-loop/scripts/local_execution.py @@ -0,0 +1,775 @@ +#!/usr/bin/env python3 +"""Local execution substrate for `review-fix-loop`. + +Implements the parts of `design/review-fix-loop.md`'s "Local ownership and +checkpointing" section that issue #97 owns: common-git-common-directory +locking, isolated (git-worktree-based) remediation attempts, durable +checkpoint persistence and resume reconciliation, verified fast-forward-only +canonical promotion, and recovery of an interrupted attempt. It deliberately +does not run a reviewer, select a fix, or publish to a remote (issues #98, +#99, and #100). + +This module has no third-party dependencies, matching the convention used by +`scripts/validate.py` (the #96 contract leaf) and by the repository's +review-suite validator: a skill folder is the unit of distribution. It loads +`validate.py` from this same directory via `importlib` rather than +duplicating any of its schema or cross-field checks, per this ticket's +dependency to "build on top of it; do not duplicate or rewrite the existing +contract/validation code." + +## Where local state lives + +Every invocation-scoped file this module writes — locks, durable checkpoints, +and preserved failed-attempt artifacts — lives under +`/review-fix-loop/`. The common directory (typically +`.git`, or a linked worktree's pointer target) is shared by every worktree of +one repository and is never tracked by Git itself, so this satisfies the +design's "skill-local ignored directory" requirement without depending on any +one worktree's lifetime or `.gitignore` entries. Isolated attempt worktrees +default to `/review-fix-loop/attempts/`, but callers may +pass any `attempts_root`. +""" + +from __future__ import annotations + +import contextlib +import dataclasses +import errno +import fcntl +import hashlib +import importlib.util +import json +import os +import re +import subprocess +import tempfile +from pathlib import Path +from typing import Any, Iterator, Sequence + +HERE = Path(__file__).resolve().parent + +_VALIDATE_SPEC = importlib.util.spec_from_file_location( + "review_fix_loop_validate", HERE / "validate.py" +) +assert _VALIDATE_SPEC and _VALIDATE_SPEC.loader +validate = importlib.util.module_from_spec(_VALIDATE_SPEC) +_VALIDATE_SPEC.loader.exec_module(validate) + +ROOT_NAMESPACE = "review-fix-loop" +LOCK_SUBDIR = "locks" +CHECKPOINT_SUBDIR = "checkpoints" +PRESERVED_ATTEMPTS_SUBDIR = "preserved-attempts" +ATTEMPTS_SUBDIR = "attempts" + +ATTEMPT_BRANCH_PREFIX = "review-fix-loop/attempt/" + + +# --------------------------------------------------------------------------- +# Errors +# --------------------------------------------------------------------------- + + +class LocalExecutionError(RuntimeError): + """Base class for every local-execution failure in this module.""" + + +class CommandError(LocalExecutionError): + """A git or subprocess command failed.""" + + +class CandidateBusyError(LocalExecutionError): + """Another process already holds a required local-ref or remote-target lock.""" + + +class CandidateIntegrityFailureError(LocalExecutionError): + """Live Git state cannot be uniquely reconciled with recorded state.""" + + +class CheckpointMismatchError(LocalExecutionError): + """A checkpoint cannot be reconciled with its invocation or live state.""" + + +class InvalidCheckpointError(LocalExecutionError): + """A checkpoint document failed contract validation.""" + + +class StaleBaseError(LocalExecutionError): + """The canonical head advanced past an attempt's recorded base before promotion.""" + + +class DirtyWorktreeError(LocalExecutionError): + """A required worktree is not clean.""" + + +class UnsafeCleanupError(LocalExecutionError): + """Refused to remove a worktree or branch outside the attempt namespace.""" + + +# --------------------------------------------------------------------------- +# Git primitives +# --------------------------------------------------------------------------- + + +def run(cmd: Sequence[str], *, cwd: Path | str | None = None, check: bool = True): + """Run a command and return the completed process, mirroring the shared + `carve-changesets/scripts/common.py` convention used elsewhere in this + repository.""" + try: + result = subprocess.run( + list(cmd), + cwd=str(cwd) if cwd is not None else None, + text=True, + capture_output=True, + check=False, + ) + except FileNotFoundError as exc: + raise CommandError(f"command not found: {cmd[0]}") from exc + + if check and result.returncode != 0: + stderr = (result.stderr or "").strip() + stdout = (result.stdout or "").strip() + detail = stderr or stdout or f"exit code {result.returncode}" + raise CommandError(f"command failed: {' '.join(cmd)}\n{detail}") + return result + + +def git(*args: str, cwd: Path | str | None = None, check: bool = True): + return run(("git", *args), cwd=cwd, check=check) + + +def git_common_dir(repo_path: Path | str) -> Path: + """Resolve the absolute Git common directory for `repo_path`. + + Locks, checkpoints, and preserved attempts are keyed by this path so that + every worktree of one repository — canonical or attempt — contends on the + same state, per the design's "same Git common directory" requirement. + """ + output = git("rev-parse", "--git-common-dir", cwd=repo_path).stdout.strip() + path = Path(output) + if not path.is_absolute(): + path = Path(repo_path) / path + return path.resolve() + + +def repo_toplevel(repo_path: Path | str) -> Path: + return Path( + git("rev-parse", "--show-toplevel", cwd=repo_path).stdout.strip() + ).resolve() + + +def current_head(repo_path: Path | str) -> str: + return git("rev-parse", "HEAD", cwd=repo_path).stdout.strip() + + +def current_branch(repo_path: Path | str) -> str: + return git("rev-parse", "--abbrev-ref", "HEAD", cwd=repo_path).stdout.strip() + + +def branch_exists(repo_path: Path | str, name: str) -> bool: + result = git( + "show-ref", + "--verify", + "--quiet", + f"refs/heads/{name}", + cwd=repo_path, + check=False, + ) + return result.returncode == 0 + + +def worktree_status(repo_path: Path | str) -> dict[str, list[str]]: + """Return the tracked/staged/unstaged/untracked/ignored worktree shape + already defined by `invocation.candidate.worktree` and + `checkpoint.worktree` in `references/*.schema.json`. This module populates + that existing shape from live `git status`; it does not define a new one. + """ + output = git("status", "--porcelain=v1", "--ignored", cwd=repo_path).stdout + staged: list[str] = [] + unstaged: list[str] = [] + untracked: list[str] = [] + ignored: list[str] = [] + for line in output.splitlines(): + if not line: + continue + code, path = line[:2], line[3:] + if code == "??": + untracked.append(path) + elif code == "!!": + ignored.append(path) + else: + if code[0] not in (" ", "?"): + staged.append(path) + if code[1] not in (" ", "?"): + unstaged.append(path) + tracked = [ + line for line in git("ls-files", cwd=repo_path).stdout.splitlines() if line + ] + return { + "tracked": tracked, + "staged": staged, + "unstaged": unstaged, + "untracked": untracked, + "ignored": ignored, + } + + +def is_clean(status: dict[str, list[str]]) -> bool: + """Whether a worktree is "clean" per the design: staged, unstaged, and + untracked must be empty. `ignored` is deliberately exempt — an ignored + file is not an uncommitted change.""" + return not status["staged"] and not status["unstaged"] and not status["untracked"] + + +@contextlib.contextmanager +def _message_file(message: str) -> Iterator[str]: + """Yield a temporary file containing a commit message, avoiding shell + interpolation for arbitrary (possibly untrusted) commit text.""" + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", prefix="review-fix-loop-commit-", delete=False + ) as handle: + handle.write(message.rstrip() + "\n") + path = handle.name + try: + yield path + finally: + with contextlib.suppress(FileNotFoundError): + os.unlink(path) + + +# --------------------------------------------------------------------------- +# Common-directory locking +# --------------------------------------------------------------------------- + + +def _sanitize_component(value: str) -> str: + return re.sub(r"[^A-Za-z0-9_.-]", "-", value).strip("-") or "x" + + +def _lock_key(*parts: str) -> str: + return hashlib.sha256("\x1f".join(parts).encode("utf-8")).hexdigest()[:32] + + +def local_ref_lock_path(common_dir: Path, ref: str) -> Path: + """Lock path for the canonical-local-ref lock keyed by common directory + and candidate ref. Held for the complete invocation; prevents two local + branches (any policy) from mutating the same ref concurrently.""" + return ( + common_dir + / ROOT_NAMESPACE + / LOCK_SUBDIR + / f"local-ref-{_lock_key(str(common_dir), ref)}.lock" + ) + + +def remote_target_lock_path( + common_dir: Path, repository_identity: str, remote_ref: str +) -> Path: + """Lock path for the `update_pr` remote-target lock keyed by common + directory, authenticated head-repository identity, and fully qualified + remote head ref. Prevents two local branches in one common directory from + targeting the same PR ref concurrently.""" + key = _lock_key(str(common_dir), repository_identity, remote_ref) + return common_dir / ROOT_NAMESPACE / LOCK_SUBDIR / f"remote-target-{key}.lock" + + +class _FlockHandle: + """A single non-blocking, process-lifetime `flock`-backed lock. + + The operating system releases the lock automatically if the holding + process exits or crashes (design: "The operating system releases them + when the process exits"), so this class never has to implement its own + expiry or lease-renewal logic. + """ + + def __init__(self, path: Path) -> None: + self.path = path + self._fd: int | None = None + + def acquire(self) -> None: + self.path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(self.path, os.O_CREAT | os.O_RDWR, 0o644) + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError as exc: + os.close(fd) + if exc.errno in (errno.EACCES, errno.EAGAIN): + raise CandidateBusyError(f"lock already held: {self.path}") from exc + raise + self._fd = fd + + def release(self) -> None: + if self._fd is not None: + fcntl.flock(self._fd, fcntl.LOCK_UN) + os.close(self._fd) + self._fd = None + + +@dataclasses.dataclass +class CandidateLocks: + local: _FlockHandle + remote: _FlockHandle | None + + +@contextlib.contextmanager +def acquire_candidate_locks( + common_dir: Path, + local_ref: str, + *, + remote_target: tuple[str, str] | None = None, +) -> Iterator[CandidateLocks]: + """Acquire the local-ref lock and, if `remote_target` is given, the + `update_pr` remote-target lock — in that fixed order — and release them + in the reverse order on exit. + + Every acquisition is non-blocking: a busy lock raises `CandidateBusyError` + immediately instead of waiting. Because no invocation ever waits on a + lock, two invocations can never form a circular wait regardless of which + local ref and remote target they each name — this is what makes "lock + ordering avoids self-induced deadlocks for multi-target `update_pr` work" + true by construction rather than by convention alone. If the remote lock + is busy, the already-acquired local lock is released before the error + propagates, so a failed acquisition never leaves a partial lock held. + """ + local_handle = _FlockHandle(local_ref_lock_path(common_dir, local_ref)) + local_handle.acquire() + remote_handle: _FlockHandle | None = None + try: + if remote_target is not None: + identity, ref = remote_target + remote_handle = _FlockHandle( + remote_target_lock_path(common_dir, identity, ref) + ) + remote_handle.acquire() + yield CandidateLocks(local=local_handle, remote=remote_handle) + finally: + if remote_handle is not None: + remote_handle.release() + local_handle.release() + + +# --------------------------------------------------------------------------- +# Durable checkpoint +# --------------------------------------------------------------------------- + + +def checkpoint_path(common_dir: Path, invocation_id: str) -> Path: + return ( + common_dir + / ROOT_NAMESPACE + / CHECKPOINT_SUBDIR + / f"{_sanitize_component(invocation_id)}.json" + ) + + +def write_checkpoint_atomic(path: Path, document: dict[str, Any]) -> None: + """Validate and atomically persist a checkpoint document. + + Writes to a sibling temporary file and `os.replace`s it into place so a + reader never observes a partially written checkpoint, matching the + design's "write checkpoints atomically." Uses `validate.canonical_json` + for deterministic serialization and rejects an invalid document before + touching disk — the durable checkpoint can never disagree with its own + schema and cross-field contract. + """ + errors = validate.validate_checkpoint(document) + if errors: + raise InvalidCheckpointError("; ".join(errors)) + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp( + dir=str(path.parent), prefix=".tmp-checkpoint-", suffix=".json" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + handle.write(validate.canonical_json(document)) + handle.flush() + os.fsync(handle.fileno()) + os.replace(tmp_name, path) + except Exception: + with contextlib.suppress(FileNotFoundError): + os.unlink(tmp_name) + raise + + +def read_checkpoint(path: Path) -> dict[str, Any]: + """Load and validate a checkpoint document. Raises `InvalidCheckpointError` + rather than returning a document that would fail its own schema.""" + try: + document = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError) as exc: + raise InvalidCheckpointError(f"{path}: {exc}") from exc + if not isinstance(document, dict): + raise InvalidCheckpointError(f"{path}: top-level JSON value must be an object") + errors = validate.validate_checkpoint(document) + if errors: + raise InvalidCheckpointError("; ".join(errors)) + return document + + +def reconcile_checkpoint_for_resume( + *, + invocation: dict[str, Any], + checkpoint: dict[str, Any], + live_head: str, + live_base_sha: str, + live_worktree_status: dict[str, list[str]], + lock_busy: bool, +) -> None: + """Enforce every design-required resume precondition or raise. + + Checks, in order: no active lock holder; the complete cross-document + identity set via `validate.validate_checkpoint_against_invocation` (same + invocation ID, repository, branch, original budget, publication policy, + initial head, and initial comparison base — this is also where "the same + original budget and authority" is verified, reusing #96's validator + rather than duplicating it); a clean candidate; and that the live head and + comparison base are exactly the checkpoint's current values. Raises one of + this module's `LocalExecutionError` subclasses and never mutates + anything; the caller decides how to report `blocked/checkpoint_mismatch` + or `blocked/candidate_busy`. + """ + if lock_busy: + raise CandidateBusyError("candidate lock is already held; cannot resume") + + mismatch_errors = validate.validate_checkpoint_against_invocation( + invocation, checkpoint + ) + if mismatch_errors: + raise CheckpointMismatchError("; ".join(mismatch_errors)) + + if not is_clean(live_worktree_status): + raise DirtyWorktreeError("worktree must be clean to resume a checkpoint") + + if checkpoint.get("current_head") != live_head: + raise CheckpointMismatchError( + f"checkpoint current_head {checkpoint.get('current_head')!r} does not " + f"match live head {live_head!r}" + ) + + checkpoint_base_sha = checkpoint.get("comparison_base", {}).get("sha") + if checkpoint_base_sha != live_base_sha: + raise CheckpointMismatchError( + f"checkpoint comparison_base {checkpoint_base_sha!r} does not match " + f"live base {live_base_sha!r}" + ) + + +# --------------------------------------------------------------------------- +# Transactional remediation attempts +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class AttemptHandle: + path: Path + branch: str + base_sha: str + + +def default_attempts_root(common_dir: Path) -> Path: + return common_dir / ROOT_NAMESPACE / ATTEMPTS_SUBDIR + + +def attempt_branch_name(invocation_id: str, sequence: int) -> str: + return f"{ATTEMPT_BRANCH_PREFIX}{_sanitize_component(invocation_id)}/{sequence}" + + +def create_attempt( + *, + repo: Path, + attempts_root: Path, + base_sha: str, + invocation_id: str, + sequence: int, +) -> AttemptHandle: + """Create an isolated attempt worktree and a dedicated branch from the + exact canonical head. + + `git worktree add -b ` creates a brand-new + working directory bound to a brand-new branch, both entirely outside the + canonical worktree's path and branch. Nothing about the canonical + candidate is read or touched by this call, satisfying "attempts occur + outside the canonical worktree and leave it unchanged until promotion." + """ + branch = attempt_branch_name(invocation_id, sequence) + if branch_exists(repo, branch): + raise LocalExecutionError(f"attempt branch already exists: {branch}") + attempt_path = attempts_root / _sanitize_component(invocation_id) / str(sequence) + if attempt_path.exists(): + raise LocalExecutionError( + f"attempt worktree path already exists: {attempt_path}" + ) + attempt_path.parent.mkdir(parents=True, exist_ok=True) + git("worktree", "add", "-b", branch, str(attempt_path), base_sha, cwd=repo) + return AttemptHandle(path=attempt_path, branch=branch, base_sha=base_sha) + + +def commit_attempt(handle: AttemptHandle, message: str) -> str: + """Stage every change in the attempt worktree and create exactly one + commit whose parent is the attempt's recorded `base_sha`. + + Raises `LocalExecutionError` if the resulting commit's parent does not + match — this should be unreachable given `create_attempt`'s own + bookkeeping, but the check keeps the transactional guarantee explicit + rather than assumed. + """ + git("add", "-A", cwd=handle.path) + with _message_file(message) as msg_path: + git("commit", "-F", msg_path, cwd=handle.path) + new_head = current_head(handle.path) + parent = git("rev-parse", f"{new_head}^", cwd=handle.path).stdout.strip() + if parent != handle.base_sha: + raise LocalExecutionError( + f"attempt commit parent {parent!r} does not match recorded base " + f"{handle.base_sha!r}" + ) + return new_head + + +def promote_attempt( + *, + canonical_worktree: Path, + canonical_branch: str, + attempt_sha: str, + expected_old_head: str, +) -> str: + """Verified fast-forward-only promotion of `attempt_sha` onto the + canonical worktree. + + Implements the design's "Transactional remediation attempts" promotion + steps: verify the canonical worktree is on `canonical_branch` and + globally clean; verify its live HEAD equals `expected_old_head`; verify + the attempt commit's parent equals `expected_old_head` (otherwise the + base drifted since the attempt started, and this raises `StaleBaseError` + without touching canonical state — "dirty or advanced canonical state + fails closed and preserves the candidate"); perform one + `git merge --ff-only` through the canonical worktree so branch, HEAD, + index, and files advance together; and verify the resulting HEAD, tree, + and cleanliness before returning. Any failure leaves the canonical + candidate exactly at its prior head — this function never resets, + stashes, or force-updates anything. + """ + on_branch = current_branch(canonical_worktree) + if on_branch != canonical_branch: + raise CandidateIntegrityFailureError( + f"canonical worktree is on {on_branch!r}, expected {canonical_branch!r}" + ) + + status = worktree_status(canonical_worktree) + if not is_clean(status): + raise DirtyWorktreeError("canonical worktree must be clean before promotion") + + live_head = current_head(canonical_worktree) + if live_head != expected_old_head: + raise StaleBaseError( + f"canonical head {live_head!r} no longer matches expected old head " + f"{expected_old_head!r}" + ) + + attempt_parent = git( + "rev-parse", f"{attempt_sha}^", cwd=canonical_worktree + ).stdout.strip() + if attempt_parent != expected_old_head: + raise StaleBaseError( + f"attempt {attempt_sha!r} parent {attempt_parent!r} does not match " + f"expected old head {expected_old_head!r}" + ) + + git("merge", "--ff-only", attempt_sha, cwd=canonical_worktree) + + new_head = current_head(canonical_worktree) + if new_head != attempt_sha: + raise CandidateIntegrityFailureError( + f"canonical head {new_head!r} does not equal promoted commit " + f"{attempt_sha!r} after merge --ff-only" + ) + resulting_tree = git( + "rev-parse", f"{new_head}^{{tree}}", cwd=canonical_worktree + ).stdout.strip() + attempt_tree = git( + "rev-parse", f"{attempt_sha}^{{tree}}", cwd=canonical_worktree + ).stdout.strip() + if resulting_tree != attempt_tree: + raise CandidateIntegrityFailureError( + "canonical tree does not equal the promoted commit's tree" + ) + if not is_clean(worktree_status(canonical_worktree)): + raise CandidateIntegrityFailureError( + "canonical worktree is not clean immediately after promotion" + ) + return new_head + + +def discard_attempt( + *, + common_dir: Path, + handle: AttemptHandle, + attempt_sha: str | None, + reason: str, +) -> dict[str, str]: + """Preserve a failed or stale-based attempt's patch and diagnostics. + + Returns a `{attempt_ref, reason}`-shaped record matching + `checkpoint.preserved_failed_attempts`, plus the on-disk `patch_path` for + operator inspection. Deliberately does not remove the attempt worktree or + branch: recovery, and an eventual explicit `cleanup_attempt`, remain the + only ways to remove them, so a failed attempt's commits (or in-progress + edits) are never silently lost — "preserve recoverable commits ... when + promotion cannot complete." + """ + artifacts_dir = ( + common_dir + / ROOT_NAMESPACE + / PRESERVED_ATTEMPTS_SUBDIR + / _sanitize_component(handle.branch) + ) + artifacts_dir.mkdir(parents=True, exist_ok=True) + patch_path = artifacts_dir / "attempt.patch" + if attempt_sha: + diff = git("diff", handle.base_sha, attempt_sha, cwd=handle.path).stdout + else: + diff = git("diff", cwd=handle.path).stdout + patch_path.write_text(diff) + (artifacts_dir / "reason.txt").write_text(reason.rstrip() + "\n") + return { + "attempt_ref": handle.branch, + "patch_path": str(patch_path), + "reason": reason, + } + + +def cleanup_attempt(*, repo: Path, handle: AttemptHandle, force: bool = False) -> None: + """Remove an attempt's worktree and branch. + + Refuses to act on anything whose branch is not inside the + `review-fix-loop/attempt/` namespace this module itself creates in + `create_attempt` — the one hard safety invariant issue #97 requires: + "cleanup never deletes user-owned work or reference branches." A forged + or corrupted handle can therefore never cause this function to remove a + user's branch or worktree. + """ + if not handle.branch.startswith(ATTEMPT_BRANCH_PREFIX): + raise UnsafeCleanupError( + f"refusing to remove branch outside the attempt namespace: " + f"{handle.branch!r}" + ) + args = ["worktree", "remove"] + if force: + args.append("--force") + args.append(str(handle.path)) + git(*args, cwd=repo) + git("branch", "-D", handle.branch, cwd=repo) + + +# --------------------------------------------------------------------------- +# Recovery +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class RecoveredAttempt: + branch: str + base_sha: str + attempt_sha: str | None + already_promoted: bool + worktree_path: Path | None + + +def _list_attempt_branches(repo: Path, prefix: str) -> list[str]: + result = git( + "for-each-ref", + "--format=%(refname:short)", + f"refs/heads/{prefix}", + cwd=repo, + ) + return [line for line in result.stdout.splitlines() if line] + + +def _find_worktree_path_for_branch(repo: Path, branch: str) -> Path | None: + output = git("worktree", "list", "--porcelain", cwd=repo).stdout + current_path: str | None = None + for line in output.splitlines(): + if line.startswith("worktree "): + current_path = line[len("worktree ") :] + elif line == f"branch refs/heads/{branch}" and current_path is not None: + return Path(current_path) + return None + + +def recover_interrupted_attempts( + *, + repo: Path, + invocation_id: str, + checkpoint: dict[str, Any], +) -> list[RecoveredAttempt]: + """Reconcile attempt branches left behind by an interrupted invocation. + + Compares every existing `review-fix-loop/attempt//*` + branch against the checkpoint's own `cycle_attempts` and `head_history`. + A branch whose tip already appears as a `committed` attempt's + `resulting_head` is already reflected in checkpoint history and is + skipped. A branch whose tip is itself a recorded head (no commit was made + before interruption) or whose tip's sole new commit has a parent + appearing in `head_history` is a uniquely identifiable leftover from an + interrupted cycle and is returned for the caller to decide whether to + retry promotion or discard. + + Anything else — a branch whose parent is not in `head_history`, or more + than one branch claiming the same starting head — cannot be uniquely + reconciled and raises `CandidateIntegrityFailureError` rather than being + silently resolved or deleted, per the design's "Accept only a uniquely + identifiable expected commit. Ambiguity returns + blocked/candidate_integrity_failure." + """ + prefix = f"{ATTEMPT_BRANCH_PREFIX}{_sanitize_component(invocation_id)}/" + branches = _list_attempt_branches(repo, prefix) + head_history = list(checkpoint.get("head_history", [])) + head_history_set = set(head_history) + committed_heads = { + attempt.get("resulting_head") + for attempt in checkpoint.get("cycle_attempts", []) + if attempt.get("outcome") == "committed" + } + current = checkpoint.get("current_head") + + recovered: list[RecoveredAttempt] = [] + claimed_bases: dict[str, str] = {} + for branch in branches: + tip = git("rev-parse", branch, cwd=repo).stdout.strip() + if tip in committed_heads: + continue + + worktree_path = _find_worktree_path_for_branch(repo, branch) + + if tip in head_history_set: + base_sha = tip + attempt_sha: str | None = None + else: + parent_result = git("rev-parse", f"{tip}^", cwd=repo, check=False) + parent = ( + parent_result.stdout.strip() if parent_result.returncode == 0 else None + ) + if parent is None or parent not in head_history_set: + raise CandidateIntegrityFailureError( + f"attempt branch {branch!r} tip {tip!r} does not derive from " + "a recorded head; cannot uniquely reconcile" + ) + base_sha = parent + attempt_sha = tip + + if base_sha in claimed_bases and claimed_bases[base_sha] != branch: + raise CandidateIntegrityFailureError( + f"more than one attempt branch claims to start from head " + f"{base_sha!r}: {claimed_bases[base_sha]!r} and {branch!r}" + ) + claimed_bases[base_sha] = branch + + recovered.append( + RecoveredAttempt( + branch=branch, + base_sha=base_sha, + attempt_sha=attempt_sha, + already_promoted=(attempt_sha is not None and attempt_sha == current), + worktree_path=worktree_path, + ) + ) + return recovered diff --git a/skills/review-fix-loop/scripts/tests/test_local_execution.py b/skills/review-fix-loop/scripts/tests/test_local_execution.py new file mode 100644 index 0000000..48b9158 --- /dev/null +++ b/skills/review-fix-loop/scripts/tests/test_local_execution.py @@ -0,0 +1,822 @@ +"""Deterministic tests for the review-fix-loop local execution substrate +(issue #97): common-directory locking, isolated attempt worktrees, +checkpoint persistence and resume reconciliation, verified fast-forward-only +canonical promotion, and recovery of an interrupted attempt. + +Every test operates against a real temporary Git repository via subprocess +`git` calls — there is no mocked or simulated Git state — so a passing test +proves the module's behavior against actual Git semantics (worktrees, +`merge --ff-only`, `flock`, process-exit lock release). +""" + +from __future__ import annotations + +import fcntl +import importlib.util +import json +import os +import sys +import tempfile +import unittest +from pathlib import Path + +SKILL_ROOT = Path(__file__).resolve().parents[2] + +SPEC = importlib.util.spec_from_file_location( + "review_fix_loop_local_execution", SKILL_ROOT / "scripts" / "local_execution.py" +) +assert SPEC and SPEC.loader +LE = importlib.util.module_from_spec(SPEC) +# Register before exec_module: the module's dataclasses use `from __future__ +# import annotations`, so Python 3.11's dataclass machinery resolves their +# string type hints via `sys.modules[cls.__module__]` and needs this entry. +sys.modules[SPEC.name] = LE +SPEC.loader.exec_module(LE) + + +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 write_and_commit(repo: Path, name: str, content: str, message: str) -> str: + (repo / name).write_text(content) + LE.git("add", "-A", cwd=repo) + LE.git("commit", "-q", "-m", message, cwd=repo) + return LE.current_head(repo) + + +class GitPrimitiveTests(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) + + def test_git_common_dir_is_absolute_and_exists(self): + common = LE.git_common_dir(self.repo) + self.assertTrue(common.is_absolute()) + self.assertTrue(common.is_dir()) + self.assertEqual(common, (self.repo / ".git").resolve()) + + def test_worktree_status_clean_repo(self): + status = LE.worktree_status(self.repo) + self.assertEqual(status["staged"], []) + self.assertEqual(status["unstaged"], []) + self.assertEqual(status["untracked"], []) + self.assertIn("README.md", status["tracked"]) + self.assertTrue(LE.is_clean(status)) + + def test_worktree_status_detects_untracked_and_ignored(self): + (self.repo / ".gitignore").write_text("ignored.txt\n") + LE.git("add", "-A", cwd=self.repo) + LE.git("commit", "-q", "-m", "add gitignore", cwd=self.repo) + (self.repo / "ignored.txt").write_text("ignored\n") + (self.repo / "untracked.txt").write_text("untracked\n") + status = LE.worktree_status(self.repo) + self.assertIn("untracked.txt", status["untracked"]) + self.assertIn("ignored.txt", status["ignored"]) + self.assertFalse(LE.is_clean(status)) + + def test_worktree_status_ignored_alone_is_clean(self): + (self.repo / ".gitignore").write_text("ignored.txt\n") + LE.git("add", "-A", cwd=self.repo) + LE.git("commit", "-q", "-m", "add gitignore", cwd=self.repo) + (self.repo / "ignored.txt").write_text("ignored\n") + status = LE.worktree_status(self.repo) + self.assertTrue(LE.is_clean(status)) + + +class LockingTests(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) + self.common = LE.git_common_dir(self.repo) + + def test_conflicting_local_ref_lock_is_busy(self): + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97"): + with self.assertRaises(LE.CandidateBusyError): + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97"): + pass + + def test_different_local_refs_do_not_contend(self): + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97-a"): + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97-b"): + pass # no contention: distinct targets, no exception + + def test_lock_released_after_context_exit(self): + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97"): + pass + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97"): + pass # re-acquiring after a clean exit must succeed + + def test_remote_target_lock_contention(self): + target = ("shaug/agent-scripts", "refs/heads/pr-branch") + with LE.acquire_candidate_locks( + self.common, "refs/heads/fix/a", remote_target=target + ): + with self.assertRaises(LE.CandidateBusyError): + with LE.acquire_candidate_locks( + self.common, "refs/heads/fix/b", remote_target=target + ): + pass + + def test_cross_policy_contention_on_same_local_ref(self): + """Two invocations (one update_pr, one local_commit) must not both + own the same local ref, regardless of remote target.""" + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/97"): + with self.assertRaises(LE.CandidateBusyError): + with LE.acquire_candidate_locks( + self.common, + "refs/heads/fix/97", + remote_target=("shaug/agent-scripts", "refs/heads/pr-branch"), + ): + pass + + def test_remote_lock_busy_releases_already_acquired_local_lock(self): + target = ("shaug/agent-scripts", "refs/heads/pr-branch") + with LE.acquire_candidate_locks( + self.common, "refs/heads/fix/a", remote_target=target + ): + try: + with LE.acquire_candidate_locks( + self.common, "refs/heads/fix/b", remote_target=target + ): + pass + except LE.CandidateBusyError: + pass + # fix/b's local lock must not still be held after the failed acquisition. + with LE.acquire_candidate_locks(self.common, "refs/heads/fix/b"): + pass + + def test_two_local_branches_targeting_same_pr_ref_contend(self): + target = ("shaug/agent-scripts", "refs/pull/42/head") + with LE.acquire_candidate_locks( + self.common, "refs/heads/branch-one", remote_target=target + ): + with self.assertRaises(LE.CandidateBusyError): + with LE.acquire_candidate_locks( + self.common, "refs/heads/branch-two", remote_target=target + ): + pass + + def test_lock_is_released_when_the_holding_descriptor_closes(self): + """A crashed holder never runs our `release()` cleanup; only the + kernel unlocks an `flock` when every file descriptor referencing it + closes (on `close()` or process exit alike). This opens and locks the + file directly — bypassing `acquire_candidate_locks` entirely — then + closes it without an explicit `LOCK_UN`, so the assertion exercises + exactly the kernel-driven release the design relies on + ("the operating system releases them when the process exits") without + spawning and killing a real subprocess, which proved to be a source of + OS-scheduling timing flakiness in this environment. + """ + lock_ref = "refs/heads/fix/process-exit" + path = LE.local_ref_lock_path(self.common, lock_ref) + path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(path, os.O_CREAT | os.O_RDWR, 0o644) + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + try: + with self.assertRaises(LE.CandidateBusyError): + with LE.acquire_candidate_locks(self.common, lock_ref): + pass + finally: + os.close(fd) # no LOCK_UN: mirrors an OS-driven release on exit + with LE.acquire_candidate_locks(self.common, lock_ref): + pass # the kernel released the lock when the descriptor closed + + +def make_checkpoint(**overrides) -> dict: + document = { + "schema_version": "1.0", + "invocation_id": "inv-1", + "repository": { + "identity": "shaug/agent-scripts", + "git_common_directory": "/work/agent-scripts/.git", + }, + "branch": "fix/97-example", + "worktree": { + "tracked": ["example.py"], + "staged": [], + "unstaged": [], + "untracked": [], + "ignored": [], + }, + "initial_head": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "current_head": "3ea8f9134120a12e741c3b3f87f67a743ad52ad1", + "comparison_base": { + "ref": "main", + "sha": "6d4e80a7e96351b471797a093ebc111917c516bd", + }, + "publication": {"policy": "local_commit"}, + "original_cycle_budget": 3, + "cycle_attempts": [], + "head_history": ["3ea8f9134120a12e741c3b3f87f67a743ad52ad1"], + "base_revision_history": [ + {"ref": "main", "sha": "6d4e80a7e96351b471797a093ebc111917c516bd"} + ], + "review_records": [], + "validation_outcomes": [], + "preserved_failed_attempts": [], + "source": { + "status": "unavailable", + "unavailable_reason": "standalone invocation has no recorded pushable source", + }, + "current_phase": "establish_evidence", + "expected_next_action": "run the first complete review", + } + document.update(overrides) + return document + + +def make_invocation(checkpoint: dict, **overrides) -> dict: + document = { + "schema_version": "1.0", + "invocation_id": checkpoint["invocation_id"], + "repository": checkpoint["repository"], + "candidate": { + "branch": checkpoint["branch"], + "head_sha": checkpoint["initial_head"], + "comparison_base": checkpoint["base_revision_history"][0], + "diff": { + "format": "unified_diff", + "complete": True, + "content": "diff --git a b\n", + }, + "worktree": checkpoint["worktree"], + "all_changes_committed": True, + "source_unavailable_reason": "standalone invocation has no recorded pushable source", + }, + "change_contract": { + "goal": "fix example", + "acceptance_criteria": ["example works"], + "non_goals": [], + "preserved_behaviors": [], + "allowed_remediation_scope": "example.py only", + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [], + }, + }, + "review_execution": {"mode": "fresh_subagent"}, + "fix_cycle_budget": {"max_fix_cycles": checkpoint["original_cycle_budget"]}, + "validation": [ + {"name": "focused", "command": "python3 -m unittest", "scope": "focused"}, + {"name": "full", "command": "just test", "scope": "full"}, + ], + "publication": {"policy": checkpoint["publication"]["policy"]}, + } + document.update(overrides) + return document + + +class CheckpointStoreTests(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.common = Path(self.tmp.name) / ".git" + self.common.mkdir(parents=True) + + def test_write_then_read_round_trip(self): + checkpoint = make_checkpoint() + path = LE.checkpoint_path(self.common, checkpoint["invocation_id"]) + LE.write_checkpoint_atomic(path, checkpoint) + self.assertTrue(path.exists()) + reloaded = LE.read_checkpoint(path) + self.assertEqual(reloaded, checkpoint) + + def test_write_rejects_invalid_checkpoint(self): + checkpoint = make_checkpoint() + del checkpoint["current_phase"] + path = LE.checkpoint_path(self.common, "bad") + with self.assertRaises(LE.InvalidCheckpointError): + LE.write_checkpoint_atomic(path, checkpoint) + self.assertFalse(path.exists()) + + def test_read_rejects_invalid_checkpoint_on_disk(self): + path = self.common / "bad-checkpoint.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"not": "a checkpoint"})) + with self.assertRaises(LE.InvalidCheckpointError): + LE.read_checkpoint(path) + + def test_write_is_atomic_no_leftover_temp_file(self): + checkpoint = make_checkpoint() + path = LE.checkpoint_path(self.common, checkpoint["invocation_id"]) + LE.write_checkpoint_atomic(path, checkpoint) + leftovers = list(path.parent.glob(".tmp-checkpoint-*")) + self.assertEqual(leftovers, []) + + def test_resume_reconciliation_success(self): + checkpoint = make_checkpoint() + invocation = make_invocation(checkpoint) + LE.reconcile_checkpoint_for_resume( + invocation=invocation, + checkpoint=checkpoint, + live_head=checkpoint["current_head"], + live_base_sha=checkpoint["comparison_base"]["sha"], + live_worktree_status={ + "tracked": ["example.py"], + "staged": [], + "unstaged": [], + "untracked": [], + "ignored": [], + }, + lock_busy=False, + ) # no exception raised + + def test_resume_rejects_when_lock_busy(self): + checkpoint = make_checkpoint() + invocation = make_invocation(checkpoint) + with self.assertRaises(LE.CandidateBusyError): + LE.reconcile_checkpoint_for_resume( + invocation=invocation, + checkpoint=checkpoint, + live_head=checkpoint["current_head"], + live_base_sha=checkpoint["comparison_base"]["sha"], + live_worktree_status={ + "tracked": [], + "staged": [], + "unstaged": [], + "untracked": [], + "ignored": [], + }, + lock_busy=True, + ) + + def test_resume_rejects_stale_invocation_identity_mismatch(self): + checkpoint = make_checkpoint() + invocation = make_invocation(checkpoint) + invocation["candidate"]["branch"] = "some-other-branch" + with self.assertRaises(LE.CheckpointMismatchError): + LE.reconcile_checkpoint_for_resume( + invocation=invocation, + checkpoint=checkpoint, + live_head=checkpoint["current_head"], + live_base_sha=checkpoint["comparison_base"]["sha"], + live_worktree_status={ + "tracked": [], + "staged": [], + "unstaged": [], + "untracked": [], + "ignored": [], + }, + lock_busy=False, + ) + + def test_resume_rejects_dirty_worktree(self): + checkpoint = make_checkpoint() + invocation = make_invocation(checkpoint) + with self.assertRaises(LE.DirtyWorktreeError): + LE.reconcile_checkpoint_for_resume( + invocation=invocation, + checkpoint=checkpoint, + live_head=checkpoint["current_head"], + live_base_sha=checkpoint["comparison_base"]["sha"], + live_worktree_status={ + "tracked": [], + "staged": [], + "unstaged": ["dirty.py"], + "untracked": [], + "ignored": [], + }, + lock_busy=False, + ) + + def test_resume_rejects_live_head_mismatch(self): + checkpoint = make_checkpoint() + invocation = make_invocation(checkpoint) + with self.assertRaises(LE.CheckpointMismatchError): + LE.reconcile_checkpoint_for_resume( + invocation=invocation, + checkpoint=checkpoint, + live_head="f" * 40, + live_base_sha=checkpoint["comparison_base"]["sha"], + live_worktree_status={ + "tracked": [], + "staged": [], + "unstaged": [], + "untracked": [], + "ignored": [], + }, + lock_busy=False, + ) + + def test_resume_rejects_live_base_mismatch(self): + checkpoint = make_checkpoint() + invocation = make_invocation(checkpoint) + with self.assertRaises(LE.CheckpointMismatchError): + LE.reconcile_checkpoint_for_resume( + invocation=invocation, + checkpoint=checkpoint, + live_head=checkpoint["current_head"], + live_base_sha="a" * 40, + live_worktree_status={ + "tracked": [], + "staged": [], + "unstaged": [], + "untracked": [], + "ignored": [], + }, + lock_busy=False, + ) + + +class AttemptLifecycleTests(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) + self.common = LE.git_common_dir(self.repo) + self.attempts_root = LE.default_attempts_root(self.common) + self.canonical_head = LE.current_head(self.repo) + + def test_create_attempt_leaves_canonical_untouched(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + self.assertTrue(handle.path.is_dir()) + self.assertNotEqual(handle.path, self.repo) + (handle.path / "example.py").write_text("value = 1\n") + # canonical worktree state is unaffected by edits in the attempt worktree + self.assertFalse((self.repo / "example.py").exists()) + self.assertEqual(LE.current_head(self.repo), self.canonical_head) + self.assertEqual(LE.current_branch(self.repo), "main") + + def test_commit_attempt_creates_commit_with_expected_parent(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + new_head = LE.commit_attempt(handle, "fix(example): add value") + parent = LE.git("rev-parse", f"{new_head}^", cwd=handle.path).stdout.strip() + self.assertEqual(parent, self.canonical_head) + self.assertEqual(LE.current_head(self.repo), self.canonical_head) + + def test_promote_attempt_success_advances_canonical_atomically(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + new_head = LE.commit_attempt(handle, "fix(example): add value") + + new_canonical = LE.promote_attempt( + canonical_worktree=self.repo, + canonical_branch="main", + attempt_sha=new_head, + expected_old_head=self.canonical_head, + ) + self.assertEqual(new_canonical, new_head) + self.assertEqual(LE.current_head(self.repo), new_head) + self.assertTrue((self.repo / "example.py").exists()) + self.assertTrue(LE.is_clean(LE.worktree_status(self.repo))) + + def test_promote_attempt_fails_closed_on_dirty_canonical(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + new_head = LE.commit_attempt(handle, "fix(example): add value") + + (self.repo / "dirty.txt").write_text("uncommitted\n") + with self.assertRaises(LE.DirtyWorktreeError): + LE.promote_attempt( + canonical_worktree=self.repo, + canonical_branch="main", + attempt_sha=new_head, + expected_old_head=self.canonical_head, + ) + # canonical is preserved exactly as it was: still dirty, still at old head + self.assertEqual(LE.current_head(self.repo), self.canonical_head) + self.assertTrue((self.repo / "dirty.txt").exists()) + + def test_promote_attempt_fails_closed_when_canonical_advanced(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + new_head = LE.commit_attempt(handle, "fix(example): add value") + + # Canonical advances out from under the attempt (a promotion race). + advanced_head = write_and_commit( + self.repo, "other.py", "value = 2\n", "unrelated change" + ) + + with self.assertRaises(LE.StaleBaseError): + LE.promote_attempt( + canonical_worktree=self.repo, + canonical_branch="main", + attempt_sha=new_head, + expected_old_head=self.canonical_head, + ) + # the losing attempt's candidate is preserved; canonical is untouched by promotion + self.assertEqual(LE.current_head(self.repo), advanced_head) + self.assertTrue(LE.branch_exists(self.repo, handle.branch)) + self.assertEqual( + LE.git("rev-parse", handle.branch, cwd=self.repo).stdout.strip(), new_head + ) + + def test_promote_attempt_fails_closed_on_wrong_canonical_branch(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + new_head = LE.commit_attempt(handle, "fix(example): add value") + + with self.assertRaises(LE.CandidateIntegrityFailureError): + LE.promote_attempt( + canonical_worktree=self.repo, + canonical_branch="not-main", + attempt_sha=new_head, + expected_old_head=self.canonical_head, + ) + self.assertEqual(LE.current_head(self.repo), self.canonical_head) + + def test_discard_attempt_preserves_patch_and_reason(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + new_head = LE.commit_attempt(handle, "fix(example): add value") + + record = LE.discard_attempt( + common_dir=self.common, + handle=handle, + attempt_sha=new_head, + reason="the same finding survived this fix", + ) + self.assertEqual(record["attempt_ref"], handle.branch) + patch_path = Path(record["patch_path"]) + self.assertTrue(patch_path.exists()) + self.assertIn("example.py", patch_path.read_text()) + # the attempt branch/commit is left in place; nothing is lost + self.assertTrue(LE.branch_exists(self.repo, handle.branch)) + + def test_discard_attempt_without_commit_preserves_working_diff(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + LE.git("add", "-A", cwd=handle.path) # staged, but validation failed pre-commit + + record = LE.discard_attempt( + common_dir=self.common, + handle=handle, + attempt_sha=None, + reason="validation failed before a commit was created", + ) + patch_path = Path(record["patch_path"]) + # nothing was committed, so the working diff itself is preserved instead + self.assertTrue(patch_path.exists()) + + def test_cleanup_attempt_removes_worktree_and_branch(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-1", + sequence=1, + ) + LE.cleanup_attempt(repo=self.repo, handle=handle) + self.assertFalse(handle.path.exists()) + self.assertFalse(LE.branch_exists(self.repo, handle.branch)) + # the canonical branch and worktree are unaffected + self.assertTrue(LE.branch_exists(self.repo, "main")) + self.assertEqual(LE.current_head(self.repo), self.canonical_head) + + def test_cleanup_attempt_refuses_branch_outside_namespace(self): + handle = LE.AttemptHandle( + path=self.repo, branch="main", base_sha=self.canonical_head + ) + with self.assertRaises(LE.UnsafeCleanupError): + LE.cleanup_attempt(repo=self.repo, handle=handle) + # nothing was touched: canonical branch and worktree remain exactly as they were + self.assertTrue(LE.branch_exists(self.repo, "main")) + self.assertTrue(self.repo.is_dir()) + self.assertEqual(LE.current_head(self.repo), self.canonical_head) + + def test_cleanup_attempt_refuses_forged_user_branch_handle(self): + LE.git("branch", "user-owned-reference-branch", cwd=self.repo) + handle = LE.AttemptHandle( + path=self.repo, + branch="user-owned-reference-branch", + base_sha=self.canonical_head, + ) + with self.assertRaises(LE.UnsafeCleanupError): + LE.cleanup_attempt(repo=self.repo, handle=handle) + self.assertTrue(LE.branch_exists(self.repo, "user-owned-reference-branch")) + + +class RecoveryTests(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) + self.common = LE.git_common_dir(self.repo) + self.attempts_root = LE.default_attempts_root(self.common) + self.canonical_head = LE.current_head(self.repo) + + def _checkpoint(self, **overrides) -> dict: + base = { + "invocation_id": "inv-recover", + "initial_head": self.canonical_head, + "current_head": self.canonical_head, + "head_history": [self.canonical_head], + } + base.update(overrides) + return make_checkpoint(**base) + + def test_recovers_uniquely_identifiable_committed_attempt(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-recover", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + attempt_sha = LE.commit_attempt(handle, "fix(example): add value") + + recovered = LE.recover_interrupted_attempts( + repo=self.repo, invocation_id="inv-recover", checkpoint=self._checkpoint() + ) + self.assertEqual(len(recovered), 1) + self.assertEqual(recovered[0].branch, handle.branch) + self.assertEqual(recovered[0].attempt_sha, attempt_sha) + self.assertEqual(recovered[0].base_sha, self.canonical_head) + self.assertFalse(recovered[0].already_promoted) + + def test_recovers_attempt_interrupted_before_any_commit(self): + LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-recover", + sequence=1, + ) + recovered = LE.recover_interrupted_attempts( + repo=self.repo, invocation_id="inv-recover", checkpoint=self._checkpoint() + ) + self.assertEqual(len(recovered), 1) + self.assertIsNone(recovered[0].attempt_sha) + self.assertEqual(recovered[0].base_sha, self.canonical_head) + + def test_already_promoted_attempt_is_not_reported_as_leftover(self): + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-recover", + sequence=1, + ) + (handle.path / "example.py").write_text("value = 1\n") + attempt_sha = LE.commit_attempt(handle, "fix(example): add value") + LE.promote_attempt( + canonical_worktree=self.repo, + canonical_branch="main", + attempt_sha=attempt_sha, + expected_old_head=self.canonical_head, + ) + checkpoint = self._checkpoint( + current_head=attempt_sha, + head_history=[self.canonical_head, attempt_sha], + cycle_attempts=[ + { + "sequence": 1, + "started_from_head": self.canonical_head, + "outcome": "committed", + "resulting_head": attempt_sha, + } + ], + ) + recovered = LE.recover_interrupted_attempts( + repo=self.repo, invocation_id="inv-recover", checkpoint=checkpoint + ) + self.assertEqual(recovered, []) + + def test_ambiguous_branch_not_derived_from_head_history_raises(self): + # An attempt branch whose base commit is not any recorded head at all. + rogue_base = write_and_commit( + self.repo, "rogue.py", "value = 1\n", "unrelated base" + ) + handle = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=rogue_base, + invocation_id="inv-recover", + sequence=1, + ) + (handle.path / "more.py").write_text("value = 2\n") + LE.commit_attempt(handle, "further edit") + + with self.assertRaises(LE.CandidateIntegrityFailureError): + LE.recover_interrupted_attempts( + repo=self.repo, + invocation_id="inv-recover", + checkpoint=self._checkpoint(), + ) + + def test_two_attempts_claiming_same_base_are_ambiguous(self): + handle1 = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-recover", + sequence=1, + ) + (handle1.path / "a.py").write_text("value = 1\n") + LE.commit_attempt(handle1, "attempt one") + + handle2 = LE.create_attempt( + repo=self.repo, + attempts_root=self.attempts_root, + base_sha=self.canonical_head, + invocation_id="inv-recover", + sequence=2, + ) + (handle2.path / "b.py").write_text("value = 2\n") + LE.commit_attempt(handle2, "attempt two") + + with self.assertRaises(LE.CandidateIntegrityFailureError): + LE.recover_interrupted_attempts( + repo=self.repo, + invocation_id="inv-recover", + checkpoint=self._checkpoint(), + ) + + +class CrossWorktreeLockingTests(unittest.TestCase): + """Locks are keyed by Git common directory, so two linked worktrees of one + repository must contend on the same candidate ref lock.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.repo = Path(self.tmp.name) / "repo" + init_repo(self.repo) + self.other_worktree = Path(self.tmp.name) / "other-worktree" + LE.git( + "worktree", + "add", + "-b", + "other-branch", + str(self.other_worktree), + "main", + cwd=self.repo, + ) + + def test_two_linked_worktrees_share_the_same_lock_namespace(self): + common_from_primary = LE.git_common_dir(self.repo) + common_from_linked = LE.git_common_dir(self.other_worktree) + self.assertEqual(common_from_primary, common_from_linked) + + with LE.acquire_candidate_locks(common_from_primary, "refs/heads/main"): + with self.assertRaises(LE.CandidateBusyError): + with LE.acquire_candidate_locks(common_from_linked, "refs/heads/main"): + pass + + +if __name__ == "__main__": + unittest.main()