fix(json-parse): exclude code-context tails from Windows-path heuristic (#93139)#95982
fix(json-parse): exclude code-context tails from Windows-path heuristic (#93139)#95982louria wants to merge 1 commit into
Conversation
The repairJson helper detects unescaped backslashes in Windows path strings (\bin, \users) and preserves them as literal characters instead of rewriting them as JSON escape sequences. The detector keys off a tail ending in `<non-alphanum><letter>:`, which matches genuine drive letters like `C:` but also matches Python block openers like `if x:`, `for x:`, `try:`, `def foo(x):`. When the next character was a real newline, the model output got rewritten so the shell saw a literal `\n` instead of an actual newline. This broke any agent tool call that carried multi-line Python in a heredoc, with the symptom that the shell parser threw `SyntaxError: unexpected character after line continuation character` and the cron agent burned thousands of output tokens trying to work around the corruption. Fix: reject any tail that contains code-context markers like parens, braces, equals, semicolons, single-quotes, commas, shell redirects, or pipes. Real Windows path tails do not contain those characters, but nearly every Python/JS/shell block opener does. Adds regression tests for the failing patterns and a mixed-content test that confirms a Windows path adjacent to code still survives. Refs openclaw#93139
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 5:46 PM ET / 21:46 UTC. Summary PR surface: Source +9, Tests +51. Total +60 across 2 files. Reproducibility: yes. Source-level reproduction is high confidence: the current predicate matches code-like Review metrics: 1 noteworthy metric.
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:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the fix in the shared JSON parser, but revise the predicate so it scopes code-context rejection to the actual candidate suffix, preserves existing Windows-path repair, covers bare code openers, and is backed by real OpenClaw tool-run proof. Do we have a high-confidence way to reproduce the issue? Yes. Source-level reproduction is high confidence: the current predicate matches code-like Is this the best way to solve the issue? No. The parser helper is the right layer, but the proposed whole-tail punctuation guard is too broad for existing Windows-path repair and too narrow for bare single-letter code-opener tails. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3811001d2783. Label changesLabel justifications:
Evidence reviewedPR surface: Source +9, Tests +51. Total +60 across 2 files. View PR surface stats
Acceptance criteria:
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
rating: 🧂 unranked krab
clawsweeper
Bot
added
P1
|
Ran an independent verification of the function looksLikeWindowsPathPrefixOLD(prefix) {
const tail = prefix.slice(-160);
return /(?:^|[^A-Za-z0-9])[A-Za-z]:(?:[\\/][^"\\/:*?<>|\r\n]*)*$/.test(tail);
}
function looksLikeWindowsPathPrefixPR(prefix) {
const tail = prefix.slice(-160);
if (/[()=;{}',<>|]/.test(tail)) return false;
return /(?:^|[^A-Za-z0-9])[A-Za-z]:(?:[\\/][^"\\/:*?<>|\r\n]*)*$/.test(tail);
}
const cases = [
["real capture: with p.open('w') as f:", "with p.open('w') as f:"],
["bare if x:", "if x:"],
["bare if r:", "if r:"],
["punctuation-before-path: see, C:\\new", "see, C:\\new"],
["punctuation-before-path: Bob's C:\\new", "Bob's C:\\new"],
["genuine simple path: C:\\Users\\x", "C:\\Users\\x"],
["for item in d:", "for item in d:"],
];
for (const [label, prefix] of cases) {
console.log(label, "| OLD:", looksLikeWindowsPathPrefixOLD(prefix), "| PR:", looksLikeWindowsPathPrefixPR(prefix));
}Results:
This confirms the fix as-written closes the specific reproducer we hit (a
For what it's worth, our own real-world capture (an agent-generated Happy to run further cases against a revised patch if useful. |
|
Follow-up with live-deployment results, since our earlier comment was module-level only. We hand-applied this PR's guard to the minified Results so far:
One methodology caveat worth stating honestly: the corruption only occurs when streamed tool-call JSON actually hits the repair path, which is nondeterministic in live traffic. Our original corrupted capture happened organically, but we could not force the repair path on demand — an identical probe against the stock bundle also came through clean. So live clean probes alone aren't conclusive; combined with the module-level before/after above (stock corrupts the capture, patched preserves it, same shipped bundle), we consider the fix verified for our reproducer class. The two gaps flagged earlier (bare |
|
This pull request has been automatically marked as stale due to inactivity. |
2 participants
What Problem This Solves
OpenClaw agent cron jobs that emit multi-line Python (or any code block whose opener ends in
:) through theexectool started failing reliably after 2026.6.6/2026.6.9 with errors like:…and the cron error log surfaced fragments of the agent's own Python being misinterpreted as tool/shell names:
Tracked in issue #93139 (also
#93139comment thread with v2026.6.6 and v2026.6.9 reproducers).On one production host, this regression took the daily/Notion-sync/language-learning cron failure rate from 0/211 (0.0%) in the 12 days before the upgrade to 8/27 (29.6%) in the 1.5 days after.
Root Cause
repairJsoninsrc/llm/utils/json-parse.tsincludes a Windows-path heuristic (looksLikeWindowsPathPrefix) so that when a model emits an unescaped path likeC:\bin\app.exeinside a JSON string, the backslash escape sequences (\b,\t,\n, …) are not greedily re-interpreted as JSON control escapes.The heuristic uses this regex on the last 160 chars of the in-progress string value:
/(?:^|[^A-Za-z0-9])[A-Za-z]:(?:[\\/][^"\\/:*?<>|\r\n]*)*$/The trailing
*makes the path segment optional — so any tail ending in<non-alphanum><letter>:matches. That happens to also describe Python block openers:C:\Usersif x:if)for x:for)r:x:When the false positive fires, the next
\nafter the:is treated as the start of a path segment and gets emitted as a literal backslash-n (two characters) into the parsed JSON value. The agent then sees its own multi-line Python turn into a garbled one-liner and the shell rejects it.Byte-level repro
od -cof the corrupted heredoc payload (with this PR not applied):Every other newline in the same string is the genuine
\n(one byte, 0x0A). Only the one immediately after:gets doubled.Why This Change Was Made
The fix is a single guard at the top of
looksLikeWindowsPathPrefix: if the tail contains any code-context marker —(,),=,;,{,},',,,<,>,|— bail out and don't treat the next backslash-escape as a path char.Those characters do not appear in real Windows path tails, but appear in essentially every Python / JS / shell block opener that ends with
:(def foo(x):,if (cond):,try:,for f in *.py:,python3 << EOF, etc.).I chose this over tightening the regex to require an actual filename segment (
[A-Za-z]:[\\/][A-Za-z0-9._\-]+) because that approach broke the existing test fixtures that depend onC:\bin(single character after the separator that's a valid\bJSON escape) staying matched. The code-context guard preserves all existing fixtures unchanged.User Impact
execheredocs /writetool fail unpredictably, ~30% in observed production; each failure burns thousands of output tokens as the agent tries to self-debug the corrupted output.if:/for:/try:/def f():blocks round-trip correctly through tool-call JSON repair. Windows paths still take the existing repair path.Evidence
Tests
All 14 existing test cases in
src/llm/utils/json-parse.test.tscontinue to pass (zero changes needed), and 7 new regression tests cover the bug:New regression tests cover:
if x:block in a heredoc (the headline bug, with bothparseJsonWithRepairandparseStreamingJsonassertions)try:/except Exception as e:chainsdef foo(x):(parens in signature)while True:(no parens / no identifier after the keyword)if r:reproducer captured in the issue's trajectory dumpsfor f in *.py:\n echo $f"see C:\\Users for path; if x:\n do_stuff()"— the path part stays asC:\Users, the code part gets a real newline.Lint
Format
(
oxfmtwas run on the test file to align with project style after I added the newdescribeblock.)Backward Compatibility Smoke
Every
parseJsonWithRepair/parseStreamingJsonfixture in the existing test file — including all fiveC:\bin\app.exe,C:\temp\x,C:\new\file,D:\reports\q,C:\users\bobWindows paths — passes after the fix. The Windows-path repair pathway is preserved verbatim; the only behavioral change is that obvious code tails no longer trip it.Notes for Reviewers
src/llm/utils/json-parse.ts(9-line guard),src/llm/utils/json-parse.test.ts(51-linedescribeblock of regression tests).CHANGELOG.mddeliberately untouched perCONTRIBUTING.md.