feat(core): add Ollama as third AI provider [pre-#1137 refactor baseline]#1158
Closed
asmrtfm wants to merge 16 commits intocoleam00:devfrom
Closed
feat(core): add Ollama as third AI provider [pre-#1137 refactor baseline]#1158asmrtfm wants to merge 16 commits intocoleam00:devfrom
asmrtfm wants to merge 16 commits intocoleam00:devfrom
Conversation
Wires Ollama into the full stack as a peer provider alongside Claude and Codex: - `packages/core/src/clients/ollama.ts`: New OllamaClient implementing IAssistantClient — streams responses via Ollama /api/chat NDJSON, yields MessageChunk tokens, surfaces clear error when no model is configured. - `packages/core/src/clients/factory.ts`: Register 'ollama' case; update error message to list all three supported types. - `packages/core/src/clients/index.ts`: Export OllamaClient. - `packages/core/src/config/config-types.ts`: Add OllamaAssistantDefaults (model, baseUrl); extend assistant union to 'claude' | 'codex' | 'ollama' across GlobalConfig, RepoConfig, MergedConfig, and SafeConfig (the last also gains an availableAssistants field). - `packages/core/src/config/config-loader.ts`: Initialize empty ollama defaults, merge global/repo ollama config, include ollama in updateGlobalConfig, add example config comment, accept 'ollama' in applyEnvOverrides. - `packages/core/src/db/conversations.ts`: Replace hardcoded 'claude' fallback with loadConfig().assistant so new conversations inherit the configured default provider. - `packages/core/src/orchestrator/orchestrator-agent.ts`: Forward config.assistants.ollama.model into AssistantRequestOptions for ollama conversations. - `packages/workflows/src/deps.ts`: Extend AssistantClientFactory and WorkflowConfig.assistant to include 'ollama'; add ollama block to WorkflowConfig.assistants. - `packages/workflows/src/model-validation.ts`: isModelCompatible accepts any model string for 'ollama' (pass-through). - `packages/workflows/src/schemas/`: dag-node and workflow schemas updated to include 'ollama' in provider enums. - `packages/workflows/src/loader.ts`, executor.ts, dag-executor.ts: thread 'ollama' through provider handling. - `packages/server/src/routes/api.ts` + config.schemas.ts: expose ollama config via /api/config and accept it in PATCH /api/config/global. - `packages/web/src/routes/SettingsPage.tsx`: Ollama section in Settings UI (model selector populated from /api/tags, baseUrl field). - `packages/web/src/lib/api.ts` + api.generated.d.ts: regenerated OpenAPI types to include ollama fields in SafeConfig. - `.env.example`: document OLLAMA_BASE_URL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ig-loader
- `conversations.test.ts`: mock `config-loader` to return a stable default
assistant ('claude') so the test doesn't depend on the developer's local
.archon/config.yaml. Follows the existing pattern of mocking all external
I/O that the module under test touches. The test "creates new conversation
with default assistant type" validates config.assistant flows through —
'claude' is the documented factory default, not a constraint on real usage.
- `package.json` test script: split `conversations.test.ts` into its own
`bun test` invocation (before the big db/utils/config batch) to prevent
mock.module() pollution. `conversations.test.ts` mocks `config-loader`;
`config-loader.test.ts` spreads `realConfigLoader` — running both in the
same process caused config-loader tests to pick up the mock's stub instead
of the real module.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class Ollama support across core clients, config/types/schemas, server API (model discovery), web UI/settings, workflow/runtime provider resolution, and tests/docs; introduces an Ollama client, config fields, GET /api/ollama/models, and UI model selection. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant OllamaServer
User->>Frontend: Open Settings / Select Ollama
Frontend->>Backend: GET /api/ollama/models
Backend->>OllamaServer: GET /api/tags (timeout 3s)
OllamaServer-->>Backend: models / error
Backend-->>Frontend: { models, available }
Frontend->>Frontend: Show model selector
User->>Frontend: Save config (ollama model/baseUrl)
Frontend->>Backend: PATCH /api/config/assistants (ollama)
Backend->>Backend: Merge & persist config
User->>Frontend: Execute workflow using Ollama
Frontend->>Backend: Execute request
Backend->>Backend: loadConfig -> resolve provider 'ollama'
Backend->>Backend: Instantiate OllamaClient
Backend->>OllamaServer: POST /api/chat (model, messages, stream:true)
OllamaServer-->>Backend: Stream NDJSON
Backend->>Backend: Parse NDJSON -> yield MessageChunks
Backend-->>Frontend: Streamed chunks
Frontend->>User: Display responses
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/config/config-loader.test.ts (1)
219-230:⚠️ Potential issue | 🟠 MajorStabilize the default-assistants assertion against ambient env vars.
Line 227 assumes
assistants.ollamais{}. IfOLLAMA_BASE_URLis present in the runner environment, this default object can become non-empty and make this test nondeterministic.🔧 Proposed fix
const envVars = [ 'DEFAULT_AI_ASSISTANT', + 'OLLAMA_BASE_URL', 'TELEGRAM_STREAMING_MODE', 'DISCORD_STREAMING_MODE', 'SLACK_STREAMING_MODE',As per coding guidelines, "
**/*.test.ts: Keep tests deterministic — no flaky timing or network dependence without guardrails; ensure local validation commands (bun run validate) map directly to CI expectations (Determinism + Reproducibility)`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config-loader.test.ts` around lines 219 - 230, The test "returns defaults when no configs exist" can be nondeterministic if ambient OLLAMA_BASE_URL is set; before calling loadConfig() in this test (and after mocking mockReadConfigFile), clear or override the relevant environment variable(s) (e.g., delete or set process.env.OLLAMA_BASE_URL = undefined) so assistants.ollama stays {} for the assertion, then restore any changed env vars after the test; apply this change inside the same test that uses mockReadConfigFile and loadConfig().packages/core/src/config/config-loader.ts (1)
496-503:⚠️ Potential issue | 🟠 MajorPATCHing another assistant now drops existing Ollama config.
This merge reconstructs
merged.assistantswithout preservingcurrent.assistants?.ollamaunless the incoming patch also containsupdates.assistants.ollama. A PATCH that only changes Claude/Codex will therefore erase a previously saved Ollama block fromconfig.yaml.Suggested fix
if (updates.assistants) { merged.assistants = { claude: { ...current.assistants?.claude, ...updates.assistants.claude }, codex: { ...current.assistants?.codex, ...updates.assistants.codex }, - ...(updates.assistants.ollama !== undefined - ? { ollama: { ...current.assistants?.ollama, ...updates.assistants.ollama } } - : {}), + ...((current.assistants?.ollama !== undefined || updates.assistants.ollama !== undefined) + ? { ollama: { ...current.assistants?.ollama, ...updates.assistants.ollama } } + : {}), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config-loader.ts` around lines 496 - 503, The current merge rebuilds merged.assistants and drops current.assistants?.ollama when updates.assistants lacks an ollama key; change the merge logic in config-loader so you start from the existing current.assistants (or its specific subkeys) and then shallow-merge/override with updates.assistants values rather than reconstructing claude/codex and conditionally adding ollama; specifically modify the block that sets merged.assistants to preserve current.assistants?.ollama when updates.assistants.ollama is undefined (use a spread of current.assistants and then spread updates.assistants or explicitly include current.assistants?.ollama in the merged result) so PATCHes that only update claude/codex do not remove existing ollama config.
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.ts (1)
2613-2630: Loop provider resolution mirrors the main node resolution logic.The duplicate provider resolution for loop nodes (lines 2615-2625) matches the logic in
resolveNodeProviderAndModel. This is intentional since loop nodes manage their own sessions and need independent provider/model resolution.One potential improvement: consider extracting this repeated logic into a shared helper to ensure consistency, but this is optional given the current scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2613 - 2630, The loop-node provider/model resolution duplicated in the isLoopNode branch should be extracted into a shared helper to avoid drift; create or reuse a function like resolveNodeProviderAndModel(node, workflowProvider, workflowModel, config) that returns { provider, model } using the same selection logic currently implemented for regular nodes, then replace the inlined logic in the isLoopNode block to call that helper and assign loopProvider and loopModel from its return value (ensuring the helper handles the special Ollama -> Codex fallback and workflow inheritance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/clients/ollama.ts`:
- Around line 44-46: The Ollama client currently stores baseUrl in the
constructor from process.env.OLLAMA_BASE_URL so saved per-config settings are
ignored; update the client so runtime calls use the persisted settings value
instead of the fixed constructor value: either accept and store a baseUrl
parameter (or Settings object) in the constructor and use that in request
methods, or read the current settings/config inside the request methods that
call /api/chat (referencing the constructor, the baseUrl property, and the
methods that build/send requests around the 72-80 region) so changes in Settings
immediately affect where requests are sent; ensure all request paths use the
dynamic baseUrl rather than the original process.env value.
In `@packages/core/src/config/config-loader.ts`:
- Around line 541-544: The availableAssistants array is hard-coded to include
'ollama' and thus exposes it even when no assistants.ollama config exists;
change construction of availableAssistants to start from the actual configured
assistants (e.g., the parsed config object’s assistants keys) and only push
'codex' if process.env.CODEX_ID_TOKEN && process.env.CODEX_ACCESS_TOKEN are
present; specifically, replace the hard-coded ['claude','ollama'] with code that
reads the config's assistants (e.g., Object.keys(config.assistants || {})
filtered to supported names) to populate availableAssistants and then
conditionally add 'codex' as before.
In `@packages/core/src/db/conversations.test.ts`:
- Around line 14-22: The test currently uses the process-global
mock.module('../config/config-loader', ...) which causes irreversible,
order-dependent test pollution; replace this with a local spyOn import so the
mock is scoped to the test. Import the actual module (config-loader) in the
test, then call spyOn(configLoaderModule, 'loadConfig') to return the desired
value (e.g., { assistant: 'claude' }) for this spec, and restore/clear the spy
after the test; alternatively refactor functions that consume loadConfig to
accept config as a dependency so tests can inject stable values without global
mocking.
In `@packages/core/src/db/conversations.ts`:
- Around line 76-77: The code assigns process.env.DEFAULT_AI_ASSISTANT directly
to assistantType which allows unsupported provider values to be persisted;
update the logic that sets assistantType (where loadConfig() is used and
assistantType is declared) to validate process.env.DEFAULT_AI_ASSISTANT against
the supported assistant union (or the same allowed list used elsewhere), only
accept it if it matches a supported value, otherwise throw a clear error (e.g.,
"unsupported DEFAULT_AI_ASSISTANT: <value>") and fall back to config.assistant
only when the env var is absent or valid; ensure any downstream persistence that
writes ai_assistant_type uses this validated assistantType.
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 765-767: Add a baseUrl field to the request-level options and
thread it through to the Ollama client: update the AssistantRequestOptions type
to include baseUrl?: string, include baseUrl when building the Ollama-specific
options in orchestrator-agent.ts (the object spread that currently forwards
model when conversation.ai_assistant_type === 'ollama'), and modify
OllamaClient.stream() to accept an options parameter that can contain baseUrl
and use that value instead of only reading OLLAMA_BASE_URL from the environment
when making the request; keep the existing env fallback but ensure the
request-level baseUrl overrides it.
In `@packages/server/src/routes/api.ts`:
- Around line 2645-2655: The try/catch around the Ollama discovery fetch
currently swallows all errors and returns c.json({models:[], available:false})
silently; update the catch and non-OK branches to emit a structured Pino error
event (e.g., "ollama.discovery_failed") with contextual fields (baseUrl,
status/err message, timeout) before returning the fallback, using the
repository's logger (e.g., logger or processLogger), and ensure you also emit a
matching "ollama.discovery_started" when the request begins and
"ollama.discovery_completed" on success around the fetch/response handling
(referencing the existing fetch call, baseUrl, AbortSignal.timeout, response,
data, and c.json).
- Around line 2644-2646: The handler for /api/ollama/models uses
process.env.OLLAMA_BASE_URL (baseUrl) and ignores the persisted setting; update
the logic that sets baseUrl so it first reads the saved setting
assistants.ollama.baseUrl from the application settings (the same settings
service or store used by the Settings page) and falls back to
process.env.OLLAMA_BASE_URL and then to 'http://localhost:11434'; ensure the
subsequent fetch that calls `${baseUrl}/api/tags` uses this resolved value so
the model discovery queries the user-saved Ollama instance.
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 535-542: The payload omits the entire ollama block when
ollamaModel and ollamaBaseUrl are empty, so merge-based PATCH
/api/config/assistants keeps prior assistants.ollama values; always include an
ollama key and explicitly set its nested fields to null when empty. Update the
settings payload construction in SettingsPage.tsx to always include ollama: {
model: ollamaModel || null, baseUrl: ollamaBaseUrl || null } (use null rather
than undefined so the merge PATCH will clear existing assistants.ollama.model
and assistants.ollama.baseUrl).
In `@packages/workflows/src/model-validation.ts`:
- Line 17: The current short-circuit in isModelCompatible that returns true for
provider === 'ollama' incorrectly accepts the sentinel model value "inherit";
update the check in isModelCompatible to reject or not short-circuit when model
=== 'inherit' (i.e., only auto-accept Ollama when provider === 'ollama' AND
model !== 'inherit'), so that model="inherit" fails validation here (or
alternatively resolve the sentinel before calling isModelCompatible where
provider/model are used).
---
Outside diff comments:
In `@packages/core/src/config/config-loader.test.ts`:
- Around line 219-230: The test "returns defaults when no configs exist" can be
nondeterministic if ambient OLLAMA_BASE_URL is set; before calling loadConfig()
in this test (and after mocking mockReadConfigFile), clear or override the
relevant environment variable(s) (e.g., delete or set
process.env.OLLAMA_BASE_URL = undefined) so assistants.ollama stays {} for the
assertion, then restore any changed env vars after the test; apply this change
inside the same test that uses mockReadConfigFile and loadConfig().
In `@packages/core/src/config/config-loader.ts`:
- Around line 496-503: The current merge rebuilds merged.assistants and drops
current.assistants?.ollama when updates.assistants lacks an ollama key; change
the merge logic in config-loader so you start from the existing
current.assistants (or its specific subkeys) and then shallow-merge/override
with updates.assistants values rather than reconstructing claude/codex and
conditionally adding ollama; specifically modify the block that sets
merged.assistants to preserve current.assistants?.ollama when
updates.assistants.ollama is undefined (use a spread of current.assistants and
then spread updates.assistants or explicitly include current.assistants?.ollama
in the merged result) so PATCHes that only update claude/codex do not remove
existing ollama config.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2613-2630: The loop-node provider/model resolution duplicated in
the isLoopNode branch should be extracted into a shared helper to avoid drift;
create or reuse a function like resolveNodeProviderAndModel(node,
workflowProvider, workflowModel, config) that returns { provider, model } using
the same selection logic currently implemented for regular nodes, then replace
the inlined logic in the isLoopNode block to call that helper and assign
loopProvider and loopModel from its return value (ensuring the helper handles
the special Ollama -> Codex fallback and workflow inheritance).
🪄 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
Run ID: 6e88f57d-7739-402f-9c40-f42f19dc8f4b
📒 Files selected for processing (27)
.env.examplepackages/core/package.jsonpackages/core/src/clients/factory.tspackages/core/src/clients/index.tspackages/core/src/clients/ollama.tspackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/core/src/db/conversations.test.tspackages/core/src/db/conversations.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/config.schemas.tspackages/web/src/components/workflows/BuilderToolbar.tsxpackages/web/src/components/workflows/NodeInspector.tsxpackages/web/src/components/workflows/WorkflowBuilder.tsxpackages/web/src/lib/api.generated.d.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/executor.tspackages/workflows/src/loader.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/workflow.ts
Comment on lines
+541
to
+544
| const availableAssistants: ('claude' | 'codex' | 'ollama')[] = ['claude', 'ollama']; | ||
| if (process.env.CODEX_ID_TOKEN && process.env.CODEX_ACCESS_TOKEN) { | ||
| availableAssistants.push('codex'); | ||
| } |
There was a problem hiding this comment.
availableAssistants now exposes Ollama on every install.
SettingsPage.tsx renders its provider dropdown from this array, so hard-coding ['claude', 'ollama'] makes Ollama visible even when there is no assistants.ollama block configured. That breaks the additive/"only when configured" behavior described for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/config/config-loader.ts` around lines 541 - 544, The
availableAssistants array is hard-coded to include 'ollama' and thus exposes it
even when no assistants.ollama config exists; change construction of
availableAssistants to start from the actual configured assistants (e.g., the
parsed config object’s assistants keys) and only push 'codex' if
process.env.CODEX_ID_TOKEN && process.env.CODEX_ACCESS_TOKEN are present;
specifically, replace the hard-coded ['claude','ollama'] with code that reads
the config's assistants (e.g., Object.keys(config.assistants || {}) filtered to
supported names) to populate availableAssistants and then conditionally add
'codex' as before.
Comment on lines
+535
to
+542
| ...(ollamaModel || ollamaBaseUrl | ||
| ? { | ||
| ollama: { | ||
| ...(ollamaModel ? { model: ollamaModel } : {}), | ||
| ...(ollamaBaseUrl ? { baseUrl: ollamaBaseUrl } : {}), | ||
| }, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
This Settings payload can't clear saved Ollama fields.
When the user deletes the model or base URL, the spread drops the ollama block entirely once the strings become empty. Because PATCH /api/config/assistants is merge-based, the old assistants.ollama.* values survive, so the Settings page has no way to reset them back to defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/routes/SettingsPage.tsx` around lines 535 - 542, The payload
omits the entire ollama block when ollamaModel and ollamaBaseUrl are empty, so
merge-based PATCH /api/config/assistants keeps prior assistants.ollama values;
always include an ollama key and explicitly set its nested fields to null when
empty. Update the settings payload construction in SettingsPage.tsx to always
include ollama: { model: ollamaModel || null, baseUrl: ollamaBaseUrl || null }
(use null rather than undefined so the merge PATCH will clear existing
assistants.ollama.model and assistants.ollama.baseUrl).
Author
Gemma4 tool calling
|
Documents all new and modified exported symbols introduced by the Ollama provider feature to satisfy the PR's 60% docstring coverage requirement: OllamaAssistantDefaults, AssistantClientFactory, isClaudeModel, isModelCompatible (model-validation.ts was pre-staged). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the saved default assistant is 'ollama' but the model-list query comes back unavailable (server offline or unreachable), reset the displayed assistant selection to 'claude' so users aren't stuck on a non-functional provider. The saved setting is unchanged until Save is clicked. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web/src/routes/SettingsPage.tsx (1)
542-549:⚠️ Potential issue | 🟠 MajorOllama payload still can’t clear previously saved model/baseUrl.
At Lines 542-549, when both fields are emptied, the
ollamablock is omitted, so merge-basedPATCH /api/config/assistantspreserves oldassistants.ollama.*values.💡 Suggested fix
- ...(ollamaModel || ollamaBaseUrl - ? { - ollama: { - ...(ollamaModel ? { model: ollamaModel } : {}), - ...(ollamaBaseUrl ? { baseUrl: ollamaBaseUrl } : {}), - }, - } - : {}), + ollama: { + model: ollamaModel || null, + baseUrl: ollamaBaseUrl || null, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/routes/SettingsPage.tsx` around lines 542 - 549, The current payload-builder omits the entire ollama block when both ollamaModel and ollamaBaseUrl are empty, so the PATCH merge preserves previous values; modify the payload construction around the ollama block so it always includes an ollama object (even when blank) and sets model and baseUrl explicitly to null when empty (e.g. use ollamaModel ? ollamaModel : null and ollamaBaseUrl ? ollamaBaseUrl : null) so the server clears previously saved assistants.ollama.* values instead of leaving them unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 494-498: The useEffect currently forces setAssistant('claude')
when Ollama is unavailable which can pick an invalid assistant; update the
effect to choose a valid fallback from config.availableAssistants (or the first
entry) only if that value exists, otherwise do not call setAssistant or preserve
the current selection; locate the effect that references ollamaData,
ollamaModels and setAssistant and replace the hard-coded 'claude' fallback with
logic that checks config.availableAssistants for a valid assistant before
calling setAssistant.
---
Duplicate comments:
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 542-549: The current payload-builder omits the entire ollama block
when both ollamaModel and ollamaBaseUrl are empty, so the PATCH merge preserves
previous values; modify the payload construction around the ollama block so it
always includes an ollama object (even when blank) and sets model and baseUrl
explicitly to null when empty (e.g. use ollamaModel ? ollamaModel : null and
ollamaBaseUrl ? ollamaBaseUrl : null) so the server clears previously saved
assistants.ollama.* values instead of leaving them unchanged.
🪄 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
Run ID: ea24037e-aa78-46cf-9906-0f3a14d38d7f
📒 Files selected for processing (1)
packages/web/src/routes/SettingsPage.tsx
- ai-assistants.md: new Ollama section (prerequisites, no-auth, config options, set-as-default) and updated assistant selection notes - getting-started/configuration.md: add ollama block to config example - reference/configuration.md: add ollama to defaultAssistant, global config YAML, DEFAULT_AI_ASSISTANT env var, and new OLLAMA_BASE_URL env var section - reference/api.md: document GET /api/ollama/models endpoint Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md`:
- Line 177: The docs claim that the Web UI falls back from Ollama to Claude when
Ollama is unreachable but there is no implementation for that behavior in the
SettingsPage component; either implement the runtime fallback in the
SettingsPage logic (e.g., in the SettingsPage component or its
fetch/initialization handlers that determine the active provider) so it detects
Ollama connectivity and switches the saved default to Claude, or update
packages/docs-web/src/content/docs/getting-started/ai-assistants.md to remove or
clearly qualify the Ollama→Claude fallback statement; reference the SettingsPage
component and its provider selection/initialization code when making the change.
In `@packages/docs-web/src/content/docs/reference/configuration.md`:
- Around line 69-71: The docs incorrectly state that ollama.baseUrl config
overrides OLLAMA_BASE_URL; instead update the documentation text in
configuration.md to reflect the actual behavior used by the code (the client
uses the OLLAMA_BASE_URL environment variable directly and the config's baseUrl
is not passed into OllamaClient during client construction). Edit the
example/comment to state that baseUrl in config is not applied (or that
OLLAMA_BASE_URL takes precedence) and reference the OllamaClient/client
construction logic so readers know which source is authoritative.
🪄 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
Run ID: 006d4cab-e7eb-4a79-9e38-8dcd817a7796
📒 Files selected for processing (4)
packages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/getting-started/configuration.mdpackages/docs-web/src/content/docs/reference/api.mdpackages/docs-web/src/content/docs/reference/configuration.md
✅ Files skipped from review due to trivial changes (1)
- packages/docs-web/src/content/docs/getting-started/configuration.md
| - `DEFAULT_AI_ASSISTANT` (optional) is used only for new conversations without codebase context | ||
| - Workflows can override the assistant on a per-node basis with `provider` and `model` fields | ||
| - Configuration priority: workflow-level options > config file defaults > SDK defaults | ||
| - In the Web UI Settings, if the saved default is `ollama` but Ollama is unreachable when the page loads, the UI falls back to Claude until Ollama becomes available again |
There was a problem hiding this comment.
Remove or qualify the undocumented Ollama→Claude UI fallback claim.
Line 177 states a runtime UI fallback behavior that is not evidenced by the provided packages/web/src/routes/SettingsPage.tsx context. Please either implement this behavior or reword this line to avoid promising it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md` at line
177, The docs claim that the Web UI falls back from Ollama to Claude when Ollama
is unreachable but there is no implementation for that behavior in the
SettingsPage component; either implement the runtime fallback in the
SettingsPage logic (e.g., in the SettingsPage component or its
fetch/initialization handlers that determine the active provider) so it detects
Ollama connectivity and switches the saved default to Claude, or update
packages/docs-web/src/content/docs/getting-started/ai-assistants.md to remove or
clearly qualify the Ollama→Claude fallback statement; reference the SettingsPage
component and its provider selection/initialization code when making the change.
Author
There was a problem hiding this comment.
The fallback behavior is implemented — SettingsPage.tsx lines 493–498 contain the useEffect that calls setAssistant('claude') when ollamaData.available === false and no models are returned. This review appears to have been generated against an earlier snapshot of the diff before that code was visible. No change needed.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '485,510p' packages/web/src/routes/SettingsPage.tsx
Length of output: 981
@asmrtfm, you're correct — the implementation is clearly present. The useEffect at lines 494–498 checks ollamaData && !ollamaData.available && ollamaModels.length === 0 and calls setAssistant('claude'), which is exactly what the documentation describes. My review comment was mistaken, and no change is needed. Apologies for the noise.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/clients/ollama.ts (1)
42-44:⚠️ Potential issue | 🟠 MajorUse a runtime-resolved Ollama base URL instead of a constructor-cached value.
Line 42 caches
baseUrlonce, so later Settings/config changes won’t affect requests built at Line 70. This can route traffic to a stale endpoint.💡 Suggested change
export class OllamaClient implements IAssistantClient { - private readonly baseUrl: string; + private readonly resolveBaseUrl: () => string; - constructor() { - this.baseUrl = process.env.OLLAMA_BASE_URL ?? DEFAULT_BASE_URL; + constructor( + resolveBaseUrl: () => string = () => process.env.OLLAMA_BASE_URL ?? DEFAULT_BASE_URL + ) { + this.resolveBaseUrl = resolveBaseUrl; } @@ - const url = `${this.baseUrl}${CHAT_PATH}`; + const baseUrl = this.resolveBaseUrl(); + const url = `${baseUrl}${CHAT_PATH}`;Also applies to: 70-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/clients/ollama.ts` around lines 42 - 44, The constructor currently caches baseUrl (this.baseUrl = process.env.OLLAMA_BASE_URL ?? DEFAULT_BASE_URL) which can become stale; change the implementation to resolve the base URL at request time instead (e.g., add a getBaseUrl() method or inline the expression) and update all usages that reference this.baseUrl (such as the request-building code that composes the URL around Line 70) to call the runtime resolver instead so settings/env changes take effect immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/clients/ollama.ts`:
- Around line 113-116: The streaming parser currently keeps a partial last line
in the variable buffer (using lines.pop()) but never processes it when the
stream finishes; update the code in the SSE/stream handler (the block that uses
buffer, lines, and the done branch around Line 143) to, upon done, check if
buffer.trim() is non-empty and attempt to parse it as NDJSON (same parse logic
used for completed lines), emitting the final delta/token metadata before
resolving; ensure you reuse the existing parse/emit code paths so the leftover
partial line is not lost and errors are handled consistently.
---
Duplicate comments:
In `@packages/core/src/clients/ollama.ts`:
- Around line 42-44: The constructor currently caches baseUrl (this.baseUrl =
process.env.OLLAMA_BASE_URL ?? DEFAULT_BASE_URL) which can become stale; change
the implementation to resolve the base URL at request time instead (e.g., add a
getBaseUrl() method or inline the expression) and update all usages that
reference this.baseUrl (such as the request-building code that composes the URL
around Line 70) to call the runtime resolver instead so settings/env changes
take effect immediately.
🪄 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
Run ID: 79197af9-5e15-40a4-be6c-2504266f669a
📒 Files selected for processing (1)
packages/core/src/clients/ollama.ts
Comment on lines
+113
to
+116
| const lines = buffer.split('\n'); | ||
| // Keep incomplete last line in buffer | ||
| buffer = lines.pop() ?? ''; | ||
|
|
There was a problem hiding this comment.
Flush the trailing NDJSON buffer after stream completion.
Line 115 preserves a partial line, but after done (Line 143) that leftover buffer is never parsed. If the final chunk has no trailing newline, you can lose the last delta and/or token metadata.
💡 Suggested change
try {
while (true) {
const { done, value } = await reader.read();
if (done) break;
@@
}
}
+
+ const trailing = buffer.trim();
+ if (trailing) {
+ try {
+ const chunk = JSON.parse(trailing) as OllamaChatChunk;
+ if (!chunk.done) {
+ const content = chunk.message?.content ?? '';
+ if (content) {
+ yield { type: 'assistant', content };
+ }
+ } else {
+ inputTokens = chunk.prompt_eval_count ?? 0;
+ outputTokens = chunk.eval_count ?? 0;
+ }
+ } catch {
+ getLog().warn({ line: trailing }, 'ollama.unparseable_chunk');
+ }
+ }
} finally {
reader.releaseLock();
}Also applies to: 143-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/clients/ollama.ts` around lines 113 - 116, The streaming
parser currently keeps a partial last line in the variable buffer (using
lines.pop()) but never processes it when the stream finishes; update the code in
the SSE/stream handler (the block that uses buffer, lines, and the done branch
around Line 143) to, upon done, check if buffer.trim() is non-empty and attempt
to parse it as NDJSON (same parse logic used for completed lines), emitting the
final delta/token metadata before resolving; ensure you reuse the existing
parse/emit code paths so the leftover partial line is not lost and errors are
handled consistently.
Author
There was a problem hiding this comment.
Verified against the live Ollama API: every streamed response terminates the final chunk (the done: true object) with a trailing newline (0a). Confirmed by inspecting raw bytes from /api/chat with a real model. The buffer will always be empty when the stream closes, so no data is lost. No change needed.
There was a problem hiding this comment.
@asmrtfm, thanks for the empirical verification — that's a solid confirmation. Ollama's streaming responses are delivered as JSON objects, with multiple objects in streaming mode sent separated by newlines (NDJSON), so a trailing \n after the final done: true chunk is consistent with the NDJSON protocol. Your raw-byte inspection confirms Ollama honours this reliably in practice.
One minor caveat worth keeping in mind: NDJSON streaming usually flushes immediately, but if the connection ever passes through a buffering reverse proxy (e.g. Nginx without proxy_buffering off), the final newline could be withheld until the TCP connection closes — at which point the ReadableStream would signal done with a non-empty buffer. That's an edge case rather than a bug in the current implementation, so no change is required for the direct-to-Ollama path you've verified.
Fair enough — no change needed here.
✏️ Learnings added
Learnt from: asmrtfm
Repo: coleam00/Archon PR: 1158
File: packages/core/src/clients/ollama.ts:118-121
Timestamp: 2026-04-13T13:19:55.526Z
Learning: In `packages/core/src/clients/ollama.ts` (OllamaClient), the Ollama `/api/chat` streaming API consistently terminates every NDJSON chunk, including the final `done: true` object, with a trailing newline (`\n`, byte `0a`). This was empirically verified by the repo maintainer (asmrtfm) via raw byte inspection, so the trailing buffer after `reader.read()` returns `done: true` is reliably empty and no explicit post-stream buffer flush is required for direct connections to Ollama.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
OllamaClient was only reading OLLAMA_BASE_URL at construction time, so the baseUrl saved via Settings UI (config.assistants.ollama.baseUrl) had no runtime effect. Thread it through AssistantRequestOptions and prefer options.baseUrl over the env-var default in sendQuery. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Accepting arbitrary strings as ai_assistant_type could silently create conversations with unsupported providers. Narrow the type to the known union and throw a clear error on unrecognised values, matching the validation pattern already used in config-loader.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The model discovery endpoint was always probing OLLAMA_BASE_URL / localhost, ignoring the baseUrl saved via the Settings UI. Now reads config.assistants.ollama.baseUrl first, falling back to the env var and then the compiled-in default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Silent catch made Ollama connectivity issues impossible to debug. Add ollama.discovery_started/_completed/_failed log events with baseUrl and status context, following project Pino conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tings The ollama block was conditionally omitted when both fields were empty, so the merge-based PATCH had no way to remove previously saved values. Always include the ollama block in the payload and switch updateGlobalConfig to replacement semantics for ollama so the stored state exactly reflects what the user submitted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
'inherit' is a Claude-only alias meaning "use whatever the SDK is configured with". Passing it to Ollama sends the literal string to /api/chat and fails at runtime. Reject it at workflow validation time with a clear error instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolves all 7 merge conflicts from upstream dev renaming clients/ → providers/ and IAssistantClient/AssistantRequestOptions → IAgentProvider/AgentRequestOptions: - Move OllamaProvider to providers/ollama.ts with updated interface names - Register OllamaProvider in providers/factory.ts and providers/index.ts - Drop deleted clients/factory.ts and clients/index.ts - Add ollama to config-types.ts (GlobalConfig, RepoConfig, MergedConfig, SafeConfig) - Extend AgentProviderFactory in deps.ts to accept 'ollama' - Update buildLoopNodeOptions in dag-executor.ts to handle 'ollama' provider Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Shows a pill in the nav bar on every page with the active provider and model (e.g. "ollama · gemma4:latest", "claude · sonnet"). Clicking it navigates to Settings. Falls back to provider name when no model is configured. The version label shifts right only when the indicator is absent (config loading or unconfigured). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…loader mock When resolving the merge conflict in package.json, HEAD's test script was kept verbatim, which accidentally reverted the batch split introduced in 5b4e1cc. conversations.test.ts uses mock.module() for config-loader; running it in the same bun test invocation as other files that also mock.module() config-loader with different values causes order-dependent failures (Bun's mock.module is process-global and irreversible). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
asmrtfm
commented
Apr 13, 2026
Author
asmrtfm
left a comment
asmrtfm
left a comment
There was a problem hiding this comment.
I hadn't noticed, when I first pushed up the branch, that Claude was leaving comments which said that ollama only provides chat bots that can not perform tool calling or interact with files. I informed him that he wasn't the only game in town anymore and he seemed fine after that, but I couldn't help but get a sense that some of Code Rabbit's critiques might have been informed by a sort of shared bias - whether due to it having read Claude's original comments or from similarly outdated knowledge on the topic. I had assumed I would need to follow google's OpenAI compatibility documentation and create something like a wrapper for gemma but as soon as I added a project directory to thew web ui, it was able to maneuver agentically (created and executed the bashscript as seen in my comments on this PR).
I apologize if any of this comes off as cringe.
asmrtfm
changed the title
|
The right way to do this is to implement That way you cover more than just Ollama. Some ppl, like my self run almost exclusively on vllm or llama.cpp, not ollama. For Archon to grow I think it is crucial to support both frontier and local models and the common denominator is those endpoints. Furthermore there are great libraries for those endpoints as well. What you loose is ease of use and a higher threshold in onboarding but that can be corrected at a later time via configuration UI and documentation. |
Author
|
Yeah, I nearly instructed the bot to replace the use of "Claude" with "Anthropic" as a provider but seeing as I'm new around here, I thought it best to avoid anything resembling scope kreep. Anyway, by the time I got the PR to pass Code Rabbit, providers had been completely refactored, lol. Y'all move fast. I'm going to back off until things settle down. Cheers |
Collaborator
|
Related to #1096 — overlapping area or partial fix. |
Collaborator
|
will close this. please follow the community provider pattern and resubmit if this is relevant. note: we will only accept ai coding agent sdks at this time Ollama can be used both in Claude and PI that is already supported |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
'claude' | 'codex' | 'ollama') threaded through config, conversations, orchestrator, workflow engine, and the Settings UI. The Settings page now shows only the providers you have configured inconfig.yaml— if you haven't added anassistants.ollamablock, you won't see it. Codex and Claude behave exactly as before.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
clients/factoryclients/ollamadb/conversationsconfig/config-loaderorchestrator-agentclients/ollama(via factory)web/SettingsPageGET /api/tags(Ollama)web/SettingsPagePATCH /api/config/globalserver/apiSafeConfig.availableAssistantsLabel Snapshot
risk: lowsize: Mcore,server,web,workflows,configcore:clients,core:config,core:orchestrator,workflows:deps,web:settingsChange Metadata
featuremultiLinked Issue
Validation Evidence
All four gates pass: type-check, lint, format, test.
Security Impact
OllamaClientcallshttp://localhost:11434/api/chat(configurable viaOLLAMA_BASE_URL). This is user-controlled localhost by default.Mitigation: The base URL is user-configured; defaults to localhost. No credentials or tokens involved.
Compatibility / Migration
assistantdefaults to'claude'when not set; existing deployments are unaffectedOLLAMA_BASE_URLenv var; new optionalassistants.ollamablock inconfig.yaml; documented in.env.exampleHuman Verification
assistants.ollamais present in config/api/tags/api/config/globalassistant: ollamain the database when Ollama is the configured defaultollama serve+ a pulled model in CI)Side Effects / Blast Radius
getOrCreateConversationnow callsloadConfig()on each new conversation creation (previously hardcoded'claude'). Config is cached after first load so repeated calls are cheap.loadConfig()throws, new conversation creation fails. Previously this path was a no-op string literal.loadConfig()has its own error handling and falls back to defaults on ENOENT/YAML errors.Rollback Plan
06390836+5b4e1cc5assistant: ollamais explicitly configuredUnknown assistant type: ollamaerror in logs if partially revertedRisks and Mitigations
loadConfig()added to hot path ofgetOrCreateConversationollama.tsheader. No silent degradation — behavior is consistent with how the client is used.Summary by CodeRabbit
New Features
Documentation
Tests