diff --git a/workers-dev/README.md b/workers-dev/README.md index 1af948341..111763d00 100644 --- a/workers-dev/README.md +++ b/workers-dev/README.md @@ -30,7 +30,7 @@ Workers are **discovered automatically** from top-level `*/iii.worker.yaml` in t | Group | Workers | Started by | |-------|---------|------------| -| **harness stack** | The stack roots (`session-manager`, `llm-router`, `context-manager`, `provider-anthropic`, `provider-openai`, `approval-gate`, `harness`) **plus everything they transitively depend on**, derived live from each worker's `iii.worker.yaml` dependencies | `workers-dev up`, `Ctrl+u` in TUI, `workers-dev start` (starts the roots; missing deps are pulled in, connected ones left alone) | +| **current stack** (default: `harness`) | The stack's roots **plus everything they transitively depend on**, derived live from each worker's `iii.worker.yaml` dependencies. The built-in `harness` stack's roots are `session-manager`, `llm-router`, `context-manager`, `provider-anthropic`, `provider-openai`, `approval-gate`, `harness`; define more stacks (or override `harness`'s roots) in `workers-dev.yaml` or from the TUI (`Space` + `n`) — see Config below | `workers-dev up` / bare `workers-dev start` start the `default_stack` (missing deps are pulled in, connected ones left alone); `Ctrl+u` in the TUI starts it directly, or opens a stack picker when more than one stack is defined | | **other** | All remaining repo workers (e.g. `telegram-bot`, `console`, …) | `workers-dev start `, `workers-dev start --all`, `Ctrl+a` in TUI | Press `d` on any worker in the TUI to see its direct dependencies and its transitive dependents (the `r` restart blast radius), each with live status. @@ -42,9 +42,9 @@ A worker connected to the engine but not started by this `workers-dev` shows **P ## Usage ```bash -workers-dev up # start harness stack + TUI +workers-dev up # start default stack + TUI workers-dev # TUI only -workers-dev start # harness stack (CLI, waits for connect) +workers-dev start # default stack (CLI, waits for connect) workers-dev start --all # every discovered Rust worker workers-dev start telegram-bot # one worker (+ missing deps) workers-dev restart llm-router # rebuild + restart dependents @@ -52,7 +52,13 @@ workers-dev logs harness -f workers-dev status ``` -Starting a worker (CLI `start ` or `s` in the TUI) pulls in its dependencies, but a dependency **already connected to the engine is left running as-is** — no rebuild, no restart, no duplicate spawn. Explicitly requested workers always (re)start; use `restart` when a dependency itself needs a rebuild. The group commands count every member as explicitly requested: `up`, bare `start`, and `Ctrl+u` always restart the whole harness stack, `start --all` and `Ctrl+a` every managed Rust worker. +Stacks: define named stacks in `workers-dev.yaml`, or create one from the TUI +with `Space` + `n` (see Config below). `up` and bare `start` start the +`default_stack`; in the TUI, `Ctrl+u` opens a stack picker (Enter = switch the +dashboard's current stack + start it) when more than one stack is defined, and +starts the only stack directly otherwise. + +Starting a worker (CLI `start ` or `s` in the TUI) pulls in its dependencies, but a dependency **already connected to the engine is left running as-is** — no rebuild, no restart, no duplicate spawn. Explicitly requested workers always (re)start; use `restart` when a dependency itself needs a rebuild. The group commands count every member as explicitly requested: `up`, bare `start`, and `Ctrl+u` always restart the whole stack, `start --all` and `Ctrl+a` every managed Rust worker. Global flags: `--repo`, `--url`, `--port`, `--release`, `--config workers-dev.yaml`, `--stop-on-exit`, `--color auto|always|never`, `--ui-watch`. @@ -91,6 +97,8 @@ Use `--color never` or `NO_COLOR=1` to force plain output. Default `--color auto |-----|--------| | `↑`/`↓` (or `k`/`j`) | Select worker (skips group headers) | | `g`/`G` (or `Home`/`End`) | Jump to the first / last worker | +| `Space` | Mark the selected worker for a new stack | +| `n` | Name and save a new stack from the marked workers | | `s` | Start selected worker | | `x` | Stop selected worker | | `r` | Restart selected worker + dependents (confirm lists the blast radius with live status) | @@ -101,17 +109,19 @@ Use `--color never` or `NO_COLOR=1` to force plain output. Default `--color auto | `+`/`-` | Resize the log pane (drags the divider in two columns, the height when stacked) | | `/` | Filter workers by name (Enter applies, Esc clears) | | `e` | Start the iii engine (`iii -c harness/engine.config.yaml`) | -| `Ctrl+u` | Start harness stack | +| `Ctrl+u` | Start stack (picker when several stacks are defined; Enter = switch + start) (in the picker: `x` delete a stack, `*` make it the default) | | `Ctrl+a` | Start all managed Rust workers | | `?` | Toggle the key-reference overlay | | `q` | Quit | On a wide terminal the dashboard is a two-column **master/detail** layout: the worker list on the left (sized to fit its columns), the selected worker's logs filling the rest on the right, with `+`/`-` dragging the divider between them. Below ~100 columns the two panes stack vertically instead, and `+`/`-` trade height. -The header shows the repo's current git branch (`⎇ feat/my-branch`, refreshed live; detached HEAD shows as `@`) so side-by-side instances on different worktrees or checkouts are easy to tell apart — the terminal/tmux pane title is set to `workers-dev ⎇ ` too — plus an at-a-glance health summary (`●` connected, `◐` compiling, `✗` crashed, `○` stopped). When an engine status query fails the header flags `⚠ unreachable` and gains a line with the remedy (`press e to start the engine`) and the underlying error. The worker list's title shows the selection position (`Workers 3/48`). The log pane shows the **selected worker only**, scrollable through the full ring buffer, following the live tail by default. Crashed workers show their exit code inline. Lines are sanitized (no ANSI, no `\r` overwrite garbage). +The header shows the repo's current git branch (`⎇ feat/my-branch`, refreshed live; detached HEAD shows as `@`) so side-by-side instances on different worktrees or checkouts are easy to tell apart — the terminal/tmux pane title is set to `workers-dev ⎇ ` too — plus an at-a-glance health summary (`●` connected, `◐` compiling, `✗` crashed, `○` stopped). When an engine status query fails the header flags `⚠ unreachable` and gains a line with the remedy (`press e to start the engine`) and the underlying error. The worker list's title shows the selection position (`Workers 3/48`). Each group's header row reads `── stack: (N) ──` for the current stack and `── other (N) ──` for everything else, where N is the post-filter worker count. The log pane shows the **selected worker only**, scrollable through the full ring buffer, following the live tail by default. Crashed workers show their exit code inline. Lines are sanitized (no ANSI, no `\r` overwrite garbage). ## Config (`workers-dev.yaml`) +Auto-loaded from the repo root when present; `--config ` overrides. + ```yaml repo: /path/to/workers engine_url: ws://127.0.0.1:49134 @@ -120,14 +130,38 @@ workers: # optional override; default = all discovered - session-manager - llm-router - harness -harness_stack: # optional roots override (must be a subset of `workers`); - - session-manager # the dashboard's stack group is always these roots - - llm-router # plus their transitive dependencies - - harness + - console +stacks: # optional named stacks; values are stack ROOTS — + console: # the group shown/started is roots + transitive deps. + - console + - session-manager + harness: # overrides the built-in harness stack's roots + - session-manager + - llm-router + - harness +default_stack: console # started by `up` / bare `start`; default: harness color: auto # auto | always | never (respects NO_COLOR) ui_watch: false # start injectable-UI workers in watcher mode (pnpm watch + III__UI_WATCH=1) ``` +The built-in `harness` stack always exists. Deleting a `stacks.harness:` +override (`x` on it in the picker) removes it from that session's picker +until the next launch, when the built-in returns with its default roots. The +old `harness_stack:` key was replaced by `stacks:` + `default_stack:` and now +fails startup with a rename hint. + +Stacks can also be created from the TUI: mark workers with `Space`, press `n`, +name it, Enter. The stack is written into this file (comments and formatting +are preserved) and becomes the current stack immediately — it is *not* started; +press `Ctrl+u` when you want that. In the `Ctrl+u` picker, `x` deletes a stack +and `*` makes it the default. Deleting the default stack is refused — set +another default first. + +Note: `workers-dev.yaml` is not gitignored, so the first save leaves an +untracked file in the repo root. `workers-dev` writes the file by editing the +lines it owns; if `stacks:` is written inline (`stacks: {a: [b]}`) it refuses +to edit and says so. + ## Troubleshooting **Garbled log lines in the dashboard** diff --git a/workers-dev/src/commands/mod.rs b/workers-dev/src/commands/mod.rs index 0617bc6eb..ba1271695 100644 --- a/workers-dev/src/commands/mod.rs +++ b/workers-dev/src/commands/mod.rs @@ -1,18 +1,20 @@ use std::io::{self, Write}; use std::sync::Arc; -use anyhow::Result; +use anyhow::{bail, Result}; use crate::logs; use crate::orchestrator::Orchestrator; use crate::status; pub async fn run_status(orchestrator: &Orchestrator) -> Result<()> { - let (views, engine_error) = orchestrator.dashboard_snapshot().await; + let default_stack = &orchestrator.config.default_stack; + let members = orchestrator.stack_members(default_stack)?; + let (views, engine_error) = orchestrator.dashboard_snapshot(&members).await; if let Some(err) = engine_error { eprintln!("warning: engine unreachable: {err}"); } - status::print_status_table(&views); + status::print_status_table(&views, default_stack); Ok(()) } @@ -22,8 +24,9 @@ pub async fn run_start(orchestrator: &Orchestrator, workers: Vec, all: b orchestrator.start_all_managed(true).await?; println!("started all managed workers"); } else { - orchestrator.start_harness_stack(true).await?; - println!("started harness stack"); + let stack = orchestrator.config.default_stack.clone(); + orchestrator.start_stack(&stack, true).await?; + println!("started stack {stack}"); } } else { orchestrator.start_workers(&workers, true).await?; @@ -86,8 +89,81 @@ pub async fn run_logs( Ok(()) } +/// Roots of `stack`, refusing an empty set. Same message `start_roots` bails +/// with for the same condition — this is strictly an earlier checkpoint on +/// the same rule, so `run_up` can refuse before `ensure_engine()` rather than +/// spawning the engine (an external side effect) for a stack that cannot +/// start anything. +fn startable_stack_roots(orchestrator: &Orchestrator, stack: &str) -> Result> { + let roots = orchestrator.stack_roots(stack)?; + if roots.is_empty() { + bail!("stack {stack} has no startable workers"); + } + Ok(roots) +} + pub async fn run_up(orchestrator: Arc) -> Result<()> { + let stack = orchestrator.config.default_stack.clone(); + startable_stack_roots(&orchestrator, &stack)?; orchestrator.ensure_engine().await?; - orchestrator.start_harness_stack(false).await?; + orchestrator.start_stack(&stack, false).await?; crate::tui::run(orchestrator).await } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::Config; + + /// Minimal `Orchestrator` with no discovered workers — `WorkerGraph::load` + /// never touches the filesystem when `workers` is empty, so this needs no + /// repo fixture, and (more importantly) no reachable engine: the guard + /// under test must fire without either. + fn orchestrator_with_stacks( + default_stack: &str, + stacks: Vec<(String, Vec)>, + ) -> Orchestrator { + let config = Config { + repo_root: std::path::PathBuf::new(), + config_path: std::path::PathBuf::new(), + engine_url: crate::config::DEFAULT_ENGINE_URL.to_string(), + release: false, + poll_interval_ms: crate::config::DEFAULT_POLL_INTERVAL_MS, + connect_timeout_ms: crate::config::DEFAULT_CONNECT_TIMEOUT_MS, + workers: Vec::new(), + stacks, + default_stack: default_stack.to_string(), + worker_specs: Vec::new(), + stop_on_exit: false, + color_mode: Default::default(), + ui_watch: false, + }; + Orchestrator::new(config, false).unwrap() + } + + /// The regression `run_up` must never reopen: an empty default stack has + /// to be rejected here, synchronously and with no engine contact, so the + /// caller (`run_up`) never reaches `ensure_engine()` for a stack that + /// cannot start anything. `start_roots` carries the same guard for the + /// actual start; this pins the earlier checkpoint independently since + /// `run_up` itself always ends by handing off to the TUI and so can't be + /// exercised end-to-end in a unit test. + #[test] + fn startable_stack_roots_rejects_an_empty_default_stack() { + let orch = orchestrator_with_stacks("ghost", vec![("ghost".to_string(), Vec::new())]); + let err = startable_stack_roots(&orch, "ghost").unwrap_err(); + assert!(err.to_string().contains("ghost"), "{err:#}"); + } + + #[test] + fn startable_stack_roots_allows_a_nonempty_stack() { + let orch = orchestrator_with_stacks( + "harness", + vec![("harness".to_string(), vec!["harness".to_string()])], + ); + assert_eq!( + startable_stack_roots(&orch, "harness").unwrap(), + vec!["harness".to_string()] + ); + } +} diff --git a/workers-dev/src/config.rs b/workers-dev/src/config.rs index 032e09285..2284e3859 100644 --- a/workers-dev/src/config.rs +++ b/workers-dev/src/config.rs @@ -4,9 +4,7 @@ use anyhow::{bail, Context, Result}; use serde::Deserialize; use crate::color::ColorMode; -use crate::discover::{ - assign_groups, discover_repo_workers, harness_stack_names, order_worker_names, WorkerSpec, -}; +use crate::discover::{discover_repo_workers, harness_stack_names, order_worker_names, WorkerSpec}; pub const DEFAULT_ENGINE_URL: &str = "ws://127.0.0.1:49134"; pub const DEFAULT_POLL_INTERVAL_MS: u64 = 2000; @@ -21,12 +19,25 @@ pub const DEFAULT_LOG_TAIL: usize = 100; #[derive(Debug, Clone)] pub struct Config { pub repo_root: PathBuf, + /// The config file this run reads and writes: `--config` when given, else + /// `/workers-dev.yaml`. Set even when the file does not exist, + /// so the TUI's first save knows where to create it. + pub config_path: PathBuf, pub engine_url: String, pub release: bool, pub poll_interval_ms: u64, pub connect_timeout_ms: u64, pub workers: Vec, - pub harness_stack: Vec, + /// Ordered named stacks: `(name, roots)`. Index 0 is always the built-in + /// `harness` stack (roots overridable via `stacks.harness:`), then file + /// stacks in YAML order. Roots are filtered to managed workers. + pub stacks: Vec<(String, Vec)>, + /// Name of the stack `up` / bare `start` / Ctrl+u start; always present + /// in `stacks`, but its roots can be empty (e.g. a `workers:` allowlist + /// that excludes every root) — `load` only warns about that, so a + /// read-only command can still open. Anything that actually starts it + /// goes through `Orchestrator::start_roots`, which refuses on empty roots. + pub default_stack: String, pub worker_specs: Vec, pub stop_on_exit: bool, pub color_mode: ColorMode, @@ -45,7 +56,11 @@ struct FileConfig { poll_interval_ms: Option, connect_timeout_ms: Option, workers: Option>, - harness_stack: Option>, + /// Removed key. Kept in the struct only so its presence can be rejected + /// with a rename hint — serde would otherwise silently ignore it. + harness_stack: Option, + stacks: Option, + default_stack: Option, stop_on_exit: Option, color: Option, ui_watch: Option, @@ -63,33 +78,87 @@ impl Config { color: Option, ui_watch: bool, ) -> Result { - let file_cfg = if let Some(path) = config_path { - let raw = std::fs::read_to_string(&path) - .with_context(|| format!("read config file {}", path.display()))?; - serde_yaml::from_str(&raw) - .with_context(|| format!("parse config file {}", path.display()))? - } else { - FileConfig::default() + // `loaded_path` is the file `file_cfg` actually came from, captured at + // the point of loading rather than reconstructed afterwards — a + // loaded file's `repo:` key can redirect the final `repo_root` + // elsewhere (honored as-is, see below), so rebuilding the path from + // that final root could name a file in a different repo than the one + // that was read. + let (file_cfg, loaded_path) = match &config_path { + // `--config` may be relative (resolved against the process cwd); + // the auto-probed path below is always absolute. Nothing in this + // crate changes cwd, so a relative `config_path` is stable, but + // don't assume the field is always absolute. + Some(path) => (load_file_config(path)?, Some(path.clone())), + None => { + // No --config: auto-load /workers-dev.yaml when present. + // The probe resolves the root without a config file (--repo / env / + // cwd ancestors); if that fails, fall through to defaults and let + // the final resolve_repo_root below report the real error. An + // auto-loaded file's `repo:` key is honored as-is — no re-search + // for another config in the new root. + match resolve_repo_root(repo.clone()) { + Ok(root) => { + let path = root.join("workers-dev.yaml"); + if path.is_file() { + (load_file_config(&path)?, Some(path)) + } else { + (FileConfig::default(), None) + } + } + Err(_) => (FileConfig::default(), None), + } + } }; let repo_root = resolve_repo_root(repo.or(file_cfg.repo))?; - let mut worker_specs = discover_repo_workers(&repo_root)?; + // `loaded_path` is `None` only when no file was read at all (explicit + // `--config` and a found auto-load both set it above) — a default + // `FileConfig` carries no `repo:` to redirect with, so + // `repo.or(file_cfg.repo)` here is just `repo`, the same input the + // probe above already resolved. `repo_root` is therefore necessarily + // the probed root, so reconstructing the path from it is safe. + let config_path = loaded_path.unwrap_or_else(|| repo_root.join("workers-dev.yaml")); + let worker_specs = discover_repo_workers(&repo_root)?; if worker_specs.is_empty() { bail!("no workers discovered under {}", repo_root.display()); } - // Resolve the stack roots before deriving worker order: an overridden - // `harness_stack:` changes the roots, and the dashboard's stack group - // (roots + transitive deps) must be regrouped before the display order - // is captured below. Filtered against the managed `workers` list further - // down — a root outside it would make `up`/`Ctrl+u` bail at start. - let stack_roots = match file_cfg.harness_stack { - Some(roots) => { - assign_groups(&mut worker_specs, &roots); - roots + // Resolve stacks before deriving worker order: the default stack's + // roots drive grouping. Root names are validated against the managed + // `workers` list further down. + if file_cfg.harness_stack.is_some() { + bail!( + "config key `harness_stack:` was replaced by `stacks:` + `default_stack:` — \ + rename to stacks: {{harness: [...]}}" + ); + } + let mut stacks: Vec<(String, Vec)> = + vec![("harness".to_string(), harness_stack_names(&worker_specs))]; + if let Some(mapping) = file_cfg.stacks { + for (name, roots) in parse_stacks(mapping)? { + match stacks.iter_mut().find(|(n, _)| *n == name) { + // `stacks.harness:` overrides the builtin roots in place, + // keeping harness at index 0. + Some(entry) => entry.1 = roots, + None => stacks.push((name, roots)), + } } - None => harness_stack_names(&worker_specs), - }; + } + let default_stack = file_cfg + .default_stack + .unwrap_or_else(|| "harness".to_string()); + ensure_no_control_chars(&default_stack)?; + if !stacks.iter().any(|(n, _)| *n == default_stack) { + bail!( + "default_stack {default_stack:?} is not a defined stack (have: {})", + stacks + .iter() + .map(|(n, _)| n.as_str()) + .collect::>() + .join(", ") + ); + } let raw_engine_url = engine_url .or(file_cfg.engine_url) @@ -133,21 +202,33 @@ impl Config { // Keep only stack roots that survived into the managed `workers` set: // the WorkerGraph is built over `workers`, so a root outside it makes - // `workers-dev up` / `Ctrl+u` bail with "unknown worker" before the TUI - // even opens. Drop with a warning, mirroring the `workers:` handling. + // stack starts bail with "unknown worker". Drop with a warning, + // mirroring the `workers:` handling. let managed: std::collections::HashSet<&str> = workers.iter().map(String::as_str).collect(); - let harness_stack: Vec = stack_roots - .into_iter() - .filter(|w| { + for (stack_name, roots) in &mut stacks { + roots.retain(|w| { let ok = managed.contains(w.as_str()); if !ok { eprintln!( - "warning: skipping harness_stack worker {w}: not in the managed workers list" + "warning: skipping stack {stack_name} worker {w}: not in the managed workers list" ); } ok - }) - .collect(); + }); + } + let default_is_empty = stacks + .iter() + .find(|(n, _)| *n == default_stack) + .is_none_or(|(_, roots)| roots.is_empty()); + if default_is_empty { + // Not a bail: `status`, `logs`, and the TUI are all read-only and + // must still be able to open so the user can see why (and fix + // it). `Orchestrator::start_roots` carries the same emptiness + // check and is what actually refuses to start this stack. + eprintln!( + "warning: default stack {default_stack:?} has no startable workers after validation" + ); + } let color_mode = color .or(file_cfg.color) @@ -164,6 +245,7 @@ impl Config { Ok(Self { repo_root, + config_path, engine_url, release: release || file_cfg.release.unwrap_or(false), poll_interval_ms: file_cfg @@ -173,7 +255,8 @@ impl Config { .connect_timeout_ms .unwrap_or(DEFAULT_CONNECT_TIMEOUT_MS), workers, - harness_stack, + stacks, + default_stack, worker_specs, stop_on_exit: stop_on_exit || file_cfg.stop_on_exit.unwrap_or(false), color_mode, @@ -186,6 +269,72 @@ impl Config { } } +fn load_file_config(path: &Path) -> Result { + let raw = std::fs::read_to_string(path) + .with_context(|| format!("read config file {}", path.display()))?; + serde_yaml::from_str(&raw).with_context(|| format!("parse config file {}", path.display())) +} + +/// Parse `text` far enough to catch the mistakes a write must refuse before +/// they reach disk — NOT a full replica of `load`: there is no repo here to +/// discover workers against, so the managed-`workers:`/stack-roots +/// filtering and warn-and-drop steps `load` runs afterward never happen in +/// this function. Two things still must be checked here, because both would +/// otherwise validate fine and only fail on the *next* launch: +/// - Deserializing into `FileConfig` alone isn't enough: `stacks:` lands +/// there as an untyped mapping, so a bare-scalar key (`123`, `true`, +/// `null`, ...) validates fine even though `parse_stacks` — what `load` +/// actually runs — rejects it for not being a string. +/// - A `default_stack:` naming a stack that doesn't exist passes +/// `FileConfig`'s own deserialization (it's just a `String`) and then +/// bricks the very next `load`. +pub fn validate_config_text(text: &str) -> Result<()> { + let cfg = serde_yaml::from_str::(text).context("parse edited config")?; + let stacks = match cfg.stacks { + Some(stacks) => parse_stacks(stacks)?, + None => Vec::new(), + }; + if let Some(default) = &cfg.default_stack { + if default != "harness" && !stacks.iter().any(|(n, _)| n == default) { + bail!("default_stack {default:?} is not a defined stack"); + } + } + Ok(()) +} + +/// Parse the `stacks:` mapping preserving YAML order (serde_yaml::Mapping is +/// insertion-ordered). Every value must be a list of worker-name strings. +fn parse_stacks(mapping: serde_yaml::Mapping) -> Result)>> { + let mut stacks = Vec::new(); + for (key, value) in mapping { + let name = key + .as_str() + .with_context(|| format!("stacks: key {key:?} is not a string"))? + .to_string(); + ensure_no_control_chars(&name)?; + let roots: Vec = serde_yaml::from_value(value) + .with_context(|| format!("stacks.{name}: expected a list of worker names"))?; + stacks.push((name, roots)); + } + Ok(stacks) +} + +/// Refuse a stack name carrying control characters (`\e`/`\x1b` and friends). +/// `workers-dev.yaml` auto-loads from the repo root, so a hostile or careless +/// config's stack name reaches every consumer that prints it — `status`'s +/// group headers, the TUI — as raw bytes (see status::group_label). Checked +/// once here, at load, rather than escaped at each render site, so nothing +/// downstream needs its own opinion on what's safe to print. A stack name +/// legitimately needs only letters, digits, `-` and `_` (`valid_stack_name` +/// in config_write.rs governs what this tool ever *writes*); a control +/// character is never one a person typed on purpose. +fn ensure_no_control_chars(name: &str) -> Result<()> { + if name.chars().any(char::is_control) { + bail!("stack name {name:?} contains control characters — not a valid stack name"); + } + Ok(()) +} + pub fn resolve_repo_root(explicit: Option) -> Result { if let Some(path) = explicit { let canonical = path @@ -291,4 +440,287 @@ mod tests { assert_eq!(host, "127.0.0.1"); assert_eq!(port, 49134); } + + use std::path::PathBuf; + use tempfile::TempDir; + + /// Minimal discoverable repo: one rust/binary worker. + fn write_repo(tmp: &TempDir) { + let dir = tmp.path().join("harness"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("iii.worker.yaml"), + "iii: v1\nname: harness\nlanguage: rust\ndeploy: binary\ndescription: test\n", + ) + .unwrap(); + std::fs::write(dir.join("Cargo.toml"), "[workspace]\n").unwrap(); + } + + fn load(tmp: &TempDir, config_path: Option) -> Result { + Config::load( + Some(tmp.path().to_path_buf()), + None, + None, + false, + config_path, + false, + None, + false, + ) + } + + #[test] + fn auto_loads_workers_dev_yaml_from_repo_root() { + let tmp = TempDir::new().unwrap(); + write_repo(&tmp); + std::fs::write( + tmp.path().join("workers-dev.yaml"), + "engine_url: ws://127.0.0.1:55555\n", + ) + .unwrap(); + let cfg = load(&tmp, None).unwrap(); + assert_eq!(cfg.engine_url, "ws://127.0.0.1:55555"); + } + + #[test] + fn absent_config_file_means_defaults() { + let tmp = TempDir::new().unwrap(); + write_repo(&tmp); + let cfg = load(&tmp, None).unwrap(); + assert_eq!(cfg.engine_url, DEFAULT_ENGINE_URL); + } + + #[test] + fn explicit_config_beats_auto_load() { + let tmp = TempDir::new().unwrap(); + write_repo(&tmp); + std::fs::write( + tmp.path().join("workers-dev.yaml"), + "engine_url: ws://127.0.0.1:55555\n", + ) + .unwrap(); + let other = tmp.path().join("elsewhere.yaml"); + std::fs::write(&other, "engine_url: ws://127.0.0.1:44444\n").unwrap(); + let cfg = load(&tmp, Some(other)).unwrap(); + assert_eq!(cfg.engine_url, "ws://127.0.0.1:44444"); + } + + #[test] + fn config_path_points_at_the_auto_loaded_file_even_when_absent() { + let tmp = TempDir::new().unwrap(); + write_repo(&tmp); + let cfg = load(&tmp, None).unwrap(); + assert_eq!(cfg.config_path, tmp.path().join("workers-dev.yaml")); + } + + /// An auto-loaded file's `repo:` key redirects `repo_root` elsewhere, but + /// `config_path` must still name the file that was actually read, not a + /// path rebuilt from the (now different) final `repo_root`. + /// + /// No `--repo` here — `repo: None` is required to let `file_cfg.repo` + /// reach the final `resolve_repo_root` call at all (an explicit `--repo` + /// would win via `.or()` regardless of what the file says). That means + /// the auto-load probe itself needs `WORKERS_DEV_REPO` to find `tmp` + /// instead of falling through to cwd / CARGO_MANIFEST_DIR, which would + /// hit this crate's own real repo. No other test reads that env var, so + /// this is safe under `cargo test`'s default parallelism today — but it + /// would race a future test that also resolves with `repo: None`. + #[test] + fn config_path_points_at_the_loaded_file_even_when_repo_key_redirects() { + let tmp = TempDir::new().unwrap(); + write_repo(&tmp); + let other_repo = TempDir::new().unwrap(); + write_repo(&other_repo); + std::fs::write( + tmp.path().join("workers-dev.yaml"), + format!("repo: {}\n", other_repo.path().display()), + ) + .unwrap(); + + unsafe { + std::env::set_var("WORKERS_DEV_REPO", tmp.path()); + } + let result = Config::load(None, None, None, false, None, false, None, false); + unsafe { + std::env::remove_var("WORKERS_DEV_REPO"); + } + let cfg = result.unwrap(); + + // The redirect really did take effect... + assert_eq!(cfg.repo_root, other_repo.path()); + // ...but config_path must still be the file read from `tmp`, not a + // `workers-dev.yaml` reconstructed under `other_repo`. + assert_eq!(cfg.config_path, tmp.path().join("workers-dev.yaml")); + } + + #[test] + fn config_path_follows_an_explicit_config_flag() { + let tmp = TempDir::new().unwrap(); + write_repo(&tmp); + let other = tmp.path().join("elsewhere.yaml"); + std::fs::write(&other, "release: false\n").unwrap(); + let cfg = load(&tmp, Some(other.clone())).unwrap(); + assert_eq!(cfg.config_path, other); + } + + #[test] + fn validate_config_text_accepts_and_rejects() { + assert!(validate_config_text("stacks:\n a:\n - b\n").is_ok()); + assert!(validate_config_text("stacks:\n a:\n - b\n bad\n").is_err()); + } + + /// Important fix: a `default_stack:` naming a stack that doesn't exist + /// used to pass verification (it's just a `String` to `FileConfig`) and + /// only fail on the *next* `Config::load` — after `write_verified` had + /// already replaced the user's file. + #[test] + fn validate_config_text_rejects_a_dangling_default_stack() { + let err = + validate_config_text("stacks:\n a:\n - b\ndefault_stack: nope\n").unwrap_err(); + assert!(err.to_string().contains("not a defined stack"), "{err:#}"); + + // The built-in `harness` stack always exists, even with no `stacks:` + // key in the file at all. + assert!(validate_config_text("default_stack: harness\n").is_ok()); + assert!(validate_config_text("stacks:\n a:\n - b\ndefault_stack: a\n").is_ok()); + } + + /// Repo with enough workers to define non-trivial stacks. + fn write_repo_multi(tmp: &TempDir) { + for name in ["harness", "session-manager", "console"] { + let dir = tmp.path().join(name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("iii.worker.yaml"), + format!( + "iii: v1\nname: {name}\nlanguage: rust\ndeploy: binary\ndescription: test\n" + ), + ) + .unwrap(); + std::fs::write(dir.join("Cargo.toml"), "[workspace]\n").unwrap(); + } + } + + fn load_with_yaml(tmp: &TempDir, yaml: &str) -> Result { + std::fs::write(tmp.path().join("workers-dev.yaml"), yaml).unwrap(); + load(tmp, None) + } + + #[test] + fn stacks_parse_with_builtin_harness_first() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let cfg = load_with_yaml( + &tmp, + "stacks:\n console: [console, session-manager]\ndefault_stack: console\n", + ) + .unwrap(); + assert_eq!(cfg.default_stack, "console"); + let names: Vec<&str> = cfg.stacks.iter().map(|(n, _)| n.as_str()).collect(); + assert_eq!(names, vec!["harness", "console"]); + // Built-in harness roots = HARNESS_STACK const filtered to discovered. + assert_eq!(cfg.stacks[0].1, vec!["session-manager", "harness"]); + assert_eq!(cfg.stacks[1].1, vec!["console", "session-manager"]); + } + + #[test] + fn stacks_keep_yaml_definition_order() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let cfg = load_with_yaml( + &tmp, + "stacks:\n zebra: [console]\n alpha: [session-manager]\n", + ) + .unwrap(); + let names: Vec<&str> = cfg.stacks.iter().map(|(n, _)| n.as_str()).collect(); + assert_eq!(names, vec!["harness", "zebra", "alpha"]); + assert_eq!(cfg.default_stack, "harness"); + } + + #[test] + fn stacks_harness_entry_overrides_builtin_roots() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let cfg = load_with_yaml(&tmp, "stacks:\n harness: [console]\n").unwrap(); + assert_eq!(cfg.stacks.len(), 1); + assert_eq!(cfg.stacks[0].0, "harness"); + assert_eq!(cfg.stacks[0].1, vec!["console"]); + } + + #[test] + fn unknown_default_stack_fails() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let err = load_with_yaml(&tmp, "default_stack: nope\n").unwrap_err(); + assert!(err.to_string().contains("not a defined stack"), "{err:#}"); + } + + #[test] + fn removed_harness_stack_key_fails_with_rename_hint() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let err = load_with_yaml(&tmp, "harness_stack:\n - harness\n").unwrap_err(); + assert!(err.to_string().contains("replaced by `stacks:`"), "{err:#}"); + } + + #[test] + fn unknown_stack_roots_warn_and_drop() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let cfg = load_with_yaml(&tmp, "stacks:\n console: [console, bogus]\n").unwrap(); + let console = cfg.stacks.iter().find(|(n, _)| n == "console").unwrap(); + assert_eq!(console.1, vec!["console"]); + } + + /// A default stack left with no startable workers (e.g. a `workers:` + /// allowlist that excludes every root) must still let `load` succeed — + /// only warn — so read-only commands (`status`, `logs`, the TUI) can + /// open. `Orchestrator::start_roots` is what refuses to actually start + /// it (see orchestrator.rs's `start_roots_refuses_an_empty_root_list`). + #[test] + fn empty_default_stack_after_filtering_warns_but_still_loads() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let cfg = load_with_yaml(&tmp, "stacks:\n ghost: [bogus]\ndefault_stack: ghost\n") + .expect("a load-time warning must not block a read-only command"); + assert_eq!(cfg.default_stack, "ghost"); + let ghost = cfg.stacks.iter().find(|(n, _)| n == "ghost").unwrap(); + assert!(ghost.1.is_empty(), "{:?}", ghost.1); + } + + #[test] + fn non_list_stack_value_fails() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let err = load_with_yaml(&tmp, "stacks:\n console: 5\n").unwrap_err(); + assert!( + err.to_string().contains("expected a list of worker names"), + "{err:#}" + ); + } + + /// `workers-dev.yaml` auto-loads from the repo root, so a stack key + /// carrying an ANSI escape (or any other control character) must never + /// reach `status`/the TUI, which print stack names unescaped. A + /// double-quoted YAML scalar can smuggle one in via `\x1b`; `parse_stacks` + /// must refuse it at load instead of letting it through to render time. + #[test] + fn parse_stacks_rejects_a_control_character_key() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let err = + load_with_yaml(&tmp, "stacks:\n \"a\\x1b[31mconsole\": [console]\n").unwrap_err(); + assert!(err.to_string().contains("control characters"), "{err:#}"); + } + + /// Same rule, on `default_stack:` — it never goes through `parse_stacks` + /// (it's a bare `String` field on `FileConfig`), so it needs its own call + /// to the same check. + #[test] + fn default_stack_rejects_a_control_character_name() { + let tmp = TempDir::new().unwrap(); + write_repo_multi(&tmp); + let err = load_with_yaml(&tmp, "default_stack: \"a\\x1b[31m\"\n").unwrap_err(); + assert!(err.to_string().contains("control characters"), "{err:#}"); + } } diff --git a/workers-dev/src/config_write.rs b/workers-dev/src/config_write.rs new file mode 100644 index 000000000..04fe46ff7 --- /dev/null +++ b/workers-dev/src/config_write.rs @@ -0,0 +1,1121 @@ +//! Editing `workers-dev.yaml` in place. +//! +//! The file is hand-written as often as it is tool-written, so writes are +//! line surgery on the original text rather than a serde round-trip: comments, +//! key order, and blank lines all survive because they are never re-serialized. +//! Anything the scanner cannot own confidently (inline `stacks: {…}`, a +//! duplicated key) is refused rather than rewritten. + +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; + +use anyhow::{bail, Context, Result}; + +/// Stack names land in YAML as plain scalars, so restrict them to characters +/// that can never need quoting or change the document's shape. +pub fn valid_stack_name(name: &str) -> bool { + !name.is_empty() + && name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') +} + +/// `valid_stack_name` on its own still admits a *leading* `-` (`-weird`, or +/// bare `-`): `-` has to stay allowed as a character at all, or the TUI's +/// name prompt — which filters one keystroke at a time via +/// `valid_stack_name(&c.to_string())` and so can't see *position* — would +/// block typing `console-dev`. But a name starting with `-` collides with a +/// YAML block-sequence indicator closely enough that the line-surgery +/// scanner can't always tell the two apart from text alone (see +/// `is_list_item`); letting this tool ever *write* one risks the exact +/// swallowing bug Critical-1 fixed, this time self-inflicted. So the +/// leading-dash rule lives here instead, as a whole-name check called only +/// where a name is about to be written into the file — `upsert_stack` and +/// `set_default_stack`. `remove_stack` targets a name that must already +/// exist to do anything, and `ensure_block_entries_recognized` already +/// refuses to touch a block it can't fully parse regardless of which entry +/// in it you're after, so it doesn't need this check too. +fn ensure_writable_name(name: &str) -> Result<()> { + if !valid_stack_name(name) { + bail!("invalid stack name {name:?} (use letters, digits, - and _)"); + } + if name.starts_with('-') { + bail!("invalid stack name {name:?} (can't start with -)"); + } + Ok(()) +} + +/// Insert or replace `name`'s entry under `stacks:`, leaving every other byte +/// of the file alone. +pub fn upsert_stack(text: &str, name: &str, roots: &[String]) -> Result { + ensure_writable_name(name)?; + let lines: Vec<&str> = text.split('\n').collect(); + let Some(head) = top_level_key(&lines, "stacks")? else { + let mut out = ensure_trailing_newline(text); + out.push_str("stacks:\n"); + for line in render_entry(" ", name, roots) { + out.push_str(&line); + out.push('\n'); + } + return Ok(out); + }; + ensure_block_style(&lines, head)?; + let block = block_range(&lines, head); + ensure_block_entries_recognized(&lines, block)?; + let entry = render_entry(&block_indent(&lines, block), name, roots); + let mut out: Vec = lines.iter().map(|l| l.to_string()).collect(); + match entry_range(&lines, block, name) { + Some((start, end)) => out.splice(start..end, entry), + None => out.splice(block.1..block.1, entry), + }; + Ok(out.join("\n")) +} + +/// Drop `name`'s entry. Removing the last entry drops the `stacks:` key too, +/// rather than leaving a dangling header. +pub fn remove_stack(text: &str, name: &str) -> Result { + let lines: Vec<&str> = text.split('\n').collect(); + let missing = || format!("stack {name} is not defined in this file"); + let Some(head) = top_level_key(&lines, "stacks")? else { + bail!(missing()); + }; + ensure_block_style(&lines, head)?; + let block = block_range(&lines, head); + ensure_block_entries_recognized(&lines, block)?; + let Some((start, end)) = entry_range(&lines, block, name) else { + bail!(missing()); + }; + let mut out: Vec = lines.iter().map(|l| l.to_string()).collect(); + out.drain(start..end); + + // Only the header itself is left to drop when the removed entry WAS the + // entire block — if anything else was in it (a sibling entry, or just a + // comment the user left behind), `stacks:` must stay as that content's + // header rather than leaving it dangling with nothing above it. + if (start, end) == block { + out.drain(head..head + 1); + } + Ok(out.join("\n")) +} + +/// Point `default_stack:` at `name`, replacing the existing key or appending it. +pub fn set_default_stack(text: &str, name: &str) -> Result { + ensure_writable_name(name)?; + let lines: Vec<&str> = text.split('\n').collect(); + let Some(i) = top_level_key(&lines, "default_stack")? else { + return Ok(format!( + "{}default_stack: {name}\n", + ensure_trailing_newline(text) + )); + }; + let mut out: Vec = lines.iter().map(|l| l.to_string()).collect(); + let cr = if lines[i].ends_with('\r') { "\r" } else { "" }; + let comment = trailing_comment(strip_cr(lines[i])); + out[i] = format!("default_stack: {name}{comment}{cr}"); + Ok(out.join("\n")) +} + +fn strip_cr(line: &str) -> &str { + line.strip_suffix('\r').unwrap_or(line) +} + +/// The trailing ` # comment` on a `default_stack:` line, including its +/// leading whitespace so the user's alignment survives a rewrite — or `""` +/// if there's none. YAML only starts a comment at a `#` that sits at the +/// start of the line or is immediately preceded by `is_yaml_space`; a `#` +/// glued to the value (`console#nospacecomment`), preceded by some other +/// Unicode space, or inside a quoted scalar (`"my#stack"`) is part of the +/// VALUE, not a comment. This module's own header says the file is +/// hand-written as often as tool-written, so any of those shapes can +/// already be sitting on this line — misreading one as a comment would +/// silently rewrite that value into garbage instead of discarding it with +/// the rest of the old line. (A byte, not `is_comment` above: this looks +/// for the hash anywhere in the line, not just after leading indentation, +/// so `is_yaml_space(bytes[i - 1] as char)` is the same predicate applied +/// at a single position rather than to a whole line's worth of leading +/// whitespace — `u8 as char` is exact for the ASCII byte values `#`, ` ` +/// and `\t` this comparison ever touches.) +fn trailing_comment(line: &str) -> &str { + let bytes = line.as_bytes(); + let Some(hash) = (0..bytes.len()) + .find(|&i| bytes[i] == b'#' && (i == 0 || is_yaml_space(bytes[i - 1] as char))) + else { + return ""; + }; + let ws_start = line[..hash] + .rfind(|c: char| !is_yaml_space(c)) + .map_or(0, |i| i + 1); + &line[ws_start..] +} + +fn ensure_trailing_newline(text: &str) -> String { + if text.is_empty() || text.ends_with('\n') { + text.to_string() + } else { + format!("{text}\n") + } +} + +/// Line index of a top-level `key:`. Errors when the key appears twice — +/// which one wins is a question for a human, not for line surgery. +fn top_level_key(lines: &[&str], key: &str) -> Result> { + let mut found = None; + for (i, line) in lines.iter().enumerate() { + let line = strip_cr(line); + let Some(rest) = line.strip_prefix(key) else { + continue; + }; + if !rest.starts_with(':') { + continue; + } + if found.is_some() { + bail!("`{key}:` appears more than once in this file — fix it by hand"); + } + found = Some(i); + } + Ok(found) +} + +/// Refuse a `stacks: {…}` written inline; only block style is ours to edit. +/// A trailing comment after the colon (`stacks: # my dev stacks`) is valid +/// block style, not inline content. +fn ensure_block_style(lines: &[&str], head: usize) -> Result<()> { + let after = strip_cr(lines[head]) + .split_once(':') + .map(|(_, rest)| rest.trim()) + .unwrap_or(""); + if !after.is_empty() && !after.starts_with('#') { + bail!("`stacks:` is written inline in this file — edit it by hand"); + } + Ok(()) +} + +/// True for a blank line or a comment line at any column — YAML comments +/// aren't bound to the surrounding block's indentation, so one test serves +/// both "does this end the block" and "does this belong to whatever line +/// follows it" (`block_range` and `leading_gap_run`). Keeping both callers +/// on this one function is deliberate: they drifted apart once already +/// (`block_range` went column-agnostic before `leading_gap_run` did, which +/// is exactly how a comment ended up silently deleted). +/// +/// Blank-ness is checked the same YAML-space-only way as `is_comment` +/// (below), not Rust's Unicode `trim`: a line that's only NBSP isn't blank +/// to YAML, it's a one-character plain scalar. +fn is_blank_or_comment(line: &str) -> bool { + line.trim_start_matches(is_yaml_space).is_empty() || is_comment(line) +} + +/// Half-open line range of the indented block under `head`. Blank lines and +/// comment lines (YAML allows comments at any column) don't end it; trailing +/// blanks are left outside. +fn block_range(lines: &[&str], head: usize) -> (usize, usize) { + let start = head + 1; + let mut end = start; + for (i, line) in lines.iter().enumerate().skip(start) { + let line = strip_cr(line); + if is_blank_or_comment(line) { + continue; + } + if line.starts_with(' ') || line.starts_with('\t') { + end = i + 1; + } else { + break; + } + } + (start, end) +} + +/// `(" ", "console")` for a ` console:` line; None for list items and blanks. +/// +/// Deliberately looser than `valid_stack_name`: that governs what this tool +/// may *write*, not what an existing entry may be named. `Config::load` (via +/// `parse_stacks`) accepts any string key — `my.stack`, `'quoted'`, `tiny :`, +/// `web:dev` — so a scanner that only recognised `valid_stack_name` shapes +/// would go blind on everything else, silently swallowing it into a +/// neighbouring entry's range or, if it's the last entry left, making +/// `remove_stack` drop the whole `stacks:` block out from under it. A key is +/// only rejected here for shapes that line surgery genuinely cannot own: +/// empty, a list item (`-` prefixed, or no key-ending colon at all), or +/// stray whitespace around the name (`tiny :`) that +/// `ensure_block_entries_recognized` then refuses to edit past rather than +/// misreading. +/// +/// The key ends at the first `:` YAML would end it at — one followed by +/// whitespace or end-of-line — not just the first `:` byte. A colon glued to +/// the next character is ordinary scalar content (`web:dev:` is the single +/// key `web:dev`, YAML only treats the *second* colon as the terminator); +/// stopping at the first `:` unconditionally truncated any such name, so +/// `entry_range` matched it against the wrong (shorter) name entirely — +/// Critical-1's exact swallowing failure, reached through name extraction +/// instead of line classification. +/// +/// A comment line is never a key, even one shaped like one (`# TODO: add +/// console` has a `:` followed by whitespace, same as a real entry). +/// Recognizing it as key `# TODO` used to cut a preceding entry's range +/// short right before it — Critical-1's swallowing failure again, this time +/// because this function had no opinion on comments at all rather than the +/// wrong one. +fn entry_key(line: &str) -> Option<(String, String)> { + if is_comment(line) { + return None; + } + let indent: String = line.chars().take_while(|c| *c == ' ').collect(); + if indent.is_empty() || indent.len() == line.len() { + return None; + } + let rest = &line[indent.len()..]; + let (end, _) = rest + .char_indices() + .find(|&(i, c)| c == ':' && rest[i + 1..].chars().next().is_none_or(is_yaml_space))?; + let key = &rest[..end]; + if key.is_empty() || key.starts_with('-') || key.trim() != key { + return None; + } + Some((indent, key.to_string())) +} + +fn indent_len(line: &str) -> usize { + line.chars().take_while(|c| *c == ' ').count() +} + +/// YAML's `s-white`: space and tab, nothing else. Rust's `char::is_whitespace` +/// (Unicode `White_Space`) is a strict superset — it's also true for NBSP +/// (U+00A0) and several other Unicode spaces that YAML does *not* treat as +/// separators. `entry_key` and `is_list_item` both used to check +/// `char::is_whitespace` where YAML's own grammar calls for `s-white`, so a +/// colon or dash followed by one of those extra codepoints was misread the +/// same way an ASCII byte-for-byte mismatch already was: a colon followed by +/// NBSP looked like a key terminator (truncating a name like `a:␠b`, ␠=NBSP, +/// to `a`), and a dash followed by NBSP looked like a real list-item +/// indicator (letting a name like `-␠weird` slip past `entry_key` +/// unrefused). Optionally-recognized YAML line breaks (U+0085, U+2028, +/// U+2029) are deliberately NOT included: nothing here has ever needed them, +/// and treating a name using one of them as ordinary content just means this +/// scanner refuses instead of losing data — the safe direction already. +fn is_yaml_space(c: char) -> bool { + c == ' ' || c == '\t' +} + +/// True when `line`, once only `is_yaml_space` indentation is stripped from +/// the front, starts with `#` — YAML's own comment rule: a `#` preceded by +/// nothing but `s-white`, or nothing at all (comments aren't bound to the +/// surrounding block's indentation, so this is checked at any column, not +/// just `entry_depth`). The one place every predicate in this module that +/// needs to answer "is this line a comment" routes through, on purpose: +/// `is_blank_or_comment`, `entry_key` and `trailing_comment` used to each +/// carry their own opinion (Unicode `trim_start`, no comment-awareness at +/// all, and `u8::is_ascii_whitespace` respectively) — and disagreeing was +/// exactly how a comment-shaped line went invisible to some of them while +/// another read it as an ordinary key. +fn is_comment(line: &str) -> bool { + line.trim_start_matches(is_yaml_space).starts_with('#') +} + +/// True when `line`'s first non-space character is a real YAML +/// block-sequence indicator: a `-` followed by `s-white`, or a `-` alone +/// at the end of the line. Per YAML, that trailing whitespace requirement is +/// what makes `-` a sequence indicator at all — `-weird` (no space after the +/// dash) is an ordinary plain scalar, so `-weird:` is a valid, if unusual, +/// key. A looser check would misread it as a list item and let it slide past +/// `entry_key` unrefused in `ensure_block_entries_recognized` below — the +/// exact swallowing bug Critical-1 fixed, reopened through a shape that fix +/// never looked at. +/// +/// The leading trim is `is_yaml_space`-based too, not Rust's Unicode +/// `trim_start` — the same fix `is_comment`/`is_blank_or_comment` needed: +/// stripping NBSP as if it were real indentation makes a line like +/// `- weird: v` look like an ordinary same-level list item, when the +/// dash there isn't a sequence indicator at all (nothing valid precedes +/// it), so the real key is `- weird`. +fn is_list_item(line: &str) -> bool { + match line.trim_start_matches(is_yaml_space).strip_prefix('-') { + Some(rest) => rest.is_empty() || rest.starts_with(is_yaml_space), + None => false, + } +} + +/// Refuse a `stacks:` block containing a line that is neither a recognized +/// entry key (`entry_key`, at the block's own indentation) nor a list item +/// at or under one — e.g. `tiny :` (space before the colon), which +/// `entry_key` correctly declines to recognize but which would otherwise +/// read as an ordinary line neither `upsert_stack` nor `remove_stack` can +/// see. Layer (a) (`entry_key` above) widened what gets recognized; this is +/// layer (b), for the shapes even that still can't own — refusing is always +/// better than mangling, which is this module's stated contract. +/// +/// A list item is recognized at the key's own indentation too, not only +/// strictly deeper: YAML allows a block sequence to sit at the same column +/// as its key (`tiny:` / `- session-manager` both at column 2), and several +/// YAML writers emit exactly that by default. Only a key line is pinned to +/// exactly `entry_depth` — a key can't itself be indented deeper than its +/// siblings without starting a nested mapping this module doesn't understand. +fn ensure_block_entries_recognized(lines: &[&str], block: (usize, usize)) -> Result<()> { + let Some(entry_depth) = (block.0..block.1) + .map(|i| strip_cr(lines[i])) + .find(|line| !is_blank_or_comment(line)) + .map(indent_len) + else { + return Ok(()); // Empty block: nothing to misread. + }; + for &line in &lines[block.0..block.1] { + let line = strip_cr(line); + if is_blank_or_comment(line) { + continue; + } + let recognized = if is_list_item(line) { + indent_len(line) >= entry_depth + } else { + indent_len(line) == entry_depth && entry_key(line).is_some() + }; + if !recognized { + bail!( + "`stacks:` entry {line:?} isn't a name or a list item this tool can read — \ + edit it by hand" + ); + } + } + Ok(()) +} + +/// Indentation the block's entries already use, or two spaces for a new block. +fn block_indent(lines: &[&str], block: (usize, usize)) -> String { + (block.0..block.1) + .find_map(|i| entry_key(strip_cr(lines[i])).map(|(indent, _)| indent)) + .unwrap_or_else(|| " ".to_string()) +} + +/// Half-open line range of `name`'s entry: its key line through the line +/// before the next entry at the same indentation (or the end of the block). +fn entry_range(lines: &[&str], block: (usize, usize), name: &str) -> Option<(usize, usize)> { + let mut start: Option<(String, usize)> = None; + for i in block.0..block.1 { + let Some((indent, key)) = entry_key(strip_cr(lines[i])) else { + continue; + }; + match &start { + None if key == name => start = Some((indent, i)), + None => {} + Some((open_indent, open)) if indent.len() <= open_indent.len() => { + return Some((*open, leading_gap_run(lines, i, open + 1))); + } + Some(_) => {} + } + } + start.map(|(_, open)| (open, block.1)) +} + +/// Back `at` up over a run of blank lines and comment lines — at any column, +/// same rule as `block_range` — stopping no earlier than `floor` (never the +/// entry's own key line: callers pass `floor = open + 1`). A blank or +/// comment line immediately above a key reads as part of that key's own +/// entry, not trailing content of the entry before it, so both must stay +/// out of that entry's replace/remove range. +/// +/// Deliberate, not an oversight: this can only ever push a comment onto the +/// *following* entry, never delete one. A comment that trails the entry +/// being replaced/removed (e.g. a note on `tiny`'s last line, right before +/// `console:`) ends up orphaned onto `console` instead — visible and still +/// valid YAML, unlike silently deleting the user's writing. +fn leading_gap_run(lines: &[&str], at: usize, floor: usize) -> usize { + let mut at = at; + while at > floor && is_blank_or_comment(strip_cr(lines[at - 1])) { + at -= 1; + } + at +} + +fn render_entry(indent: &str, name: &str, roots: &[String]) -> Vec { + let mut out = vec![format!("{indent}{name}:")]; + for root in roots { + out.push(format!("{indent}{indent}- {root}")); + } + out +} + +/// Replace `path`'s contents with `text`, but only after proving the result +/// still loads. Writes `text` to a sibling temp file — named with this +/// process's pid so two `workers-dev` instances editing the same config +/// never share, or delete, each other's temp file — flushed and fsynced +/// before the rename over the target, so a crash mid-write really cannot +/// leave a half-written config behind. +pub fn write_verified(path: &Path, text: &str) -> Result<()> { + crate::config::validate_config_text(text)?; + let tmp = temp_sibling(path); + write_synced(&tmp, text) + .inspect_err(|_| { + let _ = std::fs::remove_file(&tmp); + }) + .with_context(|| format!("write {}", tmp.display()))?; + std::fs::rename(&tmp, path) + .inspect_err(|_| { + let _ = std::fs::remove_file(&tmp); + }) + .with_context(|| format!("replace {} with {}", path.display(), tmp.display()))?; + Ok(()) +} + +/// `path`'s temp-file sibling for `write_verified`, unique per process: a +/// fixed `.tmp` name would let two `workers-dev` instances writing the +/// same config collide, with the error path of one instance's write even +/// `remove_file`-ing the other's in-flight temp file. +fn temp_sibling(path: &Path) -> PathBuf { + let name = path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| "workers-dev.yaml".to_string()); + path.with_file_name(format!("{name}.{}.tmp", std::process::id())) +} + +/// Write `text` to `tmp`, flushed and synced to disk before returning. +/// `std::fs::write` alone only guarantees the bytes reached the OS page +/// cache, not the disk — a crash between that return and `write_verified`'s +/// rename could still lose the write, which is exactly what that function's +/// doc comment promises can't happen. +fn write_synced(tmp: &Path, text: &str) -> io::Result<()> { + let mut file = std::fs::File::create(tmp)?; + file.write_all(text.as_bytes())?; + file.flush()?; + file.sync_all() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn roots(names: &[&str]) -> Vec { + names.iter().map(|s| s.to_string()).collect() + } + + /// Every text this module produces must still parse as YAML. + fn parses(text: &str) -> bool { + serde_yaml::from_str::(text).is_ok() + } + + #[test] + fn creates_stacks_block_in_an_empty_file() { + let out = upsert_stack("", "console", &roots(&["console", "session-manager"])).unwrap(); + assert_eq!( + out, + "stacks:\n console:\n - console\n - session-manager\n" + ); + assert!(parses(&out)); + } + + /// A file with other keys keeps them, and the block is appended. + #[test] + fn appends_block_to_a_file_without_stacks() { + let src = "# my dev config\nengine_url: ws://127.0.0.1:49134\nrelease: false\n"; + let out = upsert_stack(src, "tiny", &roots(&["session-manager"])).unwrap(); + assert!(out.starts_with(src), "existing content must be untouched"); + assert!(out.ends_with("stacks:\n tiny:\n - session-manager\n")); + assert!(parses(&out)); + } + + /// The whole point: comments around the edited entry survive. + #[test] + fn replaces_an_entry_and_preserves_comments() { + let src = "\ +stacks: + # the console loop + console: + - console + # everything else + tiny: + - session-manager +default_stack: tiny +"; + let out = upsert_stack(src, "console", &roots(&["console", "state"])).unwrap(); + assert!(out.contains("# the console loop")); + assert!(out.contains("# everything else")); + assert!(out.contains(" console:\n - console\n - state\n")); + assert!(out.contains(" tiny:\n - session-manager\n")); + assert!(out.contains("default_stack: tiny")); + assert!(parses(&out)); + } + + #[test] + fn inserts_a_new_entry_at_the_end_of_the_block() { + let src = "stacks:\n tiny:\n - session-manager\ncolor: auto\n"; + let out = upsert_stack(src, "console", &roots(&["console"])).unwrap(); + assert_eq!( + out, + "stacks:\n tiny:\n - session-manager\n console:\n - console\ncolor: auto\n" + ); + assert!(parses(&out)); + } + + #[test] + fn upsert_is_idempotent() { + let src = "stacks:\n tiny:\n - session-manager\n"; + let once = upsert_stack(src, "console", &roots(&["console"])).unwrap(); + let twice = upsert_stack(&once, "console", &roots(&["console"])).unwrap(); + assert_eq!(once, twice); + } + + #[test] + fn removes_one_entry_and_keeps_the_others() { + let src = "stacks:\n tiny:\n - session-manager\n console:\n - console\n"; + let out = remove_stack(src, "tiny").unwrap(); + assert_eq!(out, "stacks:\n console:\n - console\n"); + assert!(parses(&out)); + } + + /// Removing the last entry drops the now-empty `stacks:` key. + #[test] + fn removing_the_last_entry_drops_the_stacks_key() { + let src = "engine_url: ws://x:1\nstacks:\n tiny:\n - session-manager\n"; + let out = remove_stack(src, "tiny").unwrap(); + assert_eq!(out, "engine_url: ws://x:1\n"); + assert!(parses(&out)); + } + + /// Sibling of the above: when a comment is still sitting in the block, + /// dropping `name`'s entry must NOT also drop the `stacks:` header out + /// from under it — that would leave the comment as a dangling, un-headed + /// indented line. This module's contract is to never destroy what the + /// user wrote, and unlike an entry, a bare comment can never make + /// `has_entry`-style detection see the block as non-empty on its own. + #[test] + fn removing_the_last_entry_keeps_the_header_when_a_comment_remains() { + let src = "stacks:\n # the console loop\n console:\n - console\n"; + let out = remove_stack(src, "console").unwrap(); + assert_eq!(out, "stacks:\n # the console loop\n"); + assert!(parses(&out)); + } + + #[test] + fn remove_reports_an_unknown_stack() { + let src = "stacks:\n tiny:\n - session-manager\n"; + let err = remove_stack(src, "nope").unwrap_err(); + assert!( + err.to_string().contains("not defined in this file"), + "{err:#}" + ); + let err = remove_stack("release: false\n", "tiny").unwrap_err(); + assert!( + err.to_string().contains("not defined in this file"), + "{err:#}" + ); + } + + #[test] + fn sets_default_in_place_or_appends_it() { + let replaced = set_default_stack("default_stack: tiny\ncolor: auto\n", "console").unwrap(); + assert_eq!(replaced, "default_stack: console\ncolor: auto\n"); + let appended = set_default_stack("color: auto\n", "console").unwrap(); + assert_eq!(appended, "color: auto\ndefault_stack: console\n"); + assert!(parses(&replaced) && parses(&appended)); + // No comment on the line: unchanged behavior, covered above already. + } + + /// A trailing comment on `default_stack:` — and the whitespace the user + /// chose before it — must survive a value change, not just the block + /// comments `upsert_stack`/`remove_stack` already protect. + #[test] + fn set_default_preserves_a_trailing_comment_and_its_spacing() { + let out = set_default_stack("default_stack: tiny # my usual loop\n", "console").unwrap(); + assert_eq!(out, "default_stack: console # my usual loop\n"); + assert!(parses(&out)); + } + + /// Same as above, on a CRLF line: the comment must survive AND the line + /// must still end with `\r`, not just one or the other. + #[test] + fn set_default_preserves_a_trailing_comment_on_a_crlf_line() { + let out = set_default_stack( + "default_stack: tiny # my usual loop\r\ncolor: auto\r\n", + "console", + ) + .unwrap(); + assert_eq!( + out, + "default_stack: console # my usual loop\r\ncolor: auto\r\n" + ); + assert!(parses(&out)); + } + + /// A `#` glued directly to the value (no preceding whitespace) isn't a + /// YAML comment — it's part of the plain scalar. The whole old value + /// must be discarded with the rest of the line, not misread as a + /// comment and re-emitted after the new name. + #[test] + fn set_default_does_not_mistake_a_glued_hash_for_a_comment() { + let out = set_default_stack("default_stack: console#nospacecomment\n", "tiny").unwrap(); + assert_eq!(out, "default_stack: tiny\n"); + assert!(parses(&out)); + } + + /// Same failure mode, via a `#` inside a quoted value instead of glued + /// to a plain one. + #[test] + fn set_default_does_not_mistake_a_hash_inside_a_quoted_value_for_a_comment() { + let out = set_default_stack("default_stack: \"my#stack\"\n", "tiny").unwrap(); + assert_eq!(out, "default_stack: tiny\n"); + assert!(parses(&out)); + } + + /// Inline/flow style is not ours to edit — refuse instead of mangling it. + #[test] + fn refuses_inline_stacks_mapping() { + let src = "stacks: {tiny: [session-manager]}\n"; + let err = upsert_stack(src, "console", &roots(&["console"])).unwrap_err(); + assert!(err.to_string().contains("inline"), "{err:#}"); + } + + #[test] + fn refuses_a_duplicated_stacks_key() { + let src = "stacks:\n a:\n - x\nstacks:\n b:\n - y\n"; + let err = upsert_stack(src, "c", &roots(&["z"])).unwrap_err(); + assert!(err.to_string().contains("more than once"), "{err:#}"); + } + + #[test] + fn refuses_names_that_would_need_quoting() { + let err = upsert_stack("", "my stack", &roots(&["x"])).unwrap_err(); + assert!(err.to_string().contains("invalid stack name"), "{err:#}"); + assert!(valid_stack_name("console-dev_2")); + assert!(!valid_stack_name("")); + assert!(!valid_stack_name("a:b")); + } + + /// CRLF files stay CRLF on the lines we don't touch. + #[test] + fn preserves_crlf_line_endings() { + let src = "color: auto\r\nstacks:\r\n tiny:\r\n - session-manager\r\n"; + let out = upsert_stack(src, "console", &roots(&["console"])).unwrap(); + assert!(out.starts_with("color: auto\r\n")); + assert!(out.contains(" tiny:\r\n")); + assert!(out.contains(" console:\n - console\n")); + } + + /// YAML comments are legal at any column. A column-0 comment between two + /// entries must not truncate block detection, or an entry past it goes + /// invisible: upsert would then append a duplicate key instead of + /// replacing the existing one, and remove would report it missing. + #[test] + fn sees_past_a_column_zero_comment_inside_the_block() { + let src = "stacks:\n console:\n - console\n# a stray column-0 comment\n tiny:\n - session-manager\n"; + + let out = upsert_stack(src, "tiny", &roots(&["y"])).unwrap(); + assert_eq!(out.matches("tiny:").count(), 1, "{out:?}"); + assert!(out.contains("# a stray column-0 comment")); + assert!(parses(&out)); + + let out = remove_stack(src, "tiny").unwrap(); + assert!(!out.contains("tiny"), "{out:?}"); + assert!(out.contains("# a stray column-0 comment")); + assert!(parses(&out)); + } + + /// A blank line separating two entries is layout, not part of either + /// entry's content, on both the replace and the remove path. + #[test] + fn preserves_a_blank_line_between_entries() { + let src = "stacks:\n tiny:\n - session-manager\n\n console:\n - console\n"; + + let out = upsert_stack(src, "tiny", &roots(&["x"])).unwrap(); + assert!(out.contains("- x\n\n console:"), "{out:?}"); + assert!(parses(&out)); + + let out = remove_stack(src, "tiny").unwrap(); + assert!(out.contains("stacks:\n\n console:"), "{out:?}"); + assert!(parses(&out)); + } + + /// A trailing comment after `stacks:` is still block style, not inline. + #[test] + fn accepts_a_trailing_comment_on_the_stacks_header() { + let src = "stacks: # my dev stacks\n"; + let out = upsert_stack(src, "console", &roots(&["x"])).unwrap(); + assert!(out.starts_with("stacks: # my dev stacks\n")); + assert!(out.contains(" console:\n - x\n")); + assert!(parses(&out)); + } + + /// Sibling of `sees_past_a_column_zero_comment_inside_the_block`: that one + /// edits the entry AFTER the mismatched-indent comment, this one edits + /// the entry BEFORE it. Both directions must leave the comment intact — + /// it must never fall inside the range being replaced or removed just + /// because its column doesn't match the entry that follows it. + #[test] + fn preserves_a_column_zero_comment_when_editing_the_entry_before_it() { + let src = "stacks:\n console:\n - console\n# a stray column-0 comment\n tiny:\n - session-manager\n"; + + let out = upsert_stack(src, "console", &roots(&["console", "state"])).unwrap(); + assert!(out.contains("# a stray column-0 comment"), "{out:?}"); + assert!(parses(&out)); + + let out = remove_stack(src, "console").unwrap(); + assert!(out.contains("# a stray column-0 comment"), "{out:?}"); + assert!(parses(&out)); + } + + /// A run mixing blank lines and an oddly-indented comment between two + /// entries must survive as a unit, in order, on both operations. + #[test] + fn preserves_mixed_blanks_and_comments_between_entries() { + let src = + "stacks:\n tiny:\n - session-manager\n\n# stray note\n console:\n - console\n"; + + let out = upsert_stack(src, "tiny", &roots(&["x"])).unwrap(); + assert!(out.contains("- x\n\n# stray note\n console:"), "{out:?}"); + assert!(parses(&out)); + + let out = remove_stack(src, "tiny").unwrap(); + assert!( + out.contains("stacks:\n\n# stray note\n console:"), + "{out:?}" + ); + assert!(parses(&out)); + } + + /// Critical fix: `entry_key` used to gate recognition on + /// `valid_stack_name`, so a sibling entry named `my.stack` — fully + /// supported by `Config::load`/`parse_stacks`, just not a name this tool + /// would ever write itself — was invisible to the scanner and got + /// swallowed into the entry actually being edited. + #[test] + fn upsert_stack_does_not_swallow_a_dotted_sibling_key() { + let src = "stacks:\n tiny:\n - session-manager\n my.stack:\n - console\n"; + let out = upsert_stack(src, "tiny", &roots(&["console"])).unwrap(); + assert_eq!( + out, + "stacks:\n tiny:\n - console\n my.stack:\n - console\n" + ); + assert!(parses(&out)); + } + + /// Same fix, on the remove path — this used to drop `my.stack` right + /// along with `tiny`, which combined with a `default_stack: my.stack` + /// elsewhere in the file left the result unable to load at all. + #[test] + fn remove_stack_does_not_swallow_a_dotted_sibling_key() { + let src = "stacks:\n tiny:\n - session-manager\n my.stack:\n - console\ncolor: auto\ndefault_stack: my.stack\n"; + let out = remove_stack(src, "tiny").unwrap(); + assert_eq!( + out, + "stacks:\n my.stack:\n - console\ncolor: auto\ndefault_stack: my.stack\n" + ); + assert!(parses(&out)); + } + + /// No edit may ever leave the file empty when the input had entries — + /// the sharpest form of the swallowed-sibling bug: removing `tiny` used + /// to take `my.stack` down with it, and then the now-seemingly-empty + /// `stacks:` key too, truncating the whole file to 0 bytes. + #[test] + fn remove_stack_never_empties_the_file_when_a_sibling_entry_remains() { + let src = "stacks:\n my.stack:\n - console\n tiny:\n - session-manager\n"; + let out = remove_stack(src, "tiny").unwrap(); + assert!(!out.is_empty(), "{out:?}"); + assert!(out.contains("my.stack"), "{out:?}"); + assert!(parses(&out)); + } + + /// A shape `entry_key` still can't recognize even once widened (`tiny :`, + /// a stray space before the colon) must refuse the whole edit rather than + /// mangle a block it can't fully see — layer (b) of the fix, and the + /// backstop for whatever layer (a) still misses. + #[test] + fn refuses_a_stacks_block_with_an_unrecognizable_entry() { + let src = "stacks:\n tiny :\n - session-manager\n console:\n - console\n"; + let err = upsert_stack(src, "console", &roots(&["console", "state"])).unwrap_err(); + assert!(err.to_string().contains("tiny"), "{err:#}"); + + let err = remove_stack(src, "console").unwrap_err(); + assert!(err.to_string().contains("tiny"), "{err:#}"); + } + + /// Regression in the guard above: a list item indented at the SAME level + /// as its own key (`tiny:` / `- session-manager` both at column 2) is + /// valid YAML and the default emit style of several YAML writers — the + /// guard must accept a list item at or under its key's depth, not only + /// strictly under it. + #[test] + fn accepts_a_list_item_indented_level_with_its_key() { + let src = "stacks:\n tiny:\n - session-manager\n console:\n - console\n"; + + let out = upsert_stack(src, "tiny", &roots(&["session-manager", "state"])).unwrap(); + assert_eq!( + out, + "stacks:\n tiny:\n - session-manager\n - state\n console:\n - console\n" + ); + assert!(parses(&out)); + + let out = remove_stack(src, "tiny").unwrap(); + assert_eq!(out, "stacks:\n console:\n - console\n"); + assert!(parses(&out)); + } + + /// Regression opened by the `>` → `>=` widening above: `-` only starts a + /// YAML block sequence when followed by whitespace or end-of-line, so + /// `-weird:` (no space after the dash) is an ordinary plain-scalar key, + /// not a list item. The widened dash check didn't require that + /// whitespace, so at entry depth it now misread `-weird:` as a list item + /// and let it slide past `entry_key` unrefused — reopening Critical-1's + /// swallowing bug through a shape the guard was never asked to look at. + #[test] + fn refuses_a_dash_led_key_misread_as_a_list_item() { + let src = "stacks:\n tiny:\n - session-manager\n -weird:\n - console\n"; + let err = upsert_stack(src, "tiny", &roots(&["x"])).unwrap_err(); + assert!(err.to_string().contains("weird"), "{err:#}"); + + let err = remove_stack(src, "tiny").unwrap_err(); + assert!(err.to_string().contains("weird"), "{err:#}"); + } + + /// Defense in depth for the same regression: even once the guard (above) + /// refuses to misread an existing `-weird:`, this tool must never be the + /// one to *write* a leading-dash name in the first place — `-` has to + /// stay a valid character (`console-dev` is a completely ordinary name, + /// pinned below), just not the first one. + #[test] + fn refuses_writing_a_stack_name_that_starts_with_a_dash() { + let err = upsert_stack("", "-weird", &roots(&["x"])).unwrap_err(); + assert!(err.to_string().contains("can't start with -"), "{err:#}"); + + let err = set_default_stack("", "-weird").unwrap_err(); + assert!(err.to_string().contains("can't start with -"), "{err:#}"); + + let out = upsert_stack("", "console-dev", &roots(&["console"])).unwrap(); + assert!(out.contains("console-dev"), "{out:?}"); + } + + /// `entry_key` ended a key at the FIRST `:`, but YAML ends a plain-scalar + /// key at the first `:` followed by whitespace or end-of-line — a colon + /// glued to the next character (`web:dev:`) is just scalar content. A + /// stack literally named `web:dev` had its extracted "key" truncated to + /// `web`, so upserting an unrelated new stack named `web` matched (and + /// swallowed) it instead of appending beside it. + #[test] + fn upsert_leaves_a_colon_containing_sibling_key_intact() { + let src = "stacks:\n console:\n - console\n web:dev:\n - session-manager\n"; + let out = upsert_stack(src, "web", &roots(&["x"])).unwrap(); + assert_eq!( + out, + "stacks:\n console:\n - console\n web:dev:\n - session-manager\n web:\n - x\n" + ); + assert!(parses(&out)); + } + + /// Same root cause, on the remove path: the truncated key meant + /// `remove_stack` could never find `web:dev` by its real name (reporting + /// it undefined) while `remove_stack(src, "web")` — a name that isn't + /// actually defined anywhere — silently deleted it instead. + #[test] + fn remove_finds_a_colon_containing_key_by_its_full_name() { + let src = "stacks:\n console:\n - console\n web:dev:\n - session-manager\n"; + let out = remove_stack(src, "web:dev").unwrap(); + assert_eq!(out, "stacks:\n console:\n - console\n"); + assert!(parses(&out)); + } + + /// Same colon-truncation bug, one codepoint over: `entry_key` used + /// `char::is_whitespace` (Unicode `White_Space`) to decide whether a `:` + /// ends a key, but YAML's `s-white` is space and tab only. NBSP + /// (U+00A0) has `White_Space=Yes` in Unicode but is not `s-white` in + /// YAML, so a colon followed by NBSP is ordinary scalar content, not a + /// key terminator — `a:b` is the single key `a:b`, not `a`. + #[test] + fn upsert_leaves_a_key_with_nbsp_after_the_colon_intact() { + let src = "stacks:\n console:\n - console\n a:\u{00A0}b:\n - session-manager\n"; + let out = upsert_stack(src, "a", &roots(&["x"])).unwrap(); + assert_eq!( + out, + "stacks:\n console:\n - console\n a:\u{00A0}b:\n - session-manager\n a:\n - x\n" + ); + assert!(parses(&out)); + } + + /// Same root cause, in `is_list_item`: NBSP passes `char::is_whitespace` + /// too, so `-weird` was misread as a real list-item indicator + /// (dash-then-whitespace) even though YAML's dash rule also requires + /// `s-white`, not any Unicode space. That let the line slip past the + /// guard as "a recognized list item" without ever reaching `entry_key` + /// (which would refuse it, same as `-weird:` without a space at all) — + /// reopening the swallowing bug through a codepoint instead of a byte, + /// and reopening Critical-1's own zero-byte-file repro on the remove + /// path (no other entries left once the sibling is swallowed with + /// `tiny`, so the whole `stacks:` key — and then the whole file — + /// would go with it). + #[test] + fn refuses_a_dash_nbsp_key_misread_as_a_list_item() { + let src = "stacks:\n tiny:\n - session-manager\n -\u{00A0}weird:\n - console\n"; + let err = upsert_stack(src, "tiny", &roots(&["x"])).unwrap_err(); + assert!(err.to_string().contains("weird"), "{err:#}"); + + let err = remove_stack(src, "tiny").unwrap_err(); + assert!(err.to_string().contains("weird"), "{err:#}"); + } + + /// Critical-1's headline repro, reopened a third way: `#` is only a + /// YAML comment when preceded by `s-white` (or nothing). NBSP is not + /// `s-white`, so ` #a:` is a real key (`#a`), not a + /// comment — but `is_blank_or_comment` used Unicode `trim_start`, which + /// strips NBSP too, so every function in this module treated the line + /// as a skippable comment and it went invisible. Refusing (not + /// silently losing the sibling, and never truncating the file to 0 + /// bytes) is the required outcome, same as any other line this scanner + /// can't confidently own. + #[test] + fn refuses_a_comment_shaped_nbsp_key_rather_than_losing_it() { + let src = "stacks:\n tiny:\n - session-manager\n \u{00A0}#a:\n - v\n"; + + let err = upsert_stack(src, "tiny", &roots(&["x"])).unwrap_err(); + assert!( + err.to_string().contains("isn't a name or a list item"), + "{err:#}" + ); + + let err = remove_stack(src, "tiny").unwrap_err(); + assert!( + err.to_string().contains("isn't a name or a list item"), + "{err:#}" + ); + // The sharpest form of the bug: must never succeed with an empty + // file, which `write_verified` would then happily accept. + } + + /// Regression from Critical-1 itself (`entry_key` stopped requiring + /// `valid_stack_name`, which used to reject `#` as non-alphanumeric): + /// a comment that happens to look like `key: value` — an ordinary TODO + /// note — was read as a real key. `entry_range` then cut `tiny`'s + /// range short right before it, leaving `- state` behind; YAML doesn't + /// care that a comment sits between two list items of the same key, so + /// `- state` silently reattached to the rewritten `tiny:` on the next + /// load. `entry_key` must decline a comment line outright. + #[test] + fn upsert_does_not_let_a_todo_comment_pose_as_a_key() { + let src = "stacks:\n tiny:\n - session-manager\n # TODO: add console\n - state\n other:\n - o\n"; + let out = upsert_stack(src, "tiny", &roots(&["x"])).unwrap(); + assert_eq!( + out, "stacks:\n tiny:\n - x\n other:\n - o\n", + "`- state` must be replaced along with the rest of tiny's real \ + range, not left behind to silently reattach to it on reload" + ); + assert!(parses(&out)); + } + + /// A comment with no colon at all was never at risk (nothing here ever + /// misread it as a key) — this is the plain, common case, pinned so the + /// fix above can't regress it: comments between entries still survive + /// and stay attached to the right neighbour. + #[test] + fn plain_comment_inside_a_block_is_still_skipped() { + let src = + "stacks:\n tiny:\n - session-manager\n # just a note\n console:\n - console\n"; + let out = upsert_stack(src, "tiny", &roots(&["x"])).unwrap(); + assert!(out.contains("# just a note"), "{out:?}"); + assert!(out.contains(" console:\n - console\n"), "{out:?}"); + assert!(parses(&out)); + } + + /// Sibling of the TODO-comment case: a comment containing a colon, but + /// indented DEEPER than the keys (a commented-out list item, e.g. a + /// disabled root). `entry_range`'s own shallower-or-equal check already + /// happened to save this shape before this fix — pinned here so it + /// stays safe, and so "the comment check only matters at entry depth" + /// can never quietly regress into the truth. + #[test] + fn a_deeper_comment_with_a_colon_is_still_skipped() { + let src = "stacks:\n tiny:\n - session-manager\n # note: disabled\n - state\n console:\n - console\n"; + let out = upsert_stack(src, "tiny", &roots(&["x"])).unwrap(); + assert_eq!( + out, + "stacks:\n tiny:\n - x\n console:\n - console\n" + ); + assert!(parses(&out)); + } + + /// Blocker 1's exact shape, one predicate over: `is_list_item`'s own + /// leading `trim_start()` was still Rust's Unicode trim, not + /// `is_yaml_space`-based, so ` - weird: v` had its NBSP eaten + /// right along with real indentation. What's left, `- weird: v`, reads + /// as a same-level list item (`indent_len` counts only real spaces, so + /// the depth check still passes) and the guard waves it through — but + /// `#` isn't the only thing NBSP can hide a real key behind: a `-` not + /// preceded by valid indentation isn't a YAML sequence indicator + /// either, so this line is really the key `- weird`, invisible to + /// `entry_key` (which then rejects it on the surrounding-whitespace + /// check) and so invisible to `entry_range`, `block_indent`, and + /// `remove_stack`'s `has_entry` scan alike. Previously assessed (wrongly + /// — see the corrected Follow-up 5 note in `final-fix-report.md`) as + /// degrading only to a safe refusal; it does not. + #[test] + fn refuses_a_dash_after_nbsp_misread_as_a_list_item() { + let src = "stacks:\n tiny:\n - session-manager\n \u{00A0}- weird: v\n"; + + let err = upsert_stack(src, "tiny", &roots(&["x"])).unwrap_err(); + assert!(err.to_string().contains("weird"), "{err:#}"); + + let err = remove_stack(src, "tiny").unwrap_err(); + assert!(err.to_string().contains("weird"), "{err:#}"); + // Must never succeed with an empty file, which `write_verified` + // would then happily accept. + } + + /// Pins the fix directly: a fixed `.tmp` sibling is what let two + /// `workers-dev` instances collide (and delete each other's temp file on + /// the error path); the name must carry this process's pid instead. + #[test] + fn temp_sibling_name_carries_the_process_id() { + let tmp = temp_sibling(Path::new("/repo/workers-dev.yaml")); + assert_eq!( + tmp, + Path::new(&format!( + "/repo/workers-dev.yaml.{}.tmp", + std::process::id() + )) + ); + } + + #[test] + fn write_verified_replaces_the_file_atomically() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::write(&path, "color: auto\n").unwrap(); + + write_verified(&path, "color: auto\nstacks:\n a:\n - b\n").unwrap(); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "color: auto\nstacks:\n a:\n - b\n" + ); + // No temp file left behind. + let leftovers: Vec<_> = std::fs::read_dir(tmp.path()) + .unwrap() + .filter_map(Result::ok) + .filter(|e| e.file_name().to_string_lossy().ends_with(".tmp")) + .collect(); + assert!(leftovers.is_empty(), "temp file was not renamed away"); + } + + #[test] + fn write_verified_refuses_text_the_loader_cannot_parse() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::write(&path, "color: auto\n").unwrap(); + + let err = write_verified(&path, "stacks:\n a:\n - b\n bad\n").unwrap_err(); + assert!(!err.to_string().is_empty()); + // The original file is untouched. + assert_eq!(std::fs::read_to_string(&path).unwrap(), "color: auto\n"); + } + + /// A digits-only (or `true`/`false`/`null`/...) stack name writes as valid + /// YAML text — `upsert_stack` only checks ASCII alnum/-/_, which digits + /// satisfy — but reads back as a non-string mapping key, which `load`'s + /// `parse_stacks` then refuses. `write_verified` must catch that at write + /// time (via `validate_config_text` running the same parse), not leave a + /// file behind that only fails on the next launch. + #[test] + fn write_verified_refuses_a_digits_only_stack_name() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::write(&path, "color: auto\n").unwrap(); + + let err = write_verified(&path, "stacks:\n 123:\n - console\n").unwrap_err(); + assert!(!err.to_string().is_empty()); + // The original file is untouched — byte-identical. + assert_eq!(std::fs::read_to_string(&path).unwrap(), "color: auto\n"); + } +} diff --git a/workers-dev/src/discover.rs b/workers-dev/src/discover.rs index aeaa765ea..1c913547d 100644 --- a/workers-dev/src/discover.rs +++ b/workers-dev/src/discover.rs @@ -6,8 +6,8 @@ use anyhow::{Context, Result}; use serde::Deserialize; /// Harness-stack *roots*: what `workers-dev up` / `Ctrl+u` start by name. -/// The dashboard's "harness stack" group is these plus everything they -/// transitively depend on — see `assign_groups`. +/// The dashboard's stack group is these plus everything they transitively +/// depend on, computed at view time — see `stack_members`. pub const HARNESS_STACK: &[&str] = &[ "session-manager", "llm-router", @@ -20,19 +20,10 @@ pub const HARNESS_STACK: &[&str] = &[ #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum WorkerGroup { - HarnessStack, + Stack, Other, } -impl WorkerGroup { - pub fn label(self) -> &'static str { - match self { - Self::HarnessStack => "harness stack", - Self::Other => "other", - } - } -} - #[derive(Debug, Clone)] pub enum SpawnKind { CargoRun, @@ -43,7 +34,6 @@ pub enum SpawnKind { pub struct WorkerSpec { pub name: String, pub dir: PathBuf, - pub group: WorkerGroup, pub spawn: SpawnKind, /// Direct dependencies declared in iii.worker.yaml, filtered to workers /// that actually exist in this repo, sorted. @@ -108,7 +98,6 @@ pub fn discover_repo_workers(repo_root: &Path) -> Result> { specs.push(WorkerSpec { name, dir, - group: WorkerGroup::Other, // assigned below once all names are known spawn, deps, ui_dir, @@ -123,45 +112,38 @@ pub fn discover_repo_workers(repo_root: &Path) -> Result> { spec.deps.sort_unstable(); } - let default_roots: Vec = HARNESS_STACK - .iter() - .filter(|n| names.contains(**n)) - .map(|n| n.to_string()) - .collect(); - assign_groups(&mut specs, &default_roots); + // Alphabetical, deterministic discovery order. Stack grouping and display + // order are computed at view time from the current stack (assign_view_groups). + specs.sort_by(|a, b| a.name.cmp(&b.name)); Ok(specs) } -/// Group workers by deriving the harness stack from the dependency graph: -/// the stack is `roots` plus everything they transitively depend on, so a -/// newly declared dependency joins the stack group automatically instead of -/// waiting for someone to extend a hardcoded list. Re-sorts specs into -/// display order (stack first, alphabetical within a group). -pub fn assign_groups(specs: &mut [WorkerSpec], roots: &[String]) { +/// A stack's member set: `roots` plus everything they transitively depend +/// on, restricted to discovered workers. Pure — callers regroup views with +/// it at view time (see status::assign_view_groups), so a newly declared +/// dependency joins the stack automatically instead of waiting for someone +/// to extend a hardcoded list. +pub fn stack_members(specs: &[WorkerSpec], roots: &[String]) -> HashSet { let deps_by_name: HashMap<&str, &[String]> = specs .iter() .map(|s| (s.name.as_str(), s.deps.as_slice())) .collect(); - let mut stack: HashSet<&str> = HashSet::new(); + let mut members: HashSet<&str> = HashSet::new(); let mut queue: VecDeque<&str> = roots.iter().map(String::as_str).collect(); while let Some(name) = queue.pop_front() { - if !stack.insert(name) { + let Some(deps) = deps_by_name.get(name) else { continue; - } - if let Some(deps) = deps_by_name.get(name) { - queue.extend(deps.iter().map(String::as_str)); - } - } - let stack: HashSet = stack.into_iter().map(str::to_string).collect(); - - for spec in specs.iter_mut() { - spec.group = if stack.contains(&spec.name) { - WorkerGroup::HarnessStack - } else { - WorkerGroup::Other }; + // `insert` returning false — already visited — is the only thing + // stopping a cyclic dependency graph from looping forever: every + // name is queued only once, so each edge is followed at most once. + // See `stack_members_terminates_on_a_dependency_cycle` below. + if !members.insert(name) { + continue; + } + queue.extend(deps.iter().map(String::as_str)); } - specs.sort_by(|a, b| a.group.cmp(&b.group).then_with(|| a.name.cmp(&b.name))); + members.into_iter().map(str::to_string).collect() } fn classify_spawn(dir: &Path, yaml: &WorkerYaml) -> SpawnKind { @@ -228,11 +210,11 @@ mod tests { fs::write(dir.join("Cargo.toml"), "[workspace]\n").unwrap(); } - /// The stack group is derived from the graph: roots plus transitive deps. - /// A dependency two hops from `harness` lands in the stack group without - /// appearing in any hardcoded list; unmanaged dep names are dropped. + /// The member set is derived from the graph: roots plus transitive deps. + /// A dependency two hops from `harness` is a member without appearing in + /// any hardcoded list; unmanaged dep names are dropped. #[test] - fn harness_stack_group_follows_dependencies() { + fn stack_members_follows_dependencies() { let tmp = TempDir::new().unwrap(); write_worker_with_deps(&tmp, "harness", &["state", "configuration"]); write_worker_with_deps(&tmp, "state", &["iii-directory"]); @@ -240,11 +222,13 @@ mod tests { write_worker_with_deps(&tmp, "telegram-bot", &[]); let specs = discover_repo_workers(tmp.path()).unwrap(); - let group = |n: &str| specs.iter().find(|s| s.name == n).unwrap().group; - assert_eq!(group("harness"), WorkerGroup::HarnessStack); - assert_eq!(group("state"), WorkerGroup::HarnessStack); - assert_eq!(group("iii-directory"), WorkerGroup::HarnessStack); - assert_eq!(group("telegram-bot"), WorkerGroup::Other); + let members = stack_members(&specs, &["harness".to_string()]); + assert!(members.contains("harness")); + assert!(members.contains("state")); + assert!(members.contains("iii-directory")); + assert!(!members.contains("telegram-bot")); + // A root that is not a discovered worker never becomes a member. + assert!(!stack_members(&specs, &["ghost".to_string()]).contains("ghost")); // `configuration` isn't a repo worker — dropped from the spec's deps. assert_eq!( specs.iter().find(|s| s.name == "harness").unwrap().deps, @@ -252,6 +236,20 @@ mod tests { ); } + /// Mutual dependencies must terminate — the `members.insert` guard is the + /// only thing preventing an infinite traversal here. + #[test] + fn stack_members_terminates_on_a_dependency_cycle() { + let tmp = TempDir::new().unwrap(); + write_worker_with_deps(&tmp, "a", &["b"]); + write_worker_with_deps(&tmp, "b", &["a"]); + + let specs = discover_repo_workers(tmp.path()).unwrap(); + let members = stack_members(&specs, &["a".to_string()]); + assert!(members.contains("a") && members.contains("b")); + assert_eq!(members.len(), 2); + } + /// `ui/package.json` marks an injectable-UI worker; a bare `ui/` dir /// without a package.json (or no ui/ at all) does not. #[test] @@ -272,7 +270,7 @@ mod tests { } #[test] - fn discovers_and_groups_workers() { + fn discovers_and_classifies_workers() { let tmp = TempDir::new().unwrap(); write_worker(&tmp, "harness", "rust", "binary", true); write_worker(&tmp, "telegram-bot", "rust", "binary", true); @@ -280,18 +278,11 @@ mod tests { let specs = discover_repo_workers(tmp.path()).unwrap(); assert_eq!(specs.len(), 3); - assert_eq!(specs[0].name, "harness"); - assert_eq!(specs[0].group, WorkerGroup::HarnessStack); - assert!(matches!(specs[0].spawn, SpawnKind::CargoRun)); + // Alphabetical discovery order; display grouping happens at view time. + assert_eq!(specs[0].name, "claude-code"); + assert_eq!(specs[1].name, "harness"); assert_eq!(specs[2].name, "telegram-bot"); - assert_eq!(specs[2].group, WorkerGroup::Other); - assert!(matches!( - specs - .iter() - .find(|s| s.name == "claude-code") - .unwrap() - .spawn, - SpawnKind::Unsupported { .. } - )); + assert!(matches!(specs[1].spawn, SpawnKind::CargoRun)); + assert!(matches!(specs[0].spawn, SpawnKind::Unsupported { .. })); } } diff --git a/workers-dev/src/main.rs b/workers-dev/src/main.rs index 7abe9d8cf..4db318803 100644 --- a/workers-dev/src/main.rs +++ b/workers-dev/src/main.rs @@ -1,6 +1,7 @@ mod color; mod commands; mod config; +mod config_write; mod discover; mod git; mod graph; @@ -42,7 +43,7 @@ struct Cli { #[arg(long, global = true)] release: bool, - /// Optional workers-dev.yaml config file + /// Config file (default: /workers-dev.yaml when present) #[arg(long, global = true)] config: Option, @@ -66,14 +67,14 @@ struct Cli { #[derive(Subcommand, Debug)] enum Command { - /// Start the harness stack and open the TUI dashboard + /// Start the default stack and open the TUI dashboard Up, - /// Start workers (default: harness stack; use --all for every worker) + /// Start workers (default: the default stack; use --all for every worker) Start { - /// Worker name(s) to start; default = the harness stack + /// Worker name(s) to start; default = the default stack #[arg(value_name = "WORKER")] workers: Vec, - /// Start every discovered worker instead of the harness stack + /// Start every discovered worker instead of the default stack #[arg(long)] all: bool, }, diff --git a/workers-dev/src/orchestrator.rs b/workers-dev/src/orchestrator.rs index fb5557b34..4f93a47c4 100644 --- a/workers-dev/src/orchestrator.rs +++ b/workers-dev/src/orchestrator.rs @@ -9,7 +9,7 @@ use tokio::process::Command; use tokio::time; use crate::config::Config; -use crate::discover::SpawnKind; +use crate::discover::{SpawnKind, WorkerGroup}; use crate::graph::WorkerGraph; use crate::logs; use crate::runtime::{ProcState, SharedRuntimes, WorkerRuntime}; @@ -215,9 +215,54 @@ impl Orchestrator { !names.iter().any(|n| n == worker) && connected.contains(worker) } - pub async fn start_harness_stack(&self, wait_connected: bool) -> Result<()> { - self.start_workers(&self.config.harness_stack, wait_connected) - .await + /// Roots configured for a named stack. Doesn't check for emptiness — + /// `start_roots` owns that guard for an actual start; callers that need + /// to ask the question before doing anything with side effects (e.g. + /// `commands::run_up`, before `ensure_engine()`) check the result + /// themselves. + pub fn stack_roots(&self, name: &str) -> Result> { + self.config + .stacks + .iter() + .find(|(n, _)| n == name) + .map(|(_, roots)| roots.clone()) + .with_context(|| format!("unknown stack {name}")) + } + + /// Start a configured stack by name. + pub async fn start_stack(&self, name: &str, wait_connected: bool) -> Result<()> { + let roots = self.stack_roots(name)?; + self.start_roots(name, &roots, wait_connected).await + } + + /// Start a stack given its roots directly — the path the TUI uses, since + /// stacks created mid-session are not in `config.stacks`. Roots are the + /// requested set (always (re)started); missing deps are pulled in, and + /// deps already connected to the engine are left alone. + pub async fn start_roots( + &self, + stack: &str, + roots: &[String], + wait_connected: bool, + ) -> Result<()> { + // Empty roots mean every name the stack listed was filtered out of + // the managed `workers:` set (config.rs warns and drops). Refuse + // instead of falling through to `start_workers`' "no names" meaning + // — "start every managed worker" — which an empty stack must never + // trigger. + if roots.is_empty() { + bail!("stack {stack} has no startable workers"); + } + self.start_workers(roots, wait_connected).await + } + + /// Member set (roots + transitive deps) of a configured stack. + pub fn stack_members(&self, name: &str) -> Result> { + let roots = self.stack_roots(name)?; + Ok(crate::discover::stack_members( + &self.config.worker_specs, + &roots, + )) } pub async fn start_all_managed(&self, wait_connected: bool) -> Result<()> { @@ -606,14 +651,19 @@ impl Orchestrator { } pub async fn worker_views(&self) -> Result> { - Ok(self.dashboard_snapshot().await.0) + let members = self.stack_members(&self.config.default_stack)?; + Ok(self.dashboard_snapshot(&members).await.0) } /// Like `worker_views`, but also reports an engine-query error (if any) so /// the dashboard can show "engine unreachable" instead of silently blanking /// every engine status. Never fails: on engine error the views still render - /// (all disconnected) alongside the error string. - pub async fn dashboard_snapshot(&self) -> (Vec, Option) { + /// (all disconnected) alongside the error string. Views come back grouped + /// and ordered for `members` (the current stack's member set). + pub async fn dashboard_snapshot( + &self, + members: &HashSet, + ) -> (Vec, Option) { let (engine, engine_error) = match self.engine_workers().await { Ok(list) => (list, None), Err(err) => (Vec::new(), Some(format!("{err:#}"))), @@ -630,6 +680,7 @@ impl Orchestrator { let spec = self.config.worker_spec(worker).expect("spec"); views.push(build_view(spec, rt, engine_by_name.get(worker))); } + status::assign_view_groups(&mut views, members); (views, engine_error) } @@ -704,7 +755,7 @@ fn build_view( WorkerView { name: spec.name.clone(), - group: spec.group, + group: WorkerGroup::Other, // reassigned by assign_view_groups spawnable: matches!(spec.spawn, SpawnKind::CargoRun), display_status: display.to_string(), process_status: process, @@ -919,12 +970,14 @@ mod tests { let workers = crate::discover::order_worker_names(&worker_specs); let config = Config { repo_root: tmp.path().to_path_buf(), + config_path: tmp.path().join("workers-dev.yaml"), engine_url: crate::config::DEFAULT_ENGINE_URL.to_string(), release: false, poll_interval_ms: crate::config::DEFAULT_POLL_INTERVAL_MS, connect_timeout_ms: crate::config::DEFAULT_CONNECT_TIMEOUT_MS, workers, - harness_stack: vec!["harness".to_string()], + stacks: vec![("harness".to_string(), vec!["harness".to_string()])], + default_stack: "harness".to_string(), worker_specs, stop_on_exit: false, color_mode: Default::default(), @@ -943,6 +996,84 @@ mod tests { assert!(order.contains(&"scrapling".to_string())); } + /// Shared fixture for the empty-roots tests below: a repo with a single + /// `harness` worker, plus a `ghost` stack with no roots — what config.rs + /// produces once every named root has been filtered out of `workers:`. + /// Returns the `TempDir` too: it backs `repo_root`/`config_path` and must + /// outlive the `Orchestrator`. + fn test_orchestrator() -> (tempfile::TempDir, Orchestrator) { + let tmp = tempfile::TempDir::new().unwrap(); + let write = |name: &str, body: &str| { + let dir = tmp.path().join(name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("iii.worker.yaml"), body).unwrap(); + dir + }; + let harness_dir = write( + "harness", + "iii: v1\nname: harness\nlanguage: rust\ndeploy: binary\n", + ); + std::fs::write(harness_dir.join("Cargo.toml"), "[workspace]\n").unwrap(); + + let worker_specs = crate::discover::discover_repo_workers(tmp.path()).unwrap(); + let workers = crate::discover::order_worker_names(&worker_specs); + let config = Config { + repo_root: tmp.path().to_path_buf(), + config_path: tmp.path().join("workers-dev.yaml"), + engine_url: crate::config::DEFAULT_ENGINE_URL.to_string(), + release: false, + poll_interval_ms: crate::config::DEFAULT_POLL_INTERVAL_MS, + connect_timeout_ms: crate::config::DEFAULT_CONNECT_TIMEOUT_MS, + workers, + // "ghost" mirrors what config.rs produces once every root of a + // configured stack has been dropped: the stack survives with an + // empty roots list rather than being removed outright. + stacks: vec![ + ("harness".to_string(), vec!["harness".to_string()]), + ("ghost".to_string(), Vec::new()), + ], + default_stack: "harness".to_string(), + worker_specs, + stop_on_exit: false, + color_mode: Default::default(), + ui_watch: false, + }; + let orch = Orchestrator::new(config, false).unwrap(); + (tmp, orch) + } + + /// A stack can legitimately end up with empty roots when every root it + /// names is filtered out of the managed `workers:` set (config.rs warns + /// and drops). `start_stack` must refuse rather than fall through to + /// `start_workers`' "no names" meaning ("start every managed worker"). + /// No engine is reachable in this test, and `start_workers`' first move + /// is an engine round-trip (`connected_worker_names`) — so the "stack + /// ghost has no startable workers" assertion below only passes if the + /// guard in `start_stack` returns first; were it missing or placed + /// after that call, this would fail on a connection error instead. + #[tokio::test] + async fn start_stack_with_empty_roots_refuses_to_start_everything() { + let (_tmp, orch) = test_orchestrator(); + + let err = orch + .start_stack("ghost", false) + .await + .expect_err("a stack with no startable workers must refuse to start"); + assert!( + err.to_string().contains("ghost"), + "error should name the stack, got: {err}" + ); + } + + /// The empty-roots guard lives in `start_roots`, so it protects the + /// TUI's roots-based path too — and fires before any engine contact. + #[tokio::test] + async fn start_roots_refuses_an_empty_root_list() { + let (_tmp, orch) = test_orchestrator(); + let err = orch.start_roots("ghost", &[], false).await.unwrap_err(); + assert!(err.to_string().contains("ghost"), "{err:#}"); + } + /// The env var must match what `iii-console-ui`'s `ConsoleUi::new` /// derives, or the worker's poller never arms. Locks the two known /// shapes: plain names and hyphenated ones. diff --git a/workers-dev/src/status.rs b/workers-dev/src/status.rs index 11e570e32..03a7dc697 100644 --- a/workers-dev/src/status.rs +++ b/workers-dev/src/status.rs @@ -37,6 +37,31 @@ pub struct WorkerView { pub ui_watch: Option, } +use std::collections::HashSet; + +/// Group header text for one stack: the Stack group is named after the +/// current stack, everything else is "other". +pub fn group_label(group: WorkerGroup, stack_name: &str) -> String { + match group { + WorkerGroup::Stack => format!("stack:{stack_name}"), + WorkerGroup::Other => "other".to_string(), + } +} + +/// Assign groups + display order for one stack's member set: members first, +/// then other, alphabetical within each. Runs at view time so switching +/// stacks in the TUI regroups without touching discovery data. +pub fn assign_view_groups(views: &mut [WorkerView], members: &HashSet) { + for v in views.iter_mut() { + v.group = if members.contains(&v.name) { + WorkerGroup::Stack + } else { + WorkerGroup::Other + }; + } + views.sort_by(|a, b| a.group.cmp(&b.group).then_with(|| a.name.cmp(&b.name))); +} + /// Table text for the UI column, shared by the TUI and `status`. pub fn ui_watch_label(ui_watch: Option) -> &'static str { match ui_watch { @@ -68,7 +93,7 @@ pub async fn fetch_engine_workers( Ok(parsed.workers) } -pub fn print_status_table(views: &[WorkerView]) { +pub fn print_status_table(views: &[WorkerView], stack_name: &str) { if views.is_empty() { return; } @@ -78,7 +103,7 @@ pub fn print_status_table(views: &[WorkerView]) { if last_group.is_some() { println!(); } - println!("── {} ──", v.group.label()); + println!("── {} ──", group_label(v.group, stack_name)); println!( "{:<28} {:<12} {:<12} {:<6} {:<8} {:<8}", "WORKER", "PROCESS", "ENGINE", "UI", "PID", "UPTIME" @@ -105,3 +130,49 @@ pub fn print_status_table(views: &[WorkerView]) { ); } } + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashSet; + + fn view(name: &str) -> WorkerView { + WorkerView { + name: name.to_string(), + group: WorkerGroup::Other, + spawnable: true, + display_status: "stopped".to_string(), + process_status: "stopped".to_string(), + engine_status: "—".to_string(), + local_pid: None, + uptime: "—".to_string(), + exit_code: None, + ui_watch: None, + } + } + + #[test] + fn assign_view_groups_orders_stack_first_alpha_within() { + let mut views = vec![view("zeta"), view("console"), view("alpha")]; + let members: HashSet = ["zeta".to_string(), "alpha".to_string()] + .into_iter() + .collect(); + assign_view_groups(&mut views, &members); + let order: Vec<(&str, WorkerGroup)> = + views.iter().map(|v| (v.name.as_str(), v.group)).collect(); + assert_eq!( + order, + vec![ + ("alpha", WorkerGroup::Stack), + ("zeta", WorkerGroup::Stack), + ("console", WorkerGroup::Other), + ] + ); + } + + #[test] + fn group_labels() { + assert_eq!(group_label(WorkerGroup::Stack, "console"), "stack:console"); + assert_eq!(group_label(WorkerGroup::Other, "console"), "other"); + } +} diff --git a/workers-dev/src/tui/mod.rs b/workers-dev/src/tui/mod.rs index 2b7011efe..287f04f96 100644 --- a/workers-dev/src/tui/mod.rs +++ b/workers-dev/src/tui/mod.rs @@ -1,3 +1,4 @@ +mod stacks; mod theme; use std::io; @@ -65,9 +66,11 @@ const BRANCH_MAX: usize = 40; const ENGINE_START_WAIT_MS: u64 = 8_000; /// Footer help, by available width. The narrowest tier always keeps the two -/// keys a lost user needs (help, quit). +/// keys a lost user needs (help, quit). `draw_footer` gates `HELP_FULL` on +/// its own character count rather than a hand-copied number, so a tier that +/// grows can never silently clip its own tail again. const HELP_FULL: &str = - " ↑↓ select · s start · x stop · r restart · w ui-watch · / filter · f follow · ? keys · q quit "; + " ↑↓ select · Space mark · n new stack · s start · x stop · r restart · w ui-watch · / filter · f follow · ? keys · q quit "; const HELP_MID: &str = " s start · x stop · r restart · / filter · ? keys · q quit "; const HELP_MIN: &str = " / filter · ? keys · q quit "; @@ -89,6 +92,34 @@ enum UiMode { dependents: Vec, }, Busy(String), + /// Stack picker (Ctrl+u with several stacks defined). `selected` indexes + /// into the session's `stacks` list, not `config.stacks` — the two + /// diverge after any save/delete/default change made from the picker. + /// Every row is reachable, including ones with no startable roots: + /// `move_stack_selection` (`stacks.rs`) never skips them, since `x` + /// (delete) must still be able to reach an unstartable stack. + StackPicker { + selected: usize, + }, + /// Naming a new stack built from the marked workers. `roots` is frozen + /// when the prompt opens, so later marking cannot change what gets saved; + /// `expanded` is how many workers those roots reach with their deps. + NameStack { + name: String, + roots: Vec, + expanded: usize, + }, + /// Confirming an overwrite: the name already exists in the file. + ConfirmSaveStack { + name: String, + roots: Vec, + }, + /// Confirming a delete (picker's x). The file write happens only on + /// y/Enter; deleting the default stack is refused before this mode is + /// ever entered (there'd be nothing left to fall back to). + ConfirmDeleteStack { + name: String, + }, } enum ModeKind { @@ -98,6 +129,10 @@ enum ModeKind { Deps, Confirm, Busy, + StackPicker, + NameStack, + ConfirmSave, + ConfirmDelete, } fn mode_kind(mode: &UiMode) -> ModeKind { @@ -108,6 +143,10 @@ fn mode_kind(mode: &UiMode) -> ModeKind { UiMode::Deps { .. } => ModeKind::Deps, UiMode::ConfirmRestart { .. } => ModeKind::Confirm, UiMode::Busy(_) => ModeKind::Busy, + UiMode::StackPicker { .. } => ModeKind::StackPicker, + UiMode::NameStack { .. } => ModeKind::NameStack, + UiMode::ConfirmSaveStack { .. } => ModeKind::ConfirmSave, + UiMode::ConfirmDeleteStack { .. } => ModeKind::ConfirmDelete, } } @@ -134,7 +173,7 @@ struct DashboardState { } enum DisplayRowKind { - Header(WorkerGroup), + Header(WorkerGroup, usize), Worker(usize), } @@ -146,7 +185,16 @@ struct DisplayRow { struct UiCtx<'a> { engine_url: &'a str, repo_branch: Option<&'a str>, + current_stack: &'a str, + default_stack: &'a str, + /// The config file's path, displayed in the confirm-delete dialog so a + /// deletion never names a file the user can't place. + config_path: &'a str, + stacks: &'a [(String, Vec)], views: &'a [WorkerView], + /// Workers marked with Space for a new stack; `n` saves the marked set + /// as a stack (see `handle_dashboard_key`'s `n` arm). + marked: &'a std::collections::HashSet, engine_error: Option<&'a str>, display_rows: &'a [DisplayRow], mode: &'a UiMode, @@ -178,7 +226,24 @@ pub async fn run(orchestrator: Arc) -> Result<()> { // Poll the engine on a background task so a slow or unreachable engine // query can't freeze keyboard input. - let (initial_views, initial_err) = orchestrator.dashboard_snapshot().await; + // Session copies of the config's stacks, so saving a new stack (n) + // doesn't require re-reading the file: `default_stack` isn't mutated + // by saving a new stack, but lives here so a later "set default" + // (Ctrl+u picker) can update it without reaching back into `Config`. + let mut stacks: Vec<(String, Vec)> = orchestrator.config.stacks.clone(); + // Reassigned only by the picker's `*` (make default) — never by + // saving a new stack. + let mut default_stack = orchestrator.config.default_stack.clone(); + let mut current_stack = orchestrator.config.default_stack.clone(); + // Precomputed once: the confirm-delete dialog names the file it will + // edit, and `UiCtx` borrows `&str`s for the whole frame. + let config_path_display = orchestrator.config.config_path.display().to_string(); + let initial_members = orchestrator.stack_members(¤t_stack)?; + let (initial_views, initial_err) = orchestrator.dashboard_snapshot(&initial_members).await; + // The poller reads the current stack's member set each tick, so a + // switch regroups from the next poll onward (the switch itself also + // regroups the local snapshot immediately). + let shared_members = Arc::new(std::sync::RwLock::new(initial_members)); let initial_branch = crate::git::current_branch(&orchestrator.config.repo_root); set_terminal_title(initial_branch.as_deref()); let (state_tx, mut state_rx) = tokio::sync::watch::channel(DashboardState { @@ -189,10 +254,12 @@ pub async fn run(orchestrator: Arc) -> Result<()> { let poll_interval = Duration::from_millis(orchestrator.config.poll_interval_ms); let poller = { let orchestrator = orchestrator.clone(); + let shared_members = shared_members.clone(); tokio::spawn(async move { loop { tokio::time::sleep(poll_interval).await; - let (views, engine_error) = orchestrator.dashboard_snapshot().await; + let members = shared_members.read().expect("members lock").clone(); + let (views, engine_error) = orchestrator.dashboard_snapshot(&members).await; if state_tx .send(DashboardState { views, @@ -223,6 +290,9 @@ pub async fn run(orchestrator: Arc) -> Result<()> { let mut table_state = TableState::default(); let mut mode = UiMode::Dashboard; let mut filter = String::new(); + // Workers marked with Space, held by name so a mark survives filtering + // and re-sorting. Cleared after a successful save. + let mut marked: std::collections::HashSet = std::collections::HashSet::new(); let mut follow = true; let mut log_scroll: usize = 0; let mut log_height: u16 = LOG_HEIGHT_DEFAULT; @@ -263,7 +333,12 @@ pub async fn run(orchestrator: Arc) -> Result<()> { let ctx = UiCtx { engine_url: &orchestrator.config.engine_url, repo_branch: state.repo_branch.as_deref(), + current_stack: ¤t_stack, + default_stack: &default_stack, + config_path: &config_path_display, + stacks: &stacks, views: &state.views, + marked: &marked, engine_error: state.engine_error.as_deref(), display_rows: &display_rows, mode: &mode, @@ -329,6 +404,217 @@ pub async fn run(orchestrator: Arc) -> Result<()> { mode = UiMode::Dashboard; } } + ModeKind::StackPicker => { + // Selection may regroup the list; keep the + // highlight on the same worker by name. + let keep_name = selected_worker( + &display_rows, + &state.views, + table_state.selected(), + ); + match stacks::handle_stack_picker_key(key, &mut mode, &stacks) { + Some(stacks::PickerAction::Start(name)) => { + // The picker only ever returns a name + // it read from this same `stacks` + // list with non-empty roots. + let roots = stacks + .iter() + .find(|(n, _)| *n == name) + .map(|(_, roots)| roots.clone()) + .expect("picker returned a name absent from `stacks`"); + let members = crate::discover::stack_members( + &orchestrator.config.worker_specs, + &roots, + ); + current_stack = name.clone(); + // Mutates (re-sorts) state.views; the + // outer `display_rows` from the top + // of this loop iteration indexes the + // pre-sort order and must not be read + // again below — nothing does today. + crate::status::assign_view_groups( + &mut state.views, + &members, + ); + *shared_members.write().expect("members lock") = members; + let rows = build_display_rows(&state.views, &filter); + let target = keep_name + .as_deref() + .and_then(|n| row_of_worker(&rows, &state.views, n)) + .or_else(|| first_worker_row(&rows)); + table_state.select(target); + mode = UiMode::Busy(format!("starting stack {name}…")); + spawn_start_roots(&actions, name, roots); + } + // Deleting the default stack would leave + // the config unable to load — refuse + // before ever opening the confirm dialog. + Some(stacks::PickerAction::Delete(name)) => { + match stacks::refuse_delete_reason(&name, &default_stack) { + Some(reason) => { + error_banner = Some((reason, Instant::now())); + } + None => mode = UiMode::ConfirmDeleteStack { name }, + } + } + // Written straight through — no + // confirmation, and the picker stays open + // (mode untouched) either way so the + // redraw shows the `*` land on its new + // row, which is the only feedback this + // needs. + Some(stacks::PickerAction::MakeDefault(name)) => { + match stacks::set_default( + &orchestrator.config.config_path, + &name, + ) { + Ok(()) => default_stack = name, + Err(err) => { + error_banner = + Some((format!("{err:#}"), Instant::now())); + } + } + } + None => {} + } + } + ModeKind::NameStack => { + if let Some((name, roots)) = stacks::handle_name_key(key, &mut mode) + { + if stacks.iter().any(|(n, _)| *n == name) { + mode = UiMode::ConfirmSaveStack { name, roots }; + } else { + let keep_name = selected_worker( + &display_rows, + &state.views, + table_state.selected(), + ); + save_and_adopt_stack( + &orchestrator, + name, + roots, + &mut stacks, + &mut current_stack, + &mut state.views, + &shared_members, + &filter, + &mut table_state, + keep_name, + &mut marked, + &mut mode, + &mut error_banner, + ); + } + } + } + ModeKind::ConfirmSave => { + let UiMode::ConfirmSaveStack { name, roots } = &mode else { + unreachable!("mode kind matched"); + }; + match key.code { + KeyCode::Char('y') | KeyCode::Enter => { + let (name, roots) = (name.clone(), roots.clone()); + let keep_name = selected_worker( + &display_rows, + &state.views, + table_state.selected(), + ); + save_and_adopt_stack( + &orchestrator, + name, + roots, + &mut stacks, + &mut current_stack, + &mut state.views, + &shared_members, + &filter, + &mut table_state, + keep_name, + &mut marked, + &mut mode, + &mut error_banner, + ); + } + KeyCode::Char('n') | KeyCode::Esc => mode = UiMode::Dashboard, + _ => {} + } + } + ModeKind::ConfirmDelete => { + let UiMode::ConfirmDeleteStack { name } = &mode else { + unreachable!("mode kind matched"); + }; + match key.code { + KeyCode::Char('y') | KeyCode::Enter => { + let name = name.clone(); + // Same ordering as save_and_adopt_stack: + // the write must succeed before any + // session state changes. + match stacks::delete_stack( + &orchestrator.config.config_path, + &name, + ) { + Ok(()) => { + stacks.retain(|(n, _)| *n != name); + if current_stack == name { + // The deleted stack was + // current: fall back to + // the default (guaranteed + // present — it can never + // be the one just + // deleted) and regroup, + // same as a picker switch. + let keep_name = selected_worker( + &display_rows, + &state.views, + table_state.selected(), + ); + current_stack = default_stack.clone(); + let roots = stacks + .iter() + .find(|(n, _)| *n == current_stack) + .map(|(_, roots)| roots.clone()) + // default_stack is always present in + // `stacks` (refuse_delete_reason keeps it + // from ever being the one just deleted) — + // fall back rather than panic in raw mode + // if that invariant ever slips, same as + // the Ctrl+u single-stack arm. + .unwrap_or_else(|| stacks[0].1.clone()); + let members = crate::discover::stack_members( + &orchestrator.config.worker_specs, + &roots, + ); + crate::status::assign_view_groups( + &mut state.views, + &members, + ); + *shared_members + .write() + .expect("members lock") = members; + let rows = + build_display_rows(&state.views, &filter); + let target = keep_name + .as_deref() + .and_then(|n| { + row_of_worker(&rows, &state.views, n) + }) + .or_else(|| first_worker_row(&rows)); + table_state.select(target); + } + mode = + UiMode::Busy(format!("deleted stack {name}")); + } + Err(err) => { + error_banner = + Some((format!("{err:#}"), Instant::now())); + mode = UiMode::Dashboard; + } + } + } + KeyCode::Char('n') | KeyCode::Esc => mode = UiMode::Dashboard, + _ => {} + } + } ModeKind::Dashboard => { let two_col = terminal .size() @@ -342,10 +628,13 @@ pub async fn run(orchestrator: Arc) -> Result<()> { &mut table_state, &mut mode, &mut filter, + &mut marked, &mut follow, &mut log_scroll, &mut log_height, &mut table_width, + &stacks, + ¤t_stack, two_col, ); } @@ -374,6 +663,16 @@ pub async fn run(orchestrator: Arc) -> Result<()> { set_terminal_title(fresh.repo_branch.as_deref()); } state = fresh; + // The poller reads `shared_members` before its (possibly + // slow) `dashboard_snapshot` await, so a stack switch mid-await + // lands here grouped by the stale set. Re-group against the + // authoritative current set so a switch never flashes the old + // stack's grouping/header for a frame. Read-and-drop within + // this statement only — never held across an `.await`. + crate::status::assign_view_groups( + &mut state.views, + &shared_members.read().expect("members lock"), + ); needs_redraw = true; } } @@ -405,10 +704,13 @@ fn handle_dashboard_key( table_state: &mut TableState, mode: &mut UiMode, filter: &mut String, + marked: &mut std::collections::HashSet, follow: &mut bool, log_scroll: &mut usize, log_height: &mut u16, table_width: &mut u16, + stacks: &[(String, Vec)], + current_stack: &str, two_col: bool, ) -> bool { let selected = table_state.selected(); @@ -450,6 +752,44 @@ fn handle_dashboard_key( *log_scroll = 0; } } + // Mark for stack creation (n). Marks are independent of selection and + // of the filter — the name prompt shows what will be saved. Refusing + // a non-spawnable (external) worker here matches `s`'s own refusal — + // otherwise a saved stack could name a worker that fails the whole + // start rather than just being skipped. + KeyCode::Char(' ') => { + if let Some(name) = worker_name { + if views.iter().any(|v| v.name == name && v.spawnable) { + if !marked.remove(&name) { + marked.insert(name); + } + } else { + *mode = not_startable_msg(); + } + } + } + // New stack from the marked workers. Opens the name prompt; nothing + // is written or started until Enter. + KeyCode::Char('n') => { + let roots = stacks::roots_from_marks(views, marked); + if roots.is_empty() { + *mode = UiMode::Busy("mark workers with Space first".into()); + } else { + // Roots expand to roots + transitive deps at start and group + // time, so show both numbers — marking two workers can mean + // starting a dozen. + let expanded = crate::discover::stack_members( + &actions.orchestrator.config.worker_specs, + &roots, + ) + .len(); + *mode = UiMode::NameStack { + name: String::new(), + roots, + expanded, + }; + } + } KeyCode::Char('/') => *mode = UiMode::Filter, KeyCode::Char('?') => *mode = UiMode::Help, KeyCode::Char('d') => { @@ -569,8 +909,29 @@ fn handle_dashboard_key( *mode = UiMode::Busy("starting engine…".to_string()); } KeyCode::Char('u') if key.modifiers.contains(KeyModifiers::CONTROL) => { - spawn_start_harness_stack(actions); - *mode = UiMode::Busy("starting harness stack…".to_string()); + if stacks.len() <= 1 { + // Only the built-in stack: no choice to make, start it — + // exactly today's Ctrl+u. + let roots = stacks + .iter() + .find(|(n, _)| n == current_stack) + .map(|(_, roots)| roots.clone()) + // current_stack is always present in `stacks`: deleting + // it (picker's x) switches current_stack back to + // default_stack first, and the default itself can never + // be deleted (the picker refuses that before this state + // is reachable). Kept as a fallback anyway — cheaper than + // a panic in raw mode if that invariant ever slips. + .unwrap_or_else(|| stacks[0].1.clone()); + spawn_start_roots(actions, current_stack.to_string(), roots); + *mode = UiMode::Busy(format!("starting stack {current_stack}…")); + } else { + let selected = stacks + .iter() + .position(|(n, _)| n == current_stack) + .unwrap_or(0); + *mode = UiMode::StackPicker { selected }; + } } KeyCode::Char('a') if key.modifiers.contains(KeyModifiers::CONTROL) => { // Via start_all_managed (like CLI `start --all`), which pre-filters @@ -627,6 +988,59 @@ fn not_startable_msg() -> UiMode { UiMode::Busy("worker not startable from workers-dev (use iii worker add)".into()) } +/// Shared by both `n`-prompt paths (a fresh name, and after an overwrite +/// confirm): write the stack to disk, then — only on success — adopt it as +/// the session's current stack and regroup the dashboard around it. Never +/// starts anything; that stays the picker/Ctrl+u's job. A free `fn` rather +/// than a closure: it mutates enough distinct session bindings (`stacks`, +/// `current_stack`, the views, `shared_members`, `table_state`, `marked`, +/// `mode`, `error_banner`) that a capturing closure would fight the borrow +/// checker, so every one is an explicit parameter instead. +#[allow(clippy::too_many_arguments)] +fn save_and_adopt_stack( + orchestrator: &Orchestrator, + name: String, + roots: Vec, + stacks: &mut Vec<(String, Vec)>, + current_stack: &mut String, + views: &mut [WorkerView], + shared_members: &std::sync::RwLock>, + filter: &str, + table_state: &mut TableState, + keep_name: Option, + marked: &mut std::collections::HashSet, + mode: &mut UiMode, + error_banner: &mut Option<(String, Instant)>, +) { + if let Err(err) = stacks::save_stack(&orchestrator.config.config_path, &name, &roots) { + *error_banner = Some((format!("{err:#}"), Instant::now())); + *mode = UiMode::Dashboard; + return; + } + + match stacks.iter_mut().find(|(n, _)| *n == name) { + Some(entry) => entry.1 = roots.clone(), + None => stacks.push((name.clone(), roots.clone())), + } + *current_stack = name.clone(); + let members = crate::discover::stack_members(&orchestrator.config.worker_specs, &roots); + crate::status::assign_view_groups(views, &members); + *shared_members.write().expect("members lock") = members; + // Downgrade to a shared borrow: nothing below needs `views` mutably. + let views: &[WorkerView] = views; + let rows = build_display_rows(views, filter); + let target = keep_name + .as_deref() + .and_then(|n| row_of_worker(&rows, views, n)) + .or_else(|| first_worker_row(&rows)); + table_state.select(target); + marked.clear(); + *mode = UiMode::Busy(format!( + "saved stack {name} to {}", + orchestrator.config.config_path.display() + )); +} + /// Name the surrounding terminal window / tmux pane after this instance, so /// side-by-side instances are tellable apart from the window list without /// looking inside. Re-applied whenever the branch changes. @@ -664,10 +1078,10 @@ fn spawn_start(actions: &Actions, names: Vec) { }); } -fn spawn_start_harness_stack(actions: &Actions) { +fn spawn_start_roots(actions: &Actions, name: String, roots: Vec) { let orchestrator = actions.orchestrator.clone(); spawn_action(actions, async move { - orchestrator.start_harness_stack(false).await + orchestrator.start_roots(&name, &roots, false).await }); } @@ -708,20 +1122,26 @@ fn spawn_toggle_ui_watch(actions: &Actions, worker: String) { } /// Build the table rows, applying the name filter (case-insensitive). A group -/// header is emitted only when that group has at least one matching worker. +/// header (with its post-filter worker count) is emitted only when that group +/// has at least one matching worker. fn build_display_rows(views: &[WorkerView], filter: &str) -> Vec { let needle = filter.to_lowercase(); + let matches: Vec = views + .iter() + .enumerate() + .filter(|(_, v)| needle.is_empty() || v.name.to_lowercase().contains(&needle)) + .map(|(idx, _)| idx) + .collect(); let mut rows = Vec::new(); let mut last_group = None; - for (idx, view) in views.iter().enumerate() { - if !needle.is_empty() && !view.name.to_lowercase().contains(&needle) { - continue; - } - if last_group != Some(view.group) { + for idx in matches.iter().copied() { + let group = views[idx].group; + if last_group != Some(group) { + let count = matches.iter().filter(|&&i| views[i].group == group).count(); rows.push(DisplayRow { - kind: DisplayRowKind::Header(view.group), + kind: DisplayRowKind::Header(group, count), }); - last_group = Some(view.group); + last_group = Some(group); } rows.push(DisplayRow { kind: DisplayRowKind::Worker(idx), @@ -736,6 +1156,13 @@ fn first_worker_row(display_rows: &[DisplayRow]) -> Option { .position(|row| matches!(row.kind, DisplayRowKind::Worker(_))) } +/// Row index of the named worker in the display rows, if visible. +fn row_of_worker(display_rows: &[DisplayRow], views: &[WorkerView], name: &str) -> Option { + display_rows + .iter() + .position(|r| matches!(r.kind, DisplayRowKind::Worker(idx) if views[idx].name == name)) +} + /// Snap the selection back onto a visible worker row when the current one /// scrolled out (e.g. the filter changed) so the highlight is never stranded. fn clamp_selection(table_state: &mut TableState, display_rows: &[DisplayRow]) { @@ -776,7 +1203,7 @@ fn selected_worker( ) -> Option { match display_rows.get(row?)?.kind { DisplayRowKind::Worker(idx) => views.get(idx).map(|v| v.name.clone()), - DisplayRowKind::Header(_) => None, + DisplayRowKind::Header(..) => None, } } @@ -863,6 +1290,15 @@ fn draw_ui(f: &mut Frame, table_state: &mut TableState, ctx: &UiCtx) { if matches!(ctx.mode, UiMode::Help) { draw_help_overlay(f, area, ctx.color_enabled); } + if let UiMode::StackPicker { selected } = ctx.mode { + stacks::draw_stack_picker_overlay(f, body, *selected, ctx); + } + if let UiMode::ConfirmSaveStack { name, roots } = ctx.mode { + draw_confirm_save_overlay(f, body, name, roots, ctx); + } + if let UiMode::ConfirmDeleteStack { name } = ctx.mode { + draw_confirm_delete_overlay(f, body, name, ctx); + } } /// " ● name …… status" with the table's live glyph and color for `name`. @@ -1119,19 +1555,29 @@ fn draw_table(f: &mut Frame, area: Rect, table_state: &mut TableState, ctx: &UiC .display_rows .iter() .map(|row| match row.kind { - DisplayRowKind::Header(group) => { - Row::new(vec![Cell::from(format!("── {} ──", group.label())) - .style(styled_if(color, group_header_style(group)))]) - } + // No cells: a header row's real content is painted full-width, + // below, after the table renders (Table has no colspan — a Cell + // here would be clipped to the Worker column's width). `widths` + // is non-empty so `column_count` (cell-count driven) never + // affects layout; the `.header(...)` row alone still gives the + // table 6 columns. + DisplayRowKind::Header(..) => Row::new(Vec::::new()), DisplayRowKind::Worker(idx) => { let v = &ctx.views[idx]; let icon = status_icon(&v.display_status, ctx.spinner_frame); - // Name cell is just glyph + name. The wide "(iii worker add)" - // label and the crash exit code moved to the Process column, so - // the name sits right next to its status instead of behind a - // column sized for the longest label. + // Name cell is the mark (Space-toggled, for stack creation) + + // glyph + name; unmarked rows get a space so names still line + // up. The wide "(iii worker add)" label and the crash exit + // code moved to the Process column, so the name sits right + // next to its status instead of behind a column sized for the + // longest label. + let mark = if ctx.marked.contains(&v.name) { + "✓" + } else { + " " + }; let name_cell = Cell::from(Span::styled( - format!("{icon} {}", v.name), + format!("{mark}{icon} {}", v.name), styled_if(color, status_style(&v.display_status)), )); let (process_text, process_st) = process_cell(v, color); @@ -1174,15 +1620,19 @@ fn draw_table(f: &mut Frame, area: Rect, table_state: &mut TableState, ctx: &UiC .count() }; let total = worker_count(ctx.display_rows); + let marked_suffix = match ctx.marked.len() { + 0 => String::new(), + n => format!("· {n} marked "), + }; let title = match table_state.selected() { Some(sel) if sel < ctx.display_rows.len() => { format!( - " Workers {}/{} ", + " Workers {}/{} {marked_suffix}", worker_count(&ctx.display_rows[..=sel]), total ) } - _ => format!(" Workers ({total}) "), + _ => format!(" Workers ({total}) {marked_suffix}"), }; // Content-fit, left-packed widths: the worker name sits right next to its @@ -1205,7 +1655,45 @@ fn draw_table(f: &mut Frame, area: Rect, table_state: &mut TableState, ctx: &UiC ) .block(Block::default().borders(Borders::ALL).title(title)) .row_highlight_style(styled_if(color, selection_row_style())); + // ratatui's `get_row_bounds` only ever lowers the render offset when + // `selected < offset`, and the selection can never land on display row 0 + // when it's a group header (the up/down helpers skip headers) — so once + // a scroll pushes the offset to 1, it never falls back to 0 on its own, + // even after the first worker row scrolls back into view. That row's + // header is the only place the dashboard names the current stack, so + // force the offset back whenever the selection is on it. + if table_state.selected() == first_worker_row(ctx.display_rows) { + *table_state.offset_mut() = 0; + } f.render_stateful_widget(table, area, table_state); + + // Header rows render no cells of their own (see the match arm above) — + // paint their content here, spanning the table's full inner width + // instead of being confined to one column. Assumes every row (header + + // body) is its default 1-line height, true everywhere in this file. + let inner = Block::default().borders(Borders::ALL).inner(area); + let rows_h = inner.height.saturating_sub(1) as usize; // minus the column-header line + let offset = table_state.offset(); + for (i, row) in ctx.display_rows.iter().enumerate().skip(offset) { + let y = i - offset; + if y >= rows_h { + break; + } + if let DisplayRowKind::Header(group, count) = row.kind { + let label = format!( + "── {} ({count}) ──", + crate::status::group_label(group, ctx.current_stack) + ); + let line = Line::from(Span::styled( + label, + styled_if(color, group_header_style(group)), + )); + f.render_widget( + Paragraph::new(line), + Rect::new(inner.x, inner.y + 1 + y as u16, inner.width, 1), + ); + } + } } /// Process-column text + style. Carries the management/crash detail that used @@ -1270,7 +1758,7 @@ fn draw_log_pane(f: &mut Frame, area: Rect, ctx: &UiCtx) { let msg = if ctx.selected_name.is_none() { "(select a worker to view logs)" } else { - "(no output yet — press s to start this worker, or Ctrl+u to start the harness stack)" + "(no output yet — press s to start this worker, or Ctrl+u to start the stack)" }; vec![Line::from(Span::styled( msg, @@ -1331,8 +1819,25 @@ fn draw_footer(f: &mut Frame, area: Rect, ctx: &UiCtx) { format!(" filter: {}_ (Enter apply · Esc clear) ", ctx.filter), styled_if(color, Style::default().fg(Color::Yellow)), ), + UiMode::NameStack { + name, + roots, + expanded, + } => ( + format!( + " new stack: {name}_ {} marked → {} ({expanded} workers with deps) \ + (Enter save · Esc cancel) ", + roots.len(), + roots.join(", ") + ), + styled_if(color, Style::default().fg(Color::Yellow)), + ), _ => { - let help = if area.width >= 86 { + // Gate on the string's own length, not a copied-by-hand number: + // `HELP_FULL` grew past its old `>= 86` gate once already, + // clipping its own tail (`? keys · q quit`) on any terminal + // narrower than its true width but still passing that gate. + let help = if area.width as usize >= HELP_FULL.chars().count() { HELP_FULL } else if area.width >= 64 { HELP_MID @@ -1418,10 +1923,102 @@ fn draw_confirm_overlay(f: &mut Frame, area: Rect, name: &str, dependents: &[Str ); } +/// Confirm-overwrite dialog for `n`: the typed name already names a stack in +/// the file. Mirrors `draw_confirm_overlay`'s shape — the new root list is +/// what would be written, so an overwrite is never a blind guess. +fn draw_confirm_save_overlay(f: &mut Frame, area: Rect, name: &str, roots: &[String], ctx: &UiCtx) { + let color = ctx.color_enabled; + let mut lines = vec![ + Line::from(""), + Line::from(Span::styled( + format!(" Overwrite stack {name}?"), + styled_if(color, confirm_prompt_style()), + )), + Line::from(""), + Line::from(Span::styled( + " new roots", + styled_if(color, Style::default().add_modifier(Modifier::BOLD)), + )), + ]; + for w in roots { + lines.push(worker_status_line(ctx, w)); + } + + let pinned = vec![ + Line::from(""), + Line::from(vec![ + Span::styled( + " y/Enter ", + styled_if(color, Style::default().fg(Color::Cyan)), + ), + Span::raw("overwrite "), + Span::styled("n/Esc ", styled_if(color, Style::default().fg(Color::Cyan))), + Span::raw("cancel"), + ]), + Line::from(""), + ]; + + draw_dialog( + f, + area, + " confirm overwrite ".to_string(), + lines, + pinned, + 58, + color, + ); +} + +/// Confirm-delete dialog for the picker's `x`: names the stack and the file +/// it will be removed from. Mirrors `draw_confirm_save_overlay`'s shape. +fn draw_confirm_delete_overlay(f: &mut Frame, area: Rect, name: &str, ctx: &UiCtx) { + let color = ctx.color_enabled; + let path_line = format!(" removes it from {}", ctx.config_path); + // The usual fixed 58 cols clips a long config path (e.g. one nested in a + // worktree) with no visible sign of truncation — grow to fit it instead, + // the way the Busy overlay already sizes itself to its message. + let width = 58.max(path_line.chars().count() as u16 + 6); + let lines = vec![ + Line::from(""), + Line::from(Span::styled( + format!(" Delete stack {name}?"), + styled_if(color, confirm_prompt_style()), + )), + Line::from(""), + Line::from(Span::styled(path_line, styled_if(color, hint_style()))), + ]; + + let pinned = vec![ + Line::from(""), + Line::from(vec![ + Span::styled( + " y/Enter ", + styled_if(color, Style::default().fg(Color::Cyan)), + ), + Span::raw("delete "), + Span::styled("n/Esc ", styled_if(color, Style::default().fg(Color::Cyan))), + Span::raw("cancel"), + ]), + Line::from(""), + ]; + + draw_dialog( + f, + area, + " confirm delete ".to_string(), + lines, + pinned, + width, + color, + ); +} + fn draw_help_overlay(f: &mut Frame, area: Rect, color: bool) { let keys = [ ("↑ ↓ k j", "select worker"), ("g G", "jump to first / last worker"), + ("Space", "mark worker for a new stack"), + ("n", "new stack from marked workers"), ("s", "start selected worker"), ("x", "stop selected worker"), ("r", "restart selected + dependents"), @@ -1432,7 +2029,7 @@ fn draw_help_overlay(f: &mut Frame, area: Rect, color: bool) { ("+ -", "resize the log pane"), ("/", "filter workers by name"), ("e", "start the iii engine"), - ("Ctrl+u", "start the harness stack"), + ("Ctrl+u", "start stack (picker: x delete · * default)"), ("Ctrl+a", "start all managed workers"), ("?", "toggle this help"), ("q", "quit (workers keep running)"), @@ -1529,3 +2126,474 @@ fn status_icon(status: &str, spinner_frame: usize) -> &'static str { _ => "○", } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Shared worker fixture for table-rendering tests: a real name, stable + /// defaults for everything else that don't matter to the assertions. + fn view_fixture(name: &str) -> WorkerView { + WorkerView { + name: name.to_string(), + group: WorkerGroup::Other, + spawnable: true, + display_status: "stopped".to_string(), + process_status: "stopped".to_string(), + engine_status: "—".to_string(), + local_pid: None, + uptime: "—".to_string(), + exit_code: None, + ui_watch: None, + } + } + + /// Render just the worker table into an in-memory buffer, the way + /// `draw_ui` would for one frame — for tests that only care about table + /// content, not the full dashboard chrome. No filter; selection lands on + /// the first worker row, same as a fresh dashboard. + fn render_table_for_test( + views: &[WorkerView], + marked: &std::collections::HashSet, + width: u16, + height: u16, + ) -> ratatui::buffer::Buffer { + let display_rows = build_display_rows(views, ""); + let mut table_state = TableState::default(); + table_state.select(first_worker_row(&display_rows)); + let ctx = UiCtx { + engine_url: "", + repo_branch: None, + current_stack: "s", + default_stack: "s", + config_path: "workers-dev.yaml", + stacks: &[], + views, + marked, + engine_error: None, + display_rows: &display_rows, + mode: &UiMode::Dashboard, + filter: "", + selected_name: None, + log_lines: &[], + log_scroll: 0, + follow: true, + log_height: LOG_HEIGHT_DEFAULT, + table_width: TABLE_PANE_WIDTH, + spinner_frame: 0, + color_enabled: false, + error: None, + }; + let backend = ratatui::backend::TestBackend::new(width, height); + let mut terminal = Terminal::new(backend).unwrap(); + terminal + .draw(|f| draw_table(f, f.area(), &mut table_state, &ctx)) + .unwrap(); + terminal.backend().buffer().clone() + } + + /// Flatten a rendered buffer into one string, so a test can assert on + /// content without caring which exact cell it landed in. + fn buffer_text(buf: &ratatui::buffer::Buffer) -> String { + let mut out = String::new(); + for y in 0..buf.area.height { + for x in 0..buf.area.width { + out.push_str(buf[(x, y)].symbol()); + } + out.push('\n'); + } + out + } + + /// Regression test for a prior review finding: a group-header row used to + /// be a single Cell sized to the Worker column (`Constraint::Length(24)`). + /// Ratatui's `Table` has no colspan, so that clipped any label past 24 + /// chars — a real risk now that stack names are user-chosen. `draw_table` + /// repaints visible header rows spanning the table's full width; this + /// pins that the full label survives (not a 24-char prefix) AND keeps + /// `group_header_style`'s color. + #[test] + fn group_header_row_spans_full_width_not_clipped() { + let stack_name = "a-very-long-worktree-stack-name"; + let count = 999; + let label = format!( + "── {} ({count}) ──", + crate::status::group_label(WorkerGroup::Stack, stack_name) + ); + assert!( + label.chars().count() > 24, + "test label must exceed the Worker column width to exercise the bug" + ); + + let display_rows = vec![DisplayRow { + kind: DisplayRowKind::Header(WorkerGroup::Stack, count), + }]; + let marked = std::collections::HashSet::new(); + let ctx = UiCtx { + engine_url: "", + repo_branch: None, + current_stack: stack_name, + default_stack: stack_name, + config_path: "workers-dev.yaml", + stacks: &[], + views: &[], + marked: &marked, + engine_error: None, + display_rows: &display_rows, + mode: &UiMode::Dashboard, + filter: "", + selected_name: None, + log_lines: &[], + log_scroll: 0, + follow: true, + log_height: LOG_HEIGHT_DEFAULT, + table_width: TABLE_PANE_WIDTH, + spinner_frame: 0, + color_enabled: true, + error: None, + }; + + // TestBackend renders to an in-memory buffer — no real terminal + // needed. 60x4 gives exactly: top border, column header, one data + // row (our stack header), bottom border. + let backend = ratatui::backend::TestBackend::new(60, 4); + let mut terminal = Terminal::new(backend).unwrap(); + let mut table_state = TableState::default(); + terminal + .draw(|f| draw_table(f, f.area(), &mut table_state, &ctx)) + .unwrap(); + + let buf = terminal.backend().buffer(); + let row: String = (1..59).map(|x| buf[(x, 2)].symbol()).collect(); + assert_eq!(row.trim_end(), label, "header row was clipped: {row:?}"); + // group_header_style(Stack) = Style::default().fg(Cyan); with color + // enabled the painted label must carry it, not just the raw text. + assert_eq!( + buf[(1, 2)].fg, + Color::Cyan, + "header row lost its group_header_style color" + ); + } + + /// Companion to the above: the header-paint loop positions each visible + /// header by its own offset-relative row, not just "the top of the + /// window" — this pins that after the table has scrolled (offset > 0), a + /// header that isn't display row 0 still lands on the correct screen + /// line. Also `TestBackend`, still no real terminal. + #[test] + fn group_header_row_paints_correctly_after_scrolling() { + let stack_name = "s"; + let views = vec![ + view_fixture("a1"), + view_fixture("a2"), + view_fixture("b1"), + view_fixture("b2"), + ]; + // Two groups, two headers: [H(Stack,2), W(a1), W(a2), H(Other,2), W(b1), W(b2)]. + let display_rows = vec![ + DisplayRow { + kind: DisplayRowKind::Header(WorkerGroup::Stack, 2), + }, + DisplayRow { + kind: DisplayRowKind::Worker(0), + }, + DisplayRow { + kind: DisplayRowKind::Worker(1), + }, + DisplayRow { + kind: DisplayRowKind::Header(WorkerGroup::Other, 2), + }, + DisplayRow { + kind: DisplayRowKind::Worker(2), + }, + DisplayRow { + kind: DisplayRowKind::Worker(3), + }, + ]; + let label_other = format!( + "── {} (2) ──", + crate::status::group_label(WorkerGroup::Other, stack_name) + ); + let marked = std::collections::HashSet::new(); + let ctx = UiCtx { + engine_url: "", + repo_branch: None, + current_stack: stack_name, + default_stack: stack_name, + config_path: "workers-dev.yaml", + stacks: &[], + views: &views, + marked: &marked, + engine_error: None, + display_rows: &display_rows, + mode: &UiMode::Dashboard, + filter: "", + selected_name: None, + log_lines: &[], + log_scroll: 0, + follow: true, + log_height: LOG_HEIGHT_DEFAULT, + table_width: TABLE_PANE_WIDTH, + spinner_frame: 0, + color_enabled: false, + error: None, + }; + + // height=6: border + column header + 3 data rows + border. Selecting + // the last row (display index 4, "b1") forces a scroll so display + // rows 2..=4 are visible — landing the second header (display index + // 3) on screen row 3, not row 2 (the "first data row" position the + // un-scrolled test above pins). + let backend = ratatui::backend::TestBackend::new(40, 6); + let mut terminal = Terminal::new(backend).unwrap(); + let mut table_state = TableState::default(); + table_state.select(Some(4)); + terminal + .draw(|f| draw_table(f, f.area(), &mut table_state, &ctx)) + .unwrap(); + assert_eq!( + table_state.offset(), + 2, + "test setup must actually scroll (offset > 0) to be meaningful" + ); + + let buf = terminal.backend().buffer(); + let row: String = (1..39).map(|x| buf[(x, 3)].symbol()).collect(); + assert_eq!( + row.trim_end(), + label_other, + "scrolled header row painted at the wrong screen line: {row:?}" + ); + } + + /// Important fix: ratatui's `get_row_bounds` only ever lowers the render + /// offset when `selected < offset`, and the selection can never land on + /// display row 0 here — it's a group header, and the up/down nav helpers + /// skip headers. So once a scroll pushed the offset to 1 it used to + /// never fall back to 0 on its own, permanently hiding the first group + /// header even after scrolling back to the very top of the list — the + /// one place the dashboard names the current stack. Scroll down, then + /// return to the first worker row, and the header must reappear. + #[test] + fn group_header_row_becomes_visible_again_after_returning_to_first_worker() { + let stack_name = "s"; + let views = vec![ + view_fixture("a1"), + view_fixture("a2"), + view_fixture("b1"), + view_fixture("b2"), + ]; + // Same fixture as the scrolling test above: + // [H(Stack,2), W(a1), W(a2), H(Other,2), W(b1), W(b2)]. + let display_rows = vec![ + DisplayRow { + kind: DisplayRowKind::Header(WorkerGroup::Stack, 2), + }, + DisplayRow { + kind: DisplayRowKind::Worker(0), + }, + DisplayRow { + kind: DisplayRowKind::Worker(1), + }, + DisplayRow { + kind: DisplayRowKind::Header(WorkerGroup::Other, 2), + }, + DisplayRow { + kind: DisplayRowKind::Worker(2), + }, + DisplayRow { + kind: DisplayRowKind::Worker(3), + }, + ]; + let label_stack = format!( + "── {} (2) ──", + crate::status::group_label(WorkerGroup::Stack, stack_name) + ); + let marked = std::collections::HashSet::new(); + let ctx = UiCtx { + engine_url: "", + repo_branch: None, + current_stack: stack_name, + default_stack: stack_name, + config_path: "workers-dev.yaml", + stacks: &[], + views: &views, + marked: &marked, + engine_error: None, + display_rows: &display_rows, + mode: &UiMode::Dashboard, + filter: "", + selected_name: None, + log_lines: &[], + log_scroll: 0, + follow: true, + log_height: LOG_HEIGHT_DEFAULT, + table_width: TABLE_PANE_WIDTH, + spinner_frame: 0, + color_enabled: false, + error: None, + }; + + // Same 40x6 backend as the sibling scrolling test. + let backend = ratatui::backend::TestBackend::new(40, 6); + let mut terminal = Terminal::new(backend).unwrap(); + let mut table_state = TableState::default(); + + // Scroll down onto the last row first, exactly like the sibling test + // above — this pushes the offset to 2, scrolling the first header + // (display row 0) out of view. + table_state.select(Some(4)); + terminal + .draw(|f| draw_table(f, f.area(), &mut table_state, &ctx)) + .unwrap(); + assert_eq!( + table_state.offset(), + 2, + "test setup must actually scroll (offset > 0) to be meaningful" + ); + + // Now return to the first worker row (display row 1, "a1") — the + // offset must snap back to 0 so the header above it is visible again. + table_state.select(first_worker_row(&display_rows)); + terminal + .draw(|f| draw_table(f, f.area(), &mut table_state, &ctx)) + .unwrap(); + assert_eq!( + table_state.offset(), + 0, + "offset must snap back to 0 once the first worker row is selected again" + ); + + let buf = terminal.backend().buffer(); + let row: String = (1..39).map(|x| buf[(x, 2)].symbol()).collect(); + assert_eq!( + row.trim_end(), + label_stack, + "first group header did not reappear after returning to the top: {row:?}" + ); + } + + #[test] + fn marked_rows_show_a_check_and_the_title_counts_them() { + let mut views = vec![view_fixture("alpha"), view_fixture("beta")]; + crate::status::assign_view_groups( + &mut views, + &["alpha".to_string(), "beta".to_string()] + .into_iter() + .collect(), + ); + let marked: std::collections::HashSet = ["beta".to_string()].into_iter().collect(); + let buf = render_table_for_test(&views, &marked, 40, 8); + + // Row-scoped, not a whole-buffer `contains` — a regression that + // hardcoded the mark unconditionally (marking every row, or none) + // would still pass a bare "a ✓ exists somewhere" check. Row layout + // for this 40x8 backend: y=0 border, y=1 column header, y=2 the + // "stack:s" group header (both workers are members), y=3 alpha + // (unmarked), y=4 beta (marked). + let alpha_row: String = (1..39).map(|x| buf[(x, 3)].symbol()).collect(); + let beta_row: String = (1..39).map(|x| buf[(x, 4)].symbol()).collect(); + assert!( + beta_row.starts_with('✓'), + "marked worker's row must start with a check:\n{beta_row:?}" + ); + assert!( + alpha_row.starts_with(' '), + "unmarked worker's row must start with a space, not a check:\n{alpha_row:?}" + ); + + let text = buffer_text(&buf); + assert!(text.contains("1 marked"), "title must count marks:\n{text}"); + } + + #[test] + fn marked_worker_name_at_the_column_budget_is_not_truncated() { + // Longest real worker name (provider-openai-codex, verified against + // the */iii.worker.yaml directories) is 21 chars. Plus the 3-char + // mark+icon+space prefix, that's exactly `Constraint::Length(24)` on + // the Worker column — zero margin. Rendered at the table's real pane + // width (TABLE_PANE_WIDTH), not a narrow test backend, so this + // exercises the actual column budget rather than width pressure from + // an undersized buffer. + let name = "provider-openai-codex"; + assert_eq!( + name.len(), + 21, + "fixture drifted from the real worker name length" + ); + let views = vec![view_fixture(name)]; + let marked: std::collections::HashSet = [name.to_string()].into_iter().collect(); + let buf = render_table_for_test(&views, &marked, TABLE_PANE_WIDTH, 6); + + // y=0 border, y=1 column header, y=2 the single group header, y=3 + // the one worker row. + let row: String = (1..TABLE_PANE_WIDTH - 1) + .map(|x| buf[(x, 3)].symbol()) + .collect(); + assert!( + row.contains(name), + "21-char worker name must survive the Worker column untruncated:\n{row:?}" + ); + } + + /// Minor fix: `HELP_FULL` grew past its old hardcoded `>= 86` gate, so a + /// terminal narrower than the text but still passing that gate rendered + /// it anyway, clipping its tail (`? keys · q quit`). `draw_footer` now + /// gates on the string's own length, so the tier can never outgrow its + /// gate again; this also pins that `n` (new stack) is documented in it. + #[test] + fn footer_help_never_clips_and_documents_new_stack() { + assert!( + HELP_FULL.contains("n new stack"), + "the `n` (new stack) key must be documented in the full help tier" + ); + + let marked = std::collections::HashSet::new(); + let display_rows: Vec = Vec::new(); + let ctx = UiCtx { + engine_url: "", + repo_branch: None, + current_stack: "s", + default_stack: "s", + config_path: "workers-dev.yaml", + stacks: &[], + views: &[], + marked: &marked, + engine_error: None, + display_rows: &display_rows, + mode: &UiMode::Dashboard, + filter: "", + selected_name: None, + log_lines: &[], + log_scroll: 0, + follow: true, + log_height: LOG_HEIGHT_DEFAULT, + table_width: TABLE_PANE_WIDTH, + spinner_frame: 0, + color_enabled: false, + error: None, + }; + let render = |width: u16| -> String { + let backend = ratatui::backend::TestBackend::new(width, 1); + let mut terminal = Terminal::new(backend).unwrap(); + terminal.draw(|f| draw_footer(f, f.area(), &ctx)).unwrap(); + (0..width) + .map(|x| terminal.backend().buffer()[(x, 0)].symbol().to_string()) + .collect() + }; + + // At exactly HELP_FULL's own width, the full text — including its + // tail — must render intact. + let full_width = HELP_FULL.chars().count() as u16; + assert!( + render(full_width).trim_end().ends_with("q quit"), + "HELP_FULL's own gate must not clip its tail" + ); + // One column narrower must fall back to a shorter tier rather than + // render (and clip) HELP_FULL. + assert!( + !render(full_width - 1).contains("ui-watch"), + "a terminal narrower than HELP_FULL must not render (and clip) it" + ); + } +} diff --git a/workers-dev/src/tui/stacks.rs b/workers-dev/src/tui/stacks.rs new file mode 100644 index 000000000..8529a72d4 --- /dev/null +++ b/workers-dev/src/tui/stacks.rs @@ -0,0 +1,581 @@ +//! Stack management UI: the Ctrl+u picker, creating a stack from marked +//! workers, and deleting or defaulting stacks from that same picker. Split +//! out of `tui/mod.rs`, which owns the dashboard loop and was already long +//! before stacks arrived. + +use std::collections::HashSet; +use std::path::Path; + +use anyhow::{Context, Result}; +use crossterm::event::{KeyCode, KeyEvent}; +use ratatui::layout::Rect; +use ratatui::style::{Color, Modifier, Style}; +use ratatui::text::{Line, Span}; +use ratatui::Frame; + +use crate::status::WorkerView; + +use super::theme::hint_style; +use super::{draw_dialog, styled_if, UiCtx, UiMode}; + +/// Move the picker selection by one row, clamped to the list's bounds. +/// Every stack is reachable, including ones with no startable roots: `x` +/// (delete) must be able to reach them (deleting an unstartable stack is +/// exactly what you want the picker for), and Enter/`*` separately refuse to +/// act on an empty one rather than making it unreachable. +pub(super) fn move_stack_selection( + stacks: &[(String, Vec)], + current: usize, + down: bool, +) -> usize { + if down { + (current + 1).min(stacks.len().saturating_sub(1)) + } else { + current.saturating_sub(1) + } +} + +/// What the picker's keys ask the dashboard loop to do. +pub(super) enum PickerAction { + /// Switch the current stack to this one and start it (Enter). + Start(String), + /// Remove this stack from the config file (x) — caller confirms first. + Delete(String), + /// Point `default_stack:` at this one (*). + MakeDefault(String), +} + +/// Stack-picker keys. Enter on a startable stack just returns +/// `PickerAction::Start` — `mode` is left alone here. The caller only commits +/// (current_stack, Busy, spawns the start) once `stack_members` actually +/// resolves, so a lookup error can't strand the Busy dialog up with nothing +/// in flight. `x` returns Delete for the highlighted stack regardless of +/// whether it has startable roots — an empty stack is exactly the one you'd +/// want to delete. `*` shares Enter's non-empty-roots guard instead: an empty +/// stack can never become the default. Esc/q cancels back to the dashboard. +pub(super) fn handle_stack_picker_key( + key: KeyEvent, + mode: &mut UiMode, + stacks: &[(String, Vec)], +) -> Option { + let UiMode::StackPicker { selected } = mode else { + return None; + }; + match key.code { + KeyCode::Up | KeyCode::Char('k') => { + *selected = move_stack_selection(stacks, *selected, false); + } + KeyCode::Down | KeyCode::Char('j') => { + *selected = move_stack_selection(stacks, *selected, true); + } + KeyCode::Enter => { + if let Some((name, roots)) = stacks.get(*selected) { + if !roots.is_empty() { + return Some(PickerAction::Start(name.clone())); + } + } + } + KeyCode::Char('x') => { + if let Some((name, _)) = stacks.get(*selected) { + return Some(PickerAction::Delete(name.clone())); + } + } + KeyCode::Char('*') => { + // Unlike `x`, this is guarded: writing `default_stack:` at an + // empty-roots stack would pass `write_verified`'s validation + // (it checks stack-key types, not default membership/emptiness) + // and then brick the next `Config::load` — the exact failure + // class the previous task's Critical fix was about. + if let Some((name, roots)) = stacks.get(*selected) { + if !roots.is_empty() { + return Some(PickerAction::MakeDefault(name.clone())); + } + } + } + KeyCode::Esc | KeyCode::Char('q') => *mode = UiMode::Dashboard, + _ => {} + } + None +} + +/// Why `name` can't be deleted right now, or `None` if the picker should go +/// ahead and open the confirm dialog. The only reason today: deleting the +/// default stack would leave `default_stack:` pointing at nothing, and +/// `Config::load` would refuse to start on the next launch — this guard is +/// the one thing standing between the picker and that outcome, so it's +/// pulled out here to be unit-tested on its own rather than trusted to a +/// `rustc`-checked string literal buried in `run()`'s dispatch. +pub(super) fn refuse_delete_reason(name: &str, default_stack: &str) -> Option { + if name == default_stack { + Some("set another default first (*) before deleting the default stack".to_string()) + } else { + None + } +} + +/// Stack picker: one row per configured stack (`*` marks the default, the +/// current stack is bold, empty ones are dimmed but still reachable — `x` +/// can delete them). Enter switches the session's current stack AND starts +/// it. +pub(super) fn draw_stack_picker_overlay(f: &mut Frame, area: Rect, selected: usize, ctx: &UiCtx) { + let color = ctx.color_enabled; + let mut lines = vec![Line::from("")]; + for (i, (name, roots)) in ctx.stacks.iter().enumerate() { + let cursor = if i == selected { "▸" } else { " " }; + let marker = if name == ctx.default_stack { "*" } else { " " }; + let n = roots.len(); + let s = if n == 1 { "" } else { "s" }; + let suffix = if roots.is_empty() { + " (nothing startable)".to_string() + } else { + format!(" {n} root{s}") + }; + let style = if roots.is_empty() { + styled_if(color, hint_style()) + } else if i == selected { + styled_if(color, Style::default().fg(Color::Cyan)).add_modifier(Modifier::BOLD) + } else if name == ctx.current_stack { + Style::default().add_modifier(Modifier::BOLD) + } else { + Style::default() + }; + lines.push(Line::from(Span::styled( + format!(" {cursor} {marker} {name:<18}{suffix}"), + style, + ))); + } + lines.push(Line::from("")); + lines.push(Line::from(Span::styled( + format!( + " * default: {} current: {}", + ctx.default_stack, ctx.current_stack + ), + styled_if(color, hint_style()), + ))); + let pinned = vec![ + Line::from(""), + Line::from(vec![ + Span::styled( + " Enter ", + styled_if(color, Style::default().fg(Color::Cyan)), + ), + Span::raw("switch + start "), + Span::styled("x ", styled_if(color, Style::default().fg(Color::Cyan))), + Span::raw("delete "), + Span::styled("* ", styled_if(color, Style::default().fg(Color::Cyan))), + Span::raw("default "), + Span::styled("Esc ", styled_if(color, Style::default().fg(Color::Cyan))), + Span::raw("cancel"), + ]), + Line::from(""), + ]; + draw_dialog( + f, + area, + " start stack ".to_string(), + lines, + pinned, + 58, + color, + ); +} + +/// The marked workers in dashboard order — the order the user sees, so the +/// saved file reads the way the list did. +pub(super) fn roots_from_marks(views: &[WorkerView], marked: &HashSet) -> Vec { + views + .iter() + .filter(|v| marked.contains(&v.name)) + .map(|v| v.name.clone()) + .collect() +} + +/// Name-prompt keys. Enter on a non-empty name returns `(name, roots)`; the +/// caller decides whether that name overwrites an existing stack. Esc cancels. +/// Characters that would need YAML quoting are ignored as typed. +pub(super) fn handle_name_key(key: KeyEvent, mode: &mut UiMode) -> Option<(String, Vec)> { + let UiMode::NameStack { name, roots, .. } = mode else { + return None; + }; + match key.code { + KeyCode::Char(c) if crate::config_write::valid_stack_name(&c.to_string()) => name.push(c), + KeyCode::Backspace => { + name.pop(); + } + KeyCode::Enter if !name.is_empty() => return Some((name.clone(), roots.clone())), + KeyCode::Esc => *mode = UiMode::Dashboard, + _ => {} + } + None +} + +/// Read `path`'s current contents, or an empty string if it doesn't exist yet +/// (genuinely "nothing to load yet", e.g. a fresh repo's first save). Any +/// other read failure (bad permissions, a directory in the way, invalid +/// UTF-8) must NOT be treated the same way — that would build a fresh +/// document out of nothing and have `write_verified` rename it over whatever +/// was actually there. +fn read_or_empty(path: &Path) -> Result { + match std::fs::read_to_string(path) { + Ok(text) => Ok(text), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(String::new()), + Err(err) => Err(err).with_context(|| format!("read {}", path.display())), + } +} + +/// Write `name`'s stack into the config file, creating the file if needed. +pub(super) fn save_stack(path: &Path, name: &str, roots: &[String]) -> Result<()> { + let text = read_or_empty(path)?; + let next = crate::config_write::upsert_stack(&text, name, roots)?; + crate::config_write::write_verified(path, &next) + .with_context(|| format!("save stack {name} to {}", path.display())) +} + +/// Remove `name`'s entry from the config file. +pub(super) fn delete_stack(path: &Path, name: &str) -> Result<()> { + let text = read_or_empty(path)?; + let next = crate::config_write::remove_stack(&text, name)?; + crate::config_write::write_verified(path, &next) + .with_context(|| format!("delete stack {name} from {}", path.display())) +} + +/// Point `default_stack:` at `name`. +pub(super) fn set_default(path: &Path, name: &str) -> Result<()> { + let text = read_or_empty(path)?; + let next = crate::config_write::set_default_stack(&text, name)?; + crate::config_write::write_verified(path, &next) + .with_context(|| format!("set default stack {name} in {}", path.display())) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::discover::WorkerGroup; + + fn stacks() -> Vec<(String, Vec)> { + vec![ + ("harness".to_string(), vec!["harness".to_string()]), + ("ghost".to_string(), Vec::new()), + ("console".to_string(), vec!["console".to_string()]), + ] + } + + /// Shared worker fixture — mirrors `status.rs`'s test fixture of the same + /// name/shape. + fn view(name: &str) -> WorkerView { + WorkerView { + name: name.to_string(), + group: WorkerGroup::Other, + spawnable: true, + display_status: "stopped".to_string(), + process_status: "stopped".to_string(), + engine_status: "—".to_string(), + local_pid: None, + uptime: "—".to_string(), + exit_code: None, + ui_watch: None, + } + } + + #[test] + fn stack_picker_selection_reaches_empty_stacks_and_clamps() { + let stacks = stacks(); + // `ghost` (index 1) has no roots but must still be reachable — it's + // the only way `x` can ever delete it. + assert_eq!(move_stack_selection(&stacks, 0, true), 1); + assert_eq!(move_stack_selection(&stacks, 2, false), 1); + assert_eq!(move_stack_selection(&stacks, 2, true), 2); // clamp end + assert_eq!(move_stack_selection(&stacks, 0, false), 0); // clamp start + } + + #[test] + fn stack_picker_enter_switches_and_esc_cancels() { + let stacks = stacks(); + + // Enter on a startable stack returns its name but leaves `mode` + // untouched — the caller only sets Busy once `stack_members` + // resolves, so a failed lookup can't strand a Busy dialog with + // nothing in flight. + let mut mode = UiMode::StackPicker { selected: 2 }; + let chosen = handle_stack_picker_key(KeyEvent::from(KeyCode::Enter), &mut mode, &stacks); + assert!(matches!(chosen, Some(PickerAction::Start(ref n)) if n == "console")); + assert!(matches!(mode, UiMode::StackPicker { selected: 2 })); + + // Enter on an unstartable (empty) stack does nothing. + let mut mode = UiMode::StackPicker { selected: 1 }; + let chosen = handle_stack_picker_key(KeyEvent::from(KeyCode::Enter), &mut mode, &stacks); + assert!(chosen.is_none()); + assert!(matches!(mode, UiMode::StackPicker { selected: 1 })); + + let mut mode = UiMode::StackPicker { selected: 0 }; + let chosen = handle_stack_picker_key(KeyEvent::from(KeyCode::Esc), &mut mode, &stacks); + assert!(chosen.is_none()); + assert!(matches!(mode, UiMode::Dashboard)); + } + + #[test] + fn picker_keys_return_start_delete_and_default_actions() { + let stacks = vec![ + ("harness".to_string(), vec!["harness".to_string()]), + ("ghost".to_string(), Vec::new()), + ("console".to_string(), vec!["console".to_string()]), + ]; + + let mut mode = UiMode::StackPicker { selected: 2 }; + assert!(matches!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Enter), &mut mode, &stacks), + Some(PickerAction::Start(ref n)) if n == "console" + )); + assert!(matches!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Char('x')), &mut mode, &stacks), + Some(PickerAction::Delete(ref n)) if n == "console" + )); + assert!(matches!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Char('*')), &mut mode, &stacks), + Some(PickerAction::MakeDefault(ref n)) if n == "console" + )); + + // An unstartable stack can still be deleted, but not started or made + // default (the latter would write a `default_stack:` that bricks the + // next `Config::load`). + let mut mode = UiMode::StackPicker { selected: 1 }; + assert!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Enter), &mut mode, &stacks).is_none() + ); + assert!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Char('*')), &mut mode, &stacks) + .is_none() + ); + assert!(matches!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Char('x')), &mut mode, &stacks), + Some(PickerAction::Delete(ref n)) if n == "ghost" + )); + + // Esc still cancels. + let mut mode = UiMode::StackPicker { selected: 0 }; + assert!( + handle_stack_picker_key(KeyEvent::from(KeyCode::Esc), &mut mode, &stacks).is_none() + ); + assert!(matches!(mode, UiMode::Dashboard)); + } + + #[test] + fn refuse_delete_reason_blocks_only_the_default() { + let reason = refuse_delete_reason("console", "console").unwrap(); + assert!( + reason.contains("before deleting the default stack"), + "{reason:?}" + ); + assert!(refuse_delete_reason("console", "harness").is_none()); + } + + #[test] + fn delete_and_set_default_edit_the_file() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::write( + &path, + "stacks:\n console:\n - console\n tiny:\n - session-manager\n", + ) + .unwrap(); + + set_default(&path, "console").unwrap(); + assert!(std::fs::read_to_string(&path) + .unwrap() + .contains("default_stack: console")); + + delete_stack(&path, "tiny").unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains(" console:")); + assert!(!text.contains("tiny")); + + // A stack that isn't in the file (e.g. the built-in harness) says so. + let err = delete_stack(&path, "harness").unwrap_err(); + assert!( + err.to_string().contains("not defined in this file"), + "{err:#}" + ); + } + + /// A failed write (`remove_stack` refusing a duplicated `stacks:` key) + /// must leave the file exactly as it was — mirrors + /// `save_stack_leaves_the_file_untouched_on_error`. + #[test] + fn delete_stack_leaves_the_file_untouched_on_error() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + let original = "stacks:\n a:\n - x\nstacks:\n b:\n - y\n"; + std::fs::write(&path, original).unwrap(); + + let err = delete_stack(&path, "a").unwrap_err(); + assert!(err.to_string().contains("more than once"), "{err:#}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), original); + } + + /// A read failure that isn't "file doesn't exist yet" must propagate — + /// mirrors `save_stack_propagates_read_errors_other_than_not_found`, and + /// is the regression guard for `delete_stack`'s own NotFound-only default. + #[test] + fn delete_stack_propagates_read_errors_other_than_not_found() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::create_dir(&path).unwrap(); + + let err = delete_stack(&path, "console").unwrap_err(); + assert!( + err.to_string().contains(&path.display().to_string()), + "{err:#}" + ); + assert!(path.is_dir()); + } + + /// A failed write (`set_default_stack` refusing a duplicated + /// `default_stack:` key) must leave the file exactly as it was — mirrors + /// `save_stack_leaves_the_file_untouched_on_error`. + #[test] + fn set_default_leaves_the_file_untouched_on_error() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + let original = "default_stack: a\ndefault_stack: b\n"; + std::fs::write(&path, original).unwrap(); + + let err = set_default(&path, "console").unwrap_err(); + assert!(err.to_string().contains("more than once"), "{err:#}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), original); + } + + /// A read failure that isn't "file doesn't exist yet" must propagate — + /// mirrors `save_stack_propagates_read_errors_other_than_not_found`, and + /// is the regression guard for `set_default`'s own NotFound-only default. + #[test] + fn set_default_propagates_read_errors_other_than_not_found() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::create_dir(&path).unwrap(); + + let err = set_default(&path, "console").unwrap_err(); + assert!( + err.to_string().contains(&path.display().to_string()), + "{err:#}" + ); + assert!(path.is_dir()); + } + + #[test] + fn roots_from_marks_follows_dashboard_order() { + let mut views = vec![view("zeta"), view("alpha"), view("mid")]; + // zeta is a stack member: it sorts into the Stack group, ahead of the + // alphabetically-earlier Other group, so dashboard order is + // zeta, alpha, mid — deliberately NOT alphabetical. A `marks.sort()` + // reimplementation would produce alpha, zeta and fail this. + let members: HashSet = ["zeta".to_string()].into_iter().collect(); + crate::status::assign_view_groups(&mut views, &members); + let marked: HashSet = ["zeta".to_string(), "alpha".to_string()] + .into_iter() + .collect(); + assert_eq!( + roots_from_marks(&views, &marked), + vec!["zeta".to_string(), "alpha".to_string()] + ); + } + + #[test] + fn name_key_edits_accepts_and_cancels() { + let roots = vec!["console".to_string()]; + + // Valid characters append; invalid ones are ignored. + let mut mode = UiMode::NameStack { + name: String::new(), + roots: roots.clone(), + expanded: 3, + }; + for c in ['c', 'o', ' ', 'n', ':', '-', '1'] { + assert!(handle_name_key(KeyEvent::from(KeyCode::Char(c)), &mut mode).is_none()); + } + let UiMode::NameStack { name, .. } = &mode else { + panic!("still naming") + }; + assert_eq!(name, "con-1"); + + // Backspace edits. + handle_name_key(KeyEvent::from(KeyCode::Backspace), &mut mode); + let UiMode::NameStack { name, .. } = &mode else { + panic!("still naming") + }; + assert_eq!(name, "con-"); + + // Enter returns the name and its roots. + let out = handle_name_key(KeyEvent::from(KeyCode::Enter), &mut mode).unwrap(); + assert_eq!(out, ("con-".to_string(), roots.clone())); + + // Enter on an empty name does nothing. + let mut mode = UiMode::NameStack { + name: String::new(), + roots: roots.clone(), + expanded: 3, + }; + assert!(handle_name_key(KeyEvent::from(KeyCode::Enter), &mut mode).is_none()); + assert!(matches!(mode, UiMode::NameStack { .. })); + + // Esc cancels. + handle_name_key(KeyEvent::from(KeyCode::Esc), &mut mode); + assert!(matches!(mode, UiMode::Dashboard)); + } + + #[test] + fn save_stack_creates_then_updates_the_file() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + + save_stack(&path, "console", &["console".to_string()]).unwrap(); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "stacks:\n console:\n - console\n" + ); + + save_stack( + &path, + "console", + &["console".to_string(), "state".to_string()], + ) + .unwrap(); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "stacks:\n console:\n - console\n - state\n" + ); + } + + /// A failed write (here: `upsert_stack` refusing a duplicated `stacks:` + /// key) must leave the file exactly as it was — this is the property the + /// error-banner path in `save_and_adopt_stack` depends on. + #[test] + fn save_stack_leaves_the_file_untouched_on_error() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + let original = "stacks:\n a:\n - x\nstacks:\n b:\n - y\n"; + std::fs::write(&path, original).unwrap(); + + let err = save_stack(&path, "console", &["console".to_string()]).unwrap_err(); + assert!(err.to_string().contains("more than once"), "{err:#}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), original); + } + + /// A read failure that isn't "file doesn't exist yet" (here: a directory + /// sitting where the config file should be) must propagate, not be + /// treated as an empty file — that would silently overwrite whatever was + /// actually unreadable there with a fresh single-stack document. + #[test] + fn save_stack_propagates_read_errors_other_than_not_found() { + let tmp = tempfile::TempDir::new().unwrap(); + let path = tmp.path().join("workers-dev.yaml"); + std::fs::create_dir(&path).unwrap(); + + let err = save_stack(&path, "console", &["console".to_string()]).unwrap_err(); + assert!( + err.to_string().contains(&path.display().to_string()), + "{err:#}" + ); + // Untouched — still a directory, not clobbered by a written file. + assert!(path.is_dir()); + } +} diff --git a/workers-dev/src/tui/theme.rs b/workers-dev/src/tui/theme.rs index a7ccbf5a1..2fe21451c 100644 --- a/workers-dev/src/tui/theme.rs +++ b/workers-dev/src/tui/theme.rs @@ -29,7 +29,7 @@ pub fn selection_row_style() -> Style { pub fn group_header_style(group: WorkerGroup) -> Style { match group { - WorkerGroup::HarnessStack => Style::default().fg(Color::Cyan), + WorkerGroup::Stack => Style::default().fg(Color::Cyan), WorkerGroup::Other => muted_cell_style(), } }