[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 2 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).
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. |
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.