diff --git a/src/foreign-tmux.ts b/src/foreign-tmux.ts index cb54bc4c..013c140c 100644 --- a/src/foreign-tmux.ts +++ b/src/foreign-tmux.ts @@ -480,9 +480,22 @@ export interface ForeignAttachTarget { * The tmux invocation that joins a foreign session, as ONE line of POSIX sh. * * Shape and why: - * tmux -S new-session -t -s ; set destroy-unattached on ; set status off - * || exec tmux -S attach-session -r -t + * tmux -S has-session -t '=' || { echo gone; exit 1; } ; + * tmux -S kill-session -t '=' 2>/dev/null ; + * exec tmux -S new-session -t -s ; set destroy-unattached on ; set status off * + * - ⚠️ The `has-session` guard comes FIRST because `new-session -t` does not fail + * on a missing target, it invents one. Measured on tmux 3.4: with `work` gone it + * joined `work-prod`, with nothing matching it created a fresh shell, and on a + * socket with no server it STARTED one. This line re-runs every time the wrapper + * pane is respawned, and boot recovery does that for every dead pane, so once the + * owner exits their session the next Codeman restart would leave a new shell on + * their machine, in their container or on their ssh host. `=` is what makes the + * check exact (no prefix match), and `2>/dev/null` keeps tmux's own "no server" + * noise out of the pane — the message we print says it better. + * - ⚠️ The `=` prefix cannot go on `new-session -t` itself: tmux reads it as part of + * the name and creates a group literally called `=work`. It has to be a separate + * `has-session`. The read-only `attach-session` fallback takes `=` too. * - `new-session -t` creates a session in the target's GROUP: same windows, our * own session options and current window (NOT our own size — see @fileoverview * for the measurement that disproved that). @@ -491,23 +504,41 @@ export interface ForeignAttachTarget { * - No `-d`: the session must be attached before `destroy-unattached` lands on it. * - The escaped semicolons reach tmux as plain command separators after one shell * parse — the same convention the docker launch chain uses. - * - The `||` fallback is a READ-ONLY attach, for a tmux too old to group. It is - * read-only on purpose: a writable bare attach would resize the owner's - * terminal, which is the one outcome this design exists to prevent. The caller - * records it as `SessionAdopt.readOnly` so the UI can say so. + * - ⚠️ There is NO degraded fallback, on purpose. A read-only `attach-session -r` + * used to hang off `||` "for a tmux too old to group", but `||` fires on ANY + * `new-session` failure. The one that actually happens is a leftover view with + * the same name (the in-container leak described above): measured, `new-session` + * answers `duplicate session: codeman-view-…` and the fallback then handed the + * user a read-only client showing the owner's status bar, where everything typed + * is dropped while the API reports success. That is the worst shape a failure can + * take here. So: clear OUR OWN stale view first, then either group successfully + * or exit with a message. + * - The `kill-session` is safe by construction and cannot touch the human's work: + * the name is `codeman-view-`, `=` makes the match exact, and + * killing a session in a group removes only that session (measured: the owner's + * `work` survived its grouped view being killed). `2>/dev/null` because "no such + * session" is the normal case, not an error. * - Single line: the string crosses `bash -c "..."`, where a real newline would * not survive JSON escaping. */ export function buildForeignTmuxInvocation(target: ForeignAttachTarget): string { const sock = fshq(target.socketPath); const t = fshq(target.targetSession); + // `=name` is tmux's exact-match form. Only for the two commands that LOOK UP a + // session; never for `new-session -t`, which would name the group `=name`. + const exact = fshq(`=${target.targetSession}`); const v = fshq(target.viewSession); + const gone = fshq( + `Codeman: session ${target.targetSession} is gone. Codeman never creates a session it does not own.` + ); + const guard = `tmux -S ${sock} has-session -t ${exact} 2>/dev/null || { echo ${gone}; exit 1; }`; + // Our own leftover view from a previous adoption of the same Codeman session. + const dropStaleView = `tmux -S ${sock} kill-session -t ${fshq(`=${target.viewSession}`)} 2>/dev/null || true`; const group = - `tmux -S ${sock} new-session -t ${t} -s ${v} \\; ` + + `exec tmux -S ${sock} new-session -t ${t} -s ${v} \\; ` + `set-option -t ${v} destroy-unattached on \\; ` + `set-option -t ${v} status off`; - const readOnly = `exec tmux -S ${sock} attach-session -r -t ${t}`; - return `${group} || ${readOnly}`; + return `${guard} ; ${dropStaleView} ; ${group}`; } /** Local adoption: run the invocation directly in the wrapper pane. */ diff --git a/src/types/session.ts b/src/types/session.ts index b413578c..09d1b61b 100644 --- a/src/types/session.ts +++ b/src/types/session.ts @@ -424,12 +424,6 @@ export interface SessionAdopt { * Created with `destroy-unattached on` so it evaporates with our pane. */ viewSession: string; - /** - * True when the foreign tmux was too old to group and we fell back to a - * READ-ONLY attach. Surfaced in the UI — the fallback must never silently be - * a writable bare attach, which would resize the owner's terminal. - */ - readOnly?: boolean; /** The pane cwd at adoption time. Informational: an adopted pane is never `cd`'d. */ paneCurrentPath?: string; remote?: AdoptRemoteConnection; diff --git a/test/foreign-tmux.test.ts b/test/foreign-tmux.test.ts index b11216f8..377dee7b 100644 --- a/test/foreign-tmux.test.ts +++ b/test/foreign-tmux.test.ts @@ -223,6 +223,51 @@ describe('attach command builders', () => { expect(cmd).not.toMatch(/new-session\s+-d/); }); + it('checks the target exists BEFORE new-session, because new-session invents one', () => { + // ⚠️ `new-session -t` does not fail on a missing target. Measured on tmux 3.3a + // with only `work-prod` present: `new-session -t work` succeeded and joined our + // view into `work-prod`'s group — someone else's session. On a socket with no + // server it starts one. The wrapper pane re-runs this on every respawn, and boot + // recovery respawns every dead pane, so without this guard the owner exiting + // their session leaves Codeman creating shells on their machine. + const cmd = buildForeignTmuxInvocation(target); + const guard = cmd.indexOf('has-session'); + const create = cmd.indexOf('new-session'); + expect(guard).toBeGreaterThan(-1); + expect(guard).toBeLessThan(create); + expect(cmd).toMatch(/has-session -t '=work'/); + // Fails closed with a message rather than falling through to new-session. + expect(cmd).toMatch(/has-session[^;]*\|\|\s*\{\s*echo[^;]*;\s*exit 1;\s*\}/); + }); + + it('uses the exact-match = only where a session is looked UP, never on new-session -t', () => { + // Measured: tmux reads the `=` on `new-session -t` as part of the name and + // creates a group literally called `=work`, so the guard has to be separate. + // Without `=`, `has-session -t work` matches `work-prod` (verified on 3.3a). + const cmd = buildForeignTmuxInvocation(target); + expect(cmd).toContain("new-session -t 'work'"); + expect(cmd).not.toContain("new-session -t '=work'"); + // The two commands that LOOK a session up both take `=`. + expect(cmd).toContain("has-session -t '=work'"); + expect(cmd).toContain("kill-session -t '=codeman-view-abc12345'"); + }); + + it('keeps the guard on all three locations, not just the local one', () => { + // The guard lives in the one shared invocation builder, so docker and remote + // inherit it. A future split into per-location strings must keep that true: + // the ssh and container paths are exactly where a stray session costs most. + expect(buildForeignDockerAttachCommand({ ...target, dockerBase: 'docker', containerName: 'box' })).toContain( + 'has-session' + ); + expect( + buildForeignRemoteAttachCommand({ + ...target, + sshArgs: ['ssh', '-o BatchMode=yes'], + sshTarget: 'u@h', + }) + ).toContain('has-session'); + }); + it('sets destroy-unattached so the view dies with our pane', () => { expect(buildForeignTmuxInvocation(target)).toContain('destroy-unattached on'); }); @@ -234,11 +279,30 @@ describe('attach command builders', () => { expect(buildForeignTmuxInvocation(target)).not.toContain('window-size'); }); - it('falls back to a READ-ONLY attach, never a writable bare one', () => { - // A writable bare attach is precisely the thing that resizes the owner's - // terminal, so the degraded path must be the harmless one. + it('leaves NO degraded fallback — it either groups or fails with a message', () => { + // A read-only `attach-session -r` used to hang off `||`, nominally for a tmux + // too old to group. But `||` fires on ANY new-session failure, and the one that + // happens is a leftover view with the same name: measured, new-session answers + // `duplicate session: …` and the fallback then handed the user a read-only + // client where typed input is dropped while the API reported success. const cmd = buildForeignTmuxInvocation(target); - expect(cmd).toMatch(/\|\|\s*exec tmux -S .* attach-session -r -t/); + expect(cmd).not.toContain('attach-session'); + expect(cmd).not.toMatch(/new-session[^;]*\|\|/); + }); + + it('clears OUR OWN stale view before creating it, and only ours', () => { + // The view name is derived from Codeman's own session id, so a session with + // that name can only ever be ours. `=` keeps the match exact, and killing a + // session in a group removes only that session (measured: the owner's `work` + // survived its grouped view being killed). + const cmd = buildForeignTmuxInvocation(target); + expect(cmd).toContain("kill-session -t '=codeman-view-abc12345'"); + expect(cmd).not.toContain("kill-session -t '=work'"); + const kill = cmd.indexOf('kill-session'); + expect(kill).toBeGreaterThan(cmd.indexOf('has-session')); + expect(kill).toBeLessThan(cmd.indexOf('new-session')); + // A missing stale view is the normal case, never a reason to abort. + expect(cmd).toMatch(/kill-session[^;]*2>\/dev\/null \|\| true/); }); it('stays on ONE line — it crosses `bash -c "..."` where a newline dies', () => { @@ -258,8 +322,17 @@ describe('attach command builders', () => { viewSession: 'codeman-view-1', }); expect(nasty).toContain("'/tmp/a b'"); - // The metacharacters survive only INSIDE quotes, never as shell syntax. - expect(nasty).not.toMatch(/[^']ev;il/); + // The metacharacters survive only INSIDE quotes, never as shell syntax. Checked + // by stripping every single-quoted span and looking at what is left, rather than + // by what character precedes the name: the "session is gone" message quotes the + // name too, with a space in front of it, and a lookbehind assertion reads that + // as an escape when it is not one. + const outsideQuotes = nasty + .split("'") + .filter((_, i) => i % 2 === 0) + .join(' '); + expect(outsideQuotes).not.toContain('ev;il'); + expect(outsideQuotes).not.toContain('`'); }); describe('docker', () => {