chore(release): 0.7.10 — confined-shell bounding + brush-ocap-core 0.5.1 - #285
Merged
Conversation
`run_in_brush` built the fds map with only STDOUT_FD/STDERR_FD, so brush defaulted STDIN_FD to the real `std::io::stdin()`. A confined `cat`/`wc`/ `grep`/`sort` with no pipe then read the OPERATOR'S TERMINAL — hanging the turn, stealing keystrokes, and corrupting MCP stdio. Insert `openfiles::null()` (cross-platform: /dev/null on unix, NUL on Windows) as STDIN_FD so stdin-readers get immediate EOF, mirroring how the safe-subset engine hands spawned children `Stdio::null()`. This only redirects stdin to a sink; every external command still passes `before_exec` admission unchanged, so the per-command OCAP guarantee holds. Test: a confined `/bin/cat` returns promptly with empty stdout + exit 0; with a never-EOF stdin and the fix removed it blocks past the 10s ceiling (RED), proving the test's teeth. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The brush interpreter runs on `spawn_blocking`, which tokio cannot cancel by
dropping the future — the worker detaches and keeps grinding ("can't exit").
Add a per-run cancellation flag observed *inside* the run so an outer caller
can stop it promptly.
Mechanism:
- `CaveatInterceptor` gains an `Arc<AtomicBool>` cancel flag (wired per-run in
`invoke` via `with_cancel`). `before_exec`/`before_open` check it FIRST, at
every external-spawn and file-open boundary.
- On cancel the hook raises a private `BrushCancelled` panic. A returned
`Deny` is insufficient: brush's interpreter SWALLOWS a denial (converts it
to a failed-command exit and the enclosing `while`/`for` loop keeps running),
so unwinding is the only in-process seam that stops the interpreter. brush
performs no `catch_unwind` on the exec path, so the unwind reaches `block_on`.
- `run_in_brush` wraps the interpreter in `catch_unwind`: the `BrushCancelled`
sentinel becomes a clean cancellation error and the drain threads (whose
pipe writers dropped during the unwind) are joined, so the blocking worker
FINISHES — no leaked thread, no wait-out-the-runaway (report open-Q #4). Any
other panic is re-raised unchanged.
- A once-installed panic hook silences the sentinel's stderr report so an
expected cancel never spews a panic onto the operator's terminal.
OCAP preserved: cancellation only REFUSES further spawns/opens (recorded as a
structured denial) — it can never allow an un-admitted program.
Tests: a pre-tripped flag aborts at the first external command and records an
exec denial; tripping the flag mid-run stops an otherwise-infinite loop
promptly (0.18s vs a 5s leak deadline) with the worker returning cleanly.
Disabling the observation makes the loop run to the deadline (RED).
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The confined brush path had NO wall-clock timeout and hardcoded `.with_timed_out(false)`, so a grinding or blocking confined command ran unbounded and could never raise the timeout signal. - Add a `timeout` field to `BrushShellTool` (builder `with_timeout`), defaulted from the shared shell-limits contract (`LimitsPolicy::default_timeout_secs`, 60s) like the safe-subset and host engines. - Wrap the `spawn_blocking` run in `tokio::time::timeout`. On elapse, TRIP the FIX-2 cancel flag so the detached worker observes it at the next command boundary and unwinds (a bare timeout would leave it grinding on the blocking pool), then emit `timed_out:true` + exit 124, mirroring the safe-subset / host timeout envelopes. An already-spawned long child (e.g. `sleep 30`) is not itself killed here — that needs kill-on-drop in the brush fork (Effort B) — but the operator recovers immediately at the ceiling. OCAP is untouched: the timeout only STOPS execution (via a fail-closed cancel), it never admits a new program. Test: a confined `sleep` with a 1s ceiling returns at ~1s with `timed_out:true` / exit 124; raising the ceiling above the command duration lets it complete with `timed_out:false` / exit 0 (RED). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… on timeout Adversarial-review follow-up (findings #4/#5 + comment accuracy). FIX 4 [HIGH] — blocking-pool exhaustion via drain join. `run_in_brush` joined the stdout/stderr drain threads, which block for the ENTIRE lifetime of any background child that inherited a dup of the write pipe (brush hands each child a real dup(2); no kill-on-drop). So `sleep 300 & echo hi` pinned the spawn_blocking WORKER for 300s; `tail -f &` forever — repeat and tokio's 512- thread blocking pool saturates and every future confined invoke hangs. The drains now report over a channel; `collect_drained` bounded-waits (DRAIN_DETACH_DEADLINE, 500ms) then DETACHES — a cheap leaked OS drain thread that self-terminates when the child finally closes the pipe, instead of pinning a scarce worker. The cancel/timeout unwind path no longer joins at all. Common case is unaffected: a finished run's drains EOF immediately and report their full output well under the deadline. Tested directly (deterministic) rather than through brush's real `&`, which is inherently racy on the per-run current-thread runtime (finding #7 / Effort B): a held-open writer forces detach at the deadline (RED: a blocking recv hangs); a closed writer returns full output promptly. FIX 5 [LOW] — the timeout branch built its envelope without reading the denial sink, dropping leash denials recorded before the timeout. It now snapshots the sink like the Ok branch, so denial telemetry survives a timed-out run. Comment/doc accuracy (no behavior change): - The timeout comment no longer claims a runaway is "BOUNDED": the OPERATOR recovers at the ceiling, but the worker/loop is only actually STOPPED if the interpreter next hits an exec/open boundary — a pure-builtin loop, a `wait`, or a blocking fifo read are NOT stopped and leak a thread until process exit (needs a fork per-iteration hook + kill-on-drop: Effort B). - Narrowed the "no catch_unwind on the execution path" claim: a BrushCancelled raised inside a `$(...)`/`&`/coproc subtask is caught at the tokio task boundary as a JoinError, not by run_in_brush's catch_unwind — harmless for today's timeout path, but a future newt-interrupt path must not rely on it. - Documented that the cancel seam REQUIRES panic = "unwind"; under panic = "abort" a routine timeout becomes a whole-process SIGABRT. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
`before_exec` called `cx.check_exec(program)` fresh on every spawn, so a confined loop recomputed the identical admission thousands of times (`while read f; do wc -l "$f"; done` → N identical `/usr/bin/wc` checks). Add a per-invocation allow memo that short-circuits `Allow` for a program already admitted under this invocation's caveats. Security invariant — this is a pure memo, never an authority: * The key is the whole admission input. `check_exec` is a function of exactly `(cx.effective.exec, program)`, and `cx` is fixed for the invocation (`ToolContext::effective` is private with no setter), so for a fixed `cx` the answer depends on `program` alone. The memo returns the same decision the recomputation would. * One cache belongs to exactly one `cx`. The cache is minted inside `CaveatInterceptor::new` — the only place a `cx` is installed — and no constructor, setter, or accessor can inject or share one, so an `Allow` cannot bleed across invocations with different caveats. The fail-closed `Default` interceptor gets `None` and memoizes nothing. * Only `Allow` is memoized. Denials are recomputed and re-recorded every time, so the denial log and its telemetry are unchanged. * The memo is consulted STRICTLY AFTER the cancellation check. Ordering is load-bearing: the hot loop this memo speeds up is exactly the loop cancellation must be able to stop, so a memo ahead of the cancel check would make a cancelled run unstoppable from its second iteration on. Measured effect is within noise on this host — `check_exec` under `Scope::All` is already a trivial scope test — so this is a correctness- preserving cleanup of redundant work, not the fix for the confined-loop hang (see the engine-defect note in brush_shell.rs). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ine deadlock
Two things the confined-shell efficiency investigation turned up.
1. A cancellation regression guard. `tripping_cancel_stops_a_loop_of_a_
carried_coreutil` proves a loop whose body is a CARRIED util (`cat`)
is still stopped when the cancel flag is tripped, and that it leaves
exec-axis denials — i.e. it really does cross `before_exec` every
iteration. That holds today only BECAUSE the carried shim re-execs.
The moment a carried util runs in-process it stops crossing
`before_exec`, and this test fails unless the new in-process dispatch
path checks the cancel flag itself. Do not delete it when making
carried utils in-process — make it pass.
2. The measured root cause of the confined-loop "hang", recorded next to
the timeout backstop that currently masks it. A COMPOUND command
(`while`/`for`/`if`/`{…}`/subshell) used as a NON-FINAL pipeline stage
deadlocks once it writes more than one pipe buffer (64 KiB on macOS):
`interp.rs` `ExecuteInPipeline for ast::Command` runs the `Compound`
arm INLINE to completion instead of spawning it as the `Simple` arm
does, and `spawn_pipeline_processes` awaits stages in order, so the
downstream reader is not created until the compound finishes. The
compound writes into a reader-less pipe, fills it, and blocks forever.
Measured: the loop at 2000x32B (62 KiB) completes in 0.23s; at
2100x32B (65 KiB) it never completes (still running at a 240s
ceiling). It reproduces with a pure-builtin body and NO external
command, so it is not a spawn-cost problem. Below the threshold the
canonical pipeline is byte-identical to bash and already at spawn-cost
parity (100 externals: brush 0.15s vs bash 0.13s). The fix is
fork-side: the `Compound` arm must return `StartedTask` for a
non-final stage.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
`dispatch_host` always used the engine's default 60s wall-clock ceiling, so every long confined run reported the same "timed out after 60s" and a measurement could not tell "slow" from "deadlocked". Honor an optional `AB_BRUSH_TIMEOUT_SECS` override. This is what established that the compound-pipeline-stage hang is a true deadlock — the run is still going at a 240s ceiling, not merely slow. Test-support binary only; the default is unchanged when the var is unset. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Picks up the compound-pipeline deadlock fix (a `while`/`for` stage writing more than one pipe buffer into a downstream stage blocked forever), plus the per-command CommandInterceptor hook and opt-in kill-on-drop. The `^0.5` pin was already compatible; only the lock moves. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Lock-step workspace bump (all in-workspace pins move together). Ships the confined-shell hardening class: - stdin=/dev/null so a confined stdin-reader gets EOF instead of the operator's terminal (it could steal keystrokes / corrupt MCP stdio) - wall-clock timeout + a real timed_out signal (the brush path was unbounded) - a cancellation seam so a runaway confined run can actually be stopped - no blocking-pool exhaustion: the drain no longer pins a worker for the lifetime of a backgrounded child holding a pipe-writer dup - memoized per-invocation exec admission and picks up brush-ocap-core 0.5.1, which fixes the compound-pipeline deadlock: a `while`/`for` stage writing more than one pipe buffer (64 KiB) into a downstream stage blocked forever, because the compound ran inline to completion and the draining stage was never started. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Ships the confined-shell hardening class and bumps the workspace to 0.7.10 (lock-step; all in-workspace pins move together).
Supersedes #284 — that branch is contained here in full.
run_in_brushseeded only STDOUT/STDERR, so brush defaulted the child's stdin to the realstd::io::stdin(): a confinedcat/wc/grepwith no pipe read the operator's terminal, hanging the turn, stealing keystrokes, and able to corrupt MCP stdio.timed_out— the brush path had no ceiling and hardcodedtimed_out: false. The operator now always regains control (exit 124), mirroring the safe-subset and host engines.join()s the scarcespawn_blockingworker for the lifetime of a backgrounded child holding a pipe-writer dup (sleep 300 & echo hipinned a worker for 300 s;tail -f /dev/null &forever → tokio's 512-thread pool saturates and wedges every later confined invoke). A 500 ms deadline returns full output in the common case and detaches otherwise.Allowis memoized, denials always recompute, and the memo is consulted strictly after the cancel check.while/forstage writing more than one pipe buffer (64 KiB) into a downstream stage blocked forever, because the compound ran inline to completion so the draining stage was never started. Offered upstream as fix(pipeline): run compound command stages concurrently reubeno/brush#1242.Test plan
cargo build --workspace --exclude agent-bridle-py,cargo test --workspace --exclude agent-bridle-py --features brush,carried-coreutils→ 371 pass, 0 fail. (agent-bridle-pyis a PyO3 crate requiring maturin, not plaincargo build— pre-existing, unrelated.)cargo metadataresolves after the lock-step bump.Known limits (tracked, not closed here)
Cancellation is observed at exec/open boundaries, so a pure-builtin loop (
while true; do :; done) still can't be stopped and leaks a thread until process exit. brush-ocap-core 0.5.1 adds abefore_commandhook that fires for builtins too and whoseDenyterminates the run — wiring the interceptor to it closes this, and is the immediate follow-up.Out of scope
before_commandhook (next patch release).uumainwrites process fd 1 while concurrent stages run onspawn_blockingthreads).