naps62/maestro
Archived
1
0
Fork 0
Code Issues 9 Pull Requests Actions Packages Projects Releases Wiki Activity

Add SessionView to the protocol and sidecar #133

New Issue
Closed
opened 2026-09-03 10:36:05 +01:00 by naps62-yolo · 2 comments
naps62-yolo commented 2026-09-03 10:36:05 +01:00
Collaborator
Copy Link
Copy Source

A session is either a terminal session (PTY bytes through tmux, what exists today)
or a structured session (ACP rows). One process is one view, so the choice is made
at spawn and never changes for the life of the session.

Add SessionView { Terminal, Structured } to maestro-protocol and a view field
to Session (crates/maestro-protocol/src/lib.rs:49), carrying
#[serde(default)] with Terminal as the Default impl. Same field on
CreateSessionRequest (lib.rs:338), also defaulted, so every existing client and
every curl against the API keeps working untouched.

The default is what makes this safe to land alone: every sidecar already on disk
deserializes into a terminal session, and a daemon rolled back to the previous
binary reads them the same way. Add a sidecar.rs round-trip test that
deserializes a JSON object with no view key and asserts Terminal, and one that
writes Structured and reads it back.

Non-goal: spawning anything. Nothing reads view yet — this issue is the type and
its serde contract only.

A session is either a terminal session (PTY bytes through tmux, what exists today) or a structured session (ACP rows). One process is one view, so the choice is made at spawn and never changes for the life of the session. Add `SessionView { Terminal, Structured }` to `maestro-protocol` and a `view` field to `Session` (`crates/maestro-protocol/src/lib.rs:49`), carrying `#[serde(default)]` with `Terminal` as the `Default` impl. Same field on `CreateSessionRequest` (`lib.rs:338`), also defaulted, so every existing client and every `curl` against the API keeps working untouched. The default is what makes this safe to land alone: every sidecar already on disk deserializes into a terminal session, and a daemon rolled back to the previous binary reads them the same way. Add a `sidecar.rs` round-trip test that deserializes a JSON object with no `view` key and asserts `Terminal`, and one that writes `Structured` and reads it back. Non-goal: spawning anything. Nothing reads `view` yet — this issue is the type and its serde contract only.
naps62-yolo added this to the Structured view milestone 2026-09-03 10:36:05 +01:00
naps62-yolo added the area/protocoldifficulty/easytype/feat labels 2026-09-03 10:36:05 +01:00
naps62-yolo added a new dependency 2026-09-03 10:36:28 +01:00
#134 Run a `claude-code-acp` subprocess per structured session
naps62-yolo commented 2026-09-04 19:43:16 +01:00
Author
Collaborator
Copy Link
Copy Source

Out-of-scope bug found while running the full-suite gate for this issue (unrelated to the SessionView change — crates/maestro-daemon/tests/worktree.rs and src/worktree.rs are untouched by this branch):

cargo test -p maestro-daemon --test worktree add_fails_when_branch_already_exists fails deterministically:

thread 'add_fails_when_branch_already_exists' panicked at crates/maestro-daemon/tests/worktree.rs:426:46:
called `Result::unwrap_err()` on an `Ok` value: "/tmp/.../feat/dup-2"

The test asserts mgr.add_new(&repo, "feat/dup") errors when the branch already exists. It now succeeds by creating feat/dup-2 instead. Looks like a side effect of the recent "suffix branch name on new-branch worktree collision" fix (commit 49ab7e9) — that commit changed the collision behavior to auto-suffix rather than fail, but this test wasn't updated to match. Either the test's expectation is now wrong, or the auto-suffix behavior is over-broad and shouldn't apply here (a caller explicitly asking for an existing branch name probably still wants an error, not a silent rename).

Not fixed here — out of scope for #133.

