fix(actions): reach global actions at their real registration key, and 404 an action that never dispatched (#3913)

.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
+`[<routed object>, '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`.

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.

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:

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');
+    });
 });

packages/client/src/index.ts

@@ -312,6 +312,38 @@ async function* parseEventStream(res: Response): AsyncIterable<AiStreamChunk> {
   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<T>(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,37 +2911,58 @@ 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 <T = any>(
           objectName: string,
           actionName: string,
           opts?: { recordId?: string; params?: Record<string, unknown> },
       ): 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<T>(await this.unwrapResponse<any>(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 <T = any>(
           actionName: string,

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;