Plugins can break the wire protocol

[erkinalp]

Implemented a fix in my branch: https://github.com/erkinalp/deepseek-harness

[weijiafu14]

I read the branch (erkinalp:master, one commit ahead, two files: packages/host/apiproxy/src/api-proxy.ts +7/-1 and packages/llm/llm-deepseek/src/serialize.ts +59/-2). Some review notes, plus the one question the post needs to answer to be actionable.

The question first: which plugin, doing what? The title says plugins can break the wire protocol, but nothing in the diff or the post says how a plugin gets an unbalanced tool_calls sequence into the history. That matters a lot for where the fix belongs — if a plugin can inject messages that leave an assistant tool_calls without matching tool replies, the higher-value fix may be at the injection point, and ensureToolCallsBalanced is then the fail-soft net rather than the repair. A minimal repro (which plugin, what it injected, the resulting wire) would let people evaluate both.

What the patch gets right. Synthesizing a role: 'tool' reply for an orphaned tool call is not optional — OpenAI-compatible endpoints hard-reject an assistant message carrying tool_calls without matching results, and since the bad shape sits durably in the session log, every later turn of that session fails, not just the one that produced it. So a serialize-time net is the correct kind of thing to build. It is the same fail-soft argument being made on #4703 for the token-accounting underflow: a display-layer defect should not escalate into an unusable session.

Where I would push back: the fabricated call. This branch:

lastMsg.tool_calls.push({
  id: msg.tool_call_id,
  type: 'function',
  function: { name: 'tool', arguments: '{}' },
})

invents an assistant action that never happened, and gives it a fake tool name. #4671 just established on this repo that the name is load-bearing — ToolNotFoundError is constructed from the tool name (core/tools/src/index.ts:504), which is why a synthesized id cannot rescue an empty name there. Writing name: 'tool' into the model's view of its own history is the same category of thing from the other direction.

Your own else branch already has the honest alternative: render the orphan as a role: 'user' text message ([Tool Result <id>]: …). That preserves the content, tells no lie about what the assistant did, and is accepted everywhere. I would use it in both cases and drop the fabrication.

One thing worth a test, because I read it rather than ran it. In the orphan path you call flushPending() first, which pushes synthetic role: 'tool' messages onto result. Immediately after, lastMsg = result[result.length - 1] is therefore a tool message, not an assistant one — so when there are pending calls, the lastMsg.role === 'assistant' branch looks unreachable and control always lands in the user-message fallback. Whether that is intended or not, it deserves a case in the suite; right now the two branches are distinguishable only by a fairly subtle ordering.

One thing that is fine, in case a reviewer flags it. lastMsg.tool_calls.push(...) mutates in place, which would be alarming if the wire array shared structure with the session Message. It does not: serializeAssistant builds a fresh object and a freshly-mapped toolCalls array on every call, so the mutation cannot reach session state.

One interaction to test together. #4671 is an inbound bug that produces tool calls with an empty id. Your if (tc.id !== undefined && tc.id.length > 0) guard means such a call never enters pendingToolCallIds, so its result takes the orphan path. The two patches touch the same failure surface from opposite ends and are worth exercising in one test.

Interest disclosure: I maintain a third-party DSH plugin, so "plugins can break the wire protocol" is a claim I have a stake in understanding precisely — which is most of why I want the repro. We do not modify DSH's own adapter or apiproxy and could not fix either; the above is a read of your diff against the same source, not a claim of authority over it.

[erkinalp]

The question first: which plugin, doing what? The title says plugins can break the wire protocol, but nothing in the diff or the post says how a plugin gets an unbalanced tool_calls sequence into the history.

Very easy to repro: Any plugin that can call a tool, or without relying on a broken plugin: simply interrupt the session by restarting the harness mid-call.

[denial123789]

I reviewed the two changed paths and think the safest framing is three repair layers, because the serializer alone cannot establish execution truth:

1. Producer admission: reject a plugin/core message with invalid role, source, block shape, empty/duplicate call ID, or correlation before durable append. The title's plugin attribution still needs a minimal repro naming the producer and exact injected messages.
2. Agent-loop closure: every authentic assistant call gets exactly one ordered result before turn/end, with TOOL_NOT_STARTED only before durable dispatch and TOOL_OUTCOME_UNKNOWN after it.
3. Adapter containment: keep an old poisoned Session from producing provider-invalid wire, but emit a repair classification and do not mutate durable history.

At adapter time, a pending call with no result generally lacks enough execution evidence to say merely “interrupted”; conservative unknown-outcome wording is safer. For an orphan result, synthesizing function.name = "tool" and {} arguments invents an assistant action and changes model-visible history. Quarantine it as source-attributed evidence or exclude it from the provider request while retaining diagnostics; do not manufacture a tool protocol pair.

A few regression cases seem essential: empty IDs, duplicate IDs, multiple parallel calls preserving declaration order, orphan before/after pending flush, malformed arguments breaking only presentCall, and identical poisoned fixtures across DeepSeek and pi-ai/OpenAI-compatible adapters. The current flushPending() ordering also deserves a direct test because it changes the array tail before the orphan branch checks lastMsg.role.

I incorporated the patch review into a revised source-backed recovery guide and linear transcript validator: https://sandbaseai.github.io/deepseek-harness-handbook/insufficient-tool-messages.html

Disclosure: I maintain the independent community handbook linked above.

[erkinalp]

Thanks both — this is exactly the kind of review I needed. Update to the branch is in; the headline change is that I dropped the fabricated call.

@weijiafu14 — you're right that the serialize-time net is the fail-soft layer, not the repair. On the diff itself:

• Fabrication removed. The orphan path no longer does lastMsg.tool_calls.push({ …, function: { name: 'tool', arguments: '{}' } }). An unmatched tool result is always re-expressed as role:'user' with [Tool Result ]: …, in both the pending-flush and no-pending cases, which is the honest form from the else branch.
• Ordering point is moot now. Since the assistant branch is gone, the flush-then-lastMsg.role==='assistant' unreachability disappears; I pinned the flush-then-orphan order with a dedicated test.
• Mutation point: agreed, serializeAssistant builds a fresh object and fresh toolCalls array each call, so the in-place push can't reach session state — but the push is gone anyway.
• dsh-llm-deepseek drops tool call id/name when provider streams explicit nulls (every tool fails: unknown tool "") #4671 interaction: added a test showing an empty call id is never registered, so a real-id result takes the orphan path. Fully closing the empty-id call itself is the inbound dsh-llm-deepseek drops tool call id/name when provider streams explicit nulls (every tool fails: unknown tool "") #4671 fix — I've documented that boundary in the test comment rather than trying to manufacture a result for an id we can't trust.

@denial123789 — I agree with the three-layer framing, and the two higher layers (producer admission, agent-loop closure) are where the real repair belongs. What I changed on the containment layer:

• No manufactured protocol pair. Orphan results are re-expressed as user messages (content preserved, accepted everywhere) rather than excluded — I kept the content because the model may need it, and left the "quarantine/classify" idea to the injection layer.
• Unknown-outcome wording. The pending-unanswered closure now reads [Tool call produced no recorded result (outcome unknown)], matching your conservative framing rather than asserting "interrupted".
• No durable-history mutation. The net operates on the fresh wire array serializeMessages/serializeMessagesWithImages build; session messages aren't touched.
• Regression cases added: empty IDs, duplicate result IDs, parallel calls preserving declaration order, orphan before/after pending flush, plus a balanced-conversation no-op guard and the trailing-dangling-call case.

Cross-adapter note: this fix lives in llm-deepseek's serializer; llm-pi-ai has its own adapter path, so the identical poisoned fixtures would need the same treatment there if you want it applied consistently.

On the repro — it's simpler than a specific misbehaving plugin: restart the harness mid-call (any plugin that calls a tool), and the assistant tool_calls sit in the session log with no matching result. That's the durable poison; the net now also closes the tail-of-window case, not just a mid-history one.