Uh oh!
There was an error while loading. Please reload this page.
fix: surface the real reason an MCP server is unavailable - #1159
Draft
sahrizvi wants to merge 1 commit into
Draft
Conversation
Two defects where the diagnostic information already exists in the
process and is discarded before it reaches the user.
`server unavailable` logged `status.status` — the constant string
`"failed"` on that branch — and dropped `status.error`, the field
holding the actual message (`401 Unauthorized`, a transport error,
`Invalid MCP URL for "<key>"`). Extracted `unavailableLogFields()` as a
pure function so the payload is testable without standing up a
transport, and so a later edit cannot quietly drop the field again.
Environment variables that resolve to empty were never named. A
`{env:VAR}` with nothing set becomes `""`, the config parses clean, and
the server launches with a blank credential — usually a password —
failing later with an error naming neither the variable nor the file.
The names are now recorded at both substitution sites: per-server for
discovered external configs, per-file for the main config. They surface
in `/mcps` and `mcp list`, shown even when the server reports connected,
because a blank credential often connects and fails on first real use.
An unresolved bare `${VAR}` is deliberately left literal by the config
layer so a later runtime layer can fill it (the bedrock provider fills
`${AWS_REGION}` from the effective region). That case is not reported.
Closes#1121Closes#701
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsVImportant Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
6 tasks
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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 freeto 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.
Issue for this PR
Closes#1121
Closes#701
Type of change
What does this PR do?
Two defects where the diagnostic information already exists in the process and is thrown away before the user sees it.
1. The connect error is discarded (#1121). When an MCP server fails to connect, the warning logged
status.status— which on that branch is always the constant"failed"— and droppedstatus.error, the field holding the real message (401 Unauthorized, a transport error,Invalid MCP URL for "<key>"). The reporter had to read the source to find out why their server wouldn't connect.unavailableLogFields()is split out as a pure function so the payload is testable without a live transport, and so a later edit can't silently drop the field again.2. Blanked environment variables are never named (#701). A
{env:VAR}with nothing set becomes"". The config parses clean, the server launches with a blank credential — usually a password — and fails later with an error naming neither the variable nor the file. The names are now recorded at both substitution sites (per-server for discovered configs like.vscode/mcp.json; per-file for the main config) and shown in/mcpsandmcp list. They're shown even when the server reports connected, because a blank credential often connects and only fails on first real use.Deliberately not reported: an unresolved bare
${VAR}is left literal by the config layer on purpose, so a later runtime layer can fill it — the bedrock provider fills${AWS_REGION}from the effective region. Warning there would be a false positive on a supported setup.Known limitation: the main-config report is file-scoped, not server-scoped. Substitution runs on raw config text before any structure exists, so a blanked variable belonging to a non-MCP field would also show under
mcp list. The message saysresolved to empty in <file>rather than attributing it to a server. Narrowing it would need offset-to-JSON-path mapping.How did you verify your code works?
opencodesuite: 12,323 tests, 0 fail.HOMEagainst a temp project, asserting the variable is named and that a fully-resolved config stays silent.status.errorfield, or the blank-env recording, each fails exactly one test — so neither test passes vacuously.mainrather than reformatting them.Screenshots / recordings
Not a UI change.
Checklist
Stacked
A follow-up PR (#1160) adds
mcp statusand discovered-config drift reporting on top of this branch.🤖 Generated with Claude Code
https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV