Conversation
…sing
A tools/call sent with `arguments` omitted or explicitly `null` fails the
SDK's `z.record().optional()` params schema with a raw Zod error, which the
transport surfaces as -32603 (Internal error) rather than a validation
error. That's indistinguishable from the server being down, and it lands on
exactly the call an agent tries first: a tool with no required parameters
(get_wallet_integration, list_integrations, ...), invoked with no arguments
object at all.
Normalize params.arguments to {} at the dispatch layer, before the request
reaches the SDK, on both the org-authenticated (/mcp) and anonymous
(/mcp/public) routes. Every per-tool schema still enforces its own required
fields downstream unchanged (get_wallet_integration still requires
integrationId, just with a proper -32602 instead of a -32603).
Found while building an MCP client for the Agents Onchain Hackathon; full
writeup at https://github.com/harshkas4na/chaos-keeper/blob/main/docs/teardown.md#kh-1.
/analytics/runs returns network: null for a workflow run that failed
pre-flight, even when the run's own error names the chain. The aggregate
only reads the network from a step that produced gasUsed:
MIN(CASE WHEN gasUsed IS NOT NULL THEN input->>'network' END)
A run refused before broadcast - insufficient balance, spend cap, a bad
address - has no gasUsed on any step, so the CASE yields NULL for every row
and the run comes back with no chain at all.
Observed on a live organisation: three runs of one workflow, each failing
with "Insufficient BASE balance. Have: 0.0, Need: 0.000100231", all returned
network: null. The step's own log carries input.network = 8453.
Now prefers a gas-bearing step and falls back to any step that names a
network, so a successful multi-chain run is unchanged and a failed run keeps
its chain.
Why it matters beyond cosmetics: /analytics/runs is the audit trail an
external consumer reads to reason about an organisation's executions. A
failed run with no chain cannot be attributed, counted per network, or acted
on - and pre-flight failures are exactly the ones worth acting on, since
they cost nothing and repeat.
Not covered by a new automated test: this is SQL evaluated in Postgres, and
the repo's only test for this path (tests/unit/analytics-runs-route.test.ts)
mocks getUnifiedRuns, so nothing exercises the expression. Reproduction
against a live org: run a workflow whose transfer amount exceeds the wallet
balance on any chain, then GET /api/analytics/runs and read `network`.
KH-4 from the teardown referenced in #1949, #2040, #2041: get_execution's response is described only as "combined status and step-by-step logs in one response" -- the nested shape, and three ways it surprises a first-time caller, are nowhere written down. New reference page (docs/agent/mcp-get-execution.md), grounded in the current route/schema code rather than guessed: - totalSteps/completedSteps/duration are numbers in status.progress (parsed in the /status route) but strings in logs.execution (straight off the workflow_executions row, where they're text/numeric columns). - logs.logs is ordered newest-first (desc by timestamp), not execution order. logs.execution.executionTrace is the field that actually carries run order, and it's populated on every execution -- unlike status.errorContext.executionTrace, which only appears on error/ system_error runs. - status.errorContext is null (not omitted) on every non-error status. - The cross-organization / public-share-view case returns logs: null rather than an empty object. Linked from the get_execution row in the MCP tools table (docs/agent/mcp-server.md) and added to the docs/agent/_meta.ts nav, matching how mcp-validate-workflow.md is wired in. Docs only, no behavior change.
status.progress never had a duration field; it exists solely on logs.execution. Corrects the heading and body claim per review.
The log-summary subquery in fetchWorkflowRuns filtered on gasUsed IS NOT NULL. WHERE runs before GROUP BY, so a run whose steps all lack gas contributed no rows, formed no group, and left the join NULL - a pre-flight failure came back with network null and networks empty, even though logging.ts records the chain at step start and the run's own error names it. The runs table rendered "--" under Network for exactly the runs an operator opens when something broke. - Drop the gas predicate from the subquery. gasUsedWei is unaffected: SUM skips NULLs and the resulting 0 still maps to null, so "spent no gas" stays distinguishable from "chain unknown". - Read the denormalised network and gas_used_wei columns rather than re-parsing the double-encoded input/output JSONB, which matters more now that the subquery covers every step row of the page. - Restate the networks contract: it now lists the chains a run's steps targeted, not only those it wrote on. - Cover both with a Postgres-backed test that seeds a gas-free step log and asserts the returned network and networks. Tested against Postgres 16: a run seeded with a chain-bearing, gas-free step returns network base and networks [base]; a run whose gas-bearing step is on one chain and read-only step on another keeps base as its network and lists both.
executionTrace has no default (jsonb, no default()) and the pending- execution insert in app/api/workflow/[workflowId]/execute/route.ts does not set it, so an execution that never dispatches or fails before any step finishes leaves it null, not an empty array. Also re-attached the numeric-serializes-as-string parenthetical to duration, the actual numeric column -- total_steps/completed_steps are text, not numeric.
Only null hits the raw -32603 path; omitting arguments entirely already fails one layer in with a proper tool-scoped error. No behavior change.
Bumps the minor-and-patch group with 1 update in the /docs-site directory: [next](https://github.com/vercel/next.js). Updates `next` from 16.3.1 to 16.3.2 - [Release notes](https://github.com/vercel/next.js/releases) - [Commits](vercel/next.js@v16.3.1...v16.3.2) --- updated-dependencies: - dependency-name: next dependency-version: 16.3.2 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com>
checkMcpRateLimit tracked its sliding window in a module-level Map, so every replica enforced its own budget and the real ceiling was LIMIT * num_replicas. A caller spreading requests across pods got a multiple of the intended 120/min. The organization window now lives in a Redis sorted set behind the existing best-effort client in lib/redis.ts, keyed through lib/redis-keys.ts so the deployment namespace cannot drift. Sorted set rather than INCR plus EXPIRE: a fixed window admits 2 * LIMIT across a boundary, and the sliding form is what the existing reset/retryAfter metadata and the X-RateLimit-* headers already describe, so callers and tests keep the same RateLimitResult shape. Trim, count and add run in one Lua script; splitting the count from the increment across round trips would let concurrent requests each read a count under the limit and all be admitted, which is a bypass the size of the concurrency. When Redis is absent or the command fails the limiter falls back to the per-pod window instead of allowing the request. A Redis outage therefore degrades the ceiling to what it was before this change rather than removing it; failing open would leave the MCP surface unmetered exactly when the platform is least healthy. The fallback map is capped at 10k organizations, sweeping stale entries and then evicting the least recently active one, so an outage cannot be turned into memory growth. Command failures log through logSystemWarn, throttled to once a minute because this sits on the request hot path. checkMcpRateLimit is now async; both call sites await it. checkIpRateLimit is unchanged and still per-pod.
Review of the Redis-backed per-organization window found three gaps. Cold start was reported as an outage. The shared client is built with lazyConnect and enableOfflineQueue=false, so ioredis refuses the first command on a fresh client while the socket is still opening: sendCommand kicks off connect() from status "wait" and then rejects the write. Every process start therefore served its first decision from the per-pod fallback and sent "Redis unavailable" to Sentry, and since that warning is throttled to one a minute it also suppressed any genuine outage report for the first minute of a pod's life - training operators to mute the one alert that says the fleet-wide ceiling has reverted to LIMIT * num_replicas. The connection is now opened explicitly, warmed at boot from instrumentation, and awaited before the first command. Degradation had no signal a dashboard could read. Every degraded decision now increments ratelimit.mcp.degraded.total, labelled by reason, so whether the shared ceiling is actually being enforced is answerable without reading throttled Sentry warnings. The unexpected-reply branch carries the reply shape instead of reporting an Error titled "undefined", which is the only diagnostic that branch has. The Lua had no coverage at all. The unit fake reimplements the sorted-set semantics in JS and never looked at the script, so the suite stayed green with the script blanked - a state in which production would fail every EVAL, fall back to the per-pod window forever, and silently restore the ceiling this work exists to remove. Added an integration test that runs the shipped script against a real Redis (skipped when none is reachable) covering admit-exactly-LIMIT, TTL set on admit and not extended on refusal, same-millisecond members not colliding, integer reply precision and window trimming; plus a golden hash in the unit suite so the Lua cannot change without being re-verified. Also from review: - The window is scored by Redis's own TIME rather than the calling pod's clock. Replicas only shared a window to the accuracy of their local clocks, and a pod drifting past the window length would have written entries every other replica immediately trims, quietly buying itself a private budget from an infrastructure fault. - Fallback eviction is an O(1) insertion-order LRU instead of two full map scans on the request path, and the cap now covers the IP map too - the one an unauthenticated caller can actually flood between sweeps. A key at its limit is written back on refusal so it cannot be evicted into a fresh window. - Route tests mock the limiter with a resolved promise, so a call site that forgets the await turns into a blanket 429 that fails CI rather than only showing up in production.
MCP clients batch approvals from readOnlyHint and destructiveHint. The spec default for destructiveHint is true, so writing false is an explicit assertion that a tool is only additive, and destructiveHint is ignored entirely when readOnlyHint is true. Twelve tools plus every marketplace listing made a safety claim they could not support, so agent workspaces auto-approved them with no human in the loop. Audited all 44 tools on the authenticated surface. Flipped destructiveHint to true on execute_workflow, execute_protocol_action, call_workflow, tempo_sign_and_hold, tempo_cancel_hold, tempo_release_hold, update_workflow, list_workflow and update_workflow_listing. execute_protocol_action routes both reads (chronicle/eth-usd-read) and writes (aave-v3/supply) through one actionType argument, so the static annotation takes the worst case; the tool is not split here because that breaks a public MCP surface. create_workflow is also destructive. It takes an unconstrained nodes array alongside enabled, so one call arms a schedule/event/block/webhook trigger over arbitrary node content, which is a superset of execute_workflow. test_notification is destructive too: type and config are caller-supplied and reach the plugin unfiltered, sending an unrecallable message to a caller-named target or opening a database connection to a host it names. Persisting nothing does not make an outbound emission additive. createWorkflowMcpServer no longer derives its hints from listing.workflowType. That field does not describe side effects and points the wrong way: the call route sends a "read" listing to handleReadWorkflow and runs the whole body server-side on the owner's wallet and credentials, while a "write" listing only returns unsigned calldata for the caller to sign. deriveWorkflowType compounds it by classifying "write" only for write-contract and protocol-write nodes, so a listing that transfers funds, approves a spender, signs typed data or sends mail stays "read". No per-action read/write metadata exists to classify an author-controlled body, so every listing now advertises readOnlyHint false and destructiveHint true rather than claiming a safety we cannot establish. ai_generate_workflow claimed readOnlyHint true while /api/ai/generate hard-requires mcp:write, so the hint contradicted the scope model. It is now a non-destructive write. ai_generate_workflow, create_project, create_tag and deploy_template keep destructiveHint false: each only inserts, and the deploy_template clone falls to the schema default of disabled because the duplicate route sets no enabled column. That set is an explicit allowlist enforced by the new exhaustiveness test, so a future tool cannot quietly join it. idempotentHint and openWorldHint stay at their spec defaults, which are already the conservative values here. Documented the policy above registerTools. No existing test asserted the old values.
The header comment and the PR description both described the pre-change ceiling as LIMIT * 2. Prod runs replicaCount 4 (deploy/keeperhub-stack/prod/values.yaml), so the advertised 120/min was really 480/min and this change cuts the effective ceiling by three quarters rather than half. Staging runs a single replica, so its limit is already 120/min and this change is a no-op there -- worth stating, because it means staging cannot surface the impact before prod does.
Deriving destructiveHint from the read-only check propagated an existing gap into a second annotation. hasMutatingNode only matched write-contract/protocol-write plus three named web3 types, so a listing whose only action was a Tempo transfer was typed read, passed the check, and advertised readOnlyHint true with destructiveHint false. Tempo carries no native gas token, so its writes never register on the daily native value cap either -- the annotation was the only thing between an MCP client and an auto-approved stablecoin transfer. Adds a second predicate rather than stretching the first. isMutatingActionType keeps meaning chain state, and its docstring's warning about staying off the calldata path still holds. hasIrreversibleEffect is what the annotations use: chain mutation or an outbound message. The MCP spec defines readOnlyHint as not modifying the environment, which is wider than not modifying chain state -- an email is as unrecallable as a transfer. tools.ts already annotates test_notification destructive on exactly that reasoning, so a listing doing the same send now agrees with it instead of contradicting it. Both sets stay allowlists, so an unrecognised action type still reads as side-effect-free. That residual is now a test rather than an assumption; a declared side-effect field on PluginAction is what would close it. Also reverts ai_generate_workflow to readOnlyHint true. It persists nothing, and withScopeCheck enforces mcp:write regardless of any annotation, so the flip bought no enforcement and cost an accurate signal: clients allowlisting read-only tools would prompt for a call that mutates nothing. Model spend is a cost the annotation vocabulary cannot express. The suite now allows read-only tools to require a write grant, since effect and grant are separate axes, and guards that exception list. Adds an assertion that the duplicate route creates the clone disabled, which is the invariant deploy_template's non-destructive annotation rests on and which nothing previously enforced.
…site/staging/minor-and-patch-9d5bc31b60 chore(deps): bump next from 16.3.1 to 16.3.2 in /docs-site in the minor-and-patch group across 1 directory
processBlock issued one eth_getLogs per block per 500-address batch, driven off each newHeads push, so request volume scaled linearly with block rate. At Ethereum's 12 s that is 7,200 calls/day/batch and at Base's 2 s it is 43,200, but Robinhood Chain produces a block every 100 ms: 864,000 calls/day/batch, twice the entire platform's current RPC volume across all 26 chains, from a single subscriber batch on one chain. Blocks from a chain observed to be producing them faster than BATCHING_BLOCK_INTERVAL_THRESHOLD_MS are now buffered into a BATCH_WINDOW_MS window and served by a single ranged request, taking a 100 ms chain to 86,400 calls/day/batch - a bounded factor of two against Base rather than twenty. Cadence is measured rather than configured. No per-chain block time reaches this process, and an observed value needs no migration when a chain is added or its cadence changes. A chain dispatches per block until twenty intervals have been folded into the estimate, so a misjudged cadence at startup can only fail towards the existing behaviour, and the estimate resets on every block-listener attach: carrying it across a reconnect would let a stale value decide batching for what may be a different upstream. The threshold sits at 500 ms rather than the 1 s round number. The integration rig runs anvil at --block-time 1, and a chain sitting exactly on the boundary would drift across it on timer jitter alone and batch nondeterministically. 500 ms leaves 5x margin below to the chain this exists for and 2x above to the fastest cadence anything here produces, so every chain supported today keeps one request per block and the latency that goes with it. The window is measured from its first block, bounding the delay any single block waits rather than sliding forward as blocks keep arriving, and is capped at BATCH_MAX_BLOCKS so a burst cannot widen the range without bound. A ranged request also covers blocks the subscription skipped inside the window, which recovers logs the per-block loop dropped; the dedup layer absorbs the duplicates that produces. Open windows are discarded in detachBlockListener, the single teardown chokepoint, so no request fires against a provider that is about to be destroyed. BLOCK_STALENESS_TIMEOUT_MS was slack measured in wall-clock rather than in blocks: 120 s is ten blocks on Ethereum but 1.2 million on a 100 ms chain, where a silently dead subscription would go unnoticed far longer in the terms that matter. Only a chain fast enough to be batching now derives its own threshold, max(30 s, interval x 10), so every chain dispatching per block keeps the historical value exactly and this cannot introduce reconnect churn on a chain that behaves today.
…ing it The coalescing window was discarded whenever the block listener detached. That is right for unsubscribe and destroy, where no subscriber remains to dispatch to, but wrong for a reconnect: its subscribers are still attached and still expect those logs, so discarding loses events that per-block dispatch never could. Worse, the window was not reaching that point at all. reconnectLoop sleeps INITIAL_RECONNECT_DELAY_MS before its first attempt, and that delay is the same width as BATCH_WINDOW_MS, so the window timer fired against the connection that had just failed: the request threw, and the buffered blocks were lost to a caught warning before reconnect() ever ran its teardown. Disarm the timer in triggerReconnect, alongside the heartbeat it already stops, and keep the blocks. detachBlockListener now takes the discard decision from its caller rather than assuming it, and the reconnect serves the carried range on the replacement connection - the heights are historical by then, so it answers for them as well as the original would have. Found by a second review pass on the batching change; the test written for it failed against the first fix, which is what exposed that the timer was firing early rather than being discarded late.
Batching block requests is a cost change, and nothing outside this process can measure whether it worked. The package ships no prom-client, no OpenTelemetry and no /metrics route, and the health server binds HEALTH_PORT (default 3001) while the deployment declares only 3000, so an endpoint added here would not be reachable. Aetherlay cannot stand in either: aetherlay_endpoint_proxy_requests_total increments once per WebSocket connection (server.go:761, under "WebSocket connection succeeded", returning immediately after) and per request only on the HTTP path, while the proxy's own message loop counts nothing. Every eth_getLogs this class issues rides a long-lived socket, so none of them appear in that series on any cluster. That leaves the log stream, which the package logger already targets: it emits canonical single-line JSON specifically so Loki can aggregate across the app and its satellites. Count calls at the request rather than the range, since one range spanning more than GETLOGS_ADDRESS_BATCH addresses is still several requests and requests are what a provider bills. Report one line per chain that did work, resetting the interval counters and carrying a cumulative total, and stay silent for idle chains so a quiet tracker does not emit a line per chain per minute forever. The counters live on the chain entry rather than the connection, so a reconnect re-learns cadence but does not reset cost. blocksCovered against getLogsCalls is self-comparing: roughly equal while dispatching per block, blocks far ahead of calls once a window engages. That matters because nothing has ever counted this quantity, so a later reading has no historical baseline to be compared against. Also surfaced on ChainHealth so the same numbers are reachable without log search if that endpoint ever becomes reachable.
Three corrections from review of the previous commit. blocksCovered and ranges were incremented before the request, so a range whose eth_getLogs threw still counted every block it never fetched logs for. That inflates the blocks-per-call ratio precisely when a chain is failing, making the reading look most efficient when the chain is least working. Both now increment only after the range's requests return. getLogsCalls stays at issue: a request that failed was still made and still billed. A failing chain therefore reports calls and errors with no coverage, which is the honest shape. The stats timer no longer calls unref. No other timer in this class does, destroy clears it either way, and unref is not on setInterval's return type under every lib configuration this package compiles against - so it was the one line here whose type safety depended on ambient resolution. The counters are packed as key=value inside the JSON msg string, because the logger has no structured-field API. The previous commit message implied `| json` alone surfaces them; it does not, it yields msg as one string. The query needs the inner parse and the comment now carries it. Adds a getLogs failure path to MockProvider, following the convention blockNumberResponses already uses, so the coverage-on-failure behaviour is tested rather than asserted.
Review of the windowed-batching approach returned fourteen findings, six of which were the same shape: a mechanism that measured the chain, decided whether it qualified, and then had to defend that decision against bursts, threshold drift, reconnects and its own timers. Replacing the mechanism removes them rather than patching each. Each chain now keeps `lastProcessedBlock`, and a drain fetches `(mark, head]` at most once per GETLOGS_MIN_INTERVAL_MS. Nothing branches on how fast a chain is. A chain whose blocks arrive further apart than the interval always finds it already elapsed and dispatches immediately - one request per block, the historical behaviour and latency, with no threshold deciding it. A faster one finds it has not, its head runs ahead of the mark, and the accumulated range is served in one request. A 100 ms chain is still bounded to one call per second per chain. What that deletes, and the finding each one closes: - The batching threshold, the EWMA gate and the warm-up count. Dispatch no longer consults cadence, so a burst of queued newHeads messages cannot flip a 12 s chain into batching, and no chain can oscillate across a boundary. - The window timer. It could be re-armed by any block arriving during a reconnect backoff, because disarming it in triggerReconnect stopped the timer and not the work. `drain` refuses to run while isReconnecting, so the guard is now on the work itself. - pendingBlocks and the retainWindow special case at detach. There is no buffer to decide the fate of; the mark records what is owed, survives a reconnect, and the replacement connection resumes from it. - Math.min/Math.max over buffered heights, which bounded the block count but not the span it queried. A contiguous range from the mark is capped at GETLOGS_MAX_BLOCK_SPAN with the remainder left owed, and a gap past GETLOGS_MAX_CATCHUP_BLOCKS is abandoned with a warning rather than walked at one request per second while current blocks wait behind it. Three further fixes: processBlockRange now reports whether the range was served, and the mark only advances when it was. A failed range stays owed and the next drain re-queries it, where before a single failure dropped every event in up to 25 blocks with no retry. A partial multi-chunk failure dispatches nothing rather than delivering the chunks that happened to return and never fetching the rest. reconnect() no longer awaits the flush. dispatchLog awaits its handlers, each of which may sleep seconds of EventListener jitter, so awaiting it held isReconnecting true long after the socket was healthy - reporting the chain degraded on /healthz, blocking every getOrCreateProvider behind entry.reconnectPromise, and suppressing the heartbeat and staleness checks that would have caught a second drop. The staleness threshold is clamped at both ends and its multiplier raised to 60. max(30 s, interval x 10) could never return anything but the floor: the branch required an interval under 500 ms, so the product was always under 5 s, which made the constant dead and the documented rule unobservable. At 60 blocks with a 120 s ceiling, Base and Ethereum keep exactly the 120 s they had and a 100 ms chain takes the 30 s floor, with the multiplier deciding everything between. Trusting the estimate now also requires the samples to span real wall clock - measured across the samples, not to now, since silence would otherwise supply the span at exactly the moment the threshold is used. Also: ChainHealth.getLogsCalls is renamed getLogsCallsTotal to match its source, since the log line's getLogsCalls is a per-interval count and one name for both invited reading a rate as a counter. blocksCovered is documented against `ranges` rather than `getLogsCalls`, which counts one per address chunk and so halves the ratio for a chain past GETLOGS_ADDRESS_BATCH addresses for reasons unrelated to coalescing. shutdownRegistry now destroys the provider manager - nothing in src ever called destroy(), so providers and the stats interval outlived the listeners they existed for. Tests import the production constants instead of re-declaring them as literals, share one set of helpers, and cover each finding: per-block dispatch on a slow chain, coalescing on a fast one, the request-rate bound, skipped blocks, the span cap and its remainder, the abandoned gap, overlap under an in-flight request, retry of a failed range, no advance past a failed chunk, resumption from the mark across a reconnect, no request against the replaced connection, isReconnecting cleared without waiting on dispatch, and both clamp ends plus burst immunity on the staleness threshold.
…batching perf: KEEP-1088 batch eth_getLogs for sub-second block chains
docs-site/package.json moved to next 16.3.2 while docs-site/pnpm-lock.yaml still pinned 16.3.1. The Dockerfile installs that directory with --frozen-lockfile, which refuses a manifest the lockfile does not match, so every image build failed at the deps stage: ERR_PNPM_OUTDATED_LOCKFILE - next (lockfile: 16.3.1, manifest: 16.3.2) Staging is red for the same reason, so this blocks the build job on every open PR rather than any one branch. Regenerated with --ignore-workspace. Without that flag pnpm resolves the root workspace instead and leaves this lockfile untouched, which is the trap that makes the two drift apart: a bump lands in the manifest, a plain install appears to succeed, and nothing regenerates the file the image actually uses. Second occurrence in three days; KEEP-1212 fixed the same drift at 16.2.12 to 16.3.1. Worth a CI check that the docs-site manifest and lockfile agree, so the next dependency bump fails its own PR instead of staging.
Dependabot opens manifest-only bumps for /docs-site. It edits docs-site/package.json and leaves docs-site/pnpm-lock.yaml behind. The docs image installs with --frozen-lockfile, so the build then fails with ERR_PNPM_OUTDATED_LOCKFILE and stays red on every branch. This happened four times since June. Cause: pnpm walks up from the install directory to find a workspace root. Commit e192a74 added the repo-root pnpm-workspace.yaml on 2026-04-23, and it lists only "." and "sandbox". An install started in docs-site reaches that file and resolves the root workspace, so it never writes docs-site/pnpm-lock.yaml. Dependabot runs that install with no --ignore-workspace flag, sees no lockfile diff, and ships the manifest change alone. The dates match. PR #945 is the last docs-site bump that carried a lockfile and it merged about 10 hours before the root workspace file landed. keeperhub-events keeps its lockfile updated because it has its own pnpm-workspace.yaml. This adds the same file to docs-site, so pnpm stops its upward walk there. The Dockerfile deps stage copies it too, so the image install reads the same workspace context that produced the lockfile. Reproduced and verified with the pinned pnpm 10.33.3: - Before the change, `pnpm install --lockfile-only` inside docs-site reports "Scope: all 2 workspace projects", exits 0, and leaves docs-site/pnpm-lock.yaml byte-identical while the manifest declares next 16.3.2 and the lockfile pins 16.3.1. That is the command Dependabot runs. - After the change, the same command resolves 497 packages and updates docs-site/pnpm-lock.yaml. - The regenerated lockfile is byte-identical to the one PR #2134 produced with --ignore-workspace, so the change alters no resolution. - The root workspace still reports 2 projects and its lockfile passes --frozen-lockfile, so docs-site did not join it. - `docker build -f docs-site/Dockerfile --target deps .` and `--target builder .` both complete.
A cluster with no cloud load balancer answers a LoadBalancer Service with a proxy on each node. A request that arrives at a node without the ingress controller pod is forwarded, and the source address becomes an address from the cluster's own pod range. The install still works, so nothing points at the ingress controller. Step 2b now states the trap and gives the remedy, which is to run the controller as a DaemonSet. Measured during the M0 acceptance run (KEEP-1114). KEEP-1219.
app/api/execute/_lib/auth.ts returned the scope on the OAuth branch but
dropped it on the API-key branch, even though authenticateApiKey already
resolves and returns organization_api_keys.scope. scopeSatisfies(undefined, X)
is unconditionally true, so every requireScope() call across the seven
app/api/execute/* routes was a no-op for kh_ keys: a key minted mcp:read
could broadcast via /transfer, /contract-call, /check-and-execute, /node and
the protocol dispatcher. Forward result.scope so the gates actually apply.
Undefined stays full access. Every requireScope caller was enumerated: four
auth resolvers feed them, and getDualAuthContext, resolveOrganizationId and
resolveCreatorContext already propagated apiKeyAuth.scope while returning no
scope on their session branch. validateApiKey was the outlier. Cookie
sessions and internal callers never set a scope and are untouched.
Not a no-op for existing credentials. The scope column is nullable, but the
dashboard mint dialog sends the Permissions selection on POST /api/keys, so
org keys created through the UI do carry a scope. The default has all three
boxes checked, which satisfies write, but a key created with Write unchecked
goes from working to 403 at the execute endpoints. That is the intended
behaviour - a read-only key must not broadcast - and it needs a count of
affected rows before deploy:
SELECT count(*) FROM organization_api_keys
WHERE revoked_at IS NULL AND scope IS NOT NULL
AND scope NOT LIKE '%mcp:write%' AND scope NOT LIKE '%mcp:admin%';
Keys with scope NULL are coerced to undefined and keep full access, so the
kh CLI device grant (which hardcodes null) is unaffected.
Also in this change:
- requireScope emits a logSecurityEvent on denial, with the required and
granted scope plus an optional organization/credential/endpoint context the
execute routes now pass. The deny path was silent, which made a spike in
403s indistinguishable from a stolen read-only key probing a broadcast sink.
- The Permissions copy in the key-creation dialog now says that Read cannot
run workflows or send transactions, and the wallet step-up mint path sends
the selected scopes. It previously discarded them, so a wallet user who
unchecked Write still received an unscoped, full-access key.
- Public docs corrected: docs/api/direct-execution.md claimed "API keys are
unaffected by scope", and docs/api/api-keys.md did not document the scopes
field on key creation.
- Route-level regression coverage for the five sinks that actually broadcast,
driving the real handlers through the real validateApiKey and the real
requireScope, and pinning the polarity of the simulate ternary in both
directions. Verified failing against the unpatched auth.ts.
specs/api-coverage.json records the line each documented route sits on in docs/api/direct-execution.md. This branch corrects the scope statement on that page, shifting the lines below it, which leaves the checked-in coverage file stale and fails the lint job.
requireScope passed a Sentry payload to logSecurityEvent on a path any caller can drive at its own request rate. lib/logging.ts states the rule in logUserError's own body: user errors are deliberately kept out of Sentry because they are expected, high-volume, and would drown actionable system errors. A scope denial is the caller using a credential outside its grant, not a platform fault, so it belongs on that side of the line. The per-scope fingerprint collapsed the issue but every event still billed against quota, and nobody can action 'an integrator is misconfigured'. The description also claimed the denial became countable. It did not: logSecurityEvent writes Sentry and Loki and never touches Prometheus, so there was no series to alert on. Adding logUserError emits the counter, which is what Grafana can actually threshold. The structured security line still lands in Loki, so a detection query over repeated denials from one credential is unchanged. Also reorders the wallet create-key payload so the step-up spread cannot override the operator's scope selection, and adds logUserError to the four suites whose hand-rolled logging mocks would otherwise throw and turn the 403 under test into a 500.
filterLabelsForMetric keeps only ERROR_LABELS, which carries endpoint and the error_context taken from the message prefix. required_scope, granted_scope and organizationId are not in that list, so they are dropped before Prometheus and survive only in the Loki line. The deny rate is therefore alertable per endpoint but not breakable down per scope or per organization, which is not obvious from the call site passing all three. Noted so nobody builds a per-scope panel against labels that never arrive. Adding required_scope to ERROR_LABELS would enable that slice cheaply -- three possible values. organizationId should stay out: per-org labels are exactly the cardinality the allowlist exists to keep off Prometheus.
fix: propagate API key scope on the direct execution API
…all arguments A JSON-RPC 2.0 array `params` is not a shape MCP uses, but spreading one into an object silently rewrote it before the SDK ever saw it. Guard on Array.isArray so it passes through and the SDK rejects it on its own terms.
The -32603 fix is the dispatch-layer wiring, not the helper. A pure-function
test stays green if either route goes back to handing `await request.json()`
straight to the transport, so assert on what the transport actually receives:
parsedBody.params.arguments is {} for both the null and omitted shapes, on
/mcp and /mcp/public. Verified red without the wiring.
The session branch is the only path a tools/call takes on /mcp/w/[slug], and it handed the live Request to the SDK, so `arguments: null` still surfaced as -32603 on the marketplace surface external agents actually call. Parse the body up front and pass it through parsedBody, mirroring /mcp; ensureMcpAcceptHeader no longer forwards the (now consumed) body stream, which also drops the duplex ts-expect-error. Malformed JSON on a session POST now returns the route's own 400 instead of reaching the transport, matching /mcp.
…ments fix: #2108 default tools/call arguments to {} — only null hits the opaque -32603, omitted already fails cleanly
Propagating scope on /api/execute (#2030) turned four latent edges into live ones. Each was correct for OAuth and wrong for kh_ API keys, which could not reach these paths until scope started being enforced. The 403 body told every denied caller the ceiling is raised by an admin under Settings > Developer > Agents. mcpMaxScope is read only inside authenticateOAuthToken, and an API key's scope is fixed in the row at creation, so for a key that sentence names a control which does not apply and omits the one that does. requireScope now takes the credential family and picks the matching remediation, claiming none when the caller does not say. The shared part of the message no longer says "OAuth". parseScopeInput returned null for an explicitly empty scopes field, and null means no restriction, so asking for the narrowest key produced the widest one. Only omitting the field reaches null now; a supplied value floors at mcp:read, as ["bogus"] already did. The Permissions block rendered on both key tabs, but nothing reads apiKeys.scope: the webhook route looks a key up by hash and dispatches without consulting it. "Cannot run workflows or send transactions" was false there. It is now shown only for organisation keys, and webhook creation omits scopes so the stored column says what is true. The denial also incremented errors.system.auth.total, which the errors dashboard sums into its System Errors panels unfiltered - a caller using a credential outside its grant is not a platform fault. It moves to a user-side authorization category. errorCategory is a text column, not a pg enum, so the schema union tracks it without a migration. No dashboard charts errors.user.authorization.total yet, so the denial rate is not visible anywhere until a panel is added.
require-scope.test.ts pinned the denial's error category as "auth". That was the point of the change: "auth" maps to errors.system.auth.total, which the errors dashboard sums into its System Errors panels, and a caller using a credential outside its grant is not a platform fault. execute-auth-anonymous.test.ts deep-equals what validateApiKey returns, so adding credentialType to the context broke three cases that assert the shape rather than the fields they care about. The OAuth branch and both API-key branches now name the family they came back from.
…ing path Review of the previous commit found the metric change made the deny rate worse, not better. errorCounterMap in the Prometheus collector had no entry for errors.user.authorization.total, so recordErrorCounter logged "Unknown error metric" and dropped the count - where errors.system.auth .total had at least been recorded, in the wrong panel. On a path a caller drives at its own request rate that is a lost series plus a warn line per denial. The counter is now defined and mapped, so the claim in require-scope.test.ts that the rate is countable holds again. parseScopeInput still returned null for anything that was neither array nor string. scopes is unvalidated request body, so an object - the shape the creation overlay holds internally - or a boolean reached the same full-access default the change set out to remove. Only undefined and null are read as omission now; anything else present floors at mcp:read. Also from review: schema.ts tracks ErrorCategory by reference instead of a hand-copied union, so the next category added does not have to be remembered in two files. logSecurityEvent carries the credential family, without which a Loki query cannot separate an OAuth grant clamped by a ceiling change from a leaked kh_ key probing above its scope. The 403's API-key sentence no longer reads as an instruction to mint more scope, since nothing clamps key creation against the org ceiling. The direct execution docs printed the old message verbatim on the page whose primary credential is an API key. ScopeDenialContext reuses AuthMethod rather than declaring a parallel union with the same string values.
specs/api-coverage.json records doc line numbers, so rewriting the 403 example in docs/api/direct-execution.md moved them and the lint gate went stale.
Verified each assertion against lib/mcp/tools.ts, both execution routes, lib/workflow/execution-access.ts and lib/db/schema.ts. - executionTrace is [] once progress is initialized (before the first step runs), not null until the first step completes; null now means progress was never initialized, the same condition under which totalSteps is null - totalSteps has no column default, so logs.execution.totalSteps is null (not a string) on a run that was never dispatched, while status.progress coerces it to 0 - duration also exists per step on every logs.logs entry, and survives nodeIds filtering; it is not a logs.execution-only field - includeData: false omits three blobs per log entry only; every row is still returned and logs.execution keeps its own input/output - logs.execution embeds the full workflow row including nodes and edges, which none of the three arguments reduce - the cross-organization redaction is an allowlist: transactionHashes keeps only hash and chainId, and errorContext loses executionTrace and error; qualified the owned-path claims that contradicted it - the deprecated aliases 404 on a public-share execution rather than sharing get_execution's caveats, and v1.13 is their removal release, not the start of deprecation - nodeIds survivor list was missing nodeId, id, executionId, network, gasUsedWei; truncateData is a positive integer, not any number - status and logs are two concurrent HTTP reads, not one table each, so completedSteps can differ between them mid-run - errorContext stated as a rule rather than a closed status list - wired the page into docs/agent/index.md and moved it after the authoring trio in _meta.ts so both navigation surfaces agree
The previous commits fixed the REST sinks and left four gaps behind. MCP tools denied with the same OAuth-only remedy the 403 body no longer uses. A kh_ key is the credential that reaches those tools most often, and it was handed an upgrade_url to /settings/mcp/reauthorize plus a hint to request the scope on the consent screen - a page with nothing behind it for a key whose scope was written at creation. registerTools and registerMetaTools now bind the family alongside the scope, so the envelope drops upgrade_url for an API key and names the real remedy. The MCP route derives the family from the credential id in one place, since a session rebuilt from its token has nothing else left to read. The ~35 requireScope sites outside /api/execute passed no context, so after the last commit an OAuth caller there got no remediation at all - correct, but less than it had. Each one's auth helper already returns authMethod, so all 39 now pass it. Two local Ok types widened it to string on the way through; narrowed so the compiler keeps them honest. Webhook keys still displayed a scope on both read surfaces. The list in the overlay and the settings table showed "Scope: read" for a credential the webhook route dispatches on without ever consulting the column. Both now show it only where it is enforced. The table's "--" for a null scope also read as "no permissions" when it means the opposite; it says unrestricted. Key creation pre-checked all three scopes, so the least-effort mint was a full-access mcp:admin key - the widest-by-default outcome this branch removed from parseScopeInput. Read alone is checked now.
…rpc-urls fix: KEEP-1213 redact RPC credentials from the health payload
…-shape docs: document get_execution's response shape
PR #2031 made an absent organization_spend_caps row resolve to a platform default of 0.02 ETH per day instead of unlimited. Every protocol-coverage suite executes as the one persistent org e2e-test-org, so the native value they move accumulates against a single daily ledger per shard. Shard 4 sends 0.04 ETH: 0.01 for wrapped/wrap, plus 0.01 for each of the three frax-ether-v2 mints. wrapped runs first, so frax/mint lands exactly on the 0.02 cap and passes, and mint-and-give and mint-and-stake are denied. Seed the org an explicit daily_value_cap_wei of 1 ETH. CI then keeps the same platform default as staging and prod, which both pin 0.02 ETH, and this org carries its own ceiling instead. The figure is 25 times the current worst case, so a new payable fixture does not break the suite again. The write is an upsert, not an insert-if-absent: the reservation path creates a row with both cap columns NULL on the org's first value-moving request, and such a row still resolves to the platform default.
fix: correct the API-key scope edges around the direct execution API
The previous commit switched the log_summary subquery onto workflow_execution_logs.network and gas_used_wei. Migration 0117 added both columns null and only the executor writes them, so every row written before it has them NULL - including the pre-flight failures this branch exists to repair, and every historical gas-bearing step. A column-only read therefore returns no chain for an old failed run and no gas for an old successful one. Read COALESCE(column, JSONB extract) in all three aggregates instead. Correctness stops depending on an operational backfill having run, and the JSONB arm goes cold on its own as the columns fill. The re-parse cost that motivated the column-only read belongs to fetchNetworkBreakdown, which is unbounded; this subquery is already restricted to one page of executions, which is why it could afford the JSONB extract before this branch touched it. Widen the backfill so the columns can eventually carry every row. It keyed on gas-bearing rows only, on the stated reasoning that nothing read network on a gas-free row - which this branch makes false, since a pre-flight failure is exactly a row with a network in input and no gasUsed in output. Its SQL moves to scripts/lib/ so the database-backed test can run a batch rather than assert the widening in prose. Verified against Postgres 16. The two new legacy-row cases fail on 6245e1c (expected null to be 'base'; expected null to be '31000') and pass here; the backfill case returns 0 rows written under the old gas-only predicate and 1 under the widened one. tsc --noEmit clean.
The file is emitted by scripts/discover-plugins.ts on every dev/build run, so the hand edits were discarded on the next build. They were also wrong: the slug list named file stems rather than the slugs the definitions declare, and the yearn import was renamed to yarnV3Def, which the generator cannot produce. Re-running discover-plugins reproduces this restored file byte for byte.
The body-scan map choice now lives in resolveNestedForEachEdgeMap, next to identifyLoopBody and planIterationContinuation. It takes both candidate maps and returns the one a nested scan must use, so the reason the outer loop's partial map cannot serve the inner scan is stated once, at the point of the decision, instead of being implied by an argument name at the call site. The tests run the recursion around that resolver rather than choosing a map themselves: scan the outer loop, hand its real bodyEdgesBySource and the global map to the resolver, scan the inner loop with what comes back. Return outerBodyEdgesBySource from the resolver and three assertions fail, at two and three levels of nesting. Also imports the executor's own buildEdgesBySource instead of a local copy that lacked the duplicate-target guard, and gives condition-due the true edge that makes it gate the inner loop rather than dangle.
# Conflicts: # specs/api-coverage.json
runs-table.tsx decided "Composed" from run.networks.length, and its own comment states the criterion it means to apply: a run that spent on more than one chain cannot sum into one token. That is a claim about which chains bore gas, not which chains the run targeted. The two sets coincided only while the logSummary subquery gated networks behind gasUsed IS NOT NULL, so the guard was reading the right values through the wrong field. Removing that gate makes networks the targeted set for real, and a workflow that writes on one chain and reads on another renders "Composed" instead of a total that is perfectly summable. Narrowing networks back would re-hide the mismatch, and would restore a contract already false on the direct half of the union: queries.ts builds a direct run's networks from directExecutions.network, which is recorded whether or not gas landed. Add gasNetworks to the runs payload - the subset of networks that spent gas, built in the subquery from fragments already in scope - and key both the guard and the token symbol off it. The symbol matters independently: line 144 picked an arbitrary element of networks, which under the widened contract can be the read-only chain, so a Base amount could have printed with another chain's token. The old guard short-circuited before that line, so it was latent rather than live, but it was correct by accident either way. Ledger-only gas names no chain of its own and still borrows the run's, which is unambiguous only on a single-chain run; that path is unchanged. Verified against Postgres 16 and in unit. The two new gas-attribution cases fail against the old guard (expected 'Composed' to be '0.10 ETH'; expected 'Composed' to be '0.10 POL'). 6 of 6 unit, 6 of 6 database, biome clean on the touched files, tsc --noEmit exits 0 with no diagnostics.
fix(execute): #1930 fail closed on unsupported check results
…eflight-failure fix: #2093 keep the chain on a run that failed before broadcast
fix: #2049 pass global edgesBySource to nested For Each body scan
…-cap fix: KEEP-1239 give the e2e test org an explicit daily value cap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes
stagingtoprod. 86 commits, 22 PRs, 135 files, no DB migrations.Scope grew after this PR was opened, because it tracks
staging. It was 3 PRs at 2026-08-25 09:42Z, 16 at 2026-08-26 01:02Z, and is 22 now.Blocked, one thing
protocol-coverage shard 4 fails, and it is a real break
Run 32962124438, shard 4: 2 of 71 tests failed, in
tests/e2e/vitest/protocol-coverage/frax-ether-v2/ethereum/coverage.test.ts.#2031 causes this. It makes an absent organization spend cap resolve to a platform default
instead of unlimited. The default is
DEFAULT_DAILY_VALUE_CAP_WEI = "20000000000000000"inlib/execute/spend-cap-defaults.ts:52, which is 0.02 ETH.The cap is per organization per day, and the coverage suite shares one organization across a whole
shard, not per file.
createSharedCtx()(tests/e2e/vitest/protocol-coverage/_shared/setup.ts:45)creates no organization at all; every suite runs as the seeded
PERSISTENT_TEST_ORG_SLUG = "e2e-test-org"(tests/utils/db.ts:33). So each shard has a singledaily ledger that every protocol in it draws down.
Enforcement is strictly greater, in
lib/execute/value-ledger.ts:254:so a request landing exactly on the cap is allowed, and only the next one is denied.
Shard 4 (
.github/workflows/e2e-tests-ephemeral.yml:920) holds bothwrapped/ethereumandfrax-ether-v2/ethereum, and files run serially within a shard (vitestfileParallelismis off).wrappedspends first, so frax inherits a ledger that is already half consumed:running + sends > 0.02?wrapped/wrapfrax/mintfrax/mint-and-givefrax/mint-and-stakeThat is exactly the run:
mintpassed, the other two failed. Deterministic, not a flake, and itwill fail on every re-run while the shard layout holds.
The same model accounts for the three green shards. Shards 2 and 3 contain no payable fixtures.
Shard 1's only payable fixture is
rocket-pool/depositat 0.02, which passes solely because 0.02is not greater than 0.02. Shard 1 has zero headroom: one more payable fixture, or any reshard
that moves
wrappedinto it, breaks it too.e2e-tests / e2e-gateis red only as a consequence. Its log reports every other need assuccessand closes on
protocol-coverage-ephemeral(failure).Why it reached
staging: #2031's own CI skipped protocol coverage.protocol-coverage-ephemeralandprotocol-coverage-floorboth report SKIPPED on that PR, ande2e-gatepassed because they were skipped rather than because they ran. The suite only runs infull on the release PR.
The fix: seed the cap the fixture org should have had.
scripts/seed/seed-test-wallet.tscreates
e2e-test-organd its Turnkey wallet but never writes anorganization_spend_capsrow,which is why the org falls through to the platform default at all. Adding the row sets an explicit
daily_value_cap_weiof 1 ETH, well above per-shard demand (0.04 ETH on shard 4 today), so a newpayable fixture does not re-break the sweep. It also closes shard 1's zero headroom.
One caution for anyone touching this:
organization_spend_capshas two similar columns,daily_cap_wei(gas) anddaily_value_cap_wei(native value moved). Only the second one governsthis failure. The existing precedents in
scripts/seed/seed-analytics-data.ts:584andtests/e2e/playwright/utils/seed.ts:684both writedaily_cap_wei; copying either is a silentno-op that leaves the suite failing identically.
The alternative, overriding
EXECUTE_DEFAULT_DAILY_VALUE_CAP_WEIfor the coverage job, also worksand
getDefaultDailyValueCapWei()already honours it. It is the weaker option: it needs threefiles rather than one, because
.github/actions/start-app/action.ymlhas no generic envpassthrough and
protocol-nightly.ymlruns the same suite, it does not help local runs throughscripts/protocol-local.sh, and it disables #2031's new default across the whole CI app ratherthan leaving it enforcing for orgs that set no cap, which is the behaviour #2031 shipped.
Per-file organizations are not a cheap alternative: the shared org owns the Turnkey wallet the
tests sign with (
organization_wallets), so each per-file org would need its own provisioned andfunded wallet.
Lowering the frax
ethValuefixtures would also clear it, but it weakens what the suiteexercises, so I would not.
Note this unblocks CI without settling whether 0.02 ETH per day is the right production default.
That is a separate product question for #2031, and it should not be decided by whatever makes a
release green.
Resolved: metrics-db-reviewed
metrics-db-review-gatereported a missing label earlier in this PR's life, over thelib/metrics/changes. It passes on the current head, so there is no action left.No DB prep
git diff origin/prod...origin/stagingtouches no migration SQL, sodb-prep-checkpasses with nodb-prepped-prodlabel and there is no manual DB step.docs-site lands consistent
prod currently has
next16.3.1 in both the manifest and the lockfile, with nopnpm-workspace.yaml. After this it has 16.3.2 in both plus the workspace file. #2129 is the manifest-only bump that caused the drift, #2136 and #2134 carry the matching lockfile.