fix(llm): trust control escapes in JSON strings that escape backslashes#96748
fix(llm): trust control escapes in JSON strings that escape backslashes#96748ly-wang19 wants to merge 1 commit into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 28, 2026, 1:25 AM ET / 05:25 UTC. Summary PR surface: Source +16, Tests +14. Total +30 across 2 files. Reproducibility: yes. I did not execute the repro in this read-only review, but current main and Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the shared parser fix after a parser/runtime owner accepts the documented mixed-escaping semantics; keep this behavior centralized in the parser rather than adding provider-specific shims. Do we have a high-confidence way to reproduce the issue? Yes. I did not execute the repro in this read-only review, but current main and Is this the best way to solve the issue? Yes, with compatibility owner acceptance. The shared parser is the right layer; a AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 69af58ba2613. Label changesLabel justifications:
Evidence reviewedPR surface: Source +16, Tests +14. Total +30 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
proof: sufficient
repairJson has a Windows-path heuristic: inside a string, a backslash before a
control-escape letter (b/f/n/r/t) whose decoded prefix looks like a Windows path
is treated as an unescaped path separator and doubled, so a model that emits a
raw path like {"path":"C:\new\file"} is repaired to the literal path instead of
a newline/formfeed. That bet is deliberate and test-pinned for unescaped paths.
But the heuristic inspected only the DECODED prefix, which is identical whether
the wire used escaped "\\" (valid) or raw "\" (malformed). So for a string that
properly escapes its path — e.g. JSON.stringify({text:"C:\\Users\\me\nDone."}),
where the path uses "\\" and the \n is a genuine newline — it still fired and
turned the real newline into a literal backslash-n. OpenAI/Azure/ChatGPT
Responses transports feed parseStreamingJson straight into tool calls with no
JSON.parse-first guard, so a model emitting a Bash/Write/Edit argument with a
properly escaped Windows path plus multi-line content ran a corrupted command.
Fix: once a string contains a properly escaped backslash ("\\"), the model
demonstrably escapes backslashes, so its later \n/\t/etc. are genuine control
escapes — skip the path heuristic for the rest of that string. Strings with no
escaped backslash (the pinned unescaped-path cases) repair exactly as before;
this only adds positive escaping evidence as a suppression signal. Add a
regression test asserting an escaped path followed by a real newline/tab
round-trips to JSON.parse.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
dc5f774 to
1cbdeab
Compare
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
1 participant
What Problem This Solves
repairJson(src/llm/utils/json-parse.ts) — the tolerant parser behindparseStreamingJson/parseJsonWithRepairused for model tool-call arguments — corrupts genuine control escapes in strings that properly escape their backslashes.repairJsonhas a deliberate, test-pinned heuristic: inside a JSON string, a backslash before a control-escape letter (b/f/n/r/t) whose decoded prefix looks like a Windows path is assumed to be an unescaped path separator and doubled. This repairs models that emit raw paths like{"path":"C:\new\file"}into the literal path (json-parse.test.ts:17-26).The bug: the heuristic inspects only the decoded prefix, which is identical whether the wire used escaped
\\(valid) or raw\(malformed). So for a string that escapes its path correctly —JSON.stringify({ text: "Saved to C:\\Users\\me\nDone." }), where the separators are\\and the\nis a genuine newline — it still fired and rewrote the real newline into a literal\n:This reaches production: the OpenAI / Azure / ChatGPT Responses transports call
parseStreamingJsonon the final, complete tool-call argument string with noJSON.parse-first guard (openai-responses-shared.ts:780,798;openai-transport-stream.ts:1550/1681/1785/2871/3221;openai-completions.ts;mistral.ts;extensions/amazon-bedrock/stream.runtime.ts). A model emitting aBash/Write/Editargument with a properly escaped Windows path plus a later newline (a two-line command, multi-line file content) runs a corrupted command. (The native Anthropic path is unaffected — it triesparseJsonObjectPreservingUnsafeIntegersfirst.)Why This Change Was Made
A
JSON.parse-first guard was rejected because{"path":"C:\new\file"}is valid JSON (\n→LF,\f→FF) yet the existing test deliberately wants the literal path — parse-first would regress it. The correct, minimal signal is the model's own escaping behaviour: once a string contains a properly escaped\\, the model demonstrably escapes backslashes, so a following\n/\tin that same string is a genuine control escape, not a raw path separator. A per-stringsawEscapedBackslashflag (set on a real\\, reset at each string boundary) now also gates the path heuristic.This is a coherent rule rather than a special-case: a string that escapes a backslash is trusted to escape its control chars. Strings with no escaped backslash — the pinned unescaped-path cases — are completely unchanged (the flag stays false, the heuristic still fires).
User Impact
Tool-call arguments from OpenAI-family models that escape a Windows path correctly and contain genuine newlines/tabs are no longer silently corrupted. Unescaped-path repair is unchanged. No config/API change.
Evidence
Real behavior proof
Behavior addressed:
repairJson/parseStreamingJsonrewrote a genuine control escape into a literal escape sequence when it followed a properly escaped Windows path, corrupting OpenAI-family tool-call arguments.Real environment tested: Local checkout, Node v22.22.1, the real exported functions driven via
tsx(including a side-by-side OLDorigin/mainvs NEW differential), plus the Vitest suite.Exact steps or command run after this patch:
tsxrepro vsJSON.parse:parseJsonWithRepair(JSON.stringify({ text: "Saved to C:\\Users\\me\nDone." })).tsxOLD-vs-NEW differential over the reported case, all 5 pinned path tests, bare-drive inputs, and mixed-escaping inputs.tsxfuzz: 20,000 random valid JSON objects whose Windows paths are properly escaped (mixed with real control chars) — assertparseJsonWithRepair === JSON.parse.node scripts/run-vitest.mjs src/llm/utils/json-parse.test.tsjson-parse.tsfromorigin/main, kept the new test, re-ran it.Evidence after fix (OLD = origin/main, NEW = patched):
Observed result after fix: A genuine control escape after an escaped path round-trips to
JSON.parse; all 5 unescaped-path repairs and every other pinned case are byte-identical to before; 20k escaped-path fuzz clean; oxfmt--check, oxlint,tsgo:coreall green.Fail-before (proves the test catches the regression):
Scope / what was not changed (verified by the OLD-vs-NEW differential):
{"p":"C:\nfoo"}): OLD == NEW. This input is the same irreducibly-ambiguous case the heuristic already pins toward a path viajson-parse.test.ts:20(C:\new\file); it cannot be re-interpreted without breaking that shipped contract, so it is intentionally left as-is.{"path":"C:\\Users\new"}— first separator escaped, second raw): the only inputs where NEW differs from OLD besides the fix itself. Here NEW follows the demonstrated escaping (matchingJSON.parse) rather than OLD's literal-path guess. This requires self-inconsistent model output in a single string and is the deliberate consequence of the trust-the-escaping rule.