diff --git a/CHANGELOG.md b/CHANGELOG.md index c591450ad..1cb3e9c42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,14 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename ## [Unreleased] +### Plugins + +- **`run_shell` no longer defaults to a 15s timeout.** Omitted timeout arms no + timer (match Pi). Pass a per-call `timeout`, or set `shell.timeoutMs` in + settings, to bound a command. `shell.maxTimeoutMs` still clamps a resolved + timeout and does not invent one on its own. Abort and the output-byte cap are + unchanged. + ### Sub-agents - **Sub-agent `maxTurns` no longer hard-caps at 100.** Default remains 30 when @@ -20,6 +28,7 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename `maxTurns`, and `settings.subagentMaxTurns` may exceed 100 for long jobs. ## [0.2.104] - 2026-08-23 + ### TUI - **Taller live chain-of-thought preview.** Parent reasoning still paints diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e9907674b..729091f96 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -354,7 +354,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`. 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: they require operator approval via the permission gate, and auto mode forces an ask through the auto-shell policy (`sensitive-path` rule). Once the operator approves, the command runs. 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`, wired by `authz-plugin.ts`) — 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 permission gate’s shell auto-allow path consults the same policy so it never pre-approves a command authz would reject. - **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): 15s default timeout, 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout/abort only. Also applies a 10s wall-clock budget to `grep`/`search_files`. +- **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): no built-in default timeout (optional per-call or `settings.shell.timeoutMs`; `maxTimeoutMs` clamps only a resolved timeout), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout/abort only. Also applies a 10s wall-clock budget to `grep`/`search_files`. - **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/agent/prompts.ts b/src/agent/prompts.ts index ff02c512e..e008b6a17 100644 --- a/src/agent/prompts.ts +++ b/src/agent/prompts.ts @@ -69,7 +69,7 @@ export function buildHarnessFacts( ]), "- Use the provided tools for file reads/searches instead of shelling out as a substitute.", "- read_file accepts a filesystem path or a tool-output:///{callId} URI from a prior tool result when the harness exposes one; prefer the URI over re-reading huge blobs.", - "- run_shell defaults to a 15s timeout; pass timeout for builds, tests, and other long commands.", + "- run_shell has no default timeout; pass timeout for builds, tests, and other long commands.", "- Shell find, rg, and grep -r are blocked — they can walk huge trees and OOM the host. Prefer the bounded grep/search_files tools, and do not substitute another unbounded walk (fd, ls -R, scripted os.walk).", ...(subAgent ? [ @@ -207,7 +207,7 @@ const TOOL_SUMMARIES: Record = { "make a surgical edit (exact old_string match, or start_line/end_line line-range mode; never include read_file's NNNNNN\\t line prefix; substring failures include nearby file text; prefer over sed/awk in the shell)", delete_file: "delete one file with an explicit outcome (never shell rm)", run_shell: - "run a shell command (builds, tests, git; 15s default timeout — pass timeout ms to override; never to read/write/delete files, search trees, or talk to the user)", + "run a shell command (builds, tests, git; pass timeout ms to bound long commands; never to read/write/delete files, search trees, or talk to the user)", search_files: "find files by name or pattern (bounded; timeout + output caps — safer than open-ended shell find)", grep: "search file contents (bounded; timeout + output caps — safer than open-ended shell grep -r/rg)", diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 22ba9a781..dcfecbbef 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -91,8 +91,9 @@ export interface AgentToolsetArgs { // Skill directories (from enabled plugins) the use_skill tool resolves bodies // from, in addition to the project-local and bundled defaults. skillDirs?: string[]; - // Shell command timeout defaults/cap, resolved from settings. When omitted the - // shell-guard plugin applies its built-in defaults. + // Shell command timeout default/cap, resolved from settings. When omitted the + // shell-guard plugin arms no default timeout (per-call timeout or settings + // shell.timeoutMs required to bound a command). shellTimeout?: ShellTimeoutConfig; // Outer per-invocation tool run budget (dynamic runner). When omitted built-in // defaults apply. @@ -246,7 +247,8 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise ({ ...tool, diff --git a/src/config/settings.ts b/src/config/settings.ts index c6a052e88..7bf098c48 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -131,9 +131,11 @@ export interface Settings { // back to whatever the user's main session is currently using so the agent // still runs; "none" treats it as a hard error and the profile fails to load. agentModelFallback?: "active" | "none"; - // Shell command timeouts. `timeoutMs` is the default applied when the model - // does not pass a per-command timeout; `maxTimeoutMs` caps any per-command - // override so a single command cannot wait effectively unbounded. + // Shell command timeouts. `timeoutMs` is the optional default applied when the + // model does not pass a per-command timeout (unset = no default timeout, match + // Pi). `maxTimeoutMs` clamps a resolved timeout only — it alone does not invent + // one. A single command with neither settings default nor a per-call timeout + // runs until exit, abort, or the outer tool watchdog (when configured). shell?: { timeoutMs?: number; maxTimeoutMs?: number }; // Outer wall-clock budget for each tool `run()` (dynamic runner / agent dispatch). // @@ -240,7 +242,7 @@ export function listFavoriteModels(settings: Settings): ModelRef[] { } // Maps the settings shell block to the shape the shell-guard plugin expects. -// Returns undefined when unset so the plugin applies its own defaults. +// Returns undefined when unset so the plugin arms no default timeout. export function shellTimeoutFromSettings( settings?: Settings | null, ): { defaultMs?: number; maxMs?: number } | undefined { diff --git a/src/plugins/shell-guard-plugin.test.ts b/src/plugins/shell-guard-plugin.test.ts index 1e734c168..585bdfbb5 100644 --- a/src/plugins/shell-guard-plugin.test.ts +++ b/src/plugins/shell-guard-plugin.test.ts @@ -10,7 +10,6 @@ import { randomUUID } from "node:crypto"; import { BoundedShellOutput, - DEFAULT_SHELL_TIMEOUT_MS, MAX_SHELL_OUTPUT_BYTES, advertiseShellGuardTimeout, resolveShellTimeoutMs, @@ -32,8 +31,18 @@ describe("runGuardedShell", () => { expect(output).toContain("hello"); }); - test("defaults to a 15s timeout", () => { - expect(DEFAULT_SHELL_TIMEOUT_MS).toBe(15_000); + test("omitted timeout does not arm a timer", async () => { + const start = Date.now(); + const { exitCode, timedOut, output } = await runGuardedShell( + { command: "sleep 0.25; echo done" }, + neverAbort(), + ); + expect(timedOut).toBe(false); + expect(exitCode).toBe(0); + expect(output).toContain("done"); + // Completes without a timeout flag; under a 15s default this would also + // pass for a short sleep — pair with resolveShellTimeoutMs coverage. + expect(Date.now() - start).toBeLessThan(5_000); }); test("merges settings.env into the spawn environment on top of process.env", async () => { @@ -146,37 +155,75 @@ describe("runGuardedShell", () => { }); describe("resolveShellTimeoutMs", () => { - test("omitted timeout uses the 15s default", () => { - expect(resolveShellTimeoutMs(undefined, DEFAULT_SHELL_TIMEOUT_MS)).toBe(15_000); - expect(resolveShellTimeoutMs(undefined, DEFAULT_SHELL_TIMEOUT_MS, undefined)).toBe( - DEFAULT_SHELL_TIMEOUT_MS, - ); + test("omitted timeout with no default is undefined (no timer)", () => { + expect(resolveShellTimeoutMs(undefined, undefined)).toBeUndefined(); + expect(resolveShellTimeoutMs(undefined, undefined, undefined)).toBeUndefined(); + }); + + test("maxMs alone does not invent a timeout", () => { + expect(resolveShellTimeoutMs(undefined, undefined, 100)).toBeUndefined(); + expect(resolveShellTimeoutMs(undefined, undefined, 600_000)).toBeUndefined(); }); - test("non-positive requested timeout falls back to default", () => { - expect(resolveShellTimeoutMs(0, DEFAULT_SHELL_TIMEOUT_MS)).toBe(DEFAULT_SHELL_TIMEOUT_MS); - expect(resolveShellTimeoutMs(-1, DEFAULT_SHELL_TIMEOUT_MS)).toBe(DEFAULT_SHELL_TIMEOUT_MS); + test("non-positive requested timeout falls back to default when set", () => { + expect(resolveShellTimeoutMs(0, 15_000)).toBe(15_000); + expect(resolveShellTimeoutMs(-1, 15_000)).toBe(15_000); + }); + + test("non-positive requested with no default is undefined", () => { + expect(resolveShellTimeoutMs(0, undefined)).toBeUndefined(); + expect(resolveShellTimeoutMs(-1, undefined)).toBeUndefined(); }); test("requested timeout well above 10 minutes is not clamped when maxMs is omitted", () => { - expect(resolveShellTimeoutMs(5_400_000, DEFAULT_SHELL_TIMEOUT_MS)).toBe(5_400_000); - expect(resolveShellTimeoutMs(5_400_000, DEFAULT_SHELL_TIMEOUT_MS, undefined)).toBe(5_400_000); - expect(resolveShellTimeoutMs(900_000, DEFAULT_SHELL_TIMEOUT_MS)).toBe(900_000); + expect(resolveShellTimeoutMs(5_400_000, undefined)).toBe(5_400_000); + expect(resolveShellTimeoutMs(5_400_000, undefined, undefined)).toBe(5_400_000); + expect(resolveShellTimeoutMs(900_000, 15_000)).toBe(900_000); }); - test("configured maxMs still clamps", () => { - expect(resolveShellTimeoutMs(900_000, DEFAULT_SHELL_TIMEOUT_MS, 100)).toBe(100); - expect(resolveShellTimeoutMs(5_400_000, DEFAULT_SHELL_TIMEOUT_MS, 600_000)).toBe(600_000); - expect(resolveShellTimeoutMs(undefined, DEFAULT_SHELL_TIMEOUT_MS, 100)).toBe(100); + test("configured maxMs still clamps a resolved timeout", () => { + expect(resolveShellTimeoutMs(900_000, undefined, 100)).toBe(100); + expect(resolveShellTimeoutMs(5_400_000, 15_000, 600_000)).toBe(600_000); + expect(resolveShellTimeoutMs(undefined, 15_000, 100)).toBe(100); }); test("requested below maxMs is unchanged", () => { - expect(resolveShellTimeoutMs(1_000, DEFAULT_SHELL_TIMEOUT_MS, 600_000)).toBe(1_000); + expect(resolveShellTimeoutMs(1_000, undefined, 600_000)).toBe(1_000); + }); + + test("settings defaultMs applies when request is omitted", () => { + expect(resolveShellTimeoutMs(undefined, 90)).toBe(90); }); }); describe("advertiseShellGuardTimeout", () => { - test("rewrites run_shell timeout default to match the guard", () => { + test("rewrites run_shell timeout description when a settings default is set", () => { + const rewritten = advertiseShellGuardTimeout( + { + name: "run_shell", + description: "Execute a shell command", + inputSchema: { + type: "object", + properties: { + command: { type: "string" }, + timeout: { + type: "number", + description: "Timeout in milliseconds (default: 30000)", + }, + }, + required: ["command"], + }, + }, + 120_000, + ); + const timeout = ( + rewritten.inputSchema["properties"] as Record + )["timeout"]; + expect(timeout?.description).toContain("120000"); + expect(timeout?.description).not.toContain("30000"); + }); + + test("advertises no default when settings default is unset", () => { const rewritten = advertiseShellGuardTimeout({ name: "run_shell", description: "Execute a shell command", @@ -195,8 +242,9 @@ describe("advertiseShellGuardTimeout", () => { const timeout = ( rewritten.inputSchema["properties"] as Record )["timeout"]; - expect(timeout?.description).toContain(String(DEFAULT_SHELL_TIMEOUT_MS)); + expect(timeout?.description).toMatch(/no default|omit/i); expect(timeout?.description).not.toContain("30000"); + expect(timeout?.description).not.toContain("15000"); }); test("leaves other tools unchanged", () => { @@ -270,6 +318,26 @@ describe("shellGuardPlugin", () => { expect(result.content).toMatch(/timed out after 90ms/); }); + test("omitted timeout with no settings default does not time out", async () => { + const handler = shellGuardPlugin(process.cwd()).middleware!(fallback); + const result = await handler( + { id: "c2d", name: "run_shell", arguments: { command: "sleep 0.2; echo ok" } }, + neverAbort(), + ); + expect(result.content).toContain("ok"); + expect(String(result.content)).not.toMatch(/timed out/); + }); + + test("maxMs alone does not invent a timeout when the model omits timeout", async () => { + const handler = shellGuardPlugin(process.cwd(), { maxMs: 50 }).middleware!(fallback); + const result = await handler( + { id: "c2e", name: "run_shell", arguments: { command: "sleep 0.2; echo ok" } }, + neverAbort(), + ); + expect(result.content).toContain("ok"); + expect(String(result.content)).not.toMatch(/timed out/); + }); + test("passes non-shell tools through", async () => { const result = await run({ id: "c3", diff --git a/src/plugins/shell-guard-plugin.ts b/src/plugins/shell-guard-plugin.ts index 488aa3657..1ab91531e 100644 --- a/src/plugins/shell-guard-plugin.ts +++ b/src/plugins/shell-guard-plugin.ts @@ -14,10 +14,9 @@ import { // Corbits Code-side replacement for stock `@intx/tools-posix` run_shell. // We do not patch interchange: this middleware short-circuits run_shell and -// enforces a short default timeout, an output-byte cap, and process-group kill -// so open-ended walks cannot OOM the host. +// enforces an optional timeout (no built-in default — match Pi), an +// output-byte cap, and process-group kill so open-ended walks cannot OOM the host. -export const DEFAULT_SHELL_TIMEOUT_MS = 15_000; export const MAX_SHELL_OUTPUT_BYTES = 512_000; export interface ShellTimeoutConfig { @@ -28,27 +27,32 @@ export interface ShellTimeoutConfig { /** * Effective run_shell timeout. Omitting `requested` (or a non-positive value) - * uses `defaultMs`. `maxMs` clamps only when settings pass it — there is no - * implicit 10-minute ceiling. + * uses `defaultMs` when set; otherwise returns undefined (no timer). `maxMs` + * clamps only a resolved timeout — it alone does not invent one. There is no + * implicit 10-minute ceiling and no built-in 15s/2m default. */ export function resolveShellTimeoutMs( requested: number | undefined, - defaultMs: number, + defaultMs: number | undefined, maxMs?: number, -): number { - const base = requested !== undefined && requested > 0 ? requested : defaultMs; +): number | undefined { + const fromRequest = requested !== undefined && requested > 0 ? requested : undefined; + const fromDefault = defaultMs !== undefined && defaultMs > 0 ? defaultMs : undefined; + const base = fromRequest ?? fromDefault; + if (base === undefined) return undefined; if (maxMs === undefined) return base; return Math.min(base, maxMs); } /** - * Stock tools-posix still advertises timeout default 30000. Shell-guard enforces - * 15s default; rewrite the definition the model sees so schema and behavior agree. - * Corbits Code-only — does not patch interchange. + * Stock tools-posix still advertises timeout default 30000. Shell-guard has no + * built-in default; rewrite the definition the model sees so schema and behavior + * agree. When settings supply defaultMs, advertise that. Corbits Code-only — + * does not patch interchange. */ export function advertiseShellGuardTimeout( definition: ToolDefinition, - defaultMs: number = DEFAULT_SHELL_TIMEOUT_MS, + defaultMs?: number, ): ToolDefinition { if (definition.name !== "run_shell") return definition; const schema = definition.inputSchema; @@ -61,9 +65,12 @@ export function advertiseShellGuardTimeout( const cwdProp = properties["cwd"]; const nextProperties = { ...properties }; if (timeout !== undefined && typeof timeout === "object" && timeout !== null) { + const hasDefault = defaultMs !== undefined && defaultMs > 0; nextProperties["timeout"] = { ...(timeout as Record), - description: `Timeout in milliseconds (default: ${defaultMs})`, + description: hasDefault + ? `Timeout in milliseconds (default: ${defaultMs})` + : "Timeout in milliseconds (optional; omit for no default timeout)", }; } if (cwdProp === undefined) { @@ -216,7 +223,8 @@ export async function runGuardedShell( ): Promise { signal.throwIfAborted(); - const timeoutMs = args.timeout ?? DEFAULT_SHELL_TIMEOUT_MS; + // Arm setTimeout only when a positive timeout was resolved. No built-in default. + const timeoutMs = args.timeout !== undefined && args.timeout > 0 ? args.timeout : undefined; const outputCap = args.maxOutputBytes ?? MAX_SHELL_OUTPUT_BYTES; const collector = new BoundedShellOutput(outputCap); @@ -239,11 +247,16 @@ export async function runGuardedShell( } let settled = false; + let timer: ReturnType | undefined; + + const clearTimer = () => { + if (timer !== undefined) clearTimeout(timer); + }; const settle = (err?: Error) => { if (settled) return; settled = true; - clearTimeout(timer); + clearTimer(); abortCleanup(); if (err !== undefined) { reject(err); @@ -253,7 +266,7 @@ export async function runGuardedShell( const finishOutput = (exitCode: number, timedOut: boolean) => { if (settled) return; settled = true; - clearTimeout(timer); + clearTimer(); abortCleanup(); const { output, truncated } = collector.build(); resolve({ @@ -275,13 +288,15 @@ export async function runGuardedShell( child.stdout.on("data", onChunk); child.stderr.on("data", onChunk); - const timer = setTimeout(() => { - killProcessTree(child); - // A timeout is not a failure the agent should be denied output for: return - // whatever the command produced before the kill, plus the timed-out notice - // the caller appends from `timedOut`. - finishOutput(124, true); - }, timeoutMs); + if (timeoutMs !== undefined) { + timer = setTimeout(() => { + killProcessTree(child); + // A timeout is not a failure the agent should be denied output for: return + // whatever the command produced before the kill, plus the timed-out notice + // the caller appends from `timedOut`. + finishOutput(124, true); + }, timeoutMs); + } const onAbort = () => { killProcessTree(child); @@ -365,7 +380,9 @@ export function shellGuardPlugin( env?: Record, options: ShellGuardPluginOptions = {}, ): ToolPlugin { - const defaultMs = timeoutConfig?.defaultMs ?? DEFAULT_SHELL_TIMEOUT_MS; + // No built-in default — only settings.shell.timeoutMs (or a per-call timeout) + // arms a timer. maxMs alone does not invent one. + const defaultMs = timeoutConfig?.defaultMs; const maxOutputBytes = timeoutConfig?.maxOutputBytes ?? MAX_SHELL_OUTPUT_BYTES; const sessionRoot = realpathSync(cwd); let retainedShellCwd = sessionRoot; @@ -432,7 +449,7 @@ export function shellGuardPlugin( { command: wrappedCommand, cwd: executionCwd, - timeout: effectiveTimeout, + ...(effectiveTimeout !== undefined ? { timeout: effectiveTimeout } : {}), maxOutputBytes, ...(env !== undefined ? { env } : {}), }, diff --git a/src/prompts.test.ts b/src/prompts.test.ts index 066b17d0f..0a26355aa 100644 --- a/src/prompts.test.ts +++ b/src/prompts.test.ts @@ -53,7 +53,7 @@ test("harness facts state only the non-derivable tool and safety rules", () => { expect(facts).toContain("Spawn build"); expect(facts).not.toContain("not mounted on the primary Skywalker session"); expect(facts).toContain("blocked"); - expect(facts).toContain("15s timeout"); + expect(facts).toContain("no default timeout"); expect(facts).toContain("find, rg, and grep -r"); expect(facts).toMatch(/OOM the host/); expect(facts).toMatch(/Prefer the bounded grep\/search_files tools/); diff --git a/src/tui/tool-execution-watchdog.test.ts b/src/tui/tool-execution-watchdog.test.ts index b39c3beb7..2cf6943c5 100644 --- a/src/tui/tool-execution-watchdog.test.ts +++ b/src/tui/tool-execution-watchdog.test.ts @@ -96,7 +96,7 @@ describe("tool execution watchdog", () => { expect(ms).toBe(requested + RUN_SHELL_WATCHDOG_SLACK_MS); }); - test("omitted run_shell timeout is unbounded (shell-guard still 15s)", () => { + test("omitted run_shell timeout is unbounded (shell-guard also has no default)", () => { expect( resolveToolExecutionTimeoutMs(undefined, { id: "1", name: "run_shell", arguments: {} }), ).toBeUndefined(); diff --git a/tests/unit/tui/agent-tools.test.ts b/tests/unit/tui/agent-tools.test.ts index 00e58f469..79f8ad4b8 100644 --- a/tests/unit/tui/agent-tools.test.ts +++ b/tests/unit/tui/agent-tools.test.ts @@ -112,7 +112,6 @@ mock.module("../../../src/plugins/secret-guard-plugin.js", () => ({ mock.module("../../../src/plugins/shell-guard-plugin.js", () => ({ shellGuardPlugin: () => ({}), advertiseShellGuardTimeout: (defs: ToolDefinition[]) => defs, - DEFAULT_SHELL_TIMEOUT_MS: 15_000, })); mock.module("../../../src/plugins/read-file-guard-plugin.js", () => ({