fix(cli): stop inline skill-shell doc examples from triggering permission prompts

packages/opencode/src/kilocode/skills/inject.ts

@@ -4,32 +4,15 @@ import { Process } from "@/util/process"
 import { SKILL_SHELL_DISABLED, SKILL_SHELL_UNTRUSTED } from "@/kilocode/skills/display"
 import type * as Tool from "@/tool/tool"
 
-// Shell injection for skill bodies mirrors Claude's "dynamic context injection":
-// a `!`cmd`` placeholder in SKILL.md is replaced by the command's stdout before
-// the content reaches the model. Unlike the slash-command path, this runs for
-// model-initiated skill loads, so it is gated on three independent controls:
-//
-//   1. Trust: only skills from trusted sources (global ~/.claude, ~/.agents,
-//      KILO_CONFIG_DIR, and builtins) may execute. Untrusted project/downloaded
-//      skills never spawn a process.
-//   2. Kill-switch: `disabled` (KILO_DISABLE_SKILL_SHELL) turns injection off
-//      entirely, matching Claude's disableSkillShellExecution.
-//   3. Batch approval: every command in the file is decomposed with the same
-//      tree-sitter scan the bash tool uses (per sub-command patterns plus any
-//      out-of-project directories), then presented once, up front, as a single
-//      bash permission prompt naming every command — plus a separate, preceding
-//      external_directory prompt if any command touches a directory outside the
-//      project. The `skillShell` marker forces both prompts regardless of any
-//      allow/auto-approve rule; a deny rule or plan-mode veto on any sub-command
-//      still blocks. Approving both runs the batch; rejecting either aborts the load.
-//
-// Trust and the kill-switch also gate the slash-command path (`/skill`, session/prompt.ts),
-// which is user-initiated. Batch approval (control 3) is specific to this model-initiated
-// tool path — the slash-command path is not prompted because the user invoked it directly.
-//
-// Substitution runs exactly once. Command output is inlined as plain text and is
-// never re-scanned, so a command cannot emit a `!`cmd`` placeholder that a later
-// pass would execute (second-order injection).
+// Shell injection for skill bodies mirrors Claude's "dynamic context injection": a
+// `!`cmd`` placeholder in SKILL.md is replaced by the command's stdout before the
+// content reaches the model. Runs only for model-initiated skill loads (not the
+// user-initiated `/skill` path), gated by: trust (only global/builtin skills, never
+// project/downloaded ones), a kill-switch (KILO_DISABLE_SKILL_SHELL), and one batch
+// bash permission ask naming every command up front (`skillShell` forces the ask past
+// any allow/auto-approve rule; a preceding external_directory ask covers out-of-project
+// paths). Substitution runs once; output is never re-scanned, so a command can't emit
+// a placeholder that a later pass would execute.
 
 // Execution bounds: model-initiated commands must not hang the load, blow up
 // context, or overrun the batch.
