[AI-59] llm: wire bitwarden-security-engineer ADR checks to consulting-adrs - #178
[AI-59] llm: wire bitwarden-security-engineer ADR checks to consulting-adrs#178SaintPatrck wants to merge 1 commit into
Conversation
…skill Replace the local references/adr-alignment.md dedup with invocations of Skill(bitwarden-planning-tools:consulting-adrs) in bitwarden-security-context, reviewing-security-architecture, and threat-modeling. The shared ADR-alignment rule now lives in one cross-plugin skill that returns structured conflict/gap/aligned findings with cited ADRs; the reference file is removed. Adds an install-time dependency on bitwarden-planning-tools (1.0.0+). Version bump 1.3.0 -> 1.4.0 with changelog. NOTE: depends on the consulting-adrs skill (bitwarden-planning-tools 1.0.0), which is on a separate unmerged branch — that must land first.
Plugin Validation Report — PR #178Validated Verdict: no critical issues. 3 major items should be fixed before merge — one of them will fail CI.
Critical (0)None. Major — must fix1. Prettier violations will fail the
|
| Location | Issue | Fix |
|---|---|---|
skills/bitwarden-security-context/SKILL.md:66 |
ADR check weakened from mandatory to descriptive. The deleted file said "Security assessments must check alignment against these"; the replacement reads "To check a design or change against them, invoke…", which sounds optional. The other two call sites kept imperative framing. | Restore the obligation: "Security assessments must check alignment against them — invoke Skill(…), which returns…" |
| All three call sites | The security-specific rationale was dropped: the deleted file explained that "a design can satisfy P01-P06 in the abstract while still contradicting a specific, already-decided architectural direction." consulting-adrs is deliberately domain-neutral and cannot supply this. |
Restore the clause in bitwarden-security-context:66 |
skills/reviewing-security-architecture/SKILL.md:133-136 |
Of the three ground rules, the local bullets cover conflict ("Cite it, don't just flag it") and gap ("gap triggers") — but nothing covers verify-status-before-citing, the one rule with no local survivor. | Add a third bullet on ADR status verification |
Minor — pre-existing (out of scope for this PR; would need their own bump)
README.md:11-19— Skills table lists 7 of 9 skills; missingbitwarden-security-context(changed here, and invoked cross-plugin by two other plugins) andauditing-hackerone-vulns.skills/bitwarden-security-context/SKILL.md:3— description promises "data classification standards," but that table actually lives inreviewing-security-architecture/SKILL.md:88-97; the skill's third section is "Security Requirements by Category (VD/EK/AT/SC/TC)". Also uses second person ("Use when you need…") with no quoted trigger phrases, unlike the other two skills.agents/bitwarden-security-engineer.md:3— description has no<example>blocks; the repo's other four agents all use them.agents/bitwarden-security-engineer.md:6-12—skills:list declares 6 of 9, omittingbitwarden-security-context,perform-security-review,auditing-hackerone-vulns, while line 32 claims "All skills are preloaded."skills/perform-security-review/SKILL.md:4-5—argument-hintis a slash-command field, not a SKILL.md field;allowed-toolsis space-separated whereskills/auditing-hackerone-vulns/SKILL.md:4uses commas. If the parser expects a comma list, the intended tool grants may not apply.skills/reviewing-security-architecture/SKILL.md:6andskills/threat-modeling/SKILL.md:6— no H1; both open at##.- None of the three changed skills declare
allowed-tools, though they now dispatch cross-plugin. Non-blocking (omission grants defaults rather than restricting), but inconsistent withassessing-test-coverage/SKILL.md:5,architecting-solutions/SKILL.md:4, and this plugin's ownperform-security-review/SKILL.md:5.
Verified clean
Reference integrity. A repo-wide search for adr-alignment returns exactly one hit — CHANGELOG.md:12, the historical note describing the removal. No SKILL.md, references/*.md, README.md, or agent file still points at the deleted path. The now-empty plugin-root references/ directory was removed cleanly. All 15 remaining references/… and examples/… paths across the plugin resolve to existing files. The plugin no longer uses ${CLAUDE_PLUGIN_ROOT} anywhere.
Dependency target is real. plugins/bitwarden-planning-tools/skills/consulting-adrs/SKILL.md exists with name: consulting-adrs, and bitwarden-planning-tools is at 1.0.0, satisfying the "1.0.0+" claim. Skill(plugin:skill) is the correct invocation form and matches all three existing cross-plugin dispatches in this repo (assessing-test-coverage/SKILL.md:18, performing-multi-agent-code-review/SKILL.md:69, architecting-solutions/SKILL.md:11).
The migration is a content upgrade when the dependency is present. consulting-adrs covers all three deleted ground rules and strengthens each: conflict now requires quoting the decision text (line 21), "significant" gets a concrete test (line 22), and status handling covers the full Accepted/Superseded/Deprecated/Rejected/Proposed taxonomy (line 18). It adds a fourth finding type (Stale-reference) and anti-hallucination rules the deleted file lacked.
Version and changelog. 1.4.0 agrees across .claude-plugin/plugin.json, .claude-plugin/marketplace.json:57, README.md:21, and the CHANGELOG.md heading. Keep a Changelog format is correct, entries are in descending order, ### Changed is the right category, and MINOR is the right semver call for a behavior change plus a new external dependency with no removed capability.
Security scan — no findings. No secrets, tokens, API keys, or hardcoded credentials in any changed file. No settings.local.json or credential files (.env, .pem, id_rsa) tracked anywhere in the repo. No settings files touched, so no permission-scoping or dangerous-auto-approval changes. The only secret-pattern hits in the plugin are skills/detecting-secrets/SKILL.md:36,40 — unchanged by this PR and explicitly labeled illustrative "NOT a real secret" / "REAL secret" teaching fixtures. The newly depended-on consulting-adrs scopes its network access correctly: WebFetch(domain:contributing.bitwarden.com) over HTTPS, not open WebFetch.
Skill content quality. Word counts are within or acceptably near the 1,000–3,000 target: bitwarden-security-context 738 (under, but a table-dense quick-reference by design), reviewing-security-architecture 1,116, threat-modeling 1,817. Progressive disclosure is intact. Removing the ADR bullet from threat-modeling/SKILL.md:116-121 improved that section's coherence — it was the only entry using a ${CLAUDE_PLUGIN_ROOT} prefix and the only one pointing outside the skill's own references/; the remaining four bullets are uniform and all resolve.
Structure. 1 agent (valid: name 29 chars lowercase-hyphens, model: opus, color: red, tools includes Skill, ~830-word system prompt). 9 skills, all with valid name + description frontmatter matching their directory names. No hooks, no MCP config. README.md, CHANGELOG.md, .claude-plugin/plugin.json all present; no stray artifacts among 24 files.
Recommended order
pnpm run format— unblocks CI (Major Dependency Dashboard #1)- Add not-installed behavior to the three
Skill()call sites; if degrading, restore the ground rules inline (Major Initialize Bitwarden Claude Plugin Marketplace #2) - Document the
bitwarden-planning-toolsrequirement in the plugin README and agent (Major Initial AI setup #3) - Restore "must" and the P01-P06 rationale at
bitwarden-security-context:66; add the status-verification bullet toreviewing-security-architecture:133-136(Minor) - Defer the pre-existing minors to a follow-up PR with its own version bump
Validated with plugin-dev:plugin-validator, plugin-dev:skill-reviewer, and claude-config-validator:reviewing-claude-config. scripts/validate-plugin-structure.sh, scripts/validate-marketplace.sh, and prettier could not be executed in this sandbox; those checks were performed by direct file inspection instead.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Documentation-only change that repoints the ADR-alignment check in three Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ## Architecture Decision Records (ADRs) | ||
|
|
||
| Bitwarden's accepted architecture decisions are catalogued separately from the security principles above. See `${CLAUDE_PLUGIN_ROOT}/references/adr-alignment.md` for how security assessments should check alignment against them. | ||
| Bitwarden's accepted architecture decisions are catalogued separately from the security principles above. To check a design or change against them, invoke `Skill(bitwarden-planning-tools:consulting-adrs)` — it returns structured findings (conflict, gap, aligned) with cited ADRs. |
There was a problem hiding this comment.
Details and fix
All three edited skills (bitwarden-security-context:66, reviewing-security-architecture:133, threat-modeling:12) now delegate the ADR-alignment check to Skill(bitwarden-planning-tools:consulting-adrs), and references/adr-alignment.md — which carried the ground rules (conflict = finding, undocumented significant decision = gap, verify status before citing) — is deleted. If bitwarden-planning-tools is not installed, the check silently no-ops with no residual guidance.
The PR description says this "adds an install-time dependency on bitwarden-planning-tools (1.0.0+)", but nothing in the diff declares or surfaces it: plugin.json has no dependency field, and plugins/bitwarden-security-engineer/README.md is unchanged (no prerequisite noted in Overview/Usage). The CHANGELOG entry is the only place it appears, which readers won't see at install time.
Two suggested fixes:
- Add the graceful-degradation sentence this repo already uses for cross-plugin skills, e.g.
plugins/claude-config-validator/skills/reviewing-claude-config/SKILL.md:167("This skill is optional. If unavailable, rely on the manual security checks above.") andplugins/bitwarden-product-analyst/agents/product-analyst.md:195. Retaining a one-line summary of the conflict/gap/status rules inline would keep the skills useful standalone. - Note the
bitwarden-planning-toolsrequirement inplugins/bitwarden-security-engineer/README.md, mirroring the "Consumers" row already added on the producer side (plugins/bitwarden-planning-tools/README.md:21).
1 participant
🎟️ Tracking
AI-59
📔 Objective
Wires
bitwarden-security-engineer's ADR-alignment checks to theconsulting-adrsskill. Thebitwarden-security-context,reviewing-security-architecture, andthreat-modelingskills now perform the check by invokingSkill(bitwarden-planning-tools:consulting-adrs)instead of reading a localreferences/adr-alignment.md; that reference file is removed. The shared rule now lives in one cross-plugin skill that returns structured conflict/gap/aligned findings with cited ADRs.Adds an install-time dependency on
bitwarden-planning-tools(1.0.0+). Version bump 1.3.0 to 1.4.0 with changelog.Stacked on #177 (the plugin and skill), which must land first.