Implement reusable Claude code review workflow (#6072)

Co-authored-by: Patrick Honkonen <phonkonen@bitwarden.com>
2025-12-05 18:46:49 -06:00 · 2025-10-27 15:19:37 +01:00
parent 51c23ec464
commit 52304a266e
6 changed files with 288 additions and 0 deletions
--- a/.claude/CLAUDE.md
+++ b/.claude/CLAUDE.md
@@ -0,0 +1,105 @@
+# Claude Guidelines
+
+Core directives for maintaining code quality and consistency in the Bitwarden Android project.
+
+## Core Directives
+
+**You MUST follow these directives at all times.**
+
+1. **Adhere to Architecture**: All code modifications MUST follow patterns in `docs/ARCHITECTURE.md`
+2. **Follow Code Style**: ALWAYS follow `docs/STYLE_AND_BEST_PRACTICES.md`
+3. **Error Handling**: Use Result types and sealed classes per architecture guidelines
+4. **Best Practices**: Follow Kotlin idioms (immutability, appropriate data structures, coroutines)
+5. **Document Everything**: All public APIs require KDoc documentation
+6. **Dependency Management**: Use Hilt DI patterns as established in the project
+7. **Use Established Patterns**: Leverage existing components before creating new ones
+8. **File References**: Use file:line_number format when referencing code
+
+## Code Quality Standards
+
+### Module Organization
+
+**Core Library Modules:**
+- **`:core`** - Common utilities and managers shared across multiple modules
+- **`:data`** - Data sources, database, data repositories
+- **`:network`** - Networking interfaces, API clients, network utilities
+- **`:ui`** - Reusable Bitwarden Composables, theming, UI utilities
+
+**Application Modules:**
+- **`:app`** - Password Manager application, feature screens, ViewModels, DI setup
+- **`:authenticator`** - Authenticator application for 2FA/TOTP code generation
+
+**Specialized Library Modules:**
+- **`:authenticatorbridge`** - Communication bridge between :authenticator and :app
+- **`:annotation`** - Custom annotations for code generation (Hilt, Room, etc.)
+- **`:cxf`** - Android Credential Exchange (CXF/CXP) integration layer
+
+### Patterns Enforcement
+
+- **MVVM + UDF**: ViewModels with StateFlow, Compose UI
+- **Hilt DI**: Interface injection, @HiltViewModel, @Inject constructor
+- **Testing**: JUnit 5, MockK, Turbine for Flow testing
+- **Error Handling**: Sealed Result types, no throws in business logic
+
+## Security Requirements
+
+**Every change must consider:**
+- Zero-knowledge architecture preservation
+- Proper encryption key handling (Android Keystore)
+- Input validation and sanitization
+- Secure data storage patterns
+- Threat model implications
+
+## Workflow Practices
+
+### Before Implementation
+
+1. Read relevant architecture documentation
+2. Search for existing patterns to follow
+3. Identify affected modules and dependencies
+4. Consider security implications
+
+### During Implementation
+
+1. Follow existing code style in surrounding files
+2. Write tests alongside implementation
+3. Add KDoc to all public APIs
+4. Validate against architecture guidelines
+
+### After Implementation
+
+1. Ensure all tests pass
+2. Verify compilation succeeds
+3. Review security considerations
+4. Update relevant documentation
+
+## Anti-Patterns
+
+**Avoid these:**
+- Creating new patterns when established ones exist
+- Exception-based error handling in business logic
+- Direct dependency access (use DI)
+- Mutable state in ViewModels (use StateFlow)
+- Missing null safety handling
+- Undocumented public APIs
+- Tight coupling between modules
+
+## Communication & Decision-Making
+
+Always clarify ambiguous requirements before implementing. Use specific questions:
+- "Should this use [Approach A] or [Approach B]?"
+- "This affects [X]. Proceed or review first?"
+- "Expected behavior for [specific requirement]?"
+
+Defer high-impact decisions to the user:
+- Architecture/module changes, public API modifications
+- Security mechanisms, database migrations
+- Third-party library additions
+
+## Reference Documentation
+
+Critical resources:
+- `docs/ARCHITECTURE.md` - Architecture patterns and principles
+- `docs/STYLE_AND_BEST_PRACTICES.md` - Code style guidelines
+
+**Do not duplicate information from these files - reference them instead.**
--- a/.claude/prompts/review-code.md
+++ b/.claude/prompts/review-code.md
@@ -0,0 +1,20 @@
+Use the `reviewing-changes` skill to review this pull request.
+
+The PR branch is already checked out in the current working directory.
+
+Provide a comprehensive review including:
+
+- Summary of changes since last review
+- Critical issues found (be thorough)
+- Suggested improvements (be thorough)
+- Good practices observed (be concise - list only the most notable items without elaboration)
+- Action items for the author
+- Leverage collapsible <details> sections where appropriate for lengthy explanations or code snippets
+
+When reviewing subsequent commits:
+
+- Track status of previously identified issues (fixed/unfixed/reopened)
+- Identify NEW problems introduced since last review
+- Note if fixes introduced new issues
+
+IMPORTANT: Be comprehensive about issues and improvements. For good practices, be brief - just note what was done well without explaining why or praising excessively.
--- a/.claude/skills/reviewing-changes/SKILL.md
+++ b/.claude/skills/reviewing-changes/SKILL.md
@@ -0,0 +1,110 @@
+---
+name: reviewing-changes
+description: Performs comprehensive code reviews for Bitwarden Android projects, verifying architecture compliance, style guidelines, compilation safety, test coverage, and security requirements. Use when reviewing pull requests, checking commits, analyzing code changes, verifying Bitwarden coding standards, evaluating MVVM patterns, checking Hilt DI usage, reviewing security implementations, or assessing test coverage. Automatically invoked by CI pipeline or manually for interactive code reviews.
+---
+
+# Reviewing Changes
+
+## Instructions
+
+Follow this process to review code changes for Bitwarden Android:
+
+### Step 1: Understand Context
+
+Start with high-level assessment of the change's purpose and approach. Read PR/commit descriptions and understand what problem is being solved.
+
+### Step 2: Verify Compliance
+
+Systematically check each area against Bitwarden standards documented in `CLAUDE.md`:
+
+1. **Architecture**: Follow patterns in `docs/ARCHITECTURE.md`
+   - MVVM + UDF (ViewModels with `StateFlow`, Compose UI)
+   - Hilt DI (interface injection, `@HiltViewModel`)
+   - Repository pattern and proper data flow
+
+2. **Style**: Adhere to `docs/STYLE_AND_BEST_PRACTICES.md`
+   - Naming conventions, code organization, formatting
+   - Kotlin idioms (immutability, null safety, coroutines)
+
+3. **Compilation**: Analyze for potential build issues
+   - Import statements and dependencies
+   - Type safety and null safety
+   - API compatibility and deprecation warnings
+   - Resource references and manifest requirements
+
+4. **Testing**: Verify appropriate test coverage
+   - Unit tests for business logic and utility functions
+   - Integration tests for complex workflows
+   - UI tests for user-facing features when applicable
+   - Test coverage for edge cases and error scenarios
+
+5. **Security**: Given Bitwarden's security-focused nature
+   - Proper handling of sensitive data
+   - Secure storage practices (Android Keystore)
+   - Authentication and authorization patterns
+   - Data encryption and decryption flows
+   - Zero-knowledge architecture preservation
+
+### Step 3: Document Findings
+
+Identify specific violations with `file:line_number` references. Be precise about locations.
+
+### Step 4: Provide Recommendations
+
+Give actionable recommendations for improvements. Explain why changes are needed and suggest specific solutions.
+
+### Step 5: Flag Critical Issues
+
+Highlight issues that must be addressed before merge. Distinguish between blockers and suggestions.
+
+### Step 6: Acknowledge Quality
+
+Note well-implemented patterns (briefly, without elaboration). Keep positive feedback concise.
+
+## Review Anti-Patterns (DO NOT)
+
+- Be nitpicky about linter-catchable style issues
+- Review without understanding context - ask for clarification first
+- Focus only on new code - check surrounding context for issues
+- Request changes outside the scope of this changeset
+
+## Examples
+
+### Good Review Format
+
+```markdown
+## Summary
+This PR adds biometric authentication to the login flow, implementing MVVM pattern with proper state management.
+
+## Critical Issues
+- `app/login/LoginViewModel.kt:45` - Mutable state exposed; use `StateFlow` instead of `MutableStateFlow`
+- `data/auth/BiometricRepository.kt:120` - Missing null safety check on `biometricPrompt` result
+
+## Suggested Improvements
+- Consider extracting biometric prompt logic to separate use case class
+- Add integration tests for biometric failure scenarios
+- `app/login/LoginScreen.kt:89` - Consider using existing `BitwardenButton` component
+
+## Good Practices
+- Proper Hilt DI usage throughout
+- Comprehensive unit test coverage
+- Clear separation of concerns
+
+## Action Items
+1. Fix mutable state exposure in `LoginViewModel`
+2. Add null safety check in `BiometricRepository`
+3. Consider adding integration tests for error flows
+```
+
+### What to Focus On
+
+**DO focus on:**
+- Architecture violations (incorrect patterns)
+- Security issues (data handling, encryption)
+- Missing tests for critical paths
+- Compilation risks (type safety, null safety)
+
+**DON'T focus on:**
+- Minor formatting (handled by linters)
+- Personal preferences without architectural basis
+- Issues outside the changeset scope
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -10,6 +10,11 @@
 # Actions and workflow changes.
 .github/ @bitwarden/dept-development-mobile

+# Claude related files
+.claude/ @bitwarden/team-ai-sme
+.github/workflows/respond.yml @bitwarden/team-ai-sme
+.github/workflows/review-code.yml @bitwarden/team-ai-sme
+
 # Auth
 # app/src/main/java/com/x8bit/bitwarden/data/auth @bitwarden/team-auth-dev
 # app/src/main/java/com/x8bit/bitwarden/ui/auth @bitwarden/team-auth-dev
--- a/.github/workflows/respond.yml
+++ b/.github/workflows/respond.yml
@@ -0,0 +1,28 @@
+name: Respond
+
+on:
+  issue_comment:
+    types: [created]
+  pull_request_review_comment:
+    types: [created]
+  issues:
+    types: [opened, assigned]
+  pull_request_review:
+    types: [submitted]
+
+permissions: {}
+
+jobs:
+  respond:
+    name: Respond
+    uses: bitwarden/gh-actions/.github/workflows/_respond.yml@main
+    secrets:
+      AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }}
+      AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }}
+      AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }}
+    permissions:
+      actions: read
+      contents: write
+      id-token: write
+      issues: write
+      pull-requests: write
--- a/.github/workflows/review-code.yml
+++ b/.github/workflows/review-code.yml
@@ -0,0 +1,20 @@
+name: Code Review
+
+on:
+  pull_request:
+    types: [opened, synchronize, reopened, ready_for_review]
+
+permissions: {}
+
+jobs:
+  review:
+    name: Review
+    uses: bitwarden/gh-actions/.github/workflows/_review-code.yml@main
+    secrets:
+      AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }}
+      AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }}
+      AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }}
+    permissions:
+      contents: read
+      id-token: write
+      pull-requests: write