Repository navigation
refactor(openai-shim): extract stream control - #2002
Conversation
📝 WalkthroughWalkthroughThe PR extracts shared stream-control utilities, moves Anthropic SSE handling into a dedicated module, updates tool-call sequencing and suppression state, and reorganizes shim tests with new façade and JSON-fallback coverage. ChangesStream control extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (4 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/streamControl.ts`:
- Around line 93-117: Update cancelAndReject in the stream control flow to call
finishReject(error) before invoking cancellation, and guard both custom
cancelReader and reader.cancel failures so cancellation cannot prevent
settlement or cleanup. Add a regression test with a synchronously throwing
cancelReader, and verify abort and timeout cancellation still reject and clear
the timer/listener.
🪄 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: 820468b7-48aa-4057-9fa9-94064fa0cd3a
📒 Files selected for processing (5)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.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 (22)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
🧰 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/streamControl.test.tssrc/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.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/streamControl.test.tssrc/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.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/streamControl.test.tssrc/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.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/streamControl.test.tssrc/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.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/streamControl.test.tssrc/services/api/openaiShim/streamControl.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/streamControl.test.tssrc/__tests__/bugfixes.test.ts
🔇 Additional comments (4)
src/services/api/openaiShim/streamControl.ts (1)
1-92: LGTM!Also applies to: 119-120
src/services/api/openaiShim/streamControl.test.ts (1)
1-86: LGTM!src/services/api/openaiShim.ts (1)
123-132: LGTM!src/__tests__/bugfixes.test.ts (1)
65-66: LGTM!Also applies to: 80-80
cd411d7 to
c5dfe58
Compare
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.test.ts`:
- Around line 5899-5901: Update the test “the OpenAI shim façade creates
independent client instances” to compare the state-bearing nested namespaces
directly, such as beta and messages, across two separately created clients. Keep
the outer-object assertion only if useful, and ensure the test fails when those
nested client instances are shared.
In `@src/services/api/openaiShim.ts`:
- Line 1636: Add focused regression coverage for the shared fallback ID sequence
used by the JSON/Ollama and XML parser paths around the result construction at
`results.push` and the corresponding parser branches. Invoke both parsers within
one session, verify their generated fallback IDs are unique and reflect one
interleaved sequence, and use the exact provider/model path affected by the
change.
In `@src/services/api/openaiShim/streamControl.ts`:
- Around line 166-172: Update the stream parsing around the SSE buffer in the
stream-control method to recognize event delimiters formed by LF, CRLF, and CR,
while preserving the existing buffering of incomplete events and data-line
handling. Add a regression test covering CRLF-delimited events through the
OpenAI-compatible shim’s provider/model path.
🪄 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: f75cc74b-db51-422e-96df-5426058d04d6
📒 Files selected for processing (5)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/streamControl.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/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.ts
🔇 Additional comments (6)
src/services/api/openaiShim.ts (3)
141-151: LGTM!
1965-1972: LGTM!
2181-2182: LGTM!Also applies to: 2358-2359, 3277-3278
src/__tests__/bugfixes.test.ts (1)
65-66: LGTM!Also applies to: 80-80
src/services/api/openaiShim.test.ts (2)
1504-1506: LGTM!Also applies to: 1827-1828, 2055-2056, 2873-2877, 4377-4378, 4486-4487, 4794-4795, 5546-5547, 5668-5669, 5802-5817
6195-6196: LGTM!Also applies to: 10145-10157
5c55523 to
6fe9565
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/api/openaiShim.test.ts (2)
524-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the numbered extraction seam wrappers.
The repeated
seam N start/endcomments add hundreds of lines while merely restating adjacent test names. Retain only the meaningful extraction-boundary documentation.As per coding guidelines, “Keep comments useful and concise” and “Prefer small, readable changes over broad rewrites.”
🤖 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.test.ts` around lines 524 - 586, Remove the numbered “openaiShim test extraction seam 001 start/end” wrapper comments around the test, while preserving the test name, implementation, assertions, and any meaningful extraction-boundary documentation.Source: Coding guidelines
21-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
OPENAI_AZURE_STYLEtest isolation.Removing it from snapshot, cleanup, and restoration makes provider tests depend on the caller’s environment. The shim uses this variable to switch requests to Azure
api-keyauthentication, potentially altering unrelated routing and header assertions.As per path instructions, tests must isolate global/env/config state and provider profile leaks.
Also applies to: 436-520
🤖 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.test.ts` around lines 21 - 27, Restore OPENAI_AZURE_STYLE in the test environment isolation flow: include it in the environment snapshot, clear it during cleanup, and restore its original value afterward. Update the related setup and teardown logic around the provider tests so Azure authentication state cannot leak from or affect the caller environment.Source: Path instructions
🤖 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/streamControl.ts`:
- Around line 171-174: Update the SSE parsing in the stream-control method
containing the chunk line processing to accept fields beginning with “data:”
whether or not a space follows it, removing at most one optional leading space
before joining payloads. Add a focused regression test covering the
OpenAI-compatible shim/provider model path with a valid “data:{...}” frame.
---
Outside diff comments:
In `@src/services/api/openaiShim.test.ts`:
- Around line 524-586: Remove the numbered “openaiShim test extraction seam 001
start/end” wrapper comments around the test, while preserving the test name,
implementation, assertions, and any meaningful extraction-boundary
documentation.
- Around line 21-27: Restore OPENAI_AZURE_STYLE in the test environment
isolation flow: include it in the environment snapshot, clear it during cleanup,
and restore its original value afterward. Update the related setup and teardown
logic around the provider tests so Azure authentication state cannot leak from
or affect the caller environment.
🪄 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: 11af320d-c416-4fe6-9add-8337ad74d3ae
📒 Files selected for processing (5)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.ts
📜 Review details
🧰 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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.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/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (7)
src/services/api/openaiShim/streamControl.ts (2)
167-171: Existing blocker remains: support CRLF and CR SSE delimiters.The parser still recognizes only LF-delimited events. This provider-compatibility issue was already raised in the prior review.
1-125: LGTM!Also applies to: 127-166, 175-197
src/services/api/openaiShim/streamControl.test.ts (1)
1-305: LGTM!src/services/api/openaiShim.ts (1)
141-151: LGTM!Also applies to: 1636-1636, 1705-1710, 1852-1852, 1965-1972, 3829-3833, 3884-3884, 3929-3929, 4062-4062, 4080-4082, 4385-4387, 4429-4431, 4791-4793, 4850-4852
src/services/api/openaiShim.test.ts (2)
6400-6404: Assert isolation of the nested client namespaces.The outer wrapper is always newly allocated, so this still passes if
beta.messagesis shared. Compare the nested instances directly.As per path instructions, review tests for meaningful coverage and provider-profile leaks.
Source: Path instructions
3203-3209: LGTM!Also applies to: 6305-6310, 10940-10953
src/__tests__/bugfixes.test.ts (1)
65-66: LGTM!Also applies to: 80-80
6fe9565 to
4781f42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
3406-3420: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSeparate the provider-routing changes from the stream-control extraction.
These ranges change provider-override env precedence, GPT-5.4–5.6 reasoning behavior, local streaming payloads, and Azure URL construction. They materially expand this PR beyond its stated lifecycle-extraction objective. Move them to a focused provider PR and explicitly document affected providers, limitations, and follow-up work.
As per coding guidelines, “Keep changes focused on one problem or feature” and “Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.” As per path instructions, keep changes focused and avoid broad rewrites.
Also applies to: 3532-3532, 3637-3644, 3661-3661, 3698-3716, 3750-3750, 4227-4229, 4313-4355, 4370-4370
🤖 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 3406 - 3420, Revert the provider-routing changes from the stream-control extraction, including the provider override environment handling around resolveProviderRequest and the related reasoning, local streaming, and Azure URL logic identified in the diff. Keep only the lifecycle/stream-control extraction changes, and move provider behavior updates to a separate focused change with the required provider scope, limitations, and follow-up documentation.Sources: Coding guidelines, Path instructions
🤖 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/streamControl.ts`:
- Around line 159-168: Update the stream-processing loop around
throwIfStreamAborted to flush the decoder and parse any remaining buffered SSE
event before handling EOF, preserving normal stream completion afterward. Ensure
a final data event without a trailing blank line is emitted rather than dropped,
and add a focused Anthropic-compatible regression test covering this EOF case.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3406-3420: Revert the provider-routing changes from the
stream-control extraction, including the provider override environment handling
around resolveProviderRequest and the related reasoning, local streaming, and
Azure URL logic identified in the diff. Keep only the lifecycle/stream-control
extraction changes, and move provider behavior updates to a separate focused
change with the required provider scope, limitations, and follow-up
documentation.
🪄 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: da1eed78-449d-4d15-8eb3-05f2941e6922
📒 Files selected for processing (5)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.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/streamControl.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (10)
src/services/api/openaiShim/streamControl.ts (3)
167-171: Support CRLF and CR SSE delimiters.This remains present and duplicates the prior unresolved provider-compatibility finding.
171-174: Acceptdata:fields without requiring a space.This remains present and duplicates the prior unresolved provider-compatibility finding.
1-125: LGTM!src/services/api/openaiShim/streamControl.test.ts (1)
1-304: LGTM!src/services/api/openaiShim.ts (2)
1639-1639: The shared fallback sequence still lacks interleaving regression coverage.Add one session-level test exercising both JSON/Ollama and XML parsing and asserting unique IDs. This matches the existing review finding.
As per coding guidelines, “Add or update tests when a code change affects behavior.” As per path instructions, use the narrowest meaningful regression coverage.
Also applies to: 1708-1713, 1855-1855
Sources: Coding guidelines, Path instructions
85-90: LGTM!Also applies to: 144-154, 1968-1975, 2184-2185, 2361-2362, 3280-3281, 3858-3862, 3913-3913, 3958-3958, 4091-4091, 4109-4111, 4426-4428, 4470-4472, 4832-4834, 4891-4893
src/services/api/openaiShim.test.ts (3)
689-1254: Covered by the provider-scope finding insrc/services/api/openaiShim.ts.Also applies to: 2922-2941, 9879-9881, 9956-9958
6650-6652: This still verifies only outer-object allocation, not client-state isolation.Compare
betaandbeta.messagesbetween clients. This matches the existing review finding.As per path instructions, review tests for meaningful coverage and provider-profile leaks.
Source: Path instructions
25-25: LGTM!Also applies to: 441-441, 485-485, 2073-2076, 2785-2786, 3624-3628, 5128-5129, 5237-5238, 5545-5546, 6297-6298, 6419-6420, 6553-6568, 6946-6947, 7246-7248, 7458-7460, 7564-7566, 7647-7649, 7868-7870, 9304-9306, 9360-9362, 9514-9516, 10209-10211, 10688-10690, 10932-10944
src/__tests__/bugfixes.test.ts (1)
65-66: LGTM!Also applies to: 80-80
4781f42 to
428366b
Compare
428366b to
b2eb372
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
169-445: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSplit unrelated behavior from this stream-control extraction.
The PR objective is stream lifecycle extraction, but it also changes header-deadline/retry policy, onboarding UI coverage, and LongCat provider behavior. Keep these independently reviewable and revert/split them from this PR.
src/services/api/openaiShim.ts#L169-L445: move response-header deadline and retry policy changes to a dedicated PR.src/__tests__/bugfixes.test.ts#L698-L748: move onboarding/trust-dialog coverage with its corresponding UI change.src/services/api/openaiShim.test.ts#L4689-L4863: move LongCat routing and capability coverage with its provider change.As per coding guidelines, “Keep pull requests focused on one issue or one clearly scoped improvement”; as per path instructions, “Keep changes focused on a single problem.”
🤖 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 169 - 445, Split unrelated changes out of this stream-lifecycle PR: remove the response-header deadline and retry-policy changes around fetchWithHeadersDeadline and its supporting symbols in src/services/api/openaiShim.ts (lines 169-445), move the onboarding/trust-dialog coverage and corresponding UI changes from src/__tests__/bugfixes.test.ts (lines 698-748) to a separate change, and move the LongCat routing and capability coverage from src/services/api/openaiShim.test.ts (lines 4689-4863) with its provider changes; retain only stream-control extraction here.Sources: Coding guidelines, Path instructions
🤖 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/streamControl.ts`:
- Around line 175-177: Update the `[DONE]` handling in the stream reader to
cancel the underlying source before returning, while preserving the
`streamComplete` state. Add a regression test covering a provider that sends
`[DONE]` but never reaches reader EOF, and assert that the source/reader is
cancelled.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 169-445: Split unrelated changes out of this stream-lifecycle PR:
remove the response-header deadline and retry-policy changes around
fetchWithHeadersDeadline and its supporting symbols in
src/services/api/openaiShim.ts (lines 169-445), move the onboarding/trust-dialog
coverage and corresponding UI changes from src/__tests__/bugfixes.test.ts (lines
698-748) to a separate change, and move the LongCat routing and capability
coverage from src/services/api/openaiShim.test.ts (lines 4689-4863) with its
provider changes; retain only stream-control extraction here.
🪄 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: 431af2fc-810b-4a2b-92d5-142dd96c0cc4
📒 Files selected for processing (5)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- 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}: Provider changes must follow the documented integration patterns and avoid inconsistent behavior across provider paths.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Review AI-generated code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submission.
Run multiple rounds of self-review on AI-generated code; compilation alone is insufficient to establish correctness.
Files:
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep pull requests focused on one issue or one clearly scoped improvement; avoid unrelated cleanup, fixes, features, or refactors in the same change.
Preserve existing repository patterns unless intentionally refactoring them, and stay within the project's existing language, runtime, dependency, and architectural direction.
Add or update tests when a change affects behavior.
Update documentation when setup, commands, or user-facing behavior changes.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files merely because they are nearby.
Keep comments useful and concise.
Run the narrowest meaningful validation command for the touched area before opening a pull request, and ensure relevant CI checks pass before merge.
Provider-change pull requests must identify affected providers, state the tested provider/model path, and document limitations or follow-up work.
Do not assign or use provider tags; provider tags are controlled and applied by maintainers.
Security reports must follow the instructions inSECURITY.md.
PR descriptions must explain what changed and why, user or developer impact, exact checks run, and include relevant issue links; UI, terminal presentation, or VS Code extension changes require screenshots.
PR authors must address CodeRabbit findings before maintainer review proceeds.
Files:
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim/streamControl.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/streamControl.test.tssrc/services/api/openaiShim.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/streamControl.test.tssrc/services/api/openaiShim/streamControl.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
🔇 Additional comments (1)
src/services/api/openaiShim.test.ts (1)
7908-7909: 🎯 Functional CorrectnessNo redeclaration issue here
Eachconst erroris in its own test scope, so there’s no TypeScript redeclaration failure.> Likely an incorrect or invalid review comment.
2 participants
Summary
Extracts stream lifecycle primitives from
src/services/api/openaiShim.tsintosrc/services/api/openaiShim/streamControl.ts.The module owns abort errors, idempotent reader cancellation, idle-timeout policy, and Anthropic-compatible SSE parsing.
openaiShim.tsremains the stable façade. Lifecycle/controller dispatch integration stays in the façade suite because it crosses the later dispatch boundary; the module-owned stream-control behaviors move to the focused suite.All branches share stable source and test extraction seams. Those seams keep adjacent edits disjoint after multiple earlier PRs have merged, not merely when branches are compared pairwise.
Independent merge
Gitlawb/openclaude:mainat0effa0f42b6dbc6a4800e19f4b2d8d588269906b.jatmn/openclaude:de-mono1-stream-control.6fe9565163add802d296884c4b4b386b0b5a4e52.Source-file budget
src/services/api/openaiShim.ts: +56 / -201 = 257 changed lines, below the 1,500-line source-file cap. Test changes are excluded from that cap.Test migration
src/services/api/openaiShim.test.ts: +696 / -113, including the common stable extraction anchors; the four stream-control test bodies are removed from the monolith.src/services/api/openaiShim/streamControl.test.ts: 304 lines / 11 tests.Validation
git diff --check: passed.bun run typecheck: passed.bun run typecheck:type-tests: passed.bun run build: passed.bun run check: build, smoke, bundle, and dead-code checks passed; 7,193 passed / 2 skipped, with only the same 10 pre-existing PowerShell governance failures on Linux.Prepared according to
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit
[DONE]/EOF.message_stopevent.