fix(runtime): callData 的 delete 兜底返回 spec 声明的 {object, id, success},与 protocol 路径同形 (#5581)

.changeset/delete-fallback-success-shape.md

@@ -0,0 +1,39 @@
+---
+'@objectstack/runtime': patch
+---
+
+fix(runtime): `callData('delete', …)` 的 ObjectQL 兜底返回 spec 声明的 `{ object, id, success }`,与 protocol 路径同形 (#5581)
+
+`callData` 是 protocol 优先 + ObjectQL 兜底,两条路径此前对「删除成功」给的是两种形状:
+
+| 路径 | 此前 | 现在 |
+|---|---|---|
+| protocol(`deleteData`) | `{ object, id, success: true }` | 不变 |
+| ObjectQL 兜底 | `{ object, id, deleted: true }` | `{ object, id, success: true }` |
+
+规范只有一个:`DeleteDataResponseSchema`(`packages/spec/src/api/protocol.zod.ts`)声明的是
+`{ object, id, success }`,`deleted` 从未被任何 schema 声明;公开的 HTTP 文档
+(`content/docs/protocol/kernel/http-protocol.mdx`)也一直写的是 `success`。所以兜底是唯一
+的偏离方,protocol 路径与 spec、与文档都无需改动。
+
+这是 #5138 同一族缺陷的成功侧:#5138 收敛的是「记录不存在」的答案,本次收敛的是「删除成功」
+的答案 —— 后者是每一次正常请求都会走到的面,而非只在 id 写错时才碰到。此前按
+`DeleteDataResponseSchema` 写的客户端,在**未注册 `protocol` 槽**的精简装配上会从一个 HTTP 200
+里读到 `success === undefined`,即「删除到底成没成功」读不出来,而调用方无从分辨自己走的是哪条
+路径。消费端各自兼容 `success ?? deleted` 两种拼写正是 contract-first 禁止的形状,所以修在
+生产方,不在消费方。
+
+## ⚠️ 升级须知(行为变化)
+
+**仅影响没有安装 `MetadataPlugin`(`@objectstack/metadata-protocol`,即注册 `protocol` 槽)的
+精简装配。** 装了该插件的部署走 protocol 优先路径,本来就返回 `success`,不受影响。
+
+在这类精简装配上,以下三个面的 `DELETE` 成功体键名由 `deleted` 改为 `success`:
+
+- `DELETE /api/v1/data/:object/:id`
+- MCP 的 `delete_record` 工具(`domains/mcp.ts` 的 `remove` 桥)
+- 声明式端点(`objectParams.operation: 'delete'`,#5092)
+
+若你的代码读的是 `response.data.deleted`,请改读 `response.data.success` —— 这也是 spec 与
+公开文档自始至终声明的键。删除行为本身(含 #5138 落的「记录不存在则 404 `RECORD_NOT_FOUND`
+且不发出写」)完全未变,变的只有成功体拼写这一个键。

packages/mcp/src/mcp-http-tools.scopes.test.ts

@@ -37,7 +37,9 @@ function makeBridge(): McpDataBridge & McpActionBridge & { calls: any[] } {
     async aggregate(object: string, opts: any) { calls.push(['aggregate', object, opts]); return []; },
     async create(object: string, data: any) { calls.push(['create', object, data]); return { object, id: 'n1' }; },
     async update(object: string, id: string, data: any) { calls.push(['update', object, id, data]); return { object, id }; },
-    async remove(object: string, id: string) { calls.push(['remove', object, id]); return { object, id, deleted: true }; },
+    // [#5581] `success`, not `deleted` — mirrors what `callData('delete', …)`
+    // now returns on BOTH of its paths (the spec's `DeleteDataResponse`).
+    async remove(object: string, id: string) { calls.push(['remove', object, id]); return { object, id, success: true }; },
     async listActions() { calls.push(['listActions']); return [{ name: 'complete_task', objectName: 'task' }]; },
     async runAction(name: string, input: any) { calls.push(['runAction', name, input]); return { ok: true }; },
   };

packages/mcp/src/mcp-server-runtime.http.test.ts

@@ -40,7 +40,12 @@ function makeBridge(): McpDataBridge & { calls: any[] } {
     },
     async remove(object: string, id: string) {
       calls.push(['remove', object, id]);
-      return { object, id, deleted: true };
+      // [#5581] `success`, not `deleted` — this double stands in for
+      // `callData('delete', …)`, whose two paths now both answer the spec's
+      // `DeleteDataResponse` shape. No assertion here reads the key; it is
+      // kept honest so the next reader does not copy a shape the producer
+      // stopped returning.
+      return { object, id, success: true };
     },
   };
 }

packages/runtime/src/action-execution-calldata-not-found.test.ts

@@ -33,6 +33,16 @@
  * two consuming faces the issue names: `/data` through the REAL `HttpDispatcher`,
  * and the declarative endpoint executor (#5092 / PR #5136) driven with the REAL
  * `callData` bound, which is where the status a client receives is decided.
+ *
+ * ---
+ *
+ * #5581 extended the suite to the SUCCESS side of the same family. `delete`'s
+ * fallback answered `{ object, id, deleted: true }` where the protocol path
+ * answered the spec's `{ object, id, success: true }`, so the same two paths
+ * disagreed once more — this time on every successful request rather than only
+ * on a mistaken id. Same fix shape: the fallback moves to what the spec
+ * declares, and the identity is pinned across the two paths so it cannot drift
+ * apart again. See the `#5581` describe block below.
  */
 
 import { describe, it, expect } from 'vitest';
@@ -44,7 +54,7 @@ import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protoco
 // already a `dependencies` entry of `@objectstack/runtime`, so no manifest
 // change is needed to reach it.
 import { assertEngineDeleteDispatch } from '@objectstack/objectql';
-import { ApiEndpointSchema } from '@objectstack/spec/api';
+import { ApiEndpointSchema, DeleteDataResponseSchema } from '@objectstack/spec/api';
 import type { ApiEndpoint } from '@objectstack/spec/api';
 
 import { callData, type ActionExecutionDeps } from './action-execution.js';
@@ -221,10 +231,14 @@ describe('the ObjectQL fallback answers a missing id with RECORD_NOT_FOUND (#513
         await expect(run(h.deps, v, 'r1')).resolves.toBeTruthy();
     }, 60_000);
 
-    it('a real delete still deletes, and still answers `deleted: true`', async () => {
+    it('a real delete still deletes, and answers the SPEC’s `success: true`', async () => {
+        // [#5581] Was `{ object, id, deleted: true }` here — `success` missing,
+        // `deleted` declared nowhere in `DeleteDataResponseSchema`. The delete
+        // itself is unchanged; only the key the answer spells it with.
         const h = fallbackHarness();
         const out: any = await run(h.deps, 'delete', 'r1');
-        expect(out).toEqual({ object: 'task', id: 'r1', deleted: true });
+        expect(out).toEqual({ object: 'task', id: 'r1', success: true });
+        expect(out.deleted).toBeUndefined();
         expect(h.deleted).toEqual(['r1']);
         expect(h.store.has('r1')).toBe(false);
     }, 60_000);
@@ -274,6 +288,63 @@ describe('one `callData`, one answer — the two paths are field-for-field ident
     }, 60_000);
 });
 
