From 7a69229160ab3adddde758b9663c189eedfea740 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:14:45 +0000 Subject: [PATCH] fix(spec,runtime,service-i18n): the dispatcher domains and their service contracts describe the same surface (#4127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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`, 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 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015T5CeitwV1jXLvsL1A84tw --- ...patcher-call-sites-meet-their-contracts.md | 70 ++++++++ .../src/domain-handler-registry.test.ts | 93 ++++++++++ packages/runtime/src/domains/automation.ts | 159 +++++++++++------- packages/runtime/src/domains/i18n.ts | 6 +- packages/runtime/src/domains/notifications.ts | 44 ++++- packages/runtime/src/http-dispatcher.test.ts | 25 ++- .../service-i18n/src/i18n-service-plugin.ts | 14 +- packages/spec/api-surface.json | 5 + .../spec/src/contracts/automation-service.ts | 42 +++++ packages/spec/src/contracts/i18n-service.ts | 23 +++ .../contracts/notification-service.test.ts | 68 ++++++++ .../src/contracts/notification-service.ts | 93 ++++++++++ 12 files changed, 566 insertions(+), 76 deletions(-) create mode 100644 .changeset/dispatcher-call-sites-meet-their-contracts.md diff --git a/.changeset/dispatcher-call-sites-meet-their-contracts.md b/.changeset/dispatcher-call-sites-meet-their-contracts.md new file mode 100644 index 0000000000..b246262f06 --- /dev/null +++ b/.changeset/dispatcher-call-sites-meet-their-contracts.md @@ -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`, 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. diff --git a/packages/runtime/src/domain-handler-registry.test.ts b/packages/runtime/src/domain-handler-registry.test.ts index 4eb1f025a2..4a195bd0d6 100644 --- a/packages/runtime/src/domain-handler-registry.test.ts +++ b/packages/runtime/src/domain-handler-registry.test.ts @@ -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', + }); + }); }); // --------------------------------------------------------------------------- diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 2091900dff..0cebecb5a6 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -10,10 +10,81 @@ */ import { CoreServiceName } from '@objectstack/spec/system'; +import type { IAutomationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; +/** + * Translate a trigger request body into the canonical `AutomationContext` the + * engine expects, and forward the caller's resolved identity. + * + * [#4127] ONE construction point for BOTH trigger routes. It used to live + * inline in `POST /:name/trigger` while `POST /trigger/:name` — the legacy + * shape, and the one `client.automation.trigger()` calls — passed the raw HTTP + * body straight to `execute(name, body)`. Two consequences, both silent: + * + * - The `{ recordId, objectName, params }` translation never ran, so flow + * variables (`params.recordId`, the `Id` alias) resolved from + * nothing. + * - No 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 the SDK's `automation.trigger()` could + * not successfully run any data-touching flow, while the other route could. + * service-automation's own comment claims "most trigger surfaces (REST + * action / trigger endpoint) already resolve the full envelope"; for this + * endpoint that was not true. + * + * Identity forwarding is the FULLY-RESOLVED envelope, not just the user id, so + * a `runAs:'user'` flow enforces RLS exactly as the triggering user — their + * positions/permissions/tenant, not a member fallback (#1888). The engine + * elevates to a system principal only when the flow declares `runAs:'system'`. + */ +function buildAutomationContext(body: any, context: HttpProtocolContext): Record { + const ctxBody = body && typeof body === 'object' ? body : {}; + // `{recordId, objectName, params}` (the UI/SDK request shape) → the + // canonical AutomationContext shape: + // - `recordId` is exposed in `params.recordId` AND aliased to + // `Id` (camelCase) so flow variables like `leadId`, + // `caseId`, `opportunityId` resolve from a single REST contract. + // - `objectName` maps to the canonical `object` field. + const recordId = ctxBody.recordId; + const objectName = ctxBody.objectName ?? ctxBody.object; + const baseParams: Record = (ctxBody.params && typeof ctxBody.params === 'object') + ? { ...ctxBody.params } + : {}; + // Back-compat: when callers POST a flat body (no `params` wrapper), + // forward unknown top-level keys as flow params so the original + // `{ foo: 'bar' }` payload is not silently dropped. + if (!ctxBody.params) { + const reserved = new Set(['recordId', 'objectName', 'object', 'event', 'params']); + for (const [k, v] of Object.entries(ctxBody)) { + if (reserved.has(k)) continue; + if (baseParams[k] === undefined) baseParams[k] = v; + } + } + if (recordId !== undefined && baseParams.recordId === undefined) { + baseParams.recordId = recordId; + } + if (recordId !== undefined && objectName) { + const alias = `${String(objectName).replace(/_([a-z])/g, (_: string, c: string) => c.toUpperCase())}Id`; + if (baseParams[alias] === undefined) baseParams[alias] = recordId; + } + + const automationContext: Record = { + params: baseParams, + object: objectName, + event: ctxBody.event ?? 'manual', + }; + const ec = (context as any)?.executionContext; + const userIdFromAuth = (context as any)?.user?.id ?? (context as any)?.userId ?? ec?.userId; + if (userIdFromAuth) automationContext.userId = userIdFromAuth; + if (Array.isArray(ec?.positions) && ec.positions.length) automationContext.positions = ec.positions; + if (Array.isArray(ec?.permissions) && ec.permissions.length) automationContext.permissions = ec.permissions; + if (ec?.tenantId) automationContext.tenantId = ec.tenantId; + return automationContext; +} + export function createAutomationDomain(deps: DomainHandlerDeps): DomainRoute { return { prefix: '/automation', @@ -54,18 +125,23 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str const m = method.toUpperCase(); const parts = path.replace(/^\/+/, '').split('/').filter(Boolean); - // Legacy: POST /automation/trigger/:name + // Legacy: POST /automation/trigger/:name — the shape + // `client.automation.trigger()` calls. Same handling as + // `POST /:name/trigger` below: one context builder, one service method. + // + // [#4127] This branch used to probe `automationService.trigger(name, body, + // { request })` first and "fall back" to `execute`. Nothing in the repo has + // ever implemented `trigger` on the automation slot — not the engine, not + // the dev stub — and the contract never declared it, so the probe was dead + // on every deployment and the fallback WAS the route. Declaring `trigger?` + // to make the probe honest would have blessed a second name for `execute` + // (Prime Directive #12); the dead branch is gone instead. if (parts[0] === 'trigger' && parts[1] && m === 'POST') { - const triggerName = parts[1]; - if (typeof automationService.trigger === 'function') { - const result = await automationService.trigger(triggerName, body, { request: context.request }); - return { handled: true, response: deps.success(result) }; - } - // Fallback to execute - if (typeof automationService.execute === 'function') { - const result = await automationService.execute(triggerName, body); - return { handled: true, response: deps.success(result) }; - } + const triggerName = parts[1]; + if (typeof automationService.execute === 'function') { + const result = await automationService.execute(triggerName, buildAutomationContext(body, context)); + return { handled: true, response: deps.success(result) }; + } } // GET / → listFlows @@ -135,7 +211,13 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // state). Underscore-prefixed so no flow name can shadow it; MUST precede // the `/:name → getFlow` catch-all. if (parts[0] === '_status' && parts.length === 1 && m === 'GET') { - const svc = automationService as { getFlowRuntimeStates?: () => Array<{ name: string; enabled: boolean; bound: boolean }> }; + // [#4127] Was an inline cast re-declaring the shape as + // `{ name, enabled, bound }` — a third copy of it (engine, here, + // caller), and a narrower one than the engine actually returns: it + // omitted `status` / `triggerType` / `object`, the three fields the + // Studio badge needs to say WHY a flow is unbound. Reads the contract + // now, so there is one shape. + const svc = automationService as Pick; if (typeof svc.getFlowRuntimeStates === 'function') { const flows = svc.getFlowRuntimeStates(); return { handled: true, response: deps.success({ flows, total: flows.length }) }; @@ -148,57 +230,12 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str if (parts.length >= 1) { const name = parts[0]; - // POST /:name/trigger → execute + // POST /:name/trigger → execute. Body translation and identity + // forwarding live in `buildAutomationContext` (#4127), shared with the + // legacy `POST /trigger/:name` above so the two routes cannot drift. if (parts[1] === 'trigger' && m === 'POST') { if (typeof automationService.execute === 'function') { - const ctxBody = body && typeof body === 'object' ? body : {}; - // Translate UI/SDK request shape `{recordId, objectName, params}` - // into the canonical AutomationContext shape expected by the engine. - // Key transformations: - // - `recordId` is exposed in `params.recordId` AND aliased to - // `Id` (camelCase) so flow variables like `leadId`, - // `caseId`, `opportunityId` resolve from a single REST contract. - // - `objectName` is mapped to the canonical `object` field. - // - The user identity from the auth context (if any) is forwarded - // as `userId` so node executors / template interpolation can - // expand `{$User.Id}`. - const recordId = ctxBody.recordId; - const objectName = ctxBody.objectName ?? ctxBody.object; - const baseParams = (ctxBody.params && typeof ctxBody.params === 'object') ? { ...ctxBody.params } : {}; - // Back-compat: when callers POST a flat body (no `params` wrapper), - // forward unknown top-level keys as flow params so the original - // `{ foo: 'bar' }` payload is not silently dropped. - if (!ctxBody.params) { - const reserved = new Set(['recordId', 'objectName', 'object', 'event', 'params']); - for (const [k, v] of Object.entries(ctxBody)) { - if (reserved.has(k)) continue; - if (baseParams[k] === undefined) baseParams[k] = v; - } - } - if (recordId !== undefined && baseParams.recordId === undefined) { - baseParams.recordId = recordId; - } - if (recordId !== undefined && objectName) { - const alias = `${String(objectName).replace(/_([a-z])/g, (_: string, c: string) => c.toUpperCase())}Id`; - if (baseParams[alias] === undefined) baseParams[alias] = recordId; - } - const automationContext: any = { - params: baseParams, - object: objectName, - event: ctxBody.event ?? 'manual', - }; - // Forward the FULLY-RESOLVED caller identity (not just the - // user id) so a `runAs:'user'` flow enforces RLS exactly as the - // triggering user — their roles/tenant, not a member fallback - // (#1888). The engine elevates to a system principal only when - // the flow itself declares `runAs:'system'`. - const ec = (context as any)?.executionContext; - const userIdFromAuth = (context as any)?.user?.id ?? (context as any)?.userId ?? ec?.userId; - if (userIdFromAuth) automationContext.userId = userIdFromAuth; - if (Array.isArray(ec?.positions) && ec.positions.length) automationContext.positions = ec.positions; - if (Array.isArray(ec?.permissions) && ec.permissions.length) automationContext.permissions = ec.permissions; - if (ec?.tenantId) automationContext.tenantId = ec.tenantId; - const result = await automationService.execute(name, automationContext); + const result = await automationService.execute(name, buildAutomationContext(body, context)); return { handled: true, response: deps.success(result) }; } } diff --git a/packages/runtime/src/domains/i18n.ts b/packages/runtime/src/domains/i18n.ts index 73c4f7cc7a..8eb4970ee0 100644 --- a/packages/runtime/src/domains/i18n.ts +++ b/packages/runtime/src/domains/i18n.ts @@ -109,7 +109,11 @@ export async function handleI18nRequest( // Fallback: derive field labels from the locale's translation bundle. // This is not really a fallback — `getFieldLabels` is optional on // `II18nService` and nothing implements it, so this is the path every - // provider takes. Shared with service-i18n's identical derivation so + // provider takes. [#4127] That first clause only became TRUE with this + // change: the method was probed here and in service-i18n while the + // contract never declared it. Declared now, so the probe reads an + // optional capability instead of a second, unwritten contract. + // Shared with service-i18n's identical derivation so // the next bundle-shape change cannot fix one copy and miss the other, // which is how this one went on scanning the retired flat // `o..fields.` dialect after #3778 and returned `{}` diff --git a/packages/runtime/src/domains/notifications.ts b/packages/runtime/src/domains/notifications.ts index 719ba6d352..6359e0b50d 100644 --- a/packages/runtime/src/domains/notifications.ts +++ b/packages/runtime/src/domains/notifications.ts @@ -20,6 +20,7 @@ */ import { CoreServiceName } from '@objectstack/spec/system'; +import type { INotificationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -41,15 +42,36 @@ export async function handleNotificationRequest( query: any, context: HttpProtocolContext, ): Promise { - const service = await deps.resolveService(CoreServiceName.enum.notification, context.environmentId) as any; + // [#4127] Typed against the contract instead of `as any`, so a call this + // file makes that `INotificationService` does not declare is a compile + // error rather than a runtime discovery. That is the check missing when + // #4087 shipped a `/storage` handler calling `upload(key, data)` with two + // wrong arguments for months. + const service = await deps.resolveService( + CoreServiceName.enum.notification, + context.environmentId, + ) as INotificationService | undefined; // [#4058] Three ways to have no inbox capability, one answer. The - // `listInbox` duck-type was already here and, by accident, kept the known + // `listInbox` probe was already here and, by accident, kept the known // fabricating stub off this surface (the dev one implements `send` / // `sendBatch` only). `isServiceServeable` makes that explicit rather than // incidental: a self-declared non-handler (`handlerReady: false`, ADR-0076 // D12) is an empty slot even if it grows a `listInbox` later. A `degraded` // inbox that really serves keeps serving. - if (!isServiceServeable(service) || typeof service.listInbox !== 'function') return { handled: false }; + // + // [#4127] The probe STAYS, and it is not a duck-type any more: `listInbox` + // is now a declared OPTIONAL method. Optional because an inbox needs a + // durable store, and a send-only provider (SMTP, Twilio, a Slack webhook) + // fills this slot legitimately without one — a fact `handlerReady` cannot + // express, since the slot is serveable and only this capability is absent. + // The dev stub implementing exactly `send`/`sendBatch` was never a bug on + // its part: it followed the contract, and the contract was the incomplete + // thing. + if (!service || !isServiceServeable(service) || typeof service.listInbox !== 'function') { + return { handled: false }; + } + // Narrowed for the routes below: the entry probe established `listInbox`. + const inbox = service as INotificationService & Required>; const userId: string | undefined = context.executionContext?.userId; if (!userId) { @@ -64,25 +86,35 @@ export async function handleNotificationRequest( // the legacy `.replace(/\/+$/, '')` had carried the trap since ADR-0030. const subPath = path.split('/').filter(Boolean).join('/'); + // Each write route probes its OWN method rather than riding the entry + // `listInbox` probe (#4127). The three are separately optional on the + // contract, so "has an inbox to read" does not imply "has read-state to + // write" — and the same-shaped `handled: false` is what an absent + // capability already answers everywhere else in this file. Before the + // methods were declared, the entry probe was the only thing standing + // between a list-only provider and a `TypeError` on `markRead`. + // GET /notifications — list the user's inbox joined with read-state. if (subPath === '' && m === 'GET') { const read = query?.read === undefined ? undefined : String(query.read) === 'true'; const limit = query?.limit ? Number(query.limit) : undefined; const type = query?.type ? String(query.type) : undefined; - const result = await service.listInbox(userId, { read, type, limit }); + const result = await inbox.listInbox(userId, { read, type, limit }); return { handled: true, response: deps.success(result) }; } // POST /notifications/read — mark specific notifications read. if (subPath === 'read' && m === 'POST') { + if (typeof inbox.markRead !== 'function') return { handled: false }; const ids: string[] = Array.isArray(body?.ids) ? body.ids.map((x: unknown) => String(x)) : []; - const result = await service.markRead(userId, ids); + const result = await inbox.markRead(userId, ids); return { handled: true, response: deps.success(result) }; } // POST /notifications/read/all — mark all of the user's inbox read. if (subPath === 'read/all' && m === 'POST') { - const result = await service.markAllRead(userId); + if (typeof inbox.markAllRead !== 'function') return { handled: false }; + const result = await inbox.markAllRead(userId); return { handled: true, response: deps.success(result) }; } diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 3d651e6055..d430bfd3b4 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -416,10 +416,31 @@ describe('HttpDispatcher', () => { expect(result.response?.status).toBe(404); }); - it('should handle legacy trigger path POST /trigger/:name', async () => { + /** + * [#4127] This used to assert `trigger('flow_a', { data: 1 }, { request })` + * — a method the mock invented. Nothing in the repo implements `trigger` + * on the automation slot and `IAutomationService` never declared it, so + * the branch it pinned was dead on every deployment while the "fallback" + * to `execute` was the actual route. Same test shape that let #4087 sit + * green on a `/storage` handler calling `upload` with the wrong + * arguments: mock what the handler wants, and the handler always agrees. + * + * The route goes through the CONTRACT method now, with the body + * translated into an AutomationContext. Identity forwarding is covered + * in domain-handler-registry.test.ts, which can seed an + * executionContext without dispatch() overwriting it. + */ + it('routes the legacy POST /trigger/:name through execute, never a non-contract trigger()', async () => { const result = await dispatcher.handleAutomation('trigger/flow_a', 'POST', { data: 1 }, { request: {} }); expect(result.handled).toBe(true); - expect(mockAutomationService.trigger).toHaveBeenCalledWith('flow_a', { data: 1 }, { request: {} }); + expect(mockAutomationService.trigger).not.toHaveBeenCalled(); + expect(mockAutomationService.execute).toHaveBeenCalledTimes(1); + const [name, ctx] = mockAutomationService.execute.mock.calls[0]; + expect(name).toBe('flow_a'); + // A flat body survives as flow params rather than being handed to + // the engine as an AutomationContext it cannot read anything from. + expect(ctx.params).toEqual({ data: 1 }); + expect(ctx.event).toBe('manual'); }); // ── GET /actions — action descriptor registry (ADR-0018) ────────── diff --git a/packages/services/service-i18n/src/i18n-service-plugin.ts b/packages/services/service-i18n/src/i18n-service-plugin.ts index aa0b6dacb7..c7dda632d8 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.ts @@ -238,12 +238,14 @@ export class I18nServicePlugin implements Plugin { sendError(res, 400, 'INVALID_REQUEST', 'Missing object or locale parameter'); return; } - // Some implementations may provide a dedicated getFieldLabels method - const hasGetFieldLabels = 'getFieldLabels' in i18n - && typeof (i18n as Record)['getFieldLabels'] === 'function'; - if (hasGetFieldLabels) { - const labels = (i18n as II18nService & { getFieldLabels(obj: string, loc: string): Record }) - .getFieldLabels(objectName, locale); + // Some implementations may provide a dedicated getFieldLabels method. + // [#4127] Was `'getFieldLabels' in i18n` plus two casts — one through + // `Record`, one re-declaring the signature inline — + // because `II18nService` did not declare the method both this mount + // and the dispatcher's were probing for. It does now, so the probe is + // a plain optional-method check and the signature has one home. + if (typeof i18n.getFieldLabels === 'function') { + const labels = i18n.getFieldLabels(objectName, locale); sendOk(res, { object: objectName, locale, labels }); } else { // Fallback: read field labels out of the locale's translation data. diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 9707b1c325..8e2e08f29d 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3572,6 +3572,7 @@ "ExportJobDownload (interface)", "ExportJobListResult (interface)", "FinishReason (type)", + "FlowRuntimeState (interface)", "GenerateDraftOpts (interface)", "GenerateObjectOptions (interface)", "GrantShareInput (interface)", @@ -3641,6 +3642,9 @@ "IWorkflowService (interface)", "ImportObjectOpts (interface)", "ImportObjectResult (interface)", + "InboxListResult (interface)", + "InboxNotification (interface)", + "InboxQuery (interface)", "InstallPackageInput (interface)", "InstallPackageResult (interface)", "IntrospectedColumn (interface)", @@ -3664,6 +3668,7 @@ "LockAcquireOptions (interface)", "LockHandle (interface)", "Logger (interface)", + "MarkReadResult (interface)", "MessageObservability (interface)", "MetadataExportOptions (interface)", "MetadataImportOptions (interface)", diff --git a/packages/spec/src/contracts/automation-service.ts b/packages/spec/src/contracts/automation-service.ts index 9edee91a2d..fb3c2bd1d0 100644 --- a/packages/spec/src/contracts/automation-service.ts +++ b/packages/spec/src/contracts/automation-service.ts @@ -255,6 +255,30 @@ export interface ResumeSignal { [RESUME_AUTHORITY_SERVICE]?: true; } +/** + * One flow's deployment + trigger-binding state, as + * {@link IAutomationService.getFlowRuntimeStates} reports it (#4127). + */ +export interface FlowRuntimeState { + /** Flow name (snake_case) — the key {@link IAutomationService.execute} takes. */ + name: string; + /** Whether the flow is enabled; a disabled flow stays registered and never runs. */ + enabled: boolean; + /** + * Whether a trigger is actually wired to this flow. `false` for a flow with + * no declared trigger (manual / screen flows) AND for one whose declared + * trigger type has no registered trigger — `triggerType` distinguishes the + * two. + */ + bound: boolean; + /** Persisted deployment status, when the flow carries one. */ + status?: string; + /** Declared trigger type, when the flow declares one. */ + triggerType?: string; + /** Object the trigger binds to, for object-bound trigger types. */ + object?: string; +} + export interface IAutomationService { /** * Execute a named flow or script @@ -322,6 +346,24 @@ export interface IAutomationService { */ getActionDescriptors?(): ActionDescriptor[]; + /** + * Per-flow deployment + binding state, for operator surfaces. + * + * [#4127] Declared because the dispatcher's `GET /automation/_status` + * already called it (as did the CLI boot summary and the + * `kernel:bootstrapped` audit), while the contract stopped at + * {@link listFlows} — a bare `string[]` that cannot say whether a flow is + * enabled, or bound to a trigger, or why it is not. + * + * `bound: false` is the answer the operator surfaces exist for: a flow with + * no trigger (manually-invoked / screen flows) or whose declared trigger + * type has no registered trigger. `triggerType` / `object` expose the + * declared binding so the caller can say WHY, rather than only that. + * + * @returns One entry per registered flow + */ + getFlowRuntimeStates?(): FlowRuntimeState[]; + /** * Resume a run that suspended at a pausing node (ADR-0019). The run must * have previously returned `{ status: 'paused', runId }` from diff --git a/packages/spec/src/contracts/i18n-service.ts b/packages/spec/src/contracts/i18n-service.ts index eb50b3d17b..4fc7b261b3 100644 --- a/packages/spec/src/contracts/i18n-service.ts +++ b/packages/spec/src/contracts/i18n-service.ts @@ -57,6 +57,29 @@ export interface II18nService { */ setDefaultLocale?(locale: string): void; + /** + * Field labels for one object in one locale, keyed by field name. + * + * [#4127] A provider-supplied SHORTCUT, not the source of truth. Both + * serving surfaces — the dispatcher's `/i18n/labels/:object/:locale` and + * service-i18n's own mount — probe for it and, finding nothing, derive the + * labels from the locale's loaded bundle (`resolveObjectFieldLabels`). + * That derivation is the path every provider in this repo takes. + * + * It is declared anyway because both probes already existed and both call + * sites documented it as "optional on `II18nService`" — which was simply + * not true until now. An undeclared method probed in two places is how a + * second, unwritten contract starts (#4087 is what that costs). A provider + * that keeps labels somewhere the bundle cannot express — a translation + * memory, a per-tenant override table — implements this and skips the + * derivation; everyone else omits it and loses nothing. + * + * @param objectName - Object whose fields to label + * @param locale - BCP-47 locale code + * @returns Field name → label, for the fields this provider knows + */ + getFieldLabels?(objectName: string, locale: string): Record; + // ── Diff detection ───────────────────────────────────────────────── /** diff --git a/packages/spec/src/contracts/notification-service.test.ts b/packages/spec/src/contracts/notification-service.test.ts index 32b0ed8a5e..55ed84a5f7 100644 --- a/packages/spec/src/contracts/notification-service.test.ts +++ b/packages/spec/src/contracts/notification-service.test.ts @@ -139,4 +139,72 @@ describe('Notification Service Contract', () => { expect(sent[0].templateId).toBe('welcome-email'); expect(sent[0].templateData?.userName).toBe('Alice'); }); + + /** + * [#4127] The inbox half. These three backed three SDK-expressed dispatcher + * routes for as long as the routes existed, while this file described only + * the send half — so the dev stub, which implements exactly `send` and + * `sendBatch` BECAUSE it followed this contract, was the implementation the + * domain had to duck-type past. + * + * Optional, and the first test is the reason: a send-only provider (SMTP, + * Twilio, a Slack webhook) fills the slot legitimately with no inbox at all. + */ + describe('inbox (#4127)', () => { + it('keeps a send-only provider valid — the inbox trio is optional', () => { + const sendOnly: INotificationService = { + send: async () => ({ success: true }), + }; + + expect(sendOnly.listInbox).toBeUndefined(); + expect(sendOnly.markRead).toBeUndefined(); + expect(sendOnly.markAllRead).toBeUndefined(); + }); + + it('types an inbox provider the way service-messaging implements it', async () => { + const readIds = new Set(); + const rows = [ + { id: 'n1', type: 'mention', title: 'You were mentioned', body: 'in Acme', createdAt: '2026-07-30T00:00:00.000Z' }, + { id: 'n2', type: 'assignment', title: 'Case assigned', body: 'CASE-1', actionUrl: '/cases/1', createdAt: '2026-07-30T01:00:00.000Z' }, + ]; + + const service: INotificationService = { + send: async () => ({ success: true }), + listInbox: async (_userId, options) => { + const all = rows.map((r) => ({ ...r, read: readIds.has(r.id) })); + const filtered = all + .filter((r) => (options?.read === undefined ? true : r.read === options.read)) + .filter((r) => (options?.type ? r.type === options.type : true)) + .slice(0, options?.limit ?? 50); + return { notifications: filtered, unreadCount: all.filter((r) => !r.read).length }; + }, + markRead: async (_userId, ids) => { + let readCount = 0; + for (const id of ids) { + if (rows.some((r) => r.id === id) && !readIds.has(id)) { readIds.add(id); readCount += 1; } + } + return { success: true, readCount }; + }, + markAllRead: async (userId) => { + const { notifications } = await service.listInbox!(userId, { read: false }); + return service.markRead!(userId, notifications.map((n) => n.id)); + }, + }; + + const first = await service.listInbox!('u-1'); + expect(first.notifications).toHaveLength(2); + expect(first.unreadCount).toBe(2); + + expect(await service.markRead!('u-1', ['n1'])).toEqual({ success: true, readCount: 1 }); + // Re-marking an already-read notification counts nothing. + expect(await service.markRead!('u-1', ['n1'])).toEqual({ success: true, readCount: 0 }); + + const unread = await service.listInbox!('u-1', { read: false }); + expect(unread.notifications.map((n) => n.id)).toEqual(['n2']); + expect(unread.unreadCount).toBe(1); + + expect(await service.markAllRead!('u-1')).toEqual({ success: true, readCount: 1 }); + expect((await service.listInbox!('u-1', { read: false })).notifications).toHaveLength(0); + }); + }); }); diff --git a/packages/spec/src/contracts/notification-service.ts b/packages/spec/src/contracts/notification-service.ts index ef84edebd6..a8667b40da 100644 --- a/packages/spec/src/contracts/notification-service.ts +++ b/packages/spec/src/contracts/notification-service.ts @@ -55,6 +55,56 @@ export interface NotificationResult { error?: string; } +/** + * Filters for {@link INotificationService.listInbox}. Mirrors + * `ListNotificationsRequestSchema` minus `cursor` — no implementation paginates + * by cursor yet, and declaring a parameter nothing honours is the + * `declared ≠ enforced` gap this file exists to close (#4127). + */ +export interface InboxQuery { + /** Filter by read state; omitted returns both. */ + read?: boolean; + /** Filter by notification type/topic. */ + type?: string; + /** Maximum rows to return. Implementations may clamp. */ + limit?: number; +} + +/** + * One inbox row. Mirrors `NotificationSchema` (`api/protocol.zod.ts`) — the + * shape the dispatcher serializes straight to the wire. + */ +export interface InboxNotification { + /** Stable notification id — what `markRead` takes. */ + id: string; + /** Notification type/topic. */ + type: string; + /** Display title. */ + title: string; + /** Body text. */ + body: string; + /** Whether this user has read it. */ + read: boolean; + /** URL to open when the notification is clicked. */ + actionUrl?: string; + /** ISO-8601 creation timestamp. */ + createdAt: string; +} + +/** Result of {@link INotificationService.listInbox}. */ +export interface InboxListResult { + notifications: InboxNotification[]; + /** Unread count over the returned window. */ + unreadCount: number; +} + +/** Result of {@link INotificationService.markRead} / `markAllRead`. */ +export interface MarkReadResult { + success: boolean; + /** How many notifications this call actually transitioned to read. */ + readCount: number; +} + export interface INotificationService { /** * Send a notification @@ -75,4 +125,47 @@ export interface INotificationService { * @returns Array of supported channel names */ getChannels?(): NotificationChannel[]; + + /* ------------------------------------------------------------------ */ + /* Inbox — the READ half of the slot (#4127) */ + /* ------------------------------------------------------------------ */ + + /** + * List the user's in-app inbox, joined with read-state. + * + * [#4127] Declared here because the dispatcher's `/notifications` domain + * has always called it — three SDK-expressed routes (`notifications.list` + * / `.markRead` / `.markAllRead`) rest on this trio, while the contract + * described only the send half. The consequence was not academic: the dev + * notification stub implements `send` / `sendBatch` and nothing else + * *because it followed this file*, so the one implementation written to + * the contract was the one the domain had to duck-type past. + * + * OPTIONAL on purpose, and the runtime probe stays. An inbox needs a + * durable store (`service-messaging` joins `sys_notification` against its + * receipt spine); a send-only provider — SMTP, Twilio, a Slack webhook — + * fills this slot legitimately with no inbox at all. `handlerReady` cannot + * express that: the slot IS serveable, one capability of it is absent. So + * the domain still asks `typeof service.listInbox === 'function'` — but + * that is now a declared optional capability being probed, not a method + * invented at the call site. + * + * Shapes mirror the wire contract the domain serializes untouched — + * `ListNotificationsResponseSchema` / `MarkNotificationsReadResponseSchema` + * (`api/protocol.zod.ts`). + */ + listInbox?(userId: string, options?: InboxQuery): Promise; + + /** + * Mark specific notifications read for this user. + * @param userId - Owner of the inbox + * @param ids - Notification ids to mark; unknown ids are ignored + */ + markRead?(userId: string, ids: readonly string[]): Promise; + + /** + * Mark every unread notification in this user's inbox read. + * @param userId - Owner of the inbox + */ + markAllRead?(userId: string): Promise; }