Notify on codex PermissionRequest: new nativeApprovalPrompt feed semantic raises the agentPermissionPrompt alert

[austinywang]

Fixes #9592.

Problem

With notifications.agentPermissionPrompt: true (the default), a Codex seat blocked on its approval dialog never produces a user notification. Codex's PermissionRequest hook is wired to the Feed bridge (cmux hooks feed --source codex --event PermissionRequest), and FeedEventClassifier deliberately maps it to non-actionable PreToolUse telemetry so cmux's Feed does not double-prompt against Codex's own approval reviewer (that mapping is what keeps "Approve for me" working). But that normalization also silently dropped the one signal Codex fires while blocked on the user, so no agentPermissionPrompt notification was ever raised — exactly what the issue's bus capture shows (agent.hook.PreToolUse + feed.item.*, no notification.*).

Fix

The classifier's actionability and its user-attention notification were conflated in one flag. This PR separates them with a new registry semantic instead of special-casing codex at a call site:

• FeedEventClassifier gains a .nativeApprovalPrompt semantic: "the agent is blocked waiting for the user in its own approval UI." Wire behavior is unchanged (non-actionable PreToolUse telemetry, no cmux Feed approval card, no blocking wait), but the classification now carries notifiesNativeApprovalPrompt: true. Codex's PermissionRequest / permission_request register with it. Any future agent with a native approval UI gets the same behavior with one registry line.
• runFeedHook delivers a fire-and-forget notify_target_async on that flag, built through the shared AgentHookNotificationClassifier (same "Permission" / "Approval needed" strings, same c=needs-permission;p=0 meta the generic agent notification hook and Claude's permission_prompt path emit), so the app gates it under the existing "Agent Needs Permission" setting via AgentNotificationDelivery. The tool summary (command/path) becomes the notification body when present.

No new user-facing strings: the notification reuses the existing localized agent.generic.notification.subtitle.permission / agent.generic.notification.body.approvalNeeded keys and the agent display name ("Codex"). Localization audit: no UI/settings/menu/help text changed; no web message catalogs affected.

Commits

1. Failing regression test (CI red expected on this commit) — widens classify to return a FeedEventClassification struct carrying notifiesNativeApprovalPrompt (false everywhere, behavior-neutral) and adds codexPermissionRequestRaisesPermissionPromptNotification, which fails against the old mapping. The struct widening is included so the test compiles; the behavioral assertion is the failing part.
2. Fix — the .nativeApprovalPrompt semantic, the codex registry entries, and the CLI notification delivery.

Testing

• FeedEventClassificationTests: new tests assert codex PermissionRequest/permission_request classify as telemetry-with-notification, the completion-only clear scoping, and the exact attention wire commands (UUID gating, payload shape, pipe/newline sanitization, needs-permission meta).
• CLI-level behavior suite (CI-wired): tests/test_codex_permission_prompt_notification.py spawns the real CLI against the existing FakeCmuxSocket harness and asserts the actual socket transport: the exact gated notify_target_async line precedes feed.push on PermissionRequest; the exact pane-scoped clear_notifications precedes telemetry on PostToolUse; PreToolUse emits neither; and the hook awaits the app's acknowledgement (0.5s-delayed OK is waited out — a fire-and-forget regression returns instantly). Red/green proven: the suite fails against release CLI 0.64.22 ("missing gated permission notification") and passes on this branch.
• Mock-socket repro of the issue's exact command (cmux hooks feed --source codex --event PermissionRequest with a codex payload from a bound pane env):
  • Before (release CLI 0.64.22): the CLI emits only the feed.push telemetry frame with hook_event_name: "PreToolUse" — no notification, matching the issue's bus capture.
  • After (this branch's tagged build): PermissionRequest emits the same unchanged feed.push frame plus notify_target_async <workspace-uuid> <surface-uuid> Codex|Permission|shell needs approval|c=needs-permission;p=0; PreToolUse emits only the feed.push frame (no over-notification, no premature clear); PostToolUse emits feed.push plus a pane-scoped clear_notifications --tab=<ws> --panel=<sf>.
• Not covered by automated tests: the end-to-end app-side banner (requires a live app + notification center); the app-side gating path (AgentNotificationDelivery / agentNotificationShouldDeliver) is pre-existing and already unit-tested.

Review round (codex autoreview findings addressed)

• Notification body no longer contains tool input. The first cut put the tool summary (full shell command) in the notification body; commands can embed credentials and banners reach lock screens, paired phones, and the recorded notification history. The body now names only the tool — "<tool> needs approval" via the same feed.notification.permission.body string the in-app Feed approval banner uses (falling back to the existing "Approval needed").

• Stale/false alerts self-heal — on tool completion only. Codex's PermissionRequest fires before its own "Approve for me" reviewer (per Keep Codex permission hooks non-blocking #5507's contract), so an auto-approved request would leave a stale "Permission" alert. A completed tool strictly follows any approval, so codex PostToolUse feed events now clear the pane's notifications (mirroring the pane-wide clears Claude's lifecycle hooks and Hermes' approval-response hook already perform; codex's own prompt-submit hook clears denied-approval residue at the next turn). Pre-tool events deliberately do NOT clear: codex gives no ordering guarantee between PermissionRequest and its pre-tool hooks, so a start-time clear could race and erase the just-raised prompt while the agent is still blocked. The clear is registry-scoped: only sources that raise native approval prompts get it. The clear stays pane-wide and uncorrelated by design — notifications carry no request identity anywhere in cmux, and this matches every existing agent integration.

• Clears ride only the synchronous feed-hook path. A review round asked for the clear on the wrapper telemetry lane too (hooks codex post-tool-use); the next round correctly observed that wrapper-injected hooks run as fire-and-forget nohup workers with no ordering guarantee, so a delayed completion worker's clear could erase a newer request's live notification. The wrapper-lane clear was removed again (documented in sendFeedTelemetry); wrapper-path staleness is pre-existing shipped behavior that self-heals at the next prompt-submit pane clear. The feed-hook path's events arrive in codex's own order (synchronous hook commands), so its clear is ordering-safe.

• Immediate notify retained deliberately. Codex offers no post-reviewer "now prompting the user" hook, and the wrapper-injected hook schema (CodexHookInjectionSchema → cmux hooks codex notification) already posts this same immediate needs-permission notification today; suppressing until "authoritative" proof would recreate exactly the silence Codex PermissionRequest hook events never produce a user notification (agentPermissionPrompt) — event is normalized to PreToolUse at ingest #9592 reports. The alert is gated by the user's "Agent Needs Permission" setting and now self-heals as above.

• Transport ordering is acknowledged, not assumed. The notify/clear line is sent request/response and awaited (bounded 2s) before the synchronous hook returns — the same contract Claude's and Hermes' hooks use — so the next hook's process starts only after the mutation is in the app's ordered lane. The feed frame stays one-way best-effort on its own bounded connection (no implicit relay reconnect can outlive the agent's hook budget). Warm hook latency ~0.15s measured.

• Consciously accepted residual (documented in code): codex's fire-and-forget prompt-submit worker clears the pane at turn start from a detached process; if that worker is slower than the model's very first approval-needing tool call, its late clear can remove the new notification. This is the same pre-existing exposure the shipped wrapper-path notification (hooks codex notification) has always had — this PR does not widen the class. Eliminating it requires origin-time-fenced clears (a cross-layer notification-store protocol change), deliberately out of scope.

Open review findings (deferred — not addressed in this PR)

Thirteen structured-review rounds ran on this branch; every earlier finding was either fixed as prescribed (redaction, completion-only clears, acknowledged transport, relay deadline budget, alias-safe live-target resolution, probe budget reserve, CLI-level behavior tests) or consciously rejected with codebase evidence (per-request notification correlation, prompt-submit clear race). The final round raised three further transport-tail P1s that are deferred as follow-ups rather than iterated further here:

1. Stalled-probe connection reuse: after a timed-out live-target probe, the attention send reuses the same connection; the command is still written and processed, but the acknowledgement can consume the probe's late reply, weakening the pre-return-ack guarantee in that already-degraded tail (a delayed PostToolUse clear could then race a newer notification). Fix would probe on a separate connection.
2. Relay handshake budget arithmetic: if each relay-backed send requires a fresh HMAC handshake, the 2s attention deadline may not fit probe + notify on slow links; needs relay-path measurement to size properly.
3. Oversized payloads bypass attention: codex payloads over the 1 MiB stdin bound return {} before classification, so an oversized PermissionRequest doesn't notify; delivery could fall back to the trusted --event flag.

All three are narrower than the shipped baseline this PR improves on (previously no codex approval notification existed on any path), and each is confined to the new attention lane's degraded-transport tails.

Notes / scope

• The in-app "Needs input" sidebar badge is intentionally not flipped for codex approval blocks: codex has no follow-up hook that reliably marks the approval resolved (approve → tool runs → PostToolUse; deny → turn continues), so setting needs-input state here would risk a stuck badge. The notification is the contract agentPermissionPrompt documents.
• Routing uses the caller pane's CMUX_WORKSPACE_ID/CMUX_SURFACE_ID (same identities the feed frame itself rides); if either is missing or not a UUID the notification is skipped best-effort, never failing the hook.

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

Feed event classification now returns structured approval state. Codex permission requests trigger native approval notifications without creating actionable Feed events. Codex tool lifecycle events clear those notifications through an acknowledged feed transport.

Changes

Native approval prompt handling

Layer / File(s)	Summary
Structured approval classification CLI/FeedEventClassifier.swift	FeedEventClassifier returns structured results for wire events, actionability, native approval notification, and prompt clearing. Codex permission requests map to non-actionable native approval prompts.
Native notification transport CLI/cmux.swift	Feed hook processing resolves live pane identities, validates identifiers, builds notification or clear commands, acknowledges attention mutations, and sends telemetry separately.
Approval classification regression coverage cmuxTests/FeedEventClassificationTests.swift, tests/test_codex_permission_prompt_notification.py, .github/workflows/ci.yml	Tests cover Codex permission variants, source scope, non-actionability, lifecycle-based prompt clearing, command sanitization, UUID normalization, invalid targets, acknowledgment ordering, delayed authentication, live-target resolution, and CI execution.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• manaflow-ai/cmux#9755: Both changes use the feed attention protocol for agent approval and prompt-clearing notifications.

Suggested reviewers: lawrencecchen, azooz2003-bit

Sequence Diagram(s)

sequenceDiagram
  participant Codex
  participant FeedHook
  participant FeedEventClassifier
  participant FeedAttentionTransport
  participant FeedTelemetryTransport
  Codex->>FeedHook: PermissionRequest
  FeedHook->>FeedEventClassifier: classify event
  FeedEventClassifier-->>FeedHook: non-actionable approval notification
  FeedHook->>FeedAttentionTransport: send notification and await acknowledgment
  FeedHook->>FeedTelemetryTransport: send telemetry
  Codex->>FeedHook: PostToolUse
  FeedHook->>FeedEventClassifier: classify lifecycle event
  FeedEventClassifier-->>FeedHook: clear prompt
  FeedHook->>FeedAttentionTransport: clear notification and await acknowledgment

Loading

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (5 errors)

Check name	Status	Explanation	Resolution
Cmux Swift Blocking Runtime	❌ Error	CLI/cmux.swift adds a production feed-hook path that synchronously calls SocketClient.send/sendV2 and waits for socket acknowledgements under a fixed two-second deadline.	Replace synchronous socket waits and fixed deadline coordination with non-blocking async completion or an actor-owned cancellation-aware signal; preserve ordering without blocking the hook process.
Cmux Swift Package Boundaries	❌ Error	The PR materially expands CLI/FeedEventClassifier.swift with Foundation-only registry, classification, and wire builders that tests run without the app or CLI.	Extract FeedEventClassification and FeedEventClassifier into a small CmuxFeedCore SwiftPM target, exposing FeedEventClassifier; keep SocketClient, deadlines, and pane delivery in CLI.
Cmux User-Facing Error Privacy	❌ Error	The new production alert payload hard-codes the upstream vendor name Codex (Codex|Permission|...), and no product-UI vendor configuration permits that name.	Use a generic localized alert title, or derive the title only from a vendor name explicitly configured by the user; do not emit the fixed Codex name in this new alert.
Cmux Architecture Rethink	❌ Error	Codex alert ordering uses a new synchronous target-resolution/send ACK lane with a 2s deadline before feed.push; this papers over socket races and leaves detached clears able to erase newer alerts.	Make the app-owned notification bus the sole ordered source. Add an event-generation fence for notify/clear, then remove CLI target probing, ACK waits, and deadline-based sequencing.
Cmux No Ambient Global State	❌ Error	CLI/FeedEventClassifier.swift:452 adds an internal static API to the existing empty, static-only FeedEventClassifier namespace; this is new ambient helper surface, not an incidental touch.	Move attention-command behavior to a constructable, injectable FeedEventAttentionCommandBuilder owned at the CLI seam; keep payload sanitization private/file-scoped.

✅ Passed checks (20 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The implementation satisfies issue #9592 by notifying on Codex permission requests while preserving non-actionable PreToolUse telemetry.
Out of Scope Changes check	✅ Passed	The code, tests, and CI updates directly support Codex permission notification delivery, cleanup, routing, and regression coverage.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation	✅ Passed	Production changes add synchronous CLI/socket helpers and a pure classifier; the CLI target is Swift 5.0 without MainActor default isolation, and no new Sendable reference, async protocol, or UI-st...
Cmux Browser Automation Off-Main	✅ Passed	The PR changes only feed classification, CLI, CI, and related tests; browser rule-scope files are unchanged and the diff has no browser/WebKit or worker-policy changes.
Cmux Expensive Synchronous Load	✅ Passed	The cumulative Swift diff adds only pure attention-command construction and bounded socket delivery; existing transcript/session parsing paths are unchanged.
Cmux Cache Substitution Correctness	✅ Passed	The diff adds a live delivery-target resolver and does not replace a fresh persistence/history/snapshot read; ambient IDs are a documented fallback for advisory notifications.
Cmux No Hacky Sleeps	✅ Passed	The diff adds no covered non-Swift production wait; its only sleep/poll is test-only scaffolding, and the workflow change only runs that regression test.
Cmux Algorithmic Complexity	✅ Passed	The production diff adds constant-count socket calls and fixed registry/payload checks; it adds no loops or per-target scans over user-owned collections. Regression code is test-only.
Cmux Swift Concurrency	✅ Passed	The Swift diff adds no Dispatch queues/groups, Combine state, completion-handler APIs, or unowned Tasks; attention delivery is synchronous and bounded through existing SocketClient request/response...
Cmux Swift @Concurrent	✅ Passed	The full Swift diff adds no async declarations, @concurrent annotations, or actor isolation; new network helpers are synchronous methods on the non-actor CMUXCLI type.
Cmux Swiftpm Lockfiles	✅ Passed	The PR changes only CI, CLI, and tests; it adds no Package.swift, Package.resolved, .gitignore, or Xcode project dependency changes, so no lockfile rule is violated.
Cmux Swift Logging	✅ Passed	The PR adds no prohibited Swift logging APIs or diagnostics; the new notification uses sanitized tool names, and print calls are limited to existing CLI output or test harness code.
Cmux Full Internationalization	✅ Passed	Production Swift uses localized APIs with existing catalog keys; no catalog, locale, or web message files changed. Added tests and CI changes are explicitly allowed by the rule.
Cmux Swiftui State Layout	✅ Passed	The Swift diff only changes CLI feed-hook transport and tests. It adds no SwiftUI views, state wrappers, GeometryReader, lazy-row store references, or render-time state mutation.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	PR Swift diff changes feed classification and hook transport only; it adds no standalone window code, and scripts/lint_auxiliary_window_close_shortcuts.py passes.
Cmux Source Artifacts	✅ Passed	The feature diff contains only Swift source, a CI workflow, an existing Swift test, and the intentional Python regression test; no artifact directories, logs, caches, or generated outputs were added.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	The PR changes only CLI/.swift and cmuxTests/.swift; no Swift file under a production /Sources/ path is changed, so this check is not applicable.
Title check	✅ Passed	The title clearly identifies the Codex permission notification change and the new nativeApprovalPrompt semantic.
Description check	✅ Passed	The description thoroughly explains the problem, implementation, scope, testing, and known limitations, despite omitting the template checklist and demo video.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-9592-codex-permission-notify

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 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/cmux.swift`:
- Around line 34939-34973: Update the socketPath branch around
sendBestEffortFeedTelemetry so the feed line and optional promptLine are sent
through one reused SocketClient connection, avoiding separate connection and
authentication attempts. Preserve the existing best-effort behavior and send
both lines when promptLine is non-nil.

In `@CLI/FeedEventClassifier.swift`:
- Around line 169-238: Make wireMapping the sole owner of
clearsNativeApprovalPrompt: derive the appropriate clear value there for the
lifecycle branches using its existing source parameter, and remove the separate
clearsPrompt derivation and overwrite in classify. Ensure the
FeedEventClassification returned by wireMapping is passed through without
replacing its clear flag.
- Around line 65-80: Update classify’s clearsPrompt logic in FeedEventClassifier
so Codex .toolStart does not clear a pending native approval prompt. Restrict
Codex clearing to the post-approval lifecycle event that confirms the approval
decision resolved, while preserving existing clearing behavior for other
eligible sources and events.

In `@cmuxTests/FeedEventClassificationTests.swift`:
- Around line 172-181: Extend codexToolLifecycleClearsNativeApprovalPrompt to
assert that the beforeShellExecution lifecycle alias with the shell tool also
sets clearsNativeApprovalPrompt to true. Keep the existing assertions unchanged
and cover this alias alongside the other Codex PreToolUse entrypoints.

🪄 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: 07ca1e8c-e36d-4358-968b-e3831151d07e

📥 Commits

Reviewing files that changed from the base of the PR and between 3faf795 and 1b3aaa8.

📒 Files selected for processing (3)

• CLI/FeedEventClassifier.swift
• CLI/cmux.swift
• cmuxTests/FeedEventClassificationTests.swift

[coderabbitai]

Actionable comments posted: 2

🤖 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/FeedEventClassifier.swift`:
- Around line 44-52: Update the .promptSubmit handling in FeedEventClassifier,
including the telemetry("UserPromptSubmit") path, so clearsNativeApprovalPrompt
is true when the source raises native approval prompts. Preserve the existing
behavior for sources that do not use native approval prompts.

In `@cmuxTests/FeedEventClassificationTests.swift`:
- Around line 314-354: Extend
attentionCommandRequiresUUIDTargetsAndAttentionSemantics to assert nil when
surfaceId is missing. Add coverage near attentionCommandSanitizesPipeInToolName
for newline characters in tool names, verifying they are replaced or neutralized
so the generated socket command remains a single line.

🪄 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: 7d1d47b7-00d6-4d08-8e6d-1bf4c2a27749

📥 Commits

Reviewing files that changed from the base of the PR and between 1b3aaa8 and a19d517.

📒 Files selected for processing (3)

• CLI/FeedEventClassifier.swift
• CLI/cmux.swift
• cmuxTests/FeedEventClassificationTests.swift

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

CLI/cmux.swift (1)

34951-35000: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Reuse the open client for the telemetry line instead of opening a second connection.

When promptLine is non-nil and client is non-nil, the code sends the attention line over the existing client, then calls sendBestEffortFeedTelemetry(socketPath: telemetrySocketPath, ...) for the feed telemetry line. This constructs a new SocketClient, connects, and re-authenticates, even though client is already open and authenticated.

Compare this to the else if let client branch two lines below, which correctly sends the telemetry line with client.sendOneWay(command: line, writeTimeout: 0.05) on the existing connection. The client-present branch with a promptLine should do the same instead of falling back to a fresh socketPath-based connection.

This method is reachable in production: the feed-hook backward-compatibility command passes the main authenticated session client into runFeedHook, so this doubling happens on every classified event with an attention command in that call path.

⚙️ Proposed fix: reuse the open client for telemetry when available

             if let promptLine {
                 if let client {
                     _ = try? client.send(
                         command: promptLine,
                         responseTimeout: Self.feedAttentionAcknowledgeTimeoutSeconds
                     )
+                    _ = try? client.sendOneWay(command: line, writeTimeout: 0.05)
                 } else if let socketPath {
                     sendAcknowledgedFeedAttention(
                         socketPath: socketPath,
                         attentionLine: promptLine,
                         socketPassword: socketPassword
                     )
-                }
-                let telemetrySocketPath = socketPath ?? client?.socketPath
-                if let telemetrySocketPath {
                     sendBestEffortFeedTelemetry(
-                        socketPath: telemetrySocketPath,
+                        socketPath: socketPath,
                         line: line,
                         socketPassword: socketPassword
                     )
                 }
             } else if let client {

🤖 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 34951 - 35000, Update the promptLine handling in
runFeedHook so that when the existing client is available, it sends the
telemetry line with client.sendOneWay using the same bounded write timeout as
the nearby client branch. Only use sendBestEffortFeedTelemetry with the socket
path when no client is available, avoiding a second connection and
re-authentication.

🤖 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 `@tests/test_codex_permission_prompt_notification.py`:
- Around line 205-216: Replace the elapsed-based assertion in the
PermissionRequest test with deterministic acknowledgement gating: update
FakeCmuxSocket to signal notification receipt and block its acknowledgement on a
test-controlled gate, launch the hook via Popen, assert it remains running while
the gate is closed, then release the gate and await completion while preserving
the notification and empty-stdout checks.

---

Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 34951-35000: Update the promptLine handling in runFeedHook so that
when the existing client is available, it sends the telemetry line with
client.sendOneWay using the same bounded write timeout as the nearby client
branch. Only use sendBestEffortFeedTelemetry with the socket path when no client
is available, avoiding a second connection and re-authentication.

🪄 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: 9ff6c104-6624-4b81-9010-ddd76de539ed

📥 Commits

Reviewing files that changed from the base of the PR and between a19d517 and e98c53f.

📒 Files selected for processing (4)

• .github/workflows/ci.yml
• CLI/cmux.swift
• cmuxTests/FeedEventClassificationTests.swift
• tests/test_codex_permission_prompt_notification.py

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Replace the elapsed-time assertion with an acknowledgement gate.

elapsed < delay - 0.1 makes test correctness depend on scheduler timing. It can fail under load without a transport regression.

Have FakeCmuxSocket signal when it receives the notification and block its acknowledgement on a test-controlled gate. Start the hook with Popen, verify it remains running while the gate is closed, then release the gate and await completion.

As per coding guidelines, “Test code must avoid real wall-clock dependencies” and “Tests must not read wall-clock APIs … in assertions.”

🧰 Tools

🪛 Ruff (0.16.1)

[warning] 209-209: Avoid specifying long messages outside the exception class

(TRY003)

---

[warning] 211-211: Avoid specifying long messages outside the exception class

(TRY003)

---

[warning] 213-216: Avoid specifying long messages outside the exception class

(TRY003)

🤖 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 `@tests/test_codex_permission_prompt_notification.py` around lines 205 - 216,
Replace the elapsed-based assertion in the PermissionRequest test with
deterministic acknowledgement gating: update FakeCmuxSocket to signal
notification receipt and block its acknowledgement on a test-controlled gate,
launch the hook via Popen, assert it remains running while the gate is closed,
then release the gate and await completion while preserving the notification and
empty-stdout checks.

Source: Coding guidelines

[coderabbitai]

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 `@CLI/cmux.swift`:
- Around line 34661-34753: Update deliverNativeApprovalPromptAttention and the
runFeedHook telemetry flow to reuse the active connection for
sendBestEffortFeedTelemetry when activeClient.isRelayBacked is false, including
the common nil-client local socket path. Keep relay-backed connections on the
existing separate-connection telemetry path because the acknowledged send closes
them, and preserve proper closure and timeout behavior for both paths.

🪄 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: 09a408da-13ee-4c65-9ca9-1a0693632972

📥 Commits

Reviewing files that changed from the base of the PR and between e98c53f and e5b6820.

📒 Files selected for processing (2)

• CLI/cmux.swift
• tests/test_codex_permission_prompt_notification.py

[coderabbitai]

🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Reduce the extra socket round trip added per native-approval event.

deliverNativeApprovalPromptAttention sends the acknowledged attention command over client when it is provided, or otherwise opens, uses, and closes its own connection. Immediately after it returns, runFeedHook calls sendBestEffortFeedTelemetry, which always opens a brand-new SocketClient connection and re-authenticates for the feed.push telemetry line, regardless of whether client was already open.

In the common feed-hook/hooks feed dispatch path, client is nil, so each native-approval event now performs two full connect+authenticate round trips instead of one. classification.clearsNativeApprovalPrompt fires on Codex PostToolUse for every tool call on agents that raise native approval prompts, so this cost repeats on every tool call, not once per turn.

The code comments explain that combining both sends onto one connection is unsafe for relay-backed sockets, because send() closes a relay connection after use and a later sendOneWay reconnect is not timeout-bounded. That reasoning does not apply to local Unix-domain sockets: SocketClient.send() only sets shouldCloseAfterSend when relayEndpoint != nil, so a non-relay connection stays open after the acknowledged attention send and could carry the telemetry line as a follow-up sendOneWay call on the same connection before closing.

Consider branching on activeClient.isRelayBacked inside deliverNativeApprovalPromptAttention: reuse the same connection for the telemetry line when it is not relay-backed, and fall back to a separate connection only for relay-backed sockets.

Also applies to: 35004-35043

🤖 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 34661 - 34753, Update
deliverNativeApprovalPromptAttention and the runFeedHook telemetry flow to reuse
the active connection for sendBestEffortFeedTelemetry when
activeClient.isRelayBacked is false, including the common nil-client local
socket path. Keep relay-backed connections on the existing separate-connection
telemetry path because the acknowledged send closes them, and preserve proper
closure and timeout behavior for both paths.