mirror of
https://github.com/anomalyco/opencode.git
synced 2026-08-29 15:32:11 +00:00
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.
This commit is contained in:
Kit Langton
GitHub
parent
4ab31867c4
commit
a38cbd42aa
2 changed files with 96 additions and 60 deletions
|
|
@ -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 })
|
||||
|
|
|
|||
|
|
@ -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],
|
||||
)
|
||||
}),
|
||||
),
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue