From 311d01c52b6f3481c54b5be6f52897fb18608cba Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sat, 24 Oct 2026 14:10:07 +0200 Subject: [PATCH] fix: apply the review items for #688 at merge disarmTuiSwitchGuard matches the closed `#{client_tty},}` form, so a TUI on /dev/pts/4 no longer removes the hook armed for a second TUI on /dev/pts/41 (unit test plus a real-tmux case) beginAttachHandoff arms the one-shot attach hook as its last tmux call before spawnSync, after the switch keys and the per-session dressing loop, with an order assertion in tui-app.test.ts and a sentence in the invariants paragraph docs/remote-sessions.md and the buildRemoteLaunchCommand JSDoc show `remain-on-exit failed` at the end of the remote create chain Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/architecture-invariants.md | 2 +- docs/remote-sessions.md | 3 ++- src/tmux-manager.ts | 2 +- src/tui/tui-app.ts | 15 ++++++++++++--- src/tui/tui-client.ts | 5 ++++- test/tui/tui-app.test.ts | 18 ++++++++++++++++-- test/tui/tui-client.test.ts | 21 +++++++++++++++++++++ test/tui/tui-switch-guard.test.ts | 13 ++++++++++++- 8 files changed, 69 insertions(+), 10 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 63cdb9f8..184dce66 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. 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-`. 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 e22c83d5..2d80397b 100644 --- a/docs/remote-sessions.md +++ b/docs/remote-sessions.md @@ -96,7 +96,8 @@ ssh -o BatchMode=yes -t user@host \ set -t codeman-ssh- prefix C-q \; set -s escape-time 0 \; \ set -t codeman-ssh- window-size latest \; \ bind-key -T prefix C-q send-prefix \; unbind-key -a -T prefix \; \ - bind-key -T prefix C-q send-prefix' + bind-key -T prefix C-q send-prefix \; \ + set -t codeman-ssh- remain-on-exit failed' ``` Key points: diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index dde21ee2..a7ea8653 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -1231,7 +1231,7 @@ export function remoteTmuxLocation(remote: SessionRemote, sessionId: string): { * \; set -t codeman-ssh- status off \; set -t codeman-ssh- mouse off \ * \; set -t codeman-ssh- prefix C-q \; set -s escape-time 0 \; \ * bind-key -T prefix C-q send-prefix \; unbind-key -a -T prefix \; \ - * bind-key -T prefix C-q send-prefix' + * bind-key -T prefix C-q send-prefix \; set -t codeman-ssh- remain-on-exit failed' * * COD-107 — the connection options (`-p`, `-i`, `-J`, SOCKS `-o ProxyCommand`, * arbitrary `-o`) come from the shared `buildSshConnectionArgs(remote)`, so the diff --git a/src/tui/tui-app.ts b/src/tui/tui-app.ts index 34493802..538886a3 100644 --- a/src/tui/tui-app.ts +++ b/src/tui/tui-app.ts @@ -603,9 +603,9 @@ export async function beginAttachHandoff( 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))) { + // The keys are bound here but stay inert until the attach hook records this + // client's pid, and that hook is armed at the very end (see below). + if (guarded && switchTabs.length > 0) { for (let index = 1; index <= 9; index++) await client.unbindSwitchKey(`M-${index}`); for (const tab of switchTabs) { const key = `M-${tab.index}`; @@ -660,6 +660,15 @@ export async function beginAttachHandoff( if (cols && rows && rows > 1) await client.presizeWindow(target, cols, rows - 1); if (target === muxName) await client.followAttachingClient(target); } + // ⚠️ Armed LAST, as the final tmux call before the caller's `spawnSync`. The + // hook is one-shot for this tty, so a TUI killed between arming and attaching + // leaves it armed, and Linux hands the lowest free pty to the next opener: a + // browser tab's attach client could land on this tty and have its pid + // recorded, which would make these keys move that tab. Arming after the + // dressing loop above shrinks that window to the attach's own startup. A + // failed arm leaves the keys inert (the closed direction), and `restore()` + // still unbinds them. + if (switchKeys.length > 0) await client.armTuiSwitchGuard(tty as string); return { chord: oneKey ? ONE_KEY_DETACH : fallbackDetachChord(prefix, detachKey, claimed && alias ? alias : undefined), async restore(): Promise { diff --git a/src/tui/tui-client.ts b/src/tui/tui-client.ts index e4c72cb3..fe0bbe34 100644 --- a/src/tui/tui-client.ts +++ b/src/tui/tui-client.ts @@ -1111,7 +1111,10 @@ export class TuiClient { 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)) { + // The closed form, not the bare tty: `/dev/pts/4` is a substring of a + // hook armed for `/dev/pts/41`, and removing that one would leave the + // second TUI's Alt keys dead for its whole attach. + if (hook?.includes(`#{client_tty},${clientTty}}`)) { await this.exec('tmux', ['-L', this.socket, 'set-hook', '-gu', TUI_SWITCH_ATTACH_HOOK]); } } catch { diff --git a/test/tui/tui-app.test.ts b/test/tui/tui-app.test.ts index ed54858c..a9451818 100644 --- a/test/tui/tui-app.test.ts +++ b/test/tui/tui-app.test.ts @@ -638,6 +638,8 @@ describe('attach switch keys stay on the TUI client', () => { const unbinds: Array<{ key: string; owner: string | undefined }> = []; let armed: string | null = null; let disarmed: string | null = null; + // tmux calls in order, to pin where the attach hook is armed. + const order: string[] = []; const client = { readPrefixKey: async () => 'C-b', readDetachKey: async () => 'd', @@ -645,6 +647,7 @@ describe('attach switch keys stay on the TUI client', () => { bindDetachKey: async () => true, unbindDetachKey: async () => true, armTuiSwitchGuard: async (tty: string) => { + order.push('arm'); armed = tty; return true; }, @@ -652,6 +655,7 @@ describe('attach switch keys stay on the TUI client', () => { disarmed = tty; }, bindSwitchKey: async (key: string, target: string, tty: string) => { + order.push('bind'); binds.push({ key, target, tty }); return true; }, @@ -659,11 +663,17 @@ describe('attach switch keys stay on the TUI client', () => { unbinds.push({ key, owner }); }, readSessionOptions: async () => null, - applySessionOptions: async () => true, + applySessionOptions: async () => { + order.push('dress'); + return true; + }, restoreSessionOptions: async () => true, readWindowSizing: async () => null, presizeWindow: async () => true, - followAttachingClient: async () => true, + followAttachingClient: async () => { + order.push('follow'); + return true; + }, restoreWindowSizing: async () => true, } as unknown as TuiClient; @@ -681,6 +691,10 @@ describe('attach switch keys stay on the TUI client', () => { '/dev/pts/4' ); expect(armed).toBe('/dev/pts/4'); + // Armed once, as the LAST tmux call of the handoff: a TUI killed while the + // bars were being dressed must not leave a one-shot hook for its tty that a + // browser tab's attach on a reused pty would fire. + expect(order).toEqual(['bind', 'bind', 'dress', 'follow', 'dress', 'arm']); expect(binds).toEqual([ { key: 'M-1', target: 'codeman-aaaa1111', tty: '/dev/pts/4' }, { key: 'M-2', target: 'codeman-bbbb2222', tty: '/dev/pts/4' }, diff --git a/test/tui/tui-client.test.ts b/test/tui/tui-client.test.ts index a9858950..07fb1cb2 100644 --- a/test/tui/tui-client.test.ts +++ b/test/tui/tui-client.test.ts @@ -690,6 +690,27 @@ describe('TUI switch-key guard', () => { expect(hook).toContain('\\#{client_pid}'); expect(hook).toContain('set-hook -gu client-attached[90]'); }); + + it('disarms only the hook armed for its own tty, not one for a tty it is a prefix of', async () => { + // `/dev/pts/4` is a substring of `/dev/pts/41`: a bare includes() removed a + // second TUI's hook and left its Alt keys dead for that attach. + const disarm = async (armedFor: string, ownTty: string): Promise => { + const calls: string[][] = []; + const exec: TuiExecFile = async (_file, args) => { + calls.push([...args]); + if (args.includes('show-hooks')) { + return { stdout: `client-attached[90] ${tuiSwitchAttachHook(armedFor)}\n`, stderr: '' }; + } + return { stdout: '', stderr: '' }; + }; + await new TuiClient({ baseUrl, socket: 'codeman-beta', exec }).disarmTuiSwitchGuard(ownTty); + return calls.filter((args) => args.includes('set-hook')); + }; + expect(await disarm('/dev/pts/41', '/dev/pts/4')).toEqual([]); + expect(await disarm('/dev/pts/4', '/dev/pts/4')).toEqual([ + ['-L', 'codeman-beta', 'set-hook', '-gu', 'client-attached[90]'], + ]); + }); }); describe('parsePrefixBinding', () => { diff --git a/test/tui/tui-switch-guard.test.ts b/test/tui/tui-switch-guard.test.ts index 09a9c6d4..290febe0 100644 --- a/test/tui/tui-switch-guard.test.ts +++ b/test/tui/tui-switch-guard.test.ts @@ -154,7 +154,9 @@ describe.skipIf(!hasTmux)('TUI M-1..M-9 switch only the attaching client', () => expect(await client.armTuiSwitchGuard(tty)).toBe(true); const hooks = run(['show-hooks', '-g']); expect(hooks).toContain('client-attached[90]'); - expect(hooks).toContain(tty); + // The closed form, as stored: disarmTuiSwitchGuard matches on it, so a + // hook for /dev/pts/41 is not taken for /dev/pts/4's. + expect(hooks).toContain(`#{client_tty},${tty}}`); // The stored hook still names the format. Expanding it while the hook is // stored would save an empty pid (no client yet) and the guard would never match. expect(hooks).toContain('#{client_pid}'); @@ -226,5 +228,14 @@ describe.skipIf(!hasTmux)('TUI M-1..M-9 switch only the attaching client', () => 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(); + + // A hook armed for a tty this one is a prefix of (pts/4 vs pts/41) is + // another TUI's, and this tty's disarm must leave it. + const longer = `${tty}1`; + expect(await client.armTuiSwitchGuard(longer)).toBe(true); + await client.disarmTuiSwitchGuard(tty); + expect(run(['show-hooks', '-g'])).toContain('client-attached[90]'); + await client.disarmTuiSwitchGuard(longer); + expect(run(['show-hooks', '-g'])).not.toContain('client-attached[90]'); }); });