refactor(openai-shim): extract request preparation - #2073
Conversation
📝 WalkthroughWalkthroughThe OpenAI shim delegates request preparation to ChangesOpenAI request preparation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
c51bf33 to
186b84b
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: 2
🤖 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.architecture.test.ts`:
- Around line 5-6: Update the path construction around facadePath and
moduleDirectory to import and use node:path joining utilities, avoiding manual
separator concatenation when building openaiShim subpaths. Apply the same change
to the additional affected path construction around the referenced module
entries, while preserving the existing resolved locations.
In `@src/services/api/openaiShim/requestPreparation.test.ts`:
- Around line 21-44: Replace the source-text assertions that read module files
and match string patterns with behavior assertions that test the actual
functionality through prepareOpenAIRequest. Remove the Bun.file reading logic
from both test cases and instead call prepareOpenAIRequest with test data to
verify that store: false is applied by default in the request body, then verify
that when shimConfig.removeBodyFields includes store, the field is actually
removed from the prepared body. This ensures the tests fail when the actual
behavior breaks rather than when implementation details like variable names
change.
🪄 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: a0afcd10-b987-4df0-8f31-095df428e4b2
📒 Files selected for processing (7)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/requestPreparation.test.tssrc/services/api/openaiShim/requestPreparation.ts
💤 Files with no reviewable changes (2)
- src/services/api/openaiShim/architecture.test.ts
- src/tests/bugfixes.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestPreparation.test.tssrc/services/api/openaiShim/requestPreparation.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestPreparation.test.tssrc/services/api/openaiShim/requestPreparation.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.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestPreparation.test.tssrc/services/api/openaiShim/requestPreparation.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.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestPreparation.test.tssrc/services/api/openaiShim/requestPreparation.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.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/requestPreparation.test.ts
🔇 Additional comments (7)
src/services/api/openaiShim/requestPreparation.ts (2)
93-144: LGTM!Also applies to: 146-205, 207-231, 235-296, 316-353
298-315: 🗄️ Data Integrity & IntegrationThe extraction preserves the original lazy-path behavior; no parity issue exists.
The old inline code at line 1439 already used
request.resolvedModelwithoutruntimeLimitsin the lazy responses path. The extracted code at lines 306-308 maintains this exact pattern. No silent change to context-window trimming has occurred.> Likely an incorrect or invalid review comment.src/services/api/openaiShim.ts (1)
91-91: LGTM!Also applies to: 1187-1215
src/services/api/openaiShim/requestPreparation.test.ts (1)
46-116: LGTM!src/services/api/openaiShim/geminiStreamConversion.test.ts (1)
10-27: LGTM!Also applies to: 29-88
src/services/api/openaiShim.architecture.test.ts (2)
8-40: 📐 Maintainability & Code QualityKeep the architecture budget test unchanged. The current tree has an
activeReductionof 4,301 lines, which produces a 1,335-line budget. The facade is 1,335 lines, so the test passes. The four entries afterupstreamExtractionCountdo not all exist and do not reduce the budget to 676 lines.> Likely an incorrect or invalid review comment.
42-50: 📐 Maintainability & Code QualityEvery production module in
openaiShim/already has a paired test.The test at lines 42–50 passes. All 14 production modules (clientDispatch, geminiStreamConversion, messageConversion, ollamaAdapter, providerCompatibility, rawToolCallParsing, requestExecutor, requestPlanner, requestPreparation, responseConversion, streamControl, streamConversion, toolConversion, xmlToolCallParsing) have corresponding
.test.tsfiles. No type-only or barrel files (such as index.ts or types.ts) are present in the directory.
5c6d17c to
8722bea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/requestPreparation.test.ts`:
- Around line 6-18: Update the test dependencies’ convertTools stub and
assertions in the request preparation test so convertTools returns a distinct
converted OpenAI function-tool object. Assert prepared.body.tools matches that
exact object, while retaining the existing tool_choice assertion and avoiding
raw tool-count-only validation.
🪄 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: 71be965f-9db9-4337-b4c8-20c6e4c575e8
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/services/api/openaiShim/requestPreparation.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
- 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/requestPreparation.test.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/requestPreparation.test.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/requestPreparation.test.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/requestPreparation.test.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/requestPreparation.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/requestPreparation.test.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/requestPreparation.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when setup, commands, or user-facing behavior changes.
Files:
src/services/api/openaiShim/requestPreparation.test.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/requestPreparation.test.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/requestPreparation.test.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/requestPreparation.test.ts
🔇 Additional comments (2)
src/services/api/openaiShim.ts (1)
45-91: LGTM!Also applies to: 385-392, 612-640, 668-668
src/services/api/openaiShim/requestPreparation.test.ts (1)
21-119: 📐 Maintainability & Code QualityConfirm the required validation commands.
The supplied context has no check output. It also does not list exact validation commands in the PR description.
Run and record
bun test ./src/services/api/openaiShim/requestPreparation.test.ts,bun run typecheck,bun run typecheck:type-tests, andbun run build. Do not treat this PR as fully validated until those checks are available.As per path instructions: “Include exact validation commands in the PR description.”
Sources: Coding guidelines, Path instructions
|
@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/requestPreparation.tsrequestPreparation.test.tscoverage without issuing network requestsstore: false, descriptor-drivenstoreremoval, and converted OpenAI tool payloads throughprepareOpenAIRequestmain, retaining the merged transport/response-adapter extractions and the existing architecture/Gemini test improvementsopenaiShim.tsfrom 1,044 lines on the rebased base to 713 linesWhy
Request shaping remained embedded in the facade after
requestPlanner.tsandrequestExecutor.tslanded. This extraction gives the orchestration step a focused source/test owner while preserving the existing executor boundary.Impact
No intended user-facing or provider behavior change. Prepared request data is passed into the existing executor unchanged. Future unpaired production modules under
openaiShim/fail the architecture guard.Validation
bun test --feature=UNATTENDED_RETRY --max-concurrency=1 src/services/api/openaiShim/requestPreparation.test.ts— 3 passbun run test:provider— 1,488 pass; 12 xAI OAuth callback failures reproduce unchanged onupstream/mainwithEADDRINUSEbun run typecheckbun run typecheck:type-testsbun run buildContributor checklist
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit
New Features
Bug Fixes
Tests