Skip to content

linux(socket): implement surface.action / tab.action (rename, pin, mark_read) - #218

Merged
Jesssullivan merged 2 commits into
mainfrom
sid/socket-surface-action-rename
Apr 18, 2026
Merged

Jesssullivan merged 2 commits into
mainfrom
sid/socket-surface-action-rename

Conversation

@Jesssullivan

Copy link
Copy Markdown
Owner

Summary

Implements surface.action and its tab.action alias in the Linux daemon's socket dispatcher, mirroring macOS v2TabAction. Closes the most-requested per-surface metadata gap.

Supported actions (all map directly to existing fields on Panel in cmux-linux/src/workspace.zig):

Action Effect
rename Panel.custom_title = title
clear_name Panel.custom_title = null
pin / unpin Panel.is_pinned = true/false
mark_read Panel.is_manually_unread = false
mark_unread Panel.is_manually_unread = true

Tab-relative close/new actions (close_left, close_right, close_others, new_terminal_right, new_browser_right, reload, duplicate) are recognized but return a structured "action not implemented on linux" error — so callers can detect a parity gap vs. a typo without guessing.

workspace_id, surface_id, and tab_id parameters all mirror the macOS shape. When surface_id/tab_id are omitted, the focused surface is used.

The handler is registered for both surface.action and tab.action because Linux currently uses a 1:1 panel:pane mapping (one surface per pane = one "tab").

Test plan

  • Pure-socket Python test added: tests_v2/test_surface_action_rename.py
    • Exercises every supported action against a fresh, isolated workspace
    • Verifies rename round-trips through surface.list
    • Verifies the tab.action alias hits the same handler
    • Verifies focused-surface fallback (no surface_id)
    • Cleans up its own workspace
  • No GUI/CLI binary dependency — runs unchanged on both macOS and Linux daemons
  • Linux CI socket tests pick up the new test automatically (current scripts/run-socket-tests.sh blacklist has no exclusion for test_surface_action_*)
  • After test(socket): allowlist runner + Phase 1 candidate gate #217 (Phase 1 allowlist) merges, follow-up commit will add test_surface_action_rename to BASELINE

Notes

  • Title echo uses the existing std.fmt.allocPrint pattern (matches handleSurfaceList etc.). The pre-existing JSON-injection vulnerability in title echoing is left unchanged here; addressing it across the dispatcher is a separate cleanup PR.
  • Sidebar UI refresh is invoked after each mutation via window.getSidebar().refresh() — same pattern as handleWorkspaceRename.

Refs: TIN-183
Closes part of #216

…ar_name, pin/unpin, mark_read/unread

Mirrors the macOS v2TabAction dispatcher (Sources/TerminalController.swift)
for the trivial property-mutation actions on Linux's 1:1 panel:pane model.

The supported actions all map directly to fields already on the Linux
Panel struct (workspace.zig):
- rename       -> Panel.custom_title = title
- clear_name   -> Panel.custom_title = null
- pin / unpin  -> Panel.is_pinned
- mark_read    -> Panel.is_manually_unread = false
- mark_unread  -> Panel.is_manually_unread = true

Tab-relative close/new actions (close_left, close_right, close_others,
new_terminal_right, new_browser_right, reload, duplicate) are recognized
but return a structured "action not implemented on linux" error so callers
can distinguish a parity gap from an unknown action.

Workspace, surface_id, and tab_id parameters mirror the macOS shape. When
no surface_id/tab_id is provided, the focused surface is used.

The handler is registered for both "surface.action" and "tab.action" since
Linux uses 1:1 panel:pane mapping (one surface per pane = one tab).

Adds a socket-only Python test (tests_v2/test_surface_action_rename.py)
that exercises every supported action through the protocol — no GUI or
CLI binary required, so it runs unchanged on both macOS and Linux daemons.

Refs: TIN-183
Closes part of #216
@greptile-apps

greptile-apps Bot commented Apr 18, 2026 •

Copy link
Copy Markdown

Greptile Summary

This PR implements surface.action / tab.action in the Linux socket dispatcher, covering rename, clear_name, pin/unpin, and mark_read/mark_unread — all mapping directly to existing Panel fields. The three issues flagged in the previous review round (JSON injection in the rename echo, dangling pointer on alloc failure, and workspace leak in the test) are all correctly addressed.

Confidence Score: 5/5

Safe to merge; all previous P0/P1 findings are resolved and remaining findings are minor test-quality suggestions.

