fix(agent-core): recover sessions bricked by orphan tool results (#1308)

* fix(agent-core): recover sessions bricked by orphan tool results

A stray `tool` message with no preceding assistant `tool_calls` permanently
bricked a session on OpenAI-compatible providers: every turn re-sent the same
malformed history and got a 400, and switching model/provider did not help.

Two independent gaps caused this:

- kosong did not recognize the OpenAI / DeepSeek / vLLM / Qwen phrasings of the
  tool-exchange structural 400 (`role 'tool' must be a response to a preceding
  message with 'tool_calls'` and the mirror `assistant message with 'tool_calls'
  must be followed by tool messages`), so the post-400 strict-resend fallback
  that drops the orphan never fired.

- The legacy-restore compaction path kept a verbatim tail
  `history.slice(compactedCount)`; when the cut landed inside a tool exchange the
  tail began with an orphan tool result whose assistant was summarized away. The
  normal projection does not repair a leading orphan, so the malformed history
  was baked in and re-sent every turn.

Recognize the additional phrasings so the strict resend un-bricks any session,
and trim leading tool results from the legacy-restore tail so the orphan is
never persisted in the first place.

* fix(agent-core): drop orphan tool results at the projection boundary

Rework the legacy-restore half of the fix based on review feedback: mutating
`_history` at restore time desyncs every consumer that models the history from
the wire records — the transcript reducer's fold length would overcount and
make MessageService skip unflushed live-tail messages.

Keep the restored history faithful to the wire records instead, and drop a
`tool` result whose call is nowhere in the history at the projection boundary,
on every request-building projection: the normal wire (`messages`), the
post-400 strict resend (`strictMessages`), and the compaction summarizer. An
orphan is wire-invalid on strict providers and useless to the model either
way, so it never reaches a provider — no longer relying on recognizing the
provider's 400 phrasing to recover. Fragment projections (e.g. token-estimating
a history slice) leave results untouched, since a matching call may
legitimately sit outside the slice.
This commit is contained in:
Kai 2026-07-02 19:29:05 +08:00 committed by GitHub
parent 329846c569
commit 4dd926b0ac
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 298 additions and 43 deletions

View file

@ -0,0 +1,5 @@
---
"@moonshot-ai/kosong": patch
---
Recognize the OpenAI-compatible `role 'tool' must be a response to a preceding message with 'tool_calls'` and `assistant message with 'tool_calls' must be followed by tool messages` 400s (OpenAI / DeepSeek / vLLM / Qwen phrasings) as recoverable tool-exchange structural errors, so the post-400 strict-resend fallback fires and un-bricks the session instead of failing every subsequent turn — including after switching providers or models.

View file

@ -0,0 +1,5 @@
---
"@moonshot-ai/agent-core": patch
---
Drop orphan tool results at the projection boundary so a malformed history cannot brick a session. A `tool` result whose assistant `tool_call` is nowhere in the history (e.g. an older session whose compaction cut fell inside a tool exchange, restored via the legacy path) is now removed from every projected request, not only on the post-400 strict resend. The stored history is left faithful to the wire records — so consumers that model it, like the transcript fold length, stay in sync — while a strict provider (OpenAI / DeepSeek) always receives a valid request. The drop is surfaced via the projection-repair log rather than done silently.

View file

@ -412,8 +412,15 @@ export class FullCompaction {
let overflowShrinkCount = 0;
let emptyOrTruncatedShrinkCount = 0;
while (true) {
// A request-building projection: close still-open calls in the sliced
// prefix (synthesizeMissing) and drop stray results with no call anywhere
// (dropOrphanResults), so the summarizer request cannot be rejected by a
// strict provider even when the history carries a legacy-restore orphan.
const messages = [
...this.agent.context.project(historyForModel, { synthesizeMissing: true }),
...this.agent.context.project(historyForModel, {
synthesizeMissing: true,
dropOrphanResults: true,
}),
createUserMessage(instruction),
];
const estimatedCompactionRequestTokens = this.estimateRequestTokens(messages);

View file

@ -308,14 +308,18 @@ export class ContextMemory {
};
// Wire backward-compat: a pre-rework `context.apply_compaction` record (which
// has no `keptUserMessageCount`) used `[summary, ...history.slice(compactedCount)]`
// semantics and kept a verbatim recent tail. Reproduce that exact shape on
// restore so resuming a session compacted by an older version does not
// silently drop the recent assistant/tool tail beyond `compactedCount`. Gated
// on `records.restoring`, so the live/forward path — which always sets
// `contextSummary` and `keptUserMessageCount` — is unaffected. The projector's
// tool-adjacency repair keeps the restored tail well-formed for strict
// providers; compaction only runs at a clean step boundary, so the tail has no
// open tool exchange to track.
// semantics and kept a verbatim recent tail. Reproduce that shape on restore
// so resuming a session compacted by an older version does not silently drop
// the recent assistant/tool tail beyond `compactedCount`. Gated on
// `records.restoring`, so the live/forward path — which always sets
// `contextSummary` and `keptUserMessageCount` — is unaffected.
//
// The cut can land inside a tool exchange, leaving the tail starting with an
// orphan `tool` result whose assistant is now in the summarized prefix. The
// history is kept faithful to the wire records (so the transcript reducer's
// fold length stays in sync); the projector drops the orphan at the wire
// boundary — see `dropOrphanToolResults` — so a strict provider still gets a
// valid request without mutating the stored history here.
const isLegacyRestore =
this.agent.records.restoring !== null &&
input.keptUserMessageCount === undefined &&
@ -429,14 +433,21 @@ export class ContextMemory {
}
get messages(): Message[] {
return this.project(this.history);
// The normal wire projection. `dropOrphanResults` is on for every
// request-building projection (here, `strictMessages`, and the compaction
// summarizer): a stray result with no matching call anywhere is wire-invalid
// on strict providers and useless to the model, so it never reaches the
// provider — while fragment projections (e.g. token estimation of a history
// slice) leave it alone.
return this.project(this.history, { dropOrphanResults: true });
}
// Last-resort projection for the post-400 strict resend: close every open tool
// call (including a trailing in-flight one) and drop any stray tool result with
// no matching call, so the request is wire-compliant for strict providers no
// matter how the history was mangled. Only used when the provider has already
// rejected the normal projection — see the adjacency fallback in `turn-step`.
// call (including a trailing in-flight one), drop stray tool results, drop a
// leading non-user message, and merge consecutive assistant turns, so the
// request is wire-compliant for strict providers no matter how the history was
// mangled. Only used when the provider has already rejected the normal
// projection — see the adjacency fallback in `turn-step`.
get strictMessages(): Message[] {
return this.project(this.history, {
synthesizeMissing: true,

View file

@ -19,11 +19,13 @@ export interface ProjectOptions {
readonly synthesizeMissing?: boolean;
/**
* When `true`, drop any `tool_result` whose `toolCallId` matches no assistant
* `tool_use` anywhere in the provided messages. Strict providers reject such a
* stray result as an "unexpected `tool_result`". Off by default so the normal
* path never silently discards recorded output; the post-400 strict-resend
* fallback enables it (together with `synthesizeMissing`) as a last resort to
* force a wire-compliant request out of an otherwise-bricked session.
* `tool_use` anywhere in the provided messages. Such an orphan is wire-invalid
* on every strict provider and useless to the model (it has no record of the
* call the result answers). Enabled on every request-building projection the
* normal wire, the strict resend, and the compaction summarizer so a stray
* result never reaches a provider. Left OFF for non-request projections (e.g.
* token-estimating a history slice), where a result's matching call may
* legitimately sit outside the slice and must not be mistaken for an orphan.
*/
readonly dropOrphanResults?: boolean;
/**
@ -69,7 +71,7 @@ export type ProjectionAnomaly =
* was lost (a genuine defect worth investigating).
*/
| { readonly kind: 'tool_result_synthesized'; readonly toolCallId: string; readonly trailing: boolean }
/** A result with no matching call anywhere was dropped (strict resend only). */
/** A result with no matching call anywhere was dropped (wire exits only). */
| { readonly kind: 'orphan_tool_result_dropped'; readonly toolCallId: string }
/** A leading non-user message was dropped so the first turn is user (strict). */
| { readonly kind: 'leading_non_user_dropped'; readonly role: string }
@ -189,8 +191,12 @@ function repairToolExchangeAdjacency(
// Remove any `tool_result` whose `toolCallId` matches no assistant `tool_use`
// anywhere in the projected messages. Strict providers reject such a stray
// result; the post-400 strict-resend fallback drops them as a last resort. Kept
// separate from the adjacency repair so the normal path never discards output.
// result, and it is useless to the model regardless (it has no record of the
// call the result answers), so every request-building projection drops it (via
// `dropOrphanResults`). Kept separate from the adjacency repair, which only
// reorders results that DO have a matching call; this removes the ones that do
// not. Reported via `onAnomaly` so the drop leaves a trace instead of silently
// discarding a recorded result.
function dropOrphanToolResults(
messages: readonly Message[],
onAnomaly?: (anomaly: ProjectionAnomaly) => void,

View file

@ -244,7 +244,7 @@ describe('compaction — Anthropic wire compliance', () => {
{ role: 'tool', content: [{ type: 'text', text: 'done' }], toolCalls: [], toolCallId: 'call_2' },
];
// Normal send path: no synthesizeMissing, no dropOrphanResults.
// Normal send path: no synthesizeMissing.
const projected = ctx.agent.context.project(orphaned);
const wire = await toAnthropicWire(projected, [BASH_TOOL]);
assertValidAnthropic(wire);
@ -260,20 +260,19 @@ describe('compaction — Anthropic wire compliance', () => {
).toBe(true);
});
it('drops a stray tool result with no matching call on the strict resend path', async () => {
it('drops a stray tool result with no matching call from request projections', async () => {
const ctx = testAgent();
ctx.configure({ provider: PROVIDER, modelCapabilities: CAPS });
// A tool_result whose tool_use is gone (e.g. an undo removed the assistant).
// The normal path leaves it (it has no anchor); the strict resend drops it.
// A tool_result whose tool_use is gone (e.g. an undo removed the assistant,
// or a legacy-restore compaction cut mid-exchange). Every request-building
// projection (`messages`, `strictMessages`, the summarizer) enables
// dropOrphanResults — it has no anchor and is useless to the model.
const stray: ContextMessage[] = [
{ role: 'user', content: [{ type: 'text', text: 'hello' }], toolCalls: [], origin: { kind: 'user' } },
{ role: 'tool', content: [{ type: 'text', text: 'orphan output' }], toolCalls: [], toolCallId: 'gone' },
];
const projected = ctx.agent.context.project(stray, {
synthesizeMissing: true,
dropOrphanResults: true,
});
const projected = ctx.agent.context.project(stray, { dropOrphanResults: true });
expect(projected.some((m) => m.role === 'tool')).toBe(false);
const wire = await toAnthropicWire(projected);
assertValidAnthropic(wire);

View file

@ -341,8 +341,9 @@ describe('project tool_use/tool_result adjacency', () => {
tool('orphan-result'),
user('u2'),
];
// Without dropOrphanResults (fragment projections, e.g. token estimation)
// the stray result stays where it was; nothing references it.
const projected = project(history);
// The stray result stays where it was; nothing references it.
expect(projected.map((m) => [m.role, m.toolCallId])).toEqual([
['user', undefined],
['assistant', undefined],
@ -350,6 +351,10 @@ describe('project tool_use/tool_result adjacency', () => {
['tool', 'orphan-result'],
['user', undefined],
]);
// Request-building projections enable dropOrphanResults and remove it.
const wire = project(history, { dropOrphanResults: true });
expect(wire.some((m) => m.toolCallId === 'orphan-result')).toBe(false);
expect(wire.some((m) => m.toolCallId === 'a')).toBe(true);
});
it('does not crash when a tool result appears before its tool_use', () => {
@ -425,15 +430,18 @@ describe('project repair reporting', () => {
]);
});
it('reports a dropped orphan result only on the strict path', () => {
it('reports a dropped orphan result when dropOrphanResults is set', () => {
const history: ContextMessage[] = [user('u1'), assistant(['a']), tool('a'), tool('stray')];
const normal: ProjectionAnomaly[] = [];
project(history, { onAnomaly: (a) => normal.push(a) });
expect(normal).toEqual([]); // normal path leaves the stray result in place
// Fragment projections (no flag) leave the stray in place and report nothing.
const fragment: ProjectionAnomaly[] = [];
project(history, { onAnomaly: (a) => fragment.push(a) });
expect(fragment).toEqual([]);
const strict: ProjectionAnomaly[] = [];
project(history, { dropOrphanResults: true, onAnomaly: (a) => strict.push(a) });
expect(strict).toEqual([{ kind: 'orphan_tool_result_dropped', toolCallId: 'stray' }]);
// Request-building projections (normal wire, strict resend, summarizer)
// enable the flag, drop the stray, and surface the repair.
const wire: ProjectionAnomaly[] = [];
project(history, { dropOrphanResults: true, onAnomaly: (a) => wire.push(a) });
expect(wire).toEqual([{ kind: 'orphan_tool_result_dropped', toolCallId: 'stray' }]);
});
it('reports a whitespace-only text drop but not a truly-empty one', () => {

View file

@ -388,6 +388,95 @@ describe('Agent resume', () => {
]);
});
it('keeps a legacy mid-tool-exchange cut faithful but projects it wire-valid', async () => {
// A pre-rework compaction record (no `keptUserMessageCount`) restores via the
// legacy path, which keeps a verbatim tail `history.slice(compactedCount)`.
// Here the cut (compactedCount=2) lands *between* the assistant `tool_call`
// and its result, so the retained tail starts with a `tool` message whose
// assistant was summarized away — a wire-invalid orphan a strict provider
// (OpenAI / DeepSeek) rejects with "role 'tool' must be a response to a
// preceding message with 'tool_calls'". The restore keeps the history
// faithful (so the transcript reducer's fold length stays in sync); the
// projector drops the orphan at the wire boundary.
const persistence = new RecordingAgentPersistence([
{
type: 'context.append_message',
message: {
role: 'user',
content: [{ type: 'text', text: 'first prompt' }],
toolCalls: [],
origin: { kind: 'user' },
},
},
{
type: 'context.append_loop_event',
event: { type: 'step.begin', uuid: 'orphan-step', turnId: '0', step: 1 },
},
{
type: 'context.append_loop_event',
event: {
type: 'tool.call',
uuid: 'orphan-call',
turnId: '0',
step: 1,
stepUuid: 'orphan-step',
toolCallId: 'call_orphaned',
name: 'Bash',
args: { command: 'pwd' },
},
},
{
type: 'context.append_loop_event',
event: {
type: 'tool.result',
parentUuid: 'orphan-call',
toolCallId: 'call_orphaned',
result: { output: 'ok', isError: false },
},
},
{
type: 'context.append_message',
message: {
role: 'user',
content: [{ type: 'text', text: 'second prompt' }],
toolCalls: [],
origin: { kind: 'user' },
},
},
{
type: 'context.apply_compaction',
summary: 'Compacted the first exchange.',
compactedCount: 2,
tokensBefore: 120,
tokensAfter: 24,
},
]);
const ctx = testAgent({ persistence });
await ctx.agent.resume();
// The stored history stays faithful to the wire records: the orphan `tool`
// result is kept verbatim (not mutated away at restore), so downstream
// consumers that model the history from the records — e.g. the transcript
// reducer's fold length — stay in sync.
expect(ctx.agent.context.history.some((message) => message.role === 'tool')).toBe(true);
// But the projected wire the provider actually sees has no orphan: every
// `tool` result is answered by a preceding assistant `tool_calls`.
const projected = ctx.agent.context.messages;
const toolCallIds = new Set(
projected.flatMap((message) =>
message.role === 'assistant' ? message.toolCalls.map((toolCall) => toolCall.id) : [],
),
);
const orphanToolResults = projected.filter(
(message) =>
message.role === 'tool' &&
(message.toolCallId === undefined || !toolCallIds.has(message.toolCallId)),
);
expect(orphanToolResults).toEqual([]);
});
it('projects restored cancelled compactions into replay records', async () => {
const persistence = new RecordingAgentPersistence([
{

View file

@ -32,6 +32,14 @@ const ADJACENCY_400 = new APIStatusError(
// structural rejection. Verbatim from the field, doubled space included.
const MOONSHOT_TOOL_CALL_ID_400 = new APIStatusError(400, '400 tool_call_id is not found');
// The OpenAI / DeepSeek phrasing of an orphan `tool` result — a `tool` message
// with no preceding assistant `tool_calls`. This is what a DeepSeek / OpenAI-
// compatible provider returns for a history bricked by a stray tool result.
const OPENAI_ROLE_TOOL_400 = new APIStatusError(
400,
"Messages with role 'tool' must be a response to a preceding message with 'tool_calls'",
);
function userMessage(text: string): Message {
return { role: 'user', content: [{ type: 'text', text }], toolCalls: [] };
}
@ -98,6 +106,19 @@ describe('executeLoopStep — tool exchange adjacency fallback', () => {
expect(llm.calls[1]?.messages).toBe(strictMessages);
});
it('resends once and recovers after an OpenAI/DeepSeek role-tool 400', async () => {
const { input, llm, strictCalls, strictMessages } = makeHarness(OPENAI_ROLE_TOOL_400);
const result = await runTurn(input);
expect(result.stopReason).toBe('end_turn');
// Exactly two provider calls: the rejected one and the strict resend.
expect(llm.callCount).toBe(2);
expect(strictCalls.count).toBe(1);
expect(llm.calls[0]?.messages).toEqual([userMessage('normal projection')]);
expect(llm.calls[1]?.messages).toBe(strictMessages);
});
it('does not resend for an unrelated 400 — the error propagates and strict is untouched', async () => {
const { input, llm, strictCalls } = makeHarness(new APIStatusError(400, 'Bad request'));

View file

@ -174,11 +174,14 @@ export function isContextOverflowStatusError(statusCode: number, message: string
// `tool_result`/`tool` blocks are not correctly paired and adjacent — a missing
// result, a stray result with no matching call, or a result that does not
// immediately follow its call. Anthropic phrases this in terms of
// `tool_use`/`tool_result`; OpenAI-compatible providers (Moonshot / Kimi) phrase
// it as a `tool_call_id` that "is not found" in the preceding assistant message.
// The validation runs before any generation, so the error is a non-retryable
// 4xx. A caller can react by resending a re-projected, strictly wire-compliant
// request rather than leaving the session permanently stuck.
// `tool_use`/`tool_result`. OpenAI-compatible providers phrase it in terms of
// `tool_call_id` / `role 'tool'` / `tool_calls`: Moonshot / Kimi as a
// `tool_call_id` that "is not found", and OpenAI / DeepSeek / vLLM / Qwen as a
// `role 'tool'` message without a preceding `tool_calls`, or an assistant
// `tool_calls` not followed by its tool results. The validation runs before any
// generation, so the error is a non-retryable 4xx. A caller can react by
// resending a re-projected, strictly wire-compliant request rather than leaving
// the session permanently stuck.
const TOOL_EXCHANGE_ADJACENCY_MESSAGE_PATTERNS = [
/tool_use[\s\S]*tool_result/,
/tool_result[\s\S]*tool_use/,
@ -189,6 +192,27 @@ const TOOL_EXCHANGE_ADJACENCY_MESSAGE_PATTERNS = [
// (doubled space). Anchored on `tool_call_id` so an unrelated "not found"
// (e.g. a 404-style body) cannot trip the recovery.
/tool_call_id[\s\S]*not found/,
// OpenAI / DeepSeek / vLLM and other OpenAI-compatible providers phrase the
// same structural rejection in terms of `role 'tool'` / `tool_calls` instead
// of Anthropic's `tool_use` / `tool_result`, in two mirror-image shapes:
//
// - An orphan `tool` result whose preceding assistant carries no matching
// `tool_calls`: "messages with role 'tool' must be a response to a
// preceding message with 'tool_calls'".
// - An assistant `tool_calls` with no following `tool` results: "an
// assistant message with 'tool_calls' must be followed by tool messages
// responding to each 'tool_call_id'. the following tool_call_ids did not
// have response messages: ...", or the terse "(insufficient tool messages
// following tool_calls message)".
//
// Both are wire-structure defects the strict resend repairs (drop the orphan
// result / synthesize the missing one). Quote style around `tool`/`tool_calls`
// varies by provider (straight or backtick), so the anchors tolerate an
// optional surrounding quote char.
/role\s+['"`]?tool['"`]?\s+must be a response to a preceding message/,
/assistant message with\s+['"`]?tool_calls['"`]?\s+must be followed by tool messages/,
/tool_call_ids? did not have response messages/,
/insufficient tool messages following/,
] as const;
export function isToolExchangeAdjacencyError(error: unknown): boolean {

View file

@ -267,6 +267,59 @@ describe('isToolExchangeAdjacencyError', () => {
).toBe(true);
});
// OpenAI / DeepSeek / vLLM and other OpenAI-compatible providers phrase the
// orphan-`tool`-result case as a `role 'tool'` message that has no preceding
// assistant `tool_calls`. Observed verbatim in the field (see zed #41531,
// llama_index #13715). Quote style varies by provider (straight or backtick).
it('matches the OpenAI/DeepSeek role-tool-without-tool_calls 400', () => {
expect(
isToolExchangeAdjacencyError(
new APIStatusError(
400,
"Messages with role 'tool' must be a response to a preceding message with 'tool_calls'",
),
),
).toBe(true);
expect(
isToolExchangeAdjacencyError(
new APIStatusError(
400,
'Role `tool` must be a response to a preceding message with `tool_calls`',
),
),
).toBe(true);
});
// The mirror-image OpenAI-compatible rejection: an assistant `tool_calls`
// message with no following `tool` results. OpenAI/Portkey (#6621, error
// 10067) spell it out; Qwen/DashScope (#454) uses double quotes; some
// providers emit the terse "(insufficient tool messages following ...)".
it('matches the assistant-tool_calls-without-response 400', () => {
expect(
isToolExchangeAdjacencyError(
new APIStatusError(
400,
"An assistant message with 'tool_calls' must be followed by tool messages responding to each " +
"'tool_call_id'. The following tool_call_ids did not have response messages: call_hSmZB4G8",
),
),
).toBe(true);
expect(
isToolExchangeAdjacencyError(
new APIStatusError(
400,
'An assistant message with "tool_calls" must be followed by tool messages responding to each ' +
'"tool_call_id". The following tool_call_ids did not have response messages: message[322].role',
),
),
).toBe(true);
expect(
isToolExchangeAdjacencyError(
new APIStatusError(400, '(insufficient tool messages following tool_calls message)'),
),
).toBe(true);
});
it('does not match a context-overflow 400 or unrelated errors', () => {
expect(
isToolExchangeAdjacencyError(new APIContextOverflowError(400, 'context length exceeded')),
@ -275,6 +328,13 @@ describe('isToolExchangeAdjacencyError', () => {
// A bare "not found" without a tool_call_id anchor must not match, so an
// unrelated 404-style body cannot trip the tool-exchange recovery.
expect(isToolExchangeAdjacencyError(new APIStatusError(400, 'resource not found'))).toBe(false);
// A model-availability 400 (observed alongside this family in the field) is a
// config error, not a tool-exchange defect — strict resend must not fire.
expect(
isToolExchangeAdjacencyError(
new APIStatusError(400, '400 Not supported model mimo-v2.5-pro-ultraspeed'),
),
).toBe(false);
expect(isToolExchangeAdjacencyError(new APIStatusError(500, ANTHROPIC_MISSING_RESULT))).toBe(
false,
);
@ -298,6 +358,26 @@ describe('isRecoverableRequestStructureError', () => {
).toBe(true);
});
it('matches the OpenAI-compatible role-tool / assistant-tool_calls pairing 400s', () => {
expect(
isRecoverableRequestStructureError(
new APIStatusError(
400,
"Messages with role 'tool' must be a response to a preceding message with 'tool_calls'",
),
),
).toBe(true);
expect(
isRecoverableRequestStructureError(
new APIStatusError(
400,
"An assistant message with 'tool_calls' must be followed by tool messages responding to each " +
"'tool_call_id'. The following tool_call_ids did not have response messages: call_hSmZB4G8",
),
),
).toBe(true);
});
it('matches empty / whitespace-only text content rejections', () => {
expect(
isRecoverableRequestStructureError(