feat(bitwarden-code-review): recognize confirmed stack layers when validating findings

[withinfocus]

🎟️ Tracking

Layer 3 of 4 in stack #221, bottom to top: #220, #217, #219, #218. Based on #217.

No Jira ticket. Found while adding stacked pull request support to bitwarden-delivery-tools.

📔 Objective

avoiding-false-positives assumes the diff under review is self contained. A stacked pull request is not. It is reviewed against the layer below it, so a layer adds exports and types whose consumers land in a layer above. Under the previous rules the reviewer reads that as dead code and posts findings for it on every such PR.

The hard part is not the relaxation, it is deciding when it applies. Anything the author controls is the wrong signal: the base ref is their choice, and so is the PR description. Bitwarden's server and clients repos run release branches with cherry-picks, so rc, hotfix-rc, and release/* PRs all have a non-default base while being ordinary self-contained changes, and the clients monorepo carries per-client variants like hotfix-rc-desktop. Relaxing completeness scrutiny on those is the opposite of what anyone wants.

So confirmation rests on structure instead, and both checks must hold:

1. An adjacent layer exists, proven with gh pr list: either the base ref is another open PR's head, or this PR's own head is another open PR's base. The second direction is what confirms the bottom layer of a stack, whose base is trunk and which is precisely the layer shipping consumers-not-yet-present.
2. The base is not a shared long-lived branch. develop and main, plus prefix matches on rc, hotfix-rc, and release/, so the per-client variants are covered. An unrecognized base falls back to a normal review.

A stack claim in the title or body is corroboration only and never satisfies check 1 alone. Status resolves once per review rather than per finding.

The relaxation is scoped to absent consumers. A failing build or compile check stays a finding, read from gh pr checks rather than guessed at from the diff, and so does any security or correctness defect in code the layer does contain.

Bash(gh pr list:*) is added to the agent's grants for both structural checks, matching the grant code-review and code-review-local already declare.

Version 1.14.2 to 1.15.0. MINOR rather than PATCH: the plugin's own Version Format section reserves MINOR for new organizational patterns, and this adds one.

[github-actions]

Claude Code validation

Result: Issues found

Validated the two changed plugins (bitwarden-code-review, bitwarden-security-engineer) against merge base 1ce37ec (origin/main). PR #219 is itself a stacked PR — its base is fix/resolve-diff-base-ref, so the authoritative changed-file list spans both layers and the bitwarden-security-engineer and commands/ files below are reviewed here alongside the top layer's own changes.

One critical finding: a newly added instruction in perform-security-review prescribes a shell-injection mitigation that does not work, and the failure defeats the very validation it is attached to. Everything else is quality. Version bumps and changelogs are correct and complete for both plugins.

Critical

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:60 — The stated shell-quoting mitigation is false, and its failure bypasses the validation gate. The new interactive fallback tells the model to accept a base ref from the user and states that because ; & | $ ( )are legal in a ref name, it should "double-quote the answer everywhere it is interpolated, including into that check itself." Double quotes suppress; & | ( )but **not**$or backtick — both named in the same sentence. Reproduced withanswer='main$(echo PWNED_EXECUTED >&2)'`:

  git check-ref-format --branch "$answer"              → fatal: not a valid branch name, exit 128
  bash -c "git check-ref-format --branch \"$answer\""  → PWNED_EXECUTED, exit 0
  

  Interpolating executes the payload, check-ref-format then sees only the residue main, and the answer is accepted as valid. The validation step is itself the injection sink. Amplifying: the same diff adds Bash(git check-ref-format:*) at line 10, a prefix rule, so the call matches an allowed pattern.

  Fix: do not build a shell string from the answer. Reject unless it matches a strict allowlist (^[A-Za-z0-9][A-Za-z0-9._/-]*$, no .., no trailing .lock) before any shell use, and delete the claim that double-quoting covers $ and backtick. If a shell check is still wanted, single-quote it and reject any answer containing '.

Major

• plugins/bitwarden-code-review/skills/avoiding-false-positives/SKILL.md:54 — The bottom-layer path is author-controllable, contradicting the premise the whole gate rests on. Line 45 justifies the gate on confirmation being structural, "which a branch name or a sentence in the description cannot supply on its own." That holds for the upper-layer path, but opening a second PR based on your own head branch is one gh pr create and immediately flips item 8 on. Failure scenario: PR adds an exported helper nothing calls; author opens a trivial follow-up PR off that branch; gh pr list --base <headRefName> returns one entry, headRefName is not long-lived, and the unused-export finding is dropped as "the design." Fix: require the dependent PR's diff to actually reference the symbol being flagged, and soften the "author does not control" framing on line 45 and in the changelog.
• plugins/bitwarden-code-review/skills/avoiding-false-positives/SKILL.md:49 — gh pr view and gh pr list are given with no PR number and no --repo, so they bind to the current checkout rather than the PR under review. Both commands take the PR as an argument and neither checks it out — Bash(gh pr checkout:*) is in the plugin's deny list at plugins/bitwarden-code-review/.claude/settings.json:11. Running /code-review 219 from a checkout on another branch makes bare gh pr view either error or silently return a different PR's refs, and the stack determination is then made against the wrong branches. The rest of the plugin passes the number explicitly. Fix: write gh pr view <number> --json baseRefName,headRefName and gh pr list --repo <owner/repo> --head <baseRefName> …, stating that <number> is the PR the invoking command already resolved. Same for gh pr checks on line 64.
• plugins/bitwarden-code-review/skills/avoiding-false-positives/SKILL.md:39 (with :62) — The carve-out also fires on the topmost layer of a stack, where no later layer exists to supply the consumer. The upper-layer path confirms any PR based on another open PR's head, including the last one, so item 8's rationale ("callers land in a later layer") is false there and genuinely dead code in the final layer is suppressed. Fix: tie the missing-consumer suppression specifically to a non-empty gh pr list --base <headRefName> result, leaving the upper-layer path to establish stack membership for other purposes.
• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:60 — No defined behavior when the answer fails check-ref-format. The text states the rejection but not what follows. Line 63 covers only the resolve/merge-base gate failures and the GITHUB_ACTIONS case, so a typo'd branch name — the most likely input — lands in neither branch. Fix: "On a rejected answer, say why and ask once more; on a second rejection, follow the same stop-and-recommend-PR-mode path as a gate failure."
• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:55 — The merge-base gate has no stated pass criterion. "…and it shares history with HEAD (git merge-base <candidate> HEAD)" names the command but not what counts as passing — conspicuous next to candidate 1's emphatic "Key on the exit status, never the output." Fix: "…and git merge-base <candidate> HEAD exits 0."

Minor

plugins/bitwarden-code-review/skills/avoiding-false-positives/SKILL.md

• :47 — The mode gate names only "PR mode" and "Local mode", but skills/performing-multi-agent-code-review/references/modes.md defines four; branch-comparison and commit-range reviews often do have an associated PR, the exact case line 47 wants to exclude. Fix: gate positively — "apply only when the review was invoked with an explicit pull request reference."
• :3 — The frontmatter description still scopes the skill to per-finding work, but the change adds a once-per-review, PR-level determination that runs gh commands. Fix: extend the description and the trigger cell at plugins/bitwarden-code-review/README.md:26 with "stacked PR" / "unused export" / "no caller".
• :43-64 — The new section is ~485 words against the file's ~1,084 and is seven paragraphs of rationale where every other section is terse bullets. Fix (optional): keep a ~10-line decision procedure inline and move the justification to references/stacked-prs.md.
• :56 — "begins with rc" is unanchored: it matches rc-refactor-parser or a user branch rcuvelier/fix. Fails safe, but fuzzier than intended. Fix: exact rc, or prefixes rc- / rc/.
• :53-54 — The queries are named but not their success condition. Fix: add "a non-empty JSON array confirms the path."
• :64 — gh pr checks exits non-zero when checks fail or are pending, so an agent reading exit status may treat a failing run as a tool failure. Fix: add "read its output, not its exit status", mirroring the precedent this same PR added at AGENT.md:44.

plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md

• :74 — The empty-diff reporting behavior is defined only for branch comparison mode, but line 72 stops the run on an empty file in any mode. Commit mode at HEAD, an empty time window, and a no-file-change PR all reach it with nothing to say. Fix: generalize to "no changes in the reviewed range."
• :72 — "Check the exit status and the file" has no granted mechanism: allowed-tools grants no test, wc, ls, or stat. Read works and is granted but is never named, so the model may reach for wc -c and trigger a permission prompt — in CI, a hang. Fix: "check the exit status, then Read DIFF_FILE; an empty read is the empty-file case."
• :49-63 — A2 adds ~430 words, roughly half explanation rather than procedure. The procedural core is ~150 words. Fix (optional): move the "why" to references/base-ref-resolution.md alongside the existing rubric reference.
• :63 (against :41) — Line 41 says "Execute these steps in order. Do not skip, reorder, or combine steps"; line 63 then directs the model from within A2 to emit through step 6's github output, skipping 1B–5. Fix: phrase as "emit the explanation using the github procedure described in step 6, then stop," and add a carve-out to line 41 for directed early exits.

plugins/bitwarden-security-engineer/CHANGELOG.md

• :16-19 — The entry itemizes allowed-tools changes in detail but omits two deltas in the same diff: Bash(git check-ref-format:*) was added (needed by the new line-60 validation) and Bash(gh pr list *) was removed (correctly — nothing in the body invokes it). Fix: add both.

Validated clean

• Version bumps complete and consistent. bitwarden-code-review 1.14.1 → 1.15.0 across plugin.json:3, .claude-plugin/marketplace.json:27, README.md:13, and AGENT.md:3 — correct MINOR for additive behavior. bitwarden-security-engineer 1.3.0 → 1.3.1 across plugin.json:3, marketplace.json:57, and README.md:20 — correct PATCH; that plugin's agent carries no version field. Both changelogs use Keep a Changelog with correct headings and dates.
• No prompt injection (CWE-1427). Nothing in the changed material addresses the reviewer of this PR, claims repository policy to suppress findings about itself, or attempts to steer this validation. All of it was treated as data.
• No credentials. No hardcoded secrets, tokens, keys, or embedded-credential URLs in either plugin. No settings.local.json in the changeset.
• New tool grants are justified and read-only. Bash(gh pr list:*) (needed by the new stacked-PR checks) and Bash(git rev-parse:*) (needed by the new local-mode base resolution) are both used and neither writes. git rev-parse over git symbolic-ref is the right choice — symbolic-ref has writing forms. gh pr list was correctly dropped from perform-security-review, which does not use it. No grant is broader than its changeset justifies.
• No slash-command shell-execution surface. Neither changed command file contains a bash-execution block, and neither interpolates arguments into a shell string. The $ARGUMENTS reference at code-review-local.md:29 is a model instruction on an untouched line, not a shell path.
• The YAML block-sequence allowed-tools / tools rewrite is valid — raised independently by three checkers, so recorded here rather than as a finding. These three files are the only ones in the repo using the list form, but Claude Code 2.1.220 validates both fields as "a string or array of strings", confirmed in the installed binary. The argument-hint quoting in both commands is a genuine fix: the prior unquoted [PR#] | [PR URL] was a YAML flow sequence followed by a pipe.
• Technical claims in the new text spot-checked and correct: git rev-parse --abbrev-ref origin/HEAD does exit 128 while printing the literal origin/HEAD, and git check-ref-format --branch does accept ;, &, |, $, and backtick. Both references/ targets referenced by the changed skills exist on disk.

Checks run

Check	Status
Plugin structure	Skipped — run as a dedicated workflow step before this review; see the job log and check status
Marketplace	Skipped — same
Version bump	Skipped as a script — same; consistency verified manually here and reported above
Plugin validation (AI)	Passed with notes — both changed plugins
Skill review (AI)	Issues found — both changed SKILL.md files
Configuration & security	Passed — AGENT.md and both command files; no findings introduced

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This adds a stacked-PR carve-out to avoiding-false-positives, gated behind two structural checks that read adjacent open pull requests rather than trusting the base ref or the PR description. Both paths guard the branch they test against shared long-lived names (main, master, develop, and the rc, hotfix-rc, release/ prefixes), the upper-layer path on baseRefName and the bottom-layer path on headRefName, which keeps release, hotfix, and rc-to-trunk promotion PRs under full scrutiny; unconfirmed cases fall back to a normal review, and the relaxation never reaches build failures or defects in code the layer contains. The new Bash(gh pr list:*) grant is read-only, is required by both checks, matches what code-review and code-review-local already declare, and is not touched by the plugin's deny list. Version 1.15.0 is applied consistently across marketplace.json, plugin.json, README.md, and AGENT.md, with a matching changelog entry, and no earlier findings on this PR remain open.

Not covered: Skill review did not run — this review path cannot launch plugin-dev:skill-reviewer, so plugins/bitwarden-code-review/skills/avoiding-false-positives/SKILL.md was not checked for description quality, length, or progressive disclosure. performing-multi-agent-code-review covers them where plugin-dev is installed.