From 224dbaf236158c2bee9a43403a236a503b8165d9 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 03:16:32 +0000 Subject: [PATCH 1/3] fix(metadata-protocol): a batch row's httpStatus reads the declared status, not one spelling of it (#8570) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../protocol.batch-row-driver-text.test.ts | 13 +- .../protocol.batch-row-http-status.test.ts | 453 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 57 ++- .../plugins/plugin-approvals/package.json | 1 + ...-lock-batch-row-status.integration.test.ts | 173 +++++++ .../plugins/plugin-approvals/vitest.config.ts | 33 ++ ...ttp-status-real-driver.integration.test.ts | 207 ++++++++ packages/spec/src/api/contract.zod.ts | 7 + packages/types/src/thrown-http-error.ts | 39 +- pnpm-lock.yaml | 3 + 10 files changed, 979 insertions(+), 7 deletions(-) create mode 100644 packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts create mode 100644 packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts create mode 100644 packages/plugins/plugin-approvals/vitest.config.ts create mode 100644 packages/runtime/src/batch-row-http-status-real-driver.integration.test.ts diff --git a/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts b/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts index 0206524668..181a8309e3 100644 --- a/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts +++ b/packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts @@ -307,9 +307,13 @@ describe('[#8502] section 2 — the authored population survives, in all THREE d expect(res.results[0].errors[0].message).toBe('title must be ≤ 4 characters (got 15)'); expect(res.results[0].errors[0].code).toBe('VALIDATION_FAILED'); - // NO `httpStatus`: the producer declared none, and #8502 does not mint - // one — that would be an ADDITION to the wire, a separate decision. - expect(res.results[0].errors[0].httpStatus).toBeUndefined(); + // `httpStatus: 400` since #8570 — the separate decision this line used + // to defer ("the producer declared no `.status`, and minting one is an + // ADDITION to the wire") was taken there: the limb now reads the same + // resolution this one does, so the validation SHAPE declares its 400. + // The undeclared populations still gain nothing; that half is pinned in + // `protocol.batch-row-http-status.test.ts` §3. + expect(res.results[0].errors[0].httpStatus).toBe(400); }); it('a 4xx `statusCode` is quoted — THIS sink’s own population, met by neither sibling', async () => { @@ -324,6 +328,9 @@ describe('[#8502] section 2 — the authored population survives, in all THREE d "RECORD_LOCKED: record 'r1' of 'leave_request' is locked while an approval is in progress", ); expect(res.results[0].errors[0].code).toBe('RECORD_LOCKED'); + // The `statusCode` spelling reaches `httpStatus` too since #8570 — this + // row is the card's second measured one. + expect(res.results[0].errors[0].httpStatus).toBe(409); }); it('an UNDECLARED hook refusal is withheld — the measured cost of a positive list', async () => { diff --git a/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts b/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts new file mode 100644 index 0000000000..b6044eb69b --- /dev/null +++ b/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts @@ -0,0 +1,453 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8570] A batch row's `errors[].httpStatus` carries the status its producer + * DECLARED — in any of the three spellings — and nothing where none was + * declared. + * + * ## The premise, reproduced + * + * `toRowApiError` set the limb from `err.status` alone. Measured on the real + * stack (a real `ObjectQL` over a real `SqlDriver`, through all three + * bulk-write loops), two producers that reach these catches declare a genuine + * client refusal without that spelling, and shipped rows with no status at all: + * + * ```json + * { "code": "VALIDATION_FAILED", "message": "name must be ≤ 4 characters (got 15)" } + * { "code": "RECORD_LOCKED", "message": "RECORD_LOCKED: record 'ok1' of 'm8502_task' is locked while an approval is in progress" } + * ``` + * + * — while their siblings in the same response carried one (`rowRequiredIdError` + * → 400, `recordNotFoundError` → 404). A caller branching on `httpStatus` to + * tell "fix your input" from "the server broke" got an answer for some failure + * rows and silence for others, with nothing saying which. That is #7525's + * single-spelling defect, one layer below the door where it was fixed. + * + * ## The two directions this file has to separate + * + * The rule delegates to `resolveThrownHttpError` (`@objectstack/types`) — + * IMPORTED, the same resolver the `message` limb beside it uses (#8502) and the + * one `/api/v1/data` answers with. But that resolver answers for **every** + * throw, and for an undeclared one its `status` is the caller's fallback, 500. + * So there are two distinct failures to pin apart: + * + * (a) **under-broad** — the limb reads one spelling again, and the two + * populations above lose their status (§1); + * (b) **over-broad** — the limb stamps `status` instead of `declaredStatus`, + * and every undeclared driver fault and bare hook `Error` GAINS + * `httpStatus: 500` on a row that never carried one (§3). That is an + * addition to the wire for those populations, which is the scope this + * card does not have. + * + * A pin that only asserts the new rows would be green in direction (b). §3 is + * the half that is not optional. + * + * ## The doubles + * + * Same provenance rule as the sibling file: `metadata-protocol` cannot import + * `@objectstack/objectql` or `@objectstack/driver-sql` (objectql depends on + * THIS package, so the import closes a cycle), so each shape below was measured + * on its real producer and is re-checked here — §6 pins the own-property set + * and runs the PRODUCTION recogniser over each double. The populations that CAN + * be driven for real are, and both rows above are reproduced end to end in + * `packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts` + * (the real lock hook, a real engine, a real sqlite driver) and + * `packages/objectql/src/batch-row-authoring-feedback.test.ts` (the real + * validator). + */ + +import { describe, it, expect, vi } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/metadata-core'; +import { resolveThrownHttpError } from '@objectstack/types'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'leave_request', + fields: { + title: { name: 'title', type: 'text' }, + progress: { name: 'progress', type: 'number' }, + }, +}; + +/** The stable sentences the #8502 withhold produces — one per operation verb. */ +const WITHHELD = { + create: 'The create of this record failed. The reason is in the server log.', + update: 'The update of this record failed. The reason is in the server log.', + upsert: 'The upsert of this record failed. The reason is in the server log.', + delete: 'The delete of this record failed. The reason is in the server log.', +}; + +// ─── The measured populations ──────────────────────────────────────────────── + +/** + * MEASURED — a real `SqliteError` from better-sqlite3 through the real + * `SqlDriver`: own properties `[stack, message, code]`, `code: 'SQLITE_ERROR'`, + * and no status in any spelling. + */ +function driverFault(message = 'SQLITE_ERROR: no such table: leave_request'): Error { + return Object.assign(new Error(message), { code: 'SQLITE_ERROR' }); +} + +/** + * MEASURED — `@objectstack/objectql`'s `ValidationError` as it reaches these + * catches: own properties `[stack, message, code, name, fields]`, + * `code: 'VALIDATION_FAILED'`, `name: 'ValidationError'`, and deliberately NO + * `status` (per `@objectstack/types`' `validation-failure.ts`, deciding it + * means 400 is "the job of whichever boundary serves it"). Assignment ORDER + * matches the real class, whose `readonly code` initialiser runs before the + * constructor body sets `name` then `fields`. + */ +function engineValidationError(message: string, fields: unknown[]): Error { + const err = new Error(message) as Error & { code: string; fields: unknown[] }; + err.code = 'VALIDATION_FAILED'; + err.name = 'ValidationError'; + err.fields = fields; + return err; +} + +/** + * MEASURED — `plugin-approvals`' `lockedError`, raised inside the GLOBAL + * `beforeUpdate` hook it binds: own properties `[stack, message, code, + * statusCode]`, `code: 'RECORD_LOCKED'`, `statusCode: 409`, `status` + * undefined. The spelling is this card's whole point. + */ +function approvalsRecordLock(recordId: string): Error { + const err = new Error( + `RECORD_LOCKED: record '${recordId}' of 'leave_request' is locked while an approval is in progress`, + ) as Error & { code: string; statusCode: number }; + err.code = 'RECORD_LOCKED'; + err.statusCode = 409; + return err; +} + +/** An app-authored hook that refuses without declaring anything. */ +function undeclaredHookRefusal(): Error { + return new Error('Approval is required before this task may be updated. Ask your manager first.'); +} + +/** + * A refusal that declares a 5xx in the `statusCode` spelling — the edge the + * rule's gate is deliberately NOT the 4xx band for. See §4. + */ +function declaredServiceUnavailable(): Error { + const err = new Error('the approvals service is down, so this write cannot be judged') as Error + & { code: string; statusCode: number }; + err.code = 'SERVICE_UNAVAILABLE'; + err.statusCode = 503; + return err; +} + +/** A refusal spelled `statusCode` whose own code the ledger does not know. */ +function unregisteredCodeConflict(): Error { + const err = new Error('locked by something this ledger has never heard of') as Error + & { code: string; statusCode: number }; + err.code = 'PACKAGE_IS_HAUNTED'; + err.statusCode = 409; + return err; +} + +// ─── Harness ───────────────────────────────────────────────────────────────── + +/** + * The engine double every case drives — the same one the sibling file uses, so + * the rows below are produced by the ACTUAL loops, builders and rollback + * classifier rather than by an assertion's idea of them. + */ +function makeEngine(throwOn: (verb: string, id: unknown) => unknown | undefined) { + const rows = new Map([ + ['r1', { id: 'r1', title: 'one', progress: 0 }], + ['r2', { id: 'r2', title: 'two', progress: 0 }], + ['r3', { id: 'r3', title: 'three', progress: 0 }], + ]); + const handle = { id: 'trx-1' }; + + const engine: any = { + registry: { getObject: (n: string) => (n === 'leave_request' ? SCHEMA : undefined) }, + findOne: vi.fn(async (_o: string, opts?: any) => rows.get(opts?.where?.id) ?? null), + insert: vi.fn(async (_o: string, data: any) => { + const boom = throwOn('insert', data?.id); + if (boom) throw boom; + const rec = { id: data.id ?? `new-${rows.size + 1}`, ...data }; + rows.set(rec.id, rec); + return rec; + }), + update: vi.fn(async (_o: string, data: any, opts?: any) => { + const id = opts?.where?.id; + const boom = throwOn('update', id); + if (boom) throw boom; + const next = { ...rows.get(id), ...data }; + rows.set(id, next); + return next; + }), + delete: vi.fn(async (_o: string, opts?: any) => { + assertEngineDeleteDispatch(opts); + const id = opts?.where?.id; + const boom = throwOn('delete', id); + if (boom) throw boom; + if (!rows.has(id)) return false; + rows.delete(id); + return { deleted: 1 }; + }), + getDefaultDriverName: () => 'default', + getDriverByName: () => ({ beginTransaction: async () => handle }), + transaction: vi.fn(async (cb: (ctx: any) => Promise, base?: any) => { + const snapshot = new Map(rows); + try { + return await cb({ ...(base ?? {}), transaction: handle }); + } catch (err) { + rows.clear(); + for (const [k, v] of snapshot) rows.set(k, v); + throw err; + } + }), + }; + return { engine, protocol: new ObjectStackProtocolImplementation(engine) as any, rows }; +} + +/** Silence — and capture — the operator-half warn the #8502 withhold writes. */ +function captureWarn() { + return vi.spyOn(console, 'warn').mockImplementation(() => {}); +} + +/** + * Membership, never a count: `httpStatus` is optional, so `undefined` and + * "the key is not there" are the same wire fact and both must be checked + * against the row itself rather than against the length of anything. + */ +function carriesStatus(error: Record): boolean { + return Object.prototype.hasOwnProperty.call(error, 'httpStatus'); +} + +describe('[#8570] section 1 — a declared refusal that does not spell `.status` now carries one', () => { + it('the objectql VALIDATION_FAILED shape: the card\'s first row, now with 400', async () => { + const boom = engineValidationError( + 'name must be ≤ 4 characters (got 15)', + [{ field: 'name', code: 'max_length', message: 'name must be ≤ 4 characters (got 15)' }], + ); + const { protocol } = makeEngine((verb) => (verb === 'update' ? boom : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { title: 'far too long a title' } }], + }); + + expect(res.results[0].errors[0]).toEqual({ + code: 'VALIDATION_FAILED', + message: 'name must be ≤ 4 characters (got 15)', + httpStatus: 400, + }); + }); + + it('the plugin-approvals record lock: the card\'s second row, now with 409', async () => { + const { protocol } = makeEngine((verb) => (verb === 'update' ? approvalsRecordLock('r1') : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0]).toEqual({ + code: 'RECORD_LOCKED', + message: "RECORD_LOCKED: record 'r1' of 'leave_request' is locked while an approval is in progress", + httpStatus: 409, + }); + }); + + it('all THREE loops populate it, not just the one the card sampled', async () => { + // create (bulk `batchData`), upsert (`batchData`) and delete + // (`deleteManyData`) each build their row through the same helper, and + // a fix applied at one call site only would leave the other two silent. + const a = makeEngine((verb) => (verb === 'insert' ? approvalsRecordLock('new') : undefined)); + const createRes: any = await a.protocol.batchData({ + object: 'leave_request', + request: { operation: 'create', records: [{ data: { title: 'x' } }] }, + }); + expect(createRes.results[0].errors[0].httpStatus).toBe(409); + + const b = makeEngine((verb) => (verb === 'update' ? engineValidationError('too long', []) : undefined)); + const upsertRes: any = await b.protocol.batchData({ + object: 'leave_request', + request: { operation: 'upsert', records: [{ id: 'r1', data: { progress: 1 } }] }, + }); + expect(upsertRes.results[0].errors[0].httpStatus).toBe(400); + + const c = makeEngine((verb) => (verb === 'delete' ? approvalsRecordLock('r1') : undefined)); + const deleteRes: any = await c.protocol.deleteManyData({ object: 'leave_request', ids: ['r1'] }); + expect(deleteRes.results[0].errors[0].httpStatus).toBe(409); + }); +}); + +describe('[#8570] section 2 — the rows that already carried a status are untouched', () => { + it('rowRequiredIdError still answers 400 and recordNotFoundError still answers 404', async () => { + const { protocol } = makeEngine(() => undefined); + + const update: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ data: { progress: 1 } }], + }); + expect(update.results[0].errors[0]).toEqual({ + code: 'VALIDATION_FAILED', message: 'Record id is required for update', httpStatus: 400, + }); + + const del: any = await protocol.deleteManyData({ object: 'leave_request', ids: ['ghost'] }); + expect(del.results[0].errors[0]).toEqual({ + code: 'RECORD_NOT_FOUND', message: 'Record ghost not found in leave_request', httpStatus: 404, + }); + }); +}); + +describe('[#8570] section 3 — the OVER-BROAD direction: an undeclared throw gains nothing', () => { + it('a driver fault carries no `httpStatus` — not 500, not the key at all', async () => { + const warn = captureWarn(); + const { protocol } = makeEngine((verb) => (verb === 'delete' ? driverFault() : undefined)); + + const res: any = await protocol.deleteManyData({ object: 'leave_request', ids: ['r1'] }); + + const error = res.results[0].errors[0]; + // The whole row, so a stamped `httpStatus: 500` cannot hide beside a + // green `code`/`message` assertion. + expect(error).toEqual({ code: 'INTERNAL_ERROR', message: WITHHELD.delete }); + expect(carriesStatus(error)).toBe(false); + // …and it is nowhere in the payload either: `500` must not appear as a + // status the caller can read off any row of this response. + expect(JSON.stringify(res)).not.toContain('httpStatus'); + warn.mockRestore(); + }); + + it('an undeclared app-hook refusal gains nothing either', async () => { + const warn = captureWarn(); + const { protocol } = makeEngine((verb) => (verb === 'update' ? undeclaredHookRefusal() : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + const error = res.results[0].errors[0]; + expect(error).toEqual({ code: 'INTERNAL_ERROR', message: WITHHELD.update }); + expect(carriesStatus(error)).toBe(false); + warn.mockRestore(); + }); + + it('the collateral NOT_ATTEMPTED / ROLLED_BACK rows carry no status either', async () => { + // They are built by `reconcileStoppedBatch` / `buildRolledBackBatchResponse`, + // which quote the causal row's MESSAGE and mint their own code — so a + // populated causal row must not turn into a populated batch. + const warn = captureWarn(); + + const stopped = makeEngine((verb, id) => (verb === 'delete' && id === 'r1' ? approvalsRecordLock('r1') : undefined)); + const stoppedRes: any = await stopped.protocol.deleteManyData({ + object: 'leave_request', ids: ['r1', 'r2', 'r3'], + }); + expect(stoppedRes.results[0].errors[0].httpStatus).toBe(409); + expect(stoppedRes.results[1].errors[0].code).toBe('NOT_ATTEMPTED'); + expect(carriesStatus(stoppedRes.results[1].errors[0])).toBe(false); + expect(carriesStatus(stoppedRes.results[2].errors[0])).toBe(false); + + const atomic = makeEngine((verb, id) => (verb === 'update' && id === 'r2' ? approvalsRecordLock('r2') : undefined)); + const atomicRes: any = await atomic.protocol.updateManyData({ + object: 'leave_request', + records: [{ id: 'r1', data: { progress: 1 } }, { id: 'r2', data: { progress: 2 } }], + options: { atomic: true }, + }); + expect(atomicRes.results[0].errors[0].code).toBe('ROLLED_BACK'); + expect(carriesStatus(atomicRes.results[0].errors[0])).toBe(false); + expect(atomicRes.results[1].errors[0].httpStatus).toBe(409); + warn.mockRestore(); + }); +}); + +describe('[#8570] section 4 — the gate is DECLARED-ness, deliberately not the 4xx band', () => { + it('a refusal declaring a 5xx keeps its status on the row, with its text still withheld', async () => { + // Two different questions, two different gates, in one row. The message + // limb (#8502) asks "may this free text be disclosed?" — no for a 5xx. + // This limb reports a number the PRODUCER authored, and a row already + // ships `httpStatus: 503` today when the same refusal spells `.status`; + // gating on 4xx here would WITHDRAW that, which is a different card. + const warn = captureWarn(); + const { protocol } = makeEngine((verb) => (verb === 'update' ? declaredServiceUnavailable() : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0]).toEqual({ + code: 'SERVICE_UNAVAILABLE', message: WITHHELD.update, httpStatus: 503, + }); + expect(JSON.stringify(res)).not.toContain('approvals service is down'); + warn.mockRestore(); + }); + + it('the `.status` spelling of the same 5xx behaved this way BEFORE the fix, and still does', async () => { + // The anti-regression half of the case above: this row is what the + // limb already shipped, so the fix is measured as an addition to the + // undeclared-spelling populations and not a change to this one. + const warn = captureWarn(); + const boom = Object.assign(new Error('down'), { code: 'SERVICE_UNAVAILABLE', status: 503 }); + const { protocol } = makeEngine((verb) => (verb === 'update' ? boom : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0].httpStatus).toBe(503); + warn.mockRestore(); + }); +}); + +describe('[#8570] section 5 — `code` and `httpStatus` are read off ONE resolution', () => { + it('an unregistered code spelled `statusCode` yields a COHERENT row, not 409 beside INTERNAL_ERROR', async () => { + // The reason both limbs share the resolution. Deriving `code` from + // `err.status` while `httpStatus` came from the resolver would answer + // `{ code: 'INTERNAL_ERROR', httpStatus: 409 }` here — a row that + // contradicts itself, since `INTERNAL_ERROR` is the 5xx bucket. + const warn = captureWarn(); + const { protocol } = makeEngine((verb) => (verb === 'update' ? unregisteredCodeConflict() : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0]).toEqual({ + // `PACKAGE_IS_HAUNTED` is not in `StandardErrorCode ∪ ERROR_CODE_LEDGER`, + // so #8441's catalog gate replaces it with the status-derived code — + // now derived from the status the producer really declared. + code: 'RESOURCE_CONFLICT', + // Quoted, not withheld: the message limb (#8502) asks the 4xx + // question of the same resolution, and a declared 409 passes it + // whether or not the ledger knows the producer's own code. + message: 'locked by something this ledger has never heard of', + httpStatus: 409, + }); + warn.mockRestore(); + }); +}); + +describe('[#8570] section 6 — anti-vacuity: the doubles are the shapes they claim to be', () => { + it('each double carries EXACTLY the own properties measured on its real producer', () => { + expect(Object.getOwnPropertyNames(driverFault())).toEqual(['stack', 'message', 'code']); + expect(Object.getOwnPropertyNames(engineValidationError('m', []))).toEqual( + ['stack', 'message', 'code', 'name', 'fields'], + ); + expect(Object.getOwnPropertyNames(approvalsRecordLock('r1'))).toEqual( + ['stack', 'message', 'code', 'statusCode'], + ); + expect(Object.getOwnPropertyNames(undeclaredHookRefusal())).toEqual(['stack', 'message']); + }); + + it('NONE of the newly-populated population declares `.status` — the spelling the limb used to read', () => { + // If any of these grew a `.status`, this file would be green against + // the OLD implementation too, which is the way an ablation lies. + for (const e of [engineValidationError('m', []), approvalsRecordLock('r1'), declaredServiceUnavailable()]) { + expect((e as any).status).toBeUndefined(); + } + }); + + it('the PRODUCTION recogniser separates declared from undeclared exactly as the rows do', () => { + // Not this file's own predicate — the very function `toRowApiError` + // calls. `status` collapses the two populations onto 500; only + // `declaredStatus` tells them apart, which is why the limb reads it. + expect(resolveThrownHttpError(driverFault()).status).toBe(500); + expect(resolveThrownHttpError(driverFault()).declaredStatus).toBeUndefined(); + expect(resolveThrownHttpError(undeclaredHookRefusal()).declaredStatus).toBeUndefined(); + expect(resolveThrownHttpError(engineValidationError('m', [])).declaredStatus).toBe(400); + expect(resolveThrownHttpError(approvalsRecordLock('r1')).declaredStatus).toBe(409); + expect(resolveThrownHttpError(declaredServiceUnavailable()).declaredStatus).toBe(503); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e625005a99..0af87fdbb3 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1489,16 +1489,67 @@ type BatchDataRowResult = BatchOperationResult; * ⛔ `fallback` is REQUIRED, not defaulted. All three catches that build a row * know their operation, and a default would let a fourth one be added that * silently reports the wrong verb. + * + * ## `httpStatus` reads the DECLARATION, not one spelling of it (#8570) + * + * This limb read `err.status` and nothing else, so of the producers measured at + * these catches — a real `ObjectQL` over a real `SqlDriver`, driven through all + * three bulk-write loops — two well-defined client refusals shipped a row with + * no status at all: + * + * | producer | code | `.status` | `.statusCode` | validation shape | was | is | + * |---|---|---|---|---|---|---| + * | {@link rowRequiredIdError} | VALIDATION_FAILED | **400** | — | no | 400 | 400 | + * | `recordNotFoundError` (`@objectstack/core`) | RECORD_NOT_FOUND | **404** | — | no | 404 | 404 | + * | objectql `ValidationError` | VALIDATION_FAILED | — | — | **yes** | — | **400** | + * | plugin-approvals' record lock | RECORD_LOCKED | — | **409** | no | — | **409** | + * | an app hook throwing a bare `Error` | — | — | — | no | — | — | + * | driver fault (`SqliteError`, …) | SQLITE_* | — | — | no | — | — | + * + * The first two are siblings of the last four *in the same response*: a caller + * branching on `httpStatus` to tell "fix your input" from "the server broke" + * got an answer for some failure rows and nothing for others, with no signal + * saying which. The single-spelling defect is #7525's, fixed there at the HTTP + * door; this is the same defect on the row. + * + * The question is answered by {@link resolveThrownHttpError}, IMPORTED — the + * `message` limb beside it already delegates there (#8502), and a second local + * chain would be the third derivation of "what status is this throw" in one + * function. ⛔ Do not re-spell it as `status ?? statusCode ?? validation`. + * + * ## ⛔ `declaredStatus`, never `status` — the over-broad direction is real + * + * That resolver answers for EVERY throw: its `status` is 500 for a bare hook + * `Error` and for a `SqliteError`, because 500 is the caller's fallback. + * Stamping that would put `httpStatus: 500` on the last two rows of the table, + * which never carried one — an ADDITION to the wire for those populations, and + * a claim the producer never made. `declaredStatus` is the same resolution + * minus the fallback: present exactly when the throw declared a status in one + * of the three spellings, absent otherwise. So a declared refusal gains the + * status it always meant, and an undeclared fault keeps carrying none. + * + * The gate is DECLARED-ness and deliberately not the 4xx band that + * {@link clientFacingRowFailureText} uses. That limb decides disclosure of free + * text, where a 5xx must be withheld; this one decides a number the producer + * itself authored, and a row already ships `httpStatus: 503` today when the + * refusal spells `.status` — narrowing to 4xx would WITHDRAW a status the wire + * carries, which is a different decision from this one. + * + * Both limbs read the same resolution for a second reason: they must agree. + * Deriving `code` from `err.status` while `httpStatus` came from + * `declaredStatus` would mint incoherent rows — `{ code: 'INTERNAL_ERROR', + * httpStatus: 409 }` for a `statusCode`-spelled refusal whose own code the + * ledger does not know. */ function toRowApiError(err: any, fallback: string): ApiError { const thrown = typeof err?.code === 'string' && ErrorCode.safeParse(err.code).success ? (err.code as ApiError['code']) : undefined; - const status = typeof err?.status === 'number' ? err.status : undefined; + const { declaredStatus } = resolveThrownHttpError(err); return { - code: thrown ?? (status !== undefined ? standardErrorCodeForHttpStatus(status) : 'INTERNAL_ERROR'), + code: thrown ?? (declaredStatus !== undefined ? standardErrorCodeForHttpStatus(declaredStatus) : 'INTERNAL_ERROR'), message: clientFacingRowFailureText(err, fallback), - ...(status !== undefined ? { httpStatus: status } : {}), + ...(declaredStatus !== undefined ? { httpStatus: declaredStatus } : {}), }; } diff --git a/packages/plugins/plugin-approvals/package.json b/packages/plugins/plugin-approvals/package.json index 96c355ef25..cab0d003dd 100644 --- a/packages/plugins/plugin-approvals/package.json +++ b/packages/plugins/plugin-approvals/package.json @@ -27,6 +27,7 @@ }, "devDependencies": { "@objectstack/driver-sql": "workspace:*", + "@objectstack/metadata-protocol": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/service-automation": "workspace:*", "@objectstack/trigger-record-change": "workspace:*", diff --git a/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts b/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts new file mode 100644 index 0000000000..b45a0a5068 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8570] The record lock's refusal, as a BATCH ROW — driven through the REAL + * hook, a real {@link ObjectQL} and a real sqlite driver. + * + * ## The row this file exists for + * + * Measured on the real stack while #8502 was being verified, a bulk update of a + * record this plugin holds locked answered: + * + * ```json + * { "code": "RECORD_LOCKED", "message": "RECORD_LOCKED: record 'ok1' of 'm8502_task' is locked while an approval is in progress" } + * ``` + * + * — a deliberate **409** shipping with no `httpStatus` at all, while sibling + * rows of the same response carried one. `toRowApiError` read `err.status`, and + * {@link lockedError} spells its refusal `statusCode`, which is the same + * single-spelling defect that made `/api/v1/data` answer 500 to this very + * refusal until #7525. + * + * ## Why the pin lives HERE + * + * `metadata-protocol` cannot import this plugin, and its own pins therefore + * stand in for this producer with a double whose shape was measured. This file + * is the half that needs no double: the error is raised by the actual + * `beforeUpdate` hook `bindApprovalLockHook` binds, against an actual pending + * `sys_approval_request` row, and the response row is built by the actual + * `updateManyData` loop. If the hook ever re-spells its refusal — `.status`, + * or a plain `Error` — this file goes red where a double would happily keep + * asserting the old shape. + * + * The rig is `record-lock-multi-update.integration.test.ts`'s, for the same + * reason it gives: the store is better-sqlite3 through `@objectstack/driver-sql`, + * so the predicates are compiled and executed by the SQL builder rather than by + * fixture code written by the same author as the assertion. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { resolveThrownHttpError } from '@objectstack/types'; +import { bindApprovalLockHook } from './lifecycle-hooks.js'; + +const opportunity = { + name: 'opportunity', + label: 'Opportunity', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + approval_status: { name: 'approval_status', type: 'text' as const }, + }, +}; + +/** The lock hook reads pending requests off this object. */ +const approvalRequest = { + name: 'sys_approval_request', + label: 'Approval Request', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + object_name: { name: 'object_name', type: 'text' as const }, + record_id: { name: 'record_id', type: 'text' as const }, + status: { name: 'status', type: 'text' as const }, + flow_run_id: { name: 'flow_run_id', type: 'text' as const }, + node_config_json: { name: 'node_config_json', type: 'text' as const }, + }, +}; + +describe('[#8570] a locked record\'s batch row carries the 409 the hook declared', () => { + let engine: ObjectQL; + let protocol: any; + /** Held by a pending approval. */ + let lockedId: string; + /** Same object, no approval — the row that must still succeed. */ + let freeId: string; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + for (const o of [opportunity, approvalRequest]) engine.registry.registerObject(o as any); + // Real DDL through the real path. + await engine.syncSchemas(); + + lockedId = String((await engine.insert('opportunity', { name: 'Deal', amount: 100 })).id); + freeId = String((await engine.insert('opportunity', { name: 'Other', amount: 100 })).id); + await engine.insert('sys_approval_request', { + object_name: 'opportunity', + record_id: lockedId, + status: 'pending', + flow_run_id: 'run_1', + node_config_json: JSON.stringify({ lockRecord: true, approvalStatusField: 'approval_status' }), + }, { context: { isSystem: true } } as any); + + bindApprovalLockHook(engine as any); + protocol = new ObjectStackProtocolImplementation(engine as any); + }); + + it('the card\'s second row, verbatim, now carrying 409', async () => { + const res: any = await protocol.updateManyData({ + object: 'opportunity', + records: [{ id: lockedId, data: { amount: 999 } }], + }); + + expect(res.results[0].success).toBe(false); + expect(res.results[0].errors[0]).toEqual({ + code: 'RECORD_LOCKED', + message: `RECORD_LOCKED: record '${lockedId}' of 'opportunity' is locked while an approval is in progress`, + httpStatus: 409, + }); + + // The refusal was a refusal: nothing reached the store. + expect((await engine.findOne('opportunity', { where: { id: lockedId } }))?.amount).toBe(100); + }); + + it('the hook really declares its 409 in the `statusCode` spelling ONLY', async () => { + // Non-vacuity for the row above, taken off the REAL producer rather than + // asserted about it: if `lockedError` ever grew a `.status`, the row would + // be green against the pre-#8570 limb too, and this file would stop + // measuring anything. + let thrown: any = null; + try { + await engine.update('opportunity', { amount: 999 }, { where: { id: lockedId } } as any); + } catch (e) { thrown = e; } + + expect(thrown).not.toBeNull(); + expect(thrown.code).toBe('RECORD_LOCKED'); + expect(thrown.statusCode).toBe(409); + expect(thrown.status).toBeUndefined(); + expect(Object.getOwnPropertyNames(thrown)).toEqual(['stack', 'message', 'code', 'statusCode']); + // The production recogniser, on the real throw — and the field the row's + // limb reads, which is what separates a declared refusal from a fault. + expect(resolveThrownHttpError(thrown).declaredStatus).toBe(409); + }); + + it('an unlocked row in the SAME batch still succeeds and carries no error', async () => { + // The asymmetry the card is about is per-row, so the mixed response is the + // shape a caller actually has to reconcile. + const res: any = await protocol.updateManyData({ + object: 'opportunity', + records: [ + { id: freeId, data: { amount: 555 } }, + { id: lockedId, data: { amount: 999 } }, + ], + options: { continueOnError: true }, + }); + + expect(res.results[0].success).toBe(true); + expect(res.results[0].errors).toBeUndefined(); + expect(res.results[1].errors[0].httpStatus).toBe(409); + expect((await engine.findOne('opportunity', { where: { id: freeId } }))?.amount).toBe(555); + }); + + it('the batchData upsert loop answers the same way — not just updateManyData', async () => { + const res: any = await protocol.batchData({ + object: 'opportunity', + request: { operation: 'upsert', records: [{ id: lockedId, data: { amount: 999 } }] }, + }); + + expect(res.results[0].errors[0].code).toBe('RECORD_LOCKED'); + expect(res.results[0].errors[0].httpStatus).toBe(409); + }); +}); diff --git a/packages/plugins/plugin-approvals/vitest.config.ts b/packages/plugins/plugin-approvals/vitest.config.ts new file mode 100644 index 0000000000..05eaa8de87 --- /dev/null +++ b/packages/plugins/plugin-approvals/vitest.config.ts @@ -0,0 +1,33 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineConfig } from 'vitest/config'; +import path from 'node:path'; + +export default defineConfig({ + resolve: { + // [#8570] `record-lock-batch-row-status.integration.test.ts` drives the + // REAL lock hook through the REAL bulk-write loops, so it imports + // `@objectstack/metadata-protocol` as a value. That specifier resolves + // through `exports` to `dist/` — a build artifact — which would make the + // pin a verdict about build state rather than about the source in the + // checkout (`pnpm check:test-source-alias`, #7668/#7778). Aliased to + // source, which is that gate's prescribed fix; registering the package as + // an unaliased importer is explicitly NOT (the registry is shrink-only). + // + // ANCHORED regex, array form: a bare string `find` matches by PREFIX, so + // with a FILE replacement it would also swallow any subpath and resolve it + // to `…/metadata-protocol/src/index.ts/` — `ENOTDIR`, at run + // time, from a config that reads as correct. + alias: [ + { + find: /^@objectstack\/metadata-protocol$/, + replacement: path.resolve(__dirname, '../../metadata-protocol/src/index.ts'), + }, + ], + }, + test: { + globals: true, + environment: 'node', + include: ['src/**/*.test.ts'], + }, +}); diff --git a/packages/runtime/src/batch-row-http-status-real-driver.integration.test.ts b/packages/runtime/src/batch-row-http-status-real-driver.integration.test.ts new file mode 100644 index 0000000000..e1cb603f8c --- /dev/null +++ b/packages/runtime/src/batch-row-http-status-real-driver.integration.test.ts @@ -0,0 +1,207 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8570] A batch row's `httpStatus`, measured on the REAL stack — a real + * `ObjectQL` over a real `SqlDriver` on better-sqlite3, through the real + * `ObjectStackProtocolImplementation`'s bulk-write loops. + * + * ## What this file is for + * + * The card's first row was measured here, not argued: + * + * ```json + * { "code": "VALIDATION_FAILED", "message": "name must be ≤ 4 characters (got 15)" } + * ``` + * + * — a well-defined 400 shipping with no status at all, beside siblings in the + * SAME response that carried one, because the limb read `err.status` and + * objectql's `ValidationError` deliberately declares none (deciding it means + * 400 is "the job of whichever boundary serves it", per `@objectstack/types`' + * `validation-failure.ts`). Nothing below builds that error: a real record is + * malformed against a real schema and the engine's own validator rejects it. + * + * ## Both directions, on the real stack + * + * The fix delegates to `resolveThrownHttpError`, which answers for EVERY + * throw — including the ones that are not refusals at all. So the driver-fault + * case is here too, and it is the half that fails if the limb ever stamps the + * resolver's `status` (500, the fallback) instead of its `declaredStatus`: + * a `SqliteError` must keep carrying NO status, exactly as before this card. + * A file that only asserted the newly-populated row would be green either way. + * + * The card's second row — `plugin-approvals`' `RECORD_LOCKED`, spelled + * `statusCode: 409` — is driven through the REAL lock hook in + * `packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts`, + * which is where both halves of that producer can be imported. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { resolveThrownHttpError, validationFailureDetails } from '@objectstack/types'; + +/** `maxLength` is what the real record validator rejects against. */ +const TASK = { + name: 'hs_task', + fields: { + name: { type: 'text', maxLength: 4 }, + progress: { type: 'number' }, + }, +}; +const PARENT = { name: 'hs_parent', fields: { name: { type: 'text' } } }; +const CHILD = { + name: 'hs_child', + fields: { + name: { type: 'text' }, + parent: { type: 'lookup', reference_to: 'hs_parent' }, + }, +}; + +describe('[#8570] a batch row carries the status its producer DECLARED — real driver', () => { + let dir: string | null = null; + let engine: ObjectQL | null = null; + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + engine = null; + if (dir) { rmSync(dir, { recursive: true, force: true }); dir = null; } + }); + + async function rig() { + dir = mkdtempSync(join(tmpdir(), 'os-8570-real-')); + const real = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + await real.initObjects([TASK, PARENT, CHILD]); + + // Capture the RAW error at the seam and let it propagate untouched, so + // the assertions below are about what the producer really threw rather + // than about an assumption about it. + let raw: any = null; + const driver: any = Object.create(real); + for (const m of ['create', 'update', 'delete', 'bulkCreate'] as const) { + driver[m] = async (...args: any[]) => { + try { return await (real as any)[m](...args); } catch (e) { raw ??= e; throw e; } + }; + } + + engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [TASK, PARENT, CHILD]) { + engine.registry.registerObject(o as any, 'com.objectstack.test.8570'); + } + const protocol: any = new ObjectStackProtocolImplementation(engine as any); + return { protocol, rawOf: () => raw }; + } + + it('the REAL validator: the card\'s first row, verbatim, now carrying 400', async () => { + const { protocol } = await rig(); + await engine!.insert('hs_task', { id: 'ok1', name: 'ok', progress: 0 }); + + // 15 characters against `maxLength: 4` — the card's own row. + const TOO_LONG = 'fifteen chars!!'; + expect(TOO_LONG).toHaveLength(15); + + const res: any = await protocol.updateManyData({ + object: 'hs_task', + records: [{ id: 'ok1', data: { name: TOO_LONG } }], + }); + + const error = res.results[0].errors[0]; + expect(res.results[0].success).toBe(false); + // The card's measured row, plus the limb it was missing. + expect(error).toEqual({ + code: 'VALIDATION_FAILED', + message: 'name must be ≤ 4 characters (got 15)', + httpStatus: 400, + }); + + // Non-vacuity on the PRODUCER: this row gains a status only because + // the fix reads the declaration the shape carries — the thrown error + // really does spell no status in either channel, so a limb reading + // `err.status` (or `err.statusCode`) still answers nothing for it. + let thrown: any = null; + try { + await engine!.update('hs_task', { name: TOO_LONG }, { where: { id: 'ok1' } } as any); + } catch (e) { thrown = e; } + expect(thrown).not.toBeNull(); + expect(thrown.name).toBe('ValidationError'); + expect(thrown.code).toBe('VALIDATION_FAILED'); + expect(thrown.status).toBeUndefined(); + expect(thrown.statusCode).toBeUndefined(); + expect(validationFailureDetails(thrown)).toBeDefined(); + expect(resolveThrownHttpError(thrown).declaredStatus).toBe(400); + + // …and the record is unchanged, so the refusal was a refusal. + expect((await engine!.findOne('hs_task', { where: { id: 'ok1' } }))?.name).toBe('ok'); + }); + + it('the same row in a MIXED batch — the asymmetry the card measured is gone', async () => { + // The complaint was not "no row has a status", it was "some rows in one + // response do and others do not, with nothing saying which". So the two + // populations are driven into ONE response here. + const { protocol } = await rig(); + await engine!.insert('hs_task', { id: 'ok1', name: 'ok', progress: 0 }); + + const res: any = await protocol.updateManyData({ + object: 'hs_task', + records: [ + { data: { progress: 1 } }, // no id → rowRequiredIdError (400) + { id: 'ok1', data: { name: 'fifteen chars!!' } }, // real ValidationError + ], + options: { continueOnError: true }, + }); + + expect(res.results[0].errors[0]).toEqual({ + code: 'VALIDATION_FAILED', message: 'Record id is required for update', httpStatus: 400, + }); + expect(res.results[1].errors[0].code).toBe('VALIDATION_FAILED'); + expect(res.results[1].errors[0].httpStatus).toBe(400); + }); + + it('a REAL driver fault still carries NO status — the over-broad direction', async () => { + // `resolveThrownHttpError(raw).status` is 500 here, and 500 is what an + // unconditional stamp would put on this row. It never carried one and + // must not start: that is an addition to the wire for a population that + // declared nothing, which is not what this card does. + const { protocol, rawOf } = await rig(); + await engine!.insert('hs_parent', { id: 'p1', name: 'kept' }); + await engine!.insert('hs_child', { id: 'c1', name: 'dependent', parent: 'p1' }); + + const res: any = await protocol.deleteManyData({ object: 'hs_parent', ids: ['p1'] }); + + const raw = rawOf(); + expect(raw).not.toBeNull(); + expect(raw.code).toBe('SQLITE_CONSTRAINT_FOREIGNKEY'); + expect(raw.status).toBeUndefined(); + expect(raw.statusCode).toBeUndefined(); + // The production recogniser, on the real throw: a 500 that nobody + // declared. Both halves asserted, because the whole fix is the gap + // between them. + expect(resolveThrownHttpError(raw).status).toBe(500); + expect(resolveThrownHttpError(raw).declaredStatus).toBeUndefined(); + + const error = res.results[0].errors[0]; + expect(res.results[0].success).toBe(false); + expect(Object.prototype.hasOwnProperty.call(error, 'httpStatus')).toBe(false); + // Nowhere in the payload either — `reconcileStoppedBatch` copies the + // causal row's text onto its siblings, so a row-only scan can miss a + // live path. + expect(JSON.stringify(res)).not.toContain('httpStatus'); + }); + + it('a not-found row still answers 404 — the population that already worked', async () => { + const { protocol } = await rig(); + const res: any = await protocol.deleteManyData({ object: 'hs_task', ids: ['ghost'] }); + expect(res.results[0].errors[0]).toEqual({ + code: 'RECORD_NOT_FOUND', message: 'Record ghost not found in hs_task', httpStatus: 404, + }); + }); +}); diff --git a/packages/spec/src/api/contract.zod.ts b/packages/spec/src/api/contract.zod.ts index b5c427ad45..fb21574f0e 100644 --- a/packages/spec/src/api/contract.zod.ts +++ b/packages/spec/src/api/contract.zod.ts @@ -38,6 +38,13 @@ export const ApiErrorSchema = lazySchema(() => z.object({ * Optional and redundant on purpose: the response status is authoritative, so * a producer that emits only the semantic `code` is fully conformant. Callers * should branch on `code`, not on this. + * + * ABSENCE means the producer declared no status of its own — read it as "no + * claim made", never as a status of its own and never as 200 (#8570). It is + * load-bearing where this envelope rides response DATA rather than the + * response line: on a `BatchOperationResult` row, present = the throw behind + * that row declared a status, absent = an undeclared server-side fault the + * caller should treat as a 500. */ httpStatus: z.number().int().optional().describe('HTTP status of the response carrying this error'), details: z.unknown().optional().describe('Additional error context (e.g. field validation errors)'), diff --git a/packages/types/src/thrown-http-error.ts b/packages/types/src/thrown-http-error.ts index 8f26d71ac8..c0fd376785 100644 --- a/packages/types/src/thrown-http-error.ts +++ b/packages/types/src/thrown-http-error.ts @@ -73,6 +73,35 @@ import { validationFailureDetails, VALIDATION_FAILED_STATUS } from './validation export interface ThrownHttpError { /** The producer's own `status`/`statusCode`, or the caller's fallback. */ status: number; + /** + * The status the THROW ITSELF declared — `.status`, `.statusCode`, or the + * 400 a validation-shaped throw declares by shape — and **absent** when it + * declared none, i.e. when {@link ThrownHttpError.status} above is the + * caller's `fallbackStatus`. + * + * ## Why `status` cannot answer this + * + * A producer that declares `500` and one that declares nothing both resolve + * to `status: 500`, so a caller that must tell "the producer said so" from + * "I supplied the default" cannot read it off the value. The workaround in + * the repo was to probe this function with a fallback no producer declares + * — `resolveThrownHttpError(e, 0).status !== 0`, still spelled by hand in + * `packages/rest`'s publish-classification suite. That is a magic number + * standing in for a fact this function already computed, and it fails + * silently the day a producer declares the sentinel. So the fact is stated. + * + * ## Who needs the distinction + * + * A sink that mirrors the status onto RESPONSE DATA instead of into the + * response's own status line — where the fallback would not be a default but + * an invention. `metadata-protocol`'s `toRowApiError` is the measured one + * (#8570): a batch row rides a **200**, so stamping `status` there would put + * `httpStatus: 500` on every undeclared driver fault, an ADDITION to the + * wire, where stamping `declaredStatus` restores only what a producer really + * declared. Boundaries that answer with the status itself keep reading + * `status` — the fallback is exactly what they want. + */ + declaredStatus?: number; /** * A member of the declared ADR-0112 vocabulary — for a boundary whose * envelope is checked against it. Never the HTTP status. @@ -103,6 +132,7 @@ export interface ThrownHttpError { * | Question | Answer | * |---|---| * | status | `.status` → `.statusCode` → 400 if it is a validation failure → `fallbackStatus` | + * | declaredStatus | the same chain WITHOUT the fallback — absent when the throw declared none | * | code | `VALIDATION_FAILED` if it is one → a REGISTERED `.code` → derived from the status | * | declaredCode | `VALIDATION_FAILED` if it is one → any non-empty string `.code` → absent | * | message | `.message` when it is a string → `String(error)` | @@ -117,11 +147,17 @@ export function resolveThrownHttpError(error: unknown, fallbackStatus = 500): Th const e = error as any; const validation = validationFailureDetails(e); + // The validation SHAPE is a declaration too: `ValidationError` carries no + // status because deciding it means 400 is the boundary's job, but the + // producer did say "this is a client's input problem" — which is the fact + // `declaredStatus` reports. Only the `fallbackStatus` limb below is the + // caller's own invention, and it is the only one left out. const declaredStatus = typeof e?.status === 'number' ? e.status : typeof e?.statusCode === 'number' ? e.statusCode + : validation ? VALIDATION_FAILED_STATUS : undefined; - const status = declaredStatus ?? (validation ? VALIDATION_FAILED_STATUS : fallbackStatus); + const status = declaredStatus ?? fallbackStatus; const spelled = typeof e?.code === 'string' && e.code !== '' ? e.code : undefined; // A `.code` the ledger does not know cannot go in a slot typed as the closed @@ -146,6 +182,7 @@ export function resolveThrownHttpError(error: unknown, fallbackStatus = 500): Th return { status, + ...(declaredStatus !== undefined ? { declaredStatus } : {}), code, ...(declaredCode !== undefined ? { declaredCode } : {}), message: typeof e?.message === 'string' ? e.message : String(error), diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ad2feb1982..6b354f08b4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1423,6 +1423,9 @@ importers: '@objectstack/driver-sql': specifier: workspace:* version: link:../../drivers/driver-sql + '@objectstack/metadata-protocol': + specifier: workspace:* + version: link:../../metadata-protocol '@objectstack/objectql': specifier: workspace:* version: link:../../objectql From 14cde1a9027517ebb4471e0c0cbc204c7f9be0c1 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 04:28:31 +0000 Subject: [PATCH 2/3] chore: changeset for the batch row's declared httpStatus (#8570) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .changeset/batch-row-declared-http-status.md | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 .changeset/batch-row-declared-http-status.md diff --git a/.changeset/batch-row-declared-http-status.md b/.changeset/batch-row-declared-http-status.md new file mode 100644 index 0000000000..31590c8acb --- /dev/null +++ b/.changeset/batch-row-declared-http-status.md @@ -0,0 +1,31 @@ +--- +"@objectstack/metadata-protocol": patch +"@objectstack/types": minor +--- + +A bulk write's per-row `errors[].httpStatus` carries the status its producer declared, in any spelling (#8570) + +`toRowApiError` set the limb from `err.status` alone, so two well-defined client +refusals shipped a batch row with no status at all: objectql's `ValidationError` +(a 400 recognisable by shape, which deliberately declares no `status`) and +`plugin-approvals`' record lock (a 409 spelled `statusCode`). Sibling rows in the +same response did carry one — `rowRequiredIdError` → 400, +`recordNotFoundError` → 404 — so a caller branching on `httpStatus` to tell "fix +your input" from "the server broke" got an answer for some failure rows and +silence for others, with nothing saying which. Same single-spelling defect #7525 +fixed at the HTTP door, one layer down. + +The limb now asks `resolveThrownHttpError` — the resolver the HTTP doors and the +row's `message` limb already answer with — so a refusal declaring `.status`, +`.statusCode` or the `VALIDATION_FAILED` shape reaches the row as the status it +always meant. Rows whose throw declared nothing (a driver fault, a hook throwing +a bare `Error`) still carry no `httpStatus`: the resolver's 500 there is the +caller's fallback, not a producer's claim, and stamping it would add a field to +the wire for those populations rather than restore a declared one. `code` reads +the same resolution, so a row can no longer contradict itself. + +`ThrownHttpError` gains `declaredStatus` — the resolved status minus the +fallback, absent when the throw declared none. `status` is unchanged, and every +boundary that answers with the status itself keeps reading it; the new field is +for sinks that mirror a status onto response DATA, where a fallback would be an +invention. From 048b21ee9c746879d42f8e1893acb74ad6207b02 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Fri, 14 Aug 2026 04:52:08 +0000 Subject: [PATCH 3/3] fix(test): pass the required packageId in the #8570 approvals batch-row pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registry.registerObject` takes (schema, packageId, …). The new integration test omitted the second argument, which the package's own `typecheck` script cannot see — its tsconfig excludes `**/*.test.ts` — while the TEST_DEBT ratchet measures tsc WITH the test layer in the program and counted the +1. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../src/record-lock-batch-row-status.integration.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts b/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts index b45a0a5068..792a0b43ca 100644 --- a/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/record-lock-batch-row-status.integration.test.ts @@ -88,7 +88,13 @@ describe('[#8570] a locked record\'s batch row carries the 409 the hook declared useNullAsDefault: true, }), true); await engine.init(); - for (const o of [opportunity, approvalRequest]) engine.registry.registerObject(o as any); + // `packageId` is REQUIRED by `registerObject` — passed rather than elided + // so this file adds no raw `tsc` error to the package's TEST_DEBT ledger, + // which is measured with the test layer in the program (the package's own + // `typecheck` script excludes `**/*.test.ts`, so it cannot see this). + for (const o of [opportunity, approvalRequest]) { + engine.registry.registerObject(o as any, 'com.objectstack.test.8570'); + } // Real DDL through the real path. await engine.syncSchemas();