Skip to content

Recover text tool calls and handle a non-SSE response body - #552

Open
Ticed wants to merge 3 commits into
infernode-os:masterfrom
Ticed:fix/llmclient-sse-and-tool-json
Open

Recover text tool calls and handle a non-SSE response body#552
Ticed wants to merge 3 commits into
infernode-os:masterfrom
Ticed:fix/llmclient-sse-and-tool-json

Conversation

@Ticed

@Ticed Ticed commented Aug 25, 2026

Copy link
Copy Markdown

What this changes

Two things in the OpenAI-compatible client, both about responses that do not
arrive in the shape the code assumed.

Tool calls emitted as bare JSON text. Some models answer a tool-enabled
request with the call as ordinary assistant content instead of filling the
structured tool_calls field, so the call is never made and the user sees raw
JSON. extracttexttoolcalls already recovered three shapes; this adds a
fourth, a lone {"name":..., "arguments"|"parameters":{...}} object.

It only accepts content that is entirely one JSON object, whitespace aside.
Prose around it is rejected, so a model quoting an example of a tool call
cannot turn that example into a real invocation. jsonobjectend is
string-aware, so braces inside string values do not end the object early, and
jsonunslash undoes the \/ escaping json.text() applies, which otherwise
mangles paths on the TOOL: line.

A JSON body from a server that ignored stream:true. The client asked for
a stream and then parsed whatever came back as SSE. A server that answers a
streaming request with one complete chat.completion object produced an empty
response and no error. ssebodymode looks at the first non-whitespace byte of
the body: { means a complete object, anything else is treated as SSE.

Tests

tests/llmclient_texttools_test.b is new: five cases over
extracttexttoolcalls, including the rejections.

tests/llmclient_sse_fallback_test.b gains the non-SSE cases. Both files are
added to tests/mkfile TARG — they are built and run rather than sitting in
the tree unwired.

Against the current dis/lib/llmclient.dis, before the fix:

--- FAIL: StreamFallbackOnPlainJson (0.20s)
--- PASS: NonStreamingBaseline (0.20s)
--- FAIL: StreamFallbackTolerantOfLeadingWhitespace (0.21s)
--- FAIL: ToolCallsArrayFromSpecial66 (0.20s)
--- PASS: IncrementalSseNoDuplication (0.41s)
--- PASS: SseTrailerNotReparsed (0.41s)
3 passed, 3 failed

After, on macOS ARM64:

llmclient_sse_fallback       6 passed
llmclient_texttools          5 passed
llmclient_reqshape           4 passed
llmclient_think_gating       5 passed
llmsrv                       62 passed

IncrementalSseNoDuplication and SseTrailerNotReparsed pass before and
after on purpose. They pin behaviour this branch must not break: that a
streamed reply is assembled once, and that JSON arriving after [DONE] does
not replace it. An early version of the body-mode check did break the second
one, which is why it is pinned here.

module/llmclient.m gains one function, so llmsrv was rebuilt against it;
its bytecode is unchanged and its 62 tests still pass.

@Ticed

Ticed commented Aug 28, 2026

Copy link
Copy Markdown
Author

One thing worth stating explicitly about the test evidence here.

tests/llmclient_sse_fallback_test.b is present on master but absent from
tests/mkfile TARG, so it was never built and never run. It looked like
coverage without being any.

Wired against the base revision, it fails the plain-JSON streaming fallback,
the leading-whitespace tolerance, and the escaping on the recovered tool-call
path. All six assertions pass with this PR. Both test files are added to TARG
here, so they run from now on.

This came out of an audit of orphaned tests. The wider TARG wiring is being
handled separately — #563 covers the prerequisite work, and the remaining
entries depend on #559 and #560 landing.

The GoDis job is the only red check on this PR and is unrelated to the change.
#566 removes it.

@pdfinn

pdfinn commented Aug 31, 2026

Copy link
Copy Markdown
Member

Heads-up on merging this one, and it is my doing rather than yours.

This branch predates #560, which stopped tracking compiled bytecode. It has a
.dis file in its diff, so merging hits:

CONFLICT (modify/delete): <path>.dis deleted in HEAD and modified in <branch>.
Version <branch> of <path>.dis left in tree.

Git leaves the file sitting in the working tree, so resolving with git add .
would quietly re-commit bytecode that master deliberately removed. The
resolution is to delete it:

git rm <path>.dis

Or just rebase onto current master, where the file no longer exists and the
conflict does not arise.

Nothing else in the PR is affected — dis/ is a build product now, rebuilt
with:

for d in appl appl/mpeg appl/veltro tests; do (cd $d && mk install); done

hooks/post-merge does that automatically after a pull if you have run
./hooks/install.sh.

Sorry for the friction — five open PRs are in this position because of the
timing.

@pdfinn pdfinn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one turns model text into executed tool calls, which is AGENTS.md's first
threat, so I probed the guard rather than reading it. It holds.

Applied onto current master (the branch is pre-#560), built, and drove
extracttexttoolcalls directly with a granted tool set of [exec]:

  prose then object        -> 0 call(s)
  object then prose        -> 0 call(s)
  two objects              -> 0 call(s)
  ungranted tool name      -> 0 call(s)
  brace inside string      -> 1 call(s)     (correctly NOT truncated)
  markdown fenced          -> 0 call(s)
  legit bare call          -> 1 call(s)

Both constraints do real work. jsonobjectend(s) != len s rejects anything
with prose or a second object around it, so a model quoting an example cannot
have that example fired — and the string-awareness is genuine, since a }
inside a value does not end the object early. validtoolname(bname, tooldefs)
means a recovered call can only name a tool the agent was already granted, so
this cannot widen the tool set, only the encoding a granted call arrives in.
That is the right boundary: it prevents escalation without pretending to
decide whether a granted tool should have been called.

Your own tests pass too — 5 in llmclient_texttools_test, 6 in
llmclient_sse_fallback_test.

Adding both to tests/mkfile TARG rather than leaving them unwired is the
right instinct and increasingly rare; thank you.

One thing to add

dis/tests/llmclient_texttools_test.dis is not in tools/dis-manifest.txt.
The build flagged it:

note: built but not listed in tools/dis-manifest.txt:
    dis/tests/llmclient_texttools_test.dis

llmclient_sse_fallback_test.dis is already listed, so it is just the new
file. Since #560 the manifest is the tracked record of what the build must
produce, so a new module needs a TARG entry and a manifest line in the same
commit — that is in AGENTS.md now. One line.

A question, not an objection

Markdown-fenced output is rejected:

```json
{"name":"exec","arguments":{"cmd":"id"}}

Rejecting is the safe direction and I would not change it without thought. But
fencing is probably the single most common way a model emits JSON in prose, so
the feature may be missing the case it most wants. Accepting it would mean
stripping a fence before the whole-content check, which widens the surface a
little — an injected fenced block would then fire, where today it does not.

Worth saying explicitly in the code which way you chose and why, so the next
person does not "improve" it by adding fence-stripping without noticing they
are relaxing an injection guard.

## Smaller

`jsonunslash` undoing `json.text()`'s `\/` escaping is right, and the comment
explaining that tool args are not XML is the kind of thing that stops a future
revert. `ssebodymode` keying on the first non-whitespace byte is a reasonable
heuristic and the `0 = still unknown` state handles a body that has not
arrived yet.

Add the manifest line and this is good. Also needs the three `.dis` conflicts
resolved by deletion — see my other comment.

@Ticed
Ticed force-pushed the fix/llmclient-sse-and-tool-json branch from 984ce5f to f8e80ce Compare August 31, 2026 10:24
@Ticed

Ticed commented Aug 31, 2026

Copy link
Copy Markdown
Author

Added the manifest line for dis/tests/llmclient_texttools_test.dis, and
rebased onto current master so the three .dis conflicts resolve by deletion
rather than by keeping the branch's copies.

On the fenced-block question — I kept the rejection and said so in the code,
above the whole-content check:

A fenced block is deliberately not accepted, common though it is as a way for
a model to emit JSON. The whole-content check is what stops a quoted example
from being executed, and stripping a fence ahead of it would let an injected
fenced block fire. Relax this only with that trade-off in view.

Accepting a fence means stripping it before the whole-content check, which is
the check doing the security work. Better to lose an encoding than to widen the
surface for it.

Verified cd tests && mk install puts both modules at the manifest's paths.

@Ticed

Ticed commented Aug 31, 2026

Copy link
Copy Markdown
Author

On the two jobs that are red here beyond Fuzz — I triaged them rather than
asking you to, and neither is this PR.

Built three trees on macOS arm64 and ran an A/B/C: master@053fb810, this
branch's tip, and e1023a2f^.

Boot SEGV: 0/30 failures at each of the three. 90 boots, no repro. Weak on
its own — arm64 here against amd64 in CI.

GoDis TestE2ELocked: this is the flake you documented yourself in #372.

the emulator faults nondeterministically on a small, run-varying set of corpus
programs — under BOTH -c0 and -c1 [...] always ~1 of 266 [...] not
something a given PR introduced.

The failure here was match 245, c0!=c1 1, skipped 20. One of 266. Reproduced
the class locally: seed_wordfreq crashes 100/100 in isolation under -c1 and
passes ~80% in suite context. filepath_pkg.go hammered 200x in both modes gave
0 anomalies.

I had suspected e1023a2f's freemod munmap — a stale pointer into unmapped
text would explain both a SEGV and a one-bit string flip. The rates say no:
3/12 instrumented failures at this branch's tip against 2/12 at e1023a2f^,
and that commit is the only emulator delta between those checkouts.

One thing worth your attention, because #372's retry does not cover this
case.
runEmu retries only when the run crashed:

if !crashed || attempt >= maxEmuAttempts {
    return out, crashed
}

This run did not crash. It completed and printed

sh: /tnp/difftest/b4d7c81159bcde8c.dis: '/tnp' file does not exist

/tmp came back as /tnpm is 0x6D, n is 0x6E, one bit. Same heap-layout
corruption, expressed as a corrupted string rather than a fault, so crashed is
false, the retry never fires, and the bad output goes to the comparison and is
reported as a locked regression.

So the gate is protected against the crash variant and not the silent one. Happy
to send a change if you want one — retrying when a mode's output names a path
the runner did not pass in is the narrow version; retrying any c0 != c1 once
is the blunt one, and leans on the same determinism assumption #372 already
makes.

For this PR: re-running both jobs should clear them. Nothing here needs a code
change.

Fuzz (PR) is the separate ClusterFuzzLite link break from #570, fixed in #578.

@Ticed
Ticed force-pushed the fix/llmclient-sse-and-tool-json branch from f8e80ce to e15f61f Compare September 2, 2026 03:14
@Ticed

Ticed commented Sep 2, 2026

Copy link
Copy Markdown
Author

Rebased onto current master (df34b02). Force-pushed f8e80ce -> e15f61f.

git range-diff reports every commit unchanged — same content, same messages,
only the base moved. The five commits that landed underneath are #583, #582,
#566, #578 and #580; none of them touches this branch's files.

Re-verified on macOS after the rebase.

The llmclient_texttools cases pass, including the bare-parameters, bare-
arguments and whitespace variants and the prose-is-not-a-call negative.
verify-dis-build.sh reports all 964 manifest modules built.

#578 fixed the ClusterFuzzLite link break, so Fuzz should now be green here
rather than red for a reason that was never this branch's.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants