From e606637f21111ea21a0efeba55f0ad2ce4de8c46 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 21:27:44 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(runtime):=20callData=20=E7=9A=84=20dele?= =?UTF-8?q?te=20=E5=85=9C=E5=BA=95=E8=BF=94=E5=9B=9E=20spec=20=E5=A3=B0?= =?UTF-8?q?=E6=98=8E=E7=9A=84=20`{object,id,success}`=20(#5581)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `callData` 是 protocol 优先 + ObjectQL 兜底,两条路径对「删除成功」给的是两种 形状:protocol 回 `{object,id,success:true}`(合 `DeleteDataResponseSchema`), 兜底回 `{object,id,deleted:true}` —— `success` 缺失,`deleted` 从未被任何 schema 声明。按 spec 写的客户端在未注册 `protocol` 槽的精简装配上,会从一个 HTTP 200 里 读到 `success === undefined`,而调用方无从分辨自己走的是哪条路径。 规范只有一个,且 protocol 路径与公开 HTTP 文档都已站在 `success` 一侧,所以兜底 是唯一的偏离方。消费端兼容 `success ?? deleted` 两种拼写正是 contract-first 禁止 的形状,故修在生产方。 - `action-execution.ts` delete 兜底末行 `deleted: true` → `success: true` - `domains/data.ts` 那条把兜底形状写成规范的注释改正(同文件 get/update 两条 本来就是对的,只有 delete 这条对不上) - 测试钉住两条路径同形:schema safeParse + 跨路径逐字段相等 + 显式断言不带 未声明的 `deleted` 键(`z.object` 会剥掉未知键,单靠 parse 抓不到残留), 并覆盖 `/data` 与声明式端点执行器两个消费面 - `packages/mcp` 两处模拟 `callData('delete')` 的 test double 同步改口径 反向验证:把兜底改回 `deleted: true`,新增/更新的 6 条断言全红(fallback 侧), protocol 侧 parse 仍绿 —— 与动手前预判的方向一致。 Fixes #5581 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- .changeset/delete-fallback-success-shape.md | 39 ++++++ .../mcp/src/mcp-http-tools.scopes.test.ts | 4 +- .../mcp/src/mcp-server-runtime.http.test.ts | 7 +- ...ction-execution-calldata-not-found.test.ts | 116 +++++++++++++++++- packages/runtime/src/action-execution.ts | 20 ++- packages/runtime/src/domains/data.ts | 8 +- packages/spec/authorable-surface.base.json | 5 +- 7 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 .changeset/delete-fallback-success-shape.md diff --git a/.changeset/delete-fallback-success-shape.md b/.changeset/delete-fallback-success-shape.md new file mode 100644 index 0000000000..67c9fcf20c --- /dev/null +++ b/.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` +且不发出写」)完全未变,变的只有成功体拼写这一个键。 diff --git a/packages/mcp/src/mcp-http-tools.scopes.test.ts b/packages/mcp/src/mcp-http-tools.scopes.test.ts index 4354955ec1..9051adb34e 100644 --- a/packages/mcp/src/mcp-http-tools.scopes.test.ts +++ b/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 }; }, }; diff --git a/packages/mcp/src/mcp-server-runtime.http.test.ts b/packages/mcp/src/mcp-server-runtime.http.test.ts index 3a1a4ab2a6..32c159d853 100644 --- a/packages/mcp/src/mcp-server-runtime.http.test.ts +++ b/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 }; }, }; } diff --git a/packages/runtime/src/action-execution-calldata-not-found.test.ts b/packages/runtime/src/action-execution-calldata-not-found.test.ts index 376133a001..1981ee3fea 100644 --- a/packages/runtime/src/action-execution-calldata-not-found.test.ts +++ b/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); }); diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 081d1d0e6d..5726ffc5a1 100644 --- a/packages/runtime/src/action-execution.ts +++ b/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' }; } diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index 8d3c5d8483..e79ef3e48f 100644 --- a/packages/runtime/src/domains/data.ts +++ b/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) }; } diff --git a/packages/spec/authorable-surface.base.json b/packages/spec/authorable-surface.base.json index fcfe5cf368..a52aa648e6 100644 --- a/packages/spec/authorable-surface.base.json +++ b/packages/spec/authorable-surface.base.json @@ -1,6 +1,6 @@ { "description": "In-tree anchor for the authorable-surface deletion gate (#4650, #5235): a verbatim copy of the keys in authorable-surface.json as they stood at `baseRev`, a commit on origin/main. A build that CAN reach origin/main anchors on the merge base instead, and re-verifies this file against `baseRev` — so a PR that edits it to hide a deletion goes red wherever the network exists. A build that CANNOT reach GitHub (image-build stages, air-gapped, fork, historical-tag reproduction) anchors here instead of failing. Written only by `gen:schema`, only from a git-resolved baseline — never from the build that is being checked. See #5235.", - "baseRev": "168f60f1adf5e4f44ed818eccbba442052722328", + "baseRev": "2614aefb305079e2a24722ee5db54e405c60655e", "keys": [ "ai/AIModelConfig:maxTokens", "ai/AIModelConfig:model", @@ -5824,10 +5824,13 @@ "system/EmailAndPasswordConfig:resetPasswordTokenExpiresIn", "system/EmailAndPasswordConfig:revokeSessionsOnPasswordReset", "system/EmailServiceConfig:apiKey", + "system/EmailServiceConfig:appName", "system/EmailServiceConfig:defaultFrom", + "system/EmailServiceConfig:defaultTemplateContext", "system/EmailServiceConfig:options", "system/EmailServiceConfig:persist", "system/EmailServiceConfig:provider", + "system/EmailServiceConfig:queueDelivery", "system/EmailServiceConfig:retries", "system/EmailTemplateDefinition:_lock", "system/EmailTemplateDefinition:_lockDocsUrl", From 53969effc875f1a91f4dd064407e05d84f507427 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 21:29:38 +0000 Subject: [PATCH 2/2] =?UTF-8?q?chore:=20=E9=80=80=E5=9B=9E=E6=9C=AC?= =?UTF-8?q?=E5=9C=B0=20`gen:schema`=20=E9=A1=BA=E5=B8=A6=E9=87=8D=E7=94=9F?= =?UTF-8?q?=E6=88=90=E7=9A=84=20authorable-surface=20=E9=94=9A=E7=82=B9=20?= =?UTF-8?q?(#5581)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 上一提交的 `git add -A` 扫进了 `packages/spec/authorable-surface.base.json` —— 那是本地跑 `pnpm --filter ... build` 时 `gen:schema` 顺带重生成的产物 (把 `baseRev` 重锚到本工作树的基点,并带进 main 上此后新增的 `EmailServiceConfig` 三个键),与本单毫无关系。 该文件是 #4650/#5235 删除门禁的锚点,按其自述「只应由 gen:schema 从 git 解析 出的基线写入,绝不由被检查的这次构建写入」,PR 里不该出现它的改动。退回基点 版本。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- packages/spec/authorable-surface.base.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/spec/authorable-surface.base.json b/packages/spec/authorable-surface.base.json index a52aa648e6..fcfe5cf368 100644 --- a/packages/spec/authorable-surface.base.json +++ b/packages/spec/authorable-surface.base.json @@ -1,6 +1,6 @@ { "description": "In-tree anchor for the authorable-surface deletion gate (#4650, #5235): a verbatim copy of the keys in authorable-surface.json as they stood at `baseRev`, a commit on origin/main. A build that CAN reach origin/main anchors on the merge base instead, and re-verifies this file against `baseRev` — so a PR that edits it to hide a deletion goes red wherever the network exists. A build that CANNOT reach GitHub (image-build stages, air-gapped, fork, historical-tag reproduction) anchors here instead of failing. Written only by `gen:schema`, only from a git-resolved baseline — never from the build that is being checked. See #5235.", - "baseRev": "2614aefb305079e2a24722ee5db54e405c60655e", + "baseRev": "168f60f1adf5e4f44ed818eccbba442052722328", "keys": [ "ai/AIModelConfig:maxTokens", "ai/AIModelConfig:model", @@ -5824,13 +5824,10 @@ "system/EmailAndPasswordConfig:resetPasswordTokenExpiresIn", "system/EmailAndPasswordConfig:revokeSessionsOnPasswordReset", "system/EmailServiceConfig:apiKey", - "system/EmailServiceConfig:appName", "system/EmailServiceConfig:defaultFrom", - "system/EmailServiceConfig:defaultTemplateContext", "system/EmailServiceConfig:options", "system/EmailServiceConfig:persist", "system/EmailServiceConfig:provider", - "system/EmailServiceConfig:queueDelivery", "system/EmailServiceConfig:retries", "system/EmailTemplateDefinition:_lock", "system/EmailTemplateDefinition:_lockDocsUrl",