From 7fc4784d0f652843acf69a69503065e522874dbc Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Thu, 20 Aug 2026 12:18:16 +0200 Subject: [PATCH] fix(approvals): clear the red tab alert when a dialog is answered in the terminal Confirming an AskUserQuestion left its tab flowing red for the rest of the turn (owner report: ~8 minutes on a running session, with no dialog anywhere on screen). Two separate bugs, both live-verified. The re-capture erased the evidence the staleness check runs on. Claude Code fires the Notification behind the dialog (measured 6-7s on v2.1.237, documented up to ~30s), so the 600ms re-capture routinely lands on a frame the user has ALREADY answered, parses nothing, and applyCapture overwrote item.options with undefined. A MISSING options is how "we never could read this dialog" is expressed, and those items stay answerable by design, so a cleared field was indistinguishable from a never-parsed one and the item became permanently unsweepable: it survived every GET /api/approvals and every page reload, cleared only on `stop`, and still accepted an answer, sending a bare `1` into a composer with no dialog under it. applyCapture is now ADD-ONLY for options. Nothing ran the staleness check while a page was open. It lived only in GET /api/approvals, which seedApprovals() calls on init and reconnect, so `stop` was the first thing that ever cleared an answered dialog. The `working` signal now runs the pane-VERIFIED variant (resolveIfDialogGone -> verifyStillAnswerable): the heuristic only decides when to look, the screen decides the outcome, so the existing "working can flap" rule is respected. A frame that parses no options is now conclusive in two cases, and only those, so an unreadable capture still keeps the alert: the item once parsed options, or the frame shows Claude actively running a turn. A modal dialog BLOCKS the turn, so the two cannot coexist - measured, a live-dialog frame carries neither the elapsed-timer spinner nor the "esc to interrupt" footer, which the dialog replaces with "Enter to select". That second signal is reached by a delayed staleness pass (3s) scheduled alongside the re-capture, which closes the late-hook case where the prompt is answered before the hook lands: nothing ever parses, `stop` may have gone by already, and the alert outlived reloads until the 12h TTL. The pass is deliberately later than RECAPTURE_DELAY_MS, whose whole reason for existing is that the hook can beat Ink to the screen. Frontend: _onHookElicitationComplete cleared only the elicitation entry, but an AskUserQuestion arrives as permission_prompt, so it was clearing the wrong alert; it now clears both, matching the server's kind-agnostic APPROVAL_RESOLVING_EVENTS. Verified end to end on an isolated beta instance, not just in unit tests: before, resolution could only come from the stop route (approval:resolved always immediately preceding hook:stop); after, it arrives from the new paths, and a simulated late hook resolves at +3.12s with no stop, no working signal and no GET, while the pane is still working. Tests use frames captured off a live pane and each new one was confirmed to fail against the old behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- src/web/approval-inbox.ts | 136 ++++++++++++++---- src/web/public/settings-ui.js | 9 ++ src/web/session-listener-wiring.ts | 14 +- test/approval-inbox.test.ts | 213 +++++++++++++++++++++++++++++ 5 files changed, 344 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e6efbd1c..9867dfdd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -212,7 +212,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Hook events**: Claude Code hooks trigger via `/api/hook-event`. Key events: `permission_prompt`, `elicitation_dialog`, `elicitation_complete`, `elicitation_response`, `idle_prompt`, `stop`, `teammate_idle`, `task_completed`. See `src/hooks-config.ts`; upstream hook semantics mirrored in `docs/claude-code-hooks-reference.md`. ⚠️ **Every claude session INSTALLS the hooks block into its workspace** (`applyWorkspaceHooks` in hooks-config.ts → `ensureCodemanHooks`, an add-only merge that keeps a user's own handlers), from EVERY claude create path — both interactive routes, cron fires, legacy scheduled runs, the plan-orchestrator one-shots — and from `restoreMuxSessions()` for sessions recovered on server start (that boot sweep skips a workspace that no longer exists, so a deleted repo with a surviving tmux session is never resurrected as an empty dir). Before 2026-08-15 hooks were written ONLY when Codeman created the case DIRECTORY, so a linked case / cloned repo — where most sessions actually run — had no hooks at all and every hook-driven surface was silently dead there: an AskUserQuestion dialog blocked the pane while the tab and the phone overview both read a calm `idle`, with no Approvals Inbox item, no push, no definitive `stop`/`idle_prompt` for respawn and no `stop`/`blocked` for the wait endpoints. The escape hatch is the synced `workspaceHooksEnabled` setting (App Settings → Agents & CLIs → Claude, **default ON**); OFF restores the old behavior, where a Codeman block that is already there is still refreshed when stale (COD-91) but one is never added. ⚠️ Route the decision through `applyWorkspaceHooks` rather than calling `ensureCodemanHooks` at a new site, or the setting silently stops applying to that path. ⚠️ Claude Code RE-READS `settings.local.json`, so an already-running session starts firing hooks without a restart (measured 2026-08-15) — and the notification for a blocking dialog is delayed by Claude Code (~30s), so the alert trails the dialog. ⚠️ An AskUserQuestion / plan-selection dialog arrives as **`permission_prompt`**, not `elicitation_dialog` (that one is MCP elicitation), so it renders as the RED "needs you" alert, not the yellow idle one. -**Approvals Inbox** (cross-session queue of prompts waiting on a human; `approvalsInboxEnabled`, SYNCED, default OFF: every surface is opt-in; only the store and answer endpoints run regardless, so flipping it ON shows anything already pending): `web/approval-inbox.ts` is a `sessionWaits`-style singleton fed by `/api/hook-event`, holding at most ONE item per session (a new prompt supersedes), claude-mode only, in-memory. Cards are answered via `POST /api/approvals/:id/answer`, which sends a digit / Esc / idle-prompt text through `writeViaMux` (menu answers never carry `\r`). ⚠️ `option` digits are accepted ONLY when they match options parsed from the captured pane frame, and the answer path RE-CAPTURES the pane first (a dialog that no longer parses on screen means the keystroke would land in the composer, so refuse with 409). ⚠️ Resolution on the heuristic `working` signal is restricted to `idle` items; permission/question items clear only on definitive signals (`stop`, `elicitation_complete`/`elicitation_response`, exit/delete, answer, supersede, 12h TTL). ⚠️ **Viewing a session ACKNOWLEDGES its idle item, it does not resolve it** (`POST /api/approvals/session/:sessionId/viewed` → `acknowledgedAt` → `approval:updated`): the item stays pending (still answerable, still Read My Mind context) and only stops arming the yellow tab alert. That flag is what makes the clear durable, since the view-clears-idle rule used to live in one browser's memory and `seedApprovals()` re-armed the alert on the next reload while other devices never heard about it at all; the local half is `markIdleAlertSeen()` (app.js), called from BOTH `selectSession` paths, including the already-active early return, where a click could otherwise never clear the alert. ⚠️ **Only a HUMAN opening a session acknowledges**: `selectSession(id, { auto: true })` marks the three selections the APP makes (boot restore, a solo window opening its target, the fallback after the active session is closed) and skips the acknowledgement, so a page load cannot silently spend an alert the user never saw. The flag defaults to user-initiated, so an untagged call site fails toward acknowledging rather than toward an alert nothing can clear; `test/session-select-ack-gate.test.ts` pins both the gate and the tagged call sites. Idle-only by construction (`acknowledge()` defaults to `['idle']`): looking at a permission/question dialog does not answer it. ⚠️ Same rule on the input path: `_ackDelivery` (app.js) spends the IDLE alert only, via that same `markIdleAlertSeen()`. It used to `clearPendingHooks(sessionId)` with no kind, so one keystroke wiped a RED alert on that device while the dialog was still up, the other devices stayed red, and a reload re-seeded it. ⚠️ Claude Code fires no "permission answered" hook (only `elicitation_complete`/`elicitation_response`, i.e. the question flavor), so an answered-in-the-terminal dialog would otherwise sit pending until `stop`: `GET /api/approvals` therefore runs a **staleness sweep** over the caller's own items via `verifyStillAnswerable()`, which is deliberately the conservative check the answer path uses (only an item whose ORIGINAL frame parsed options can be dropped, so an unreadable capture keeps the alert rather than losing a live one). The frontend seeds from `GET /api/approvals` in `handleInit` **regardless of the setting**: the seed re-arms the tab-alert state machine (`setPendingHook`) unconditionally, and only populating `this.approvals` (the inbox surfaces) is gated — seeding used to be gated wholesale, which left a reloaded page with NO red tab while a permission dialog sat blocking a session (2026-08-15); `_onApprovalResolved` clears the pending-hook alert unconditionally for the same reason. ⚠️ The red/yellow tab alert itself is a STEADY border/background/dot with a pulse on top: the original keyframes swung to transparent at 0%/100%, so half of every cycle looked like a normal tab. Push Approve/Deny buttons stay gated on the setting (`sendPushNotifications` strips `actions`/`approvalId` when OFF) and are answered from `sw.js` directly so they work with no tab open. Surfaces (all gated on the setting): header bell (marker-hidden until count > 0, phones never show it) + drawer (`approvals-ui.js`), phone overview NEEDS YOU answer strips (`mobile-overview.js`). Design: `docs/approvals-inbox-plan.md`. +**Approvals Inbox** (cross-session queue of prompts waiting on a human; `approvalsInboxEnabled`, SYNCED, default OFF: every surface is opt-in; only the store and answer endpoints run regardless, so flipping it ON shows anything already pending): `web/approval-inbox.ts` is a `sessionWaits`-style singleton fed by `/api/hook-event`, holding at most ONE item per session (a new prompt supersedes), claude-mode only, in-memory. Cards are answered via `POST /api/approvals/:id/answer`, which sends a digit / Esc / idle-prompt text through `writeViaMux` (menu answers never carry `\r`). ⚠️ `option` digits are accepted ONLY when they match options parsed from the captured pane frame, and the answer path RE-CAPTURES the pane first (a dialog that no longer parses on screen means the keystroke would land in the composer, so refuse with 409). ⚠️ Resolution on the heuristic `working` signal ALONE is restricted to `idle` items; a permission/question item gets the pane-VERIFIED variant on that same signal (`resolveIfDialogGone()` → `verifyStillAnswerable()`), so the heuristic only decides when to LOOK and the screen decides the outcome. That is what clears a dialog answered in the terminal mid-turn; the other definitive signals are `stop`, `elicitation_complete`/`elicitation_response`, exit/delete, answer, supersede and the 12h TTL. ⚠️ **Viewing a session ACKNOWLEDGES its idle item, it does not resolve it** (`POST /api/approvals/session/:sessionId/viewed` → `acknowledgedAt` → `approval:updated`): the item stays pending (still answerable, still Read My Mind context) and only stops arming the yellow tab alert. That flag is what makes the clear durable, since the view-clears-idle rule used to live in one browser's memory and `seedApprovals()` re-armed the alert on the next reload while other devices never heard about it at all; the local half is `markIdleAlertSeen()` (app.js), called from BOTH `selectSession` paths, including the already-active early return, where a click could otherwise never clear the alert. ⚠️ **Only a HUMAN opening a session acknowledges**: `selectSession(id, { auto: true })` marks the three selections the APP makes (boot restore, a solo window opening its target, the fallback after the active session is closed) and skips the acknowledgement, so a page load cannot silently spend an alert the user never saw. The flag defaults to user-initiated, so an untagged call site fails toward acknowledging rather than toward an alert nothing can clear; `test/session-select-ack-gate.test.ts` pins both the gate and the tagged call sites. Idle-only by construction (`acknowledge()` defaults to `['idle']`): looking at a permission/question dialog does not answer it. ⚠️ Same rule on the input path: `_ackDelivery` (app.js) spends the IDLE alert only, via that same `markIdleAlertSeen()`. It used to `clearPendingHooks(sessionId)` with no kind, so one keystroke wiped a RED alert on that device while the dialog was still up, the other devices stayed red, and a reload re-seeded it. ⚠️ Claude Code fires no "permission answered" hook (only `elicitation_complete`/`elicitation_response`, i.e. the question flavor), so an answered-in-the-terminal dialog would otherwise sit pending until `stop`: `GET /api/approvals` therefore runs a **staleness sweep** over the caller's own items via `verifyStillAnswerable()`, which is deliberately the conservative check the answer path uses (only an item whose ORIGINAL frame parsed options can be dropped, so an unreadable capture keeps the alert rather than losing a live one). ⚠️ **`applyCapture()` is therefore ADD-ONLY for `options`**: a re-capture that parses nothing must never erase a parse an earlier one found. Claude Code delays the Notification hook behind the dialog (measured 6s, documented ~30s), so the 600ms re-capture routinely lands on a frame the user has ALREADY answered; clearing the field there made the item permanently unsweepable, because `verifyStillAnswerable()` reads a MISSING `options` as "we never could read this dialog" and keeps such items answerable by design. The red "needs you" then survived every sweep AND every page reload, went away only on `stop` (2026-08-20: a confirmed question left a tab flowing red for ~8 minutes while the turn ran on), and the stale card still accepted an answer, typing a bare `1` into a composer with no dialog under it. Pinned by `test/approval-inbox.test.ts`. ⚠️ A frame that parses no options is CONCLUSIVE in exactly two cases, and the second one closes the late-hook hole: the item once parsed options (they cannot vanish while the dialog is up), or the frame shows Claude actively running a turn. A modal dialog BLOCKS the turn, so the two cannot coexist — measured on v2.1.237, a live-dialog frame carries neither the `… (13s` timer NOR the `esc to interrupt` footer, which the dialog replaces with `Enter to select · ↑/↓ to navigate · Esc to cancel`. Anything else stays answerable, so an unreadable capture still keeps the alert. That second signal is reached by a delayed staleness pass (`STALE_CHECK_DELAY_MS`, 3s) scheduled alongside the re-capture, because a prompt answered BEFORE the hook lands creates an item whose FIRST capture already has no dialog in it: nothing ever parsed, `stop` may have fired already, and the alert then outlived reloads until the 12h TTL. ⚠️ That pass must stay comfortably LATER than `RECAPTURE_DELAY_MS`, whose whole reason for existing is that the hook can beat Ink to the screen — resolving inside the paint window would clear the alert for a dialog that was about to appear. The frontend seeds from `GET /api/approvals` in `handleInit` **regardless of the setting**: the seed re-arms the tab-alert state machine (`setPendingHook`) unconditionally, and only populating `this.approvals` (the inbox surfaces) is gated — seeding used to be gated wholesale, which left a reloaded page with NO red tab while a permission dialog sat blocking a session (2026-08-15); `_onApprovalResolved` clears the pending-hook alert unconditionally for the same reason. ⚠️ The red/yellow tab alert itself is a STEADY border/background/dot with a pulse on top: the original keyframes swung to transparent at 0%/100%, so half of every cycle looked like a normal tab. Push Approve/Deny buttons stay gated on the setting (`sendPushNotifications` strips `actions`/`approvalId` when OFF) and are answered from `sw.js` directly so they work with no tab open. Surfaces (all gated on the setting): header bell (marker-hidden until count > 0, phones never show it) + drawer (`approvals-ui.js`), phone overview NEEDS YOU answer strips (`mobile-overview.js`). Design: `docs/approvals-inbox-plan.md`. **Read My Mind intent profiles** (phase 1 of `docs/readmymind-plan.md`; `readMyMindEnabled`, SYNCED, default OFF): per-CASE profiles (user-stated `goals` + the user's recent real prompts), keyed by owner + realpath(workingDir) so they survive `/clear`/respawns and multi-user scoping is structural. Capture rides the transcript (`transcript:user_prompt` from `transcript-watcher.ts`), NOT the input paths: `POST /input` sees only programmatic prompts and the WS channel is raw keystrokes. The listener lives inside `startTranscriptWatcher()`'s `if (!watcher)` block (outside it would duplicate per hook event) and is claude-only + gated on the setting per event. Store: `src/intent-store.ts` singleton, `intents.json` written 0600 tmp+rename (prompts can contain secrets; never fed to `/api/search`). Endpoints: GET/PUT/DELETE `/api/sessions/:id/intent` + POST `/api/sessions/:id/readmymind` (`readmymind-routes.ts`, ownership via `findSessionOrFail` WITH `req`; registrations stay the bare `app.('path')` shape, the endpoints.md drift scanner cannot see generics). **Phase 2 (predictor + 🧠 button)**: `readmymind-context.ts` is the PURE budgeted assembler (9 ranked sources, drop order siblings→away→workspace→tools, sections 1-4 truncate only); IO lives in `readmymind-collectors.ts` (transcript TAIL read — the live watcher keeps only a 500-char snippet — + git signals, skipped for remote-SSH cases) and the route; `readmymind-predictor.ts` reuses the AiCheckerBase spawn mechanics standalone (verdict-shaped base vs freeform JSON) as a mutable singleton routes call and tests stub. Claude-mode only (400), one in flight per session (409 CONFLICT), model = `readMyMindModel` setting defaulting to `AI_CHECK_MODEL` (opus, decided). Frontend `readmymind-ui.js`: header 🧠 marker-hidden (`btn-readmymind--hidden`) until the setting is ON; phones hide it in mobile.css and get a keyboard-accessory 🧠 key instead (ships in BOTH bar templates, revealed by the `rmm-enabled` class on the BAR element — setMode() rebuilds button innerHTML, so per-key state would be wiped; synced at init + every `applyHeaderVisibilitySettings()`). Alternate suggestions render as tappable rows that swap into the editable field without losing edits; Rethink rejects the whole shown set and carries the optional steer note (`#readMyMindSteer`, sent as `steer`, shown in ready + empty-result phases, cleared on each open). Suggestions render via value/`textContent` ONLY and Send/Insert go through `POST /input` (server-side, so the sendEnterKey/local-echo trap does not apply) — nothing auto-sends, ever. User guide: `docs/readmymind.md`. diff --git a/src/web/approval-inbox.ts b/src/web/approval-inbox.ts index ba0dd838..7712bb30 100644 --- a/src/web/approval-inbox.ts +++ b/src/web/approval-inbox.ts @@ -22,14 +22,14 @@ * - Acknowledgement (`acknowledge()`, idle items only) is NOT resolution: the * item stays pending, it just stops arming the tab alert on every client. * - * @dependencies utils (stripAnsi) + * @dependencies utils (stripAnsi, CLAUDE_WORKING_LINE_PATTERN) * @consumedby web/routes/hook-event-routes (notePrompt/resolve), web/routes/approval-routes, * web/session-listener-wiring (working/exit resolution), web/server (emit callbacks + stop) * * @module web/approval-inbox */ -import { stripAnsi } from '../utils/index.js'; +import { stripAnsi, CLAUDE_WORKING_LINE_PATTERN } from '../utils/index.js'; // ─── Types ─────────────────────────────────────────────────────────────────── @@ -106,6 +106,12 @@ const ITEM_TTL_MS = 12 * 60 * 60 * 1000; * single delayed re-capture picks up the frame the immediate capture missed. */ const RECAPTURE_DELAY_MS = 600; +/** + * Delayed staleness pass for the late-hook case (see notePrompt). Comfortably + * clear of RECAPTURE_DELAY_MS so a dialog Ink has not painted yet is never + * mistaken for one that is gone. + */ +const STALE_CHECK_DELAY_MS = 3000; /** Context kept per item: enough for a dialog plus a few lines above it. */ const MAX_CONTEXT_CHARS = 4000; const MAX_CONTEXT_LINES = 30; @@ -195,7 +201,8 @@ export function parseDialogOptions(context: string | undefined): ApprovalOption[ export class ApprovalInbox { /** Keyed by sessionId; the one-active-item-per-session invariant lives here. */ private items = new Map(); - private recaptureTimers = new Map>(); + /** Post-capture timers per item id (re-capture + the delayed staleness check). */ + private itemTimers = new Map[]>(); /** Capture callbacks kept for answer-time re-verification; dropped on remove. */ private captures = new Map string | null>(); private seq = 0; @@ -229,31 +236,55 @@ export class ApprovalInbox { if (args.capture) this.captures.set(args.sessionId, args.capture); this.onPending?.(item); if (args.capture && !this.stopped) { - const timer = setTimeout(() => { - this.recaptureTimers.delete(item.id); - // Only update the item if it is still the live one for the session. - if (this.items.get(args.sessionId)?.id !== item.id) return; + // Pass 1 (600ms): enrich the card with the painted frame. + this.scheduleForItem(item, RECAPTURE_DELAY_MS, () => { this.applyCapture(item, args.capture); this.onUpdated?.(item); - }, RECAPTURE_DELAY_MS); - this.recaptureTimers.set(item.id, timer); + }); + // Pass 2: the late-hook staleness check. Claude Code fires the + // Notification behind the dialog, so a prompt answered before the hook + // lands creates an item for a dialog that is ALREADY gone: nothing ever + // parsed, so the "options vanished" test can never fire, `stop` may have + // gone by already, and the red alert then outlived reloads until the 12h + // TTL. This pass re-reads the pane and resolves when the frame proves no + // dialog is up. Deliberately LATER than the re-capture, whose whole + // reason for existing is that Ink may not have painted the dialog yet: + // resolving inside that window could clear the alert for a dialog that + // was about to appear. + this.scheduleForItem(item, STALE_CHECK_DELAY_MS, () => { + this.verifyStillAnswerable(item.id); + }); } return item; } /** * Answer-time guard: re-capture the pane and check the dialog is still on - * screen before keystrokes are sent at it. Only conclusive when the ORIGINAL - * frame parsed options: if a fresh capture then parses none, the dialog is - * gone (answered in the terminal moments ago), so the item resolves and the - * answer must be refused, because the digit would land in whatever now has - * focus. Unparseable-from-the-start items stay answerable (approve/deny - * only), same risk the terminal user already carries. + * screen before keystrokes are sent at it. If the dialog is gone (answered in + * the terminal moments ago) the item resolves and the answer is refused, + * because the digit would land in whatever now has focus. + * + * A fresh frame that parses NO options is conclusive in two cases, and only + * those; anything else stays answerable, so an unreadable capture keeps the + * alert rather than losing a live dialog: + * + * 1. The item HAD parsed options. They cannot vanish while the dialog is up. + * 2. The frame shows Claude actively running a turn. A modal dialog BLOCKS + * the turn, so a working line and a dialog cannot coexist — measured on + * v2.1.237: a live-dialog frame carries neither the `… (13s` timer nor + * even the `esc to interrupt` footer, which the dialog replaces with + * `Enter to select · ↑/↓ to navigate · Esc to cancel`. + * + * Case 2 is what closes the late-hook hole. Claude Code fires the + * Notification behind the dialog, so a prompt answered before the hook lands + * produces an item whose FIRST capture already has no dialog in it — never + * parsed, so case 1 can never fire, and the red alert then outlived even + * `stop` (which had already fired) and survived reloads until the 12h TTL. */ verifyStillAnswerable(id: string): boolean { const item = this.getById(id); if (!item) return false; - if (item.kind === 'idle' || !item.options) return true; + if (item.kind === 'idle') return true; const capture = this.captures.get(item.sessionId); if (!capture) return true; let raw: string | null = null; @@ -266,14 +297,39 @@ export class ApprovalInbox { if (!context) return true; const options = parseDialogOptions(context); if (!options) { - this.remove(item, 'resolved_in_terminal'); - return false; + if (item.options || CLAUDE_WORKING_LINE_PATTERN.test(context)) { + this.remove(item, 'resolved_in_terminal'); + return false; + } + return true; // never parsed and the pane is not visibly working: unreadable, not gone } item.context = context; item.options = options; return true; } + /** + * "This session's pane started moving again": re-verify its pending DIALOG + * item against the screen and resolve it if the dialog is gone. + * + * The staleness check itself lived only in `GET /api/approvals`, which + * nothing calls while a page is open (`seedApprovals()` runs on init and + * reconnect), so a dialog answered in the terminal kept its red tab alert for + * the whole rest of the turn. The `working` signal is exactly the moment an + * answer lands, and routing it through `verifyStillAnswerable` is what makes + * it safe to act on for a permission/question item: `working` is heuristic + * and can flap, but it only decides WHEN to look — the pane decides the + * outcome, and an unreadable capture keeps the alert. + * + * Cheap by construction: a Map miss unless a dialog item is actually pending, + * and the item is gone after the first successful resolve. + */ + resolveIfDialogGone(sessionId: string): void { + const item = this.getForSession(sessionId); + if (!item || item.kind === 'idle') return; + this.verifyStillAnswerable(item.id); + } + /** Pending item for a session, TTL-checked. */ getForSession(sessionId: string): ApprovalItem | undefined { const item = this.items.get(sessionId); @@ -359,12 +415,27 @@ export class ApprovalInbox { /** Clear all timers (shutdown/tests). Items become inert; no events fire after this. */ stop(): void { this.stopped = true; - for (const timer of this.recaptureTimers.values()) clearTimeout(timer); - this.recaptureTimers.clear(); + for (const timers of this.itemTimers.values()) for (const timer of timers) clearTimeout(timer); + this.itemTimers.clear(); this.items.clear(); this.captures.clear(); } + /** + * Run `fn` after `delayMs` if the item is still the live one for its session, + * tracking the timer so `remove()`/`stop()` can cancel it. + */ + private scheduleForItem(item: ApprovalItem, delayMs: number, fn: () => void): void { + const timer = setTimeout(() => { + const timers = this.itemTimers.get(item.id)?.filter((t) => t !== timer) ?? []; + if (timers.length > 0) this.itemTimers.set(item.id, timers); + else this.itemTimers.delete(item.id); + if (this.items.get(item.sessionId)?.id !== item.id) return; + fn(); + }, delayMs); + this.itemTimers.set(item.id, [...(this.itemTimers.get(item.id) ?? []), timer]); + } + private applyCapture(item: ApprovalItem, capture?: () => string | null): void { if (!capture) return; let raw: string | null = null; @@ -377,17 +448,30 @@ export class ApprovalInbox { if (!context) return; item.context = context; // Idle prompts are not dialogs; never offer digit answers for them. - if (item.kind !== 'idle') item.options = parseDialogOptions(context); + if (item.kind === 'idle') return; + const options = parseDialogOptions(context); + // ⚠️ ADD-ONLY: a re-capture that parses NOTHING must never erase options a + // previous capture found. Claude Code delays the Notification hook behind + // the dialog (measured 6s here, up to ~30s), so the 600ms re-capture very + // often lands AFTER the user has already answered in the terminal, on a + // frame with no dialog in it. Clearing the field there was the whole bug: + // `verifyStillAnswerable` reads a MISSING `options` as "never parsed" and + // keeps such an item answerable by design, so a cleared field made the item + // permanently unsweepable — the red "needs you" alert then survived every + // `GET /api/approvals` and every page reload and only went away on `stop` + // (owner report 2026-08-20: a confirmed question left a tab flowing red for + // ~8 minutes while the turn ran on), and the stale card still accepted an + // answer, typing a bare `1` into a composer with no dialog under it. + // Keeping the parse means a later capture is CONCLUSIVE: options present + + // fresh frame without them == answered in the terminal. + if (options) item.options = options; } private remove(item: ApprovalItem, resolution: ApprovalResolution): void { this.items.delete(item.sessionId); this.captures.delete(item.sessionId); - const timer = this.recaptureTimers.get(item.id); - if (timer) { - clearTimeout(timer); - this.recaptureTimers.delete(item.id); - } + for (const timer of this.itemTimers.get(item.id) ?? []) clearTimeout(timer); + this.itemTimers.delete(item.id); if (!this.stopped) { this.onResolved?.({ id: item.id, sessionId: item.sessionId, kind: item.kind, resolution }); } diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index e91a2806..2190263d 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -41,8 +41,17 @@ Object.assign(CodemanApp.prototype, { _onHookElicitationComplete(data) { // Question answered in the terminal: clear the action alert without // waiting for `stop` (the turn may keep running for a long time). + // ⚠️ BOTH action kinds, matching the server's APPROVAL_RESOLVING_EVENTS, + // which resolves a session's pending item whatever its kind. An + // AskUserQuestion dialog arrives as `permission_prompt` (only MCP + // elicitation is `elicitation_dialog`), so clearing just the elicitation + // entry left the red alert armed on exactly the dialog these events are + // most often about. Normally the server's `approval:resolved` broadcast + // clears it too; this is the path that still works when the store holds no + // item for the session (restart, superseded). if (data.sessionId) { this.clearPendingHooks(data.sessionId, 'elicitation_dialog'); + this.clearPendingHooks(data.sessionId, 'permission_prompt'); } }, diff --git a/src/web/session-listener-wiring.ts b/src/web/session-listener-wiring.ts index f52a91b8..1d619d2b 100644 --- a/src/web/session-listener-wiring.ts +++ b/src/web/session-listener-wiring.ts @@ -219,10 +219,18 @@ export function createSessionListeners(session: Session, deps: SessionListenerDe // An idle-prompt inbox item means "composer is waiting"; any working // transition means input arrived, so the item is moot. ONLY the idle // kind: `working` is heuristic and can flap mid-turn, so clearing a - // pending permission/question dialog on it would false-clear real - // approvals (those resolve via stop / elicitation hooks / answer-time - // re-capture instead). + // pending permission/question dialog on the signal ALONE would + // false-clear real approvals. approvalInbox.resolveForSession(session.id, 'resolved_in_terminal', ['idle']); + // A permission/question dialog gets the pane-VERIFIED variant instead: + // the signal only decides when to look, `verifyStillAnswerable` re-reads + // the screen and resolves only when the dialog is really gone. Without + // this, answering a dialog in the terminal left its red "needs you" alert + // armed for the rest of the turn, because the only other staleness check + // lives in `GET /api/approvals` and nothing calls that while a page is + // open. `stop` was the first thing to clear it, which on a long turn is + // minutes away. + approvalInbox.resolveIfDialogGone(session.id); deps.broadcast(SseEvent.SessionWorking, { id: session.id }); // Full state ride-along: the home screens sort the running group on // lastSubmitAt, and without this the browser keeps the stamp it loaded diff --git a/test/approval-inbox.test.ts b/test/approval-inbox.test.ts index 87f138c2..e5524159 100644 --- a/test/approval-inbox.test.ts +++ b/test/approval-inbox.test.ts @@ -40,6 +40,39 @@ const ASK_USER_QUESTION_FRAME = [ 'Enter to select · ↑/↓ to navigate · Esc to cancel', ].join('\n'); +// Both frames captured off a live Claude Code v2.1.237 pane. They are the +// discriminator behind the late-hook resolution: a modal dialog BLOCKS the +// turn, so a working line and a dialog cannot coexist. Note the dialog frame +// carries no `esc to interrupt` footer either — the dialog replaces it. +const LIVE_DIALOG_FRAME = [ + '● Bash(sleep 12; echo "slept 12s")', + ' ⎿ slept 12s', + '────────────────────────────────────────', + ' ☐ Proceed', + '', + 'Proceed?', + '', + '❯ 1. Yes', + ' Go ahead and proceed.', + ' 2. No', + ' Do not proceed.', + ' 3. Type something.', + '────────────────────────────────────────', + ' 4. Chat about this', + '', + 'Enter to select · ↑/↓ to navigate · Esc to cancel', +].join('\n'); + +const WORKING_FRAME = [ + '● Bash(sleep 25)', + ' ⎿ Tip: Use git worktrees to run multiple Claude sessions in parallel.', + '✢ Clauding… (13s · ↓ 1.4k tokens)', + '────────────────────────────────────────', + '❯', + '────────────────────────────────────────', + ' ⏵⏵ bypass permissions on (shift+tab to cycle) · esc to interrupt · ← for agents', +].join('\n'); + function collect(inbox: ApprovalInbox) { const pending: ApprovalItem[] = []; const updated: ApprovalItem[] = []; @@ -333,6 +366,186 @@ describe('ApprovalInbox', () => { }); }); + describe('answered-in-the-terminal staleness', () => { + // Claude Code delays the Notification hook behind the dialog (measured 6s + // on v2.1.237, documented up to ~30s), so the 600ms re-capture routinely + // lands on a frame the user has ALREADY answered. Erasing `options` there + // made the item permanently unsweepable, because a missing `options` is how + // "we never could read this dialog" is expressed, and such items stay + // answerable on purpose. The red tab alert then survived every + // `GET /api/approvals` and every reload, clearing only on `stop`. + it('a re-capture taken after the answer does not erase parsed options', () => { + const { updated } = collect(inbox); + let frame = ASK_USER_QUESTION_FRAME; + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => frame, + }); + expect(item.options).toHaveLength(5); + + // Answered in the terminal before the re-capture fires. + frame = "● User answered Claude's questions:\n ⎿ · Which color do you prefer? → Red\n\n✶ Cooking… (6s)"; + vi.advanceTimersByTime(600); + + expect(updated).toHaveLength(1); + expect(inbox.getById(item.id)?.context).toContain('User answered'); + expect(inbox.getById(item.id)?.options).toHaveLength(5); + // ...which keeps the staleness check conclusive instead of inconclusive. + expect(inbox.verifyStillAnswerable(item.id)).toBe(false); + expect(inbox.getById(item.id)).toBeUndefined(); + }); + + it('resolveIfDialogGone resolves a dialog answered in the terminal', () => { + const { resolved } = collect(inbox); + let frame = PERMISSION_FRAME; + const item = inbox.notePrompt({ sessionId: 's1', sessionName: 'w1', kind: 'permission', capture: () => frame }); + frame = 'the dialog is gone, claude is typing'; + + inbox.resolveIfDialogGone('s1'); + + expect(inbox.getById(item.id)).toBeUndefined(); + expect(resolved.at(-1)).toMatchObject({ id: item.id, resolution: 'resolved_in_terminal' }); + }); + + it('resolveIfDialogGone keeps a dialog that is still on screen', () => { + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => PERMISSION_FRAME, + }); + inbox.resolveIfDialogGone('s1'); + expect(inbox.getById(item.id)).toBeDefined(); + }); + + it('resolveIfDialogGone never touches an idle item (that is the working signal job)', () => { + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'idle', + capture: () => 'composer is empty', + }); + inbox.resolveIfDialogGone('s1'); + expect(inbox.getById(item.id)).toBeDefined(); + }); + + // The late-hook hole: Claude Code fires the Notification behind the dialog, + // so a prompt answered before the hook lands produces an item whose FIRST + // capture already has no dialog in it. Nothing ever parsed, so "options + // vanished" can never fire, and `stop` had already gone by too — the red + // alert then survived reloads until the 12h TTL. + it('resolves an item that never parsed options once the pane is visibly working', () => { + const { resolved } = collect(inbox); + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => WORKING_FRAME, + }); + expect(item.options).toBeUndefined(); + + expect(inbox.verifyStillAnswerable(item.id)).toBe(false); + expect(inbox.getById(item.id)).toBeUndefined(); + expect(resolved.at(-1)).toMatchObject({ id: item.id, resolution: 'resolved_in_terminal' }); + }); + + it('keeps an unreadable dialog answerable when the pane is NOT visibly working', () => { + // The conservative rule this fix must not loosen: no options and no proof + // the turn is running means "we cannot read it", not "it is gone". + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => 'some dialog shape we cannot parse', + }); + expect(item.options).toBeUndefined(); + expect(inbox.verifyStillAnswerable(item.id)).toBe(true); + expect(inbox.getById(item.id)).toBeDefined(); + }); + + it('a real live-dialog frame carries no working line, so it is never false-resolved', () => { + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => LIVE_DIALOG_FRAME, + }); + expect(item.options).toHaveLength(4); + expect(inbox.verifyStillAnswerable(item.id)).toBe(true); + expect(inbox.getById(item.id)).toBeDefined(); + }); + + it('resolveIfDialogGone clears a late-hook item on the next working signal', () => { + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => WORKING_FRAME, + }); + inbox.resolveIfDialogGone('s1'); + expect(inbox.getById(item.id)).toBeUndefined(); + }); + + it('the delayed staleness pass resolves a late-hook item with no working signal needed', () => { + const { resolved } = collect(inbox); + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => WORKING_FRAME, + }); + expect(item.options).toBeUndefined(); + + vi.advanceTimersByTime(600); // re-capture: enrichment only, never resolves + expect(inbox.getById(item.id)).toBeDefined(); + + vi.advanceTimersByTime(2400); // the delayed staleness pass + expect(inbox.getById(item.id)).toBeUndefined(); + expect(resolved.at(-1)).toMatchObject({ id: item.id, resolution: 'resolved_in_terminal' }); + }); + + it('a dialog Ink paints late is NOT resolved by the delayed pass', () => { + // The paint race the re-capture exists for: the hook can beat Ink to the + // screen. Resolving inside that window would clear the alert for a dialog + // that was about to appear, so the frame is what decides, every time. + let frame = WORKING_FRAME; + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => frame, + }); + frame = LIVE_DIALOG_FRAME; // Ink finishes painting + + vi.advanceTimersByTime(600); + expect(inbox.getById(item.id)?.options).toHaveLength(4); + vi.advanceTimersByTime(2400); + expect(inbox.getById(item.id)).toBeDefined(); + }); + + it('resolving cancels both pending timers', () => { + const { updated } = collect(inbox); + const item = inbox.notePrompt({ + sessionId: 's1', + sessionName: 'w1', + kind: 'permission', + capture: () => PERMISSION_FRAME, + }); + inbox.dismiss(item.id); + vi.advanceTimersByTime(5000); + expect(updated).toHaveLength(0); + expect(inbox.listPending()).toHaveLength(0); + }); + + it('resolveIfDialogGone is a no-op for a session with nothing pending', () => { + const { resolved } = collect(inbox); + expect(() => inbox.resolveIfDialogGone('nobody')).not.toThrow(); + expect(resolved).toHaveLength(0); + }); + }); + it('stop() clears items and silences events', () => { const { resolved } = collect(inbox); inbox.notePrompt({ sessionId: 's1', sessionName: 'w1', kind: 'permission' });