Repository navigation
refactor(openai-shim): extract Ollama adapter - #2004
Conversation
📝 WalkthroughWalkthroughThe Ollama OpenAI-compatibility implementation moves into a dedicated adapter, with request normalization, response conversion, streaming support, shim integration, and focused regression tests. ChangesOllama adapter extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/__tests__/bugfixes.test.ts`:
- Around line 94-106: Replace the source-text checks in the test “Ollama adapter
uses native chat with request-level num_ctx” with behavioral coverage of the
Ollama provider/model path: verify native /api/chat request normalization and
both streaming and non-streaming response conversion. Reuse existing
endpoint-test fixtures or helpers where available; if that path is already
covered, remove this redundant test.
In `@src/services/api/openaiShim/ollamaAdapter.ts`:
- Around line 100-131: Update normalizeOllamaNativeMessages to track assistant
tool-call IDs and their function names while iterating messages, then translate
role "tool" messages from tool_call_id to the corresponding native tool_name and
omit tool_call_id. Preserve existing message normalization for other roles and
add a regression test covering an assistant tool call followed by its tool
result.
- Around line 173-176: Update the non-streaming response construction in the
Ollama adapter so finish_reason is set to "tool_calls" whenever toolCalls is
non-empty, otherwise preserve mapOllamaDoneReason(data.done_reason). Add or
update coverage for a native non-streaming tool response to verify this
behavior.
🪄 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: 9c140ef4-d459-4a2c-acd5-6b146d865c52
📒 Files selected for processing (5)
src/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/ollamaAdapter.test.tssrc/services/api/openaiShim/ollamaAdapter.ts
💤 Files with no reviewable changes (1)
- src/services/api/openaiShim.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
- 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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/ollamaAdapter.test.tssrc/services/api/openaiShim/ollamaAdapter.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/ollamaAdapter.test.tssrc/services/api/openaiShim/ollamaAdapter.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/ollamaAdapter.test.tssrc/services/api/openaiShim/ollamaAdapter.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/__tests__/bugfixes.test.tssrc/services/api/openaiShim/ollamaAdapter.test.tssrc/services/api/openaiShim/ollamaAdapter.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/ollamaAdapter.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/ollamaAdapter.test.tssrc/services/api/openaiShim/ollamaAdapter.tssrc/services/api/openaiShim.ts
🔇 Additional comments (2)
src/services/api/openaiShim.ts (1)
122-128: LGTM!Also applies to: 478-478, 4356-4365
src/services/api/openaiShim/ollamaAdapter.test.ts (1)
1-8: 📐 Maintainability & Code QualityReport the required provider validation results.
The supplied context reports shim tests and
typecheck, but notbun run typecheck:type-tests,bun run test:provider,bun run test:provider-recommendation, orbun run security:pr-scan. Please run and report them before merge.As per coding guidelines, “Run provider tests and provider recommendation tests when changing provider behavior” and “Run
bun run security:pr-scanbefore submitting a pull request”; as per path instructions, run the listed focused adapter and shim tests.Sources: Coding guidelines, Path instructions
907c0cf to
890ab7d
Compare
78b7cd0 to
b201388
Compare
b201388 to
50ed940
Compare
50ed940 to
89cafc7
Compare
jatmn
left a comment
There was a problem hiding this comment.
@kevincodex1 LGTM, please merge
Summary
Extracts native Ollama request/response adaptation from
src/services/api/openaiShim.tsintosrc/services/api/openaiShim/ollamaAdapter.ts.The module owns native URL selection, context sizing, multipart/tool-message normalization, and native streaming/non-streaming conversion. The façade keeps route selection and delegates the adapter work.
All branches share stable source and test extraction seams so accumulated merges remain conflict-free in arbitrary order.
Independent merge
mainat0effa0f42b6dbc6a4800e19f4b2d8d588269906b.jatmn/openclaude:de-mono1-ollama-adapter.b2013885285c2ed208ae4cf22bada64612a2124b.Source-file budget
openaiShim.ts: +45 / -408 = 453 changed lines, below the 1,500-line cap. Tests are excluded.Test migration
ollamaAdapter.test.ts: 321 lines / 7 focused tests.Validation
git diff --check,bun run typecheck,bun run typecheck:type-tests, andbun run build: passed.bun run check: 7,190 passed / 2 skipped; only the same 10 pre-existing PowerShell governance failures remain on Linux.Prepared according to
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit