diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index b4565de..2316f54 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -24,7 +24,7 @@ "name": "pr-flow", "source": "./plugins/pr-flow", "description": "PR review feedback loop for Claude Code. Create PRs with readiness checks, commit + push + trigger @claude review, inspect status, and work through review issues interactively.", - "version": "1.2.1" + "version": "1.2.2" } ] } diff --git a/plugins/pr-flow/.claude-plugin/plugin.json b/plugins/pr-flow/.claude-plugin/plugin.json index bd80772..4c6b95e 100644 --- a/plugins/pr-flow/.claude-plugin/plugin.json +++ b/plugins/pr-flow/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "pr-flow", "description": "PR review feedback loop for Claude Code. Create PRs with readiness checks, commit + push + trigger @claude review, inspect status, and work through review issues interactively.", - "version": "1.2.1", + "version": "1.2.2", "author": { "name": "gering" }, diff --git a/plugins/pr-flow/README.md b/plugins/pr-flow/README.md index aaada8c..8954689 100644 --- a/plugins/pr-flow/README.md +++ b/plugins/pr-flow/README.md @@ -58,9 +58,9 @@ If a clear pattern emerges (e.g. 18 of 20 last PRs were squashed), `/merge` sugg ### Force-push safety -**What it does.** Any force-push inside `/rebase` uses `--force-with-lease`, never `--force`. If the remote advanced since the last fetch (e.g., a teammate pushed), the push fails fast with a clear message. +**What it does.** Any force-push inside `/rebase` uses `--force-with-lease`, never `--force`. If the remote advanced since the last fetch (e.g., a teammate pushed), the push fails fast with a clear message. Whether `/rebase` asks first depends on risk: when the files changed on base and on the branch don't overlap, it rebases + force-pushes immediately (invoking the skill is the authorization); when files overlap, it presents a selection menu (rebase / show diff / leave as-is) before touching anything. -**Why it matters.** `--force` overwrites teammate commits silently. `--force-with-lease` is the safer default. The skills never bypass it, even under `--no-verify` scenarios — if a hook fails, we investigate, we don't skip. +**Why it matters.** `--force` overwrites teammate commits silently. `--force-with-lease` is the safer default. And a conflict-free catch-up rebase shouldn't cost a confirmation round-trip — prompts are reserved for cases where judgment is actually needed. ### Structured review output @@ -186,7 +186,7 @@ Each skill runs a preflight check and stops with a clear message if requirements ## Design principles -- **Interactive by default** — no silent commits, pushes, or fixes without user confirmation. The one explicit opt-in exception is `/cycle --loop`: the invocation authorizes the autonomous cycle, and even then the final squash asks first +- **Interactive by default, silent when risk-free** — no silent commits or fixes without user confirmation; decisions are real selection menus, not free-text prompts. Two deliberate exceptions where the invocation itself is the authorization: `/cycle --loop` (autonomous cycle; the final squash still asks) and `/rebase` with zero file overlap (conflict-free catch-up, aborts cleanly if a conflict appears anyway) - **Read-only where it matters** — `/check` never mutates anything - **User stays in control** — `/fix` does not auto-trigger `/cycle`; you decide when to re-push (or hand the wheel to `/cycle --loop` deliberately) - **Root cause over workaround** — `/merge` refuses `--admin` bypass. A failing required check is a signal to fix the check, not to skip it diff --git a/plugins/pr-flow/skills/cycle/SKILL.md b/plugins/pr-flow/skills/cycle/SKILL.md index c520005..ed07c1a 100644 --- a/plugins/pr-flow/skills/cycle/SKILL.md +++ b/plugins/pr-flow/skills/cycle/SKILL.md @@ -45,6 +45,7 @@ Strip the flags first; whatever is left over is the commit message. 3. **Handle uncommitted changes**: - Run: `git status --porcelain` + - **Guard against an unresolved merge state first**: if any entry carries a conflict status (`UU`, `AA`, `DD`, or any `U`/`U` combination — e.g. markers already in the tree from a merge/rebase/stash-pop you ran yourself before invoking `/cycle`), STOP. Do not `git add -A` over conflict markers. Report the conflicted files and tell the user to resolve them, then re-run `/cycle`. (Note: a stash-pop conflict from the step-2 rebase makes `/rebase` return non-cleanly, so step 2 already stops before here — this guard covers the pre-existing-markers case.) - If changes exist: - If `$ARGUMENTS` provided, use as commit message - Otherwise, generate a concise commit message based on the changes (show diff first) diff --git a/plugins/pr-flow/skills/merge/SKILL.md b/plugins/pr-flow/skills/merge/SKILL.md index 2deccd4..b70006b 100644 --- a/plugins/pr-flow/skills/merge/SKILL.md +++ b/plugins/pr-flow/skills/merge/SKILL.md @@ -15,7 +15,7 @@ user_invocable: true **No sanity-check prompts.** When every preflight check is ✅ (or ➖ N/A), execute the merge immediately without a final "Proceed? / Merge ausführen?" confirmation. The user ran `/merge` — that IS the authorization. -**Only ask when a decision is needed.** A prompt is warranted only for: draft-to-ready flip (step 1), rebase confirmation (delegated to `/rebase`), merge-method when ambiguous (step 9), WIP commit cleanup (step 10), and the three-way f/m/a decision when ⚠️ warnings exist (step 13). Cleanup defaults (step 12) apply silently unless something is protected/unusual. +**Only ask when a decision is needed.** A prompt is warranted only for: draft-to-ready flip (step 1), rebase confirmation (delegated to `/rebase`, step 3), merge-method when ambiguous (step 9), WIP commit cleanup (step 10), and the three-way f/m/a decision when ⚠️ warnings exist (step 13). Cleanup defaults (step 12) apply silently unless something is protected/unusual. **If ⚠️ warnings exist**, present the three-way prompt in step 13 **exactly once**. Never stack it with additional confirmation questions. @@ -35,24 +35,26 @@ user_invocable: true - `n`: stop - Store `PR_NUMBER`, `BASE_BRANCH`, `HEAD_BRANCH`. -2. **Rebase check** — delegate to `/rebase --no-poll --auto`: +2. **Local cleanliness** (run BEFORE the rebase — the step-3 rebase must not mutate a dirty tree): + - `git status --porcelain` — if anything: stop "Commit or stash local changes before merging (`/cycle` handles commit + push)." + - `git log @{u}..HEAD --oneline 2>/dev/null` — if unpushed commits: stop "Push local commits first (`/cycle` handles this)." + - Why first: `/rebase --auto` (step 3) auto-stashes a dirty tree and may force-push the rebased branch. If `/merge` rebased+pushed and only then rejected the dirty tree, it would have burned a force-push + CI run for a merge that never happens. Gating cleanliness here means the rebase only runs on a tree `/merge` will actually accept. + +3. **Rebase check** — delegate to `/rebase --no-poll --auto`: - Run `/rebase` with **both** `--no-poll` and `--auto`. Rationale: - `--no-poll`: polling would delay the merge — any review should already have been handled by a prior `/cycle`. - `--auto`: the user invoked `/merge`; that invocation authorizes rebase + force-push as preflight. Asking again would be a redundant second prompt. + - The working tree is already clean (step 2), so `/rebase --auto` won't stash/pop anything. - `/rebase --auto` still aborts cleanly on conflicts (no destructive behavior skipped — only the routine confirmation is skipped). - If `/rebase` aborts due to conflicts → stop this skill: "Merge requires up-to-date branch. Resolve conflicts, then re-run `/merge`." -3. **Local cleanliness**: - - `git status --porcelain` — if anything: stop "Commit or stash local changes before merging." - - `git log @{u}..HEAD --oneline 2>/dev/null` — if unpushed commits: stop "Push local commits first (`/cycle` handles this)." - 4. **Refresh PR state** (GitHub recomputes after push/rebase): - - Wait ~5s if anything was pushed/rebased in step 2 + - Wait ~5s if anything was pushed/rebased in step 3 - Re-run: `gh pr view --json mergeable,mergeStateStatus,reviews` - Interpret `mergeStateStatus`: - `CLEAN` → ✅ ready - `HAS_HOOKS` → ✅ ready (hooks will run) - - `BEHIND` → ⚠️ base moved again since step 2, re-run `/rebase` + - `BEHIND` → ⚠️ base moved again since step 3, re-run `/rebase` - `BLOCKED` → ⚠️ required reviews or checks missing (step 5 + 6 will detail) - `CONFLICTING` → ❌ stop: "Merge conflicts. Resolve manually (`git merge origin/` or `git rebase`)" - `UNSTABLE` → ⚠️ CI not green but branch protection allows merge — treat as warning @@ -138,8 +140,8 @@ user_invocable: true ── Preflight ── - ✅ Branch up-to-date with main ✅ Local clean (no uncommitted/unpushed) + ✅ Branch up-to-date with main ── GitHub state ── ✅ Mergeable: CLEAN diff --git a/plugins/pr-flow/skills/rebase/SKILL.md b/plugins/pr-flow/skills/rebase/SKILL.md index 7576ce7..7cbd495 100644 --- a/plugins/pr-flow/skills/rebase/SKILL.md +++ b/plugins/pr-flow/skills/rebase/SKILL.md @@ -1,20 +1,20 @@ --- name: rebase description: | - Rebases against the PR's actual base (`gh pr view`): shows new commits, - auto-stashes, aborts on conflicts, warns before force-push. + Rebases against the PR's actual base: proceeds without asking when + changed files don't overlap, menu otherwise, aborts on conflicts. Trigger: "rebase against main", "sync with base", "am I behind?". user_invocable: true --- # PR Rebase Check -> Detect whether the current branch is behind its target/base branch. If yes, show what's new on base, ask for confirmation, and rebase. Abort cleanly on conflicts. After a successful force-push (standalone use only), wait for an auto-triggered Claude review and present the result. +> Detect whether the current branch is behind its target/base branch. If yes, show what's new on base, then rebase — proceeding without a prompt when the changed files don't overlap, asking via a menu when they do. Abort cleanly on conflicts. After a successful force-push (standalone use only), wait for an auto-triggered Claude review and present the result. ## Arguments - `--no-poll` — Skip the post-push review polling step. Set automatically when this skill is invoked from `/cycle` or `/open` (the parent does its own polling). -- `--auto` — Skip the step-5 confirmation prompt. Proceed as if the user answered `y` — rebase, then force-push if an upstream exists. Used when the parent skill (`/merge`) has already authorized the rebase+push as part of its own invocation. Conflicts still abort cleanly. Stopping conditions (uncommitted changes, missing upstream, etc.) still apply. +- `--auto` — Skip the step-5 menu entirely (even when files overlap) — rebase, then force-push if an upstream exists. Used when the parent skill (`/merge`, `/cycle`) has already authorized the rebase+push as part of its own invocation. Conflicts still abort cleanly. Uncommitted changes are auto-stashed and popped afterward (not a stopping condition under `--auto`); other stopping conditions (missing upstream, detached HEAD, etc.) still apply. ## Why this skill exists @@ -53,58 +53,63 @@ This skill is also used internally by `/open` (step 2) and `/cycle` (step 2) — - If empty → ✅ "Branch is up-to-date with ``. No rebase needed." — stop. - If non-empty → continue to step 5. -5. **Show what's new and ask** (single confirmation covers rebase + force-push): +5. **Assess conflict risk, then proceed or ask**: - First, determine whether a force-push will follow — check if the branch has an upstream: - ``` - git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null - ``` - Store as `HAS_UPSTREAM` (true if the command succeeds with a non-empty value). + Gather the facts first: + - Upstream: `git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null` → store as `HAS_UPSTREAM` (true if non-empty). + - File overlap between both sides since the merge-base. Pass `--no-renames` so a rename can't hide a conflict: with rename detection on, a base-side rename `foo`→`bar` shows only `bar`, so if the branch edits `foo` the paths miss each other — and a rebase that CAN conflict (when the rename commit also touched those lines) would slip through as "no overlap". `--no-renames` surfaces the old path (`foo` as a delete) so that overlap is caught. This is deliberately conservative: a clean rename git would auto-resolve also gets flagged (an extra menu, never a skipped check). It also pins the result against the user's `diff.renames` config so the classification is deterministic. + ``` + comm -12 <(git diff --no-renames --name-only HEAD...origin/ | sort) \ + <(git diff --no-renames --name-only origin/...HEAD | sort) + ``` + - Always print the new-commits summary (from step 4) so the user sees what's coming in. - Then ask: + **Safe path — overlap is empty:** do NOT prompt (beyond step 6's uncommitted-changes guard, which may still ask in standalone mode). Announce in one line and continue to step 6: ``` - Branch `` is behind `` by N commit(s): - - abc123 Fix null check in auth handler - def456 Bump dependency X to 2.3.0 - 789aaa Refactor logger init - - Rebase `` onto `origin/` and force-push with --force-with-lease? - [y] yes, rebase + force-push - [n] no, leave as-is (warning will remain) - [d] show full diff first before deciding + No file overlap with `` — proceeding with rebase + force-push (--force-with-lease). ``` - - `d`: run `git log -p HEAD..origin/` (paginate for large diffs), then ask again - - `n`: stop with ⚠️ "Rebase skipped — branch remains N commits behind ``" - - `y`: continue to step 6. The user's `y` authorizes both the rebase AND the subsequent force-push (if `HAS_UPSTREAM`). Do NOT re-ask later. + Invoking `/rebase` is the authorization for this conflict-free case. The actual force-push happens in step 8, after the rebase succeeds — if a conflict surfaces anyway (semantic, or a rename `--no-renames` didn't catch), step 7's clean abort catches it and no force-push occurs. + + **Decision path — overlap is non-empty:** a judgment call is needed, so present a real selection menu via the **AskUserQuestion tool** — never a free-text `y/n/d` prompt: + - Question: "Branch `` is N commit(s) behind ``; M overlapping file(s): . Rebase + force-push?" + - Options: + 1. **Rebase + force-push** (Recommended) — continue to step 6. This single choice authorizes both the rebase AND the force-push (if `HAS_UPSTREAM`); do NOT re-ask later. + 2. **Show diff first** — run `git log -p HEAD..origin/` for the overlapping files (paginate), then re-present the menu. + 3. **Leave as-is** — stop with ⚠️ "Rebase skipped — branch remains N commits behind ``". - **If `--auto` was passed**: skip this prompt entirely. Still print the summary of new commits for transparency, then proceed to step 6 as if the user answered `y`. The parent skill's invocation is the authorization. + **If `--auto` was passed**: skip the menu even on overlap — print the summary, then proceed as if option 1 was chosen. The parent skill's invocation is the authorization. 6. **Uncommitted changes guard**: - Run: `git status --porcelain` - - If changes exist: stop with error "Uncommitted changes present — commit or stash before rebasing." - - Offer: "Stash them automatically? `git stash push -m 'pr-flow rebase auto-stash'` — [y/N]" - - If yes: stash, remember to pop after rebase - - If no: stop, user handles it + - If changes exist: + - **Standalone**: ask via the **AskUserQuestion tool** (menu, not free text): + - Question: "Uncommitted changes present — they'd block the rebase. Stash them?" + - Options: + 1. **Auto-stash** (Recommended) — `git stash push -m 'pr-flow rebase auto-stash'`, remember to pop after the rebase + 2. **Abort** — stop, user commits/stashes manually and re-runs `/rebase` + - **`--auto` mode**: auto-stash directly — `git stash push -m 'pr-flow rebase auto-stash'`, remember to pop after the rebase. No prompt: stash+pop is reversible (step 7 pops it on every `--auto` exit — clean success, conflict-abort, or other failure — and surfaces it if the pop itself conflicts), and the parent's invocation already authorized the rebase preparation — that is what `--auto` is for. This matters for `/cycle`, which rebases (step 2) *before* committing its pending changes (step 3): a hard stop here would abort the cycle before anything is committed. 7. **Execute rebase**: - Run: `git rebase origin/` - **On success**: - - If auto-stash was used, run `git stash pop`. If pop conflicts, warn but continue. + - If auto-stash was used, run `git stash pop`. If the pop itself **conflicts**, do NOT silently continue — STOP and report that the working tree now holds stash-pop conflict markers that must be resolved before any commit. A parent (`/cycle`) must NOT `git add -A` over them. This is not a "returned cleanly" outcome. - ✅ "Rebased successfully. Branch is now on top of `` (N commits replayed)." - - **On conflicts**: - - Run: `git rebase --abort` (clean state restored) - - If auto-stash was used, `git stash pop` to restore working tree + - **On conflicts** (rebase reports merge conflicts): + - Run: `git rebase --abort` (branch back to its pre-rebase commit; an auto-stash, if any, is still held — NOT yet popped) - ❌ "Rebase conflicts detected. Aborted cleanly — branch is back to its original state." - - List the conflicting files (from the rebase output) and suggest: - - "Option A: Resolve manually with `git rebase origin/`, fix conflicts, `git rebase --continue`" - - "Option B: Merge instead — `git merge origin/` (preserves branch history but creates a merge commit)" - - Ask which the user prefers — do NOT automatically resolve conflicts. + - **`--auto` mode**: if auto-stash was used, `git stash pop` to restore the parent's working tree (the parent expects its changes back). If that pop **conflicts**, report it explicitly as a stash-pop conflict (working tree now holds markers) — NOT as a plain "rebase aborted" — so the parent does not `git add -A` over it. Then report the conflicting files and stop, returning control to the parent (`/cycle`/`/merge` handle the outcome). Do NOT present a menu. + - **Standalone**: do NOT pop an auto-stash yet — a dirty tree would block the options below. List the conflicting files, then ask via the **AskUserQuestion tool** (menu, not free text): + - **Resolve manually** — run `git rebase origin/`, fix conflicts, `git rebase --continue`, then `git stash pop` if an auto-stash is held + - **Merge instead** — `git merge origin/` (creates a merge commit), then `git stash pop` if an auto-stash is held + - **Leave as-is** — stop, branch stays behind; if an auto-stash is held, `git stash pop` now to restore the working tree + - If an auto-stash is held and its pop is deferred to an option above, tell the user explicitly: "Your uncommitted changes are safe in stash `pr-flow rebase auto-stash` — pop them when done." + - Do NOT automatically resolve conflicts, regardless of choice or mode. + - **On any other failure** (non-zero exit that is neither clean success nor a merge conflict — e.g. a leftover `rebase-apply`/`rebase-merge` dir, a rejecting pre-rebase hook, an unreachable base): if a rebase is in progress, `git rebase --abort`; then if auto-stash was used, `git stash pop` so the changes are never stranded — and if that pop conflicts, surface the stash-pop markers (parent must not `git add -A` over them), same as the other branches; stop with the raw error. **Never leave an auto-stash dangling.** 8. **Post-rebase: remote state**: - - If `HAS_UPSTREAM` is true: execute `git push --force-with-lease` directly — the `y` from step 5 already covered this. Do NOT ask again. + - If `HAS_UPSTREAM` is true: execute `git push --force-with-lease` directly — step 5 already authorized this (safe path: the `/rebase` invocation; decision path: option 1; `--auto`: the parent's invocation). Do NOT ask again. - Set `FORCE_PUSHED = true` on success. - - If the push is rejected (e.g. remote advanced since `y`), stop with the error and suggest re-running `/rebase` — do NOT auto-retry with `--force`. + - If the push is rejected (remote advanced since the last fetch), stop with the error and suggest re-running `/rebase` — do NOT auto-retry with `--force`. - If `HAS_UPSTREAM` is false: no push needed, branch is local only (`FORCE_PUSHED = false`) 9. **Wait for auto-triggered review** (standalone only): @@ -151,7 +156,7 @@ This skill is also used internally by `/open` (step 2) and `/cycle` (step 2) — ## Notes -- This skill **never force-pushes without confirmation** — but the single `y` in step 5 authorizes both rebase and the subsequent force-push, so you only confirm once +- **Confirmation model**: zero file overlap → proceed without asking about the rebase itself (the `/rebase` invocation is the authorization; conflicts still abort cleanly). Overlap → one menu choice authorizes both rebase and force-push. The step-6 uncommitted-changes guard is independent of overlap and may still prompt on the safe path (standalone only — under `--auto` it auto-stashes instead of prompting). Decisions are always real selection menus (AskUserQuestion), never free-text prompts - Conflicts → abort + suggest, never auto-resolve - Safe to run repeatedly: if no rebase is needed, it's a 2-command no-op - Designed to be called both standalone and from `/open` / `/cycle`