Skip to content

Commit 6ec3d5a

Browse files
committed
chore: port reviewing-changes skill skeleton to .claude/
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.
1 parent 15ab4ca commit 6ec3d5a

18 files changed

Lines changed: 3066 additions & 0 deletions

.claude/CLAUDE.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,35 @@ Four layers; dependencies point strictly downward:
6767
4. **Bindings**`bitwarden-uniffi` (Swift/Kotlin) and `bitwarden-wasm-internal`
6868
(TypeScript/JavaScript). Thin bindings only — no business logic.
6969

70+
## Code Review
71+
72+
For any code-review task (review PR / review changes / review this code / look at a diff), use the
73+
repo-local **`reviewing-changes`** skill (`.claude/skills/reviewing-changes/SKILL.md`). It detects the
74+
change type and loads the matching checklist (`checklists/*.md`) and reference sheets
75+
(`reference/*.md`) for that kind of change. Treat this skill as the entry point for review work rather
76+
than reviewing ad hoc.
77+
78+
The enforceable review rules live in the path-scoped `.claude/rules/` files (auto-loaded when you
79+
touch matching files): `code-review.md` (cross-cutting power-user rules, with severities), alongside
80+
the topic files `crypto.md`, `rust-conventions.md`, `bindings.md`, and `generated-api-crates.md`.
81+
82+
**Multi-agent review — forward the guidelines.** When a review runs in multi-agent mode, the
83+
dispatching agent MUST forward the full contents of this `.claude/CLAUDE.md` and the loaded
84+
`.claude/rules/` files (plus any near-file `README` / crate `CLAUDE.md`) to every review subagent.
85+
Under the `evaluation-standards` gate a style/quality finding is only postable if its rule is written
86+
in a file the subagent has loaded — a subagent that never received these rules cannot legitimately
87+
raise them. Do not assume a subagent inherited this context; pass it explicitly.
88+
89+
## Deep-Dive Index
90+
91+
_Forthcoming — populated in Phase C._ This section will map each change-area to the `/docs/<page>.md`
92+
deep-dive page the reviewer should read before reviewing that kind of change. The paths below are
93+
placeholders and are NOT live yet:
94+
95+
- crypto / FFI change → `docs/patterns/crypto-safe-module.md` (Phase C)
96+
- state / repository change → `docs/patterns/state-management.md` (Phase C)
97+
- WASM / UniFFI binding change → `docs/patterns/bindings.md` (Phase C)
98+
7099
## References
71100