The three blocking issues from the prior review round — JSON injection, dangling-pointer on alloc failure, and workspace leak in the test — are all correctly fixed. The new handler follows the established buf.writer(alloc) / writeJsonString pattern consistently. Remaining findings are P2: one vacuous assertion in the unsupported-action test branch, and a missing surface.list round-trip check for the focused-surface fallback path.

tests_v2/test_surface_action_rename.py — minor test coverage gaps in the unsupported-action and focused-surface fallback branches.

Important Files Changed

Filename Overview
cmux-linux/src/socket.zig Adds handleSurfaceAction for surface.action/tab.action; all three previous P0/P1 concerns (JSON injection in rename echo, dangling pointer on alloc failure, and memory layout) are addressed correctly — uses writeJsonString, alloc-before-free ordering, and matches the existing buf.writer(alloc) pattern throughout the file.
tests_v2/test_surface_action_rename.py Comprehensive happy-path coverage; workspace leak is fixed with try/finally. Two minor test-quality gaps: the unsupported-action assertion is vacuous (accepts both error and success), and the focused-surface fallback rename is not verified via surface.list.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Dispatcher as socket.zig dispatcher
    participant Handler as handleSurfaceAction
    participant WS as Workspace / panels map
    participant Sidebar

    Client->>Dispatcher: surface.action / tab.action
    Dispatcher->>Handler: route

    Handler->>WS: resolve workspace (workspace_id or selected)
    Handler->>WS: resolve panel (surface_id → tab_id → focused_panel_id)

    alt action = rename
        Handler->>WS: alloc.dupe(title), free old custom_title
        Handler->>Sidebar: refresh()
        Handler-->>Client: {action:rename, surface_id, title} via writeJsonString
    else action = clear_name
        Handler->>WS: free old, custom_title = null
        Handler->>Sidebar: refresh()
        Handler-->>Client: {action:clear_name, surface_id}
    else action = pin / unpin
        Handler->>WS: is_pinned = true/false
        Handler->>Sidebar: refresh()
        Handler-->>Client: {action:pin/unpin, pinned:true/false}
    else action = mark_read / mark_unread
        Handler->>WS: is_manually_unread = false/true
        Handler->>Sidebar: refresh()
        Handler-->>Client: {action:mark_read/mark_unread, surface_id}
    else close_left/right/others/new_*/reload/duplicate
        Handler-->>Client: {error: action not implemented on linux}
    else unknown action
        Handler-->>Client: {error: unsupported action, supported:[...]}
    end
Loading

Reviews (2): Last reviewed commit: "linux(socket): address Greptile P1s on s..." | Re-trigger Greptile

Comment thread cmux-linux/src/socket.zig Outdated
Comment thread cmux-linux/src/socket.zig Outdated
Comment thread tests_v2/test_surface_action_rename.py Outdated
Two real findings from Greptile review on PR #218:

1. **Dangling pointer on alloc failure** (P1) — handleSurfaceAction's
   rename path freed `panel.custom_title` before attempting `dupe`. If
   dupe fails, the early return leaves the field pointing at freed
   memory and a follow-up surface.list/action will use-after-free. Fix:
   allocate the new dupe FIRST, then free the old, then assign.

2. **JSON injection in title echo** (P1, security) — `title` and
   `action` were interpolated directly into the JSON envelope via
   `{s}`, so a value containing `"`, `\\`, or control characters would
   produce malformed JSON the client cannot parse. Fix: use the
   existing `writeJsonString` helper (line 460) to escape user-supplied
   strings in the rename echo, the "not implemented" error, and the
   "unsupported action" error.

Also addresses the P2 finding on test_surface_action_rename.py — the
explicit `c.close_workspace(...)` cleanup at the end of the test ran
only when no assertion raised, leaking the workspace on failure. Refactor
the assertion body into `_run_assertions(c, ws_id)` and run cleanup in a
`finally:` block so partial test failures cannot corrupt subsequent runs
on the same daemon.

Refs: cmux PR #218 review.
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
Mirrors the fix on PR #218 (commit 01e3be2) — the workspace.action
handler had the same two patterns that Greptile flagged as P1 on
surface.action:

1. **Use-after-free on alloc failure** — rename and set_color freed
   ws.custom_title / ws.custom_color before attempting the dupe of the
   new value. If dupe fails, the early return leaves the field pointing
   at freed memory. Fix: allocate the new value FIRST, then free the
   old, then assign.

