Fix Review Skill

Overview

This skill implements Trail of Bits' fix review methodology for the agent-studio framework. When a security audit produces findings and developers commit fixes, this skill systematically verifies that each finding is properly addressed without introducing new vulnerabilities or regressions.

Source repository: https://github.com/trailofbits/skills

License: CC-BY-SA-4.0 Output: Structured FIX_REVIEW_REPORT.md

When to Use

• After developers commit fixes for security audit findings

• During PR review of security-related changes

• When verifying remediation of vulnerability reports

• Before closing security findings in tracking systems

• When reviewing patches for CVE remediations

• After automated security tool findings are addressed

Iron Law

NO FINDING CLOSED WITHOUT FIX VERIFICATION

A finding is not fixed until the fix has been reviewed against the original finding description, verified to address the root cause, and confirmed not to introduce new issues.

Fix Status Categories

Status Meaning

FIXED

Finding is fully addressed. Root cause eliminated. No regressions introduced.

PARTIALLY_FIXED

Some aspects addressed but gaps remain. More dangerous than unfixed.

NOT_ADDRESSED

Fix does not relate to the finding, or finding location unchanged.

CANNOT_DETERMINE

Insufficient context or code complexity prevents definitive assessment.

NEW_ISSUE

Fix introduces a new vulnerability or regression.

Anti-Pattern Detection

The following anti-patterns indicate potentially incorrect or incomplete fixes:

Anti-Pattern 1: Validation Removed

ALERT: Input validation was removed or weakened in the fix.

Indicators:

• Deleted input sanitization calls

• Relaxed regex patterns

• Removed length/type checks

• Weakened allowlist to blocklist

Anti-Pattern 2: Access Control Weakened

ALERT: Authorization checks were reduced or bypassed in the fix.

Indicators:

• Removed authentication middleware

• Changed role checks from strict to permissive

• Added bypass conditions to access control

• Removed rate limiting

Anti-Pattern 3: Error Handling Reduced

ALERT: Error handling was simplified or removed, potentially hiding failures.

Indicators:

• Replaced specific catch blocks with generic catch-all

• Added empty catch blocks

• Removed error logging

• Changed error responses from specific to generic (hiding useful diagnostics)

Anti-Pattern 4: External Call Reordering

ALERT: Order of external calls changed, potentially creating race conditions or TOCTOU.

Indicators:

• Authorization check moved after data access

• Validation moved after processing

• Lock acquisition moved or removed

• Transaction boundaries changed

Anti-Pattern 5: Integer Operation Changes

ALERT: Integer arithmetic modified, check for overflow/underflow/truncation.

Indicators:

• Changed integer types (int64 to int32, uint to int)

• Added/removed overflow checks

• Changed comparison operators (> to >=, == to !=)

• Modified loop bounds

Anti-Pattern 6: Crypto Downgrade

ALERT: Cryptographic operations were changed in a potentially weakening way.

Indicators:

• Algorithm changed (AES-256 to AES-128, SHA-256 to SHA-1)

• Key length reduced

• Removed salt/nonce/IV

• Changed from authenticated to unauthenticated encryption

Workflow

Step 1: Load Findings

Read the original findings report to understand what was found:

Read findings file

cat findings-report.md

Or extract from issue tracker

gh issue view <issue-number>

Step 2: Identify Fix Commits

Map each finding to its fix commit(s):

View commits since the findings report

git log --oneline --since="<audit-date>"

View a specific fix commit

git show <commit-hash>

View PR diff

gh pr diff <pr-number>

Step 3: Analyze Each Fix

For each finding-fix pair:

• Re-read the finding description and affected code location

• Read the fix diff line by line

• Check root cause: Does the fix address the root cause or just the symptom?

• Check completeness: Are all instances of the pattern fixed?

• Check anti-patterns: Does the fix match any anti-pattern?

• Check regressions: Does the fix break adjacent functionality?

• Assign status: FIXED / PARTIALLY_FIXED / NOT_ADDRESSED / CANNOT_DETERMINE / NEW_ISSUE

