fix(maestro): close three holes in the wire contract #45

2026-08-22T14:28:14+01:00

naps62-yolo commented

2026-08-22 14:28:14 +01:00

Three typed fields that type-checked on both sides and carried nothing across.

`SessionStatus::Dead` — never produced, rendered by all three clients

The daemon removed sessions on death and never marked them, yet tui/sidebar.rs, cli/sessions.rs and web/catalog.ts all render a dead state and DESIGN.md specifies the exited badge.

Per your call: dead sessions are kept until dismissed, with their output readable. A dead session can't be resumed — process, PTY and sidecar are all gone — so Dead is only worth having as a notification and a chance to read what a crashed agent printed. That last part was the real loss: cleanup_record dropped the PtyBridge, destroying the replay buffer.

PtyBridge::seal() stops the reader and closes the live stream while keeping the buffer. So:

an attached client still gets a Close frame — a maestro attach must not sit in raw mode against an exited shell (NAP-226, still covered)
attaching afterwards replays the final output, then ends
the session is read-only: send_keys and resize are no-ops, not tmux failures
dismissal is the existing DELETE /api/sessions/:id, which skips the tmux call it would otherwise make against a session tmux already lost

Dead sessions do not survive a daemon restart — recover adopts only what tmux still holds.

`CreateSessionRequest.env` — accepted, stored, echoed, dropped

new_session_piped had no env parameter at all. It now reaches tmux as sorted -e KEY=VAL flags (sorted so the argv is deterministic and assertable). A key containing = or NUL is rejected — tmux would parse it as a different assignment than the caller wrote.

`PROTOCOL_VERSION` — written, read by nobody

Adds maestro_protocol::is_compatible, compared on the major component only (within a major, added fields are #[serde(default)]). An empty or unparseable version reads as compatible, so daemons predating the field don't trip a warning. maestro list shows the version; the TUI warns on mismatch.

Also: session errors no longer all collapse to 502. An invalid env key is a client mistake and answers 400.

Verification

cargo fmt --all -- --check                   ok
cargo clippy --all-targets -- -D warnings    0
cargo test --all                             309 passed, 0 failed
pnpm -C web test                             36 passed
pnpm -C web typecheck                        clean

Two existing tests encoded the old reap behaviour and were rewritten rather than deleted: the real-tmux liveness_watcher_* case now asserts mark-then-dismiss, and a new attaching_to_a_dead_session_replays_its_final_output pins the property this change exists for.

Sixth of nine from the architecture review (#40–#44 merged).

Three typed fields that type-checked on both sides and carried nothing across. ### `SessionStatus::Dead` — never produced, rendered by all three clients The daemon removed sessions on death and never marked them, yet `tui/sidebar.rs`, `cli/sessions.rs` and `web/catalog.ts` all render a dead state and DESIGN.md specifies the exited badge. Per your call: **dead sessions are kept until dismissed, with their output readable.** A dead session can't be resumed — process, PTY and sidecar are all gone — so `Dead` is only worth having as a notification *and* a chance to read what a crashed agent printed. That last part was the real loss: `cleanup_record` dropped the `PtyBridge`, destroying the replay buffer. `PtyBridge::seal()` stops the reader and closes the live stream while keeping the buffer. So: - an attached client still gets a Close frame — a `maestro attach` must not sit in raw mode against an exited shell (NAP-226, still covered) - attaching *afterwards* replays the final output, then ends - the session is read-only: `send_keys` and `resize` are no-ops, not tmux failures - dismissal is the existing `DELETE /api/sessions/:id`, which skips the tmux call it would otherwise make against a session tmux already lost Dead sessions do **not** survive a daemon restart — `recover` adopts only what tmux still holds. ### `CreateSessionRequest.env` — accepted, stored, echoed, dropped `new_session_piped` had no env parameter at all. It now reaches tmux as sorted `-e KEY=VAL` flags (sorted so the argv is deterministic and assertable). A key containing `=` or NUL is rejected — tmux would parse it as a different assignment than the caller wrote. ### `PROTOCOL_VERSION` — written, read by nobody Adds `maestro_protocol::is_compatible`, compared on the major component only (within a major, added fields are `#[serde(default)]`). An empty or unparseable version reads as compatible, so daemons predating the field don't trip a warning. `maestro list` shows the version; the TUI warns on mismatch. Also: session errors no longer all collapse to 502. An invalid env key is a client mistake and answers 400. <details> <summary>Verification</summary> ``` cargo fmt --all -- --check ok cargo clippy --all-targets -- -D warnings 0 cargo test --all 309 passed, 0 failed pnpm -C web test 36 passed pnpm -C web typecheck clean ``` Two existing tests encoded the old reap behaviour and were rewritten rather than deleted: the real-tmux `liveness_watcher_*` case now asserts mark-then-dismiss, and a new `attaching_to_a_dead_session_replays_its_final_output` pins the property this change exists for. </details> Sixth of nine from the architecture review (#40–#44 merged).

naps62-yolo added 1 commit 2026-08-22 14:28:14 +01:00

fix(maestro): close three holes in the wire contract

ci / web (pull_request) Successful in 27s

Details

ci / rust (pull_request) Successful in 1m40s

Details

ci / web (push) Successful in 35s

Details

ci / rust (push) Successful in 2m23s

Details

b20651c33e

Each was a typed field that type-checked on both sides and carried
nothing across.

SessionStatus::Dead was never constructed by the daemon, yet the TUI, CLI
and SPA all render it and DESIGN.md specifies the exited badge. Sessions
were removed on death, never marked. They are now marked Dead and kept
until the operator dismisses them, with the PtyBridge sealed rather than
dropped: attached clients still get a Close frame (NAP-226), and the
replay buffer survives so a later attach can read what a crashed agent
printed. Previously that output was destroyed. Dead sessions are
read-only and do not survive a daemon restart.

CreateSessionRequest.env was accepted, stored, echoed back and dropped —
new_session_piped had no env parameter. It now reaches tmux as sorted
`-e KEY=VAL` flags. A key containing `=` or NUL is rejected, since tmux
would parse it as a different assignment than the caller wrote.

PROTOCOL_VERSION was written by the daemon and read by nobody. Adds
`maestro_protocol::is_compatible`, compared on the major component only;
`maestro list` shows the daemon version and the TUI warns on mismatch.

Also stops mapping every non-404 session error to 502: an invalid env key
is a client mistake and now answers 400.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

naps62-yolo merged commit b20651c33e into main

2026-08-22 14:30:13 +01:00

naps62-yolo deleted branch lithuanians

2026-08-22 14:30:13 +01:00

naps62-yolo referenced this pull request

2026-08-22 16:32:14 +01:00

fix(maestro): three defects the review flagged and left open #50

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: naps62/maestro#45

fix(maestro): close three holes in the wire contract #45

SessionStatus::Dead — never produced, rendered by all three clients

CreateSessionRequest.env — accepted, stored, echoed, dropped

PROTOCOL_VERSION — written, read by nobody

`SessionStatus::Dead` — never produced, rendered by all three clients

`CreateSessionRequest.env` — accepted, stored, echoed, dropped

`PROTOCOL_VERSION` — written, read by nobody