+// ---------------------------------------------------------------------------
+// [#5581] The same claim on the SUCCESS side
+// ---------------------------------------------------------------------------
+
+/**
+ * #5138 unified what the two paths say when the id names no row. This unifies
+ * what they say when the delete SUCCEEDS — the other half of the same "one
+ * `callData`, two answers" family, and the half a caller reaches on every
+ * normal request rather than only on a mistake.
+ *
+ * Measured on `main` @ `2614aefb3`, same two harnesses, same single row:
+ *
+ * ```
+ * PROTOCOL del (hit): {"object":"task","id":"r1","success":true}
+ * FALLBACK del (hit): {"object":"task","id":"r1","deleted":true}
+ * ```
+ *
+ * `DeleteDataResponseSchema` is the authority and declares `success`; the
+ * protocol path has produced it since #4435. So the fallback was the only
+ * thing to move, exactly as in #5138.
+ *
+ * The schema is asserted AND the two bodies are compared field-for-field,
+ * because neither alone is enough: `z.object` strips unknown keys, so a body
+ * carrying BOTH `success` and a leftover `deleted` parses green — only the
+ * cross-path equality catches that. Conversely the equality alone would stay
+ * green if both paths drifted together, which the schema parse catches.
+ */
+describe('one `callData`, one success body — delete answers the spec shape on both paths (#5581)', () => {
+    it('the fallback’s success body parses as DeleteDataResponse', async () => {
+        const out = await run(fallbackHarness().deps, 'delete', 'r1');
+        const parsed = DeleteDataResponseSchema.safeParse(out);
+        expect(parsed.success, JSON.stringify((parsed as any).error?.issues ?? [])).toBe(true);
+    }, 60_000);
+
+    it('the protocol’s success body parses as DeleteDataResponse', async () => {
+        const out = await run(protocolHarness().deps, 'delete', 'r1');
+        const parsed = DeleteDataResponseSchema.safeParse(out);
+        expect(parsed.success, JSON.stringify((parsed as any).error?.issues ?? [])).toBe(true);
+    }, 60_000);
+
+    it('fallback success body === protocol success body, field for field', async () => {
+        const withoutProtocol = await run(fallbackHarness().deps, 'delete', 'r1');
+        const withProtocol = await run(protocolHarness().deps, 'delete', 'r1');
+        expect(withoutProtocol).toEqual(withProtocol);
+        expect(withoutProtocol).toEqual({ object: 'task', id: 'r1', success: true });
+    }, 60_000);
+
+    it('neither path carries the undeclared `deleted` key', async () => {
+        // The was-red assertion in its narrowest form. `z.object` would strip
+        // `deleted` and still report a valid parse, so the key is named here.
+        for (const deps of [fallbackHarness().deps, protocolHarness().deps]) {
+            const out: any = await run(deps, 'delete', 'r1');
+            expect(Object.keys(out).sort()).toEqual(['id', 'object', 'success']);
+        }
+    }, 60_000);
+});
+
 // ---------------------------------------------------------------------------
 // Consuming face 1 — `/data` through the REAL dispatcher
 // ---------------------------------------------------------------------------