72101
- [SDK architecture](https://contributing.bitwarden.com/architecture/sdk/) ·

.claude/prompts/review-code.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Use the `reviewing-changes` skill to review this pull request.
2+
3+
The PR branch is already checked out in the current working directory.

.claude/rules/code-review.md

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
paths:
3+
- "crates/**/*.rs"
4+
- "bitwarden_license/**/*.rs"
5+
---
6+
7+
# Code review rules
8+
9+
Enforceable review rules distilled from Bitwarden SDK power-user PR feedback, each tagged with its
10+
severity. Topic-specific rules also live in the sibling rules files (`crypto.md`,
11+
`rust-conventions.md`, `bindings.md`); these complement, not replace, them.
12+
13+
1. `[CRITICAL]` Never use `Vec<u8>` / `String` to carry keys or crypto material; use typed newtypes
14+
(e.g. `SymmetricKey`) so key material cannot be confused with ordinary bytes. (See `crypto.md`
15+
for the key-reference / hazmat rules this builds on.)
16+
2. `[IMPORTANT]` New functionality lives on a `Client` struct via extension traits
17+
(`client.vault() -> VaultClient`), not as standalone statics or free functions.
18+
3. `[CRITICAL]` New crypto operations and FFI exports require pinned test vectors — JSON vectors
19+
pinned in code; see the `create-testvectors` skill.
20+
4. `[CRITICAL]` Never re-expose low-level / hazmat crypto through a feature crate's public API (the
21+
crypto crates already must not expose it; this rule covers feature crates re-exporting it
22+
downstream — see `crypto.md`).
23+
5. `[IMPORTANT]` All public types need doc comments; do NOT suppress `missing_docs` crate-wide to
24+
silence the warning.
25+
6. `[IMPORTANT]` Use restrictive visibility by default (`pub(crate)` / `pub(super)`); a bare `pub`
26+
must be justified by a real cross-crate consumer.
27+
7. `[CRITICAL]` An IV / nonce must never be passed in from outside; generate it internally at the
28+
point of encryption.
29+
8. `[DEBT]` No untracked `TODO`s — every `TODO` must link a Jira ticket.
30+
9. `[SUGGESTED]` Public docs describe usage and interface only, not internals; avoid
31+
over-documentation that restates the implementation.
32+
10. `[IMPORTANT]` Use Cargo weak dependencies (`?`) for optional / licensed crates, so an optional
33+
feature does not implicitly pull a crate across the OSS boundary.
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
---
2+
name: reviewing-changes
3+
description: SDK-specific code review checklist and Rust/crate/crypto/FFI/WASM pattern validation for the Bitwarden internal SDK — use this for any review task, even if the user doesn't explicitly ask for a "checklist". Detects change type automatically and loads the right review strategy (feature additions, bug fixes, UI refinements, refactoring, dependency updates, infrastructure). Triggered by "review", "code review", "review this code", "review PR", "review changes", "check this code", code review requests on Rust crate/crypto/FFI/UniFFI/WASM files, or any time someone asks to look at a diff, PR, or code changes in bitwarden/sdk-internal.
4+
---
5+
6+
# Reviewing Changes - Android Additions
7+
8+
This skill provides Android-specific workflow additions that complement the base `bitwarden-code-reviewer` agent standards.
9+
10+
## Instructions
11+
12+
**IMPORTANT**: Work systematically through each step before providing feedback. Each checklist file includes structured thinking guidance for its review passes.
13+
14+
### Step 1: Retrieve Additional Details
15+
16+
Retrieve any additional information linked to the pull request using available tools (JIRA MCP, GitHub API).
17+
18+
If pull request title and message do not provide enough context, request additional details from the reviewer:
19+
- Link a JIRA ticket
20+
- Associate a GitHub issue
21+
- Link to another pull request
22+
- Add more detail to the PR title or body
23+
24+
**Android metadata checks** — flag as ❓ if any of these are missing:
25+
- PR includes `*Screen.kt` or Composable changes but has no screenshots
26+
- PR adds new `ViewModel` or `Repository` but has no test plan or test file changes
27+
28+
### Step 2: Detect Change Type with Android Refinements
29+
30+
Use the base change type detection from the agent, with Android-specific refinements:
31+
32+
**Android-specific patterns:**
33+
- **Feature Addition**: New `ViewModel`, new `Repository`, new `@Composable` functions, new `*Screen.kt` files
34+
- **UI Refinement**: Changes only in `*Screen.kt`, `*Composable.kt`, `ui/` package files
35+
- **Infrastructure**: Changes to `.github/workflows/`, `gradle/`, `build.gradle.kts`, `libs.versions.toml`
36+
- **Dependency Update**: Changes only to `libs.versions.toml` or `build.gradle.kts` with version bumps
37+
38+
### Step 3: Load Appropriate Checklist
39+
40+
Based on detected type, read the relevant checklist file:
41+
42+
- **Dependency Update**`checklists/dependency-update.md` (expedited review)
43+
- **Bug Fix**`checklists/bug-fix.md` (focused review)
44+
- **Feature Addition**`checklists/feature-addition.md` (comprehensive review)
45+
- **UI Refinement**`checklists/ui-refinement.md` (design-focused review)
46+
- **Refactoring**`checklists/refactoring.md` (pattern-focused review)
47+
- **Infrastructure**`checklists/infrastructure.md` (tooling-focused review)
48+
49+
The checklist provides:
50+
- Multi-pass review strategy
51+
- Type-specific focus areas
52+
- What to check and what to skip
53+
- Structured thinking guidance
54+
55+
### Step 4: Execute Review Following Checklist
56+
57+
Follow the checklist's multi-pass strategy, thinking through each pass systematically.
58+
59+
### Step 5: Consult Android Reference Materials As Needed
60+
61+
Load reference files only when needed for specific questions:
62+
63+
- **Re-reviews** → invoke `reviewing-incremental-changes` agent skill; scope to changed lines only, do not flag new issues in unchanged code
64+
- **Issue prioritization**`reference/priority-framework.md` (Critical vs Suggested vs Optional)
65+
- **Phrasing feedback**`reference/review-psychology.md` (questions vs commands, I-statements)
66+
- **Architecture questions**`reference/architectural-patterns.md` (MVVM, Hilt DI, module org, error handling)
67+
- **Security questions (quick reference)**`reference/security-patterns.md` (common patterns and anti-patterns)
68+
- **Security questions (comprehensive)** → (SDK `/docs` deep-dive — populated in Phase C; not yet present)
69+
- **Testing questions**`reference/testing-patterns.md` (unit tests, mocking, null safety)
70+
- **UI questions**`reference/ui-patterns.md` (Compose patterns, theming)
71+
- **Style questions (project-specific)**`reference/style-patterns.md` (Kotlin rules enforced in review)
72+
- **Style questions (general)** → (SDK `/docs` deep-dive — populated in Phase C; not yet present)
73+
74+
## Core Principles
75+
76+
- **Priority order**: Security → Correctness → Breaking Changes → Performance → Maintainability
77+
- **Appropriate depth**: Match review rigor to change complexity and risk
78+
- **Specific references**: Always use `file:line_number` format for precise location
79+
- **Actionable feedback**: Say what to do and why, not just what's wrong
80+
- **Efficient reviews**: Use multi-pass strategy, skip what's not relevant
81+
- **Android patterns**: Validate MVVM, Hilt DI, Compose conventions, Kotlin idioms
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# Bug Fix Review Checklist
2+
3+
## Multi-Pass Strategy
4+
5+
### First Pass: Understand the Bug
6+
7+
**1. Understand root cause:**
8+
- What was the broken behavior?
9+
- What caused it?
10+
- How does this fix address the root cause?
11+
12+
**2. Assess scope:**
13+
- How many files changed?
14+
- Is this a targeted fix or broader refactoring?
15+
- Does this affect multiple features?
16+
17+
**3. Check for side effects:**
18+
- Could this break other features?
19+
- Are there edge cases not considered?
20+
21+
### Second Pass: Verify the Fix
22+
23+
**4. Code changes:**
24+
- Does the fix make sense?
25+
- Is it the simplest solution?
26+
- Any unnecessary changes included?
27+
28+
**5. Testing:**
29+
- Is there a regression test?
30+
- Does test verify the bug is fixed?
31+
- Are edge cases covered?
32+
33+
**6. Related code:**
34+
- Same pattern in other places that might have same bug?
35+
- Should other similar code be fixed too?
36+
37+
## What to CHECK
38+
39+
**Root Cause Analysis**
40+
- Does the fix address the root cause or just symptoms?
41+
- Is the explanation in PR/commit clear?
42+
43+
**Regression Testing**
44+
- Is there a new test that would fail without this fix?
45+
- Does test cover the reported bug scenario?
46+
- Are related edge cases tested?
47+
48+
**Side Effects**
49+
- Could this break existing functionality?
50+
- Are there similar code paths that need checking?
51+
- Does this change behavior in unexpected ways?
52+
53+
**Fix Scope**
54+
- Is the fix appropriately scoped (not too broad, not too narrow)?
55+
- Are all instances of the bug fixed?
56+
- Any related bugs discovered during investigation?
57+
58+
## What to SKIP
59+
60+
**Full Architecture Review** - Unless fix reveals architectural problems
61+
**Comprehensive Testing Review** - Focus on regression tests, not entire test suite
62+
**Major Refactoring Suggestions** - Unless directly related to preventing similar bugs
63+
64+
## Red Flags
65+
66+
🚩 **No test for the bug** - How will we prevent regression?
67+
🚩 **Fix doesn't match root cause** - Is this fixing symptoms?
68+
🚩 **Broad changes beyond the bug** - Should this be split into separate PRs?
69+
🚩 **Similar patterns elsewhere** - Should those be fixed too?
70+
71+
## Key Questions to Ask
72+
73+
Use `reference/review-psychology.md` for phrasing:
74+
75+
- "Can we add a test that would fail without this fix?"
76+
- "I see this pattern in [other file] - does it have the same issue?"
77+
- "Is this fixing the root cause or masking the symptom?"
78+
- "Could this change affect [related feature]?"
79+
80+
## Prioritizing Findings
81+
82+
Use `reference/priority-framework.md` to classify findings as Critical/Important/Suggested/Optional.
83+
84+
## Output Format
85+
86+
See `examples/review-outputs.md` for the required output format and inline comment structure.
87+
88+
## Example Review
89+
90+
```markdown
91+
**Overall Assessment:** APPROVE
92+
93+
See inline comments for suggested improvements.
94+
```
95+
96+
**Inline comment examples:**
97+
98+
```
99+
**data/auth/BiometricRepository.kt:120** - SUGGESTED: Extract null handling
100+
101+
<details>
102+
<summary>Details</summary>
103+
104+
Root cause analysis: BiometricPrompt result was nullable but code assumed non-null, causing crash on cancellation (PM-12345).
105+
106+
Consider extracting null handling pattern:
107+
108+
\```kotlin
109+
private fun handleBiometricResult(result: BiometricPrompt.AuthenticationResult?): AuthResult {
110+
return result?.let { AuthResult.Success(it) } ?: AuthResult.Cancelled
111+
}
112+
\```
113+
114+
This pattern could be reused if we add other biometric auth points.
115+
</details>
116+
```
117+
118+
```
119+
**app/auth/BiometricViewModel.kt:89** - SUGGESTED: Add regression test
120+
121+
<details>
122+
<summary>Details</summary>
123+
124+
Add test for cancellation scenario to prevent regression:
125+
126+
\```kotlin
127+
@Test
128+
fun `when biometric cancelled then returns cancelled state`() = runTest {
129+
coEvery { repository.authenticate() } returns Result.failure(CancelledException())
130+
viewModel.onBiometricAuth()
131+
assertEquals(AuthState.Cancelled, viewModel.state.value)
132+
}
133+
\```
134+
135+
This prevents regression of the bug just fixed.
136+
</details>
137+
```

0 commit comments

Comments
 (0)