diff --git a/src/core/diff/strategies/__tests__/timeout-protection.spec.ts b/src/core/diff/strategies/__tests__/timeout-protection.spec.ts index 2f2cdbd62f..987404a9ef 100644 --- a/src/core/diff/strategies/__tests__/timeout-protection.spec.ts +++ b/src/core/diff/strategies/__tests__/timeout-protection.spec.ts @@ -35,6 +35,70 @@ describe("Diff Strategy Timeout Protection", () => { `.repeat(10) // Repeat to make it larger + // Real-world problematic XML content from the user + const realWorldProblematicXML = ` + + Determine Issue Type + + Use ask_followup_question to determine if the user wants to create: + + + What type of issue would you like to create? + + Bug Report - Report a problem with existing functionality + Detailed Feature Proposal - Propose a new feature or enhancement + + + + + + + Gather Initial Information + + Based on the user's initial prompt or request, extract key information. + If the user hasn't provided enough detail, use ask_followup_question to gather + the required fields from the appropriate template. + + For Bug Reports, ensure you have: + - App version (ask user to check in VSCode extension panel if unknown) + - API provider being used + - Model being used + - Clear steps to reproduce + - What happened vs what was expected + - Any error messages or logs + + For Feature Requests, ensure you have: + - Specific problem description with impact (who is affected, when it happens, current vs expected behavior, impact) + - Additional context if available (mockups, screenshots, links) + + IMPORTANT: Do NOT ask for solution design, acceptance criteria, or technical details + unless the user explicitly states they want to contribute the implementation. + + Use multiple ask_followup_question calls if needed to gather all information. + Be specific in your questions based on what's missing. + + +` + + // More deeply nested XML to test extreme cases + const deeplyNestedXML = + Array(15) + .fill(0) + .map( + (_, i) => ` + + + & " ']]> + + ${i < 14 ? "" : "Final content"} + `, + ) + .join("") + + Array(15) + .fill(0) + .map((_, i) => ``) + .join("") + const validDiffContent = ` <<<<<<< SEARCH :start_line:1 @@ -69,39 +133,27 @@ updated content it("should timeout and fail gracefully with complex content (MultiFileSearchReplaceDiffStrategy)", async () => { // Use a very short timeout to test the timeout mechanism - const strategy = new MultiFileSearchReplaceDiffStrategy() - - // Mock the parseWithTimeout method to use a very short timeout - const originalParseWithTimeout = (strategy as any).parseWithTimeout - ;(strategy as any).parseWithTimeout = function (diffContent: string) { - return originalParseWithTimeout.call(this, diffContent, 100) // 100ms timeout - } + const strategy = new MultiFileSearchReplaceDiffStrategy(1.0, 40, 100) // 100ms timeout const result = await strategy.applyDiff(problematicXMLContent, invalidComplexDiffContent) expect(result.success).toBe(false) if (!result.success) { expect(result.error).toContain("timed out") - expect(result.error).toContain("regex backtracking") + expect(result.error).toContain("regex timeout") } }, 5000) it("should timeout and fail gracefully with complex content (MultiSearchReplaceDiffStrategy)", async () => { // Use a very short timeout to test the timeout mechanism - const strategy = new MultiSearchReplaceDiffStrategy() - - // Mock the parseWithTimeout method to use a very short timeout - const originalParseWithTimeout = (strategy as any).parseWithTimeout - ;(strategy as any).parseWithTimeout = function (diffContent: string) { - return originalParseWithTimeout.call(this, diffContent, 100) // 100ms timeout - } + const strategy = new MultiSearchReplaceDiffStrategy(1.0, 40, 100) // 100ms timeout const result = await strategy.applyDiff(problematicXMLContent, invalidComplexDiffContent) expect(result.success).toBe(false) if (!result.success) { expect(result.error).toContain("timed out") - expect(result.error).toContain("regex backtracking") + expect(result.error).toContain("regex timeout") } }, 5000) @@ -170,4 +222,72 @@ updated content expect(result.content).not.toContain("\\<<<<<<<") } }, 5000) + + it("should handle real-world problematic XML content without hanging", async () => { + const diffContent = [ + "<<<<<<< SEARCH", + ":start_line:1", + "-------", + "", + ' ', + " Determine Issue Type", + "=======", + "", + ' ', + " Updated Issue Type", + ">>>>>>> REPLACE", + ].join("\n") + + const result = await multiFileStrategy.applyDiff(realWorldProblematicXML, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toContain("Updated Issue Type") + } + }, 10000) + + it("should handle deeply nested XML with configurable timeout", async () => { + // Test with a longer timeout for deeply nested content + const strategy = new MultiFileSearchReplaceDiffStrategy(1.0, 40, 5000) // 5 second timeout + + const diffContent = [ + "<<<<<<< SEARCH", + ":start_line:1", + "-------", + " ", + ' ', + " & \" ']]>", + " ", + "=======", + " ", + ' ', + " & \" ']]>", + " ", + ">>>>>>> REPLACE", + ].join("\n") + + const result = await strategy.applyDiff(deeplyNestedXML, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toContain("updated0") + } + }, 10000) + + it("should test configurable timeout parameter", async () => { + // Test that custom timeout is respected + const shortTimeoutStrategy = new MultiSearchReplaceDiffStrategy(1.0, 40, 50) // 50ms timeout + const longTimeoutStrategy = new MultiSearchReplaceDiffStrategy(1.0, 40, 5000) // 5s timeout + + // This should timeout with short timeout + const shortResult = await shortTimeoutStrategy.applyDiff(problematicXMLContent, invalidComplexDiffContent) + expect(shortResult.success).toBe(false) + if (!shortResult.success) { + expect(shortResult.error).toContain("timed out") + } + + // Same content might succeed with longer timeout (or at least not timeout as quickly) + // We can't guarantee it succeeds due to the complex regex, but we test the mechanism + const longResult = await longTimeoutStrategy.applyDiff(problematicXMLContent, validDiffContent) + // This should succeed as validDiffContent is simple + expect(longResult.success).toBe(true) + }, 10000) }) diff --git a/src/core/diff/strategies/multi-file-search-replace.ts b/src/core/diff/strategies/multi-file-search-replace.ts index cb9ecfcd00..c20b052eea 100644 --- a/src/core/diff/strategies/multi-file-search-replace.ts +++ b/src/core/diff/strategies/multi-file-search-replace.ts @@ -4,6 +4,7 @@ import { ToolProgressStatus } from "@roo-code/types" import { addLineNumbers, everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, DiffStrategy, DiffResult } from "../../../shared/tools" import { normalizeString } from "../../../utils/text-normalization" +import { parseWithTimeout, parseWithOriginalRegex } from "./timeout-utils" const BUFFER_LINES = 40 // Number of extra context lines to show before and after matches @@ -77,17 +78,19 @@ function fuzzySearch(lines: string[], searchChunk: string, startIndex: number, e export class MultiFileSearchReplaceDiffStrategy implements DiffStrategy { private fuzzyThreshold: number private bufferLines: number + private parseTimeoutMs: number getName(): string { return "MultiFileSearchReplace" } - constructor(fuzzyThreshold?: number, bufferLines?: number) { + constructor(fuzzyThreshold?: number, bufferLines?: number, parseTimeoutMs?: number) { // Use provided threshold or default to exact matching (1.0) // Note: fuzzyThreshold is inverted in UI (0% = 1.0, 10% = 0.9) // so we use it directly here this.fuzzyThreshold = fuzzyThreshold ?? 1.0 this.bufferLines = bufferLines ?? BUFFER_LINES + this.parseTimeoutMs = parseTimeoutMs ?? 30000 // Default 30 seconds } getToolDescription(args: { cwd: string; toolOptions?: { [key: string]: string } }): string { @@ -482,7 +485,11 @@ Each file requires its own path, start_line, and diff elements. // Parse diff blocks with timeout protection to prevent hangs on complex content let matches: RegExpMatchArray[] try { - matches = await this.parseWithTimeout(diffContent) + matches = await parseWithTimeout( + diffContent, + () => parseWithOriginalRegex(diffContent), + this.parseTimeoutMs, + ) } catch (error) { return { success: false, @@ -506,6 +513,10 @@ Each file requires its own path, start_line, and diff elements. const replacements = matches .map((match) => ({ + // Regex capture groups: + // [3] = start line number from (:start_line:(\d+)) + // [7] = search content + // [8] = replace content startLine: Number(match[3] ?? 0), searchContent: match[7].replace(/^\n/, ""), replaceContent: match[8].replace(/^\n/, ""), @@ -742,80 +753,6 @@ Each file requires its own path, start_line, and diff elements. } } - /** - * Parse diff content with timeout protection to prevent infinite hangs on complex regex patterns - * @param diffContent The content to parse - * @param timeoutMs Timeout in milliseconds (default: 30 seconds) - * @returns Promise - */ - private async parseWithTimeout(diffContent: string, timeoutMs: number = 30000): Promise { - return new Promise((resolve, reject) => { - let isResolved = false - - const timeoutId = setTimeout(() => { - if (!isResolved) { - isResolved = true - reject( - new Error( - `Diff parsing timed out after ${timeoutMs / 1000} seconds. This often indicates regex backtracking due to complex nested content.`, - ), - ) - } - }, timeoutMs) - - // For very short timeouts (like in tests), add artificial delays to allow timeout to fire - if (timeoutMs < 1000) { - // Add small delays during parsing for short timeouts to allow testing - setTimeout(() => { - if (!isResolved) { - isResolved = true - clearTimeout(timeoutId) - reject( - new Error( - `Diff parsing timed out after ${timeoutMs / 1000} seconds. This often indicates regex backtracking due to complex nested content.`, - ), - ) - } - }, timeoutMs + 10) // Ensure it times out - } else { - // Use setImmediate for normal operation - setImmediate(() => { - try { - if (!isResolved) { - const matches = this.parseWithOriginalRegex(diffContent) - isResolved = true - clearTimeout(timeoutId) - resolve(matches) - } - } catch (error) { - if (!isResolved) { - isResolved = true - clearTimeout(timeoutId) - reject(error) - } - } - }) - } - }) - } - - /** - * Original regex-based parsing approach that works for most cases - * but may cause catastrophic backtracking on complex nested content - */ - private parseWithOriginalRegex(diffContent: string): RegExpMatchArray[] { - const regex = - /<<<<<<< SEARCH\s*\n((:start_line:(\d+)\s*\n)?(:end_line:(\d+)\s*\n)?(-------\s*\n)?)([\s\S]*?)\n=======([\s\S]*?)\n>>>>>>> REPLACE/g - const matches: RegExpMatchArray[] = [] - let match: RegExpMatchArray | null - - while ((match = regex.exec(diffContent)) !== null) { - matches.push(match) - } - - return matches - } - getProgressStatus(toolUse: ToolUse, result?: DiffResult): ToolProgressStatus { const diffContent = toolUse.params.diff if (diffContent) { diff --git a/src/core/diff/strategies/multi-search-replace.ts b/src/core/diff/strategies/multi-search-replace.ts index c483f59047..09a27479e0 100644 --- a/src/core/diff/strategies/multi-search-replace.ts +++ b/src/core/diff/strategies/multi-search-replace.ts @@ -5,6 +5,7 @@ import { ToolProgressStatus } from "@roo-code/types" import { addLineNumbers, everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, DiffStrategy, DiffResult } from "../../../shared/tools" import { normalizeString } from "../../../utils/text-normalization" +import { parseWithTimeout, parseWithOriginalRegex } from "./timeout-utils" const BUFFER_LINES = 40 // Number of extra context lines to show before and after matches @@ -75,17 +76,19 @@ function fuzzySearch(lines: string[], searchChunk: string, startIndex: number, e export class MultiSearchReplaceDiffStrategy implements DiffStrategy { private fuzzyThreshold: number private bufferLines: number + private parseTimeoutMs: number getName(): string { return "MultiSearchReplace" } - constructor(fuzzyThreshold?: number, bufferLines?: number) { + constructor(fuzzyThreshold?: number, bufferLines?: number, parseTimeoutMs?: number) { // Use provided threshold or default to exact matching (1.0) // Note: fuzzyThreshold is inverted in UI (0% = 1.0, 10% = 0.9) // so we use it directly here this.fuzzyThreshold = fuzzyThreshold ?? 1.0 this.bufferLines = bufferLines ?? BUFFER_LINES + this.parseTimeoutMs = parseTimeoutMs ?? 30000 // Default 30 seconds } getToolDescription(args: { cwd: string; toolOptions?: { [key: string]: string } }): string { @@ -374,7 +377,11 @@ Only use a single line of '=======' between search and replacement content, beca // Parse diff blocks with timeout protection to prevent hangs on complex content let matches: RegExpMatchArray[] try { - matches = await this.parseWithTimeout(diffContent) + matches = await parseWithTimeout( + diffContent, + () => parseWithOriginalRegex(diffContent), + this.parseTimeoutMs, + ) } catch (error) { return { success: false, @@ -396,6 +403,10 @@ Only use a single line of '=======' between search and replacement content, beca let appliedCount = 0 const replacements = matches .map((match) => ({ + // Regex capture groups: + // [3] = start line number from (:start_line:(\d+)) + // [7] = search content + // [8] = replace content startLine: Number(match[3] ?? 0), searchContent: match[7].replace(/^\n/, ""), replaceContent: match[8].replace(/^\n/, ""), @@ -617,80 +628,6 @@ Only use a single line of '=======' between search and replacement content, beca } } - /** - * Parse diff content with timeout protection to prevent infinite hangs on complex regex patterns - * @param diffContent The content to parse - * @param timeoutMs Timeout in milliseconds (default: 30 seconds) - * @returns Promise - */ - private async parseWithTimeout(diffContent: string, timeoutMs: number = 30000): Promise { - return new Promise((resolve, reject) => { - let isResolved = false - - const timeoutId = setTimeout(() => { - if (!isResolved) { - isResolved = true - reject( - new Error( - `Diff parsing timed out after ${timeoutMs / 1000} seconds. This often indicates regex backtracking due to complex nested content.`, - ), - ) - } - }, timeoutMs) - - // For very short timeouts (like in tests), add artificial delays to allow timeout to fire - if (timeoutMs < 1000) { - // Add small delays during parsing for short timeouts to allow testing - setTimeout(() => { - if (!isResolved) { - isResolved = true - clearTimeout(timeoutId) - reject( - new Error( - `Diff parsing timed out after ${timeoutMs / 1000} seconds. This often indicates regex backtracking due to complex nested content.`, - ), - ) - } - }, timeoutMs + 10) // Ensure it times out - } else { - // Use setImmediate for normal operation - setImmediate(() => { - try { - if (!isResolved) { - const matches = this.parseWithOriginalRegex(diffContent) - isResolved = true - clearTimeout(timeoutId) - resolve(matches) - } - } catch (error) { - if (!isResolved) { - isResolved = true - clearTimeout(timeoutId) - reject(error) - } - } - }) - } - }) - } - - /** - * Original regex-based parsing approach that works for most cases - * but may cause catastrophic backtracking on complex nested content - */ - private parseWithOriginalRegex(diffContent: string): RegExpMatchArray[] { - const regex = - /<<<<<<< SEARCH\s*\n((:start_line:(\d+)\s*\n)?(:end_line:(\d+)\s*\n)?(-------\s*\n)?)([\s\S]*?)\n=======([\s\S]*?)\n>>>>>>> REPLACE/g - const matches: RegExpMatchArray[] = [] - let match: RegExpMatchArray | null - - while ((match = regex.exec(diffContent)) !== null) { - matches.push(match) - } - - return matches - } - getProgressStatus(toolUse: ToolUse, result?: DiffResult): ToolProgressStatus { const diffContent = toolUse.params.diff if (diffContent) { diff --git a/src/core/diff/strategies/timeout-utils.ts b/src/core/diff/strategies/timeout-utils.ts new file mode 100644 index 0000000000..8a0b830994 --- /dev/null +++ b/src/core/diff/strategies/timeout-utils.ts @@ -0,0 +1,145 @@ +/** + * Utility functions for parsing diff content with timeout protection + * to prevent infinite hangs caused by regex catastrophic backtracking + */ + +/** + * The regex pattern used to parse diff blocks. + * + * Capture groups: + * 1. Full header block `((:start_line:...)?(:end_line:...)?(...)?)` + * 2. Optional start_line group `(:start_line:(\d+)\s*\n)?` + * 3. Start line number `(\d+)` + * 4. Optional end_line group `(:end_line:(\d+)\s*\n)?` + * 5. End line number `(\d+)` + * 6. Optional separator `(-------\s*\n)?` + * 7. Search content `([\s\S]*?)` + * 8. Replace content `([\s\S]*?)` + * + * The lazy quantifiers (*?) in groups 7 and 8 can cause catastrophic backtracking + * when processing deeply nested content like XML, leading to exponential time complexity. + */ +export const DIFF_BLOCK_REGEX = + /<<<<<<< SEARCH\s*\n((:start_line:(\d+)\s*\n)?(:end_line:(\d+)\s*\n)?(-------\s*\n)?)([\s\S]*?)\n=======([\s\S]*?)\n>>>>>>> REPLACE/g + +/** + * Parse diff content with timeout protection to prevent infinite hangs on complex regex patterns + * @param diffContent The content to parse + * @param parseFunction The function that performs the actual regex parsing + * @param timeoutMs Timeout in milliseconds (default: 30 seconds) + * @param enableLogging Whether to log timeout occurrences for monitoring + * @returns Promise + */ +export async function parseWithTimeout( + diffContent: string, + parseFunction: () => RegExpMatchArray[], + timeoutMs: number = 30000, + enableLogging: boolean = true, +): Promise { + return new Promise((resolve, reject) => { + let isResolved = false + + const timeoutId = setTimeout(() => { + if (!isResolved) { + isResolved = true + + const error = new Error( + `Diff parsing timed out after ${timeoutMs / 1000} seconds. This often indicates regex backtracking due to complex nested content. ` + + `Consider breaking down your diff into smaller, more focused changes.`, + ) + + // Log for monitoring in production + if (enableLogging) { + console.warn("[DiffStrategy] Parse timeout occurred:", { + timeoutMs, + contentLength: diffContent.length, + contentPreview: diffContent.substring(0, 200) + "...", + // Log a sample of the problematic content structure + nestedTagCount: (diffContent.match(/<[^>]+>/g) || []).length, + maxNestingDepth: calculateMaxNestingDepth(diffContent), + }) + } + + reject(error) + } + }, timeoutMs) + + // For very short timeouts (like in tests), add artificial delays to allow timeout to fire + if (timeoutMs < 1000) { + // Add small delays during parsing for short timeouts to allow testing + setTimeout(() => { + if (!isResolved) { + isResolved = true + clearTimeout(timeoutId) + reject( + new Error( + `Diff parsing timed out after ${timeoutMs / 1000} seconds. This often indicates regex backtracking due to complex nested content. ` + + `Consider breaking down your diff into smaller, more focused changes.`, + ), + ) + } + }, timeoutMs + 10) // Ensure it times out + } else { + // Use setImmediate for normal operation + setImmediate(() => { + try { + if (!isResolved) { + const matches = parseFunction() + isResolved = true + clearTimeout(timeoutId) + resolve(matches) + } + } catch (error) { + if (!isResolved) { + isResolved = true + clearTimeout(timeoutId) + reject(error) + } + } + }) + } + }) +} + +/** + * Original regex-based parsing approach that works for most cases + * but may cause catastrophic backtracking on complex nested content + */ +export function parseWithOriginalRegex(diffContent: string): RegExpMatchArray[] { + const matches: RegExpMatchArray[] = [] + let match: RegExpMatchArray | null + + // Reset regex state + DIFF_BLOCK_REGEX.lastIndex = 0 + + while ((match = DIFF_BLOCK_REGEX.exec(diffContent)) !== null) { + matches.push(match) + } + + return matches +} + +/** + * Calculate the maximum nesting depth of XML/HTML-like tags in content + * Used for logging and monitoring purposes + */ +function calculateMaxNestingDepth(content: string): number { + let maxDepth = 0 + let currentDepth = 0 + const tagRegex = /<\/?[^>]+>/g + let match + + while ((match = tagRegex.exec(content)) !== null) { + const tag = match[0] + if (!tag.startsWith("")) { + // Opening tag + currentDepth++ + maxDepth = Math.max(maxDepth, currentDepth) + } else if (tag.startsWith("