From 7be147b472776ed8a2d017de4a3368d2265f486f Mon Sep 17 00:00:00 2001 From: Ibrahim Khan <2005ibrahimkhan@gmail.com> Date: Sun, 19 Jul 2026 07:55:43 +0000 Subject: [PATCH] fix(core): omit undefined optional keys from glob/grep permission metadata The glob and grep tools built their permission metadata by assigning input.path/input.limit/input.include directly. When those optional inputs are omitted, the decoded input has no such key, so reading it yields undefined and materializes undefined-valued keys in the metadata bag (e.g. { root: ".", path: undefined, limit: undefined }). Permission metadata is Schema.Record(String, Unknown) and is serialized as JSON for session.permission.list. The undefined entries survive schema encoding as own properties and break strict JSON encoding of the response, so a single pending glob/grep permission with omitted optional inputs can make the whole listing fail. Only include the optional keys when their value is defined, matching the webfetch/websearch tools which spread the exact-optional decoded input and never introduce undefined keys. Refs #37650 --- packages/core/src/tool/glob.ts | 4 +- packages/core/src/tool/grep.ts | 6 +- packages/core/test/tool-glob.test.ts | 84 ++++++++++++++++++++++++++++ packages/core/test/tool-grep.test.ts | 84 ++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 packages/core/test/tool-glob.test.ts create mode 100644 packages/core/test/tool-grep.test.ts diff --git a/packages/core/src/tool/glob.ts b/packages/core/src/tool/glob.ts index f8bd1869e11e..9531a2ae0154 100644 --- a/packages/core/src/tool/glob.ts +++ b/packages/core/src/tool/glob.ts @@ -65,8 +65,8 @@ const layer = Layer.effectDiscard( save: ["*"], metadata: { root: input.path ?? ".", - path: input.path, - limit: input.limit, + ...(input.path !== undefined && { path: input.path }), + ...(input.limit !== undefined && { limit: input.limit }), }, sessionID: context.sessionID, agent: context.agent, diff --git a/packages/core/src/tool/grep.ts b/packages/core/src/tool/grep.ts index f455bd4c8a0a..c2fc680fa552 100644 --- a/packages/core/src/tool/grep.ts +++ b/packages/core/src/tool/grep.ts @@ -84,9 +84,9 @@ const layer = Layer.effectDiscard( save: ["*"], metadata: { root: ".", - path: input.path, - include: input.include, - limit: input.limit, + ...(input.path !== undefined && { path: input.path }), + ...(input.include !== undefined && { include: input.include }), + ...(input.limit !== undefined && { limit: input.limit }), }, sessionID: context.sessionID, agent: context.agent, diff --git a/packages/core/test/tool-glob.test.ts b/packages/core/test/tool-glob.test.ts new file mode 100644 index 000000000000..db2758949af5 --- /dev/null +++ b/packages/core/test/tool-glob.test.ts @@ -0,0 +1,84 @@ +import { describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder" +import { LayerNode } from "@opencode-ai/core/effect/layer-node" +import { Location } from "@opencode-ai/core/location" +import { PermissionV2 } from "@opencode-ai/core/permission" +import { Ripgrep } from "@opencode-ai/core/ripgrep" +import { AbsolutePath } from "@opencode-ai/core/schema" +import { SessionV2 } from "@opencode-ai/core/session" +import { GlobTool } from "@opencode-ai/core/tool/glob" +import { ToolRegistry } from "@opencode-ai/core/tool/registry" +import { ToolOutputStore } from "@opencode-ai/core/tool-output-store" +import { location } from "./fixture/location" +import { testEffect } from "./lib/effect" +import { toolIdentity, executeTool } from "./lib/tool" + +const sessionID = SessionV2.ID.make("ses_glob_test") +const assertions: PermissionV2.AssertInput[] = [] + +const permission = Layer.succeed( + PermissionV2.Service, + PermissionV2.Service.of({ + assert: (input) => Effect.sync(() => assertions.push(input)), + ask: () => Effect.die("unused"), + reply: () => Effect.die("unused"), + get: () => Effect.die("unused"), + forSession: () => Effect.die("unused"), + list: () => Effect.die("unused"), + }), +) +const ripgrep = Layer.succeed( + Ripgrep.Service, + Ripgrep.Service.of({ + find: () => Effect.die("unused"), + glob: () => Effect.succeed([]), + grep: () => Effect.die("unused"), + }), +) +const current = Layer.succeed( + Location.Service, + Location.Service.of(location({ directory: AbsolutePath.make("/project") })), +) + +const it = testEffect( + AppNodeBuilder.build(LayerNode.group([ToolRegistry.node, ToolRegistry.toolsNode, GlobTool.node]), [ + [PermissionV2.node, permission], + [Ripgrep.node, ripgrep], + [Location.node, current], + [ToolOutputStore.node, ToolOutputStore.nodeWithoutConfig], + ]), +) + +const call = (input: typeof GlobTool.Input.Encoded) => ({ + sessionID, + ...toolIdentity, + call: { type: "tool-call" as const, id: "call-glob", name: "glob", input }, +}) + +describe("GlobTool permission metadata", () => { + it.effect("omits absent optional fields so the metadata bag holds no undefined values", () => + Effect.gen(function* () { + assertions.length = 0 + const registry = yield* ToolRegistry.Service + yield* executeTool(registry, call({ pattern: "**/*.txt" })) + + expect(assertions).toHaveLength(1) + const metadata = assertions[0].metadata! + expect(metadata).toEqual({ root: "." }) + // toEqual ignores undefined-valued keys, so guard the actual keys directly: + // undefined entries break JSON encoding of session.permission.list. + expect(Object.keys(metadata)).toEqual(["root"]) + }), + ) + + it.effect("keeps provided optional fields", () => + Effect.gen(function* () { + assertions.length = 0 + const registry = yield* ToolRegistry.Service + yield* executeTool(registry, call({ pattern: "**/*.ts", path: "src", limit: 10 })) + + expect(assertions[0].metadata).toEqual({ root: "src", path: "src", limit: 10 }) + }), + ) +}) diff --git a/packages/core/test/tool-grep.test.ts b/packages/core/test/tool-grep.test.ts new file mode 100644 index 000000000000..061d802b483f --- /dev/null +++ b/packages/core/test/tool-grep.test.ts @@ -0,0 +1,84 @@ +import { describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder" +import { LayerNode } from "@opencode-ai/core/effect/layer-node" +import { Location } from "@opencode-ai/core/location" +import { PermissionV2 } from "@opencode-ai/core/permission" +import { Ripgrep } from "@opencode-ai/core/ripgrep" +import { AbsolutePath } from "@opencode-ai/core/schema" +import { SessionV2 } from "@opencode-ai/core/session" +import { GrepTool } from "@opencode-ai/core/tool/grep" +import { ToolRegistry } from "@opencode-ai/core/tool/registry" +import { ToolOutputStore } from "@opencode-ai/core/tool-output-store" +import { location } from "./fixture/location" +import { testEffect } from "./lib/effect" +import { toolIdentity, executeTool } from "./lib/tool" + +const sessionID = SessionV2.ID.make("ses_grep_test") +const assertions: PermissionV2.AssertInput[] = [] + +const permission = Layer.succeed( + PermissionV2.Service, + PermissionV2.Service.of({ + assert: (input) => Effect.sync(() => assertions.push(input)), + ask: () => Effect.die("unused"), + reply: () => Effect.die("unused"), + get: () => Effect.die("unused"), + forSession: () => Effect.die("unused"), + list: () => Effect.die("unused"), + }), +) +const ripgrep = Layer.succeed( + Ripgrep.Service, + Ripgrep.Service.of({ + find: () => Effect.die("unused"), + glob: () => Effect.die("unused"), + grep: () => Effect.succeed([]), + }), +) +const current = Layer.succeed( + Location.Service, + Location.Service.of(location({ directory: AbsolutePath.make("/project") })), +) + +const it = testEffect( + AppNodeBuilder.build(LayerNode.group([ToolRegistry.node, ToolRegistry.toolsNode, GrepTool.node]), [ + [PermissionV2.node, permission], + [Ripgrep.node, ripgrep], + [Location.node, current], + [ToolOutputStore.node, ToolOutputStore.nodeWithoutConfig], + ]), +) + +const call = (input: typeof GrepTool.Input.Encoded) => ({ + sessionID, + ...toolIdentity, + call: { type: "tool-call" as const, id: "call-grep", name: "grep", input }, +}) + +describe("GrepTool permission metadata", () => { + it.effect("omits absent optional fields so the metadata bag holds no undefined values", () => + Effect.gen(function* () { + assertions.length = 0 + const registry = yield* ToolRegistry.Service + yield* executeTool(registry, call({ pattern: "needle" })) + + expect(assertions).toHaveLength(1) + const metadata = assertions[0].metadata! + expect(metadata).toEqual({ root: "." }) + // toEqual ignores undefined-valued keys, so guard the actual keys directly: + // undefined entries break JSON encoding of session.permission.list. + expect(Object.keys(metadata)).toEqual(["root"]) + }), + ) + + it.effect("keeps provided optional fields", () => + Effect.gen(function* () { + assertions.length = 0 + const registry = yield* ToolRegistry.Service + yield* executeTool(registry, call({ pattern: "needle", path: "src", include: "*.ts", limit: 5 })) + + expect(assertions[0].metadata).toEqual({ root: ".", path: "src", include: "*.ts", limit: 5 }) + }), + ) +})