Coderabbit Fix

---
name: coderabbit-fix
description: Implement fixes for specific CodeRabbit review issues. Runs in isolated subagent context with focused task. Verifies fixes with tests before returning. Use one per issue from triage task list.
---

# Fixing Issues

## Overview

This skill implements a single, focused fix from a CodeRabbit issue. It runs in an isolated subagent context with clear constraints to prevent scope creep. Each subagent fixes exactly one issue and nothing more.

**Input**: Task object from `coderabbit-triage` (task_id, file, line, issue, instructions, constraints)

**Output**: Fixed code + detailed summary of changes and verification

**When to use**: Called once per task from the triage plan. Can run in parallel with other instances of this skill.

## Process

### Step 1: Understand the Issue

Before implementing, fully understand the problem:

1. **Read the issue description** from task input
2. **Read CodeRabbit's suggestion** carefully
3. **Open the file** and read the problematic code section (include 5 lines before and after)
4. **Ask yourself**:
   - Why is this broken?
   - What are the consequences if not fixed?
   - What does the suggested fix address?
5. **Identify the root cause**, not just the symptom

### Step 2: Evaluate Suggestion (Technical Rigor)

Don't blindly follow CodeRabbit's suggestion. Verify it:

```text
CodeRabbit suggests: "Add HMAC-SHA256 signature verification"

Questions to ask:
- Is HMAC-SHA256 the right algorithm? (Yes, industry standard for webhooks)
- Are there edge cases? (Replay attacks, timing attacks, key rotation)
- Is the suggestion complete? (Includes replay prevention? Yes, via timestamp)
- Are there better approaches? (Could use RS256? Less suitable here, HMAC is correct)
- Will this break existing behavior? (No, adds validation, doesn't change behavior)

Decision: ✅ Implement as suggested, plus timing attack protection via timingSafeEqual
```

If suggestion seems incomplete or wrong:

- Document your concern
- Propose alternative with reasoning
- Example: "CodeRabbit suggests simple cache, but recommend Redis for distributed systems"

### Step 3: Implement Fix (With Rigor)

Follow the fixer_instructions from your task exactly.

**For critical issues** (security, correctness):

