fix(codex): hold the fabricated permission BEL for the Codex quiet window - #14715
brennanb2025 wants to merge 1 commit into
Conversation
…ndow Codex "Approve for me" could still raise "Bell in <worktree> · 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
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for this PR @brennanb2025. Anything left for this to be merged? |
|
Superseded by #21389, which is merged. This held the fabricated permission BEL for the 1.5s quiet window so an auto-resolved "Approve for me" pause could cancel it. The merged change removes the need: the row never enters a waiting state for a reviewer-owned approval, so the synthetic "action required" title and its BEL are never emitted in the first place. Two things found while reproducing this that argue against the timing approach in general:
Closing as superseded. The suppressors and |
ELI5
When Codex pauses to ask permission, Orca fakes a terminal "ding" so the pane lights up. Under "Approve for me" Codex answers its own question a moment later — but the ding had already fired, so you got a false Attention requested notification. This PR makes the ding wait a moment and cancel itself if Codex resolves the pause. The visible "Codex - action required" label still appears instantly.
What Changed
src/shared/codex-attention-quiet-window.ts(new) — ownsCODEX_ATTENTION_QUIET_MS(moved out of the renderer coordinator, value unchanged at 1500ms), plusshouldDeferSyntheticPermissionBellandbuildSyntheticTerminalTitleFrame.src/main/synthetic-permission-bell-deferral.ts(new) — per-pane timer map that arms, re-arms, cancels, and bulk-cancels the held-back BEL.src/main/index.ts—driveSyntheticTitleFromHooknow emits the OSC title without the trailing BEL for a Codex permission pause and defers the BEL instead. Any later non-permission hook state, a pane status clear, or window teardown cancels it.src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts— imports the shared constant instead of defining a second copy.No wire fields, no new env vars, no installer/relay/WSL/SSH changes.
Why
#8519 added a 1.5s quiet window for Codex attention — but only on the renderer's
dispatchAttentionpath (the rich "… - Codex needs input" notification).Orca raises attention for the same hook event a second way.
src/main/index.tsfabricates a synthetic title frame per hook state, and forwaiting/blockedit appends a standalone BEL:That frame goes to
runtime.ingestSyntheticTitleFrame→applySyntheticTitleFrame→onBellinpty-connection.ts, which marks the worktree/tab unread and, after onlyAGENT_TASK_COMPLETE_NOTIFICATION_GRACE_MS(250ms), dispatches{ source: 'terminal-bell' }. That notification renders asBell in <worktree>/<repo> · Attention requested— exactly the notification in the issue screenshot.So the reporter's observation is explained without the auto-review needing to exceed 1.5 seconds: the 1.5s window never covered this path at all. The reporter said as much ("This reproduction does not prove that the auto-review itself took more than exactly 1.5 seconds"); this is the missing half.
The fix makes Orca's own fabricated attention signal honor the decision #8519 already made, and puts both paths on one shared constant so they cannot drift apart again.
Scope discipline. Only the BEL Orca fabricates is deferred. A real BEL emitted by Codex's own output is untouched —
onBelldocuments that suppressing real PTY bells is a transport-layer guess, and that stays true.Not over-suppressing. Codex
request_user_inputalso normalizes towaiting, but no auto-reviewer ever answers a question put to the user, so it is exempt and rings inline. Non-Codex runtimes are untouched. And a Codex pause that is not resolved still rings — 1.5s later, not never.Superseded / related PRs
Supersedes #11046 (
fix(codex): suppress auto-review-owned permission attention, @GoodFarming). Its diagnosis of the symptom is right, but the remedy does not fix the reported repro:resolveAuthoritativeCodexApprovalReviewercontains a literalvoid args.wireReviewerand returnsresolveCodexApprovalReviewer(args.agentArgs). Reviewer ownership therefore comes only from Orca launch args matchingapprovals_reviewer=auto_review.unknown, suppression never engages, and the bell still fires.ORCA_CODEX_APPROVAL_REVIEWERthrough installer-utils, the managed hook script,pty-subprocess, WSL interop (WSLENV), the SSH relay, the relay hook server, hydrate persistence, and a newcodexApprovalReviewerIPC/wire field carry no effect on the fix — and the hook version bump1 → 2is wire surface taken on for it.driveSyntheticTitleFromHookbehind the same launch-arg proof.Also related to #8387 / #8519 (the timer this restores parity with) and #13991 (closed as a duplicate of #11046).
#10947 / PR #11174 is independent — not bundled. #10947 is about where Codex status hooks get installed (real
~/.codex/hooks.jsonvs managedCODEX_HOME) and shares no code with the attention path; this PR touches none of the five files #11174 touches. Verified rather than assumed, per the precedent set in #14611.Not a naming coincidence (per #14626). This gate keys on the normalized
agentType === 'codex'plus normalizedstate, never on a hook event name, so it does not depend on Codex and Claude happening to share event vocabulary.Linked Issue
Fixes #13600
Visual Proof
N/A— and stated plainly rather than implied: this was not verified on Windows. #13600 isos:Windows+ WSL, and the Windows host available to this session is unreachable (remote_runtime_unavailable). The change is derived from the code path and covered by automated tests that drive the realcreateTerminalTitleTrackerbell detector; no live Windows/WSL repro was performed and none is claimed.Nothing here is platform-dependent — no
process.platformbranch is added or changed, and the deferral runs identically on macOS, Linux, and Windows. The affected notification is OS-level, so there is no rendered UI surface to screenshot.Testing
Coverage on the two assertions that matter:
PermissionRequestraises no bellholds the attention BEL out of a Codex permission frame while keeping the title— frame fed to the real tracker yields 0 bellsrings once when the deferred BEL is released for a real pause— 1 bellrings a Codex request_user_input pause inline, without the quiet window— 1 bell,deferBell === falsestill rings a non-Codex permission frame inline— 1 belldrops the BEL when the pause resolves inside the window (#13600)pins the pre-fix frame as the one that rang inline— the shipped 1.4.179 frame still rings, so the new frame's silence is the fix and not a tracker that stopped seeing fabricated bellsNon-vacuity proven by mutation against pre-fix behavior:
needsUserInput ? BELL : '') → 2 tests fail.SyntheticPermissionBellDeferral.cancelto a no-op → 3 tests fail.anyinto the new shared module → oxlint errors, confirming the lint gate is live on these paths rather than silently clean.Scoped checks (no full typecheck — OOM risk):
npx tsc --noEmitover the two new modules and a probe asserting theParsedAgentStatusPayload→driveSyntheticTitleFromHook/shouldDeferSyntheticPermissionBell/buildSyntheticTerminalTitleFramecall sites: exit 0.npx oxlinton all new/changed files: clean; pre-commitoxlint+ react-doctor +oxfmtpassed.Platforms actually exercised: macOS (unit tests). Windows / WSL: not exercised — see Visual Proof. SSH: no transport code touched.
AI Disclosure
Claude Opus 5 via Claude Code.
Review
codexApprovalReviewerwire field and bumps the hook version.process.platformbranch added or changed; deferral behaves identically on macOS, Linux, and Windows.CODEX_ATTENTION_QUIET_MSmoved modules with its value unchanged; every non-Codex path and every non-permission state emits byte-identical frames.setTimeoutper Codex permission pause,unref'd so it cannot hold the app open at quit, replaced on re-arm and cleared on resume/status-clear/teardown.Known residual (stated, not hidden): if a Codex auto-review round trip exceeds the quiet window, both attention paths still fire. That is now a single shared constant instead of two divergent behaviors, but tuning it needs a live Windows/WSL repro this session could not obtain, so it is deliberately left at the value #8519 chose.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) — scoped lint/tests/typecheck run locally as listed above; full typecheck deliberately skipped (OOM risk), CI covers itAuthor