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
90 changes: 90 additions & 0 deletions .changeset/slot-lookups-return-their-contract.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
---
"@objectstack/spec": minor
"@objectstack/runtime": patch
---

fix(spec,runtime): a service-slot lookup returns the slot's contract, not `any` — and it immediately found two more gaps (#4127)

#4127's most valuable item was the one it did not do: "**给这个类别加个 gate**".
The four contract gaps it catalogued were found by a human sweeping the
dispatcher by hand. A sweep is not repeatable, and this one was not complete —
see `/auth` below.

The root was one line:

```ts
// domain-handler-registry.ts
getService(name: string): any; // ← every domain's service handle
```

Against `any`, a domain calling a method its contract declares and a domain
calling a method nobody declares typecheck identically. That is what let #4087
ship a `/storage` handler passing two arguments no implementation takes, and
what hid #4127's four.

**`CoreServiceContracts` — the slot → contract ledger.** `CoreServiceName` named
the slots and `contracts/*` described them; nothing connected the two. It does
now, and `getService<K>(name: K)` resolves through it, so a call outside the
contract is a **compile error at the call site**.

An entry is a claim, so entries are only made where the binding is evidenced —
by the provider that registers the slot (`service-storage` → `file-storage`,
`objectql` → `data`, whose own comment reads "ObjectQL implements IDataEngine"),
or by dispatcher work that proved it (#4143/#4150 for `automation`,
`notification`, `i18n`). **`ui` is deliberately unmapped**: the slot exists and
`domains/ui.ts` serves it, but no `IUiService` was ever written. An unmapped slot
resolves to `unknown`, not `any` — it must be cast deliberately, so the gap stays
legible instead of looking checked.

**Two findings, within minutes of turning it on:**

**`/auth` called a method that does not exist.** `domains/auth.ts` probed
`authService.handler(request, response)`. `IAuthService` declares
`handleRequest(request): Promise<Response>`; `AuthManager` implements exactly
that and has no `handler`. The probe was false on every deployment — #4143's dead
`automation.trigger` again. **#4127's manual sweep never mentions `/auth`**,
neither in its gap list nor in its "扫干净的" list: the file the compiler flagged
first is the one the human pass skipped entirely.

Not a live hole: the Hono adapter calls `handleRequest` itself and only falls
through to the dispatcher when no usable auth service answered, so nothing was
served by the mock in that deployment. But reading the contract makes the branch
reachable for the first time — a host calling `handleAuth` directly WITH an auth
service registered used to get `mockAuthFallback`'s `mock_<uuid>` session instead
of real authentication, and now gets the auth service.

**`POST /analytics/sql` invoked an optional method unguarded.** `generateSql?` is
optional on `IAnalyticsService` — unlike `query`/`getMeta` beside it — and the
call had no probe, so a provider without it answers a 500 from `TypeError`
instead of saying the capability is absent. service-analytics implements it,
which is why nothing noticed; the contract permits a provider that does not, and
this slot is multi-provider by design. It answers `handled: false` now, the same
404 the file's entry gate already gives for absent analytics capability.

**`isServiceServeable` is a type guard now** (`svc is NonNullable<T>`). Every
domain already calls it first on a resolved slot, so one predicate narrows away
the `undefined` for the whole handler body — the null check and the capability
check were always the same check.

**The test-side hole, closed for this batch.** #4127's last section predicted it:
the mocks are written to what the handler wants, so handler and test agree with
each other and with no implementation. **Three** tests across two files mocked
`{ handler }` for auth — including one whose entire subject was the *resolution
path*, so it proved the lookup worked and nothing about the call. `ContractMock<T>`
(`Partial<Record<keyof T, unknown>>`) now guards the mocks: keys are checked
against the contract, signatures deliberately left `unknown` so `vi.fn()` does not
force everything back to `as any`. The automation mock's `trigger` — genuinely
not on the contract — stays as an explicit, labelled negative control outside the
checked literal, because a test asserting the route *never* calls it is the point.

Nothing is renamed and no runtime behavior changes except the two fixes above.
The 12 domains not calling `getService` are untouched; `resolveService` (which
also takes non-`CoreServiceName` names like `protocol` and `objectql`) is
deliberately left for a later batch rather than widened here.

