From 2e1c589ef753f665b3ddd8ec1b1659cf82ededc0 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 23 Oct 2026 20:39:36 +0000 Subject: [PATCH] fix(acpmux): guard sidebar group labels and empty groups Agent-Session: https://cmux.com/deeplink/agent?harness=codex&session=01a16a55-c3f2-79d1-8872-cb25f230352a&host=huge-blue --- .../Handlers/AgentSessionWorkspace.swift | 33 +++++++++++++++++ .../AgentSessionGroupJoinTests.swift | 12 ++++++ cmux-tui/crates/acpmux/docs/agent-surfaces.md | 14 +++++-- cmux-tui/crates/acpmux/skills/acpmux/SKILL.md | 14 +++++-- cmux-tui/crates/acpmux/src/cli/run/launch.rs | 37 +++++++++++++++++++ 5 files changed, 103 insertions(+), 7 deletions(-) diff --git a/Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/AgentSessionWorkspace.swift b/Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/AgentSessionWorkspace.swift index e227f9ae37eb..05d6d70dd605 100644 --- a/Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/AgentSessionWorkspace.swift +++ b/Packages/macOS/CmuxNext/Sources/CmuxNextApp/Handlers/AgentSessionWorkspace.swift @@ -42,6 +42,9 @@ enum AgentSessionWorkspace { // The Chief host's own acpmux runs the session (`chief:`); absent: this Mac's. let host = invocation["host"]?.stringValue.flatMap { $0.isEmpty ? nil : $0 } let group = invocation["group"]?.stringValue.flatMap { $0.isEmpty ? nil : $0 } + if let group, !validGroupName(group) { + throw ActionFailure(message: "agent.openSessionWorkspace: group must be a non-empty human label, not a cmux id; omit group or use a label such as \"GUI push\"") + } let task = try open(session: session, name: name, key: key, cwd: cwd, host: host, group: group, services: context.services) context.services.registry.track(task) } @@ -142,6 +145,7 @@ enum AgentSessionWorkspace { /// new group of that name when none can hold it: the path of a row drag and of New Group, so it is one /// undoable sidebar edit. @MainActor static func join(_ id: String, group name: String, sidebar: SidebarBridge) { + guard containsWorkspace(id, in: sidebar.model.sections) else { return } let ids = [SidebarWorkspaceID(id)] if let target = group(named: name, accepting: SidebarWorkspaceID(id), in: sidebar.model.sections) { sidebar.handle(.move(ids, toGroup: target)) @@ -150,6 +154,35 @@ enum AgentSessionWorkspace { sidebar.groupFlow.newGroup(of: ids, name: name, byUser: false) } + /// A workspace must still be listed before a group is created. A late or + /// failed agent-tab attach otherwise leaves an empty persistent group. + static func containsWorkspace(_ id: String, in sections: [CmuxNextSidebar.SidebarSection]) -> Bool { + sections.contains { section in + section.nodes.contains { node in + switch node { + case .workspace(let workspace): + return workspace.id.rawValue == id + case .group(let group): + return group.workspaces.contains { $0.id.rawValue == id } + case .item: + return false + } + } + } + } + + /// Sidebar group arguments are labels. Object ids are rejected so an + /// automation retry cannot turn an id into a persistent visible title. + static func validGroupName(_ name: String) -> Bool { + let label = name.trimmingCharacters(in: .whitespacesAndNewlines) + guard !label.isEmpty else { return false } + let lower = label.lowercased() + if ["grp_", "ws_", "tab_", "pane_", "win_", "workspace_group:"].contains(where: lower.hasPrefix) { + return false + } + return UUID(uuidString: label) == nil + } + /// The first listed group named `name` that may hold workspace `id` (the drag's rule, /// `SidebarEdits.canMove`: a machine row's group holds that machine's workspaces), in any section and /// either organization; nil when none may. Only the first group of that name was tried before, so a diff --git a/Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/AgentSessionGroupJoinTests.swift b/Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/AgentSessionGroupJoinTests.swift index aff2a9b33579..d5fbbd30b140 100644 --- a/Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/AgentSessionGroupJoinTests.swift +++ b/Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/AgentSessionGroupJoinTests.swift @@ -76,4 +76,16 @@ import Testing ])] #expect(AgentSessionWorkspace.predecessor(named: "pane-motion", inGroup: Self.lanes, in: sections) == nil) } + + @Test func aMissingWorkspaceCannotCreateAnEmptyGroup() { + let sections = [SidebarSection(kind: .machine(Self.local), nodes: [])] + #expect(!AgentSessionWorkspace.containsWorkspace("late-workspace", in: sections)) + } + + @Test func groupNamesRejectObjectIdsAndBlankLabels() { + for name in ["", " ", "grp_012345", "ws_012345", "550e8400-e29b-41d4-a716-446655440000"] { + #expect(!AgentSessionWorkspace.validGroupName(name), name) + } + #expect(AgentSessionWorkspace.validGroupName("GUI push")) + } } diff --git a/cmux-tui/crates/acpmux/docs/agent-surfaces.md b/cmux-tui/crates/acpmux/docs/agent-surfaces.md index 454b188cd6e6..a74c220f1f7d 100644 --- a/cmux-tui/crates/acpmux/docs/agent-surfaces.md +++ b/cmux-tui/crates/acpmux/docs/agent-surfaces.md @@ -6,7 +6,7 @@ by who is calling, not by what is possible. | You are | Start an agent for a task | Talk to it, wait | Show it to the person | | --- | --- | --- | --- | -| A script, a person, or an agent with a shell | `acpmux new -n NAME --worktree REPO[@BASE] --prompt-file F --report-to ME --group G -d` | `acpmux send`, `acpmux wait`, `acpmux last` | `--group G` on the same command (runs `cmux agent open`) | +| A script, a person, or an agent with a shell | `acpmux new -n NAME --worktree REPO[@BASE] --prompt-file F --report-to ME -d` | `acpmux send`, `acpmux wait`, `acpmux last` | Optional `--group G` on the same command (runs `cmux agent open`) | | An agent inside acpmux that wants a child (`cmux-subsessions` MCP) | `start_session {prompt, name, worktree: true}` | `send_message`, `wait_for_sessions`, `get_session` | `app_agent_open {session, group}` through the cmux MCP | | The cmux app or an agent with the cmux MCP | the app's own New Chat, or one of the two above | the chat pane | `app_agent_open` (`cmux agent open SESSION [--group G]`), or a tab of a pane: `app_agent_open_tab` (`cmux agent open-tab SESSION --target PANE`) | @@ -47,9 +47,15 @@ acpmux new -n gui/composer \ every other run of characters becomes one `-`, at most 48 characters (`Gui/Composer` is session `gui-composer`, folder `gui-composer`, branch `cmux/gui-composer`); the name as given is the sidebar title. -- **Sidebar.** `--group G` runs `cmux agent open SESSION --group G` with a - workspace key derived from the session id, so a second run finds the same - row and opens nothing. A failed sidebar step warns; the prompt is still sent. +- **Sidebar (opt-in).** `--group G` is a persistent app-side effect: it runs + `cmux agent open SESSION --group G`, creates a workspace, and places it in + the named sidebar group (made when missing). Use a human label, never a + `grp_…`/`ws_…`/UUID object id; omit it unless the person asked for placement. + The workspace key is derived from the session id, so a second run finds the + same row and opens nothing. A failed sidebar step warns; the prompt is still + sent. To recover, run `cmux --json workspace group list`, then use + `cmux workspace group remove --workspace ` or + `cmux workspace group delete` for the affected empty group. It needs the cmux app on this machine. Another machine's session cannot be opened in a group yet: `cmux agent open` takes only this Mac's session ids (no `HOST/NAME`), and its `host` only a Chief home (`chief:`). diff --git a/cmux-tui/crates/acpmux/skills/acpmux/SKILL.md b/cmux-tui/crates/acpmux/skills/acpmux/SKILL.md index 0546c72cc8b8..201a62ef7404 100644 --- a/cmux-tui/crates/acpmux/skills/acpmux/SKILL.md +++ b/cmux-tui/crates/acpmux/skills/acpmux/SKILL.md @@ -131,13 +131,21 @@ acpmux session kill --purge NAME # delete (only sessions you c `send` on a busy or blocked session never refuses: the prompt queues and the output says `queued behind …`. -Start an agent for a task (its own worktree, its brief, shown in the app) with -one command; running it again reuses the session and worktree and sends nothing: +Start an agent for a task (its own worktree and brief) with one command; running +it again reuses the session and worktree and sends nothing: ```bash -acpmux new -n NAME --worktree REPO[@BASE] --prompt-file BRIEF.md --report-to ME --group "GROUP" -d +acpmux new -n NAME --worktree REPO[@BASE] --prompt-file BRIEF.md --report-to ME -d ``` +`--group "GROUP"` is optional person-facing app state. It runs `cmux agent +open`, creates a workspace and places it in the named cmux sidebar group (and +may create that group); it does not move focus. Use a human label only, never a +`grp_…`, `ws_…` or other object id. Omit it unless the person asked for this +sidebar placement. To recover from an unwanted placement, inspect +`cmux --json workspace group list`, then move/delete the affected workspace or +group with `cmux workspace group …`; rerunning without `--group` is safe. + Never script `git worktree add` + `acpmux new` + `acpmux send` by hand. Which surface for what (CLI, `cmux-subsessions` MCP, cmux MCP): `cmux-tui/crates/acpmux/docs/agent-surfaces.md` in the cmux repository. diff --git a/cmux-tui/crates/acpmux/src/cli/run/launch.rs b/cmux-tui/crates/acpmux/src/cli/run/launch.rs index 71b7d23016b2..ce87d5048fa6 100644 --- a/cmux-tui/crates/acpmux/src/cli/run/launch.rs +++ b/cmux-tui/crates/acpmux/src/cli/run/launch.rs @@ -76,6 +76,9 @@ impl Launch { } }; let file_prompt = args.prompt_file.as_deref().map(read_prompt_file).transpose()?; + if let Some(group) = &args.group { + validate_group(group)?; + } if args.group.is_some() && name.is_none() { bail!("--group needs --name, so a second run finds the same sidebar row"); } @@ -159,6 +162,23 @@ pub(crate) fn session_name(name: &str) -> Result { Ok(out) } +/// A group is a person's label, not a cmux object id. Rejecting ids before +/// `session/new` keeps a bad launcher from creating persistent sidebar junk. +pub(crate) fn validate_group(group: &str) -> Result<()> { + let label = group.trim(); + let lower = label.to_ascii_lowercase(); + let id_prefix = ["grp_", "ws_", "tab_", "pane_", "win_", "workspace_group:"] + .iter() + .any(|prefix| lower.starts_with(prefix)); + let uuid = uuid::Uuid::parse_str(label).is_ok(); + if label.is_empty() || id_prefix || uuid { + bail!( + "--group must be a non-empty human label, not a cmux id; use e.g. `--group \"GUI push\"`, or omit --group" + ); + } + Ok(()) +} + /// `REPO[@BASE]`: the last `@` splits off the base (a ref or commit), /// unless the whole of it is a folder. fn split_worktree(spec: &str) -> (&str, Option<&str>) { @@ -350,6 +370,23 @@ mod tests { assert!(session_name("//").is_err()); } + #[test] + fn a_group_is_a_human_label_and_never_an_object_id() { + for group in [ + "", + " ", + "grp_012345", + "ws_012345", + "workspace_group:42", + "550e8400-e29b-41d4-a716-446655440000", + ] { + let error = validate_group(group).unwrap_err().to_string(); + assert!(error.contains("human label"), "{group:?}: {error}"); + assert!(error.contains("omit --group"), "{group:?}: {error}"); + } + assert!(validate_group("GUI push").is_ok()); + } + #[test] fn the_last_at_splits_off_the_base() { assert_eq!(split_worktree("~/r@origin/feat"), ("~/r", Some("origin/feat")));