Repository navigation
Settle auto-resolved Codex approval notifications - #10019
austinywang wants to merge 71 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds stable Codex approval identities and routes approval notifications through coordinated delivery and resolution. It suppresses automatic approvals, correlates completion events with prompts, supports ID and scope clearing, and adds deterministic lifecycle and regression tests. ChangesCorrelated approval notification flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CodexHook
participant FeedEventClassifier
participant AgentApprovalNotificationCoordinator
participant TerminalNotificationQueue
participant TerminalNotificationStore
CodexHook->>FeedEventClassifier: classify permission request
FeedEventClassifier->>AgentApprovalNotificationCoordinator: stage approval identity
AgentApprovalNotificationCoordinator->>TerminalNotificationQueue: deliver correlated notification
TerminalNotificationQueue->>TerminalNotificationStore: store correlation key
CodexHook->>FeedEventClassifier: process matching tool completion
FeedEventClassifier->>TerminalNotificationQueue: resolve approval ID
TerminalNotificationQueue->>TerminalNotificationStore: clear correlated notification
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/AgentNotificationGateTests.swift`:
- Around line 157-180: Update
pendingApprovalsInOnePaneCoalesceIntoOneNotification to resolve firstApprovalID
after staging and running the approvals, then assert the second approval remains
delivered and fixture.deliveries.clears is empty. Ensure the test specifically
verifies that resolving the stale first approval does not clear the newer
pending approval.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8cc22a98-1886-4bec-91c2-d59244dd3010
📒 Files selected for processing (1)
cmuxTests/AgentNotificationGateTests.swift
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/AgentHookNotificationPolicy.swift`:
- Around line 80-89: Update canonicalJSON to return an optional String and
return nil when the value is absent or cannot be deterministically
JSON-serialized; remove the String(describing:) fallback. Propagate this
optional through the approval identity construction in make so it returns nil
when canonicalJSON fails, preserving the caller’s existing uncorrelated
settle-window behavior.
In `@CLI/cmux.swift`:
- Around line 32544-32571: Extract the shared Codex auto-review classification
from the notification-hook site at CLI/cmux.swift:32544-32571 into a helper that
reads the bounded rollout tail, calls
CodexApprovalNotificationPolicy.reviewRoute, and checks .autoReview; update both
sites to use it, including runFeedHook at CLI/cmux.swift:35137-35161 with its
stdinObj source. Also verify or adjust reviewRoute so empty or unavailable
rolloutLines fail closed by returning a non-.autoReview route.
In `@cmuxTests/AgentNotificationGateTests.swift`:
- Around line 254-289: Update reorderedOldResolutionDoesNotCancelNewApproval so
secondApprovalID is staged before the delayed firstApprovalID entries, while
preserving the duplicate old request coverage. Keep the scheduler execution and
assertions focused on delivering only the newer approval notification, ensuring
exact-resolution and scope-level tombstone handling cannot allow the stale
request to replace it.
In `@Sources/AgentApprovalNotificationCoordinator.swift`:
- Around line 124-139: Update resolve(surfaceID:approvalID:) to remove every
candidate in state.candidates whose approvalID matches the supplied approvalID,
rather than selecting only the lowest-sequence candidate. Preserve the existing
tombstone behavior when no candidate matches, then call finishResolution with
the updated state so duplicate candidates cannot keep the pane alive.
In `@Sources/TerminalNotificationQueue.swift`:
- Around line 607-615: Update the .clearNotificationCorrelation case to clear
notifications even when agentNotificationDeliveryTarget returns nil: use
target.tabId when available, otherwise fall back to the enqueued key.tabId,
while preserving the existing correlationKey.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 438f03a6-b31c-440e-803e-a8026c3512dd
📒 Files selected for processing (16)
CLI/AgentHookNotificationPolicy.swiftCLI/FeedEventClassifier.swiftCLI/cmux.swiftSources/AgentApprovalCorrelationID.swiftSources/AgentApprovalNotificationCoordinator.swiftSources/AgentNotificationDelivery.swiftSources/AgentNotificationGate.swiftSources/TerminalController.swiftSources/TerminalNotificationLiveRetargetDelivery.swiftSources/TerminalNotificationQueue.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentNotificationGateTests.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/FeedEventClassificationTests.swifttests/test_codex_permission_prompt_notification.py
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 similar comment
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLI/AgentHookNotificationPolicy.swift (1)
160-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail closed when no matching turn ID exists.
When
rawObjecthas no turn ID, Line 175 returns the reviewer route from the newest rollout context. That context can belong to another turn. An unrelatedauto_reviewroute then suppresses a blocking approval banner.Require a matching structured turn ID before using rollout data. Keep direct request fields as the only route source when no turn ID is available.
Proposed fix
let requestedTurnID = firstString( in: rawObject, keys: ["turn_id", "turnId"] ) - var latestTurnContext: [String: Any]? + guard let requestedTurnID else { return nil } for line in rolloutLines.reversed() { guard let data = line.data(using: .utf8), let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], object["type"] as? String == "turn_context", let payload = object["payload"] as? [String: Any] else { continue } - if latestTurnContext == nil { - latestTurnContext = payload - } - guard let requestedTurnID else { - return reviewRoute(in: payload) - } if firstString(in: payload, keys: ["turn_id", "turnId"]) == requestedTurnID { return reviewRoute(in: payload) } } - - if let latestTurnContext, - firstString(in: latestTurnContext, keys: ["turn_id", "turnId"]) == nil { - return reviewRoute(in: latestTurnContext) - } return nilAs per coding guidelines: “A missing reliable signal must fail closed.” As per path instructions: approval routing must use authoritative structured identifiers and fail closed when rollout data cannot resolve them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/AgentHookNotificationPolicy.swift` around lines 160 - 188, Update the rollout-context resolution around requestedTurnID and latestTurnContext so rollout data is used only when a structured turn ID is present and matches the requested turn ID. Remove the fallback that routes from a context without turn_id, and preserve direct request fields as the sole route source when requestedTurnID is absent or no matching context exists.Sources: Coding guidelines, Path instructions
Sources/AgentApprovalNotificationCoordinator.swift (1)
292-309: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove fixed-delay approval coordination from the runtime path.
Task.sleepcontrols whether the coordinator delivers an approval notification. This creates a timing-dependent state window instead of resolving from authoritative approval lifecycle signals.Use an explicit approval-pending or completion signal to trigger delivery. Keep cancellation in the coordinator.
As per coding guidelines, “flag
Task.sleep… used for … delayed coordination.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/AgentApprovalNotificationCoordinator.swift` around lines 292 - 309, Update scheduleOnMainActor so approval notification delivery is triggered by an explicit approval-pending or completion signal rather than Task.sleep(for:). Preserve the existing cancellation behavior by retaining the returned task cancellation mechanism, and keep delivery on MainActor through the action closure.Source: Coding guidelines
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
32545-32552: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAuto-review duplication is resolved; residual transcript-path fallback asymmetry remains.
Both call sites now delegate to the shared
CodexApprovalNotificationPolicy().isAutoReviewed(rawObject:transcriptPath:readRolloutLines:)method. This addresses the previously flagged duplication of the rollout-read-and-classify logic.One asymmetry remains between the two call sites:
- At line 32547, the generic agent hook path falls back to
mapped?.transcriptPathwheninput.transcriptPathis absent.- At lines 35144-35147, the
runFeedHookpath only readstranscript_path/transcriptPathfrom the raw stdin object, with no session-store fallback.If a Codex event omits the transcript path key on a given hook delivery,
runFeedHookhas no way to recover it, while the generic hook path does. This does not create a safety bug (missing transcript data meansisAutoReviewedfails closed and still notifies), but it does mean the same underlying approval request can classify differently across the two delivery paths depending on which one is missing the transcript path key.Unify the transcript-path resolution for both call sites, or confirm that Codex always supplies
transcript_pathon every hook invocation this path handles.Also applies to: 35140-35152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 32545 - 32552, Unify transcript-path resolution between the generic agent hook call and runFeedHook’s CodexApprovalNotificationPolicy().isAutoReviewed invocation. Update runFeedHook to use its raw transcript_path/transcriptPath value with the same session-store fallback used by the generic path, preserving fail-closed behavior when neither source provides a path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/AgentHookNotificationPolicy.swift`:
- Around line 55-60: Update CodexApprovalNotificationIdentity.make to use the
authoritative per-request tool-call or approval identifier shared by prompt and
completion lifecycle events when constructing approvalID. Do not rely on the
derived session/turn/tool/input hash for exact clearing; if no shared identifier
is available, fail closed rather than returning a potentially colliding
approvalID.
In `@Sources/AgentApprovalNotificationCoordinator.swift`:
- Around line 126-134: In the exact-resolution flow, update the surrounding
resolution method to record exactResolutionTombstones for the approvalID before
checking state.candidates, so every exact resolution suppresses delayed
duplicate PermissionRequest deliveries. Preserve the existing candidate-removal
behavior, and add a regression test that stages an approval, resolves it, then
stages the same approvalID again and verifies no notification is delivered.
---
Outside diff comments:
In `@CLI/AgentHookNotificationPolicy.swift`:
- Around line 160-188: Update the rollout-context resolution around
requestedTurnID and latestTurnContext so rollout data is used only when a
structured turn ID is present and matches the requested turn ID. Remove the
fallback that routes from a context without turn_id, and preserve direct request
fields as the sole route source when requestedTurnID is absent or no matching
context exists.
In `@Sources/AgentApprovalNotificationCoordinator.swift`:
- Around line 292-309: Update scheduleOnMainActor so approval notification
delivery is triggered by an explicit approval-pending or completion signal
rather than Task.sleep(for:). Preserve the existing cancellation behavior by
retaining the returned task cancellation mechanism, and keep delivery on
MainActor through the action closure.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 32545-32552: Unify transcript-path resolution between the generic
agent hook call and runFeedHook’s
CodexApprovalNotificationPolicy().isAutoReviewed invocation. Update runFeedHook
to use its raw transcript_path/transcriptPath value with the same session-store
fallback used by the generic path, preserving fail-closed behavior when neither
source provides a path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab99f37b-f06a-48c2-971b-75d64ffaeea3
📒 Files selected for processing (6)
CLI/AgentHookNotificationPolicy.swiftCLI/cmux.swiftSources/AgentApprovalNotificationCoordinator.swiftSources/TerminalNotificationQueue.swiftcmuxTests/AgentNotificationGateTests.swiftcmuxTests/FeedEventClassificationTests.swift
|
Review follow-up on 858de98:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/FeedEventClassificationTests.swift`:
- Around line 441-449: Update codexRolloutReviewerRequiresMatchingTurnID so
rawObject includes a requested turn ID distinct from the rollout’s
"another-turn" value, ensuring CodexApprovalNotificationPolicy.reviewRoute
reaches and verifies the turn-ID mismatch while preserving the nil expectation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7231a56d-105e-4a24-a430-0a00eaa7cc39
📒 Files selected for processing (5)
CLI/AgentHookNotificationPolicy.swiftCLI/cmux.swiftSources/AgentApprovalNotificationCoordinator.swiftcmuxTests/AgentNotificationGateTests.swiftcmuxTests/FeedEventClassificationTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 similar comment
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 similar comment
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
b19a78e to
abe72e5
Compare
|
All contributors have signed the CLA ✍️ ✅ |
abe72e5 to
6f23147
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 similar comment
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Deployment failed for project cmux166 with the following error: Learn More: https://vercel.com/manaflow?upgradeToPro=build-rate-limit |
…oapprove-notify-swarm # Conflicts: # CLI/cmux.swift # Sources/AgentNotificationDelivery.swift # tests/test_codex_permission_prompt_notification.py
…oapprove-notify-swarm
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a4bf746. Configure here.
…oapprove-notify-swarm
teamleaderleo
added
S2: major
|
Closing, superseded by the #10017 fix on main. |
This branch was successfully deployed
3 participants
Summary
auto_reviewevidence; correlate tool completion and turn resolution so promptly resolved requests do not produce a banner.Fixes #10017 without regressing #9592. The earlier portal-tracker revert is already in main and is not part of this PR's current net diff.
Behavior and trade-offs
Verification
Validation results and exact tested SHAs are maintained in the single review-audit comment, including the historical test-only regression replays. Expanded hosted tests exercise the built CLI and captured socket frames, not source-text expectations. Runtime dogfood and visual evidence must be completed on the current full-branch-tagged build before self-merge.
Localization and scope
Audited the modified user-facing text: the existing English/Japanese
clear_notificationshelp entry lists the approval selectors alongside the existing correlation selector. No new UI copy, web message keys, iOS files, or mobile behavior are introduced by this PR's net diff.Summary by cubic
Codex
PermissionRequestevents no longer show an approval banner immediately. They now pass through a short, cancellable settle window, so auto-approved or denied requests stay silent while unresolved approvals remain visible; matching completions clear only their request. Fixes #10017 without regressing #9592.auto_reviewbanners while retaining feed telemetry; settling also covers journal-owned notifications and hooks without a shared native ID, while non-Codex, legacy, and ambiguous routes keep existing behavior.clear_notifications --approval-id=...and--approval-scope=...without changing existing selectors.hooks codex install, and makes watchdog cleanup deterministic.Written for commit 054e7bc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Note
High Risk
Changes core notification delivery, clearing semantics, and hook I/O for Codex approvals; incorrect correlation or settle logic could silence real prompts or leave stale banners.
Overview
Codex permission notifications no longer fire immediately. The CLI derives stable approval correlation IDs (provider call IDs or hashed session/turn/tool/input tuples) and embeds them in notification meta and
clear_notifications(--approval-id,--approval-scope, optional fallback). The app addsAgentApprovalNotificationCoordinator, which stages requests behind a short settle window, skips delivery when auto-review is proven from Codex rollout context, and resolves or scope-clears only the matching prompt so stale completions cannot wipe newer blockers.Feed and hook paths share the same identity: feed attention commands emit correlated notify/clear lines; generic Codex hooks skip banners for
auto_review, use approval meta when identity exists, clear onpost-tool-useand Stop (scope), and avoid deduping correlated raises. Journal /notify_target_asyncroutes needs-permission Codex events through the coordinator when meta carries the digest-shaped approval id.Supporting hardening: bounded Codex hook stdin and transcript tail reads (
O_NOFOLLOW, fixed read window), read-only shared lock for hook session lookups, safer fire-and-forget shell watchdog, and conservative Codex hook script GC during explicit install only.Reviewed by Cursor Bugbot for commit e0c03c7. Bugbot is set up for automated code reviews on this repo. Configure here.