Merge pull request 'fix(secrets): wrap an image server's own command, not just entrypoint' (#121) from fix/injector-image-server-argv into main

Migrating docmost and my-home-assistant, both rendered their secret and
neither picked it up. The pod spec showed why:

  command: (empty)
  args:    ["node","build/index.js"]
  server.entrypoint: (unset)

wrapCommand consulted only `server.entrypoint` for non-package servers, so
with `entrypoint` unset it returned undefined, the wrapper was skipped
entirely, and the container ran its normal command — which never sourced
/vault/secrets/<name>. The failure is silent by construction: the agent
init container succeeds, the file is there, and the server simply starts
with empty credentials.

An explicit `command` on an image server is already a complete command
line — mcpd's exec mode would run exactly it — so it should be wrapped
verbatim. `entrypoint` is only needed when there is no command at all and
the image's own ENTRYPOINT would take over.

Now branches on the three real shapes: package server (prepend the runner
entrypoint mcpd owns), image + command (use verbatim), image only (require
the declared entrypoint).

Seven tests, one per shape plus the two undefined cases. Reintroducing the
old logic fails two of them — checked before keeping.

Both servers were rolled back to secretDelivery: env and are healthy; they
can migrate once this ships.

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

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,51 @@ 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;
+
+    // Build the COMPLETE argv the container should run. It differs by shape:
+    //
+    //   package server  — mcpd owns the runner image, whose ENTRYPOINT
+    //                     (`npx -y` / `uvx`) is lost once we take over
+    //                     `command`, so it must be prepended here.
+    //   image + command — `command` is already a full command line; mcpd would
+    //                     have run exactly it. Prepending anything breaks it.
+    //   image only      — the image's own ENTRYPOINT would run and mcpd cannot
+    //                     introspect it, so the row must declare `entrypoint`.
+    //
+    // Getting this wrong returns undefined and SILENTLY skips the wrapper: the
+    // agent still renders the file, nothing sources it, and the server starts
+    // with empty credentials. Observed on docmost and my-home-assistant, which
+    // carry a `command` but no `entrypoint`.
+    const hasPackage = server.packageName !== null && server.packageName !== undefined && server.packageName !== '';
+    const argv = hasPackage
+      ? [...(server.runtime === 'python' ? ['uvx'] : ['npx', '-y']), ...(command ?? [server.packageName as string])]
+      : command !== undefined && command.length > 0
+        ? command
+        : ((server.entrypoint as string[] | null) ?? undefined);
+
+    if (argv === undefined || argv.length === 0) return undefined;
+    return wrapCommandForInjector(argv, 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-argv.test.ts

@@ -0,0 +1,58 @@
+/**
+ * Which argv the injector wrapper wraps, by server shape.
+ *
+ * Getting this wrong is SILENT: wrapCommand returns undefined, the wrapper is
+ * skipped, the agent still renders /vault/secrets/<name>, nothing sources it,
+ * and the server starts with empty credentials. Observed live on docmost and
+ * my-home-assistant, which carry a `command` but no `entrypoint`.
+ */
+import { describe, it, expect } from 'vitest';
+import { ServerIdentityService } from '../src/services/server-identity.service.js';
+import type { SecretBackendService } from '../src/services/secret-backend.service.js';
+
+const svc = new ServerIdentityService(
+  {} as unknown as SecretBackendService,
+  { namespace: 'mcpctl-servers', ensure: async () => undefined, remove: async () => undefined },
+);
+
+const withSecret = { env: [{ name: 'T', valueFrom: { secretRef: { name: 'creds', key: 'K' } } }] };
+/** The wrapper is `sh -c <script> arg0 arg1...`; argv starts at index 3. */
+const argvOf = (r: string[] | undefined): string[] | undefined => r?.slice(3);
+
+describe('wrapCommand argv by server shape', () => {
+  it('prepends the node runner entrypoint for a package server', () => {
+    const r = svc.wrapCommand({ ...withSecret, packageName: '@leval/mcp-grafana', runtime: 'node', entrypoint: null } as never, ['@leval/mcp-grafana']);
+    expect(argvOf(r)).toEqual(['npx', '-y', '@leval/mcp-grafana']);
+  });
+
+  it('prepends uvx for a python package server', () => {
+    const r = svc.wrapCommand({ ...withSecret, packageName: 'mcp-searxng', runtime: 'python', entrypoint: null } as never, ['mcp-searxng']);
+    expect(argvOf(r)).toEqual(['uvx', 'mcp-searxng']);
+  });
+
+  it('uses an image server\'s command verbatim — prepending anything breaks it', () => {
+    // The docmost/home-assistant regression: this used to return undefined.
+    const r = svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: null } as never, ['node', 'build/index.js']);
+    expect(argvOf(r)).toEqual(['node', 'build/index.js']);
+  });
+
+  it('falls back to the declared entrypoint for an image server with no command', () => {
+    const r = svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: ['/usr/local/bin/gitea-mcp'] } as never, undefined);
+    expect(argvOf(r)).toEqual(['/usr/local/bin/gitea-mcp']);
+  });
+
+  it('returns undefined when there is genuinely nothing to run', () => {
+    expect(svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: null } as never, undefined)).toBeUndefined();
+  });
+
+  it('returns undefined for a server with no secret refs', () => {
+    expect(svc.wrapCommand({ env: [], packageName: 'p', runtime: 'node', entrypoint: null } as never, ['p'])).toBeUndefined();
+  });
+
+  it('always sources before exec, whatever the shape', () => {
+    const r = svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: null } as never, ['node', 'x.js']);
+    expect(r?.[0]).toBe('/bin/sh');
+    expect(r?.[2]).toContain('. /vault/secrets/creds');
+    expect(r?.[2]).toContain('exec "$0" "$@"');
+  });
+});

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