Verified: `@objectstack/runtime` **933 tests / 65 files**, `@objectstack/spec`
**7095 / 272** (6 new, pinning the map against the enum in both directions),
service-automation **457**, service-analytics **413**, service-messaging **137**,
service-i18n **62**, adapter-hono **73**; `tsc --noEmit` on spec, runtime,
downstream-contract and all four examples; `pnpm lint`; and all nine
`@objectstack/spec` `check:*` gates — clean.
13 changes: 9 additions & 4 deletions packages/runtime/src/domain-handler-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -626,11 +626,16 @@ describe('HttpDispatcher extracted domains (PR-6: automation)', () => {
// ---------------------------------------------------------------------------

describe('HttpDispatcher extracted domains (PR-7: auth/ai)', () => {
it('/auth delegates to the auth service handler when registered', async () => {
const handler = vi.fn().mockResolvedValue({ ok: true });
const result = await makeDispatcher({ auth: { handler } }).dispatch('POST', '/auth/sign-in/email', { email: 'x@y.z' }, {}, {} as any);
// [#4127] Third copy of the fabricated `handler` mock (with
// http-dispatcher.test.ts's two). Three tests across two files all agreed
// the auth domain calls `handler`; no auth service has one, and
// `IAuthService` declares `handleRequest`. That is how a dead branch stays
// green — every test was written from the handler, not from the contract.
it('/auth delegates to the auth service via the contract handleRequest when registered', async () => {
const handleRequest = vi.fn().mockResolvedValue({ ok: true });
const result = await makeDispatcher({ auth: { handleRequest } }).dispatch('POST', '/auth/sign-in/email', { email: 'x@y.z' }, {}, {} as any);
expect(result.handled).toBe(true);
expect(handler).toHaveBeenCalledTimes(1);
expect(handleRequest).toHaveBeenCalledTimes(1);
});

it('/auth mock fallback serves sign-up when no auth service is registered', async () => {
Expand Down
23 changes: 21 additions & 2 deletions packages/runtime/src/domain-handler-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
*/

import type { HttpProtocolContext, HttpDispatcherResult } from './http-dispatcher.js';
import type { CoreServiceName } from '@objectstack/spec/system';
import type { CoreServiceContract } from '@objectstack/spec/contracts';

/**
* The normalized request slice a domain handler receives. `path` is the
Expand Down Expand Up @@ -78,8 +80,25 @@ export interface DomainRoute {
export interface DomainHandlerDeps {
/** Environment-scoped service resolution (per-request kernel aware). */
resolveService(name: string, environmentId?: string): any;
/** Unscoped service lookup on the current kernel (may return a Promise). */
getService(name: string): any;
/**
* Unscoped service lookup on the current kernel, typed by the slot.
*
* [#4127] Returned `any`, which is why nothing could tell a domain calling
* a method its contract declares from one calling a method nobody declared:
* both typecheck against `any`. #4087 rode that for months (a `/storage`
* handler passing two arguments no implementation takes), and the four gaps
* in #4127 were found by sweeping the domains by hand — not repeatable.
*
* {@link CoreServiceContract} resolves the slot to its contract, so the
* compiler asks the question on every call. A slot with no contract written
* yet resolves to `unknown`, so it must be cast deliberately and the gap
* stays visible.
*
* `undefined` when the slot is empty — the caller MUST narrow before use
* (`isServiceServeable` does it and also rejects a self-declared
* non-handler, ADR-0076 D12).
*/
getService<K extends CoreServiceName>(name: K): Promise<CoreServiceContract<K> | undefined>;
/**
* Environment-scoped ObjectQL lookup with a registry-shape check
* (resolves the `objectql` service and returns it only when it exposes
Expand Down
12 changes: 12 additions & 0 deletions packages/runtime/src/domains/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,18 @@ export async function handleAnalyticsRequest(
if (subPath === 'sql' && m === 'POST') {
// [#3878] Same body contract as /query — validated the same way.
assertAnalyticsQueryBody(body);
// [#4127] `generateSql` is OPTIONAL on `IAnalyticsService` — unlike
// `query` / `getMeta` above, which are required — and this call had no
// guard, so a provider filling the slot without it answered a 500 from
// `TypeError: generateSql is not a function` instead of saying the
// capability is absent. service-analytics implements it, which is why
// nothing noticed; the contract permits a provider that does not, and
// the registry names this slot as multi-provider by design.
//
// `handled: false` is the file's own answer for absent analytics
// capability (the entry gate above), so an absent SUB-capability gets
// the same 404 rather than a new third shape.
if (typeof analyticsService.generateSql !== 'function') return { handled: false };
// [#2852] Scope the generated SQL to the caller too, so a preview
// reflects the same per-object read filter the real query applies.
const result = await analyticsService.generateSql(body, context?.executionContext);
Expand Down
25 changes: 22 additions & 3 deletions packages/runtime/src/domains/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,29 @@ export function createAuthDomain(deps: DomainHandlerDeps): DomainRoute {
* path: sub-path after /auth/
*/
export async function handleAuthRequest(deps: DomainHandlerDeps, path: string, method: string, body: any, context: HttpProtocolContext): Promise<HttpDispatcherResult> {
// 1. Try generic Auth Service
// 1. Try generic Auth Service.
//
// [#4127] This probed `authService.handler(request, response)` — a method
// no implementation has, taking two arguments the contract's does not.
// `IAuthService` declares `handleRequest(request): Promise<Response>` and
// `AuthManager` implements exactly that, so the probe was false on every
// deployment: #4087's shape, found by the compiler the moment `getService`
// started returning `IAuthService` instead of `any`. It survived the manual
// sweep in #4127, which never listed `/auth` in either its gap list or its
// "clean" list, and it was pinned GREEN by a test mocking `{ handler }` —
// the fabricated shape, not the declared one (the same test-side hole that
// kept #4087 green, catalogued in #4127's last section).
//
// Reading the contract also makes the branch reachable for the first time.
// The Hono adapter calls `handleRequest` itself and only falls through to
// this dispatcher when no usable auth service answered, so nothing was
// silently served by the mock below in that deployment — but a host that
// reaches `handleAuth` directly WITH an auth service registered used to get
// `mockAuthFallback`'s `mock_<uuid>` session instead of real authentication.
// It now gets the auth service.
const authService = await deps.getService(CoreServiceName.enum.auth);
if (authService && typeof authService.handler === 'function') {
const response = await authService.handler(context.request, context.response);
if (authService && typeof authService.handleRequest === 'function') {
const response = await authService.handleRequest(context.request as Request);
return { handled: true, result: response };
}

Expand Down
61 changes: 51 additions & 10 deletions packages/runtime/src/http-dispatcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,21 @@ import { HttpDispatcher } from './http-dispatcher.js';
import { ObjectKernel } from '@objectstack/core';
import { ApiErrorSchema } from '@objectstack/spec/api';
import type { ConnectorDescriptor } from '@objectstack/spec/integration';
import type { IAuthService, IAutomationService } from '@objectstack/spec/contracts';

/**
* [#4127] Mock-shape guard: every key must be a method the contract DECLARES.
*
* Signatures stay `unknown` on purpose — `vi.fn()` does not match a contract
* signature, and forcing it to would push these mocks straight back to `as any`,
* which is the state this is fixing. What it catches is the failure that keeps
* actually happening: a mock naming a method the contract does not have, so the
* handler and its test agree with each other and with no implementation.
* `upload(file, { request })` in #4087, `authService.handler` and
* `automation.trigger` here — each one sat green for months behind a mock
* written to the handler's wish rather than the declared surface.
*/
type ContractMock<T> = Partial<Record<keyof T, unknown>>;

describe('HttpDispatcher', () => {
let kernel: ObjectKernel;
Expand Down Expand Up @@ -162,7 +177,8 @@ describe('HttpDispatcher', () => {
let mockAutomationService: any;

beforeEach(() => {
mockAutomationService = {
// [#4127] Everything the CONTRACT declares, checked against it.
const contractMethods = {
listFlows: vi.fn().mockResolvedValue(['flow_a', 'flow_b']),
getFlow: vi.fn().mockResolvedValue({ name: 'flow_a', label: 'Flow A' }),
registerFlow: vi.fn(),
Expand All @@ -171,7 +187,6 @@ describe('HttpDispatcher', () => {
toggleFlow: vi.fn().mockResolvedValue(undefined),
listRuns: vi.fn().mockResolvedValue([{ id: 'run_1', status: 'completed' }]),
getRun: vi.fn().mockResolvedValue({ id: 'run_1', status: 'completed' }),
trigger: vi.fn().mockResolvedValue({ success: true }),
resume: vi.fn().mockResolvedValue({ success: true, output: {}, durationMs: 7 }),
// Sync per IAutomationService — `ScreenSpec | null`, not a promise.
getSuspendedScreen: vi.fn().mockReturnValue({ nodeId: 'collect', fields: [] }),
Expand All @@ -196,6 +211,17 @@ describe('HttpDispatcher', () => {
{ name: 'flow_a', enabled: true, bound: true },
{ name: 'flow_b', enabled: false, bound: false },
]),
} satisfies ContractMock<IAutomationService>;

mockAutomationService = {
...contractMethods,
// NEGATIVE CONTROL (#4143) — deliberately NOT on the contract.
// Nothing in the repo implements `trigger` on the automation
// slot; it exists here only so the legacy-route test below can
// assert it is never called. Kept outside the checked literal
// so it reads as the exception it is, instead of quietly
// re-opening the hole the check above closes.
trigger: vi.fn().mockResolvedValue({ success: true }),
};

// Set up kernel services to include automation
Expand Down Expand Up @@ -912,21 +938,30 @@ describe('HttpDispatcher', () => {
});

describe('handleAuth with async service', () => {
it('should resolve auth service from Promise', async () => {
// [#4127] This mocked `{ handler }` — a method `IAuthService` does
// not declare and `AuthManager` does not have — and asserted it was
// called, which is why the dead branch stayed green. Exactly the
// test-side hole that kept #4087 alive: the mock was written to the
// handler's fabricated shape instead of the declared contract. It
// pins `handleRequest` now, and `satisfies` makes a future drift
// back to an undeclared name a compile error rather than a passing
// test asserting a call nothing makes.
it('should resolve auth service from Promise and call the contract method', async () => {
const mockAuth = {
handler: vi.fn().mockResolvedValue({ user: { id: '1' } }),
};
handleRequest: vi.fn().mockResolvedValue({ user: { id: '1' } }),
verify: vi.fn().mockResolvedValue({ success: true }),
} satisfies Pick<IAuthService, 'handleRequest' | 'verify'>;
(kernel as any).getService = vi.fn().mockImplementation((name: string) => {
if (name === 'auth') return Promise.resolve(mockAuth);
return null;
});

const result = await dispatcher.handleAuth('', 'POST', {}, { request: {}, response: {} });
expect(result.handled).toBe(true);
expect(mockAuth.handler).toHaveBeenCalled();
expect(mockAuth.handleRequest).toHaveBeenCalled();
});

it('should fallback to mock auth when async auth service has no handler', async () => {
it('should fallback to mock auth when async auth service has no handleRequest', async () => {
(kernel as any).getService = vi.fn().mockResolvedValue({});

const result = await dispatcher.handleAuth('/login', 'POST', { email: 'test@example.com' }, { request: {} });
Expand Down Expand Up @@ -1136,17 +1171,23 @@ describe('HttpDispatcher', () => {
});

it('should prefer getServiceAsync over getService for auth', async () => {
// [#4127] Second copy of the same fabricated `handler` mock — this
// one asserted the resolution PATH (getServiceAsync over
// getService) while pinning a method no auth service has, so it
// proved the lookup worked and nothing about the call. The path
// assertion is the point of this test and is unchanged; the mock
// now names the contract method the handler actually invokes.
const asyncAuth = {
handler: vi.fn().mockResolvedValue({ user: { id: '1' } }),
};
handleRequest: vi.fn().mockResolvedValue({ user: { id: '1' } }),
} satisfies ContractMock<IAuthService>;
(kernel as any).getServiceAsync = vi.fn().mockResolvedValue(asyncAuth);
(kernel as any).getService = vi.fn().mockImplementation(() => {
throw new Error("Service 'auth' is async - use await");
});

const result = await dispatcher.handleAuth('', 'POST', {}, { request: {}, response: {} });
expect(result.handled).toBe(true);
expect(asyncAuth.handler).toHaveBeenCalled();
expect(asyncAuth.handleRequest).toHaveBeenCalled();
expect((kernel as any).getServiceAsync).toHaveBeenCalledWith('auth');
});

Expand Down
9 changes: 8 additions & 1 deletion packages/runtime/src/service-serveable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,15 @@ import { readServiceSelfInfo } from '@objectstack/spec/api';
* An empty slot and a slot filled by a self-declared stub are the same amount
* of capability, so callers answer both the same way — whatever their empty-slot
* exit already is (`handled: false` → 404, or an explicit 501).
*
* [#4127] A type guard, not a `boolean`. Every domain already calls this as its
* first act on a resolved slot, so once `getService` returns
* `Contract | undefined` this one predicate narrows away the `undefined` for the
* whole handler body — the null check and the capability check are the same
* check, and were always meant to be. Written as `svc is NonNullable<T>` so the
* caller keeps its own contract type instead of being widened to a shared one.
*/
export function isServiceServeable(svc: unknown): boolean {
export function isServiceServeable<T>(svc: T): svc is NonNullable<T> {
if (!svc) return false;
return readServiceSelfInfo(svc)?.handlerReady !== false;
}
Loading
Loading