Repository navigation
Tool-call arguments go back to the provider as valid JSON - #31
solvyxtech wants to merge 2 commits into
Conversation
solvyxtech
left a comment
There was a problem hiding this comment.
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)
capture()(engine.ts ~4085, the per-attempt record "for training the safeguard") writestranscript: 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 awire({ repairArgs: false })(or a separaterecord()that only stripsmoltand 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.tsandsrc/opencode.tsconflict with4d94f37(OpenCode Zen models) onclaude/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-identicalfails without a global git identity (Committer identity unknown), and #27's CI run is red. It failed here too on a clean box. Thelocal-asksuite 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-argsyet.
Not merging from here; happy to re-review once 1 is in.
… every request after one broken call
…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.
|
Rebased onto the new base. It sits on #30, which is on #27. How each review item was handled.
The second review approved #31. Its non-blocking points are covered by 2 and 3. Not done: memoising
|
fafccd0 to
3079b1a
Compare
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).