Reduce verbosity in reviewing-changes skill for clean PRs (#6121)

Co-authored-by: Claude <noreply@anthropic.com>

.claude/skills/reviewing-changes/SKILL.md

@@ -162,8 +162,38 @@ See `examples/review-outputs.md` for complete examples.
 
 ## Core Principles
 
+- **Minimal reviews for clean PRs**: 2-3 lines when no issues found (see Special Case below)
+- **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
+
+## Special Case: Clean PRs with No Issues
+
+When you find NO critical, important, or suggested issues:
+
+**Minimal Approval Format:**
+```
+**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
+
+**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

.claude/skills/reviewing-changes/checklists/refactoring.md

@@ -180,55 +180,115 @@ Follow the format guidance from `SKILL.md` Step 5 (concise summary with critical
 See inline comments for all issue details.
 ```
 
-## Example Review
+## Example Reviews
 
+### Example 1: Refactoring with Incomplete Migration
+
+**Context**: Refactoring authentication to Repository pattern, but one ViewModel still uses old pattern
+
+**Summary Comment:**
 ```markdown
-## Summary
-Refactors authentication flow to use Repository pattern instead of direct Manager access
+**Overall Assessment:** REQUEST CHANGES
 
-Scope: 12 files changed, 8 ViewModels updated, Repository interface extracted
+**Critical Issues:**
+- Incomplete migration (app/vault/VaultViewModel.kt:89)
 
-## Critical Issues
-None - behavior preserved, tests passing
-
-## Suggested Improvements
-
-**app/vault/VaultViewModel.kt:89** - Old pattern still used
-This ViewModel still injects AuthManager directly. Should it use AuthRepository like the others?
-```kotlin
-// Current
-class VaultViewModel @Inject constructor(
-    private val authManager: AuthManager  // Old pattern
-)
-
-// Should be
-class VaultViewModel @Inject constructor(
-    private val authRepository: AuthRepository  // New pattern
-)
+See inline comments for details.
 ```
 
-**data/auth/AuthManager.kt:1** - Add deprecation notice
+**Inline Comment 1** (on `app/vault/VaultViewModel.kt:89`):
+```markdown
+**IMPORTANT**: Incomplete migration
+
+<details>
+<summary>Details and fix</summary>
+
+This ViewModel still injects AuthManager directly. Should it use AuthRepository like the other 11 ViewModels?
+
+\```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.
+</details>
+```
+
+**Inline Comment 2** (on `data/auth/AuthManager.kt:1`):
+```markdown
+**SUGGESTED**: Add deprecation notice
+
+<details>
+<summary>Details</summary>
+
 Can we add @Deprecated to AuthManager to guide future development?
-```kotlin
+
+\```kotlin
 @Deprecated(
     message = "Use AuthRepository interface instead",
     replaceWith = ReplaceWith("AuthRepository"),
     level = DeprecationLevel.WARNING
 )
-class AuthManager
+class AuthManager @Inject constructor(...)
+\```
+
+This helps prevent new code from using the old pattern.
+</details>
 ```
 
-**docs/ARCHITECTURE.md** - Document the new pattern
-Should we update the architecture docs to reflect this Repository pattern?
-The current docs still reference AuthManager as the recommended approach.
+---
 
-## Good Practices
-- Repository interface clearly defined
-- All data access methods use Result types
-- Tests updated to match new pattern
+### Example 2: Clean Refactoring (No Issues)
 
-## Action Items
-1. Update VaultViewModel to use AuthRepository
-2. Add @Deprecated to AuthManager with migration guidance
-3. Update ARCHITECTURE.md to document Repository pattern
+**Context**: Refactoring with complete migration, all patterns followed correctly, tests passing
+
+**Review Comment:**
+```markdown
+**Overall Assessment:** APPROVE
+
+Clean refactoring moving ExitManager to :ui module. Follows established patterns, eliminates duplication, tests updated correctly.
 ```
+
+**Token count:** ~30 tokens (vs ~800 for verbose format)
+
+**Why this works:**
+- 3 lines total
+- Clear approval decision
+- Briefly notes what was done
+- No elaborate sections, checkmarks, or excessive praise
+- Author gets immediate green light to merge
+
+**What NOT to do for clean refactorings:**
+```markdown
+❌ DO NOT create these sections:
+
+## Summary
+This PR successfully refactors ExitManager into shared code...
+
+## Key Strengths
+- ✅ Follows established module organization patterns
+- ✅ Removes code duplication between apps
+- ✅ Improves test coverage
+- ✅ Maintains consistent behavior
+[...20 more checkmarks...]
+
+## Code Quality & Architecture
+**Architectural Compliance:** ✅
+- Correctly places manager in :ui module
+- Follows established pattern for UI-layer managers
+[...detailed analysis...]
+
+## Changes
+- ✅ Moved ExitManager interface from app → ui module
+- ✅ Moved ExitManagerImpl from app → ui module
+[...listing every file...]
+```
+
+This is excessive. **For clean PRs: 2-3 lines maximum.**

.claude/skills/reviewing-changes/examples/review-outputs.md

@@ -851,6 +851,132 @@ Prevents regression of the bug just fixed.
 
 ---
 
+## 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):**

.claude/skills/reviewing-changes/reference/review-psychology.md

@@ -4,12 +4,12 @@ Effective code review feedback is clear, actionable, and constructive. This guid
 
 ## 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.
 - Ask questions for design decisions, be prescriptive for clear violations
 - Focus on code, not people ("This code..." not "You...")
 - Use I-statements for subjective feedback ("Hard for me to understand...")
 - Explain rationale with every recommendation
 - Avoid: "just", "simply", "obviously", "easy"
-- Keep positive feedback brief (list only, no elaboration)
 
 ---