From d5abc30a5bd957eb47f58de9f50990ed19b1b9ab Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 01:25:47 +0000 Subject: [PATCH 1/3] fix(service-package): classify a publish driver fault as 5xx and stop returning driver text (#8131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/v1/packages/publish` answered `400 PACKAGE_PUBLISH_FAILED` with the raw driver message whenever the `INSERT INTO sys_packages` statement failed. Reproduced on a real SQLite engine before changing anything: 400 {"code":"PACKAGE_PUBLISH_FAILED","message":"no such table: sys_packages"} 400 {"code":"PACKAGE_PUBLISH_FAILED", "message":"NOT NULL constraint failed: sys_packages.tenant_ref"} Two independent defects: a server fault labelled a client error, and a driver dump handed back as caller-visible data. Fixed at the producer. `publish` no longer returns `(error as Error).message`; it returns a discriminated `driverFault` carrying a stable, non-interpolated sentence, and re-throws a refusal that declares its own status so the door's existing mapping answers it with that status and code. The door maps a returned driver fault to 500. The producer half is load-bearing, and measured to be: the 5xx withhold lives in `sendThrownError`, which a RETURNED failure never reaches at any status, and `looksLikeInternalErrorLeak('no such table: sys_packages')` is false — so reclassifying alone would have left the driver line on the wire. The discriminant is the STATUS channel only. Accepting a string `code` as a declaration was tried and reverted: every SQL driver populates it (ERR_SQLITE_ERROR, 42P01, ER_NO_SUCH_TABLE), so it re-threw genuine driver faults into a 500 whose message the heuristic does not withhold. Caller-facing 4xx is untouched, per the card's binding scope guard. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .changeset/lucky-schools-smash.md | 56 +++ .../src/package-envelope.conformance.test.ts | 39 +- ...kage-publish-status-classification.test.ts | 403 ++++++++++++++++++ packages/rest/src/package-routes.ts | 34 +- .../services/service-package/src/index.ts | 126 +++++- .../src/publish-driver-fault.test.ts | 341 +++++++++++++++ 6 files changed, 993 insertions(+), 6 deletions(-) create mode 100644 .changeset/lucky-schools-smash.md create mode 100644 packages/rest/src/package-publish-status-classification.test.ts create mode 100644 packages/services/service-package/src/publish-driver-fault.test.ts diff --git a/.changeset/lucky-schools-smash.md b/.changeset/lucky-schools-smash.md new file mode 100644 index 0000000000..a2675bf477 --- /dev/null +++ b/.changeset/lucky-schools-smash.md @@ -0,0 +1,56 @@ +--- +'@objectstack/service-package': major +'@objectstack/rest': patch +--- + +Package publish: a driver fault is answered as a server error, and its driver text no longer reaches the caller + +`POST /api/v1/packages/publish` answered **`400 PACKAGE_PUBLISH_FAILED`** when the +`INSERT INTO sys_packages` statement itself failed, carrying the driver's own message as +the caller-facing text. Measured on a real SQLite engine, that was literally: + +``` +400 {"success":false,"error":{"code":"PACKAGE_PUBLISH_FAILED", + "message":"no such table: sys_packages"}} +400 {"success":false,"error":{"code":"PACKAGE_PUBLISH_FAILED", + "message":"NOT NULL constraint failed: sys_packages.tenant_ref"}} +``` + +Two defects in one line. The **status** was a client error for a fault the client had no +part in — the mirror of the mislabelling fixed for the throw path, and it hid a real +server fault from every dashboard that buckets by status. The **message** was raw driver +text: a constraint dump naming physical tables and columns. + +Fixed at the producer, which is the only place that closes it. A 5xx message withhold +already exists at this door, but it is applied when an error is *thrown*, and this +failure was *returned* — so it never met the withhold at any status. The withhold is also +a phrasing heuristic, and `no such table: sys_packages` trips none of its keywords, so +reclassifying alone would have moved the driver line from a 400 to a 500 and left it on +the wire. + +Now: the driver's text goes to the log and nowhere else (it was already logged — nothing +an operator sees changes), and the caller gets a stable sentence that names what happened +without quoting the driver. + +**Caller-facing 4xx messages are unchanged.** A missing manifest, an invalid manifest, and +any coded refusal thrown from below `publish` all keep their own status, code and +self-correcting message — a `409 DESTRUCTIVE_CHANGE` is still a 409. + +**Breaking — `PackageService.publish` return shape.** A bare `error` string could not say +which side was at fault, so the door had one status for both and picked the wrong one. It +is replaced by a discriminated outcome: + +```ts +// FROM +publish(...): Promise<{ success: boolean; error?: string }> +// TO +publish(...): Promise<{ success: boolean; driverFault?: { message: string } }> +``` + +**Fix:** read `result.driverFault?.message` where you read `result.error`. If you +implement `PackageService` yourself: report a broken write as +`{ success: false, driverFault: { message } }` with a message safe to show a caller, and +**throw** — rather than return — a refusal that carries its own `status`, so the door +answers it with that status and code. + + diff --git a/packages/rest/src/package-envelope.conformance.test.ts b/packages/rest/src/package-envelope.conformance.test.ts index 7f116fbd11..3df09ebdec 100644 --- a/packages/rest/src/package-envelope.conformance.test.ts +++ b/packages/rest/src/package-envelope.conformance.test.ts @@ -236,11 +236,44 @@ describe('packages envelope (#3843) — error bodies', () => { run: () => drive(mount({}), 'POST', `${PKGS}/publish`, { body: { manifest: {}, metadata: {} } }), }, { - name: 'a publish the service refuses', - status: 400, + // [#8131] Was `a publish the service refuses`, driving + // `{ success: false, error: 'version already published' }` against a + // 400. That fixture pinned the exact limb #8131 removed — a bare + // `error` string, which could not say whether the caller or the write + // was at fault, so the door answered one status for both and picked the + // client's. Replaced rather than re-spelled: the two outcomes it + // conflated are now separate cases, here and below. + name: 'a publish whose WRITE broke — a driver fault, so a 5xx', + status: 500, code: 'PACKAGE_PUBLISH_FAILED', run: () => drive( - mount({ publish: async () => ({ success: false, error: 'version already published' }) }), + mount({ + publish: async () => ({ + success: false, + driverFault: { message: 'The package registry could not store this package.' }, + }), + }), + 'POST', + `${PKGS}/publish`, + { body: { manifest: MANIFEST, metadata: {} } }, + ), + }, + { + // The other half of what the old fixture named: a genuine REFUSAL still + // answers 4xx, with the producer's own code — it is thrown, not + // returned, and #8016's mapping reads it off the throw. + name: 'a publish the service REFUSES keeps its own 4xx and code', + status: 409, + code: 'RESOURCE_CONFLICT', + run: () => drive( + mount({ + publish: async () => { + throw Object.assign(new Error('com.acme.crm@1.0.0 is already published.'), { + status: 409, + code: 'RESOURCE_CONFLICT', + }); + }, + }), 'POST', `${PKGS}/publish`, { body: { manifest: MANIFEST, metadata: {} } }, diff --git a/packages/rest/src/package-publish-status-classification.test.ts b/packages/rest/src/package-publish-status-classification.test.ts new file mode 100644 index 0000000000..67444abcfd --- /dev/null +++ b/packages/rest/src/package-publish-status-classification.test.ts @@ -0,0 +1,403 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8131] `POST /api/v1/packages/publish` answers a driver fault as a 5xx, + * and the caller's own errors as 4xx. + * + * ## The defect + * + * `packageService.publish` reported failure by RETURNING, so the handler + * answered `sendError(res, 400, 'PACKAGE_PUBLISH_FAILED', result.error …)`. + * Two things were wrong with that line and they are independent: + * + * - **the status** — a driver fault answered `400`, a *client* error. The + * mirror of what #8016 fixed on the throw path (`a caller who was refused + * was told the platform had broken`): here the platform broke and the + * caller was told they had made a mistake. Every dashboard that buckets by + * status counted a server fault as a client one. + * - **the message** — `result.error` was `(error as Error).message`, the raw + * driver line, straight onto the wire. + * + * ## Why fixing the status did not fix the message + * + * The dispatch's load-bearing assumption was that once this path is a 5xx, + * #8086's withhold covers it "with no new rule". Both halves of that are + * measured false here, in sections 3 and 4, because it is the reason the fix + * had to reach the producer: + * + * - the withhold is applied by `sendThrownError`, which a RETURNED failure + * never reaches — `sendError` has no predicate in it at any status; + * - and `looksLikeInternalErrorLeak('no such table: sys_packages')` is + * **false** — the commonest real failure of the `INSERT INTO sys_packages` + * statement names no keyword the heuristic knows. + * + * So the producer now emits a stable sentence and no driver text at all + * (`service-package/src/publish-driver-fault.test.ts` drives that with a real + * SQLite engine). This file pins the door's half: the classification, and the + * 4xx paths that must NOT move. + * + * ## What is deliberately NOT asserted + * + * That the body "no longer contains" a driver line, on its own. That passes on + * a route that emits nothing at all, including one whose handler never ran. + * Every case below asserts the POSITIVE shape — the exact status, the exact + * code, the exact message — and the service-reached half where a stub can say + * so. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ApiErrorSchema, BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; +import type { RouteHandler } from '@objectstack/spec/contracts'; +import { + INTERNAL_ERROR_MESSAGE, + looksLikeInternalErrorLeak, + resolveThrownHttpError, + sendError, +} from '@objectstack/types'; +import { registerPackageRoutes } from './package-routes.js'; + +const PKGS = '/api/v1/packages'; +const MANIFEST = { id: 'com.acme.crm', version: '1.0.0' }; +const BODY = { manifest: MANIFEST, metadata: { author: 'acme' } }; + +/** + * The exact sentence the producer emits. Spelled here rather than imported: + * `@objectstack/service-package` resolves through `exports` to `dist/`, so a + * VALUE import of it would make this suite a verdict about a build artifact + * (`check:test-source-alias`). A drift between the two spellings is caught by + * the producer's own suite, which asserts the constant it exports. + */ +const DRIVER_FAULT_SENTENCE = + 'The package registry could not store this package. The failure was logged on the server; ' + + 'no package data was written.'; + +/** The driver lines the real engine produced for this statement, measured. */ +const REAL_DRIVER_LINES = [ + 'no such table: sys_packages', + 'NOT NULL constraint failed: sys_packages.tenant_ref', +]; + +interface Captured { status: number; body: any } + +const CLEARS_THE_GATE = async () => ({ + userId: 'u_pkg', + systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'], +}); + +function mount(svc: Record, options: Record = {}) { + const routes = new Map(); + const server = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); }, + put: () => {}, delete: (p: string, h: RouteHandler) => { routes.set(`DELETE:${p}`, h); }, + patch: () => {}, use: () => {}, listen: async () => {}, close: async () => {}, + } as any; + registerPackageRoutes(server, () => svc as any, '/api/v1', { + resolveExecutionContext: CLEARS_THE_GATE, ...options, + } as any); + return routes; +} + +async function drive( + routes: Map, + method: string, + path: string, + req: Record = {}, +): Promise { + const handler = routes.get(`${method}:${path}`); + if (!handler) throw new Error(`no handler for ${method} ${path}`); + const captured: Captured = { status: 0, body: undefined }; + const res: any = { + json(d: any) { captured.body = d; }, send() {}, + status(c: number) { captured.status = c; return res; }, header() { return res; }, + }; + await handler({ params: {}, query: {}, body: undefined, headers: {}, method, path, ...req } as any, res); + return captured; +} + +/** The declared envelope, imported from `packages/spec` rather than restated. */ +function expectDeclaredEnvelope(captured: Captured): any { + expect(BaseResponseSchema.safeParse(captured.body).success).toBe(true); + expect(envelopeViolations(captured.body)).toEqual([]); + expect(captured.body?.success).toBe(false); + const parsed = ApiErrorSchema.safeParse(captured.body?.error); + expect(parsed.error?.issues ?? []).toEqual([]); + expect(parsed.success).toBe(true); + return captured.body.error; +} + +// --------------------------------------------------------------------------- +// 1. A reported driver fault is a 5xx carrying the stable sentence +// --------------------------------------------------------------------------- + +describe('[#8131] a returned driver fault answers 5xx, not 400', () => { + it('status, code and message together — the classification AND the disclosure', async () => { + const publish = vi.fn(async () => ({ + success: false, driverFault: { message: DRIVER_FAULT_SENTENCE }, + })); + + const captured = await drive(mount({ publish }), 'POST', `${PKGS}/publish`, { body: BODY }); + + // The seam really ran — otherwise every assertion below is about a route + // that refused before reaching `publish`, which is a different answer. + expect(publish, 'publish was never called').toHaveBeenCalledTimes(1); + + const error = expectDeclaredEnvelope(captured); + // ① the half that was mislabelled + expect(captured.status).toBe(500); + // ② the code is kept — it discloses nothing and says more than INTERNAL_ERROR + expect(error.code).toBe('PACKAGE_PUBLISH_FAILED'); + // ③ the positive message shape, not "it changed" + expect(error.message).toBe(DRIVER_FAULT_SENTENCE); + }); + + it('no driver line the real engine emits can reach the wire through this path', async () => { + // Paired with the positive assertion above so it cannot pass vacuously: + // the body is a real failure body with a real message, and these strings + // are still absent from it. + for (const line of REAL_DRIVER_LINES) { + const captured = await drive( + mount({ publish: async () => ({ success: false, driverFault: { message: DRIVER_FAULT_SENTENCE } }) }), + 'POST', `${PKGS}/publish`, { body: BODY }, + ); + expect(captured.body?.error?.message).toBe(DRIVER_FAULT_SENTENCE); + expect(JSON.stringify(captured.body)).not.toContain(line); + expect(JSON.stringify(captured.body)).not.toContain('sys_packages'); + } + }); + + it('a service that reports failure without saying why still answers 5xx', async () => { + // The `??` arm. It is not a leniency alias: it is the answer for an + // implementation that returns a bare `{ success: false }`, which the old + // `error?: string` could not tell apart from a driver dump. + const captured = await drive( + mount({ publish: async () => ({ success: false }) }), + 'POST', `${PKGS}/publish`, { body: BODY }, + ); + const error = expectDeclaredEnvelope(captured); + expect(captured.status).toBe(500); + expect(error.code).toBe('PACKAGE_PUBLISH_FAILED'); + expect(error.message).toBe(`Failed to publish ${MANIFEST.id}.`); + }); + + it('a successful publish is untouched', async () => { + const captured = await drive( + mount({ publish: async () => ({ success: true }) }), + 'POST', `${PKGS}/publish`, { body: BODY }, + ); + expect(captured.status).toBe(200); + expect(captured.body?.success).toBe(true); + expect(captured.body?.data?.package).toEqual({ id: 'com.acme.crm', version: '1.0.0' }); + }); +}); + +// --------------------------------------------------------------------------- +// 2. The caller's own errors are STILL 4xx — the over-block guard +// --------------------------------------------------------------------------- +// +// The ruling this card carries is that 4xx must not be swept. Without this +// section the change above is satisfied by "answer 500 for every publish +// failure", which would destroy the self-correcting messages #4277 exists for +// and re-break what #8016 fixed. + +describe('[#8131] a genuine CALLER error on this route is still 4xx', () => { + const CALLER_ERRORS: Array<{ name: string; body: any; status: number; code: string; message: string }> = [ + { + name: 'no manifest/metadata at all', + body: {}, + status: 400, + code: 'MISSING_REQUIRED_FIELD', + message: 'Missing required fields: manifest, metadata', + }, + { + name: 'a manifest with no id/version', + body: { manifest: {}, metadata: {} }, + status: 400, + code: 'PACKAGE_MANIFEST_INVALID', + message: 'Invalid manifest: id and version are required', + }, + ]; + + for (const c of CALLER_ERRORS) { + it(`${c.name}: ${c.status} ${c.code}, message intact`, async () => { + const publish = vi.fn(); + const captured = await drive(mount({ publish }), 'POST', `${PKGS}/publish`, { body: c.body }); + + // The refusal's other half: the service was never reached. A status + // assertion alone would not notice a handler that published anyway. + expect(publish, 'publish ran on a request that should have been refused').not.toHaveBeenCalled(); + + const error = expectDeclaredEnvelope(captured); + expect(captured.status).toBe(c.status); + expect(error.code).toBe(c.code); + // The self-correcting sentence survives verbatim — it names what to fix. + expect(error.message).toBe(c.message); + }); + } + + it('a REFUSAL thrown from below publish keeps its own status and code', async () => { + // The producer re-throws a declared envelope rather than swallowing it, so + // #8016's mapping answers. Before #8131 this arrived as + // `{ success: false, error }` and came out as `400 PACKAGE_PUBLISH_FAILED` + // — the producer's status AND code both lost. + const refusal = Object.assign(new Error('Uninstalling drops 3 tables; pass force: true.'), { + status: 409, code: 'DESTRUCTIVE_CHANGE', + }); + const captured = await drive( + mount({ publish: async () => { throw refusal; } }), + 'POST', `${PKGS}/publish`, { body: BODY }, + ); + const error = expectDeclaredEnvelope(captured); + expect(captured.status).toBe(409); + expect(error.code).toBe('DESTRUCTIVE_CHANGE'); + expect(error.message).toBe('Uninstalling drops 3 tables; pass force: true.'); + }); + + it('the 4xx/5xx split is decided by the CHANNEL, not by the message', async () => { + // The same sentence, once thrown with a declared 4xx and once returned as + // a driver fault. If the door ever starts sniffing the text, this splits. + const sentence = 'com.acme.crm@1.0.0 could not be written.'; + const thrown = await drive( + mount({ publish: async () => { throw Object.assign(new Error(sentence), { status: 422, code: 'VALIDATION_ERROR' }); } }), + 'POST', `${PKGS}/publish`, { body: BODY }, + ); + expect(thrown.status).toBe(422); + expect(thrown.body?.error?.message).toBe(sentence); + + const returned = await drive( + mount({ publish: async () => ({ success: false, driverFault: { message: sentence } }) }), + 'POST', `${PKGS}/publish`, { body: BODY }, + ); + expect(returned.status).toBe(500); + expect(returned.body?.error?.message).toBe(sentence); + }); +}); + +// --------------------------------------------------------------------------- +// 3. WHY the producer half was required: the withhold is unreachable here +// --------------------------------------------------------------------------- +// +// Recorded as executable fact rather than prose, because the dispatch assumed +// the opposite and the assumption is the kind that gets re-made. + +describe('[#8131] the 5xx withhold does NOT cover a RETURNED failure', () => { + it('sendError applies no leak predicate — at any status', async () => { + // The withhold (#8086) lives in `sendThrownError`. `sendError` is the + // envelope writer and deliberately carries no disclosure rule (its own + // module note disclaims one). So reclassifying this path to 500 without + // fixing the producer would have moved the driver line from a 400 to a + // 500 and left it on the wire. + const leak = 'SQLITE_ERROR: no such table: sys_packages'; + expect(looksLikeInternalErrorLeak(leak)).toBe(true); + + const captured: Captured = { status: 0, body: undefined }; + const res: any = { + json(d: any) { captured.body = d; }, send() {}, + status(c: number) { captured.status = c; return res; }, header() { return res; }, + }; + sendError(res, 500, 'PACKAGE_PUBLISH_FAILED', leak); + + expect(captured.status).toBe(500); + // Verbatim — no withhold ran. This is the measurement, not a wish. + expect(captured.body?.error?.message).toBe(leak); + expect(captured.body?.error?.message).not.toBe(INTERNAL_ERROR_MESSAGE); + }); +}); + +// --------------------------------------------------------------------------- +// 4. …and the heuristic would have missed it anyway +// --------------------------------------------------------------------------- + +describe('[#8131] looksLikeInternalErrorLeak does not recognise this statement’s commonest failure', () => { + it('`no such table: sys_packages` is measured FALSE', () => { + // The same ceiling #8086 pinned for the Postgres phrasing, hit by SQLite + // too: the message names no `sqlite_`, no `sqlstate`, no `constraint + // failed`, and does not START with a statement keyword. So even routed + // through `sendThrownError` at 500 it would have travelled whole. + // + // ⛔ Do not "fix" this by widening the predicate — that is a phrasing arms + // race across every dialect, and #8136 rules it out explicitly. The cure + // is the producer, which is where #8131 put it. + expect(looksLikeInternalErrorLeak('no such table: sys_packages')).toBe(false); + + // Its sibling DOES trip, which is what makes the case above a real gap + // rather than a claim that the predicate never works. + expect(looksLikeInternalErrorLeak('NOT NULL constraint failed: sys_packages.tenant_ref')).toBe(true); + }); + + it('this case goes red the day the predicate learns the phrasing — that is the signal', () => { + // Stated positively so the day it changes is visible, per #8086's ceiling + // note. A reader arriving because this went red should delete it, not + // repair it. + expect(looksLikeInternalErrorLeak('no such table: sys_packages')).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// 5. The producer's declaration test agrees with the shared #8016 rule +// --------------------------------------------------------------------------- +// +// `service-package` asks "did this throw DECLARE an envelope?" with a local +// predicate rather than importing `resolveThrownHttpError`, because +// value-importing `@objectstack/types` there would make that package's unit +// pins a verdict about a build artifact (`check:test-source-alias`). The +// agreement is therefore asserted HERE, where `@objectstack/types` is already +// a value dependency — so the two cannot drift silently. + +describe('[#8131] the producer re-throws exactly what the shared rule can map', () => { + /** + * `declaresHttpAnswer` keys on the STATUS channel — `.status` or + * `.statusCode` — and deliberately not on `.code`. Asked here of the shared + * rule with a sentinel `fallbackStatus` no producer declares: a resolved + * status that is still the sentinel means nothing was declared. + */ + const declaredStatus = (error: unknown) => resolveThrownHttpError(error, 0).status !== 0; + + const SHAPES: Array<{ name: string; error: unknown; rethrown: boolean }> = [ + { name: '.status', error: Object.assign(new Error('x'), { status: 409 }), rethrown: true }, + { name: '.statusCode', error: Object.assign(new Error('x'), { statusCode: 400 }), rethrown: true }, + { name: 'a declared 5xx', error: Object.assign(new Error('x'), { status: 503, code: 'SERVICE_UNAVAILABLE' }), rethrown: true }, + { name: 'bare Error', error: new Error('no such table: sys_packages'), rethrown: false }, + { name: 'a string throw', error: 'boom', rethrown: false }, + { name: 'null', error: null, rethrown: false }, + ]; + + for (const s of SHAPES) { + it(`${s.name}: re-thrown=${s.rethrown}, and the shared rule agrees a status was declared=${s.rethrown}`, () => { + expect(declaredStatus(s.error)).toBe(s.rethrown); + }); + } + + /** + * ⛔ Why `.code` is excluded, asserted rather than argued. + * + * The shared rule keeps a producer's `.code` in `declaredCode` even when the + * ledger does not know it — correct for the dispatcher door, which puts + * unregistered codes on the wire by design. But a **driver** populates that + * same field: `ERR_SQLITE_ERROR`, `42P01`, `ER_NO_SUCH_TABLE`. So + * `declaredCode` cannot be a "this is a refusal" signal at the producer, and + * the case below shows what accepting it would have cost — the raw driver + * line, resolved to a 500 whose message the heuristic does not withhold. + */ + it('a driver `code` reads as a declaredCode but must NOT make the producer re-throw', () => { + const driverError = Object.assign(new Error('no such table: sys_packages'), { + code: 'ERR_SQLITE_ERROR', + }); + + // The shared rule does record it… + const resolved = resolveThrownHttpError(driverError, 0); + expect(resolved.declaredCode).toBe('ERR_SQLITE_ERROR'); + // …while declaring NO status of its own, which is the signal that counts. + expect(resolved.status).toBe(0); + expect(declaredStatus(driverError)).toBe(false); + + // And had it been re-thrown, this is what the door would have answered: + // a 500 whose message is the driver line verbatim, because the heuristic + // does not recognise this phrasing. + const asThrown = resolveThrownHttpError(driverError); + expect(asThrown.status).toBe(500); + expect(asThrown.code).toBe('INTERNAL_ERROR'); + expect(looksLikeInternalErrorLeak(asThrown.message)).toBe(false); + expect(asThrown.message).toBe('no such table: sys_packages'); + }); +}); diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index 2efd8593d3..d56ecfa358 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -417,7 +417,39 @@ export function registerPackageRoutes( return; } - sendError(res, 400, 'PACKAGE_PUBLISH_FAILED', result.error ?? `Failed to publish ${manifest.id}.`); + // [#8131] A REPORTED publish failure is a DRIVER FAULT, and a driver + // fault is a **5xx**. This answered `400` for as long as it existed — + // telling a caller to fix a request that was never the problem, and + // hiding a real server fault from every dashboard that buckets by + // status. It is the mirror of what #8016 fixed on the throw path there + // (`a caller who was refused was told the platform had broken`); here + // the platform broke and the caller was told they had made a mistake. + // + // The CALLER's own errors on this route are unaffected and still 4xx: + // the missing-field and invalid-manifest refusals above are checked + // before `publish` is called at all, and a coded refusal thrown from + // below `publish` is re-thrown by the producer and answered by + // {@link sendThrownError} with its own status (#8016) — so a `409 + // DESTRUCTIVE_CHANGE` is still a 409, not swept in here. + // + // The code stays `PACKAGE_PUBLISH_FAILED` rather than becoming + // `INTERNAL_ERROR`: it is registered, it is more informative than the + // generic fallback, and it discloses nothing (the *message* was the + // disclosure, and the producer no longer emits one). `envelopeViolations` + // imposes no code↔status agreement, so a registered code on a 5xx is + // conformant — `SERVICE_UNAVAILABLE` at 503 is the same shape. + // + // `result.driverFault.message` is a CONSTANT the producer owns and never + // interpolates into; the `??` arm is not a leniency alias but the answer + // for a `PackageService` implementation that reports failure without + // saying why, which is the one thing the old `error?: string` could not + // distinguish from a driver dump. + sendError( + res, + 500, + 'PACKAGE_PUBLISH_FAILED', + result.driverFault?.message ?? `Failed to publish ${manifest.id}.`, + ); } catch (error) { sendThrownError(res, error); } diff --git a/packages/services/service-package/src/index.ts b/packages/services/service-package/src/index.ts index ce7f1940ae..582459d7a8 100644 --- a/packages/services/service-package/src/index.ts +++ b/packages/services/service-package/src/index.ts @@ -26,13 +26,107 @@ export interface PackageRecord { updated_at: string; } +/** + * [#8131] The caller-facing sentence a driver-fault publish answers with. + * + * Deliberately a CONSTANT with no interpolation at all. The whole defect this + * closes was `(error as Error).message` being handed back as caller-visible + * data, so the remedy is not "interpolate something safer" — it is that this + * producer interpolates *nothing* into the message a caller reads. Exported so + * the door and its pins can assert the POSITIVE shape rather than the absence + * of a driver line (an absence assertion passes for any rewrite, including a + * worse one). + * + * It says the three things a caller can act on: the write did not land, the + * detail exists but on the server, and this is not theirs to fix. + */ +export const PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE = + 'The package registry could not store this package. The failure was logged on the server; ' + + 'no package data was written.'; + +/** + * [#8131] A publish that failed because the WRITE broke — a server fault. + * + * Its `message` is always safe to hand a caller: the driver's own text went to + * the log and travels no further. + */ +export interface PackagePublishDriverFault { + /** A stable sentence. NEVER interpolates driver text. */ + message: string; +} + +/** + * [#8131] The outcome of {@link PackageService.publish}. + * + * The discriminant is the CHANNEL, not a field to parse: + * + * - **Returned** `{ success: false, driverFault }` — the write itself broke. + * The caller did nothing wrong, so this is a **5xx**, and the driver's text + * is not theirs to read. + * - **Thrown**, carrying an ADR-0112 envelope — a *refusal*. `publish` does + * not swallow those; they leave by the door's `sendThrownError`, where + * #8016's mapping answers them with the producer's own status and code + * (a `409 DESTRUCTIVE_CHANGE` stays a 409) and #8086's withhold judges + * their prose. + * + * Replaces `error?: string`, which was that leak's carrier: a bare string + * cannot say which side is at fault, so the door had no way to answer anything + * but one status for both, and it picked the wrong one. + */ +export interface PackagePublishResult { + success: boolean; + /** Present only when the write broke. Absent on success. */ + driverFault?: PackagePublishDriverFault; +} + export interface PackageService { - publish(data: { manifest: ObjectStackManifest; metadata: PackageMetadata }): Promise<{ success: boolean; error?: string }>; + publish(data: { manifest: ObjectStackManifest; metadata: PackageMetadata }): Promise; get(packageId: string, version?: string): Promise; list(): Promise; delete(packageId: string, version?: string): Promise<{ success: boolean }>; } +/** + * [#8131] Does this throw DECLARE an HTTP answer of its own (ADR-0112)? + * + * The channel is the **status**, in both spellings `resolveThrownHttpError` + * (`@objectstack/types`, the ONE rule both package doors call since #8016) + * reads, and for the reason that function documents: both are produced in this + * repo (`metadata-protocol` throws `status`, `plugin-approvals` throws + * `statusCode`), and reading only one is how a deliberate refusal became a + * 500. + * + * ⛔ **`.code` is deliberately NOT a declaration here, and that is a measured + * decision, not an omission.** Every SQL driver populates a string `code` on + * its errors — `node:sqlite` throws `ERR_SQLITE_ERROR`, better-sqlite3 + * `SQLITE_ERROR`, Postgres the SQLSTATE `42P01`, MySQL `ER_NO_SUCH_TABLE`. + * An earlier draft of this predicate accepted any non-empty string `code`, and + * the real-driver cases in `publish-driver-fault.test.ts` went red at once: + * every genuine driver fault was re-thrown as if it were a refusal, resolved + * to `500 INTERNAL_ERROR` with the driver's own message, and — because + * `looksLikeInternalErrorLeak` is false for `no such table: sys_packages` — + * that message reached the wire. The exact leak this card closes, re-opened by + * the classifier. A producer that wants a specific answer declares a + * **status**; a bare string `code` is a channel it shares with every driver we + * ship, so it cannot carry intent at this seam. + * + * The predicate is asked HERE rather than imported because + * `@objectstack/types` resolves through `exports` to `dist/`, so + * value-importing it would make this package's unit pins a verdict about a + * build artifact (`check:test-source-alias`). Its agreement with the shared + * rule is pinned at the door, where `@objectstack/types` is already a value + * dependency — see `package-publish-status-classification.test.ts` §5. + * + * Note this is a test of DECLARATION, not of the status's band. A declared + * 5xx is re-thrown too: the producer said what it was, so the door's shared + * mapping — not this catch — is what should answer for it. + */ +function declaresHttpAnswer(error: unknown): boolean { + if (typeof error !== 'object' || error === null) return false; + const { status, statusCode } = error as { status?: unknown; statusCode?: unknown }; + return typeof status === 'number' || typeof statusCode === 'number'; +} + /** * Normalize the result of `objectql.execute()` into a row array. * @@ -111,10 +205,38 @@ export class PackageServicePlugin implements Plugin { logger.info(`Published package: ${data.manifest.id}@${data.manifest.version}`); return { success: true }; } catch (error) { + // [#8131] ① The raw driver text goes to the LOG — the one place it + // belongs, and where it already went. Nothing about the operator's + // diagnostics changes here; what changes is that this is now the + // ONLY place it goes. logger.error('Failed to publish package', error as Error); + + // ② A throw that DECLARES its own envelope is a REFUSAL, and a + // refusal is not this method's to swallow. Re-thrown so it leaves by + // the door's catch-all, where #8016's shared mapping answers it with + // the producer's own status and code. Swallowing these is what + // flattened every coded refusal reachable from this call path into + // one `400 PACKAGE_PUBLISH_FAILED` — the mirror of the defect #8016 + // fixed for the throw path, and the reason a caller error and a + // server fault were indistinguishable on this route. + if (declaresHttpAnswer(error)) throw error; + + // ③ Everything else is a DRIVER FAULT: the `INSERT INTO + // sys_packages` broke, the caller's request was never the problem, + // and the driver's line — a constraint dump naming physical columns, + // a `SQLITE_ERROR`, an `SQLSTATE` — is not theirs to read. A stable + // sentence goes back instead. + // + // ⚠️ This is the half that actually closes the disclosure, and it + // has to be: the 5xx withhold (#8086) lives in the door's + // `sendThrownError`, which a RETURNED failure never reaches at any + // status — and even reached, `looksLikeInternalErrorLeak` is + // measured FALSE for `no such table: sys_packages`, the commonest + // real failure of this very statement. Correct classification alone + // would have left the text on the wire. return { success: false, - error: (error as Error).message, + driverFault: { message: PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE }, }; } }, diff --git a/packages/services/service-package/src/publish-driver-fault.test.ts b/packages/services/service-package/src/publish-driver-fault.test.ts new file mode 100644 index 0000000000..70d6384806 --- /dev/null +++ b/packages/services/service-package/src/publish-driver-fault.test.ts @@ -0,0 +1,341 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8131] `publish` stops handing driver text back as caller-visible data, + * and stops swallowing refusals. + * + * ## What was measured before the fix + * + * The card was filed with its `publish` path read from SOURCE, not reproduced, + * and said so. It was reproduced before this change: a REAL SQLite database + * behind `objectql.execute`, running the real `INSERT INTO sys_packages …` + * statement from `index.ts`, driven through the real handler. Two forced + * failures, both answered on the wire as: + * + * HTTP 400 + * {"success":false,"error":{"code":"PACKAGE_PUBLISH_FAILED", + * "message":"no such table: sys_packages"}} + * + * HTTP 400 + * {"success":false,"error":{"code":"PACKAGE_PUBLISH_FAILED", + * "message":"NOT NULL constraint failed: sys_packages.tenant_ref"}} + * + * i.e. exactly the card's claim, and on a **400** — a client error for a fault + * the client had no part in. + * + * ## Why the PRODUCER half is the load-bearing one + * + * The dispatch assumed that reclassifying this path to 5xx would put it inside + * #8086's withhold "with no new rule". Measured, that is false **twice**: + * + * 1. **Structurally** — the withhold lives in the door's `sendThrownError`. + * A failure that is *returned* reaches `sendError` directly and never + * meets it, at any status. Classification alone changes 400 to 500 and + * leaves the driver line exactly where it was. + * 2. **Semantically** — even routed through the withhold, + * `looksLikeInternalErrorLeak('no such table: sys_packages')` is **false** + * (it names no `sqlite_`, no `sqlstate`, no `constraint failed`, and does + * not start with a statement keyword). That is the commonest real failure + * of this very statement. Pinned at the door in + * `package-publish-status-classification.test.ts`, where the predicate + * lives. + * + * So the disclosure is closed HERE, at the producer, where no heuristic is + * involved and no dialect's phrasing has to be recognised — option C of #8086, + * for this producer. + * + * ## The driver is real on purpose + * + * A hand-thrown `new Error('no such table: …')` would pin the plumbing while + * proving nothing about what SQLite actually emits for these statements. The + * text asserted below is produced by SQLite, from the real DDL and the real + * INSERT. + */ + +import { describe, it, expect } from 'vitest'; +import { DatabaseSync } from 'node:sqlite'; +import { PackageServicePlugin, PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE, type PackageService } from './index.js'; + +const MANIFEST = { id: 'com.acme.crm', version: '1.0.0' } as any; +const METADATA = { author: 'acme' }; + +interface Booted { + svc: PackageService; + errorLogs: Array<{ msg: string; err?: any }>; +} + +/** + * A REAL SQLite database behind `objectql.execute`. `ensureTable` and the + * publish INSERT are the statements in `index.ts`, run verbatim. + * + * `mutate` runs AFTER the plugin has started, so the table it breaks is the + * one the service just created — the failure lands on the INSERT, which is + * the seam this card is about, not on boot. + */ +async function boot(mutate?: (db: DatabaseSync) => void): Promise { + const db = new DatabaseSync(':memory:'); + const engine = { + async execute({ sql, args }: { sql: string; args?: unknown[] }) { + const stmt = db.prepare(sql); + return /^\s*select/i.test(sql) + ? stmt.all(...((args ?? []) as any[])) + : stmt.run(...((args ?? []) as any[])); + }, + }; + + const errorLogs: Array<{ msg: string; err?: any }> = []; + const ctx: any = { + logger: { + debug: () => {}, info: () => {}, warn: () => {}, + error: (msg: string, err?: any) => errorLogs.push({ msg, err }), + }, + getService: (n: string) => (n === 'objectql' ? engine : undefined), + registerService: () => {}, + }; + let registered: PackageService | undefined; + ctx.registerService = (_n: string, s: PackageService) => { registered = s; }; + + const plugin = new PackageServicePlugin(); + await plugin.init(ctx); + await plugin.start(ctx); + if (mutate) mutate(db); + return { svc: registered!, errorLogs }; +} + +/** Every string a caller could read out of a publish outcome. */ +function callerVisibleText(result: unknown): string { + return JSON.stringify(result ?? null); +} + +// --------------------------------------------------------------------------- +// 1. A real driver fault: stable sentence out, raw text to the log only +// --------------------------------------------------------------------------- + +describe('[#8131] a real INSERT INTO sys_packages failure', () => { + const FAULTS: Array<{ name: string; break: (db: DatabaseSync) => void; driverText: string }> = [ + { + name: 'the table is gone (the missing-table family)', + break: (db) => db.exec('DROP TABLE sys_packages'), + driverText: 'no such table: sys_packages', + }, + { + name: 'a constraint dump naming the physical table and column', + break: (db) => { + db.exec('DROP TABLE sys_packages'); + db.exec(`CREATE TABLE sys_packages ( + id TEXT NOT NULL, version TEXT NOT NULL, manifest TEXT NOT NULL, + metadata TEXT NOT NULL, hash TEXT NOT NULL, + created_at TEXT, updated_at TEXT, tenant_ref TEXT NOT NULL, + PRIMARY KEY (id, version))`); + }, + driverText: 'NOT NULL constraint failed: sys_packages.tenant_ref', + }, + ]; + + for (const fault of FAULTS) { + it(`${fault.name}: the caller gets the stable sentence, the log gets the driver line`, async () => { + const { svc, errorLogs } = await boot(fault.break); + + const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); + + // ── The DISCLOSURE half and the ANTI-VACUITY half, asserted together. + // `not.toContain(driverText)` alone is green on a path that emitted no + // text at all — including a `publish` that never ran. The log assertion + // below is what proves SQLite really produced this exact line on this + // call, so the absence above is a withhold and not a no-op. + expect(result.success).toBe(false); + expect(result.driverFault?.message).toBe(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE); + expect(callerVisibleText(result)).not.toContain(fault.driverText); + expect(callerVisibleText(result)).not.toContain('sys_packages'); + + // The diagnostics are UNCHANGED — this fix moves the text, it does not + // delete it. An operator loses nothing. + const logged = errorLogs.find((l) => l.msg === 'Failed to publish package'); + expect(logged, 'the driver fault was never logged').toBeDefined(); + expect(String(logged!.err?.message)).toBe(fault.driverText); + }); + } + + it('the old `error` limb is gone, not merely unused', async () => { + // The carrier itself. A fix that kept `error` populated "for + // compatibility" would pass every assertion above while leaving the leak + // one field over — this is the case that refuses that shape. + const { svc } = await boot((db) => db.exec('DROP TABLE sys_packages')); + const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); + expect(Object.keys(result).sort()).toEqual(['driverFault', 'success']); + expect((result as Record).error).toBeUndefined(); + }); + + it('a healthy publish is unaffected (anti-vacuity for the whole section)', async () => { + const { svc, errorLogs } = await boot(); + const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); + expect(result).toEqual({ success: true }); + expect(errorLogs).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// 2. A DECLARED refusal is re-thrown, not swallowed +// --------------------------------------------------------------------------- +// +// The other half of the classification. Before this change `publish` caught +// every throw, so a coded refusal reachable from this call path came back as +// `{ success: false, error }` and the door answered `400 +// PACKAGE_PUBLISH_FAILED` — losing the producer's status AND its code. That is +// the same flattening #8016 removed from the door's catch-alls, one frame +// lower, where #8016's mapping could never see it. + +/** An engine whose `execute` throws whatever the case declares. */ +async function bootThrowing(error: unknown): Promise { + const errorLogs: Array<{ msg: string; err?: any }> = []; + let registered: PackageService | undefined; + let started = false; + const engine = { + async execute() { + // Let `ensureTable` (which runs first, inside `start`) succeed, so the + // throw lands on the publish INSERT and not on boot. + if (!started) return null; + throw error; + }, + }; + const ctx: any = { + logger: { + debug: () => {}, info: () => {}, warn: () => {}, + error: (msg: string, err?: any) => errorLogs.push({ msg, err }), + }, + getService: (n: string) => (n === 'objectql' ? engine : undefined), + registerService: (_n: string, s: PackageService) => { registered = s; }, + }; + const plugin = new PackageServicePlugin(); + await plugin.init(ctx); + await plugin.start(ctx); + started = true; + return { svc: registered!, errorLogs }; +} + +describe('[#8131] a throw that DECLARES an envelope is re-thrown, not swallowed', () => { + const DECLARED: Array<{ name: string; error: any }> = [ + { + name: 'a 409 with a registered code (the established DESTRUCTIVE_CHANGE shape)', + error: Object.assign(new Error('Uninstalling drops 3 tables.'), { + status: 409, code: 'DESTRUCTIVE_CHANGE', + }), + }, + { + name: 'the `statusCode` spelling — both are produced in this repo', + error: Object.assign(new Error('[tenant_scope_required] pass organizationId.'), { + statusCode: 400, + }), + }, + { + name: 'a declared 5xx — the producer said what it was, so it still answers', + error: Object.assign(new Error('The registry is warming up.'), { + status: 503, code: 'SERVICE_UNAVAILABLE', + }), + }, + ]; + + for (const c of DECLARED) { + it(`${c.name}: propagates UNCHANGED`, async () => { + const { svc, errorLogs } = await bootThrowing(c.error); + + // Identity, not merely "some throw": the door's #8016 mapping reads the + // producer's own `status`/`code` off this object, so a re-wrap would + // silently change the answer. + await expect(svc.publish({ manifest: MANIFEST, metadata: METADATA })) + .rejects.toBe(c.error); + + // Still logged on the way out — the log is not conditional on the exit. + expect(errorLogs.some((l) => l.msg === 'Failed to publish package')).toBe(true); + }); + } + + it('an UNDECLARED throw is NOT re-thrown — it is the driver fault of section 1', async () => { + // The discriminant, from the other side. Without this case the rule above + // is satisfied by "re-throw everything", which would put the raw driver + // line back on the wire through the door's catch-all. + const { svc } = await bootThrowing(new Error('no such table: sys_packages')); + const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); + expect(result.success).toBe(false); + expect(result.driverFault?.message).toBe(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE); + }); + + it('a non-object throw declares nothing and is a driver fault', async () => { + const { svc } = await bootThrowing('SQLITE_ERROR: disk I/O error'); + const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); + expect(result.success).toBe(false); + expect(callerVisibleText(result)).not.toContain('SQLITE_ERROR'); + }); + + /** + * ⛔ The regression that a `.code`-reading discriminant causes, pinned per + * driver dialect. + * + * An earlier draft of `declaresHttpAnswer` accepted any non-empty string + * `code` as a declaration — which reads as reasonable and is wrong, because + * a string `code` is exactly what every SQL driver puts on its errors. Under + * that draft each shape below was re-thrown as if it were a refusal, + * resolved to `500 INTERNAL_ERROR` carrying the driver's own message, and + * (the heuristic being false for the missing-table phrasing) put that + * message back on the wire — the very leak this card closes. + * + * These are the real spellings, not invented ones: `node:sqlite` really does + * throw `ERR_SQLITE_ERROR` — it is what made the section-1 cases fail while + * this fix was being written. + */ + const DRIVER_CODES: Array<{ dialect: string; code: unknown; message: string }> = [ + { dialect: 'node:sqlite', code: 'ERR_SQLITE_ERROR', message: 'no such table: sys_packages' }, + { dialect: 'better-sqlite3', code: 'SQLITE_ERROR', message: 'no such table: sys_packages' }, + { dialect: 'postgres (SQLSTATE)', code: '42P01', message: 'relation "sys_packages" does not exist' }, + { dialect: 'mysql', code: 'ER_NO_SUCH_TABLE', message: "Table 'os.sys_packages' doesn't exist" }, + { dialect: 'a numeric errno', code: 1299, message: 'NOT NULL constraint failed: sys_packages.hash' }, + { dialect: 'an empty string', code: '', message: 'no such table: sys_packages' }, + ]; + + for (const d of DRIVER_CODES) { + it(`a ${d.dialect} error \`code\` is NOT a declaration — still a driver fault`, async () => { + const { svc } = await bootThrowing(Object.assign(new Error(d.message), { code: d.code })); + const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); + expect(result.success).toBe(false); + expect(result.driverFault?.message).toBe(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE); + expect(callerVisibleText(result)).not.toContain(d.message); + expect(callerVisibleText(result)).not.toContain('sys_packages'); + }); + } +}); + +// --------------------------------------------------------------------------- +// 3. The message is a constant, and that is the point +// --------------------------------------------------------------------------- + +describe('[#8131] the caller-facing sentence interpolates nothing', () => { + it('is identical across different faults and different packages', async () => { + // If the sentence ever grows an interpolation, it grows a channel. Two + // different failures on two different packages must be byte-identical. + const a = await boot((db) => db.exec('DROP TABLE sys_packages')); + const first = await a.svc.publish({ manifest: MANIFEST, metadata: METADATA }); + + const b = await boot((db) => { + db.exec('DROP TABLE sys_packages'); + db.exec('CREATE TABLE sys_packages (id TEXT NOT NULL, extra TEXT NOT NULL)'); + }); + const second = await b.svc.publish({ + manifest: { id: 'com.other.app', version: '9.9.9' } as any, + metadata: { author: 'other' }, + }); + + expect(first.driverFault?.message).toBe(second.driverFault?.message); + expect(first.driverFault?.message).toBe(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE); + // Names neither package, so it cannot be echoing anything it was handed. + expect(second.driverFault?.message).not.toContain('com.other.app'); + }); + + it('says what a caller can act on: not persisted, logged, not theirs to fix', async () => { + // Pinned as prose because the sentence IS the contract here — a future + // edit that shortens it to "Publish failed." would pass every other case + // in this file while deleting what the caller needed. + expect(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE).toContain('could not store'); + expect(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE).toContain('logged on the server'); + expect(PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE).toContain('no package data was written'); + }); +}); From 56c53ed64cf4c145b5d37426c5cb81156771bca6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 02:06:37 +0000 Subject: [PATCH 2/3] docs(changeset): state the ADR-0087 disposition without a ledger-style FROM/TO block (#8131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ledger's subject is metadata; this change touches a runtime TS interface only. The consumer guidance stays — it is now prose plus the compiler error, which is the channel that actually reaches a caller of this service. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .changeset/lucky-schools-smash.md | 34 +++++++------------ .../src/publish-driver-fault.test.ts | 6 ++-- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/.changeset/lucky-schools-smash.md b/.changeset/lucky-schools-smash.md index a2675bf477..c74a58c4a1 100644 --- a/.changeset/lucky-schools-smash.md +++ b/.changeset/lucky-schools-smash.md @@ -1,5 +1,5 @@ --- -'@objectstack/service-package': major +'@objectstack/service-package': minor '@objectstack/rest': patch --- @@ -26,31 +26,23 @@ already exists at this door, but it is applied when an error is *thrown*, and th failure was *returned* — so it never met the withhold at any status. The withhold is also a phrasing heuristic, and `no such table: sys_packages` trips none of its keywords, so reclassifying alone would have moved the driver line from a 400 to a 500 and left it on -the wire. +the wire (measured, both). -Now: the driver's text goes to the log and nowhere else (it was already logged — nothing -an operator sees changes), and the caller gets a stable sentence that names what happened +Now the driver's text goes to the log and nowhere else — it was already logged, so nothing +an operator sees changes — and the caller gets a stable sentence that names what happened without quoting the driver. **Caller-facing 4xx messages are unchanged.** A missing manifest, an invalid manifest, and any coded refusal thrown from below `publish` all keep their own status, code and self-correcting message — a `409 DESTRUCTIVE_CHANGE` is still a 409. -**Breaking — `PackageService.publish` return shape.** A bare `error` string could not say -which side was at fault, so the door had one status for both and picked the wrong one. It -is replaced by a discriminated outcome: +**BREAKING — the `PackageService.publish` return shape.** A bare `error` string could not +say which side was at fault, so the door had one status for both and picked the wrong one. +`publish` now reports a broken write as `{ success: false, driverFault: { message } }`; +the `error` field is removed. If you only *call* `publish`, read +`result.driverFault?.message` where you read `result.error`. If you *implement* +`PackageService`, report a broken write through `driverFault` with a message safe to show +a caller, and **throw** — rather than return — a refusal that carries its own `status`, so +the door answers it with that status and code. -```ts -// FROM -publish(...): Promise<{ success: boolean; error?: string }> -// TO -publish(...): Promise<{ success: boolean; driverFault?: { message: string } }> -``` - -**Fix:** read `result.driverFault?.message` where you read `result.error`. If you -implement `PackageService` yourself: report a broken write as -`{ success: false, driverFault: { message } }` with a message safe to show a caller, and -**throw** — rather than return — a refusal that carries its own `status`, so the door -answers it with that status and code. - - + diff --git a/packages/services/service-package/src/publish-driver-fault.test.ts b/packages/services/service-package/src/publish-driver-fault.test.ts index 70d6384806..94cf0ab52f 100644 --- a/packages/services/service-package/src/publish-driver-fault.test.ts +++ b/packages/services/service-package/src/publish-driver-fault.test.ts @@ -57,7 +57,7 @@ import { DatabaseSync } from 'node:sqlite'; import { PackageServicePlugin, PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE, type PackageService } from './index.js'; const MANIFEST = { id: 'com.acme.crm', version: '1.0.0' } as any; -const METADATA = { author: 'acme' }; +const METADATA = { objects: [], views: [] }; interface Booted { svc: PackageService; @@ -163,7 +163,7 @@ describe('[#8131] a real INSERT INTO sys_packages failure', () => { const { svc } = await boot((db) => db.exec('DROP TABLE sys_packages')); const result = await svc.publish({ manifest: MANIFEST, metadata: METADATA }); expect(Object.keys(result).sort()).toEqual(['driverFault', 'success']); - expect((result as Record).error).toBeUndefined(); + expect((result as unknown as Record).error).toBeUndefined(); }); it('a healthy publish is unaffected (anti-vacuity for the whole section)', async () => { @@ -321,7 +321,7 @@ describe('[#8131] the caller-facing sentence interpolates nothing', () => { }); const second = await b.svc.publish({ manifest: { id: 'com.other.app', version: '9.9.9' } as any, - metadata: { author: 'other' }, + metadata: { objects: [] }, }); expect(first.driverFault?.message).toBe(second.driverFault?.message); From 4ebe2ffe3976ac9547c08d62b3940e783455b6a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 02:38:30 +0000 Subject: [PATCH 3/3] test(rest): invert the leak-predicate pins that #8132 turned red, and re-prove the fix without them (#8131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI went red on three cases in this PR's own new suite, all reading `expected true to be false`. Cause: #8132 (f598aa8) landed on main after this branch point and taught `looksLikeInternalErrorLeak` the bare-SQLite and Postgres phrasings, so `no such table: sys_packages` is now recognised. The pins asserted it was NOT. That is the outcome those cases were written to signal, and their own instruction — and #8136's — was "delete or invert; do not repair to green". Inverted, not silently flipped: the prose that claimed a gap is rewritten, because the gap is closed. What this does NOT change is why the producer fix exists. That argument had two halves and only the second is retired: - structural (still true, and now the sole reason): the withhold lives in `sendThrownError`; a RETURNED failure reaches `sendError`, which consults no predicate at any status; - semantic (retired by #8132): the phrasing used to trip nothing. Re-measured against the WIDENED predicate, main's producer with only the status corrected to 500 still answers `500 {"code":"PACKAGE_PUBLISH_FAILED","message":"no such table: sys_packages"}` — the driver line on the wire while a predicate that recognises it perfectly is never asked. A new case pins exactly that, so nobody concludes #8132 made this fix redundant. Docblocks, the producer comment and the changeset are corrected to match; no behaviour change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .changeset/lucky-schools-smash.md | 8 +- ...kage-publish-status-classification.test.ts | 124 +++++++++++++----- .../services/service-package/src/index.ts | 10 +- .../src/publish-driver-fault.test.ts | 27 ++-- 4 files changed, 114 insertions(+), 55 deletions(-) diff --git a/.changeset/lucky-schools-smash.md b/.changeset/lucky-schools-smash.md index c74a58c4a1..9bd182d635 100644 --- a/.changeset/lucky-schools-smash.md +++ b/.changeset/lucky-schools-smash.md @@ -23,10 +23,10 @@ text: a constraint dump naming physical tables and columns. Fixed at the producer, which is the only place that closes it. A 5xx message withhold already exists at this door, but it is applied when an error is *thrown*, and this -failure was *returned* — so it never met the withhold at any status. The withhold is also -a phrasing heuristic, and `no such table: sys_packages` trips none of its keywords, so -reclassifying alone would have moved the driver line from a 400 to a 500 and left it on -the wire (measured, both). +failure was *returned* — so it never met the withhold at any status. Reclassifying alone +would have moved the driver line from a 400 to a 500 and left it on the wire; that is +measured, and it stays true against the widened leak predicate that now recognises this +phrasing, because nothing on the returned path ever consults one. Now the driver's text goes to the log and nowhere else — it was already logged, so nothing an operator sees changes — and the caller gets a stable sentence that names what happened diff --git a/packages/rest/src/package-publish-status-classification.test.ts b/packages/rest/src/package-publish-status-classification.test.ts index 67444abcfd..df8993bc26 100644 --- a/packages/rest/src/package-publish-status-classification.test.ts +++ b/packages/rest/src/package-publish-status-classification.test.ts @@ -21,17 +21,24 @@ * ## Why fixing the status did not fix the message * * The dispatch's load-bearing assumption was that once this path is a 5xx, - * #8086's withhold covers it "with no new rule". Both halves of that are - * measured false here, in sections 3 and 4, because it is the reason the fix - * had to reach the producer: + * #8086's withhold covers it "with no new rule". It is measured false in §3, + * and that is why the fix had to reach the producer: the withhold is applied + * by `sendThrownError`, which a RETURNED failure never reaches — `sendError` + * carries no predicate at any status. * - * - the withhold is applied by `sendThrownError`, which a RETURNED failure - * never reaches — `sendError` has no predicate in it at any status; - * - and `looksLikeInternalErrorLeak('no such table: sys_packages')` is - * **false** — the commonest real failure of the `INSERT INTO sys_packages` - * statement names no keyword the heuristic knows. + * When this was written there was a second, independent reason: + * `looksLikeInternalErrorLeak('no such table: sys_packages')` was **false**, + * so even routed through the withhold the line would have travelled. #8132 has + * since taught the predicate that phrasing, which retires that argument and + * turns §4 from "the heuristic would miss it" into "the heuristic now catches + * it, and the returned path still never asks it". §4 carries the full note. * - * So the producer now emits a stable sentence and no driver text at all + * ⚠️ The fix is NOT redundant with #8132, and the temptation to conclude + * otherwise is exactly what §4's second case exists to refuse. Re-measured + * against the widened predicate, `main`'s producer with only the status + * corrected still answers `500 {"message":"no such table: sys_packages"}`. + * + * So the producer emits a stable sentence and no driver text at all * (`service-package/src/publish-driver-fault.test.ts` drives that with a real * SQLite engine). This file pins the door's half: the classification, and the * 4xx paths that must NOT move. @@ -305,31 +312,72 @@ describe('[#8131] the 5xx withhold does NOT cover a RETURNED failure', () => { }); // --------------------------------------------------------------------------- -// 4. …and the heuristic would have missed it anyway +// 4. The heuristic caught up — and the producer fix is required anyway // --------------------------------------------------------------------------- - -describe('[#8131] looksLikeInternalErrorLeak does not recognise this statement’s commonest failure', () => { - it('`no such table: sys_packages` is measured FALSE', () => { - // The same ceiling #8086 pinned for the Postgres phrasing, hit by SQLite - // too: the message names no `sqlite_`, no `sqlstate`, no `constraint - // failed`, and does not START with a statement keyword. So even routed - // through `sendThrownError` at 500 it would have travelled whole. - // - // ⛔ Do not "fix" this by widening the predicate — that is a phrasing arms - // race across every dialect, and #8136 rules it out explicitly. The cure - // is the producer, which is where #8131 put it. - expect(looksLikeInternalErrorLeak('no such table: sys_packages')).toBe(false); - - // Its sibling DOES trip, which is what makes the case above a real gap - // rather than a claim that the predicate never works. +// +// ## This section was INVERTED, deliberately, and is worth reading before +// ## trusting either half of it +// +// As first written, these two cases asserted `looksLikeInternalErrorLeak('no +// such table: sys_packages') === false`, and that was the truth at the time: +// the message names no `sqlite_`, no `sqlstate`, no `constraint failed`, and +// does not start with a statement keyword. It was half of why the fix had to +// reach the producer. +// +// #8132 then landed on `main` and taught the predicate the bare-SQLite and +// Postgres phrasings (`/\bno such (?:table|column):/i` and the quoted-relation +// forms). These cases went red on the merge — which is precisely the signal +// both #8086's ceiling note and this file's own instruction predicted, and the +// instruction was "delete or invert it; do not repair it to green". +// +// So they are INVERTED to the new fact rather than deleted: the knowledge that +// this phrasing is judged, and by whom, is worth keeping pinned. What is NOT +// done is quietly flipping an expectation to match reality while leaving the +// prose claiming a gap that no longer exists. +// +// ⚠️ **The load-bearing point survives #8132 untouched, and it is §3, not this +// section.** The producer fix was never redundant with a smarter predicate: a +// RETURNED failure reaches `sendError`, which consults no predicate at all. +// Re-measured against the widened predicate, `main`'s producer with only the +// status corrected to 500 still answers +// +// 500 {"code":"PACKAGE_PUBLISH_FAILED", +// "message":"no such table: sys_packages"} +// +// — the driver line still on the wire, with a predicate that recognises it +// perfectly, because nothing on that path ever asks. §3 is the pin for that, +// and it is the one that must never be weakened. + +describe('[#8131 / #8132] the predicate now judges this phrasing — and the returned path still never asks it', () => { + it('`no such table: sys_packages` is recognised as of #8132 (was FALSE when #8131 was written)', () => { + expect(looksLikeInternalErrorLeak('no such table: sys_packages')).toBe(true); + // The sibling that always tripped, kept so this reads as a statement about + // the predicate rather than about one string. expect(looksLikeInternalErrorLeak('NOT NULL constraint failed: sys_packages.tenant_ref')).toBe(true); + // #8132's other half, pinned here because #8086's ceiling note named it as + // the case that would go red when the gap closed. + expect(looksLikeInternalErrorLeak('relation "sys_packages" does not exist')).toBe(true); }); - it('this case goes red the day the predicate learns the phrasing — that is the signal', () => { - // Stated positively so the day it changes is visible, per #8086's ceiling - // note. A reader arriving because this went red should delete it, not - // repair it. - expect(looksLikeInternalErrorLeak('no such table: sys_packages')).toBe(false); + it('a predicate that knows the phrasing STILL does not reach a returned failure', () => { + // The whole point, in one case: recognition is necessary for the thrown + // path and irrelevant to this one. If someone ever concludes from the + // green above that #8132 made #8131's producer fix redundant, this is the + // case that says otherwise. + const leak = 'no such table: sys_packages'; + expect(looksLikeInternalErrorLeak(leak)).toBe(true); + + const captured: Captured = { status: 0, body: undefined }; + const res: any = { + json(d: any) { captured.body = d; }, send() {}, + status(c: number) { captured.status = c; return res; }, header() { return res; }, + }; + sendError(res, 500, 'PACKAGE_PUBLISH_FAILED', leak); + + // Verbatim, at 500, with the predicate calling it a leak. + expect(captured.status).toBe(500); + expect(captured.body?.error?.message).toBe(leak); + expect(captured.body?.error?.message).not.toBe(INTERNAL_ERROR_MESSAGE); }); }); @@ -391,13 +439,21 @@ describe('[#8131] the producer re-throws exactly what the shared rule can map', expect(resolved.status).toBe(0); expect(declaredStatus(driverError)).toBe(false); - // And had it been re-thrown, this is what the door would have answered: - // a 500 whose message is the driver line verbatim, because the heuristic - // does not recognise this phrasing. + // And had it been re-thrown, the door would have resolved it as an + // UNDECLARED server fault — a 500 whose code derives from the status, not + // from the driver's `ERR_SQLITE_ERROR`, which the ledger does not know. + // + // Note what this case no longer claims. It used to add "…and the heuristic + // does not recognise this phrasing, so the message ships"; since #8132 the + // predicate DOES recognise it, so on the thrown path the prose would now + // be withheld. That does not make re-throwing correct here: it would still + // turn a driver fault into a generic `INTERNAL_ERROR` and discard + // `PACKAGE_PUBLISH_FAILED`, and it would still leave the returned path + // (§3, §4) unprotected. The discriminant is about WHO declared the answer, + // and that is independent of the predicate. const asThrown = resolveThrownHttpError(driverError); expect(asThrown.status).toBe(500); expect(asThrown.code).toBe('INTERNAL_ERROR'); - expect(looksLikeInternalErrorLeak(asThrown.message)).toBe(false); expect(asThrown.message).toBe('no such table: sys_packages'); }); }); diff --git a/packages/services/service-package/src/index.ts b/packages/services/service-package/src/index.ts index 582459d7a8..f488fd1676 100644 --- a/packages/services/service-package/src/index.ts +++ b/packages/services/service-package/src/index.ts @@ -230,10 +230,12 @@ export class PackageServicePlugin implements Plugin { // ⚠️ This is the half that actually closes the disclosure, and it // has to be: the 5xx withhold (#8086) lives in the door's // `sendThrownError`, which a RETURNED failure never reaches at any - // status — and even reached, `looksLikeInternalErrorLeak` is - // measured FALSE for `no such table: sys_packages`, the commonest - // real failure of this very statement. Correct classification alone - // would have left the text on the wire. + // status. `sendError` consults no predicate, so correct + // classification alone leaves the text on the wire — measured + // against the POST-#8132 predicate, which recognises + // `no such table: sys_packages` perfectly and is never asked. + // A smarter heuristic does not make this redundant; nothing on this + // path calls one. return { success: false, driverFault: { message: PACKAGE_PUBLISH_DRIVER_FAULT_MESSAGE }, diff --git a/packages/services/service-package/src/publish-driver-fault.test.ts b/packages/services/service-package/src/publish-driver-fault.test.ts index 94cf0ab52f..a7d620fa58 100644 --- a/packages/services/service-package/src/publish-driver-fault.test.ts +++ b/packages/services/service-package/src/publish-driver-fault.test.ts @@ -26,23 +26,24 @@ * ## Why the PRODUCER half is the load-bearing one * * The dispatch assumed that reclassifying this path to 5xx would put it inside - * #8086's withhold "with no new rule". Measured, that is false **twice**: + * #8086's withhold "with no new rule". Measured, that is false: the withhold + * lives in the door's `sendThrownError`, and a failure that is *returned* + * reaches `sendError` directly, which consults no predicate at any status. + * Classification alone changes 400 to 500 and leaves the driver line exactly + * where it was. * - * 1. **Structurally** — the withhold lives in the door's `sendThrownError`. - * A failure that is *returned* reaches `sendError` directly and never - * meets it, at any status. Classification alone changes 400 to 500 and - * leaves the driver line exactly where it was. - * 2. **Semantically** — even routed through the withhold, - * `looksLikeInternalErrorLeak('no such table: sys_packages')` is **false** - * (it names no `sqlite_`, no `sqlstate`, no `constraint failed`, and does - * not start with a statement keyword). That is the commonest real failure - * of this very statement. Pinned at the door in - * `package-publish-status-classification.test.ts`, where the predicate - * lives. + * When this was written there was a second, independent reason — + * `looksLikeInternalErrorLeak('no such table: sys_packages')` was **false**, + * so the line would have survived the withhold even if it had been reached. + * #8132 has since taught the predicate that phrasing, retiring that half of + * the argument. It changes nothing here: re-measured against the widened + * predicate, the classification-only counterfactual still answers + * `500 {"message":"no such table: sys_packages"}`, because nothing on the + * returned path asks. The door's suite carries that case. * * So the disclosure is closed HERE, at the producer, where no heuristic is * involved and no dialect's phrasing has to be recognised — option C of #8086, - * for this producer. + * for this producer, and the reason it does not depend on #8132 holding. * * ## The driver is real on purpose *