@@ -328,6 +399,20 @@ describe('/data through the real HttpDispatcher inherits the one answer (#5138)'
         expect(res.handled).toBe(true);
         expect(res.response.status).toBe(200);
     }, 60_000);
+
+    it('[#5581] DELETE on a row that exists answers 200 with the spec’s `success` body', async () => {
+        // The face the issue is actually about: a client reading the DELETE
+        // 200 off a deployment WITHOUT the protocol slot. It used to find
+        // `success === undefined` and an undeclared `deleted` in its place, so
+        // "did the delete succeed" was unreadable from a 200 response.
+        const { dispatcher, deleted } = dispatcherOverFallback();
+        const res: any = await dispatcher.dispatch('DELETE', '/data/task/r1', undefined, {}, { request: {} } as HttpProtocolContext);
+        expect(res.handled).toBe(true);
+        expect(res.response.status).toBe(200);
+        expect(res.response.body.data).toEqual({ object: 'task', id: 'r1', success: true });
+        expect(DeleteDataResponseSchema.safeParse(res.response.body.data).success).toBe(true);
+        expect(deleted).toEqual(['r1']);
+    }, 60_000);
 });
 
 // ---------------------------------------------------------------------------
@@ -379,4 +464,29 @@ describe('a declared endpoint answers 404 RECORD_NOT_FOUND on the wire (#5092/#5
         expect(error.message).toBe(`Record ${GHOST} not found in task`);
         expect(h.deleted).toEqual([]);
     }, 60_000);
