Skip to content

feat(control) 5/5: a person in a session no longer fails a dispatch - #185

Merged
senamakel merged 2 commits into
split/4-wirefrom
split/5-control
Aug 5, 2026
Merged

feat(control) 5/5: a person in a session no longer fails a dispatch#185
senamakel merged 2 commits into
split/4-wirefrom
split/5-control

Conversation

@sanil-23

@sanil-23 sanil-23 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Stack 5/5 — base split/4-wire. Depends on: 1/5, 2/5, 3/5, 4/5.

# PR Branch Base Layer
1 #181 split/1-agent-model main session origin/name + agent declarations (model only)
2 #182 split/2-tree-ui split/1-agent-model Hosts and Agents tabs become one Host → Agent → Session tree
3 #183 split/3-vocabulary split/2-tree-ui rename: "harness" is a type, not an entity
4 #184 split/4-wire split/3-vocabulary hub advert: hosts[], per-agent hostId/maxSessions, result sessionId
5 #185 split/5-control split/4-wire dispatch candidacy, hold/hand-back, host-wide cap removed

Each PR's diff shows only its own layer. split/5-control's tree is byte-identical to
bcc61e63, the merge commit on #180.


Last of five stacked PRs replacing #180. Review 1/5 – 4/5 first. The tip of this branch
is byte-identical to bcc61e63, the merge commit on #180.

What this layer does

feat(control): a person in a session no longer fails a dispatch

Phase E of the agent topology: dispatch candidacy and the control semantics around a
session an operator is holding. All of it is exercisable on one machine — declare two
agents, dispatch, take a session, watch the work route around it, hand back, see the
result.

  • Candidacy (E1) is now two independent rules that only coincide today. A hold is on a
    session: claim_idle already refuses to hand a user-owned session to the
    orchestrator, so reuse is consulted first and a person working never makes a dispatch
    fail. Serialization is on the checkout: under strategy: checkout an agent's sessions
    share one working tree, so a fresh session queues rather than starting beside the writer
    already there. PtyManager::operator_hold(cwd) is replaced by the neutral
    sessions_in(cwd), with the policy stated where the strategy lives.
  • A mid-turn takeover suspends instead of discarding (E3): the fold, its events, usage
    and workspace context are retained, everything already written is folded out first, and
    the task stays open. Held time does not accrue against the worker's idle ceiling, and
    the worker announces the hold so the hub's no-progress window pauses too (E4) — gated
    exactly like link liveness, so a worker that dies mid-hold is still reaped.
  • On hand-back the runtime runs a fresh turn in that same session (E5), prompted with
    the original instruction plus a directive to review the session history and workspace
    state and either report the finished work or complete it. Its answer is emitted as the
    pending task's result, under the same task id, because it is the same call.
  • The blanket harnessHeld refusal is retired (E6): the only path left to it is a
    queue that outlived the caller's budget, and that frame stays byte-identical so the
    backend's retry stays a retry.
  • Control state is no longer advertised at all. control, controlReason,
    controlSince and the handback brief are per-agent keys describing a per-session fact,
    and a backend folding them by agentId would mark every task on an agent as held when a
    person took one session.

perf(daemon): lift the host-wide task cap so work stops queueing for a slot

Both daemons capped concurrent tasks at 2, so a third dispatch waited on a semaphore
permit even when it targeted a different agent in a different workspace. That cap predates
declared agents. The limits that own the real hazard now sit at its grain: per-agent
max_sessions and checkout serialization. The semaphore stays as the accounting behind
active_count, and an operator can still set concurrency to impose a real cap.

Reordering note

feat(control) was the eighth commit in the original branch, ahead of
fix(hub): stop advertising hostId. It moved down one slot so the wire layer (4/5) is
self-contained. It replayed with no conflicts and the tip is unchanged.

What a reviewer should look at closely

  • The dropped host-wide cap is the riskiest single change in the stack. It is safe
    only if the per-agent and per-checkout limits genuinely cover the hazard the cap used to
    cover. Worth checking that an agent with strategy: checkout really cannot get two
    concurrent writers.
  • Held time and the no-progress window. E4 pauses the hub's watchdog while a session is
    held; the gate is deliberately the same as link liveness so a worker that dies mid-hold
    is still reaped. That gate is the thing that stops "held" becoming "leaked forever".
  • The hand-back turn is now the only way a held in-flight task reaches a result, so a
    dispatch that meets a person always ends in a real result or a real error — never in
    silence. Any path that can still drop it is a hang.
  • Control keys leaving the wire is a behaviour change for anything that was reading
    them. The local state and every behaviour it drives are unchanged; only the advert is
    quieter.

