Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
34 changes: 33 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,36 @@ 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);
});
});
});
19 changes: 18 additions & 1 deletion packages/providers/src/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
* Dynamically instantiates the appropriate agent provider based on type string.
* 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.

}
6 changes: 5 additions & 1 deletion packages/providers/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ 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
export { CLAUDE_CAPABILITIES } from './claude/capabilities';
export { CODEX_CAPABILITIES } from './codex/capabilities';

// Error
export { UnknownProviderError } from './errors';
Expand Down
35 changes: 9 additions & 26 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 @@ -251,23 +252,14 @@ async function resolveNodeProviderAndModel(
workflowRunId: string,
_cwd: string,
workflowLevelOptions: WorkflowLevelOptions,
deps: WorkflowDeps
_deps: WorkflowDeps
): 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 +271,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 @@ -2360,16 +2351,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
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
23 changes: 22 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,25 @@ 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 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');
});
});
});
18 changes: 18 additions & 0 deletions packages/workflows/src/model-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,24 @@ export function isClaudeModel(model: string): boolean {
);
}

/**
* Infer provider from a model name. Returns 'claude' if the model matches
* Claude naming patterns, 'codex' otherwise.
*
* When no model is provided, returns the default provider.
*
* Phase 2 will replace this with a registry-driven lookup that iterates
* built-in provider registrations.
*/
export function inferProviderFromModel(
model: string | undefined,
defaultProvider: 'claude' | 'codex'
): 'claude' | 'codex' {
if (!model) return defaultProvider;
if (isClaudeModel(model)) return 'claude';
return 'codex';
Comment on lines +24 to +26

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

'inherit' is incorrectly inferred as Claude.

inferProviderFromModel() currently treats 'inherit' as Claude via isClaudeModel(), which can incorrectly override the configured default provider and route execution to the wrong provider.

Proposed fix
 export function inferProviderFromModel(
   model: string | undefined,
   defaultProvider: 'claude' | 'codex'
 ): 'claude' | 'codex' {
   if (!model) return defaultProvider;
+  if (model === 'inherit') return defaultProvider;
   if (isClaudeModel(model)) return 'claude';
   return 'codex';
 }
🤖 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 24 - 26,
inferProviderFromModel currently classifies the string 'inherit' as Claude
because isClaudeModel('inherit') returns true; add an explicit guard in
inferProviderFromModel to return defaultProvider when model === 'inherit' (i.e.,
check model === 'inherit' before calling isClaudeModel) so the configured
defaultProvider isn't overridden, or alternatively update isClaudeModel to
exclude the literal 'inherit' from Claude detection; reference the
inferProviderFromModel, isClaudeModel and defaultProvider symbols when making
the change.

}

export function isModelCompatible(provider: 'claude' | 'codex', model?: string): boolean {
if (!model) return true;
if (provider === 'claude') return isClaudeModel(model);
Expand Down
Loading