[AI-58] llm: add bitwarden-planning-tools plugin with consulting-adrs skill - #177
[AI-58] llm: add bitwarden-planning-tools plugin with consulting-adrs skill#177SaintPatrck wants to merge 4 commits into
Conversation
|
Claude Code is validating plugin components and security... If this comment does not update with results, check the Actions log. |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed Code Review Details
No new findings this round. Prior threads were checked and not reopened: the em-dash re-escaping churn in No dependency manifest files are touched by this PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ce76862 to
8e05440
Compare
Claude Code validationResult: Pass Validated the new CriticalNone. Major
Minor
Notes (not findings)
Verified clean
Checks run
The three script checks are run by the workflow as dedicated steps before this review, so their absence from this report is not a pass — read their results from the job log. |
d4374e1 to
928b0d5
Compare
928b0d5 to
ab8303f
Compare
| "should_not_trigger_pass": "4/4", | ||
| "reliability": { | ||
| "all_runs_agree_rate": 0.9166666666666666, | ||
| "should_trigger_reliable": "7/8", | ||
| "should_not_trigger_reliable": "4/4" | ||
| }, |
There was a problem hiding this comment.
❓ QUESTION: The two negative cases that over-triggered were dropped and the reliability figures recomputed — scope decision or worth re-running?
Details
ab8303f removed "Help me write a new ADR documenting the token-rotation decision we just landed on." and "What's an ADR and when should our team bother writing one?" from trigger-eval.json, and adjusted this block to match: should_not_trigger_pass 6/6 → 4/4, all_runs_agree_rate 0.786 → 0.917, should_not_trigger_reliable 4/6 → 4/4. Those were the only two negative cases that failed (each fired 1/3), and the _notes line disclosing them was trimmed at the same time.
The remaining 12 per-query records are self-consistent with the new totals, so nothing here misreports. Two things worth confirming:
- If the removal was a scope call (authoring and conceptual asks are arguably ambiguous labels rather than skill defects), a one-line note in
evals/README.mdunder "Known issues / boundaries" would keep that reasoning with the artifact — the under-trigger case is documented there, this one now isn't. - If they are still in scope, ADR-authoring is the nearest over-trigger boundary for this skill and now has only partial coverage via
"Update ADR-0012's status to superseded...".
Either resolution is fine; the ask is that the recorded baseline carries the reasoning, since it's what the next refresh will be compared against.
There was a problem hiding this comment.
Scope decision. The dropped tests were flakey and not relevant until competing or complimentary skills are identified.
| name: consulting-adrs | ||
| description: Check a design, change, plan, or threat model against Bitwarden's Architecture Decision Records (ADRs), or locate and summarize the ADR catalog. Use when assessing whether an approach conflicts with, is governed by, or lacks an accepted ADR, or when someone needs to find or summarize ADRs. Produces structured findings (conflict, gap, stale-reference, aligned) with cited ADRs, or an ADR summary when that is the ask. | ||
| allowed-tools: WebFetch(domain:contributing.bitwarden.com), Read, Grep | ||
| disallowed-tools: Write, Edit, NotebookEdit, Agent |
There was a problem hiding this comment.
Bash survives the fork's tool removal, reopening the write and egress paths the fork exists to close.
Details and fix
The commit message states the reason for the fork plainly: the catalog fetch pulls untrusted public HTML into the context, and line 63 codifies that contributing.bitwarden.com is not trusted-by-construction. disallowed-tools removes Write, Edit, NotebookEdit, and Agent — the last one, per the PR body, because a forked agent otherwise delegates its way around its own tool scope.
Bash does the same thing more directly. With agent: general-purpose, the fork starts from the full tool set, and allowed-tools is a pre-approval rather than a ceiling (claude-config-validator/skills/reviewing-claude-config/reference/claude-code-requirements.md:32) — which is exactly why disallowed-tools is needed here in the first place. A Bash-capable fork can write files (>), reach hosts outside the WebFetch domain scope (curl), and read anything under cwd, so removing Write/Edit alone does not hold the boundary against content injected into a fetched ADR page.
The skill needs WebFetch, Read, and Grep; it has no use for Bash.
disallowed-tools: Write, Edit, NotebookEdit, Agent, BashIf forked skills do resolve allowed-tools as an exhaustive allowlist, this is belt-and-braces and costs nothing; if they don't, it closes the widest remaining hole in the isolation this commit builds.
Establish bitwarden-planning-tools as the pre-implementation planning home (counterpart to bitwarden-delivery-tools' post-implementation mechanics), and land consulting-adrs here as its first skill. consulting-adrs checks a design/change/plan/threat-model against Bitwarden's ADRs (or locates/summarizes the catalog) and returns structured conflict/gap/aligned findings with cited ADRs. Ships the full eval harness (trigger/structure/behavior) with baselines on claude-opus-4-8: triggering 7/8 should-trigger, 6/6 should-not; behavior with-skill 1.00 vs baseline 0.78 over 9 cases. WebFetch scoped to contributing.bitwarden.com. Proposed as the future home for tech breakdowns, initiative-funnel navigation, and architecting-solutions as planning tools consolidate here.
Treat fetched ADR pages as untrusted data, matching the sibling architecting-solutions skill's rule for the same domain. Drop the Cross-Plugin Integration table's consumer inventory rather than correct its premature claim — a skill has no need to track which plugins call it, and the table's only other row duplicated the Overview's own counterpart framing.
The catalog fetch pulls untrusted public HTML into whatever conversation asks for a validation, so the skill runs in its own context and returns only findings. Isolation means the subject arrives as an explicit input rather than ambient context, and a templated output keeps the roll-up terminal. A cited URL is confirmed against the catalog index, because a derived URL that does not resolve is worse than no URL.
d487c03 to
00f6b90
Compare
1 participant
🎟️ Tracking
AI-58
📔 Objective
Adds
bitwarden-planning-tools, a new plugin housing pre-implementation planning and preparation skills.Its first skill is
consulting-adrs: it checks a design, change, plan, or threat model against Bitwarden's Architecture Decision Records, or locates and summarizes the catalog, returning structured findings (conflict, gap, stale-reference, aligned) with cited ADRs. A cited URL is confirmed against the catalog index, so a finding never carries a link that does not resolve.The skill runs as a forked subagent and blocks until it returns, so consulting the catalog never pulls untrusted public HTML into the calling conversation. The caller passes the subject in and gets findings back. Inside the fork,
WebFetchis scoped tocontributing.bitwarden.comalongsideReadandGrep, whileWrite,Edit,NotebookEdit, andAgentare removed, the last of those because a forked agent otherwise delegates its way around its own tool scope.Ships a full eval harness per the AI Review Guidelines (triggering, structure, behavior) with baselines on
claude-opus-4-8, graded blind byclaude-opus-5andclaude-sonnet-5, recorded inskills/consulting-adrs/evals/.#178 wires
bitwarden-security-engineer's ADR-alignment checks to this skill and is stacked on this PR, which must land first.