fix: #2004 enforce the simulate invariant across /api/execute/*

[dickwin2003]

fix: #2004 enforce the simulate invariant across /api/execute/*

Closes #2004 (members #1929, #1959, #1933).

simulate now behaves uniformly across /api/execute/*: a route either honours a dry run or refuses one, and no route accepts the flag and broadcasts. One guard module, two guards, every route wired — and a filesystem-enumerated invariant test so a seventh route cannot merge without declaring a side.

The five-route audit matrix

Route	Body simulate before	Body simulate after	Query ?simulate= before	Query ?simulate= after	Test coverage
/api/execute/transfer	honoured (dry-run)	unchanged — still honoured	silently ignored → broadcast	400 unsupported_param	execute-simulate-invariant.test.ts (3 assertions) + execute-simulate-route.test.ts
/api/execute/contract-call	honoured (dry-run)	unchanged — still honoured	silently ignored → broadcast (test pinned 202 + broadcast)	400 unsupported_param (test deliberately rewritten)	same
/api/execute/check-and-execute	honoured (dry-run)	unchanged — still honoured	silently ignored → broadcast	400 unsupported_param	same
/api/execute/{protocol}/{action} ([...slug])	accepted, ignored → real broadcast (#1929)	400, before the idempotency key is reserved — no execution row consumed	silently ignored → broadcast	400	execute-simulate-invariant.test.ts refusing-route block + resolution-precedence test
/api/execute/node	accepted, dropped by the validator whitelist → real broadcast (the 9/7 addition)	400, before the whitelist runs	silently ignored → broadcast	400	execute-simulate-invariant.test.ts refusing-route block

Two non-write routes are wired for item 2's "every /api/execute/* route" and pinned in the manifest: swap (501 stub, body never read — ?simulate= now 400, plain POST still 501) and [executionId]/status (GET-only — ?simulate= now 400).

The single decision point

Both guards live in app/api/execute/_lib/simulate-flag.ts next to parseSimulateFlag, and follow the same NextResponse | null guard convention as requireScope/requireWallet:

• rejectSimulateQuery(request) — any simulate key in the query string (true, false, empty, mistyped) is a 400 naming the three routes that honour a body dry run. Other query parameters are untouched.
• refuseSimulateBody(body) — any body simulate on a no-dry-run route is a 400. There is no correct flag shape on a refusing route, so every shape is refused, including "true".

The 400 body carries code: "unsupported_param", field: "simulate", and a message naming /api/execute/transfer, /api/execute/contract-call, and /api/execute/check-and-execute.

Placement, per route:

• Honouring routes: after auth, before body parse — the rest of the route is untouched, so their existing semantics (strict-boolean body flag, mcp:read scope downgrade on dry runs, simulation cores) are unchanged.
• [...slug]: query guard after auth; body guard after request.json(), before beginIdempotentFromRequest — a refused request consumes no idempotency key and no executionId, which fix(execute): protocol action route silently ignores simulate and broadcasts real transactions #1929's version burned silently.
• node: query guard after auth; body guard after request.json(), before validateRequest — the fixed whitelist (route.ts:155-164 on staging) used to drop the field without a trace.

Item 2 is an explicit call, made here

tests/integration/execute-simulate-route.test.ts:272 used to assert POST /api/execute/contract-call?simulate=true → 202 + writeContractCore called, under the comment "the query string must NOT be honoured — there is exactly one way to ask for a dry-run." That position is retired deliberately and in this PR, not in review: as the 8/19 comment put it, a family where transfer rejects the query flag and contract-call ignores it is the worst of the three uniform answers — and a caller that asked for a dry run and received a broadcast cannot tell the difference on the wire. The rewritten test asserts the 400 and that nothing was reserved or broadcast. PR #2090 (transfer-only query guard, since closed) was exactly the split this avoids.

The invariant test enumerates routes from the filesystem

tests/integration/execute-simulate-invariant.test.ts (per the 9/7 comment — "not from a list someone maintains by hand"):

1. Walks app/api/execute for every route.ts (_lib excluded) and requires each discovered route to appear exactly once in a stance manifest — and the manifest to contain no stale entries. A seventh route file cannot merge without declaring one of: honors-body, refuses, stub-501 (swap), read-only-get (status).
2. Generates the behavioural assertions from that manifest, so declaring a stance without wiring the guard fails the suite. Per stance:
   • honours: body simulate:true → simulator is the only side effect (checkAndReserveExecution, markRunning, completeExecution, failExecution, createExecution, setRetryCount, writeContractCore, transferFundsCore, transferTokenCore, and the node step fn all asserted not called); query ?simulate=true → 400 + zero side effects; body "true" → 400.
   • refuses: body simulate:true → 400 + zero side effects; body "true" → 400; query ?simulate=true → 400 + zero side effects — including a test that the [...slug] refusal fires even when the protocol action would resolve.
   • stub-501 / read-only-get: ?simulate= → 400; swap without the flag still 501.

What is intentionally not here

• Real dry-run support for protocol actions / node steps (item 1's other branch — the refusal branch is the one the issue blessed as "the smaller change that removes the hazard today"; a future dry-run flips one route's answer, not the contract).
• Documentation: execute_protocol_action: the documented preflight broadcasts, and the prescribed retry broadcasts again #2097's territory. One note for that issue: docs/api/direct-execution.md's Dry-Run Simulation section now has a fifth sentence to consider, since the invariant itself is enforced in code and test.
• Case-insensitive query matching: the guard matches the exact simulate key, same as parseSimulateFlag matches the exact body field — an execute endpoint has exactly one shape per input.

Verification (all run locally on Windows)

• pnpm type-check (tsgo, app) — 0 errors; pnpm type-check:executor — 0 errors
• pnpm fix (biome) — clean; remaining warning is the pre-existing file-size advisory present on an untouched tree
• pnpm vitest run on the touched surface — execute-simulate-invariant (21 new tests) + execute-simulate-route + execute-node-reserved-fields + execute-protocol-* (4 files) + execute-scope-enforcement + execute-route-integration-authz + api-not-found + oauth-scope-route-gate + direct-execution-api + execute-api — all green (247 tests)
• Full tests/unit suite — same failure set as the pre-existing Windows baseline established on the unmodified tree during feat: #2288 workflow-scoped key-value state (State Get + State Set) #2368's verification (docker-sandbox and path-separator environment failures; unrelated to these routes), with the same method: compare against the unchanged base worktree before attributing anything to this PR.

[github-actions]

About the build check on this pull request

This pull request comes from a fork, so GitHub does not pass it the credentials build normally uses for our image registry cache and staging build configuration. The build still runs and still compiles the image, so a red build here is real; it just takes longer than on team branches.

Every workflow run on a pull request from a fork also waits for a maintainer to approve it, so checks can sit at "awaiting approval" for a while after each push. Nothing is needed from you for either of these.

[suisuss]

Welcome, and thanks for taking this one on - CONTRIBUTING.md carries the repo conventions and ISSUES.md the issue-first rule, and both are already satisfied here.

What this changes

app/api/execute/_lib/simulate-flag.ts grows two guards beside the existing parseSimulateFlag: rejectSimulateQuery returns a 400 unsupported_param when simulate appears in the query string, and refuseSimulateBody returns the same when it appears as a top-level body key. Both are presence checks - the value is never inspected. The query guard is wired into all seven routes under app/api/execute/, the body guard into [...slug] and node. A new 647-line integration suite walks the route directory, compares it against a hand-written stance manifest in both directions, and runs per-stance behavioural checks; the existing "query-string simulate is ignored" test is replaced with a 400 assertion.

The severe half is real and I traced it: POST /api/execute/[...slug] with simulate in the body previously fell through buildProtocolFunctionArgs - which reads only registry-declared input names - into writeContractCore, i.e. a real signed broadcast. lib/mcp/tools.ts:2470-2478 forwards args.params verbatim as that body, so an agent that put simulate: true in params got a broadcast. That path now 400s. For callers not sending the key nothing changes: both guards return null on absence, and the query guard reads only request.url, so it cannot perturb the later request.json().

Does it match the description

Undersells in one direction and oversells in another. The diff also changes GET /api/execute/{executionId}/status and POST /api/execute/swap, neither of which could ever have broadcast anything. And the stated invariant is family-wide while the enforcement is top-level-key only - see the first two blockers.

Blocking

• app/api/execute/node/route.ts:559 with :363-374 - the refusal is top-level only, and config.simulate still broadcasts. stripReservedConfig removes exactly network, integrationId, web3Connection and _context. -> {"actionType":"web3/write-contract","config":{"network":"1","contractAddress":"0x…","functionName":"transfer","simulate":true}} passes refuseSimulateBody, passes validateRequest, survives the strip, reaches the step, which ignores the key - a real signed broadcast, the exact shape this PR forbids. config is where a node caller writes step parameters, so it is the likelier mistake rather than the unlikelier one, and the comment at :669 already models this surface for the other reserved keys. -> Extend the refusal into config, or add simulate to stripReservedConfig and refuse it there.

• app/api/execute/check-and-execute/route.ts - body.action is cast to ActionBody and its fields read individually, so {"simulate": false, "action": {…, "simulate": true}} broadcasts. Pre-existing rather than introduced, and outside the stated boundary - but the PR asserts an invariant across the family, and this is the second nesting that escapes it.

• docs/api/direct-execution.md:619 - the docs say simulate is rejected only when it is a non-boolean value, and say nothing about the two routes that now reject the key outright. -> A caller reading the page sends {"simulate": true} to a protocol action expecting it to be ignored, and gets a 400 with code: "unsupported_param" that appears nowhere in the documentation. The behaviour change needs the docs edit, the OpenAPI route and specs/api-coverage.json in the same PR.

Mechanical - actionable as-is

• tests/integration/execute-simulate-invariant.test.ts:522,569 - the file's header says "a route declared here automatically gets its behavioural checks - declaring a stance without wiring the guard fails this suite". It does not: Part 2 iterates HONORING_HARNESSES, Part 3 iterates REFUSING_HARNESSES, and nothing asserts ROUTE_STANCES is a subset of either. Add bridge/route.ts, declare {stance:"refuses"} to pass the discovery check, wire no guard, and the suite is green with zero coverage of it. One line per stance - expect(Object.keys(REFUSING_HARNESSES).sort()).toEqual(refusingStances) - makes the claim true.

• app/api/execute/[...slug]/route.ts:427 - the guard runs before executeProtocolAction branches on meta.actionType === "read" (:194-204), which calls readContractCore and never broadcasts. So a read action is refused with "it executes for real and cannot dry-run", which is false for that action. Refusing uniformly is defensible; the message is not.

• Guard ordering is inconsistent: five routes refuse before requireScope, swap and status after. The same request shape yields 400 on five and 403 on two for an under-scoped key.

• The 400s on the POST routes are emitted before checkRateLimit and are not wrapped in applyRateLimitHeaders, unlike every other response those routes produce, so a valid key can issue unmetered ?simulate= requests.

Does it match the description: Scope creep

Two seams, and each side ships and is correct with the other reverted.

Side A is refuseSimulateBody on [...slug] and node plus its two harness cases. It is the whole safety content, it is additive for every caller not sending the key, and it changes no documented contract.

Side B is rejectSimulateQuery on all seven routes, the rewrite of the existing query-string test, and the docs edit that is currently missing. It is a different kind of change: breaking for any caller sending ?simulate=, including ?simulate=false.

The invariant test depends on both, so it trims to whichever ships. Split them: A first, and B as an explicitly breaking API change with the documentation. The config gap in the first blocker belongs with A rather than after it - shipping A alone leaves the same hazard reachable through the nesting the file already warns about. Note the one-way semantic tie: with A reverted, B's own refusal text ("refused by every other /api/execute/* route") becomes a false statement the API emits about itself.

With the team

• Whether ?simulate=false and {"simulate": false} should be a 400. A generic client that always serialises the flag previously executed and now fails hard. I'm weighing refusing the key outright, which is simpler and cannot be mistaken for a dry run, against refusing only a truthy value, which keeps those clients working - the tradeoff is a silent behaviour difference between the two shapes against breaking a caller that was never asking for a dry run. I'm taking it to the core team and will come back. Nothing here is blocked on you.

Verdict

Changes requested - the invariant is enforced on the top-level key only, so config.simulate on /api/execute/node still signs and broadcasts.