Skip to content

Commit 75b9e51

Browse files
authored
fix(spec,runtime): a service-slot lookup returns the slot's contract, not any (#4127) (#4168)
#4127's most valuable item was the one it did not do: add a gate for the class. Its four contract gaps were found by a human sweeping the dispatcher by hand. A sweep is not repeatable, and this one was not complete. The root was one line — `getService(name: string): any` in domain-handler-registry.ts. 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` is 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 made only 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`, so it must be cast deliberately and the gap stays legible. 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)` and `AuthManager` implements exactly that, with no `handler`. False on every deployment — #4143's dead `automation.trigger` again. #4127's sweep never mentions `/auth` in either its gap list or its "clean" list: the file the compiler flagged first is the one the human pass skipped. Not a live hole — the Hono adapter calls `handleRequest` itself and only falls through when no usable auth service answered — but reading the contract makes the branch reachable for the first time, so a host calling `handleAuth` directly WITH an auth service now gets it instead of mockAuthFallback's `mock_<uuid>` session. - `POST /analytics/sql` invoked an optional method unguarded. `generateSql?` is optional on IAnalyticsService — unlike `query`/`getMeta` beside it — so a provider without it answered a 500 from TypeError instead of saying the capability is absent. Answers `handled: false` now, the same 404 the entry gate already gives for absent analytics capability. `isServiceServeable` becomes a type guard (`svc is NonNullable<T>`). Every domain already calls it first on a resolved slot, so one predicate narrows away the `undefined` for the whole body — the null check and the capability check were always the same check. The test-side hole #4127 predicted, closed for this batch: THREE tests across two files mocked `{ handler }` for auth, including one whose subject was the resolution path, so it proved the lookup worked and nothing about the call. `ContractMock<T>` guards mock keys against the contract; signatures stay `unknown` so vi.fn() does not force everything back to `as any`. The automation mock's `trigger` stays as a labelled negative control outside the checked literal — a test asserting the route never calls it is the point. The 12 domains not calling `getService` are untouched. `resolveService`, which also takes non-CoreServiceName names like `protocol` and `objectql`, is left for a later batch rather than widened here. Refs #4127
1 parent e8d0c21 commit 75b9e51

11 files changed

Lines changed: 369 additions & 20 deletions
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/runtime": patch
4+
---
5+
6+
fix(spec,runtime): a service-slot lookup returns the slot's contract, not `any` — and it immediately found two more gaps (#4127)
7+
8+
#4127's most valuable item was the one it did not do: "**给这个类别加个 gate**".
9+
The four contract gaps it catalogued were found by a human sweeping the
10+
dispatcher by hand. A sweep is not repeatable, and this one was not complete —
11+
see `/auth` below.
12+
13+
The root was one line:
14+
15+
```ts
16+
// domain-handler-registry.ts
17+
getService(name: string): any; // ← every domain's service handle
18+
```
19+
20+
Against `any`, a domain calling a method its contract declares and a domain
21+
calling a method nobody declares typecheck identically. That is what let #4087
22+
ship a `/storage` handler passing two arguments no implementation takes, and
23+
what hid #4127's four.
24+
25+
**`CoreServiceContracts` — the slot → contract ledger.** `CoreServiceName` named
26+
the slots and `contracts/*` described them; nothing connected the two. It does
27+
now, and `getService<K>(name: K)` resolves through it, so a call outside the
28+
contract is a **compile error at the call site**.
29+
30+
An entry is a claim, so entries are only made where the binding is evidenced —
31+
by the provider that registers the slot (`service-storage``file-storage`,
32+
`objectql``data`, whose own comment reads "ObjectQL implements IDataEngine"),
33+
or by dispatcher work that proved it (#4143/#4150 for `automation`,
34+
`notification`, `i18n`). **`ui` is deliberately unmapped**: the slot exists and
35+
`domains/ui.ts` serves it, but no `IUiService` was ever written. An unmapped slot
36+
resolves to `unknown`, not `any` — it must be cast deliberately, so the gap stays
37+
legible instead of looking checked.
38+
39+
**Two findings, within minutes of turning it on:**
40+
41+
**`/auth` called a method that does not exist.** `domains/auth.ts` probed
42+
`authService.handler(request, response)`. `IAuthService` declares
43+
`handleRequest(request): Promise<Response>`; `AuthManager` implements exactly
44+
that and has no `handler`. The probe was false on every deployment — #4143's dead
45+
`automation.trigger` again. **#4127's manual sweep never mentions `/auth`**,
46+
neither in its gap list nor in its "扫干净的" list: the file the compiler flagged
47+
first is the one the human pass skipped entirely.
48+
49+
Not a live hole: the Hono adapter calls `handleRequest` itself and only falls
50+
through to the dispatcher when no usable auth service answered, so nothing was
51+
served by the mock in that deployment. But reading the contract makes the branch
52+
reachable for the first time — a host calling `handleAuth` directly WITH an auth
53+
service registered used to get `mockAuthFallback`'s `mock_<uuid>` session instead
54+
of real authentication, and now gets the auth service.
55+
56+
**`POST /analytics/sql` invoked an optional method unguarded.** `generateSql?` is
57+
optional on `IAnalyticsService` — unlike `query`/`getMeta` beside it — and the
58+
call had no probe, so a provider without it answers a 500 from `TypeError`
59+
instead of saying the capability is absent. service-analytics implements it,
60+
which is why nothing noticed; the contract permits a provider that does not, and
61+
this slot is multi-provider by design. It answers `handled: false` now, the same
62+
404 the file's entry gate already gives for absent analytics capability.
63+
64+
**`isServiceServeable` is a type guard now** (`svc is NonNullable<T>`). Every
65+
domain already calls it first on a resolved slot, so one predicate narrows away
66+
the `undefined` for the whole handler body — the null check and the capability
67+
check were always the same check.
68+
69+
**The test-side hole, closed for this batch.** #4127's last section predicted it:
70+
the mocks are written to what the handler wants, so handler and test agree with
71+
each other and with no implementation. **Three** tests across two files mocked
72+
`{ handler }` for auth — including one whose entire subject was the *resolution
73+
path*, so it proved the lookup worked and nothing about the call. `ContractMock<T>`
74+
(`Partial<Record<keyof T, unknown>>`) now guards the mocks: keys are checked
75+
against the contract, signatures deliberately left `unknown` so `vi.fn()` does not
76+
force everything back to `as any`. The automation mock's `trigger` — genuinely
77+
not on the contract — stays as an explicit, labelled negative control outside the
78+
checked literal, because a test asserting the route *never* calls it is the point.
79+
80+
Nothing is renamed and no runtime behavior changes except the two fixes above.
81+
The 12 domains not calling `getService` are untouched; `resolveService` (which
82+
also takes non-`CoreServiceName` names like `protocol` and `objectql`) is
83+
deliberately left for a later batch rather than widened here.
84+
85+
Verified: `@objectstack/runtime` **933 tests / 65 files**, `@objectstack/spec`
86+
**7095 / 272** (6 new, pinning the map against the enum in both directions),
87+
service-automation **457**, service-analytics **413**, service-messaging **137**,
88+
service-i18n **62**, adapter-hono **73**; `tsc --noEmit` on spec, runtime,
89+
downstream-contract and all four examples; `pnpm lint`; and all nine
90+
`@objectstack/spec` `check:*` gates — clean.

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -626,11 +626,16 @@ describe('HttpDispatcher extracted domains (PR-6: automation)', () => {
626626
// ---------------------------------------------------------------------------
627627

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

636641
it('/auth mock fallback serves sign-up when no auth service is registered', async () => {

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

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
*/
3535

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

3840
/**
3941
* The normalized request slice a domain handler receives. `path` is the
@@ -78,8 +80,25 @@ export interface DomainRoute {
7880
export interface DomainHandlerDeps {
7981
/** Environment-scoped service resolution (per-request kernel aware). */
8082
resolveService(name: string, environmentId?: string): any;
81-
/** Unscoped service lookup on the current kernel (may return a Promise). */
82-
getService(name: string): any;
83+
/**
84+
* Unscoped service lookup on the current kernel, typed by the slot.
85+
*
86+
* [#4127] Returned `any`, which is why nothing could tell a domain calling
87+
* a method its contract declares from one calling a method nobody declared:
88+
* both typecheck against `any`. #4087 rode that for months (a `/storage`
89+
* handler passing two arguments no implementation takes), and the four gaps
90+
* in #4127 were found by sweeping the domains by hand — not repeatable.
91+
*
92+
* {@link CoreServiceContract} resolves the slot to its contract, so the
93+
* compiler asks the question on every call. A slot with no contract written
94+
* yet resolves to `unknown`, so it must be cast deliberately and the gap
95+
* stays visible.
96+
*
97+
* `undefined` when the slot is empty — the caller MUST narrow before use
98+
* (`isServiceServeable` does it and also rejects a self-declared
99+
* non-handler, ADR-0076 D12).
100+
*/
101+
getService<K extends CoreServiceName>(name: K): Promise<CoreServiceContract<K> | undefined>;
83102
/**
84103
* Environment-scoped ObjectQL lookup with a registry-shape check
85104
* (resolves the `objectql` service and returns it only when it exposes

packages/runtime/src/domains/analytics.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,18 @@ export async function handleAnalyticsRequest(
127127
if (subPath === 'sql' && m === 'POST') {
128128
// [#3878] Same body contract as /query — validated the same way.
129129
assertAnalyticsQueryBody(body);
130+
// [#4127] `generateSql` is OPTIONAL on `IAnalyticsService` — unlike
131+
// `query` / `getMeta` above, which are required — and this call had no
132+
// guard, so a provider filling the slot without it answered a 500 from
133+
// `TypeError: generateSql is not a function` instead of saying the
134+
// capability is absent. service-analytics implements it, which is why
135+
// nothing noticed; the contract permits a provider that does not, and
136+
// the registry names this slot as multi-provider by design.
137+
//
138+
// `handled: false` is the file's own answer for absent analytics
139+
// capability (the entry gate above), so an absent SUB-capability gets
140+
// the same 404 rather than a new third shape.
141+
if (typeof analyticsService.generateSql !== 'function') return { handled: false };
130142
// [#2852] Scope the generated SQL to the caller too, so a preview
131143
// reflects the same per-object read filter the real query applies.
132144
const result = await analyticsService.generateSql(body, context?.executionContext);

packages/runtime/src/domains/auth.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,29 @@ export function createAuthDomain(deps: DomainHandlerDeps): DomainRoute {
5151
* path: sub-path after /auth/
5252
*/
5353
export async function handleAuthRequest(deps: DomainHandlerDeps, path: string, method: string, body: any, context: HttpProtocolContext): Promise<HttpDispatcherResult> {
54-
// 1. Try generic Auth Service
54+
// 1. Try generic Auth Service.
55+
//
56+
// [#4127] This probed `authService.handler(request, response)` — a method
57+
// no implementation has, taking two arguments the contract's does not.
58+
// `IAuthService` declares `handleRequest(request): Promise<Response>` and
59+
// `AuthManager` implements exactly that, so the probe was false on every
60+
// deployment: #4087's shape, found by the compiler the moment `getService`
61+
// started returning `IAuthService` instead of `any`. It survived the manual
62+
// sweep in #4127, which never listed `/auth` in either its gap list or its
63+
// "clean" list, and it was pinned GREEN by a test mocking `{ handler }` —
64+
// the fabricated shape, not the declared one (the same test-side hole that
65+
// kept #4087 green, catalogued in #4127's last section).
66+
//
67+
// Reading the contract also makes the branch reachable for the first time.
68+
// The Hono adapter calls `handleRequest` itself and only falls through to
69+
// this dispatcher when no usable auth service answered, so nothing was
70+
// silently served by the mock below in that deployment — but a host that
71+
// reaches `handleAuth` directly WITH an auth service registered used to get
72+
// `mockAuthFallback`'s `mock_<uuid>` session instead of real authentication.
73+
// It now gets the auth service.
5574
const authService = await deps.getService(CoreServiceName.enum.auth);
56-
if (authService && typeof authService.handler === 'function') {
57-
const response = await authService.handler(context.request, context.response);
75+
if (authService && typeof authService.handleRequest === 'function') {
76+
const response = await authService.handleRequest(context.request as Request);
5877
return { handled: true, result: response };
5978
}
6079

packages/runtime/src/http-dispatcher.test.ts

Lines changed: 51 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,21 @@ import { HttpDispatcher } from './http-dispatcher.js';
44
import { ObjectKernel } from '@objectstack/core';
55
import { ApiErrorSchema } from '@objectstack/spec/api';
66
import type { ConnectorDescriptor } from '@objectstack/spec/integration';
7+
import type { IAuthService, IAutomationService } from '@objectstack/spec/contracts';
8+
9+
/**
10+
* [#4127] Mock-shape guard: every key must be a method the contract DECLARES.
11+
*
12+
* Signatures stay `unknown` on purpose — `vi.fn()` does not match a contract
13+
* signature, and forcing it to would push these mocks straight back to `as any`,
14+
* which is the state this is fixing. What it catches is the failure that keeps
15+
* actually happening: a mock naming a method the contract does not have, so the
16+
* handler and its test agree with each other and with no implementation.
17+
* `upload(file, { request })` in #4087, `authService.handler` and
18+
* `automation.trigger` here — each one sat green for months behind a mock
19+
* written to the handler's wish rather than the declared surface.
20+
*/
21+
type ContractMock<T> = Partial<Record<keyof T, unknown>>;
722

823
describe('HttpDispatcher', () => {
924
let kernel: ObjectKernel;
@@ -162,7 +177,8 @@ describe('HttpDispatcher', () => {
162177
let mockAutomationService: any;
163178

164179
beforeEach(() => {
165-
mockAutomationService = {
180+
// [#4127] Everything the CONTRACT declares, checked against it.
181+
const contractMethods = {
166182
listFlows: vi.fn().mockResolvedValue(['flow_a', 'flow_b']),
167183
getFlow: vi.fn().mockResolvedValue({ name: 'flow_a', label: 'Flow A' }),
168184
registerFlow: vi.fn(),
@@ -171,7 +187,6 @@ describe('HttpDispatcher', () => {
171187
toggleFlow: vi.fn().mockResolvedValue(undefined),
172188
listRuns: vi.fn().mockResolvedValue([{ id: 'run_1', status: 'completed' }]),
173189
getRun: vi.fn().mockResolvedValue({ id: 'run_1', status: 'completed' }),
174-
trigger: vi.fn().mockResolvedValue({ success: true }),
175190
resume: vi.fn().mockResolvedValue({ success: true, output: {}, durationMs: 7 }),
176191
// Sync per IAutomationService — `ScreenSpec | null`, not a promise.
177192
getSuspendedScreen: vi.fn().mockReturnValue({ nodeId: 'collect', fields: [] }),
@@ -196,6 +211,17 @@ describe('HttpDispatcher', () => {
196211
{ name: 'flow_a', enabled: true, bound: true },
197212
{ name: 'flow_b', enabled: false, bound: false },
198213
]),
214+
} satisfies ContractMock<IAutomationService>;
215+
216+
mockAutomationService = {
217+
...contractMethods,
218+
// NEGATIVE CONTROL (#4143) — deliberately NOT on the contract.
219+
// Nothing in the repo implements `trigger` on the automation
220+
// slot; it exists here only so the legacy-route test below can
221+
// assert it is never called. Kept outside the checked literal
222+
// so it reads as the exception it is, instead of quietly
223+
// re-opening the hole the check above closes.
224+
trigger: vi.fn().mockResolvedValue({ success: true }),
199225
};
200226

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

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

924959
const result = await dispatcher.handleAuth('', 'POST', {}, { request: {}, response: {} });
925960
expect(result.handled).toBe(true);
926-
expect(mockAuth.handler).toHaveBeenCalled();
961+
expect(mockAuth.handleRequest).toHaveBeenCalled();
927962
});
928963

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

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

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

11471188
const result = await dispatcher.handleAuth('', 'POST', {}, { request: {}, response: {} });
11481189
expect(result.handled).toBe(true);
1149-
expect(asyncAuth.handler).toHaveBeenCalled();
1190+
expect(asyncAuth.handleRequest).toHaveBeenCalled();
11501191
expect((kernel as any).getServiceAsync).toHaveBeenCalledWith('auth');
11511192
});
11521193

packages/runtime/src/service-serveable.ts

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

0 commit comments

Comments
 (0)