Merge remote-tracking branch 'origin/main' into feat/web-search-templates

# Conflicts:
#	completions/mcpctl.bash
#	completions/mcpctl.fish
#	src/cli/src/commands/create.ts
#	src/db/src/seed/index.ts

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

src/mcpd/src/validation/template.schema.ts

@@ -24,7 +24,13 @@ export const VolumeSpecSchema = z.object({
 export type VolumeSpecInput = z.infer<typeof VolumeSpecSchema>;
 
 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),

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({

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');
+  });
+});