refactor(openai-shim): extract Codex dispatch - #2074
Conversation
📝 WalkthroughWalkthroughChangesThe PR extracts Codex request dispatch from Codex dispatch extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
faa5b99 to
fb780d6
Compare
fb780d6 to
fbfb68c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/services/api/openaiShim/codexDispatch.test.ts`:
- Around line 86-116: Add negative tests alongside the existing “refreshes an
expired GitHub Copilot token once before retrying” test in the codex dispatch
suite: verify dispatchCodexRequest throws the original APIError and does not
retry when refreshCopilotTokenOn401 returns false or an unchanged token, verify
an already-aborted request throws the value from preserveCallerAbortError, and
verify an empty apiKey throws the /onboard-github message. Keep
performCodexRequest call counts and refresh calls asserted for the retry cases.
In `@src/services/api/openaiShim/codexDispatch.ts`:
- Around line 36-50: Update
CodexDispatchDependencies.classifyResponseHeadersTimeout to return an explicit
undefined-capable classification type instead of unknown alone, preserving the
undefined sentinel consumed by the call site around the dispatch logic.
- Around line 27-32: Remove the local COPILOT_HEADERS declaration in
codexDispatch.ts and import the canonical COPILOT_HEADERS exported by
deviceFlow.ts. Preserve the existing consumers and behavior while ensuring this
module uses the shared constant rather than maintaining a duplicate version.
🪄 Autofix
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: fefba057-63a5-4643-859e-dbf307dca0ad
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.ts
💤 Files with no reviewable changes (1)
- src/services/api/openaiShim.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and ESM imports throughout the source code.
Run
bun run typecheckandbun run typecheck:type-testsfor TypeScript changes when applicable.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use React and Ink patterns for terminal UI components.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color andexecafor child-process execution when those capabilities are needed.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use existing service and provider integration patterns when implementing API, MCP, OAuth, wiki, voice, or related integrations.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Add or update tests when behavior changes, and run the narrowest useful focused test checks.
Files:
src/services/api/openaiShim/codexDispatch.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add new Python code, Python provider paths, or Python dependencies without explicit maintainer approval.
**/*.{ts,tsx,js,jsx}: Follow the existing code style in touched source files, prefer small readable changes, avoid unrelated reformatting, and keep comments useful and concise.
Preserve existing repository patterns unless intentionally refactoring them, and avoid broad rewrites or unnecessary generated changes.
Review AI-assisted code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting it.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Add or update tests when a code change affects behavior.
Use focused tests such asbun test ./path/to/test-file.test.tswhen validating a narrowly scoped change.
Files:
src/services/api/openaiShim/codexDispatch.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim/codexDispatch.test.tssrc/services/api/openaiShim/codexDispatch.tssrc/services/api/openaiShim.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/openaiShim/codexDispatch.test.ts
🔇 Additional comments (6)
src/services/api/openaiShim/codexDispatch.ts (2)
92-170: LGTM!
172-218: LGTM!src/services/api/openaiShim/codexDispatch.test.ts (3)
14-72: LGTM!
118-197: LGTM!
89-95: 📐 Maintainability & Code QualityDo not block on implicit
any.tsconfig.jsonsetsnoImplicitAnytofalse, so these parameters do not failbun run typecheck. Typingoptionsremains optional. The cast at line 128 is still needed because the optionalfetchercall can returnundefined.> Likely an incorrect or invalid review comment.src/services/api/openaiShim.ts (1)
458-479: 🩺 Stability & AvailabilityNo change needed.
> Likely an incorrect or invalid review comment.
|
@kevincodex1 LGTM |
…rtial port Port of upstream c305788 (91 files, 9-commit series) via fork-aware selective apply. The PR delivers a unified interruption trace infrastructure; fork already had the QueryGuard-side trace helpers from prior syncs, so the partial port brings across the new trace modules and consumer-side wiring while dropping the codex / gemini / sdk-v2 / goal subtrees that don't exist in the fork provider policy. New trace modules added (from upstream main, fork previously lacked): - src/utils/interruptionTrace.ts (23k, requestAbort / trace / flushInterruptionTrace / resolution observer) - src/utils/replInterruption.ts (small facade for REPL consumers) - src/utils/queryEventDriver.ts (lifecyle event hook) - src/utils/swarm/inProcessPermissionAbort.ts (in-process teammate abort snapshot) - src/cli/printInterruption.ts (CLI-side enforcement hook) - src/tools/SendMessageTool/shutdownInterruptionTrace.ts (shutdownApproved helper for SendMessageTool) New tests: - src/utils/interruptionTrace.test.ts - src/utils/replInterruption.test.ts - src/utils/queryEventDriver.test.ts - src/state/teammateViewHelpers.interruptionTrace.test.ts (verifies that LocalAgentTask stopOrDismiss propagates through trace infrastructure) Existing-file patches applied verbatim (git apply --3way clean, no conflict): - QueryEngine.ts, grpc/server.ts, hooks/toolPermission/{PermissionContext, handlers/{interactiveHandler.ts, interactiveHandler.test.ts}}, hooks/useBackgroundTaskNavigation.ts, hooks/useSSHSession.ts, components/permissions/PermissionRequest.tsx, services/PromptSuggestion/{speculation.ts}, services/api/claude.abortClassification.test.ts, services/compact/{compact.ts}, services/tools/StreamingToolExecutor.ts (conflict resolved by accepting upstream's trace-context fork), state/teammateViewHelpers.ts, tasks/LocalAgentTask/LocalAgentTask.tsx, tools/SendMessageTool/SendMessageTool.ts, utils/abortController.ts, utils/attachments.ts, utils/combinedAbortSignal.ts (conflict resolved on opts.trace field, accepting upstream's full opts), utils/computerUse/wrapper.tsx, utils/diagLogs.ts, utils/forkedAgent.ts, utils/fsOperations.ts, utils/gracefulShutdown.ts, utils/handlePromptSubmit.ts, utils/permissions/{PermissionPromptToolResultSchema.ts, permissions.ts}, utils/queryLifecycle.{ts,test.ts}, utils/swarm/{inProcessRunner.ts, spawnInProcess.ts}, docs/advanced-setup.md Conflict-resolved with upstream-theirs: - src/utils/QueryGuard.ts: ONLY added the upstream _handleTimeout causalEventId emission so a fired timeout now flows into the trace bus. Did NOT drop fork's _buildTimeoutInfo, setLifecycleHook, public getActiveOperations, or the prior _traceActivityCount / _lastTraceActivityAt work from earlier syncs. Manual Edit-by-hunk, not git cherry-pick (per docs/sync-upstream.md). Skipped (per AGENTS.md provider policy): - src/services/api/codexShim.* (codex provider not in fork) - src/services/api/openaiShim/{clientDispatch, streamControl, streamConversion, transport, responseAdapters, geminiStreamConversion, providerStreamInterruptionTrace}.{ts,test.ts}: the openaiShim subdirectory split into per-responsibility files does not exist in fork's openaiShim monolith; porting the per-file trace requires the parallel Twigpine#2073/Twigpine#2074 refactors (deferred as separate task) - src/services/goal/{controller, evaluator}.{ts,test.ts}: fork has no goal service - src/entrypoints/sdk/{interruption, query, v2}.ts: fork has no v2 SDK entrypoint - tests/sdk/*.test.ts: fork has no tests/sdk/ runner - src/QueryEngine.interruptionTrace.test.ts and 9 other *.interruptionTrace.test.ts for codex / goal / sdk-v2 / openaiShim- submodule targets: dangling fixtures without their corresponding source port Untouched conflict files (reverted to fork base for upstream's incompatible rewrites, to be retried after sync'ing pre-reqs): - src/hooks/useInboxPoller.ts, src/hooks/useCancelRequest.ts, src/hooks/useReplBridge.tsx, src/remote/remotePermissionBridge.ts, src/screens/REPL.tsx, src/query.ts, src/cli/print.ts, src/services/api/claude.ts, src/services/api/openaiShim.test.ts, etc: their conflict hunks pulled in fork-missing helpers (requestPermissionModeChange from permissionModeChange.ts, codex gRPC bridge, SDK v2 QueryLifecycle wiring) and would not compile without first porting those modules. Verification (5-phase per docs/verification-checklist.md): - bun run typecheck: 0 errors (was 0 before, remained 0) - bun run build: opencc v0.21.0 -> dist/cli.mjs + dist/sdk.mjs - bun test: 5163 pass / 196 skip / 0 fail across 5359 tests (was 5134 / 183 / 0 baseline -> +29 new tests, +13 new skipped, 0 regressions) Re-applying this commit pre-requisite on follow-up sync windows: follow-up commits should bring in permissionModeChange.ts, services/goal/, openaiShim/{clientDispatch, streamControl, streamConversion, transport, responseAdapters}* and entries/sdk/ before re-running this PR's hook-side patches.
2 participants
Summary
openaiShim/codexDispatch.tsandcodexDispatch.test.tsopenaiShim.test.tsgeminiStreamConversion.test.tsopenaiShim.tsand enforce same-basename source/test ownership for every production module underopenaiShim/openaiShim.tsfrom 1,582 to 1,473 lines (25 additions, 134 deletions)Why
The remaining Codex routing block is a distinct responsibility inside
openaiShim.ts. The architecture test was also stored underopenaiShim/without a same-basename production module, whilegeminiStreamConversion.tshad no paired test. This draft fixes those ownership boundaries without broad legacy-test cleanup that would overlap the other independent drafts.This branch is based directly on upstream
mainatb3735bed. The shared architecture relocation and Gemini pairing are byte-identical in all four drafts, and the conditional facade budget accounts for whichever extractions exist. All six branch pairs and all 24 four-PR merge orders were simulated successfully. With all four drafts applied, the facade is 676 lines.Impact
No intended runtime behavior change. Codex dispatch has a focused owner and direct tests, and future unpaired production modules under
openaiShim/fail the architecture guard.Provider paths tested: GitHub Copilot Codex Responses and first-party Codex Responses.
Validation
bun run typecheck— passbun run build— passbun run check— build, smoke, and dead-code checks pass; full test run reports 7,793 pass, 2 skip, and 45 environment-only failures in restricted temporary-git/filesystem/cache/persisted-output operations and xAI loopback bindingContributor checklist
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit
Bug Fixes
Tests