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

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.

Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com>

Author: GeiserX

.changeset/mcp-health-missing-credential.md

@@ -0,0 +1,11 @@
+---
+"executor": patch
+---
+
+**An MCP health check no longer reports `healthy` when the connection's credential is missing**
+
+Rendering skips an auth placement whose value is unresolved — that is the renderer's documented behaviour, and callers own the missing-value policy. The MCP health check had no such policy, so it dialled unauthenticated, and any server that lists tools without auth answered. `discoverTools` succeeding maps straight to `healthy`, so a connection whose credential was gone reported as healthy.
+
+Health status is the signal telling a user to re-authenticate, which makes `healthy` the one answer it must never give in that state. The check now reports `expired` with the unresolved input names, mirroring the OpenAPI health check, which already did exactly this.
+
+The MCP tool-invocation path already refused for the same reason. `resolveTools` is deliberately left alone — its own comment records that discovery tolerating unresolved credentials is intended, since an open server lists tools unauthenticated.

packages/plugins/mcp/src/sdk/health-missing-credential.test.ts

@@ -0,0 +1,111 @@
+// ---------------------------------------------------------------------------
+// A health check must not report `healthy` for a connection whose credential is
+// missing.
+//
+// Rendering SKIPS a placement whose value is unresolved — that is the renderer's
+// documented behaviour, and callers own the policy. The invoke path already
+// refuses (it names "dialing unauthenticated" as the thing it is avoiding), and
+// the OpenAPI health check reports `expired`. The MCP health check had neither,
+// so it dialled unauthenticated, and any server that lists tools without auth
+// answered — reporting a connection with no credential as healthy.
+//
+// That matters more than an ordinary wrong status: health is the signal telling
+// a user to re-authenticate, so `healthy` is the one answer it must never give
+// when the credential is gone.
+//
+// Driven against the plugin's own `checkHealth` rather than through a live
+// connection, because the precondition under test — a connection that EXISTS but
+// whose credential does not resolve — is exactly the state the connection APIs
+// are designed to prevent you from creating. Calling the seam directly is the
+// only way to construct it without faking the thing being tested.
+// ---------------------------------------------------------------------------
+
+import { describe, expect, it } from "@effect/vitest";
+import { Effect, Layer } from "effect";
+import { FetchHttpClient } from "effect/unstable/http";
+
+import { mcpPlugin } from "./plugin";
+
+const ENDPOINT = "https://mcp.example.test/sse";
+
+/** A remote MCP integration whose api-key method needs one input. */
+const config = {
+  transport: "remote" as const,
+  endpoint: ENDPOINT,
+  remoteTransport: "streamable-http" as const,
+  authenticationTemplate: [
+    {
+      slug: "api_key",
+      kind: "apikey" as const,
+      placements: [{ carrier: "header" as const, name: "X-Api-Key", variable: "token" }],
+    },
+  ],
+};
+
+/** Answers anything with 200 — standing in for a server that lists tools with no
+ *  auth at all, which is what turned a missing credential into `healthy`. */
+const permissiveClientLayer = FetchHttpClient.layer.pipe(
+  Layer.provide(
+    Layer.succeed(FetchHttpClient.Fetch)(
+      (async (_input: RequestInfo | URL) =>
+        new Response("{}", {
+          status: 200,
+          headers: { "content-type": "application/json" },
+        })) as typeof globalThis.fetch,
+    ),
+  ),
+);
+
+const checkHealthWith = (values: Record<string, string | null>) =>
+  Effect.gen(function* () {
+    const plugin = mcpPlugin();
+    const checkHealth = (plugin as { readonly checkHealth?: unknown }).checkHealth;
+    if (typeof checkHealth !== "function") {
+      return yield* Effect.die("mcpPlugin no longer exposes checkHealth");
+    }
+    return yield* (
+      checkHealth as (input: {
+        readonly ctx: { readonly httpClientLayer: typeof permissiveClientLayer };
+        readonly credential: {
+          readonly config: typeof config;
+          readonly values: Record<string, string | null>;
+          readonly template: string;
+          readonly connection: string;
+          readonly integration: string;
+        };
+      }) => Effect.Effect<{ readonly status: string; readonly detail?: string }>
+    )({
+      ctx: { httpClientLayer: permissiveClientLayer },
+      credential: {
+        config,
+        values,
+        template: "api_key",
+        connection: "main",
+        integration: "health_mcp",
+      },
+    });
+  });
+
+describe("MCP health check with an unresolved credential", () => {
+  it.effect("reports expired, not healthy, when the api-key input is missing", () =>
+    Effect.gen(function* () {
+      const health = yield* checkHealthWith({});
+
+      // The fetch above answers everything 200, so nothing except the gate
+      // stands between this and `healthy`.
+      expect(health.status).not.toBe("healthy");
+      expect(health.status).toBe("expired");
+      expect(String(health.detail ?? "")).toContain("token");
+    }),
+  );
+
+  it.effect("does not short-circuit when the input IS resolved", () =>
+    Effect.gen(function* () {
+      // The other half: a gate that returned `expired` unconditionally would
+      // satisfy the test above while breaking every healthy connection.
+      const health = yield* checkHealthWith({ token: "sk-present" });
+
+      expect(health.status).not.toBe("expired");
+    }),
+  );
+});

packages/plugins/mcp/src/sdk/plugin.ts

@@ -1559,6 +1559,30 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => {
         if (!parsed) {
           return { status: "unknown" as const, checkedAt: Date.now() } satisfies HealthCheckResult;
         }
+        // An unresolved apikey input reports `expired`, not `healthy`.
+        //
+        // Rendering skips a placement whose value is missing, so without this the
+        // probe dials UNAUTHENTICATED — and any server that lists tools without
+        // auth answers, making a connection whose credential is gone report as
+        // healthy. Health is the signal that tells a user to re-authenticate, so
+        // that is the one status it must never give here. The invoke path already
+        // refuses for the same reason, and the OpenAPI health check reports
+        // `expired` in exactly this case.
+        if (parsed.transport === "remote") {
+          const method = selectAuthMethod(parsed, String(credential.template));
+          if (method?.kind === "apikey") {
+            const missing = requiredPlacementVariables(method.placements).filter(
+              (variable) => credential.values[variable] == null,
+            );
+            if (missing.length > 0) {
+              return {
+                status: "expired" as const,
+                checkedAt: Date.now(),
+                detail: `Connection has no resolvable credential value for input(s): ${missing.join(", ")}.`,
+              } satisfies HealthCheckResult;
+            }
+          }
+        }
         const connector = yield* buildConnectorInput(
           parsed,
           credential.values,