Merge pull request 'fix(secrets): injector-delivered servers must attach, not exec' (#120) from feat/injector-attach-mode into main

With injected delivery the credentials exist ONLY in PID 1's environment:
the container command is a shell that sources /vault/secrets/<name> and
execs the real server, so the values live in that process and nowhere
else — not in the pod spec, which is the whole point.

mcpd picks its STDIO mode from the server's shape: an explicit command or
a packageName selects `exec`, a bare dockerImage selects `attach`. `exec`
spawns a NEW process inside the container, which never sourced the file
and therefore starts with empty credentials. The server comes up, answers
tools/list, and fails every authenticated call — precisely the silent
empty-token failure this feature exists to prevent.

Seen as `Readiness check (list_datasources) failed: process exited 1` on a
pod whose PID 1 demonstrably held the token (verified via /proc/1/environ:
GRAFANA_URL set, token 46 chars, nothing in the pod spec).

So secretDelivery: injector now forces attach regardless of server shape.
Safe because wrapCommandForInjector execs rather than forks, so PID 1 IS
the server. It also means no bouncer or HTTP shim is needed — mcpd's
PersistentStdio already holds one long-lived connection; it was simply
pointed at the wrong process.

Note this inverts an assumption I had earlier: image-entrypoint servers
were already on the attach path and would have worked; it is the
package-based ones that were broken. gitea remains the sole exception, and
for the unrelated reason that it is distroless and has no shell to source
the file.

Extracts the decision as a pure `chooseStdioMode()` so it can be tested —
it is subtle and fails silently. Six cases pinned, including that env
delivery still execs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki

src/mcpd/src/repositories/mcp-server.repository.ts

@@ -34,6 +34,8 @@ export class McpServerRepository implements IMcpServerRepository {
         env: data.env,
         healthCheck: (data.healthCheck ?? Prisma.JsonNull) as Prisma.InputJsonValue,
         volumes: data.volumes,
+        secretDelivery: data.secretDelivery,
+        entrypoint: (data.entrypoint ?? Prisma.DbNull) as Prisma.InputJsonValue,
       },
     });
   }
@@ -53,6 +55,8 @@ export class McpServerRepository implements IMcpServerRepository {
     if (data.env !== undefined) updateData['env'] = data.env;
     if (data.healthCheck !== undefined) updateData['healthCheck'] = (data.healthCheck ?? Prisma.JsonNull) as Prisma.InputJsonValue;
     if (data.volumes !== undefined) updateData['volumes'] = data.volumes;
+    if (data.secretDelivery !== undefined) updateData['secretDelivery'] = data.secretDelivery;
+    if (data.entrypoint !== undefined) updateData['entrypoint'] = (data.entrypoint ?? Prisma.JsonNull) as Prisma.InputJsonValue;
 
     return this.prisma.mcpServer.update({ where: { id }, data: updateData });
   }

src/mcpd/src/services/instance.service.ts

@@ -564,8 +564,13 @@ export class InstanceService {
           spec.serviceAccountName = identity;
           spec.automountServiceAccountToken = true;
           spec.annotations = this.serverIdentity!.annotationsFor(identity, spec.envFromSecret);
+          // Replaces the image entrypoint (see wrapCommand); the original argv
+          // is folded in, so spec.command must not also be emitted as args.
           const wrapped = this.serverIdentity!.wrapCommand(server, spec.command);
-          if (wrapped !== undefined) spec.command = wrapped;
+          if (wrapped !== undefined) {
+            spec.entrypoint = wrapped;
+            delete spec.command;
+          }
         } catch (idErr) {
           const msg = idErr instanceof Error ? idErr.message : String(idErr);
           return this.markInstanceError(instance, `secret identity provisioning failed: ${msg}`);

src/mcpd/src/services/k8s/manifest-generator.ts

@@ -259,7 +259,12 @@ function buildContainerSpec(spec: ContainerSpec) {
   // In Docker, spec.command maps to Cmd (args to entrypoint).
   // In k8s, we use `args` to pass arguments to the image's entrypoint,
   // preserving the runner image's entrypoint (uvx, npx -y, etc.)
-  if (spec.command && spec.command.length > 0) {
+  //
+  // `entrypoint` is the exception: it REPLACES the entrypoint (k8s `command`),
+  // which injected secret delivery needs so the sourcing shell can be PID 1.
+  if (spec.entrypoint && spec.entrypoint.length > 0) {
+    container.command = spec.entrypoint;
+  } else if (spec.command && spec.command.length > 0) {
     container.args = spec.command;
   }
 

src/mcpd/src/services/mcp-proxy-service.ts

@@ -62,6 +62,59 @@ function parseStreamableResponse(body: string): McpProxyResponse {
   return JSON.parse(body) as McpProxyResponse;
 }
 
+/**
+ * Decide how mcpd opens a STDIO session against a running container.
+ *
+ *   attach → connect to PID 1's stdin/stdout
+ *   exec   → spawn a NEW process inside the container
+ *
+ * Pure and exported so the choice is testable: it is subtle, and getting it
+ * wrong fails silently rather than loudly (see the injector case below).
+ */
+export function chooseStdioMode(server: {
+  name: string;
+  id: string;
+  secretDelivery?: string | null;
+  command?: string[] | null;
+  packageName?: string | null;
+  dockerImage?: string | null;
+  runtime?: string | null;
+}): StdioMode {
+  // Injected delivery MUST attach, whatever the server type.
+  //
+  // The secrets exist only in PID 1's environment: the container command is a
+  // shell that sources /vault/secrets/<name> and execs the real server, so the
+  // values live in that process and nowhere else — not in the pod spec, which
+  // is the entire point of the feature.
+  //
+  // `exec` spawns a NEW process, which never sourced the file and so starts
+  // with empty credentials. The server comes up, answers tools/list, and fails
+  // every authenticated call — the exact silent-empty-token failure this
+  // feature exists to prevent. Observed as
+  // `Readiness check (list_datasources) failed: process exited 1` on a pod
+  // whose PID 1 demonstrably held the token.
+  //
+  // Safe because wrapCommandForInjector execs rather than forks, so PID 1 IS
+  // the server process.
+  if (server.secretDelivery === 'injector') return { kind: 'attach' };
+
+  if (server.command !== null && server.command !== undefined && server.command.length > 0) {
+    return { kind: 'exec', command: server.command };
+  }
+  if (server.packageName !== null && server.packageName !== undefined && server.packageName !== '') {
+    return { kind: 'exec', command: buildRuntimeSpawnCmd(server.runtime ?? 'node', server.packageName) };
+  }
+  // Image entrypoint IS the MCP server.
+  if (server.dockerImage !== null && server.dockerImage !== undefined && server.dockerImage !== '') {
+    return { kind: 'attach' };
+  }
+
+  throw new InvalidStateError(
+    `Server '${server.name}' (${server.id}) uses STDIO transport but has no ` +
+    `packageName, command, or dockerImage. Configure one of these.`,
+  );
+}
+
 export class McpProxyService {
   /** Session IDs per server for streamable-http protocol */
   private sessions = new Map<string, string>();
@@ -159,20 +212,15 @@ export class McpProxyService {
       //   - command set      → exec the given command in the container.
       //   - dockerImage only → attach to PID 1 (image entrypoint IS the MCP server).
       //   - nothing          → unreachable, reject.
-      const runtime = (server.runtime as string | null) ?? 'node';
-      let mode: StdioMode;
-      if (command && command.length > 0) {
-        mode = { kind: 'exec', command };
-      } else if (packageName) {
-        mode = { kind: 'exec', command: buildRuntimeSpawnCmd(runtime, packageName) };
-      } else if (dockerImage) {
-        mode = { kind: 'attach' };
-      } else {
-        throw new InvalidStateError(
-          `Server '${server.name}' (${server.id}) uses STDIO transport but has no ` +
-          `packageName, command, or dockerImage. Configure one of these.`,
-        );
-      }
+      const mode = chooseStdioMode({
+        name: server.name as string,
+        id: server.id as string,
+        secretDelivery: server.secretDelivery as string | null,
+        command,
+        packageName,
+        dockerImage,
+        runtime: server.runtime as string | null,
+      });
 
       // Try persistent connection first
       try {
@@ -181,7 +229,7 @@ export class McpProxyService {
         this.removeClient(instance.containerId);
         // Fall back to one-shot exec when we have a command to run.
         if (mode.kind === 'exec') {
-          return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, runtime);
+          return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, (server.runtime as string | null) ?? 'node');
         }
         // Attach mode has no one-shot equivalent, but the failure is usually
         // a stale pipe from an in-place container restart — retry once

src/mcpd/src/services/orchestrator.ts

@@ -62,6 +62,17 @@ export interface ContainerSpec {
   serviceAccountName?: string;
   /** Injected agents need the projected SA token; plain servers do not. */
   automountServiceAccountToken?: boolean;
+  /**
+   * REPLACES the image's ENTRYPOINT (k8s `command`), unlike `command`, which is
+   * appended to it as `args`.
+   *
+   * Needed only for injected secret delivery: the container has to run a shell
+   * that sources the rendered file before exec'ing the real process, and that
+   * shell must BE the entrypoint. Because it replaces the entrypoint, the argv
+   * here has to include whatever the image's entrypoint would have contributed
+   * (`npx -y`, `uvx`, ...).
+   */
+  entrypoint?: string[];
   /** Host port to bind (null = auto-assign) */
   hostPort?: number | null;
   /** Container port to expose */

src/mcpd/src/services/server-identity.service.ts

@@ -96,26 +96,6 @@ export class ServerIdentityService {
     });
   }
 
-  /**
-   * Rewrite argv so the rendered secrets are sourced before the server runs.
-   *
-   * `command` is what mcpd already computed: for package-based servers that is
-   * the runner image's entrypoint plus the package, which mcpd owns. For a
-   * dockerImage server it may be absent — the image's own ENTRYPOINT would run,
-   * and mcpd cannot introspect it, which is why `entrypoint` is required on the
-   * server row in that case (enforced at validation).
-   */
-  wrapCommand(
-    server: Pick<McpServer, 'env' | 'entrypoint'>,
-    command: string[] | undefined,
-  ): string[] | undefined {
-    const secretNames = this.secretNamesFor(server);
-    if (secretNames.length === 0) return command;
-    const argv = command ?? (server.entrypoint as string[] | null) ?? undefined;
-    if (argv === undefined || argv.length === 0) return command;
-    return wrapCommandForInjector(argv, secretNames);
-  }
-
   /** Identity name for a server — also the SA, policy and role name. */
   identityNameFor(serverName: string): string {
     return `${IDENTITY_PREFIX}${serverName}`;
@@ -135,6 +115,39 @@ export class ServerIdentityService {
     return [...names].sort((a, b) => a.localeCompare(b));
   }
 
+  /**
+   * Build the container ENTRYPOINT that sources the rendered secrets and then
+   * execs the real server.
+   *
+   * This must REPLACE the image's entrypoint, not extend it. mcpd normally puts
+   * a package server's argv into k8s `args` so the runner image's `npx -y` /
+   * `uvx` entrypoint still runs; a wrapper placed there would be executed BY
+   * npx (`npx -y /bin/sh -c ...`) and fail. So the argv returned here folds in
+   * whatever the image's entrypoint would have contributed.
+   *
+   * Returns undefined when there is nothing to wrap, leaving the pod on the
+   * normal entrypoint+args path.
+   */
+  wrapCommand(
+    server: Pick<McpServer, 'env' | 'entrypoint' | 'packageName' | 'runtime'>,
+    command: string[] | undefined,
+  ): string[] | undefined {
+    const secretNames = this.secretNamesFor(server);
+    if (secretNames.length === 0) return undefined;
+
+    // mcpd owns the runner images, so their entrypoints are known. A
+    // dockerImage server's is not introspectable — hence `entrypoint` being
+    // required on the row at validation time.
+    const imageEntrypoint = server.packageName
+      ? server.runtime === 'python'
+        ? ['uvx']
+        : ['npx', '-y']
+      : ((server.entrypoint as string[] | null) ?? undefined);
+    if (imageEntrypoint === undefined || imageEntrypoint.length === 0) return undefined;
+
+    return wrapCommandForInjector([...imageEntrypoint, ...(command ?? [])], secretNames);
+  }
+
   /**
    * Converge the identity for one server. Returns the identity name so the
    * caller can stamp it onto the pod spec.

src/mcpd/tests/injector-manifest.test.ts

@@ -136,3 +136,23 @@ describe('shell quoting survives adversarial values', () => {
     expect(out).toBe('');
   });
 });
+
+describe('the wrapper must REPLACE the image entrypoint, not extend it', () => {
+  // Regression: mcpd maps ContainerSpec.command -> k8s `args` so the runner
+  // image's `npx -y` / `uvx` entrypoint still runs. Emitting the wrapper there
+  // meant the pod actually ran `npx -y /bin/sh -c '...'`, which crashlooped.
+  it('emits container.command (entrypoint) and no args', () => {
+    const pod = generatePodSpec({
+      name: 'g', image: 'runner',
+      entrypoint: ['/bin/sh', '-c', '. /vault/secrets/s; exec "$0" "$@"', 'npx', '-y', '@leval/mcp-grafana'],
+    } as ContainerSpec, 'mcpctl-servers');
+    expect(pod.spec.containers[0]?.command?.[0]).toBe('/bin/sh');
+    expect(pod.spec.containers[0]?.args).toBeUndefined();
+  });
+
+  it('leaves the normal entrypoint+args path alone when not wrapping', () => {
+    const pod = generatePodSpec({ name: 'g', image: 'runner', command: ['@leval/mcp-grafana'] } as ContainerSpec, 'mcpctl-servers');
+    expect(pod.spec.containers[0]?.command).toBeUndefined();
+    expect(pod.spec.containers[0]?.args).toEqual(['@leval/mcp-grafana']);
+  });
+});

src/mcpd/tests/mcp-server-repository-fields.test.ts

@@ -0,0 +1,48 @@
+/**
+ * The server repository maps update/create fields explicitly, field by field.
+ * That means a new column silently does nothing until it is added here — and
+ * the failure is invisible: `mcpctl patch server x secretDelivery=injector`
+ * returns "patched" while the value never changes.
+ *
+ * Caught exactly that way in production. These assert the mapping instead.
+ */
+import { describe, it, expect, vi } from 'vitest';
+import { McpServerRepository } from '../src/repositories/mcp-server.repository.js';
+import type { PrismaClient } from '@prisma/client';
+
+function prismaSpy() {
+  const update = vi.fn(async ({ data }: { data: Record<string, unknown> }) => data);
+  const create = vi.fn(async ({ data }: { data: Record<string, unknown> }) => data);
+  return { spy: { mcpServer: { update, create } } as unknown as PrismaClient, update, create };
+}
+
+describe('McpServerRepository field mapping', () => {
+  it('persists secretDelivery on update', async () => {
+    const { spy, update } = prismaSpy();
+    await new McpServerRepository(spy).update('id1', { secretDelivery: 'injector' });
+    expect(update.mock.calls[0]?.[0].data).toMatchObject({ secretDelivery: 'injector' });
+  });
+
+  it('persists entrypoint on update', async () => {
+    const { spy, update } = prismaSpy();
+    await new McpServerRepository(spy).update('id1', { entrypoint: ['/bin/x', '--flag'] });
+    expect(update.mock.calls[0]?.[0].data).toMatchObject({ entrypoint: ['/bin/x', '--flag'] });
+  });
+
+  it('leaves both untouched when not supplied', async () => {
+    const { spy, update } = prismaSpy();
+    await new McpServerRepository(spy).update('id1', { description: 'x' });
+    const data = update.mock.calls[0]?.[0].data ?? {};
+    expect(data).not.toHaveProperty('secretDelivery');
+    expect(data).not.toHaveProperty('entrypoint');
+  });
+
+  it('persists secretDelivery on create', async () => {
+    const { spy, create } = prismaSpy();
+    await new McpServerRepository(spy).create({
+      name: 'x', description: '', transport: 'STDIO', replicas: 1, env: [], volumes: [],
+      secretDelivery: 'injector',
+    } as never);
+    expect(create.mock.calls[0]?.[0].data).toMatchObject({ secretDelivery: 'injector' });
+  });
+});

src/mcpd/tests/stdio-mode.test.ts

@@ -0,0 +1,44 @@
+/**
+ * How mcpd opens a STDIO session: attach to PID 1, or exec a new process.
+ *
+ * Subtle and silent when wrong. With injected secret delivery the credentials
+ * exist ONLY in PID 1's environment (a shell sourced /vault/secrets/<name> and
+ * exec'd the server), so an `exec` starts a process with empty credentials —
+ * the server comes up, answers tools/list, and fails every authenticated call.
+ */
+import { describe, it, expect } from 'vitest';
+import { chooseStdioMode } from '../src/services/mcp-proxy-service.js';
+
+const base = { name: 's', id: 'id1' };
+
+describe('chooseStdioMode', () => {
+  it('attaches for an injector server even though it has a packageName', () => {
+    // The regression: packageName would otherwise select exec, and exec loses
+    // the secrets entirely.
+    expect(chooseStdioMode({ ...base, secretDelivery: 'injector', packageName: '@leval/mcp-grafana' }))
+      .toEqual({ kind: 'attach' });
+  });
+
+  it('attaches for an injector server even though it has an explicit command', () => {
+    expect(chooseStdioMode({ ...base, secretDelivery: 'injector', command: ['node', 'x.js'] }))
+      .toEqual({ kind: 'attach' });
+  });
+
+  it('still execs a package server on the default env delivery', () => {
+    const m = chooseStdioMode({ ...base, secretDelivery: 'env', packageName: '@leval/mcp-grafana', runtime: 'node' });
+    expect(m.kind).toBe('exec');
+  });
+
+  it('still prefers an explicit command over packageName on env delivery', () => {
+    expect(chooseStdioMode({ ...base, secretDelivery: 'env', command: ['node', 'x.js'], packageName: 'p' }))
+      .toEqual({ kind: 'exec', command: ['node', 'x.js'] });
+  });
+
+  it('attaches for an image-entrypoint server, as before', () => {
+    expect(chooseStdioMode({ ...base, dockerImage: 'gitea/mcp:latest' })).toEqual({ kind: 'attach' });
+  });
+
+  it('rejects a server with no way to start', () => {
+    expect(() => chooseStdioMode({ ...base })).toThrow(/packageName, command, or dockerImage/);
+  });
+});