From 46774b71f9bf40a522116ebbe81e636112b45b96 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sat, 24 Oct 2026 14:45:34 +0200 Subject: [PATCH] fix: address the release-prep review of #688 Give a client that fails the M-1..M-9 guard its key back (send-keys as the if-shell else argument), so a browser tab's Esc then digit reaches its pane instead of being eaten, also after a TUI killed before restore(); only M-1..M-9 are accepted since the key is now interpolated into that command Test it: the unit test pins the else argument and the key check, and the real-tmux test runs cat -v in alpha and waits for the other client's Esc 2 to echo there, both before and after the TUI dies Say so in docs/architecture-invariants.md and docs/tui.md, and reword the killed-TUI and failed-arm notes that said the keys do nothing Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/architecture-invariants.md | 2 +- docs/tui.md | 3 ++- src/tui/tui-client.ts | 17 +++++++++++++++-- test/tui/tui-client.test.ts | 11 +++++++++-- test/tui/tui-switch-guard.test.ts | 15 ++++++++++++--- 5 files changed, 39 insertions(+), 9 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 184dce66..bbeb2aa2 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. 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()` (bind `C-q send-prefix`, then `unbind-key -a -T prefix`, then bind it again, so a user config that left the prefix table empty cannot make `unbind-key -a` abort the `\;` chain) 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. It is armed as the handoff's last tmux call before `spawnSync`, after the keys and the per-session bars, so only a TUI killed during the attach's own startup can leave that hook armed for its tty. 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`. +**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()` (bind `C-q send-prefix`, then `unbind-key -a -T prefix`, then bind it again, so a user config that left the prefix table empty cannot make `unbind-key -a` abort the `\;` chain) 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. It is armed as the handoff's last tmux call before `spawnSync`, after the keys and the per-session bars, so only a TUI killed during the attach's own startup can leave that hook armed for its tty. 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-`. A client that fails the guard gets the key back: the `if-shell` else argument is `send-keys M-`, which writes Esc and the digit into that client's own pane, so a vim `2j` in a browser tab is not eaten by the root binding. 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 never switches and every client gets it in its pane through the else branch. `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/tui.md b/docs/tui.md index b0562ae2..92244f11 100644 --- a/docs/tui.md +++ b/docs/tui.md @@ -195,7 +195,8 @@ That is the **session strip**: the other sessions stay visible from inside a pan 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. 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 +same socket cannot use them to jump sessions, Esc followed by a digit included (that +tab's pane still receives the Esc and the digit). 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/tui/tui-client.ts b/src/tui/tui-client.ts index fe0bbe34..42ce2261 100644 --- a/src/tui/tui-client.ts +++ b/src/tui/tui-client.ts @@ -459,6 +459,9 @@ export const TUI_SWITCH_CLIENT_PID = '@codeman-tui-pid'; */ export const TUI_SWITCH_ATTACH_HOOK = 'client-attached[90]'; +/** The keys `bindSwitchKey` binds. The key is interpolated into the binding's `send-keys` else command. */ +const TUI_SWITCH_KEY = /^M-[1-9]$/; + /** 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]+)$/; @@ -1049,6 +1052,14 @@ export class TuiClient { * the same `M-` inside `escape-time`, does not match. An unsafe tty * refuses the bind rather than falling back to an unscoped `switch-client`. * + * A client that fails the guard gets the key back: the if-shell else + * argument is `send-keys `, which writes it into that client's own pane + * exactly as an unbound key would arrive. Without it the root binding ate + * the key, so a browser tab's vim `2j` lost its Esc and its digit for + * the whole attach, and for good after a TUI killed before `restore()`. The + * key is interpolated into that command string, so only `M-1`..`M-9` are + * accepted. + * * 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 @@ -1058,7 +1069,7 @@ export class TuiClient { * that session follows this terminal. */ async bindSwitchKey(key: string, target: string, clientTty: string): Promise { - if (!MUX_NAME_PATTERN.test(target) || !isTuiClientTty(clientTty)) return false; + if (!TUI_SWITCH_KEY.test(key) || !MUX_NAME_PATTERN.test(target) || !isTuiClientTty(clientTty)) return false; try { await this.exec('tmux', [ '-L', @@ -1071,6 +1082,7 @@ export class TuiClient { '-F', tuiSwitchGuard(clientTty), `switch-client -t ${target} ; set-window-option -t ${target} window-size latest`, + `send-keys ${key}`, ]); return true; } catch { @@ -1081,7 +1093,8 @@ export class TuiClient { /** * 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. + * a failure means Alt+1..9 never switch (they reach the pane as Esc then + * the digit), which is the closed direction. */ async armTuiSwitchGuard(clientTty: string): Promise { if (!isTuiClientTty(clientTty)) return false; diff --git a/test/tui/tui-client.test.ts b/test/tui/tui-client.test.ts index 07fb1cb2..ce1c39d1 100644 --- a/test/tui/tui-client.test.ts +++ b/test/tui/tui-client.test.ts @@ -642,23 +642,30 @@ describe('TuiClient.bindSwitchKey', () => { expect(bind).toBeDefined(); expect(bind).not.toContain(';'); expect(bind).not.toContain('\\;'); - const command = bind?.at(-1) ?? ''; + const command = bind?.at(-2) ?? ''; expect(command).toContain( 'switch-client -t codeman-aaaa1111 ; set-window-option -t codeman-aaaa1111 window-size latest' ); + // The else branch: a client that fails the guard (a browser tab's Esc + // then a digit) gets the key in its own pane instead of losing it. + expect(bind?.at(-1)).toBe('send-keys M-2'); 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); + // The key is interpolated into the else command, so only M-1..M-9 bind. + expect(await client.bindSwitchKey('M-2 ; kill-server', 'codeman-aaaa1111', '/dev/pts/4')).toBe(false); + expect(await client.bindSwitchKey('M-0', 'codeman-aaaa1111', '/dev/pts/4')).toBe(false); + expect(calls.filter((args) => args.includes('bind-key'))).toHaveLength(1); }); }); }); 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"'; + '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" "send-keys M-2"'; it('recognises our binding and the legacy unscoped one, and leaves a user binding alone', () => { expect(isTuiSwitchBinding(guarded)).toBe(true); diff --git a/test/tui/tui-switch-guard.test.ts b/test/tui/tui-switch-guard.test.ts index 290febe0..917fd670 100644 --- a/test/tui/tui-switch-guard.test.ts +++ b/test/tui/tui-switch-guard.test.ts @@ -142,8 +142,15 @@ describe.skipIf(!hasTmux)('TUI M-1..M-9 switch only the attaching client', () => if (socketPath.endsWith(`/${socket}`)) rmSync(socketPath, { force: true }); }); + /** How many Esc-then-2 presses alpha's pane has echoed (the tty's ECHOCTL shows ESC as `^[`). */ + function escTwosInAlpha(): number { + return run(['capture-pane', '-p', '-t', alpha]).split('^[2').length - 1; + } + 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']); + // alpha echoes what reaches its pane, so a key another client presses can + // be seen arriving there instead of being eaten by the root binding. + run(['new-session', '-d', '-s', alpha, '-x', '80', '-y', '24', 'stty -ixon; cat -v']); run(['new-session', '-d', '-s', beta, '-x', '80', '-y', '24', 'sleep 300']); helper = spawn('python3', ['-c', PTY_HELPER, socket, alpha], { stdio: ['pipe', 'pipe', 'inherit'] }); @@ -207,8 +214,10 @@ describe.skipIf(!hasTmux)('TUI M-1..M-9 switch only the attaching client', () => expect(otherTty).toMatch(CLIENT_TTY); expect(otherTty).not.toBe(tty); + expect(escTwosInAlpha()).toBe(0); other.write('\x1b2'); - await new Promise((resolve) => setTimeout(resolve, 400)); + // Not moved, and not swallowed: the else branch hands the key to its pane. + expect(await waitFor(() => escTwosInAlpha() === 1)).toBe(true); expect(sessions().get(otherTty)?.session).toBe(alpha); expect(sessions().get(tty)?.session).toBe(beta); @@ -220,7 +229,7 @@ describe.skipIf(!hasTmux)('TUI M-1..M-9 switch only the attaching client', () => expect(run(['show-options', '-gv', '@codeman-tui-pid']).trim()).toBe(pid); other.write('\x1b2'); - await new Promise((resolve) => setTimeout(resolve, 400)); + expect(await waitFor(() => escTwosInAlpha() === 2)).toBe(true); expect(sessions().get(otherTty)?.session).toBe(alpha); // restore() drops the key because it still names this tty, and the dead pid.