From 85cf2c0f4f7fa795583bfbf9b63cc10d48f43e95 Mon Sep 17 00:00:00 2001 From: Kirill Date: Tue, 28 Apr 2026 08:18:06 +0300 Subject: [PATCH 1/2] feat(workflows): support Codex MCP nodes --- bun.lock | 40 ++-- packages/cli/src/commands/workflow.test.ts | 49 ++++- packages/cli/src/commands/workflow.ts | 31 +++- .../core/src/services/title-generator.test.ts | 20 +- packages/core/src/services/title-generator.ts | 5 +- .../docs/guides/authoring-workflows.md | 4 +- .../src/content/docs/guides/mcp-servers.md | 51 +++--- packages/providers/package.json | 3 +- packages/providers/src/claude/index.ts | 8 +- packages/providers/src/claude/provider.ts | 93 +--------- packages/providers/src/codex/capabilities.ts | 2 +- packages/providers/src/codex/provider.test.ts | 153 +++++++++++++++- packages/providers/src/codex/provider.ts | 171 +++++++++++++++++- packages/providers/src/index.ts | 1 + packages/providers/src/mcp/config.ts | 135 ++++++++++++++ packages/providers/src/registry.test.ts | 4 +- packages/workflows/package.json | 1 + packages/workflows/src/dag-executor.test.ts | 18 +- packages/workflows/src/dag-executor.ts | 2 +- packages/workflows/src/loader.test.ts | 16 ++ packages/workflows/src/validator.test.ts | 5 +- 21 files changed, 645 insertions(+), 167 deletions(-) create mode 100644 packages/providers/src/mcp/config.ts diff --git a/bun.lock b/bun.lock index d06d5ccac0..cd7ed7e8b0 100644 --- a/bun.lock +++ b/bun.lock @@ -23,7 +23,7 @@ }, "packages/adapters": { "name": "@archon/adapters", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@archon/core": "workspace:*", "@archon/git": "workspace:*", @@ -41,7 +41,7 @@ }, "packages/cli": { "name": "@archon/cli", - "version": "0.3.6", + "version": "0.3.9", "bin": { "archon": "./src/cli.ts", }, @@ -63,7 +63,7 @@ }, "packages/core": { "name": "@archon/core", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@archon/git": "workspace:*", "@archon/isolation": "workspace:*", @@ -83,7 +83,7 @@ }, "packages/docs-web": { "name": "@archon/docs-web", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@astrojs/starlight": "^0.38.0", "astro": "^6.1.0", @@ -92,7 +92,7 @@ }, "packages/git": { "name": "@archon/git", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@archon/paths": "workspace:*", }, @@ -102,7 +102,7 @@ }, "packages/isolation": { "name": "@archon/isolation", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@archon/git": "workspace:*", "@archon/paths": "workspace:*", @@ -113,7 +113,7 @@ }, "packages/paths": { "name": "@archon/paths", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "dotenv": "^17", "pino": "^9", @@ -126,13 +126,13 @@ }, "packages/providers": { "name": "@archon/providers", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@anthropic-ai/claude-agent-sdk": "^0.2.89", "@archon/paths": "workspace:*", "@mariozechner/pi-ai": "^0.67.5", "@mariozechner/pi-coding-agent": "^0.67.5", - "@openai/codex-sdk": "^0.116.0", + "@openai/codex-sdk": "^0.125.0", "@sinclair/typebox": "^0.34.41", }, "devDependencies": { @@ -144,7 +144,7 @@ }, "packages/server": { "name": "@archon/server", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@archon/adapters": "workspace:*", "@archon/core": "workspace:*", @@ -163,7 +163,7 @@ }, "packages/web": { "name": "@archon/web", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@dagrejs/dagre": "^2.0.4", "@radix-ui/react-alert-dialog": "^1.1.15", @@ -215,7 +215,7 @@ }, "packages/workflows": { "name": "@archon/workflows", - "version": "0.3.6", + "version": "0.3.9", "dependencies": { "@archon/git": "workspace:*", "@archon/paths": "workspace:*", @@ -695,21 +695,21 @@ "@open-draft/until": ["@open-draft/until@2.1.0", "", {}, "sha512-U69T3ItWHvLwGg5eJ0n3I62nWuE6ilHlmz7zM0npLBRvPRd7e6NYmg54vvRtP5mZG7kZqZCFVdsTWo7BPtBujg=="], - "@openai/codex": ["@openai/codex@0.116.0", "", { "optionalDependencies": { "@openai/codex-darwin-arm64": "npm:@openai/codex@0.116.0-darwin-arm64", "@openai/codex-darwin-x64": "npm:@openai/codex@0.116.0-darwin-x64", "@openai/codex-linux-arm64": "npm:@openai/codex@0.116.0-linux-arm64", "@openai/codex-linux-x64": "npm:@openai/codex@0.116.0-linux-x64", "@openai/codex-win32-arm64": "npm:@openai/codex@0.116.0-win32-arm64", "@openai/codex-win32-x64": "npm:@openai/codex@0.116.0-win32-x64" }, "bin": { "codex": "bin/codex.js" } }, "sha512-K6q9P2ZmpnzGmpS6Ybjvsdtvu8AbJx3f/Z4KmjH1u85StSS9TWMSQB8z0PPObKMejbtiIkHwhGyEIHi4iBYjig=="], + "@openai/codex": ["@openai/codex@0.125.0", "", { "optionalDependencies": { "@openai/codex-darwin-arm64": "npm:@openai/codex@0.125.0-darwin-arm64", "@openai/codex-darwin-x64": "npm:@openai/codex@0.125.0-darwin-x64", "@openai/codex-linux-arm64": "npm:@openai/codex@0.125.0-linux-arm64", "@openai/codex-linux-x64": "npm:@openai/codex@0.125.0-linux-x64", "@openai/codex-win32-arm64": "npm:@openai/codex@0.125.0-win32-arm64", "@openai/codex-win32-x64": "npm:@openai/codex@0.125.0-win32-x64" }, "bin": { "codex": "bin/codex.js" } }, "sha512-GiE9wlgL95u/5BRirY5d3EaRLU1tu7Y1R09R8lCHHVmcQdSmhS809FdPDWH3gIYHS7ZriAPqXwJ3aLA0WKl40Q=="], - "@openai/codex-darwin-arm64": ["@openai/codex@0.116.0-darwin-arm64", "", { "os": "darwin", "cpu": "arm64" }, "sha512-WkdL083p8uMeASpg8bwV0DPGgzkm48LjN3MyU2m/YukujbiLnknAmG29O2q2rFCLm0oLSDIGUK8EnXA4ZcAF9Q=="], + "@openai/codex-darwin-arm64": ["@openai/codex@0.125.0-darwin-arm64", "", { "os": "darwin", "cpu": "arm64" }, "sha512-Gn2fHiSO0XgyHp1OSd5DWUTm66Bv9UEuipW5pVEj1E+hWZCOrdqnYttllKFWtRGj5yiKefNX3JIxONgh/ZwlOQ=="], - "@openai/codex-darwin-x64": ["@openai/codex@0.116.0-darwin-x64", "", { "os": "darwin", "cpu": "x64" }, "sha512-Ax8uTwYSNIwGrzcNRcn0jJQhZzNcKGDbbn00Emde7gGOemjSLhRALjUaKjckAaW5xWnNqHTGdtzzPB4phNlDYg=="], + "@openai/codex-darwin-x64": ["@openai/codex@0.125.0-darwin-x64", "", { "os": "darwin", "cpu": "x64" }, "sha512-TZ5Lek2X/UXTI9LXFxzarvQaJeuTrqVh4POc7soO/8RclVnCxADnCf15sivxLd5eiFW4t0myGoeVoM4lciRiRg=="], - "@openai/codex-linux-arm64": ["@openai/codex@0.116.0-linux-arm64", "", { "os": "linux", "cpu": "arm64" }, "sha512-X7cL8rBSGDB+RSZc2FoKiqcMVeLPMmo06bkss/en4lLQsV1XG2DZI56WuXg92IOX3SjYl6Av/eOWgsb1t3UeLQ=="], + "@openai/codex-linux-arm64": ["@openai/codex@0.125.0-linux-arm64", "", { "os": "linux", "cpu": "arm64" }, "sha512-pPnJoJD6rZ2Iin0zNt/up36bO2/EOp2B+1/rPHu/lSq3PJbT3Fmnfut2kJy5LylXb7bGA2XQbtqOogZzIbnlkA=="], - "@openai/codex-linux-x64": ["@openai/codex@0.116.0-linux-x64", "", { "os": "linux", "cpu": "x64" }, "sha512-S9InOgJT3tj6uQp55NqrCA1k5tklwFaH00JdC2ElbRmxchm7ard4WxHSJZX9TiY8enj4cQoLIC04NFTUCO+/PQ=="], + "@openai/codex-linux-x64": ["@openai/codex@0.125.0-linux-x64", "", { "os": "linux", "cpu": "x64" }, "sha512-K2NTTEeBpz/G+N2x17UGWfauRt3So+ir4f+U/60l5PPnYEJB/w3YZrlXo2G9og8Dm9BqtoBAjoPV74sRv9tWWQ=="], - "@openai/codex-sdk": ["@openai/codex-sdk@0.116.0", "", { "dependencies": { "@openai/codex": "0.116.0" } }, "sha512-qrn1Pu5G1GJ9w4m/Lk3L3466ulMGG9SfyR0LPAaXdisuQI1rqgoUOuoZ4byX7cCzn0x1g2+WPc0apZgjMEK04Q=="], + "@openai/codex-sdk": ["@openai/codex-sdk@0.125.0", "", { "dependencies": { "@openai/codex": "0.125.0" } }, "sha512-1xCIHdSbQVF880nJ2aVWdPIsWZbSpKODwuP9y/gvtChDYhYfYEW0DKp2H8ZlctkzIjlzS/WzYmP6ZZPHIvs2Dg=="], - "@openai/codex-win32-arm64": ["@openai/codex@0.116.0-win32-arm64", "", { "os": "win32", "cpu": "arm64" }, "sha512-kX2oAUzkgZX9OsYpd4omv9IGf+9VWj4Vy3UtIAnQKBu1DTSzmTJmXDuDn87mkyUciSZadm2QbeqQQzm2NC0NYw=="], + "@openai/codex-win32-arm64": ["@openai/codex@0.125.0-win32-arm64", "", { "os": "win32", "cpu": "arm64" }, "sha512-zxoUakw9oIHIFrAyk400XkkLBJFA6nOym0NDq6sQ/jhdcYraKqNSRCII2nsBwZHk+/4zgUvuk52iuutgysY/rQ=="], - "@openai/codex-win32-x64": ["@openai/codex@0.116.0-win32-x64", "", { "os": "win32", "cpu": "x64" }, "sha512-6sBIMOoA9FNuxQvCCnK0P548Wqrlk3I9SMdtOCUg2zYzYU7jOF2mWS1VpRQ6R+Jvo2x50dxeJZ+W37dBmXfprw=="], + "@openai/codex-win32-x64": ["@openai/codex@0.125.0-win32-x64", "", { "os": "win32", "cpu": "x64" }, "sha512-ofpOK+OWH5QFuUZ9pTM0d/PcXUXiIP5z5DpRcE9MlucJoyOl4Zy4Nu3NcuHF4YzCkZMQb6x3j0tjDEPHKqNQzw=="], "@oslojs/encoding": ["@oslojs/encoding@1.1.0", "", {}, "sha512-70wQhgYmndg4GCPxPPxPGevRKqTIJ2Nh4OkiMWmDAVYsTQ+Ta7Sq+rPevXyXGdzr30/qZBnyOalCszoMxlyldQ=="], diff --git a/packages/cli/src/commands/workflow.test.ts b/packages/cli/src/commands/workflow.test.ts index 4c80ee3d50..0ece533690 100644 --- a/packages/cli/src/commands/workflow.test.ts +++ b/packages/cli/src/commands/workflow.test.ts @@ -691,7 +691,54 @@ describe('workflowRunCommand', () => { 'hello world', 'claude', '/test/path', - 'assist' + 'assist', + {} + ); + }); + + it('uses the workflow provider for title generation', async () => { + const { discoverWorkflowsWithConfig } = await import('@archon/workflows/workflow-discovery'); + const { executeWorkflow } = await import('@archon/workflows/executor'); + const conversationDb = await import('@archon/core/db/conversations'); + const codebaseDb = await import('@archon/core/db/codebases'); + const core = await import('@archon/core'); + + (discoverWorkflowsWithConfig as ReturnType).mockResolvedValueOnce({ + workflows: [ + makeTestWorkflowWithSource({ + name: 'figma-mcp-smoke', + description: 'Smoke test Figma MCP', + provider: 'codex', + }), + ], + errors: [], + }); + (conversationDb.getOrCreateConversation as ReturnType).mockResolvedValueOnce({ + id: 'conv-123', + ai_assistant_type: 'claude', + }); + (core.loadConfig as ReturnType).mockResolvedValueOnce({ + assistant: 'claude', + assistants: { codex: { model: 'gpt-5.4' } }, + defaults: {}, + }); + (codebaseDb.findCodebaseByDefaultCwd as ReturnType).mockResolvedValueOnce(null); + (conversationDb.updateConversation as ReturnType).mockResolvedValueOnce(undefined); + (executeWorkflow as ReturnType).mockResolvedValueOnce({ + success: true, + workflowRunId: 'run-123', + }); + (core.generateAndSetTitle as ReturnType).mockClear(); + + await workflowRunCommand('/test/path', 'figma-mcp-smoke', 'check figma', { noWorktree: true }); + + expect(core.generateAndSetTitle).toHaveBeenCalledWith( + 'conv-123', + 'check figma', + 'codex', + '/test/path', + 'figma-mcp-smoke', + { model: 'gpt-5.4' } ); }); diff --git a/packages/cli/src/commands/workflow.ts b/packages/cli/src/commands/workflow.ts index bdee2f5398..63faa6a12c 100644 --- a/packages/cli/src/commands/workflow.ts +++ b/packages/cli/src/commands/workflow.ts @@ -14,13 +14,14 @@ import { createLogger, getArchonHome } from '@archon/paths'; import { join } from 'node:path'; import { createWorkflowDeps } from '@archon/core/workflows/store-adapter'; import { discoverWorkflowsWithConfig } from '@archon/workflows/workflow-discovery'; +import { inferProviderFromModel } from '@archon/workflows/model-validation'; import { resolveWorkflowName } from '@archon/workflows/router'; import { executeWorkflow } from '@archon/workflows/executor'; import { getWorkflowEventEmitter, type WorkflowEmitterEvent, } from '@archon/workflows/event-emitter'; -import type { WorkflowLoadResult } from '@archon/workflows/schemas/workflow'; +import type { WorkflowDefinition, WorkflowLoadResult } from '@archon/workflows/schemas/workflow'; import type { WorkflowRun } from '@archon/workflows/schemas/workflow-run'; import { approveWorkflow, @@ -129,6 +130,22 @@ function buildRegistrationFailureError(action: string, error: Error): Error { ); } +/** + * Resolve the provider used for CLI conversation titles from the workflow itself. + * This keeps auxiliary title generation aligned with workflow execution instead + * of falling back to a stale conversation default. + */ +function resolveTitleAssistantType( + workflow: WorkflowDefinition, + defaultAssistant: string | undefined, + conversationAssistant: string | undefined +): string { + const fallbackAssistant = defaultAssistant ?? conversationAssistant ?? 'claude'; + if (workflow.provider) return workflow.provider; + if (workflow.model) return inferProviderFromModel(workflow.model, fallbackAssistant); + return fallbackAssistant; +} + /** Render a workflow event to stderr as a progress line. Called only when --quiet is not set. */ function renderWorkflowEvent(event: WorkflowEmitterEvent, verbose: boolean): void { switch (event.type) { @@ -636,12 +653,20 @@ export async function workflowRunCommand( } // Auto-generate title for CLI workflow conversations (fire-and-forget) + const workflowConfig = await loadConfig(cwd); + const titleAssistantType = resolveTitleAssistantType( + workflow, + workflowConfig.assistant, + conversation.ai_assistant_type + ); + const titleAssistantConfig = workflowConfig.assistants?.[titleAssistantType] ?? {}; void generateAndSetTitle( conversation.id, userMessage, - conversation.ai_assistant_type, + titleAssistantType, workingCwd, - workflowName + workflowName, + titleAssistantConfig ); // Register cleanup handlers for graceful termination diff --git a/packages/core/src/services/title-generator.test.ts b/packages/core/src/services/title-generator.test.ts index 0d85e43c78..8c5268380d 100644 --- a/packages/core/src/services/title-generator.test.ts +++ b/packages/core/src/services/title-generator.test.ts @@ -27,7 +27,7 @@ const mockSendQuery = mock(async function* (): AsyncGenerator { prompt: string, cwd: string, resumeSessionId?: string, - options?: { model?: string; tools?: string[] } + options?: { model?: string; tools?: string[]; assistantConfig?: Record } ) => AsyncGenerator >; @@ -177,6 +177,24 @@ describe('title-generator', () => { expect(optionsArg.nodeConfig?.allowed_tools).toEqual([]); }); + test('passes assistantConfig through to the provider', async () => { + const assistantConfig = { model: 'gpt-5.4', modelReasoningEffort: 'medium' }; + + await generateAndSetTitle( + 'conv-12', + 'Some message', + 'codex', + '/tmp', + 'figma-mcp-smoke', + assistantConfig + ); + + const optionsArg = mockSendQuery.mock.calls[0][3] as { + assistantConfig?: Record; + }; + expect(optionsArg.assistantConfig).toEqual(assistantConfig); + }); + test('handles double failure gracefully (AI fails + fallback DB write fails)', async () => { mockSendQuery.mockImplementation(async function* (): AsyncGenerator { throw new Error('AI failure'); diff --git a/packages/core/src/services/title-generator.ts b/packages/core/src/services/title-generator.ts index 2331a984ef..553b70ba64 100644 --- a/packages/core/src/services/title-generator.ts +++ b/packages/core/src/services/title-generator.ts @@ -29,13 +29,15 @@ const MAX_TITLE_LENGTH = 100; * @param assistantType - Provider identifier (e.g. 'claude', 'codex') * @param cwd - Working directory for the AI client * @param workflowName - Optional workflow name for additional context + * @param assistantConfig - Optional provider-specific defaults for the selected assistant */ export async function generateAndSetTitle( conversationDbId: string, userMessage: string, assistantType: string, cwd: string, - workflowName?: string + workflowName?: string, + assistantConfig?: Record ): Promise { try { getLog().debug({ conversationDbId, assistantType }, 'title.generate_started'); @@ -52,6 +54,7 @@ export async function generateAndSetTitle( for await (const chunk of client.sendQuery(titlePrompt, cwd, undefined, { model: titleModel, + assistantConfig, nodeConfig: { allowed_tools: [] }, // No tool access — pure text generation })) { if (chunk.type === 'assistant') { diff --git a/packages/docs-web/src/content/docs/guides/authoring-workflows.md b/packages/docs-web/src/content/docs/guides/authoring-workflows.md index 2e3f4f9e37..ec3bc91a99 100644 --- a/packages/docs-web/src/content/docs/guides/authoring-workflows.md +++ b/packages/docs-web/src/content/docs/guides/authoring-workflows.md @@ -165,7 +165,7 @@ nodes: provider: claude # Per-node provider override model: haiku # Per-node model override # hooks: # Optional: per-node SDK hook callbacks (Claude only) — see hooks guide - # mcp: .archon/mcp/servers.json # Optional: per-node MCP servers (Claude only) + # mcp: .archon/mcp/servers.json # Optional: per-node MCP servers (Codex and Claude) # skills: [remotion-best-practices] # Optional: per-node skills (Claude only) — see skills guide ``` @@ -1173,7 +1173,7 @@ Before deploying a workflow: 8. **`allowed_tools` / `denied_tools`** — restrict tools per node (Claude only, SDK-enforced) 9. **`retry:`** — auto-retries transient errors (default: 2 retries / 3 total attempts, 3 s backoff); customize per node 10. **`hooks`** — attach SDK hook callbacks to Claude nodes for tool control and context injection -11. **`mcp:`** — attach per-node MCP servers via JSON config (Claude only) +11. **`mcp:`** — attach per-node MCP servers via JSON config (Codex and Claude) 12. **`skills:`** — preload skills into Claude nodes for domain expertise 13. **`agents:`** — inline Claude sub-agent definitions invokable via the `Task` tool 14. **`effort` / `thinking`** — control reasoning depth and thinking mode per node or workflow (Claude only) diff --git a/packages/docs-web/src/content/docs/guides/mcp-servers.md b/packages/docs-web/src/content/docs/guides/mcp-servers.md index c777964d75..4b64bcfb46 100644 --- a/packages/docs-web/src/content/docs/guides/mcp-servers.md +++ b/packages/docs-web/src/content/docs/guides/mcp-servers.md @@ -13,7 +13,8 @@ DAG workflow nodes support a `mcp` field that attaches MCP (Model Context Protoc servers to individual nodes. Each node gets exactly the external tools it needs — GitHub, Linear, Postgres, etc. — without over-provisioning. -**Claude only** — Codex nodes will warn and ignore the `mcp` field. +MCP works with Codex and Claude workflow nodes. Pi nodes still warn and ignore +the `mcp` field. ## Quick Start @@ -50,6 +51,9 @@ to the AI, and it shuts down when the node completes. MCP config files are JSON objects where each key is a server name and the value is a server configuration. Three transport types are supported: +Archon also accepts the common wrapper format `{ "mcpServers": { ... } }`; this +is useful when copying config from tools that already export MCP JSON. + ### stdio (default) Runs a local process. This is the most common type. @@ -119,8 +123,9 @@ Connects to an SSE endpoint. ## Environment Variable Expansion -Values in `env` and `headers` fields support `$VAR_NAME` references that are -expanded from `process.env` at execution time. +Values in `env` and `headers` fields support `$VAR_NAME` references. They are +expanded from Archon's process environment at execution time. Codex workflow +nodes also include codebase-scoped env vars in that expansion. ```json { @@ -165,21 +170,23 @@ A single config file can define multiple servers: } ``` -## Automatic Tool Wildcards +## Provider Tool Wiring -When a node loads MCP servers, tool wildcards are automatically added to `allowedTools`. -For servers named `github` and `postgres`, the node gets: +Claude nodes automatically add tool wildcards to `allowedTools`. For servers +named `github` and `postgres`, the node gets: - `mcp__github__*` - `mcp__postgres__*` -This means all tools from those servers are immediately available without manually -listing them. The wildcards merge with any existing `allowed_tools` on the node. +Codex nodes pass the same MCP config as per-node `mcp_servers` overrides to the +Codex SDK, so the servers are available for that node without requiring global +`~/.codex/config.toml` setup. ## MCP-Only Nodes -Combine `mcp` with `allowed_tools: []` to create nodes that can only use MCP tools -and have no access to built-in tools (Bash, Read, Write, etc.): +For providers that support tool restrictions, combine `mcp` with +`allowed_tools: []` to create nodes that can only use MCP tools and have no +access to built-in tools (Bash, Read, Write, etc.): ```yaml nodes: @@ -190,7 +197,9 @@ nodes: ``` This is useful for sandboxing — the AI can only interact through the MCP server -and cannot touch the filesystem or run shell commands. +and cannot touch the filesystem or run shell commands. Codex currently does not +support Archon's `allowed_tools` / `denied_tools` restrictions, so this pattern +is enforced for Claude nodes but not Codex nodes. ## Connection Failure Handling @@ -205,12 +214,12 @@ MCP server connection failed: github (failed) The node continues executing but without the tools from the failed server. Check your config file path, server command, and environment variables if this happens. -User-level Claude plugin MCPs inherited from `~/.claude/` (e.g. `telegram`, -`notion`) routinely fail to connect inside the headless workflow subprocess -and are **not** surfaced here — they're not actionable for the workflow author. -They appear only in debug logs as `dag.mcp_plugin_connection_suppressed`. Run -the CLI with `--verbose` (or set `LOG_LEVEL=debug` on the server) if you need -to see them. +User-level plugin MCPs inherited from provider-specific user config routinely +fail to connect inside headless workflow subprocesses and are **not** surfaced +when the workflow did not configure MCP itself — they're not actionable for the +workflow author. They appear only in debug logs as +`dag.mcp_plugin_connection_suppressed` for Claude workflows. Run the CLI with +`--verbose` (or set `LOG_LEVEL=debug` on the server) if you need to see them. ## Workflow Examples @@ -368,8 +377,8 @@ bun run cli workflow run archon-smart-pr-review "Review PR #123" ## Limitations -- **Claude only** — Codex nodes warn and ignore the `mcp` field. Configure MCP - servers globally in the Codex CLI config instead. +- **Codex tool restrictions** — Codex nodes support `mcp`, but Archon's + `allowed_tools` / `denied_tools` restrictions are still ignored by Codex. - **Haiku model** — Tool search (lazy loading for many tools) is not supported on Haiku. You'll see a warning. Consider using Sonnet or Opus for MCP nodes. - **No load-time validation** — The MCP config file is read at execution time, not @@ -386,8 +395,8 @@ bun run cli workflow run archon-smart-pr-review "Review PR #123" | `MCP config must be a JSON object` | Top-level value is array or string | Wrap in `{ "server-name": { ... } }` | | `undefined env vars: VAR_NAME` | Environment variable not set | Export the variable or add it to your `.env` | | `MCP server connection failed` | Server process crashed or URL unreachable | Check command/URL, test the server standalone | -| Plugin MCP missing from workflow output | User-level plugin MCPs (from `~/.claude/`) are filtered out of workflow warnings | Run with `--verbose` and look for `dag.mcp_plugin_connection_suppressed` | -| `mcp config but uses Codex` | Node resolved to Codex provider | Set `provider: claude` on the node or switch default | +| Plugin MCP missing from workflow output | User-level plugin MCPs are filtered out of workflow warnings | Run with `--verbose` and look for provider MCP debug logs | +| `allowed_tools` ignored with Codex | Codex provider does not support Archon's tool restrictions yet | Do not rely on `allowed_tools: []` for Codex sandboxing | | `Haiku model with MCP servers` | Haiku doesn't support tool search | Use `model: sonnet` or `model: opus` instead | ## Finding MCP Servers diff --git a/packages/providers/package.json b/packages/providers/package.json index b1e523d2ab..b430ba2b33 100644 --- a/packages/providers/package.json +++ b/packages/providers/package.json @@ -13,6 +13,7 @@ "./codex/provider": "./src/codex/provider.ts", "./codex/config": "./src/codex/config.ts", "./codex/binary-resolver": "./src/codex/binary-resolver.ts", + "./mcp/config": "./src/mcp/config.ts", "./community/pi": "./src/community/pi/index.ts", "./errors": "./src/errors.ts", "./registry": "./src/registry.ts" @@ -26,7 +27,7 @@ "@archon/paths": "workspace:*", "@mariozechner/pi-ai": "^0.67.5", "@mariozechner/pi-coding-agent": "^0.67.5", - "@openai/codex-sdk": "^0.116.0", + "@openai/codex-sdk": "^0.125.0", "@sinclair/typebox": "^0.34.41" }, "devDependencies": { diff --git a/packages/providers/src/claude/index.ts b/packages/providers/src/claude/index.ts index cc540542e4..d54a98b75f 100644 --- a/packages/providers/src/claude/index.ts +++ b/packages/providers/src/claude/index.ts @@ -1,8 +1,4 @@ export { ClaudeProvider } from './provider'; export { parseClaudeConfig, type ClaudeProviderDefaults } from './config'; -export { - loadMcpConfig, - buildSDKHooksFromYAML, - withFirstMessageTimeout, - getProcessUid, -} from './provider'; +export { loadMcpConfig } from '../mcp/config'; +export { buildSDKHooksFromYAML, withFirstMessageTimeout, getProcessUid } from './provider'; diff --git a/packages/providers/src/claude/provider.ts b/packages/providers/src/claude/provider.ts index 5cbef54079..ebd4984244 100644 --- a/packages/providers/src/claude/provider.ts +++ b/packages/providers/src/claude/provider.ts @@ -36,8 +36,7 @@ import { parseClaudeConfig } from './config'; import { CLAUDE_CAPABILITIES } from './capabilities'; import { resolveClaudeBinaryPath } from './binary-resolver'; import { createLogger } from '@archon/paths'; -import { readFile } from 'fs/promises'; -import { resolve, isAbsolute } from 'path'; +import { loadMcpConfig } from '../mcp/config'; /** Lazy-initialized logger (deferred so test mocks can intercept createLogger) */ let cachedLog: ReturnType | undefined; @@ -199,96 +198,6 @@ export function getProcessUid(): number | undefined { return typeof process.getuid === 'function' ? process.getuid() : undefined; } -// ─── MCP Config Loading (absorbed from dag-executor) ─────────────────────── - -/** - * Expand $VAR_NAME references in string-valued records from process.env. - */ -function expandEnvVarsInRecord( - record: Record, - missingVars: string[] -): Record { - const result: Record = {}; - for (const [key, val] of Object.entries(record)) { - if (typeof val !== 'string') { - getLog().warn({ key, valueType: typeof val }, 'mcp_env_value_coerced_to_string'); - result[key] = String(val); - continue; - } - result[key] = val.replace(/\$([A-Z_][A-Z0-9_]*)/g, (_, varName: string) => { - const envVal = process.env[varName]; - if (envVal === undefined) { - missingVars.push(varName); - } - return envVal ?? ''; - }); - } - return result; -} - -function expandEnvVars(config: Record): { - expanded: Record; - missingVars: string[]; -} { - const result: Record = {}; - const missingVars: string[] = []; - for (const [serverName, serverConfig] of Object.entries(config)) { - if (typeof serverConfig !== 'object' || serverConfig === null) { - getLog().warn({ serverName, valueType: typeof serverConfig }, 'mcp_server_config_not_object'); - continue; - } - const server = { ...(serverConfig as Record) }; - if (server.env && typeof server.env === 'object') { - server.env = expandEnvVarsInRecord(server.env as Record, missingVars); - } - if (server.headers && typeof server.headers === 'object') { - server.headers = expandEnvVarsInRecord( - server.headers as Record, - missingVars - ); - } - result[serverName] = server; - } - return { expanded: result, missingVars }; -} - -/** - * Load MCP server config from a JSON file and expand environment variables. - */ -export async function loadMcpConfig( - mcpPath: string, - cwd: string -): Promise<{ servers: Record; serverNames: string[]; missingVars: string[] }> { - const fullPath = isAbsolute(mcpPath) ? mcpPath : resolve(cwd, mcpPath); - - let raw: string; - try { - raw = await readFile(fullPath, 'utf-8'); - } catch (err) { - const e = err as NodeJS.ErrnoException; - if (e.code === 'ENOENT') { - throw new Error(`MCP config file not found: ${mcpPath} (resolved to ${fullPath})`); - } - throw new Error(`Failed to read MCP config file: ${mcpPath} — ${e.message}`); - } - - let parsed: Record; - try { - parsed = JSON.parse(raw) as Record; - } catch (parseErr) { - const detail = (parseErr as SyntaxError).message; - throw new Error(`MCP config file is not valid JSON: ${mcpPath} — ${detail}`); - } - - if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { - throw new Error(`MCP config must be a JSON object (Record): ${mcpPath}`); - } - - const { expanded, missingVars } = expandEnvVars(parsed); - const serverNames = Object.keys(expanded); - return { servers: expanded, serverNames, missingVars }; -} - // ─── SDK Hooks Building (absorbed from dag-executor) ─────────────────────── /** YAML hook matcher shape (matches @archon/workflows/schemas/dag-node WorkflowNodeHooks) */ diff --git a/packages/providers/src/codex/capabilities.ts b/packages/providers/src/codex/capabilities.ts index 9b179e2170..7366a5962c 100644 --- a/packages/providers/src/codex/capabilities.ts +++ b/packages/providers/src/codex/capabilities.ts @@ -2,7 +2,7 @@ import type { ProviderCapabilities } from '../types'; export const CODEX_CAPABILITIES: ProviderCapabilities = { sessionResume: true, - mcp: false, + mcp: true, hooks: false, skills: false, agents: false, diff --git a/packages/providers/src/codex/provider.test.ts b/packages/providers/src/codex/provider.test.ts index 669826ebc3..9a703061a9 100644 --- a/packages/providers/src/codex/provider.test.ts +++ b/packages/providers/src/codex/provider.test.ts @@ -1,4 +1,7 @@ import { describe, test, expect, mock, beforeEach } from 'bun:test'; +import { mkdtemp, rm, writeFile } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; import { createMockLogger } from '../test/mocks/logger'; const mockLogger = createMockLogger(); @@ -72,7 +75,7 @@ describe('CodexProvider', () => { const caps = client.getCapabilities(); expect(caps).toEqual({ sessionResume: true, - mcp: false, + mcp: true, hooks: false, skills: false, agents: false, @@ -783,6 +786,118 @@ describe('CodexProvider', () => { } }); + test('passes workflow MCP config as Codex mcp_servers overrides', async () => { + const testDir = await mkdtemp(join(tmpdir(), 'codex-provider-mcp-')); + const originalToken = process.env.ARCHON_CODEX_MCP_TOKEN; + process.env.ARCHON_CODEX_MCP_TOKEN = 'token-from-process'; + + try { + await writeFile( + join(testDir, 'mcp.json'), + JSON.stringify({ + figma: { + type: 'http', + url: 'http://127.0.0.1:3845/mcp', + headers: { Authorization: 'Bearer $ARCHON_CODEX_MCP_TOKEN' }, + startup_timeout_sec: 20, + }, + local: { + type: 'stdio', + command: 'npx', + args: ['-y', 'figma-mcp'], + env: { TOKEN: '$ARCHON_CODEX_MCP_TOKEN' }, + }, + }) + ); + + mockRunStreamed.mockResolvedValue({ + events: (async function* () { + yield { type: 'turn.completed', usage: defaultUsage }; + })(), + }); + + for await (const _ of client.sendQuery('test prompt', testDir, undefined, { + nodeConfig: { mcp: 'mcp.json' }, + })) { + // consume + } + + expect(MockCodex).toHaveBeenCalledWith( + expect.objectContaining({ + config: { + mcp_servers: { + figma: { + url: 'http://127.0.0.1:3845/mcp', + http_headers: { Authorization: 'Bearer token-from-process' }, + startup_timeout_sec: 20, + }, + local: { + command: 'npx', + args: ['-y', 'figma-mcp'], + env: { TOKEN: 'token-from-process' }, + }, + }, + }, + }) + ); + expect(mockLogger.info).toHaveBeenCalledWith( + { serverNames: ['figma', 'local'], mcpPath: 'mcp.json' }, + 'codex.mcp_config_loaded' + ); + } finally { + if (originalToken === undefined) { + delete process.env.ARCHON_CODEX_MCP_TOKEN; + } else { + process.env.ARCHON_CODEX_MCP_TOKEN = originalToken; + } + await rm(testDir, { recursive: true, force: true }); + } + }); + + test('uses request env when expanding workflow MCP config variables', async () => { + const testDir = await mkdtemp(join(tmpdir(), 'codex-provider-mcp-env-')); + + try { + await writeFile( + join(testDir, 'mcp.json'), + JSON.stringify({ + figma: { + command: 'figma-mcp', + env: { TOKEN: '$FIGMA_TOKEN' }, + }, + }) + ); + + mockRunStreamed.mockResolvedValue({ + events: (async function* () { + yield { type: 'turn.completed', usage: defaultUsage }; + })(), + }); + + for await (const _ of client.sendQuery('test prompt', testDir, undefined, { + env: { FIGMA_TOKEN: 'from-codebase-env' }, + nodeConfig: { mcp: 'mcp.json' }, + })) { + // consume + } + + expect(MockCodex).toHaveBeenCalledWith( + expect.objectContaining({ + config: { + mcp_servers: { + figma: { + command: 'figma-mcp', + env: { TOKEN: 'from-codebase-env' }, + }, + }, + }, + }) + ); + } finally { + await rm(testDir, { recursive: true, force: true }); + } + }); + test('reuses the singleton Codex instance across sequential calls without env', async () => { mockRunStreamed.mockResolvedValue({ events: (async function* () { @@ -918,6 +1033,42 @@ describe('CodexProvider', () => { ); }); + test('surfaces MCP client errors when workflow MCP is configured', async () => { + const testDir = await mkdtemp(join(tmpdir(), 'codex-provider-mcp-error-')); + + try { + await writeFile( + join(testDir, 'mcp.json'), + JSON.stringify({ figma: { command: 'figma-mcp' } }) + ); + mockRunStreamed.mockResolvedValue({ + events: (async function* () { + yield { type: 'error', message: 'MCP client connection timeout' }; + yield { type: 'turn.completed', usage: defaultUsage }; + })(), + }); + + const chunks = []; + for await (const chunk of client.sendQuery('test', testDir, undefined, { + nodeConfig: { mcp: 'mcp.json' }, + })) { + chunks.push(chunk); + } + + expect(chunks[0]).toEqual({ + type: 'system', + content: '\u26A0\uFE0F MCP client connection timeout', + }); + expect(chunks[1]).toEqual({ + type: 'result', + sessionId: 'new-thread-id', + tokens: { input: 10, output: 5 }, + }); + } finally { + await rm(testDir, { recursive: true, force: true }); + } + }); + test('handles turn.failed events', async () => { mockRunStreamed.mockResolvedValue({ events: (async function* () { diff --git a/packages/providers/src/codex/provider.ts b/packages/providers/src/codex/provider.ts index b9e1d493e9..ef63b4951f 100644 --- a/packages/providers/src/codex/provider.ts +++ b/packages/providers/src/codex/provider.ts @@ -4,6 +4,7 @@ */ import { Codex, + type CodexOptions, type ThreadOptions, type TurnOptions, type TurnCompletedEvent, @@ -19,6 +20,7 @@ import { parseCodexConfig } from './config'; import { CODEX_CAPABILITIES } from './capabilities'; import { resolveCodexBinaryPath } from './binary-resolver'; import { createLogger } from '@archon/paths'; +import { loadMcpConfig } from '../mcp/config'; /** Lazy-initialized logger (deferred so test mocks can intercept createLogger) */ let cachedLog: ReturnType | undefined; @@ -27,6 +29,14 @@ function getLog(): ReturnType { return cachedLog; } +type CodexConfigOverrides = NonNullable; +type CodexConfigValue = CodexConfigOverrides[string]; + +interface ProviderWarning { + code: string; + message: string; +} + // Singleton Codex instance (async because binary path resolution is async) let codexInstance: Codex | null = null; let codexInitPromise: Promise | null = null; @@ -87,6 +97,113 @@ function buildCodexEnv(requestEnv: Record): Record +): Record { + return requestEnv ? { ...process.env, ...requestEnv } : process.env; +} + +const CODEX_MCP_PASSTHROUGH_KEYS = [ + 'command', + 'args', + 'env', + 'url', + 'enabled', + 'required', + 'startup_timeout_sec', + 'startup_timeout_ms', + 'tool_timeout_sec', + 'enabled_tools', + 'disabled_tools', + 'supports_parallel_tool_calls', + 'cwd', + 'env_vars', + 'experimental_environment', + 'http_headers', + 'env_http_headers', + 'oauth_resource', + 'scopes', + 'bearer_token_env_var', + 'default_tools_approval_mode', + 'tools', +] as const; + +function toCodexConfigValue(value: unknown): CodexConfigValue | undefined { + if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') { + return value; + } + + if (Array.isArray(value)) { + const result: CodexConfigValue[] = []; + for (const item of value) { + const converted = toCodexConfigValue(item); + if (converted !== undefined) result.push(converted); + } + return result; + } + + if (typeof value === 'object' && value !== null) { + const result: CodexConfigOverrides = {}; + for (const [key, nestedValue] of Object.entries(value)) { + const converted = toCodexConfigValue(nestedValue); + if (converted !== undefined) result[key] = converted; + } + return result; + } + + return undefined; +} + +function setCodexConfigValue(target: CodexConfigOverrides, key: string, value: unknown): void { + const converted = toCodexConfigValue(value); + if (converted !== undefined) { + target[key] = converted; + } +} + +function convertMcpServerConfigForCodex( + serverConfig: Record +): CodexConfigOverrides { + const result: CodexConfigOverrides = {}; + + for (const key of CODEX_MCP_PASSTHROUGH_KEYS) { + if (key in serverConfig) { + setCodexConfigValue(result, key, serverConfig[key]); + } + } + + // Archon's MCP JSON format uses `headers`; Codex config uses `http_headers`. + if ('headers' in serverConfig && !('http_headers' in result)) { + setCodexConfigValue(result, 'http_headers', serverConfig.headers); + } + + return result; +} + +function buildCodexMcpConfigOverrides( + servers: Record +): CodexConfigOverrides | undefined { + const mcpServers: CodexConfigOverrides = {}; + + for (const [serverName, serverConfig] of Object.entries(servers)) { + if (typeof serverConfig !== 'object' || serverConfig === null || Array.isArray(serverConfig)) { + getLog().warn( + { serverName, valueType: typeof serverConfig }, + 'codex.mcp_server_config_not_object' + ); + continue; + } + + const converted = convertMcpServerConfigForCodex(serverConfig as Record); + if (Object.keys(converted).length > 0) { + mcpServers[serverName] = converted; + } + } + + if (Object.keys(mcpServers).length === 0) return undefined; + return { mcp_servers: mcpServers }; +} + const CODEX_MODEL_FALLBACKS: Record = { 'gpt-5.3-codex': 'gpt-5.2-codex', }; @@ -191,7 +308,8 @@ async function* streamCodexEvents( events: AsyncIterable>, hasOutputFormat: boolean, threadId: string | null | undefined, - abortSignal?: AbortSignal + abortSignal?: AbortSignal, + surfaceMcpClientErrors = false ): AsyncGenerator { const state: CodexStreamState = {}; let accumulatedText = ''; @@ -213,7 +331,7 @@ async function* streamCodexEvents( if (event.type === 'error') { const errorEvent = event as { message: string }; getLog().error({ message: errorEvent.message }, 'stream_error'); - if (!errorEvent.message.includes('MCP client')) { + if (surfaceMcpClientErrors || !errorEvent.message.includes('MCP client')) { yield { type: 'system', content: `⚠️ ${errorEvent.message}` }; } continue; @@ -476,17 +594,22 @@ export class CodexProvider implements IAgentProvider { private async createCodexClient( configCodexBinaryPath: string | undefined, - requestEnv?: Record + requestEnv?: Record, + codexConfigOverrides?: CodexConfigOverrides ): Promise { - if (!requestEnv || Object.keys(requestEnv).length === 0) { + if ((!requestEnv || Object.keys(requestEnv).length === 0) && !codexConfigOverrides) { return getCodex(configCodexBinaryPath); } try { - return new Codex({ + const codexOptions: CodexOptions = { codexPathOverride: await resolveCodexBinaryPath(configCodexBinaryPath), - env: buildCodexEnv(requestEnv), - }); + ...(requestEnv && Object.keys(requestEnv).length > 0 + ? { env: buildCodexEnv(requestEnv) } + : {}), + ...(codexConfigOverrides ? { config: codexConfigOverrides } : {}), + }; + return new Codex(codexOptions); } catch (error) { const err = error as Error; if (isModelAccessError(err.message)) { @@ -508,9 +631,38 @@ export class CodexProvider implements IAgentProvider { ): AsyncGenerator { const assistantConfig = requestOptions?.assistantConfig ?? {}; const codexConfig = parseCodexConfig(assistantConfig); + const providerWarnings: ProviderWarning[] = []; + let codexConfigOverrides: CodexConfigOverrides | undefined; + + if (requestOptions?.nodeConfig?.mcp) { + const mcpPath = requestOptions.nodeConfig.mcp; + const { servers, serverNames, missingVars } = await loadMcpConfig( + mcpPath, + cwd, + buildMcpEnvSource(requestOptions.env) + ); + codexConfigOverrides = buildCodexMcpConfigOverrides(servers); + getLog().info({ serverNames, mcpPath }, 'codex.mcp_config_loaded'); + if (missingVars.length > 0) { + const uniqueVars = [...new Set(missingVars)]; + getLog().warn({ missingVars: uniqueVars }, 'codex.mcp_env_vars_missing'); + providerWarnings.push({ + code: 'mcp_env_vars_missing', + message: `MCP config references undefined env vars: ${uniqueVars.join(', ')}. These will be empty strings - MCP servers may fail to authenticate.`, + }); + } + } + + for (const warning of providerWarnings) { + yield { type: 'system', content: `Warning: ${warning.message}` }; + } // 1. Initialize SDK and build thread options - const codex = await this.createCodexClient(codexConfig.codexBinaryPath, requestOptions?.env); + const codex = await this.createCodexClient( + codexConfig.codexBinaryPath, + requestOptions?.env, + codexConfigOverrides + ); const threadOptions = buildThreadOptions(cwd, requestOptions?.model, assistantConfig); if (requestOptions?.abortSignal?.aborted) { @@ -588,7 +740,8 @@ export class CodexProvider implements IAgentProvider { result.events as AsyncIterable>, hasOutputFormat, thread.id, - requestOptions?.abortSignal + requestOptions?.abortSignal, + Boolean(requestOptions?.nodeConfig?.mcp) ); return; } catch (error) { diff --git a/packages/providers/src/index.ts b/packages/providers/src/index.ts index d430f8d402..246309cc37 100644 --- a/packages/providers/src/index.ts +++ b/packages/providers/src/index.ts @@ -43,6 +43,7 @@ export { parseCodexConfig, type CodexProviderDefaults } from './codex/config'; // Utilities (needed by consumers) export { resetCodexSingleton } from './codex/provider'; +export { loadMcpConfig, type LoadedMcpConfig } from './mcp/config'; export { resolveCodexBinaryPath, fileExists as codexFileExists } from './codex/binary-resolver'; export { resolveClaudeBinaryPath, fileExists as claudeFileExists } from './claude/binary-resolver'; diff --git a/packages/providers/src/mcp/config.ts b/packages/providers/src/mcp/config.ts new file mode 100644 index 0000000000..a40efa7248 --- /dev/null +++ b/packages/providers/src/mcp/config.ts @@ -0,0 +1,135 @@ +import { createLogger } from '@archon/paths'; +import { readFile } from 'fs/promises'; +import { isAbsolute, resolve } from 'path'; + +/** Lazy-initialized logger. */ +let cachedLog: ReturnType | undefined; +function getLog(): ReturnType { + if (!cachedLog) cachedLog = createLogger('provider.mcp'); + return cachedLog; +} + +type EnvSource = Record; + +export interface LoadedMcpConfig { + servers: Record; + serverNames: string[]; + missingVars: string[]; +} + +/** + * Expand $VAR_NAME references in string-valued records from the supplied + * environment source. + */ +function expandEnvVarsInRecord( + record: Record, + missingVars: string[], + envSource: EnvSource +): Record { + const result: Record = {}; + for (const [key, val] of Object.entries(record)) { + if (typeof val !== 'string') { + getLog().warn({ key, valueType: typeof val }, 'mcp_env_value_coerced_to_string'); + result[key] = String(val); + continue; + } + result[key] = val.replace(/\$([A-Z_][A-Z0-9_]*)/g, (_, varName: string) => { + const envVal = envSource[varName]; + if (envVal === undefined) { + missingVars.push(varName); + } + return envVal ?? ''; + }); + } + return result; +} + +function expandEnvVars( + config: Record, + envSource: EnvSource +): { + expanded: Record; + missingVars: string[]; +} { + const result: Record = {}; + const missingVars: string[] = []; + for (const [serverName, serverConfig] of Object.entries(config)) { + if (typeof serverConfig !== 'object' || serverConfig === null) { + getLog().warn({ serverName, valueType: typeof serverConfig }, 'mcp_server_config_not_object'); + continue; + } + const server = { ...(serverConfig as Record) }; + if (server.env && typeof server.env === 'object') { + server.env = expandEnvVarsInRecord( + server.env as Record, + missingVars, + envSource + ); + } + if (server.headers && typeof server.headers === 'object') { + server.headers = expandEnvVarsInRecord( + server.headers as Record, + missingVars, + envSource + ); + } + result[serverName] = server; + } + return { expanded: result, missingVars }; +} + +function normalizeMcpConfig( + parsed: Record, + mcpPath: string +): Record { + const keys = Object.keys(parsed); + if (keys.length !== 1 || keys[0] !== 'mcpServers') { + return parsed; + } + + const servers = parsed.mcpServers; + if (typeof servers !== 'object' || servers === null || Array.isArray(servers)) { + throw new Error(`MCP config field "mcpServers" must be a JSON object: ${mcpPath}`); + } + + return servers as Record; +} + +/** + * Load MCP server config from a JSON file and expand environment variables. + */ +export async function loadMcpConfig( + mcpPath: string, + cwd: string, + envSource: EnvSource = process.env +): Promise { + const fullPath = isAbsolute(mcpPath) ? mcpPath : resolve(cwd, mcpPath); + + let raw: string; + try { + raw = await readFile(fullPath, 'utf-8'); + } catch (err) { + const e = err as NodeJS.ErrnoException; + if (e.code === 'ENOENT') { + throw new Error(`MCP config file not found: ${mcpPath} (resolved to ${fullPath})`); + } + throw new Error(`Failed to read MCP config file: ${mcpPath} - ${e.message}`); + } + + let parsed: Record; + try { + parsed = JSON.parse(raw) as Record; + } catch (parseErr) { + const detail = (parseErr as SyntaxError).message; + throw new Error(`MCP config file is not valid JSON: ${mcpPath} - ${detail}`); + } + + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error(`MCP config must be a JSON object (Record): ${mcpPath}`); + } + + const normalized = normalizeMcpConfig(parsed, mcpPath); + const { expanded, missingVars } = expandEnvVars(normalized, envSource); + const serverNames = Object.keys(expanded); + return { servers: expanded, serverNames, missingVars }; +} diff --git a/packages/providers/src/registry.test.ts b/packages/providers/src/registry.test.ts index 64b879a91c..fcfc65f7e4 100644 --- a/packages/providers/src/registry.test.ts +++ b/packages/providers/src/registry.test.ts @@ -113,7 +113,7 @@ describe('registry', () => { const codexCaps = codex.getCapabilities(); expect(claudeCaps.mcp).toBe(true); - expect(codexCaps.mcp).toBe(false); + expect(codexCaps.mcp).toBe(true); expect(claudeCaps.hooks).toBe(true); expect(codexCaps.hooks).toBe(false); }); @@ -129,7 +129,7 @@ describe('registry', () => { test('returns Codex capabilities without instantiation', () => { const caps = getProviderCapabilities('codex'); - expect(caps.mcp).toBe(false); + expect(caps.mcp).toBe(true); expect(caps.hooks).toBe(false); expect(caps.envInjection).toBe(true); }); diff --git a/packages/workflows/package.json b/packages/workflows/package.json index 568e36966c..60db946d36 100644 --- a/packages/workflows/package.json +++ b/packages/workflows/package.json @@ -11,6 +11,7 @@ "./deps": "./src/deps.ts", "./event-emitter": "./src/event-emitter.ts", "./workflow-discovery": "./src/workflow-discovery.ts", + "./model-validation": "./src/model-validation.ts", "./script-discovery": "./src/script-discovery.ts", "./command-validation": "./src/command-validation.ts", "./defaults": "./src/defaults/bundled-defaults.ts", diff --git a/packages/workflows/src/dag-executor.test.ts b/packages/workflows/src/dag-executor.test.ts index 6ae086b3bb..7457fca567 100644 --- a/packages/workflows/src/dag-executor.test.ts +++ b/packages/workflows/src/dag-executor.test.ts @@ -23,7 +23,12 @@ mock.module('@archon/paths', () => ({ if (folder) paths.unshift(folder); return paths; }, + getWorkflowFolderSearchPaths: () => ['.archon/workflows'], getDefaultCommandsPath: () => '/nonexistent/defaults', + getDefaultWorkflowsPath: () => '/nonexistent/defaults/workflows', + getHomeWorkflowsPath: () => '/nonexistent/home/workflows', + getLegacyHomeWorkflowsPath: () => '/nonexistent/home/.archon/workflows', + getArchonHome: () => '/nonexistent/home', })); // --- Bootstrap provider registry (after path mocks, before dag-executor import) --- @@ -38,7 +43,7 @@ import { substituteNodeOutputRefs, executeDagWorkflow, } from './dag-executor'; -import { loadMcpConfig } from '@archon/providers/claude/provider'; +import { loadMcpConfig } from '@archon/providers/mcp/config'; import type { DagNode, BashNode, ScriptNode, NodeOutput, WorkflowRun } from './schemas'; import { discoverWorkflows } from './workflow-discovery'; import { parseWorkflow } from './loader'; @@ -118,7 +123,7 @@ const mockClaudeCapabilities = () => ({ /** Limited capabilities for Codex mock */ const mockCodexCapabilities = () => ({ sessionResume: true, - mcp: false, + mcp: true, hooks: false, skills: false, agents: false, @@ -2190,6 +2195,15 @@ describe('loadMcpConfig', () => { expect(result.missingVars).toEqual([]); }); + it('loads standard mcpServers-wrapped config JSON', async () => { + const servers = { figma: { url: 'http://127.0.0.1:3845/mcp' } }; + await writeFile(join(testDir, 'wrapped.json'), JSON.stringify({ mcpServers: servers })); + + const result = await loadMcpConfig('wrapped.json', testDir); + expect(result.serverNames).toEqual(['figma']); + expect(result.servers).toEqual(servers); + }); + it('loads multiple servers from one config', async () => { const config = { github: { command: 'npx', args: ['-y', '@mcp/server-github'] }, diff --git a/packages/workflows/src/dag-executor.ts b/packages/workflows/src/dag-executor.ts index 3ba9824566..c0134aac35 100644 --- a/packages/workflows/src/dag-executor.ts +++ b/packages/workflows/src/dag-executor.ts @@ -316,7 +316,7 @@ export function substituteNodeOutputRefs( } // buildSDKHooksFromYAML moved to @archon/providers/src/claude/provider.ts -// loadMcpConfig moved to @archon/providers/src/claude/provider.ts +// loadMcpConfig moved to @archon/providers/src/mcp/config.ts /** * Resolve per-node provider and model. diff --git a/packages/workflows/src/loader.test.ts b/packages/workflows/src/loader.test.ts index 7b0be0bebd..37e924c393 100644 --- a/packages/workflows/src/loader.test.ts +++ b/packages/workflows/src/loader.test.ts @@ -39,11 +39,17 @@ import * as bundledDefaults from './defaults/bundled-defaults'; describe('Workflow Loader', () => { let testDir: string; + const originalArchonHome = process.env.ARCHON_HOME; + const originalArchonDocker = process.env.ARCHON_DOCKER; beforeEach(async () => { // Create unique temp directory for each test testDir = join(tmpdir(), `workflow-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); await mkdir(testDir, { recursive: true }); + process.env.ARCHON_HOME = join(testDir, 'home'); + delete process.env.ARCHON_DOCKER; + const { resetLegacyHomeWarningForTests } = await import('./workflow-discovery'); + resetLegacyHomeWarningForTests(); }); afterEach(async () => { @@ -53,6 +59,16 @@ describe('Workflow Loader', () => { } catch { // Ignore cleanup errors } + if (originalArchonHome === undefined) { + delete process.env.ARCHON_HOME; + } else { + process.env.ARCHON_HOME = originalArchonHome; + } + if (originalArchonDocker === undefined) { + delete process.env.ARCHON_DOCKER; + } else { + process.env.ARCHON_DOCKER = originalArchonDocker; + } }); describe('parseWorkflow (via discoverWorkflows)', () => { diff --git a/packages/workflows/src/validator.test.ts b/packages/workflows/src/validator.test.ts index bd2b418e17..67b288b0e2 100644 --- a/packages/workflows/src/validator.test.ts +++ b/packages/workflows/src/validator.test.ts @@ -212,7 +212,7 @@ describe('validateWorkflowResources — MCP validation', () => { expect(mcpErrors).toHaveLength(0); }); - test('warns when MCP used with codex provider', async () => { + test('does not warn when MCP is used with codex provider', async () => { const mcpPath = join(tmpDir, 'good.json'); await writeFile(mcpPath, '{"server": {"command": "npx"}}'); const workflow = makeWorkflow( @@ -222,8 +222,7 @@ describe('validateWorkflowResources — MCP validation', () => { ); const issues = await validateWorkflowResources(workflow, tmpDir); const mcpWarnings = issues.filter(i => i.field === 'mcp' && i.level === 'warning'); - expect(mcpWarnings).toHaveLength(1); - expect(mcpWarnings[0].message).toContain('not supported by provider'); + expect(mcpWarnings).toHaveLength(0); }); }); From c5a34ce5577ee6fb12b3eeeea53a6bc2ab583fe5 Mon Sep 17 00:00:00 2001 From: Kirill Date: Tue, 28 Apr 2026 10:27:37 +0300 Subject: [PATCH 2/2] fix: address MCP workflow review feedback --- packages/cli/src/commands/workflow.ts | 45 ++++++++----- .../docs/guides/authoring-workflows.md | 2 +- .../src/content/docs/guides/mcp-servers.md | 2 +- packages/providers/src/codex/provider.test.ts | 67 ++++++++++++++----- packages/providers/src/codex/provider.ts | 5 +- packages/providers/src/mcp/config.ts | 62 +++++++++++------ packages/workflows/src/dag-executor.test.ts | 48 +++++++++++++ 7 files changed, 177 insertions(+), 54 deletions(-) diff --git a/packages/cli/src/commands/workflow.ts b/packages/cli/src/commands/workflow.ts index 63faa6a12c..9637d41a7d 100644 --- a/packages/cli/src/commands/workflow.ts +++ b/packages/cli/src/commands/workflow.ts @@ -653,21 +653,36 @@ export async function workflowRunCommand( } // Auto-generate title for CLI workflow conversations (fire-and-forget) - const workflowConfig = await loadConfig(cwd); - const titleAssistantType = resolveTitleAssistantType( - workflow, - workflowConfig.assistant, - conversation.ai_assistant_type - ); - const titleAssistantConfig = workflowConfig.assistants?.[titleAssistantType] ?? {}; - void generateAndSetTitle( - conversation.id, - userMessage, - titleAssistantType, - workingCwd, - workflowName, - titleAssistantConfig - ); + void (async (): Promise => { + let workflowConfig: Awaited> | undefined; + try { + workflowConfig = await loadConfig(cwd); + } catch (error) { + getLog().warn({ err: error as Error, cwd }, 'workflow.title_config_load_failed'); + } + + try { + const titleAssistantType = resolveTitleAssistantType( + workflow, + workflowConfig?.assistant, + conversation.ai_assistant_type + ); + const titleAssistantConfig = workflowConfig?.assistants?.[titleAssistantType] ?? {}; + await generateAndSetTitle( + conversation.id, + userMessage, + titleAssistantType, + workingCwd, + workflowName, + titleAssistantConfig + ); + } catch (error) { + getLog().warn( + { err: error as Error, conversationId: conversation.id }, + 'workflow.title_generation_failed' + ); + } + })(); // Register cleanup handlers for graceful termination let terminating = false; diff --git a/packages/docs-web/src/content/docs/guides/authoring-workflows.md b/packages/docs-web/src/content/docs/guides/authoring-workflows.md index ec3bc91a99..3ccb48f9c6 100644 --- a/packages/docs-web/src/content/docs/guides/authoring-workflows.md +++ b/packages/docs-web/src/content/docs/guides/authoring-workflows.md @@ -205,7 +205,7 @@ nodes: | `allowed_tools` | string[] | — | Whitelist of built-in tools. `[]` = no tools. Claude only | | `denied_tools` | string[] | — | Tools to remove. Applied after `allowed_tools`. Claude only | | `hooks` | object | — | Per-node SDK hook callbacks. Claude only. See [Hooks](/guides/hooks/) | -| `mcp` | string | — | Path to MCP server config JSON file. Claude only. See [MCP Servers](/guides/mcp-servers/) | +| `mcp` | string | — | Path to MCP server config JSON file. Codex and Claude. See [MCP Servers](/guides/mcp-servers/) | | `skills` | string[] | — | Skills to preload. Claude only. See [Skills](/guides/skills/) | | `agents` | object | — | Inline sub-agent definitions keyed by kebab-case ID. Claude only. See [Inline sub-agents](#inline-sub-agents) | | `effort` | `'low'`\|`'medium'`\|`'high'`\|`'max'` | — | Reasoning depth. Claude only. Also settable at workflow level | diff --git a/packages/docs-web/src/content/docs/guides/mcp-servers.md b/packages/docs-web/src/content/docs/guides/mcp-servers.md index 4b64bcfb46..26111b4594 100644 --- a/packages/docs-web/src/content/docs/guides/mcp-servers.md +++ b/packages/docs-web/src/content/docs/guides/mcp-servers.md @@ -172,7 +172,7 @@ A single config file can define multiple servers: ## Provider Tool Wiring -Claude nodes automatically add tool wildcards to `allowedTools`. For servers +Claude nodes automatically add tool wildcards to `allowed_tools`. For servers named `github` and `postgres`, the node gets: - `mcp__github__*` diff --git a/packages/providers/src/codex/provider.test.ts b/packages/providers/src/codex/provider.test.ts index 9a703061a9..dbd34a60f9 100644 --- a/packages/providers/src/codex/provider.test.ts +++ b/packages/providers/src/codex/provider.test.ts @@ -824,20 +824,20 @@ describe('CodexProvider', () => { expect(MockCodex).toHaveBeenCalledWith( expect.objectContaining({ - config: { - mcp_servers: { - figma: { + config: expect.objectContaining({ + mcp_servers: expect.objectContaining({ + figma: expect.objectContaining({ url: 'http://127.0.0.1:3845/mcp', http_headers: { Authorization: 'Bearer token-from-process' }, startup_timeout_sec: 20, - }, - local: { + }), + local: expect.objectContaining({ command: 'npx', args: ['-y', 'figma-mcp'], env: { TOKEN: 'token-from-process' }, - }, - }, - }, + }), + }), + }), }) ); expect(mockLogger.info).toHaveBeenCalledWith( @@ -883,16 +883,53 @@ describe('CodexProvider', () => { expect(MockCodex).toHaveBeenCalledWith( expect.objectContaining({ - config: { - mcp_servers: { - figma: { + config: expect.objectContaining({ + mcp_servers: expect.objectContaining({ + figma: expect.objectContaining({ command: 'figma-mcp', env: { TOKEN: 'from-codebase-env' }, - }, - }, + }), + }), + }), + }) + ); + } finally { + await rm(testDir, { recursive: true, force: true }); + } + }); + + test('prefixes workflow MCP warnings for workflow forwarding', async () => { + const testDir = await mkdtemp(join(tmpdir(), 'codex-provider-mcp-warning-')); + delete process.env.ARCHON_CODEX_MISSING_TOKEN; + + try { + await writeFile( + join(testDir, 'mcp.json'), + JSON.stringify({ + figma: { + command: 'figma-mcp', + env: { TOKEN: '$ARCHON_CODEX_MISSING_TOKEN' }, }, }) ); + mockRunStreamed.mockResolvedValue({ + events: (async function* () { + yield { type: 'turn.completed', usage: defaultUsage }; + })(), + }); + + const chunks = []; + for await (const chunk of client.sendQuery('test prompt', testDir, undefined, { + nodeConfig: { mcp: 'mcp.json' }, + })) { + chunks.push(chunk); + } + + expect(chunks[0]).toEqual({ + type: 'system', + content: + '⚠️ MCP config references undefined env vars: ARCHON_CODEX_MISSING_TOKEN. These will be empty strings - MCP servers may fail to authenticate.', + }); } finally { await rm(testDir, { recursive: true, force: true }); } @@ -1008,7 +1045,7 @@ describe('CodexProvider', () => { test('suppresses MCP timeout errors', async () => { mockRunStreamed.mockResolvedValue({ events: (async function* () { - yield { type: 'error', message: 'MCP client connection timeout' }; + yield { type: 'error', message: 'mcp client connection timeout' }; yield { type: 'turn.completed', usage: defaultUsage }; })(), }); @@ -1028,7 +1065,7 @@ describe('CodexProvider', () => { // Error is still logged even though not sent to user expect(mockLogger.error).toHaveBeenCalledWith( - { message: 'MCP client connection timeout' }, + { message: 'mcp client connection timeout' }, 'stream_error' ); }); diff --git a/packages/providers/src/codex/provider.ts b/packages/providers/src/codex/provider.ts index ef63b4951f..5fc508691a 100644 --- a/packages/providers/src/codex/provider.ts +++ b/packages/providers/src/codex/provider.ts @@ -331,7 +331,8 @@ async function* streamCodexEvents( if (event.type === 'error') { const errorEvent = event as { message: string }; getLog().error({ message: errorEvent.message }, 'stream_error'); - if (surfaceMcpClientErrors || !errorEvent.message.includes('MCP client')) { + const isMcpClientError = errorEvent.message.toLowerCase().includes('mcp client'); + if (surfaceMcpClientErrors || !isMcpClientError) { yield { type: 'system', content: `⚠️ ${errorEvent.message}` }; } continue; @@ -654,7 +655,7 @@ export class CodexProvider implements IAgentProvider { } for (const warning of providerWarnings) { - yield { type: 'system', content: `Warning: ${warning.message}` }; + yield { type: 'system', content: `⚠️ ${warning.message}` }; } // 1. Initialize SDK and build thread options diff --git a/packages/providers/src/mcp/config.ts b/packages/providers/src/mcp/config.ts index a40efa7248..f69f09cad3 100644 --- a/packages/providers/src/mcp/config.ts +++ b/packages/providers/src/mcp/config.ts @@ -1,14 +1,6 @@ -import { createLogger } from '@archon/paths'; import { readFile } from 'fs/promises'; import { isAbsolute, resolve } from 'path'; -/** Lazy-initialized logger. */ -let cachedLog: ReturnType | undefined; -function getLog(): ReturnType { - if (!cachedLog) cachedLog = createLogger('provider.mcp'); - return cachedLog; -} - type EnvSource = Record; export interface LoadedMcpConfig { @@ -17,6 +9,12 @@ export interface LoadedMcpConfig { missingVars: string[]; } +function describeJsonType(value: unknown): string { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'array'; + return typeof value; +} + /** * Expand $VAR_NAME references in string-valued records from the supplied * environment source. @@ -24,14 +22,15 @@ export interface LoadedMcpConfig { function expandEnvVarsInRecord( record: Record, missingVars: string[], - envSource: EnvSource + envSource: EnvSource, + fieldPath: string ): Record { const result: Record = {}; for (const [key, val] of Object.entries(record)) { if (typeof val !== 'string') { - getLog().warn({ key, valueType: typeof val }, 'mcp_env_value_coerced_to_string'); - result[key] = String(val); - continue; + throw new Error( + `MCP config ${fieldPath}.${key} must be a string (got ${describeJsonType(val)})` + ); } result[key] = val.replace(/\$([A-Z_][A-Z0-9_]*)/g, (_, varName: string) => { const envVal = envSource[varName]; @@ -54,23 +53,40 @@ function expandEnvVars( const result: Record = {}; const missingVars: string[] = []; for (const [serverName, serverConfig] of Object.entries(config)) { - if (typeof serverConfig !== 'object' || serverConfig === null) { - getLog().warn({ serverName, valueType: typeof serverConfig }, 'mcp_server_config_not_object'); - continue; + if (typeof serverConfig !== 'object' || serverConfig === null || Array.isArray(serverConfig)) { + throw new Error( + `MCP server "${serverName}" must be a JSON object (got ${describeJsonType(serverConfig)})` + ); } const server = { ...(serverConfig as Record) }; - if (server.env && typeof server.env === 'object') { + if (server.env !== undefined) { + if (typeof server.env !== 'object' || server.env === null || Array.isArray(server.env)) { + throw new Error( + `MCP config ${serverName}.env must be a JSON object of string values (got ${describeJsonType(server.env)})` + ); + } server.env = expandEnvVarsInRecord( server.env as Record, missingVars, - envSource + envSource, + `${serverName}.env` ); } - if (server.headers && typeof server.headers === 'object') { + if (server.headers !== undefined) { + if ( + typeof server.headers !== 'object' || + server.headers === null || + Array.isArray(server.headers) + ) { + throw new Error( + `MCP config ${serverName}.headers must be a JSON object of string values (got ${describeJsonType(server.headers)})` + ); + } server.headers = expandEnvVarsInRecord( server.headers as Record, missingVars, - envSource + envSource, + `${serverName}.headers` ); } result[serverName] = server; @@ -83,10 +99,16 @@ function normalizeMcpConfig( mcpPath: string ): Record { const keys = Object.keys(parsed); - if (keys.length !== 1 || keys[0] !== 'mcpServers') { + if (!keys.includes('mcpServers')) { return parsed; } + if (keys.length > 1) { + throw new Error( + `MCP config cannot mix top-level "mcpServers" with other keys: ${mcpPath}. Use either a direct server map or { "mcpServers": { ... } }.` + ); + } + const servers = parsed.mcpServers; if (typeof servers !== 'object' || servers === null || Array.isArray(servers)) { throw new Error(`MCP config field "mcpServers" must be a JSON object: ${mcpPath}`); diff --git a/packages/workflows/src/dag-executor.test.ts b/packages/workflows/src/dag-executor.test.ts index 7457fca567..da5d6c9d87 100644 --- a/packages/workflows/src/dag-executor.test.ts +++ b/packages/workflows/src/dag-executor.test.ts @@ -2204,6 +2204,18 @@ describe('loadMcpConfig', () => { expect(result.servers).toEqual(servers); }); + it('rejects mixed mcpServers wrapper and top-level metadata', async () => { + const servers = { figma: { url: 'http://127.0.0.1:3845/mcp' } }; + await writeFile( + join(testDir, 'mixed-wrapper.json'), + JSON.stringify({ $schema: 'https://example.com/schema.json', mcpServers: servers }) + ); + + await expect(loadMcpConfig('mixed-wrapper.json', testDir)).rejects.toThrow( + 'cannot mix top-level "mcpServers" with other keys' + ); + }); + it('loads multiple servers from one config', async () => { const config = { github: { command: 'npx', args: ['-y', '@mcp/server-github'] }, @@ -2298,6 +2310,42 @@ describe('loadMcpConfig', () => { await writeFile(join(testDir, 'str.json'), '"hello"'); await expect(loadMcpConfig('str.json', testDir)).rejects.toThrow('must be a JSON object'); }); + + it('throws on array-valued server config', async () => { + await writeFile(join(testDir, 'server-array.json'), JSON.stringify({ figma: [] })); + + await expect(loadMcpConfig('server-array.json', testDir)).rejects.toThrow( + 'MCP server "figma" must be a JSON object' + ); + }); + + it('throws on non-string env values', async () => { + await writeFile( + join(testDir, 'env-number.json'), + JSON.stringify({ figma: { command: 'figma-mcp', env: { TOKEN: 123 } } }) + ); + + await expect(loadMcpConfig('env-number.json', testDir)).rejects.toThrow( + 'MCP config figma.env.TOKEN must be a string' + ); + }); + + it('throws on non-string header values', async () => { + await writeFile( + join(testDir, 'header-array.json'), + JSON.stringify({ + figma: { + type: 'http', + url: 'http://127.0.0.1:3845/mcp', + headers: { Authorization: ['Bearer token'] }, + }, + }) + ); + + await expect(loadMcpConfig('header-array.json', testDir)).rejects.toThrow( + 'MCP config figma.headers.Authorization must be a string' + ); + }); }); // ---------------------------------------------------------------------------