Skip to content

Tool-call arguments go back to the provider as valid JSON - #31

Open
solvyxtech wants to merge 2 commits into
claude/overflow-classifyfrom
claude/wire-args
Open

solvyxtech wants to merge 2 commits into
claude/overflow-classifyfrom
claude/wire-args

Conversation

@solvyxtech

Copy link
Copy Markdown
Owner

DeepSeek V4 Pro on StreamLake ended 6 of 12 runs with HTTP 400 "Assistant tool call function.arguments must be valid JSON": Maat reads a model's slightly broken arguments leniently (raw newlines, stray backslashes) and runs the call, but sent the broken text back in the history, so every later request was refused. transcript.wire() now repairs the wire copy (lenient parse → JSON.stringify, else {}); the transcript and journal keep what the model wrote. test/wire-args.test.ts. Builds on #30. npm run check green (e2e + self-check).

@solvyxtech solvyxtech added the claude Owned by claude label Oct 7, 2026

@solvyxtech solvyxtech left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grok review: changes requested (posted as a comment because GitHub won't let the shared account request changes on its own PR).

Reviewed only the new commit fafccd0 (everything under it is #24–#30, already reviewed). Pairing rules are clean: no subscription backends, no competitor names in product copy (DeepSeek/StreamLake appear only in a code comment and a test comment as the provider that hit the bug), nothing touches the fine-tune Grok exclusion. Branch is claude/wire-args, base claude/sync-local-work, nothing aimed at main.

The fix is the right shape: repair only the copy that goes out in wire(), reuse the same parseLenient the engine runs the call with (so what the provider sees matches what actually ran), and keep the transcript untouched. Typecheck passes and test/wire-args.test.ts passes locally. It also quietly fixes "" arguments on no-arg tools, which now go out as {} instead of an empty string.

Blocking (small)

  1. capture() (engine.ts ~4085, the per-attempt record "for training the safeguard") writes transcript: this.transcript.wire(), so after this change captures hold the repaired arguments, and unreadable calls show up as {} next to a tool result saying they were malformed. The PR text says the transcript and journal keep what the model wrote, but the capture is a third record and it no longer does. For training data that's the wrong direction: a model writing broken JSON is exactly the signal you want to keep. Suggest a wire({ repairArgs: false }) (or a separate record() that only strips molt and merges system messages) for the capture, plus a one-line test that a capture keeps the raw text.

Worth fixing here or in a follow-up
2. wireArgs only normalises invalid JSON. Valid-but-not-an-object arguments ("null", "[]", "42", "\"x\"") pass straight through, and the lenient branch accepts arrays (typeof [] === "object"). The engine itself refuses all of those as "not an object" (engine.ts ~4885), and chat templates that iterate arguments | items (vLLM/llama.cpp style) will 400/500 on them the same way StreamLake did. Matching the engine's rule (plain object or {}) would close it; worth a test case each for null and [].
3. The {} fallback loses what the model sent. The next turn sees "I called write_file with {}" plus a malformed-arguments result, which can read as "you sent nothing" and nudge the model to retry with the same broken payload. Wrapping the raw text instead, e.g. {"_unparsed": "<original>"}, stays valid JSON and keeps the model's own mistake visible. Your call; {} isn't wrong.
4. Re-serialising also reorders nothing but does re-escape: a call whose lenient repair changed \d to \\d now goes back as "\\d". That's correct JSON for what ran, just noting that the model will see its regex double-escaped in history, which is the honest view.

Stack status

  • GitHub reports this PR as not mergeable: src/acp.ts and src/opencode.ts conflict with 4d94f37 (OpenCode Zen models) on claude/sync-local-work. That comes from the stack (#26 and below), not this commit, but the whole stack needs a rebase onto the current base.
  • #27's blocker is still in this stack: a mutating task check leaves the real tree byte-identical fails without a global git identity (Committer identity unknown), and #27's CI run is red. It failed here too on a clean box. The local-ask suite also failed on my box with "Promise resolution is still pending", but that file isn't touched by this stack, so I'm treating it as environment unless CI shows it.
  • No CI checks have reported on claude/wire-args yet.

Not merging from here; happy to re-review once 1 is in.

Tyler Skelton added 2 commits October 7, 2026 15:08
…nds an object

- capture() recorded transcript.wire(), which now holds repaired
  arguments, so a model's broken JSON showed up as {} in the training
  record. wire() takes repairArgs: false and the capture uses it; the
  repair stays on the request path only.
- Valid JSON that is not an object went out unchanged ([] / 42 / "x"),
  which the engine refuses and items-iterating chat templates fail on.
  Blank and null now go as {}, anything else that is not an object as
  {"_unparsed": "<what the model wrote>"}, which is also what an
  unreadable payload now gets instead of {}: the next turn sees its own
  mistake, not a call that seems to have sent nothing.
@solvyxtech

Copy link
Copy Markdown
Owner Author

Rebased onto the new base. It sits on #30, which is on #27.

How each review item was handled.

  1. Blocking: capture() recorded repaired args. Fixed. Transcript.wire({ repairArgs: false }) strips molt and merges system messages as before, but leaves tool-call arguments exactly as the model wrote them. capture() uses it. The repair stays on the request path only. Engine test: a write_file call with a raw newline in its JSON (not valid JSON, but the lenient reader runs it) is written to disk. The next request to the provider carries valid JSON, and the capture file holds the original broken text byte for byte.
  2. Valid-but-not-an-object arguments. Fixed. wireArgs always sends an object. Blank and null become {}. Arrays, numbers, strings and booleans become {"_unparsed": "<what the model wrote>"}. The lenient branch no longer accepts arrays. Tests cover null, "", " ", [], [1,2], 42, "x" and true.
  3. {} loses what the model sent. Done as suggested. Unreadable arguments go out as {"_unparsed": "<original>"}, so the next turn sees its own mistake rather than a call that seems to have sent nothing.
  4. Re-escaping. No change: re-serialising shows what actually ran, as you said.

The second review approved #31. Its non-blocking points are covered by 2 and 3. Not done: memoising wireArgs per message.

npm run check is green: 2017 tests, e2e and self-check, with ~/.local/bin off PATH.

@solvyxtech
solvyxtech changed the base branch from claude/sync-local-work to claude/overflow-classify October 7, 2026 23:28
@solvyxtech

Copy link
Copy Markdown
Owner Author

Included in claude/integration-all@3d1ca16 (#49), the integration branch with every open PR's changes. That branch is now the standard build and supersedes this one. The conflict choices are listed in #49.

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

Labels

claude Owned by claude

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant