From daeea54495f583da0bc4e7403aadfd342dd9e19f Mon Sep 17 00:00:00 2001 From: DDT <141296842+Hugo-DDT@users.noreply.github.com> Date: Fri, 23 Oct 2026 07:41:46 +0000 Subject: [PATCH] fix(tmux): stop nested prefix keys and TUI Alt+digits from moving other clients Docker and owned-remote tmux servers kept the stock prefix table, so Ctrl+Q then )/(/s/w/L switch-client'd the inner client onto a sibling. Those create chains now leave only C-q send-prefix. codeman tui bound M-1..M-9 in the server-wide root table. The keys stay, but switch-client runs only for the attaching client (tty plus a one-shot pid). A browser tab's Esc-then-digit no longer jumps sessions, and a killed TUI leaves a binding that matches no live client. Co-authored-by: DDT <1786035110@qq.com> --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- docs/remote-sessions.md | 9 +- docs/tui.md | 4 +- src/tmux-manager.ts | 23 +++- src/tmux-prefix-table.ts | 37 +++++- src/tui/tui-app.ts | 42 ++++-- src/tui/tui-client.ts | 209 +++++++++++++++++++++++++---- test/docker-exec-options.test.ts | 6 + test/remote-ssh-options.test.ts | 6 + test/tmux-prefix-table.test.ts | 87 +++++++++++- test/tui/tui-app.test.ts | 80 ++++++++++- test/tui/tui-client.test.ts | 69 ++++++++-- test/tui/tui-switch-guard.test.ts | 214 ++++++++++++++++++++++++++++++ 14 files changed, 728 insertions(+), 62 deletions(-) create mode 100644 test/tui/tui-switch-guard.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 02704d0f..23613879 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,7 +102,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph - **`xterm-zerolag-input` is single-source** — BOTH echo addons live ONLY in `packages/xterm-zerolag-input/src/`, bundled into TWO **gitignored** vendor files: `vendor/xterm-zerolag-input.js` (buffer overlay, entry `zerolag-input-addon.ts`) and `vendor/xterm-predictive-echo.js` (codex write-through, entry `predictive-echo-addon.ts`) — dev by `scripts/postinstall.js`, prod by `scripts/build.mjs`. `app.js`/terminal-ui.js only **consume** them via `new LocalEchoOverlay(terminal)` / `new PredictiveEchoOverlay(terminal)`; there is no inline copy. So: change the package source, then rerun the bundle step (`npm install` for dev, `npm run build` for prod). **Never hand-edit `app.js` for overlay behavior, and never commit the gitignored vendor bundles.** Always test on mobile after touching it. → [architecture-invariants#xterm-zerolag-input-is-single-source](docs/architecture-invariants.md#xterm-zerolag-input-is-single-source), `docs/local-echo-overlay-plan.md` - **Default bind is loopback-only; non-loopback without a password starts but warns** — the server defaults to `--host 127.0.0.1`. Binding non-loopback (`--host`/`-H`/`CODEMAN_HOST`) without `CODEMAN_PASSWORD` starts anyway but prints a loud warning; `--allow-unauthenticated-network` / `CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK=1` acknowledges it. ⚠️ The production systemd unit passes no `--host`, so prod binds **localhost only**: reach it via `tailscale serve`/tunnel to `127.0.0.1`. A loopback bind is reachable through a same-host tunnel but NOT by a browser hitting the box's LAN IP. `install.sh` is separate and prompts for the binding (defaulting to LAN + a password), and preserves the existing binding on re-runs. → [architecture-invariants#default-bind-and-the-non-loopback-warning-path](docs/architecture-invariants.md#default-bind-and-the-non-loopback-warning-path), `docs/security-architecture.md` - **Instance isolation / multi-instance attach danger** — the data dir (`~/.codeman`) and tmux socket (`tmux -L codeman`) are PROCESS-WIDE and shared by every Codeman on the machine, derived from `CODEMAN_INSTANCE` via `src/config/instance.ts`. ⚠️ A 2nd instance on the SAME socket **discovers and attaches PTYs to the first instance's live sessions**, resizing and mutating them. `$HOME` isolation is NOT enough because tmux is system-global. To run two instances, give each a distinct `CODEMAN_INSTANCE` (scopes dir + socket together), or set `CODEMAN_TMUX_SOCKET` + `CODEMAN_DATA_DIR` individually; `scripts/run-beta.sh` does this for a beta alongside prod. **Any new `~/.codeman/...` path MUST go through `dataPath()`**, never `join(homedir(), '.codeman', …)`, and **any new `tmux -L` caller through `resolveTmuxSocketName()`** (both in `config/instance.ts`): the TUI shells out to tmux from a second process, and a hardcoded `codeman` there would point a beta instance at prod's panes. → [architecture-invariants#instance-isolation-and-the-multi-instance-attach-danger](docs/architecture-invariants.md#instance-isolation-and-the-multi-instance-attach-danger) -- **The prefix table on Codeman's socket holds only ` send-prefix`** — browser tabs are tmux attach clients, and tmux's stock bindings let Ctrl+B `)`/`s` move a tab onto another session. `createSession` resets the table, and `reconcileSessions()` at boot for sessions that survived an upgrade (`src/tmux-prefix-table.ts`); `root` and `C- detach-client` (the TUI attach's way out) are left alone, so while a `codeman tui` attach is open its `root` keys reach browser tabs too, Alt+1..9 `switch-client` included (open follow-up). ⚠️ So `Ctrl+B d` does not exist there: anything attaching a user's terminal claims its own detach key (F1 in `root`, as `codeman tui` and `scripts/tmux-manager.sh` do). → [architecture-invariants#the-prefix-table-on-codemans-socket-holds-only-send-prefix](docs/architecture-invariants.md#the-prefix-table-on-codemans-socket-holds-only-send-prefix) +- **The prefix table on Codeman's socket holds only ` send-prefix`** — browser tabs are tmux attach clients, and tmux's stock bindings let Ctrl+B `)`/`s` move a tab onto another session. `createSession` resets the table, and `reconcileSessions()` at boot for sessions that survived an upgrade (`src/tmux-prefix-table.ts`); `root` and `C- detach-client` (the TUI attach's way out) are left alone, so F1 and that alias reach browser tabs for the length of an attach. Alt+1..9 are `if-shell` bindings that `switch-client` only for the TUI's own client (`client_tty` plus a one-shot pid hook), so a browser tab's Esc-then-digit does not move it. Docker and owned-remote nested servers get the same reduction (`C-q send-prefix` only) in their create chain. ⚠️ So `Ctrl+B d` does not exist there: anything attaching a user's terminal claims its own detach key (F1 in `root`, as `codeman tui` and `scripts/tmux-manager.sh` do). → [architecture-invariants#the-prefix-table-on-codemans-socket-holds-only-send-prefix](docs/architecture-invariants.md#the-prefix-table-on-codemans-socket-holds-only-send-prefix) - **node-pty's macOS `spawn-helper` ships without `+x`** (issues #6, #204): `node-pty@1.1.0` publishes `prebuilds/darwin-/spawn-helper` as mode 0644, and macOS launches every PTY through it, so a stock macOS install fails every session start with `Error: posix_spawnp failed.` **Linux can never reproduce it**: `spawn-helper` is an `OS=="mac"` gyp target and node-pty ships no Linux prebuild, so node-gyp always emits an executable helper there. ⚠️ The flip side of that: since Linux has no prebuild, `npm install` **needs a C/C++ toolchain there** (`make`, `g++`, `python3`), so `install.sh` checks for and installs one alongside Node/tmux/git — a stock Ubuntu 24 server has none and died inside node-gyp with `not found: make`. Do not drop that step. ⚠️ Look in **`prebuilds/-/`**, not just `build/Release/`, which does not exist on macOS. Repair is a chmod, never a mandatory rebuild (that would require Xcode CLI tools and deletes `prebuilds/` before compiling): `npm run fix:node-pty` chmods every helper then proves it by really opening a PTY. `spawnPtyWithHelperRepair()` (`utils/node-pty-repair.ts`) wraps every `pty.spawn()` in `session.ts` and self-heals a broken install on the first failure. → [architecture-invariants#node-ptys-macos-spawn-helper-must-be-executable](docs/architecture-invariants.md#node-ptys-macos-spawn-helper-must-be-executable) - **User-chosen paths are probed BOUNDED, never with `existsSync`/`statSync`** (#516): a linked case or a `workingDir` can sit on a network mount that stopped answering. A synchronous check there freezes the whole server, and an unbounded async `stat`/`lstat`/`readFile` holds one of libuv's threadpool workers (4 by default, shared with every `fs`, `dns.lookup` and `crypto` call) until the mount returns. On a request or spawn path use `probePath()`/`probePathKind()` (`utils/bounded-path-probe.ts`, read its `@fileoverview`). ⚠️ `unknown` is NEVER `absent`: nothing is created, scaffolded or 404'd on it. Bulk scans keep the stall cap; a request for ONE path the user named may pass `{ pastCap: true }`, which still stops at the ceiling that keeps one worker free; and a helper that would otherwise touch the path skips whatever is still `unknown`. User-facing errors for it go through `describeUnknownPath()`, so a refused probe is not reported as a broken folder. - **Headless screenshots: `deviceScaleFactor` MUST be 1, and write unique filenames** — under DSF=2 xterm's WebGL renderer draws glyphs at ~2× nominal size while still *reporting* nominal cell dims, so only the pixels reveal it and only the terminal font looks wrong. And overwriting a fixed output path leaves OS image viewers showing the old render, which reads as "the fix didn't work"; `scripts/capture-real-overview.mjs` mints a timestamped filename per run. Seed the per-device `localStorage` keys (`codeman:skin`, `codeman-font-size`, `codeman-app-settings`) so the capture matches a real device. → [architecture-invariants#headless-screenshot-capture](docs/architecture-invariants.md#headless-screenshot-capture) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index f446f4a1..1e5c234c 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -18,7 +18,7 @@ Implementation detail extracted from `CLAUDE.md` so that file stays small enough ### The prefix table on Codeman's socket holds only send-prefix -**Browser tabs are tmux clients, so tmux's prefix table must not reach `switch-client`** (`src/tmux-prefix-table.ts`, called from `createSession` before `respawn-pane`, and from `reconcileSessions()` at boot whenever the socket lists a pane, so sessions that survived an upgrade are covered). Every keystroke typed into a browser terminal reaches the `tmux attach-session` client behind it, and app.js deliberately lets Ctrl+B through to the terminal (readline, Claude Code's "ctrl+b ctrl+b"). With tmux's stock prefix table, Ctrl+B then `(` `)` `L` `s` `w` moved that client onto ANOTHER session, which the tab then showed and typed into under the original's label; `d` detached it, `$` renamed the session, `:` opened tmux's prompt. So the table is reduced to ` send-prefix` (the socket's configured prefix, read with `show-options -gv prefix`), once per tmux server in practice: the reset only runs while `list-keys -T prefix` shows anything else. ⚠️ Keep `send-prefix`: Claude Code inside tmux backgrounds a command with "ctrl+b ctrl+b". ⚠️ Never touch the `root` table, and tolerate (and re-bind after a reset) `C- detach-client`: `codeman tui`'s attach claims F1 and Alt+1..9 in `root` and the held-Ctrl alias `C-d` in `prefix` (`beginAttachHandoff`), and a session created mid-attach must not take that way out away. ⚠️ Key tables are server-wide, so what the TUI binds for its attach applies to browser tabs as well: F1 and `C-d` detach a tab's client, and **`M-1`..`M-9` (`switch-client -t `) move it onto another session**, the very thing this reset exists to stop. The browser keeps a typed Alt+digit out of the terminal (`terminal-ui.js` swallows it for its own tab switching), but Esc followed by a digit within tmux's `escape-time` (500 ms on tmux 3.4) arrives as the same `M-`. This lasts for the length of the attach, and indefinitely after a TUI that is killed before `restore()` runs. Not fixed here; scoping those bindings to the TUI's own client is the open follow-up. ⚠️ The consequence for every terminal attach: `Ctrl+B d` does not exist on this socket. Anything that attaches a user's terminal claims its own way out for the attach, as `beginAttachHandoff` and `scripts/tmux-manager.sh` (F1 in `root`) do, and never advertises tmux's stock chord. Tests: `test/tmux-prefix-table.test.ts` (real tmux on a private socket, driven through a pty attach client), the wiring in `test/tmux-manager.test.ts`, the script in `test/tmux-manager-script-attach.test.ts`. +**Browser tabs are tmux clients, so tmux's prefix table must not reach `switch-client`** (`src/tmux-prefix-table.ts`, called from `createSession` before `respawn-pane`, and from `reconcileSessions()` at boot whenever the socket lists a pane, so sessions that survived an upgrade are covered). Every keystroke typed into a browser terminal reaches the `tmux attach-session` client behind it, and app.js deliberately lets Ctrl+B through to the terminal (readline, Claude Code's "ctrl+b ctrl+b"). With tmux's stock prefix table, Ctrl+B then `(` `)` `L` `s` `w` moved that client onto ANOTHER session, which the tab then showed and typed into under the original's label; `d` detached it, `$` renamed the session, `:` opened tmux's prompt. So the table is reduced to ` send-prefix` (the socket's configured prefix, read with `show-options -gv prefix`), once per tmux server in practice: the reset only runs while `list-keys -T prefix` shows anything else. The same reduction is applied when Codeman creates the nested tmux behind a Docker case (`-L codeman-docker`) or an owned remote session (`-L codeman-remote`). Those servers set `prefix C-q` and used to keep tmux's stock prefix table, so inside the pane Ctrl+Q then `)` `(` `s` `w` or `L` ran `switch-client` / `choose-tree` on the inner server and moved its client onto a sibling session. `nestedPrefixTableResetCommands()` (`unbind-key -a -T prefix`, then `bind-key -T prefix C-q send-prefix`) is appended to both create chains, before `remain-on-exit failed` on the remote chain so a remote tmux that rejects `failed` cannot drop it. Those sockets are dedicated to Codeman, so the server-global table is the right scope. A non-owned remote attach joins another Codeman's `-L codeman` and does not run it. ⚠️ Keep `send-prefix`: Claude Code inside tmux backgrounds a command with "ctrl+b ctrl+b". ⚠️ Never touch the `root` table, and tolerate (and re-bind after a reset) `C- detach-client`: `codeman tui`'s attach claims F1 and Alt+1..9 in `root` and the held-Ctrl alias `C-d` in `prefix` (`beginAttachHandoff`), and a session created mid-attach must not take that way out away. ⚠️ Key tables are server-wide. F1 and the held-Ctrl `C-d` still detach every client for the length of an attach (a way out, not a session move). **`M-1`..`M-9` do not move other clients.** `bindSwitchKey` binds them to `if-shell -F` (`tuiSwitchGuard`) so `switch-client` runs only when `#{client_tty}` is the TUI's terminal and `#{client_pid}` equals `@codeman-tui-pid`. A one-shot `client-attached[90]` hook (`tuiSwitchAttachHook`) sets that option as the TUI's client attaches, then removes itself. A browser tab does not match, including Esc followed by a digit within `escape-time` (500 ms on tmux 3.4), which arrives as the same `M-`. If the TUI is killed before `restore()`, the binding can remain, but the stored pid is the dead client and the hook will not re-arm a pty that reuses the number, so the key does nothing. `restore()` removes the keys only while they still name this tty, and the next attach that claims the strip releases any of these bindings first. A second TUI attach replaces the keys (one binding per key). Escape-time on Codeman's socket stays at tmux's default: lowering it would change Esc delivery for every pane, and the guard already refuses other clients. ⚠️ The consequence for every terminal attach: `Ctrl+B d` does not exist on this socket. Anything that attaches a user's terminal claims its own way out for the attach, as `beginAttachHandoff` and `scripts/tmux-manager.sh` (F1 in `root`) do, and never advertises tmux's stock chord. Tests: `test/tmux-prefix-table.test.ts` (real tmux on a private socket, driven through a pty attach client, including the nested `C-q` table), `test/tui/tui-switch-guard.test.ts` (the guarded Alt keys against a second client), the wiring in `test/tmux-manager.test.ts`, the script in `test/tmux-manager-script-attach.test.ts`. ### Reverse-proxy base path diff --git a/docs/remote-sessions.md b/docs/remote-sessions.md index 22045cb8..10fa793a 100644 --- a/docs/remote-sessions.md +++ b/docs/remote-sessions.md @@ -94,7 +94,8 @@ ssh -o BatchMode=yes -t user@host \ 'tmux -L codeman-remote new-session -A -s codeman-ssh- -c "cd && exec " \; \ set -t codeman-ssh- status off \; set -t codeman-ssh- mouse off \; \ set -t codeman-ssh- prefix C-q \; set -s escape-time 0 \; \ - set -t codeman-ssh- window-size latest' + set -t codeman-ssh- window-size latest \; \ + unbind-key -a -T prefix \; bind-key -T prefix C-q send-prefix' ``` Key points: @@ -108,7 +109,11 @@ Key points: - **`-L codeman-remote`** = a DEDICATED socket for sessions launched by remote Codemans, NOT the canonical `-L codeman` socket the remote host's own Codeman uses. Options are set per-session (`set -t`), never `-g`, so a shared remote - tmux server's other sessions are untouched (#145 hardening). Note the + tmux server's other sessions are untouched (#145 hardening). The prefix + *table* is server-global, so the create chain reduces it to `C-q send-prefix` + (`unbind-key -a -T prefix` then `bind-key`). Otherwise Ctrl+Q then `)` moves + the inner client onto a sibling session. A non-owned attach does not do this: + it joins the remote Codeman's own socket. Note the asymmetry: **discovery/attach (COD-105) target the canonical `-L codeman` socket** — they join sessions the remote's own Codeman manages, while owned durable launches live on `-L codeman-remote`. diff --git a/docs/tui.md b/docs/tui.md index 8abab15b..b0562ae2 100644 --- a/docs/tui.md +++ b/docs/tui.md @@ -193,7 +193,9 @@ wears a bar across the top: That is the **session strip**: the other sessions stay visible from inside a pane, numbered exactly as the dashboard numbers them, with the one you are in inverted. -`Alt+1`..`Alt+9` switch between them without going back to the dashboard first. With +`Alt+1`..`Alt+9` switch between them without going back to the dashboard first. Those keys +run `switch-client` only for this terminal's tmux client, so a browser tab on the +same socket cannot use them to jump sessions, Esc followed by a digit included. With more sessions than fit, the strip shows a window around the current one and marks each cut end with `…`; the way-out hint is measured first and always keeps its space. diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index 961d5683..48defcc6 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -128,7 +128,7 @@ import { import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; import { ensureDeepSeekStatusShim } from './deepseek-status-shim.js'; -import { restrictTmuxPrefixTable } from './tmux-prefix-table.js'; +import { NESTED_TMUX_PREFIX, nestedPrefixTableResetCommands, restrictTmuxPrefixTable } from './tmux-prefix-table.js'; /** How long a cached process snapshot stays usable. */ const PROC_SNAPSHOT_TTL_MS = 2000; @@ -1229,7 +1229,8 @@ export function remoteTmuxLocation(remote: SessionRemote, sessionId: string): { * ssh -o BatchMode=yes -t [] user@host \ * 'tmux -L codeman-remote new-session -A -s codeman-ssh- -c "cd && exec " \ * \; set -t codeman-ssh- status off \; set -t codeman-ssh- mouse off \ - * \; set -t codeman-ssh- prefix C-q \; set -s escape-time 0' + * \; set -t codeman-ssh- prefix C-q \; set -s escape-time 0 \; \ + * unbind-key -a -T prefix \; bind-key -T prefix C-q send-prefix' * * COD-107 — the connection options (`-p`, `-i`, `-J`, SOCKS `-o ProxyCommand`, * arbitrary `-o`) come from the shared `buildSshConnectionArgs(remote)`, so the @@ -1343,7 +1344,7 @@ export function buildRemoteLaunchCommand(options: { `tmux -L ${REMOTE_TMUX_SOCKET} new-session -A -s ${remoteName} -c ${shellescape(remote.remotePath)} ${shellescape(paneCommand)}`, `set -t ${remoteName} status off`, `set -t ${remoteName} mouse off`, - `set -t ${remoteName} prefix C-q`, + `set -t ${remoteName} prefix ${NESTED_TMUX_PREFIX}`, 'set -s escape-time 0', // COD-106 — shared/collaborative sessions: tmux defaults to sizing a window // to the SMALLEST attached client, so two Codemans at different viewports @@ -1352,6 +1353,13 @@ export function buildRemoteLaunchCommand(options: { // Per-session scoped (`set -t `, matching #145's hardening) so a shared // remote tmux server's other sessions keep their own sizing behavior. `set -t ${remoteName} window-size latest`, + // The prefix TABLE is server-global (unlike `prefix C-q`, which is per + // session). This socket is dedicated to Codeman's owned sessions, so the + // same reduction as the local socket applies: Ctrl+Q then )/(/s/w/L must + // not switch-client / choose-tree onto a sibling. BEFORE remain-on-exit, + // which stays last because an old remote tmux rejects `failed` and aborts + // the rest of the chain. + ...nestedPrefixTableResetCommands(), // #210: keep a CRASHED pane so the failure is still on screen. Without this, // tmux destroys the pane -> window -> session (and, being the only session, // the whole remote server) the instant the pane command exits, which tears the @@ -1368,7 +1376,8 @@ export function buildRemoteLaunchCommand(options: { // LAST in the chain on purpose: tmux aborts the remaining commands of a `\;` // sequence once one errors (also verified), and `failed` needs tmux >= 3.2 on // the REMOTE host. Trailing, a rejection costs only this option; leading, it - // would silently drop status/mouse/prefix/escape-time/window-size with it. + // would silently drop status/mouse/prefix/escape-time/window-size and the + // prefix-table reduction with it. `set -t ${remoteName} remain-on-exit failed`, ].join(' \\; '); @@ -1540,8 +1549,12 @@ export function buildDockerLaunchCommand(opts: DockerLaunchOptions): string { `new-session -A -s ${dkrName} -c ${workdir} ${shellescape(paneCommand)}`, `set -t ${dkrName} status off`, `set -t ${dkrName} mouse off`, - `set -t ${dkrName} prefix C-q`, + `set -t ${dkrName} prefix ${NESTED_TMUX_PREFIX}`, 'set -s escape-time 0', + // Same reduction as the owned-remote chain: the in-container server is + // Codeman's (`-L codeman-docker`), and its stock prefix table would move + // the inner client to a sibling session on Ctrl+Q then )/(/s/w/L. + ...nestedPrefixTableResetCommands(), ].join(' \\; '); const execEnvFlags: string[] = []; diff --git a/src/tmux-prefix-table.ts b/src/tmux-prefix-table.ts index 4e37716c..60874300 100644 --- a/src/tmux-prefix-table.ts +++ b/src/tmux-prefix-table.ts @@ -27,11 +27,18 @@ * per tmux server in practice. * * That tolerance is server-wide like every key table: while such an alias is - * bound, a browser tab's client can detach itself with it too. The same goes - * for the `root` keys an attach claims, including Alt+1..9, which run - * `switch-client`: a tab that receives Esc then a digit within tmux's - * `escape-time` is moved onto another session for as long as those bindings - * exist. Leaving `root` alone keeps the attach working; it does not close that. + * bound, a browser tab's client can detach itself with it too. F1 in `root` + * is the same kind of way out. Alt+1..9 are not: `codeman tui` binds those + * through `if-shell -F` so `switch-client` runs only for the TUI's own client + * (see `bindSwitchKey` in src/tui/tui-client.ts). This file still does not + * touch `root`. + * + * Docker cases and owned remote sessions run a second tmux (`-L codeman-docker`, + * `-L codeman-remote`) with `prefix C-q`. That server is not this socket, so + * the reset above never sees it, and its stock prefix table still switches + * the inner client. `nestedPrefixTableResetCommands` is the same reduction, + * appended to those create chains. A non-owned remote attach joins another + * Codeman's `-L codeman` and must not run it. * * @module tmux-prefix-table */ @@ -43,6 +50,26 @@ import { shellescape } from './remote-hosts.js'; /** tmux's stock prefix, used when `show-options` cannot be read. */ const DEFAULT_PREFIX = 'C-b'; +/** + * Prefix on the dedicated tmux inside a Docker case and an owned remote + * session. Codeman's own socket keeps the user's prefix; these nested servers + * always use C-q so it does not collide with the outer prefix. + */ +export const NESTED_TMUX_PREFIX = 'C-q'; + +/** + * Commands that reduce a nested tmux's prefix table to `C-q send-prefix`. + * + * Joined with `\;` onto the `new-session` that creates the server + * (`buildRemoteLaunchCommand`, `buildDockerLaunchCommand`). No `tmux` binary + * and no `-g`: the table is server-global, which is what we want on these + * dedicated sockets and nowhere else. Put them BEFORE `remain-on-exit failed` + * so a remote tmux that rejects `failed` does not drop the reduction. + */ +export function nestedPrefixTableResetCommands(): readonly [string, string] { + return ['unbind-key -a -T prefix', `bind-key -T prefix ${NESTED_TMUX_PREFIX} send-prefix`]; +} + /** One `list-keys -T prefix` line: key and command. */ const PREFIX_LINE = /^bind-key\s+(?:-\S+\s+)*?-T\s+prefix\s+(\S+)\s+(.+)$/; diff --git a/src/tui/tui-app.ts b/src/tui/tui-app.ts index cc837aee..34493802 100644 --- a/src/tui/tui-app.ts +++ b/src/tui/tui-app.ts @@ -56,6 +56,8 @@ import { composerScroll, composerStep, composerText, createComposer, type TuiCom import { formatAwayDigest } from './tui-digest.js'; import { ATTACH_BANNER_MARKER, + isTuiClientTty, + readControllingTty, TuiClient, type TuiSessionOptions, type TuiWindowSizing, @@ -550,7 +552,14 @@ export async function beginAttachHandoff( label: string, tabs: readonly TuiAttachTab[] = [], cols?: number, - rows?: number + rows?: number, + /** + * Tty the attach client will inherit. Omit to read this process's terminal. + * `null` means there is no usable tty: switch keys are not bound (an unscoped + * `switch-client` would move every client on the socket, including a browser + * tab). Tests pass a tty so they do not depend on stdin. + */ + clientTty?: string | null ): Promise { const prefix = (await client.readPrefixKey(muxName)) ?? undefined; // Read, not assumed: see detachChord() for the `d` vs `D` mix-up this closes. @@ -584,13 +593,29 @@ export async function beginAttachHandoff( // usable rather than decorative. Only the first nine, only sessions that // really have a pane, and only keys tmux reports as free — the same rule the // way-out key follows, so a binding of the user's own is never shadowed. + // + // The key table is server-global. The command is guarded to this terminal's + // client (see bindSwitchKey): without a tty we bind nothing, and a leaked + // unscoped `switch-client` from an older TUI is removed so it cannot keep + // moving browser tabs. When we do claim the keys, drop whatever previous + // attach left on M-1..M-9 first — including keys this strip will not reuse. const switchKeys: string[] = []; - for (const tab of tabs.slice(0, 9)) { - if (!tab.muxName) continue; - const key = `M-${tab.index}`; - const bound = await client.readPrefixBinding(key, 'root'); - if (bound !== null && !bound.startsWith('switch-client')) continue; - if (await client.bindSwitchKey(key, tab.muxName)) switchKeys.push(key); + const tty = clientTty === undefined ? readControllingTty() : clientTty || null; + const switchTabs = tabs.slice(0, 9).filter((tab) => tab.index >= 1 && tab.index <= 9 && tab.muxName); + const guarded = Boolean(tty && isTuiClientTty(tty)); + if (!guarded) { + for (let index = 1; index <= 9; index++) await client.unbindSwitchKey(`M-${index}`, ''); + } else if (switchTabs.length > 0 && (await client.armTuiSwitchGuard(tty as string))) { + for (let index = 1; index <= 9; index++) await client.unbindSwitchKey(`M-${index}`); + for (const tab of switchTabs) { + const key = `M-${tab.index}`; + const bound = await client.readPrefixBinding(key, 'root'); + // Takeover just cleared ours. Anything still here belongs to the user. + if (bound !== null) continue; + if (await client.bindSwitchKey(key, tab.muxName as string, tty as string)) switchKeys.push(key); + } + } else { + for (let index = 1; index <= 9; index++) await client.unbindSwitchKey(`M-${index}`, ''); } const bannerFor = (activeMux: string, ownLabel: string): Record => buildAttachBanner({ @@ -638,7 +663,8 @@ export async function beginAttachHandoff( return { chord: oneKey ? ONE_KEY_DETACH : fallbackDetachChord(prefix, detachKey, claimed && alias ? alias : undefined), async restore(): Promise { - for (const key of switchKeys) await client.unbindSwitchKey(key); + for (const key of switchKeys) await client.unbindSwitchKey(key, tty ?? ''); + if (tty && isTuiClientTty(tty)) await client.disarmTuiSwitchGuard(tty); if (oneKey) await client.unbindDetachKey(ONE_KEY_DETACH, 'root'); if (claimed && alias) await client.unbindDetachKey(alias); // Options first, then the size: dropping the status bar gives its row diff --git a/src/tui/tui-client.ts b/src/tui/tui-client.ts index c4f035af..735ec945 100644 --- a/src/tui/tui-client.ts +++ b/src/tui/tui-client.ts @@ -41,7 +41,7 @@ * @module tui/tui-client */ -import { execFile as execFileCb } from 'node:child_process'; +import { execFile as execFileCb, execFileSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import http from 'node:http'; import https from 'node:https'; @@ -445,6 +445,86 @@ export function parsePrefixBinding(stdout: string, key: string, table: TuiKeyTab return null; } +/** + * User option holding the tmux client pid of the TUI attach that owns M-1..M-9. + * Set by `TUI_SWITCH_ATTACH_HOOK` when that client attaches, read by the + * `if-shell -F` guard on those keys. A dead pid matches no live client. + */ +export const TUI_SWITCH_CLIENT_PID = '@codeman-tui-pid'; + +/** + * Indexed so it does not replace a user's own `client-attached` hook. High + * index to stay clear of `[0]`. One-shot: the hook unsets itself after it + * records the pid, so a later client that reuses the pty does not re-arm it. + */ +export const TUI_SWITCH_ATTACH_HOOK = 'client-attached[90]'; + +/** Linux `/dev/pts/N` and macOS `/dev/ttysNNN`. Anything else is not a client tty we will interpolate into a format. */ +const TUI_CLIENT_TTY = /^\/dev\/(?:pts\/[0-9]+|ttys[0-9]+)$/; + +export function isTuiClientTty(value: string): boolean { + return TUI_CLIENT_TTY.test(value); +} + +/** + * The terminal this process is attached to, when it is a safe tmux client tty. + * `codeman tui` inherits that tty into `tmux attach-session`, which is how the + * guard knows its own client before the client exists. A pipe (tests, no TTY) + * is null: callers then must not bind `switch-client` for every client. + */ +export function readControllingTty(): string | null { + try { + const out = execFileSync('tty', { encoding: 'utf8', stdio: ['inherit', 'pipe', 'ignore'] }).trim(); + return isTuiClientTty(out) ? out : null; + } catch { + return null; + } +} + +/** + * `if-shell -F` guard for a TUI switch key. True only for the client whose tty + * is `tty` AND whose pid is the one the attach hook stored. Evaluated when the + * key is pressed, against the client that pressed it. + */ +export function tuiSwitchGuard(tty: string): string { + return `#{&&:#{==:#{client_tty},${tty}},#{==:#{client_pid},#{${TUI_SWITCH_CLIENT_PID}}}}`; +} + +/** + * `set-hook` command for `TUI_SWITCH_ATTACH_HOOK`. `\#{client_pid}` is deferred + * so tmux expands it when the hook fires, not when the hook is stored (a bare + * `#{client_pid}` is expanded at `set-hook` time, in no client, and stored empty). + */ +export function tuiSwitchAttachHook(tty: string): string { + return ( + `if-shell -F "#{==:#{client_tty},${tty}}" ` + + `"set -Fg ${TUI_SWITCH_CLIENT_PID} \\"\\#{client_pid}\\"; set-hook -gu ${TUI_SWITCH_ATTACH_HOOK}"` + ); +} + +const LEGACY_TUI_SWITCH = /^switch-client -t (\S+?)(?: \\; set-window-option -t \1 window-size latest)?$/; + +/** + * A root binding this TUI installed: the guarded `if-shell` form, or the + * pre-guard `switch-client -t ` form a killed TUI can leave behind. + * A user's own `switch-client -n` or a target that is not a Codeman session + * is not ours. + */ +export function isTuiSwitchBinding(command: string): boolean { + const text = command.trim(); + if (text.includes(TUI_SWITCH_CLIENT_PID) && text.includes('#{client_tty}') && text.includes('switch-client -t ')) { + return true; + } + const legacy = LEGACY_TUI_SWITCH.exec(text); + return legacy !== null && MUX_NAME_PATTERN.test(legacy[1] ?? ''); +} + +/** The tty embedded in a guarded switch binding, or null for a legacy one. */ +export function tuiSwitchBindingTty(command: string): string | null { + const match = /#\{==:#\{client_tty\},(\/dev\/(?:pts\/[0-9]+|ttys[0-9]+))\}/.exec(command); + return match?.[1] ?? null; +} + /** * The phrase the attach status bar carries. It doubles as the marker that tells * OUR bar apart from a user's own when sweeping one that leaked, so @@ -959,21 +1039,25 @@ export class TuiClient { * `switch-client` rather than detach-then-attach: it keeps the terminal, so * the move is instant and the TUI stays blocked in its `spawnSync` exactly as * before, still holding the restore it owes. + * + * The binding is NOT server-wide in effect. Key tables are, so the key is + * bound for every client, but the command is `if-shell -F` and runs only + * when the client that pressed it is this TUI (`clientTty` plus the pid the + * attach hook stored). A browser tab's Esc-then-digit, which tmux reads as + * the same `M-` inside `escape-time`, does not match. An unsafe tty + * refuses the bind rather than falling back to an unscoped `switch-client`. + * + * The two commands live in the if-shell argument, where `;` is a separator + * inside that string. A bare `;` as its own argv element would end the + * `bind-key` and run `window-size latest` immediately, which is the bug the + * old `\;` argument existed to avoid. The second command is `window-size + * latest`: the window is already the right size on arrival (see + * presizeWindow), so it changes nothing then, and a resize while sitting in + * that session follows this terminal. */ - async bindSwitchKey(key: string, target: string): Promise { - if (!MUX_NAME_PATTERN.test(target)) return false; + async bindSwitchKey(key: string, target: string, clientTty: string): Promise { + if (!MUX_NAME_PATTERN.test(target) || !isTuiClientTty(clientTty)) return false; try { - // Two commands: go there, then let that window follow this terminal again. - // It is already the right size (see presizeWindow), so `latest` changes - // nothing on arrival and costs no repaint — it matters only if the - // terminal is resized while sitting in that session. - // - // ⚠️ The separator MUST be an escaped `\;`. A bare `;` is a command - // separator to tmux's own parser, so it ends the `bind-key` and RUNS - // what follows immediately: the binding kept only `switch-client`, and - // every session got `window-size latest` executed on it at attach time. - // That is what left sessions on `latest` after a detach, since the - // snapshot was then taken from already-corrupted state. await this.exec('tmux', [ '-L', this.socket, @@ -981,15 +1065,10 @@ export class TuiClient { '-T', 'root', key, - 'switch-client', - '-t', - target, - '\\;', - 'set-window-option', - '-t', - target, - 'window-size', - 'latest', + 'if-shell', + '-F', + tuiSwitchGuard(clientTty), + `switch-client -t ${target} ; set-window-option -t ${target} window-size latest`, ]); return true; } catch { @@ -997,10 +1076,88 @@ export class TuiClient { } } - /** Give back a switch key, but only while it still points at a session. */ - async unbindSwitchKey(key: string): Promise { + /** + * Record this TUI's client pid when it attaches, then drop the hook so a + * later client on a reused pty does not take the binding over. Best effort: + * a failure means Alt+1..9 do nothing, which is the closed direction. + */ + async armTuiSwitchGuard(clientTty: string): Promise { + if (!isTuiClientTty(clientTty)) return false; + try { + await this.exec('tmux', [ + '-L', + this.socket, + 'set-hook', + '-g', + TUI_SWITCH_ATTACH_HOOK, + tuiSwitchAttachHook(clientTty), + ]); + return true; + } catch { + return false; + } + } + + /** + * Undo `armTuiSwitchGuard` for this tty. The hook is removed only while it + * still names `clientTty` (a second TUI may have replaced it). The pid option + * is removed only when no live client still has it, so a second attach that + * already recorded its own pid is left alone. + */ + async disarmTuiSwitchGuard(clientTty: string): Promise { + if (!isTuiClientTty(clientTty)) return; + try { + const { stdout } = await this.exec('tmux', ['-L', this.socket, 'show-hooks', '-g']); + const hook = stdout.split('\n').find((line) => line.startsWith(`${TUI_SWITCH_ATTACH_HOOK} `)); + if (hook?.includes(clientTty)) { + await this.exec('tmux', ['-L', this.socket, 'set-hook', '-gu', TUI_SWITCH_ATTACH_HOOK]); + } + } catch { + /* no server, or the hook is already gone */ + } + let pid = ''; + try { + const { stdout } = await this.exec('tmux', ['-L', this.socket, 'show-options', '-gv', TUI_SWITCH_CLIENT_PID]); + pid = stdout.trim(); + } catch { + return; + } + if (!pid) return; + let live = ''; + try { + ({ stdout: live } = await this.exec('tmux', ['-L', this.socket, 'list-clients', '-F', '#{client_pid}'])); + } catch { + live = ''; + } + const pids = new Set( + live + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0) + ); + if (pids.has(pid)) return; + try { + await this.exec('tmux', ['-L', this.socket, 'set-option', '-gu', TUI_SWITCH_CLIENT_PID]); + } catch { + /* already unset */ + } + } + + /** + * Give back a switch key, but only while it is still one of ours. + * + * `ownerTty` set: keep a binding that names a different tty, so this attach's + * restore does not take a second TUI's keys. `ownerTty` omitted: any of our + * bindings on the key goes (the attaching TUI is taking the table over). + * Pass `''` to drop only a legacy unscoped binding. + */ + async unbindSwitchKey(key: string, ownerTty?: string): Promise { const bound = await this.readPrefixBinding(key, 'root'); - if (!bound || !bound.startsWith('switch-client')) return; + if (!bound || !isTuiSwitchBinding(bound)) return; + if (ownerTty !== undefined) { + const boundTty = tuiSwitchBindingTty(bound); + if (boundTty !== null && boundTty !== ownerTty) return; + } try { await this.exec('tmux', ['-L', this.socket, 'unbind-key', '-T', 'root', key]); } catch { diff --git a/test/docker-exec-options.test.ts b/test/docker-exec-options.test.ts index ad1c061d..247c19c4 100644 --- a/test/docker-exec-options.test.ts +++ b/test/docker-exec-options.test.ts @@ -106,6 +106,12 @@ describe('buildDockerLaunchCommand', () => { expect(cmd).toContain('tmux -L codeman-docker setenv -g CODEMAN_SESSION_ID'); expect(cmd).toContain('new-session -A -s codeman-dkr-1a2b3c4d'); expect(cmd).toContain("sh -lc '"); + // The in-container tmux keeps prefix C-q. Without this reduction its stock + // prefix table runs switch-client / choose-tree for the inner client. + const unbind = cmd.indexOf('unbind-key -a -T prefix'); + const bind = cmd.indexOf('bind-key -T prefix C-q send-prefix'); + expect(unbind).toBeGreaterThan(cmd.indexOf('prefix C-q')); + expect(bind).toBeGreaterThan(unbind); }); it('pins a deterministic conversation id with a reboot-surviving fallback (fresh launch)', () => { diff --git a/test/remote-ssh-options.test.ts b/test/remote-ssh-options.test.ts index cb565424..acddd56a 100644 --- a/test/remote-ssh-options.test.ts +++ b/test/remote-ssh-options.test.ts @@ -164,6 +164,12 @@ describe('COD-107 buildRemoteLaunchCommand — threads connection args', () => { 'set -s escape-time 0', // COD-106 — shared/collaborative sizing, per-session scoped (never -g). `set -t ${remoteName} window-size latest`, + // #671 — the nested server's prefix table is stock unless this chain + // reduces it. Ctrl+Q then ) would switch-client onto a sibling session. + // Before remain-on-exit: a remote tmux that rejects `failed` aborts the + // rest of the chain, and that option has to stay last. + 'unbind-key -a -T prefix', + 'bind-key -T prefix C-q send-prefix', // #210 — keep a CRASHED pane for diagnosis. `failed` (not `on`, which would // also strand a pane after a clean `exit`), and LAST because tmux aborts the // remaining commands of a `\;` chain on error and `failed` needs tmux >= 3.2. diff --git a/test/tmux-prefix-table.test.ts b/test/tmux-prefix-table.test.ts index 0219c60f..f1d7a82a 100644 --- a/test/tmux-prefix-table.test.ts +++ b/test/tmux-prefix-table.test.ts @@ -15,7 +15,11 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { execSync } from 'node:child_process'; import { rmSync } from 'node:fs'; import * as pty from 'node-pty'; -import { prefixTableNeedsReset, restrictTmuxPrefixTable } from '../src/tmux-prefix-table.js'; +import { + nestedPrefixTableResetCommands, + prefixTableNeedsReset, + restrictTmuxPrefixTable, +} from '../src/tmux-prefix-table.js'; /** A trimmed copy of tmux 3.4's stock prefix table, as `list-keys -T prefix` prints it. */ const STOCK = [ @@ -278,3 +282,84 @@ describe.skipIf(!hasTmux)('restrictTmuxPrefixTable against a real tmux (private } }); }); + +describe.skipIf(!hasTmux)('nested Docker/remote tmux prefix table (private socket)', () => { + // What buildRemoteLaunchCommand / buildDockerLaunchCommand create: prefix C-q + // on a dedicated server, then the two commands nestedPrefixTableResetCommands + // emits. The inner client is the docker-exec / ssh process; Ctrl+Q is its prefix. + const socket = `codeman-nested-prefix-${process.pid}`; + const tmux = `tmux -L ${socket} -f /dev/null`; + const run = (cmd: string): string => execSync(cmd, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + let client: pty.IPty | null = null; + + const clientSession = () => run(`${tmux} list-clients -F '#{client_session}'`).trim(); + + async function waitFor(check: () => boolean, ms = 3000): Promise { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (check()) return true; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + return check(); + } + + beforeAll(async () => { + // `stty -ixon` in the pane so a delivered C-q is visible to cat -v as ^Q + // instead of being eaten as XON. The attach client below is what must also + // deliver C-q; node-pty opens the pty raw, which clears IXON on that side. + run(`${tmux} new-session -d -s alpha -x 80 -y 24 'stty -ixon; cat -v'`); + run(`${tmux} new-session -d -s beta -x 80 -y 24 'stty -ixon; cat -v'`); + run(`${tmux} set-option -t alpha prefix C-q`); + run(`${tmux} set-option -t beta prefix C-q`); + const env = { ...process.env } as Record; + delete env.TMUX; + client = pty.spawn('tmux', ['-L', socket, '-f', '/dev/null', 'attach-session', '-t', 'alpha'], { + name: 'xterm-256color', + cols: 80, + rows: 24, + env, + }); + expect(await waitFor(() => clientSession() === 'alpha')).toBe(true); + }); + + afterAll(() => { + client?.kill(); + let socketPath = ''; + try { + socketPath = run(`${tmux} display-message -p '#{socket_path}'`).trim(); + run(`${tmux} kill-server`); + } catch { + /* already gone */ + } + if (socketPath.endsWith(`/${socket}`)) rmSync(socketPath, { force: true }); + }); + + it('with the stock table, Ctrl+Q ) moves the inner client onto the other session', async () => { + client!.write('\x11)'); + expect(await waitFor(() => clientSession() === 'beta')).toBe(true); + const name = run(`${tmux} list-clients -F '#{client_name}'`).trim(); + run(`${tmux} switch-client -c ${name} -t alpha`); + expect(await waitFor(() => clientSession() === 'alpha')).toBe(true); + }); + + it('after the nested reset, Ctrl+Q ), s and w leave the client, and Ctrl+Q Ctrl+Q reaches the pane', async () => { + for (const command of nestedPrefixTableResetCommands()) run(`${tmux} ${command}`); + + const prefix = run(`${tmux} list-keys -T prefix`) + .split('\n') + .filter((line) => line.trim()); + expect(prefix).toEqual([expect.stringMatching(/-T prefix C-q\s+send-prefix$/)]); + + client!.write('\x11)'); + await new Promise((resolve) => setTimeout(resolve, 300)); + client!.write('\x11s'); + await new Promise((resolve) => setTimeout(resolve, 300)); + client!.write('\x11w'); + await new Promise((resolve) => setTimeout(resolve, 300)); + expect(clientSession()).toBe('alpha'); + expect(run(`${tmux} display-message -p -t alpha '#{pane_in_mode}'`).trim()).toBe('0'); + + client!.write('\x11\x11'); + expect(await waitFor(() => run(`${tmux} capture-pane -p -t alpha`).includes('^Q'))).toBe(true); + }); +}); diff --git a/test/tui/tui-app.test.ts b/test/tui/tui-app.test.ts index 309be727..ed54858c 100644 --- a/test/tui/tui-app.test.ts +++ b/test/tui/tui-app.test.ts @@ -626,12 +626,90 @@ describe('the way out when F1 is taken, on a socket with no detach key', () => { restoreWindowSizing: async () => true, } as unknown as TuiClient; - const handoff = await beginAttachHandoff(client, 'codeman-abc12345', 'w1-case'); + const handoff = await beginAttachHandoff(client, 'codeman-abc12345', 'w1-case', [], undefined, undefined, null); expect(handoff.chord).toBe('Ctrl+B then Ctrl+D'); }); }); +describe('attach switch keys stay on the TUI client', () => { + it('binds M-1 through the guard for the tty it was given, and does not bind without one', async () => { + const binds: Array<{ key: string; target: string; tty: string }> = []; + const unbinds: Array<{ key: string; owner: string | undefined }> = []; + let armed: string | null = null; + let disarmed: string | null = null; + const client = { + readPrefixKey: async () => 'C-b', + readDetachKey: async () => 'd', + readPrefixBinding: async () => null, + bindDetachKey: async () => true, + unbindDetachKey: async () => true, + armTuiSwitchGuard: async (tty: string) => { + armed = tty; + return true; + }, + disarmTuiSwitchGuard: async (tty: string) => { + disarmed = tty; + }, + bindSwitchKey: async (key: string, target: string, tty: string) => { + binds.push({ key, target, tty }); + return true; + }, + unbindSwitchKey: async (key: string, owner?: string) => { + unbinds.push({ key, owner }); + }, + readSessionOptions: async () => null, + applySessionOptions: async () => true, + restoreSessionOptions: async () => true, + readWindowSizing: async () => null, + presizeWindow: async () => true, + followAttachingClient: async () => true, + restoreWindowSizing: async () => true, + } as unknown as TuiClient; + + const tabs = [ + { index: 1, label: 'w1', active: true, muxName: 'codeman-aaaa1111' }, + { index: 2, label: 'w2', active: false, muxName: 'codeman-bbbb2222' }, + ]; + const handoff = await beginAttachHandoff( + client, + 'codeman-aaaa1111', + 'w1', + tabs, + undefined, + undefined, + '/dev/pts/4' + ); + expect(armed).toBe('/dev/pts/4'); + expect(binds).toEqual([ + { key: 'M-1', target: 'codeman-aaaa1111', tty: '/dev/pts/4' }, + { key: 'M-2', target: 'codeman-bbbb2222', tty: '/dev/pts/4' }, + ]); + // Takeover clears M-1..M-9 before claiming, so a killed TUI's leftovers go. + expect(unbinds.filter((entry) => entry.owner === undefined).map((entry) => entry.key)).toEqual([ + 'M-1', + 'M-2', + 'M-3', + 'M-4', + 'M-5', + 'M-6', + 'M-7', + 'M-8', + 'M-9', + ]); + + await handoff.restore(); + expect(disarmed).toBe('/dev/pts/4'); + expect(unbinds.filter((entry) => entry.owner === '/dev/pts/4').map((entry) => entry.key)).toEqual(['M-1', 'M-2']); + + binds.length = 0; + armed = null; + await beginAttachHandoff(client, 'codeman-aaaa1111', 'w1', tabs, undefined, undefined, null); + expect(armed).toBeNull(); + expect(binds).toEqual([]); + }); +}); + describe('naming a session the TUI starts', () => { it("follows the web UI's w- convention", () => { expect(nextSessionName('mirofish', [])).toBe('w1-mirofish'); diff --git a/test/tui/tui-client.test.ts b/test/tui/tui-client.test.ts index 8b78fdb8..9e0da27e 100644 --- a/test/tui/tui-client.test.ts +++ b/test/tui/tui-client.test.ts @@ -25,6 +25,11 @@ import { parseSessionOptions, parseDetachKey, parsePrefixBinding, + isTuiSwitchBinding, + tuiSwitchBindingTty, + tuiSwitchAttachHook, + tuiSwitchGuard, + isTuiClientTty, ATTACH_BANNER_MARKER, parseTmuxSessionList, parseWindowSizing, @@ -619,31 +624,73 @@ describe('attach window sizing', () => { }); describe('TuiClient.bindSwitchKey', () => { - it('escapes the command separator, or tmux runs the second command instead of binding it', () => { + it('guards switch-client to one client tty, and keeps both commands inside the if-shell string', () => { // ⚠️ A bare `;` argument is a command separator to tmux's OWN parser: it - // ends the bind-key and executes what follows immediately. That bound only - // `switch-client` and ran `window-size latest` against every session at - // attach time, which is why sessions were left on `latest` after a detach — - // the sizing snapshot was taken from already-corrupted state. Verified - // against real tmux both ways before this test was written. + // ends the bind-key and executes what follows immediately. The two commands + // therefore live in the single if-shell argument, where `;` separates them + // when the key fires rather than when the key is bound. An unscoped + // `switch-client` is refused: key tables are server-wide, and that binding + // would move a browser tab on Esc then a digit. const calls: string[][] = []; const exec: TuiExecFile = async (_file, args) => { calls.push([...args]); return { stdout: '', stderr: '' }; }; const client = new TuiClient({ baseUrl, socket: 'codeman-beta', exec }); - return client.bindSwitchKey('M-2', 'codeman-aaaa1111').then(() => { + return client.bindSwitchKey('M-2', 'codeman-aaaa1111', '/dev/pts/4').then(async () => { const bind = calls.find((args) => args.includes('bind-key')); expect(bind).toBeDefined(); - expect(bind).toContain('\\;'); expect(bind).not.toContain(';'); - // Both commands have to be in the ONE binding. - expect(bind?.join(' ')).toContain('switch-client -t codeman-aaaa1111'); - expect(bind?.join(' ')).toContain('window-size latest'); + expect(bind).not.toContain('\\;'); + const command = bind?.at(-1) ?? ''; + expect(command).toContain( + 'switch-client -t codeman-aaaa1111 ; set-window-option -t codeman-aaaa1111 window-size latest' + ); + expect(bind).toContain('if-shell'); + expect(bind).toContain('-F'); + expect(bind?.join(' ')).toContain('#{client_tty},/dev/pts/4'); + expect(bind?.join(' ')).toContain('#{@codeman-tui-pid}'); + expect(await client.bindSwitchKey('M-2', 'codeman-aaaa1111', '/tmp/not-a-tty')).toBe(false); + expect(await client.bindSwitchKey('M-2', 'other-session', '/dev/pts/4')).toBe(false); }); }); }); +describe('TUI switch-key guard', () => { + const guarded = + 'if-shell -F "#{&&:#{==:#{client_tty},/dev/pts/4},#{==:#{client_pid},#{@codeman-tui-pid}}}" "switch-client -t codeman-aaaa1111 ; set-window-option -t codeman-aaaa1111 window-size latest"'; + + it('recognises our binding and the legacy unscoped one, and leaves a user binding alone', () => { + expect(isTuiSwitchBinding(guarded)).toBe(true); + expect(tuiSwitchBindingTty(guarded)).toBe('/dev/pts/4'); + expect( + isTuiSwitchBinding( + 'switch-client -t codeman-aaaa1111 \\; set-window-option -t codeman-aaaa1111 window-size latest' + ) + ).toBe(true); + expect(isTuiSwitchBinding('switch-client -t codeman-aaaa1111')).toBe(true); + expect(tuiSwitchBindingTty('switch-client -t codeman-aaaa1111')).toBeNull(); + expect(isTuiSwitchBinding('switch-client -n')).toBe(false); + expect(isTuiSwitchBinding('switch-client -t my-session')).toBe(false); + expect(isTuiSwitchBinding('display-message mine')).toBe(false); + }); + + it('refuses a tty that is not safe to interpolate, and defers client_pid in the hook', () => { + expect(isTuiClientTty('/dev/pts/4')).toBe(true); + expect(isTuiClientTty('/dev/ttys003')).toBe(true); + expect(isTuiClientTty('/tmp/pty')).toBe(false); + expect(isTuiClientTty('/dev/pts/4; rm')).toBe(false); + expect(tuiSwitchGuard('/dev/pts/4')).toContain('#{client_tty},/dev/pts/4'); + expect(tuiSwitchGuard('/dev/pts/4')).toContain('#{@codeman-tui-pid}'); + // A bare #{client_pid} is expanded when the hook is stored, with no client + // attached, and the pid is then empty forever. The backslash defers it. + const hook = tuiSwitchAttachHook('/dev/pts/4'); + expect(hook).toContain('/dev/pts/4'); + expect(hook).toContain('\\#{client_pid}'); + expect(hook).toContain('set-hook -gu client-attached[90]'); + }); +}); + describe('parsePrefixBinding', () => { const REAL = [ 'bind-key -T prefix d detach-client', diff --git a/test/tui/tui-switch-guard.test.ts b/test/tui/tui-switch-guard.test.ts new file mode 100644 index 00000000..8cf62461 --- /dev/null +++ b/test/tui/tui-switch-guard.test.ts @@ -0,0 +1,214 @@ +/** + * @fileoverview Alt+1..9 on a `codeman tui` attach must not move any other tmux + * client. Key tables are server-wide, so the binding exists for every client; + * the command runs only for the TUI's own tty and pid. + * + * Against a real tmux on a private socket (never Codeman's). One client is + * attached on a pty whose tty is known BEFORE `tmux attach` runs, which is how + * the TUI arms the hook. A second client is what a browser tab is. + * + * Port: N/A (no server) + */ +import { afterAll, describe, expect, it } from 'vitest'; +import { execFileSync, execSync, spawn, type ChildProcess } from 'node:child_process'; +import { readlinkSync, rmSync } from 'node:fs'; +import { createInterface } from 'node:readline'; +import * as pty from 'node-pty'; +import { TuiClient } from '../../src/tui/tui-client.js'; + +const hasTmux = (() => { + try { + execSync('tmux -V', { stdio: 'ignore' }); + execSync('python3 -c "import pty"', { stdio: 'ignore' }); + return true; + } catch { + return false; + } +})(); + +/** + * Holds a pty open, prints its tty, and execs `tmux attach` on that slave only + * after stdin says GO. IXON is off so a byte is never swallowed as flow control. + */ +const PTY_HELPER = ` +import os, pty, sys, termios, signal +socket, session = sys.argv[1], sys.argv[2] +master, slave = pty.openpty() +attr = termios.tcgetattr(slave) +attr[0] &= ~(termios.IXON | termios.IXOFF) +termios.tcsetattr(slave, termios.TCSANOW, attr) +sys.stdout.write(os.ttyname(slave) + "\\n") +sys.stdout.flush() +if not sys.stdin.readline().startswith("GO"): + sys.exit(1) +env = os.environ.copy() +env["TERM"] = "xterm-256color" +env.pop("TMUX", None) +pid = os.fork() +if pid == 0: + os.setsid() + os.dup2(slave, 0) + os.dup2(slave, 1) + os.dup2(slave, 2) + os.close(master) + os.close(slave) + os.execvpe("tmux", ["tmux", "-f", "/dev/null", "-L", socket, "attach-session", "-t", session], env) +os.close(slave) +sys.stdout.write("ATTACHED\\n") +sys.stdout.flush() +while True: + line = sys.stdin.readline() + if not line or line.startswith("EXIT"): + break + if line.startswith("KEY "): + os.write(master, bytes.fromhex(line[4:].strip())) +os.kill(pid, signal.SIGTERM) +try: + os.waitpid(pid, 0) +except ChildProcessError: + pass +os.close(master) +`; + +describe.skipIf(!hasTmux)('TUI M-1..M-9 switch only the attaching client', () => { + const socket = `codeman-tui-switch-${process.pid}`; + const alpha = 'codeman-aaaa1111'; + const beta = 'codeman-bbbb2222'; + const run = (args: readonly string[]): string => + execFileSync('tmux', ['-L', socket, '-f', '/dev/null', ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + const client = new TuiClient({ socket }); + let helper: ChildProcess | null = null; + let other: pty.IPty | null = null; + const lines: string[] = []; + let waiters: Array<(line: string) => void> = []; + + function pushLine(line: string): void { + const waiter = waiters.shift(); + if (waiter) waiter(line); + else lines.push(line); + } + + function nextLine(): Promise { + const queued = lines.shift(); + if (queued !== undefined) return Promise.resolve(queued); + return new Promise((resolve) => waiters.push(resolve)); + } + + function sessions(): Map { + let out = ''; + try { + out = run(['list-clients', '-F', '#{client_tty}\t#{client_session}\t#{client_pid}']); + } catch { + return new Map(); + } + const map = new Map(); + for (const line of out.split('\n')) { + const [tty, session, pid] = line.trim().split('\t'); + if (tty && session) map.set(tty, { session, pid: pid ?? '' }); + } + return map; + } + + async function waitFor(check: () => boolean, ms = 4000): Promise { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (check()) return true; + await new Promise((resolve) => setTimeout(resolve, 40)); + } + return check(); + } + + afterAll(() => { + try { + helper?.stdin?.write('EXIT\n'); + } catch { + /* helper already gone */ + } + helper?.kill(); + other?.kill(); + let socketPath = ''; + try { + socketPath = run(['display-message', '-p', '#{socket_path}']).trim(); + run(['kill-server']); + } catch { + /* already gone */ + } + if (socketPath.endsWith(`/${socket}`)) rmSync(socketPath, { force: true }); + }); + + it('switches the armed client only, including after that client has died', async () => { + run(['new-session', '-d', '-s', alpha, '-x', '80', '-y', '24', 'sleep 300']); + run(['new-session', '-d', '-s', beta, '-x', '80', '-y', '24', 'sleep 300']); + + helper = spawn('python3', ['-c', PTY_HELPER, socket, alpha], { stdio: ['pipe', 'pipe', 'inherit'] }); + createInterface({ input: helper.stdout! }).on('line', pushLine); + const tty = (await nextLine()).trim(); + expect(tty).toMatch(/^\/dev\/pts\/[0-9]+$/); + + expect(await client.armTuiSwitchGuard(tty)).toBe(true); + const hooks = run(['show-hooks', '-g']); + expect(hooks).toContain('client-attached[90]'); + expect(hooks).toContain(tty); + // Expanded at set-hook time this is empty and the guard never matches. + expect(hooks).toContain('#{client_pid}'); + + helper.stdin!.write('GO\n'); + expect(await nextLine()).toBe('ATTACHED'); + expect(await waitFor(() => sessions().get(tty)?.session === alpha)).toBe(true); + + const pid = sessions().get(tty)?.pid ?? ''; + expect(pid).toMatch(/^[0-9]+$/); + expect(await waitFor(() => run(['show-options', '-gv', '@codeman-tui-pid']).trim() === pid)).toBe(true); + // One-shot: a later client on a reused pty is not recorded. + expect(run(['show-hooks', '-g'])).not.toContain('client-attached[90]'); + + expect(await client.bindSwitchKey('M-2', 'not-a-codeman-session', tty)).toBe(false); + expect(await client.bindSwitchKey('M-2', beta, '/tmp/not-a-tty')).toBe(false); + expect(await client.bindSwitchKey('M-2', beta, tty)).toBe(true); + const listed = run(['list-keys', '-T', 'root']); + expect(listed).toContain('if-shell -F'); + expect(listed).toContain(tty); + expect(listed).toContain(`switch-client -t ${beta}`); + + // Esc then 2, one write, inside escape-time: tmux reads it as M-2. + helper.stdin!.write('KEY 1b32\n'); + expect(await waitFor(() => sessions().get(tty)?.session === beta)).toBe(true); + + const env = { ...process.env } as Record; + delete env.TMUX; + other = pty.spawn('tmux', ['-L', socket, '-f', '/dev/null', 'attach-session', '-t', alpha], { + name: 'xterm-256color', + cols: 80, + rows: 24, + env, + }); + const otherTty = readlinkSync(`/proc/${other.pid}/fd/0`); + expect(otherTty).not.toBe(tty); + expect(await waitFor(() => sessions().get(otherTty)?.session === alpha)).toBe(true); + + other.write('\x1b2'); + await new Promise((resolve) => setTimeout(resolve, 400)); + expect(sessions().get(otherTty)?.session).toBe(alpha); + expect(sessions().get(tty)?.session).toBe(beta); + + // Killed before restore(): the binding remains, the pid is the dead client. + helper.stdin!.write('EXIT\n'); + helper = null; + expect(await waitFor(() => !sessions().has(tty))).toBe(true); + expect(run(['list-keys', '-T', 'root'])).toContain(`#{client_tty},${tty}`); + expect(run(['show-options', '-gv', '@codeman-tui-pid']).trim()).toBe(pid); + + other.write('\x1b2'); + await new Promise((resolve) => setTimeout(resolve, 400)); + expect(sessions().get(otherTty)?.session).toBe(alpha); + + // restore() drops the key because it still names this tty, and the dead pid. + await client.unbindSwitchKey('M-2', tty); + await client.disarmTuiSwitchGuard(tty); + expect(run(['list-keys', '-T', 'root'])).not.toContain(`switch-client -t ${beta}`); + expect(() => run(['show-options', '-gv', '@codeman-tui-pid'])).toThrow(); + }); +});