diff --git a/.changelogs/NEXT.md b/.changelogs/NEXT.md new file mode 100644 index 0000000..4a54b84 --- /dev/null +++ b/.changelogs/NEXT.md @@ -0,0 +1,4 @@ +# Unreleased Changes + +## PR review loop +- **[issue-134] Review fixes can no longer be left behind when a PR is opened or merged** — if a reviewer's fixes are committed but never pushed, the review loop now pushes them itself, `/do:pr` pushes anything still outstanding before opening the PR, and it refuses to merge while your branch is ahead of the remote, naming the commits that would have been dropped. Previously the fixes stayed on your machine while every reviewer reported clean and CI passed against the older pushed code. diff --git a/commands/do/pr.md b/commands/do/pr.md index beb24d0..8cc37fb 100644 --- a/commands/do/pr.md +++ b/commands/do/pr.md @@ -141,6 +141,7 @@ This phase drives the **multi-reviewer wrapper** (defined under "Reviewer loop b ## Open the PR +- **First, assert the branch's commits reached the remote.** The Local Code Review gate and every pre-PR local reviewer above commit their fixes onto this branch; if one of their push steps didn't run, `gh pr create` opens a PR containing only the pre-review commits and those findings never reach the PR at all. Confirm `git log --oneline @{u}..HEAD` is empty; if it isn't, push first (`git push origin {current_branch}` — an explicit refspec, never a bare `git push`, which under `push.default=matching` fans out to every same-named local branch — retrying once after `git pull --rebase --autostash` on a non-fast-forward) and only then create the PR. **If the push still fails after that one retry, do NOT create the PR** — print the unpushed SHAs and the push error and stop, exactly as the unpushed-commits merge gate below refuses to merge; a PR opened from a tree missing the review fixes is the precise failure this check exists to prevent. Skip the check when the branch has no upstream (`git rev-parse --abbrev-ref --symbolic-full-name @{u} >/dev/null 2>&1` fails — detached HEAD or no origin): there is nothing to compare against. Note `git status` is **not** a substitute — a clean working tree says nothing about committed-but-unpushed commits, which is exactly the state this catches. - Create a PR / merge request from `{current_branch}` to `{default_branch}`: - GitHub: `gh pr create --base {default_branch} --head {current_branch} --title "..." --body "..."` - GitLab: `glab mr create --source-branch {current_branch} --target-branch {default_branch} --title "..." --description "..."` (add `--yes` to skip the interactive prompt; `--remove-source-branch` if the project deletes merged branches) @@ -192,21 +193,34 @@ Otherwise combine `LOCAL_OVERALL_STATUS` (from "Pre-PR Local Reviews", or `clean **If `MERGE_ENABLED` is not `true`, skip this section** — report the PR/MR URL plus the review summary and stop. This is the historical `/do:pr` behavior: open the PR and hand it back for manual merge. -When `MERGE_ENABLED=true`, gate the merge on **both** the review result and CI: +When `MERGE_ENABLED=true`, gate the merge on **all three** of the review result, the unpushed-commits check, and CI: 1. **Review gate** — consume the review loop's `{OVERALL_STATUS}` exactly as `/do:release` does: - `clean` — eligible (this includes the no-reviewer path above, which set `OVERALL_STATUS=clean` on a passing Local Code Review gate, and copilot `too-large`, plus `capped` from any of the four loops — an explicitly configured cap, `~max=` or `--review-iterations`, reached after applying every fix; a *built-in* cap is `guardrail`, which is inconclusive). - `partial` — eligible only when an explicit `--review-stop-on-findings`/`--review-stop-on-clean` flag was set (the user opted into the short-circuit). - `inconclusive` or `dirty` — **do NOT merge.** Leave the PR open and report the proximate status + URL so the user can intervene. A requested reviewer that never produced a verdict is not a clean review. -2. **Resolve the merge method** into `{MERGE_METHOD}`: the explicit flag or saved `merge-method` default if set; otherwise query `gh repo view --json mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed` and pick from the repo's allowed methods — if exactly one is allowed use it; if several are, prefer `squash`, then `merge`, then `rebase`. State the chosen method. (GitLab: omit the method flag and let `glab` use the project default.) -3. **Merge once CI is green** — GitHub (`gh`): +2. **Unpushed-commits gate** — **refuse to merge while the local branch is ahead of its remote.** The PR-side reviewers above commit their fixes onto this branch, as does any fix you applied after the last push; if those commits never reached `origin`, the tree that was reviewed and CI-tested is not the tree the merge would land, and merging silently drops every one of those fixes while every signal still reads green. Check it: + + ```bash + if git rev-parse --abbrev-ref --symbolic-full-name @{u} >/dev/null 2>&1; then + UNPUSHED="$(git log --oneline @{u}..HEAD)" + if [ -n "$UNPUSHED" ]; then + echo "REFUSING TO MERGE — these commits are not on the remote:"; echo "$UNPUSHED" + exit 1 + fi + fi + ``` + + The snippet **fails closed on purpose**: it exits non-zero rather than just printing, so the gate can't be skimmed past the way a bare `git log` whose output needs interpreting can. If it exits non-zero, **do not merge**: report the unpushed SHAs, leave the PR open, and say the branch has local work to push. Skip the gate entirely when the branch has no upstream — there is nothing to compare against. This gate is independent of `{OVERALL_STATUS}`: a `clean` review whose fixes are unpushed is exactly the failure it exists to catch. +3. **Resolve the merge method** into `{MERGE_METHOD}`: the explicit flag or saved `merge-method` default if set; otherwise query `gh repo view --json mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed` and pick from the repo's allowed methods — if exactly one is allowed use it; if several are, prefer `squash`, then `merge`, then `rebase`. State the chosen method. (GitLab: omit the method flag and let `glab` use the project default.) +4. **Merge once CI is green** — GitHub (`gh`): - First try GitHub-native auto-merge, so the merge lands when required checks pass even if this session ends: `gh pr merge {number} --auto --{MERGE_METHOD} --delete-branch`. - If that errors because auto-merge is not enabled on the repo (e.g. `gh` reports auto-merge is not allowed / not enabled), **fall back to watching checks in-session, then merging directly**: `gh pr checks {number} --required --watch --fail-fast` — scope the watch to **required** checks only so an optional/non-required job's failure or slowness can't block a merge that branch protection would allow; on success run `gh pr merge {number} --{MERGE_METHOD} --delete-branch`. If a required check **fails**, apply the **CI flake handling** routine (one conservative re-run on the same commit — see `~/.claude/lib/ci-flake-handling.md`): if the same SHA passes on the single re-run, treat it as a flake and proceed with the merge (logging which check flaked); if it fails again, leave the PR open and report which check failed — do not merge. (If `gh` reports no required checks exist on the branch, the required-CI gate is vacuously satisfied — merge directly.) - GitLab (`glab`): `glab mr merge {number} --auto-merge --yes --remove-source-branch` (merges when the pipeline succeeds). If the installed `glab` doesn't support `--auto-merge`, fall back to polling `glab ci status` until the pipeline passes, then `glab mr merge {number} --yes`. -4. **Verify** the result: `gh pr view {number} --json state,mergedAt` (GitLab: `glab mr view {number}`). Distinguish *merged now* from *queued to auto-merge on green CI*. -5. After a **completed** merge, switch back and sync the default branch locally: `git checkout {default_branch} && git pull --rebase --autostash`. When the merge is merely **queued** (native auto-merge, checks still running), skip the local sync — the merge hasn't happened yet — and say so. +5. **Verify** the result: `gh pr view {number} --json state,mergedAt` (GitLab: `glab mr view {number}`). Distinguish *merged now* from *queued to auto-merge on green CI*. +6. After a **completed** merge, switch back and sync the default branch locally: `git checkout {default_branch} && git pull --rebase --autostash`. When the merge is merely **queued** (native auto-merge, checks still running), skip the local sync — the merge hasn't happened yet — and say so. -Never merge on `dirty`/`inconclusive`, never merge before required checks pass, and never override branch protection — `--auto` respects it, and the in-session fallback waits on `gh pr checks`. +Never merge on `dirty`/`inconclusive`, never merge while the branch has unpushed commits, never merge before required checks pass, and never override branch protection — `--auto` respects it, and the in-session fallback waits on `gh pr checks`. **Report the final status** to the user including the PR/MR URL, the multi-reviewer aggregate report (per-pass status table plus overall status), and — when merge mode was enabled — whether the PR merged, is queued to auto-merge on green CI, or was left open (with why). diff --git a/commands/do/release.md b/commands/do/release.md index 919ab21..e4c3637 100644 --- a/commands/do/release.md +++ b/commands/do/release.md @@ -203,7 +203,7 @@ The merge gate consumes the **wrapper's `{OVERALL_STATUS}`** plus, for any copil - `clean` — every executed pass returned `clean` (copilot `too-large`, and `capped` from any of the four loops, all count as clean here, per each loop's own rule; `capped` means an **explicitly configured** cap was reached after applying every fix — the default `--review-iterations 1` outcome on a GitHub-side pass, or a per-entry `~max=`. A *built-in* cap that cuts off a still-productive loop is `guardrail`, which is inconclusive below), **or** no external reviewer was requested (`--review-with` omitted → `REVIEW_AGENTS=[]`) and the Local Code Review gate plus build/tests passed (the no-review path set `OVERALL_STATUS=clean`). **Eligible to merge.** - `partial` — the wrapper stopped early because of an explicit stop-mode flag (`--review-stop-on-findings` or `--review-stop-on-clean`) and the executed passes all completed normally. **Eligible to merge** — the user opted into the short-circuit. -- `inconclusive` — the executed list contained **at least one** pass whose status was inconclusive (`timeout`, `error`, `guardrail`, `skipped`, `not-requestable` — an `@` whose request failed and never reviewed — `no-verdict` — a local agent that ran but did not answer in the verdict format — or ollama `incomplete` — a partially-reviewed diff), regardless of whether other passes returned `clean`. **Do NOT merge** — the user asked for multiple perspectives and at least one never produced a verdict. +- `inconclusive` — the executed list contained **at least one** pass whose status was inconclusive (`timeout`, `error`, `guardrail`, `skipped`, `not-requestable` — an `@` whose request failed and never reviewed — `no-verdict` — a local agent that ran but did not answer in the verdict format — ollama `incomplete` — a partially-reviewed diff — or `push-failed`, a pass whose fix commits never reached the remote, which counts here even on an `~opt` pass), regardless of whether other passes returned `clean`. **Do NOT merge** — the user asked for multiple perspectives and at least one never produced a verdict. - `dirty` — a pass returned a hard-error status (`cli-error`, `broken-build`, `test-failed`, `rejected`) and the wrapper short-circuited. **Do NOT merge.** For `dirty` or `inconclusive`: diff --git a/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index 57d7468..b084b59 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -16,7 +16,7 @@ The calling command must populate these before reaching this loop: Both suffixes are optional, order-independent, and repeat-free (at most one of each); see the two bullets below for their semantics. - `{REVIEW_MODELS}` — optional per-agent **default-model map** the calling command resolved from saved config (the `review-models` key, project-over-global merged per agent — see `~/.claude/lib/review-config-defaults.md`). Keys are agent slugs (`codex`/`claude`/`agy`/`grok`/`ollama`); values are model strings. It supplies an entry's model **only when that entry's `--review-with` token carried no `[]` bracket** — the bracket always wins. Absent/empty when the command resolved no saved default. Precedence per entry, top wins: explicit `[]` bracket → `{REVIEW_MODELS}[slug]` → the reviewer's built-in default. - - **Optional (non-blocking) marker `~opt`.** Any entry may carry a trailing `~opt` suffix — `ollama~opt`, `ollama[qwen2.5-coder:32b]~opt`, `@some-bot~opt`, `copilot~opt` — marking that reviewer **optional**: it is still requested, still runs, and its findings are still fixed exactly like a required reviewer, but an *inconclusive* result from it (timeout / skipped / incomplete / no-verdict) is **excluded from the merge gate** — it never flips `{OVERALL_STATUS}` to `inconclusive` and so never blocks `--merge`. This is for reviewers you want a second opinion from but that don't reliably return a verdict (e.g. a local Ollama model that sometimes emits no findings). The parser strips `~opt` into a per-entry boolean `{OPTIONAL}` (default `false`) **before** any slug/`[model]`/`@login` parsing; the `~opt` suffix is **not** part of the dedup identity, so `ollama~opt` and `ollama` are the same reviewer. A hard-error from an optional reviewer (`cli-error`/`broken-build`/`test-failed`/`rejected`) is **not** exempted — a broken tree still blocks regardless of optionality (see the aggregate rules). `~opt` is deliberately shell-metacharacter-free so a `--review-with` value stays inert wherever it lands in a command string. + - **Optional (non-blocking) marker `~opt`.** Any entry may carry a trailing `~opt` suffix — `ollama~opt`, `ollama[qwen2.5-coder:32b]~opt`, `@some-bot~opt`, `copilot~opt` — marking that reviewer **optional**: it is still requested, still runs, and its findings are still fixed exactly like a required reviewer, but an *inconclusive* result from it (timeout / skipped / incomplete / no-verdict) is **excluded from the merge gate** — it never flips `{OVERALL_STATUS}` to `inconclusive` and so never blocks `--merge`. This is for reviewers you want a second opinion from but that don't reliably return a verdict (e.g. a local Ollama model that sometimes emits no findings). The parser strips `~opt` into a per-entry boolean `{OPTIONAL}` (default `false`) **before** any slug/`[model]`/`@login` parsing; the `~opt` suffix is **not** part of the dedup identity, so `ollama~opt` and `ollama` are the same reviewer. Two things are **not** exempted (see the aggregate rules): a hard-error (`cli-error`/`broken-build`/`test-failed`/`rejected`) — a broken tree still blocks regardless of optionality — and `push-failed`, the wrapper's own status for a pass whose fix commits never reached the remote, since an optional reviewer's fixes stranded locally still mean the merged tree is not the reviewed tree. `~opt` is deliberately shell-metacharacter-free so a `--review-with` value stays inert wherever it lands in a command string. - **Per-reviewer iteration cap `~max=`.** Any entry may carry a trailing `~max=` suffix — `claude~max=2`, `ollama[qwen2.5-coder:32b]~max=1`, `@some-bot~max=3`, `copilot~max=0` — capping how many **review → fix → re-review cycles** that one reviewer runs before it stops. This is the per-entry form of `--review-iterations`, and unlike that flag it applies to **every** reviewer type, including the local agents (`codex`/`claude`/`agy`/`grok`) and `ollama` whose caps were otherwise fixed at 3. It is what makes a mixed run like `--review-with claude~max=2,ollama~max=1,codex~max=3` express a different budget per reviewer in one call. `` must be a **non-negative integer**: any `n ≥ 1` runs at most `n` cycles (still exiting early when a round is clean or the convergence gate converges), and `0` means "loop until that reviewer is clean", bounded by each inner loop's own 10-iteration safety guardrail. A reviewer that stops because it reached an **explicitly configured** `~max=` (n ≥ 1) with work still outstanding returns `capped`, which is **clean-equivalent for the merge gate** — you asked for exactly `n` rounds and got them, so the cap is not a failure. That is the difference between `~max` and the built-in caps: exhausting a *built-in* cap still returns `guardrail` (inconclusive, blocks the merge), because nobody chose that ceiling. The suffix chains freely with `~opt` in either order (`ollama~opt~max=1` ≡ `ollama~max=1~opt`); at most one `~max=` per entry. Like `~opt` it is **not** part of the dedup identity (`ollama~max=2` and `ollama` are the same reviewer) and is shell-metacharacter-free by design. - `{REVIEW_STOP_MODE}` — one of: - `all` (default) — run every listed reviewer in order, regardless of what each reports @@ -73,6 +73,65 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe `capped` means the same thing in all four loops: the reviewer reached an **explicitly configured** iteration budget (`{MAX_EXPLICIT}` true — a `~max=` suffix, or `--review-iterations` on a GitHub-side pass) after applying every fix it surfaced. It is **clean-equivalent for the merge gate** — the user asked for `n` rounds and got `n` rounds. `guardrail` is the opposite case and stays inconclusive: a *built-in* ceiling (the local-agent / Ollama default of 3, or the 10-iteration safety backstop in unlimited `~max=0` mode) cut off a loop that was still landing substantive findings, so nobody vouched for stopping there. 4. **Record the pass result** (status + number of new commits since `PASS_START_SHA` + this entry's `{OPTIONAL}` flag). Keep a per-pass row for the aggregate report. +5. **Assert the pass's fixes reached the remote.** Every inner loop pushes its own fix commits as its final step, but nothing downstream ever re-checks that it happened — so an orchestrator that improvises a loop body instead of running it step by step leaves the fixes committed *locally*, where `gh pr create` and the merge gate will never see them. Every reviewer still reports `clean`/`capped` and CI still passes (it is testing the pushed tree, which predates the fixes), so the failure is completely silent. Verify it here, once, for every reviewer type: + + ```bash + # Skip entirely when there is no upstream to compare against: /do:review and + # /do:better can run on a branch that was never pushed, and a detached HEAD or a + # missing origin fails this the same way. This is a "did the push step run" check, + # not a "must have a remote" requirement. + if git rev-parse --abbrev-ref --symbolic-full-name @{u} >/dev/null 2>&1; then + UNPUSHED="$(git log --oneline @{u}..HEAD)" + else + UNPUSHED="" # no upstream — nothing to assert + fi + # Derive the push target from CONFIG, not by splitting the abbrev-ref: a remote + # name may itself contain a slash (`up/stream/main`), and a LOCAL upstream + # (`branch..remote=.`, what `git branch --set-upstream-to=main` produces) + # abbreviates to a bare `main` with no slash at all — a `%%/`+`#*/` split mangles + # both into a bogus remote. Empty on a detached HEAD, which skips the check. + BR="$(git branch --show-current)" + PUSH_REMOTE="$(git config --get "branch.$BR.remote")" + PUSH_BRANCH="$(git config --get "branch.$BR.merge")" # already a full refs/heads/ + if [ -z "$PUSH_REMOTE" ] || [ "$PUSH_REMOTE" = "." ]; then + UNPUSHED="" # upstream is a local branch (or none) — no remote to assert against + fi + # Scope it to THIS pass. A pass that committed nothing has nothing of its own + # stranded, so anything ahead of the upstream predates the pass — deliberately + # unpushed local work the caller never asked this loop to publish. Use `if`, not + # `[ … ] && …`: as the block's last statement a false test would exit non-zero and + # read as a failed command to the orchestrator running this snippet. + if [ "$PASS_START_SHA" = "$(git rev-parse HEAD)" ]; then + UNPUSHED="" # pass committed nothing — nothing of its own is stranded + fi + # Push in the SAME shell that derived the target — these variables do not survive + # across separate Bash invocations, and an empty PUSH_REMOTE would push to the + # empty-string remote: a fatal error, reported as a bogus push-failed. + if [ -n "$UNPUSHED" ]; then + if ! git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"; then + # Never leave a conflicted rebase behind: `push-failed` is a continue-signal, + # so the next reviewer would otherwise run against a detached, mid-rebase HEAD + # with this pass's fix commit unapplied — worse than the unpushed state. + if git pull --rebase --autostash; then + git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH" + else + git rebase --abort 2>/dev/null # no-op when the pull failed for another reason + false + fi + fi + fi + ``` + + If `UNPUSHED` was non-empty the inner loop's push step did not run, and the block above already pushed it — `git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"`, retried once behind `git pull --rebase --autostash` on a non-fast-forward, exactly the retry the loop files use. **Run the whole thing as one block**: `PUSH_REMOTE`/`PUSH_BRANCH` live only in the shell that set them. If the retry's rebase hits a conflict it is **aborted**, leaving the branch exactly as the reviewer left it, and the pass is recorded `push-failed` — never left mid-rebase. That matters more here than in the inner loops, which run the same `pull --rebase && push` idiom as their last act before reporting failure: `push-failed` is a *continue-signal*, so execution goes on to the next reviewer, and a detached mid-rebase HEAD would both corrupt that reviewer's tree and silently disable its own assertion (no resolvable `@{u}` on a detached HEAD ⇒ the check skips). If the push still fails, record the pass per the rules below. + + **Push to the ref the upstream names — not a bare `git push`, and not `git push origin HEAD`.** A bare push fans out under `push.default=matching` to every local branch with a same-named remote (publishing unrelated branches — including a `release` branch that may auto-tag and publish), and errors outright under `push.default=nothing`. `git push origin HEAD` is the subtler trap: with no ``, git resolves `HEAD` to the **local** branch name and pushes to `refs/heads/`, ignoring the upstream entirely — so on a branch whose upstream has a different name (or lives on another remote) it creates a spurious remote branch, leaves the real PR head stale, and `@{u}..HEAD` is *still* non-empty afterward. That is issue #134's exact failure, reintroduced by the guard meant to prevent it. Deriving the destination from `branch..merge` is what makes the push land on the branch the PR was opened from — and because that value is already fully qualified, it is `HEAD:$PUSH_BRANCH`, never `HEAD:refs/heads/$PUSH_BRANCH`. + + If the push still fails, **record the pass as `push-failed`** and print the unpushed SHAs so the user can see exactly what is stranded. This overrides whatever status the inner loop returned — **except a hard-error** (`cli-error`/`broken-build`/`test-failed`/`rejected`), which keeps its own status so the hard-error short-circuit still fires and the aggregate stays `dirty`; note the stranded SHAs in that pass's Notes instead. Downgrading a `dirty` pass to `push-failed` would turn the aggregate into `inconclusive` and let a caller like `/do:pr` — which aborts before creating a PR only on `dirty` — open a PR against a branch it was supposed to refuse. + + Three properties make this check load-bearing: + - **`git status` is not a substitute.** A clean working tree says nothing about committed-but-unpushed commits — that is precisely the state this catches, so the comparison must be against the upstream ref specifically. + - **It is posture-agnostic.** It applies unchanged when `{REVIEWER_APPLIES}=true` — that flag changes who *edits*, not who *pushes*. + - **It only ever pushes *forward*, and only this pass's own work.** The check publishes commits the pass itself created; it never force-pushes, never rewrites what the remote already has, and never publishes pre-existing local commits on a pass that committed nothing. A *faithfully-executed* hard-error pass (`broken-build`/`test-failed`/`rejected`) already reverted to its own start commit with `git reset --hard`, so `PASS_START_SHA == HEAD` and the check falls through as a no-op — but still **run** the check on a hard-error pass rather than assuming that, since an improvised loop body may have skipped the revert. That is what the hard-error carve-out above covers, and an improvised loop body is the whole reason this assertion exists. ### Stop-mode decision @@ -81,13 +140,15 @@ After each pass completes (before moving to the next reviewer), evaluate `{REVIE | Mode | Continue to next reviewer when... | Stop when... | |------|------------------------------------|---------------| | `all` | always (until list exhausted) | list exhausted | -| `on-findings` | this pass is inconclusive (status ∈ `timeout`/`error`/`guardrail`/`no-verdict`/`skipped`/`not-requestable`), regardless of whether commits were added; OR this pass returned a verdict status (`clean` or `capped` for any loop, or `too-large` for copilot) AND made zero changes (`PASS_START_SHA == HEAD`) | this pass returned a verdict status AND made any change (commits added since `PASS_START_SHA`) | +| `on-findings` | this pass is inconclusive (status ∈ `timeout`/`error`/`guardrail`/`no-verdict`/`skipped`/`not-requestable`/`push-failed`), regardless of whether commits were added; OR this pass returned a verdict status (`clean` or `capped` for any loop, or `too-large` for copilot) AND made zero changes (`PASS_START_SHA == HEAD`) | this pass returned a verdict status AND made any change (commits added since `PASS_START_SHA`) | | `on-clean` | this pass returned a non-clean status (including inconclusive, and including `capped` from any loop — which means the configured iteration budget ran out with fixes applied, not a confirming clean re-review) OR made changes | this pass returned `clean` (or copilot `too-large`) AND made zero changes | **Hard-error short-circuit (applies in all modes)**: if the inner loop returns `cli-error`, `broken-build`, `test-failed`, or `rejected`, stop the multi-reviewer loop immediately. These statuses mean the branch is in a state subsequent reviewers shouldn't run against (broken build / reverted state / explicit reject). Surface the failing reviewer's status as the wrapper's overall status — do not silently continue. Inconclusive non-fix statuses (`copilot` `timeout`/`error`/`guardrail`, the GitHub-reviewer (`@`) `timeout`/`not-requestable`/`error`/`guardrail`, local-agent `guardrail`/`no-verdict`, ollama `incomplete`, plus the `skipped` precondition statuses) do NOT count as findings — they mean the reviewer couldn't produce a verdict, not that it found something to fix. Treat them as continue-signals in every stop mode, even if the inner loop somehow added commits before bailing out: a stop-mode short-circuit must require a *verdict* status (`clean`, `too-large`, `capped`) before honoring the commits-added / no-commits-added condition. This matches the table above and prevents a flaky reviewer that crashed mid-fix from claiming the stop-mode's "found something" signal. +**`push-failed` is a continue-signal too, for a different reason.** Step 5's assertion overrides a pass's status to `push-failed` when its fix commits could not be published — the reviewer *did* return a verdict, but the wrapper could not get its fixes to the remote. Since `push-failed` is not one of the verdict statuses, it can never satisfy a stop-mode short-circuit: a pass whose fixes are stranded locally must not be the reason the remaining reviewers are skipped. + **Convergence within a single reviewer.** The stop-mode above governs *which reviewers* run and in what order — it is the user's explicit choice and this wrapper never second-guesses it. How long *one* reviewer keeps re-reviewing its own fixes is a separate concern, owned by each inner loop's re-loop step via the convergence gate (`~/.claude/lib/review-convergence-gate.md`): a reviewer converges — stops re-requesting — once a round lands only marginal/edge-case findings, well before its mechanical iteration ceiling. So a reviewer returning `clean`/`capped` may have converged on diminishing returns rather than exhausting a hard cap; that is expected and is still a verdict status for stop-mode purposes. The gate never causes the wrapper to skip a reviewer the user listed. ### Parallel dispatch (`{REVIEW_MODE}=parallel`) @@ -103,8 +164,9 @@ Run only when `{REVIEW_MODE}=parallel` was explicitly resolved (flag or saved de Each reviewer's review is independent and writes to its own log. Record each reviewer's review-phase status: `clean` (no findings), `findings` (produced ≥1 finding), or an inconclusive status (`timeout`/`error`/`cli-error`/`skipped`/`not-requestable`/ollama `incomplete`). 3. **Barrier**: wait for every launched review to finish (each is bounded by its own loop's timeout). Wait ACTIVELY, with the local-agent loop's bounded blocking-chunk idiom (repeated ~9-minute foreground `for … sleep 10` calls checking each reviewer's `$DONE_FILE`) — **never end your turn expecting the host to notify you when a background review exits.** That notification only exists for top-level sessions; when this loop runs inside a subagent (a `/do:next --swarm` worker, a CoS/background agent), ending the turn terminates the run and the reviews' findings are lost. 4. **Dedupe the union** of findings across all reviewers — collapse findings that name the same file + line + substantively the same issue into one (keep the clearest description/fix, and note which reviewers raised it). -5. **Apply once, sequentially, in the orchestrator** (the only writer): for each deduped finding, apply the fix, run `{BUILD_CMD}` (skip when empty) + `{TEST_CMD}`, dropping any finding whose fix breaks the build/tests or that is wrong on inspection. Before committing, **run the fix regression guard** on the applied diff (`git diff "$PARALLEL_START_SHA..HEAD"`) — scan for unscoped state-clearing/restoring writes and side effects added to hot paths, re-scope any that fail, and add a focused regression test where the fix touches scoping or timestamp/side-effect logic (see `~/.claude/lib/fix-regression-guard.md`). The guard matters **most** here: step 6 below does no automatic re-review, so a fix's own regression has no second reviewer to catch it. Commit the applied fixes (group sensibly) as `address review (parallel: ): `, then **push once**. Because fixes are applied after collection, there is no per-reviewer commit attribution as in series — the aggregate report notes the parallel commit instead. -6. **Re-review is NOT automatic in parallel mode.** The series loop's per-reviewer re-review recursion (re-review the new commits, governed by the convergence gate) does not run here, because no single reviewer owns the apply. If the applied fixes warrant another look, that is a follow-up series run — and apply the convergence gate (`~/.claude/lib/review-convergence-gate.md`) to that decision too: only re-run when the applied fixes were *substantive*, not to chase marginal edge cases. Say so in the report rather than silently re-fanning out. +5. **Apply once, sequentially, in the orchestrator** (the only writer): for each deduped finding, apply the fix, run `{BUILD_CMD}` (skip when empty) + `{TEST_CMD}`, dropping any finding whose fix breaks the build/tests or that is wrong on inspection. Before committing, **run the fix regression guard** on the applied diff (`git diff "$PARALLEL_START_SHA..HEAD"`) — scan for unscoped state-clearing/restoring writes and side effects added to hot paths, re-scope any that fail, and add a focused regression test where the fix touches scoping or timestamp/side-effect logic (see `~/.claude/lib/fix-regression-guard.md`). The guard matters **most** here: step 7 below does no automatic re-review, so a fix's own regression has no second reviewer to catch it. Commit the applied fixes (group sensibly) as `address review (parallel: ): `, then **push once**. Because fixes are applied after collection, there is no per-reviewer commit attribution as in series — the aggregate report notes the parallel commit instead. +6. **Assert the applied fixes reached the remote** — run **the same block** the series dispatch's step 5 defines, verbatim, with `PARALLEL_START_SHA` substituted for `PASS_START_SHA`, so the config-based target derivation and the push travel together in one shell. Record `push-failed` if the push and its one retry both fail. This matters *more* in parallel mode than in series: the union apply is the only writer in the whole run, so an unpushed union strands **every** reviewer's fixes at once rather than one pass's. +7. **Re-review is NOT automatic in parallel mode.** The series loop's per-reviewer re-review recursion (re-review the new commits, governed by the convergence gate) does not run here, because no single reviewer owns the apply. If the applied fixes warrant another look, that is a follow-up series run — and apply the convergence gate (`~/.claude/lib/review-convergence-gate.md`) to that decision too: only re-run when the applied fixes were *substantive*, not to chase marginal edge cases. Say so in the report rather than silently re-fanning out. A hard-error during apply (build/tests cannot be made green, or a finding forces a revert) sets `{OVERALL_STATUS}=dirty` exactly as the series hard-error short-circuit does. @@ -133,14 +195,19 @@ The **Optional** column reflects each pass's `{OPTIONAL}` flag (from a `~opt` su The **Iterations** column is `{rounds actually run}/{MAX_ITERATIONS}` for that pass (`{MAX_ITERATIONS}` renders as `∞` when the cap is `~max=0`). It is what makes a `capped` row legible — `1/1` says the reviewer stopped because it spent exactly the budget you gave it, not because it ran out of things to say. Name the source in Notes when a per-entry `~max=` set the cap. -**Optional passes are excluded from the `inconclusive` determination.** A pass whose `{OPTIONAL}` is true and whose status is inconclusive (`timeout`/`error`/`guardrail`/`no-verdict`/`skipped`/`not-requestable`/ollama `incomplete`) is treated as clean-equivalent *for the aggregate only* — it never contributes to `{OVERALL_STATUS}=inconclusive`, so it never blocks `--merge`. This is the whole point of `~opt`. A hard-error is the one thing optionality does **not** excuse: if an optional reviewer returns `cli-error`/`broken-build`/`test-failed`/`rejected`, the hard-error short-circuit still fires and the aggregate is still `dirty` — a broken or reverted tree blocks the merge no matter which reviewer produced it. +A `push-failed` row must name the inner loop's original status and list the stranded SHAs in Notes (e.g. `clean, but 2 commits unpushed: a1b2c3d, e4f5a6b`) — the whole value of the status is telling the reader *which* fixes are sitting on their disk. + +**Optional passes are excluded from the `inconclusive` determination.** A pass whose `{OPTIONAL}` is true and whose status is inconclusive (`timeout`/`error`/`guardrail`/`no-verdict`/`skipped`/`not-requestable`/ollama `incomplete`) is treated as clean-equivalent *for the aggregate only* — it never contributes to `{OVERALL_STATUS}=inconclusive`, so it never blocks `--merge`. This is the whole point of `~opt`. Two things optionality does **not** excuse: + +- **A hard-error.** If an optional reviewer returns `cli-error`/`broken-build`/`test-failed`/`rejected`, the hard-error short-circuit still fires and the aggregate is still `dirty` — a broken or reverted tree blocks the merge no matter which reviewer produced it. +- **`push-failed`.** An optional reviewer's *findings* are still real fixes, and a `push-failed` pass means those fixes are committed only on this machine. `~opt` says "don't block the merge on this reviewer's missing verdict"; it does not say "merge a tree that isn't the reviewed tree." So `push-failed` contributes to `{OVERALL_STATUS}=inconclusive` regardless of `{OPTIONAL}`. `{OVERALL_STATUS}` is computed by evaluating each rule top-down; the first matching rule wins: - `dirty` — the wrapper stopped due to a hard-error short-circuit (`cli-error`, `broken-build`, `test-failed`, `rejected`); the failing pass's status is the proximate cause. **Applies regardless of the pass's `{OPTIONAL}` flag** — a broken tree is never merge-eligible. -- `inconclusive` — the executed list contains at least one **non-optional** pass whose status is inconclusive (`timeout`, `error`, `guardrail`, `skipped`, `not-requestable` — an `@` whose request failed and never reviewed — `no-verdict` — a local agent that ran but did not answer in the verdict format — or ollama `incomplete` — a partially-reviewed diff), regardless of whether other passes returned `clean`. Passes marked `~opt` are ignored here (see the exclusion note above). `skipped` covers preconditions-not-met cases — e.g., `codex` in `/do:review` PR mode (codex review --base only accepts a git ref) or `copilot` when no PR exists for the branch. Reached when, e.g., `--review-with copilot,codex` runs copilot which times out and codex which returns clean — the user asked for both perspectives and only got one, so the aggregate is not unconditionally `clean`. Also covers the all-inconclusive case (e.g., `--review-with copilot` that times out and the list exhausts). Distinct from `dirty` (build is fine) but still **not eligible to merge** — the user must re-run or intervene +- `inconclusive` — the executed list contains at least one **non-optional** pass whose status is inconclusive (`timeout`, `error`, `guardrail`, `skipped`, `not-requestable` — an `@` whose request failed and never reviewed — `no-verdict` — a local agent that ran but did not answer in the verdict format — or ollama `incomplete` — a partially-reviewed diff), **or any pass at all — optional included — whose status is `push-failed`** (the wrapper's own status from the push assertion: that pass's fixes are committed locally but never reached the remote, so the reviewed tree is not the tree a merge would land), regardless of whether other passes returned `clean`. Passes marked `~opt` are ignored here **for the inconclusive statuses listed first** — but never for `push-failed`, which lands the aggregate here regardless of `{OPTIONAL}` (see the exclusion note above). `skipped` covers preconditions-not-met cases — e.g., `codex` in `/do:review` PR mode (codex review --base only accepts a git ref) or `copilot` when no PR exists for the branch. Reached when, e.g., `--review-with copilot,codex` runs copilot which times out and codex which returns clean — the user asked for both perspectives and only got one, so the aggregate is not unconditionally `clean`. Also covers the all-inconclusive case (e.g., `--review-with copilot` that times out and the list exhausts). Distinct from `dirty` (build is fine) but still **not eligible to merge** — the user must re-run or intervene - `partial` — some passes were skipped due to a stop-mode decision (`on-findings` or `on-clean` short-circuit) AND every executed pass returned a clean-equivalent status — `clean`, copilot `too-large`, or `capped` from any of the four loops (same clean-equivalence as the `clean` rule below; a `~max=` pass that lands fixes and returns `capped` is exactly what trips `on-findings`, so excluding it here would leave that run matching **no** rule at all) — OR was an excluded optional-inconclusive (no *non-optional* inconclusive remaining) -- `clean` — every **non-optional** executed pass returned `clean` (or copilot `too-large`, or `capped` from any of the four loops — all treated as clean for merge purposes per each loop's own rule — `capped` means the pass reached an **explicitly configured** iteration cap, from a per-entry `~max=` or from `{REVIEW_ITERATIONS}` on a GitHub-side pass, after applying every fix it surfaced; a *built-in* cap that stops a productive loop is `guardrail`, which is inconclusive and lands in the rule above), every **optional** pass returned `clean` or an excluded-inconclusive status, AND no hard-error short-circuit fired AND no *non-optional* inconclusive statuses remain AND no stop-mode short-circuit fired +- `clean` — every **non-optional** executed pass returned `clean` (or copilot `too-large`, or `capped` from any of the four loops — all treated as clean for merge purposes per each loop's own rule — `capped` means the pass reached an **explicitly configured** iteration cap, from a per-entry `~max=` or from `{REVIEW_ITERATIONS}` on a GitHub-side pass, after applying every fix it surfaced; a *built-in* cap that stops a productive loop is `guardrail`, which is inconclusive and lands in the rule above), every **optional** pass returned `clean` or an excluded-inconclusive status, AND no hard-error short-circuit fired AND no *non-optional* inconclusive statuses remain AND **no pass at all — optional included — returned `push-failed`** (the rules are evaluated top-down so such a pass already matched `inconclusive` above; stating it here keeps the `clean` rule true on its own terms rather than by ordering alone) AND no stop-mode short-circuit fired -In **parallel mode** the same rules apply to each reviewer's *review-phase* status (`clean` / `findings` / inconclusive) plus the single apply step, with the same optional exclusion: `dirty` if the apply step couldn't reach a green build/tests (or a finding forced a revert — regardless of optionality); `inconclusive` if any **non-optional** reviewer's review was inconclusive (`timeout`/`error`/`cli-error`/`skipped`/`not-requestable`/ollama `incomplete`) — with one carve-out: a `cli-error` from an **optional** reviewer is NOT excused by `~opt` (hard-errors are never exempted, matching the series-mode rule above) and still yields `inconclusive`; otherwise `clean` once every non-optional reviewer was clean or its findings were applied and verified (optional reviewers' other inconclusive reviews are ignored). `partial` never occurs in parallel mode (there is no stop-mode short-circuit). +In **parallel mode** the same rules apply to each reviewer's *review-phase* status (`clean` / `findings` / inconclusive) plus the single apply step, with the same optional exclusion: `dirty` if the apply step couldn't reach a green build/tests (or a finding forced a revert — regardless of optionality); `inconclusive` if any **non-optional** reviewer's review was inconclusive (`timeout`/`error`/`cli-error`/`skipped`/`not-requestable`/ollama `incomplete`) — with one carve-out: a `cli-error` from an **optional** reviewer is NOT excused by `~opt` (hard-errors are never exempted, matching the series-mode rule above) and still yields `inconclusive`; `inconclusive` too when the apply step's push assertion (step 6) recorded `push-failed`, which is a property of the single union apply rather than of any one reviewer and so is never excused by optionality; otherwise `clean` once every non-optional reviewer was clean or its findings were applied and verified (optional reviewers' other inconclusive reviews are ignored). `partial` never occurs in parallel mode (there is no stop-mode short-circuit). The calling command (do:pr / do:release / do:review) uses `{OVERALL_STATUS}` to decide its own next action. For do:release in particular, the merge gate must require `{OVERALL_STATUS}=clean` — never merge on `dirty` or `inconclusive`, and on `partial` only when the stop-mode was explicitly set (i.e. the user opted into the short-circuit). diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 609278c..1402ec9 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -6,6 +6,7 @@ const fs = require('fs'); const path = require('path'); const readLib = (name) => fs.readFileSync(path.join(__dirname, '..', 'lib', name), 'utf8'); +const readCommand = (name) => fs.readFileSync(path.join(__dirname, '..', 'commands', 'do', name), 'utf8'); describe('review-loop parse contracts', () => { it('requires structured local-agent verdicts without weakening Codex handling', () => { @@ -100,4 +101,144 @@ describe('review-loop parse contracts', () => { /- `partial` — .*every executed pass returned a clean-equivalent status — `clean`, copilot `too-large`, or `capped`/, ); }); + + it('asserts each pass pushed its fixes, and skips the check without an upstream', () => { + // Every inner loop pushes its own fix commits as its last step and nothing + // downstream re-checks it, so an improvised loop body leaves the fixes local + // while the reviewer still reports clean and CI still passes on the stale + // pushed tree. The assertion must compare against the UPSTREAM ref (a clean + // working tree says nothing about committed-but-unpushed commits) and must + // no-op on a never-pushed branch, which /do:review and /do:better allow. + const wrapper = readLib('multi-reviewer-loop.md'); + assert.match(wrapper, /\*\*Assert the pass's fixes reached the remote\.\*\*/); + assert.match(wrapper, /UNPUSHED="\$\(git log --oneline @\{u\}\.\.HEAD\)"/); + assert.match(wrapper, /git rev-parse --abbrev-ref --symbolic-full-name @\{u\} >\/dev\/null 2>&1/); + assert.match(wrapper, /else\n\s*UNPUSHED=""/, 'no upstream must skip the assertion'); + assert.match(wrapper, /`git status` is not a substitute/); + assert.match(wrapper, /\*\*record the pass as `push-failed`\*\*/); + // The union apply in parallel mode is the only writer, so it needs the same guard. + assert.match(wrapper, /\*\*Assert the applied fixes reached the remote\*\*/); + }); + + it('routes push-failed to inconclusive and refuses to let ~opt excuse it', () => { + // Same stranding failure the ~opt/capped test above guards: a status added to + // the dispatch step but never wired into the aggregate rules is inert. And + // push-failed specifically must NOT follow the optional-inconclusive exclusion + // — an optional reviewer's fixes sitting unpushed still mean the merged tree + // is not the reviewed tree. + const wrapper = readLib('multi-reviewer-loop.md'); + assert.match( + wrapper, + /\*\*or any pass at all — optional included — whose status is `push-failed`\*\*/, + 'the inconclusive rule must consume push-failed regardless of {OPTIONAL}', + ); + assert.match(wrapper, /- \*\*`push-failed`\.\*\* An optional reviewer's \*findings\* are still real fixes/); + // Inconclusive, not a verdict: it can never satisfy a stop-mode short-circuit. + assert.match(wrapper, /`no-verdict`\/`skipped`\/`not-requestable`\/`push-failed`/); + // The `inconclusive` bullet ends by excusing ~opt passes. Unqualified, that + // sentence flatly contradicts the push-failed carve-out two clauses earlier and + // an orchestrator could read it as license to merge an ~opt reviewer's stranded + // fixes — so the exemption must name the statuses it applies to. + // Structural, not verbatim: a reworded but still-unqualified exemption + // ("Optional passes are ignored here.") would reintroduce the same + // contradiction while a literal-string guard kept passing. + assert.doesNotMatch( + wrapper, + /whose status is `push-failed`[\s\S]{0,2000}?(Passes marked `~opt` are ignored here\.|`~opt` passes are ignored here\.|are ignored here \(see)/, + 'any ~opt exemption following the push-failed clause must name the statuses it covers', + ); + assert.match(wrapper, /but never for `push-failed`, which lands the aggregate here regardless of `\{OPTIONAL\}`/); + // A hard-error must keep its own status: rewriting it to push-failed would + // silence the hard-error short-circuit and downgrade the aggregate from dirty + // to inconclusive, past do:pr's "abort before creating the PR on dirty" gate. + assert.match(wrapper, /\*\*except a hard-error\*\* \(`cli-error`\/`broken-build`\/`test-failed`\/`rejected`\), which keeps its own status/); + // The consumers that restate the aggregate rule must agree with it. + assert.match(readCommand('release.md'), /or `push-failed`, a pass whose fix commits never reached the remote/); + }); + + it('scopes the push assertion to the pass and never pushes by fan-out', () => { + // Two ways this check could do damage rather than prevent it: publishing + // deliberately-unpushed local commits on a pass that committed nothing (it is a + // "did the push step run" check, not a "sync my branch" command), and a bare + // `git push`, which under push.default=matching fans out to every same-named + // local branch — including a release branch that may auto-tag and publish. + const wrapper = readLib('multi-reviewer-loop.md'); + assert.match(wrapper, /if \[ "\$PASS_START_SHA" = "\$\(git rev-parse HEAD\)" \]; then/); + // Parallel mode reuses the whole block (derivation + scoping + push together) + // rather than restating a push whose variables nothing in that section defines. + assert.match( + wrapper, + /\*\*the same block\*\* the series dispatch's step 5 defines, verbatim, with `PARALLEL_START_SHA` substituted for `PASS_START_SHA`/, + 'parallel mode needs the same zero-commit scoping and target derivation', + ); + // The destination must come from @{u}, never from the local branch name: + // `git push origin HEAD` resolves to refs/heads/, so when the + // upstream is named differently it pushes a spurious branch, leaves the PR head + // stale, and @{u}..HEAD stays non-empty — #134's failure inside its own guard. + // Derive from config, not by splitting the abbrev-ref: a remote name may contain + // a slash, and a LOCAL upstream (remote ".") abbreviates with no slash at all — + // which a %%/ + #*/ split turns into a bogus remote, firing a false push-failed + // on a healthy branch. A local upstream has no remote to assert against at all. + assert.match(wrapper, /PUSH_REMOTE="\$\(git config --get "branch\.\$BR\.remote"\)"/); + assert.match(wrapper, /PUSH_BRANCH="\$\(git config --get "branch\.\$BR\.merge"\)"/); + assert.match(wrapper, /\[ "\$PUSH_REMOTE" = "\." \]/, 'a local-branch upstream must skip the check'); + assert.match(wrapper, /not a bare `git push`, and not `git push origin HEAD`/); + // Every PRESCRIBED push (identified by an explicit HEAD: destination — the prose + // warnings about `git push origin HEAD` carry none) must use the derived form. + // A verbatim doesNotMatch only ever blocks the one phrasing it quotes. + // Scan raw text, not just backticked spans: the in-block occurrences are inside a + // fence and carry no backticks, so a backtick-anchored scan would miss the very + // command that actually runs. + const prescribed = [...wrapper.matchAll(/git push [^\n`]*?HEAD:[^\s"`]*/g)].map((m) => m[0]); + assert.ok(prescribed.length >= 3, 'the push must appear in the block (twice, with retry) and in prose'); + assert.ok( + prescribed.every((c) => c === 'git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH'), + `every prescribed push must target the upstream-derived ref, got: ${prescribed.join(' | ')}`, + ); + // branch..merge is already refs/heads/; re-prefixing would produce + // refs/heads/refs/heads/. + assert.doesNotMatch( + wrapper, + /git push [^\n`]*HEAD:refs\/heads\//, + 'no prescribed push may re-prefix refs/heads/ (naming it in a warning is fine)', + ); + // The variables must be consumed in the shell that set them — spec snippets run + // as separate Bash calls, where an empty PUSH_REMOTE means `git push "" "HEAD:"`. + // The point is that the push lives inside the guard, in the same shell — not + // that it sits on any particular line. + assert.match(wrapper, /if \[ -n "\$UNPUSHED" \]; then[\s\S]{0,600}?git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); + // A conflicted retry must not strand the branch mid-rebase: push-failed is a + // continue-signal, so the next reviewer would inherit a detached HEAD whose own + // assertion then silently skips (no resolvable @{u} to compare against). + // Anchored to the else-branch, not a bare substring: naming `git rebase --abort` + // anywhere in the prose would otherwise satisfy this while the code path is gone. + // The trailing `false` is what makes a failed push observable to the orchestrator + // — without it the abort's own success flips the block to exit 0, the pass is + // never recorded push-failed, and the stranded commits reach the merge gate. + assert.match( + wrapper, + /else\n\s*git rebase --abort 2>\/dev\/null[^\n]*\n\s*false\n/, + 'a conflicted retry must abort the rebase AND still exit non-zero so the pass records push-failed', + ); + + const pr = readCommand('pr.md'); + assert.match(pr, /`git push origin \{current_branch\}` — an explicit refspec, never a bare `git push`/); + // Without a stop-on-failure clause the orchestrator falls through to gh pr create + // and opens exactly the stale pre-review PR this guard exists to prevent. + assert.match(pr, /\*\*If the push still fails after that one retry, do NOT create the PR\*\*/); + }); + + it('blocks PR creation and merge on unpushed commits in do:pr', () => { + // Backstop for the two moments where unpushed review fixes become user-visible + // damage: a PR opened from the pre-review tree, and a merge that lands it. + const pr = readCommand('pr.md'); + assert.match(pr, /\*\*First, assert the branch's commits reached the remote\.\*\*/); + assert.match(pr, /\*\*Unpushed-commits gate\*\* — \*\*refuse to merge while the local branch is ahead of its remote\.\*\*/); + // Fails closed: the gate exits non-zero rather than printing a result someone + // has to interpret — the whole failure mode here is a step being skimmed past. + assert.match(pr, /UNPUSHED="\$\(git log --oneline @\{u\}\.\.HEAD\)"[\s\S]{0,200}?if \[ -n "\$UNPUSHED" \]; then/); + assert.match(pr, /REFUSING TO MERGE — these commits are not on the remote:/); + assert.match(pr, /\s+exit 1/); + assert.match(pr, /Never merge on `dirty`\/`inconclusive`, never merge while the branch has unpushed commits/); + }); });