Validation

cargo fmt --all -- --check
cargo clippy --locked --all-targets -- -D warnings
cargo check --locked --all-targets
cargo build --locked --all-targets
cargo test --locked --no-fail-fast

Result on this branch: fmt, clippy -D warnings, check, and build all clean.
test reports 3900 passed, 5 failed, 14 ignored on macOS. All five failures are
pre-existing on main and unrelated to this stack:

  • daemon::providers::acp::tests::execution::a_new_acp_session_is_reported_before_the_task_completes
  • daemon::providers::tests::direct_runs_report_the_session_before_workspace_context
    (both are idle-watchdog timing tests; they pass in isolation)
  • worker::pty::tests::session::a_launch_root_preserves_trailing_whitespace
  • worker::pty::tests::session::a_session_snapshots_head_before_the_harness_can_commit
  • worker::pty::tests::session::an_unborn_repository_records_its_root_without_a_launch_commit
    (the last three are the macOS /var vs /private/var symlink)

Coverage was deliberately not run locally; CI owns that gate.

Summary by CodeRabbit

  • New Features

    • Operator-held sessions now pause active work and resume after control is returned, preserving task progress.
    • Tasks targeting occupied workspaces queue instead of failing immediately, with clear retryable outcomes when waiting expires.
    • Remote sessions are identified in the interface; they can be watched but not controlled locally.
    • Session hold and resume updates are delivered immediately.
    • Increased default concurrency allows more simultaneous tasks while retaining configurable limits.
  • Bug Fixes

    • Watchdog timers no longer expire while an announced session hold is active.
    • Remote-session state no longer remains visible after changing views.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The daemon now emits unthrottled hold and resume markers and allows higher default concurrency. The hub pauses watchdogs during holds. The TUI queues conflicting work, suspends active turns during takeover, resumes them after hand-back, and blocks takeover of remote sessions.

Changes

Session control flow

Layer / File(s) Summary
Daemon markers and concurrency
src/sdk/src/daemon/...
The daemon exports hold and resume markers, forwards them immediately, and raises default concurrency to 1024.
Hub hold state and watchdogs
src/sdk/src/hub/runner/..., src/sdk/src/hub/roster/..., src/sdk/src/hub/tests/...
The hub shares held state with watchdogs, pauses timeout windows during holds, and omits control state from agent metadata.
Executor queue and hand-back
src/tui/src/worker/executor/..., src/tui/src/worker/pty/...
The executor queues checkout-conflicting tasks, suspends active turns during takeover, and resumes them with a hand-back prompt.
Remote-session takeover guard
src/tui/src/ui/app/...
The UI tracks remote session selections and rejects local takeover while preserving remote viewing.
Control-flow validation
src/tui/src/worker/executor_tests/..., src/tui/tests/e2e_session_takeover.rs
Tests cover queueing, takeover suspension, hand-back, watchdog behavior, and session preservation.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Dispatcher
  participant PtyManager
  participant PtySessionExecutor
  participant Operator
  Dispatcher->>PtyManager: inspect workspace sessions
  PtyManager-->>Dispatcher: return session candidates
  Dispatcher->>PtySessionExecutor: queue or start task
  Operator->>PtySessionExecutor: take over active session
  PtySessionExecutor->>Operator: emit held marker
  Operator->>PtySessionExecutor: return control
  PtySessionExecutor->>PtySessionExecutor: inject hand-back prompt
Loading

Possibly related PRs

Suggested reviewers: senamakel

Poem

A rabbit watched the sessions hold,
Then queued each task with care.
The turn stayed warm through operator hands,
And resumed when control was fair.
Remote rows now gently say,
“Watch here, but do not take away.”

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main dispatch-control change: operator-held sessions no longer cause dispatches to fail.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@sanil-23

sanil-23 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@sanil-23

sanil-23 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@sanil-23

sanil-23 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

sanil-23 and others added 2 commits August 5, 2026 14:10
Phase E of the agent topology: dispatch candidacy and the control
semantics around a session an operator is holding. Everything here is
exercisable on one machine — declare two agents, dispatch, take a
session, watch the work route around it, hand back, see the result.

