fix-review

# Fix Review

Differential analysis to verify commits address security findings without introducing bugs.

## When to Use

- Reviewing fix branches against security audit reports
- Validating that remediation commits actually address findings
- Checking if specific findings (TOB-XXX format) have been fixed
- Analyzing commit ranges for bug introduction patterns
- Cross-referencing code changes with audit recommendations

## When NOT to Use

- Initial security audits (use audit-context-building or differential-review)
- Code review without a specific baseline or finding set
- Greenfield development with no prior audit
- Documentation-only changes

---

## Rationalizations (Do Not Skip)

| Rationalization | Why It's Wrong | Required Action |
|-----------------|----------------|-----------------|
| "The commit message says it fixes TOB-XXX" | Messages lie; code tells truth | Verify the actual code change addresses the finding |
| "Small fix, no new bugs possible" | Small changes cause big bugs | Analyze all changes for anti-patterns |
| "I'll check the important findings" | All findings matter | Systematically check every finding |
| "The tests pass" | Tests may not cover the fix | Verify fix logic, not just test status |
| "Same developer, they know the code" | Familiarity breeds blind spots | Fresh analysis of every change |

---

## Quick Reference

### Input Requirements

| Input | Required | Format |
|-------|----------|--------|
| Source commit | Yes | Git commit hash or ref (baseline before fixes) |
| Target commit(s) | Yes | One or more commit hashes to analyze |
| Security report | No | Local path, URL, or Google Drive link |

### Finding Status Values

| Status | Meaning |
|--------|---------|
| FIXED | Code change directly addresses the finding |
| PARTIALLY_FIXED | Some aspects addressed, others remain |
| NOT_ADDRESSED | No relevant changes found |
| CANNOT_DETERMINE | Insufficient context to verify |

---

## Workflow

### Phase 1: Input Gathering

Collect required inputs from user:

Source commit:  [hash/ref before fixes]
Target commit:  [hash/ref to analyze]
Report:         [optional: path, URL, or "none"]

If user provides multiple target commits, process each separately with the same source.

### Phase 2: Report Retrieval

When a security report is provided, retrieve it based on format:

**Local file (PDF, MD, JSON, HTML):**
Read the file directly using the Read tool. Claude processes PDFs natively.

**URL:**
Fetch web content using the WebFetch tool.

**Google Drive URL that fails:**
See references/report-parsing.md for Google Drive fallback logic using gdrive CLI.

### Phase 3: Finding Extraction

Parse the report to extract findings:

**Trail of Bits format:**
- Look for "Detailed Findings" section
- Extract findings matching pattern: TOB-[A-Z]+-[0-9]+
- Capture: ID, title, severity, description, affected files

**Other formats:**
- Numbered findings (Finding 1, Finding 2)
- Severity-based sections (Critical, High, Medium, Low)
- JSON with findings array

See references/report-parsing.md for detailed parsing strategies.

### Phase 4: Commit Analysis

For each target commit, analyze the commit range:

# Get commit list from source to target
git log <source>..<target> --oneline

# Get full diff
git diff <source>..<target>

# Get changed files
git diff <source>..<target> --name-only

For each commit in the range:
1. Examine the diff for bug introduction patterns
2. Check for security anti-patterns (see references/bug-detection.md)
3. Map changes to relevant findings

### Phase 5: Finding Verification

For each finding in the report:

1. **Identify relevant commits** - Match by:
- File paths mentioned in finding
- Function/variable names in finding description
- Commit messages referencing the finding ID

2. **Verify the fix** - Check that:
- The root cause is addressed (not just symptoms)
- The fix follows the report's recommendation
- No new vulnerabilities are introduced

3. **Assign status** - Based on evidence:
- FIXED: Clear code change addresses the finding
- PARTIALLY_FIXED: Some aspects fixed, others remain
- NOT_ADDRESSED: No relevant changes
- CANNOT_DETERMINE: Need more context

4. **Document evidence** - For each finding:
- Commit hash(es) that address it
- Specific file and line changes
- How the fix addresses the root cause

See references/finding-matching.md for detailed matching strategies.

### Phase 6: Output Generation

Generate two outputs:

**1. Report file (FIX_REVIEW_REPORT.md):**

# Fix Review Report

**Source:** <commit>
**Target:** <commit>
**Report:** <path or "none">
**Date:** <date>

## Executive Summary

[Brief overview: X findings reviewed, Y fixed, Z concerns]

## Finding Status

| ID | Title | Severity | Status | Evidence |
|----|-------|----------|--------|----------|
| TOB-XXX-1 | Finding title | High | FIXED | abc123 |
| TOB-XXX-2 | Another finding | Medium | NOT_ADDRESSED | - |

## Bug Introduction Concerns

[Any potential bugs or regressions detected in the changes]

## Per-Commit Analysis

### Commit abc123: "Fix reentrancy in withdraw()"

**Files changed:** contracts/Vault.sol
**Findings addressed:** TOB-XXX-1
**Concerns:** None

[Detailed analysis]

## Recommendations

[Any follow-up actions needed]

**2. Conversation summary:**

Provide a concise summary in the conversation:
- Total findings: X
- Fixed: Y
- Not addressed: Z
- Concerns: [list any bug introduction risks]

---

## Bug Detection

Analyze commits for security anti-patterns. Key patterns to watch:
- Access control weakening (modifiers removed)
- Validation removal (require/assert deleted)
- Error handling reduction (try/catch removed)
- External call reordering (state after call)
- Integer operation changes (SafeMath removed)
- Cryptographic weakening

See references/bug-detection.md for comprehensive detection patterns and examples.

---

## Integration with Other Skills

**differential-review:** For initial security review of changes (before audit)

**issue-writer:** To format findings into formal audit reports

**audit-context-building:** For deep context when analyzing complex fixes

---

## Tips for Effective Reviews

**Do:**
- Verify the actual code change, not just commit messages
- Check that fixes address root causes, not symptoms
- Look for unintended side effects in adjacent code
- Cross-reference multiple findings that may interact
- Document evidence for every status assignment

**Don't:**
- Trust commit messages as proof of fix
- Skip findings because they seem minor
- Assume passing tests mean correct fixes
- Ignore changes outside the "fix" scope
- Mark FIXED without clear evidence

---

## Reference Files

For detailed guidance, consult:

- **references/finding-matching.md** - Strategies for matching commits to findings
- **references/bug-detection.md** - Comprehensive anti-pattern detection
- **references/report-parsing.md** - Parsing different report formats, Google Drive fallback