diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b9c48c811..32b12bd85 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -394,7 +394,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`, 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 delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels (schema advertised by `advertiseShellGuardTimeout`; 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`. 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/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index d32229c37..ea85d4876 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -206,11 +206,11 @@ Listings are list-free, dumps are dump-locked: a bounded `ls`/`tree` prints name #### Background shell mode -`run_shell` accepts `background: true` (shell-guard plugin, after the permission chain — a denied command spawns nothing). The starting call resolves `cwd` as usual and applies a per-call `timeout` only when passed (no 120s default on background), skips the pwd probe, spawns a detached process group via the registry in `src/shell/background-shell.ts`, and returns `{shell_id, status: "running"}` immediately; the retained shell cwd is never mutated by a background run. Limits: 8 running, 8 completed entries (ring; evicted ids collect as not-found — truncated output is spilled to a `tool-output:///bg-shell-` blob named in the completion message). On process exit the host delivers `buildShellBackgroundMessage(exit)` (exit status, timed-out marker, ~2KB output preview, spill URI) through the same continuation channel as compaction — wired in all three loop hosts (TUI, exec, sub-agent). `shell_collect` (`{shell_id, action: "collect"|"cancel", wait_ms?}`, default non-blocking) retrieves status/output or kills the process group; it is ungated by design (cancel only kills the session's own child). Timeout keeps its meaning: expiry kills the group and reports exit code 124 with `timed_out: true`. The tool watchdog exempts background starts and `shell_collect` (same list as `spawn_agent`/`wait_agents`/`ask_director`). Toolset dispose calls `disposeAll("session closed")` before the posix teardown, so `/clear`, interrupt, and reload kill every live background process group. +`run_shell` accepts `background: true` (shell-guard plugin, after the permission chain — a denied command spawns nothing). The starting call resolves `cwd` as usual and applies a per-call `timeout` only when passed (no 120s default on background), skips the pwd probe, spawns a detached process group via the registry in `src/shell/background-shell.ts`, and returns `{shell_id, status: "running"}` immediately; the retained shell cwd is never mutated by a background run. Limits: 8 running, 8 completed entries (ring; evicted ids collect as not-found — truncated output is spilled to a `tool-output:///bg-shell-` blob named in the completion message). Completion is process-exit, not stdio-close: a grandchild holding the pipe does not park `shell_collect`. On process exit the host delivers `buildShellBackgroundMessage(exit)` (exit status, timed-out marker, ~2KB output preview, spill URI) through the same continuation channel as compaction — wired in all three loop hosts (TUI, exec, sub-agent). `shell_collect` (`{shell_id, action: "collect"|"cancel", wait_ms?}`, default non-blocking, `wait_ms` capped at 300s) retrieves status/output or kills the process group; it is ungated by design (cancel only kills the session's own child). Timeout keeps its meaning: expiry kills the group and reports exit code 124 with `timed_out: true`. The tool watchdog exempts background starts and `shell_collect` (same list as `spawn_agent`/`wait_agents`/`ask_director`) because collect's own wait cap bounds the poll. Surfaces that do not mount `shell_collect` (intern, migrator) do not advertise `background: true` and the registry is unwired — background starts fail closed. Parent abort, deadline, and `close_agent` release parked collect waiters and dispose live background groups; `interrupt_agent` only releases waiters so the session (and its children) stay resumable. Toolset dispose calls `disposeAll("session closed")` before the posix teardown, so `/clear`, interrupt, and reload kill every live background process group. - **Alt+Enter** queues a follow-up (kind `"queue"`) delivered only on **session-idle** — parent-idle **and** no live fleet lanes (`run` goes idle). Session-idle Alt+Enter is a no-op. **Ctrl+C** stops the run. -Idle-with-fleet is shipped: after a non-blocking `spawn_agent` dispatch the parent turn can settle while workers keep running. The runner emits a `fleet` event carrying the live-lane count; the bridge holds the run busy on that count, so mid-hold Enter upgrades to a new primary turn (sent immediately) instead of queueing a steer, follow-ups keep waiting for true session-idle, and any steer left pending at the hold's engagement delivers immediately — the parent it was steering has already stopped. While the hold is up and the parent is not processing, occupancy flushes mailbox mail (`driveMailboxMail` + `buildMailboxMailMessage`) on store subscribe and idle-with-fleet settle — one child done while siblings run is enough. Skip that shot when a fleet-dry open-task continuation is latched. Exec-primary `wait_agents` mounts with no yield predicate (`createWaitAgentsTool({ sessions, fleetRecords })`) and blocks to ready/timeout/abort; `shouldYieldWait` remains a supported-but-unwired tool option (no production mount passes it). +Idle-with-fleet is shipped: after a non-blocking `spawn_agent` dispatch the parent turn can settle while workers keep running. The runner emits a `fleet` event carrying the live-lane count; the bridge holds the run busy on that count, so mid-hold Enter upgrades to a new primary turn (sent immediately) instead of queueing a steer, follow-ups keep waiting for true session-idle, and any steer left pending at the hold's engagement delivers immediately — the parent it was steering has already stopped. While the hold is up and the parent is not processing, occupancy flushes mailbox mail (`driveMailboxMail` + `buildMailboxMailMessage`) on store subscribe and idle-with-fleet settle — one child done while siblings run is enough. Skip that shot when a fleet-dry open-task continuation is latched. Exec-primary `wait_agents` mounts with no yield predicate (`createWaitAgentsTool({ sessions, fleetRecords })`) and blocks to ready/timeout/abort; parent interrupt/abort cancels that parked waiter via the tool AbortSignal (`timed_out`, workers stay running) rather than a second waiter table. `interrupt_agent` on a child unblocks wait as interrupted through the mailbox overlay. `shouldYieldWait` remains a supported-but-unwired tool option (no production mount passes it). `src/tui/stream-event-map.ts` maps reactor events onto the bridge's inbound events, and `src/tui/turn-state.ts` tracks the turn's status. `src/tui/turns-to-blocks.ts` hydrates a resumed session's stored turns into the same content blocks. diff --git a/src/agent/background-shell-tool.ts b/src/agent/background-shell-tool.ts index b78170666..8e2a8ab64 100644 --- a/src/agent/background-shell-tool.ts +++ b/src/agent/background-shell-tool.ts @@ -2,6 +2,7 @@ import { type } from "arktype"; import type { ToolDefinition } from "@intx/types/runtime"; import { createBackgroundShellRegistry, + MAX_SHELL_COLLECT_WAIT_MS, type BackgroundShellExit, type BackgroundShellRegistry, } from "../shell/background-shell.js"; @@ -41,7 +42,8 @@ export const shellCollectDefinition: ToolDefinition = { wait_ms: { type: "number", description: - 'For action="collect": milliseconds to wait for completion before returning "running" (default 0, non-blocking).', + 'For action="collect": milliseconds to wait for completion before returning "running" (default 0, non-blocking, capped at ' + + `${MAX_SHELL_COLLECT_WAIT_MS}).`, }, }, required: ["shell_id", "action"], diff --git a/src/agent/directors/intern/package.test.ts b/src/agent/directors/intern/package.test.ts index e56011322..a661c9138 100644 --- a/src/agent/directors/intern/package.test.ts +++ b/src/agent/directors/intern/package.test.ts @@ -28,6 +28,12 @@ describe("internPackage", () => { expect(p).toMatch(/You do NOT:[\s\S]*Debug failures/); }); + test("fail-closes without shell_collect rather than pinning prompt copy", () => { + const allow = internPackage.tools?.allow ?? []; + expect(allow).toContain("run_shell"); + expect(allow).not.toContain("shell_collect"); + }); + test("spawn.maySpawn is false", () => { expect(internPackage.spawn.maySpawn).toBe(false); }); @@ -40,6 +46,7 @@ describe("internPackage", () => { expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); expect(allow).toContain("delete_file"); + expect(allow).not.toContain("shell_collect"); for (const name of [ "grep", "search_files", diff --git a/src/agent/directors/intern/package.ts b/src/agent/directors/intern/package.ts index 033b5f320..92558748d 100644 --- a/src/agent/directors/intern/package.ts +++ b/src/agent/directors/intern/package.ts @@ -60,6 +60,7 @@ You do NOT: - Do not overthink or get creative with the implementation - If you're given step-by-step instructions, follow them exactly as written - If instructions are ambiguous or unclear, STOP and report Blockers for the parent (Skywalker) +- Background shells are forbidden (background: true starts are uncollectable without shell_collect, which is not mounted) — use foreground calls with timeouts only **When to STOP and Report Blockers** diff --git a/src/agent/directors/migrator/package.test.ts b/src/agent/directors/migrator/package.test.ts index ad13c8fe2..c6b6ee339 100644 --- a/src/agent/directors/migrator/package.test.ts +++ b/src/agent/directors/migrator/package.test.ts @@ -90,6 +90,7 @@ describe("migratorPackage", () => { "list_agents", "search_agents", "wait_agents", + "shell_collect", ] as const) { expect(allow).not.toContain(verb); } diff --git a/src/agent/directors/tool-sets.test.ts b/src/agent/directors/tool-sets.test.ts index 2c9443963..1f3eeb4fc 100644 --- a/src/agent/directors/tool-sets.test.ts +++ b/src/agent/directors/tool-sets.test.ts @@ -138,6 +138,7 @@ describe("REVIEW_TOOLS / INTERN_TOOLS", () => { expect(INTERN_TOOLS).toContain("run_shell"); expect(INTERN_TOOLS).toContain("read_file"); expect(INTERN_TOOLS).toContain("list_dir"); + expect(INTERN_TOOLS as readonly string[]).not.toContain("shell_collect"); for (const name of [ "grep", "search_files", diff --git a/src/plugins/shell-guard-plugin.test.ts b/src/plugins/shell-guard-plugin.test.ts index 564e44327..6f1c82ea7 100644 --- a/src/plugins/shell-guard-plugin.test.ts +++ b/src/plugins/shell-guard-plugin.test.ts @@ -610,6 +610,23 @@ describe("advertiseShellGuardTimeout", () => { /does not change the retained shell cwd/i, ); }); + + test("omits background when collect is not mounted", () => { + const rewritten = advertiseShellGuardTimeout( + runShellDef(), + undefined, + undefined, + false, + ); + const properties = rewritten.inputSchema["properties"] as Record< + string, + unknown + >; + expect(properties["background"]).toBeUndefined(); + expect(timeoutSchema(rewritten)?.description).not.toMatch( + /background:true/, + ); + }); }); describe("shellGuardPlugin", () => { diff --git a/src/plugins/shell-guard-plugin.ts b/src/plugins/shell-guard-plugin.ts index 4a81ad949..355c93a52 100644 --- a/src/plugins/shell-guard-plugin.ts +++ b/src/plugins/shell-guard-plugin.ts @@ -89,12 +89,15 @@ export function formatShellTimeoutNotice(timeoutMs: number): string { * Schema `default` is the foreground omit path. Description says omit * timeout on background:true so the model does not copy 120000 onto * background calls (a copied value becomes a requested timeout and kills - * the job). + * the job). Surfaces that do not mount shell_collect pass + * advertiseBackground=false so background is not advertised without a + * collect path. */ export function advertiseShellGuardTimeout( definition: ToolDefinition, defaultMs?: number, maxMs?: number, + advertiseBackground = true, ): ToolDefinition { if (definition.name !== "run_shell") return definition; const schema = definition.inputSchema; @@ -120,7 +123,9 @@ export function advertiseShellGuardTimeout( ) { nextProperties["timeout"] = { ...(timeout as Record), - description: `Timeout in milliseconds (foreground default: ${advertisedDefault}; omit on background:true for no timeout)`, + description: advertiseBackground + ? `Timeout in milliseconds (foreground default: ${advertisedDefault}; omit on background:true for no timeout)` + : `Timeout in milliseconds (foreground default: ${advertisedDefault})`, default: advertisedDefault, }; } @@ -131,14 +136,18 @@ export function advertiseShellGuardTimeout( "Optional working directory for this call only (does not change the session shell cwd retained across calls)", }; } - nextProperties["background"] = { - type: "boolean", - description: - "Set true to run without holding the turn open (prefer this for builds, test suites, and dev servers). " + - "Returns a shell_id immediately; the exit status and output are delivered when the process finishes. " + - "Use shell_collect to collect or cancel. Omit timeout for no timeout. " + - "Does not change the retained shell cwd.", - }; + if (advertiseBackground) { + nextProperties["background"] = { + type: "boolean", + description: + "Set true to run without holding the turn open (prefer this for builds, test suites, and dev servers). " + + "Returns a shell_id immediately; the exit status and output are delivered when the process finishes. " + + "Use shell_collect to collect or cancel. Omit timeout for no timeout. " + + "Does not change the retained shell cwd.", + }; + } else { + delete nextProperties["background"]; + } return { ...definition, inputSchema: { diff --git a/src/shell/background-shell.test.ts b/src/shell/background-shell.test.ts index 8bdda13ba..1a6163b0f 100644 --- a/src/shell/background-shell.test.ts +++ b/src/shell/background-shell.test.ts @@ -5,6 +5,7 @@ import { randomUUID } from "node:crypto"; import { MAX_COMPLETED_BACKGROUND_SHELLS, MAX_RUNNING_BACKGROUND_SHELLS, + MAX_SHELL_COLLECT_WAIT_MS, createBackgroundShellRegistry, } from "./background-shell.js"; @@ -22,6 +23,10 @@ async function waitUntilGone(token: string): Promise { } describe("background shell registry", () => { + test("default collect wait cap is 300s", () => { + expect(MAX_SHELL_COLLECT_WAIT_MS).toBe(300_000); + }); + test("start returns a handle immediately while the process runs", async () => { const registry = createBackgroundShellRegistry(); const started = registry.start({ @@ -245,4 +250,68 @@ describe("background shell registry", () => { registry.disposeAll("test done"); } }); + + test("collect wait is capped so a huge wait_ms cannot park unbounded", async () => { + const registry = createBackgroundShellRegistry({ maxCollectWaitMs: 80 }); + const started = registry.start({ + command: "sleep 30", + cwd: tmpCwd, + }); + if ("error" in started) throw new Error(started.error); + try { + const t0 = Date.now(); + const snapshot = await registry.collect(started.id, 30_000); + const elapsed = Date.now() - t0; + expect(snapshot.state).toBe("running"); + expect(elapsed).toBeLessThan(2_000); + } finally { + registry.disposeAll("test done"); + } + }); + + test("non-finite wait_ms is still capped", async () => { + const registry = createBackgroundShellRegistry({ maxCollectWaitMs: 80 }); + const started = registry.start({ + command: "sleep 30", + cwd: tmpCwd, + }); + if ("error" in started) throw new Error(started.error); + try { + const t0 = Date.now(); + const snapshot = await registry.collect( + started.id, + Number.POSITIVE_INFINITY, + ); + const elapsed = Date.now() - t0; + expect(snapshot.state).toBe("running"); + expect(elapsed).toBeLessThan(2_000); + } finally { + registry.disposeAll("test done"); + } + }); + + test("collect completes when the child exits even if stdio stays open", async () => { + if (process.platform === "win32") return; + const token = `ic_bg_stdio_hold_${randomUUID()}`; + const registry = createBackgroundShellRegistry(); + // Grandchild inherits the piped stdout so Node's 'close' waits on it. + // The shell itself exits immediately; collect must not require close. + const started = registry.start({ + command: `bash -c 'exec -a ${token} sleep 600 & exit 0'`, + cwd: tmpCwd, + }); + if ("error" in started) throw new Error(started.error); + try { + const t0 = Date.now(); + const exited = await registry.collect(started.id, 5_000); + expect(Date.now() - t0).toBeLessThan(2_000); + expect(exited.state).toBe("completed"); + if (exited.state !== "completed") return; + expect(exited.exit.exitCode).toBe(0); + } finally { + registry.disposeAll("test done"); + spawnSync("pkill", ["-f", token]); + await waitUntilGone(token); + } + }); }); diff --git a/src/shell/background-shell.ts b/src/shell/background-shell.ts index f30ae698c..850d8376b 100644 --- a/src/shell/background-shell.ts +++ b/src/shell/background-shell.ts @@ -14,6 +14,10 @@ import { export const MAX_RUNNING_BACKGROUND_SHELLS = 8; export const MAX_COMPLETED_BACKGROUND_SHELLS = 8; +export const MAX_SHELL_COLLECT_WAIT_MS = 300_000; +// Prefer close so trailing stdio is captured, but never require it: a grandchild +// holding the pipe must not park collect after the child has already exited. +const STDIO_DRAIN_MS = 100; /** * Signals the whole process group. With detached:true the shell is the group @@ -75,10 +79,23 @@ export interface BackgroundShellRegistry { runningCount: () => number; } +function resolveCollectWaitMs(waitMs: number, maxMs: number): number { + if (typeof waitMs !== "number" || Number.isNaN(waitMs) || waitMs <= 0) { + return 0; + } + if (!Number.isFinite(waitMs)) return maxMs; + return Math.min(waitMs, maxMs); +} + export function createBackgroundShellRegistry( - options: { onExit?: (exit: BackgroundShellExit) => void } = {}, + options: { + onExit?: (exit: BackgroundShellExit) => void; + maxCollectWaitMs?: number; + } = {}, ): BackgroundShellRegistry { const { onExit } = options; + const maxCollectWaitMs = + options.maxCollectWaitMs ?? MAX_SHELL_COLLECT_WAIT_MS; const running = new Map(); const completed = new Map(); const exitWaiters = new Map void>>(); @@ -150,10 +167,16 @@ export function createBackgroundShellRegistry( if (timer !== undefined) clearTimeout(timer); finish(1, false); }); - child.on("close", (code, sig) => { + child.on("exit", (code, sig) => { if (timer !== undefined) clearTimeout(timer); if (!running.has(id)) return; - finish(code ?? (sig !== null ? 128 : 1), false); + const exitCode = code ?? (sig !== null ? 128 : 1); + const settle = (): void => finish(exitCode, false); + const grace = setTimeout(settle, STDIO_DRAIN_MS); + child.once("close", () => { + clearTimeout(grace); + settle(); + }); }); return { id }; }; @@ -166,7 +189,8 @@ export function createBackgroundShellRegistry( const done = completed.get(id); if (done !== undefined) return { state: "completed", exit: done }; if (!running.has(id)) return { state: "not-found" }; - if (waitMs > 0) { + const budget = resolveCollectWaitMs(waitMs, maxCollectWaitMs); + if (budget > 0) { // An already-aborted collect releases immediately as still-running: // interrupt must not park the session on a live descendant, and must // not kill it either — the child belongs to the still-alive session. @@ -185,7 +209,7 @@ export function createBackgroundShellRegistry( forget(); signal?.removeEventListener("abort", onAbort); resolve(); - }, waitMs); + }, budget); const onAbort = (): void => { clearTimeout(timer); forget(); diff --git a/src/subagent/run-persist-close.test.ts b/src/subagent/run-persist-close.test.ts index 24250ed4b..ca3f3c494 100644 --- a/src/subagent/run-persist-close.test.ts +++ b/src/subagent/run-persist-close.test.ts @@ -1,8 +1,12 @@ /** * Persist close_agent must surface a leftover-child posix dispose, not treat - * it as a successful bounded close. + * it as a successful bounded close. Intern persist (no shell_collect) must + * still disposeAll leftover registry children even though the session stays + * retained. */ import { describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { randomUUID } from "node:crypto"; import { mkdtemp } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -10,7 +14,10 @@ import { join } from "node:path"; import type { ReactorEmittedEvent } from "@intx/inference"; import { withMockedModuleDuring } from "../../tests/helpers/mock-module.js"; +import { defined } from "../../tests/helpers/defined.js"; +import { INTERN_TOOLS } from "../agent/directors/tool-sets.js"; import { createPermissionGate } from "../permission/gate.js"; +import type { BackgroundShellRegistry } from "../shell/background-shell.js"; import type { RunSubAgentParams } from "./types.js"; const permissionGate = createPermissionGate({ @@ -45,6 +52,28 @@ function stubAgent() { }; } +/** Poll until a process carries `token`; fail if it never becomes visible. */ +async function waitUntilPresent(token: string): Promise { + const started = Date.now(); + while (Date.now() - started < 5_000) { + const probe = spawnSync("pgrep", ["-f", token], { encoding: "utf8" }); + if ((probe.stdout?.trim() ?? "").length > 0) return; + await new Promise((r) => setTimeout(r, 50)); + } + throw new Error(`tagged child never appeared: ${token}`); +} + +/** Poll until no process carries `token`; fail instead of asserting on a pid. */ +async function waitUntilGone(token: string): Promise { + const started = Date.now(); + while (Date.now() - started < 5_000) { + const probe = spawnSync("pgrep", ["-f", token], { encoding: "utf8" }); + if ((probe.stdout?.trim() ?? "").length === 0) return; + await new Promise((r) => setTimeout(r, 50)); + } + throw new Error(`tagged child still alive after 5s: ${token}`); +} + describe("persist close_agent leftover dispose", () => { test("onAgentReady close rejects when posix dispose reports leftover children", async () => { const cwd = await mkdtemp(join(tmpdir(), "corbits-persist-close-")); @@ -242,3 +271,112 @@ describe("persist close_agent leftover dispose", () => { ); }); }); + +describe("intern persist reaps leftover registry children when collect is unmounted", () => { + test("a leftover background child is disposeAll'd even though the intern session is retained", async () => { + expect(INTERN_TOOLS as readonly string[]).not.toContain("shell_collect"); + const cwd = await mkdtemp(join(tmpdir(), "corbits-intern-persist-reap-")); + const token = `ic_intern_persist_${randomUUID()}`; + let registry: BackgroundShellRegistry | undefined; + const disposeReasons: string[] = []; + let leftoverId: string | undefined; + + await withMockedModuleDuring( + import.meta.resolve("../shell/background-shell.js"), + (real: typeof import("../shell/background-shell.js")) => ({ + ...real, + createBackgroundShellRegistry: ( + opts: Parameters[0], + ) => { + const inner = real.createBackgroundShellRegistry(opts); + const wrapped: BackgroundShellRegistry = { + ...inner, + disposeAll: (reason: string) => { + disposeReasons.push(reason); + inner.disposeAll(reason); + }, + }; + registry = wrapped; + return wrapped; + }, + }), + async () => + withMockedModuleDuring( + import.meta.resolve("../agent/live-tool-dispatch.js"), + (real: typeof import("../agent/live-tool-dispatch.js")) => ({ + ...real, + createAgentWithLiveToolDispatch: async () => + ({ + ...stubAgent(), + send: async () => { + const captured = defined(registry); + const started = captured.start({ + // Token must be argv/process-title, not a shell comment: + // pgrep -f only sees the exec'd sleep, so a comment leak + // would make waitUntilGone succeed even if kill failed. + command: `bash -c 'exec -a ${token} sleep 600'`, + cwd, + }); + if ("error" in started) throw new Error(started.error); + leftoverId = started.id; + expect(captured.runningCount()).toBe(1); + if (process.platform !== "win32") { + await waitUntilPresent(token); + } + return { + type: "reply" as const, + reply: "done", + turn: { role: "assistant", content: [] }, + }; + }, + }) as unknown as Awaited< + ReturnType + >, + }), + async () => { + const { runSubAgent } = await import("./run.js"); + let handles: + | { + close: (deadlineMs?: number) => Promise; + } + | undefined; + const params: RunSubAgentParams = { + cwd, + workdirBase: join(cwd, ".ctx"), + permissionGate, + provider: { + providerName: "test", + baseURL: "http://localhost", + model: "test-model", + }, + description: "intern persist leftover registry probe", + prompt: "finish the first turn", + persist: true, + directorId: "intern", + capabilities: { mode: "allow", tools: [...INTERN_TOOLS] }, + onAgentReady: (h) => { + handles = h; + }, + }; + try { + const result = await runSubAgent(params); + expect(result.agentRetained).toBe(true); + expect(disposeReasons).toEqual(["sub-agent closed"]); + const captured = defined(registry); + expect(captured.runningCount()).toBe(0); + const leftover = await captured.collect(defined(leftoverId), 0); + expect(leftover.state).toBe("not-found"); + if (process.platform !== "win32") { + await waitUntilGone(token); + } + } finally { + registry?.disposeAll("test done"); + if (handles !== undefined) { + await handles.close(1000).catch(() => undefined); + } + } + }, + ), + ); + }); +}); diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 9805e40df..3f37ad5a8 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -632,6 +632,10 @@ async function runSubAgentInner( const backgroundShells = createBackgroundShellRegistry({ onExit: (exit) => backgroundExitSink?.(exit), }); + // Intern / migrator (and any surface that filters out shell_collect) must + // not spawn background children they cannot collect. The getter is live so + // the capability filter below can unwire it before the first tool call. + let backgroundCollectMounted = true; // Child tools resolve spills against the child's own store first, then // the parent's: parent tool-output:// URIs handed in the brief must // remain readable after spawn, and the child's own spills stay local. @@ -658,7 +662,8 @@ async function runSubAgentInner( : {}), ...(params.shellEnv !== undefined ? { shellEnv: params.shellEnv } : {}), readFileGuard: { blobReader: sessionBlobReader }, - getBackgroundShellRegistry: () => backgroundShells, + getBackgroundShellRegistry: () => + backgroundCollectMounted ? backgroundShells : undefined, getShellOutputFeeds: () => childShellOutputFeed, getBlobWriter: () => childBlobWriter, getContextDir: () => childContextDir, @@ -802,6 +807,26 @@ async function runSubAgentInner( if (params.capabilities !== undefined) { tools = applyCapabilityFilter(tools, params.capabilities); } + backgroundCollectMounted = tools.some( + (tool) => tool.definition.name === "shell_collect", + ); + if (!backgroundCollectMounted) { + tools = tools.map((tool) => + tool.definition.name === "run_shell" + ? { + ...tool, + definition: advertiseEditFileLineRange( + advertiseShellGuardTimeout( + tool.definition, + shellTimeout?.defaultMs, + shellTimeout?.maxMs, + false, + ), + ), + } + : tool, + ); + } // Every sub-agent is an agent: multi-step jobs get their own manage_tasks // checklist. The handler is local to this loop; parent and child never share @@ -1386,6 +1411,11 @@ async function runSubAgentInner( // Aborting the send signal only rejects the promise; the child reactor keeps // running until close() (same hard-stop rule as the parent in runner.ts). closeOnAbort = (): void => { + // Parent abort / deadline / close_agent: unpark shell_collect waiters so + // a worker blocked on collect cannot wedge teardown. interrupt_agent is + // the keep-alive path (releaseWaiters only, children stay). + backgroundShells.releaseWaiters(); + backgroundShells.disposeAll("parent abort"); void (async () => { try { await agent?.close(); @@ -1415,6 +1445,7 @@ async function runSubAgentInner( ): Promise => { if (!runController.signal.aborted) runController.abort(new Error("closed by close_agent")); + backgroundShells.disposeAll("closed by close_agent"); try { await awaitBoundedTeardown( disposeSubAgentSession({ @@ -1736,8 +1767,10 @@ async function runSubAgentInner( // A persisted, cleanly-completed session skips teardown here — it // stays open until close_agent (or a later failed/aborted run) tears it // down. - if (!persisting) { + if (!persisting || !backgroundCollectMounted) { backgroundShells.disposeAll("sub-agent closed"); + } + if (!persisting) { // Bounded like close_agent: a session teardown wedged by a live shell // descendant fails the run instead of parking it forever. await awaitBoundedTeardown( diff --git a/src/tui/tool-execution-watchdog.ts b/src/tui/tool-execution-watchdog.ts index c34805d57..d158ebb3b 100644 --- a/src/tui/tool-execution-watchdog.ts +++ b/src/tui/tool-execution-watchdog.ts @@ -116,7 +116,7 @@ export function resolveToolExecutionTimeoutMs( call?.name === "spawn_agent" || call?.name === "wait_agents" || call?.name === "ask_director" || - // shell_collect with wait_ms is a bounded poll over a background shell that + // shell_collect with wait_ms is a capped poll over a background shell that // outlives the turn; aborting the collect would not stop the process. call?.name === "shell_collect" ) {