diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 8d5a4cca08..858ab2ce4f 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -3,6 +3,7 @@ import { detectFormatFromEndpoint, getTargetFormat } from "../services/provider. import { injectSystemPrompt } from "../services/systemPrompt.ts"; import { translateRequest, needsTranslation } from "../translator/index.ts"; import { FORMATS } from "../translator/formats.ts"; +import { splitMisplacedToolResults } from "../translator/helpers/claudeHelper.ts"; import { createSSETransformStreamWithLogger, createPassthroughStreamWithLogger, @@ -2965,6 +2966,11 @@ export async function handleChatCore({ return []; }); } + + // #2815: move stray tool_result blocks out of assistant messages. + payload.messages = splitMisplacedToolResults( + payload.messages as ClaudeMessage[] + ) as unknown as Record[]; }; try { @@ -3058,6 +3064,11 @@ export async function handleChatCore({ // Only lift system/developer messages — preserves Claude Code's // native payload structure (documents, tool chains, thinking, etc.) extractSystemRoleMessages(translatedBody); + if (Array.isArray(translatedBody.messages)) { + translatedBody.messages = splitMisplacedToolResults( + translatedBody.messages as ClaudeMessage[] + ) as typeof translatedBody.messages; + } } else { normalizeClaudeUpstreamMessages(translatedBody, { preserveToolResultBlocks: true }); } diff --git a/open-sse/translator/helpers/claudeHelper.ts b/open-sse/translator/helpers/claudeHelper.ts index 6f9bc3d6e4..088c354b67 100644 --- a/open-sse/translator/helpers/claudeHelper.ts +++ b/open-sse/translator/helpers/claudeHelper.ts @@ -57,6 +57,61 @@ export function hasValidContent(msg: ClaudeMessage): boolean { return false; } +// Move tool_result blocks out of assistant messages into the preceding user +// turn. Anthropic 400s on tool_result inside assistant. Drop tool_results +// whose tool_use_id has not been emitted by an earlier assistant turn — +// keeping them just shifts the 400 to "unexpected tool_use_id". See #2815. +export function splitMisplacedToolResults(messages: ClaudeMessage[]): ClaudeMessage[] { + if (!Array.isArray(messages) || messages.length === 0) return messages; + + const out: ClaudeMessage[] = []; + const seenToolUseIds = new Set(); + + const recordToolUseIds = (blocks: ClaudeContentBlock[]) => { + for (const b of blocks) { + if (b?.type === "tool_use" && typeof b.id === "string") { + seenToolUseIds.add(b.id); + } + } + }; + + for (const msg of messages) { + if (msg.role !== "assistant" || !Array.isArray(msg.content)) { + out.push(msg); + continue; + } + + const toolResults = msg.content.filter((b) => b?.type === "tool_result"); + if (toolResults.length === 0) { + out.push(msg); + recordToolUseIds(msg.content); + continue; + } + + const validToolResults = toolResults.filter( + (b) => typeof b?.tool_use_id === "string" && seenToolUseIds.has(b.tool_use_id) + ); + const remaining = msg.content.filter((b) => b?.type !== "tool_result"); + + if (validToolResults.length > 0) { + const prev = out[out.length - 1]; + if (prev && prev.role === "user" && Array.isArray(prev.content)) { + out[out.length - 1] = { ...prev, content: [...prev.content, ...validToolResults] }; + } else { + out.push({ role: "user", content: validToolResults }); + } + } + + // Drop the assistant message entirely if only tool_result blocks remained. + if (remaining.length > 0) { + out.push({ ...msg, content: remaining }); + recordToolUseIds(remaining); + } + } + + return out; +} + // Fix tool_use/tool_result ordering for Claude API // 1. Assistant message with tool_use: remove text AFTER tool_use (Claude doesn't allow) // 2. Merge consecutive same-role messages @@ -226,6 +281,10 @@ export function prepareClaudeRequest( body.tools = body.tools.filter((tool) => tool.name && tool.name?.trim()); } + // Pass 1.45: Move stray tool_result blocks out of assistant messages + // before any ordering fix runs (#2815). + filtered = splitMisplacedToolResults(filtered); + // Pass 1.5: Fix tool_use/tool_result ordering // Each tool_use must have tool_result in the NEXT message (not same message with other content) filtered = fixToolUseOrdering(filtered); diff --git a/src/lib/skills/interception.ts b/src/lib/skills/interception.ts index 575f298b3b..f26b954109 100644 --- a/src/lib/skills/interception.ts +++ b/src/lib/skills/interception.ts @@ -1,4 +1,5 @@ import { skillExecutor } from "./executor"; +import { skillRegistry } from "./registry"; import { builtinSkills } from "./builtins"; import { detectProvider } from "./injection"; import { OMNIROUTE_WEB_SEARCH_FALLBACK_TOOL_NAME } from "@omniroute/open-sse/services/webSearchFallback.ts"; @@ -215,17 +216,35 @@ function parseArguments(args: string | Record): Record { + // Only intercept tool_use blocks that resolve to a builtin handler or a + // registered custom skill. Unknown tool names are forwarded untouched so + // client-native tools (Bash, Read, etc.) are not turned into Skill-not-found + // tool_result blocks appended back into the assistant response. See #2815. + + // Ensure the registry cache is warm for this apiKeyId before filtering. + // isRegisteredCustomSkill() reads registeredSkills synchronously, so on a + // cold/fresh process a skill that exists only in the DB would be missed + // (false negative → silently skipped). Mirror the pattern used in + // open-sse/mcp-server/tools/skillTools.ts. loadFromDatabase() is a no-op + // when the cache is already warm (TTL = 60 s), so repeated calls are cheap. + await skillRegistry.loadFromDatabase(context.apiKeyId); + const toolCalls = extractToolCalls(response, modelId).filter((call) => { - const builtinHandlerName = resolveBuiltinHandlerName(call.name, context); - if (builtinHandlerName) { - return true; - } - return context.customSkillExecutionEnabled !== false; + if (typeof call?.name !== "string" || !call.name) return false; + if (resolveBuiltinHandlerName(call.name, context)) return true; + if (context.customSkillExecutionEnabled === false) return false; + return isRegisteredCustomSkill(call.name, context.apiKeyId); }); if (toolCalls.length === 0) { diff --git a/tests/unit/skills-interception.test.ts b/tests/unit/skills-interception.test.ts index b1fa4005c2..3cc7b4491b 100644 --- a/tests/unit/skills-interception.test.ts +++ b/tests/unit/skills-interception.test.ts @@ -265,3 +265,71 @@ test("handleToolCallExecution appends Responses API function_call_output items", }, ]); }); + +test("handleToolCallExecution forwards unregistered client-native tool_use untouched (#2815)", async () => { + const original = { + content: [ + { type: "tool_use", id: "tool-native", name: "Bash", input: { command: "ls" } }, + { type: "text", text: "Calling Bash" }, + ], + }; + const result = await handleToolCallExecution(original, "claude-3-7-sonnet", executionContext); + + assert.equal(result, original); + assert.equal( + (result.content as Array<{ type: string }>).some((b) => b.type === "tool_result"), + false + ); +}); + +test("handleToolCallExecution intercepts a registered skill alongside an unregistered tool (#2815)", async () => { + const mixed = await handleToolCallExecution( + { + content: [ + { type: "tool_use", id: "tool-native", name: "Bash", input: { command: "ls" } }, + { type: "tool_use", id: "tool-skill", name: "lookup@1.0.0", input: { id: "9" } }, + ], + }, + "claude-3-7-sonnet", + executionContext + ); + + assert.deepEqual(mixed.content, [ + { type: "tool_use", id: "tool-native", name: "Bash", input: { command: "ls" } }, + { type: "tool_use", id: "tool-skill", name: "lookup@1.0.0", input: { id: "9" } }, + { + type: "tool_result", + tool_use_id: "tool-skill", + content: '{"record":"resolved:9"}', + }, + ]); +}); + +test("handleToolCallExecution loads registry from DB on cold cache (covers loadFromDatabase fix)", async () => { + // Skills are in the DB (registered in beforeEach) but we evict the in-memory + // cache to simulate a cold/fresh process. Without the loadFromDatabase() call + // at the top of handleToolCallExecution, isRegisteredCustomSkill() would + // return false (false negative) and the skill would be silently skipped. + skillRegistry["registeredSkills"].clear(); + skillRegistry["versionCache"].clear(); + skillRegistry.invalidateCache(); + + const result = await handleToolCallExecution( + { + content: [ + { type: "tool_use", id: "tool-skill", name: "lookup@1.0.0", input: { id: "cold" } }, + ], + }, + "claude-3-7-sonnet", + executionContext + ); + + assert.deepEqual(result.content, [ + { type: "tool_use", id: "tool-skill", name: "lookup@1.0.0", input: { id: "cold" } }, + { + type: "tool_result", + tool_use_id: "tool-skill", + content: '{"record":"resolved:cold"}', + }, + ]); +}); diff --git a/tests/unit/translator-helper-branches.test.ts b/tests/unit/translator-helper-branches.test.ts index 5318462a37..7266db3df5 100644 --- a/tests/unit/translator-helper-branches.test.ts +++ b/tests/unit/translator-helper-branches.test.ts @@ -257,6 +257,44 @@ test("claudeHelper validates content, ordering and request preparation branches" { type: "tool_use", id: "call_1", name: "lookup", input: {} }, ]); + // splitMisplacedToolResults: a tool_result whose tool_use_id was already + // emitted by an earlier assistant turn is moved into the preceding user + // message. The trailing tool_use survives on the assistant side. (#2815) + const split = claudeHelper.splitMisplacedToolResults([ + { role: "user", content: [{ type: "text", text: "q" }] }, + { role: "assistant", content: [{ type: "tool_use", id: "call_x", name: "Read", input: {} }] }, + { + role: "assistant", + content: [ + { type: "tool_result", tool_use_id: "call_x", content: "ok" }, + { type: "tool_use", id: "call_y", name: "Read", input: {} }, + ], + }, + ]); + assert.deepEqual(split, [ + { role: "user", content: [{ type: "text", text: "q" }] }, + { role: "assistant", content: [{ type: "tool_use", id: "call_x", name: "Read", input: {} }] }, + { role: "user", content: [{ type: "tool_result", tool_use_id: "call_x", content: "ok" }] }, + { role: "assistant", content: [{ type: "tool_use", id: "call_y", name: "Read", input: {} }] }, + ]); + + // tool_result whose id has not been seen earlier is dropped — moving it + // would just shift the 400 to "unexpected tool_use_id". + const droppedOrphan = claudeHelper.splitMisplacedToolResults([ + { role: "user", content: [{ type: "text", text: "q" }] }, + { + role: "assistant", + content: [ + { type: "tool_result", tool_use_id: "self-ref", content: "Skill not found" }, + { type: "tool_use", id: "self-ref", name: "Read", input: {} }, + ], + }, + ]); + assert.deepEqual(droppedOrphan, [ + { role: "user", content: [{ type: "text", text: "q" }] }, + { role: "assistant", content: [{ type: "tool_use", id: "self-ref", name: "Read", input: {} }] }, + ]); + const prepared = claudeHelper.prepareClaudeRequest( { system: [