Keep Codex permission hooks non-blocking - #5507
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. 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:
📝 WalkthroughWalkthroughCodex PermissionRequest shifts to non-blocking telemetry (PreToolUse/.toolStart). cmux bridges Codex app-server approval notifications into Feed PermissionRequest events, normalizes feed decisions into app-server responses, tightens per-agent hook timeouts (Codex: 5s), adds working-directory CLI resolution/validation, and updates Swift/Python tests and docs. ChangesCodex PermissionRequest as Non-blocking Telemetry
Sequence Diagram(s)sequenceDiagram
participant CodexAppServer
participant CmuxWatcher
participant Feed
participant AppServerConnection
CodexAppServer->>CmuxWatcher: item/*/requestApproval notification
CmuxWatcher->>Feed: feed.push PermissionRequest event
Feed-->>CmuxWatcher: push response (mode / timed_out)
CmuxWatcher->>AppServerConnection: respond(mapped decision, timeout)
AppServerConnection-->>CodexAppServer: approval response payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (8 errors, 1 warning, 2 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 970950f280
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Codex setup should collapse duplicate cmux-owned prompt hooks to one entry, saw \(allCommands)" | ||
| ) | ||
| XCTAssertTrue( | ||
| codexHookEntries.contains { |
There was a problem hiding this comment.
When cmuxTests is built, this new assertion references codexHookEntries, but this test only defines allCommands above and never binds the flattened hook dictionaries. The symbol exists in a different test method, so the suite will fail to compile before any Codex hook regression tests can run.
Useful? React with 👍 / 👎.
![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/CLIGenericHookPersistenceTests.swift`:
- Around line 3143-3156: The test
testCodexHookInstallPrefersLaunchingAppBundledCLI references codexHookEntries
which is undefined; declare and initialize codexHookEntries before the
XCTAssertTrue calls by filtering the existing hookEntries collection (e.g., let
codexHookEntries = hookEntries.filter { ($0["source"] as? String) == "codex" }
or an equivalent predicate that selects Codex hooks) so the subsequent
assertions can compile and validate the Codex hook entries. Ensure you reference
the same hookEntries variable used earlier in the test.
🪄 Autofix (Beta)
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
Run ID: 1c4444d6-ecb2-4b6c-8d7d-896521d08803
📒 Files selected for processing (8)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/FeedEventClassifier.swiftCLI/cmux.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/FeedEventClassificationTests.swiftdocs/agent-hooks.mddocs/feed.mdtests/test_codex_feed_hooks.py
Greptile SummaryThis PR makes Codex
Confidence Score: 3/5The core reclassification and timeout fix are safe; the watcher suppression logic has a reconnect gap that can leave Codex app-server requests permanently unanswered on a live connection. The approval bridge suppression mechanism does not reset on watcher reconnect. If Codex re-presents a previously-timed-out approval on the fresh connection, the watcher silently consumes the request without sending any JSON-RPC response, leaving it stalled with no path to Codex's native TUI. CLI/cmux.swift — the handleApprovalRequest suppression path and resetConnectionSubscriptions, which should also clear suppressedApprovalKeys to allow re-presented approvals to be forwarded on a fresh connection. Important Files Changed
Sequence DiagramsequenceDiagram
participant Codex as Codex App-Server
participant Watcher as cmux codex-teams watcher
participant Feed as cmux Feed socket
participant UI as Feed UI
Codex->>Watcher: item/commandExecution/requestApproval
Watcher->>Watcher: check suppressed?
alt Not suppressed
Watcher->>Feed: feed.push wait 120s
Feed->>UI: PermissionRequest card
UI-->>Feed: User decision
Feed-->>Watcher: resolved mode always
Watcher->>Codex: decision acceptForSession
else Feed timed out
Watcher->>Watcher: suppressApproval
Note over Watcher,Codex: No response sent
else Suppressed on reconnect
Watcher->>Watcher: return consumed
Note over Watcher,Codex: Request stalls on live connection
end
Reviews (15): Last reviewed commit: "Keep Codex approval fallback native" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| XCTAssertTrue( | ||
| codexHookEntries.contains { | ||
| ($0["command"] as? String)?.contains("hooks codex prompt-submit") == true | ||
| && ($0["timeout"] as? Int) == 5 | ||
| }, | ||
| "Codex lifecycle hooks must use Codex's second-based timeout field, saw \(codexHookEntries)" | ||
| ) | ||
| XCTAssertTrue( | ||
| codexHookEntries.contains { | ||
| ($0["command"] as? String)?.contains("hooks feed --source codex --event PermissionRequest") == true | ||
| && ($0["timeout"] as? Int) == 5 | ||
| }, | ||
| "Codex Feed hooks are telemetry and must use a short second-based timeout, saw \(codexHookEntries)" |
There was a problem hiding this comment.
codexHookEntries is out of scope — compile error
codexHookEntries is only defined inside testGrokHookInstallPinsInstallingCLIAndSocketWithoutCMUXInterpolation (line 2918), a completely different function that reads from .grok/hooks/cmux-session.json. The two new assertions here are inside testCodexHookInstallPrefersLaunchingAppBundledCLI, which still derives only allCommands at line 3116-3121 without ever binding codexHookEntries. The fix is to split the allCommands derivation in testCodexHookInstallPrefersLaunchingAppBundledCLI the same way the Grok function was split — introduce a codexHookEntries binding there and derive allCommands from it.
| let codexHookEntries = hooks.values | ||
| .compactMap { $0 as? [[String: Any]] } | ||
| .flatMap { $0 } | ||
| .compactMap { $0["hooks"] as? [[String: Any]] } | ||
| .flatMap { $0 } | ||
| let allCommands = codexHookEntries |
There was a problem hiding this comment.
The variable is named codexHookEntries but this function (testGrokHookInstallPinsInstallingCLIAndSocketWithoutCMUXInterpolation) reads from .grok/hooks/cmux-session.json. The rename was clearly intended for the Codex test function, and this Grok function only needs it as a stepping stone to allCommands — a neutral name like hookEntries would be more accurate here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Implemented the Codex app-server approval bridge on top of the non-blocking hook fix. Verification:
Research result from openai/codex: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc3f0c9963
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private let readinessLock = NSLock() | ||
| private let stateLock = NSLock() | ||
| private var lastAgentSurfaceId: String? | ||
| private var subscribedThreadIds = Set<String>() |
There was a problem hiding this comment.
This set is kept on the watcher across the outer reconnect loop, but thread/resume subscriptions are tied to the current WebSocket connection. After a transient app-server disconnect, backfillLoadedThreads will call subscribeToThreadIfNeeded on the new connection, see the old thread ids already present, and skip thread/resume, so existing Codex threads stop delivering approval requests/thread updates until the watcher process is restarted.
Useful? React with 👍 / 👎.
| ?? requestIdString(requestId) | ||
| let cwd = stringValue(in: params, keys: ["cwd"]) | ||
| let reason = stringValue(in: params, keys: ["reason"]) | ||
| let command = stringValue(in: params, keys: ["command"]) |
There was a problem hiding this comment.
Codex app-server command approval payloads commonly carry command as an argv array rather than a string; in that case stringValue returns nil and the forwarded Feed item omits both tool_input.command and context.toolSummary. For normal shell approvals the Feed card then cannot show the command the user is being asked to approve, making the new app-server approval path unsafe/confusing even though a command was supplied.
Useful? React with 👍 / 👎.
| guard method == "item/commandExecution/requestApproval" | ||
| || method == "item/fileChange/requestApproval" | ||
| else { return false } | ||
| guard let params = message["params"] as? [String: Any] else { return true } |
There was a problem hiding this comment.
params is absent or not a dictionary, the function returns true (consumed) but never calls connection.respond(...). The Codex app-server issued a JSON-RPC request with a requestId and will block waiting for a reply that never arrives, which stalls that Codex thread indefinitely.
| guard let params = message["params"] as? [String: Any] else { return true } | |
| guard let params = message["params"] as? [String: Any] else { | |
| // Malformed request: still ack with an empty result so the | |
| // app-server doesn't block waiting on a reply that never arrives. | |
| try? connection.respond(requestId: requestId, result: [:]) | |
| return true | |
| } |
| private func pushCodexApprovalToFeed(event: [String: Any]) throws -> [String: Any] { | ||
| try socketClient.sendV2(method: "feed.push", params: [ | ||
| "event": event, | ||
| "wait_timeout_seconds": 120 | ||
| ], responseTimeout: 125) | ||
| } |
There was a problem hiding this comment.
pushCodexApprovalToFeed calls socketClient.sendV2(..., responseTimeout: 125), a synchronous 125-second wait on the same thread that runs listenForNotifications. While one approval is in flight, connection.receiveObject() never returns, so thread-state updates, new approval requests from parallel Codex agents, and all other app-server messages queue in the socket buffer. With multiple concurrent agents or a talkative Codex session, the buffer can fill, causing Codex's send to back-pressure and stall. Consider dispatching each approval to a dedicated thread or using a structured-concurrency child task so the receive loop can keep draining.
There was a problem hiding this comment.
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)
docs/feed.md (1)
165-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate troubleshooting text to match current Codex telemetry behavior.
Line 165 says Feed only sees Codex permission hooks, but this doc now states Feed also records Codex
PreToolUsetelemetry. Please align this sentence with the updated behavior to avoid operator confusion.🤖 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 `@docs/feed.md` at line 165, The sentence claiming "Feed only sees Codex permission hooks" is outdated; update the troubleshooting text to state that Feed records both Codex permission hooks (e.g., request_user_input) and Codex PreToolUse telemetry. Edit the line that currently mentions only permission hooks so it references both types (permission hooks and PreToolUse) and clarify that request_user_input is not a TUI hook but is captured as a permission hook, while PreToolUse telemetry is also recorded by Feed.
🤖 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 18567-18569: The code that constructs threadId calls
stringValue(in:params, keys: ["threadId", "thread_id"]) and then falls back to
stringValue(in:params, keys: ["threadID", "thread_id"]), redundantly checking
"thread_id" twice. Update the second call used for the fallback (the one
referencing "threadID") to only check the unchecked variant (e.g., keys:
["threadID"]) so the fallback sequence becomes stringValue(...
["threadId","thread_id"]) ?? stringValue(... ["threadID"]) ?? "unknown" (refer
to the threadId variable and the stringValue(in:params,keys:) calls).
---
Outside diff comments:
In `@docs/feed.md`:
- Line 165: The sentence claiming "Feed only sees Codex permission hooks" is
outdated; update the troubleshooting text to state that Feed records both Codex
permission hooks (e.g., request_user_input) and Codex PreToolUse telemetry. Edit
the line that currently mentions only permission hooks so it references both
types (permission hooks and PreToolUse) and clarify that request_user_input is
not a TUI hook but is captured as a permission hook, while PreToolUse telemetry
is also recorded by Feed.
🪄 Autofix (Beta)
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
Run ID: 05e2ceb3-1551-41f6-9714-af37e2ffc65e
📒 Files selected for processing (4)
CLI/cmux.swiftSources/Feed/FeedPermissionActionPolicy.swiftcmuxTests/FeedCoordinatorTests.swiftdocs/feed.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d57d329f53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cwd = stringValue(in: params, keys: ["cwd"]) | ||
| let reason = stringValue(in: params, keys: ["reason"]) | ||
| let command = stringValue(in: params, keys: ["command"]) | ||
| let toolName = method == "item/fileChange/requestApproval" ? "Write" : "Bash" |
There was a problem hiding this comment.
When the app-server request is item/fileChange/requestApproval, this builds an actionable Feed card from only the approval request params and labels it as Write, but the Codex app-server docs state that the actual proposed edit details (changes/paths/diff) are carried on the preceding item/started fileChange item, while the approval request itself only carries ids/reason/grantRoot (https://github.com/openai/codex/blob/main/codex-rs/app-server/README.md#file-change-approvals). In file-change approval scenarios, Feed users can therefore approve a write without seeing what file or diff they are approving; cache/correlate the item/started fileChange by itemId and include those changes, or avoid surfacing these as actionable until the details are available.
Useful? React with 👍 / 👎.
| if codexTeamsModeRequestsPersistentApproval(mode), | ||
| codexTeamsAvailableDecisions(params).contains("acceptForSession") { | ||
| return "acceptForSession" | ||
| } | ||
| return "accept" |
There was a problem hiding this comment.
When a Codex command approval advertises persistent choices such as acceptWithExecpolicyAmendment or applyNetworkPolicyAmendment, clicking Feed's newly-enabled Codex “Always Allow” falls through to plain accept unless acceptForSession is present. The app-server docs say clients should prefer availableDecisions and that command approvals can require those amendment object responses (https://github.com/openai/codex/blob/main/codex-rs/app-server/README.md#command-execution-approvals), so this path silently downgrades or may reject persistence for commands that need filesystem/network policy amendments; construct the advertised amendment response from the proposed amendment fields instead of returning accept.
Useful? React with 👍 / 👎.
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/FeedCoordinatorTests.swift`:
- Around line 36-55: The test only exercises absolute -C paths; add cases to
testCodexTeamsValidatesExplicitWorkingDirectoryExists that pass a relative -C
(e.g. a child folder name) and assert validateCodexTeamsWorkingDirectory uses
baseDirectory to resolve and validate the resulting path (mirroring
testCodexTeamsResolvesExplicitWorkingDirectoryFlags and the behavior in
codexTeamsResolvedWorkingDirectory); create the child directory under the
temporary baseDirectory, call CMUXCLI.validateCodexTeamsWorkingDirectory with
commandArgs ["-C", "<child>"] and baseDirectory set to the temp base, assert no
throw for existing child and XCTAssertThrowsError for a missing child path.
🪄 Autofix (Beta)
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
Run ID: be2c8111-c8d2-4159-a461-ed9444134173
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/FeedCoordinatorTests.swift
| func testCodexTeamsValidatesExplicitWorkingDirectoryExists() throws { | ||
| let existing = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-codex-teams-cwd-\(UUID().uuidString)", isDirectory: true) | ||
| try FileManager.default.createDirectory(at: existing, withIntermediateDirectories: true) | ||
| defer { try? FileManager.default.removeItem(at: existing) } | ||
|
|
||
| XCTAssertNoThrow( | ||
| try CMUXCLI.validateCodexTeamsWorkingDirectory( | ||
| commandArgs: ["-C", existing.path], | ||
| baseDirectory: "/tmp" | ||
| ) | ||
| ) | ||
|
|
||
| XCTAssertThrowsError( | ||
| try CMUXCLI.validateCodexTeamsWorkingDirectory( | ||
| commandArgs: ["-C", existing.appendingPathComponent("missing").path], | ||
| baseDirectory: "/tmp" | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify working directory resolution and validation handle absolute vs relative paths consistently
# Search for the implementation of both functions
ast-grep --pattern $'func codexTeamsResolvedWorkingDirectory(commandArgs: $_, baseDirectory: $_) -> $_ {
$$$
}'
ast-grep --pattern $'func validateCodexTeamsWorkingDirectory(commandArgs: $_, baseDirectory: $_) throws {
$$$
}'Repository: manaflow-ai/cmux
Length of output: 42
Clarify -C/--cd/--cwd working-directory semantics
codexTeamsResolvedWorkingDirectorytreats absolute-Cvalues as standardized absolute paths and intentionally ignoresbaseDirectory;validateCodexTeamsWorkingDirectoryvalidates that resolved path exists and is a directory.testCodexTeamsValidatesExplicitWorkingDirectoryExistscovers the absolute branch; for parity withtestCodexTeamsResolvesExplicitWorkingDirectoryFlags, add relative-C <child>validation cases againstbaseDirectory.
🤖 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 `@cmuxTests/FeedCoordinatorTests.swift` around lines 36 - 55, The test only
exercises absolute -C paths; add cases to
testCodexTeamsValidatesExplicitWorkingDirectoryExists that pass a relative -C
(e.g. a child folder name) and assert validateCodexTeamsWorkingDirectory uses
baseDirectory to resolve and validate the resulting path (mirroring
testCodexTeamsResolvesExplicitWorkingDirectoryFlags and the behavior in
codexTeamsResolvedWorkingDirectory); create the child directory under the
temporary baseDirectory, call CMUXCLI.validateCodexTeamsWorkingDirectory with
commandArgs ["-C", "<child>"] and baseDirectory set to the temp base, assert no
throw for existing child and XCTAssertThrowsError for a missing child path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ed2a21e71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| enum FeedPermissionActionPolicy { | ||
| static func supportsPersistentPermissionModes(source: WorkstreamSource) -> Bool { | ||
| source != .codex && source != .hermesAgent | ||
| source != .hermesAgent |
There was a problem hiding this comment.
With this source-only policy, FeedPanelView shows “Always Allow” for every Codex approval, but Codex app-server requests can advertise the exact choices in availableDecisions (the app-server README says clients can use it to render the choices) and the new response helpers still fall back from Feed mode always to plain accept when no persistent Codex decision is available. In those command/file-change prompts, clicking “Always Allow” silently grants only a one-off approval; gate the persistent button on the specific request's available_decisions/grant data instead of enabling it for all Codex items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmuxTests/FeedCoordinatorTests.swift (2)
10-10: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider migrating to Swift Testing.
This test suite adds several new test methods while remaining on XCTest. Swift Testing is the current Apple-supported primitive for unit and integration tests. Since the suite is being actively expanded and doesn't appear to rely on XCTest-specific harnesses (unlike socket-based integration tests), migrating to Swift Testing would improve maintainability and align with project standards.
Migration template:
XCTestCasesubclass →@Suite struct;func testFoo()→@Test func foo();XCTAssertEqual(a, b)→#expect(a == b);XCTUnwrap(x)→try#require(x).🤖 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 `@cmuxTests/FeedCoordinatorTests.swift` at line 10, Change the XCTest-based suite to Swift Testing by replacing the XCTestCase subclass declaration (FeedCoordinatorTests: XCTestCase) with an `@Suite` struct FeedCoordinatorTests and add the Swift Testing import; convert each func testFoo() into `@Test` func foo() and replace XCTest assertions (XCTAssertEqual, XCTAssertTrue, etc.) with `#expect`(expression) equivalents and unwraps (XCTUnwrap) with try `#require`(value); ensure any setUp/tearDown logic is ported into per-test setup or Suite-level fixtures supported by Swift Testing and update test names/signatures to throw if they used try/XCTUnwrap.
83-119:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffValidate nested approval payload structure, not just field presence.
Lines 114-118 use
XCTAssertNotNilto check thatapproval_params,additional_permissions,network_approval_context,command_actions, andproposed_execpolicy_amendmentexist intool_input, but do not validate their actual content. Similarly, line 119 only checks thetypefield ofrelated_item, ignoringid,command, andcwd. These fields are part of the Codex app-server approval contract that Feed UI and decision mapping rely on. IfcodexTeamsFeedEventdrops a field or corrupts the structure, this test will still pass.Validate the actual structure of at least one nested field to ensure the encoding is correct.
🔍 Example: Validate additional_permissions structure
XCTAssertNotNil(toolInput["approval_params"]) -XCTAssertNotNil(toolInput["additional_permissions"]) +let additionalPermissions = try XCTUnwrap(toolInput["additional_permissions"] as? [String: Any]) +let fileSystem = try XCTUnwrap(additionalPermissions["fileSystem"] as? [String: Any]) +XCTAssertEqual(fileSystem["write"] as? [String], ["/tmp/project"]) XCTAssertNotNil(toolInput["network_approval_context"]) XCTAssertNotNil(toolInput["command_actions"]) XCTAssertNotNil(toolInput["proposed_execpolicy_amendment"]) -XCTAssertEqual((toolInput["related_item"] as? [String: Any])?["type"] as? String, "commandExecution") +let relatedItem = try XCTUnwrap(toolInput["related_item"] as? [String: Any]) +XCTAssertEqual(relatedItem["type"] as? String, "commandExecution") +XCTAssertEqual(relatedItem["id"] as? String, "call-1") +XCTAssertEqual(relatedItem["command"] as? String, "touch /tmp/cmux-security-review") +XCTAssertEqual(relatedItem["cwd"] as? String, "/tmp/project")🤖 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 `@cmuxTests/FeedCoordinatorTests.swift` around lines 83 - 119, The test currently only checks presence of nested keys in toolInput; update the assertions to validate one or more nested structures' contents: replace the XCTAssertNotNil checks for "approval_params", "additional_permissions", "network_approval_context", "command_actions", and "proposed_execpolicy_amendment" with concrete equality/structure checks (e.g., assert "additional_permissions" equals ["fileSystem": ["write": ["/tmp/project"]]], assert "network_approval_context" contains host == "example.com", assert "command_actions" is an array whose first element is ["type":"write","path":"/tmp/cmux-security-review"], and assert "proposed_execpolicy_amendment" contains the expected prefix amendment). Also extend the related_item assertion (currently only checking ["type"]) to assert "id" == "call-1", "command" == "touch /tmp/cmux-security-review", and "cwd" == "/tmp/project" so the codexTeamsFeedEvent encoding is verified end-to-end.CLI/cmux.swift (2)
18646-18685:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't embed raw approval payloads in
tool_input.
approval_paramsandrelated_itemcopy whole upstream objects into the Feed event, and the pass-through fields below keep forwarding additional raw nested payloads. This bridge already lifts the user-visible bits it needs (item_id,command,cwd, decisions, etc.); keeping the original dictionaries means new upstream fields can get persisted/streamed unchanged, including patches or policy details that should stay redacted. Build an allowlisted summary instead of shipping the raw blobs.Minimum local hardening
var toolInput: [String: Any] = [ "app_server_method": method, "request_id": requestIdString(requestId), - "item_id": itemId, - "approval_params": params + "item_id": itemId ] @@ - if let relatedItem { - toolInput["related_item"] = relatedItem + if let relatedItem { if command == nil, let relatedCommand = relatedItem["command"] as? String { toolInput["command"] = relatedCommandBased on learnings, workstream/feed payloads in this repo must be allowlisted and redact sensitive fields before persistence or streaming.
🤖 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 18646 - 18685, The code is embedding raw upstream dictionaries into toolInput via "approval_params" and "related_item" (and by copying passthrough fields) which risks leaking sensitive/unexpected fields; replace these with allowlisted summaries: stop assigning params and relatedItem directly, instead build a new summary dictionary (e.g. via helper functions summarizeApprovalParams(params) and summarizeRelatedItem(relatedItem)) that only extracts the specific safe fields you already read (item_id, command, cwd, decisions from codexTeamsDecisionNames, permissions, network_approval_context, additional_permissions, command_actions, proposed*_amendment, grant_root, approval_id, etc.) and put those summaries into toolInput["approval_params"] and toolInput["related_item"]; reuse existing helpers like stringValue(...) and codexTeamsDecisionNames(...) when populating the summary, and ensure any unknown keys are omitted so raw upstream payloads are never forwarded or persisted.Source: Learnings
18745-18798:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on unsupported Feed modes.
These helpers only special-case
deny; every other string falls through to an approval response. If Feed returns"","allow", or any future/invalid value, the bridge currently grants the Codex request instead of rejecting it. Gate on the known allow-set and treat everything else as deny/error.Suggested fix
+ static let codexTeamsAllowedPermissionModes: Set<String> = ["once", "always", "all", "bypass"] + static func codexTeamsCommandApprovalDecision(params: [String: Any], mode: String) -> Any { - if mode == "deny" { return "decline" } + guard codexTeamsAllowedPermissionModes.contains(mode) else { return "decline" } if codexTeamsModeRequestsPersistentApproval(mode), codexTeamsAvailableDecisions(params).contains("acceptForSession") { return "acceptForSession" @@ static func codexTeamsPermissionsApprovalResponse(params: [String: Any], mode: String) -> [String: Any] { - if mode == "deny" { + guard codexTeamsAllowedPermissionModes.contains(mode) else { return [ "permissions": [String: Any](), "scope": "turn" ] }Based on learnings,
WorkstreamPermissionModein this repo only permitsonce,always,all,bypass, anddeny, and unknown values should fail closed.🤖 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 18745 - 18798, The helpers currently treat any non-"deny" mode as allow — change to fail closed by validating mode against the known WorkstreamPermissionMode allow-set ["once","always","all","bypass","deny"]; in codexTeamsCommandApprovalDecision and codexTeamsFileChangeApprovalDecision add a top-level guard that if mode is not in that set then behave as "deny" (return "decline" for command/file-change); in codexTeamsPermissionsApprovalResponse do the same (treat unknown mode as deny and return the same structure used when mode == "deny"); keep codexTeamsModeRequestsPersistentApproval as-is for persistent-checks.Source: Learnings
♻️ Duplicate comments (1)
cmuxTests/FeedCoordinatorTests.swift (1)
36-55: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd relative working-directory validation coverage.
The past review comment requesting relative
-Cpath validation cases remains unaddressed. This test only exercises absolute paths; add cases that validate relative paths resolved againstbaseDirectory, mirroring the structure intestCodexTeamsResolvesExplicitWorkingDirectoryFlags.📋 Suggested test expansion
func testCodexTeamsValidatesExplicitWorkingDirectoryExists() throws { let existing = FileManager.default.temporaryDirectory .appendingPathComponent("cmux-codex-teams-cwd-\(UUID().uuidString)", isDirectory: true) try FileManager.default.createDirectory(at: existing, withIntermediateDirectories: true) defer { try? FileManager.default.removeItem(at: existing) } + + let child = existing.appendingPathComponent("child", isDirectory: true) + try FileManager.default.createDirectory(at: child, withIntermediateDirectories: true) XCTAssertNoThrow( try CMUXCLI.validateCodexTeamsWorkingDirectory( commandArgs: ["-C", existing.path], baseDirectory: "/tmp" ) ) + + XCTAssertNoThrow( + try CMUXCLI.validateCodexTeamsWorkingDirectory( + commandArgs: ["-C", "child"], + baseDirectory: existing.path + ) + ) XCTAssertThrowsError( try CMUXCLI.validateCodexTeamsWorkingDirectory( commandArgs: ["-C", existing.appendingPathComponent("missing").path], baseDirectory: "/tmp" ) ) + + XCTAssertThrowsError( + try CMUXCLI.validateCodexTeamsWorkingDirectory( + commandArgs: ["-C", "missing-child"], + baseDirectory: existing.path + ) + ) }🤖 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 `@cmuxTests/FeedCoordinatorTests.swift` around lines 36 - 55, Expand testCodexTeamsValidatesExplicitWorkingDirectoryExists to include relative-path cases: after creating the temporary directory referenced by `existing`, add assertions that calling CMUXCLI.validateCodexTeamsWorkingDirectory with commandArgs like ["-C", "<relative-path-to-existing>"] and baseDirectory set to a directory that makes that relative path resolve to `existing` succeeds, and that a relative path resolving to a non-existent subpath (e.g., "missing" under that base) throws; use the same helper `existing.appendingPathComponent("missing").path` logic but supply a relative string and appropriate baseDirectory so the function `CMUXCLI.validateCodexTeamsWorkingDirectory` is exercised for relative resolution as in testCodexTeamsResolvesExplicitWorkingDirectoryFlags.
🤖 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 18329-18333: The resetConnectionSubscriptions() function currently
only clears subscribedThreadIds which leaves connection-scoped approval-item
cache (approvalItemById and approvalItemOrder) intact; update
resetConnectionSubscriptions() to also clear
approvalItemById.removeAll(keepingCapacity: true) and
approvalItemOrder.removeAll(keepingCapacity: true) while holding stateLock so
approval-item state is reset on reconnect and no stale relatedItem data leaks
into a new session.
In `@cmuxTests/FeedCoordinatorTests.swift`:
- Around line 206-214: The test only asserts the presence of
acceptWithExecpolicyAmendment but not that the proposedExecpolicyAmendment from
the params is preserved; update the assertion around
CMUXCLI.codexTeamsAppServerApprovalResponse to extract amendmentDecision and
then validate that the returned amendment structure includes the same
proposedExecpolicyAmendment (e.g., compare
amendmentDecision["acceptWithExecpolicyAmendment"] or
amendmentDecision["proposedExecpolicyAmendment"] against the original params
value) so the test fails if the builder drops or mutates the amendment.
- Around line 126-184: The test currently only checks presence of the nested
permissions payload; update
testCodexAppServerPermissionsApprovalBuildsFeedEventAndResponse to assert the
permissions content is preserved end-to-end: after creating permissions and
calling CMUXCLI.codexTeamsFeedEvent, unwrap event["tool_input"] and assert its
"permissions" (or nested "approval_params"->"permissions" if applicable) equals
the original permissions structure; then for the "once" (allow) response from
CMUXCLI.codexTeamsAppServerApprovalResponse, unwrap once["permissions"] and
assert it matches the original permissions dictionary (deep-equality), leaving
the existing "deny" empty assertion intact.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 18646-18685: The code is embedding raw upstream dictionaries into
toolInput via "approval_params" and "related_item" (and by copying passthrough
fields) which risks leaking sensitive/unexpected fields; replace these with
allowlisted summaries: stop assigning params and relatedItem directly, instead
build a new summary dictionary (e.g. via helper functions
summarizeApprovalParams(params) and summarizeRelatedItem(relatedItem)) that only
extracts the specific safe fields you already read (item_id, command, cwd,
decisions from codexTeamsDecisionNames, permissions, network_approval_context,
additional_permissions, command_actions, proposed*_amendment, grant_root,
approval_id, etc.) and put those summaries into toolInput["approval_params"] and
toolInput["related_item"]; reuse existing helpers like stringValue(...) and
codexTeamsDecisionNames(...) when populating the summary, and ensure any unknown
keys are omitted so raw upstream payloads are never forwarded or persisted.
- Around line 18745-18798: The helpers currently treat any non-"deny" mode as
allow — change to fail closed by validating mode against the known
WorkstreamPermissionMode allow-set ["once","always","all","bypass","deny"]; in
codexTeamsCommandApprovalDecision and codexTeamsFileChangeApprovalDecision add a
top-level guard that if mode is not in that set then behave as "deny" (return
"decline" for command/file-change); in codexTeamsPermissionsApprovalResponse do
the same (treat unknown mode as deny and return the same structure used when
mode == "deny"); keep codexTeamsModeRequestsPersistentApproval as-is for
persistent-checks.
In `@cmuxTests/FeedCoordinatorTests.swift`:
- Line 10: Change the XCTest-based suite to Swift Testing by replacing the
XCTestCase subclass declaration (FeedCoordinatorTests: XCTestCase) with an
`@Suite` struct FeedCoordinatorTests and add the Swift Testing import; convert
each func testFoo() into `@Test` func foo() and replace XCTest assertions
(XCTAssertEqual, XCTAssertTrue, etc.) with `#expect`(expression) equivalents and
unwraps (XCTUnwrap) with try `#require`(value); ensure any setUp/tearDown logic is
ported into per-test setup or Suite-level fixtures supported by Swift Testing
and update test names/signatures to throw if they used try/XCTUnwrap.
- Around line 83-119: The test currently only checks presence of nested keys in
toolInput; update the assertions to validate one or more nested structures'
contents: replace the XCTAssertNotNil checks for "approval_params",
"additional_permissions", "network_approval_context", "command_actions", and
"proposed_execpolicy_amendment" with concrete equality/structure checks (e.g.,
assert "additional_permissions" equals ["fileSystem": ["write":
["/tmp/project"]]], assert "network_approval_context" contains host ==
"example.com", assert "command_actions" is an array whose first element is
["type":"write","path":"/tmp/cmux-security-review"], and assert
"proposed_execpolicy_amendment" contains the expected prefix amendment). Also
extend the related_item assertion (currently only checking ["type"]) to assert
"id" == "call-1", "command" == "touch /tmp/cmux-security-review", and "cwd" ==
"/tmp/project" so the codexTeamsFeedEvent encoding is verified end-to-end.
---
Duplicate comments:
In `@cmuxTests/FeedCoordinatorTests.swift`:
- Around line 36-55: Expand
testCodexTeamsValidatesExplicitWorkingDirectoryExists to include relative-path
cases: after creating the temporary directory referenced by `existing`, add
assertions that calling CMUXCLI.validateCodexTeamsWorkingDirectory with
commandArgs like ["-C", "<relative-path-to-existing>"] and baseDirectory set to
a directory that makes that relative path resolve to `existing` succeeds, and
that a relative path resolving to a non-existent subpath (e.g., "missing" under
that base) throws; use the same helper
`existing.appendingPathComponent("missing").path` logic but supply a relative
string and appropriate baseDirectory so the function
`CMUXCLI.validateCodexTeamsWorkingDirectory` is exercised for relative
resolution as in testCodexTeamsResolvesExplicitWorkingDirectoryFlags.
🪄 Autofix (Beta)
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
Run ID: 0de5aea3-8f03-433b-bb9e-26a2a83d09ca
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/FeedCoordinatorTests.swift
| private func resetConnectionSubscriptions() { | ||
| stateLock.lock() | ||
| subscribedThreadIds.removeAll(keepingCapacity: true) | ||
| stateLock.unlock() | ||
| } |
There was a problem hiding this comment.
Clear the approval-item cache on reconnect.
resetConnectionSubscriptions() only drops subscribedThreadIds. approvalItemById and approvalItemOrder survive a new app-server connection, so a reused item id can attach a stale relatedItem from the previous session to a fresh approval request. That can show the wrong command/cwd/patch in Feed and leak prior-session context.
Suggested fix
private func resetConnectionSubscriptions() {
stateLock.lock()
- subscribedThreadIds.removeAll(keepingCapacity: true)
- stateLock.unlock()
+ defer { stateLock.unlock() }
+ subscribedThreadIds.removeAll(keepingCapacity: true)
+ approvalItemById.removeAll(keepingCapacity: true)
+ approvalItemOrder.removeAll(keepingCapacity: true)
}Based on the reconnect flow in this file, the approval-item cache is connection-scoped state and should be reset with the subscription set.
🤖 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 18329 - 18333, The
resetConnectionSubscriptions() function currently only clears
subscribedThreadIds which leaves connection-scoped approval-item cache
(approvalItemById and approvalItemOrder) intact; update
resetConnectionSubscriptions() to also clear
approvalItemById.removeAll(keepingCapacity: true) and
approvalItemOrder.removeAll(keepingCapacity: true) while holding stateLock so
approval-item state is reset on reconnect and no stale relatedItem data leaks
into a new session.
| func testCodexAppServerPermissionsApprovalBuildsFeedEventAndResponse() throws { | ||
| let permissions: [String: Any] = [ | ||
| "network": ["enabled": true], | ||
| "fileSystem": [ | ||
| "read": ["/tmp/read"], | ||
| "write": ["/tmp/write"] | ||
| ] | ||
| ] | ||
| let event = CMUXCLI.codexTeamsFeedEvent( | ||
| method: "item/permissions/requestApproval", | ||
| requestId: "permissions-request", | ||
| params: [ | ||
| "threadId": "thread-1", | ||
| "turnId": "turn-1", | ||
| "itemId": "permissions-call", | ||
| "environmentId": "local", | ||
| "cwd": "/tmp/project", | ||
| "reason": "Need broader access", | ||
| "permissions": permissions | ||
| ], | ||
| workspaceId: "workspace-1" | ||
| ) | ||
|
|
||
| XCTAssertEqual(event["tool_name"] as? String, "request_permissions") | ||
| XCTAssertEqual(event["_opencode_request_id"] as? String, "codex-app-server-permissions-call") | ||
| let toolInput = try XCTUnwrap(event["tool_input"] as? [String: Any]) | ||
| XCTAssertEqual(toolInput["app_server_method"] as? String, "item/permissions/requestApproval") | ||
| XCTAssertNotNil(toolInput["approval_params"]) | ||
| XCTAssertNotNil(toolInput["permissions"]) | ||
|
|
||
| let once = try XCTUnwrap( | ||
| CMUXCLI.codexTeamsAppServerApprovalResponse( | ||
| method: "item/permissions/requestApproval", | ||
| params: ["permissions": permissions], | ||
| mode: "once" | ||
| ) | ||
| ) | ||
| XCTAssertEqual(once["scope"] as? String, "turn") | ||
| XCTAssertNotNil(once["permissions"]) | ||
|
|
||
| let always = try XCTUnwrap( | ||
| CMUXCLI.codexTeamsAppServerApprovalResponse( | ||
| method: "item/permissions/requestApproval", | ||
| params: ["permissions": permissions], | ||
| mode: "always" | ||
| ) | ||
| ) | ||
| XCTAssertEqual(always["scope"] as? String, "session") | ||
|
|
||
| let deny = try XCTUnwrap( | ||
| CMUXCLI.codexTeamsAppServerApprovalResponse( | ||
| method: "item/permissions/requestApproval", | ||
| params: ["permissions": permissions], | ||
| mode: "deny" | ||
| ) | ||
| ) | ||
| XCTAssertEqual(deny["scope"] as? String, "turn") | ||
| XCTAssertEqual((deny["permissions"] as? [String: Any])?.isEmpty, true) | ||
| } |
There was a problem hiding this comment.
Validate permissions payload structure in event and response.
This test builds a nested permissions structure (lines 127-133) but only checks its presence in the event via XCTAssertNotNil (line 154). Similarly, the response validation for once mode (line 164) checks that permissions is not nil but doesn't verify the actual content matches the input structure. Only the deny case (line 183) validates the payload (asserting it's empty).
Validate that the permissions structure flows correctly from params → event tool_input → response payload for at least one allow case.
🔍 Suggested validation
let toolInput = try XCTUnwrap(event["tool_input"] as? [String: Any])
XCTAssertEqual(toolInput["app_server_method"] as? String, "item/permissions/requestApproval")
XCTAssertNotNil(toolInput["approval_params"])
-XCTAssertNotNil(toolInput["permissions"])
+let eventPermissions = try XCTUnwrap(toolInput["permissions"] as? [String: Any])
+let eventFileSystem = try XCTUnwrap(eventPermissions["fileSystem"] as? [String: Any])
+XCTAssertEqual(eventFileSystem["write"] as? [String], ["/tmp/write"])
+XCTAssertEqual(eventFileSystem["read"] as? [String], ["/tmp/read"])
let once = try XCTUnwrap(
CMUXCLI.codexTeamsAppServerApprovalResponse(
method: "item/permissions/requestApproval",
params: ["permissions": permissions],
mode: "once"
)
)
XCTAssertEqual(once["scope"] as? String, "turn")
-XCTAssertNotNil(once["permissions"])
+let oncePermissions = try XCTUnwrap(once["permissions"] as? [String: Any])
+let onceFileSystem = try XCTUnwrap(oncePermissions["fileSystem"] as? [String: Any])
+XCTAssertEqual(onceFileSystem["write"] as? [String], ["/tmp/write"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testCodexAppServerPermissionsApprovalBuildsFeedEventAndResponse() throws { | |
| let permissions: [String: Any] = [ | |
| "network": ["enabled": true], | |
| "fileSystem": [ | |
| "read": ["/tmp/read"], | |
| "write": ["/tmp/write"] | |
| ] | |
| ] | |
| let event = CMUXCLI.codexTeamsFeedEvent( | |
| method: "item/permissions/requestApproval", | |
| requestId: "permissions-request", | |
| params: [ | |
| "threadId": "thread-1", | |
| "turnId": "turn-1", | |
| "itemId": "permissions-call", | |
| "environmentId": "local", | |
| "cwd": "/tmp/project", | |
| "reason": "Need broader access", | |
| "permissions": permissions | |
| ], | |
| workspaceId: "workspace-1" | |
| ) | |
| XCTAssertEqual(event["tool_name"] as? String, "request_permissions") | |
| XCTAssertEqual(event["_opencode_request_id"] as? String, "codex-app-server-permissions-call") | |
| let toolInput = try XCTUnwrap(event["tool_input"] as? [String: Any]) | |
| XCTAssertEqual(toolInput["app_server_method"] as? String, "item/permissions/requestApproval") | |
| XCTAssertNotNil(toolInput["approval_params"]) | |
| XCTAssertNotNil(toolInput["permissions"]) | |
| let once = try XCTUnwrap( | |
| CMUXCLI.codexTeamsAppServerApprovalResponse( | |
| method: "item/permissions/requestApproval", | |
| params: ["permissions": permissions], | |
| mode: "once" | |
| ) | |
| ) | |
| XCTAssertEqual(once["scope"] as? String, "turn") | |
| XCTAssertNotNil(once["permissions"]) | |
| let always = try XCTUnwrap( | |
| CMUXCLI.codexTeamsAppServerApprovalResponse( | |
| method: "item/permissions/requestApproval", | |
| params: ["permissions": permissions], | |
| mode: "always" | |
| ) | |
| ) | |
| XCTAssertEqual(always["scope"] as? String, "session") | |
| let deny = try XCTUnwrap( | |
| CMUXCLI.codexTeamsAppServerApprovalResponse( | |
| method: "item/permissions/requestApproval", | |
| params: ["permissions": permissions], | |
| mode: "deny" | |
| ) | |
| ) | |
| XCTAssertEqual(deny["scope"] as? String, "turn") | |
| XCTAssertEqual((deny["permissions"] as? [String: Any])?.isEmpty, true) | |
| } | |
| func testCodexAppServerPermissionsApprovalBuildsFeedEventAndResponse() throws { | |
| let permissions: [String: Any] = [ | |
| "network": ["enabled": true], | |
| "fileSystem": [ | |
| "read": ["/tmp/read"], | |
| "write": ["/tmp/write"] | |
| ] | |
| ] | |
| let event = CMUXCLI.codexTeamsFeedEvent( | |
| method: "item/permissions/requestApproval", | |
| requestId: "permissions-request", | |
| params: [ | |
| "threadId": "thread-1", | |
| "turnId": "turn-1", | |
| "itemId": "permissions-call", | |
| "environmentId": "local", | |
| "cwd": "/tmp/project", | |
| "reason": "Need broader access", | |
| "permissions": permissions | |
| ], | |
| workspaceId: "workspace-1" | |
| ) | |
| XCTAssertEqual(event["tool_name"] as? String, "request_permissions") | |
| XCTAssertEqual(event["_opencode_request_id"] as? String, "codex-app-server-permissions-call") | |
| let toolInput = try XCTUnwrap(event["tool_input"] as? [String: Any]) | |
| XCTAssertEqual(toolInput["app_server_method"] as? String, "item/permissions/requestApproval") | |
| XCTAssertNotNil(toolInput["approval_params"]) | |
| let eventPermissions = try XCTUnwrap(toolInput["permissions"] as? [String: Any]) | |
| let eventFileSystem = try XCTUnwrap(eventPermissions["fileSystem"] as? [String: Any]) | |
| XCTAssertEqual(eventFileSystem["write"] as? [String], ["/tmp/write"]) | |
| XCTAssertEqual(eventFileSystem["read"] as? [String], ["/tmp/read"]) | |
| let once = try XCTUnwrap( | |
| CMUXCLI.codexTeamsAppServerApprovalResponse( | |
| method: "item/permissions/requestApproval", | |
| params: ["permissions": permissions], | |
| mode: "once" | |
| ) | |
| ) | |
| XCTAssertEqual(once["scope"] as? String, "turn") | |
| let oncePermissions = try XCTUnwrap(once["permissions"] as? [String: Any]) | |
| let onceFileSystem = try XCTUnwrap(oncePermissions["fileSystem"] as? [String: Any]) | |
| XCTAssertEqual(onceFileSystem["write"] as? [String], ["/tmp/write"]) | |
| let always = try XCTUnwrap( | |
| CMUXCLI.codexTeamsAppServerApprovalResponse( | |
| method: "item/permissions/requestApproval", | |
| params: ["permissions": permissions], | |
| mode: "always" | |
| ) | |
| ) | |
| XCTAssertEqual(always["scope"] as? String, "session") | |
| let deny = try XCTUnwrap( | |
| CMUXCLI.codexTeamsAppServerApprovalResponse( | |
| method: "item/permissions/requestApproval", | |
| params: ["permissions": permissions], | |
| mode: "deny" | |
| ) | |
| ) | |
| XCTAssertEqual(deny["scope"] as? String, "turn") | |
| XCTAssertEqual((deny["permissions"] as? [String: Any])?.isEmpty, true) | |
| } |
🤖 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 `@cmuxTests/FeedCoordinatorTests.swift` around lines 126 - 184, The test
currently only checks presence of the nested permissions payload; update
testCodexAppServerPermissionsApprovalBuildsFeedEventAndResponse to assert the
permissions content is preserved end-to-end: after creating permissions and
calling CMUXCLI.codexTeamsFeedEvent, unwrap event["tool_input"] and assert its
"permissions" (or nested "approval_params"->"permissions" if applicable) equals
the original permissions structure; then for the "once" (allow) response from
CMUXCLI.codexTeamsAppServerApprovalResponse, unwrap once["permissions"] and
assert it matches the original permissions dictionary (deep-equality), leaving
the existing "deny" empty assertion intact.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 851b3378d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private func pushCodexApprovalToFeed(event: [String: Any]) throws -> [String: Any] { | ||
| try socketClient.sendV2(method: "feed.push", params: [ | ||
| "event": event, | ||
| "wait_timeout_seconds": 120 | ||
| ], responseTimeout: 125) |
There was a problem hiding this comment.
When the Codex TUI resolves the same app-server approval (the fallback path described here for timed-out/no-decision Feed prompts), the watcher is blocked inside this 120s feed.push wait and never processes the app-server serverRequest/resolved cleanup notification. In that case the Feed card remains pending/actionable after Codex has already proceeded, and a later Feed click can send a stale response for a request that was already resolved; keep reading/resolving app-server cleanup while the Feed prompt is open or cancel the Feed waiter when serverRequest/resolved arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f63044ace
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try connection.respondError( | ||
| requestId: requestId, | ||
| code: -32601, | ||
| message: "cmux Codex Teams watcher does not handle \(method)" |
There was a problem hiding this comment.
When a subscribed Codex thread triggers another server-initiated request (for example mcpServer/elicitation/request or item/tool/call, which the app-server README documents as client-handled JSON-RPC requests), this watcher now sends -32601 for every request that is not one of the approval methods. In those threads, the watcher is an initialized/subscribed client, so this error response can cause the elicitation or dynamic tool call to fail instead of letting the interactive Codex client handle it; avoid subscribing to request types the watcher cannot service or handle/forward them rather than rejecting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c36c8bb. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36c8bb664
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "item/commandExecution/requestApproval", "item/fileChange/requestApproval": | ||
| return ["decision": noDecisionApprovalDecision(params: params)] | ||
| case "item/permissions/requestApproval": | ||
| return permissionsApprovalResponse(params: params, mode: "deny") |
There was a problem hiding this comment.
In Codex Teams, handleApprovalRequest calls this no-decision path when feed.push fails or returns without a Feed click; mapping that case to cancel/decline (or an empty permissions grant) is still an explicit app-server response, so the root Codex TUI cannot fall back to its native approval prompt and the command/edit/permission request is denied or canceled without a user decision. This affects any approval where Feed is unavailable or times out; avoid answering the app-server request as a negative decision when the Feed bridge has no decision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb19d16ff3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try handleApprovalRequest(message, method: method, requestId: requestId, connection: connection) { | ||
| return | ||
| } | ||
| if message["id"] != nil { return } |
There was a problem hiding this comment.
Fresh evidence in this revision is that this branch now drops every server-initiated request with an id that is not one of the approval methods, without sending any JSON-RPC result or error. For subscribed Codex threads that emit other app-server requests such as mcpServer/elicitation/request, item/tool/call, or attestation generation, the app-server expects the connected client to answer; leaving the request unanswered can stall the active turn until timeout instead of letting the interactive client proceed. Either avoid subscribing this watcher to request types it cannot service, or forward/respond to them explicitly.
Useful? React with 👍 / 👎.
| if approvalIsSuppressed(suppressionKey) { | ||
| fputs("cmux codex-teams watcher leaving previously unresolved approval \(suppressionKey) to native Codex\n", stderr) | ||
| return true | ||
| } |
There was a problem hiding this comment.
resetConnectionSubscriptions() clears subscribedThreadIds on reconnect but leaves suppressedApprovalKeys intact. If Codex's app-server re-presents a previously-timed-out approval on the fresh watcher connection — with the same approvalId/itemId but a new JSON-RPC requestId — this guard fires, returns true (consumed), and never calls connection.respond(...). The Codex app-server now has an unanswered JSON-RPC request open on a live connection, so Codex's native TUI cannot step in unless Codex has its own per-request timeout. Because resetConnectionSubscriptions is the natural reset boundary, clearing suppressedApprovalKeys there would let re-presented approvals be forwarded to Feed again on the fresh connection rather than stalling indefinitely.
…erts Review findings on the previous commit: - The notification body carried the full tool summary (complete shell command). Commands can embed credentials, and notification banners reach lock screens, paired phones, and the recorded notification history. The body now names only the tool — the same "<tool> needs approval" string the in-app Feed approval banner uses — never the tool input. - Codex fires PermissionRequest before its own "Approve for me" reviewer (#5507), so an auto-approved request would leave a stale or false "Permission" alert with nothing pending. Codex tool lifecycle progress (PreToolUse/PostToolUse feed events) now clears the pane's notifications, mirroring Claude's pre-tool-use clear_notifications contract. The clear is registry-scoped to sources that raise native approval prompts, so other agents' tool telemetry never touches the notification queue. The immediate notify on PermissionRequest is retained deliberately: codex has no post-reviewer hook, and the wrapper-injected schema already posts this same immediate needs-permission notification via `hooks codex notification`; suppressing until authoritative proof would recreate the silence reported in #9592. Verified against a mock socket: PermissionRequest now emits `notify_target_async <ws> <sf> Codex|Permission|shell needs approval|c=needs-permission;p=0` and PreToolUse/PostToolUse emit `clear_notifications --tab=<ws> --panel=<sf>`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
PermissionRequestFeed hooks as non-blocking telemetry so CodexApprove for mecan run its own reviewer.5) instead of accidentally writing millisecond values into Codex config.Verification
git diff --check origin/main..HEADCMUX_CLI_BIN="/Users/lawrence/Library/Developer/Xcode/DerivedData/cmux-codexp/Build/Products/Debug/cmux DEV codexp.app/Contents/Resources/bin/cmux" python3 tests/test_codex_feed_hooks.py./scripts/reload-cloud.sh --tag codexpDogfood
Use the tagged build and run Codex with hooks installed in
Approve for me. Trigger a command that would have previously shown a FeedPermissionRequest. The expected behavior is that Codex continues through its own approval or auto-review path without waiting for a cmux Feed approval.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Changes permission/approval routing for Codex across hooks, app-server JSON-RPC, and Feed UI; incorrect bridging could block workflows or mis-map security-sensitive decisions.
Overview
Codex permission hooks no longer block on Feed:
PermissionRequestis classified as telemetry (likePreToolUse), feed hooks use zero wait and 5s installed timeouts (seconds, not mistaken millisecond values), so Codex “Approve for me” can run before cmux intercepts.Real Codex approvals move to
cmux codex-teams: a newCodexTeamsApprovalBridgeand watcher intercept app-serverrequestApprovalRPCs, push actionable cards viafeed.push(~120s), map Feed decisions back to Codex (includingavailableDecisions/ policy amendments), and fall through to native Codex when Feed fails or does not resolve—without auto-denying.Feed UI and notifications now gate permission actions per request (once / always / all / bypass) using
tool_input_capabilities, including dynamic macOS notification categories and an “All tools” action.codex-teamsvalidates-C/--cwddirectories and authenticates isolated socket clients for the watcher.Reviewed by Cursor Bugbot for commit bb19d16. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keep Codex
PermissionRequest/PreToolUsehooks non-blocking telemetry and route real approvals through thecmux codex-teamsbridge so Codex “Approve for me” runs first. Gate once/always/all/bypass per request, harden sockets/UI/cwd checks, and fall back to Codex’s native reviewer when Feed can’t resolve or times out.New Features
cmux codex-teamsbridgesitem/commandExecution|fileChange|permissions/requestApprovalto Feed viafeed.push, isolates ~120s waits, ignores unsupported app-server requests, validates-C/--cd/--cwd, authenticates and closes sockets/watchers, and caches up to 500 related items.availableDecisionsand policy amendments; gates once/always/all/bypass per request based on Codex-advertised capabilities (tool_input_capabilities); keeps scopes explicit; file-change approvals are one‑shot by default; preserves persistent approvals when supported.Bug Fixes
PermissionRequestas telemetry (likePreToolUse); use 5s hook timeouts (seconds), clean bad trust hashes, and never auto‑deny on bridge timeouts.Written for commit bb19d16. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes / Improvements
Documentation
Tests