Conversation
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 8:12 AM ET / 12:12 UTC. ClawSweeper reviewWhat this changesThe PR preserves valid JSON escapes globally and moves malformed Windows-path recovery to session filesystem tools and the model-facing browser upload tool. Merge readiness⛔ Blocked before merge - 5 items remain Keep open: the parser fix remains needed, but moving recovery out of the shared parser breaks malformed Windows paths supplied to built-in media tools before their media-reference classifier. The draft also needs a rebase and real Windows behavior proof. Priority: P1 Review scores
Verification
How this fits togetherModel tool arguments pass through tolerant JSON parsing before file, media, and browser tools consume them. This PR changes whether decoded control characters in Windows-style paths are repaired before each tool resolves or classifies the input. flowchart LR
A[Model tool JSON] --> B[Tolerant JSON parser]
B --> C[Tool arguments]
C --> D[Filesystem path resolver]
C --> E[Media reference classifier]
C --> F[Browser upload]
D --> G[File operations]
E --> H[Media generation and upload]
F --> H
Before merge
Findings
Agent review detailsSecurityNone. PR surfaceSource +52, Tests +60. Total +112 across 5 files. View PR surface stats
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep valid JSON escapes in the shared parser, then recover malformed Windows drive paths once at the shared local-media reference boundary before classification; rebase the branch and prove both multiline content and media-path recovery on Windows. Do we have a high-confidence way to reproduce the issue? Yes—source establishes that a malformed Windows media path becomes a control-character string after parsing and is rejected by media classification before recovery. This review did not execute the untrusted PR branch. Is this the best way to solve the issue? No—the parser boundary is the right place to preserve valid JSON escapes, but every local file-consuming tool must recover malformed Windows paths before its own first path interpretation; media classification is still uncovered. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9903991f62b5. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (25 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review Updated the approach: replaced the loose heuristic with a two-stage precision check instead of removing it entirely. The new |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review Removed |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review Added |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Commenting for visibility -- this is a very annoying bug that prevents agents from being able to correctly write python files or run inline python scripts. It's fairly trivial to patch locally, but it deserves a proper fix. It also shows a critical need for a unit test that tests basic functions such as read() and write() with tricky sequences that include escape characters. This is not the kind of bug that should be making it into releases |
a1f67be to
92b0d5e
Compare
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review |
0551c6b to
095e2db
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
c2a2018 to
134c672
Compare
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review |
134c672 to
a973674
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
a973674 to
ed6906b
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@vincentkoc ClawSweeper latest review says a maintainer decision is needed on the core-to-plugin recovery seam before this can land. Could you advise on the preferred boundary or review the current approach? Happy to adjust. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Closing due to inactivity. |
What Problem This Solves
Fixes #114292 ? write tool content containing
\nafter a Windows-looking prefix was written literally instead of as newlines, producing PythonSyntaxError.Change
The shared parser now always preserves valid JSON escapes, including
\nin writecontent, web-searchquery, and multiline execcommand.Malformed Windows-path recovery is applied at filesystem boundaries only:
resolveToCwdinpath-utils.tsrecovers malformed Windows path separators, so every built-in filesystem path tool (read, edit, write, find, grep, ls) keeps the existing recovery contract.pathsentry before resolving uploads.@openclaw/ai/internal/runtimefor plugin/tool boundaries.Verification
Parser regression tests cover content, query, exec command, and the recovery helper. The write tool has an integration test that writes through
createWriteTooland reads back a file whose malformed path segment was recovered.Local runs:
Files changed
packages/ai/src/utils/json-parse.tssrc/llm/utils/json-parse.test.tssrc/agents/sessions/tools/path-utils.tssrc/agents/sessions/tools/write.test.tsextensions/browser/src/browser-tool.ts@clawsweeper re-review