From e0733ce752d2e83bf6582e3bd46e9e4a000bc780 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:27:25 +0000 Subject: [PATCH 1/2] fix(plugin-sharing)!: the share-link routes emit the declared envelope, and the last ratchet retires (#3983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fifth and final drifting route module. Unlike the four in #3843 this one was not found by reading — `scripts/check-route-envelope.mjs` surfaced it the moment that scan went repo-wide, which is the argument for a repo-wide guard over per-package copies. It also turned out to be the one where the drift had broken shipped SDK methods, not merely mis-shaped a body. Three of these routes are `disposition: 'sdk'` in `runtime/src/route-ledger.ts`, and `unwrapResponse` decides a body is an envelope by finding a boolean `success`. With no flag it returns the body verbatim, so against this surface `shareLinks.create()` — documented "Returns the link row (incl. token)" — handed back `{ link: … }` and `.token` was undefined, while `shareLinks.list()`, typed `Promise`, handed back `{ links: [] }` and any `.map()` on it threw. `packages/client/src/admin-surfaces.test.ts` mocks all three as `{ success: true, data: }`: the SDK was written and tested against the DISPATCHER's shape and only ever worked there. So the target shape was not a design decision. `runtime/src/domains/share-links.ts` serves these same five paths — for cloud's per-environment kernels it is the designed PRIMARY surface — and has always answered in the declared envelope with `data` carrying the payload directly. This module now answers identically, which is what makes `unwrapResponse` return the same value either way: POST /share-links { link } -> data: link GET /share-links { links } -> data: link[] DELETE /share-links/:id { ok: true } -> data: { ok: true } GET /:token/resolve { record, link, redact… } -> data: { … } GET /:token/messages { data: rows } -> data: rows errors { error: { code, msg } } -> + success: false `{ ok: true }` survives on revoke, but as the payload rather than as the body: at the top level it was a second word for `success` (#3689 retired that from storage); under `data` it is what the dispatcher already returned. The error half was already nested `{ code, message }` — #3675's changeset cited this module as the good example — so only the flag is new there, and all eleven codes were already SCREAMING_SNAKE and registered, so ADR-0112 needs nothing. Consumers swept: the framework has ZERO. In objectui, ShareDialog was already dual-shape tolerant on all three routes it calls, and carried that tolerance precisely BECAUSE both shapes existed in the fleet. SharedRecordPage needed one fix of the kind a shape-swap would have missed — it renamed the wire's `redactFields` to `redactedFields` only on the bare branch, so on the enveloped dispatcher path the "fields are hidden by the owner" notice never rendered. Converting this surface would have spread that to every share page; fixed in objectui#2980, which merges first. The module had no test file at all (only `share-link-service.test.ts`), so it gains a driven conformance suite: 26 cases over all five routes and thirteen error branches, every body parsed against the real spec schemas, plus the `unwrapResponse` decision restated so the two broken SDK methods are pinned as working. Reverting the two emitters fails 22 of the 26. Guard: 7 conformant / 0 ratcheted / 1 exempt, from 6 / 1 / 1. `privateOk` is also narrowed to what its doc always claimed — a literal `ok` at the TOP of a body, where it competes with `success`; the same literal inside `data` is payload, which is what a conformant revoke returns. Four self-test assertions pin both readings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --- .changeset/share-link-routes-envelope.md | 90 ++++ .../share-link-envelope.conformance.test.ts | 485 ++++++++++++++++++ .../plugin-sharing/src/share-link-routes.ts | 92 +++- scripts/check-route-envelope.mjs | 86 ++-- 4 files changed, 710 insertions(+), 43 deletions(-) create mode 100644 .changeset/share-link-routes-envelope.md create mode 100644 packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts diff --git a/.changeset/share-link-routes-envelope.md b/.changeset/share-link-routes-envelope.md new file mode 100644 index 0000000000..e8833c87e2 --- /dev/null +++ b/.changeset/share-link-routes-envelope.md @@ -0,0 +1,90 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing)!: the share-link routes emit the declared envelope, and the last ratchet retires (#3983) + +The fifth and final drifting route module. Unlike the four in #3843, this one was +not found by reading — `scripts/check-route-envelope.mjs` surfaced it the moment +that scan went repo-wide, which is the whole argument for a repo-wide guard over +per-package copies. It also turned out to be the one where the drift had actually +**broken shipped SDK methods**, not merely mis-shaped a body. + +## Two SDK methods did not work on this surface + +Three of these routes are `disposition: 'sdk'` in `runtime/src/route-ledger.ts`, +and `ObjectStackClient.unwrapResponse` decides a body is an envelope by finding a +boolean `success`. With no flag it hands back the body verbatim: + +| method | documented / typed as | actually returned | +|---|---|---| +| `shareLinks.create()` | "the link row (incl. `token`)" | `{ link: … }` — so `.token` was `undefined` | +| `shareLinks.list()` | `Promise` | `{ links: [] }` — so `.map()` threw | + +`packages/client/src/admin-surfaces.test.ts` mocks all three as +`{ success: true, data: }`. The SDK was written and tested against the +**dispatcher's** shape and only ever worked there. + +## This is a convergence, not a redesign + +`runtime/src/domains/share-links.ts` serves the same five paths, and for cloud's +per-environment kernels it is the *designed primary* surface +(`registerShareLinkRoutes: false`). It has always answered in the declared +envelope. The plugin now answers identically: + +| route | was | now | +|---|---|---| +| `POST /share-links` | `{ link }` | `{ success: true, data: link }` | +| `GET /share-links` | `{ links }` | `{ success: true, data: link[] }` | +| `DELETE /share-links/:idOrToken` | `{ ok: true }` | `{ success: true, data: { ok: true } }` | +| `GET /:token/resolve` | `{ record, link, redactFields }` | `{ success: true, data: { … } }` | +| `GET /:token/messages` | `{ data: rows }` | `{ success: true, data: rows }` | +| errors | `{ error: { code, message } }` | `{ success: false, error: { … } }` | + +`data` carries each payload **directly** — `data: links`, not `data: { links }`. +That is what makes `unwrapResponse` return the same value on both surfaces, and +it is what the SDK already expected. + +## Breaking: raw `fetch` callers add one hop + +SDK callers get the fix for free (two of them go from broken to working). Direct +body readers add `.data`: + +```diff +- const { links } = await (await fetch('/api/v1/share-links')).json(); ++ const { data: links } = await (await fetch('/api/v1/share-links')).json(); +``` + +`{ ok: true }` on revoke survives, but as the payload rather than as the body: at +the top level it was a second word for `success`, which #3689 retired from +storage; under `data` it is what the dispatcher already returned. + +The `error` half was already nested `{ code, message }` — #3675's changeset cited +this module as the good example of that — so only the `success` flag is new there. +All eleven codes were already SCREAMING_SNAKE and registered, so ADR-0112 needs +nothing. + +## Consumers + +Swept, and the result is smaller than #3983 assumed. The framework has **zero** +consumers of these routes. In objectui, `ShareDialog` was already dual-shape +tolerant on all three routes it calls (`body.links ?? body.data`, +`created.link ?? created.data`, and revoke never reads the body) — it needs no +change, and it carried that tolerance precisely *because* both shapes existed in +the fleet. + +`SharedRecordPage` did need one fix, and it is the kind a shape-swap would have +missed: it renamed the wire's `redactFields` to `redactedFields` only on the +*bare* branch, so on the already-enveloped dispatcher path the "fields are hidden +by the owner" notice never rendered. Converting this surface would have spread +that to every share page. Fixed in objectui#2980, which merges first. + +## Guard + +**7 conformant / 0 ratcheted / 1 exempt**, from 6 / 1 / 1. The ratchet mechanism +stays for the next module that needs it. + +`privateOk` also got narrowed to what its own doc always claimed — a literal `ok` +at the **top** of a body, where it competes with `success`. The same literal +inside `data` is payload, which is what a conformant revoke returns. Four +self-test assertions pin both readings. diff --git a/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts b/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts new file mode 100644 index 0000000000..64108aeaf0 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts @@ -0,0 +1,485 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Response-envelope conformance for `/api/v1/share-links/*` (#3983). + * + * This module was the fifth drifting one, and the only one found by + * `scripts/check-route-envelope.mjs` rather than by #3843's hand survey — the + * case for a repo-wide scan over a per-package one. It was also the worst of the + * five, in a way "missing the `success` flag" undersells: + * + * Three of these routes are `disposition: 'sdk'` in `runtime/src/route-ledger.ts` + * (`shareLinks.create` / `.list` / `.revoke`), and `unwrapResponse` decides a body + * is an envelope by finding a boolean `success`. With no flag it returned the body + * verbatim, so `shareLinks.create()` — documented "Returns the link row (incl. + * `token`)" — handed back `{ link: … }` and `.token` was `undefined`, while + * `shareLinks.list()`, typed `Promise`, handed back `{ links: [] }` and any + * `.map()` on it threw. `packages/client/src/admin-surfaces.test.ts` mocks all + * three as `{ success: true, data: }`: the SDK was written and tested + * against the DISPATCHER's shape, and only worked there. + * + * So the target shape was not a design decision. `runtime/src/domains/share-links.ts` + * serves these same five paths (for cloud's per-environment kernels it is the + * designed PRIMARY surface), and has always answered `data: link`, `data: links`, + * `data: { ok: true }`, `data: { record, link, redactFields }`, `data: rows`. This + * module converged onto that. The `sameShapeAsDispatcher` suite at the bottom is + * what keeps the two from drifting apart again — the failure mode #3833 hit one + * domain over, where each surface held its own copy of a mapping. + * + * As with the four modules in #3843, the STATIC half — proving no route can bypass + * the `sendOk` / `sendError` pair — is not here. It is the repo-wide guard, which + * counts response write sites per module and now pins this one at the conformant + * `2 / 1 / 1`. What lives here is the half that has to sit next to the routes: + * every branch driven, every body parsed against the real spec schemas. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { BaseResponseSchema } from '@objectstack/spec/api'; +import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { registerShareLinkRoutes } from './share-link-routes'; +import type { ShareLinkService } from './share-link-service'; +import type { SharingEngine } from './sharing-service'; + +class MockHttp implements IHttpServer { + routes = new Map(); + private add(method: string, path: string, handler: RouteHandler) { + this.routes.set(`${method} ${path}`, handler); + } + get(path: string, h: RouteHandler) { this.add('GET', path, h); return this as any; } + post(path: string, h: RouteHandler) { this.add('POST', path, h); return this as any; } + put(path: string, h: RouteHandler) { this.add('PUT', path, h); return this as any; } + delete(path: string, h: RouteHandler) { this.add('DELETE', path, h); return this as any; } + patch(path: string, h: RouteHandler) { this.add('PATCH', path, h); return this as any; } + use() { return this as any; } + listen() { return Promise.resolve(); } + close() { return Promise.resolve(); } + getInstance() { return null; } +} + +interface Captured { status: number; body: any } + +const LINK_ROW = { + id: 'sl_1', + token: 'tok_abcdefgh', + object_name: 'crm_account', + record_id: 'acc_1', + permission: 'view', + audience: 'anyone', + expires_at: null, + label: 'For the auditor', + created_at: '2026-01-01T00:00:00.000Z', +}; + +/** + * The route layer is what is under test, so the service and engine are stubs. + * `overrides` lets one case make a single method throw or answer null without + * restating the rest. + */ +function mount(overrides: { + service?: Partial>; + engine?: Partial; + userId?: string | undefined; +} = {}) { + const service = { + createLink: vi.fn(async () => ({ ...LINK_ROW })), + listLinks: vi.fn(async () => [{ ...LINK_ROW }]), + revokeLink: vi.fn(async () => undefined), + resolveToken: vi.fn(async () => ({ link: { ...LINK_ROW }, redactFields: ['ssn'] })), + ...(overrides.service ?? {}), + } as unknown as ShareLinkService; + + const engine = { + find: vi.fn(async () => [{ id: 'acc_1', name: 'Acme', ssn: '123-45-6789' }]), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + ...(overrides.engine ?? {}), + } as unknown as SharingEngine; + + const http = new MockHttp(); + const userId = 'userId' in overrides ? overrides.userId : 'u_1'; + registerShareLinkRoutes(http, service, engine, { + contextFromRequest: () => (userId ? { userId, tenantId: 't_1' } : {}), + }); + return { http, service, engine }; +} + +async function drive( + http: MockHttp, + key: string, + opts: { params?: Record; body?: any; query?: any; headers?: any } = {}, +): Promise { + const handler = http.routes.get(key); + if (!handler) throw new Error(`no handler for ${key}`); + const captured: Captured = { status: 200, body: undefined }; + const res: IHttpResponse = { + json: vi.fn((data: any) => { captured.body = data; }) as any, + send: vi.fn() as any, + status: vi.fn((code: number) => { captured.status = code; return res; }) as any, + header: vi.fn(() => res) as any, + }; + const req: IHttpRequest = { + params: opts.params ?? {}, + query: opts.query ?? {}, + body: opts.body, + headers: opts.headers ?? {}, + method: 'GET', + path: '/', + }; + await handler(req, res); + return captured; +} + +const B = '/api/v1/share-links'; + +// ── Success bodies ──────────────────────────────────────────────────────────── + +describe('share-link envelope (#3983) — success bodies', () => { + const CASES: Array<{ + name: string; + status: number; + /** What `data` must BE, asserted against the value rather than by key. */ + expectData: (data: any) => void; + /** Keys the PRE-#3983 body carried at the top level, which must now be gone. */ + deadTopLevel: string[]; + run: () => Promise; + }> = [ + { + name: 'POST /share-links', + status: 201, + // `data` IS the link row — not `{ link }`. This is the assertion that makes + // `client.shareLinks.create().token` work through `unwrapResponse`. + expectData: (d) => expect(d).toMatchObject({ id: 'sl_1', token: 'tok_abcdefgh' }), + deadTopLevel: ['link'], + run: async () => drive(mount().http, `POST ${B}`, { body: { object: 'crm_account', recordId: 'acc_1' } }), + }, + { + name: 'GET /share-links', + status: 200, + // `data` IS the array — not `{ links }`. `shareLinks.list()` is typed + // `Promise`, and this is what makes that type true here. + expectData: (d) => { + expect(Array.isArray(d)).toBe(true); + expect(d[0]).toMatchObject({ id: 'sl_1' }); + }, + deadTopLevel: ['links'], + run: async () => drive(mount().http, `GET ${B}`), + }, + { + name: 'DELETE /share-links/:idOrToken', + status: 200, + // `{ ok: true }` survives, but as the PAYLOAD rather than as the body. At the + // top level it was a second word for `success` (#3689 retired that from + // storage); under `data` it is what the dispatcher twin already returns. + expectData: (d) => expect(d).toEqual({ ok: true }), + deadTopLevel: ['ok'], + run: async () => drive(mount().http, `DELETE ${B}/:idOrToken`, { params: { idOrToken: 'sl_1' } }), + }, + { + name: 'GET /share-links/:token/resolve', + status: 200, + expectData: (d) => { + expect(d.record).toMatchObject({ id: 'acc_1', name: 'Acme' }); + expect(d.link).toMatchObject({ token: 'tok_abcdefgh' }); + expect(d.redactFields).toEqual(['ssn']); + }, + deadTopLevel: ['record', 'link', 'redactFields'], + run: async () => drive(mount().http, `GET ${B}/:token/resolve`, { params: { token: 'tok_abcdefgh' } }), + }, + { + name: 'GET /share-links/:token/messages', + status: 200, + // The only route that already had a `data` key — but no flag, so + // `unwrapResponse` returned the WRAPPER `{ data: rows }` instead of `rows`. + expectData: (d) => { + expect(Array.isArray(d)).toBe(true); + expect(d[0]).toMatchObject({ id: 'm_1' }); + }, + deadTopLevel: [], + run: async () => { + const { http } = mount({ + service: { + resolveToken: vi.fn(async () => ({ + link: { ...LINK_ROW, object_name: 'ai_conversations' }, + redactFields: [], + })), + }, + engine: { find: vi.fn(async () => [{ id: 'm_1', role: 'user', content: 'hi' }]) }, + }); + return drive(http, `GET ${B}/:token/messages`, { params: { token: 'tok_abcdefgh' } }); + }, + }, + ]; + + for (const c of CASES) { + it(`${c.name} answers ${c.status} { success: true, data }`, async () => { + const { status, body } = await c.run(); + expect(status).toBe(c.status); + + // The contract itself, imported — not a restatement of it. + const parsed = BaseResponseSchema.safeParse(body); + expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + expect(body.success).toBe(true); + expect(body.error).toBeUndefined(); + c.expectData(body.data); + }); + } + + it('the pre-#3983 shape is dead — flag always present, payload no longer at the top', async () => { + for (const c of CASES) { + const { body } = await c.run(); + expect(typeof body.success, `${c.name} answers no success flag`).toBe('boolean'); + for (const k of c.deadTopLevel) { + expect(body[k], `${c.name} still answers a top-level ${k}`).toBeUndefined(); + } + } + }); + + it('no success body carries `ok` beside `success` — one word for one thing (#3689)', async () => { + for (const c of CASES) { + const { body } = await c.run(); + expect(body.ok, `${c.name} answers a top-level ok beside success`).toBeUndefined(); + } + }); +}); + +// ── unwrapResponse, the reason the flag matters ─────────────────────────────── + +describe('share-link envelope (#3983) — what the SDK now gets', () => { + /** + * `ObjectStackClient.unwrapResponse` in one line: a boolean `success` means the + * body is an envelope, so hand back `data`; otherwise hand back the body. Stated + * here rather than imported because `@objectstack/client` is not a dependency of + * this package — the point is the DECISION it makes on these bodies. + */ + const unwrap = (body: any) => (typeof body?.success === 'boolean' ? body.data : body); + + it('create yields the link row itself, so `.token` is defined', async () => { + const { body } = await drive(mount().http, `POST ${B}`, { + body: { object: 'crm_account', recordId: 'acc_1' }, + }); + // Pre-#3983 this was `{ link: {...} }` and `.token` was undefined — the + // documented return value of `client.shareLinks.create` did not exist. + expect(unwrap(body).token).toBe('tok_abcdefgh'); + }); + + it('list yields the array itself, so `.map` does not throw', async () => { + const { body } = await drive(mount().http, `GET ${B}`); + const links = unwrap(body); + expect(Array.isArray(links)).toBe(true); + // Pre-#3983 `unwrap` returned `{ links: [...] }` and this line threw. + expect(links.map((l: any) => l.id)).toEqual(['sl_1']); + }); + + it('messages yields the rows, not the { data } wrapper', async () => { + const { http } = mount({ + service: { + resolveToken: vi.fn(async () => ({ + link: { ...LINK_ROW, object_name: 'ai_conversations' }, + redactFields: [], + })), + }, + engine: { find: vi.fn(async () => [{ id: 'm_1' }]) }, + }); + const { body } = await drive(http, `GET ${B}/:token/messages`, { params: { token: 'tok_abcdefgh' } }); + expect(unwrap(body)).toEqual([{ id: 'm_1' }]); + }); +}); + +// ── Error bodies ────────────────────────────────────────────────────────────── + +describe('share-link envelope (#3983) — error bodies', () => { + const CASES: Array<{ name: string; status: number; code: string; run: () => Promise }> = [ + { + name: 'anonymous create', + status: 401, + code: 'UNAUTHENTICATED', + run: async () => drive(mount({ userId: undefined }).http, `POST ${B}`, { body: { object: 'a', recordId: 'b' } }), + }, + { + name: 'anonymous list', + status: 401, + code: 'UNAUTHENTICATED', + run: async () => drive(mount({ userId: undefined }).http, `GET ${B}`), + }, + { + name: 'anonymous revoke', + status: 401, + code: 'UNAUTHENTICATED', + run: async () => drive(mount({ userId: undefined }).http, `DELETE ${B}/:idOrToken`, { + params: { idOrToken: 'sl_1' }, + }), + }, + { + name: 'create without object / recordId', + status: 400, + code: 'VALIDATION_FAILED', + run: async () => drive(mount().http, `POST ${B}`, { body: {} }), + }, + { + name: 'create where the service throws', + status: 500, + code: 'INTERNAL', + run: async () => drive( + mount({ service: { createLink: vi.fn(async () => { throw new Error('boom'); }) } }).http, + `POST ${B}`, + { body: { object: 'a', recordId: 'b' } }, + ), + }, + { + name: 'resolve of a password-gated link with no password', + status: 401, + code: 'NEEDS_PASSWORD', + run: async () => drive( + mount({ + service: { resolveToken: vi.fn(async () => null) }, + engine: { find: vi.fn(async () => [{ token: 'tok_abcdefgh', password_hash: 'h', revoked_at: null, expires_at: null }]) }, + }).http, + `GET ${B}/:token/resolve`, + { params: { token: 'tok_abcdefgh' } }, + ), + }, + { + name: 'resolve of a password-gated link with the WRONG password', + status: 401, + code: 'WRONG_PASSWORD', + run: async () => drive( + mount({ + service: { resolveToken: vi.fn(async () => null) }, + engine: { find: vi.fn(async () => [{ token: 'tok_abcdefgh', password_hash: 'h', revoked_at: null, expires_at: null }]) }, + }).http, + `GET ${B}/:token/resolve`, + { params: { token: 'tok_abcdefgh' }, query: { password: 'nope' } }, + ), + }, + { + name: 'resolve of an audience=signed_in link while anonymous', + status: 401, + code: 'SIGN_IN_REQUIRED', + run: async () => drive( + mount({ + userId: undefined, + service: { resolveToken: vi.fn(async () => null) }, + engine: { find: vi.fn(async () => [{ token: 'tok_abcdefgh', audience: 'signed_in', revoked_at: null, expires_at: null }]) }, + }).http, + `GET ${B}/:token/resolve`, + { params: { token: 'tok_abcdefgh' } }, + ), + }, + { + name: 'resolve of a revoked link', + status: 410, + code: 'EXPIRED_OR_REVOKED', + run: async () => drive( + mount({ + service: { resolveToken: vi.fn(async () => null) }, + engine: { find: vi.fn(async () => [{ token: 'tok_abcdefgh', revoked_at: '2026-01-02T00:00:00.000Z' }]) }, + }).http, + `GET ${B}/:token/resolve`, + { params: { token: 'tok_abcdefgh' } }, + ), + }, + { + name: 'resolve of a token that does not exist', + status: 404, + code: 'INVALID_OR_EXPIRED', + run: async () => drive( + mount({ service: { resolveToken: vi.fn(async () => null) }, engine: { find: vi.fn(async () => []) } }).http, + `GET ${B}/:token/resolve`, + { params: { token: 'tok_abcdefgh' } }, + ), + }, + { + name: 'resolve where the underlying record is gone', + status: 410, + code: 'RECORD_GONE', + run: async () => drive( + mount({ engine: { find: vi.fn(async () => []) } }).http, + `GET ${B}/:token/resolve`, + { params: { token: 'tok_abcdefgh' } }, + ), + }, + { + name: 'messages for an unresolvable token', + status: 404, + code: 'NOT_FOUND', + run: async () => drive( + mount({ service: { resolveToken: vi.fn(async () => null) } }).http, + `GET ${B}/:token/messages`, + { params: { token: 'tok_abcdefgh' } }, + ), + }, + { + // The link resolves, but it shares a `crm_account` — messages are an + // `ai_conversations`-only affordance. + name: 'messages for a link that is not an ai_conversations', + status: 400, + code: 'UNSUPPORTED', + run: async () => drive(mount().http, `GET ${B}/:token/messages`, { params: { token: 'tok_abcdefgh' } }), + }, + ]; + + for (const c of CASES) { + it(`${c.name} → ${c.status} ${c.code}, in the declared envelope`, async () => { + const { status, body } = await c.run(); + expect(status).toBe(c.status); + + const parsed = BaseResponseSchema.safeParse(body); + expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + + expect(body.success).toBe(false); + expect(body.error.code).toBe(c.code); + expect(typeof body.error.message).toBe('string'); + expect(body.error.message.length).toBeGreaterThan(0); + + // `error` was already nested here before #3983 — #3675's changeset cited + // this module as the good example of that. Pinned so a revert to the + // bare-string dialect the sibling modules carried cannot land quietly. + expect(typeof body.error).not.toBe('string'); + expect(body.code).toBeUndefined(); + }); + } + + it('every code is SCREAMING_SNAKE, per ADR-0112', async () => { + for (const c of CASES) { + const { body } = await c.run(); + expect(body.error.code, `${c.name} answers a non-conforming code`).toMatch(/^[A-Z][A-Z0-9_]*$/); + } + }); +}); + +// ── The two surfaces must not drift apart again ─────────────────────────────── + +describe('share-link envelope (#3983) — same shape as the dispatcher twin', () => { + /** + * `runtime/src/domains/share-links.ts` serves these same five paths and is the + * designed primary surface for cloud's per-environment kernels. Restated here as + * the shape of `data` per route, because the runtime is not a dependency of this + * package — importing it would invert the dependency direction for a test. + * + * The dispatcher additionally emits a legacy `link` / `links` key ALONGSIDE + * `data` on create and list. That is a producer-side transition shim for readers + * predating the envelope; it is deliberately NOT copied here, and deleting it + * there is tracked separately (it needs a cloud-side sweep). + */ + it('data is the payload directly on every route, matching the dispatcher', async () => { + const create = await drive(mount().http, `POST ${B}`, { body: { object: 'a', recordId: 'b' } }); + expect(create.body.data).toMatchObject({ token: 'tok_abcdefgh' }); // dispatcher: data: link + + const list = await drive(mount().http, `GET ${B}`); + expect(Array.isArray(list.body.data)).toBe(true); // dispatcher: data: links + + const revoke = await drive(mount().http, `DELETE ${B}/:idOrToken`, { params: { idOrToken: 'sl_1' } }); + expect(revoke.body.data).toEqual({ ok: true }); // dispatcher: data: { ok: true } + + const resolve = await drive(mount().http, `GET ${B}/:token/resolve`, { params: { token: 'tok_abcdefgh' } }); + expect(Object.keys(resolve.body.data).sort()) // dispatcher: data: { record, link, redactFields } + .toEqual(['link', 'record', 'redactFields']); + }); + + it('redaction still happens, and `redactFields` names what was stripped', async () => { + const { body } = await drive(mount().http, `GET ${B}/:token/resolve`, { params: { token: 'tok_abcdefgh' } }); + // The stub record carries `ssn`; the stub link redacts it. + expect(body.data.record.ssn).toBeUndefined(); + expect(body.data.record.name).toBe('Acme'); + expect(body.data.redactFields).toEqual(['ssn']); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/share-link-routes.ts b/packages/plugins/plugin-sharing/src/share-link-routes.ts index 754a2a8998..3f08ffdd48 100644 --- a/packages/plugins/plugin-sharing/src/share-link-routes.ts +++ b/packages/plugins/plugin-sharing/src/share-link-routes.ts @@ -3,10 +3,18 @@ /** * REST surface for ShareLinkService. * - * POST /api/v1/share-links → create a link - * GET /api/v1/share-links → list links (?object, ?recordId, ?includeRevoked) - * DELETE /api/v1/share-links/:idOrToken → revoke - * GET /api/v1/share-links/:token/resolve → resolve token, returns { record, link, redactFields } + * POST /api/v1/share-links → create a link → data: link + * GET /api/v1/share-links → list links → data: link[] + * (?object, ?recordId, ?includeRevoked) + * DELETE /api/v1/share-links/:idOrToken → revoke → data: { ok: true } + * GET /api/v1/share-links/:token/resolve → resolve token → data: { record, link, redactFields } + * GET /api/v1/share-links/:token/messages → conversation rows → data: ai_messages[] + * + * Every body is the declared `{ success: true, data }` / `{ success: false, + * error: { code, message } }` envelope, built in exactly two places — {@link sendOk} + * and {@link sendError}. Read `sendOk`'s note first: the shapes above are the + * dispatcher twin's, which this module converged onto in #3983, and the `success` + * flag they used to omit is why two `client.shareLinks.*` methods were broken here. * * The resolve route is intentionally public — it's the only endpoint * holders of a token need. It does: @@ -48,8 +56,63 @@ export interface ShareLinkRoutesOptions { // verified `contextFromRequest` (the plugin does). const defaultContext = (_req: IHttpRequest): ShareLinkExecutionContext => ({}); +/** + * Emit an error in the DECLARED envelope — `BaseResponseSchema` + + * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. + * `{ success: false, error: { code, message } }`. + * + * The nested `{ code, message }` was already right (#3675's changeset cited this + * module as the good example of it); the `success` flag is what was missing. + * See {@link sendOk} for why that flag is load-bearing rather than decorative. + */ function sendError(res: IHttpResponse, status: number, code: string, message: string) { - res.status(status).json({ error: { code, message } }); + res.status(status).json({ success: false, error: { code, message } }); +} + +/** + * Emit a success body in the DECLARED envelope — `BaseResponseSchema` + * (`packages/spec/src/api/contract.zod.ts`), i.e. `{ success: true, data }`, + * with `data` carrying each route's payload DIRECTLY. + * + * ## This is a convergence, not a new dialect + * + * These five routes have a twin: `runtime/src/domains/share-links.ts` serves + * the same paths off the dispatcher, and for cloud's per-environment kernels + * that twin is the DESIGNED PRIMARY surface (`registerShareLinkRoutes: false`). + * It has always answered in the declared envelope with `data` as the payload — + * `data: link`, `data: links`, `data: { ok: true }`, `data: { record, link, + * redactFields }`, `data: rows`. This module answered `{ link }`, `{ links }`, + * `{ ok: true }`, `{ record, link, redactFields }`, `{ data: rows }`. Same + * routes, two shapes, decided by which surface happened to mount them — the + * asymmetry #3636 fixed for `/i18n`, one domain over. + * + * ## What that asymmetry actually broke + * + * Three of these routes are `disposition: 'sdk'` in `runtime/src/route-ledger.ts` + * (`shareLinks.create` / `.list` / `.revoke`), and `ObjectStackClient.unwrapResponse` + * keys on a boolean `success`. With no flag it returns the body verbatim, so + * against THIS surface: + * + * • `shareLinks.create()` — documented "Returns the link row (incl. `token`)" + * — handed back `{ link: … }`, making `.token` `undefined`. + * • `shareLinks.list()` — typed `Promise` — handed back `{ links: [] }`, + * so any `.map()` on it threw. + * + * `packages/client/src/admin-surfaces.test.ts` mocks all three as + * `{ success: true, data: }`: the SDK was written and tested against + * the dispatcher's shape. So this is not envelope cosmetics — it is why two SDK + * methods did not work on the plugin surface at all. + * + * ## Why `data` carries the payload bare + * + * `sendOk(res, links)`, not `sendOk(res, { links })`. That is what makes + * `unwrapResponse` return the same value on both surfaces, and it is what the + * consumers already read (`body.links ?? body.data`, `created.link ?? created.data`, + * `body?.data ?? []`) — they carry that tolerance precisely BECAUSE both shapes + * exist in the fleet. Prime Directive #12: the shim goes once the producer agrees. + */ +function sendOk(res: IHttpResponse, data: unknown, status = 200): void { + res.status(status).json({ success: true, data }); } /** Strip `redactFields` from a record (also removes from nested arrays of objects). */ @@ -101,7 +164,7 @@ export function registerShareLinkRoutes( // endpoint also returns it (admins need to copy/recreate URLs), // but downstream API consumers typically derive the public URL // from `link.token` immediately. - await res.status(201).json({ link }); + sendOk(res, link, 201); } catch (err: any) { sendError(res, err?.status ?? 500, err?.code ?? 'INTERNAL', err?.message ?? 'Failed to create link'); } @@ -113,7 +176,7 @@ export function registerShareLinkRoutes( const ctx = await ctxOf(req); if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to list share links'); const q = req.query ?? {}; - const link = await service.listLinks( + const links = await service.listLinks( { object: typeof q.object === 'string' ? q.object : undefined, recordId: typeof q.recordId === 'string' ? q.recordId : undefined, @@ -124,7 +187,7 @@ export function registerShareLinkRoutes( }, ctx, ); - await res.json({ links: link }); + sendOk(res, links); } catch (err: any) { sendError(res, err?.status ?? 500, err?.code ?? 'INTERNAL', err?.message ?? 'Failed to list links'); } @@ -136,7 +199,11 @@ export function registerShareLinkRoutes( const ctx = await ctxOf(req); if (!ctx.userId) return sendError(res, 401, 'UNAUTHENTICATED', 'Sign in to revoke share links'); await service.revokeLink(req.params.idOrToken, ctx); - await res.status(200).json({ ok: true }); + // `{ ok: true }` moves from BEING the body to being its `data`. It was a + // second word for `success` at the top level (#3689 retired that from + // storage); inside `data` it is just the payload, and it is the payload the + // dispatcher twin and `admin-surfaces.test.ts` both already use. + sendOk(res, { ok: true }); } catch (err: any) { sendError(res, err?.status ?? 500, err?.code ?? 'INTERNAL', err?.message ?? 'Failed to revoke link'); } @@ -206,7 +273,7 @@ export function registerShareLinkRoutes( return sendError(res, 410, 'RECORD_GONE', 'The shared record no longer exists'); } - await res.json({ + sendOk(res, { record: applyRedaction(record, resolved.redactFields), link: { id: resolved.link.id, @@ -262,7 +329,10 @@ export function registerShareLinkRoutes( limit: 500, context: SYSTEM_CTX, } as any); - res.status(200).json({ data: rows ?? [] }); + // Already had a `data` key, but no `success` flag — so `unwrapResponse` + // returned the WRAPPER `{ data: rows }` rather than `rows`. Adding the flag + // is what makes the same read (`body.data`) work through the SDK too. + sendOk(res, rows ?? []); } catch (err: any) { sendError( res, diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index f783f7fe24..6b48e68989 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -32,8 +32,10 @@ * lifting it to a repo-wide scan buys the thing per-package copies structurally * cannot: **a module nobody thought to convert still gets audited.** Two modules * in the table below were found exactly that way, neither of them in #3843's - * hand-written survey — `share-link-routes.ts` (ratcheted, #3983) and - * `hmr-routes.ts` (exempt). + * hand-written survey — `share-link-routes.ts` (ratcheted on discovery, converted + * by #3983) and `hmr-routes.ts` (exempt). The first turned out to be the one where + * the drift had actually broken SDK methods, which is the case for scanning rather + * than surveying. * * A module discovered by the scan but absent from the table is an ERROR, not a * default: silently applying `2 / 1 / 1` to an unknown module would let a new @@ -78,7 +80,9 @@ const ROOT = join(fileURLToPath(new URL('.', import.meta.url)), '..'); * * responses — response write sites (`res.json(…)`); one per envelope builder * ok / err — literal `success: true` / `success: false` (one builder each) - * privateOk — literal `ok: true|false`, a second word for `success` (#3689) + * privateOk — literal `ok: true|false` at the TOP of a response body, i.e. a + * sibling of where `success` belongs: a second word for it (#3689). + * Inside `data` the same literal is payload, not a flag (#3983). * stringError— bodies whose `error` is a bare string (the pre-#3675 dialect) * ratchet — set ONLY for a module with outstanding drift. It pins the * CURRENT numbers so nothing gets worse, and names the issue that @@ -102,6 +106,11 @@ const MODULES = { // but built it inline in four places, so this module carried a ratchet at 5 / 4 / 1 // until those collapsed behind a `sendOk`. 'packages/services/service-i18n/src/i18n-service-plugin.ts': { responses: 2, ok: 1, err: 1 }, + // Converted by #3983, the last ratchet. This module was never emitting a + // `success` flag at all, which broke `client.shareLinks.create()`/`.list()` + // through `unwrapResponse`; it converged onto the shapes its dispatcher twin + // (`runtime/src/domains/share-links.ts`) had always returned. + 'packages/plugins/plugin-sharing/src/share-link-routes.ts': { responses: 2, ok: 1, err: 1 }, // ── Exempt ────────────────────────────────────────────────────────────── @@ -120,16 +129,10 @@ const MODULES = { }, // ── Ratchet: real, tracked, NOT blessed ───────────────────────────────── - - // Found BY THIS SCAN, absent from #3843's survey — the fifth drifting module. - // `sendError` already nests `{ code, message }` (that is why #3675's changeset - // cited it as a good example), but NO body carries the `success` flag, and one - // answers `{ ok: true }` — the private second word #3689 retired from storage. - // Converting it is breaking for share-link consumers and needs its own - // consumer sweep, exactly as #3687 did; it is not riding along with #3843. - 'packages/plugins/plugin-sharing/src/share-link-routes.ts': { - responses: 6, ok: 0, err: 0, privateOk: 1, ratchet: '#3983', - }, + // + // Empty as of #3983. The mechanism stays — it is how the next drifting module + // gets recorded honestly instead of being either fixed on the spot or quietly + // skipped. Declare current counts plus a `ratchet` naming the issue. }; /** Identifiers whose `.json()` READS a request rather than writing a response. */ @@ -167,35 +170,41 @@ export function scanSource(source, fileName = 'module.ts') { found.responses += 1; found.sites.push(`${fileName}:${line(node)}`); - // Is the first argument an object literal whose `error` is a bare string? + // Facts that only mean something at the ROOT of a response body, so they + // are read off this call's own object literal rather than the whole module. const arg = node.arguments[0]; if (arg && ts.isObjectLiteralExpression(arg)) { for (const prop of arg.properties) { + if (!ts.isPropertyAssignment(prop) || !ts.isIdentifier(prop.name)) continue; + const key = prop.name.text; + const init = prop.initializer; + // `error` as a bare string — the pre-#3675 dialect. if ( - ts.isPropertyAssignment(prop) && - ts.isIdentifier(prop.name) && - prop.name.text === 'error' && - (ts.isStringLiteral(prop.initializer) || ts.isTemplateExpression(prop.initializer) || - ts.isNoSubstitutionTemplateLiteral(prop.initializer)) + key === 'error' && + (ts.isStringLiteral(init) || ts.isTemplateExpression(init) || + ts.isNoSubstitutionTemplateLiteral(init)) ) { found.stringError += 1; } + // A literal `ok` is a second word for `success` only where it could BE + // the flag: a sibling of `success` at the top of the body. The same + // literal inside `data` is payload — `data: { ok: true }` is what a + // revoke endpoint legitimately returns, and the dispatcher twin + // (`runtime/src/domains/share-links.ts`) has always returned it (#3983). + if (key === 'ok' && (init.kind === ts.SyntaxKind.TrueKeyword || init.kind === ts.SyntaxKind.FalseKeyword)) { + found.privateOk += 1; + } } } } - // Literal envelope flags, anywhere in the module. - if (ts.isPropertyAssignment(node) && ts.isIdentifier(node.name)) { - const key = node.name.text; - const kind = node.initializer.kind; - const isTrue = kind === ts.SyntaxKind.TrueKeyword; - const isFalse = kind === ts.SyntaxKind.FalseKeyword; - if (key === 'success' && isTrue) found.ok += 1; - if (key === 'success' && isFalse) found.err += 1; - // A COMPUTED `ok` is a domain verdict that happens to share the name — - // `POST /external/validate` reports `ok: results.every(r => r.ok)`. Only a - // literal is a second envelope flag. - if (key === 'ok' && (isTrue || isFalse)) found.privateOk += 1; + // The `success` flag counts ANYWHERE in the module, unlike `ok` above: it is + // the envelope's own flag wherever the body gets built, including the + // `const body = { success: true, data }; res.json(body)` form that a + // call-local scan cannot see. + if (ts.isPropertyAssignment(node) && ts.isIdentifier(node.name) && node.name.text === 'success') { + if (node.initializer.kind === ts.SyntaxKind.TrueKeyword) found.ok += 1; + if (node.initializer.kind === ts.SyntaxKind.FalseKeyword) found.err += 1; } ts.forEachChild(node, visit); @@ -329,11 +338,24 @@ function selfTest() { r = scanSource(`res.status(503).json({ error: 'datasource_admin_unavailable' });`); assert(r.stringError === 1, `bare-string error not caught → ${JSON.stringify(r)}`); - // A literal `ok` is a second success word; a COMPUTED one is a domain verdict. + // A literal `ok` at the top of a body is a second success word. r = scanSource(`res.json({ ok: true, key });`); assert(r.privateOk === 1, `literal ok not caught → ${JSON.stringify(r)}`); + // A COMPUTED one is a domain verdict that happens to share the name — + // `POST /external/validate` reports `ok: results.every(r => r.ok)`. r = scanSource(`sendOk(res, { ok: results.every((x) => x.ok), results });`); assert(r.privateOk === 0, `computed ok must be left alone → ${JSON.stringify(r)}`); + // …and a literal one INSIDE `data` is payload, not a competing flag. Both + // forms below are what a conformant revoke endpoint returns (#3983); the + // `responses`/`ok`/`err` counts stay the real guarantee that the two writers + // are the enveloped ones. + r = scanSource(`res.json({ success: true, data: { ok: true } });`); + assert(r.privateOk === 0 && r.ok === 1, `nested ok is payload → ${JSON.stringify(r)}`); + r = scanSource(`${sound}\nhttp.delete('/c', (q, res) => sendOk(res, { ok: true }));`); + assert( + r.privateOk === 0 && r.responses === 2 && r.ok === 1 && r.err === 1, + `ok passed as a helper's data must not count → ${JSON.stringify(r)}`, + ); console.log('✓ check-route-envelope self-test passed'); } From 3a67ab4bf710bccf706cc59c27eff67bac86e835 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:39:15 +0000 Subject: [PATCH 2/2] chore(console): bump the objectui pin to 96ee72e so the enveloped resolve body meets a page that renames redactFields (#3983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 2 of #3983's merge order, in the same commit range as the conversion so the coupling is atomic: merging the envelope change against a console dist built from the old pin would ship enveloped resolve bodies to a page that only renames `redactFields` -> `redactedFields` on the BARE branch, so the "fields are hidden by the owner" notice would vanish from every share page for one release window. objectui#2980 fixes that and is the pin's new tip. Also fixes `bump-objectui.sh` for the workflow AGENTS.md mandates. Its guard tested `-d "$OBJECTUI_ROOT/.git"`, but in a git WORKTREE `.git` is a regular file holding a `gitdir:` pointer — so the script rejected every linked worktree and only ever ran from a primary clone, while the Prime Directives require a per-task worktree in each repo a task spans. `-e` accepts both. The error message also claimed the default sibling path even when OBJECTUI_ROOT was set explicitly, which pointed the reader at the wrong thing; it now names the path it actually rejected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --- .changeset/console-96ee72e85439.md | 15 +++++++++++++++ .objectui-sha | 2 +- scripts/bump-objectui.sh | 12 ++++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 .changeset/console-96ee72e85439.md diff --git a/.changeset/console-96ee72e85439.md b/.changeset/console-96ee72e85439.md new file mode 100644 index 0000000000..031902fb74 --- /dev/null +++ b/.changeset/console-96ee72e85439.md @@ -0,0 +1,15 @@ +--- +"@objectstack/console": minor +--- + +Console (objectui) refreshed to `96ee72e85439`. Frontend changes in this range: + +- fix(console): render the redaction notice on the enveloped resolve body (objectstack#3983) (#2980) +- feat(sdui): guard the public contract against silent drift (#2979) +- fix(sdui): lazy public blocks reach a kind:'react' page scope; ReactRunner keeps its errors (#2976) +- fix(list,data): bridge every spec view operator onto the filter AST (#2901) (#2974) +- fix(errors): error-code branches survive the framework's ADR-0112 rename (objectstack#3841) (#2977) +- fix(fields): a select no longer wipes itself when its value outruns its options (#2968) (#2969) +- fix(approvals): decision outputs reach both decision surfaces (#2955) (#2961) + +objectui range: `e651c936870e...96ee72e85439` diff --git a/.objectui-sha b/.objectui-sha index cb5ea11493..1f4ede2220 100644 --- a/.objectui-sha +++ b/.objectui-sha @@ -1 +1 @@ -e651c936870e26ef0c15252baad479d293fb3cf3 +96ee72e85439b439fefc8c1f378818d263718dfa diff --git a/scripts/bump-objectui.sh b/scripts/bump-objectui.sh index 785c8ea02e..e442d2af7e 100755 --- a/scripts/bump-objectui.sh +++ b/scripts/bump-objectui.sh @@ -48,8 +48,16 @@ for arg in "$@"; do esac done -if [[ -z "${OBJECTUI_ROOT}" || ! -d "${OBJECTUI_ROOT}/.git" ]]; then - echo "✗ Cannot find objectui checkout at ${FRAMEWORK_ROOT}/../objectui" +# `-e`, not `-d`: in a git WORKTREE `.git` is a regular file holding a `gitdir:` +# pointer, so a `-d` test rejected every linked worktree — and AGENTS.md requires +# one per task, so this rejected the mandated workflow and only ever worked from a +# primary clone. +if [[ -z "${OBJECTUI_ROOT}" || ! -e "${OBJECTUI_ROOT}/.git" ]]; then + if [[ -n "${OBJECTUI_ROOT}" ]]; then + echo "✗ ${OBJECTUI_ROOT} is not a git checkout (no .git)" + else + echo "✗ Cannot find objectui checkout at ${FRAMEWORK_ROOT}/../objectui" + fi echo " Override with: OBJECTUI_ROOT=/path/to/objectui scripts/bump-objectui.sh" exit 1 fi