From 95ccb81142357cc5cc55e78150abd5b39fa0e0b1 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Thu, 30 Jul 2026 23:50:31 -0700 Subject: [PATCH 1/2] feat(review-fix-loop): deliver and evaluate standalone update_pr workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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=:` 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 --- CHANGELOG.md | 20 +- skills/review-fix-loop/SKILL.md | 88 +- .../review-fix-loop/references/update-pr.md | 217 +++++ .../review-fix-loop/scripts/local_commit.py | 400 ++++++++- .../scripts/tests/test_update_pr.py | 763 ++++++++++++++++++ skills/review-fix-loop/scripts/update_pr.py | 524 ++++++++++++ 6 files changed, 1958 insertions(+), 54 deletions(-) create mode 100644 skills/review-fix-loop/references/update-pr.md create mode 100644 skills/review-fix-loop/scripts/tests/test_update_pr.py create mode 100644 skills/review-fix-loop/scripts/update_pr.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b3b03e..c84a0ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,25 @@ summary: Chronological history of repository and skill changes. # Changelog -## 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 +## 2026-07-30 — Delivered and evaluated the standalone review-fix-loop `update_pr` workflow, 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): add and evaluate the standalone `update_pr` workflow + (`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; resolves and cross-validates the + fork/remote publication target without assuming "origin" ownership, validates + `remote_iteration_grants`, and preserves the converged local commit with an + actionable recovery path when the publication race is lost, the local + candidate's history does not descend from the expected old head, or the remote + is unavailable, with disposable-local-remote fixtures 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, and the remote-target lock actually being exercised end to end; + generalizes `local_commit.py`'s internal loop into a policy-parameterized + `_run_engine` both entry points share, with `run_local_commit`'s own behavior + and its 21 existing tests unchanged - 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 @@ -16,6 +33,7 @@ summary: Chronological history of repository and skill changes. exhaustion, validation failure (unavailable/untractable/tractable), operator input (declined finding and scope expansion), expanding/oscillating finding sets, repeated failed attempts, and interrupted-attempt recovery + (`eaa1ded44eef0fa29d874d93196ffa7d3e0e1e79`) - 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 diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 0e5ad07..22082ac 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 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. +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` and `update_pr` workflows, the latter publishing exactly once via an expected-old fast-forward Git update after convergence. Use to check a review-fix-loop document against the shared schemas, to run one complete review pass, to acquire a candidate lock or run/recover an isolated remediation attempt, or to run the complete standalone loop (review, decide, fix, validate, commit, repeat, converge or stop) under either publication policy. See design/review-fix-loop.md, references/CONTRACT.md, references/reviewer-orchestration.md, references/local-commit.md, and references/update-pr.md for the full design. 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). -Four of its children are implemented so far: +Five 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 @@ -42,14 +42,24 @@ Four of its children are implemented so far: — with no remote write in any path. See [Run the standalone `local_commit` workflow](#run-the-standalone-local_commit-workflow) below. +- [Issue #100](https://github.com/shaug/agent-scripts/issues/100) adds and + evaluates `update_pr`: the same review/fix/converge machinery as + `local_commit` — 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 (never + assuming "origin" ownership), validates `remote_iteration_grants`, and + preserves the converged local commit with an actionable recovery path when the + publication race is lost or the remote is unavailable. See + [Run the standalone `update_pr` workflow](#run-the-standalone-update_pr-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. +Use [`scripts/local_commit.py`](scripts/local_commit.py)'s +`run_local_commit(...)` to run a complete standalone `local_commit` invocation +end to end, and [`scripts/update_pr.py`](scripts/update_pr.py)'s +`run_update_pr(...)` for `update_pr`; 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 @@ -213,12 +223,68 @@ 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. +## Run the standalone `update_pr` workflow + +Read [`references/update-pr.md`](references/update-pr.md) in full before running +an end-to-end `update_pr` invocation; it implements design's "Publication +policy" > `update_pr`, "Origin-visible exception", workflow step 8 "Publish +after convergence", and the `update_pr` parts of "Local ownership and +checkpointing" > "Cooperative ownership". In summary: +[`scripts/update_pr.py`](scripts/update_pr.py)'s `run_update_pr(...)` composes +the exact same review/fix/converge engine `local_commit.py` already implements — +it never reimplements Resolve/Establish evidence/Review/Decide/Fix/Validate and +commit/Invalidate and repeat, and every intermediate fix commit stays local +exactly as under `local_commit`. Only the publication tail differs: + +1. Resolve and cross-validate the publication target from + `candidate.source_binding` (the actual pushable remote — never assumed to be + this repository's own `origin`, so a fork's own remote is handled identically + to a same-repository branch) and `publication.pull_request` (the PR's own + head-repository/head-ref/expected-old identity); a mismatch between the two + fails closed with `blocked/missing_authority` before any lock, review, or + mutation. Validate every `remote_iteration_grants` entry against that same + resolved target the same way. +2. Acquire the same local candidate lock as `local_commit` plus the + `update_pr`-only remote-target lock (already implemented by + [Local execution](#local-execution)'s `acquire_candidate_locks`). +3. Run the identical loop, with one added check at the two points + `local_commit`'s engine already calls a policy hook from (before establishing + evidence for a fresh review, and before starting a fix attempt): reread the + live remote head and stop with `blocked/remote_advanced` if it no longer + matches the invocation's recorded `expected_old_head_sha`. +4. The instant the aggregate review is clean, and only then: fetch and reread + the exact remote head; require it to equal `expected_old_head_sha`; prove the + local candidate is a non-rewriting descendant of it; perform one + `--force-with-lease=:` push; and read the + ref back to confirm it now equals the converged local head. Any failure at + any of those steps returns `blocked` (`remote_advanced`, + `candidate_integrity_failure`, or `publication_failed`) and preserves the + converged local commit exactly as `local_commit` always would — this never + merges, rebases, force-updates, or otherwise supersedes a competing + candidate. + +See [`scripts/tests/test_update_pr.py`](scripts/tests/test_update_pr.py) for +complete disposable-remote coverage (every test drives a real temporary Git +repository and a real disposable local bare repository as the publication remote +— never this repository's actual `origin`): a successful converge-then-publish +run with and without a fix cycle, a fork target resolved 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 being exercised through +`run_update_pr`, and rejection of an invalid invocation or a `local_commit` +invocation at the API boundary. + ## Non-goals -- Publishing anything, including the `update_pr` expected-old fast-forward - update (issue #100). +- Creating, merging, closing, or otherwise managing a pull request. +- Force-pushing or rewriting remote history — `update_pr` performs only one + expected-old, fast-forward-only Git update. - Migrating `implement-ticket`, `babysit-pr`, `carve-changesets`, or any other existing caller. - Owning acceptance criteria or a caller-specific acceptance ledger — the contract records `acceptance_reconciliation_required` but the caller always retains its own ledger. +- Distributed coordination beyond this skill's local locks and Git + compare-and-swap (no distributed lease, fencing token, or coordinator + service). diff --git a/skills/review-fix-loop/references/update-pr.md b/skills/review-fix-loop/references/update-pr.md new file mode 100644 index 0000000..92b2312 --- /dev/null +++ b/skills/review-fix-loop/references/update-pr.md @@ -0,0 +1,217 @@ +# Standalone `update_pr` workflow + +This document implements +[`design/review-fix-loop.md`](../../../design/review-fix-loop.md)'s "Publication +policy" > `update_pr`, "Origin-visible exception", workflow step 8 "Publish +after convergence", and the `update_pr` parts of "Local ownership and +checkpointing" > "Cooperative ownership" — for the executing agent that follows +[`SKILL.md`](../SKILL.md). It does not redefine any contract, locking, +checkpoint, reviewer-orchestration, or review/fix/converge-loop behavior already +owned by [`references/CONTRACT.md`](CONTRACT.md), +[`references/reviewer-orchestration.md`](reviewer-orchestration.md), and +[`references/local-commit.md`](local-commit.md); it composes them into the one +entry point issue #100 delivers: +[`scripts/update_pr.py`](../scripts/update_pr.py)'s `run_update_pr(...)`. + +## What this module composes, and what it does not reimplement + +`local_commit.py` (#99) already implements every workflow step from 1 "Resolve" +through 7 "Invalidate and repeat" as a private `_run_engine` function, +parameterized by a `_Policy` describing what, if anything, a publication policy +adds on top of the shared review/fix/converge loop. `run_local_commit` is a thin +wrapper around `_run_engine` bound to the trivial `local_commit` policy (every +hook `None` — its behavior is exactly what it always was before this policy +abstraction existed). `update_pr.py` is the *only other* caller of +`_run_engine`; it never reimplements the loop's control flow, fix-cycle budget +enforcement, or checkpoint/terminal-result assembly — it supplies a populated +`_Policy` built from this invocation's own resolved publication target: + +- `remote_target` — the `(repository, head_ref)` pair `local_execution.py`'s + `acquire_candidate_locks` already accepts as its `update_pr`-only + remote-target lock argument (issue #97). This module does not reimplement + locking; it only supplies the identity that lock is keyed by. +- `checkpoint_pull_request` — the `{head_repository, head_ref, base_ref}` shape + `checkpoint.schema.json`'s `publication.pull_request` already defines (issue + #96). This module does not redefine that shape; it only populates it from the + resolved target. +- `check_remote` — a hook `_run_engine` calls at two points every ordinary loop + iteration already reaches (immediately before establishing evidence for a + fresh review, and immediately before starting a fix attempt). Returns `None` + when the remote has not observably advanced, or + `{"reason": "remote_advanced", "remote_head": | None}` when it has. + `_run_engine` converts a non-`None` result into a `blocked` terminal result + using the exact same `_default_publication` status rule described below — this + module never computes `publication.status` itself for this path. +- `publish` — a hook `_run_engine` calls exactly once, only immediately after an + aggregate review comes back `clean`, in place of `local_commit`'s + unconditional "publish nothing" return. Returns a `_PublishOutcome` + (`status: "published" | "failed"`, plus `remote_head_before`/ + `remote_head_after`/`blocked_reason`/`operator_action`); `_run_engine` + converts that into the converged-and-published terminal result or a `blocked` + one, again reusing the same status rule. + +What issue #100 itself owns, and what did not exist before this module: + +- `resolve_publication_target` — resolving and cross-validating the fork/ remote + publication target ("Resolve fork and remote publication targets explicitly"). +- `validate_remote_iteration_grants` — validating every + `remote_iteration_grants` entry against that resolved target ("Require and + validate the origin-visible grants described by the design"). +- The two `check_remote`/`publish` closures themselves: the actual + `git ls-remote`/`git push --force-with-lease`/readback sequence design's + "Publication policy" > `update_pr` describes. +- `local_commit.py`'s small, additive generalization that makes the shared + engine policy-aware: `_Policy`, `_PublishOutcome`, `_default_publication`, the + `_State.policy` field, and the `publication_override`/`source_override` + parameters `_finalize`/`_terminal_result` now accept. Every one of these is + additive — `run_local_commit`'s own behavior and its 21 existing tests are + unchanged; see `local_commit.py`'s own module docstring for the exact + cross-module contract these private names now form between the two files. + +## Host boundary + +Identical to `local_commit.py`'s: `reviewer`, `decide`, and `apply_fix` are +still the caller-supplied ports for the three genuinely host/runtime actions +(running one review pass, deciding a finding's disposition, and writing a fix's +content). This module adds no new host port. Resolving the publication target, +checking the remote, and publishing are real Git operations this dependency-free +module performs itself via `local_execution.py`'s bundled `git` helper — +`git ls-remote`, `git merge-base --is-ancestor`, and +`git push ... --force-with-lease=:` — exactly +as `local_commit.py` already performs every other Git operation itself. No test +in this skill's suite, and no code path this module can reach, ever touches this +repository's real `origin` remote; every test drives a disposable local bare +repository addressed by its filesystem path, matching +`carve-changesets/scripts/tests/helpers.py`'s established convention. + +## Resolving the publication target + +`validate.validate_invocation` already requires an `update_pr` invocation to +carry both `candidate.source_binding` (the actual pushable location: a +repository identity, remote URL, fully qualified ref, and observed object ID) +and `publication.pull_request` (the PR's own head-repository, head-ref, +expected-old head SHA, base ref, and base SHA). Schema validation cannot check +that those two halves of the invocation's *own* contract agree with each other; +`resolve_publication_target` does: + +- `candidate.source_binding.repository` must equal + `publication.pull_request.head_repository`. +- `candidate.source_binding.ref` must equal `publication.pull_request.head_ref`. +- `publication.pull_request.head_ref` must be a fully qualified ref + (`refs/...`). + +A mismatch — the invocation disagreeing with itself about what it is allowed to +push — raises `TargetResolutionError`, and `run_update_pr` converts that into a +structured `blocked/missing_authority` terminal result before acquiring any +lock, running any review, or touching the repository at all. This is +deliberately symmetric for a same-repository branch and a fork: the resolved +target's `remote_url` always comes from `source_binding.remote_url`, never from +a configured `origin` remote or any assumption about which repository owns the +branch. + +`validate_remote_iteration_grants` applies the identical check to every +`publication.remote_iteration_grants` entry (this ticket's "Require and validate +the origin-visible grants described by the design"): a grant whose +`repository`/`ref` do not match the resolved target, or whose `ref` is not fully +qualified, fails closed the same way. This module does not implement invoking +any origin-visible-exception mechanism itself — no host port in this ticket's +scope demonstrably requires an origin-visible head mid-loop — it only validates +grant structure so a stale or mismatched one is never silently accepted. + +## The publish step + +Design's "Publication policy" > `update_pr` describes six steps; `publish` +implements all of them: + +1. Fetch and reread the exact remote head (`git ls-remote`). +2. Require it to equal `expected_old_head_sha`. A `None` (ref does not exist) or + mismatched value returns `blocked` without ever attempting a push — + `publication_failed` for a missing ref (a target/configuration problem), + `remote_advanced` for a mismatched one (another clone won the race). +3. Prove the local candidate is a non-rewriting descendant of + `expected_old_head_sha` via `git merge-base --is-ancestor`. A failure here + means this invocation's own recorded expected-old head is not actually an + ancestor of its local candidate (a configuration problem, not a race) and + returns `blocked/candidate_integrity_failure` without attempting a push. +4. Perform one + `git push : --force-with-lease=:` + — the strongest exact-old primitive the Git transport supports, combined with + the independent ancestry check above. This is a compare-and-swap guard, never + authority to rewrite history. +5. If the push itself is rejected (a race that occurred between steps 1 and 4), + reread the remote head again to distinguish a genuine race + (`remote_advanced`, with the actual competing head reported) from any other + push failure (`publication_failed`). +6. On a successful push, read the ref back and require it to equal the converged + local head; a mismatch is `publication_failed`. + +Every failure path preserves the converged local commit untouched — `publish` +never merges, rebases, force-updates, or otherwise supersedes a competing +candidate — and reports it via the terminal result's `unpushed_commits`, +`head.final`, and `operator_action`, matching this ticket's "Preserve and report +the converged commit if the publication race is lost or publication is +unavailable" requirement. + +## Publication status, reused rather than reinvented + +`references/CONTRACT.md`'s terminal-result rule is: under `update_pr`, a +`converged` result's `publication.status` must be `published`; a +`changes_remaining` result's must be `withheld`; a `blocked` result's must be +`failed` only for `remote_advanced`/`publication_failed` and `withheld` for +every other reason (including `candidate_integrity_failure`, `candidate_busy`, +`missing_authority`, and every ordinary review/validation stop). This module +never recomputes that rule itself: `local_commit._default_publication` is the +one place it lives, and every blocked path in this module — the two mid-loop +`check_remote` stops and every non-published outcome of `publish` — routes its +blocked reason through that same function (directly, or via `_run_engine`'s own +use of it) rather than trusting a hook's own `"failed"` label as the +schema-facing status. This is why an early version of this implementation could +set `publication.status: "failed"` for `candidate_integrity_failure` and fail +its own schema validation — a defect worth naming here so a future change to +either side of this rule updates both together. + +## Terminal states this module actually returns + +- `converged` — the final head's review is `clean`, required validation passed, + `write_isolation` was `enforced` throughout, and the exact expected-old + fast-forward publish landed and read back correctly. `publication.status` is + `published` and `unpushed_commits` is empty. +- `changes_remaining` — identical reasons to `local_commit` + (`cycle_budget_exhausted`, `current_candidate_validation_failure`, + `repeated_failed_attempt`, `expanding_findings`, `oscillation`). + `publication.status` is `withheld` — remediation stopped before convergence, + so this module never even attempts to resolve the publication target's live + state. +- `blocked` — every `local_commit` blocked reason remains reachable + (`candidate_busy`, `candidate_integrity_failure`, `checkpoint_mismatch`, + `missing_capability`, `reviewer_integrity_failure`, `validation_unavailable`, + `operator_input_required`, `scope_decision_required`), plus three this module + adds: + - `missing_authority` — the publication target or a remote-iteration grant + failed to resolve, before any lock or mutation. + - `remote_advanced` — the remote head observably moved away from + `expected_old_head_sha`, either at a mid-loop check or at publish time. + - `publication_failed` — the remote was unreachable, the push failed for a + reason other than a losing race, or the post-push readback did not match. + +## Tests + +[`scripts/tests/test_update_pr.py`](../scripts/tests/test_update_pr.py) drives +`run_update_pr` against a real temporary Git repository and a real disposable +local bare repository used as the publication remote — never this repository's +actual `origin` — covering: 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 (and proof the main repository's own bare remote never saw the +fork's ref at all); a competing remote update that cannot be overwritten (a +second clone wins the race, and the converged local commit is preserved while +the remote is left holding the competitor's head); local non-fast-forward +history relative to the recorded expected-old head; a +source-binding/pull-request repository mismatch (a misconfigured target) failing +closed before any lock; a remote-iteration grant referencing the wrong ref +failing closed before any lock; an unreachable remote failing closed at the +publish step alone, without losing the already-converged local commit; the +`update_pr`-only remote-target lock actually being exercised through +`run_update_pr` (not only `local_execution.py`'s own unit tests); and rejection +of an invalid invocation or a `local_commit`-policy 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 index 6ce2367..fac6b63 100644 --- a/skills/review-fix-loop/scripts/local_commit.py +++ b/skills/review-fix-loop/scripts/local_commit.py @@ -1,9 +1,11 @@ #!/usr/bin/env python3 -"""End-to-end standalone `local_commit` workflow for `review-fix-loop` (issue #99). +"""End-to-end standalone `local_commit` workflow for `review-fix-loop` (issue #99), +and the shared engine issue #100's `update_pr.py` composes its publication tail +onto. 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: +through 9 "Return"): - `validate.py` (#96) — the invocation/checkpoint/terminal-result contract and its cross-field semantics. @@ -19,6 +21,17 @@ Fix, Validate and commit, Invalidate and repeat, Publish, Return), fix-cycle budget enforcement, and terminal-result assembly. +`run_local_commit` is a thin wrapper around the private `_run_engine`, bound to +the trivial `local_commit` `_Policy` (every hook `None` — the engine's +behavior is exactly what it always was before this policy abstraction +existed). `update_pr.py` (issue #100) is the only other caller of +`_run_engine`; it supplies a populated `_Policy` built from its own resolved +publication target instead of reimplementing this loop. `_run_engine`, +`_Policy`, `_PublishOutcome`, `_State`, `_finalize`, `_minimal_blocked_result`, +and `_validate_and_require_policy` are shared, private, cross-module surface +between the two files in this same skill directory — never a public API for +anything outside it. + ## Host boundary Three actions in the design are inherently host/runtime actions that this @@ -165,6 +178,85 @@ def _default_classify_validation_failure( return None +# --------------------------------------------------------------------------- +# Publication policy (shared engine hook; issue #100's `update_pr` is the only +# non-trivial policy so far) +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class _PublishOutcome: + """The result of one attempted publication, returned by a `_Policy.publish` + hook. `status` is either `"published"` (the exact expected-old fast-forward + update landed and read back correctly) or `"failed"` (nothing was + published; `blocked_reason` names why, and the caller's already-converged + local commit is preserved exactly as-is).""" + + status: str # "published" | "failed" + operator_action: str + non_converged_exposure: bool = False + remote_head_before: str | None = None + remote_head_after: str | None = None + blocked_reason: str | None = None # required when status == "failed" + + +@dataclasses.dataclass(frozen=True) +class _Policy: + """What distinguishes one publication policy's effect on the shared engine. + + `local_commit` (this module's own default) never sets `remote_target`, + `check_remote`, or `publish` — every hook stays `None`, so the engine's + behavior for `local_commit` is unchanged from before this policy + abstraction existed. `update_pr.py` supplies a populated `_Policy` built + from its own resolved publication target; the engine never constructs one + itself for `update_pr`. + """ + + name: str # "local_commit" | "update_pr" + remote_target: tuple[str, str] | None = None + checkpoint_pull_request: Mapping[str, str] | None = None + # Returns `None` when the remote has not observably advanced, or + # `{"reason": , "remote_head": | None}` when it + # has — `remote_head` is the live value observed, when the check could + # determine one, for the resulting terminal result's + # `publication.remote_head_before`/`remote_head_after`. + check_remote: Callable[["_State"], dict[str, Any] | None] | None = None + publish: Callable[["_State"], _PublishOutcome] | None = None + + +_LOCAL_COMMIT_POLICY = _Policy(name="local_commit") + + +def _default_publication( + policy_name: str, *, terminal_state: str, reason: str | None +) -> dict[str, Any]: + """The policy-appropriate `publication` dict for every ordinary return path + that never attempts a remote write itself (every `blocked`/ + `changes_remaining` reason except the dedicated publish step's own + outcome, which always supplies its own explicit `publication_override`). + + `local_commit` is always `not_applicable`, exactly as before this + function existed. `update_pr` mirrors `validate.py`'s own + `BLOCKED_REASONS_IMPLYING_PUBLICATION_FAILED` rule instead of duplicating + it: `withheld` for every ordinary stop, `failed` only for the two reasons + that mean a publication attempt did not land (`remote_advanced`, + `publication_failed`). + """ + if policy_name == "local_commit": + return { + "policy": "local_commit", + "status": "not_applicable", + "non_converged_exposure": False, + } + status = ( + "failed" + if terminal_state == "blocked" + and reason in VALIDATE.BLOCKED_REASONS_IMPLYING_PUBLICATION_FAILED + else "withheld" + ) + return {"policy": policy_name, "status": status, "non_converged_exposure": False} + + # --------------------------------------------------------------------------- # Internal helpers # --------------------------------------------------------------------------- @@ -355,6 +447,7 @@ class _State: base_ref: str current_base_sha: str original_budget: int + policy: _Policy 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( @@ -423,7 +516,9 @@ def _checkpoint_document( state: _State, *, phase: str, next_action: str ) -> dict[str, Any]: invocation = state.invocation - publication: dict[str, Any] = {"policy": "local_commit"} + publication: dict[str, Any] = {"policy": state.policy.name} + if state.policy.checkpoint_pull_request is not None: + publication["pull_request"] = dict(state.policy.checkpoint_pull_request) document = { "schema_version": "1.0", "invocation_id": invocation["invocation_id"], @@ -467,6 +562,8 @@ def _terminal_result( terminal_state: str, reason: str | None, operator_action: str, + publication_override: Mapping[str, Any] | None = None, + source_override: Mapping[str, Any] | None = None, ) -> dict[str, Any]: invocation = state.invocation consumed = state.consumed_cycles() @@ -476,6 +573,23 @@ def _terminal_result( state.current_head != state.initial_head or state.current_base_sha != state.base_revision_history[0]["sha"] ) + publication = ( + dict(publication_override) + if publication_override is not None + else _default_publication( + state.policy.name, terminal_state=terminal_state, reason=reason + ) + ) + source = ( + dict(source_override) + if source_override is not None + else _terminal_source_status( + invocation["candidate"], repo=state.repo, current_head=state.current_head + ) + ) + unpushed_commits = ( + [] if publication.get("status") == "published" else list(created_commits) + ) document: dict[str, Any] = { "schema_version": "1.0", "invocation_id": invocation["invocation_id"], @@ -501,15 +615,9 @@ def _terminal_result( "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, - }, + "source": source, + "unpushed_commits": unpushed_commits, + "publication": publication, "acceptance_reconciliation_required": bool(identity_changed), "unresolved_or_deferred_findings": list(state.unresolved_or_deferred), "operator_action": operator_action, @@ -528,6 +636,8 @@ def _finalize( reason: str | None, operator_action: str, phase: str, + publication_override: Mapping[str, Any] | None = None, + source_override: Mapping[str, Any] | None = None, ) -> dict[str, Any]: _persist_checkpoint(state, phase=phase, next_action=operator_action) result = _terminal_result( @@ -535,6 +645,8 @@ def _finalize( terminal_state=terminal_state, reason=reason, operator_action=operator_action, + publication_override=publication_override, + source_override=source_override, ) errors = VALIDATE.validate_terminal_result(result) if errors: @@ -544,8 +656,33 @@ def _finalize( return result +def _remote_check_publication_override( + state: _State, remote_check: Mapping[str, Any] +) -> dict[str, Any]: + """Build the `publication` dict for a mid-loop `check_remote` stop, reusing + `_default_publication`'s exact status rule and attaching the observed + remote head (when the check could determine one) as both + `remote_head_before` and `remote_head_after` — nothing was pushed, so the + remote's live value is unchanged by this invocation either way.""" + publication = dict( + _default_publication( + state.policy.name, terminal_state="blocked", reason=remote_check["reason"] + ) + ) + remote_head = remote_check.get("remote_head") + if remote_head is not None: + publication["remote_head_before"] = remote_head + publication["remote_head_after"] = remote_head + return publication + + def _minimal_blocked_result( - invocation: Mapping[str, Any], *, repo: Path, reason: str, operator_action: str + invocation: Mapping[str, Any], + *, + repo: Path, + reason: str, + operator_action: str, + policy_name: str = "local_commit", ) -> dict[str, Any]: """Build a `blocked` terminal result for a stop that occurs before this invocation could acquire its candidate lock — nothing has been @@ -582,11 +719,9 @@ def _minimal_blocked_result( "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, - }, + "publication": _default_publication( + policy_name, terminal_state="blocked", reason=reason + ), "acceptance_reconciliation_required": False, "unresolved_or_deferred_findings": [], "operator_action": operator_action, @@ -601,6 +736,28 @@ def _minimal_blocked_result( return document +def _validate_and_require_policy( + invocation: Mapping[str, Any], expected_policy: str +) -> None: + """Shared entry-point guard for both `run_local_commit` and + `update_pr.run_update_pr`: validate the invocation against the complete + contract and require its exact `publication.policy`. Raises + `LocalCommitError` for a caller/programming error — an invalid invocation + or one whose policy does not match the entry point invoked — never for an + ordinary runtime stop condition, matching this module's existing + convention.""" + errors = VALIDATE.validate_invocation(dict(invocation)) + if errors: + raise LocalCommitError( + "invocation failed contract validation: " + "; ".join(errors) + ) + policy = invocation["publication"]["policy"] + if policy != expected_policy: + raise LocalCommitError( + f"expected publication.policy {expected_policy!r}, got {policy!r}" + ) + + def run_local_commit( invocation: Mapping[str, Any], *, @@ -620,25 +777,63 @@ def run_local_commit( `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. + + A thin wrapper around `_run_engine` bound to the trivial `local_commit` + policy (every hook `None`); see `update_pr.run_update_pr` for the other + entry point that shares this same engine with a populated `_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}" - ) + _validate_and_require_policy(invocation, "local_commit") + return _run_engine( + invocation, + repo=repo, + reviewer=reviewer, + decide=decide, + apply_fix=apply_fix, + run_validation=run_validation, + classify_validation_failure=classify_validation_failure, + host_supports_fresh_subagent=host_supports_fresh_subagent, + attempts_root=attempts_root, + resume_checkpoint=resume_checkpoint, + policy=_LOCAL_COMMIT_POLICY, + ) + +def _run_engine( + invocation: Mapping[str, Any], + *, + repo: Path, + reviewer: ReviewerPort, + decide: DeciderPort, + apply_fix: FixerPort, + run_validation: ValidationRunnerPort, + classify_validation_failure: ValidationClassifierPort, + host_supports_fresh_subagent: bool, + attempts_root: Path | None, + resume_checkpoint: Mapping[str, Any] | None, + policy: _Policy, +) -> dict[str, Any]: + """The shared review/fix/converge engine both publication policies use. + + Composes `validate.py`, `local_execution.py`, and + `reviewer_orchestration.py` exactly as `run_local_commit` always has; the + only new behavior `policy` can add is an additional remote-target lock, + an optional remote-staleness check at two boundaries (before establishing + evidence for a fresh review, and before starting a fix attempt), and a + publish step that runs only once, immediately after a clean review, in + place of `local_commit`'s unconditional "publish nothing" return. Callers + never construct `_Policy` themselves except `run_local_commit` + (`_LOCAL_COMMIT_POLICY`, every hook `None`) and `update_pr.run_update_pr` + (a populated `_Policy` built from its own resolved publication target). + """ 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) + lock_cm = LE.acquire_candidate_locks( + common_dir, local_ref, remote_target=policy.remote_target + ) try: lock_cm.__enter__() except LE.CandidateBusyError as exc: @@ -647,6 +842,7 @@ def run_local_commit( repo=repo, reason="candidate_busy", operator_action=f"the candidate lock is already held: {exc}", + policy_name=policy.name, ) try: @@ -654,7 +850,7 @@ def run_local_commit( live_status = LE.worktree_status(repo) if not LE.is_clean(live_status): raise LocalCommitError( - "worktree must be clean before a local_commit invocation" + f"worktree must be clean before a {policy.name} invocation" ) state = _State( @@ -667,6 +863,7 @@ def run_local_commit( base_ref=candidate["comparison_base"]["ref"], current_base_sha=candidate["comparison_base"]["sha"], original_budget=invocation["fix_cycle_budget"]["max_fix_cycles"], + policy=policy, ) if resume_checkpoint is not None: @@ -818,6 +1015,23 @@ def run_local_commit( while True: if pending_finding is None: + if state.policy.check_remote is not None: + remote_check = state.policy.check_remote(state) + if remote_check is not None: + return _finalize( + state, + terminal_state="blocked", + reason=remote_check["reason"], + operator_action=( + "the remote head for this candidate advanced before " + "this invocation could converge; reconcile the two " + "candidates manually before retrying" + ), + phase="establish_evidence", + publication_override=_remote_check_publication_override( + state, remote_check + ), + ) validation_outcomes = _run_validation_suite( invocation, cwd=state.repo, run_validation=run_validation ) @@ -963,22 +1177,106 @@ def run_local_commit( if record["aggregate_verdict"] == "clean": state.review_records.append(record) + converged_created_commits = state.head_history[1:] + if state.policy.publish is None: + _persist_checkpoint( + state, phase="return", next_action="none; converged" + ) + 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", + ) + + # `update_pr`: the review/fix/converge machinery above is + # identical to `local_commit`'s; only the publication tail + # differs. Every ordinary iteration cycle stayed local — + # this is the one and only remote write this invocation + # ever attempts, and only now that review is clean. + outcome = policy.publish(state) + if outcome.status == "published": + publication_override: dict[str, Any] = { + "policy": policy.name, + "status": "published", + "non_converged_exposure": outcome.non_converged_exposure, + } + else: + # Mirror `_default_publication`'s own + # blocked-reason rule rather than trusting + # `outcome.status` directly: only + # `remote_advanced`/`publication_failed` mean a + # publication attempt itself did not land + # (`failed`); every other blocked reason this + # publish step can return (for example a local + # `candidate_integrity_failure` ancestry check + # that never even attempted a push) is + # `withheld`, exactly like every other blocked + # reason the engine returns. + publication_override = dict( + _default_publication( + policy.name, + terminal_state="blocked", + reason=outcome.blocked_reason, + ) + ) + publication_override["non_converged_exposure"] = ( + outcome.non_converged_exposure + ) + if outcome.remote_head_before is not None: + publication_override["remote_head_before"] = ( + outcome.remote_head_before + ) + if outcome.remote_head_after is not None: + publication_override["remote_head_after"] = ( + outcome.remote_head_after + ) + + if outcome.status == "published": + source_override = None + if outcome.remote_head_after is not None: + original_source_sha = invocation["candidate"][ + "source_binding" + ]["observed_object_id"] + source_override = { + "status": "bound", + "initial_head": original_source_sha, + "final_head": outcome.remote_head_after, + "ahead_by": 0, + "behind_by": 0, + } + _persist_checkpoint( + state, + phase="return", + next_action="none; converged and published", + ) + return _finalize( + state, + terminal_state="converged", + reason=None, + operator_action=outcome.operator_action, + phase="return", + publication_override=publication_override, + source_override=source_override, + ) + _persist_checkpoint( - state, phase="return", next_action="none; converged" + state, phase="publish", next_action=outcome.operator_action ) - 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", + terminal_state="blocked", + reason=outcome.blocked_reason, + operator_action=outcome.operator_action, + phase="publish", + publication_override=publication_override, ) gating_ids = frozenset( @@ -1143,6 +1441,24 @@ def run_local_commit( phase="fix", ) + if state.policy.check_remote is not None: + remote_check = state.policy.check_remote(state) + if remote_check is not None: + return _finalize( + state, + terminal_state="blocked", + reason=remote_check["reason"], + operator_action=( + "the remote head for this candidate advanced before " + "this fix attempt could start; reconcile the two " + "candidates manually before retrying" + ), + phase="fix", + publication_override=_remote_check_publication_override( + state, remote_check + ), + ) + started_from_head = state.current_head sequence = attempt_sequence attempt_sequence += 1 diff --git a/skills/review-fix-loop/scripts/tests/test_update_pr.py b/skills/review-fix-loop/scripts/tests/test_update_pr.py new file mode 100644 index 0000000..2896738 --- /dev/null +++ b/skills/review-fix-loop/scripts/tests/test_update_pr.py @@ -0,0 +1,763 @@ +"""Disposable-remote integration tests for the standalone `update_pr` workflow +(issue #100). + +Every test drives `update_pr.run_update_pr` against a real temporary Git +repository *and* a real disposable local bare repository used as the +publication remote (`git init --bare`, addressed by its filesystem path — +never a configured named remote, never this repository's actual `origin`), +matching `carve-changesets/scripts/tests/helpers.py`'s established +"disposable local remote" convention and this module's own "no mocked Git +state" convention. No test in this file, and no code path `update_pr.py` +itself can reach, ever touches this repository's real `origin` remote. + +Covers the ticket's required scenarios: a successful converge-then-publish +run; a stale expected-old remote state; local non-fast-forward history +relative to the recorded expected-old head; a misconfigured ("wrong") +publication target; a mismatched remote-iteration grant; and an unreachable +remote. Also covers the remote-target lock actually being exercised through +`run_update_pr` (not just `local_execution.py`'s own unit tests) and that a +well-formed grant does not itself block a run. +""" + +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 + + +UP = _load("review_fix_loop_update_pr", "update_pr.py") +LC = UP.LC +LE = UP.LE +VALIDATE = UP.VALIDATE + + +# --------------------------------------------------------------------------- +# Repository + disposable remote 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 init_bare_remote(root: Path, *, name: str = "remote.git") -> Path: + bare = root / name + LE.git("init", "-q", "--bare", str(bare)) + return bare + + +def start_candidate( + repo: Path, + bare: Path, + *, + branch: str = "fix/100-example", + marker: str = "broken", + validation_flag: str = "pass", + push_head: bool = True, +) -> tuple[str, str]: + """Create `branch` off `main` with one commit, and (by default) push it + to `bare` at the same ref name — establishing the "existing pull request" + state `update_pr` requires an expected old head for. 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) + if push_head: + LE.git("push", str(bare), f"{branch}:refs/heads/{branch}", cwd=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"}, +] + + +def make_invocation( + repo: Path, + bare: Path, + *, + branch: str, + base_sha: str, + head_sha: str, + invocation_id: str = "update-pr-test", + max_fix_cycles: int = 3, + validation: list[dict[str, str]] | None = None, + grants: list[dict[str, str]] | None = None, + head_repository: str = "shaug/agent-scripts", + source_repository: str | None = None, + remote_url: 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) + head_ref = f"refs/heads/{branch}" + publication: dict[str, Any] = { + "policy": "update_pr", + "pull_request": { + "head_repository": head_repository, + "head_ref": head_ref, + "expected_old_head_sha": head_sha, + "base_ref": "main", + "base_sha": base_sha, + }, + } + if grants is not None: + publication["remote_iteration_grants"] = grants + 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, + "pull_request": {"repository": "shaug/agent-scripts", "number": 123}, + "source_binding": { + "repository": source_repository or head_repository, + "remote_url": remote_url or str(bare), + "ref": head_ref, + "observed_object_id": head_sha, + }, + }, + "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": publication, + } + + +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, mirroring + `test_local_commit.py`'s own fixture of the same name.""" + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + 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 make_clean_reviewer(): + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + del packet, briefing, independence, sequence + 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) + + 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 accepting_decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="accepted", rationale=f"{finding['id']} is tractable" + ) + + +class UpdatePrRepoTestCase(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.bare = init_bare_remote(Path(self.tmp.name)) + + +# --------------------------------------------------------------------------- +# Success +# --------------------------------------------------------------------------- + + +class SuccessTests(UpdatePrRepoTestCase): + def test_converges_and_publishes_the_exact_expected_old_fast_forward(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="broken") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + ) + result = UP.run_update_pr( + 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["publication"]["status"], "published") + self.assertEqual(result["publication"]["remote_head_before"], head_sha) + self.assertEqual( + result["publication"]["remote_head_after"], result["head"]["final"] + ) + self.assertEqual(result["unpushed_commits"], []) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + # The disposable remote itself actually advanced to the converged head. + remote_head = LE.git( + "ls-remote", str(self.bare), "refs/heads/fix/100-example", cwd=self.repo + ).stdout.split()[0] + self.assertEqual(remote_head, result["head"]["final"]) + + def test_immediate_convergence_with_no_fix_cycle_still_publishes(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + ) + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(result["publication"]["status"], "published") + self.assertEqual(result["created_commits"], []) + self.assertEqual(result["publication"]["remote_head_before"], head_sha) + self.assertEqual(result["publication"]["remote_head_after"], head_sha) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + def test_a_well_formed_grant_does_not_block_publication(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + grants=[ + { + "mechanism_id": "ci-check", + "kind": "external_ci", + "repository": "shaug/agent-scripts", + "ref": "refs/heads/fix/100-example", + "origin_only_evidence": "CI only evaluates the pushed remote ref", + } + ], + ) + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(result["publication"]["status"], "published") + + +# --------------------------------------------------------------------------- +# Stale expected-old state (someone else's commit already sits on the remote) +# --------------------------------------------------------------------------- + + +class StaleExpectedOldTests(UpdatePrRepoTestCase): + def test_a_competing_remote_update_cannot_be_overwritten(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + ) + + # Simulate another clone winning the race: a second local clone + # commits on top of the same expected-old head and pushes first. + other_clone = Path(self.tmp.name) / "other-clone" + LE.git("clone", "-q", str(self.repo), str(other_clone)) + LE.git("checkout", "-q", "fix/100-example", cwd=other_clone) + (other_clone / "marker.txt").write_text("someone-else-fixed-it\n") + LE.git("add", "-A", cwd=other_clone) + LE.git( + "-c", + "user.email=other@example.com", + "-c", + "user.name=Other", + "commit", + "-q", + "-m", + "a competing fix", + cwd=other_clone, + ) + LE.git( + "push", + str(self.bare), + "fix/100-example:refs/heads/fix/100-example", + cwd=other_clone, + ) + competing_head = LE.current_head(other_clone) + self.assertNotEqual(competing_head, head_sha) + + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "remote_advanced") + self.assertEqual(result["publication"]["status"], "failed") + self.assertEqual(result["publication"]["remote_head_before"], competing_head) + # Our own converged local commit is preserved, not lost or overwritten. + self.assertEqual(result["head"]["final"], head_sha) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + # The remote still holds the competing candidate's head, untouched. + remote_head = LE.git( + "ls-remote", str(self.bare), "refs/heads/fix/100-example", cwd=self.repo + ).stdout.split()[0] + self.assertEqual(remote_head, competing_head) + + +# --------------------------------------------------------------------------- +# Non-fast-forward history (this invocation's own recorded expected-old head +# is not actually an ancestor of its local candidate) +# --------------------------------------------------------------------------- + + +class NonFastForwardHistoryTests(UpdatePrRepoTestCase): + def test_local_candidate_not_descending_from_expected_old_fails_closed(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + + # A sibling branch's commit, unrelated to fix/100-example's real + # history, used as a deliberately wrong `expected_old_head_sha` — the + # local candidate cannot possibly be a descendant of it. + LE.git("checkout", "-q", "-b", "unrelated", base_sha, cwd=self.repo) + (self.repo / "unrelated.txt").write_text("unrelated\n") + LE.git("add", "-A", cwd=self.repo) + LE.git("commit", "-q", "-m", "unrelated commit", cwd=self.repo) + unrelated_sha = LE.current_head(self.repo) + LE.git("checkout", "-q", "fix/100-example", cwd=self.repo) + + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + ) + invocation["publication"]["pull_request"]["expected_old_head_sha"] = ( + unrelated_sha + ) + # The remote must still report the (now-wrong) expected value so the + # pre-push staleness check passes and the ancestry check is reached. + LE.git( + "push", + "--force", + str(self.bare), + f"{unrelated_sha}:refs/heads/fix/100-example", + cwd=self.repo, + ) + + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "candidate_integrity_failure") + # No push was ever attempted (the ancestry check fires first), so + # this is `withheld`, not `failed` — matching every other blocked + # reason outside `remote_advanced`/`publication_failed`. + self.assertEqual(result["publication"]["status"], "withheld") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + # Nothing was pushed: the remote still holds the unrelated commit. + remote_head = LE.git( + "ls-remote", str(self.bare), "refs/heads/fix/100-example", cwd=self.repo + ).stdout.split()[0] + self.assertEqual(remote_head, unrelated_sha) + + +# --------------------------------------------------------------------------- +# Wrong target (the invocation's own contract disagrees with itself) +# --------------------------------------------------------------------------- + + +class WrongTargetTests(UpdatePrRepoTestCase): + def test_source_binding_repository_mismatch_fails_closed_before_any_lock(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + head_repository="shaug/agent-scripts", + source_repository="someone-else/a-fork", + ) + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "missing_authority") + self.assertEqual(result["publication"]["status"], "withheld") + self.assertEqual(result["budget"]["consumed_cycles"], 0) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + # No lock was ever taken and no remote was ever touched: the target + # was rejected before "Resolve" could finish. + remote_head = LE.git( + "ls-remote", str(self.bare), "refs/heads/fix/100-example", cwd=self.repo + ).stdout.split()[0] + self.assertEqual(remote_head, head_sha) + + def test_fork_target_is_resolved_explicitly_without_assuming_origin(self): + """A consistent fork target (source_binding and pull_request agree, + and the remote_url is genuinely a different repository's remote, + never this repo's own origin) must publish exactly as a + same-repository branch would.""" + fork_bare = init_bare_remote(Path(self.tmp.name), name="fork.git") + base_sha, head_sha = start_candidate( + self.repo, fork_bare, marker="fixed", branch="fix/100-fork-example" + ) + invocation = make_invocation( + self.repo, + fork_bare, + branch="fix/100-fork-example", + base_sha=base_sha, + head_sha=head_sha, + head_repository="contributor/agent-scripts-fork", + source_repository="contributor/agent-scripts-fork", + ) + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "converged") + self.assertEqual(result["publication"]["status"], "published") + remote_head = LE.git( + "ls-remote", + str(fork_bare), + "refs/heads/fix/100-fork-example", + cwd=self.repo, + ).stdout.split()[0] + self.assertEqual(remote_head, result["head"]["final"]) + # The (unrelated, untouched) main-repository bare remote never saw + # this ref at all — proof this never fell back to "origin." + main_repo_refs = LE.git( + "ls-remote", + str(self.bare), + "refs/heads/fix/100-fork-example", + cwd=self.repo, + ).stdout.strip() + self.assertEqual(main_repo_refs, "") + + +# --------------------------------------------------------------------------- +# Missing / mismatched grants +# --------------------------------------------------------------------------- + + +class MismatchedGrantTests(UpdatePrRepoTestCase): + def test_a_grant_for_a_different_ref_fails_closed_before_any_lock(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + grants=[ + { + "mechanism_id": "ci-check", + "kind": "external_ci", + "repository": "shaug/agent-scripts", + "ref": "refs/heads/some-other-branch", + "origin_only_evidence": "CI only evaluates the pushed remote ref", + } + ], + ) + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "missing_authority") + self.assertIn("some-other-branch", result["operator_action"]) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + remote_head = LE.git( + "ls-remote", str(self.bare), "refs/heads/fix/100-example", cwd=self.repo + ).stdout.split()[0] + self.assertEqual(remote_head, head_sha) + + +# --------------------------------------------------------------------------- +# Remote failure +# --------------------------------------------------------------------------- + + +class RemoteFailureTests(UpdatePrRepoTestCase): + def test_an_unreachable_remote_fails_closed_at_publish_without_losing_the_commit( + self, + ): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="broken") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + remote_url=str(Path(self.tmp.name) / "does-not-exist.git"), + ) + result = UP.run_update_pr( + 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"], "publication_failed") + self.assertEqual(result["publication"]["status"], "failed") + # The fix cycle still ran and converged locally; only publication + # itself failed, and the converged commit is preserved and reported. + self.assertEqual(len(result["created_commits"]), 1) + self.assertEqual(result["unpushed_commits"], result["created_commits"]) + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + self.assertEqual(LE.current_head(self.repo), result["head"]["final"]) + + +# --------------------------------------------------------------------------- +# Locking actually wired through `run_update_pr` +# --------------------------------------------------------------------------- + + +class LockingTests(UpdatePrRepoTestCase): + def test_remote_target_lock_already_held_blocks_without_mutation(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-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/100-example", + remote_target=("shaug/agent-scripts", "refs/heads/fix/100-example"), + ): + result = UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + self.assertEqual(result["terminal_state"], "blocked") + self.assertEqual(result["reason"], "candidate_busy") + self.assertEqual(result["publication"]["status"], "withheld") + self.assertEqual(VALIDATE.validate_terminal_result(result), []) + + +# --------------------------------------------------------------------------- +# Input validation (caller/programming errors, not structured stops) +# --------------------------------------------------------------------------- + + +class InputValidationTests(UpdatePrRepoTestCase): + def test_rejects_local_commit_policy(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + ) + invocation["publication"] = {"policy": "local_commit"} + del invocation["candidate"]["source_binding"] + invocation["candidate"]["source_unavailable_reason"] = "no source" + with self.assertRaises(UP.UpdatePrError): + UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + + def test_rejects_invalid_invocation(self): + base_sha, head_sha = start_candidate(self.repo, self.bare, marker="fixed") + invocation = make_invocation( + self.repo, + self.bare, + branch="fix/100-example", + base_sha=base_sha, + head_sha=head_sha, + ) + del invocation["fix_cycle_budget"] + with self.assertRaises(UP.UpdatePrError): + UP.run_update_pr( + invocation, + repo=self.repo, + reviewer=make_clean_reviewer(), + decide=accepting_decide, + apply_fix=fixing_apply_fix, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/skills/review-fix-loop/scripts/update_pr.py b/skills/review-fix-loop/scripts/update_pr.py new file mode 100644 index 0000000..1d27e04 --- /dev/null +++ b/skills/review-fix-loop/scripts/update_pr.py @@ -0,0 +1,524 @@ +#!/usr/bin/env python3 +"""Standalone `update_pr` publication tail for `review-fix-loop` (issue #100). + +Composes the exact same review/fix/converge engine `local_commit.py` (issue +#99) already implements — this module never reimplements Resolve/Establish +evidence/Review/Decide/Fix/Validate and commit/Invalidate and repeat; it +supplies `local_commit._run_engine` with a populated `local_commit._Policy` +built from this invocation's resolved publication target, so every +intermediate fix commit stays local, exactly as under `local_commit`, until +the aggregate review is clean. Only the final "Publish" workflow step differs: +after convergence, this module performs the design's exact expected-old, +fast-forward-only Git update (`design/review-fix-loop.md`, "Publication +policy" > `update_pr`) instead of `local_commit`'s unconditional "publish +nothing." + +## What this module adds on top of the shared engine + +- `resolve_publication_target` — resolves and cross-validates the fork/remote + publication target from `candidate.source_binding` (the actual pushable + remote URL and ref, which may be a fork, never assumed to be "origin") and + `publication.pull_request` (the PR's own head-repository/head-ref/expected- + old identity), per design's "Resolve fork and remote publication targets + explicitly." A mismatch between the two — this invocation's own contract + disagreeing with itself about what it is allowed to push — fails closed + with `blocked/missing_authority` before any lock, review, or mutation. +- `validate_remote_iteration_grants` — validates every + `publication.remote_iteration_grants` entry (this ticket's "Require and + validate the origin-visible grants described by the design") resolves to + the same repository and fully qualified ref as the resolved publication + target. This module does not implement invoking any origin-visible- + exception mechanism itself (no host port in this ticket's scope needs + mid-loop remote visibility); it only validates grant structure so a stale + or mismatched grant fails closed rather than silently being accepted. +- A `check_remote` policy hook: at the two loop boundaries + `local_commit._run_engine` already calls it from (before establishing + evidence for a fresh review, and before starting a fix attempt), reread the + live remote head via `git ls-remote` and stop with + `blocked/remote_advanced` if it no longer matches the invocation's recorded + `expected_old_head_sha`. A transient remote-query failure here does not + itself stop the invocation — ordinary iteration stays entirely local and + only the final publish step's own failure is authoritative — so a + momentarily unreachable remote never blocks otherwise-convergent local + work. +- A `publish` policy hook, invoked exactly once, only immediately after the + aggregate review is clean: fetch and reread the exact remote head; require + it to equal `expected_old_head_sha`; prove the local candidate is a + non-rewriting descendant of it; perform one + `--force-with-lease=:` push; and read the + ref back to confirm it now equals the converged local head. Any failure at + any of those steps returns a `blocked` outcome that preserves the converged + local commit exactly as `local_commit` always would — this module never + merges, rebases, force-updates, or otherwise supersedes a competing + candidate. + +## Host boundary + +Identical to `local_commit.py`'s: `reviewer`, `decide`, and `apply_fix` are +still the caller-supplied ports for the three genuinely host/runtime actions. +This module adds no new host port — resolving the publication target, +checking the remote, and publishing are all real Git operations this +dependency-free module performs itself via `local_execution.py`'s bundled +`git` helper, exactly as `local_commit.py` already does for every other Git +operation. No test in this skill's suite, and no code path in this module, +ever touches this repository's real `origin` remote — every test drives a +disposable local (`file://`-equivalent path) bare repository, matching +`carve-changesets/scripts/tests/helpers.py`'s established convention. +""" + +from __future__ import annotations + +import dataclasses +import sys +from importlib import util as importlib_util +from pathlib import Path +from typing import Any, Mapping + +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) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +# `local_commit.py` already loads its own bundled `validate.py` and +# `local_execution.py` as `LC.VALIDATE`/`LC.LE`; reuse those exact instances +# rather than loading a second, separate copy of either module. +LC = _load_bundled_module( + "review_fix_loop_local_commit_for_update_pr", HERE / "local_commit.py" +) +VALIDATE = LC.VALIDATE +LE = LC.LE + +# Re-exported so a caller can build ports without importing local_commit.py +# directly. +ReviewPass = LC.ReviewPass +FixDecision = LC.FixDecision +ValidationOutcome = LC.ValidationOutcome +default_run_validation = LC.default_run_validation + + +class UpdatePrError(LC.LocalCommitError): + """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 `update_pr`) — never for an + ordinary runtime stop condition, which is always a structured terminal + result instead, matching `local_commit.LocalCommitError`'s own + convention. `isinstance`-compatible with `LocalCommitError` since both + entry points share the same internal engine and its internal-error + raises. + """ + + +# --------------------------------------------------------------------------- +# Publication target resolution ("Resolve fork and remote publication targets +# explicitly") +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class PublicationTarget: + """The resolved, cross-validated `update_pr` publication target. + + `remote_url` and `head_ref` are the actual pushable location — always + taken from `candidate.source_binding`, never assumed to be this + repository's own `origin` remote, so a fork's own remote URL is used + exactly as a same-repository branch's would be. `repository` is the + identity string shared by `source_binding.repository` and + `publication.pull_request.head_repository` once cross-validated. + """ + + repository: str + remote_url: str + head_ref: str + expected_old_head_sha: str + base_ref: str + base_sha: str + + +class TargetResolutionError(RuntimeError): + """The invocation's own publication fields disagree with each other, or a + remote-iteration grant does not resolve to the same target. + + Always converted to a structured `blocked/missing_authority` terminal + result by this module's callers — never raised across the + `run_update_pr` boundary — per the design's "Publication fails closed + when grants or target identity are missing or stale" requirement. + """ + + def __init__(self, message: str): + super().__init__(message) + + +def resolve_publication_target(invocation: Mapping[str, Any]) -> PublicationTarget: + """Resolve and cross-validate this invocation's publication target. + + `validate.validate_invocation` already requires `update_pr` to carry both + `candidate.source_binding` and `publication.pull_request`; this function + checks the one thing schema validation cannot: that the two halves of the + invocation's own contract agree about which repository and ref are being + published to. A same-repository branch and a fork both resolve + identically here — this function never special-cases "origin." + """ + source_binding = invocation["candidate"]["source_binding"] + pull_request = invocation["publication"]["pull_request"] + + if source_binding["repository"] != pull_request["head_repository"]: + raise TargetResolutionError( + "candidate.source_binding.repository " + f"{source_binding['repository']!r} does not match " + "publication.pull_request.head_repository " + f"{pull_request['head_repository']!r}; refusing to resolve an " + "ambiguous publication target" + ) + if source_binding["ref"] != pull_request["head_ref"]: + raise TargetResolutionError( + f"candidate.source_binding.ref {source_binding['ref']!r} does not " + "match publication.pull_request.head_ref " + f"{pull_request['head_ref']!r}; refusing to resolve an ambiguous " + "publication target" + ) + if not pull_request["head_ref"].startswith("refs/"): + raise TargetResolutionError( + "publication.pull_request.head_ref " + f"{pull_request['head_ref']!r} is not a fully qualified ref" + ) + + return PublicationTarget( + repository=source_binding["repository"], + remote_url=source_binding["remote_url"], + head_ref=pull_request["head_ref"], + expected_old_head_sha=pull_request["expected_old_head_sha"], + base_ref=pull_request["base_ref"], + base_sha=pull_request["base_sha"], + ) + + +def validate_remote_iteration_grants( + invocation: Mapping[str, Any], target: PublicationTarget +) -> list[str]: + """Validate every `remote_iteration_grants` entry against the resolved + target ("Require and validate the origin-visible grants described by the + design"). + + This module does not implement invoking any origin-visible-exception + mechanism (design's "Origin-visible exception"): no host port in this + ticket's scope demonstrably requires an origin-visible head mid-loop. + Every grant is still validated so a stale or mismatched one fails closed + rather than being silently accepted — "Unknown mechanisms, + repository/ref mismatches, missing evidence ... fail closed without a + remote write." + """ + errors: list[str] = [] + for grant in invocation["publication"].get("remote_iteration_grants", []): + if grant["repository"] != target.repository: + errors.append( + f"remote_iteration_grants[{grant['mechanism_id']!r}]: " + f"repository {grant['repository']!r} does not match the " + f"resolved publication target {target.repository!r}" + ) + if grant["ref"] != target.head_ref: + errors.append( + f"remote_iteration_grants[{grant['mechanism_id']!r}]: ref " + f"{grant['ref']!r} does not match the resolved publication " + f"target ref {target.head_ref!r}" + ) + elif not grant["ref"].startswith("refs/"): + errors.append( + f"remote_iteration_grants[{grant['mechanism_id']!r}]: ref " + f"{grant['ref']!r} is not a fully qualified ref" + ) + return errors + + +# --------------------------------------------------------------------------- +# Remote primitives +# --------------------------------------------------------------------------- + + +class _RemoteQueryError(RuntimeError): + """`git ls-remote` itself failed (network, auth, or a nonexistent remote). + + Caught by every caller in this module; never propagates past this + module's own policy hooks. A remote-query failure during ordinary + iteration is tolerated (`_check_remote` treats it as "not observably + advanced"); the same failure during the publish step is definitive and + becomes `blocked/publication_failed`. + """ + + +def _ls_remote(remote_url: str, ref: str, *, cwd: Path) -> str | None: + """Return the exact object ID `ref` resolves to on `remote_url`, or `None` + if the remote has no such ref. Never mutates any local ref or the + canonical worktree; safe to call at any point in the loop.""" + result = LE.git("ls-remote", remote_url, ref, cwd=cwd, check=False) + if result.returncode != 0: + detail = (result.stderr or result.stdout or "").strip() + raise _RemoteQueryError( + f"git ls-remote {remote_url} {ref} failed: " + f"{detail or f'exit code {result.returncode}'}" + ) + line = result.stdout.strip() + if not line: + return None + return line.split()[0] + + +# --------------------------------------------------------------------------- +# Policy hooks +# --------------------------------------------------------------------------- + + +def _make_check_remote(target: PublicationTarget, repo: Path): + def check_remote(state: Any) -> dict[str, Any] | None: + del state # only the target's static identity matters for this check + try: + live = _ls_remote(target.remote_url, target.head_ref, cwd=repo) + except _RemoteQueryError: + # Tolerated: ordinary iteration never depends on remote + # reachability. Only the publish step's own failure handling is + # authoritative for a genuinely unreachable remote. + return None + if live is not None and live != target.expected_old_head_sha: + return {"reason": "remote_advanced", "remote_head": live} + return None + + return check_remote + + +def _make_publish(target: PublicationTarget, repo: Path): + def publish(state: Any) -> LC._PublishOutcome: + try: + remote_before = _ls_remote(target.remote_url, target.head_ref, cwd=repo) + except _RemoteQueryError as exc: + return LC._PublishOutcome( + status="failed", + blocked_reason="publication_failed", + operator_action=( + "could not query the remote head before publishing: " + f"{exc}; the converged commit {state.current_head} " + "remains local and unpushed" + ), + ) + + if remote_before is None: + return LC._PublishOutcome( + status="failed", + blocked_reason="publication_failed", + operator_action=( + f"the remote ref {target.head_ref} does not exist on " + f"{target.repository}; cannot verify the expected old head " + f"before publishing. The converged commit " + f"{state.current_head} remains local and unpushed." + ), + ) + if remote_before != target.expected_old_head_sha: + return LC._PublishOutcome( + status="failed", + blocked_reason="remote_advanced", + remote_head_before=remote_before, + remote_head_after=remote_before, + operator_action=( + f"the remote head for {target.head_ref} is " + f"{remote_before!r}, not the expected old head " + f"{target.expected_old_head_sha!r}; another clone won the " + f"publication race. Reconcile the two candidates " + f"manually; the converged commit {state.current_head} " + "remains local and unpushed." + ), + ) + + ancestry = LE.git( + "merge-base", + "--is-ancestor", + target.expected_old_head_sha, + state.current_head, + cwd=repo, + check=False, + ) + if ancestry.returncode != 0: + return LC._PublishOutcome( + status="failed", + blocked_reason="candidate_integrity_failure", + remote_head_before=remote_before, + remote_head_after=remote_before, + operator_action=( + f"local head {state.current_head} is not a descendant of " + f"the expected old head {target.expected_old_head_sha!r}; " + "refusing to publish over what would be a rewritten " + "history" + ), + ) + + refspec = f"{state.branch}:{target.head_ref}" + lease = f"--force-with-lease={target.head_ref}:{target.expected_old_head_sha}" + push_result = LE.git( + "push", target.remote_url, refspec, lease, cwd=repo, check=False + ) + if push_result.returncode != 0: + try: + remote_after = _ls_remote(target.remote_url, target.head_ref, cwd=repo) + except _RemoteQueryError: + remote_after = None + if ( + remote_after is not None + and remote_after != target.expected_old_head_sha + ): + return LC._PublishOutcome( + status="failed", + blocked_reason="remote_advanced", + remote_head_before=remote_before, + remote_head_after=remote_after, + operator_action=( + "the push was rejected: another clone advanced " + f"{target.head_ref} to {remote_after!r} before this " + "invocation published. Reconcile the two candidates " + f"manually; the converged commit {state.current_head} " + "remains local and unpushed." + ), + ) + detail = (push_result.stderr or push_result.stdout or "").strip() + return LC._PublishOutcome( + status="failed", + blocked_reason="publication_failed", + remote_head_before=remote_before, + remote_head_after=remote_after, + operator_action=( + f"the publication push failed: {detail or 'unknown error'}; " + f"the converged commit {state.current_head} remains local " + "and unpushed" + ), + ) + + try: + remote_after = _ls_remote(target.remote_url, target.head_ref, cwd=repo) + except _RemoteQueryError as exc: + return LC._PublishOutcome( + status="failed", + blocked_reason="publication_failed", + remote_head_before=remote_before, + operator_action=( + "the push appeared to succeed but the post-push readback " + f"failed: {exc}" + ), + ) + if remote_after != state.current_head: + return LC._PublishOutcome( + status="failed", + blocked_reason="publication_failed", + remote_head_before=remote_before, + remote_head_after=remote_after, + operator_action=( + f"post-push readback of {target.head_ref} returned " + f"{remote_after!r}, not the converged head " + f"{state.current_head!r}" + ), + ) + + return LC._PublishOutcome( + status="published", + remote_head_before=remote_before, + remote_head_after=remote_after, + non_converged_exposure=False, + operator_action=( + f"published: {target.head_ref} advanced from {remote_before} " + f"to {remote_after} on {target.repository}" + ), + ) + + return publish + + +# --------------------------------------------------------------------------- +# Entry point +# --------------------------------------------------------------------------- + + +def run_update_pr( + invocation: Mapping[str, Any], + *, + repo: Path, + reviewer: LC.ReviewerPort, + decide: LC.DeciderPort, + apply_fix: LC.FixerPort, + run_validation: LC.ValidationRunnerPort = LC.default_run_validation, + classify_validation_failure: LC.ValidationClassifierPort = LC._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 `update_pr` review-fix-loop invocation. + + Returns a schema-valid terminal-result document (`converged`, + `changes_remaining`, or `blocked`). Every intermediate fix commit stays + local until the aggregate review is clean; the exact one expected-old + fast-forward push described by `design/review-fix-loop.md`'s "Publication + policy" > `update_pr` happens at most once, immediately after that clean + review, never before. + + Raises `UpdatePrError` only for a caller/programming error (an invalid + invocation, or `publication.policy != "update_pr"`) — the same convention + `local_commit.run_local_commit` already uses. A resolvable-but-invalid + publication target (a repository/ref mismatch between + `candidate.source_binding` and `publication.pull_request`, or a + mismatched `remote_iteration_grants` entry) is not a programming error; + it returns a structured `blocked/missing_authority` terminal result + instead, per the design's fail-closed publication contract. + """ + try: + LC._validate_and_require_policy(invocation, "update_pr") + except LC.LocalCommitError as exc: + raise UpdatePrError(str(exc)) from exc + + try: + target = resolve_publication_target(invocation) + except TargetResolutionError as exc: + return LC._minimal_blocked_result( + invocation, + repo=repo, + reason="missing_authority", + operator_action=str(exc), + policy_name="update_pr", + ) + + grant_errors = validate_remote_iteration_grants(invocation, target) + if grant_errors: + return LC._minimal_blocked_result( + invocation, + repo=repo, + reason="missing_authority", + operator_action="; ".join(grant_errors), + policy_name="update_pr", + ) + + policy = LC._Policy( + name="update_pr", + remote_target=(target.repository, target.head_ref), + checkpoint_pull_request={ + "head_repository": target.repository, + "head_ref": target.head_ref, + "base_ref": target.base_ref, + }, + check_remote=_make_check_remote(target, repo), + publish=_make_publish(target, repo), + ) + + return LC._run_engine( + invocation, + repo=repo, + reviewer=reviewer, + decide=decide, + apply_fix=apply_fix, + run_validation=run_validation, + classify_validation_failure=classify_validation_failure, + host_supports_fresh_subagent=host_supports_fresh_subagent, + attempts_root=attempts_root, + resume_checkpoint=resume_checkpoint, + policy=policy, + ) From 50f124d0d3da7d31d75329eeeca5f21562d0e279 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Fri, 31 Jul 2026 00:20:56 -0700 Subject: [PATCH 2/2] fix(review-fix-loop): extract shared update_pr/local_commit test fixtures ## 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 --- CHANGELOG.md | 11 ++ .../review-fix-loop/references/update-pr.md | 40 ++-- .../review-fix-loop/scripts/tests/helpers.py | 183 ++++++++++++++++++ .../scripts/tests/test_local_commit.py | 150 +++----------- .../scripts/tests/test_update_pr.py | 174 +++-------------- 5 files changed, 261 insertions(+), 297 deletions(-) create mode 100644 skills/review-fix-loop/scripts/tests/helpers.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c84a0ad..87c70fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ summary: Chronological history of repository and skill changes. # Changelog +## 2026-07-31 — Recorded the first review-fix-loop `update_pr` fix cycle + +- fix(review-fix-loop): extract the test fixtures shared between + `test_local_commit.py` and `test_update_pr.py` (the module loader, a bare + local repository, the always-passing validation commands, the + marker-file-driven fake reviewer, and the accepting decider/fixer) into a + sibling `scripts/tests/helpers.py`, matching + `carve-changesets/scripts/tests/helpers.py`'s established precedent, closing + the one code-simplicity gap the first review-code-change pass on #100 found + ## 2026-07-30 — Delivered and evaluated the standalone review-fix-loop `update_pr` workflow, 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): add and evaluate the standalone `update_pr` workflow @@ -23,6 +33,7 @@ summary: Chronological history of repository and skill changes. generalizes `local_commit.py`'s internal loop into a policy-parameterized `_run_engine` both entry points share, with `run_local_commit`'s own behavior and its 21 existing tests unchanged + (`95ccb81142357cc5cc55e78150abd5b39fa0e0b1`) - 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 diff --git a/skills/review-fix-loop/references/update-pr.md b/skills/review-fix-loop/references/update-pr.md index 92b2312..76f0b1a 100644 --- a/skills/review-fix-loop/references/update-pr.md +++ b/skills/review-fix-loop/references/update-pr.md @@ -52,7 +52,7 @@ enforcement, or checkpoint/terminal-result assembly — it supplies a populated What issue #100 itself owns, and what did not exist before this module: -- `resolve_publication_target` — resolving and cross-validating the fork/ remote +- `resolve_publication_target` — resolving and cross-validating the fork/remote publication target ("Resolve fork and remote publication targets explicitly"). - `validate_remote_iteration_grants` — validating every `remote_iteration_grants` entry against that resolved target ("Require and @@ -199,19 +199,25 @@ either side of this rule updates both together. [`scripts/tests/test_update_pr.py`](../scripts/tests/test_update_pr.py) drives `run_update_pr` against a real temporary Git repository and a real disposable local bare repository used as the publication remote — never this repository's -actual `origin` — covering: 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 (and proof the main repository's own bare remote never saw the -fork's ref at all); a competing remote update that cannot be overwritten (a -second clone wins the race, and the converged local commit is preserved while -the remote is left holding the competitor's head); local non-fast-forward -history relative to the recorded expected-old head; a -source-binding/pull-request repository mismatch (a misconfigured target) failing -closed before any lock; a remote-iteration grant referencing the wrong ref -failing closed before any lock; an unreachable remote failing closed at the -publish step alone, without losing the already-converged local commit; the -`update_pr`-only remote-target lock actually being exercised through -`run_update_pr` (not only `local_execution.py`'s own unit tests); and rejection -of an invalid invocation or a `local_commit`-policy invocation at the API -boundary. +actual `origin`. Fixtures shared with `test_local_commit.py` — the module +loader, a bare local repository, the always-passing validation commands, the +marker-file-driven fake reviewer, and the accepting decider/fixer — live in this +sibling directory's own +[`scripts/tests/helpers.py`](../scripts/tests/helpers.py) rather than being +duplicated across both files, matching +`carve-changesets/scripts/tests/helpers.py`'s established precedent. Covers: 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 (and proof the main +repository's own bare remote never saw the fork's ref at all); a competing +remote update that cannot be overwritten (a second clone wins the race, and the +converged local commit is preserved while the remote is left holding the +competitor's head); local non-fast-forward history relative to the recorded +expected-old head; a source-binding/pull-request repository mismatch (a +misconfigured target) failing closed before any lock; a remote-iteration grant +referencing the wrong ref failing closed before any lock; an unreachable remote +failing closed at the publish step alone, without losing the already-converged +local commit; the `update_pr`-only remote-target lock actually being exercised +through `run_update_pr` (not only `local_execution.py`'s own unit tests); and +rejection of an invalid invocation or a `local_commit`-policy invocation at the +API boundary. diff --git a/skills/review-fix-loop/scripts/tests/helpers.py b/skills/review-fix-loop/scripts/tests/helpers.py new file mode 100644 index 0000000..66ef81a --- /dev/null +++ b/skills/review-fix-loop/scripts/tests/helpers.py @@ -0,0 +1,183 @@ +"""Shared test fixtures for review-fix-loop's `local_commit` and `update_pr` +end-to-end test suites (`test_local_commit.py`, `test_update_pr.py`). + +Mirrors this repository's own `carve-changesets/scripts/tests/helpers.py` +precedent: fixture plumbing used by more than one test module in the same +skill lives in one sibling `helpers.py`, so a real scenario difference +between the two suites shows up as a difference in test code, not as +copy-pasted setup that has to be kept in sync by hand. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from typing import Any + +SKILL_ROOT = Path(__file__).resolve().parents[2] + + +def load_module(name: str, filename: str): + """Load one of this skill's bundled `scripts/` modules under a + dedicated `sys.modules` name, matching the dependency-free + importlib-loading convention every script in this skill already uses.""" + 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 + + +# A dedicated `local_commit.py` load for this module's own use of `LE.git` +# and the `ReviewPass`/`FixDecision` dataclasses below. Every fixture here is +# duck-typed (attribute access only, never `isinstance`), so constructing a +# `ReviewPass`/`FixDecision` from this module's own load is exactly as valid +# to either suite's separately loaded `LC`/`UP.LC` as one built from that +# suite's own load — see `local_commit.py`'s own module docstring for why +# cross-module-instance duck-typing is safe throughout this skill's engine. +LC = load_module("review_fix_loop_test_helpers_local_commit", "local_commit.py") +LE = LC.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) + + +ALWAYS_PASS_VALIDATION = [ + {"name": "focused unit test", "command": "true", "scope": "focused"}, + {"name": "full repository gate", "command": "true", "scope": "full"}, +] + +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` once it reads + 'fixed', `changes_required` with one `FINDING_ID` finding otherwise. + Shared by both the `local_commit` and `update_pr` end-to-end suites.""" + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + 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 make_clean_reviewer(): + """A fake reviewer that always reports a clean aggregate verdict, + regardless of candidate content. Used where a suite only needs immediate + convergence and does not care about `marker.txt`'s content.""" + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + del packet, briefing, independence, sequence + 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) + + 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 accepting_decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="accepted", rationale=f"{finding['id']} is tractable" + ) diff --git a/skills/review-fix-loop/scripts/tests/test_local_commit.py b/skills/review-fix-loop/scripts/tests/test_local_commit.py index 0dfdb21..3863b5b 100644 --- a/skills/review-fix-loop/scripts/tests/test_local_commit.py +++ b/skills/review-fix-loop/scripts/tests/test_local_commit.py @@ -15,34 +15,39 @@ "unavailable" and "no tractable correction" shapes), operator input (rejected/deferred disposition and scope expansion), and recovery from an interrupted attempt. + +The module loader, a bare local repository, the always-passing validation +commands, the marker-file-driven fake reviewer, and the accepting +decider/fixer are shared with `test_update_pr.py` via this sibling +directory's own `helpers.py`, matching `carve-changesets/scripts/tests/ +helpers.py`'s established precedent for one skill's own shared test fixture +module. """ 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 +import helpers +LC = helpers.load_module("review_fix_loop_local_commit", "local_commit.py") +LE = helpers.load_module( + "review_fix_loop_local_execution_for_tests", "local_execution.py" +) +VALIDATE = helpers.load_module("review_fix_loop_validate_for_tests", "validate.py") -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") +# Fixtures shared with test_update_pr.py; see helpers.py. +init_repo = helpers.init_repo +ALWAYS_PASS_VALIDATION = helpers.ALWAYS_PASS_VALIDATION +CLEAN_TEMPLATE = helpers.CLEAN_TEMPLATE +FINDING_ID = helpers.FINDING_ID +_finding = helpers.finding +make_marker_reviewer = helpers.make_marker_reviewer +fixing_apply_fix = helpers.fixing_apply_fix +accepting_decide = helpers.accepting_decide # --------------------------------------------------------------------------- @@ -50,16 +55,6 @@ def _load(name: str, filename: str): # --------------------------------------------------------------------------- -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, *, @@ -79,11 +74,6 @@ def start_candidate( 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", @@ -162,108 +152,12 @@ def make_invocation( } -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() diff --git a/skills/review-fix-loop/scripts/tests/test_update_pr.py b/skills/review-fix-loop/scripts/tests/test_update_pr.py index 2896738..df97a8a 100644 --- a/skills/review-fix-loop/scripts/tests/test_update_pr.py +++ b/skills/review-fix-loop/scripts/tests/test_update_pr.py @@ -10,6 +10,15 @@ state" convention. No test in this file, and no code path `update_pr.py` itself can reach, ever touches this repository's real `origin` remote. +Fixtures shared with `test_local_commit.py` (the module loader, a bare local +repository, the always-passing validation commands, the marker-file-driven +fake reviewer, and the accepting decider/fixer) live in this sibling +directory's own `helpers.py` rather than being duplicated here — following +the same precedent `carve-changesets/scripts/tests/helpers.py` already set +for that skill. Only the parts genuinely specific to `update_pr` (the +disposable bare-remote fixture, `start_candidate`'s push, and +`make_invocation`'s publication/source-binding shape) live in this file. + Covers the ticket's required scenarios: a successful converge-then-publish run; a stale expected-old remote state; local non-fast-forward history relative to the recorded expected-old head; a misconfigured ("wrong") @@ -21,48 +30,35 @@ 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 - +import helpers -UP = _load("review_fix_loop_update_pr", "update_pr.py") +UP = helpers.load_module("review_fix_loop_update_pr", "update_pr.py") LC = UP.LC LE = UP.LE VALIDATE = UP.VALIDATE +# Fixtures shared with test_local_commit.py; see helpers.py. +init_repo = helpers.init_repo +ALWAYS_PASS_VALIDATION = helpers.ALWAYS_PASS_VALIDATION +CLEAN_TEMPLATE = helpers.CLEAN_TEMPLATE +FINDING_ID = helpers.FINDING_ID +_finding = helpers.finding +make_marker_reviewer = helpers.make_marker_reviewer +make_clean_reviewer = helpers.make_clean_reviewer +fixing_apply_fix = helpers.fixing_apply_fix +accepting_decide = helpers.accepting_decide + # --------------------------------------------------------------------------- # Repository + disposable remote 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 init_bare_remote(root: Path, *, name: str = "remote.git") -> Path: bare = root / name LE.git("init", "-q", "--bare", str(bare)) @@ -94,12 +90,6 @@ def start_candidate( return base_sha, head_sha -ALWAYS_PASS_VALIDATION = [ - {"name": "focused unit test", "command": "true", "scope": "focused"}, - {"name": "full repository gate", "command": "true", "scope": "full"}, -] - - def make_invocation( repo: Path, bare: Path, @@ -172,126 +162,6 @@ def make_invocation( } -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, mirroring - `test_local_commit.py`'s own fixture of the same name.""" - - def reviewer( - *, packet, briefing, head_sha, comparison_base_sha, independence, sequence - ): - 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 make_clean_reviewer(): - def reviewer( - *, packet, briefing, head_sha, comparison_base_sha, independence, sequence - ): - del packet, briefing, independence, sequence - 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) - - 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 accepting_decide(*, finding, change_contract, attempt_number): - del change_contract, attempt_number - return LC.FixDecision( - disposition="accepted", rationale=f"{finding['id']} is tractable" - ) - - class UpdatePrRepoTestCase(unittest.TestCase): def setUp(self): self.tmp = tempfile.TemporaryDirectory()