[codex] split Codex WS and compat cleanup#87
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/plugins/backends/openaicodex/ws_session.go`:
- Around line 124-133: The closeIdle path in wsSessionStore can still act on a
stale timer callback, so update closeIdle to verify the session is still the
armed idle target before closing it. Use the existing wsSessionStore.closeIdle,
wsSessionConn.stopIdleTimerLocked, and forgetLocked flow to guard against stale
callbacks by checking the timer state and confirming the map entry still matches
the same wsSessionConn under the store lock before calling closeConnLocked or
removing it.
In `@internal/plugins/backends/openaicodex/ws_stream.go`:
- Around line 93-96: The mapper error path in ws_stream.go returns without
releasing the session, unlike the read-error and EOF paths. Update the error
handling in the stream processing flow around s.mapper.handleData and the
surrounding cleanup logic to ensure the same release/eviction behavior runs
before returning on malformed frame errors, so the session semaphore is not left
held if Close is never called.
In `@internal/plugins/features/codexclientcompat/bridge_opencode.go`:
- Around line 62-87: There is duplicated JSON parsing and function-call type
validation logic between isFunctionCallPart and collectKnownToolCallIDs, so
extract a shared helper (for example, parseFunctionCallID) that unmarshals the
Part content, normalizes the "function_call"/"function" type check, and returns
the call id and name. Update both isFunctionCallPart and collectKnownToolCallIDs
to use that helper so the accepted variants and extraction rules stay consistent
in one place.
- Around line 89-117: The convertOrphanedToolResults flow is reordering
tool-result parts because kept parts are buffered in kept while orphaned results
are appended to out immediately. Update convertOrphanedToolResults (and, if
needed, convertOrphanedToolResult) so each part is emitted in the same order it
appears in call.Messages, preserving known and orphaned tool results within a
message instead of flushing kept parts only after the loop.
🪄 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: 63545cb7-9d5f-4219-ad72-9e71fe007017
📒 Files selected for processing (13)
internal/plugins/backends/openaicodex/stream.gointernal/plugins/backends/openaicodex/ws.gointernal/plugins/backends/openaicodex/ws_session.gointernal/plugins/backends/openaicodex/ws_stream.gointernal/plugins/features/codexclientcompat/bridge_droid.gointernal/plugins/features/codexclientcompat/bridge_hermes.gointernal/plugins/features/codexclientcompat/bridge_opencode.gointernal/plugins/features/codexclientcompat/bridge_pi.gointernal/plugins/features/codexclientcompat/compat.gointernal/plugins/frontends/anthropic/handler.gointernal/plugins/frontends/gemini/handler.gointernal/plugins/frontends/openailegacy/handler.gointernal/plugins/frontends/openairesponses/handler.go
💤 Files with no reviewable changes (1)
- internal/plugins/features/codexclientcompat/compat.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: qa
🧰 Additional context used
📓 Path-based instructions (5)
internal/plugins/frontends/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
In
internal/plugins/frontends/, implement OpenAI Responses, OpenAI legacy, Anthropic, and Gemini frontends.
Files:
internal/plugins/frontends/anthropic/handler.gointernal/plugins/frontends/gemini/handler.gointernal/plugins/frontends/openailegacy/handler.gointernal/plugins/frontends/openairesponses/handler.go
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Review as production Go code. Prioritize correctness, race conditions, goroutine leaks, context cancellation, timeout handling, error wrapping, nil-pointer risks, resource cleanup, defer placement, API compatibility, interface design, dependency boundaries, and testability. Avoid generic style comments when gofmt/golangci-lint already covers the issue.
Files:
internal/plugins/frontends/anthropic/handler.gointernal/plugins/frontends/gemini/handler.gointernal/plugins/features/codexclientcompat/bridge_hermes.gointernal/plugins/features/codexclientcompat/bridge_pi.gointernal/plugins/frontends/openailegacy/handler.gointernal/plugins/backends/openaicodex/stream.gointernal/plugins/features/codexclientcompat/bridge_droid.gointernal/plugins/frontends/openairesponses/handler.gointernal/plugins/backends/openaicodex/ws_session.gointernal/plugins/backends/openaicodex/ws_stream.gointernal/plugins/features/codexclientcompat/bridge_opencode.gointernal/plugins/backends/openaicodex/ws.go
internal/**
⚙️ CodeRabbit configuration file
internal/**: Focus on package boundaries, hidden coupling, unexported API design, concurrency safety, deterministic behavior, and whether logic belongs in this internal package.
Files:
internal/plugins/frontends/anthropic/handler.gointernal/plugins/frontends/gemini/handler.gointernal/plugins/features/codexclientcompat/bridge_hermes.gointernal/plugins/features/codexclientcompat/bridge_pi.gointernal/plugins/frontends/openailegacy/handler.gointernal/plugins/backends/openaicodex/stream.gointernal/plugins/features/codexclientcompat/bridge_droid.gointernal/plugins/frontends/openairesponses/handler.gointernal/plugins/backends/openaicodex/ws_session.gointernal/plugins/backends/openaicodex/ws_stream.gointernal/plugins/features/codexclientcompat/bridge_opencode.gointernal/plugins/backends/openaicodex/ws.go
internal/plugins/features/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
In
internal/plugins/features/, implement official feature and reference plugins.
Files:
internal/plugins/features/codexclientcompat/bridge_hermes.gointernal/plugins/features/codexclientcompat/bridge_pi.gointernal/plugins/features/codexclientcompat/bridge_droid.gointernal/plugins/features/codexclientcompat/bridge_opencode.go
internal/plugins/backends/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
In
internal/plugins/backends/, implement provider/local/compatible backend adapters, with the exact bundle defined ininternal/pluginreg/standard_table.go.
Files:
internal/plugins/backends/openaicodex/stream.gointernal/plugins/backends/openaicodex/ws_session.gointernal/plugins/backends/openaicodex/ws_stream.gointernal/plugins/backends/openaicodex/ws.go
🔇 Additional comments (14)
internal/plugins/frontends/anthropic/handler.go (1)
130-130: LGTM!internal/plugins/frontends/gemini/handler.go (1)
128-128: LGTM!internal/plugins/frontends/openailegacy/handler.go (1)
122-122: LGTM!internal/plugins/frontends/openairesponses/handler.go (1)
166-166: LGTM!internal/plugins/features/codexclientcompat/bridge_droid.go (1)
1-90: LGTM!internal/plugins/features/codexclientcompat/bridge_pi.go (1)
1-65: LGTM!internal/plugins/features/codexclientcompat/bridge_hermes.go (2)
39-51: 📐 Maintainability & Code QualityHermes uses a custom critical-instruction block instead of the shared helper; keep it only if the wording is intentionally different, otherwise route it through the common builder to avoid drift.
53-61: 🎯 Functional CorrectnessConfirm
extCodexToolStrictKeysemantics
applyHermesToolStrictwritesfalseto a flag named like “strict”; confirm this is intentionally disabling Codex-side strict schema validation for Hermes, not an inverted boolean.internal/plugins/backends/openaicodex/ws_session.go (1)
1-123: LGTM!Also applies to: 136-208
internal/plugins/backends/openaicodex/ws_stream.go (2)
14-92: LGTM!Also applies to: 97-100, 124-160
101-121: 🩺 Stability & AvailabilityWait for the cancellation callback before reusing the connection. If
context.AfterFunccan still run afterstopCancel(), a late deadline write can poison the websocket for the next read.internal/plugins/backends/openaicodex/ws.go (2)
232-239: 🩺 Stability & AvailabilityVerify generic pre-first-event errors close the stream.
The
previous_response_not_foundbranch explicitly releaseswsStream, but Line 238 relies onopenManagedFirstEvent/openManagedUntilCommittedhaving already closed it. The shownopenManagedUntilCommitteddoes; please confirmopenManagedFirstEventhas the same guarantee, or close here before returning.As per path instructions,
**/*.go: “Prioritize ... resource cleanup...”.Source: Path instructions
104-216: LGTM!Also applies to: 241-247
internal/plugins/backends/openaicodex/stream.go (1)
89-91: LGTM!
- ws_session: guard closeIdle against stale idle-timer callbacks that race with acquire reusing a tracked session (skip only when the session is still the map entry and its timer was stopped since firing); orphaned sessions are still closed to avoid leaking connections. - ws_stream: releaseOnce(true) on mapper errors so the session is evicted even if the caller never calls Close, matching the read-error path. - codexclientcompat: extract parseFunctionCallID shared by isFunctionCallPart and collectKnownToolCallIDs so accepted type variants stay in sync. - codexclientcompat: preserve part order in convertOrphanedToolResults so converted orphaned results keep their original position relative to known results instead of being flushed after them. Adds focused tests for each change. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
openai-codexWebSocket internals into same-package session and stream filescodexclientcompatbridge-specific logic into same-package bridge filesValidation
go test ./internal/plugins/backends/openaicodex ./internal/plugins/features/codexclientcompat ./internal/plugins/frontends/anthropic ./internal/plugins/frontends/gemini ./internal/plugins/frontends/openailegacy ./internal/plugins/frontends/openairesponsesstaticcheck -checks "S*,U1000" ./internal/plugins/backends/openaicodex ./internal/plugins/features/codexclientcompat ./internal/plugins/frontends/anthropic ./internal/plugins/frontends/gemini ./internal/plugins/frontends/openailegacy ./internal/plugins/frontends/openairesponsesgit diff --checkmake qaNotes