Skip to content

feat(review-fix-loop): deliver and evaluate standalone update_pr workflow - #112

Merged
shaug merged 2 commits into
mainfrom
claude/100-review-fix-loop-update-pr
Jul 31, 2026
Merged

feat(review-fix-loop): deliver and evaluate standalone update_pr workflow#112
shaug merged 2 commits into
mainfrom
claude/100-review-fix-loop-update-pr

Conversation

@shaug

@shaug shaug commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Summary

Delivers and evaluates the standalone update_pr review-fix-loop workflow
(issue #100, epic #95): a converged candidate publishes exactly once, after
a complete clean review, using an expected-old fast-forward Git update.

  • scripts/update_pr.py's run_update_pr composes the exact same
    review/fix/converge engine local_commit.py (Deliver and evaluate standalone review-fix-loop local_commit #99) already implements —
    every intermediate fix commit stays local — plus one expected-old,
    fast-forward-only Git publish immediately after convergence.
  • Resolves and cross-validates the fork/remote publication target from
    candidate.source_binding and publication.pull_request without ever
    assuming "origin" ownership; validates every remote_iteration_grants
    entry against that resolved target.
  • Rereads the live remote head at two loop boundaries (before establishing
    evidence for a fresh review, and before starting a fix attempt) and stops
    with blocked/remote_advanced if it no longer matches the recorded
    expected-old head.
  • At publish time: fetch and reread the exact remote head, require it to
    equal the expected-old head, prove the local candidate is a non-rewriting
    descendant via merge-base --is-ancestor, perform one
    --force-with-lease=<head_ref>:<expected_old_head_sha> push, and read the
    ref back to confirm it landed — preserving the converged local commit and
    reporting an actionable recovery path on any failure.
  • Generalizes local_commit.py's internal loop into a policy-parameterized
    _run_engine both entry points share (_Policy, _PublishOutcome,
    _default_publication); run_local_commit's own behavior and its 21
    existing tests are unchanged.
  • Extracts the test fixtures shared between test_local_commit.py and
    test_update_pr.py into a sibling scripts/tests/helpers.py, matching
    carve-changesets/scripts/tests/helpers.py's established precedent
    (closes the one finding a first review-code-change pass found).

Acceptance criteria (issue #100)

  • No remote write occurs before a complete clean review and required
    validation.
  • Publication fails closed when grants or target identity are missing or
    stale.
  • A successful run advances only the intended remote ref from the
    expected old commit.
  • A competing remote update cannot be overwritten.
  • Lost races and remote failures preserve the converged local commit and
    report an actionable recovery path.
  • Fork-target behavior is covered without assuming origin ownership.

Validation

  • python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p "test_*.py" — 265/265 passed.
  • just format — passed.
  • just lint — passed.
  • just test — passed (full repository gate, every skill's suite).

Disposable-local-bare-repository integration tests
(scripts/tests/test_update_pr.py) cover: a successful converge-then-publish
run with and without a fix cycle; a well-formed remote_iteration_grants
entry that does not itself block publication; a fork target resolved
explicitly without assuming origin ownership; a competing remote update that
cannot be overwritten; local non-fast-forward history relative to the
recorded expected-old head; a misconfigured publication target; a mismatched
remote-iteration grant; an unreachable remote; the remote-target lock
actually exercised through run_update_pr; and rejection of an invalid or
wrong-policy invocation at the API boundary. No test, and no code path this
module can reach, ever touches this repository's actual origin remote.

Review history

Two repository-owned review-code-change passes (fresh, isolated,
read-only, no implementation transcript):

  1. changes_required — one strong_recommendation code-simplicity finding
    (duplicated test fixtures between the two suites). Fixed by extracting
    scripts/tests/helpers.py.
  2. clean — all three lenses (solution simplicity, correctness, code
    simplicity) clean. One non-gating defer (correctness): design names
    four remote-recheck boundaries, the implementation checks two mid-loop
    points plus the publish step's own always-fresh recheck; independently
    confirmed this creates no exploitable gap since only the publish step
    ever writes and it always rechecks fresh as its own first step.

Non-goals (per issue #100)

  • Creating, merging, closing, or otherwise managing a pull request.
  • Force-pushing or rewriting remote history.
  • Distributed coordination beyond local locks and Git compare-and-swap.

Fixes #100

🤖 Generated with Claude Code

shaug and others added 2 commits July 30, 2026 23:50
…flow

## Summary
- Add `scripts/update_pr.py`'s `run_update_pr`, composing the exact same
  review/fix/converge engine `local_commit.py` already implements (every
  intermediate fix commit stays local) plus one expected-old,
  fast-forward-only Git publish immediately after convergence.
- Resolve and cross-validate the fork/remote publication target from
  `candidate.source_binding` and `publication.pull_request` without ever
  assuming "origin" ownership, and validate every `remote_iteration_grants`
  entry against that resolved target, failing closed with
  `blocked/missing_authority` before any lock or mutation.
- Reread the live remote head at two loop boundaries (before establishing
  evidence for a fresh review, and before starting a fix attempt) and stop
  with `blocked/remote_advanced` if it no longer matches the invocation's
  recorded expected-old head.
- At the publish step: fetch and reread the exact remote head, require it to
  equal the expected-old head, prove the local candidate is a non-rewriting
  descendant via `merge-base --is-ancestor`, perform one
  `--force-with-lease=<head_ref>:<expected_old_head_sha>` push, and read the
  ref back to confirm it landed — preserving the converged local commit and
  reporting an actionable recovery path on any failure
  (`remote_advanced`/`candidate_integrity_failure`/`publication_failed`).
- Generalize `local_commit.py`'s internal loop into a policy-parameterized
  `_run_engine` both entry points share (`_Policy`, `_PublishOutcome`,
  `_default_publication`), with `run_local_commit`'s own behavior and its 21
  existing tests unchanged.
- Add `scripts/tests/test_update_pr.py`: disposable-local-bare-repository
  integration tests for a successful converge-then-publish run (with and
  without a fix cycle), a fork target, a competing remote update that cannot
  be overwritten, non-fast-forward history, a misconfigured target, a
  mismatched remote-iteration grant, an unreachable remote, the
  remote-target lock exercised end to end, and invalid-invocation rejection.
- Add `references/update-pr.md` and update `SKILL.md`/`CHANGELOG.md`.

## Why
- Issue #100 (epic #95): deliver and evaluate the `update_pr` publication
  tail so a caller can publish a converged review-fix-loop candidate exactly
  once, safely, without force-pushing or assuming origin ownership.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ures

## Summary
- Add `scripts/tests/helpers.py`: the module loader, a bare local
  repository, the always-passing validation commands, the
  marker-file-driven fake reviewer, and the accepting decider/fixer shared
  between `test_local_commit.py` and `test_update_pr.py`.
- Update both test files to import these fixtures from `helpers.py` instead
  of duplicating them, matching `carve-changesets/scripts/tests/helpers.py`'s
  established precedent for one skill's own shared test fixture module.
- Update `references/update-pr.md` and both test files' module docstrings to
  describe the shared-fixture layout.

## Why
- Closes the one code-simplicity gap (`strong_recommendation`) the first
  review-code-change pass on #100 found: ~150 lines of near-verbatim
  duplicated test setup between the two suites.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@shaug
shaug merged commit 729135b into main Jul 31, 2026
1 check passed
@shaug
shaug deleted the claude/100-review-fix-loop-update-pr branch July 31, 2026 07:36
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.

Deliver and evaluate review-fix-loop update_pr

1 participant