diff --git a/.changeset/tidy-pandas-repeat.md b/.changeset/tidy-pandas-repeat.md new file mode 100644 index 0000000000..c3eac689c1 --- /dev/null +++ b/.changeset/tidy-pandas-repeat.md @@ -0,0 +1,23 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +`auditMetaItem` no longer reports a failed audit read as an empty audit trail + +The `catch` closing the audit read in `ObjectStackProtocolImplementation.auditMetaItem` +was unqualified. Its comment named two benign causes — the `sys_metadata_audit` table not +being provisioned (legacy environments) and a host engine that exposes no `find` — but the +clause took every other cause with them: a connection drop, a permission denial, a +timeout, a malformed row, a query bug. Each was reported to the caller as the well-formed +statement `{ events: [] }`, i.e. "this item has no audit entries". + +This is the compliance surface behind `GET /api/v1/meta/:type/:name/audit`, which exists +so Studio's audit-log tab can show who tried what and whether a lock blocked it, so an +empty answer reads as *nobody touched this item*. Because the swallowed failures are +transient, the same item could report a full trail one minute and a clean one the next. + +Both benign causes still answer `{ events: [] }` exactly as documented. Every other read +failure now raises `SERVICE_UNAVAILABLE` / 503 carrying the driver error as `cause`, which +the route's existing error handler turns into an honest 5xx — the same treatment the +sibling `listCommits` and `getMetaItem` reads in this package already give (ADR-0110 D3: a +miss and a fault are different facts). diff --git a/packages/metadata-protocol/src/protocol.audit-read-failure-propagation.test.ts b/packages/metadata-protocol/src/protocol.audit-read-failure-propagation.test.ts new file mode 100644 index 0000000000..727b47f498 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.audit-read-failure-propagation.test.ts @@ -0,0 +1,220 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #9638 — `auditMetaItem`'s unqualified `catch` reported ANY failed audit read +// as `{ events: [] }`. +// +// The catch named two benign causes in its comment ("table not provisioned +// (legacy env) or driver doesn't expose `find`") and then took every OTHER +// cause with them: a connection drop, a permission denial, a malformed row, a +// query bug, a timeout. All of them reached the caller as the well-formed +// statement "this item has no audit entries". +// +// ADR-0110 D3 — a miss and a fault are different facts. This is the compliance +// surface: `auditMetaItem` is the read behind +// `GET /api/v1/meta/:type/:name/audit`, which exists so Studio's 审计日志 tab +// can show who tried what and whether a lock blocked it. An empty answer there +// reads as *nobody touched this item*. +// +// This file pins BOTH directions, because a method that raised unconditionally +// would satisfy the first half and destroy the documented feature: +// +// • a NON-benign failure now propagates as 503 / SERVICE_UNAVAILABLE, which +// the `/audit` route's existing `handleRouteError` turns into an honest +// 5xx (it reads `error.status`, `packages/rest/src/error-response.ts`); +// • BOTH benign causes still answer `{ events: [] }`, verbatim. +// +// ⚠️ Anti-vacuity. An "it propagates" assertion is worthless if the assertions +// cannot see the difference between a populated trail and an empty one in the +// first place — this repo has been bitten by exactly that shape +// (`body.item.fields` vs `body.data.item.fields`). The POSITIVE CONTROL below +// reads a real row all the way through the mapping and asserts its fields, so +// every "empty" assertion in this file is known to be a measurement rather +// than a shape that could never have been non-empty. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +/** A protocol whose engine read fails with `error`. */ +function protocolWhoseReadFails(error: unknown) { + const find = vi.fn(async () => { throw error; }); + const engine = { registry: { getObject: () => undefined }, find }; + return { p: new ObjectStackProtocolImplementation(engine as any), find }; +} + +/** A protocol whose engine read succeeds, returning `rows`. */ +function protocolReading(rows: any[]) { + const find = vi.fn(async () => rows); + const engine = { registry: { getObject: () => undefined }, find }; + return { p: new ObjectStackProtocolImplementation(engine as any), find }; +} + +const ITEM = { type: 'views', name: 'shared_grid' } as const; + +/** + * Capture the rejection, or fail loudly naming what was RESOLVED instead. + * + * Deliberately not a bare `.rejects.toThrow()`: that cannot separate "answered + * with the wrong body" from "did not raise at all", and the wrong body — a + * well-formed empty trail — *is* the defect. It also would not print the + * `{ events: [] }` that makes a failure here self-explanatory. + */ +async function rejectionOf(promise: Promise): Promise { + let resolved: unknown; + try { + resolved = await promise; + } catch (error) { + return error; + } + throw new Error( + `expected the read failure to propagate, but it RESOLVED with ` + + `${JSON.stringify(resolved)} — the defect: a fault disguised as an empty audit trail`, + ); +} + +describe('#9638 auditMetaItem: a failed audit read is a fault, not an empty trail', () => { + // ── The propagating half ──────────────────────────────────────────────── + // + // Three flavours, because the old catch was unqualified and each of these + // means "the rows may well exist and simply were not seen". + const nonBenign: Array<[string, Error]> = [ + ['a connection drop', new Error('connect ECONNREFUSED 127.0.0.1:5432')], + ['a permission denial', new Error('permission denied for table sys_metadata_audit')], + ['a timeout', new Error('query timeout after 30000ms')], + ]; + + it.each(nonBenign)( + '⭐ THE PIN — %s propagates as 503 SERVICE_UNAVAILABLE instead of `{ events: [] }`', + async (_label, driverError) => { + const { p } = protocolWhoseReadFails(driverError); + + const error = await rejectionOf(p.auditMetaItem({ ...ITEM })); + + // ADR-0112: `code` AND `status` together. `status` alone would pass + // for any 5xx and `code` alone carries no HTTP verdict, and it is + // the PAIR the REST boundary reads. + expect(error.code).toBe('SERVICE_UNAVAILABLE'); + expect(error.status).toBe(503); + }, + ); + + it('the driver error rides as `cause`, so the operator still sees what actually broke', async () => { + const driverError = new Error('connect ECONNREFUSED 127.0.0.1:5432'); + const { p } = protocolWhoseReadFails(driverError); + + const error = await rejectionOf(p.auditMetaItem({ ...ITEM })); + + // Not the driver error itself: unwrapped it has no `status`, so the + // REST boundary would have to guess from message text — and + // `mapDataError` guesses `no such table` back into a 404 miss. + expect(error.cause).toBe(driverError); + }); + + it('503 is a status `handleRouteError` turns into a 5xx — not a 2xx and not a client error', async () => { + const { p } = protocolWhoseReadFails(new Error('query timeout after 30000ms')); + + const error = await rejectionOf(p.auditMetaItem({ ...ITEM })); + + // The route's catch passes this straight to `handleRouteError`, which + // reads `error.status` in the 400-599 band. Pinning the band is what + // makes "an honest 5xx" a checkable claim at this layer. + expect(error.status).toBeGreaterThanOrEqual(500); + expect(error.status).toBeLessThan(600); + }); + + // ── The benign half — both causes the old comment named ───────────────── + + it.each([ + ['sqlite', 'no such table: sys_metadata_audit'], + ['sqlite, driver-prefixed', 'SQLITE_ERROR: no such table: sys_metadata_audit'], + ['postgres', 'relation "sys_metadata_audit" does not exist'], + ['mysql', "Table 'db.sys_metadata_audit' doesn't exist"], + ])( + 'BENIGN 1/2 — an unprovisioned table (%s) still answers `{ events: [] }`', + async (_dialect, message) => { + const { p } = protocolWhoseReadFails(new Error(message)); + + // The documented promise, kept exactly as documented: a legacy + // install prior to ADR-0010 has genuinely no rows, so the empty + // answer IS the truth and a first boot must not explode. + await expect(p.auditMetaItem({ ...ITEM })).resolves.toEqual({ events: [] }); + }, + ); + + it('BENIGN 2/2 — a host engine exposing no `find` still answers `{ events: [] }`', async () => { + // `MetadataHostEngine` carries `[key: string]: any`, so a metadata-only + // store or a partial double with no `find` satisfies the type. + const engine = { registry: { getObject: () => undefined } }; + const p = new ObjectStackProtocolImplementation(engine as any); + + await expect(p.auditMetaItem({ ...ITEM })).resolves.toEqual({ events: [] }); + }); + + it('the missing-`find` answer is decided BEFORE the read, not by classifying a TypeError', async () => { + // Why this matters: a missing method raises `TypeError: … is not a + // function`, and the ONLY thing separating that from a genuine + // TypeError raised INSIDE a real driver's `find` (a null deref on a + // malformed row — an actual fault) is the V8 message text. Classifying + // it in the catch would re-open the fail-open this card closes. So the + // capability is asked as a precondition, and a driver that DOES have + // `find` and throws a TypeError is a fault. + const { p } = protocolWhoseReadFails( + new TypeError("Cannot read properties of undefined (reading 'occurred_at')"), + ); + + const error = await rejectionOf(p.auditMetaItem({ ...ITEM })); + + expect(error.code).toBe('SERVICE_UNAVAILABLE'); + expect(error.status).toBe(503); + }); + + // ── Anti-vacuity ──────────────────────────────────────────────────────── + + it('POSITIVE CONTROL — a real row maps through, so "empty" above is a measurement', async () => { + const { p, find } = protocolReading([{ + id: 'evt_1', + occurred_at: '2026-08-18T10:00:00.000Z', + actor: 'alice', + source: 'studio', + operation: 'save', + outcome: 'denied', + code: 'METADATA_LOCKED', + lock_state: 'locked', + lock_overridden: false, + request_id: 'req_7', + note: 'blocked by package lock', + }]); + + const result = await p.auditMetaItem({ ...ITEM }); + + // If this file's assertions could not tell a populated trail from an + // empty one, THIS is the case that would fail — which is exactly why + // it is here rather than assumed. + expect(result.events).toHaveLength(1); + expect(result.events[0]).toMatchObject({ + id: 'evt_1', + actor: 'alice', + operation: 'save', + outcome: 'denied', + code: 'METADATA_LOCKED', + lockState: 'locked', + lockOverridden: false, + requestId: 'req_7', + note: 'blocked by package lock', + }); + expect(find).toHaveBeenCalledTimes(1); + }); + + it('⭐ a genuine zero-row read and a FAULT are no longer the same answer', async () => { + // The equivalence the defect created, stated as one assertion. A read + // that succeeded and found nothing is the empty trail; a read that + // failed is not an answer at all. + const { p: readEmpty } = protocolReading([]); + await expect(readEmpty.auditMetaItem({ ...ITEM })).resolves.toEqual({ events: [] }); + + const { p: readBroke } = protocolWhoseReadFails( + new Error('connect ECONNREFUSED 127.0.0.1:5432'), + ); + const error = await rejectionOf(readBroke.auditMetaItem({ ...ITEM })); + expect(error.status).toBe(503); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7a0c4673b8..08dc03bd09 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -6334,6 +6334,21 @@ export class ObjectStackProtocolImplementation implements * prior to ADR-0010) the call returns `{ events: [] }` instead of * raising, keeping the Studio tab harmless. * + * [#9638] `{ events: [] }` means "the audit trail was read and this item + * has no entries" and NOTHING else. Exactly two causes answer it without a + * read: the unprovisioned table above, and a host engine that exposes no + * `find` (the `typeof` probe before the read). Every other failure — a + * connection drop, a permission denial, a timeout, a malformed row, a query + * bug — RAISES, because the rows may well exist and simply were not seen, + * and a compliance reader must never be handed "nobody touched this item" + * on those terms (ADR-0110 D3). The unqualified `catch` that used to report + * all of them as an empty trail is what this closes. + * + * @throws {@link metadataStoreUnavailableError} — a 503 carrying the driver + * error as `cause`, for every read failure that is not an + * unprovisioned table. The `/audit` route's existing + * `handleRouteError` turns it into an honest 5xx. + * * `organizationId` SCOPES the read and is enforced in the query below: * rows for that organization plus env-wide (`organization_id IS NULL`) * rows, and nothing else. Omitted (or `null`) reads the env-wide rows @@ -6437,6 +6452,35 @@ export class ObjectStackProtocolImplementation implements // organization reads exactly the env-wide rows an org-less write // produces. Fail-closed, and symmetric with the write path. const organizationId = request.organizationId ?? null; + // [#9638] The FIRST of the two benign causes the catch below used to + // name, asked as a PRECONDITION rather than as an error shape. + // + // `MetadataHostEngine` carries `[key: string]: any`, so a metadata-only + // store or a partial test double with no `find` satisfies the type and + // reaches here. That is a real, documented deployment shape and it must + // keep answering `{ events: [] }`. + // + // ⚠️ Asked HERE, before the `try`, because it cannot be asked soundly + // INSIDE the catch. Measured: a missing method raises + // `TypeError: this.engine.find is not a function`, which + // `isMissingTableError` correctly reports as NOT benign — but the only + // signal separating it from a genuine `TypeError` raised *inside* a + // real driver's `find` (a malformed row, a null deref — actual faults) + // is the V8 message text. Sniffing that text would re-open exactly the + // fail-open this card closes, one error class narrower. A `typeof` + // probe is a fact about the engine, not a guess about an error, so it + // cannot misclassify a fault as a capability gap. + // + // Same shape as the sibling limb one layer up: the `/audit` route's own + // capability probe (`typeof p.auditMetaItem !== 'function'`, #9426) + // likewise decides BEFORE the call rather than classifying its failure. + if (typeof (this.engine as { find?: unknown }).find !== 'function') { + console.warn( + `[Protocol] auditMetaItem: host engine exposes no \`find\`; ` + + `reporting no audit entries for ${request.type}/${request.name}`, + ); + return { events: [] }; + } try { // Org-scoped lookup: include rows for the specific org AND // env-wide (organization_id IS NULL) rows so the editor @@ -6496,8 +6540,31 @@ export class ObjectStackProtocolImplementation implements })); return { events }; } catch (err: any) { - // Table not provisioned (legacy env) or driver doesn't - // expose `find` — return empty rather than 500ing the tab. + // [#9638] Benign (the table has not been provisioned in this legacy + // env) falls through to the empty answer; everything else is a read + // that DID NOT HAPPEN and leaves as a 503. Byte-for-byte the shape + // the sibling {@link listCommits} carries (#5980), and the same + // {@link isMissingTableError} predicate `DatabaseLoader` (#5108) and + // `SysMetadataRepository` (#4867) ask — a driver quirk is taught to + // the platform once rather than re-spelled per seam. + // + // This `catch` used to be UNQUALIFIED. A connection drop, a + // permission denial, a malformed row, a query bug or a timeout was + // reported to the caller as the well-formed statement "this item has + // no audit entries" — ADR-0110 D3 broken (a miss and a fault are + // different facts) on the COMPLIANCE surface. `auditMetaItem` is the + // read behind `GET /api/v1/meta/:type/:name/audit`, which exists so + // Studio's 审计日志 tab can show who tried what and whether a lock + // blocked it; an empty answer there reads as *nobody touched this + // item*. Worse than the static capability gap #9426 fixed one layer + // up, because a transient read failure makes the same item report a + // full trail one minute and a clean one the next. The `console.warn` + // below is on the SERVER; it was never an answer to the reader. + // + // The second cause the old comment named — a host engine with no + // `find` — is decided by the precondition probe above the `try`, so + // it never reaches here and this arm has exactly ONE benign cause. + this.rethrowUnlessMetadataStoreUnprovisioned(err); console.warn( `[Protocol] auditMetaItem read failed for ${request.type}/${request.name}: ${err?.message ?? err}`, );