Repository navigation
refactor(openai-shim): extract typed request body planning - #2010
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe OpenAI shim now delegates compatibility environment hydration and transport-specific request-body planning to ChangesOpenAI shim request planning
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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: 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/requestPlanner.test.ts`:
- Around line 15-18: Update the test around hydrateOpenAIShimCompatibilityEnv to
make the credential resolver stub capture its input and assert that the Bankr
base URL, https://bankr.test/v1, is supplied as baseUrl when resolving
route-key. Preserve the existing assertions for OPENAI_BASE_URL, OPENAI_MODEL,
and OPENAI_API_KEY.
🪄 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: bf064e8c-4467-4232-8902-8df226082dae
📒 Files selected for processing (3)
src/services/api/openaiShim.tssrc/services/api/openaiShim/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Runbun run typecheckandbun run typecheck:type-testsfor TypeScript changes.
Run provider tests and provider recommendation tests when changing provider behavior:bun run test:providerandbun run test:provider-recommendation.
Files:
src/services/api/openaiShim/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.tssrc/services/api/openaiShim.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Runbun run security:pr-scanbefore submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.
Files:
src/services/api/openaiShim/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.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/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/services/api/openaiShim/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.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/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.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/requestPlanner.test.ts
🔇 Additional comments (2)
src/services/api/openaiShim/requestPlanner.ts (1)
1-25: LGTM!src/services/api/openaiShim.ts (1)
123-123: LGTM!Also applies to: 1056-1059
f9b0657 to
688c6e6
Compare
jatmn
changed the title
1e8a2cf to
4c2c1f8
Compare
4c2c1f8 to
dd4d122
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openaiShim.ts (1)
1836-1841: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the stacked extraction-boundary comments.
Two boundary blocks now sit back-to-back, and "Native Ollama/body serialization remains request-planner-owned" reads as pending work when the planner already owns it. One accurate marker above Line 1842 is enough.
As per coding guidelines, "keep comments useful and concise".
🤖 Prompt for 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. In `@src/services/api/openaiShim.ts` around lines 1836 - 1841, Consolidate the adjacent extraction-boundary comment blocks near the executor/request serialization transition into one concise, accurate marker above the executor attempt loop. Remove the obsolete statement implying native Ollama/body serialization is pending, while retaining only the boundary and lazy serialized-body ownership details needed by the extraction.Source: Coding guidelines
🤖 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.ts`:
- Around line 1454-1462: Remove buildAnthropicMessagesBody, buildGeminiBody, and
buildOllamaChatBody from the planner destructuring near
createRequestBodyPlanner; retain only the builders directly used by the
surrounding executor, including serializeBody.
In `@src/services/api/openaiShim/requestPlanner.test.ts`:
- Around line 344-355: Extend the test shared omit flags rebuild transport
bodies without tools to also cover Anthropic and Gemini planners, setting
omitTools.anthropic and omitTools.gemini and asserting their rebuilt bodies omit
both tools and tool_choice. Preserve the existing Responses assertions and use
the planner’s transport-specific body builders.
In `@src/services/api/openaiShim/requestPlanner.ts`:
- Around line 416-425: Move the stable-stringify rationale comment from above
buildOllamaChatBody to the serializeBody implementation around line 447, keeping
it adjacent to the logic that handles prefix-cache byte identity and the
skipStableStringify opt-out. Do not alter the serialization behavior or
unrelated comments.
- Around line 234-239: Update the tool_choice assignment in the request-planning
flow to use the same omitTools.anthropic guard as the tools assignment, so
toolless retries omit tool_choice when tools are omitted. Preserve existing
tool_choice behavior when Anthropic tools are included.
- Around line 42-43: Update the RequestTransport type alias to preserve
literal-member completion and narrowing while still accepting arbitrary
transport strings, using a branded open-ended string form instead of a direct “|
string” union. Ensure effectiveTransport and request.transport continue
accepting custom transport values.
- Around line 178-185: Remove the redundant inline type casts from both calls to
convertToolsToResponsesTools, including the block near the params.tools check
and the corresponding block around the later usage. Pass params.tools directly,
preserving the existing guards and conversion behavior.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 1836-1841: Consolidate the adjacent extraction-boundary comment
blocks near the executor/request serialization transition into one concise,
accurate marker above the executor attempt loop. Remove the obsolete statement
implying native Ollama/body serialization is pending, while retaining only the
boundary and lazy serialized-body ownership details needed by the extraction.
🪄 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: 99518197-4289-47f1-ae3c-11fa2df36b1f
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.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 (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/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.tssrc/services/api/openaiShim.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/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.tssrc/services/api/openaiShim.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/requestPlanner.test.tssrc/services/api/openaiShim/requestPlanner.tssrc/services/api/openaiShim.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/requestPlanner.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (7)
src/services/api/openaiShim/requestPlanner.ts (3)
1-40: LGTM!
241-273: LGTM!Also applies to: 277-414
447-470: LGTM!src/services/api/openaiShim.ts (1)
86-89: LGTM!Also applies to: 719-722
src/services/api/openaiShim/requestPlanner.test.ts (2)
1-43: LGTM!Also applies to: 45-91, 93-152, 154-203, 205-304
357-366: LGTM!src/services/api/openaiShim.test.ts (1)
1331-1336: LGTM!Also applies to: 1372-1372
Add planner tests that exercise serializeBody() for responses, anthropic_messages, and gemini transports, including omit-flag rebuilds. Remove the vacuous Gemini tool_choice assertion that never guarded behavior.
2 participants
Summary
Extracts transport request-body planning into
openaiShim/requestPlanner.ts.The typed planner owns Responses, Anthropic Messages, Gemini, and native Ollama body builders; compatibility environment hydration; serialization; and shared omit-tools retry state. The façade prepares provider context and delegates transport-specific body construction.
Independent merge
0effa0f42b6dbc6a4800e19f4b2d8d588269906b.jatmn/openclaude:de-mono1-request-planner.1e8a2cfafb74f93eb3b970147f21d71a236c20ea.Source-file budget
openaiShim.ts: +59 / -339 = 398 changed lines, below 1,500; tests excluded.Test migration
requestPlanner.test.ts: 359 lines / 11 focused tests.Validation
bun run check: 7,196 passed / 2 skipped; only 10 pre-existing PowerShell governance failures remain on Linux.Prepared according to
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit