chore: port reviewing-changes skill skeleton to .claude/ - #1311
chore: port reviewing-changes skill skeleton to .claude/#1311addisonbeck wants to merge 1 commit into
Conversation
addisonbeck
added
the
ai-review-vnext
Phase A of the SDK code-review system. Ports Android's proven repo-local .claude/ review skeleton into the SDK and codifies the top-10 power-user review rules into .claude/CLAUDE.md. Because the SDK already runs the identical shared _review-code.yml CI workflow that Android does, this repo-local tree gives the SDK Android-equivalent, steered code reviews with zero gh-actions or infra changes: the CI reviewer reads .claude/CLAUDE.md (now carrying the rules + a pointer to the reviewing-changes skill) and the skill auto-activates via its description. The subagent-forwarding instruction added to CLAUDE.md closes the gap where multi-agent review subagents did not inherit the repo rules -- findings are only postable under the evaluation-standards gate if the rule is in a loaded file. New tree: - .claude/prompts/review-code.md -- generic stub delegating to skill - .claude/skills/reviewing-changes/SKILL.md -- skill orchestrator - .claude/skills/reviewing-changes/checklists/ -- 6 change-type checklists (Android-flavored intentionally; Rust-ified in Phase B) - .claude/skills/reviewing-changes/reference/ -- 7 reference sheets - .claude/skills/reviewing-changes/examples/review-outputs.md Checklist/reference content remains Android-flavored: Phase A is structural porting + rule codification. Phase B Rust-ifies content; Phase C populates the Deep-Dive Index.
761a576 to
6ec3d5a
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Phase A port of the Android Code Review Details
PR Metadata Assessment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # Reviewing Changes - Android Additions | ||
|
|
||
| This skill provides Android-specific workflow additions that complement the base `bitwarden-code-reviewer` agent standards. |
There was a problem hiding this comment.
Details and fix
The frontmatter description advertises "Rust/crate/crypto/FFI/WASM pattern validation" and triggers on any review task, while the body it loads says this is an Android skill and closes with "Android patterns: Validate MVVM, Hilt DI, Compose conventions, Kotlin idioms" (line 81). Step 2 keys change-type detection off *Screen.kt, ViewModel, libs.versions.toml, and build.gradle.kts — none of which exist here.
The commit message explains the Android flavor is intentional for Phase A, but that context is not in any file the reviewer loads, and .claude/CLAUDE.md now points every review task at this skill. Between this merge and Phase B, CI reviews of Rust PRs are steered by Kotlin/Compose guidance.
A cheap interim guard, right under the heading:
> **Phase A note:** the checklists and reference sheets below are ported verbatim from the Android
> repo and are being Rust-ified in Phase B. Treat the Android technology names (Compose, Hilt,
> MVVM, `*Screen.kt`, Gradle) as placeholders — map each to its SDK equivalent (WASM/UniFFI
> bindings, `Client` extension traits, crate layering, `Cargo.toml`) and do not raise findings that
> reference Android/Kotlin APIs.Alternatively, narrow the description so the skill does not auto-activate until Phase B lands.
| @@ -0,0 +1,90 @@ | |||
| # Security Patterns Quick Reference | |||
|
|
|||
| Quick reference for Bitwarden Android security patterns during code reviews. For comprehensive details, read `docs/ARCHITECTURE.md#security`. | |||
There was a problem hiding this comment.
docs/ARCHITECTURE.md / docs/STYLE_AND_BEST_PRACTICES.md across the new tree, but this repo has no docs/ directory.
Details and fix
There is no docs/ directory in sdk-internal, so every one of these pointers is dead. Counts per file:
| File | Refs |
|---|---|
reference/architectural-patterns.md |
8 |
reference/priority-framework.md |
3 |
reference/security-patterns.md |
3 |
reference/testing-patterns.md |
3 |
reference/ui-patterns.md |
3 |
checklists/feature-addition.md |
2 |
examples/review-outputs.md |
2 |
checklists/dependency-update.md, reference/review-psychology.md, reference/style-patterns.md |
1 each |
Two failure modes: the reviewer burns tool calls on Read misses, and several are inside example finding text (Reference: docs/ARCHITECTURE.md#security) that gets copied into posted comments, pointing SDK authors at a file that does not exist.
SKILL.md already annotates its own dangling pointers with "(SDK /docs deep-dive — populated in Phase C; not yet present)". Applying the same annotation here — or stripping the Reference: lines until Phase C populates the deep-dive index — would keep Phase A self-consistent.
| paths: | ||
| - "crates/**/*.rs" | ||
| - "bitwarden_license/**/*.rs" |
There was a problem hiding this comment.
Cargo.toml, so rule 10 never loads for the PRs it targets.
Details and fix
The scope is *.rs only, and .claude/CLAUDE.md states rules "load automatically when you touch matching files". Rule 10 ("Use Cargo weak dependencies (?) for optional / licensed crates") is a manifest rule — a dependency or feature-flag PR that touches only Cargo.toml / Cargo.lock will never load this file, which is exactly the case where the OSS/commercial boundary risk shows up.
paths:
- "crates/**/*.rs"
- "bitwarden_license/**/*.rs"
- "**/Cargo.toml"Also worth noting: .claude/CLAUDE.md describes this file as "cross-cutting power-user rules", which reads broader than the *.rs scope actually delivers.
| ## Optional (Acknowledge But Don't Require) | ||
|
|
||
| Note good practices to reinforce positive patterns. Keep these **brief** - list only, no elaboration. | ||
|
|
||
| ### Good Practices | ||
|
|
||
| **Format**: Simple bullet list, no explanation | ||
|
|
||
| ```markdown | ||
| ## Good Practices | ||
| - Proper Hilt DI usage throughout | ||
| - Comprehensive unit test coverage | ||
| - Clear separation of concerns | ||
| - Well-documented public APIs | ||
| ``` | ||
|
|
||
| **Don't do this** (too verbose): | ||
| ```markdown | ||
| ## Good Practices |
There was a problem hiding this comment.
♻️ DEBT: This "Good Practices" guidance contradicts examples/review-outputs.md, which lists positive-feedback sections as an anti-pattern.
Details and fix
Two conflicting instructions ship in the same skill:
- Here (and
reference/review-psychology.md, "Positive Feedback") the reviewer is told to emit a## Good Practicesbullet list. examples/review-outputs.mdsays "No 'Good Practices' or 'Action Items' sections" and "No positive commentary sections", matching the Bitwarden reviewer standard that forbids praise-only output.
Same conflict on comment shape: the examples in this file and in review-psychology.md use **file:line** - CRITICAL: … with details fully visible, while examples/review-outputs.md requires ❌ **CRITICAL**: … with everything inside <details> and calls the uncollapsed form an anti-pattern. checklists/infrastructure.md's "Example Review" also uses the multi-section summary that review-outputs.md rejects.
Whichever file the reviewer happens to load determines the output format. Suggest dropping the Optional/Good Practices sections and reformatting the inline examples in priority-framework.md, review-psychology.md, and the checklists to the review-outputs.md format, so there is one source of truth.
|
|
||
| Load reference files only when needed for specific questions: | ||
|
|
||
| - **Re-reviews** → invoke `reviewing-incremental-changes` agent skill; scope to changed lines only, do not flag new issues in unchanged code |
There was a problem hiding this comment.
♻️ DEBT: reviewing-incremental-changes is not available in this repo's review setup, so the re-review instruction silently no-ops.
Details and fix
The skills exposed to the SDK reviewer come from bitwarden-code-review, bitwarden-security-engineer, and claude-config-validator, plus repo-local .claude/skills/ — none of them provide reviewing-incremental-changes. Same for bitwarden-code-reviewer (line 8) and the evaluation-standards gate cited in the .claude/CLAUDE.md addition; those names come from the Android setup.
Since the "scope to changed lines only, do not flag new issues in unchanged code" guidance is the valuable part, inlining it here rather than delegating to an unavailable skill would make it actually take effect.
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1311 +/- ##
=======================================
Coverage 85.86% 85.86%
=======================================
Files 493 493
Lines 70931 70931
=======================================
Hits 60906 60906
Misses 10025 10025 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-37516
📔 Objective
🚨 Breaking Changes