fix(workflows): filter user-plugin MCP noise out of workflow warnings#1327
Merged
fix(workflows): filter user-plugin MCP noise out of workflow warnings#1327
Conversation
Before this change, the dag-executor surfaced every entry in the Claude SDK's "MCP server connection failed: …" system message to the user. That message includes user-level plugin MCPs inherited from ~/.claude/ (e.g. `telegram`) that fail to connect in the headless workflow subprocess — they're non-actionable noise for the workflow author. Fix: - Pre-compute the set of workflow-configured MCP server names per node by parsing the `mcp:` config file once at the start of executeNodeInternal. No caller-facing API change; no duplication of the provider's env-var expansion logic (we only need the keys). - Split the system-message handler: the `MCP server connection failed:` path now surfaces only the subset of failing names that match the node's configured set; user-plugin failures are debug-logged as `dag.mcp_plugin_connection_suppressed`. The `⚠️ ` branch is unchanged. Supersedes #1134 (closed as stale — the Windows HOME fix in that PR was already shipped via #1302, and the claude.ts enabledPlugins change targeted a file that has since moved into @archon/providers). Credits @MrFadiAi for identifying and reporting the underlying issue. Co-authored-by: Fadi Ai <MrFadiAi@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds parsing and config-loading helpers and updates the dag executor to load a node's Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner/Client
participant Exec as DagExecutor
participant FS as FileSystem
participant SDK as SDK Stream
participant User as Workflow User
participant Log as Debug Logger
Runner->>Exec: executeNode(node)
Exec->>FS: loadConfiguredMcpServerNames(node.mcp, cwd)
FS-->>Exec: Set[string] of configured servers
SDK->>Exec: system message "MCP server connection failed: ..."
Exec->>Exec: parseMcpFailureServerNames(message)
alt matches configured servers
Exec->>User: forward filtered MCP failure message
else all unconfigured (plugin noise)
Exec->>Log: dag.mcp_plugin_connection_suppressed (debug)
end
SDK->>Exec: system message "⚠️ ..." (provider warning)
Exec->>User: forward provider warning verbatim
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/workflows/src/dag-executor.ts (1)
885-912: Preserve MCP failure details while filtering names.
filteredMsgforwards only server names, and the debug log stores only names, so the original failure reason/status is lost for both workflow-configured servers and suppressed plugins. Keep the original entry text per parsed name while still filtering unconfigured servers.♻️ Proposed diagnostics-preserving filter
const failedNames = parseMcpFailureServerNames(msg.content); + const failureEntryByName = new Map<string, string>(); + for (const entry of msg.content.slice(MCP_FAILURE_PREFIX.length).split(', ')) { + const name = entry.split(' (')[0]?.trim(); + if (name && !failureEntryByName.has(name)) { + failureEntryByName.set(name, entry.trim()); + } + } const workflowFailures = failedNames.filter(n => configuredMcpNames.has(n)); const pluginFailures = failedNames.filter(n => !configuredMcpNames.has(n)); if (workflowFailures.length > 0) { - const filteredMsg = `${MCP_FAILURE_PREFIX}${workflowFailures.join(', ')}`; + const filteredMsg = `${MCP_FAILURE_PREFIX}${workflowFailures + .map(n => failureEntryByName.get(n) ?? n) + .join(', ')}`; getLog().warn( { nodeId: node.id, systemContent: filteredMsg }, 'dag.provider_warning_forwarded' ); @@ if (pluginFailures.length > 0) { getLog().debug( - { nodeId: node.id, pluginFailures }, + { + nodeId: node.id, + pluginFailures: pluginFailures.map(n => failureEntryByName.get(n) ?? n), + }, 'dag.mcp_plugin_connection_suppressed' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 885 - 912, The current code uses parseMcpFailureServerNames to extract only server names and loses the original failure text; update the filtering so you keep the original parsed entries (name+reason/status) while still splitting them into workflowFailures vs pluginFailures by checking configuredMcpNames. Specifically, change how failedNames is produced and filtered (references: parseMcpFailureServerNames, configuredMcpNames) so filteredMsg (MCP_FAILURE_PREFIX + ...) and the plugin debug log include the full original entry text per server rather than just the bare names, and continue to call safeSendMessage and error/log paths unchanged for workflowFailures and pluginFailures.packages/workflows/src/dag-executor.test.ts (1)
5761-5853: Add one executor-path regression test for MCP warning filtering.The helper tests are solid, but they do not prove
executeDagWorkflowactually forwards configured MCP failures and suppresses plugin failures. A small mocked-provider test would cover the changed branch end-to-end without network or timing dependencies.🧪 Example deterministic test coverage
+describe('executeDagWorkflow -- MCP plugin-noise filtering', () => { + let testDir: string; + + beforeEach(async () => { + testDir = join(tmpdir(), `dag-mcp-filter-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await mkdir(testDir, { recursive: true }); + mockSendQueryDag.mockClear(); + mockGetAgentProviderDag.mockImplementation(() => ({ + sendQuery: mockSendQueryDag, + getType: () => 'claude', + getCapabilities: mockClaudeCapabilities, + })); + }); + + afterEach(async () => { + await rm(testDir, { recursive: true, force: true }); + }); + + it('forwards only workflow-configured MCP failures', async () => { + await writeFile(join(testDir, 'mcp.json'), JSON.stringify({ github: { command: 'npx' } })); + mockSendQueryDag.mockImplementation(function* () { + yield { + type: 'system', + content: 'MCP server connection failed: telegram (disconnected), github (timeout)', + }; + yield { type: 'result', sessionId: 'mcp-filter-session' }; + }); + + const platform = createMockPlatform(); + + await executeDagWorkflow( + createMockDeps(), + platform, + 'conv-mcp-filter', + testDir, + { + name: 'mcp-filter-test', + nodes: [{ id: 'review', prompt: 'Review', mcp: 'mcp.json' }], + }, + makeWorkflowRun('mcp-filter-run'), + 'claude', + undefined, + join(testDir, 'artifacts'), + join(testDir, 'logs'), + 'main', + 'docs/', + minimalConfig + ); + + const messages = (platform.sendMessage as ReturnType<typeof mock>).mock.calls.map( + (call: unknown[]) => call[1] as string + ); + expect(messages.some(m => m.includes('github'))).toBe(true); + expect(messages.some(m => m.includes('telegram'))).toBe(false); + }); +}); + describe('parseMcpFailureServerNames', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 5761 - 5853, Add an end-to-end unit test for executeDagWorkflow that verifies MCP warning filtering by mocking a provider to emit both MCP server connection failures and plugin-level failures, and asserting executeDagWorkflow forwards only the configured MCP failures (using loadConfiguredMcpServerNames/parseMcpFailureServerNames semantics) while suppressing plugin failures; to implement, add a test that registers a fake provider which produces deterministic log/messages, call executeDagWorkflow with a nodeMcpPath pointing at a temp config created via the existing loadConfiguredMcpServerNames helper, then assert the workflow result/logs contain the MCP server names parsed by parseMcpFailureServerNames and do not contain plugin failure entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5761-5853: Add an end-to-end unit test for executeDagWorkflow that
verifies MCP warning filtering by mocking a provider to emit both MCP server
connection failures and plugin-level failures, and asserting executeDagWorkflow
forwards only the configured MCP failures (using
loadConfiguredMcpServerNames/parseMcpFailureServerNames semantics) while
suppressing plugin failures; to implement, add a test that registers a fake
provider which produces deterministic log/messages, call executeDagWorkflow with
a nodeMcpPath pointing at a temp config created via the existing
loadConfiguredMcpServerNames helper, then assert the workflow result/logs
contain the MCP server names parsed by parseMcpFailureServerNames and do not
contain plugin failure entries.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 885-912: The current code uses parseMcpFailureServerNames to
extract only server names and loses the original failure text; update the
filtering so you keep the original parsed entries (name+reason/status) while
still splitting them into workflowFailures vs pluginFailures by checking
configuredMcpNames. Specifically, change how failedNames is produced and
filtered (references: parseMcpFailureServerNames, configuredMcpNames) so
filteredMsg (MCP_FAILURE_PREFIX + ...) and the plugin debug log include the full
original entry text per server rather than just the bare names, and continue to
call safeSendMessage and error/log paths unchanged for workflowFailures and
pluginFailures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0509c545-b54b-4669-81b4-6166ac95a096
📒 Files selected for processing (3)
CHANGELOG.mdpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
Collaborator
Author
PR Review Summary (multi-agent)Six specialist agents reviewed this PR. The core fix (split workflow-configured vs. plugin MCP failures) is sound and well-tested at the helper level. There is one likely-real bug, one observability gap that violates the project's fail-fast rule, and a handful of polish items. Critical Issues (2)
Important Issues (4)
Suggestions (3)
Strengths
VerdictNEEDS FIXES — the status-stripping bug (Critical 1) and the silent catch (Critical 2) should both be addressed before merge. The other items are quick polish or doc updates. Recommended Actions
|
…ervability Address review feedback on PR #1327: - parseMcpFailureServerNames now returns {name, segment} entries so the forwarded "MCP server connection failed: ..." message preserves the per-server status detail (e.g. "(timeout)", "(disconnected)") that the bare-name reconstruction was dropping. - loadConfiguredMcpServerNames now debug-logs read failures as dag.mcp_filter_config_read_failed instead of swallowing them silently. A transient EMFILE/EBUSY at filter time would otherwise silently reclassify a real workflow-MCP failure as plugin noise. - Add 4 integration tests through executeDagWorkflow covering the mixed workflow/plugin split, all-plugin suppression, no-mcp:-config nodes, and the unchanged⚠️ warning path. - Drop a WHAT comment above configuredMcpNames and a temporal phrase ("before this filter landed") that would rot on merge. - Document the filtering boundary in guides/mcp-servers.md and add a troubleshooting row for users debugging silently-suppressed plugin MCPs.
Collaborator
Author
|
Pushed 7a32dde addressing the multi-agent review:
Skipped as YAGNI:
|
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
Supersedes #1134 (closed — stale and conflicting).
The Claude SDK surfaces a system message of the form:
whenever any MCP server in the subprocess fails to connect. Until now the dag-executor forwarded the entire message to the conversation. That meant user-level plugin MCPs inherited from
~/.claude/(e.g.telegram,notion) — which have no connection to what the workflow author configured and routinely fail inside a headless subprocess — were shown as workflow warnings.Fix: only surface failures for servers the workflow actually configured via
mcp:.Changes
packages/workflows/src/dag-executor.tsparseMcpFailureServerNames(message)— parses the SDK's formatted failure string into an ordered, deduped name list.loadConfiguredMcpServerNames(nodeMcpPath, cwd)— reads the node'smcp:JSON file and returnsObject.keys(). Deliberately does not env-expand (the provider owns that and will surface its own parse errors via⚠️); parse/read failures return an empty set.executeNodeInternalpre-computesconfiguredMcpNamesonce at entry.msg.type === 'system'handler now has two distinct branches:MCP server connection failed:→ split by workflow-configured vs. plugin; surface only the workflow-configured subset; debug-log the rest asdag.mcp_plugin_connection_suppressed.⚠️→ unchanged (always surface — these are author-actionable, e.g. missing env vars, Haiku+MCP, invalid structured output).packages/workflows/src/dag-executor.test.ts— 13 new tests covering both helpers (malformed input, dedup, absolute/relative paths, missing files, invalid JSON, non-object JSON).CHANGELOG.md— entry under Fixed, credits @MrFadiAi.Why this approach over #1134
The original PR also tried to pass
settings: { enabledPlugins: {} }to the Claude SDK to disable user plugins in the subprocess. We deliberately skip that: it would change subprocess behavior globally (not just silence the warning), and the SDK's plugin-disable flag is an ergonomics concession that could mask real issues. Filtering at the display layer keeps all diagnostics available in debug logs while giving workflow authors a clean conversation. If a workflow-configured MCP really fails, the warning still surfaces verbatim.Parsing the
mcp:JSON twice (once here for names, once in the provider for full env-expanded server configs) was judged cheaper than changing the provider contract to return the names it loaded. The dag-executor read is cached per node-invocation and tolerant of any file-read failure.Test plan
bun run validategreen locally (type-check + lint + format:check + full test suite)bun test packages/workflows/src/dag-executor.test.ts— 177 pass / 0 failparseMcpFailureServerNames+loadConfiguredMcpServerNamesCo-authored-by: Fadi Ai MrFadiAi@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests
Documentation