fix(tui): stop the subagent contention gate from falsely refusing child shell calls - #5732
Conversation
…ld shell calls Dogfood repro: a delegated child's bash was refused with "another child is writing in this shared checkout" even for read-only commands and even when the peer was long finished; the parent burned turns, cancelled the child, and redid the work in the foreground. Root causes and repairs, all in the claim/liveness layer: - The gate ignored the read-only evidence the execution envelope had already computed. A shell run the read-only classifier proves mutation-free can never collide with a peer's writes, so the gate now carves it out (proven_readonly_shell_run shares the classifier, not its role restriction — the envelope grant is role-scoped because it grants shell to inspection roles; the gate grants nothing and only asks about collision). - A panicked child never reached its terminal commit (spawn_supervised caught the panic and returned), so it stayed Running and gated peers until heartbeat auto-cancel — indefinitely while a lingering shell kept the heartbeat fresh. run_subagent_task now runs its body under a guard that commits a crash terminal result through the same finish_terminal_result arbitration, then resumes unwinding so the supervisor still logs and dumps. - A headless worker record parked at WaitingForUser (no paired agent entry, so no user can ever answer it) counted as a live claimant forever. The liveness predicate now excludes it; a paired waiting child stays live. - active_coordination_owners duplicated the liveness predicate and had drifted from it; it now delegates, so claim admission, the peer gate, and stale-claim release share one definition — and agents/coordinate action=release can actually clear a leaked claim again. - The refusal names the blocking peer ids and the concrete remediations (wait, parent-cancel, release a stale claim, relaunch with worktree isolation) instead of dead-ending the child. Regression tests (all fail on their bug signature pre-fix, pass post-fix): contended_shared_writers_keep_proven_readonly_shell, contended_shared_writer_refusal_names_blocking_peer_and_remediation, panicked_child_task_terminalizes_and_stops_gating_peers, waiting_for_user_headless_worker_is_not_a_live_coordination_owner. Verified: 4/4 new tests; 169/169 claim/contention/coordination families; full lib suite 11530 passed / 0 failed on post-#5724 main; fmt clean. Pre-existing clippy too_many_arguments at runtime_threads.rs:2562 is on main and untouched by this change (reproduced with this change stashed). Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @Hmbown's task in 3m 31s —— View job Review complete
Note: SummaryThis is a well-scoped fix touching exactly the claim/liveness layer described in the PR body. I traced each of the four claimed root causes against the code and they hold up:
Nothing incorrect found. The blocking-peer names in the refusal message ( TestsThe four new tests each map 1:1 to a root cause and match the described "proven red pre-fix" methodology:
Signatures used in the new tests ( Minor observations (non-blocking)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| body: impl std::future::Future<Output = ()> + Send, | ||
| ) { | ||
| use futures_util::FutureExt; | ||
| let panicked = std::panic::AssertUnwindSafe(body).catch_unwind().await; |
There was a problem hiding this comment.
🟡 Panics release the parent barrier early
When a child panics, catch_unwind drops its foreground registration before the crash result is committed. The parent can settle without receiving the failure.
Prompt for agents
In crates/tui/src/tools/subagent/mod.rs, supervise_subagent_task_body catches a panic around run_subagent_task_inner(task), but the inner future owns SubAgentTask and therefore its ForegroundChildRegistration. Panic unwinding drops that registration before supervise_subagent_task_body calls finish_terminal_result. Keep the foreground registration alive in the outer supervisor until terminal failure delivery and commit finish, while preserving normal access to its parking signal and the existing single-winner terminal arbitration. Add a regression test that blocks or observes terminal delivery and proves the foreground registry does not settle before the panic result is committed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // A shell run the read-only classifier proves mutation-free cannot | ||
| // collide with the peer's writes no matter how contended the | ||
| // checkout is, so the gate below does not apply to it. | ||
| && !proven_readonly_shell_run(name, &input) |
There was a problem hiding this comment.
📝 Info: Read-only carve-out stays bounded
proven_readonly_shell_run runs after all authority checks and shares the executor’s classifier. It bypasses only contention, not shell or mutation policy.
Was this helpful? React with 👍 or 👎 to provide feedback.
| && !(record.status == AgentWorkerStatus::WaitingForUser | ||
| && !self.agents.contains_key(id)) |
There was a problem hiding this comment.
📝 Info: Interactive waiting claims remain live
The exclusion requires no paired agent record. Paired children remain live through their waiting worker record, even when their agent status is interrupted.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc094abe1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A shell run the read-only classifier proves mutation-free cannot | ||
| // collide with the peer's writes no matter how contended the | ||
| // checkout is, so the gate below does not apply to it. | ||
| && !proven_readonly_shell_run(name, &input) |
There was a problem hiding this comment.
When two write-capable children are contended, this skips coordination for commands such as git diff, but builders execute Bash under ShellPolicy::Full, not the hardened read-only path. Consequently the command still runs shell hooks and honors Git helpers such as a configured external diff command, either of which can mutate the shared checkout while the peer is writing. Only bypass the contention gate if execution is also forced through the read-only environment/sandbox, rather than relying solely on the input classifier.
Useful? React with 👍 / 👎.
| manager.live_peer_shared_write_claim_owners(&self.owner_agent_id); | ||
| if !blocking_peers.is_empty() { | ||
| return Err(anyhow!( | ||
| "Tool {name} cannot prove a bounded file target, and another child is writing in this shared checkout (blocking peers: {}). Wait for the peer to finish, ask the parent to cancel it, run agents/coordinate action=release to clear a stale claim, or relaunch the children with worktree isolation.", |
There was a problem hiding this comment.
When this refusal occurs, it tells the child to run agents/coordinate action=release, but that legacy tool is explicitly retired from the model catalog and AgentToolAction exposes no release action. The child therefore cannot perform the advertised stale-claim recovery and may retry or remain blocked; expose recovery through agent or direct the request to a genuinely reachable operator path.
AGENTS.md reference: AGENTS.md:L89-L91
Useful? React with 👍 / 👎.
| result.status = | ||
| SubAgentStatus::Failed(format!("sub-agent task panicked: {message}")); | ||
| result.result = None; | ||
| result.needs_input = None; | ||
| // Arbitrated exactly like the natural terminal commit: when a | ||
| // cancel or another terminal outcome already won, this is a | ||
| // no-op rather than a second result. | ||
| manager.finish_terminal_result(&agent_id, result, false, true); |
There was a problem hiding this comment.
If the child panics while one of its background Bash jobs is still running, this marks the owner terminal without cancelling that job. is_live_coordination_owner then ignores the child's claim, so another writer can be admitted while the orphaned process is still mutating the shared checkout—the exact lingering-shell case described by the new panic guard. Cancel or otherwise retain coordination ownership for running shell jobs before committing the failed terminal state.
Useful? React with 👍 / 👎.
| body: impl std::future::Future<Output = ()> + Send, | ||
| ) { | ||
| use futures_util::FutureExt; | ||
| let panicked = std::panic::AssertUnwindSafe(body).catch_unwind().await; |
There was a problem hiding this comment.
For a turn-owned child, unwinding body drops the SubAgentTask and its ForegroundChildRegistration as this awaited catch_unwind completes, before the handler acquires the manager lock and publishes the terminal result. If the parent is settling concurrently, its barrier can therefore observe zero children, flush, and emit TurnComplete before the panic receipt is delivered. Keep the registration outside the caught body, or otherwise release it only after finish_terminal_result completes.
Useful? React with 👍 / 👎.
| let message = crate::utils::panic_message(&*panic); | ||
| { | ||
| let mut manager = manager_handle.write().await; | ||
| match manager.get_result(&agent_id) { | ||
| Ok(mut result) => { | ||
| result.status = | ||
| SubAgentStatus::Failed(format!("sub-agent task panicked: {message}")); |
There was a problem hiding this comment.
When a panic payload contains user data—for example, Rust slicing and assertion panics can embed the entire offending string—this copies that payload into SubAgentStatus::Failed. Terminal delivery then places the error in the mailbox and subagent.failed sentinel, so it can be appended to the parent transcript and sent in the next provider request; previously the detailed panic was confined to local logging and the crash dump. Publish a bounded generic failure here and retain the raw message only in the local diagnostic path.
Useful? React with 👍 / 👎.
1 participant
Dogfood repro: a delegated child's bash was refused with "another child is writing in this shared checkout" even for read-only commands and even when the peer was long finished; the parent burned turns, cancelled the child, and redid the work in the foreground.
Root causes fixed (claim/liveness layer)
ls,git status) is now carved out of the contention refusal.spawn_supervisedcaught the panic, the terminal commit never ran), stayingRunningand gating peers until heartbeat auto-cancel — indefinitely with a lingering shell. The task body now runs under a guard that commits a crash terminal result through the existing arbitration, then resumes unwinding.WaitingForUserworker records (no paired agent entry — no user can ever answer) counted as live claimants forever; the predicate now excludes them.active_coordination_ownersduplicated and drifted from the liveness predicate — it now delegates, so admission, the peer gate, andagents/coordinate action=releaseshare one definition and release can actually clear a leaked claim.Tests
Four regression tests, each proven red on its bug signature pre-fix and green post-fix; 169/169 claim/contention/coordination families; full lib suite 11530/0 on post-#5724 main; fmt clean. (One pre-existing main clippy lint at runtime_threads.rs:2562 is untouched and reproduced with this change stashed.)
No-Issue: dogfood-reported defect, tracked in the Ops takeover plan (Lane 1).