From 67011befe3ae31397644a4796cef493b84f94c31 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Tue, 4 Aug 2026 16:43:07 -0700 Subject: [PATCH] [eric] mcp: discovery refuses a credentialless env_vars server, and the failure translator reads the whole tail not the npx dump --- backend/apps/tools_lib/mcp_discovery.py | 3 +- backend/apps/tools_lib/tools_lib.py | 4 +++ backend/tests/test_mcp_failure_reason.py | 34 +++++++++++++++++++ frontend/src/app/pages/Tools/integrations.tsx | 2 +- 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/backend/apps/tools_lib/mcp_discovery.py b/backend/apps/tools_lib/mcp_discovery.py index be8bd501..01b9bc0c 100644 --- a/backend/apps/tools_lib/mcp_discovery.py +++ b/backend/apps/tools_lib/mcp_discovery.py @@ -182,7 +182,8 @@ async def discover_mcp_tools_stdio(command: str, args: list[str] | None = None, await asyncio.wait_for(asyncio.shield(stderr_task), timeout=1.0) except (asyncio.TimeoutError, asyncio.CancelledError, Exception): pass - tail = "".join(stderr_tail[-10:]).strip() + # Full window, not the last few lines: an npx wrapper prints ~20 lines of Node crash dump AFTER the server's one useful fatal line, so a short tail hands the translator pure noise. + tail = "".join(stderr_tail).strip() # A Go server's dying breath is a JSON line with a goroutine dump. Handing that to # the UI hides the one fact the user can act on, which is usually "sign in again". raise HTTPException(status_code=502, detail=readable_mcp_failure(tail)) diff --git a/backend/apps/tools_lib/tools_lib.py b/backend/apps/tools_lib/tools_lib.py index 3a3fedde..039c4ff1 100644 --- a/backend/apps/tools_lib/tools_lib.py +++ b/backend/apps/tools_lib/tools_lib.py @@ -362,6 +362,10 @@ async def delete_tool(tool_id: str): async def discover_tools(tool_id: str): tool = load(tool_id) + # A credential-driven server with no credentials dies at boot with a crash dump; refuse to spawn and say the useful thing instead. + if tool.auth_type == "env_vars" and not tool.credentials: + raise HTTPException(status_code=409, detail=f"{tool.name} isn't connected yet. Connect it first, then discover its tools.") + if tool.auth_type == "oauth2" and tool.auth_status == "connected": if tool.oauth_tokens.get("refresh_token"): if tool.name.lower() == "airtable": diff --git a/backend/tests/test_mcp_failure_reason.py b/backend/tests/test_mcp_failure_reason.py index 36cd2b21..524e037e 100644 --- a/backend/tests/test_mcp_failure_reason.py +++ b/backend/tests/test_mcp_failure_reason.py @@ -25,6 +25,32 @@ SLACK_NO_TOKENS = ( '"app":"slack-mcp-server","stacktrace":"provider.New\\n\\tapi.go:682"}' ) +# Byte-for-byte the FULL stderr of a tokenless `npx -y slack-mcp-server` run (2026-08-04): the Go +# fatal comes FIRST, then the npm wrapper's execFileSync crash dump buries it under ~20 Node lines. +# This is the exact toast Haik and Eric saw; a translator fed only the last few lines can never win. +SLACK_NO_TOKENS_WITH_NPX_DUMP = SLACK_NO_TOKENS + """ +node:child_process:963 + throw err; + ^ + +Error: Command failed: /Users/x/.npm/_npx/2f12aed4e6049c73/node_modules/slack-mcp-server-darwin-arm64/bin/slack-mcp-server-darwin-arm64 --transport stdio + at genericNodeError (node:internal/errors:983:15) + at wrappedFn (node:internal/errors:537:14) + at checkExecSyncError (node:child_process:924:11) + at Object.execFileSync (node:child_process:960:15) + at Object. (/Users/x/.npm/_npx/2f12aed4e6049c73/node_modules/slack-mcp-server/bin/index.js:64:14) + at Module._compile (node:internal/modules/cjs/loader:1692:14) + at TracingChannel.traceSync (node:diagnostics_channel:322:14) { + status: 1, + signal: null, + output: [ null, null, null ], + pid: 96234, + stdout: null, + stderr: null +} + +Node.js v24.4.0""" + def test_the_real_slack_failure_becomes_reconnect_advice(): out = readable_mcp_failure(SLACK_REAL) @@ -46,6 +72,14 @@ def test_missing_tokens_reads_differently_from_expired_ones(): assert never != expired +def test_the_npx_crash_dump_never_buries_the_real_reason(): + # The regression that survived 1.7.2: the useful fatal line is FIRST and the Node noise last. + out = readable_mcp_failure(SLACK_NO_TOKENS_WITH_NPX_DUMP) + assert "signed in" in out + for leak in ("TracingChannel", "execFileSync", "node:child_process", "status: 1", "Node.js v"): + assert leak not in out, f"{leak!r} leaked into what the user reads" + + def test_revoked_access_says_so(): assert "revoked" in readable_mcp_failure('{"error":"token_revoked","message":"bad"}').lower() diff --git a/frontend/src/app/pages/Tools/integrations.tsx b/frontend/src/app/pages/Tools/integrations.tsx index 68958a4b..a0969a32 100644 --- a/frontend/src/app/pages/Tools/integrations.tsx +++ b/frontend/src/app/pages/Tools/integrations.tsx @@ -185,7 +185,7 @@ export const INTEGRATIONS: Integration[] = [ id: 'slack', name: 'Slack', description: 'Search messages, send messages, read channels, DMs, and threads in Slack workspaces.', - mcp_config: { type: 'stdio', command: 'npx', args: ['-y', 'slack-mcp-server@latest', '--transport', 'stdio'], env: { SLACK_MCP_ADD_MESSAGE_TOOL: 'true' } }, + mcp_config: { type: 'stdio', command: 'npx', args: ['-y', 'slack-mcp-server@1.3.0', '--transport', 'stdio'], env: { SLACK_MCP_ADD_MESSAGE_TOOL: 'true' } }, color: '#4A154B', website: 'https://github.com/korotovsky/slack-mcp-server', icon: (