Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/core/src/orchestrator/orchestrator-agent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ mock.module('@archon/providers', () => ({
getType: mock(() => 'claude'),
getCapabilities: mock(() => ({})),
})),
getProviderCapabilities: mock(() => ({ envInjection: true })),
}));

mock.module('../db/env-vars', () => ({
Expand Down
14 changes: 13 additions & 1 deletion packages/core/src/orchestrator/orchestrator-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import * as commandHandler from '../handlers/command-handler';
import { formatToolCall } from '@archon/workflows/utils/tool-formatter';
import { classifyAndFormatError } from '../utils/error-formatter';
import { toError } from '../utils/error';
import { getAgentProvider } from '@archon/providers';
import { getAgentProvider, getProviderCapabilities } from '@archon/providers';
import { getArchonHome, getArchonWorkspacesPath } from '@archon/paths';
import { syncArchonToWorktree } from '../utils/worktree-sync';
import { syncWorkspace, toRepoPath } from '@archon/git';
Expand Down Expand Up @@ -772,6 +772,18 @@ export async function handleMessage(
}
}
const effectiveEnv = { ...(config.envVars ?? {}), ...dbEnvVars };

// Warn if provider doesn't support env injection but env vars are configured
if (Object.keys(effectiveEnv).length > 0) {
const providerCaps = getProviderCapabilities(providerKey);
if (!providerCaps.envInjection) {
getLog().warn(
{ provider: providerKey, envVarCount: Object.keys(effectiveEnv).length },
'orchestrator.unsupported_env_injection'
);
}
}

const requestOptions: SendQueryOptions = {
assistantConfig: (config.assistants[providerKey] ?? {}) as Record<string, unknown>,
env: Object.keys(effectiveEnv).length > 0 ? effectiveEnv : undefined,
Expand Down
16 changes: 16 additions & 0 deletions packages/providers/src/claude/capabilities.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import type { ProviderCapabilities } from '../types';

export const CLAUDE_CAPABILITIES: ProviderCapabilities = {
sessionResume: true,
mcp: true,
hooks: true,
skills: true,
toolRestrictions: true,
structuredOutput: true,
envInjection: true,
costControl: true,
effortControl: true,
thinkingControl: true,
fallbackModel: true,
sandbox: true,
};
16 changes: 2 additions & 14 deletions packages/providers/src/claude/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import type {
NodeConfig,
} from '../types';
import { parseClaudeConfig } from './config';
import { CLAUDE_CAPABILITIES } from './capabilities';
import { createLogger } from '@archon/paths';
import { readFile } from 'fs/promises';
import { resolve, isAbsolute } from 'path';
Expand Down Expand Up @@ -819,20 +820,7 @@ export class ClaudeProvider implements IAgentProvider {
}

getCapabilities(): ProviderCapabilities {
return {
sessionResume: true,
mcp: true,
hooks: true,
skills: true,
toolRestrictions: true,
structuredOutput: true,
envInjection: true,
costControl: true,
effortControl: true,
thinkingControl: true,
fallbackModel: true,
sandbox: true,
};
return CLAUDE_CAPABILITIES;
}

/**
Expand Down
16 changes: 16 additions & 0 deletions packages/providers/src/codex/capabilities.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import type { ProviderCapabilities } from '../types';

export const CODEX_CAPABILITIES: ProviderCapabilities = {
sessionResume: true,
mcp: false,
hooks: false,
skills: false,
toolRestrictions: false,
structuredOutput: true,
envInjection: true,
costControl: false,
effortControl: false,
thinkingControl: false,
fallbackModel: false,
sandbox: false,
};
16 changes: 2 additions & 14 deletions packages/providers/src/codex/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {
ProviderCapabilities,
} from '../types';
import { parseCodexConfig } from './config';
import { CODEX_CAPABILITIES } from './capabilities';
import { resolveCodexBinaryPath } from './binary-resolver';
import { createLogger } from '@archon/paths';

Expand Down Expand Up @@ -496,20 +497,7 @@ export class CodexProvider implements IAgentProvider {
}

getCapabilities(): ProviderCapabilities {
return {
sessionResume: true,
mcp: false,
hooks: false,
skills: false,
toolRestrictions: false,
structuredOutput: true,
envInjection: true,
costControl: false,
effortControl: false,
thinkingControl: false,
fallbackModel: false,
sandbox: false,
};
return CODEX_CAPABILITIES;
}

async *sendQuery(
Expand Down
42 changes: 41 additions & 1 deletion packages/providers/src/factory.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, test, expect } from 'bun:test';
import { getAgentProvider } from './factory';
import { getAgentProvider, getProviderCapabilities } from './factory';
import { UnknownProviderError } from './errors';

describe('factory', () => {
Expand Down Expand Up @@ -62,4 +62,44 @@ describe('factory', () => {
expect(codexCaps.hooks).toBe(false);
});
});

describe('getProviderCapabilities', () => {
test('returns Claude capabilities without instantiation', () => {
const caps = getProviderCapabilities('claude');
expect(caps.mcp).toBe(true);
expect(caps.hooks).toBe(true);
expect(caps.envInjection).toBe(true);
});

test('returns Codex capabilities without instantiation', () => {
const caps = getProviderCapabilities('codex');
expect(caps.mcp).toBe(false);
expect(caps.hooks).toBe(false);
expect(caps.envInjection).toBe(true);
});

test('matches runtime getCapabilities for Claude', () => {
const staticCaps = getProviderCapabilities('claude');
const runtimeCaps = getAgentProvider('claude').getCapabilities();
expect(staticCaps).toEqual(runtimeCaps);
});

test('matches runtime getCapabilities for Codex', () => {
const staticCaps = getProviderCapabilities('codex');
const runtimeCaps = getAgentProvider('codex').getCapabilities();
expect(staticCaps).toEqual(runtimeCaps);
});

test('throws UnknownProviderError for unknown type', () => {
expect(() => getProviderCapabilities('unknown')).toThrow(UnknownProviderError);
});

test('throws UnknownProviderError for empty string', () => {
expect(() => getProviderCapabilities('')).toThrow(UnknownProviderError);
});

test('is case sensitive - Claude throws', () => {
expect(() => getProviderCapabilities('Claude')).toThrow(UnknownProviderError);
});
});
});
21 changes: 19 additions & 2 deletions packages/providers/src/factory.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
/**
* Agent Provider Factory
*
* Dynamically instantiates the appropriate agent provider based on type string.
* Dynamic provider instantiation and static capability lookup.
* Built-in providers only: Claude and Codex.
*/
import type { IAgentProvider } from './types';
import type { IAgentProvider, ProviderCapabilities } from './types';
import { ClaudeProvider } from './claude/provider';
import { CodexProvider } from './codex/provider';
import { CLAUDE_CAPABILITIES } from './claude/capabilities';
import { CODEX_CAPABILITIES } from './codex/capabilities';
import { UnknownProviderError } from './errors';
import { createLogger } from '@archon/paths';

Expand Down Expand Up @@ -39,3 +41,18 @@ export function getAgentProvider(type: string): IAgentProvider {
throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]);
}
}

