(MOT-4377) feat(workers-dev): named stacks — select a default, create and manage them from the TUI - #742
Conversation
…tent The Config example's `workers:` override omitted `console` while its `stacks.console:` entry named it as a root. Copied verbatim, the `console` stack silently ends up containing only `session-manager` — config.rs drops the missing root with a warning that the TUI's alternate screen then hides. Add `console` to the example's `workers:` list so every root any example stack references is in the managed set.
start_stack passed a stack's roots straight to start_workers, whose names.is_empty() branch means "start every managed worker" (~50 concurrent cargo run builds). A stack can legitimately end up with empty roots when every root it names gets filtered out of the managed workers: set. It was unreachable only because every current caller happens to guard it. Bail in start_stack as soon as roots resolve empty, before start_workers ever talks to the engine. Also tightens the Ctrl+u stack picker: handle_stack_picker_key set UiMode::Busy on Enter before its caller validated the pick via stack_members(), so a failed lookup could strand the Busy dialog with nothing in flight. Busy now only gets set in the caller's Ok arm, alongside current_stack and the spawned start. And the picker's stacks[*selected] indexing is now stacks.get(*selected), a no-op on Some but no longer a raw index.
…ult they could set
…not parse `entry_key` recognised an existing `stacks:` entry by running it through `valid_stack_name`, which governs what this tool may *write*, not what `Config::load`/`parse_stacks` already accept reading back (any string key, so `my.stack`, `'quoted'`, and `tiny :` are all fully supported stacks today). A sibling entry in one of those shapes was invisible to the line scanner and got silently swallowed into a neighbouring entry's edit range, or made `remove_stack`'s "any entries left?" check false so the whole `stacks:` block was dropped — in one repro, truncating the file to 0 bytes, all while still producing text `validate_config_text` accepts. Fix in two layers: - `entry_key` now recognises any key shape `parse_stacks` would accept, rejecting only what's structurally not a key (empty, a list item, or stray whitespace around the name). - `ensure_block_entries_recognized` refuses the whole edit if any line in the block still isn't recognisable as an entry or a list item under one (e.g. `tiny :`, a space before the colon) — refusing beats mangling. Adds regression tests for a `my.stack` sibling surviving both `upsert_stack` and `remove_stack` on its neighbour, a `tiny :` entry causing a refusal, and an explicit 0-byte-file guard.
`validate_config_text` checked that `stacks:` keys were well-typed but never that `default_stack` names one of them. A write that left it dangling (e.g. deleting the stack it points at, or renaming one under it) passed verification and `write_verified` replaced the file — only for the very next `Config::load` to refuse to start. Bail when `default_stack` is set, isn't the built-in `harness`, and names no parsed stack. Also corrects the function's doc comment, which overclaimed it parses "exactly as load would" — it has no repo to discover workers against, so `load`'s managed-workers/stack-roots filtering never runs here.
…tack is empty The "default stack has no startable workers" check ran during `Config::load`, so a `workers:` allowlist that excludes every root of the default stack made `status`, `logs`, and the TUI all refuse to start — not just the start path. Before named stacks, only starting misbehaved this way; the user could still open the dashboard to see why. `Orchestrator::start_roots` already carries the same guard and is what actually starts a stack, so the load-time bail only bought an earlier message at the cost of every read-only command. Downgrade it to a warning, matching the neighbouring warn-and-drop messages, and update the Config struct's `default_stack` doc comment, which claimed roots were always non-empty.
ratatui 0.29'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 deliberately skip headers. So once a scroll pushed the offset to 1, it never fell back to 0 on its own, even after navigating back to the very first worker row: the top group header stayed permanently scrolled out of view for the rest of the session. That header is the only place the dashboard names the current stack (`group_label`), and creating/switching stacks is now routine, so losing that indicator permanently matters more than it used to. Force the offset back to 0 in `draw_table` whenever the selection lands on the first worker row, before handing off to `render_stateful_widget`.
…elp tier Four comments still described the implementation plan that produced this code rather than the code itself: `StackPicker`'s doc claimed `selected` indexes `config.stacks` (it indexes the session's `stacks` list, which diverges after any save/delete/default change) and that empty-roots rows are "skipped" (they're deliberately reachable, so `x` can delete them); three comments in `tui/mod.rs` and one in `tui/stacks.rs` referenced "Task 5" and "this task" by the implementation plan's task numbering. Reworded all five to describe the current code. Also: `HELP_FULL` grew to 108 (now 122, with `n` added) characters but stayed gated at `width >= 86`, so a terminal between the gate and the text's real width rendered it anyway and clipped its tail (`? keys · q quit`). `n` (new stack) was also missing from every footer tier despite being a routine key now. `draw_footer` gates `HELP_FULL` on its own character count instead of a hand-copied number, so this can't drift out of sync again, and `n new stack` is now listed alongside `Space mark`.
CRITICAL 1's new guard (ensure_block_entries_recognized) classified a `stacks:` block line by indentation depth first: at exactly entry_depth it required entry_key to match, and only accepted a list item when indented strictly deeper. A block sequence indented at the same column as its own key is valid YAML and the default emit style of several YAML writers, but entry_key always returns None for a `-`-prefixed line — so a same-level list item was misclassified as an unrecognized key and refused the whole edit, making `n` and `x` permanently dead for any config written that way. No data loss (refusing beats mangling), but a confusing dead end on an ordinary file. Classify by shape first instead: a list item qualifies at its key's depth or deeper; a candidate key must still sit at exactly entry_depth. The existing refusals (`tiny :`, a deeper non-list nested value) are unchanged — neither is a list item, so neither takes the new branch.
Follow-up 1's `>` -> `>=` widening (list items recognized at their key's
own indentation, not just strictly deeper) used a bare
`line.trim_start().starts_with('-')` to spot list items. Per YAML, `-` is
only a block-sequence indicator when followed by whitespace or end-of-line;
`-weird` (no space after the dash) is an ordinary plain scalar, so
`-weird:` is a valid sibling key. The widened check had no such
requirement, so at entry depth it started matching `-weird:` too, routing
it around the `entry_key` branch and letting it slip past unrefused --
reopening Critical-1's swallowing bug through a shape that fix's changes
never considered. Reachable from the TUI itself: `valid_stack_name` allows
`-` anywhere including first (it has to, or the name prompt's
per-keystroke filter would block typing `console-dev`), so a user can
create a `-weird` stack via `n`, and a later save or delete on a sibling
can then swallow it (or, per Critical-1's own zero-byte mechanism,
truncate the file).
Fix in two layers:
- The guard: new `is_list_item` only counts a line as a list item when the
dash is followed by whitespace or nothing, matching YAML's actual rule.
A dash-led key without that space now falls through to the `entry_key`
branch, where it's refused as intended (entry_key has rejected
`-`-prefixed keys since Critical-1, for the same reason it still can't
tell such a key apart from a real list item).
- Defense in depth: `valid_stack_name` stays unchanged (it's still the
TUI's per-keystroke filter, so `-` must stay valid at every position).
New `ensure_writable_name` layers a leading-dash rejection on top of it
and is called by `upsert_stack` and `set_default_stack` -- the two
points that write a name into the file -- so this tool can never create
such a stack itself. `remove_stack` doesn't need it: it can only target a
name that already exists, and the guard fix above already refuses to
touch a block it can't fully parse regardless of which entry is targeted.
entry_key found a `stacks:` entry's name via `rest.split(':').next()` --
the text up to the FIRST `:` byte. YAML ends a plain-scalar mapping key at
the first `:` followed by whitespace or end-of-line; a `:` glued directly
to the next character is ordinary scalar content, not a key terminator. So
a stack named `web:dev` had its extracted key truncated to `web`: matching
that truncated key made `entry_range` treat an unrelated `upsert_stack(...,
"web", ...)` call as targeting the `web:dev` entry, overwriting it instead
of appending a sibling -- and made `remove_stack(..., "web:dev")` report
"not defined" (the truncated key never equals the full search name) while
`remove_stack(..., "web")` -- a name that isn't actually defined anywhere
-- silently deleted it. Both are Critical-1's exact swallow/undelete-
ability failure mode, reached through name extraction rather than line
classification, and both reachable from the TUI (`web:dev`-style names are
ordinary hand-written stacks `Config::load` already accepts).
Find the key-ending colon the way YAML would: the first `:` whose
following character is whitespace or absent, via `char_indices().find(...)`
instead of `split(':').next()`. That find's `?` subsumes the old "must
contain a colon" length check, so it's dropped; the empty/leading-dash/
surrounding-whitespace rejections are unchanged.
Line classification (`is_list_item` / `ensure_block_entries_recognized`,
the OTHER half of this module's parsing) was independently re-verified
total at 657c285 and is untouched here.
…g a stacks block
entry_key's colon-ends-a-key check (Follow-up 3) and is_list_item's
dash-starts-a-list-item check (Follow-up 2) both used char::is_whitespace
-- Rust's Unicode White_Space property -- where YAML's own grammar calls
for s-white, which is space and tab only. White_Space is a strict
superset: NBSP (U+00A0), EM SPACE (U+2003), IDEOGRAPHIC SPACE (U+3000),
NARROW NO-BREAK SPACE (U+202F), and others all pass is_whitespace but are
not s-white, so both checks over-fired on them.
That reopened the same two swallowing bugs those two Follow-ups had just
closed, one codepoint over: a colon followed by NBSP looked like a key
terminator, truncating a name like `a:<NBSP>b` to `a` and losing the real
entry on upsert (Follow-up 3's exact bug); a dash followed by NBSP looked
like a real list-item indicator, waving a name like `-<NBSP>weird` through
`ensure_block_entries_recognized` without ever reaching entry_key's
leading-dash refusal, losing the sibling on upsert and, on remove,
returning Ok("") -- the config truncated to zero bytes, since nothing was
left in the block afterward (Critical-1's own headline repro, reopened via
Follow-up 2's dash fix).
Fix: one shared `is_yaml_space` predicate (space and tab only) used at
both sites instead of char::is_whitespace. Left out YAML's line-break
codepoints (U+0085/U+2028/U+2029) deliberately -- nothing here has ever
needed them, and skipping them only means a name using one degrades to a
safe refusal at write_verified, never to loss.
Three predicates in this file each independently answered "is this line a
comment," and disagreed with each other and with YAML:
- is_blank_or_comment used Rust's Unicode trim_start(), so a colon-key
line beginning with NBSP-then-`#` (e.g. ` <NBSP>#a:`) had its NBSP
stripped along with real indentation and was misread as a skippable
comment by block_range, ensure_block_entries_recognized, and
leading_gap_run alike -- even though `#` is only a YAML comment when
preceded by s-white, and NBSP isn't s-white, so this line is really the
entry `<NBSP>#a`. Every function that skips "just a comment" skipped the
entry instead: upsert_stack returned Ok with the sibling silently gone,
and remove_stack returned Ok("") -- Critical-1's headline zero-byte-file
repro, still live after four rounds of fixes that never touched this
specific predicate. Pre-existing, not a regression; only bites when the
pathological key comes after a well-formed entry in the same block.
- entry_key had no comment-awareness at all, a regression from Critical-1
itself: before that commit, entry_key's valid_stack_name check rejected
`#` and spaces as non-alphanumeric for free. Once Critical-1 loosened
that check (to recognize my.stack, web:dev, etc.), an ordinary comment
shaped like `key: value` -- e.g. `# TODO: add console` -- passed every
remaining check and came back as key `# TODO`. entry_range then treated
it as a competing sibling, cutting the real preceding entry's range
short right before it; YAML doesn't care that a comment sits between two
list items of the same key, so the list item left behind silently
reattached to the rewritten entry on the next load (tiny: [x, state]
instead of the requested tiny: [x]). Correct at base 7f07614.
Fix: one shared is_comment(line) predicate, built on is_yaml_space, that
is_blank_or_comment and entry_key both route through instead of carrying
their own opinions. A third site asking the same question, trailing_comment
(byte-level u8::is_ascii_whitespace, close to correct but not identical),
was brought onto is_yaml_space too, for the same reason: agreeing by
construction beats two implementations that happen to agree today.
is_list_item's leading line.trim_start() was still Rust's Unicode trim,
the one call in this module Follow-up 5 didn't reach. A line like
` <NBSP>- weird: v` had its NBSP stripped along with real indentation,
leaving `- weird: v`, which passes the dash check; indent_len (real spaces
only) still reports the block's own entry depth, so after Follow-up 2's
`>` -> `>=` widening (list items recognized at their key's own indentation,
not just strictly deeper) the guard accepts it as an ordinary same-level
list item. But a `-` not preceded by valid YAML indentation isn't a
sequence indicator at all -- the real key is `<NBSP>- weird`, which
entry_key then rejects (same surrounding-whitespace check that already
catches `tiny :`), leaving it invisible to entry_range, block_indent, and
remove_stack's has_entry scan alike. Blocker 1's exact mechanism, one
predicate over: upsert_stack(src, "tiny", [x]) returned Ok with the
sibling silently gone, and remove_stack(src, "tiny") returned Ok("") --
Critical-1's zero-byte-file repro again. Same 16 trigger codepoints as
Follow-up 4/5; pre-existing at base, not a regression; unreachable from
the TUI (valid_stack_name doesn't admit NBSP), hand-edited files only.
Fix: is_list_item's leading trim now uses is_yaml_space too, same as its
own after-the-dash check already did (Follow-up 2) and every other
predicate in this module now does (Follow-up 5). Last Unicode-trim call
in the file.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 56 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change replaces the fixed harness stack with named, persistent stacks. Configuration validates stack definitions and defaults. Worker membership follows dependencies. CLI startup, status output, orchestration, and TUI controls now use the active stack. ChangesNamed worker stack lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant TUI
participant ConfigWriter
participant Config
participant Orchestrator
participant WorkerEngine
Operator->>TUI: mark workers and save a stack
TUI->>ConfigWriter: upsert_stack
ConfigWriter->>Config: validate edited YAML
TUI->>Orchestrator: start_stack
Orchestrator->>WorkerEngine: start stack roots
WorkerEngine-->>Orchestrator: worker state
Orchestrator-->>TUI: grouped dashboard snapshot
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
workers-dev/src/discover.rs (2)
211-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a dependency-cycle case to this test.
stack_membersrelies onmembers.insert(name)returningfalseto terminate on a cyclic dependency graph. Nothing pins that today. If a future refactor moves the insert, the traversal becomes an infinite loop that hangs the TUI poll path. A two-worker mutual dependency fixture covers it in a few lines.💚 Proposed test addition
/// 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); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/discover.rs` around lines 211 - 232, Add a dependency-cycle test alongside stack_members_follows_dependencies that creates two workers with mutual dependencies, discovers them, and calls stack_members from one root. Assert traversal terminates and returns exactly both worker names, covering the members.insert guard behavior.
133-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the double map lookup into one.
contains_key(name)andget(name)hashnametwice per queue pop. A singlegetexpresses the same guard and the same traversal.♻️ Proposed refactor
while let Some(name) = queue.pop_front() { - if !deps_by_name.contains_key(name) || !members.insert(name) { - continue; - } - if let Some(deps) = deps_by_name.get(name) { - queue.extend(deps.iter().map(String::as_str)); - } + let Some(deps) = deps_by_name.get(name) else { + continue; // not a discovered worker + }; + if !members.insert(name) { + continue; // already visited (also breaks dependency cycles) + } + queue.extend(deps.iter().map(String::as_str)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/discover.rs` around lines 133 - 140, Update the queue traversal loop to use one deps_by_name.get(name) lookup while preserving the members.insert(name) guard and dependency expansion behavior; reuse the retrieved dependency value for queue.extend instead of calling contains_key and get separately.workers-dev/src/config_write.rs (1)
902-910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the refusal reason, not just that an error occurred.
assert!(!err.to_string().is_empty())passes for any error. These tests are regression guards for specific misreads, so a future change that fails for an unrelated reason would still keep them green. Assert on the distinguishing text, the wayrefuses_a_dash_led_key_misread_as_a_list_itemalready does witherr.to_string().contains("weird").♻️ Proposed tightening
let err = upsert_stack(src, "tiny", &roots(&["x"])).unwrap_err(); - assert!(!err.to_string().is_empty()); + assert!(err.to_string().contains("weird"), "{err:#}"); let err = remove_stack(src, "tiny").unwrap_err(); - assert!(!err.to_string().is_empty()); + assert!(err.to_string().contains("weird"), "{err:#}");Also applies to: 1000-1010
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/config_write.rs` around lines 902 - 910, Strengthen the assertions in refuses_a_dash_nbsp_key_misread_as_a_list_item and the corresponding tests around the later occurrence so each error message is checked for the distinguishing “weird” text, matching refuses_a_dash_led_key_misread_as_a_list_item, instead of only asserting that the message is non-empty.workers-dev/src/config.rs (1)
510-536: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding the
WORKERS_DEV_REPOmutation with a shared test lock.The doc comment already states the hazard.
cargo testruns tests in the same binary in parallel, so any future test that callsConfig::loadwithrepo: Nonewill observe this env var. A smallstatic ENV_LOCK: Mutex<()>held for the duration of theset_var/load/remove_varsequence removes the latent race and makes the constraint enforceable instead of documented. A panic betweenset_varandremove_varalso leaks the variable today; a guard scope fixes that too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/config.rs` around lines 510 - 536, Protect the WORKERS_DEV_REPO environment-variable mutation in config_path_points_at_the_loaded_file_even_when_repo_key_redirects with a shared static ENV_LOCK Mutex. Hold the lock across set_var, Config::load, and cleanup, and scope cleanup so the variable is removed even if loading panics; preserve the existing assertions and test behavior.workers-dev/src/tui/stacks.rs (1)
196-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject names that read back as non-string YAML keys at prompt time.
valid_stack_nameadmits an all-digit name such as123. The name survives the prompt andupsert_stack, and is only refused later byvalidate_config_textwhenparse_stacksfinds a non-string mapping key (pinned bywrite_verified_refuses_a_digits_only_stack_nameinworkers-dev/src/config_write.rs). The file stays intact, so this is not a corruption risk, but the error banner shows a YAML parse message instead of a name problem.Adding an all-digits check to
ensure_writable_nameinworkers-dev/src/config_write.rsproduces the same clear error class as the leading-dash rule already does.♻️ Proposed addition to `ensure_writable_name`
if name.starts_with('-') { bail!("invalid stack name {name:?} (can't start with -)"); } + // A digits-only name is valid YAML text but reads back as a number key, + // which `parse_stacks` then refuses — catch it here with a useful message. + if name.chars().all(|c| c.is_ascii_digit()) { + bail!("invalid stack name {name:?} (can't be only digits)"); + } Ok(())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/tui/stacks.rs` around lines 196 - 210, Update ensure_writable_name in config_write.rs to reject names composed entirely of digits, matching the existing leading-dash validation and returning the same clear name-validation error class. Ensure handle_name_key uses this validation before accepting or submitting a stack name so digit-only names are rejected at the prompt rather than during YAML parsing.workers-dev/src/tui/mod.rs (1)
915-925: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePanicking fallbacks defeat the stated no-panic-in-raw-mode policy. Three stack lookups guard an invariant but can still abort the process. A panic unwinds past the terminal restoration at the end of
run, so raw mode and the alternate screen stay active and the user's terminal is left unusable.
workers-dev/src/tui/mod.rs#L915-L925: replacestacks[0].1.clone()withstacks.first()and treat the empty case as "nothing to start".workers-dev/src/tui/mod.rs#L572-L582: replacestacks[0].1.clone()withstacks.first()and fall back to an empty root list.workers-dev/src/tui/mod.rs#L420-L424: replace.expect(...)with alet ... elsethat shows an error banner, matching the other two arms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/tui/mod.rs` around lines 915 - 925, Remove panic-prone stack lookups in workers-dev/src/tui/mod.rs at lines 915-925 and 572-582: use stacks.first() and treat an empty stack collection as no roots to start, using an empty root list at lines 572-582. At lines 420-424, replace the expect-based lookup with a let-else branch that displays an error banner consistently with the other arms, preserving the no-panic raw-mode behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers-dev/src/commands/mod.rs`:
- Around line 92-95: Update run_up to resolve and validate the configured
default stack roots before calling ensure_engine, returning the existing
empty-stack error without spawning iii when no roots are configured. Pass the
validated roots into the stack-start flow, while preserving the existing guard
in start_roots for the TUI path.
In `@workers-dev/src/config_write.rs`:
- Around line 90-96: Update the last-entry cleanup around has_entry and
block_range so deleting the final stacks: entry does not leave an indented
orphan comment after the stacks: header is removed. Preserve non-entry block
comments by keeping the entire block, or remove those comments explicitly
together with the block; ensure the resulting output contains no dangling
indented comment.
- Around line 436-454: The write_verified function uses a shared temporary
filename and insufficiently explicit durability semantics. Generate a unique
temporary path per write, write through a file handle, explicitly flush and sync
it before renaming, and retain cleanup on failures; also update the associated
documentation to avoid claiming that rename alone provides crash durability.
In `@workers-dev/src/status.rs`:
- Around line 42-49: Ensure stack names loaded by Config::load, including
default_stack and YAML stack keys, cannot inject terminal control characters by
validating them as printable text or escaping controls before rendering. Update
group_label and the relevant configuration-loading path so every configured name
passed to terminal output follows the same rule.
---
Nitpick comments:
In `@workers-dev/src/config_write.rs`:
- Around line 902-910: Strengthen the assertions in
refuses_a_dash_nbsp_key_misread_as_a_list_item and the corresponding tests
around the later occurrence so each error message is checked for the
distinguishing “weird” text, matching
refuses_a_dash_led_key_misread_as_a_list_item, instead of only asserting that
the message is non-empty.
In `@workers-dev/src/config.rs`:
- Around line 510-536: Protect the WORKERS_DEV_REPO environment-variable
mutation in config_path_points_at_the_loaded_file_even_when_repo_key_redirects
with a shared static ENV_LOCK Mutex. Hold the lock across set_var, Config::load,
and cleanup, and scope cleanup so the variable is removed even if loading
panics; preserve the existing assertions and test behavior.
In `@workers-dev/src/discover.rs`:
- Around line 211-232: Add a dependency-cycle test alongside
stack_members_follows_dependencies that creates two workers with mutual
dependencies, discovers them, and calls stack_members from one root. Assert
traversal terminates and returns exactly both worker names, covering the
members.insert guard behavior.
- Around line 133-140: Update the queue traversal loop to use one
deps_by_name.get(name) lookup while preserving the members.insert(name) guard
and dependency expansion behavior; reuse the retrieved dependency value for
queue.extend instead of calling contains_key and get separately.
In `@workers-dev/src/tui/mod.rs`:
- Around line 915-925: Remove panic-prone stack lookups in
workers-dev/src/tui/mod.rs at lines 915-925 and 572-582: use stacks.first() and
treat an empty stack collection as no roots to start, using an empty root list
at lines 572-582. At lines 420-424, replace the expect-based lookup with a
let-else branch that displays an error banner consistently with the other arms,
preserving the no-panic raw-mode behavior.
In `@workers-dev/src/tui/stacks.rs`:
- Around line 196-210: Update ensure_writable_name in config_write.rs to reject
names composed entirely of digits, matching the existing leading-dash validation
and returning the same clear name-validation error class. Ensure handle_name_key
uses this validation before accepting or submitting a stack name so digit-only
names are rejected at the prompt rather than during YAML parsing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cc2ed3a-1f01-4081-8c71-13af805e2b93
📒 Files selected for processing (11)
workers-dev/README.mdworkers-dev/src/commands/mod.rsworkers-dev/src/config.rsworkers-dev/src/config_write.rsworkers-dev/src/discover.rsworkers-dev/src/main.rsworkers-dev/src/orchestrator.rsworkers-dev/src/status.rsworkers-dev/src/tui/mod.rsworkers-dev/src/tui/stacks.rsworkers-dev/src/tui/theme.rs
| /// 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(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject terminal control characters in configured stack names.
group_label writes stack_name directly to terminal output. Config::load accepts YAML stack keys as arbitrary strings from an auto-loaded repository file. A quoted key can contain escape characters and modify terminal state when a user runs status or opens the TUI.
Validate loaded stack names as printable text, or escape control characters before rendering. Apply the same rule to default_stack.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workers-dev/src/status.rs` around lines 42 - 49, Ensure stack names loaded by
Config::load, including default_stack and YAML stack keys, cannot inject
terminal control characters by validating them as printable text or escaping
controls before rendering. Update group_label and the relevant
configuration-loading path so every configured name passed to terminal output
follows the same rule.
…removal remove_stack's emptiness check (has_entry) only looked for recognized entries, so removing the last entry from a block that still held a comment dropped the `stacks:` header too, leaving the comment as a dangling, un-headed indented line. Check whether the removed entry was the *entire* original block instead: if anything else was in it (sibling entry or a stray comment), the header stays.
write_verified's temp file was a fixed <file>.tmp sibling, so two workers-dev instances writing the same config could clobber each other's temp file (including via the error-path remove_file of an unrelated instance's in-flight write). Name it with the writer's pid instead. Also make the "a crash mid-write cannot leave a half-written config behind" doc comment true: std::fs::write only guarantees the bytes reached the page cache, not disk. Write through a File, flush, and sync_all before the rename.
Several config_write tests asserted !err.to_string().is_empty(), which passes for any error at all -- including a future failure for an unrelated reason. Tighten them to assert the distinguishing text, the way refuses_a_dash_led_key_misread_as_a_list_item already does.
…ngine run_up called ensure_engine() (which can spawn an iii process) before start_stack, whose start_roots guard rejects an empty-roots stack -- so `workers-dev up` on a configuration that cannot start anything still performed the external side effect of starting the engine. Resolve the default stack's roots and check for empty first. Extracted the roots lookup (previously duplicated in start_stack and stack_members) into Orchestrator::stack_roots so run_up's preflight check reuses it instead of a third copy; start_roots' own guard is unchanged.
workers-dev.yaml auto-loads from the repo root, so a stack key or default_stack containing an ANSI escape (e.g. via a double-quoted \x1b) would previously reach status's group headers and the TUI unescaped. Refuse any stack name containing a control character in parse_stacks and for default_stack, at load time so every consumer is covered by one rule instead of needing its own escaping.
Termination on a cyclic dependency graph rests entirely on members.insert(name) returning false to stop re-queueing a visited name. Nothing pinned that, and a regression there is an infinite loop in the TUI's poll path -- a hang, not a wrong answer.
contains_key(name) followed by get(name) hashed the queue-pop name twice per iteration. Use one get, keeping the members.insert guard that breaks cycles (now commented in place, since the termination test added in the previous commit gives that comment somewhere to point to).
|
Thanks — all four actionable comments and all three nitpicks were valid and are addressed in Reject an empty default stack before starting the engine ( Reject terminal control characters in configured stack names ( Use a unique temp file and don't claim crash durability from the rename ( Preserve orphaned block comments when the last entry is deleted ( Nitpicks — all taken. The dependency-cycle test ( Two left alone, with reasons: 116 tests, |
1 participant
Closes MOT-4377.
workers-devhad one hardcoded "harness stack", and defining a different one meant quitting the TUI to hand-edit YAML. This adds named stacks you can select, and then create and manage without leaving the dashboard.Selecting a stack
<repo_root>/workers-dev.yamlnow auto-loads (no--configneeded);--configstill overrides it.stacks:— a mapping of<name>: [roots…], YAML order preserved — anddefault_stack:. A stack's values are roots, expanded to roots + transitive deps for both grouping and starting, so a newly declared dependency joins automatically.harnessstack always exists; astacks.harness:entry overrides its roots.── stack:<name> (N) ──, full width).Ctrl+ustarts the default when only one stack exists, and opens a picker when there are several — Enter switches the grouping and starts.Creating and managing stacks
Spacemarks the selected worker (marked rows show✓; the table title counts them).nnames the marked set as a new stack.Ctrl+uwhen you want that.xdeletes a stack (confirm dialog naming the file),*makes it the default. Deleting the default is refused; deleting the current stack falls back to the default.How writes work
Every write is: edit the YAML text in place → validate by re-parsing with the real loader → write a temp file beside the target →
renameover it. Comments, blank lines and key order survive because nothing is re-serialized. A failed write changes nothing — not the file, not the session state.The scanner was aligned to YAML's own rules rather than approximations of them:
s-whiteis space and tab only,#starts a comment only at line start or after whitespace, and-is a sequence indicator only before whitespace. That came out of fuzzing 40,000 generated documents againstserde_yamlwith a zero-byte-file invariant asserted on every success — unsafe outcomes went 768 → 0.Deliberate refusals (refusing beats mangling, and each says so): inline
stacks: {…}, duplicated top-level keys, multi-line flow sequences, nested mapping values,? key, tab-indented keys,tiny :,-weird:, and quoted keys on the delete path.Breaking change
harness_stack:is hard-removed. A localworkers-dev.yamlstill using it fails startup with a rename hint pointing atstacks: {harness: [...]}. Nothing in the repo commits such a file — only per-worktree ones are affected.Testing
109 tests in
workers-dev;cargo fmt --check,cargo clippy --all-features -- -D warningsand--all-features --all-targets -- -D warningsall clean.Manual QA was run live under tmux against a real engine: creating a stack from marks and confirming every comment in the file survived,
*moving the default (including preserving a trailing comment on that line), the delete guardrail refusing the default, deleting a non-default stack, and the inline-stacks:refusal leaving the file untouched.Not exercised: pressing Enter in the picker (switch and start) against a live engine — it spawns real
cargo runbuilds for every root, against an engine another session owned. The path is unit-tested and traced end to end, but not observed.Notes
Space/n/x/*, behavior matches the previous release apart from deliberately reworded status strings (started harness stack→started stack harness).workers-dev.yamlis not gitignored, so the first save leaves an untracked file in the repo root; the save notice names the path it wrote and the README says so.Summary by CodeRabbit
New Features
Configuration
Documentation