Skip to content

Report expired, not healthy, when an MCP connection's credential is missing - #1582

Open
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-health-missing-credential
Open

Report expired, not healthy, when an MCP connection's credential is missing#1582
GeiserX wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-health-missing-credential

Conversation

@GeiserX

@GeiserX GeiserX commented Aug 13, 2026

Copy link
Copy Markdown

TL;DR

An MCP connection whose credential is gone reports healthy. The health check builds its connector from whatever values it has; the renderer skips a placement whose value is unresolved; the probe therefore goes out unauthenticated, and any server that lists tools without auth answers. discoverTools succeeding maps straight to status: "healthy".

That is the one status it must never give in that state, because health is the signal telling a user to re-authenticate.

Fix: gate on the required placement variables and report expired with the missing input names — exactly what the OpenAPI health check already does.


Why the three MCP render paths differ

buildConnectorInput renders auth placements and is reached from three places. They are not the same case, so only one changes:

path before after
tool invocation already refuses — its comment names "dialing unauthenticated" as the thing being avoided unchanged
resolveTools ungated on purpose — "Discovery tolerates unresolved credentials (an open server lists tools unauthenticated; a bad value just yields zero tools)" unchanged
checkHealth no gate, no comment reports expired

The renderer's own contract is what makes this a caller's decision rather than a bug in rendering:

a credential placement whose variable resolved to nothing is skipped — callers own their missing-value policy (fail the invocation, dial unauthenticated, …)

Two of the three callers had made that decision explicitly. The health check had not.

Tests

health-missing-credential.test.ts drives the plugin's own checkHealth with a fetch that answers everything 200, so nothing except the gate stands between the result and healthy.

Both directions are pinned. A gate that returned expired unconditionally would satisfy the headline assertion while breaking every healthy connection, so the second test asserts a resolved input still gets through.

Driven at the plugin seam rather than through a live connection deliberately: the precondition — a connection that exists but whose credential does not resolve — is the state the connection APIs are designed to prevent you from creating, so constructing it through them is not possible without faking the thing under test.

Mutation-checked, each mutation verified to have landed, with an unmutated control before and after:

mutation result
remove the gate (i.e. revert this PR) killed
report expired unconditionally killed

The second mutation initially matched two sites, because the invoke path's gate opens with the same line; it was re-run with an anchor unique to the health gate rather than mutating an arbitrary one.

Package: 128 passed / 29 skipped. tsgo --noEmit, oxlint --deny-warnings and oxfmt --check clean.

…issing

checkHealth built its connector from whatever values it had. A missing value is
skipped by the renderer, so the probe dialled unauthenticated, and any server
that lists tools without auth answered -- discoverTools succeeding maps to
healthy. A connection whose credential was gone therefore reported healthy,
which is the one status that must never appear in that state, because health is
what tells a user to re-authenticate.

Mirrors the OpenAPI health check. resolveTools stays ungated on purpose.
@GeiserX

GeiserX commented Aug 13, 2026

Copy link
Copy Markdown
Author

Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there.

This PR stands alone and doesn't depend on any of the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant