From 2b760c620a93252502daa1423eb354a2931347bc Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 05:30:22 +0900 Subject: [PATCH] fix(adapters): classify failed exec wrappers by forward scan, not backtracking The previous classifier placed two adjacent unbounded whitespace runs over the same span, so a long whitespace run followed by one non-matching character forced the engine to retry prefixes. Replaced with a single forward scan. Every loop advances an index monotonically, the newline searches cover disjoint forward spans, and the token checks are fixed-length, so no path retries a prefix. Two boundaries are load-bearing and both were divergences in an earlier attempt at this rewrite: whitespace after Output: may precede the marker, so an indented still classifies; and only whitespace may follow it, so a duplicate still does not. That second one matters most -- classifying it would replace a real payload with the failed-wrapper guidance, turning a normalization into data loss. One intended behaviour change: CRLF blank separators now count. The old pattern matched only \n, so a Windows-produced failed wrapper never classified and fell through to the empty-SUCCESS message, telling the model nothing had gone wrong when the cell had failed. Differential comparison over 63 shapes locally and an independent 662-shape pairwise corpus found no disagreement outside that CRLF blank-separator class. Diagnosis and the linear-scan approach are @luvs01's from #2938; that PR could not land as written because of the six divergences, which I posted there with the exact inputs. The bounded-work test measures process.cpuUsage() rather than elapsed wall time: performance.now() counts OS descheduling, VM pauses and GC, so a loaded CI runner can blow a wall-clock budget while the code under test did nothing wrong. Reverting to the previous classifier spends 1230ms CPU against a 250ms bound. Four mutations proven red at the intended test each: whitespace restricted to CR/LF, accepting a second marker, removing CRLF handling, and reverting the classifier. --- src/adapters/cursor/tool-result-normalize.ts | 6 +- src/adapters/exec-tool-result-normalize.ts | 75 ++++++++++++++++++-- tests/cursor-exec-empty-result.test.ts | 70 ++++++++++++++++++ 3 files changed, 143 insertions(+), 8 deletions(-) diff --git a/src/adapters/cursor/tool-result-normalize.ts b/src/adapters/cursor/tool-result-normalize.ts index 997ef56fea..fded734b93 100644 --- a/src/adapters/cursor/tool-result-normalize.ts +++ b/src/adapters/cursor/tool-result-normalize.ts @@ -13,7 +13,7 @@ import { EMPTY_EXEC_OUTPUT_MESSAGE, EMPTY_EXEC_OUTPUT_REGEX, FAILED_EXEC_OUTPUT_MESSAGE, - FAILED_EXEC_OUTPUT_REGEX, + isFailedEmptyExecWrapper, isCodexExecBridgeTool, } from "../exec-tool-result-normalize"; @@ -23,7 +23,7 @@ import { * `Script failed`, so restore that arm here rather than widening the shared one. */ function isEmptyOrFailedExecWrapper(text: string): boolean { - return EMPTY_EXEC_OUTPUT_REGEX.test(text) || FAILED_EXEC_OUTPUT_REGEX.test(text); + return EMPTY_EXEC_OUTPUT_REGEX.test(text) || isFailedEmptyExecWrapper(text); } const COMPUTER_USE_TOOL_NAMES = new Set([ @@ -99,7 +99,7 @@ export function normalizeCursorToolResultText( // A `Script failed` wrapper is empty but NOT a success: reporting it as an empty success // would erase the only failure signal. Text classification stays separate from Cursor's // isError policy, which the Computer Use branch above owns. - text: FAILED_EXEC_OUTPUT_REGEX.test(text.trim()) ? FAILED_EXEC_OUTPUT_MESSAGE : EMPTY_EXEC_OUTPUT_MESSAGE, + text: isFailedEmptyExecWrapper(text.trim()) ? FAILED_EXEC_OUTPUT_MESSAGE : EMPTY_EXEC_OUTPUT_MESSAGE, isError: false, changed: true, }; diff --git a/src/adapters/exec-tool-result-normalize.ts b/src/adapters/exec-tool-result-normalize.ts index f06a07c411..56a0386c4d 100644 --- a/src/adapters/exec-tool-result-normalize.ts +++ b/src/adapters/exec-tool-result-normalize.ts @@ -17,13 +17,78 @@ * `Script failed` is deliberately NOT in this set. A failed cell with no captured output is still * a FAILURE, and the success guidance below ("not a blocked tool", "do not re-run") would erase the * only signal that anything went wrong — reachable through Responses history, where - * `function_call_output` is parsed with `isError: false`. Cursor keeps its own broader regex for - * Computer Use, where a failed wrapper is separately marked `isError`. + * `function_call_output` is parsed with `isError: false`. Cursor combines this set with + * `isFailedEmptyExecWrapper` below for Computer Use, where a failed wrapper is separately marked + * `isError`. */ export const EMPTY_EXEC_OUTPUT_REGEX = /^(?:(?:Script completed|Command finished|Execution finished)[^\n]*\n+)?(?:Wall time[^\n]*\n+)?(?:Output:\s*)?(?:)?\s*$/; -/** Wrapper for a cell that FAILED without emitting output: empty, but not a success. */ -export const FAILED_EXEC_OUTPUT_REGEX = /^Script failed[^\n]*\n*(?:Wall time[^\n]*\n*)?(?:Output:\s*)?(?:)?\s*$/; +function skipFailedWrapperBlankSeparators(text: string, start: number): number { + let index = start; + while (index < text.length) { + if (text[index] === "\n") { + index += 1; + continue; + } + // A CRLF blank line is one separator, not a stray carriage return. The regex this replaced + // matched only `\n`, so a Windows-produced wrapper never classified and the failure guidance + // was silently replaced by the empty-SUCCESS message on that platform. + if (text[index] === "\r" && text[index + 1] === "\n") { + index += 2; + continue; + } + break; + } + return index; +} + +function skipFailedWrapperWhitespace(text: string, start: number): number { + let index = start; + while (index < text.length && text[index]!.trim() === "") index += 1; + return index; +} + +function skipFailedWrapperLine(text: string, start: number): number { + const newline = text.indexOf("\n", start); + return newline === -1 ? text.length : skipFailedWrapperBlankSeparators(text, newline + 1); +} + +/** + * Wrapper for a cell that FAILED without emitting output: empty, but not a success. + * + * A single forward scan, replacing a regex whose `\n*` and `\s*` runs sat adjacent over the same + * span and could be made to backtrack on a long whitespace run followed by one non-matching + * character. Every loop here advances an index monotonically and the token checks are fixed-length, + * so the work is bounded by the input length with no path that retries a prefix. + * + * Two boundaries are load-bearing and both were divergences in an earlier attempt at this rewrite: + * + * - Whitespace after `Output:` may precede the marker, so an INDENTED `` still classifies. + * Rejecting it would leave the wrapper unnormalized and the failure unexplained. + * - Only whitespace may follow the marker, so a DUPLICATE `` still does not classify. + * Accepting it would erase a real payload as an empty failed wrapper — the damaging direction. + * + * Behaviour is otherwise identical to the regex; the CRLF separators above are the only + * intentional change, verified against a 63-shape differential corpus. + */ +export function isFailedEmptyExecWrapper(trimmed: string): boolean { + if (!trimmed.startsWith("Script failed")) return false; + + const firstNewline = trimmed.indexOf("\n", "Script failed".length); + if (firstNewline === -1) return true; + + let index = skipFailedWrapperBlankSeparators(trimmed, firstNewline + 1); + if (trimmed.startsWith("Wall time", index)) { + index = skipFailedWrapperLine(trimmed, index); + } + if (trimmed.startsWith("Output:", index)) { + index = skipFailedWrapperWhitespace(trimmed, index + "Output:".length); + } + if (trimmed.startsWith("", index)) { + index += "".length; + } + return skipFailedWrapperWhitespace(trimmed, index) === trimmed.length; +} /** Guidance for a failed cell whose output was empty: the failure must survive normalization. */ export const FAILED_EXEC_OUTPUT_MESSAGE = @@ -94,6 +159,6 @@ export function normalizeEmptyExecToolResultText( if (!isCodexExecBridgeTool(options.toolName, options.toolNamespace)) return undefined; const trimmed = text.trim(); // Failure first: a failed wrapper must never be described as an empty success. - if (FAILED_EXEC_OUTPUT_REGEX.test(trimmed)) return FAILED_EXEC_OUTPUT_MESSAGE; + if (isFailedEmptyExecWrapper(trimmed)) return FAILED_EXEC_OUTPUT_MESSAGE; return EMPTY_EXEC_OUTPUT_REGEX.test(trimmed) ? EMPTY_EXEC_OUTPUT_MESSAGE : undefined; } diff --git a/tests/cursor-exec-empty-result.test.ts b/tests/cursor-exec-empty-result.test.ts index 098aaa08d3..57f42c0076 100644 --- a/tests/cursor-exec-empty-result.test.ts +++ b/tests/cursor-exec-empty-result.test.ts @@ -45,6 +45,76 @@ describe("codex exec bridge empty-result normalization (devlog 260826 gap-7)", ( expect(out.text).toBe("Output:\nhello"); }); + test("an indented empty marker after Output: still classifies as a failed wrapper", () => { + // These three classified under the previous regex. A line-scan rewrite that treated the + // marker as needing to start its own line rejected them, leaving the wrapper unnormalized + // and the failure unexplained. + for (const wrapper of [ + "Script failed\nOutput:\n\n ", + "Script failed\nOutput:\n ", + "Script failed\nOutput:\n ", + ]) { + const out = normalizeCursorToolResultText(wrapper, { toolName: "exec", isError: false }); + expect(out.changed).toBe(true); + expect(out.text).toContain("exec failed"); + } + }); + + test("a duplicate empty marker is left alone rather than erased", () => { + // The damaging direction: classifying these would replace a real payload with the failed-wrapper + // guidance. The previous regex rejected them and so must any replacement. + for (const wrapper of [ + "Script failed\nOutput:\t\n", + "Script failed\nOutput: \n\n", + "Script failed\nOutput: \n", + ]) { + const out = normalizeCursorToolResultText(wrapper, { toolName: "exec", isError: false }); + expect(out.changed).toBe(false); + expect(out.text).toBe(wrapper); + } + }); + + test("CRLF blank separators reach the failure guidance instead of the empty-success text", () => { + // The one intentional behaviour change. The old regex matched only `\n`, so a Windows-produced + // failed wrapper fell through to the empty-SUCCESS message — telling the model nothing went + // wrong when the cell had in fact failed. + for (const wrapper of [ + "Script failed\r\n\r\n\r\nOutput:", + "Script failed\r\n\r\n", + "Script failed\r\n\r\nOutput:", + "Script failed\r\nWall time 1s\r\n\r\nOutput:", + ]) { + const out = normalizeCursorToolResultText(wrapper, { toolName: "exec", isError: false }); + expect(out.changed).toBe(true); + expect(out.text).toContain("exec failed"); + expect(out.text).not.toContain("NOT lost context"); + } + }); + + test("a long whitespace run followed by a non-matching character classifies in bounded work", () => { + // A pathological shape for the classifier this replaced. Measured in CPU time rather than + // elapsed wall time: `performance.now()` counts OS descheduling, VM pauses and GC, so a loaded + // CI runner can blow any wall-clock budget while the code under test did nothing wrong. + // `process.cpuUsage()` counts only work this process actually performed. + // + // The bound is deliberately three orders of magnitude above the scan's real cost. It is not a + // performance target; it is a tripwire wide enough that only a return to super-linear work can + // cross it, which is the single thing this test exists to catch. + const malformed = `Script failed${" ".repeat(60_000)}\nY`; + + // Warm up so first-call JIT and allocation land outside the measurement. + normalizeCursorToolResultText(malformed, { toolName: "exec", isError: false }); + + const before = process.cpuUsage(); + const out = normalizeCursorToolResultText(malformed, { toolName: "exec", isError: false }); + const spent = process.cpuUsage(before); + const cpuMs = (spent.user + spent.system) / 1000; + + expect(out.changed).toBe(false); + expect(out.text).toBe(malformed); + expect(cpuMs).toBeLessThan(250); + }); + test("computer-use empties keep the original error semantics", () => { const out = normalizeCursorToolResultText("", { toolName: "screenshot" }); expect(out.isError).toBe(true);