Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions .changeset/resolve-service-returns-its-contract.md
Original file line number Diff line number Diff line change
@@ -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.
21 changes: 20 additions & 1 deletion packages/runtime/src/domain-handler-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<K extends CoreServiceName>(name: K, environmentId?: string): Promise<CoreServiceContract<K> | undefined>;
resolveService(name: string, environmentId?: string): any;
/**
* Unscoped service lookup on the current kernel, typed by the slot.
Expand Down
11 changes: 9 additions & 2 deletions packages/runtime/src/domains/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */ }
Expand Down Expand Up @@ -242,7 +248,8 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str
*/
async function getMcpResourceMetadataUrl(deps: DomainHandlerDeps, context: HttpProtocolContext): Promise<string | null> {
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 {
Expand Down
6 changes: 5 additions & 1 deletion packages/runtime/src/domains/packages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' };
Expand Down
11 changes: 7 additions & 4 deletions packages/runtime/src/http-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpDispatcher['getService']>[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<HttpDispatcher['getService']>[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.
Expand Down
43 changes: 43 additions & 0 deletions packages/spec/src/contracts/auth-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
32 changes: 32 additions & 0 deletions packages/spec/src/contracts/auth-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,36 @@ export interface IAuthService {
* @returns Authenticated user or undefined
*/
getCurrentUser?(request: Request): Promise<AuthUser | undefined>;

/**
* The MCP resource identifier (RFC 8707 `resource` / token `aud`) —
* `<origin><apiPrefix>/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;
}
Loading