Merge pull request #546 from corbitsdev/cl-6925-drop-the-15s-default-run_shell-timeout

Drop the built-in run_shell timeout default

Author: TheGreatAxios

CHANGELOG.md

@@ -13,13 +13,22 @@ 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
   unset; values must still be integers ≥1. `task(maxTurns)`, profile
   `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

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).

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<string, string> = {
     "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)",

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<AgentT
     return { content: "Tasks updated." };
   };
 
-  // Align the advertised run_shell timeout with shell-guard's resolved default.
+  // Align the advertised run_shell timeout with shell-guard (no built-in default;
+  // advertise settings.shell.timeoutMs when set).
   const baseTools: AgentTool[] = [
     ...fromToolRunner(posixTools).map((tool) => ({
       ...tool,

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 {

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<string, { description: string }>
+    )["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<string, { description: string }>
     )["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",