refactor(core): detect external command paths with a bash parser

packages/core/package.json

@@ -117,6 +117,7 @@
     "jsonc-parser": "3.3.1",
     "semver": "^7.6.3",
     "turndown": "7.2.0",
+    "unbash": "4.0.3",
     "venice-ai-sdk-provider": "2.1.1",
     "which": "6.0.1",
     "zod": "catalog:"

packages/core/src/tool/shell.ts

@@ -11,6 +11,7 @@ import { PluginRuntime } from "../plugin/runtime"
 import { NonNegativeInt } from "../schema"
 import { SessionSchema } from "../session/schema"
 import { Shell } from "../shell"
+import { Bash } from "../util/bash"
 import { Tool, type Content } from "./tool"
 
 export const name = "shell"
@@ -69,7 +70,6 @@ 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: Restore PowerShell and cmd-specific invocation/path handling on Windows.
 // TODO: Add plugin shell.env environment augmentation once V2 plugin hooks exist.
 // TODO: Persist job status and define restart recovery before exposing remote observation.
@@ -78,16 +78,13 @@ const modelOutput = (output: Output): string | undefined => {
 // TODO: Revisit binary output handling if stdout/stderr decoding is text-only.
 // TODO: Stream full shell output into managed storage while retaining only a bounded in-memory preview.
 
-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(/[;,|&]+$/, "")
+  for (const value of Bash.pathWords(command)) {
     if (!path.isAbsolute(value)) continue
     const resolved = yield* fs.resolve(value)
     if (FSUtil.contains(cwd, resolved)) continue

packages/core/src/util/bash.ts

@@ -0,0 +1,45 @@
+export * as Bash from "./bash"
+
+import { parse } from "unbash"
+import type { Word } from "unbash"
+
+/**
+ * Filesystem paths a bash command references, dequoted and unescaped. Every parsed word is a
+ * candidate because the shell expands them all; callers filter with `path.isAbsolute`, which
+ * already discards descriptors (`2>&1`), heredoc delimiters and unexpanded parameters.
+ */
+export function pathWords(command: string) {
+  return [...walk(parse(command))]
+}
+
+/**
+ * unbash resolves these lazily, as prototype accessors rather than own properties, so walking
+ * `Object.entries` alone silently skips every path nested under them.
+ */
+const LAZY = ["parts", "expression", "initialize", "test", "update"]
+
+function* walk(node: unknown): Generator<string> {
+  if (Array.isArray(node)) {
+    for (const item of node) yield* walk(item)
+    return
+  }
+  if (!node || typeof node !== "object") return
+  if (isWord(node)) yield node.value
+  const fields = node as Record<string, unknown>
+  for (const [key, child] of Object.entries(fields)) {
+    // A heredoc body is data rather than a path, though its expansions still run.
+    if (key === "body" && typeof fields.operator === "string" && isWord(child)) {
+      yield* walk(child.parts)
+      continue
+    }
+    yield* walk(child)
+  }
+  for (const key of LAZY) if (!Object.hasOwn(fields, key)) yield* walk(fields[key])
+}
+
+/** Words carry a source position; the expansion parts nested inside them do not. */
+function isWord(node: unknown): node is Word {
+  if (!node || typeof node !== "object") return false
+  const fields = node as Record<string, unknown>
+  return typeof fields.value === "string" && typeof fields.pos === "number"
+}

packages/core/test/util/bash.test.ts

@@ -0,0 +1,108 @@
+import { describe, expect, test } from "bun:test"
+import path from "path"
+import { Bash } from "@opencode-ai/core/util/bash"
+
+/**
+ * The shell tool only acts on absolute words, and collects them into a set, so assert against
+ * the same slice it sees without depending on traversal order or repeats.
+ */
+function absolute(command: string) {
+  return [...new Set(Bash.pathWords(command).filter((value) => path.isAbsolute(value)))].sort()
+}
+
+describe("util.bash", () => {
+  test("reads arguments, command names, and separators", () => {
+    expect(absolute("cat /outside/x")).toEqual(["/outside/x"])
+    expect(absolute("cat /outside/x;")).toEqual(["/outside/x"])
+    expect(absolute("/outside/bin/tool")).toEqual(["/outside/bin/tool"])
+    expect(absolute("cat /outside/a && cat /outside/b")).toEqual(["/outside/a", "/outside/b"])
+  })
+
+  test("dequotes and unescapes paths containing spaces", () => {
+    expect(absolute("cat /outside/my\\ dir/file")).toEqual(["/outside/my dir/file"])
+    expect(absolute(`cat /outside/"my dir"/file`)).toEqual(["/outside/my dir/file"])
+    expect(absolute(`cat '/outside/my dir/file'`)).toEqual(["/outside/my dir/file"])
+  })
+
+  test("ignores comments", () => {
+    expect(absolute("echo ok # /outside/secret/x")).toEqual([])
+  })
+
+  test("leaves unexpanded parameters relative but keeps literal prefixes", () => {
+    expect(absolute("cat $HOME/x")).toEqual([])
+    expect(absolute("cat ${HOME}/x")).toEqual([])
+    expect(absolute("cat /outside/$VAR")).toEqual(["/outside/$VAR"])
+  })
+
+  test("reads redirect targets, including without a space", () => {
+    expect(absolute("echo hi > /outside/out.txt")).toEqual(["/outside/out.txt"])
+    expect(absolute("echo hi >/outside/out.txt")).toEqual(["/outside/out.txt"])
+    expect(absolute("cat < /outside/in.txt")).toEqual(["/outside/in.txt"])
+    expect(absolute("{ echo hi; } > /outside/brace/out")).toEqual(["/outside/brace/out"])
+  })
+
+  test("separates the file and descriptor forms of >&", () => {
+    expect(absolute("echo hi &> /outside/out.txt")).toEqual(["/outside/out.txt"])
+    expect(absolute("echo hi >& /outside/out.txt")).toEqual(["/outside/out.txt"])
+    expect(absolute("echo hi 2>&1")).toEqual([])
+    expect(absolute("echo hi >&-")).toEqual([])
+  })
+
+  test("reads assignment values and their substitutions", () => {
+    expect(absolute("RESULT=$(cat /outside/secret/x)")).toEqual(["/outside/secret/x"])
+    expect(absolute("FOO=/outside/assign/x cat y")).toEqual(["/outside/assign/x"])
+  })
+
+  test("reads parameter expansion defaults", () => {
+    expect(absolute(`cat "${"${FILE:-/outside/default/x}"}"`)).toEqual(["/outside/default/x"])
+    expect(absolute("cat ${FILE:=/outside/default/x}")).toEqual(["/outside/default/x"])
+  })
+
+  test("reads array assignment elements", () => {
+    expect(absolute("FILES=( /outside/a /outside/b )")).toEqual(["/outside/a", "/outside/b"])
+  })
+
+  test("descends into substitutions nested in expansions and arithmetic", () => {
+    expect(absolute("cat ${value:$(cat /outside/slice/x)}")).toEqual(["/outside/slice/x"])
+    expect(absolute("cat ${value//a/$(cat /outside/repl/x)}")).toEqual(["/outside/repl/x"])
+    expect(absolute("echo $(( $(cat /outside/arith/x) ))")).toEqual(["/outside/arith/x"])
+    expect(absolute("(( $(cat /outside/arith/x) ))")).toEqual(["/outside/arith/x"])
+    expect(absolute("for (( i=$(cat /outside/init/x); i<2; i++ )); do echo hi; done")).toEqual(["/outside/init/x"])
+  })
+
+  test("treats heredoc delimiters and bodies as data, not paths", () => {
+    expect(absolute("cat <<EOF\n/outside/heredoc/x\nEOF")).toEqual([])
+    expect(absolute("cat <<EOF\n$(ls /outside/nested)\nEOF")).toEqual(["/outside/nested"])
+  })
+
+  test("descends into substitutions", () => {
+    expect(absolute("cat $(ls /outside/nested)")).toEqual(["/outside/nested"])
+    expect(absolute("diff <(cat /outside/a) <(cat /outside/b)")).toEqual(["/outside/a", "/outside/b"])
+  })
+
+  // A quoted `sh -c` payload stays one opaque word; reading inside it means knowing which
+  // programs re-interpret an argument as a script, which is a separate question from parsing.
+  test("reads a shell invocation as plain words", () => {
+    expect(absolute(`sh -c "cat /outside/secret/x"`)).toEqual([])
+    expect(absolute(`/bin/bash -lc "cat /outside/secret/x"`)).toEqual(["/bin/bash"])
+  })
+
+  test("reads compound and test constructs", () => {
+    expect(absolute("for f in /outside/a /outside/b; do cat $f; done")).toEqual(["/outside/a", "/outside/b"])
+    expect(absolute("[ -f /outside/test/x ]")).toEqual(["/outside/test/x"])
+    expect(absolute("[[ -f /outside/test/x ]]")).toEqual(["/outside/test/x"])
+    expect(absolute("case /outside/x in /outside/*) cat /outside/y;; esac")).toEqual([
+      "/outside/*",
+      "/outside/x",
+      "/outside/y",
+    ])
+    expect(absolute("while cat /outside/w; do cat /outside/body; done")).toEqual(["/outside/body", "/outside/w"])
+    expect(absolute("if cat /outside/if; then cat /outside/then; else cat /outside/else; fi")).toEqual([
+      "/outside/else",
+      "/outside/if",
+      "/outside/then",
+    ])
+    expect(absolute("f() { cat /outside/fn; }")).toEqual(["/outside/fn"])
+    expect(absolute("cat /outside/a | grep /outside/b")).toEqual(["/outside/a", "/outside/b"])
+  })
+})