fix(control-plane-sdk): make the MCP path enforce required idempotency identically to the SDK (SPL-266)

[isuttell]

Fixes SPL-266.

The rule

idempotency: "required" means one thing on every path: a mutation that cannot name its own replay is refused before it leaves, by the caller's own client, in the caller's own vocabulary.

Before this PR the SDK threw locally on a missing key while the MCP path silently shipped a request the Worker could only reject — and for the two body-less DELETEs (flags_delete, flag_variants_delete) the derived tool schema had no idempotency_key field at all, so an agent was advertised a call it could not satisfy. That is the impossible remedy ADR-0036 forbids.

What changed

• Derivation (packages/contracts/src/mcp-tools.ts): a required route whose contract gives the key no request body gets idempotency_key injected into its flat tool schema. Uniform rule: every required route advertises the field, from the body when there is one and by injection when there is not.
• One helper, one behaviour (packages/control-plane-sdk/src/idempotency-header.ts): the MCP adapter now lifts the key through the same withIdempotencyHeader the typed clients use, instead of its own inline check. No parallel helper, no third behaviour.
• Typed refusal: withIdempotencyHeader throws IdempotencyKeyRequiredError, carrying an ErrorResponse with the Worker's VALIDATION_ERROR code and details.issues[] envelope. A blank or whitespace-only key is refused the same way — it names no replay, and forwarding it would move the refusal across the boundary.
• MCP handler (apps/mcp-server/src/mcp-handler.ts): that one class maps to a tool result with isError: true. Previously it surfaced as JSON-RPC -32603 "Internal error" with the remedy buried in error.data.message as prose. The generic Internal error branch is untouched and still covers unexpected throws. The catch body is a named toolCallFailure helper (the inline branch pushed callTool to cognitive complexity 11).
• Type-level requirement (operation-result.ts, flags-client.ts): the two DELETEs take ControlPlaneIdempotentOperationOptions, so a typed caller omitting the key is a compile error. The runtime ?. backstop stays for untyped/JS callers, with the reason documented on the shared type rather than per call site.
• Packaging: IdempotencyKeyRequiredError is exported from a real ./idempotency-header subpath. Re-exporting it through mcp-operation-adapter.ts tripped noBarrelFile, so the subpath was forced, not preferred. apps/mcp-server/vitest.config.unit.ts gained the matching alias plus a note on the prefix-match ordering hazard.
• Docs (docs/spec/contracts/mcp-tool-derivation.md): corrected — the key is no longer always "derived from the route body schema", the two DELETEs are named, and the refusal shape is specified.

On "same shape"

The client-side refusal carries the Worker's code and its details.issues[] envelope, not its issue path. worker-runtime reads the Idempotency-Key header and reports ["headers","idempotency-key"]; an MCP caller sends JSON and cannot set a header, so the refusal reports ["idempotency_key"] — the field the agent actually controls. The spec paragraph states that split and the reason, so the next client-side check copies it deliberately.

Proved by execution

• Derivation gap, RED: flags_delete / flag_variants_delete failed "advertises idempotency_key as a required field"; all 10 required routes failed "fails loud when the key is missing" with promise resolved "Request { method: 'POST', … }" instead of rejecting.
• Type-level gap, RED: 5 × error TS2578: Unused '@ts-expect-error' directive.
• Guard defeated deliberately: injected idempotency: "required" onto flags_list → these routes declare idempotency: "required" but no MCP probe proves the adapter sends the Idempotency-Key header: expected [ 'flags_list' ] to deeply equal []. Reverted.
• -32603 regression, RED before the fix: expected { code: -32603, …(2) } to be undefined, received { code: -32603, data: { message: "control-plane-sdk: flags_delete requires an idempotency key" }, message: "Internal error" }. GREEN after. A second test pins that a genuinely unexpected throw still yields -32603 with its own data.message; it passed in the RED run too, so it constrains the fix rather than moving with it.
• Blank key, RED: expected function to throw an error, but it didn't, and expected { headers: { "idempotency-key": " " } } to deeply equal {}. GREEN after.
• Fixtures that encoded the old behaviour: three existing tests called required routes with no key and broke once the adapter went loud. Each was given a real key; none was weakened.
• pnpm verify:ci (plain, no TURBO_FORCE), exit 0: Tasks: 85 successful, 85 total / Cached: 61 cached, 85 total. Every package this PR touches executed rather than replayed. contracts came back cached in the final run: it is unchanged since the first commit, and its four task hashes are identical to the run where each was cache miss, executing.

Reasoned about, not executed

• No e2e / Playwright (no local-e2e-fleet machine lock held).
• Nothing was run against any deployed environment; every proof above is local.
• McpOperationCallOptions.idempotencyKey is dead code, pre-existing and left alone.
• Scope-resolution failures on the MCP path still return an untyped message with no code. Same defect class, out of this PR's scope; the handler comment is scoped to the idempotency rule so it does not promise otherwise.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. To trigger a review, include @coderabbitai review in the PR description. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1128f1-e29b-4f24-8244-762a32d43504

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[isuttell]

Round 2 — review feedback addressed in 0a98e79.

Blocker (fixed): a missing idempotency_key reached the agent as JSON-RPC -32603 "Internal error". Reproduced first, verbatim, before touching the handler:

FAIL  |unit| src/mcp-idempotency-refusal.test.ts > mcp missing-idempotency-key refusal > returns a typed VALIDATION_ERROR tool result and issues no upstream request
AssertionError: expected { code: -32603, …(2) } to be undefined

- Expected:
undefined

+ Received:
{
  "code": -32603,
  "data": { "message": "control-plane-sdk: flags_delete requires an idempotency key" },
  "message": "Internal error",
}

Fix: withIdempotencyHeader now throws a typed IdempotencyKeyRequiredError carrying the same ErrorResponse shape the Worker returns for this rule (VALIDATION_ERROR, details.issues[].path = ["idempotency_key"]). mcp-handler maps that one class to a tool result with isError: true; no prose sniffing. The generic Internal error branch is unchanged and still covers unexpected throws — asserted by a second test that passed in the RED run too. GREEN: Test Files 1 passed (1) / Tests 2 passed (2).

Should-fix 1: the ?. backstop rationale moved onto ControlPlaneIdempotentOperationOptions in operation-result.ts, so it covers both call sites and cannot be missed.

Should-fix 2: captureRequest/RequestCaptured extracted to packages/control-plane-sdk/src/idempotency-probe-capture.ts; both contract tests kept.

Should-fix 3: docs/spec/contracts/mcp-tool-derivation.md now names the two DELETEs, documents the injection rule, and states the refusal shape (never -32603).

Two incidental fixes the gate forced, both mechanical: callTool hit cognitive complexity 11, so the catch body is now a named toolCallFailure helper; and re-exporting the error class from mcp-operation-adapter.ts tripped noBarrelFile, so ./idempotency-header is a real export subpath (plus the matching vitest alias, following the existing per-subpath convention in vitest.config.unit.ts).

pnpm verify:ci (plain, no TURBO_FORCE), exit 0:

 Tasks:    85 successful, 85 total
Cached:    61 cached, 85 total
  Time:    34.542s

Every package touched by this PR executed rather than replayed: control-plane-sdk and mcp-server were cache miss, executing for lint/build/test/typecheck in this run. contracts came back cached — flagging it rather than glossing it: it is unchanged since round 1 and its four task hashes here (f4bb55e0396ecf01, b314c61a58ac4bcc, 6ddc2446f1065c69, 30662b3b054aec37) are byte-identical to the round-1 run where each was cache miss, executing.

No e2e (no local-e2e-fleet lock held). credential-cache-backfill.test.ts did not fail in any run.