mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 14:39:42 +02:00
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) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
+110
-26
@@ -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<string, ApprovalItem>();
|
||||
private recaptureTimers = new Map<string, ReturnType<typeof setTimeout>>();
|
||||
/** Post-capture timers per item id (re-capture + the delayed staleness check). */
|
||||
private itemTimers = new Map<string, ReturnType<typeof setTimeout>[]>();
|
||||
/** Capture callbacks kept for answer-time re-verification; dropped on remove. */
|
||||
private captures = new Map<string, () => 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 });
|
||||
}
|
||||
|
||||
@@ -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');
|
||||
}
|
||||
},
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user