Skip to content

fix(metadata-protocol): a package publish refused by a lock or a 409 leaves the inner verdict's own audit row (#8594) - #8627

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-8594-denial-audit-survives-rollback
Aug 14, 2026
Merged

fix(metadata-protocol): a package publish refused by a lock or a 409 leaves the inner verdict's own audit row (#8594)#8627
os-zhuang merged 1 commit into
mainfrom
claude/issue-8594-denial-audit-survives-rollback

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8594

The defect

publishPackageDrafts promotes every draft inside ONE engine.transaction()
(ADR-0067 D2). promoteDraftForPublish runs inside that closure, and it wrote
its denial audit rows there — assertLockAllowsWrite's item_locked (with
its lock_state column) and recordOptimisticConflictAudit's
metadata_conflict. On a transactional engine both rolled back with the batch:
the refusal is what aborted the batch, so the row describing the refusal was
destroyed by the very rollback it caused. A compliance query filtering
code = 'item_locked' found nothing for a package publish refused by a lock.

#8400's batch_aborted row gave a refused batch a trail, but it carries the
batch's fact, not the item-level verdict's vocabulary or its lock column — which
is the gap this card names.

Premise re-verified on the merged ref, and the triage assumption measured

The card and its triage comment predate #8400's landing (fda61e41, PR #8605).
Re-checked on origin/main @ 60cbf9d45d with #8400 in: both quoted assertions
still pass, i.e. the write is genuinely made and genuinely undone. Baseline
confirmed before building on it.

Triage's mechanism assumption — "option 1 … the single-item route already
writes outside a transaction"
— is the whole argument for option 1, so it was
measured, not assumed, and it holds:

  • the lock refusal is raised beforerepo.promoteDraft, so it is outside
    any repository transaction by construction;
  • the 409 is caught afterrepo.put's own withTxn has unwound and
    rethrown.

That measurement is now a permanent pin, not a note — premise: the SINGLE-ITEM publish route audits outside a transaction — its item_locked row survives, run
against the same really-rolling-back harness. It is green before and after
this change, which is exactly what a premise pin should be. Option 1 taken as
the card describes it; no fork.

The shape

promoteDraftForPublish no longer writes those rows. Each refusal carries its
row out as data on the thrown error, and each of the two callers records it on
its own side of its own transaction:

  • publishMetaItem records it in a catch — that route has no transaction, so
    the row lands where it always effectively landed, still filed under
    source: 'protocol.publishMetaItem';
  • publishPackageDrafts records it from the rollback handler, outside the
    transaction, re-stamped source: 'protocol.publishPackageDrafts' (the shared
    Phase-1 helper names the single-item route for both of its callers, and filing
    a batch publish under that name would make the trail lie about the door used).

The helper is now ignorant of whose transaction it is inside — it cannot know,
and guessing is what broke. The placement no longer depends on the engine's
capabilities either: an engine with notransaction() lands the same row in
the same place, pinned.

Mechanically this is two extractions plus a hand-off, so the four other routes
that audit are byte-identical to before: assertLockAllowsWrite is now
lockWriteRefusal (the verdict as a value) plus the write, and
recordOptimisticConflictAudit is now optimisticConflictAuditEntry plus the
write. save / rollback / delete call the unchanged wrappers.

Two rows, two facts, neither replacing the other. A batch refused by a lock
now leaves item_locked (+ lock_state) and#8400's batch_aborted. A
refusal that reached neither gate — driver fault, NOT_OVERRIDABLE,
INVALID_METADATA — still leaves exactly the one batch_aborted row it left
before. No new code value is minted: ADR-0112 D6b keeps
sys_metadata_audit.code a closed persisted vocabulary and the values that land
were already in it. The one new gate hit was a 'batch_aborted' comparison in a
test, marked per-site with the documented adr0112-ok: D6b, following #8400's
precedent rather than exempting the file.

The pin inversion is the deliverable's proof, not a test that broke

protocol.package-publish-audit-rows.test.ts ended its locked-item case on

expect(h.auditRows.some((a) => a.code === 'item_locked')).toBe(false);

That assertion was measuring this defect rather than fixing it — a true
statement about main, not a contract. It now reads .toBe(true), deliberately
and with the reasoning written into the file's header and beside the line. Two
count-based assertions in the same file became membership assertions on
code for the same reason: a lock refusal now leaves two denial rows, and a
length assertion would have read the fix as a regression.

An inversion alone is not enough — a "fix" that simply stopped rolling
anything back would satisfy it. So every case asserting a surviving denial also
asserts ADR-0067 D2 in the same breath: publishedCount: 0, published: [],
the already-promoted sibling still a draft with no active row, no
sys_metadata_commit row, no allowed row.

