diff --git a/src/tui/src/app_loop.rs b/src/tui/src/app_loop.rs index f443cc36d..f82d350d4 100644 --- a/src/tui/src/app_loop.rs +++ b/src/tui/src/app_loop.rs @@ -549,9 +549,9 @@ pub(crate) async fn run_tui(raw: &[String]) -> anyhow::Result<()> { let declared_local_hosts = std::sync::Arc::new(std::sync::Mutex::new( crate::local_host::all_local_hosts(&loaded.config.host, &loaded.config.hosts), )); - // Only meaningful while this device hosts: with hosting off there is no bus - // binding or session manager to hand a new host. - let local_host_spawner = hosting + // Only meaningful while this device hosts: with hosting off there are no + // host options to read declared harnesses from. + let local_host_harnesses = hosting .then(|| { crate::local_host::options_from_config_with_custom_and_hooks( &loaded.config.host, @@ -566,18 +566,7 @@ pub(crate) async fn run_tui(raw: &[String]) -> anyhow::Result<()> { }, ) .ok() - .map(|options| { - crate::local_host::LocalHostSpawner::new( - local_network.clone(), - harness_sessions.clone(), - options, - env.clone(), - host_runtimes.clone(), - started_hosts.clone(), - declared_local_hosts.clone(), - loaded.config.fleet.agent_declarations.clone(), - ) - }) + .map(crate::local_host::LocalHostHarnesses::new) }) .flatten(); let local_dispatch = crate::hub_relay::LocalDispatch { @@ -665,7 +654,7 @@ pub(crate) async fn run_tui(raw: &[String]) -> anyhow::Result<()> { &mut terminal, runtime.clone(), SessionWiring { - local_hosts: local_host_spawner.clone(), + local_hosts: local_host_harnesses.clone(), loaded: loaded.clone(), startup_status: status.take(), link_obs: link_obs.clone(), diff --git a/src/tui/src/event_loop/cmd_dispatch/mod.rs b/src/tui/src/event_loop/cmd_dispatch/mod.rs index 40b5253d9..0aa147caf 100644 --- a/src/tui/src/event_loop/cmd_dispatch/mod.rs +++ b/src/tui/src/event_loop/cmd_dispatch/mod.rs @@ -68,7 +68,7 @@ pub(super) fn run_cmd( runtime: &Arc, _workflows_config: &medulla::config::WorkflowsConfig, msg_tx: &tokio::sync::mpsc::UnboundedSender, - local_hosts: Option<&crate::local_host::LocalHostSpawner>, + local_hosts: Option<&crate::local_host::LocalHostHarnesses>, ) { let cmd = match feedback::run_feedback_cmd(cmd, runtime, msg_tx) { Some(cmd) => *cmd, @@ -191,70 +191,44 @@ pub(super) fn run_cmd( let _ = tx.send(AppMsg::Status(status)); }); } - Cmd::StartLocalHost { host, index } => { - let Some(spawner) = local_hosts.cloned() else { - let _ = msg_tx.send(AppMsg::Status( - "This device is not hosting, so a local host cannot start here".to_string(), - )); - return; - }; + Cmd::WorkerOp(op) => { let rt = runtime.clone(); let tx = msg_tx.clone(); tokio::spawn(async move { - // Start it first, register it second. A roster entry whose - // address nothing answers on is the failure this whole feature - // exists to avoid — the orchestrator would dispatch to it and - // the task would vanish. - let specs = match spawner.spawn(&host, index) { - Ok(specs) => specs, - Err(error) => { - let _ = - tx.send(AppMsg::Status(format!("Local host did not start: {error}"))); - return; - } - }; - // The host's first declared agent. The registry op below is - // keyed by address and replaces any entry sharing one, so - // registering the siblings here would leave exactly one anyway — - // a host added mid-run advertises its default agent until the - // add path is agent-keyed rather than address-keyed. Every agent - // is advertised on the next launch, where the roster is built - // from the declarations directly. - let Some(spec) = specs.into_iter().next() else { - let _ = tx.send(AppMsg::Status( - "Local host started, but declares no agent".to_string(), - )); - return; - }; - let workspace = spec - .workspace - .as_ref() - .map(|workspace| workspace.path.clone()) - .unwrap_or_default(); - // Registered through the same op a remote add uses, so both - // kinds reach the roster by one path. - let status = match rt - .worker_op(medulla::runtime::WorkerOp::Add { - address: Some(spec.address.clone()), - handle: None, - label: Some(spec.name.clone()), - harness: Some(spec.harness.clone()), - }) - .await - { - Ok(()) => format!("Local host running · {workspace}"), - Err(e) => format!("Started, but not registered: {e}"), + let status = match rt.worker_op(op).await { + Ok(()) => "Worker registry updated".to_string(), + Err(e) => e.to_string(), }; let _ = tx.send(AppMsg::Status(status)); }); } - Cmd::WorkerOp(op) => { + Cmd::WorkerOps(ops) => { let rt = runtime.clone(); let tx = msg_tx.clone(); tokio::spawn(async move { - let status = match rt.worker_op(op).await { - Ok(()) => "Worker registry updated".to_string(), - Err(e) => e.to_string(), + let total = ops.len(); + let mut applied = 0usize; + let mut failure = None; + for op in ops { + // Stop at the first failure rather than pressing on. The + // ops are one operator action, and continuing past a + // refusal would half-remove a host — some agents gone, the + // rest still routed to — which is the state hardest to + // reason about from the screen. + match rt.worker_op(op).await { + Ok(()) => applied += 1, + Err(e) => { + failure = Some(e.to_string()); + break; + } + } + } + let status = match failure { + None => "Worker registry updated".to_string(), + Some(e) if applied == 0 => e, + // Says what landed as well as what stopped it: the operator + // is looking at a list that is now partly changed. + Some(e) => format!("Removed {applied} of {total}, then: {e}"), }; let _ = tx.send(AppMsg::Status(status)); }); diff --git a/src/tui/src/event_loop/cmd_dispatch/workflows.rs b/src/tui/src/event_loop/cmd_dispatch/workflows.rs index 7c4b766e3..7d068148d 100644 --- a/src/tui/src/event_loop/cmd_dispatch/workflows.rs +++ b/src/tui/src/event_loop/cmd_dispatch/workflows.rs @@ -88,7 +88,7 @@ async fn run( model: (!workflows_config.default_model.is_empty()) .then(|| workflows_config.default_model.clone()), // The same presets this session's primary host advertises (see - // `LocalHostSpawner::custom_harnesses`), so an `agent` step naming a + // `LocalHostHarnesses::custom_harnesses`), so an `agent` step naming a // custom harness preset does not fail with "not configured on this // host" purely because this one-shot daemon started with none. custom_harnesses: custom_harnesses.to_vec(), diff --git a/src/tui/src/event_loop/types.rs b/src/tui/src/event_loop/types.rs index e568e4690..3e7449f1a 100644 --- a/src/tui/src/event_loop/types.rs +++ b/src/tui/src/event_loop/types.rs @@ -106,10 +106,10 @@ pub(crate) enum SessionExit { pub(crate) struct SessionWiring { /// The loaded configuration for this session. pub loaded: medulla::config::LoadedConfig, - /// Starts a host on this device after launch. `None` when this device is - /// not hosting — there is then no bus binding or session manager to hand a - /// new host, and the command says so rather than half-starting one. - pub local_hosts: Option, + /// The custom harnesses this device's primary host declares, for a + /// workflow `agent` step to resolve a harness name against. `None` when this + /// device is not hosting — there are then no host options to read them from. + pub local_hosts: Option, /// A note to show on the status line at startup, if any. pub startup_status: Option, /// The tiny.place presence observation, when that service is running. diff --git a/src/tui/src/hub_relay/tests.rs b/src/tui/src/hub_relay/tests.rs index 767d9469d..1ec7be35b 100644 --- a/src/tui/src/hub_relay/tests.rs +++ b/src/tui/src/hub_relay/tests.rs @@ -355,8 +355,8 @@ fn a_roster_remembered_from_a_hosting_run_is_dropped_when_hosting_is_off() { #[test] fn a_host_added_after_launch_is_not_remembered_as_a_remote_peer() { // The sink filters at *save* time, so a launch-time snapshot of the local - // addresses did not know about a host started mid-session through - // `LocalHostSpawner`. Its device-local entry was written into the saved + // addresses did not know about a host that joined the local list after that + // snapshot was taken. Its device-local entry was written into the saved // roster and would be advertised on a later run at an address nothing binds. let dir = tempfile::tempdir().expect("tempdir"); let home = dir.path(); diff --git a/src/tui/src/local_host/mod.rs b/src/tui/src/local_host/mod.rs index 5de9e92aa..1149c8429 100644 --- a/src/tui/src/local_host/mod.rs +++ b/src/tui/src/local_host/mod.rs @@ -471,117 +471,25 @@ pub(crate) fn start( .map(Some) } -/// Starts a host on this device after the app is already running. +/// The custom harnesses this device's hosting configuration declares. /// -/// Everything a host needs to exist — the in-process bus, the session manager, -/// the daemon options — is built once at launch and owned by the app loop. A -/// host declared later has no way to reach any of it, which is why adding one -/// used to mean restarting. This carries exactly those pieces to wherever the -/// command is handled. +/// What is left of a larger type. It used to start a host on this device after +/// launch, for the Add Host wizard's "local" kind — a harness plus a directory, +/// which is what an *agent* is now, declared from the host tree instead. Only +/// one reader outlived that: a workflow's `agent` step may name a custom +/// harness, and it resolves the name against this list. /// -/// Cheap to clone: every field is already shared. +/// Cheap to clone: the options behind it are already shared. #[derive(Clone)] -pub(crate) struct LocalHostSpawner { - /// The bus the hub dispatches over. - network: LocalBridgeNetwork, - /// The session manager the UI reads screens from and types into. Shared, so - /// a host started now is as watchable as one started at launch. - sessions: PtyManager, - /// The primary's options, used as the template every extra inherits. +pub(crate) struct LocalHostHarnesses { + /// The primary host's options, which carry the declared custom harnesses. options: EmbeddedDaemonOptions, - /// The process environment, for provider detection and the host switch. - env: HashMap, - /// The runtimes the harness pane resolves tasks against. A new host's - /// runtime is pushed here or its screen would never be found. - runtimes: std::sync::Arc>>, - /// The started hosts, kept alive for the session. Dropping a `LocalHost` - /// stops it, so a spawner that did not hold them would start a host and - /// immediately kill it. - started: std::sync::Arc>>, - /// The agent declarations a newly started host reads its roster entries - /// from. Carried rather than re-read so a host started now and one started - /// at launch are built from the same list. - declared: Vec, - /// Every host this device declares, shared with the hub's roster filter and - /// its `hosts[]` advert. A host bound here must be appended or the roster - /// sink will persist it as a remote entry — and the hub will advertise its - /// agents as running on somebody else's machine. - local_hosts: medulla::hub::SharedLocalHosts, } -impl LocalHostSpawner { - /// Build a spawner over the pieces the app loop owns. - /// - /// Long by construction: every argument is a distinct piece of process-wide - /// state the app loop owns and a host started later has no other way to - /// reach. Grouping them into a struct would only rename the same list. - #[allow(clippy::too_many_arguments)] - pub(crate) fn new( - network: LocalBridgeNetwork, - sessions: PtyManager, - options: EmbeddedDaemonOptions, - env: HashMap, - runtimes: std::sync::Arc>>, - started: std::sync::Arc>>, - local_hosts: medulla::hub::SharedLocalHosts, - declared: Vec, - ) -> Self { - Self { - network, - sessions, - options, - env, - runtimes, - started, - local_hosts, - declared, - } - } - - /// Start `config` now and return the roster entries describing its declared - /// agents — one per agent, so a host is registered as everything it runs - /// rather than as one entry standing in for all of them. - /// - /// `index` is the entry's position within `[[hosts]]`, which is the basis - /// [`all_local_hosts`] and [`start_all`] derive an unnamed host's address - /// from. It is passed in rather than counted here for exactly that reason: - /// counting *started* hosts includes the primary, so a first unnamed extra - /// bound `local-host-2` this run and `local-host-1` on the next launch — - /// an address the roster remembered that nothing would ever bind again. - pub(crate) fn spawn( - &self, - config: &HostSection, - index: usize, - ) -> Result, String> { - let host = start_at( - config, - &self.env, - &self.network, - extra_options(&self.options, config)?, - self.sessions.clone(), - extra_host_address(config, index), - false, - &self.declared, - )?; - let specs = host.specs().to_vec(); - // Before the roster entry exists, so the hub's save filter and its - // `hosts[]` advert both already know this address is device-local by the - // time registration triggers one. - self.local_hosts - .lock() - .expect("local hosts") - .push(medulla::config::LocalHostRef { - id: host.address().to_string(), - name: display_name(config, host.workspace(), false), - workspace: host.workspace().to_string(), - primary: false, - }); - self.runtimes - .lock() - .expect("local harness runtimes") - .push(host.runtime()); - self.started.lock().expect("started hosts").push(host); - Ok(specs) +impl LocalHostHarnesses { + /// Hold a host's options for the sake of the harnesses they declare. + pub(crate) fn new(options: EmbeddedDaemonOptions) -> Self { + Self { options } } /// The custom-harness presets this device's primary host was started with. diff --git a/src/tui/src/ui/app/commands/dispatch.rs b/src/tui/src/ui/app/commands/dispatch.rs index 99c534764..24ebf107d 100644 --- a/src/tui/src/ui/app/commands/dispatch.rs +++ b/src/tui/src/ui/app/commands/dispatch.rs @@ -158,7 +158,6 @@ impl App { self.save_custom_harness(Some(&id), &text); None } - PromptKind::LocalHostWorkspace(harness) => self.add_local_host(harness, &text), PromptKind::RejectProposal { workflow, proposal_id, diff --git a/src/tui/src/ui/app/hosts/edit.rs b/src/tui/src/ui/app/hosts/edit.rs index 13dc6976f..ed768eee4 100644 --- a/src/tui/src/ui/app/hosts/edit.rs +++ b/src/tui/src/ui/app/hosts/edit.rs @@ -159,35 +159,215 @@ impl App { let Some(agent) = self.selected_host_agent() else { return false; }; - if !agent.declared || !agent.editable { + if !agent.editable { return false; } + // A seeded agent has no declaration to remove, so removing one has to + // *start* the list rather than shorten it — see + // [`adopt_seeded_agents`](App::adopt_seeded_agents). + if !agent.declared { + let Some(host) = self.selected_host_row() else { + return false; + }; + return self.adopt_seeded_agents(&host, &agent.agent_id); + } + self.undeclare_agent_id(&agent.agent_id) + } + /// Remove the host under the cursor, and with it every agent this machine + /// declared on that host. + /// + /// A host is not a registry object — it is a group of agents that share an + /// address — so taking one out means undeclaring what this machine wrote + /// down for it *and* removing its roster entries. As with a single agent, + /// the checkouts those agents ran in are left alone. + /// + /// The primary local host is refused: it is the machine the operator is + /// typing on, it is declared by `[host]` rather than by a removable entry, + /// and it would be back at the next launch anyway. + /// + /// Returns the roster mutations to apply on top of the writes, or `None` + /// when there was nothing live to remove — in which case the declarations + /// were still dropped and the status says so. + pub(in crate::ui::app) fn remove_selected_host(&mut self) -> Option { + let host = self.selected_host_row()?; + // A host this device *runs* is not a registry entry to drop — it is + // `[host]` or an entry in `[[hosts]]`, and it starts again from that at + // the next launch. Worse, a running host with no declarations seeds one + // agent per CLI on PATH, so emptying it is what brings them back. + // Removing it would look like a removal for as long as the process lived + // and undo itself on restart, which is the failure this whole change is + // about. Its agents are still removable one by one. + // + // Asked of the *running* hosts rather than of `host.kind`: a row is also + // `Local` when it exists only because agents are declared against its id + // with no daemon here, and that host has no config entry to come back + // from — undeclaring its agents removes it for good. + if self + .local_host_refs() + .iter() + .any(|local| local.id.trim() == host.id.trim()) + { + self.set_status(format!( + "{} is declared on this device — remove its agents individually", + host.label + )); + return None; + } + // Anything else is a host this device does not run: a remote machine, or + // one that exists only because agents were declared against its id. The + // declarations are what keep such a row alive, so they go first — and + // seeded rows cannot occur here, because seeding is done by a running + // daemon and this host has none. + let declared: Vec = host + .agents + .iter() + .filter(|agent| agent.declared && agent.editable) + .map(|agent| agent.agent_id.clone()) + .collect(); + let undeclared = declared + .iter() + .filter(|agent_id| self.undeclare_agent_id(agent_id)) + .count(); + let workers = self.runtime.workers(); + let ops: Vec = host + .agents + .iter() + .filter(|agent| agent.live) + // Trimmed on both sides, matching how the projection claimed the + // worker in the first place. Comparing raw would silently leave a + // padded id in the registry after its host row had gone. + .filter_map(|agent| { + workers + .iter() + .find(|worker| worker.id.trim() == agent.agent_id.trim()) + }) + .map(|worker| WorkerOp::Remove { + id: worker.id.clone(), + }) + .collect(); + // Compared against what was attempted, not just counted: a declaration + // that would not write is an agent that comes back at the next launch, + // and reporting the removal as complete is how the operator finds that + // out later rather than now. + let shortfall = declared.len().saturating_sub(undeclared); + self.set_status(if shortfall > 0 { + format!( + "{}: {shortfall} of {} declaration(s) could not be removed", + host.label, + declared.len() + ) + } else { + format!( + "Removed {} · {undeclared} declaration(s), {} roster entr{}", + host.label, + ops.len(), + if ops.len() == 1 { "y" } else { "ies" } + ) + }); + (!ops.is_empty()).then_some(Cmd::WorkerOps(ops)) + } + + /// Write a host's seeded agents down as declarations, minus the one being + /// removed. + /// + /// An install that has never declared anything advertises a *seeded* list — + /// one agent per coding-agent CLI found on `PATH` + /// ([`seed_declarations`](medulla::runtime::seed_declarations)) — so those + /// rows have no declaration behind them to delete. Removing the roster entry + /// alone is not a removal: the seed is recomputed from `PATH` at the next + /// start and the agent is back, which is exactly what "I deleted it and it + /// is still there" looks like. + /// + /// So the first removal is what makes the list real. The survivors are + /// written as declarations, and from then on the fleet is what the operator + /// declared rather than what happened to be installed. Declarations on other + /// hosts are carried through untouched. + /// + /// Returns whether the list was written. + fn adopt_seeded_agents(&mut self, host: &HostRow, removing: &str) -> bool { + let current = self.loaded.config.fleet.agent_declarations.clone(); + let survivors: Vec = host + .agents + .iter() + .filter(|agent| agent.agent_id.trim() != removing.trim()) + // Only what this machine can declare. A row it may not edit is not + // ours to write down, and writing it would claim an agent that + // belongs to another host's config. + .filter(|agent| agent.editable) + .filter_map(|agent| { + // A host can hold both kinds of row at once, so the declared + // ones are carried through *as they are*. Rebuilding them from + // the projection would look harmless — the id, harness, + // workspace and roles all survive the trip — and would silently + // drop the two fields the projection does not carry: the name + // the operator gave the agent, and any strategy other than the + // `checkout` default `AgentDeclaration::new` hardcodes. + medulla::config::agent_declaration(¤t, &agent.agent_id) + .cloned() + .or_else(|| declaration_for(host, agent)) + }) + .collect(); + let mut declarations: Vec = self + .loaded + .config + .fleet + .agent_declarations + .iter() + .filter(|declaration| !declaration.on_host(&host.id)) + .cloned() + .collect(); + declarations.extend(survivors); + let Some(path) = self.config_path.clone() else { + self.loaded.config.fleet.agent_declarations = declarations; + self.set_status(format!( + "Removed {removing} (this run only — no config file)" + )); + return true; + }; + match medulla::config::persist_agent_declarations(&path, &declarations) { + Ok(()) => { + self.loaded.config.fleet.agent_declarations = declarations; + self.set_status(format!( + "Removed {removing} · the remaining agents are now declared" + )); + true + } + Err(error) => { + self.set_status(format!("{removing} was not removed: {error}")); + false + } + } + } + + /// Undeclare one agent by id, writing the shortened list to disk. + /// + /// The half of [`undeclare_selected_agent`](App::undeclare_selected_agent) + /// that does not depend on the cursor, so removing a whole host can reach it + /// per agent rather than by walking the selection over each row first. + fn undeclare_agent_id(&mut self, agent_id: &str) -> bool { let Some(path) = self.config_path.clone() else { // Same rule as a role edit: nowhere to write is not a refusal, it is // an edit that lasts one run — and the status is what keeps the // agent's return at the next launch from being a surprise. medulla::config::remove_agent_declaration( &mut self.loaded.config.fleet.agent_declarations, - &agent.agent_id, + agent_id, ); self.set_status(format!( "Undeclared {} (this run only — no config file)", - agent.agent_id + agent_id )); return true; }; let current = self.loaded.config.fleet.agent_declarations.clone(); - match medulla::config::undeclare_agent(&path, ¤t, &agent.agent_id) { + match medulla::config::undeclare_agent(&path, ¤t, agent_id) { Ok(declarations) => { self.loaded.config.fleet.agent_declarations = declarations; - self.set_status(format!( - "Undeclared {} · its files are untouched", - agent.agent_id - )); + self.set_status(format!("Undeclared {} · its files are untouched", agent_id)); true } Err(error) => { - self.set_status(format!("{} was not undeclared: {error}", agent.agent_id)); + self.set_status(format!("{} was not undeclared: {error}", agent_id)); false } } @@ -226,3 +406,16 @@ impl App { } } } + +/// The declaration a seeded row stands for. +/// +/// `None` when the row names no harness or no workspace: a declaration is a +/// `harness × workspace` pair, and one missing half cannot be written down. +fn declaration_for(host: &HostRow, agent: &HostAgentRow) -> Option { + let harness = agent.harness.as_deref()?; + let workspace = agent.workspace.as_deref()?; + let mut declaration = + AgentDeclaration::new(agent.agent_id.trim(), host.id.trim(), harness, workspace); + declaration.roles = agent.roles.clone(); + Some(declaration) +} diff --git a/src/tui/src/ui/app/hosts/mod.rs b/src/tui/src/ui/app/hosts/mod.rs index fa8a22eb2..8e9373ec5 100644 --- a/src/tui/src/ui/app/hosts/mod.rs +++ b/src/tui/src/ui/app/hosts/mod.rs @@ -141,6 +141,16 @@ impl App { tree.get(row.host)?.agents.get(row.agent?).cloned() } + /// Whether the preview's role toggles hold the arrows. + /// + /// The Hosts page has two cursors on one screen — the tree and the toggle + /// list — and which of them a keypress reaches is not legible from the + /// rendered buffer alone: an agent preview is drawn either way. + /// Test/inspection seam. + pub fn host_roles_focused(&self) -> bool { + self.host_roles_focus + } + /// Whether the cursor is on a host header rather than an agent. /// Test/inspection seam. pub fn hosts_cursor_on_host(&self) -> bool { diff --git a/src/tui/src/ui/app/hosts/tests.rs b/src/tui/src/ui/app/hosts/tests.rs index 919f37992..5a556b333 100644 --- a/src/tui/src/ui/app/hosts/tests.rs +++ b/src/tui/src/ui/app/hosts/tests.rs @@ -500,3 +500,215 @@ fn a_host_this_device_does_not_serve_drops_out_unless_it_still_holds_something() "declared agents keep their host listed" ); } + +/// Press `d` on whatever the cursor is on, and return the fleet mutation it +/// asked for. +fn press_delete(app: &mut App) -> Option { + app.on_event(crossterm::event::Event::Key( + crossterm::event::KeyEvent::new( + crossterm::event::KeyCode::Char('d'), + crossterm::event::KeyModifiers::NONE, + ), + )) +} + +/// The ids a fleet mutation removes, in order. +fn removed_ids(cmd: Option) -> Vec { + match cmd { + Some(Cmd::WorkerOp(medulla::runtime::WorkerOp::Remove { id })) => vec![id], + Some(Cmd::WorkerOps(ops)) => ops + .into_iter() + .map(|op| match op { + medulla::runtime::WorkerOp::Remove { id } => id, + other => panic!("expected only removals, got {other:?}"), + }) + .collect(), + other => panic!("expected a removal, got {other:?}"), + } +} + +#[test] +fn deleting_a_host_takes_every_agent_on_it_not_just_the_one_that_probed_it() { + // Two agents on one remote machine. Before this was pointer-aware, `d` on + // the host row resolved to the host's `detail_worker` — a single entry — + // and removed that one agent while leaving the host and its sibling. + let (mut app, _dir) = app_with( + vec![worker("peer-claude", "7Kx"), worker("peer-codex", "7Kx")], + Vec::new(), + ); + // Row 0 is the local host; row 1 is the remote host header. + cursor_to(&mut app, 1); + assert!(app.hosts_cursor_on_host(), "the cursor is on the host row"); + + let removed = removed_ids(press_delete(&mut app)); + assert_eq!( + removed, + vec!["peer-claude".to_string(), "peer-codex".to_string()], + "both agents on the host go, not only the one that probed it" + ); +} + +#[test] +fn deleting_an_agent_leaves_its_host_and_its_siblings_standing() { + let (mut app, _dir) = app_with( + vec![worker("peer-claude", "7Kx"), worker("peer-codex", "7Kx")], + Vec::new(), + ); + // Row 2 is the first agent under the remote host. + cursor_to(&mut app, 2); + assert_eq!( + app.selected_host_agent().map(|agent| agent.agent_id), + Some("peer-claude".to_string()) + ); + + assert_eq!( + removed_ids(press_delete(&mut app)), + vec!["peer-claude".to_string()], + "only the agent under the cursor" + ); +} + +#[test] +fn deleting_a_declared_agent_by_its_host_row_undeclares_it_too() { + // A declaration is what re-creates an agent at the next launch, so removing + // the host has to drop it as well or the removal does not survive a restart. + let (mut app, _dir) = app_with( + vec![worker("peer-codex", "7Kx")], + vec![AgentDeclaration::new( + "peer-codex", + "7Kx", + "codex", + "/w/api", + )], + ); + cursor_to(&mut app, 1); + assert!(app.hosts_cursor_on_host()); + + assert_eq!( + removed_ids(press_delete(&mut app)), + vec!["peer-codex".to_string()] + ); + // Asserted on the list the app reads, not on the file: `app_with` seeds + // declarations in memory only, so a file assertion here would pass whether + // or not the removal happened. + assert!( + app.loaded.config.fleet.agent_declarations.is_empty(), + "the declaration went with the host" + ); +} + +#[test] +fn a_host_this_device_runs_is_not_removable() { + // It is declared by `[host]` rather than by a removable entry, so it would + // be back at the next launch — and a running host with no declarations + // seeds one agent per CLI on PATH, so emptying it is exactly what brings + // them back. Reporting a removal that does not last is worse than refusing. + let (mut app, _dir) = app_with( + vec![worker("medulla-claude", "this-device")], + vec![AgentDeclaration::new( + "medulla-claude", + "this-device", + "claude", + "/w/medulla", + )], + ); + cursor_to(&mut app, 0); + assert!(app.hosts_cursor_on_host()); + assert!(app.selected_host_is_local()); + + assert!( + press_delete(&mut app).is_none(), + "the local host asks for no fleet mutation" + ); + assert_eq!( + app.loaded.config.fleet.agent_declarations.len(), + 1, + "and it takes nothing down with it" + ); +} + +#[test] +fn deleting_a_seeded_agent_declares_the_rest_so_the_removal_survives() { + // The state every fresh install is in: agents in the roster, nothing in + // `[fleet].agentDeclarations`. Those rows are seeded from the CLIs on PATH, + // so there is no declaration to shorten — and removing only the roster entry + // would let the seed put it back at the next start. + let (mut app, _dir) = app_with( + vec![ + worker("medulla-claude", "this-device"), + worker("medulla-codex", "this-device"), + ], + Vec::new(), + ); + cursor_to(&mut app, 1); + let target = app + .selected_host_agent() + .expect("row 1 is an agent") + .agent_id; + assert!( + app.loaded.config.fleet.agent_declarations.is_empty(), + "nothing is declared yet" + ); + + let _ = press_delete(&mut app); + + let declared: Vec = app + .loaded + .config + .fleet + .agent_declarations + .iter() + .map(|declaration| declaration.agent_id.clone()) + .collect(); + assert!( + !declared.contains(&target), + "the removed agent is not declared: {declared:?}" + ); + assert!( + !declared.is_empty(), + "and its siblings now are, so the list stops being seeded: {declared:?}" + ); +} + +#[test] +fn adopting_seeds_does_not_rewrite_the_agents_already_declared() { + // A host can hold both kinds of row at once. The declared one carries fields + // the projection never sees — the operator's name for it, and any strategy + // other than the default — so rebuilding it from the row would drop them + // while looking like it had preserved everything. + let mut named = AgentDeclaration::new("medulla-claude", "this-device", "claude", "/w/medulla"); + named.name = Some("build box".into()); + let (mut app, _dir) = app_with( + vec![ + worker("medulla-claude", "this-device"), + worker("medulla-codex", "this-device"), + ], + vec![named], + ); + // Row 2 is the seeded sibling: undeclared, so removing it adopts the rest. + cursor_to(&mut app, 2); + let target = app + .selected_host_agent() + .expect("row 2 is an agent") + .agent_id; + assert_ne!( + target, "medulla-claude", + "the seeded row, not the declared one" + ); + + let _ = press_delete(&mut app); + + let kept = app + .loaded + .config + .fleet + .agent_declarations + .iter() + .find(|declaration| declaration.agent_id == "medulla-claude") + .expect("the declared survivor is still declared"); + assert_eq!( + kept.name.as_deref(), + Some("build box"), + "and kept the name the projection does not carry" + ); +} diff --git a/src/tui/src/ui/app/keys/routing/add_host.rs b/src/tui/src/ui/app/keys/routing/add_host.rs index 8a69c6973..f33a9d668 100644 --- a/src/tui/src/ui/app/keys/routing/add_host.rs +++ b/src/tui/src/ui/app/keys/routing/add_host.rs @@ -1,106 +1,41 @@ //! Keyboard handling for the Add Host wizard. //! -//! Split from the Routing key module because it is a multi-step flow with its -//! own state — which kind, whether that kind is settled, which harness — rather -//! than the single-key actions its sibling panes use, and `mod.rs` is for +//! Split from the Routing key module because it is a flow with its own keys +//! rather than the single-key actions its sibling panes use, and `mod.rs` is for //! wiring. use crossterm::event::KeyCode; -use crate::ui::composer::Draft; use medulla::daemon::pairing::REMOTE_JOIN_COMMAND; -use super::super::super::types::{AddHostKind, App, Prompt, PromptKind}; +use super::super::super::types::App; use super::RoutingKey; impl App { - /// Open the existing host-address prompt from the dedicated Add Host pane, - /// or copy the line the operator has to run on the machine being added. + /// Open the host-address prompt, or copy the line to run on the machine + /// being added. + /// + /// There is no kind to choose: this page adds another *machine*. A harness + /// plus a directory on this one is an agent, and it is declared from the + /// host tree (`n`) rather than here. pub(super) fn add_host_key(&mut self, code: KeyCode) -> RoutingKey { match code { - // The arrows move whichever list the page is currently showing: - // the kind picker until one is chosen, the harness list after. One - // pair of keys for a page that is read top to bottom. - KeyCode::Up | KeyCode::Char('k') | KeyCode::Down | KeyCode::Char('j') => { - let up = matches!(code, KeyCode::Up | KeyCode::Char('k')); - // A confirmed kind is settled — the wizard has moved past it and - // the arrows belong to the live step. Letting them keep driving - // the kind list meant confirming Remote and arrowing to Local - // carried the confirmation across, so the next Enter skipped - // "Choose a harness type" and asked for a directory for a harness type - // nobody had picked. Esc is how you go back a step. - match (self.add_host_selected_kind(), self.add_host_kind_chosen) { - (AddHostKind::Local, true) => { - let len = self.add_host_providers().len(); - self.add_host_harness = crate::ui::selection::moved( - self.add_host_harness.min(len.saturating_sub(1)), - len, - up, - ); - } - // Remote's confirmed step is instructions, with nothing to - // move through. Consumed rather than passed on, so an arrow - // here cannot silently reopen the kind choice. - (_, true) => {} - (_, false) => { - self.add_host_kind = crate::ui::selection::moved( - self.add_host_kind.min(AddHostKind::ALL.len() - 1), - AddHostKind::ALL.len(), - up, - ); - } - } - RoutingKey::Handled(None) - } KeyCode::Enter | KeyCode::Char('a') => { - match self.add_host_selected_kind() { - // Both kinds settle the choice first, so the page always - // reads the same way: pick a kind, then the steps it needs - // light up. Remote's are instructions rather than a list, - // but jumping straight to the prompt made the same key mean - // two different things depending on which row was under it. - AddHostKind::Remote if !self.add_host_kind_chosen => { - self.add_host_kind_chosen = true; - self.set_status( - "Run the line on that machine · Enter to paste its address", - ); - } - AddHostKind::Remote => { - self.add_host_kind_chosen = false; - self.open_add_host_prompt(); - } - // Two Enters for a local host: the first settles the - // harness, the second asks where it works. Collapsing them - // would mean the arrows never reached the harness list. - AddHostKind::Local if !self.add_host_kind_chosen => { - self.add_host_kind_chosen = true; - self.set_status("Choose a harness type · Enter to set the directory"); - } - AddHostKind::Local => { - let providers = self.add_host_providers(); - let harness = providers[self.add_host_harness.min(providers.len() - 1)]; - self.prompt = Some(Prompt { - kind: PromptKind::LocalHostWorkspace(harness), - title: format!( - "Directory for the {} host — blank uses this one", - harness.as_str() - ), - draft: Draft::new(), - }); - self.add_host_kind_chosen = false; - self.set_status("Add local host · Enter save · Esc cancel"); - } - } + self.open_add_host_prompt(); RoutingKey::Handled(None) } // Copying it here rather than retyping it there is the whole point: - // this end is a local terminal, so the copy is free. Remote only — - // there is no install line in the local flow, and the hint no longer - // offers one. - KeyCode::Char('c') if self.add_host_selected_kind() == AddHostKind::Remote => { + // this end is a local terminal, so the copy is free. + KeyCode::Char('c') => { self.copy_line("the worker install line", REMOTE_JOIN_COMMAND); RoutingKey::Handled(None) } + // The page is instructions — there is no list to walk. Consumed so + // an arrow cannot fall through to the pane navigation and move the + // operator off a page they are reading. + KeyCode::Up | KeyCode::Char('k') | KeyCode::Down | KeyCode::Char('j') => { + RoutingKey::Handled(None) + } _ => RoutingKey::Unhandled, } } diff --git a/src/tui/src/ui/app/keys/routing/mod.rs b/src/tui/src/ui/app/keys/routing/mod.rs index 86d42c34a..b2948aa44 100644 --- a/src/tui/src/ui/app/keys/routing/mod.rs +++ b/src/tui/src/ui/app/keys/routing/mod.rs @@ -16,15 +16,6 @@ use super::super::types::{ impl App { /// Handle Routing navigation and the active pane's actions. pub(super) fn on_routing_key(&mut self, code: KeyCode) -> RoutingKey { - // Claimed before the pane navigation, which treats Esc as "leave the - // content pane". Mid-wizard that is the wrong answer: a mis-picked kind - // should cost one step, not the whole page. Esc leaves the page as - // usual once the wizard is back at its first step. - if code == KeyCode::Esc && self.routing_index == RP_ADD_HOST && self.add_host_kind_chosen { - self.add_host_kind_chosen = false; - self.set_status("Choose a kind of host"); - return RoutingKey::Handled(None); - } match multi_pane::navigate( code, ROUTING_SUBPAGES.len(), @@ -207,22 +198,31 @@ impl App { } } - /// Remove what the cursor is on: an agent, or a whole remote host. + /// Remove what the cursor is on: one agent, or the whole host it sits under. /// /// An agent this machine declared is *undeclared* first — dropping only the /// roster entry would leave the declaration behind to re-create it at the /// next launch, which reads as a removal that did not take. fn remove_host_row(&mut self) -> Option { - // Both reads happen before the undeclare: removing a declaration - // reshapes the tree under the cursor, and resolving the roster entry - // afterwards would answer for whichever row slid into its place. - let agent = self.selected_host_agent(); - let worker = self.selected_host(); // The removal key is reachable while the role toggles hold the arrows — // `host_roles_key` passes `d`/`x` through. Leaving the focus on would // point the next arrow at the roles of whichever row slid up into the // cursor, which is not the agent whose toggles were open. self.host_roles_focus = false; + // The row kind decides what is removed, and it has to be asked first. + // Falling through to the agent path on a host row used to resolve + // `selected_host()` to that host's `detail_worker` — whichever single + // entry happened to have probed the machine — so `d` on a host of three + // agents removed *one* of them and left the host standing. The cursor + // says host; the removal has to mean host. + if self.hosts_cursor_on_host() { + return self.remove_selected_host(); + } + // Both reads happen before the undeclare: removing a declaration + // reshapes the tree under the cursor, and resolving the roster entry + // afterwards would answer for whichever row slid into its place. + let agent = self.selected_host_agent(); + let worker = self.selected_host(); let undeclared = self.undeclare_selected_agent(); match (agent, worker) { // Declared, not running: the declaration was the whole of it. diff --git a/src/tui/src/ui/app/rail/mod.rs b/src/tui/src/ui/app/rail/mod.rs index def8ab4e3..f218f19e2 100644 --- a/src/tui/src/ui/app/rail/mod.rs +++ b/src/tui/src/ui/app/rail/mod.rs @@ -257,6 +257,12 @@ impl App { if hosting { rows.push(RailRow::NewAgent); } + // Only over a tree that exists — see [`RailRow::AgentsHeader`]. Counted + // from the groups rather than from `rows`, because the host rows that + // wrap them have not been pushed yet. + if hosts.iter().any(|host| !host.agents.is_empty()) { + rows.push(RailRow::AgentsHeader); + } // Which agents this machine may open a session under: a session is // started by the host that owns the agent, so only the agents declared // here get the action. Collected once rather than re-scanned per group. diff --git a/src/tui/src/ui/app/rail/tests.rs b/src/tui/src/ui/app/rail/tests.rs index 99845476a..6b121065d 100644 --- a/src/tui/src/ui/app/rail/tests.rs +++ b/src/tui/src/ui/app/rail/tests.rs @@ -366,8 +366,9 @@ fn a_row_answers_for_the_agent_and_the_lane_behind_it() { assert_eq!(row.agent_id(), session.agent_id.as_deref()); assert_eq!(row.lane_index(), session.lane_index); } - // Hosts and the create action are about no agent and no lane. - RailRow::Host(_) | RailRow::NewAgent => { + // Hosts, the heading and the create action are about no agent and + // no lane. + RailRow::Host(_) | RailRow::NewAgent | RailRow::AgentsHeader => { assert_eq!(row.agent_id(), None); assert_eq!(row.lane_index(), None); assert_eq!(row.session_id(), None); @@ -398,6 +399,9 @@ fn only_the_rows_that_name_something_take_the_cursor() { for row in app.rail_rows() { match row { RailRow::Host(_) => assert!(!row.selectable(), "a host header is a label"), + RailRow::AgentsHeader => { + assert!(!row.selectable(), "the agents heading is a label") + } RailRow::Agent(_) | RailRow::Session(_) | RailRow::NewAgent @@ -408,3 +412,51 @@ fn only_the_rows_that_name_something_take_the_cursor() { } } } + +#[test] +fn the_agents_heading_sits_under_the_create_action_and_only_over_a_tree() { + let mut app = hosting_app(); + app.loaded.config.fleet.agent_declarations = + vec![AgentDeclaration::new("api-codex", "", "codex", "/w/api")]; + let rows = app.rail_rows(); + let new_agent = rows + .iter() + .position(|row| matches!(row, RailRow::NewAgent)) + .expect("the create action is on the rail"); + let heading = rows + .iter() + .position(|row| matches!(row, RailRow::AgentsHeader)) + .expect("a rail with agents heads them"); + let first_agent = rows + .iter() + .position(|row| matches!(row, RailRow::Agent(_))) + .expect("this fixture declares agents"); + assert_eq!( + heading, + new_agent + 1, + "the heading follows the button it explains" + ); + assert!( + heading < first_agent, + "and precedes the tree it heads: {rows:?}" + ); + + // The other half of "only over a tree": a hosting device with nothing + // declared and no traffic still offers the create action, and a heading + // there would announce a section that is not on the rail. Built on the empty + // runtime rather than `hosting_app`, whose demo lanes are themselves agents. + let mut bare = App::new( + Arc::new(MockRuntime::empty()) as Arc, + LoadedConfig::defaults("medulla.tui.json".into()), + ); + bare.set_local_sessions(shell_harnesses(PtyManager::new())); + let rows = bare.rail_rows(); + assert!( + rows.iter().any(|row| matches!(row, RailRow::NewAgent)), + "the create action is still offered: {rows:?}" + ); + assert!( + !rows.iter().any(|row| matches!(row, RailRow::AgentsHeader)), + "but nothing is headed: {rows:?}" + ); +} diff --git a/src/tui/src/ui/app/rail/types.rs b/src/tui/src/ui/app/rail/types.rs index afa88b0ed..660bdfb1f 100644 --- a/src/tui/src/ui/app/rail/types.rs +++ b/src/tui/src/ui/app/rail/types.rs @@ -145,6 +145,16 @@ pub enum RailRow { /// other variant — and a rail is a `Vec` rebuilt each frame, so the /// widest variant is what every row costs. Session(Box), + /// The heading over the agent tree. + /// + /// Sits under the create action, so the rail reads as "here is the button, + /// and here is what it has made". Without it the first agent row followed + /// the button directly and the two read as one block — a list whose first + /// entry happened to be green. + /// + /// Emitted only when there is a tree to head: a heading over nothing + /// announces a section the operator cannot see. + AgentsHeader, /// The action row that declares a new agent on this machine. /// /// Sits directly above the tree it produces, because a machine with no @@ -182,6 +192,8 @@ impl RailRow { RailRow::Agent(_) => true, RailRow::Session(_) => true, RailRow::NewAgent => true, + // A label, not a row: the cursor skips it like a host header. + RailRow::AgentsHeader => false, RailRow::NewSession { .. } => true, RailRow::Lane(row) => row.selectable(), } @@ -209,7 +221,10 @@ impl RailRow { RailRow::Agent(row) => row.lane_index, RailRow::Session(row) => row.lane_index, RailRow::Lane(row) => row.lane_index(), - RailRow::Host(_) | RailRow::NewAgent | RailRow::NewSession { .. } => None, + RailRow::Host(_) + | RailRow::NewAgent + | RailRow::AgentsHeader + | RailRow::NewSession { .. } => None, } } diff --git a/src/tui/src/ui/app/render/agents/rail/mod.rs b/src/tui/src/ui/app/render/agents/rail/mod.rs index b2245f94c..9365a4e8c 100644 --- a/src/tui/src/ui/app/render/agents/rail/mod.rs +++ b/src/tui/src/ui/app/render/agents/rail/mod.rs @@ -343,6 +343,12 @@ impl App { self.declared_agent_line(agent, lanes, active, waiting_sessions) } RailRow::NewAgent => self.new_agent_line(active), + // Same shape as the lane list's `── functions ──`, so the two + // headings on one rail read as the same kind of thing. + RailRow::AgentsHeader => TLine::from(Span::styled( + "── agents ──", + Style::default().add_modifier(Modifier::DIM), + )), RailRow::NewSession { .. } => self.new_session_line(active), RailRow::Session(session) => match (&session.task, &session.local) { (Some(task), _) => self.agent_row_line( @@ -390,10 +396,17 @@ impl App { waiting_sessions, ); } - let style = if active { - self.theme.selection() + // The name carries the row; the harness and directory qualify it. Both + // were dim, which left an idle agent with nothing to read it by — the + // whole row receded, including the one word that identifies it. Only the + // qualifier recedes now. + let (name_style, detail_style) = if active { + (self.theme.selection(), self.theme.selection()) } else { - Style::default().add_modifier(Modifier::DIM) + ( + Style::default().fg(color("cyan")), + Style::default().add_modifier(Modifier::DIM), + ) }; let detail = match (agent.harness(), agent.workspace()) { (Some(harness), Some(workspace)) => format!( @@ -403,7 +416,10 @@ impl App { (Some(harness), None) => format!(" · {harness}"), _ => String::new(), }; - TLine::from(Span::styled(format!("○ {}{detail}", agent.label()), style)) + TLine::from(vec![ + Span::styled(format!("○ {}", agent.label()), name_style), + Span::styled(detail, detail_style), + ]) } /// Format the `+ New agent` action row. diff --git a/src/tui/src/ui/app/render/routing/add_host.rs b/src/tui/src/ui/app/render/routing/add_host.rs index 61a425968..25dbbf6c8 100644 --- a/src/tui/src/ui/app/render/routing/add_host.rs +++ b/src/tui/src/ui/app/render/routing/add_host.rs @@ -1,4 +1,10 @@ -//! Guided entry point for connecting another host. +//! Guided entry point for connecting another *machine*. +//! +//! Remote only, and deliberately. A "local host" used to be a kind offered +//! here — a harness plus a directory on this machine — but that is exactly what +//! an agent is now, and it is declared where the agents are: `n` on a local host +//! row in the Hosts tab. Offering both meant two flows that wrote the same thing +//! and disagreed about what to call it. //! //! The page is written as two steps because pairing is genuinely two steps, and //! because each copy it asks for runs in the *easy* direction. Step one is @@ -19,223 +25,62 @@ use medulla::daemon::pairing::REMOTE_JOIN_COMMAND; use super::super::super::types::App; impl App { - /// The harnesses a new local host can be pointed at. - /// - /// What this machine actually has, not what the protocol knows about: a - /// picker offering a CLI that is not installed produces a host that accepts - /// work and fails every task. - pub(in crate::ui::app) fn add_host_providers(&self) -> Vec { - // Computed once per process. Detection stat-checks provider binaries on - // `PATH`, and both the draw and the key handler call this — so without - // the cache the page did that work every frame and every keystroke. - self.add_host_provider_cache - .get_or_init(|| { - let env: std::collections::HashMap = std::env::vars().collect(); - let detected = medulla::daemon::providers::detect_providers(&env, None, None); - if detected.is_empty() { - vec![medulla::protocol::HarnessProvider::Claude] - } else { - detected - } - }) - .clone() - } - - /// The kind currently under the cursor. - pub(in crate::ui::app) fn add_host_selected_kind( - &self, - ) -> super::super::super::types::AddHostKind { - use super::super::super::types::AddHostKind; - AddHostKind::ALL[self.add_host_kind.min(AddHostKind::ALL.len() - 1)] - } - - /// Draw the Add Host wizard: one step live at a time, the rest shown but - /// inert. + /// Draw the Add Host wizard: what to run over there, then what to paste here. /// - /// Every step stays on screen so the shape of the task is visible from the - /// first keypress — but only the live one carries a cursor and full colour. - /// Rendering two identical-looking lists at once was the problem this - /// fixes: the arrows drive exactly one of them and nothing said which. + /// Both steps stay on screen so the shape of the task is visible from the + /// first keypress. Each copy it asks for runs in the *easy* direction — step + /// one is copied from this local terminal, step two is put on this + /// terminal's clipboard by the remote worker over OSC 52. pub(super) fn draw_add_host(&self, f: &mut Frame, area: Rect) { - use super::super::super::types::AddHostKind; - let dim = Style::default().add_modifier(Modifier::DIM); let live = Style::default() .fg(Color::Cyan) .add_modifier(Modifier::BOLD); - let done = Style::default().fg(Color::Green); - let picked = Style::default() - .fg(Color::Green) - .add_modifier(Modifier::BOLD); - let kind = self.add_host_selected_kind(); - let on_kind = !self.add_host_kind_chosen; - - // A step header: live when it is the one taking keys, done when it is - // behind the cursor, dim when it is still ahead. - let header = |n: usize, title: &str, state: StepState| { - let style = match state { - StepState::Live => live, - StepState::Done => done, - StepState::Ahead => dim, - }; - let mark = match state { - StepState::Done => "✓", - _ => "·", - }; - TLine::from(Span::styled(format!("{n} {mark} {title}"), style)) - }; + let header = + |n: usize, title: &str| TLine::from(Span::styled(format!("{n} · {title}"), live)); let mut lines = vec![ - TLine::from("Add a host for the orchestrator to delegate to."), + TLine::from("Connect another machine for the orchestrator to delegate to."), + TLine::from(Span::styled( + "To add an agent on this device, press n on its host row in the tree.", + dim, + )), TLine::from(""), ]; - // 1 · kind - lines.push(header( - 1, - "What kind of host", - if on_kind { - StepState::Live - } else { - StepState::Done - }, - )); + lines.push(header(1, "On the machine you want to add")); lines.push(TLine::from("")); - for (index, option) in AddHostKind::ALL.iter().enumerate() { - let under_cursor = index == self.add_host_kind.min(AddHostKind::ALL.len() - 1); - // Once chosen, only the choice remains: the alternatives are noise - // on a step the operator has already answered. - if !on_kind && !under_cursor { - continue; - } - let marker = if on_kind && under_cursor { - " ▸ " - } else { - " " - }; - let label_style = match (on_kind, under_cursor) { - (true, true) => picked, - (false, true) => done, - _ => Style::default(), - }; - lines.push(TLine::from(vec![ - Span::styled(marker, if under_cursor { label_style } else { dim }), - Span::styled(option.label(), label_style), - Span::styled(format!(" {}", option.description()), dim), - ])); - } + lines.push(TLine::from(Span::styled( + format!(" {REMOTE_JOIN_COMMAND}"), + Style::default().fg(Color::Green), + ))); lines.push(TLine::from("")); - - match kind { - AddHostKind::Remote => { - let state = if on_kind { - StepState::Ahead - } else { - StepState::Live - }; - let body = if on_kind { dim } else { Style::default() }; - lines.push(header(2, "On the machine you want to add", state)); - lines.push(TLine::from("")); - lines.push(TLine::from(Span::styled( - format!(" {REMOTE_JOIN_COMMAND}"), - if on_kind { - dim - } else { - Style::default().fg(Color::Green) - }, - ))); - lines.push(TLine::from("")); - lines.push(TLine::from(Span::styled( - " Press c to copy the installer, then paste it into an SSH session.", - dim, - ))); - lines.push(TLine::from(Span::styled( - " Provision the host-link identity before starting `medulla daemon`.", - dim, - ))); - lines.push(TLine::from("")); - lines.push(header(3, "Back here", state)); - lines.push(TLine::from("")); - lines.push(TLine::from(Span::styled( - " The worker prints its address and puts it on your clipboard,", - body, - ))); - lines.push(TLine::from(Span::styled( - " even over SSH. Press Enter and paste it, with an optional label.", - body, - ))); - lines.push(TLine::from("")); - lines.push(TLine::from(Span::styled( - " Example: 7Kx…9fQ Primary build machine", - dim, - ))); - } - AddHostKind::Local => { - let state = if on_kind { - StepState::Ahead - } else { - StepState::Live - }; - lines.push(header(2, "Which harness type it runs", state)); - lines.push(TLine::from("")); - let providers = self.add_host_providers(); - for (index, provider) in providers.iter().enumerate() { - let under_cursor = index == self.add_host_harness.min(providers.len() - 1); - let marker = if !on_kind && under_cursor { - " ▸ " - } else { - " " - }; - let style = match (on_kind, under_cursor) { - (false, true) => picked, - (true, _) => dim, - _ => Style::default(), - }; - lines.push(TLine::from(vec![ - Span::styled(marker, style), - Span::styled(provider.as_str().to_string(), style), - ])); - } - lines.push(TLine::from("")); - lines.push(header(3, "Where it works", state)); - lines.push(TLine::from("")); - let body = if on_kind { dim } else { Style::default() }; - lines.push(TLine::from(Span::styled( - " Press Enter and give it a directory. Blank uses this one:", - body, - ))); - lines.push(TLine::from("")); - lines.push(TLine::from(Span::styled( - format!( - " {}", - std::env::current_dir() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_else(|_| ".".to_string()) - ), - dim, - ))); - lines.push(TLine::from("")); - lines.push(TLine::from(Span::styled( - " Runs in this process, so its session is watchable and typeable.", - dim, - ))); - } - } - + lines.push(TLine::from(Span::styled( + " Press c to copy the installer, then paste it into an SSH session.", + dim, + ))); + lines.push(TLine::from(Span::styled( + " Provision the host-link identity before starting `medulla daemon`.", + dim, + ))); + lines.push(TLine::from("")); + lines.push(header(2, "Back here")); + lines.push(TLine::from("")); + lines.push(TLine::from( + " The worker prints its address and puts it on your clipboard,", + )); + lines.push(TLine::from( + " even over SSH. Press Enter and paste it, with an optional label.", + )); lines.push(TLine::from("")); lines.push(TLine::from(Span::styled( - // The install line exists only in the Remote branch, so offering `c` - // on a local host advertises a key that copies nothing. - match (on_kind, kind) { - (true, _) => "↑↓ choose a kind · Enter continue · Esc back to the menu", - (false, AddHostKind::Remote) => { - "Enter continue · Esc start over · c copy the install line" - } - (false, AddHostKind::Local) => { - "↑↓ choose a harness type · Enter continue · Esc start over" - } - }, + " Example: 7Kx…9fQ Primary build machine", + dim, + ))); + lines.push(TLine::from("")); + lines.push(TLine::from(Span::styled( + "Enter paste the address · c copy the install line · Esc back to the menu", dim, ))); @@ -247,14 +92,3 @@ impl App { ); } } - -/// Where a wizard step sits relative to the cursor. -#[derive(Clone, Copy, PartialEq, Eq)] -enum StepState { - /// Behind the cursor: answered. - Done, - /// The step taking keys right now. - Live, - /// Not reached yet. - Ahead, -} diff --git a/src/tui/src/ui/app/render/routing/hosts/list.rs b/src/tui/src/ui/app/render/routing/hosts/list.rs index c134698b4..8163dabe3 100644 --- a/src/tui/src/ui/app/render/routing/hosts/list.rs +++ b/src/tui/src/ui/app/render/routing/hosts/list.rs @@ -5,6 +5,7 @@ use ratatui::style::{Color, Modifier, Style}; use ratatui::text::{Line as TLine, Span, Text}; use ratatui::widgets::Paragraph; use ratatui::Frame; +use unicode_width::UnicodeWidthStr; use medulla::ui::hosts::{HostAgentRow, HostKind, HostRow}; @@ -42,12 +43,23 @@ impl App { let footer_rows = 1 + usize::from(self.snapshot.link.is_some()) * 2; let visible = usize::from(inner.height).saturating_sub(footer_rows).max(1); let start = crate::ui::selection::viewport_start(selected, rows.len(), visible); + // Measured over the whole tree, not the visible window: columns that + // resize as the list scrolls make the text appear to shift sideways + // under a cursor that only moved down. + let columns = Columns::measure(tree); for (index, row) in rows.iter().enumerate().skip(start).take(visible) { let Some(host) = tree.get(row.host) else { continue; }; let (text, mut style) = match row.agent.and_then(|at| host.agents.get(at)) { - Some(agent) => (agent_line(agent), agent_style(agent)), + Some(agent) => ( + agent_line( + agent, + row.agent == Some(host.agents.len().saturating_sub(1)), + &columns, + ), + agent_style(agent), + ), None => (host_line(host), host_style(host)), }; if index == selected { @@ -97,38 +109,156 @@ fn host_line(host: &HostRow) -> String { ) } +/// The width of each aligned column in the agent rows. +/// +/// Measured across the *whole* tree rather than per host, which is the point: +/// columns that restart under every header are not columns, and comparing two +/// machines' agents means reading down a line rather than across a paragraph. +#[derive(Debug, Clone, Copy)] +struct Columns { + /// Width of the agent-id column. + id: usize, + /// Width of the harness column. + harness: usize, + /// Width of the workspace column. + workspace: usize, +} + +/// Ceilings for the measured columns. +/// +/// A single long id or a deep checkout path would otherwise set a width every +/// other row pays for, pushing the columns that carry the most meaning — roles, +/// and whether the agent is running at all — off a narrow terminal. +const MAX_ID: usize = 26; +const MAX_HARNESS: usize = 8; +const MAX_WORKSPACE: usize = 32; + +impl Columns { + /// Measure the columns the tree needs, each capped. + fn measure(tree: &[HostRow]) -> Self { + let agents = || tree.iter().flat_map(|host| host.agents.iter()); + Columns { + id: agents() + .map(|agent| inline_text(&agent.agent_id).width()) + .max() + .unwrap_or(0) + .min(MAX_ID), + harness: agents() + .filter_map(|agent| agent.harness.as_deref()) + .map(|harness| inline_text(&harness.to_uppercase()).width()) + .max() + .unwrap_or(0) + .min(MAX_HARNESS), + workspace: agents() + .filter_map(|agent| agent.workspace.as_deref()) + .map(|workspace| inline_text(workspace).width()) + .max() + .unwrap_or(0) + .min(MAX_WORKSPACE), + } + } +} + +/// Pad `value` out to `width`, truncating with `…` when it does not fit. +/// +/// Measured in display columns, not bytes or `char`s: a CJK label is two +/// columns wide, and padding it by character count is what makes one row's +/// columns sit a cell to the left of every other row's. +fn cell(value: &str, width: usize) -> String { + let have = value.width(); + if have <= width { + return format!("{value}{}", " ".repeat(width - have)); + } + let mut out = String::new(); + let mut used = 0; + for c in value.chars() { + let w = c.to_string().width(); + if used + w > width.saturating_sub(1) { + break; + } + out.push(c); + used += w; + } + out.push('…'); + format!("{out}{}", " ".repeat(width.saturating_sub(used + 1))) +} + +/// Pad a path out to `width`, truncating from the *left* when it does not fit. +/// +/// The tail of a checkout path is what identifies it — `…/medulla/src/tui` +/// says which tree it is, `/Users/sanil/Projects/…` says only that it is one +/// of many under the same parent. +fn path_cell(value: &str, width: usize) -> String { + let have = value.width(); + if have <= width { + return format!("{value}{}", " ".repeat(width - have)); + } + let keep = width.saturating_sub(1); + let mut tail: Vec = Vec::new(); + let mut used = 0; + for c in value.chars().rev() { + let w = c.to_string().width(); + if used + w > keep { + break; + } + tail.push(c); + used += w; + } + tail.reverse(); + let tail: String = tail.into_iter().collect(); + format!("…{tail}{}", " ".repeat(width.saturating_sub(used + 1))) +} + /// An agent under its host: the id a dispatch targets, its harness, where it /// works, and the roles it is offered for. /// -/// Indented under the header, and marked when it is the manual default (`●`). -/// A declared agent the roster has no entry for is flagged rather than hidden: -/// it is why nothing is being dispatched to it. -fn agent_line(agent: &HostAgentRow) -> String { +/// Drawn as a branch of its host (`├─`, `└─` for the last one) with the fields +/// in fixed columns, so the shape of the fleet is readable down the page rather +/// than reconstructed from separators. Marked when it is the manual default +/// (`●`). A declared agent the roster has no entry for is flagged rather than +/// hidden: it is why nothing is being dispatched to it. +fn agent_line(agent: &HostAgentRow, last: bool, columns: &Columns) -> String { + let branch = if last { "└─" } else { "├─" }; let mark = if agent.selected { "●" } else { " " }; let harness = agent .harness .as_deref() - .map(|value| format!(" · {}", inline_text(&value.to_uppercase()))) + .map(|value| inline_text(&value.to_uppercase())) .unwrap_or_default(); let workspace = agent .workspace .as_deref() - .map(|value| format!(" · {}", inline_text(value))) + .map(inline_text) .unwrap_or_default(); let roles = match agent.roles.len() { 0 => String::new(), - 1 => " · 1 role".to_string(), - count => format!(" · {count} roles"), + 1 => "1 role".to_string(), + count => format!("{count} roles"), }; let state = match (agent.live, agent.declared) { - (false, _) => " · declared, not running", - (true, false) => " · undeclared", + (false, _) => "declared, not running", + (true, false) => "undeclared", (true, true) => "", }; + // The trailing columns are joined rather than padded — nothing lines up + // after them, and padding the last cell only adds trailing blanks that + // widen the selection highlight past the text. + let tail: Vec<&str> = [roles.as_str(), state] + .into_iter() + .filter(|part| !part.is_empty()) + .collect(); + let tail = match tail.is_empty() { + true => String::new(), + false => format!(" · {}", tail.join(" · ")), + }; format!( - " {mark} {}{harness}{workspace}{roles}{state}", - inline_text(&agent.agent_id) + " {branch} {mark} {} {} {}{tail}", + cell(&inline_text(&agent.agent_id), columns.id), + cell(&harness, columns.harness), + path_cell(&workspace, columns.workspace), ) + .trim_end() + .to_string() } /// A remote host reads dim: it is context, not something to act on. diff --git a/src/tui/src/ui/app/state.rs b/src/tui/src/ui/app/state.rs index 7b86f54b9..6868cef02 100644 --- a/src/tui/src/ui/app/state.rs +++ b/src/tui/src/ui/app/state.rs @@ -74,7 +74,6 @@ impl App { agent_scroll: 0, chat_scroll: 0, command_index: 0, - add_host_provider_cache: std::cell::OnceCell::new(), host_index: 0, host_roles_focus: false, host_role_index: 0, @@ -100,9 +99,6 @@ impl App { wf: Default::default(), #[cfg(feature = "workflows")] workflow_store_override: None, - add_host_kind: 0, - add_host_harness: 0, - add_host_kind_chosen: false, routing_index: 0, routing_focused: false, routing_strategy_index, diff --git a/src/tui/src/ui/app/types.rs b/src/tui/src/ui/app/types.rs index 112adb8ce..d0762c8ef 100644 --- a/src/tui/src/ui/app/types.rs +++ b/src/tui/src/ui/app/types.rs @@ -77,11 +77,11 @@ pub const TABS: [&str; 6] = [ /// what may be stood up there, how to add another, and how work is routed /// between them. /// -/// Only Workspaces is commented out, and only because Add Host › Local -/// supersedes it: an entry there was advisory routing context, whereas a local -/// host actually runs work in its directory. Its draw arm, keys and -/// `[host].workspaces` persistence all still build, so restoring it is putting -/// its name back here and renumbering. +/// Only Workspaces is commented out. An entry there was advisory routing +/// context; declaring an agent is what actually puts work in a directory, and +/// that is done from the host tree. Its draw arm, keys and `[host].workspaces` +/// persistence all still build, so restoring it is putting its name back here +/// and renumbering. pub const ROUTING_SUBPAGES: [&str; 5] = [ "Hosts", "Harness Types", @@ -146,44 +146,6 @@ pub(super) const SP_CONTEXT: usize = 6; pub(super) const SP_ACCOUNT: usize = 7; pub(super) const SP_HELP: usize = 8; -/// Which kind of host the Add Host page is collecting. -/// -/// The two differ in everything that matters — a remote is reached by address -/// over tiny.place, a local one by a directory on this machine — so asking -/// which first is what lets each ask only for what it needs. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum AddHostKind { - /// A directory on this machine, served in-process. - Local, - /// Another machine, reached by its tiny.place address. - Remote, -} - -impl AddHostKind { - /// The choices in the order they are offered. - pub const ALL: [AddHostKind; 2] = [AddHostKind::Local, AddHostKind::Remote]; - - /// The one-word name shown in the picker. - pub fn label(self) -> &'static str { - match self { - AddHostKind::Local => "Local", - AddHostKind::Remote => "Remote", - } - } - - /// What choosing this actually does, so the picker explains itself. - pub fn description(self) -> &'static str { - match self { - AddHostKind::Local => { - "a directory on this machine · runs in this process, watchable and typeable" - } - AddHostKind::Remote => { - "another machine · reached by its tiny.place address, needs a contact edge" - } - } - } -} - /// The index of a tab by name, or 0 if unknown. Keeps tab jumps robust as the tab /// list grows. pub(super) fn tab_pos(name: &str) -> usize { @@ -334,18 +296,16 @@ pub enum Cmd { Logout, /// Apply a worker fleet mutation. WorkerOp(WorkerOp), - /// Start a host on this device now, and register it with the hub. + /// Apply several fleet mutations as one operator action. /// - /// Carries the declaration rather than only an index into config: the - /// config is the app's, and the loop that can actually start a host is not. - /// `index` is the entry's position within `[[hosts]]`, which is the basis an - /// unnamed host's address is derived from at every other site. - StartLocalHost { - /// The host declaration to bind. - host: Box, - /// Its position within `[[hosts]]`. - index: usize, - }, + /// Removing a *host* is the case this exists for: a host is a group of + /// roster entries sharing an address, and the registry has no host-level + /// op — so taking one out means taking each of its agents out. Carrying + /// them together keeps that one keypress one status line, rather than N + /// racing "Worker registry updated" messages for what the operator did + /// once. They are applied in order, and a failure reports the op it + /// stopped on instead of being swallowed by the next success. + WorkerOps(Vec), /// Retarget the live screen subscription: stop watching one task, start /// watching another. Both halves ride one command so the change is atomic /// from the loop's point of view — a stop that landed without its start @@ -750,9 +710,6 @@ pub(super) enum PromptKind { CustomHarnessAdd, /// Edit the custom harness with the given stable id. CustomHarnessEdit(String), - /// The working directory for a new local host, with the harness already - /// chosen. Blank accepts the default — where this process is running. - LocalHostWorkspace(medulla::protocol::HarnessProvider), /// Reject a workflow proposal with the operator's explanation. RejectProposal { /// The workflow the proposal belongs to. @@ -916,14 +873,6 @@ pub struct App { pub(super) chat_scroll: usize, /// Selected row in the command peek, while it is open. pub(super) command_index: usize, - /// Installed harness types offered by the Add Host wizard, detected once. - /// - /// Detection reads the environment and stat-checks every provider binary on - /// `PATH`. The wizard asked on every render frame *and* every keypress, so a - /// page that is drawn at the frame rate was doing filesystem work to answer - /// a question whose answer cannot change while the process runs. - pub(super) add_host_provider_cache: - std::cell::OnceCell>, /// Selected row on the Routing Hosts page. pub(super) host_index: usize, /// Whether ↑↓ on the Hosts page drives the role toggles in the preview @@ -981,15 +930,6 @@ pub struct App { /// checkout. `None` resolves the layered store, as a real session does. #[cfg(feature = "workflows")] pub(super) workflow_store_override: Option>, - /// Which kind of host the Add Host page is offering — a cursor into - /// [`AddHostKind::ALL`]. - pub(super) add_host_kind: usize, - /// Which harness type a new local host will run — a cursor into the detected - /// provider list. - pub(super) add_host_harness: usize, - /// Whether the kind picker has been answered, so the arrows move on to the - /// harness-type list rather than re-picking local versus remote. - pub(super) add_host_kind_chosen: bool, /// The active Routing subpage (index into [`ROUTING_SUBPAGES`]). pub(super) routing_index: usize, /// Whether keyboard focus is inside the Routing content pane. diff --git a/src/tui/src/ui/app/workspaces.rs b/src/tui/src/ui/app/workspaces.rs index dbf5e0bfb..46c5bdc0f 100644 --- a/src/tui/src/ui/app/workspaces.rs +++ b/src/tui/src/ui/app/workspaces.rs @@ -152,90 +152,3 @@ fn absolute(path: &str) -> String { .map(|cwd| cwd.join(path).to_string_lossy().into_owned()) .unwrap_or_else(|_| path.to_string_lossy().into_owned()) } - -impl App { - /// Declare another host on this machine, working in `dir`. - /// - /// A blank `dir` accepts the default — the directory this process is - /// running in — because that is the common case and the one the operator - /// can see. Anything else is canonicalized so the declaration names a - /// place rather than whatever `medulla` happened to start in next time. - /// - /// Written to config *and* started now, so the host appears in the roster - /// the way a remote add does rather than waiting for the next launch. The - /// config write comes first and the start is only requested once it - /// succeeded — a host running against a declaration that was never saved - /// disappears on restart with nothing to explain it. - pub(in crate::ui::app) fn add_local_host( - &mut self, - harness: medulla::protocol::HarnessProvider, - dir: &str, - ) -> Option { - let dir = dir.trim(); - let workspace = if dir.is_empty() { - std::env::current_dir() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_default() - } else { - std::fs::canonicalize(dir) - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_else(|_| absolute(dir)) - }; - if self - .loaded - .config - .hosts - .iter() - .any(|h| h.workspace == workspace) - || self.loaded.config.host.workspace == workspace - { - self.set_status(format!("Already hosted · {workspace}")); - return None; - } - let name = std::path::Path::new(&workspace) - .file_name() - .map(|n| n.to_string_lossy().into_owned()) - .unwrap_or_default(); - let entry = medulla::config::HostSection { - name, - // Blank so the address is derived from the name at startup rather - // than inheriting the primary's and failing to bind. - address: String::new(), - workspace: workspace.clone(), - providers: vec![harness.as_str().to_string()], - default_provider: harness.as_str().to_string(), - ..medulla::config::HostSection::default() - }; - let Some(path) = self.config_path.clone() else { - self.set_status("No config file to save to"); - return None; - }; - // Persist a candidate list, and only adopt it once the write succeeded. - // Pushing first left a host in the in-process config that was never - // written and never started, and the duplicate check above then refused - // every retry for that directory — a failure the operator could not - // clear without restarting. - let mut hosts = self.loaded.config.hosts.clone(); - hosts.push(entry); - if let Err(e) = medulla::config::persist_local_hosts(&path, &hosts) { - self.set_status(format!("Local host was not saved: {e}")); - return None; - } - self.loaded.config.hosts = hosts; - // Same landing as a remote add, so both kinds end in the same place — - // and the host is started now rather than at the next launch, so the row - // appears here the way a remote one does. - self.focus_routing_subpage("Hosts"); - self.set_status(format!("Starting local host · {workspace}")); - let index = self.loaded.config.hosts.len().saturating_sub(1); - self.loaded - .config - .hosts - .last() - .cloned() - .map(|host| super::types::Cmd::StartLocalHost { - host: Box::new(host), - index, - }) - } -} diff --git a/src/tui/tests/feature_app_more/views.rs b/src/tui/tests/feature_app_more/views.rs index da484ee71..ef6eef340 100644 --- a/src/tui/tests/feature_app_more/views.rs +++ b/src/tui/tests/feature_app_more/views.rs @@ -11,11 +11,8 @@ fn hosts_add_prompt_emits_add_cmd_for_address() { let (mut app, _rt) = empty_app(); tab(&mut app, "Hosts"); app.focus_routing_subpage("Add Host"); - // Local leads the picker, so a remote add starts by choosing Remote — the - // page asks which kind before it asks for anything kind-specific, then a - // second confirm opens the address prompt. - let _ = app.on_event(key(KeyCode::Down)); - let _ = app.on_event(key(KeyCode::Char('a'))); + // The page adds a machine and nothing else, so there is no kind to choose: + // one confirm opens the address prompt. let _ = app.on_event(key(KeyCode::Char('a'))); let (title, _) = app.prompt_state().expect("add prompt open"); assert!(title.starts_with("Add host")); @@ -36,11 +33,8 @@ fn workers_add_prompt_handle_form() { let (mut app, _rt) = empty_app(); tab(&mut app, "Hosts"); app.focus_routing_subpage("Add Host"); - // Local leads the picker, so a remote add starts by choosing Remote — the - // page asks which kind before it asks for anything kind-specific, then a - // second confirm opens the address prompt. - let _ = app.on_event(key(KeyCode::Down)); - let _ = app.on_event(key(KeyCode::Char('a'))); + // The page adds a machine and nothing else, so there is no kind to choose: + // one confirm opens the address prompt. let _ = app.on_event(key(KeyCode::Char('a'))); type_str(&mut app, "@dev-2"); let cmd = app.on_event(key(KeyCode::Enter)); @@ -60,11 +54,8 @@ fn workers_add_prompt_empty_is_cancelled() { let (mut app, _rt) = empty_app(); tab(&mut app, "Hosts"); app.focus_routing_subpage("Add Host"); - // Local leads the picker, so a remote add starts by choosing Remote — the - // page asks which kind before it asks for anything kind-specific, then a - // second confirm opens the address prompt. - let _ = app.on_event(key(KeyCode::Down)); - let _ = app.on_event(key(KeyCode::Char('a'))); + // The page adds a machine and nothing else, so there is no kind to choose: + // one confirm opens the address prompt. let _ = app.on_event(key(KeyCode::Char('a'))); let cmd = app.on_event(key(KeyCode::Enter)); assert!(cmd.is_none()); diff --git a/src/tui/tests/feature_paste/composer.rs b/src/tui/tests/feature_paste/composer.rs index 0f7519cf7..202b0f171 100644 --- a/src/tui/tests/feature_paste/composer.rs +++ b/src/tui/tests/feature_paste/composer.rs @@ -79,10 +79,7 @@ fn an_open_prompt_overlay_takes_the_paste_flattened_to_one_line() { let mut app = App::new(Arc::new(MockRuntime::empty()), loaded()); app.tab_index = tab_index("Hosts"); app.focus_routing_subpage("Add Host"); - // Local leads the kind picker, so a remote add is Down then two confirms: - // the second opens the address prompt, which is the single-line field here. - let _ = app.on_event(key(KeyCode::Down)); - let _ = app.on_event(key(KeyCode::Char('a'))); + // One confirm opens the address prompt, which is the single-line field here. let _ = app.on_event(key(KeyCode::Char('a'))); assert!(app.prompt_state().is_some(), "the address prompt is open"); diff --git a/src/tui/tests/feature_workers/list.rs b/src/tui/tests/feature_workers/list.rs index 8cb8c28ec..75b53e153 100644 --- a/src/tui/tests/feature_workers/list.rs +++ b/src/tui/tests/feature_workers/list.rs @@ -212,12 +212,10 @@ fn hosts_r_refreshes_selected_machine_details() { fn add_host_page_renders_guidance_and_opens_the_prompt() { let mut app = app_with_workers(None); app.focus_routing_subpage("Add Host"); - // Local leads the picker; choosing Remote lights up the pairing guidance. - let _ = app.on_event(key(KeyCode::Down)); - let _ = app.on_event(key(KeyCode::Enter)); + // The page is the pairing guidance — there is no kind to choose first. let out = render(&mut app, 120, 40); - assert!(out.contains("Add a host for the orchestrator to delegate to.")); + assert!(out.contains("Connect another machine for the orchestrator to delegate to.")); assert!(out.contains("Example: 7Kx…9fQ")); assert!(!out.contains("@build-box")); @@ -229,13 +227,10 @@ fn add_host_page_renders_guidance_and_opens_the_prompt() { #[test] fn adding_a_remote_host_still_lands_on_the_tree() { - // The wizard is unchanged by the tree: choose Remote, confirm the pairing - // instructions, type the address, and the add op is emitted while the page - // returns to the list the add is about. + // Read the pairing instructions, Enter for the address prompt, type it, and + // the add op is emitted while the page returns to the list the add is about. let mut app = app_with_roster(Vec::new(), None); app.focus_routing_subpage("Add Host"); - let _ = app.on_event(key(KeyCode::Down)); // Local leads; Remote is second - let _ = app.on_event(key(KeyCode::Enter)); // settle the kind let _ = app.on_event(key(KeyCode::Enter)); // open the address prompt assert!(app.prompt_state().is_some(), "the address prompt opened"); for ch in "@build-box".chars() { @@ -467,41 +462,3 @@ fn hosts_e_opens_edit_label_prompt_prefilled() { other => panic!("expected Update, got {other:?}"), } } - -#[test] -fn a_local_host_that_could_not_be_saved_can_be_retried() { - // The entry used to be pushed into the in-process config before the write, - // so a failed save left a host that was never written and never started — - // and the duplicate check then refused every retry for that directory, - // which the operator could not clear without restarting. - let mut app = app_with_roster(Vec::new(), None); - let workspace = std::env::temp_dir().to_string_lossy().into_owned(); - - // Drive the wizard: Local is first, one Enter settles the harness step, the - // next opens the directory prompt. No config path is set on this app, so - // the save cannot succeed. - let add = |app: &mut App, workspace: &str| { - app.focus_routing_subpage("Add Host"); - let _ = app.on_event(key(KeyCode::Enter)); - let _ = app.on_event(key(KeyCode::Enter)); - assert!(app.prompt_state().is_some(), "the directory prompt opened"); - for ch in workspace.chars() { - let _ = app.on_event(key(KeyCode::Char(ch))); - } - app.on_event(key(KeyCode::Enter)) - }; - - assert!( - add(&mut app, &workspace).is_none(), - "nothing starts when nothing was saved" - ); - assert!(app.status().contains("No config file"), "{}", app.status()); - - // The second attempt must reach the same failure, not "Already hosted". - assert!(add(&mut app, &workspace).is_none()); - assert!( - !app.status().contains("Already hosted"), - "a failed save must not block the retry: {}", - app.status() - ); -} diff --git a/src/tui/tests/feature_workers/roles.rs b/src/tui/tests/feature_workers/roles.rs index 1fcf6c4f8..b1b1f1f80 100644 --- a/src/tui/tests/feature_workers/roles.rs +++ b/src/tui/tests/feature_workers/roles.rs @@ -129,10 +129,22 @@ fn removing_a_row_takes_the_arrows_back_off_the_role_list() { let _ = app.on_event(key(KeyCode::Char('d'))); - // Down now walks the tree again rather than the role list. + // Down now walks the tree again rather than the role list. Which agent it + // lands on is deliberately not asserted: removing an undeclared row writes + // the survivors down (they stop being seeded), and a newly declared agent + // sorts above the roster-only rows — so the row order after this keypress is + // not the one the cursor started in. What matters here is that the arrows + // are back on the tree, which is a preview of an agent rather than toggles + // being driven. + assert!( + !app.host_roles_focused(), + "the removal took the arrows off the toggles" + ); let _ = app.on_event(key(KeyCode::Down)); - let out = render(&mut app, 130, 44); - assert!(out.contains("Agent · w2"), "the cursor moved on: {out}"); + assert!( + !app.host_roles_focused(), + "and Down walked the tree rather than the role list" + ); } #[test] diff --git a/src/tui/tests/feature_workers/routing.rs b/src/tui/tests/feature_workers/routing.rs index 1a9ed3da4..1d11fec64 100644 --- a/src/tui/tests/feature_workers/routing.rs +++ b/src/tui/tests/feature_workers/routing.rs @@ -134,10 +134,7 @@ fn strategy_selection_persists_to_config_and_reloads_highlighted() { fn add_host_shows_the_line_to_run_on_the_machine_being_added() { let mut app = app_with_workers(None); app.focus_routing_subpage("Add Host"); - // The page asks which kind first; the pairing procedure belongs to Remote, - // and only becomes the live step once that kind is confirmed. - let _ = app.on_event(key(KeyCode::Down)); - let _ = app.on_event(key(KeyCode::Enter)); + // The pairing procedure is the page — nothing to choose before it. let out = render(&mut app, 160, 44); // The page is a procedure, not a definition: both halves of the pairing and // the two keys that drive them. @@ -155,18 +152,9 @@ fn add_host_copies_the_install_line_rather_than_asking_it_to_be_retyped() { let sink = app.capture_clipboard(); app.focus_routing_subpage("Add Host"); - // Local leads the picker, and the local flow has no install line — `c` - // there would advertise a key that copies nothing. - assert!(app.on_event(key(KeyCode::Char('c'))).is_none()); - assert!( - sink.lock().unwrap().is_empty(), - "nothing to copy on the local branch: {:?}", - sink.lock().unwrap() - ); - - // Arrowing to Remote puts the line on screen, which is exactly when the key - // means something. - let _ = app.on_event(key(KeyCode::Down)); + // The page adds a machine and nothing else, so the install line is always + // on screen and `c` always has something to give. It used to copy nothing + // until the operator had arrowed off the retired local kind. assert!(app.on_event(key(KeyCode::Char('c'))).is_none()); let copied = sink.lock().unwrap().clone(); assert_eq!(copied.len(), 1, "one copy: {copied:?}"); @@ -177,35 +165,3 @@ fn add_host_copies_the_install_line_rather_than_asking_it_to_be_retyped() { app.status() ); } - -#[test] -fn arrowing_after_a_confirmed_kind_does_not_carry_the_confirmation_across() { - // Confirming Remote and then arrowing to Local used to leave - // `add_host_kind_chosen` set, so the next Enter skipped "Choose a harness" - // and asked for a directory for a harness nobody had picked. - let mut app = app_with_workers(None); - app.focus_routing_subpage("Add Host"); - let _ = app.on_event(key(KeyCode::Down)); // Remote - let _ = app.on_event(key(KeyCode::Enter)); // confirm it - - let _ = app.on_event(key(KeyCode::Up)); // try to go back to Local - let out = render(&mut app, 160, 44); - assert!( - out.contains("On the machine you want to add"), - "a confirmed kind stays put: {out}" - ); - - // Esc is the way back, and it lands on the kind step rather than leaving. - let _ = app.on_event(key(KeyCode::Esc)); - let _ = app.on_event(key(KeyCode::Up)); - let _ = app.on_event(key(KeyCode::Enter)); - let out = render(&mut app, 160, 44); - assert!( - out.contains("Which harness type it runs"), - "local now reaches its harness-type step: {out}" - ); - assert!( - app.prompt_state().is_none(), - "and has not skipped to a prompt" - ); -}