Skip to content

Commit d5c75e2

Browse files
authored
fix(spec,runtime,service-i18n): the dispatcher domains and their service contracts describe the same surface (#4127) (#4143)
#4087 retired a `/storage` bridge that called `upload(key, data, options?)` as `upload(file, { request })`. Sweeping the other dispatcher domains against `packages/spec/src/contracts/*` found the mirror-image gap in three places: the call site and the implementation agreed, and the CONTRACT was the thing nobody had written. Each was worked around with `typeof x.foo === 'function'` — a duck-type is what "the contract does not cover this" looks like when nobody fixes the contract. Fixed at the contract (Prime Directive #12). Contracts: - `INotificationService` += `listInbox?` / `markRead?` / `markAllRead?` and `InboxQuery` / `InboxNotification` / `InboxListResult` / `MarkReadResult`. Three SDK-expressed routes rested on them, implemented by service-messaging, while the contract described only `send`. The dev stub implements exactly `send`/`sendBatch` BECAUSE it followed the contract — so the one implementation written to spec was the one the domain had to duck-type past. Optional, because a send-only provider (SMTP, Twilio, a Slack webhook) fills the slot legitimately without an inbox — a fact `handlerReady` cannot express, since the slot is serveable and only this capability is absent. - `II18nService` += `getFieldLabels?`. Both serving surfaces probed for it and both documented it as "optional on II18nService", which was untrue until now. - `IAutomationService` += `getFlowRuntimeStates?` and `FlowRuntimeState`. The dispatcher's inline cast declared `{ name, enabled, bound }` — a third copy of the shape, narrower than the engine returns, dropping the `status` / `triggerType` / `object` fields that say WHY a flow is unbound. Two runtime defects fell out of the same sweep, both on the legacy trigger route — the one `client.automation.trigger()` calls: - It passed the raw HTTP body to `execute(name, body)`, so the `{recordId, objectName, params}` translation never ran AND no caller identity was forwarded. A flow's default `runAs` is `'user'`, and a `runAs:'user'` run whose trigger resolved no user has its data operations REFUSED (#3760, fail-closed) — so that SDK method could not run a data-touching flow at all, while `POST /:name/trigger` could. service-automation's own comment claims "most trigger surfaces (REST action / trigger endpoint) already resolve the full envelope"; for this endpoint it was not. Both routes share one context builder now. - The `automationService.trigger(...)` probe it tried first is deleted. Nothing in the repo has ever implemented `trigger` on the automation slot and the contract never declared it, so the branch was unreachable everywhere and its `execute` "fallback" was the route. Declaring `trigger?` would have blessed a second name for `execute`. Call sites read the contract now instead of `as any` / inline re-declarations, so the next deviation is a compile error. service-i18n's probe loses two casts with it — one through `Record<string, unknown>`, one restating the signature; its build failed the moment the method became declared, which is the point. The test that pinned the dead `trigger` branch asserted `trigger` was called with three arguments no implementation takes — the same shape that kept #4087 green for months. It now pins the contract method and the translated context. Not included: `getConnectorDescriptors`, the fourth gap in #4127. It needs the `ConnectorDescriptor` / `ConnectorActionDescriptor` / `ConnectorOrigin` / `ConnectorState` cluster promoted from service-automation into `packages/spec`, which is its own change. Refs #4127
1 parent 8c711fb commit d5c75e2

12 files changed

Lines changed: 566 additions & 76 deletions
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/runtime": patch
4+
"@objectstack/service-i18n": patch
5+
---
6+
7+
fix(spec,runtime,service-i18n): the dispatcher domains and their service contracts describe the same surface (#4127)
8+
9+
#4087 retired a `/storage` bridge that called `upload(key, data, options?)` as
10+
`upload(file, { request })` — a shape no implementation has. Sweeping the other
11+
dispatcher domains against `packages/spec/src/contracts/*` found the mirror-image
12+
gap in three places: the call site and the implementation agreed, and the
13+
**contract** was the thing that had never been written down. Each one was worked
14+
around at the call site with `typeof x.foo === 'function'` — a duck-type is what
15+
"the contract does not cover this" looks like when nobody fixes the contract.
16+
17+
Fixed at the contract, per Prime Directive #12.
18+
19+
**`INotificationService` — the inbox half.** `listInbox` / `markRead` /
20+
`markAllRead` now exist, with `InboxQuery` / `InboxNotification` /
21+
`InboxListResult` / `MarkReadResult`. Three SDK-expressed routes
22+
(`notifications.list` / `.markRead` / `.markAllRead`) have rested on them all
23+
along, implemented by `service-messaging`, while this contract described only
24+
`send`. The cost was not theoretical: the dev notification stub implements
25+
exactly `send` and `sendBatch` **because it followed the contract**, so the one
26+
implementation written to spec was the one the dispatcher had to duck-type past.
27+
28+
They are optional, and the probe stays: an inbox needs a durable store, and a
29+
send-only provider (SMTP, Twilio, a Slack webhook) fills the slot legitimately
30+
without one. `handlerReady` cannot express that — the slot is serveable, one
31+
capability of it is absent. The `/notifications` domain now takes
32+
`INotificationService` instead of `as any`, and each write route probes its own
33+
method rather than riding the entry `listInbox` check (they are separately
34+
optional, so "has an inbox to read" never implied "has read-state to write").
35+
36+
**`II18nService.getFieldLabels`.** Both serving surfaces — the dispatcher's
37+
`/i18n/labels/:object/:locale` and service-i18n's own mount — probed for it and
38+
both documented it as "optional on `II18nService`", which was not true. It is
39+
now. service-i18n's probe loses two casts with it (one through
40+
`Record<string, unknown>`, one re-declaring the signature inline).
41+
42+
**`IAutomationService.getFlowRuntimeStates`** + the `FlowRuntimeState` type.
43+
`GET /automation/_status` (and the CLI boot summary, and the
44+
`kernel:bootstrapped` audit) already called it while the contract stopped at
45+
`listFlows(): string[]`. The dispatcher's inline cast declared it as
46+
`{ name, enabled, bound }` — a third copy of the shape and a narrower one than
47+
the engine returns, dropping the `status` / `triggerType` / `object` fields that
48+
say WHY a flow is unbound.
49+
50+
Two runtime fixes fell out of the same sweep:
51+
52+
- **`POST /automation/trigger/:name` now builds a real `AutomationContext`.**
53+
It passed the raw HTTP body to `execute(name, body)`, so the
54+
`{ recordId, objectName, params }` translation never ran and — the sharper
55+
half — no caller identity was forwarded. A flow's default `runAs` is `'user'`,
56+
and a `runAs:'user'` run whose trigger resolved no user has its data
57+
operations REFUSED (#3760, fail-closed), so `client.automation.trigger()`
58+
could not run a data-touching flow at all while `POST /:name/trigger` could.
59+
service-automation's own comment claims "most trigger surfaces (REST action /
60+
trigger endpoint) already resolve the full envelope"; for this endpoint it was
61+
not true. Both routes share one context builder now.
62+
- **The dead `automationService.trigger(...)` probe is gone.** Nothing in the
63+
repo has ever implemented `trigger` on the automation slot and the contract
64+
never declared it, so the branch was unreachable on every deployment and its
65+
`execute` "fallback" was the route. Declaring `trigger?` would have blessed a
66+
second name for `execute`; the dead branch is deleted instead.
67+
68+
No migration. Every added contract member is optional, so existing
69+
implementations stay valid; the two runtime fixes only make routes that were
70+
failing or degraded behave like their working twins.

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,99 @@ describe('HttpDispatcher extracted domains (PR-6: automation)', () => {
526526
const result = await makeDispatcher().dispatch('GET', '/automation', undefined, {}, {} as any);
527527
expect(result.response?.status ?? 404).not.toBe(200);
528528
});
529+
530+
/**
531+
* [#4127] Both trigger routes build the SAME AutomationContext.
532+
*
533+
* `POST /trigger/:name` — the legacy shape, and the one
534+
* `client.automation.trigger()` calls — used to pass the raw HTTP body to
535+
* `execute(name, body)`: no `{recordId, objectName, params}` translation
536+
* and, worse, no caller identity. A flow's default `runAs` is `'user'`, and
537+
* a `runAs:'user'` run whose trigger resolved no user has its data ops
538+
* REFUSED (#3760), so the SDK method could not run a data-touching flow at
539+
* all while `POST /:name/trigger` could.
540+
*/
541+
it('both trigger routes translate the body and forward the caller identity', async () => {
542+
const execute = vi.fn().mockResolvedValue({ success: true });
543+
const automation = { execute, listFlows: vi.fn(), getFlow: vi.fn() };
544+
const ctx: any = {
545+
executionContext: {
546+
userId: 'u-1',
547+
positions: ['sales_rep'],
548+
permissions: ['lead.read'],
549+
tenantId: 't-1',
550+
},
551+
};
552+
const body = { recordId: 'lead-9', objectName: 'sales_lead', extra: 'kept' };
553+
554+
// Direct delegate calls — `dispatch()` would re-resolve identity off
555+
// the auth-less mock kernel and overwrite the seeded executionContext,
556+
// the same reason the `/keys` test above bypasses it.
557+
const dispatcher = makeDispatcher({ automation });
558+
await dispatcher.handleAutomation('/trigger/nurture', 'POST', body, ctx);
559+
await dispatcher.handleAutomation('/nurture/trigger', 'POST', body, ctx);
560+
561+
expect(execute).toHaveBeenCalledTimes(2);
562+
const [legacyName, legacyCtx] = execute.mock.calls[0];
563+
const [modernName, modernCtx] = execute.mock.calls[1];
564+
expect(legacyName).toBe('nurture');
565+
expect(modernName).toBe('nurture');
566+
// Same context out of both routes — that is the whole point.
567+
expect(legacyCtx).toEqual(modernCtx);
568+
// Body translation: recordId reaches params, aliased by object name,
569+
// and an unwrapped top-level key survives.
570+
expect(legacyCtx.object).toBe('sales_lead');
571+
expect(legacyCtx.params.recordId).toBe('lead-9');
572+
expect(legacyCtx.params.salesLeadId).toBe('lead-9');
573+
expect(legacyCtx.params.extra).toBe('kept');
574+
// Identity envelope (#1888): not just the user id.
575+
expect(legacyCtx.userId).toBe('u-1');
576+
expect(legacyCtx.positions).toEqual(['sales_rep']);
577+
expect(legacyCtx.permissions).toEqual(['lead.read']);
578+
expect(legacyCtx.tenantId).toBe('t-1');
579+
});
580+
581+
/**
582+
* [#4127] `trigger` is not a method of the automation slot — nothing in
583+
* the repo implements it and `IAutomationService` never declared it, so
584+
* the probe that used to precede the `execute` fallback was dead on every
585+
* deployment. A service that grows one must not be preferred over the
586+
* contract method.
587+
*/
588+
it('never calls a non-contract `trigger` method, even when one exists', async () => {
589+
const trigger = vi.fn().mockResolvedValue({ success: true });
590+
const execute = vi.fn().mockResolvedValue({ success: true });
591+
const automation = { trigger, execute, listFlows: vi.fn(), getFlow: vi.fn() };
592+
593+
const result = await makeDispatcher({ automation })
594+
.dispatch('POST', '/automation/trigger/nurture', {}, {}, {} as any);
595+
596+
expect(result.response?.status).toBe(200);
597+
expect(trigger).not.toHaveBeenCalled();
598+
expect(execute).toHaveBeenCalledTimes(1);
599+
});
600+
601+
/**
602+
* [#4127] `getFlowRuntimeStates` is declared on `IAutomationService` now,
603+
* and `/automation/_status` reads it through the contract type instead of
604+
* an inline cast that omitted `status` / `triggerType` / `object` — the
605+
* three fields that say WHY a flow is unbound.
606+
*/
607+
it('/automation/_status passes through the full FlowRuntimeState shape', async () => {
608+
const automation = {
609+
listFlows: vi.fn(),
610+
getFlow: vi.fn(),
611+
getFlowRuntimeStates: vi.fn().mockReturnValue([
612+
{ name: 'nurture', enabled: true, bound: false, status: 'active', triggerType: 'on_create', object: 'sales_lead' },
613+
]),
614+
};
615+
const result = await makeDispatcher({ automation }).dispatch('GET', '/automation/_status', undefined, {}, {} as any);
616+
expect(result.response?.status).toBe(200);
617+
expect(result.response?.body?.data?.flows?.[0]).toEqual({
618+
name: 'nurture', enabled: true, bound: false,
619+
status: 'active', triggerType: 'on_create', object: 'sales_lead',
620+
});
621+
});
529622
});
530623

531624
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)