2. **JSON injection in echoed user input** — title, color, and action
   were interpolated directly into the response envelope via `{s}`. A
   value containing `"`, `\\`, or control characters would produce
   malformed JSON the client cannot parse. Fix: build the response with
   ArrayList + writer + writeJsonString for every user-supplied string
   (title, color, action — including the close_*/unimplemented/
   unsupported error envelopes).

Refs: cmux PR #221 (preemptive — same review will surface here).
@Jesssullivan

Copy link
Copy Markdown
Owner Author

Thanks Greptile — both P1 findings and the P2 test cleanup gap fixed in commit 01e3be2:

  • Use-after-free on alloc failure (line 1277): allocate dupe first, then free old. panel.custom_title is never assigned a stale pointer.
  • JSON injection in title echo (line 1283): title and action (in the unimplemented + unsupported error envelopes) are now escaped via the existing writeJsonString helper. The rename echo uses an ArrayList + writer pattern so ", \\, and control characters in user-supplied titles cannot break the JSON envelope.
  • Workspace leaked on test failure (test line 197): refactored the assertion body into _run_assertions(c, ws_id) and moved cleanup into a finally: block so partial failures cannot leak workspaces between test runs on the same daemon.

Same alloc-then-free / escape-on-echo patterns also applied preemptively to handleWorkspaceAction on PR #221 (commit 3b0b92a).

Jesssullivan added a commit that referenced this pull request Apr 18, 2026
The macOS build gates the v2 socket behind an optional password handshake.
The Linux build does not yet wire any credential store, but the v2 protocol
is still expected to expose auth.login so v1/v2 clients can probe the gate
deterministically instead of hitting method_not_found.

Add a constant handler that always returns {authenticated:true, required:false}
matching the macOS response shape, and a socket round-trip test exercising
no-params, spurious password params, repeated calls, and a follow-up
system.ping to assert no implicit gate is engaged.

Sprint A item #4 from #220 (TIN-183 follow-up after #218).
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
The macOS build ships a system.tree RPC that flattens the entire UI hierarchy
into a single response so clients (CLIs, shell prompts, status bars) can avoid
N+1 round-trips. Linux currently exposes the leaf RPCs (window.list,
workspace.list, surface.list, pane.list) but not the composed view.

Add handleSystemTree mirroring the macOS shape:
  active: { workspace_id, surface_id, window_id }   // matches identify
  windows[]: { id, ref, index, workspace_count, selected_workspace_id,
               workspaces[]: { id, ref, index, title, selected, pinned,
                               panes[]: { id, ref, index, focused, surface_count,
                                          selected_surface_id, surfaces[]: {
                                            id, ref, index, index_in_pane, type,
                                            focused, selected, selected_in_pane,
                                            pane_id, pane_ref, title } } } }

Linux uses 1:1 panel:pane today, so each pane has exactly one surface and
pane.id == surface.id. The shape leaves room for future pane grouping
(multiple surfaces per pane) without breaking clients.

The handler is alloc-free in the steady-state error path: every writer.* call
that fails returns the same canonical empty envelope so partial responses
never leak. Workspace and panel titles flow through writeJsonString to keep
user-supplied strings safe.

The new socket round-trip test creates an extra workspace, asserts the
envelope, restores the baseline workspace selection, and closes the
scratch workspace in a finally block so it never leaves stale state.

Sprint A item #5 from #220 (TIN-183 follow-up after #218 / #221 / #222).
@Jesssullivan
Jesssullivan merged commit b8e2163 into main Apr 18, 2026
26 of 27 checks passed
@Jesssullivan
Jesssullivan deleted the sid/socket-surface-action-rename branch April 18, 2026 04:44
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
…close_*) (#221)

* linux(socket): implement workspace.action with 12 sub-actions

Mirrors macOS v2WorkspaceAction (Sources/TerminalController.swift). Wires
the dispatch entry plus a handler that supports the property-mutation and
ordering actions that map onto existing Linux primitives:

  rename, clear_name, pin, unpin, set_color, clear_color,
  move_up, move_down, move_top, close_above, close_below, close_others

Recognized macOS actions that have no Linux backing field yet
(set_description, clear_description, mark_read, mark_unread) return a
structured "action not implemented on linux" error so callers can detect
parity gaps without falling off a cliff. Unknown actions return an
"unsupported action" error with a "supported" allowlist.

The close_above/below/others variants pre-compute the set of target IDs
before mutation to avoid the index-shift bug that would otherwise occur
when closeWorkspace renumbers the workspace list mid-loop. Pinned
workspaces are skipped, matching macOS semantics.

Includes tests_v2/test_workspace_action.py — a pure socket round-trip
test that exercises every supported action, the "not implemented" error
path, the unknown-action error path, and the focused-workspace fallback
when no workspace_id is provided. Cleanup pins baselines so the close_*
suite cannot disturb pre-existing workspaces.

Refs: cmux issue #220 (Phase 3 Sprint A item 1).

* linux(socket): apply Greptile P1 fixes to workspace.action

Mirrors the fix on PR #218 (commit 01e3be2) — the workspace.action
handler had the same two patterns that Greptile flagged as P1 on
surface.action:

1. **Use-after-free on alloc failure** — rename and set_color freed
   ws.custom_title / ws.custom_color before attempting the dupe of the
   new value. If dupe fails, the early return leaves the field pointing
   at freed memory. Fix: allocate the new value FIRST, then free the
   old, then assign.

2. **JSON injection in echoed user input** — title, color, and action
   were interpolated directly into the response envelope via `{s}`. A
   value containing `"`, `\\`, or control characters would produce
   malformed JSON the client cannot parse. Fix: build the response with
   ArrayList + writer + writeJsonString for every user-supplied string
   (title, color, action — including the close_*/unimplemented/
   unsupported error envelopes).

Refs: cmux PR #221 (preemptive — same review will surface here).

* linux(socket): name WorkspaceLookup so peer typing works

CI on this branch failed across every distro with:

    src/socket.zig:936:19: error: incompatible types:
      'socket.findWorkspaceById__struct_7670' and
      'socket.handleWorkspaceAction__struct_7750'

The fallback branch in handleWorkspaceAction constructed an anonymous
struct literal `.{ .ws = ..., .index = ... }`, which Zig treats as a
distinct type from the anonymous struct returned by findWorkspaceById,
breaking peer typing in the `if/else` expression.

Promote the lookup result into a named struct (`WorkspaceLookup`) and
annotate the `found` binding so both arms of the conditional coerce to
the same type. The literal in the else block now constructs the named
type explicitly.

Local regression-only; no behavior change.
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
The macOS build gates the v2 socket behind an optional password handshake.
The Linux build does not yet wire any credential store, but the v2 protocol
is still expected to expose auth.login so v1/v2 clients can probe the gate
deterministically instead of hitting method_not_found.

Add a constant handler that always returns {authenticated:true, required:false}
matching the macOS response shape, and a socket round-trip test exercising
no-params, spurious password params, repeated calls, and a follow-up
system.ping to assert no implicit gate is engaged.

Sprint A item #4 from #220 (TIN-183 follow-up after #218).
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
The macOS build ships a system.tree RPC that flattens the entire UI hierarchy
into a single response so clients (CLIs, shell prompts, status bars) can avoid
N+1 round-trips. Linux currently exposes the leaf RPCs (window.list,
workspace.list, surface.list, pane.list) but not the composed view.

Add handleSystemTree mirroring the macOS shape:
  active: { workspace_id, surface_id, window_id }   // matches identify
  windows[]: { id, ref, index, workspace_count, selected_workspace_id,
               workspaces[]: { id, ref, index, title, selected, pinned,
                               panes[]: { id, ref, index, focused, surface_count,
                                          selected_surface_id, surfaces[]: {
                                            id, ref, index, index_in_pane, type,
                                            focused, selected, selected_in_pane,
                                            pane_id, pane_ref, title } } } }

Linux uses 1:1 panel:pane today, so each pane has exactly one surface and
pane.id == surface.id. The shape leaves room for future pane grouping
(multiple surfaces per pane) without breaking clients.

The handler is alloc-free in the steady-state error path: every writer.* call
that fails returns the same canonical empty envelope so partial responses
never leak. Workspace and panel titles flow through writeJsonString to keep
user-supplied strings safe.

The new socket round-trip test creates an extra workspace, asserts the
envelope, restores the baseline workspace selection, and closes the
scratch workspace in a finally block so it never leaves stale state.

Sprint A item #5 from #220 (TIN-183 follow-up after #218 / #221 / #222).
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
#223)

The macOS build ships a system.tree RPC that flattens the entire UI hierarchy
into a single response so clients (CLIs, shell prompts, status bars) can avoid
N+1 round-trips. Linux currently exposes the leaf RPCs (window.list,
workspace.list, surface.list, pane.list) but not the composed view.

Add handleSystemTree mirroring the macOS shape:
  active: { workspace_id, surface_id, window_id }   // matches identify
  windows[]: { id, ref, index, workspace_count, selected_workspace_id,
               workspaces[]: { id, ref, index, title, selected, pinned,
                               panes[]: { id, ref, index, focused, surface_count,
                                          selected_surface_id, surfaces[]: {
                                            id, ref, index, index_in_pane, type,
                                            focused, selected, selected_in_pane,
                                            pane_id, pane_ref, title } } } }

Linux uses 1:1 panel:pane today, so each pane has exactly one surface and
pane.id == surface.id. The shape leaves room for future pane grouping
(multiple surfaces per pane) without breaking clients.

The handler is alloc-free in the steady-state error path: every writer.* call
that fails returns the same canonical empty envelope so partial responses
never leak. Workspace and panel titles flow through writeJsonString to keep
user-supplied strings safe.

The new socket round-trip test creates an extra workspace, asserts the
envelope, restores the baseline workspace selection, and closes the
scratch workspace in a finally block so it never leaves stale state.

Sprint A item #5 from #220 (TIN-183 follow-up after #218 / #221 / #222).
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
Add 11 new candidate tests enabled by Sprint A (PRs #218-#229) and
Sprint B (PR #230) socket handlers. These run non-fatally under
CMUX_TEST_PHASE1=1 to observe green runs before promoting to baseline.

New candidates: surface.action rename/close/new/reload, workspace.action,
auth.login, system.tree, notification.create_for_target, app.simulate_active,
surface.report_tty, pane.resize, sprint_b_core_parity.
Jesssullivan added a commit that referenced this pull request Apr 18, 2026
…230)

* linux(socket): Sprint B — close macOS parity gap (74 → 182 methods)

Core implementations:
- surface.send_key: validated key echo stub (ghostty_surface_key TBD)
- surface.ports_kick: accepts and echoes params (remote not on linux)
- workspace.equalize_splits: proportional ratio equalization via split
  tree walk, supports optional orientation filter
- debug.terminals: full terminal metadata dump (workspace, surface,
  focus, title, tty_name)

Batch stubs (reachable, return structured errors):
- 22 debug introspection methods (debug.layout, debug.sidebar.visible,
  debug.command_palette.*, etc.)
- 6 workspace.remote.* methods
- 76 browser automation methods (browser.click, browser.eval,
  browser.screenshot, etc.) — gated behind has_webkit

Dispatch table: 74 → 189 entries (macOS has 182).

* fix(socket): add 4 missing method stubs (settings, feedback, markdown)

Caught by audit agent: settings.open, feedback.open, feedback.submit,
markdown.open were in macOS dispatch but missing from Sprint B batch.

* fix(socket): replace std.fmt.formatInt with writer.print for Zig compat

std.fmt.formatInt is not available in all Zig 0.14.x builds (fails on
Arch, Fedora, Rocky, Ubuntu CI). Use w.print("{d}", .{val}) which is
the portable writer API.

* test(socket): expand Phase 1 candidate list with Sprint A+B tests

Add 11 new candidate tests enabled by Sprint A (PRs #218-#229) and
Sprint B (PR #230) socket handlers. These run non-fatally under
CMUX_TEST_PHASE1=1 to observe green runs before promoting to baseline.

New candidates: surface.action rename/close/new/reload, workspace.action,
auth.login, system.tree, notification.create_for_target, app.simulate_active,
surface.report_tty, pane.resize, sprint_b_core_parity.

* linux(socket): promote debug.layout, sidebar.visible, terminal.is_focused from stubs

debug.layout: serializes split tree to JSON {type, orientation, ratio,
panel_id} hierarchy — enables test introspection of pane geometry.

debug.sidebar.visible: returns {visible:true} (Linux sidebar is always
visible, not toggleable like macOS).

debug.terminal.is_focused: checks focused panel type, returns
{focused:bool}.

* linux(socket): wire debug.terminal.read_text to surface.read_text handler

Same operation, different dispatch path. Avoids test failures from
debug-prefixed callers hitting the empty stub.

This branch was successfully deployed

1 inactive deployment
gpu-tests — 01e3be26 Deployed Apr 18, 2026 by Jesssullivan via GPU smoke test (honey) #104
distro-tests — 01e3be26 Deployed Apr 18, 2026 by Jesssullivan via Distro package tests (self-hosted KVM) #57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant