refactor(openai-shim): extract transport lifecycle - #2071
Conversation
📝 WalkthroughWalkthroughThe OpenAI shim now delegates transport behavior to ChangesOpenAI transport extraction
Gemini stream validation
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 |
d8f3967 to
a3b939e
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: 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/transport.test.ts`:
- Around line 54-104: Add focused coverage for cancellation during response-body
handling in fetchWithHeadersDeadline, verifying the caller-signal listener is
cleaned up after cancellation. Update the existing
disarms-the-deadline-on-headers-arrive test to use fake timers or a sufficiently
wide timing margin instead of the fixed 30 ms delay, while preserving its
response-body assertion.
🪄 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: 80b21786-ba92-4230-b68c-31f660bae683
📒 Files selected for processing (5)
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.tssrc/services/api/openaiShim/transport.ts
💤 Files with no reviewable changes (1)
- src/services/api/openaiShim.test.ts
📜 Review details
⚠️ CI failures not shown inline (6)
GitHub Actions: PR Checks / web: refactor(openai-shim): extract transport lifecycle
Conclusion: failure
##[group]Run bun run --cwd web build
�[36;1mbun run --cwd web build�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
$ astro check && astro build && bun scripts/verify-dist.ts
�[2m14:04:29�[22m �[34m[types]�[39m Generated �[2m49ms�[22m
�[2m14:04:29�[22m �[34m[check]�[39m Getting diagnostics for Astro files in /home/runner/work/openclaude/openclaude/web...
Result (35 files):
- 0 errors
- 0 warnings
- 0 hints
�[2m14:04:34�[22m �[34m[types]�[39m Generated �[2m50ms�[22m
�[2m14:04:34�[22m �[34m[build]�[39m output: �[34m"static"�[39m
�[2m14:04:34�[22m �[34m[build]�[39m mode: �[34m"static"�[39m
�[2m14:04:34�[22m �[34m[build]�[39m directory: �[34m/home/runner/work/openclaude/openclaude/web/dist/�[39m
�[2m14:04:34�[22m �[34m[build]�[39m Collecting build info...
�[2m14:04:34�[22m �[34m[build]�[39m �[32m✓ Completed in 80ms.�[39m
�[2m14:04:34�[22m �[34m[build]�[39m Building static entrypoints...
�[2m14:04:35�[22m �[34m[vite]�[39m �[32m✓ built in 1.36s�[39m
�[2m14:04:35�[22m �[34m[vite]�[39m �[32m✓ built in 21ms�[39m
�[2m14:04:35�[22m �[34m[build]�[39m Rearranging server assets...
�[42m�[30m generating static routes �[39m�[49m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/404.html�[22m �[2m(+12ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/buddy/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/changelog/index.html�[22m �[2m(+4ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/cli-reference/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/configuration/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/installation/index.html�[22m �[2m(+3ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/keybindings/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/providers/index.html�[22m �[2m(+5ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/quickstart/index.html�[22m �[2m(+4ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/skills/index.html�[22m �[2m(+4ms)�[22m
�[2m14...
GitHub Actions: PR Checks / 2_web.txt: refactor(openai-shim): extract transport lifecycle
Conclusion: failure
##[group]Run bun run --cwd web build
�[36;1mbun run --cwd web build�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
$ astro check && astro build && bun scripts/verify-dist.ts
�[2m14:04:29�[22m �[34m[types]�[39m Generated �[2m49ms�[22m
�[2m14:04:29�[22m �[34m[check]�[39m Getting diagnostics for Astro files in /home/runner/work/openclaude/openclaude/web...
Result (35 files):
- 0 errors
- 0 warnings
- 0 hints
�[2m14:04:34�[22m �[34m[types]�[39m Generated �[2m50ms�[22m
�[2m14:04:34�[22m �[34m[build]�[39m output: �[34m"static"�[39m
�[2m14:04:34�[22m �[34m[build]�[39m mode: �[34m"static"�[39m
�[2m14:04:34�[22m �[34m[build]�[39m directory: �[34m/home/runner/work/openclaude/openclaude/web/dist/�[39m
�[2m14:04:34�[22m �[34m[build]�[39m Collecting build info...
�[2m14:04:34�[22m �[34m[build]�[39m �[32m✓ Completed in 80ms.�[39m
�[2m14:04:34�[22m �[34m[build]�[39m Building static entrypoints...
�[2m14:04:35�[22m �[34m[vite]�[39m �[32m✓ built in 1.36s�[39m
�[2m14:04:35�[22m �[34m[vite]�[39m �[32m✓ built in 21ms�[39m
�[2m14:04:35�[22m �[34m[build]�[39m Rearranging server assets...
�[42m�[30m generating static routes �[39m�[49m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/404.html�[22m �[2m(+12ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/buddy/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/changelog/index.html�[22m �[2m(+4ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/cli-reference/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/configuration/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/installation/index.html�[22m �[2m(+3ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/keybindings/index.html�[22m �[2m(+6ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/providers/index.html�[22m �[2m(+5ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/quickstart/index.html�[22m �[2m(+4ms)�[22m
�[2m14:04:35�[22m �[34m├─�[39m �[2m/docs/skills/index.html�[22m �[2m(+4ms)�[22m
�[2m14...
GitHub Actions: PR Checks / smoke-and-tests (22): refactor(openai-shim): extract transport lifecycle
Conclusion: failure
and caller overrides
##[endgroup]
##[group]src/components/PromptInput/PromptInputFooterLeftSide.test.ts:
(pass) applyHistorySearchActiveState > closes help before activating history search
(pass) applyHistorySearchActiveState > does not change help when history search ends
(pass) resolveFooterOverlay > lets history search replace help and inline suggestions
(pass) resolveFooterOverlay > shows suggestions before help outside history search
(pass) resolveTransientFooterMessage > shows transient feedback outside history search
(pass) resolveTransientFooterMessage > lets history search replace pending transient feedback
(pass) resolveTransientFooterMessage > prioritizes exit feedback when paste feedback is also active
(pass) resolveRegularFooterActive > requires both parent visibility and no transient message
##[endgroup]
##[group]src/components/TrustDialog/utils.test.ts:
(pass) getRelativeSettingsFilePathForSource returns the canonical .openclaude/ paths
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > reports canonical paths when hooks present in both sources [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > reports only project when local has no hooks [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > empty hooks object does not count as having hooks
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > hooks with empty matcher arrays do not count [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > fileSuggestion alone counts as a hook source
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > disableAllHooks suppresses even when hooks are configured [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > null settings produce no sources [1.00ms]
(pass) TrustDialog utils — canonical paths...
GitHub Actions: PR Checks / 1_smoke-and-tests (24.11.x).txt: refactor(openai-shim): extract transport lifecycle
Conclusion: failure
s) classifies a FastAPI validation rejection at its normal 422 status
(pass) classifies a root structured tool_stream unsupported message
(pass) classifies a root tool_stream extra-field rejection alongside tool validation details
(pass) does not classify a generic 400 as tool_stream_unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Tool "tool_stream" is unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Function 'tool_stream' is invalid
(pass) does not classify a tool name error as a tool_stream parameter rejection: Tool: tool_stream is unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Function: tool_stream is invalid
(pass) does not classify a tool name error as a tool_stream parameter rejection: tool_stream is unsupported as a function
(pass) does not classify a tool name error as a tool_stream parameter rejection: tool_stream is unsupported as a tool
(pass) does not classify a tool name error as a tool_stream parameter rejection: Additional properties are not allowed in function tool_stream
(pass) does not classify a tool name error as a tool_stream parameter rejection: The tool named "tool_stream" is unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Function name tool_stream is invalid
(pass) does not classify an invalid schema for a tool named tool_stream as a parameter rejection
(pass) does not classify a raw tool-schema property error as a parameter rejection [1.00ms]
(pass) does not classify a tool-schema error whose location follows the parameter name
(pass) does not classify a generic schema diagnostic as a parameter rejection: Invalid schema: param=tool_stream
(pass) does not classify a generic schema diagnostic as a parameter rejection: Malformed tool schema: unexpected property tool_stream
(pass) does not classify a generic schema diagnostic as a parameter rejecti...
GitHub Actions: PR Checks / smoke-and-tests (24.11.x): refactor(openai-shim): extract transport lifecycle
Conclusion: failure
ortcutsHint > always suppresses during ctrl-r search and caller overrides
##[endgroup]
##[group]src/components/PromptInput/PromptInputFooterLeftSide.test.ts:
(pass) applyHistorySearchActiveState > closes help before activating history search [1.00ms]
(pass) applyHistorySearchActiveState > does not change help when history search ends
(pass) resolveFooterOverlay > lets history search replace help and inline suggestions
(pass) resolveFooterOverlay > shows suggestions before help outside history search
(pass) resolveTransientFooterMessage > shows transient feedback outside history search
(pass) resolveTransientFooterMessage > lets history search replace pending transient feedback
(pass) resolveTransientFooterMessage > prioritizes exit feedback when paste feedback is also active
(pass) resolveRegularFooterActive > requires both parent visibility and no transient message
##[endgroup]
##[group]src/components/TrustDialog/utils.test.ts:
(pass) getRelativeSettingsFilePathForSource returns the canonical .openclaude/ paths
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > reports canonical paths when hooks present in both sources [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > reports only project when local has no hooks [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > empty hooks object does not count as having hooks
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > hooks with empty matcher arrays do not count [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > fileSuggestion alone counts as a hook source [1.00ms]
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > disableAllHooks suppresses even when hooks are configured
(pass) TrustDialog utils — canonical paths from source-of-truth > getHooksSources > null settings produce n...
GitHub Actions: PR Checks / 3_smoke-and-tests (22).txt: refactor(openai-shim): extract transport lifecycle
Conclusion: failure
t its normal 422 status [1.00ms]
(pass) classifies a root structured tool_stream unsupported message
(pass) classifies a root tool_stream extra-field rejection alongside tool validation details
(pass) does not classify a generic 400 as tool_stream_unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Tool "tool_stream" is unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Function 'tool_stream' is invalid
(pass) does not classify a tool name error as a tool_stream parameter rejection: Tool: tool_stream is unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Function: tool_stream is invalid
(pass) does not classify a tool name error as a tool_stream parameter rejection: tool_stream is unsupported as a function
(pass) does not classify a tool name error as a tool_stream parameter rejection: tool_stream is unsupported as a tool
(pass) does not classify a tool name error as a tool_stream parameter rejection: Additional properties are not allowed in function tool_stream
(pass) does not classify a tool name error as a tool_stream parameter rejection: The tool named "tool_stream" is unsupported
(pass) does not classify a tool name error as a tool_stream parameter rejection: Function name tool_stream is invalid
(pass) does not classify an invalid schema for a tool named tool_stream as a parameter rejection
(pass) does not classify a raw tool-schema property error as a parameter rejection
(pass) does not classify a tool-schema error whose location follows the parameter name
(pass) does not classify a generic schema diagnostic as a parameter rejection: Invalid schema: param=tool_stream
(pass) does not classify a generic schema diagnostic as a parameter rejection: Malformed tool schema: unexpected property tool_stream
(pass) does not classify a generic schema diagnostic as a parameter rejection: Additional properties are not allowed: too...
🧰 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.tssrc/services/api/openaiShim/transport.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.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.tssrc/services/api/openaiShim/transport.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.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.tssrc/services/api/openaiShim/transport.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.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.tssrc/services/api/openaiShim/transport.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.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/transport.test.tssrc/services/api/openaiShim.architecture.test.ts
🔇 Additional comments (10)
src/services/api/openaiShim/transport.ts (7)
22-65: LGTM!
67-120: LGTM!
122-177: LGTM!
245-302: LGTM!
304-338: LGTM!
340-383: LGTM!
238-242: 🩺 Stability & AvailabilityNo change required.
All in-repository callers pass
options.signalas bothinit.signalandcallerSignal. No current call path supplies onlyinit.signal.> Likely an incorrect or invalid review comment.src/services/api/openaiShim/transport.test.ts (1)
9-52: LGTM!src/services/api/openaiShim.architecture.test.ts (1)
5-25: LGTM!src/services/api/openaiShim.ts (1)
98-107: 🗄️ Data Integrity & IntegrationPreserve the current shim facade exports.
Only
getApiTimeoutMswas exported bysrc/services/api/openaiShim.ts; the other transport symbols were private. The extraction does not change the shim export surface.> Likely an incorrect or invalid review comment.
a3b939e to
4093316
Compare
4093316 to
7bb26e0
Compare
Rebase onto current main and clean up the transport extraction follow-ups: drop stale facade imports left after the move and restore the full API timeout parser negative-case coverage in transport.test.ts.
7bb26e0 to
3519eda
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 in the test around facadePath
and moduleDirectory to import and use node:path.join for transport and related
module paths. Remove manually appended separators, relying on path.join to
normalize separators across platforms while preserving the existing target
paths.
- Line 19: Update the existence check in openaiShim.architecture.test so the
upstream mandatory slice includes transport.ts by raising the count used for the
initial extractionDeltas slice. Keep the later optional-accounting logic
unchanged, but make sure the assertion loop now covers transport.ts via the same
symbol names used there (upstreamExtractionCount and extractionDeltas) so the
new module is required to exist.
In `@src/services/api/openaiShim/geminiStreamConversion.test.ts`:
- Around line 29-45: Extend the Gemini conversion tests around the existing
“converts Gemini text, tool calls, usage, and finish state” case with separate
no-tool responses that verify STOP maps to end_turn and, when supported by the
converter, MAX_TOKENS maps to max_tokens. Keep the existing tool-call coverage
intact, but ensure finish-reason assertions run without functionCall parts so
they validate the finish mapping directly.
- Around line 77-88: Extend the test “rejects an already-aborted Gemini stream
without yielding events” to use a custom ReadableStream or cancellation spy,
then assert that the stream reader’s cancel operation is invoked when the
AbortSignal is already aborted. Preserve the existing AbortError rejection
assertion and keep the coverage focused on cleanup during initial abort
handling.
- Around line 35-68: Strengthen the test around geminiSseToAnthropic by
asserting the Read tool input includes file_path: 'a.ts', and verify the emitted
events occur in order: content-block start, tool input, content-block stop,
message_delta, then message_stop. Preserve the existing message_start and text
assertions while adding these user-visible tool-event checks.
🪄 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: b6433eaa-0910-48de-84e5-15965f8228a5
📒 Files selected for processing (7)
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/transport.test.tssrc/services/api/openaiShim/transport.ts
💤 Files with no reviewable changes (2)
- src/services/api/openaiShim/architecture.test.ts
- 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: smoke-and-tests (22)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with strict mode and ESM imports.
Use React and Ink for terminal UI components.
Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color,commanderfor CLI argument parsing, andexecafor child processes where those concerns are needed.
Files:
src/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.ts
src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place API, MCP, OAuth, wiki, voice, and other service integrations under
src/services/.
Files:
src/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.ts
**/*.{ts,tsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Do not silently change provider tags; maintainers control them during review.
Files:
src/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{js,jsx,ts,tsx}: Follow the existing code style in touched JavaScript and TypeScript source files.
Keep comments useful and concise in JavaScript and TypeScript source files.
Files:
src/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.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/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.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/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/transport.test.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/geminiStreamConversion.test.tssrc/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/transport.test.ts
🔇 Additional comments (9)
src/services/api/openaiShim.ts (1)
98-106: LGTM!src/services/api/openaiShim.architecture.test.ts (1)
42-50: LGTM!src/services/api/openaiShim/transport.test.ts (2)
86-104: Add a post-header cancellation regression test.The mocked
fetchignoresinit.signal. An active deadline can abort after headers without changingresponse.text(). This test cannot verify that the deadline is disarmed.Capture the request signal and assert that it remains un-aborted after the deadline interval. Also exercise early body cancellation through the fallback signal-composition path. After cancellation, abort the caller signal and verify that the captured combined signal does not abort.
As per path instructions, add focused tests for transport lifecycle, timeout, and cancellation behavior.
Source: Path instructions
1-84: LGTM!src/services/api/openaiShim/transport.ts (4)
1-65: LGTM!
67-177: LGTM!
179-243: LGTM!
245-383: LGTM!src/services/api/openaiShim/geminiStreamConversion.test.ts (1)
1-27: LGTM!
Use path.join in the architecture guard, require transport.ts in the mandatory extraction slice, strengthen Gemini stream conversion coverage, and add transport deadline/cancellation regression tests with fake timers.
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/transport.test.ts`:
- Around line 115-141: Update the test around fetchWithHeadersDeadline so that
after response.body.cancel() completes cleanupAfterBody, a later caller.abort()
does not abort capturedSignal or propagate its reason. Keep the timeout
assertion intact and change the final expectations to verify the cleaned-up
combined request signal remains un aborted.
🪄 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: 47107cb4-2595-4a84-aed2-eb9fd346ccd8
📒 Files selected for processing (3)
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/transport.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with strict mode and ESM imports.
Use React and Ink for terminal UI components.
Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color,commanderfor CLI argument parsing, andexecafor child processes where those concerns are needed.
Files:
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/transport.test.ts
src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place API, MCP, OAuth, wiki, voice, and other service integrations under
src/services/.
Files:
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/transport.test.ts
**/*.{ts,tsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Do not silently change provider tags; maintainers control them during review.
Files:
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/transport.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{js,jsx,ts,tsx}: Follow the existing code style in touched JavaScript and TypeScript source files.
Keep comments useful and concise in JavaScript and TypeScript source files.
Files:
src/services/api/openaiShim.architecture.test.tssrc/services/api/openaiShim/geminiStreamConversion.test.tssrc/services/api/openaiShim/transport.test.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/transport.test.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/transport.test.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/transport.test.ts
🔇 Additional comments (6)
src/services/api/openaiShim/transport.test.ts (1)
1-19: LGTM!Also applies to: 88-113
src/services/api/openaiShim.architecture.test.ts (1)
2-2: LGTM!Also applies to: 26-34
src/services/api/openaiShim/geminiStreamConversion.test.ts (4)
29-43: LGTM!
45-94: LGTM!
96-124: LGTM!
126-144: LGTM!
Exercise the combineRequestSignals fallback without AbortSignal.any so early body cancellation removes caller listeners and a later caller.abort does not abort the combined fetch signal.
|
Addressed the remaining transport.test.ts review: the early body-cancel case now disables |
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/transport.test.ts`:
- Around line 117-124: Update the AbortSignal.any cleanup in the test’s finally
block to distinguish an originally absent property from an originally defined
one: restore the saved descriptor when originalAbortSignalAny exists, otherwise
delete the temporary AbortSignal.any property. Ensure each test leaves the
global AbortSignal state exactly as it was before the test.
🪄 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: 88faf541-d1de-4141-adf0-fee602a89dfc
📒 Files selected for processing (1)
src/services/api/openaiShim/transport.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: web
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with strict mode and ESM imports.
Use React and Ink for terminal UI components.
Prefer existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Usechalkfor terminal color,commanderfor CLI argument parsing, andexecafor child processes where those concerns are needed.
Files:
src/services/api/openaiShim/transport.test.ts
src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place API, MCP, OAuth, wiki, voice, and other service integrations under
src/services/.
Files:
src/services/api/openaiShim/transport.test.ts
**/*.{ts,tsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Do not silently change provider tags; maintainers control them during review.
Files:
src/services/api/openaiShim/transport.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{js,jsx,ts,tsx}: Follow the existing code style in touched JavaScript and TypeScript source files.
Keep comments useful and concise in JavaScript and TypeScript source files.
Files:
src/services/api/openaiShim/transport.test.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/transport.test.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/transport.test.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/transport.test.ts
Delete the temporary AbortSignal.any override when the runtime did not define an own property, so transport and facade signal-cleanup tests leave global AbortSignal state unchanged for later cases.
2 participants
Summary
openaiShim/transport.tsandtransport.test.tsopenaiShim.tsAPI while reducing the facade from 1,582 to 1,221 lines (10 additions, 371 deletions)geminiStreamConversion.test.tsopenaiShim.tsand enforce same-basename source/test ownership for every production module underopenaiShim/Why
The merged OpenAI shim extractions left transport lifecycle and diagnostic logic in the facade. This isolates that cohesive responsibility without depending on any other follow-up extraction.
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.Impact
No intended user-facing or provider behavior change. Transport timeout, cancellation, proxy retry, redaction, and error-classification behavior remains wired through the same facade entrypoints. Future unpaired production modules under
openaiShim/fail the architecture guard.Validation
bun run typecheckbun run buildbun run smokebun run deadcodebun run check(environment-limited: 7,792 pass, 2 skip, 45 unrelated sandbox/filesystem/loopback failures)Contributor checklist
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit
New Features
Tests