+
+    it('[#5581] a declared DELETE endpoint carries the spec’s `success` body too', async () => {
+        // The executor reuses `/data`'s delegation byte for byte, so it
+        // inherited the fork the same way. Note the two `success` flags are
+        // DIFFERENT facts and both are pinned here: `body.success` is the HTTP
+        // envelope's ok-flag (`successAnswer`), `body.data.success` is
+        // `DeleteDataResponse`'s "the deletion happened". Reading one for the
+        // other is the mistake this shape invites, so neither is left implied.
+        const h = fallbackHarness();
+        const ctx = buildEndpointExecutionContext({
+            request: { method: 'GET', path: '/api/v1/apps/showcase/task', query: { id: 'r1' }, headers: {} },
+            match: { endpoint: declaredEndpoint('delete'), params: {} },
+            executionContext: EC,
+        });
+        const answer = await executeEndpointTarget(ctx, {
+            callData: (action, params, driver, scope, ec) => callData(h.deps, REQ, action, params, driver, scope, ec),
+        });
+
+        expect(answer.status).toBe(200);
+        const body = answer.body as any;
+        expect(body.success).toBe(true);
+        expect(body.data).toEqual({ object: 'task', id: 'r1', success: true });
+        expect(DeleteDataResponseSchema.safeParse(body.data).success).toBe(true);
+        expect(h.deleted).toEqual(['r1']);
+    }, 60_000);
 });

packages/runtime/src/action-execution.ts

@@ -239,7 +239,25 @@ export async function callData(deps: ActionExecutionDeps,
             const existing = (all as any[]).find((i: any) => i.id === params.id);
             if (!existing) throw recordNotFoundError(params.object, params.id);
             await ql.delete(params.object, findOpts({ where: { id: params.id } }));
-            return { object: params.object, id: params.id, deleted: true };
+            // [#5581] `success`, not `deleted`. The success body was the other
+            // half of the same "one `callData`, two answers" defect #5138 fixed
+            // on the not-found side: the protocol path returns the SPEC's shape
+            // (`DeleteDataResponseSchema` — `{ object, id, success }`,
+            // `packages/spec/src/api/protocol.zod.ts:472`) and this fallback
+            // returned `{ object, id, deleted: true }` — `success` missing, and
+            // `deleted` declared nowhere in the spec. A client written against
+            // the declared shape read `success === undefined` off an HTTP 200
+            // on any deployment that did not register the `protocol` slot, and
+            // had no way to tell which path had served it.
+            //
+            // The spec is the authority, not this literal: `success` is what
+            // `DeleteDataResponseSchema` declares, what the protocol path has
+            // returned since #4435, and what the public HTTP docs already
+            // document (`content/docs/protocol/kernel/http-protocol.mdx`).
+            // Teaching consumers to read `success ?? deleted` would have been
+            // the contract-first-forbidden shape — two spellings of one fact,
+            // kept alive by every reader.
+            return { object: params.object, id: params.id, success: true };
         }
         throw { statusCode: 503, message: 'Data service not available' };
     }

packages/runtime/src/domains/data.ts

@@ -99,7 +99,13 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m
         // DELETE /data/:object/:id
         if (parts.length === 2 && m === 'DELETE') {
             const id = parts[1];
-            // Spec: returns DeleteDataResponse = { object, id, deleted }
+            // Spec: returns DeleteDataResponse = { object, id, success }
+            // [#5581] Said `deleted` until this fix — the one comment in this
+            // trio that did NOT match its schema (`DeleteDataResponseSchema`
+            // declares `success`; the 87/94 get/update comments above were
+            // already right). It described the ObjectQL fallback's off-spec
+            // body as if it were the spec, so the next reader of this line
+            // would have written a consumer against `deleted`.
             const result = await actionExec.callData(deps, _context, 'delete', { object: objectName, id }, _context.dataDriver, _context.environmentId, _context.executionContext);
             return { handled: true, response: deps.success(result) };
         }