fix(ollama): preserve configured context window#26475
fix(ollama): preserve configured context window#26475jimmielightner wants to merge 1 commit intoopenclaw:mainfrom
Conversation
| return { | ||
| ...implicitModel, | ||
| ...explicitModel, | ||
| input: implicitModel.input, | ||
| input: explicitModel.input ?? implicitModel.input, | ||
| reasoning: "reasoning" in explicitModel ? explicitModel.reasoning : implicitModel.reasoning, | ||
| contextWindow: implicitModel.contextWindow, | ||
| maxTokens: implicitModel.maxTokens, | ||
| contextWindow: explicitModel.contextWindow ?? implicitModel.contextWindow, | ||
| maxTokens: explicitModel.maxTokens ?? implicitModel.maxTokens, | ||
| }; |
There was a problem hiding this comment.
Missing test coverage for new merge behavior
The contextWindow/maxTokens override behavior is the core fix of this PR, but no test validates it. The existing preserves-explicit-reasoning-override.test.ts already sets contextWindow: 1000000 in its explicit model config — it would be straightforward to add an assertion there (e.g., expect(m25?.contextWindow).toBe(1000000)) to ensure the explicit context window isn't overwritten by the catalog default (200000). Same for maxTokens and input.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/models-config.ts
Line: 54-61
Comment:
**Missing test coverage for new merge behavior**
The `contextWindow`/`maxTokens` override behavior is the core fix of this PR, but no test validates it. The existing `preserves-explicit-reasoning-override.test.ts` already sets `contextWindow: 1000000` in its explicit model config — it would be straightforward to add an assertion there (e.g., `expect(m25?.contextWindow).toBe(1000000)`) to ensure the explicit context window isn't overwritten by the catalog default (200000). Same for `maxTokens` and `input`.
How can I resolve this? If you propose a fix, please make it concise.f8d6b16 to
ed0af24
Compare
|
Superseded by #29205, which includes a scoped preservation strategy for higher explicit token limits while keeping stale-low capability refresh behavior intact. Credit: @jimmielightner for raising and implementing the configured context-window preservation need in this PR. |
Summary
contextWindow/maxTokenswhen merging explicit and implicit provider entriesTesting
ollama psnow reportscontext_length: 202752Greptile Summary
This PR fixes
mergeProviderModelsso that user-configuredcontextWindowandmaxTokensvalues are preserved when merging explicit (user) and implicit (built-in catalog) provider model entries. Previously, the implicit catalog values always won for these fields, which meant users couldn't override the default 128k context window for Ollama models even when the model supported larger contexts.contextWindowandmaxTokensnow useexplicitModel.value ?? implicitModel.value, consistent with the existingreasoningoverride patterninputfield also changed from always-implicit to prefer-explicit, allowing users to override input modality capabilities...implicitModelspread added as a base layer before...explicitModel, ensuring new catalog fields propagate automatically while user overrides still take prioritycontextWindow/maxTokenspreservation behaviorConfidence Score: 4/5
reasoningfield override pattern. The??operator correctly handles the fallback behavior. The addition of...implicitModelas a base spread is a sensible improvement. No new tests were added, but the change is low-risk and the existing reasoning override tests still pass. The only minor gap is test coverage for the new contextWindow/maxTokens preservation behavior.Last reviewed commit: f8d6b16
(2/5) Greptile learns from your feedback when you react with thumbs up/down!