diff --git a/docs/agent-coordination-plan.md b/docs/agent-coordination-plan.md index e7f8039ba..8a263be95 100644 --- a/docs/agent-coordination-plan.md +++ b/docs/agent-coordination-plan.md @@ -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). diff --git a/electron/agent-hooks/observations.test.ts b/electron/agent-hooks/observations.test.ts index 9035fbb78..7d471abf1 100644 --- a/electron/agent-hooks/observations.test.ts +++ b/electron/agent-hooks/observations.test.ts @@ -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({})); diff --git a/electron/agent-hooks/status.ts b/electron/agent-hooks/status.ts index 7b72573dc..be279ab28 100644 --- a/electron/agent-hooks/status.ts +++ b/electron/agent-hooks/status.ts @@ -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 = @@ -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', }; diff --git a/electron/chat/claude.test.ts b/electron/chat/claude.test.ts index 3ac50eab5..59035014d 100644 --- a/electron/chat/claude.test.ts +++ b/electron/chat/claude.test.ts @@ -23,6 +23,23 @@ const askOptions = (overrides: Partial = {}): 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(() => { @@ -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 () => { diff --git a/src/components/StatusDot.tsx b/src/components/StatusDot.tsx index e4948e23e..ecded77fd 100644 --- a/src/components/StatusDot.tsx +++ b/src/components/StatusDot.tsx @@ -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'; diff --git a/src/components/TerminalView.tsx b/src/components/TerminalView.tsx index e2fd39dd8..3e19119d1 100644 --- a/src/components/TerminalView.tsx +++ b/src/components/TerminalView.tsx @@ -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'; @@ -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(); @@ -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(); } } else if (webglDetachTimer === undefined) { webglDetachTimer = window.setTimeout(() => { diff --git a/src/components/TerminalView.webgl.client.test.tsx b/src/components/TerminalView.webgl.client.test.tsx new file mode 100644 index 000000000..1e03d96eb --- /dev/null +++ b/src/components/TerminalView.webgl.client.test.tsx @@ -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()), + 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()), + 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( + () => ( + + ), + 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); +}); diff --git a/src/lib/terminalFitManager.client.test.tsx b/src/lib/terminalFitManager.client.test.tsx index a504263a3..143baf1b2 100644 --- a/src/lib/terminalFitManager.client.test.tsx +++ b/src/lib/terminalFitManager.client.test.tsx @@ -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); + }); +}); diff --git a/src/lib/terminalFitManager.ts b/src/lib/terminalFitManager.ts index e07e05b1e..4e786b8e2 100644 --- a/src/lib/terminalFitManager.ts +++ b/src/lib/terminalFitManager.ts @@ -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); + }); } diff --git a/src/store/agentHookStatus.test.ts b/src/store/agentHookStatus.test.ts index e709ddc9d..1b777e118 100644 --- a/src/store/agentHookStatus.test.ts +++ b/src/store/agentHookStatus.test.ts @@ -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'); diff --git a/src/store/agentHookStatus.ts b/src/store/agentHookStatus.ts index e8b9aad4e..2f56a2bfb 100644 --- a/src/store/agentHookStatus.ts +++ b/src/store/agentHookStatus.ts @@ -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; @@ -96,8 +96,11 @@ function clearTimer(timers: Map>, 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)); diff --git a/src/store/background-tasks.client.test.tsx b/src/store/background-tasks.client.test.tsx index 6839c8d06..421e91511 100644 --- a/src/store/background-tasks.client.test.tsx +++ b/src/store/background-tasks.client.test.tsx @@ -257,11 +257,8 @@ it('returns for a new turn after an idle re-report', () => { expect(isTaskBackgrounded('one')).toBe(false); }); -it.each([ - ['waiting', 'PermissionRequest'], - ['working', 'UserPromptSubmit'], -] as const)('stays in the back when a %s hook claim goes stale', (state, event) => { - hook(state, event); +it('stays in the back when a working hook claim goes stale', () => { + hook('working', 'UserPromptSubmit'); sendTaskToBack('one'); vi.advanceTimersByTime(AGENT_HOOK_STALE_MS); expect(getAgentHookStatus('one-agent')).toBeNull(); @@ -272,6 +269,19 @@ it.each([ expect(isTaskBackgrounded('one')).toBe(false); }); +it('stays in the back while a sent-back wait goes unanswered', () => { + hook('waiting', 'PermissionRequest'); + sendTaskToBack('one'); + vi.advanceTimersByTime(AGENT_HOOK_STALE_MS * 2); + expect(getAgentHookStatus('one-agent')?.state).toBe('waiting'); + expect(isTaskBackgrounded('one')).toBe(true); + + // A new ask still wakes it. + vi.advanceTimersByTime(1_000); + hook('waiting', 'PermissionRequest'); + expect(isTaskBackgrounded('one')).toBe(false); +}); + it('returns when terminal activity finishes after an unchanged hook expiry', () => { hook('working', 'UserPromptSubmit'); sendTaskToBack('one'); diff --git a/src/store/background-tasks.ts b/src/store/background-tasks.ts index 5c443d4a5..8032918e2 100644 --- a/src/store/background-tasks.ts +++ b/src/store/background-tasks.ts @@ -59,7 +59,7 @@ interface AgentActivity { /** Facts derived from the hook status or, once it is gone, from output heuristics. */ activity: string; hooked: boolean; - /** When the working/waiting claim was reported; only such claims expire. */ + /** When the working claim was reported; only such claims expire. */ claimAt?: number; } @@ -87,7 +87,7 @@ function agentActivity(agentId: string, questions: readonly TaskOpenQuestion[]): hookMarker(hook), ]), hooked: hook !== null, - claimAt: hook && hook.state !== 'done' ? hook.updatedAt : undefined, + claimAt: hook?.state === 'working' ? hook.updatedAt : undefined, }; } diff --git a/src/store/taskStatus.test.ts b/src/store/taskStatus.test.ts index d61bd3e58..ea1bd6785 100644 --- a/src/store/taskStatus.test.ts +++ b/src/store/taskStatus.test.ts @@ -111,6 +111,7 @@ import { clearAgentActivity, } from './taskStatus'; import { + AGENT_HOOK_STALE_MS, applyAgentActivityObservation, applyAgentHookEvent, getAgentHookStatus, @@ -1696,6 +1697,33 @@ describe('hook-reported agent status', () => { expect(getTaskOpenQuestion('task-1')?.agentId).toBe('agent-1'); }); + it('reports a failed turn as an error until the next turn starts', () => { + hook('agent-1', 'working'); + hook('agent-1', 'done', 'StopFailure'); + expect(getTaskAttentionState('task-1')).toBe('error'); + expect(taskNeedsAttention('task-1')).toBe(true); + + // Claude's idle reminder must not quietly clear the failure. + vi.advanceTimersByTime(60_000); + hook('agent-1', 'done', 'Notification'); + expect(getTaskAttentionState('task-1')).toBe('error'); + + hook('agent-1', 'working'); + expect(getTaskAttentionState('task-1')).toBe('active'); + }); + + it('does not report an error for a turn that finished normally', () => { + hook('agent-1', 'done', 'Stop'); + expect(getTaskAttentionState('task-1')).toBe('idle'); + }); + + it('keeps needs_input for a prompt left unanswered past the stale window', () => { + hook('agent-1', 'waiting', 'PermissionRequest'); + vi.advanceTimersByTime(AGENT_HOOK_STALE_MS * 2); + expect(getTaskAttentionState('task-1')).toBe('needs_input'); + expect(getTaskOpenQuestion('task-1')?.agentId).toBe('agent-1'); + }); + it('preserves hook state on reattachment and clears it on an actual new launch or teardown', () => { hook('agent-1', 'waiting', 'PermissionRequest'); markAgentSpawned('agent-1'); diff --git a/src/store/taskStatus.ts b/src/store/taskStatus.ts index 281926133..809ba11dd 100644 --- a/src/store/taskStatus.ts +++ b/src/store/taskStatus.ts @@ -1033,12 +1033,15 @@ export function getReadyLabel(taskId: string): string { return 'Ready'; } +/** An agent exited with an error, or a running one reported that its turn + * failed (API error, rate limit) and stopped working with its process still up. */ function hasTaskAgentError(taskId: string): boolean { const task = store.tasks[taskId]; if (!task) return false; return task.agentIds.some((id) => { const agent = store.agents[id]; if (agent && isAgentChat(task, id)) return !!agent.chatState?.error; + if (agent?.status === 'running') return getAgentHookStatus(id)?.event === 'StopFailure'; if (agent?.status !== 'exited') return false; return agent.exitCode !== 0 || agent.signal !== null; });