Step 4: Generate Report

FIX_REVIEW_REPORT Format

Fix Review Report

Date: YYYY-MM-DD Reviewer: [agent-type] Audit Reference: [original audit report path] Commits Reviewed: [list of commit hashes]

Summary

Finding Reviews

Finding F-001: [Title]

• Original Severity: Critical/High/Medium/Low/Informational
• Original Location: file:line
• Fix Commit: <hash>
• Fix Status: FIXED | PARTIALLY_FIXED | NOT_ADDRESSED | CANNOT_DETERMINE
• Anti-Patterns Detected: None | [list]

Original Finding Summary: [Brief description of the finding]

Fix Analysis: [Line-by-line analysis of what the fix does]

Root Cause Addressed: Yes/No/Partial [Explanation]

Regression Risk: None/Low/Medium/High [Explanation of potential regressions]

Remaining Gaps (if PARTIALLY_FIXED):

• Gap 1: [description]
• Gap 2: [description]

Finding F-002: [Title]

[Same structure as above]

New Issues Introduced

NI-001: [Title]

• Introduced By: <commit-hash>
• Location: file:line
• Severity: Critical/High/Medium/Low
• Description: [What the new issue is]
• Recommendation: [How to fix it]

Recommendations

1. [Prioritized list of remaining actions]
2. [Re-audit recommendations if needed]

Integration with Agent-Studio

Recommended Workflow

• Run audit-context-building to deeply analyze the code

• After fixes are committed, invoke fix-review to verify

• Feed report to code-reviewer for additional review perspectives

• Use variant-analysis to check if similar patterns exist elsewhere

Complementary Skills

Skill Relationship

audit-context-building

Provides deep context for understanding fixes

differential-review

Security-focused diff review (lower granularity)

variant-analysis

Finds unpatched variants of fixed vulnerabilities

static-analysis

Automated confirmation of fix effectiveness

code-reviewer

General code quality review of fix commits

Iron Laws

• NO FINDING CLOSED WITHOUT FIX VERIFICATION — A finding is not fixed until the fix has been reviewed against the original finding, verified to address root cause, and confirmed not to introduce new issues.

• ALWAYS compare fix against original finding — reviewing the diff in isolation misses context; the fix must address the specific vulnerability described.

• ALWAYS check for all instances — if a pattern is fixed in one location, verify that all other instances of the same pattern are also fixed.

• NEVER close PARTIALLY_FIXED findings — partial fixes are more dangerous than unfixed findings because they create false confidence.

• ALWAYS check adjacent code paths — fixes that pass all anti-pattern checks can still introduce regressions in code that depends on the changed behavior.

Anti-Patterns

Anti-Pattern Why It Fails Correct Approach

Reviewing diff without reading finding Fix may address wrong issue or only surface symptom Always re-read original finding before reviewing fix diff

Closing finding after any code change Change may be unrelated or insufficient Verify fix status is FIXED with root cause elimination confirmed

Ignoring partial fixes PARTIALLY_FIXED is more dangerous than NOT_ADDRESSED (false security) Track remaining gaps explicitly; keep finding open

Skipping anti-pattern checklist Subtle regressions (weakened validation, reordered checks) go unseen Run all 6 anti-pattern checks on every fix diff

Not checking for variant vulnerabilities Same bug pattern likely exists elsewhere in codebase Invoke variant-analysis after confirming fix

Memory Protocol (MANDATORY)

Before starting:

Read .claude/context/memory/learnings.md

Check for:

• Original findings report and any prior fix review reports

• Known anti-pattern frequencies from previous fix reviews

• Previous fix quality patterns for this codebase

After completing:

• Anti-pattern frequency data -> .claude/context/memory/learnings.md

• Fix quality concern -> .claude/context/memory/issues.md

• Decision about finding status -> .claude/context/memory/decisions.md

ASSUME INTERRUPTION: If it's not in memory, it didn't happen.