MCP 2026-07-28 readiness: Waves 0-2 + adversarial review round (16 defects found and fixed) - #550
Draft
Weegy wants to merge 117 commits into
Draft
MCP 2026-07-28 readiness: Waves 0-2 + adversarial review round (16 defects found and fixed)#550Weegy wants to merge 117 commits into
Weegy wants to merge 117 commits into
Conversation
MIGRATION_DOMAINS listed five domains and omitted middleware/migrations, the core runtime domain holding 0001-0030. Every migration there had shipped without ever being applied — or re-applied for the idempotency check — against a real Postgres in CI: the entire MCP schema (0003, 0008, 0009, 0010/0013, 0012/0014, 0015/0016, 0017-0020) and every dev-platform migration (0022-0030). Suspected during #330, now confirmed and closed. No latent schema defect was exposed. All 30 files apply and re-apply cleanly against pgvector/pgvector:pg16, in both possible domain orderings and additionally with rows present. The domain is self-contained: no cross-domain foreign keys, no object names shared with the other five domains, and no extension dependency (gen_random_uuid is core since pg13). The comment now records the three domains that remain uncovered, each of which needs its own audit before being enabled.
No pg test touched MCP before this — only memoryStoreConformance, pluginVerdictStore and skillLifecycleStore existed. Covers the registry seed and catalog-kind backfill (0010 + 0013, including that 0013's UPDATE actually lifts the official registry off the 'generic' column default), the kind/auth_kind/source/registered_via CHECK sets, marketplace provenance with ON DELETE SET NULL detaching an imported server from a deleted catalog, the 0014 partial unique index on top-level MCP grants (and that it leaves native grants alone), and the 0015/0016 OAuth surface — authorize-time endpoint pinning plus token/flow cascade on server delete. Each assertion was mutation-checked against a deliberately broken schema. A second suite covers what the CI gate structurally cannot: the CI idempotency check re-applies against an EMPTY database, so it can never catch a migration that only breaks once rows exist. It re-applies all 30 files with MCP rows in place, in its own throwaway database — re-running 0001/0003 drops and recreates the NOTIFY triggers, which must not happen underneath a concurrently running suite. Both suites skip when no test Postgres is reachable and scope every row to a w04-mcp- tenant prefix. Pools are capped: the runner executes files concurrently and ~16 other pg suites each hold a default-sized (max 10) pool, so an uncapped extra pool here exhausts max_connections and cancels an unrelated suite mid-run (observed on ConductorWebhookSubscriptionStore).
…capture outputSchema Issue #547 (W1-3) — plumbing only, no canvas synthesis. Discovery now keeps a tool's declared outputSchema: McpToolDescriptor and McpDiscoveredTool gained an optional outputSchema, and listTools copies it from tools/list (object-valued only; anything else is dropped). It rides along in the existing mcp_servers.discovered_tools jsonb column, so it survives a restart without re-discovery and needs no migration. subAgentToolHydration rehydrates it on the way back out and seeds the manager's cache, since mcpNativeHandler only closes over a tool name. structuredContent is no longer discarded. A new extractStructured() reads it and McpManager hands it to an optional McpManagerOptions.structuredSink as { kind: 'structured_output', serverId, toolName, turnId, structured, outputSchema? }. Error results and absent/null payloads emit nothing. This is deliberately out-of-band rather than a widened return type. callTool() still returns Promise<string> and NativeToolHandler is untouched, which keeps the published plugin contract stable and keeps every MCP result on the 'typeof result === string' path that gates Privacy Shield masking in the orchestrator — a non-string result would bypass the shield. The payload union is a discriminated 'kind' so #544 (MRTR) can add 'input_required' without another refactor. renderToolResult is byte-for-byte unchanged and is now pinned by a golden suite (text-only, mixed blocks, structuredContent-only, empty content, array-valued structuredContent, isError, whitespace fallback, nullish). A mutation check installs a hostile sink that rewrites and deep-mutates its payload and returns a different object, then asserts the LLM-bound string is unchanged; verified to fail against a deliberately in-band mutant. Operator surface: a read-only 'returns structured output' badge in the MCP Control Center, with en + de strings.
…ains-middleware # Conflicts: # docs/CHANGELOG.md
…red-content-sidecar # Conflicts: # docs/CHANGELOG.md
…le-flight refresh Three live security/correctness defects in the MCP OAuth path. D1 — no RFC 9207 `iss` validation. The OAuth callback trusted the `state` parameter alone. `state` proves a response belongs to a flow we started; it does NOT prove which authorization server issued the code, so a malicious or compromised MCP server could steer the callback and have a code minted by one AS redeemed at another. `iss` is now validated against the issuer bound to the flow BEFORE the code is exchanged, so a rejected callback persists nothing — no token row, no vault write. A mismatched `iss`, or an absent one from an AS that advertised `authorization_response_iss_parameter_supported`, is rejected. That advertisement is captured at authorize time in the new `mcp_oauth_flows.iss_required` column rather than re-discovered at the callback, for the same reason migration 0016 pinned the token endpoint: a server that can flip the flag in between would simply opt itself out of the check. D2 — silent 'operator' fallback (confused deputy). Both the operator router and the runtime McpManager resolved the OAuth user key as `… ?? 'operator'`, so a Teams or Telegram turn whose user had no mapped identity reached the customer's MCP server holding the OPERATOR's token. Resolution now goes through the new `services/mcpDelegation.ts` and the new `mcp_servers.delegation` column: `per_user` yields no token when no identity resolves and the turn fails closed through the existing `onAuthFailure` path with an explanation; `service` is the explicit opt-in to one shared identity. The fallback literal is gone from every call site. D3 — refresh race. `getValidAccessToken` permitted N concurrent refreshes per (server, user). Against an AS with rotating refresh tokens the losers get `invalid_grant` and the last writer can persist an already-retired token, silently disconnecting the user. Concurrent callers now share one in-flight promise keyed by (serverId, userKey), cleared in a `finally` so a failed refresh never poisons later attempts. Also: - `mcp_oauth_tokens.issuer` records which AS minted a token; a rotated issuer drops the stored token instead of replaying it against a different server. - `mcp_call_log.acting_identity` records WHOSE authority each call used (`caller_agent` is the orchestrator slug, not the identity). Resolved via a new optional `McpAuthProvider.resolveIdentity`, threaded through `callTool` before the dispatch guard so denied calls are attributed too. An unattributable call is recorded as `unresolved`, never left blank. - OAuth failure logging goes through `services/secretRedaction.ts`: tokens, `code`, and `code_verifier` can no longer reach a log line, including values a provider echoed back that we never minted. The callback's error page is redacted too. - New `PUT /mcp-servers/:id/delegation` plus a delegation control in McpAuthSection, with `adminMcp.auth.delegation*` keys in en.json and de.json. Tests: 40 in test/mcpOAuth.test.ts covering iss present/absent/mismatched/blank and trailing-slash equivalence, fail-closed resolution, issuer rotation, and redaction. The D3 test is mutation-checked — it asserts exactly ONE token-endpoint HTTP request under 8 concurrent callers (verified to report 8 and fail when the in-flight map is removed), not a count of mock invocations. BEHAVIOUR CHANGE (operator-visible): a fail-closed `per_user` default for every row would break installed deployments whose channel users reach MCP servers today BECAUSE of the 'operator' fallback. Migration 0031 is therefore deliberately asymmetric — every EXISTING `mcp_servers` row that already holds a stored operator token is set to `delegation = 'service'`, preserving today's behaviour, and only NEWLY created servers get the safe `per_user` default. Operators must review grandfathered servers and switch the ones that should be per-user.
…s-delegation # Conflicts: # docs/CHANGELOG.md
One hung sub-agent used to pin the whole Promise.allSettled batch for the rest of the turn: domainQueryTool awaits agent.ask() with no abort and no timeout, and there was no per-tool deadline anywhere in the orchestrator. dispatchTool now races an AbortSignal-backed deadline (default 120s, OMADIA_TOOL_DISPATCH_TIMEOUT_MS, 0 disables) and returns a structured Error: result on timeout. The abandoned dispatch is marked aborted, so a late result is discarded before the first write into turn state (raw-result capture, canvas sentinel, KG ingestion, privacy interning) and late sub-agent events are dropped by an abort-guarded observer.
…rized
callTool passed no RequestOptions and silently inherited the SDK's 60s
default, so the real ceiling was undocumented and un-tunable. It now
passes an explicit { timeout, resetTimeoutOnProgress, maxTotalTimeout }
(env-tunable), where resetTimeoutOnProgress keeps long streaming calls
alive and maxTotalTimeout is the absolute ceiling.
looksTransient() also matched a bare -32001, contradicting its own
contract: the code is implementation-defined and servers legitimately use
it for Unauthorized (omadia's LoopbackMcpServer does), so a genuine auth
failure got one doomed retry before surfacing. Auth now wins; a real SDK
request timeout still retries via its "Request timed out" message.
The mutation check is captureRawToolResult: a real turn-state write the routine runner reads back. Verified by temporarily removing the deadlineSignal guard — the suite then fails on the capture assertion, not on a missing error string. Also covers batch siblings resolving normally, the 0-disables path, and a bad env value falling back to the default.
The re-apply-under-data suite originally built its private copy of the domain in a throwaway database. That isolates correctly, but CREATE/DROP DATABASE is a cluster-wide operation: run inside the full suite with a test Postgres reachable, it stalled the concurrently executing dev-platform pg suites long enough that 29 of their tests were cancelled with "test did not finish before its parent". Reproduced deterministically against appStore.pg.test.ts and devJobStore.pg.test.ts, and confirmed absent from the same run with this file removed. It now runs against a dedicated schema on one pinned connection with `public` off the search_path. The migrations name every object unqualified, so they build a private copy there and never touch — or take ACCESS EXCLUSIVE on — the shared tables. Cancellations: 29 -> 0. The test asserts the isolation itself (table count in the schema), because a leaked search_path would turn the migrations into no-ops against the shared tables and make every later assertion pass vacuously. Both suites now share the file's single capped pool, closed once in a file-level after hook.
…MCP server
W0-3 — sort the dynamic tool segments by name so the Anthropic prompt-cache
tool block is byte-stable across machines and deploys.
`buildToolsList()` stamps `cache_control: {type:'ephemeral'}` on the last tool
spec, which makes the whole tool block a single cacheable chunk. The cache keys
on a byte-exact prefix, but two of the segments feeding that block were iterated
straight out of Maps — plugin load order for the native tool registry,
`created_at` row order for domain tools — with no sort anywhere. Stable within
one process, divergent across Fly machines and across deploys: a silent,
signal-free cache miss for the tool block and everything after it.
- new `toolOrdering.ts`: `compareToolNames` (locale-pinned `localeCompare(b,'en')`
so the result does not depend on the host's LANG/LC_COLLATE), `sortByToolName`,
`sortBySpecName`, `normalizeDiscoveredToolOrder`
- `buildToolsList()`: native + domain segments sorted; the deliberate
fixed-literal prefix (memory, knowledge-graph, ...) keeps its existing order
- `ToolDispatchService.listDispatchableToolSpecs()`: sorted, so the loopback
server and the CLI bridge inherit it
- `LoopbackMcpServer` tools/list: sorted independently, because `deps.tools` is
caller-supplied
- `resolveSubAgentTools()`: sorted (grants arrive in `created_at` order)
- `setMcpDiscoveredTools()`: normalizes by name before persisting, so a server
that returns `tools/list` in a different order each call stops churning the
JSONB column and any grant-epoch diff derived from it
Ordering is advertisement-only. Collision resolution is unchanged — native tools
still win a duplicate name, decided by Map insertion and never by array position
— and that is now pinned by a test whose colliding name deliberately sorts last.
W1-2 — make the loopback MCP server stateless.
`sessionIdGenerator: undefined` is the SDK's stateless mode: no session id is
issued and no session validation is performed, so the CLI bridge needs neither
the `initialize` handshake nor `Mcp-Session-Id`. The previous comment claiming
session ids "remain required by the protocol" was wrong.
SDK 1.29.0 enforces the other half of that contract — a stateless transport
throws "Stateless transport cannot be reused across requests" on its second use.
The MCP server + transport pair is therefore built per request (matching the
SDK's own stateless example) and torn down in a `finally`. Both are in-memory
handler tables with no I/O, and this server sees a handful of requests per CLI
turn. `enableJsonResponse` stays on, which also guarantees the response is fully
written before teardown.
Non-POST is now declined with 405, which the MCP spec explicitly allows for the
optional GET SSE stream. Without it the per-request transport leaks: a GET stream
never ends, so `handleRequest` never resolves and the request scope never tears
down. Under the old stateful transport a session-less GET was rejected with 400,
so nothing regresses.
Tests were written first for W1-2 and observed to fail (HTTP 400) before the
production change. The wire test is parameterized over replaying vs never
sending the session header, plus a case with no `initialize` at all. The 401
`-32001` body and the 413 oversized-POST case still hold.
Deliberately NOT implemented: the `ttlMs` tool-list cache also proposed in #545.
Its premise is false — `subAgentToolHydration` reads
`mcp_servers.discovered_tools` and never calls `listTools`, so steady state is
already ~4 wire calls per server per day — and it would re-advertise removed or
repurposed tools inside exactly the window the #454 scan-verdict gate exists to
close.
MANUAL VERIFICATION REQUIRED BEFORE MERGE: the loopback server's only consumer
is the Claude CLI bridge, spawned with `--strict-mcp-config --mcp-config <path>
--allowedTools mcp__omadia__*`. If the installed CLI refuses to proceed when the
server issues no `mcp-session-id`, the bridge yields a server with ZERO tools and
the turn silently degrades to a toolless answer rather than erroring — no
automated test catches that. A pass against the real `claude` CLI with the
stubbed `createLoopbackServer` bypassed is needed. Not performed here: spawning a
nested `claude` session is blocked in this environment. Installed CLI version on
this machine is 2.1.220.
Nothing in the repo proved the client could complete an initialize → tools/list → tools/call sequence: mcpCallAudit dials a refused port, mcpRescan stubs listTools, and the cliBridge tests stub the server. These drive a live in-process LoopbackMcpServer, sometimes through a recording proxy that injects one transport-level failure, so retry and pool behaviour are observed rather than inferred: - listTools + callTool succeed over the wire; a second call reuses the pool - a successful call is audited as ok (previously uncovered) - a genuine Unauthorized surfaces immediately: exactly one POST, exactly one pool invalidation (fails if -32001 is classified transient again) - the shipped once-retry still fires exactly once for -32000 and then succeeds, dropping the pooled connection once - a stale token invalidates the pool and the next call reconnects
…dline-and-mcp-client-tests
…-28) MCP 2026-07-28 reclassifies the legacy HTTP+SSE transport as Deprecated with a minimum 12-month removal window. Discourage 'sse' for NEW registrations while keeping every existing SSE server fully working — no protocol work, SSEClientTransport stays wired and the DB CHECK is untouched. - DEPRECATED_MCP_TRANSPORTS + isDeprecatedMcpTransport in mcpClient.ts as the single source of truth, re-exported from @omadia/orchestrator. - mcpNode() gains an additive transportDeprecated flag derived from it (no migration); exported for unit tests. - Marketplace import path (the second way an sse row can be minted): prefer an http remote when a catalog entry offers both, still allow an sse-only entry but flag it via McpCatalogEntry.transportDeprecated. - web-ui McpServerNode gains optional transportDeprecated; McpTransport keeps 'sse' (published plugin contract stays as-is). Refs #541
…ggle Issue #541 acceptance 4 + 6. http (Streamable HTTP) stays the default and the only remote option shown; 'sse' appears in the picker only after ticking 'Show deprecated transports', labelled '(deprecated)'. Existing sse rows get a Deprecated badge in the transport column with a hint pointing at Streamable HTTP as the migration target. Nothing is hard-blocked: the MCP removal window is at least 12 months, so an operator can still deliberately register a legacy SSE server. i18n: adminMcp.servers.{transportDeprecated,transportDeprecatedHint, showDeprecatedTransports,deprecatedOption} in both en.json and de.json. Refs #541
…gression - mcpRegistryClient: prefers http when a catalog entry offers both remotes, still imports an sse-only entry (flagged), and the preference cannot bypass the untrusted-remote guard; the pre-existing sse fixture now also asserts transportDeprecated. - mcpNode: transportDeprecated true for sse, false for http/stdio. - Regression: an sse config still yields a real SSEClientTransport (and http a StreamableHTTPClientTransport) — the unit discourages, it does not remove. Refs #541
Issue #541 acceptance 8 + 9. First test file for the MCP Control Center page: the picker offers only http/stdio by default, exposes 'sse (deprecated)' after ticking 'Show deprecated transports' and lets it be selected (never hard-blocked), resets to http when the toggle goes off, badges existing sse rows and falls back to the local list when an older middleware omits the flag. Refs #541
…n' into feat/mcp-2026-07-28-wave0-wave1 # Conflicts: # docs/CHANGELOG.md
…t' into feat/mcp-2026-07-28-wave0-wave1 # Conflicts: # docs/CHANGELOG.md
…-sidecar' into feat/mcp-2026-07-28-wave0-wave1 # Conflicts: # docs/CHANGELOG.md # middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts
…er-and-stateless-loopback' into feat/mcp-2026-07-28-wave0-wave1
…cp-client-tests' into feat/mcp-2026-07-28-wave0-wave1
Lift the shape devJobOrchestratorTool.ts hand-rolled (start returns a handle at once, work runs detached, a card streams into the turn) into a reusable seam so any tool can be marked longRunning. - taskTypes.ts: TaskDescriptor + TaskStore/TaskReadStore, with a status vocabulary (working | input_required | completed | failed) chosen to project mechanically onto MCP Tasks later. - inMemoryTaskStore.ts: reference implementor of the claim/lease and terminal-transition semantics. - longRunningTool.ts: defineLongRunningTool() -> the non-blocking <tool>_start / _status / _list triple + pending card buffer. - taskReaper.ts: orphan sweep (abandoned live tasks, accumulated terminal tasks). - subAgentTaskTool.ts: deferred sub-agent dispatch as second consumer. No MCP Tasks protocol handlers: internal LocalSubAgent dispatches never cross an MCP boundary, and SEP-2663's tasks/update is unshipped.
Client ID Metadata Documents (issue #546) become the third link of an explicit acquisition chain: stored -> cimd -> dcr (deprecated, warns) -> manual. Corrects the issue's premise: the OAuth 2.1 + PKCE stack already shipped in epic #459 W9, so this is a delta on it. CIMD replaces Dynamic Client Registration at MCP-native brokers only -- Entra ID and Okta do not support it and keep using the existing manual path, which has no sunset. DCR is kept working and merely warns. - migration 0032: 'cimd' in the registered_via CHECK set + client_metadata_url - GET /.well-known/omadia-mcp-client, allowlisted via the shared constant - 501 (not 500) when FLOW_PUBLIC_BASE_URL is unset -- CIMD needs INBOUND https reachability, so a firewalled install degrades to manual instead of breaking - SSRF guard reuses assertPublicHttpsUrl on the metadata probe - describeAuth gains acquisitionMode / cimdSupported / cimdBlockedReason
Additive adapter (new file only): projects DevJobStore onto the generic
TaskStore seam - ten-value DevJobStatus down to the four-value
MCP-Tasks-shaped vocabulary, dev_job_events onto the seam's event tail,
claimNextQueued onto claimNextPending, and finalizeDevJob onto finish so
the brand-gated terminal choke point is preserved.
Zero edits to devJobStore.ts / devJobOrchestratorTool.ts and no
migration: dev_job_start still returns {"status":"job_started",...} so
the web-ui card parser and existing tests are untouched. Keeping this in
its own file also keeps the in-flight dev-platform plugin extraction
(PR #536/#538) to a file move rather than a conflict.
Documents the one intentional divergence: the seam's finish() is
lease-fenced, dev_job's finishTerminal deliberately is not (cancel routes
and the reaper finalize jobs they never claimed), so the adapter accepts
a matching lease OR no lease, and rejects a mismatched one.
Extend the lenient CallToolResult schema with resultType + inputRequests,
both readable off the shipped SDK 1.29.0. An input_required result parks
the call in a new PendingMcpInput store keyed on the
{userId, sessionId, correlationId} triple and returns a stable sentinel
to the model instead of a result: no retry attempt consumed, no failure
audit row. The MCP call audit gains a three-valued outcome so a parked
call is neither reported as a failure nor as a delivered result.
Rides W1-3's McpSidecarKind union, which was shaped for exactly this
second member.
…guard, migration 0032 - strategy chain: stored beats cimd beats dcr beats manual, each asserted via an OBSERVABLE consequence (which client_id reaches the provider, what was persisted) rather than a mock call count - CIMD skipped when the AS does not advertise support, when no metadata URL is configured, and when the document is not inbound-reachable - metadata endpoint shape, 501 without FLOW_PUBLIC_BASE_URL, and a direct assertion that served redirect_uris === McpOAuthService.redirectUri - SSRF rejection of loopback / RFC1918 / link-local / non-https metadata URLs, including that no request leaves before the guard runs - publicPaths asserted against the shared CIMD_METADATA_PATH constant - migration 0032 pg test isolated in a dedicated tenant SCHEMA (never a scratch database: CREATE/DROP DATABASE is cluster-wide and cancels concurrent suites)
…cy invariants 31 tests across the reference store and the registration helper. Each invariant was verified by deliberately breaking it, rebuilding, and confirming a real assertion failure: - M1 lease fence removed -> claim+lease suite fails - M2 reaper keyed on heartbeat only -> never-claimed-orphan test fails - M3 task input copied onto a card -> privacy invariant test fails - M4 takePendingCards stops draining -> card test fails - M5 terminal immutability guard removed -> zombie-worker test fails M5 initially SURVIVED: finish() cleared the lease, so the lease check alone carried terminal immutability and the guard was unreachable. Fixed properly rather than by weakening the claim - reapOrphans now PRESERVES claimedBy, which is the correct semantics (the reaper is not the owner, and a zombie worker waking up after being reaped legitimately still holds a matching lease). The guard is now the only thing rejecting it, and the new zombie-worker test fails without it.
… statement A pool-level 'SET search_path' binds only the one pooled client that served it, so the next query lands on a different client and silently resolves against public. Two assertions failed on the first real run because of it. The search_path is now a connection option on a dedicated pool, with a separate admin pool for CREATE/DROP SCHEMA.
…ight files Fifteen literal 0x00 bytes, used as composite map-key separators, written as the two-character escape instead. Behaviour is bit-identical; what changes is that ripgrep no longer classifies these files as binary and stops searching partway through, silently truncating every audit that crosses them. This wave lost real time to exactly that: a correct finding was retracted because a stale byte offset was checked instead of a fresh scan. Proven no-op rather than assumed. A byte-level walker over 2216 files found exactly 15 NULs in 8 files, all .ts, none followed by an ASCII digit -- the only case where the escape would change meaning -- and the 13 changed literals were read back off disk and evaluated to confirm they still yield real U+0000. Test totals are identical before and after (5797/5792/1/4). Note for the record: the NUL guard at postgresMemoryStore.ts:206 has no effective coverage. Disabling it entirely leaves the whole suite green, because its only test skips without postgres and asserts nothing about NUL anyway. The proof for that site is the byte diff, not the green suite.
postgresMemoryStore.normalize() rejects a virtual path containing a NUL byte and then reports 'Path contains a space.' -- almost certainly because the raw 0x00 in the source was invisible to whoever wrote it. An error that misnames the input it rejected sends the reader looking for a bug that is not there. Found while escaping that same NUL (W6-1). The guard also turned out to have no effective coverage: disabling it outright left the entire middleware suite green. Its only test is memoryStoreConformance.pg.test.ts, which skips without a postgres -- and CI has no postgres service on the middleware job, so it never runs there at all -- and it asserts nothing about NUL regardless. The new tests need no postgres by construction: normalize() runs before any query, so a pool that THROWS when queried is itself the assertion. If validation ever moved after the first query, the fake fires and the test fails with the wrong error. A negative control pins that a clean path survives validation and reaches the pool, so a normalize() that rejected everything could not pass. Mutation-checked with a rebuild, because the test imports through the package barrel and a src-only mutation would have been invisible: removing the guard fails 3 of 4, negative control still green.
…L suites vitest defaults testTimeout to 5000ms and web-ui never overrode it, but the heavier React Testing Library suites -- template proposals, slot pickers, the publish flow -- measure 5-13s unloaded. A ceiling below a test's real cost does not catch hangs, it manufactures them. Demonstrated rather than assumed: four runs of an unchanged tree gave 0, 9, 25 and 0 failures purely as machine load varied, and every failure was a timeout at the 5000ms mark rather than an assertion. The 25-failure run happened while the middleware suite was running beside it. 30s, and the same for hooks. Deliberately generous, matching the middleware --test-timeout added in this wave: the job of the number is to stop a hung test from burning the CI wall with no attribution, not to police tests that are slow but honest. Verified against the condition that produced the failures: 427/427 pass while the full middleware suite runs concurrently. This corrects an earlier finding in the same wave that read web-ui as already adequately bounded. It was bounded, but in the wrong direction.
…derstated The ratchet reported middleware/test rising 1036 -> 1043 in this wave. None of it is new coupling: no test file added here contains a single dev-platform token, and no changed test file's count moves when measured with rg --text. The cause is the metric itself. scripts/check-core-decoupling.mjs invokes rg without --text and traverses directories, which is exactly the mode where ripgrep applies binary detection -- and two files in this zone carried raw NUL bytes, so rg classified them as binary and stopped reading partway through. Seven dev-platform references sat past that cut-off and were never counted. Proven by reversal: put the raw NULs back and the zone reports 1036; escape them and it reports 1043. Nothing about the code changed between those two measurements. So this is not a baseline being raised to accommodate new debt -- it is a baseline being corrected upward to the number that was always true. The decoupling ratchet has been under-counting since those NULs were introduced, which also means it would not have noticed new coupling added past a NUL in either file.
1. routineRunner's two branches inherited opposite identities. The templated branch opens a fresh turn scope; the untemplated one opened none, so orchestrator.runTurn read whatever ambient scope was open as its parent and inherited its mcpUserKey. A routine fired from inside a chat turn would then run under the INVOKING user's MCP identity rather than its owner's -- and the same routine would behave differently depending only on whether an output template happens to be configured. Both branches now open the scope. 2. 'server-attested end to end' overclaimed. The dispatcher copies userRef.id verbatim and verifies nothing itself, so the guarantee is exactly as strong as the inbound-webhook authentication in the Teams/Telegram/Slack adapters -- which live outside this repo and cannot be checked from here. The comment now says adapter-attested, and records the bound: resolveOrCreateChannelIdentity creates on miss, so a forged id matching no known identity mints a fresh uuid holding no token and fails closed. Impersonation needs an already-known channel user id. 3. A lone ?? in an otherwise truthiness-based chain. Every other link guards on truthiness, so a parent carrying an empty string would short-circuit the ??, suppress the valid key the channel branch would have produced, and then be dropped by the truthy spread -- silently downgrading a resolvable turn to unresolved. Unreachable today; defect-in-waiting. The review's central question -- can a client-controlled value reach mcpUserKey -- came back no across all nine enumerated callers of runTurn and chatStream, including the server-to-server API-key channel from #438/#439, which sets kind:'custom', gets no channelIdentity, and fails closed.
…ay bypass An earlier entry in this wave claimed the #547 structured-content sidecar leaks PII to the browser. Cross-vendor review refuted it: Privacy Shield v4's boundary is server<->LLM, not server<->browser. internToolResultV4 returns a digest for the tool_result block while real rows stay server-side behind a datasetId, and the browser receives real values by design -- PrivacyRenderedAnswer.text carries them and the UI highlights maskedValues so the user can see what the server resolved behind the boundary. Getting that wrong blocked a correct feature and proposed a plugin-api contract change that is not needed for the stated reason. Attacking the premise is what surfaced the real defect, which is on the actual boundary and live: the MCP input replay calls callTool directly, skipping dispatchTool and therefore interning, and interpolates the raw result into the note that goes to the model. #547's renderer stays deferred, now for honest reasons: it is a full-stack change on an already-large PR, and the sidecar bypasses receipt and dataset accounting even where masking is not owed.
Cross-vendor review of the W5-1 admin surface. `public_mcp_key_bindings` rows are the entire authorization model for an internet-facing, API-key-authenticated endpoint, so three of these are grant-widening bugs rather than hygiene. FINDING 1 (HIGH) — revoke was silently undone by any later save. `validateBindingInput` defaulted `enabled: input.enabled ?? true`, and the upsert wrote that manufactured `true` over the stored row. An operator revoked a key after an incident; the next save from a stale tab, a second operator, a config replay, or the admin UI's own form (which does not round-trip `enabled`) handed the third-party key its whole allowlist back, silently, answering 201 Created. Fixed by preserving on conflict: an omitted `enabled` now stays absent through validation and resolves in the store — against the stored column on conflict, against `true` only for a row that does not exist yet. Re-arming requires saying so. Because that makes revoke sticky, un-parking gets its own explicit route (`POST /:keyId/restore`) and a confirmed UI affordance, so the fix does not strand an operator with no way back except psql. POST / now answers 201 only for a row it actually created. "Created" over an existing binding is the operator's only per-request signal that they landed on somebody else's row. FINDING 2 (MEDIUM) — `Number(null)` is 0, and 0 is a valid write budget. The `=== undefined` guard let JSON `null` through, so a client sending `null` to mean "use the default" got a key that authenticates, resolves its binding, and is throttled to nothing on every write while the UI shows write tools listed. `[]`, `false`, `""` coerced identically; `true` became 1. Both optional fields are now type-checked and a bad value is a 400. The same silence on `enabled` (present-but-non-boolean was dropped, which under the old default meant "activate") is rejected too. FINDING 3 (LOW) — `String(err)` put pg table, column, constraint and sometimes connection detail into 500 bodies that land in browser devtools and UI logs. Logged server-side, generic on the wire.
…aviour Revoke becoming sticky, 201 narrowing to created-only, and the two fields switching from coercion to type-checking all change what an operator and any config-replay client observe. The Added section for this surface said "lists, creates and revokes", which is no longer the whole story.
…in surface The load-bearing one: a revoked binding was silently re-armed by any upsert that omitted 'enabled'. Revoke is the incident-response lever on an internet-facing surface, and it was undone by an operation that never mentions revocation -- answering 201 Created for what overwrote a deliberately parked row. Fixed at the store, not the router. The real bug was validateBindingInput doing 'enabled: input.enabled ?? true', turning ABSENCE into ASSERTION before the store ever saw the difference. The field now stays absent through validation and is resolved only where the current row is known, via COALESCE against the table-qualified column -- not EXCLUDED, which holds the row this statement proposed and would resolve straight back to the insert's true. That trap has its own test and its own mutation. Making revoke sticky removed the only way to un-park from the UI, so this also adds POST /:keyId/restore and a confirmed Restore button: un-parking is now an explicit act in an access log rather than a side effect of pressing Save. Also: type-check instead of coerce (JSON null was becoming a zero write budget, and a non-boolean 'enabled' was silently dropped -- which under the old default meant activate), generic 500 bodies instead of raw driver text, 201 only for rows actually created, and a parameter-aware recordingPool. That last one closes a demonstrated hole: swapping setEnabled's placeholders while still passing [keyId, enabled] left the whole suite green and broke revoke in production.
…LM wire Privacy Shield v4's boundary is server <-> LLM provider: `dispatchTool` interns every raw tool result via `internToolResultV4` and hands the model only an identity-free digest, while the real rows stay server-side behind a datasetId. The browser is on the TRUSTED side and legitimately receives real values. The MCP input-replay path (MRTR, #544/W2-1) bypassed that boundary. The replayer registered in `middleware/src/index.ts` calls `mcpManager.callTool` DIRECTLY rather than through `dispatchTool`, so nothing interned its result; `runMcpInputReplay` then interpolated that string verbatim into the note, and `withMcpInputNote` folded the note into the turn's ingested text bound for the model. A user answering an MCP credential card against an HR or accounting server therefore put the returned personnel row on the LLM wire in cleartext — where the same row from the same tool on an ordinary turn would have been a digest. The existing comment above the note proves this was a near-miss: it notes the text "goes on the LLM wire" and then justifies omitting only the user's typed values. The tool result two lines below was overlooked. Fix shape (a): intern inside `runMcpInputReplay`, where the turn's privacy handle is in scope, immediately before the note is built. Shape (b) — route the replay through `dispatchTool` — was investigated and rejected on four counts, recorded in the new method's doc comment: * `dispatchTool` keys on the hydrated, namespaced `mcpNativeToolName(...)`, while the parked record carries the RAW MCP tool name; no reverse mapping exists and one would break the moment a server is renamed between turns. * the replayer must re-resolve the server's LIVE config (endpoint, headers, Vault-resolved env), not a hydration-time closure — the reason it is registered in `index.ts` in the first place. * a replay must still complete a call the previous turn already made, even when the tool is no longer granted or hydrated for the current agent. * it must not re-enter dispatch-only deadline, audit and MRTR park semantics; a replay that re-parks is what MCP_INPUT_MAX_REPLAY_DEPTH exists to prevent. `guardReplayResult` mirrors the ordinary path exactly: no privacy handle means byte-identical legacy behaviour; an operator-flagged privacy-bypass server (`isMcpServerPrivacyBypassed`, still clamped by OMADIA_PRIVACY_FORCE_GUARDED through `resolveEffectivePrivacyMode`) passes raw and records a receipt entry; everything else is interned. Fail-open on a throwing guard is deliberate parity with `dispatchTool`, documented as such. `mcpDomainForServer` is extracted in mcpClient.ts so the bypass receipt entry carries the SAME plugin id an ordinary dispatch would, from one derivation. Regression test drives a real Orchestrator turn against a real MCP server over a real socket and asserts on the text that crossed the provider boundary, never on a call count. Verified by mutation: reverting the note to the raw result turns it red with the personnel row printed verbatim in the failure.
…overage
Two test names in `mcpStructuredOutputPrivacy.test.ts` encoded a conclusion the
code does not support, because the author misread which side of the Privacy
Shield v4 boundary the browser sits on.
* `BASELINE — the TEXT result a client receives IS masked at the dispatch
seam` — wrong as written. The interned digest is what the MODEL receives.
The client legitimately gets real values via `takeRenderedAnswerV4`
(`plugin-api/src/privacyReceipt.ts:164-174`), rendered with
`highlightTerms={message.maskedValues}`.
* `LEAK — the STRUCTURED sidecar carries the same PII in CLEAR on the same
call` — the assertion is true, the word LEAK is not. The boundary is
server <-> LLM provider, not server <-> browser, and the browser is on the
trusted side.
Both now describe the mechanism: the dispatched text IS interned, the sidecar
is NOT. The header gains an explicit "which boundary this is about" section and
records the earlier misreading, so the next reader does not re-derive it.
The sidecar bypassing interning remains a real and documented property worth
pinning — it makes the payload unsafe to forward across the model boundary and
unsafe for any consumer to assume masked. Naming and framing only: all 20
assertions are unchanged, none weakened or removed.
The MCP input replay called mcpManager.callTool directly rather than going through dispatchTool, so its result was never interned -- and was then interpolated verbatim into the note folded into the turn's ingested text. A replayed HR or accounting tool returning a personnel row sent that row to the model in cleartext, where the identical tool on an ordinary turn would have yielded only a digest. Live in any deployment with a graph pool. Interned in orchestrator.ts rather than routed through dispatchTool, on evidence: dispatchTool keys on the hydrated mcpNativeToolName while the parked record carries the raw name and no reverse mapping exists; the replayer must re-resolve live server config; a replay must complete a call the PREVIOUS turn made even if the tool is no longer granted; and it must not re-enter dispatch park semantics, which is what MCP_INPUT_MAX_REPLAY_DEPTH exists to prevent. guardReplayResult mirrors the ordinary path exactly -- no handle means byte-identical legacy behaviour, a bypassed server gets raw plus a receipt entry, everything else is interned. The mutation check prints the personnel row on the wire verbatim, so the defect is reproduced rather than merely asserted. Also corrects two test names that encoded a refuted verdict. Privacy Shield's boundary is server<->LLM, not server<->browser: the browser is the trusted side and receives real values by design. All 20 assertions are unchanged; only the naming and the header framing moved.
This was referenced Jul 31, 2026
… narrow Three gaps in a file I added earlier in this wave, all found by review: The negative control used a single tidy path, so it could not catch the mistake standing closest to the bug this file exists for. That bug was a NUL branch reporting 'Path contains a space.'; someone reading that message in a stale checkout fixes it the other way round and adds a real space rejection. With one space-free control path, every other test stays green while every memory file whose name contains a space breaks at runtime. The control now covers a space, a non-traversal dot, and non-ASCII. 'Applies the guard on every entry point' covered four of eight, omitting directoryExists, createFile, and -- worst -- delete and rename, the two destructive ones. All eight now. rename's destination was entirely unpinned: a refactor keeping normalize(from) and dropping normalize(to) passed the whole suite. pg parameterisation would still reject a NUL in a bind parameter, so this is defence in depth rather than an injection hole, but the guard should not lean on the driver. Mutation-checked with a rebuild, since the import goes through the package barrel: adding a space rejection fails the control, dropping normalize(to) fails the rename test, and a git-status guard confirmed the source returned to HEAD after each.
…n is introduced The backfill grandfathering operator-token servers to `delegation = 'service'` had no first-apply condition. Changing `delegation` does not delete the operator token row, so the EXISTS predicate still matched on every subsequent application: an operator who opted an existing server into `per_user` — the exact action 0031's own header instructs them to take — had it silently flipped back to `service`, handing unmapped channel users operator authority again and undoing the D2 confused-deputy fix. No log, no error. Re-application is the expected mode: migrations/README.md documents that there is no runner and no applied-migrations bookkeeping yet. The backfill was idempotent in the SQL sense but not with respect to operator intent. The ADD COLUMN and the backfill now sit inside one DO block gated on a pg_attribute lookup for mcp_servers.delegation, so both run exactly once, when the column is first added. ADD COLUMN is plain rather than IF NOT EXISTS: the gate already proved absence, and masking a broken gate would turn a logic error into silent drift. The CHECK-constraint guard stays a separate top-level block — it carries no operator intent and must still repair a database where the column exists but the constraint does not. Neither existing gate could see this: CI re-applies against an empty database, and the pg suite re-applied against rows it never flipped. Tests: add the re-apply regression (flip a grandfathered server to per_user, re-apply, assert it stays) and a constraint-independence test that drops the CHECK and proves re-apply restores it. Both are restore-safe via try/finally so they cannot perturb sibling tests. Also from the same review: - the schema-relative guard only matched `public.` inside a quoted literal, so `ALTER TABLE public.mcp_servers` slipped through; now a regex over the comment-stripped text, via a shared helper - the CHECK-constrains test asserted bare SQLSTATE 23514, which mcp_servers_transport_check also satisfies; now asserts the constraint name - the constraint-existence lookup gains the conrelid filter its title claims - header prose named assertSchemaRelative(), which does not exist
The backfill had no first-apply condition, so every re-application re-asserted delegation='service' over any server an operator had deliberately hardened to per_user -- the D2 confused-deputy fix undoing itself, silently, with an unmapped channel user regaining operator authority at the customer's MCP server. Re-application is the expected mode: middleware/migrations has no runner and no bookkeeping table yet, and its own README asks for idempotent statements. Neither gate could see it. CI re-applies against an empty database, and the backfill suite re-applied against rows it had never flipped. The ADD COLUMN and the backfill now share one DO block gated on pg_attribute: if delegation is absent, add it and grandfather the operator-token servers; if present, do nothing. ADD COLUMN is plain rather than IF NOT EXISTS -- the gate already proved absence, so IF NOT EXISTS would turn a broken gate into silent drift instead of a loud failure. The CHECK block stays top-level and unconditional so it still repairs a table that has the column but not the constraint. The ::regclass justification changed and the comment was rewritten rather than left standing: the old argument leaned on an ADD COLUMN that is now conditional. The gate block evaluates 'mcp_servers'::regclass unconditionally and runs first, so reaching the constraint block still proves the relation resolved. Also from the same review: the schema-relative guard now matches any qualified reference rather than one spelling in a string literal (over comment-stripped text, since the migration's own prose says 'public.'), the CHECK test asserts err.constraint rather than a bare SQLSTATE that any check violation satisfies, the constraint lookup filters on conrelid, and the header stops naming a function that does not exist.
This was referenced Jul 31, 2026
Open
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
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.
MCP 2026-07-28 readiness — Waves 0, 1, 2 (+ Wave 3 investigated and deliberately excluded)
Consolidated PR for the MCP issue cluster #540–#547. Twelve independently built and independently verified units, merged in collision order. No SDK change — everything runs on the installed
@modelcontextprotocol/sdk@1.29.0; see the Wave 3 section for why that is now a deliberate, evidence-backed decision rather than a deferral.133 files, +24652/−2664, 83 commits. Migrations
0031,0032,0033.An adversarial codex review (GPT-5.x, high effort) was run over the finished branch and found four security defects and twelve correctness defects. All are fixed in this PR — see "Review round" below. Two of them change what this PR means, so they are stated up front rather than buried.
Caution
This PR silently switched the Privacy Shield v4 data plane ON for streaming turns — it was previously inert.
dispatchToolDeadlinedreadsturnContext.current()?.privacyHandle. Because the streamingturnContextstore was empty (theenterWith-in-an-async-generator defect fixed here), that handle was alwaysundefinedonchatStream. Consequence: nov4_*tools were served, no raw-result capture, no canvas tap, no sub-agent dataset bridge — and no masking or interning of tool results at all. An operator running aprivacy.redact@1provider has been sending unmasked tool results to the LLM on every streaming turn.The fix closes a real pre-existing hole. It also changes observable behaviour for anyone tuned against the old state, and MCP audit attribution flips from
unattributed/nullto real values at the deploy boundary, so persisted telemetry changes shape. This was found by the review, not by the implementing unit — it is the "fifth consumer" that the four accounted-for behaviour changes missed.Caution
Correction to an earlier claim in this PR: masking did NOT fail closed on the public endpoint's error path.
Both dispatch branches returned
this.errMsg(error)directly, skippingafterDispatchand therefore skipping masking entirely; the endpoint'smaskingFailed()check could not catch it, because masking never ran and so never "failed". A tool raisingFault: Invalid field 'x' on record {'id':42,'name':'Jane Doe','email':'jane@acme.de','vat':'DE811234567'}returned that string verbatim to an external caller. Fixed below. The "fails closed" claim was true for the intern path only.Why the issues' dependency graph does not apply
Seven of the eight issues declare "depends on #540". Verified against the shipped packages, six are nominal:
sessionIdGenerator: undefined,experimental/tasks/,outputSchema/structuredContent, and evenresultType/inputRequests(via a passthroughResultSchema) are all available in the installed 1.29.0. Several issue bodies also carry premises that do not match the code — corrections are noted per unit and should be folded back into the issues.Wave 0 — live defects found during analysis (in no issue)
W0-1 — MCP OAuth: RFC 9207
issvalidation, explicit delegation, single-flight refresh. The callback trustedstatealone;issis now validated beforeexchangeCode, with reject tests asserting the code was never exchanged, zero token rows, zero vault writes. The silent'operator'userKey fallback was a confused deputy — a channel turn with no mapped identity acted with the operator's authority at the customer's MCP server; all three call sites now fail closed, andonAuthFailurefails closed before starting a flow (starting one would bind a token to whoever clicks through).getValidAccessTokensingle-flights per(serverId, userKey)so a lost race cannot revoke a rotating refresh token.Warning
Operator-visible. Migration
0031is deliberately asymmetric: every existingmcp_serversrow holding a stored operator token is set todelegation = 'service', preserving today's behaviour; only new servers default to the safeper_user. A fail-closed default for every row would break installed deployments whose channel users reach MCP servers today because of the fallback. Operators must review each grandfathered server in the MCP Control Center.W0-2 — per-tool dispatch deadline. There was no per-tool timeout anywhere, and because the batch is
Promise.allSettled, one slow sub-agent blocked the whole parallel batch for the entire turn. Every dispatch now races anAbortSignaldeadline with a late-result firewall.callToolpasses explicitRequestOptions.-32001was classified transient, contradictinglooksTransient()'s own doc comment, so a genuine Unauthorized was retried — fixed; the once-retry mitigation for the flaky hosted proxy is untouched.W0-3 — deterministic tool ordering. Prompt caching is live, but the tool block was assembled in plugin-load and
created_atorder with no sort — stable within one process, divergent across Fly machines and deploys, silently dropping the cache for the whole block. Dynamic segments are now name-sorted; collision precedence is asserted by name, not position.cache_controlturned out not to be observable where expected (llmProviderSeamcollapses it into a request-levelcacheHints.toolsboolean), so tests assert that plus a deterministic last element and golden-snapshotLlmRequest.tools. #545 rescoped: its caching half rests on a false premise — hydration readsdiscovered_toolsJSONB and never callslistTools, so steady state is ~4 wire calls per server per day, and a TTL cache would re-advertise repurposed tools inside exactly the window #454's scan-verdict gate exists to close. Not built, by decision.W0-4 — CI actually applies
middleware/migrations.MIGRATION_DOMAINSomitted the domain holding0001–0030— the entire MCP schema plus every dev-platform migration had shipped without ever being applied or idempotency-checked in CI. No latent defect was exposed, and that is verified, not assumed: all 30 apply and re-apply cleanly in both domain orderings and with rows present, and a deliberately non-idempotentCREATE TABLEwas injected as a negative control and was caught on pass 2. AddsmcpRegistrySchema.pg.test.ts, the first pg coverage for MCP at all, isolated in a dedicated schema — the first version usedCREATE/DROP DATABASE, which is cluster-wide and deterministically cancelled 29 tests in concurrent suites.W0-5 — first real
McpManagerround-trip test. The repo had zero tests of a successful client→server round-trip: one stub dialled a refused port, another stubbedlistTools, a third stubbed the whole loopback. The single real-protocol test exercised the server, not the client.Wave 1 — stateless-ready, no SDK change
W1-1 — deprecate the legacy HTTP+SSE transport. Canonical marker, additive
transportDeprecatedon the serializer (no migration, DB CHECK untouched), operator picker gatesssebehind a toggle with a badge on existing rows. Never hard-blocked.'sse'stays in every union including the publishedplugin-apicontract — narrowing it would be a semver break. The issue missed a second registration path: the marketplace importer mapped catalogremote.typecontainingssestraight through, so a UI-only change would not have achieved the stated goal.pickRemoteCandidate()now scans allremotes[]and prefers a non-deprecated one.W1-2 — genuinely stateless loopback server. #540's one-line framing is wrong:
sessionIdGenerator: undefinedis supported, but the SDK throws'Stateless transport cannot be reused across requests'on second use — a bare flag flip makes only the first request work and 500s everything after.Server+ transport are now built per request with teardown in afinally. Non-POST returns 405, because the per-request transport leaks on GET (an SSE stream never ends, sohandleRequestnever resolves).W1-3 — preserve
structuredContent, captureoutputSchema. Both are preserved via an out-of-band sidecar sink, deliberately not a widened return type:NativeToolHandler: Promise<string>is a published contract every tool implements including private plugins, and the privacy branch guards ontypeof result === 'string', so widening would bypass Privacy Shield entirely. The sink's union was pre-shaped for'input_required'— which is exactly how W2-1 docked onto it. #547's canvas framing is stale:omadia-ui-orchestrator(3075 LOC) andcanvas-core(2041 LOC) already do deterministic payload→column binding; MCP tools simply could not feed it.Wave 2 — capabilities
W2-1 — MRTR
resultType: "input_required". Client-side, riding the existingask_user_choicedrain as a two-turn flow. Turn suspension was ruled out: there is no per-turn suspend/resume store, and parking mid-tool-loop would blow through every proxy and Teams idle timeout. The store is keyed{userId, sessionId, correlationId}—sessionScopealone is a known cross-user hole (#445). The card carries mandatory server attribution, because a server can now request arbitrary free text mid-call and a hostile one could otherwise phish credentials through a card that looks like omadia's own UI.W2-2 — generalize long-running tools (#543, rescoped). MCP Tasks does nothing for the stated motivation — internal sub-agent dispatches never cross an MCP boundary — and SEP-2663 is absent even from SDK v2. So the goal was taken as "sub-agents stop blocking turns": a generic lease-fenced
TaskStoreseam plusdefineLongRunningTool(), withdev_jobas first implementor unchanged and a genuine second consumer (deferred sub-agent dispatch, tested against a realLocalSubAgent). A chat turn is never parked — always handle plus streaming card. Deferred-result privacy is solved for the data path: a result reaches the model only via<tool>_status, an ordinary tool call inside a live turn, so interning moves from completion time to poll time.W2-3 — public stateless MCP endpoint (#542). Merged dark: there is no UI or API to create key→agent bindings yet, so an operator would have to insert
public_mcp_key_bindingsrows by hand. Safe to merge, not yet operable — the admin surface is the obvious follow-up.Scopes:
mcp:list/mcp:invoke/mcp:write:<tool>. The wildcard exclusion lives insidehasScoperather than in a parallel matcher, because a parallel matcher gets forgotten and still returnstruefor*. Baremcp:writeis rejected at mint time. Isolation comes from the binding column plus per-row filtering; no row ⇒ zero tools. Masking fails CLOSED here (internToolResultV4never throws, so the dispatcher's fail-open branch is unreachable) — a deliberate divergence from the chat path, which fails open. Intern-exempt tools are permanently unservable, since the exemption is checked before the handle.W2-4 — CIMD as a third client-acquisition mode (#546). The issue's premise is false — the registry does not support only static headers; a full OAuth 2.1 + PKCE stack shipped in #459 W9. Chain:
stored → cimd → dcr (deprecated, warns) → manual. Entra and Okta do not support CIMD; they use pre-registered app registrations, for which the correct path is the existing manual client. Both paths are therefore permanent and first-class. The metadata URL derives fromFLOW_PUBLIC_BASE_URLalone, deliberately not the?? PUBLIC_BASE_URLfallback the redirect URI uses — that defaults tohttp://localhost:3979, precisely the shape that is not inbound-reachable. Unset → 501 naming the knob and the manual alternative; CIMD requires the IdP to fetch omadia, which is impossible behind a corporate firewall.Cross-cutting fixes (found while building the above)
W3-B — dispatch privacy seam + idempotency. The SEAM comment misattributed its own subject: masking lives in
dispatchToolDeadlined, notdispatchToolInner(84 lines of pure branch selection with zero privacy code). Routing through the orchestrator would therefore not have masked anything, andcliSubAgentbuilds aToolDispatchServicewith an intentionally empty registry where an Orchestrator dependency is unsatisfiable — and would have widened that sub-agent's tool surface. So the privacy-relevant subset was replicated in the orchestrator's exact order, with an explicitprivacydependency plus ambient fallback, and an optional ALS-propagated caller context.WriteCapabilityexisted but had never been wired; it is now the write source of truth, not name-derived. Two real defects fixed en route: a cache key that collided (("a:b","t")and("b","t:a")mapped to one entry, letting one caller replay another tool's stored write result), andwriteCapabilitiesbeing unreachable from the plugin-facing accessor — without which every real Odoo/M365 write would have stayed unprotected while unit tests passed.W3-A — four pre-existing defects.
turnContextwas empty inside tool handlers on the streaming path. Sharpened diagnosis:enterWithin an async generator survives until the generator's first suspension, so readers before the first yielded event worked by accident — everything at or after tool dispatch got an empty store. Fixed withturnContext.runGenerator. Four behaviours now activate on every streaming turn: skill-bound MCP tools were previously refused unconditionally; chat-launched dev jobs failed closed on missinguserId; MCP→KG ingestion now attributesaclOwners; and — note this one — plugin memory writes landed under thedefaultAgent namespace instead of the acting one, so there will be a seam between older and newer entries. Gap reported, not closed: no production code setsmcpUserKeyon a chat turn, so aper_userserver there still auditsunresolved. That is a product decision.embeddingGateWriteFence.pg.test.ts— a test bug, not a product bug. An unqualifiedDROP TABLE IF EXISTSin the only suite withpublicon its search_path fell through topublic.*; two of those tables have no dependents, so the same fall-through was silently dropping sibling suites' tables. It only surfaced because a third table had a foreign key.DevJobStore.findStalledgained a scope predicate, restoring a pg sweep test that had been downgraded to a unit test.Wave 3 — the v2 SDK port: investigated, and deliberately NOT included
The port was attempted last, after everything above was green. It was stopped before a line was written, on evidence.
LATEST_PROTOCOL_VERSIONis still2025-11-25andSUPPORTED_PROTOCOL_VERSIONScontains no 2026-07-28. It is a separate "modern era" negotiated viaserver/discover+versionNegotiation, deliberately kept out of theinitializelist. A real round-trip negotiatedera=legacy— protocol-identical to v1. The port alone buys zero protocol capability.resultTypefrom every decodedtools/callresult, while other unknown keys pass through. The strip happens in the codec's inbound decode, before any caller-supplied schema, so a lenient schema cannot recover it.isInputRequiredResult()would return false for every result — MRTR (W2-1) would silently stop parking calls, audit them asok, and hand the model an empty result. No repo test would catch it: after a port the MRTR tests use a v2Serverthat rejects ourinputRequestsshape server-side for an unrelated reason, masking the real failure.McpError/ErrorCodedo not exist in v2;setRequestHandlertakes a method string, not a Zod schema;callToollost its schema parameter;PerRequestHTTPServerTransportreturns a WebResponsewhile both servers are Nodehttp, so it would mean more hand-rolling, not less; and there are 8 importers, not 2.Recommendation: split #540 into a spike for the
resultTypestrip (likely needs an upstream question — it gates MRTR) and a separate PR for the mechanical port. Never fold it into this PR: it would swap the foundation under twelve green units for zero protocol gain until modern-era negotiation is also adopted.Review round — adversarial codex pass over the finished branch
Two
codex execreviews (gpt-5.x,reasoning_effort=high, read-only sandbox) were run against the completed branch, deliberately prompted to falsify the implementers' own claims rather than to summarise the diff. Both initially timed out on the 300 s cap; re-running them one narrow target at a time, with source inlined so codex verified instead of exploring, is what produced results.Security findings, all fixed:
maskErrorText, deliberately not by reusingafterDispatch: raw capture is documented as receiving "the tool result" and its consumers treat it as business data, so feeding a driver stack trace in writes query fragments into row-shaped sinks; and the operator bypass is consent about a tool's declared output shape, which arbitrary exception text is not. Only the intern exemption andinternToolResultV4transfer. A neworigin: 'tool' | 'dispatcher'field lets the assertion distinguish handler-authored content from the dispatcher's own refusal strings — absent ⇒ treated as'tool', i.e. fails closed.publicMcpPrivacy.masked()existed, documented as "lets the endpoint assert that the boundary was actually crossed rather than skipped", and was never called anywhere. It is now a required assertion on the public response path — this is the structural fix that makes chore(deps,ci): Bump docker/build-push-action from 6 to 7 #1 unrepeatable.0031's backfill was broader than its own header. It tested for anymcp_oauth_tokensrow rather than an operator token, so a server holding only e.g.user_key='alice@corp.com'was flipped todelegation='service'. Immediate effect is fail-closed breakage, but it silently converts a per-user server to shared identity with no operator decision. Predicate narrowed;0031had never been applied anywhere, so it was corrected in place.tools/listandtools/callused different predicates, so a tool named in a binding but not advertised by the agent was hidden from list yet accepted by call, and the differing error shapes were a weak oracle for binding contents. One predicate now, uniform rejection message.Correctness findings, all fixed: crossed claims stranding two same-kind tasks for 15 min under dead leases; a successful result discarded when the lease had been reaped; the orphan reaper force-failing tasks legitimately parked on a human; the once-retry making the effective MCP ceiling 2 × 180 s = 360 s and thereby re-creating the very timeout inversion this PR set out to remove;
assertOuterIsLooser()existing only in a test file soOMADIA_TOOL_DISPATCH_TIMEOUT_MS=90000re-created the inversion with green CI (now enforced at boot); teardown errors replacing the original exit reason; a reapersetIntervalthat did not await its previous sweep (36 concurrent sweeps observed without the guard); in-flight idempotency entries that never expired; and three comments asserting invariants the code did not hold.Claims that survived the review, listed because a review that only reports failures is not evidence of anything:
*genuinely cannot grantmcp:write:<tool>; a missing/malformed/NULL binding row yields zero tools with no permissive default;issis validated beforeexchangeCodeand a mismatch persists nothing; no token,codeorcode_verifierreaches a log line; the idempotency cache key is provably injective;replayDepthis capped; the terminal guard does stop a zombie worker;looksTransientcorrectly excludes auth errors.One SUSPECTED finding was refuted rather than patched:
pendingMcpInput.claim()is trust-on-first-claim, but the record carries nouserId/sessionIdto compare against — binding at claim time is how it acquires them, the correlationId is a random UUID learned only from the parking call's own result inside the same turn batch, and replay is enforced bytake()on the full triple.Known-limitation honesty: a parked task cannot currently be resumed through the seam at all (
requireInputreleases the lease,claimNextPendingonly picksworking), so finding 3's reaper bug was latent rather than live — nothing callsrequireInputin production yet. Worth its own issue. Likewise the sub-agent dataset-registration leak after a dispatch abort is bounded by turn lifetime and needs a new published@omadia/plugin-apimethod to unwind properly; the comment now states that precisely instead of overclaiming.Verification
build/lint/typecheckeslint-disable, present onmain)testlint/typechecktesti18n:checkThe final run is fully green on an idle machine (load ~17). Earlier runs at load ~214 produced a failure and once an outright kill —
npm testforks a Node process per test file, so results under heavy load are noise. That retroactively explains most of the "disjoint failure sets" several units reported: it is largely a load artefact, not a test-design problem. Note the total differs between runs (5740 here vs 5884/5920 in unit reports) because those runs had a Postgres container up and these pg suites are skipped without one — not a silent skip.Run with a real
npm ci, not a symlinkednode_modules: symlinking makes@omadia/orchestratorresolve against the main checkout, so orchestrator edits are silently not loaded and tests pass without exercising them.~120 mutation checks across the twelve units and the review round, every one verified by deliberately breaking the invariant, rebuilding and re-running until a real assertion failed — not by counting mock invocations. Several survived first attempt and were treated as findings rather than explained away: the buffered
runTurnpath had no coverage at all;chat/page.tsx's strip logic was untested; and a terminal-immutability guard was unreachable becausefinishcleared the lease — fixed by correcting the semantics so the guard became load-bearing.Caution
A mutation harness committed sabotaged production code and it was caught by audit, not by tests. A
git add -Araced a mutation run and shippedtoolTimeoutMsasreturn 2_147_483_647— the per-tool timeout disabled. Found by auditing every mutation target againstHEADrather than trusting the harness's own revert, fixed, and guards added (refuse dirty tree, non-zero exit on leftovers). The merged tree was independently re-checked: no such value and no mutation markers remain in production code.Important
MANUAL VERIFICATION REQUIRED BEFORE MERGE. The stateless loopback (W1-2) could not be verified against the real
claudeCLI — spawning a nested session is blocked in the build environment. Installed CLI here is 2.1.220. If that CLI refuses to proceed when the server issues nomcp-session-id, the bridge yields a server with zero tools and the turn degrades to a toolless answer instead of erroring — no automated test catches this.Needs sign-off before the public endpoint is ever enabled
PUBLIC_MCP_ALLOW_WITHOUT_PRIVACY_MASKING— arguably should not exist.captureRawToolResultlogs pre-masking byte counts to stdout.Follow-ups this PR creates or leaves open
Admin surface for
public_mcp_key_bindings(endpoint is unusable without it) · a producer formcpUserKeyon chat turns · plugin-memory namespace seam (existing notes stay underdefault) · omadia-as-MCP-serverinput_required· a durable secondTaskStoreimplementor and an MCP-Tasks projection ·mcp_call_log.outcomecolumn · #540 split per the Wave 3 section · #547's canvas half (needs a pairedomadia-uiPR and a policy decision on whether a third-party server may drive the canvas) · #540's pooling simplification (the pool is also the OAuth-invalidation unit and holds stdio child processes).Closes #541
Closes #546
Refs #540, #542, #543, #544, #545, #547
#543and#545are complete against their decided scope but not their literal titles;#542's endpoint is merged dark pending an admin surface;#544and#547have their client/plumbing halves done here. Recommend retitling or closing rather than leaving them ambiguous.Wave 4–6 — closing the gaps (2026-07-31)
A second pass over this branch, planned as six units and reviewed cross-vendor with
gpt-5.6-sol. Two findings came out of the review that neither the implementers nor I hadlooked for, and one of them is a live production defect that was never in this PR's diff.
Ship-blockers found and closed
per_userMCP delegation was unreachable from chatMigration
0031made delegation explicit and gave new servers a fail-closedper_userdefault.resolveMcpUserKeyreadsturnContext.current()?.mcpUserKey— and the only thing that ever setit was the operator discover route.
routes/chat.tsdid not importturnContextat all. Everynewly created
per_userserver was dead from chat out of the box; existing installs were maskedonly because
0031backfills token-holding servers toservice.Both HTTP entries now open a turn scope carrying the identity. The streaming one uses
turnContext.runGenerator, notenter—enterWithbinds to the async resource executing at thatinstant and an async generator resumes in the caller's context, so the identity would be gone by the
orchestrator's first yield.
The value is
sessionIdentity(req), deliberately notresolveUserId(req), which falls throughto the client-sent
x-user-idheader. Keying MCP tokens on a client-controlled header would let anycaller act as any user. The channel-side producer is gated on
channelIdentityfor the same reason./authorizestores tokensunder the session-shaped key, so an affected user still fails closed rather than reaching their
server. Closing it needs a new method on the
KnowledgeGraphcontract. Narrower than it sounds — aper_usertoken can only exist for someone who completed/authorize, which requires a session, soa channel-only user has no token and failing closed is correct for them.
The MCP input replay put raw tool output on the LLM wire
Found by cross-vendor review. Live in any deployment with a graph pool, and not in this PR's
original diff.
Privacy Shield v4's boundary is server ↔ LLM provider, not server ↔ browser. The replay that
runs after a user answers an MCP input card called
mcpManager.callTooldirectly rather than goingthrough
dispatchTool, so its result was never interned — and was then interpolated verbatim intothe note folded into the turn's ingested text. A replayed HR or accounting tool returning a
personnel row sent that row to the model in cleartext.
The comment above the interpolation shows this was a near-miss rather than a decision: it reasons
explicitly about the LLM wire, but only about the user's typed values, and overlooks the tool result
two lines below.
A revoked public-MCP key binding was silently re-armed
Revoke is the incident-response lever on an internet-facing surface, and any upsert that omitted
enabledundid it — answering201 Createdfor what overwrote a deliberately parked row.The real bug was at the validation layer, not the router:
enabled: input.enabled ?? trueturnedabsence into assertion before the store could tell the difference. The field now stays absent
through validation and is resolved only where the current row is known, via
COALESCEagainst thetable-qualified column — not
EXCLUDED, which holds the row this statement proposed and resolvesstraight back to the insert's
true.Making revoke sticky removed the only un-park path from the UI, so this adds
POST /:keyId/restoreand a confirmed Restore button: un-parking is now an explicit act in an access log.
Also fixed
0031built neither of its guards reliably. Aconnamelookup with no relationfilter is cluster-wide, so a same-named constraint in any other schema silently skipped the
ALTER TABLE. A hardcodedto_regclass('public.…')in an otherwise unqualified file answeredabout a table the statement never touches. The backfill test previously rewrote the migration
to make it apply; it now applies verbatim.
entirely by
public_mcp_key_bindingsrows, which could previously only be created inpsql. Thepublic endpoint still receives only the read-only store — it gains no write path to its own
authorization table.
the untemplated one did not, so a routine fired from inside a chat turn inherited the invoking
user's identity — and behaved differently depending only on whether an output template happened to
be configured.
Infinity, so a hang burned the 15-minute CI wall with no attribution); web-ui's was 5 s, belowthe honest 5–13 s cost of its heavier RTL suites, which manufactured failures rather than catching
hangs. Four runs of an unchanged tree gave 0, 9, 25 and 0 failures purely as load varied.
0x00separators made ripgrep classifythose files as binary and stop reading partway through — silently truncating every audit that
crossed them. Proven no-op: none is followed by an ASCII digit, and every changed literal was read
back off disk and evaluated. This also turned out to be under-counting the decoupling ratchet by
seven references.
suite green.
Deliberately not done
refuted by the review: the browser is the trusted side and receives real values by design. The
renderer is deferred for ordinary reasons instead — a full-stack change on an already-large PR, and
the sidecar bypasses Privacy Shield's receipt and dataset accounting even where masking is not
owed, which wants a decision first.
[mcp_input_required:…]marker is itself interned, so the card never renders. The obvious fix isworse than the bug —
callToolreturns server text verbatim, so a prefix-shaped exemption isserver-forgeable: any MCP server could prefix an ordinary result and opt itself out of
interning, silently. A safe exemption must key on provenance, which is a design decision with an
owner. Filed rather than guessed.
TaskStoreand resume path, reaper fencing, the@modelcontextprotocol/*@2port, MCPpooling simplification, and a
keyId/agentIdexistence check — all filed as follow-ups.Verification
Every new assertion was mutation-checked: break the production line it claims to cover, rebuild,
re-run, confirm it fails for the stated reason, restore. That discipline earned its keep three
times — a suite that hung instead of going red because a throwaway server was closed after its
assertions rather than in a
finally; a mutation run that passed over deliberately broken codebecause the test imported through a package barrel; and a mutation harness that corrupted the module
under test and would have reported "caught" for the wrong reason.
Gates: middleware 5831/5835 (4 skipped, 0 failed), web-ui 427/427, typecheck clean, lint 0
errors, i18n 3335 keys in both locales, decoupling ratchet held.