diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index da16a4412..2371c3b52 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -410,7 +410,7 @@ tool call - **Secret Guard** (`secret-guard-plugin.ts`) — Hard-denies path-keyed tool calls (`read_file`, `write_file`, …) that would put a sensitive file into (or write it from) the model context. Runs before the permission plugin, so the path-arg deny holds even under `--dangerously-skip-permissions`. An operator-chosen `--config` path cannot be covered by static patterns, so entry points pass the resolved active settings path as `extraDeniedPaths` (exact match, lexical plus realpath). Shell commands that _reference_ a sensitive path (tokenized so `cat .env`, `bun --env-file=.env run …`, and quote/env-assignment forms are detected) are not hard-denied here. Normal mode requires operator approval via the permission gate, and auto mode forces the same ask through the auto-shell policy (`sensitive-path` rule). Yolo/skip-permissions bypasses that prompt after catastrophic authorization checks, but path-keyed access to statically protected secret paths remains hard-denied. Shell detection is best-effort: token matching defeats quoting and env-assignment/redirection forms but not dynamic path construction (variable indirection, `printf` assembly). Tool-result secret scrub still redacts credential-shaped output. - **Authorization** (`run-shell-authz.ts`, enforced by the permission gate) — Denies catastrophic shell command patterns by regex, and hard-blocks shell `find`, head-position `rg`, and recursive `grep -r` (they can walk huge trees and OOM the host). Bounded `grep`/`search_files` tools remain practical alternatives (timeout + output caps); the patterns match those three command shapes only — an `ls -R`, `fd`, or scripted `os.walk` is just as unbounded and is not caught, so the block message tells the model not to substitute one. The gate hard-denies these at the top of its verdict path — before auto-allow, prompting, grants, and skipPermissions — so no mode or stored grant can admit them. - **Permission** (`permission-plugin.ts`) — Delegates consequential calls to the permission gate. -- **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): 120s foreground default (`settings.shell.timeoutMs` overrides that default only; a positive per-call `timeout` is the bound with no ceiling — `maxTimeoutMs` does not clamp it), background `run_shell` has no default (only a per-call timeout arms a timer), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout, abort, and plugin dispose (live children tracked in the plugin and reaped by `posixTools.dispose`), and `background: true` — the call returns a `shell_id` at once (registry in `src/shell/background-shell.ts`), the process group keeps running past the turn, completion is process-exit (stdio-close is not required), delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels with a 300s wait cap (schema advertised by `advertiseShellGuardTimeout` only when `shell_collect` is mounted; evaluated by the permission chain at start time like any shell call). Foreground expiry is exit 124 + `timedOut:true` plus a nudge to retry with `background:true`. Also applies a 10s wall-clock budget to `grep`/`search_files`. Ripgrep detached spawns are not tracked. +- **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): 120s foreground default (`settings.shell.timeoutMs` overrides that default only; a positive per-call `timeout` is the bound with no ceiling — `maxTimeoutMs` does not clamp it), background `run_shell` has no default (only a per-call timeout arms a timer), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout, abort, and plugin dispose (live children tracked in the plugin and reaped by `posixTools.dispose`), and `background: true` — the call returns a `shell_id` at once (registry in `src/shell/background-shell.ts`), the process group keeps running past the turn, completion is process-exit (stdio-close is not required), delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels with a 300s wait cap (schema advertised by `advertiseShellGuardTimeout` only when `shell_collect` is mounted; evaluated by the permission chain at start time like any shell call). Foreground expiry is exit 124 + `timedOut:true` plus a nudge to retry with `background:true`. Also applies a 10s wall-clock budget to `grep`/`search_files` (budget expiry and budget-torn-down aborts surface as explicit timeout errors, never as empty results; an unbounded workspace-root `search_files` walk is refused up front with scope guidance instead of running to the budget). Ripgrep detached spawns are not tracked. - **Read File Guard** (`read-file-guard-plugin.ts`) — Corbits Code-only short-circuit for `read_file` on real filesystem paths and configured `tool-output://` URIs (interchange stays unpatched): streaming reads that never decode the whole file in one pass, caps model-facing output at 50KB, defaults to 2000 lines, truncates long lines with recovery hints, samples the first chunk to reject binary, and stops at an 8MB scan ceiling. Emits `offset` continuation notices so the model can page without losing file or spill content on disk. - **Verify** (`verify-plugin.ts`) — Re-reads after `write_file` / `edit_file` and errors on mismatch. Per-path serialization (`file-mutation-lock.ts`) prevents parallel edits on one file from tripping verification. - **Edit file line range** (`edit-file-line-range-plugin.ts`) — Corbits Code-only short-circuit for `edit_file` mode B (`start_line`/`end_line`/`new_string`), same pattern as shell-guard; schema advertised via `advertiseEditFileLineRange`. Modes are mutually exclusive: a call supplying both `old_string` and `start_line`/`end_line` is rejected with a recoverable error naming which fields to omit (no file-content disambiguation). diff --git a/src/plugins/ripgrep-plugin.ts b/src/plugins/ripgrep-plugin.ts index 206931fee..8e0448ac5 100644 --- a/src/plugins/ripgrep-plugin.ts +++ b/src/plugins/ripgrep-plugin.ts @@ -14,6 +14,10 @@ import { type RgLimits, type SpawnRg, } from "./rg-run.js"; +import { + formatUnboundedSearchMessage, + isUnboundedRootSearch, +} from "./tool-time-budget.js"; import { createExtraDeniedPathMatcher } from "./secret-guard-plugin.js"; // A grep over a large tree with the pure-TypeScript walker enumerates the whole @@ -221,6 +225,21 @@ export function ripgrepPlugin( if (call.name === "search_files") { const pattern = str(call.arguments.pattern); if (pattern === undefined) return next(call, signal); + // Unbounded workspace-root walks hang until the shell-guard budget + // fires, and a timeout reads as a false negative. Refuse up front + // with scope guidance instead of starting a walk that cannot finish + // in budget. + const rawPath = + typeof call.arguments.path === "string" + ? call.arguments.path + : undefined; + if (isUnboundedRootSearch({ path: rawPath, pattern, cwd })) { + return { + callId: call.id, + content: formatUnboundedSearchMessage("search_files", pattern), + isError: true, + }; + } const path = str(call.arguments.path) ?? cwd; const maxResults = num(call.arguments.max_results) ?? DEFAULT_SEARCH_MAX; diff --git a/src/plugins/search-files-fail-closed.test.ts b/src/plugins/search-files-fail-closed.test.ts new file mode 100644 index 000000000..8ed160dc8 --- /dev/null +++ b/src/plugins/search-files-fail-closed.test.ts @@ -0,0 +1,280 @@ +import { describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createPosixTools } from "@intx/tools-posix"; +import { mapSearchBudgetOutcome } from "./shell-guard-plugin.js"; +import { BUDGET_EXPIRED } from "../util/budget-race.js"; +import { + formatUnboundedSearchMessage, + isUnboundedRootSearch, + TIMEOUT_PREFIX, +} from "./tool-time-budget.js"; +import { ripgrepPlugin } from "./ripgrep-plugin.js"; +import type { RgChild, SpawnRg } from "./rg-run.js"; + +/** + * CL-9469: search_files must fail closed on unbounded root walks instead of + * timing out. A workspace-root search with a recursive glob refuses up front + * with scope guidance; bounded searches still return hits. + */ + +async function withFixture( + run: (paths: { cwd: string; sub: string }) => Promise, +): Promise { + const cwd = await mkdtemp(join(tmpdir(), "cl9469-search-scope-")); + const sub = join(cwd, "sub"); + try { + await mkdir(join(sub, "nested"), { recursive: true }); + await mkdir(join(cwd, "node_modules"), { recursive: true }); + await writeFile(join(cwd, "top.ts"), "export const top = 1;\n"); + await writeFile(join(sub, "keep.ts"), "export const keep = 1;\n"); + await writeFile(join(sub, "nested", "deep.ts"), "export const deep = 1;\n"); + await writeFile( + join(cwd, "node_modules", "skip.ts"), + "export const s = 1;\n", + ); + await run({ cwd, sub }); + } finally { + await rm(cwd, { recursive: true, force: true }); + } +} + +function throwingSpawn(calls: string[][]): SpawnRg { + return (args) => { + calls.push(args); + throw new Error("search must not walk: rg must not spawn"); + }; +} + +// rg absent: the ENOENT error event is exactly what runRg treats as +// "rg not installed" ({ kind: "unavailable" } -> fallback walker leg). +function rgMissingSpawn(): SpawnRg { + return () => ({ + pid: undefined, + stdout: { on: () => undefined }, + stderr: { on: () => undefined }, + on: ((event: string, listener: (arg: never) => void) => { + if (event === "error") { + const err = Object.assign(new Error("spawn rg ENOENT"), { + code: "ENOENT", + }); + queueMicrotask(() => listener(err as never)); + } + }) as RgChild["on"], + kill: () => undefined, + }); +} + +describe("CL-9469 search_files fails closed on unbounded root walks", () => { + test("root recursive glob is refused with scope guidance, not a walk", async () => { + await withFixture(async ({ cwd }) => { + const spawnCalls: string[][] = []; + const tools = createPosixTools({ + cwd, + plugins: [ripgrepPlugin(cwd, {}, throwingSpawn(spawnCalls))], + }); + const started = Date.now(); + const result = await tools.run( + { id: "1", name: "search_files", arguments: { pattern: "**/*.ts" } }, + new AbortController().signal, + ); + expect(Date.now() - started).toBeLessThan(5_000); + expect(spawnCalls.length).toBe(0); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain("narrow `path`"); + expect(String(result.content)).toContain("glob"); + expect(String(result.content)).toContain("not the same as"); + expect(String(result.content)).not.toContain( + "[timed out before completing]", + ); + }); + }); + + test("explicit root path with recursive glob is refused the same way", async () => { + await withFixture(async ({ cwd }) => { + const spawnCalls: string[][] = []; + const tools = createPosixTools({ + cwd, + plugins: [ripgrepPlugin(cwd, {}, throwingSpawn(spawnCalls))], + }); + for (const path of [".", cwd]) { + const result = await tools.run( + { + id: "1", + name: "search_files", + arguments: { pattern: "**/*.ts", path }, + }, + new AbortController().signal, + ); + expect(spawnCalls.length).toBe(0); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain("narrow `path`"); + } + }); + }); + + test("bare star at the root is refused", async () => { + await withFixture(async ({ cwd }) => { + const spawnCalls: string[][] = []; + const tools = createPosixTools({ + cwd, + plugins: [ripgrepPlugin(cwd, {}, throwingSpawn(spawnCalls))], + }); + const result = await tools.run( + { id: "1", name: "search_files", arguments: { pattern: "*" } }, + new AbortController().signal, + ); + expect(spawnCalls.length).toBe(0); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain("narrow `path`"); + }); + }); + + test("scoped subdirectory recursive glob still returns hits", async () => { + await withFixture(async ({ cwd, sub }) => { + for (const spawn of [undefined, rgMissingSpawn()] as const) { + const tools = createPosixTools({ + cwd, + plugins: [ripgrepPlugin(cwd, {}, spawn)], + }); + const result = await tools.run( + { + id: "1", + name: "search_files", + arguments: { pattern: "**/*.ts", path: sub }, + }, + new AbortController().signal, + ); + expect(result.isError !== true).toBe(true); + expect(String(result.content)).toContain("keep.ts"); + expect(String(result.content)).toContain("deep.ts"); + } + }); + }); + + test("tight glob at the root still returns hits", async () => { + await withFixture(async ({ cwd }) => { + for (const spawn of [undefined, rgMissingSpawn()] as const) { + const tools = createPosixTools({ + cwd, + plugins: [ripgrepPlugin(cwd, {}, spawn)], + }); + const result = await tools.run( + { + id: "1", + name: "search_files", + arguments: { pattern: "*.ts" }, + }, + new AbortController().signal, + ); + expect(result.isError !== true).toBe(true); + expect(String(result.content)).toContain("top.ts"); + } + }); + }); +}); + +describe("CL-9469 unbounded-root predicate", () => { + const cwd = "/repo/root"; + test("omitted, empty, dot, and root paths count as the workspace root", () => { + for (const path of [undefined, "", ".", "./", "/repo/root"]) { + expect(isUnboundedRootSearch({ path, pattern: "**/*.ts", cwd })).toBe( + true, + ); + } + }); + test("subdirectories and non-filesystem targets are not the root", () => { + for (const path of [ + "sub", + "sub/nested", + "/elsewhere", + "archive:///x", + "tool-output:///y", + ]) { + expect(isUnboundedRootSearch({ path, pattern: "**/*.ts", cwd })).toBe( + false, + ); + } + }); + test("tight globs stay allowed at the root", () => { + for (const pattern of ["*.ts", "*config*", "README.md", "*.{ts,tsx}"]) { + expect(isUnboundedRootSearch({ path: undefined, pattern, cwd })).toBe( + false, + ); + } + }); + test("bare star counts as unbounded", () => { + expect(isUnboundedRootSearch({ path: undefined, pattern: "*", cwd })).toBe( + true, + ); + }); + test("refusal message carries scope guidance and never reads as a timeout", () => { + const message = formatUnboundedSearchMessage("search_files", "**/*.ts"); + expect(message).toContain("narrow `path`"); + expect(message).toContain("glob"); + expect(message).toContain("not the same as"); + expect(message).not.toContain(TIMEOUT_PREFIX); + // Shell-guard maps /abort/i errors to timeouts when its budget fires; + // refusal must flow through untouched. + expect(message).not.toMatch(/abort/i); + }); +}); + +describe("CL-9469 timeout path is never empty success", () => { + test("budget expiry is an explicit timeout error, not empty results", () => { + const result = mapSearchBudgetOutcome("1", "search_files", BUDGET_EXPIRED, { + budgetAborted: true, + parentAborted: false, + }); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain(TIMEOUT_PREFIX); + expect(String(result.content)).toContain("narrow `path`"); + expect(String(result.content)).toContain("not the same as"); + }); + test("parent abort during the race reads as aborted, not a timeout", () => { + const result = mapSearchBudgetOutcome("1", "grep", BUDGET_EXPIRED, { + budgetAborted: true, + parentAborted: true, + }); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain("aborted"); + expect(String(result.content)).not.toContain(TIMEOUT_PREFIX); + }); + test("timeout error from an inner plugin passes through as an error", () => { + const inner = { + callId: "1", + content: `grep ${TIMEOUT_PREFIX} — narrow down`, + isError: true, + } as const; + const result = mapSearchBudgetOutcome("1", "grep", inner, { + budgetAborted: true, + parentAborted: false, + }); + expect(result).toBe(inner); + expect(result.isError).toBe(true); + }); + test("generic abort torn down by the budget becomes a timeout error", () => { + const result = mapSearchBudgetOutcome( + "1", + "search_files", + { callId: "1", content: "aborted", isError: true }, + { budgetAborted: true, parentAborted: false }, + ); + expect(result.isError).toBe(true); + expect(String(result.content)).toContain(TIMEOUT_PREFIX); + }); + test("genuine empty success passes through untouched", () => { + const empty = { + callId: "1", + content: 'no files matching "*.zzz-nope"', + } as const; + const result = mapSearchBudgetOutcome("1", "search_files", empty, { + budgetAborted: false, + parentAborted: false, + }); + expect(result).toBe(empty); + expect(result.isError !== true).toBe(true); + expect(String(result.content)).not.toContain(TIMEOUT_PREFIX); + }); +}); diff --git a/src/plugins/shell-guard-plugin.ts b/src/plugins/shell-guard-plugin.ts index 355c93a52..53bd8d208 100644 --- a/src/plugins/shell-guard-plugin.ts +++ b/src/plugins/shell-guard-plugin.ts @@ -10,13 +10,14 @@ import { import { formatSearchTimeoutMessage, TIMEOUT_PREFIX, + type ScopedSearchTool, } from "./tool-time-budget.js"; import { BUDGET_EXPIRED, budgetExpiry, withTimeout, } from "../util/budget-race.js"; -import type { ToolDefinition } from "@intx/types/runtime"; +import type { ToolDefinition, ToolResult } from "@intx/types/runtime"; import { assertShellCwdUsable, isShellCwdWithinSession, @@ -161,6 +162,42 @@ export function advertiseShellGuardTimeout( const SEARCH_TOOL_TIMEOUT_MS = 10_000; const SEARCH_TOOLS = new Set(["grep", "search_files"]); +// Timeout outcome mapping for scoped search tools, extracted pure so the +// fail-closed behavior is unit-testable without waiting out the budget: a +// budget expiry or an abort torn down by the budget is always an explicit +// error carrying the timeout notice — a timeout never reads as empty +// results, and genuine empty successes pass through untouched. +export function mapSearchBudgetOutcome( + callId: string, + tool: ScopedSearchTool, + outcome: ToolResult | typeof BUDGET_EXPIRED, + signals: { budgetAborted: boolean; parentAborted: boolean }, +): ToolResult { + if (outcome === BUDGET_EXPIRED) { + const content = signals.parentAborted + ? `${tool} aborted` + : formatSearchTimeoutMessage(tool); + return { callId, content, isError: true }; + } + if ( + outcome.isError === true && + typeof outcome.content === "string" && + outcome.content.includes(TIMEOUT_PREFIX) + ) { + return outcome; + } + if ( + signals.budgetAborted && + !signals.parentAborted && + outcome.isError === true && + typeof outcome.content === "string" && + /abort/i.test(outcome.content) + ) { + return { callId, content: formatSearchTimeoutMessage(tool), isError: true }; + } + return outcome; +} + interface RunShellArgs { command: string; timeout?: number; @@ -716,42 +753,15 @@ export function shellGuardPlugin( budgetExpiry(budget.signal), ]); - if (outcome === BUDGET_EXPIRED) { - const content = signal.aborted - ? `${call.name} aborted` - : formatSearchTimeoutMessage( - call.name as "grep" | "search_files", - ); - return { callId: call.id, content, isError: true }; - } - - if ( - outcome.isError === true && - typeof outcome.content === "string" && - outcome.content.includes(TIMEOUT_PREFIX) - ) { - return outcome; - } - - // The base tool honored the abort and returned a generic abort error; - // surface a clearer timeout message when the budget, not the parent, - // triggered it. - if ( - budget.signal.aborted && - !signal.aborted && - outcome.isError === true && - typeof outcome.content === "string" && - /abort/i.test(outcome.content) - ) { - return { - callId: call.id, - content: formatSearchTimeoutMessage( - call.name as "grep" | "search_files", - ), - isError: true, - }; - } - return outcome; + return mapSearchBudgetOutcome( + call.id, + call.name as "grep" | "search_files", + outcome, + { + budgetAborted: budget.signal.aborted, + parentAborted: signal.aborted, + }, + ); } finally { budget.dispose(); } diff --git a/src/plugins/tool-time-budget.ts b/src/plugins/tool-time-budget.ts index 14893ceb4..ae6f7dfad 100644 --- a/src/plugins/tool-time-budget.ts +++ b/src/plugins/tool-time-budget.ts @@ -1,5 +1,7 @@ /** Corbits Code-local timeout copy for search/read guards. Do not patch interchange. */ +import { resolve as resolvePath } from "node:path"; + export type ScopedSearchTool = "grep" | "search_files"; export const TIMEOUT_PREFIX = "[timed out before completing]"; @@ -25,6 +27,46 @@ export function formatSearchTimeoutMessage( return `${trimmed}\n\n${notice}`; } +// A search_files pattern that walks the whole tree: recursive descent or a +// bare star with no literal constraint. Tighter globs (`*.ts`, `*config*`) +// still match a bounded name space and stay allowed at the root. +export function isUnboundedSearchGlob(pattern: string): boolean { + return pattern === "*" || pattern.includes("**"); +} + +// The raw path argument resolved against the session root: omitted, empty, +// ".", and the root itself all land on the workspace root. Non-filesystem +// targets (archive:///, tool-output:///) resolve elsewhere and never match. +export function isWorkspaceRootSearch( + path: string | undefined, + cwd: string, +): boolean { + const candidate = path !== undefined && path.length > 0 ? path : "."; + return resolvePath(cwd, candidate) === resolvePath(cwd); +} + +export function isUnboundedRootSearch(args: { + path: string | undefined; + pattern: string; + cwd: string; +}): boolean { + return ( + isWorkspaceRootSearch(args.path, args.cwd) && + isUnboundedSearchGlob(args.pattern) + ); +} + +export function formatUnboundedSearchMessage( + tool: ScopedSearchTool, + pattern: string, +): string { + return ( + `${tool} refused an unbounded workspace-root walk for pattern "${pattern}" — ` + + `${scopedSearchRetryHints(tool)}. ` + + `This is not the same as "no matches".` + ); +} + export function formatToolExecutionTimeoutMessage( toolName: string, timeoutMs: number,