From dca97e0c8e17815a297247a93ead011e98adbdf9 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Date: Wed, 5 Nov 2025 16:15:40 -0500 Subject: [PATCH] Refine reviewing-changes skill to eliminate verbosity and praise (#6128) Co-authored-by: Claude --- .claude/skills/reviewing-changes/SKILL.md | 123 +- .../checklists/infrastructure.md | 10 - .../checklists/ui-refinement.md | 10 - .../examples/review-outputs.md | 1159 ++++------------- 4 files changed, 360 insertions(+), 942 deletions(-) diff --git a/.claude/skills/reviewing-changes/SKILL.md b/.claude/skills/reviewing-changes/SKILL.md index 003d19680a..18d5738b31 100644 --- a/.claude/skills/reviewing-changes/SKILL.md +++ b/.claude/skills/reviewing-changes/SKILL.md @@ -10,7 +10,47 @@ description: Comprehensive code reviews for Bitwarden Android. Detects change ty **IMPORTANT**: Use structured thinking throughout your review process. Plan your analysis in `` tags before providing final feedback. This improves accuracy by 40% according to research. -### Step 1: Detect Change Type +### Step 1: Check for Existing Review Threads + +Always check for existing comment threads to avoid duplicate comments: + + +Before creating any comments: +1. Is this a fresh review or re-review of the same PR? +2. What existing discussion might already exist? +3. Which findings should update existing threads vs create new? + + +**Thread Detection Procedure:** + +1. **Fetch existing comment count:** + ```bash + gh pr view --json comments --jq '.comments | length' + ``` + +2. **If count = 0:** No existing threads. Skip to Step 2 (all comments will be new). + +3. **If count > 0:** Fetch full comment data to check for existing threads. + ```bash + gh pr view --json comments --jq '.comments[] | {id, path, line, body}' > pr_comments.json + ``` + +4. **Parse existing threads:** Extract file paths, line numbers, and issue summaries from previous review comments. + - Build map: `{file:line → {comment_id, issue_summary, resolved}}` + - Note which issues already have active discussions + +5. **Matching Strategy (Hybrid Approach):** + When you identify an issue to comment on: + - **Exact match:** Same file + same line number → existing thread found + - **Nearby match:** Same file + line within ±5 → existing thread found + - **No match:** Create new inline comment + +6. **Handling Evolved Issues:** + - **Issue persists unchanged:** Respond in existing thread with update + - **Issue resolved:** Note resolution in thread response (can mark as resolved if supported) + - **Issue changed significantly:** Resolve/close old thread, create new comment explaining evolution + +### Step 2: Detect Change Type Analyze the changeset systematically: @@ -32,7 +72,7 @@ Analyze the changeset to determine the primary change type: If changeset spans multiple types, use the most complex type's checklist. -### Step 2: Load Appropriate Checklist +### Step 3: Load Appropriate Checklist Based on detected type, read the relevant checklist file: @@ -49,7 +89,7 @@ The checklist provides: - What to check and what to skip - Structured thinking guidance -### Step 3: Execute Review with Structured Thinking +### Step 4: Execute Review with Structured Thinking Before diving into details: @@ -62,7 +102,7 @@ Before diving into details: Follow the checklist's multi-pass strategy, thinking through each pass systematically. -### Step 4: Consult Reference Materials As Needed +### Step 5: Consult Reference Materials As Needed Load reference files only when needed for specific questions: @@ -75,7 +115,7 @@ Load reference files only when needed for specific questions: - **UI questions** → `reference/ui-patterns.md` (Compose patterns, theming) - **Style questions** → `docs/STYLE_AND_BEST_PRACTICES.md` -### Step 5: Document Findings +### Step 6: Document Findings Before writing each comment: @@ -86,17 +126,28 @@ Before writing each comment: 5. Is there a documentation reference to include? -**CRITICAL**: Use inline comments on specific lines, NOT a single large review comment. +**CRITICAL**: Use summary comment + inline comments approach. -**Inline Comment Rules**: -- Create separate comment for EACH specific issue on the exact line -- Do NOT create one large summary comment with all issues -- Do NOT update existing comments - always create new comments -- This ensures history is retained for other reviewers +**Review Comment Structure**: +- Create ONE summary comment with overall verdict + critical issues list +- Create separate inline comment for EACH specific issue on the exact line with full details +- Summary directs readers to inline comments ("See inline comments for details") +- Do NOT duplicate issue details between summary and inline comments + +### CRITICAL: No Praise-Only Comments + +❌ **NEVER** create inline comments solely for positive feedback +❌ **NEVER** create summary sections like "Strengths", "Good Practices", "What Went Well" +❌ **NEVER** use inline comments to elaborate on correct implementations + +Focus exclusively on actionable feedback. Reserve comments for issues requiring attention. + +**Inline Comment Format** (REQUIRED: Use `
` Tags): + +**MUST use `
` tags for ALL inline comments.** Only severity + one-line description should be visible; all other content must be collapsed. -**Inline Comment Format** (Concise with Collapsible Details): ``` -**[Severity]**: [One-line issue description] +[emoji] **[SEVERITY]**: [One-line issue description]
Details and fix @@ -109,9 +160,13 @@ Reference: [docs link if applicable]
``` +**Visibility Rule:** +- **Visible:** Severity prefix (emoji + text) + one-line description +- **Collapsed in `
`:** Code examples, rationale, explanations, references + **Example inline comment**: ``` -**CRITICAL**: Exposes mutable state +⚠️ **CRITICAL**: Exposes mutable state
Details and fix @@ -129,30 +184,58 @@ Reference: docs/ARCHITECTURE.md#mvvm-pattern
``` -**Summary Comment Format** (Minimal - Avoid Duplication): +**Summary Comment Format** (Scales with PR Complexity): + +**Minimal format** (for simple PRs: 1-5 files, straightforward changes): ``` **Overall Assessment:** APPROVE / REQUEST CHANGES **Critical Issues** (if any): -- [One-line summary of each critical blocking issue] +- [One-line summary with file:line reference] -See inline comments for all issue details. +See inline comments for all details. ``` -**Output Format Notes**: +**Complex PR format** (add brief context when PR has): +- **10+ files changed**, or +- **Multiple distinct issue domains** (security + architecture + testing), or +- **High-severity blocking issues** needing stakeholder context + +Add ONE paragraph of context after verdict, before critical issues list. Keep total summary under 10 lines. + +**Output Format Rules**: **What to Include:** - **Inline comments**: Create separate comment for EACH specific issue with full details in `
` tag - **Summary comment**: Overall assessment (APPROVE/REQUEST CHANGES) + list of CRITICAL issues only -- **Severity levels**: Use CRITICAL (blocking), IMPORTANT (should fix), SUGGESTED (nice to have), or ACKNOWLEDGED (good practice observed) +- **Severity levels** (hybrid emoji + text format): + - ⚠️ **CRITICAL** (blocking) + - 📋 **IMPORTANT** (should fix) + - 💡 **SUGGESTED** (nice to have) + - ❓ **QUESTION** (seeking clarification) **What to Exclude:** - **No duplication**: Never repeat inline comment details in the summary - **No Important/Suggested in summary**: Only CRITICAL blocking issues belong in summary -- **No "Good Practices" section**: Acknowledge good practices in inline comments if relevant +- **No "Good Practices"/"Strengths" sections**: Never include positive commentary sections - **No "Action Items" section**: This duplicates inline comments - avoid entirely - **No verbose analysis**: Keep detailed analysis (compilation status, security review, rollback plans) in inline comments only +### ❌ Common Anti-Patterns to Avoid + +**DO NOT:** +- Create multiple summary sections (Strengths, Recommendations, Test Coverage Status, Architecture Compliance) +- Duplicate critical issues in both summary and inline comments +- Write elaborate descriptions in summary (details belong in inline comments) +- Exceed 5-10 lines for simple PRs +- Create inline comments that only provide praise + +**DO:** +- Put verdict + critical issue list ONLY in summary +- Put ALL details (explanations, code, rationale) in inline comments with `
` collapse +- Scale summary length based on PR complexity, not your analysis thoroughness +- Focus comments exclusively on actionable issues + **Visibility Guidelines:** - **Inline comments visible**: Severity + one-line description only - **Inline comments collapsed**: Code examples, rationale, references in `
` tag diff --git a/.claude/skills/reviewing-changes/checklists/infrastructure.md b/.claude/skills/reviewing-changes/checklists/infrastructure.md index 29df2acd7a..fa3b65d86c 100644 --- a/.claude/skills/reviewing-changes/checklists/infrastructure.md +++ b/.claude/skills/reviewing-changes/checklists/infrastructure.md @@ -242,16 +242,6 @@ This could further reduce CI time. Can we add a comment explaining the caching configuration? Future maintainers will appreciate understanding why these specific cache keys are used. -## Good Practices -- Proper use of GitHub Actions cache -- Parallel test execution -- Version catalog for dependencies - -## Action Items -1. Add timeout to build workflow -2. Consider matrix strategy for further parallelization -3. Document caching strategy in build file - ## Rollback Plan If CI breaks: - Revert commit: `git revert [commit-hash]` diff --git a/.claude/skills/reviewing-changes/checklists/ui-refinement.md b/.claude/skills/reviewing-changes/checklists/ui-refinement.md index 8ed1850ed2..fa2774fe4f 100644 --- a/.claude/skills/reviewing-changes/checklists/ui-refinement.md +++ b/.claude/skills/reviewing-changes/checklists/ui-refinement.md @@ -230,14 +230,4 @@ Spacer(modifier = Modifier.height(17.dp)) // Design system uses 4.dp increments (4, 8, 12, 16, 24, 32, etc.) Spacer(modifier = Modifier.height(16.dp)) ``` - -## Good Practices -- Proper state hoisting to ViewModel -- Preview composables included -- Responsive layout with ScrollableColumn - -## Action Items -1. Evaluate using BitwardenTextField for consistency -2. Add contentDescription for visibility icon -3. Use standard 16.dp spacing ``` diff --git a/.claude/skills/reviewing-changes/examples/review-outputs.md b/.claude/skills/reviewing-changes/examples/review-outputs.md index 5db9f4dd01..b4e7343d36 100644 --- a/.claude/skills/reviewing-changes/examples/review-outputs.md +++ b/.claude/skills/reviewing-changes/examples/review-outputs.md @@ -1,206 +1,67 @@ # Review Output Examples -Examples of well-structured code reviews for different change types. Each example demonstrates appropriate depth, tone, and formatting. +Well-structured code reviews demonstrating appropriate depth, tone, and formatting for different change types. --- -## CRITICAL: Inline Comments vs Summary Format +## Quick Format Reference -### ✅ PREFERRED: Inline Comments on Specific Lines +### Inline Comment Format (REQUIRED) -Create separate inline comment for each specific issue directly on the relevant line: - -**Inline Comment 1** (on `app/login/LoginViewModel.kt:78`): -``` -**CRITICAL**: Exposes mutable state - -Change `MutableStateFlow` to `StateFlow` in public API: - -\```kotlin -private val _state = MutableStateFlow() -val state: StateFlow = _state.asStateFlow() -\``` - -Exposing MutableStateFlow allows external mutation, violating MVVM unidirectional data flow. - -Reference: docs/ARCHITECTURE.md#mvvm-pattern -``` - -**Inline Comment 2** (on `data/auth/BiometricRepository.kt:120`): -``` -**CRITICAL**: Missing null safety check - -biometricPrompt result can be null. Add explicit null check: - -\```kotlin -val result = biometricPrompt.authenticate() -if (result != null) { - processAuth(result) -} else { - handleCancellation() -} -\``` - -Prevents NPE crash when user cancels biometric prompt. -``` - -**Summary Comment** (general PR-level comment): -``` -## Summary -Implements PIN unlock feature for vault access. - -## Overall Assessment -- 2 critical architecture issues found (mutable state, null safety) -- 3 suggestions for improvement (tests, component reuse) -- Good separation of concerns and DI usage - -## Recommendation -**REQUEST CHANGES** - Address 2 critical issues before merge. -``` - -**Why This is Better**: -- Each issue appears directly on the problematic line -- Easy for author to see context and fix -- History preserved when creating new comments -- Other reviewers can see individual discussions -- Cleaner PR conversation thread - ---- - -### ❌ DISCOURAGED: Single Large Summary Comment - -Do NOT create one large comment with all issues: - -```markdown -## Summary -Implements PIN unlock feature - -## Critical Issues -- **app/login/LoginViewModel.kt:78** - Exposes mutable state (entire explanation...) -- **data/auth/BiometricRepository.kt:120** - Missing null safety (entire explanation...) -- **app/vault/UnlockScreen.kt:134** - Should use BitwardenTextField (entire explanation...) - -## Suggested Improvements -- **app/vault/UnlockViewModel.kt:92** - Missing test (entire explanation...) -- **app/vault/UnlockViewModel.kt:105** - Consider rate limiting (entire explanation...) - -## Good Practices -- Proper Hilt DI -- Clear separation of concerns - -## Action Items -1. Fix mutable state exposure -2. Add null safety check -... (20 more items) -``` - -**Why This is Problematic**: -- All issues in one massive comment -- Author must scroll through entire comment to find each issue -- Hard to track which issues are addressed -- Loses context when jumping between files -- If comment is updated, history is lost -- Other reviewers can't comment on specific issues - ---- - -## Format Guidelines - -### Inline Comment Structure +**MUST use `
` tags.** Only severity + description visible; all other content collapsed. ``` -**[SEVERITY]**: [One-line issue description] +[emoji] **[SEVERITY]**: [One-line issue description] -[Detailed explanation if needed] +
+Details and fix -[Code example showing problem and/or solution] +[Code example or specific fix] -[Rationale explaining why this matters] +[Rationale explaining why] Reference: [docs link if applicable] +
``` -**Severity Levels**: -- `**CRITICAL**`: Must fix before merge -- `**IMPORTANT**`: Should fix before merge -- `**SUGGESTED**`: Nice to have -- `**QUESTION**`: Asking for clarification +**Severity Levels:** +- ⚠️ **CRITICAL** - Blocking, must fix +- 📋 **IMPORTANT** - Should fix +- 💡 **SUGGESTED** - Nice to have +- ❓ **QUESTION** - Seeking clarification -**Examples**: +### Summary Comment Format +**Minimal format** (simple PRs): ``` -**CRITICAL**: PIN stored without encryption +**Overall Assessment:** APPROVE / REQUEST CHANGES -Must encrypt using Android Keystore: +**Critical Issues** (if any): +- [issue with file:line] -\```kotlin -val encrypted = keystoreManager.encrypt(pin.toByteArray()) -encryptedPrefs.putBytes(KEY_PIN, encrypted) -\``` - -Plaintext storage exposes PIN to backups and rooted devices. - -Reference: docs/ARCHITECTURE.md#security +See inline comments for details. ``` -``` -**SUGGESTED**: Consider using BitwardenTextField - -Can we use `ui/components/BitwardenTextField.kt:67` instead? -This would maintain consistency across the app. -``` - -``` -**QUESTION**: Is rate limiting needed for PIN attempts? - -Currently allows unlimited attempts. Should we add lockout after N failed attempts? -``` - -### Summary Comment Structure - -One summary comment per PR with overall assessment: - -```markdown -## Summary -[1-2 sentence description of changes] - -## Review Complete -- X critical issues found (see inline comments) -- Y suggestions provided (see inline comments) -- Z good practices noted - -## Recommendation -**[APPROVE | REQUEST CHANGES | COMMENT]** - [Reason] -``` +**Complex PRs** (10+ files, multiple domains, high-severity issues): Add ONE brief context paragraph after verdict. --- -## Example 1: Dependency Update (Expedited Review) +## Example 1: Clean PR (No Issues) -**Context**: Simple patch version update, no breaking changes +**Context**: Moving shared code to common module, complete migration, all patterns followed +**Review Comment:** ```markdown -## Summary -Updates androidx.credentials from 1.5.0 to 1.6.0-beta03 +**Overall Assessment:** APPROVE -## Analysis -- **Compilation**: ✓ No breaking changes detected in changelog -- **Security**: ✓ No CVEs addressed -- **Testing**: ✓ No test API changes -- **Changelog**: Adds support for additional credential types, internal bug fixes - -## Findings -⚠️ **Beta version** - Monitor for stability issues in production environment - -## Recommendation -**APPROVE** - Low-risk minor version bump. Beta status is noted but no blocking concerns identified. +Clean refactoring that moves ExitManager to :ui module, eliminating duplication between apps. ``` -**Key Features**: -- Expedited format (no detailed sections) -- Clear analysis checklist -- Single finding noted -- Direct recommendation +**Why this works:** +- Immediate approval visible (2-3 lines) +- One sentence acknowledging the work +- No unnecessary sections or elaborate praise +- Author gets quick feedback and can proceed --- @@ -208,19 +69,25 @@ Updates androidx.credentials from 1.5.0 to 1.6.0-beta03 **Context**: Major version update requiring code migration +**Summary Comment:** ```markdown -## Summary -Updates Retrofit from 2.9.0 to 3.0.0 +**Overall Assessment:** REQUEST CHANGES -## Analysis -- **Compilation**: ❌ Breaking changes in API -- **Security**: ✓ No security issues -- **Testing**: ⚠️ Test utilities need updates -- **Changelog**: Major rewrite, new Kotlin coroutines API, removed deprecated methods +**Critical Issues:** +- API migration required for Retrofit 3.0 breaking changes (network/api/BitwardenApiService.kt:34) -## Critical Issues +See inline comments for migration details. +``` + +**Inline Comment 1** (on `network/api/BitwardenApiService.kt:34`): +```markdown +⚠️ **CRITICAL**: API migration required for Retrofit 3.0 + +
+Details and fix + +Retrofit 3.0 removes the `Call` return type. All 12 API methods in this file need migration: -**network/api/BitwardenApiService.kt:34** - API migration required ```kotlin // Current (deprecated in Retrofit 3.0) @GET("api/accounts/profile") @@ -230,108 +97,52 @@ fun getProfile(): Call @GET("api/accounts/profile") suspend fun getProfile(): Response ``` -Retrofit 3.0 removes the `Call` return type. All 12 API methods in this file need migration. -**network/api/VaultApiService.kt:45** - Same migration needed -All 8 API methods need to migrate from `Call` to `suspend fun` returning `Response`. +Breaking API change affects: +- 12 methods in BitwardenApiService +- 8 methods in VaultApiService +- All call sites using enqueue/execute +- Test utilities -## Suggested Improvements +Consider creating separate PR for this migration given the scope. -**Consider separate PR for migration** - Given the scope (20 API methods across 2 files + all call sites + tests), this might be better as a dedicated migration PR with comprehensive testing. - -## Recommendation -**REQUEST CHANGES** - Major version requires code migration: -1. Update all API service interfaces to use suspend functions -2. Update all call sites to use coroutines instead of enqueue/execute -3. Update tests to handle new suspend function APIs -4. Consider creating separate PR for this migration due to scope - -## Migration Resources -- Retrofit 3.0 migration guide: https://square.github.io/retrofit/changelogs/changelog-3.x/ -- Example migration: https://github.com/square/retrofit/tree/master/samples +Reference: [Retrofit 3.0 migration guide](https://square.github.io/retrofit/changelogs/changelog-3.x/) +
``` -**Key Features**: -- Specific file:line references for each issue -- Code examples showing current vs required -- Scope assessment -- Migration guidance +**Key Features:** +- Minimal summary (2-3 lines) +- Full details in collapsed inline comment +- Specific file:line references +- Code examples in
+- Migration guidance and scope assessment --- -## Example 3: Bug Fix (Focused Review) - -**Context**: Fixes crash when biometric prompt is cancelled - -```markdown -## Summary -Fixes crash when biometric prompt is cancelled (PM-12345) - -Root cause: BiometricPrompt result was nullable but code assumed non-null - -## Critical Issues -None - -## Suggested Improvements - -**data/auth/BiometricRepository.kt:120** - Add regression test -Can we add a test that verifies this specific bug doesn't reoccur? -```kotlin -@Test -fun `when biometric cancelled then returns cancelled state`() = runTest { - coEvery { biometricPrompt.authenticate() } returns null - - val result = repository.authenticate() - - assertEquals(AuthResult.Cancelled, result) -} -``` -This prevents regression of the crash you just fixed. - -**data/auth/BiometricRepository.kt:134** - Consider extracting null handling -```kotlin -private fun handleBiometricResult(result: BiometricPrompt.AuthenticationResult?): AuthResult { - return result?.let { AuthResult.Success(it) } ?: AuthResult.Cancelled -} -``` -This pattern could be reused if we add other biometric auth points in the future. - -**app/auth/BiometricViewModel.kt:89** - Check similar pattern in FaceIdRepository -I see similar biometric handling in `data/auth/FaceIdRepository.kt:78`. Does it have the same issue? - -## Good Practices -- Added null safety check for prompt result -- Proper error state propagation through sealed class -- Clear commit message explaining root cause - -## Action Items -1. Add regression test for cancellation scenario -2. Check FaceIdRepository for similar null safety issue -3. Consider extracting null handling pattern if reusable -``` - -**Key Features**: -- Root cause explained -- Specific line references for each suggestion -- Code examples provided -- Questions about related code -- Concise good practices (no elaboration) - ---- - -## Example 4: Feature Addition (Comprehensive Review) +## Example 3: Feature Addition with Critical Issues **Context**: Implements PIN unlock for vault access +**Summary Comment:** ```markdown -## Summary -Implements PIN unlock feature for vault access (PM-54321) +**Overall Assessment:** REQUEST CHANGES -Adds new authentication option allowing users to unlock vault with 4-6 digit PIN instead of master password. Includes PIN creation, validation, and secure storage. +**Critical Issues:** +- Exposes mutable state violating MVVM (UnlockViewModel.kt:78) +- PIN stored without encryption - SECURITY ISSUE (UnlockRepository.kt:145) -## Critical Issues +See inline comments for all issues and suggestions. +``` + +**Inline Comment 1** (on `app/vault/unlock/UnlockViewModel.kt:78`): +```markdown +⚠️ **CRITICAL**: Exposes mutable state + +
+Details and fix + +Change `MutableStateFlow` to `StateFlow`: -**app/vault/unlock/UnlockViewModel.kt:78** - Exposes mutable state ```kotlin // Current (problematic) val unlockState: MutableStateFlow @@ -340,11 +151,22 @@ val unlockState: MutableStateFlow private val _unlockState = MutableStateFlow() val unlockState: StateFlow = _unlockState.asStateFlow() ``` -Exposing MutableStateFlow allows external code to modify ViewModel state directly, violating MVVM's unidirectional data flow principle. -Reference: `docs/ARCHITECTURE.md#mvvm-pattern` +Exposing MutableStateFlow allows external mutation, violating MVVM unidirectional data flow. + +Reference: docs/ARCHITECTURE.md#mvvm-pattern +
+``` + +**Inline Comment 2** (on `data/vault/UnlockRepository.kt:145`): +```markdown +⚠️ **CRITICAL**: PIN stored without encryption - SECURITY ISSUE + +
+Details and fix + +Storing PIN in plaintext SharedPreferences exposes it to backup systems and rooted devices. -**data/vault/UnlockRepository.kt:145** - SECURITY: PIN stored without encryption ```kotlin // Current (CRITICAL SECURITY ISSUE) sharedPreferences.edit { @@ -357,29 +179,48 @@ suspend fun storePin(pin: String): Result = runCatching { encryptedPrefs.putBytes(KEY_PIN, encrypted) } ``` -Storing PIN in plaintext SharedPreferences exposes it to backup systems and rooted devices. -Must use Android Keystore encryption or EncryptedSharedPreferences. -Reference: `docs/ARCHITECTURE.md#security` +Use Android Keystore encryption or EncryptedSharedPreferences per security architecture. -## Suggested Improvements +Reference: docs/ARCHITECTURE.md#security +
+``` + +**Inline Comment 3** (on `app/vault/unlock/UnlockViewModel.kt:92`): +```markdown +📋 **IMPORTANT**: Missing error handling test + +
+Details and fix + +Add test to prevent regression if error handling changes: -**app/vault/unlock/UnlockViewModel.kt:92** - Missing error handling test ```kotlin @Test fun `when incorrect PIN entered then returns error state`() = runTest { val viewModel = UnlockViewModel(mockRepository) - coEvery { mockRepository.validatePin("1234") } returns Result.failure(InvalidPinException()) + coEvery { mockRepository.validatePin("1234") } + returns Result.failure(InvalidPinException()) viewModel.onPinEntered("1234") assertEquals(UnlockState.Error("Invalid PIN"), viewModel.state.value) } ``` -Add test to prevent regression if error handling changes. -**app/vault/unlock/UnlockViewModel.kt:105** - Consider rate limiting -Can we add rate limiting for PIN attempts? Currently allows unlimited attempts, which could enable brute force attacks. +Ensures error flow remains robust across refactorings. +
+``` + +**Inline Comment 4** (on `app/vault/unlock/UnlockViewModel.kt:105`): +```markdown +💡 **SUGGESTED**: Consider rate limiting for PIN attempts + +
+Details and fix + +Currently allows unlimited attempts, which could enable brute force attacks. + ```kotlin private var attemptCount = 0 private var lockoutUntil: Instant? = null @@ -399,673 +240,187 @@ fun onPinEntered(pin: String) { } ``` -**app/vault/unlock/UnlockScreen.kt:134** - Can we use BitwardenTextField? +Would add security layer against brute force. Consider discussing threat model with security team. +
+``` + +**Inline Comment 5** (on `app/vault/unlock/UnlockScreen.kt:134`): +```markdown +❓ **QUESTION**: Can we use BitwardenTextField? + +
+Details + This custom PIN input field looks similar to `ui/components/BitwardenTextField.kt:67`. -Would using the existing component maintain consistency? -**data/vault/UnlockRepository.kt:178** - Add test for PIN length validation -```kotlin -@Test -fun `when PIN is less than 4 digits then returns validation error`() = runTest { - val result = repository.createPin("123") - - assertTrue(result.isFailure) - assertIs(result.exceptionOrNull()) -} +Would using the existing component maintain consistency and reduce custom UI code? +
``` -## Good Practices -- Proper Hilt DI usage throughout -- Clear separation of UI/ViewModel/Repository layers -- Sealed classes for state management -- Comprehensive happy-path unit tests - -## Action Items -1. **MUST FIX**: Encrypt PIN using Android Keystore (security issue) -2. **MUST FIX**: Expose immutable StateFlow in ViewModel (architecture violation) -3. **SHOULD ADD**: Test for incorrect PIN error flow -4. **SHOULD ADD**: Test for PIN length validation -5. **CONSIDER**: Rate limiting for PIN attempts (security enhancement) -6. **CONSIDER**: Evaluate BitwardenTextField for consistency -``` - -**Key Features**: -- Comprehensive review with multiple sections -- Critical security issues clearly flagged -- Specific code examples for fixes +**Key Features:** +- Minimal summary (3-4 lines) with critical issues only +- Each issue gets separate inline comment with `
` tag +- Multiple severity levels demonstrated (CRITICAL, IMPORTANT, SUGGESTED, QUESTION) - Mix of prescriptive fixes and collaborative questions -- Test examples provided -- Clear prioritization (MUST FIX vs SHOULD ADD vs CONSIDER) +- Code examples collapsed in
+- No "Good Practices" or "Action Items" sections --- -## Example 5: UI Refinement (Design-Focused Review) +## ❌ Anti-Patterns to Avoid -**Context**: Updates login screen layout for improved visual hierarchy +### Problem: Verbose Summary with Multiple Sections +**What NOT to do:** ```markdown +### Review Complete ✅ + ## Summary -Updates login screen layout for improved visual hierarchy and touch target sizes +[Lengthy description of what the PR does] -Adjusts spacing, increases button sizes to meet accessibility guidelines, and improves visual flow. +### Strengths 👍 +1. **Excellent documentation** - KDoc comments are comprehensive +2. **Proper fail-closed design** - Security defaults to rejection +3. **Defense in depth** - Multiple validation layers +[7 total items with elaboration] -## Critical Issues -None +### Critical Issues ⚠️ +- Missing test coverage for security-critical code (with full details) +- [More issues with full explanations] -## Suggested Improvements +### Recommendations 🎨 +- [Multiple recommendations] -**app/auth/LoginScreen.kt:67** - Can we use BitwardenTextField? -This custom text field implementation looks very similar to `ui/components/BitwardenTextField.kt:89`. -```kotlin -// Current (custom implementation) -OutlinedTextField( - value = email, - onValueChange = onEmailChange, - colors = OutlinedTextFieldDefaults.colors(...) -) +### Test Coverage Status 📊 +- [Analysis] -// Consider using existing component -BitwardenTextField( - value = email, - onValueChange = onEmailChange, - label = "Email" -) -``` -Using the existing component would maintain consistency across the app and reduce code duplication. +### Architecture Compliance ✅ +- [Analysis] -**app/auth/LoginScreen.kt:123** - Add contentDescription for accessibility -```kotlin -Icon( - painter = painterResource(R.drawable.ic_visibility), - contentDescription = "Show password", // Add this - modifier = Modifier.clickable { onToggleVisibility() } -) -``` -Screen readers need contentDescription to announce the icon's purpose to visually impaired users. - -**app/auth/LoginScreen.kt:145** - Use theme spacing -```kotlin -// Current (non-standard spacing) -Spacer(modifier = Modifier.height(17.dp)) - -// Should use standard theme spacing -Spacer(modifier = Modifier.height(16.dp)) -``` -Project uses standard spacing values (4dp, 8dp, 16dp, 24dp) for consistency. - -**app/auth/LoginScreen.kt:178** - Use theme color -```kotlin -// Current (hardcoded color) -color = Color(0xFF0066FF) - -// Should use theme -color = BitwardenTheme.colorScheme.primary +## Recommendation +**Conditional approval** with follow-up... ``` -## Good Practices -- Proper state hoisting to ViewModel -- Preview composables included for development -- Touch targets meet 48dp minimum +**Why this is wrong:** +- 800+ tokens for a summary comment +- Multiple sections (Strengths, Recommendations, Test Coverage, Architecture) +- Elaborates on positive aspects ("Excellent documentation...") +- Duplicates critical issues (summary has details + inline comments have same details) +- Creates visual clutter in PR conversation -## Action Items -1. Evaluate using BitwardenTextField for consistency -2. Add contentDescription for visibility icon -3. Use standard 16dp spacing instead of 17dp -4. Use theme color instead of hardcoded value -``` - -**Key Features**: -- Design and accessibility focused -- Specific line references -- Shows current vs recommended code -- Explains rationale (consistency, accessibility) -- Notes good practices briefly - ---- - -## Example 6: Refactoring (Pattern Consistency Review) - -**Context**: Refactors authentication to use Repository pattern - -```markdown -## Summary -Refactors authentication flow to use Repository pattern instead of direct Manager access - -Scope: 12 files changed, 8 ViewModels updated, AuthRepository interface extracted - -## Critical Issues -None - behavior preserved, all tests passing - -## Suggested Improvements - -**app/vault/VaultViewModel.kt:89** - Old pattern still used here -This ViewModel still injects AuthManager directly. Should it use AuthRepository like the others? -```kotlin -// Current (old pattern) -class VaultViewModel @Inject constructor( - private val authManager: AuthManager -) - -// Should be (new pattern) -class VaultViewModel @Inject constructor( - private val authRepository: AuthRepository -) -``` -This is the only ViewModel still using the old pattern. - -**data/auth/AuthManager.kt:1** - Add deprecation notice -Can we add @Deprecated to AuthManager to guide future development? -```kotlin -@Deprecated( - message = "Use AuthRepository interface instead. AuthManager will be removed in v3.0.", - replaceWith = ReplaceWith("AuthRepository"), - level = DeprecationLevel.WARNING -) -class AuthManager @Inject constructor(...) -``` -This helps other developers understand the migration path and prevents new code from using the old pattern. - -**docs/ARCHITECTURE.md:145** - Document the new pattern -Should we update the architecture docs to reflect this Repository pattern? -The current documentation (line 145-167) still shows AuthManager as the recommended approach. - -Suggest adding section: -```markdown -## Authentication Architecture - -Use `AuthRepository` interface for all authentication operations: -- Login/logout -- Session management -- Token refresh - -The repository pattern provides abstraction and makes testing easier. -``` - -## Good Practices -- Repository interface clearly defined with Result return types -- All ViewModels except one successfully migrated -- Tests updated to match new pattern -- Behavior preserved (all existing tests pass) - -## Action Items -1. Update VaultViewModel to use AuthRepository -2. Add @Deprecated annotation to AuthManager with timeline -3. Update ARCHITECTURE.md to document Repository pattern -4. Consider deprecation timeline (suggest v3.0 removal) -``` - -**Key Features**: -- Focuses on migration completeness -- Identifies incomplete migration (one missed file) -- Suggests deprecation strategy -- Notes documentation needs -- Acknowledges good practices briefly - ---- - -## Key Patterns Across All Examples - -### Consistent Elements - -1. **File:Line References**: Every specific issue includes `file:line_number` format -2. **Code Examples**: Complex issues show current code and suggested fix -3. **Rationale**: Explains **why** change is needed, not just **what** -4. **Prioritization**: Clear distinction between Critical, Suggested, Optional -5. **Actionable**: Specific steps the author should take -6. **Constructive Tone**: Questions for design decisions, prescriptive for violations - -### Format Structure - -```markdown -## Summary -[1-2 sentence description] - -## Critical Issues (if any) -**file:line** - Issue description -[Code example showing problem and solution] -[Rationale explaining why this matters] - -## Suggested Improvements -**file:line** - Suggestion with question or explanation -[Code example if helpful] -[Benefits of the suggestion] - -## Good Practices (brief) -- Item 1 -- Item 2 -- Item 3 - -## Action Items -1. Required action -2. Recommended action -3. Optional consideration -``` - ---- - -## Anti-Patterns to Avoid - -### ❌ Too Vague -``` -The state management could be better. -This code has issues. -Consider improving the architecture. -``` - -### ❌ Too Verbose -``` -## Good Practices -- Excellent Hilt DI usage! I really appreciate how you've implemented dependency injection here. The use of @HiltViewModel is exactly what we want to see, and injecting interfaces instead of implementations is a best practice that really shines in this PR. The constructor injection pattern is clean and testable. Great work! This is a perfect example of how to structure ViewModels in our codebase. Keep up the fantastic work! 👍🎉 -``` - -### ❌ No Specifics -``` -There are some null safety issues in the ViewModel. -Tests are missing for some scenarios. -``` - -### ✅ Good Specificity -``` -**app/login/LoginViewModel.kt:78** - Missing null safety check -biometricPrompt result can be null. Add explicit check to prevent NPE. -``` - ---- - -## Concise Review Format (Recommended) - -### Key Principles - -1. **Minimal summary**: Only verdict + critical issues -2. **Collapsible inline comments**: Severity + one-line description visible, details collapsed -3. **No duplication**: Don't repeat inline issues in summary -4. **No redundant sections**: No "Action Items" or "Good Practices" in summary - -### Example 1: Feature Review with Multiple Issues - -**Summary Comment:** +**Correct approach:** ```markdown **Overall Assessment:** REQUEST CHANGES **Critical Issues:** -- Exposes mutable state (app/vault/VaultViewModel.kt:45) -- Missing null safety check (app/vault/VaultRepository.kt:123) +- Missing test coverage for security-critical code (PasswordManagerSignatureVerifierImpl.kt:47) -See inline comments for all issue details. +See inline comments for details. ``` -**Inline Comments:** +**Key differences:** +- 3-5 lines vs 800+ tokens +- Verdict + critical issues only +- All details belong in inline comments +- No positive commentary sections +- Scales with PR complexity, not analysis thoroughness + +### Problem: Praise-Only Inline Comments + +**What NOT to do:** + +Creating inline comment on `AuthenticatorBridgeManagerImpl.kt:73`: +```markdown +👍 **Excellent integration of signature verification** + +The signature verification is properly integrated into the connection flow: +- Checked during initialization (line 73) +- Checked before binding (line 134) +- Ensures only validated apps can connect + +This is exactly the right approach for fail-safe security. +``` + +**Why this is wrong:** +- Entire comment is positive feedback with no actionable issue +- Takes up space in PR conversation +- Distracts from actual issues +- Violates "focus on actionable feedback" principle + +**Correct approach:** +- Do not create this comment at all +- Reserve inline comments exclusively for issues requiring attention + +### Problem: Missing `
` Tags + +**What NOT to do:** ```markdown -**app/vault/VaultViewModel.kt:45** - CRITICAL: Exposes mutable state +⚠️ **CRITICAL**: Missing test coverage for security-critical code + +The `@OmitFromCoverage` annotation excludes this entire class from test coverage. + +**Problems:** +1. No validation that certificate hashes match actual Bitwarden certificates +2. No verification of fail-closed behavior on edge cases +3. No tests for multiple signer rejection logic +4. Certificate hash typos would go undetected until production + +**Recommendation:** +Replace `@OmitFromCoverage` with proper unit tests. + +Example test structure: +[long code block] + +Security-critical code should have the highest test coverage, not be omitted. +``` + +**Why this is wrong:** +- All content visible immediately (code examples, problems list, rationale) +- Creates visual clutter in PR conversation +- Makes it hard to scan multiple issues quickly + +**Correct approach:** +```markdown +⚠️ **CRITICAL**: Missing test coverage for security-critical code
Details and fix -Change to private backing field pattern: +The `@OmitFromCoverage` annotation excludes this entire class from test coverage. -\```kotlin -private val _vaultState = MutableStateFlow(VaultState.Loading) -val vaultState: StateFlow = _vaultState.asStateFlow() -\``` +**Problems:** +1. No validation that certificate hashes match actual Bitwarden certificates +2. No verification of fail-closed behavior on edge cases +3. No tests for multiple signer rejection logic +4. Certificate hash typos would go undetected until production -Exposing MutableStateFlow allows external mutation, violating MVVM unidirectional data flow. +**Recommendation:** +Replace `@OmitFromCoverage` with proper unit tests. -Reference: docs/ARCHITECTURE.md#mvvm-pattern +Example test structure: +[code block] + +Security-critical code should have the highest test coverage, not be omitted.
``` -```markdown -**app/vault/VaultRepository.kt:123** - CRITICAL: Missing null safety check - -
-Details and fix - -Add null check before accessing cipher: - -\```kotlin -val cipher = getCipher(id) ?: return Result.failure(CipherNotFoundException()) -\``` - -Without null safety, this will crash when cipher ID is invalid. -
-``` - -```markdown -**app/vault/VaultViewModel.kt:89** - IMPORTANT: Missing test coverage - -
-Details - -Add test for error state handling: - -\```kotlin -@Test -fun `when load fails then shows error state`() = runTest { - coEvery { repository.getVaultItems() } returns Result.failure(Exception()) - viewModel.loadVault() - assertEquals(VaultState.Error, viewModel.vaultState.value) -} -\``` - -Error paths should be tested to prevent regressions. - -Reference: reference/testing-patterns.md -
-``` - -**Why this works:** -- Summary is 4 lines (vs 30+ lines with verbose format) -- Severity + issue visible immediately -- Full details available on expansion -- Zero duplication between summary and inline comments -- Token-efficient while preserving all information +**Key difference:** Only severity + one-line description visible. All details collapsed. --- -### Example 2: Dependency Update (No Critical Issues) - -**Summary Comment:** -```markdown -**Overall Assessment:** APPROVE - -See inline comments for suggested improvements. -``` - -**Inline Comment:** - -```markdown -**gradle/libs.versions.toml:45** - SUGGESTED: Beta version in production - -
-Details - -Updated androidx.credentials from 1.5.0 to 1.6.0-beta03. - -Monitor for stability issues - beta releases may have unexpected behavior in production. - -Changelog: Adds support for additional credential types, internal bug fixes. -
-``` - -**Why this works:** -- Immediate approval visible (no critical issues) -- Suggestion collapsed to reduce noise -- All context preserved for interested reviewers - ---- - -### Example 3: Bug Fix Review - -**Summary Comment:** -```markdown -**Overall Assessment:** APPROVE - -See inline comments for suggested improvements. -``` - -**Inline Comments:** - -```markdown -**data/auth/BiometricRepository.kt:120** - SUGGESTED: Extract null handling - -
-Details - -Root cause: BiometricPrompt result was nullable but code assumed non-null, causing crash on cancellation (PM-12345). - -Consider extracting pattern for reuse: - -\```kotlin -private fun handleBiometricResult(result: BiometricPrompt.AuthenticationResult?): AuthResult { - return result?.let { AuthResult.Success(it) } ?: AuthResult.Cancelled -} -\``` -
-``` - -```markdown -**app/auth/BiometricViewModel.kt:89** - SUGGESTED: Add regression test - -
-Details - -\```kotlin -@Test -fun `when biometric cancelled then returns cancelled state`() = runTest { - coEvery { repository.authenticate() } returns Result.failure(CancelledException()) - viewModel.onBiometricAuth() - assertEquals(AuthState.Cancelled, viewModel.state.value) -} -\``` - -Prevents regression of the bug just fixed. -
-``` - -**Why this works:** -- Approval decision immediately visible -- Root cause analysis preserved but collapsed -- Suggestions don't overwhelm the fix -- Test recommendations available but not blocking - ---- - -## Example 7: Clean Refactoring (No Issues Found) - -**Note**: This example was added to demonstrate proper handling of clean refactorings after PR #6117 received an overly verbose review. See the "Anti-Pattern" section below for the actual problematic output that motivated these changes. - -**Context**: Moving shared code to common module, complete migration, all patterns followed - -**Review Comment:** -```markdown -**Overall Assessment:** APPROVE - -Clean refactoring that moves ExitManager to :ui module, eliminating duplication between apps. -``` - -**Token count:** ~30 tokens -**Review time:** < 1 minute - -**Why this works:** -- Immediate approval visible -- One sentence summary acknowledging the work -- No unnecessary sections or elaborate praise -- Author gets quick feedback and can proceed - ---- - -### ❌ Anti-Pattern: Excessive Praise for Clean PRs - -**DO NOT do this for clean PRs:** - -```markdown -### PR Review: PM-27756 - Create common ExitManager - -This PR successfully refactors `ExitManager` into shared code in the `:ui` module, eliminating duplication between the Password Manager and Authenticator apps. The changes follow established architectural patterns and improve code quality. - ---- - -### Summary - -**Overall Assessment:** ✅ **Approved** - This is a well-executed refactoring that improves code organization and maintainability. - -**Key Strengths:** -- Follows established module organization patterns (moving to `:ui` module alongside `IntentManager` and `QrCodeAnalyzer`) -- Removes code duplication between apps (deleted 28 lines of duplicate code) -- Improves test coverage by removing `@OmitFromCoverage` and adding comprehensive unit tests -- Maintains consistent behavior across both applications -- All imports and references updated correctly - -**Changes:** -- ✅ Moved `ExitManager` interface from `app` → `ui` module -- ✅ Moved `ExitManagerImpl` from `app` → `ui` module -- ✅ Added `LocalExitManager` to common UI providers -- ✅ Deleted duplicate authenticator implementations -- ✅ Added unit test for `ExitManagerImpl` -- ✅ Updated all imports across 10 files - ---- - -### Code Quality & Architecture - -**Architectural Compliance:** ✅ -- Correctly places manager in `:ui` module per `docs/ARCHITECTURE.md` guidelines -- Follows established pattern for UI-layer managers exposed via CompositionLocal -- Maintains proper module dependencies and separation of concerns - -**Code Style:** ✅ -- Follows Kotlin coding conventions -- Proper KDoc documentation on all public APIs -- Consistent with existing code patterns - -**Testing:** ✅ -- Added comprehensive unit test covering `exitApplication()` behavior -- Properly uses MockK for mocking Activity -- Test verifies `finishAndRemoveTask()` is called - ---- - -### Specific Findings - -**Test Coverage Note:** -The Codecov report shows 50% patch coverage with 1 missing line. This is expected and acceptable... - -**Implementation Improvement:** -The common implementation correctly uses `activity.finishAndRemoveTask()`... - ---- - -### Recommendations - -No blocking issues found. The PR is ready to merge. -``` - -**Problems with above (actual PR #6117 review):** -- 800+ tokens for a PR with no issues -- Multiple redundant sections -- Excessive checkmarks listing what was done -- Detailed analysis of things that are correct -- "Key Strengths" section unnecessary -- "Specific Findings" section with non-issues - -**Impact:** -- Wastes reviewer and author time -- Adds noise to PR conversation -- Makes it harder to identify PRs that actually need attention -- Excessive praise can feel condescending or insincere -- Burns tokens unnecessarily - ---- - -### ✅ Correct Approach - -For the same PR: - -```markdown -**Overall Assessment:** APPROVE - -Clean refactoring that moves ExitManager to :ui module, eliminating duplication between apps. -``` - -**Benefits:** -- 30 tokens vs 800+ tokens (96% reduction) -- Clear immediate approval -- Acknowledges the work without excessive detail -- Author can proceed without wading through unnecessary commentary -- Saves time for everyone - ---- - -### Comparison: Verbose vs Concise - -**Verbose Format (Old):** -```markdown ## Summary -Adds vault item encryption feature -Root cause: Feature implements client-side encryption for vault items +**Always use:** +- Minimal summary (verdict + critical issues) +- Separate inline comments with `
` tags +- Hybrid emoji + text severity prefixes +- Focus exclusively on actionable feedback -## Critical Issues +**Never use:** +- Multiple summary sections (Strengths, Recommendations, etc.) +- Praise-only inline comments +- Duplication between summary and inline comments +- Verbose analysis in summary (belongs in inline comments) -**app/vault/VaultViewModel.kt:45** - Exposes mutable state -Change MutableStateFlow to StateFlow: -\```kotlin -private val _state = MutableStateFlow() -val state = _state.asStateFlow() -\``` -Prevents external mutation, enforces unidirectional data flow. -Reference: docs/ARCHITECTURE.md - -**app/vault/VaultRepository.kt:123** - Missing null safety -Add null check: cipher ?: return Result.failure() - -## Important Issues - -[3 more issues with full details] - -## Suggested Improvements - -[5 more issues with full details] - -## Good Practices -- Clean MVVM separation -- Proper Hilt DI usage -- Comprehensive test coverage - -## Action Items -1. Fix mutable state exposure (app/vault/VaultViewModel.kt:45) -2. Add null safety (app/vault/VaultRepository.kt:123) -3. [8 more action items duplicating the issues above] -``` - -**Token count:** ~800-1000 tokens -**Issues:** Heavy duplication, verbose praise, action items redundant with inline comments - -**Concise Format (New):** -```markdown -**Overall Assessment:** REQUEST CHANGES - -**Critical Issues:** -- Exposes mutable state (app/vault/VaultViewModel.kt:45) -- Missing null safety (app/vault/VaultRepository.kt:123) - -See inline comments for all issue details. -``` - -Plus inline comments with `
` tags. - -**Token count:** ~200-300 tokens visible, ~600-800 total (expandable) -**Benefits:** -- 60-70% token reduction -- Zero duplication -- Faster scanning -- All details preserved - ---- - -### Implementation Notes - -**When to use which format:** - -**Use Concise Format for:** -- All reviews going forward (new default) -- High token efficiency needed -- Multiple issues to report -- When details would overwhelm - -**Visible Content (Not Collapsed):** -- Severity level -- One-line issue description -- File:line reference - -**Collapsed Content (In `
`):** -- Code examples (before/after) -- Detailed rationale -- References to documentation -- Implementation suggestions - -**Never Include in Summary:** -- Issue details (those are in inline comments) -- Good Practices section (eliminates noise) -- Action Items (duplicates inline comments) +See `SKILL.md` for complete review guidelines.