From d87c6d8a589ea88ffa085aa83a809935954bb5a7 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:32:44 -0700 Subject: [PATCH 01/18] docs: say plainly that --issues picks a destination and --scan-only stops the pipeline --issues reads like a run mode ("file issues instead of doing the work") but is specified as a sink selector ("record deferred findings in the tracker rather than PLAN.md"). Users hit this by running /do:better --issues or /do:simplify --issues and getting a full remediation pipeline with per-category PRs. The combination that delivers the expected behaviour is --scan-only --issues, and it was underspecified: the disposition rules file an issue for any finding the run "defers", and a scan-only run remediates nothing, so every finding is deferred -- but that was left to inference. Two runs with identical flags could defensibly file all findings or none. Fixes both halves: - --scan-only and its Phase 2 gate now state that under issue mode every surviving finding is filed, and that the filed issues are the run's entire output. - The --issues bullet now leads with what it controls and says outright that it does not change what the run does, pointing at --scan-only. - plan-issue-mode.md grows a section covering the scan-only case directly, since that partial is where the "deferred" wording originates. - review-config-defaults.md notes that a saved issues: true does not suppress remediation, and why --issues must not be redefined to imply --scan-only: anyone with that default set would silently stop getting fixes. No behaviour change to --issues itself. --- commands/do/better-swift.md | 6 ++++-- commands/do/better.md | 6 ++++-- commands/do/depfree.md | 6 ++++-- lib/plan-issue-mode.md | 17 +++++++++++++++++ lib/review-config-defaults.md | 2 +- 5 files changed, 30 insertions(+), 7 deletions(-) diff --git a/commands/do/better-swift.md b/commands/do/better-swift.md index 61b6213..88f0f11 100644 --- a/commands/do/better-swift.md +++ b/commands/do/better-swift.md @@ -13,7 +13,7 @@ Run the full DevSecOps lifecycle optimized for Swift/SwiftUI multi-platform proj Parse `$ARGUMENTS` for: - **`--interactive`**: pause at each decision point for user approval -- **`--scan-only`**: run Phase 0 + 1 + 2 only (audit and plan), skip remediation +- **`--scan-only`**: run Phase 0 + 1 + 2 only (audit and plan), skip remediation — no worktree, no code changes, no PRs. **When `ISSUE_MODE` is also true, this is the "audit and file the work, don't touch my code" combination**: every surviving finding is filed as a labelled tracker issue before the run exits, not just the deferred subset (see the Phase 2 gate). `--scan-only` is the flag that stops the pipeline; `--issues` only chooses where findings are recorded - **`--no-merge`**: run through PR creation (Phase 5), skip the review loop and merge - **`--review-with `**: which reviewer(s) run the Phase 6 review loop on each PR. Accepted slugs: `copilot`, `codex`, `agy` (aliases `gemini` / `antigravity` — all run the Antigravity CLI's `agy` binary), `claude`, `grok`, `ollama` (bare `ollama` auto-selects the most capable installed coding model; `ollama[]` pins a specific installed model, e.g. `ollama[qwen2.5-coder:32b]` — strip the bracket into a per-entry `OLLAMA_MODEL`; `codex`/`claude`/`agy`/`grok` likewise accept a `[]` bracket — e.g. `codex[o3]`, `claude[claude-opus-4-8]`, `grok[grok-code-fast-1]` — stripped into a per-entry `REVIEW_MODEL`, empty → the reviewer's built-in default; `copilot` and `@` take no model bracket), or an arbitrary GitHub login `@` — any GitHub user or App/bot (e.g. `@octocat`, `@org-review-bot`, `@some-app[bot]`); slashdo requests its review on the PR and waits for it (GitHub only, never posts an approval itself) (comma-separated, ordered list; split on `,`, trim whitespace, normalize `gemini`/`antigravity` → `agy`, dedupe preserving first-occurrence order, with each model-taking agent's (`codex`/`claude`/`agy`/`grok`/`ollama`) `[]` bracket suffix part of the dedup identity). Record as `REVIEW_AGENTS`. **There is no built-in default** — if omitted, leave `REVIEW_AGENTS` **unset for now**; the saved-defaults step below fills it from `/do:config` if a default exists, and **only if it is still unset after that** is `REVIEW_AGENTS=[]` (Phase 6 skipped, PRs left open without merging — see Phase 6). `copilot` is never added implicitly. Any slot may end in `~opt` (e.g. `ollama~opt`, `ollama[qwen2.5-coder:32b]~opt`) to mark that reviewer **optional/non-blocking** — still requested and its findings still fixed, but an inconclusive result from it (timeout/skipped/incomplete/no-verdict) never blocks the PR merge (a hard-error from it still does); strip `~opt` into a per-entry `{OPTIONAL}` flag before slug parsing, and it is **not** part of the dedup identity (`ollama~opt` == `ollama`, optional-wins on collapse). A slot may also end in `~max=` (e.g. `claude~max=2`, `ollama~max=1`) to cap how many review → fix → re-review cycles **that one reviewer** runs — the per-entry form of `--review-iterations`, and the only way to move the local-agent / `ollama` caps (otherwise fixed at 3), so one run can budget each reviewer separately (`claude~max=2,ollama~max=1,codex~max=3`). `` is a non-negative integer (`0` = loop until clean, bounded by each loop's 10-iteration guardrail); strip it into a per-entry `{ENTRY_MAX}` alongside `~opt` — both suffixes come off the right of the token in either order before slug parsing, and neither is part of the dedup identity (the cap comes from the first occurrence that carried a `~max`, so a bare earlier occurrence does not erase a later cap). A reviewer that stops because it reached a `~max` you set returns `capped`, which is clean-equivalent for the merge gate. See `lib/multi-reviewer-loop.md`. Abort on an unknown slug with `Unknown --review-with value: {value}. Use one of: copilot, codex, agy, claude, grok, ollama, @ (each optionally suffixed ~opt and/or ~max=).` The reserved token `none` (case-insensitive) is **not** validated as a slug — `--review-with none` means no reviewer (set `REVIEW_AGENTS=[]`) and overrides any saved `review-with` default. - **`--review-stop-on-findings`** / **`--review-stop-on-clean`** (mutually exclusive): forwarded to the multi-reviewer loop for each PR; control when a per-PR reviewer list stops early. Set `REVIEW_STOP_MODE` (`all` default, `on-findings`, or `on-clean`). If both are present, abort with `--review-stop-on-findings and --review-stop-on-clean cannot be combined`. @@ -25,7 +25,7 @@ After parsing the review flags above, apply any **saved defaults** (set via `/do !`cat ~/.claude/lib/review-config-defaults.md` -- **`--issues`** / **`--no-issues`** / **`--issues-label `**: track deferred findings as GitHub/GitLab issues instead of PLAN.md lines (see Phase 2). `--issues` sets `ISSUE_MODE=true`; `--no-issues` forces `ISSUE_MODE=false`. If the user passes **neither**, take `ISSUE_MODE` from the saved `issues` default resolved above (built-in default `false`). Set `PLAN_LABEL` from `--issues-label`, else the saved `issues-label` default, else `plan`. +- **`--issues`** / **`--no-issues`** / **`--issues-label `**: selects **where deferred findings are recorded** — GitHub/GitLab issues instead of PLAN.md lines (see Phase 2). **It does NOT change what the run does**: remediation, PRs, CI, the review loop, and merge all proceed exactly as normal. To audit and file work *without* remediating, combine it with **`--scan-only`**. `--issues` sets `ISSUE_MODE=true`; `--no-issues` forces `ISSUE_MODE=false`. If the user passes **neither**, take `ISSUE_MODE` from the saved `issues` default resolved above (built-in default `false`). Set `PLAN_LABEL` from `--issues-label`, else the saved `issues-label` default, else `plan`. - **Path filter**: limit scanning scope to specific directories or files - **Focus areas**: e.g., "security only", "platform coverage and accessibility" @@ -623,6 +623,8 @@ For each file touched by multiple categories, document why it was assigned to on **GATE: If `--scan-only` was passed, STOP HERE.** Print the summary and exit. +**When `ISSUE_MODE` is also true, file every surviving finding as an issue before exiting** — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. + ## Phase 3: Worktree Remediation Only proceed with CRITICAL, HIGH, and MEDIUM findings for code remediation. LOW findings remain tracked in PLAN.md but are not auto-remediated. Test Quality & Coverage findings are handled separately in Phase 4c. diff --git a/commands/do/better.md b/commands/do/better.md index e6eefdc..6b00609 100644 --- a/commands/do/better.md +++ b/commands/do/better.md @@ -13,7 +13,7 @@ Run the full DevSecOps lifecycle: audit the codebase with up to 10 deduplicated Parse `$ARGUMENTS` for: - **`--interactive`**: pause at each decision point for user approval -- **`--scan-only`**: run Phase 0 + 1 + 2 only (audit and plan), skip remediation +- **`--scan-only`**: run Phase 0 + 1 + 2 only (audit and plan), skip remediation — no worktree, no code changes, no PRs. **When `ISSUE_MODE` is also true, this is the "audit and file the work, don't touch my code" combination**: every surviving finding is filed as a labelled tracker issue before the run exits, not just the deferred subset (see the Phase 2 gate). `--scan-only` is the flag that stops the pipeline; `--issues` only chooses where findings are recorded - **`--simplify-only`** (alias: **`--refactor-only`**): narrow the run to structural quality — refactoring, architecture, DRY, simplification, and cognitive load. Set `SIMPLIFY_ONLY=true`. The pipeline is unchanged (worktree remediation → per-category PRs → CI → review loop → merge); only the audit roster, the category set, and the remediation contract narrow. See [Simplify-Only Mode](#simplify-only-mode---simplify-only) for the exact deltas. `/do:simplify` is the shorthand for `/do:better --simplify-only` - **`--no-merge`**: run through PR creation (Phase 5), skip the review loop and merge - **`--review-with `**: which reviewer(s) run the Phase 6 review loop on each PR. Accepted slugs: `copilot`, `codex`, `agy` (aliases `gemini` / `antigravity` — all run the Antigravity CLI's `agy` binary), `claude`, `grok`, `ollama` (bare `ollama` auto-selects the most capable installed coding model; `ollama[]` pins a specific installed model, e.g. `ollama[qwen2.5-coder:32b]` — strip the bracket into a per-entry `OLLAMA_MODEL`; `codex`/`claude`/`agy`/`grok` likewise accept a `[]` bracket — e.g. `codex[o3]`, `claude[claude-opus-4-8]`, `grok[grok-code-fast-1]` — stripped into a per-entry `REVIEW_MODEL`, empty → the reviewer's built-in default; `copilot` and `@` take no model bracket), or an arbitrary GitHub login `@` — any GitHub user or App/bot (e.g. `@octocat`, `@org-review-bot`, `@some-app[bot]`); slashdo requests its review on the PR and waits for it (GitHub only, never posts an approval itself) (comma-separated, ordered list; split on `,`, trim whitespace, normalize `gemini`/`antigravity` → `agy`, dedupe preserving first-occurrence order, with each model-taking agent's (`codex`/`claude`/`agy`/`grok`/`ollama`) `[]` bracket suffix part of the dedup identity). Record as `REVIEW_AGENTS`. **There is no built-in default** — if omitted, leave `REVIEW_AGENTS` **unset for now**; the saved-defaults step below fills it from `/do:config` if a default exists, and **only if it is still unset after that** is `REVIEW_AGENTS=[]` (Phase 6 skipped, PRs left open without merging — see Phase 6). `copilot` is never added implicitly. Any slot may end in `~opt` (e.g. `ollama~opt`, `ollama[qwen2.5-coder:32b]~opt`) to mark that reviewer **optional/non-blocking** — still requested and its findings still fixed, but an inconclusive result from it (timeout/skipped/incomplete/no-verdict) never blocks the PR merge (a hard-error from it still does); strip `~opt` into a per-entry `{OPTIONAL}` flag before slug parsing, and it is **not** part of the dedup identity (`ollama~opt` == `ollama`, optional-wins on collapse). A slot may also end in `~max=` (e.g. `claude~max=2`, `ollama~max=1`) to cap how many review → fix → re-review cycles **that one reviewer** runs — the per-entry form of `--review-iterations`, and the only way to move the local-agent / `ollama` caps (otherwise fixed at 3), so one run can budget each reviewer separately (`claude~max=2,ollama~max=1,codex~max=3`). `` is a non-negative integer (`0` = loop until clean, bounded by each loop's 10-iteration guardrail); strip it into a per-entry `{ENTRY_MAX}` alongside `~opt` — both suffixes come off the right of the token in either order before slug parsing, and neither is part of the dedup identity (the cap comes from the first occurrence that carried a `~max`, so a bare earlier occurrence does not erase a later cap). A reviewer that stops because it reached a `~max` you set returns `capped`, which is clean-equivalent for the merge gate. See `lib/multi-reviewer-loop.md`. Abort on an unknown slug with `Unknown --review-with value: {value}. Use one of: copilot, codex, agy, claude, grok, ollama, @ (each optionally suffixed ~opt and/or ~max=).` The reserved token `none` (case-insensitive) is **not** validated as a slug — `--review-with none` means no reviewer (set `REVIEW_AGENTS=[]`) and overrides any saved `review-with` default. @@ -27,7 +27,7 @@ After parsing the review flags above, apply any **saved defaults** (set via `/do !`cat ~/.claude/lib/review-config-defaults.md` - **`--strict`** (alias: **`--nuclear`**): enable the Structural Ambition agent (10th audit agent) and promote its blocker-tier findings to CRITICAL severity for remediation. Flags file-size growth past 1000 lines, ad-hoc conditionals bolted onto unrelated flows, thin wrappers, boundary leaks, and missed code-judo simplifications. Set `STRICT_MODE=true` when present. `--simplify-only` implies it — set `STRICT_MODE=true` whenever `SIMPLIFY_ONLY=true`, whether or not `--strict` was passed -- **`--issues`** / **`--no-issues`** / **`--issues-label `**: track deferred findings as GitHub/GitLab issues instead of PLAN.md lines (see Phase 2). `--issues` sets `ISSUE_MODE=true`; `--no-issues` forces `ISSUE_MODE=false`. If the user passes **neither**, take `ISSUE_MODE` from the saved `issues` default resolved above (built-in default `false`). Set `PLAN_LABEL` from `--issues-label`, else the saved `issues-label` default, else `plan`. +- **`--issues`** / **`--no-issues`** / **`--issues-label `**: selects **where deferred findings are recorded** — GitHub/GitLab issues instead of PLAN.md lines (see Phase 2). **It does NOT change what the run does**: remediation, per-category PRs, CI, the review loop, and merge all proceed exactly as normal. To audit and file work *without* remediating, combine it with **`--scan-only`**. `--issues` sets `ISSUE_MODE=true`; `--no-issues` forces `ISSUE_MODE=false`. If the user passes **neither**, take `ISSUE_MODE` from the saved `issues` default resolved above (built-in default `false`). Set `PLAN_LABEL` from `--issues-label`, else the saved `issues-label` default, else `plan`. - **Path filter**: limit scanning scope to specific directories or files - **Focus areas**: e.g., "security only", "DRY and bugs" @@ -493,6 +493,8 @@ Omit the **UX** row when `HAS_UI=false`, the **Structural** row when `STRICT_MOD **GATE: If `--scan-only` was passed, STOP HERE.** Print the summary and exit. +**When `ISSUE_MODE` is also true, file every surviving finding as an issue before exiting** — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. + ## Phase 3: Worktree Remediation Only proceed with CRITICAL, HIGH, and MEDIUM findings for code remediation. LOW findings remain tracked in PLAN.md but are not auto-remediated. Test Quality & Coverage findings are handled separately in Phase 4c. diff --git a/commands/do/depfree.md b/commands/do/depfree.md index 1685107..82fd5c9 100644 --- a/commands/do/depfree.md +++ b/commands/do/depfree.md @@ -15,7 +15,7 @@ Every small library is an attack surface. Supply chain compromises are real and Parse `$ARGUMENTS` for: - **`--interactive`**: pause at each decision point for user approval -- **`--scan-only`**: run Phase 0 + 1 + 2 only (audit and plan), skip remediation +- **`--scan-only`**: run Phase 0 + 1 + 2 only (audit and plan), skip remediation — no worktree, no code changes, no PRs. **When `ISSUE_MODE` is also true, this is the "audit and file the work, don't touch my code" combination**: every surviving finding is filed as a labelled tracker issue before the run exits, not just the deferred subset (see the Phase 2 gate). `--scan-only` is the flag that stops the pipeline; `--issues` only chooses where findings are recorded - **`--no-merge`**: run through PR creation, skip the review loop and merge - **`--heavy`**: aggressive mode — only keep foundational frameworks and language runtimes; replace everything else that is feasibly replaceable (see Heavy Mode below) - **`--review-with `**: which reviewer(s) run the Phase 5c review loop on the PR. Accepted slugs: `copilot`, `codex`, `agy` (aliases `gemini` / `antigravity` — all run the Antigravity CLI's `agy` binary), `claude`, `grok`, `ollama` (bare `ollama` auto-selects the most capable installed coding model; `ollama[]` pins a specific installed model, e.g. `ollama[qwen2.5-coder:32b]` — strip the bracket into a per-entry `OLLAMA_MODEL`; `codex`/`claude`/`agy`/`grok` likewise accept a `[]` bracket — e.g. `codex[o3]`, `claude[claude-opus-4-8]`, `grok[grok-code-fast-1]` — stripped into a per-entry `REVIEW_MODEL`, empty → the reviewer's built-in default; `copilot` and `@` take no model bracket), or an arbitrary GitHub login `@` — any GitHub user or App/bot (e.g. `@octocat`, `@org-review-bot`, `@some-app[bot]`); slashdo requests its review on the PR and waits for it (GitHub only, never posts an approval itself) (comma-separated, ordered list; split on `,`, trim whitespace, normalize `gemini`/`antigravity` → `agy`, dedupe preserving first-occurrence order, with each model-taking agent's (`codex`/`claude`/`agy`/`grok`/`ollama`) `[]` bracket suffix part of the dedup identity). Record as `REVIEW_AGENTS`. **There is no built-in default** — if omitted, leave `REVIEW_AGENTS` **unset for now**; the saved-defaults step below fills it from `/do:config` if a default exists, and **only if it is still unset after that** is `REVIEW_AGENTS=[]` (Phase 5c skipped, PR left open without merging — see Phase 5c/5d). `copilot` is never added implicitly. Any slot may end in `~opt` (e.g. `ollama~opt`, `ollama[qwen2.5-coder:32b]~opt`) to mark that reviewer **optional/non-blocking** — still requested and its findings still fixed, but an inconclusive result from it (timeout/skipped/incomplete/no-verdict) never blocks the PR merge (a hard-error from it still does); strip `~opt` into a per-entry `{OPTIONAL}` flag before slug parsing, and it is **not** part of the dedup identity (`ollama~opt` == `ollama`, optional-wins on collapse). A slot may also end in `~max=` (e.g. `claude~max=2`, `ollama~max=1`) to cap how many review → fix → re-review cycles **that one reviewer** runs — the per-entry form of `--review-iterations`, and the only way to move the local-agent / `ollama` caps (otherwise fixed at 3), so one run can budget each reviewer separately (`claude~max=2,ollama~max=1,codex~max=3`). `` is a non-negative integer (`0` = loop until clean, bounded by each loop's 10-iteration guardrail); strip it into a per-entry `{ENTRY_MAX}` alongside `~opt` — both suffixes come off the right of the token in either order before slug parsing, and neither is part of the dedup identity (the cap comes from the first occurrence that carried a `~max`, so a bare earlier occurrence does not erase a later cap). A reviewer that stops because it reached a `~max` you set returns `capped`, which is clean-equivalent for the merge gate. See `lib/multi-reviewer-loop.md`. Abort on an unknown slug with `Unknown --review-with value: {value}. Use one of: copilot, codex, agy, claude, grok, ollama, @ (each optionally suffixed ~opt and/or ~max=).` The reserved token `none` (case-insensitive) is **not** validated as a slug — `--review-with none` means no reviewer (set `REVIEW_AGENTS=[]`) and overrides any saved `review-with` default. @@ -28,7 +28,7 @@ After parsing the review flags above, apply any **saved defaults** (set via `/do !`cat ~/.claude/lib/review-config-defaults.md` -- **`--issues`** / **`--no-issues`** / **`--issues-label `**: track deferred removals as GitHub/GitLab issues instead of PLAN.md lines (see Phase 2). `--issues` sets `ISSUE_MODE=true`; `--no-issues` forces `ISSUE_MODE=false`. If the user passes **neither**, take `ISSUE_MODE` from the saved `issues` default resolved above (built-in default `false`). Set `PLAN_LABEL` from `--issues-label`, else the saved `issues-label` default, else `plan`. +- **`--issues`** / **`--no-issues`** / **`--issues-label `**: selects **where deferred removals are recorded** — GitHub/GitLab issues instead of PLAN.md lines (see Phase 2). **It does NOT change what the run does**: remediation, PRs, CI, the review loop, and merge all proceed exactly as normal. To audit and file work *without* remediating, combine it with **`--scan-only`**. `--issues` sets `ISSUE_MODE=true`; `--no-issues` forces `ISSUE_MODE=false`. If the user passes **neither**, take `ISSUE_MODE` from the saved `issues` default resolved above (built-in default `false`). Set `PLAN_LABEL` from `--issues-label`, else the saved `issues-label` default, else `plan`. - **Specific packages**: limit audit scope to named packages (e.g., "chalk dotenv") Set `HEAVY_MODE` to `true` if `--heavy` was passed, `false` otherwise. @@ -381,6 +381,8 @@ For each dependency to remove: **GATE: If `--scan-only` was passed, STOP HERE.** Print the summary and exit. +**When `ISSUE_MODE` is also true, file every surviving finding as an issue before exiting** — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. + **GATE: If no removable dependencies were found, print "All dependencies are justified" and exit.** **Interactive mode**: Present the removal plan via `AskUserQuestion`: diff --git a/lib/plan-issue-mode.md b/lib/plan-issue-mode.md index e08fab6..33af86d 100644 --- a/lib/plan-issue-mode.md +++ b/lib/plan-issue-mode.md @@ -16,6 +16,10 @@ while work happens on issues. - **`--issues`**: file plan items as tracker issues instead of `PLAN.md` lines. Record `ISSUE_MODE=true` (default `false`). A saved `issues=true` default resolves to the same `ISSUE_MODE=true` when neither `--issues` nor `--no-issues` is typed. + **This flag selects a destination, not a run mode.** It changes *where* items are + recorded and nothing else — a command that remediates, opens PRs, or merges still + does all of that. In a command that also offers `--scan-only`, that is the flag + which stops the pipeline; see "Recording every finding under `--scan-only`" below. - **`--issues-label `**: the label that scopes plan-tracking issues. Record `PLAN_LABEL` (default `plan`). Only meaningful when `ISSUE_MODE` is true. @@ -43,6 +47,19 @@ while work happens on issues. `EXISTING_ISSUES`. Listing **all** open issues (not just `--label `) avoids re-filing a finding someone already opened by hand under a different label. +## Recording every finding under `--scan-only` + +The rules below record an item when a command **defers** it — decides not to act on +it this run. A `--scan-only` run acts on *nothing*, so under `--scan-only` + +`ISSUE_MODE` **every surviving finding is deferred and must be filed**, not just the +subset a full run would have skipped. The filed issues are the entire output of that +run: no worktree, no code changes, no PRs. Apply the same dedup, labels, and +title/body rules below to all of them, and report the created and reused `#`s +in the command's summary. + +This is the combination to reach for when the intent is "audit and file the work, +don't touch my code" — `--issues` alone does not do it. + ## Recording a plan item - **PLAN.md mode (default):** append diff --git a/lib/review-config-defaults.md b/lib/review-config-defaults.md index 756e914..14dfbbe 100644 --- a/lib/review-config-defaults.md +++ b/lib/review-config-defaults.md @@ -27,7 +27,7 @@ Procedure (run once, during argument parsing): - `reviewer-applies` → `--reviewer-applies` (boolean; `true` means the flag is set) - `review-stop-mode` → the stop-mode flags: `"on-findings"` ≡ `--review-stop-on-findings`, `"on-clean"` ≡ `--review-stop-on-clean`, `"all"` (or absent) ≡ neither - `review-mode` → `--review-mode ` (string; how the multi-reviewer loop dispatches its reviewers — `series` runs them one-after-another so each sees the prior's fixes, `parallel` runs their reviews concurrently then applies the union once). Built-in default `series`. Read by every command that runs the multi-reviewer loop (`/do:pr`, `/do:review`, `/do:better`, `/do:better-swift`, `/do:depfree`, `/do:release`). `/do:rpr` does not run the multi-reviewer wrapper (its parallelism is review-thread resolution, not reviewer dispatch), so it ignores `review-mode` — exactly as it ignores `review-iterations` / `review-stop-mode`. - - `issues` → the `--issues` / `--no-issues` flags (boolean; `true` ≡ `--issues` = issue mode, `false` or absent ≡ PLAN.md mode). The per-run override is a typed flag in **either** direction — `--issues` forces issue mode and `--no-issues` forces PLAN.md mode — exactly like `--reviewer-applies`/`--no-reviewer-applies`: whichever the user typed wins over the saved default. A stored `false` is an explicit opt-out a project uses (typically with `--project`) to mask an inherited global `issues=true`; `--unset issues` instead removes the key and falls back to the lower-precedence value. + - `issues` → the `--issues` / `--no-issues` flags (boolean; `true` ≡ `--issues` = issue mode, `false` or absent ≡ PLAN.md mode). **A saved `issues: true` does not suppress remediation** — it only routes deferred findings to the tracker instead of `PLAN.md`. Commands that remediate, open PRs, and merge keep doing all of that on every run; `--scan-only` is what stops the pipeline. (This is why `--issues` must never be redefined to imply `--scan-only`: anyone with this default set would silently stop getting fixes.) The per-run override is a typed flag in **either** direction — `--issues` forces issue mode and `--no-issues` forces PLAN.md mode — exactly like `--reviewer-applies`/`--no-reviewer-applies`: whichever the user typed wins over the saved default. A stored `false` is an explicit opt-out a project uses (typically with `--project`) to mask an inherited global `issues=true`; `--unset issues` instead removes the key and falls back to the lower-precedence value. - `issues-label` → `--issues-label ` (string; the label that scopes plan-tracking issues, built-in default `plan`). Only meaningful once issue mode is on (via flag or the `issues` default). - `self` → the `--self` / `--no-self` flags (boolean; `true` ≡ `--self` = claim only issues filed by the running account `@me`, `false` or absent ≡ claim any open issue). The per-run override is a typed flag in either direction — `--self` forces self-only and `--no-self` forces any-author — whichever the user typed wins over the saved default, like `issues`. A stored `false` masks an inherited global `self=true` (typically with `--project`); `--unset self` instead removes the key and falls back to the lower-precedence value. Only `/do:next` reads this key, and only in issue mode (PLAN.md items have no author). - `merge` → the `--merge` / `--no-merge` flags (boolean; `true` ≡ `--merge` = auto-merge the PR once reviews **and** CI are solid, `false` or absent ≡ leave the PR open). The per-run override is a typed flag in either direction — `--merge` forces auto-merge and `--no-merge` forces leave-open — whichever the user typed wins over the saved default, like `issues`. Only `/do:pr` reads this key. From d091ffed20f34965114af496f9c26b90be53f1b1 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:15:07 -0700 Subject: [PATCH 02/18] fix([issue-134]): assert review-fix commits are pushed before opening a PR or merging Every reviewer loop pushes its own fix commits as its final step, but nothing downstream verified it happened. An orchestrator that improvised a loop body instead of following it left the fixes local: gh pr create opened a PR carrying only the pre-review commit, every reviewer still reported clean, CI still passed (it was testing the pushed tree, green because it predated the fixes), and the merge gate had nothing to object to. The failure was completely silent. Primary fix in the multi-reviewer wrapper, so it lands once for all seven commands that drive it: series dispatch gains a step 5 asserting the pass's commits reached the remote, pushing with the loops' own one-retry, and recording a new push-failed status when the push still fails. Parallel dispatch gets the same assertion after its single union apply. push-failed is inconclusive and, unlike the other inconclusive statuses, is never excused by ~opt: a non-blocking reviewer's findings are still real fixes, and merging a tree that isn't the reviewed tree is not what ~opt opts out of. Backstops in do:pr at the two moments unpushed work becomes visible damage: Open the PR pushes first if the branch is ahead, and the merge gate refuses to merge while it is, reporting the unpushed SHAs. Both checks compare against the upstream ref specifically (git status says nothing about committed-but-unpushed commits) and skip when there is no upstream, so a never-pushed branch under /do:review or /do:better is unaffected. --- commands/do/pr.md | 22 ++++++++++----- lib/multi-reviewer-loop.md | 38 ++++++++++++++++++++++---- test/review-loop-contract.test.js | 45 +++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/commands/do/pr.md b/commands/do/pr.md index beb24d0..f7128a5 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`, retrying once after `git pull --rebase --autostash` on a non-fast-forward) and only then create the PR. 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,30 @@ 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 + git log --oneline @{u}..HEAD # must be empty to merge + fi + ``` + + If that output is non-empty, **do not merge**: print the unpushed SHAs, leave the PR open, and report that the branch has unmerged 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/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index 57d7468..699f5bc 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -73,6 +73,26 @@ 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 + ``` + + If `UNPUSHED` is non-empty, the inner loop's push step did not run. Push now with the same retry the loop files use — `git push`; on a non-fast-forward, `git pull --rebase --autostash` once, then retry. If the push still fails, **record the pass as `push-failed`** (overriding whatever status the inner loop returned) and print the unpushed SHAs so the user can see exactly what is stranded. + + 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 status- and posture-agnostic.** It runs on every pass regardless of the inner loop's status, and applies unchanged when `{REVIEWER_APPLIES}=true` — that flag changes who *edits*, not who *pushes*. + - **It only ever pushes *forward*.** A hard-error pass (`broken-build`/`test-failed`/`rejected`) already reverted to its own start commit, so `@{u}..HEAD` comes back empty and nothing is pushed. The check publishes work that exists locally; it never force-pushes and never rewrites what the remote already has. ### Stop-mode decision @@ -81,13 +101,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`) @@ -104,7 +126,8 @@ Run only when `{REVIEW_MODE}=parallel` was explicitly resolved (flag or saved de 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. +6. **Assert the applied fixes reached the remote** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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 +156,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 (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 -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..0f1667e 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,48 @@ 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, /`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`/); + }); + + 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\.\*\*/); + assert.match(pr, /git log --oneline @\{u\}\.\.HEAD {3}# must be empty to merge/); + assert.match(pr, /Never merge on `dirty`\/`inconclusive`, never merge while the branch has unpushed commits/); + }); }); From ef0ecf804dd6c95e3265cf42d8e136c2b2a6e910 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:15:20 -0700 Subject: [PATCH 03/18] docs([issue-134]): log issue #134 to changelog --- .changelogs/NEXT.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .changelogs/NEXT.md diff --git a/.changelogs/NEXT.md b/.changelogs/NEXT.md new file mode 100644 index 0000000..9267e80 --- /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, and `/do:pr` refuses to open a PR or 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. From a11eb495e2cb1fcea4ae1f0d89a3c2c863b21358 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:17:28 -0700 Subject: [PATCH 04/18] address self-review: fix stale step reference, ~opt contract, and changelog claim --- .changelogs/NEXT.md | 2 +- lib/multi-reviewer-loop.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changelogs/NEXT.md b/.changelogs/NEXT.md index 9267e80..4a54b84 100644 --- a/.changelogs/NEXT.md +++ b/.changelogs/NEXT.md @@ -1,4 +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, and `/do:pr` refuses to open a PR or 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. +- **[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/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index 699f5bc..cf85a4a 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 @@ -125,7 +125,7 @@ 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. +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** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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. From b4e0a74c20fb5659483291bc90aab807b8b963e2 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:25:17 -0700 Subject: [PATCH 05/18] address review (claude): scope push assertion to the pass, use an explicit refspec, keep hard-errors dirty - The inconclusive rule's trailing '~opt passes are ignored here' contradicted the push-failed carve-out three clauses earlier, so an ~opt reviewer's stranded fixes could still compute clean and merge. Scope the exemption to the statuses it covers. - Push with an explicit refspec: a bare 'git push' under push.default=matching fans out to every same-named local branch, including a release branch that auto-tags. - Skip the assertion when the pass committed nothing, so it never publishes deliberately-unpushed local work on a pass that had no fixes of its own. - A hard-error keeps its status instead of being rewritten to push-failed, which would downgrade the aggregate from dirty to inconclusive and slip past do:pr's abort-before-creating-the-PR gate. - Add the missing push-failed enumeration to do:release's restatement of the rule. --- commands/do/pr.md | 2 +- commands/do/release.md | 2 +- lib/multi-reviewer-loop.md | 16 +++++++++----- test/review-loop-contract.test.js | 35 +++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/commands/do/pr.md b/commands/do/pr.md index f7128a5..8861be4 100644 --- a/commands/do/pr.md +++ b/commands/do/pr.md @@ -141,7 +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`, retrying once after `git pull --rebase --autostash` on a non-fast-forward) and only then create the PR. 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. +- **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) 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 cf85a4a..759047f 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -85,14 +85,20 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe else UNPUSHED="" # no upstream — nothing to assert 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. + [ "$PASS_START_SHA" = "$(git rev-parse HEAD)" ] && UNPUSHED="" ``` - If `UNPUSHED` is non-empty, the inner loop's push step did not run. Push now with the same retry the loop files use — `git push`; on a non-fast-forward, `git pull --rebase --autostash` once, then retry. If the push still fails, **record the pass as `push-failed`** (overriding whatever status the inner loop returned) and print the unpushed SHAs so the user can see exactly what is stranded. + If `UNPUSHED` is non-empty, the inner loop's push step did not run. Push now with the same retry the loop files use — `git push origin HEAD`; on a non-fast-forward, `git pull --rebase --autostash` once, then retry. **Push the branch explicitly, never a bare `git push`**: under `push.default=matching` a bare push fans out to every local branch that has a same-named remote (publishing unrelated branches — including a `release` branch that may auto-tag and publish), and under `push.default=nothing` it errors outright. `origin HEAD` is the loop files' `git push origin {BRANCH_NAME}` without requiring the wrapper to carry a branch-name input. + + 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 status- and posture-agnostic.** It runs on every pass regardless of the inner loop's status, and applies unchanged when `{REVIEWER_APPLIES}=true` — that flag changes who *edits*, not who *pushes*. - - **It only ever pushes *forward*.** A hard-error pass (`broken-build`/`test-failed`/`rejected`) already reverted to its own start commit, so `@{u}..HEAD` comes back empty and nothing is pushed. The check publishes work that exists locally; it never force-pushes and never rewrites what the remote already has. + - **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.** A hard-error pass (`broken-build`/`test-failed`/`rejected`) already reverted to its own start commit, so `PASS_START_SHA == HEAD` and the check is a no-op. 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. ### Stop-mode decision @@ -126,7 +132,7 @@ Run only when `{REVIEW_MODE}=parallel` was explicitly resolved (flag or saved de 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 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** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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. +6. **Assert the applied fixes reached the remote** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream) or when the apply committed nothing (`PARALLEL_START_SHA == HEAD`), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with `git push origin HEAD` (never a bare `git push` — see step 5) and the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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. @@ -165,7 +171,7 @@ A `push-failed` row must name the inner loop's original status and list the stra `{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), **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 (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 diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 0f1667e..23ec144 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -134,6 +134,41 @@ describe('review-loop parse contracts', () => { 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. + assert.doesNotMatch( + wrapper, + /Passes marked `~opt` are ignored here \(see the exclusion note above\)/, + 'the ~opt exemption must be scoped to the inconclusive statuses, not stated unconditionally', + ); + 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, /\[ "\$PASS_START_SHA" = "\$\(git rev-parse HEAD\)" \] && UNPUSHED=""/); + assert.match(wrapper, /\*\*Push the branch explicitly, never a bare `git push`\*\*/); + assert.match(wrapper, /`git push origin HEAD`/); + assert.match(wrapper, /`PARALLEL_START_SHA == HEAD`/, 'parallel mode needs the same zero-commit scoping'); + + 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', () => { From 4d7b5253ba60ff0f497f0939372c1064d0a74fd2 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:31:04 -0700 Subject: [PATCH 06/18] address review (claude): derive the push target from @{u}, not the local branch name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git push origin HEAD` resolves to `refs/heads/` and ignores the upstream, so on a branch whose upstream is named differently it creates a spurious remote branch, leaves the real PR head stale, and `@{u}..HEAD` stays non-empty — issue #134's exact failure, reintroduced inside the guard against it. Reproduced in a scratch repo before fixing. The push target is now derived from the upstream ref the check already resolves. Also from this round: - Run the assertion on hard-error passes rather than asserting they always no-op. A faithfully-executed loop reverts to its start commit and the check falls through, but an improvised loop body may have skipped the revert — which is the case this whole assertion exists for. - Use `if` instead of a trailing `[ … ] && …`, so the snippet doesn't exit non-zero on the live path and read as a failed command to the orchestrator. - Make the test guards structural: a reworded but still-unqualified `~opt` exemption would have slipped past the literal-string match. --- lib/multi-reviewer-loop.md | 21 +++++++++++++++------ test/review-loop-contract.test.js | 26 ++++++++++++++++++++------ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index 759047f..407d6d7 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -80,25 +80,34 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe # /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 + if UPSTREAM="$(git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null)"; then UNPUSHED="$(git log --oneline @{u}..HEAD)" + # Derive the push target from the upstream itself, not the local branch name. + # e.g. origin/next/issue-134 -> remote "origin", branch "next/issue-134". + PUSH_REMOTE="${UPSTREAM%%/*}"; PUSH_BRANCH="${UPSTREAM#*/}" else UNPUSHED="" # no upstream — nothing to assert 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. - [ "$PASS_START_SHA" = "$(git rev-parse HEAD)" ] && UNPUSHED="" + # 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 ``` - If `UNPUSHED` is non-empty, the inner loop's push step did not run. Push now with the same retry the loop files use — `git push origin HEAD`; on a non-fast-forward, `git pull --rebase --autostash` once, then retry. **Push the branch explicitly, never a bare `git push`**: under `push.default=matching` a bare push fans out to every local branch that has a same-named remote (publishing unrelated branches — including a `release` branch that may auto-tag and publish), and under `push.default=nothing` it errors outright. `origin HEAD` is the loop files' `git push origin {BRANCH_NAME}` without requiring the wrapper to carry a branch-name input. + If `UNPUSHED` is non-empty, the inner loop's push step did not run. Push now with the same retry the loop files use — `git push "$PUSH_REMOTE" "HEAD:refs/heads/$PUSH_BRANCH"`; on a non-fast-forward, `git pull --rebase --autostash` once, then retry. + + **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 `` from `@{u}` is what makes the push land on the branch the PR was opened from. 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.** A hard-error pass (`broken-build`/`test-failed`/`rejected`) already reverted to its own start commit, so `PASS_START_SHA == HEAD` and the check is a no-op. 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. + - **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 @@ -132,7 +141,7 @@ Run only when `{REVIEW_MODE}=parallel` was explicitly resolved (flag or saved de 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 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** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream) or when the apply committed nothing (`PARALLEL_START_SHA == HEAD`), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with `git push origin HEAD` (never a bare `git push` — see step 5) and the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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. +6. **Assert the applied fixes reached the remote** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream) or when the apply committed nothing (`PARALLEL_START_SHA == HEAD`), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with `git push "$PUSH_REMOTE" "HEAD:refs/heads/$PUSH_BRANCH"` derived from `@{u}` (never a bare `git push`, never `git push origin HEAD` — see step 5 for why both are wrong) and the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 23ec144..726bcaf 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -112,7 +112,8 @@ describe('review-loop parse contracts', () => { 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, /if UPSTREAM="\$\(git rev-parse --abbrev-ref --symbolic-full-name @\{u\} 2>\/dev\/null\)"; then/); + assert.match(wrapper, /UNPUSHED="" # no upstream — nothing to assert/); 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. @@ -138,10 +139,13 @@ describe('review-loop parse contracts', () => { // 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, - /Passes marked `~opt` are ignored here \(see the exclusion note above\)/, - 'the ~opt exemption must be scoped to the inconclusive statuses, not stated unconditionally', + /whose status is `push-failed`[\s\S]{0,600}?(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 @@ -159,10 +163,20 @@ describe('review-loop parse contracts', () => { // `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, /\[ "\$PASS_START_SHA" = "\$\(git rev-parse HEAD\)" \] && UNPUSHED=""/); - assert.match(wrapper, /\*\*Push the branch explicitly, never a bare `git push`\*\*/); - assert.match(wrapper, /`git push origin HEAD`/); + assert.match(wrapper, /if \[ "\$PASS_START_SHA" = "\$\(git rev-parse HEAD\)" \]; then/); assert.match(wrapper, /`PARALLEL_START_SHA == HEAD`/, 'parallel mode needs the same zero-commit scoping'); + // 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. + assert.match(wrapper, /PUSH_REMOTE="\$\{UPSTREAM%%\/\*\}"; PUSH_BRANCH="\$\{UPSTREAM#\*\/\}"/); + assert.match(wrapper, /git push "\$PUSH_REMOTE" "HEAD:refs\/heads\/\$PUSH_BRANCH"/); + assert.match(wrapper, /not a bare `git push`, and not `git push origin HEAD`/); + assert.doesNotMatch( + wrapper, + /push with `git push origin HEAD`/, + 'the push target must be derived from the upstream ref, not the local branch name', + ); const pr = readCommand('pr.md'); assert.match(pr, /`git push origin \{current_branch\}` — an explicit refspec, never a bare `git push`/); From 3ccec0a9e3cec1788072dd084d1af41a95d2ddca Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 14:37:29 -0700 Subject: [PATCH 07/18] address review (claude): derive the push target from config, and push in the same shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failure modes reproduced in a scratch repo before fixing: - Splitting the abbrev-ref mangles two real upstream shapes. A branch tracking a LOCAL branch (`branch..remote=.`, what `git branch --set-upstream-to=main` produces) abbreviates to a bare `main`, so both halves of the split return `main` and the push targets a nonexistent remote — and on such a branch `@{u}..HEAD` is non-empty by construction, so the assertion fires on a healthy branch and records a false `push-failed` that blocks the merge. A remote whose name contains a slash (`up/stream/main`) mis-splits the same way. Derive remote and branch from `branch..remote` / `.merge` instead, and treat a local upstream as "no remote to assert against". Since `.merge` is already fully qualified the destination is `HEAD:$PUSH_BRANCH`, not `HEAD:refs/heads/…`. - The push was prescribed in prose OUTSIDE the fenced block that defined PUSH_REMOTE/PUSH_BRANCH. Spec snippets run as separate Bash invocations where shell variables do not persist, so it would have executed against the empty-string remote and reported another false `push-failed`. Fold the push and its retry into the same block; parallel step 6 now reuses that whole block rather than restating a push whose variables nothing in that section defines. Tests made structural: every prescribed push is scanned from raw text (the in-block occurrences carry no backticks) and must use the derived form. --- lib/multi-reviewer-loop.md | 29 +++++++++++++++------ test/review-loop-contract.test.js | 42 +++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index 407d6d7..f1ab7b2 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -80,14 +80,22 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe # /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 UPSTREAM="$(git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null)"; then + if git rev-parse --abbrev-ref --symbolic-full-name @{u} >/dev/null 2>&1; then UNPUSHED="$(git log --oneline @{u}..HEAD)" - # Derive the push target from the upstream itself, not the local branch name. - # e.g. origin/next/issue-134 -> remote "origin", branch "next/issue-134". - PUSH_REMOTE="${UPSTREAM%%/*}"; PUSH_BRANCH="${UPSTREAM#*/}" 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 @@ -96,11 +104,18 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe 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 + git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH" || + { git pull --rebase --autostash && git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"; } + fi ``` - If `UNPUSHED` is non-empty, the inner loop's push step did not run. Push now with the same retry the loop files use — `git push "$PUSH_REMOTE" "HEAD:refs/heads/$PUSH_BRANCH"`; on a non-fast-forward, `git pull --rebase --autostash` once, then retry. + 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 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 `` from `@{u}` is what makes the push land on the branch the PR was opened from. + **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. @@ -141,7 +156,7 @@ Run only when `{REVIEW_MODE}=parallel` was explicitly resolved (flag or saved de 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 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** — the identical check the series dispatch's step 5 defines, applied once here after the union push: skip when `git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails (no upstream) or when the apply committed nothing (`PARALLEL_START_SHA == HEAD`), otherwise require `git log --oneline @{u}..HEAD` to be empty; if it isn't, push with `git push "$PUSH_REMOTE" "HEAD:refs/heads/$PUSH_BRANCH"` derived from `@{u}` (never a bare `git push`, never `git push origin HEAD` — see step 5 for why both are wrong) and the one `git pull --rebase --autostash` retry, and record `push-failed` if it still fails. 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. +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. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 726bcaf..db82a5b 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -112,8 +112,8 @@ describe('review-loop parse contracts', () => { 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, /if UPSTREAM="\$\(git rev-parse --abbrev-ref --symbolic-full-name @\{u\} 2>\/dev\/null\)"; then/); - assert.match(wrapper, /UNPUSHED="" # no upstream — nothing to assert/); + 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. @@ -164,19 +164,47 @@ describe('review-loop parse contracts', () => { // 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/); - assert.match(wrapper, /`PARALLEL_START_SHA == HEAD`/, 'parallel mode needs the same zero-commit scoping'); + // 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. - assert.match(wrapper, /PUSH_REMOTE="\$\{UPSTREAM%%\/\*\}"; PUSH_BRANCH="\$\{UPSTREAM#\*\/\}"/); - assert.match(wrapper, /git push "\$PUSH_REMOTE" "HEAD:refs\/heads\/\$PUSH_BRANCH"/); + // 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, - /push with `git push origin HEAD`/, - 'the push target must be derived from the upstream ref, not the local branch name', + /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:"`. + assert.match(wrapper, /if \[ -n "\$UNPUSHED" \]; then\n\s*git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); const pr = readCommand('pr.md'); assert.match(pr, /`git push origin \{current_branch\}` — an explicit refspec, never a bare `git push`/); From 1d029cb4534765ae5969620255ab16b229432296 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 15:02:29 -0700 Subject: [PATCH 08/18] address review (ollama): make the merge gate fail closed and the clean rule self-contained MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - do:pr's unpushed-commits gate printed the SHAs and left the decision to adjacent prose. Since the failure this whole change guards against IS a step being skimmed past, the snippet now exits non-zero instead of emitting output someone has to interpret. - The `clean` aggregate rule excluded push-failed only by rule ordering (the inconclusive rule matches first). State it explicitly so the rule is true on its own terms — the round-1 CRITICAL in this same file was exactly a later sentence quietly contradicting an earlier carve-out. - Relax three test regexes that pinned incidental formatting (an exact three-space comment gap, an adjacency assumption between a guard and its body, and a 600-char proximity window) so a reformat with no behavior change can't fail them. Rejected from this pass: the claim that `git rev-parse --abbrev-ref --symbolic-full-name` combines conflicting flags and errors. It is a standard idiom and ran correctly here on every upstream shape tested. Also rejected a suggestion to let the prescribed-push scan span newlines, which would make the regex greedy across the retry line and match less precisely than it does now. --- commands/do/pr.md | 8 ++++++-- lib/multi-reviewer-loop.md | 2 +- test/review-loop-contract.test.js | 12 +++++++++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/commands/do/pr.md b/commands/do/pr.md index 8861be4..8cc37fb 100644 --- a/commands/do/pr.md +++ b/commands/do/pr.md @@ -203,11 +203,15 @@ When `MERGE_ENABLED=true`, gate the merge on **all three** of the review result, ```bash if git rev-parse --abbrev-ref --symbolic-full-name @{u} >/dev/null 2>&1; then - git log --oneline @{u}..HEAD # must be empty to merge + 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 ``` - If that output is non-empty, **do not merge**: print the unpushed SHAs, leave the PR open, and report that the branch has unmerged 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. + 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`. diff --git a/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index f1ab7b2..e6f7d98 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -197,7 +197,7 @@ A `push-failed` row must name the inner loop's original status and list the stra - `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), **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`; `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). diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index db82a5b..a350110 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -144,7 +144,7 @@ describe('review-loop parse contracts', () => { // contradiction while a literal-string guard kept passing. assert.doesNotMatch( wrapper, - /whose status is `push-failed`[\s\S]{0,600}?(Passes marked `~opt` are ignored here\.|`~opt` passes are ignored here\.|are ignored here \(see)/, + /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\}`/); @@ -204,7 +204,9 @@ describe('review-loop parse contracts', () => { ); // 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:"`. - assert.match(wrapper, /if \[ -n "\$UNPUSHED" \]; then\n\s*git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); + // Tolerate an intervening comment line — the point is that the push lives inside + // the guard, not that it sits on the literally-next line. + assert.match(wrapper, /if \[ -n "\$UNPUSHED" \]; then\s*(?:#[^\n]*\n\s*)*git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); const pr = readCommand('pr.md'); assert.match(pr, /`git push origin \{current_branch\}` — an explicit refspec, never a bare `git push`/); @@ -219,7 +221,11 @@ describe('review-loop parse contracts', () => { 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\.\*\*/); - assert.match(pr, /git log --oneline @\{u\}\.\.HEAD {3}# must be empty to merge/); + // 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/); }); }); From c68b15a9f322624e2984ada1b00a4b87a082a71e Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 15:42:12 -0700 Subject: [PATCH 09/18] address review (claude): abort a conflicted retry rebase instead of stranding the branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry's `git pull --rebase --autostash` can hit a content conflict, and nothing aborted the half-finished rebase. Reproduced end to end: after a concurrent push to the same branch plus an overlapping edit, the block left `rebase in progress`, HEAD detached, the tree conflicted, and the pass's own fix commit reachable only as REBASE_HEAD. That is worse than the state issue #134 describes, because `push-failed` is a continue-signal: the wrapper goes on to the next reviewer, which now runs against a detached, conflicted tree — and that reviewer's own push assertion silently no-ops, since a detached HEAD has no resolvable @{u}, so PUSH_REMOTE is empty and the check skips. The guard built to surface stranded fixes would report nothing. The inner loops carry the same pull-rebase-then-push idiom, but there it is the loop's last act before reporting failure; here execution deliberately continues, which is what turns a recoverable failure into a corrupted tree for every later pass. Abort the rebase on the conflict path and keep the non-zero exit that drives `push-failed`, so the branch is always returned exactly as the reviewer left it. --- lib/multi-reviewer-loop.md | 15 ++++++++++++--- test/review-loop-contract.test.js | 10 +++++++--- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/multi-reviewer-loop.md b/lib/multi-reviewer-loop.md index e6f7d98..b084b59 100644 --- a/lib/multi-reviewer-loop.md +++ b/lib/multi-reviewer-loop.md @@ -108,12 +108,21 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe # 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 - git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH" || - { git pull --rebase --autostash && git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"; } + 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 push still fails, record the pass per the rules below. + 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`. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index a350110..d8fe6bf 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -204,9 +204,13 @@ describe('review-loop parse contracts', () => { ); // 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:"`. - // Tolerate an intervening comment line — the point is that the push lives inside - // the guard, not that it sits on the literally-next line. - assert.match(wrapper, /if \[ -n "\$UNPUSHED" \]; then\s*(?:#[^\n]*\n\s*)*git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); + // 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). + assert.match(wrapper, /git rebase --abort/, 'a conflicted retry must not leave a rebase in progress'); const pr = readCommand('pr.md'); assert.match(pr, /`git push origin \{current_branch\}` — an explicit refspec, never a bare `git push`/); From 6645cb8b1b9c1e96f97e55197281f624221f8dae Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 15:46:16 -0700 Subject: [PATCH 10/18] address review (claude): anchor the rebase-abort assertion to the else-branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bare 'false' — the line that makes a failed push observable to the orchestrator — was guarded by nothing. Deleting it left the suite green while flipping the abort path's exit status to 0 (git rebase --abort succeeds), so a genuine push failure would read as success, the pass would never be recorded push-failed, and the stranded commits would reach the merge gate: issue #134 verbatim. The old assertion also matched 'git rebase --abort' anywhere in the file, so moving the abort out of the else-branch and merely naming it in prose kept the suite green with a conflicted rebase left in place. One anchored assertion catches both. Verified by re-running both mutations: each now fails exactly one test. --- test/review-loop-contract.test.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index d8fe6bf..1402ec9 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -210,7 +210,16 @@ describe('review-loop parse contracts', () => { // 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). - assert.match(wrapper, /git rebase --abort/, 'a conflicted retry must not leave a rebase in progress'); + // 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`/); From 00e9b37f6f9e9ecf3cfacaa4c590dde2635d8fdd Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" <70015+atomantic@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:22:10 +0000 Subject: [PATCH 11/18] fix: guard possibly-empty array expansions so bash 3.2 can't fake a reviewer cli-error (#139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: guard possibly-empty array expansions so bash 3.2 can't fake a reviewer cli-error (#138) Stock macOS ships neither timeout(1) nor gtimeout, so TIMEOUT_CMD is legitimately empty there. Under /bin/bash 3.2 a bare "${ARR[@]}" on an empty array is an unset expansion, so with set -u it aborts with 'unbound variable' before the reviewer ever starts. Every file then returns RC=1 with empty output, the loop counts them all as REVIEW_ERRORS, and the pass resolves to cli-error -- a hard error that ~opt does not excuse -- blocking the merge on a PR no reviewer looked at. Switch TIMEOUT_CMD, MODEL_FLAG, and OLLAMA_FLAGS to the ${ARR[@]+"${ARR[@]}"} form across the ollama, local-agent, and enhance loops; document that an absent timeout binary is the common macOS case and a supported configuration rather than a reviewer failure; and add contract tests that fail if an unguarded expansion returns. * address review (claude+antigravity): collapse double-wrapped guards and hoist the rule into one partial The mechanical rewrite in the previous commit wrapped three already-guarded expansions a second time, publishing a competing spelling of the very rule this change exists to teach. Collapse them, and move the five near-identical copies of the explanation into lib/empty-array-expansion.md (the lib/gh-host.md convention) so the next amendment can't land in two files and rot in three. The contract test now scans by pattern instead of a hardcoded array-name list, so a newly introduced optional-argument array is covered the day it lands, and it also rejects the double-wrap slip. Its companion no longer pins the loops' prose wording -- it asserts the shared partial carries the contract and that each loop links to it. Registering the new partial in install.sh/uninstall.sh was caught by the existing curl-installer LIBS allowlist test. * address review (antigravity): catch the quoted form of a double-wrapped guard too ${A[@]+"${A[@]+…}"} is the same slip as ${A[@]+${A[@]+…}} but slipped past the detector, so the contract test allowed exactly one spelling of the mistake it exists to reject. --- install.sh | 2 +- lib/empty-array-expansion.md | 41 +++++++++++++++++++++ lib/enhance-loop.md | 22 ++++++----- lib/local-agent-review-loop.md | 31 +++++++++------- lib/ollama-review-loop.md | 9 +++-- test/review-loop-contract.test.js | 61 +++++++++++++++++++++++++++++++ uninstall.sh | 2 +- 7 files changed, 140 insertions(+), 28 deletions(-) create mode 100644 lib/empty-array-expansion.md diff --git a/install.sh b/install.sh index a8707a9..8c2da6d 100755 --- a/install.sh +++ b/install.sh @@ -60,7 +60,7 @@ OLD_COMMANDS=(cam good makegoals makegood optimize-md) # enumerates lib/ dynamically, so it doesn't need updating. LIBS=( ci-flake-handling code-review-checklist copilot-review-loop - enhance-loop epic-children + empty-array-expansion enhance-loop epic-children finding-disposition fix-regression-guard gh-host github-reviewer-loop graphql-escaping local-agent-review-loop multi-reviewer-loop ollama-review-loop diff --git a/lib/empty-array-expansion.md b/lib/empty-array-expansion.md new file mode 100644 index 0000000..66e2649 --- /dev/null +++ b/lib/empty-array-expansion.md @@ -0,0 +1,41 @@ +## Expanding a possibly-empty shell array (`${ARR[@]+"${ARR[@]}"}`) + +**The rule.** Any array that can legitimately be empty — `TIMEOUT_CMD`, `MODEL_FLAG`, +`OLLAMA_FLAGS`, or any other optional-argument array — must be expanded as: + +```bash +${ARR[@]+"${ARR[@]}"} # correct +"${ARR[@]}" # WRONG when ARR can be empty +``` + +**Why the guard is required, not decoration.** Under **bash 3.2** — still `/bin/bash` +on macOS — a bare `"${ARR[@]}"` on an *empty* array is an **unset** expansion, so with +`set -u` the shell aborts with `ARR[@]: unbound variable` **before the command runs**: + +``` +$ /bin/bash -c 'set -u; A=(); "${A[@]}" echo ok' +/bin/bash: A[@]: unbound variable # exit 127 — echo never ran + +$ /bin/bash -c 'set -u; A=(); ${A[@]+"${A[@]}"} echo ok' +ok # exit 0 +``` + +The `${ARR[@]+…}` form suppresses the expansion entirely when the array is unset or +empty, and is identical to `"${ARR[@]}"` when it is not — quoting and word boundaries +are preserved, so an element containing spaces still arrives as one argument. It is +safe on bash 3.2+, bash 4/5, and zsh alike, so it is the only form to write. + +**Why it matters here specifically.** Stock macOS ships **neither** `timeout(1)` (GNU +coreutils) nor `gtimeout` (the Homebrew-prefixed coreutils build), so a probing +`TIMEOUT_CMD=()` stays empty on a typical Mac — the *common* case, not an edge case. +Running without a timeout wrapper is a **supported configuration** (the invocation is +bounded by the CLI's own limits instead) and must never be recorded as a reviewer +failure. But with an unguarded expansion the review loops fail in the worst possible +way: every invocation aborts before the reviewer starts, returning RC=1 with empty +output, so the loop counts each file as a review error and the pass resolves to +`cli-error` — a hard error that `~opt` does **not** excuse — blocking the merge on a +PR no reviewer ever looked at. + +**Diagnosing it.** Every file erroring with an empty response, and nothing but +`unbound variable` in the captured stderr, is this bug — an environment condition, not +a reviewer failure. Fix the expansion and re-run the pass; do not record `cli-error`. diff --git a/lib/enhance-loop.md b/lib/enhance-loop.md index e74d102..4be6bdc 100644 --- a/lib/enhance-loop.md +++ b/lib/enhance-loop.md @@ -103,11 +103,15 @@ Resolve the timeout wrapper — the only genuinely run-once piece; this is settl logic; run it, don't narrate it: ```bash -# macOS ships no timeout(1) unless coreutils is installed; probe for it. Empty array = -# no wrapper (rely on each CLI's own limits). An ARRAY, not a string: zsh (a common -# host shell) does not word-split an unquoted expansion, so a two-word string like -# 'timeout 1800' would be executed as one bogus command name; the array expands to -# separate words in bash and zsh alike, and to zero words when empty. Enhancement is +# Stock macOS ships NEITHER timeout(1) (GNU coreutils) nor gtimeout (Homebrew +# coreutils), so probe for them and expect the empty array — the common case, not an +# edge case. Empty array = no wrapper (rely on each CLI's own limits). An ARRAY, not a +# string: zsh (a common host shell) does not word-split an unquoted expansion, so a +# two-word string like 'timeout 1800' would be executed as one bogus command name; the +# array expands to separate words in bash and zsh alike. Expand it (and MODEL_FLAG) +# only in the guarded ${ARR[@]+"${ARR[@]}"} form — the bare form aborts under bash 3.2 +# + `set -u` before the CLI runs; see ~/.claude/lib/empty-array-expansion.md. +# Enhancement is # lighter than a full review (no build/test), but a large draft on a heavy model can # still exceed the ~10-min host foreground cap, so the same background+poll launch # below is used. @@ -151,11 +155,11 @@ as a positional argument (never via stdin) and prints the improved draft to stdo | `claude` | Dispatch an in-process sub-agent via the `Agent` tool (`subagent_type: "general-purpose"`, prompt `$ENHANCE_PROMPT`, `model` = `{ENH_MODEL}` when set) — **not** `claude -p`, so it stays on the host session's plan billing instead of hitting the API. Its returned message is the agent's stdout. | -| `claude` | `claude -p "$ENHANCE_PROMPT" "${MODEL_FLAG[@]}" --dangerously-skip-permissions` | +| `claude` | `claude -p "$ENHANCE_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --dangerously-skip-permissions` | -| `codex` | `codex "${MODEL_FLAG[@]}" --sandbox read-only -a never exec "$ENHANCE_PROMPT"` — `exec` (free-form prompt) is the right subcommand here, not `codex review`; `-m`/`--model`, `--sandbox`, and `-a` are all top-level flags that MUST precede `exec`. `--sandbox read-only` enforces the read-only contract at the sandbox level while still allowing tree reads and git queries — the same posture `lib/local-agent-review-loop.md` uses for its review-only codex pass (only its *reviewer-applies* path needs `danger-full-access`). | +| `codex` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --sandbox read-only -a never exec "$ENHANCE_PROMPT"` — `exec` (free-form prompt) is the right subcommand here, not `codex review`; `-m`/`--model`, `--sandbox`, and `-a` are all top-level flags that MUST precede `exec`. `--sandbox read-only` enforces the read-only contract at the sandbox level while still allowing tree reads and git queries — the same posture `lib/local-agent-review-loop.md` uses for its review-only codex pass (only its *reviewer-applies* path needs `danger-full-access`). | | `agy` | `agy --dangerously-skip-permissions --model "$AGY_ENH_MODEL" --print-timeout 30m -p "$ENHANCE_PROMPT"` | -| `grok` | `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p "$ENHANCE_PROMPT"` | +| `grok` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p "$ENHANCE_PROMPT"` | **Grok flag rationale.** `grok -p`/`--single ` runs a single-turn headless prompt, prints the response to stdout, and exits — the grok analog of `claude -p` / @@ -247,7 +251,7 @@ one's output): # after <<>> to end-of-output", so any stderr the CLI emits after # the answer (telemetry warnings, timing/shutdown lines, update nags) would be # pasted verbatim into the enhanced draft and end up in the filed issue. - "${TIMEOUT_CMD[@]}" {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" + ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" ``` Then wait with **bounded blocking-chunk foreground calls** — do NOT end your turn to wait for a notification (a stopped subagent is dead, not waiting): diff --git a/lib/local-agent-review-loop.md b/lib/local-agent-review-loop.md index 5365413..3704583 100644 --- a/lib/local-agent-review-loop.md +++ b/lib/local-agent-review-loop.md @@ -78,10 +78,15 @@ fi # codex doesn't have slashdo installed, so describe the task directly). CODEX_APPLY_PROMPT="Review the diff from $BASE_BRANCH to HEAD in this repo for logic issues (correctness, security, test coverage, contract drift). The linter, type-checker, and test suite already run separately — do NOT spend effort on syntax, lint, formatting, or build errors, and do NOT raise style/rename/extract-a-helper suggestions; report only behavior bugs you can tie to a concrete wrong outcome. For each finding, apply the fix in the working tree, then run \`$BUILD_CMD\` (skip if empty) and \`$TEST_CMD\` to verify, and commit each fix with message 'address review (codex): '. Do not introduce changes beyond the scope of fixing the findings. Do not skip tests or weaken assertions." -# Resolve the timeout wrapper used by the step-2 invocation (`"${TIMEOUT_CMD[@]}" {INVOCATION}`). -# macOS ships no `timeout(1)` unless coreutils is installed, so probing is required: -# bare `timeout 1800 …` would exit 127 before the reviewer runs. Empty array = no wrapper -# (rely on the CLI's own internal limits). An ARRAY, not a string, for the same zsh +# Resolve the timeout wrapper used by the step-2 invocation +# (`${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}`). +# Stock macOS ships NEITHER `timeout(1)` (GNU coreutils) nor `gtimeout` (Homebrew +# coreutils), so an empty array is the common case here, not an edge case; bare +# `timeout 1800 …` would exit 127 before the reviewer runs. Empty array = no wrapper +# (rely on the CLI's own internal limits) — a supported configuration, never a +# reviewer failure; expand it (and MODEL_FLAG) only in the guarded +# ${ARR[@]+"${ARR[@]}"} form — see ~/.claude/lib/empty-array-expansion.md. +# An ARRAY, not a string, for the same zsh # reason as MODEL_FLAG below: zsh does not word-split an unquoted expansion, so a # two-word string ('timeout 1800') would be executed as one bogus command name and # fail every invocation precisely on machines that HAVE coreutils installed. @@ -132,17 +137,17 @@ Pick the invocation based on `{REVIEW_AGENT}` and `{REVIEWER_APPLIES}`: | `claude` | Dispatch an in-process sub-agent via the `Agent` tool with `subagent_type: "general-purpose"` and `$LOCAL_PROMPT` (see Step 2) — **not** `claude -p`, so it stays on plan billing | Same sub-agent dispatch; the sub-agent applies and commits fixes directly in the shared working tree | -| `claude` | `claude -p "$LOCAL_PROMPT" "${MODEL_FLAG[@]}" --dangerously-skip-permissions` | `claude -p "$LOCAL_PROMPT" "${MODEL_FLAG[@]}" --dangerously-skip-permissions` | +| `claude` | `claude -p "$LOCAL_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --dangerously-skip-permissions` | `claude -p "$LOCAL_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --dangerously-skip-permissions` | -| `codex` | `codex "${MODEL_FLAG[@]}" --sandbox read-only review --base "$BASE_BRANCH" --title "$REVIEW_TITLE"` | `codex "${MODEL_FLAG[@]}" --sandbox danger-full-access -a never exec "$CODEX_APPLY_PROMPT"` | +| `codex` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --sandbox read-only review --base "$BASE_BRANCH" --title "$REVIEW_TITLE"` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --sandbox danger-full-access -a never exec "$CODEX_APPLY_PROMPT"` | | `agy` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m -p "$LOCAL_PROMPT"` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m -p "$LOCAL_PROMPT"` | -| `grok` | `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p "$LOCAL_PROMPT"` | `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p "$LOCAL_PROMPT"` | +| `grok` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p "$LOCAL_PROMPT"` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p "$LOCAL_PROMPT"` | For `claude`, `agy`, and `grok`, the same `$LOCAL_PROMPT` drives both modes — it already encodes the mode (review-only vs reviewer-applies) directly, branching on `$REVIEWER_APPLIES` above. For `codex`, the invocation itself swaps because `codex review` (review-only) and `codex exec` (apply-fixes) are different subcommands with incompatible flag sets. `--print-timeout 30m` raises agy's print-mode wait above its 5-minute default so a real review of a multi-file diff isn't cut off mid-stream; on stock macOS (no `timeout`/`gtimeout`, so `TIMEOUT_CMD` is empty) it is also the only *shell-level* bound on the invocation. **But these bounds only take effect when the invocation runs in the background (Step 2).** Run as a blocking foreground Bash call, the run is killed first by the host tool's ~10-minute foreground cap — earlier than either `timeout 1800` or `--print-timeout 30m` — which is the timeout consumers were hitting. `--print-timeout 30m` does NOT cut off an actively-streaming agent — it bounds the wait for the *next* response chunk — which is why it's safe to set generously, and why it never masked the old skill hang (that hang was the orchestrator sitting idle waiting on background sub-agents, not a slow stream). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight): agy's *default* may be a heavy "Thinking" tier that spends many minutes in hidden reasoning plus multi-round tool calls. How much output is visible meanwhile is **model-dependent** — lighter models narrate their actions incrementally, heavy thinking tiers can emit nothing until the final answer — so on a slow model a routine review shows little or no output for 20-30 minutes and is easily mistaken for a hang. A quiet log during Step 2's poll is therefore NOT evidence the reviewer is stuck; only a `$DONE_FILE` with a non-zero code, or a 30-minute overrun, is. Pinning a fast-but-capable model keeps reviews prompt; bump `AGY_REVIEW_MODEL` to a heavier tier when you want more depth and accept the longer wait (the background launch + 30-minute bound cover it). > **Pass the prompt as a positional argument — never via stdin.** `claude -p`, `agy -p` (`--print`), and `grok -p` (`--single`) all take the prompt as the argument directly after the flag: `agy --dangerously-skip-permissions -p "$LOCAL_PROMPT"`, `grok --permission-mode bypassPermissions -p "$LOCAL_PROMPT"`. They do **not** read the prompt from stdin. Do NOT write `echo "$LOCAL_PROMPT" | agy --dangerously-skip-permissions -p`, `agy -p < prompt.txt`, or `printf … | agy -p` — agy ignores piped stdin and exits with `agy --print takes the prompt as an argument, not stdin`, forcing a wasted second invocation. The `> "$LOG_FILE" 2> "$ERR_FILE"` redirect in Step 2 captures the reviewer's *output*; it is unrelated to how the prompt goes in. Keep `"$LOCAL_PROMPT"` as the quoted argument to `-p` exactly as shown in the invocation table. -**Pinning the reviewer's model (`"${MODEL_FLAG[@]}"` / `--model`).** When `{REVIEW_MODEL}` is set (from an `[]` bracket or a saved `review-models` default — resolved by the caller), the reviewer runs on that model; when empty, `MODEL_FLAG` is an empty array so `codex`/`claude`/`grok` fall back to the CLI's own default. For **codex**, `-m`/`--model` is a **top-level** Codex option (like `--sandbox` and `-a`), so it MUST precede the `review`/`exec` subcommand — that is why `"${MODEL_FLAG[@]}"` sits before `--sandbox` in both codex invocations; passing it after the subcommand would exit 2 with an unexpected-argument error, exactly as `-a` does. (The two paths pass *different* sandbox policies — `read-only` for review-only, `danger-full-access` for reviewer-applies — see below.) For **claude**, `--model` is a session flag valid alongside `-p`. For **grok**, `-m`/`--model` is a session flag valid alongside `-p`, so `"${MODEL_FLAG[@]}"` sits inline in the invocation (empty array → grok's own default). For **agy**, the model is always pinned via `--model "$AGY_REVIEW_MODEL"` (resolved above with `{REVIEW_MODEL}` taking precedence over the `AGY_REVIEW_MODEL` env and the built-in default) — agy's own default may be a slow "Thinking" tier, so it is never left unpinned. Because the model string may contain spaces/parens, `MODEL_FLAG` is a shell array (see the pre-flight block) — never a bare string. +**Pinning the reviewer's model (`${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` / `--model`).** When `{REVIEW_MODEL}` is set (from an `[]` bracket or a saved `review-models` default — resolved by the caller), the reviewer runs on that model; when empty, `MODEL_FLAG` is an empty array so `codex`/`claude`/`grok` fall back to the CLI's own default. For **codex**, `-m`/`--model` is a **top-level** Codex option (like `--sandbox` and `-a`), so it MUST precede the `review`/`exec` subcommand — that is why `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` sits before `--sandbox` in both codex invocations; passing it after the subcommand would exit 2 with an unexpected-argument error, exactly as `-a` does. (The two paths pass *different* sandbox policies — `read-only` for review-only, `danger-full-access` for reviewer-applies — see below.) For **claude**, `--model` is a session flag valid alongside `-p`. For **grok**, `-m`/`--model` is a session flag valid alongside `-p`, so `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` sits inline in the invocation (empty array → grok's own default). For **agy**, the model is always pinned via `--model "$AGY_REVIEW_MODEL"` (resolved above with `{REVIEW_MODEL}` taking precedence over the `AGY_REVIEW_MODEL` env and the built-in default) — agy's own default may be a slow "Thinking" tier, so it is never left unpinned. Because the model string may contain spaces/parens, `MODEL_FLAG` is a shell array (see the pre-flight block) — never a bare string. Notes on each invocation: - **claude / agy / grok** run the self-contained `$LOCAL_PROMPT` (a single-agent inline review), **not** slashdo's `/do-review` skill — the skill's sub-agent fan-out never re-syncs into a print-mode/headless response, so it would hang and emit zero findings (see the `$LOCAL_PROMPT` rationale above). Under Claude Code the `claude` reviewer is an in-process sub-agent (via the `Agent` tool) that runs `$LOCAL_PROMPT` directly, rather than a `claude -p` subprocess — and because the prompt is a single-agent inline review, it does not recursively spawn the skill's own sub-agents. In `REVIEWER_APPLIES=true` mode, `$LOCAL_PROMPT` tells the CLI to apply each fix, verify with build+tests, commit as `address review (): ` (`` = the reviewing CLI's slug, `claude`, `agy`, or `grok`), and NOT push (the orchestrating agent verifies and pushes). The parenthesized agent name records which reviewer surfaced the finding, useful when scanning the log of a release that ran multiple reviewers. In `REVIEWER_APPLIES=false` mode, `$LOCAL_PROMPT` tells the CLI to emit `FINDING :` blocks (or `NO FINDINGS`) to stdout for the orchestrator to parse — the orchestrator then commits the fixes using the same `address review (): ` form to preserve attribution. @@ -157,8 +162,8 @@ Flag rationale (reckless / unattended mode): - **review-only → `read-only`.** Verified: `codex --sandbox read-only review --base ` reads the diff, tracked-file list, commit graph and base tree and returns normal severity-tagged findings, while `printf … > file` inside the repo fails with `zsh:1: operation not permitted`. Review quality is unaffected and the contract becomes unbypassable. This matches `lib/enhance-loop.md`, which already runs codex `--sandbox read-only` for the same reason. - **reviewer-applies → `danger-full-access`.** This path must write fixes, run build/tests, and reach the network unattended, so full access is the intended posture on a trusted single-user machine (mirrors `claude --dangerously-skip-permissions` / `agy --dangerously-skip-permissions`). - `--sandbox` and `-a` are independent top-level flags and may be combined (`codex --sandbox danger-full-access -a never exec …`). -- `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m` — `--dangerously-skip-permissions` auto-approves all tool permission requests so the Antigravity CLI runs unattended (the headless equivalent of confirming every prompt). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight, default `Gemini 3.5 Flash (High)`, override via `AGY_REVIEW_MODEL`): without it agy picks its own default, which may be a heavy "Thinking" tier that spends many minutes in hidden reasoning and — depending on the model, emits little or no visible output meanwhile — makes a review look hung for 20-30 minutes; a fast capable model returns in well under a minute on a small diff. This is the agy successor to the Gemini CLI's `gemini --yolo` + `env GEMINI_SANDBOX=false`: agy folds both "auto-approve tools" and "no sandbox gate" into the single flag, and runs the prompt non-interactively via `-p` — which takes the prompt as its positional argument (`agy … -p "$LOCAL_PROMPT"`), **not** from stdin. Piping into `agy -p` (e.g. `echo … | agy -p`) fails with `agy --print takes the prompt as an argument, not stdin` and wastes an invocation; always pass the quoted prompt as the argument. `--print-timeout 30m` raises the print-mode wait above agy's 5-minute default so a real multi-file review isn't cut off, and — since stock macOS has no `timeout`/`gtimeout` and `TIMEOUT_CMD` is empty — is the effective bound on the invocation; it bounds the wait for the next response chunk, not the total runtime, so an actively-streaming review is never truncated. Unlike the old gemini invocation, no `env VAR=…` prefix is needed, so it composes cleanly with the `"${TIMEOUT_CMD[@]}" {INVOCATION}` wrapper at step 2 of the loop when one is present. -- `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p` — `-p`/`--single` runs a single-turn headless prompt, prints the response to stdout, and exits (the grok analog of `claude -p` / `agy -p`). `--permission-mode bypassPermissions` auto-approves every tool execution so grok runs unattended (grok's equivalent of `--dangerously-skip-permissions`); it folds "auto-approve tools" into one flag, so no separate sandbox/`env VAR=…` prefix is needed and it composes cleanly with the `"${TIMEOUT_CMD[@]}" {INVOCATION}` wrapper. `"${MODEL_FLAG[@]}"` pins the reviewing model for a `grok[]` bracket (empty array → grok's own default; grok accepts the long `--model` form alongside `-p`). Like `agy -p`, `grok -p` takes the prompt as its positional argument — **not** from stdin (`grok … -p "$LOCAL_PROMPT"`); do not pipe into it. Grok has no `--print-timeout` equivalent, so the run is bounded by `TIMEOUT_CMD` (when present) and grok's own internal limits — the same background-launch + poll in Step 2 keeps it off the host's ~10-minute foreground cap. +- `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m` — `--dangerously-skip-permissions` auto-approves all tool permission requests so the Antigravity CLI runs unattended (the headless equivalent of confirming every prompt). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight, default `Gemini 3.5 Flash (High)`, override via `AGY_REVIEW_MODEL`): without it agy picks its own default, which may be a heavy "Thinking" tier that spends many minutes in hidden reasoning and — depending on the model, emits little or no visible output meanwhile — makes a review look hung for 20-30 minutes; a fast capable model returns in well under a minute on a small diff. This is the agy successor to the Gemini CLI's `gemini --yolo` + `env GEMINI_SANDBOX=false`: agy folds both "auto-approve tools" and "no sandbox gate" into the single flag, and runs the prompt non-interactively via `-p` — which takes the prompt as its positional argument (`agy … -p "$LOCAL_PROMPT"`), **not** from stdin. Piping into `agy -p` (e.g. `echo … | agy -p`) fails with `agy --print takes the prompt as an argument, not stdin` and wastes an invocation; always pass the quoted prompt as the argument. `--print-timeout 30m` raises the print-mode wait above agy's 5-minute default so a real multi-file review isn't cut off, and — since stock macOS has no `timeout`/`gtimeout` and `TIMEOUT_CMD` is empty — is the effective bound on the invocation; it bounds the wait for the next response chunk, not the total runtime, so an actively-streaming review is never truncated. Unlike the old gemini invocation, no `env VAR=…` prefix is needed, so it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper at step 2 of the loop when one is present. +- `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p` — `-p`/`--single` runs a single-turn headless prompt, prints the response to stdout, and exits (the grok analog of `claude -p` / `agy -p`). `--permission-mode bypassPermissions` auto-approves every tool execution so grok runs unattended (grok's equivalent of `--dangerously-skip-permissions`); it folds "auto-approve tools" into one flag, so no separate sandbox/`env VAR=…` prefix is needed and it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper. `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` pins the reviewing model for a `grok[]` bracket (empty array → grok's own default; grok accepts the long `--model` form alongside `-p`). Like `agy -p`, `grok -p` takes the prompt as its positional argument — **not** from stdin (`grok … -p "$LOCAL_PROMPT"`); do not pipe into it. Grok has no `--print-timeout` equivalent, so the run is bounded by `TIMEOUT_CMD` (when present) and grok's own internal limits — the same background-launch + poll in Step 2 keeps it off the host's ~10-minute foreground cap. Because these flags grant the headless CLI full unattended write access to the working tree — and the Claude-Code sub-agent likewise shares this working tree — the verify step in this loop (build + tests + diff inspection by the main thread) is mandatory and non-skippable — it is the only line of defense between the reviewing agent's output and the remote branch. This applies in *both* editing modes: in review-only mode the orchestrator's own fixes are still verified before push, because the orchestrator may misread the CLI's findings or introduce its own regressions. @@ -207,7 +212,7 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro LOG_FILE="$(mktemp -t local-review-${REVIEW_AGENT}.XXXXXX.log)" ERR_FILE="${LOG_FILE}.err" DONE_FILE="${LOG_FILE}.exit" - "${TIMEOUT_CMD[@]}" {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" + ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" ``` **Keep stderr OUT of `$LOG_FILE` (`2> "$ERR_FILE"`, never `2>&1`).** Step 3 validates `$LOG_FILE` as a *strict* verdict document — it must hold nothing but `NO FINDINGS` or complete `FINDING :` blocks, and anything else is a parse failure. Every CLI writes non-verdict chatter to stderr (startup and deprecation banners, auth notices, agy/grok progress narration, a `timeout` kill message), so merging the streams would let one stray banner turn a perfectly clean review into a parse failure that blocks the merge. Same split, same reason, as `lib/ollama-review-loop.md`. @@ -227,11 +232,11 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro ```bash LOG_FILE="$(mktemp -t local-review-${REVIEW_AGENT}.XXXXXX.log)" ERR_FILE="${LOG_FILE}.err" - "${TIMEOUT_CMD[@]}" {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE" + ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE" EXIT_CODE=$? ``` - - `TIMEOUT_CMD` was already resolved during pre-flight (the array `(timeout 1800)`, `(gtimeout 1800)`, or empty). Just expand it as `"${TIMEOUT_CMD[@]}"` — an empty array expands to zero words in bash and zsh alike, becoming a direct invocation. No re-checking or commentary needed. + - `TIMEOUT_CMD` was already resolved during pre-flight (the array `(timeout 1800)`, `(gtimeout 1800)`, or empty; on stock macOS it is empty, which is a supported configuration and never a reviewer failure). Expand it exactly as `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}` — the guard is required, **not** decoration: the bare form aborts under bash 3.2 + `set -u` before the reviewer starts, which surfaces as a false `cli-error` that blocks the merge (see `~/.claude/lib/empty-array-expansion.md`). Same rule for `MODEL_FLAG`. No re-checking or commentary needed. - If `EXIT_CODE != 0` and the CLI produced no commits, set `STATUS=cli-error`, print the last 80 lines of **`$ERR_FILE`** (that is where a failing CLI writes its diagnostics now that the streams are split — fall back to `$LOG_FILE` if `$ERR_FILE` is empty), and exit the loop. Surface both paths so the user can inspect. A `124` exit (from `timeout`/`gtimeout`) or an empty log after the poll loop gave up means the review genuinely ran past 30 minutes — report it as `cli-error` with the log paths, do not record `clean`. 3. **Detect changes and apply fixes** (logic depends on `{REVIEWER_APPLIES}`): diff --git a/lib/ollama-review-loop.md b/lib/ollama-review-loop.md index 89f8f45..f187be6 100644 --- a/lib/ollama-review-loop.md +++ b/lib/ollama-review-loop.md @@ -22,12 +22,13 @@ When to use this: 3. **Resolve `{OLLAMA_MODEL}`** (see "Model resolution" below). If resolution yields no usable model, set `STATUS=skipped` and return. 4. Force review-only: set `REVIEWER_APPLIES=false` regardless of what the caller passed. If the caller passed `--reviewer-applies`, print: `--reviewer-applies has no effect on the ollama pass; Ollama is non-agentic, so the orchestrator always applies the fixes.` 5. Record `{REPO_DIR}` (`git rev-parse --show-toplevel`), `{BRANCH_NAME}` (`git branch --show-current`), `{BASE_BRANCH}`, `{BUILD_CMD}`, and `{TEST_CMD}`. Also record `{MAX_ITERATIONS}` — how many review → fix → re-review cycles this reviewer may run, resolved by the caller (the multi-reviewer loop: a per-entry `~max=` suffix on the `--review-with` token → this loop's built-in default of `3`). **Defaults to `3`**; `0` means **unlimited**, bounded by the 10-iteration safety guardrail in the Loop's step 6. Local models are the most common reason to want a small cap — `ollama~max=1` buys one review-and-fix pass without paying for re-review rounds on slow hardware. Record `{MAX_EXPLICIT}` alongside it — `true` only when the cap came from a `~max=` the user typed or saved — which step 6 uses to distinguish `capped` (a budget the user chose, clean-equivalent for the merge gate) from `guardrail` (a built-in ceiling, inconclusive). The `--review-iterations` flag never reaches this loop; `~max` is the only way to move this cap. -6. **Resolve the timeout wrapper.** macOS ships no `timeout(1)` unless coreutils is installed, so probing is required; an empty array = no wrapper (rely on Ollama's own limits). An ARRAY, not a string, for the same zsh reason as `OLLAMA_FLAGS` below: zsh does not word-split an unquoted expansion, so a two-word string (`timeout 600`) would be executed as one bogus command name and fail every invocation precisely on machines that HAVE coreutils installed. Settled logic — run it verbatim, do NOT narrate the probe or the fallback: +6. **Resolve the timeout wrapper.** Stock macOS ships **neither** `timeout(1)` (GNU coreutils) nor `gtimeout` (the Homebrew-prefixed coreutils build), so an empty `TIMEOUT_CMD` is the *common* case on a Mac, not an edge case. An empty array = no wrapper (rely on Ollama's own limits), which is a **supported configuration**: the review runs unbounded and must never be recorded as a reviewer failure. An ARRAY, not a string, for the same zsh reason as `OLLAMA_FLAGS` below: zsh does not word-split an unquoted expansion, so a two-word string (`timeout 600`) would be executed as one bogus command name and fail every invocation precisely on machines that HAVE coreutils installed. Settled logic — run it verbatim, do NOT narrate the probe or the fallback: ```bash TIMEOUT_CMD=() if command -v timeout >/dev/null 2>&1; then TIMEOUT_CMD=(timeout 600) elif command -v gtimeout >/dev/null 2>&1; then TIMEOUT_CMD=(gtimeout 600); fi ``` + **Always expand a possibly-empty array as `${ARR[@]+"${ARR[@]}"}`, never as a bare `"${ARR[@]}"`** — the guard is required, not decoration: the bare form aborts under bash 3.2 + `set -u` before the command runs, which surfaces as a false `cli-error`. Applies to `TIMEOUT_CMD` and `OLLAMA_FLAGS` below, and to any other array that can legitimately be empty. See `~/.claude/lib/empty-array-expansion.md`. 7. **Select the structured-output format.** The review asks the model for JSON so the orchestrator parses a data structure instead of scraping a free-text format. Define the schema once and pick the strongest mode the installed Ollama supports — schema-constrained outputs require Ollama ≥ 0.5.0: ```bash FINDINGS_SCHEMA='{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"file":{"type":"string"},"line":{"type":"integer"},"severity":{"type":"string","enum":["CRITICAL","IMPROVEMENT","NIT"]},"description":{"type":"string"},"fix":{"type":"string"}},"required":["file","line","severity","description","fix"]}}},"required":["findings"]}' @@ -118,16 +119,16 @@ If the diff has no logic issues worth raising, return {\"findings\": []}. --- DIFF --- $FILE_DIFF" - RESP=$(printf '%s' "$PROMPT" | "${TIMEOUT_CMD[@]}" ollama run "${OLLAMA_FLAGS[@]}" "$OLLAMA_MODEL" 2>> "$ERR_FILE") + RESP=$(printf '%s' "$PROMPT" | ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} ollama run ${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"} "$OLLAMA_MODEL" 2>> "$ERR_FILE") RC=$? printf '\n===== FILE: %s =====\n%s\n' "$F" "$RESP" >> "$LOG_FILE" ``` Three things keep the captured findings a clean, parseable data structure rather than buried in terminal noise: - **`--format "$OLLAMA_FORMAT"` (probed into `$OLLAMA_FLAGS`).** When supported, grammar-constrains the output to JSON (and, in schema mode, to the exact field names and the `severity` enum). This is the single biggest reliability win: the orchestrator parses a structure instead of scraping a free-text format that local models adhere to unreliably. On a client too old to support `--format` the flag is dropped (see the pre-flight probe) and the loop falls back to prompt-only JSON, parsed leniently. The delimiter lines (`===== FILE: =====`, the path embedded per file) let you split `$LOG_FILE` into one JSON object per reviewed file — split on the regex `^===== FILE: (.+) =====$`, capturing the path. - **`2>> "$ERR_FILE"` (not `2>&1`).** `ollama run` writes the actual model response to **stdout** but renders its progress spinner — braille frames (`⠙ ⠹ ⠼`) wrapped in ANSI cursor codes (`\e[?25l`, `\e[1G`, `\e[K`) — to **stderr**. Merging the two with `2>&1` is what fills the log with spinner garbage. Send stderr to a separate file so `$LOG_FILE` holds only model JSON; no ANSI stripping pass is then needed. - - **`"${OLLAMA_FLAGS[@]}"` (`--format`, `--hidethinking`, `--nowordwrap`).** Probed for support in pre-flight and included only when present (as a shell array, so it expands to separate words under both bash and zsh, and to nothing when empty) — passing a flag an older `ollama run` doesn't recognize makes it exit non-zero and error the whole pass. `--hidethinking` suppresses reasoning models' (e.g. `qwen3`, `deepseek-r1`) chain-of-thought, which can otherwise precede the constrained JSON even under `--format`; it is a no-op on non-thinking *models* but is absent on older ollama *versions* (~0.5–0.8), hence the probe. `--nowordwrap` stops Ollama hard-wrapping long lines to the terminal width, which would otherwise inject newlines into a long `fix` string. + - **`${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"}` (`--format`, `--hidethinking`, `--nowordwrap`).** Probed for support in pre-flight and included only when present (as a shell array, so it expands to separate words under both bash and zsh, and — in this `${ARR[@]+…}` form — to nothing when empty, without tripping `set -u` under bash 3.2; see pre-flight step 6) — passing a flag an older `ollama run` doesn't recognize makes it exit non-zero and error the whole pass. `--hidethinking` suppresses reasoning models' (e.g. `qwen3`, `deepseek-r1`) chain-of-thought, which can otherwise precede the constrained JSON even under `--format`; it is a no-op on non-thinking *models* but is absent on older ollama *versions* (~0.5–0.8), hence the probe. `--nowordwrap` stops Ollama hard-wrapping long lines to the terminal width, which would otherwise inject newlines into a long `fix` string. 4. Treat a file as a failed (zero-coverage) review when **either** `RC != 0` **or** `$RESP` is empty/whitespace-only (`[ -z "$(printf '%s' "$RESP" | tr -d '[:space:]')" ]`). The empty-but-exit-0 case is real and silent: a reasoning model can spend its whole token budget on hidden thinking (`--hidethinking`) and emit no JSON, yet `ollama run` still exits 0 — observed with `qwen3.6:35b`. Without this guard the empty section parses as "no findings" and the file is miscounted as cleanly reviewed. On either condition, append a `[ollama error reviewing $F — RC=$RC, empty=$([ -z "$(printf '%s' "$RESP" | tr -d '[:space:]')" ] && echo yes || echo no); see $ERR_FILE]` marker to the log, **increment `REVIEW_ERRORS`**, and continue to the next file (one file's failure should not abort the whole review). Coverage accounting after the loop — first recompute `REVIEWABLE=$((TOTAL_FILES - SKIPPED_EMPTY))` (the files actually sent to the model; empty-diff skips never reached it), then define a coverage gap as any reviewable file that errored or was truncated (`REVIEW_ERRORS + TRUNCATED > 0`): - - If *every reviewable* file errored (`REVIEWABLE > 0` and `REVIEW_ERRORS == REVIEWABLE`), set `STATUS=cli-error`, print the last 80 lines of `$ERR_FILE` (genuine ollama errors live there, not in `$LOG_FILE`; but in the exit-0 empty-response failure mode `$ERR_FILE` may hold only spinner noise, so also surface the per-file `[ollama error reviewing …]` markers from `$LOG_FILE`), and exit — nothing was reviewed. (Use `REVIEWABLE`, not `TOTAL_FILES`: an empty-diff skip would otherwise make `REVIEW_ERRORS == TOTAL_FILES` unreachable and misclassify a total failure as merely `incomplete`.) + - If *every reviewable* file errored (`REVIEWABLE > 0` and `REVIEW_ERRORS == REVIEWABLE`), set `STATUS=cli-error`, print the last 80 lines of `$ERR_FILE` (genuine ollama errors live there, not in `$LOG_FILE`; but in the exit-0 empty-response failure mode `$ERR_FILE` may hold only spinner noise, so also surface the per-file `[ollama error reviewing …]` markers from `$LOG_FILE`), and exit — nothing was reviewed. **Before reporting it, rule out the shell-expansion false positive**: every file erroring with an empty response and nothing but `unbound variable` in `$ERR_FILE` is the bash-3.2 empty-array symptom (`~/.claude/lib/empty-array-expansion.md`), not a reviewer failure — so print `ollama: no timeout/gtimeout on this machine — running unbounded` once and re-run the pass with the guarded expansion rather than recording `cli-error`. (Use `REVIEWABLE`, not `TOTAL_FILES`: an empty-diff skip would otherwise make `REVIEW_ERRORS == TOTAL_FILES` unreachable and misclassify a total failure as merely `incomplete`.) - JSON parse errors are accounted for after the per-file invocations (see the defensive parsing rule below). Like invocation errors and truncation, they are coverage gaps: a non-empty response that cannot be parsed is not evidence that the file was clean. - If there is any coverage gap but not a total failure (`REVIEW_ERRORS + PARSE_ERRORS + TRUNCATED > 0` and `REVIEW_ERRORS + PARSE_ERRORS < REVIEWABLE`), the diff was only **partially** reviewed. Still process the findings from the parts that were reviewed (apply their fixes in step 3), but the pass **must not report `clean`** — at the point step 3 would set `STATUS=clean`, set `STATUS=incomplete` instead (see step 3). `incomplete` is treated as inconclusive by the multi-reviewer aggregate (not eligible to merge), because part of the change was never reviewed. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 1402ec9..8b479c5 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -8,6 +8,15 @@ 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'); +// The loop partials whose invocations carry arrays that can legitimately be empty +// (TIMEOUT_CMD when no timeout/gtimeout is installed, MODEL_FLAG when no model is +// pinned, OLLAMA_FLAGS on an ollama too old for the optional flags). +const LOOPS_WITH_OPTIONAL_ARRAYS = [ + 'local-agent-review-loop.md', + 'ollama-review-loop.md', + 'enhance-loop.md', +]; + describe('review-loop parse contracts', () => { it('requires structured local-agent verdicts without weakening Codex handling', () => { const body = readLib('local-agent-review-loop.md'); @@ -241,4 +250,56 @@ describe('review-loop parse contracts', () => { assert.match(pr, /\s+exit 1/); assert.match(pr, /Never merge on `dirty`\/`inconclusive`, never merge while the branch has unpushed commits/); }); + + it('guards every possibly-empty array expansion against bash 3.2 + set -u', () => { + // Stock macOS has neither `timeout` nor `gtimeout`, so TIMEOUT_CMD is legitimately + // empty there — and under /bin/bash 3.2 a bare "${ARR[@]}" on an empty array is an + // UNSET expansion that aborts with `unbound variable` before the reviewer ever runs. + // Every file then comes back RC=1 with empty output, the loop counts them all as + // REVIEW_ERRORS, and the pass resolves to `cli-error` — a hard error `~opt` does not + // excuse — blocking the merge on a PR no reviewer looked at. Only the + // ${ARR[@]+"${ARR[@]}"} form is safe on bash 3.2, bash 4/5, and zsh alike. + // Scan by PATTERN, not by a hardcoded array-name list, so a newly introduced + // optional-argument array is covered the day it lands. + for (const name of LOOPS_WITH_OPTIONAL_ARRAYS) { + const body = readLib(name); + // A bare "${ARR[@]}" — the lookbehind lets through the guarded ${ARR[@]+"${ARR[@]}"}, + // and the negative lookahead exempts the literal name `ARR`, which is the prose + // metavariable used to *state* the rule, never a real array. + const bare = body.match(/(? { + // An absent timeout binary is an environment condition, not a reviewer failure. + // The explanation lives in ONE partial (the lib/gh-host.md convention) — five + // near-identical copies is how the rule drifted mid-PR the first time. Assert the + // partial carries the contract and that each loop links to it; do NOT assert on + // the loops' own prose wording, which is theirs to copy-edit. + const partial = readLib('empty-array-expansion.md'); + assert.match(partial, /\$\{ARR\[@\]\+"\$\{ARR\[@\]\}"\}\s+# correct/); + assert.match(partial, /Stock macOS ships \*\*neither\*\* `timeout\(1\)`/); + assert.match(partial, /supported configuration\*\*/); + assert.match(partial, /unbound variable/); + + for (const name of LOOPS_WITH_OPTIONAL_ARRAYS) { + assert.match( + readLib(name), + /lib\/empty-array-expansion\.md/, + `${name} must point at the shared empty-array-expansion partial rather than restating it`, + ); + } + }); }); diff --git a/uninstall.sh b/uninstall.sh index 88f9857..94ac6dd 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -33,7 +33,7 @@ OLD_COMMANDS=(cam good makegoals makegood optimize-md) # NOTE: keep in sync with install.sh LIBS — see comment there. LIBS=( ci-flake-handling code-review-checklist copilot-review-loop - enhance-loop epic-children + empty-array-expansion enhance-loop epic-children finding-disposition fix-regression-guard gh-host github-reviewer-loop graphql-escaping local-agent-review-loop multi-reviewer-loop ollama-review-loop From 57a5531816455ae8c0477ea6232a3ee97f38a479 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 23:29:16 +0000 Subject: [PATCH 12/18] docs: say that --issues picks a destination, not a run mode, in simplify and config (#135) --- .changelogs/NEXT.md | 3 +++ commands/do/config.md | 4 ++-- commands/do/simplify.md | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.changelogs/NEXT.md b/.changelogs/NEXT.md index 4a54b84..be0a77e 100644 --- a/.changelogs/NEXT.md +++ b/.changelogs/NEXT.md @@ -2,3 +2,6 @@ ## 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. + +## Docs +- **[issue-135] `--issues` no longer reads like it stops the run** — `/do:simplify`'s flag list and `/do:config`'s issue-mode default now say outright that `--issues` selects where findings are *recorded* and does not suppress remediation, and point at `--scan-only --issues` for the audit-and-file run people actually reach for. Previously the name suggested a run mode, and users were surprised when `/do:simplify --issues` went on to remediate, PR, and merge. diff --git a/commands/do/config.md b/commands/do/config.md index 8659dd7..c4c0a0b 100644 --- a/commands/do/config.md +++ b/commands/do/config.md @@ -15,7 +15,7 @@ argument-hint: "[--show] [--project] [--review-with ] [--review-models []` bracket. It applies to the model-taking reviewers `codex`, `claude`, `agy`, `grok`, and `ollama` (the GitHub-side `copilot` and `@` take no model). An explicit `[]` bracket always wins over this saved `review-models` default — whether the bracket was typed on the run or carried by a saved `review-with` default (a bracket in `review-with` is source-agnostic in the resolver) — and a project-scoped `review-models` entry overrides the global one for that agent. Set one agent at a time without disturbing the others; `--review-models codex=` (empty value) clears just that agent's entry. -The same store also holds an **issue-mode default**: `/do:config --issues` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to filing/working tracker issues instead of `PLAN.md`. Pass `--no-issues` on a single run to fall back to PLAN.md mode for that run, and `--issues-label ` to save the scoping label. +The same store also holds an **issue-mode default**: `/do:config --issues` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to filing/working tracker issues instead of `PLAN.md`. It selects a **destination**, not a run mode — a saved `issues=true` does not suppress remediation, so the review commands still fix, PR, and merge as normal (pair with `--scan-only` for an audit-and-file run). Pass `--no-issues` on a single run to fall back to PLAN.md mode for that run, and `--issues-label ` to save the scoping label. It also holds a **self-only issue gate for `/do:next`**: `/do:config --self` makes `/do:next --issues` claim only issues filed by the authenticated account (`@me`) and never consider issues opened by anyone else — a security boundary for shared/multi-contributor trackers. Pass `--no-self` on a single run to fall back to claiming any open issue. Only `/do:next` reads this key. @@ -48,7 +48,7 @@ At read time, **per-project overrides global, key by key** (see `lib/review-conf - `--reviewer-applies` → key `reviewer-applies`, value `true`. Its explicit opposite `--no-reviewer-applies` → key `reviewer-applies`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `reviewer-applies=true` back off. (`--unset reviewer-applies` removes the key entirely and falls back to the lower-precedence value.) `--reviewer-applies` and `--no-reviewer-applies` are mutually exclusive. - `--review-stop-on-findings` / `--review-stop-on-clean` → key `review-stop-mode`, value `"on-findings"` / `"on-clean"`. The explicit default `--review-stop-all` → key `review-stop-mode`, value `"all"` — store this when a **project** default needs to override an inherited global stop-mode back to "run every reviewer". These three are mutually exclusive — if more than one is present, abort with `--review-stop-on-findings, --review-stop-on-clean, and --review-stop-all are mutually exclusive`. - `--review-mode ` → key `review-mode`. Must be one of `series`/`parallel`; else abort with `--review-mode must be one of series, parallel (got: {value}).` Store the string verbatim. Selects how the multi-reviewer loop dispatches reviewers (`series` — one-at-a-time, each sees the prior's fixes, the built-in default; `parallel` — reviews run concurrently then the union is applied once). Read by `/do:pr`, `/do:review`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, and `/do:release` (`/do:rpr` ignores it). - - `--issues` → key `issues`, value `true`. Its explicit opposite `--no-issues` → key `issues`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `issues=true` back to PLAN.md mode. (`--unset issues` removes the key entirely and falls back to the lower-precedence value.) `--issues` and `--no-issues` are mutually exclusive. A saved `issues=true` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to issue mode; an explicit `--issues`/`--no-issues` on a run still wins. + - `--issues` → key `issues`, value `true`. Its explicit opposite `--no-issues` → key `issues`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `issues=true` back to PLAN.md mode. (`--unset issues` removes the key entirely and falls back to the lower-precedence value.) `--issues` and `--no-issues` are mutually exclusive. A saved `issues=true` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to issue mode — which changes only where findings are recorded, never whether the run remediates; an explicit `--issues`/`--no-issues` on a run still wins. - `--issues-label ` → key `issues-label`. Store the string verbatim — the label that scopes plan-tracking issues (built-in default `plan`). Only meaningful once issue mode is on. - `--self` → key `self`, value `true`. Its explicit opposite `--no-self` → key `self`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `self=true` back to claiming any open issue. (`--unset self` removes the key entirely and falls back to the lower-precedence value.) `--self` and `--no-self` are mutually exclusive. A saved `self=true` makes `/do:next --issues` claim only issues filed by the running account (`@me`) — auto-pick filters out others and an explicit `#` for someone else's issue is refused — as a security boundary; an explicit `--self`/`--no-self` on a run still wins. Only `/do:next` reads this key, and only in issue mode (PLAN.md items have no author). - `--merge` → key `merge`, value `true`. Its explicit opposite `--no-merge` → key `merge`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `merge=true` back to leave-open. (`--unset merge` removes the key entirely and falls back to the lower-precedence value.) `--merge` and `--no-merge` are mutually exclusive. The shorthand `--merge=` sets `merge=true` **and** `merge-method=` in one token (the `` is validated against `squash`/`rebase`/`merge` with the same abort as `--merge-method` below). If both `--merge=` and `--merge-method ` are given with **different** methods, abort with `--merge= and --merge-method specify conflicting methods ({first} vs {second})`; identical methods are accepted. A saved `merge=true` makes `/do:pr` auto-merge once reviews and CI are solid; an explicit `--merge`/`--no-merge` on a run still wins. Only `/do:pr` reads this key. diff --git a/commands/do/simplify.md b/commands/do/simplify.md index b911a00..9b412ed 100644 --- a/commands/do/simplify.md +++ b/commands/do/simplify.md @@ -24,7 +24,8 @@ Argument handling: - Pass `$ARGUMENTS` through to `do:better` verbatim. It parses every flag itself, including the saved `/do:config` defaults, so there is nothing to extract or re-validate here. - `--simplify-only` / `--refactor-only` in `$ARGUMENTS` is redundant but harmless — do not error on it. - `--strict` / `--nuclear` is implied (`SIMPLIFY_ONLY=true` sets `STRICT_MODE=true`); passing it explicitly changes nothing. -- Every other `do:better` flag works as documented: `--scan-only` stops after the narrowed plan, `--interactive` prompts at each gate, `--no-merge` stops after PR creation, `--issues`/`--issues-label` file deferred findings as tracker issues, and the review flags (`--review-with`, `--review-mode`, `--review-iterations`, `--review-stop-on-*`, `--reviewer-applies`) drive the Phase 6 loop. +- Every other `do:better` flag works as documented: `--scan-only` stops after the narrowed plan, `--interactive` prompts at each gate, `--no-merge` stops after PR creation, and the review flags (`--review-with`, `--review-mode`, `--review-iterations`, `--review-stop-on-*`, `--reviewer-applies`) drive the Phase 6 loop. +- `--issues` / `--issues-label ` select **where** deferred findings are recorded — tracker issues instead of `PLAN.md` lines. They do **not** change what the run does: `/do:simplify --issues` still remediates in a worktree, opens per-category PRs, runs CI and the review loop, and merges. To audit and file the work without touching your code, combine with `--scan-only` — a `--scan-only` run in issue mode remediates nothing, so **every** surviving finding is filed as a labelled issue, and those issues are the run's entire output. ## Notes From f6e59ddc42331575411599b38120e47faec3c72a Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 23:32:53 +0000 Subject: [PATCH 13/18] address review (antigravity): gate the merge claim on --review-with and scope it to the pipeline commands --- .changelogs/NEXT.md | 2 +- commands/do/config.md | 4 ++-- commands/do/simplify.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changelogs/NEXT.md b/.changelogs/NEXT.md index be0a77e..852b4a8 100644 --- a/.changelogs/NEXT.md +++ b/.changelogs/NEXT.md @@ -4,4 +4,4 @@ - **[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. ## Docs -- **[issue-135] `--issues` no longer reads like it stops the run** — `/do:simplify`'s flag list and `/do:config`'s issue-mode default now say outright that `--issues` selects where findings are *recorded* and does not suppress remediation, and point at `--scan-only --issues` for the audit-and-file run people actually reach for. Previously the name suggested a run mode, and users were surprised when `/do:simplify --issues` went on to remediate, PR, and merge. +- **[issue-135] `--issues` no longer reads like it stops the run** — `/do:simplify`'s flag list and `/do:config`'s issue-mode default now say outright that `--issues` selects where findings are *recorded* and does not suppress remediation, and point at `--scan-only --issues` for the audit-and-file run people actually reach for. Previously the name suggested a run mode, and users were surprised when `/do:simplify --issues` went on to remediate and open PRs. diff --git a/commands/do/config.md b/commands/do/config.md index c4c0a0b..6a61ccd 100644 --- a/commands/do/config.md +++ b/commands/do/config.md @@ -15,7 +15,7 @@ argument-hint: "[--show] [--project] [--review-with ] [--review-models []` bracket. It applies to the model-taking reviewers `codex`, `claude`, `agy`, `grok`, and `ollama` (the GitHub-side `copilot` and `@` take no model). An explicit `[]` bracket always wins over this saved `review-models` default — whether the bracket was typed on the run or carried by a saved `review-with` default (a bracket in `review-with` is source-agnostic in the resolver) — and a project-scoped `review-models` entry overrides the global one for that agent. Set one agent at a time without disturbing the others; `--review-models codex=` (empty value) clears just that agent's entry. -The same store also holds an **issue-mode default**: `/do:config --issues` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to filing/working tracker issues instead of `PLAN.md`. It selects a **destination**, not a run mode — a saved `issues=true` does not suppress remediation, so the review commands still fix, PR, and merge as normal (pair with `--scan-only` for an audit-and-file run). Pass `--no-issues` on a single run to fall back to PLAN.md mode for that run, and `--issues-label ` to save the scoping label. +The same store also holds an **issue-mode default**: `/do:config --issues` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to filing/working tracker issues instead of `PLAN.md`. It selects a **destination**, not a run mode — a saved `issues=true` does not suppress remediation, so the pipeline commands (`/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`) still remediate and PR as normal, merging on the same `--review-with` gate as ever (pair with `--scan-only` for an audit-and-file run). Pass `--no-issues` on a single run to fall back to PLAN.md mode for that run, and `--issues-label ` to save the scoping label. It also holds a **self-only issue gate for `/do:next`**: `/do:config --self` makes `/do:next --issues` claim only issues filed by the authenticated account (`@me`) and never consider issues opened by anyone else — a security boundary for shared/multi-contributor trackers. Pass `--no-self` on a single run to fall back to claiming any open issue. Only `/do:next` reads this key. @@ -48,7 +48,7 @@ At read time, **per-project overrides global, key by key** (see `lib/review-conf - `--reviewer-applies` → key `reviewer-applies`, value `true`. Its explicit opposite `--no-reviewer-applies` → key `reviewer-applies`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `reviewer-applies=true` back off. (`--unset reviewer-applies` removes the key entirely and falls back to the lower-precedence value.) `--reviewer-applies` and `--no-reviewer-applies` are mutually exclusive. - `--review-stop-on-findings` / `--review-stop-on-clean` → key `review-stop-mode`, value `"on-findings"` / `"on-clean"`. The explicit default `--review-stop-all` → key `review-stop-mode`, value `"all"` — store this when a **project** default needs to override an inherited global stop-mode back to "run every reviewer". These three are mutually exclusive — if more than one is present, abort with `--review-stop-on-findings, --review-stop-on-clean, and --review-stop-all are mutually exclusive`. - `--review-mode ` → key `review-mode`. Must be one of `series`/`parallel`; else abort with `--review-mode must be one of series, parallel (got: {value}).` Store the string verbatim. Selects how the multi-reviewer loop dispatches reviewers (`series` — one-at-a-time, each sees the prior's fixes, the built-in default; `parallel` — reviews run concurrently then the union is applied once). Read by `/do:pr`, `/do:review`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, and `/do:release` (`/do:rpr` ignores it). - - `--issues` → key `issues`, value `true`. Its explicit opposite `--no-issues` → key `issues`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `issues=true` back to PLAN.md mode. (`--unset issues` removes the key entirely and falls back to the lower-precedence value.) `--issues` and `--no-issues` are mutually exclusive. A saved `issues=true` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to issue mode — which changes only where findings are recorded, never whether the run remediates; an explicit `--issues`/`--no-issues` on a run still wins. + - `--issues` → key `issues`, value `true`. Its explicit opposite `--no-issues` → key `issues`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `issues=true` back to PLAN.md mode. (`--unset issues` removes the key entirely and falls back to the lower-precedence value.) `--issues` and `--no-issues` are mutually exclusive. A saved `issues=true` makes every command that accepts `--issues` (`/do:next`, `/do:replan`, `/do:better`, `/do:better-swift`, `/do:simplify`, `/do:depfree`, `/do:review`, `/do:rpr`) default to issue mode — which changes only where deferred findings are recorded, without altering remediation behavior; an explicit `--issues`/`--no-issues` on a run still wins. - `--issues-label ` → key `issues-label`. Store the string verbatim — the label that scopes plan-tracking issues (built-in default `plan`). Only meaningful once issue mode is on. - `--self` → key `self`, value `true`. Its explicit opposite `--no-self` → key `self`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `self=true` back to claiming any open issue. (`--unset self` removes the key entirely and falls back to the lower-precedence value.) `--self` and `--no-self` are mutually exclusive. A saved `self=true` makes `/do:next --issues` claim only issues filed by the running account (`@me`) — auto-pick filters out others and an explicit `#` for someone else's issue is refused — as a security boundary; an explicit `--self`/`--no-self` on a run still wins. Only `/do:next` reads this key, and only in issue mode (PLAN.md items have no author). - `--merge` → key `merge`, value `true`. Its explicit opposite `--no-merge` → key `merge`, value `false` — store this (rather than `--unset`) when a **project** default needs to override an inherited global `merge=true` back to leave-open. (`--unset merge` removes the key entirely and falls back to the lower-precedence value.) `--merge` and `--no-merge` are mutually exclusive. The shorthand `--merge=` sets `merge=true` **and** `merge-method=` in one token (the `` is validated against `squash`/`rebase`/`merge` with the same abort as `--merge-method` below). If both `--merge=` and `--merge-method ` are given with **different** methods, abort with `--merge= and --merge-method specify conflicting methods ({first} vs {second})`; identical methods are accepted. A saved `merge=true` makes `/do:pr` auto-merge once reviews and CI are solid; an explicit `--merge`/`--no-merge` on a run still wins. Only `/do:pr` reads this key. diff --git a/commands/do/simplify.md b/commands/do/simplify.md index 9b412ed..d4246b7 100644 --- a/commands/do/simplify.md +++ b/commands/do/simplify.md @@ -25,7 +25,7 @@ Argument handling: - `--simplify-only` / `--refactor-only` in `$ARGUMENTS` is redundant but harmless — do not error on it. - `--strict` / `--nuclear` is implied (`SIMPLIFY_ONLY=true` sets `STRICT_MODE=true`); passing it explicitly changes nothing. - Every other `do:better` flag works as documented: `--scan-only` stops after the narrowed plan, `--interactive` prompts at each gate, `--no-merge` stops after PR creation, and the review flags (`--review-with`, `--review-mode`, `--review-iterations`, `--review-stop-on-*`, `--reviewer-applies`) drive the Phase 6 loop. -- `--issues` / `--issues-label ` select **where** deferred findings are recorded — tracker issues instead of `PLAN.md` lines. They do **not** change what the run does: `/do:simplify --issues` still remediates in a worktree, opens per-category PRs, runs CI and the review loop, and merges. To audit and file the work without touching your code, combine with `--scan-only` — a `--scan-only` run in issue mode remediates nothing, so **every** surviving finding is filed as a labelled issue, and those issues are the run's entire output. +- `--issues` / `--issues-label ` select **where** deferred findings are recorded — tracker issues instead of `PLAN.md` lines. They do **not** change what the run does: `/do:simplify --issues` still remediates in a worktree, opens per-category PRs, and runs CI — and, when `--review-with` supplies a reviewer, runs the review loop and merges. To audit and file the work without touching your code, combine with `--scan-only` — a `--scan-only` run in issue mode remediates nothing, so **every** surviving finding is filed as a labeled issue, and those issues are the run's entire output. ## Notes From 65db11f93a0861ab1f4ce6dba7151a3925ef711a Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 16:55:58 -0700 Subject: [PATCH 14/18] chore: release v3.27.1 --- .changelogs/NEXT.md | 7 ------- .changelogs/v3.27.1.md | 22 ++++++++++++++++++++++ package.json | 2 +- 3 files changed, 23 insertions(+), 8 deletions(-) delete mode 100644 .changelogs/NEXT.md create mode 100644 .changelogs/v3.27.1.md diff --git a/.changelogs/NEXT.md b/.changelogs/NEXT.md deleted file mode 100644 index 852b4a8..0000000 --- a/.changelogs/NEXT.md +++ /dev/null @@ -1,7 +0,0 @@ -# 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. - -## Docs -- **[issue-135] `--issues` no longer reads like it stops the run** — `/do:simplify`'s flag list and `/do:config`'s issue-mode default now say outright that `--issues` selects where findings are *recorded* and does not suppress remediation, and point at `--scan-only --issues` for the audit-and-file run people actually reach for. Previously the name suggested a run mode, and users were surprised when `/do:simplify --issues` went on to remediate and open PRs. diff --git a/.changelogs/v3.27.1.md b/.changelogs/v3.27.1.md new file mode 100644 index 0000000..2d59afa --- /dev/null +++ b/.changelogs/v3.27.1.md @@ -0,0 +1,22 @@ +# Release v3.27.1 + +Released: 2026-08-12 + +## Highlights + +- **Review fixes can no longer be silently left on your machine.** The review loop now pushes its own fix commits, `/do:pr` pushes anything still outstanding before opening a PR, and it refuses to merge while your branch is ahead of the remote — naming the commits that would have been dropped. +- **`--issues` reads as what it is: a destination, not a run mode.** `/do:simplify` and `/do:config` now say outright that the flag selects where findings are *recorded* and does not suppress remediation, and point at `--scan-only --issues` for the audit-and-file run people were actually reaching for. +- **Reviewer status reporting is more honest under bash 3.2** — an empty array expansion can no longer masquerade as a reviewer CLI error. + +## Fixed +- **[issue-134] Review fixes are asserted pushed before a PR is opened or merged.** If a reviewer's fixes were committed but never pushed, every reviewer would report clean and CI would pass against the older pushed code while the fixes stayed local. The review loop now pushes them itself, `/do:pr` pushes anything outstanding before opening the PR, and the merge gate fails closed when the local branch is ahead of its upstream, listing the commits at risk. +- **A conflicted retry rebase is aborted instead of stranding the branch**, and the push target is derived from the tracking ref (`@{u}`) and config rather than assuming the local branch name matches the remote. +- **Possibly-empty array expansions are guarded** so bash 3.2 can't report a spurious reviewer `cli-error`. (#139) + +## Docs +- **[issue-135] `--issues` no longer reads like it stops the run.** `/do:simplify`'s flag list and `/do:config`'s issue-mode default now state that `--issues` selects where findings are recorded and does not suppress remediation, and reference `--scan-only --issues` for the audit-and-file workflow. (#135, #136) +- **The merge claim is gated on `--review-with` and scoped to the pipeline commands**, so commands that never run a reviewer no longer imply one gates their merge. + +## Full Changelog + +**Full Diff**: https://github.com/atomantic/slashdo/compare/v3.27.0...v3.27.1 diff --git a/package.json b/package.json index 245943f..547cb55 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "slash-do", - "version": "3.27.0", + "version": "3.27.1", "description": "Curated slash commands for AI coding assistants — Claude Code, OpenCode, Antigravity CLI, Codex, and Grok Build", "author": "Adam Eivy ", "license": "MIT", From 18b89017d4922acf2684f9936386e4e48bdd390f Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 17:07:32 -0700 Subject: [PATCH 15/18] address review (claude): commit gate fixes, derive do:pr's push target from config, and file issues before the scan-only exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-PR assertion compares against the upstream ref, so it only ever sees committed work — but /do:pr's Local Code Review gate never told anyone to commit its fixes (unlike /do:release's, which does). A gate fix left in the working tree was therefore invisible to the guard and the PR opened from the pre-fix tree. The same bullet prescribed 'git push origin {current_branch}', which hardcodes the local branch name as the destination — the exact property multi-reviewer-loop.md calls out as #134's failure reintroduced inside its own guard. Derive remote and ref from branch..remote/.merge, matching that file. The scan-only issue-filing instruction sat in the paragraph AFTER 'STOP HERE ... and exit', so an orchestrator following the gate literally exited before reaching it and '--scan-only --issues' filed nothing. Fold it into the gate sentence. --- commands/do/better-swift.md | 4 ++-- commands/do/better.md | 4 ++-- commands/do/depfree.md | 4 ++-- commands/do/pr.md | 11 ++++++++-- test/review-loop-contract.test.js | 35 +++++++++++++++++++++++-------- 5 files changed, 41 insertions(+), 17 deletions(-) diff --git a/commands/do/better-swift.md b/commands/do/better-swift.md index 88f0f11..fee77e7 100644 --- a/commands/do/better-swift.md +++ b/commands/do/better-swift.md @@ -621,9 +621,9 @@ For each file touched by multiple categories, document why it was assigned to on | TOTAL | ... | ... | ... | ... | ... | ``` -**GATE: If `--scan-only` was passed, STOP HERE.** Print the summary and exit. +**GATE: If `--scan-only` was passed, STOP HERE** — but not before doing the one thing a scan-only run in issue mode exists to do: **when `ISSUE_MODE` is also true, file every surviving finding as an issue first**, then print the summary and exit. (When `ISSUE_MODE` is false, just print the summary and exit.) -**When `ISSUE_MODE` is also true, file every surviving finding as an issue before exiting** — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. +**Filing every surviving finding** means all of them — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. ## Phase 3: Worktree Remediation diff --git a/commands/do/better.md b/commands/do/better.md index 6b00609..5975a72 100644 --- a/commands/do/better.md +++ b/commands/do/better.md @@ -491,9 +491,9 @@ When `SIMPLIFY_ONLY=true`, emit only the [`SIMPLIFY_CATEGORIES`](#the-category-s Omit the **UX** row when `HAS_UI=false`, the **Structural** row when `STRICT_MODE=false`, and the **Cognitive Load** row when `SIMPLIFY_ONLY=false`. When `SIMPLIFY_ONLY=true`, keep only the [`SIMPLIFY_CATEGORIES`](#the-category-set) rows. -**GATE: If `--scan-only` was passed, STOP HERE.** Print the summary and exit. +**GATE: If `--scan-only` was passed, STOP HERE** — but not before doing the one thing a scan-only run in issue mode exists to do: **when `ISSUE_MODE` is also true, file every surviving finding as an issue first**, then print the summary and exit. (When `ISSUE_MODE` is false, just print the summary and exit.) -**When `ISSUE_MODE` is also true, file every surviving finding as an issue before exiting** — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. +**Filing every surviving finding** means all of them — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. ## Phase 3: Worktree Remediation diff --git a/commands/do/depfree.md b/commands/do/depfree.md index 82fd5c9..bc567b3 100644 --- a/commands/do/depfree.md +++ b/commands/do/depfree.md @@ -379,9 +379,9 @@ For each dependency to remove: | Total | ... | | ``` -**GATE: If `--scan-only` was passed, STOP HERE.** Print the summary and exit. +**GATE: If `--scan-only` was passed, STOP HERE** — but not before doing the one thing a scan-only run in issue mode exists to do: **when `ISSUE_MODE` is also true, file every surviving finding as an issue first**, then print the summary and exit. (When `ISSUE_MODE` is false, just print the summary and exit.) -**When `ISSUE_MODE` is also true, file every surviving finding as an issue before exiting** — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. +**Filing every surviving finding** means all of them — not just the ones the disposition rules would defer. A scan-only run remediates nothing, so "deferred" covers the whole set; the filed issues ARE the run's output. Apply the same labels, dedup-against-`EXISTING_ISSUES`, and title/body rules the disposition partial specifies, and report the created and reused `#`s in the summary. Do not open a worktree or write any code. **GATE: If no removable dependencies were found, print "All dependencies are justified" and exit.** diff --git a/commands/do/pr.md b/commands/do/pr.md index 8cc37fb..35ac382 100644 --- a/commands/do/pr.md +++ b/commands/do/pr.md @@ -103,7 +103,7 @@ This review catches bugs that Copilot misses — incomplete pattern copying is t c. For each finding, quote the specific code line and explain why it's a problem 4. After reviewing all files, verify: does the code actually deliver what the commits claim? 5. Print a review summary table (see do:review for format) -6. Fix any issues, run tests, and verify tests cover the changed code paths +6. Fix any issues, run tests, verify tests cover the changed code paths, then **commit and push those fixes** — leaving them uncommitted in the working tree is invisible to the "Open the PR" assertion below, which compares against the upstream ref and so only ever sees *committed* work 7. Only after printing the review summary may you proceed to "Pre-PR Local Reviews" If the diff touches more than 15 files, delegate later batches to a subagent to keep context clean. @@ -141,7 +141,14 @@ 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. +- **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 — deriving the destination from the branch's upstream config exactly as `lib/multi-reviewer-loop.md` step 5 does, in one shell so the variables survive: + + ```bash + BR="$(git branch --show-current)" + git push "$(git config --get "branch.$BR.remote")" "HEAD:$(git config --get "branch.$BR.merge")" + ``` + + — never a bare `git push`, which under `push.default=matching` fans out to every same-named local branch, and never `git push origin {current_branch}`, which hardcodes the *local* branch name as the destination: on a branch whose upstream is named differently (or lives on another remote) that pushes a spurious remote branch, leaves the real PR head stale, and `@{u}..HEAD` is still non-empty afterward while the push itself "succeeded". Retry once after `git pull --rebase --autostash` on a non-fast-forward, 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) diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 8b479c5..6ddc712 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -148,14 +148,19 @@ describe('review-loop parse contracts', () => { // 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', - ); + // Structural, not verbatim: match EVERY " here" exemption wherever it is + // worded and require each one to qualify itself, rather than blocklisting the + // handful of phrasings we happened to think of. A reworded exemption + // ("Optional passes do not count here.") is caught by the same rule. + const exemptions = [...wrapper.matchAll(/(?:ignored|excluded|excused|do not count)\s+here/g)]; + assert.ok(exemptions.length > 0, 'the aggregate rules must state an ~opt exemption somewhere'); + for (const m of exemptions) { + assert.match( + wrapper.slice(m.index, m.index + 200), + /push-failed/, + `an ~opt exemption at index ${m.index} does not name the statuses it covers — unqualified, it reads as license to merge a pass whose fixes never reached the remote`, + ); + } 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 @@ -230,8 +235,20 @@ describe('review-loop parse contracts', () => { 'a conflicted retry must abort the rebase AND still exit non-zero so the pass records push-failed', ); + // do:pr's pre-PR push must derive its destination the SAME way, from the branch's + // upstream config — `git push origin {current_branch}` hardcodes the local branch + // name as the destination, which is the very property multi-reviewer-loop.md calls + // "#134's failure, reintroduced by the guard meant to prevent it": on a branch whose + // upstream is named differently the push succeeds against a spurious ref, @{u}..HEAD + // stays non-empty, and the run opens the PR anyway. const pr = readCommand('pr.md'); - assert.match(pr, /`git push origin \{current_branch\}` — an explicit refspec, never a bare `git push`/); + assert.match(pr, /git push "\$\(git config --get "branch\.\$BR\.remote"\)" "HEAD:\$\(git config --get "branch\.\$BR\.merge"\)"/); + assert.match(pr, /never a bare `git push`/); + assert.match(pr, /never `git push origin \{current_branch\}`/); + // The gate that feeds this assertion must actually COMMIT its fixes: the check + // compares against the upstream ref, so a fix left uncommitted in the working tree + // is invisible to it and the PR opens from the pre-fix tree regardless. + assert.match(pr, /commit and push those fixes\*\* — leaving them uncommitted/); // 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\*\*/); From 8e1b68ac5c2065198f797164c66f802a2f9f450b Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 17:14:03 -0700 Subject: [PATCH 16/18] address review (claude): restore the local-upstream guard I dropped, and scope do:pr's -u push to the never-published case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My previous fix copied multi-reviewer-loop.md's config-derived push but left behind its `[ "$PUSH_REMOTE" = "." ]` guard. On a branch whose upstream is a local ref (branch..remote=".", what `git branch --set-upstream-to=main` produces) @{u} resolves, so the no-upstream carve-out never fires and the push became `git push . HEAD:refs/heads/main` — silently fast-forwarding the local default branch onto the feature branch, exiting 0, and emptying @{u}..HEAD. The guard would then report the branch had reached the remote and open a PR for a branch never pushed anywhere. Reproduced in a scratch repo both before and after. Also: `git push -u origin ` rewrites branch..remote/.merge, so running it unconditionally re-points an existing upstream at origin/ and defeats the config-derived guard at its source. Scope it to the never-published case. Mirror the lib's `git rebase --abort` on a conflicted retry, since /do:next and /do:pr-better invoke /do:pr programmatically and would inherit the mid-rebase tree. Name the push form the review gate should use, rather than leaving the orchestrator to pick one of the two forms this file forbids. Tests: broaden the ~opt exemption scan to match by verb stem with a window on both sides, and add coverage for the local-upstream guard and the scan-only gate placement (the latter had none, and its regression reads fine in isolation). --- commands/do/pr.md | 22 +++++++--- test/review-loop-contract.test.js | 67 ++++++++++++++++++++++++++----- 2 files changed, 75 insertions(+), 14 deletions(-) diff --git a/commands/do/pr.md b/commands/do/pr.md index 35ac382..f70223b 100644 --- a/commands/do/pr.md +++ b/commands/do/pr.md @@ -87,7 +87,9 @@ Print: `PR flow: {current_branch} → {default_branch}` - `git rebase {default_branch}` to replay this branch's commits on top of the now-current default branch. - If the rebase hits conflicts, **abort** (`git rebase --abort`) and stop — print the conflicting files and ask the user to resolve them, rather than guessing at a merge. Do not proceed to review against a half-rebased tree. - After a clean rebase the branch's merge-base with the refreshed local `{default_branch}` is current, so `git diff {default_branch}...HEAD` shows only this branch's own changes. -- Push the branch to remote: `git push -u origin {current_branch}` (use `--force-with-lease` if the rebase above rewrote already-pushed history; never a bare `--force`) +- Push the branch to remote (use `--force-with-lease` on any of these if the rebase above rewrote already-pushed history; never a bare `--force`). **Which form depends on whether the branch already tracks a remote** — `-u` *rewrites* `branch..remote`/`.merge`, so using it unconditionally would re-point an existing upstream at `origin/{current_branch}` and defeat the config-derived guard under "Open the PR" at its source: + - **No upstream yet** (`git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails — the branch was never published): `git push -u origin {current_branch}`, which publishes it and sets the tracking config for the first time. + - **An upstream already exists**: push to the ref that upstream names, deriving it from config exactly as "Open the PR" below does — never `-u`, and never a destination built from the local branch name. A branch whose upstream is `upstream/feature-x` or `origin/pr-123-head` must keep pointing there. ## Local Code Review (REQUIRED GATE) @@ -103,7 +105,7 @@ This review catches bugs that Copilot misses — incomplete pattern copying is t c. For each finding, quote the specific code line and explain why it's a problem 4. After reviewing all files, verify: does the code actually deliver what the commits claim? 5. Print a review summary table (see do:review for format) -6. Fix any issues, run tests, verify tests cover the changed code paths, then **commit and push those fixes** — leaving them uncommitted in the working tree is invisible to the "Open the PR" assertion below, which compares against the upstream ref and so only ever sees *committed* work +6. Fix any issues, run tests, verify tests cover the changed code paths, then **commit and push those fixes** — using the upstream-derived push described under "Open the PR" below, never a bare `git push` and never a destination built from the local branch name. Leaving the fixes uncommitted is invisible to that section's assertion, which compares against the upstream ref and so only ever sees *committed* work 7. Only after printing the review summary may you proceed to "Pre-PR Local Reviews" If the diff touches more than 15 files, delegate later batches to a subagent to keep context clean. @@ -141,14 +143,24 @@ 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 — deriving the destination from the branch's upstream config exactly as `lib/multi-reviewer-loop.md` step 5 does, in one shell so the variables survive: +- **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 — deriving the destination from the branch's upstream config exactly as `lib/multi-reviewer-loop.md` step 5 does, as **one block** so the variables survive (they do not persist across separate Bash calls): ```bash BR="$(git branch --show-current)" - git push "$(git config --get "branch.$BR.remote")" "HEAD:$(git config --get "branch.$BR.merge")" + 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 + # Upstream is a LOCAL branch (branch..remote=".", what `git branch + # --set-upstream-to=main` produces) or absent — there is no remote to push to. + echo "REFUSING TO CREATE THE PR — '$BR' tracks a local ref, not a remote; publish it first (git push -u origin $BR)" + exit 1 + fi + git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH" ``` - — never a bare `git push`, which under `push.default=matching` fans out to every same-named local branch, and never `git push origin {current_branch}`, which hardcodes the *local* branch name as the destination: on a branch whose upstream is named differently (or lives on another remote) that pushes a spurious remote branch, leaves the real PR head stale, and `@{u}..HEAD` is still non-empty afterward while the push itself "succeeded". Retry once after `git pull --rebase --autostash` on a non-fast-forward, 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. + The `"$PUSH_REMOTE" = "."` guard is **load-bearing, not defensive**: on a local upstream `@{u}` resolves fine, so the no-upstream carve-out below never fires, and an unguarded push runs `git push . HEAD:refs/heads/main` — which silently fast-forwards the *local* default branch onto this branch's HEAD, exits 0, and leaves `@{u}..HEAD` empty. The assertion would then report the branch "reached the remote" and open a PR for a branch that was never pushed to any remote at all. + + Never a bare `git push`, which under `push.default=matching` fans out to every same-named local branch, and never `git push origin {current_branch}`, which hardcodes the *local* branch name as the destination: on a branch whose upstream is named differently (or lives on another remote) that pushes a spurious remote branch, leaves the real PR head stale, and `@{u}..HEAD` is still non-empty afterward while the push itself "succeeded". On a non-fast-forward, retry once behind `git pull --rebase --autostash` — and if that rebase **conflicts, abort it** (`git rebase --abort 2>/dev/null`) before reporting the failure, exactly as `lib/multi-reviewer-loop.md` step 5 does. `/do:pr` is invoked programmatically by `/do:next` and `/do:pr-better`, so a branch left detached mid-rebase becomes the tree their next step runs against — and a later `@{u}` assertion silently skips on a detached HEAD. 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. A **local** upstream is the case that does *not* skip — `@{u}` resolves, so the check runs and the `"."` guard above stops it rather than letting it push into the local repo. 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) diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 6ddc712..42114f1 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -148,15 +148,18 @@ describe('review-loop parse contracts', () => { // 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: match EVERY " here" exemption wherever it is - // worded and require each one to qualify itself, rather than blocklisting the - // handful of phrasings we happened to think of. A reworded exemption - // ("Optional passes do not count here.") is caught by the same rule. - const exemptions = [...wrapper.matchAll(/(?:ignored|excluded|excused|do not count)\s+here/g)]; - assert.ok(exemptions.length > 0, 'the aggregate rules must state an ~opt exemption somewhere'); + // Structural, not verbatim: match every " here" exemption by verb STEM, so + // inflections ("does not count here", "is not counted here", "waived here") are + // covered rather than only the handful of full phrasings we thought of, and check + // a window on BOTH sides — a qualification may precede the phrase ("Passes marked + // `~opt` whose status is not `push-failed` are ignored here") just as easily as + // follow it. No length>0 canary: a correctly-qualified rewording that happens to + // avoid every stem must not fail for being differently worded — line 159's + // positive assertion is what pins that the qualification exists at all. + const exemptions = [...wrapper.matchAll(/(?:ignor|exclud|excus|count|waiv|appl)\w*\s+here/gi)]; for (const m of exemptions) { assert.match( - wrapper.slice(m.index, m.index + 200), + wrapper.slice(Math.max(0, m.index - 200), m.index + 200), /push-failed/, `an ~opt exemption at index ${m.index} does not name the statuses it covers — unqualified, it reads as license to merge a pass whose fixes never reached the remote`, ); @@ -242,18 +245,64 @@ describe('review-loop parse contracts', () => { // upstream is named differently the push succeeds against a spurious ref, @{u}..HEAD // stays non-empty, and the run opens the PR anyway. const pr = readCommand('pr.md'); - assert.match(pr, /git push "\$\(git config --get "branch\.\$BR\.remote"\)" "HEAD:\$\(git config --get "branch\.\$BR\.merge"\)"/); + assert.match(pr, /PUSH_REMOTE="\$\(git config --get "branch\.\$BR\.remote"\)"/); + assert.match(pr, /PUSH_BRANCH="\$\(git config --get "branch\.\$BR\.merge"\)"/); + assert.match(pr, /git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); assert.match(pr, /never a bare `git push`/); assert.match(pr, /never `git push origin \{current_branch\}`/); // The gate that feeds this assertion must actually COMMIT its fixes: the check // compares against the upstream ref, so a fix left uncommitted in the working tree // is invisible to it and the PR opens from the pre-fix tree regardless. - assert.match(pr, /commit and push those fixes\*\* — leaving them uncommitted/); + assert.match(pr, /\*\*commit and push those fixes\*\*/); + assert.match(pr, /Leaving the fixes uncommitted is invisible to that section's assertion/); + // The gate's push must name its form, not leave it to the orchestrator — the two + // forms it would otherwise reach for are the two this file forbids. + assert.match(pr, /using the upstream-derived push described under "Open the PR"/); // 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('refuses to push into a local upstream instead of a remote', () => { + // `git branch --set-upstream-to=main` sets branch..remote=".", and @{u} then + // resolves fine — so the "no upstream, skip the check" carve-out never fires. An + // unguarded derived push runs `git push . HEAD:refs/heads/main`, which silently + // fast-forwards the LOCAL default branch onto this branch's HEAD, exits 0, and + // leaves @{u}..HEAD empty — so the guard reports the branch reached the remote and + // opens a PR for a branch never pushed anywhere. Verified against a scratch repo. + for (const [label, body] of [['pr.md', readCommand('pr.md')], ['multi-reviewer-loop.md', readLib('multi-reviewer-loop.md')]]) { + assert.match(body, /\[ "\$PUSH_REMOTE" = "\." \]/, `${label} must guard against a local ("." ) upstream`); + } + const pr = readCommand('pr.md'); + // The guard must come BEFORE the push it protects, and must stop the run. + assert.match(pr, /if \[ -z "\$PUSH_REMOTE" \] \|\| \[ "\$PUSH_REMOTE" = "\." \][\s\S]{0,600}?exit 1[\s\S]{0,200}?git push "\$PUSH_REMOTE" "HEAD:\$PUSH_BRANCH"/); + // -u rewrites branch..remote/.merge, so an unconditional `git push -u origin + // ` re-points an existing upstream and defeats the derived guard at + // its source — it must be scoped to the never-published case. + assert.match(pr, /\*\*No upstream yet\*\*[^\n]*`git push -u origin \{current_branch\}`/); + assert.match(pr, /`-u` \*rewrites\* `branch\.\.remote`\/`\.merge`/); + // A conflicted retry must not strand the branch mid-rebase for /do:next and + // /do:pr-better, which invoke /do:pr programmatically. + assert.match(pr, /conflicts, abort it\*\* \(`git rebase --abort 2>\/dev\/null`\)/); + }); + + it('files issues inside the scan-only gate, not after it', () => { + // The instruction originally sat in the paragraph AFTER "STOP HERE ... and exit", + // so an orchestrator following the gate literally exited before reaching it and + // `--scan-only --issues` filed nothing at all. Moving it back reads fine in + // isolation, which is exactly why it needs a test rather than a reviewer. + for (const name of ['better.md', 'better-swift.md', 'depfree.md']) { + const body = readCommand(name); + const gate = body.match(/\*\*GATE: If `--scan-only` was passed, STOP HERE[^\n]*/); + assert.ok(gate, `${name}: scan-only gate not found`); + assert.match( + gate[0], + /file every surviving finding as an issue first/, + `${name}: the issue-filing instruction must be inside the gate sentence, before the exit`, + ); + } + }); + 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. From 08455424815efb412b0d8128a0cc9b82b0e149b0 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 17:19:00 -0700 Subject: [PATCH 17/18] address review (claude): key do:pr's push case split on the remote value, not on whether @{u} resolves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2's split routed on `git rev-parse @{u}` succeeding, but a branch tracking a local ref resolves @{u} perfectly well — so a local upstream fell through to the derived-push case, which carries no "." guard, reproducing the same CRITICAL one section earlier: `git push . HEAD:refs/heads/main` fast-forwards local main, exits 0, empties @{u}..HEAD, and the PR-creation assertion then reads the branch as pushed. The alternative reading was no better — an orchestrator copying the guard wholesale dead-ends at Commit and Push printing "REFUSING TO CREATE THE PR" where no PR is being created, so a local-upstream branch could never be published at all. Discriminate on branch..remote being empty or "." instead. Both route to `git push -u origin `, which publishes the branch and re-points the local upstream at the remote — making the later guard defense-in-depth rather than a dead end. Tests now pin the discriminator itself, not just the -u form's placement. --- commands/do/pr.md | 14 +++++++++++--- test/review-loop-contract.test.js | 13 ++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/commands/do/pr.md b/commands/do/pr.md index f70223b..b0ce233 100644 --- a/commands/do/pr.md +++ b/commands/do/pr.md @@ -87,9 +87,17 @@ Print: `PR flow: {current_branch} → {default_branch}` - `git rebase {default_branch}` to replay this branch's commits on top of the now-current default branch. - If the rebase hits conflicts, **abort** (`git rebase --abort`) and stop — print the conflicting files and ask the user to resolve them, rather than guessing at a merge. Do not proceed to review against a half-rebased tree. - After a clean rebase the branch's merge-base with the refreshed local `{default_branch}` is current, so `git diff {default_branch}...HEAD` shows only this branch's own changes. -- Push the branch to remote (use `--force-with-lease` on any of these if the rebase above rewrote already-pushed history; never a bare `--force`). **Which form depends on whether the branch already tracks a remote** — `-u` *rewrites* `branch..remote`/`.merge`, so using it unconditionally would re-point an existing upstream at `origin/{current_branch}` and defeat the config-derived guard under "Open the PR" at its source: - - **No upstream yet** (`git rev-parse --abbrev-ref --symbolic-full-name @{u}` fails — the branch was never published): `git push -u origin {current_branch}`, which publishes it and sets the tracking config for the first time. - - **An upstream already exists**: push to the ref that upstream names, deriving it from config exactly as "Open the PR" below does — never `-u`, and never a destination built from the local branch name. A branch whose upstream is `upstream/feature-x` or `origin/pr-123-head` must keep pointing there. +- Push the branch to remote (use `--force-with-lease` on any of these if the rebase above rewrote already-pushed history; never a bare `--force`). **Which form depends on whether the branch's upstream names a remote** — `-u` *rewrites* `branch..remote`/`.merge`, so using it unconditionally would re-point an existing upstream at `origin/{current_branch}` and defeat the config-derived guard under "Open the PR" at its source. Discriminate on `branch..remote`, **not** on whether `@{u}` resolves: a branch tracking a *local* ref (`branch..remote=.`, what `git branch --set-upstream-to=main` produces) resolves `@{u}` perfectly well, so an `@{u}`-based test would route it into the second case and push into the local repository: + + ```bash + BR="$(git branch --show-current)" + PUSH_REMOTE="$(git config --get "branch.$BR.remote")" + ``` + + - **Not yet published to a remote** — `PUSH_REMOTE` is empty (no upstream at all) **or** `.` (upstream is a local branch): `git push -u origin {current_branch}`. This publishes the branch and sets the tracking config for the first time, re-pointing a local upstream at the remote — which is exactly what makes the "Open the PR" guard below defense-in-depth rather than a dead end. + - **A genuine remote upstream** (`PUSH_REMOTE` is a real remote name): push to the ref that upstream names, deriving it from config exactly as "Open the PR" below does — never `-u`, and never a destination built from the local branch name. A branch whose upstream is `upstream/feature-x` or `origin/pr-123-head` must keep pointing there. + + The empty/`.` check is part of **the derivation being reused here**, not part of the PR-creation assertion — so it applies at this step too, even though the guard's message under "Open the PR" is phrased for that later moment. ## Local Code Review (REQUIRED GATE) diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 42114f1..104029f 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -154,8 +154,9 @@ describe('review-loop parse contracts', () => { // a window on BOTH sides — a qualification may precede the phrase ("Passes marked // `~opt` whose status is not `push-failed` are ignored here") just as easily as // follow it. No length>0 canary: a correctly-qualified rewording that happens to - // avoid every stem must not fail for being differently worded — line 159's - // positive assertion is what pins that the qualification exists at all. + // avoid every stem must not fail for being differently worded — the verbatim + // "but never for `push-failed`" assertion further down is what pins that the + // qualification exists at all. const exemptions = [...wrapper.matchAll(/(?:ignor|exclud|excus|count|waiv|appl)\w*\s+here/gi)]; for (const m of exemptions) { assert.match( @@ -279,8 +280,14 @@ describe('review-loop parse contracts', () => { // -u rewrites branch..remote/.merge, so an unconditional `git push -u origin // ` re-points an existing upstream and defeats the derived guard at // its source — it must be scoped to the never-published case. - assert.match(pr, /\*\*No upstream yet\*\*[^\n]*`git push -u origin \{current_branch\}`/); + assert.match(pr, /\*\*Not yet published to a remote\*\*[^\n]*`git push -u origin \{current_branch\}`/); assert.match(pr, /`-u` \*rewrites\* `branch\.\.remote`\/`\.merge`/); + // The Commit-and-Push case split must key on the REMOTE VALUE, not on whether + // @{u} resolves: a local upstream resolves fine, so an @{u}-based test routes it + // into the derived-push case and pushes into the local repo — the same CRITICAL, + // one section earlier. The "not yet published" case must cover empty AND ".". + assert.match(pr, /Discriminate on `branch\.\.remote`, \*\*not\*\* on whether `@\{u\}` resolves/); + assert.match(pr, /\*\*Not yet published to a remote\*\* — `PUSH_REMOTE` is empty \(no upstream at all\) \*\*or\*\* `\.`/); // A conflicted retry must not strand the branch mid-rebase for /do:next and // /do:pr-better, which invoke /do:pr programmatically. assert.match(pr, /conflicts, abort it\*\* \(`git rebase --abort 2>\/dev\/null`\)/); From 1cbc601072ce275c699a066ba65fc1031560aea4 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 13 Aug 2026 07:54:46 -0700 Subject: [PATCH 18/18] address review (claude): pin the -u prohibition on the second push case, where the round-1 bug would re-enter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-3 assertions covered the first case-split bullet and the discriminator sentence, but nothing pinned the second bullet's 'never -u'. Reintroducing `git push -u origin {current_branch}` there — the exact round-1 CRITICAL, where -u rewrites branch..remote/.merge and re-points a differently-named or non-origin upstream at origin/ — passed all 15 tests in the file. It now fails. --- test/review-loop-contract.test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 104029f..7b79dd7 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -288,6 +288,11 @@ describe('review-loop parse contracts', () => { // one section earlier. The "not yet published" case must cover empty AND ".". assert.match(pr, /Discriminate on `branch\.\.remote`, \*\*not\*\* on whether `@\{u\}` resolves/); assert.match(pr, /\*\*Not yet published to a remote\*\* — `PUSH_REMOTE` is empty \(no upstream at all\) \*\*or\*\* `\.`/); + // ...and the OTHER half of the split, which is where the round-1 CRITICAL would + // re-enter: `-u` on a genuine remote upstream rewrites branch..remote/.merge, + // re-pointing a differently-named or non-origin upstream at origin/. + // Without this, reintroducing `-u` in the second bullet passes every other test. + assert.match(pr, /\*\*A genuine remote upstream\*\*[^\n]*never `-u`/); // A conflicted retry must not strand the branch mid-rebase for /do:next and // /do:pr-better, which invoke /do:pr programmatically. assert.match(pr, /conflicts, abort it\*\* \(`git rebase --abort 2>\/dev\/null`\)/);