fix(client)!: DeleteDataResult 声明它自称的 schema —— success,不是 deleted (#5638)

.changeset/client-delete-result-success.md

@@ -0,0 +1,98 @@
+---
+"@objectstack/client": major
+"@objectstack/cli": patch
+---
+
+fix(client)!: `DeleteDataResult` declares the schema it names — `success`, not `deleted` (#5638)
+
+`DeleteDataResult` — the return type of `client.data.delete()` and of the
+project-scoped `client.project(id).data.delete()` — carried the comment
+`Spec: DeleteDataResponseSchema` above a declaration that contradicted it:
+
+```ts
+/** Spec: DeleteDataResponseSchema */
+export interface DeleteDataResult {
+  object: string;
+  id: string;
+  deleted: boolean;   // ← the schema declares `success`
+}
+```
+
+`DeleteDataResponseSchema` (`packages/spec/src/api/protocol.zod.ts`) declares
+`{ object, id, success }`. `deleted` has never been declared by any schema, and
+no server path has ever returned it on `/data/:object/:id`.
+
+**The old key was never readable at runtime — this rename reveals a defect, it
+does not break working code.** Both `delete` surfaces are pure `unwrapResponse`
+/ `_unwrap` passthroughs: the SDK returns the server's body untouched, so this
+interface is a *claim* about the wire, never a rewrite of it. The claim was
+false in the one direction that matters — the compiler endorsed the wrong
+spelling:
+
+```ts
+const r = await client.data.delete('task', id);
+if (r.deleted) { … }   // compiled; `undefined` at runtime; branch never taken
+if (r.success) { … }   // rejected by the compiler; correct on the wire
+```
+
+## What to change
+
+`r.deleted` → `r.success`. That is the whole migration. Nothing about the
+request, the route, the status codes or the error shapes changes, and no server
+needs upgrading: the value you are now allowed to read is the one that was
+already arriving.
+
+⛔ **Do not write `r.success ?? r.deleted`.** There is one producer shape, and a
+consumer that accepts two spellings is the shape contract-first exists to
+prevent — the same ruling #5581 applied on the producer side. No deprecated
+`deleted?: boolean` transition key ships for the same reason; a transition
+period is for keys that *worked*, and this one never did.
+
+## Why the type was wrong on every deployment, not just some
+
+The protocol path (`deleteData`) has always answered `success`. #5581 / PR
+#5641 brought the ObjectQL fallback — the path a slim assembly without
+`MetadataPlugin` takes — to the same shape. So before that fix the declaration
+was wrong on ordinary deployments and accidentally right on slim ones; after
+it, both paths answer `{ object, id, success }` and the declaration was simply
+wrong everywhere. The consumer-side correction had to follow the producer, not
+lead it.
+
+## `os data delete` was reading the phantom key too
+
+`packages/cli/src/commands/data/delete.ts` built its `--format json` / `--format
+yaml` payload with `deleted: result.deleted`. That evaluated to `undefined`, and
+`JSON.stringify` drops undefined values — so the `deleted` key the command has
+always declared **never appeared in a single run**. It now carries
+`result.success`, the server's own verdict.
+
+Observable change: `os data delete --format json` gains `deleted: true` (YAML
+likewise) on a successful delete. The key name stays `deleted` deliberately —
+it is the CLI's output key, not the protocol's, and the payload's top-level
+`success` already means something different (the CLI envelope's "the command
+completed"). Conflating the two is the hazard #5641 called out when it noted
+that `body.success` and `body.data.success` are different facts. Scripts
+reading `.deleted` from this command were reading `undefined` before and get a
+boolean now; nothing that worked stops working.
+
+## Downstream
+
+`objectui`'s `ObjectStackDataSource.delete()` is a live victim of the old
+declaration — it guards `emitMutation` on `result.deleted`, so the delete
+mutation event has never fired against a real server and the method returns
+`undefined` where it declares `boolean`. Its own suite stayed green because the
+fixture mocks `{ deleted: true }`, a body no server produces. Filed as
+objectstack-ai/objectui#3412, which is blocked on this package publishing —
+its fix is a type unblock, not a behaviour change, since `success` is already
+what arrives.
+
+## Pins
+
+`packages/client/src/data-delete-result-shape.test.ts` asserts mutual
+assignability between `DeleteDataResult` and the spec's `DeleteDataResponse`,
+so a rename on either side (or a re-added optional `deleted`) fails
+`check:test-typecheck`. `client.hono.test.ts` gains the delete case this live
+server suite never had: a real DELETE over HTTP whose body is read as
+`deleted.success` and whose key set is asserted literally — `z.object` strips
+unknown keys, so a passing parse alone cannot prove no stray `deleted` rode
+along.

content/docs/kernel/runtime-services/data-service.mdx

@@ -30,7 +30,8 @@ services.data.delete(object: string, id: string): Promise<DeleteDataResult>
 - `get`: single record payload
 - `find`: list payload + pagination metadata
 - `create`/`update`: mutated record payload
-- `delete`: success marker / deleted ID payload
+- `delete`: `{ object, id, success }` — the spec's `DeleteDataResponse`. The
+  flag is `success`, not `deleted` (#5638)
 
 ## Typical Errors
 

packages/cli/src/commands/data/delete.ts

@@ -57,15 +57,24 @@ export default class DataDelete extends Command {
       // Delete the record
       const result = await client.data.delete(args.object, args.id);
 
+      // [#5638] `deleted` is THIS COMMAND's output key; its value is the
+      // protocol's `DeleteDataResponse.success`. Two different booleans live
+      // in this payload and must not be conflated: the top-level `success` is
+      // the CLI envelope's "the command completed" flag (this branch is only
+      // reached when it did), while the server's flag is its statement that
+      // the deletion happened. Until now this read was `result.deleted` — a
+      // key no server has ever returned — so it evaluated to `undefined` and
+      // `JSON.stringify` dropped it: the documented key was simply absent
+      // from every `os data delete --format json` run.
       if (flags.format === 'json') {
         await emitJson({
           success: true,
           object: result.object,
           id: result.id,
-          deleted: result.deleted,
+          deleted: result.success,
         });
       } else if (flags.format === 'yaml') {
-        await formatOutput({ success: true, object: result.object, id: result.id, deleted: result.deleted }, 'yaml');
+        await formatOutput({ success: true, object: result.object, id: result.id, deleted: result.success }, 'yaml');
       } else {
         printSuccess(`Record deleted: ${result.id}`);
       }

packages/client/src/client.hono.test.ts

@@ -200,4 +200,37 @@ describe('ObjectStackClient (with Hono Server)', () => {
         expect(resultsResponse.records.length).toBeGreaterThan(0);
         expect(resultsResponse.records[0].name).toBe('Hono User');
     });
+
+    // [#5638] The one method whose declared return shape this suite never
+    // exercised — and the one that was wrong. `DeleteDataResult` claimed
+    // `deleted: boolean`; the schema it names declares `success`. This is the
+    // runtime half of the pin: a REAL delete, over HTTP, against the server
+    // this package's consumers talk to, read through the declared type.
+    //
+    // Nothing in this test is mocked into the answer: the DELETE route calls
+    // `protocol.deleteData` (rest-server.ts), NOT the broker shim above, so the
+    // body asserted here is the server's own.
+    it('should delete data via hono, answering the SPEC\'s `success` body', async () => {
+        const client = new ObjectStackClient({ baseUrl });
+        await client.connect();
+
+        const created = await client.data.create('customer', {
+            name: 'Doomed User',
+            email: 'doomed@example.com',
+        });
+
+        // Spec: DeleteDataResponse = { object, id, success }
+        const deleted = await client.data.delete('customer', created.id);
+        expect(deleted.success).toBe(true);
+        expect(deleted.object).toBe('customer');
+        expect(deleted.id).toBe(created.id);
+        // The undeclared key must not ride along (a passing schema parse would
+        // strip it silently, so assert the key set itself).
+        expect(Object.keys(deleted).sort()).toEqual(['id', 'object', 'success']);
+
+        // And the delete actually happened — a success flag nobody cross-checks
+        // is the cheapest thing in the world to keep green.
+        const remaining = await client.data.find('customer', { where: { id: created.id } });
+        expect(remaining.records.length).toBe(0);
+    });
 });

packages/client/src/data-delete-result-shape.test.ts

@@ -0,0 +1,112 @@
+/**
+ * [#5638] `DeleteDataResult` is a CLAIM about the server's delete body, and it
+ * has to be the claim its own comment makes: `Spec: DeleteDataResponseSchema`.
+ *
+ * It declared `{ object, id, deleted: boolean }` while
+ * `DeleteDataResponseSchema` declares `{ object, id, success: boolean }`. Both
+ * `delete` surfaces (`client.data.delete` and the project-scoped
+ * `client.project(id).data.delete`) are pure `unwrapResponse` / `_unwrap`
+ * passthroughs — no runtime rewriting anywhere — so a consumer writing
+ * `r.deleted` compiled fine and read `undefined` against every server path
+ * that exists (#5581 / PR #5641 brought the ObjectQL fallback to `success`
+ * too, so both producer paths now answer the same shape).
+ *
+ * WHICH LAYER ACTUALLY MOVES, and why that is not the usual one: the fix is a
+ * type declaration, and types are erased before vitest ever runs. Reverting
+ * `success` back to `deleted` therefore leaves every runtime assertion below
+ * GREEN — they assert on a body the mock/server produced, which the client
+ * never touched either way. The assertion that goes red on revert is the
+ * type-level one (`SpecAndSdkAgree`), plus the `r.success` reads, and both are
+ * enforced by `pnpm --filter @objectstack/client typecheck` → `check:test-typecheck`
+ * (#5449/#5546), which compiles this file with the package's full strictness
+ * and no debt-ledger entry. The runtime assertions below are shape pins: they
+ * pin that the passthrough stays a passthrough, which is what makes the type
+ * the only thing standing between a consumer and the wire.
+ */
+import { describe, it, expect, vi } from 'vitest';
+import { DeleteDataResponseSchema, type DeleteDataResponse } from '@objectstack/spec/api';
+import { ObjectStackClient, type DeleteDataResult } from './index';
+
+/** Mutual assignability — `A extends B` alone would pass on a strict subset. */
+type Exact<A, B> = [A] extends [B] ? ([B] extends [A] ? true : false) : false;
+
+/**
+ * The pin. `DeleteDataResult` must be structurally the spec type, field for
+ * field: rename either side, add a key to one, or make one optional, and this
+ * stops being `true` — a type error at `check:test-typecheck`, not a runtime
+ * failure. (This is deliberately an equality, not `extends`: the old `deleted`
+ * declaration was neither a subset nor a superset of the schema, and a
+ * one-directional check would still have caught it — but a future
+ * `success: boolean; deleted?: boolean` "transition" shape would slip past
+ * one, and that shape is exactly what #5638's triage ruled out.)
+ */
+const SpecAndSdkAgree: Exact<DeleteDataResult, DeleteDataResponse> = true;
+
+/** Helper: a client whose fetch answers `body` with `status`. */
+function createMockClient(body: any, status = 200) {
+  const fetchMock = vi.fn().mockResolvedValue({
+    ok: status >= 200 && status < 300,
+    status,
+    statusText: status === 200 ? 'OK' : 'Error',
+    json: async () => body,
+    headers: new Headers(),
+  });
+  const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock });
+  return { client, fetchMock };
+}
+
+/** The body every server path answers a successful delete with (#5581). */
+const SPEC_BODY = { object: 'task', id: 't1', success: true };
+
+describe('[#5638] DeleteDataResult === DeleteDataResponseSchema', () => {
+  it('declares the schema\'s shape, field for field (type-level)', () => {
+    // Reading the binding is what keeps `noUnusedLocals` satisfied; the
+    // assertion that matters already happened in tsc, above.
+    expect(SpecAndSdkAgree).toBe(true);
+  });
+
+  it('the schema still declares `success` and has never declared `deleted`', () => {
+    const parsed = DeleteDataResponseSchema.safeParse(SPEC_BODY);
+    expect(parsed.success).toBe(true);
+    expect(DeleteDataResponseSchema.safeParse({ object: 'task', id: 't1', deleted: true }).success)
+      .toBe(false);
+  });
+
+  for (const [surface, del] of [
+    ['client.data.delete', (c: ObjectStackClient) => c.data.delete('task', 't1')],
+    ['client.project(id).data.delete', (c: ObjectStackClient) => c.project('proj-xyz').data.delete('task', 't1')],
+  ] as const) {
+    describe(surface, () => {
+      it('hands back the spec body, and `success` is reachable through the declared type', async () => {
+        const { client } = createMockClient(SPEC_BODY);
+        const r: DeleteDataResult = await del(client);
+
+        // The typed read. Under the pre-#5638 declaration this line is
+        // TS2339 (`success` does not exist) — and the line a consumer WOULD
+        // have written, `r.deleted`, compiled and evaluated to `undefined`.
+        expect(r.success).toBe(true);
+        expect(r.object).toBe('task');
+        expect(r.id).toBe('t1');
+
+        // `z.object` strips unknown keys, so a passing `safeParse` cannot by
+        // itself prove no stray `deleted` came along (the #5641 caveat).
+        // Assert the key set literally as well.
+        expect(DeleteDataResponseSchema.safeParse(r).success).toBe(true);
+        expect(Object.keys(r).sort()).toEqual(['id', 'object', 'success']);
+        expect('deleted' in (r as object)).toBe(false);
+      });
+
+      it('does not manufacture `success` — an off-spec body passes through verbatim', async () => {
+        // Contract-first: the fix belongs in the type (and, for the server,
+        // in #5581), NOT in a tolerant consumer that reads
+        // `body.success ?? body.deleted`. If someone ever adds that alias,
+        // this test is where it shows up.
+        const { client } = createMockClient({ object: 'task', id: 't1', deleted: true });
+        const r = await del(client) as unknown as Record<string, unknown>;
+
+        expect(r.success).toBeUndefined();
+        expect(r.deleted).toBe(true);
+      });
+    });
+  }
+});

packages/client/src/index.ts

@@ -232,11 +232,21 @@ export interface UpdateDataResult<T = any> {
   droppedFields?: DroppedFieldsEvent[];
 }
 
-/** Spec: DeleteDataResponseSchema */
+/**
+ * Spec: DeleteDataResponseSchema
+ *
+ * [#5638] The success flag is `success`, matching the schema this comment
+ * names (`packages/spec/src/api/protocol.zod.ts`). It was declared `deleted`
+ * — a key no schema has ever declared and no server path has ever returned on
+ * `/data/:object/:id`, so `r.deleted` compiled and read `undefined` at
+ * runtime. Both `delete` surfaces below are pure `unwrapResponse` / `_unwrap`
+ * passthroughs: this interface is a claim about the server's body, never a
+ * rewrite of it, so the claim has to be the schema's.
+ */
 export interface DeleteDataResult {
   object: string;
   id: string;
-  deleted: boolean;
+  success: boolean;
 }
 
 export interface StandardError {