Skip to content

fix(rest): route probeMcpServeable through the shared environment entry point so /discovery's mcp advertisement follows the request's kernel - #9291

Open
os-project-manager wants to merge 1 commit into
mainfrom
claude/issue-9120-probe-mcp-serveable-shared-entry
Open

fix(rest): route probeMcpServeable through the shared environment entry point so /discovery's mcp advertisement follows the request's kernel#9291
os-project-manager wants to merge 1 commit into
mainfrom
claude/issue-9120-probe-mcp-serveable-shared-entry

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes #9120

What was wrong

RestServer.resolveRequestEnvironmentId describes itself, in its own doc-comment, as

THE single entry point for every unscoped-route environment decision (protocol, i18n, exec-ctx, analytics, …) so they can never disagree about which kernel a request belongs to.

Re-counted on origin/main @ b0fa4fc1a rather than taken from the card: eight call sites go through it — resolveEndpointMatchAuthority, resolveMetadataService, resolveProtocol, resolveI18nService, computeExecCtx, resolveSecurityService, the analytics service resolver and the security-explain service resolver. The card's count still holds.

probeMcpServeable is the ninth site that needs the request's environment — and the one whose answer decides whether /discovery advertises routes.mcp. It re-derived its own:

let environmentId: string | undefined = req?.params?.environmentId;
if ((!environmentId || environmentId === ':environmentId') && this.defaultEnvironmentIdProvider) {
    try { environmentId = this.defaultEnvironmentIdProvider() || undefined; } catch { /* ignore */ }
}

That is the shared chain minus its first and middle steps: the host's ADR-0006 kernel-resolver seam, and the legacy hostname / X-Environment-Id chain beneath it. This is declared not equal to enforced in the very code #4024 added to close that class.

Single-environment boots were correct throughout, which is why this never surfaced: there defaultEnvironmentIdProvider is registered, and it is also step 3 of the shared chain, so both spellings agreed. The defect is multi-tenant-only — on a hostname-routed host an unscoped /discovery request carries no params.environmentId, and no default provider is registered (that is createSingleEnvironmentPlugin's wiring). Neither input the probe read was present, so it fell through to serviceExistsProvider: the host kernel's answer, not the request environment's.

The fix

probeMcpServeable now calls resolveRequestEnvironmentId like its eight siblings. Per the triage scope, the 'platform' guard and the serviceExistsProvider fallback are unchanged.

One addition the swap requires: the unsubstituted ':environmentId' route pattern is normalised to "no id" before the call. The entry point short-circuits on any truthy explicit value, so passing the pattern through would have handed it to getOrCreate as if it were a kernel id. The pin never mistakes the literal ":environmentId" placeholder for an environment covers this, and it was one of the seven that failed pre-fix.

This also makes good the parity the probe's doc-comment already claimed with resolveRegisteredServices, whose kernel arrives as ctx.__kernel — set downstream of the same entry point, which is why that sibling was never exposed to this.

Reverse verification — direction predicted before running

Predicted: the four multi-tenant reachability pins, the placeholder pin and both /discovery end-to-end pins go red against unfixed code; the single-env, 'platform', fallback and null pins stay green (they encode behaviour that was already correct).

Observed, tests written first and run against unfixed packages/rest/src/rest-server.ts7 failed / 13 passed, exactly the predicted split:

× probeMcpServeable > answers for the REQUEST environment when the host kernel serves mcp and that environment does not
× probeMcpServeable > answers for the REQUEST environment when it serves mcp and the host kernel does not
× probeMcpServeable > reaches the request environment through the legacy hostname chain when no resolver is injected
× probeMcpServeable > follows X-Environment-Id, which the hand-rolled derivation never read
✓ probeMcpServeable > keeps the single-environment answer unchanged (default provider goes to that kernel)
✓ probeMcpServeable > keeps the 'platform' guard — the reserved id is never handed to getOrCreate
× probeMcpServeable > never mistakes the literal ":environmentId" placeholder for an environment
✓ probeMcpServeable > keeps the serviceExistsProvider fallback when no environment resolves at all
✓ probeMcpServeable > still reports null ("cannot probe") when nothing in either path can answer
× /discovery mcp advertisement > withholds routes.mcp when the request environment cannot serve it, though the host kernel can
× /discovery mcp advertisement > advertises routes.mcp when the request environment serves it, though the host kernel does not

Both misadvertisement directions are therefore measured, not argued:

  • host kernel has mcp, request's environment does not — pre-fix /discovery returned routes.mcp = '/api/v1/mcp' for an environment whose route 501s (expected '/api/v1/mcp' to be undefined);
  • host kernel lacks it, environment has it — pre-fix the route was withheld from an environment that would have served it (expected undefined to be '/api/v1/mcp'). mcpServeable !== false fails open only for a null probe, never for a confident false computed against the wrong kernel.

After the fix: 20 passed (20) in that file.

The req confirmation the card asked for

Confirmed — no finding. The /discovery handler passes probeMcpServeable(req) the same handler req the other resolvers receive, unmodified. The only other request-derived values in that handler are getDirectMountRouteBases(...) and getMountedEmailRouteBase(...), which take an environment-id string for route-template substitution and are not environment resolvers.

The two /discovery end-to-end pins prove it empirically rather than by reading: they drive the real registered GET /api/v1/discovery handler with a request carrying only a Host header, and the advertisement follows the resolution chain — which it can only do if the req reaching the probe still carried the headers that chain reads.

Tests

New pins live in packages/rest/src/rest-env-resolution.test.ts, the existing ADR-0076 D11 step 4 seam file — the right home, since this is that seam's ninth consumer. The harness's buildRest gained a serviceExistsProvider seam (it was hard-wired undefined), which is what lets a test state "the host says yes and the environment says no" at all.

Verification at 6e7fe991d (the head this PR pushes):

  • pnpm --filter @objectstack/rest typecheck — pass
  • pnpm --filter @objectstack/rest test122 test files, 2022 tests, all passed
  • gate union from node scripts/pm/dispatch-gates.mjs over the actual changed paths, all green: check:authz-resolver, check:route-envelope, check:dispatcher-error-vocabulary, check:cross-package-test-inputs, check:changeset-gate-self-tests, check:objectui-changeset, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check:affected-docs
  • convention-triggered (this diff edits a test file), all green: check:engine-double-contract, check:where-matcher, check:query-options-erasure, check:type-check-coverage, plus check:nul-bytes

check:route-envelope reports its pre-existing #7035 ratchet warnings on rest-server.ts (stringError 44, siblingCode 69) and exits 0 — untouched by this diff, and the error-envelope seam is deliberately out of scope here.

Scope

Environment-resolution seam only. #9232 and #9267 are not addressed here — no error-envelope code is touched.

A same-class observation found while verifying is filed separately rather than fixed here: the /discovery handler builds its document from this.protocol.getDiscovery() (the control-plane protocol) rather than resolveProtocol(environmentId, req), on the scoped route too. That is a different seam with a much wider blast radius than this card's, so it gets its own issue.


Generated by Claude Code

…ry point (#9120)

`resolveRequestEnvironmentId` is, by its own doc-comment, "THE single entry
point for every unscoped-route environment decision ... so they can never
disagree about which kernel a request belongs to". Eight consumers go through
it; `probeMcpServeable` -- the ninth site needing the request's environment,
and the one deciding whether /discovery advertises `routes.mcp` -- re-derived
its own from `params.environmentId` else `defaultEnvironmentIdProvider`.

That is the shared chain minus the host's ADR-0006 `kernel-resolver` seam and
the legacy hostname / X-Environment-Id steps. Single-environment boots agreed
anyway (the default provider is step 3 of the shared chain), so the defect was
multi-tenant-only: on a hostname-routed host an unscoped /discovery request has
no `params.environmentId` and no default provider is registered, so the probe
fell through to `serviceExistsProvider` -- the HOST kernel's answer, not the
request environment's. Both misadvertisement directions were reachable and are
now pinned.

The `'platform'` guard and the `serviceExistsProvider` fallback are unchanged.
The unsubstituted `':environmentId'` route pattern is normalised to "no id"
before the call, since the entry point short-circuits on any truthy explicit
value.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest, touching 6 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/client-sdk.mdx (via getBookTree (sdk), meta.getBookTree (sdk))

4 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx (via RestServer (symbol))
  • content/docs/releases/v12.mdx (via RestServer (symbol))
  • content/docs/releases/v14.mdx (via /book/:name/tree (route))
  • content/docs/releases/v16.mdx (via RestServer (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see

Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json origin/mainpackageMentionDocs.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 17, 2026
@os-project-manager
os-project-manager marked this pull request as ready for review August 17, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants