-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(providers): adapt Ollama support to @archon/providers package (#1137 rebase) #1201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,16 @@ mock.module('./connection', () => ({ | |
| getDialect: () => mockPostgresDialect, | ||
| })); | ||
|
|
||
| // 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: {} }, | ||
| }) | ||
| ), | ||
| })); | ||
|
Comment on lines
+14
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 This file mocks import * as configLoader from '../config/config-loader';
spyOn(configLoader, 'loadConfig').mockResolvedValue({
assistant: 'claude',
assistants: { claude: {}, codex: {}, ollama: {} },
});🤖 Prompt for AI Agents |
||
|
|
||
| import { | ||
| getOrCreateConversation, | ||
| updateConversation, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { pool, getDialect } from './connection'; | |
| import type { Conversation } from '../types'; | ||
| import { ConversationNotFoundError } from '../types'; | ||
| import { createLogger } from '@archon/paths'; | ||
| import { loadConfig } from '../config/config-loader'; | ||
|
|
||
| /** Lazy-initialized logger (deferred so test mocks can intercept createLogger) */ | ||
| let cachedLog: ReturnType<typeof createLogger> | undefined; | ||
|
|
@@ -72,7 +73,18 @@ export async function getOrCreateConversation( | |
| // Check if we should inherit from a parent conversation (e.g., Discord thread inheriting from parent channel) | ||
| let inheritedCodebaseId: string | null = null; | ||
| let inheritedCwd: string | null = null; | ||
| let assistantType = process.env.DEFAULT_AI_ASSISTANT ?? 'claude'; | ||
| 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` | ||
| ); | ||
| } | ||
| } | ||
|
Comment on lines
+76
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep This block makes 🤖 Prompt for AI Agents |
||
|
|
||
| if (parentConversationId) { | ||
| const parent = await pool.query<Conversation>( | ||
|
|
@@ -82,7 +94,7 @@ export async function getOrCreateConversation( | |
| if (parent.rows[0]) { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate DB assistant types before narrowing them. These casts trust whatever string is stored in 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 |
||
| getLog().debug( | ||
| { inheritedCodebaseId, inheritedCwd }, | ||
| 'db.conversation_parent_context_inherited' | ||
|
|
@@ -100,7 +112,7 @@ export async function getOrCreateConversation( | |
| [codebaseId] | ||
| ); | ||
| if (codebase.rows[0]) { | ||
| assistantType = codebase.rows[0].ai_assistant_type; | ||
| assistantType = codebase.rows[0].ai_assistant_type as 'claude' | 'codex' | 'ollama'; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import type { ProviderCapabilities } from '../types'; | ||
|
|
||
| /** | ||
| * Ollama capability flags. | ||
| * Ollama runs locally via /api/chat — no session resume, MCP, hooks, or SDK-level | ||
| * tool restrictions. Structured output, cost control, effort, and sandbox are also | ||
| * unsupported at this time. | ||
| */ | ||
| export const OLLAMA_CAPABILITIES: ProviderCapabilities = { | ||
| sessionResume: false, | ||
| mcp: false, | ||
| hooks: false, | ||
| skills: false, | ||
| toolRestrictions: false, | ||
| structuredOutput: false, | ||
| envInjection: false, | ||
| costControl: false, | ||
| effortControl: false, | ||
| thinkingControl: false, | ||
| fallbackModel: false, | ||
| sandbox: false, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * Typed config parsing for Ollama provider defaults. | ||
| * Validates and narrows the opaque assistantConfig to typed fields. | ||
| */ | ||
| import type { OllamaProviderDefaults } from '../types'; | ||
|
|
||
| // Re-export so consumers can import the type from either location | ||
| export type { OllamaProviderDefaults } from '../types'; | ||
|
|
||
| /** | ||
| * Parse raw assistantConfig into typed Ollama defaults. | ||
| * Defensive: invalid fields are silently dropped. | ||
| */ | ||
| export function parseOllamaConfig(raw: Record<string, unknown>): OllamaProviderDefaults { | ||
| const result: OllamaProviderDefaults = {}; | ||
|
|
||
| if (typeof raw.model === 'string') { | ||
| result.model = raw.model; | ||
| } | ||
|
|
||
| if (typeof raw.baseUrl === 'string') { | ||
| result.baseUrl = raw.baseUrl; | ||
| } | ||
|
|
||
| return result; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export { OllamaProvider } from './provider'; | ||
| export { parseOllamaConfig, type OllamaProviderDefaults } from './config'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 configuredbaseUrl, and a PATCH payload that updates onlymodelwill wipe the saved URL for the same reason. Please mergeollamalike 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