feat(workflows): support Codex MCP nodes#1456
Closed
borshyo wants to merge 1 commit intocoleam00:devfrom
Closed
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughCentralizes MCP JSON loading into a shared module, adds MCP support and per-node MCP passthrough to the Codex provider (including error surfacing and env precedence), updates exports/tests/docs, and removes the Claude-local MCP loader in favor of the shared loader. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,220,255,0.5)
participant User
participant WorkflowRunner as Workflow
participant Provider as Codex Provider
participant MCP as MCP Loader
participant CodexSDK as Codex SDK
end
User->>Workflow: trigger node execution (nodeConfig.mcp)
Workflow->>Provider: sendQuery(..., nodeConfig)
Provider->>MCP: loadMcpConfig(mcpPath, cwd, envSource)
MCP-->>Provider: servers, serverNames, missingVars
Provider->>CodexSDK: create client with config + mcp_servers
CodexSDK-->>Provider: client stream / errors
Provider->>Workflow: emit result chunks
Provider->>Workflow: emit MCP client errors as system chunks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 858e8a9958
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| for (const warning of providerWarnings) { | ||
| yield { type: 'system', content: `Warning: ${warning.message}` }; |
There was a problem hiding this comment.
The new Codex MCP warning chunks are emitted as Warning: ... here, but workflow execution only forwards provider system messages that start with ⚠️ (or the MCP failure prefix) in packages/workflows/src/dag-executor.ts (msg.content.startsWith('⚠️') branch). As a result, MCP env-missing warnings introduced in this change are silently dropped during workflow runs, which hides actionable misconfiguration and makes MCP auth failures hard to diagnose.
Useful? React with 👍 / 👎.
Comment on lines
+85
to
+87
| const keys = Object.keys(parsed); | ||
| if (keys.length !== 1 || keys[0] !== 'mcpServers') { | ||
| return parsed; |
There was a problem hiding this comment.
This wrapper detection only normalizes when the top-level object is exactly { "mcpServers": ... }; if a copied config includes metadata keys (for example $schema alongside mcpServers), normalization is skipped and providers treat mcpServers as a server name instead of the server map. In Codex this can drop all MCP servers from mcp_servers overrides, so workflows run without the intended MCP tools despite a seemingly valid wrapped config.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/providers/src/codex/provider.test.ts (1)
789-899: Make the MCP config assertions less brittle.These checks pin the entire
configobject, so they’ll start failing if Codex gains unrelated per-call options alongsidemcp_servers. A nestedexpect.objectContainingaround the MCP payload would keep the test focused on the behavior you care about.Suggested adjustment
- 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(MockCodex).toHaveBeenCalledWith( + expect.objectContaining({ + config: expect.objectContaining({ + mcp_servers: expect.objectContaining({ + figma: expect.objectContaining({ + url: 'http://127.0.0.1:3845/mcp', + }), + local: expect.objectContaining({ + command: 'npx', + }), + }), + }), + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/codex/provider.test.ts` around lines 789 - 899, The assertions pin the entire config object; update the two MockCodex expectations so they only assert the mcp_servers portion by nesting expect.objectContaining: replace expect.objectContaining({ config: { mcp_servers: { ... } } }) with expect.objectContaining({ config: expect.objectContaining({ mcp_servers: expect.objectContaining({ /* server entries like figma/local or figma only */ }) }) }) and likewise use expect.objectContaining for the inner server entries (figma, local) in both tests to avoid brittleness when Codex adds other per-call options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Around line 1176-1177: Update the Node Fields table row that currently
contains the "Claude only" note to reflect the new behavior: change the
annotation for the mcp field (the table row referencing "mcp" / "MCP") so it
indicates MCP servers can be attached to both Codex and Claude nodes (and keep
the "skills" note as only for Claude if that remains true); locate the table row
containing the literal "Claude only" and replace that text with a short,
consistent note such as "Codex and Claude" or otherwise match the summary
bullets for `mcp:` and `skills:` so the table no longer contradicts the summary.
In `@packages/docs-web/src/content/docs/guides/mcp-servers.md`:
- Around line 175-176: The doc uses the internal camelCase field name
allowedTools but the guide is user-facing and elsewhere uses the YAML-facing
allowed_tools; update the text and any examples to use allowed_tools instead of
allowedTools (search for occurrences of allowedTools and replace with
allowed_tools) and ensure the surrounding explanation and sample YAML reflect
the snake_case user-facing field name consistently.
In `@packages/providers/src/codex/provider.ts`:
- Around line 334-335: The current filter uses a case-sensitive check
errorEvent.message.includes('MCP client'), causing misses for variants like "Mcp
client" or "mcp client"; update the conditional in the error-emitting block so
it does a case-insensitive match (e.g., compare lowercased strings or use a
case-insensitive regex) when deciding whether to suppress MCP client messages,
referencing the surfaceMcpClientErrors flag and errorEvent.message to preserve
existing behavior.
In `@packages/providers/src/mcp/config.ts`:
- Around line 81-96: normalizeMcpConfig currently unwraps a single-key {
"mcpServers": ... } but silently allows mixed formats; change it to explicitly
reject any config that contains "mcpServers" alongside any other top-level keys
(e.g., throw an Error if keys.includes('mcpServers') && keys.length > 1) so
mixed wrapper-and-metadata formats fail fast; keep the existing behavior for the
pure single-key wrapper (keys.length === 1 && keys[0] === 'mcpServers') and for
configs that do not include 'mcpServers' at all, and mention/consider downstream
callers like expandEnvVars when forming the error message to make debugging
clear.
---
Nitpick comments:
In `@packages/providers/src/codex/provider.test.ts`:
- Around line 789-899: The assertions pin the entire config object; update the
two MockCodex expectations so they only assert the mcp_servers portion by
nesting expect.objectContaining: replace expect.objectContaining({ config: {
mcp_servers: { ... } } }) with expect.objectContaining({ config:
expect.objectContaining({ mcp_servers: expect.objectContaining({ /* server
entries like figma/local or figma only */ }) }) }) and likewise use
expect.objectContaining for the inner server entries (figma, local) in both
tests to avoid brittleness when Codex adds other per-call options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f974646-842d-4651-a22c-df0a41d3e9bf
📒 Files selected for processing (15)
packages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/guides/mcp-servers.mdpackages/providers/package.jsonpackages/providers/src/claude/index.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/capabilities.tspackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/index.tspackages/providers/src/mcp/config.tspackages/providers/src/registry.test.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/loader.test.tspackages/workflows/src/validator.test.ts
Comment on lines
+175
to
+176
| Claude nodes automatically add tool wildcards to `allowedTools`. For servers | ||
| named `github` and `postgres`, the node gets: |
There was a problem hiding this comment.
Use user-facing field name in docs (allowed_tools)
Line 175 references allowedTools (internal casing), but this guide is workflow-author facing and otherwise uses allowed_tools. Keeping one form avoids confusion in YAML authoring.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~176-~176: The official name of this software platform is spelled with a capital “H”.
Context: ...ds to allowedTools. For servers named github and postgres, the node gets: - `mcp...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs-web/src/content/docs/guides/mcp-servers.md` around lines 175 -
176, The doc uses the internal camelCase field name allowedTools but the guide
is user-facing and elsewhere uses the YAML-facing allowed_tools; update the text
and any examples to use allowed_tools instead of allowedTools (search for
occurrences of allowedTools and replace with allowed_tools) and ensure the
surrounding explanation and sample YAML reflect the snake_case user-facing field
name consistently.
Comment on lines
+334
to
335
| if (surfaceMcpClientErrors || !errorEvent.message.includes('MCP client')) { | ||
| yield { type: 'system', content: `⚠️ ${errorEvent.message}` }; |
There was a problem hiding this comment.
Make MCP-client error filtering case-insensitive
Line 334 uses a case-sensitive includes('MCP client'). If the SDK emits variants like Mcp client / mcp client, filtering behavior becomes inconsistent.
Suggested fix
- if (surfaceMcpClientErrors || !errorEvent.message.includes('MCP client')) {
+ const isMcpClientError = errorEvent.message.toLowerCase().includes('mcp client');
+ if (surfaceMcpClientErrors || !isMcpClientError) {
yield { type: 'system', content: `⚠️ ${errorEvent.message}` };
}📝 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
| if (surfaceMcpClientErrors || !errorEvent.message.includes('MCP client')) { | |
| yield { type: 'system', content: `⚠️ ${errorEvent.message}` }; | |
| const isMcpClientError = errorEvent.message.toLowerCase().includes('mcp client'); | |
| if (surfaceMcpClientErrors || !isMcpClientError) { | |
| yield { type: 'system', content: `⚠️ ${errorEvent.message}` }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/provider.ts` around lines 334 - 335, The current
filter uses a case-sensitive check errorEvent.message.includes('MCP client'),
causing misses for variants like "Mcp client" or "mcp client"; update the
conditional in the error-emitting block so it does a case-insensitive match
(e.g., compare lowercased strings or use a case-insensitive regex) when deciding
whether to suppress MCP client messages, referencing the surfaceMcpClientErrors
flag and errorEvent.message to preserve existing behavior.
Comment on lines
+81
to
+96
| function normalizeMcpConfig( | ||
| parsed: Record<string, unknown>, | ||
| mcpPath: string | ||
| ): Record<string, unknown> { | ||
| 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<string, unknown>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to understand context
cat -n packages/providers/src/mcp/config.ts | head -120Repository: coleam00/Archon
Length of output: 4461
🏁 Script executed:
# Search for tests related to normalizeMcpConfig
find . -type f -name "*.test.ts" -o -name "*.spec.ts" | xargs rg "normalizeMcpConfig" -lRepository: coleam00/Archon
Length of output: 43
🏁 Script executed:
# Look for any MCP specification or documentation
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs rg -l "mcpServers\|MCP config" 2>/dev/null | head -10Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Search for all usages of normalizeMcpConfig
rg "normalizeMcpConfig" --type ts --type tsx -B 3 -A 3Repository: coleam00/Archon
Length of output: 87
🏁 Script executed:
# Look for any MCP-related documentation or schema files
fd -e json -e schema | xargs rg -l "mcpServers" 2>/dev/nullRepository: coleam00/Archon
Length of output: 43
🏁 Script executed:
# Continue reading the config.ts file to see complete context
cat -n packages/providers/src/mcp/config.ts | sed -n '96,160p'Repository: coleam00/Archon
Length of output: 1678
🏁 Script executed:
# Search for test files in the mcp directory
find packages/providers/src/mcp -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) 2>/dev/nullRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Search for usages of normalizeMcpConfig with correct syntax
rg "normalizeMcpConfig" -t ts -B 3 -A 3 2>/dev/nullRepository: coleam00/Archon
Length of output: 1078
🏁 Script executed:
# Check if there are any example or documentation files
find packages/providers -type f -name "*.json" -o -name "*.md" | head -20Repository: coleam00/Archon
Length of output: 124
🏁 Script executed:
# Look for MCP-related documentation or examples
rg "mcpServers|MCP config" --type ts -B 2 -A 2 | head -100Repository: coleam00/Archon
Length of output: 7186
🏁 Script executed:
# Look at the test cases more closely to understand intended behavior
cat packages/workflows/src/dag-executor.test.ts | grep -A 15 "loads standard mcpServers-wrapped"Repository: coleam00/Archon
Length of output: 766
🏁 Script executed:
# Search for any comments or documentation about MCP config format
rg "mcpServers|MCP.*config.*format|MCP.*structure" packages/providers/src/mcp/ -B 2 -A 2Repository: coleam00/Archon
Length of output: 823
🏁 Script executed:
# Check if there are examples or documentation about MCP config shape
find . -name "*.md" -o -name "README*" | xargs rg -l "mcp" 2>/dev/null | head -5Repository: coleam00/Archon
Length of output: 343
🏁 Script executed:
# Look at how the loaded config is actually used after normalization
rg "expanded\|servers" packages/providers/src/mcp/config.ts -B 2 -A 2Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Search for any validator or schema that might define expected MCP structure
find packages -name "*mcp*" -type f | head -20Repository: coleam00/Archon
Length of output: 116
🏁 Script executed:
# Read the MCP servers documentation
cat packages/docs-web/src/content/docs/guides/mcp-servers.mdRepository: coleam00/Archon
Length of output: 13041
🏁 Script executed:
# Look for examples in the docs about configuration format
rg "mcpServers|{.*command" packages/docs-web/src/content/docs/guides/mcp-servers.md -B 2 -A 5Repository: coleam00/Archon
Length of output: 167
🏁 Script executed:
# Check if there are example config files in the repository
find . -name "*.json" -path "*mcp*" -o -name "*.json" -path "*example*" | head -10Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Look for any tests that specifically test the normalizeMcpConfig behavior with edge cases
rg -A 30 "normalizeMcpConfig|loads standard mcpServers" packages/workflows/src/dag-executor.test.tsRepository: coleam00/Archon
Length of output: 1420
🏁 Script executed:
# Let's see what happens downstream when expandEnvVars processes a mixed object
cat -n packages/providers/src/mcp/config.ts | sed -n '47,79p'Repository: coleam00/Archon
Length of output: 1352
🏁 Script executed:
# Check the full flow to see if there's error handling for malformed server configs
cat -n packages/providers/src/mcp/config.ts | sed -n '24,45p'Repository: coleam00/Archon
Length of output: 889
🏁 Script executed:
# Let's check what the actual behavior would be with a mixed format
# by examining if there are any tests covering this edge case
rg -A 20 "test.*normalizeMcp|it.*wrapper|it.*mixed" packages/workflows/src/dag-executor.test.tsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Check if there's any validation elsewhere that rejects mixed formats
rg "mcpServers" packages/ -B 3 -A 3 --type ts | grep -A 5 -B 5 "metadata\|additional\|extra"Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Let me verify the exact behavior with a practical scenario
# Check if there are any real-world examples or edge case tests
rg "mcpServers" packages/workflows/src/dag-executor.test.ts -B 5 -A 10Repository: coleam00/Archon
Length of output: 831
🏁 Script executed:
# Check if there are any comments explaining the strict behavior
rg -B 5 -A 5 "keys.length !== 1|only.*top-level" packages/providers/src/mcp/config.tsRepository: coleam00/Archon
Length of output: 405
🏁 Script executed:
# Look at the docstring for normalizeMcpConfig
cat -n packages/providers/src/mcp/config.ts | sed -n '75,96p'Repository: coleam00/Archon
Length of output: 789
🏁 Script executed:
# Let me verify the actual handling by simulating the mixed format scenario
python3 << 'EOF'
# Simulate what happens with mixed format: { mcpServers: {...}, metadata: "foo" }
mixed = { "mcpServers": { "github": { "command": "npx" } }, "metadata": "extra" }
keys = list(mixed.keys())
print(f"keys.length = {len(keys)}")
print(f"keys[0] = {keys[0]}")
print(f"Unwrap condition (len == 1 AND key == 'mcpServers'): {len(keys) == 1 and keys[0] == 'mcpServers'}")
print(f"Would return as-is: {not (len(keys) == 1 and keys[0] == 'mcpServers')}")
print(f"\nAfter returning as-is, expandEnvVars would iterate:")
for key, val in mixed.items():
is_obj = isinstance(val, dict)
print(f" key='{key}', value_is_object={is_obj}, would_skip={not is_obj}")
EOFRepository: coleam00/Archon
Length of output: 349
Consider explicitly rejecting mixed wrapper-and-metadata formats to fail fast.
While documentation only specifies two supported formats (direct server map or single-key { "mcpServers": ... } wrapper), the current code silently mishandles mixed formats like { "mcpServers": {...}, "metadata": "..." } by treating the wrapper itself as a server name in expandEnvVars(). This violates the guideline to "prefer throwing early with a clear error for unsupported or unsafe states." Either validate that the unwrapped config is being used correctly, or explicitly reject configurations with extraneous top-level keys alongside mcpServers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/mcp/config.ts` around lines 81 - 96,
normalizeMcpConfig currently unwraps a single-key { "mcpServers": ... } but
silently allows mixed formats; change it to explicitly reject any config that
contains "mcpServers" alongside any other top-level keys (e.g., throw an Error
if keys.includes('mcpServers') && keys.length > 1) so mixed wrapper-and-metadata
formats fail fast; keep the existing behavior for the pure single-key wrapper
(keys.length === 1 && keys[0] === 'mcpServers') and for configs that do not
include 'mcpServers' at all, and mention/consider downstream callers like
expandEnvVars when forming the error message to make debugging clear.
borshyo
force-pushed
the
codex/codex-mcp-workflows
branch
from
858e8a9 to
3d4e767
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/docs-web/src/content/docs/guides/mcp-servers.md (1)
175-176:⚠️ Potential issue | 🟡 MinorUse the user-facing field name (
allowed_tools) in docs.Line 175 currently uses
allowedTools, which conflicts with the YAML-facing name used elsewhere in this guide.Suggested fix
-Claude nodes automatically add tool wildcards to `allowedTools`. For servers +Claude nodes automatically add tool wildcards to `allowed_tools`. For servers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs-web/src/content/docs/guides/mcp-servers.md` around lines 175 - 176, The docs line uses the code-facing identifier allowedTools but should use the user-facing YAML name allowed_tools; update the sentence that currently reads "Claude nodes automatically add tool wildcards to `allowedTools`" (the surrounding example mentioning servers named `github` and `postgres`) to use `allowed_tools` instead so terminology matches the rest of the guide.packages/providers/src/codex/provider.ts (1)
334-335:⚠️ Potential issue | 🟡 MinorMake MCP-client error filtering case-insensitive.
Line 334 uses a case-sensitive match, so variants like
mcp client/Mcp clientcan bypass intended filtering behavior.Suggested fix
- if (surfaceMcpClientErrors || !errorEvent.message.includes('MCP client')) { + const isMcpClientError = errorEvent.message.toLowerCase().includes('mcp client'); + if (surfaceMcpClientErrors || !isMcpClientError) { yield { type: 'system', content: `⚠️ ${errorEvent.message}` }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/codex/provider.ts` around lines 334 - 335, The current conditional in the Codex provider uses a case-sensitive check errorEvent.message.includes('MCP client'), which lets variants like "mcp client" bypass filtering; update the check used in the if block that references surfaceMcpClientErrors and errorEvent.message so it performs a case-insensitive match (e.g., compare errorEvent.message.toLowerCase().includes('mcp client') or use a case-insensitive regex) and ensure you guard against a missing/undefined message before calling toLowerCase(); keep the rest of the logic (yielding system message) unchanged.packages/providers/src/mcp/config.ts (1)
85-96:⚠️ Potential issue | 🟠 MajorReject mixed top-level
mcpServers+ sibling keys explicitly.If
mcpServersappears alongside other top-level keys, the loader currently treats the file as a direct map and can misinterpret the wrapper itself as a server entry instead of failing fast.As per coding guidelines: "Prefer throwing early with a clear error for unsupported or unsafe states; never silently swallow errors (Fail Fast + Explicit Errors)".Suggested fix
function normalizeMcpConfig( parsed: Record<string, unknown>, mcpPath: string ): Record<string, unknown> { const keys = Object.keys(parsed); + if (keys.includes('mcpServers') && keys.length > 1) { + throw new Error( + `MCP config cannot mix top-level "mcpServers" with other keys: ${mcpPath}` + ); + } if (keys.length !== 1 || keys[0] !== 'mcpServers') { return parsed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/mcp/config.ts` around lines 85 - 96, The loader currently returns parsed when keys.length !== 1 || keys[0] !== 'mcpServers', which allows files that contain mcpServers plus other top-level keys to be treated as valid; change the logic to detect the mixed-case and throw instead: if keys.includes('mcpServers') && keys.length > 1 throw a clear Error mentioning mcpServers and mcpPath; otherwise keep the existing branches (return parsed for unrelated structures, and later validate parsed.mcpServers as an object). Reference symbols: parsed, keys, mcpServers, mcpPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/workflow.ts`:
- Around line 655-670: The change made title generation a blocking step by
awaiting loadConfig(cwd); instead restore fire-and-forget behavior by moving
config loading and title generation into an unawaited async task that catches
and swallows errors: create an inline async wrapper (or helper) that calls
loadConfig, resolveTitleAssistantType, determines titleAssistantConfig, then
calls generateAndSetTitle(conversation.id, ...), and invoke it without await;
ensure any exceptions from loadConfig or generateAndSetTitle are caught/logged
but do not propagate to the main workflowRunCommand path.
In `@packages/providers/src/mcp/config.ts`:
- Around line 31-34: The current branch in the config parsing logic (where val
and key are handled) should not coerce non-string env/headers values; replace
the getLog().warn + String(val) coercion with an explicit thrown error that
includes the key name and the actual typeof val (e.g., throw new Error(`Invalid
config: expected string for ${key}, got ${typeof val}`)); update the code path
around result[key] assignment in the same function so invalid types immediately
fail rather than being coerced, and ensure any callers of this config loader
will surface that error (preserve existing getLog() usage only for context, not
for swallowing the error).
- Around line 57-60: The current check in config.ts allows arrays because typeof
returns "object"; update the validation for serverConfig (use
Array.isArray(serverConfig)) and treat arrays as invalid by throwing a clear
error (include serverName and the actual value/type) instead of
warning/continuing; replace or augment the block that currently calls
getLog().warn({ serverName, valueType: typeof serverConfig },
'mcp_server_config_not_object') so that when serverConfig is null, not an
object, or an array you throw a descriptive Error describing the unsupported
array-valued server config for serverName.
---
Duplicate comments:
In `@packages/docs-web/src/content/docs/guides/mcp-servers.md`:
- Around line 175-176: The docs line uses the code-facing identifier
allowedTools but should use the user-facing YAML name allowed_tools; update the
sentence that currently reads "Claude nodes automatically add tool wildcards to
`allowedTools`" (the surrounding example mentioning servers named `github` and
`postgres`) to use `allowed_tools` instead so terminology matches the rest of
the guide.
In `@packages/providers/src/codex/provider.ts`:
- Around line 334-335: The current conditional in the Codex provider uses a
case-sensitive check errorEvent.message.includes('MCP client'), which lets
variants like "mcp client" bypass filtering; update the check used in the if
block that references surfaceMcpClientErrors and errorEvent.message so it
performs a case-insensitive match (e.g., compare
errorEvent.message.toLowerCase().includes('mcp client') or use a
case-insensitive regex) and ensure you guard against a missing/undefined message
before calling toLowerCase(); keep the rest of the logic (yielding system
message) unchanged.
In `@packages/providers/src/mcp/config.ts`:
- Around line 85-96: The loader currently returns parsed when keys.length !== 1
|| keys[0] !== 'mcpServers', which allows files that contain mcpServers plus
other top-level keys to be treated as valid; change the logic to detect the
mixed-case and throw instead: if keys.includes('mcpServers') && keys.length > 1
throw a clear Error mentioning mcpServers and mcpPath; otherwise keep the
existing branches (return parsed for unrelated structures, and later validate
parsed.mcpServers as an object). Reference symbols: parsed, keys, mcpServers,
mcpPath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b00d546-63b8-4492-a936-c4ff1c886d8a
📒 Files selected for processing (20)
packages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.tspackages/core/src/services/title-generator.test.tspackages/core/src/services/title-generator.tspackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/guides/mcp-servers.mdpackages/providers/package.jsonpackages/providers/src/claude/index.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/capabilities.tspackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/index.tspackages/providers/src/mcp/config.tspackages/providers/src/registry.test.tspackages/workflows/package.jsonpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/loader.test.tspackages/workflows/src/validator.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/workflows/package.json
- packages/providers/src/codex/capabilities.ts
- packages/providers/src/index.ts
- packages/providers/package.json
- packages/workflows/src/dag-executor.ts
- packages/providers/src/claude/index.ts
- packages/docs-web/src/content/docs/guides/authoring-workflows.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/workflows/src/validator.test.ts
- packages/providers/src/registry.test.ts
- packages/workflows/src/dag-executor.test.ts
- packages/workflows/src/loader.test.ts
- packages/providers/src/codex/provider.test.ts
Comment on lines
655
to
670
| // 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 | ||
| ); |
There was a problem hiding this comment.
Don't make title generation a hard dependency of workflowRunCommand.
loadConfig(cwd) now runs on the main execution path, so a missing or malformed .archon/config.yaml will abort the workflow before it even starts. That regresses the previous fire-and-forget behavior for titles.
Suggested fix
- const workflowConfig = await loadConfig(cwd);
- const titleAssistantType = resolveTitleAssistantType(
- workflow,
- workflowConfig.assistant,
- conversation.ai_assistant_type
- );
- const titleAssistantConfig = workflowConfig.assistants?.[titleAssistantType] ?? {};
+ let workflowConfig;
+ try {
+ workflowConfig = await loadConfig(cwd);
+ } catch (error) {
+ getLog().warn({ err: error as Error, cwd }, 'workflow.title_config_load_failed');
+ }
+
+ const titleAssistantType = resolveTitleAssistantType(
+ workflow,
+ workflowConfig?.assistant,
+ conversation.ai_assistant_type
+ );
+ const titleAssistantConfig = workflowConfig?.assistants?.[titleAssistantType] ?? {};📝 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
| // 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 | |
| ); | |
| // Auto-generate title for CLI workflow conversations (fire-and-forget) | |
| let workflowConfig; | |
| try { | |
| workflowConfig = await loadConfig(cwd); | |
| } catch (error) { | |
| getLog().warn({ err: error as Error, cwd }, 'workflow.title_config_load_failed'); | |
| } | |
| const titleAssistantType = resolveTitleAssistantType( | |
| workflow, | |
| workflowConfig?.assistant, | |
| conversation.ai_assistant_type | |
| ); | |
| const titleAssistantConfig = workflowConfig?.assistants?.[titleAssistantType] ?? {}; | |
| void generateAndSetTitle( | |
| conversation.id, | |
| userMessage, | |
| titleAssistantType, | |
| workingCwd, | |
| workflowName, | |
| titleAssistantConfig | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/workflow.ts` around lines 655 - 670, The change
made title generation a blocking step by awaiting loadConfig(cwd); instead
restore fire-and-forget behavior by moving config loading and title generation
into an unawaited async task that catches and swallows errors: create an inline
async wrapper (or helper) that calls loadConfig, resolveTitleAssistantType,
determines titleAssistantConfig, then calls generateAndSetTitle(conversation.id,
...), and invoke it without await; ensure any exceptions from loadConfig or
generateAndSetTitle are caught/logged but do not propagate to the main
workflowRunCommand path.
Comment on lines
+31
to
+34
| if (typeof val !== 'string') { | ||
| getLog().warn({ key, valueType: typeof val }, 'mcp_env_value_coerced_to_string'); | ||
| result[key] = String(val); | ||
| continue; |
There was a problem hiding this comment.
Do not coerce non-string env/headers values; fail with a clear error.
String coercion here hides invalid config and can turn malformed secrets/headers into unusable values without surfacing a hard failure.
Suggested fix
function expandEnvVarsInRecord(
record: Record<string, unknown>,
missingVars: string[],
- envSource: EnvSource
+ envSource: EnvSource,
+ fieldPath: string
): Record<string, string> {
const result: Record<string, string> = {};
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`);
}- server.env = expandEnvVarsInRecord(
+ server.env = expandEnvVarsInRecord(
server.env as Record<string, unknown>,
missingVars,
- envSource
+ envSource,
+ `${serverName}.env`
);
...
- server.headers = expandEnvVarsInRecord(
+ server.headers = expandEnvVarsInRecord(
server.headers as Record<string, unknown>,
missingVars,
- envSource
+ envSource,
+ `${serverName}.headers`
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/mcp/config.ts` around lines 31 - 34, The current
branch in the config parsing logic (where val and key are handled) should not
coerce non-string env/headers values; replace the getLog().warn + String(val)
coercion with an explicit thrown error that includes the key name and the actual
typeof val (e.g., throw new Error(`Invalid config: expected string for ${key},
got ${typeof val}`)); update the code path around result[key] assignment in the
same function so invalid types immediately fail rather than being coerced, and
ensure any callers of this config loader will surface that error (preserve
existing getLog() usage only for context, not for swallowing the error).
Comment on lines
+57
to
+60
| if (typeof serverConfig !== 'object' || serverConfig === null) { | ||
| getLog().warn({ serverName, valueType: typeof serverConfig }, 'mcp_server_config_not_object'); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Reject array-valued server configs during validation.
Arrays currently pass the object check and flow through as pseudo-objects, which weakens entry validation and can produce malformed provider config.
Suggested fix
- 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`
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/mcp/config.ts` around lines 57 - 60, The current check
in config.ts allows arrays because typeof returns "object"; update the
validation for serverConfig (use Array.isArray(serverConfig)) and treat arrays
as invalid by throwing a clear error (include serverName and the actual
value/type) instead of warning/continuing; replace or augment the block that
currently calls getLog().warn({ serverName, valueType: typeof serverConfig },
'mcp_server_config_not_object') so that when serverConfig is null, not an
object, or an array you throw a descriptive Error describing the unsupported
array-valued server config for serverName.
3d4e767 to
85cf2c0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mcp:were effectively Claude-only. Codex nodes declaredmcp: false, validation warned, and Codex workflows could not attach per-node MCP servers such as Figma without configuring them globally outside the workflow.mcp_serversoverrides, and enabling MCP capability metadata for Codex. The shared MCP loader now lives under@archon/providers/mcp/configand supports both Archon's direct server map and the common{ "mcpServers": { ... } }wrapper.allowed_tools/denied_toolsrestrictions remain unsupported, so MCP-only sandboxing viaallowed_tools: []is still Claude-only. Pi still ignoresmcp. No workflow schema shape, database schema, server API, or Web UI behavior changes.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
claude/provider.tsproviders/mcp/config.tscodex/provider.tsproviders/mcp/config.tscodex/provider.tsconfigoverridesmcp_servers.<name>.*per callcodex/capabilities.tsmcp: trueworkflows/validator.tscapability checksmcp:Label Snapshot
risk: mediumsize: Mworkflows|providers|docs|testsproviders:codex-mcpChange Metadata
featuremultiLinked Issue
No linked issue — this closes a provider capability gap for Codex-based workflow users.
Validation Evidence (required)
Commands and result summary:
Additional targeted coverage included in the full suite:
bun run validatecompleted successfully after this commit. New tests cover Codexmcp_serversoverride construction, request-env expansion, surfacing workflow-configured MCP client errors,mcpServerswrapper loading, and Codex capability/validator behavior.Security Impact (required)
mcp:file.mcp:.Yes, describe risk and mitigation:headersare translated to Codexhttp_headers; values are not logged.Compatibility / Migration
mcp:are unchanged.{ "mcpServers": { ... } }JSON is also accepted.Human Verification (required)
What was personally validated beyond CI:
nodeConfig.mcp, loads JSON, and passes per-callconfig.mcp_serversto the Codex SDK.$VAR_NAMEexpansion uses request/codebase env over process env when provided.headersin Archon MCP JSON maps to Codexhttp_headers.mcpServerswrapper JSON loads as the server map.mcp: true, and workflow resource validation no longer warns for Codex MCP nodes.bun run validatepasses locally.~/.archon/workflowsno longer pollutes loader/dag unit tests due test-localARCHON_HOMEisolation.Side Effects / Blast Radius (required)
@archon/providersCodex/Claude provider internals, workflow MCP validation tests, docs. No server route, database, adapter, isolation, or Web UI state changes.mcp_serversconfig parser; unsupported server fields are not passed except for a conservative allowlist.allowed_tools: []still does not enforce MCP-only sandboxing, so docs explicitly call out that caveat.loadMcpConfigout ofclaude/provider.tschanges an internal import path;claude/index.tsre-exports it for compatibility.Rollback Plan (required)
git revert 858e8a99ondev.mcp:fail to start due Codex config parsing errors.Risks and Mitigations
allowed_tools: []now enforces MCP-only sandboxing for Codex.mcpServerswrapper instead of Archon's direct map.Summary by CodeRabbit
New Features
Behavior / Bug Fixes
Documentation