diff --git a/.claude/prompts/review-code.md b/.claude/prompts/review-code.md index 0efd6eb47c..010e9c9a07 100644 --- a/.claude/prompts/review-code.md +++ b/.claude/prompts/review-code.md @@ -1,27 +1,3 @@ Use the `reviewing-changes` skill to review this pull request. The PR branch is already checked out in the current working directory. - -## CRITICAL OUTPUT REQUIREMENTS - -**Summary Format (REQUIRED):** -- **Clean PRs (no issues)**: 2-3 lines MAXIMUM - - Format: `**Overall Assessment:** APPROVE\n[One sentence]` - - Example: "Clean refactoring following established patterns" - -- **PRs with issues**: Verdict + critical issues list (5-10 lines MAX) - - Format: `**Overall Assessment:** APPROVE/REQUEST CHANGES\n**Critical Issues:**\n- issue 1\nSee inline comments` - - All details go in inline comments with `
` tags, NOT in summary - -**NEVER create:** -- ❌ Praise sections ("Strengths", "Good Practices", "Excellent X") -- ❌ Verbose analysis sections (Architecture Assessment, Technical Review, Code Quality, etc.) -- ❌ Tables, statistics, or detailed breakdowns in summary -- ❌ Multiple summary sections -- ❌ Checkmarks listing everything done correctly - -**Inline Comments:** -- Create separate inline comment for each specific issue/recommendation -- Use collapsible `
` sections for code examples and explanations -- Only severity + one-line description visible; all other content collapsed -- Track status of previously identified issues if this is a subsequent review diff --git a/.claude/skills/reviewing-changes/SKILL.md b/.claude/skills/reviewing-changes/SKILL.md index ba417919e7..df7feca57a 100644 --- a/.claude/skills/reviewing-changes/SKILL.md +++ b/.claude/skills/reviewing-changes/SKILL.md @@ -1,56 +1,34 @@ --- name: reviewing-changes -version: 2.0.0 -description: Comprehensive code reviews for Bitwarden Android. Detects change type (dependency update, bug fix, feature, UI, refactoring, infrastructure) and applies appropriate review depth. Validates MVVM patterns, Hilt DI, security requirements, and test coverage per project standards. Use when reviewing pull requests, checking commits, analyzing code changes, or evaluating architectural compliance. +version: 3.0.0 +description: Android-specific code review workflow additions for Bitwarden Android. Provides change type refinements, checklist loading, and reference material organization. Complements bitwarden-code-reviewer agent's base review standards. --- -# Reviewing Changes +# Reviewing Changes - Android Additions + +This skill provides Android-specific workflow additions that complement the base `bitwarden-code-reviewer` agent standards. ## Instructions -**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. +**IMPORTANT**: Use structured thinking throughout your review process. Plan your analysis in `` tags before providing final feedback. -### Step 1: Check for Existing Review Threads - -Always check for existing comment threads to avoid duplicate comments: +### Step 1: Retrieve Additional Details -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? +Determine if more context is available for the changes: +1. Are there JIRA tickets or GitHub Issues mentioned in the PR title or body? +2. Are there other GitHub pull requests mentioned in the PR title or body? -**Thread Detection Procedure:** +Retrieve any additional information linked to the pull request using available tools (JIRA MCP, GitHub API). -1. **Fetch existing comment count:** - ```bash - gh pr view --json comments --jq '.comments | length' - ``` +If pull request title and message do not provide enough context, request additional details from the reviewer: +- Link a JIRA ticket +- Associate a GitHub issue +- Link to another pull request +- Add more detail to the PR title or body -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 +### Step 2: Detect Change Type with Android Refinements Analyze the changeset systematically: @@ -60,17 +38,13 @@ Analyze the changeset systematically: 4. What's the risk level of these changes? -Analyze the changeset to determine the primary change type: +Use the base change type detection from the agent, with Android-specific refinements: -**Detection Rules:** -- **Dependency Update**: Only gradle files changed (`libs.versions.toml`, `build.gradle.kts`) with version number modifications -- **Bug Fix**: PR/commit title contains "fix", "bug", or issue ID; addresses existing broken behavior -- **Feature Addition**: New files, new ViewModels, significant new functionality -- **UI Refinement**: Only UI/Compose files changed, layout/styling focus -- **Refactoring**: Code restructuring without behavior change, pattern improvements -- **Infrastructure**: CI/CD files, Gradle config, build scripts, tooling changes - -If changeset spans multiple types, use the most complex type's checklist. +**Android-specific patterns:** +- **Feature Addition**: New `ViewModel`, new `Repository`, new `@Composable` functions, new `*Screen.kt` files +- **UI Refinement**: Changes only in `*Screen.kt`, `*Composable.kt`, `ui/` package files +- **Infrastructure**: Changes to `.github/workflows/`, `gradle/`, `build.gradle.kts`, `libs.versions.toml` +- **Dependency Update**: Changes only to `libs.versions.toml` or `build.gradle.kts` with version bumps ### Step 3: Load Appropriate Checklist @@ -89,7 +63,7 @@ The checklist provides: - What to check and what to skip - Structured thinking guidance -### Step 4: Execute Review with Structured Thinking +### Step 4: Execute Review Following Checklist Before diving into details: @@ -102,7 +76,7 @@ Before diving into details: Follow the checklist's multi-pass strategy, thinking through each pass systematically. -### Step 5: Consult Reference Materials As Needed +### Step 5: Consult Android Reference Materials As Needed Load reference files only when needed for specific questions: @@ -115,206 +89,10 @@ 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 6: Document Findings - -## 🛑 STOP: Determine Output Format FIRST - - -Before writing ANY output, answer this critical question: -1. Did I find ANY issues (Critical, Important, Suggested, or Questions)? -2. If NO issues found → This is a CLEAN PR → Use 2-3 line minimal format and STOP -3. If issues found → Use verdict + critical issues list + inline comments format -4. NEVER create praise sections or elaborate on correct implementations - - -**Decision Tree:** - -``` -Do you have ANY issues to report (Critical/Important/Suggested/Questions)? -│ -├─ NO → CLEAN PR -│ └─ Use 2-3 line format: -│ "**Overall Assessment:** APPROVE -│ [One sentence describing what PR does well]" -│ └─ STOP. Do not proceed to detailed format guidance. -│ -└─ YES → PR WITH ISSUES - └─ Use minimal summary + inline comments: - "**Overall Assessment:** APPROVE / REQUEST CHANGES - **Critical Issues:** - - [issue with file:line] - See inline comments for details." -``` - -## Special Case: Clean PRs with No Issues - -When you find NO critical, important, or suggested issues: - -**Minimal Approval Format (REQUIRED):** -``` -**Overall Assessment:** APPROVE - -[One sentence describing what the PR does well] -``` - -**Examples:** -- "Clean refactoring following established patterns" -- "Solid bug fix with comprehensive test coverage" -- "Well-structured feature implementation meeting all standards" - -**NEVER do this for clean PRs:** -- ❌ Multiple sections (Key Strengths, Changes, Code Quality, etc.) -- ❌ Listing everything that was done correctly -- ❌ Checkmarks for each file or pattern followed -- ❌ Elaborate praise or detailed positive analysis -- ❌ Tables, statistics, or detailed breakdowns - -**Why brevity matters:** -- Respects developer time (quick approval = move forward faster) -- Reduces noise in PR conversations -- Saves tokens and processing time -- Focuses attention on PRs that actually need discussion - -**If you're tempted to write more than 3 lines for a clean PR, STOP. You're doing it wrong.** - ---- - - -Before writing each comment: -1. Is this issue Critical, Important, Suggested, or just Acknowledgment? -2. Should I ask a question or provide direction? -3. What's the rationale I need to explain? -4. What code example would make this actionable? -5. Is there a documentation reference to include? - - -**CRITICAL**: Use summary comment + inline comments approach. - -**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. - -``` -[emoji] **[SEVERITY]**: [One-line issue description] - -
-Details and fix - -[Code example or specific fix] - -[Rationale explaining why] - -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 - -
-Details and fix - -Change `MutableStateFlow` to `StateFlow`: - -\```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 -
-``` - -**Summary Comment Format (REQUIRED - No Exceptions):** - -When you have issues to report, use this format ONLY: - -``` -**Overall Assessment:** APPROVE / REQUEST CHANGES - -**Critical Issues** (if any): -- [One-line summary with file:line reference] - -See inline comments for all details. -``` - -**Maximum Length**: 5-10 lines total, regardless of PR size or complexity. - -**No exceptions for**: -- ❌ Large PRs (10+ files) -- ❌ Multiple issue domains -- ❌ High-severity issues -- ❌ Complex changes - -All details belong in inline comments with `
` tags, NOT in the summary. - -**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** (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"/"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 -- Keep summary to 5-10 lines maximum, regardless of PR size or your analysis depth -- 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 -- **Summary visible**: Verdict + critical issues list only - -See `examples/review-outputs.md` for complete examples. - ## Core Principles -- **Minimal reviews for clean PRs**: 2-3 lines when no issues found (see Step 6 format guidance) -- **Issues-focused feedback**: Only comment when there's something actionable; acknowledge good work briefly without elaboration (see priority-framework.md:145-166) - **Appropriate depth**: Match review rigor to change complexity and risk - **Specific references**: Always use `file:line_number` format for precise location - **Actionable feedback**: Say what to do and why, not just what's wrong -- **Constructive tone**: Ask questions for design decisions, explain rationale, focus on code not people -- **Efficient reviews**: Use multi-pass strategy, time-box reviews, skip what's not relevant +- **Efficient reviews**: Use multi-pass strategy, skip what's not relevant +- **Android patterns**: Validate MVVM, Hilt DI, Compose conventions, Kotlin idioms diff --git a/.claude/skills/reviewing-changes/examples/review-outputs.md b/.claude/skills/reviewing-changes/examples/review-outputs.md index 9095edf74f..58ddaea5a6 100644 --- a/.claude/skills/reviewing-changes/examples/review-outputs.md +++ b/.claude/skills/reviewing-changes/examples/review-outputs.md @@ -2,6 +2,27 @@ Well-structured code reviews demonstrating appropriate depth, tone, and formatting for different change types. +## Table of Contents + +**Format Reference:** +- [Quick Format Reference](#quick-format-reference) + - [Inline Comment Format](#inline-comment-format-required) + - [Summary Comment Format](#summary-comment-format) + +**Examples:** +- [Example 1: Clean PR (No Issues)](#example-1-clean-pr-no-issues) +- [Example 2: Dependency Update with Breaking Changes](#example-2-dependency-update-with-breaking-changes) +- [Example 3: Feature Addition with Critical Issues](#example-3-feature-addition-with-critical-issues) + +**Anti-Patterns:** +- [❌ Anti-Patterns to Avoid](#-anti-patterns-to-avoid) + - [Problem: Verbose Summary with Multiple Sections](#problem-verbose-summary-with-multiple-sections) + - [Problem: Praise-Only Inline Comments](#problem-praise-only-inline-comments) + - [Problem: Missing `
` Tags](#problem-missing-details-tags) + +**Summary:** +- [Summary](#summary) + --- ## Quick Format Reference @@ -25,10 +46,11 @@ Reference: [docs link if applicable] ``` **Severity Levels:** -- ⚠️ **CRITICAL** - Blocking, must fix -- 📋 **IMPORTANT** - Should fix -- 💡 **SUGGESTED** - Nice to have -- ❓ **QUESTION** - Seeking clarification +- ❌ **CRITICAL** - Blocking, must fix (security, crashes, architecture violations) +- ⚠️ **IMPORTANT** - Should fix (missing tests, quality issues) +- ♻️ **DEBT** - Technical debt (duplication, convention violations, future rework needed) +- 🎨 **SUGGESTED** - Nice to have (refactoring, improvements) +- 💭 **QUESTION** - Seeking clarification (requirements, design decisions) ### Summary Comment Format @@ -81,7 +103,7 @@ See inline comments for migration details. **Inline Comment 1** (on `network/api/BitwardenApiService.kt:34`): ```markdown -⚠️ **CRITICAL**: API migration required for Retrofit 3.0 +❌ **CRITICAL**: API migration required for Retrofit 3.0
Details and fix @@ -136,7 +158,7 @@ See inline comments for all issues and suggestions. **Inline Comment 1** (on `app/vault/unlock/UnlockViewModel.kt:78`): ```markdown -⚠️ **CRITICAL**: Exposes mutable state +❌ **CRITICAL**: Exposes mutable state
Details and fix @@ -160,7 +182,7 @@ Reference: docs/ARCHITECTURE.md#mvvm-pattern **Inline Comment 2** (on `data/vault/UnlockRepository.kt:145`): ```markdown -⚠️ **CRITICAL**: PIN stored without encryption - SECURITY ISSUE +❌ **CRITICAL**: PIN stored without encryption - SECURITY ISSUE
Details and fix @@ -188,7 +210,7 @@ Reference: docs/ARCHITECTURE.md#security **Inline Comment 3** (on `app/vault/unlock/UnlockViewModel.kt:92`): ```markdown -📋 **IMPORTANT**: Missing error handling test +⚠️ **IMPORTANT**: Missing error handling test
Details and fix @@ -214,7 +236,7 @@ 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 +🎨 **SUGGESTED**: Consider rate limiting for PIN attempts
Details and fix @@ -246,7 +268,7 @@ Would add security layer against brute force. Consider discussing threat model w **Inline Comment 5** (on `app/vault/unlock/UnlockScreen.kt:134`): ```markdown -❓ **QUESTION**: Can we use BitwardenTextField? +💭 **QUESTION**: Can we use BitwardenTextField?
Details @@ -356,7 +378,7 @@ This is exactly the right approach for fail-safe security. **What NOT to do:** ```markdown -⚠️ **CRITICAL**: Missing test coverage for security-critical code +❌ **CRITICAL**: Missing test coverage for security-critical code The `@OmitFromCoverage` annotation excludes this entire class from test coverage. @@ -382,7 +404,7 @@ Security-critical code should have the highest test coverage, not be omitted. **Correct approach:** ```markdown -⚠️ **CRITICAL**: Missing test coverage for security-critical code +❌ **CRITICAL**: Missing test coverage for security-critical code
Details and fix @@ -422,5 +444,3 @@ Security-critical code should have the highest test coverage, not be omitted. - Praise-only inline comments - Duplication between summary and inline comments - Verbose analysis in summary (belongs in inline comments) - -See `SKILL.md` for complete review guidelines. diff --git a/.claude/skills/reviewing-changes/reference/architectural-patterns.md b/.claude/skills/reviewing-changes/reference/architectural-patterns.md index 609695217f..6244bae869 100644 --- a/.claude/skills/reviewing-changes/reference/architectural-patterns.md +++ b/.claude/skills/reviewing-changes/reference/architectural-patterns.md @@ -2,6 +2,22 @@ Quick reference for Bitwarden Android architectural patterns during code reviews. For comprehensive details, read `docs/ARCHITECTURE.md` and `docs/STYLE_AND_BEST_PRACTICES.md`. +## Table of Contents + +**Core Patterns:** +- [MVVM + UDF Pattern](#mvvm--udf-pattern) + - [ViewModel Structure](#viewmodel-structure) + - [UI Layer (Compose)](#ui-layer-compose) +- [Hilt Dependency Injection](#hilt-dependency-injection) + - [ViewModels](#viewmodels) + - [Repositories and Managers](#repositories-and-managers) +- [Module Organization](#module-organization) +- [Error Handling](#error-handling) + - [Use Result Types, Not Exceptions](#use-result-types-not-exceptions) +- [Quick Checklist](#quick-checklist) + +--- + ## MVVM + UDF Pattern ### ViewModel Structure diff --git a/.claude/skills/reviewing-changes/reference/priority-framework.md b/.claude/skills/reviewing-changes/reference/priority-framework.md index 841469848f..c8d505a0f2 100644 --- a/.claude/skills/reviewing-changes/reference/priority-framework.md +++ b/.claude/skills/reviewing-changes/reference/priority-framework.md @@ -1,8 +1,28 @@ -# Issue Priority Framework +# Finding Priority Framework Use this framework to classify findings during code review. Clear prioritization helps authors triage and address issues effectively. -## Critical (Blocker - Must Fix Before Merge) +## Table of Contents + +**Severity Categories:** +- [❌ CRITICAL (Blocker - Must Fix Before Merge)](#critical-blocker---must-fix-before-merge) +- [⚠️ IMPORTANT (Should Fix)](#important-should-fix) +- [♻️ DEBT (Technical Debt)](#debt-technical-debt) +- [🎨 SUGGESTED (Nice to Have)](#suggested-nice-to-have) +- [💭 QUESTION (Seeking Clarification)](#question-seeking-clarification) +- [Optional (Acknowledge But Don't Require)](#optional-acknowledge-but-dont-require) + +**Guidelines:** +- [Classification Guidelines](#classification-guidelines) + - [When Something is Between Categories](#when-something-is-between-categories) + - [Context Matters](#context-matters) +- [Examples by Change Type](#examples-by-change-type) +- [Special Cases](#special-cases) +- [Summary](#summary) + +--- + +## ❌ **CRITICAL** (Blocker - Must Fix Before Merge) These issues **must** be addressed before the PR can be merged. They pose immediate risks to security, stability, or architecture integrity. @@ -49,7 +69,7 @@ This violates MVVM encapsulation pattern. --- -## Important (Should Fix) +## ⚠️ **IMPORTANT** (Should Fix) These issues should be addressed but don't block merge if there's a compelling reason. They improve code quality, maintainability, or robustness. @@ -102,7 +122,53 @@ Fetching items one-by-one in loop. Consider batch fetch to reduce database queri --- -## Suggested (Nice to Have) +## ♻️ **DEBT** (Technical Debt) + +Code that duplicates existing patterns, violates established conventions, or will require rework within 6 months. Introduces technical debt that should be tracked for future cleanup. + +### Duplication +- Copy-pasted code blocks across files +- Repeated validation or business logic +- Multiple implementations of same pattern +- Data transformation duplicated in multiple places + +**Example**: +``` +**app/vault/VaultListViewModel.kt:156** - DEBT: Duplicates encryption logic +Same encryption pattern exists in VaultRepository.kt:234 and SyncManager.kt:89. +Extract to shared EncryptionUtil to reduce maintenance burden. +``` + +### Convention Violations +- Inconsistent error handling approaches within same module +- Mixing architectural patterns (MVVM + MVC) +- Not following established DI patterns +- Deviating from project code style significantly + +**Example**: +``` +**data/auth/AuthRepository.kt:78** - DEBT: Exception-based error handling +Project standard is Result for error handling. This uses try-catch with throws. +Creates inconsistency and makes testing harder. +Reference: docs/ARCHITECTURE.md#error-handling +``` + +### Future Rework Required +- Hardcoded values that should be configurable +- Temporary workarounds without TODO/FIXME +- Code that will need changes when planned features arrive +- Tight coupling that prevents future extensibility + +**Example**: +``` +**app/settings/SettingsViewModel.kt:45** - DEBT: Hardcoded feature flags +Feature flags should come from remote config for A/B testing. +Will require rework when experimentation framework launches. +``` + +--- + +## 🎨 **SUGGESTED** (Nice to Have) These are improvement opportunities but not required. Consider the effort vs. benefit before requesting changes. @@ -142,6 +208,80 @@ Could be extracted to separate validator class for reusability and testing. --- +## 💭 **QUESTION** (Seeking Clarification) + +Questions about requirements, unclear intent, or potential conflicts that require human knowledge to answer. Open inquiries that cannot be resolved through code inspection alone. + +### Requirements Clarification +- Ambiguous acceptance criteria +- Multiple valid implementation approaches +- Unclear business rules or edge case handling +- Conflicting requirements between specs and implementation + +**Example**: +``` +**app/vault/ItemListViewModel.kt:67** - QUESTION: Expected sort behavior for equal timestamps? +When items have identical timestamps, should secondary sort be by: +- Name (alphabetical) +- Creation order +- Item type priority + +Spec doesn't specify tie-breaking logic. +``` + +### Design Decisions +- Architecture choices that could go multiple ways +- Trade-offs between approaches without clear winner +- Feature flag strategy or rollout approach +- API design with multiple valid patterns + +**Example**: +``` +**data/sync/SyncManager.kt:134** - QUESTION: Should sync failures retry automatically? +Current implementation fails immediately. Options: +- Exponential backoff (3 retries) +- User-triggered retry only +- Background retry on network restore + +What's the expected UX? +``` + +### System Integration +- Unclear contracts with external systems +- Potential conflicts with other features/modules +- Assumptions about third-party API behavior +- Cross-team coordination needs + +**Example**: +``` +**app/auth/BiometricPrompt.kt:89** - QUESTION: Compatibility with pending device credentials PR? +PR #1234 is refactoring device credentials. Should this: +- Merge first and adapt later +- Wait for #1234 to land +- Coordinate with that author + +Timing unclear. +``` + +### Testing Strategy +- Uncertainty about test scope or approach +- Questions about mocking external dependencies +- Edge cases that need product input +- Performance testing requirements + +**Example**: +``` +**data/vault/EncryptionTest.kt:45** - QUESTION: Should we test against real Keystore? +Currently using mocked Keystore. Real Keystore testing would: ++ Catch hardware-specific issues +- Slow down CI significantly +- Require API 23+ emulators + +What's the priority? +``` + +--- + ## Optional (Acknowledge But Don't Require) Note good practices to reinforce positive patterns. Keep these **brief** - list only, no elaboration. @@ -175,11 +315,26 @@ Note good practices to reinforce positive patterns. Keep these **brief** - list - If yes → Critical - If no → Important +**If unsure between Important and Debt**: +- Ask: "Is this a bug/defect or just duplication/inconsistency?" +- If bug/defect → Important +- If duplication/inconsistency → Debt + **If unsure between Important and Suggested**: - Ask: "Would I block merge over this?" - If yes → Important - If no → Suggested +**If unsure between Debt and Suggested**: +- Ask: "Will this require rework within 6 months?" +- If yes → Debt +- If no → Suggested + +**If unsure between Suggested and Question**: +- Ask: "Am I requesting a change or asking for clarification?" +- If requesting change → Suggested +- If seeking clarification → Question + **If unsure between Suggested and Optional**: - Ask: "Is this actionable feedback or just acknowledgment?" - If actionable → Suggested @@ -270,5 +425,7 @@ Missing tests for refactored helper → SUGGESTED **Critical**: Block merge, must fix (security, stability, architecture) **Important**: Should fix before merge (testing, quality, performance) +**Debt**: Technical debt introduced, track for future cleanup **Suggested**: Nice to have, consider effort vs benefit +**Question**: Seeking clarification on requirements or design **Optional**: Acknowledge good practices, keep brief diff --git a/.claude/skills/reviewing-changes/reference/review-psychology.md b/.claude/skills/reviewing-changes/reference/review-psychology.md index 6866068dfb..8606d454e1 100644 --- a/.claude/skills/reviewing-changes/reference/review-psychology.md +++ b/.claude/skills/reviewing-changes/reference/review-psychology.md @@ -2,6 +2,20 @@ Effective code review feedback is clear, actionable, and constructive. This guide provides phrasing patterns for inline comments. +## Table of Contents + +**Guidelines:** +- [Core Directives](#core-directives) +- [Phrasing Templates](#phrasing-templates) + - [Critical Issues (Prescriptive)](#critical-issues-prescriptive) + - [Suggested Improvements (Exploratory)](#suggested-improvements-exploratory) + - [Questions (Collaborative)](#questions-collaborative) + - [Test Suggestions](#test-suggestions) +- [When to Be Prescriptive vs Ask Questions](#when-to-be-prescriptive-vs-ask-questions) +- [Special Cases](#special-cases) + +--- + ## Core Directives - **Keep positive feedback minimal**: For clean PRs with no issues, use 2-3 line approval only. When acknowledging good practices in PRs with issues, use single bullet list with no elaboration. Never create elaborate sections praising correct implementations.