- Add comprehensive error handling
- Add defensive checks (don't assume inputs are valid)
- Include security-focused logging
- Handle edge cases explicitly

Example: Signature verification

```typescript
// ✅ Correct: Defensive, handles all cases
function verifyWebhookSignature(request) {
  // Defensive: Check all required inputs
  if (!request.headers["x-webhook-signature"]) {
    logger.warn({ event: "signature_missing", webhook_id: request.id });
    throw new UnauthorizedError("Missing signature header");
  }

  if (!request.body) {
    logger.warn({ event: "body_empty", webhook_id: request.id });
    throw new BadRequestError("Empty webhook body");
  }

  // Compute signature with constant-time comparison
  const expectedSignature = crypto
    .createHmac("sha256", process.env.WEBHOOK_SECRET)
    .update(request.body)
    .digest("hex");

  const providedSignature = request.headers["x-webhook-signature"];

  // Use timing-safe comparison to prevent timing attacks
  if (!crypto.timingSafeEqual(Buffer.from(expectedSignature), Buffer.from(providedSignature))) {
    logger.warn({
      event: "signature_invalid",
      webhook_id: request.id,
      expected_length: expectedSignature.length,
      provided_length: providedSignature.length,
    });
    throw new UnauthorizedError("Invalid webhook signature");
  }

  return true;
}
```

**For important issues** (bugs, performance):

- Fix the immediate problem
- Add comments explaining the fix
- Verify related functionality doesn't break

**For minor issues** (style, clarity):

- Make focused change
- Don't refactor unnecessarily
- Preserve existing patterns

### Step 4: Add Safety Checks

Think about what can go wrong:

**Signature verification**:

- ✅ Empty header → return 401
- ✅ Timing attacks → use timingSafeEqual
- ✅ Signature mismatch → log + return 401
- ✅ Missing secret → throw at startup

**Idempotency handling**:

- ✅ Missing idempotency key → generate UUID
- ✅ Duplicate key → return cached result
- ✅ Cache grows unbounded → implement TTL cleanup
- ✅ Concurrent identical keys → use Promise deduplication

**Error logging**:

- ✅ Sensitive data → filter from logs
- ✅ PII in error messages → redact
- ✅ Log levels correct → info/warn/error appropriately

### Step 5: Test Locally

**Required tests**:

```bash
# 1. Run related tests first
npm test -- src/webhooks.test.ts

# 2. Check specific test cases pass
# For signature: test valid, invalid, missing, timing attack
# For idempotency: test duplicate keys, cache TTL, concurrent
# For logging: test all error types logged, no sensitive data

# 3. Run full suite
npm test

# 4. Check coverage hasn't decreased
npm test -- --coverage
```

**For critical issues**: All tests must pass. Zero tolerance.

**For important/minor issues**: All existing tests pass + new test for fix.

### Step 6: Verify with CodeRabbit (Optional)

If uncertain fix is complete, re-run CodeRabbit locally:

```bash
coderabbit --prompt-only --type diff HEAD~1
```

If CodeRabbit reports the issue still exists:

- Revisit the fix
- Check if it was actually applied
- Review if suggestion was incomplete

### Step 7: Report Back

Return summary in this format:

```jsonc
{
  "task_id": 1,
  "status": "✅ FIXED",
  "file": "src/webhooks.ts",
  "line": 42,
  "severity": "critical",
  "domain": "payment-webhook-security",

  "changes_summary": {
    "lines_added": 35,
    "lines_deleted": 2,
    "files_modified": ["src/webhooks.ts"]
  },

  "what_changed": [
    "Added HMAC-SHA256 signature verification at webhook handler entry",
    "Implemented replay attack protection via timestamp validation (5 minute window)",
    "Added timing-safe comparison to prevent timing attacks",
    "Added structured error logging for all signature failures",
    "Created verifyWebhookSignature() utility function"
  ],

  "why_this_approach": {
    "algorithm": "HMAC-SHA256 is industry standard for webhook security (see RFC 6234)",
    "timing_safe": "timingSafeEqual prevents timing attacks that could leak signature info",
    "replay_protection": "Timestamp validation (5 min window) prevents replay attacks if key is compromised",
    "error_logging": "Structured logs enable debugging in production without exposing secrets",
    "function_extraction": "Separate function makes testing and reuse possible"
  },

  "code_snippet": {
    "before": "function handleWebhook(req, res) { ... }",
    "after": "function handleWebhook(req, res) { verifyWebhookSignature(req); ... }"
  },

  "tests_passed": {
    "total": 8,
    "webhook_security_tests": "8/8 passing",
    "all_tests": "42/42 passing",
    "new_tests_added": [
      "✅ Valid signature verification succeeds",
      "✅ Invalid signature verification fails with 401",
      "✅ Missing signature verification fails with 401",
      "✅ Timing attack with modified signature fails",
      "✅ Stale timestamp (>5 min) fails with 401",
      "✅ Future timestamp fails with 401"
    ]
  },

  "quality_checklist": {
    "follows_task_instructions": true,
    "respects_constraints": true,
    "all_tests_passing": true,
    "no_new_warnings": true,
    "code_style_consistent": true,
    "error_handling_complete": true,
    "logging_appropriate": true,
    "security_hardened": true
  },

  "estimated_time": "12 minutes",
  "actual_time": "14 minutes",

  "ready_for_next_phase": true,

  "notes": "Implementation includes defense-in-depth: signature + timing attack prevention + replay protection + detailed logging. Aligns with OWASP webhook security guidelines."
}
```

## Key Principles

**Task isolation**: Fix only what's assigned. Don't improve other code.

**Technical rigor**: Understand root cause, not just symptoms. Question suggestions.

**Safety first**: Add defensive checks, error handling, comprehensive logging.

**Test evidence**: Don't claim it's fixed. Prove it with passing tests.

**Constraints matter**: Fixer_instructions and constraints are guardrails. Respect them.

## Common Patterns

**Signature verification**:

- Use crypto.timingSafeEqual
- Include replay attack prevention
- Log all failures
- Test timing attacks

**Idempotency**:

- Extract idempotency key early
- Check cache before processing
- Store result after processing
- Implement TTL-based cleanup

**Error logging**:

- Structured JSON logging
- Include request IDs for tracing
- Filter sensitive data
- Appropriate log levels

## Constraints (Universal)

- **DO**: Implement exactly as instructed
- **DO**: Add error handling and logging
- **DO**: Run all tests before returning
- **DO**: Include reasoning for technical decisions
- **DON'T**: Change unrequested code
- **DON'T**: Refactor or "improve" other functions
- **DON'T**: Remove existing error handling
- **DON'T**: Skip tests - prove it works
- **DON'T**: Ignore the constraints in your specific task

## Integration Points

```text
coderabbit-triage
         ↓
[dispatch multiple coderabbit-fix instances]
         ↓
coderabbit-fix #1 (parallel)
coderabbit-fix #2 (parallel)
coderabbit-fix #3 (parallel)
         ↓
Main agent collects summaries
         ↓
Final verification: full test suite + CodeRabbit re-check
```

## Error Handling

**If fix doesn't compile**:

- Report error immediately
- Include error message
- Ask for clarification on constraints

**If tests fail**:

- Debug systematically
- Include test output in report
- Don't return until tests pass

**If constraint violated**:

- Immediately notify
- Roll back changes
- Ask for revised instructions

Always provide evidence. Never claim success without proof.