fix(core): simplify shell execution boundary (#39530)

This commit is contained in:
Aiden Cline 2026-07-29 11:16:45 -05:00 committed by GitHub
parent 247f14f955
commit 813c41ff6c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 12 additions and 62 deletions

View file

@ -173,7 +173,7 @@ export function args(file: string, command: string) {
if (n === "nu" || n === "fish") return ["-c", command]
if (n === "zsh" || n === "bash") return ["-c", command]
if (n === "cmd") return ["/c", command]
if (ps(file)) return ["-NoProfile", "-Command", command]
if (ps(file)) return ["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", command]
return ["-c", command]
}

View file

@ -1,6 +1,5 @@
export * as ShellTool from "./shell"
import path from "path"
import { ToolFailure } from "@opencode-ai/ai"
import type { Content } from "@opencode-ai/schema/tool"
import type { Context as PluginContext } from "@opencode-ai/plugin/effect/plugin"
@ -66,18 +65,14 @@ const Output = Schema.Struct({
...StructuredOutput.fields,
output: Schema.String,
status: Schema.optionalKey(Schema.Literals(["completed", "running"])),
warnings: Schema.optionalKey(Schema.Array(Schema.String)),
})
type Output = typeof Output.Type
const modelOutput = (output: Output): string | undefined => {
const warnings = output.warnings?.length
? `\n\nWarnings:\n${output.warnings.map((warning) => `- ${warning}`).join("\n")}`
: ""
if (output.status === "running") return `${warnings.trimStart()}${warnings ? "\n\n" : ""}${BACKGROUND_INSTRUCTION}`
if (output.timeout) return `${warnings.trimStart()}${warnings ? "\n\n" : ""}Command timed out before completion.`
return `${warnings.trimStart()}${warnings ? "\n\n" : ""}Command exited with code ${output.exit}.`
if (output.status === "running") return BACKGROUND_INSTRUCTION
if (output.timeout) return "Command timed out before completion."
return `Command exited with code ${output.exit}.`
}
/**
@ -86,31 +81,12 @@ const modelOutput = (output: Output): string | undefined => {
*/
// TODO: Port tree-sitter bash / PowerShell parser-based approval reduction.
// TODO: Port BashArity reusable command-prefix approvals.
// TODO: Replace token-based command-argument external-directory advisories with parser-based detection.
// TODO: Add plugin shell.env environment augmentation once plugin hooks exist.
// TODO: Persist job status and define restart recovery before exposing remote observation.
// TODO: Add HTTP job observation only after durable status, restart recovery, and authorization are defined.
// TODO: Revisit process-group cleanup and platform coverage with shell-specific tests if current AppProcess semantics do not fully cover it.
// TODO: Revisit binary output handling if stdout/stderr decoding is text-only.
const shellTokens = (command: string) => command.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) ?? []
const unquote = (value: string) => value.replace(/^(['"])(.*)\1$/, "$2")
const externalCommandDirectories = Effect.fn("ShellTool.externalCommandDirectories")(function* (
fs: FSUtil.Interface,
command: string,
cwd: string,
) {
const directories = new Set<string>()
for (const token of shellTokens(command)) {
const value = unquote(token).replace(/[;,|&]+$/, "")
if (!path.isAbsolute(value)) continue
const resolved = yield* fs.resolve(value)
if (FSUtil.contains(cwd, resolved)) continue
directories.add(yield* fs.resolve(path.dirname(resolved)))
}
return [...directories]
})
export const Plugin = {
id: "opencode.tool.shell",
effect: Effect.fn("ShellTool.Plugin")(function* (ctx: PluginContext) {
@ -179,10 +155,6 @@ export const Plugin = {
agent: context.agent,
source,
})
const warnings = (yield* externalCommandDirectories(fsUtil, input.command, target.canonical)).map(
(directory) =>
`Command argument references external directory ${path.join(directory, "*").replaceAll("\\", "/")}. Shell runs with host-user filesystem, process, and network authority; this scan is advisory only.`,
)
yield* permission.assert({
action: name,
resources: [input.command],
@ -264,7 +236,6 @@ export const Plugin = {
shellID: info.id,
truncated: false,
status: "running" as const,
...(warnings.length ? { warnings } : {}),
}
}
@ -279,14 +250,13 @@ export const Plugin = {
shellID: info.id,
truncated: false,
status: "running" as const,
...(warnings.length ? { warnings } : {}),
}
}
if (result?.info.status === "error")
return yield* Effect.fail(new Error(result.info.error ?? "Command failed"))
if (result?.info.status === "cancelled") return yield* Effect.fail(new Error("Command cancelled"))
return { ...(yield* Deferred.await(settled)), ...(warnings.length ? { warnings } : {}) }
return yield* Deferred.await(settled)
}).pipe(
Effect.map((output) => {
const content: Array<Content> = [{ type: "text", text: output.output }]

View file

@ -59,6 +59,13 @@ describe("shell", () => {
expect(ShellSelect.args("/usr/bin/fish", "echo hi")).toEqual(["-c", "echo hi"])
expect(ShellSelect.args("/bin/zsh", "echo hi")).toEqual(["-c", "echo hi"])
expect(ShellSelect.args("/bin/bash", "echo hi")).toEqual(["-c", "echo hi"])
expect(ShellSelect.args("pwsh", "Write-Output hi")).toEqual([
"-NoLogo",
"-NoProfile",
"-NonInteractive",
"-Command",
"Write-Output hi",
])
})
if (process.platform === "win32") {

View file

@ -346,33 +346,6 @@ describe("ShellTool", () => {
),
)
it.live("reports external command arguments as advisory warnings without enforcing approval", () =>
Effect.acquireUseRelease(
Effect.promise(() => Promise.all([tmpdir(), tmpdir()])),
([active, outside]) => {
reset()
denyAction = "external_directory"
const target = path.join(outside.path, "secret.txt")
return withSession(active.path, (registry) => executeTool(registry, call({ command: `cat ${target}` }))).pipe(
Effect.andThen((settled) =>
Effect.sync(() => {
expect(assertions.map((item) => item.action)).toEqual(["shell"])
expect(settled.metadata).not.toHaveProperty("warnings")
expect(settled.content?.[1]).toMatchObject({
type: "text",
text: expect.stringContaining("Warnings:"),
})
}),
),
)
},
([active, outside]) =>
Effect.promise(() =>
Promise.all([active[Symbol.asyncDispose](), outside[Symbol.asyncDispose]()]).then(() => undefined),
),
),
)
it.live("keeps non-zero exits useful", () =>
Effect.acquireUseRelease(
Effect.promise(() => tmpdir()),