hotfix(ollama): Show only Ollama models after provider selection#55290
hotfix(ollama): Show only Ollama models after provider selection#55290BruceMacD merged 5 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis hotfix adds Changes:
Confidence Score: 5/5Safe to merge — minimal, targeted one-line fix with a matching regression test and no behavioral changes to other providers. The change is a single set entry addition. The provider lookup already lowercases keys before comparison, so 'ollama' integrates without any further edits. The previously flagged missing test concern has been fully addressed with a well-structured test that mirrors the existing kilocode pattern. No other code paths are affected. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/agents/model-catalog.ts | One-line fix adding 'ollama' to NON_PI_NATIVE_MODEL_PROVIDERS set; correct and complete given the existing lowercase key comparison logic. |
| src/agents/model-catalog.test.ts | Adds regression test for Ollama provider model catalog inclusion, mirroring the established kilocode test pattern. |
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix-ollama-mode..." | Re-trigger Greptile
| let modelSuppressionPromise: Promise<typeof import("./model-suppression.runtime.js")> | undefined; | ||
|
|
||
| const NON_PI_NATIVE_MODEL_PROVIDERS = new Set(["deepseek", "kilocode"]); | ||
| const NON_PI_NATIVE_MODEL_PROVIDERS = new Set(["deepseek", "kilocode", "ollama"]); |
There was a problem hiding this comment.
Missing test coverage for new Ollama entry
The PR description acknowledges no test was added. The existing model-catalog.test.ts already has an equivalent test at line 248 ("merges configured models for opted-in non-pi-native providers") that covers the same code path for kilocode. Adding a similar test for ollama would lock in the fix and guard against future regressions (e.g., someone renaming the set entry or changing the comparison):
it("merges configured models for opted-in ollama provider", async () => {
mockSingleOpenAiCatalogModel();
const result = await loadModelCatalog({
config: {
models: {
providers: {
ollama: {
baseUrl: "http://127.0.0.1:11434",
api: "ollama",
models: [{ id: "llama3.2", name: "Llama 3.2" }],
},
},
},
} as OpenClawConfig,
});
expect(result).toContainEqual(
expect.objectContaining({ provider: "ollama", id: "llama3.2", name: "Llama 3.2" }),
);
});This mirrors the structure already used for kilocode and would be the "smallest reliable guardrail" mentioned in the PR's own regression test plan.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-catalog.ts
Line: 37
Comment:
**Missing test coverage for new Ollama entry**
The PR description acknowledges no test was added. The existing `model-catalog.test.ts` already has an equivalent test at line 248 ("merges configured models for opted-in non-pi-native providers") that covers the same code path for `kilocode`. Adding a similar test for `ollama` would lock in the fix and guard against future regressions (e.g., someone renaming the set entry or changing the comparison):
```ts
it("merges configured models for opted-in ollama provider", async () => {
mockSingleOpenAiCatalogModel();
const result = await loadModelCatalog({
config: {
models: {
providers: {
ollama: {
baseUrl: "http://127.0.0.1:11434",
api: "ollama",
models: [{ id: "llama3.2", name: "Llama 3.2" }],
},
},
},
} as OpenClawConfig,
});
expect(result).toContainEqual(
expect.objectContaining({ provider: "ollama", id: "llama3.2", name: "Llama 3.2" }),
);
});
```
This mirrors the structure already used for `kilocode` and would be the "smallest reliable guardrail" mentioned in the PR's own regression test plan.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
I've added the test case as suggested and made additional modifications to fix type errors.
Thank you for pointing out the missing test coverage. I've added a dedicated test case for the Ollama provider following the same pattern as the existing kilocode test. I noticed that the initial test case needed additional model properties to match the ModelDefinitionConfig type requirements, so I included:
- reasoning
- input
- cost
- contextWindow
- maxTokens
These properties were necessary to resolve TypeScript compilation errors. The test now properly verifies that Ollama models are merged into the catalog when configured, providing the regression guard you recommended.
The changes are committed in the fix branch and should address the coverage gap you identified.
Signal tests failing unrelated to Ollama hotfixAnalysis
Recommendation
|
b080aa9 to
110adc0
Compare
|
Merged via squash. Thanks @Luckymingxuan! |
) * Fix: Add ollama to NON_PI_NATIVE_MODEL_PROVIDERS to ensure correct model selection * Add test coverage for ollama provider to ensure models are merged correctly * Fix test case for ollama provider by adding required model properties * Changelog: add Ollama model picker fix * Changelog: move Ollama fix entry to section tail --------- Co-authored-by: Bruce MacDonald <brucewmacdonald@gmail.com>
…nclaw#55290) * Fix: Add ollama to NON_PI_NATIVE_MODEL_PROVIDERS to ensure correct model selection * Add test coverage for ollama provider to ensure models are merged correctly * Fix test case for ollama provider by adding required model properties * Changelog: add Ollama model picker fix * Changelog: move Ollama fix entry to section tail --------- Co-authored-by: Bruce MacDonald <brucewmacdonald@gmail.com>
[HOTFIX] Fix Ollama Model Picker to Directly Show Available Models
Summary
"ollama"to theNON_PI_NATIVE_MODEL_PROVIDERSset insrc/agents/model-catalog.ts.Change Type
Scope
Linked Issue/PR
Root Cause / Regression History
NON_PI_NATIVE_MODEL_PROVIDERSset, so configured Ollama models were not added to the model catalog during discovery.Regression Test Plan
src/agents/model-catalog.test.tsUser-visible / Behavior Changes
Diagram
Security Impact
Repro + Verification
Environment
Steps
Expected
Actual (before fix)
Human Verification
Compatibility / Migration
Risks and Mitigations