fix: resolve the diff base from origin/HEAD instead of assuming main - #217
fix: resolve the diff base from origin/HEAD instead of assuming main#217withinfocus wants to merge 1 commit into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed this head against the files rather than the thread replies: the reviewer agent's Not covered: Skill review did not run — this review path cannot launch Code Review Details
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the base-ref change at this head against the files rather than the thread replies. Not covered: Skill review did not run — this review path cannot launch Code Review Details
PR Metadata Assessment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
3165253 to
0dc172d
Compare
70acfc2 to
2c29796
Compare
2c29796 to
6332774
Compare
4244054 to
8ef7114
Compare
30cb4ee to
bfc1a4a
Compare
96ec72f to
ef55ed0
Compare
290ff29 to
350b792
Compare
350b792 to
f4e9b0e
Compare
f4e9b0e to
004faff
Compare
0d5e4de to
7757699
Compare
7152a18 to
be12975
Compare
737c13c to
742202e
Compare
Local-mode code review and branch-comparison security review both diffed against a literal `main`, which is the wrong base in any repo whose default branch is named something else. Both now resolve `origin/HEAD` and ask when it cannot be resolved, and both gained the `git symbolic-ref` grant needed to run that resolution.
742202e to
f83c636
Compare
| - For 🎨 SUGGESTED and ❓ QUESTION: Never reopen after human provides answer/decision | ||
|
|
||
| Invoke `Skill(posting-bitwarden-review-comments)` to format and post each validated finding as an inline comment. | ||
| Invoke `Skill(posting-bitwarden-review-comments)` to format and post each validated finding as an inline comment. If the output destination in effect is local files — declared in the prompt, or set by entering local mode in Step 1 — format them the same way but write them to `review-inline-comments.md` in the working directory instead of posting. |
There was a problem hiding this comment.
♻️ DEBT: The local-file redirect for inline comments exists only at this call site; the skill it loads still says post to GitHub.
Details and fix
This changeset made the destination authoritative inside the skill for the summary path — skills/posting-review-summary/SKILL.md:14 gained a first routing row keyed on OUTPUT: local files, and its description was updated to name "caller-declared local-file output". The inline path got the opposite treatment: skills/posting-bitwarden-review-comments/SKILL.md is untouched, so its description still reads "Use this skill when posting inline comments to GitHub pull requests", its body is headed "GitHub Comment Posting Protocol", and rule 2 is "MUST Use inline comments for code-specific findings". It contains no local destination at all.
So on /code-review-local 123 — TARGET: PR #123 plus OUTPUT: local files, which line 21 of that command always appends — the agent enters PR mode, reaches this step, and loads a skill whose entire text says post, while still holding mcp__github_inline_comment__create_inline_comment. Only the sentence on this line stands between that and a comment on a command documented as never posting, which is weaker than the binding line 42 calls "the control".
Fix: add a destination row to skills/posting-bitwarden-review-comments/SKILL.md mirroring the one added to posting-review-summary/SKILL.md, so the skill is authoritative on where its own output lands rather than depending on the caller restating it.
|
|
||
| In those three cases, and only those, clear the diff file with step 7's `Write` before stopping. On the success path it must survive: steps 2 and 4 hand its path to the agents. | ||
|
|
||
| Report as the step 1-A2 abort does, to the resolved output destination. Stopping here skips step 5, so add the `Pending changes: not reviewed` note when the tree was dirty. Emit no assessment. Emit no APPROVE or REQUEST CHANGES assessment; nothing was reviewed. |
There was a problem hiding this comment.
♻️ DEBT: This line ends with the same instruction twice, and the second names a verdict this skill's report does not define.
Details and fix
As written: Emit no assessment. Emit no APPROVE or REQUEST CHANGES assessment; nothing was reviewed.
Two problems:
- The first sentence is a duplicate of the second.
- "APPROVE or REQUEST CHANGES" names nothing in this skill. Step 5's report template (lines 139-196) defines only the Summary count table plus 🚨 Blockers /
⚠️ Improvements / 📝 Notes / ✅ Strengths / ❌ Dismissed — no approve/request-changes verdict anywhere. That vocabulary belongs tobitwarden-code-review'sposting-review-summary, whose template does use it, so this reads as a copy across plugins. A model following the line has nothing to suppress, and may infer such a verdict is normally expected here.
Suggested replacement naming a construct that exists:
Report as the step 1-A2 abort does, to the resolved output destination. Stopping here skips step 5, so add the `Pending changes: not reviewed` note when the tree was dirty. Emit no summary table and no findings sections; nothing was reviewed.
| - Every candidate passes a `git merge-base` gate, so a shallow clone cannot yield a zero-byte diff reviewed as clean | ||
| - Every candidate passes an `^[A-Za-z0-9_][A-Za-z0-9._/-]*$` allowlist before reaching a command; a legal ref name can still be a shell payload or a leading-dash option | ||
| - Step 1B checks exit status and content before launching agents, and clears the diff file's contents on every abort path | ||
| - Step 1B and the step 1-A2 abort both write their reason to the `github` output destination, since stopping skips step 6 |
There was a problem hiding this comment.
♻️ DEBT: This entry records the superseded routing — both aborts write to the resolved destination, not to github.
Details and fix
SKILL.md:55 (the A2 abort): "Write it to the resolved output destination, the same one a completed review would use." SKILL.md:71 (the 1B stops): "Report as the step 1-A2 abort does, to the resolved output destination."
github is one of three outcomes there, and the change away from keying on the environment was deliberate — the thread that drove it closed with "I also changed the gate on both paths from the bare GITHUB_ACTIONS check to the resolved destination, since step 6 gives an explicit --output chat precedence over the environment variable." This bullet is the only place that still records the earlier design, so a reader consulting it concludes an abort under --output file or chat reports nowhere.
| - Step 1B and the step 1-A2 abort both write their reason to the `github` output destination, since stopping skips step 6 | |
| - Step 1B and the step 1-A2 abort both write their reason to the resolved output destination, since stopping skips step 6 |
|
|
||
| ### Removed | ||
|
|
||
| - The `Bash(gh api --method GET *)` and `Bash(rm -f /tmp/security-review-*)` grants, with no replacement. Step 7 clears the temp diff with the granted `Write` tool instead, so cleanup still runs unattended. **Migration:** a deployment that needs GHAS evidence on the unattended path must add its own `gh api` allow rule and pair it with a read-only `GH_TOKEN`. Any `allowed-tools` wildcard absorbs an inserted `-X DELETE`, so no rule can constrain the verb — `references/tool-grants.md` shows the bypass. Step 1-C's four scan-evidence calls now prompt, and in CI they are denied and render as `Not checked (permission denied)` rather than `None`. A deployment that wants GHAS evidence unattended should control it with a read-only `GH_TOKEN` and its own narrow grant |
There was a problem hiding this comment.
♻️ DEBT: This bullet states the migration guidance twice and counts four scan-evidence calls where SKILL.md says three run.
Details and fix
The GH_TOKEN guidance appears at both ends of the same bullet:
Migration: a deployment that needs GHAS evidence on the unattended path must add its own
gh apiallow rule and pair it with a read-onlyGH_TOKEN.
A deployment that wants GHAS evidence unattended should control it with a read-only
GH_TOKENand its own narrow grant
Same instruction, ~70 words apart, in the one bullet a reader on a pinned version consults for what they have to do.
The count is also off against the skill: SKILL.md:75 reads "Three run per invocation: code scanning takes either the PR-mode or the ref-mode form." Four commands are listed, three prompt, so three are denied in CI — which is the number that matters here, since it is the number of Not checked (permission denied) rows the report renders.
Fix: change "four scan-evidence calls" to "three", and drop the trailing sentence in favour of the **Migration:** one, which already carries it.
1 participant
Both review paths diffed against a hardcoded
main. On a repository whose default branch is named something else, or a branch cut fromrc, they reviewed the wrong range.perform-security-reviewgains--base-refand otherwise resolves the base from an ordered candidate list:origin/HEADfirst, then the repository default branch viagh repo view. Every candidate passes an allowlist before it reaches a command, thenrev-parse --verify, thenmerge-base. If none passes, the run stops and says so instead of writing a zero-byte diff that four agents then review as clean.The reviewer agent's local mode diffs
origin/HEAD...HEADand passes the symbolic ref straight to git rather than resolving it to a name first. An empty result counts as a failure, not a clean review.Quoting the
argument-hintin both commands fixed frontmatter that had never parsed, which made theirallowed-toolslive for the first time and put every grant in those files under review.gh apiandrmare gone. Any wildcard in anallowed-toolsrule absorbs an inserted flag, so no rule can constraingh api's verb;references/tool-grants.mdshows the bypass. GHAS scan evidence now prompts, and CI records it as not checked rather than as nothing found./code-reviewno longer resolves a pull request from the checkout. Both commands pass the resolved target on aTARGET:line, and/code-review-localaddsOUTPUT: local filesso its review cannot reach a GitHub comment.bitwarden-code-review1.14.1 to 2.0.0 andbitwarden-security-engineer1.3.0 to 2.0.0. Major on both, for the removed grants and the changed resolution.