From bd75e8435ce5418e5065927f87c4b3e53f8c1af5 Mon Sep 17 00:00:00 2001 From: "timothy.vang" Date: Thu, 9 Jul 2026 20:27:34 +0000 Subject: [PATCH] fix(opencode): keep invalid repair sink under DFIR deny-all SessionPrompt tool_choice=required still failed FORCE_AGENT when models invented MCP names: experimental_repairToolCall remapped to `invalid`, but DFIR `"*": deny` stripped that sink so AI SDK reported "unavailable tool 'invalid'". Keep the sink executable (hidden via activeTools), remap underscore/hyphen drift onto real tools, and cover with unit tests. --- packages/opencode/src/session/llm.ts | 35 ++---- packages/opencode/src/session/llm/request.ts | 9 +- .../opencode/src/session/llm/tool-repair.ts | 82 +++++++++++++ .../opencode/test/session/tool-repair.test.ts | 112 ++++++++++++++++++ 4 files changed, 212 insertions(+), 26 deletions(-) create mode 100644 packages/opencode/src/session/llm/tool-repair.ts create mode 100644 packages/opencode/test/session/tool-repair.test.ts diff --git a/packages/opencode/src/session/llm.ts b/packages/opencode/src/session/llm.ts index aaccbfb..33d10c8 100644 --- a/packages/opencode/src/session/llm.ts +++ b/packages/opencode/src/session/llm.ts @@ -29,6 +29,7 @@ import * as OtelTracer from "@effect/opentelemetry/Tracer" import { LLMAISDK } from "./llm/ai-sdk" import { LLMNativeRuntime } from "./llm/native-runtime" import { LLMRequestPrep } from "./llm/request" +import { isInternalRepairTool, repairFailedToolCall } from "./llm/tool-repair" export const OUTPUT_TOKEN_MAX = ProviderTransform.OUTPUT_TOKEN_MAX @@ -294,36 +295,20 @@ const live: Layer.Layer< // Copilot returns the authoritative billed amount only in provider-specific response fields. includeRawChunks: input.model.providerID.includes("github-copilot"), async experimental_repairToolCall(failed) { - const lower = failed.toolCall.toolName.toLowerCase() - if (lower !== failed.toolCall.toolName && prepared.tools[lower]) { - return { - ...failed.toolCall, - toolName: lower, - } - } - // Surface a short available-tool list so weak local models can - // self-correct instead of looping on invented names until timeout. - const available = Object.keys(prepared.tools) - .filter((name) => name !== "invalid") - .slice(0, 48) - const hint = - available.length > 0 - ? ` Available tools (${available.length}): ${available.join(", ")}` - : "" - return { - ...failed.toolCall, - input: JSON.stringify({ - tool: failed.toolCall.toolName, - error: `${failed.error.message}.${hint}`, - }), - toolName: "invalid", - } + // Remap separator/case drift onto real tools; else route to internal + // `invalid` sink (kept under DFIR deny-all — see LLMRequestPrep). + return repairFailedToolCall({ + toolCall: failed.toolCall, + tools: prepared.tools, + errorMessage: failed.error.message, + }) }, temperature: prepared.params.temperature, topP: prepared.params.topP, topK: prepared.params.topK, providerOptions: ProviderTransform.providerOptions(input.model, prepared.params.options), - activeTools: Object.keys(prepared.tools).filter((x) => x !== "invalid"), + // Hide internal repair sink from the model; keep it in `tools` for execute. + activeTools: Object.keys(prepared.tools).filter((x) => !isInternalRepairTool(x)), tools: prepared.tools, toolChoice: input.toolChoice, maxOutputTokens: prepared.params.maxOutputTokens, diff --git a/packages/opencode/src/session/llm/request.ts b/packages/opencode/src/session/llm/request.ts index 4f93411..7683c9e 100644 --- a/packages/opencode/src/session/llm/request.ts +++ b/packages/opencode/src/session/llm/request.ts @@ -210,7 +210,14 @@ function resolveTools(input: Pick input.user.tools?.[k] !== false && !disabled.has(k)) + // Keep the internal `invalid` repair sink even under DFIR `"*": "deny"` profiles. + // activeTools still hides it from the model; experimental_repairToolCall needs it + // executable so unknown names become a tool-result retry hint instead of + // "unavailable tool 'invalid'". + return Record.filter( + input.tools, + (_, k) => k === "invalid" || (input.user.tools?.[k] !== false && !disabled.has(k)), + ) } export function hasToolCalls(messages: ModelMessage[]): boolean { diff --git a/packages/opencode/src/session/llm/tool-repair.ts b/packages/opencode/src/session/llm/tool-repair.ts new file mode 100644 index 0000000..7c6de14 --- /dev/null +++ b/packages/opencode/src/session/llm/tool-repair.ts @@ -0,0 +1,82 @@ +/** + * Repair weak-model tool names so SessionPrompt can recover instead of looping. + * + * Local models (e.g. gpt-oss) invent underscore/hyphen variants such as + * `findevil-agent_mcp_audit_append` when the registry has + * `findevil-agent-mcp_audit_append`. AI SDK's experimental_repairToolCall + * only helps when we remap to a real tool or to the internal `invalid` sink. + */ + +/** Internal repair sink — must stay executable, never advertised via activeTools. */ +export const INTERNAL_REPAIR_TOOL = "invalid" + +export function isInternalRepairTool(name: string) { + return name === INTERNAL_REPAIR_TOOL +} + +/** Collapse `-` / `_` runs so separator drift still matches registry names. */ +export function normalizeToolNameKey(name: string) { + return name.toLowerCase().replace(/[-_]+/g, "_") +} + +/** + * Map a model-emitted tool name onto an existing tools map entry. + * Returns undefined when no recoverable match exists (caller may use invalid sink). + */ +export function repairToolName(requested: string, tools: Record): string | undefined { + if (Object.prototype.hasOwnProperty.call(tools, requested) && !isInternalRepairTool(requested)) { + return requested + } + + const lower = requested.toLowerCase() + if (lower !== requested && Object.prototype.hasOwnProperty.call(tools, lower) && !isInternalRepairTool(lower)) { + return lower + } + + const target = normalizeToolNameKey(requested) + for (const name of Object.keys(tools)) { + if (isInternalRepairTool(name)) continue + if (normalizeToolNameKey(name) === target) return name + } + return undefined +} + +/** + * Build the experimental_repairToolCall result for a failed tool invocation. + * Prefers remapping to a real tool; otherwise routes to the internal invalid sink + * with an available-tool hint when that sink is present. + */ +export function repairFailedToolCall(input: { + readonly toolCall: T + readonly tools: Record + readonly errorMessage: string +}): T | null { + const repaired = repairToolName(input.toolCall.toolName, input.tools) + if (repaired) { + return { + ...input.toolCall, + toolName: repaired, + } + } + + if (!Object.prototype.hasOwnProperty.call(input.tools, INTERNAL_REPAIR_TOOL)) { + // DFIR deny-all profiles used to strip `invalid`; without the sink, return null + // so AI SDK rethrows the original NoSuchToolError (not a second 'invalid' miss). + return null + } + + const available = Object.keys(input.tools) + .filter((name) => !isInternalRepairTool(name)) + .slice(0, 48) + const hint = + available.length > 0 ? ` Available tools (${available.length}): ${available.join(", ")}` : "" + + return { + ...input.toolCall, + input: JSON.stringify({ + tool: input.toolCall.toolName, + error: `${input.errorMessage}.${hint}`, + }), + toolName: INTERNAL_REPAIR_TOOL, + } +} diff --git a/packages/opencode/test/session/tool-repair.test.ts b/packages/opencode/test/session/tool-repair.test.ts new file mode 100644 index 0000000..3219eb6 --- /dev/null +++ b/packages/opencode/test/session/tool-repair.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, test } from "bun:test" +import { Permission } from "../../src/permission" +import { + INTERNAL_REPAIR_TOOL, + isInternalRepairTool, + normalizeToolNameKey, + repairFailedToolCall, + repairToolName, +} from "../../src/session/llm/tool-repair" + +describe("tool-repair", () => { + const tools = { + "findevil-agent-mcp_audit_append": {}, + "findevil-mcp_evtx_query": {}, + "findevil-mcp_case_open": {}, + invalid: {}, + } + + test("normalizeToolNameKey collapses separator drift", () => { + expect(normalizeToolNameKey("findevil-agent_mcp_audit_append")).toBe( + normalizeToolNameKey("findevil-agent-mcp_audit_append"), + ) + expect(normalizeToolNameKey("findevil_mcp_evtx_query")).toBe(normalizeToolNameKey("findevil-mcp_evtx_query")) + }) + + test("repairToolName remaps underscore/hyphen MCP name drift", () => { + // m23/m24 FORCE_AGENT residual: model used agent_mcp instead of agent-mcp + expect(repairToolName("findevil-agent_mcp_audit_append", tools)).toBe("findevil-agent-mcp_audit_append") + expect(repairToolName("findevil_mcp_evtx_query", tools)).toBe("findevil-mcp_evtx_query") + expect(repairToolName("findevil-mcp_case_open", tools)).toBe("findevil-mcp_case_open") + }) + + test("repairToolName remaps case drift", () => { + expect(repairToolName("FinDevil-MCP_evtx_query", tools)).toBe("findevil-mcp_evtx_query") + }) + + test("repairToolName returns undefined for unknown tools", () => { + expect(repairToolName("totally_invented_tool", tools)).toBeUndefined() + expect(repairToolName("invalid", tools)).toBeUndefined() + }) + + test("repairFailedToolCall remaps separator drift to a real tool", () => { + const result = repairFailedToolCall({ + toolCall: { + toolCallId: "call_1", + toolName: "findevil-agent_mcp_audit_append", + input: JSON.stringify({ path: "/tmp/audit.jsonl" }), + }, + tools, + errorMessage: "Model tried to call unavailable tool 'findevil-agent_mcp_audit_append'", + }) + expect(result).toMatchObject({ + toolCallId: "call_1", + toolName: "findevil-agent-mcp_audit_append", + input: JSON.stringify({ path: "/tmp/audit.jsonl" }), + }) + }) + + test("repairFailedToolCall routes unknown names to invalid sink with available-tool hint", () => { + const result = repairFailedToolCall({ + toolCall: { + toolCallId: "call_2", + toolName: "not_a_real_tool", + input: "{}", + }, + tools, + errorMessage: "Model tried to call unavailable tool 'not_a_real_tool'", + }) + expect(result?.toolName).toBe(INTERNAL_REPAIR_TOOL) + const payload = JSON.parse(result!.input) as { tool: string; error: string } + expect(payload.tool).toBe("not_a_real_tool") + expect(payload.error).toContain("findevil-agent-mcp_audit_append") + expect(payload.error).not.toContain("invalid") + }) + + test("repairFailedToolCall returns null when invalid sink missing (deny-all residual)", () => { + const noSink = { + "findevil-mcp_evtx_query": {}, + } + expect( + repairFailedToolCall({ + toolCall: { toolCallId: "call_3", toolName: "bogus", input: "{}" }, + tools: noSink, + errorMessage: "missing", + }), + ).toBeNull() + }) + + test("isInternalRepairTool only matches invalid", () => { + expect(isInternalRepairTool("invalid")).toBe(true) + expect(isInternalRepairTool("findevil-mcp_evtx_query")).toBe(false) + }) +}) + +describe("DFIR deny-all keeps invalid repair sink", () => { + test("Permission.disabled marks invalid under * deny, but allow-list keeps findevil tools", () => { + const ruleset = Permission.fromConfig({ + "*": "deny", + "findevil-mcp_*": "allow", + "findevil-agent-mcp_*": "allow", + }) + const disabled = Permission.disabled( + ["invalid", "bash", "findevil-mcp_evtx_query", "findevil-agent-mcp_audit_append"], + ruleset, + ) + // Without the resolveTools special-case, invalid would be stripped — this is the gap #15 left open. + expect(disabled.has("invalid")).toBe(true) + expect(disabled.has("bash")).toBe(true) + expect(disabled.has("findevil-mcp_evtx_query")).toBe(false) + expect(disabled.has("findevil-agent-mcp_audit_append")).toBe(false) + }) +})