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:

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

Step 2: Evaluate Suggestion (Technical Rigor)

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

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

// ✅ 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:

# 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:

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:

{
  "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

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.