feat(ui) 2/5: both tabs become the Host → Agent → Session tree - #182
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughThis PR adds shared local-host resolution, a Host → Agent → Session data model, hierarchical Hosts and Agents TUI views, agent declaration and session actions, and navigation between orchestrator-started sessions and session rows. ChangesHosts, agents, and session flows
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
This was referenced Aug 5, 2026
Merged
The page rendered `Runtime::workers()` flat and called each row a host — its own comment admitted the collapse. That was true only while a machine advertised one worker; a machine now declares one agent per `harness × workspace`, so the list was agents with the host level taken out of it, and a fleet you cannot see the shape of is one you cannot manage. The tab is now the topology the advert is a projection of (spec §2.4): - **The hosts this machine runs come first, always.** They are resolved from config, not from what started, so a host that is declared but idle is still listed — that is the state where the operator most needs to see it. A running primary overrides its identity from the live observation, since `[host].workspace` is usually blank and only it has resolved that. - **Each host carries its agents**, from two sources that are deliberately not merged: declarations for a local host (an agent exists because it is written down), and the roster for a remote one — because the host link does not exchange declared agent lists yet (plan §D1). A remote host says so on its own row and again in its preview rather than passing this hub's roster off as that machine's declarations. - **The cursor walks both levels.** A host row previews the machine — capacity, readiness, budgets, read from whichever entry probed it — and an agent row previews the thing a dispatch targets, with the role toggles. **Roles are assigned per agent and persisted.** `set_roles` moves the roster this process holds, and the roster is rebuilt from the declarations at every launch — so assigning a role that way was a change the operator watched take effect and then lost. Every toggle now writes the declaration through `declare_agent` *first* and moves the live roster second; a failed write makes no live change either, because a UI showing a role the file does not have is worse than one that refused. An agent the roster knows but nothing declared — the migration seed — is declared here from what it reports, since a role assigned to something nobody wrote down has nowhere to persist to. Renaming and removing follow the same rule. **The v1 capability split is enforced in the UI**: the local host offers agent creation (`n`) and role editing; a remote host is operator-read-only, because its agents are declared on that machine. Orchestrator dispatch to a remote agent is untouched — this is only about affordances in this terminal. Shared, so the list and the binder cannot disagree about which address a `[[hosts]]` section will bind: `medulla::config::local_hosts` now owns the device-local host resolution the TUI binary derived privately, and `medulla::ui::hosts` owns the tree itself. Co-Authored-By: Claude <noreply@anthropic.com>
The rail concatenated two lists — lanes folded from task events, and the
operator's own harnesses under a `── your harnesses ──` divider — which is
exactly the split the agent/session redefinition removes. A task *is* an
agent session; they differ only by origin. And lanes come from traffic, so
an agent nobody had dispatched to had no row at all.
- A3: `RailRow::{Agent(Lane), Agent(Sub), NewHarness, HarnessSeparator,
Harness}` becomes `RailRow::{Host, Agent, Session, NewAgent, Lane}`.
Agents come from `[fleet].agentDeclarations`, so a declared agent with
zero sessions still renders; a lane nothing declares still gets a row.
Sessions are one row type under their agent whatever started them, and
the divider is gone. Host rows appear only once a remote host exists.
`Lane` is the residue for what is not an agent — the orchestrator's own
conversation, the functions divider, a `+N more` counter.
- A2: `+ New agent` replaces `+ New harness`. It reuses the harness picker
(now carrying a `PickerPurpose`) for harness type × workspace dir, then a
name prompt, and persists through `declare_agent`. Starting a session in
an undeclared directory offers the same flow inline.
- A4: `Ctrl-T` on a row that names an agent opens a session *of that agent*
— its declared harness in its declared workspace, named by the operator,
`SessionOrigin::User`, with the managed/unmanaged choice kept.
- A7: the orchestrator's conversation carries a "sessions started" block;
clicking an entry moves focus to that session and the rail follows.
`Ctrl-O` returns — scoped to "not already on the orchestrator" so the
chord keeps releasing the mouse where drag-select is actually wanted.
Session → agent resolves by the id the hub already filed the task under for
a dispatch, and by `harness × workspace` for an operator-started PTY. A
session in an undeclared directory stays listed rather than hidden.
Co-Authored-By: Claude <noreply@anthropic.com>
The Agents rail was written before the Hosts tab grew the shared `Host → Agent` projection, so it built its own host and agent levels out of declarations plus the event fold. Two derivations of one tree is two answers to "what exists", and the rail left a follow-up note saying so. The rail's host and agent levels now come from `medulla::ui::hosts::host_rows` — the same call the Hosts tab renders — with the folded lanes placed onto the agents it produces. A lane the tree does not know (a backend-side roster agent, a peer session) still keeps a row, so nothing that used to be visible disappears. Session rows are unchanged: they are the rail's own level, dispatched ones resolved by the roster id the hub filed them under and operator-started ones by harness × workspace. Progressive disclosure now reads off the projection: host headers appear once it holds more than one host, which is also when a registered remote peer first becomes a machine of its own rather than a bare lane. Two fixes fall out of making the projection load-bearing for both tabs: an agent declared with no host id is claimed by the machine looking at it (it was rendered by the rail and dropped by the Hosts tab), and a peer that reached the registry twice is one agent, not two. Co-Authored-By: Claude <noreply@anthropic.com>
Three things an operator hit on the Agents tab. The pane leaked the orchestrator's stream onto rows that are not it. `Selection::lane_index` defaulted to lane 0 — the orchestrator's — for every row with no lane of its own, so selecting `+ New agent`, a host header, or an agent nothing had been dispatched to showed the orchestrator thinking, attributed to a row that had not thought anything. The fallback is gone: `lane_index` is an `Option`, `lane()` answers `None`, and the new `summary` module renders what each of those rows actually is — the agent's identity, harness, workspace, roles and session count; the host's reach and agent count; what each action row will do. `+ New session` was unreachable. `open_new_session` shipped with the tree bound only to `^T`, so only an operator who already knew the chord could find it. `RailRow::NewSession` closes each agent's group, under its sessions, for the agents this machine declares — Enter and a click open the same named, user-owned flow. Spawned sessions were listed in one block for the whole conversation. They are attributed to the user turn that caused them — the event stream already carries both halves, a `User` opening a turn and every `TaskStart` until the next one belonging to it — and drawn under that turn. Sessions this stream cannot account for keep a trailing group rather than vanishing. The click-through is unchanged in kind: still keyed by task id through `focus_session_for_task`, but the hit map is now one slot per drawn row, since the entries are no longer contiguous. Co-Authored-By: Claude <noreply@anthropic.com>
sanil-23
force-pushed
the
split/2-tree-ui
branch
from
August 5, 2026 07:18
cd1c08f to
ccbd1b1
Compare
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Three tests stand a session up by pointing the codex bin at /bin/sh, which Windows has no equivalent of — CreateProcessW cannot find it, so they fail there while passing everywhere else. This module's sibling pty tests already carry the same guard for the same reason; these were written without it. The row model they exercise is portable; only this way of standing a session up is not. Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
src/tui/src/ui/app/hosts/tests.rs (1)
51-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
cursor_toto match what it does.The doc comment says "Move the cursor to the row
stepsbelow the top of the list", but the helper only presses Downstepstimes from the current position. It does not reset the cursor.the_cursor_walks_hosts_and_the_agents_under_themdepends on that accumulation: it calls the helper with 0, then 1, then 1 to reach row 2.Name it
down, assrc/tui/tests/feature_workers/helpers.rsdoes, so a later test does not read the current name as absolute positioning.♻️ Proposed rename
-/// Move the cursor to the row `steps` below the top of the list. -fn cursor_to(app: &mut App, steps: usize) { +/// Focus the Hosts page and press Down `steps` times from the current row. +fn down(app: &mut App, steps: usize) { app.focus_routing_subpage("Hosts");🤖 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/hosts/tests.rs` around lines 51 - 62, Rename the test helper function cursor_to to down, updating its doc comment and all call sites including the_cursor_walks_hosts_and_the_agents_under_them. Preserve its behavior of pressing Down the requested number of times from the current cursor position rather than implying absolute positioning.src/tui/tests/feature_workers/roles.rs (2)
138-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the last role and the key-press count from the catalog.
Line 149 hardcodes
repo-orchestratoras the last built-in role, and line 152 hardcodes 12 Down presses to reach it. Both hold only for the current catalog contents and order. Adding or reordering a built-in role breaks this test, and the failure will read as a scrolling regression rather than a catalog change.
src/tui/src/ui/app/hosts/tests.rsreads roles fromapp.agent_templates(). Do the same here: take the last template id and press Downtemplates.len() - 1times. The same applies toimplementeron line 132.🤖 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/tests/feature_workers/roles.rs` around lines 138 - 160, Update the role visibility tests around a_role_below_the_fold_stays_visible_when_selected and the implementer assertion to derive role IDs and navigation counts from app.agent_templates(). Use the catalog’s last template ID instead of hardcoding repo-orchestrator, and press Down templates.len() - 1 times so the tests remain valid when roles are added or reordered.
44-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis workaround suggests the toggle does not update the row when there is no config file.
Removing the role needs a second app with the role pre-set on the worker, rather than a second
Spaceon the same app.app_with_rostersupplies no config file, sopersist_agent_rolesreturns early atsrc/tui/src/ui/app/hosts/edit.rsline 104 without updatingself.loaded.config.fleet.agent_declarations. A second toggle on the first app would therefore read the same empty role list and send[role]again.The comment explains the whole-list replacement, but not why a fresh app is needed. State the reason, or assert the first app's row directly after the toggle so the behavior is pinned rather than avoided.
🤖 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/tests/feature_workers/roles.rs` around lines 44 - 67, Clarify this test around the second app created with app_with_roster: explain that the no-config setup causes persist_agent_roles to return before updating loaded.config.fleet.agent_declarations, so a second toggle on the original app would reuse the empty role list. Alternatively, assert the original app’s row state immediately after the first toggle to verify the update directly.src/tui/tests/feature_workers/list.rs (1)
278-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the no-op assertion for
e.
on_eventreturningNonedoes not prove thatedid nothing.hosts_e_opens_edit_label_prompt_prefilledshows thatereturnsNoneand still opens the edit prompt. With an empty roster this loop would pass even ifeopened a prompt for a host that has no roster entry.Add a
prompt_state()check after the loop.💚 Proposed assertion
for code in [ KeyCode::Enter, KeyCode::Char('d'), KeyCode::Char('e'), KeyCode::Char('r'), ] { assert!(app.on_event(key(code)).is_none()); } + assert!( + app.prompt_state().is_none(), + "no prompt opens for a host with no roster entry" + ); }🤖 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/tests/feature_workers/list.rs` around lines 278 - 287, Strengthen the empty-roster test around the event loop by checking app.prompt_state() after processing Enter, d, e, and r, and assert that no prompt is open. Keep the existing on_event(...).is_none() assertions, using the prompt-state assertion to ensure e does not open an edit prompt without a roster entry.src/tui/src/ui/app/hosts/mod.rs (1)
104-127: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider building the host tree once per frame.
host_treecallslocal_host_refsandself.runtime.workers()on every call.hosts_view,hosts_row_count,selected_host_row,selected_host_agent,hosts_cursor_on_host, andselected_hosteach call it again, andselected_hostalso callsruntime.workers()a second time on line 169. A single draw plus key dispatch therefore rebuilds the same tree several times and reclones the roster each time.If the roster grows, cache the tree for the frame (for example resolve
hosts_viewonce in the render and key paths and pass it down) instead of recomputing per accessor.🤖 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/hosts/mod.rs` around lines 104 - 127, Cache and reuse the host tree within each render and key-dispatch flow instead of rebuilding it through host_tree and the accessors hosts_view, hosts_row_count, selected_host_row, selected_host_agent, hosts_cursor_on_host, and selected_host. Resolve the tree once per frame/path, derive flattened rows and selection from that shared value, and avoid the additional runtime.workers() lookup in selected_host while preserving existing cursor clamping and selection behavior.src/tui/src/ui/app/rail/resolve.rs (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the unit tests to a sibling
resolve_tests.rs.
resolve.rsis a single-file leaf module. The repository convention places its unit tests in a siblingresolve_tests.rs, declared fromrail/mod.rswith#[cfg(test)] mod resolve_tests;. This file declares an inline#[cfg(test)] mod testsinstead.Based on learnings: "unit tests for a single-file Rust leaf module must be placed in a sibling
<module>_tests.rsfile until the module becomes a directory module".🤖 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/rail/resolve.rs` around lines 63 - 64, Move the inline #[cfg(test)] mod tests from resolve.rs into a sibling resolve_tests.rs file, preserving the existing test contents and imports. Declare the new test module from rail/mod.rs as #[cfg(test)] mod resolve_tests;, and remove the inline test module from resolve.rs.Source: Learnings
src/tui/src/ui/app/render/agents/rail/mod.rs (1)
337-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not fall back to lane 0 for a task-backed session row.
session.lane_index.unwrap_or(0)reintroduces the lane-0 fallback this PR removes elsewhere.Selection::lane_indexis now anOptionfor exactly this reason, andrender/agents/types.rsrecords that lane 0 is the orchestrator's. If a task-backed session row ever reaches here withlane_index == None,agent_row_linerenders it against the orchestrator's lane.
split_foldcurrently always setslane_indexon a dispatched session, so this is a latent path rather than a live defect. Render the row without a lane instead of borrowing lane 0, so the guard cannot decay if the assembly changes.🤖 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/rail/mod.rs` around lines 337 - 347, Remove the lane-0 fallback from the task-backed session branch in the RailRow rendering match. Update the AgentRow::Sub construction in agent_row_line to preserve session.lane_index as None when absent, preventing task rows from being rendered against the orchestrator lane.
🤖 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/sdk/src/config/local_hosts.rs`:
- Around line 35-53: Update local_hosts to validate uniqueness of resolved IDs
across the primary host and all extras before returning the collection; reject
or otherwise surface duplicates so callers cannot bind or render conflicting
hosts. Compare the final normalized addresses, including explicit
primary-address reuse and case-insensitive slug collisions such as “API” and
“api”, and add regression coverage for both scenarios.
In `@src/sdk/src/ui/hosts/mod.rs`:
- Around line 30-262: Move host projection behavior from host_rows, local_row,
has_probe_details, agent_from_declaration, agent_from_worker, and remote_label
into a dedicated hosts submodule such as projection.rs. Keep mod.rs limited to
module documentation, submodule declarations, and public re-exports so existing
callers retain the same accessible API.
- Around line 104-110: Update the remote-host handling around
HostRow.detail_worker so a later worker with probe details (capacity or
readiness data) replaces an earlier unprobed selection, while retaining the
existing worker when it already has probe details. Add a regression test
covering an unprobed first worker followed by a probed second worker and verify
the second worker is selected for remote-host details.
In `@src/tui/src/ui/app/agent_control.rs`:
- Around line 244-247: Update `declaration_for` to look up `agent_id` through
`local_agent_declarations` so only declarations matching the current host are
returned. Preserve the existing cloned `Option<AgentDeclaration>` result,
ensuring `open_new_session` rejects remotely declared agents accurately.
In `@src/tui/src/ui/app/hosts/edit.rs`:
- Around line 173-175: Update the early-return branch in the rename flow to emit
the same “this run only — no config file” narration used by persist_agent_roles
before returning. Preserve the existing return behavior and ensure the message
is shown when config_path is absent.
- Around line 85-101: Update the seeded declaration construction around
AgentDeclaration::new to reject agents whose agent.workspace is missing or
blank, mirroring the existing agent.harness validation. Set an explanatory
status using agent.agent_id and return false before creating the declaration;
only pass a non-empty workspace to AgentDeclaration::new.
- Around line 104-109: Update persist_agent_roles in
src/tui/src/ui/app/hosts/edit.rs:104-109 to apply the new agent declarations to
self.loaded.config.fleet.agent_declarations before returning when config_path is
None. In src/tui/tests/feature_workers/roles.rs:44-67, toggle roles twice on the
same app instance instead of creating a second app. In
src/tui/src/ui/app/hosts/tests.rs:111-155, add coverage for a declared agent
without config_path, asserting the “this run only” status and updated in-memory
roles.
In `@src/tui/src/ui/app/input/mouse.rs`:
- Around line 367-375: Update both action-row branches in the mouse input
handler—after open_new_agent_picker() and open_new_session(&agent_id)—to call
retarget_watch() before returning None. Ensure these no-watch-target actions
clear the existing self.watching subscription while preserving the current
picker/session opening behavior.
In `@src/tui/src/ui/app/keys/routing/mod.rs`:
- Around line 215-236: The remove_host_row flow must clear host_roles_focus
before removing the selected host row so subsequent arrow keys do not route
through host_roles_key for the changed row. Update remove_host_row to reset the
role focus before undeclare_selected_agent, and add a regression test covering
removal while role focus is active followed by an arrow-key event.
In `@src/tui/src/ui/app/rail/resolve.rs`:
- Around line 36-46: Update the launch metadata flow that constructs
`SessionRow` so `HarnessChoice::custom` preserves and stores its preset ID
rather than `base_harness` in `provider`. Then update `agent_for_session` to
resolve declarations using that preserved preset ID while retaining the existing
workspace-path matching and normal harness behavior.
In `@src/tui/src/ui/app/render/routing/hosts/preview.rs`:
- Around line 195-197: Update the role rendering flow around role_budget and the
role-line construction near the checkbox handling so zero available rows produce
no role rows, a one-row budget produces only the summary, and larger budgets
render the checkbox only when space remains. Ensure the returned role lines
never exceed role_budget and preserve the existing rendering behavior for
budgets above one.
In `@src/tui/src/ui/app/state.rs`:
- Around line 562-567: Update on_orchestrator_lane() handling of RailRow::Lane
to match AgentRow::Lane { lane_index } explicitly, preserving the
empty-lane-list fallback only for that variant; return false for wrapped
AgentRow::More and other non-lane rows. Add an assertion covering the
overflow-row case to ensure it is never classified as the orchestrator.
---
Nitpick comments:
In `@src/tui/src/ui/app/hosts/mod.rs`:
- Around line 104-127: Cache and reuse the host tree within each render and
key-dispatch flow instead of rebuilding it through host_tree and the accessors
hosts_view, hosts_row_count, selected_host_row, selected_host_agent,
hosts_cursor_on_host, and selected_host. Resolve the tree once per frame/path,
derive flattened rows and selection from that shared value, and avoid the
additional runtime.workers() lookup in selected_host while preserving existing
cursor clamping and selection behavior.
In `@src/tui/src/ui/app/hosts/tests.rs`:
- Around line 51-62: Rename the test helper function cursor_to to down, updating
its doc comment and all call sites including
the_cursor_walks_hosts_and_the_agents_under_them. Preserve its behavior of
pressing Down the requested number of times from the current cursor position
rather than implying absolute positioning.
In `@src/tui/src/ui/app/rail/resolve.rs`:
- Around line 63-64: Move the inline #[cfg(test)] mod tests from resolve.rs into
a sibling resolve_tests.rs file, preserving the existing test contents and
imports. Declare the new test module from rail/mod.rs as #[cfg(test)] mod
resolve_tests;, and remove the inline test module from resolve.rs.
In `@src/tui/src/ui/app/render/agents/rail/mod.rs`:
- Around line 337-347: Remove the lane-0 fallback from the task-backed session
branch in the RailRow rendering match. Update the AgentRow::Sub construction in
agent_row_line to preserve session.lane_index as None when absent, preventing
task rows from being rendered against the orchestrator lane.
In `@src/tui/tests/feature_workers/list.rs`:
- Around line 278-287: Strengthen the empty-roster test around the event loop by
checking app.prompt_state() after processing Enter, d, e, and r, and assert that
no prompt is open. Keep the existing on_event(...).is_none() assertions, using
the prompt-state assertion to ensure e does not open an edit prompt without a
roster entry.
In `@src/tui/tests/feature_workers/roles.rs`:
- Around line 138-160: Update the role visibility tests around
a_role_below_the_fold_stays_visible_when_selected and the implementer assertion
to derive role IDs and navigation counts from app.agent_templates(). Use the
catalog’s last template ID instead of hardcoding repo-orchestrator, and press
Down templates.len() - 1 times so the tests remain valid when roles are added or
reordered.
- Around line 44-67: Clarify this test around the second app created with
app_with_roster: explain that the no-config setup causes persist_agent_roles to
return before updating loaded.config.fleet.agent_declarations, so a second
toggle on the original app would reuse the empty role list. Alternatively,
assert the original app’s row state immediately after the first toggle to verify
the update directly.
🪄 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: 74fcae5e-d21f-475d-9049-ffb7ec45261d
📒 Files selected for processing (57)
src/sdk/src/config/local_hosts.rssrc/sdk/src/config/local_hosts_tests.rssrc/sdk/src/config/mod.rssrc/sdk/src/ui/hosts/mod.rssrc/sdk/src/ui/hosts/tests.rssrc/sdk/src/ui/hosts/types.rssrc/sdk/src/ui/mod.rssrc/tui/src/local_host/mod.rssrc/tui/src/ui/app/agent_control.rssrc/tui/src/ui/app/agent_control_tests.rssrc/tui/src/ui/app/commands/dispatch.rssrc/tui/src/ui/app/harness_control.rssrc/tui/src/ui/app/harness_workspace_tests.rssrc/tui/src/ui/app/hosts/edit.rssrc/tui/src/ui/app/hosts/mod.rssrc/tui/src/ui/app/hosts/tests.rssrc/tui/src/ui/app/input/mouse.rssrc/tui/src/ui/app/input/nav.rssrc/tui/src/ui/app/input/tests.rssrc/tui/src/ui/app/keys/agents.rssrc/tui/src/ui/app/keys/mod.rssrc/tui/src/ui/app/keys/routing/mod.rssrc/tui/src/ui/app/mod.rssrc/tui/src/ui/app/overlays_tests.rssrc/tui/src/ui/app/rail.rssrc/tui/src/ui/app/rail/mod.rssrc/tui/src/ui/app/rail/resolve.rssrc/tui/src/ui/app/rail/tests.rssrc/tui/src/ui/app/rail/types.rssrc/tui/src/ui/app/render/agents/mod.rssrc/tui/src/ui/app/render/agents/rail/mod.rssrc/tui/src/ui/app/render/agents/rail/status_line_tests.rssrc/tui/src/ui/app/render/agents/started.rssrc/tui/src/ui/app/render/agents/started_tests.rssrc/tui/src/ui/app/render/agents/summary.rssrc/tui/src/ui/app/render/agents/transcript.rssrc/tui/src/ui/app/render/agents/transcript_tests.rssrc/tui/src/ui/app/render/agents/types.rssrc/tui/src/ui/app/render/routing/hosts.rssrc/tui/src/ui/app/render/routing/hosts/format.rssrc/tui/src/ui/app/render/routing/hosts/list.rssrc/tui/src/ui/app/render/routing/hosts/mod.rssrc/tui/src/ui/app/render/routing/hosts/preview.rssrc/tui/src/ui/app/render/settings/help.rssrc/tui/src/ui/app/session_focus.rssrc/tui/src/ui/app/session_focus_tests.rssrc/tui/src/ui/app/state.rssrc/tui/src/ui/app/tests.rssrc/tui/src/ui/app/types.rssrc/tui/tests/feature_agent_lanes.rssrc/tui/tests/feature_agents_focus.rssrc/tui/tests/feature_app_more/views.rssrc/tui/tests/feature_harness_control.rssrc/tui/tests/feature_workers.rssrc/tui/tests/feature_workers/helpers.rssrc/tui/tests/feature_workers/list.rssrc/tui/tests/feature_workers/roles.rs
💤 Files with no reviewable changes (2)
- src/tui/src/ui/app/render/routing/hosts.rs
- src/tui/src/ui/app/rail.rs
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Review follow-ups on the Host → Agent → Session tree (#182). The two that change what an operator can do: - `^T` acted on any selected agent row, including one declared on another machine, so it started a *local* process for a remote agent — which the rail then listed as an orphan, because it resolves sessions against the local declarations. `declaration_for` now filters by host, and the refusal names the machine to open it on instead. - A session started from a custom preset was compared to declarations by the CLI underneath it (`claude`) rather than by the preset id the declaration records (`deepseek`), so every preset-backed session was filed under no agent. The preset id is carried through the launch and `SessionRow::harness_id` is what the rail matches on. The rest: - `on_orchestrator_lane` matches a lane's own row explicitly. `RailRow:: Lane` also wraps the overflow control and the `── functions ──` divider, and the divider names no lane at all — it fell through to the "no lanes yet ⇒ the orchestrator is all there is" answer and would have claimed the orchestrator's composer. - One rule for the no-config-file path: an edit applies for this run, in the declaration list as well as the roster, and says how long it lasts. Roles updated only the roster (so the row redrew with the old ones), a rename was silent, and undeclaring refused. A seed with no workspace is now refused like one with no harness — an agent is `harness × workspace`, and a declaration missing half of it is one no session can be opened from. - `local_hosts` returns unique addresses; two sections that resolve to one cannot both bind, so listing both drew a host that will not be there. - A remote host's detail row picks a *probed* entry rather than the first. - `mod.rs` keeps the wiring; the host projection moves to `projection.rs`. - Clicking either action row retargets the watch, so a click arriving from a task row stops that stream. - `d` on the Hosts page clears the role focus before reshaping the tree. - The role list never draws past the rows the pane gave it. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Collaborator
Author
|
@coderabbitai resolve |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/tui/src/ui/app/agent_control_tests.rs (2)
293-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the absence of a spawn against the session manager, not the rail.
Line 299 checks that no rail row carries a session id. That is a projection of the session list, not the session list. If
start_agent_sessionspawned a process that the rail failed to attribute to an agent, this assertion would still pass, and the test name promises the opposite. Assert on the harness manager's session count before and after the call.🤖 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/agent_control_tests.rs` around lines 293 - 301, Update the test around the refusal flow to assert that the harness session manager’s session count is unchanged before and after the call, rather than checking session IDs through app.rail_rows(). Capture the count before invoking the action and compare it afterward, preserving the existing status assertion.
269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the status assertion so it cannot pass on the agent id.
contains("studio")also matches the agent idstudio-codex. A refusal message that names only the agent, and never the host, still passes this test. The comment states the refusal must name the host that owns the agent. Assert on the full host phrase the refusal emits, or assert on a substring thatstudio-codexdoes not contain.🤖 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/agent_control_tests.rs` around lines 269 - 273, Tighten the status assertion in the agent control test so it checks for the host name itself rather than a substring that also matches the agent id. Update the assertion around app.status() to use the refusal’s full host phrase, or another host-specific substring that studio-codex cannot satisfy, while keeping the existing status flow and failure message context intact.src/tui/src/ui/app/rail/resolve.rs (1)
48-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTrim the session side of the harness comparison too.
Line 50 trims
declaration.harnessbut notharness.harness_id()rejects only a fully blank preset; it returns a padded preset id such as" deepseek"unchanged. That value then fails to match a declaration written asdeepseek, and the session lands in the orphan list — the exact failure this function was changed to fix. Trim both sides.♻️ Proposed fix
- let harness = row.harness_id(); + let harness = row.harness_id().trim();🤖 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/rail/resolve.rs` around lines 48 - 52, Update the harness comparison in the declaration lookup around harness_id() and declarations.iter().find() to trim the session-side harness value before applying case-insensitive equality. Preserve the existing declaration trimming and workspace path matching.
🤖 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/sdk/src/ui/hosts/projection.rs`:
- Around line 142-156: Update the deduplication check in the worker loop around
agents.push(agent_from_worker(...)) to compare worker.id and agent.agent_id
after trimming whitespace, matching the normalized comparison used when finding
declarations. Preserve the existing behavior for declared and undeclared worker
rows.
In `@src/tui/src/worker/pty/types.rs`:
- Around line 274-279: Update Session::harness_id in
src/tui/src/worker/pty/types.rs:274-279 to return the trimmed preset subslice
after filtering empty values, while preserving the provider fallback. No direct
change is needed in src/tui/src/ui/app/rail/resolve.rs:48-52; it is corrected by
the accessor fix.
---
Nitpick comments:
In `@src/tui/src/ui/app/agent_control_tests.rs`:
- Around line 293-301: Update the test around the refusal flow to assert that
the harness session manager’s session count is unchanged before and after the
call, rather than checking session IDs through app.rail_rows(). Capture the
count before invoking the action and compare it afterward, preserving the
existing status assertion.
- Around line 269-273: Tighten the status assertion in the agent control test so
it checks for the host name itself rather than a substring that also matches the
agent id. Update the assertion around app.status() to use the refusal’s full
host phrase, or another host-specific substring that studio-codex cannot
satisfy, while keeping the existing status flow and failure message context
intact.
In `@src/tui/src/ui/app/rail/resolve.rs`:
- Around line 48-52: Update the harness comparison in the declaration lookup
around harness_id() and declarations.iter().find() to trim the session-side
harness value before applying case-insensitive equality. Preserve the existing
declaration trimming and workspace path matching.
🪄 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: 53022e0b-b271-477d-95c0-1e2ddbeadaaa
📒 Files selected for processing (42)
src/sdk/src/config/local_hosts.rssrc/sdk/src/config/local_hosts_tests.rssrc/sdk/src/ui/hosts/mod.rssrc/sdk/src/ui/hosts/projection.rssrc/sdk/src/ui/hosts/tests.rssrc/tui/examples/pty_load.rssrc/tui/src/ui/app/agent_control.rssrc/tui/src/ui/app/agent_control_tests.rssrc/tui/src/ui/app/changes/baseline_tests.rssrc/tui/src/ui/app/commands/dispatch.rssrc/tui/src/ui/app/hosts/edit.rssrc/tui/src/ui/app/hosts/tests.rssrc/tui/src/ui/app/input/mouse.rssrc/tui/src/ui/app/input/tests.rssrc/tui/src/ui/app/keys/routing/mod.rssrc/tui/src/ui/app/mod.rssrc/tui/src/ui/app/rail/resolve.rssrc/tui/src/ui/app/rail/tests.rssrc/tui/src/ui/app/render/agents/rail/tests.rssrc/tui/src/ui/app/render/routing/hosts/preview.rssrc/tui/src/ui/app/render/settings/status_line.rssrc/tui/src/ui/app/state.rssrc/tui/src/ui/app/state_tests.rssrc/tui/src/ui/harness_pane/spawn.rssrc/tui/src/ui/harness_pane/tests/session.rssrc/tui/src/worker/app/tests/helpers/mod.rssrc/tui/src/worker/executor/run.rssrc/tui/src/worker/executor/types.rssrc/tui/src/worker/executor_tests/live.rssrc/tui/src/worker/executor_tests/sessions.rssrc/tui/src/worker/pty/handle/state.rssrc/tui/src/worker/pty/handle/types.rssrc/tui/src/worker/pty/manager/open.rssrc/tui/src/worker/pty/tests/mod.rssrc/tui/src/worker/pty/types.rssrc/tui/tests/e2e_local_harness_pane.rssrc/tui/tests/e2e_screen_stream.rssrc/tui/tests/feature_harness_control.rssrc/tui/tests/feature_harness_handoff.rssrc/tui/tests/feature_paste/attached.rssrc/tui/tests/feature_workers/helpers.rssrc/tui/tests/feature_workers/roles.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- src/tui/tests/feature_workers/helpers.rs
- src/tui/tests/feature_harness_control.rs
- src/tui/src/ui/app/commands/dispatch.rs
- src/sdk/src/config/local_hosts_tests.rs
- src/tui/src/ui/app/render/routing/hosts/preview.rs
- src/tui/src/ui/app/input/mouse.rs
- src/tui/src/ui/app/mod.rs
- src/tui/src/ui/app/keys/routing/mod.rs
- src/sdk/src/ui/hosts/tests.rs
- src/tui/src/ui/app/rail/tests.rs
- src/tui/tests/feature_workers/roles.rs
- src/tui/src/ui/app/state.rs
- src/tui/src/ui/app/hosts/edit.rs
Comment on lines
+142
to
+156
| let worker = workers | ||
| .iter() | ||
| .find(|worker| worker.id.trim() == declaration.agent_id.trim()); | ||
| agents.push(agent_from_declaration(declaration, worker)); | ||
| } | ||
| let mut detail_worker = None; | ||
| for worker in workers.iter().filter(|worker| worker.address.trim() == id) { | ||
| if detail_worker.is_none() && has_probe_details(worker) { | ||
| detail_worker = Some(worker.id.clone()); | ||
| } | ||
| if agents.iter().any(|agent| agent.agent_id == worker.id) { | ||
| continue; | ||
| } | ||
| agents.push(agent_from_worker(worker, true)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim agent_id consistently, or a spaced declaration duplicates its agent row.
Line 144 matches a declaration to a worker with trimmed ids. Line 152 dedupes with untrimmed ids. If a declaration's agent_id carries surrounding whitespace, the declared row is created with live = true, but line 152 does not recognize the same worker, so the worker is appended a second time as an undeclared row. That is the duplicate listing the comment at lines 100-102 exists to prevent.
♻️ Proposed fix
- if agents.iter().any(|agent| agent.agent_id == worker.id) {
+ if agents
+ .iter()
+ .any(|agent| agent.agent_id.trim() == worker.id.trim())
+ {
continue;
}📝 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
| let worker = workers | |
| .iter() | |
| .find(|worker| worker.id.trim() == declaration.agent_id.trim()); | |
| agents.push(agent_from_declaration(declaration, worker)); | |
| } | |
| let mut detail_worker = None; | |
| for worker in workers.iter().filter(|worker| worker.address.trim() == id) { | |
| if detail_worker.is_none() && has_probe_details(worker) { | |
| detail_worker = Some(worker.id.clone()); | |
| } | |
| if agents.iter().any(|agent| agent.agent_id == worker.id) { | |
| continue; | |
| } | |
| agents.push(agent_from_worker(worker, true)); | |
| } | |
| let worker = workers | |
| .iter() | |
| .find(|worker| worker.id.trim() == declaration.agent_id.trim()); | |
| agents.push(agent_from_declaration(declaration, worker)); | |
| } | |
| let mut detail_worker = None; | |
| for worker in workers.iter().filter(|worker| worker.address.trim() == id) { | |
| if detail_worker.is_none() && has_probe_details(worker) { | |
| detail_worker = Some(worker.id.clone()); | |
| } | |
| if agents | |
| .iter() | |
| .any(|agent| agent.agent_id.trim() == worker.id.trim()) | |
| { | |
| continue; | |
| } | |
| agents.push(agent_from_worker(worker, true)); | |
| } |
🤖 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/ui/hosts/projection.rs` around lines 142 - 156, Update the
deduplication check in the worker loop around
agents.push(agent_from_worker(...)) to compare worker.id and agent.agent_id
after trimming whitespace, matching the normalized comparison used when finding
declarations. Preserve the existing behavior for declared and undeclared worker
rows.
Comment on lines
+274
to
+279
| pub fn harness_id(&self) -> &str { | ||
| self.preset | ||
| .as_deref() | ||
| .filter(|preset| !preset.trim().is_empty()) | ||
| .unwrap_or_else(|| self.provider.as_str()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
harness_id() returns an untrimmed preset id, so every caller must trim defensively. Line 277 filters on preset.trim() but line 275 returns the original preset slice. A preset id written with surrounding whitespace passes the blank check and is returned with its padding. agent_for_session then compares that padded value against a trimmed declaration harness, finds no match, and files the session in the orphan list — the failure this change set out to fix.
src/tui/src/worker/pty/types.rs#L274-L279: return the trimmed slice, for exampleself.preset.as_deref().map(str::trim).filter(|preset| !preset.is_empty()).unwrap_or_else(|| self.provider.as_str()).str::trimreturns a subslice with the same lifetime, so the&strreturn type still holds.src/tui/src/ui/app/rail/resolve.rs#L48-L52: no change is needed once the accessor is fixed. If you prefer to leave the accessor as is, trim at line 48 instead:let harness = row.harness_id().trim();.
📍 Affects 2 files
src/tui/src/worker/pty/types.rs#L274-L279(this comment)src/tui/src/ui/app/rail/resolve.rs#L48-L52
🤖 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/pty/types.rs` around lines 274 - 279, Update
Session::harness_id in src/tui/src/worker/pty/types.rs:274-279 to return the
trimmed preset subslice after filtering empty values, while preserving the
provider fallback. No direct change is needed in
src/tui/src/ui/app/rail/resolve.rs:48-52; it is corrected by the accessor fix.
sanil-23
added a commit
that referenced
this pull request
Aug 5, 2026
Review follow-ups on the Host → Agent → Session tree (#182). The two that change what an operator can do: - `^T` acted on any selected agent row, including one declared on another machine, so it started a *local* process for a remote agent — which the rail then listed as an orphan, because it resolves sessions against the local declarations. `declaration_for` now filters by host, and the refusal names the machine to open it on instead. - A session started from a custom preset was compared to declarations by the CLI underneath it (`claude`) rather than by the preset id the declaration records (`deepseek`), so every preset-backed session was filed under no agent. The preset id is carried through the launch and `SessionRow::harness_id` is what the rail matches on. The rest: - `on_orchestrator_lane` matches a lane's own row explicitly. `RailRow:: Lane` also wraps the overflow control and the `── functions ──` divider, and the divider names no lane at all — it fell through to the "no lanes yet ⇒ the orchestrator is all there is" answer and would have claimed the orchestrator's composer. - One rule for the no-config-file path: an edit applies for this run, in the declaration list as well as the roster, and says how long it lasts. Roles updated only the roster (so the row redrew with the old ones), a rename was silent, and undeclaring refused. A seed with no workspace is now refused like one with no harness — an agent is `harness × workspace`, and a declaration missing half of it is one no session can be opened from. - `local_hosts` returns unique addresses; two sections that resolve to one cannot both bind, so listing both drew a host that will not be there. - A remote host's detail row picks a *probed* entry rather than the first. - `mod.rs` keeps the wiring; the host projection moves to `projection.rs`. - Clicking either action row retargets the watch, so a click arriving from a task row stops that stream. - `d` on the Hosts page clears the role focus before reshaping the tree. - The role list never draws past the rows the pane gave it. Co-Authored-By: Claude <noreply@anthropic.com>
sanil-23
added a commit
that referenced
this pull request
Aug 5, 2026
… and control (#189) * feat(hosts): render the Hosts tab as Host → Agents with persisted roles The page rendered `Runtime::workers()` flat and called each row a host — its own comment admitted the collapse. That was true only while a machine advertised one worker; a machine now declares one agent per `harness × workspace`, so the list was agents with the host level taken out of it, and a fleet you cannot see the shape of is one you cannot manage. The tab is now the topology the advert is a projection of (spec §2.4): - **The hosts this machine runs come first, always.** They are resolved from config, not from what started, so a host that is declared but idle is still listed — that is the state where the operator most needs to see it. A running primary overrides its identity from the live observation, since `[host].workspace` is usually blank and only it has resolved that. - **Each host carries its agents**, from two sources that are deliberately not merged: declarations for a local host (an agent exists because it is written down), and the roster for a remote one — because the host link does not exchange declared agent lists yet (plan §D1). A remote host says so on its own row and again in its preview rather than passing this hub's roster off as that machine's declarations. - **The cursor walks both levels.** A host row previews the machine — capacity, readiness, budgets, read from whichever entry probed it — and an agent row previews the thing a dispatch targets, with the role toggles. **Roles are assigned per agent and persisted.** `set_roles` moves the roster this process holds, and the roster is rebuilt from the declarations at every launch — so assigning a role that way was a change the operator watched take effect and then lost. Every toggle now writes the declaration through `declare_agent` *first* and moves the live roster second; a failed write makes no live change either, because a UI showing a role the file does not have is worse than one that refused. An agent the roster knows but nothing declared — the migration seed — is declared here from what it reports, since a role assigned to something nobody wrote down has nowhere to persist to. Renaming and removing follow the same rule. **The v1 capability split is enforced in the UI**: the local host offers agent creation (`n`) and role editing; a remote host is operator-read-only, because its agents are declared on that machine. Orchestrator dispatch to a remote agent is untouched — this is only about affordances in this terminal. Shared, so the list and the binder cannot disagree about which address a `[[hosts]]` section will bind: `medulla::config::local_hosts` now owns the device-local host resolution the TUI binary derived privately, and `medulla::ui::hosts` owns the tree itself. Co-Authored-By: Claude <noreply@anthropic.com> * feat(ui): make the Agents tab the Host → Agent → Session tree The rail concatenated two lists — lanes folded from task events, and the operator's own harnesses under a `── your harnesses ──` divider — which is exactly the split the agent/session redefinition removes. A task *is* an agent session; they differ only by origin. And lanes come from traffic, so an agent nobody had dispatched to had no row at all. - A3: `RailRow::{Agent(Lane), Agent(Sub), NewHarness, HarnessSeparator, Harness}` becomes `RailRow::{Host, Agent, Session, NewAgent, Lane}`. Agents come from `[fleet].agentDeclarations`, so a declared agent with zero sessions still renders; a lane nothing declares still gets a row. Sessions are one row type under their agent whatever started them, and the divider is gone. Host rows appear only once a remote host exists. `Lane` is the residue for what is not an agent — the orchestrator's own conversation, the functions divider, a `+N more` counter. - A2: `+ New agent` replaces `+ New harness`. It reuses the harness picker (now carrying a `PickerPurpose`) for harness type × workspace dir, then a name prompt, and persists through `declare_agent`. Starting a session in an undeclared directory offers the same flow inline. - A4: `Ctrl-T` on a row that names an agent opens a session *of that agent* — its declared harness in its declared workspace, named by the operator, `SessionOrigin::User`, with the managed/unmanaged choice kept. - A7: the orchestrator's conversation carries a "sessions started" block; clicking an entry moves focus to that session and the rail follows. `Ctrl-O` returns — scoped to "not already on the orchestrator" so the chord keeps releasing the mouse where drag-select is actually wanted. Session → agent resolves by the id the hub already filed the task under for a dispatch, and by `harness × workspace` for an operator-started PTY. A session in an undeclared directory stays listed rather than hidden. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(ui): source both tabs' Host → Agent tree from one projection The Agents rail was written before the Hosts tab grew the shared `Host → Agent` projection, so it built its own host and agent levels out of declarations plus the event fold. Two derivations of one tree is two answers to "what exists", and the rail left a follow-up note saying so. The rail's host and agent levels now come from `medulla::ui::hosts::host_rows` — the same call the Hosts tab renders — with the folded lanes placed onto the agents it produces. A lane the tree does not know (a backend-side roster agent, a peer session) still keeps a row, so nothing that used to be visible disappears. Session rows are unchanged: they are the rail's own level, dispatched ones resolved by the roster id the hub filed them under and operator-started ones by harness × workspace. Progressive disclosure now reads off the projection: host headers appear once it holds more than one host, which is also when a registered remote peer first becomes a machine of its own rather than a bare lane. Two fixes fall out of making the projection load-bearing for both tabs: an agent declared with no host id is claimed by the machine looking at it (it was rendered by the rail and dropped by the Hosts tab), and a peer that reached the registry twice is one agent, not two. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ui): a rail row shows its own content, offers its own session Three things an operator hit on the Agents tab. The pane leaked the orchestrator's stream onto rows that are not it. `Selection::lane_index` defaulted to lane 0 — the orchestrator's — for every row with no lane of its own, so selecting `+ New agent`, a host header, or an agent nothing had been dispatched to showed the orchestrator thinking, attributed to a row that had not thought anything. The fallback is gone: `lane_index` is an `Option`, `lane()` answers `None`, and the new `summary` module renders what each of those rows actually is — the agent's identity, harness, workspace, roles and session count; the host's reach and agent count; what each action row will do. `+ New session` was unreachable. `open_new_session` shipped with the tree bound only to `^T`, so only an operator who already knew the chord could find it. `RailRow::NewSession` closes each agent's group, under its sessions, for the agents this machine declares — Enter and a click open the same named, user-owned flow. Spawned sessions were listed in one block for the whole conversation. They are attributed to the user turn that caused them — the event stream already carries both halves, a `User` opening a turn and every `TaskStart` until the next one belonging to it — and drawn under that turn. Sessions this stream cannot account for keep a trailing group rather than vanishing. The click-through is unchanged in kind: still keyed by task id through `focus_session_for_task`, but the hit map is now one slot per drawn row, since the entries are no longer contiguous. Co-Authored-By: Claude <noreply@anthropic.com> * test(ui): gate the pty-backed rail tests to unix Three tests stand a session up by pointing the codex bin at /bin/sh, which Windows has no equivalent of — CreateProcessW cannot find it, so they fail there while passing everywhere else. This module's sibling pty tests already carry the same guard for the same reason; these were written without it. The row model they exercise is portable; only this way of standing a session up is not. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ui): scope a session to the host that declares its agent Review follow-ups on the Host → Agent → Session tree (#182). The two that change what an operator can do: - `^T` acted on any selected agent row, including one declared on another machine, so it started a *local* process for a remote agent — which the rail then listed as an orphan, because it resolves sessions against the local declarations. `declaration_for` now filters by host, and the refusal names the machine to open it on instead. - A session started from a custom preset was compared to declarations by the CLI underneath it (`claude`) rather than by the preset id the declaration records (`deepseek`), so every preset-backed session was filed under no agent. The preset id is carried through the launch and `SessionRow::harness_id` is what the rail matches on. The rest: - `on_orchestrator_lane` matches a lane's own row explicitly. `RailRow:: Lane` also wraps the overflow control and the `── functions ──` divider, and the divider names no lane at all — it fell through to the "no lanes yet ⇒ the orchestrator is all there is" answer and would have claimed the orchestrator's composer. - One rule for the no-config-file path: an edit applies for this run, in the declaration list as well as the roster, and says how long it lasts. Roles updated only the roster (so the row redrew with the old ones), a rename was silent, and undeclaring refused. A seed with no workspace is now refused like one with no harness — an agent is `harness × workspace`, and a declaration missing half of it is one no session can be opened from. - `local_hosts` returns unique addresses; two sections that resolve to one cannot both bind, so listing both drew a host that will not be there. - A remote host's detail row picks a *probed* entry rather than the first. - `mod.rs` keeps the wiring; the host projection moves to `projection.rs`. - Clicking either action row retargets the watch, so a click arriving from a task row stops that stream. - `d` on the Hosts page clears the role focus before reshaping the tree. - The role list never draws past the rows the pane gave it. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(ui): retire "harness" as a noun for anything you interact with Plan §A6/F1: a harness is a *type* — the value on an agent that says which coding CLI its sessions run — not an entity. Everything the operator touches is an agent or one of its sessions, and the old vocabulary was the route by which the host ≡ worker ≡ agent collapse kept leaking back in. Types and fields: `HarnessControl` → `SessionControl` (control is per-session), `LocalHarnesses` → `LocalSessions`, `HarnessPicker` → `AgentPicker`, `App.harnesses` → `local_sessions`, `harness_pane_session` → `pane_session`, `selected_harness_session` → `rail_session`, `own_harness_lines` → `own_session_lines`, `harnesses_waiting` → `sessions_waiting`, `Cmd::{HoldHarness,HandOffHarness}` → `{HoldSession,HandOffSession}`, `BaselineSource::HarnessLaunch` → `SessionLaunch`, and the take/hand-back/toggle/release entry points with them. `ui/app/harness_control.rs` and `render/harness_modals.rs` follow their contents to `session_control.rs` / `session_modals.rs`, as do four integration-test files. Copy: every status line, prompt, modal, pane title, help line and toast that called a session a harness now calls it a session — including the hand-back and take-control questions, the kill confirmation, and the key hint bar. The `/harness` command is now `/session` (old spelling kept as an alias, with a test pinning it), and Routing's `Harnesses` page is `Harness Types`. The rail title counts the agents on the tree instead of the lanes traffic happened to fold, so a quiet machine with three declared agents no longer reads "Agents · 0". Kept as "harness" where it genuinely means the type or the CLI process: `HarnessProvider`/`HarnessChoice`, the `harness` field on a declaration, `[harness]` config, custom presets, `harness_pty` and the pane internals that talk to the child, and every wire name (`HarnessControlFrame`, `HarnessHandoff`, `harness_select`) — hub and protocol are untouched beyond two stale doc cross-references. Docs: TERMINOLOGY.md now defines Agent as a declaration, Harness as a type and not a level, Host → Agent → Session as the chain, and Session as the runtime instance with origin and owner; README and workflows.md follow. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ui): read takeover from the attachment that is happening Review follow-ups on the vocabulary pass (#183). `took_control_by_attach` was only ever set to `true`, and nothing clears it on release — so a `true` left by an attachment that really did take a session survived into the next one. If that next session was already the operator's, a pty write failure read the stale flag and handed it to the orchestrator, taking away a session they had been holding before they focused in. It is now assigned from `took` on every attachment. `SessionControl`'s doc defined "unmanaged" as a session *born* user-held. It is not a kind of session: `claim_idle` reads the current holder, so a dispatched session the operator takes is unmanaged for as long as they keep it. The doc now describes current control and leaves origin to `SessionOrigin`. And the containment diagram in `TERMINOLOGY.md` gets a language on its fence, for MD040. Co-Authored-By: Claude <noreply@anthropic.com> * feat(hub): advertise real host topology and report the session that served a task register_agents gains a hosts[] block (one entry per host this hub fronts, kind derived from the locally declared hosts, address/name/resources only where real values exist) and each agent now carries its own hostId plus maxSessions. Task results report the sessionId that served them, which the backend's ManagerTaskEntry.agentSessionId already expects. metadata.workspace deliberately stays a bare path string: the backend parses both shapes, so {path,type} buys nothing today and ships with phase D. Control and handoff keys are byte-identical, pinned by assertions — the backend's control folds depend on them exactly as they are. Co-Authored-By: Claude <noreply@anthropic.com> * fix(hub): stop advertising hostId on workspace-backed agents The library reserves AgentDescriptor.hostId for a LOCAL agent — 'only meaningful when workspaceId is absent' and 'must NEVER be set on a harness-backed agent', whose host is derived by walking up from its workspace. Setting it on every agent made the server take its 'a supplied workspaceId or hostId always wins' early return and skip synthesizing a workspaceId from metadata.workspace, orphaning every agent from the agent -> workspace -> harness -> host chain: host_list still rendered the topology, but placement answered 'no agent inside <host> is available (none declared there)' and no task could be dispatched. The host reaches the wire once, in the hosts[] block. An agent with no workspace keeps hostId — it has nothing to walk up from, and that is the case the field exists for. Co-Authored-By: Claude <noreply@anthropic.com> * 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. 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> * perf(daemon): lift the host-wide task cap so work stops queueing for 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> * fix(ui): trim join keys consistently, keep mod.rs to wiring, and collapse the hand-back instruction - projection.rs deduped undeclared workers with raw ids while the loop above claimed them with trimmed ones, so a padded agent_id listed the same worker twice — the duplicate that loop exists to prevent. - SessionRow::harness_id() is a join key; it now trims once at the source rather than leaving every caller to trim defensively. - render/agents/mod.rs and render/mod.rs held behaviour; moved to child modules, per the repo's mod.rs rule. - the hand-back prompt is one line with text after {instruction}, so a newline in it dropped the 'do not redo it' tail entirely — collapsed now. - covers the render-driven remote-session lifecycle. Co-Authored-By: Claude <noreply@anthropic.com> * test(ui): assert the hand-back question in the vocabulary it now speaks #187's pointer tests were written against the pre-sweep prompt, which said "You still have this harness". The sweep renamed the thing the operator holds, so the question now reads "...this session" and four of these assertions were looking for a string nothing prints. One of them was worse than a failure: the same literal appears under a `!`, so the test asserting that clicking a session's *own* row asks nothing was passing because the text it searched for could never be found — it would have passed just as well if the question had been asked every time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 2/5 — base
split/1-agent-model. Depends on: 1/5.split/1-agent-modelmainsplit/2-tree-uisplit/1-agent-modelHost → Agent → Sessiontreesplit/3-vocabularysplit/2-tree-uisplit/4-wiresplit/3-vocabularyhosts[], per-agenthostId/maxSessions, resultsessionIdsplit/5-controlsplit/4-wireEach PR's diff shows only its own layer.
split/5-control's tree is byte-identical tobcc61e63, the merge commit on #180.Second of five stacked PRs replacing #180. The diff shown here is only this layer;
review 1/5 first.
What this layer does
The UI: both tabs become the
Host → Agent → Sessiontree, sourced from one projection.feat(hosts): render the Hosts tab as Host → Agents with persisted rolesThe page rendered
Runtime::workers()flat and called each row a host. It is now thetopology the advert is a projection of: hosts this machine runs come first (resolved from
config, so a declared-but-idle host is still listed), each carrying its agents — from
declarations for a local host, from the roster for a remote one, deliberately not merged.
Role toggles write the declaration through
declare_agentfirst and move the liveroster second.
feat(ui): make the Agents tab the Host → Agent → Session treeRailRow::{Agent(Lane), Agent(Sub), NewHarness, HarnessSeparator, Harness}becomesRailRow::{Host, Agent, Session, NewAgent, Lane}. A declared agent with zero sessionsnow renders; the
── your harnesses ──divider is gone;+ New agentreplaces+ New harness;Ctrl-Topens a session of that agent.refactor(ui): source both tabs' Host → Agent tree from one projectionThe rail's host and agent levels now come from
medulla::ui::hosts::host_rows— the samecall the Hosts tab renders — with folded lanes placed onto the agents it produces.
fix(ui): a rail row shows its own content, offers its own sessionThree operator-visible bugs: the pane leaked the orchestrator's stream onto rows that are
not it (
Selection::lane_indexdefaulted to lane 0; it is now anOptionand a newsummarymodule renders what each row actually is);+ New sessionwas unreachable(bound only to
^T, now aRailRow::NewSessionclosing each agent's group); spawnedsessions are now attributed to the user turn that caused them.
Reordering note
In the original branch this fourth commit (
fix(ui): a rail row shows its own content)landed after the vocabulary rename and the control work. It has been moved up to sit
with the rest of the tree UI, since that is the layer it belongs to. It rebased with a
single one-line conflict (
self.harnesseshad not yet been renamed toself.local_sessionsat this point in the stack) and the stack tip is unchanged.What a reviewer should look at closely
lane_indexis nowOption. The removed lane-0 fallback is the actual fix for theleaked-transcript bug; any place that re-introduces a default would bring it back.
A remote host says so on its own row rather than passing this hub's roster off as that
machine's declarations — the host link does not exchange declared agent lists yet.
write makes no live change, because a UI showing a role the file does not have is worse
than one that refused.
#171), not the rail's.push_groupnolonger imposes its own cap; it records that the fold owes an overflow row. A second cap
here would clip the page the operator just asked to see.
peer session. Nothing that used to be visible disappears.
Validation
Result on this branch:
fmt,clippy -D warnings,check, andbuildall clean.testreports 3879 passed, 5 failed, 14 ignored on macOS. All five failures arepre-existing on
mainand unrelated to this stack:daemon::providers::acp::tests::execution::a_new_acp_session_is_reported_before_the_task_completesdaemon::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_whitespaceworker::pty::tests::session::a_session_snapshots_head_before_the_harness_can_commitworker::pty::tests::session::an_unborn_repository_records_its_root_without_a_launch_commit(the last three are the macOS
/varvs/private/varsymlink)Coverage was deliberately not run locally; CI owns that gate.
Summary by CodeRabbit