fix(tools): strip arg_value XML suffix from exec/read args#48792
fix(tools): strip arg_value XML suffix from exec/read args#48792zerone0x wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Some models (e.g. Qwen via DashScope) emit tool call arguments with trailing XML fragments like `</arg_value>>` appended to string values. This corrupts exec commands and file paths, blocking all file operations and command execution on affected platforms. Add `stripXmlArgValueSuffix` to strip these artifacts from tool call argument string values in both the parameter normalization layer (for read/write/edit tools) and the exec tool's command parameter. Fixes openclaw#48780 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71c6c921fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function stripXmlArgValueSuffixFromRecord(record: Record<string, unknown>): void { | ||
| for (const key of Object.keys(record)) { | ||
| const value = record[key]; | ||
| if (typeof value === "string" && value.includes("</arg_value>")) { | ||
| record[key] = stripXmlArgValueSuffix(value); |
There was a problem hiding this comment.
Restrict XML suffix stripping to path/command fields
The new record-wide sanitization mutates every string parameter, so write/edit payloads can be silently corrupted when content, oldText, or newText legitimately end with </arg_value> (or </arg_value>>). Because normalizeToolParams is used by the write/edit wrappers, this truncates user-requested file content and can also break edit matching for valid XML/text inputs; the stripping should be scoped to known affected fields (e.g. command/path aliases) instead of all string keys.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds a defensive sanitization layer that strips malformed
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-tools.params.ts
Line: 96
Comment:
**Regex quantifier may leave residual `>` for unusual provider output**
The pattern `>>{0,2}$` matches `</arg_value>` followed by 0, 1, or 2 additional `>` characters — handling 1, 2, or 3 total `>` after `arg_value`. If a particularly malformed provider response happens to emit four or more trailing `>` (e.g. `</arg_value>>>>>`), the regex will only strip `</arg_value>>>` and leave the remainder in the value.
Using `>*` (zero-or-more) instead of `>{0,2}` would handle any number of trailing `>` characters without changing behavior for the documented cases:
```suggestion
const XML_ARG_VALUE_SUFFIX_RE = /<\/arg_value>>*$/;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 230-232
Comment:
**Other string exec params not sanitized**
Only `params.command` is stripped, but the exec tool also accepts other string-typed parameters — `workdir`, `host`, `security`, `ask`, and `node` — that could be equally affected if the provider leaks the `</arg_value>` artifact into those values. A malformed `workdir` would silently break the working-directory resolution, which is hard to debug.
Consider sanitizing the full set of string exec params, or centralising via a shared helper similar to `stripXmlArgValueSuffixFromRecord`:
```typescript
// Strip all string exec params, not just command
for (const key of ["command", "workdir", "host", "security", "ask", "node"] as const) {
if (typeof params[key] === "string") {
params[key] = stripXmlArgValueSuffix(params[key]!);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 71c6c92 |
| * | ||
| * @see https://github.com/openclaw/openclaw/issues/48780 | ||
| */ | ||
| const XML_ARG_VALUE_SUFFIX_RE = /<\/arg_value>>{0,2}$/; |
There was a problem hiding this comment.
> for unusual provider output
The pattern >>{0,2}$ matches </arg_value> followed by 0, 1, or 2 additional > characters — handling 1, 2, or 3 total > after arg_value. If a particularly malformed provider response happens to emit four or more trailing > (e.g. </arg_value>>>>>), the regex will only strip </arg_value>>> and leave the remainder in the value.
Using >* (zero-or-more) instead of >{0,2} would handle any number of trailing > characters without changing behavior for the documented cases:
| const XML_ARG_VALUE_SUFFIX_RE = /<\/arg_value>>{0,2}$/; | |
| const XML_ARG_VALUE_SUFFIX_RE = /<\/arg_value>>*$/; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.params.ts
Line: 96
Comment:
**Regex quantifier may leave residual `>` for unusual provider output**
The pattern `>>{0,2}$` matches `</arg_value>` followed by 0, 1, or 2 additional `>` characters — handling 1, 2, or 3 total `>` after `arg_value`. If a particularly malformed provider response happens to emit four or more trailing `>` (e.g. `</arg_value>>>>>`), the regex will only strip `</arg_value>>>` and leave the remainder in the value.
Using `>*` (zero-or-more) instead of `>{0,2}` would handle any number of trailing `>` characters without changing behavior for the documented cases:
```suggestion
const XML_ARG_VALUE_SUFFIX_RE = /<\/arg_value>>*$/;
```
How can I resolve this? If you propose a fix, please make it concise.| // Strip malformed XML closing-tag suffixes leaked by some providers (e.g. Qwen/DashScope). | ||
| // See https://github.com/openclaw/openclaw/issues/48780 | ||
| params.command = stripXmlArgValueSuffix(params.command); |
There was a problem hiding this comment.
Only params.command is stripped, but the exec tool also accepts other string-typed parameters — workdir, host, security, ask, and node — that could be equally affected if the provider leaks the </arg_value> artifact into those values. A malformed workdir would silently break the working-directory resolution, which is hard to debug.
Consider sanitizing the full set of string exec params, or centralising via a shared helper similar to stripXmlArgValueSuffixFromRecord:
// Strip all string exec params, not just command
for (const key of ["command", "workdir", "host", "security", "ask", "node"] as const) {
if (typeof params[key] === "string") {
params[key] = stripXmlArgValueSuffix(params[key]!);
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bash-tools.exec.ts
Line: 230-232
Comment:
**Other string exec params not sanitized**
Only `params.command` is stripped, but the exec tool also accepts other string-typed parameters — `workdir`, `host`, `security`, `ask`, and `node` — that could be equally affected if the provider leaks the `</arg_value>` artifact into those values. A malformed `workdir` would silently break the working-directory resolution, which is hard to debug.
Consider sanitizing the full set of string exec params, or centralising via a shared helper similar to `stripXmlArgValueSuffixFromRecord`:
```typescript
// Strip all string exec params, not just command
for (const key of ["command", "workdir", "host", "security", "ask", "node"] as const) {
if (typeof params[key] === "string") {
params[key] = stripXmlArgValueSuffix(params[key]!);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs changes before merge. Summary Reproducibility: yes. for source-level reproduction: the linked bug gives the Windows Qwen/DashScope corrupted Next step before merge Security Review findings
Review detailsBest possible solution: Land a rebased, field-scoped sanitizer on current main that strips the malformed suffix only from command/path-like tool arguments, covers all exec string options, preserves write/edit payload text, and adds regression plus negative tests. Do we have a high-confidence way to reproduce the issue? Yes for source-level reproduction: the linked bug gives the Windows Qwen/DashScope corrupted Is this the best way to solve the issue? No. The defensive direction is right, but this implementation targets a stale normalizer, mutates all string params including payload text, and only sanitizes exec Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 89db1e5440f5. |
Summary
Fixes #48780
Some models (e.g. Qwen via DashScope) emit tool call arguments with trailing XML fragments like
</arg_value>>appended to string values. This corruptsexec()commands (e.g.echo "test"becomesecho "test</arg_value>>) andread()file paths, blocking all file operations and command execution on affected platforms.Changes
src/agents/pi-tools.params.ts: AddstripXmlArgValueSuffix()function andstripXmlArgValueSuffixFromRecord()helper that strips</arg_value>with optional trailing>characters from all string values in tool call argument records. Called fromnormalizeToolParams()which coversread,write, andedittools.src/agents/bash-tools.exec.ts: Import and applystripXmlArgValueSuffix()to thecommandparameter in the exec tool's execute handler, since exec does not go throughnormalizeToolParams.src/agents/pi-tools.params.test.ts: Add unit tests covering the suffix stripping for various patterns and integration withnormalizeToolParams.Root cause
Models using XML-based internal tool calling formats (common with Qwen/DashScope) sometimes fail to properly separate the argument value from the XML closing tag, resulting in
</arg_value>>being concatenated to the end of string argument values. This is a provider-side bug, but we can defensively strip these artifacts at the parameter normalization layer.Test plan