diff --git a/.changeset/actions-global-key-and-failure-status.md b/.changeset/actions-global-key-and-failure-status.md new file mode 100644 index 0000000000..ddc981b480 --- /dev/null +++ b/.changeset/actions-global-key-and-failure-status.md @@ -0,0 +1,55 @@ +--- +"@objectstack/runtime": minor +"@objectstack/client": minor +--- + +fix(actions): reach global actions at their real registration key, and 404 an action that never dispatched (#3913) + +**1 — the registration key and the lookup key disagreed.** Both writers +register an objectName-less action under the literal `'global'`: `AppPlugin` +(`action.object || 'global'`) and `ObjectQLPlugin.actionObjectKey`. The REST +route's fallback probed `'*'`, and `engine.executeAction` is an exact-string +`Map` lookup with no wildcard semantics — so the probe could only ever miss: + +``` +Action 'log_call' on object '*' not found +``` + +`POST /api/v1/actions/global/log_call` worked by **accident** (the path segment +happened to spell the registration key); `POST /api/v1/actions//log_call` never +worked at all, and neither did falling back from an object-scoped route to a +global handler. `'global'` is now the canonical key +(`GLOBAL_ACTION_OBJECT_KEY`), the probe order is +`[, 'global', '*']` for both the REST route and the MCP +`run_action` bridge (`actionHandlerObjectKeys` — one list, two surfaces), and a +single-segment path (`/actions//:action`) routes at `'global'` instead of +400-ing. A handler registered directly under `'*'` still resolves; the doc +comments that called `'global'` a "wildcard" are corrected at every site. + +**2 — "no such action" was reported as a success.** The not-found exit called +`deps.success(...)`, which always emits `{status: 200, body: {success: true, +data}}`, so a request naming an action that does not exist came back as: + +```json +{"success":true,"data":{"success":false,"error":"Action 'log_call' on object '*' not found"}} +``` + +Every caller that did not hand-unwrap the INNER envelope read the outer +`success: true` and reported a success that never happened — including the +shipped console, which showed a green toast (fixed on that side in +objectui#2963). Nothing **dispatched** there, so it is a **404** now, joining +the answers this route already gives a status: 403 denied, 400 wrong action +type, 503 unavailable. The miss also names the **routed** object rather than +whichever probe ran last (the old fallback said `on object '*'`, an object the +caller never asked for). + +A handler that **ran and rejected** is unchanged: HTTP 200 with +`data: {success: false, error, code?, fields?}`. That is a business outcome, +not a transport error, and #3937 pins it. The line is "did a handler run" — +below it the payload, above it the status. + +`client.actions.invoke` / `invokeGlobal` still do **not** throw. `client.fetch` +throws on every non-2xx, so `invoke` now catches and folds a dispatch failure +into the same `{ success, data?, error? }` result with `error` as a plain +string — otherwise the routes that just gained a status would have started +propagating exceptions into callers that only ever checked `result.success`. diff --git a/content/docs/api/client-sdk.mdx b/content/docs/api/client-sdk.mdx index 81cfd53a79..dad5eabba5 100644 --- a/content/docs/api/client-sdk.mdx +++ b/content/docs/api/client-sdk.mdx @@ -391,16 +391,18 @@ if (run.status === 'paused') { await client.automation.getScreen('convert_lead', runId); // Actions — Invoke server-registered action handlers -// (engine.registerAction). The result envelope is the HANDLER's own -// `{ success, data | error }` — a business failure comes back as -// `success: false` + `error` instead of a thrown exception, so it can be -// surfaced as a toast. +// (engine.registerAction). The result is `{ success, data | error }` — a +// failure comes back as `success: false` + `error` instead of a thrown +// exception, so it can be surfaced as a toast. This surface is the one place +// a non-2xx does NOT throw: a handler that rejects answers 200 with the inner +// envelope, a request that never dispatched answers 404 / 403 / 400 / 503, +// and the SDK folds both into the same result. const res = await client.actions.invoke('crm_lead', 'convert', { recordId, params: { create_opportunity: true }, }); if (!res.success) console.warn(res.error); -// Global (object-less) actions dispatch to the wildcard handler: +// Global (object-less) actions dispatch to the server's 'global' handler key: await client.actions.invokeGlobal('nightly_cleanup', { params: { dryRun: true } }); // API Keys — the raw secret is returned exactly ONCE; store it immediately. diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index 53abf17252..6a12b4a3fe 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -229,11 +229,21 @@ curl -b cookies.txt -X POST \ https://your-app.example.com/api/v1/actions/todo_task/complete_task \ -H "Content-Type: application/json" \ -d '{ "recordId": "rec_123", "params": {} }' -# → { "success": true, "data": ... } (errors: { "success": false, "error": ... }) +# → 200 { "success": true, "data": { "success": true, "data": ... } } ``` -Global (object-less) actions post to `/api/v1/actions/global/:action`. For -credentials, see [API Authentication](/docs/api#authentication). +Failures split by **whether a handler ran**: + +- **It ran and rejected** — HTTP **200**, `data: { "success": false, "error", + "code"?, "fields"? }`. An action that fails is a normal business outcome, not + a transport error, so it rides the payload. Always branch on `data.success`. +- **It never dispatched** — a real status: **404** no such action, **403** + denied, **400** wrong action type or a param-contract violation, **503** + unavailable — with `{ "success": false, "error": { "message", "code" } }`. + +Global (object-less) actions post to `/api/v1/actions/global/:action`, or to +`/api/v1/actions//:action` with the object segment left empty. For credentials, +see [API Authentication](/docs/api#authentication). The endpoint dispatches on the **declared `type`**, exactly like the MCP `run_action` tool — so the same URL invokes a script handler or a flow: diff --git a/packages/client/src/actions-surface.test.ts b/packages/client/src/actions-surface.test.ts index 3f2a627844..730117f4f2 100644 --- a/packages/client/src/actions-surface.test.ts +++ b/packages/client/src/actions-surface.test.ts @@ -62,7 +62,7 @@ describe('client.actions', () => { expect(JSON.parse(fetchMock.mock.calls[0][1].body)).toEqual({ params: {} }); }); - it('invokeGlobal targets the wildcard shape /actions/global/:action', async () => { + it('invokeGlobal targets the object-less shape /actions/global/:action', async () => { const { client, fetchMock } = createMockClient({ success: true, data: { success: true } }); await client.actions.invokeGlobal('nightly_cleanup', { params: { dryRun: true } }); expect(String(fetchMock.mock.calls[0][0])).toBe( @@ -72,9 +72,10 @@ describe('client.actions', () => { }); it('surfaces the handler business failure without throwing (inner success:false)', async () => { - // The dispatcher reports handler throws as HTTP 200 with an inner - // `{ success:false, error }` — a toastable business failure, not a - // transport error (http-dispatcher.handleActions catch branch). + // A handler that RAN and rejected is a business outcome, reported in + // the payload at HTTP 200 — unchanged by #3913, which only moved the + // DISPATCH failures to a status. The inner envelope is authoritative + // on a 2xx. const { client } = createMockClient({ success: true, data: { success: false, error: 'Lead is already converted' }, @@ -83,4 +84,32 @@ describe('client.actions', () => { expect(result.success).toBe(false); expect(result.error).toBe('Lead is already converted'); }); + + it('reports a dispatch failure (404) as the same non-throwing envelope (#3913)', async () => { + // An unregistered action never dispatched, so the server answers a real + // status with `{success:false, error:{message,code}}`. `client.fetch` + // throws on any non-2xx, but this surface's contract is to REPORT, not + // throw — callers toast `result.error` rather than wrapping every call + // in a try/catch, and that must not change just because these routes + // gained a status. + const { client } = createMockClient( + { success: false, error: { message: "Action 'log_call' on object 'global' not found", code: 404 } }, + 404, + ); + const result = await client.actions.invokeGlobal('log_call'); + expect(result.success).toBe(false); + // The message, not `[object Object]` — the error body's `error` is an + // object on this shape. + expect(result.error).toBe("Action 'log_call' on object 'global' not found"); + }); + + it('reports a 403 denial the same way', async () => { + const { client } = createMockClient( + { success: false, error: { message: 'Missing capability can_convert', code: 403 } }, + 403, + ); + const result = await client.actions.invoke('crm_lead', 'convert'); + expect(result.success).toBe(false); + expect(result.error).toBe('Missing capability can_convert'); + }); }); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 29a19e8a1f..b520ac68a5 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -312,6 +312,38 @@ async function* parseEventStream(res: Response): AsyncIterable { yield* emit(decoder.decode()); } +/** Best human-readable text for an action failure, whatever shape carried it. */ +function actionErrorMessage(e: any): string { + if (typeof e === 'string' && e) return e; + if (typeof e?.message === 'string' && e.message) return e.message; + return 'Action failed'; +} + +/** + * Fold a 2xx `POST /api/v1/actions/...` body into the `{ success, data?, + * error? }` shape `client.actions.invoke` has always returned. + * + * `payload` is what `unwrapResponse` produced — the INNER envelope. Two shapes + * arrive on a 2xx and a caller must not have to tell them apart: + * + * - `{ success: true, data }` — the action ran and returned. + * - `{ success: false, error, code?, fields? }` — the handler RAN and + * rejected. That is a business outcome, so it rides the payload at HTTP + * 200 (#3937) and the inner envelope is authoritative. + * + * The non-2xx case never reaches here: those are DISPATCH failures (404 / 403 + * / 400 / 503), `fetch` throws on them, and `invoke` catches — this surface + * does not throw either way. + */ +function normalizeActionResult(payload: any): { success: boolean; data?: T; error?: string } { + if (payload && typeof payload === 'object' && 'success' in payload) { + return payload.success === false + ? { success: false, error: actionErrorMessage(payload.error) } + : { success: true, data: payload.data as T }; + } + return { success: true, data: payload as T }; +} + export class ObjectStackClient { private baseUrl: string; private token?: string; @@ -2879,16 +2911,25 @@ export class ObjectStackClient { * * The dispatcher accepts the record id either in the URL or in the body; * this client always sends it in the body (`{ recordId, params }`), which - * both server shapes honor. The HTTP envelope is - * `{ success, data: { success, data | error } }` — the OUTER envelope is - * transport success; the INNER `success:false` + `error` is the action - * handler's own (business) failure, deliberately not thrown so callers can - * surface it as a toast rather than a crash. + * both server shapes honor. + * + * Result shape is `{ success, data | error }` and this surface does NOT + * throw — a failed business action is a toast, not a crash. + * + * Two server answers fold into that one result: + * - a handler that RAN and rejected → HTTP 200 with the inner + * `{success: false, error}` (unchanged — a business outcome is reported + * in the payload, not as a transport error); + * - a request that never DISPATCHED → a real status (404 unregistered, 403 + * denied, 400 wrong action type, 503 unavailable) with + * `{success: false, error: {message, code}}`. `fetch` throws on those, so + * `invoke` catches and reports instead of propagating — #3913, where an + * unregistered action came back as a 200 and read as a success. */ actions = { /** * Invoke a server-registered action on an object. - * Falls back to the server's wildcard ('*') handler when no + * Falls back to the server's object-less ('global') handler when no * object-specific handler is registered. */ invoke: async ( @@ -2896,20 +2937,32 @@ export class ObjectStackClient { actionName: string, opts?: { recordId?: string; params?: Record }, ): Promise<{ success: boolean; data?: T; error?: string }> => { - const res = await this.fetch( - `${this.baseUrl}/api/v1/actions/${encodeURIComponent(objectName)}/${encodeURIComponent(actionName)}`, - { - method: 'POST', - body: JSON.stringify({ recordId: opts?.recordId, params: opts?.params ?? {} }), - }, - ); - return this.unwrapResponse(res) as Promise<{ success: boolean; data?: T; error?: string }>; + try { + const res = await this.fetch( + `${this.baseUrl}/api/v1/actions/${encodeURIComponent(objectName)}/${encodeURIComponent(actionName)}`, + { + method: 'POST', + body: JSON.stringify({ recordId: opts?.recordId, params: opts?.params ?? {} }), + }, + ); + return normalizeActionResult(await this.unwrapResponse(res)); + } catch (err: any) { + // [#3913] `fetch` throws on every non-2xx, and a DISPATCH + // failure is a non-2xx now (404 unregistered / 403 denied / 400 + // wrong type / 503 unavailable) — but this surface's contract is + // to report, not throw, so it must not start propagating on the + // routes that just gained a status. `fetch` has already lifted + // the server's human-readable message onto `err.message` (with + // `err.code` / `err.httpStatus` kept for programmatic use), so + // the toast text is unchanged. + return { success: false, error: actionErrorMessage(err) }; + } }, /** * Invoke a global (object-less) action — the server's - * `POST /actions/global/:action` shape, dispatched to the wildcard - * handler registry. + * `POST /actions/global/:action` shape, dispatched to the `'global'` + * handler-registry key. */ invokeGlobal: async ( actionName: string, diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index f88b576def..04c2169cac 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -1340,7 +1340,14 @@ export class ObjectQLPlugin implements Plugin { * Resolve the engine object key an action registers under. Standalone * `action` metadata declares `objectName` (spec `ActionSchema`); bundle * collectors attach `object`; object-less actions register under the - * `'global'` wildcard key, matching AppPlugin's bundle registration. + * `'global'` key, matching AppPlugin's bundle registration. + * + * `'global'` is the CANONICAL object-less key (#3913) — not a wildcard. + * `executeAction` is an exact-string `Map` lookup, so every reader has to + * probe this literal; the runtime's `actionHandlerObjectKeys` does, and the + * runtime's `standaloneActionObjectName` must stay in lockstep with this + * method or the declaration the MCP surface resolves stops matching the + * handler that actually runs. */ private actionObjectKey(action: any): string { if (typeof action?.objectName === 'string' && action.objectName.length > 0) return action.objectName; diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index ca2eca8a3e..4e51e7838e 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -421,7 +421,7 @@ export async function dispatchFlowAction(deps: ActionExecutionDeps, // `triggerData` envelope). const result: any = await automation.execute(action.target, { record, - ...(objectName !== 'global' ? { object: objectName } : {}), + ...(isObjectLessActionKey(objectName) ? {} : { object: objectName }), userId: ec?.userId, ...(Array.isArray(ec?.positions) && ec.positions.length ? { positions: ec.positions } : {}), ...(Array.isArray(ec?.permissions) && ec.permissions.length ? { permissions: ec.permissions } : {}), @@ -429,6 +429,10 @@ export async function dispatchFlowAction(deps: ActionExecutionDeps, params: seedFlowActionParams(deps, action, { objectName, record, params, recordId }), }); if (result && typeof result === 'object' && 'success' in result && result.success === false) { + // The flow RAN and rejected — a business outcome, so the REST route + // reports it in the payload (`{success: false, error}`, HTTP 200), the + // same as a script body that throws. Only a route that never dispatched + // gets a status (#3913). throw new Error(`Flow '${action.target}' failed: ${result.error ?? 'unknown error'}`); } return result ?? null; @@ -753,7 +757,7 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, // Load the subject record under RLS when row-context (engages the same // permission path as get_record — an unseen record reads as not-found). let record: Record = {}; - if (recordId && objectName !== 'global') { + if (recordId && !isObjectLessActionKey(objectName)) { try { const got: any = await callData('get', { object: objectName, id: recordId }, driver, envId, ec); if (got?.record) record = got.record; @@ -811,20 +815,19 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, }; // Handler key: body-based actions register under `name` (AppPlugin); // target-bound script actions register under `target` (user code). - // Probe both, then the wildcard object, distinguishing the engine's - // "action not registered" miss from a genuine handler error. + // 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, ); - const notRegistered = (err: any) => /Action '.+' on object '.+' not found/i.test(String(err?.message ?? err)); - for (const obj of [objectName, '*']) { + 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 (!notRegistered(err)) throw err; // real handler failure → surface + if (!isActionNotRegisteredError(err)) throw err; // real handler failure → surface } } } @@ -921,7 +924,59 @@ export async function collectActionDeclarations(deps: ActionExecutionDeps, export function standaloneActionObjectName(deps: ActionExecutionDeps, action: any): string { if (typeof action?.objectName === 'string' && action.objectName.length > 0) return action.objectName; if (typeof action?.object === 'string' && action.object.length > 0) return action.object; - return 'global'; + return GLOBAL_ACTION_OBJECT_KEY; +} + +/** + * The engine object key an object-LESS ("global") action registers under. + * + * Canonical since #3913, and it is `'global'` because that is what the two + * writers have always written: `AppPlugin` (`action.object || 'global'`) and + * `ObjectQLPlugin.actionObjectKey`. `engine.executeAction` is an exact-string + * `Map` lookup with no wildcard semantics, so the READERS have to probe the + * same literal — before this, the REST route and the MCP bridge both rotated + * to `'*'`, which nothing ever registers, and every global action came back as + * `Action '' on object '*' not found`. + */ +export const GLOBAL_ACTION_OBJECT_KEY = 'global'; + +/** + * The engine object keys to probe, in order, for a route's action handler. + * + * The routed object first, then the canonical object-less key (#3913), then + * the legacy `'*'` — kept last so a handler that user code registered directly + * against the wildcard still resolves. Deduped, so a request routed AT + * `/actions/global/:action` probes `'global'` exactly once. + */ +export function actionHandlerObjectKeys(objectName: string): string[] { + return [objectName, GLOBAL_ACTION_OBJECT_KEY, '*'].filter( + (k, i, all) => all.indexOf(k) === i, + ); +} + +/** + * True when the error is `executeAction`'s "no such key in the registry" miss + * rather than a genuine handler failure — the difference between rotating to + * the next candidate key and surfacing the error to the caller. + * + * Matched on the message because that is all `executeAction` throws + * (`engine.ts`: `Action '' on object '' not found`). A handler + * whose own message happens to read that way is misclassified as a miss; the + * rotation ends in a 404 either way, so the blast radius is the status code. + */ +export function isActionNotRegisteredError(err: any): boolean { + return /Action '.+' on object '.+' not found/i.test(String(err?.message ?? err)); +} + +/** + * True when the routed "object" is the object-less placeholder rather than a + * real object — the canonical `'global'`, the legacy `'*'`, or nothing at all + * (`POST /actions//:action`). Callers use it to skip work that only makes + * sense for a record-scoped action: loading `ctx.record`, and naming an + * `object` on the flow-automation context. + */ +export function isObjectLessActionKey(objectName: string | undefined | null): boolean { + return !objectName || objectName === GLOBAL_ACTION_OBJECT_KEY || objectName === '*'; } /** @@ -943,9 +998,9 @@ export function standaloneActionObjectName(deps: ActionExecutionDeps, action: an * standalone `defineAction` declaration was invisible here, so its declared * `type` (and its `requiredPermissions`) were never read on the REST path * even though the MCP path honoured both. A standalone declaration is - * accepted only when it belongs to the routed object or to the `'global'` - * wildcard — the same `:` / `'*'` key rotation the handler - * lookup performs. + * accepted only when it belongs to the routed object or is object-less — the + * same key rotation {@link actionHandlerObjectKeys} performs for the handler + * lookup. */ export async function resolveRouteActionDeclaration(deps: ActionExecutionDeps, args: { ql: any; objectName: string; actionName: string; envId?: string }, @@ -967,7 +1022,7 @@ export async function resolveRouteActionDeclaration(deps: ActionExecutionDeps, const ownsRoute = (action: any): boolean => { const owner = standaloneActionObjectName(deps, action); - return owner === objectName || owner === 'global'; + return owner === objectName || isObjectLessActionKey(owner); }; try { diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index c73ae4e3dc..651ae366dd 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -10,6 +10,7 @@ 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 { countServerTiming } from '@objectstack/observability'; /** @@ -651,10 +652,14 @@ export class AppPlugin implements Plugin { for (const action of actions) { const handler = actionBodyRunner(action); if (!handler) continue; + // Object-less actions register under the canonical + // `'global'` key (#3913) — the literal every reader probes + // (`actionHandlerObjectKeys`), since `executeAction` is an + // exact-string Map lookup with no wildcard semantics. const objectKey = typeof action.object === 'string' && action.object.length > 0 ? action.object - : 'global'; + : GLOBAL_ACTION_OBJECT_KEY; try { ql.registerAction(objectKey, action.name, handler, `app:${appId}`); registered++; diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 8ea3a5ce2f..f483b7b04e 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -12,7 +12,8 @@ * * - `POST /actions/:object/:action` — record-scoped action * - `POST /actions/:object/:action/:recordId` — record-scoped action with id in URL - * - `POST /actions/global/:action` — wildcard ("*") action + * - `POST /actions/global/:action` — object-less ("global") action + * - `POST /actions//:action` — object-less action, empty segment * * The route dispatches on the declared action TYPE (#3915), the same way the * MCP `run_action` bridge does — `script` through the handler registry, @@ -20,6 +21,19 @@ * a spec-faithful REST/SDK caller could not invoke a `type: 'flow'` action at * all: it fell through to the registry and came back as * `Action '' on object '*' not found`. + * + * #3913 closed two holes: + * - object-less actions register under `'global'` (AppPlugin + + * ObjectQLPlugin) but the handler fallback probed `'*'`, which nothing + * registers — so a global action was reachable only by spelling `global` + * into the URL, and `POST /actions//:action` never resolved at all; + * - an action that no key carried came back as HTTP 200 + * `{success: true, data: {success: false, error: "… not found"}}`, so a + * caller that did not hand-inspect the INNER envelope read it as a success. + * Nothing DISPATCHED there, so it is a 404 now — joining the pre-dispatch + * answers this route already gives a status (403 denied, 400 wrong type, + * 503 unavailable). A handler that RAN and rejected still reports in the + * payload, unchanged: that is a business outcome, not a transport error. */ import * as actionExec from '../action-execution.js'; @@ -43,7 +57,8 @@ export function createActionsDomain(deps: DomainHandlerDeps): DomainRoute { * * - `POST /actions/:object/:action` — record-scoped action * - `POST /actions/:object/:action/:recordId` — record-scoped action with id in URL - * - `POST /actions/global/:action` — wildcard ("*") action + * - `POST /actions/global/:action` — object-less ("global") action + * - `POST /actions//:action` — object-less action, empty segment * * Body shape: `{ recordId?: string, params?: Record }`. * The handler is invoked with an `ActionContext` of: @@ -71,12 +86,18 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string return { handled: true, response: deps.error('Method not allowed', 405) }; } const parts = path.split('/').filter(Boolean); - if (parts.length < 2) { + if (parts.length < 1) { return { handled: true, response: deps.error('Path must be /actions/:object/:action', 400) }; } - const objectName = parts[0]; - const actionName = parts[1]; - const recordIdFromPath = parts[2]; + // A single segment is an OBJECT-LESS action (#3913): `POST /actions//log_call` + // is what an SDK that has no object to name emits, and `filter(Boolean)` + // already ate the empty segment. Route it at the canonical `'global'` key + // rather than 400-ing — before this it was the one global-action shape that + // could never work, and `/actions/global/log_call` only worked by accident + // (the literal path segment happened to match the registration key). + const objectName = parts.length > 1 ? parts[0] : actionExec.GLOBAL_ACTION_OBJECT_KEY; + const actionName = parts.length > 1 ? parts[1] : parts[0]; + const recordIdFromPath = parts.length > 1 ? parts[2] : undefined; // Resolve project scope so the right project kernel's ObjectQL is // used (single-environment default when unset), then let the host @@ -152,13 +173,6 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string return { handled: true, response: deps.error(actionExec.flowActionUnavailableError(actionDef), 503) }; } - // Resolve the handler — fall back to wildcard '*' if the object-specific key is missing. - // Since engine.executeAction throws when the key is unknown, we probe via the internal - // map by attempting the call inside a try/catch and rotating to '*'. - const tryExecute = async (obj: string) => { - return ql.executeAction(obj, actionName, actionContext); - }; - const reqBody = body && typeof body === 'object' ? body : {}; const recordId = recordIdFromPath ?? reqBody.recordId; const reqParams = (reqBody.params && typeof reqBody.params === 'object') ? reqBody.params : {}; @@ -173,7 +187,7 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // Load the record (best-effort) so handlers can rely on `ctx.record`. let record: Record = {}; - if (recordId && objectName !== 'global') { + if (recordId && !actionExec.isObjectLessActionKey(objectName)) { try { const got = await actionExec.callData(deps, 'get', { object: objectName, id: recordId }, _context.dataDriver, _context.environmentId, _context.executionContext); if (got?.record) record = got.record; @@ -251,18 +265,32 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string ); // ── script/body dispatch ── - // Try object-specific first; on "not found" error, fall back to wildcard. + // 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; - try { - result = await tryExecute(objectName); - } catch (err: any) { - const msg = String(err?.message ?? err ?? ''); - if (/not found/i.test(msg) && objectName !== '*') { - result = await tryExecute('*'); - } else { - throw err; + 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) { + // 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 + // `on object '*'`, an object the caller never asked for). + return { + handled: true, + response: deps.error(`Action '${actionName}' on object '${objectName}' not found`, 404), + }; + } return { handled: true, response: deps.success({ success: true, data: result }) }; } catch (err: any) { const full = err?.message ?? String(err); @@ -286,6 +314,13 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // every existing caller branches on `data.success`; turning this into a // 4xx would be a wire-contract break in exchange for a strictly additive // fix. + // + // [#3913] That reasoning covers a handler that RAN and rejected — it + // does NOT cover a route that never dispatched at all. An unregistered + // action has no business outcome to report, so it exits above as a 404, + // alongside the pre-dispatch gates this route already answers with a + // status (403 denied, 400 wrong type, 503 unavailable). The line is + // "did a handler run": below it, the payload; above it, the status. const code: unknown = err?.code; const fields: unknown = err?.fields; return { diff --git a/packages/runtime/src/http-dispatcher.actions-global-key.test.ts b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts new file mode 100644 index 0000000000..7254b3b053 --- /dev/null +++ b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts @@ -0,0 +1,274 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * REST `/actions` — the object-less registration key, and failure status (#3913). + * + * Two independent defects that compounded into "global actions are + * unreachable, and when they fail you are told they succeeded": + * + * 1. **Registration key vs lookup key.** Both writers register an + * objectName-less action under the literal `'global'` — `AppPlugin` + * (`action.object || 'global'`) and `ObjectQLPlugin.actionObjectKey`. The + * REST fallback probed `'*'`, and `engine.executeAction` is an + * exact-string `Map` lookup with no wildcard semantics, so the probe could + * only ever miss: `Action 'log_call' on object '*' not found`. + * `POST /actions/global/log_call` worked by ACCIDENT (the path segment + * happened to spell the registration key) and `POST /actions//log_call` + * never worked at all. + * + * 2. **A miss was wrapped as transport success.** The not-found exit called + * `deps.success(...)`, which is always `{status: 200, body: {success: + * true, data}}` — so "this action does not exist" went out as HTTP 200 + * `{success: true, data: {success: false, error}}`, and every caller that + * did not hand-unwrap the inner envelope (the shipped console among them, + * which showed a green toast) swallowed it. + * + * Only the DISPATCH FAILURE moved to a status. A handler that RAN and + * rejected still reports `{success: false, error, code?, fields?}` at HTTP + * 200 — that is a business outcome, not a transport error, and #3937 pins + * it (`actions-validation-envelope.test.ts`). The line this file asserts is + * "did a handler run": below it the payload, above it the status, alongside + * the pre-dispatch answers the route already gives one (403 denied, 400 + * wrong type, 503 unavailable). + * + * The engine double here is deliberately faithful on the one point that + * matters: a real `Map` keyed `:` that throws + * `engine.ts`'s verbatim miss message. A double that resolves any key would + * pass while the bug was live. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HttpDispatcher } from './http-dispatcher.js'; + +/** + * A dispatcher over an engine whose action registry is a real exact-string + * Map — `registered` is keyed exactly as `engine.registerAction` keys it. + */ +function makeDispatcher(opts: { + registered?: Record any>; + objectDef?: any; +} = {}) { + const registered = opts.registered ?? {}; + const executeAction = vi.fn(async (object: string, action: string, ctx: any) => { + const handler = registered[`${object}:${action}`]; + if (!handler) throw new Error(`Action '${action}' on object '${object}' not found`); + return handler(ctx); + }); + const objectDef = opts.objectDef; + const ql: any = { + executeAction, + getSchema: (name: string) => (objectDef && name === objectDef.name ? objectDef : undefined), + registry: { + getObject: (name: string) => (objectDef && name === objectDef.name ? objectDef : undefined), + getItem: () => undefined, + }, + find: vi.fn(async () => []), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + }; + const metadata: any = { + load: vi.fn(async () => null), + listObjects: vi.fn(async () => (objectDef ? [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) as any, executeAction }; +} + +const ctx = (): any => ({ request: {}, environmentId: 'platform', executionContext: { userId: 'u1', systemPermissions: [] } }); + +describe('REST /actions — object-less ("global") action key (#3913)', () => { + it('reaches a handler registered under `global` from POST /actions/global/:action', async () => { + const { dispatcher } = makeDispatcher({ + registered: { 'global:log_call': () => ({ logged: true }) }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toEqual({ success: true, data: { logged: true } }); + }); + + it('reaches the same handler from POST /actions//:action — the empty-object shape', async () => { + // `filter(Boolean)` eats the empty segment, leaving a single part. That + // used to 400 as "Path must be /actions/:object/:action"; a lone + // segment is an action name with no object, so it routes at 'global'. + const { dispatcher, executeAction } = makeDispatcher({ + registered: { 'global:log_call': () => ({ logged: true }) }, + }); + + const res = await dispatcher.handleActions('//log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toEqual({ success: true, data: { logged: true } }); + expect(executeAction).toHaveBeenCalledWith('global', 'log_call', expect.anything()); + }); + + it('falls back from the routed object to `global` — the key the writers actually use', async () => { + // The pre-#3913 fallback rotated to '*', which nothing registers, so + // an object-scoped route could never reach a global handler. + const { dispatcher, executeAction } = makeDispatcher({ + registered: { 'global:log_call': () => ({ logged: true }) }, + objectDef: { name: 'crm_contact', actions: [] }, + }); + + const res = await dispatcher.handleActions('/crm_contact/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toEqual({ success: true, data: { logged: true } }); + expect(executeAction).toHaveBeenNthCalledWith(1, 'crm_contact', 'log_call', expect.anything()); + expect(executeAction).toHaveBeenNthCalledWith(2, 'global', 'log_call', expect.anything()); + }); + + it('still honours a handler registered directly under the legacy `*` key', async () => { + const { dispatcher } = makeDispatcher({ + registered: { '*:log_call': () => ({ logged: 'wildcard' }) }, + objectDef: { name: 'crm_contact', actions: [] }, + }); + + const res = await dispatcher.handleActions('/crm_contact/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toEqual({ success: true, data: { logged: 'wildcard' } }); + }); + + it('probes `global` exactly once when the route already names it', async () => { + const { dispatcher, executeAction } = makeDispatcher({ registered: {} }); + + await dispatcher.handleActions('/global/nope', 'POST', {}, ctx()); + + const probed = executeAction.mock.calls.map((c: any[]) => c[0]); + expect(probed).toEqual(['global', '*']); + }); + + it('prefers the object-specific handler over the global one', async () => { + const { dispatcher } = makeDispatcher({ + registered: { + 'crm_contact:log_call': () => ({ scope: 'object' }), + 'global:log_call': () => ({ scope: 'global' }), + }, + objectDef: { name: 'crm_contact', actions: [] }, + }); + + const res = await dispatcher.handleActions('/crm_contact/log_call', 'POST', {}, ctx()); + + expect(res.response.body.data).toEqual({ success: true, data: { scope: 'object' } }); + }); + + it('does not try to load a record for an object-less action', async () => { + const { dispatcher } = makeDispatcher({ + registered: { 'global:log_call': (c: any) => ({ record: c.record }) }, + }); + + const res = await dispatcher.handleActions('/global/log_call/rec_1', 'POST', {}, ctx()); + + // No `get` against an object named 'global' — only the id echo. + expect(res.response.body.data.data.record).toEqual({ id: 'rec_1' }); + }); + + it('404s an unregistered action, naming the ROUTED object rather than the last probe', async () => { + const { dispatcher } = makeDispatcher({ registered: {} }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(404); + expect(res.response.body.success).toBe(false); + // Not `on object '*'` — an object the caller never asked for, which is + // exactly the message #3913 was reported with. + expect(res.response.body.error.message).toBe("Action 'log_call' on object 'global' not found"); + }); + + it('routes the empty-object shape through the real dispatcher prefix too', async () => { + // `createActionsDomain` hands the domain `req.path.substring(8)`, so + // `/api/v1/actions//log_call` arrives here as `/log_call`. Pin the + // whole path, not just the domain-relative one — the reported failure + // was against the full URL. + const { dispatcher, executeAction } = makeDispatcher({ + registered: { 'global:log_call': () => ({ logged: true }) }, + }); + + const res = await dispatcher.dispatch('POST', '/actions//log_call', {}, {}, { + request: { headers: {} }, + }); + + expect(res.response.status).toBe(200); + expect(executeAction).toHaveBeenCalledWith('global', 'log_call', expect.anything()); + }); + + it('still rejects a path with no action segment at all', async () => { + const { dispatcher } = makeDispatcher({ registered: {} }); + + const res = await dispatcher.handleActions('/', 'POST', {}, ctx()); + + expect(res.response.status).toBe(400); + expect(res.response.body.error.message).toMatch(/Path must be/); + }); +}); + +describe('REST /actions — dispatch failure vs business failure (#3913)', () => { + it('a handler that RAN and rejected still reports in the payload at HTTP 200', async () => { + // The line #3913 draws is "did a handler run". This one did, so the + // failure is a business outcome and stays in the envelope — the + // contract #3937 pins. Asserted here too so the not-found 404 above + // can never be widened into "every action failure is a 4xx" by + // someone reading only this file. + const { dispatcher } = makeDispatcher({ + registered: { + 'global:log_call': () => { + const err: any = new Error("action 'log_call' threw: Contact has no phone number"); + err.innerMessage = 'Contact has no phone number'; + throw err; + }, + }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body.data.success).toBe(false); + // The debug wrapper stays in the server log, not on the wire. + expect(res.response.body.data.error).toBe('Contact has no phone number'); + }); + + it('carries a validation failure\'s code/fields in the payload, unchanged (#3937)', async () => { + const { dispatcher } = makeDispatcher({ + registered: { + 'global:log_call': () => { + const err: any = new Error('Validation failed'); + err.code = 'VALIDATION_FAILED'; + err.fields = [{ field: 'phone', message: 'required' }]; + throw err; + }, + }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body.data).toMatchObject({ + success: false, + code: 'VALIDATION_FAILED', + fields: [{ field: 'phone', message: 'required' }], + }); + }); + + it('leaves the success envelope untouched', async () => { + // Only the not-found exit moved; a successful action keeps the + // `{success: true, data: {success: true, data}}` shape the SDK reads. + const { dispatcher } = makeDispatcher({ + registered: { 'global:log_call': () => ({ id: 'call_1' }) }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(200); + expect(res.response.body).toMatchObject({ + success: true, + data: { success: true, data: { id: 'call_1' } }, + }); + }); +});