fix: use original title from webhook payload instead of fetched title

[ashwin-ant]

Summary

Use the original PR/issue title from the webhook payload instead of the title fetched via GraphQL. This ensures the title used in prompts is the one that existed when the trigger event occurred.

Changes

• Add extractOriginalTitle() helper to extract title from webhook payload for all entity event types
• Add originalTitle parameter to fetchGitHubData() that overrides the fetched title
• Update tag mode to extract and pass the original title from webhook context
• Add tests for extractOriginalTitle and the originalTitle parameter

Testing

• Added 6 tests for extractOriginalTitle() covering all event types
• Added 2 tests for fetchGitHubData() verifying title override behavior
• All 586 tests pass

[claude]

Code Quality: Inconsistent optional chaining pattern

This function uses optional chaining (context.payload.issue?.title) while the similar extractTriggerTimestamp() function (lines 31-43) doesn't. This creates ambiguity about whether these fields are guaranteed to exist after the type guard checks.

Consider standardizing the approach:

• If fields are guaranteed by type guards, remove optional chaining from both functions
• If they might be missing, add optional chaining to extractTriggerTimestamp() as well

[claude]

Security: Missing sanitization for title field

The title is assigned without sanitization, but other user-controlled content (bodies, comments) goes through sanitizeContent() in formatter.ts. While GitHub webhook payloads are generally trusted, a malicious actor with write access could craft titles with:

• Invisible Unicode characters (zero-width spaces, bidirectional overrides)
• Markdown formatting exploits
• Prompt injection attempts

Recommendation: Apply sanitizeContent() to titles in formatContext() (formatter.ts:17, 27) to maintain defense-in-depth, especially since this PR specifically addresses security concerns around title handling.

Reference: CWE-74 (Improper Neutralization of Special Elements)

[ddworken]

Seems worth fixing?

[claude]

Test Coverage: Missing security validation test

Consider adding a test that explicitly validates the TOCTOU prevention use case:

it("should use original title from webhook even if title was edited after trigger", async () => {
  // Scenario: 
  // 1. PR created with title "Original Title"
  // 2. @claude triggered at timestamp T
  // 3. Attacker edits title to "Malicious Title" after T
  // 4. GraphQL would fetch "Malicious Title" but originalTitle should override it
  
  const mockOctokits = {
    graphql: jest.fn().mockResolvedValue({
      repository: {
        pullRequest: {
          title: "Malicious Title (edited after trigger)", // GraphQL fetched
          // ... other fields
        },
      },
    }),
  };

  const result = await fetchGitHubData({
    octokits: mockOctokits as any,
    repository: "test-owner/test-repo",
    prNumber: "123",
    isPR: true,
    originalTitle: "Original Title (from webhook at trigger time)",
  });

  expect(result.contextData.title).toBe("Original Title (from webhook at trigger time)");
});

This would explicitly document and validate the security benefit this PR provides.

[claude]

Code Review Summary

I've completed a comprehensive review of this PR using specialized review agents for code quality, performance, security, documentation, and test coverage. Overall, the implementation is solid and well-tested, but there are a few noteworthy items to address:

Key Findings

Security (Medium Priority):

• The title field should be sanitized before use in prompts, similar to how bodies and comments are sanitized. See inline comment on fetcher.ts:386-389

Documentation:

• The fetchGitHubData() function needs JSDoc documentation, especially for the new originalTitle parameter and its security benefits

Code Quality:

• Inconsistent optional chaining pattern between extractOriginalTitle() and extractTriggerTimestamp(). See inline comment on fetcher.ts:52-68

Test Coverage:

• Consider adding an explicit TOCTOU security validation test. See inline comment on data-fetcher.test.ts:1026-1061

Positive Aspects

• Excellent test coverage (8 new tests, all passing)
• No performance concerns - the changes are efficient and add no overhead
• Clean implementation that follows existing patterns
• Addresses a real security concern (TOCTOU attacks on title changes)

Recommendations

1. Apply sanitizeContent() to titles in formatContext() (formatter.ts)
2. Add JSDoc to fetchGitHubData() documenting the new parameter
3. Standardize the optional chaining approach across similar extraction functions
4. Consider adding the suggested security test to explicitly document the TOCTOU prevention

Overall, this is a well-implemented fix for a legitimate security concern. The main recommendation is to extend the existing sanitization practices to include title fields for defense-in-depth.