PM-33385 - Craft a multi-agent code review#96
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Co-authored-by: Copilot <copilot@github.com>
theMickster
force-pushed
the
multi-agent-review
branch
from
985e9ed to
50067d7
Compare
| "metadata": { | ||
| "description": "Official Bitwarden Claude Plugin Marketplace", | ||
| "version": "1.0.1", | ||
| "pluginRoot": "./plugins" |
There was a problem hiding this comment.
I have been experimenting with cross agent configuration (namely Github Copilot) and I found that this configuration is incorrect. It should only be used it all plugins below did not have the ./plugins/ prefix in the source property. However, since all of them do, this should be removed.
There was a problem hiding this comment.
As noted in the pull request description, these standards are intentionally different because of the depth of work being done by the subagents. The intention is that we don't surface noise to engineers, but real signals of problems. I feel strongly that humans should be relied upon for suggestions and nit-picks. Claude should stay in this lane IMO.
There was a problem hiding this comment.
Happy to chat in a thread about my perspective and why I cut out all suggestions in this suggested skill.
| - Speculative issues that depend on specific inputs or runtime state without evidence those inputs occur in practice. | ||
| - Pre-existing issues not introduced or worsened by this change. | ||
|
|
||
| ## Confidence Scoring |
There was a problem hiding this comment.
I have found good success with providing Claude with a scale that has concrete descriptions of what is and is not considered a confident finding.
There was a problem hiding this comment.
Arguably the best suggestion from the /skill-creator was this markdown. Given the subagents a concrete messaging format has drastically improved consistency of findings and being able to track what agent came up with what finding is also very helpful.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the new Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
theMickster
added
the
ai-review-vnext
theMickster
removed
ai-review
theMickster
added
ai-review
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
This brought up a lot of composition questions from me, and letting other plugins own their space better with deferred review coming from here.
This feels a lot like the security review skill the security engineer has, but different in approach, and that concerns me about consistency. I think we need one to depend on the other, at least for approach definition, otherwise we will get rather different results.
This does seem to work fine as-is, but before it's available I think we have to solve the composition problem.
|
|
||
| - Model: Default to the opus model unless `--model` is specified. | ||
| - Announce which model is being used before starting the review. | ||
| - Don't write to GitHub. All findings go to a local markdown file. |
There was a problem hiding this comment.
❓ Can this lean into the available commands and distinguish between online and local?
There was a problem hiding this comment.
For v1, I did not envision the multi-agent review as being leveraged by the reusable workflow. My plan is that the skill is used and refined locally prior to making the necessary refactoring changes so that it works both for local reviews and for use in the GitHub reusable workflow.
| **Required — zero-knowledge and threat-model preamble.** Include this block verbatim in the subagent prompt: | ||
|
|
||
| > **Zero-knowledge invariant.** Bitwarden servers only store and synchronize encrypted vault data. The server, Bitwarden employees, and third parties must never be able to access unencrypted vault data. Encryption and decryption happen client-side only. The Master Key and Stretched Master Key are never stored on or transmitted to Bitwarden servers. | ||
| > | ||
| > **Threat-model directive.** Evaluate every change against P01–P06 and the requirements under VD/EK/AT/SC/TC (loaded via the `bitwarden-security-context` skill per the preceding block). For each finding that touches vault data, keys, auth tokens, or user authenticity, name the principle or category it implicates. |
There was a problem hiding this comment.
💭 I don't understand why this is here -- can you explain? I feel that this would be picked up from the skill and this will be more difficult to maintain; updates over there would miss this.
There was a problem hiding this comment.
Sure thing. I worked closely with the /skill-creator:skill-creator and Claude Docs sub-agents to craft the preamble. I think that line 53 could be slightly less specific on the P01-P06 principals; but there's strong evidence from the docs why these agents perform better with a strong preamble from the skill versus from the Claude orchestration agent just making something up.
The hardcoded preamble is following the our CLAUDE.md's explicit instruction: "When spawning subagents, include the Core Vocabulary definitions and zero-knowledge invariant in the agent prompt — subagents do not inherit CLAUDE.md context."
The docs back up why: for custom subagents like bitwarden-tech-lead, the docs say they "receive only this system prompt (plus basic environment details like working directory), not the full Claude Code system prompt" — so CLAUDE.md doesn't flow through.
Even for general-purpose (built-in) subagents that do load CLAUDE.md, the memory docs note "there's no guarantee of strict compliance" for user-message-delivered context.
The bitwarden-security-context skill handles the detailed vocabulary load. The preamble is the upfront framing that has to be present before the skill call — the two serve different roles.
Subagents receive only this system prompt (plus basic environment details like working directory), not the full Claude Code system prompt.
Claude Docs sub-agents
"CLAUDE.md content is delivered as a user message after the system prompt, not as part of the system prompt itself. Claude reads it and tries to follow it, but there's no guarantee of strict compliance."
How Claude remembers your project
|
|
||
| > **Tool discipline.** | ||
| > | ||
| > - Use Bash for all `gh`/`git` commands. Never use WebFetch or WebSearch. |
There was a problem hiding this comment.
The prompt passes the diff, file paths, and PR metadata directly — subagents don't need to discover anything. The "no probe" rule kills token waste. Step 1 also already verified the environment before subagents launch, so they're operating in a validated context.
|
|
||
| 3. Launch 3 agents using the `general-purpose` subagent type to independently review the changes. Each receives the diff and the Review Rules; each emits findings as a JSON array per the Finding Shape schema. In PR mode, pass the PR title and description only to Agent 3 per Context Partitioning — Agents 1 and 2 receive diff + Review Rules only. Send all 3 Agent tool calls in a single message (do NOT use run_in_background). | ||
|
|
||
| **Agent 1: Code quality agent** |
There was a problem hiding this comment.
💭 Similar to some of my other comments I wondering if this is better served as a call to a review skill within the software engineer plugin.
There was a problem hiding this comment.
This was an interested finding from the skill-creator to not attempt to leverage a custom subagent, but, instead, just use the general-purpose agent with a strongly grounded prompt.
I would prefer to leave this as-is for v1 unless we can prove that use a custom subagent is superior.
There was a problem hiding this comment.
I didn't dig super deep into comparing the general purpose agent with the project preamble grounding versus the security agent. Swapping the bitwarden-security-engineer should be a rather easy change that I can try this week.
There was a problem hiding this comment.
@SaintPatrck I made refactoring changes in a5689a5 to implement our custom security subagent. I also worked with the /skill-creator to refactor the wall of text that was the additional LLM security context wording.
SaintPatrck
left a comment
SaintPatrck
left a comment
There was a problem hiding this comment.
Started a first pass. There's a lot to unpack here.
Overall it's looking good. Nothing major stands out so far. Did leave a few suggestions. I'll pick back up Monday.
I'd like to start a thread regarding this comment in Slack. This could get philosophical. 😆
I agree that thinking through composition given the changes we have made in the last couple weeks are good to think though together and experiment. I'm also good to unpack r3180238995 some more. My wording was likely a little harsh; but I think where I was coming from is that I wanted this multi-agent review to primarily spotlight things of strong significance (blockers, bugs, and known bad patterns/practices) and less on minor things. 🤷🏼 |
…de-review/SKILL.md Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…de-review/SKILL.md Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Validation Summary — PR #96Plugin: Three validations ran:
Overall verdict: APPROVE with minor changes recommended. No critical or major security issues. All referenced files exist, version bump is correct, marketplace.json/plugin.json/CHANGELOG.md are consistent, and there are no committed secrets or dangerous permission grants. CriticalNone. Major (should fix)1.
|
|
|
||
| 4. Launch a single `general-purpose` validation subagent for all findings from Steps 2 and 3. The subagent receives the diff fetched with the mode's diff command from Step 1, the full array of finding objects, the Review Rules, and — in PR mode only — the PR title and description. The subagent returns an array of Step 4 objects (one per input finding) per the Finding Shape schema. | ||
|
|
||
| **Chunking escape hatch.** If raw findings from Steps 2 and 3 number more than 25, partition them into chunks of ≤ 15 (preserving collateral context within each chunk; do not split a `source_agent` group across chunks if it would put related findings on opposite sides) and launch one validation subagent per chunk in a single message (do NOT use run_in_background). |
There was a problem hiding this comment.
val-N IDs will collide across parallel chunked validation subagents and silently overwrite each other in the master map.
Details and fix
When more than 25 raw findings trigger the chunking path, multiple validation subagents run in parallel. Each subagent receives no shared counter and — per references/finding-shape.md:34 — emits its own collateral Finding objects with id: "val-N" starting at val-1. The Step 6 merge is keyed by id (per references/finding-shape.md:49-51), so if two parallel subagents each emit a val-1 (or both reach val-2, etc.), the later merge silently overwrites the earlier collateral finding. Collateral findings are by definition new issues that surfaced during validation, so silently losing them defeats the purpose of the collateral-change check on exactly the large-PR path where chunking is most likely.
Suggested fix: namespace the collateral ID by chunk index — e.g. val-{chunk}-N — and propagate the chunk index into each validation subagent's prompt. Update references/finding-shape.md to match.
SaintPatrck
left a comment
SaintPatrck
left a comment
There was a problem hiding this comment.
I'm on board with moving this forward and iterating.
3 participants
🎟️ Tracking
PM-33385 - Create multi-agent code review skill
📔 Objective
Adds a new
performing-multi-agent-code-reviewskill. Intentionally a different lens from the existingbitwarden-code-revieweragent — that agent reviews code the way a human reviewer would; this skill has Claude evaluate code as Claude, with output constraints a human can read. The pipeline runs 6 subagents per review each with a specific purpose and well-defined prompt.Nitty-gritty skill details
Expand
--modelto dial cost vs. depth.quality,bug,security) → per-finding validation → severity audit.Caught bysub-agent attribution (Architecture,Code quality,Bug analysis,Security & logic,Validation).Use cases
Expand
- At the end of a [Claude Code Agent Teams](https://code.claude.com/docs/en/agent-teams) coding session — slots into an agent feedback loop where teammates work, address findings, and refactor. - When an engineer is approaching the end of local development work and needs depth of review. - When an engineer has completed work on a draft PR and needs depth of review prior to publish. - When an engineer is peer-reviewing a high-density, cross-team, or very complex pull request.Field test results on live PRs
Expand
Cost estimates
Expand
Cost comparison sdk-internal/pull/1020
multi-agent review
single-agent
/bitwarden-code-review:code-review-localCost comparison misc/pull/348
multi-agent review
single-agent review
🧪 Testing
Expand for reviewer steps to try the four modes locally
Step 1: Install the required sibling plugins
The skill aborts without
bitwarden-tech-leadandbitwarden-security-engineeravailable in the same marketplace. Confirm both are installed before invoking.Step 2: Trigger each mode and confirm the report shape
In a Bitwarden repo checkout, invoke each form below and confirm a
code-review-*.mdlands in the directory you selected at invocation time (plugin data dir by default), with severity-bucketed findings and aCaught by:line on every finding:/performing-multi-agent-code-review 12345(substitute a real PR number)/perform-multi-agent-code-review/perform-multi-agent-code-review/performing-multi-agent-code-review on the last 5 commitsStep 3: Confirm the dismissed-findings block
Each report should include a collapsed "dismissed findings" section listing findings that didn't survive Step 4 validation or Step 5 severity audit, with the rejection reason.