Skip to content

Commit 1f563f5

Browse files
committed
fix(agent): keep background bash for workers and wait on MCP for skill hits
Workers gated background on the removed shell_collect tool, so they lost detached bash. They now mount background whenever run_shell is mounted. tool_search waits for pending connectors even when a skill matched. Drop the remaining shell_collect and skywalker/critic/counsel references and make the dispatch card tool-neutral across model families.
1 parent 5634a90 commit 1f563f5

33 files changed

Lines changed: 97 additions & 191 deletions

‎docs/ARCHITECTURE.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,7 @@ tool call
414414
- **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.
415415
- **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.
416416
- **Permission** (`permission-plugin.ts`) — Delegates consequential calls to the permission gate.
417-
- **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.
417+
- **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 `run_shell stop=<shell_id>` cancels (schema advertised by `advertiseShellGuardTimeout` whenever a background registry is wired; 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.
418418
- **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 to diagnose blocked PDF inspection (missing `pdftotext` vs installed-but-worker-cannot-execute vs malformed/unreadable file, each with one next action), and stops at an 8MB scan ceiling. Emits `offset` continuation notices so the model can page without losing file or spill content on disk.
419419
- **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.
420420
- **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).
@@ -439,7 +439,7 @@ tool call
439439

440440
**Approval log** (`src/permission/approval-log.ts`, CL-5666): every consequential decision the gate makes — auto-mode allow/deny or an interactive prompt's allow-once/allow-with-scope/deny/timeout/abort — is appended as one JSONL record to `approvals.jsonl` in the session dir, carrying the classifier/auto-shell rule name that fired (the existing `auto-shell-policy.ts`/`classify.ts` rule names, plus a small closed set of additional fixed literals the log itself defines for decisions those modules don't otherwise name — `auto-allowed-tool`, `non-interactive`, `mega-chain` — never model- or user-authored text), whether the decision was `auto` or `interactive`, a shell chain's segment count, and queued/displayed/settled timestamps. `displayedAt` is set by `PermissionRequest.markDisplayed`, called from `gate-wire.ts`'s `open()` the moment a request actually reaches the overlay host — distinct from when it was raised, so the gap it exposes is the CL-5664 signal (a queued gate arming its timeout before the operator could see it). No command text, file content, path, credential, or other free text is ever recorded — only tool name, rule, mode, segment count, and timing; a sub-agent's free-text dispatch label is deliberately left out, even though it would enable a per-agent breakdown, because nothing constrains what a model puts in it. A hard size cap on the serialized line is defense in depth against a future field reintroducing free text. Writes are fire-and-forget and swallow their own errors; the log defaults to a no-op so nothing depends on it being wired. `scripts/approval-forensics.ts` aggregates across local sessions the same way `intervention-forensics.ts` does for stop/nudge events: per-tool counts by outcome and mode, duration/display-delay percentiles, mega-chain counts, and a duplicate-rate proxy (sessions that hit the same rule more than once).
441441

442-
**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline still runs until the parent turn's stall budget fires, the operator cancels, or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, `shell_collect`, and `run_shell` with `background: true` stay exempt from the per-tool watchdog. On TUI, an in-flight `shell_collect` or `ask_director` is still bounded by the stall watchdog — including after a tool batch resolves and after compact continuation re-entry — so a typical poll does not pin the turn. Concurrent same-name `shell_collect` calls still share one `callIdByName` slot (one id per name); the leftover stays named in the per-id `callNameById` map so the stall watchdog still bounds it after the mapping-owning sibling finishes. `wait_agents` is exec-primary only and is not mounted on TUI. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool.
442+
**Tool wall-clock budget vs. permission prompts.** Each tool `run()` is wrapped by an outer execution watchdog (`src/tui/tool-execution-watchdog.ts`). The watchdog arms when Settings set `tools.timeoutMs` / `tools.maxTimeoutMs`, when foreground `run_shell` has an effective timeout (120s default or per-call, plus 1000ms slack so this layer cannot beat shell-guard), or for `mcp__*` calls. Fleet wait tools are exempt, regardless of Settings: the generic per-tool budget never aborts a sub-agent run while the parent waits for it. That exemption is unconditional, not because the worker is otherwise bounded — there is no turn budget; `deadlineMs` is opt-in, and there is no no-progress or thrash stop. A stuck worker that never trips stall or deadline still runs until the parent turn's stall budget fires, the operator cancels, or, in eval mode, `--agent-timeout-ms` bounds it. `spawn_agent`, `wait_agents`, `ask_director`, and `run_shell` with `background: true` stay exempt from the per-tool watchdog. On TUI, an in-flight `ask_director` is still bounded by the stall watchdog — including after a tool batch resolves and after compact continuation re-entry — so a typical poll does not pin the turn. Concurrent same-name calls still share one `callIdByName` slot (one id per name); the leftover stays named in the per-id `callNameById` map so the stall watchdog still bounds it after the mapping-owning sibling finishes. `wait_agents` is exec-primary only and is not mounted on TUI. By default (`tools.waitForApproval`, Settings → Tools, **On**), an armed budget freezes while the operator is deciding on a permission prompt, so a late approve still runs the tool and the agent waits for the decision instead of timing out under the modal. When **Off**, the budget keeps ticking during the prompt; if it expires first the tool is skipped and the permission modal is dismissed via the budget AbortSignal (auto-deny with a timeout message). The TUI permission queue (`src/tui/gate-wire.ts`, backed by `src/permission/queue.ts`) attaches that signal so ghost prompts cannot outlive an already-aborted tool.
443443

444444
`mcp__*` tool calls are the exception to "arms only when Settings set it": they arm unconditionally with a 5-minute default (`DEFAULT_MCP_TOOL_TIMEOUT_MS`), overridable via `mcp.timeoutMs` and still capped by `tools.maxTimeoutMs` (CL-6895). Nothing else bounds an MCP call — the stall watchdog treats an in-flight tool as activity by design, so a wedged MCP server previously hung a tool call, and the turn, forever. On expiry the call returns a normal tool-error result ("MCP tool `<name>` timed out after `<n>`s — the server may be wedged; retry or continue without it"); the turn is never aborted. The MCP client itself (`src/mcp/client.ts`, wrapping `@modelcontextprotocol/sdk`) multiplexes concurrent requests over one connection by JSON-RPC message id with no serial queue or mutex in our code or in the vendored SDK's `Protocol.request()` — so concurrent calls to the same server are not expected to deadlock each other. Live forensics for CL-6895 showed multi-minute MCP calls that eventually completed successfully, consistent with a slow server response rather than a client-side deadlock.
445445

0 commit comments

Comments
 (0)