fix: Ollama provider tool result handling and premature context condensing#848
fix: Ollama provider tool result handling and premature context condensing#848navedmerchant wants to merge 3 commits into
Conversation
navedmerchant
requested review from
JamesRobert20,
edelauna,
hannesrudolph and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates Ollama model parsing and loading, tool-result conversion, and tool schema handling. It also adds tests covering maxTokens behavior, partial model fetch failures, tool-result role mapping, image placement, and recursive schema sanitization. ChangesOllama provider fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (2)
src/api/providers/__tests__/native-ollama.spec.ts (1)
788-864: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a nested-schema case.
This test only covers top-level
additionalPropertiesstripping. As noted innative-ollama.ts(convertToolsToOllama), the current implementation only strips the top-level key, so a schema withadditionalPropertiesnested inside apropertiessub-object wouldn't be covered here or fixed in the implementation. Adding a nested-schema fixture would catch a regression once/if the stripping logic is made recursive. As per coding guidelines,**/*.{test,spec}.{ts,tsx,js}should cover "validation, serialization, request construction" for pure logic — this is exactly that kind of case.🤖 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/api/providers/__tests__/native-ollama.spec.ts` around lines 788 - 864, The current test only verifies top-level removal of additionalProperties in NativeOllamaHandler’s tool conversion, so it misses nested schema cases. Extend the native-ollama.spec.ts coverage around createMessage and convertToolsToOllama by adding a fixture with additionalProperties inside a nested properties object, then assert the outgoing mockChat tools payload strips that nested key as well. This should exercise the same tool schema path used by apply_diff and confirm the implementation handles recursive schema sanitization, not just the top-level parameters object.Source: Coding guidelines
src/api/providers/native-ollama.ts (1)
208-225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrip
additionalPropertiesrecursivelyNative tools already nest
additionalPropertiesunderitemsand nested objects, e.g.src/core/prompts/tools/native-tools/ask_followup_question.tsandsrc/core/prompts/tools/native-tools/read_file.ts, so the current top-level delete leaves those fields in Ollama payloads.🤖 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/api/providers/native-ollama.ts` around lines 208 - 225, The tool schema sanitization in native-ollama only removes additionalProperties at the top level, so nested occurrences still leak into Ollama payloads. Update the mapping logic in the tool conversion path to recursively strip additionalProperties from tool.function.parameters, including nested objects and items, while preserving the rest of the schema shape.
🤖 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/api/providers/fetchers/ollama.ts`:
- Around line 56-59: The Ollama fetcher tests still assert the old maxTokens
value, so update the expectations in ollama.test.ts to match the new default
inherited by parseOllamaModel. Adjust both failing assertions to use maxTokens
4096 instead of 40960, keeping the test cases aligned with
ollamaDefaultModelInfo and the behavior of parseOllamaModel.
In `@src/api/providers/native-ollama.ts`:
- Around line 73-82: The Ollama tool-result path in native-ollama.ts is
attaching images to native tool messages and reusing the same image accumulator
across iterations. Update the logic around the ollamaMessages push so tool
results stay text-only with only content and tool_name, move any images onto the
adjacent user message instead, and reset toolResultImages for each tool result
to prevent leakage between results.
---
Nitpick comments:
In `@src/api/providers/__tests__/native-ollama.spec.ts`:
- Around line 788-864: The current test only verifies top-level removal of
additionalProperties in NativeOllamaHandler’s tool conversion, so it misses
nested schema cases. Extend the native-ollama.spec.ts coverage around
createMessage and convertToolsToOllama by adding a fixture with
additionalProperties inside a nested properties object, then assert the outgoing
mockChat tools payload strips that nested key as well. This should exercise the
same tool schema path used by apply_diff and confirm the implementation handles
recursive schema sanitization, not just the top-level parameters object.
In `@src/api/providers/native-ollama.ts`:
- Around line 208-225: The tool schema sanitization in native-ollama only
removes additionalProperties at the top level, so nested occurrences still leak
into Ollama payloads. Update the mapping logic in the tool conversion path to
recursively strip additionalProperties from tool.function.parameters, including
nested objects and items, while preserving the rest of the schema shape.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5ab80b3-81ab-49dc-b494-b55d67f5de07
📒 Files selected for processing (3)
src/api/providers/__tests__/native-ollama.spec.tssrc/api/providers/fetchers/ollama.tssrc/api/providers/native-ollama.ts
| // Strip additionalProperties from the parameters schema. | ||
| // This field is not part of Ollama's tool schema definition and can | ||
| // cause issues with some models' tool-calling templates. | ||
| const rawParams = tool.function.parameters as Record<string, unknown> | undefined |
There was a problem hiding this comment.
The delete parameters.additionalProperties only strips the property at the top level of the parameters object. If your tools define nested properties (for example, object properties or items of type object), those nested schemas will still leak additionalProperties: false.
Since Ollama model templates are highly sensitive to these fields, consider recursing through the schema to sanitize it completely. You can introduce a recursive helper like:
function stripAdditionalProperties(schema: unknown): unknown {
if (!schema || typeof schema !== "object") return schema;
if (Array.isArray(schema)) return schema.map(stripAdditionalProperties);
const result: Record<string, unknown> = {};
for (const [key, value] of Object.entries(schema)) {
if (key === "additionalProperties") continue;
result[key] = stripAdditionalProperties(value);
}
return result;
}- getOllamaModels: append per-request .catch() so a single failing /api/show no longer rejects the whole Promise.all and wipes out all healthy models (issue #851) - native-ollama: keep tool results text-only; move images extracted from tool results onto the adjacent user message and reset the per-result image accumulator to prevent leakage between tool results - native-ollama: recursively strip additionalProperties from tool schemas (top-level and nested properties/items) instead of only the top level - tests: update ollama fetcher maxTokens expectations to the inherited default (4096), add regression test for individual model fetch failure, and add coverage for text-only tool results, image relocation, image leakage prevention, and nested additionalProperties stripping
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/__tests__/native-ollama.spec.ts (1)
900-944: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
itemsschemas in the recursive sanitizer test.The helper explicitly recurses arrays/
items, but this test only exercises nested object properties. Add an array-of-objects parameter withitems.additionalPropertiesto guard the common schema shape that previously leaked unsupported fields.Suggested test expansion
properties: { path: { type: "string", description: "File path" }, + files: { + type: "array", + items: { + type: "object", + properties: { + path: { type: "string" }, + }, + additionalProperties: false, + }, + }, options: { type: "object", properties: { dry_run: { type: "boolean" }, @@ // Nested additionalProperties also stripped expect(params.properties.options).not.toHaveProperty("additionalProperties") expect(params.properties.options.properties.dry_run).toEqual({ type: "boolean" }) + expect(params.properties.files.items).not.toHaveProperty("additionalProperties") + expect(params.properties.files.items.properties.path).toEqual({ type: "string" })As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js}files should use package-local unit tests for serialization and request construction logic.🤖 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/api/providers/__tests__/native-ollama.spec.ts` around lines 900 - 944, The recursive sanitizer test in native-ollama.spec.ts only covers nested object properties, so it misses the array/items schema path that the sanitizer also handles. Expand the existing createMessage/mockChat assertion test to include a tool parameter with an array of objects and an items schema containing additionalProperties, then verify the serialized callArgs.tools[0].function.parameters has stripped additionalProperties from both the top level and the nested items/object schemas. Use the existing createMessage, handler, mockChat, and params assertions as the location to extend the coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/api/providers/__tests__/native-ollama.spec.ts`:
- Around line 900-944: The recursive sanitizer test in native-ollama.spec.ts
only covers nested object properties, so it misses the array/items schema path
that the sanitizer also handles. Expand the existing createMessage/mockChat
assertion test to include a tool parameter with an array of objects and an items
schema containing additionalProperties, then verify the serialized
callArgs.tools[0].function.parameters has stripped additionalProperties from
both the top level and the nested items/object schemas. Use the existing
createMessage, handler, mockChat, and params assertions as the location to
extend the coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b591716b-d1b2-4528-afad-bf69d4483915
📒 Files selected for processing (4)
src/api/providers/__tests__/native-ollama.spec.tssrc/api/providers/fetchers/__tests__/ollama.test.tssrc/api/providers/fetchers/ollama.tssrc/api/providers/native-ollama.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
2 participants
Related GitHub Issue
Closes: #847
Also addresses: #851
Description
This PR fixes three issues in the native Ollama provider, plus a follow-up batch addressing review feedback on this PR and the related issue #851:
Premature context condensing —
parseOllamaModelpreviously setmaxTokensto the fullcontextWindow. SincegetModelMaxOutputTokensreserves 20% of the window for output, this caused condensing to trigger far earlier than necessary. The fix inherits the sane default (4096) fromollamaDefaultModelInfoinstead, somaxTokenscorrectly represents max output tokens rather than the full context window.Tool results sent as user messages —
convertToOllamaMessagesalways sent tool results with the"user"role, making it impossible for the model to distinguish tool results from actual user messages. The fix trackstool_useIDs to tool names via aMap, and when a matchingtool_useblock is found, sends the result with Ollama's native"tool"role andtool_namefield.additionalPropertiesin tool schema — The tool parameter schema includedadditionalProperties, which is not part of Ollama's tool schema definition and broke tool-calling templates on some models. The fix stripsadditionalPropertiesfrom the parameters before sending tools to Ollama.Review feedback (#848) and issue #851 follow-ups
Concurrent model fetch single-point-of-failure ([BUG] Ollama model details concurrent fetch is a single-point-of-failure (Promise.all error) #851) —
getOllamaModelsretrieved model details concurrently withPromise.allover individual/api/showPOST requests. A single failing request (corrupt model manifest, timeout, or server overload) rejected the wholePromise.all, wiping out all otherwise healthy models. Each per-model promise now has a.catch()that logs the individual failure and resolves cleanly, soPromise.allcompletes and the remaining healthy models still load.Tool results must be text-only — Tool results were attaching
imagesto nativetool-role messages, which Ollama'stoolrole does not support and can invalidate the request. ThetoolResultImagesaccumulator was also reused across iterations, so one image could leak into later tool results. Tool results are now text-only (content+tool_name); images extracted from tool results are moved onto the adjacentusermessage (the only role that supports images), and the per-result image accumulator is reset for each tool result to prevent leakage.Recursive
additionalPropertiesstripping —delete parameters.additionalPropertiesonly stripped the field at the top level, leaving nestedproperties/itemsschemas leakingadditionalPropertiesinto Ollama payloads. A recursivestripAdditionalPropertieshelper now sanitizes the schema at every nesting level while preserving the rest of the schema shape.Test Procedure
src/api/providers/__tests__/native-ollama.spec.tscovering:"tool"role withtool_namewhen thetool_useID is known"user"role when thetool_useID is unknownadditionalPropertiesbeing stripped from tool parameter schemas (top-level and nested)maxTokensinheriting the default rather than the full context windowsrc/api/providers/fetchers/__tests__/ollama.test.tscovering:maxTokensexpectations aligned with the inherited default (4096)/api/showrequest fails (issue [BUG] Ollama model details concurrent fetch is a single-point-of-failure (Promise.all error) #851 regression test)cd src && npx vitest run api/providers/__tests__/native-ollama.spec.tscd src && npx vitest run api/providers/fetchers/__tests__/ollama.test.tsPre-Submission Checklist
Screenshots / Videos
N/A — no UI changes.
Documentation Updates
Additional Notes
Also resolves the CodeRabbit review comments on this PR (maxTokens test expectations, text-only tool results / image relocation, and recursive
additionalPropertiesstripping).Get in Touch
@navedmerchant
Summary by CodeRabbit