From a38cbd42aaaba5c004163174870b9faa13e795d3 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Fri, 28 Aug 2026 23:17:48 -0400 Subject: [PATCH] refactor(core): isolate shell tool preparation Name the tool-owned pre-spawn preparation boundary while preserving hook edits, permission ordering, directory validation, and effective timeout reporting. Strengthen the existing regression assertions. --- packages/core/src/tool/plugin/shell.ts | 103 +++++++++++++------------ packages/core/test/tool-shell.test.ts | 53 ++++++++++--- 2 files changed, 96 insertions(+), 60 deletions(-) diff --git a/packages/core/src/tool/plugin/shell.ts b/packages/core/src/tool/plugin/shell.ts index 2a240d63251..70464c906b9 100644 --- a/packages/core/src/tool/plugin/shell.ts +++ b/packages/core/src/tool/plugin/shell.ts @@ -2,6 +2,8 @@ export * as ShellTool from "./shell.js" import { ToolFailure } from "@opencode-ai/ai" import type { Context } from "@opencode-ai/plugin/effect/plugin" +import type { ShellCreateBefore } from "@opencode-ai/plugin/effect/shell" +import type { Tool } from "@opencode-ai/schema/tool" import { Deferred, Effect, Schema, Scope } from "effect" import { Config } from "../../config.js" import { Environment } from "../../environment/index.js" @@ -107,6 +109,56 @@ export const Plugin = { const permission = yield* Permission.Service const config = yield* Config.Service + const prepare = Effect.fn("ShellTool.prepare")(function* (invocation: ShellCreateBefore, context: Tool.Context) { + const source = { + type: "tool" as const, + messageID: context.messageID, + id: context.id, + } + const target = yield* mutation.resolve({ path: invocation.cwd, kind: "directory" }) + invocation.cwd = target.absolute + const timeout = invocation.timeout + const portable = Config.latest(yield* config.entries(), "experimental")?.portable_shell_scanner === true + const parsed = yield* ShellParse.scan(invocation.command, invocation.shell, target.absolute, { portable }) + const directories = yield* Effect.forEach(parsed.directories, (directory) => + mutation.resolve({ + path: LocationMutation.resolvePath(target.absolute, directory), + kind: "directory", + }), + ) + const external = [target, ...directories] + .map((item) => item.externalDirectory) + .filter((item) => item !== undefined) + .filter((item, index, items) => items.findIndex((other) => other.resource === item.resource) === index) + if (external.length > 0) + yield* permission.assert({ + action: "external_directory", + resources: external.map((item) => item.resource), + save: external.map((item) => item.save), + sessionID: context.sessionID, + agent: context.agent, + source, + }) + if (parsed.commands.length > 0) + yield* permission.assert({ + action: name, + resources: parsed.commands.map((command) => command.resource), + save: parsed.commands.map((command) => command.save), + sessionID: context.sessionID, + agent: context.agent, + source, + }) + // Approval can outlive the directory, so validate immediately before spawning. + const workdir = yield* Environment.typeFollowing(environment.files, target.absolute).pipe( + Effect.catchTag("Environment.NotFound", () => + Effect.fail(new Error(`Working directory does not exist: ${target.absolute}`)), + ), + ) + if (workdir !== "directory") + return yield* Effect.fail(new Error(`Working directory is not a directory: ${target.absolute}`)) + return timeout + }) + const notifyWhenDone = Effect.fn("ShellTool.notifyWhenDone")( function* ( sessionID: SessionSchema.ID, @@ -151,11 +203,6 @@ export const Plugin = { output: Output, execute: (input, context) => Effect.gen(function* () { - const source = { - type: "tool" as const, - messageID: context.messageID, - id: context.id, - } const timeout = input.background === true ? (input.timeout ?? 0) : (input.timeout ?? DEFAULT_TIMEOUT_MS) let finalTimeout = timeout const info = yield* shell.create( @@ -168,51 +215,7 @@ export const Plugin = { }, (invocation) => Effect.gen(function* () { - const target = yield* mutation.resolve({ path: invocation.cwd, kind: "directory" }) - invocation.cwd = target.absolute - finalTimeout = invocation.timeout - const portable = - Config.latest(yield* config.entries(), "experimental")?.portable_shell_scanner === true - const parsed = yield* ShellParse.scan(invocation.command, invocation.shell, target.absolute, { - portable, - }) - const directories = yield* Effect.forEach(parsed.directories, (directory) => - mutation.resolve({ - path: LocationMutation.resolvePath(target.absolute, directory), - kind: "directory", - }), - ) - const external = [target, ...directories] - .map((item) => item.externalDirectory) - .filter((item) => item !== undefined) - .filter( - (item, index, items) => items.findIndex((other) => other.resource === item.resource) === index, - ) - if (external.length > 0) - yield* permission.assert({ - action: "external_directory", - resources: external.map((item) => item.resource), - save: external.map((item) => item.save), - sessionID: context.sessionID, - agent: context.agent, - source, - }) - if (parsed.commands.length > 0) - yield* permission.assert({ - action: name, - resources: parsed.commands.map((command) => command.resource), - save: parsed.commands.map((command) => command.save), - sessionID: context.sessionID, - agent: context.agent, - source, - }) - const workdir = yield* Environment.typeFollowing(environment.files, target.absolute).pipe( - Effect.catchTag("Environment.NotFound", () => - Effect.fail(new Error(`Working directory does not exist: ${target.absolute}`)), - ), - ) - if (workdir !== "directory") - return yield* Effect.fail(new Error(`Working directory is not a directory: ${target.absolute}`)) + finalTimeout = yield* prepare(invocation, context) }), ) yield* context.progress({ shellID: info.id }) diff --git a/packages/core/test/tool-shell.test.ts b/packages/core/test/tool-shell.test.ts index 36ceed7f802..7c9ebadbcf8 100644 --- a/packages/core/test/tool-shell.test.ts +++ b/packages/core/test/tool-shell.test.ts @@ -31,6 +31,7 @@ import { SessionStore } from "@opencode-ai/core/session/store" import { Permission } from "@opencode-ai/core/permission" import { PermissionSaved } from "@opencode-ai/core/permission/saved" import { PluginRuntime } from "@opencode-ai/core/plugin/runtime" +import { PluginHooks } from "@opencode-ai/core/plugin/hooks" import { PluginSupervisor } from "@opencode-ai/core/plugin/supervisor" import { Shell } from "@opencode-ai/core/shell" import { ShellSelect } from "@opencode-ai/core/shell/select" @@ -771,6 +772,8 @@ describe("ShellTool", () => { sessionID, action: "shell", resources: [isWindows ? "Start-Sleep -Milliseconds 100" : helloCommand], + agent: toolIdentity.agent, + source: { type: "tool", messageID: toolIdentity.messageID, id: "call-shell" }, }, ]) expect(assertions[0]?.save).toEqual([isWindows ? "Start-Sleep *" : "printf *"]) @@ -928,7 +931,15 @@ describe("ShellTool", () => { Effect.andThen( withSession(tmp.path, (registry) => executeTool(registry, call({ command: cwdCommand, workdir: "src" }))), ), - Effect.andThen(Effect.sync(() => expect(assertions.map((input) => input.action)).toEqual(["shell"]))), + Effect.andThen((settled) => + Effect.sync(() => { + expect(settled).toMatchObject({ + status: "error", + error: { message: `Working directory is not a directory: ${workdir}` }, + }) + expect(assertions.map((input) => input.action)).toEqual(["shell"]) + }), + ), ) }, (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]().then(() => undefined)), @@ -964,23 +975,26 @@ describe("ShellTool", () => { ) it.live( - "approves an external directory used by a directory-change command", + "deduplicates external directory approvals across workdir and directory-change commands", () => Effect.acquireUseRelease( Effect.promise(() => Promise.all([tmpdir(), tmpdir()])), ([active, outside]) => { - reset() const command = isWindows ? `Set-Location -LiteralPath '${outside.path}'; (Get-Location).Path` : `cd '${outside.path}' && pwd` return withSession(active.path, (registry) => - executeTool(registry, call({ command }, "call-external-cd")), - ).pipe( - Effect.andThen( - Effect.sync(() => { + Effect.forEach([{ command }, { command, workdir: outside.path }], (input) => + Effect.gen(function* () { + reset() + const settled = yield* executeTool(registry, call(input, "call-external-cd")) + expect(settled).toMatchObject({ status: "completed" }) expect(assertions.map((item) => item.action)).toEqual(["external_directory", "shell"]) expect(assertions[0]).toMatchObject({ resources: [path.join(realpathSync(outside.path), "*").replaceAll("\\", "/")], + sessionID, + agent: toolIdentity.agent, + source: { type: "tool", messageID: toolIdentity.messageID, id: "call-external-cd" }, }) }), ), @@ -1279,21 +1293,40 @@ describe("ShellTool", () => { ) it.live( - "returns a useful timeout outcome", + "authorizes the hook-edited command and workdir and reports its timeout", () => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), (tmp) => { reset() + const timeout = isWindows ? 3_000 : 500 return withSession(tmp.path, (registry) => - executeTool(registry, call({ command: timeoutOutputCommand, timeout: isWindows ? 3_000 : 500 })), + Effect.gen(function* () { + const hooks = yield* PluginHooks.Service + yield* hooks.register("shell", "create.before", (invocation) => + Effect.sync(() => { + invocation.command = timeoutOutputCommand + invocation.cwd = tmp.path + invocation.timeout = timeout + }), + ) + return yield* executeTool(registry, call({ command: helloCommand, workdir: "missing", timeout: 60_000 })) + }), ).pipe( Effect.andThen((settled) => Effect.sync(() => { expect(settled.metadata).toMatchObject({ timeout: true, truncated: false }) expect(settled.metadata).not.toHaveProperty("exit") - expect(settled.content?.[0]).toMatchObject(Expected.text(expect.stringContaining("before timeout"))) + const content = settled.content?.[0] + expect(content?.type).toBe("text") + if (content?.type !== "text") throw new Error("Expected text content") + expect(content.text).toContain("before timeout") + expect(content.text).toContain(`Command exceeded timeout of ${timeout} ms.`) expect(settled.content?.[1]).toMatchObject(Expected.text(expect.stringContaining("Command timed out"))) + expect(assertions.map((input) => input.action)).toEqual(["shell"]) + expect(assertions[0]?.resources).toEqual( + isWindows ? [idleCommand] : ["printf 'before timeout'", idleCommand], + ) }), ), )