From 61f36da16696da96771172a8575775e1cf62837f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 13:56:11 +0000 Subject: [PATCH] fix(spec,runtime): `resolveService` returns the slot's contract too, and the `: any` escapes on core slots are gone (#4127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batch 2 of the #4127 gate. #4168 typed `getService` — easy, because every one of its call sites already passed a `CoreServiceName`. `resolveService` is the mixed one, and it is where the remaining `any` lived. Overloads split it exactly where the evidence does. A `CoreServiceName` resolves to the slot's contract; anything else keeps `any`: - Core slots, however written. 17 call sites address a core slot with a bare literal (`'metadata'` x10, `'automation'` x3, `'auth'` x3, `'ai'`) rather than `CoreServiceName.enum.*` — the same slot addressed two ways. Both resolve to the contract now, with no edit to the call sites. - Everything else — `protocol` (x22), `objectql` (x9), `mcp`, `kernel-resolver`, `security`, `scope-manager`. Real services with no `CoreServiceName` entry and no written contract. They keep `any` rather than being given a shape here that nothing verifies: that `any` is where the ledger honestly ends, and writing those contracts is its own change. The typing was being erased at three call sites, and that is the actual finding. `const x: any = await deps.resolveService('auth', ...)` defeats all of this — the annotation wins and #4168's work does nothing there. Sweeping for the pattern found three on core slots: `/mcp` x2 — two more undeclared methods. The domain calls `authService?.getMcpResourceUrl?.()` and `?.getMcpResourceMetadataUrl?.()`. AuthManager implements both (plugin-auth uses them internally); IAuthService declared neither. Call site and implementation agree, the contract is the thing nobody wrote. The `: any` + optional-chaining combination made this WORSE than the earlier gaps, not better: invisible to the type system AND accidentally safe. An absent method returns `undefined`, so the skill route silently fell back to deriving an MCP URL from the request host — a real disagreement between the auth service's canonical value and the derived one would have looked like normal operation. The whole point of `getMcpResourceUrl` is that it comes off the auth `basePath` so the two CANNOT disagree about the API prefix; the route's own comment says "the auth service owns the canonical value". Both declared optional: an auth provider without MCP/OAuth support fills the slot legitimately, and `getMcpResourceMetadataUrl` returning `null` (OAuth track off) stays distinct from the method being absent. `/packages` x1 — `const metadata: any = await deps.getService(...metadata)`, feeding `new SeedLoaderService(ql, metadata, ...)`. Annotation dropped; it typechecks against IMetadataService now. Its neighbours `protocol` and `ql` keep their `any` for the honest reason above. No other core-slot lookup is annotated away — the sweep is exhaustive over domains/*.ts. `api-surface.json` is unchanged: the two additions are interface MEMBERS, not new exports. Refs #4127 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015T5CeitwV1jXLvsL1A84tw --- .../resolve-service-returns-its-contract.md | 63 +++++++++++++++++++ .../runtime/src/domain-handler-registry.ts | 21 ++++++- packages/runtime/src/domains/mcp.ts | 11 +++- packages/runtime/src/domains/packages.ts | 6 +- packages/runtime/src/http-dispatcher.ts | 11 ++-- .../spec/src/contracts/auth-service.test.ts | 43 +++++++++++++ packages/spec/src/contracts/auth-service.ts | 32 ++++++++++ 7 files changed, 179 insertions(+), 8 deletions(-) create mode 100644 .changeset/resolve-service-returns-its-contract.md diff --git a/.changeset/resolve-service-returns-its-contract.md b/.changeset/resolve-service-returns-its-contract.md new file mode 100644 index 0000000000..03c4526ca5 --- /dev/null +++ b/.changeset/resolve-service-returns-its-contract.md @@ -0,0 +1,63 @@ +--- +"@objectstack/spec": minor +"@objectstack/runtime": patch +--- + +fix(spec,runtime): `resolveService` returns the slot's contract too, and the `: any` escapes on core slots are gone (#4127) + +Batch 2 of the #4127 gate. #4168 typed `getService` — easy, because every one of +its call sites already passed a `CoreServiceName`. `resolveService` is the mixed +one, and it is where the remaining `any` lived. + +**Overloads split it exactly where the evidence does.** A `CoreServiceName` +resolves to the slot's contract; anything else keeps `any`: + +- **Core slots, however written.** 17 call sites address a core slot with a bare + literal — `'metadata'` ×10, `'automation'` ×3, `'auth'` ×3, `'ai'` — rather + than `CoreServiceName.enum.*`. The same slot was being addressed two ways; + both resolve to the contract now, with no edit to the call sites. +- **Everything else** — `protocol` (×22), `objectql` (×9), `mcp`, + `kernel-resolver`, `security`, `scope-manager`. Real services with no + `CoreServiceName` entry and no written contract. They keep `any` rather than + being given a shape here that nothing verifies: **that `any` is where the + ledger honestly ends**, and writing those contracts is its own change. + +**The typing was being erased at three call sites, and that is the actual +finding.** A `const x: any = await deps.resolveService('auth', …)` defeats every +bit of this — the annotation wins, and #4168's work does nothing there. Sweeping +for the pattern found three on core slots: + +**`/mcp` ×2 — two more undeclared methods.** The domain calls +`authService?.getMcpResourceUrl?.()` and `?.getMcpResourceMetadataUrl?.()`. +`AuthManager` implements both (and plugin-auth uses them internally); +`IAuthService` declared neither. Classic #4127 shape — call site and +implementation agree, the contract is the thing nobody wrote. + +The `: any` + optional-chaining combination made this *worse* than the earlier +gaps, not better: it made the call invisible to the type system **and** +accidentally safe. An absent method returns `undefined`, so the skill route +silently fell back to deriving an MCP URL from the request host — meaning a real +disagreement between the auth service's canonical value and the derived one +would have looked exactly like normal operation. The whole point of +`getMcpResourceUrl` is that it comes off the auth `basePath` so the two *cannot* +disagree about the API prefix; the route's own comment says "the auth service +owns the canonical value". + +Both are declared optional: an auth provider without MCP/OAuth support fills the +slot legitimately, and `getMcpResourceMetadataUrl` returning `null` (OAuth track +off — AS disabled or the origin fails the OAuth 2.1 transport rule) stays +distinct from the method being absent. + +**`/packages` ×1 —** `const metadata: any = await deps.getService(…metadata)`, +feeding `new SeedLoaderService(ql, metadata, …)`. Annotation dropped; it +typechecks against `IMetadataService` now. Its neighbours `protocol` and `ql` +keep their `any` for the honest reason above. + +No other core-slot lookup is annotated away — the sweep is exhaustive over +`domains/*.ts`. + +Verified: `@objectstack/runtime` **937 tests / 65 files**, `@objectstack/spec` +**7112 / 273** (3 new on the auth contract), adapter-hono **73**; `tsc --noEmit` +on spec, runtime, downstream-contract and all four examples; `pnpm lint`; all +nine `check:*` gates. `api-surface.json` is unchanged — the two additions are +interface MEMBERS, not new exports. diff --git a/packages/runtime/src/domain-handler-registry.ts b/packages/runtime/src/domain-handler-registry.ts index f93d239b12..77aa811d44 100644 --- a/packages/runtime/src/domain-handler-registry.ts +++ b/packages/runtime/src/domain-handler-registry.ts @@ -78,7 +78,26 @@ export interface DomainRoute { * more dispatcher surface. */ export interface DomainHandlerDeps { - /** Environment-scoped service resolution (per-request kernel aware). */ + /** + * Environment-scoped service resolution (per-request kernel aware), typed + * by the slot when the slot is a core one. + * + * [#4127 batch 2] `getService` got this treatment first because every one of + * its call sites already passed a `CoreServiceName`. This one is mixed, and + * the overloads split it exactly where the evidence does: + * + * - A **`CoreServiceName`** — however it is written. 17 call sites address a + * core slot with a bare literal (`'metadata'` ×10, `'automation'` ×3, + * `'auth'` ×3, `'ai'`) rather than `CoreServiceName.enum.*`, so the same + * slot was being addressed two ways; both resolve to the contract now. + * - **Anything else** — `protocol`, `objectql`, `mcp`, `kernel-resolver`, + * `security`, `scope-manager`. These are real services with no + * `CoreServiceName` entry and no written contract, so they keep today's + * `any` rather than being given a shape here that nothing verifies. + * Writing their contracts is the next batch; until then the `any` marks + * where the ledger ends. + */ + resolveService(name: K, environmentId?: string): Promise | undefined>; resolveService(name: string, environmentId?: string): any; /** * Unscoped service lookup on the current kernel, typed by the slot. diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 65855385ad..40840be0a6 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -193,7 +193,13 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str // when the auth plugin isn't loaded. let mcpUrl: string | undefined; try { - const authService: any = await deps.resolveService('auth', context.environmentId); + // [#4127] Was `const authService: any`, which erased the slot's type + // even after `resolveService` started returning it — the escape hatch + // this batch closes. `getMcpResourceUrl` is declared on `IAuthService` + // now, so `?.()` reads a declared optional capability (an auth provider + // without MCP/OAuth support fills this slot legitimately) instead of + // guessing at a method the contract never mentioned. + const authService = await deps.resolveService('auth', context.environmentId); const url = authService?.getMcpResourceUrl?.(); if (typeof url === 'string' && url) mcpUrl = url; } catch { /* fall through to host derivation */ } @@ -242,7 +248,8 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str */ async function getMcpResourceMetadataUrl(deps: DomainHandlerDeps, context: HttpProtocolContext): Promise { try { - const authService: any = await deps.resolveService('auth', context.environmentId); + // [#4127] Same `: any` erasure as the skill route above; same fix. + const authService = await deps.resolveService('auth', context.environmentId); const url = authService?.getMcpResourceMetadataUrl?.(); return typeof url === 'string' && url ? url : null; } catch { diff --git a/packages/runtime/src/domains/packages.ts b/packages/runtime/src/domains/packages.ts index 024c54d79d..578d49b28e 100644 --- a/packages/runtime/src/domains/packages.ts +++ b/packages/runtime/src/domains/packages.ts @@ -624,7 +624,11 @@ organizationId: string | undefined, _context: HttpProtocolContext, ): Promise<{ success: boolean; inserted?: number; updated?: number; errors?: unknown[]; error?: string }> { const protocol: any = await deps.resolveService('protocol'); - const metadata: any = await deps.getService(CoreServiceName.enum.metadata); + // [#4127] `metadata` was annotated `: any`, which erased the slot type even + // after the lookup started returning `IMetadataService`. `protocol` and + // `ql` keep theirs: neither is a `CoreServiceName` slot and neither has a + // written contract, so their `any` is where the ledger honestly ends. + const metadata = await deps.getService(CoreServiceName.enum.metadata); const ql: any = await deps.resolveService('objectql'); if (!protocol || typeof protocol.getMetaItem !== 'function' || !ql || !metadata) { return { success: false, error: 'seed apply: required services unavailable' }; diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index f40107d908..636b96798a 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -225,10 +225,13 @@ export class HttpDispatcher { * touch — see {@link DomainHandlerDeps}. */ private readonly domainDeps: DomainHandlerDeps = { - resolveService: (name, environmentId) => this.resolveService(name, environmentId), - // Deps take plain strings (domain modules pass CoreServiceName enum - // values anyway); the dispatcher method's parameter is the enum type. - getService: (name) => this.getService(name as Parameters[0]), + // [#4127] Both are slot-typed on the deps contract now (`resolveService` + // via overloads, since it also takes non-core names like `protocol`). + // The parameters are annotated because an arrow cannot be contextually + // typed against an overloaded signature. Resolution below stays + // name-based and unchanged — the typing lives in what the DOMAINS see. + resolveService: (name: string, environmentId?: string) => this.resolveService(name, environmentId), + getService: (name: string) => this.getService(name as Parameters[0]), getObjectQL: (environmentId) => this.getObjectQLService(environmentId), // Reads off the per-request RESOLVED kernel (`this.kernel` is set by // dispatch() before any handler runs) — see the deps contract note. diff --git a/packages/spec/src/contracts/auth-service.test.ts b/packages/spec/src/contracts/auth-service.test.ts index cc46be43f5..d99147fdf0 100644 --- a/packages/spec/src/contracts/auth-service.test.ts +++ b/packages/spec/src/contracts/auth-service.test.ts @@ -71,4 +71,47 @@ describe('Auth Service Contract', () => { expect(sessions.has('s1')).toBe(false); expect(sessions.has('s2')).toBe(true); }); + + // [#4127 batch 2] The `/mcp` domain called both of these through + // `const authService: any` + `?.()`, so the calls were invisible to the type + // system AND accidentally safe: an absent method returned `undefined` and the + // route silently fell back, making a real disagreement look like normal + // operation. AuthManager implements both; only the contract was missing. + it('should expose the MCP resource identity an auth service owns', () => { + const service: IAuthService = { + handleRequest: async () => new Response('OK'), + verify: async () => ({ success: true }), + getMcpResourceUrl: () => 'https://acme.example.com/api/v1/mcp', + getMcpResourceMetadataUrl: () => 'https://acme.example.com/.well-known/oauth-protected-resource', + }; + + expect(service.getMcpResourceUrl!()).toBe('https://acme.example.com/api/v1/mcp'); + expect(service.getMcpResourceMetadataUrl!()).toContain('/.well-known/oauth-protected-resource'); + }); + + it('should let getMcpResourceMetadataUrl report the OAuth track as off', () => { + // `null` is the fail-closed answer: the embedded AS is disabled, or the + // origin fails the OAuth 2.1 transport rule. API keys remain and nothing is + // advertised. Distinct from the method being absent entirely. + const service: IAuthService = { + handleRequest: async () => new Response('OK'), + verify: async () => ({ success: true }), + getMcpResourceMetadataUrl: () => null, + }; + + expect(service.getMcpResourceMetadataUrl!()).toBeNull(); + }); + + it('should allow an auth service with no MCP surface at all', () => { + // Both are optional: an auth provider without MCP/OAuth support fills the + // slot legitimately, and the `/mcp` route derives a URL from the request + // host instead. + const service: IAuthService = { + handleRequest: async () => new Response('OK'), + verify: async () => ({ success: true }), + }; + + expect(service.getMcpResourceUrl).toBeUndefined(); + expect(service.getMcpResourceMetadataUrl).toBeUndefined(); + }); }); diff --git a/packages/spec/src/contracts/auth-service.ts b/packages/spec/src/contracts/auth-service.ts index 90151d1a10..e7db6c7cf5 100644 --- a/packages/spec/src/contracts/auth-service.ts +++ b/packages/spec/src/contracts/auth-service.ts @@ -84,4 +84,36 @@ export interface IAuthService { * @returns Authenticated user or undefined */ getCurrentUser?(request: Request): Promise; + + /** + * The MCP resource identifier (RFC 8707 `resource` / token `aud`) — + * `/mcp`. + * + * [#4127] Declared because `/mcp` already called it. The dispatcher's skill + * route needs the canonical value the auth service derives from its own + * `basePath`, precisely so the two cannot disagree about the API prefix; + * its own comment says "the auth service owns the canonical value". It was + * reached through `const authService: any` + `?.()`, which made the call + * invisible to the type system AND accidentally safe — an absent method + * returned `undefined` and the route silently fell back to deriving a URL + * from the request host, so a real disagreement would have looked like + * normal operation. + * + * @returns The absolute MCP resource URL + */ + getMcpResourceUrl?(): string; + + /** + * Absolute URL of the RFC 9728 protected-resource metadata document, + * advertised in `WWW-Authenticate` on 401s from the MCP endpoint so clients + * can bootstrap the OAuth flow. `null` when the OAuth track is off for this + * deployment (the embedded AS disabled, or the origin fails the OAuth 2.1 + * transport rule) — API keys remain and nothing is advertised, fail-closed. + * + * [#4127] Same story as {@link getMcpResourceUrl}: called by `/mcp`, + * implemented by the auth manager, declared by nobody. + * + * @returns The metadata URL, or `null` when the OAuth track is off + */ + getMcpResourceMetadataUrl?(): string | null; }