Refine reviewing-changes skill to eliminate verbosity and praise (#6128)

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

.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 `<thinking>` 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:
+
+<thinking>
+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?
+</thinking>
+
+**Thread Detection Procedure:**
+
+1. **Fetch existing comment count:**
+   ```bash
+   gh pr view <pr-number> --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 <pr-number> --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
 
 <thinking>
 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
 
 <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
 
 <thinking>
 Before writing each comment:
@@ -86,17 +126,28 @@ Before writing each comment:
 5. Is there a documentation reference to include?
 </thinking>
 
-**CRITICAL**: Use inline comments on specific lines, NOT a single large review comment.
+**CRITICAL**: Use summary comment + inline comments approach.
 
-**Inline Comment Rules**:
+**Review Comment Structure**:
-- Create separate comment for EACH specific issue on the exact line
+- Create ONE summary comment with overall verdict + critical issues list
-- Do NOT create one large summary comment with all issues
+- Create separate inline comment for EACH specific issue on the exact line with full details
-- Do NOT update existing comments - always create new comments
+- Summary directs readers to inline comments ("See inline comments for details")
-- This ensures history is retained for other reviewers
+- 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 `<details>` Tags):
+
+**MUST use `<details>` 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>
 <summary>Details and fix</summary>
@@ -109,9 +160,13 @@ Reference: [docs link if applicable]
 </details>
 ```
 
+**Visibility Rule:**
+- **Visible:** Severity prefix (emoji + text) + one-line description
+- **Collapsed in `<details>`:** Code examples, rationale, explanations, references
+
 **Example inline comment**:
 ```
-**CRITICAL**: Exposes mutable state
+⚠️ **CRITICAL**: Exposes mutable state
 
 <details>
 <summary>Details and fix</summary>
@@ -129,30 +184,58 @@ Reference: docs/ARCHITECTURE.md#mvvm-pattern
 </details>
 ```
 
-**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 `<details>` 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 `<details>` 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 `<details>` tag

.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]`

.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
 ```