diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c7d1c5de8f3..52243973a03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -611,12 +611,55 @@ jobs: source scripts/ci/bun-crash-signatures.sh run_macos_suite() { - local suite_log suite_status + local suite_log suite_status watchdog_pid suite_log="$(mktemp -t ocx-macos-suite.XXXXXX)" || return $? + + # --timeout 60000 bounds a test that is awaiting work; it cannot bound a + # wedged runner. A Bun isolate on this lane has been observed wedged + # inside spawnSync: the spawned child is already dead but the worker's + # private wait loop never polls its exit again, so the suite writes + # nothing further, the per-test ceiling fires inside the spin without + # breaking it, and only the job's timeout-minutes ends it -- twenty + # silent minutes later, on whichever file happened to be running. + # A suite making progress emits a result line inside every per-test + # window, so silence several times that window is a wedge, not slow + # work: kill Bun so the leg fails inside minutes, at the file the log + # last grouped. macOS has no timeout(1), so the bound is a growth poll + # on the log the suite already tees into. + ( + last_size=-1 + silent_for=0 + # A suite that finishes kills this subshell mid-`sleep`; the orphaned + # sleep would keep the step's output pipes open for its remaining run, + # so its descriptors are detached up front. + while sleep 15 /dev/null 2>&1; do + size="$(wc -c < "$suite_log" 2>/dev/null || echo 0)" + if [ "$size" -le "$last_size" ]; then + silent_for=$((silent_for + 15)) + else + silent_for=0 + last_size=$size + fi + if [ "$silent_for" -ge "${OCX_MACOS_SUITE_SILENCE_SECONDS:-300}" ]; then + echo "::error::macOS suite produced no output for ${silent_for}s -- a wedged isolate, not slow work (several times the 60s per-test ceiling). Killing Bun; the last ##[group] in this log names the file it stalled inside." + pkill -KILL -x bun 2>/dev/null || true + # tee's read ends only when every descendant that inherited the + # merged stream is dead; a lingering grandchild must not hang the + # leg a second time, so give EOF a moment, then reap tee itself. + sleep 5 /dev/null 2>&1 + pkill -KILL -f "tee $suite_log" 2>/dev/null || true + exit 0 + fi + done + ) & + watchdog_pid=$! + # The per-test ceiling applies to every invocation, including each isolated # serial file. One attempt, whatever the outcome. bun test --isolate --timeout 60000 "$@" 2>&1 | tee "$suite_log" suite_status="${PIPESTATUS[0]}" + kill "$watchdog_pid" 2>/dev/null + wait "$watchdog_pid" 2>/dev/null if [ "$suite_status" -eq 0 ]; then rm -f "$suite_log" return 0 diff --git a/src/responses/apply-patch-envelope.ts b/src/responses/apply-patch-envelope.ts index fdca20259bc..95a49d1f995 100644 --- a/src/responses/apply-patch-envelope.ts +++ b/src/responses/apply-patch-envelope.ts @@ -28,6 +28,11 @@ const FREEFORM_FALLBACK_KEYS: Readonly> = { apply_patch: ["patch", "content"], }; +/** The field names a tool's freeform wrapper may unwrap through, besides canonical `input`. */ +export function freeformFallbackKeys(toolName: string): readonly string[] { + return FREEFORM_FALLBACK_KEYS[toolName] ?? []; +} + function stripMarkdownCodeFence(text: string, toolName: string): string { if (toolName !== "exec" && toolName !== "apply_patch") return text; const match = OUTER_MARKDOWN_CODE_FENCE.exec(text.trim()); @@ -46,7 +51,7 @@ export function unwrapFreeformToolInput(argumentsText: unknown, toolName = ""): ? stripMarkdownCodeFence(record.input, toolName) : argumentsText; } - const fallbackKeys = FREEFORM_FALLBACK_KEYS[toolName] ?? []; + const fallbackKeys = freeformFallbackKeys(toolName); const candidates = fallbackKeys.filter(key => typeof record[key] === "string"); if (candidates.length === 1) { return stripMarkdownCodeFence(record[candidates[0]] as string, toolName); diff --git a/src/responses/freeform-wrapper-scan.ts b/src/responses/freeform-wrapper-scan.ts index 5ba91987e49..2d20fe7ff1d 100644 --- a/src/responses/freeform-wrapper-scan.ts +++ b/src/responses/freeform-wrapper-scan.ts @@ -19,7 +19,8 @@ // `raw` no wrapper can apply, because the text is not an object or because it is one that // `JSON.parse` will reject. Completion returns the buffer, so streaming it agrees. // `hold` undecided. A key that has not arrived yet can still change the answer, so nothing -// is published until the object parses and completion's own rule decides. +// is published until the object closes — where the scan itself already holds the +// member table completion's own rule consults, so no parse of the buffer is needed. // // The bound matters as much as the classification. A scan that walks the whole buffer on every // delta is quadratic in the argument size, so classification gives up after @@ -53,7 +54,7 @@ const HOLD = -1; const NEVER = -2; export type FreeformWrapperScan = - | { kind: "hold"; parse: boolean } + | { kind: "hold" } | { kind: "raw" } | { kind: "input"; valueStart: number }; @@ -192,38 +193,36 @@ function scanValue(text: string, from: number): number { /** * Which wrapper the completed text will unwrap to, as far as this prefix can say. * - * Fallback keys are deliberately not recognized here. They only unwrap when exactly one of them - * carries a string, and a second one can still arrive, so no prefix decides them — which makes - * them indistinguishable from any other undecided object and lets one HOLD cover both. + * `fallbackKeys` is the tool's alternate-field vocabulary — `freeformFallbackKeys`, the same + * list `unwrapFreeformToolInput` filters against. No open prefix can decide a fallback key, + * because it unwraps only as the SINGLE string field and a second one can still arrive; so the + * scan keeps a table of the members it has already walked and consults it once, at the close. */ -export function scanFreeformWrapper(text: string): FreeformWrapperScan { +export function scanFreeformWrapper(text: string, fallbackKeys: readonly string[]): FreeformWrapperScan { // One clamp rather than a budget threaded through every helper. Every helper already holds // when it runs off the end of what it can see, so a buffer whose classification needs more // than this holds for exactly the right reason, and no scan can cost more than this many // characters however large the arguments grow. Indices into the clamp are indices into the // full text, because the clamp is a prefix of it. - const bounded = text.length > MAX_FREEFORM_WRAPPER_SCAN_CHARS - ? text.slice(0, MAX_FREEFORM_WRAPPER_SCAN_CHARS) - : text; - // `parse` is true only where this scan actually SAW the object close. Every other hold ran - // out of buffer or out of budget, and in both cases asking `JSON.parse` is work with no - // possible payoff: the first is provably incomplete, and the second would re-read a growing - // buffer on every delta that happens to end in a brace — repeated braces inside a long - // unterminated string are enough to make that quadratic. Holding a budget-exhausted prefix - // costs nothing that matters, because the value it would release arrives in the same instant - // as the authoritative completion that follows it. - const hold = (): FreeformWrapperScan => ({ kind: "hold", parse: false }); + const wholeText = text.length <= MAX_FREEFORM_WRAPPER_SCAN_CHARS; + const bounded = wholeText ? text : text.slice(0, MAX_FREEFORM_WRAPPER_SCAN_CHARS); + const hold = (): FreeformWrapperScan => ({ kind: "hold" }); const open = skipWhitespace(bounded, 0); if (open === HOLD) return hold(); // Not an object, so no wrapper rule reaches it: arrays, scalars and ordinary bodies all // complete as themselves. if (bounded[open] !== "{") return { kind: "raw" }; + // Every top-level member is recorded as the scan passes it, last occurrence winning — the + // keep-last rule `JSON.parse` applies to duplicate keys. When the object closes, this table + // answers the only question completion asks of it — which members carried strings — so the + // closed object classifies directly instead of reparsing a buffer the scan just walked. + const members = new Map(); let i = open + 1; for (;;) { const at = skipWhitespace(bounded, i); if (at === HOLD) return hold(); - if (bounded[at] === "}") return afterTopLevelClose(bounded, at + 1); + if (bounded[at] === "}") return afterTopLevelClose(bounded, at + 1, members, fallbackKeys, wholeText); if (bounded[at] !== '"') return { kind: "raw" }; const nameEnd = scanString(bounded, at); @@ -254,6 +253,7 @@ export function scanFreeformWrapper(text: string): FreeformWrapperScan { : { kind: "raw" }; } + members.set(name, { stringValue: bounded[valueAt] === '"', valueStart: valueAt + 1 }); const valueEnd = scanValue(bounded, valueAt); if (valueEnd === HOLD) return hold(); if (valueEnd === NEVER) return { kind: "raw" }; @@ -264,16 +264,35 @@ export function scanFreeformWrapper(text: string): FreeformWrapperScan { i = next + 1; continue; } - if (bounded[next] === "}") return afterTopLevelClose(bounded, next + 1); + if (bounded[next] === "}") return afterTopLevelClose(bounded, next + 1, members, fallbackKeys, wholeText); return { kind: "raw" }; } } /** * The object closed without a canonical key. Only whitespace may follow one that parses, so - * anything else makes the text raw; otherwise the completed parse decides between a fallback - * wrapper and no wrapper at all. + * anything else makes the text raw. Whitespace to the end IS legal, but the member table + * already says which way completion goes on it: exactly one string-valued fallback field is + * the wrapper's value, and any other member shape streams the raw text — byte-exact through + * whatever trailing whitespace arrived, where a reparsed growing buffer was quadratic work on + * every whitespace delta and a held one swallowed the whitespace bytes entirely (#566). */ -function afterTopLevelClose(text: string, from: number): FreeformWrapperScan { - return skipWhitespace(text, from) === HOLD ? { kind: "hold", parse: true } : { kind: "raw" }; +function afterTopLevelClose( + text: string, + from: number, + members: ReadonlyMap, + fallbackKeys: readonly string[], + wholeText: boolean, +): FreeformWrapperScan { + const trailing = skipWhitespace(text, from); + if (trailing !== HOLD) return { kind: "raw" }; + // A clamped prefix ends inside this whitespace, so what follows is unseen: hold rather than + // guess at a tail that could still invalidate the close. When the whole text fit, the object + // plus its trailing whitespace is complete — and its members decide it without a parse. + if (!wholeText) return { kind: "hold" }; + const candidates = fallbackKeys.filter(key => members.get(key)?.stringValue === true); + if (candidates.length === 1) { + return { kind: "input", valueStart: members.get(candidates[0]!)!.valueStart }; + } + return { kind: "raw" }; } diff --git a/src/responses/progressive-freeform-input.ts b/src/responses/progressive-freeform-input.ts index 8ac1af79013..e400c780941 100644 --- a/src/responses/progressive-freeform-input.ts +++ b/src/responses/progressive-freeform-input.ts @@ -1,4 +1,4 @@ -import { unwrapFreeformToolInput } from "./apply-patch-envelope"; +import { freeformFallbackKeys } from "./apply-patch-envelope"; import { JSON_ESCAPES, scanFreeformWrapper } from "./freeform-wrapper-scan"; /** @@ -92,11 +92,12 @@ function decodeJsonStringPrefix(body: string): string | null { * damage is what is available without giving up progressive streaming, and the args are * unusable in that case whichever representation wins. * - * A fallback key is not decidable. It only unwraps when it is the SINGLE string field, and a - * second key can still arrive — so a value emitted early would have to be taken back. That is - * the rewind this holds instead: stream nothing until the object closes, then publish the one - * repaired body. The routed passthrough in `responses-custom-tool-repair.ts` already holds - * any object prefix for the same reason (#5047). + * A fallback key decides only at the object's close. It unwraps as the SINGLE string field, + * and a second key can still arrive while the object is open — so a value emitted early would + * have to be taken back. The scan therefore keeps a member table and consults it at the + * close, publishing the wrapped value (or the raw text) without ever reparsing the buffer. + * The routed passthrough in `responses-custom-tool-repair.ts` already holds any object prefix + * for the same reason (#5047). * * An object that has not reached a canonical key YET is in exactly that position, and used to * be treated as raw because it did not match the literal `{"input":"`. It holds now: `input` @@ -106,7 +107,7 @@ function decodeJsonStringPrefix(body: string): string | null { * not holding was publishing bytes the completed item removes. */ export function progressiveFreeformInput(args: string, toolName: string): string | null { - const scan = scanFreeformWrapper(args); + const scan = scanFreeformWrapper(args, freeformFallbackKeys(toolName)); if (scan.kind === "input") { const decoded = decodeJsonStringPrefix(args.slice(scan.valueStart)); if (decoded === null) return null; @@ -114,17 +115,10 @@ export function progressiveFreeformInput(args: string, toolName: string): string } if (scan.kind === "raw") return mayBecomeFencedBody(args, toolName) ? null : args; - // Undecided: some wrapper may still apply, and only the completed object says which one. - // The parse runs exactly where the scan SAW the object close, so it reads a buffer the scan - // already walked and its cost is bounded by the same clamp. A hold from either limit stays - // held: an incomplete object has nothing to parse, and re-reading a budget-exhausted buffer - // on every delta is quadratic work for a delta that would arrive in the same instant as the - // authoritative completion behind it. - if (!scan.parse) return null; - try { - JSON.parse(args); - } catch { - return null; - } - return unwrapFreeformToolInput(args, toolName); + // Undecided: an incomplete object, or a close whose tail is clamped out of view. Both stay + // held — the first is provably incomplete, and re-walking a budget-exhausted buffer on every + // delta is work for a preview that arrives in the same instant as the authoritative + // completion behind it. A closed object the scan could see whole never reaches here: its + // member table already resolved it to `input` or `raw`. + return null; } diff --git a/structure/transports/responses.md b/structure/transports/responses.md index b1e28958fa1..ffbf485ad44 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -140,15 +140,17 @@ follows is: an own `input` with a string value streams progressively, because co it precedence over everything else in the object whatever its position; an `input` with a non-string value, a text that is not an object, and an object `JSON.parse` can no longer accept all publish their own bytes, because that is what completion returns for them; every other -object HOLDS until it parses, because a key that has not arrived yet can still change the -answer. Fallback fields fall out of that last rule rather than being recognized separately: -they only unwrap as the single string field, so no prefix decides them. Classification is +object HOLDS until it closes, because a key that has not arrived yet can still change the +answer. Fallback fields decide at that close rather than from a parse: they only unwrap as the +single string field, so the scan keeps a last-wins table of the members it already walked and +consults it once — one string fallback field releases its value as `input`, anything else +streams the raw text byte-exact through whatever trailing whitespace follows. Classification is bounded to `MAX_FREEFORM_WRAPPER_SCAN_CHARS`, which keeps the work per delta from growing with the arguments. Past the bound nothing is previewed at all: the authoritative parse still -unwraps the wrapper at completion, so the bound costs preview and never agreement. The parse -that releases a held object therefore runs only where the scan SAW the object close, which is -what keeps a buffer whose deltas happen to end on a brace from being re-read on every one of -them. +unwraps the wrapper at completion, so the bound costs preview and never agreement. Because the +close resolves from the member table instead of `JSON.parse`, fragmented trailing whitespace +and deltas that happen to end on a brace can never reparse a growing provider-controlled +buffer. What that policy costs is worth stating plainly, because it is a real narrowing. A body that IS a parseable JSON object but not a wrapper — `{"code":1}` or `{"code":"a","script":"b"}` — now diff --git a/tests/responses/responses-freeform-wrapper-keys.test.ts b/tests/responses/responses-freeform-wrapper-keys.test.ts index 8416692c0d8..a22dfafcf83 100644 --- a/tests/responses/responses-freeform-wrapper-keys.test.ts +++ b/tests/responses/responses-freeform-wrapper-keys.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test } from "bun:test"; +import { describe, expect, spyOn, test } from "bun:test"; import { unwrapFreeformToolInput } from "../../src/responses/apply-patch-envelope"; import { MAX_FREEFORM_WRAPPER_SCAN_CHARS } from "../../src/responses/freeform-wrapper-scan"; import { progressiveFreeformInput } from "../../src/responses/progressive-freeform-input"; @@ -111,6 +111,45 @@ describe("freeform wrapper keys the literal matcher could not see", () => { expect(emissions(program, "exec").length).toBeGreaterThan(1); }); + test("does not reparse a closed object on trailing whitespace deltas", () => { + const parse = spyOn(JSON, "parse"); + try { + const body = "{}" + " ".repeat(MAX_FREEFORM_WRAPPER_SCAN_CHARS + 1); + // Inside the bound every whitespace prefix publishes byte-exact; past it the scan holds + // and completion stays authoritative, exactly like every other budget-exhausted buffer. + expect(published(body, "exec")).toBe(body.slice(0, MAX_FREEFORM_WRAPPER_SCAN_CHARS)); + // Nothing reparses the accumulated buffer at all anymore: a closed object resolves from + // the scan's own member table, and `{}` carries no member names to decode. Provider- + // controlled whitespace deltas therefore cannot turn into repeated parse work. + expect(parse).not.toHaveBeenCalled(); + } finally { + parse.mockRestore(); + } + }); + + test("a closed object's trailing whitespace streams byte-exact", () => { + // The review gap in the first fix: once the object closed, later whitespace prefixes held + // forever, so the concatenated deltas lost the whitespace the completed item keeps. Every + // raw-object spelling must publish all of its bytes, fragmented or in one chunk. + for (const body of ["{} ", '{"a":1} ', '{"code":"a","script":"b"}\t\n', '{ "x" : [ 1 , 2 ] } ']) { + expect({ body, completed: unwrapFreeformToolInput(body, "exec") }) + .toEqual({ body, completed: body }); + expect({ body, streamed: published(body, "exec") }).toEqual({ body, streamed: body }); + expect({ body, whole: progressiveFreeformInput(body, "exec") }) + .toEqual({ body, whole: body }); + expect({ body, retractions: stream(body, "exec").retractions }) + .toEqual({ body, retractions: [] }); + } + + // A closed fallback wrapper publishes its value once; the trailing whitespace is wrapper + // syntax rather than input bytes, so it is correctly absent whichever way it arrives. + for (const body of ['{"code":"cmd"}', '{"code":"cmd"} ', '{"code":"cmd"}\n']) { + expect({ body, completed: unwrapFreeformToolInput(body, "exec") }) + .toEqual({ body, completed: "cmd" }); + expect({ body, streamed: published(body, "exec") }).toEqual({ body, streamed: "cmd" }); + } + }); + test("canonical input and plain bodies stay progressive", () => { // Holding an undecided object must not cost the progressive streaming these paths provide. const canonical = '{"input":"line one\\nline two"}';