BitSquare/Codeman
2
0
Fork 0
mirror of https://github.com/Ark0N/Codeman.git synced 2026-10-05 23:19:43 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(session): persist an exit retraction, and let tests reach the watcher

Browse Source 
Ten findings from a two-model review of this branch. Both reviewers cleared the
detection logic itself; everything here is a gap around it.

A route that starts a command in a pane now PERSISTS as well as broadcasts.
`/interactive` and `/shell` did neither before, and the pane-exit watcher cannot
cover for them: its next tick finds `paneExit` already cleared in memory,
reports no change and writes nothing, so `state.json` kept saying the agent had
exited for as long as the session stayed quiet. Nothing reads that record for a
decision yet, which is exactly why it had to be fixed now — part 2 is designed
to read it. The `clearPaneExitForNewPane()` docstring claimed its callers
already persisted; that claim was false for these two, and now says what the
caller owes instead.

The watcher's four guards were unreachable by any test. `refreshPaneExits()`
opened with `if (IS_TEST_MODE) return;`, so the read gate, the in-flight
suppression, the generation counter and the empty-read rule could each be
deleted with the whole suite green. The tmux call moves into `readPaneRows()`,
which a test subclass overrides — the shape `runRemoteReconnectTick` already
uses in this file for the same reason — and the test-mode gate moves with it, so
what a test cannot do is spawn a process rather than exercise the bookkeeping.
Each of the four guards now has a test that fails when it is deleted.

The muted status dot turned out to be a specificity fight on three surfaces, not
two. `.tab-status.error` was not excluded, so a session whose agent exited and
whose PTY-exit breaker then tripped lost its red dot to the mute — the state the
browser answers with a "restart it?" confirm, and a needs-you colour by the same
argument that protects the two alert classes. And mobile.css gives a `busy` dot
a 9px size and a green glow with `!important`, while `status` stays `busy` for a
pane whose agent died mid-turn, so a phone rendered a grey dot still wearing the
green halo beside a badge reading "exited". Both measured against the real
stylesheets, both now excluded, and the CSS test reads mobile.css too instead of
being structurally blind to half the problem.

Six comments said things that were not true. Two named the stats collector as
what replaces a restored reading, which is the opposite of the design. The
interval constant argued that 2000 ms keeps a read inside a tick, when the
5000 ms exec timeout means it cannot — which is why the in-flight guard exists.
`MuxSession.discovered` did not say the flag is permanent, though `saveSessions()`
serializes it. The empty-read docstring claimed a distinction that `|| true`
makes impossible. The invariants doc promised more than its drift test delivers.
And CLAUDE.md had no pointer at all, leaving its two hardest prohibitions
("never set `status: 'error'`", "never null the pid") only in the file it is
meant to route people to.

Refs Ark0N/Codeman#446.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Michael Grundberg
2026-09-22 15:24:09 +02:00
co-authored by Claude Opus 5
parent 90fd0a5a15
commit 9c286eeddf
12 changed files with 316 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/tmux-manager.ts
+46 -20
View File
@@ -157,8 +157,9 @@ const DEFAULT_STATS_INTERVAL_MS = 2000;
* How often the pane-exit watcher re-reads every pane on the socket. The
* watcher owns this cadence: it does NOT ride `startStatsCollection()`, whose
* lifetime a browser panel controls (see {@link TmuxManager.startPaneExitWatcher}).
* Matched to the stats cadence above because both cost one batched tmux read,
* and kept well under EXEC_TIMEOUT_MS so a normal read finishes inside a tick.
* Matched to the stats cadence above because both cost one batched tmux read.
* ⚠ It does NOT bound a read: EXEC_TIMEOUT_MS is 5000 ms, so a slow read can
* outlive two ticks, which is exactly why `paneExitReadInFlight` exists.
*/
const DEFAULT_PANE_EXIT_INTERVAL_MS = 2000;
@@ -3079,9 +3080,13 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
* polls rather than probing per session.
*
* A failed or empty probe leaves the previous answers ALONE rather than
* clearing them. An empty read is "tmux did not answer", and clearing on it
* would turn a transient failure into a silent retraction of a death Codeman
* had already observed. A NON-empty read is different: `list-panes -a` lists
* clearing them, because the two cannot be told apart: the command ends in
* `|| true`, so a tmux that errored and a socket with genuinely no panes both
* arrive as empty output. Treating that as "tmux did not answer" is the
* conservative reading — clearing on it would turn a transient failure into a
* silent retraction of a death Codeman had already observed, and the cost of
* being wrong the other way is one stale entry for a socket that no longer
* has the pane. A NON-empty read is different: `list-panes -a` lists
* every pane on the socket, so it is authoritative and {@link applyPaneExits}
* prunes against it.
*
@@ -3096,8 +3101,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
* command in a pane calls {@link clearPaneExit} itself.
*/
async refreshPaneExits(now: number = Date.now()): Promise<void> {
if (IS_TEST_MODE) return;
// Nothing on this socket could answer, so do not exec tmux to find that
// Nothing on this socket could answer, so do not read tmux to find that
// out. See `hasObservablePaneSession`: the watcher above still ticks.
if (!hasObservablePaneSession(this.sessions.values())) return;
if (this.paneExitReadInFlight) return;
@@ -3106,18 +3110,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
this.paneExitReadInFlight = true;
let rows: PaneRow[];
try {
// execAsync, not execSync: this runs on a 2000 ms timer, and a synchronous
// exec freezes the port while the process stays alive (see the
// event-loop-monitor note in CLAUDE.md). The three `isPaneDead()` callers
// stay synchronous because each is answering one request right then.
const { stdout } = await execAsync(`${this.tmux()} list-panes -a -F '${PANE_LIST_FORMAT}' 2>/dev/null || true`, {
encoding: 'utf-8',
timeout: EXEC_TIMEOUT_MS,
});
rows = parsePaneRows(stdout.trim());
} catch (err) {
console.error('[TmuxManager] Failed to read pane exit state:', err);
return;
rows = await this.readPaneRows();
} finally {
this.paneExitReadInFlight = false;
}
@@ -3130,6 +3123,37 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
this.applyPaneExits(derivePaneExits(rows, now));
}
/**
* Read every pane on the socket. The ONLY part of the pane-exit watcher that
* touches tmux, which is what lets a test subclass drive the guards in
* {@link refreshPaneExits} — the in-flight suppression, the generation
* check, the empty-read retraction rule and the read gate — against rows it
* chooses. Split out for the reason `runRemoteReconnectTick` is: a guard no
* test can reach is a guard that can be deleted without anything failing.
*
* A failed read answers with NO rows, which the caller treats as "tmux did
* not answer" and which therefore retracts nothing.
*/
protected async readPaneRows(): Promise<PaneRow[]> {
// The test-mode gate lives HERE rather than at the top of the tick, so that
// what tests cannot do is spawn a process, not exercise the bookkeeping.
if (IS_TEST_MODE) return [];
try {
// execAsync, not execSync: this runs on a 2000 ms timer, and a synchronous
// exec freezes the port while the process stays alive (see the
// event-loop-monitor note in CLAUDE.md). The three `isPaneDead()` callers
// stay synchronous because each is answering one request right then.
const { stdout } = await execAsync(`${this.tmux()} list-panes -a -F '${PANE_LIST_FORMAT}' 2>/dev/null || true`, {
encoding: 'utf-8',
timeout: EXEC_TIMEOUT_MS,
});
return parsePaneRows(stdout.trim());
} catch (err) {
console.error('[TmuxManager] Failed to read pane exit state:', err);
return [];
}
}
/**
* Fold one authoritative observation into {@link paneExits}. Split out from
* the tmux call so the merge rules are unit-testable.
@@ -3190,7 +3214,9 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
clearInterval(this.paneExitInterval);
}
this.paneExitInterval = setInterval(() => {
if (IS_TEST_MODE) return;
// No IS_TEST_MODE guard: `readPaneRows()` is the only thing that would
// spawn a process and it refuses under test, so a test can drive this
// whole loop with fake timers instead of being locked out of it.
void this.refreshPaneExits()
.then(() => this.emit('paneExitsUpdated'))
.catch((err) => console.error('[TmuxManager] Pane exit watcher error:', err));