fix: address PR review comments for GitHub issue comments feature

- Use GraphQL variables instead of string interpolation for safety
- Add cursor validation to prevent potential GraphQL injection
- Add 30s timeout for spawned gh process to prevent hanging
- Export ValidationComment and ValidationLinkedPR from validation-schema
- Remove duplicate interface definitions from validate-issue.ts
- Use ISO date format instead of locale-dependent toLocaleDateString()
- Reset error state when issue is deselected in useIssueComments hook

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Kacper
2025-12-28 22:40:37 +01:00
parent 97ae4b6362
commit 6bdac230df
4 changed files with 58 additions and 42 deletions

View File

@@ -43,6 +43,16 @@ interface GraphQLResponse {
errors?: Array<{ message: string }>; errors?: Array<{ message: string }>;
} }
/** Timeout for GitHub API requests in milliseconds */
const GITHUB_API_TIMEOUT_MS = 30000;
/**
* Validate cursor format (GraphQL cursors are typically base64 strings)
*/
function isValidCursor(cursor: string): boolean {
return /^[A-Za-z0-9+/=]+$/.test(cursor);
}
/** /**
* Fetch comments for a specific issue using GitHub GraphQL API * Fetch comments for a specific issue using GitHub GraphQL API
*/ */
@@ -53,33 +63,45 @@ async function fetchIssueComments(
issueNumber: number, issueNumber: number,
cursor?: string cursor?: string
): Promise<IssueCommentsResult> { ): Promise<IssueCommentsResult> {
const cursorParam = cursor ? `, after: "${cursor}"` : ''; // Validate cursor format to prevent potential injection
if (cursor && !isValidCursor(cursor)) {
throw new Error('Invalid cursor format');
}
const query = `{ // Use GraphQL variables instead of string interpolation for safety
repository(owner: "${owner}", name: "${repo}") { const query = `
issue(number: ${issueNumber}) { query GetIssueComments($owner: String!, $repo: String!, $issueNumber: Int!, $cursor: String) {
comments(first: 50${cursorParam}) { repository(owner: $owner, name: $repo) {
totalCount issue(number: $issueNumber) {
pageInfo { comments(first: 50, after: $cursor) {
hasNextPage totalCount
endCursor pageInfo {
} hasNextPage
nodes { endCursor
id }
author { nodes {
login id
avatarUrl author {
login
avatarUrl
}
body
createdAt
updatedAt
} }
body
createdAt
updatedAt
} }
} }
} }
} }`;
}`;
const requestBody = JSON.stringify({ query }); const variables = {
owner,
repo,
issueNumber,
cursor: cursor || null,
};
const requestBody = JSON.stringify({ query, variables });
const response = await new Promise<GraphQLResponse>((resolve, reject) => { const response = await new Promise<GraphQLResponse>((resolve, reject) => {
const gh = spawn('gh', ['api', 'graphql', '--input', '-'], { const gh = spawn('gh', ['api', 'graphql', '--input', '-'], {
@@ -87,12 +109,19 @@ async function fetchIssueComments(
env: execEnv, env: execEnv,
}); });
// Add timeout to prevent hanging indefinitely
const timeoutId = setTimeout(() => {
gh.kill();
reject(new Error('GitHub API request timed out'));
}, GITHUB_API_TIMEOUT_MS);
let stdout = ''; let stdout = '';
let stderr = ''; let stderr = '';
gh.stdout.on('data', (data: Buffer) => (stdout += data.toString())); gh.stdout.on('data', (data: Buffer) => (stdout += data.toString()));
gh.stderr.on('data', (data: Buffer) => (stderr += data.toString())); gh.stderr.on('data', (data: Buffer) => (stderr += data.toString()));
gh.on('close', (code) => { gh.on('close', (code) => {
clearTimeout(timeoutId);
if (code !== 0) { if (code !== 0) {
return reject(new Error(`gh process exited with code ${code}: ${stderr}`)); return reject(new Error(`gh process exited with code ${code}: ${stderr}`));
} }

View File

@@ -21,6 +21,8 @@ import {
issueValidationSchema, issueValidationSchema,
ISSUE_VALIDATION_SYSTEM_PROMPT, ISSUE_VALIDATION_SYSTEM_PROMPT,
buildValidationPrompt, buildValidationPrompt,
ValidationComment,
ValidationLinkedPR,
} from './validation-schema.js'; } from './validation-schema.js';
import { import {
trySetValidationRunning, trySetValidationRunning,
@@ -35,24 +37,6 @@ import { getAutoLoadClaudeMdSetting } from '../../../lib/settings-helpers.js';
/** Valid model values for validation */ /** Valid model values for validation */
const VALID_MODELS: readonly AgentModel[] = ['opus', 'sonnet', 'haiku'] as const; const VALID_MODELS: readonly AgentModel[] = ['opus', 'sonnet', 'haiku'] as const;
/**
* Comment structure for validation prompt
*/
interface ValidationComment {
author: string;
createdAt: string;
body: string;
}
/**
* Linked PR structure for validation prompt
*/
interface ValidationLinkedPR {
number: number;
title: string;
state: string;
}
/** /**
* Request body for issue validation * Request body for issue validation
*/ */

View File

@@ -155,7 +155,7 @@ Be thorough in your analysis but focus on files that are directly relevant to th
/** /**
* Comment data structure for validation prompt * Comment data structure for validation prompt
*/ */
interface ValidationComment { export interface ValidationComment {
author: string; author: string;
createdAt: string; createdAt: string;
body: string; body: string;
@@ -164,7 +164,7 @@ interface ValidationComment {
/** /**
* Linked PR data structure for validation prompt * Linked PR data structure for validation prompt
*/ */
interface ValidationLinkedPR { export interface ValidationLinkedPR {
number: number; number: number;
title: string; title: string;
state: string; state: string;
@@ -207,7 +207,9 @@ export function buildValidationPrompt(
// Limit to most recent 10 comments to control prompt size // Limit to most recent 10 comments to control prompt size
const recentComments = comments.slice(-10); const recentComments = comments.slice(-10);
const commentsText = recentComments const commentsText = recentComments
.map((c) => `**${c.author}** (${new Date(c.createdAt).toLocaleDateString()}):\n${c.body}`) .map(
(c) => `**${c.author}** (${new Date(c.createdAt).toISOString().slice(0, 10)}):\n${c.body}`
)
.join('\n\n---\n\n'); .join('\n\n---\n\n');
commentsSection = `\n\n### Comments (${comments.length} total${comments.length > 10 ? ', showing last 10' : ''})\n\n${commentsText}`; commentsSection = `\n\n### Comments (${comments.length} total${comments.length > 10 ? ', showing last 10' : ''})\n\n${commentsText}`;

View File

@@ -101,6 +101,7 @@ export function useIssueComments(issueNumber: number | null): UseIssueCommentsRe
setHasNextPage(false); setHasNextPage(false);
setEndCursor(undefined); setEndCursor(undefined);
setLoading(false); setLoading(false);
setError(null);
} }
return () => { return () => {