diff --git a/.changeset/adr-0110-action-declaration-admission.md b/.changeset/adr-0110-action-declaration-admission.md new file mode 100644 index 0000000000..d68e623202 --- /dev/null +++ b/.changeset/adr-0110-action-declaration-admission.md @@ -0,0 +1,36 @@ +--- +'@objectstack/runtime': major +'@objectstack/objectql': minor +'@objectstack/metadata': minor +--- + +**ADR-0110 — an action's identity is its `name`, and anything executable over a +governed surface must have a declaration.** + +`POST /api/v1/actions/:object/:action` resolved the DECLARATION from the URL +segment as a `name` but dispatched the HANDLER using that same segment as a +registry key. For a target-bound action (`{ name: 'complete_task', target: +'completeTask' }`) those are different strings, so the two documented callers +each worked on exactly the half the other broke: the documented curl resolved +the declaration then 404ed, while the Console's `target`-addressed call +dispatched fine and resolved no declaration — silently skipping the ADR-0066 D4 +capability gate and the ADR-0104 param contract (#3935). + +- **D1/D2** — identity is always the declarative `name`; the handler key is + derived from the resolved declaration through a rotation now shared with the + MCP `run_action` bridge (`resolveActionHandlerKeys`, `executeRegisteredAction`). + The REST route previously rotated only the object key, never the handler key. +- **D3 (breaking)** — declaration resolution is a trichotomy. A genuinely + undeclared handler is **refused (404)** with the `defineAction` to add, rather + than executed ungated with system privileges; an unreachable metadata plane is + a **503** rather than a silent ungating (`MetadataManager.loadDiagnosed` tells + a clean miss from an outage). `OS_ALLOW_UNDECLARED_ACTIONS=1` is the migration + valve — it warns on every invocation and is removed in 18. +- **D5** — `reconcileActionRegistrations` plus `ObjectQLEngine.listRegisteredActions` + power a `kernel:ready` inventory logging every registered-but-undeclared + handler (refused at dispatch) and every declared script action bound to no + handler — the ADR-0078 converse, mechanised. +- **D6** — security-gate strictness is opt-**out** (`OS_ALLOW_*`), never opt-in. + +Apps whose actions are all declared need no changes beyond gaining enforcement +of the `requiredPermissions` they already declared. diff --git a/content/docs/releases/v17.mdx b/content/docs/releases/v17.mdx index 87e690d803..9a1042a245 100644 --- a/content/docs/releases/v17.mdx +++ b/content/docs/releases/v17.mdx @@ -156,6 +156,44 @@ and flags whitelists the mapping would *widen* so every edit stays reviewable. `['upsert']`) strips to `[]`, which is **deny-all** — the object's API closes rather than widening. +### An action must be declared to be invocable, and is identified by `name` (ADR-0110, #3935) + +Two halves of the `/api/v1/actions/:object/:action` contract were broken in +complementary ways, which is why neither surfaced: the route resolved the +**declaration** from the URL segment as a `name`, but dispatched the +**handler** using that same segment as a registry key. For a target-bound +action (`{ name: 'complete_task', target: 'completeTask' }`) those differ — so +the documented `curl .../todo_task/complete_task` resolved the declaration and +then 404ed, while the Console's `target`-addressed call dispatched fine and +resolved **no** declaration, silently skipping the ADR-0066 D4 capability gate +and the ADR-0104 param contract. + +Three changes land together: + +- **Identity is `name`.** The URL, MCP `run_action`, and every future surface + identify an action by its declarative `name`. `target` is a binding + expression — polymorphic per type, `${param.X}`-interpolatable, and legally + non-unique — so it never identifies anything. The server derives the handler + key from the declaration it resolved, using the rotation the MCP bridge + already used. The Console posts `name` (objectui ships in lockstep). +- **An undeclared handler is refused.** `engine.registerAction` with no + matching declaration has no `requiredPermissions` to enforce and no param + contract to check, yet executes with system privileges. It now returns 404 + naming the `defineAction` to add. Deleting a declaration used to *remove* an + action's gate while leaving it callable; removal now narrows. +- **An unreachable metadata plane refuses instead of degrading.** A loader + failure used to be indistinguishable from "no declaration", so an outage + silently ungated every action it could not see. That is now a **503** — the + same posture as the datasource entry below. + +**Migration:** boot logs list every registered-but-undeclared handler under +`[action-governance]`, alongside declared script actions bound to no handler. +Declare each one. `OS_ALLOW_UNDECLARED_ACTIONS=1` runs them meanwhile, warning +on every invocation; **it is removed in 18**. Apps whose actions are all +declared — anything with working Console buttons — need no changes, other than +gaining enforcement of the `requiredPermissions` they already declared. Callers +that hard-coded a `target` in an action URL switch to the action's `name`. + ### A flow run with no trigger user may not touch data (#3760) An effective `runAs: 'user'` run that resolved **no trigger user** used to diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index da777216a8..15f01f59a5 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -232,6 +232,13 @@ curl -b cookies.txt -X POST \ # → 200 { "success": true, "data": { "success": true, "data": ... } } ``` +The URL names the action by its **`name`**, never by `target`. `target` binds +the action to whatever runs it — a handler key here, a flow id for +`type: 'flow'`, a URL for `type: 'url'` — so it is an implementation detail: +the server resolves your declaration by name and derives the handler key from +it. Rename the underlying function freely; as long as the declaration's +`target` follows, the public URL is unchanged. + Failures split three ways: - **It ran and rejected** — HTTP **200**, `data: { "success": false, "error", @@ -264,6 +271,47 @@ The endpoint dispatches on the **declared `type`**, exactly like the MCP | `api` | **400** — it dispatches on `target`; call that endpoint directly. | | `url` / `modal` / `form` | **400** — client-side navigation; there is nothing for the server to run. | +## Headless actions: declare it, then hide it + +An action that should be callable but not appear in the UI is still a +**declared** action. Hiding is a property you set; it is not the absence of a +declaration: + +```typescript +defineAction({ + name: 'recalculate_commissions', + type: 'script', + target: 'recalcCommissions', + locations: [], // no UI surface + requiredPermissions: ['finance.admin'], // still gated + // `ai.exposed` is false by default — no MCP tool either +}) +``` + +You keep the capability gate, the param contract, the audit trail, and Setup +visibility for admins. A declaration that no surface renders costs you nothing. + + +Registering a handler **without** a declaration is not a way to hide an action — +it is refused. An undeclared handler has no `requiredPermissions` to enforce and +no param contract to check, yet it would execute with system privileges, so the +server declines it: + +``` +Action 'recalc' on 'crm_account' has no declaration — add +`defineAction({ name: 'recalc', … })`, or register the handler under a declared +action's `target`. +``` + +If server-side logic should never be reachable over HTTP at all, do not register +it as an action — export a plain function and call it from your own code. +`engine.registerAction` means "publish this on the HTTP and MCP surfaces". + +Migrating an app that has undeclared handlers? Boot logs list every one of them +under `[action-governance]`, and `OS_ALLOW_UNDECLARED_ACTIONS=1` runs them +(warning each time) while you add the declarations. That valve is removed in 18. + + ## Expose it to AI (MCP) Actions are **not** AI-visible by default. Opting in takes two fields — and diff --git a/docs/adr/0110-action-identity-and-declaration-admission.md b/docs/adr/0110-action-identity-and-declaration-admission.md index 1abcd6aa9c..26bb9350d5 100644 --- a/docs/adr/0110-action-identity-and-declaration-admission.md +++ b/docs/adr/0110-action-identity-and-declaration-admission.md @@ -1,6 +1,7 @@ # ADR-0110: An action's identity is its `name`; anything executable over a governed surface must have a declaration (the declaration-admission gate) -**Status**: Proposed (2026-07-29) — **targeted at protocol 17** (`@objectstack/spec@17.0.0-rc.0`, in RC now). The fail-closed inversion lands in that major rather than staging across two, joining its two siblings already in the v17 breaking set: *"A flow run with no trigger user may not touch data"* (#3760 — missing identity fails closed) and *"A datasource that cannot connect fails the boot"* (#3741/#3758/#3826 — an unreachable dependency refuses rather than degrades). +**Status**: **Accepted — implemented** (2026-07-29) in protocol 17 (`@objectstack/spec@17.0.0-rc.0`). The fail-closed inversion landed in that major rather than staging across two, joining its two siblings in the v17 breaking set: *"A flow run with no trigger user may not touch data"* (#3760 — missing identity fails closed) and *"A datasource that cannot connect fails the boot"* (#3741/#3758/#3826 — an unreachable dependency refuses rather than degrades). +Evidence: D1/D2 — `action-execution.ts` (`resolveActionHandlerKeys`, `executeRegisteredAction`) shared by `domains/actions.ts` and `invokeBusinessAction`, pinned by `http-dispatcher.actions-identity-addressing.test.ts` (the documented curl now gates AND dispatches). D3 — the trichotomy in `domains/actions.ts`, reading the `degraded` signal `MetadataManager.loadDiagnosed` supplies (`metadata-manager.ts`). D5 — `reconcileActionRegistrations` + `ObjectQLEngine.listRegisteredActions`, wired to the `kernel:ready` inventory in `app-plugin.ts`, covered by `action-reconciliation.test.ts`. D1 client — `../objectui` `useConsoleActionRuntime.tsx`. D4 + migration — `content/docs/ui/actions.mdx`, `content/docs/releases/v17.mdx`. **Deciders**: ObjectStack Protocol Architects **Builds on**: [ADR-0049](./0049-no-unenforced-security-properties.md) (enforce-or-remove gate; a security property that parses but does nothing is worse than absent), [ADR-0066](./0066-unified-authorization-model.md) (D4 — action `requiredPermissions`, dual-surface, server is source of truth), [ADR-0078](./0078-no-silently-inert-metadata.md) (no silently inert metadata — this ADR is its **converse**: no silently ungoverned executable), [ADR-0096](./0096-execution-surface-identity-admission.md) (missing **identity** must not fail open; this ADR extends the same posture to missing **declaration**), [ADR-0104](./0104-field-runtime-value-shape-contract.md) (D2 — declared action param contract), [ADR-0109](./0109-ai-tool-authoring-model.md) (every declarative action materialises `action_`; the declaration IS the AI-facing capability) **Consumers**: `@objectstack/runtime` (`domains/actions.ts`, `action-execution.ts`), `@objectstack/objectql` (`registerAction` contract), `@objectstack/lint` (new reconciliation rule), `../objectui` (`useConsoleActionRuntime` invocation URL), `content/docs/ui/actions.mdx` (public REST contract) diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 54a81354f0..3ec9ae9564 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -1395,23 +1395,53 @@ export class MetadataManager implements IMetadataService { /** * Load a single metadata item from loaders. * Iterates through registered loaders until found. + * + * Returns `null` both when no loader HAS the item and when every loader + * FAILED — see {@link loadDiagnosed} when the caller must tell those apart. */ async load( type: string, name: string, options?: MetadataLoadOptions ): Promise { + return (await this.loadDiagnosed(type, name, options)).data; + } + + /** + * `load`, plus whether the answer can be trusted as complete. + * + * [ADR-0110 D3] A miss and an outage are different facts with opposite + * security meanings, and plain `load` cannot express the difference: a + * loader that throws is warn-logged and skipped, so a database the metadata + * plane cannot reach returns the same `null` as a name that was never + * declared. Callers that gate on a declaration MUST NOT read that `null` as + * "the author declared no gate" — an availability failure would silently + * widen access (the REST `/actions` route's fail-open branch, #3935). + * + * `degraded` is true when at least one loader threw AND no loader answered + * with the item. The posture is deliberately conservative: with a loader + * down we cannot prove the item is absent, so we decline to claim it is. + * A clean miss (every loader answered, none had it) is NOT degraded. + */ + async loadDiagnosed( + type: string, + name: string, + options?: MetadataLoadOptions + ): Promise<{ data: T | null; degraded: boolean; errors: string[] }> { + const errors: string[] = []; for (const loader of this.loaders.values()) { try { const result = await loader.load(type, name, options); if (result.data) { - return result.data as T; + return { data: result.data as T, degraded: false, errors }; } } catch (e) { + const message = e instanceof Error ? e.message : String(e); + errors.push(`${loader.contract.name}: ${message}`); this.logger.warn(`Loader ${loader.contract.name} failed to load ${type}:${name}`, { error: e }); } } - return null; + return { data: null, degraded: errors.length > 0, errors }; } /** diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 5ec3fbe48b..f7a0544252 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -745,6 +745,31 @@ export class ObjectQL implements IDataEngine { return entry.handler(ctx); } + /** + * Every action handler currently registered, as `{ objectName, actionName }` + * pairs (plus the owning package when one was given). + * + * [ADR-0110 D5] The handler registry is one half of the + * declaration↔executable bijection; with no way to enumerate it, the other + * half could only be checked when someone happened to invoke a route. The + * boot reconciliation reads this to list handlers no declaration covers — + * which since D3 are refused at dispatch, so having the inventory is what + * makes that refusal a checklist instead of a support ticket. + */ + listRegisteredActions(): Array<{ objectName: string; actionName: string; package?: string }> { + const out: Array<{ objectName: string; actionName: string; package?: string }> = []; + for (const [key, entry] of this.actions.entries()) { + const sep = key.indexOf(':'); + if (sep < 0) continue; + out.push({ + objectName: key.slice(0, sep), + actionName: key.slice(sep + 1), + ...(entry.package ? { package: entry.package } : {}), + }); + } + return out; + } + /** * Remove all actions registered by a specific package. */ diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 4e51e7838e..2c3eaebe8a 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -29,6 +29,19 @@ function actionParamsStrict(): boolean { return typeof process !== 'undefined' && process.env?.OS_ACTION_PARAMS_STRICT_ENABLED === '1'; } +/** + * [ADR-0110 D3/D6] Migration valve for executing an action that has no + * declaration. Enforcement is the DEFAULT and this opts OUT of it — the + * direction matters: a security gate whose strictness is opt-in (as + * `OS_ACTION_PARAMS_STRICT_ENABLED` is, acceptably, for a DX contract) ships + * open for everyone who never read the release notes. `OS_ALLOW_*` is the + * sanctioned shape for a security escape hatch; it warns on every invocation + * and is slated for removal in 18. + */ +export function undeclaredActionsAllowed(deps: ActionExecutionDeps): boolean { + return typeof process !== 'undefined' && process.env?.OS_ALLOW_UNDECLARED_ACTIONS === '1'; +} + const _warnedActionParams = new Set(); function warnActionParamsOnce(key: string, message: string): void { if (_warnedActionParams.has(key)) return; @@ -813,25 +826,15 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, executionContext: buildActionExecutionContext(ec), params: { ...params, recordId, objectName }, }; - // Handler key: body-based actions register under `name` (AppPlugin); - // target-bound script actions register under `target` (user code). - // Probe both, then the object-less keys (#3913), distinguishing the - // engine's "action not registered" miss from a genuine handler error. - const primary = action.body ? action.name : (action.target || action.name); - const candidates = [primary, action.target, action.name].filter( - (k: unknown, i: number, a: unknown[]): k is string => typeof k === 'string' && a.indexOf(k) === i, + // [ADR-0110 D2] Handler-key derivation + the probe rotation are shared with + // the REST `/actions` route — one addressing algorithm, not two. + const dispatch = await executeRegisteredAction( + deps, ql, objectName, resolveActionHandlerKeys(action), actionContext, ); - for (const obj of actionHandlerObjectKeys(objectName)) { - for (const key of candidates) { - try { - const result = await ql.executeAction(obj, key, actionContext); - return { ok: true, action: action.name, objectName, ...(recordId ? { recordId } : {}), result: result ?? null }; - } catch (err: any) { - if (!isActionNotRegisteredError(err)) throw err; // real handler failure → surface - } - } + if (!dispatch.dispatched) { + throw new Error(`No handler registered for action '${name}' on '${objectName}'`); } - throw new Error(`No handler registered for action '${name}' on '${objectName}'`); + return { ok: true, action: action.name, objectName, ...(recordId ? { recordId } : {}), result: dispatch.result ?? null }; } /** @@ -914,6 +917,72 @@ export async function collectActionDeclarations(deps: ActionExecutionDeps, return out; } +/** + * [ADR-0110 D5] Reconcile the two halves of the declaration↔executable + * bijection and report the orphans on both sides. + * + * ADR-0078 outlaws a declaration nothing executes (silently *inert*); D3 + * outlaws an executable nothing declares (silently *ungoverned*). Together + * they are one invariant — everything declared runs, everything that runs is + * declared — and this is the mechanism that makes a violation visible instead + * of waiting for someone to invoke the route. + * + * Two findings: + * - `undeclared_handler` — a registered key that reconciles to no + * declaration. Since D3 those are REFUSED at dispatch, so this list is the + * upgrade checklist: everything on it is an endpoint that stopped working + * and the exact `defineAction` that fixes it. + * - `unbound_declaration` — a declared `script` action with no `body` and no + * handler under any candidate key: a button wired to nothing. + * + * A handler reconciles when some declaration for its object (or an object-less + * one) yields it among {@link resolveActionHandlerKeys} — the SAME derivation + * dispatch uses, so the inventory cannot disagree with the router. + */ +export function reconcileActionRegistrations(deps: ActionExecutionDeps, + registered: Array<{ objectName: string; actionName: string; package?: string }>, + declarations: Array<{ action: any; objectName: string }>, +): { + undeclaredHandlers: Array<{ objectName: string; actionName: string; package?: string }>; + unboundDeclarations: Array<{ objectName: string; actionName: string }>; +} { + // Every key any declaration can address, per owning object key. + const addressable = new Map>(); + const addKey = (objectKey: string, handlerKey: string) => { + let set = addressable.get(objectKey); + if (!set) addressable.set(objectKey, (set = new Set())); + set.add(handlerKey); + }; + for (const { action, objectName } of declarations) { + for (const key of resolveActionHandlerKeys(action)) addKey(objectName, key); + } + + const covers = (objectName: string, actionName: string): boolean => { + if (addressable.get(objectName)?.has(actionName)) return true; + // A handler registered under an object-less key is addressable by any + // object-less declaration, mirroring `actionHandlerObjectKeys`. + if (!isObjectLessActionKey(objectName)) return false; + for (const [objectKey, keys] of addressable) { + if (isObjectLessActionKey(objectKey) && keys.has(actionName)) return true; + } + return false; + }; + + const undeclaredHandlers = registered.filter((r) => !covers(r.objectName, r.actionName)); + + const registeredKeys = new Set(registered.map((r) => `${r.objectName}:${r.actionName}`)); + const unboundDeclarations: Array<{ objectName: string; actionName: string }> = []; + for (const { action, objectName } of declarations) { + if ((action?.type ?? 'script') !== 'script') continue; // only script needs a handler + if (action?.body) continue; // its handler is synthesized + const bound = resolveActionHandlerKeys(action).some((key) => + actionHandlerObjectKeys(objectName).some((obj) => registeredKeys.has(`${obj}:${key}`))); + if (!bound) unboundDeclarations.push({ objectName, actionName: action?.name }); + } + + return { undeclaredHandlers, unboundDeclarations }; +} + /** * Owning object of a standalone `action` item — must stay in lockstep with * the ObjectQL plugin's `actionObjectKey` (the engine registration key), so @@ -968,6 +1037,69 @@ export function isActionNotRegisteredError(err: any): boolean { return /Action '.+' on object '.+' not found/i.test(String(err?.message ?? err)); } +/** + * [ADR-0110 D2] Handler-key candidates for an action, most-specific first — + * the *addressing* half of "resolve, then address". + * + * A registration key is NOT an action's identity. `app-plugin.ts` + * auto-registers **body** actions under `name`, while user code registers a + * **target-bound** script action under `target` + * (`engine.registerAction('todo_task', 'completeTask', …)`). Identity is + * always the declarative `name` (D1); which key the handler happens to live + * under is derived HERE, from the already-resolved declaration, so no caller + * ever has to know it. + * + * `fallbackKey` (the routed URL segment) is the last candidate and exists for + * the UNDECLARED case, where there is no declaration to derive anything from. + * It is deduped away whenever the declaration already yields it, so it never + * widens what a declared action can reach. + */ +export function resolveActionHandlerKeys(action: any, fallbackKey?: string): string[] { + const primary = action ? (action.body ? action.name : (action.target || action.name)) : undefined; + return [primary, action?.target, action?.name, fallbackKey].filter( + (k: unknown, i: number, a: unknown[]): k is string => + typeof k === 'string' && k.length > 0 && a.indexOf(k) === i, + ); +} + +/** + * [ADR-0110 D2] Run a script/body action through the engine's handler + * registry: rotate the derived key candidates across the object-key rotation + * (`actionHandlerObjectKeys`), telling an "unregistered key" miss apart from + * a genuine handler failure. + * + * Shared by the REST `/actions` route and the MCP `run_action` bridge so both + * surfaces address handlers identically. Before it was shared, REST rotated + * only the OBJECT and used the URL segment verbatim as the key — strictly + * weaker than MCP, and the reason the documented + * `POST /api/v1/actions/todo_task/complete_task` curl 404ed for every + * target-bound action while the Console's `target`-addressed call worked + * (and skipped the D4 gate on the way past). + * + * Reports a total miss as `{ dispatched: false }` rather than throwing, so + * neither surface has to pattern-match this function's own error message to + * tell "no handler anywhere" (a routing miss — 404) from "the handler ran and + * failed" (a business outcome, which propagates). Each surface words its own + * miss: REST 404s naming the routed object, MCP throws naming the action. + */ +export async function executeRegisteredAction(deps: ActionExecutionDeps, + ql: any, + objectName: string, + candidates: string[], + actionContext: any, +): Promise<{ dispatched: boolean; result?: any }> { + for (const obj of actionHandlerObjectKeys(objectName)) { + for (const key of candidates) { + try { + return { dispatched: true, result: await ql.executeAction(obj, key, actionContext) }; + } catch (err: any) { + if (!isActionNotRegisteredError(err)) throw err; // real handler failure → surface + } + } + } + return { dispatched: false }; +} + /** * True when the routed "object" is the object-less placeholder rather than a * real object — the canonical `'global'`, the legacy `'*'`, or nothing at all @@ -1004,7 +1136,7 @@ export function isObjectLessActionKey(objectName: string | undefined | null): bo */ export async function resolveRouteActionDeclaration(deps: ActionExecutionDeps, args: { ql: any; objectName: string; actionName: string; envId?: string }, -): Promise<{ action: any; obj: any } | undefined> { +): Promise<{ action: any; obj: any; degraded?: boolean; reason?: string }> { const { ql, objectName, actionName, envId } = args; let obj: any; @@ -1032,13 +1164,35 @@ export async function resolveRouteActionDeclaration(deps: ActionExecutionDeps, /* registry without an item lookup → fall through to the metadata service */ } + // [ADR-0110 D3] A miss and an OUTAGE are different facts. `load` answers + // `null` for both — a loader that throws is warn-logged and skipped — so + // reading its `null` as "no declaration, hence no gate to enforce" lets an + // unreachable metadata plane silently ungate every action it can't see. + // `loadDiagnosed` reports whether the answer is trustworthy; a service + // that predates it (or a test double) simply reports nothing degraded. + let degraded = false; + let reason: string | undefined; try { const meta: any = await deps.resolveService('metadata', envId); - const fromMeta: any = await meta?.load?.('action', actionName); - if (fromMeta && ownsRoute(fromMeta)) return { action: fromMeta, obj }; - } catch { - /* no metadata service on this kernel → no declaration to resolve */ + if (meta && typeof meta.loadDiagnosed === 'function') { + const diag: any = await meta.loadDiagnosed('action', actionName); + if (diag?.data && ownsRoute(diag.data)) return { action: diag.data, obj }; + if (diag?.degraded) { + degraded = true; + reason = Array.isArray(diag.errors) && diag.errors.length > 0 + ? diag.errors.join('; ') + : 'the metadata plane reported a loader failure'; + } + } else { + const fromMeta: any = await meta?.load?.('action', actionName); + if (fromMeta && ownsRoute(fromMeta)) return { action: fromMeta, obj }; + } + } catch (err: any) { + // `resolveService` swallows its own resolution failures, so reaching + // here means the metadata service itself threw while answering. + degraded = true; + reason = err?.message ?? String(err); } - return obj ? { action: undefined, obj } : undefined; + return { action: undefined, obj, degraded, reason }; } diff --git a/packages/runtime/src/action-reconciliation.test.ts b/packages/runtime/src/action-reconciliation.test.ts new file mode 100644 index 0000000000..8043ed5356 --- /dev/null +++ b/packages/runtime/src/action-reconciliation.test.ts @@ -0,0 +1,125 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0110 D5] Declaration ↔ executable reconciliation. + * + * ADR-0078 outlaws a declaration nothing executes (silently *inert*); ADR-0110 + * D3 outlaws an executable nothing declares (silently *ungoverned*). Together + * they are one invariant — everything declared runs, everything that runs is + * declared — and this reconciliation is what makes a violation visible at boot + * instead of when a caller happens to hit the route. + * + * The load-bearing property: reconciliation derives handler keys through the + * SAME {@link resolveActionHandlerKeys} the router dispatches through, so the + * inventory can never disagree with what the route actually does. A test that + * hard-coded its own key derivation would drift silently. + */ + +import { describe, it, expect } from 'vitest'; +import { reconcileActionRegistrations } from './action-execution.js'; + +const deps: any = { resolveService: () => undefined, getObjectQL: async () => undefined }; + +const reconcile = ( + registered: Array<{ objectName: string; actionName: string; package?: string }>, + declarations: Array<{ action: any; objectName: string }>, +) => reconcileActionRegistrations(deps, registered, declarations); + +describe('reconcileActionRegistrations — undeclared handlers (ADR-0110 D5)', () => { + it('reconciles a target-bound handler to its declaration', async () => { + // app-todo's real shape: registered under `completeTask`, declared as + // `complete_task`. The names differ, so a naive name-equality check + // would report a false orphan on every well-formed app. + const { undeclaredHandlers } = reconcile( + [{ objectName: 'todo_task', actionName: 'completeTask' }], + [{ objectName: 'todo_task', action: { name: 'complete_task', type: 'script', target: 'completeTask' } }], + ); + + expect(undeclaredHandlers).toEqual([]); + }); + + it('reports a handler no declaration covers', async () => { + const { undeclaredHandlers } = reconcile( + [ + { objectName: 'todo_task', actionName: 'completeTask' }, + { objectName: 'todo_task', actionName: 'secretRecalc', package: 'app:todo' }, + ], + [{ objectName: 'todo_task', action: { name: 'complete_task', type: 'script', target: 'completeTask' } }], + ); + + expect(undeclaredHandlers).toEqual([ + { objectName: 'todo_task', actionName: 'secretRecalc', package: 'app:todo' }, + ]); + }); + + it('reconciles a body action registered under its NAME', async () => { + const { undeclaredHandlers } = reconcile( + [{ objectName: 'todo_task', actionName: 'archive_task' }], + [{ objectName: 'todo_task', action: { name: 'archive_task', type: 'script', body: 'return 1;' } }], + ); + + expect(undeclaredHandlers).toEqual([]); + }); + + it('does not let another OBJECT\'s declaration cover a handler', async () => { + const { undeclaredHandlers } = reconcile( + [{ objectName: 'crm_contact', actionName: 'completeTask' }], + [{ objectName: 'todo_task', action: { name: 'complete_task', type: 'script', target: 'completeTask' } }], + ); + + expect(undeclaredHandlers).toHaveLength(1); + }); + + it('lets an object-less declaration cover an object-less handler', async () => { + const { undeclaredHandlers } = reconcile( + [{ objectName: 'global', actionName: 'logCall' }], + [{ objectName: '*', action: { name: 'log_call', type: 'script', target: 'logCall' } }], + ); + + expect(undeclaredHandlers).toEqual([]); + }); +}); + +describe('reconcileActionRegistrations — unbound declarations (ADR-0078 side)', () => { + it('reports a declared script action with no handler anywhere', async () => { + const { unboundDeclarations } = reconcile( + [], + [{ objectName: 'todo_task', action: { name: 'complete_task', type: 'script', target: 'completeTask' } }], + ); + + expect(unboundDeclarations).toEqual([{ objectName: 'todo_task', actionName: 'complete_task' }]); + }); + + it('does not report a body action — its handler is synthesized', async () => { + const { unboundDeclarations } = reconcile( + [], + [{ objectName: 'todo_task', action: { name: 'archive_task', type: 'script', body: 'return 1;' } }], + ); + + expect(unboundDeclarations).toEqual([]); + }); + + it('does not report non-script types — they dispatch elsewhere', async () => { + const { unboundDeclarations } = reconcile( + [], + [ + { objectName: 'crm_lead', action: { name: 'convert_lead', type: 'flow', target: 'wizard' } }, + { objectName: 'crm_lead', action: { name: 'open_docs', type: 'url', target: 'https://x.test' } }, + { objectName: 'todo_task', action: { name: 'defer_task', type: 'modal', target: 'defer_modal' } }, + ], + ); + + expect(unboundDeclarations).toEqual([]); + }); + + it('accepts a handler registered under the object-less key for an object-scoped declaration', async () => { + // `actionHandlerObjectKeys` rotates object → 'global' → '*' at + // dispatch, so a declaration bound to a global handler IS reachable. + const { unboundDeclarations } = reconcile( + [{ objectName: 'global', actionName: 'completeTask' }], + [{ objectName: 'todo_task', action: { name: 'complete_task', type: 'script', target: 'completeTask' } }], + ); + + expect(unboundDeclarations).toEqual([]); + }); +}); diff --git a/packages/runtime/src/app-plugin.test.ts b/packages/runtime/src/app-plugin.test.ts index 321dad2836..93f12c5b62 100644 --- a/packages/runtime/src/app-plugin.test.ts +++ b/packages/runtime/src/app-plugin.test.ts @@ -16,7 +16,16 @@ describe('AppPlugin', () => { }, registerService: vi.fn(), getService: vi.fn(), - getServices: vi.fn() + getServices: vi.fn(), + // `hook` / `trigger` are REQUIRED members of PluginContext. This + // double omitted them and got away with it only because every + // `ctx.hook` call site happened to be conditional (jobs register + // one only when the bundle declares jobs). ADR-0110 D5's governance + // inventory registers one unconditionally, which surfaced the gap — + // model the real interface rather than shrinking the code to fit + // an incomplete double. + hook: vi.fn(), + trigger: vi.fn() } as unknown as PluginContext; }); diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index 37c952c304..b12dae7eba 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -10,7 +10,12 @@ import { loadDisabledPackageIds } from './package-state-store.js'; import type { IMetadataService, II18nService } from '@objectstack/spec/contracts'; import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js'; import { hookBodyRunnerFactory, actionBodyRunnerFactory } from './sandbox/body-runner.js'; -import { GLOBAL_ACTION_OBJECT_KEY } from './action-execution.js'; +import { + GLOBAL_ACTION_OBJECT_KEY, + collectActionDeclarations, + reconcileActionRegistrations, + type ActionExecutionDeps, +} from './action-execution.js'; import { countServerTiming } from '@objectstack/observability'; /** @@ -685,6 +690,65 @@ export class AppPlugin implements Plugin { }); } + // ── [ADR-0110 D5] Action governance inventory ──────────────────── + // Reconcile the handler registry against the declarations, on + // `kernel:ready` so imperative `engine.registerAction(...)` calls in + // user code (which run after this bind) are counted. + // + // Since D3 an undeclared handler is REFUSED at dispatch. A hard + // failure with no inventory is a support ticket; with one it is a + // checklist, so this list is the whole point of shipping the refusal + // and the inventory in the same release. Best-effort and warn-only — + // a diagnostic must never be the reason a kernel fails to boot. + // A host whose context predates `hook` (or a partial test double) must + // not lose its kernel to a DIAGNOSTIC — the guard keeps this block's + // own promise, which registering unconditionally did not. + ctx.hook?.('kernel:ready', async () => { + try { + const engine: any = ctx.getService('objectql'); + if (!engine || typeof engine.listRegisteredActions !== 'function') return; + let meta: any; + try { meta = ctx.getService('metadata'); } catch { /* optional */ } + // The reconciliation reads no services of its own — it is pure + // over the two lists — so a thin deps shim is enough here. + const actionDeps: ActionExecutionDeps = { + resolveService: (name: string) => { try { return ctx.getService(name); } catch { return undefined; } }, + getObjectQL: async () => engine, + }; + const declarations = await collectActionDeclarations(actionDeps, meta); + const { undeclaredHandlers, unboundDeclarations } = reconcileActionRegistrations( + actionDeps, engine.listRegisteredActions(), declarations, + ); + if (undeclaredHandlers.length > 0) { + ctx.logger.warn( + '[action-governance] registered handlers with NO declaration — these are refused ' + + 'at dispatch (ADR-0110 D3); declare each one with `defineAction`, or set ' + + 'OS_ALLOW_UNDECLARED_ACTIONS=1 to run them during migration (removed in 18)', + { + appId, + count: undeclaredHandlers.length, + handlers: undeclaredHandlers.map((h) => `${h.objectName}:${h.actionName}`), + }, + ); + } + if (unboundDeclarations.length > 0) { + ctx.logger.warn( + '[action-governance] declared script actions with NO handler — a button wired to ' + + 'nothing (ADR-0078); add a `body`, or register a handler under the declared `target`', + { + appId, + count: unboundDeclarations.length, + actions: unboundDeclarations.map((d) => `${d.objectName}:${d.actionName}`), + }, + ); + } + } catch (err: any) { + ctx.logger.debug('[action-governance] inventory skipped', { + appId, error: err?.message ?? String(err), + }); + } + }); + // ── Auto-register declarative Background Jobs ──────────────────── // Jobs declared via `defineStack({ jobs })` are scheduled against the // running `IJobService` on `kernel:ready` (so the service plugin and diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 1282828ccf..ead2ea33bd 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -142,7 +142,7 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // read it. let actionSchema: any; let actionDef: any; - try { + { // Standalone declarations (ObjectQL registry artifacts / Studio- // authored `action` rows) resolve here too, so a route whose action // never appears inside an object definition is gated and dispatched @@ -155,12 +155,64 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string }); actionSchema = declaration?.obj; actionDef = declaration?.action; + + // [ADR-0110 D3] Resolution is a TRICHOTOMY, and only one branch may + // dispatch. This block used to be a `try { … } catch { /* no gate to + // enforce */ }`, which collapsed three states with opposite meanings + // into one fail-open path: a declaration that resolved, a metadata + // plane that could not answer, and an action that genuinely has no + // declaration all arrived as "ungated, run it". + // + // ── 2/3: the metadata plane could not answer ── + // An availability failure is not an authorization decision. Refusing + // is the same posture v17 already takes for a datasource that cannot + // connect (#3741) and a flow run with no trigger user (#3760): decline + // rather than degrade, because the alternative is that an outage + // quietly removes the gate an author declared. + if (declaration.degraded) { + return { + handled: true, + response: deps.error( + `Cannot verify the declaration for action '${actionName}' on '${objectName}' — ` + + `the metadata plane is unavailable (${declaration.reason ?? 'unknown failure'}). ` + + `Refusing rather than running it ungated.`, + 503, + ), + }; + } + + // ── 3/3: genuinely undeclared ── + // A handler with no declaration is invisible to every governance + // surface — ADR-0066 D4 has no `requiredPermissions` to read, ADR-0104 + // no param contract, ADR-0109 materialises no `action_` tool — + // yet it executes TRUSTED. Refuse, and say what to add. The valve is + // for an upgrade that cannot stop to declare one at 3am; it warns on + // every invocation and is slated for removal in 18. + if (!actionDef) { + if (!actionExec.undeclaredActionsAllowed(deps)) { + return { + handled: true, + response: deps.error( + `Action '${actionName}' on '${objectName}' has no declaration — ` + + `add \`defineAction({ name: '${actionName}', … })\`, or register the handler under a ` + + `declared action's \`target\`. Undeclared handlers cannot be permission-gated ` + + `(ADR-0110 D3); set OS_ALLOW_UNDECLARED_ACTIONS=1 to run it during migration.`, + 404, + ), + }; + } + console.warn( + `[action-governance] UNDECLARED action '${objectName}/${actionName}' executed under ` + + `OS_ALLOW_UNDECLARED_ACTIONS — it is ungated (no requiredPermissions, no param contract) ` + + `and invisible to the AI surface. Declare it; the valve is removed in 18.`, + ); + } + + // ── 1/3: declared → gate against it ── const gateError = actionExec.actionPermissionError(deps, actionDef, _context?.executionContext, objectName); if (gateError) { return { handled: true, response: deps.error(gateError, 403) }; } - } catch { - /* schema unresolved → no declared gate to enforce (handler-only action) */ } // [#3915] Action-TYPE dispatch. Per spec every non-`script` type @@ -277,24 +329,24 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string `(system-elevated context, RLS/FLS-bypassing) for user '${userFromAuth.id}'`, ); - // ── script/body dispatch ── - // Probe the routed object first, then the object-less keys (#3913): - // the canonical `'global'` both writers register under, then the legacy - // `'*'`. `executeAction` is an exact-string Map lookup with no wildcard - // semantics, so every candidate key has to be tried for real; only its - // "not registered" miss rotates, a genuine handler error propagates. - let dispatched = false; - let result: any; - for (const obj of actionExec.actionHandlerObjectKeys(objectName)) { - try { - result = await ql.executeAction(obj, actionName, actionContext); - dispatched = true; - break; - } catch (err: any) { - if (!actionExec.isActionNotRegisteredError(err)) throw err; - } - } - if (!dispatched) { + // ── script/body dispatch ── [ADR-0110 D2] "resolve, then address": + // the handler KEY is derived from the resolved declaration, not read + // off the URL. `app-plugin.ts` registers a body action under `name` + // while user code registers a target-bound one under `target`, so the + // URL segment matches the registration key only by luck — which is why + // the documented `/actions/todo_task/complete_task` curl used to 404 + // for every target-bound action. The candidate keys rotate across the + // object-key rotation (#3913) inside the shared helper the MCP + // `run_action` bridge also calls, so both surfaces address handlers + // identically. The routed URL segment stays as the last candidate for + // an UNDECLARED action, which has no declaration to derive from. + const dispatch = await actionExec.executeRegisteredAction( + deps, ql, objectName, + actionExec.resolveActionHandlerKeys(actionDef, actionName), + actionContext, + ); + const result = dispatch.result; + if (!dispatch.dispatched) { // No key carried a handler. That is a routing miss, not a server // fault — 404, and named after the ROUTED object rather than // whichever probe happened to run last (the old fallback reported diff --git a/packages/runtime/src/http-dispatcher.actions-global-key.test.ts b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts index 7254b3b053..f0a36f9343 100644 --- a/packages/runtime/src/http-dispatcher.actions-global-key.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts @@ -47,6 +47,15 @@ import { HttpDispatcher } from './http-dispatcher.js'; function makeDispatcher(opts: { registered?: Record any>; objectDef?: any; + /** + * [ADR-0110 D3] This file's axis is ADDRESSING, not governance: every case + * below asks "which key did the route probe, and what envelope came back". + * An undeclared action is refused since D3, so the double declares + * whatever name is asked for — a plain script action bound to its own name, + * which is the very key the URL segment used to be taken as, so the probe + * order these tests pin is unchanged. Pass `false` to exercise the refusal. + */ + declared?: boolean; } = {}) { const registered = opts.registered ?? {}; const executeAction = vi.fn(async (object: string, action: string, ctx: any) => { @@ -65,8 +74,16 @@ function makeDispatcher(opts: { find: vi.fn(async () => []), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), }; + const declareAny = opts.declared !== false; + const synthesize = (type: string, name: string) => + (declareAny && type === 'action') + ? { name, type: 'script', target: name, objectName: 'global' } + : null; const metadata: any = { - load: vi.fn(async () => null), + load: vi.fn(async (type: string, name: string) => synthesize(type, name)), + loadDiagnosed: vi.fn(async (type: string, name: string) => ({ + data: synthesize(type, name), degraded: false, errors: [], + })), listObjects: vi.fn(async () => (objectDef ? [objectDef] : [])), getObject: vi.fn(async () => objectDef), }; @@ -145,6 +162,22 @@ describe('REST /actions — object-less ("global") action key (#3913)', () => { expect(probed).toEqual(['global', '*']); }); + // [ADR-0110 D3] The fixture above declares whatever it is asked for so the + // addressing cases stay on their own axis. That must not be read as "the + // object-less route is exempt from governance" — it is not. + it('refuses an object-less action that has no declaration, before any probe', async () => { + const { dispatcher, executeAction } = makeDispatcher({ + registered: { 'global:log_call': () => ({ logged: true }) }, + declared: false, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(404); + expect(res.response.body.error.message).toMatch(/has no declaration/i); + expect(executeAction).not.toHaveBeenCalled(); + }); + it('prefers the object-specific handler over the global one', async () => { const { dispatcher } = makeDispatcher({ registered: { diff --git a/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts b/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts new file mode 100644 index 0000000000..07dc4731fd --- /dev/null +++ b/packages/runtime/src/http-dispatcher.actions-identity-addressing.test.ts @@ -0,0 +1,226 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * REST `/actions/:object/:action` — identity vs addressing (ADR-0110 D1/D2, + * #3935). + * + * The spec separates two fields that the route used to let bleed into each + * other: `name` is the machine identity (`SnakeCaseIdentifierSchema`), while + * `target` is a BINDING EXPRESSION — polymorphic per type (URL / script key / + * flow id / FormView name), `${param.X}`-interpolatable, and legitimately + * non-unique. The route resolved the DECLARATION from the URL segment as a + * name, but dispatched the HANDLER using the same segment as a registry key. + * + * For a target-bound script action those are different strings, so the two + * documented callers each worked on exactly the half the other broke: + * + * - the documented curl (`.../todo_task/complete_task`, the NAME) resolved + * the declaration — gate + params enforced — then 404ed, because the + * handler is registered under `completeTask`; + * - the Console (`target || name`, the KEY) dispatched fine, but resolved + * NO declaration, so ADR-0066 D4 `requiredPermissions` and the ADR-0104 + * param contract were silently skipped. + * + * These tests pin the D2 fix: identity is always `name`, and the handler key + * is DERIVED from the resolved declaration (the rotation the MCP bridge has + * always used), so the documented URL both gates and dispatches. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HttpDispatcher } from './http-dispatcher.js'; + +/** app-todo's real shape: declarative name ≠ handler registration key. */ +const targetBoundAction = { + name: 'complete_task', + label: 'Complete', + type: 'script', + target: 'completeTask', +}; + +/** + * A dispatcher whose engine registers handlers under `registeredKeys` only — + * `executeAction` throws the engine's exact "not registered" miss for any + * other key, so the key rotation is exercised for real rather than mocked. + */ +function makeDispatcher(opts: { + objectDef?: any; + registeredKeys?: Record; // objectKey -> handler keys + record?: any; +} = {}) { + const objectDef = opts.objectDef ?? { name: 'todo_task', actions: [targetBoundAction] }; + const registered = opts.registeredKeys ?? { todo_task: ['completeTask'] }; + const calls: Array<{ object: string; key: string }> = []; + + const executeAction = vi.fn(async (object: string, key: string, _ctx: any) => { + calls.push({ object, key }); + if (!registered[object]?.includes(key)) { + throw new Error(`Action '${key}' on object '${object}' not found`); + } + return { ran: key }; + }); + + const ql: any = { + executeAction, + getSchema: (name: string) => (name === objectDef.name ? objectDef : undefined), + registry: { + getObject: (name: string) => (name === objectDef.name ? objectDef : undefined), + getItem: () => undefined, + }, + find: vi.fn(async () => (opts.record ? [opts.record] : [])), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + }; + const metadata: any = { + load: vi.fn(async () => null), + listObjects: vi.fn(async () => [objectDef]), + getObject: vi.fn(async () => objectDef), + }; + const kernel: any = { + context: { + getService: (n: string) => + n === 'objectql' || n === 'data' ? ql : n === 'metadata' ? metadata : null, + }, + }; + return { dispatcher: new HttpDispatcher(kernel), executeAction, calls }; +} + +const ctxFor = (executionContext: any = { userId: 'u1', systemPermissions: [] }): any => ({ + request: {}, + environmentId: 'platform', + executionContext, +}); + +describe('REST /actions — identity is `name`, the handler key is derived (ADR-0110 D2, #3935)', () => { + it('dispatches the DOCUMENTED name-addressed curl to a target-registered handler', async () => { + // `content/docs/ui/actions.mdx` teaches exactly this URL. Before D2 it + // 404ed: the route used `complete_task` as the registry key, but the + // handler lives under `completeTask`. + const { dispatcher, calls } = makeDispatcher(); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', { recordId: 'task_1' }, ctxFor(), + ); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toMatchObject({ success: true, data: { ran: 'completeTask' } }); + expect(calls).toContainEqual({ object: 'todo_task', key: 'completeTask' }); + }); + + it('enforces the D4 capability gate on the name-addressed URL', async () => { + const gated = { ...targetBoundAction, requiredPermissions: ['task.manage'] }; + const { dispatcher, executeAction } = makeDispatcher({ + objectDef: { name: 'todo_task', actions: [gated] }, + }); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', { recordId: 'task_1' }, + ctxFor({ userId: 'u1', systemPermissions: [] }), + ); + + expect(res.response.status).toBe(403); + expect(res.response.body.error.message).toMatch(/task\.manage/); + expect(executeAction).not.toHaveBeenCalled(); + }); + + it('runs the gated action for a caller who holds the capability', async () => { + const gated = { ...targetBoundAction, requiredPermissions: ['task.manage'] }; + const { dispatcher } = makeDispatcher({ + objectDef: { name: 'todo_task', actions: [gated] }, + }); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', { recordId: 'task_1' }, + ctxFor({ userId: 'u1', systemPermissions: ['task.manage'] }), + ); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toMatchObject({ success: true, data: { ran: 'completeTask' } }); + }); + + it('prefers `name` when a BODY action declares a target (the AppPlugin registration key)', async () => { + // A body action is auto-registered under `name` even when it also + // carries a target, so the derivation must not blindly prefer target. + const bodyAction = { name: 'archive_task', type: 'script', target: 'someOtherThing', body: 'return 1;' }; + const { dispatcher, calls } = makeDispatcher({ + objectDef: { name: 'todo_task', actions: [bodyAction] }, + registeredKeys: { todo_task: ['archive_task'] }, + }); + + const res = await dispatcher.handleActions( + '/todo_task/archive_task', 'POST', {}, ctxFor(), + ); + + expect(res.response.status).toBe(200); + expect(calls[0]).toEqual({ object: 'todo_task', key: 'archive_task' }); + }); + + it('still resolves a handler registered under the declarative name', async () => { + const { dispatcher } = makeDispatcher({ + registeredKeys: { todo_task: ['complete_task'] }, + }); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', {}, ctxFor(), + ); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toMatchObject({ success: true, data: { ran: 'complete_task' } }); + }); + + it('rotates to the object-less registration keys for a global handler', async () => { + const { dispatcher, calls } = makeDispatcher({ + registeredKeys: { global: ['completeTask'] }, + }); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', {}, ctxFor(), + ); + + expect(res.response.status).toBe(200); + expect(calls).toContainEqual({ object: 'global', key: 'completeTask' }); + }); + + it('404s naming the ROUTED object when no candidate key carries a handler', async () => { + const { dispatcher } = makeDispatcher({ registeredKeys: {} }); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', {}, ctxFor(), + ); + + expect(res.response.status).toBe(404); + expect(res.response.body.error.message).toContain("'todo_task'"); + }); + + it('propagates a genuine handler failure instead of rotating past it', async () => { + // The rotation must only advance on the engine's "not registered" + // miss; a handler that throws its own error is a business outcome. + const objectDef = { name: 'todo_task', actions: [targetBoundAction] }; + const executeAction = vi.fn(async (_o: string, key: string) => { + if (key === 'completeTask') throw new Error('task already closed'); + throw new Error(`Action '${key}' on object 'todo_task' not found`); + }); + const ql: any = { + executeAction, + getSchema: () => objectDef, + registry: { getObject: () => objectDef, getItem: () => undefined }, + find: vi.fn(async () => []), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + }; + const kernel: any = { + context: { + getService: (n: string) => + n === 'objectql' || n === 'data' ? ql + : n === 'metadata' ? { load: async () => null, listObjects: async () => [objectDef] } + : null, + }, + }; + const dispatcher = new HttpDispatcher(kernel); + + const res = await dispatcher.handleActions( + '/todo_task/complete_task', 'POST', {}, ctxFor(), + ); + + // Business failure reports in the payload (the route's long-standing + // wire contract), NOT as a 404 routing miss. + expect(res.response.status).toBe(200); + expect(res.response.body.data).toMatchObject({ success: false, error: 'task already closed' }); + }); +}); diff --git a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts index 08405e3d03..b0ee5aa9f8 100644 --- a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts @@ -40,6 +40,8 @@ function makeDispatcher(opts: { standaloneAction?: any; registryAction?: any; record?: any; + /** Simulate a metadata plane that cannot answer (ADR-0110 D3). */ + metadataDegraded?: string; } = {}) { const executeAction = vi.fn(async () => ({ ran: 'script' })); const objectDef = opts.objectDef ?? { name: 'crm_lead', actions: [] }; @@ -57,6 +59,14 @@ function makeDispatcher(opts: { const metadata: any = { load: vi.fn(async (type: string, name: string) => type === 'action' && opts.standaloneAction?.name === name ? opts.standaloneAction : null), + // The real MetadataManager reports whether a `null` is a clean miss or + // an unanswerable one (every loader threw) — the D3 trichotomy needs + // that distinction, so the double carries it too. + loadDiagnosed: vi.fn(async (type: string, name: string) => ({ + data: type === 'action' && opts.standaloneAction?.name === name ? opts.standaloneAction : null, + degraded: Boolean(opts.metadataDegraded), + errors: opts.metadataDegraded ? [opts.metadataDegraded] : [], + })), listObjects: vi.fn(async () => [objectDef]), getObject: vi.fn(async () => objectDef), }; @@ -340,15 +350,57 @@ describe('REST /actions — script dispatch is unchanged (#3915 regression guard expect(res.response.body.data).toEqual({ success: true, data: { ran: 'script' } }); }); - it('still runs an UNDECLARED action through the handler registry (handler-only actions)', async () => { - // `engine.registerAction(...)` with no metadata declaration anywhere — - // the type dispatch must not turn these into a 400. + // [ADR-0110 D3] An UNDECLARED action used to run here, ungated — this test + // asserted exactly that. A handler with no declaration has no + // `requiredPermissions` to enforce, no param contract, and materialises no + // `action_` tool, yet it executes TRUSTED; it now refuses. The valve + // test below is the only path that still runs it. + it('refuses an UNDECLARED action with a prescriptive error instead of running it ungated', async () => { const { dispatcher, executeAction } = makeDispatcher({ objectDef: { name: 'crm_lead', actions: [] } }); const res = await dispatcher.handleActions('/crm_lead/handler_only', 'POST', {}, ctxFor()); - expect(executeAction).toHaveBeenCalledTimes(1); - expect(res.response.body.data.success).toBe(true); + expect(res.response.status).toBe(404); + expect(res.response.body.error.message).toMatch(/has no declaration/i); + expect(res.response.body.error.message).toMatch(/defineAction\(\{ name: 'handler_only'/); + expect(res.response.body.error.message).toMatch(/OS_ALLOW_UNDECLARED_ACTIONS=1/); + expect(executeAction).not.toHaveBeenCalled(); + }); + + it('runs an UNDECLARED action when the migration valve is set, warning every time', async () => { + const prev = process.env.OS_ALLOW_UNDECLARED_ACTIONS; + process.env.OS_ALLOW_UNDECLARED_ACTIONS = '1'; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const { dispatcher, executeAction } = makeDispatcher({ objectDef: { name: 'crm_lead', actions: [] } }); + + const res = await dispatcher.handleActions('/crm_lead/handler_only', 'POST', {}, ctxFor()); + + expect(executeAction).toHaveBeenCalledTimes(1); + expect(res.response.body.data.success).toBe(true); + expect(warn).toHaveBeenCalledWith(expect.stringMatching(/UNDECLARED action 'crm_lead\/handler_only'/)); + } finally { + warn.mockRestore(); + if (prev === undefined) delete process.env.OS_ALLOW_UNDECLARED_ACTIONS; + else process.env.OS_ALLOW_UNDECLARED_ACTIONS = prev; + } + }); + + // [ADR-0110 D3] An unreachable metadata plane must not read as "no + // declaration, hence no gate" — an availability failure would silently + // widen access. Same posture as v17's datasource-that-cannot-connect. + it('503s rather than running ungated when the metadata plane cannot answer', async () => { + const { dispatcher, executeAction } = makeDispatcher({ + objectDef: { name: 'crm_lead', actions: [] }, + metadataDegraded: 'database-loader: ECONNREFUSED', + }); + + const res = await dispatcher.handleActions('/crm_lead/mark_done', 'POST', {}, ctxFor()); + + expect(res.response.status).toBe(503); + expect(res.response.body.error.message).toMatch(/metadata plane is unavailable/i); + expect(res.response.body.error.message).toContain('ECONNREFUSED'); + expect(executeAction).not.toHaveBeenCalled(); }); }); @@ -394,10 +446,15 @@ describe('REST /actions — standalone declarations (#3915)', () => { automation: { execute }, }); - await dispatcher.handleActions('/crm_contact/convert_lead', 'POST', {}, ctxFor()); + const res = await dispatcher.handleActions('/crm_contact/convert_lead', 'POST', {}, ctxFor()); + // Another object's declaration must not gate or dispatch this route. expect(execute).not.toHaveBeenCalled(); - expect(executeAction).toHaveBeenCalledTimes(1); // falls through to the registry, as before + // [ADR-0110 D3] It used to fall through to the registry and run + // ungated; with no declaration OWNED BY THIS ROUTE there is nothing to + // enforce, so it refuses. + expect(res.response.status).toBe(404); + expect(executeAction).not.toHaveBeenCalled(); }); it('enforces the ADR-0066 D4 capability gate on a standalone declaration too', async () => {