Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/agent-coordination-plan.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ Step A has passed `npm run check`, `npm run check:static`, and the full unit/cli

Step B has passed `npm run check`, `npm run check:static`, and the full unit/client suites (4,995 tests passed; 33 opt-in or otherwise skipped tests). The real-PTY coordinator suite also passed all 9 tests. Full tests required execution outside the sandbox because local socket binding and some Git subprocesses were blocked with `EPERM`. Native Electron smoke testing remains unperformed; no paid real-agent suite was run.

Step B preserves hook observations across reattachment, rejects late events from replaced/retired launches, invalidates finished activity on prompt submission, and shares matching-tool wait reduction and source timestamps. Renderer snapshot ordering covers newer subscribed events, retired launches, and missed retirements while unsubscribed. Local interrupt/approval inference remains renderer-owned and labeled. Ongoing hook evidence expires from UI activity after 30 minutes and falls back to labeled terminal/process inference; MCP retains the cached hook observation with `freshness: stale`. Session readiness, input readiness, turn completion, and failure are distinct labels, independent of assignment completion. Static dot tooltips use absolute observation times; the existing clock-driven status line uses relative ages.
Step B preserves hook observations across reattachment, rejects late events from replaced/retired launches, invalidates finished activity on prompt submission, and shares matching-tool wait reduction and source timestamps. Renderer snapshot ordering covers newer subscribed events, retired launches, and missed retirements while unsubscribed. Local interrupt/approval inference remains renderer-owned and labeled. Working hook evidence expires from UI activity after 30 minutes and falls back to labeled terminal/process inference; MCP retains the cached hook observation with `freshness: stale`. Waiting evidence never expires, because a blocked agent stays silent until someone answers. A failed turn stays failed through Claude's idle reminder until the next turn starts. Session readiness, input readiness, turn completion, and failure are distinct labels, independent of assignment completion. Static dot tooltips use absolute observation times; the existing clock-driven status line uses relative ages.

Two independent Step B implementation reviews found a P2 provenance defect (reducing main observations again against local interrupt state) and a P3 tooltip defect (nonreactive terminal observation timestamps). Both are corrected: accepted main observations replace hook fields directly while preserving local unread/suppression handling, and terminal timestamps are reactive and cleared on replacement/cleanup. Regression tests also cover a newer snapshot replacing an older tool wait. Both reviewers' isolated reproductions now pass, along with 186 focused unit tests and the full client suite (1,123 passed; 6 skipped).

Expand Down
26 changes: 23 additions & 3 deletions electron/agent-hooks/observations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,34 @@ describe('live agent activity observations', () => {
detail: 'Provider unavailable',
freshness: 'current',
});
observeAgentHook(event({ state: 'done', event: 'Notification', at: 200 }));
// The idle reminder says nothing new about a failed turn.
expect(
observeAgentHook(event({ state: 'done', event: 'Notification', at: 200 })),
).toBeUndefined();
expect(getAgentActivityEvidence(agentId, 201)).toMatchObject({
activity: 'ready',
since: 200,
activity: 'turn_finished',
event: 'StopFailure',
since: 100,
detail: 'Provider unavailable',
});
});

it('turns a finished turn into readiness on the idle reminder', () => {
registerAgentLaunch(agentId, taskId, launchId);
observeAgentHook(event({ state: 'done', event: 'Stop', at: 100 }));
observeAgentHook(event({ state: 'done', event: 'Notification', at: 200 }));
expect(getAgentActivityEvidence(agentId, 201)).toMatchObject({ activity: 'ready', since: 200 });
});

it('keeps an unanswered wait current however long it lasts', () => {
registerAgentLaunch(agentId, taskId, launchId);
observeAgentHook(event({ state: 'waiting', event: 'PermissionRequest', prompt: 'permission' }));
expect(getAgentActivityEvidence(agentId, 100 + AGENT_HOOK_STALE_MS)).toMatchObject({
activity: 'waiting',
freshness: 'current',
});
});

