Repository navigation
fix: codex provider reliability and session resume - #247
Merged
Merged
Conversation
Set disposed=true in onClose/onError handlers before calling rejectAll to
prevent callers from writing to a destroyed stdin after stream teardown.
Add stdout.setEncoding("utf8") so Node emits string chunks, removing the
redundant .toString() call on each data event.
…ion listener early Replace public mutable fields isAlive and threadId with private backing fields and public read-only getters. Move the notification listener registration to immediately after RPC client construction so notifications emitted during the handshake are not lost.
Replaces the @openai/codex-sdk adapter with a direct JSON-RPC 2.0 integration via CodexAppServer and CodexEventMapper. Each session keeps one persistent subprocess alive between turns; turn/start is sent per message and completion is awaited via turn.completed/turn.failed notifications. Adds sendTurn() helper to CodexAppServer to avoid exposing the internal RPC client. Includes version gating (min 0.37.0) and idle-session eviction unchanged from the prior implementation.
The mapper parameter was never used inside runTurn's body. It was passed in solely to keep the signature consistent with call sites, with a void mapper expression to silence the unused-variable lint warning. The mapper is already accessible via the notification listener closure wired in sendMessage, so the parameter is unnecessary.
Add dispose() calls to existing tests to clean up stream listeners, and add four new test cases covering: sendNotification on a disposed client, stdout error event rejection, RPC error response rejection, and stdout end event rejection.
Extract the inline Provider Architecture Convention from AGENTS.md into docs/guides/provider-architecture.md and replace it with a reference link. Fix the Codex Provider section in ARCHITECTURE.md to use the correct numbering (8.4) and formatting style consistent with 8.3 Claude Provider.
Add AgentEventType const object and union type to packages/contracts so call sites reference named constants instead of bare strings. Prevents typos, enables rename refactors to propagate at compile time, and improves autocomplete for all provider and service code. Update all event construction and type comparison sites in: - codex-event-mapper.ts - codex-provider.ts - claude-provider.ts - agent-service.ts - index.ts
…ations
The codex app-server turn/start RPC requires input to be a sequence, not
a bare string. Plain-text messages were being sent as strings causing:
Invalid request: invalid type: string "hey", expected a sequence
Fix in two layers:
- buildCodexInput always returns TurnInputPart[] (removed string return path)
- sendTurn coerces any string input to [{type:text, text}] as a safety net
Also filter lifecycle notifications (thread/*, codex/event/*) in
CodexAppServer before forwarding to the turn mapper. These are server
housekeeping events, not turn events, and were producing spurious warn
logs from CodexEventMapper.
The codex app-server carries the threadId in the thread/started notification rather than in the thread/start RPC response result. The previous fix that filtered lifecycle notifications before forwarding them to the turn mapper broke handshake because it also silenced thread/started before the threadId could be extracted. Fix: in runHandshake, attach a raw listener directly on the RPC client before sending thread/start. This captures the threadId from thread/started even though the lifecycle filter still suppresses it from reaching the turn mapper. The RPC response result is tried first; the notification is the fallback. An explicit error is thrown if neither source provides a threadId. Also add a pre-flight guard in sendTurn to fail fast with a clear message rather than letting the server reject a null threadId.
The previous attempt captured the notification threadId via a variable set inside a listener removed in the finally block. If the notification arrives in a separate I/O chunk after the RPC response, the finally block runs first (await creates a microtask boundary) and the listener is already gone. Replace the variable approach with a persistent promise. The listener and its timeout (3s) outlive the sendRequest await, so the notification is captured regardless of chunk timing. The error message now includes the raw response payload to help diagnose if neither source carries the threadId.
The codex app-server (>= 0.104.0) returns the session ID nested at result.thread.id rather than result.threadId. Accept both shapes so the code works across protocol versions.
codex app-server >= 0.104.0 uses a different notification protocol than originally assumed. Observed events: turn/started, item/started, item/completed, account/rateLimits/updated, error, turn/completed. - Replace assumed turn.event/turn.completed/turn.failed (dot-notation) with actual slash-notation protocol in codex-types.ts - Add account/ prefix to LIFECYCLE_NOTIFICATION_PREFIXES to silence account/rateLimits/updated before it reaches the mapper - Rewrite CodexEventMapper to handle item/completed (message text and function_call tool use) and turn/completed; turn/started and item/started are silently consumed - Update runTurn completion detection from turn.completed to turn/completed - Rewrite test suite against the actual notification protocol
Diagnostics revealed the actual notification structure: - item/completed echoes the user message with type 'userMessage' (silently consumed) - turn/completed wraps result in params.turn, not at the top level - turn/completed with params.turn.status 'failed' carries the error in params.turn.error - usage is at params.turn.usage, not params.usage Changes: - Update TurnCompletedPayload to reflect actual shape (turn: TurnResult wrapper) - Emit Error event (not TurnComplete) when turn/completed has status 'failed' - Read usage from params.turn.usage - Silence userMessage item type in item/completed - Accept both 'text' and 'output_text' content part types for assistant messages
When a session.error event fires, a synthetic system message is appended
to the thread's message list so the error is visible in the chat history
rather than silently updating only the sidebar badge and internal state.
- threadStore: on session.error, create a synthetic Message with
role 'system' and JSON content {__type: 'agent_error', message: '...'}
and append it to the visible message list
- MessageBubble: detect the agent_error marker and render an inline
error card with destructive styling (red border/background, AlertCircle
icon) that matches the chat's design language
The actual streaming protocol sends assistant text token-by-token via item/agentMessage/delta with params.delta containing each token. The item/completed notification only echoes the user message (userMessage type) and does not carry assistant text. - Add AgentMessageDeltaPayload type and item/agentMessage/delta to CodexNotification union - Handle in mapper: accumulate params.delta into lastAssistantText and emit TextDelta per token - turn/completed then emits Message with the fully accumulated text - Remove diagnostic raw: logging from unrecognized notification warn
Updates types, event mapper, and app-server to match the canonical codex app-server >= 0.104.0 protocol from the openai/codex source repo. Key changes: - Add handlers for "message" items (OpenAI Responses API shape with content array) and "function_call" items alongside existing codex-native types - Add "item/commandExecution/outputDelta" handler to buffer streaming shell output per item ID, used when item/completed fires - Add handlers for commandExecution, fileChange, mcpToolCall, and dynamicToolCall item/completed types emitting ToolUse + ToolResult - Fix "error" notification to read params.error.message (canonical shape) and only surface non-retried errors via willRetry flag - Add SILENCED_METHODS and SILENT_ITEM_TYPES sets to suppress known informational notifications without warn-level noise - Expand LIFECYCLE_NOTIFICATION_PREFIXES with account/, hook/, rawResponseItem/, serverRequest/, mcpServer/, fuzzyFileSearch/, windows, app/, fs/, thread/realtime/ - Fix CompletedItem type to cover all ThreadItem variant fields - Fix error notification test to use canonical params.error.message shape
Tracks sandbox mode per SessionEntry. If the user changes permission mode (e.g. supervised → full access) mid-session, kills the old app-server process and starts a fresh thread so the new sandbox mode takes effect. Without this, the running session would keep the sandbox mode it was started with, ignoring the user's selection.
Root cause: thread/start used wrong field names (workingDirectory instead of cwd, sandboxMode instead of sandbox). These fields were silently ignored, so the sandbox defaulted to read-only and no approval policy was set - causing the agent to block on approval prompts indefinitely. Key changes: - thread/start: use cwd, sandbox, approvalPolicy (was workingDirectory, sandboxMode) - thread/resume: pass sandbox + approvalPolicy overrides so resumed threads pick up current user settings - Auto-approval responses: route by method with correct enum values per type (acceptForSession for v2, approved_for_session for deprecated) - Permissions requests: grant full filesystem + network for the session - tool_call_records: use INSERT OR IGNORE to handle duplicate IDs from resumed sessions
When thread/resume fails, the codex provider now emits a context_lost system event and logs a warning instead of silently falling back to a fresh thread/start. This makes context loss visible for debugging instead of leaving the user wondering why the agent forgot everything.
thread/resume returned the thread ID at result.thread.id (nested) but we only read result.threadId (flat). The ID was silently undefined, so the code fell through to thread/start every restart - losing context. Also fixes review findings: - Accept both response shapes in thread/started notification handler - Add sendResponse to RPC client for server-initiated request replies - Auto-approve codex approval prompts (command, file, permissions) - Pass model + effort per-turn via turn/start and thread/resume - Map reasoningLevel to codex effort field (max -> high) - Hoist SILENCED_METHODS and SILENT_ITEM_TYPES to module-level constants - Fix commandOutputBuffers lookup miss when item.id is absent - Use distinct fc-/fchg- prefixes to avoid toolCallId collisions - Race SIGTERM grace period against exit event instead of fixed 3s sleep - Expand recoverable thread/resume error patterns - Validate cliPath against shell metacharacters before spawn - Persist rotated thread IDs mid-session via threadIdChanged event - Close double-ended emission race between fatal handler and runTurn - Fix log messages to use past tense per project convention
📝 WalkthroughWalkthroughReplaces the external Codex SDK with an in-repo persistent Changes
Sequence DiagramsequenceDiagram
participant App as Application / Provider
participant ServerMgr as CodexAppServer
participant RPC as CodexRpcClient
participant Proc as codex app-server (child)
participant Mapper as CodexEventMapper
App->>ServerMgr: start() / init session
activate ServerMgr
ServerMgr->>Proc: spawn child process
ServerMgr->>RPC: create client over stdin/stdout
ServerMgr->>RPC: sendRequest("initialize")
RPC->>Proc: NDJSON JSON-RPC initialize
Proc-->>RPC: initialize response
RPC-->>ServerMgr: resolved
ServerMgr->>RPC: sendRequest("model/list") (best-effort)
RPC->>Proc: model/list
Proc-->>RPC: model/list response
ServerMgr->>RPC: sendRequest("thread/resume")
alt resume success
Proc-->>RPC: thread/resume response -> ServerMgr (threadId)
else resume fail
Proc-->>RPC: error -> ServerMgr
ServerMgr->>RPC: sendRequest("thread/start")
Proc-->>RPC: thread/start response -> ServerMgr (threadId)
end
deactivate ServerMgr
App->>ServerMgr: sendTurn(input)
activate ServerMgr
ServerMgr->>RPC: sendRequest("turn/start", input)
loop streaming notifications
Proc-->>RPC: notification (NDJSON)
RPC-->>ServerMgr: notification event
ServerMgr->>Mapper: mapNotification(...)
Mapper-->>ServerMgr: [AgentEvent...]
ServerMgr-->>App: emit mapped AgentEvents
end
Proc-->>RPC: turn/completed
RPC->>Mapper: mapNotification(turn/completed)
Mapper-->>ServerMgr: [Message, TurnComplete]
ServerMgr-->>App: emit final events
deactivate ServerMgr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 9
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/stores/threadStore.ts (1)
1255-1270:⚠️ Potential issue | 🟡 MinorClear
streamingPreviewByThreadinsession.errorcleanup too.
session.errorremovesstreamingByThread[threadId]but leavesstreamingPreviewByThread[threadId], which can keep stale preview UI state after failures.Suggested patch
set((state) => { const nextRunning = new Set(state.runningThreadIds); nextRunning.delete(threadId); const nextStreaming = { ...state.streamingByThread }; delete nextStreaming[threadId]; + const nextStreamingPreview = { ...state.streamingPreviewByThread }; + delete nextStreamingPreview[threadId]; const nextStartTimes = { ...state.agentStartTimes }; delete nextStartTimes[threadId]; const nextToolCalls = { ...state.toolCallsByThread }; delete nextToolCalls[threadId]; const nextSubagents = { ...state.activeSubagentsByThread }; @@ const base = { error: errorMsg, runningThreadIds: nextRunning, streamingByThread: nextStreaming, + streamingPreviewByThread: nextStreamingPreview, agentStartTimes: nextStartTimes, toolCallsByThread: nextToolCalls, activeSubagentsByThread: nextSubagents, isCompactingByThread: nextCompacting, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/stores/threadStore.ts` around lines 1255 - 1270, The error cleanup currently removes streamingByThread[threadId] but leaves streamingPreviewByThread[threadId], leaving stale preview UI state; update the session.error branch in threadStore.ts to also remove the preview entry by creating nextStreamingPreview (clone state.streamingPreviewByThread, delete nextStreamingPreview[threadId]) and include streamingPreviewByThread: nextStreamingPreview in the base object alongside streamingByThread: nextStreaming so the preview state is cleared when an error occurs.
🧹 Nitpick comments (3)
apps/web/src/components/chat/MessageBubble.tsx (1)
153-161: Prefer a shadcn alert primitive for the new error banner.This custom alert-style block should use an existing UI primitive from
apps/web/src/components/ui/(if available) instead of bespoke container markup in this component.As per coding guidelines
apps/web/src/components/**/*.{ts,tsx}: Always use existing shadcn primitives from apps/web/src/components/ui/ before creating custom UI elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/MessageBubble.tsx` around lines 153 - 161, Replace the bespoke error banner in MessageBubble.tsx (the block that checks const agentError = parseAgentError(message.content) and returns a custom div with AlertCircle) with the shadcn Alert primitive from apps/web/src/components/ui/ (e.g., import and use Alert, AlertTitle/AlertDescription or Alert + appropriate slots) so styling and accessibility are consistent; keep the existing AlertCircle icon and agentError text but move them into the Alert primitive’s title/description or content slots, preserving classes for spacing if needed and removing the custom border/bg markup.apps/web/src/stores/threadStore.ts (1)
1229-1241: Normalize error payload to a guaranteed string before serializing.If
params.erroris ever non-string at runtime,messagein the synthetic JSON can become non-string and the dedicated agent-error UI parser will skip it.Suggested patch
- const errorMsg = (params.error as string) || "Unknown error"; + const rawError = params.error; + const errorMsg = + typeof rawError === "string" + ? rawError + : rawError instanceof Error + ? rawError.message + : rawError != null + ? JSON.stringify(rawError) + : "Unknown error";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/stores/threadStore.ts` around lines 1229 - 1241, The synthetic agent error message may serialize a non-string payload and break the agent-error UI; ensure the message is normalized to a string before embedding in content by coercing params.error (or the local errorMsg) to a guaranteed string (e.g., String(params.error) or JSON.stringify for objects) when constructing the errorMessage object so the content JSON has "message" as a string and sequence uses get().messages.length + 1 as before.apps/server/src/providers/codex/__tests__/codex-version.test.ts (1)
42-130: Add a regression test for unsafecliPathrejection.Given the shell-based version check, add one test that verifies invalid shell-control characters are rejected before calling
spawnSync.✅ Test case to add
describe("checkCodexVersion error messages", () => { beforeEach(() => { vi.clearAllMocks(); }); + + it("rejects unsafe cliPath before spawning", () => { + const result = checkCodexVersion("%ComSpec% /c whoami"); + expect(result.ok).toBe(false); + expect(mockSpawnSync).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex/__tests__/codex-version.test.ts` around lines 42 - 130, Add a new test in apps/server/src/providers/codex/__tests__/codex-version.test.ts that calls checkCodexVersion with an unsafe cliPath containing shell-control characters (e.g. "codex; rm -rf /" or "`$(rm -rf /)`"), assert that mockSpawnSync / spawnSync is NOT called (vi.mocked(spawnSync) or mockSpawnSync).mockClear/expect not.toHaveBeenCalled(), and assert the returned result.ok is false and result.error indicates the cliPath was rejected (contains "invalid" / "unsafe" / "not allowed" text). Use the existing patterns in other tests (mockSpawnSync.mockReturnValue, expect(...).toBe(false)) and reference checkCodexVersion and spawnSync to locate the relevant logic.
🤖 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/providers/codex/codex-app-server.ts`:
- Around line 203-234: The handler currently auto-approves every serverRequest;
change it to first check this.options.approvalPolicy (or approvalPolicy) and
only perform the automatic approvals when it explicitly equals the
non-interactive policy (e.g., "non-interactive"); for all other policies, return
a closed/denied response instead of granting permissions—apply this gate in the
this.rpc.on("serverRequest", ...) handler before computing the result for
methods like "item/permissions/requestApproval",
"applyPatchApproval"/"execCommandApproval" and the v2 types, and use
this.rpc.sendResponse(request.id, ...) to send an explicit denial or error when
auto-approval is not allowed.
- Around line 148-158: After calling spawn(...) bind child.on("error", ...)
immediately (before assigning this.child, setting this._isAlive true, or
constructing the CodexRpcClient) to catch spawn/startup failures; in that
handler set this._isAlive = false, emit a 'fatal' event with the error, and
reject the surrounding Promise so the caller sees the failure instead of an
unhandled EventEmitter error. Ensure the CodexRpcClient constructor is only
invoked after the error listener is attached and no RPC is created if the spawn
throws.
- Around line 269-276: In the Windows branch of kill() (inside
codex-app-server.ts where process.platform === "win32") the early return when
this.child.pid == null leaves this._isAlive true; change the logic so that
before returning you set this._isAlive = false (and optionally clear this.child)
so liveness is cleared even when PID is missing; update the branch around the
logger.warn call (referencing this.child, this.cliPath, logger.warn) to flip
_isAlive and then return.
In `@apps/server/src/providers/codex/codex-event-mapper.ts`:
- Around line 177-179: The synthetic toolCallId fallback using Date.now() is
unsafe because concurrent items can collide; replace uses of `const toolCallId =
item.id ?? \`fc-${Date.now()}\`` (and the similar fallbacks at the other
occurrences) with a collision-safe generator such as `crypto.randomUUID()` or a
per-mapper monotonic counter; update the code paths that create `toolCallId`
(the block that checks `itemType === "function_call"`) and any places that
generate synthetic IDs for `toolUse` / `toolResult` so they all use the same
safe ID generator (keep the `item.id` when present, otherwise call the
generator) and ensure imports/state for the generator (e.g., `crypto` or a
mapper-scoped counter) are added.
In `@apps/server/src/providers/codex/codex-provider.ts`:
- Around line 161-169: The call to checkCodexVersion (which uses spawnSync) is
blocking on the WebSocket request path in sendMessage; change it so version
checks are not performed synchronously per session: implement a cached async
version lookup keyed by cliPath (or perform the check on startup/registration)
and replace the direct synchronous call in codex-provider.ts (the block using
checkCodexVersion and meetsMinVersion) with a non-blocking check that reads from
the cache (or awaits an async validation) and only emits errors if the
cached/async result indicates failure; ensure the cache invalidates or refreshes
on cliPath change and surface async validation errors off the hot request path.
- Around line 275-307: Attach the "notification" and "fatal" listeners and start
the timeout before calling server.sendTurn so you can't miss a turn/completed
event; specifically, move the setup of onNotification, onFatal, turnTimer,
cleanup, server.on("notification", onNotification) and server.once("fatal",
onFatal) to run before invoking server.sendTurn, then call
server.removeListener("fatal", earlyFatalHandler) just after listener
registration (or inside the same setup block) and finally await
server.sendTurn(input, turnOptions); ensure cleanup is used in all
resolution/rejection paths and preserve usage of TURN_TIMEOUT_MS and serverDied
as in the existing Promise logic.
In `@apps/server/src/providers/codex/codex-rpc-client.ts`:
- Around line 102-110: The code currently calls stdin.write(...) in sendRequest,
sendNotification, and sendResponse without handling write callbacks or stdin
"error"/"close" events, so if the child's stdin is closed the pending map can
leak and outbound messages are silently dropped; update sendRequest to pass a
write callback that on write error immediately clears the pending entry
(this.pending.delete(id)), clears the timer and rejects the promise with the
write error, and also ensure sendNotification/sendResponse use the same
write-callback/error-path to log or reject as appropriate; additionally
subscribe to stdin "error" and "close" (similar to how stdout is handled) and on
those events iterate this.pending to clear timers and reject all outstanding
promises with a descriptive error so the session is treated as broken.
In `@apps/server/src/providers/codex/codex-version.ts`:
- Around line 21-29: The current sanitization using SHELL_METACHAR_RE is
incomplete and misses single quotes and spaces, allowing shell interpretation
when calling spawnSync(cliPath, ..., { shell: true }); update the sanitizer used
in codex-version.ts (SHELL_METACHAR_RE) to also reject any whitespace and
single-quote characters (e.g., include \s and ' in the character class or add
explicit checks) so cliPath with spaces or ' is refused before calling
spawnSync; keep the check immediately before the spawnSync call and return the
same { ok: false, error: ... } shape when the stricter test fails.
In `@apps/server/src/repositories/tool-call-record-repo.ts`:
- Line 66: The INSERT OR IGNORE can silently skip duplicate-id writes so update
create() and bulkCreate() to check the DB run result (e.g.,
statementResult.changes or equivalent) after executing the "INSERT OR IGNORE
INTO tool_call_records ..." statement; if changes === 0, explicitly SELECT the
existing row by id from tool_call_records and return that canonical persisted
row (or surface a conflict error if that behavior is preferred), and for
bulkCreate inspect each insert result similarly and replace ignored inserts in
the returned array with the fetched persisted rows (also add tests for
duplicate-id conflicts to cover this case).
---
Outside diff comments:
In `@apps/web/src/stores/threadStore.ts`:
- Around line 1255-1270: The error cleanup currently removes
streamingByThread[threadId] but leaves streamingPreviewByThread[threadId],
leaving stale preview UI state; update the session.error branch in
threadStore.ts to also remove the preview entry by creating nextStreamingPreview
(clone state.streamingPreviewByThread, delete nextStreamingPreview[threadId])
and include streamingPreviewByThread: nextStreamingPreview in the base object
alongside streamingByThread: nextStreaming so the preview state is cleared when
an error occurs.
---
Nitpick comments:
In `@apps/server/src/providers/codex/__tests__/codex-version.test.ts`:
- Around line 42-130: Add a new test in
apps/server/src/providers/codex/__tests__/codex-version.test.ts that calls
checkCodexVersion with an unsafe cliPath containing shell-control characters
(e.g. "codex; rm -rf /" or "`$(rm -rf /)`"), assert that mockSpawnSync /
spawnSync is NOT called (vi.mocked(spawnSync) or mockSpawnSync).mockClear/expect
not.toHaveBeenCalled(), and assert the returned result.ok is false and
result.error indicates the cliPath was rejected (contains "invalid" / "unsafe" /
"not allowed" text). Use the existing patterns in other tests
(mockSpawnSync.mockReturnValue, expect(...).toBe(false)) and reference
checkCodexVersion and spawnSync to locate the relevant logic.
In `@apps/web/src/components/chat/MessageBubble.tsx`:
- Around line 153-161: Replace the bespoke error banner in MessageBubble.tsx
(the block that checks const agentError = parseAgentError(message.content) and
returns a custom div with AlertCircle) with the shadcn Alert primitive from
apps/web/src/components/ui/ (e.g., import and use Alert,
AlertTitle/AlertDescription or Alert + appropriate slots) so styling and
accessibility are consistent; keep the existing AlertCircle icon and agentError
text but move them into the Alert primitive’s title/description or content
slots, preserving classes for spacing if needed and removing the custom
border/bg markup.
In `@apps/web/src/stores/threadStore.ts`:
- Around line 1229-1241: The synthetic agent error message may serialize a
non-string payload and break the agent-error UI; ensure the message is
normalized to a string before embedding in content by coercing params.error (or
the local errorMsg) to a guaranteed string (e.g., String(params.error) or
JSON.stringify for objects) when constructing the errorMessage object so the
content JSON has "message" as a string and sequence uses get().messages.length +
1 as before.
🪄 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: b1c70a0d-cb73-4125-bba7-c01d0eb15be8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
AGENTS.mdARCHITECTURE.mdapps/server/package.jsonapps/server/src/index.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/providers/codex/__tests__/codex-event-mapper.test.tsapps/server/src/providers/codex/__tests__/codex-rpc-client.test.tsapps/server/src/providers/codex/__tests__/codex-version.test.tsapps/server/src/providers/codex/codex-app-server.tsapps/server/src/providers/codex/codex-event-mapper.tsapps/server/src/providers/codex/codex-provider.tsapps/server/src/providers/codex/codex-rpc-client.tsapps/server/src/providers/codex/codex-types.tsapps/server/src/providers/codex/codex-version.tsapps/server/src/repositories/tool-call-record-repo.tsapps/server/src/services/agent-service.tsapps/web/src/components/chat/MessageBubble.tsxapps/web/src/stores/threadStore.tsdocs/guides/provider-architecture.mdpackages/contracts/src/events/agent-event.tspackages/contracts/src/index.ts
💤 Files with no reviewable changes (1)
- apps/server/package.json
| constructor(@inject("Database") private readonly db: Database.Database) { | ||
| this.stmtInsert = db.prepare( | ||
| "INSERT INTO tool_call_records (id, message_id, parent_tool_call_id, tool_name, input_summary, output_summary, status, started_at, completed_at, sort_order) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | ||
| "INSERT OR IGNORE INTO tool_call_records (id, message_id, parent_tool_call_id, tool_name, input_summary, output_summary, status, started_at, completed_at, sort_order) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm OR IGNORE + no changes-check in repo:"
rg -n -C3 'INSERT OR IGNORE INTO tool_call_records|stmtInsert\.run\(|\.changes\b' apps/server/src/repositories/tool-call-record-repo.ts
echo
echo "2) Confirm duplicate/conflict behavior is not covered by tests:"
rg -n -C2 'duplicate|conflict|OR IGNORE|toolCallId' apps/server/src/__tests__/tool-call-record-repo.test.tsRepository: Mzeey-Emipre/mcode
Length of output: 1224
🏁 Script executed:
# Show the full create() method implementation
sed -n '80,110p' apps/server/src/repositories/tool-call-record-repo.tsRepository: Mzeey-Emipre/mcode
Length of output: 895
🏁 Script executed:
# Show the full bulkCreate() method implementation
sed -n '112,135p' apps/server/src/repositories/tool-call-record-repo.tsRepository: Mzeey-Emipre/mcode
Length of output: 769
🏁 Script executed:
# Check what TOOL_CALL_RECORD_COLUMNS contains and if rowToToolCallRecord exists
rg -n 'TOOL_CALL_RECORD_COLUMNS|rowToToolCallRecord' apps/server/src/repositories/tool-call-record-repo.tsRepository: Mzeey-Emipre/mcode
Length of output: 502
🏁 Script executed:
# Check the database schema definition for the id column
sed -n '160,190p' apps/server/src/store/database.tsRepository: Mzeey-Emipre/mcode
Length of output: 1267
🏁 Script executed:
# View the test file to understand current coverage and assess gaps
wc -l apps/server/src/__tests__/tool-call-record-repo.test.ts && head -150 apps/server/src/__tests__/tool-call-record-repo.test.tsRepository: Mzeey-Emipre/mcode
Length of output: 4483
Handle ignored inserts explicitly to avoid returning non-persisted records.
At Line 66, INSERT OR IGNORE can silently drop a duplicate id write due to the PRIMARY KEY constraint. The create() method (line 85) and bulkCreate() method (line 118) do not inspect the insert result, so they return/complete successfully even when the insert is ignored. This masks duplicate-id collisions and causes create() to return a freshly constructed record that may not exist in the database—misleading callers about which fields were actually persisted.
The tests do not cover duplicate-id or conflict scenarios, leaving this silent-failure path undetected.
Proposed fix (detect ignored insert and return canonical persisted row)
export class ToolCallRecordRepo {
private readonly stmtInsert: Database.Statement;
+ private readonly stmtGetById: Database.Statement;
private readonly stmtListByMessage: Database.Statement;
private readonly stmtListByParent: Database.Statement;
private readonly stmtCountByMessage: Database.Statement;
constructor(`@inject`("Database") private readonly db: Database.Database) {
this.stmtInsert = db.prepare(
"INSERT OR IGNORE INTO tool_call_records (id, message_id, parent_tool_call_id, tool_name, input_summary, output_summary, status, started_at, completed_at, sort_order) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)",
);
+ this.stmtGetById = db.prepare(
+ `SELECT ${TOOL_CALL_RECORD_COLUMNS} FROM tool_call_records WHERE id = ?`,
+ );
@@
create(input: CreateToolCallRecordInput): ToolCallRecord {
@@
- this.stmtInsert.run(
+ const result = this.stmtInsert.run(
id,
input.messageId,
input.parentToolCallId ?? null,
input.toolName,
input.inputSummary,
input.outputSummary,
input.status,
now,
completedAt,
input.sortOrder,
);
+
+ if (result.changes === 0) {
+ const existing = this.stmtGetById.get(id) as ToolCallRecordRow | undefined;
+ if (existing) return rowToToolCallRecord(existing);
+ throw new Error(`Tool call record insert ignored but no existing row found for id=${id}`);
+ }
return {
id,
message_id: input.messageId,
parent_tool_call_id: input.parentToolCallId ?? null,
tool_name: input.toolName,
input_summary: input.inputSummary,
output_summary: input.outputSummary,
status: input.status,
started_at: now,
completed_at: completedAt,
sort_order: input.sortOrder,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/repositories/tool-call-record-repo.ts` at line 66, The INSERT
OR IGNORE can silently skip duplicate-id writes so update create() and
bulkCreate() to check the DB run result (e.g., statementResult.changes or
equivalent) after executing the "INSERT OR IGNORE INTO tool_call_records ..."
statement; if changes === 0, explicitly SELECT the existing row by id from
tool_call_records and return that canonical persisted row (or surface a conflict
error if that behavior is preferred), and for bulkCreate inspect each insert
result similarly and replace ignored inserts in the returned array with the
fetched persisted rows (also add tests for duplicate-id conflicts to cover this
case).
Validate that an existing server's port falls within the current mode's range before reusing it. Dev (19500-19599), source-prod (19600-19699), and packaged (19700-19799) instances no longer cross-contaminate. Also guard createStreamPort() calls when the server was reused from another instance, since MessagePort requires an owned UtilityProcess.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/server/src/providers/codex/codex-provider.ts (2)
163-175:⚠️ Potential issue | 🟠 MajorMove the CLI version probe off the request path.
checkCodexVersion()still runs synchronously here, and itsspawnSync(..., timeout: 5000)can block the entire Node process for each new session. This was already flagged earlier and still needs to be moved behind async/cached validation keyed bycliPath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex/codex-provider.ts` around lines 163 - 175, The synchronous checkCodexVersion(cliPath) call must be removed from the request/session path to avoid blocking; replace it with an async, cached validation keyed by cliPath (e.g., a map from cliPath -> Promise<versionResult>) that performs the actual probe off the critical path using an async spawn with a timeout and resolves to a {ok, version, error} shape; update the session-start logic in codex-provider.ts to await or read the cached result (or optimistically start the session and emit the Error/Ended events only after the async probe resolves) and keep the existing meetsMinVersion(versionResult.version, "0.37.0") check against the cached/awaited result so that spawnSync is never called per-request.
277-309:⚠️ Potential issue | 🔴 CriticalRegister the completion listener before starting the turn.
The
turn/completedlistener is still attached only afterawait server.sendTurn(...). If the app-server emits completion before that promise resolves, this code misses the terminal event and waits untilTURN_TIMEOUT_MS. This was already called out in a previous review and is still present.Suggested fix
try { - await server.sendTurn(input, turnOptions); - - // turn/start returns immediately as an acknowledgment. - // Wait for the turn to complete via a turn/completed notification, server death, or timeout. await new Promise<void>((resolve, reject) => { - // Remove the early guard; the Promise-scoped handler takes over. - server.removeListener("fatal", earlyFatalHandler); - const cleanup = () => { clearTimeout(turnTimer); server.removeListener("notification", onNotification); server.removeListener("fatal", onFatal); }; @@ }; const turnTimer = setTimeout(() => { cleanup(); reject(new Error(`Codex turn timed out after ${TURN_TIMEOUT_MS / 1000}s`)); }, TURN_TIMEOUT_MS); server.on("notification", onNotification); server.once("fatal", onFatal); + server.removeListener("fatal", earlyFatalHandler); + + void server.sendTurn(input, turnOptions).catch((err) => { + cleanup(); + reject(err instanceof Error ? err : new Error(String(err))); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex/codex-provider.ts` around lines 277 - 309, The turn/completed notification listener and related handlers (onNotification, onFatal, turnTimer) must be registered before calling server.sendTurn so we don't miss a completion emitted during sendTurn; move the Promise construction (which sets server.on("notification", onNotification), server.once("fatal", onFatal), and the timeout using TURN_TIMEOUT_MS) to occur before invoking server.sendTurn, ensure you still remove the earlyFatalHandler (server.removeListener("fatal", earlyFatalHandler)) inside the Promise setup, and keep the same cleanup logic to clear the timeout and remove listeners when the turn completes, fails, or times out.
🤖 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/providers/codex/codex-provider.ts`:
- Around line 141-145: The idle-eviction timestamp is updated before calling
runTurn(), so long-running turns consume idle budget; modify the session
lifecycle to refresh existing.lastUsedAt when the turn actually completes
(inside the completion callback/promise resolution path of runTurn) and also on
incoming notifications/events, and add a minimal in-flight counter or flag
(e.g., session.inFlightTurns or session.isBusy) incremented before runTurn
starts and decremented when it finishes so the eviction logic treats active
sessions as non-idle; update the eviction checks to consider in-flight
count/flag and the refreshed lastUsedAt to prevent immediate post-turn eviction
(refer to existing.lastUsedAt, runTurn(sessionId, threadId, existing.server,
...), and the eviction code paths noted at the other occurrences).
- Around line 148-160: When killing the old CodexAppServer on permission-mode
change, guard the cleanup so a late exit of the old process cannot delete a
newly installed session: before calling this.sessions.delete(sessionId) and
this.sdkSessionIds.delete(sessionId) (and any fatal/exit handler cleanup),
verify that this.sessions.get(sessionId) === existing (or otherwise confirm the
stored session instance/server identity still matches the one being killed);
only perform the delete/cleanup when the current stored session is the same
instance as `existing.server`. Apply the same guard in the other handler block
(the similar code at the other occurrence mentioned) so replacements aren't
removed by late callbacks.
---
Duplicate comments:
In `@apps/server/src/providers/codex/codex-provider.ts`:
- Around line 163-175: The synchronous checkCodexVersion(cliPath) call must be
removed from the request/session path to avoid blocking; replace it with an
async, cached validation keyed by cliPath (e.g., a map from cliPath ->
Promise<versionResult>) that performs the actual probe off the critical path
using an async spawn with a timeout and resolves to a {ok, version, error}
shape; update the session-start logic in codex-provider.ts to await or read the
cached result (or optimistically start the session and emit the Error/Ended
events only after the async probe resolves) and keep the existing
meetsMinVersion(versionResult.version, "0.37.0") check against the
cached/awaited result so that spawnSync is never called per-request.
- Around line 277-309: The turn/completed notification listener and related
handlers (onNotification, onFatal, turnTimer) must be registered before calling
server.sendTurn so we don't miss a completion emitted during sendTurn; move the
Promise construction (which sets server.on("notification", onNotification),
server.once("fatal", onFatal), and the timeout using TURN_TIMEOUT_MS) to occur
before invoking server.sendTurn, ensure you still remove the earlyFatalHandler
(server.removeListener("fatal", earlyFatalHandler)) inside the Promise setup,
and keep the same cleanup logic to clear the timeout and remove listeners when
the turn completes, fails, or times out.
🪄 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: d4a0c840-a05a-4eac-a3dc-239aee0a5a8c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/server/package.jsonapps/server/src/index.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/providers/codex/codex-provider.tsapps/web/src/stores/threadStore.tspackages/contracts/src/index.ts
💤 Files with no reviewable changes (1)
- apps/server/package.json
✅ Files skipped from review due to trivial changes (2)
- packages/contracts/src/index.ts
- apps/server/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/providers/claude/claude-provider.ts
Comment on lines
+141
to
+145
| if (existing.sandboxMode === sandbox) { | ||
| // Same permission mode - reuse the running session | ||
| existing.lastUsedAt = Date.now(); | ||
| existing.mapper.reset(); | ||
| void this.runTurn(sessionId, threadId, existing.server, input, turnOptions); |
There was a problem hiding this comment.
Idle eviction is measuring from turn start, not last activity.
lastUsedAt is only refreshed before runTurn() starts. That means a long turn burns most of the 10-minute idle budget while it's still active, so a session that took 8–9 minutes to answer can be evicted almost immediately after completion. Track in-flight turns and/or refresh activity when the turn finishes or notifications arrive.
Also applies to: 252-253, 327-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/codex/codex-provider.ts` around lines 141 - 145,
The idle-eviction timestamp is updated before calling runTurn(), so long-running
turns consume idle budget; modify the session lifecycle to refresh
existing.lastUsedAt when the turn actually completes (inside the completion
callback/promise resolution path of runTurn) and also on incoming
notifications/events, and add a minimal in-flight counter or flag (e.g.,
session.inFlightTurns or session.isBusy) incremented before runTurn starts and
decremented when it finishes so the eviction logic treats active sessions as
non-idle; update the eviction checks to consider in-flight count/flag and the
refreshed lastUsedAt to prevent immediate post-turn eviction (refer to
existing.lastUsedAt, runTurn(sessionId, threadId, existing.server, ...), and the
eviction code paths noted at the other occurrences).
Comment on lines
+148
to
+160
| // Permission mode changed - kill the old session so we can start fresh with the correct sandbox | ||
| logger.info("Codex session restarted due to permission mode change", { | ||
| sessionId, | ||
| from: existing.sandboxMode, | ||
| to: sandbox, | ||
| }); | ||
| this.sessions.delete(sessionId); | ||
| // Clear the stored SDK thread ID so the new session starts fresh rather than | ||
| // resuming the old thread (which would inherit the old sandbox mode). | ||
| this.sdkSessionIds.delete(sessionId); | ||
| existing.server.kill().catch((err: unknown) => { | ||
| logger.warn("Codex session kill on permission change failed", { error: String(err) }); | ||
| }); |
There was a problem hiding this comment.
Guard cleanup so an old server can't delete its replacement.
After a permission-mode change, the old existing.server.kill() runs asynchronously while a new CodexAppServer can be installed under the same sessionId. These fatal/exit handlers then unconditionally call this.sessions.delete(sessionId), so a late exit from the old process can wipe out the fresh session.
Suggested fix
+ const clearSessionIfCurrent = () => {
+ if (this.sessions.get(sessionId)?.server === server) {
+ this.sessions.delete(sessionId);
+ }
+ };
+
server.on("fatal", (error: string) => {
logger.error("CodexAppServer fatal", { sessionId, error });
this.emit("event", { type: AgentEventType.Error, threadId, error } satisfies AgentEvent);
this.emit("event", { type: AgentEventType.Ended, threadId } satisfies AgentEvent);
- this.sessions.delete(sessionId);
+ clearSessionIfCurrent();
});
server.on("exit", () => {
if (!server.isAlive) {
- this.sessions.delete(sessionId);
+ clearSessionIfCurrent();
}
});Also applies to: 211-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/codex/codex-provider.ts` around lines 148 - 160,
When killing the old CodexAppServer on permission-mode change, guard the cleanup
so a late exit of the old process cannot delete a newly installed session:
before calling this.sessions.delete(sessionId) and
this.sdkSessionIds.delete(sessionId) (and any fatal/exit handler cleanup),
verify that this.sessions.get(sessionId) === existing (or otherwise confirm the
stored session instance/server identity still matches the one being killed);
only perform the delete/cleanup when the current stored session is the same
instance as `existing.server`. Apply the same guard in the other handler block
(the similar code at the other occurrence mentioned) so replacements aren't
removed by late callbacks.
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/desktop/src/main/server-manager.ts`:
- Line 147: The port-range handling is inconsistent: tryExistingServer treats
portMax as exclusive (rejects ports >= portMax) while findAvailablePort (and
allocation sites) use an inclusive upper bound, allowing allocation of a port
that will never be reused; fix by making allocation half-open to match reuse
semantics—update findAvailablePort and any allocation call sites (the code that
picks/loops over ports and the allocation call referenced near tryExistingServer
and the other occurrences noted) to treat the upper bound as exclusive (use port
< portMax rather than <= portMax) so that selection and reuse semantics align
across tryExistingServer, findAvailablePort, and the other affected blocks.
🪄 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: 5f4fdca8-8182-4536-8388-9eb2086fd501
📒 Files selected for processing (2)
apps/desktop/src/main/main.tsapps/desktop/src/main/server-manager.ts
| * probing its health endpoint. Returns the lock info if healthy, null otherwise. | ||
| */ | ||
| async function tryExistingServer(): Promise<ServerLock | null> { | ||
| async function tryExistingServer(portMin: number, portMax: number): Promise<ServerLock | null> { |
There was a problem hiding this comment.
Port-range semantics are inconsistent between reuse and allocation.
Line 159 treats portMax as exclusive (>= portMax), but findAvailablePort() still allocates with an inclusive upper bound (<= max). That means a boundary port can be allocated but later never reused, causing avoidable duplicate server spawns.
Suggested fix (make allocation half-open to match Line 159)
-async function findAvailablePort(min: number, max: number): Promise<number> {
- for (let port = min; port <= max; port++) {
+async function findAvailablePort(min: number, maxExclusive: number): Promise<number> {
+ for (let port = min; port < maxExclusive; port++) {
const available = await new Promise<boolean>((resolve) => {
const srv = createServer();
srv.once("error", () => resolve(false));
srv.listen(port, () => {
const addr = srv.address() as AddressInfo;
srv.close(() => resolve(addr.port === port));
});
});
if (available) return port;
}
- throw new Error(`No available port found in range ${min}-${max}`);
+ throw new Error(`No available port found in range ${min}-${maxExclusive - 1}`);
}Also applies to: 157-162, 220-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/server-manager.ts` at line 147, The port-range handling
is inconsistent: tryExistingServer treats portMax as exclusive (rejects ports >=
portMax) while findAvailablePort (and allocation sites) use an inclusive upper
bound, allowing allocation of a port that will never be reused; fix by making
allocation half-open to match reuse semantics—update findAvailablePort and any
allocation call sites (the code that picks/loops over ports and the allocation
call referenced near tryExistingServer and the other occurrences noted) to treat
the upper bound as exclusive (use port < portMax rather than <= portMax) so that
selection and reuse semantics align across tryExistingServer, findAvailablePort,
and the other affected blocks.
- Gate serverRequest auto-approval behind approvalPolicy === "never" - Catch spawn failures with child error listener before state assignment - Clear _isAlive on Windows kill() early return when PID is missing - Replace Date.now() fallback IDs with crypto.randomUUID() - Cache checkCodexVersion per cliPath to avoid blocking spawnSync - Extend SHELL_METACHAR_RE to reject single quotes and whitespace - Wire turn listeners before sendTurn to prevent race condition - Add stdin write callbacks and error/close listeners to RPC client - Clean up streamingPreviewByThread in session.error handler - Coerce error param to string in session.error handler - Add shell injection rejection test for checkCodexVersion
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/server/src/providers/codex/codex-rpc-client.ts (1)
126-131:⚠️ Potential issue | 🟠 MajorTreat stdin write callback failures as fatal transport errors.
These paths still degrade inconsistently on write failure:
sendRequestonly rejects one request, whilesendNotification/sendResponsejust log and continue. If stdin is broken, other in-flight requests can hang until timeout and server-initiated RPCs may never receive a response. Mark the client broken (disposed = true) andrejectAll(...)on any write callback error.Suggested fix
+ private handleWriteFailure(context: string, err: Error): void { + logger.error("CodexRpcClient: stdin write failed", { context, error: err.message }); + if (!this.disposed) { + this.disposed = true; + this.rejectAll(new Error(`stdin write failed (${context}): ${err.message}`)); + } + } sendRequest<TParams, TResult>( @@ this.pending.set(id, { resolve, reject, timer }); this.stdin.write(message, (err) => { if (err) { clearTimeout(timer); this.pending.delete(id); - reject(new Error(`stdin write failed for ${method}: ${err.message}`)); + const writeErr = new Error(`stdin write failed for ${method}: ${err.message}`); + reject(writeErr); + this.handleWriteFailure(`request:${method}`, writeErr); } }); }); } @@ this.stdin.write(message, (err) => { - if (err) logger.warn("CodexRpcClient: notification write failed", { method, error: err.message }); + if (err) this.handleWriteFailure(`notification:${method}`, new Error(err.message)); }); @@ this.stdin.write(message, (err) => { - if (err) logger.warn("CodexRpcClient: response write failed", { id, error: err.message }); + if (err) this.handleWriteFailure(`response:${id}`, new Error(err.message)); }); }Also applies to: 149-151, 166-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/codex/codex-rpc-client.ts` around lines 126 - 131, On any this.stdin.write callback error (in sendRequest, sendNotification, sendResponse) treat it as a fatal transport error: set this.disposed = true, clear any per-request timer and remove pending entries for that id (as done in sendRequest), and call this.rejectAll(new Error(...)) to reject all in-flight promises; for sendRequest also reject the current promise as before, for sendNotification/sendResponse stop just logging and perform the same disposed + rejectAll behavior so no requests hang. Ensure the error message includes method/id context and reuse the same failure-handling steps at the three write-callback sites currently around lines shown (the write callbacks in sendRequest, sendNotification, sendResponse).
🤖 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/providers/codex/codex-rpc-client.ts`:
- Around line 56-63: The onData handler appends incoming chunks to lineBuffer
which can grow unbounded; add a hard max (e.g. MAX_LINE_BUFFER_BYTES) and check
after concatenation in onData: if lineBuffer.length > MAX_LINE_BUFFER_BYTES,
log/emit an error, destroy/close the stream/connection and stop processing to
fail fast; otherwise proceed with splitting, keep the last partial segment as
before and call processLine for complete lines. Update any tests or callers of
codex-rpc-client to expect the stream to be closed/errored on buffer overflow.
---
Duplicate comments:
In `@apps/server/src/providers/codex/codex-rpc-client.ts`:
- Around line 126-131: On any this.stdin.write callback error (in sendRequest,
sendNotification, sendResponse) treat it as a fatal transport error: set
this.disposed = true, clear any per-request timer and remove pending entries for
that id (as done in sendRequest), and call this.rejectAll(new Error(...)) to
reject all in-flight promises; for sendRequest also reject the current promise
as before, for sendNotification/sendResponse stop just logging and perform the
same disposed + rejectAll behavior so no requests hang. Ensure the error message
includes method/id context and reuse the same failure-handling steps at the
three write-callback sites currently around lines shown (the write callbacks in
sendRequest, sendNotification, sendResponse).
🪄 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: 6c87558c-c774-458a-aae2-8b9af307adf2
📒 Files selected for processing (7)
apps/server/src/providers/codex/__tests__/codex-version.test.tsapps/server/src/providers/codex/codex-app-server.tsapps/server/src/providers/codex/codex-event-mapper.tsapps/server/src/providers/codex/codex-provider.tsapps/server/src/providers/codex/codex-rpc-client.tsapps/server/src/providers/codex/codex-version.tsapps/web/src/stores/threadStore.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/providers/codex/codex-provider.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/server/src/providers/codex/tests/codex-version.test.ts
- apps/server/src/providers/codex/codex-version.ts
- apps/server/src/providers/codex/codex-event-mapper.ts
- apps/server/src/providers/codex/codex-app-server.ts
- apps/web/src/stores/threadStore.ts
Comment on lines
+56
to
+63
| this.onData = (chunk: string) => { | ||
| this.lineBuffer += chunk; | ||
| const lines = this.lineBuffer.split("\n"); | ||
| // Keep the last (potentially incomplete) segment in the buffer | ||
| this.lineBuffer = lines.pop() ?? ""; | ||
| for (const line of lines) { | ||
| this.processLine(line); | ||
| } |
There was a problem hiding this comment.
Bound lineBuffer to avoid unbounded memory growth.
lineBuffer grows indefinitely until a newline is received. A malformed/hostile stream (or upstream bug) can OOM this process. Add a max buffer size and fail fast when exceeded.
Suggested fix
/** Default timeout in milliseconds for RPC requests. */
const DEFAULT_TIMEOUT_MS = 20_000;
+/** Maximum buffered stdout bytes without a newline before aborting. */
+const MAX_LINE_BUFFER_CHARS = 1_000_000;
@@
this.onData = (chunk: string) => {
this.lineBuffer += chunk;
+ if (this.lineBuffer.length > MAX_LINE_BUFFER_CHARS) {
+ const err = new Error("stdout line buffer exceeded limit");
+ logger.error("CodexRpcClient: excessive stdout buffering", { size: this.lineBuffer.length });
+ this.disposed = true;
+ this.rejectAll(err);
+ return;
+ }
const lines = this.lineBuffer.split("\n");📝 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.
Suggested change
| this.onData = (chunk: string) => { | |
| this.lineBuffer += chunk; | |
| const lines = this.lineBuffer.split("\n"); | |
| // Keep the last (potentially incomplete) segment in the buffer | |
| this.lineBuffer = lines.pop() ?? ""; | |
| for (const line of lines) { | |
| this.processLine(line); | |
| } | |
| this.onData = (chunk: string) => { | |
| this.lineBuffer += chunk; | |
| if (this.lineBuffer.length > MAX_LINE_BUFFER_CHARS) { | |
| const err = new Error("stdout line buffer exceeded limit"); | |
| logger.error("CodexRpcClient: excessive stdout buffering", { size: this.lineBuffer.length }); | |
| this.disposed = true; | |
| this.rejectAll(err); | |
| return; | |
| } | |
| const lines = this.lineBuffer.split("\n"); | |
| // Keep the last (potentially incomplete) segment in the buffer | |
| this.lineBuffer = lines.pop() ?? ""; | |
| for (const line of lines) { | |
| this.processLine(line); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/codex/codex-rpc-client.ts` around lines 56 - 63,
The onData handler appends incoming chunks to lineBuffer which can grow
unbounded; add a hard max (e.g. MAX_LINE_BUFFER_BYTES) and check after
concatenation in onData: if lineBuffer.length > MAX_LINE_BUFFER_BYTES, log/emit
an error, destroy/close the stream/connection and stop processing to fail fast;
otherwise proceed with splitting, keep the last partial segment as before and
call processLine for complete lines. Update any tests or callers of
codex-rpc-client to expect the stream to be closed/errored on buffer overflow.
Merged
This was referenced Apr 29, 2026
This was referenced Jul 6, 2026
This was referenced Jul 20, 2026
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
1 participant
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.
What
Rewrites the Codex provider from
@openai/codex-sdk(per-turn process spawning) to a persistentcodex app-serverchild process per session communicating via JSON-RPC 2.0 over stdin/stdout. Also fixes a critical session resume bug that silently dropped conversation context on every app restart.Why
Two problems:
Reliability: The SDK spawned a new native binary every turn. On Windows, stdin pipe timing caused frequent
"Codex Exec exited with code 1: Reading prompt from stdin..."errors. This is the same per-turn spawning pattern that already failed for the Claude provider.Session resume broken:
thread/resumereturned the thread ID atresult.thread.id(nested) but we only readresult.threadId(flat). The ID was silentlyundefined, the code fell through tothread/start, and a brand new thread was created on every app restart - losing all conversation context.Key changes
Architecture (prior commits on this branch)
@openai/codex-sdkwith directcodex app-serverJSON-RPC 2.0 protocolcodex-rpc-client.ts,codex-app-server.ts,codex-event-mapper.ts,codex-types.ts,codex-version.tsSession resume fix (this commit)
result.threadIdandresult.thread.idresponse shapes forthread/resumethread/startednotification for mid-session thread rotationthreadIdChangedevent (context compaction)Hardening
turn/startandthread/resumespawn()endedemission race between fatal handler andrunTurnthread/resumeerror patternscommandOutputBufferslookup miss whenitem.idis absentfc-prefix collision betweenfunction_callandfileChangefallback IDsTest plan
npx tsc --noEmitpasses inapps/servercodex-rpc-client,codex-event-mapper,codex-versionthread/resume succeeded,sameId: true, full turn history in response)Summary by CodeRabbit
New Features
Bug Fixes
Documentation