Skip to content

feat(review-fix-loop): implement local locking, isolated attempts, and recovery - #109

Merged
shaug merged 1 commit into
mainfrom
claude/97-review-fix-loop-local-execution
Jul 30, 2026
Merged

feat(review-fix-loop): implement local locking, isolated attempts, and recovery#109
shaug merged 1 commit into
mainfrom
claude/97-review-fix-loop-local-execution

Conversation

@shaug

@shaug shaug commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

Implements the local execution substrate review-fix-loop needs before any
caller can run an end-to-end loop: 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.

  • Add skills/review-fix-loop/scripts/local_execution.py (dependency-free,
    loads the merged scripts/validate.py contract leaf via importlib rather
    than duplicating any schema or cross-field check):
    • acquire_candidate_locks — non-blocking, common-directory-keyed local-ref
      lock plus the optional update_pr remote-target lock, acquired in that
      fixed order and released in reverse; every acquisition fails fast rather
      than waiting, so no invocation can ever form a circular wait.
    • create_attempt / commit_attempt — isolated git worktree-based
      remediation attempts created from the exact canonical head, entirely
      outside the canonical worktree and branch.
    • promote_attempt — verified fast-forward-only canonical promotion:
      checks the canonical worktree is clean and at the expected old head and
      that the attempt's parent matches before merging, and verifies the
      resulting HEAD/tree/cleanliness afterward. Any failure leaves canonical
      state exactly as it was.
    • discard_attempt / cleanup_attempt — preserves a failed attempt's patch
      and reason for operator inspection; cleanup refuses to touch anything
      outside the review-fix-loop/attempt/ branch namespace it creates.
    • write_checkpoint_atomic / read_checkpoint /
      reconcile_checkpoint_for_resume — atomic, schema-validated checkpoint
      persistence and the complete design-required resume precondition set (no
      active lock holder, matching cross-document identity via Define review-fix-loop invocation, checkpoint, and terminal-result contracts #96's own
      validator, a clean candidate, live head/base agreement).
    • recover_interrupted_attempts — reconciles attempt branches an
      interrupted invocation left behind against a checkpoint's own history,
      returning each uniquely identifiable leftover and raising rather than
      guessing when reconciliation is ambiguous.
  • Add skills/review-fix-loop/scripts/tests/test_local_execution.py: 41
    deterministic tests against real temporary Git repositories (no mocked Git
    state) covering lock contention (same-ref, cross-policy, shared remote
    target, cross-linked-worktree), kernel-driven lock release, checkpoint
    atomicity/round-trip/resume reconciliation and every resume-rejection path,
    isolated attempt creation/commit, promotion success and every fail-closed
    path (dirty canonical, advanced canonical, wrong branch), discard/cleanup
    safety, and interrupted-attempt recovery including ambiguous-state
    rejection.
  • Update skills/review-fix-loop/SKILL.md additively: document the new
    module and narrow the remaining non-goals to what issues Implement review-fix-loop reviewer isolation and complete-review orchestration #98Deliver and evaluate review-fix-loop update_pr #100 still
    own (running a reviewer, selecting/writing a fix's content, publishing).
  • Record the change in CHANGELOG.md.

Why

Issue #97 (child of epic #95, blocked by the now-merged contract leaf #96)
requires this substrate so later children (#98 reviewer isolation, #99
local_commit, #100 update_pr) can assemble the complete review-fix loop
on a tested, Git-native foundation instead of each reimplementing locking,
attempt isolation, and recovery independently. Per the epic's design
(design/review-fix-loop.md, "Local ownership and checkpointing"), this
covers Workflow steps 1 (acquire the lock before any mutation), 6 (validate,
commit, and promote or preserve on failure), and 9 (checkpoint, release,
return).

Non-goals (unchanged, per ticket)

Acceptance criteria (all pre-merge)

  • Conflicting local invocations cannot both own the same target —
    non-blocking flock-backed lock keyed by common directory and ref.
  • Lock ordering avoids self-induced deadlocks for multi-target
    update_pr work — acquisition never blocks, so no invocation can wait
    on another.
  • Attempts occur outside the canonical worktree and leave it unchanged
    until promotion — create_attempt uses git worktree add into a
    dedicated path and branch.
  • Interrupted attempts can be recovered or reported without losing
    commits — recover_interrupted_attempts.
  • Dirty or advanced canonical state fails closed and preserves the
    candidate — promote_attempt's precondition checks.
  • Cleanup never deletes user-owned work or reference branches —
    cleanup_attempt refuses any branch outside the
    review-fix-loop/attempt/ namespace.

Validation

$ python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py'
Ran 154 tests — OK (repeated 3x, no flakes)

$ just format
All checks passed! 111 files left unchanged.

$ just lint
All checks passed! (ruff, mdformat, skills-ref validate for all 8 skills,
plugin packaging validation)

$ just test
All skill test suites pass (implement-epic, implement-ticket,
review-code-change, review-code-simplicity, review-correctness,
review-fix-loop, review-solution-simplicity, review-suite) — OK

Review

A fresh, read-only review-code-change pass (independent agent, raw evidence
only — no implementation transcript) returned aggregate verdict clean
(solution-simplicity, correctness, code-simplicity all clean) against this
exact head. One defer-severity, non-gating correctness observation was
recorded and intentionally not acted on: this module does not itself reread
the comparison-base ref during a live remediation cycle (only at resume,
via reconcile_checkpoint_for_resume); the reviewer agreed this is
correctly out of this ticket's scope, since driving repeated remediation
cycles and rebuilding review evidence after base drift requires the reviewer
orchestration this ticket's non-goals explicitly exclude (issue #98). Noted
here for whichever child assembles the full loop.

Refs #97
Supports #95

…d recovery

## Summary
- Add `skills/review-fix-loop/scripts/local_execution.py`, a dependency-free
  module implementing the local execution substrate design/review-fix-loop.md
  describes in "Local ownership and checkpointing": 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 on a dirty or
  advanced canonical worktree, atomic schema-validated checkpoint persistence
  and resume reconciliation, preserved failed-attempt artifacts, namespace-safe
  cleanup, and recovery of an interrupted attempt against a checkpoint's own
  history.
- Loads `scripts/validate.py` (the #96 contract leaf) via `importlib` instead
  of duplicating any schema or cross-field check, per this ticket's dependency
  to build on the merged contracts leaf.
- Add `skills/review-fix-loop/scripts/tests/test_local_execution.py`: 41
  deterministic unit tests against real temporary Git repositories covering
  lock contention (including cross-policy and shared-PR-ref contention, and
  kernel-driven release when the holding descriptor closes), checkpoint
  atomicity/round-trip/resume reconciliation, isolated-attempt creation and
  commit, promotion success and every fail-closed path (dirty canonical,
  advanced canonical, wrong branch), discard/cleanup safety (refusing to touch
  anything outside the `review-fix-loop/attempt/` namespace), and interrupted-
  attempt recovery including ambiguous-state rejection.
- Update `skills/review-fix-loop/SKILL.md` additively: describe the new local
  execution module and narrow the remaining non-goals to what issues #98#100
  still own (running a reviewer, selecting/writing a fix's content,
  publishing).
- Record the change in `CHANGELOG.md`, backfilling the prior entry's SHA per
  this repository's changelog convention.

## Why
- Issue #97 (child of epic #95) requires the local execution substrate so
  later children (#98 reviewer isolation, #99 `local_commit`, #100
  `update_pr`) can build the complete review-fix loop on top of a safe,
  tested Git-native foundation rather than each reimplementing locking,
  attempt isolation, and recovery independently.

## Acceptance criteria
- Conflicting local invocations cannot both own the same target: non-blocking
  `flock`-backed locks keyed by common directory and ref.
- Lock ordering avoids self-induced deadlocks for multi-target `update_pr`
  work: acquisition never blocks, so no invocation can wait on another.
- Attempts occur outside the canonical worktree and leave it unchanged until
  promotion: `create_attempt` uses `git worktree add` into a dedicated path
  and branch.
- Interrupted attempts can be recovered or reported without losing commits:
  `recover_interrupted_attempts` reconciles leftover attempt branches against
  checkpoint history.
- Dirty or advanced canonical state fails closed and preserves the candidate:
  `promote_attempt` verifies cleanliness and expected head before any merge.
- Cleanup never deletes user-owned work or reference branches: `cleanup_attempt`
  refuses any branch outside the `review-fix-loop/attempt/` namespace.

## Validation
- `python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py'`
  — 154 tests, OK (run 3x for flake-freedom)
- `just test` — all skill and review-suite test suites pass (571 tests total)
- `just format` — all checks pass, no diffs beyond this change
- `just lint` — `lint-py`, `lint-md`, `lint-skills` (skills-ref validate), and
  `validate-plugins` all pass
@shaug
shaug merged commit 26b4cf4 into main Jul 30, 2026
1 check passed
@shaug
shaug deleted the claude/97-review-fix-loop-local-execution branch July 30, 2026 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant