Skip to content

Commit 2cb6d3c

Browse files
os-zhuangclaude
andauthored
fix(spec,runtime): resolveService returns the slot's contract too, and the : any escapes on core slots are gone (#4127) (#4176)
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 Claude-Session: https://claude.ai/code/session_015T5CeitwV1jXLvsL1A84tw Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9b702dc commit 2cb6d3c

7 files changed

Lines changed: 179 additions & 8 deletions

File tree

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/runtime": patch
4+
---
5+
6+
fix(spec,runtime): `resolveService` returns the slot's contract too, and the `: any` escapes on core slots are gone (#4127)
7+
8+
Batch 2 of the #4127 gate. #4168 typed `getService` — easy, because every one of
9+
its call sites already passed a `CoreServiceName`. `resolveService` is the mixed
10+
one, and it is where the remaining `any` lived.
11+
12+
**Overloads split it exactly where the evidence does.** A `CoreServiceName`
13+
resolves to the slot's contract; anything else keeps `any`:
14+
15+
- **Core slots, however written.** 17 call sites address a core slot with a bare
16+
literal — `'metadata'` ×10, `'automation'` ×3, `'auth'` ×3, `'ai'` — rather
17+
than `CoreServiceName.enum.*`. The same slot was being addressed two ways;
18+
both resolve to the contract now, with no edit to the call sites.
19+
- **Everything else**`protocol` (×22), `objectql` (×9), `mcp`,
20+
`kernel-resolver`, `security`, `scope-manager`. Real services with no
21+
`CoreServiceName` entry and no written contract. They keep `any` rather than
22+
being given a shape here that nothing verifies: **that `any` is where the
23+
ledger honestly ends**, and writing those contracts is its own change.
24+
25+
**The typing was being erased at three call sites, and that is the actual
26+
finding.** A `const x: any = await deps.resolveService('auth', …)` defeats every
27+
bit of this — the annotation wins, and #4168's work does nothing there. Sweeping
28+
for the pattern found three on core slots:
29+
30+
**`/mcp` ×2 — two more undeclared methods.** The domain calls
31+
`authService?.getMcpResourceUrl?.()` and `?.getMcpResourceMetadataUrl?.()`.
32+
`AuthManager` implements both (and plugin-auth uses them internally);
33+
`IAuthService` declared neither. Classic #4127 shape — call site and
34+
implementation agree, the contract is the thing nobody wrote.
35+
36+
The `: any` + optional-chaining combination made this *worse* than the earlier
37+
gaps, not better: it made the call invisible to the type system **and**
38+
accidentally safe. An absent method returns `undefined`, so the skill route
39+
silently fell back to deriving an MCP URL from the request host — meaning a real
40+
disagreement between the auth service's canonical value and the derived one
41+
would have looked exactly like normal operation. The whole point of
42+
`getMcpResourceUrl` is that it comes off the auth `basePath` so the two *cannot*
43+
disagree about the API prefix; the route's own comment says "the auth service
44+
owns the canonical value".
45+
46+
Both are declared optional: an auth provider without MCP/OAuth support fills the
47+
slot legitimately, and `getMcpResourceMetadataUrl` returning `null` (OAuth track
48+
off — AS disabled or the origin fails the OAuth 2.1 transport rule) stays
49+
distinct from the method being absent.
50+
51+
**`/packages` ×1 —** `const metadata: any = await deps.getService(…metadata)`,
52+
feeding `new SeedLoaderService(ql, metadata, …)`. Annotation dropped; it
53+
typechecks against `IMetadataService` now. Its neighbours `protocol` and `ql`
54+
keep their `any` for the honest reason above.
55+
56+
No other core-slot lookup is annotated away — the sweep is exhaustive over
57+
`domains/*.ts`.
58+
59+
Verified: `@objectstack/runtime` **937 tests / 65 files**, `@objectstack/spec`
60+
**7112 / 273** (3 new on the auth contract), adapter-hono **73**; `tsc --noEmit`
61+
on spec, runtime, downstream-contract and all four examples; `pnpm lint`; all
62+
nine `check:*` gates. `api-surface.json` is unchanged — the two additions are
63+
interface MEMBERS, not new exports.

packages/runtime/src/domain-handler-registry.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,26 @@ export interface DomainRoute {
7878
* more dispatcher surface.
7979
*/
8080
export interface DomainHandlerDeps {
81-
/** Environment-scoped service resolution (per-request kernel aware). */
81+
/**
82+
* Environment-scoped service resolution (per-request kernel aware), typed
83+
* by the slot when the slot is a core one.
84+
*
85+
* [#4127 batch 2] `getService` got this treatment first because every one of
86+
* its call sites already passed a `CoreServiceName`. This one is mixed, and
87+
* the overloads split it exactly where the evidence does:
88+
*
89+
* - A **`CoreServiceName`** — however it is written. 17 call sites address a
90+
* core slot with a bare literal (`'metadata'` ×10, `'automation'` ×3,
91+
* `'auth'` ×3, `'ai'`) rather than `CoreServiceName.enum.*`, so the same
92+
* slot was being addressed two ways; both resolve to the contract now.
93+
* - **Anything else** — `protocol`, `objectql`, `mcp`, `kernel-resolver`,
94+
* `security`, `scope-manager`. These are real services with no
95+
* `CoreServiceName` entry and no written contract, so they keep today's
96+
* `any` rather than being given a shape here that nothing verifies.
97+
* Writing their contracts is the next batch; until then the `any` marks
98+
* where the ledger ends.
99+
*/
100+
resolveService<K extends CoreServiceName>(name: K, environmentId?: string): Promise<CoreServiceContract<K> | undefined>;
82101
resolveService(name: string, environmentId?: string): any;
83102
/**
84103
* Unscoped service lookup on the current kernel, typed by the slot.

packages/runtime/src/domains/mcp.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,13 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str
193193
// when the auth plugin isn't loaded.
194194
let mcpUrl: string | undefined;
195195
try {
196-
const authService: any = await deps.resolveService('auth', context.environmentId);
196+
// [#4127] Was `const authService: any`, which erased the slot's type
197+
// even after `resolveService` started returning it — the escape hatch
198+
// this batch closes. `getMcpResourceUrl` is declared on `IAuthService`
199+
// now, so `?.()` reads a declared optional capability (an auth provider
200+
// without MCP/OAuth support fills this slot legitimately) instead of
201+
// guessing at a method the contract never mentioned.
202+
const authService = await deps.resolveService('auth', context.environmentId);
197203
const url = authService?.getMcpResourceUrl?.();
198204
if (typeof url === 'string' && url) mcpUrl = url;
199205
} catch { /* fall through to host derivation */ }
@@ -242,7 +248,8 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str
242248
*/
243249
async function getMcpResourceMetadataUrl(deps: DomainHandlerDeps, context: HttpProtocolContext): Promise<string | null> {
244250
try {
245-
const authService: any = await deps.resolveService('auth', context.environmentId);
251+
// [#4127] Same `: any` erasure as the skill route above; same fix.
252+
const authService = await deps.resolveService('auth', context.environmentId);
246253
const url = authService?.getMcpResourceMetadataUrl?.();
247254
return typeof url === 'string' && url ? url : null;
248255
} catch {

packages/runtime/src/domains/packages.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,11 @@ organizationId: string | undefined,
624624
_context: HttpProtocolContext,
625625
): Promise<{ success: boolean; inserted?: number; updated?: number; errors?: unknown[]; error?: string }> {
626626
const protocol: any = await deps.resolveService('protocol');
627-
const metadata: any = await deps.getService(CoreServiceName.enum.metadata);
627+
// [#4127] `metadata` was annotated `: any`, which erased the slot type even
628+
// after the lookup started returning `IMetadataService`. `protocol` and
629+
// `ql` keep theirs: neither is a `CoreServiceName` slot and neither has a
630+
// written contract, so their `any` is where the ledger honestly ends.
631+
const metadata = await deps.getService(CoreServiceName.enum.metadata);
628632
const ql: any = await deps.resolveService('objectql');
629633
if (!protocol || typeof protocol.getMetaItem !== 'function' || !ql || !metadata) {
630634
return { success: false, error: 'seed apply: required services unavailable' };

packages/runtime/src/http-dispatcher.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -225,10 +225,13 @@ export class HttpDispatcher {
225225
* touch — see {@link DomainHandlerDeps}.
226226
*/
227227
private readonly domainDeps: DomainHandlerDeps = {
228-
resolveService: (name, environmentId) => this.resolveService(name, environmentId),
229-
// Deps take plain strings (domain modules pass CoreServiceName enum
230-
// values anyway); the dispatcher method's parameter is the enum type.
231-
getService: (name) => this.getService(name as Parameters<HttpDispatcher['getService']>[0]),
228+
// [#4127] Both are slot-typed on the deps contract now (`resolveService`
229+
// via overloads, since it also takes non-core names like `protocol`).
230+
// The parameters are annotated because an arrow cannot be contextually
231+
// typed against an overloaded signature. Resolution below stays
232+
// name-based and unchanged — the typing lives in what the DOMAINS see.
233+
resolveService: (name: string, environmentId?: string) => this.resolveService(name, environmentId),
234+
getService: (name: string) => this.getService(name as Parameters<HttpDispatcher['getService']>[0]),
232235
getObjectQL: (environmentId) => this.getObjectQLService(environmentId),
233236
// Reads off the per-request RESOLVED kernel (`this.kernel` is set by
234237
// dispatch() before any handler runs) — see the deps contract note.

packages/spec/src/contracts/auth-service.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,4 +71,47 @@ describe('Auth Service Contract', () => {
7171
expect(sessions.has('s1')).toBe(false);
7272
expect(sessions.has('s2')).toBe(true);
7373
});
74+
75+
// [#4127 batch 2] The `/mcp` domain called both of these through
76+
// `const authService: any` + `?.()`, so the calls were invisible to the type
77+
// system AND accidentally safe: an absent method returned `undefined` and the
78+
// route silently fell back, making a real disagreement look like normal
79+
// operation. AuthManager implements both; only the contract was missing.
80+
it('should expose the MCP resource identity an auth service owns', () => {
81+
const service: IAuthService = {
82+
handleRequest: async () => new Response('OK'),
83+
verify: async () => ({ success: true }),
84+
getMcpResourceUrl: () => 'https://acme.example.com/api/v1/mcp',
85+
getMcpResourceMetadataUrl: () => 'https://acme.example.com/.well-known/oauth-protected-resource',
86+
};
87+
88+
expect(service.getMcpResourceUrl!()).toBe('https://acme.example.com/api/v1/mcp');
89+
expect(service.getMcpResourceMetadataUrl!()).toContain('/.well-known/oauth-protected-resource');
90+
});
91+
92+
it('should let getMcpResourceMetadataUrl report the OAuth track as off', () => {
93+
// `null` is the fail-closed answer: the embedded AS is disabled, or the
94+
// origin fails the OAuth 2.1 transport rule. API keys remain and nothing is
95+
// advertised. Distinct from the method being absent entirely.
96+
const service: IAuthService = {
97+
handleRequest: async () => new Response('OK'),
98+
verify: async () => ({ success: true }),
99+
getMcpResourceMetadataUrl: () => null,
100+
};
101+
102+
expect(service.getMcpResourceMetadataUrl!()).toBeNull();
103+
});
104+
105+
it('should allow an auth service with no MCP surface at all', () => {
106+
// Both are optional: an auth provider without MCP/OAuth support fills the
107+
// slot legitimately, and the `/mcp` route derives a URL from the request
108+
// host instead.
109+
const service: IAuthService = {
110+
handleRequest: async () => new Response('OK'),
111+
verify: async () => ({ success: true }),
112+
};
113+
114+
expect(service.getMcpResourceUrl).toBeUndefined();
115+
expect(service.getMcpResourceMetadataUrl).toBeUndefined();
116+
});
74117
});

packages/spec/src/contracts/auth-service.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,4 +84,36 @@ export interface IAuthService {
8484
* @returns Authenticated user or undefined
8585
*/
8686
getCurrentUser?(request: Request): Promise<AuthUser | undefined>;
87+
88+
/**
89+
* The MCP resource identifier (RFC 8707 `resource` / token `aud`) —
90+
* `<origin><apiPrefix>/mcp`.
91+
*
92+
* [#4127] Declared because `/mcp` already called it. The dispatcher's skill
93+
* route needs the canonical value the auth service derives from its own
94+
* `basePath`, precisely so the two cannot disagree about the API prefix;
95+
* its own comment says "the auth service owns the canonical value". It was
96+
* reached through `const authService: any` + `?.()`, which made the call
97+
* invisible to the type system AND accidentally safe — an absent method
98+
* returned `undefined` and the route silently fell back to deriving a URL
99+
* from the request host, so a real disagreement would have looked like
100+
* normal operation.
101+
*
102+
* @returns The absolute MCP resource URL
103+
*/
104+
getMcpResourceUrl?(): string;
105+
106+
/**
107+
* Absolute URL of the RFC 9728 protected-resource metadata document,
108+
* advertised in `WWW-Authenticate` on 401s from the MCP endpoint so clients
109+
* can bootstrap the OAuth flow. `null` when the OAuth track is off for this
110+
* deployment (the embedded AS disabled, or the origin fails the OAuth 2.1
111+
* transport rule) — API keys remain and nothing is advertised, fail-closed.
112+
*
113+
* [#4127] Same story as {@link getMcpResourceUrl}: called by `/mcp`,
114+
* implemented by the auth manager, declared by nobody.
115+
*
116+
* @returns The metadata URL, or `null` when the OAuth track is off
117+
*/
118+
getMcpResourceMetadataUrl?(): string | null;
87119
}

0 commit comments

Comments
 (0)