Reverse verification — direction predicted before running, both ways

(a) Revert the production edits, keep the tests. Predicted RED on the
deliverable assertions, GREEN on the premise.
Measured: 6 failed / 9 passed.
Red exactly where predicted —

AssertionError: expected [ 'batch_aborted' ] to deeply equal [ 'batch_aborted', 'item_locked' ]
AssertionError: expected false to be true // the inverted pin
AssertionError: expected [] to have a length of 1 // item_locked row
AssertionError: expected undefined to be defined // the /audit read door
AssertionError: expected [] to have a length of 1 // metadata_conflict row
AssertionError: expected { id: 'a_7', …(13) } to match object { name: 'locked_grid', …(4) }

The last one is the no-transaction() case and it is red for a different
reason than the rest, as predicted: with nothing to roll back the row already
survived pre-fix, so what fails there is its source, not its existence. The
premise pin and the driver-fault guard stayed green.

(b) The over-broad direction, in both of its shapes.

  • "the transaction no longer rolls back"inTxn forced to a passthrough.
    Predicted: denial assertions stay GREEN, ADR-0067 D2 controls go RED.
    Measured: 3 failed / 12 passed, all three expected [ 'active' ] to deeply equal [ 'draft' ]. The controls are load-bearing.
  • "rows survive that should not"recordPendingDenialAudit forced to mint an
    inner-vocabulary row when no gate reached a verdict. Predicted: the
    driver-fault case goes RED. Measured: 1 failed / 14 passed, expected [ … ] to have a length of 1 but got 2.

Both ablations were applied on top of a committed fix and restored with
git checkout HEAD -- packages/metadata-protocol/src/protocol.ts; no
git stash anywhere.

Tests

  • protocol.package-publish-audit-rows.test.ts — 15 passed. Inverted pin +
    membership conversions, and a new [#8594] block: the premise pin, the lock
    verdict in full (code, lock_state, source, scope) with its D2 control,
    the /audit read door, the metadata_conflict verdict with its D2 control,
    and the no-transaction() engine.
  • The metadata_conflict case provokes a real repository ConflictError — a
    rival author advancing the active row between promoteDraft's two reads
    (advanceActiveOnRead, scoped to package-scoped active reads so the pre-flight
    commitItems scan does not consume the one-shot) — not an injected error.
  • @objectstack/metadata-protocol full suite: 87 files / 1278 tests passed.
  • Downstream consumers (the prefix filter direction — dependents of
    @objectstack/metadata-protocol, i.e. downstream), rebuilt first to dodge the
    dist-resolution trap: objectql 203/3579, rest 115/1896, runtime
    155/2369 — all passed.
  • tsc --noEmit over the package: 63 errors, exactly the DEBT ledger's
    frozen count, unchanged; zero in either file I touched.

Gates run locally: check:error-code-casing (red first, fixed with the per-site
D6b marker, then green), check:nul-bytes (+ a grep -naP control-byte
self-scan of all three files), check:cross-package-test-inputs,
check:durability-log-level, check:filter-alias-parity,
check:query-options-erasure, check:type-check-coverage,
check:engine-double-contract, check:empty-changeset,
check:changeset-gate-self-tests, check:objectui-changeset,
check-changeset-no-major, check-adr-0087-registration. The list was
re-derived against the actual changed paths with
scripts/pm/dispatch-gates.mjs; that re-derivation added the .changeset
family (check:objectui-changeset, check-adr-0087-registration,
check-empty-changeset, check-changeset-no-major), which a pre-change gate
list could not have named.

Scope

#8595 — the pre-flight refusal (the preflightViolations early return above
the transaction, which writes nothing at all) — is a hard same-file serial and is
not addressed here; that branch is untouched. This shape does make it
follow easily: the pre-flight sits outside the transaction already, so it needs a
row written, not a row relocated, and recordMetadataAudit is right there.
That is an observation for whoever takes it, not a change in this PR.

protocol.ts region declared on the card before the first edit:
promoteDraftForPublish / publishPackageDrafts audit emission — checked
against the file's other live claims (the ADR-0070 D1 gate region, and
toRowApiError around lines 1491-1552); no overlap with either.

Generated by Claude Code


Generated by Claude Code

@vercel

vercelBot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 14, 2026 3:48am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol.

3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 14, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 14, 2026 04:17
@os-zhuang
os-zhuang added this pull request to the merge queueAug 14, 2026
Merged via the queue into main with commit 8a102d0Aug 14, 2026
27 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8594-denial-audit-survives-rollback branch August 14, 2026 04:31
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The lock/conflict denial audit rows roll back with the batch on publishPackageDrafts — a refused item in a package publish still leaves no trail

2 participants

@os-zhuang@claude