fix(ollama): resolve per-provider baseUrl in createStreamFn#61776
Conversation
Greptile SummaryFixes a real bug where Confidence Score: 5/5Safe to merge — the fix is surgical, correctly scoped, and both code paths are tested. The logic change is correct and well-tested. The only remaining finding is a P2 test-style suggestion about importOriginal on a broad module; it does not affect correctness or production behaviour. No files require special attention beyond the optional mock factory improvement in extensions/ollama/index.test.ts. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/ollama/index.test.ts
Line: 33-39
Comment:
**`importOriginal` on a broad module**
Per the repo's test-performance guardrail, `importOriginal()` should be reserved for narrow modules where partial-real behaviour is genuinely required. `stream.ts` is ~767 lines with many exported functions; loading the full module graph per mock resolution adds unnecessary overhead. Consider an explicit mock factory that stubs only what's exercised in this file:
```ts
vi.mock("./src/stream.js", () => ({
OLLAMA_NATIVE_BASE_URL: "http://127.0.0.1:11434",
createConfiguredOllamaStreamFn: createConfiguredOllamaStreamFnMock,
createConfiguredOllamaCompatStreamWrapper: vi.fn(),
resolveConfiguredOllamaProviderConfig: vi.fn((p) =>
(p.config?.models?.providers as Record<string, unknown>)?.[p.providerId ?? ""],
),
}));
```
Not a blocker — defer if the test file is not a hotspot.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(ollama): resolve per-provider baseUr..." | Re-trigger Greptile |
| vi.mock("./src/stream.js", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("./src/stream.js")>(); | ||
| return { | ||
| ...actual, | ||
| createConfiguredOllamaStreamFn: createConfiguredOllamaStreamFnMock, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
importOriginal on a broad module
Per the repo's test-performance guardrail, importOriginal() should be reserved for narrow modules where partial-real behaviour is genuinely required. stream.ts is ~767 lines with many exported functions; loading the full module graph per mock resolution adds unnecessary overhead. Consider an explicit mock factory that stubs only what's exercised in this file:
vi.mock("./src/stream.js", () => ({
OLLAMA_NATIVE_BASE_URL: "http://127.0.0.1:11434",
createConfiguredOllamaStreamFn: createConfiguredOllamaStreamFnMock,
createConfiguredOllamaCompatStreamWrapper: vi.fn(),
resolveConfiguredOllamaProviderConfig: vi.fn((p) =>
(p.config?.models?.providers as Record<string, unknown>)?.[p.providerId ?? ""],
),
}));Not a blocker — defer if the test file is not a hotspot.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/ollama/index.test.ts
Line: 33-39
Comment:
**`importOriginal` on a broad module**
Per the repo's test-performance guardrail, `importOriginal()` should be reserved for narrow modules where partial-real behaviour is genuinely required. `stream.ts` is ~767 lines with many exported functions; loading the full module graph per mock resolution adds unnecessary overhead. Consider an explicit mock factory that stubs only what's exercised in this file:
```ts
vi.mock("./src/stream.js", () => ({
OLLAMA_NATIVE_BASE_URL: "http://127.0.0.1:11434",
createConfiguredOllamaStreamFn: createConfiguredOllamaStreamFnMock,
createConfiguredOllamaCompatStreamWrapper: vi.fn(),
resolveConfiguredOllamaProviderConfig: vi.fn((p) =>
(p.config?.models?.providers as Record<string, unknown>)?.[p.providerId ?? ""],
),
}));
```
Not a blocker — defer if the test file is not a hotspot.
How can I resolve this? If you propose a fix, please make it concise.The createStreamFn callback hardcoded config.models.providers.ollama.baseUrl, ignoring the actual provider ID from the context. When multiple Ollama providers are configured on different ports (e.g. ollama on 11434, ollama2 on 11435), all requests routed to the first provider's port. Export resolveConfiguredOllamaProviderConfig from stream.ts and use it with the ctx.provider parameter to dynamically look up the correct baseUrl per provider. Closes openclaw#61678
ce71b59 to
baa6cfd
Compare
Summary
ollamaandollama2),createStreamFnhardcodedconfig.models.providers.ollama.baseUrl— ignoring the active provider's actualbaseUrl.createStreamFnto use theproviderparameter fromProviderCreateStreamFnContextand resolve the correct config viaresolveConfiguredOllamaProviderConfig.resolveConfiguredOllamaProviderConfigfromextensions/ollama/src/stream.ts(was already implemented but unexported).Test plan
createStreamFn resolves baseUrl from the active provider id when multiple Ollama providers are configuredCloses #61678