test(mcp): the check MCP never had, and the bug it found

packages/cli/src/commands/replay.ts

@@ -682,7 +682,14 @@ async function replayRestored(
     const mcp =
       trace === undefined
         ? undefined
-        : await mcpForReplay(args, ctx.events, trace, out, join(ctx.runDir, 'mcp-frames.jsonl'));
+        : await mcpForReplay(
+            args,
+            ctx.events,
+            trace,
+            out,
+            join(ctx.runDir, 'mcp-frames.jsonl'),
+            ctx.runDir,
+          );
 
     const adapter = defaultAdapters().get(ctx.manifest.adapter.id);
     const launch = await adapter.prepare({
@@ -1009,7 +1016,9 @@ async function replayFork(
 
   // A fork continues the run live past the checkpoint, so its MCP traffic is new and belongs in the
   // fork's own trace. Without this the layer simply stopped at the fork point.
-  const mcp = await mcpForReplay(args, ctx.events, writer, out).catch(abandon);
+  const mcp = await mcpForReplay(args, ctx.events, writer, out, undefined, ctx.runDir).catch(
+    abandon,
+  );
 
   const proxy = await createProxy({
     mode: 'hybrid',

packages/cli/src/mcp.ts

@@ -29,6 +29,28 @@ export interface McpCapture {
  */
 export type { McpFrameRecord };
 
+/**
+ * The recording's own copy of the config it was given, kept beside the frames it produced.
+ *
+ * A replay used to find the source by reading the path out of the `mcp_instrumented` note, and a
+ * path is a poor thing to depend on. It has to survive the trace's redactor — `mkdtemp` and every
+ * CI workspace end in a random segment, and `orca-int-mcp-stdio-feH8gJ` is 25 characters of
+ * mixed-case base62, so roughly one in seven cleared the entropy sweep's threshold and reached the
+ * trace as `<secret:high_entropy:…>`. `stat` then failed, MCP was dropped for the replay, and the
+ * agent was launched with no `MCP_CONFIG_PATH`: `KeyError` from a recording that was perfectly
+ * good, intermittently, according to what `mkdtemp` picked.
+ *
+ * It also has to still be there, and mean the same thing. Deleting the config, moving it, or
+ * editing a server's command between the recording and the replay all changed what the replay ran
+ * — for a file the operator has no reason to think of as part of the run.
+ *
+ * Keeping the bytes removes both. Nothing has to survive redaction, because this is not in the
+ * trace; nothing outside the run directory has to still exist. It carries the same material as the
+ * rewritten `mcp-config.json` written next to it — same directory, same `0600` — so it is not a
+ * new kind of thing to protect.
+ */
+export const MCP_SOURCE_FILENAME = 'mcp-source.json';
+
 /**
  * The MCP config a *replay* should instrument: the flag if one was given, else whatever the
  * recording itself used.
@@ -43,6 +65,7 @@ export type { McpFrameRecord };
  * the *rewritten* config in the parent run directory: its servers already point at the parent's
  * frames file, so reusing it would append this replay's traffic to the recording it is replaying.
  */
+
 export function mcpSourceFrom(
   flagValue: string | undefined,
   events: { type: string; attrs?: Record<string, unknown> }[],
@@ -76,7 +99,21 @@ export async function mcpForReplay(
   out: Output,
   /** The recording's own frames, so the servers are answered from rather than started. */
   recordedFrames?: string,
+  /** The recording's directory, which holds its own copy of the config it was given. */
+  recordedRunDir?: string,
 ): Promise<McpCapture | undefined> {
+  // The recording's copy first, and only then the path it wrote down. The copy cannot have been
+  // redacted, moved or edited since; the path can have been all three.
+  const kept = recordedRunDir === undefined ? undefined : join(recordedRunDir, MCP_SOURCE_FILENAME);
+  if (kept !== undefined && (await stat(kept).catch(() => null))) {
+    return setupMcpCapture({
+      sourceConfigPath: kept,
+      runDir: writer.runDir,
+      out,
+      ...(recordedFrames === undefined ? {} : { replayFrames: recordedFrames }),
+    });
+  }
+
   const source = mcpSourceFrom(args.str('mcp-config'), events);
   if (source === undefined) {
     if (usedMcp(events)) {
@@ -205,6 +242,11 @@ export async function setupMcpCapture(opts: {
     opts.out.warn('mcp.config_unreadable', { path: opts.sourceConfigPath });
     return undefined;
   }
+  // Kept before it is parsed, so a later run reads back what this run was handed — byte for byte,
+  // key order and all. See {@link MCP_SOURCE_FILENAME} for why a path was not good enough.
+  await writeFile(join(opts.runDir, MCP_SOURCE_FILENAME), raw, { mode: 0o600 }).catch(
+    () => undefined,
+  );
 
   let parsed: unknown;
   try {

packages/cli/test/mcp-capture.test.ts

@@ -1,5 +1,5 @@
 import { execFile } from 'node:child_process';
-import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises';
+import { mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import { dirname, join } from 'node:path';
 import { fileURLToPath } from 'node:url';
@@ -9,7 +9,13 @@ import { TraceReader, deriveCheckpoints, resolveRunSelector } from '@orcareplay/
 import { validateEvent } from '@orcareplay/schema';
 import { parseArgs } from '../src/args.js';
 import { Output } from '../src/out.js';
-import { mcpSourceFrom, setupMcpCapture, usedMcp } from '../src/mcp.js';
+import {
+  MCP_SOURCE_FILENAME,
+  mcpForReplay,
+  mcpSourceFrom,
+  setupMcpCapture,
+  usedMcp,
+} from '../src/mcp.js';
 import { recordCommand } from '../src/commands/record.js';
 import { replayCommand } from '../src/commands/replay.js';
 import { startFakeModel } from './fixtures/fake-model.mjs';
@@ -204,7 +210,13 @@ describe('mcp capture', () => {
       for (const event of mcp) expect(validateEvent(event).valid).toBe(true);
     });
 
-    it('says so rather than silently dropping the layer when the config has moved', async () => {
+    it('replays a moved config, because the recording kept the bytes', async () => {
+      // This used to warn `mcp.source_missing` and drop the layer, and that was the best it could
+      // do: the replay found the config by reading a path out of the trace, so a config the
+      // operator moved was a config the replay could not open. The recording now keeps its own
+      // copy, which also removes the reason the path had to survive redaction — see
+      // `MCP_SOURCE_FILENAME`.
+      //
       // The config lives *outside* the workspace here, because exact replay restores the recorded
       // tree over the working directory — deleting a file that was in the snapshot just brings it
       // back, which is the restore working correctly and would make this test pass for the wrong
@@ -220,6 +232,25 @@ describe('mcp capture', () => {
 
       await replayCommand(parseArgs(['replay', 'last']), out, workspace);
 
+      const printed = lines.join('');
+      expect(printed).toContain('mcp.instrumented');
+      expect(printed).not.toContain('mcp.source_missing');
+    });
+
+    it('still says so for a recording made before the copy was kept', async () => {
+      // The warning is not gone, and must not be: a trace recorded by an older orca has only the
+      // path, and a dropped capture layer must never read as a quiet success. Same run as above
+      // with the kept copy removed, which is exactly what those traces look like.
+      const elsewhere = await mkdtemp(join(tmpdir(), 'orca-mcp-cfg-'));
+      const configPath = join(elsewhere, 'mcp.json');
+      await writeFile(configPath, await readFile(join(workspace, 'mcp.json'), 'utf8'));
+      const { runDir } = await record(configPath);
+      await rm(elsewhere, { recursive: true, force: true });
+      await rm(join(runDir, MCP_SOURCE_FILENAME));
+      lines.length = 0;
+
+      await replayCommand(parseArgs(['replay', 'last']), out, workspace);
+
       const printed = lines.join('');
       expect(printed).toContain('mcp.source_missing');
       expect(printed, 'a dropped capture layer must not read as a quiet success').not.toContain(
@@ -280,6 +311,98 @@ describe('mcpSourceFrom', () => {
   });
 });
 
+/**
+ * The recording keeps the config it was given, so a replay does not depend on a path.
+ *
+ * Found by an integration check that failed about one run in seven. The path was read back out of
+ * the `mcp_instrumented` note, and a path in the trace has to survive the redactor:
+ * `orca-int-mcp-stdio-feH8gJ` is 25 characters of mixed-case base62, over `MIN_ENTROPY_LENGTH`, so
+ * often enough the entropy sweep replaced the directory with `<secret:high_entropy:…>`. `stat`
+ * then failed, MCP was dropped for the replay, and the agent was launched with no
+ * `MCP_CONFIG_PATH` — `KeyError` from a recording that was perfectly good.
+ *
+ * Widening the redactor was the wrong fix and was tried first: an exemption keyed on `cwd`/`source`
+ * containing a separator also shelters a URL with a token in its query, a database DSN, and a
+ * `data:image/png;base64,…` forgery — which bypasses the raster validation that
+ * `data-uri-redaction.test.ts` exists to enforce. Measured before it was reverted: a forged image
+ * under `source` survived with **zero** redaction records, so the miss was invisible in
+ * `redactions.json` too. The file's own comment says an exemption granted on the label was a hole
+ * every time it was tried; this was that hole again.
+ */
+describe('the config the recording kept', () => {
+  /** An Output that says nothing, because these assert on files rather than on what was printed. */
+  const quiet = () => new Output({ write: () => {}, isTTY: false });
+
+  let runDir: string;
+  let source: string;
+
+  beforeEach(async () => {
+    runDir = await mkdtemp(join(tmpdir(), 'orca-mcp-src-'));
+    source = join(runDir, 'given.json');
+    await writeFile(source, CONFIG_TEXT);
+  });
+
+  afterEach(async () => {
+    await rm(runDir, { recursive: true, force: true });
+  });
+
+  const CONFIG_TEXT = `{\n  "mcpServers": {\n    "probe": { "command": "python", "args": ["s.py"] }\n  }\n}\n`;
+
+  it('keeps the bytes it was handed, not a re-serialisation of them', async () => {
+    // Byte for byte, so key order and whitespace a later reader might depend on survive.
+    await setupMcpCapture({ sourceConfigPath: source, runDir, out: quiet() });
+    expect(await readFile(join(runDir, MCP_SOURCE_FILENAME), 'utf8')).toBe(CONFIG_TEXT);
+  });
+
+  it('keeps it beside the rewritten one, at the same mode', async () => {
+    await setupMcpCapture({ sourceConfigPath: source, runDir, out: quiet() });
+    const mode = (await stat(join(runDir, MCP_SOURCE_FILENAME))).mode & 0o777;
+    const rewritten = (await stat(join(runDir, 'mcp-config.json'))).mode & 0o777;
+    expect(mode).toBe(rewritten);
+  });
+
+  it('is what a replay reads, even after the original is gone', async () => {
+    await setupMcpCapture({ sourceConfigPath: source, runDir, out: quiet() });
+    // The case the note could never survive: the operator moved or deleted their config, or the
+    // path in the trace was redacted into something `stat` cannot find.
+    await rm(source);
+
+    const replayDir = await mkdtemp(join(tmpdir(), 'orca-mcp-replay-'));
+    try {
+      const capture = await mcpForReplay(
+        { str: () => undefined },
+        [{ type: 'note', attrs: { rule: 'mcp_instrumented', source: '/gone/<secret:x>/c.json' } }],
+        { runDir: replayDir } as never,
+        quiet(),
+        undefined,
+        runDir,
+      );
+      expect(capture, 'the replay found nothing to instrument').toBeTruthy();
+      expect(capture?.rewritten).toEqual(['probe']);
+    } finally {
+      await rm(replayDir, { recursive: true, force: true });
+    }
+  });
+
+  it('still falls back to the note, for a run recorded before this', async () => {
+    const replayDir = await mkdtemp(join(tmpdir(), 'orca-mcp-replay-'));
+    try {
+      const capture = await mcpForReplay(
+        { str: () => undefined },
+        [{ type: 'note', attrs: { rule: 'mcp_instrumented', source } }],
+        { runDir: replayDir } as never,
+        quiet(),
+        undefined,
+        // A directory with no kept copy in it, which is every run recorded before this change.
+        replayDir,
+      );
+      expect(capture?.rewritten).toEqual(['probe']);
+    } finally {
+      await rm(replayDir, { recursive: true, force: true });
+    }
+  });
+});
+
 describe('a capture line that parsed is not yet a frame', () => {
   /**
    * The same rule the shell frames reader has, one file over, and for the same reason: several

test/integrations/agents/mcp_agent.py

@@ -0,0 +1,44 @@
+"""An agent that talks to a model *and* to an MCP server it launches from a config file.
+
+Both halves on purpose. The model call is what every other check asserts and what makes the
+exchange counts comparable; the MCP session is the half no other check covers at all.
+
+It reads the config rather than building `StdioServerParameters` in code, because that is the shape
+orca can instrument — it rewrites the config to put its shim between client and server. A client
+that constructs the parameters itself leaves orca nothing to rewrite, and the session is then
+invisible to every capture layer: the transport is OS pipes, so the proxy never sees it, and
+`mcp.client.stdio` spawns in list form rather than through a shell, so the PATH shim never fires.
+That gap is real and is not what this check covers.
+"""
+
+import asyncio
+import json
+import os
+
+from mcp import ClientSession, StdioServerParameters, stdio_client
+from openai import OpenAI
+
+
+async def main() -> None:
+    with open(os.environ["MCP_CONFIG_PATH"], encoding="utf8") as fh:
+        config = json.load(fh)
+    name, entry = next(iter(config["mcpServers"].items()))
+    params = StdioServerParameters(
+        command=entry["command"], args=entry.get("args", []), env=entry.get("env")
+    )
+
+    async with stdio_client(params) as (read, write):
+        async with ClientSession(read, write) as session:
+            await session.initialize()
+            tools = await session.list_tools()
+            print("TOOLS:", [t.name for t in tools.tools])
+            result = await session.call_tool("lookup", {"key": "alpha"})
+            print("MCP:", result.content[0].text)
+
+    reply = OpenAI().chat.completions.create(
+        model="stub-1", messages=[{"role": "user", "content": "hello"}]
+    )
+    print("GOT:", reply.choices[0].message.content)
+
+
+asyncio.run(main())

test/integrations/agents/mcp_server.py

@@ -0,0 +1,20 @@
+"""An MCP server over stdio, for the check that records one and then takes it away.
+
+Deliberately trivial and deliberately deterministic: the point of the check is the transport and
+the replay, not the tool. `lookup` answers from its argument so a replay that served the wrong
+recorded frame is visible in the output rather than only in a count.
+"""
+
+from mcp.server.fastmcp import FastMCP
+
+app = FastMCP("orca-check")
+
+
+@app.tool()
+def lookup(key: str) -> str:
+    """Return a canned value for a key."""
+    return f"VALUE-FOR-{key}"
+
+
+if __name__ == "__main__":
+    app.run()