fix(llm): fail closed on malformed native tool calls

[canblmz1]

Summary

Prevents native OpenAI-compatible tool calls from executing when their arguments are malformed or when a tool-call stream ends without a confirmed terminal signal.

Root cause

Two behaviors combined:

• malformed non-empty tool arguments were silently parsed as {} after JSON.parse failed
• a tool-call stream ending by bare EOF with neither finish_reason nor [DONE] was treated like a confirmed completion

That allowed malformed or unconfirmed tool arguments to reach the normal execution path.

Fix

• malformed non-empty tool arguments now surface through the existing parse failure / repair path instead of becoming {}
• the stream consumer records whether a terminal signal was actually observed
• pending tool calls whose stream ends without a terminal signal route through the existing truncated/model-failure path before dispatch

Legitimate zero-argument calls remain supported.

A provider that sends an explicit finish_reason without [DONE] remains accepted, explicit finish_reason: "length" behavior is unchanged, and plain-text-only responses are unaffected.

Validation

Execution-level regression coverage using the real provider, step executor, ToolRegistry, and instrumented no-op tools:

• healthy valid tool call -> executes once
• malformed args + clean terminal -> 0 executions
• malformed args + abrupt EOF -> 0
• container-level truncation + EOF -> 0
• syntactically complete args + ambiguous EOF -> 0
• explicit finish_reason: "length" -> 0
• parallel complete + truncated under ambiguous EOF -> 0 / 0
• stream read error -> 0

Also verified:

• explicit finish_reason without [DONE] remains accepted
• plain-text bare-EOF behavior remains unchanged

Tests

• npm run lint (typecheck): clean
• npm run build: clean
• Targeted suites (openai provider/stream-consumer/tool-call-adapter, step-executor, reliability, parallel-tool-calls integration, plus the new execution-integrity suite): 135/135 passed
• Full suite: 3983 passed, 20 failed, 5 skipped (4008 total)
• The same 20 failures reproduce identically on unmodified current main (same test names, same files) — confirmed by running the full suite on both in the same session. No new failure names introduced by this change.

[sosidudku1]

Thanks for this. The failure mode you are closing is real and it is the expensive kind: a truncated function.arguments string silently became {} and the tool still ran. For os.fs.delete or os.shell.run that turns "the stream got cut mid-argument" into "we executed something the model never asked for", and in unattended runs nobody is watching when it happens.

The test design is also a step above the usual: driving the real OpenAiProvider and the real executeStep() with a counting no-op tool proves the tool never got invoked, rather than proving a function returned an error. The redaction test on ToolCallArgumentsParseError and the two compatibility tests for providers that omit [DONE] show you thought about the blast radius.

I merged this onto current main and ran it. It merges clean, npm run lint is clean, and the full suite is 4851 passed / 2 failed, with both failures reproducing identically on unmodified main (send-message-concurrency, fs-glob-real). So nothing here regresses the suite.

Two things I would want resolved before this lands, both found by tracing the flag rather than by the tests.

1. The qwen tagged path gets no protection at all (openai-provider.ts:230)

hasPendingToolCalls reads streamFinal.toolCalls, but on the taggedToolCompatibility: "qwen" path the tool calls do not exist yet at that point. They are synthesised from content by adaptQwenCompletionResult at line 154, which runs after completionFromStreamFinal at line 137 has already computed truncated and stop.

Probe on the merged tree, qwen-compat provider, tagged call in content, connection closed with no finish_reason and no [DONE]:

QWEN   -> toolCalls: 1 | truncated: false | stop: true | finishReason: tool_calls
NATIVE -> toolCalls: 1 | truncated: true  | stop: false

Same ambiguous termination, same dispatchable call, opposite verdict. The native path is protected and the qwen path is wide open. Either the guard needs to move after the adapt seam (or run again there), or the PR description should state the qwen path is out of scope and why, so it does not come back as a follow-up issue.

2. A final chunk without its trailing blank line is now misread as ambiguous (openai-stream-consumer.ts:47)

terminalObserved is only assigned inside the \n\n-delimited event loop (line 55). If a provider or proxy sends a valid terminal chunk as the last bytes without the trailing \n\n, that event is never parsed, so neither branch that sets terminalObserved is reached.

Differential probe, identical request body, only the tree differs:

clean main:  finishReason: tool_calls | truncated: false   -> call executes
with PR:     finishReason: null       | truncated: true    -> ModelError

This one worries me more than a missed detection, because it is a false positive on a working path and there is no way back. detectModelFailure turns truncated: true into a thrown ModelError("truncated") before the parser ever runs, and unlike reason "empty", "truncated" has no exemption in isNativeToolsEmptyCompletionHandledByParser. So there is no repair round-trip and no recovery: every request to such a provider fails. Flushing the tail buffer as a final event at EOF would cover it.

