From 944edd3d67169698fe14c596de01b7349f8f7510 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 04:56:38 +0000 Subject: [PATCH 1/2] fix(rest): serve a sandboxed hook refusal's declared `code` at every status, not only where a bespoke arm caught it first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classifyDataError`'s sandbox unwrap door (`typeof error?.innerMessage === 'string'`) rendered from the raw error and emitted no `code` at all, while every arm around it renders from the resolved envelope. A hook throwing `{ code: 'RECORD_LOCKED', status: 409 }` reached the client with the status and no machine-readable code; `DELETE_RESTRICTED` at the same 409 kept its code only because it has a bespoke arm above the door. Never a status policy: the door dropped `code` on a declared 400 exactly as on 409, and kept it on 5xx by falling through to the passthrough. The code now rides via `thrownCodeFields` — the one definition the three sibling arms use — so the door joins the closed ADR-0112 vocabulary and invents nothing for a producer that declared none. Fixes #10345 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .changeset/write-path-refusal-code-parity.md | 42 ++ packages/rest/src/error-response.ts | 63 ++- .../src/rest-hook-refusal-code-parity.test.ts | 503 ++++++++++++++++++ .../rest-hook-script-fault-envelope.test.ts | 13 +- .../src/rest-sandbox-declared-status.test.ts | 17 +- .../src/rest-thrown-code-vocabulary.test.ts | 34 +- packages/rest/src/rest.test.ts | 7 +- 7 files changed, 661 insertions(+), 18 deletions(-) create mode 100644 .changeset/write-path-refusal-code-parity.md create mode 100644 packages/rest/src/rest-hook-refusal-code-parity.test.ts 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..8ad6f21cfe --- /dev/null +++ b/packages/rest/src/rest-hook-refusal-code-parity.test.ts @@ -0,0 +1,503 @@ +// 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 for the three uncoded rows, GREEN for the two controls. +// §2 predicted RED for the sandbox rows, GREEN for the plain twin and 5xx. +// §3 predicted GREEN throughout — it pins what must NOT move. +// §4 predicted RED for the demote, GREEN for the two non-declarations. +// §5 predicted RED for the `statusCode` batch rows, GREEN for the rest. +// +// The measured result is recorded in the PR body rather than here, so a wrong +// prediction cannot be quietly rewritten to fit. +// --------------------------------------------------------------------------- + +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(); }); From e551f5b1661fd6f4c59437b23858c3385abc7ef4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 05:26:19 +0000 Subject: [PATCH 2/2] test(rest): record the measured ablation result beside the prediction it corrects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prediction said 13 red in this file and 2 in the vocabulary suite; the measurement was 15 and 3. Both misses are recorded rather than fitted: one §3 case asserts the code as well as the status and belongs with the defect pins, and §1 of the vocabulary suite contributes two assertions per arm, not one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../src/rest-hook-refusal-code-parity.test.ts | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/packages/rest/src/rest-hook-refusal-code-parity.test.ts b/packages/rest/src/rest-hook-refusal-code-parity.test.ts index 8ad6f21cfe..f2f530d416 100644 --- a/packages/rest/src/rest-hook-refusal-code-parity.test.ts +++ b/packages/rest/src/rest-hook-refusal-code-parity.test.ts @@ -54,14 +54,24 @@ // (`@objectstack/spec/api`, `@objectstack/types`) are untouched by the // mutation. // -// §1 predicted RED for the three uncoded rows, GREEN for the two controls. -// §2 predicted RED for the sandbox rows, GREEN for the plain twin and 5xx. -// §3 predicted GREEN throughout — it pins what must NOT move. -// §4 predicted RED for the demote, GREEN for the two non-declarations. -// §5 predicted RED for the `statusCode` batch rows, GREEN for the rest. +// §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. // -// The measured result is recorded in the PR body rather than here, so a wrong -// prediction cannot be quietly rewritten to fit. +// 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';