fix: stop finite timeouts from killing legitimate work

[asto18089]

Summary

A timeout-behaviour audit (21-area sweep of the engine, judged against real usage: model inference streams for minutes, tool executions run tens of minutes, users walk away from approval prompts) found finite wall-clock caps sitting on paths whose work legitimately runs long, human decisions being auto-expired, and local waits with no bound at all. This PR fixes the high-impact set, and closes the review gaps found in two follow-up self-reviews (see "Review round" and "Second review round" below).

Fixes

Kills live work (worst class)

• External tool approvals no longer auto-deny after 300s: the decision wait is unbounded (human-paced) and now ends on turn interrupt, engine death, or runtime shutdown, emitting approval.decided{interrupted:true} so clients clear pending UI. The in-process engine approval wait was already unbounded and excluded from the turn wall clock; the runtime-thread path now matches it.
• request_user_input waits unboundedly instead of answering the model with ToolError::Timeout after 300s — a human-paced answer is not a stuck request — and, like the approval wait, its human wait is excluded from the per-turn wall-clock budget so an answer past the budget is not collected and then dropped with the turn.
• Background-task idle detection now treats a running tool as progress: the journal only records tool calls at start/completion, so the 2-minute idle deadline fired during any silent build/test/MCP call. In-flight tools are tracked by the journal item id on both lifecycle edges, and the steady-state loop refreshes progress while any tool is running — not only on event arrival. The wall-time budget remains the spend backstop for a hung tool.
• MCP tool calls: TUI pool default execute timeout 60s → 1800s; the per-request response-read wait is widened to at least that request's own budget, so raising execute_timeout actually governs a tools/call while the read_timeout knob keeps bounding quick requests (resources/read, discovery); the HTTP transport's client-level total is a separate ceiling of max(read, execute); engine-side stdio proxy gets a dedicated 1800s tools/call budget instead of the 120s generic request cap; headless runtime dispatch backstop 300s → 1800s; MCP OAuth browser-callback wait 300s → 900s.
• js_execution, code_execution, and plugin scripts: 120s → 600s, and the interpreter child is verified killed on timeout (the Child is held explicitly and reaped synchronously; see "Review round" and the corrected kill narrative in "Third review round"), and the call returns promptly even when a grandchild holds the pipes — on the timeout and the clean-exit path alike (see "Second" and "Third" review rounds).
• image_analyze: the 120s total reqwest deadline on a multi-MB upload + long non-streaming vision generation is replaced with a 30-minute total envelope around send, retry, and body consumption (connect still bounded at 10s). reqwest's read_timeout was evaluated and rejected for this path: its request-phase timer starts at send() and never resets, so it silently re-introduces a total deadline from send — exactly the bug being fixed.
• RLM child completions now honor the advertised sub_query_timeout_secs (it was accepted, clamped, stored — and never read; the bridge always used its 120s const), and the knob now follows the rlm_query recursion into nested bridges; inline repl rounds 180s → 900s.
• web fetch hard cap 60s → 300s (10 MB bodies need more than ~1.3 Mbps); SSRF pre-flight DNS lookup bounded at 10s so a hung resolver cannot stall the fetch envelope per redirect.
• Sub-agent tool timeout default 300s → 1800s at its single source of truth (the heartbeat floor derives from it automatically).
• The OpenSandbox external backend's hardcoded 30s client total on the exec request — which is the remote command — is raised to 600s under a named constant (matches the Bash tool's foreground cap); a remote build or test run was previously cut off at the HTTP layer with no way to raise it.

Hangs with no bound

