From b934f968716676c07d09d205f134881cabb004b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 10:04:02 +0000 Subject: [PATCH] fix(actions): reach global actions at their real key, serve handler failures with a real status (#3913) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent defects that compounded into "global actions are unreachable, and when one fails you are told it succeeded". 1 — registration key vs lookup key. Both writers register an objectName-less action under the literal 'global' (AppPlugin's `action.object || 'global'`, ObjectQLPlugin.actionObjectKey), but the REST fallback probed '*' — and `engine.executeAction` is an exact-string Map lookup with no wildcard semantics, so that probe could only miss. `/actions/global/:action` worked by accident (the path segment happened to spell the registration key); `/actions//:action` never worked, and neither did falling back from an object-scoped route to a global handler. 'global' is now canonical (GLOBAL_ACTION_OBJECT_KEY). One probe order — [routed object, 'global', '*'] via actionHandlerObjectKeys — serves both the REST route and the MCP run_action bridge, with the legacy '*' kept last so a handler registered directly against it still resolves. A single-segment path routes at 'global' instead of 400-ing, and the miss now reports the ROUTED object rather than whichever probe ran last. 2 — every handler failure was wrapped as transport success. Both exits called deps.success(), which always emits {status: 200, body: {success: true, data}}, so a denial or a thrown handler went out as HTTP 200 {success:true, data: {success:false, error}} and any caller that skipped the inner envelope swallowed it. Failures now exit through the dispatcher's error path: 404 unregistered, 400 for a deliberate action-body throw (the mapping @objectstack/rest's mapDataError already uses for the same SandboxError) and for a rejected flow (code FLOW_FAILED), the error's own .status when it carries one, 400 + fields[] for a ValidationError, 500 otherwise. The success envelope is unchanged. client.actions.invoke/invokeGlobal still do not throw — they fold the new non-2xx shape into the same {success, data?, error?} result and keep honouring the pre-#3913 200-with-inner-failure shape, so a current SDK still talks to an older server. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F --- .../actions-global-key-and-failure-status.md | 64 ++++ content/docs/api/client-sdk.mdx | 11 +- content/docs/ui/actions.mdx | 12 +- packages/client/src/actions-surface.test.ts | 37 ++- packages/client/src/index.ts | 81 ++++- packages/objectql/src/plugin.ts | 9 +- packages/runtime/src/action-execution.ts | 85 ++++- packages/runtime/src/app-plugin.ts | 7 +- packages/runtime/src/domains/actions.ts | 94 ++++-- ...http-dispatcher.actions-global-key.test.ts | 296 ++++++++++++++++++ ...p-dispatcher.actions-type-dispatch.test.ts | 12 +- 11 files changed, 635 insertions(+), 73 deletions(-) create mode 100644 .changeset/actions-global-key-and-failure-status.md create mode 100644 packages/runtime/src/http-dispatcher.actions-global-key.test.ts 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..721994d7e0 --- /dev/null +++ b/.changeset/actions-global-key-and-failure-status.md @@ -0,0 +1,64 @@ +--- +"@objectstack/runtime": minor +"@objectstack/client": minor +--- + +fix(actions): reach global actions at their real registration key, and stop serving handler failures as HTTP 200 (#3913) + +Two independent defects that compounded into "global actions are unreachable, +and when one fails you are told it succeeded". + +**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 — every handler failure was wrapped as transport success.** Both the +success and the failure exit called `deps.success(...)`, which always emits +`{status: 200, body: {success: true, data}}`. So any failure — a denial, a +missing action, a deliberate `throw` in an action body — went out 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. Failures now exit +through the dispatcher's error path with a real status: + +| Failure | Status | +|:---|:---| +| No handler registered under any key | **404**, naming the routed object rather than whichever probe ran last | +| Deliberate `throw` from an action body (`SandboxError`) | **400** with the business message — the mapping `@objectstack/rest`'s `mapDataError` has always used for the identical error | +| A `flow` action whose flow ran and rejected | **400**, `code: FLOW_FAILED` | +| Error carrying its own `.status` (e.g. a `FORBIDDEN`) | that status | +| Record `ValidationError` | **400** with `fields[]` (#3918 parity) | +| Anything else | **500** | + +The **success** envelope is unchanged (`{success: true, data: {success: true, +data}}`), and `client.actions.invoke` / `invokeGlobal` still do **not** throw — +they fold the new non-2xx shape back into the same `{ success, data?, error? }` +result, with `error` as a plain string, and keep honouring the pre-#3913 +`200 + inner success:false` shape so a current SDK can still talk to an older +server. + +**Migration:** anything reading `response.data.success` off a raw HTTP call +should read the HTTP status (or the top-level `success`) instead — a failed +action is no longer a 200. Callers going through `@objectstack/client` need no +change. diff --git a/content/docs/api/client-sdk.mdx b/content/docs/api/client-sdk.mdx index 57e0e5aecc..b0bcef5700 100644 --- a/content/docs/api/client-sdk.mdx +++ b/content/docs/api/client-sdk.mdx @@ -391,16 +391,17 @@ 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: the server answers a failed action with a real +// status (403 / 404 / 400 / 500) and the SDK folds it into the envelope. 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 4296669246..3f7cd4edf7 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -227,11 +227,17 @@ 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). +A failure carries a **real HTTP status** — 403 denied, 404 no such action, 400 +for a business rejection or a param-contract violation, 500 for an unexpected +fault — with `{ "success": false, "error": { "message", "code" } }`. Read the +status; a 200 is the only answer that means the handler ran. + +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..974ecda7c6 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( @@ -71,10 +71,10 @@ describe('client.actions', () => { expect(JSON.parse(fetchMock.mock.calls[0][1].body)).toEqual({ params: { dryRun: true } }); }); - 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). + it('surfaces the handler business failure without throwing (pre-#3913 server: 200 + inner success:false)', async () => { + // Servers older than #3913 report a handler throw as HTTP 200 with an + // inner `{ success:false, error }`. A current SDK still has to talk to + // them, so the inner envelope stays authoritative on a 2xx. const { client } = createMockClient({ success: true, data: { success: false, error: 'Lead is already converted' }, @@ -83,4 +83,31 @@ describe('client.actions', () => { expect(result.success).toBe(false); expect(result.error).toBe('Lead is already converted'); }); + + it('surfaces a real 4xx as the same non-throwing envelope (#3913)', async () => { + // Post-#3913 the server answers a handler failure with a real status + // and `{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. + const { client } = createMockClient( + { success: false, error: { message: 'Lead is already converted', code: 'FLOW_FAILED' } }, + 400, + ); + const result = await client.actions.invoke('crm_lead', 'convert', { recordId: 'lead_1' }); + 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('Lead is already converted'); + }); + + it('reports a 404 for an unregistered action instead of throwing', async () => { + 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); + expect(result.error).toMatch(/not found/); + }); }); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 4aec1e47a1..88bf38f5cc 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 only 2xx a post-#3913 server sends. + * - `{ success: false, error: '…' }` — a server older than #3913 answering a + * handler failure with HTTP 200. A current SDK still has to talk to those, + * so the inner envelope stays authoritative when it reports a failure. + * + * The non-2xx case never reaches here: `fetch` throws on it, and `invoke` + * catches (this surface does not throw — a failed business action is a toast, + * not a crash, which is the property #3913 kept while fixing the status code). + */ +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,23 @@ 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 still does + * NOT throw — callers surface a failure as a toast rather than a crash. + * What changed in #3913 is the WIRE: a handler failure used to come back as + * HTTP 200 `{success: true, data: {success: false, error}}`, so any caller + * that forgot to unwrap the inner envelope read the outer `success: true` + * and reported a success it never had. The server now answers a failure + * with a real status (404 unregistered, 403 denied, 400 business/validation, + * 500 otherwise) and `{success: false, error: {message, code, details}}`; + * {@link normalizeActionResult} folds both shapes — old server, new server — + * into the same `{ success, data?, error? }` with `error` as a plain string. */ 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 +2935,30 @@ 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 handler failure + // IS a non-2xx now — but this surface's contract is to report, + // not throw. `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 from the pre-#3913 wire. + 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 b686cae484..aa447800c8 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -353,7 +353,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 } : {}), @@ -363,7 +363,15 @@ export async function dispatchFlowAction(deps: ActionExecutionDeps, params: { ...record, ...params }, }); if (result && typeof result === 'object' && 'success' in result && result.success === false) { - throw new Error(`Flow '${action.target}' failed: ${result.error ?? 'unknown error'}`); + // The flow RAN and rejected — a business outcome, not a server fault. + // Tagged 400 so the REST route's error exit serves it as one (#3913): + // `errorFromThrown` reads `.status`, and staying under 500 also keeps + // the message out of the internal-error-leak sanitiser, which a flow's + // own wording has no reason to trip. + const err: any = new Error(`Flow '${action.target}' failed: ${result.error ?? 'unknown error'}`); + err.status = 400; + err.code = 'FLOW_FAILED'; + throw err; } return result ?? null; } @@ -621,7 +629,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; @@ -670,20 +678,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 } } } @@ -780,7 +787,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 === '*'; } /** @@ -802,9 +861,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 }, @@ -826,7 +885,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 773c33c466..d350e33596 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'; /** @@ -637,10 +638,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 9087e6c09b..b4b469f8aa 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,16 @@ * 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 the last 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; + * - every handler failure came back as HTTP 200 + * `{success: true, data: {success: false, error}}`, so a caller that did + * not hand-inspect the INNER envelope silently swallowed it. Failures now + * carry a real status (404 unregistered, 400 validation, 500 otherwise). */ import * as actionExec from '../action-execution.js'; @@ -43,7 +54,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: @@ -65,12 +77,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 @@ -146,13 +164,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 : {}; @@ -167,7 +178,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; @@ -253,28 +264,59 @@ 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) { + // [#3913] A handler failure is a REAL failure. It used to come back as + // HTTP 200 `{success: true, data: {success: false, error}}` — transport + // success wrapping a business error — so every caller that did not + // hand-check the INNER envelope (including the shipped console, which + // showed a green success toast) swallowed it silently. Failures now + // exit through the dispatcher's error path with a real status. const full = err?.message ?? String(err); // The sandbox wraps a user throw as ` '' threw: ` for // server logs; surface only the business `` (SandboxError.innerMessage) // to the client so an action's error toast reads as plain text instead of // leaking the debug prefix. Keep the full wrapper in the log for debugging. const inner: unknown = err?.innerMessage; - const clientMsg = (typeof inner === 'string' && inner) ? inner : full; - if (clientMsg !== full) console.error(`[action ${objectName}/${actionName}] ${full}`); - return { handled: true, response: deps.success({ success: false, error: clientMsg }) }; + if (typeof inner === 'string' && inner) { + console.error(`[action ${objectName}/${actionName}] ${full}`); + // A deliberate `throw` from inside an action BODY is a business + // rule, not a server fault — 400, exactly as `@objectstack/rest`'s + // `mapDataError` has always served the identical SandboxError from + // the data routes. 400 also keeps the message intact: the 5xx leak + // sanitiser would rewrite a business message that merely opens with + // "Update …" as a generic internal error. + return { handled: true, response: deps.error(inner, 400) }; + } + // Anything else is unexpected: an error carrying its own `.status` wins, + // a record `ValidationError` maps to 400 with `fields[]` (#3918), and + // the rest are 500 — sanitised by the dispatcher's leak guard. + return { handled: true, response: deps.errorFromThrown(err, 500) }; } } 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..0479d1ecd0 --- /dev/null +++ b/packages/runtime/src/http-dispatcher.actions-global-key.test.ts @@ -0,0 +1,296 @@ +// 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. **Every handler failure was wrapped as transport success.** Both exits + * called `deps.success(...)`, which is always `{status: 200, body: + * {success: true, data}}` — so a failure 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. + * + * 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 — handler failures carry a real status (#3913)', () => { + it('serves a deliberate throw from an action body as 400 with the business message', async () => { + // The sandbox wraps a user throw as ` '' threw: ` and + // preserves the business message on `.innerMessage` — the same shape + // `@objectstack/rest`'s `mapDataError` has always served as a 400. + 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(400); + expect(res.response.body.success).toBe(false); + // The debug wrapper stays in the server log, not on the wire. + expect(res.response.body.error.message).toBe('Contact has no phone number'); + }); + + it('never reports a failure as HTTP 200 {success:true, data:{success:false}}', async () => { + const { dispatcher } = makeDispatcher({ + registered: { 'global:log_call': () => { throw new Error('handler exploded'); } }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(500); + expect(res.response.body.success).toBe(false); + // The regression in one assertion: the outer envelope must not claim + // success while the inner one reports failure. + expect(res.response.body.data).toBeUndefined(); + }); + + it('honours an error that carries its own status — a FORBIDDEN stays a 403', async () => { + const { dispatcher } = makeDispatcher({ + registered: { + 'global:log_call': () => { + const err: any = new Error('Not allowed to write crm_call'); + err.status = 403; + err.code = 'FORBIDDEN'; + throw err; + }, + }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(403); + expect(res.response.body.error.message).toBe('Not allowed to write crm_call'); + expect(res.response.body.error.details?.code).toBe('FORBIDDEN'); + }); + + it('maps a record ValidationError to 400 with fields[] (#3918 parity)', async () => { + const { dispatcher } = makeDispatcher({ + registered: { + 'global:log_call': () => { + const err: any = new Error('Validation failed'); + err.code = 'VALIDATION_FAILED'; + err.fields = [{ name: 'phone', message: 'required' }]; + throw err; + }, + }, + }); + + const res = await dispatcher.handleActions('/global/log_call', 'POST', {}, ctx()); + + expect(res.response.status).toBe(400); + expect(res.response.body.error.details).toMatchObject({ + code: 'VALIDATION_FAILED', + fields: [{ name: 'phone', message: 'required' }], + }); + }); + + it('leaves the success envelope untouched', async () => { + // Only the FAILURE wire changed; 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' } }, + }); + }); +}); 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 821f45290c..0c06a94cb4 100644 --- a/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts +++ b/packages/runtime/src/http-dispatcher.actions-type-dispatch.test.ts @@ -149,7 +149,7 @@ describe('REST /actions — flow dispatch (#3915)', () => { expect(passed).not.toHaveProperty('object'); }); - it('surfaces a flow failure through the action error envelope', async () => { + it('surfaces a flow failure as a 400, not as a 200 wrapping success:false (#3913)', async () => { const execute = vi.fn(async () => ({ success: false, error: 'lead already converted' })); const { dispatcher } = makeDispatcher({ objectDef: { name: 'crm_lead', actions: [flowAction] }, @@ -158,8 +158,14 @@ describe('REST /actions — flow dispatch (#3915)', () => { const res = await dispatcher.handleActions('/crm_lead/convert_lead', 'POST', {}, ctxFor()); - expect(res.response.body.data.success).toBe(false); - expect(res.response.body.data.error).toMatch(/crm_convert_lead_wizard.*lead already converted/i); + // The flow ran and rejected — a business outcome, so a 400 carrying the + // flow's own wording. It used to be HTTP 200 `{success: true, data: + // {success: false, error}}`, which any caller that skipped the inner + // envelope read as a success. + expect(res.response.status).toBe(400); + expect(res.response.body.success).toBe(false); + expect(res.response.body.error.message).toMatch(/crm_convert_lead_wizard.*lead already converted/i); + expect(res.response.body.error.details?.code).toBe('FLOW_FAILED'); }); it('reports a missing automation service as 503, not as a business failure', async () => {