diff --git a/.changeset/publish-denial-audit-survives-batch-rollback.md b/.changeset/publish-denial-audit-survives-batch-rollback.md new file mode 100644 index 0000000000..849c2c996f --- /dev/null +++ b/.changeset/publish-denial-audit-survives-batch-rollback.md @@ -0,0 +1,48 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): a package publish refused by a lock or a 409 now leaves its own audit row (#8594) + +`publishPackageDrafts` — Studio's "publish whole app" — promotes every draft +inside ONE `engine.transaction()` (ADR-0067 D2, "a commit cannot half-land"). +`promoteDraftForPublish` runs inside that closure, and it used to write its +**denial** audit rows there: the ADR-0010 lock refusal (`code: 'item_locked'`, +with the `lock_state` column) and the optimistic-lock 409 +(`code: 'metadata_conflict'`). + +On a transactional engine both rolled back with the batch. The refusal is what +aborted the batch, so the row describing that refusal was destroyed by the very +rollback it caused — the defect #7748 exists to close, surviving on this one +route. A compliance query filtering `code = 'item_locked'` found **nothing** for +a package publish refused by a lock, however many times it had been attempted. + +The `batch_aborted` row added in #8400 gave a refused batch *a* trail, but it +carries the batch's fact ("the whole batch rolled back; nothing landed"), not the +item-level verdict's vocabulary or its lock column — so the query above still +came back empty. + +**What changed.** `promoteDraftForPublish` no longer writes those rows. It hands +each refusal its own row as data, and each of its two callers records it on its +own side of its own transaction: + +- `publishMetaItem` (single-item) records it where it always effectively landed — + that route opens no transaction of its own, and its rows are unchanged, still + filed under `source: 'protocol.publishMetaItem'`; +- `publishPackageDrafts` (batch) records it from the rollback handler, outside + the transaction, filed under `source: 'protocol.publishPackageDrafts'`. + +The placement no longer depends on the engine's capabilities either: an engine +with no `transaction()` at all lands the same row in the same place. + +**What a refused batch now leaves.** Two rows for the causal item, each carrying +a different fact and neither replacing the other: the inner verdict +(`item_locked` with its `lock_state`, or `metadata_conflict` naming the losing +race) and #8400's `batch_aborted`. A refusal that never reached either gate — a +driver fault, `NOT_OVERRIDABLE`, `INVALID_METADATA` — still leaves exactly the +one `batch_aborted` row it left before; no code value is minted for it. + +No new `code` value: ADR-0112 D6b keeps `sys_metadata_audit.code` a closed +persisted vocabulary, and the values that now land are the ones that were already +in it. Nothing about ADR-0067 D2 changes — a refused batch still promotes +nothing, records no commit, and reports `publishedCount: 0`. diff --git a/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts b/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts index 42ac2e13cd..7c9e3c447f 100644 --- a/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts +++ b/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts @@ -73,6 +73,25 @@ // Reverse verification, direction predicted BEFORE running // --------------------------------------------------------------------------- // See the PR body for prediction vs. measurement. +// +// --------------------------------------------------------------------------- +// [#8594] One assertion in here was MEASURING the defect, and was inverted +// --------------------------------------------------------------------------- +// The locked-item case above ended on +// +// expect(h.auditRows.some((a) => a.code === 'item_locked')).toBe(false); +// +// which pinned the fact that the INNER verdict's row was destroyed by the +// rollback its own refusal caused — a true statement about `main` on the day it +// was written, and the defect #8594 exists to close, not a contract. It now +// reads `.toBe(true)`, deliberately, and the second `describe` at the bottom of +// this file asserts that row in full (its `code`, its `lock_state`, its route), +// plus the `metadata_conflict` sibling that had no coverage at all. +// +// An inversion on its own would be satisfied by a "fix" that stopped rolling +// anything back, so every case that asserts a surviving denial also asserts +// ADR-0067 D2 in the same breath — nothing promoted, no commit row, no +// `allowed` row. See that block's header. import { describe, expect, it, vi } from 'vitest'; // [#5619] The producer's OWN write-verb dispatch decisions, imported from @@ -140,8 +159,16 @@ type Harness = { * Arm a driver fault on every `sys_metadata` write for this item name. * Mutable so a test can stage its drafts through a healthy engine and break * it only for the batch under test. + * + * [#8594] `advanceActiveOnRead` arms an OPTIMISTIC-LOCK race instead of a + * driver fault: a rival author advancing the active row in the window + * between the two reads `promoteDraft` makes (`get` for the parent version, + * then `put`'s own read inside its transaction), which is exactly the + * 409 `METADATA_CONFLICT` the repository raises. Scoped to package-scoped + * active reads so the pre-flight `commitItems` scan — which reads active + * rows WITHOUT a `package_id` — does not consume the one-shot. */ - faults: { failMetadataWriteFor?: string }; + faults: { failMetadataWriteFor?: string; advanceActiveOnRead?: string }; /** Audit rows that LANDED and were not rolled back. */ auditRows: any[]; /** @@ -153,7 +180,7 @@ type Harness = { }; function makeStubEngine(opts: { failAudit?: boolean } = {}): Harness { - const faults: { failMetadataWriteFor?: string } = {}; + const faults: { failMetadataWriteFor?: string; advanceActiveOnRead?: string } = {}; const rows = new Map(); const historyRows: HistoryRow[] = []; const commitRows: any[] = []; @@ -196,7 +223,23 @@ function makeStubEngine(opts: { failAudit?: boolean } = {}): Harness { return historyRows.find((h) => matchesHistory(h, opts2.where)) ?? null; } if (table === 'sys_metadata_commit') return null; - return findRow(opts2.where)?.row ?? null; + const hit = findRow(opts2.where); + if (!hit) return null; + // [#8594] The rival author lands BETWEEN the two reads. THIS read is + // answered with the row as it stood (the snapshot below), and the + // stored row is advanced immediately after — so the next read, the + // one `put` makes inside its own transaction, sees a different + // checksum, the parent-version check fails, and the repository + // raises the ConflictError that really produces `metadata_conflict`. + if (faults.advanceActiveOnRead === hit.row.name + && opts2.where?.state === 'active' + && 'package_id' in (opts2.where ?? {})) { + delete faults.advanceActiveOnRead; + const asItStood = { ...hit.row }; + hit.row.checksum = 'sha256:advanced_by_a_rival'; + return asItStood; + } + return hit.row; }, async find(table: string, opts2: { where: Record; orderBy?: any; limit?: number }) { if (table === 'sys_metadata_audit') { @@ -533,16 +576,23 @@ describe('[#8400] publishPackageDrafts audits the batch it publishes', () => { } as any); expect(res.publishedCount).toBe(0); + // Membership, not length: [#8594] a lock refusal now leaves BOTH the + // batch row and the inner verdict's own row, and the scope claim under + // test here is about each row's `organization_id`, not about how many + // rows one refusal produces. const denied = publishRows(h, 'denied'); - expect(denied).toHaveLength(1); - expect(denied[0].name).toBe('envwide_locked'); - // The denied row reads its scope from `__batchItem`, which is the - // `listDrafts` row — the draft's OWN scope, the same source the allowed - // row's `draftOrgId` comes from. Keyed on the caller's active org this - // would be `ORG` and the two outcomes would disagree about where the - // publish was refused. - expect(denied[0].organization_id).toBeNull(); - expect(denied[0].organization_id).not.toBe(ORG); + expect(denied.map((a) => a.code).sort()).toEqual(['batch_aborted', 'item_locked']); + for (const row of denied) { + expect(row.name).toBe('envwide_locked'); + // The denied rows read their scope from the draft's OWN scope — the + // `listDrafts` row (`__batchItem` for the batch row, the promote + // request for the inner one), the same source the allowed row's + // `draftOrgId` comes from. Keyed on the caller's active org these + // would be `ORG` and the two outcomes would disagree about where the + // publish was refused. + expect(row.organization_id).toBeNull(); + expect(row.organization_id).not.toBe(ORG); + } }); // ── the denied outcome, and the placement that makes it durable ────────── @@ -586,12 +636,16 @@ describe('[#8400] publishPackageDrafts audits the batch it publishes', () => { // allowed rows are driven off a COMMITTED batch, not an attempted one. expect(publishRows(h, 'allowed')).toHaveLength(0); - // Exactly ONE denial survives, and it is the one written outside the - // transaction. + // The batch-level row survives, and it is the one written outside the + // transaction. Asserted by MEMBERSHIP on its `code`, never by counting + // the list: #8594 adds a SECOND denial beside it (the inner verdict's + // own row, below), and a length assertion here would have read that + // addition as a regression instead of as the fix. const denied = publishRows(h, 'denied'); - expect(denied).toHaveLength(1); - expect(h.auditRows).toHaveLength(auditedBefore + 1); - expect(denied[0]).toMatchObject({ + // adr0112-ok: D6b — persisted audit column, its own vocabulary + const batchAborted = denied.filter((a) => a.code === 'batch_aborted'); + expect(batchAborted).toHaveLength(1); + expect(batchAborted[0]).toMatchObject({ type: 'view', name: 'locked_grid', organization_id: ORG, @@ -604,15 +658,26 @@ describe('[#8400] publishPackageDrafts audits the batch it publishes', () => { actor: 'admin', source: 'protocol.publishPackageDrafts', }); - expect(String(denied[0].note)).toContain(PKG); + expect(String(batchAborted[0].note)).toContain(PKG); + expect(auditedBefore).toBeGreaterThan(0); // the staging saves really ran - // THE PLACEMENT MEASUREMENT. `assertLockAllowsWrite` wrote its - // `item_locked` row from inside the transaction: it was ATTEMPTED… + // ── THE PLACEMENT MEASUREMENT ──────────────────────────────────────── + // The lock gate's `item_locked` row was ATTEMPTED… expect(h.auditAttempts.some((a) => a.code === 'item_locked')).toBe(true); - // …and it is GONE, rolled back with the batch it was recording. A - // refusal audited from in there leaves nothing behind — which is why - // the row above is written from the `catch` instead. - expect(h.auditRows.some((a) => a.code === 'item_locked')).toBe(false); + // …and [#8594] it now SURVIVES. + // + // ⚠️ THIS ASSERTION WAS INVERTED, deliberately. It read `.toBe(false)` + // and that was this file MEASURING the defect rather than fixing it: + // `assertLockAllowsWrite` wrote the row from INSIDE the batch + // transaction, so the refusal's own record was destroyed by the very + // rollback the refusal caused, and a compliance query on + // `code = 'item_locked'` found nothing for a package publish refused by + // a lock. `batch_aborted` above gave the batch route *a* trail but not + // the inner verdict's vocabulary. `promoteDraftForPublish` now hands the + // row to its caller instead of writing it, and the caller records it + // outside the transaction — so both rows survive, each carrying its own + // fact. The inner one is asserted in full in the `[#8594]` block below. + expect(h.auditRows.some((a) => a.code === 'item_locked')).toBe(true); }); it('the denied row quotes the refusal but never the driver dialect (the note is wire-visible)', async () => { @@ -632,6 +697,12 @@ describe('[#8400] publishPackageDrafts audits the batch it publishes', () => { } as any); expect(res.publishedCount).toBe(0); + // [#8594] Exactly ONE row here, and the length assertion is the + // point: a driver fault carries NO inner verdict, so nothing may + // appear beside `batch_aborted`. This is the over-broad guard for + // #8594's shape — a fix that minted an inner-vocabulary row for + // every refusal, instead of replaying only the ones the gates really + // reached, would put a second row here and go red. const denied = publishRows(h, 'denied'); expect(denied).toHaveLength(1); expect(denied[0]).toMatchObject({ @@ -683,3 +754,279 @@ describe('[#8400] publishPackageDrafts audits the batch it publishes', () => { } }); }); + +/** + * [#8594] The INNER verdict's own row survives the rollback it caused. + * + * --------------------------------------------------------------------------- + * What #8400 left, and why it was not enough + * --------------------------------------------------------------------------- + * #8400 gave a refused batch a `batch_aborted` row written from the `catch`, + * outside the transaction. That is a real trail, but it is the BATCH's fact. + * The item-level verdicts — `assertLockAllowsWrite`'s `item_locked` (with its + * `lock_state` column) and `recordOptimisticConflictAudit`'s `metadata_conflict` + * — were still written from INSIDE `promoteDraftForPublish`, i.e. inside the + * batch transaction, and were destroyed by the very rollback their own refusal + * caused. A compliance query filtering `code = 'item_locked'` therefore found + * nothing for a package publish refused by a lock: the vocabulary that query + * runs on never reached the table on this route. + * + * --------------------------------------------------------------------------- + * The shape, and the premise it rests on (measured, not assumed) + * --------------------------------------------------------------------------- + * `promoteDraftForPublish` no longer writes those rows. It attaches them to the + * refusal it throws, and each of its two callers records them on ITS side of + * ITS transaction. The whole argument for that shape is a claim about the + * single-item route — that it already audits outside a transaction — so the + * first case below MEASURES it rather than taking it on faith, against the same + * really-rolling-back harness. That case is green before AND after the + * production change: it is the premise, not the deliverable. + * + * --------------------------------------------------------------------------- + * The control that keeps the inversion honest + * --------------------------------------------------------------------------- + * "The denial rows survive" is also satisfied by a fix that simply stopped + * rolling anything back — the trap a sibling inversion fell into. So every case + * here that asserts a surviving denial ALSO asserts ADR-0067 D2 in the same + * breath: the batch promoted NOTHING (`publishedCount: 0`, every draft still a + * draft with no active row, no `sys_metadata_commit` row, no `allowed` row). + * If the transaction stopped rolling back, those go red while the denial + * assertions stay green — which is the whole point of asserting both. + */ +describe('[#8594] a refused publish leaves the INNER verdict, in its own vocabulary', () => { + /** Every audit row for `code`, whatever the operation. */ + const byCode = (h: Harness, code: string) => h.auditRows.filter((a) => a.code === code); + + // ── the premise ────────────────────────────────────────────────────────── + // The single-item route's denial row lands and STAYS on the same harness + // whose `transaction()` really rolls back — because `publishMetaItem` opens + // no transaction of its own. That is the fact the whole "hand the row to the + // caller" shape is built on, so it is pinned here instead of assumed. + it('premise: the SINGLE-ITEM publish route audits outside a transaction — its `item_locked` row survives', async () => { + const h = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(h.engine, undefined, 'env_test'); + + await stageDraft(protocol, 'solo_locked'); + await protocol.saveMetaItem({ + type: 'view', name: 'solo_locked', organizationId: ORG, + item: viewBody('solo_locked', 'protected', { _lock: 'no-overlay' }), + packageId: PKG, actor: 'admin', + } as any); + + let caught: any; + try { + await protocol.publishMetaItem({ + type: 'view', name: 'solo_locked', organizationId: ORG, actor: 'admin', + } as any); + } catch (e) { caught = e; } + + // ADR-0112 envelope: `code` AND `status`, never a bare `toThrow()`. + expect(caught?.code).toBe('ITEM_LOCKED'); + expect(caught?.status).toBe(403); + + const locked = byCode(h, 'item_locked'); + expect(locked).toHaveLength(1); + expect(locked[0]).toMatchObject({ + type: 'view', + name: 'solo_locked', + organization_id: ORG, + operation: 'publish', + outcome: 'denied', + // adr0112-ok: D6b — persisted audit column, its own vocabulary + code: 'item_locked', + lock_state: 'no-overlay', + actor: 'admin', + // The single-item route keeps naming itself — unchanged by #8594. + source: 'protocol.publishMetaItem', + }); + }); + + // ── the deliverable: the lock refusal ─────────────────────────────────── + it('a batch refused by a LOCK leaves an `item_locked` row with its `lock_state` — and still promotes nothing', async () => { + const h = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(h.engine, undefined, 'env_test'); + + await stageDraft(protocol, 'case_grid'); + await stageDraft(protocol, 'locked_grid'); + await protocol.saveMetaItem({ + type: 'view', name: 'locked_grid', organizationId: ORG, + item: viewBody('locked_grid', 'protected', { _lock: 'no-overlay' }), + packageId: PKG, actor: 'admin', + } as any); + + const res = await protocol.publishPackageDrafts({ + packageId: PKG, organizationId: ORG, actor: 'admin', + } as any); + + // ── THE DELIVERABLE ────────────────────────────────────────────────── + // The verdict that aborted the batch, in the vocabulary a compliance + // query filters on, with the lock column that says WHICH lock. + const locked = byCode(h, 'item_locked'); + expect(locked).toHaveLength(1); + expect(locked[0]).toMatchObject({ + type: 'view', + name: 'locked_grid', + organization_id: ORG, + operation: 'publish', + outcome: 'denied', + // adr0112-ok: D6b — persisted audit column, its own vocabulary + code: 'item_locked', + lock_state: 'no-overlay', + actor: 'admin', + // Re-stamped by the route that wrote it: the shared Phase-1 helper + // passes `protocol.publishMetaItem` for both of its callers, and + // filing a batch publish under the single-item route's name would + // make the trail lie about which door was used. + source: 'protocol.publishPackageDrafts', + }); + + // …beside #8400's batch-level row, which records a different fact and is + // NOT replaced. Membership on the pair, never a count of the list. + expect(publishRows(h, 'denied').map((a) => a.code).sort()) + .toEqual(['batch_aborted', 'item_locked']); + + // ── THE ADR-0067 D2 CONTROL ────────────────────────────────────────── + // Rows surviving is only the fix if the rollback still happens. A fix + // that made the denial durable by no longer rolling anything back would + // satisfy every assertion above and fail every one below. + expect(res.success).toBe(false); + expect(res.publishedCount).toBe(0); + expect(res.published).toEqual([]); + // `case_grid` was promoted BEFORE `locked_grid` refused, and it unwound: + // still a draft, with no active row anywhere. + expect([...h.rows.values()].filter((r) => r.name === 'case_grid').map((r) => r.state)) + .toEqual(['draft']); + expect([...h.rows.values()].some((r) => r.state === 'active' && r.name === 'case_grid')) + .toBe(false); + // The ADR-0067 commit row is written inside the same transaction, so it + // unwound too — a recorded commit can never describe a partial publish. + expect(h.commitRows).toHaveLength(0); + // And no item was audited as published. + expect(publishRows(h, 'allowed')).toHaveLength(0); + }); + + // ── the read door ─────────────────────────────────────────────────────── + // A row nobody can query is not a trail. `GET /api/v1/meta/:type/:name/audit` + // serves from `auditMetaItem`, and that is where a compliance report reads + // `code` / `lockState` from. + it('auditMetaItem surfaces the `item_locked` verdict for a batch-refused item', async () => { + const h = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(h.engine, undefined, 'env_test'); + + await stageDraft(protocol, 'locked_grid'); + await protocol.saveMetaItem({ + type: 'view', name: 'locked_grid', organizationId: ORG, + item: viewBody('locked_grid', 'protected', { _lock: 'no-overlay' }), + packageId: PKG, actor: 'admin', + } as any); + await protocol.publishPackageDrafts({ + packageId: PKG, organizationId: ORG, actor: 'admin', + } as any); + + const { events } = await protocol.auditMetaItem({ type: 'view', name: 'locked_grid' }); + const verdict = events.find((e) => e.code === 'item_locked'); + expect(verdict).toBeDefined(); + expect(verdict).toMatchObject({ + operation: 'publish', + outcome: 'denied', + lockState: 'no-overlay', + source: 'protocol.publishPackageDrafts', + }); + }); + + // ── the deliverable: the optimistic-lock refusal ───────────────────────── + // The card's other inner verdict. `recordOptimisticConflictAudit` sat in the + // same place with the same fate, and no case anywhere measured it on the + // batch route: the 409 is raised by the repository's parent-version check, + // which needs a real rival write between `promoteDraft`'s two reads (see + // `advanceActiveOnRead`), not an injected error. + it('a batch refused by a 409 CONFLICT leaves a `metadata_conflict` row — and still promotes nothing', async () => { + const h = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(h.engine); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + // Staged FIRST so it is promoted first and is genuinely unwound by + // the conflict that follows it — the ADR-0067 D2 control below is + // about a promotion that really happened, not one never attempted. + await stageDraft(protocol, 'bystander_grid'); + await stageDraft(protocol, 'raced_grid'); + // An active row for the draft to advance PAST — with no active row + // the parent version is null on both reads and there is no race. + await protocol.saveMetaItem({ + type: 'view', name: 'raced_grid', organizationId: ORG, + item: viewBody('raced_grid', 'head'), packageId: PKG, actor: 'admin', + } as any); + + // The rival lands between the two reads of `raced_grid`'s active row. + h.faults.advanceActiveOnRead = 'raced_grid'; + + const res = await protocol.publishPackageDrafts({ + packageId: PKG, organizationId: ORG, actor: 'admin', + } as any); + + // ── THE DELIVERABLE ────────────────────────────────────────────── + const conflict = byCode(h, 'metadata_conflict'); + expect(conflict).toHaveLength(1); + expect(conflict[0]).toMatchObject({ + type: 'view', + name: 'raced_grid', + organization_id: ORG, + operation: 'publish', + outcome: 'denied', + // adr0112-ok: D6b — persisted audit column, its own vocabulary + code: 'metadata_conflict', + actor: 'admin', + source: 'protocol.publishPackageDrafts', + }); + // The note names the losing race, which is the fact an author needs. + expect(String(conflict[0].note)).toContain('sha256:advanced_by_a_rival'); + expect(publishRows(h, 'denied').map((a) => a.code).sort()) + .toEqual(['batch_aborted', 'metadata_conflict']); + // The batch reports the refusal in its own envelope too. + expect(res.failed.some((f) => f.code === 'METADATA_CONFLICT')).toBe(true); + + // ── THE ADR-0067 D2 CONTROL ────────────────────────────────────── + expect(res.success).toBe(false); + expect(res.publishedCount).toBe(0); + expect(h.commitRows).toHaveLength(0); + expect(publishRows(h, 'allowed')).toHaveLength(0); + // The bystander draft that promoted before the conflict unwound. + expect([...h.rows.values()].filter((r) => r.name === 'bystander_grid').map((r) => r.state)) + .toEqual(['draft']); + } finally { + warn.mockRestore(); + } + }); + + // ── the engine without a transaction ───────────────────────────────────── + // `publishPackageDrafts` falls through to a plain sequential run when the + // engine has no `transaction()` (memory driver, minimal stubs). The row must + // land there too: after #8594 the placement no longer depends on the + // engine's capabilities, which is the reason the helper stopped guessing. + it('an engine with NO transaction() still lands the `item_locked` row', async () => { + const h = makeStubEngine(); + delete h.engine.transaction; + const protocol = new ObjectStackProtocolImplementation(h.engine, undefined, 'env_test'); + + await stageDraft(protocol, 'locked_grid'); + await protocol.saveMetaItem({ + type: 'view', name: 'locked_grid', organizationId: ORG, + item: viewBody('locked_grid', 'protected', { _lock: 'no-overlay' }), + packageId: PKG, actor: 'admin', + } as any); + + const res = await protocol.publishPackageDrafts({ + packageId: PKG, organizationId: ORG, actor: 'admin', + } as any); + expect(res.publishedCount).toBe(0); + + expect(byCode(h, 'item_locked')).toHaveLength(1); + expect(byCode(h, 'item_locked')[0]).toMatchObject({ + name: 'locked_grid', + operation: 'publish', + outcome: 'denied', + lock_state: 'no-overlay', + source: 'protocol.publishPackageDrafts', + }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e625005a99..4b6ab82f83 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -3188,6 +3188,53 @@ export type MetadataAuthoringGate = (ctx: MetadataAuthoringGateContext) => void */ export type MetadataAuthoringChannel = 'environment' | 'package-author'; +/** + * One `sys_metadata_audit` row as a VALUE — exactly the entry + * `recordMetadataAudit` writes, named so a refusal can hand its row to whoever + * is in a position to make it durable instead of writing it where it stands. + * + * [#8594] Why the entry became a value. `promoteDraftForPublish` used to audit + * its own denials inline, and it has two callers standing on opposite sides of + * a transaction boundary: `publishMetaItem` runs it in no transaction of its + * own, `publishPackageDrafts` runs it inside ONE `engine.transaction()` for the + * whole batch (ADR-0067 D2). A denial row written inline therefore landed on + * the single-item route and ROLLED BACK on the batch route — destroyed by the + * very rollback its own refusal had caused, which is the defect #7748 exists to + * close. Making the row data lets the helper stay ignorant of whose transaction + * it is inside (it cannot know, and guessing is what broke) and puts the + * placement decision where the transaction actually is: in the caller. + */ +interface MetadataAuditEntry { + type: string; + name: string; + organizationId?: string | null; + operation: 'save' | 'publish' | 'rollback' | 'delete' | 'reset'; + outcome: 'allowed' | 'denied' | 'forced'; + code: string; + lockState?: MetadataLock; + lockOverridden?: boolean; + actor?: string; + source?: string; + requestId?: string; + note?: string; +} + +/** + * An `Error` carrying the denial row it still owes (see {@link MetadataAuditEntry}). + * + * The property is name-mangled like `__batchItem` beside it: it is an internal + * hand-off between one private helper and its callers inside this file, never a + * wire field. `clientFacingFailureText` / `clientFacingFailureCode` read + * `message` / `code`, so nothing here reaches a response body. + */ +type ErrorWithPendingAudit = Error & { __pendingAudit?: MetadataAuditEntry }; + +/** Attach the denial row `err` owes, and return `err` so call sites read as one `throw`. */ +function withPendingAudit(err: E, audit: MetadataAuditEntry): E { + (err as E & { __pendingAudit?: MetadataAuditEntry }).__pendingAudit = audit; + return err; +} + /** * Implements the per-domain contracts this class ACTUALLY provides (ADR-0076 * D10 — the facade never implemented the other domains; those live in their @@ -10015,20 +10062,7 @@ export class ObjectStackProtocolImplementation implements * compliance trail. Phase 2 will make the audit table a hard * dependency. */ - private async recordMetadataAudit(entry: { - type: string; - name: string; - organizationId?: string | null; - operation: 'save' | 'publish' | 'rollback' | 'delete' | 'reset'; - outcome: 'allowed' | 'denied' | 'forced'; - code: string; - lockState?: MetadataLock; - lockOverridden?: boolean; - actor?: string; - source?: string; - requestId?: string; - note?: string; - }): Promise { + private async recordMetadataAudit(entry: MetadataAuditEntry): Promise { try { await this.engine.insert('sys_metadata_audit', { occurred_at: new Date().toISOString(), @@ -10055,13 +10089,20 @@ export class ObjectStackProtocolImplementation implements } /** - * Phase 1 L3 enforcement for write operations (save / publish / - * rollback). Returns null on allow. Returns the structured `Error` - * the caller should `throw` on deny — also records the denial in - * the audit log so refused attempts are visible in compliance - * reports (refused writes never reach sys_metadata_history). + * [#8594] The ADR-0010 L3 write verdict as a VALUE: the structured `Error` + * a refused caller should throw, PLUS the denial row that refusal owes — + * and it writes NOTHING itself. Null on allow. + * + * Split out of {@link assertLockAllowsWrite} (which is now this plus the + * write) for the one caller that cannot let the row be written where the + * verdict is reached: `promoteDraftForPublish` runs inside a batch + * transaction on one of its two routes, so a row written here would roll + * back with the batch its own refusal aborted. Callers that are not inside + * a transaction keep using `assertLockAllowsWrite` unchanged — this is an + * extraction, not a behaviour change, and `save` / `rollback` still get + * their row from the same expression they always did. */ - private async assertLockAllowsWrite(args: { + private async lockWriteRefusal(args: { type: string; name: string; organizationId?: string; @@ -10069,7 +10110,7 @@ export class ObjectStackProtocolImplementation implements actor?: string; source?: string; requestId?: string; - }): Promise { + }): Promise<{ err: Error; audit: MetadataAuditEntry } | null> { if (this.environmentId === undefined) return null; const state = await this.getEffectiveLock(args.type, args.name, args.organizationId ?? null); const refusal = evaluateLockForWrite(state.lock); @@ -10083,21 +10124,50 @@ export class ObjectStackProtocolImplementation implements (err as any).status = 403; (err as any).lock = state.lock; (err as any).lockReason = reason; - await this.recordMetadataAudit({ - type: args.type, - name: args.name, - organizationId: args.organizationId ?? null, - operation: args.operation, - outcome: 'denied', - // adr0112-ok: D6b — persisted audit column, its own vocabulary - code: 'item_locked', - lockState: state.lock, - actor: args.actor, - source: args.source ?? `protocol.${args.operation}MetaItem`, - requestId: args.requestId, - note: reason, - }); - return err; + return { + err, + audit: { + type: args.type, + name: args.name, + organizationId: args.organizationId ?? null, + operation: args.operation, + outcome: 'denied', + // adr0112-ok: D6b — persisted audit column, its own vocabulary + code: 'item_locked', + lockState: state.lock, + actor: args.actor, + source: args.source ?? `protocol.${args.operation}MetaItem`, + requestId: args.requestId, + note: reason, + }, + }; + } + + /** + * Phase 1 L3 enforcement for write operations (save / publish / + * rollback). Returns null on allow. Returns the structured `Error` + * the caller should `throw` on deny — also records the denial in + * the audit log so refused attempts are visible in compliance + * reports (refused writes never reach sys_metadata_history). + * + * ⚠️ Records the row WHERE IT STANDS, so only call it from a site that is + * not inside a transaction it could be rolled back by. From inside one, use + * {@link lockWriteRefusal} and hand the row to the caller that owns the + * transaction (see `promoteDraftForPublish`). + */ + private async assertLockAllowsWrite(args: { + type: string; + name: string; + organizationId?: string; + operation: 'save' | 'publish' | 'rollback'; + actor?: string; + source?: string; + requestId?: string; + }): Promise { + const refusal = await this.lockWriteRefusal(args); + if (!refusal) return null; + await this.recordMetadataAudit(refusal.audit); + return refusal.err; } /** Counterpart of {@link assertLockAllowsWrite} for delete. */ @@ -10170,7 +10240,28 @@ export class ObjectStackProtocolImplementation implements expectedParent?: unknown; actualHead?: unknown; }): Promise { - await this.recordMetadataAudit({ + await this.recordMetadataAudit(ObjectStackProtocolImplementation.optimisticConflictAuditEntry(args)); + } + + /** + * [#8594] The same row as a VALUE, for the site that must not write it + * where the conflict is caught — see {@link lockWriteRefusal} for the full + * argument. `recordOptimisticConflictAudit` above is now this plus the + * write, so the four routes that call it are byte-identical to before and + * the fifth (`promoteDraftForPublish`) hands the row to its caller. + */ + private static optimisticConflictAuditEntry(args: { + type: string; + name: string; + organizationId?: string | null; + operation: 'save' | 'publish' | 'rollback' | 'delete'; + actor?: string; + source: string; + requestId?: string; + expectedParent?: unknown; + actualHead?: unknown; + }): MetadataAuditEntry { + return { type: args.type, name: args.name, organizationId: args.organizationId ?? null, @@ -10182,6 +10273,35 @@ export class ObjectStackProtocolImplementation implements source: args.source, ...(args.requestId ? { requestId: args.requestId } : {}), note: `expected parent ${args.expectedParent ?? 'null'} but current is ${args.actualHead ?? 'null'}`, + }; + } + + /** + * [#8594] Write the denial row a refusal is still carrying — on the + * CALLER's side of the caller's transaction. + * + * The counterpart of {@link withPendingAudit}: `promoteDraftForPublish` + * attaches the row to the error it throws instead of writing it, and each + * of its two callers records it from a position where a rollback cannot + * reach it. A no-op for every other refusal (the driver faults, + * `NOT_OVERRIDABLE`, `INVALID_METADATA` …), which carry no row and never + * did — this closes the two denials that USED to write one and lose it, and + * mints nothing new. + * + * `source` is overridable because the row is now written by the route that + * owns it: the batch route must not file its rows under + * `protocol.publishMetaItem`, which is the name the shared Phase-1 helper + * passes for both of them. + */ + private async recordPendingDenialAudit( + err: unknown, + overrides?: { source?: string }, + ): Promise { + const pending = (err as ErrorWithPendingAudit | null | undefined)?.__pendingAudit; + if (!pending) return; + await this.recordMetadataAudit({ + ...pending, + ...(overrides?.source ? { source: overrides.source } : {}), }); } @@ -12153,7 +12273,16 @@ export class ObjectStackProtocolImplementation implements */ projectionApplied?: MutationProjectionOutcome; }> { - const { singularType, orgId, result } = await this.promoteDraftForPublish(request); + // [#8594] The refusal's own row is written HERE, by the route that owns + // the (absent) transaction — see `promoteDraftForPublish`'s header. This + // site has no transaction of its own, so recording it in the `catch` is + // where it always effectively landed; what changed is that the helper no + // longer assumes that on behalf of the batch route too. + const { singularType, orgId, result } = await this.promoteDraftForPublish(request) + .catch(async (err: unknown) => { + await this.recordPendingDenialAudit(err); + throw err; + }); // [#7748] ADR-0010 — success audit (best-effort), the same shape // `saveMetaItem` and `deleteMetaItem` write on their allowed paths. // @@ -12224,6 +12353,29 @@ export class ObjectStackProtocolImplementation implements * back together — the "a commit cannot half-land" invariant. * `publishMetaItem` composes it with {@link runPublishSideEffects} for * the single-item path. + * + * ## [#8594] …and NO AUDIT WRITES either — refusals carry their row out + * + * Both denials this method can raise — the ADR-0010 lock refusal and the + * optimistic-lock 409 — used to write their `sys_metadata_audit` row right + * here. Which is durable on one of its two routes and destroyed on the + * other: `publishMetaItem` calls this outside any transaction, while + * `publishPackageDrafts` calls it inside the batch's, so the row describing + * the refusal was rolled back BY THAT REFUSAL — the trail ended up with + * nothing about a publish refused by a lock, which is the exact defect + * #7748 exists to close. `batch_aborted` (#8400) gave the batch route *a* + * trail but not the inner verdict's vocabulary: a compliance query on + * `code = 'item_locked'` still found nothing. + * + * So the row leaves as data on the thrown error ({@link withPendingAudit}), + * and each caller records it with {@link recordPendingDenialAudit} from a + * position a rollback cannot reach. This helper does not know whose + * transaction it is inside — it cannot, and guessing is what broke — so the + * placement decision moves to the only two places that do know. + * + * ⚠️ A NEW CALLER INHERITS THAT OBLIGATION: catch, call + * `recordPendingDenialAudit`, rethrow. Skip it and the refusal is silent + * again — the pre-#7748 state, not merely a worse one. */ private async promoteDraftForPublish(request: { type: string; name: string; organizationId?: string; actor?: string; message?: string; @@ -12258,7 +12410,11 @@ export class ObjectStackProtocolImplementation implements if (orgRefusal) throw orgRefusal; } // ADR-0010 L3 — lock blocks publish too (publishing is a write). - const _publishLockErr = await this.assertLockAllowsWrite({ + // + // [#8594] `lockWriteRefusal`, not `assertLockAllowsWrite`: the row rides + // OUT on the error and each caller records it on its own side of its own + // transaction. See this method's header for why it cannot be written here. + const _publishLockRefusal = await this.lockWriteRefusal({ type: request.type, name: request.name, ...(request.organizationId ? { organizationId: request.organizationId } : {}), @@ -12266,7 +12422,9 @@ export class ObjectStackProtocolImplementation implements ...(request.actor ? { actor: request.actor } : {}), source: 'protocol.publishMetaItem', }); - if (_publishLockErr) throw _publishLockErr; + if (_publishLockRefusal) { + throw withPendingAudit(_publishLockRefusal.err, _publishLockRefusal.audit); + } await this.ensureOverlayIndex(); const orgId = request.organizationId ?? null; const repo = this.getOverlayRepo(orgId); @@ -12321,17 +12479,22 @@ export class ObjectStackProtocolImplementation implements conflict.status = 409; conflict.expectedParent = err.expectedParent; conflict.actualHead = err.actualHead; - await this.recordOptimisticConflictAudit({ - type: request.type, - name: request.name, - organizationId: orgId, - operation: 'publish', - ...(request.actor ? { actor: request.actor } : {}), - source: 'protocol.publishMetaItem', - expectedParent: err.expectedParent, - actualHead: err.actualHead, - }); - throw conflict; + // [#8594] Attached, not written — same reason as the lock gate + // above. The repository's own transaction has already unwound by + // the time this `catch` runs, but the BATCH caller's has not. + throw withPendingAudit( + conflict, + ObjectStackProtocolImplementation.optimisticConflictAuditEntry({ + type: request.type, + name: request.name, + organizationId: orgId, + operation: 'publish', + ...(request.actor ? { actor: request.actor } : {}), + source: 'protocol.publishMetaItem', + expectedParent: err.expectedParent, + actualHead: err.actualHead, + }), + ); } throw err; } @@ -12913,13 +13076,23 @@ export class ObjectStackProtocolImplementation implements // the exact defect #7748 exists to close, reintroduced on the batch // route. // - // ⚠️ Note the pre-existing sibling this does NOT fix: the denial - // rows `assertLockAllowsWrite` / `recordOptimisticConflictAudit` - // write from inside `promoteDraftForPublish` ARE inside this - // transaction on the batch route, so they roll back. That is why - // this row is unconditional rather than "only when the inner gates - // didn't already record one": on a transactional engine there is - // nothing left of theirs to duplicate. Filed separately. + // ⚠️ [#8594 — the sibling above is now FIXED, one line below.] The + // note that stood here said the inner denial rows were written from + // inside this transaction and rolled back with it, so this row could + // be unconditional: there was never anything of theirs to duplicate. + // That is no longer true — `promoteDraftForPublish` hands its refusal + // row out instead of writing it, and the line below lands it here, + // outside the transaction, in the INNER verdict's own vocabulary + // (`item_locked` / `metadata_conflict`, with its `lock_state`). + // + // The row below stays unconditional anyway, for a different reason: + // it records a DIFFERENT fact. `item_locked` says why THIS item was + // refused; `batch_aborted` says the whole batch rolled back and + // nothing landed — the ADR-0067 D2 consequence, which is what the + // other drafts' authors need to read, and the only row there is when + // the cause carries no inner verdict at all (a driver fault, + // `NOT_OVERRIDABLE`, `INVALID_METADATA`). Two facts, two rows, same + // causal item. // // ⚠️ `note` is WIRE-VISIBLE — `auditMetaItem` maps it straight onto // the `GET /api/v1/meta/:type/:name/audit` response — so it carries @@ -12928,6 +13101,20 @@ export class ObjectStackProtocolImplementation implements // `failed[].error` around that rule through a second door. The full // untruncated text is already in the `console.warn` above, which is // where an operator reads it. + // ═══ [#8594] The INNER verdict's own row — also outside the txn ═══ + // + // `promoteDraftForPublish` refused this batch and handed its denial + // row out on the error rather than writing it inside the closure + // that has since unwound. Recording it here gives a package publish + // refused by a lock the same `code = 'item_locked'` + `lock_state` + // row a single-item publish has always left — the vocabulary a + // compliance query actually filters on, which `batch_aborted` alone + // could not supply. No-op when the cause carries no inner verdict. + // + // `source` is re-stamped: the shared Phase-1 helper passes + // `protocol.publishMetaItem` for both of its routes, and this row + // was written by the batch route. + await this.recordPendingDenialAudit(e, { source: 'protocol.publishPackageDrafts' }); if (causal) { await this.recordMetadataAudit({ type: causal.type,