From 06376053d6221f4d623fd311813957449c2b51f6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 14 Aug 2026 22:43:09 -0700 Subject: [PATCH] fix(codex): hold the fabricated permission BEL for the Codex quiet window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex "Approve for me" could still raise "Bell in · Attention requested" after #8519 because that PR debounced only the renderer's hook-derived attention notification. Main fabricates its own attention signal for the same hook: driveSyntheticTitleFromHook injected a standalone BEL alongside the "Codex - action required" OSC title on every Codex PermissionRequest, and that BEL reached onBell through the per-PTY tracker with only the 250ms terminal-bell grace — never the 1.5s Codex window. Route the fabricated BEL through the same window: the OSC title still lands immediately so the visible status is never delayed, while the BEL is held and dropped if the pause resolves inside the window. CODEX_ATTENTION_QUIET_MS now lives in shared so the two attention paths cannot drift apart again. Codex request_user_input also normalizes to `waiting`, but no auto-reviewer answers a question put to the user, so it keeps ringing inline. Fixes #13600 --- src/main/index.ts | 50 ++++++- ...synthetic-permission-bell-deferral.test.ts | 101 +++++++++++++ .../synthetic-permission-bell-deferral.ts | 48 +++++++ .../agent-completion-coordinator.ts | 3 +- .../codex-attention-quiet-window.test.ts | 135 ++++++++++++++++++ src/shared/codex-attention-quiet-window.ts | 48 +++++++ 6 files changed, 378 insertions(+), 7 deletions(-) create mode 100644 src/main/synthetic-permission-bell-deferral.test.ts create mode 100644 src/main/synthetic-permission-bell-deferral.ts create mode 100644 src/shared/codex-attention-quiet-window.test.ts create mode 100644 src/shared/codex-attention-quiet-window.ts diff --git a/src/main/index.ts b/src/main/index.ts index cafb072a8aa..973b92bb4b7 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -340,6 +340,12 @@ import { import { LocalPtyProvider } from './providers/local-pty-provider' import { KeybindingService } from './keybindings/keybinding-service' import { applyElectronProxySettings } from './network/proxy-settings' +import { + SYNTHETIC_PERMISSION_BELL, + buildSyntheticTerminalTitleFrame, + shouldDeferSyntheticPermissionBell +} from '../shared/codex-attention-quiet-window' +import { SyntheticPermissionBellDeferral } from './synthetic-permission-bell-deferral' import { preserveAgentAuthBeforeRestart } from './agent-auth-restart-preservation' import { CliInstaller } from './cli/cli-installer' import { installLinuxBareOrcaDispatcher } from './cli/linux-bare-orca-dispatcher' @@ -1504,6 +1510,7 @@ function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {}): Brow setMigrationUnsupportedPtyListener(null) // Why: stop the spinner timer here — it would fire into destroyed webContents, and per-pane teardown may never run for restored-but-untorn panes. stopAllSyntheticTitleSpinners() + syntheticPermissionBellDeferral.cancelAll() }) mainWindow = window window.on('show', resumeSyntheticTitleSpinnerTimer) @@ -1583,6 +1590,17 @@ function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {}): Brow getDashboardPopoutWindow()?.webContents.send('agentStatus:set', statusEvent) } recordAgentStateCrashBreadcrumb(payload.agentType ?? 'unknown', payload.state) + // Why: a pause that resolved itself must drop its deferred BEL even when this event + // suppresses the synthetic title entirely (#13600). + if ( + !shouldDeferSyntheticPermissionBell({ + agentType: payload.agentType, + state: payload.state, + toolName: payload.toolName + }) + ) { + syntheticPermissionBellDeferral.cancel(paneKey) + } // Why: native OSC titles miss some idle/permission frames, so inject hook-derived ones to keep the renderer title tracker in sync. const profile = getSyntheticAgentTitleProfile(payload.agentType) if ( @@ -1590,7 +1608,7 @@ function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {}): Brow shouldDriveSyntheticAgentTitleFromHook(payload.agentType, payload.state) && !suppressSyntheticCodexAutoApprovalTitle ) { - driveSyntheticTitleFromHook(paneKey, payload.state, profile) + driveSyntheticTitleFromHook(paneKey, payload, profile) } } ) @@ -1598,6 +1616,10 @@ function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {}): Brow if (mainWindow?.isDestroyed()) { return } + // Why: the pane's status is gone, so its held-back permission BEL has nothing left to announce. + if ('paneKey' in clear) { + syntheticPermissionBellDeferral.cancel(clear.paneKey) + } mainWindow?.webContents.send('agentStatus:clear', clear) getDashboardPopoutWindow()?.webContents.send('agentStatus:clear', clear) }) @@ -1812,6 +1834,7 @@ const syntheticTitleSpinnerByPaneKey = new Map< SyntheticTitleSpinnerEntry >() let syntheticTitleSpinnerTimer: ReturnType | null = null +const syntheticPermissionBellDeferral = new SyntheticPermissionBellDeferral() type ServeOptions = { json: boolean @@ -2051,9 +2074,10 @@ function resumeSyntheticTitleSpinnerTimer(): void { function driveSyntheticTitleFromHook( paneKey: string, - state: AgentStatusState, + status: { agentType?: string | null; state: AgentStatusState; toolName?: string }, profile: SyntheticAgentTitleProfile ): void { + const { agentType, state, toolName } = status const ptyId = getPtyIdForPaneKey(paneKey) if (!ptyId) { return @@ -2077,9 +2101,25 @@ function driveSyntheticTitleFromHook( stopSyntheticTitleSpinner(paneKey) const needsUserInput = state === 'blocked' || state === 'waiting' const label = needsUserInput ? profile.permissionLabel : profile.idleLabel - sendSyntheticTitle(ptyId, `\x1b]0;${label}\x07${needsUserInput ? '\x07' : ''}`, { - force: true - }) + // Why: this fabricated BEL is Orca's own attention signal, so it must clear the same Codex + // quiet window the renderer applies to the OS notification — ringing it inline let an + // "Approve for me" pause raise "Attention requested" before the auto-reviewer replied (#13600). + const { frame, deferBell } = buildSyntheticTerminalTitleFrame({ + agentType, + state, + toolName, + label + }) + sendSyntheticTitle(ptyId, frame, { force: true }) + if (deferBell) { + syntheticPermissionBellDeferral.defer(paneKey, () => { + // Why: re-resolve the PTY — the pane can be torn down or re-bound inside the quiet window. + const livePtyId = getPtyIdForPaneKey(paneKey) + if (livePtyId) { + sendSyntheticTitle(livePtyId, SYNTHETIC_PERMISSION_BELL, { force: true }) + } + }) + } } function shouldSuppressCodexAutoApprovalSyntheticTitleFromHook(args: { diff --git a/src/main/synthetic-permission-bell-deferral.test.ts b/src/main/synthetic-permission-bell-deferral.test.ts new file mode 100644 index 00000000000..6e2755dda3e --- /dev/null +++ b/src/main/synthetic-permission-bell-deferral.test.ts @@ -0,0 +1,101 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { SyntheticPermissionBellDeferral } from './synthetic-permission-bell-deferral' +import { CODEX_ATTENTION_QUIET_MS } from '../shared/codex-attention-quiet-window' + +const PANE = 'tab-1:leaf-a' +const OTHER_PANE = 'tab-1:leaf-b' + +describe('SyntheticPermissionBellDeferral', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('rings a pause the agent never resolves, one quiet window later', () => { + const deferral = new SyntheticPermissionBellDeferral() + const emit = vi.fn() + + deferral.defer(PANE, emit) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS - 1) + expect(emit).not.toHaveBeenCalled() + + vi.advanceTimersByTime(1) + expect(emit).toHaveBeenCalledTimes(1) + expect(deferral.hasPending(PANE)).toBe(false) + }) + + it('drops the BEL when the pause resolves inside the window (#13600)', () => { + const deferral = new SyntheticPermissionBellDeferral() + const emit = vi.fn() + + deferral.defer(PANE, emit) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS - 1) + expect(deferral.cancel(PANE)).toBe(true) + + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS * 4) + expect(emit).not.toHaveBeenCalled() + }) + + it('cancels only the named pane', () => { + const deferral = new SyntheticPermissionBellDeferral() + const resolved = vi.fn() + const stillWaiting = vi.fn() + + deferral.defer(PANE, resolved) + deferral.defer(OTHER_PANE, stillWaiting) + deferral.cancel(PANE) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + + expect(resolved).not.toHaveBeenCalled() + expect(stillWaiting).toHaveBeenCalledTimes(1) + }) + + it('re-arming a pane replaces its pending BEL instead of queueing a second ring', () => { + const deferral = new SyntheticPermissionBellDeferral() + const first = vi.fn() + const second = vi.fn() + + deferral.defer(PANE, first) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS - 1) + deferral.defer(PANE, second) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + + expect(first).not.toHaveBeenCalled() + expect(second).toHaveBeenCalledTimes(1) + }) + + it('reports nothing to cancel for an unarmed pane', () => { + const deferral = new SyntheticPermissionBellDeferral() + + expect(deferral.cancel(PANE)).toBe(false) + expect(deferral.hasPending(PANE)).toBe(false) + }) + + it('cancelAll drops every pending BEL at window teardown', () => { + const deferral = new SyntheticPermissionBellDeferral() + const first = vi.fn() + const second = vi.fn() + + deferral.defer(PANE, first) + deferral.defer(OTHER_PANE, second) + deferral.cancelAll() + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS * 4) + + expect(first).not.toHaveBeenCalled() + expect(second).not.toHaveBeenCalled() + expect(deferral.hasPending(OTHER_PANE)).toBe(false) + }) + + it('honors an injected window for callers that need a different cadence', () => { + const deferral = new SyntheticPermissionBellDeferral(50) + const emit = vi.fn() + + deferral.defer(PANE, emit) + vi.advanceTimersByTime(50) + + expect(emit).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/synthetic-permission-bell-deferral.ts b/src/main/synthetic-permission-bell-deferral.ts new file mode 100644 index 00000000000..ce6ec833078 --- /dev/null +++ b/src/main/synthetic-permission-bell-deferral.ts @@ -0,0 +1,48 @@ +import { CODEX_ATTENTION_QUIET_MS } from '../shared/codex-attention-quiet-window' + +/** + * Holds back the BEL that main fabricates for a Codex permission pause until the quiet window + * elapses, so an "Approve for me" pause Codex resolves itself never rings the terminal bell + * (#13600). The OSC title still lands immediately — only the attention signal waits. + * + * Cancellation is the whole point: any later non-permission state for the pane drops the pending + * BEL, and a re-armed pause replaces its predecessor rather than queueing a second ring. + */ +export class SyntheticPermissionBellDeferral { + private readonly timers = new Map>() + + constructor(private readonly quietMs: number = CODEX_ATTENTION_QUIET_MS) {} + + /** Arm (or re-arm) the deferred BEL for `paneKey`; `emit` runs only if the window elapses uncancelled. */ + defer(paneKey: string, emit: () => void): void { + this.cancel(paneKey) + const timer = setTimeout(() => { + this.timers.delete(paneKey) + emit() + }, this.quietMs) + // Why: a pending decorative bell must never hold the app open at quit. + timer.unref?.() + this.timers.set(paneKey, timer) + } + + cancel(paneKey: string): boolean { + const timer = this.timers.get(paneKey) + if (timer === undefined) { + return false + } + clearTimeout(timer) + this.timers.delete(paneKey) + return true + } + + cancelAll(): void { + for (const timer of this.timers.values()) { + clearTimeout(timer) + } + this.timers.clear() + } + + hasPending(paneKey: string): boolean { + return this.timers.has(paneKey) + } +} diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts index b3059ca6390..371d11c79d9 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts @@ -17,6 +17,7 @@ import type { } from './agent-completion-coordinator-types' import type { RuntimeTerminalProcessInspection } from '@/runtime/runtime-terminal-inspection' import { isPiCompatibleAgentType } from '../../../../shared/pi-agent-kind' +import { CODEX_ATTENTION_QUIET_MS } from '../../../../shared/codex-attention-quiet-window' import { titleHasExplicitAgentIdentity, titleIsInconclusiveNativeDroidTitle @@ -49,8 +50,6 @@ const PENDING_TITLE_TTL_MS = Math.max(2_000, INSPECTION_TIMEOUT_MS + 500) const PENDING_TITLE_MAX_TTL_MS = Math.max(30_000, PENDING_TITLE_TTL_MS) const COMPLETION_REPLAY_GUARD_MS = 1_000 const HOOK_DONE_QUIET_MS = 1_500 -// Why: under "Approve for me" Codex resumes almost immediately, so debounce the OS attention notification so a self-resolving pause raises no false banner (#8387). -const CODEX_ATTENTION_QUIET_MS = 1_500 const POLL_TIER_INTERVAL_MS: Record = { active: ACTIVE_POLL_INTERVAL_MS, diff --git a/src/shared/codex-attention-quiet-window.test.ts b/src/shared/codex-attention-quiet-window.test.ts new file mode 100644 index 00000000000..f609194d7c2 --- /dev/null +++ b/src/shared/codex-attention-quiet-window.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it } from 'vitest' +import { + CODEX_ATTENTION_QUIET_MS, + SYNTHETIC_PERMISSION_BELL, + buildSyntheticTerminalTitleFrame, + shouldDeferSyntheticPermissionBell +} from './codex-attention-quiet-window' +import { createTerminalTitleTracker } from './terminal-output-side-effects' +import { getSyntheticAgentTitleProfile } from './synthetic-agent-title' + +const CODEX_PERMISSION_LABEL = getSyntheticAgentTitleProfile('codex')?.permissionLabel ?? '' +const CODEX_IDLE_LABEL = getSyntheticAgentTitleProfile('codex')?.idleLabel ?? '' + +/** Count the bells a fabricated frame would ring through the real per-PTY tracker. */ +function bellsFromSyntheticFrames(frames: readonly string[]): number { + let bells = 0 + const tracker = createTerminalTitleTracker({ onBell: () => (bells += 1) }) + try { + for (const frame of frames) { + tracker.applySyntheticTitleFrame(frame) + } + } finally { + tracker.dispose() + } + return bells +} + +describe('shouldDeferSyntheticPermissionBell', () => { + it('defers only Codex permission pauses', () => { + expect(shouldDeferSyntheticPermissionBell({ agentType: 'codex', state: 'waiting' })).toBe(true) + expect(shouldDeferSyntheticPermissionBell({ agentType: 'codex', state: 'blocked' })).toBe(true) + expect(shouldDeferSyntheticPermissionBell({ agentType: 'codex', state: 'done' })).toBe(false) + expect(shouldDeferSyntheticPermissionBell({ agentType: 'codex', state: 'working' })).toBe(false) + }) + + it('leaves every other runtime ringing immediately', () => { + for (const agentType of ['claude', 'cursor', 'opencode', 'pi', null, undefined]) { + expect(shouldDeferSyntheticPermissionBell({ agentType, state: 'waiting' })).toBe(false) + } + }) + + it('never defers a question put to the user — no auto-reviewer answers those', () => { + for (const toolName of ['request_user_input', 'AskUserQuestion', 'requestUserInput']) { + expect( + shouldDeferSyntheticPermissionBell({ agentType: 'codex', state: 'waiting', toolName }) + ).toBe(false) + } + }) + + it('still defers an ordinary Codex approval pause carrying a tool name', () => { + expect( + shouldDeferSyntheticPermissionBell({ + agentType: 'codex', + state: 'waiting', + toolName: 'exec_command' + }) + ).toBe(true) + }) +}) + +describe('buildSyntheticTerminalTitleFrame', () => { + it('holds the attention BEL out of a Codex permission frame while keeping the title', () => { + const { frame, deferBell } = buildSyntheticTerminalTitleFrame({ + agentType: 'codex', + state: 'waiting', + label: CODEX_PERMISSION_LABEL + }) + + expect(deferBell).toBe(true) + expect(frame).toBe(`\x1b]0;${CODEX_PERMISSION_LABEL}\x07`) + // The lone \x07 left in the frame is the OSC terminator, not a bell. + expect(bellsFromSyntheticFrames([frame])).toBe(0) + }) + + it('still rings a non-Codex permission frame inline', () => { + const claudePermissionLabel = 'Claude - action required' + const { frame, deferBell } = buildSyntheticTerminalTitleFrame({ + agentType: 'claude', + state: 'blocked', + label: claudePermissionLabel + }) + + expect(deferBell).toBe(false) + expect(frame).toBe(`\x1b]0;${claudePermissionLabel}\x07\x07`) + expect(bellsFromSyntheticFrames([frame])).toBe(1) + }) + + it('rings a Codex request_user_input pause inline, without the quiet window', () => { + const { frame, deferBell } = buildSyntheticTerminalTitleFrame({ + agentType: 'codex', + state: 'waiting', + toolName: 'request_user_input', + label: CODEX_PERMISSION_LABEL + }) + + expect(deferBell).toBe(false) + expect(bellsFromSyntheticFrames([frame])).toBe(1) + }) + + it('never rings a terminal idle frame', () => { + const { frame, deferBell } = buildSyntheticTerminalTitleFrame({ + agentType: 'codex', + state: 'done', + label: CODEX_IDLE_LABEL + }) + + expect(deferBell).toBe(false) + expect(frame).toBe(`\x1b]0;${CODEX_IDLE_LABEL}\x07`) + expect(bellsFromSyntheticFrames([frame])).toBe(0) + }) + + it('rings once when the deferred BEL is released for a real pause', () => { + // Guards the over-suppression failure mode: a Codex pause the user must answer still + // reaches onBell — just after the quiet window instead of inside it. + const { frame } = buildSyntheticTerminalTitleFrame({ + agentType: 'codex', + state: 'waiting', + label: CODEX_PERMISSION_LABEL + }) + + expect(bellsFromSyntheticFrames([frame, SYNTHETIC_PERMISSION_BELL])).toBe(1) + }) + + it('pins the pre-fix frame as the one that rang inline (#13600 regression anchor)', () => { + // The shipped 1.4.179 frame; asserting it still rings proves the new frame's silence is + // the fix, not a tracker that stopped seeing fabricated bells. + expect(bellsFromSyntheticFrames([`\x1b]0;${CODEX_PERMISSION_LABEL}\x07\x07`])).toBe(1) + }) +}) + +describe('CODEX_ATTENTION_QUIET_MS', () => { + it('is the single window both attention paths wait out', () => { + expect(CODEX_ATTENTION_QUIET_MS).toBe(1_500) + }) +}) diff --git a/src/shared/codex-attention-quiet-window.ts b/src/shared/codex-attention-quiet-window.ts new file mode 100644 index 00000000000..3bfe3e48c14 --- /dev/null +++ b/src/shared/codex-attention-quiet-window.ts @@ -0,0 +1,48 @@ +import { isAskUserQuestionTool } from './agent-question-answered-intent' +import type { AgentStatusState } from './agent-status-types' + +/** + * Why: under Codex "Approve for me" the auto-reviewer resolves the permission pause itself, + * so every Codex attention signal waits out one quiet window and is dropped if work resumes + * inside it (#8387/#8519). Shared so the renderer's OS notification and main's fabricated + * permission BEL cannot drift apart again (#13600). + */ +export const CODEX_ATTENTION_QUIET_MS = 1_500 + +/** The standalone BEL main fabricates to light up a permission pause. */ +export const SYNTHETIC_PERMISSION_BELL = '\x07' + +/** + * Codex-only: no other runtime self-approves its own permission pause, so their permission BEL + * still rings immediately — matching the renderer's Codex-scoped attention debounce. + * + * `request_user_input` also normalizes to `waiting`, but no auto-reviewer ever answers a question + * put to the user, so it keeps ringing inline rather than paying the quiet window. + */ +export function shouldDeferSyntheticPermissionBell(args: { + agentType: string | null | undefined + state: AgentStatusState + toolName?: string | undefined +}): boolean { + return ( + args.agentType === 'codex' && + (args.state === 'waiting' || args.state === 'blocked') && + !isAskUserQuestionTool(args.toolName) + ) +} + +/** + * Terminal-state (non-working) synthetic title frame. The OSC title always lands now so the + * visible "action required" status is never delayed; only the attention BEL can be held back. + */ +export function buildSyntheticTerminalTitleFrame(args: { + agentType: string | null | undefined + state: AgentStatusState + toolName?: string | undefined + label: string +}): { frame: string; deferBell: boolean } { + const needsUserInput = args.state === 'blocked' || args.state === 'waiting' + const deferBell = shouldDeferSyntheticPermissionBell(args) + const inlineBell = needsUserInput && !deferBell ? SYNTHETIC_PERMISSION_BELL : '' + return { frame: `\x1b]0;${args.label}\x07${inlineBell}`, deferBell } +}