diff --git a/.changeset/write-path-refusal-code-parity.md b/.changeset/write-path-refusal-code-parity.md new file mode 100644 index 0000000000..e801df08dc --- /dev/null +++ b/.changeset/write-path-refusal-code-parity.md @@ -0,0 +1,42 @@ +--- +'@objectstack/rest': patch +--- + +Serve the `code` a sandboxed hook declared on `/api/v1/data` refusals, at every +status — not only where a bespoke arm happened to catch the condition first + +Measured on a booted 17.1.0 server: a hook throwing +`Object.assign(new Error(msg), { code: 'RECORD_LOCKED', status: 409 })` reached +the client as `409 {"error":"…","object":"crm_opportunity"}` — the status but no +machine-readable `code`. Same for `DUPLICATE_VALUE` on `POST` and `FORBIDDEN` on +`403`, while `DELETE_RESTRICTED` at the same 409 and `VALIDATION_FAILED` at 400 +carried theirs. A client that must tell "this record is frozen, do not retry" +from "this value is already taken, offer a merge" got `409` for both and had to +substring-match prose that is localised and deliberately reworded over time — +the failure mode the ADR-0112 `code` vocabulary exists to remove. + +The branch is `classifyDataError`'s **sandbox unwrap door** in +`error-response.ts` (`typeof error?.innerMessage === 'string'`), which rendered +from the raw error and emitted no `code` at all, while every arm around it +renders from the resolved envelope. That is the whole of the reported +correlation between "no `code`" and "the unwrapped message": they are one +branch, not cause and effect. + +It was never the status policy it looked like from outside. The door dropped +`code` on a declared **400** exactly as on a declared 409, and kept it on a +declared 5xx by falling through to the passthrough below — one sandboxed +producer, its code surviving 503 and lost at 409. What made the reading look +status-shaped is which codes have a bespoke arm above the door: +`DELETE_RESTRICTED` and `VALIDATION_FAILED` do and never reach it, +`RECORD_LOCKED` / `DUPLICATE_VALUE` / `FORBIDDEN` do not and did. + +The code now rides via `thrownCodeFields`, the one definition the three sibling +arms already use, so the door joins the closed ADR-0112 vocabulary: a registered +spelling arrives verbatim, an unregistered one is demoted to `declaredCode` +beside the status-derived member. Nothing is invented — a producer that declared +no code still gets a body carrying none. + +Unchanged: the business message is still the unwrapped `innerMessage` (never the +`hook 'x' threw: …` debug wrapper), a crashing hook body is still the sanitised +`500 INTERNAL_ERROR`, a declared 5xx still withholds its prose, and a refusal +that declares no status still answers 400. diff --git a/packages/rest/src/error-response.ts b/packages/rest/src/error-response.ts index 5ab7b4a8e5..bfe7b80adc 100644 --- a/packages/rest/src/error-response.ts +++ b/packages/rest/src/error-response.ts @@ -362,6 +362,12 @@ function declaredHttpStatus(error: any): number | undefined { * reaches the flat body at all. It could not have been a legal ADR-0112 code in * any case; a number in the field callers branch on is the loudest possible * violation of a closed vocabulary. All four flat arms now ask ONE question. + * + * [#10345] Five arms, since the sandbox unwrap door joined them. It emitted no + * `code` at all, so #9232 found nothing there to narrow and left it out — and + * an exit that never speaks the vocabulary is the one a vocabulary sweep + * cannot see. `rest-thrown-code-vocabulary.test.ts`'s `ARMS` table enumerates + * all five. */ function thrownCodeFields(error: any, status: number): { code?: string; declaredCode?: string } { const thrown = resolveThrownHttpError(error, status); @@ -613,9 +619,48 @@ function classifyDataError(error: any, object?: string): { status: number; body: // the custom-action route performs in http-dispatcher's handleAction. // The full wrapper still reaches server logs via the callers' // "[REST] Unhandled error" logging and the BodyRunner's own error log. - // Deliberately NO `code` field: older @objectstack/client builds (still - // bundled in deployed consoles) prepend any `code` to the human-readable - // message, which would reintroduce the English noise this branch removes. + // + // [#10345] The `code` the producer declared rides too — via + // {@link thrownCodeFields}, the same one definition the three arms around + // it use. This branch used to omit `code` unconditionally, and that + // omission is what the card measured: a QuickJS hook throwing + // `{ code: 'RECORD_LOCKED', status: 409 }` reached the client with the + // status and no machine-readable code, so a caller could tell "frozen + // record" from "value already taken" only by substring-matching localised + // prose — the failure mode the ADR-0112 enum exists to remove. + // + // The old rationale, recorded because it was load-bearing until it wasn't: + // "older @objectstack/client builds (still bundled in deployed consoles) + // prepend any `code` to the human-readable message". Both halves are now + // false, measured rather than assumed: + // + // - The shipping client does the OPPOSITE by explicit rule. Its error + // construction keeps `.message` to "the server's human-readable message + // — no `[ObjectStack]` branding and no `CODE:` prefix" and attaches the + // code programmatically as `error.code` (`packages/client/src/index.ts`, + // the `fetch` failure path). + // - "Older bundled client, current server" is not a supported pairing. + // #4007 retired the compat read for exactly that combination: SDK and + // server ship on one release train (a changesets fixed group), and + // ADR-0112 renamed the code VALUES anyway, so a code an old console + // could mis-render is a code it could no longer match either. + // + // And the omission was never the status policy it looked like from + // outside. It dropped `code` on a declared 400 exactly as on a declared + // 409, and KEPT it on a declared 5xx by falling through to the passthrough + // below — so one sandboxed producer got its code on 503 and lost it on + // 409. What made the card's reading look status-shaped is which codes have + // a bespoke arm ABOVE this one: `DELETE_RESTRICTED` and + // `VALIDATION_FAILED` do and never reach here, `RECORD_LOCKED` / + // `DUPLICATE_VALUE` / `FORBIDDEN` do not and did. + // + // ⛔ Nothing is invented for a producer that declared no code: + // `thrownCodeFields` answers `{}` there, which is ADR-0112's rule and the + // answer the sibling arms already give (`rest-thrown-code-vocabulary.test.ts` + // §3). Adding `code` here narrows nothing and widens nothing about the + // VOCABULARY either — an unregistered spelling is demoted to + // `declaredCode` by the same shared resolver, so this door stops being the + // one flat exit #9232 could not reach. if (typeof error?.innerMessage === 'string' && error.innerMessage) { // [#7543] …but only when the body REPORTED something. A body that // CRASHED arrives here too, and its `TypeError: not a function` is an @@ -638,10 +683,17 @@ function classifyDataError(error: any, object?: string): { status: number; body: // pins (`hook-error-format.dogfood.test.ts`) require. const declared = declaredHttpStatus(error); if (declared === undefined || declared < 500) { + // [#10345] `status` is resolved BEFORE the code fields are asked + // for, and handed to {@link thrownCodeFields} as the fallback, so + // an unregistered spelling demotes against the status the client + // actually receives rather than against a default — the #9232 §5 + // rule, applied here for the first time. + const status = declared ?? 400; return { - status: declared ?? 400, + status, body: { error: error.innerMessage, + ...thrownCodeFields(error, status), ...(object ? { object } : {}), }, }; @@ -1424,7 +1476,8 @@ function resolveErrorResponse(error: any, object?: string): { status: number; bo // [#9232] The surviving `code` is the NARROWED one — see // {@link thrownCodeFields}. This arm's old gate was bare truthiness, so // it also admitted a non-string `code`; that limb is gone with the - // narrowing, and the four flat arms now ask one question. + // narrowing, and the flat arms now ask one question (five of them since + // #10345 brought the sandbox unwrap door into the vocabulary). // [#9934] Both passthrough arms ride a producer-declared `userMessage` // onto the body, the same rule as the exported `mapDataError` wrapper — // see {@link withDeclaredUserMessage}. On the 5xx arm the PROSE is diff --git a/packages/rest/src/rest-hook-refusal-code-parity.test.ts b/packages/rest/src/rest-hook-refusal-code-parity.test.ts new file mode 100644 index 0000000000..f2f530d416 --- /dev/null +++ b/packages/rest/src/rest-hook-refusal-code-parity.test.ts @@ -0,0 +1,513 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#10345] A sandboxed hook refusal keeps its ADR-0112 `code` on the +// `/api/v1/data` write path — at 409 and 403 exactly as at 400. +// +// --------------------------------------------------------------------------- +// The branch, named — the card deliberately made no claim about it +// --------------------------------------------------------------------------- +// The owner is `classifyDataError`'s SANDBOX UNWRAP door in +// `error-response.ts` (`typeof error?.innerMessage === 'string'`), not +// `toRowApiError` (no such function exists in this package) and not a +// per-status policy anywhere. The door rendered from the RAW error — the +// unwrapped `innerMessage` — and emitted no `code` at all, while every arm +// around it renders from the RESOLVED envelope. That is the split behind the +// card's `innerMessage` correlation: the two always moved together because +// they are the same branch, not because one causes the other. +// +// Why the card's reading looked status-shaped, and why it is not: +// +// `DELETE_RESTRICTED` / `VALIDATION_FAILED` have a BESPOKE arm above the +// unwrap, so they never reach it → `code` present, and the message is +// the sandbox WRAPPER those arms read off `error.message`. +// `RECORD_LOCKED` / `DUPLICATE_VALUE` / `FORBIDDEN` have none → they fall +// into the unwrap → `code` dropped, message unwrapped. +// +// Measured on this branch before the fix, and pinned in §2 below: the unwrap +// dropped `code` on a declared **400** too, and KEPT it on a declared 5xx (by +// falling through to the passthrough). One sandboxed producer, its code +// surviving 503 and lost at 409 — a branch accident, not a narrowing of the +// response shape on non-400. +// +// The sibling that settles the intent question: the custom-action door +// (`runtime/src/domains/actions.ts`) performs the SAME unwrap on the SAME +// `SandboxError` and then exits through `errorFromThrown`, so it has always +// answered with the business message AND the code. Unwrapping and carrying the +// code were never alternatives. +// +// --------------------------------------------------------------------------- +// Assertions are `code` + `status` pairs on the BODY, never `toThrow` +// --------------------------------------------------------------------------- +// These doors return / send a body; a `toThrow`-shaped assertion could not +// separate "answered without the code" from "did not answer", and the missing +// code IS the defect (ADR-0112). +// +// --------------------------------------------------------------------------- +// Anti-vacuity — directions predicted BEFORE running, measured after +// --------------------------------------------------------------------------- +// Baseline leg: this file run with ONLY `error-response.ts` reverted to +// `origin/main` (the fix committed first; revert via `git checkout origin/main +// -- `, restore via `git checkout -- `, both under a +// `trap`). No rebuild is needed between legs and none was done: every symbol +// under test is reached by a RELATIVE import inside this package, which vitest +// transforms from source — the only `exports`-resolved workspace deps here +// (`@objectstack/spec/api`, `@objectstack/types`) are untouched by the +// mutation. +// +// §1 predicted RED 3 / GREEN 2 measured 3 red — as predicted. +// §2 predicted RED 3 / GREEN 1 measured 3 red — as predicted. +// §3 predicted GREEN throughout measured 1 RED. The prediction was WRONG, +// recorded rather than rewritten: "an undeclared-status refusal WITH a +// code still answers 400" asserts the status AND the code, and pre-fix +// only its status half held. Its neighbours in the section really are +// direction-insensitive; this one was mis-shelved, and the red is the +// correct answer for it. +// §4 predicted RED 1 / GREEN 2 measured 2 red — the "registered spelling +// arrives verbatim" case reddens too, because pre-fix no code arrives at +// all. Predicted as a control; it is not one. +// §5 predicted RED 6 / GREEN 5 measured 6 red — as predicted. +// +// Total 15 of 27 red here (prediction: 13), plus 3 of the 4 cases the fifth +// `ARMS` entry adds to `rest-thrown-code-vocabulary.test.ts` (prediction: 2 — +// §1 contributes two assertions per arm, not one). 18 red across both files. +// The predictions above are left as written, per this repo's rule that a wrong +// prediction is reported rather than fitted to the measurement. +// --------------------------------------------------------------------------- + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +// `.js` extension deliberately: this package resolves `nodenext`, so an +// extensionless relative import is a `tsc` error (TS2835). +import { mapDataError, RestServer } from './rest-server.js'; + +const DATA_COLLECTION = '/api/v1/data/:object'; +const DATA_ITEM = '/api/v1/data/:object/:id'; + +// --------------------------------------------------------------------------- +// Fixtures — the shape `runtime/src/sandbox/quickjs-runner.ts` produces: +// `.message` is the ` '' threw: ` debug wrapper, `.innerMessage` +// the business text, `.status` the #7867 side-channel. Reproduced here so +// `@objectstack/rest` does not depend on `@objectstack/runtime` to run its own +// tests. +// --------------------------------------------------------------------------- + +/** The card's own repro: `Object.assign(new Error(msg), { code, status })` from a hook body. */ +function sandboxRefusal( + businessMessage: string, + extra: Record = {}, + hook = 'account_protection', +) { + const err: any = new Error(`hook '${hook}' threw: Error: ${businessMessage}`); + err.name = 'SandboxError'; + err.innerMessage = businessMessage; + return Object.assign(err, extra); +} + +/** + * The SAME refusal thrown outside the sandbox — same `code`, same `status`, no + * `innerMessage`. The control that isolates the branch: one property is the + * whole difference between the two answers. + */ +function plainRefusal(businessMessage: string, extra: Record = {}) { + return Object.assign(new Error(businessMessage), extra); +} + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +function setup(protocolOverrides: Record = {}) { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([{ name: 'crm_account' }]), + getMetaItem: vi.fn().mockResolvedValue({}), + findData: vi.fn().mockResolvedValue([]), + createData: vi.fn().mockResolvedValue({}), + updateData: vi.fn().mockResolvedValue({}), + deleteData: vi.fn().mockResolvedValue({}), + batchData: vi.fn().mockResolvedValue({}), + createManyData: vi.fn().mockResolvedValue({}), + updateManyData: vi.fn().mockResolvedValue({}), + deleteManyData: vi.fn().mockResolvedValue({}), + ...protocolOverrides, + }; + const rest = new RestServer( + createMockServer() as any, + protocol, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return rest; +} + +function routeOf(rest: any, method: string, path: string) { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + return route; +} + +async function call(rest: any, method: string, path: string, req: Record) { + const res = makeRes(); + await routeOf(rest, method, path).handler({ method, query: {}, headers: {}, ...req }, res); + return res; +} + +let errorSpy: ReturnType; +beforeEach(() => { errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); +afterEach(() => { errorSpy.mockRestore(); }); + +// --------------------------------------------------------------------------- +// §1 The card's six measured rows, walked on the real routes in process +// +// The card measured these against a booted 17.1.0 server with real SQLite; this +// section reproduces the same six answers at the REST boundary so the contract +// is pinned where it is decided. +// --------------------------------------------------------------------------- + +describe('[#10345] the reported rows: a 409 refusal reaches the client WITH its code', () => { + it('PATCH a locked record → 409 RECORD_LOCKED, business message, not the wrapper', async () => { + const rest = setup({ + updateData: vi.fn().mockRejectedValue(sandboxRefusal( + 'Opportunity is closed (closed_won); only description, next_step, notes may be edited.', + { code: 'RECORD_LOCKED', status: 409 }, + )), + }); + + const res = await call(rest, 'PATCH', DATA_ITEM, { + params: { object: 'crm_opportunity', id: 'rec1' }, body: { amount: 10 }, + }); + + expect(res.statusCode).toBe(409); + expect(res.body.code).toBe('RECORD_LOCKED'); + // The unwrap is NOT what the fix trades away — asserted together with + // the code so "delete the unwrap door" cannot pass this file either. + expect(res.body.error).toBe( + 'Opportunity is closed (closed_won); only description, next_step, notes may be edited.', + ); + expect(JSON.stringify(res.body)).not.toMatch(/threw:|hook '/); + expect(res.body.object).toBe('crm_opportunity'); + }, 60_000); + + it('POST a duplicate contact → 409 DUPLICATE_VALUE', async () => { + const rest = setup({ + createData: vi.fn().mockRejectedValue(sandboxRefusal( + 'Another contact (Ada Lovelace) with email ada@example.com already exists.', + { code: 'DUPLICATE_VALUE', status: 409 }, + )), + }); + + const res = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'crm_contact' }, body: { email: 'ada@example.com' }, + }); + + expect(res.statusCode).toBe(409); + expect(res.body.code).toBe('DUPLICATE_VALUE'); + expect(res.body.error).toContain('already exists'); + }, 60_000); + + it('POST refused by a Do-Not-Call guard → 403 FORBIDDEN', async () => { + // The card took ONE 403 sample and said so. This pins the answer rather + // than inheriting the card's inference from a single reading. + const rest = setup({ + createData: vi.fn().mockRejectedValue(sandboxRefusal( + 'Do Not Call is set on this contact.', + { code: 'FORBIDDEN', status: 403 }, + )), + }); + + const res = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'crm_task' }, body: { type: 'call' }, + }); + + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('FORBIDDEN'); + }, 60_000); + + it('CONTROL — the 400 row keeps its `code` AND its `fields` (bespoke arm, untouched)', async () => { + // GREEN before and after. The row the card measured as working; if the + // fix had moved the bespoke arms this reddens. + const err = sandboxRefusal('Website must start with http:// or https://', { + code: 'VALIDATION_FAILED', status: 400, fields: [], + }); + const rest = setup({ createData: vi.fn().mockRejectedValue(err) }); + + const res = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'crm_account' }, body: { website: 'ftp://x' }, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('VALIDATION_FAILED'); + expect(res.body.fields).toEqual([]); + }, 60_000); + + it('LIVE CONTROL — DELETE 409 already carried its code, and still does', async () => { + // The row that proves the defect was a BRANCH and not a policy about + // conflicts: same status class, same producer, different arm. It must + // stay green through any repair of the sibling rows. + const rest = setup({ + deleteData: vi.fn().mockRejectedValue(sandboxRefusal( + 'Cannot delete customer account: 1 open opportunity still references it.', + { code: 'DELETE_RESTRICTED', status: 409 }, + )), + }); + + const res = await call(rest, 'DELETE', DATA_ITEM, { + params: { object: 'crm_account', id: 'rec1' }, + }); + + expect(res.statusCode).toBe(409); + expect(res.body.code).toBe('DELETE_RESTRICTED'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// §2 The branch, isolated — one property is the entire difference +// --------------------------------------------------------------------------- + +describe('[#10345] the sandbox unwrap is the branch, and it was never status-shaped', () => { + it('sandbox vs plain: the same code+status answered differently before the fix', () => { + const sandboxed = mapDataError( + sandboxRefusal('this record is frozen', { code: 'RECORD_LOCKED', status: 409 }), + 'crm_opportunity', + ); + const plain = mapDataError( + plainRefusal('this record is frozen', { code: 'RECORD_LOCKED', status: 409 }), + 'crm_opportunity', + ); + + // CONTROL: the plain twin always carried its code (it exits at the + // declared-status passthrough, one branch below the unwrap). + expect(plain.body.code).toBe('RECORD_LOCKED'); + // The defect, pinned as the DIFFERENCE rather than as one reading. + expect(sandboxed.body.code).toBe('RECORD_LOCKED'); + expect(sandboxed.status).toBe(plain.status); + }); + + it('a declared 400 through the unwrap carries its code too — the demotion was never about 409', () => { + // Kills the "the write path narrows its response shape on non-400" + // reading directly: pre-fix this answered `{ error, object }` with no + // code, at 400, on the same door. + const r = mapDataError( + sandboxRefusal('amount must be positive', { code: 'VALUE_OUT_OF_RANGE', status: 400 }), + 'crm_opportunity', + ); + expect(r.status).toBe(400); + expect(typeof r.body.code).toBe('string'); + }); + + it('CONTROL — the same producer at 5xx always kept its code, by falling through', () => { + // The internal contradiction the fix removes: one sandboxed producer, + // its code surviving 503 and lost at 409. Green before and after. + const r = mapDataError( + sandboxRefusal('upstream ledger unavailable', { code: 'SERVICE_UNAVAILABLE', status: 503 }), + 'crm_account', + ); + expect(r.status).toBe(503); + expect(r.body.code).toBe('SERVICE_UNAVAILABLE'); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + }); + + it('the whole 4xx band answers with the code, off either declaration spelling', () => { + for (const status of [401, 403, 404, 409, 423, 451]) { + for (const spelling of ['status', 'statusCode'] as const) { + const r = mapDataError( + sandboxRefusal('refused', { code: 'RECORD_LOCKED', [spelling]: status }), + 'crm_account', + ); + expect(r.status, `${spelling}=${status}`).toBe(status); + expect(r.body.code, `${spelling}=${status}`).toBe('RECORD_LOCKED'); + } + } + }); +}); + +// --------------------------------------------------------------------------- +// §3 What must NOT move — GREEN by construction, before and after +// --------------------------------------------------------------------------- + +describe('[#10345] the pinned defaults the fix must not disturb', () => { + it('a refusal that declares NO code still carries none — nothing is invented', () => { + // ADR-0112: the PRODUCER names the condition. Narrowing or widening the + // vocabulary must never start ADDING codes to bodies that carried none. + const r = mapDataError(sandboxRefusal('month-end close is in progress'), 'crm_account'); + expect(r.status).toBe(400); + expect(r.body.error).toBe('month-end close is in progress'); + expect(r.body).not.toHaveProperty('code'); + expect(r.body).not.toHaveProperty('declaredCode'); + }); + + it('an undeclared-status refusal WITH a code still answers 400 — the default did not move', () => { + const r = mapDataError( + sandboxRefusal('this record is frozen', { code: 'RECORD_LOCKED' }), + 'crm_account', + ); + expect(r.status).toBe(400); + expect(r.body.code).toBe('RECORD_LOCKED'); + }); + + it('a body CRASH is still the sanitised 500, even carrying a code and a status', () => { + // Crash classification outranks everything about the error, including a + // declared refusal shape. A `TypeError` must never be re-labelled as a + // caller-addressed refusal just because the fix reads more of the error. + const err = sandboxRefusal('x', { code: 'RECORD_LOCKED', status: 409 }); + err.innerMessage = 'TypeError: not a function'; + const r = mapDataError(err, 'crm_account'); + + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(String(r.body.error)).not.toContain('TypeError'); + }); + + it('the 5xx prose is still withheld and the 4xx prose still reaches the caller', () => { + expect( + mapDataError(sandboxRefusal('connect ECONNREFUSED 10.0.0.5:5432', { + code: 'SERVICE_UNAVAILABLE', status: 503, + })).body.error, + ).toBe(INTERNAL_ERROR_MESSAGE); + expect( + mapDataError(sandboxRefusal('this record is frozen', { + code: 'RECORD_LOCKED', status: 409, + })).body.error, + ).toBe('this record is frozen'); + }); +}); + +// --------------------------------------------------------------------------- +// §4 The vocabulary at this door — it stops being the one flat exit #9232 could +// not reach, so it answers the closed vocabulary like its three siblings. +// --------------------------------------------------------------------------- + +describe('[#10345 / #9232] the unwrap door speaks the closed ADR-0112 vocabulary', () => { + it('an unregistered spelling is DEMOTED, not shipped verbatim', () => { + const r = mapDataError( + sandboxRefusal('the package is haunted', { code: 'PACKAGE_IS_HAUNTED', status: 409 }), + 'crm_account', + ); + expect(r.status).toBe(409); + // The status-derived member arrives in `code`, the producer's string in + // `declaredCode` — the same answer the sibling arms give. + expect(r.body.code).toBe('RESOURCE_CONFLICT'); + expect(r.body.declaredCode).toBe('PACKAGE_IS_HAUNTED'); + }); + + it('a REGISTERED spelling arrives verbatim with no `declaredCode` beside it', () => { + const r = mapDataError( + sandboxRefusal('this record is frozen', { code: 'RECORD_LOCKED', status: 409 }), + 'crm_account', + ); + expect(r.body.code).toBe('RECORD_LOCKED'); + expect(r.body.declaredCode).toBeUndefined(); + }); + + it('CONTROL — a numeric errno and an empty string are not declarations', () => { + for (const code of [1062, '']) { + const r = mapDataError(sandboxRefusal('refused', { code, status: 409 }), 'crm_account'); + expect(r.body.code, String(code)).toBeUndefined(); + expect(r.body.declaredCode, String(code)).toBeUndefined(); + } + }); +}); + +// --------------------------------------------------------------------------- +// §5 The routes the card never exercised — MEASURED, not assumed +// +// Batch / bulk / clone exit through `handleRouteError` → `resolveErrorResponse`, +// whose passthrough is a `status`-ONLY read sitting ABOVE `mapDataError`. So +// they share the unwrap door only through the `statusCode` spelling — and that +// half lost its code exactly as the single-row routes did. +// --------------------------------------------------------------------------- + +const BULK_ROUTES: Array<{ + name: string; path: string; method: string; protocolKey: string; + req: Record; +}> = [ + { + name: 'batch', method: 'POST', path: `${DATA_COLLECTION}/batch`, protocolKey: 'batchData', + req: { params: { object: 'crm_opportunity' }, body: { operation: 'update', records: [{ id: 'r1', name: 'x' }] } }, + }, + { + name: 'createMany', method: 'POST', path: `${DATA_COLLECTION}/createMany`, protocolKey: 'createManyData', + req: { params: { object: 'crm_contact' }, body: [{ email: 'a@b.com' }] }, + }, + { + name: 'updateMany', method: 'POST', path: `${DATA_COLLECTION}/updateMany`, protocolKey: 'updateManyData', + req: { params: { object: 'crm_opportunity' }, body: { records: [{ id: 'r1', data: { name: 'x' } }] } }, + }, + { + name: 'deleteMany', method: 'POST', path: `${DATA_COLLECTION}/deleteMany`, protocolKey: 'deleteManyData', + req: { params: { object: 'crm_account' }, body: { ids: ['r1'] } }, + }, + { + name: 'clone', method: 'POST', path: `${DATA_ITEM}/clone`, protocolKey: 'cloneData', + req: { params: { object: 'crm_account', id: 'r1' }, body: {} }, + }, +]; + +describe('[#10345] batch / bulk / clone share the door through the `statusCode` spelling', () => { + for (const route of BULK_ROUTES) { + it(`${route.name}: a \`statusCode\`-declared 409 keeps its code`, async () => { + const rest = setup({ + [route.protocolKey]: vi.fn().mockRejectedValue( + sandboxRefusal('this record is frozen', { code: 'RECORD_LOCKED', statusCode: 409 }), + ), + }); + + const res = await call(rest, route.method, route.path, route.req); + + expect(res.statusCode).toBe(409); + expect(res.body.code).toBe('RECORD_LOCKED'); + }, 60_000); + + it(`CONTROL ${route.name}: a \`status\`-declared 409 already kept it (exits one door earlier)`, async () => { + const rest = setup({ + [route.protocolKey]: vi.fn().mockRejectedValue( + sandboxRefusal('this record is frozen', { code: 'RECORD_LOCKED', status: 409 }), + ), + }); + + const res = await call(rest, route.method, route.path, route.req); + + expect(res.statusCode).toBe(409); + expect(res.body.code).toBe('RECORD_LOCKED'); + }, 60_000); + } +}); + +describe('[#10345] the read-shaped data route on the same door', () => { + it('POST /data/:object/query — a refused query keeps its code too', async () => { + // `POST …/query` calls `mapDataError` directly like the single-row + // writes, so it shared the defect and shares the repair. Recorded + // because "write path" named the routes the card exercised, not the + // set the branch covers. + const rest = setup({ + findData: vi.fn().mockRejectedValue( + sandboxRefusal('this object is not readable outside business hours', { + code: 'FORBIDDEN', status: 403, + }), + ), + }); + + const res = await call(rest, 'POST', `${DATA_COLLECTION}/query`, { + params: { object: 'crm_account' }, body: { object: 'crm_account' }, + }); + + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('FORBIDDEN'); + }, 60_000); +}); diff --git a/packages/rest/src/rest-hook-script-fault-envelope.test.ts b/packages/rest/src/rest-hook-script-fault-envelope.test.ts index 7f4c70b94b..8cf1ef70aa 100644 --- a/packages/rest/src/rest-hook-script-fault-envelope.test.ts +++ b/packages/rest/src/rest-hook-script-fault-envelope.test.ts @@ -322,11 +322,14 @@ describe('[#7543] a hook that deliberately refuses still speaks in its own words expect(r.body.error).not.toBe(INTERNAL_ERROR_MESSAGE); }); - it('the refusal envelope still carries NO `code` — #7543 did not relitigate that', () => { - // Deliberate and load-bearing: older @objectstack/client builds prepend - // any `code` to the human-readable message, which would reintroduce the - // English noise the unwrap removes. The fix changes WHICH errors take - // this branch, not what the branch emits. + it('the refusal envelope carries no `code` when the hook declared none', () => { + // [#10345] Re-read, not rewritten to fit. This assertion always tested a + // producer that declares NO code, so it pins ADR-0112's "nothing is + // invented for a half-declaration" and is green on both sides of that + // card. What it never tested — and what the comment here used to claim + // — is that the door withholds a code the producer DID declare; that + // claim was the defect, and its "older clients prepend the code to the + // message" rationale is retired (see `error-response.ts`). const r = mapDataError(hookRefusal('h', 'month-end close is in progress'), 'showcase_task'); expect(r.body).not.toHaveProperty('code'); }); diff --git a/packages/rest/src/rest-sandbox-declared-status.test.ts b/packages/rest/src/rest-sandbox-declared-status.test.ts index e162c10839..4a6b7d0719 100644 --- a/packages/rest/src/rest-sandbox-declared-status.test.ts +++ b/packages/rest/src/rest-sandbox-declared-status.test.ts @@ -25,8 +25,14 @@ // - a body throw that declares NO status keeps the verbatim-message 400; // - a body that CRASHES (`isScriptFaultMessage`) stays the sanitised 500 — // even when the crash object carries a stray `status`; -// - the envelope still carries NO `code` field (old @objectstack/client -// builds prepend `code` to the human-readable message). +// - the envelope carries no `code` for a body that DECLARED none. +// ⚠️ [#10345] That third bullet used to read "the envelope still carries NO +// `code` field (old @objectstack/client builds prepend `code` to the +// human-readable message)". Every fixture below declares no code, so what +// the section actually pinned was ADR-0112's "invent nothing" half, and it +// is green on both sides of #10345. The blanket claim was the defect: a +// hook that DID declare a code lost it here. See `error-response.ts` for +// why the client-compat rationale is retired. // // Reverse verification (measured against this branch with ONLY // `error-response.ts` reverted to the pre-fix `origin/main` copy — the fix @@ -77,9 +83,10 @@ describe('[#9967] mapDataError: a sandboxed body that declares a 4xx status keep }); it('the body is byte-identical to the undeclared 400 envelope — only the status moves', () => { - // The unwrap branch's own contract (deliberately NO `code`, `object` - // rides) is unchanged by the fix; compared output-to-output so a field - // later added to BOTH envelopes (e.g. #9934's marking) keeps this green. + // The unwrap branch's own contract (`object` rides; no `code` is + // invented for a producer that declared none) is unchanged by the fix; + // compared output-to-output so a field later added to BOTH envelopes + // (#9934's marking, #10345's declared `code`) keeps this green. const declared = mapDataError(sandboxRefusal({ status: 403 }), 'showcase_task'); const undeclared = mapDataError(sandboxRefusal(), 'showcase_task'); diff --git a/packages/rest/src/rest-thrown-code-vocabulary.test.ts b/packages/rest/src/rest-thrown-code-vocabulary.test.ts index 339e91a23a..2cd0b1488b 100644 --- a/packages/rest/src/rest-thrown-code-vocabulary.test.ts +++ b/packages/rest/src/rest-thrown-code-vocabulary.test.ts @@ -91,12 +91,33 @@ const thrownWithStatus = (status: number, code?: unknown) => Object.assign(new Error('boom'), { status, ...(code !== undefined ? { code } : {}) }); const thrownWithStatusCode = (statusCode: number, code?: unknown) => Object.assign(new Error('boom'), { statusCode, ...(code !== undefined ? { code } : {}) }); +/** + * [#10345] `runtime/src/sandbox/quickjs-runner.ts`'s `SandboxError`: the debug + * wrapper on `.message`, the business text on `.innerMessage`. Reaches the + * unwrap door, which sits above the passthrough. + */ +const sandboxThrownWithStatus = (status: number, code?: unknown) => { + const err: any = Object.assign(new Error("hook 'g' threw: Error: boom"), { + status, ...(code !== undefined ? { code } : {}), + }); + err.name = 'SandboxError'; + err.innerMessage = 'boom'; + return err; +}; /** - * The four flat arms a thrown error can leave through, each reached by the + * The flat arms a thrown error can leave through, each reached by the * spelling/band combination named beside it. Enumerated rather than tested one * at a time because the defect this card measured was FOUR verbatim * passthroughs, and a fix that reached three of them would read as done. + * + * [#10345] A FIFTH arm joined the list. The sandbox unwrap door emitted no + * `code` at all, so #9232 had nothing to narrow there and left it out — and + * "no code, ever" is exactly the shape a vocabulary sweep cannot see. It now + * carries the producer's declared code like its four siblings, so it is + * enumerated here rather than pinned only in its own file: the rule this + * table states is "every flat arm", and an arm that is not in the table is an + * arm the next narrowing will miss again. */ const ARMS = [ { @@ -125,6 +146,17 @@ const ARMS = [ }, status: 503, }, + { + // [#10345] The sandbox unwrap — reached by `.innerMessage` plus a + // declared client-band status, and the ONLY arm whose body text is the + // unwrapped business message rather than `error.message`. + name: 'mapDataError sandbox unwrap 4xx (`.innerMessage`)', + answer: (code: unknown) => { + const r = mapDataError(sandboxThrownWithStatus(409, code)); + return { status: r.status, body: r.body }; + }, + status: 409, + }, ] as const; let errorSpy: ReturnType; diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 0d836ba5c9..b9bd0287c8 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2690,8 +2690,11 @@ describe('mapDataError — schema/constraint envelopes', () => { expect(r.status).toBe(400); expect(r.body.error).toBe('制作基地被「项目主计划批次」引用(3 条),删除被阻断,请先解除引用'); expect(r.body.object).toBe('pm_base'); - // No `code`: older bundled clients prepend any code to the message, - // which would reintroduce the English noise this unwrap removes. + // [#10345] No `code` because this producer DECLARED none — not because + // the unwrap door withholds it. That door carries a declared code now; the + // old "older bundled clients prepend any code to the message" rationale was + // retired with the measurement in `error-response.ts`. What stays pinned + // here is ADR-0112's half: nothing is invented for a half-declaration. expect(r.body.code).toBeUndefined(); });