[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 5 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Re-review of the ADR-check rewiring. Verified the deleted 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).
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.
ce76862 to
8e05440
Compare
928b0d5 to
ab8303f
Compare
…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.
4c86059 to
36cb60a
Compare
Claude Configuration Validation — PR #178PR: AI-59 llm: wire bitwarden-security-engineer ADR checks to consulting-adrs VerdictNo critical issues and no errors. Structure, versioning, changelog, frontmatter, JSON validity, and reference integrity all pass, and the secret/permission scan is clean. Three warnings should be addressed before merge — all concern the cross-plugin dependency this PR introduces rather than the rewrite itself. Recommendation: approve with changes requested (warnings only). What this PR doesDeletes 1. Plugin Validation (plugin-validator, plugin-dev)Result: PASS with warnings. Verified clean:
2. Skill Review (skill-reviewer, plugin-dev)Result: needs improvement — rewrite is correct, dependency handling is not.
Cross-plugin invocation syntax: correct. Finding taxonomy in the skills: correct. Positives: progressive disclosure is well executed in 3. Configuration & Security Review (reviewing-claude-config, claude-config-validator)Result: PASS — no security findings.
Warnings (should fix)Warning 1 — MAJOR: No graceful-degradation guidance at the three new call sites
All three issue an unconditional imperative to invoke a skill in a separately-installable plugin, with no instruction for the case where Remediation — append a degradation clause to each call site. Two conventions already exist in this marketplace; pick one:
For a security artifact, prefer skip-and-disclose over silent skip: e.g. "If Warning 2 — MAJOR: The cross-plugin dependency is asserted but never documented for users
Remediation — add a Related semver note: replacing a self-contained capability with a hard cross-plugin dependency is arguably breaking for installs that only have this plugin. Adopting the skip-and-disclose fallback in warning 1 would make MINOR (1.4.0) unambiguously correct. Warning 3 — MINOR: Stale finding taxonomy left in the changelog
Remediation — change to Suggestions (optional, low priority)
Notes on things checked and cleared
Checks that could not run
|
|
|
||
| ### Changed | ||
|
|
||
| - `bitwarden-security-context`, `reviewing-security-architecture`, and `threat-modeling` now perform the ADR-alignment check by invoking `Skill(bitwarden-planning-tools:consulting-adrs)` instead of reading a local `references/adr-alignment.md`. Removed that reference file; the shared rule now lives in the `consulting-adrs` skill, which returns structured conflict/gap/aligned findings with cited ADRs. Requires the `bitwarden-planning-tools` plugin (1.0.0+). |
There was a problem hiding this comment.
🎨 SUGGESTED: Changelog taxonomy is still stale — omits stale-reference.
Details and fix
The "Fix stale finding taxonomy" commit updated both SKILL.md call sites to the four tokens consulting-adrs actually emits (conflict / gap / stale-reference / aligned, per plugins/bitwarden-planning-tools/README.md:15), but this changelog line still describes three.
| - `bitwarden-security-context`, `reviewing-security-architecture`, and `threat-modeling` now perform the ADR-alignment check by invoking `Skill(bitwarden-planning-tools:consulting-adrs)` instead of reading a local `references/adr-alignment.md`. Removed that reference file; the shared rule now lives in the `consulting-adrs` skill, which returns structured conflict/gap/aligned findings with cited ADRs. Requires the `bitwarden-planning-tools` plugin (1.0.0+). | |
| - `bitwarden-security-context`, `reviewing-security-architecture`, and `threat-modeling` now perform the ADR-alignment check by invoking `Skill(bitwarden-planning-tools:consulting-adrs)` instead of reading a local `references/adr-alignment.md`. Removed that reference file; the shared rule now lives in the `consulting-adrs` skill, which returns structured conflict/gap/stale-reference/aligned findings with cited ADRs. Requires the `bitwarden-planning-tools` plugin (1.0.0+). |
|
Demoting back to draft until potential impact on code review process is evaluated. |
d487c03 to
00f6b90
Compare
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/stale-reference/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.