Repository navigation
mux: implement the 8 proposed protocol-6 commands (wait-for, run, send-key, copy, ids, notify, list-agents, report-agent) - #7584
Conversation
Implements wait-for, run, send-key, copy, ids, notify, list-agents, report-agent server-side (mux-core) with CLI verbs (mux-tui) and flips their status to implemented in the spec. Protocol stays 6 (additive). Highlights: - wait-for: registers the attach tap before the first screen check (attach-then-check, race-free) so a one-shot match is never missed. - notify: per-surface unread state + a notification event; the mux TUI renders an attention border on the selected tab/pane, an unread dot in the tab bar, and a workspace sidebar dot, cleared on focus; never steals focus; skips the unread flag for the already-active surface. Ratatui TestBackend render test covers all three indicators. - report-agent/list-agents: per-surface agent-state store with hook > socket authority (a newer hook wins; socket never clobbers). - send-key via the ghostty key encoder; copy screen/selection/scrollback; run spawns an explicit command; ids lists tree ids. Conformance gains an implemented-verbs fixture; new mux-core + CLI + ratatui tests. Reviewed via the plan/code/judge loop (2 rounds).
📝 WalkthroughWalkthroughImplements eight new mux control-socket verbs (wait-for, run, send-key, copy, ids, notify, list-agents, report-agent), adds a notification and agent-tracking subsystem to mux-core, per-surface text selection state, a key-chord parser, corresponding mux-tui CLI/rendering support, conformance fixtures, and spec/docs status updates. ChangesImplemented verbs and notification/agent system
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 21❌ Failed checks (1 warning, 20 inconclusive)
✅ Passed checks (4 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 |
Greptile SummaryThis PR implements the remaining proposed mux protocol-6 commands. The main changes are:
Confidence Score: 4/5The new command paths are mostly mergeable, but browser key input and selection copy need focused fixes.
mux/crates/mux-core/src/server.rs; mux/crates/mux-core/src/mux.rs Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| surface: surface.id, | ||
| pane: pane_id, | ||
| screen: screen_id, | ||
| workspace: ws_id, |
There was a problem hiding this comment.
This helper removes unread state without emitting the same tree-change signal used by explicit notification clearing. When a remote client focuses the notified tab, the server state is cleared but subscribers may keep rendering the old unread dot until another unrelated tree event arrives.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6bdfc73. Configure here.
| .filter(|record| surface.is_none_or(|surface| record.surface == surface)) | ||
| .filter(|record| state.is_none_or(|state| record.state == state)) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Agent records persist after close
Medium Severity
The new per-surface agent_records store is populated by report-agent but never cleared when a surface is reaped via close_surface or surface_exited. Unfiltered list-agents still returns those entries, so automation can treat closed tabs as active agents long after they leave the tree.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6bdfc73. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mux/crates/mux-tui/src/app.rs (1)
2259-2274: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemote selections need to update the server-side surface state
copy_selectiononly persists the selected text whensurfaceisSurfaceHandle::Local. For remote-attached sessions,Command::Copy { mode: "selection" }still readssurface.selection_text()from the authoritative server-sideSurface, so the selected text never becomes available there.If remote sessions are meant to support
copy --mode selection, this path needs to write the selection back to the remoteSurfacetoo.🤖 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 `@mux/crates/mux-tui/src/app.rs` around lines 2259 - 2274, The selection copy path only stores text on SurfaceHandle::Local, so remote-attached sessions never persist the selection where Command::Copy with mode selection reads it. Update copy_selection to write the selected text back to the authoritative Surface for remote sessions as well, using the existing surface/selection state handling around selection_text_absolute and set_selection_text, while keeping the clipboard and toast behavior unchanged.
♻️ Duplicate comments (1)
mux/crates/mux-core/src/server.rs (1)
733-879: 🎯 Functional Correctness | 🟠 MajorCommand handlers look correct overall (WaitFor's attach-before-check ordering avoids the missed one-shot-match race, Run's argv/command and pane/new_workspace mutual-exclusion checks are sound, SendKey/Copy/Ids/Notify/ListAgents/ReportAgent all validate existence and enum values before mutating state).
One dependent concern:
Notify/ReportAgentvalidatesurfaceexistence viaget_surfacebefore calling intomux.post_notification/mux.report_agent, but as noted onmux.rs, those entries are never cleaned up when the surface later closes — see that comment for the root cause.🤖 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 `@mux/crates/mux-core/src/server.rs` around lines 733 - 879, The Notify and ReportAgent handlers are fine, but their surface lookup relies on entries that can remain stale after a surface closes. Fix the cleanup path in mux state management so surfaces are removed when they are closed, and ensure the logic around get_surface, mux.post_notification, and mux.report_agent cannot see dead surfaces. Update the relevant surface teardown/close code in mux.rs to keep these handlers working against only live surfaces.
🤖 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 `@mux/bindings/conformance/fixtures.json`:
- Around line 139-141: Add a requires field to the implemented-verbs-json
fixture so it skips gracefully on older servers. Update the fixture definition
in fixtures.json to reference the new protocol v6 command set using the same
pattern as move-tab-same-position, ensuring the fixture is gated by the relevant
commands for run, wait-for, send-key, copy, ids, notify, list-agents, and
report-agent.
In `@mux/crates/ghostty-vt/src/key.rs`:
- Around line 138-151: The single-character chord handling in the key parsing
path is emitting unshifted UTF-8 for shifted letters, so `shift+a` is treated
like plain `a` while `SHIFT` is consumed. Update the single-character branch in
`physical_key_for_char` handling so letters preserve shifted output in
`input.utf8` when `Mods::SHIFT` is present, while keeping
`input.unshifted_codepoint` lowercase and only consuming shift where
appropriate. Apply the same fix in the matching parser in `keys.rs`, and add
tests covering `shift+a` plus one shifted symbol case to confirm the behavior.
In `@mux/crates/mux-core/src/mux.rs`:
- Around line 139-140: Purge per-surface state when a surface is closed:
`agent_records` and `surface_notifications` in `Mux` currently retain entries
forever because `post_notification` and `report_agent` insert by `SurfaceId` but
`close_surface`/`close_surfaces` never remove them. Update the surface teardown
path in `Mux::close_surface` and the batch cleanup in `Mux::close_surfaces` to
delete both maps’ entries for every closed `SurfaceId`, using the existing
notification cleanup pattern as the reference so `list_agents` cannot return
stale data.
In `@mux/crates/mux-core/src/server.rs`:
- Around line 670-701: spawn_attach_notification_stream currently creates a
mux-wide subscriber thread that lives until writer.send() fails, so it never
ends with the attached surface and is also started before attach setup succeeds.
Update spawn_attach_notification_stream to tie its recv loop to the target
surface_id lifecycle (for example, exit when mux.surface(surface_id) is gone or
share the same shutdown path as the main attach stream), and only spawn it after
the corresponding attach_stream()/attach_frames() setup has succeeded. Also
apply the same lifetime fix in RemoteSession’s notification handling path.
In `@mux/crates/mux-tui/src/cli.rs`:
- Around line 665-682: build_run currently checks --command against positional
argv locally, but it does not mirror the server-side mutual exclusion between
--pane and --new-workspace. Update build_run in cli.rs to validate that these
two options cannot be used together, returning a UsageError before constructing
the JSON payload, alongside the existing command/argv checks.
In `@mux/crates/mux-tui/src/ui/sidebar.rs`:
- Around line 16-23: The sidebar unread indicator in
workspace_has_unread/sidebar rendering currently treats every unread
notification the same and ignores severity. Update the sidebar dot logic to
derive its color from the unread tab’s notification level, using the same
notification_color-based severity mapping already used in ui/pane.rs
(draw_box/draw_tab_bar), so warning/error states are preserved instead of always
showing notification_info. Reference the workspace_has_unread helper and the
sidebar dot rendering path when applying the fix.
In `@mux/crates/mux-tui/tests/cli.rs`:
- Around line 123-124: The send-key test only checks that the command succeeds,
but it does not confirm the key actually reached the terminal. Update the
`send_key` scenario in the CLI test to follow the `send-key` call with a
readback assertion, using the existing test helpers and the same `surface`/`cli`
flow, so it verifies terminal output (for example via `copy` or `read-screen`)
instead of only exit status. Use the surrounding test case in `cli.rs` as the
place to add the follow-up verification and mirror the conformance fixture’s
pattern of sending input then asserting the resulting screen text.
In `@mux/spec/cli.md`:
- Around line 83-90: The verb table in cli.md is being parsed incorrectly
because the pipe-separated option values are treated as extra columns. Update
the affected rows for copy, ids, notify, and report-agent in the table so the
literal pipe characters inside cells are escaped, keeping the column structure
intact while preserving the existing unique command symbols.
---
Outside diff comments:
In `@mux/crates/mux-tui/src/app.rs`:
- Around line 2259-2274: The selection copy path only stores text on
SurfaceHandle::Local, so remote-attached sessions never persist the selection
where Command::Copy with mode selection reads it. Update copy_selection to write
the selected text back to the authoritative Surface for remote sessions as well,
using the existing surface/selection state handling around
selection_text_absolute and set_selection_text, while keeping the clipboard and
toast behavior unchanged.
---
Duplicate comments:
In `@mux/crates/mux-core/src/server.rs`:
- Around line 733-879: The Notify and ReportAgent handlers are fine, but their
surface lookup relies on entries that can remain stale after a surface closes.
Fix the cleanup path in mux state management so surfaces are removed when they
are closed, and ensure the logic around get_surface, mux.post_notification, and
mux.report_agent cannot see dead surfaces. Update the relevant surface
teardown/close code in mux.rs to keep these handlers working against only live
surfaces.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62bb93cd-75f8-4a88-8ab1-aa36ff6ba05f
⛔ Files ignored due to path filters (1)
mux/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
mux/Cargo.tomlmux/bindings/conformance/fixtures.jsonmux/bindings/conformance/runner.pymux/crates/ghostty-vt/src/key.rsmux/crates/ghostty-vt/src/lib.rsmux/crates/mux-core/Cargo.tomlmux/crates/mux-core/src/browser.rsmux/crates/mux-core/src/lib.rsmux/crates/mux-core/src/mux.rsmux/crates/mux-core/src/server.rsmux/crates/mux-core/src/surface.rsmux/crates/mux-core/tests/pty.rsmux/crates/mux-tui/src/app.rsmux/crates/mux-tui/src/cli.rsmux/crates/mux-tui/src/config.rsmux/crates/mux-tui/src/main.rsmux/crates/mux-tui/src/session/mod.rsmux/crates/mux-tui/src/session/remote.rsmux/crates/mux-tui/src/session/tree.rsmux/crates/mux-tui/src/ui/pane.rsmux/crates/mux-tui/src/ui/sidebar.rsmux/crates/mux-tui/tests/cli.rsmux/docs/configuration.mdmux/spec/cli.mdmux/spec/commands.mdmux/spec/events.md
| { | ||
| "name": "implemented-verbs-json", | ||
| "steps": [ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider adding a requires field for backward compatibility.
All eight commands in this fixture (run, wait-for, send-key, copy, ids, notify, list-agents, report-agent) are new in protocol v6. Without a requires field, the fixture will fail hard against older servers instead of skipping gracefully. The existing move-tab-same-position fixture sets "requires": { "commands": ["move-tab"] } for the same reason.
♻️ Suggested addition
{
"name": "implemented-verbs-json",
+ "requires": { "commands": ["run", "wait-for", "send-key", "copy", "ids", "notify", "list-agents", "report-agent"] },
"steps": [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "name": "implemented-verbs-json", | |
| "steps": [ | |
| { | |
| "name": "implemented-verbs-json", | |
| "requires": { "commands": ["run", "wait-for", "send-key", "copy", "ids", "notify", "list-agents", "report-agent"] }, | |
| "steps": [ |
🤖 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 `@mux/bindings/conformance/fixtures.json` around lines 139 - 141, Add a
requires field to the implemented-verbs-json fixture so it skips gracefully on
older servers. Update the fixture definition in fixtures.json to reference the
new protocol v6 command set using the same pattern as move-tab-same-position,
ensuring the fixture is gated by the relevant commands for run, wait-for,
send-key, copy, ids, notify, list-agents, and report-agent.
| name if name.len() == 1 => { | ||
| let c = name.chars().next()?; | ||
| input.key = physical_key_for_char(c); | ||
| if input.key == sys::GHOSTTY_KEY_UNIDENTIFIED { | ||
| return None; | ||
| } | ||
| input.unshifted_codepoint = c.to_ascii_lowercase() as u32; | ||
| if !mods.contains(Mods::CTRL) { | ||
| input.utf8 = c.to_string(); | ||
| if mods.contains(Mods::SHIFT) { | ||
| input.consumed_mods = Mods::SHIFT; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== key.rs relevant section ==\n'
sed -n '100,220p' mux/crates/ghostty-vt/src/key.rs
printf '\n== tests in key.rs / nearby ==\n'
rg -n "send-key|ctrl\\+c|enter|up|invalid chord|shift\\+" mux/crates/ghostty-vt/src -n
printf '\n== all references to consumed_mods / unshifted_codepoint / utf8 ==\n'
rg -n "consumed_mods|unshifted_codepoint|utf8 =" mux/crates/ghostty-vt/src
printf '\n== physical_key_for_char definition ==\n'
rg -n "fn physical_key_for_char|physical_key_for_char\\(" mux/crates/ghostty-vt/src -nRepository: manaflow-ai/cmux
Length of output: 7853
🏁 Script executed:
set -euo pipefail
printf '\n== physical_key_for_char ==\n'
sed -n '1,90p' mux/crates/ghostty-vt/src/key.rs
printf '\n== tests section ==\n'
sed -n '260,360p' mux/crates/ghostty-vt/src/key.rs
printf '\n== encoder-side semantics in Rust wrapper ==\n'
sed -n '200,250p' mux/crates/ghostty-vt/src/key.rs
printf '\n== search for keyboard protocol docs/comments in repo ==\n'
rg -n "unshifted_codepoint|consumed_mods|shift.*utf8|SHIFT.*utf8|GhosttyKeyEvent|kitty keyboard protocol|keyboard protocol" mux ghostty -g '!**/target/**'Repository: manaflow-ai/cmux
Length of output: 25215
🏁 Script executed:
set -euo pipefail
printf '\n== mux-tui key parser ==\n'
sed -n '70,120p' mux/crates/mux-tui/src/keys.rs
printf '\n== key_encode.zig: shift / utf8 handling ==\n'
sed -n '730,770p' ghostty/src/input/key_encode.zig
sed -n '1490,1565p' ghostty/src/input/key_encode.zig
sed -n '2010,2055p' ghostty/src/input/key_encode.zig
sed -n '2160,2285p' ghostty/src/input/key_encode.zig
printf '\n== key.zig event semantics ==\n'
sed -n '1,120p' ghostty/src/input/key.zigRepository: manaflow-ai/cmux
Length of output: 16589
🏁 Script executed:
set -euo pipefail
printf '\n== key_input_from_chord definition and docs ==\n'
rg -n -A4 -B4 "fn key_input_from_chord|send-key|chord" mux/crates/ghostty-vt/src/key.rs mux/crates/mux-tui/src/keys.rs mux/spec/commands.md ghostty/include/ghostty/vt/key.h ghostty/include/ghostty/vt/README.md
printf '\n== any docs/examples mentioning shift+a or shifted symbols ==\n'
rg -n "shift\\+a|shift\\+1|shift\\+,|shift\\+<|shift\\+letter|shifted symbol|shifted" mux ghostty -g '!**/target/**'Repository: manaflow-ai/cmux
Length of output: 5642
Shifted single-character chords need shifted UTF-8.
shift+a hits this branch with c = 'a', so it currently emits "a" while also marking SHIFT as consumed. That drops the shift for letters; utf8 should be "A" there, with unshifted_codepoint staying a. The same parser in mux/crates/mux-tui/src/keys.rs has the same behavior. Shifted symbols like shift+1 and shift+, still need explicit mapping if send-key is expected to support them. Add coverage for shift+a and one shifted symbol.
🤖 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 `@mux/crates/ghostty-vt/src/key.rs` around lines 138 - 151, The
single-character chord handling in the key parsing path is emitting unshifted
UTF-8 for shifted letters, so `shift+a` is treated like plain `a` while `SHIFT`
is consumed. Update the single-character branch in `physical_key_for_char`
handling so letters preserve shifted output in `input.utf8` when `Mods::SHIFT`
is present, while keeping `input.unshifted_codepoint` lowercase and only
consuming shift where appropriate. Apply the same fix in the matching parser in
`keys.rs`, and add tests covering `shift+a` plus one shifted symbol case to
confirm the behavior.
| agent_records: Mutex<HashMap<SurfaceId, AgentRecord>>, | ||
| surface_notifications: Mutex<HashMap<SurfaceId, SurfaceNotification>>, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
agent_records / surface_notifications are never purged when a surface closes — unbounded leak + stale data.
post_notification and report_agent insert into surface_notifications / agent_records keyed by SurfaceId, but close_surface/close_surfaces (unchanged in this diff) only touch state.surfaces/state.panes/workspaces — they never call anything analogous to clear_surface_notification or remove from agent_records. Since Mux lives for the whole session/daemon lifetime and SurfaceId is a monotonically increasing counter that's never reused, every surface that is ever created and closed leaves a permanent orphaned entry in these two maps.
This isn't just a memory-growth concern: list_agents(None, ...) / report-agent+list-agents will keep returning agent state for surfaces that no longer exist in the tree (since list_agents reads agent_records directly, independent of state), which is a real functional/data-integrity problem for any automation consuming this API over a long-running session with surface churn (exactly the intended use case for the new agent-reporting feature).
🐛 Proposed fix — purge on surface removal
pub fn close_surface(&self, target: SurfaceId) {
let (removed, empty) = {
let mut state = self.state.lock().unwrap();
(remove_surface(&mut state, target), state.workspaces.is_empty())
};
if let Some(surface) = removed {
surface.kill();
+ self.agent_records.lock().unwrap().remove(&target);
+ self.surface_notifications.lock().unwrap().remove(&target);
self.emit(MuxEvent::TreeChanged);
}
if empty {
self.emit(MuxEvent::Empty);
}
}Apply the same cleanup in close_surfaces for the batch pane/screen/workspace-close paths.
Also applies to: 370-417
🤖 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 `@mux/crates/mux-core/src/mux.rs` around lines 139 - 140, Purge per-surface
state when a surface is closed: `agent_records` and `surface_notifications` in
`Mux` currently retain entries forever because `post_notification` and
`report_agent` insert by `SurfaceId` but `close_surface`/`close_surfaces` never
remove them. Update the surface teardown path in `Mux::close_surface` and the
batch cleanup in `Mux::close_surfaces` to delete both maps’ entries for every
closed `SurfaceId`, using the existing notification cleanup pattern as the
reference so `list_agents` cannot return stale data.
| fn spawn_attach_notification_stream( | ||
| mux: Arc<Mux>, | ||
| surface_id: SurfaceId, | ||
| writer: LineWriter, | ||
| ) -> std::io::Result<()> { | ||
| let events = mux.subscribe(); | ||
| std::thread::Builder::new() | ||
| .name("mux-attach-notifications".into()) | ||
| .spawn(move || { | ||
| while let Ok(event) = events.recv() { | ||
| let MuxEvent::Notification(notification) = event else { | ||
| continue; | ||
| }; | ||
| if notification.surface != Some(surface_id) { | ||
| continue; | ||
| } | ||
| let value = json!({ | ||
| "event": "notification", | ||
| "notification": notification.notification, | ||
| "title": notification.title, | ||
| "body": notification.body, | ||
| "level": notification.level.as_str(), | ||
| "surface": notification.surface, | ||
| }); | ||
| if writer.send(&value).is_err() { | ||
| break; | ||
| } | ||
| } | ||
| }) | ||
| .map(|_| ()) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
spawn_attach_notification_stream leaks a full-mux subscriber thread per attached surface for the life of the connection.
This thread subscribes to the entire mux event stream and only exits when writer.send() fails — i.e. only when the whole connection closes. Unlike the sibling attach-out thread (which naturally terminates via attach.stream.recv()/frames.notify.recv() returning Err when the specific surface exits), this thread has no tie to surface_id's lifecycle at all. A client that attaches to many distinct surfaces over one long-lived connection (e.g. a TUI switching through many transient tabs) accumulates one permanently-blocked thread + one permanent Mux::subscribers entry per surface ever attached, not per currently-attached surface.
Separately, it's spawned unconditionally before the actual attach succeeds: if surface.attach_stream()? fails for a PTY surface, the command returns an error to the client, but the already-spawned notification thread keeps running regardless.
Consider tying this thread's lifetime to the surface (e.g. break the loop once mux.surface(surface_id) returns None, or drive it from the same select/recv_timeout loop as the primary attach thread and share a shutdown signal), and only spawn it after attach_stream()/attach_frames() succeeds.
RemoteSession's message handler processes incoming JSON events and forwards notification-related updates into the app's event stream.
Also applies to: 1167-1169
🤖 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 `@mux/crates/mux-core/src/server.rs` around lines 670 - 701,
spawn_attach_notification_stream currently creates a mux-wide subscriber thread
that lives until writer.send() fails, so it never ends with the attached surface
and is also started before attach setup succeeds. Update
spawn_attach_notification_stream to tie its recv loop to the target surface_id
lifecycle (for example, exit when mux.surface(surface_id) is gone or share the
same shutdown path as the main attach stream), and only spawn it after the
corresponding attach_stream()/attach_frames() setup has succeeded. Also apply
the same lifetime fix in RemoteSession’s notification handling path.
| fn build_run(flags: &FlagMap) -> Result<Value, UsageError> { | ||
| let mut value = json!({}); | ||
| flags.insert_optional_u64(&mut value, "pane")?; | ||
| flags.insert_optional_string(&mut value, "cwd"); | ||
| flags.insert_optional_string(&mut value, "name"); | ||
| if flags.optional("new-workspace").is_some() { | ||
| value["new_workspace"] = json!(true); | ||
| } | ||
| match (flags.optional("command"), flags.positionals.is_empty()) { | ||
| (Some(command), true) => value["command"] = json!(command), | ||
| (Some(_), false) => { | ||
| return Err(UsageError("--command and argv are mutually exclusive".to_string())); | ||
| } | ||
| (None, false) => value["argv"] = json!(flags.positionals), | ||
| (None, true) => return Err(UsageError("argv or --command is required".to_string())), | ||
| } | ||
| Ok(value) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider validating --pane/--new-workspace mutual exclusion client-side too.
build_run validates --command vs positional argv mutual exclusivity locally, but not --pane vs --new-workspace (only Command::Run on the server rejects that combination). For consistency and faster feedback (no round trip needed), consider adding the same check here.
♻️ Suggested addition
fn build_run(flags: &FlagMap) -> Result<Value, UsageError> {
let mut value = json!({});
flags.insert_optional_u64(&mut value, "pane")?;
flags.insert_optional_string(&mut value, "cwd");
flags.insert_optional_string(&mut value, "name");
if flags.optional("new-workspace").is_some() {
+ if flags.optional("pane").is_some() {
+ return Err(UsageError("--pane and --new-workspace are mutually exclusive".to_string()));
+ }
value["new_workspace"] = json!(true);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn build_run(flags: &FlagMap) -> Result<Value, UsageError> { | |
| let mut value = json!({}); | |
| flags.insert_optional_u64(&mut value, "pane")?; | |
| flags.insert_optional_string(&mut value, "cwd"); | |
| flags.insert_optional_string(&mut value, "name"); | |
| if flags.optional("new-workspace").is_some() { | |
| value["new_workspace"] = json!(true); | |
| } | |
| match (flags.optional("command"), flags.positionals.is_empty()) { | |
| (Some(command), true) => value["command"] = json!(command), | |
| (Some(_), false) => { | |
| return Err(UsageError("--command and argv are mutually exclusive".to_string())); | |
| } | |
| (None, false) => value["argv"] = json!(flags.positionals), | |
| (None, true) => return Err(UsageError("argv or --command is required".to_string())), | |
| } | |
| Ok(value) | |
| } | |
| fn build_run(flags: &FlagMap) -> Result<Value, UsageError> { | |
| let mut value = json!({}); | |
| flags.insert_optional_u64(&mut value, "pane")?; | |
| flags.insert_optional_string(&mut value, "cwd"); | |
| flags.insert_optional_string(&mut value, "name"); | |
| if flags.optional("new-workspace").is_some() { | |
| if flags.optional("pane").is_some() { | |
| return Err(UsageError("--pane and --new-workspace are mutually exclusive".to_string())); | |
| } | |
| value["new_workspace"] = json!(true); | |
| } | |
| match (flags.optional("command"), flags.positionals.is_empty()) { | |
| (Some(command), true) => value["command"] = json!(command), | |
| (Some(_), false) => { | |
| return Err(UsageError("--command and argv are mutually exclusive".to_string())); | |
| } | |
| (None, false) => value["argv"] = json!(flags.positionals), | |
| (None, true) => return Err(UsageError("argv or --command is required".to_string())), | |
| } | |
| Ok(value) | |
| } |
🤖 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 `@mux/crates/mux-tui/src/cli.rs` around lines 665 - 682, build_run currently
checks --command against positional argv locally, but it does not mirror the
server-side mutual exclusion between --pane and --new-workspace. Update
build_run in cli.rs to validate that these two options cannot be used together,
returning a UsageError before constructing the JSON payload, alongside the
existing command/argv checks.
| fn workspace_has_unread(ws: &crate::session::WorkspaceView) -> bool { | ||
| ws.screens | ||
| .iter() | ||
| .flat_map(|screen| screen.panes.iter()) | ||
| .flat_map(|pane| pane.tabs.iter()) | ||
| .any(|tab| tab.notification.is_some_and(|notification| notification.unread)) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Sidebar unread dot ignores notification severity.
Unlike draw_box/draw_tab_bar in ui/pane.rs, which color by the notification's level via notification_color(...), the sidebar dot always uses notification_info regardless of whether the underlying unread notification is warning or error. This loses the severity cue in the one place meant to summarize unread state per workspace.
♻️ Proposed fix to surface severity in the sidebar dot
-fn workspace_has_unread(ws: &crate::session::WorkspaceView) -> bool {
- ws.screens
- .iter()
- .flat_map(|screen| screen.panes.iter())
- .flat_map(|pane| pane.tabs.iter())
- .any(|tab| tab.notification.is_some_and(|notification| notification.unread))
-}
+fn workspace_unread_level(ws: &crate::session::WorkspaceView) -> Option<&'static str> {
+ ws.screens
+ .iter()
+ .flat_map(|screen| screen.panes.iter())
+ .flat_map(|pane| pane.tabs.iter())
+ .filter_map(|tab| tab.notification.filter(|n| n.unread))
+ .map(|n| n.level)
+ .max_by_key(|level| match *level {
+ "error" => 2,
+ "warning" => 1,
+ _ => 0,
+ })
+}- if workspace_has_unread(ws) && content_w > 1 {
- let dot_style =
- style.fg(app.config.theme.notification_info).add_modifier(Modifier::BOLD);
+ if let Some(level) = workspace_unread_level(ws).filter(|_| content_w > 1) {
+ let color = match level {
+ "error" => app.config.theme.notification_error,
+ "warning" => app.config.theme.notification_warning,
+ _ => app.config.theme.notification_info,
+ };
+ let dot_style = style.fg(color).add_modifier(Modifier::BOLD);
buf[(0, y)].set_symbol("•").set_style(dot_style);
}Also applies to: 86-90
🤖 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 `@mux/crates/mux-tui/src/ui/sidebar.rs` around lines 16 - 23, The sidebar
unread indicator in workspace_has_unread/sidebar rendering currently treats
every unread notification the same and ignores severity. Update the sidebar dot
logic to derive its color from the unread tab’s notification level, using the
same notification_color-based severity mapping already used in ui/pane.rs
(draw_box/draw_tab_bar), so warning/error states are preserved instead of always
showing notification_info. Reference the workspace_has_unread helper and the
sidebar dot rendering path when applying the fix.
| let send_key = cli(&server, &["send-key", "--surface", &surface.to_string(), "enter"]); | ||
| assert_success(&send_key); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Verify send-key effect with a follow-up read.
The test only asserts send-key exits successfully but doesn't verify the key was actually received by the terminal. The conformance fixture is more thorough — it sends ["h", "i", "enter"] and then checks scrollback for "hi". Consider adding a follow-up copy or read-screen assertion to catch key-encoding regressions.
♻️ Suggested addition
let send_key = cli(&server, &["send-key", "--surface", &surface.to_string(), "enter"]);
assert_success(&send_key);
+
+ // Verify the key was received by the terminal.
+ let after_key = wait_for_screen(&server, surface, "\n");
+ // Or use copy --mode scrollback to check for expected output.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let send_key = cli(&server, &["send-key", "--surface", &surface.to_string(), "enter"]); | |
| assert_success(&send_key); | |
| let send_key = cli(&server, &["send-key", "--surface", &surface.to_string(), "enter"]); | |
| assert_success(&send_key); | |
| // Verify the key was received by the terminal. | |
| let after_key = wait_for_screen(&server, surface, "\n"); | |
| // Or use copy --mode scrollback to check for expected output. |
🤖 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 `@mux/crates/mux-tui/tests/cli.rs` around lines 123 - 124, The send-key test
only checks that the command succeeds, but it does not confirm the key actually
reached the terminal. Update the `send_key` scenario in the CLI test to follow
the `send-key` call with a readback assertion, using the existing test helpers
and the same `surface`/`cli` flow, so it verifies terminal output (for example
via `copy` or `read-screen`) instead of only exit status. Use the surrounding
test case in `cli.rs` as the place to add the follow-up verification and mirror
the conformance fixture’s pattern of sending input then asserting the resulting
screen text.
| | `wait-for` | implemented | `--surface <id> --pattern <regex> --timeout-ms <n>` | none | none | | ||
| | `run` | implemented | `-- <argv...>` or `--command <cmd>` | `--pane <id>`, `--new-workspace`, `--cwd <path>`, `--name <name>` | surface id | | ||
| | `send-key` | implemented | `--surface <id> <key>...` | none | none | | ||
| | `copy` | implemented | `--surface <id> --mode screen|selection|scrollback` | none | text | | ||
| | `ids` | implemented | none | `--kind workspace|screen|pane|surface` | id lines | | ||
| | `notify` | implemented | `--title <title> --body <body>` | `--level info|warning|error`, `--surface <id>` | notification id | | ||
| | `list-agents` | implemented | none | `--surface <id>`, `--state <state>` | agent lines | | ||
| | `report-agent` | implemented | `--surface <id> --state <state> --source socket|hook` | `--session <id>` | none | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipe-separated values in the verb table.
The | characters in these cells are being parsed as extra columns, so the table renders incorrectly for copy, ids, notify, and report-agent.
Proposed fix
-| `copy` | implemented | `--surface <id> --mode screen|selection|scrollback` | none | text |
-| `ids` | implemented | none | `--kind workspace|screen|pane|surface` | id lines |
-| `notify` | implemented | `--title <title> --body <body>` | `--level info|warning|error`, `--surface <id>` | notification id |
-| `report-agent` | implemented | `--surface <id> --state <state> --source socket|hook` | `--session <id>` | none |
+| `copy` | implemented | `--surface <id> --mode screen\|selection\|scrollback` | none | text |
+| `ids` | implemented | none | `--kind workspace\|screen\|pane\|surface` | id lines |
+| `notify` | implemented | `--title <title> --body <body>` | `--level info\|warning\|error`, `--surface <id>` | notification id |
+| `report-agent` | implemented | `--surface <id> --state <state> --source socket\|hook` | `--session <id>` | none |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `wait-for` | implemented | `--surface <id> --pattern <regex> --timeout-ms <n>` | none | none | | |
| | `run` | implemented | `-- <argv...>` or `--command <cmd>` | `--pane <id>`, `--new-workspace`, `--cwd <path>`, `--name <name>` | surface id | | |
| | `send-key` | implemented | `--surface <id> <key>...` | none | none | | |
| | `copy` | implemented | `--surface <id> --mode screen|selection|scrollback` | none | text | | |
| | `ids` | implemented | none | `--kind workspace|screen|pane|surface` | id lines | | |
| | `notify` | implemented | `--title <title> --body <body>` | `--level info|warning|error`, `--surface <id>` | notification id | | |
| | `list-agents` | implemented | none | `--surface <id>`, `--state <state>` | agent lines | | |
| | `report-agent` | implemented | `--surface <id> --state <state> --source socket|hook` | `--session <id>` | none | | |
| | `wait-for` | implemented | `--surface <id> --pattern <regex> --timeout-ms <n>` | none | none | | |
| | `run` | implemented | `-- <argv...>` or `--command <cmd>` | `--pane <id>`, `--new-workspace`, `--cwd <path>`, `--name <name>` | surface id | | |
| | `send-key` | implemented | `--surface <id> <key>...` | none | none | | |
| | `copy` | implemented | `--surface <id> --mode screen\|selection\|scrollback` | none | text | | |
| | `ids` | implemented | none | `--kind workspace\|screen\|pane\|surface` | id lines | | |
| | `notify` | implemented | `--title <title> --body <body>` | `--level info\|warning\|error`, `--surface <id>` | notification id | | |
| | `list-agents` | implemented | none | `--surface <id>`, `--state <state>` | agent lines | | |
| | `report-agent` | implemented | `--surface <id> --state <state> --source socket\|hook` | `--session <id>` | none | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 86-86: Table column count
Expected: 5; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 87-87: Table column count
Expected: 5; Actual: 8; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 88-88: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 88-88: Table column count
Expected: 5; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 90-90: Table column count
Expected: 5; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 `@mux/spec/cli.md` around lines 83 - 90, The verb table in cli.md is being
parsed incorrectly because the pipe-separated option values are treated as extra
columns. Update the affected rows for copy, ids, notify, and report-agent in the
table so the literal pipe characters inside cells are escaped, keeping the
column structure intact while preserving the existing unique command symbols.
Source: Linters/SAST tools
This branch was previously deployed
1 participant
Implements the eight commands that were marked
proposedinmux/spec/commands.md, server-side in mux-core with CLI verbs and full tests, and flips their spec status to implemented. Protocol stays v6 (additive).--surface --pattern --timeout-ms: blocks on the surface's output stream (attach-then-check ordering, so a one-shot match that lands before the wait is never missed), regex match with a real deadline, no busy-spin.-- <argv>/--command: spawns a surface running an explicit command (vs the login shell), with--pane/--new-workspace/--cwd/--nameplacement.<key>...: encodes named keys/chords (enter, ctrl+c, arrows, alt+…) through the ghostty key encoder synced from the terminal's modes.--mode screen|selection|scrollback: viewport / current selection / full scrollback text.--kind: tree ids (with short ids).--title --body [--level] [--surface]: per-surface unread state + anotificationevent. The mux TUI renders the user-requested UX — an attention border on the selected tab/pane in the level color, an unread dot in the tab bar, and a workspace sidebar dot — cleared when the tab is focused, never stealing focus, and skipped for the already-active surface. A RatatuiTestBackendtest asserts all three indicators appear and clear.Tests: per-command mux-core behavior tests, a CLI integration subset, the Ratatui render test, and a new conformance fixture so the commands run (not SKIP). Reviewed via a 2-round plan/code/judge loop (APPROVE). Verified locally: cargo fmt/clippy/test (130), conformance 6/6, smoke-tui, and a live CLI transcript exercising every verb (incl. agent-authority ordering and wait-for match/timeout).
Note: this and #7412 (agent state machine) both touch per-surface agent state; whichever lands second reconciles the store.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Adds blocking
wait-foron the control socket and new PTY spawn paths viarun, plus agent/notification state that must stay consistent with related work (#7412); scope is large but covered by conformance and integration tests.Overview
Implements the eight proposed protocol-6 control socket commands (
wait-for,run,send-key,copy,ids,notify,list-agents,report-agent) end-to-end: mux-core handlers, mux-tui CLI verbs, specs marked implemented, plus a conformance fixture that exercises the full flow.Automation:
wait-forregex-matches viewport text on the surface attach stream;runspawns PTY tabs with explicitargvor shellcommandand placement options;send-keyuses newkey_input_from_chordin ghostty-vt;copyreads screen, scrollback, or mux-stored selection text.Telemetry & UI:
notifydrives per-surface unread state,notificationevents on subscribe/attach, and unread metadata inlist-workspaces. The TUI shows level-colored pane borders, tab-bar dots, and sidebar dots (theme keys added), cleared when the user views the tab without stealing focus.report-agent/list-agentsstore agent state with hook over socket authority.Adds
regexdependency, mux unit/integration tests, and selection persistence when copying from the TUI.Reviewed by Cursor Bugbot for commit 6bdfc73. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Implements the eight proposed protocol‑6 commands across
mux-coreand the CLI, adds a notification event with TUI indicators, and keeps the protocol at v6. This enables automation sync, explicit command spawns, key injection, text copy, id listing with short ids, notifications, and agent state APIs.New Features
-- <argv>or--command, place in--paneor--new-workspace, set--cwd/--name; returns surface/pane/screen/workspace.ghostty-vtencoder synced to terminal modes.screen|selection|scrollbacktext.notificationevents.Dependencies
regextomuxandmux-core.Written for commit 6bdfc73. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
wait-fornow supports timeout-based matching more reliably.