Out-of-scope bug found while running the full-suite gate for this issue (unrelated to the `SessionView` change — `crates/maestro-daemon/tests/worktree.rs` and `src/worktree.rs` are untouched by this branch): `cargo test -p maestro-daemon --test worktree add_fails_when_branch_already_exists` fails deterministically: ``` thread 'add_fails_when_branch_already_exists' panicked at crates/maestro-daemon/tests/worktree.rs:426:46: called `Result::unwrap_err()` on an `Ok` value: "/tmp/.../feat/dup-2" ``` The test asserts `mgr.add_new(&repo, "feat/dup")` errors when the branch already exists. It now succeeds by creating `feat/dup-2` instead. Looks like a side effect of the recent "suffix branch name on new-branch worktree collision" fix (commit `49ab7e9`) — that commit changed the collision behavior to auto-suffix rather than fail, but this test wasn't updated to match. Either the test's expectation is now wrong, or the auto-suffix behavior is over-broad and shouldn't apply here (a caller explicitly asking for an existing branch name probably still wants an error, not a silent rename). Not fixed here — out of scope for #133.
naps62-yolo commented 2026-09-04 19:55:34 +01:00
Author
Collaborator
Copy Link
Copy Source

Second manifestation of the same bug, from a full cargo test --workspace run (also unrelated to SessionView, crates/maestro-e2e/tests/e2e.rs untouched by this branch):

second_spawn_on_same_branch_is_rejected (maestro-e2e) fails the same way:

thread 'second_spawn_on_same_branch_is_rejected' panicked at crates/maestro-e2e/tests/e2e.rs:354:18:
expected rejection for duplicate branch, got Ok(Session { .. cwd: "/tmp/.../worktrees/feature/dup-2" .. })

Same root cause as the worktree.rs failure above: the auto-suffix from 49ab7e9 now also swallows the case where the daemon is expected to reject spawning a second session onto a branch already checked out by a live session — it silently spawns onto feature/dup-2 instead. The neighboring test colliding_branch_name_gets_suffixed_too (which passes) suggests the suffix is intentional for a bare filesystem-path collision; this test wants a hard rejection when the branch itself is already in use by another session. The two behaviors need to be told apart, not applied as one rule.

Second manifestation of the same bug, from a full `cargo test --workspace` run (also unrelated to `SessionView`, `crates/maestro-e2e/tests/e2e.rs` untouched by this branch): `second_spawn_on_same_branch_is_rejected` (maestro-e2e) fails the same way: ``` thread 'second_spawn_on_same_branch_is_rejected' panicked at crates/maestro-e2e/tests/e2e.rs:354:18: expected rejection for duplicate branch, got Ok(Session { .. cwd: "/tmp/.../worktrees/feature/dup-2" .. }) ``` Same root cause as the `worktree.rs` failure above: the auto-suffix from `49ab7e9` now also swallows the case where the daemon is expected to reject spawning a second session onto a branch already checked out by a live session — it silently spawns onto `feature/dup-2` instead. The neighboring test `colliding_branch_name_gets_suffixed_too` (which passes) suggests the suffix is intentional for a bare filesystem-path collision; this test wants a hard rejection when the *branch* itself is already in use by another session. The two behaviors need to be told apart, not applied as one rule.
This repo is archived. You cannot comment on issues.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
area/android
area/cli
area/config
area/daemon
area/protocol
area/tui
area/web
code-review
difficulty/easy
difficulty/hard
difficulty/medium
epic
Tracks a multi-issue epic / parent work item
 tui
type/docs
type/feat
type/fix
type/refactor
type/test
No labels area/protocol difficulty/easy type/feat
Milestone
No items
No Milestone Structured view
Projects
Clear projects
No projects
Assignees
Clear assignees
hermes (Hermes (agent)) naps62-yolo (naps62-yolo (agent)) resende (Resende) naps62 (Miguel Palhas)
No Assignees
1 Participants
Due Date
No due date set.
Blocks
#134 Run a `claude-code-acp` subprocess per structured session
naps62/maestro
Reference: naps62/maestro#133