Skip to content

fix(ai): preserve valid JSON escapes while keeping Windows path recovery - #114925

Closed
sijie-Z wants to merge 2 commits into
openclaw:mainfrom
sijie-Z:fix/json-repair-windows-path-false-positive
Closed

sijie-Z wants to merge 2 commits into
openclaw:mainfrom
sijie-Z:fix/json-repair-windows-path-false-positive

Conversation

@sijie-Z

@sijie-Z sijie-Z commented Jul 28, 2026

Copy link
Copy Markdown

What Problem This Solves

Fixes #114292 ? write tool content containing \n after a Windows-looking prefix was written literally instead of as newlines, producing Python SyntaxError.

Change

The shared parser now always preserves valid JSON escapes, including \n in write content, web-search query, and multiline exec command.

Malformed Windows-path recovery is applied at filesystem boundaries only:

  • resolveToCwd in path-utils.ts recovers malformed Windows path separators, so every built-in filesystem path tool (read, edit, write, find, grep, ls) keeps the existing recovery contract.
  • Browser upload recovers each paths entry before resolving uploads.
  • The recovery helper is exported from @openclaw/ai/internal/runtime for 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 createWriteTool and reads back a file whose malformed path segment was recovered.

Local runs:

Test Files  1 passed (1)   src/llm/utils/json-parse.test.ts
Tests      19 passed (19)

Test Files  1 passed (1)   src/agents/sessions/tools/write.test.ts
Tests      1 passed (1)    recovers malformed Windows path segments before resolving

Files changed

  • packages/ai/src/utils/json-parse.ts
  • src/llm/utils/json-parse.test.ts
  • src/agents/sessions/tools/path-utils.ts
  • src/agents/sessions/tools/write.test.ts
  • extensions/browser/src/browser-tool.ts

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 28, 2026
@clawsweeper

clawsweeper Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 8:12 AM ET / 12:12 UTC.

ClawSweeper review

What this changes

The 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
Reviewed head: 423f28fa5e72a354f3f0569bb2dc76fc76cb5c8f

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The central parser direction is sound, but an introduced media-path regression, stale conflicts, and mock-only proof keep the draft from merge readiness.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The changed production owners are the tolerant parser and path consumers, but the supplied evidence is only parser/write test output. Add a redacted after-fix Windows tool run showing literal \\n content becomes a newline and a malformed Windows path recovers; update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed production owners are the tolerant parser and path consumers, but the supplied evidence is only parser/write test output. Add a redacted after-fix Windows tool run showing literal \\n content becomes a newline and a malformed Windows path recovers; update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed 6 items Introduced parser regression: The PR removes the shared control-escape recovery, so a malformed Windows media path decoded as a control character is no longer repaired before a media tool sees it.
Uncovered media boundary: Image-tool media references are classified before common media access; a decoded C: plus newline value is treated as an unsupported scheme rather than a Windows drive path. Image generation, music, video, and PDF tools use the same pre-classification pattern.
Current main still has the reported defect: Current main retains the JSON control-escape heuristic that can double a valid newline after a Windows-looking prefix; the linked issue therefore remains unresolved.
Findings 1 actionable finding [P1] Recover malformed Windows paths before media-reference classification
Security None None.

How this fits together

Model 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
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed production owners are the tolerant parser and path consumers, but the supplied evidence is only parser/write test output. Add a redacted after-fix Windows tool run showing literal \n content becomes a newline and a malformed Windows path recovers; update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
  • Recover malformed Windows paths before media-reference classification (P1) - Moving recovery out of the parser leaves media inputs such as a malformed C:\new.png decoded as C: plus a newline; the media classifier rejects that value before any recovery. Apply the helper at the shared local-media pre-classification boundary and add a direct media-tool regression covering the affected path.
  • Resolve merge risk (P1) - The branch cannot merge cleanly with current main and must be rebased before its adapted behavior can be reviewed.
  • Resolve merge risk (P1) - The external contributor has supplied only test output, not real after-fix Windows behavior proof.
  • Complete next step (P2) - A concrete P1 repair exists, but this external draft has merge conflicts and requires contributor-owned real Windows proof before it can be considered.

Findings

  • [P1] Recover malformed Windows paths before media-reference classification — packages/ai/src/utils/json-parse.ts:80-83
Agent review details

Security

None.

PR surface

Source +52, Tests +60. Total +112 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 71 19 +52
Tests 2 63 3 +60
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 134 22 +112

Review metrics

Metric Value Why it matters
Production versus test delta production +71/-19, tests +63/-3 The production growth is justified by moving recovery to owning tool boundaries, but the new boundary currently omits media inputs.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #114292
Summary: This PR is a candidate fix for the open Windows newline-parsing regression, but needs a complete boundary repair before it can resolve that issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Repair the media boundary before merge (recommended)
    Recover malformed Windows drive paths before shared media-reference classification, add a direct media-tool regression, then rebase and refresh exact-head review.
  2. Pause the draft
    Leave this draft unmerged if its contributor cannot rebase it and show the requested Windows behavior proof.

Technical review

Best 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:

  • [P1] Recover malformed Windows paths before media-reference classification — packages/ai/src/utils/json-parse.ts:80-83
    Moving recovery out of the parser leaves media inputs such as a malformed C:\new.png decoded as C: plus a newline; the media classifier rejects that value before any recovery. Apply the helper at the shared local-media pre-classification boundary and add a direct media-tool regression covering the affected path.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9903991f62b5.

Labels

Label justifications:

  • P1: The PR targets a shipped Windows tool-argument regression and currently introduces a comparable failure for media-path arguments.
  • merge-risk: 🚨 compatibility: Merging would change existing malformed Windows-path handling for built-in media tools.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owners are the tolerant parser and path consumers, but the supplied evidence is only parser/write test output. Add a redacted after-fix Windows tool run showing literal \n content becomes a newline and a malformed Windows path recovers; update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.

Evidence

What I checked:

  • Introduced parser regression: The PR removes the shared control-escape recovery, so a malformed Windows media path decoded as a control character is no longer repaired before a media tool sees it. (packages/ai/src/utils/json-parse.ts:80, 423f28fa5e72)
  • Uncovered media boundary: Image-tool media references are classified before common media access; a decoded C: plus newline value is treated as an unsupported scheme rather than a Windows drive path. Image generation, music, video, and PDF tools use the same pre-classification pattern. (src/agents/tools/image-tool.ts:1027, 423f28fa5e72)
  • Current main still has the reported defect: Current main retains the JSON control-escape heuristic that can double a valid newline after a Windows-looking prefix; the linked issue therefore remains unresolved. (packages/ai/src/utils/json-parse.ts:83, 9903991f62b5)
  • Shipped affected lineage: The original streaming-argument normalization commit is contained in v2026.8.1, while this PR’s fix commit is not contained in that release. (packages/ai/src/utils/json-parse.ts:1, b579c0a65be1)
  • Rebase is required: An exact three-way merge against fetched main conflicts in the parser, browser tool, path utility, and write test, and has a delete/modify conflict for the old parser test. (423f28fa5e72)
  • Only mock-level proof is supplied: The PR body reports focused parser and write tests, but no redacted after-fix Windows tool run; the added write test also returns before assertions outside Windows. (src/agents/sessions/tools/write.test.ts:181, 423f28fa5e72)

Likely related people:

  • Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Recover malformed Windows paths before shared media-reference classification and add a direct media-tool regression.
  • Rebase onto current main and request an exact-head review.
  • Attach redacted Windows after-fix output for multiline content and malformed path recovery.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (25 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-05T04:22:31.026Z sha a97367486c69393f7b58830020f774ecdbd9503b :: needs real behavior proof before merge. :: [P1] Restore recovery for every path-shaped tool argument
  • reviewed 2026-08-05T04:29:38.585Z sha a97367486c69393f7b58830020f774ecdbd9503b :: needs real behavior proof before merge. :: [P1] Restore recovery for every filesystem path tool
  • reviewed 2026-08-05T06:19:08.449Z sha ed6906b :: needs real behavior proof before merge. :: [P1] Recover malformed paths in the image tool | [P1] Normalize Browser upload paths at the shared endpoint | [P1] Handle control escapes immediately after a drive prefix
  • reviewed 2026-08-06T09:36:34.351Z sha 423f28f :: needs real behavior proof before merge. :: [P1] Recover malformed paths in every media file-input tool | [P1] Normalize Browser upload paths at the shared resolver
  • reviewed 2026-08-15T14:18:18.971Z sha 423f28f :: needs real behavior proof before merge. :: [P1] Recover malformed paths at the read boundary | [P1] Normalize Browser uploads in the shared resolver | [P1] Recover local media references after parsing
  • reviewed 2026-08-17T01:24:32.787Z sha 423f28f :: needs real behavior proof before merge. :: [P1] Recover malformed paths through the read resolver | [P1] Normalize Browser uploads in their shared resolver | [P1] Recover media file inputs at their shared boundary
  • reviewed 2026-08-19T01:10:12.840Z sha 423f28f :: needs real behavior proof before merge. :: [P1] Normalize Browser uploads in the shared resolver
  • reviewed 2026-08-25T01:56:34.519Z sha 423f28f :: needs real behavior proof before merge. :: [P1] Normalize upload paths in the shared resolver

@sijie-Z

sijie-Z commented Jul 28, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

Updated the approach: replaced the loose heuristic with a two-stage precision check instead of removing it entirely. The new looksLikeWindowsPathPrefix requires the entire prefix to be a pure drive-letter path (^[A-Za-z]:[\\/]), and looksLikePathContinuation checks the character after the escape. This preserves malformed Windows path recovery while fixing the \n double-escaping regression.

@clawsweeper

clawsweeper Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 28, 2026
@sijie-Z

sijie-Z commented Jul 28, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

Removed looksLikePathContinuation entirely. The pure-drive-prefix guard alone is sufficient — mixed code content like import sys\nprint(1) never matches ^[A-Za-z]:[\/]. This restores recovery for all malformed Windows path forms including C:\n$RECYCLE.BIN, terminal C:\n, punctuation, and non-ASCII path segments.

@clawsweeper

clawsweeper Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 28, 2026
@sijie-Z

sijie-Z commented Jul 28, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

Added looksLikeContentBoundary: when the character after \n is whitespace, a quote, or a delimiter, the \n is treated as an intentional newline. This preserves C:\path\nnext line while still recovering C:\newfolder, C:\nFOO, C:\n$RECYCLE.BIN.

@clawsweeper

clawsweeper Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 28, 2026
@vincentkoc vincentkoc self-assigned this Jul 29, 2026
@m908xer-png

m908xer-png commented Aug 3, 2026

Copy link
Copy Markdown

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

@sijie-Z
sijie-Z marked this pull request as draft August 5, 2026 01:27
@sijie-Z
sijie-Z force-pushed the fix/json-repair-windows-path-false-positive branch from a1f67be to 92b0d5e Compare August 5, 2026 01:53
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling and removed size: S labels Aug 5, 2026
@sijie-Z

sijie-Z commented Aug 5, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Aug 5, 2026
@sijie-Z

sijie-Z commented Aug 5, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@sijie-Z
sijie-Z force-pushed the fix/json-repair-windows-path-false-positive branch from 0551c6b to 095e2db Compare August 5, 2026 02:44
@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@sijie-Z
sijie-Z marked this pull request as ready for review August 5, 2026 03:04
@sijie-Z
sijie-Z force-pushed the fix/json-repair-windows-path-false-positive branch from c2a2018 to 134c672 Compare August 5, 2026 03:33
@openclaw-barnacle openclaw-barnacle Bot added the agents Agent runtime and tooling label Aug 5, 2026
@sijie-Z

sijie-Z commented Aug 5, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@sijie-Z

sijie-Z commented Aug 5, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@sijie-Z
sijie-Z force-pushed the fix/json-repair-windows-path-false-positive branch from 134c672 to a973674 Compare August 5, 2026 03:36
@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@sijie-Z
sijie-Z force-pushed the fix/json-repair-windows-path-false-positive branch from a973674 to ed6906b Compare August 5, 2026 06:12
@sijie-Z

sijie-Z commented Aug 5, 2026

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@sijie-Z

sijie-Z commented Aug 5, 2026

Copy link
Copy Markdown
Author

@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.

@sijie-Z
sijie-Z marked this pull request as draft August 17, 2026 01:16
@openclaw-barnacle

Copy link
Copy Markdown

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

@openclaw-barnacle

Copy link
Copy Markdown

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: escape sequences in content parameter no longer parsed as newlines after v2026.7.1 upgrade`

3 participants