fix(skills): skip interception for unregistered client-native tools (#2815) (#2817)

Integrated into release/v3.8.6.

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<string, unknown>[];
   };
 
   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 });
       }

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<string>();
+
+  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);

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<string, unknown>): Record<string,
   }
 }
 
+function isRegisteredCustomSkill(toolName: string, apiKeyId: string): boolean {
+  const [name, version] = toolName.includes("@") ? toolName.split("@", 2) : [toolName, undefined];
+  const identifier = version ? `${name}@${version}` : name;
+  return skillRegistry.getSkill(identifier, apiKeyId) != null;
+}
+
 export async function handleToolCallExecution(
   response: any,
   modelId: string,
   context: ExecutionContext
 ): Promise<any> {
+  // 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) {

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"}',
+    },
+  ]);
+});

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: [