it('expires ongoing evidence without mutating the snapshot and invalidates finished facts', () => {
registerAgentLaunch(agentId, taskId, launchId);
observeAgentHook(event({}));
Expand Down
7 changes: 6 additions & 1 deletion electron/agent-hooks/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ export function transitionAgentHookStatus(
previous.toolUseId !== event.toolUseId
)
return previous;
// Claude's idle reminder re-reports a finished turn. After a failed turn it
// says nothing new, and the failure must stay visible until a new turn starts.
if (previous?.event === 'StopFailure' && event.state === 'done' && event.event === 'Notification')
return previous;
const sameState = previous?.state === event.state;
const finished = (name: string) => name === 'Stop' || name === 'StopFailure';
const sameActivity =
Expand Down Expand Up @@ -278,8 +282,9 @@ export function activityEvidenceFromHook(
since: status.since,
prompt: status.prompt,
detail: status.detail,
// A waiting agent is silent until answered, so only `working` goes stale.
freshness:
status.state !== 'done' && now - status.updatedAt >= AGENT_HOOK_STALE_MS
status.state === 'working' && now - status.updatedAt >= AGENT_HOOK_STALE_MS
? 'stale'
: 'current',
};
Expand Down
21 changes: 19 additions & 2 deletions electron/chat/claude.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,23 @@ const askOptions = (overrides: Partial<AskOptions> = {}): AskOptions => ({
...overrides,
});

/** A killed orphan lingers as a zombie where init does not reap it, as in
* some containers, and signal 0 still reaches a zombie. */
function isRunning(pid: number): boolean {
try {
process.kill(pid, 0);
} catch {
return false;
}
try {
// "pid (comm) state …"; comm may itself contain parentheses.
const stat = readFileSync(`/proc/${pid}/stat`, 'utf8');
return stat.charAt(stat.lastIndexOf(')') + 2) !== 'Z';
} catch {
return true; // No procfs (macOS); signal 0 is all there is.
}
}

const chats: ClaudeChat[] = [];
let settingsDir: string;
beforeEach(() => {
Expand Down Expand Up @@ -639,9 +656,9 @@ describe('Claude chat adapter', () => {
await h.chat.runShell('(sleep 30 & echo $! > job.pid); echo started');
await vi.waitFor(() => expect(h.chat.state.items[0].activity?.status).toBe('completed'));
const pid = Number(readFileSync(join(cwd, 'job.pid'), 'utf8'));
expect(() => process.kill(pid, 0)).not.toThrow();
expect(isRunning(pid)).toBe(true);
h.chat.stop();
await vi.waitFor(() => expect(() => process.kill(pid, 0)).toThrow());
await vi.waitFor(() => expect(isRunning(pid)).toBe(false));
});

it('refuses a ! command while Claude is answering', async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/components/StatusDot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export function getDotTooltip(
if (attention === 'active') return 'Active — agent is working';
if (attention === 'shell_busy') return 'Terminal busy — no agent working';
if (attention === 'needs_input') return 'Waiting for input';
if (attention === 'error') return 'Error — agent exited with an error';
if (attention === 'error') return 'Error — an agent exited or its turn failed';
// Without this, a review-flagged task whose agent is still active falls
// through to the dot-status map and reads "Busy" under a purple dot.
if (attention === 'review') return 'Ready for review';
Expand Down
7 changes: 4 additions & 3 deletions src/components/TerminalView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import {
registerTerminal,
unregisterTerminal,
markDirty,
redrawTerminal,
redrawAllTerminals,
} from '../lib/terminalFitManager';
import { dataTransferToShellArgs, escapePath } from '../lib/terminalDrop';
import { cleanCopiedTerminalText } from '../lib/copy-text';
Expand Down Expand Up @@ -1146,7 +1146,8 @@ export function TerminalView(props: TerminalViewProps) {
// on macOS it is repainted instead (issue #121): a WebGL surface throttled
// while backgrounded can come back with a corrupt glyph atlas, and a
// hidden pane never fires terminalFitManager's IntersectionObserver.
// Linux never showed the corruption, so it skips that repaint.
// The repaint covers every terminal because the atlas it rebuilds is
// shared. Linux never showed the corruption, so it skips that repaint.
let prevOnScreen: boolean | undefined;
createEffect(() => {
const onScreen = paneOnScreen();
Expand All @@ -1159,7 +1160,7 @@ export function TerminalView(props: TerminalViewProps) {
// loadAddon → setRenderer already repaints this pane in full.
attachWebgl();
} else if (isMac && prevOnScreen === false) {
redrawTerminal(agentId);
redrawAllTerminals();
Comment thread
johannesjo marked this conversation as resolved.
}
} else if (webglDetachTimer === undefined) {
webglDetachTimer = window.setTimeout(() => {
Expand Down
90 changes: 90 additions & 0 deletions src/components/TerminalView.webgl.client.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import { createSignal } from 'solid-js';
import { render } from 'solid-js/web';
import { afterEach, beforeEach, expect, it, vi } from 'vitest';
import { IPC } from '../../electron/ipc/channels';
import { WEBGL_DETACH_DELAY_MS } from '../lib/terminalPaneVisibility';
import { TerminalView } from './TerminalView';

const { redrawAllTerminals } = vi.hoisted(() => ({ redrawAllTerminals: vi.fn() }));

vi.mock('../lib/platform', async (importOriginal) => ({
...(await importOriginal<typeof import('../lib/platform')>()),
isMac: true,
}));

// happy-dom has no WebGL; a stand-in addon is enough to put the pane on the
// WebGL path that keeps its context across a brief trip off screen.
vi.mock('@xterm/addon-webgl', () => ({
WebglAddon: class {
activate(): void {}
dispose(): void {}
onContextLoss() {
return { dispose() {} };
}
},
}));

vi.mock('../lib/terminalFitManager', async (importOriginal) => ({
...(await importOriginal<typeof import('../lib/terminalFitManager')>()),
redrawAllTerminals,
}));

let dispose: (() => void) | undefined;

beforeEach(() => {
vi.useFakeTimers();
// happy-dom has no canvas; xterm's DOM renderer only measures glyph widths.
vi.spyOn(HTMLCanvasElement.prototype, 'getContext').mockReturnValue({
font: '',
fillRect: vi.fn(),
clearRect: vi.fn(),
measureText: () => ({ width: 8 }),
} as unknown as CanvasRenderingContext2D);
vi.stubGlobal('OffscreenCanvas', undefined);
vi.stubGlobal('electron', {
ipcRenderer: {
invoke: vi.fn((cmd: string) =>
cmd === IPC.SpawnAgent ? new Promise(() => {}) : Promise.resolve(undefined),
),
on: () => () => undefined,
},
});
redrawAllTerminals.mockClear();
});

afterEach(() => {
dispose?.();
dispose = undefined;
document.body.replaceChildren();
vi.useRealTimers();
vi.unstubAllGlobals();
vi.restoreAllMocks();
});

it('repaints every terminal, not just its own, when a pane returns on macOS (issue #121)', () => {
const [visible, setVisible] = createSignal(true);
const host = document.createElement('div');
document.body.append(host);
dispose = render(
() => (
<TerminalView
taskId="task-1"
agentId="agent-1"
command="codex"
args={[]}
cwd="/tmp"
isShell
visible={visible()}
/>
),
host,
);

setVisible(false);
vi.advanceTimersByTime(WEBGL_DETACH_DELAY_MS / 2);
expect(redrawAllTerminals).not.toHaveBeenCalled();
setVisible(true);

// The glyph atlas is shared, so a repaint must reset every terminal's model.
expect(redrawAllTerminals).toHaveBeenCalledTimes(1);
});
58 changes: 58 additions & 0 deletions src/lib/terminalFitManager.client.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,61 @@ describe('deferred resizes', () => {
expect(addon.fit).toHaveBeenCalledTimes(1);
});
});

describe('repainting terminals', () => {
const ids = ['repaint-a', 'repaint-b', 'repaint-c'];

function registerRepaintable() {
return ids.map((id) => {
const term = {
rows: 24,
buffer: { active: { viewportY: 0, baseY: 0 } },
clearTextureAtlas: vi.fn(),
refresh: vi.fn(),
};
manager.registerTerminal(
id,
document.createElement('div'),
{ fit() {} } as unknown as FitAddon,
term as unknown as Terminal,
);
return { id, term };
});
}

afterEach(() => {
for (const id of ids) manager.unregisterTerminal(id);
});

it('resets every terminal, since they share one glyph atlas', async () => {
const terms = registerRepaintable();
// A terminal torn down mid-repaint must not stop the others.
terms[1].term.clearTextureAtlas.mockImplementation(() => {
throw new Error('disposed');
});

manager.redrawAllTerminals();
await Promise.resolve();

for (const { id, term } of terms) {
expect(term.clearTextureAtlas).toHaveBeenCalledTimes(1);
if (id !== 'repaint-b') expect(term.refresh).toHaveBeenCalledWith(0, 23);
}
});

it('shares one repaint between requests made in the same tick', async () => {
const terms = registerRepaintable();

// Every pane of a task returning on screen asks at once.
manager.redrawAllTerminals();
manager.redrawAllTerminals();
manager.redrawAllTerminals();
await Promise.resolve();
for (const { term } of terms) expect(term.clearTextureAtlas).toHaveBeenCalledTimes(1);

// A later request repaints again.
manager.redrawAllTerminals();
await Promise.resolve();
for (const { term } of terms) expect(term.clearTextureAtlas).toHaveBeenCalledTimes(2);
});
});
24 changes: 18 additions & 6 deletions src/lib/terminalFitManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,23 @@ function redraw(term: Terminal): void {
}
}

export function redrawTerminal(id: string): void {
const entry = entries.get(id);
if (entry) redraw(entry.term);
}

/**
* Repaint every terminal. There is deliberately no single-terminal variant:
* xterm shares one glyph atlas between all WebGL terminals with the same font
* and theme, and clearing it only resets the calling terminal's render model.
* The others keep pointing at atlas cells that are then refilled with other
* glyphs, which garbles them (issue #121). The repaints are deferred to the
* next frame, so every model is cleared before any glyph is drawn again.
*
* Requests made in the same tick share one repaint: every pane of a task that
* returns on screen asks at once.
*/
let redrawQueued = false;
export function redrawAllTerminals(): void {
for (const [, entry] of entries) redraw(entry.term);
if (redrawQueued) return;
redrawQueued = true;
queueMicrotask(() => {
redrawQueued = false;
for (const [, entry] of entries) redraw(entry.term);
});
}
27 changes: 27 additions & 0 deletions src/store/agentHookStatus.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,33 @@ describe('agentHookStatus', () => {
expect(getAgentHookStatus('a2')?.state).toBe('done');
});

it('keeps an unanswered prompt however long it waits', () => {
applyAgentHookEvent(event({ agentId: 'a1', state: 'working' }));
applyAgentHookEvent(
event({
agentId: 'a1',
state: 'waiting',
event: 'PermissionRequest',
prompt: 'permission',
at: Date.now() + 1_000,
}),
);
vi.advanceTimersByTime(AGENT_HOOK_STALE_MS * 4);
expect(getAgentHookStatus('a1')).toMatchObject({ state: 'waiting', prompt: 'permission' });
expect(getTaskAgentHookStatus('t1')?.state).toBe('waiting');
});

it('keeps a restored wait whose observation is older than the stale window', () => {
const old = observation({
state: 'waiting',
event: 'PermissionRequest',
prompt: 'permission',
at: Date.now() - AGENT_HOOK_STALE_MS * 2,
});
applyAgentActivitySnapshot({ sequence: old.sequence, observations: [old] });
expect(getAgentHookStatus('a1')?.state).toBe('waiting');
});

it('infers an interrupt from a bare Esc that no hook event follows', () => {
applyAgentHookEvent(event({ state: 'working' }));
noteAgentTerminalInput('a1', '\x1b');
Expand Down
9 changes: 6 additions & 3 deletions src/store/agentHookStatus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export interface TaskAgentHookStatus extends AgentHookStatus {
agentId: string;
}

/** A `working`/`waiting` claim this old has lost its hook stream; heuristics take over. */
/** A `working` claim this old has lost its hook stream; heuristics take over. */
export { AGENT_HOOK_STALE_MS };
/** Esc/Ctrl-C only counts as an interrupt if no hook event follows within this window. */
const INTERRUPT_SETTLE_MS = 500;
Expand Down Expand Up @@ -96,8 +96,11 @@ function clearTimer(timers: Map<string, ReturnType<typeof setTimeout>>, agentId:
function setStatus(agentId: string, status: AgentHookStatus): void {
updateStatuses((next) => next.set(agentId, status));
clearTimer(staleTimers, agentId);
// `done` is a terminal fact and stays put; only an ongoing claim can go stale.
if (status.state === 'done') return;
// Only `working` can go stale: a working agent keeps firing hooks, so long
// silence means the stream was lost. A waiting agent is silent by design
// until someone answers, and an unanswered prompt matters most after a long
// absence. `done` is a terminal fact and stays put.
if (status.state !== 'working') return;
const remaining = status.updatedAt + AGENT_HOOK_STALE_MS - Date.now();
if (remaining <= 0) {
updateStatuses((next) => next.delete(agentId));
Expand Down
Loading
Loading