Skip to content

Commit 29cbd8c

Browse files
committed
refactor(shell): remove the unused background shell collect and waiter API
Nothing polls a background shell since shell_collect was removed: results arrive through onExit and run_shell stop cancels. Drop collect, releaseWaiters, the completed-run ring, and the collect wait cap, and move the registry tests onto onExit.
1 parent b3e1b98 commit 29cbd8c

8 files changed

Lines changed: 89 additions & 336 deletions

‎docs/IMPLEMENTATION.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ src/
123123
permission-plugin.ts Tiered operator approval (owns catastrophic shell deny)
124124
shell/
125125
run-shell-authz.ts Shared run_shell deny policy (gate-enforced)
126-
background-shell.ts Background run_shell registry (start/collect/cancel/disposeAll)
126+
background-shell.ts Background run_shell registry (start/cancel/disposeAll)
127127
verify-plugin.ts Write/edit verification (per-path lock)
128128
file-mutation-lock.ts Serialize mutations per file for verify
129129
lsp-hint-plugin.ts TS/JS LSP setup hint on unavailable server
@@ -208,7 +208,7 @@ Listings are list-free, dumps are dump-locked: a bounded `ls`/`tree` prints name
208208

209209
#### Background shell mode
210210

211-
`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-<id>` blob named in the completion message). Completion is process-exit, not stdio-close: a grandchild holding the pipe does not delay 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). `run_shell` with `stop: <shell_id>` kills the process group; it is ungated by design (cancel only kills the session's own child), and a finished run needs no poll because its result is delivered on its own. Timeout keeps its meaning: expiry kills the group and reports exit code 124 with `timed_out: true`. The tool watchdog exempts background starts because the process outlives the turn. A worker whose capability filter drops `run_shell` has no background registry wired — background starts fail closed. Parent abort, deadline, and `close_agent` release parked registry 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.
211+
`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-<id>` blob named in the completion message). Completion is process-exit, not stdio-close: a grandchild holding the pipe does not delay 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). `run_shell` with `stop: <shell_id>` kills the process group; it is ungated by design (cancel only kills the session's own child), and a finished run needs no poll because its result is delivered on its own. Timeout keeps its meaning: expiry kills the group and reports exit code 124 with `timed_out: true`. The tool watchdog exempts background starts because the process outlives the turn. A worker whose capability filter drops `run_shell` has no background registry wired — background starts fail closed. Parent abort, deadline, and `close_agent` dispose live background groups; `interrupt_agent` leaves them 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.
212212

213213
- **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.
214214

‎src/plugins/shell-guard-plugin.test.ts‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { spawnSync, type ChildProcess } from "node:child_process";
1010
import { randomUUID } from "node:crypto";
1111

1212
import { createBackgroundShellRegistry } from "../shell/background-shell.js";
13+
import { createExitRecorder } from "../shell/background-shell-test-harness.js";
1314
import { createShellOutputFeed } from "../session/shell-output-feed.js";
1415

1516
import {
@@ -267,7 +268,10 @@ describe("background run_shell (shellGuardPlugin)", () => {
267268
}
268269

269270
test("background:true returns a running handle immediately", async () => {
270-
const registry = createBackgroundShellRegistry();
271+
const recorder = createExitRecorder();
272+
const registry = createBackgroundShellRegistry({
273+
onExit: recorder.onExit,
274+
});
271275
const start = Date.now();
272276
const result = await runWith(registry, {
273277
id: "bg1",
@@ -283,8 +287,7 @@ describe("background run_shell (shellGuardPlugin)", () => {
283287
expect(parsed.shell_id.length).toBeGreaterThan(0);
284288
// The START call resolved while the child was still sleeping.
285289
expect(Date.now() - start).toBeLessThan(400);
286-
const snapshot = await registry.collect(parsed.shell_id, 5_000);
287-
expect(snapshot.state).toBe("completed");
290+
await recorder.waitFor(parsed.shell_id);
288291
registry.disposeAll("test done");
289292
});
290293

@@ -334,7 +337,10 @@ describe("background run_shell (shellGuardPlugin)", () => {
334337
});
335338

336339
test("background omitted timeout skips the foreground default", async () => {
337-
const registry = createBackgroundShellRegistry();
340+
const recorder = createExitRecorder();
341+
const registry = createBackgroundShellRegistry({
342+
onExit: recorder.onExit,
343+
});
338344
const handler = defined(
339345
shellGuardPlugin(process.cwd(), { defaultMs: 90 }, undefined, {
340346
getBackgroundShellRegistry: () => registry,
@@ -353,12 +359,9 @@ describe("background run_shell (shellGuardPlugin)", () => {
353359
);
354360
expect(result.isError).toBeUndefined();
355361
const parsed = JSON.parse(String(result.content)) as { shell_id: string };
356-
const snapshot = await registry.collect(parsed.shell_id, 5_000);
357-
expect(snapshot.state).toBe("completed");
358-
if (snapshot.state === "completed") {
359-
expect(snapshot.exit.timedOut).toBe(false);
360-
expect(snapshot.exit.output).toContain("survived");
361-
}
362+
const exit = await recorder.waitFor(parsed.shell_id);
363+
expect(exit.timedOut).toBe(false);
364+
expect(exit.output).toContain("survived");
362365
registry.disposeAll("test done");
363366
});
364367

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import type { BackgroundShellExit } from "./background-shell.js";
2+
3+
/** Collects `onExit` deliveries so tests can await one shell's completion. */
4+
export function createExitRecorder(): {
5+
onExit: (exit: BackgroundShellExit) => void;
6+
waitFor: (id: string, timeoutMs?: number) => Promise<BackgroundShellExit>;
7+
} {
8+
const exits = new Map<string, BackgroundShellExit>();
9+
return {
10+
onExit: (exit) => {
11+
exits.set(exit.id, exit);
12+
},
13+
waitFor: async (id, timeoutMs = 5_000) => {
14+
const deadline = Date.now() + timeoutMs;
15+
while (Date.now() < deadline) {
16+
const exit = exits.get(id);
17+
if (exit !== undefined) return exit;
18+
await new Promise((resolve) => setTimeout(resolve, 20));
19+
}
20+
throw new Error(
21+
`no exit delivered for shell ${id} within ${timeoutMs}ms`,
22+
);
23+
},
24+
};
25+
}

0 commit comments

Comments
 (0)