Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion commands/do/pr-better.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ The only Phase 7-equivalent housekeeping that applies:
After Phase A leaves all fixes committed on `{CURRENT_BRANCH}`, hand off to the workflow defined in `~/.claude/commands/do/pr.md`, forwarding all five review flags extracted in argument forwarding — `REVIEW_AGENT_ARG` (the `--review-with <value>` pair, possibly comma-separated), `REVIEW_STOP_ARG` (the stop-mode flag, if any), `REVIEW_MODE_ARG` (the `--review-mode <value>` pair, if passed), `REVIEWER_APPLIES_ARG` (the `--reviewer-applies` token, if passed), and `REVIEW_ITERATIONS_ARG` (the `--review-iterations <value>` pair, if passed) — so the chosen reviewer(s), stop-mode, dispatch mode (series/parallel), editing mode, and copilot iteration cap all run on the combined PR:

1. **Detect branches** — already done in pre-flight, reuse those values
2. **Commit and push** — commit any remaining staged changes (e.g., the PLAN.md update), then run `do:pr`'s "Commit and Push" step verbatim: fast-forward the **local** `{default_branch}` ref to origin and rebase the branch onto it (`git fetch origin {default_branch}:{default_branch} && git rebase {default_branch}`, aborting and surfacing conflicts rather than guessing) so the reviewers below — which diff against the local `{default_branch}` — see a current base, then `git push -u origin {current_branch}` (`--force-with-lease` if the rebase rewrote pushed history)
2. **Commit and push** — commit any remaining staged changes (e.g., the PLAN.md update), then run `do:pr`'s "Commit and Push" step verbatim: fast-forward the **local** `{default_branch}` ref to origin and rebase the branch onto it (`git fetch origin {default_branch}:{default_branch} && git rebase {default_branch}`), resolving and continuing through conflicts with [lib/rebase-conflict-resolution.md](../../lib/rebase-conflict-resolution.md), including regeneration of generated artifacts from their canonical inputs, so the reviewers below — which diff against the local `{default_branch}` — see a current base; then push using `do:pr`'s upstream-derived rules (`--force-with-lease` if the rebase rewrote pushed history). A rebase conflict is not a handoff or stop condition.
3. **Local Code Review (REQUIRED GATE)** — run the full review gate from `do:pr`. The do:better Phase 4b internal review covered the worktree diff against the default branch, but the do:pr review gate also covers any prior commits on the feature branch that predate this run. Do not skip it.
4. **Open the PR** — create a single PR with a description that summarizes both:
- The original feature work on the branch (from prior commits)
Expand Down
4 changes: 2 additions & 2 deletions commands/do/pr.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Print: `PR flow: {current_branch} → {default_branch}`
- **Sync the branch onto the latest `origin/{default_branch}` first.** Reviewers below diff the branch against `{REVIEW_BASE}` (`git diff {REVIEW_BASE}...HEAD`) — the **local** `{default_branch}` ref normally, `origin/{default_branch}` on the linked-worktree fallback below — anchored on the merge-base. If `origin/{default_branch}` moved forward since this branch was cut and the branch was never rebased, reviewers evaluate against a stale base and flag unrelated changes that landed on the default branch outside this branch's work. Rebase to eliminate that noise:
- `git fetch origin {default_branch}:{default_branch}` to fast-forward the **local** `{default_branch}` ref (and its remote-tracking ref) to match origin. A plain `git fetch origin {default_branch}` only moves the remote-tracking ref — the reviewers diff against the *local* ref, so it must be the one advanced, or the merge-base stays stale and the noise remains. This works because the flow already moved you to a feature branch (Detect Branches step 3), so the default branch isn't checked out **here**. **In a linked worktree it can still fail** — git's refusal is repo-wide, so `fatal: refusing to fetch into branch 'refs/heads/{default_branch}' checked out at …` means the *parent* repo holds it, which is the normal state when `/do:next` or a claim flow invoked `/do:pr`. That is not a divergence and not a reason to stop: fall back to `git fetch origin {default_branch}` and rebase onto the remote-tracking ref (`git rebase origin/{default_branch}`), and have the reviewers diff `origin/{default_branch}...HEAD` — the local ref is the parent repo's to advance, and its cleanup phase does that. **Record the resolved base as `{REVIEW_BASE}`** — `{default_branch}` normally, `origin/{default_branch}` on this fallback — and pass it to the review loops as their `{BASE_BRANCH}` input; that is the only name they read, so a fallback that isn't recorded under it leaves them diffing the stale local ref anyway. If your local `{default_branch}` has diverged from origin and the fetch can't fast-forward it (unusual), surface that and stop rather than forcing it.
- `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.
- If the rebase hits conflicts, **resolve them and continue the rebase**. Do not abort or stop merely because conflicts exist. Read and follow [lib/rebase-conflict-resolution.md](../../lib/rebase-conflict-resolution.md): inspect the replayed commit and both sides, resolve human-authored sources semantically, regenerate generated artifacts from their canonical inputs (for example `server/lib/apiRouteCatalog.generated.json`), run the focused checks, stage the resolution, and repeat `git rebase --continue` until complete. Only the playbook's last-resort, evidence-backed ambiguity may abort the rebase; a generated-file conflict alone never qualifies.
- After a clean rebase the branch's merge-base with `{REVIEW_BASE}` is current, so `git diff {REVIEW_BASE}...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's upstream names a remote** — `-u` *rewrites* `branch.<name>.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.<name>.remote`, **not** on whether `@{u}` resolves: a branch tracking a *local* ref (`branch.<name>.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:

Expand Down Expand Up @@ -169,7 +169,7 @@ This phase drives the **multi-reviewer wrapper** (defined under "Reviewer loop b

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.
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`. If that rebase conflicts, **resolve it through [lib/rebase-conflict-resolution.md](../../lib/rebase-conflict-resolution.md), continue until the rebase completes, rerun the focused checks affected by the resolution, then retry the same upstream-derived push**. Do not classify an active rebase conflict as a push failure and do not stop merely to ask the user to resolve it. `/do:pr` is invoked programmatically by `/do:next` and `/do:pr-better`, so the resolution must leave the expected branch checked out and fully rebased before the workflow continues. **If the push still fails after that one retry, do NOT create the PR** — this means after any conflict was resolved and the rebase completed; 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)
Expand Down
2 changes: 1 addition & 1 deletion install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ LIBS=(
gh-host github-reviewer-loop graphql-escaping
local-agent-review-loop model-tiers multi-reviewer-loop next-swarm ollama-review-loop
per-finding-root-cause plan-id-format plan-issue-mode
post-review-doc-recommendations remediation-agent-template
post-review-doc-recommendations rebase-conflict-resolution remediation-agent-template
review-agent-selection review-config-defaults review-convergence-gate
swift-review-checklist swift-gotchas
review-surface-scan review-surface-quality review-security-audit
Expand Down
2 changes: 1 addition & 1 deletion lib/local-agent-review-loop.md
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro
```bash
git push origin {BRANCH_NAME}
```
If the push fails (e.g., non-fast-forward), run `git pull --rebase --autostash && git push origin {BRANCH_NAME}` once before reporting failure.
If the push fails (e.g., non-fast-forward), run `git pull --rebase --autostash` and then retry the push once. If the pull stops on conflicts, do not abort or report failure merely because the conflict exists: read and follow [rebase-conflict-resolution.md](./rebase-conflict-resolution.md), resolve and continue the rebase, rerun the build/tests affected by the resolution, then push. Report failure only after the completed resolution and retry still cannot publish the branch.

6. **Re-loop or stop**:
- `ITERATION=$((ITERATION + 1))`
Expand Down
10 changes: 5 additions & 5 deletions lib/multi-reviewer-loop.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,20 @@ This is the default path. Iterate `REVIEW_AGENTS` in order, running each reviewe
# empty-string remote: a fatal error, reported as a bogus push-failed.
if [ -n "$UNPUSHED" ]; then
if ! git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"; then
# Never leave a conflicted rebase behind: `push-failed` is a continue-signal,
# so the next reviewer would otherwise run against a detached, mid-rebase HEAD
# with this pass's fix commit unapplied — worse than the unpushed state.
# A conflict is a resolution handoff, not push-failed. The caller MUST apply
# rebase-conflict-resolution.md and rerun this config-derived block before it
# records a status or dispatches the next reviewer.
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
echo "REBASE_CONFLICT_NEEDS_RESOLUTION"
false
fi
fi
fi
```

If `UNPUSHED` was non-empty the inner loop's push step did not run, and the block above already pushed it — `git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"`, retried once behind `git pull --rebase --autostash` on a non-fast-forward, exactly the retry the loop files use. **Run the whole thing as one block**: `PUSH_REMOTE`/`PUSH_BRANCH` live only in the shell that set them. If the retry's rebase hits a conflict it is **aborted**, leaving the branch exactly as the reviewer left it, and the pass is recorded `push-failed` — never left mid-rebase. That matters more here than in the inner loops, which run the same `pull --rebase && push` idiom as their last act before reporting failure: `push-failed` is a *continue-signal*, so execution goes on to the next reviewer, and a detached mid-rebase HEAD would both corrupt that reviewer's tree and silently disable its own assertion (no resolvable `@{u}` on a detached HEAD ⇒ the check skips). If the push still fails, record the pass per the rules below.
If `UNPUSHED` was non-empty the inner loop's push step did not run, and the block above already attempted to push 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. `REBASE_CONFLICT_NEEDS_RESOLUTION` is an explicit handoff to the orchestrator, not the final result of the block: while Git reports an active rebase or unmerged paths, do **not** record `push-failed` and do not dispatch another reviewer. Read and follow [rebase-conflict-resolution.md](./rebase-conflict-resolution.md), complete the rebase and its focused validation, then rerun this entire config-derived block so the variables and destination are fresh. Only a non-conflict pull error, the playbook's last-resort abort after a genuine ambiguity, or a push that still fails after the completed resolution becomes `push-failed`. This prevents both premature abandonment and a next reviewer inheriting a detached, mid-rebase HEAD.

**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 `<dst>`, git resolves `HEAD` to the **local** branch name and pushes to `refs/heads/<local-name>`, 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.<name>.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`.

Expand Down
2 changes: 1 addition & 1 deletion lib/ollama-review-loop.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro
```bash
git push origin {BRANCH_NAME}
```
If the push fails (non-fast-forward), run `git pull --rebase --autostash && git push origin {BRANCH_NAME}` once before reporting failure.
If the push fails (non-fast-forward), run `git pull --rebase --autostash` and then retry the push once. If the pull stops on conflicts, do not abort or report failure merely because the conflict exists: read and follow [rebase-conflict-resolution.md](./rebase-conflict-resolution.md), resolve and continue the rebase, rerun the build/tests affected by the resolution, then push. Report failure only after the completed resolution and retry still cannot publish the branch.
6. **Re-loop or stop**:
- `ITERATION=$((ITERATION + 1))`
- **Apply the convergence gate** (`~/.claude/lib/review-convergence-gate.md`) before another round: if the round just completed made zero commits or landed only *marginal* findings (edge-case guards, hypotheticals with no concrete wrong outcome), **converge — set `STATUS=clean` (or `STATUS=incomplete` if the round had any coverage gap, `REVIEW_ERRORS + PARSE_ERRORS + TRUNCATED > 0`) and exit**, noting the diminishing-returns convergence in the report. The coverage-gap exception is the same invariant step 3 enforces: a partially-reviewed diff is never `clean`, even when the gate converges. Only a round with at least one *substantive* finding earns another pass.
Expand Down
Loading