Two smaller ones, not blocking:

3. openai-tool-call-adapter.ts:143 The catch is unqualified, so any error from the try block gets relabelled ToolCallArgumentsParseError(name) and the original is dropped. Only SyntaxError from JSON.parse is intended here. If something unrelated ever throws inside that block, the operator and the repair prompt both get told the arguments were malformed, the one-shot repair is spent on a non-problem, and the real cause never reaches the logs. Narrowing to SyntaxError (and letting anything else through) keeps the diagnostic honest.

4. native-tool-call-execution-integrity.test.ts:244 Test 7 hand-builds its CompletionResult with truncated: true instead of driving a real stream, so it exercises only the step-executor half. Its comment says the literal is "exactly what completionFromStreamFinal() now derives", but nothing in the suite enforces that. If the provider-side computation regresses for parallel calls, test 7 stays green. Worth driving it through a real fetch like tests 1 to 6, or at least asserting the provider derives that shape.

On the whole: right diagnosis, right instinct to fail closed, and the zero-arg case is handled correctly. It is finding 2 that I think has to change before merge, since it converts a working provider into a hard failure.

[canblmz1]

Thanks for tracing this past the tests — both blockers were real, and the second one in particular caught a false-positive failure mode I definitely wouldn't want to ship.

I've updated the existing branch and addressed all four points:

1. Qwen tagged calls — the termination-safety decision now runs after adaptQwenCompletionResult(), so it sees the final dispatchable toolCalls for both native and tagged paths. A synthetic finishReason: "tool_calls" from the adapter no longer counts as proof that the provider actually terminated cleanly. I added coverage for both ambiguous-EOF Qwen (blocked) and explicitly terminated Qwen (allowed).

2. Final SSE event without the trailing blank line — on EOF the consumer now flushes the TextDecoder and runs any remaining non-empty buffer through the same parseOpenAiSseEvent() path as delimiter-separated events. I added execution-level cases for both a terminal finish_reason event and [DONE] arriving as the final bytes with no trailing \n\n.

3. Broad catch — agreed. Only SyntaxError is translated to ToolCallArgumentsParseError now; unrelated exceptions are rethrown unchanged.

4. Parallel-call test — Test 7 no longer hand-builds truncated: true. It now drives a real two-call SSE stream through OpenAiProvider, asserts the provider derives truncated: true / stop: false, and then proves neither counting tool is invoked by the real executor.

I also merged the current upstream main into the branch so the PR is now based on the same tree you're reviewing against, without a force-push.

I don't have a fresh full-suite execution to quote from this environment, so I don't want to recycle the previous numbers as if they covered these follow-up changes. The regression coverage is on the branch now, and I'd appreciate a rerun of the same current-main suite when you get a chance.

Thanks again for the careful review. The Qwen seam and especially the undelimited terminal event were exactly the kind of cross-path details this change needed checked

[sosidudku1]

Reran everything against current main and merged this as dcf77f1. Thanks for the quick turnaround on all four points.

Re-probed both blockers with the same probes that originally caught them:

qwen tagged call + bare EOF        -> truncated: true   (was false)
qwen tagged call + clean terminal  -> truncated: false, executes
terminal event with no trailing \n\n -> truncated: false, executes (was true)
[DONE] as final bytes, no trailing \n\n -> executes
native bare EOF (control)          -> still blocked

I also probed the new EOF tail-flush for cases the fix could have opened up, since an undelimited tail now gets parsed instead of discarded:

stream cut mid-JSON in the final event -> truncated: true
tail appends more tool args, no terminal -> truncated: true
SSE comment (": ping") after a clean terminal -> not confused, executes
multi-byte UTF-8 split across chunk boundary -> decoded correctly
empty body / [DONE]-only -> no crash
abort mid-stream with a pending call -> throws before dispatch

Nice catch on the detail I did not spell out: treating the adapter's synthetic finishReason: "tool_calls" as non-evidence and keying off the real terminalObserved is the right call, and pulling it into applyToolCallTerminationSafety() makes the native and tagged paths provably share one decision.

Suite on the merged tree: 5721 passed, 0 new failures against a same-session main baseline (identical failure set, both trees). lint and build clean.

One non-blocking follow-up, no action needed for this PR. I ablated your three fixes to check the tests actually pin them: removing the guard fails 4 tests and reverting the EOF flush fails 2, both good. But widening the catch back to bare catch {} leaves all 12 adapter tests green, so the SyntaxError-only narrowing is correct but unpinned. A case asserting a non-SyntaxError propagates unchanged would stop that silently regressing later.