[Bugfix #1264] A clean harness exit reruns fresh instead of killing the session#1266
Conversation
CMAP review
Codex's finding — real, and worth the round trip
Correct. My clear-the-stamp rule was "clears on printable input, CR, or LF", and The mistake was reasoning about printability when the actual question is recency — a quit gesture doesn't have to be printable. FixThe rule collapses to its correct form: the stamp survives iff the most recent input byte is private noteInput(payload: Buffer): void {
if (payload.length === 0) return;
this.lastCtrlCAt = payload[payload.length - 1] === CTRL_C_BYTE ? Date.now() : null;
}Simpler and strictly more correct — the Added regression coverage for Re-verified end-to-end after the change (real SessionManager → real shellper → real On the split verdictTwo APPROVEs with "no issues" didn't outvote one specific, reproducible finding — codex was the only reviewer that traced the byte-level rule against a gesture the tests didn't cover, which is the same class of gap (an uncovered recovery gesture) that let #1264 ship in the first place. |
…ing the session Since v3.2.4 a double Ctrl-C left the agent dead, needing a manual respawn. #1241 had made any clean exit end the session, on the reading that a quit is the user's decision and must not be overridden. The corrected framing (architect, on the issue): the problem is not telling ^C^C apart from a typed quit — they are genuinely indistinguishable, both exiting {code: 0, signal: 0} on Claude CLI 2.1.220 — it is that a clean exit kills the shellper at all. So no discrimination is needed: - ANY clean exit: the shellper and session survive; the harness is rerun in the same PTY WITHOUT recovery — a fresh conversation, since the user just deliberately left the old one. - Unnatural exits (crash, signal, nonzero): unchanged, restart WITH recovery. - A session ends only via an explicit kill (afx / UI / DELETE). SessionManager cannot build recovery-free args itself — it holds only the argv it was launched with, which may already carry --resume. So callers supply a FreshLaunch factory. A factory, not a precomputed arg list: every clean exit is a genuinely new conversation and needs its own minted id. buildArchitectFreshLaunch mints it, re-injects the role (which the resume path deliberately skips), and PERSISTS it to the architect row — otherwise a later crash would resume the conversation the user just walked away from. All three architect wiring sites build it from baseArgs, never from the resolved args, so a --resume can never leak into the rerun. PtySession now takes the same restart-wait path for clean exits as for crashes — session kept alive, 'exit' suppressed so Tower keeps the architect row and its clients — sharing one extracted startRestartWait that differs only in the notice, since 'restarting' would misdescribe getting a NEW conversation back. Clean-exit reruns do not touch restartCount or the crash-loop history: a user quitting their agent is a gesture, not a failure, and a long-lived architect can out-quit any budget. Safety valve (beyond the prescribed design, flagged in the PR): unlimited reruns would remove a bound BOTH prior versions had (#1241 ended the session; pre-3.2.4 had maxRestarts), letting a harness misconfigured to exit 0 on startup respawn forever. A clean exit within 2s of launch is a broken command rather than a human gesture, so 5 consecutive such exits give up. A real quit never trips it; one fast exit followed by a healthy session resets the count. Builders are out of scope by architect decision — their bash launch loop is tracked separately in #1267.
…l resume Replaces the #1241 matrix, which asserted the behavior this issue reverses. session-manager: - a clean exit reruns the harness and the session STAYS (no session-clean-exit) - the relaunch argv carries no --resume and none of the old conversation id — the headline guarantee - every clean exit mints a NEW conversation, never reusing the first - restartCount and the crash-loop history stay untouched across repeated clean exits (the budget must not be consumed by gestures) - no fresh-launch factory (plain shells) still reruns, with args unchanged - signal death and crash restart WITH recovery: argv preserved and the factory never consulted The real-shellper 'exit 0' test now asserts the fast-clean-exit valve instead of the old end-the-session behavior: that command exits cleanly the instant it launches, which is a broken harness rather than a user gesture, so it is bounded and given up — and still never applies the crashLoopFallback (#1149). tower-shellper-integration: a clean exit suppresses 'exit' and awaits the rerun (emitting it would clear the architect row — the #1264 symptom), shows a notice that does not promise the old conversation back, and the bounded wait still tears down if no rerun ever arrives.
e57ea6a to
37fe551
Compare
waleedkadous
changed the title
…-up loud Two review findings plus the architect's ruling on the fast-exit valve. 1. CMAP (codex): reconnectSession accepted freshLaunch in its options type but never copied it onto the session. Worse than it sounds — a reconnected architect's args already carry --resume (baked by the #832 restart resolution), so after ANY Tower restart a clean exit would have relaunched straight back into the conversation the user just quit. The headline guarantee, silently broken on the most common surface, with every fresh-path test still green. Fixed, with a regression test that drives the real reconnectSession over a real shellper socket. (My first attempt at that test asserted on a hand-built object literal and never called reconnectSession — it would not have caught the bug it was written for. Rewritten.) 2. Architect ruled KEEP on the fast-clean-exit valve, requiring the give-up be loud. A new 'session-gave-up' event carries a plain-language reason that Tower writes into the terminal itself via PtySession.notice, not just the log: someone with a misconfigured command needs to see why their pane went quiet rather than meeting a mystery dead session. Deliberately NOT reusing 'session-error' — that also fires for transient client errors that do not end the session, so surfacing it as a teardown notice would be wrong. maxRestarts exhaustion has the same silent-death UX and is NOT covered here; noted in the PR as a follow-up rather than quietly widening scope. 3. Valve test coverage per the architect: gives up after 5 fast exits with a reason naming the behavior and saying respawning stopped; one healthy session resets the counter; real (slow) quits never trip it even at twice the threshold.
CMAP round 2 (reworked design)
Codex's finding — the most consequential of either round
Verified against the source: the reconnect path assembled The impact is worse than the phrasing suggests. A reconnected architect's args already carry Fixed, plus the regression test codex correctly noted was missing. My first attempt at that test was bad and I want it on the record: it asserted on a hand-built object literal and never called Also in this commit — the architect's ruling on the fast-exit valveKeep it, with the give-up made loud: a new Verification after the fixes
Incidental discoverySeveral pre-existing reconnect tests guard with Note on the split verdictsTwo rounds, and in both a pair of APPROVEs sat alongside a single specific, reproducible codex finding — both of which were real defects. Worth remembering when weighing a 2-1 CMAP. |
1 participant
Summary
Since v3.2.4, killing an agent with double Ctrl-C left it dead and needed a manual respawn. The fix is not to tell
^C^Capart from a typed quit — the corrected framing is that a clean harness exit should never kill the shellper at all.Fixes #1264
Root Cause
#1241 made any clean exit end the session, reading a quit as a decision that must not be overridden. Probing the real CLI shows why discrimination was a dead end anyway — Claude CLI 2.1.220 under node-pty:
^C^C{exitCode: 0, signal: 0}/quit{exitCode: 0, signal: 0}Byte-identical. The corrected spec removes the need to distinguish them.
Scope note:
restartOnExitis set only for architect sessions (tower-instances.ts×2,tower-terminals.ts×1), so this is the surface that actually died. Builders run the harness inside a bash loop and are out of scope by architect decision — tracked in #1267.Fix
--resume; fresh conversation).DELETE).session-manager.tsFreshLaunchfactory instead of ending the session; not counted as a restart.pty-session.tsstartRestartWaitwith the crash path — differing only in the notice.tower-utils.tsbuildArchitectFreshLaunch— mints a new conversation id, re-injects the role, persists the id to the architect row.tower-instances.ts×2,tower-terminals.ts×1Two things that are load-bearing rather than incidental:
FreshLaunchis a factory, not a precomputed arg list. Every clean exit is a genuinely new conversation and needs its own minted id. Reusing one set of args would make the second^C^Ccollide with the session id the first one just created.The new id is persisted. Otherwise an unnatural exit after a clean rerun would resume the conversation the user just deliberately walked away from. All three wiring sites build the factory from
baseArgs/cmdParts, never from the resolved args — those may already carry--resume, which would defeat the whole point.Design note: the fast-clean-exit valve (architect-approved, first-class)
Making clean-exit reruns unlimited is correct — quitting is a gesture, not a failure — but it removes a bound that both prior versions had (#1241 ended the session; pre-3.2.4 had
maxRestarts). A harness misconfigured to exit 0 on startup would then respawn forever.So: a clean exit within 2s of launch is evidence of a broken command, not a human gesture — 5 consecutive such exits give up. A real quit never trips it, and one fast exit followed by a healthy session resets the counter. Genuine gestures stay unlimited.
The give-up is loud by design: a new
session-gave-upevent carries a plain-language reason that Tower writes into the terminal (PtySession.notice) as well as the log, so a user with a misconfigured command sees why their pane went quiet instead of meeting a mystery dead session. This deliberately does not reusesession-error, which also fires for transient client errors that do not end the session — surfacing that as a teardown notice would lie.Known gap, not addressed here:
maxRestartsexhaustion has the identical silent-death UX and does not get this treatment. Out of scope for this fix; flagging it as a follow-up candidate rather than quietly widening the diff.Test Plan
End-to-end — real
SessionManager→ real shellper process → realclaude, asserting on the argv actually relaunched:^C^Cfresh-restart, session live, agent back up--session-id, no--resume, factory called 1×restart#1, session live, agent back upUnit (replacing the #1241 matrix, which asserted the behavior this reverses):
session-clean-exit--resumeand none of the old conversation idrestartCountand crash-loop history untouched across repeated clean exitsexitand awaits the rerun; bounded wait still tears down if none arrivesexit 0case now asserts the fast-clean-exit valve, and still never appliescrashLoopFallback(Tower recovery: Claude architect crash-loops on stale --resume with no user escape #1149)Full suite: 3759 passed, 0 failed, 48 skipped (194 files); build green.
CMAP on the superseded design (kept for the record)
gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES — codex correctly found that Ctrl-D after a Ctrl-C would have restarted an EOF quit; fixed before the rework. Two approvals did not outvote one specific, reproducible finding. Re-CMAP on this diff is running.
reconnectSession— a post-Tower-restart clean exit still relaunches fresh, driven through the real reconnect path over a real shellper socket (CMAP finding, see below)CMAP
Round 1 (superseded
0x03design) — gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES: Ctrl-D after a Ctrl-C would have restarted an EOF quit. Fixed before the rework.Round 2 (this design) — gemini APPROVE · claude APPROVE · codex REQUEST_CHANGES:
Correct, and the most consequential finding in either round. A reconnected architect's args already carry
--resume, so after any Tower restart (i.e. everylocal-install) a clean exit would have relaunched into the conversation the user just quit — the headline guarantee silently broken on the most common surface, with every fresh-path test still passing. Fixed in4b7db2d6, with the regression test codex noted was missing.My first attempt at that test asserted on a hand-built object literal and never called
reconnectSession— it would not have caught the bug it was written for. Rewritten against a realShellperProcessover a real socket.Two rounds, two APPROVEs each sitting alongside one specific reproducible codex finding, and both findings real.
Incidental discovery while fixing it: several pre-existing reconnect tests guard with
if (client)and passDate.now()as the process start time, which never matches the real one — so those test bodies silently never execute. Not touched here; worth its own issue.