@@ -58,32 +41,24 @@ export namespace SkillInject {
   }
 
   export const render = Effect.fn("SkillInject.render")(function* (opts: Options) {
-    // Placeholders inside fenced code blocks are documentation examples, not live commands.
-    const fenced = fences(opts.content)
-    const live = ConfigMarkdown.shell(opts.content).filter((m) => !fenced(m.index))
+    // Fenced blocks and inline code spans (`` !`cmd` ``) are documentation, not live commands.
+    const inert = ranges(opts.content)
+    const live = ConfigMarkdown.shell(opts.content).filter((m) => !inert(m.index))
     if (live.length === 0) return opts.content
 
-    // Defense-in-depth ordering: policy checks first, approval gate last. `replace` only
-    // rewrites live (unfenced) placeholders; fenced ones stay as literal text.
-    const replace = (value: (command: string) => string) => rewrite(opts.content, fenced, value)
+    // Policy checks before the approval gate; `replace` only touches live placeholders.
+    const replace = (value: (command: string) => string) => rewrite(opts.content, inert, value)
     if (opts.disabled) return replace(() => SKILL_SHELL_DISABLED)
     if (!opts.trusted) return replace(() => SKILL_SHELL_UNTRUSTED)
 
-    // `shell` is resolved by the caller via Shell.acceptable(cfg.shell), which
-    // rejects shells the tree-sitter bash scanner can't parse (fish/nu), keeping
-    // the parse used for the permission decision aligned with execution.
     const shell = opts.shell
-    // Deduplicate identical commands, then cap the batch so a skill can't queue
-    // an unbounded number of processes.
+    // Dedupe, then cap so a skill can't queue an unbounded number of processes.
     const commands = Array.from(new Set(live.map(([, cmd]) => cmd))).slice(0, MAX_COMMANDS)
 
-    // Decompose each command into sub-command patterns + out-of-project dir globs
-    // via the shared bash scan, so plan-mode denies and external_directory checks
-    // apply per sub-command instead of matching the raw string as one glob. Also
-    // authorize the verbatim command: decomposition drops cd/set-location segments
-    // and strips chaining metacharacters, so a payload like `cd $HOME; cat secret`
-    // would otherwise slip past the metachar deny rules (`*;*`, `*|*`, `*\n*`) and
-    // hide the escape. Keeping the raw string as a pattern makes those rules fire.
+    // Decompose each command into sub-command patterns + out-of-project dirs via the shared
+    // bash scan, so deny/plan-mode rules and external_directory checks apply per sub-command.
+    // Also authorize the verbatim string: decomposition drops cd/chaining metacharacters, so
+    // `cd $HOME; cat secret` would otherwise dodge the `*;*`/`*|*`/`*\n*` deny rules.
     const patterns = new Set<string>()
     const dirs = new Set<string>()
     for (const command of commands) {
@@ -93,19 +68,13 @@ export namespace SkillInject {
       for (const dir of scan.dirs) dirs.add(dir)
     }
 
-    // Fail closed: an empty pattern set would make the bash ask below auto-approve
-    // (Permission.ask iterates patterns, so forceAsk/veto never run for an empty
-    // list). Each command contributes its verbatim string above, so this is
-    // unreachable — but abort rather than risk a silent, unprompted execution.
+    // Fail closed: an empty pattern set would auto-approve the ask below. Unreachable since
+    // each command adds its own string above, but abort rather than risk silent execution.
     if (patterns.size === 0) return yield* Effect.die(new Error("skill shell produced no authorizable commands"))
 
-    // Up-front approval before any command runs: a bash ask naming every command, preceded
-    // by a separate external_directory ask when a sub-command touches a directory outside the
-    // project (below). `patterns` are the decomposed sub-commands used for rule matching;
-    // `metadata.commands` is the verbatim per-placeholder list the prompt displays, so what is
-    // shown is exactly what runs (decomposition drops cd/set-location segments and splits
-    // pipelines, which must not hide from the user). `skillShell` forces both prompts over
-    // allow/YOLO rules; a deny/veto on any sub-command propagates as a defect and aborts.
+    // One up-front bash ask naming every command (metadata.commands is the verbatim list
+    // shown, since decomposition can drop/split segments); external_directory asks first if
+    // any command leaves the project. `skillShell` forces both asks past allow/YOLO rules.
     const metadata = { skillShell: true, skill: opts.skill, commands }
     if (dirs.size > 0) {
       yield* opts.ctx.ask({
@@ -122,9 +91,7 @@ export namespace SkillInject {
       metadata,
     })
 
-    // Run each command in the instance directory, bounded per-command by ctx.abort (ESC)
-    // and a timeout, and across the batch by an aggregate wall-clock budget, with output
-    // truncated so it can't blow up or poison the prompt.
+    // Run each command, bounded per-command by ctx.abort/timeout and by an aggregate budget.
     const outputs = new Map<string, string>()
     const deadline = Date.now() + BUDGET_MS
     for (const command of commands) {
@@ -135,27 +102,23 @@ export namespace SkillInject {
       outputs.set(command, yield* run(command, shell, opts.cwd, opts.ctx.abort))
     }
 
-    // A placeholder that was capped out of `commands` isn't in `outputs`; mark it rather
-    // than silently inlining an empty string.
+    // Mark commands capped out of `commands` rather than silently inlining "".
     return replace((command) => outputs.get(command) ?? LIMIT_NOTE)
   })
 
   const run = Effect.fn("SkillInject.run")(function* (command: string, shell: string, cwd: string, abort: AbortSignal) {
     const timeout = new AbortController()
-    // A cleared timer bounds the run without leaking a pending 2-minute timeout per command;
-    // ESC (ctx.abort) still kills the child via the same combined signal.
     const signal = AbortSignal.any([abort, timeout.signal])
     const timer = setTimeout(() => timeout.abort(), TIMEOUT_MS)
     const result = yield* Effect.promise(() =>
       Process.text([command], { shell, cwd, abort: signal, nothrow: true }).catch(() => undefined),
     ).pipe(Effect.ensuring(Effect.sync(() => clearTimeout(timer))))
 
-    // With nothrow the promise resolves even when the child was killed, inlining partial
-    // stdout; detect the kill via the signals so an aborted/timed-out command is marked.
+    // nothrow resolves even on kill, inlining partial stdout; check the signals to mark it.
     if (abort.aborted) return "[skill shell command aborted]"
     if (timeout.signal.aborted) return "[skill shell command timed out]"
     if (!result) return "[skill shell command failed]"
-    // A failing command with empty stdout would inline ""; surface a marker with any stderr.
+    // Empty stdout on failure would inline ""; surface a marker with any stderr instead.
     if (result.code !== 0 && result.text.length === 0) {
       const err = result.stderr.toString().trim()
       return err ? "[skill shell command failed]\n" + truncate(err) : "[skill shell command failed]"
@@ -170,32 +133,88 @@ export namespace SkillInject {
     return buf.toString("utf8", 0, MAX_OUTPUT_BYTES) + "\n[skill shell output truncated]"
   }
 
-  // Rewrite only live (unfenced) placeholders in the ORIGINAL content, substituting once and
-  // never re-scanning the result, so inlined output containing `!`cmd`` stays inert and a
-  // fenced documentation example is left as literal text.
-  function rewrite(content: string, fenced: (index: number) => boolean, value: (command: string) => string) {
+  // Rewrites only live placeholders, once, in the original content — inlined output
+  // containing `!`cmd`` stays inert, and documentation examples stay literal text.
+  function rewrite(content: string, inert: (index: number) => boolean, value: (command: string) => string) {
     return content.replace(ConfigMarkdown.SHELL_REGEX, (match, command: string, index: number) =>
-      fenced(index) ? match : value(command),
+      inert(index) ? match : value(command),
     )
   }
 
-  // Return a predicate that reports whether a character offset falls inside a fenced code
-  // block (``` or ~~~), so placeholders in documentation examples are treated as inert.
+  // Predicate for offsets inside a fenced code block (``` or ~~~). `spans` comes out sorted
+  // and non-overlapping, so `within` can binary-search it.
   function fences(content: string): (index: number) => boolean {
-    const ranges: Array<[number, number]> = []
+    const spans: Array<[number, number]> = []
     const fence = /^[ \t]*(`{3,}|~{3,})[^\n]*$/gm
     let open: { start: number; marker: string } | undefined
     for (const m of content.matchAll(fence)) {
       const marker = m[1]
-      // CommonMark: a closing fence uses the same char and is at least as long as the opener,
-      // so an inner shorter/different fence stays content. Keep the real opener length.
+      // A closing fence uses the same char and is at least as long as the opener (CommonMark).
       if (!open) open = { start: m.index, marker }
       else if (marker[0] === open.marker[0] && marker.length >= open.marker.length) {
-        ranges.push([open.start, m.index + m[0].length])
+        spans.push([open.start, m.index + m[0].length])
         open = undefined
       }
     }
-    if (open) ranges.push([open.start, content.length]) // unterminated fence runs to EOF
-    return (index: number) => ranges.some(([s, e]) => index >= s && index < e)
+    if (open) spans.push([open.start, content.length]) // unterminated fence runs to EOF
+    return within(spans)
+  }
+
+  // Also treats inline code spans of 2+ backticks as inert: a single-backtick span can't
+  // contain a backtick, so a single-backtick pair nested in a longer run (e.g. `` !`cmd` ``)
+  // is always documentation, never a real placeholder. Pairing is scoped to one blank-line
+  // paragraph at a time — spans can't cross a blank line — so unrelated runs in different
+  // paragraphs can never merge into one range and swallow a real placeholder between them.
+  function ranges(content: string): (index: number) => boolean {
+    const fenced = fences(content)
+    const spans: Array<[number, number]> = []
+    for (const para of paragraphs(content)) {
+      const pending = new Map<number, number>() // run length -> start of its unmatched opener
+      for (const m of para.text.matchAll(/`+/g)) {
+        const start = para.start + m.index
+        if (fenced(start)) continue
+        const len = m[0].length
+        if (len < 2) continue
+        const open = pending.get(len)
+        if (open === undefined) {
+          pending.set(len, start)
+          continue
+        }
+        spans.push([open, start + len])
+        pending.delete(len)
+      }
+    }
+    // Spans close in resolution order, not start order, so sort before binary search.
+    spans.sort((a, b) => a[0] - b[0])
+    return (index: number) => fenced(index) || within(spans)(index)
+  }
+
+  // Splits content on blank lines, keeping each chunk's absolute start offset.
+  function paragraphs(content: string): Array<{ start: number; text: string }> {
+    const out: Array<{ start: number; text: string }> = []
+    const blank = /\n[ \t]*\n/g
+    let start = 0
+    for (const m of content.matchAll(blank)) {
+      out.push({ start, text: content.slice(start, m.index) })
+      start = m.index + m[0].length
+    }
+    out.push({ start, text: content.slice(start) })
+    return out
+  }
+
+  // Binary search over a sorted, non-overlapping [start, end) range list.
+  function within(spans: Array<[number, number]>): (index: number) => boolean {
+    return (index: number) => {
+      let lo = 0
+      let hi = spans.length - 1
+      while (lo <= hi) {
+        const mid = (lo + hi) >> 1
+        const [s, e] = spans[mid]
+        if (index < s) hi = mid - 1
+        else if (index >= e) lo = mid + 1
+        else return true
+      }
+      return false
+    }
   }
 }

packages/opencode/test/kilocode/skills/inject.test.ts

@@ -298,6 +298,79 @@ describe("skill shell injection", () => {
     }),
   )
 
+  unix("does not execute a placeholder shown as a double-backtick inline code example", () =>
+    Effect.gen(function* () {
+      // `` !`cmd` `` is the standard CommonMark way to display the literal `!`cmd`` syntax
+      // as documentation; only the live placeholder must run.
+      yield* writeGlobalSkill(
+        "inline-example-shell",
+        "Live: !`printf LIVE`\n\nSyntax: `` !`cmd` `` runs a command.",
+      )
+
+      const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
+      const result = yield* loadSkill("inline-example-shell", (req) =>
+        Effect.sync(() => {
+          requests.push(req)
+        }),
+      )
+
+      expect(result.output).toContain("Live: LIVE")
+      expect(result.output).toContain("Syntax: `` !`cmd` `` runs a command.")
+      const bash = requests.filter((r) => r.permission === "bash")
+      expect(bash[0]?.patterns).toEqual(["printf LIVE"])
+    }),
+  )
+
+  unix("does not ask or run anything for a skill with only an inline code example", () =>
+    Effect.gen(function* () {
+      // This is the real-world trigger: kilo-config.md documents the placeholder syntax
+      // with `` !`cmd` `` outside any fence, which must never request permission or run.
+      yield* writeGlobalSkill("doc-only-shell", "Template variables include `` !`cmd` `` (shell output).")
+
+      const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
+      const result = yield* loadSkill("doc-only-shell", (req) =>
+        Effect.sync(() => {
+          requests.push(req)
+        }),
+      )
+
+      expect(result.output).toContain("Template variables include `` !`cmd` `` (shell output).")
+      expect(requests.some((r) => r.permission === "bash")).toBe(false)
+    }),
+  )
+
+  unix("does not let distant unrelated inline code spans merge into one inert range", () =>
+    Effect.gen(function* () {
+      // Code spans cannot cross a blank line. Stray double-backticks in an earlier paragraph
+      // and a later, unrelated (unclosed) one must not pair across the live placeholder that
+      // sits between them and silently swallow it — that would skip both its execution and
+      // the marker that would otherwise flag a rejected/untrusted command.
+      const body = [
+        "Use the C++ operator `` and note the ``literal`` form.",
+        "",
+        "## Step 2",
+        "",
+        "!`printf LIVE`",
+        "",
+        "Done, see the output above.",
+        "",
+        "Trailing note about `` quoting.",
+      ].join("\n")
+      yield* writeGlobalSkill("distant-spans-shell", body)
+
+      const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
+      const result = yield* loadSkill("distant-spans-shell", (req) =>
+        Effect.sync(() => {
+          requests.push(req)
+        }),
+      )
+
+      expect(result.output).toContain("LIVE")
+      const bash = requests.filter((r) => r.permission === "bash")
+      expect(bash[0]?.patterns).toEqual(["printf LIVE"])
+    }),
+  )
+
   unix("does not re-execute shell placeholders emitted by command output", () =>
     Effect.gen(function* () {
       // The command emits a literal placeholder `!<backtick>echo pwned<backtick>`
@@ -372,6 +445,18 @@ describe("SkillInject.render gating", () => {
     }),
   )
 
+  it.effect("does not scale quadratically with fence and backtick-run count", () =>
+    Effect.gen(function* () {
+      // A pathological SKILL.md with many fences plus many short backtick runs previously
+      // took ~30s (O(runs x fences) fence lookups, O(runs^2) pairing). This content runs
+      // before the trust check, so it must stay bounded even for an untrusted skill.
+      const content = "```\n```\n".repeat(40000) + "`x ".repeat(120000) + "!`printf ran`"
+      const started = Date.now()
+      yield* Effect.promise(() => run({ trusted: false, disabled: false, content }))
+      expect(Date.now() - started).toBeLessThan(5000)
+    }),
+  )
+
   it.effect("content without placeholders is returned unchanged", () =>
     Effect.gen(function* () {
       const out = yield* Effect.promise(() => run({ trusted: true, disabled: false, content: "no commands here" }))