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
70 changes: 70 additions & 0 deletions .changeset/dispatcher-call-sites-meet-their-contracts.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
---
"@objectstack/spec": minor
"@objectstack/runtime": patch
"@objectstack/service-i18n": patch
---

fix(spec,runtime,service-i18n): the dispatcher domains and their service contracts describe the same surface (#4127)

#4087 retired a `/storage` bridge that called `upload(key, data, options?)` as
`upload(file, { request })` — a shape no implementation has. 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 that had never been written down. Each one was worked
around at the call site 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, per Prime Directive #12.

**`INotificationService` — the inbox half.** `listInbox` / `markRead` /
`markAllRead` now exist, with `InboxQuery` / `InboxNotification` /
`InboxListResult` / `MarkReadResult`. Three SDK-expressed routes
(`notifications.list` / `.markRead` / `.markAllRead`) have rested on them all
along, implemented by `service-messaging`, while this contract described only
`send`. The cost was not theoretical: the dev notification stub implements
exactly `send` and `sendBatch` **because it followed the contract**, so the one
implementation written to spec was the one the dispatcher had to duck-type past.

They are optional, and the probe stays: an inbox needs a durable store, and a
send-only provider (SMTP, Twilio, a Slack webhook) fills the slot legitimately
without one. `handlerReady` cannot express that — the slot is serveable, one
capability of it is absent. The `/notifications` domain now takes
`INotificationService` instead of `as any`, and each write route probes its own
method rather than riding the entry `listInbox` check (they are separately
optional, so "has an inbox to read" never implied "has read-state to write").

**`II18nService.getFieldLabels`.** Both serving surfaces — the dispatcher's
`/i18n/labels/:object/:locale` and service-i18n's own mount — probed for it and
both documented it as "optional on `II18nService`", which was not true. It is
now. service-i18n's probe loses two casts with it (one through
`Record<string, unknown>`, one re-declaring the signature inline).

**`IAutomationService.getFlowRuntimeStates`** + the `FlowRuntimeState` type.
`GET /automation/_status` (and the CLI boot summary, and the
`kernel:bootstrapped` audit) already called it while the contract stopped at
`listFlows(): string[]`. The dispatcher's inline cast declared it as
`{ name, enabled, bound }` — a third copy of the shape and a narrower one than
the engine returns, dropping the `status` / `triggerType` / `object` fields that
say WHY a flow is unbound.

Two runtime fixes fell out of the same sweep:

- **`POST /automation/trigger/:name` now builds a real `AutomationContext`.**
It passed the raw HTTP body to `execute(name, body)`, so the
`{ recordId, objectName, params }` translation never ran and — the sharper
half — 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 `client.automation.trigger()`
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 true. Both routes share one context builder now.
- **The dead `automationService.trigger(...)` probe is gone.** Nothing in the
repo has ever implemented `trigger` on the automation slot and the contract
never declared it, so the branch was unreachable on every deployment and its
`execute` "fallback" was the route. Declaring `trigger?` would have blessed a
second name for `execute`; the dead branch is deleted instead.

No migration. Every added contract member is optional, so existing
implementations stay valid; the two runtime fixes only make routes that were
failing or degraded behave like their working twins.
93 changes: 93 additions & 0 deletions packages/runtime/src/domain-handler-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,99 @@ describe('HttpDispatcher extracted domains (PR-6: automation)', () => {
const result = await makeDispatcher().dispatch('GET', '/automation', undefined, {}, {} as any);
expect(result.response?.status ?? 404).not.toBe(200);
});