• Non-streaming model requests get a 30-minute total envelope: the retry loop is wrapped in one outer timeout, and each attempt's request carries the same budget as a reqwest per-request total covering connect through body end (a slow-drip body previously had no bound even after the header envelope resolves). The Anthropic dialect's non-streaming Messages request — a direct send with no retry loop and no bound at all — gets the same budget. Streaming paths keep their own open-cap + per-chunk idle protection; their opens go through a dedicated entry point that sets no per-request total, because reqwest's total wraps the response body and would otherwise ride on the returned body and hard-cut a live stream (see "Second review round").
• Skill/plugin installs get connect (10s) + total (600s) timeouts; previously none — a stalled connection hung installs and 8-way registry sync forever.
• Snapshot git subprocesses bounded at 300s with degrade-to-disabled instead of blocking the turn pipeline on a wedged git (NFS/FUSE, hung hooks). The pipes are drained concurrently while the child runs, and the timeout path detaches the readers (grandchildren can hold the pipes); a SIGKILLed git add -A/checkout can no longer leave an index.lock that permanently and silently poisons every later snapshot — a lock older than an hour is cleared with a warning on open.
• The app-server OpenAI-compatible proxy (non-streaming upstream forward) gets connect (10s) + total (1800s) timeouts; its client previously had none, so an accept-and-stall provider wedged the handler forever.
• Plugin tools keep model-invoked-interpreter parity (600s + effective kill); provider PKCE browser sign-in callback 300s → 900s (same human-paced reasoning as the MCP OAuth callback); self-update asset download 300s → 600s (tens-of-MB binaries at ~100 KiB/s); image_ocr bounded at 300s (spawn_blocking + timeout, shared by read_file's image path — a wedged backend keeps its blocking thread until it returns, but the tool call is bounded) and pandoc_convert converted to a tokio Command with explicit kill at a 600s bound; previously both ran unbounded sync children on the async executor. The task-gate child is now killed on gate timeout instead of being left running orphaned.

Review round (what the first follow-up self-review caught and this PR now fixes)

• task_manager idle fix was doubly ineffective: started/completed edges were keyed by different id namespaces (tool-use id vs journal item id) so the tracking set never drained, and progress was only refreshed on event arrival — the exact silent window the fix targets has no events. Rebuilt properly, with both edges pinned by tests (silent tool survives the idle deadline; completion restores it; a hung tool still hits the wall budget).
• vision read_timeout claim was false: reqwest's request-phase read timer is a hidden total-from-send() deadline, so the "connect + per-read idle" change still killed generations at ~120s. Replaced with the envelope pattern and a corrected comment; wiremock tests pin both the stalled-provider cutoff and the in-budget happy path.
• snapshot run_git would have deadlocked: read-after-exit cannot drain ls-tree -r/diff output; rebuilt with concurrent reader threads (mirroring the sandbox exec plumbing) and a large-output regression test under a tightened test-only timeout.
• MCP max(read, execute) clamped the documented read_timeout knob and extended 30-minute liveness to non-tools/call requests. Replaced with per-request widening; three tests pin the knob, the transport ceiling, and the widening rule; docs/MCP.md (en + zh) now documents the defaults and the split.
• Approval wait had two dead exits: a crashed engine left the pending approval suspended forever, and nothing ever cancelled the runtime token. The wait now probes the engine's op channel (non-consuming) and RuntimeThreadManager::shutdown() cancels the token; both exits are tested and emit approval.decided{interrupted:true}.
• SSE projection dropped the interrupted flag (and kept a null timeout): fixed, with the legacy approval.timeout arm kept for old-journal replays; docs/RUNTIME_API.md documents the full resolution contract.
• kill_on_drop through Command::output() was misdiagnosed as not killing: the minimal reproduction of the earlier round was wrong — a five-line reproduction against the pinned tokio shows the kill (and reap) does fire when the future is dropped; the original observation was a reaper race. All three interpreter sites (js, code, plugin) still hold the Child and kill explicitly on timeout — kept because the reap is synchronous and testable and the call can return at child exit instead of coupling the deadline to pipe EOF — with pid-liveness regression tests; see "Third review round".
• Non-streaming bodies and the Anthropic dialect were still unbounded: per-attempt reqwest totals now cover connect through body end, and the Anthropic non-streaming request gets the same budget; a wiremock test pins the envelope.
• Residual outliers closed: plugin tools (120s + orphan), PKCE login (300s), self-update download (300s), image_ocr/pandoc_convert (unbounded).
• Docs: MCP.md defaults/semantics (en + zh), RUNTIME_API.md approval resolution contract.
• Two stale config expectations that the original commit's compile break had masked (the sub-agent heartbeat floor moved with the new 1800s default: /config subagents status now resolves 1830, and a 900s api timeout no longer dominates the tool-timeout floor) were caught by CI's full-workspace run and fixed with the dominance documented and a maxed-api-timeout case added.

Second review round (what an independent full re-review caught and this PR now fixes)

• The per-attempt reqwest total leaked onto the streaming open and hard-cut live streams at 30 minutes. send_with_retry applied the total to every attempt — but it is also the transport for the default ChatCompletions dual-stack stream open and every Responses open, and reqwest wraps the response body in the total deadline: the caller's 45s open budget only bounds the open, and the total rode on the returned body, truncating any generation 30 minutes after send (the exact failure class this PR removes). Stream opens now go through a dedicated no-total entry point, with wiremock tests pinning that an open answering past the envelope succeeds and that a caller-pinned total (list_models' 30s — previously silently overwritten by the envelope) survives; the loop envelope now dominates the per-attempt total it wraps.
• request_user_input burned the default 1-hour turn wall clock while the user thought. Only the approval wait paused the budget (begin_human_wait); the user-input wait's comment claimed to mirror it but did not, so an answer submitted past the budget was collected and then discarded with the failed turn at the next provider boundary. The wait now pauses the budget like the approval wait; an engine test pins that a 1s budget plus a 1.2s-delayed answer completes the turn (fails with "budget exhausted" without the pause).
• Timeout arms joined pipe-drain tasks that grandchildren can keep alive. Killing the direct child closes only its write ends; a grandchild that inherited the pipes keeps them open, so read_to_end never sees EOF and joining hung the caller past the 600s/300s bound (the old "joins cannot hang" comments asserted the opposite of real pipe semantics). All three interpreter timeout arms abort the drain tasks instead (handing grandchildren an EPIPE), run_git detaches its readers on the timeout path, and a js regression test spawns a pipe-holding grandchild: the join implementation measured 30.0s against the 5s test budget. run_git's comment also cited a nonexistent helper (run_sandboxed_exec) and now names the real one.
• Two same-class holes outside the original sweep: the OpenSandbox backend's hardcoded 30s exec total (raised to a named 600s constant matching the Bash foreground cap) and the app-server proxy's completely unbounded upstream forward (now connect 10s + total 1800s, mirroring the TUI non-streaming envelope).
• A timed-out snapshot git could permanently and silently break snapshots: SIGKILL skips git's lock cleanup, so a killed git add -A/checkout left index.lock and every later snapshot failed fast on it with only a log line. Stale locks (older than an hour, far past the command bound) are now cleared with a warning on open; a fresh lock is left alone.
• Small gaps closed: task-gate child killed on gate timeout (was orphaned, no kill_on_drop); fetch_url schema still said "max 60,000" after the cap moved to 300s; nested rlm_query bridges now inherit sub_query_timeout_secs; plugin gained the pid-liveness test the PR claimed (plus the same 5s test-only timeout as js/code); shutdown()'s doc now states it resolves only approvals and has no production caller yet; SSE projection regression coverage for the interrupted flag and the legacy approval.timeout replay arm.

Third review round (what an independent 13-agent re-review caught and this PR now fixes)

• The idle-watchdog fix never reached the production path. The in-flight-tool suppression only refreshed drive_engine_turn's own guard; the worker supervisor (run_task) wraps the executor in a second guard fed exclusively from the task-event channel, where a silent tool produces nothing after ToolStarted — automation tasks still died at the 2-minute idle deadline, and the earlier tests (all driving drive_engine_turn directly) could not see that layer. The in-flight window is now published as a ToolHeartbeat task event the supervisor counts as progress, never persisted or surfaced; the seam is pinned from both sides, each test failing if its half reverts.
• The interpreter rewrite's central kill narrative was false (see the corrected "Review round" bullet): kill_on_drop through a dropped output() future does kill on the pinned tokio. The explicit structure stays for the right reasons; pandoc and the task gate, which use the stock idiom, were never broken.
• The interpreter success path could still hang past the deadline. The budget wrapped only child.wait(); after a normally-exiting child the drain joins were unbounded, so a grandchild inheriting the pipes (npm run dev & then exit) held the call — the pre-PR timeout(output()) bounded this. All three sites now bound the post-exit drain with a short grace, returning the captured output with a note on stderr, and the three near-identical blocks are extracted into one shared tools::process::run_bounded_child (which also aborts the plugin stdin writer on the timeout path instead of detaching it). New grandchild tests fail if the grace is inflated back to unbounded.
• Two snapshot git calls were never bounded, and the module comment claimed they were. git init and the date-pinned commit-tree (reachable from the per-turn prune path) ran bare .output(); both route through the bounded core now, whose success-path readers get the same grace treatment and whose kill reaps on a detached thread — on a hard-wedged mount git dies only when its uninterruptible syscall returns, and a blocking wait() would hang the turn pipeline exactly like the wedged git would. A timed-out git also reaches the user's stderr through the once-per-workspace snapshot notice: it leaves a fresh index.lock that fast-fails snapshots for about an hour, and that must not be log-only.
• The app-server runtime bridge client had no timeouts — the same builder-without-deadlines shape the previous round fixed for the upstream proxy, one module away; the bridge lock is held for the whole turn, so one request to a live-but-wedged runtime child queued every later JSON-RPC message forever. Connect 10s, POST total 1800s, and a re-arming 60s SSE chunk-idle bound (the runtime emits 15s keepalives), with no total on the live stream.
• The MCP send leg sat outside every budget. A server wedged without draining stdin blocked write_all forever; the send now shares the request budget and routes through the guarded error path, because a timed-out partial write desyncs the line protocol. Two wiring tests pin the widened read wait and the bounded send at call_method level rather than only in the pure functions. The 900s OAuth wait's tool description, two comments, and the synthetic-tool test still said five minutes — all synced.
• Smaller gaps closed: the engine code-execution pid test is now #[cfg(unix)]-gated (libc is unix-only; the Windows test leg would not compile); the heartbeat operator docs resolve 1830 through both floors instead of a stale 630; RUNTIME_API.md states that runtime shutdown is a latent resolution path until a host wires shutdown() up; the OpenSandbox exec client gets the 10s connect family; PANDOC_TIMEOUT no longer swallows the tool's rustdoc; the OCR disclosure covers the orphaned tesseract child; the injected client test envelope went 250ms → 2s so unrelated non-streaming tests cannot flake against the injection window under CI load.

Fourth review round (what the second external review caught and this PR now fixes)

• The snapshot git readers leaked a thread per pipe whenever a grandchild held a pipe. Both exits left reader threads pinned on pipes whose write ends live on in a grandchild: the drain-grace expiry on the success path discarded the JoinHandles, and the timeout path detached the readers outright, so a hook or subprocess that daemonized left one OS thread per pipe per git call blocked in read for its own lifetime — the prior regression only backgrounded a sleep 30, so the leak self-healed and the hole went unnoticed. The drain loop is now cancellable on both platforms — a non-blocking, polled read on Unix, a PeekNamedPipe probe loop on Windows — and both exits cancel and join the readers, so the joins are bounded by one poll interval and the call provably leaves no reader threads behind. Cancelling also drops the read ends, handing the grandchild an EPIPE on its next write, the same contract as the interpreter runner. A live-reader counter makes the join observable: the new regression drives two calls against a never-exiting grandchild and fails with "4 still running" if the join reverts to a detach (red-green verified against a simulated revert).
• The CodeQL gate went red on a moved, pre-existing flow — traced and fixed at the source. Routing run_git through the bounded drain core relocated the flagged argv sinks, and code scanning re-registered the flow as a new alert instead of matching base alert TUI QoL: make /config searchable and grouped Hmbown/Codewhale#198 (PR-ref alerts cannot be dismissed, and Rust has no inline suppression support upstream yet). The alert's path resolved to a single origin: the /v1/snapshots/{id}/restore route, whose request-path id reached git as a treeish. Two changes close it: (1) the side-repo paths ride git's documented GIT_DIR/GIT_WORK_TREE environment interface — the same options by definition, same never-touch-the-user's-repo guarantee — so they are out of argv; (2) the restore endpoint now resolves the requested id against the snapshots the side repo actually knows and 404s on anything else, so an unknown id never reaches a git command line and the allowlist contains is the scanner's modeled trust boundary. Verified locally with CodeQL 2.27.0 (build-mode none): the fixed tree reproduces the base's pre-existing alerts at identical locations and reports no alert for the snapshot flow.

Fifth review round (what an independent ten-agent fresh re-review caught and this PR now fixes)

• ToolHeartbeat was persist-urgent. It was missing from the execution_event_persist_urgent exclusion list, so every ~200 ms heartbeat during a silent build rewrote the whole task record under the manager-wide state lock — contradicting the variant's own "never persisted" contract. Now excluded, pinned by classification and wiring-level assertions (heartbeat applies unpersisted, a real lifecycle edge still flushes).
• MCP request-leg timeouts left the connection Ready. A timed-out send can leave a partial line in the pipe, and a timed-out outer read wait can still be answered late; finish_guarded_error does not tear the connection down, so the pool handed the broken transport right back out even though the code comment claimed the caller reconnects. Both arms now mark the connection Disconnected (matching the inner read timeout), the comment states the real behavior, and wiring assertions pin the state on both paths.
• The snapshot stderr notice never fired for its motivating case. It was wired only to open_or_init errors, so the documented "wedged git add -A times out and leaves a fresh index.lock" scenario stayed log-only. Session and prune failures now route through the same once-per-workspace notice; the TimedOut gate keeps it scoped.
• git init inherited ambient GIT_DIR/GIT_WORK_TREE. It was the one git invocation not cleared of a shell-exported GIT_DIR, which could redirect the one-time init away from the hashed side-repo path. Now env_removed, and the stale comment still describing flag-based routing is corrected.
• Plugin scripts could pass truncated output as a success. The drain-grace truncation note lands on stderr, which the plugin surface discards on a successful exit — an unparseable, possibly cut-off output became a silent success=true. The note is now a shared constant with a probe, and the plugin arm fails loudly when the output does not parse and the drain was truncated.
• A queued approval decision could be eaten by a same-instant interrupt/shutdown. The biased select favored the cancel token, so a decision already in the oneshot was discarded and reported as an interrupted deny — contradicting the documented "a decision the user actually made never carries interrupted". Both exit arms now rescue a queued decision via try_recv; a deterministic test pins the race.
• The bridge's SSE header wait was unbounded. A runtime child that accepts the event-stream GET but never writes response headers held the bridge lock forever — the queue-forever scenario surviving through the headers phase. The header wait now shares the 60s chunk idle deadline; the live stream keeps no total.
• Smaller gaps: the OpenSandbox comment claimed the 600s budget "matches the Bash tool's foreground cap" (the Bash cap is effectively unbounded; the real parity class is the interpreter tools' 600s cap); the human-wait wall-clock test margins went 1s/1.2s → 2s/3s against CI load; SUBAGENTS.md (en + zh) no longer implies a resolvable tool_timeout_secs config key (only api_timeout_secs exists; the tool timeout is a compile-time constant); RUNTIME_API.md documents the restore endpoint's exact-match/404 contract; the in-flight tracking set now comments the disclosed compaction blind spot; and the first commit's author/SOB email is normalized to the account address (whole-chain replay: trees byte-identical, range-diff all-equal).

Sixth review round (what an independent 15-area fresh re-review, run after rebasing onto the merged #67, caught and this PR now fixes)

• Headless exec could park forever on request_user_input. The exec event loop auto-resolves approvals but had no UserInputRequired arm, and the engine's human wait is unbounded now — a headless run whose model asked a clarifying question parked forever where the old 300s cap resumed it. The loop now cancels the request (the tool gets a cancelled error, the turn continues) and says so once on stderr.
• A timed-out git init permanently wedged snapshots. needs_init keyed on directory existence alone; a killed init leaves .git without HEAD, so init was skipped forever and every later snapshot failed with "not a git repository" (the config failures on that path were swallowed too). open_or_init now uses open_existing's readiness predicate; git init is idempotent, so re-initializing the partial directory is safe.
• A dead monitor stranded pending approvals. settle_claimed_turn_failure settled user inputs and dynamic tools but not approvals: the map entry leaked and no approval.decided was published, contradicting the documented resolution contract. It now settles this turn's approvals the same way the interrupt exit does (deny + interrupted).
• A delivered decision could still be mislabeled interrupted. deliver_external_approval removes the map entry before sending, so a user decision could land in the oneshot after both rescue checks saw Empty. The interrupted exit now makes one final try_recv and routes a found decision through the decision path; a send landing after that finds a dropped receiver and honestly reports not-delivered.
• Heartbeats took the manager-wide state lock for a no-op and set dirty. Each ~200ms tick entered apply_execution_event, acquired the lock, matched => {}, and marked the record dirty — deferring real mutations' persistence past the debounce window for the whole silent tool. process_execution_event now short-circuits before the lock (the supervisor guard is already refreshed by the progress classification).
• The vision envelope broke the error taxonomy and the permit wait sat outside every bound. The envelope expiry surfaced as execution_failed instead of ToolError::Timeout (every other bounded tool classifies); the runtime-chat inference permit was acquired before the envelope, so a contested permit could park the tool unbounded — the acquire now lives inside the envelope, still held for the whole request.
• Envelope-exceeded requests skipped the recovery probe. maybe_probe_recovery ran on the exhausted-retries path only; a provider that just wedged past 30 minutes is exactly when the /models health probe matters.
• Three app-server adjacencies: a stream that dies mid-turn (idle/header timeout, transport error) now leaves a best-effort interrupt, so a retried message no longer fails with "already has an active turn" until the orphan completes; the /health boot probe bounds each attempt by the remaining deadline (a child that accepts but never writes headers used to hang it forever and respawn a child per message); the proxy drops the config read guard once the endpoint is resolved instead of holding it across the (now up to 30-minute) upstream round trip.
• The interpreter runner's wait()-error exit leaked the drain tasks (the one exit that was neither timeout nor clean exit); aborts now run before propagating. The RLM sub-query builder floors at 1s (0 built an instantly-firing timeout that would kill every child query), the session default references CHILD_TIMEOUT_SECS instead of a duplicate literal, and the inline-round comment no longer implies a single minutes-long sub-query survives a path where the bridge still bounds each one.
• Docs/comments corrected: the stale-lock comment no longer claims a lock older than the command bound cannot belong to a live writer (a git wedged in uninterruptible I/O can; removal stays benign and the comment says why); the bounded-git comment names the real sandbox-CLI gap instead of claiming to mirror it; the OpenSandbox comment matches its own rationale (the Bash foreground cap is effectively unbounded); the fetch-envelope comment discloses the pre-envelope SSRF pre-flight lookup; the self-update comment states the real ~58 MiB at 100 KiB/s; the MCP execute-timeout comment discloses the per-leg worst case; the 1800s definitions in core, subagent-limits, MCP, and the dynamic-tool wait now cross-reference their family; RUNTIME_API.md's restore contract is accurate (any snapshot the side repo knows, not just the listed window; the guarantee is that a non-matching id never reaches a git command; real 40-hex example id; approval.timeout marked legacy in the common-events list; "every resolution" scoped to forced resolutions).
• Test hardening: the fetch hard cap is pinned to 300,000 ms so the schema's advertised max cannot drift; the reader-leak regression asserts its own reader delta against a pre-loop baseline instead of a process-global zero, so unrelated concurrent git calls cannot fail it.

Testing

• cargo check --tests clean for codewhale-tui, codewhale-mcp, codewhale-core; cargo clippy --workspace --all-features --locked -D warnings clean (CI's allow-list); cargo fmt --check clean.
• Targeted suites all green: runtime_threads 159, task_manager 48, tools::web 192, mcp 76 (+231 in the TUI pool), core 82, vision 12, js_execution 10, repl 31, snapshot 57, skills::install 20, client 165, tools::plugin 19, tools::file 119, rlm::bridge 10, codewhale-mcp 76, plus the full workspace suite (cargo test --workspace --all-features --locked).
• Pre-existing, unrelated to this PR: 2–3 remote_control restart/recovery tests flake under full-suite parallel load on the base commit too (verified by rerunning the same suite on pinvou3-clean); isolated runs pass.
• New regression tests: approval pends-until-interrupt/shutdown/engine-death; silent-tool idle suppression + wall-budget backstop; snapshot large-output drain + stale index.lock cleanup; MCP read-knob/transport-ceiling/per-request widening; vision envelope (stall + happy path); client non-streaming envelope + stream-open-no-total + pinned-total survival; interpreter kill (pid liveness, js + code + plugin) + prompt return past a pipe-holding grandchild; snapshot readers cancelled and joined (repeated permanent-grandchild calls leave no reader threads, pinned by a live-reader counter); user-input human wait excluded from the turn wall clock; RLM sub-query budget; SSE interrupted/legacy-approval.timeout projection.
• The old approval_timeout_denies... test is replaced by approval_pends_until_interrupt... pinning the new semantics (no auto-deny; interrupt resolves the pending approval and clears state so the next turn can start).
• retrieval_defaults_are_coherent... updated to assert the fetch hard cap dominates the search cap (page fetches stream 10 MB bodies; search APIs answer small JSON) instead of strict equality.
• Known pre-existing, unrelated: running the full tools::subagent:: test filter stack-overflows on this host with or without this PR (CI runs the suites with a 16 MiB stack; local runs need RUST_MIN_STACK=16777216 for some broad filters).
• Full-suite parallel-load flakes, verified against the base commit (3 base runs fail only the documented remote_control pair): on this branch's head, three additional global-state tests each failed once across repeated full-suite runs — model_inventory::ollama_default_prefers_live_local_tags..., shell_dispatcher::test_env_lock::uncontended_read..., and client::tests::deepseek_anthropic_translate_uses_messages_endpoint — all pass in isolation and on rerun, all live in files this PR does not touch (provider-lake / merged-catalog global state races), and all are the same test-robustness class as the remote_control pair; noted as follow-up hardening, not smuggled fixes.

Notes for reviewers

• The Desktop (pinvou3-app) side ships a companion PR (Speculative cached-prefix turn branches with verifier selection Hmbown/Codewhale#532) with the app-side waits (shell_env hook budget, pre-turn checkpoint bound, connector probes, relay pong watchdog, stop button, OAuth window, ingest/monitor bounds). Merge order with Verifier chains over cached repo context beyond RLM Hmbown/Codewhale#530 (gitlink advance) needs coordination so the pinned gitlink moves once.
• Known remaining gaps, disclosed rather than smuggled: the standalone codewhale sandbox run CLI (run_sandbox_command) still joins its pipe readers unconditionally after the child exits — pre-existing on the base branch and untouched here (a grandchild holding the pipes delays only that standalone process); and on Windows tokio backs child pipes with its shared blocking pool, so an aborted interpreter drain leaves the pool thread parked on the read until the grandchild closes the pipe (the pool is bounded and reused; the Unix reactor path, where the interpreter tests run, has no such reservation). the isolated-request path (send_with_isolated_retry) has no envelope of its own — its only caller wraps it in a 4–300s outer timeout; a timeout-bounded image_ocr cannot kill the native macOS Vision FFI mid-call (the tool returns, the blocking thread returns when the backend does); grandchildren spawned by interpreted scripts are not chased (kill covers the direct child, matching every other process-kill site in the repo). Since the approval/user-input waits became unbounded, an automation task blocked on a human answer now occupies its worker until the task execution wall_time (default 30 minutes) instead of failing at the old 300s cap — and that clock does not pause for human waits, so a slow answer still ends the task; pausing it safely needs reliable wait-window detection and is left for a follow-up; and the idle watchdog still cannot see a long-running compaction (its journal item carries no tool marker), so a background task mid-compaction remains exposed to the 2-minute idle deadline.
• From the fifth-round re-review, disclosed rather than fixed: with the approval wait unbounded, a vanished deciding client now leaves an interactive turn parked indefinitely where the base self-released at 300s — shutdown() remains latent until a host wires it, the pending-approval map (unlike the dynamic-tool map) has no cap, and LRU protection pins parked turns. A reconnected client can still answer, interrupt and engine death still resolve, and automation tasks remain bounded by wall_time; production wiring of the shutdown path (or a pending cap) is follow-up work. Relatedly, the 1800s tool timeout now exactly equals the default sub-agent child wall-time and the task wall_time backstop, so one maximal tool call can consume an entire child/task budget with nothing left for the surrounding steps — documented trade-off, not changed here.
• Fourth parallel-load flake of the already-disclosed class: client::anthropic::tests::anthropic_stream_open_error_is_not_retried failed once under full-suite load and passes in isolation and on rerun (same global-state class as the three below; the file is this PR's but the flake is load-order, not semantic).
• Known follow-up debt, not smuggled in (verified by the fifth-round audit): run_git's drain core and the sandbox exec plumbing are two copies of the same sync spawn/drain/bounded-join shape and should share one helper — and the sandbox twin still carries the grandchild-wedge and blocking-reap hazards this PR fixed on the git side, which makes it the priority; the 1800s budget is defined in nine places across four crates that comments keep in sync — one shared constant home would remove the drift risk; the pid-liveness poll loop is triplicated across the three interpreter test modules; the Anthropic envelope test cannot inject a shrunk budget because anthropic.rs uses the raw constant instead of the test accessor; and MCP OAuth metadata/login clients plus the image_analyze upload cap remain unbounded (pre-existing). (The three near-identical interpreter blocks were extracted into the shared tools::process::run_bounded_child in the third review round.)
• The desktop app pins 92427bd8d (advanced by the merged Verifier chains over cached repo context beyond RLM Hmbown/Codewhale#530); this branch is rebased on the current pinvou3-clean, so the gitlink can advance in one step once this merges.

No-Issue: fix wave from a repo-wide timeout-behaviour audit; no single tracking issue exists yet.

[JensenChen28]

发现一个会绕过本 PR 所设总预算的阻塞问题。

js_execution 只把 child.wait() 放进 600 秒 timeout；父进程正常退出后，成功分支又在 timeout 之外无期限等待 stdout/stderr drain task。脚本可以启动一个继承 stdout/stderr 的后台孙进程后立即正常退出，此时 child.wait() 立刻成功，但 drain 要等孙进程关闭管道，甚至可能永远不返回。现有回归只覆盖“父进程一直运行直到被 timeout 杀掉”的分支，未覆盖父进程先成功退出的情形。plugin 使用了相同结构，snapshot::run_git 的成功分支也会在 wait_timeout 返回后无界 join reader thread。

请让总预算覆盖“等待父进程 + 收集两条管道”的整个生命周期，或在父进程退出后按剩余预算等待 drain，超时则关闭/abort reader；并补一个“父进程启动继承管道的后台孙进程后立即退出”的回归。其余检查中，git diff --check 以及 PR 的 check/gate/DCO/CodeQL 均通过；sync 失败来自镜像同步，不改变上述代码结论。

[JensenChen28]

上轮 JS/plugin 与 snapshot 成功路径的无界等待已改为有界返回；共享 async runner 的 2 秒 drain grace 能中止任务，原阻塞点已解决。但 snapshot 的同步实现引入了新的资源泄漏：run_bounded_git 在 drain grace 超时后于 snapshot/repo.rs:1166-1167 直接丢弃 JoinHandle。Rust 线程不会因此取消，reader 仍永久阻塞在管道 read；仓库 hook、clean filter 或子进程若启动长期后台孙进程并继承 stdout/stderr，每次 snapshot/git 调用都会遗留最多两个 OS 线程。现有回归只后台 sleep 30，所以泄漏会在 30 秒后自行结束，覆盖不到永久持管道及重复调用导致的线程耗尽。请让 reader 可被主动终止/关闭，或改用可取消的异步/非阻塞 drain，并增加永久持管道后重复调用仍不增长线程/任务的回归。

[asto18089]

Thanks for both reviews — round 1 was fixed in the previous push, and your round 2 finding is confirmed and fixed in 7add69d (two commits on top).

Round 1 (drain joins) — already resolved before this push, for the record. The success-path drains on all three interpreters now go through the shared run_bounded_child with a 2s grace that aborts on expiry, and snapshot::run_bounded_git bounds its post-exit drain the same way; each site carries the "grandchild spawned after a clean exit" regression you asked for.

Round 2 (reader-thread leak) — confirmed exactly as described. Both exits left reader threads blocked in a pipe read whose write end lives on in a grandchild: the drain-grace expiry dropped the JoinHandles, and the timeout path detached outright — one leaked OS thread per pipe per git call for a daemonizing hook/subprocess. The sleep 30 regression self-healed and never saw it. Fixed on both platforms:

• The drain loop is now cancellable: a non-blocking, poll-driven read on Unix (O_NONBLOCK on our read end only), and a PeekNamedPipe probe loop on Windows (std pipes have no non-blocking mode there; data is read only when already buffered). A cancelled reader exits within one 50ms poll interval and drops the read end, handing the grandchild an EPIPE on its next write — the same contract as the interpreter runner.
• Both exits now cancel and join the readers, so the joins are bounded by the poll interval and the call provably leaves no reader threads behind — including the timeout path, which previously detached.
• The new regression (repeated_calls_with_a_permanent_grandchild_do_not_leak_reader_threads) drives two calls against a never-exiting grandchild and asserts a live-reader counter drains back to zero. Red-green verified: with the join reverted to a detach it fails with exactly 4 still running.

One nuance for completeness, since you reviewed the interpreter runner too: on Unix, tokio's child pipes are reactor-backed (O_NONBLOCK + PollEvented), so the async runner's abort() genuinely frees everything. On Windows, tokio backs child pipes with its blocking pool, so an aborted drain leaves a pool thread parked on the read until the grandchild closes the pipe — inherent to tokio, and the pool is bounded and reused, so it does not grow the way the detached std::threads did. Both behaviors are now disclosed in the PR notes.

The CodeQL gate, for transparency: it went red on this PR not because of a new flow but because routing run_git through the bounded core relocated the --git-dir/--work-tree argv sinks, and code scanning re-flagged the unchanged flow as a new alert — the identical flow is already two open alerts on the base branch. Since PR-ref alerts can't be dismissed and Rust has no inline suppression support yet upstream (github/codeql#21638 is still open), both paths now pass the side-repo paths through git's documented GIT_DIR/GIT_WORK_TREE environment interface — the same options by definition, same never-touch-the-user's-repo guarantee — so the paths are out of argv and the flow stops re-flagging on refactors. Post-merge, the base's two stale alerts retire with the code they pointed at.

Disclosed, not fixed (out of scope): the standalone codewhale sandbox run CLI (run_sandbox_command in lib.rs) still joins its readers unconditionally after the child exits — pre-existing on the base branch and untouched by this PR; happy to file it as a follow-up.

All 59 snapshot:: tests pass locally (including the two bounded-git regressions), cargo clippy --workspace --all-features --locked -D warnings and cargo fmt --check are clean.

[JensenChen28]

已复核当前 head 7add69d3 及上一轮 reader-thread 泄漏修复。Unix 路径将 pipe fd 设为 nonblocking 并以 50ms poll 检查取消；Windows 路径以 PeekNamedPipe 做同等有界轮询；成功 drain-grace 超时与命令 timeout 两个出口都会设置 cancel 并 join reader，因此不再丢弃 JoinHandle。新增永久持管道孙进程的重复调用回归通过 live-reader 计数验证 reader 回到 0。随后将 side-repo 路径改走 Git 官方 GIT_DIR / GIT_WORK_TREE 环境接口，保持原有隔离语义。

上一轮阻塞项已解决，未发现新的代码级阻塞问题。git diff --check 通过，当前 head 的 required checks（DCO、Gitleaks、check、gate）全部通过；非 required 的 CNB sync / CodeQL 汇总失败不改变本次代码结论。

[asto18089]

Follow-up on the CodeQL gate — the picture changed after my previous comment, and the earlier "stops re-flagging" claim needs correcting.

The env routing did remove the --git-dir/--work-tree argv sinks, but the analysis re-flagged the remaining .args(args) sink — and this time I traced the alert's path properly (CodeQL 2.27.0, build-mode none, locally): the taint originates at the /v1/snapshots/{id}/restore route, where the request-path id flowed straight into git checkout <id> as a treeish. That remote flow is exactly what base alert Hmbown#198 covers, so every refactor of run_git re-registered it as a new alert instead of matching — PR-ref alerts can't be dismissed and Rust has no inline suppression upstream (github/codeql#21638 is still open), so no amount of reshuffling the sink would keep the gate green.

Fixed at the source in c88ceb0: the restore endpoint now resolves the requested id against the snapshots the side repo actually knows (repo.list → contains) and returns 404 for anything else, so unknown ids never reach a git command line. The allowlist membership check is also the scanner's modeled sanitizer shape, and it is a real hardening rather than an analyzer trick — previously any string from the request path was handed to git as a treeish (it would have failed loudly, but still).

Verified locally before pushing: the fixed tree reproduces the base branch's four pre-existing command-line-injection alerts at identical locations (operate.rs:720/722/724, native_memory.rs:148) and reports no alert for the snapshot flow. A regression test pins the 404 (restore_snapshot_endpoint_helper_rejects_unknown_snapshot_id).

Net effect of the two review rounds, for the record: the reader-thread leak is fixed with cancellable drains and a red-green regression, and the restore endpoint now validates its input against the snapshot allowlist.

[asto18089]

第五轮全新复审(10-agent 独立舰队)与修复

按“以全新视角重新审阅”的要求,本 PR 在最新 pinvou3-clean(c4e6caf)上由 10 个独立审查代理做了全量重审:按区域划分 8 个(审批/用户输入等待链、任务空闲看门狗、MCP 超时族、client 信封与 vision、解释器 runner、snapshot git 有界核、sandbox/bridge/proxy、杂项工具与文档),外加逐提交夹带审计和对抗性跨域红队各 1 个。全部 PR 正文声明逐条对照代码核实,reqwest 语义对 Vendored 0.13.4 源码验证。

核实为真并已修复(7 项,提交 028fd0e2a..dfbcc89ce):

1. MAJOR — ToolHeartbeat 被判为 persist-urgent:不在排除名单,静默构建期间每 ~200ms 全量重写任务 JSON 并持有全局锁,与变体自身 "never persisted" 契约矛盾。已排除 + 分类/接线双层钉测试。
2. MAJOR — MCP send/外层 read 超时不毒化连接:超时的写可能留下半行、超时的外层读可能晚到,两条路径都未置 Disconnected,连接池把坏传输原样再借出(与注释声称的"调用方会重连"不符,is_mcp_stale_session_error 也不匹配)。两臂均已毒化,wiring 断言钉死。
3. MAJOR(声明与接线不符)— snapshot 通知未覆盖 motivating case:stderr 通知只接了 open_or_init,"git add -A 超时留 index.lock" 场景仍 log-only。session/prune 失败已接入同一 once-per-workspace 通知。
4. git init 未清除环境中的 GIT_DIR/GIT_WORK_TREE(唯一没做 env 隔离的调用点),已 env_remove。
5. plugin 成功路径丢弃截断注释:截断输出可伪装成静默成功;注释改为共享常量 + 探测,解析失败且截断时显式报错。
6. 审批 select 的 biased 顺序会在取消与决策同瞬间丢弃用户已排队决策(违反"a decision the user actually made never carries interrupted");两个出口臂均先 try_recv 抢救,含确定性红绿测试。
7. bridge SSE GET 的 headers 等待无界(accept-but-no-headers 子进程可永久持有 bridge 锁);现已共享 60s chunk idle 界。

另有小型修正:OpenSandbox 注释"Bash foreground cap"事实错误(实际是解释器 600s 同类)、user-input 墙钟测试余量 1s/1.2s→2s/3s、SUBAGENTS.md(en+zh)不再暗示不存在的 tool_timeout_secs 配置键、RUNTIME_API.md 补 restore 精确匹配/404 契约、compaction 空闲盲区在代码处披露、首提交 author/SOB 邮箱归一(整链 commit-tree 重放,树逐字节一致,range-diff 全等号)。

红队六向量四向 sealed,两向产出上列 #1/#6;新增披露(见正文 Notes):审批无限等待下决策方客户端消失会让交互 turn 无限滞留(base 300s 自愈;shutdown() 仍无生产接线、pending map 无上限),以及 1800s 工具超时与子代理/任务 wall-time 相等碰撞——均为记录在案的后续项,不在本 PR 夹带。

审阅结论:夹带审计三重验证(文件集闭包/hunk 级重放/逐提交对账)零夹带;两笔自认债务(沙箱 drain 孪生、1800s×8)核实属实且延期合理,沙箱孪生仍带本 PR 已修的 hazard,已标注为最优先跟进;无造轮子(reqwest total-rides-body 与 read_timeout 语义均对 vendored 源验证)。本地 clippy -D warnings、cargo fmt --check、全 workspace 测试(RUST_MIN_STACK=16777216)全绿;一次 anthropic_stream_open_error_is_not_retried 失败为正文已披露的并行负载全局状态 flake 同类第 4 例,隔离重跑通过。CI 的 "Sync to CNB" 失败为全分支存量基建问题(与代码无关)。

请 @JensenChen28 有空时重批。

[asto18089]

Round-6 fresh review: rebase onto merged #67 + 15-area independent re-review + fix wave

Rebased onto 6f780290f. One organic merge conflict: the vision test module, where base #67 added the pixel-dimension/BMP-degrade tests in the same region this PR rewrites — both sides are kept, and the PR's two envelope tests now reuse the base tool_with_base_url helper instead of a duplicate config builder (all 15 module tests pass; base test bodies byte-identical). The rebased head ran a fully green CI pass (including check, CodeQL, and the sync conflict gate clearing) before the fix wave below was pushed on top.

Verdict: the core thesis survives a fully fresh adversarial pass. Fifteen parallel review tracks re-derived the claims from source and ran the targeted suites (approval/runtime-threads 9, task_manager 51, snapshot/repo 38, tools::process 2 + js 10 + plugin 19, mcp 214, vision 15, client 165, rlm 67, app-server 100, web 91, runtime_api 185 — all green). No BLOCKERs. The pass found one real regression, one real correctness hole, and a batch of contract/comment inaccuracies; all are fixed in the 11 commits now pushed. Two cross-area findings re-confirmed as disclosed debt rather than defects.

Fixed in this push

• e4c47e237 exec regression (the one true blocker-class find): the headless event loop auto-resolves approvals but had no UserInputRequired arm; with the wait now unbounded, a headless run whose model asked a clarifying question parked forever. It now cancels the request (cancelled error to the tool, turn continues) with a one-time stderr note.
• 7b82f3dcc partial git init wedged snapshots forever: needs_init keyed on directory existence; a killed init leaves .git without HEAD on exactly the wedged-mount scenario this PR targets, and every later snapshot fast-failed. open_or_init now uses open_existing's readiness predicate; re-init is idempotent.
• 484c1dc6f two approval-exit hardenings: (1) a dead monitor task stranded the pending approval — the map leaked and no approval.decided fired, contradicting the documented contract; settle_claimed_turn_failure now settles this turn's approvals like the interrupt exit. (2) deliver_external_approval removes the map entry before sending, so a real decision could land after both rescue checks saw Empty and be reported as an interrupted deny; the interrupted exit now makes a final try_recv and routes a found decision through the decision path.
• a689b703d heartbeats took the manager state lock for a no-op and set dirty, deferring real mutations' persistence past the debounce window for the whole silent tool; short-circuited before the lock.
• 54e1f88e4 vision: envelope timeout is now ToolError::Timeout (taxonomy parity with every other bounded tool; the existing "timed out after" assertions still pass), and the runtime-chat inference permit is acquired inside the envelope — the ownership window is part of the bounded call, so a contested permit can no longer park the tool outside every total bound.
• cf10b3cbc client: the envelope-exceeded branch now runs maybe_probe_recovery; a provider that just wedged for 30 minutes is exactly when the /models health probe matters.
• a0f109364 app-server, three adjacencies: a stream that dies mid-turn leaves a best-effort interrupt (retries no longer hit "already has an active turn" until the orphan completes); the /health boot probe bounds each attempt (an accept-and-never-write-headers child used to hang it forever, respawning a child per message); the proxy drops the config read guard before the upstream round trip instead of holding it (write-preferring lock) for up to 30 minutes.
• 04bd7968a tools::process: the wait()-error exit aborted nothing; drains and the stdin writer now abort before propagating. Module doc states the EPIPE contract is Unix-only.
• 83dc99763 rlm: with_sub_query_timeout_secs(0) built an instantly-firing timeout (now floored at 1s); the session default references CHILD_TIMEOUT_SECS instead of a duplicate literal; the inline-round comment no longer implies a single minutes-long sub-query survives a path where each one is still bridge-bounded.
• bc447c1f0 docs/comments: stale-lock doc no longer overclaims safety vs a D-state holder (and names why removal stays benign); bounded-git comment names the real sandbox-CLI gap instead of claiming to mirror it; OpenSandbox comment matches its own rationale; fetch-envelope comment discloses the pre-envelope SSRF pre-flight; update.rs arithmetic is honest (~58 MiB at 100 KiB/s); MCP execute-timeout comment discloses the per-leg worst case; the 1800s definitions in core / subagent-limits / MCP / dynamic-tool now cross-reference their family; RUNTIME_API.md restore contract corrected (any known snapshot, non-matching id never reaches a git argv, real 40-hex example id, approval.timeout marked legacy, "every resolution" scoped to forced resolutions).
• 7a03db628 test pin: HARD_MAX_TIMEOUT == 300_000 ms asserted beside the coherence invariant, so the fetch schema's advertised max cannot drift; the reader-leak regression now asserts its own reader delta against a pre-loop baseline instead of a process-global zero (unrelated concurrent git calls can no longer fail it).

Re-confirmed as disclosed debt (unchanged)

• Background tasks still bound human waits with the 30-minute task wall_time, which auto-denies a pending decision at expiry — disclosed in the body; this pass additionally confirmed the new heartbeat suppression means base's 2-minute idle kill no longer fires either, so the wall clock is the only remaining bound there. Fixing it safely needs wait-window detection in ExecutionGuard; left as the stated follow-up.
• The three coincident 1800s budgets (task wall / sub-agent child wall+tool timeout / MCP execute) resolve by outermost-wins; inside tasks a raised MCP execute_timeout above 1800s is clipped and the SUBAGENTS.md heartbeat-floor guarantee does not hold. The body documents the equal-budget trade-off; the MINOR-1-style clamping idea (effective inner budget = min(configured, remaining outer)) is a real follow-up candidate but changes budget semantics, so not smuggled in here.
• Snapshot pre-git work is still unbounded blocking on a wedged workspace mount (workspace.canonicalize(), first-init size walk) — the hang predates this PR, but the new "degrades instead of hanging" claim covers only the git calls. The honest scope is now stated in the code; a bounded-canonicalize helper is the natural follow-up.
• Anthropic non-streaming envelope still has no direct regression test (the body's old wording implied a weaker test existed; wording now corrected via the round section). The full-suite parallel-load flakes and the remaining documented gaps in "Notes for reviewers" stand as written.

Verification

• cargo fmt --check, cargo clippy --all-features --locked -- -D warnings clean for both touched crates; cargo check --tests --locked clean.
• All targeted suites listed above green after the fixes; the 9 approval tests confirm the rescue restructure keeps approval_decision_queued_before_shutdown_is_honored_not_interrupted passing, and the snapshot suite confirms the new init predicate plus the reader-test rescope.
• Cosmetic, for a future rebase only: f7921abe1 carries its Signed-off-by trailer mid-body; DCO passes, but the trailer should close the message.