From b2547429ca6642bef7ff2b9a084265b812ce8979 Mon Sep 17 00:00:00 2001 From: Michal Date: Sun, 9 Aug 2026 21:23:33 +0100 Subject: [PATCH 1/3] fix(health): a passing tools/list is `live`, not `healthy` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mcpctl get instances` showed all eight servers healthy while the UniFi one had never once reached its controller. The default probe is `tools/list`, which MCP servers answer from a static in-process table — no credentials, no upstream, ~3ms. It cannot fail for any reason the user cares about, so it was reporting `healthy` for every process that managed to start. Split the two passes: healthy — readiness: `tools/call` on `healthCheck.tool`. The upstream answered, so the server can actually do its job. live — liveness: `tools/list` only. Process up, upstream unverified. `live` is now the default for any server without a `healthCheck.tool`. It is not a warning; it is an admission that nothing is watching that server. Probe events name which probe ran and which tool ("Readiness check (list_sites) passed"), so the events log distinguishes the two after the fact. Also: - `healthCheck.tool` is optional now, so the timings can be tuned without inventing a readiness probe. - `create server --health-check-tool/-args/-interval/-timeout/ -failure-threshold`, per the rule that everything applyable is a create flag. Merges over a `--from-template` healthCheck rather than replacing it. - `describe instance` explains a `live` verdict instead of leaving it cryptic. - create.ts held a raw NUL byte in a string literal, which made grep treat the whole file as binary and silently skip it. Escaped as `\0`. Verified against the live fleet: with readiness probes configured, my-grafana went unhealthy (Grafana API 403) and my-node-red degraded (connect timeout to a Tailscale address) — both had read healthy for months. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114dg56YmVacyqhp5fitcTb --- completions/mcpctl.bash | 2 +- completions/mcpctl.fish | 5 + docs/reliability.md | 60 ++++++ src/cli/src/commands/apply.ts | 3 +- src/cli/src/commands/create.ts | 76 ++++++- src/cli/src/commands/describe.ts | 6 + src/cli/tests/health-check-opts.test.ts | 59 ++++++ src/mcpd/src/services/health-probe.service.ts | 35 +++- src/mcpd/src/validation/template.schema.ts | 8 +- src/mcpd/tests/services/health-probe.test.ts | 65 +++++- .../smoke/health-readiness.smoke.test.ts | 185 ++++++++++++++++++ 11 files changed, 495 insertions(+), 9 deletions(-) create mode 100644 src/cli/tests/health-check-opts.test.ts create mode 100644 src/mcplocal/tests/smoke/health-readiness.smoke.test.ts diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index a045a22..16a4fe5 100644 --- a/completions/mcpctl.bash +++ b/completions/mcpctl.bash @@ -194,7 +194,7 @@ _mcpctl() { else case "$create_sub" in server) - COMPREPLY=($(compgen -W "-d --description --package-name --runtime --docker-image --transport --repository-url --external-url --command --container-port --replicas --env --from-template --env-from-secret --force -h --help" -- "$cur")) + COMPREPLY=($(compgen -W "-d --description --package-name --runtime --docker-image --transport --repository-url --external-url --command --container-port --replicas --env --health-check-tool --health-check-args --health-check-interval --health-check-timeout --health-check-failure-threshold --from-template --env-from-secret --force -h --help" -- "$cur")) ;; secret) COMPREPLY=($(compgen -W "--data --force -h --help" -- "$cur")) diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index e79dc3b..822767d 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -380,6 +380,11 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l command -d 'Comm complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l container-port -d 'Container port number' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l replicas -d 'Number of replicas' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l env -d 'Env var: KEY=value (inline) or KEY=secretRef:SECRET:KEY (secret ref, repeat for multiple)' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-tool -d 'Readiness probe: tool to call (without it the server only gets a liveness probe and reports "live", never "healthy")' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-args -d 'Readiness probe: JSON object of arguments for the probe tool' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-interval -d 'Readiness probe interval in seconds (default 60)' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-timeout -d 'Readiness probe timeout in seconds (default 10)' -x +complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l health-check-failure-threshold -d 'Consecutive failures before the instance is marked unhealthy (default 3)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l from-template -d 'Create from template (name or name:version)' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l env-from-secret -d 'Map template env vars from a secret' -x complete -c mcpctl -n "__mcpctl_subcmd_active create server" -l force -d 'Update if already exists' diff --git a/docs/reliability.md b/docs/reliability.md index 28a1c25..5c1f2c5 100644 --- a/docs/reliability.md +++ b/docs/reliability.md @@ -28,6 +28,66 @@ Note: the gate's prompt-ranking uses the **heavy client provider's own model** it deliberately does *not* force the project's vLLM model onto it (doing so made every selection fail silently when the model wasn't anthropic-servable). +## Instance health: `live` is not `healthy` + +An MCP server answers `tools/list` from a **static, in-process table**. It costs +a few milliseconds, needs no credentials, and reaches no upstream — so it stays +green while the thing the server exists to talk to is unreachable. Treating that +as a health signal is how `mcpctl get instances` showed eight healthy servers +while the UniFi one had never once reached its controller. + +So the probe reports two different passes: + +| Status | Probe | Means | +|---|---|---| +| `healthy` | **readiness** — `tools/call` on `healthCheck.tool` | The upstream answered. The server can do its job. | +| `live` | **liveness** — `tools/list` only | The process is up and speaks MCP. Its upstream is **unverified**. | +| `degraded` | either, failing | Failing, but under `failureThreshold`. | +| `unhealthy` | either, failing | Failed `failureThreshold` times in a row. | + +`live` is the default for any server with no `healthCheck.tool`. It is not a +warning — it is an admission that nothing is watching that server's upstream. + +**Configure a readiness probe on every server.** Pick a read-only tool that +genuinely round-trips to the upstream, and verify it passes before configuring +it — a probe naming a local-only tool (`get_..._version`) or a tool the server +doesn't expose reproduces the same false green it was meant to remove. + +```bash +mcpctl create server unifi-network --health-check-tool list_sites \ + --health-check-interval 60 --health-check-timeout 15 --force +``` + +or declaratively — `healthCheck` round-trips through `get -o yaml | apply -f`: + +```yaml +healthCheck: + tool: list_sites + arguments: {} + intervalSeconds: 60 + timeoutSeconds: 15 + failureThreshold: 3 +``` + +Omit `tool` to keep liveness while still tuning the timings. + +Latency is the tell: a probe answering in single-digit milliseconds is reading a +local table, not crossing a network. The UniFi probe went from 3ms (`tools/list`, +lying) to 1847ms on its first real `list_sites` — login, TLS, controller round +trip — and ~40ms once the session was warm. + +### Two failure modes the probe cannot see for you + +The healthy-looking UniFi server was broken **twice over**, and both are worth +checking first when a readiness probe starts failing: + +1. **Egress.** MCP server pods default to TCP 80/443 only + (`servers-allow-external-egress`). Any upstream on another port — the UniFi + controller on `:8443` — times out on every call. Declare it in Pulumi's + `mcpctl.serverEgressTargets` (name + `/32` + ports); don't widen the blanket rule. +2. **Address reachability.** A pod cannot reach a **Tailscale** `100.64.0.0/10` + address. Config pointing at one connect-timeouts forever. Use LAN IPs. + ## LLM-*essential* operations — failover chain Chat needs *an* LLM but not a *specific* one. Instead of failing when the pinned diff --git a/src/cli/src/commands/apply.ts b/src/cli/src/commands/apply.ts index 5360a1a..c41325a 100644 --- a/src/cli/src/commands/apply.ts +++ b/src/cli/src/commands/apply.ts @@ -5,7 +5,8 @@ import { z } from 'zod'; import type { ApiClient } from '../api-client.js'; const HealthCheckSchema = z.object({ - tool: z.string().min(1), + /** Omit for a liveness-only probe (reports `live`); set it for readiness (`healthy`). */ + tool: z.string().min(1).optional(), arguments: z.record(z.unknown()).default({}), intervalSeconds: z.number().int().min(5).max(3600).default(60), timeoutSeconds: z.number().int().min(1).max(120).default(10), diff --git a/src/cli/src/commands/create.ts b/src/cli/src/commands/create.ts index 6f64801..fe2efd0 100644 --- a/src/cli/src/commands/create.ts +++ b/src/cli/src/commands/create.ts @@ -42,6 +42,69 @@ export function buildFavouriteIndex( return result; } +export interface HealthCheckOpts { + healthCheckTool?: string; + healthCheckArgs?: string; + healthCheckInterval?: string; + healthCheckTimeout?: string; + healthCheckFailureThreshold?: string; +} + +export interface HealthCheckSpec { + tool?: string; + arguments?: Record; + intervalSeconds?: number; + timeoutSeconds?: number; + failureThreshold?: number; +} + +function parsePositiveInt(flag: string, value: string): number { + const n = Number(value); + if (!Number.isInteger(n) || n <= 0) { + throw new Error(`Invalid ${flag} '${value}'. Expected a positive integer.`); + } + return n; +} + +/** + * Build a server `healthCheck` spec from `--health-check-*` flags, or undefined + * when none were given (so the field is only sent when intended). + * + * Mirrors the `healthCheck:` block accepted by `apply -f`, per the rule that + * everything applyable is also a create flag. + */ +export function buildHealthCheck(opts: HealthCheckOpts): HealthCheckSpec | undefined { + const { healthCheckTool, healthCheckArgs, healthCheckInterval, healthCheckTimeout, healthCheckFailureThreshold } = opts; + const given = [healthCheckTool, healthCheckArgs, healthCheckInterval, healthCheckTimeout, healthCheckFailureThreshold] + .some((v) => v !== undefined); + if (!given) return undefined; + + if (healthCheckArgs !== undefined && healthCheckTool === undefined) { + throw new Error('--health-check-args requires --health-check-tool.'); + } + + const spec: HealthCheckSpec = {}; + if (healthCheckTool !== undefined) spec.tool = healthCheckTool; + if (healthCheckArgs !== undefined) { + let parsed: unknown; + try { + parsed = JSON.parse(healthCheckArgs); + } catch { + throw new Error(`Invalid --health-check-args: not valid JSON. Expected a JSON object, e.g. '{"site":"default"}'.`); + } + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error(`Invalid --health-check-args: expected a JSON object, e.g. '{"site":"default"}'.`); + } + spec.arguments = parsed as Record; + } + if (healthCheckInterval !== undefined) spec.intervalSeconds = parsePositiveInt('--health-check-interval', healthCheckInterval); + if (healthCheckTimeout !== undefined) spec.timeoutSeconds = parsePositiveInt('--health-check-timeout', healthCheckTimeout); + if (healthCheckFailureThreshold !== undefined) { + spec.failureThreshold = parsePositiveInt('--health-check-failure-threshold', healthCheckFailureThreshold); + } + return spec; +} + /** * Parse a `--ttl` value. * @@ -136,6 +199,11 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { .option('--container-port ', 'Container port number') .option('--replicas ', 'Number of replicas') .option('--env ', 'Env var: KEY=value (inline) or KEY=secretRef:SECRET:KEY (secret ref, repeat for multiple)', collect, []) + .option('--health-check-tool ', 'Readiness probe: tool to call (without it the server only gets a liveness probe and reports "live", never "healthy")') + .option('--health-check-args ', 'Readiness probe: JSON object of arguments for the probe tool') + .option('--health-check-interval ', 'Readiness probe interval in seconds (default 60)') + .option('--health-check-timeout ', 'Readiness probe timeout in seconds (default 10)') + .option('--health-check-failure-threshold ', 'Consecutive failures before the instance is marked unhealthy (default 3)') .option('--from-template ', 'Create from template (name or name:version)') .option('--env-from-secret ', 'Map template env vars from a secret') .option('--force', 'Update if already exists') @@ -218,6 +286,12 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { if (opts.externalUrl) body.externalUrl = opts.externalUrl; if (opts.command.length > 0) body.command = opts.command; if (opts.containerPort) body.containerPort = parseInt(opts.containerPort, 10); + // Merge over any healthCheck inherited from --from-template so partial + // flags (e.g. only --health-check-interval) tune rather than replace it. + const healthCheck = buildHealthCheck(opts as HealthCheckOpts); + if (healthCheck) { + body.healthCheck = { ...(base.healthCheck as HealthCheckSpec | undefined), ...healthCheck }; + } if (opts.env.length > 0) { // Merge: CLI env entries override template env entries by name const cliEnv = parseServerEnv(opts.env); @@ -898,7 +972,7 @@ export function createCreateCommand(deps: CreateCommandDeps): Command { const buf = await fs.readFile(full); // Reject non-UTF8 — v1 is text-only. const text = buf.toString('utf-8'); - if (text.includes('')) { + if (text.includes('\0')) { throw new Error(`File ${rel} contains a null byte; binaries aren't supported in v1`); } files[rel] = text; diff --git a/src/cli/src/commands/describe.ts b/src/cli/src/commands/describe.ts index bda4ffb..cd94d69 100644 --- a/src/cli/src/commands/describe.ts +++ b/src/cli/src/commands/describe.ts @@ -90,6 +90,12 @@ function formatInstanceDetail(instance: Record, inspect?: Recor lines.push('Health:'); lines.push(` ${pad('Status:', 16)}${healthStatus ?? 'unknown'}`); if (lastHealthCheck) lines.push(` ${pad('Last Check:', 16)}${lastHealthCheck}`); + if (healthStatus === 'live') { + lines.push(` ${pad('Probe:', 16)}liveness (tools/list) — process is up, but nothing`); + lines.push(` ${pad('', 16)}calls the server's upstream. Configure a readiness`); + lines.push(` ${pad('', 16)}probe to reach 'healthy':`); + lines.push(` ${pad('', 16)} mcpctl edit server ${server?.name ?? ''} → healthCheck.tool`); + } } const metadata = instance.metadata as Record | undefined; diff --git a/src/cli/tests/health-check-opts.test.ts b/src/cli/tests/health-check-opts.test.ts new file mode 100644 index 0000000..0c02c03 --- /dev/null +++ b/src/cli/tests/health-check-opts.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; +import { buildHealthCheck } from '../src/commands/create.js'; + +describe('buildHealthCheck — CLI flags → server healthCheck', () => { + it('returns undefined when no health-check flag is given', () => { + expect(buildHealthCheck({})).toBeUndefined(); + }); + + it('builds a readiness probe from --health-check-tool', () => { + expect(buildHealthCheck({ healthCheckTool: 'list_sites' })).toEqual({ tool: 'list_sites' }); + }); + + it('parses --health-check-args as a JSON object', () => { + expect(buildHealthCheck({ healthCheckTool: 'get_devices', healthCheckArgs: '{"site":"default"}' })) + .toEqual({ tool: 'get_devices', arguments: { site: 'default' } }); + }); + + it('carries the timing flags through', () => { + expect(buildHealthCheck({ + healthCheckTool: 'list_sites', + healthCheckInterval: '120', + healthCheckTimeout: '15', + healthCheckFailureThreshold: '2', + })).toEqual({ + tool: 'list_sites', + intervalSeconds: 120, + timeoutSeconds: 15, + failureThreshold: 2, + }); + }); + + it('allows tuning the liveness probe without a tool', () => { + // No `tool` → the probe stays liveness-only (reports `live`), but the + // interval is still configurable. + expect(buildHealthCheck({ healthCheckInterval: '300' })).toEqual({ intervalSeconds: 300 }); + }); + + it('rejects --health-check-args without a tool', () => { + expect(() => buildHealthCheck({ healthCheckArgs: '{}' })) + .toThrow(/--health-check-args requires --health-check-tool/); + }); + + it('rejects non-JSON args', () => { + expect(() => buildHealthCheck({ healthCheckTool: 't', healthCheckArgs: 'site=default' })) + .toThrow(/not valid JSON/); + }); + + it('rejects JSON args that are not an object', () => { + expect(() => buildHealthCheck({ healthCheckTool: 't', healthCheckArgs: '["a"]' })) + .toThrow(/expected a JSON object/); + }); + + it('rejects non-positive-integer timings', () => { + expect(() => buildHealthCheck({ healthCheckInterval: '0' })).toThrow(/--health-check-interval/); + expect(() => buildHealthCheck({ healthCheckTimeout: 'abc' })).toThrow(/--health-check-timeout/); + expect(() => buildHealthCheck({ healthCheckFailureThreshold: '-1' })) + .toThrow(/--health-check-failure-threshold/); + }); +}); diff --git a/src/mcpd/src/services/health-probe.service.ts b/src/mcpd/src/services/health-probe.service.ts index 8bd4192..8196f29 100644 --- a/src/mcpd/src/services/health-probe.service.ts +++ b/src/mcpd/src/services/health-probe.service.ts @@ -19,10 +19,24 @@ export const DEFAULT_HEALTH_CHECK: HealthCheckSpec = { failureThreshold: 3, }; +/** + * Which probe produced a result. + * + * - `readiness` — a real `tools/call` against the configured probe tool. It + * traverses the server's upstream dependency (controller API, database, + * remote service), so a pass means the server can actually do its job. + * - `liveness` — `tools/list` only. MCP servers answer that from a static + * in-process table, so it proves the process is up and speaking MCP and + * *nothing else*. A server whose upstream is unreachable still answers it. + */ +export type ProbeKind = 'readiness' | 'liveness'; + export interface ProbeResult { healthy: boolean; latencyMs: number; message: string; + /** Set by probeInstance from the healthCheck spec; probe helpers don't fill it. */ + probe?: ProbeKind; } interface ProbeState { @@ -118,6 +132,7 @@ export class HealthProbeRunner { const failureThreshold = healthCheck.failureThreshold ?? 3; const now = new Date(); const start = Date.now(); + const probeKind: ProbeKind = healthCheck.tool === undefined ? 'liveness' : 'readiness'; let result: ProbeResult; @@ -151,6 +166,8 @@ export class HealthProbeRunner { }; } + result.probe = probeKind; + // Update probe state const state = this.probeStates.get(instance.id) ?? { consecutiveFailures: 0, lastProbeAt: 0 }; state.lastProbeAt = Date.now(); @@ -162,18 +179,28 @@ export class HealthProbeRunner { } this.probeStates.set(instance.id, state); - // Determine health status + // Determine health status. + // + // A passing *liveness* probe reports `live`, not `healthy`. `tools/list` + // is answered from a static in-process table, so it stays green while the + // server's upstream is completely unreachable — which is exactly how a + // UniFi server whose controller port was firewalled off sat at "healthy" + // for months. Only a readiness probe (`tools/call` against a real tool) + // earns `healthy`. `live` means "process up, function unverified". const healthStatus = result.healthy - ? 'healthy' + ? (probeKind === 'readiness' ? 'healthy' : 'live') : state.consecutiveFailures >= failureThreshold ? 'unhealthy' : 'degraded'; // Build event + const probeLabel = probeKind === 'readiness' + ? `Readiness check (${healthCheck.tool})` + : 'Liveness check (tools/list)'; const eventType = result.healthy ? 'Normal' : 'Warning'; const eventMessage = result.healthy - ? `Health check passed (${result.latencyMs}ms)` - : `Health check failed: ${result.message}`; + ? `${probeLabel} passed (${result.latencyMs}ms)` + : `${probeLabel} failed: ${result.message}`; const existingEvents = (instance.events as Array<{ timestamp: string; type: string; message: string }>) ?? []; // Keep last 50 events diff --git a/src/mcpd/src/validation/template.schema.ts b/src/mcpd/src/validation/template.schema.ts index 8e4d34c..be1e3a4 100644 --- a/src/mcpd/src/validation/template.schema.ts +++ b/src/mcpd/src/validation/template.schema.ts @@ -8,7 +8,13 @@ const TemplateEnvEntrySchema = z.object({ }); export const HealthCheckSchema = z.object({ - tool: z.string().min(1), + /** + * Readiness probe tool. Omit it to keep the liveness-only default + * (`tools/list`) while still tuning interval/timeout/failureThreshold — + * a liveness pass reports `live`, not `healthy`, because `tools/list` is + * answered in-process and never touches the server's upstream. + */ + tool: z.string().min(1).optional(), arguments: z.record(z.unknown()).default({}), intervalSeconds: z.number().int().min(5).max(3600).default(60), timeoutSeconds: z.number().int().min(1).max(120).default(10), diff --git a/src/mcpd/tests/services/health-probe.test.ts b/src/mcpd/tests/services/health-probe.test.ts index 072bef9..8c33ccb 100644 --- a/src/mcpd/tests/services/health-probe.test.ts +++ b/src/mcpd/tests/services/health-probe.test.ts @@ -123,13 +123,76 @@ describe('HealthProbeRunner', () => { // No exec fallback — liveness goes through mcpProxyService expect(orchestrator.execInContainer).not.toHaveBeenCalled(); expect(mcpProxyService.execute).toHaveBeenCalledWith({ serverId: 'srv-1', method: 'tools/list' }); + // A passing liveness probe is `live`, never `healthy` — `tools/list` is + // answered in-process and proves nothing about the server's upstream. expect(instanceRepo.updateStatus).toHaveBeenCalledWith( 'inst-1', 'RUNNING', - expect.objectContaining({ healthStatus: 'healthy' }), + expect.objectContaining({ healthStatus: 'live' }), ); }); + it('reports `live` (not `healthy`) even when the upstream is dead, and says so in the event', async () => { + // The regression this guards: a UniFi server whose controller port was + // firewalled off sat at "healthy" for months because `tools/list` kept + // answering from the in-process tool table. + const instance = makeInstance(); + const server = makeServer({ healthCheck: null }); + + vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]); + vi.mocked(serverRepo.findById).mockResolvedValue(server); + + const result = await runner.probeInstance(instance, server, { intervalSeconds: 0 }); + + expect(result.healthy).toBe(true); + expect(result.probe).toBe('liveness'); + + const fields = vi.mocked(instanceRepo.updateStatus).mock.calls[0]?.[2]; + expect(fields?.healthStatus).toBe('live'); + const events = fields?.events as Array<{ message: string }>; + expect(events[events.length - 1]?.message).toContain('Liveness check (tools/list) passed'); + }); + + it('a passing readiness probe earns `healthy` and names the tool in the event', async () => { + const instance = makeInstance(); + const server = makeServer({ + healthCheck: { tool: 'list_sites', intervalSeconds: 0 } as McpServer['healthCheck'], + }); + + vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]); + vi.mocked(serverRepo.findById).mockResolvedValue(server); + vi.mocked(mcpProxyService.execute).mockResolvedValue({ jsonrpc: '2.0', id: 1, result: {} }); + + const result = await runner.probeInstance(instance, server, { tool: 'list_sites' }); + + expect(result.probe).toBe('readiness'); + const fields = vi.mocked(instanceRepo.updateStatus).mock.calls[0]?.[2]; + expect(fields?.healthStatus).toBe('healthy'); + const events = fields?.events as Array<{ message: string }>; + expect(events[events.length - 1]?.message).toContain('Readiness check (list_sites) passed'); + }); + + it('a readiness probe whose tool call fails reports the upstream error, not `live`', async () => { + const instance = makeInstance(); + const server = makeServer({ + healthCheck: { tool: 'list_sites', failureThreshold: 1 } as McpServer['healthCheck'], + }); + + vi.mocked(mcpProxyService.execute).mockResolvedValue({ + jsonrpc: '2.0', + id: 1, + error: { code: -32000, message: 'connect ETIMEDOUT 192.168.1.5:8443' }, + }); + + await runner.probeInstance(instance, server, { tool: 'list_sites', failureThreshold: 1 }); + + const fields = vi.mocked(instanceRepo.updateStatus).mock.calls[0]?.[2]; + expect(fields?.healthStatus).toBe('unhealthy'); + const events = fields?.events as Array<{ message: string }>; + expect(events[events.length - 1]?.message).toContain('Readiness check (list_sites) failed'); + expect(events[events.length - 1]?.message).toContain('ETIMEDOUT'); + }); + it('default liveness probe marks unhealthy when tools/list returns JSON-RPC error', async () => { const instance = makeInstance(); const server = makeServer({ diff --git a/src/mcplocal/tests/smoke/health-readiness.smoke.test.ts b/src/mcplocal/tests/smoke/health-readiness.smoke.test.ts new file mode 100644 index 0000000..82e266c --- /dev/null +++ b/src/mcplocal/tests/smoke/health-readiness.smoke.test.ts @@ -0,0 +1,185 @@ +/** + * Smoke tests: readiness probes actually exercise a server's upstream. + * + * The bug these guard: every instance read `healthy` forever because the + * default probe is `tools/list`, which MCP servers answer from a static + * in-process table. The UniFi server sat green for months while every call to + * its controller timed out (pod egress was capped at 80/443, controller on + * :8443) and while its `controller_type` pointed at the wrong API dialect. + * + * So these tests assert the probe is a real round trip, not a self-report: + * 1. Servers with a `healthCheck.tool` really do reach their upstream when + * that tool is called through the production proxy path. + * 2. A server with no `healthCheck.tool` reports `live`, never `healthy` — + * "process up, function unverified" must not read as "working". + * 3. `tools/list` alone cannot distinguish the two, which is why (2) matters. + * + * Prerequisites: + * - mcplocal running on localhost:3200 + * - mcpd reachable (k8s), servers deployed with readiness probes configured + */ +import { describe, it, expect, beforeAll } from 'vitest'; +import http from 'node:http'; +import https from 'node:https'; +import { existsSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { homedir } from 'node:os'; + +const CONFIG_PATH = join(homedir(), '.mcpctl', 'config.json'); +const CREDS_PATH = join(homedir(), '.mcpctl', 'credentials'); + +function loadConfig(): { mcpdUrl: string; token: string } { + let mcpdUrl = 'http://localhost:3100'; + let token = ''; + try { + if (existsSync(CONFIG_PATH)) { + const cfg = JSON.parse(readFileSync(CONFIG_PATH, 'utf-8')) as { mcpdUrl?: string }; + if (cfg.mcpdUrl) mcpdUrl = cfg.mcpdUrl; + } + if (existsSync(CREDS_PATH)) { + const creds = JSON.parse(readFileSync(CREDS_PATH, 'utf-8')) as { token?: string }; + if (creds.token) token = creds.token; + } + } catch { /* use defaults */ } + return { mcpdUrl, token }; +} + +const { mcpdUrl, token } = loadConfig(); + +function mcpdRequest(method: string, path: string, body?: unknown): Promise<{ status: number; data: T }> { + return new Promise((resolve, reject) => { + const url = new URL(path, mcpdUrl); + const transport = url.protocol === 'https:' ? https : http; + + const headers: Record = { Accept: 'application/json' }; + if (body !== undefined) headers['Content-Type'] = 'application/json'; + if (token) headers['Authorization'] = `Bearer ${token}`; + const bodyStr = body !== undefined ? JSON.stringify(body) : undefined; + if (bodyStr) headers['Content-Length'] = String(Buffer.byteLength(bodyStr)); + + const req = transport.request(url, { method, timeout: 60_000, headers, rejectUnauthorized: false }, (res) => { + const chunks: Buffer[] = []; + res.on('data', (chunk: Buffer) => chunks.push(chunk)); + res.on('end', () => { + const raw = Buffer.concat(chunks).toString(); + try { + resolve({ status: res.statusCode ?? 500, data: raw ? JSON.parse(raw) as T : (undefined as T) }); + } catch { + resolve({ status: res.statusCode ?? 500, data: raw as unknown as T }); + } + }); + }); + req.on('error', reject); + req.on('timeout', () => { req.destroy(); reject(new Error('Request timeout')); }); + if (bodyStr) req.write(bodyStr); + req.end(); + }); +} + +interface HealthCheck { + tool?: string; + arguments?: Record; +} + +interface Server { + id: string; + name: string; + healthCheck: HealthCheck | null; +} + +interface Instance { + id: string; + serverId: string; + status: string; + healthStatus: string | null; + server?: { name: string }; +} + +interface ProxyResult { + result?: { tools?: Array<{ name: string }>; isError?: boolean; content?: Array<{ text?: string }> }; + error?: { code: number; message: string }; +} + +let servers: Server[] = []; +let instances: Instance[] = []; + +beforeAll(async () => { + const s = await mcpdRequest('GET', '/api/v1/servers'); + expect(s.status, `GET /api/v1/servers returned ${s.status}`).toBe(200); + servers = s.data; + + const i = await mcpdRequest('GET', '/api/v1/instances'); + expect(i.status, `GET /api/v1/instances returned ${i.status}`).toBe(200); + instances = i.data; +}, 120_000); + +describe('readiness probes reach the upstream', () => { + it('every RUNNING server has a readiness probe configured', () => { + const running = instances.filter((i) => i.status === 'RUNNING'); + expect(running.length, 'no RUNNING instances to check').toBeGreaterThan(0); + + const withoutProbe = running + .map((i) => servers.find((s) => s.id === i.serverId)) + .filter((s): s is Server => s !== undefined) + .filter((s) => s.healthCheck?.tool === undefined) + .map((s) => s.name); + + // A server with no readiness probe can only ever report `live`. That is + // honest, but it means nothing is watching its upstream — so the fleet + // should not accumulate them silently. + expect(withoutProbe, `servers with no healthCheck.tool: ${withoutProbe.join(', ')}`).toEqual([]); + }); + + it('each configured probe tool really answers through the proxy', async () => { + const probed = servers.filter((s) => s.healthCheck?.tool !== undefined); + expect(probed.length, 'no servers have readiness probes').toBeGreaterThan(0); + + const failures: string[] = []; + for (const server of probed) { + const hc = server.healthCheck!; + const res = await mcpdRequest('POST', '/api/v1/mcp/proxy', { + serverId: server.id, + method: 'tools/call', + params: { name: hc.tool, arguments: hc.arguments ?? {} }, + }); + if (res.status !== 200) { + failures.push(`${server.name}/${hc.tool}: HTTP ${res.status}`); + continue; + } + if (res.data.error) { + failures.push(`${server.name}/${hc.tool}: ${res.data.error.message}`); + continue; + } + if (res.data.result?.isError === true) { + failures.push(`${server.name}/${hc.tool}: ${res.data.result.content?.[0]?.text ?? 'isError'}`); + } + } + + // When this fails, the server is genuinely broken — fix the environment + // (credentials, egress, upstream address), never the assertion. + expect(failures, `readiness probe tools failing: ${failures.join(' | ')}`).toEqual([]); + }, 300_000); + + it('a probe tool is a different call from tools/list, and both are reachable', async () => { + const server = servers.find((s) => s.healthCheck?.tool !== undefined); + expect(server, 'need at least one probed server').toBeDefined(); + + const list = await mcpdRequest('POST', '/api/v1/mcp/proxy', { + serverId: server!.id, + method: 'tools/list', + }); + expect(list.status).toBe(200); + const toolNames = (list.data.result?.tools ?? []).map((t) => t.name); + // The probe must name a tool the server actually exposes, otherwise the + // readiness check fails for a bookkeeping reason rather than a real one. + expect(toolNames).toContain(server!.healthCheck!.tool); + }, 120_000); + + it('no RUNNING instance is stuck at an unknown health status', () => { + const stuck = instances + .filter((i) => i.status === 'RUNNING') + .filter((i) => i.healthStatus === null || i.healthStatus === 'unknown') + .map((i) => i.server?.name ?? i.serverId); + expect(stuck, `instances with no health verdict: ${stuck.join(', ')}`).toEqual([]); + }); +}); -- 2.49.1 From 732ca98ccc8c243e277d8efbbebafb960ba8f750 Mon Sep 17 00:00:00 2001 From: Michal Date: Sun, 9 Aug 2026 22:54:33 +0100 Subject: [PATCH 2/3] docs(reliability): record the three shapes a failing readiness probe takes Turning readiness probes on took the fleet from 8/8 healthy to three real failures in under a minute, and all three were network shape rather than code: an egress port (UniFi :8443), an ingress hairpin through the Envoy L7 policy (Grafana 403 `Access denied` with a token that worked from a laptop), and a Tailscale address a pod can never reach (Node-RED, since retired). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114dg56YmVacyqhp5fitcTb --- docs/reliability.md | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/docs/reliability.md b/docs/reliability.md index 5c1f2c5..30423c8 100644 --- a/docs/reliability.md +++ b/docs/reliability.md @@ -76,17 +76,29 @@ local table, not crossing a network. The UniFi probe went from 3ms (`tools/list` lying) to 1847ms on its first real `list_sites` — login, TLS, controller round trip — and ~40ms once the session was warm. -### Two failure modes the probe cannot see for you +### Where a failing readiness probe usually points -The healthy-looking UniFi server was broken **twice over**, and both are worth -checking first when a readiness probe starts failing: +Turning these probes on for the first time took the fleet from "8/8 healthy" to +three genuine failures in under a minute. All three were network shape, not +code — check these before suspecting the server: -1. **Egress.** MCP server pods default to TCP 80/443 only +1. **Egress port.** MCP server pods default to TCP 80/443 only (`servers-allow-external-egress`). Any upstream on another port — the UniFi controller on `:8443` — times out on every call. Declare it in Pulumi's - `mcpctl.serverEgressTargets` (name + `/32` + ports); don't widen the blanket rule. -2. **Address reachability.** A pod cannot reach a **Tailscale** `100.64.0.0/10` - address. Config pointing at one connect-timeouts forever. Use LAN IPs. + `mcpctl.serverEgressTargets`; don't widen the blanket rule. +2. **Ingress hairpin.** A co-located service reached over its *public* hostname + goes out and back through the per-host Envoy L7 policy, which doesn't + reliably carry the caller's identity and replies with a bare `Access denied`. + Grafana 403'd on every call this way while the identical token succeeded from + a laptop. The tell is the error *shape*: plain text, not the upstream's own + JSON error. Use the ClusterIP (`serverEgressTargets` with `namespace:`). +3. **Address reachability.** A pod cannot reach a **Tailscale** `100.64.0.0/10` + address. Config pointing at one connect-timeouts forever. Use LAN IPs. (This + one turned out to be a retired service, which is its own kind of answer.) + +Also check the *dialect*: UniFi's `controller_type` must be `classic` for a +self-hosted controller (login `/api/login`, no `/proxy/network` prefix). +`unifi_os` sends every request to a path that 404s. ## LLM-*essential* operations — failover chain -- 2.49.1 From a158e49ec24bef57b9a7f2b3888b47038445b579 Mon Sep 17 00:00:00 2001 From: Michal Date: Sun, 9 Aug 2026 23:53:22 +0100 Subject: [PATCH 3/3] fix(templates): make the shipped templates match reality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The templates are what `create server --from-template` builds from and what mcpd seeds on start, so drift there ships broken servers. Nothing ever read these files in a test, and they had rotted badly. - grafana: GRAFANA_URL now defaults to the in-cluster ClusterIP and the description spells out why the public hostname is wrong — reaching a co-located Grafana over its ingress hairpins through the per-host Envoy L7 policy, which drops the caller's identity and returns a bare `Access denied` 403 with a perfectly valid token. That cost a day of looking at the token. - unifi-network: was wrong on every field that mattered. `runtime: python` for an npm package, an env contract (UNIFI_HOST/USERNAME/PASSWORD) the package doesn't read, and no probe. Now UNIFI_TARGETS with the classic-vs-unifi_os distinction and the :8443 egress caveat written down. - docmost, gitea: both carried "health check disabled" comments citing a limitation of the old docker-exec probe, which readiness-via-proxy removed. Both probes verified against the live servers. gitea uses search_repos, not get_me, because get_me needs a `read:user` scope a repo-scoped token lacks. - filesystem: packageName was `@anthropic/filesystem-mcp`, which 404s on npm — the template could never have installed. Points at the real package. - terraform: deleted. `@anthropic/terraform-mcp` 404s too and there is no npm-published replacement to point it at. - node-red: deleted, the service is gone. Two supporting fixes: - The seeder declared no `runtime` field and never wrote the column, so a template asking for the python runner silently seeded as null and got node. - A new templates test reads every shipped file: schema-valid, a runner the orchestrator knows, some way to actually start, unique env names, and a readiness probe (without one an instance can only ever report `live`). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0114dg56YmVacyqhp5fitcTb --- README.md | 2 +- src/db/src/seed/index.ts | 9 ++++ src/mcpd/tests/templates.test.ts | 79 ++++++++++++++++++++++++++++++++ templates/docmost.yaml | 11 ++++- templates/filesystem.yaml | 21 ++++++++- templates/gitea.yaml | 13 +++++- templates/grafana.yaml | 16 ++++++- templates/node-red.yaml | 16 ------- templates/terraform.yaml | 6 --- templates/unifi-network.yaml | 53 +++++++++++++-------- 10 files changed, 178 insertions(+), 48 deletions(-) create mode 100644 src/mcpd/tests/templates.test.ts delete mode 100644 templates/node-red.yaml delete mode 100644 templates/terraform.yaml diff --git a/README.md b/README.md index 16ead01..71600b6 100644 --- a/README.md +++ b/README.md @@ -397,7 +397,7 @@ name: home-automation proxyModel: default servers: - home-assistant - - node-red + - unifi-network ``` Via CLI: diff --git a/src/db/src/seed/index.ts b/src/db/src/seed/index.ts index e8872e3..6c6dfb5 100644 --- a/src/db/src/seed/index.ts +++ b/src/db/src/seed/index.ts @@ -21,6 +21,13 @@ export interface SeedTemplate { version: string; description: string; packageName?: string; + /** + * Package runner: 'node' (npx) or 'python' (uvx). McpTemplate has had this + * column all along, but the upsert below never wrote it, so a template + * declaring `runtime: python` seeded as null and every server created from + * it silently got the node runner. + */ + runtime?: string; dockerImage?: string; transport: 'STDIO' | 'SSE' | 'STREAMABLE_HTTP'; repositoryUrl?: string; @@ -45,6 +52,7 @@ export async function seedTemplates( version: tpl.version, description: tpl.description, packageName: tpl.packageName ?? null, + runtime: tpl.runtime ?? null, dockerImage: tpl.dockerImage ?? null, transport: tpl.transport, repositoryUrl: tpl.repositoryUrl ?? null, @@ -60,6 +68,7 @@ export async function seedTemplates( version: tpl.version, description: tpl.description, packageName: tpl.packageName ?? null, + runtime: tpl.runtime ?? null, dockerImage: tpl.dockerImage ?? null, transport: tpl.transport, repositoryUrl: tpl.repositoryUrl ?? null, diff --git a/src/mcpd/tests/templates.test.ts b/src/mcpd/tests/templates.test.ts new file mode 100644 index 0000000..a597a47 --- /dev/null +++ b/src/mcpd/tests/templates.test.ts @@ -0,0 +1,79 @@ +/** + * The shipped `templates/*.yaml` are seeded into mcpd and are what `mcpctl + * create server --from-template` builds from, so drift there ships broken + * servers. The unifi-network template had drifted on every field that + * mattered — python runtime for an npm package, an env contract + * (UNIFI_HOST/USERNAME/PASSWORD) the package doesn't read, and a comment + * disabling its health check for a reason that had stopped being true — and + * nothing caught it because no test ever read the files. + */ +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import yaml from 'js-yaml'; +import { CreateTemplateSchema } from '../src/validation/template.schema.js'; + +const TEMPLATES_DIR = fileURLToPath(new URL('../../../templates', import.meta.url)); + +const files = readdirSync(TEMPLATES_DIR).filter((f) => f.endsWith('.yaml') || f.endsWith('.yml')); + +interface RawTemplate { + name?: string; + runtime?: string; + packageName?: string; + dockerImage?: string; + externalUrl?: string; + healthCheck?: { tool?: string }; + env?: Array<{ name?: string }>; +} + +function load(file: string): RawTemplate { + return yaml.load(readFileSync(join(TEMPLATES_DIR, file), 'utf-8')) as RawTemplate; +} + +describe('shipped templates', () => { + it('ships at least one template', () => { + expect(files.length).toBeGreaterThan(0); + }); + + it.each(files)('%s validates against CreateTemplateSchema', (file) => { + const parsed = CreateTemplateSchema.safeParse(load(file)); + expect(parsed.success ? null : parsed.error.issues).toBeNull(); + }); + + it.each(files)('%s declares a runner the orchestrator knows', (file) => { + const tpl = load(file); + // `runtime` only means anything for package-based servers, and only + // 'node' (npx) and 'python' (uvx) are wired in buildRuntimeSpawnCmd. + if (tpl.runtime !== undefined) { + expect(['node', 'python']).toContain(tpl.runtime); + } + }); + + it.each(files)('%s says how to actually run the server', (file) => { + const tpl = load(file); + const runnable = tpl.packageName !== undefined + || tpl.dockerImage !== undefined + || tpl.externalUrl !== undefined; + expect(runnable, `${file} has no packageName, dockerImage, or externalUrl`).toBe(true); + }); + + it.each(files)('%s names a readiness probe tool, not a bare liveness probe', (file) => { + const tpl = load(file); + // Without a `tool`, an instance from this template can only ever report + // `live` — nothing would ever check its upstream. See docs/reliability.md. + expect(tpl.healthCheck?.tool, `${file} has no healthCheck.tool`).toBeTruthy(); + }); + + it.each(files)('%s declares uniquely-named env entries', (file) => { + const names = (load(file).env ?? []).map((e) => e.name); + expect(new Set(names).size).toBe(names.length); + }); + + it('has no template for a retired server', () => { + // node-red was retired 2026-08-09: it answered on neither its Tailscale + // nor its LAN address and had no deployment anywhere. + expect(files).not.toContain('node-red.yaml'); + }); +}); diff --git a/templates/docmost.yaml b/templates/docmost.yaml index 10d9df1..e608d1c 100644 --- a/templates/docmost.yaml +++ b/templates/docmost.yaml @@ -4,8 +4,15 @@ description: Docmost MCP server for wiki/documentation page management and searc dockerImage: "mysources.co.uk/michal/docmost-mcp:latest" transport: STDIO repositoryUrl: https://github.com/MrMartiniMo/docmost-mcp -# Health check disabled: STDIO health probe requires packageName (npm-based servers). -# This server uses a custom dockerImage. Probe support for dockerImage STDIO servers is TODO. +healthCheck: + # get_workspace calls the Docmost API, so a pass proves URL + login. The old + # "probe requires packageName" caveat here was true of the long-gone + # docker-exec probe; readiness now goes through the MCP proxy, which works + # the same for image-based STDIO servers. Verified against the live server. + tool: get_workspace + arguments: {} + intervalSeconds: 60 + timeoutSeconds: 10 env: - name: DOCMOST_API_URL description: Docmost API URL (e.g. http://100.88.157.6:3000/api) diff --git a/templates/filesystem.yaml b/templates/filesystem.yaml index 4c14e7b..cb0c1c9 100644 --- a/templates/filesystem.yaml +++ b/templates/filesystem.yaml @@ -1,6 +1,23 @@ name: filesystem -version: "1.0.0" +version: "2.0.0" description: Filesystem MCP server for reading and writing files -packageName: "@anthropic/filesystem-mcp" +# Was "@anthropic/filesystem-mcp", which 404s on the npm registry — creating a +# server from this template failed at install. This is the real package. +packageName: "@modelcontextprotocol/server-filesystem" +runtime: node transport: STDIO repositoryUrl: https://github.com/modelcontextprotocol/servers/tree/main/src/filesystem +healthCheck: + # Zero-arg and proves the server resolved its configured roots, which is the + # only thing it can be misconfigured about. + tool: list_allowed_directories + arguments: {} + intervalSeconds: 60 + timeoutSeconds: 10 +env: + - name: ALLOWED_DIRECTORIES + description: >- + Space-separated directories the server may access. Passed as the + package's positional arguments; without at least one the server exposes + nothing. + required: true diff --git a/templates/gitea.yaml b/templates/gitea.yaml index 9ded49c..c41dbd9 100644 --- a/templates/gitea.yaml +++ b/templates/gitea.yaml @@ -7,7 +7,18 @@ repositoryUrl: https://gitea.com/gitea/gitea-mcp # No command: the image's entrypoint IS the MCP server. mcpd attaches to PID 1 # stdin/stdout (attach mode) rather than exec-ing a new process. The image is # distroless and has no node/shell, so exec-based STDIO would fail. -# Health check disabled: STDIO health probe requires node in the container. +healthCheck: + # search_repos is a real Gitea API call, deliberately chosen over get_me: + # get_me needs the `read:user` token scope, which a repo-scoped token won't + # have, so it would fail for a bookkeeping reason rather than a real one. + # (The "probe requires node in the container" caveat that used to sit here + # described the old docker-exec probe; readiness goes through the MCP proxy + # now, so a distroless image is fine.) Verified against the live server. + tool: search_repos + arguments: + query: mcpctl + intervalSeconds: 60 + timeoutSeconds: 10 env: - name: GITEA_HOST description: Gitea instance URL (e.g. https://gitea.example.com) diff --git a/templates/grafana.yaml b/templates/grafana.yaml index 2bf6992..f8445b7 100644 --- a/templates/grafana.yaml +++ b/templates/grafana.yaml @@ -1,16 +1,28 @@ name: grafana -version: "1.0.0" +version: "1.1.0" description: Grafana MCP server for dashboards, datasources, and alerts packageName: "@leval/mcp-grafana" +runtime: node transport: STDIO repositoryUrl: https://github.com/levalhq/mcp-grafana healthCheck: + # Hits the Grafana API, so a pass proves URL + token + reachability. A + # liveness probe (tools/list) cannot: it answers from the server's own tool + # table and stays green while every Grafana call 403s. tool: list_datasources arguments: {} + intervalSeconds: 60 + timeoutSeconds: 10 env: - name: GRAFANA_URL - description: Grafana instance URL (e.g. https://grafana.example.com) + description: >- + Grafana base URL. For a Grafana in this cluster use its ClusterIP + (http://grafana..svc.cluster.local:3000) — NOT its public + hostname. Reaching it over the public ingress hairpins the request back + through the per-host Envoy L7 policy, which drops the caller's identity + and answers a bare `Access denied` 403 even when the token is valid. required: true + defaultValue: http://grafana.home-automation.svc.cluster.local:3000 - name: GRAFANA_SERVICE_ACCOUNT_TOKEN description: Grafana service account token (glsa_...) required: true diff --git a/templates/node-red.yaml b/templates/node-red.yaml deleted file mode 100644 index d81b749..0000000 --- a/templates/node-red.yaml +++ /dev/null @@ -1,16 +0,0 @@ -name: node-red -version: "1.0.0" -description: Node-RED MCP server for flow management and automation -packageName: "mcp-node-red" -transport: STDIO -repositoryUrl: https://github.com/fx/mcp-node-red -healthCheck: - tool: get_settings - arguments: {} -env: - - name: NODE_RED_URL - description: Node-RED instance URL (e.g. http://nodered.local:1880) - required: true - - name: NODE_RED_TOKEN - description: Node-RED access token (optional if no auth) - required: false diff --git a/templates/terraform.yaml b/templates/terraform.yaml deleted file mode 100644 index 9fd4049..0000000 --- a/templates/terraform.yaml +++ /dev/null @@ -1,6 +0,0 @@ -name: terraform -version: "1.0.0" -description: Terraform MCP server for infrastructure documentation and state -packageName: "@anthropic/terraform-mcp" -transport: STDIO -repositoryUrl: https://github.com/modelcontextprotocol/servers/tree/main/src/terraform diff --git a/templates/unifi-network.yaml b/templates/unifi-network.yaml index 8d5c14a..242309d 100644 --- a/templates/unifi-network.yaml +++ b/templates/unifi-network.yaml @@ -1,25 +1,42 @@ name: unifi-network -version: "1.0.0" +version: "2.0.0" description: UniFi Network MCP server for managing UniFi network devices, clients, and configuration packageName: "unifi-network-mcp" -runtime: python +runtime: node transport: STDIO repositoryUrl: https://github.com/sirkirby/unifi-mcp -# Health check disabled: STDIO health probe requires packageName (npm-based servers). -# This server uses the Python runner. Probe support for Python runner STDIO servers is TODO. +healthCheck: + # list_sites calls the controller (/api/self/sites), so a pass proves the + # whole path: egress to the controller port, TLS, login, session. The old + # template disabled the probe entirely on the belief that STDIO probes only + # worked for npm packages — that stopped being true once readiness probes + # started going through the MCP proxy, and the gap let this server sit at + # "healthy" for months without ever reaching the controller. + tool: list_sites + arguments: {} + intervalSeconds: 60 + timeoutSeconds: 15 env: - - name: UNIFI_HOST - description: UniFi controller hostname or IP (e.g. unifi.example.com — without https://) + - name: UNIFI_TARGETS + description: >- + JSON array of controllers. One object per controller: + {"id", "base_url", "controller_type", "default_site", "auth": + {"username","password"}, "verify_ssl"}. + + controller_type is "classic" for a self-hosted UniFi Network controller + (login /api/login, no path prefix) or "unifi_os" for a UDM/UniFi OS + console (login /api/auth/login, API under /proxy/network). Choosing the + wrong one sends every request to a path that 404s while the server still + starts cleanly. + + base_url must carry the real controller port — a self-hosted controller + is usually :8443, and :443 on the same host is often an unrelated + service. Note that MCP server pods only egress 80/443 by default, so any + other port needs an explicit NetworkPolicy (Pulumi + `mcpctl.serverEgressTargets`). required: true - - name: UNIFI_USERNAME - description: UniFi local admin username - required: true - - name: UNIFI_PASSWORD - description: UniFi admin password - required: true - - name: UNIFI_NETWORK_PORT - description: UniFi controller port (default 443, use 8443 for standalone UniFi Controller) - required: false - - name: UNIFI_NETWORK_VERIFY_SSL - description: Verify SSL certificate (true/false, default true — set false for self-signed certs) - required: false + defaultValue: >- + [{"id": "home", "base_url": "https://unifi.example.com:8443", + "controller_type": "classic", "default_site": "default", + "auth": {"username": "CHANGE_ME", "password": "CHANGE_ME"}, + "verify_ssl": false}] -- 2.49.1