mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
Michael GrundbergandClaude Opus 5
9c286eeddf
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>
290 lines
13 KiB
TypeScript
290 lines
13 KiB
TypeScript
/**
|
|
* @fileoverview The exited-agent badge on a session tab (Ark0N/Codeman#446).
|
|
*
|
|
* The server publishes `session.paneExit` when the agent inside a local tmux
|
|
* pane has exited while `remain-on-exit` kept the pane. These cover the three
|
|
* things the browser owns: turning that field into a label, getting the label
|
|
* onto and off a tab, and what colour the tab's status dot ends up once the
|
|
* exit, the alert rules and the rich rail's own rules have all had a say.
|
|
*
|
|
* The incremental render path is the only one a live session ever reaches.
|
|
* Going from live to exited adds and removes no tab, so the full rebuild never
|
|
* runs for it, which is why `applyPaneExitBadge()` is a named function rather
|
|
* than a block inside the render loop.
|
|
*
|
|
* Port: N/A
|
|
*/
|
|
import { readFileSync } from 'node:fs';
|
|
import { resolve } from 'node:path';
|
|
import { JSDOM } from 'jsdom';
|
|
import postcss from 'postcss';
|
|
import { describe, expect, it } from 'vitest';
|
|
|
|
describe('the exited-agent tab label', () => {
|
|
const appJs = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
|
|
const load = <T>(name: string) => {
|
|
const source = appJs.match(new RegExp(`function ${name}\\([\\s\\S]*?\\n\\}`))?.[0];
|
|
if (!source) throw new Error(`${name} not found in app.js`);
|
|
return new Function(`${source}\nreturn ${name};`)() as T;
|
|
};
|
|
const paneExitLabel = load<(p: unknown) => string>('paneExitLabel');
|
|
|
|
it('renders nothing for an unknown answer, which must never read as alive', () => {
|
|
expect(paneExitLabel(undefined)).toBe('');
|
|
expect(paneExitLabel(null)).toBe('');
|
|
});
|
|
|
|
it('names the exit code', () => {
|
|
expect(paneExitLabel({ status: 137, at: 1 })).toBe('exited (137)');
|
|
});
|
|
|
|
it('shows a clean exit as 0 rather than hiding it', () => {
|
|
expect(paneExitLabel({ status: 0, at: 1 })).toBe('exited (0)');
|
|
});
|
|
|
|
it('names a signal death, which the maintainer wants kept on screen', () => {
|
|
expect(paneExitLabel({ signal: 9, at: 1 })).toBe('exited (signal 9)');
|
|
});
|
|
|
|
it('says only "exited" when tmux knew the pane died but not how', () => {
|
|
// Measured on tmux 3.2a: a SIGKILLed pane reports neither status nor signal.
|
|
// Showing that as "exited (0)" would make an unexplained death look clean.
|
|
expect(paneExitLabel({ at: 1 })).toBe('exited');
|
|
});
|
|
});
|
|
|
|
describe('the exited-agent badge in a tab', () => {
|
|
// The incremental render path is the only one a live session reaches: going
|
|
// from live to exited adds and removes no tab, so the full rebuild never runs
|
|
// for it. These drive that path's DOM work against a real tab element.
|
|
const appJs = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
|
|
const source = [
|
|
appJs.match(/function paneExitLabel\([\s\S]*?\n\}/)?.[0],
|
|
appJs.match(/function applyPaneExitBadge\([\s\S]*?\n\}/)?.[0],
|
|
].join('\n');
|
|
const dom = new JSDOM('<!DOCTYPE html><html><body></body></html>');
|
|
const applyPaneExitBadge = new Function('document', `${source}\nreturn applyPaneExitBadge;`)(dom.window.document) as (
|
|
tab: unknown,
|
|
paneExit: unknown
|
|
) => void;
|
|
|
|
const makeTab = () => {
|
|
const tab = dom.window.document.createElement('div');
|
|
tab.className = 'session-tab';
|
|
tab.innerHTML = '<span class="tab-name">w1-case</span>';
|
|
return tab;
|
|
};
|
|
const badge = (tab: { querySelector: (s: string) => { textContent: string | null } | null }) =>
|
|
tab.querySelector('.tab-exited-badge');
|
|
|
|
it('draws no badge while the answer is unknown', () => {
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, undefined);
|
|
expect(badge(tab)).toBeNull();
|
|
});
|
|
|
|
it('adds the badge after the name once the agent exits', () => {
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, { status: 137, at: 1 });
|
|
expect(badge(tab)?.textContent).toBe('exited (137)');
|
|
expect(tab.querySelector('.tab-name')?.nextElementSibling?.className).toBe('tab-exited-badge');
|
|
});
|
|
|
|
it('marks the badge data-i18n-skip, like the other generated status text', () => {
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, { status: 0, at: 1 });
|
|
expect(badge(tab)?.hasAttribute('data-i18n-skip')).toBe(true);
|
|
});
|
|
|
|
it('updates the text in place rather than stacking a second badge', () => {
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, { status: 0, at: 1 });
|
|
const first = badge(tab);
|
|
applyPaneExitBadge(tab, { status: 137, at: 2 });
|
|
expect(tab.querySelectorAll('.tab-exited-badge')).toHaveLength(1);
|
|
expect(badge(tab)).toBe(first);
|
|
expect(badge(tab)?.textContent).toBe('exited (137)');
|
|
});
|
|
|
|
it('marks the tab so the status dot can be quieted', () => {
|
|
// The dot renders from `status`, which stays `idle` or `busy` for an exited
|
|
// pane by design, so the tab carries the exit as a class and CSS does the
|
|
// rest. Without it a green or pulsing dot sits beside the badge.
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, { status: 0, at: 1 });
|
|
expect(tab.classList.contains('tab-agent-exited')).toBe(true);
|
|
});
|
|
|
|
it('unmarks the tab when the pane comes back', () => {
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, { status: 0, at: 1 });
|
|
applyPaneExitBadge(tab, undefined);
|
|
expect(tab.classList.contains('tab-agent-exited')).toBe(false);
|
|
});
|
|
|
|
it('removes the badge when the pane comes back', () => {
|
|
// The retraction half: a respawned pane must not keep reading "exited".
|
|
const tab = makeTab();
|
|
applyPaneExitBadge(tab, { status: 0, at: 1 });
|
|
applyPaneExitBadge(tab, undefined);
|
|
expect(badge(tab)).toBeNull();
|
|
});
|
|
|
|
it('is what the incremental render path calls', () => {
|
|
expect(appJs).toContain('applyPaneExitBadge(tab, session.paneExit)');
|
|
});
|
|
});
|
|
|
|
describe('what colour the status dot ends up', () => {
|
|
/*
|
|
* The dot renders from `status`, which stays `idle` or `busy` for an exited
|
|
* pane, so the mute is a CSS rule keyed on the `tab-agent-exited` class. It
|
|
* competes with two other families of rule over the same dot, and this tree
|
|
* has lost that competition before: the alert rules and the rich-rail state
|
|
* rules already exclude each other by hand rather than by cascade.
|
|
*
|
|
* So the cascade is resolved rather than asserted from selector text. Every
|
|
* rule in styles.css that paints `.tab-status` goes into a real document and
|
|
* a real engine answers, which is what makes a rule moved up the file or a
|
|
* selector given one more class fail here.
|
|
*
|
|
* ⚠ In styles.css the rules inside an at-rule are skipped, so the desktop
|
|
* cases describe a wide viewport with motion allowed. mobile.css is loaded
|
|
* separately for the phone cases, and there its @media blocks are FLATTENED
|
|
* rather than skipped, because that file is phone-and-tablet-only and its
|
|
* whole content sits inside them. jsdom reports a custom property
|
|
* unresolved, so the expected values are the `var(--x)` tokens the
|
|
* stylesheets write.
|
|
*/
|
|
const readRules = (file: string, flattenMedia: boolean): string[] => {
|
|
const out: string[] = [];
|
|
postcss.parse(readFileSync(resolve(import.meta.dirname, `../src/web/public/${file}`), 'utf8')).walkRules((rule) => {
|
|
if (!rule.selector.includes('.tab-status')) return;
|
|
const parents: string[] = [];
|
|
let insideAtRule = false;
|
|
for (let p = rule.parent; p && p.type !== 'root'; p = p.parent) {
|
|
if (p.type === 'rule') parents.unshift(p.selector);
|
|
else insideAtRule = true;
|
|
}
|
|
if (insideAtRule && !flattenMedia) return;
|
|
const decls: string[] = [];
|
|
rule.each((node) => {
|
|
if (node.type === 'decl') decls.push(`${node.prop}: ${node.value}${node.important ? ' !important' : ''};`);
|
|
});
|
|
if (decls.length === 0) return;
|
|
const selectors = rule.selectors.map((sel) => (parents.length ? `${parents.join(' ')} ${sel}` : sel));
|
|
out.push(`${selectors.join(',')} { ${decls.join(' ')} }`);
|
|
});
|
|
return out;
|
|
};
|
|
|
|
const dotRules = readRules('styles.css', false);
|
|
// index.html loads mobile.css after styles.css, so it goes last here too.
|
|
const phoneRules = [...dotRules, ...readRules('mobile.css', true)];
|
|
|
|
/** Paint the dot of one tab and read back what the cascade decided. */
|
|
const dot = (opts: { tab: string; dotState?: string; rail?: boolean; phone?: boolean }) => {
|
|
const railAttrs = opts.rail ? ` data-tab-orientation="vertical" data-tab-rail-detail="rich"` : '';
|
|
const container = opts.rail ? 'tab-rail' : 'session-tabs';
|
|
const rules = opts.phone ? phoneRules : dotRules;
|
|
const dom = new JSDOM(
|
|
`<!DOCTYPE html><html${railAttrs}><head><style>${rules.join('\n')}</style></head><body>` +
|
|
`<div class="${container}"><div class="session-tab ${opts.tab}">` +
|
|
`<span id="dot" class="tab-status ${opts.dotState ?? 'idle'}"></span></div></div></body></html>`
|
|
);
|
|
const style = dom.window.getComputedStyle(dom.window.document.getElementById('dot')!);
|
|
return {
|
|
background: style.background,
|
|
opacity: style.opacity,
|
|
boxShadow: style.boxShadow,
|
|
animation: style.animation,
|
|
};
|
|
};
|
|
|
|
it('finds the rules it is meant to be resolving', () => {
|
|
// A selector rename that emptied this list would make every case below pass
|
|
// against a stylesheet with no rules in it.
|
|
expect(dotRules.some((rule) => rule.includes('tab-agent-exited'))).toBe(true);
|
|
expect(dotRules.some((rule) => rule.includes('tab-alert-action'))).toBe(true);
|
|
});
|
|
|
|
it('mutes the dot of an exited session', () => {
|
|
expect(dot({ tab: 'tab-agent-exited' })).toMatchObject({ background: 'var(--text-muted)', opacity: '0.5' });
|
|
});
|
|
|
|
it('leaves a live session green', () => {
|
|
expect(dot({ tab: '' }).background).toBe('var(--green)');
|
|
});
|
|
|
|
it('keeps a pending permission dialog RED on an exited session', () => {
|
|
// The one the maintainer asked for: the exit must not quiet an alert. A
|
|
// board that says two things at once is a board people stop trusting, and
|
|
// between "the agent is gone" and "this session is blocked on you", the
|
|
// one that needs a human wins.
|
|
expect(dot({ tab: 'tab-agent-exited tab-alert-action' }).background).toBe('var(--red)');
|
|
});
|
|
|
|
it('keeps a pending idle alert YELLOW on an exited session', () => {
|
|
expect(dot({ tab: 'tab-agent-exited tab-alert-idle' }).background).toBe('var(--yellow)');
|
|
});
|
|
|
|
it('mutes a dot the exit caught mid-turn, and stops it pulsing', () => {
|
|
// `.tab-status.busy` animates `pulse`, so muting the colour alone would
|
|
// leave a grey dot breathing as if the agent were still working.
|
|
expect(dot({ tab: 'tab-agent-exited', dotState: 'busy' })).toMatchObject({
|
|
background: 'var(--text-muted)',
|
|
opacity: '0.5',
|
|
animation: 'none',
|
|
});
|
|
});
|
|
|
|
it('mutes the dot on a rich tab rail too, halo included', () => {
|
|
// The rail's own state rules are far more specific than the strip's mute
|
|
// (measured: an exited session kept a full green dot AND the working halo),
|
|
// so the mute carries a rail twin that must stay below them in source order.
|
|
expect(dot({ tab: 'tab-agent-exited tab-state-working', dotState: 'busy', rail: true })).toMatchObject({
|
|
background: 'var(--text-muted)',
|
|
opacity: '0.5',
|
|
boxShadow: 'none',
|
|
});
|
|
expect(dot({ tab: 'tab-agent-exited tab-state-idle', rail: true }).background).toBe('var(--text-muted)');
|
|
});
|
|
|
|
it('leaves an errored dot red, which is the state that offers a restart', () => {
|
|
// `status: 'error'` is the PTY-exit breaker's value and the browser answers
|
|
// it with a "restart it?" confirm, so it is a needs-you colour by the same
|
|
// argument that protects the two alert classes. Reachable when a restart of
|
|
// a dead pane keeps failing: the breaker trips while the pane stays dead.
|
|
expect(dot({ tab: 'tab-agent-exited', dotState: 'error' }).background).toBe('var(--red)');
|
|
});
|
|
|
|
it('mutes the dot on a phone, glow and all', () => {
|
|
// mobile.css enlarges the working dot to 9px and gives it a green glow with
|
|
// !important, and `status` stays `busy` for a pane whose agent died
|
|
// mid-turn — so without a phone-side rule this renders a grey dot wearing a
|
|
// green halo beside a badge reading "exited".
|
|
expect(dot({ tab: 'tab-agent-exited', dotState: 'busy', phone: true })).toMatchObject({
|
|
background: 'var(--text-muted)',
|
|
boxShadow: 'none',
|
|
});
|
|
});
|
|
|
|
it('keeps an alert red on a phone as well', () => {
|
|
expect(dot({ tab: 'tab-agent-exited tab-alert-action', dotState: 'busy', phone: true }).background).toBe(
|
|
'var(--red)'
|
|
);
|
|
});
|
|
|
|
it('finds the phone rules it is meant to be resolving', () => {
|
|
// Same self-guard as the desktop one: if mobile.css stopped contributing
|
|
// rules, every phone case above would pass against the desktop cascade.
|
|
expect(phoneRules.length).toBeGreaterThan(dotRules.length);
|
|
});
|
|
|
|
it('still keeps an alert red on the rich tab rail', () => {
|
|
expect(
|
|
dot({ tab: 'tab-agent-exited tab-alert-action tab-state-working', dotState: 'busy', rail: true }).background
|
|
).toBe('var(--red)');
|
|
});
|
|
});
|