Apply all findings

Apply All Findings

Overview

Address EVERY finding from code review. Findings are either FIXED or DEFERRED with tracking issues.

Core principle: Minor issues accumulate into major problems.

The rule: If it was worth noting, it's worth tracking.

ABSOLUTE REQUIREMENT: Every finding results in ONE of:

1. Fixed in this PR (verified)
2. Tracking issue created (linked in review artifact)

There is NO third option. "Won't fix without tracking" is NOT permitted.

Why All Findings

Minor Issues Compound

1 unclear variable name +
1 missing null check +
1 inconsistent style +
1 outdated comment =
Confusing, fragile code

Selective Fixing Creates Precedent

"This minor issue can wait" →
"That minor issue can wait too" →
"We don't fix minor issues" →
Technical debt mountain

Thoroughness Builds Quality Culture

Every finding addressed →
High standards maintained →
Quality becomes habit

The Process

Step 1: Gather All Findings

From comprehensive-review, you have:

### Findings

1. [Critical] SQL injection in findUser()
2. [Major] N+1 query in getOrders()
3. [Minor] Variable 'x' should be renamed
4. [Minor] Missing JSDoc on helper()
5. [Minor] Inconsistent quote style

Step 2: Create Checklist

Every finding becomes a todo:

- [ ] Fix SQL injection in findUser()
- [ ] Fix N+1 query in getOrders()
- [ ] Rename variable 'x' to descriptive name
- [ ] Add JSDoc to helper()
- [ ] Fix quote style to use single quotes

Step 3: Address Systematically

Work through the list. For each finding:

If Fixable:

1. Fix the issue
2. Verify the fix
3. Check off the item
4. Move to next finding

If Not Fixable in This PR:

1. Verify valid deferral reason (see deferred-finding skill)
2. Create tracking issue with full documentation
3. Add tracking issue to review artifact
4. Mark as DEFERRED (not unaddressed)
5. Move to next finding

# Create tracking issue for deferred finding
gh issue create \
  --title "[Finding] [Description] (from #123)" \
  --label "review-finding,depth:1" \
  --body "[Full deferred-finding template]"

# Create spawned-from label if needed
gh label create "spawned-from:#123" --color "C2E0C6" 2>/dev/null || true
gh issue edit [NEW_ISSUE] --add-label "spawned-from:#123"

Step 4: Verify All Complete

Before considering done:

# Re-run linting
pnpm lint

# Re-run tests
pnpm test

# Re-run type check
pnpm typecheck

All checks must pass.

Step 5: Update Review Artifact

After all findings addressed, update artifact in issue comment:

1. All FIXED findings marked ✅ FIXED
2. All DEFERRED findings have tracking issue # linked
3. "Unaddressed: 0" in summary
4. "Review Status: COMPLETE"

Addressing by Type

Critical/Major Findings

These require code changes:

// Finding: SQL injection in findUser()
// Before
return db.query(`SELECT * FROM users WHERE username = '${username}'`);

// After
return db.query('SELECT * FROM users WHERE username = ?', [username]);

Minor: Naming

// Finding: Variable 'x' should be renamed
// Before
const x = users.filter(u => u.active);

// After
const activeUsers = users.filter(user => user.isActive);

Minor: Documentation

// Finding: Missing JSDoc on helper()
// Before
function helper(data: Data): Result {

// After
/**
 * Transforms raw data into the expected result format.
 *
 * @param data - Raw data from the API
 * @returns Transformed result ready for display
 */
function helper(data: Data): Result {

Minor: Style

// Finding: Inconsistent quote style
// Before
const name = "Alice";
const greeting = 'Hello';

// After (using project standard: single quotes)
const name = 'Alice';
const greeting = 'Hello';

Handling Deferrals

Valid Deferral Reasons

Reason	Example	Requires
Out of scope	Architectural change	Tracking issue
External dependency	Infrastructure change	Tracking issue
Breaking change	Major version bump	Tracking issue
Separate concern	Independent feature	Tracking issue

NOT Valid Deferral Reasons

Excuse	Reality	Action
"It's minor"	Minor compounds	Fix now
"Takes too long"	Debt takes longer	Fix now
"Good enough"	Never enough	Fix now
"Not important"	Then why note it?	Fix now
"Do it later"	Without tracking? No.	Fix or create issue

Deferral MUST Create Issue

ABSOLUTE: No deferral without tracking issue.

# WRONG - Deferred without tracking
"We'll fix the SQL injection later"  # NO

# RIGHT - Deferred with tracking
gh issue create --title "[Finding] SQL injection in findUser (from #123)" ...
# Then link #456 in review artifact

Verification

After addressing all findings:

Run All Checks

# Linting
pnpm lint

# Type checking
pnpm typecheck

# Tests
pnpm test

# Build
pnpm build

Review the Diff

git diff

Verify:

• All findings addressed
• No unrelated changes
• Tests updated if behavior changed

Self-Review Again

Quick pass through 7 criteria to ensure fixes didn't introduce new issues.

Checklist

Before moving on from review:

• All critical findings addressed
• All major findings addressed
• All minor findings addressed
• Any deferred finding has tracking issue created
• Tracking issues linked in review artifact
• All automated checks pass
• Fixes reviewed for correctness
• No new issues introduced
• Review artifact updated with final status
• "Unaddressed: 0" confirmed

Common Pushback (Rejected)

Pushback	Response
"We can fix minors later"	Without tracking? No. Create issue or fix now.
"This is slowing us down"	Debt slows you down more.
"It's not important"	Then why was it noted?
"Good enough"	Good enough is never enough.
"The reviewer is being picky"	Attention to detail is valuable.

Integration

This skill is called by:

• issue-driven-development - Step 10

This skill follows:

• comprehensive-review - Generates the findings

This skill uses:

• deferred-finding - For creating tracking issues

This skill ensures:

• No accumulated minor issues
• Consistent quality standards
• Complete reviews, not partial
• All deferrals tracked in GitHub