Skip to content

Commit 590c840

Browse files
committed
fix(search): fail closed on unbounded workspace-root search_files
Workspace-root search_files with a recursive glob walked the whole tree until the shell-guard budget fired, and a timeout reads as a false negative. Refuse the walk up front with scope guidance instead.
1 parent b3a95ad commit 590c840

5 files changed

Lines changed: 389 additions & 38 deletions

File tree

‎docs/ARCHITECTURE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,7 @@ tool call
410410
- **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.
411411
- **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.
412412
- **Permission** (`permission-plugin.ts`) — Delegates consequential calls to the permission gate.
413-
- **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.
413+
- **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.
414414
- **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.
415415
- **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.
416416
- **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).

‎src/plugins/ripgrep-plugin.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ import {
1414
type RgLimits,
1515
type SpawnRg,
1616
} from "./rg-run.js";
17+
import {
18+
formatUnboundedSearchMessage,
19+
isUnboundedRootSearch,
20+
} from "./tool-time-budget.js";
1721
import { createExtraDeniedPathMatcher } from "./secret-guard-plugin.js";
1822

1923
// A grep over a large tree with the pure-TypeScript walker enumerates the whole
@@ -221,6 +225,21 @@ export function ripgrepPlugin(
221225
if (call.name === "search_files") {
222226
const pattern = str(call.arguments.pattern);
223227
if (pattern === undefined) return next(call, signal);
228+
// Unbounded workspace-root walks hang until the shell-guard budget
229+
// fires, and a timeout reads as a false negative. Refuse up front
230+
// with scope guidance instead of starting a walk that cannot finish
231+
// in budget.
232+
const rawPath =
233+
typeof call.arguments.path === "string"
234+
? call.arguments.path
235+
: undefined;
236+
if (isUnboundedRootSearch({ path: rawPath, pattern, cwd })) {
237+
return {
238+
callId: call.id,
239+
content: formatUnboundedSearchMessage("search_files", pattern),
240+
isError: true,
241+
};
242+
}
224243
const path = str(call.arguments.path) ?? cwd;
225244
const maxResults =
226245
num(call.arguments.max_results) ?? DEFAULT_SEARCH_MAX;
Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
import { createPosixTools } from "@intx/tools-posix";
6+
import { mapSearchBudgetOutcome } from "./shell-guard-plugin.js";
7+
import { BUDGET_EXPIRED } from "../util/budget-race.js";
8+
import {
9+
formatUnboundedSearchMessage,
10+
isUnboundedRootSearch,
11+
TIMEOUT_PREFIX,
12+
} from "./tool-time-budget.js";
13+
import { ripgrepPlugin } from "./ripgrep-plugin.js";
14+
import type { RgChild, SpawnRg } from "./rg-run.js";
15+
16+
/**
17+
* CL-9469: search_files must fail closed on unbounded root walks instead of
18+
* timing out. A workspace-root search with a recursive glob refuses up front
19+
* with scope guidance; bounded searches still return hits.
20+
*/
21+
22+
async function withFixture(
23+
run: (paths: { cwd: string; sub: string }) => Promise<void>,
24+
): Promise<void> {
25+
const cwd = await mkdtemp(join(tmpdir(), "cl9469-search-scope-"));
26+
const sub = join(cwd, "sub");
27+
try {
28+
await mkdir(join(sub, "nested"), { recursive: true });
29+
await mkdir(join(cwd, "node_modules"), { recursive: true });
30+
await writeFile(join(cwd, "top.ts"), "export const top = 1;\n");
31+
await writeFile(join(sub, "keep.ts"), "export const keep = 1;\n");
32+
await writeFile(join(sub, "nested", "deep.ts"), "export const deep = 1;\n");
33+
await writeFile(
34+
join(cwd, "node_modules", "skip.ts"),
35+
"export const s = 1;\n",
36+
);
37+
await run({ cwd, sub });
38+
} finally {
39+
await rm(cwd, { recursive: true, force: true });
40+
}
41+
}
42+
43+
function throwingSpawn(calls: string[][]): SpawnRg {
44+
return (args) => {
45+
calls.push(args);
46+
throw new Error("search must not walk: rg must not spawn");
47+
};
48+
}
49+
50+
// rg absent: the ENOENT error event is exactly what runRg treats as
51+
// "rg not installed" ({ kind: "unavailable" } -> fallback walker leg).
52+
function rgMissingSpawn(): SpawnRg {
53+
return () => ({
54+
pid: undefined,
55+
stdout: { on: () => undefined },
56+
stderr: { on: () => undefined },
57+
on: ((event: string, listener: (arg: never) => void) => {
58+
if (event === "error") {
59+
const err = Object.assign(new Error("spawn rg ENOENT"), {
60+
code: "ENOENT",
61+
});
62+
queueMicrotask(() => listener(err as never));
63+
}
64+
}) as RgChild["on"],
65+
kill: () => undefined,
66+
});
67+
}
68+
69+
describe("CL-9469 search_files fails closed on unbounded root walks", () => {
70+
test("root recursive glob is refused with scope guidance, not a walk", async () => {
71+
await withFixture(async ({ cwd }) => {
72+
const spawnCalls: string[][] = [];
73+
const tools = createPosixTools({
74+
cwd,
75+
plugins: [ripgrepPlugin(cwd, {}, throwingSpawn(spawnCalls))],
76+
});
77+
const started = Date.now();
78+
const result = await tools.run(
79+
{ id: "1", name: "search_files", arguments: { pattern: "**/*.ts" } },
80+
new AbortController().signal,
81+
);
82+
expect(Date.now() - started).toBeLessThan(5_000);
83+
expect(spawnCalls.length).toBe(0);
84+
expect(result.isError).toBe(true);
85+
expect(String(result.content)).toContain("narrow `path`");
86+
expect(String(result.content)).toContain("glob");
87+
expect(String(result.content)).toContain("not the same as");
88+
expect(String(result.content)).not.toContain(
89+
"[timed out before completing]",
90+
);
91+
});
92+
});
93+
94+
test("explicit root path with recursive glob is refused the same way", async () => {
95+
await withFixture(async ({ cwd }) => {
96+
const spawnCalls: string[][] = [];
97+
const tools = createPosixTools({
98+
cwd,
99+
plugins: [ripgrepPlugin(cwd, {}, throwingSpawn(spawnCalls))],
100+
});
101+
for (const path of [".", cwd]) {
102+
const result = await tools.run(
103+
{
104+
id: "1",
105+
name: "search_files",
106+
arguments: { pattern: "**/*.ts", path },
107+
},
108+
new AbortController().signal,
109+
);
110+
expect(spawnCalls.length).toBe(0);
111+
expect(result.isError).toBe(true);
112+
expect(String(result.content)).toContain("narrow `path`");
113+
}
114+
});
115+
});
116+
117+
test("bare star at the root is refused", async () => {
118+
await withFixture(async ({ cwd }) => {
119+
const spawnCalls: string[][] = [];
120+
const tools = createPosixTools({
121+
cwd,
122+
plugins: [ripgrepPlugin(cwd, {}, throwingSpawn(spawnCalls))],
123+
});
124+
const result = await tools.run(
125+
{ id: "1", name: "search_files", arguments: { pattern: "*" } },
126+
new AbortController().signal,
127+
);
128+
expect(spawnCalls.length).toBe(0);
129+
expect(result.isError).toBe(true);
130+
expect(String(result.content)).toContain("narrow `path`");
131+
});
132+
});
133+
134+
test("scoped subdirectory recursive glob still returns hits", async () => {
135+
await withFixture(async ({ cwd, sub }) => {
136+
for (const spawn of [undefined, rgMissingSpawn()] as const) {
137+
const tools = createPosixTools({
138+
cwd,
139+
plugins: [ripgrepPlugin(cwd, {}, spawn)],
140+
});
141+
const result = await tools.run(
142+
{
143+
id: "1",
144+
name: "search_files",
145+
arguments: { pattern: "**/*.ts", path: sub },
146+
},
147+
new AbortController().signal,
148+
);
149+
expect(result.isError !== true).toBe(true);
150+
expect(String(result.content)).toContain("keep.ts");
151+
expect(String(result.content)).toContain("deep.ts");
152+
}
153+
});
154+
});
155+
156+
test("tight glob at the root still returns hits", async () => {
157+
await withFixture(async ({ cwd }) => {
158+
for (const spawn of [undefined, rgMissingSpawn()] as const) {
159+
const tools = createPosixTools({
160+
cwd,
161+
plugins: [ripgrepPlugin(cwd, {}, spawn)],
162+
});
163+
const result = await tools.run(
164+
{
165+
id: "1",
166+
name: "search_files",
167+
arguments: { pattern: "*.ts" },
168+
},
169+
new AbortController().signal,
170+
);
171+
expect(result.isError !== true).toBe(true);
172+
expect(String(result.content)).toContain("top.ts");
173+
}
174+
});
175+
});
176+
});
177+
178+
describe("CL-9469 unbounded-root predicate", () => {
179+
const cwd = "/repo/root";
180+
test("omitted, empty, dot, and root paths count as the workspace root", () => {
181+
for (const path of [undefined, "", ".", "./", "/repo/root"]) {
182+
expect(isUnboundedRootSearch({ path, pattern: "**/*.ts", cwd })).toBe(
183+
true,
184+
);
185+
}
186+
});
187+
test("subdirectories and non-filesystem targets are not the root", () => {
188+
for (const path of [
189+
"sub",
190+
"sub/nested",
191+
"/elsewhere",
192+
"archive:///x",
193+
"tool-output:///y",
194+
]) {
195+
expect(isUnboundedRootSearch({ path, pattern: "**/*.ts", cwd })).toBe(
196+
false,
197+
);
198+
}
199+
});
200+
test("tight globs stay allowed at the root", () => {
201+
for (const pattern of ["*.ts", "*config*", "README.md", "*.{ts,tsx}"]) {
202+
expect(isUnboundedRootSearch({ path: undefined, pattern, cwd })).toBe(
203+
false,
204+
);
205+
}
206+
});
207+
test("bare star counts as unbounded", () => {
208+
expect(isUnboundedRootSearch({ path: undefined, pattern: "*", cwd })).toBe(
209+
true,
210+
);
211+
});
212+
test("refusal message carries scope guidance and never reads as a timeout", () => {
213+
const message = formatUnboundedSearchMessage("search_files", "**/*.ts");
214+
expect(message).toContain("narrow `path`");
215+
expect(message).toContain("glob");
216+
expect(message).toContain("not the same as");
217+
expect(message).not.toContain(TIMEOUT_PREFIX);
218+
// Shell-guard maps /abort/i errors to timeouts when its budget fires;
219+
// refusal must flow through untouched.
220+
expect(message).not.toMatch(/abort/i);
221+
});
222+
});
223+
224+
describe("CL-9469 timeout path is never empty success", () => {
225+
test("budget expiry is an explicit timeout error, not empty results", () => {
226+
const result = mapSearchBudgetOutcome("1", "search_files", BUDGET_EXPIRED, {
227+
budgetAborted: true,
228+
parentAborted: false,
229+
});
230+
expect(result.isError).toBe(true);
231+
expect(String(result.content)).toContain(TIMEOUT_PREFIX);
232+
expect(String(result.content)).toContain("narrow `path`");
233+
expect(String(result.content)).toContain("not the same as");
234+
});
235+
test("parent abort during the race reads as aborted, not a timeout", () => {
236+
const result = mapSearchBudgetOutcome("1", "grep", BUDGET_EXPIRED, {
237+
budgetAborted: true,
238+
parentAborted: true,
239+
});
240+
expect(result.isError).toBe(true);
241+
expect(String(result.content)).toContain("aborted");
242+
expect(String(result.content)).not.toContain(TIMEOUT_PREFIX);
243+
});
244+
test("timeout error from an inner plugin passes through as an error", () => {
245+
const inner = {
246+
callId: "1",
247+
content: `grep ${TIMEOUT_PREFIX} — narrow down`,
248+
isError: true,
249+
} as const;
250+
const result = mapSearchBudgetOutcome("1", "grep", inner, {
251+
budgetAborted: true,
252+
parentAborted: false,
253+
});
254+
expect(result).toBe(inner);
255+
expect(result.isError).toBe(true);
256+
});
257+
test("generic abort torn down by the budget becomes a timeout error", () => {
258+
const result = mapSearchBudgetOutcome(
259+
"1",
260+
"search_files",
261+
{ callId: "1", content: "aborted", isError: true },
262+
{ budgetAborted: true, parentAborted: false },
263+
);
264+
expect(result.isError).toBe(true);
265+
expect(String(result.content)).toContain(TIMEOUT_PREFIX);
266+
});
267+
test("genuine empty success passes through untouched", () => {
268+
const empty = {
269+
callId: "1",
270+
content: 'no files matching "*.zzz-nope"',
271+
} as const;
272+
const result = mapSearchBudgetOutcome("1", "search_files", empty, {
273+
budgetAborted: false,
274+
parentAborted: false,
275+
});
276+
expect(result).toBe(empty);
277+
expect(result.isError !== true).toBe(true);
278+
expect(String(result.content)).not.toContain(TIMEOUT_PREFIX);
279+
});
280+
});

0 commit comments

Comments
 (0)