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

[xizhuomengcontin]

Orca-Code-Review — push 1

Severity	Count
P0	0
P1	0
P2	0
P3	0

✅ no blocking findings

MCP is the thickest-built capture layer in the repository — mcp-shim plus cli/src/mcp.ts is ~1900 lines with its unit tests — and it was the only one with no integration check at all. Nothing asserted end to end that a recorded MCP session comes back, and replay.done's counters cannot say so either: they count model exchanges, so a broken MCP replay leaves that line reading exactly as it does on success.

The check records an agent that talks to a model and to an MCP server it launches from a config, then takes the server away as well as the origin — the only way to tell a replay that served the recorded frames from one that quietly started the server again — and asserts the agent still printed the recorded tool result, plus mcp.request / mcp.response in the trace.

It failed about one run in seven.

What it found

A replay finds the config by reading a path out of the mcp_instrumented note, and a path in the trace has to survive the redactor. mkdtemp and every CI workspace end in a deliberately random segment: orca-int-mcp-stdio-feH8gJ is 25 characters — over MIN_ENTROPY_LENGTH — mixed-case with digits, so when the suffix happened to clear entropy > 4.0 the whole directory was replaced:

{"rule":"mcp_instrumented","source":"C:\…\Temp\<secret:high_entropy:caf9cb4e>\mcp.json"}

stat then fails, MCP is dropped for the replay, and the agent is launched with no MCP_CONFIG_PATH — KeyError from a recording that was perfectly good, intermittently, according to what mkdtemp picked.

The first fix was wrong, and the review caught it

The first version widened spansOf to shelter any cwd/source value containing a path separator. Measured against it before reverting, with new Redactor({salt:'x'}):

value	result
{"source":"https://api.contoso.com/v2?key=<token>"}	unchanged, 0 records
{"url":"https://api.contoso.com/v2?key=<token>"}	redacted, 1 record
{"cwd":"postgresql://orca:<password>@db.internal/prod"}	unchanged, 0 records
{"source":"data:image/png;base64,<real PNG><token>"}	unchanged, 0 records
{"url":"data:image/png;base64,<real PNG><token>"}	redacted, 12 records

The last pair is the worst of it: a source-keyed value skipped the raster validation that data-uri-redaction.test.ts exists to enforce. And zero records means redactions.json does not mention the miss, so it is invisible — and orca scrub runs the same redactString, so a second pass does not catch it either.

The comment above rasterSpans lists four earlier attempts to grant that exemption on the label or the syntax around it, and calls each one a hole. This was the fifth.

Narrowing the pattern to absolute paths — the review's other suggestion — closes those four cases and still shelters /var/run/secrets/<token>: a Kubernetes secret mount is an absolute path with no punctuation to disqualify it. The exemption would still be keyed on the label.

What it does instead

The redactor is untouched. Policy stays at 4. The dependency it was weakened for is gone instead.

setupMcpCapture writes the bytes it was handed to <runDir>/mcp-source.json — beside the rewritten config, same directory, same 0600, same material — and mcpForReplay reads that before it considers the path in the note. Nothing has to survive redaction, because it is not in the trace.

It also fixes the case the note could never cover: 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. says so rather than silently dropping the layer when the config has moved asserted that warning; it is now two tests — a moved config replays, and a run whose kept copy is absent still warns, because every trace recorded before this looks like that and a dropped capture layer must never read as a quiet success.

Why it took a while to find

The runner said replay exited -1 and nothing else. Three reasons, all fixed here:

• e.status is spawnSync's field. execFile rejects with code, so every failure in this suite has always reported -1, whatever the command did.
• e.message was dropped, and for execFile it carries the child's stderr — the only evidence there is when a spawn fails, times out, or overruns maxBuffer, since all three reject with empty stdout/stderr.
• The last lines of it, not the first. A Python traceback puts the exception at the bottom, so taking the top kept asyncio.run(main()) and dropped KeyError: 'MCP_CONFIG_PATH'.

The same suite now reports AttributeError: module 'litellm' has no attribute 'completion' where it used to report -1.

A note on the hunt, because it shaped the fix: a standalone harness reproducing the same record→replay sequence passed 60/60 and never saw it. Its temp directories were named mcpflake-XXXXXX — 15 characters, under MIN_ENTROPY_LENGTH, so the sweep never looked at them. The bug needs a long enough prefix, which is why it lives in the runner's orca-int-<check-id>- names and nowhere else I tried.

Verified

mcp-stdio after the fix	14/14, having been 1-in-7 before
integration suite	18 checks (was 17)
mutation test	removing the kept-copy branch turns 2 of the 4 new MCP tests red
unit suite, Windows, clean run	10 failed / 2412 passed — a strict subset of the known Windows set, none in the files this touches

One run of the same tree reported 24 failures, every extra one a 30s testTimeout on a machine that was also running the integration suite. None reproduced on a clean run; recorded here rather than left out.

🤖 Generated with Claude Code

[orcacode-review]

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 604 calls · 50.6M tokens · 99% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

[orcacode-review]

🟠 P1 Narrow the cwd/source shield to real paths: "contains a separator" exempts URLs, DSNs and data URIs (including forged images) from the sweep

RECORDED_PATH_VALUE grants the exemption on the label plus one character — any value under a key named cwd/source that contains one / or \ is shielded wholesale from the entropy sweep, and the shielded span is the whole value, so anything parked in it survives. Measured against the module at this commit (new Redactor({salt:'x'}).redactString(...)):

• {"source":"https://api.contoso.com/v2?key=gT7hQ2vX9mK4pL8nR3wZ6yB1"} -> returned unchanged, hits: []; the identical value under url -> <secret:high_entropy:07e50e9b>, 1 record.
• {"cwd":"postgresql://orca:gT7hQ2vX9mK4pL8nR3wZ6yB1@db.internal:5432/prod"} -> unchanged, no record (a database password, which no shape rule covers).
• Nested, which is how a body is actually stored: {"attrs":{"body":"{\"source\":\"https://api.example.com/v1/jobs/<token>/output\"}"}} -> token survives; the same body with \"url\" is swept.
• {"source":"data:image/png;base64,<1x1 PNG><token>"} -> returned unchanged, hits: [], while the same forged value under url is swept by the raster forgery check. So for a source-keyed value the shield also bypasses the PNG validation that rasterSpans and data-uri-redaction.test.ts exist to enforce — the file's own comment there says granting an exemption on "the syntax around it or the label in front of it" was a hole every time it was tried. That is exactly what this rule does.

Consequence: a credential with no known shape — the case §5 of the spec and SECURITY.md say the sweep is for — is written verbatim into events.jsonl/blobs, and because hits is empty it is not recorded in redactions.json either, so the miss is invisible. orca scrub uses the same redactString, so the second pass does not catch it before a trace is attached to an issue. The comment's claim that "the value has to look like a path — it must contain a separator" is false: a URL, a DSN, a command line and a base64 data URI all contain one.

Fix (keeps the case the rule was added for): require the value to actually be an absolute filesystem path, and reject the punctuation that makes it something else. cwd from run.start and the mcp_instrumented note's source are both absolute (resolve(cwd, mcpConfigPath)), and both new pattern tests still pass:

const RECORDED_PATH_VALUE =
/(\")(?:cwd|source)\1\s:\s*\1(?:/|[A-Za-z]:[\/])(?:[^"\\?=@\s]|\.)*?\1/g;

(https://…, postgresql://…, data:image/png;base64,… and command lines no longer match, so the sweep and the raster validator see them again. If the relaxation is only needed for orca's own path, the alternative is to stop deriving it from the trace — write the config path into manifest.json, which is not swept, and read it back from there — and leave the sweep untouched.)

[xizhuomengcontin]

The P1 is right, and the fix it flagged is reverted in 511fb92. I reproduced all four cases before changing anything:

value	result
{"source":"https://api.contoso.com/v2?key=<token>"}	unchanged, 0 records
{"url":"https://api.contoso.com/v2?key=<token>"}	redacted, 1 record
{"cwd":"postgresql://orca:<password>@db.internal/prod"}	unchanged, 0 records
{"source":"data:image/png;base64,<real PNG><token>"}	unchanged, 0 records
{"url":"data:image/png;base64,<real PNG><token>"}	redacted, 12 records

The forged-image row is the one that decided it: a source-keyed value skipped the raster validation. And zero records is the sharper half — redactions.json does not mention the miss, and orca scrub runs the same redactString, so nothing downstream catches it. The comment above rasterSpans lists four earlier attempts to grant that exemption on the label or the surrounding syntax and calls each one a hole; mine was the fifth, and the docstring I wrote claiming "the value has to look like a path" was simply false.

I did not take the narrowed pattern, though it does close those four. It still shelters /var/run/secrets/<token> — a Kubernetes secret mount is an absolute path with no ?, =, @ or whitespace in it — and the exemption would still be keyed on the label, which is the property that keeps turning out to be the problem.

I took the alternative in your parenthesis instead, with one change: rather than the manifest, the bytes go in the run directory. setupMcpCapture writes what it was handed to <runDir>/mcp-source.json — beside the rewritten config, same 0600, same material, so no new kind of thing to protect — and mcpForReplay reads that before the note's path. The redactor is untouched and the policy version goes back to 4.

That also covers a case the note never could: deleting or editing the config between recording and replay silently changed what the replay ran. The mcp.source_missing warning is still there and still tested, for traces recorded before this that have only the path.

Verified: mcp-stdio 14/14 with redaction.ts back at main's version, and removing the kept-copy branch turns 2 of the 4 new tests red.

[orcacode-review]

🐳 OrcaCode Review

✅ No findings — nothing to flag in this PR. Great work!

_{OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 380 calls · 31.2M tokens · 98% cached}

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter