fix(codex): handle app-server approvals and idle completion#787
fix(codex): handle app-server approvals and idle completion#7870KABE wants to merge 1 commit intochenhg5:mainfrom
Conversation
chenhg5
left a comment
There was a problem hiding this comment.
Review Summary
Solid implementation of server-initiated approval requests and improved turn completion detection for Codex app-server. The code is well-structured with proper concurrency handling.
✅ Good:
-
Approval flow: Clean separation of
pendingApprovalsmap with proper mutex protection. The goroutine-based timeout pattern (5min) with select on ctx/timer is correct. -
Message type discrimination: The
readLoopswitch onhasID && !hasMethod/hasID && hasMethod/defaultis a clean way to distinguish responses, server requests, and notifications. -
Turn completion deduplication:
completeTurn()checkscurrentTurnand clears it atomically, preventing duplicateEventResultemissions. -
thread/status/changedhandling: Correctly detects codex 0.125+ idle status as turn completion signal. -
Cleanup:
rejectPendingApprovals()properly drains all pending channels on connection close. -
Tool name mapping:
handleApprovalRequesttranslatesitem/commandExecution/requestApproval→ "Bash",item/fileChange/requestApproval→ "Patch" for user-friendly display.
💭 Observations (non-blocking):
connect()now hardcodes--listen stdio://instead of conditionally adding--listenwhen URL is set. This is correct for the stdio transport use case.handleDynamicToolCallreturns a stub error response — appropriate for now since dynamic tools aren't supported.
LGTM. CI green, well-tested pattern, addresses real Codex protocol evolution.
This PR fixes codex app-server session handling in cc-connect by adding support for server-initiated approval requests and by improving turn completion detection for newer Codex versions.
Previously, the app-server adapter mainly handled client-initiated RPC requests and notifications. With newer Codex behavior, the server may also send approval requests that require a response, and turn completion may be signaled via thread status changes instead of only turn/completed. This patch updates the adapter to handle those cases correctly.