Fix Codex approval request syncing#25
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request introduces approval policy support and pending app-server request tracking to the Codex agent adapter. Changes flow through the adapter layer, unified API, web app state management, schema definitions, and include comprehensive tests validating the new pending-request merging behavior and approval policy propagation. Changes
Sequence DiagramsequenceDiagram
actor App as Web App
participant CA as CodexAgent<br/>Adapter
participant AC as AppServer<br/>Client
participant LS as Live/Read<br/>Sources
AC->>CA: onServerRequest (ThreadConversationRequest)
CA->>CA: Extract threadId,<br/>validate payload
CA->>CA: Store in<br/>pendingAppServerRequests<br/>ByThreadId
CA->>App: onAppServerRequest<br/>(event)
App->>CA: readThread(threadId)
CA->>LS: Fetch thread state
LS-->>CA: Base thread state
CA->>CA: Merge pending requests<br/>into thread.requests
CA-->>App: Merged thread state
App->>CA: submitUserInput(threadId, requestId)
CA->>CA: Locate & remove pending<br/>request from tracking
CA-->>App: Success, pending cleared
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/agents/adapters/codex-agent.ts (1)
678-705:⚠️ Potential issue | 🟠 MajorUse the merged pending-request state when routing replies.
Line 683 still checks only the raw
readThread(false)payload. If a request exists only inpendingAppServerRequestsByThreadIdduring the sync gap, this app-server branch is skipped and the code falls through to IPC ownership checks. That recreates the race this change is trying to close.Suggested fix
- const parsedRoutingThread = parseThreadConversationState(threadForRouting.thread); + const parsedRoutingThread = this.mergePendingAppServerRequests( + parseThreadConversationState(threadForRouting.thread), + ); const routingPendingRequest = findPendingRequestWithId( parsedRoutingThread, input.requestId, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/agents/adapters/codex-agent.ts` around lines 678 - 705, The routing check currently inspects the raw readThread(false) result (threadForRouting → parseThreadConversationState → findPendingRequestWithId) which can miss requests stored in pendingAppServerRequestsByThreadId; modify the flow in the codex-agent method so that after parsing threadForRouting you merge in pending app-server requests (use the existing mergePendingAppServerRequests(parsedRoutingThread) or equivalent) and then call findPendingRequestWithId against that merged state; ensure subsequent actions (submitUserInput, removePendingAppServerRequest, refreshing the thread, updating streamSnapshotByThreadId/streamSnapshotOriginByThreadId, and setThreadTitle) run only when the merged-state pending request is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/agents/adapters/codex-agent.ts`:
- Around line 153-162: The current code rebuilds the request object and only
parses id/method/params which drops other top-level fields (e.g., completed) and
hides schema mismatches; change the parse to run
ThreadConversationRequestSchema.safeParse(message.value) (i.e., pass the full
incoming message.value instead of reconstructing it), and if
parsedRequest.success is false, fail hard or log the zod error details before
returning so schema mismatches are visible and completed requests aren’t
re-cached as pending; update any downstream use of parsedRequest to reference
parsedRequest.data accordingly.
In `@apps/web/src/App.tsx`:
- Around line 870-886: The sync signature function requestProgressSignature
currently only uses request.id, method, and completed, so mutations to the
request body (e.g., reason, questions, availableDecisions) are ignored; update
requestProgressSignature (and the sibling signature used in the other block) to
include serialized, deterministic representations of the request body fields you
care about (at least request.reason, request.questions, and
request.availableDecisions) — e.g., append JSON.stringify of those fields (or
join their values) alongside id/method/completed, ensuring null/undefined are
normalized to a stable string so the signature changes whenever those body
fields change and setLiveState()/setReadThreadState() will detect updates.
- Around line 670-710: mergePendingRequests currently only tracks keys for
non-completed items from the primary, so completed requests present in either
source can be reintroduced from the fallback; fix this by computing a set of
completed keys from both primary and secondary (collect buildPendingRequestKey
for requests where completed === true) and then only include pending requests
whose key is not in that completedKeys set; update mergePendingRequests (used by
buildRequestSourceState) to skip any secondary request whose key is listed as
completed in either source so read-thread fallbacks cannot reopen resolved
requests, and ensure buildRequestSourceState continues to only merge when both
liveConversationState and readConversationState are present.
In `@packages/codex-api/src/app-server-transport.ts`:
- Around line 17-20: Change the AppServerTransport interface so
setIncomingMessageHandler is required (remove the optional ?), then update the
call in AppServerClient.setIncomingMessageHandler to invoke
transport.setIncomingMessageHandler(...) without optional chaining so missing
implementations fail at compile time; update any transport implementations and
test mocks to implement setIncomingMessageHandler(handler: (message:
JsonRpcIncomingMessage) => void | null): void accordingly.
---
Outside diff comments:
In `@apps/server/src/agents/adapters/codex-agent.ts`:
- Around line 678-705: The routing check currently inspects the raw
readThread(false) result (threadForRouting → parseThreadConversationState →
findPendingRequestWithId) which can miss requests stored in
pendingAppServerRequestsByThreadId; modify the flow in the codex-agent method so
that after parsing threadForRouting you merge in pending app-server requests
(use the existing mergePendingAppServerRequests(parsedRoutingThread) or
equivalent) and then call findPendingRequestWithId against that merged state;
ensure subsequent actions (submitUserInput, removePendingAppServerRequest,
refreshing the thread, updating
streamSnapshotByThreadId/streamSnapshotOriginByThreadId, and setThreadTitle) run
only when the merged-state pending request is found.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5e02bfc-487e-4f03-bfcb-7a2cb44dc393
📒 Files selected for processing (10)
apps/server/src/agents/adapters/codex-agent.tsapps/server/src/index.tsapps/server/src/unified/adapter.tsapps/web/src/App.tsxapps/web/test/app.test.tsxpackage.jsonpackages/codex-api/src/app-server-client.tspackages/codex-api/src/app-server-transport.tspackages/codex-api/src/json-rpc.tspackages/codex-api/test/json-rpc.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/codex-api/src/app-server-transport.ts`:
- Around line 104-106: Wrap the incomingMessageHandler invocation in a try/catch
to prevent handler-thrown exceptions from breaking the transport loop: replace
the direct call to incomingMessageHandler?.(message) with a guarded call inside
try { this.incomingMessageHandler?.(message); } catch (err) {
this.handleTransportError?.(err, message); return; } so any exception is
surfaced via a transport-level error handler (implement handleTransportError if
missing) and the loop returns safely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2c775d6-c235-4856-a6ed-14248dbcbe20
📒 Files selected for processing (6)
apps/server/src/agents/adapters/codex-agent.tsapps/web/src/App.tsxapps/web/test/app.test.tsxpackages/codex-api/src/app-server-client.tspackages/codex-api/src/app-server-transport.tspackages/codex-api/test/app-server-client.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/App.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/codex-api/src/app-server-client.ts
- apps/server/src/agents/adapters/codex-agent.ts
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)
apps/web/src/App.tsx (1)
917-925:⚠️ Potential issue | 🟠 MajorInclude
liveStateErrorin the live-state sync signature.
buildRequestSourceState()and the chat warning banner now depend onliveState.liveStateError, butsetLiveState()still dedupes without it. If the reducer fails or recovers without changingconversationState, the app will keep the previousliveState, so request merging can stay in the wrong mode and the warning banner will not toggle.🔧 Minimal fix
function buildLiveStateSyncSignature( state: LiveStateResponse | null | undefined, ): string { if (!state) { return ""; } const conversationState = state.conversationState; return [ state.threadId, state.ownerClientId ?? "", + JSON.stringify(state.liveStateError ?? null), String(getConversationStateUpdatedAt(conversationState)), String(conversationState?.turns.length ?? -1), modeSelectionSignatureFromConversationState(conversationState), conversationProgressSignature(conversationState), requestProgressSignature(conversationState), ].join("|"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/App.tsx` around lines 917 - 925, The live-state dedupe signature built in the function that returns the joined array (used by setLiveState) omits liveState.liveStateError, causing reducers or recoveries that only change liveStateError to be ignored; update that signature builder (the function returning state.threadId, state.ownerClientId..., modeSelectionSignatureFromConversationState, conversationProgressSignature, requestProgressSignature) to include the current liveState.liveStateError (e.g., stringify it or default to empty) so setLiveState will treat changes to liveStateError as a different signature and trigger updates; ensure any callers like buildRequestSourceState and setLiveState rely on the new signature shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/App.tsx`:
- Around line 1596-1599: The computed canEditThreadMode currently allows brief
editability because activeThreadAgentId can fall back to selectedAgentId; update
the guard to also require hasResolvedSelectedThreadProvider so controls stay
disabled until the thread's provider is resolved (i.e., include a boolean check
for hasResolvedSelectedThreadProvider in the canEditThreadMode expression).
Additionally, in the toolbar handlers that call applyModeDraft (referenced as
applyModeDraft and the local mode/model/effort update handlers), early-return if
!hasResolvedSelectedThreadProvider to avoid mutating local state and invoking
applyModeDraft before resolution.
---
Outside diff comments:
In `@apps/web/src/App.tsx`:
- Around line 917-925: The live-state dedupe signature built in the function
that returns the joined array (used by setLiveState) omits
liveState.liveStateError, causing reducers or recoveries that only change
liveStateError to be ignored; update that signature builder (the function
returning state.threadId, state.ownerClientId...,
modeSelectionSignatureFromConversationState, conversationProgressSignature,
requestProgressSignature) to include the current liveState.liveStateError (e.g.,
stringify it or default to empty) so setLiveState will treat changes to
liveStateError as a different signature and trigger updates; ensure any callers
like buildRequestSourceState and setLiveState rely on the new signature shape.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 834f6e0e-7ee8-4e53-8739-2afc8c9891ab
📒 Files selected for processing (4)
apps/server/src/agents/adapters/codex-agent.tsapps/web/src/App.tsxpackages/codex-api/src/app-server-client.tspackages/codex-api/src/app-server-transport.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/agents/adapters/codex-agent.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/main.tsx`:
- Around line 20-24: The service-worker cleanup call using
navigator.serviceWorker.getRegistrations().then(...registration.unregister())
currently ignores failures; update the logic to handle errors by awaiting the
unregister operations inside an async function or promise chain and adding a
.catch that logs the error (e.g., include the registration or error details) so
failures from navigator.serviceWorker.getRegistrations and
registration.unregister() are not dropped; locate the call to
navigator.serviceWorker.getRegistrations and modify the promise chain around
Promise.all(registrations.map(registration => registration.unregister())) to
catch and log any thrown errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0418347f-4010-4d84-8edd-e92793dbdff6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/server/src/agents/adapters/codex-agent.tsapps/server/src/agents/types.tsapps/server/src/unified/adapter.tsapps/server/test/codex-agent-live-state.test.tsapps/server/test/unified-adapter.test.tsapps/web/src/lib/api.tsapps/web/src/main.tsxpackages/codex-protocol/src/thread.tspackages/codex-protocol/test/protocol.test.tspackages/unified-surface/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/codex-protocol/test/protocol.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/unified/adapter.ts
Owner
|
Codex here, acting on behalf of @achimala. I pushed a fix for the live-state parse failure we hit while testing this PR. The issue was that PR #25 makes Farfield parse full What changed:
Verified locally:
I also verified the exact failing local thread ( Note: I tried to start the broader rebase cleanup for #25, but the branch has substantial conflicts against current |
Owner
|
Codex here, acting on behalf of @achimala. I pushed one more small UI recovery fix after local testing. The direct thread route can briefly ask for a Codex thread before Farfield has registered the provider owner for that thread, which produces What changed:
Verified locally:
I also reloaded the same local thread in the in-app browser and confirmed the thread renders with no error bar. |
achimala
force-pushed
the
codex/fix-approval-request-sync
branch
from
c6cfbd4 to
11934d5
Compare
Owner
|
Codex here, acting on behalf of @achimala. I rebased this PR onto current What changed in the cleanup:
Repo-rule check on the final diff is clean: no Verified locally:
The PR is mergeable now. CodeRabbit is currently re-reviewing the pushed head. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/agents/adapters/codex-agent.ts`:
- Around line 304-307: The handler uses request.params.threadId but some request
types (applyPatchApproval, execCommandApproval) extract the thread id as
params.conversationId; update handleServerRequest and any similar
refresh/broadcast logic (also the other occurrence around the block at lines
2353-2370) to use the already-extracted thread id (conversationId) when present:
read the thread id from the extracted value (e.g., the variable set by
capturePendingAppServerRequest or the helper that pulled params.conversationId)
and pass that into refresh/broadcast/update calls instead of
request.params.threadId so all app-server request types route to the correct
thread.
- Around line 546-549: mergePendingAppServerRequests currently only appends
cached pending entries so stale requests that were resolved upstream never get
evicted; update the logic (called from applyPendingCollaborationMode around
parseThreadConversationState in readThread/readLiveState flows) to compare
cached pending request IDs against the authoritative request IDs from
parseThreadConversationState and remove any cached entries not present upstream
before merging (i.e., perform an eviction pass or replace cached entries for
missing IDs), ensuring mergePendingAppServerRequests returns a clean
conversation state without reintroducing resolved/stale requests.
In `@apps/web/src/App.tsx`:
- Around line 2575-2579: The current branch in the shouldLoadStreamEvents check
calls setError("") which clears the global error banner; change this to only
clear the transient thread-registration error by comparing the existing error
state (or an error type/flag) before clearing so unrelated errors
(catalog/settings) are not dismissed. Locate the block using
shouldLoadStreamEvents, selectedThreadIdRef.current and threadId, and replace
the unconditional setError("") with logic that checks the current error is the
thread-load/transient error (or a dedicated threadLoadError state) and only then
clear it; apply the same scoped fix in the other similar block around lines
referencing the same symbols (the 2594–2608 region).
- Around line 757-810: mergePendingRequests currently prefers the primary (live)
request when keys collide; change it so the secondary (readThread) copy wins on
key collisions. Update mergePendingRequests (and its use in
buildRequestSourceState) to: compute completedKeys from both sources as now,
start merged from secondary.filter(request => request.completed !== true &&
!completedKeys.has(buildPendingRequestKey(request))), track seenKeys from that
merged set, then iterate primary and only add requests whose key is not in
seenKeys and not completed. This ensures buildPendingRequestKey collisions
prefer the read-thread copy.
In `@packages/unified-surface/src/index.ts`:
- Line 1026: Replace the loose string schema for the approvalPolicy field with a
Zod enum of the canonical values so validation fails fast; specifically change
the approvalPolicy schema (the property named approvalPolicy in the relevant Zod
object) from z.string().optional() to a Zod enum with values
"untrusted","on-failure","on-request","never" (and mark it optional if needed),
and make the corresponding change at the other location where approvalPolicy is
defined earlier in the file (the other approvalPolicy schema around the earlier
definition at ~line 988) so both schema boundaries use z.enum([...]).optional()
for consistent, strict validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84bec153-89a0-40ae-9411-8e9058d439f1
📒 Files selected for processing (12)
apps/server/src/agents/adapters/codex-agent.tsapps/server/src/agents/types.tsapps/server/src/unified/adapter.tsapps/server/test/codex-agent-live-state.test.tsapps/server/test/unified-adapter.test.tsapps/web/src/App.tsxapps/web/src/lib/api.tsapps/web/src/main.tsxapps/web/test/app.test.tsxpackages/codex-api/test/json-rpc.test.tspackages/codex-protocol/test/protocol.test.tspackages/unified-surface/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/codex-protocol/test/protocol.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/codex-api/test/json-rpc.test.ts
- apps/server/test/unified-adapter.test.ts
- apps/web/test/app.test.tsx
- apps/web/src/main.tsx
- apps/server/test/codex-agent-live-state.test.ts
Owner
|
Codex here, acting on behalf of @achimala. I pushed one more follow-up commit, Fixes in this follow-up:
Verification: |
Owner
|
Codex acting on behalf of @achimala here. I pushed one more follow-up commit, 0508571, after testing the approval flow in Farfield against thread 019dccf7-d79b-7e10-b39e-301373c87360. What changed:
Verified:
Leaving the PR paused for maintainer local testing before merge. |
achimala
approved these changes
Apr 27, 2026
Owner
|
Thanks for the contribution, @lelonco. This is merged now. Codex acting on behalf of @achimala made a few maintainer follow-ups before merging to keep the approval sync path aligned with current Farfield protocol/server/web behavior. Verified locally with typecheck, the full test suite, the repo rule check, and an in-app browser approval-flow test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests