From 8ac337a2b2ac800aa79a373459308abb6c9e63bb Mon Sep 17 00:00:00 2001 From: Kai Date: Wed, 1 Jul 2026 02:16:19 +0800 Subject: [PATCH] fix(agent-core): harden strict-provider wire compliance so malformed history can't brick a session (#1241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(agent-core): rework compaction to keep only user prompts and summary * refactor(agent-core): rewrite compaction summary as first-person handoff Rework the full-compaction summary to read as the agent's own continuing notes instead of a third-party report: - compaction-instruction.md: free-form first-person continuation that preserves exact commands, paths and outcomes, states the precise next action, and flags claimed-but-unverified work rather than trusting it. - compaction-summary-prefix.md: skeptical "your own working notes" framing; drop the collaborative third-party prefix. - system.md: add compaction-awareness guidance so the model continues naturally from a summary and re-checks any reported "done". - Rename the compaction helpers module to handoff.ts. Update tests and regenerate snapshots for the new prompt text, and fill in contextSummary in the restored-compaction replay expectations. * fix(agent-core): count image/audio/video parts in token estimation estimateTokensForContentPart returned 0 for image_url/audio_url/video_url, so auto-compaction triggers, the overflow-shrink budget, the kept-user budget, and the reported context size all went blind to media — a media-heavy session could overflow the model window while the estimate reported a near-empty context. Media parts now carry a fixed estimate (MEDIA_TOKEN_ESTIMATE), and the content-part switch is exhaustive so a new ContentPart kind must declare its estimate rather than silently count as zero. * feat(agent-core): re-surface active background tasks after compaction Folding the live context to [recent user prompts, summary] drops the messages that started background tasks and their status updates, so the model could forget a task is still running and spawn a duplicate. injectAfterCompaction now appends a system-reminder listing active background tasks (with guidance to use TaskOutput/TaskList/TaskStop instead of re-spawning). It runs only post-compaction and carries an injection origin, so the next compaction drops and rebuilds it rather than stacking copies; the all-user-role post-compaction shape is preserved (no tool-pairing reintroduced). * test(agent-core): add compaction scenario guards and risk probes Adds compaction-scenarios.test.ts driving the real Agent/ContextMemory/ FullCompaction machinery: - A guard test locking in that repeated compaction folds the prior summary into the new one instead of stacking two summaries. - Seven `it.fails` probes that executably reproduce known, currently-accepted edge-case defects so the suite stays green while documenting each one precisely; any of them will flip red (forcing removal of `.fails`) the day the behavior is fixed. They cover: assistant/tool appended during an in-flight summarizer call being dropped; unbounded shrink on empty summaries; the fixed 20k kept-user budget overflowing a small model window; a tool result orphaned when compaction starts mid-exchange; legacy compaction records dropping their verbatim tail on replay; micro-compaction clearing recent tool results in an overflow-shrunk suffix; and media being discarded when the oldest kept user message is truncated. * fix(agent-core): repair tool_use/tool_result adjacency in projected context A tool call and its result can end up non-adjacent in history — a background-task notification or flushed steer lands between them, or an interrupted/nested step delays the result — which strict providers reject with HTTP 400. The projector now moves each tool_use's result up to immediately follow it (projection-time only; the stored history is untouched), and full compaction projects its summarizer input with a synthetic result for any still-open call so the summary request stays well-formed. Micro-compaction only surfaced this latent ordering by busting the prompt cache, so it now defaults off. Includes projector adjacency regression tests, a context-level integration test, and a compaction synthesize-missing guard; the prior "keeps an unresolved tool exchange out of the compaction prompt" test is updated to the now-well-formed (synthetic-result) behavior. * fix(agent-core): preserve the verbatim tail when restoring legacy compactions A pre-rework `context.apply_compaction` record used `[summary, ...history.slice(compactedCount)]` semantics and kept a verbatim recent tail, but it has no `keptUserMessageCount`. The reworked applyCompaction re-folded such records into the all-user shape, dropping the recent assistant/tool tail — so resuming a session compacted by an older version silently lost its most recent context. On restore of such a record (gated on records.restoring, no keptUserMessageCount, and compactedCount < history length) reproduce the old shape instead. The forward/live path is unchanged; the projector's tool-adjacency repair keeps the restored tail well-formed, and compaction only runs at clean step boundaries so the tail has no open exchange. The legacy-tail probe now passes as a regression guard via the real restore path. * fix(agent-core): align legacy compaction foldedLength with live restore The transcript reducer re-derived foldedLength for pre-rework context.apply_compaction records (no keptUserMessageCount) using the new kept-user+summary rule, but ContextMemory's restore now reproduces the legacy [summary, ...history.slice(compactedCount)] shape for those records. The two diverged for legacy sessions, so MessageService's foldedLength-vs-live-history comparison could mis-handle GET /messages (miss or misorder recent output). The reducer now mirrors the live legacy fold: when compactedCount is below the pre-compaction length it computes 1 + (length - compactedCount); otherwise it falls back to the kept-user derivation. The MessageService transcript test's fixture is corrected to a new-format record, matching its all-user live mock. * fix(kosong): merge a follow-up user turn into the preceding tool_results The Anthropic message merge keyed on isToolResultOnly(last) === isToolResultOnly(converted), which left a tool_result-only user turn followed by a plain-text user turn unmerged. After tool-exchange repair this shape (assistant tool_use -> tool_result -> injected notification) produces two adjacent user messages, which strict Anthropic-compatible backends reject with HTTP 400. Switch to the asymmetric predicate isToolResultOnly(last) || !isToolResultOnly(converted): a tool-result-only running message absorbs whatever user turn follows (parallel tool_results or a trailing text), yielding a valid [tool_result, ..., text] message; a plain-text running message still only absorbs plain text. [tool_result, text] is valid for both native Anthropic (which concatenates anyway) and strict backends. * test(agent-core): pin micro-compaction flag in the shrunk-suffix probe The 'does not clear recent tool results when projecting a shrunk suffix' probe is an it.fails that only documents a real defect while micro-compaction is active. It inherited the ambient KIMI_CODE_EXPERIMENTAL master switch, so its pass/fail flipped with the runner: green locally (master switch on) but a hard failure in CI, where the flag defaults off and MicroCompaction.compact() is a no-op that leaves the tool result intact. Enable KIMI_CODE_EXPERIMENTAL_MICRO_COMPACTION explicitly for this probe so it deterministically exercises the micro-compaction path regardless of the environment. * fix(agent-core): harden full compaction against in-flight races, unbounded shrink, and media loss Three compaction-path fixes surfaced by review, each flipping its documenting it.fails probe to a passing it: - Append race (CMP-02): after the summarizer returns, the post-summary history check only compared the compacted prefix. A live step appending to the tail while a manual/SDK compaction was in flight slipped through — an appended assistant/tool turn is neither summarized (the summary covers only the snapshot) nor kept (the rebuild keeps user input), so it vanished. Now cancel when the appended tail contains a non-user message; an appended user message is still kept (rebuild picks it up), preserving the existing 'keeps messages appended while compacting an unchanged prefix' behavior. - Unbounded empty/truncated shrink: an empty or truncated summary dropped the oldest message and reset retryCount, so a model that kept returning empty could issue ~one request per history entry. Bound the shrink attempts by MAX_COMPACTION_RETRY_ATTEMPTS, mirroring the overflow-shrink counter. - Media dropped on truncation (CMP-07): truncating the oldest kept user message replaced its whole content with one text block, discarding any image/audio/video. Keep the non-text parts and spend the remaining budget (maxTokens minus their cost) on truncated text. * fix(vis): mirror legacy compaction tail in the model-mode projector For a pre-rework context.apply_compaction record (no keptUserMessageCount), agent-core's ContextMemory restore and the transcript reducer keep the old [summary, ...history.slice(compactedCount)] tail — a verbatim recent tail including assistant/tool. The vis model-mode projector always applied the new kept-user selection, so opening an older compacted session in model mode hid the assistant/tool tail the resumed agent still holds (and surfaced a pre-compaction user message the agent dropped). Branch on a missing keptUserMessageCount with compactedCount < history length and reproduce the legacy shape, matching the agent-core restore. * fix(agent-core): cancel compaction on any droppable user-role tail The in-flight append guard cancelled only when the tail grew with a non-user role. A user-role message that compaction would still drop — a background-task notification, hook/cron reminder, or shell-command output — slipped through: appended after the summary snapshot (so absent from the summary) and dropped by the all-user rebuild (which keeps only real user input), vanishing silently. Key the guard on the same predicate applyCompaction uses (!isRealUserInput) so it cancels whenever the appended tail holds anything compaction would drop. A real user message is still kept, so a live user turn racing a manual/SDK compaction continues to complete. * fix(agent-core): exclude pre-clear prompts from legacy folded length The transcript reducer's legacy fallback (records predating keptUserMessageCount, compacted with no verbatim tail) re-derived the kept-user count from the whole transcript, including messages before the last context.clear. Live ContextMemory rebuilds _history from post-clear messages only, so counting pre-clear prompts overstated foldedLength; MessageService then saw context.history.length <= foldedLength and skipped appending unflushed live tail messages, dropping recent output from the messages endpoint for old sessions compacted after a clear. Derive only from entries at or after clearFloor to match the live context. * fix(agent-core): drop media when truncating the oldest kept prompt Revert the media-preserving truncation: keeping non-text parts on the truncated boundary message overshot the kept-user budget when the media alone exceeded it, and reordered interleaved text/media parts. Both codex (no media-aware truncation) and Claude Code (strips media at compaction) decline to preserve media on a truncated message, since media cannot be partially truncated and keeping it whole breaks the budget. truncateUserMessage now keeps only the truncated text. Recent messages that fit the budget are still kept verbatim with their media; only the oldest, partially-overflowing boundary message loses its attachments. * fix(agent-core): make manual compaction and turns mutually exclusive A manual/SDK compaction could start while a turn was streaming, or a new turn could launch while a compaction was in flight. Either way the turn mutates the shared context (streaming content into an existing assistant message, or appending new messages) during the summarizer await, and that output is neither summarized nor preserved by the all-user rebuild — silent loss that object-identity checks can't detect (the streamed message is mutated in place). Guard both directions so the agent does one of {turn, compaction} at a time: begin() refuses a manual compaction while a turn is active, and launch() refuses a new turn while a compaction is in progress. Auto compaction is exempt — it runs from within the turn at a step boundary, which blocks the turn for its duration. * chore(changeset): consolidate compaction changesets into one * chore(agent-core): drop external-product references from compaction comments * test(agent-core): add Anthropic wire-compliance smoke tests for compaction Drive real compaction output and the compaction summarizer projection through the real Anthropic provider conversion and assert the wire request is well-formed: strict user/assistant alternation and every tool_use answered by an adjacent tool_result. Locks in the cross-layer guarantee (projector merge + Anthropic consecutive-user merge + adjacency repair + synthesizeMissing) that compacted sessions stay valid for strict Anthropic-compatible backends. * fix(agent-core): defer and replay inputs during manual compaction instead of rejecting Manual/SDK compaction runs outside a turn, so the earlier guard rejected prompts/steers that arrived while it held the context. That broke three things: a REST/web prompt got stuck 'running' (no terminal turn event), a background-task/cron steer was silently lost (null was read as 'buffered' but nothing was), and a follow-up prompt could land in the window after isCompacting cleared but before reminders were reinjected. Reuse the existing defer-and-replay model instead of rejecting: - steer() and launch() buffer into steerBuffer while a compaction is in progress (returning null = buffered), mirroring how an active turn defers input. - FullCompaction.compactionWorker keeps isCompacting true through refreshSystemPrompt + injectAfterCompaction (moving markCompleted and the completed event after reinjection), then replays the buffer via TurnFlow.onCompactionFinished — on success, on an A1 prefix/tail cancel, and on failure/abort. - onCompactionFinished flushes into an active turn if one exists, else launches a fresh turn from the deferred input. No PromptService change: a deferred prompt's eventual turn.started lets it associate the pending prompt and clear it on turn.ended. * feat(kosong): detect tool_use/tool_result adjacency errors Add isToolExchangeAdjacencyError to classify the strict-provider 400 raised when an assistant tool_use is not correctly paired with its tool_result (missing, stray, or non-adjacent), excluding context-overflow 400s. Lets the agent loop recognize the error and resend a wire-compliant request instead of leaving the session stuck. * fix(agent-core): close mid-history orphan tool calls and resend wire-compliant after a strict 400 Strict providers (Anthropic) reject a request whose assistant tool_use is not answered by an adjacent tool_result, and the same malformed history is re-sent every turn, permanently bricking the session. - Projector now closes a mid-history tool call whose result is missing entirely (a later turn proves it is not in-flight) with a synthetic result; the trailing in-flight call is still left untouched. - Add a strict projection (synthesize every open call, drop stray results) and, on a tool_use/tool_result adjacency 400, resend the request once with it. - Report every projection repair (reorder / synthesize / drop) via log and telemetry, deduped by signature, so a silently-mangled history leaves a trace. Trailing-tail synthesis (expected under compaction) is not flagged. * fix(kosong): merge consecutive user turns for strict providers Gemini/Vertex require strictly alternating user/model turns and reject consecutive user turns with HTTP 400. They arise after compaction (kept prompts + user-role summary + injected reminders) and when a turn is steered in right after a tool result. Anthropic already merged them inline; the Google converter did not, so post-compaction requests failed. Extract the asymmetric merge into a shared mergeConsecutiveUserMessages helper applied at each strict provider's conversion boundary: refactor Anthropic to use it (behavior unchanged) and apply it at the Google converter's exit. A conformance suite drives every strict provider with the post-compaction shape and a steer-after-tool-result shape, asserting no consecutive same-role turns reach the wire, so a new strict provider cannot silently omit the merge. The provider-agnostic projector stays structure-preserving: lenient providers (OpenAI/Kimi) keep distinct turns for clearer message boundaries; only strict providers normalize, where the requirement lives. * feat(kosong): recognize the broader structural request-rejection family Add isRecoverableRequestStructureError, covering the strict-provider 400s that stem from a malformed message array re-sent every turn: tool_use/tool_result pairing, empty/whitespace-only text blocks, a non-user first message, and non-alternating roles. Context-overflow 400s are excluded (handled by compaction). Lets the loop trigger one strict, wire-compliant resend for the whole family rather than only tool-pairing errors. * fix(agent-core): sanitize whitespace and strict-resend structural 400s, with diagnostics - Drop empty AND whitespace-only text blocks in projection (Anthropic rejects whitespace-only with "text content blocks must contain non-whitespace text", which otherwise sticks a session); treat whitespace-only tool output as empty. - Broaden the post-400 strict resend to the whole structural family and add two strict-only passes to the strict projection: drop leading non-user messages (first message must be user) and merge consecutive assistant turns. - Log + telemetry for every wire repair the projector applies (reorder, synthesize, drop orphan, drop leading, merge assistants, drop whitespace), deduped by signature; log the strict resend outcome (recovered or still rejected) so a stuck session always leaves a trace. * fix(agent-core): normalize empty-equivalent tool result arrays to the empty placeholder A tool result whose ContentPart[] output has no sendable content (an empty array, or only empty/whitespace-only text blocks) was returned verbatim, so projection stripped the blank blocks, left the tool message empty, and threw on every send — bricking the session locally. String outputs were already normalized; do the same for arrays. A non-text part or any non-whitespace text still keeps the real output. * chore(changeset): simplify the wire-compliance changeset --- .../harden-strict-provider-wire-compliance.md | 5 + .../agent-core/src/agent/context/index.ts | 107 +++++++- .../agent-core/src/agent/context/projector.ts | 239 +++++++++++++++--- packages/agent-core/src/agent/turn/index.ts | 1 + packages/agent-core/src/loop/run-turn.ts | 8 + packages/agent-core/src/loop/turn-step.ts | 51 +++- .../compaction/anthropic-compliance.test.ts | 58 +++++ .../agent-core/test/agent/context.test.ts | 65 ++++- .../test/agent/context/projector.test.ts | 230 ++++++++++++++++- .../loop/tool-exchange-fallback.e2e.test.ts | 124 +++++++++ packages/kosong/src/errors.ts | 45 ++++ packages/kosong/src/index.ts | 2 + packages/kosong/test/errors.test.ts | 98 +++++++ 13 files changed, 987 insertions(+), 46 deletions(-) create mode 100644 .changeset/harden-strict-provider-wire-compliance.md create mode 100644 packages/agent-core/test/loop/tool-exchange-fallback.e2e.test.ts diff --git a/.changeset/harden-strict-provider-wire-compliance.md b/.changeset/harden-strict-provider-wire-compliance.md new file mode 100644 index 000000000..b98307895 --- /dev/null +++ b/.changeset/harden-strict-provider-wire-compliance.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Stop a malformed message history from permanently bricking a session on strict providers (Anthropic). The request is repaired before sending — orphaned tool calls are closed and empty/whitespace-only text blocks dropped — and if the provider still rejects its structure, it is resent once with a wire-compliant rebuild. diff --git a/packages/agent-core/src/agent/context/index.ts b/packages/agent-core/src/agent/context/index.ts index 15c2c64d3..b9192ded0 100644 --- a/packages/agent-core/src/agent/context/index.ts +++ b/packages/agent-core/src/agent/context/index.ts @@ -13,7 +13,12 @@ import { type CompactionInput, type CompactionResult, } from '../compaction'; -import { project, type ProjectOptions, trimTrailingOpenToolExchange } from './projector'; +import { + project, + type ProjectionAnomaly, + type ProjectOptions, + trimTrailingOpenToolExchange, +} from './projector'; import { USER_PROMPT_ORIGIN, type AgentContextData, @@ -43,6 +48,9 @@ export class ContextMemory { private pendingToolResultIds = new Set(); private deferredMessages: ContextMessage[] = []; private _lastAssistantAt: number | null = null; + // Signature of the last logged set of projection repairs, so a repair that + // recurs identically on every send is logged once rather than per step. + private lastProjectionRepairSignature: string | null = null; constructor(protected readonly agent: Agent) {} @@ -314,13 +322,95 @@ export class ContextMemory { } project(messages: readonly ContextMessage[], options?: ProjectOptions): Message[] { - return project(this.agent.microCompaction.compact(messages), options); + const anomalies: ProjectionAnomaly[] = []; + const result = project(this.agent.microCompaction.compact(messages), { + ...options, + onAnomaly: (anomaly) => { + anomalies.push(anomaly); + options?.onAnomaly?.(anomaly); + }, + }); + this.reportProjectionRepairs(anomalies); + return result; + } + + // Surface the projector's wire-repairs so a silently-mangled history leaves a + // trace instead of being papered over. Deduped by signature: a repair that + // recurs identically every send (e.g. a persistently lost result re-synthesized + // each turn) logs once, not per step. Trailing-tail synthesis is excluded — it + // is the expected close of an in-flight call under `synthesizeMissing` + // (compaction / strict resend), not a defect. + private reportProjectionRepairs(anomalies: readonly ProjectionAnomaly[]): void { + const notable = anomalies.filter( + (anomaly) => !(anomaly.kind === 'tool_result_synthesized' && anomaly.trailing), + ); + if (notable.length === 0) { + this.lastProjectionRepairSignature = null; + return; + } + const signature = notable + .map((anomaly) => ('toolCallId' in anomaly ? `${anomaly.kind}:${anomaly.toolCallId}` : anomaly.kind)) + .toSorted() + .join('|'); + if (signature === this.lastProjectionRepairSignature) return; + this.lastProjectionRepairSignature = signature; + + let reordered = 0; + let synthesized = 0; + let droppedOrphan = 0; + let leadingDropped = 0; + let assistantsMerged = 0; + let whitespaceDropped = 0; + for (const anomaly of notable) { + if (anomaly.kind === 'tool_result_reordered') reordered += 1; + else if (anomaly.kind === 'tool_result_synthesized') synthesized += 1; + else if (anomaly.kind === 'orphan_tool_result_dropped') droppedOrphan += 1; + else if (anomaly.kind === 'leading_non_user_dropped') leadingDropped += 1; + else if (anomaly.kind === 'consecutive_assistants_merged') assistantsMerged += 1; + else whitespaceDropped += 1; + } + const toolCallIds = [ + ...new Set( + notable.flatMap((anomaly) => ('toolCallId' in anomaly ? [anomaly.toolCallId] : [])), + ), + ].slice(0, 5); + this.agent.log.warn('repaired the request to keep it wire-valid', { + reordered, + synthesized, + droppedOrphan, + leadingDropped, + assistantsMerged, + whitespaceDropped, + toolCallIds, + }); + this.agent.telemetry.track('context_projection_repaired', { + reordered, + synthesized, + dropped_orphan: droppedOrphan, + leading_dropped: leadingDropped, + assistants_merged: assistantsMerged, + whitespace_dropped: whitespaceDropped, + }); } get messages(): Message[] { return this.project(this.history); } + // 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`. + get strictMessages(): Message[] { + return this.project(this.history, { + synthesizeMissing: true, + dropOrphanResults: true, + dropLeadingNonUser: true, + mergeConsecutiveAssistants: true, + }); + } + useProjectedHistoryFrom(source: ContextMemory): void { this.clear(); this.pushHistory(...trimTrailingOpenToolExchange(source.project(source.history))); @@ -500,7 +590,12 @@ function toolResultOutputForModel(result: ExecutableToolResult): string | Conten return isEmptyOutputText(output) ? TOOL_EMPTY_STATUS : output; } - if (output.length === 0) { + // Treat an array output with no sendable content (empty, or only empty/ + // whitespace-only text blocks) the same as an empty string output: emit the + // placeholder. Otherwise projection would strip the blank text blocks, leave + // the tool message empty, and throw on every send — bricking the session. A + // non-text part (image/etc.) or any non-whitespace text keeps the real output. + if (isEmptyEquivalentContentArray(output)) { return [ { type: 'text', @@ -514,8 +609,12 @@ function toolResultOutputForModel(result: ExecutableToolResult): string | Conten return output; } +function isEmptyEquivalentContentArray(output: readonly ContentPart[]): boolean { + return output.every((part) => part.type === 'text' && part.text.trim().length === 0); +} + function isEmptyOutputText(output: string): boolean { - return output.length === 0 || output.trim() === TOOL_OUTPUT_EMPTY_TEXT; + return output.trim().length === 0 || output.trim() === TOOL_OUTPUT_EMPTY_TEXT; } function formatUndoUnavailableMessage( diff --git a/packages/agent-core/src/agent/context/projector.ts b/packages/agent-core/src/agent/context/projector.ts index c10de2f9a..9471b2ead 100644 --- a/packages/agent-core/src/agent/context/projector.ts +++ b/packages/agent-core/src/agent/context/projector.ts @@ -5,19 +5,94 @@ import type { ContextMessage } from './types'; export interface ProjectOptions { /** - * When `true`, emit a synthetic `tool_result` for any assistant `tool_use` - * whose result is not present in the provided messages. Used by full - * compaction, where the compacted prefix is a slice that may exclude a - * delayed result preserved in the retained tail; the synthetic result keeps - * the exchange closed so the summary request is not rejected. Leave `false` - * for normal turns, where a missing result means the call is still in-flight - * and must not be closed prematurely. + * When `true`, emit a synthetic `tool_result` for *every* assistant `tool_use` + * whose result is not present in the provided messages — including a trailing, + * still-in-flight call. Used by full compaction, where the compacted prefix is + * a slice that may exclude a delayed result preserved in the retained tail; the + * synthetic result keeps the exchange closed so the summary request is not + * rejected. Leave `false` for normal turns: a *trailing* missing result there + * means the call is still in-flight and must not be closed prematurely. (A + * *non-trailing* missing result is always closed regardless of this flag — see + * `repairToolExchangeAdjacency` — because a later turn proves it is not + * in-flight.) */ 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. + */ + readonly dropOrphanResults?: boolean; + /** + * When `true`, drop leading messages until the first one is a user turn. Strict + * providers require the first message to be `user`; a history that (after + * dropping/compaction) starts with an assistant or tool message is rejected. + * Strict-resend only — the normal path keeps the original opening. + */ + readonly dropLeadingNonUser?: boolean; + /** + * When `true`, merge back-to-back assistant messages into one. Strict providers + * reject consecutive same-role turns ("roles must alternate"); consecutive user + * turns are already merged at the provider boundary, but consecutive assistant + * turns are not. Strict-resend only. Content is concatenated verbatim — callers + * must not rely on this when extended-thinking ordering matters, but two + * consecutive assistant turns do not arise in well-formed transcripts. + */ + readonly mergeConsecutiveAssistants?: boolean; + /** + * Optional sink invoked for every repair the projector applies to keep the + * outgoing wire valid: a displaced result moved back next to its call, a + * synthetic result invented for a missing one, a stray result dropped, a + * leading non-user message dropped, or consecutive assistants merged. The + * projection itself stays a pure transform; the caller decides whether/how to + * surface these (the context logs them so a silently-mangled history is never + * papered over without a trace). Not called when the history is already + * well-formed. + */ + readonly onAnomaly?: (anomaly: ProjectionAnomaly) => void; } +/** + * A repair the projector applied to make the history wire-valid. Each one means + * the stored history was not directly sendable to a strict provider. + */ +export type ProjectionAnomaly = + /** A recorded result was not adjacent to its call and had to be moved up. */ + | { readonly kind: 'tool_result_reordered'; readonly toolCallId: string } + /** + * No result existed for a call, so a placeholder was synthesized. `trailing` + * is true when it closed a still-open tail call (expected under + * `synthesizeMissing`), false when it closed a mid-history orphan whose result + * 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). */ + | { 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 } + /** Two adjacent assistant turns were merged into one (strict). */ + | { readonly kind: 'consecutive_assistants_merged' } + /** A non-empty but all-whitespace text block was dropped (always). */ + | { readonly kind: 'whitespace_text_dropped'; readonly role: string }; + export function project(history: readonly ContextMessage[], options?: ProjectOptions): Message[] { - return repairToolExchangeAdjacency(mergeAdjacentUserMessages(history), options); + let result = repairToolExchangeAdjacency( + mergeAdjacentUserMessages(history, options?.onAnomaly), + options, + ); + if (options?.mergeConsecutiveAssistants === true) { + result = mergeConsecutiveAssistantMessages(result, options.onAnomaly); + } + if (options?.dropOrphanResults === true) { + result = dropOrphanToolResults(result, options.onAnomaly); + } + if (options?.dropLeadingNonUser === true) { + result = dropLeadingNonUserMessages(result, options.onAnomaly); + } + return result; } // Strict providers (Anthropic) require every assistant `tool_use` to be answered @@ -32,16 +107,21 @@ export function project(history: readonly ContextMessage[], options?: ProjectOpt // Repair the adjacency so every assistant `tool_use` is immediately followed by // its matching `tool_result` message(s). Matching results are moved up from // wherever they appear later in the history; any intervening messages keep their -// relative order and simply follow the repaired exchange. A tool call with no -// recorded result anywhere later in the history is left untouched by default — -// it is still in-flight (pending) rather than orphaned, and the -// trailing-open-exchange trim plus the interrupted-result synthesis during replay -// own those cases. With `synthesizeMissing`, a synthetic `tool_result` is emitted -// for such calls instead; full compaction uses this to keep a sliced prefix -// closed when a delayed result lives in the retained tail. This is purely a -// projection-time fix: the underlying history is left untouched, so replay and -// transcripts keep their original order, while the model always sees a -// well-formed tool exchange. +// relative order and simply follow the repaired exchange. +// +// A tool call with no recorded result anywhere is closed with a synthetic +// `tool_result` UNLESS it belongs to the trailing exchange (no later +// user/assistant message follows it). A non-trailing missing result can never be +// in-flight — a subsequent turn proves the model already moved on — so leaving it +// open would strand the whole session behind a 400 on every send; it is closed +// here instead. The trailing exchange is left untouched by default: there a +// missing result genuinely means the call is still pending, and the +// trailing-open-exchange trim plus replay's interrupted-result synthesis own that +// case. With `synthesizeMissing`, even the trailing call is closed; full +// compaction uses this to keep a sliced prefix closed when a delayed result lives +// in the retained tail. This is purely a projection-time fix: the underlying +// history is left untouched, so replay and transcripts keep their original order, +// while the model always sees a well-formed tool exchange. const SYNTHETIC_TOOL_RESULT_TEXT = 'Tool result is not available in the current context. Do not assume the tool completed successfully.'; @@ -49,6 +129,16 @@ function repairToolExchangeAdjacency( messages: readonly Message[], options?: ProjectOptions, ): Message[] { + // The trailing exchange is the only one whose missing result may still be + // in-flight: any assistant `tool_use` that precedes a later user/assistant + // message has been overtaken by a new turn and cannot be pending. Find the last + // non-tool message so an orphan can be classified as trailing (index >= it) or + // mid-history (index < it). + let lastNonToolIndex = messages.length - 1; + while (lastNonToolIndex >= 0 && messages[lastNonToolIndex]?.role === 'tool') { + lastNonToolIndex -= 1; + } + const out: Message[] = []; const consumed = new Set(); for (let i = 0; i < messages.length; i++) { @@ -61,6 +151,10 @@ function repairToolExchangeAdjacency( out.push(message); const pending = new Set(message.toolCalls.map((toolCall) => toolCall.id)); + // Tracks whether a foreign message (anything that is not one of this + // exchange's own results) sits between the call and a later matching result; + // if so, that result was displaced and pulling it up is a real repair. + let foreignBetween = false; for (let j = i + 1; j < messages.length && pending.size > 0; j++) { if (consumed.has(j)) continue; const next = messages[j]!; @@ -69,23 +163,94 @@ function repairToolExchangeAdjacency( out.push(next); consumed.add(j); pending.delete(toolCallId); + if (foreignBetween) options?.onAnomaly?.({ kind: 'tool_result_reordered', toolCallId }); + } else { + foreignBetween = true; } } - if (options?.synthesizeMissing === true) { - // Close any tool call whose result is absent from the provided messages. - // Only used by full compaction, where the prefix is a slice that may - // exclude a delayed result preserved in the retained tail. For normal - // turns a missing result means the call is still in-flight, so it is left - // for the trailing-open-exchange trim and replay's interrupted-result - // synthesis instead of being closed here. + // Close any tool call whose result is absent. A mid-history orphan (a later + // user/assistant message follows) is always closed — it cannot be in-flight. + // The trailing exchange is closed only when `synthesizeMissing` is set, so a + // genuinely pending call is left for the trim / replay synthesis otherwise. + const isMidHistory = i < lastNonToolIndex; + if (options?.synthesizeMissing === true || isMidHistory) { for (const missingId of pending) { out.push(makeSyntheticToolResult(missingId)); + options?.onAnomaly?.({ + kind: 'tool_result_synthesized', + toolCallId: missingId, + trailing: !isMidHistory, + }); } } } return out; } +// 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. +function dropOrphanToolResults( + messages: readonly Message[], + onAnomaly?: (anomaly: ProjectionAnomaly) => void, +): Message[] { + const toolUseIds = new Set(); + for (const message of messages) { + if (message.role === 'assistant') { + for (const toolCall of message.toolCalls) toolUseIds.add(toolCall.id); + } + } + return messages.filter((message) => { + if (message.role !== 'tool' || message.toolCallId === undefined) return true; + if (toolUseIds.has(message.toolCallId)) return true; + onAnomaly?.({ kind: 'orphan_tool_result_dropped', toolCallId: message.toolCallId }); + return false; + }); +} + +// Merge back-to-back assistant messages into one. Strict providers reject +// consecutive same-role turns; the provider boundary already merges consecutive +// user turns, but not assistant turns. Strict-resend only. Content is +// concatenated verbatim (no reordering), so this is safe for the well-formed +// transcripts where it never fires, and a best-effort last resort otherwise. +function mergeConsecutiveAssistantMessages( + messages: readonly Message[], + onAnomaly?: (anomaly: ProjectionAnomaly) => void, +): Message[] { + const out: Message[] = []; + for (const message of messages) { + const previous = out.at(-1); + if (previous !== undefined && previous.role === 'assistant' && message.role === 'assistant') { + out[out.length - 1] = { + ...previous, + content: [...previous.content, ...message.content], + toolCalls: [...previous.toolCalls, ...message.toolCalls], + }; + onAnomaly?.({ kind: 'consecutive_assistants_merged' }); + continue; + } + out.push(message); + } + return out; +} + +// Drop leading messages until the first one is a user turn. Strict providers +// require the first message to be `user`; a history that starts with an +// assistant or tool message (after dropping/compaction edge cases) is rejected. +// Strict-resend only. +function dropLeadingNonUserMessages( + messages: readonly Message[], + onAnomaly?: (anomaly: ProjectionAnomaly) => void, +): Message[] { + let start = 0; + while (start < messages.length && messages[start]!.role !== 'user') { + onAnomaly?.({ kind: 'leading_non_user_dropped', role: messages[start]!.role }); + start += 1; + } + return start === 0 ? [...messages] : messages.slice(start); +} + function makeSyntheticToolResult(toolCallId: string): Message { return { role: 'tool', @@ -95,10 +260,13 @@ function makeSyntheticToolResult(toolCallId: string): Message { }; } -function mergeAdjacentUserMessages(history: readonly ContextMessage[]): Message[] { +function mergeAdjacentUserMessages( + history: readonly ContextMessage[], + onAnomaly?: (anomaly: ProjectionAnomaly) => void, +): Message[] { const out: ContextMessage[] = []; for (const source of history) { - const message = prepareMessageForProjection(source); + const message = prepareMessageForProjection(source, onAnomaly); if (message === null) continue; const previous = out.at(-1); @@ -115,13 +283,26 @@ function mergeAdjacentUserMessages(history: readonly ContextMessage[]): Message[ return out.map(stripContextMetadata); } -function prepareMessageForProjection(message: ContextMessage): ContextMessage | null { +function prepareMessageForProjection( + message: ContextMessage, + onAnomaly?: (anomaly: ProjectionAnomaly) => void, +): ContextMessage | null { if (message.partial === true) return null; let content: ContentPart[] | undefined; for (const [index, part] of message.content.entries()) { - if (part.type === 'text' && part.text.length === 0) { + // Strict providers reject a text block that is empty OR whitespace-only + // ("text content blocks must contain non-whitespace text"). Drop both; a + // block with surrounding whitespace but real content is kept verbatim. + if (part.type === 'text' && part.text.trim().length === 0) { content ??= message.content.slice(0, index); + // Report only whitespace-only (non-empty) blocks: a truly empty `''` block + // is routine cleanup (e.g. a trailing empty text part after a tool call), + // whereas a block that is non-empty yet all-whitespace signals something + // upstream fed blank content and is worth surfacing for debugging. + if (part.text.length > 0) { + onAnomaly?.({ kind: 'whitespace_text_dropped', role: message.role }); + } continue; } content?.push(part); diff --git a/packages/agent-core/src/agent/turn/index.ts b/packages/agent-core/src/agent/turn/index.ts index 08f2178d0..847794aaa 100644 --- a/packages/agent-core/src/agent/turn/index.ts +++ b/packages/agent-core/src/agent/turn/index.ts @@ -682,6 +682,7 @@ export class TurnFlow { signal, llm: this.agent.llm, buildMessages: () => this.agent.context.messages, + buildMessagesStrict: () => this.agent.context.strictMessages, dispatchEvent: this.buildDispatchEvent(turnId), tools: this.agent.tools.loopTools, log: this.agent.log, diff --git a/packages/agent-core/src/loop/run-turn.ts b/packages/agent-core/src/loop/run-turn.ts index 326dba854..3ee74cbcd 100644 --- a/packages/agent-core/src/loop/run-turn.ts +++ b/packages/agent-core/src/loop/run-turn.ts @@ -34,6 +34,12 @@ export interface RunTurnInput { readonly signal: AbortSignal; readonly llm: LLM; readonly buildMessages: LoopMessageBuilder; + /** + * Optional strict, guaranteed wire-compliant rebuild of the request messages. + * Used only to resend once after a provider rejects the normal projection with + * a tool_use/tool_result adjacency 400 (see `executeLoopStep`). + */ + readonly buildMessagesStrict?: LoopMessageBuilder | undefined; readonly dispatchEvent: LoopEventDispatcher; readonly tools?: readonly ExecutableTool[] | undefined; readonly hooks?: LoopHooks | undefined; @@ -51,6 +57,7 @@ export async function runTurn(input: RunTurnInput): Promise { signal, llm, buildMessages, + buildMessagesStrict, dispatchEvent, tools, hooks, @@ -85,6 +92,7 @@ export async function runTurn(input: RunTurnInput): Promise { turnId, signal, buildMessages, + buildMessagesStrict, dispatchEvent, llm, tools, diff --git a/packages/agent-core/src/loop/turn-step.ts b/packages/agent-core/src/loop/turn-step.ts index 2a0fc6132..8d72cad5d 100644 --- a/packages/agent-core/src/loop/turn-step.ts +++ b/packages/agent-core/src/loop/turn-step.ts @@ -9,10 +9,11 @@ import { randomUUID } from 'node:crypto'; -import type { TokenUsage } from '@moonshot-ai/kosong'; +import { isRecoverableRequestStructureError, type TokenUsage } from '@moonshot-ai/kosong'; import type { Logger } from '#/logging/types'; import type { LoopEventDispatcher } from './events'; +import { errorMessage } from './errors'; import type { LLM, LLMChatParams, LLMChatResponse } from './llm'; import { chatWithRetry } from './retry'; import { runToolCallBatch, type ToolCallStepContext } from './tool-call'; @@ -33,6 +34,7 @@ export interface ExecuteLoopStepDeps { readonly turnId: string; readonly signal: AbortSignal; readonly buildMessages: LoopMessageBuilder; + readonly buildMessagesStrict?: LoopMessageBuilder | undefined; readonly dispatchEvent: LoopEventDispatcher; readonly llm: LLM; readonly tools?: readonly ExecutableTool[] | undefined; @@ -51,6 +53,7 @@ export async function executeLoopStep(deps: ExecuteLoopStepDeps): Promise<{ turnId, signal, buildMessages, + buildMessagesStrict, dispatchEvent, llm, tools, @@ -110,16 +113,56 @@ export async function executeLoopStep(deps: ExecuteLoopStepDeps): Promise<{ stepUuid, }), }; - const response: LLMChatResponse = await chatWithRetry({ + const retryInput = { llm, - params: chatParams, dispatchEvent, turnId, currentStep, stepUuid, maxAttempts: maxRetryAttempts, log, - }); + } as const; + let response: LLMChatResponse; + try { + response = await chatWithRetry({ ...retryInput, params: chatParams }); + } catch (error) { + // A structural request rejection (tool_use/tool_result pairing, empty or + // whitespace-only text, non-user first message, non-alternating roles) means + // the projected history is not wire-compliant for a strict provider — and + // since the same history is re-sent every turn, the session would stay stuck + // on this error forever. Resend ONCE with a strict, guaranteed-compliant + // rebuild (every open call closed, stray results dropped, leading non-user + // trimmed, consecutive assistants merged) as a last resort. Any other error, + // or a host that supplied no strict builder, propagates unchanged. + if (buildMessagesStrict === undefined || !isRecoverableRequestStructureError(error)) throw error; + signal.throwIfAborted(); + log?.warn('provider rejected request structure; resending with strict projection', { + turnStep: `${turnId}.${String(currentStep)}`, + model: llm.modelName, + }); + const strictMessages = await buildMessagesStrict(); + signal.throwIfAborted(); + try { + response = await chatWithRetry({ + ...retryInput, + params: { ...chatParams, messages: strictMessages }, + }); + } catch (strictError) { + // The strictly-sanitized rebuild was still rejected — our wire-compliance + // repair did not cover this case. Surface it loudly: the session is stuck + // and this is the signal we need to diagnose the gap. + log?.error('strict resend still rejected by provider; request remains wire-invalid', { + turnStep: `${turnId}.${String(currentStep)}`, + model: llm.modelName, + originalError: errorMessage(error), + strictError: errorMessage(strictError), + }); + throw strictError; + } + log?.info('recovered after strict resend', { + turnStep: `${turnId}.${String(currentStep)}`, + }); + } const usage = response.usage; const usageResult = await recordUsage(usage); const stopTurnAfterUsage = usageResult?.stopTurn === true; diff --git a/packages/agent-core/test/agent/compaction/anthropic-compliance.test.ts b/packages/agent-core/test/agent/compaction/anthropic-compliance.test.ts index 521f0cbd3..fa2315858 100644 --- a/packages/agent-core/test/agent/compaction/anthropic-compliance.test.ts +++ b/packages/agent-core/test/agent/compaction/anthropic-compliance.test.ts @@ -221,6 +221,64 @@ describe('compaction — Anthropic wire compliance', () => { assertValidAnthropic(wire); }); + it('closes a mid-history tool call whose result is missing on the normal send path', async () => { + const ctx = testAgent(); + ctx.configure({ provider: PROVIDER, modelCapabilities: CAPS }); + // 'call_1' was issued but its result was never recorded; a later real turn + // ('call_2' + result) proves it is not in-flight. On a strict provider this + // bricks the session — every normal send re-rejects. The projector closes the + // mid-history orphan WITHOUT synthesizeMissing (the normal send path). + const orphaned: ContextMessage[] = [ + { role: 'user', content: [{ type: 'text', text: 'run it' }], toolCalls: [], origin: { kind: 'user' } }, + { + role: 'assistant', + content: [{ type: 'text', text: 'first call' }], + toolCalls: [{ type: 'function', id: 'call_1', name: 'Bash', arguments: '{}' }], + }, + { role: 'user', content: [{ type: 'text', text: 'next thing' }], toolCalls: [], origin: { kind: 'user' } }, + { + role: 'assistant', + content: [{ type: 'text', text: 'second call' }], + toolCalls: [{ type: 'function', id: 'call_2', name: 'Bash', arguments: '{}' }], + }, + { role: 'tool', content: [{ type: 'text', text: 'done' }], toolCalls: [], toolCallId: 'call_2' }, + ]; + + // Normal send path: no synthesizeMissing, no dropOrphanResults. + const projected = ctx.agent.context.project(orphaned); + const wire = await toAnthropicWire(projected, [BASH_TOOL]); + assertValidAnthropic(wire); + // The mid-history orphan 'call_1' is closed by a synthetic tool_result. + const call1Index = wire.findIndex((m) => + m.content.some((b) => b.type === 'tool_use' && b.id === 'call_1'), + ); + expect(call1Index).toBeGreaterThanOrEqual(0); + expect( + wire[call1Index + 1]!.content.some( + (b) => b.type === 'tool_result' && b.tool_use_id === 'call_1', + ), + ).toBe(true); + }); + + it('drops a stray tool result with no matching call on the strict resend path', 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. + 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, + }); + expect(projected.some((m) => m.role === 'tool')).toBe(false); + const wire = await toAnthropicWire(projected); + assertValidAnthropic(wire); + }); + it('closes a still-open tool call in the summarizer request with a synthetic result', async () => { const ctx = testAgent(); ctx.configure({ provider: PROVIDER, modelCapabilities: CAPS }); diff --git a/packages/agent-core/test/agent/context.test.ts b/packages/agent-core/test/agent/context.test.ts index eaec4923d..9ebe197ab 100644 --- a/packages/agent-core/test/agent/context.test.ts +++ b/packages/agent-core/test/agent/context.test.ts @@ -168,6 +168,52 @@ describe('Agent context', () => { expect(textOf(output)).toContain('exit code'); }); + it('normalizes a whitespace-only array tool result to the empty-output placeholder', () => { + const ctx = testAgent(); + ctx.configure(); + + ctx.dispatch({ + type: 'context.append_loop_event', + event: { type: 'step.begin', uuid: 's1', turnId: 't', step: 1 }, + }); + ctx.dispatch({ + type: 'context.append_loop_event', + event: { + type: 'tool.call', + uuid: 'call_ws', + turnId: 't', + step: 1, + stepUuid: 's1', + toolCallId: 'call_ws', + name: 'Run', + args: {}, + }, + }); + ctx.dispatch({ + type: 'context.append_loop_event', + event: { + type: 'tool.result', + parentUuid: 'call_ws', + toolCallId: 'call_ws', + // Array (ContentPart[]) output whose only block is whitespace. The tool + // contract allows arbitrary content arrays (e.g. MCP tools), so this must + // be normalized to the empty placeholder rather than left to be stripped + // empty by projection (which would throw on every send). + result: { output: [{ type: 'text', text: ' \n' }] }, + }, + }); + + expect(() => ctx.agent.context.messages).not.toThrow(); + expect(ctx.agent.context.messages).toMatchObject([ + { role: 'assistant', toolCalls: [{ id: 'call_ws' }] }, + { + role: 'tool', + content: [{ type: 'text', text: 'Tool output is empty.' }], + toolCallId: 'call_ws', + }, + ]); + }); + it('renders tool error and empty-output status as model-visible text', () => { const ctx = testAgent(); ctx.configure(); @@ -227,7 +273,7 @@ describe('Agent context', () => { ]); }); - it('drops empty text parts only in LLM projection', () => { + it('drops empty and whitespace-only text parts in LLM projection', () => { const history: ContextMessage[] = [ { role: 'user', @@ -247,12 +293,20 @@ describe('Agent context', () => { content: [{ type: 'text', text: '' }], toolCalls: [{ type: 'function', id: 'call_empty', name: 'empty', arguments: '{}' }], }, + { + role: 'tool', + content: [{ type: 'text', text: 'result' }], + toolCalls: [], + toolCallId: 'call_empty', + }, { role: 'assistant', content: [{ type: 'think', think: '', encrypted: 'enc_empty_thinking' }], toolCalls: [], }, { + // Whitespace-only message: strict providers reject the block, so the + // whole message is dropped from the projection. role: 'user', content: [{ type: 'text', text: ' ' }], toolCalls: [], @@ -271,13 +325,14 @@ describe('Agent context', () => { toolCalls: [{ type: 'function', id: 'call_empty', name: 'empty', arguments: '{}' }], }, { - role: 'assistant', - content: [{ type: 'think', think: '', encrypted: 'enc_empty_thinking' }], + role: 'tool', + content: [{ type: 'text', text: 'result' }], toolCalls: [], + toolCallId: 'call_empty', }, { - role: 'user', - content: [{ type: 'text', text: ' ' }], + role: 'assistant', + content: [{ type: 'think', think: '', encrypted: 'enc_empty_thinking' }], toolCalls: [], }, ]); diff --git a/packages/agent-core/test/agent/context/projector.test.ts b/packages/agent-core/test/agent/context/projector.test.ts index 6dc2a3ab6..8ddcad859 100644 --- a/packages/agent-core/test/agent/context/projector.test.ts +++ b/packages/agent-core/test/agent/context/projector.test.ts @@ -1,7 +1,7 @@ import type { ContentPart, Message, ToolCall } from '@moonshot-ai/kosong'; import { describe, expect, it } from 'vitest'; -import { project } from '../../../src/agent/context/projector'; +import { project, type ProjectionAnomaly } from '../../../src/agent/context/projector'; import type { ContextMessage } from '../../../src/agent/context/types'; // --------------------------------------------------------------------------- @@ -14,8 +14,10 @@ import type { ContextMessage } from '../../../src/agent/context/types'; // result exists anywhere in the projected history, that result sits in the // consecutive tool messages immediately following the assistant message. // -// A tool call with no recorded result anywhere is considered still in-flight -// (pending) and is intentionally left untouched — it is not an orphan. +// A tool call with no recorded result anywhere is closed with a synthetic +// `tool_result` when a later turn follows it (it cannot be in-flight). Only the +// trailing exchange's missing result is left untouched — there the call is +// genuinely still pending. interface MisplacedToolUse { readonly assistantIndex: number; @@ -63,6 +65,30 @@ function findMisplacedToolUses(messages: readonly Message[]): MisplacedToolUse[] return violations; } +/** + * Strict wire-compliance check: every assistant `tool_use` must be answered by a + * `tool_result` in the consecutive tool messages immediately following it. Use + * only where no trailing in-flight call is expected — an in-flight call has no + * result by design and would (correctly) fail this check. + */ +function everyToolUseImmediatelyAnswered(messages: readonly Message[]): boolean { + for (let i = 0; i < messages.length; i++) { + const message = messages[i]!; + if (message.role !== 'assistant' || message.toolCalls.length === 0) continue; + const adjacentResultIds = new Set(); + let j = i + 1; + while (j < messages.length && messages[j]!.role === 'tool') { + const id = messages[j]!.toolCallId; + if (id !== undefined) adjacentResultIds.add(id); + j++; + } + if (message.toolCalls.some((toolCall) => !adjacentResultIds.has(toolCall.id))) { + return false; + } + } + return true; +} + // --------------------------------------------------------------------------- // Builders // --------------------------------------------------------------------------- @@ -223,7 +249,7 @@ describe('project tool_use/tool_result adjacency', () => { it('leaves a pending (in-flight) tool call without a recorded result untouched', () => { const history: ContextMessage[] = [user('u1'), assistant(['a', 'b']), tool('a')]; - // b has no recorded result — it is still pending, not orphaned. + // b has no recorded result — it is the trailing exchange, still in-flight. const projected = project(history); expect(projected.map((m) => [m.role, m.toolCallId])).toEqual([ ['user', undefined], @@ -234,6 +260,42 @@ describe('project tool_use/tool_result adjacency', () => { expect(projected.some((m) => m.toolCallId === 'b')).toBe(false); }); + it('synthesizes a result for a mid-history tool call whose result is missing entirely', () => { + // 'a' has no recorded result anywhere, but a later turn (u2 / assistant b) + // proves the model already moved on — the call cannot be in-flight, so it is + // a genuine orphan that strict providers reject. It must be closed in place. + const history: ContextMessage[] = [ + user('u1'), + assistant(['a']), + user('u2'), + assistant(['b']), + tool('b'), + ]; + const projected = project(history); + const aIndex = projected.findIndex((m) => m.toolCalls.some((tc) => tc.id === 'a')); + expect(projected[aIndex + 1]).toMatchObject({ role: 'tool', toolCallId: 'a' }); + expect(textOf(projected[aIndex + 1])).toContain('not available'); + expect(findMisplacedToolUses(projected)).toEqual([]); + // No assistant tool_use is left unanswered for a strict provider. + expect(everyToolUseImmediatelyAnswered(projected)).toBe(true); + }); + + it('closes a mid-history orphan while leaving the trailing in-flight call untouched', () => { + // 'a' is a mid-history orphan (a later turn follows); 'b' is the trailing + // exchange whose result is genuinely still pending. + const history: ContextMessage[] = [ + user('u1'), + assistant(['a']), + user('u2'), + assistant(['b']), + ]; + const projected = project(history); + const aIndex = projected.findIndex((m) => m.toolCalls.some((tc) => tc.id === 'a')); + expect(projected[aIndex + 1]).toMatchObject({ role: 'tool', toolCallId: 'a' }); + // The trailing in-flight call 'b' is not closed with a synthetic result. + expect(projected.some((m) => m.toolCallId === 'b')).toBe(false); + }); + it('synthesizes a tool result for a missing tool call when synthesizeMissing is set', () => { const history: ContextMessage[] = [user('u1'), assistant(['a', 'b']), tool('a')]; const projected = project(history, { synthesizeMissing: true }); @@ -313,6 +375,166 @@ describe('project tool_use/tool_result adjacency', () => { }); }); +// --------------------------------------------------------------------------- +// Repair reporting (onAnomaly) +// --------------------------------------------------------------------------- + +describe('project repair reporting', () => { + it('reports nothing for an already well-formed history', () => { + const anomalies: ProjectionAnomaly[] = []; + project([user('u1'), assistant(['a']), tool('a'), user('u2')], { + onAnomaly: (a) => anomalies.push(a), + }); + expect(anomalies).toEqual([]); + }); + + it('reports a displaced result that had to be moved up', () => { + const anomalies: ProjectionAnomaly[] = []; + project([user('u1'), assistant(['a']), notification('ping'), tool('a')], { + onAnomaly: (a) => anomalies.push(a), + }); + expect(anomalies).toEqual([{ kind: 'tool_result_reordered', toolCallId: 'a' }]); + }); + + it('does not report adjacent parallel results that are merely out of order', () => { + const anomalies: ProjectionAnomaly[] = []; + project([user('u1'), assistant(['a', 'b', 'c']), tool('c'), tool('a'), tool('b')], { + onAnomaly: (a) => anomalies.push(a), + }); + expect(anomalies).toEqual([]); + }); + + it('reports a mid-history synthesis as a non-trailing (defect) repair', () => { + const anomalies: ProjectionAnomaly[] = []; + project([user('u1'), assistant(['a']), user('u2'), assistant(['b']), tool('b')], { + onAnomaly: (a) => anomalies.push(a), + }); + expect(anomalies).toEqual([ + { kind: 'tool_result_synthesized', toolCallId: 'a', trailing: false }, + ]); + }); + + it('marks a forced trailing synthesis as trailing (expected, not a defect)', () => { + const anomalies: ProjectionAnomaly[] = []; + project([user('u1'), assistant(['a', 'b']), tool('a')], { + synthesizeMissing: true, + onAnomaly: (a) => anomalies.push(a), + }); + expect(anomalies).toEqual([ + { kind: 'tool_result_synthesized', toolCallId: 'b', trailing: true }, + ]); + }); + + it('reports a dropped orphan result only on the strict path', () => { + 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 + + const strict: ProjectionAnomaly[] = []; + project(history, { dropOrphanResults: true, onAnomaly: (a) => strict.push(a) }); + expect(strict).toEqual([{ kind: 'orphan_tool_result_dropped', toolCallId: 'stray' }]); + }); + + it('reports a whitespace-only text drop but not a truly-empty one', () => { + const anomalies: ProjectionAnomaly[] = []; + project( + [ + // empty '' block (routine) followed by real text — not reported + { role: 'user', content: [textPart(''), textPart('hi')], toolCalls: [] }, + // whitespace-only block dropped — reported + { role: 'assistant', content: [textPart(' \n'), textPart('ok')], toolCalls: [] }, + ], + { onAnomaly: (a) => anomalies.push(a) }, + ); + expect(anomalies).toEqual([{ kind: 'whitespace_text_dropped', role: 'assistant' }]); + }); + + it('reports leading-non-user drops and consecutive-assistant merges (strict)', () => { + const anomalies: ProjectionAnomaly[] = []; + project( + [ + { role: 'assistant', content: [textPart('opener')], toolCalls: [] }, + user('hi'), + { role: 'assistant', content: [textPart('one')], toolCalls: [] }, + { role: 'assistant', content: [textPart('two')], toolCalls: [] }, + ], + { dropLeadingNonUser: true, mergeConsecutiveAssistants: true, onAnomaly: (a) => anomalies.push(a) }, + ); + expect(anomalies).toContainEqual({ kind: 'consecutive_assistants_merged' }); + expect(anomalies).toContainEqual({ kind: 'leading_non_user_dropped', role: 'assistant' }); + }); +}); + +// --------------------------------------------------------------------------- +// Whitespace-only text + strict-provider sanitizers +// --------------------------------------------------------------------------- + +function ws(text: string): ContextMessage { + return { role: 'user', content: [textPart(text)], toolCalls: [] }; +} + +function assistantText(text: string): ContextMessage { + return { role: 'assistant', content: [textPart(text)], toolCalls: [] }; +} + +describe('project drops whitespace-only text', () => { + it('drops a text block that is only whitespace (Anthropic rejects it)', () => { + const projected = project([ + user('real'), + { + role: 'assistant', + content: [textPart(' '), textPart('answer')], + toolCalls: [], + }, + ]); + const assistantMsg = projected.find((m) => m.role === 'assistant'); + expect(assistantMsg?.content).toEqual([{ type: 'text', text: 'answer' }]); + }); + + it('drops a message whose only text block is whitespace', () => { + const projected = project([user('real'), ws(' \n\t ')]); + expect(projected.map((m) => textOf(m))).toEqual(['real']); + }); + + it('keeps surrounding whitespace inside a non-empty block', () => { + const projected = project([user(' hello ')]); + expect(textOf(projected[0])).toBe(' hello '); + }); +}); + +describe('project strict-provider sanitizers', () => { + it('drops leading non-user messages so the first message is a user turn', () => { + // History that (pathologically) starts with an assistant turn. + const projected = project( + [assistantText('stray opener'), user('hi'), assistant(['a']), tool('a')], + { dropLeadingNonUser: true }, + ); + expect(projected[0]?.role).toBe('user'); + expect(textOf(projected[0])).toBe('hi'); + }); + + it('only drops leading non-user under the strict flag (normal path keeps them)', () => { + const history: ContextMessage[] = [assistantText('stray opener'), user('hi')]; + expect(project(history)[0]?.role).toBe('assistant'); + expect(project(history, { dropLeadingNonUser: true })[0]?.role).toBe('user'); + }); + + it('merges consecutive assistant messages under the strict flag', () => { + const projected = project([user('hi'), assistantText('part one'), assistantText('part two')], { + mergeConsecutiveAssistants: true, + }); + expect(projected.map((m) => m.role)).toEqual(['user', 'assistant']); + expect(textOf(projected[1])).toContain('part one'); + expect(textOf(projected[1])).toContain('part two'); + }); + + it('leaves consecutive assistant messages untouched on the normal path', () => { + const projected = project([user('hi'), assistantText('part one'), assistantText('part two')]); + expect(projected.map((m) => m.role)).toEqual(['user', 'assistant', 'assistant']); + }); +}); + // --------------------------------------------------------------------------- // Property-based fuzz test // --------------------------------------------------------------------------- diff --git a/packages/agent-core/test/loop/tool-exchange-fallback.e2e.test.ts b/packages/agent-core/test/loop/tool-exchange-fallback.e2e.test.ts new file mode 100644 index 000000000..6357cb424 --- /dev/null +++ b/packages/agent-core/test/loop/tool-exchange-fallback.e2e.test.ts @@ -0,0 +1,124 @@ +/** + * Post-400 strict-resend fallback. + * + * When a strict provider rejects a step with a tool_use/tool_result adjacency + * 400, the same history would be re-sent every turn and the session would stay + * stuck forever. `executeLoopStep` resends ONCE with a strict, guaranteed + * wire-compliant rebuild (`buildMessagesStrict`). Any other error propagates + * unchanged and the strict builder is never consulted. + */ + +import { APIStatusError, type Message } from '@moonshot-ai/kosong'; +import { describe, expect, it } from 'vitest'; + +import { + createLoopEventDispatcher, + runTurn, + type LoopMessageBuilder, + type RunTurnInput, +} from '../../src/loop/index'; +import { CollectingSink } from './fixtures/collecting-sink'; +import { FakeLLM, makeEndTurnResponse } from './fixtures/fake-llm'; +import { RecordingContext } from './fixtures/recording-context'; + +const ADJACENCY_400 = new APIStatusError( + 400, + 'messages.142: `tool_use` ids were found without `tool_result` blocks immediately after: ' + + 'toolu_01MWFhDRqdbB4nzCJNuWYiun. Each `tool_use` block must have a corresponding ' + + '`tool_result` block in the next message.', +); + +function userMessage(text: string): Message { + return { role: 'user', content: [{ type: 'text', text }], toolCalls: [] }; +} + +interface Harness { + readonly input: RunTurnInput; + readonly llm: FakeLLM; + readonly strictCalls: { count: number }; + readonly strictMessages: Message[]; +} + +function makeHarness(error: unknown): Harness { + const llm = new FakeLLM({ + responses: [makeEndTurnResponse('unused'), makeEndTurnResponse('recovered')], + throwOnIndex: { index: 0, error }, + }); + const context = new RecordingContext({ messages: [userMessage('normal projection')] }); + const sink = new CollectingSink({}); + const strictMessages: Message[] = [userMessage('strict projection')]; + const strictCalls = { count: 0 }; + const buildMessagesStrict: LoopMessageBuilder = () => { + strictCalls.count += 1; + return strictMessages; + }; + const input: RunTurnInput = { + turnId: 'turn-1', + signal: new AbortController().signal, + llm, + buildMessages: context.buildMessages, + buildMessagesStrict, + dispatchEvent: createLoopEventDispatcher({ + appendTranscriptRecord: context.appendTranscriptRecord, + emitLiveEvent: sink.emit, + }), + }; + return { input, llm, strictCalls, strictMessages }; +} + +describe('executeLoopStep — tool exchange adjacency fallback', () => { + it('resends once with strict messages after an adjacency 400 and recovers', async () => { + const { input, llm, strictCalls, strictMessages } = makeHarness(ADJACENCY_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); + // The first attempt used the normal projection; the resend used the strict one. + 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')); + + await expect(runTurn(input)).rejects.toThrow('Bad request'); + + expect(llm.callCount).toBe(1); + expect(strictCalls.count).toBe(0); + }); + + it('resends only once: if the strict rebuild is also rejected, it gives up (no loop)', async () => { + // Throw a recoverable structural 400 on every attempt; the loop must stop + // after exactly two provider calls (first attempt + one strict resend). + const llm = new FakeLLM({ responses: [] }); + let calls = 0; + llm.chat = async () => { + calls += 1; + throw ADJACENCY_400; + }; + const context = new RecordingContext({ messages: [userMessage('normal')] }); + const sink = new CollectingSink({}); + let strictCount = 0; + const input: RunTurnInput = { + turnId: 'turn-1', + signal: new AbortController().signal, + llm, + buildMessages: context.buildMessages, + buildMessagesStrict: () => { + strictCount += 1; + return [userMessage('strict')]; + }, + dispatchEvent: createLoopEventDispatcher({ + appendTranscriptRecord: context.appendTranscriptRecord, + emitLiveEvent: sink.emit, + }), + }; + + await expect(runTurn(input)).rejects.toBe(ADJACENCY_400); + expect(calls).toBe(2); // first attempt + one strict resend, then give up + expect(strictCount).toBe(1); + }); +}); diff --git a/packages/kosong/src/errors.ts b/packages/kosong/src/errors.ts index 989f02249..a2a8cdc4a 100644 --- a/packages/kosong/src/errors.ts +++ b/packages/kosong/src/errors.ts @@ -143,6 +143,51 @@ export function isContextOverflowStatusError(statusCode: number, message: string return CONTEXT_OVERFLOW_MESSAGE_PATTERNS.some((pattern) => pattern.test(lowerMessage)); } +// Strict providers (Anthropic) reject a request whose assistant `tool_use` and +// `tool_result` 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. 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/, + /unexpected\s+`?tool_result/, +] as const; + +export function isToolExchangeAdjacencyError(error: unknown): boolean { + if (!(error instanceof APIStatusError)) return false; + if (error instanceof APIContextOverflowError) return false; + if (error.statusCode !== 400 && error.statusCode !== 422) return false; + const lowerMessage = error.message.toLowerCase(); + return TOOL_EXCHANGE_ADJACENCY_MESSAGE_PATTERNS.some((pattern) => pattern.test(lowerMessage)); +} + +// The broader family of structural request rejections a strict provider returns +// when the message array itself is malformed — tool_use/tool_result pairing, +// empty or whitespace-only text blocks, a non-user first message, or +// non-alternating roles. All are deterministic 4xx validation failures (no +// generation happened) on a history that is re-sent every turn, so the only +// recovery is to resend a re-projected, strictly wire-compliant request rather +// than leave the session permanently stuck. Context-overflow 400s are excluded — +// they are handled by compaction, not by re-projection. +const STRUCTURAL_REQUEST_MESSAGE_PATTERNS = [ + /text content blocks must be non-empty/, + /text content blocks must contain non-whitespace/, + /first message must use the .*user.* role/, + /roles must alternate/, + /multiple .*(?:user|assistant).* roles in a row/, +] as const; + +export function isRecoverableRequestStructureError(error: unknown): boolean { + if (isToolExchangeAdjacencyError(error)) return true; + if (!(error instanceof APIStatusError)) return false; + if (error instanceof APIContextOverflowError) return false; + if (error.statusCode !== 400 && error.statusCode !== 422) return false; + const lowerMessage = error.message.toLowerCase(); + return STRUCTURAL_REQUEST_MESSAGE_PATTERNS.some((pattern) => pattern.test(lowerMessage)); +} + export function isProviderRateLimitError(error: unknown): boolean { if (error instanceof APIProviderRateLimitError) return true; diff --git a/packages/kosong/src/index.ts b/packages/kosong/src/index.ts index b8bd9bdcb..01662bcbc 100644 --- a/packages/kosong/src/index.ts +++ b/packages/kosong/src/index.ts @@ -68,7 +68,9 @@ export { ChatProviderError, isContextOverflowStatusError, isProviderRateLimitError, + isRecoverableRequestStructureError, isRetryableGenerateError, + isToolExchangeAdjacencyError, } from './errors'; /** diff --git a/packages/kosong/test/errors.test.ts b/packages/kosong/test/errors.test.ts index 0317f405a..ad1f82431 100644 --- a/packages/kosong/test/errors.test.ts +++ b/packages/kosong/test/errors.test.ts @@ -7,7 +7,9 @@ import { APITimeoutError, ChatProviderError, isProviderRateLimitError, + isRecoverableRequestStructureError, isRetryableGenerateError, + isToolExchangeAdjacencyError, normalizeAPIStatusError, } from '#/errors'; import { describe, expect, it } from 'vitest'; @@ -209,6 +211,102 @@ describe('normalizeAPIStatusError', () => { }); }); +describe('isToolExchangeAdjacencyError', () => { + // The exact Anthropic message observed in the field when a tool_use was not + // immediately followed by its tool_result. + const ANTHROPIC_MISSING_RESULT = + 'messages.142: `tool_use` ids were found without `tool_result` blocks immediately after: ' + + 'toolu_01MWFhDRqdbB4nzCJNuWYiun. Each `tool_use` block must have a corresponding ' + + '`tool_result` block in the next message.'; + + it('matches the missing-tool_result 400', () => { + expect(isToolExchangeAdjacencyError(new APIStatusError(400, ANTHROPIC_MISSING_RESULT))).toBe( + true, + ); + }); + + it('matches the reverse unexpected-tool_result 400', () => { + expect( + isToolExchangeAdjacencyError( + new APIStatusError( + 400, + 'messages.5: `tool_result` block(s) provided when previous message does not ' + + 'contain any `tool_use` blocks', + ), + ), + ).toBe(true); + expect( + isToolExchangeAdjacencyError(new APIStatusError(400, 'unexpected `tool_result` block')), + ).toBe(true); + }); + + it('also matches a 422 with the same shape', () => { + expect(isToolExchangeAdjacencyError(new APIStatusError(422, ANTHROPIC_MISSING_RESULT))).toBe( + true, + ); + }); + + it('does not match a context-overflow 400 or unrelated errors', () => { + expect( + isToolExchangeAdjacencyError(new APIContextOverflowError(400, 'context length exceeded')), + ).toBe(false); + expect(isToolExchangeAdjacencyError(new APIStatusError(400, 'Bad request'))).toBe(false); + expect(isToolExchangeAdjacencyError(new APIStatusError(500, ANTHROPIC_MISSING_RESULT))).toBe( + false, + ); + expect(isToolExchangeAdjacencyError(new Error(ANTHROPIC_MISSING_RESULT))).toBe(false); + expect(isToolExchangeAdjacencyError('boom')).toBe(false); + }); +}); + +describe('isRecoverableRequestStructureError', () => { + it('matches the whole tool_use/tool_result adjacency family', () => { + expect( + isRecoverableRequestStructureError( + new APIStatusError(400, '`tool_use` ids were found without `tool_result` blocks'), + ), + ).toBe(true); + }); + + it('matches empty / whitespace-only text content rejections', () => { + expect( + isRecoverableRequestStructureError( + new APIStatusError(400, 'messages: text content blocks must be non-empty'), + ), + ).toBe(true); + expect( + isRecoverableRequestStructureError( + new APIStatusError(400, 'text content blocks must contain non-whitespace text'), + ), + ).toBe(true); + }); + + it('matches first-message-must-be-user and role-alternation rejections', () => { + expect( + isRecoverableRequestStructureError( + new APIStatusError(400, 'messages: first message must use the "user" role'), + ), + ).toBe(true); + expect( + isRecoverableRequestStructureError( + new APIStatusError( + 400, + 'messages: roles must alternate between "user" and "assistant", but found multiple "user" roles in a row', + ), + ), + ).toBe(true); + }); + + it('does not match context overflow, auth, or non-status errors', () => { + expect( + isRecoverableRequestStructureError(new APIContextOverflowError(400, 'context length exceeded')), + ).toBe(false); + expect(isRecoverableRequestStructureError(new APIStatusError(401, 'unauthorized'))).toBe(false); + expect(isRecoverableRequestStructureError(new APIStatusError(400, 'Bad request'))).toBe(false); + expect(isRecoverableRequestStructureError(new Error('roles must alternate'))).toBe(false); + }); +}); + describe('isProviderRateLimitError', () => { it('matches explicit HTTP 429 status errors', () => { expect(isProviderRateLimitError(new APIProviderRateLimitError('rate limited'))).toBe(true);