fix: resolve the diff base from origin/HEAD instead of assuming main

[withinfocus]

🎟️ Tracking

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

No Jira ticket. This came out of an audit of how the plugins resolve a diff base.

📔 Objective

Two review paths compared against a literal main:

• bitwarden-code-reviewer local mode (git diff main...HEAD)
• perform-security-review branch comparison mode (git diff main...HEAD)

In a repo whose default branch is named anything else, both produce the wrong range and the review silently covers the wrong commits. Most other plugins already resolve this properly, so these two were the outliers.

Both now resolve the base with git rev-parse --abbrev-ref origin/HEAD, which yields origin/main and is the form performing-multi-agent-code-review already uses under a Bash(git rev-parse:*) grant. git rev-parse is read-only across its whole argument space, which matters because these agents read contributor-authored diffs unattended: git symbolic-ref would have done the same job, but its two-argument and --delete forms write, and a prefix grant reaches them.

Three things follow from making the base variable:

• The report header. perform-security-review Step 5 emitted Code Review: {branch} vs main. Left alone it would have published a security report naming a base it was not reviewed against. It now reads vs {base-ref}.
• A path that has no origin/HEAD. actions/checkout and --single-branch clones do not create the ref, so the resolution exits 128 there. Branch comparison mode falls back to the repository's default_branch over gh api, already covered by an existing grant, and prompts only on interactive runs. Under $GITHUB_ACTIONS it stops with a clear error rather than waiting for an answer nobody can give.
• A subagent cannot prompt. The reviewer agent is always reached through Task, so "ask the user" is not available to it. It returns the request for a base branch to the invoking command instead of guessing.

Both grants are added alongside the instructions that need them.

Versions: bitwarden-code-review 1.14.1 to 1.14.2, bitwarden-security-engineer 1.3.0 to 1.3.1.

[github-actions]

Claude Code validation

Result: Issues found

Validated PR #217 (fix/resolve-diff-base-ref, head fc73bdd) against merge base 1ce37ec. In scope: two changed plugins (bitwarden-code-review, bitwarden-security-engineer), one agent, two commands, one skill, one new skill reference file, and the root marketplace manifest. No root-level CLAUDE.md, .claude/, settings.json, hooks.json, or MCP config changed, so .claude-pr/ was empty and every file was read from the working tree.

The changeset is a real fix: the previous argument-hint values in both command files were unquoted and contained |, which made their YAML fail to parse outright — meaning neither command's allowed-tools list was in effect before this PR. That is now corrected, and the empty-diff guard added to perform-security-review step 1B closes a genuine "four agents report clean over zero bytes" hole. The findings below are what the fix left open.

The verdict is Issues found on the security clause, not on severity: three findings concern a tool grant or an unvalidated value reaching a shell. No critical findings.

Critical

None.

Major

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:52 — Shell-safety allowlist is applied to only one of three candidate sources. Line 58 requires a user-typed branch name to match ^[A-Za-z0-9._/-]+$ "before the value is placed in any command", but candidate 1 (git rev-parse --abbrev-ref origin/HEAD, taken "verbatim") and candidate 2 (the API's .default_branch) reach line 68 unvalidated — the one command the skill explicitly authorizes to use shell redirection. git check-ref-format --branch accepts main$(id), main`id`, main;id, main&id, and main|id as legal ref names (verified), so a repository whose default branch is so named yields command substitution inside git diff <base-ref>...HEAD > /tmp/.... The changeset's own new reference file argues this exact point at references/base-ref-resolution.md:70-74, then applies the control to one path out of three. Before this change the base ref was the literal main, so the exposure is introduced here. Fix: move the allowlist ahead of both gates so it covers every candidate regardless of source — "every candidate must match ^[A-Za-z0-9._/-]+$ before either gate; a candidate that does not match is discarded and resolution falls through" — and drop the per-source wording at line 58 so there is one control at one choke point.

• plugins/bitwarden-code-review/agents/bitwarden-code-reviewer/AGENT.md:43 — Same hazard, no control at all. The new local-mode instruction resolves a base ref and interpolates it directly into git diff <base-ref>...HEAD with no validation of any kind. The paragraph deliberately borrows two gates from perform-security-review (the exit-status rule, the keep-origin/-prefix rule) and states a reasoned decision to skip a third, but drops the allowlist silently with no rationale. Fix: preferred — remove the interpolation entirely by passing the symbolic ref to git rather than the resolved name: git diff origin/HEAD...HEAD, resolving the name for display only if the stop-and-report message needs it. Otherwise carry over the sibling skill's ^[A-Za-z0-9._/-]+$ check before the value reaches any command.

• plugins/bitwarden-code-review/commands/code-review/code-review.md:11 — Bash(git rev-parse:*) granted to a command that cannot reach the code path needing it. This command's body runs no bash at all; all three prompt variants it builds (lines 53, 79) hardcode "Review the currently checked out pull request", so the agent is always sent into PR mode. git rev-parse is used only in local mode (AGENT.md:43), and the subagent draws tool access from its own AGENT.md:14 grant rather than from the invoking command, so the command-level grant is dead either way. The changelog justifies it as applying "to both commands that reach local mode", which is true of code-review-local.md but not this one — and the immediately preceding commit (1ce37ec) set the opposite precedent by giving Bash(git status:*) to code-review-local only. Fix: drop line 11 from code-review.md; keep it in code-review-local.md and AGENT.md, where it is earned. (Reported independently by two reviewers; merged at the higher severity.)

• plugins/bitwarden-code-review/commands/code-review/code-review.md:20 — Write(/tmp/review-summary.md) does not match the path it is meant to scope. In Claude Code permission syntax a single leading / resolves relative to the settings source, not the filesystem root; // is required for a true absolute path. As written the rule matches <settings-dir>/tmp/review-summary.md, not /tmp/review-summary.md — the path the body names at line 58 and that skills/posting-review-summary/SKILL.md:159 writes in agent mode. Practical impact is currently limited and fails closed: the command body never writes anything itself, and the subagent that does still holds a bare Write. But the grant does not do what the changelog says it does, and it would fail silently in non-interactive CI if the command session ever wrote the summary directly. Fix: Write(//tmp/review-summary.md). (Confirmed independently by two reviewers against reference/claude-code-requirements.md:291-293.)

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:58 — The interactive fallback is unexecutable in most non-CI contexts, and its failure mode restores the bug this PR removes. printenv GITHUB_ACTIONS rules out GitHub Actions; it does not establish that a human is reachable. When the skill runs inside a subagent (this plugin's own agent invokes sibling skills via Skill()), under claude -p, or in non-GitHub CI, the variable is unset, the skill asks a question, and a parent agent answers it by inventing a plausible branch name — most likely main. That is the hardcoded-main bug again, now laundered through a step that looks validated. The sibling plugin already recognises this: AGENT.md:43 says "You run as a subagent and cannot prompt mid-run, so do not assume main". Fix: make the abort unconditional when no candidate passes, and add an explicit --base-ref <ref> parameter to argument-hint (line 4) and the ## Parameters section, validated with the same allowlist. A caller-supplied argument works in every invocation context; a mid-run question does not.

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:53 — "Build this candidate only if candidate 1 failed" is ambiguous, and one reading defeats the merge-base gate. Line 51 says stop at the first candidate passing both gates, and references/base-ref-resolution.md:44-46 says the merge-base gate exists so a resolvable-but-disconnected candidate "falls through to the next candidate". But line 52 has just defined candidate 1's success as "exits 0", so "candidate 1 failed" most naturally reads as the command failed. Under that reading, a shallow clone where origin/HEAD resolves but git merge-base exits 128 never builds candidate 2 and aborts, even though the API lookup might have passed. Fix: restate as "Build candidate 2 only if candidate 1 did not pass both gates — either the rev-parse exited non-zero, or the resulting candidate failed either gate."

Minor

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:10 — Bash(git check-ref-format:*) is granted, but the skill never runs it; its only mention (line 58) states it is deliberately not the control being used. A permission with no corresponding command, which invites a future editor to reach for the disclaimed check. Fix: delete line 10. (Found by two reviewers independently.)

• plugins/bitwarden-code-review/commands/code-review-local/code-review-local.md:2 — the new argument-hint appends | (blank for local changes), but untouched body line 33 says the opposite: "If no arguments provided, ask the user if there is a related PR number or URL". commands/code-review-local/README.md:17 agrees with the body. This is a fresh mismatch introduced by the same PR that was fixing one. Fix: use "[PR#] | [PR URL] | (blank to be asked PR vs. local)", or change line 33 to default to local review.

• plugins/bitwarden-code-review/commands/code-review/code-review.md:24 — the new hint is accurate (the body has no $ARGUMENTS handling at all, so the old PR#/URL promise was silently ignored), but the hint edit stranded two now-contradictory statements the diff did not touch: line 24 still says "a GitHub pull request or local changes", and commands/code-review/README.md:7-14 still documents /code-review [PR#] with optional arguments. Fix: in this PR, cut "or local changes" from line 24 and update the command README to say the command takes no arguments.

• plugins/bitwarden-security-engineer/skills/perform-security-review/references/base-ref-resolution.md:48 — "the abort path deletes the file rather than assuming it is empty" contradicts SKILL.md:61 ("no diff file exists to clean up") and SKILL.md:228 ("A run that aborted in step 1-A2 never created one"). The reference is describing step 1B's cleanup but calls it "the abort path", which in SKILL.md names a different thing — and a reader arriving at this file cold is the intended use. Fix: "step 1B deletes the file on any non-success exit; the step 1-A2 abort never creates one."

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:61 — the abort routes its output on GITHUB_ACTIONS alone and skips step 6, inverting step 6's own rule (line 204) that --output wins and the env var is only the default. --output file --output-dir ~/reports inside Actions writes the abort text to /tmp/review-summary.md and nothing at the requested path. Fix: route the abort text through step 6's output selection, then stop.

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:70 — "check for content by Reading the file" makes the orchestrator pull the entire diff into its own context on every successful run purely to test emptiness, reversing the design at lines 113 and 124 that keeps the diff out of the coordinator by passing a path. Fix: Read with limit: 1, or grant Bash(wc -c /tmp/security-review-*.diff) and key on the byte count.

• plugins/bitwarden-code-review/agents/bitwarden-code-reviewer/AGENT.md:43 — six ordered directives compressed into one ~150-word paragraph, with the override stated after the procedure it overrides: the agent is told to resolve the ref and fetch the diff, and only in the next sentence learns "If the invoking prompt names a base ref, use it and skip this resolution." An agent executing top-down runs the resolution before reaching the sentence that cancels it. "Skip PR metadata and thread detection" is also stranded at the end of a paragraph about ref resolution. Fix: restructure as an ordered sub-list with the precedence condition first.

• plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:49,58,70 — two rationale passages are duplicated verbatim between SKILL.md and the new reference file (check-ref-format is not a shell-safety check → base-ref-resolution.md:67; redirection-truncates-first → base-ref-resolution.md:41-43). Duplicated rationale is what the reference file exists to prevent, and the copies will drift. Fix: keep only the imperative in SKILL.md and let the reference carry the why; also trims ~60 words from the longest step.

• SKILL.md:5-20, AGENT.md:14-29, code-review.md:3-20, code-review-local.md:2-20 — the YAML block-sequence form for tools/allowed-tools is a repo-wide style outlier: it is the only sequence among ~25 SKILL.md files and every other plugin's agent and command. This is style, not breakage — see the adjudication note below. Fix (optional): either collapse to the repo-standard comma-separated string, keeping the corrected :* entries, or adopt the sequence form deliberately and note it as the new convention.

Notes

One reviewer disagreement, adjudicated. The skill-reviewer rated the YAML-sequence rewrite of allowed-tools as a possible grant-dropping breakage. I verified this directly rather than taking either side: plugin-dev/skills/command-development/references/frontmatter-reference.md:62 documents allowed-tools as "Type: String or Array of strings" with an explicit block-sequence example, and the skills frontmatter reference states the field "Accepts a space- or comma-separated string, or a YAML list." Both plugin-validators also parsed all four files successfully. The sequence form is supported; the concern is downgraded to the style note above. Recorded here because a silent downgrade would hide a real disagreement between checkers.

Noted, not counted (pre-existing, no edit worsened it). AGENT.md's new text says "stopping is safe", but it stops only when origin/HEAD fails to resolve. A base that resolves yet shares no merge base with HEAD — the shallow-clone case base-ref-resolution.md:28-46 documents — makes the three-dot diff exit 128 with empty output, readable as "no changes". The previous git diff main...HEAD had the same exposure with no error handling at all, so this changeset does not worsen it. Worth a follow-up; perform-security-review gained a merge-base gate for exactly this and the agent did not. Separately, Bash(git branch --show-current:*) in SKILL.md is bypassable (git branch --show-current --no-show-current -D victim exits 0 and deletes the branch, verified), but :* and * are documented equivalents so the reformat admits exactly the same command set as before — pre-existing, also worth a follow-up.

Verified clean. Version bumps are consistent and correctly sized: bitwarden-code-review 1.14.1→1.14.2 across plugin.json, marketplace.json, README.md:13, and AGENT.md; bitwarden-security-engineer 1.3.0→1.3.1 across plugin.json, marketplace.json:57, and README.md:20 (its agent is a flat file with no version: field, so nothing to bump there). Both CHANGELOGs use Keep a Changelog format with correct categories. No hardcoded credentials in any added line. No settings.local.json in the changeset. No hooks or MCP config in either plugin. references/base-ref-resolution.md resolves from SKILL.md:49. SKILL.md grew 1,800→2,368 body words, still inside the 1,000–3,000 target. No CWE-1427: nothing in the changeset addresses a reviewer, claims repository-policy authority, or attempts to direct this review — all six reviewers checked for this independently.

Checks run

Check	Status
Plugin structure (script)	Skipped — run as a dedicated workflow step before this review; see the job log and check status
Marketplace (script)	Skipped — same
Version bump (script)	Skipped — same. Bumps were verified manually by both plugin validations and are consistent
Plugin validation (AI)	Ran — both changed plugins validated (bitwarden-code-review, bitwarden-security-engineer)
Skill review (AI)	Ran — perform-security-review/SKILL.md
Configuration & security	Ran — secret scan clean; agent, both commands, and the new reference file reviewed
Enhanced secret detection (cross-plugin)	Skipped — bitwarden-security-engineer is not installed in the plugin cache; manual pattern scan used

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This replaces two hardcoded main diff bases with origin/HEAD resolution in bitwarden-code-reviewer local mode and perform-security-review branch comparison mode, and adds the read-only grants (git rev-parse, git merge-base, printenv GITHUB_ACTIONS) the new path needs. The choice of git rev-parse over git symbolic-ref holds up: the prefix grant stays read-only across its whole argument space, which matters for agents that read contributor-authored diffs unattended. Keying resolution on exit status rather than stdout is correct — git rev-parse --abbrev-ref origin/HEAD exits 128 and still prints origin/HEAD — and the merge-base gate plus the step 1B exit/emptiness checks close the zero-byte-diff-reviewed-as-clean path. Both plugins carry matching version bumps and changelog entries across marketplace.json, plugin.json, README.md, and AGENT.md; the quoted argument-hint also repairs frontmatter that previously failed to parse, so both commands' allowed-tools take effect for the first time.

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

Code Review Details

• 🎨 : Bash(git check-ref-format:*) grants a command the procedure no longer runs
  • plugins/bitwarden-security-engineer/skills/perform-security-review/SKILL.md:10

[claude]

🎨 SUGGESTED: Finding 1: Bash(git check-ref-format:*) grants a command the procedure no longer runs.

Details and fix

The only two mentions of git check-ref-format in the skill (SKILL.md:58 and references/base-ref-resolution.md:67) both say it is not the control — the allowlist regex is. No step invokes it, and the changelog's Added section lists only git rev-parse, git merge-base, and printenv GITHUB_ACTIONS, so this line looks like a leftover from the earlier iteration where the check was the validation.

Dropping the line keeps allowed-tools a faithful description of what the skill runs:

  - Bash(git branch --show-current:*)
  - Bash(git diff:*)

Low risk either way — git check-ref-format is read-only — but an unused grant in a security skill's allowlist invites the next reader to assume the check happens.