Candidacy (E1) is now two independent rules that only coincide today.
A hold is on a *session*: `claim_idle` already refuses to hand a
user-owned session to the orchestrator, so reuse is consulted first and
a person working never makes a dispatch fail. Serialization is on the
*checkout*: under `strategy: checkout` an agent's sessions share one
working tree, so a fresh session cannot start beside the writer that is
there — it queues. `PtyManager::operator_hold(cwd)` ("is this workspace
held") is replaced by `sessions_in(cwd)`, a neutral query, with the
policy stated where the strategy lives. Under `worktree` (§G) rule 2
simply stops applying, with no control logic to revisit. F3 still owns
serializing two *orchestrator* sessions in one checkout, which main
allows and which is a scheduling change, not a control one.

A mid-turn takeover suspends instead of discarding (E3): the fold, its
events, usage and workspace context are retained, everything already
written is folded out first, and the task stays open. Held time does not
accrue against the worker's idle ceiling, and the worker announces the
hold so the hub's no-progress window pauses too (E4) — gated exactly
like link liveness, so a worker that dies mid-hold is still reaped once
the session comes back. On hand-back the runtime runs a *fresh* turn in
that same session (E5), prompted with the original instruction plus a
directive to review the session history and workspace state and either
report the finished work or complete it; its answer is emitted as the
pending task's result, under the same task id, because it is the same
call. The blanket `harnessHeld` refusal is retired (E6): the only path
left to it is a queue that outlived the caller's budget, which keeps its
frame byte-identical so the backend's retry stays a retry. Taking
control is gated to local sessions and says so by name (E7).

Control state is no longer advertised at all: `control`, `controlReason`,
`controlSince` and the handback brief are per-agent keys describing a
per-session fact, and a backend folding them by `agentId` would mark
every task on an agent as held when a person took one session. The local
state and every behaviour it drives are unchanged; only the wire is
quieter. That makes the hand-back turn the *only* way a held in-flight
task reaches a result, so a dispatch that meets a person now always ends
in a real result or a real error — never in silence.

Co-Authored-By: Claude <noreply@anthropic.com>
…a slot

Both the networked and embedded daemons capped concurrent tasks at 2, so a
third dispatch waited on a semaphore permit even when it targeted a
different agent in a different workspace — where nothing could collide.
That cap predates declared agents: it was the only guard when a machine
was one worker with one implicit session.

The limits that own the real hazard now sit at its grain: per-agent
max_sessions derived from the workspace strategy, and the checkout
serialization that keeps a second writer out of a tree someone is in. A
host-wide count knows about neither, so it could only delay work that was
already safe.

The semaphore stays as the accounting behind active_count, and an operator
can still set concurrency to impose a real cap on a small machine.

Co-Authored-By: Claude <noreply@anthropic.com>

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@sanil-23

sanil-23 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/tui/src/ui/app/types.rs (1)

1126-1135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Keep the file below 500 lines of code.

src/tui/src/ui/app/types.rs contains 490 non-blank, non-comment lines, so it does not currently violate the 500-line limit. Split the file before future changes exceed the limit.

🤖 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/tui/src/ui/app/types.rs` around lines 1126 - 1135, Keep
src/tui/src/ui/app/types.rs under 500 non-blank, non-comment lines by moving
related type definitions or fields, including pane_remote_session, into an
appropriate module before adding further content. Update module declarations and
references so behavior and visibility remain unchanged.

Source: Coding guidelines

src/tui/src/worker/executor_tests/control.rs (1)

283-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the two abort branches on the hold path.

hold.rs adds two error branches that no test in this module reaches:

  • await_checkout_release returns "task aborted while queued behind an operator" when the requester aborts while the dispatch is parked in the queue.
  • await_handback returns "{provider} task aborted while an operator held the session" when the requester aborts during a hold.

Both are ordinary events: an orchestrator that gives up on a task while a person is at the keyboard. Neither is pinned, so a regression that ignores the abort and waits out the full budget would pass this suite. The second branch also carries a specific invariant worth asserting — the operator's session must survive the abort, because finish_turn skips close while control == User.

Each test mirrors an existing one and only needs opts.abort retained and aborted mid-wait.

Do you want me to generate both tests?
As per coding guidelines: "Cover new branches and maintain the 80% line-coverage gate defined by .github/workflows/ci.yml."

🤖 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/tui/src/worker/executor_tests/control.rs` around lines 283 - 327, Add two
focused tests in control.rs to cover the new abort branches in hold.rs: one for
await_checkout_release returning the queued-abort error and one for
await_handback returning the held-abort error. Reuse the existing harness,
options, and fake_harness_script setup, keep opts.abort enabled, trigger the
abort mid-wait, and assert the exact abort message path is reached instead of
waiting for the full timeout. In the await_handback case, also assert the
operator session still exists after the abort to preserve the
finish_turn/control == User behavior.

Source: Coding guidelines

src/sdk/src/hub/runner/mod.rs (1)

101-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the new behavior out of mod.rs.

Both changes add behavior to directory module roots. Keep these files as module wiring.

  • src/sdk/src/hub/runner/mod.rs#L101-L120: move live_sleep into a focused watchdog submodule and import it from mod.rs.
  • src/sdk/src/hub/runner/pump/mod.rs#L157-L190: move control-marker parsing and status-control handling into a focused pump submodule.

As per coding guidelines, **/mod.rs: “Keep mod.rs focused on module documentation, mod/pub use wiring, and glue that fits no more specific submodule.”

🤖 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 101 - 120, Move live_sleep from
src/sdk/src/hub/runner/mod.rs lines 101-120 into a focused watchdog submodule,
then import or re-export it from the runner module root without changing its
behavior. Also move the control-marker parsing and status-control handling from
src/sdk/src/hub/runner/pump/mod.rs lines 157-190 into a focused pump submodule,
wiring it through pump/mod.rs; both module roots should retain only
documentation, module declarations, re-exports, and necessary glue.

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/render/agents/mod.rs`:
- Around line 129-140: Move the session-selection and remote-session
classification logic from Agents::render in
src/tui/src/ui/app/render/agents/mod.rs:129-140 into a dedicated Agents child
module, preserving its behavior and wiring the module from mod.rs. Also move the
frame-state reset behavior from src/tui/src/ui/app/render/mod.rs:297-299 into a
dedicated render child module; both mod.rs files should retain only
documentation, module/public-use wiring, and unavoidable glue.

In `@src/tui/src/ui/app/session_control_tests.rs`:
- Around line 42-46: Extend the session-control tests around
app.take_session_control to exercise the render-driven lifecycle: create a
remote RailRow::Session, draw the Agents view so remote-session classification
runs, then invoke take_session_control; subsequently configure local sessions,
draw a non-Agents tab to trigger the per-frame reset, and verify the resulting
empty-row behavior is unaffected by stale remote state. Replace or supplement
direct pane_remote_session assignment with the render-based setup.

In `@src/tui/src/worker/executor/hold.rs`:
- Around line 63-74: Update handback_prompt to normalize instruction by
collapsing or replacing newline characters with spaces before interpolating it,
while preserving the existing prompt wording and behavior. Ensure the returned
string remains a single physical line even when the supplied instruction is
multi-line.

---

Nitpick comments:
In `@src/sdk/src/hub/runner/mod.rs`:
- Around line 101-120: Move live_sleep from src/sdk/src/hub/runner/mod.rs lines
101-120 into a focused watchdog submodule, then import or re-export it from the
runner module root without changing its behavior. Also move the control-marker
parsing and status-control handling from src/sdk/src/hub/runner/pump/mod.rs
lines 157-190 into a focused pump submodule, wiring it through pump/mod.rs; both
module roots should retain only documentation, module declarations, re-exports,
and necessary glue.

In `@src/tui/src/ui/app/types.rs`:
- Around line 1126-1135: Keep src/tui/src/ui/app/types.rs under 500 non-blank,
non-comment lines by moving related type definitions or fields, including
pane_remote_session, into an appropriate module before adding further content.
Update module declarations and references so behavior and visibility remain
unchanged.

In `@src/tui/src/worker/executor_tests/control.rs`:
- Around line 283-327: Add two focused tests in control.rs to cover the new
abort branches in hold.rs: one for await_checkout_release returning the
queued-abort error and one for await_handback returning the held-abort error.
Reuse the existing harness, options, and fake_harness_script setup, keep
opts.abort enabled, trigger the abort mid-wait, and assert the exact abort
message path is reached instead of waiting for the full timeout. In the
await_handback case, also assert the operator session still exists after the
abort to preserve the finish_turn/control == User behavior.
🪄 Autofix

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: 20060893-fc79-4db9-8e36-ff22eac25efd

📥 Commits

Reviewing files that changed from the base of the PR and between 6dc9ddd and db3ec0c.

📒 Files selected for processing (33)
  • src/sdk/src/daemon/embedded/types.rs
  • src/sdk/src/daemon/entry.rs
  • src/sdk/src/daemon/mod.rs
  • src/sdk/src/daemon/task_loop/run.rs
  • src/sdk/src/daemon/tests/capability_tests.rs
  • src/sdk/src/daemon/tests/mod.rs
  • src/sdk/src/daemon/types.rs
  • src/sdk/src/hub/roster/mod.rs
  • src/sdk/src/hub/runner/mod.rs
  • src/sdk/src/hub/runner/pump/mod.rs
  • src/sdk/src/hub/runner/types.rs
  • src/sdk/src/hub/socket/task_run.rs
  • src/sdk/src/hub/tests/handoff_advert.rs
  • src/sdk/src/hub/tests/held.rs
  • src/sdk/src/hub/tests/held_watchdog.rs
  • src/sdk/src/hub/tests/mod.rs
  • src/sdk/src/hub/tests/roster.rs
  • src/tui/src/ui/app/render/agents/mod.rs
  • src/tui/src/ui/app/render/mod.rs
  • src/tui/src/ui/app/session_control.rs
  • src/tui/src/ui/app/session_control_tests.rs
  • src/tui/src/ui/app/state.rs
  • src/tui/src/ui/app/types.rs
  • src/tui/src/worker/executor/hold.rs
  • src/tui/src/worker/executor/mod.rs
  • src/tui/src/worker/executor/run.rs
  • src/tui/src/worker/executor/types.rs
  • src/tui/src/worker/executor_tests/control.rs
  • src/tui/src/worker/executor_tests/mod.rs
  • src/tui/src/worker/executor_tests/sessions.rs
  • src/tui/src/worker/pty/manager/session.rs
  • src/tui/src/worker/pty/tests/control.rs
  • src/tui/tests/e2e_session_takeover.rs

Comment on lines +129 to +140
// A session row this device is not running: watchable, but not takeable
// (§E7). Recorded here because this is the only place that can tell the
// difference — one row down the cursor, both cases are a `None` session.
self.pane_remote_session = match (&selection.session, selection.rows.get(selection.active))
{
(None, Some(RailRow::Session(row))) => Some(
row.agent_id
.clone()
.unwrap_or_else(|| "another host".to_string()),
),
_ => None,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Move render behavior out of these mod.rs files.

Both changes add implementation logic to mod.rs. Keep each mod.rs limited to module documentation and wiring. Move the behavior into dedicated child modules.

  • src/tui/src/ui/app/render/agents/mod.rs#L129-L140: move session-selection and remote-session classification into an Agents child module.
  • src/tui/src/ui/app/render/mod.rs#L297-L299: move frame-state reset behavior into a dedicated render child module.

As per coding guidelines, “Keep mod.rs focused on module documentation, mod/pub use wiring, and glue that fits no more specific submodule.”

📍 Affects 2 files
  • src/tui/src/ui/app/render/agents/mod.rs#L129-L140 (this comment)
  • src/tui/src/ui/app/render/mod.rs#L297-L299
🤖 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/tui/src/ui/app/render/agents/mod.rs` around lines 129 - 140, Move the
session-selection and remote-session classification logic from Agents::render in
src/tui/src/ui/app/render/agents/mod.rs:129-140 into a dedicated Agents child
module, preserving its behavior and wiring the module from mod.rs. Also move the
frame-state reset behavior from src/tui/src/ui/app/render/mod.rs:297-299 into a
dedicated render child module; both mod.rs files should retain only
documentation, module/public-use wiring, and unavoidable glue.

Source: Coding guidelines

Comment on lines +42 to +46
let mut app = app();
app.pane_session = None;
app.pane_remote_session = Some("mac-studio-claude".to_string());

app.take_session_control();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Cover the render-driven remote-session lifecycle.

Lines 43-44 set pane_remote_session directly. The tests do not execute remote-row classification in src/tui/src/ui/app/render/agents/mod.rs Lines 132-140 or the per-frame reset in src/tui/src/ui/app/render/mod.rs Lines 297-299.

Add a test that selects a remote RailRow::Session, draws Agents, and invokes take_session_control. Then draw a non-Agents tab with local sessions configured and verify that stale remote state does not affect the empty-row result.

As per coding guidelines, “Cover new branches.”

Also applies to: 82-87

🤖 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/tui/src/ui/app/session_control_tests.rs` around lines 42 - 46, Extend the
session-control tests around app.take_session_control to exercise the
render-driven lifecycle: create a remote RailRow::Session, draw the Agents view
so remote-session classification runs, then invoke take_session_control;
subsequently configure local sessions, draw a non-Agents tab to trigger the
per-frame reset, and verify the resulting empty-row behavior is unaffected by
stale remote state. Replace or supplement direct pane_remote_session assignment
with the render-based setup.

Source: Coding guidelines

Comment on lines +63 to +74
/// One line, because it is typed into a composer. A line-oriented harness reads
/// a prompt up to the first newline, so a multi-line brief would arrive as a
/// prompt plus stray input.
pub(super) fn handback_prompt(instruction: &str) -> String {
format!(
"An operator took control of this session and has been working in it; \
you now have it back. Review this session's history and the current \
state of the workspace, then finish this task: {instruction} — if the \
work is already done, do not redo it: report the final result. \
Otherwise continue from where things now stand and complete it.",
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Collapse newlines in instruction before interpolating it.

The doc comment states the prompt must be one line, because a line-oriented harness reads a prompt up to the first newline. instruction is options.prompt, which a peer supplies and which is often multi-line. A multi-line instruction therefore splits the hand-back prompt: the harness reads the first physical line as the prompt, and the rest arrives as stray input on the next read. The wording match *"you now have it back"* sits in the first line, so the turn still starts, but it starts against a truncated task description.

Normalize the interpolated value so the invariant the doc states is enforced by the code.

🐛 Proposed fix to keep the prompt on one line
 pub(super) fn handback_prompt(instruction: &str) -> String {
+    // The invariant above is enforced here rather than assumed: a delegated
+    // instruction is routinely multi-line, and a raw interpolation would end
+    // the prompt at its first newline and type the remainder as stray input.
+    let instruction = instruction.split_whitespace().collect::<Vec<_>>().join(" ");
     format!(
         "An operator took control of this session and has been working in it; \
          you now have it back. Review this session's history and the current \
          state of the workspace, then finish this task: {instruction} — if the \
          work is already done, do not redo it: report the final result. \
          Otherwise continue from where things now stand and complete it.",
     )
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// One line, because it is typed into a composer. A line-oriented harness reads
/// a prompt up to the first newline, so a multi-line brief would arrive as a
/// prompt plus stray input.
pub(super) fn handback_prompt(instruction: &str) -> String {
format!(
"An operator took control of this session and has been working in it; \
you now have it back. Review this session's history and the current \
state of the workspace, then finish this task: {instruction}if the \
work is already done, do not redo it: report the final result. \
Otherwise continue from where things now stand and complete it.",
)
}
/// One line, because it is typed into a composer. A line-oriented harness reads
/// a prompt up to the first newline, so a multi-line brief would arrive as a
/// prompt plus stray input.
pub(super) fn handback_prompt(instruction: &str) -> String {
// The invariant above is enforced here rather than assumed: a delegated
// instruction is routinely multi-line, and a raw interpolation would end
// the prompt at its first newline and type the remainder as stray input.
let instruction = instruction.split_whitespace().collect::<Vec<_>>().join(" ");
format!(
"An operator took control of this session and has been working in it; \
you now have it back. Review this session's history and the current \
state of the workspace, then finish this task: {instruction}if the \
work is already done, do not redo it: report the final result. \
Otherwise continue from where things now stand and complete it.",
)
}
🤖 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/tui/src/worker/executor/hold.rs` around lines 63 - 74, Update
handback_prompt to normalize instruction by collapsing or replacing newline
characters with spaces before interpolating it, while preserving the existing
prompt wording and behavior. Ensure the returned string remains a single
physical line even when the supplied instruction is multi-line.

@senamakel
senamakel merged commit eb7dd35 into split/4-wire Aug 5, 2026
6 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants