feat(providers): adapt Ollama support to @archon/providers package (#1137 rebase)#1201
Closed
asmrtfm wants to merge 1 commit intocoleam00:devfrom
Closed
feat(providers): adapt Ollama support to @archon/providers package (#1137 rebase)#1201asmrtfm wants to merge 1 commit intocoleam00:devfrom
asmrtfm wants to merge 1 commit intocoleam00:devfrom
Conversation
- Add OllamaProvider in @archon/providers (NDJSON streaming, getCapabilities) - Register ollama in provider factory alongside claude and codex - Extend config types, config-loader, SafeConfig with availableAssistants - Add GET /api/ollama/models discovery endpoint (proxies Ollama /api/tags) - Update PATCH /api/config/assistants to accept ollama settings - Restructure SettingsPage: Default provider+model section + three collapsible provider sections (Claude/Codex/Ollama) with checkboxes - Restore model indicator pill in TopNav - Add OLLAMA_BASE_URL to .env.example - Fix conversations.test.ts batch isolation and add config-loader mock - Update config-loader.test.ts default assistants expectation - Add scripts/dev-restart.sh Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds comprehensive support for Ollama as a new AI assistant provider throughout the system, including configuration defaults, a new Ollama provider implementation with streaming chat capability, API endpoints for model discovery, frontend configuration UI, and workflow/orchestration wiring. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client
participant OllamaProvider
participant OllamaAPI as Ollama API<br/>(baseUrl/api/chat)
User->>Client: Send message with assistant='ollama'
activate Client
Client->>OllamaProvider: sendQuery(prompt, options)
activate OllamaProvider
OllamaProvider->>OllamaProvider: Resolve model from config<br/>or env OLLAMA_BASE_URL
OllamaProvider->>OllamaAPI: POST /api/chat<br/>{model, messages, stream: true}
activate OllamaAPI
OllamaAPI-->>OllamaProvider: Stream newline-delimited JSON
deactivate OllamaAPI
loop For each chunk
OllamaProvider->>OllamaProvider: Parse OllamaChatChunk<br/>Buffer partial lines
alt done: false
OllamaProvider-->>Client: Yield MessageChunk<br/>(content streaming)
else done: true
OllamaProvider->>OllamaProvider: Extract token counts<br/>(prompt_eval_count, eval_count)
OllamaProvider-->>Client: Yield result MessageChunk<br/>(with token totals)
end
end
deactivate OllamaProvider
Client-->>User: Display streamed response
deactivate Client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/routes/SettingsPage.tsx (1)
463-476:⚠️ Potential issue | 🟠 MajorSaving unrelated changes will also write an Ollama config block.
handleSave()always sendsollama, and the server treats the presence ofbody.ollamaas an update. Saving a Claude/Codex-only change will therefore materializeassistants.ollama.baseUrl: http://localhost:11434even for users who never configured Ollama.Suggested fix
+ const ollamaChanged = + ollamaModel !== (config.assistants.ollama?.model ?? '') || + ollamaBaseUrl !== (config.assistants.ollama?.baseUrl ?? 'http://localhost:11434'); + mutation.mutate({ assistant, claude: { model: claudeModel }, // Omit codex key when no model is set — server treats absence as "no codex changes" ...(codexModel ? { codex: { model: codexModel, modelReasoningEffort: reasoning, webSearchMode: webSearch }, } : {}), - ollama: { - ...(ollamaModel ? { model: ollamaModel } : {}), - ...(ollamaBaseUrl ? { baseUrl: ollamaBaseUrl } : {}), - }, + ...(ollamaChanged + ? { + ollama: { + ...(ollamaModel ? { model: ollamaModel } : {}), + ...(ollamaBaseUrl ? { baseUrl: ollamaBaseUrl } : {}), + }, + } + : {}), });🤖 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 463 - 476, The payload always includes an ollama key which causes accidental updates; modify handleSave so that ollama is only added to the object passed to mutation.mutate when there is something to update (i.e., when ollamaModel or ollamaBaseUrl is truthy). Build a small ollamaPayload from ollamaModel and ollamaBaseUrl, and conditionally spread it into the mutate call like the codex block (e.g., ...(ollamaPayload ? { ollama: ollamaPayload } : {})), ensuring no empty ollama object is sent when neither setting is provided.
🧹 Nitpick comments (1)
packages/providers/src/factory.ts (1)
42-44: Align the new Ollama log event with the repo’s Pino naming convention.
provider_selecteddoes not match the required{domain}.{action}_{state}format, so this new branch is extending the existing observability inconsistency. A name likeprovider.select_completedwould fit the convention.As per coding guidelines: "Log events with Pino using the format
{domain}.{action}_{state}(e.g.,session.create_started,workflow.step_completed); always pair_startedwith_completedor_failed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/factory.ts` around lines 42 - 44, The log event string for the Ollama branch uses an inconsistent name; update the getLog().debug call in the case 'ollama' to follow the repository Pino naming convention (domain.action_state) — for example change the event key from 'provider_selected' to 'provider.select_completed' while keeping the same metadata ({ provider: 'ollama' }) and return new OllamaProvider(); ensure the getLog().debug invocation and the OllamaProvider return remain in the same branch.
🤖 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/config/config-loader.ts`:
- Around line 288-290: The current assignment replaces the entire Ollama config
when merging global.assistants.ollama into result.assistants.ollama, which drops
unspecified fields (e.g., baseUrl) on partial updates; change the merge to
shallow-merge existing result.assistants.ollama with global.assistants.ollama
instead of outright replacement (use the same merge approach used for the other
providers), and introduce/respect an explicit clear signal (e.g., a null/""
sentinel) if callers intend to remove a field; apply this fix for the
occurrences referencing global.assistants.ollama and result.assistants.ollama
(and the similar blocks at the other noted locations).
In `@packages/core/src/db/conversations.test.ts`:
- Around line 14-22: Replace the process-global Bun mock.module() usage that
mocks config-loader with a local spy so mocks are restored between tests: import
the config-loader module (referenced as configLoader) and replace the
mock.module('../config/config-loader') pattern by calling spyOn(configLoader,
'loadConfig') to mockResolvedValue the desired config, and ensure tests call
mockRestore() (or rely on test framework cleanup) so loadConfig is restored;
remove the mock.module() call and any references to it to avoid cross-test
leakage.
In `@packages/core/src/db/conversations.ts`:
- Line 97: The code currently force-casts the DB field ai_assistant_type into a
union (assistantType = parent.rows[0].ai_assistant_type as 'claude' | 'codex' |
'ollama'), which can hide invalid DB values; replace the cast with an explicit
whitelist check: read the raw value (e.g., const raw =
parent.rows[0].ai_assistant_type), verify it is one of
['claude','codex','ollama'], and if not throw a clear Error stating the invalid
ai_assistant_type and the conversation id/context; apply the same check where
the same cast appears (the other occurrence mentioned) so you fail fast instead
of propagating an unsupported provider.
- Around line 76-87: Remove the DEFAULT_AI_ASSISTANT env parsing from
getOrCreateConversation() in packages/core/src/db/conversations.ts and instead
implement the env override and validation inside loadConfig() in
packages/core/src/config/config-loader.ts: update loadConfig() to read
process.env.DEFAULT_AI_ASSISTANT, validate it against the allowed values
('claude'|'codex'|'ollama'), override the config.assistant when valid (and throw
or fallback consistently when invalid), and ensure its return type reflects the
validated assistant; after that, simplify getOrCreateConversation() to consume
only config.assistant and delete the assistantType/envAssistant block so there
is a single source of truth.
In `@packages/providers/src/ollama/provider.ts`:
- Around line 137-143: The current JSON.parse error handler in the Ollama stream
loop logs the raw `trimmed` chunk via getLog().warn, which may expose user
content; change the handler in provider.ts (around the JSON.parse of `trimmed`
into OllamaChatChunk) to avoid logging the full chunk: log only metadata (e.g.,
stream position or chunk length) or a masked preview (e.g., first 8 characters +
'...' or a fixed-length hash/ellipsis) instead of `trimmed`. Keep the existing
warning key ('ollama.unparseable_chunk') and include minimal context
(maskedPreview or length) so no full model output, tokens, or PII are written to
logs.
- Around line 63-83: sendQuery is dropping repository context because _cwd is
never forwarded; update the messages array construction in sendQuery to include
the working-directory/project context (use _cwd) before the user prompt — e.g.,
add a message (system or context role) that contains the repository path and any
available repo-summary/manifest info derived from _cwd so Ollama receives
repo-scoped context; modify the messages.push flow around messages (the messages
variable and its existing system/user pushes) to include this new context
message.
In `@packages/server/src/routes/api.ts`:
- Around line 2488-2513: Validate and bound the outbound Ollama probe by parsing
and validating the computed baseUrl (from loadConfig/process.env or default)
inside the registerOpenApiRoute handler for getOllamaModelsRoute: ensure the URL
uses an allowed scheme (http or https) and that the hostname is in an allowlist
(e.g., localhost, 127.0.0.1, ::1 and any explicitly configured trusted
hostnames) or otherwise reject early with a clear 4xx response and a logged
warning via getLog() rather than proceeding; then create an AbortController with
a short timeout (e.g., 1–3s) and pass its signal to fetch(`${baseUrl}/api/tags`)
so slow/unresponsive targets are aborted, and handle fetch errors (including
abort) by logging the error with getLog().warn/error and returning a safe JSON
response (or a clear error status) instead of silently swallowing
exceptions—apply these changes where loadConfig(), fetch, and getLog() are used
in this route.
In `@packages/server/src/routes/schemas/config.schemas.ts`:
- Around line 55-60: The ollama.baseUrl currently accepts any string; update the
zod schema in config.schemas.ts (the ollama object) to validate baseUrl as a
well-formed HTTP or HTTPS URL instead of a plain string. Replace the current
baseUrl validator (z.string().optional()) with a URL validation that ensures the
value parses as a URL and that its protocol is http: or https: (use
z.string().url() plus a refine or a custom check against new URL(...).protocol),
keep the field optional but reject non-HTTP(S) values with a clear error
message.
In `@packages/workflows/src/model-validation.ts`:
- Around line 20-26: inferProviderFromModel currently falls back to 'codex' for
any non-Claude model; change it to explicitly check each provider recognizer
(isClaudeModel, isCodexModel, isOllamaModel) and return the matching provider
('claude'|'codex'|'ollama'), and if none match return the passed defaultProvider
instead of hardcoding 'codex'; also update the corresponding compatibility logic
in isModelCompatible (the other block at lines ~38-46) so it does not accept
unknown models as Codex — require an explicit provider when the model cannot be
uniquely classified.
In `@scripts/dev-restart.sh`:
- Around line 7-8: The LOG_DIR calculation uses dirname on REPO_ROOT, placing
logs outside the repository; update the dev-restart.sh logic so LOG_DIR is set
to "$REPO_ROOT/logs" (not "$(dirname "$REPO_ROOT")/logs"), ensure LOG_DIR exists
before writing, and keep LOG_FILE defined as "$LOG_DIR/server.log" so all log
files are created inside the repository rather than its parent directory.
- Around line 13-18: Remove the two overly broad pkill patterns "pkill -f
\"bun.*dev:server\"" and "pkill -f \"bun.*dev:web\"" and rely on the more
specific package-targeted pkill commands that match "bun.*@archon/server.*dev"
and "bun.*@archon/web.*dev" to avoid killing unrelated Bun processes, and
replace the hardcoded fuser call that releases port 3090 with a call that uses
the $PORT environment variable (falling back to 3090 if unset) so the script
honors custom port configuration.
---
Outside diff comments:
In `@packages/web/src/routes/SettingsPage.tsx`:
- Around line 463-476: The payload always includes an ollama key which causes
accidental updates; modify handleSave so that ollama is only added to the object
passed to mutation.mutate when there is something to update (i.e., when
ollamaModel or ollamaBaseUrl is truthy). Build a small ollamaPayload from
ollamaModel and ollamaBaseUrl, and conditionally spread it into the mutate call
like the codex block (e.g., ...(ollamaPayload ? { ollama: ollamaPayload } :
{})), ensuring no empty ollama object is sent when neither setting is provided.
---
Nitpick comments:
In `@packages/providers/src/factory.ts`:
- Around line 42-44: The log event string for the Ollama branch uses an
inconsistent name; update the getLog().debug call in the case 'ollama' to follow
the repository Pino naming convention (domain.action_state) — for example change
the event key from 'provider_selected' to 'provider.select_completed' while
keeping the same metadata ({ provider: 'ollama' }) and return new
OllamaProvider(); ensure the getLog().debug invocation and the OllamaProvider
return remain in the same branch.
🪄 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: 96ae5d23-05ff-4d0b-a3a8-e80ba0719e27
📒 Files selected for processing (29)
.env.examplepackages/core/package.jsonpackages/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/providers/package.jsonpackages/providers/src/factory.tspackages/providers/src/index.tspackages/providers/src/ollama/capabilities.tspackages/providers/src/ollama/config.tspackages/providers/src/ollama/index.tspackages/providers/src/ollama/provider.tspackages/providers/src/types.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/config.schemas.tspackages/web/src/components/layout/TopNav.tsxpackages/web/src/lib/api.generated.d.tspackages/web/src/lib/api.tspackages/web/src/routes/SettingsPage.tsxpackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/executor.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/workflow.tsscripts/dev-restart.sh
Comment on lines
+288
to
+290
| if (global.assistants?.ollama !== undefined) { | ||
| result.assistants.ollama = global.assistants.ollama; | ||
| } |
There was a problem hiding this comment.
Don't replace the whole Ollama block on partial merges.
These assignments drop unspecified fields. A repo config like assistants.ollama: { model: 'llama3' } will erase a globally configured baseUrl, and a PATCH payload that updates only model will wipe the saved URL for the same reason. Please merge ollama like the other providers here, and use an explicit clear signal if the API needs to remove a field.
Also applies to: 343-345, 454-460
🤖 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 288 - 290, The
current assignment replaces the entire Ollama config when merging
global.assistants.ollama into result.assistants.ollama, which drops unspecified
fields (e.g., baseUrl) on partial updates; change the merge to shallow-merge
existing result.assistants.ollama with global.assistants.ollama instead of
outright replacement (use the same merge approach used for the other providers),
and introduce/respect an explicit clear signal (e.g., a null/"" sentinel) if
callers intend to remove a field; apply this fix for the occurrences referencing
global.assistants.ollama and result.assistants.ollama (and the similar blocks at
the other noted locations).
Comment on lines
+14
to
+22
| // Mock config-loader: getOrCreateConversation calls loadConfig() for the default assistant | ||
| mock.module('../config/config-loader', () => ({ | ||
| loadConfig: mock(() => | ||
| Promise.resolve({ | ||
| assistant: 'claude', | ||
| assistants: { claude: {}, codex: {}, ollama: {} }, | ||
| }) | ||
| ), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all Bun module mocks in tests.
rg -n --type=ts "mock\.module\(" packages
# Show every test that mocks config-loader specifically.
rg -n --type=ts "mock\.module\(\s*['\"][^'\"]*config-loader['\"]" packages
# Show existing spy-based loadConfig stubs, if any.
rg -n --type=ts "spyOn\([^,]+,\s*['\"]loadConfig['\"]\)" packagesRepository: coleam00/Archon
Length of output: 29543
🏁 Script executed:
# Check the config-loader.ts file structure and exports
head -100 packages/core/src/config/config-loader.ts
# Check all mock implementations of loadConfig to see if they differ
echo "=== conversations.test.ts mock ===" && sed -n '14,25p' packages/core/src/db/conversations.test.ts
echo -e "\n=== store-adapter.test.ts mock ===" && sed -n '51,60p' packages/core/src/workflows/store-adapter.test.ts
echo -e "\n=== orchestrator.test.ts mock ===" && sed -n '118,130p' packages/core/src/orchestrator/orchestrator.test.ts
echo -e "\n=== orchestrator-isolation.test.ts mock ===" && sed -n '65,75p' packages/core/src/orchestrator/orchestrator-isolation.test.ts
echo -e "\n=== orchestrator-agent.test.ts mock ===" && sed -n '145,155p' packages/core/src/orchestrator/orchestrator-agent.test.ts
echo -e "\n=== config-loader.test.ts mock ===" && sed -n '24,35p' packages/core/src/config/config-loader.test.tsRepository: coleam00/Archon
Length of output: 5249
🏁 Script executed:
# Check what mockLoadConfig is in orchestrator.test.ts
grep -n "mockLoadConfig" packages/core/src/orchestrator/orchestrator.test.ts | head -20
# Check what mockLoadConfig is in orchestrator-agent.test.ts
grep -n "mockLoadConfig" packages/core/src/orchestrator/orchestrator-agent.test.ts | head -20
# Check the actual loadConfig export from config-loader
grep -n "export.*loadConfig" packages/core/src/config/config-loader.tsRepository: coleam00/Archon
Length of output: 593
🏁 Script executed:
# Get full context of conversations.test.ts around lines 14-22
sed -n '1,35p' packages/core/src/db/conversations.test.ts
# Check if loadConfig is used directly in conversations.test.ts
grep -n "loadConfig" packages/core/src/db/conversations.test.ts
# Check the orchestrator.test.ts mockLoadConfig implementation
sed -n '105,115p' packages/core/src/orchestrator/orchestrator.test.ts
# Check the orchestrator-agent.test.ts mockLoadConfig implementation
sed -n '45,55p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 1886
🏁 Script executed:
# Check how loadConfig is imported and used in conversations.ts
sed -n '1,50p' packages/core/src/db/conversations.ts | grep -A5 "loadConfig"
# Check what getOrCreateConversation does
grep -n "getOrCreateConversation" packages/core/src/db/conversations.ts
# View the implementation
sed -n '80,150p' packages/core/src/db/conversations.tsRepository: coleam00/Archon
Length of output: 2897
🏁 Script executed:
# Find where loadConfig is actually called in getOrCreateConversation
sed -n '58,120p' packages/core/src/db/conversations.tsRepository: coleam00/Archon
Length of output: 2480
Use spyOn(configLoader, 'loadConfig') instead of mock.module().
This file mocks config-loader alongside at least four other test files (store-adapter.test.ts, orchestrator.test.ts, orchestrator-isolation.test.ts, orchestrator-agent.test.ts) with different implementations. Bun's mock.module() is process-global and persists across test files; when tests run in any order, an earlier test's mock can bleed into this test and cause flaky failures. Import the module and use spyOn(configLoader, 'loadConfig') so mockRestore() cleans up correctly:
import * as configLoader from '../config/config-loader';
spyOn(configLoader, 'loadConfig').mockResolvedValue({
assistant: 'claude',
assistants: { claude: {}, codex: {}, ollama: {} },
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/db/conversations.test.ts` around lines 14 - 22, Replace the
process-global Bun mock.module() usage that mocks config-loader with a local spy
so mocks are restored between tests: import the config-loader module (referenced
as configLoader) and replace the mock.module('../config/config-loader') pattern
by calling spyOn(configLoader, 'loadConfig') to mockResolvedValue the desired
config, and ensure tests call mockRestore() (or rely on test framework cleanup)
so loadConfig is restored; remove the mock.module() call and any references to
it to avoid cross-test leakage.
Comment on lines
+76
to
+87
| const config = await loadConfig(); | ||
| let assistantType: 'claude' | 'codex' | 'ollama' = config.assistant; | ||
| const envAssistant = process.env.DEFAULT_AI_ASSISTANT; | ||
| if (envAssistant) { | ||
| if (envAssistant === 'claude' || envAssistant === 'codex' || envAssistant === 'ollama') { | ||
| assistantType = envAssistant; | ||
| } else { | ||
| throw new Error( | ||
| `Invalid DEFAULT_AI_ASSISTANT: "${envAssistant}". Must be one of: claude, codex, ollama` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep DEFAULT_AI_ASSISTANT parsing in loadConfig(), not in the DB layer.
This block makes getOrCreateConversation() a second source of truth for assistant selection. With the current shared config path, conversation creation can honor DEFAULT_AI_ASSISTANT=ollama here while other callers of loadConfig() still read a different default assistant, so provider choice becomes inconsistent across the app. Please fix the env override centrally in packages/core/src/config/config-loader.ts and keep this function consuming config.assistant only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/db/conversations.ts` around lines 76 - 87, Remove the
DEFAULT_AI_ASSISTANT env parsing from getOrCreateConversation() in
packages/core/src/db/conversations.ts and instead implement the env override and
validation inside loadConfig() in packages/core/src/config/config-loader.ts:
update loadConfig() to read process.env.DEFAULT_AI_ASSISTANT, validate it
against the allowed values ('claude'|'codex'|'ollama'), override the
config.assistant when valid (and throw or fallback consistently when invalid),
and ensure its return type reflects the validated assistant; after that,
simplify getOrCreateConversation() to consume only config.assistant and delete
the assistantType/envAssistant block so there is a single source of truth.
| inheritedCodebaseId = parent.rows[0].codebase_id; | ||
| inheritedCwd = parent.rows[0].cwd; | ||
| assistantType = parent.rows[0].ai_assistant_type; | ||
| assistantType = parent.rows[0].ai_assistant_type as 'claude' | 'codex' | 'ollama'; |
There was a problem hiding this comment.
Validate DB assistant types before narrowing them.
These casts trust whatever string is stored in ai_assistant_type. If a stale row contains anything else, we'll persist or propagate an invalid provider and only fail later when provider dispatch runs. Parse the value explicitly and throw here.
Proposed fix
+function parseAssistantType(value: string): 'claude' | 'codex' | 'ollama' {
+ if (value === 'claude' || value === 'codex' || value === 'ollama') {
+ return value;
+ }
+ throw new Error(`Unsupported ai_assistant_type: "${value}"`);
+}
+
if (parent.rows[0]) {
inheritedCodebaseId = parent.rows[0].codebase_id;
inheritedCwd = parent.rows[0].cwd;
- assistantType = parent.rows[0].ai_assistant_type as 'claude' | 'codex' | 'ollama';
+ assistantType = parseAssistantType(parent.rows[0].ai_assistant_type);
getLog().debug(
{ inheritedCodebaseId, inheritedCwd },
'db.conversation_parent_context_inherited'
@@
if (codebase.rows[0]) {
- assistantType = codebase.rows[0].ai_assistant_type as 'claude' | 'codex' | 'ollama';
+ assistantType = parseAssistantType(codebase.rows[0].ai_assistant_type);
}As per coding guidelines: "Prefer throwing early with clear errors for unsupported or unsafe states; never silently swallow errors or broaden permissions; document intentional fallback behavior with comments".
Also applies to: 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/db/conversations.ts` at line 97, The code currently
force-casts the DB field ai_assistant_type into a union (assistantType =
parent.rows[0].ai_assistant_type as 'claude' | 'codex' | 'ollama'), which can
hide invalid DB values; replace the cast with an explicit whitelist check: read
the raw value (e.g., const raw = parent.rows[0].ai_assistant_type), verify it is
one of ['claude','codex','ollama'], and if not throw a clear Error stating the
invalid ai_assistant_type and the conversation id/context; apply the same check
where the same cast appears (the other occurrence mentioned) so you fail fast
instead of propagating an unsupported provider.
Comment on lines
+63
to
+83
| async *sendQuery( | ||
| prompt: string, | ||
| _cwd: string, | ||
| _resumeSessionId?: string, | ||
| options?: SendQueryOptions | ||
| ): AsyncGenerator<MessageChunk> { | ||
| const assistantCfg = parseOllamaConfig(options?.assistantConfig ?? {}); | ||
| const model = options?.model ?? assistantCfg.model; | ||
|
|
||
| if (!model) { | ||
| throw new Error( | ||
| 'Ollama requires a model to be specified. ' + | ||
| 'Set `model` in your workflow or .archon/config.yaml assistants.ollama.model.' | ||
| ); | ||
| } | ||
|
|
||
| const messages: { role: string; content: string }[] = []; | ||
| if (options?.systemPrompt && typeof options.systemPrompt === 'string') { | ||
| messages.push({ role: 'system', content: options.systemPrompt }); | ||
| } | ||
| messages.push({ role: 'user', content: prompt }); |
There was a problem hiding this comment.
Ollama requests still drop repository context.
_cwd is ignored here and the only context forwarded is prompt plus optional systemPrompt. That matches the regression called out in the PR notes: repo-scoped Ollama runs will answer without any working-directory/project context until this provider threads it through like the other providers do.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/ollama/provider.ts` around lines 63 - 83, sendQuery is
dropping repository context because _cwd is never forwarded; update the messages
array construction in sendQuery to include the working-directory/project context
(use _cwd) before the user prompt — e.g., add a message (system or context role)
that contains the repository path and any available repo-summary/manifest info
derived from _cwd so Ollama receives repo-scoped context; modify the
messages.push flow around messages (the messages variable and its existing
system/user pushes) to include this new context message.
Comment on lines
+55
to
+60
| ollama: z | ||
| .object({ | ||
| model: z.string().optional(), | ||
| baseUrl: z.string().optional(), | ||
| }) | ||
| .optional(), |
There was a problem hiding this comment.
Validate ollama.baseUrl as a real HTTP(S) URL.
Right now any string is accepted and later fed into a server-side fetch. That turns typos into runtime failures and makes the settings endpoint an easy way to point the server at arbitrary targets. Reject malformed or non-HTTP(S) values here.
Suggested fix
ollama: z
.object({
model: z.string().optional(),
- baseUrl: z.string().optional(),
+ baseUrl: z
+ .string()
+ .url()
+ .refine(value => value.startsWith('http://') || value.startsWith('https://'), {
+ message: 'Ollama baseUrl must be an http(s) URL',
+ })
+ .optional(),
})
.optional(),As per coding guidelines, "Prefer throwing early with clear errors for unsupported or unsafe states; never silently swallow errors or broaden permissions; document intentional fallback behavior with comments".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ollama: z | |
| .object({ | |
| model: z.string().optional(), | |
| baseUrl: z.string().optional(), | |
| }) | |
| .optional(), | |
| ollama: z | |
| .object({ | |
| model: z.string().optional(), | |
| baseUrl: z | |
| .string() | |
| .url() | |
| .refine(value => value.startsWith('http://') || value.startsWith('https://'), { | |
| message: 'Ollama baseUrl must be an http(s) URL', | |
| }) | |
| .optional(), | |
| }) | |
| .optional(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/routes/schemas/config.schemas.ts` around lines 55 - 60,
The ollama.baseUrl currently accepts any string; update the zod schema in
config.schemas.ts (the ollama object) to validate baseUrl as a well-formed HTTP
or HTTPS URL instead of a plain string. Replace the current baseUrl validator
(z.string().optional()) with a URL validation that ensures the value parses as a
URL and that its protocol is http: or https: (use z.string().url() plus a refine
or a custom check against new URL(...).protocol), keep the field optional but
reject non-HTTP(S) values with a clear error message.
Comment on lines
+414
to
+417
| // Section enable/disable (local UI only — controls whether inputs are editable) | ||
| const [claudeEnabled, setClaudeEnabled] = useState(true); | ||
| const [codexEnabled, setCodexEnabled] = useState(true); | ||
| const [ollamaEnabled, setOllamaEnabled] = useState(true); |
There was a problem hiding this comment.
The provider enable/disable checkboxes are cosmetic right now.
These booleans never affect available, hasChanges, or the PATCH payload, so a provider can be “disabled” in the UI and still remain selectable and fully enabled after reload. Either persist this state in config or drop the checkboxes from this PR.
Also applies to: 480-504, 576-724
Comment on lines
20
to
26
| export function inferProviderFromModel( | ||
| model: string | undefined, | ||
| defaultProvider: 'claude' | 'codex' | ||
| ): 'claude' | 'codex' { | ||
| defaultProvider: 'claude' | 'codex' | 'ollama' | ||
| ): 'claude' | 'codex' | 'ollama' { | ||
| if (!model) return defaultProvider; | ||
| if (isClaudeModel(model)) return 'claude'; | ||
| return 'codex'; |
There was a problem hiding this comment.
Unknown models still get silently routed to Codex.
After adding Ollama, inferProviderFromModel() still returns 'codex' for every non-Claude model, and isModelCompatible() also accepts those models for Codex. That means a workflow/node with model: llama3 but no explicit provider will execute against Codex instead of remaining on Ollama or failing fast. The fallback needs to preserve the default provider for unknown models, or require an explicit provider once the model cannot be uniquely classified.
Also applies to: 38-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/model-validation.ts` around lines 20 - 26,
inferProviderFromModel currently falls back to 'codex' for any non-Claude model;
change it to explicitly check each provider recognizer (isClaudeModel,
isCodexModel, isOllamaModel) and return the matching provider
('claude'|'codex'|'ollama'), and if none match return the passed defaultProvider
instead of hardcoding 'codex'; also update the corresponding compatibility logic
in isModelCompatible (the other block at lines ~38-46) so it does not accept
unknown models as Codex — require an explicit provider when the model cannot be
uniquely classified.
Comment on lines
+7
to
+8
| LOG_DIR="$(dirname "$REPO_ROOT")/logs" | ||
| LOG_FILE="$LOG_DIR/server.log" |
There was a problem hiding this comment.
Log directory path points outside the repository.
Line 7 computes LOG_DIR as the parent of repo root ($(dirname "$REPO_ROOT")/logs), which is surprising given Line 3’s usage guidance and can cause misplaced logs.
Proposed fix
-LOG_DIR="$(dirname "$REPO_ROOT")/logs"
+LOG_DIR="$REPO_ROOT/logs"
LOG_FILE="$LOG_DIR/server.log"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| LOG_DIR="$(dirname "$REPO_ROOT")/logs" | |
| LOG_FILE="$LOG_DIR/server.log" | |
| LOG_DIR="$REPO_ROOT/logs" | |
| LOG_FILE="$LOG_DIR/server.log" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-restart.sh` around lines 7 - 8, The LOG_DIR calculation uses
dirname on REPO_ROOT, placing logs outside the repository; update the
dev-restart.sh logic so LOG_DIR is set to "$REPO_ROOT/logs" (not "$(dirname
"$REPO_ROOT")/logs"), ensure LOG_DIR exists before writing, and keep LOG_FILE
defined as "$LOG_DIR/server.log" so all log files are created inside the
repository rather than its parent directory.
Comment on lines
+13
to
+18
| pkill -f "bun.*@archon/server.*dev" 2>/dev/null || true | ||
| pkill -f "bun.*@archon/web.*dev" 2>/dev/null || true | ||
| pkill -f "bun.*dev:server" 2>/dev/null || true | ||
| pkill -f "bun.*dev:web" 2>/dev/null || true | ||
| # Release port 3090 if anything else is still holding it | ||
| fuser -k 3090/tcp 2>/dev/null || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: detect broad kill patterns and hardcoded port assumptions
fd -a "dev-restart.sh" scripts
rg -n 'pkill -f "bun\.\*dev:server"|pkill -f "bun\.\*dev:web"|fuser -k 3090/tcp|LOG_DIR="\$\(dirname "\$REPO_ROOT"\)/logs"' scripts/dev-restart.shRepository: coleam00/Archon
Length of output: 300
🏁 Script executed:
cat -n scripts/dev-restart.sh | head -30Repository: coleam00/Archon
Length of output: 1284
🏁 Script executed:
# Check for PORT variable usage and PID file patterns in the entire script
rg -n "PORT|\.pid|PID" scripts/dev-restart.shRepository: coleam00/Archon
Length of output: 162
🏁 Script executed:
# Verify the exact patterns and context around the flagged lines
sed -n '1,25p' scripts/dev-restart.shRepository: coleam00/Archon
Length of output: 947
🏁 Script executed:
# See the rest of the script to understand PID usage
sed -n '25,50p' scripts/dev-restart.shRepository: coleam00/Archon
Length of output: 292
🏁 Script executed:
# Check if PORT is used as an environment variable in other scripts or documentation
rg -i "PORT.*3090|PORT=|export PORT" scripts/ --type shRepository: coleam00/Archon
Length of output: 138
🏁 Script executed:
# Check if there's any PID file management in other scripts or if this is a pattern
rg "\.pid" scripts/ --type shRepository: coleam00/Archon
Length of output: 41
Lines 15–16 and line 18 use overly broad patterns that can kill unrelated processes and ignore port configuration.
The pkill patterns bun.*dev:server and bun.*dev:web (lines 15–16) will match any bun process with those strings in the command, not just Archon's. The hardcoded port 3090 (line 18) doesn't support custom port configuration and will conflict when multiple instances need to run on different ports.
Recommended fix
+PORT="${PORT:-3090}"
+
echo "==> Stopping existing Archon processes..."
pkill -f "bun.*@archon/server.*dev" 2>/dev/null || true
pkill -f "bun.*@archon/web.*dev" 2>/dev/null || true
-pkill -f "bun.*dev:server" 2>/dev/null || true
-pkill -f "bun.*dev:web" 2>/dev/null || true
# Release port 3090 if anything else is still holding it
-fuser -k 3090/tcp 2>/dev/null || true
+fuser -k "${PORT}/tcp" 2>/dev/null || trueRemove lines 15–16 (generic dev script names) and use lines 13–14 instead (specific Archon package names). Replace the hardcoded port with $PORT environment variable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pkill -f "bun.*@archon/server.*dev" 2>/dev/null || true | |
| pkill -f "bun.*@archon/web.*dev" 2>/dev/null || true | |
| pkill -f "bun.*dev:server" 2>/dev/null || true | |
| pkill -f "bun.*dev:web" 2>/dev/null || true | |
| # Release port 3090 if anything else is still holding it | |
| fuser -k 3090/tcp 2>/dev/null || true | |
| PORT="${PORT:-3090}" | |
| echo "==> Stopping existing Archon processes..." | |
| pkill -f "bun.*@archon/server.*dev" 2>/dev/null || true | |
| pkill -f "bun.*@archon/web.*dev" 2>/dev/null || true | |
| # Release port 3090 if anything else is still holding it | |
| fuser -k "${PORT}/tcp" 2>/dev/null || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-restart.sh` around lines 13 - 18, Remove the two overly broad
pkill patterns "pkill -f \"bun.*dev:server\"" and "pkill -f \"bun.*dev:web\""
and rely on the more specific package-targeted pkill commands that match
"bun.*@archon/server.*dev" and "bun.*@archon/web.*dev" to avoid killing
unrelated Bun processes, and replace the hardcoded fuser call that releases port
3090 with a call that uses the $PORT environment variable (falling back to 3090
if unset) so the script honors custom port configuration.
Collaborator
|
Related to #1096 — overlapping area or partial fix. |
Collaborator
|
Please feel free to adapt to the provider pattern behind the community provider registry Note: we will only accept Coding agent SDK's at this point you can use ollama in claude code or already implemented PI |
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
2 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.
Context
This PR is the post-refactor successor to #1158, which added Ollama as a third AI provider and was fully validated against
devbefore upstream PR #1137 extracted@archon/providersfrom@archon/core. That branch is preserved as a reference baseline. This branch adapts the same feature to the new package structure introduced by #1137, #1162, and #1185.Summary
OllamaProviderintopackages/providers/src/ollama/implementing the newIAgentProviderinterface withsendQuery(),getType(), andgetCapabilities()'ollama'in@archon/providersfactory alongsideclaudeandcodexparseOllamaConfig()following theassistantConfig: Record<string, unknown>pattern introduced by refactor: decompose provider sendQuery() into explicit helper boundaries #1162WorkflowConfig,AgentProviderFactory, and all provider union types to include'ollama'inferProviderFromModel/isModelCompatiblein@archon/workflowsfor OllamaGET /api/ollama/modelsdiscovery endpoint (proxies Ollama/api/tags)PATCH /api/config/assistantsto accept Ollama settingsTopNavlinking to SettingsOLLAMA_BASE_URLto.env.exampleconversations.test.tsbatch isolation and addconfig-loadermockscripts/dev-restart.shKnown Regression: Project Context with Ollama
After the
@archon/providersrefactor, the project (codebase) selection dropdown on the Chat page no longer appears to give Ollama models awareness of the selected repository. Prior to the rebase,gemma4:latestwas able to traverse the selected repo's file tree without the user explicitly mentioning it — consistent with how Claude and Codex behave when a codebase is selected.The root cause has not been fully traced but likely involves how
assistantConfig(and by extension the working directory / codebase context) is now plumbed throughSendQueryOptionstoOllamaProvider.sendQuery(). The provider receivescwdas a parameter but the system prompt construction that injects repo context may not be wiring it through correctly for Ollama after the refactor.Recommendation: Workspaces
This regression surfaced a deeper UX gap that is worth addressing as a follow-on feature. Rather than per-codebase project selection, a Workspaces model would be more natural for real-world usage:
This has emerged as a clearly effective pattern in practice: org-level agents that can reason across multiple repos (dependency graphs, cross-repo refactors, shared config) and project-level agents that stay focused. The current per-codebase model forces users to context-switch manually. A workspace layer would let Ollama (and Claude/Codex) operate at the right granularity without the user having to spell out paths in every message.
Validation
Security Impact
OllamaProvidercalls${baseUrl}/api/chat(defaulthttp://localhost:11434), user-configured🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation