feat(tui): kill watched harnesses with confirmation - #141
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe change adds negotiated screen-kill capabilities, task-scoped kill messages, authenticated termination routing, TUI confirmation, and orchestrator-aware worker session shutdown. Tests cover protocol compatibility, UI behavior, authorization, and end-to-end termination. ChangesScreen-kill capability and termination primitives
Hub negotiation and routing
TUI and worker behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TUI
participant Runtime
participant HubHandle
participant ScreenRouter
participant DaemonRuntime
participant PtyManager
TUI->>Runtime: kill_task(worker, task_id)
Runtime->>HubHandle: kill(worker, task_id)
HubHandle->>ScreenRouter: send Kill(task_id, correlation_id)
ScreenRouter->>DaemonRuntime: terminate_task(sender, task_id, correlation_id)
DaemonRuntime->>PtyManager: signal termination
PtyManager-->>ScreenRouter: close owned session
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![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: e2d04d0a92
ℹ️ 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".
# Conflicts: # src/sdk/src/runtime/backend/runtime.rs # src/sdk/src/runtime/mod.rs # src/tui/src/event_loop/cmd_dispatch/mod.rs # src/tui/src/ui/app/keys/mod.rs # src/tui/src/ui/app/types.rs # src/tui/src/worker/stream/tests.rs
The kill command now resolves the target from the current rail selection instead of relying on a cached watch target, preventing stale or mismatched kills. The daemon runtime gains a dedicated abort method that safely cancels a task while holding the lock, and the router uses it directly rather than closing sessions indirectly.
The abort signal now carries a termination flag that, when set, stops the serving harness and closes its session instead of merely sending an interrupt. This lets a kill request fully tear down a task while preserving the existing interrupt behaviour for ordinary aborts.
The test for task-scoped kill now polls the session state until the harness process actually stops, instead of asserting immediately after the kill. This makes the test robust against timing differences in process termination.
The session id was moved into the report callback, preventing its use later in the task. Cloning it first ensures the id remains available for the rest of the test flow.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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: d242b6fae9
ℹ️ 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".
The runner now records the task id on each dispatch waiter, and the screen kill message carries a correlation id so a delayed kill cannot match a later dispatch that reused the same task id. The daemon probe advertises the new screen_kill capability, which older workers omit and therefore deserialize as false, preventing an upgraded controller from sending them an unknown screen control message.
The kill path now checks that the worker advertises harness termination support and resolves the active dispatch correlation id before sending the kill message. The daemon verifies this correlation id when terminating, preventing stale or mismatched kill requests from aborting a task that has since been reused by a different dispatch.
The test fixtures for ScreenMessage::Kill were missing the correlation_id field, which is now required by the message structure. Added the field to all three test locations to keep the test data consistent with the updated message definition.
Add tests for the new screenKill agent capability, asserting it is omitted for older workers and defaults to false when absent. Also extend the TUI end-to-end screen stream test to verify that a stale kill dispatch with a reused task id does not stop the live session.
Reformat the serde attribute on `screen_kill` and the `terminate_task` guard condition to fit on single lines, improving code readability without changing behavior.
The terminate_task documentation now reflects that the operation is a termination signal rather than an abort, and explicitly describes how the correlation check prevents a delayed request from terminating a later dispatch that reused the same task id.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4ae395c62
ℹ️ 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".
The capability probe no longer acquires a concurrency slot before running, since control-plane negotiation must not block behind harness-task slots it may need to terminate. The probe remains serialized by the capability cache lock and bounded by its own timeout, and the TUI router now unsubscribes using the correct source channel when killing a task.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 026dd2192f
ℹ️ 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".
Cache the last successfully negotiated capabilities per worker address during task dispatch, and use this cached data in the kill path instead of issuing a fresh capability probe. This ensures an emergency kill can proceed even when a worker is wedged and unable to respond to a new negotiation request.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 697445cab6
ℹ️ 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".
Screen termination support is now advertised only when the embedding worker installs an authenticated screen-message router, preventing false capability claims in headless or unauthenticated contexts. The daemon runtime exposes an explicit enablement method, and the hub runner now negotiates capabilities with abort awareness so a backend cancellation during negotiation is honored before task dispatch, with stale capability entries cleared on refresh failure.
The daemon runtime now imports AtomicBool alongside the existing atomic types, preparing for upcoming state tracking that requires a boolean atomic flag.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The `run` method now calls `run_inner` directly instead of `run_with_screen_kill`, passing `None` for the screen-control parameter. This aligns the public API with the internal execution path and removes the now-redundant wrapper.
The stop_turn path now checks whether the operator has taken over the session before interrupting and closing it. This prevents the orchestrator from terminating a session that has been handed to the user, avoiding a race between control handoff and termination.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 218c4ede65
ℹ️ 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".
The capabilities negotiation now always returns the abort notification handle alongside the result, so callers can still use it to cancel a task even when capability discovery fails. This simplifies the dispatch flow by removing the need to reconstruct the abort handle from an optional tuple.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701c292ad5
ℹ️ 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".
The task runner now checks worker capabilities and abort state before dispatching a task, allowing the control socket to return early with an aborted outcome when the task has already been cancelled. This prevents unnecessary work and ensures the screen kill capability is properly negotiated with the worker before execution begins.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c62e32a1ab
ℹ️ 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".
The hub now forwards the externally visible task id when running negotiated tasks, so abort signals and waiter registrations reference the id callers expect rather than the internal request id. This also clears the armed kill state on any mouse activity in the TUI, cancelling a pending harness kill and showing a status message when the user interacts.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sdk/src/hub/socket/task_run.rs (1)
165-182: 🚀 Performance & Scalability | 🔵 TrivialEvery task dispatch now pays a full capability round trip.
capabilities_for_dispatchruns before every dispatch, andTaskRunner::capabilitiesclears its cache and performs a fresh network round trip (bounded byself.ack_window) each time — there is no short-lived reuse across back-to-back dispatches to the same worker. This adds per-task latency to every dispatch, including ones with an explicitproviderthat do not need capability data for routing.This trade-off looks intentional: reusing a cached result would reintroduce the exact staleness problem this negotiation exists to avoid (an emergency kill needing up-to-date
screen_killsupport). Monitor per-dispatch latency after this change, particularly for workflows that delegate many small tasks in quick succession to the same worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdk/src/hub/socket/task_run.rs` around lines 165 - 182, Review the pre-dispatch negotiation around TaskRunner::capabilities_for_dispatch and the TaskRunner::capabilities cache behavior; avoid introducing cached capability reuse that could make emergency kill support stale. Preserve the fresh, ack_window-bounded negotiation for dispatches requiring capability data, while considering whether explicit-provider dispatches can bypass capability negotiation without affecting abort handling or task result emission.src/sdk/src/hub/runner/mod.rs (1)
225-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving the new kill/dispatch logic out of
mod.rs.
kill_correlation_for,run_negotiated, andrun_inneradd new behavior-heavy code directly torunner/mod.rs. This crate already extracts comparable logic — capability negotiation — into its own file,runner/capabilities.rs. Move the new kill and dispatch logic to a dedicated submodule, for examplerunner/dispatch.rs, and re-export it frommod.rs.As per coding guidelines, "Keep
mod.rsfocused on module documentation,mod/pub usewiring, and glue that fits no more specific submodule."Also applies to: 282-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdk/src/hub/runner/mod.rs` around lines 225 - 236, Move the behavior-heavy kill and dispatch implementations, including kill_correlation_for, run_negotiated, and run_inner, from runner/mod.rs into a dedicated runner/dispatch.rs submodule. Keep mod.rs limited to module wiring and glue, declare the new submodule there, and re-export the moved public API so existing callers retain the same access path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui/src/ui/app/keys/agents.rs`:
- Around line 85-93: Add an App helper near set_status, such as arm_kill, that
sets the kill confirmation status and then assigns kill_armed last; update the
KeyCode::Char('K') handling to call this helper instead of performing the two
operations inline, preserving the existing no-target status path.
In `@src/tui/src/ui/app/keys/mod.rs`:
- Around line 47-56: Update the kill confirmation branch in the key handling
function around self.kill_armed to confirm only an unmodified lowercase y key;
require that Ctrl, Alt, and other modifiers are absent before returning
Cmd::KillTask, while preserving cancellation for all other keypresses.
---
Nitpick comments:
In `@src/sdk/src/hub/runner/mod.rs`:
- Around line 225-236: Move the behavior-heavy kill and dispatch
implementations, including kill_correlation_for, run_negotiated, and run_inner,
from runner/mod.rs into a dedicated runner/dispatch.rs submodule. Keep mod.rs
limited to module wiring and glue, declare the new submodule there, and
re-export the moved public API so existing callers retain the same access path.
In `@src/sdk/src/hub/socket/task_run.rs`:
- Around line 165-182: Review the pre-dispatch negotiation around
TaskRunner::capabilities_for_dispatch and the TaskRunner::capabilities cache
behavior; avoid introducing cached capability reuse that could make emergency
kill support stale. Preserve the fresh, ack_window-bounded negotiation for
dispatches requiring capability data, while considering whether
explicit-provider dispatches can bypass capability negotiation without affecting
abort handling or task result emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18b2d91f-bac2-40b3-b88c-6b463371f9ec
📒 Files selected for processing (33)
src/sdk/src/daemon/capabilities/mod.rssrc/sdk/src/daemon/providers/types.rssrc/sdk/src/daemon/runtime.rssrc/sdk/src/daemon/task_loop/probe.rssrc/sdk/src/daemon/types.rssrc/sdk/src/hub/handle/mod.rssrc/sdk/src/hub/runner/capabilities.rssrc/sdk/src/hub/runner/mod.rssrc/sdk/src/hub/runner/types.rssrc/sdk/src/hub/socket/task_run.rssrc/sdk/src/runtime/mod.rssrc/sdk/src/runtime/openhuman/mod.rssrc/sdk/src/tinyplace/frames/tests/capabilities.rssrc/sdk/src/tinyplace/frames/tests/codec.rssrc/sdk/src/tinyplace/frames/tests/mod.rssrc/sdk/src/tinyplace/frames/types.rssrc/sdk/src/tinyplace/screen/tests.rssrc/sdk/src/tinyplace/screen/types.rssrc/tui/src/event_loop/cmd_dispatch/mod.rssrc/tui/src/ui/app/input.rssrc/tui/src/ui/app/keys/agents.rssrc/tui/src/ui/app/keys/mod.rssrc/tui/src/ui/app/render/agents/composer.rssrc/tui/src/ui/app/state.rssrc/tui/src/ui/app/tests.rssrc/tui/src/ui/app/types.rssrc/tui/src/worker/executor/run.rssrc/tui/src/worker/pty/handle/control.rssrc/tui/src/worker/pty/manager/session.rssrc/tui/src/worker/stream/router.rssrc/tui/src/worker/stream/tests.rssrc/tui/src/worker_loop/commands.rssrc/tui/tests/e2e_screen_stream.rs
The call to run_negotiated in hub_ops.rs was reformatted to fit on a single line, reducing unnecessary line breaks without changing any behavior.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The kill confirmation is now armed through a dedicated method that sets the status and stores the target as one state transition, and the confirming `y` keypress only proceeds when no modifiers are held, preventing accidental kills with modified key combinations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b1cfc308
ℹ️ 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".
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35072e96ea
ℹ️ 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".
The kill path now sends the original wire task id to the worker instead of the visible task id, ensuring the worker can correctly identify and terminate the intended task. Additionally, the capabilities probe now returns cached values immediately while refreshing them in the background when screen kill is active, avoiding a blocking wait during task termination.
The input handler now checks that a task is still running before routing key events to it, preventing stale or completed tasks from receiving input.
The kill command now uses a dedicated kill_target method that only returns a target when the selected task is running, preventing accidental termination of non-running tasks. The watch_target method no longer filters by task status, allowing it to serve other purposes without the kill-specific constraint.
The kill confirmation tests previously used a bare app fixture without a running task, which did not reflect the real scenario where a task is active. A new helper now seeds a running task before these tests, ensuring the kill flow is exercised against a realistic state.
The test helper `app_with_running_task` referenced `TuiEvent` via the old module path, which has been moved to `crate::ui::events`. The import is updated to reflect the new location, ensuring the test compiles and runs correctly.
The test helper now constructs the app first and then removes any TaskComplete events for the demo task from the snapshot, rather than scripting a TaskStart event before app creation. This aligns the fixture with the current event flow where task completion is the relevant state to filter.
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
# Conflicts: # src/tui/src/ui/app/input.rs
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b55896b2f0
ℹ️ 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".
| .inner | ||
| .screen_kill | ||
| .load(std::sync::atomic::Ordering::Relaxed) | ||
| && self.inner.capabilities.lock().await.is_none() |
There was a problem hiding this comment.
Fresh evidence after the cold-probe fix: when an earlier capability request has spawned the background get_capabilities, that function holds inner.capabilities across the inference, so a later pre-dispatch request blocks on this .lock().await before it can enter the static-response branch. If inference exceeds the runner's three 12-second acknowledgement windows, the task is dispatched with screen_kill: false; thus a backend capability probe immediately followed by a task can still make that task permanently unkillable. Track the in-progress probe separately or otherwise return the static control facts without waiting on this mutex.
Useful? React with 👍 / 👎.
| if abort.is_aborted() { | ||
| // A real interrupt, not a kill: Ctrl-C reaches the harness the | ||
| // same way the operator's would, and the session survives it. | ||
| let _ = self.sessions.write(id, &[0x03]); | ||
| if abort.is_terminated() { | ||
| self.stop_turn(id); |
There was a problem hiding this comment.
When a kill arrives before await_turn starts—while the PTY executor is selecting/opening a session or waiting in inject_prompt—the abort is not observed until this loop. The executor can therefore launch a new harness, spend the readiness/dialog/paste budgets waiting, and even submit the task after the operator confirmed the kill, only closing it afterward. Race setup and prompt injection against the abort signal, or check termination throughout those stages, so a confirmed kill cannot start additional work.
Useful? React with 👍 / 👎.
1 participant
Summary
Kaction for the selected watched harness in the Agents railyconfirmation before sending the destructive requestUser impact
Operators can recover from a hung harness without leaving the TUI. Press
Escto focus the Agents rail, select the task, pressK, then pressy; any other key cancels.Validation
cargo testcargo clippy --all-targets -- -D warningscargo fmt --checkSummary by CodeRabbit
New Features
K, with explicit confirmation.Bug Fixes
Compatibility