/**
* [#4127] Both trigger routes build the SAME AutomationContext.
*
* `POST /trigger/:name` — the legacy shape, and the one
* `client.automation.trigger()` calls — used to pass the raw HTTP body to
* `execute(name, body)`: no `{recordId, objectName, params}` translation
* and, worse, no caller identity. A flow's default `runAs` is `'user'`, and
* a `runAs:'user'` run whose trigger resolved no user has its data ops
* REFUSED (#3760), so the SDK method could not run a data-touching flow at
* all while `POST /:name/trigger` could.
*/
it('both trigger routes translate the body and forward the caller identity', async () => {
const execute = vi.fn().mockResolvedValue({ success: true });
const automation = { execute, listFlows: vi.fn(), getFlow: vi.fn() };
const ctx: any = {
executionContext: {
userId: 'u-1',
positions: ['sales_rep'],
permissions: ['lead.read'],
tenantId: 't-1',
},
};
const body = { recordId: 'lead-9', objectName: 'sales_lead', extra: 'kept' };

// Direct delegate calls — `dispatch()` would re-resolve identity off
// the auth-less mock kernel and overwrite the seeded executionContext,
// the same reason the `/keys` test above bypasses it.
const dispatcher = makeDispatcher({ automation });
await dispatcher.handleAutomation('/trigger/nurture', 'POST', body, ctx);
await dispatcher.handleAutomation('/nurture/trigger', 'POST', body, ctx);

expect(execute).toHaveBeenCalledTimes(2);
const [legacyName, legacyCtx] = execute.mock.calls[0];
const [modernName, modernCtx] = execute.mock.calls[1];
expect(legacyName).toBe('nurture');
expect(modernName).toBe('nurture');
// Same context out of both routes — that is the whole point.
expect(legacyCtx).toEqual(modernCtx);
// Body translation: recordId reaches params, aliased by object name,
// and an unwrapped top-level key survives.
expect(legacyCtx.object).toBe('sales_lead');
expect(legacyCtx.params.recordId).toBe('lead-9');
expect(legacyCtx.params.salesLeadId).toBe('lead-9');
expect(legacyCtx.params.extra).toBe('kept');
// Identity envelope (#1888): not just the user id.
expect(legacyCtx.userId).toBe('u-1');
expect(legacyCtx.positions).toEqual(['sales_rep']);
expect(legacyCtx.permissions).toEqual(['lead.read']);
expect(legacyCtx.tenantId).toBe('t-1');
});

/**
* [#4127] `trigger` is not a method of the automation slot — nothing in
* the repo implements it and `IAutomationService` never declared it, so
* the probe that used to precede the `execute` fallback was dead on every
* deployment. A service that grows one must not be preferred over the
* contract method.
*/
it('never calls a non-contract `trigger` method, even when one exists', async () => {
const trigger = vi.fn().mockResolvedValue({ success: true });
const execute = vi.fn().mockResolvedValue({ success: true });
const automation = { trigger, execute, listFlows: vi.fn(), getFlow: vi.fn() };

const result = await makeDispatcher({ automation })
.dispatch('POST', '/automation/trigger/nurture', {}, {}, {} as any);

expect(result.response?.status).toBe(200);
expect(trigger).not.toHaveBeenCalled();
expect(execute).toHaveBeenCalledTimes(1);
});

/**
* [#4127] `getFlowRuntimeStates` is declared on `IAutomationService` now,
* and `/automation/_status` reads it through the contract type instead of
* an inline cast that omitted `status` / `triggerType` / `object` — the
* three fields that say WHY a flow is unbound.
*/
it('/automation/_status passes through the full FlowRuntimeState shape', async () => {
const automation = {
listFlows: vi.fn(),
getFlow: vi.fn(),
getFlowRuntimeStates: vi.fn().mockReturnValue([
{ name: 'nurture', enabled: true, bound: false, status: 'active', triggerType: 'on_create', object: 'sales_lead' },
]),
};
const result = await makeDispatcher({ automation }).dispatch('GET', '/automation/_status', undefined, {}, {} as any);
expect(result.response?.status).toBe(200);
expect(result.response?.body?.data?.flows?.[0]).toEqual({
name: 'nurture', enabled: true, bound: false,
status: 'active', triggerType: 'on_create', object: 'sales_lead',
});
});
});

// ---------------------------------------------------------------------------
Expand Down
Loading
Loading