Ten findings from reviewing the accumulated blitz/spawn-v2 diff. The two
that could wedge a repo:
- The cleanup guard refused any worktree whose commits were on no other
ref, which is every ordinary unpushed branch. Paths are deterministic
now, so the leftover tree blocked the next spawn and each branch became
a one-shot. `worktree remove` keeps refs/heads/<branch>, so committed
work is never at risk: guard on an unclean working tree alone.
- A worktree whose directory had vanished failed the git probe and was
reported as unsaved work, stranding the registration forever. Prune it
instead, and prune before `worktree add` tests the target path.
Also:
- The CLI hardcoded create_branch: true, so the daemon's existing-branch
path was unreachable. Added --existing-branch.
- Pastes were forwarded without their DECSET 2004 markers, so a remote
shell ran a multi-line paste line by line.
- spawnForm emitted before its seed was restored and after destroy(), so
a rebuilt draft blanked its cwd and a dead form clobbered the live one.
- showRawCwd and the probe's worktree untick never reached the leaf.
- A late autocomplete fetch reopened the popup under a blurred input.
- The "remove worktree when killed" checkbox promised something the
daemon no longer honours; removed.
Escape closes the popup, refocuses the input and stops propagation so it
can't also discard a surrounding draft pane. Tab commits the highlighted
entry and lets focus move to the next field. Blur / clicking outside
dismisses, and a dismissed popup stays closed until the operator types
again.
forward_to_session dropped Paste(_) on the floor, so pastes into a
session vanished once terminal.rs enabled bracketed paste. Split the
arm: Paste forwards the payload verbatim (the remote readline
reassembles it), only Mouse events have no wire encoding.
Action::OpenSpawnModal becomes NewDraftPane: pressing s turns the empty
pane into an inline draft form (or opens a new tab when the pane shows a
session). The form is a pane-level widget with its own field focus; a
successful submit replaces the draft with the new session in place,
matching the web client's draft panes. SpawnModal and OverlayKind::Spawn
are removed; kill-confirm and help overlays stay.
Six findings from reviewing the accumulated blitz/spawn-v2 diff:
- loadRecents restored the seeded cwd but never emitted it, so the leaf's
draft kept cwd:"" and the next render reseeded the form blank.
- stripDrafts did not recurse, so every half-filled draft in a split tab
was written to localStorage; the old test passed only because hydrate
discards draft on the way back in.
- showRawCwd and selectPreset grabbed focus from async init paths, so a
draft pane stole the caret seconds after mounting.
- .mv-draft inherited no layout once the form left .modal-body.
- onSuccess mounted the new session onto a leaf that may have been closed
while the POST was in flight, stranding it in no pane.
The umbrella `maestro` binary had zero coverage of its feature-gate
combinations (cli / daemon / registry / tui). Add:
- Unit tests in `main.rs` for `is_cli_subcommand` classification (CLI vs
server subcommands, plus the always-false non-`cli` stub) and the
`-v` verbosity -> tracing-directive mapping.
- A `verbosity_directive` helper, extracted from `cli_filter` purely to
make the verbosity mapping testable without touching the process-global
`RUST_LOG` (behavior unchanged).
- An integration test (`tests/cli.rs`) driving the real built binary via
`CARGO_BIN_EXE_maestro`, asserting the `--help`/`--version`/subcommand
dispatch surface. Per-subcommand checks are `#[cfg]`-gated so the suite
is meaningful under every feature subset.
- A CI feature-gate matrix step: build + test each of cli/daemon/registry/
tui in isolation, and assert the no-feature build is rejected.
All four subsets and the default build pass fmt, clippy, and tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The subtree search returned early whenever it found *any* group member,
so an in-tree stage that was busy or sleeping on a timer masked a
reparented member genuinely parked on the tty.
Checking every member instead would have meant a /proc scan per session
per tick whenever nothing was parked — the common case. ProcGroups
snapshots /proc once per activity pass and indexes by process group, so
the cost is one walk however many sessions the daemon owns, and the
subtree walk disappears: the index is a superset of it.
Also fixes the background-helper test, which had stopped discriminating.
It polled until awaiting_input was true and caught a moment before the
helper spawned, so it passed against a deepest-descendant walk too. It now
waits for the helper to exist first. Both regression tests were re-checked
by patching each old implementation back in.
Reported in review on #51.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A member of the foreground group can sit outside the pane's subtree: a
stage that forks a terminal-reading child and exits leaves that child
reparented to init while it keeps the foreground pgid. The subtree search
finds nothing and the session reads as idle.
An empty subtree result now falls back to scanning /proc for the group.
The scan only runs when the cheap path found nothing, so the common case
still costs a handful of reads.
Tested at the level it can honestly be tested: scan_for_pgid against this
process's own group, and against a group that cannot exist. Reproducing
the reparenting scenario end to end is not practical — setpgid across
sessions is EPERM, and from a shell the window closes as soon as the shell
reclaims the terminal.
Reported in review on #51.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tpgid names a process *group*, not a process. Its leader can exit while
another member still holds the terminal — a foreground pipeline whose
first stage finished — and then /proc/<tpgid> does not exist, so reading
it directly reported the session idle while it sat waiting on the user.
awaiting_input now searches the pane's subtree for live processes whose
pgrp matches, and asks whether any is parked on the tty. Members stay in
that subtree because their parent shell is alive, so they never reparent.
Reported in review on #51. The regression test needs an interactive shell
— job control is what puts a pipeline in its own process group — and waits
for the leader to actually be gone before asserting, otherwise it passes
against the old lookup while the shell is still foreground.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A plain deadline loop says the same thing without catching a panic to
turn it back into an assert.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
awaiting_input walked `children` to the deepest descendant, guessing that
the newest child is the foreground one. An agent that starts a background
helper and then blocks on read landed on the helper, so a session genuinely
waiting on the user was reported as idle.
tpgid in /proc/<pid>/stat is the kernel's own answer — the foreground
process group of the controlling terminal — and the group leader's pid is
the group id. One field read replaces a 64-deep walk.
Adds tests/activity.rs: the first end-to-end coverage of this pipeline,
which had none. The background-helper case fails against the old walk and
passes against tpgid.
Also makes the attach handler subscribe before upgrading. Checking
existence and then subscribing inside the callback left a window for the
liveness watcher to reap in between, which attach::serve compensated for
with a clean close; acquiring the subscription up front means the 404 and
the subscription are one step and the window does not exist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SPA attach had no reconnect at all. Any dropped frame closed the socket,
`onClosed` fired, and the pane was destroyed permanently — the TUI
reconnects forever on the same endpoint. Session death also reached the UI
on two uncoordinated channels: the row from the events socket, the pane
from `ws.onclose`, with nothing reconciling them, so either could happen
without the other.
The catalog is now the authority. `shouldRetryClose` decides from session
status: Running means the link dropped, so retry with 500ms-to-10s
backoff; Dead means the PTY stream was sealed and nothing more is coming;
absent means it is gone. Application closes (4000+, a relayed daemon
rejection) and clean closes are never retried. The shell also closes any
pane whose session has left the catalog. The policy lives in reconnect.ts
so it is tested without a DOM.
kill_session checked `has()` before `kill()`, which only opened a window
for the liveness watcher to reap in between — turning a 404 into a 502.
`kill` reports the miss itself.
with_spawn_history logged and carried on when `Arc::get_mut` failed,
producing a daemon whose recent-dirs picker is permanently empty with
nothing to show for it. It panics now: that is a construction-order
mistake, at startup, in code that runs once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
promote() early-returned for anything that was not a Click, leaving the
pending click intact — so click, drag, click at the same cell within
400ms was reported as a double-click. Same for a scroll or a keystroke
between the two.
Any non-click input now clears the pending click. Two clicks with
something in between are two clicks, however fast.
Reported in review on #48.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The keybinding parser validated strings, not whether any producer could
emit the event — so the shipped default `mouse:dblclick(sidebar-row)`
could never fire, and nothing failed.
`MouseKind::DblClick` appeared only in the enum and in parse/format;
`canonicalise` emits Down/Drag/ScrollUp/ScrollDown. Adds a `ClickTracker`
that promotes a second click at the same cell within 400ms, keeping
`canonicalise` a pure function of one event — the tracker takes `now` as a
parameter, so the timing rule is tested without sleeping.
`Region::Sidebar` and `Region::SplitDivider` are deleted: `region_at` can
only return Modal, Tab, SidebarRow, Pane or Unknown, so the parser was
accepting names the renderer cannot produce. It now rejects them.
Shift-Tab is wired to `overlays::prev_field`, which existed with no
caller, leaving the spawn modal cycleable in one direction only. crossterm
reports it as `KeyCode::BackTab` rather than a modified Tab, so `InputKey`
gains a `BackTab` variant that also forwards `\x1b[Z` to the pane.
Left alone: `Action::FocusPane(Direction)` discards its direction, but
that is v1 having one pane per tab, not a defect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Miguel Palhas
naps62-yolo
naps62