-
Notifications
You must be signed in to change notification settings - Fork 0
fix(opencode): keep invalid repair sink under DFIR deny-all #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, unknown>): 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<T extends { toolName: string; input: string }>(input: { | ||
| readonly toolCall: T | ||
| readonly tools: Record<string, unknown> | ||
| 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, | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
OPENCODE_EXPERIMENTAL_NATIVE_LLMsessions with DFIR-style"*": "deny"permissions, this now leaves the internalinvalidtool inprepared.tools; the native branch passes all ofprepared.toolsintonativeTools(...)/toDefinitions(...)and has noactiveToolsfilter like the AI SDK branch. That means the repair sink is advertised to the provider rather than only kept executable, so the model can selectinvaliddirectly and loop on repair hints. Filter the sink from native tool definitions while keeping it available for repair execution.Useful? React with 👍 / 👎.