/**
* Get provider capabilities without instantiating a provider.
* Used by dag-executor and orchestrator for capability warnings.
*/
export function getProviderCapabilities(type: string): ProviderCapabilities {
switch (type) {
case 'claude':
return CLAUDE_CAPABILITIES;
case 'codex':
return CODEX_CAPABILITIES;
default:
throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]);
}
Comment on lines +49 to +57
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Return an immutable snapshot here.

Line 52 and Line 54 hand out the shared capability singletons by reference. Because those same objects are now part of the public surface, one accidental mutation by a consumer can silently change later capability checks for every caller in-process.

Possible fix
export function getProviderCapabilities(type: string): ProviderCapabilities {
  switch (type) {
    case 'claude':
-      return CLAUDE_CAPABILITIES;
+      return { ...CLAUDE_CAPABILITIES };
    case 'codex':
-      return CODEX_CAPABILITIES;
+      return { ...CODEX_CAPABILITIES };
    default:
      throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]);
  }
}
📝 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
export function getProviderCapabilities(type: string): ProviderCapabilities {
switch (type) {
case 'claude':
return CLAUDE_CAPABILITIES;
case 'codex':
return CODEX_CAPABILITIES;
default:
throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]);
}
export function getProviderCapabilities(type: string): ProviderCapabilities {
switch (type) {
case 'claude':
return { ...CLAUDE_CAPABILITIES };
case 'codex':
return { ...CODEX_CAPABILITIES };
default:
throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]);
}
}
🤖 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 49 - 57,
getProviderCapabilities currently returns shared capability singletons
(CLAUDE_CAPABILITIES, CODEX_CAPABILITIES) by reference; produce and return an
immutable snapshot instead to prevent consumer mutation from affecting global
state. Modify getProviderCapabilities to create a defensive copy of the selected
capability object (e.g., shallow/deep clone as appropriate for nested
properties) and freeze it (or deep-freeze) before returning; preserve the
UnknownProviderError(...) usage for the default case and continue to reference
REGISTERED_PROVIDERS when constructing the error. Ensure the cloning/freezing
logic is applied to both CLAUDE_CAPABILITIES and CODEX_CAPABILITIES returns so
callers receive an immutable snapshot.

}
4 changes: 3 additions & 1 deletion packages/providers/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ export type {
// Import from ./types directly or from the config modules — both work.

// Factory
export { getAgentProvider } from './factory';
export { getAgentProvider, getProviderCapabilities } from './factory';
// Static capability constants are intentionally NOT re-exported here.
// Use getProviderCapabilities() instead — it's the correct public seam.

// Error
export { UnknownProviderError } from './errors';
Expand Down
42 changes: 11 additions & 31 deletions packages/workflows/src/dag-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type {
ProviderCapabilities,
TokenUsage,
} from '@archon/providers/types';
import { getProviderCapabilities } from '@archon/providers';
import type {
DagNode,
ApprovalNode,
Expand Down Expand Up @@ -47,7 +48,7 @@ import { formatToolCall } from './utils/tool-formatter';
import { createLogger } from '@archon/paths';
import { getWorkflowEventEmitter } from './event-emitter';
import { evaluateCondition } from './condition-evaluator';
import { isClaudeModel, isModelCompatible } from './model-validation';
import { inferProviderFromModel, isModelCompatible } from './model-validation';
import {
logNodeStart,
logNodeComplete,
Expand Down Expand Up @@ -250,24 +251,14 @@ async function resolveNodeProviderAndModel(
conversationId: string,
workflowRunId: string,
_cwd: string,
workflowLevelOptions: WorkflowLevelOptions,
deps: WorkflowDeps
workflowLevelOptions: WorkflowLevelOptions
): Promise<{
provider: 'claude' | 'codex';
model: string | undefined;
options: SendQueryOptions | undefined;
}> {
let provider: 'claude' | 'codex';

if (node.provider) {
provider = node.provider;
} else if (node.model && isClaudeModel(node.model)) {
provider = 'claude';
} else if (node.model) {
provider = 'codex';
} else {
provider = workflowProvider;
}
const provider: 'claude' | 'codex' =
node.provider ?? inferProviderFromModel(node.model, workflowProvider);

const model =
node.model ??
Expand All @@ -279,9 +270,8 @@ async function resolveNodeProviderAndModel(
);
}

// Get provider capabilities for capability warnings
const aiClient = deps.getAgentProvider(provider);
const caps = aiClient.getCapabilities();
// Get provider capabilities for capability warnings (static lookup, no instantiation)
const caps = getProviderCapabilities(provider);

// Capability warnings — inform users when features are unsupported
const capChecks: [string, keyof ProviderCapabilities, boolean][] = [
Expand Down Expand Up @@ -2040,8 +2030,7 @@ async function executeApprovalNode(
conversationId,
workflowRun.id,
cwd,
workflowLevelOptions,
deps
workflowLevelOptions
);

const output = await executeNodeInternal(
Expand Down Expand Up @@ -2360,16 +2349,8 @@ export async function executeDagWorkflow(
// 3b. Loop node dispatch — manages its own AI sessions and iteration
if (isLoopNode(node)) {
// Resolve per-node provider/model overrides (same logic as other node types)
let loopProvider: 'claude' | 'codex';
if (node.provider) {
loopProvider = node.provider;
} else if (node.model && isClaudeModel(node.model)) {
loopProvider = 'claude';
} else if (node.model) {
loopProvider = 'codex';
} else {
loopProvider = workflowProvider;
}
const loopProvider: 'claude' | 'codex' =
node.provider ?? inferProviderFromModel(node.model, workflowProvider);
const loopModel =
node.model ??
(loopProvider === workflowProvider
Expand Down Expand Up @@ -2494,8 +2475,7 @@ export async function executeDagWorkflow(
conversationId,
workflowRun.id,
cwd,
workflowLevelOptions,
deps
workflowLevelOptions
);

// 5. Determine session — parallel or context:fresh → always fresh
Expand Down
7 changes: 2 additions & 5 deletions packages/workflows/src/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type { WorkflowDefinition, WorkflowRun, WorkflowExecutionResult } from '.
import { executeDagWorkflow } from './dag-executor';
import { logWorkflowStart, logWorkflowError } from './logger';
import { getWorkflowEventEmitter } from './event-emitter';
import { isClaudeModel, isModelCompatible } from './model-validation';
import { inferProviderFromModel, isModelCompatible } from './model-validation';
import { classifyError } from './executor-shared';

/** Lazy-initialized logger (deferred so test mocks can intercept createLogger) */
Expand Down Expand Up @@ -283,11 +283,8 @@ export async function executeWorkflow(
if (workflow.provider) {
resolvedProvider = workflow.provider;
providerSource = 'workflow definition';
} else if (workflow.model && isClaudeModel(workflow.model)) {
resolvedProvider = 'claude';
providerSource = 'inferred from workflow model';
} else if (workflow.model) {
resolvedProvider = 'codex';
resolvedProvider = inferProviderFromModel(workflow.model, config.assistant);
providerSource = 'inferred from workflow model';
} else {
resolvedProvider = config.assistant;
Expand Down
28 changes: 27 additions & 1 deletion packages/workflows/src/model-validation.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, it, expect } from 'bun:test';
import { isClaudeModel, isModelCompatible } from './model-validation';
import { isClaudeModel, isModelCompatible, inferProviderFromModel } from './model-validation';

describe('model-validation', () => {
describe('isClaudeModel', () => {
Expand Down Expand Up @@ -66,4 +66,30 @@ describe('model-validation', () => {
expect(isModelCompatible('codex', '')).toBe(true);
});
});

describe('inferProviderFromModel', () => {
it('should return default when model is undefined', () => {
expect(inferProviderFromModel(undefined, 'claude')).toBe('claude');
expect(inferProviderFromModel(undefined, 'codex')).toBe('codex');
});

it('should return default when model is empty string', () => {
expect(inferProviderFromModel('', 'claude')).toBe('claude');
expect(inferProviderFromModel('', 'codex')).toBe('codex');
});

it('should infer claude from Claude model names', () => {
expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude');
expect(inferProviderFromModel('opus', 'codex')).toBe('claude');
expect(inferProviderFromModel('haiku', 'codex')).toBe('claude');
expect(inferProviderFromModel('inherit', 'codex')).toBe('claude');
expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude');
});
Comment on lines +81 to +87
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't classify inherit as Claude.

inherit is a sentinel for "keep the default provider", not a Claude model alias. If this expectation drives the helper, the new node.provider ?? inferProviderFromModel(node.model, workflowProvider) call sites in packages/workflows/src/dag-executor.ts will reroute model: inherit nodes from Codex workflows to Claude.

Suggested test update
     it('should infer claude from Claude model names', () => {
       expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude');
       expect(inferProviderFromModel('opus', 'codex')).toBe('claude');
       expect(inferProviderFromModel('haiku', 'codex')).toBe('claude');
-      expect(inferProviderFromModel('inherit', 'codex')).toBe('claude');
       expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude');
     });
+
+    it('should keep the default provider for inherit', () => {
+      expect(inferProviderFromModel('inherit', 'claude')).toBe('claude');
+      expect(inferProviderFromModel('inherit', 'codex')).toBe('codex');
+    });
📝 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
it('should infer claude from Claude model names', () => {
expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude');
expect(inferProviderFromModel('opus', 'codex')).toBe('claude');
expect(inferProviderFromModel('haiku', 'codex')).toBe('claude');
expect(inferProviderFromModel('inherit', 'codex')).toBe('claude');
expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude');
});
it('should infer claude from Claude model names', () => {
expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude');
expect(inferProviderFromModel('opus', 'codex')).toBe('claude');
expect(inferProviderFromModel('haiku', 'codex')).toBe('claude');
expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude');
});
it('should keep the default provider for inherit', () => {
expect(inferProviderFromModel('inherit', 'claude')).toBe('claude');
expect(inferProviderFromModel('inherit', 'codex')).toBe('codex');
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/workflows/src/model-validation.test.ts` around lines 76 - 82, The
test and helper treat the model name "inherit" as a Claude alias; change
inferProviderFromModel to treat "inherit" as the sentinel meaning "use existing
provider" (i.e., do not map it to 'claude' — return undefined or null), and
update the test in model-validation.test.ts so
expect(inferProviderFromModel('inherit','codex')) reflects that sentinel
behavior instead of 'claude'; also ensure the call sites that use node.provider
?? inferProviderFromModel(node.model, workflowProvider) (refer to
dag-executor.ts) will preserve the workflowProvider when inferProviderFromModel
returns the sentinel/undefined.


it('should infer codex from non-Claude model names', () => {
expect(inferProviderFromModel('gpt-5.3-codex', 'claude')).toBe('codex');
expect(inferProviderFromModel('gpt-4', 'claude')).toBe('codex');
expect(inferProviderFromModel('o1-mini', 'claude')).toBe('codex');
});
});
});
Loading
Loading