Skip to content

fix(metadata-protocol): audit the batch package publish in Phase 2, outside the transaction (#8400) - #8605

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-8400-publish-drafts-audit
Aug 14, 2026
Merged

fix(metadata-protocol): audit the batch package publish in Phase 2, outside the transaction (#8400)#8605
os-zhuang merged 4 commits into
mainfrom
claude/issue-8400-publish-drafts-audit

Conversation

@claude

@claudeclaudeBot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Fixes#8400

publishPackageDrafts — Studio's "publish whole app", POST /packages/:id/publish-drafts
promoted every draft in a package and wrote no sys_metadata_audit rows at all. It calls
promoteDraftForPublish directly rather than publishMetaItem, so #7748's row never ran for
it: a batch that published twenty artifacts left the trail exactly as empty as a batch nobody
ran.

Placement, and why it is the fix

The card's placement argument is the ruling and I implemented it as written: Phase 2, after
the batch transaction commits, driven off promoted[].

outcomewhere it is writtenwhy there
publish / allowed, one per promoted itemtop of Phase 2, off promoted[]persistence committed, side effects not yet run — the position saveMetaItem, deleteMetaItem and publishMetaItem all take
publish / denied, one for the causal itemthe rollback catch, outside engine.transaction()a row written inside rolls back with the batch it records; for the refusal that caused the rollback that means the trail records nothing about a refused write — the defect #7748 exists to close

Both rows carry source: 'protocol.publishPackageDrafts', so the trail distinguishes "publish
whole app" from a single-item publish.

Three details that were not obvious going in:

  1. The allowed rows are keyed on the draft's own org, not the request's active org.
    listDrafts surfaces env-wide (organization_id IS NULL) drafts to a non-null-org caller
    and the promote targets the draft's own scope ([BUG] publish-drafts fails with no_draft after saving draft via Studio UI #3115). Keyed on the caller's org, the row
    would record the publish against a partition the active row never entered. promoted[]
    only carried { type, name }, so the scope is now captured explicitly as draftOrgId.
  2. note is wire-visible.auditMetaItem maps it straight onto the
    GET /api/v1/meta/:type/:name/audit response, so the denied row carries
    clientFacingFailureText, not e.message — otherwise the driver dialect [finding] metadata-protocol's batch verbs still put caught error text on client-facing payloads — the 8 producers option C did not reach #8333 withheld
    from failed[].error would reach a client through a second door. The full raw sentence
    stays in the existing console.warn, where an operator reads it.
  3. code: 'batch_aborted' is one fixed value, not the lower-cased causal code. The audit
    code column is a documented closed set (ADR-0112 D6b). Lower-casing whatever
    error.code happened to abort the batch would turn it into an open set that grows with the
    error catalog. The causal code rides in note. The column's declared set in
    sys-metadata-audit.object.ts is updated to include the new value, so declared still
    equals written.

The stale comment in publishMetaItem that said the batch path "is therefore still
unaudited" is corrected in the same commit.

Anti-vacuity: how I know the harness persists rather than skips

The card names the trap — most multi-table fakes open insert with
if (table === 'sys_metadata_audit') return { id: 'audit_skip' };, which makes every audit
assertion pass for the wrong reason. Three things, not one:

  • The new harness is built on the shape of protocol.lifecycle-audit-rows.test.ts and has
    noaudit_skip branch: the audit insert appends to auditRows and mints a real id like
    any other table.
  • A positive control exercises the one site that already worked before draft-publish-lifecycle: the metadata audit trail records only save — publish, rollback and the 409 conflict denial never write a row #7748
    (save) and asserts the landed row's full payload and that its id matches /^a_\d+$/
    i.e. a real persisted row, not the sentinel. Under a revert of the production edits this
    control must stay green, and it did.
  • A second positive control proves the harness's transaction() really rolls back, which
    is what makes the placement claim falsifiable at all (see below).

Attempted and landed are tracked separately — auditAttempts records every insert aimed at
the audit table and is deliberately excluded from the rollback snapshot. Best-effort semantics
(ADR-0010 §3.6) swallow a failed audit write, so without that channel "row missing" and "write
failed" are indistinguishable. Here it separates three states, not two: never attempted
(the defect), attempted-and-rolled-back (the wrong placement), attempted-and-rejected (a
provisioning fault).

Prediction vs measurement

Predictions were written down before either run
(ablation-prediction.md in the session scratchpad), test by test and assertion by assertion.

Ablation 1 — revert the production edit, keep the tests

git checkout origin/main -- packages/metadata-protocol/src/protocol.ts

Predicted 6 red / 2 green. Measured 6 red / 2 green. No divergence — including which
assertion fails first in each case:

× a batch publish writes one `publish`/`allowed` row per promoted draft
AssertionError: expected [] to have a length of 2 but got +0
× the QA shape: a two-item batch is counted, not swallowed
AssertionError: expected { save: 2 } to match object { save: 2, publish: 2 }
× auditMetaItem surfaces the batch publish row
AssertionError: expected [ 'save' ] to include 'publish'
× a refused batch leaves a `denied` row that SURVIVES the rollback it caused
AssertionError: expected [] to have a length of 1 but got +0
× the denied row quotes the refusal but never the driver dialect
AssertionError: expected [] to have a length of 1 but got +0
× a failing audit table does not fail the batch publish — attempts still observable
AssertionError: expected [] to deeply equal [ 'case_grid', 'ticket_grid' ]
Tests 6 failed | 2 passed (8)

Two predicted sub-facts worth stating because they are where a weaker test would have gone
vacuous:

  • In the last case, expect(h.auditRows).toHaveLength(0) passes under the defect too.
    That assertion alone would be green forever. The failing one is the auditAttempts check —
    under the defect nothing is attempted, so there is nothing for the failing table to reject.
  • In the refused-batch case, the five assertions before the audit check (success: false,
    publishedCount: 0, BATCH_ABORTED, case_grid still a draft, zero allowed rows) all pass
    under the defect, correctly: they describe the rollback, not the audit.

Ablation 2 — the placement ablation

Ablation 1 proves an insert exists somewhere; it cannot tell the two sides of the transaction
apart. So the same insert, same payload, was moved into the Phase-1 closure and measured.

Predicted 2 red / 6 green. Measured 2 red / 6 green. No divergence:

× a refused batch leaves a `denied` row that SURVIVES the rollback it caused
AssertionError: expected [] to have a length of 1 but got +0
× the denied row quotes the refusal but never the driver dialect
AssertionError: expected [] to have a length of 1 but got +0
Tests 2 failed | 6 passed (8)

The placement is load-bearing and measured, not asserted.

Ablation 3 — the scope ablation (added in review)

Both audit rows are keyed on the draft's own org, not the publishing session's
active org. __batchItem is stamped as d, the listDrafts row, whose mapper is
organizationId: row.organization_id ?? null — the same source promoted[].draftOrgId
comes from, so the two outcomes agree by construction. But the original fixtures used
ORG for the draft and the session, so a row keyed on either would have passed: the
scope was unpinned in both directions.

Two fixtures now differ — an env-wide draft (organization_id IS NULL) published,
and refused, by a non-null-org caller, which is the #3115 shape listDrafts surfaces
through its $or. Ablating the production code to key both rows on the caller's orgId:

Predicted 2 red / 8 green. Measured 2 red / 8 green. No divergence:

× scope: an env-wide draft published by an org-scoped caller audits ENV-WIDE, not to the caller org
AssertionError: expected 'org_alpha' to be null
× scope: an env-wide draft REFUSED under an org-scoped caller audits ENV-WIDE too
AssertionError: expected 'org_alpha' to be null
Tests 2 failed | 8 passed (10)

The other 8 stay green because in every one of them the draft's org and the caller's org
are the same value — which is exactly why they could not pin the scope and these two
were added.

What the tests do NOT pin, stated plainly

The allowed side's placement is under-determined by this suite, and that is not an
oversight in the tests — it is the card's own observation that for an allowed publish an
in-transaction row "is arguably right". Both placements produce the same observable result:
the batch committed, so a row written inside would have survived too. The reason it sits in
Phase 2 anyway is that one route must not audit its two outcomes under two different
durability rules; that argument is in the code comment, not in a red test.

The sharpest single measurement in the suite is inside the refused-batch case:
assertLockAllowsWrite writes its item_locked denial from inside the batch transaction,
so it is attempted and then rolled away, while the batch_aborted row is written from the
catch and survives. One refusal, two audit writes, two different fates, decided entirely by
which side of the transaction they sit on:

expect(h.auditAttempts.some((a) => a.code === 'item_locked')).toBe(true); // attempted
expect(h.auditRows.some((a) => a.code === 'item_locked')).toBe(false); // and gone

Verification

  • packages/metadata-protocol: 86 files, 1271 tests passed
  • packages/metadata-core: 10 files, 162 tests passed
  • typecheck green on both packages (source and test projects)
  • Gate families run locally, all green: check:nul-bytes, check:error-code-casing,
    check:test-source-alias, check:engine-double-contract,
    check:cross-package-test-inputs, check:durability-log-level,
    check:filter-alias-parity, check:query-options-erasure,
    check:changeset-gate-self-tests, check:objectui-changeset,
    check-adr-0087-registration, check-changeset-no-major, check-empty-changeset,
    check:type-check-debt

check:error-code-casing legitimately failed first: the test asserts code: 'batch_aborted'
in a code position. Fixed with the per-site adr0112-ok: D6b opt-out the gate documents,
rather than exempting the whole file — the rest of the file stays guarded.

The gate list was re-derived against the actual changed paths with
node scripts/pm/dispatch-gates.mjs after the final commit, not taken from the dispatch
prompt alone.

Adjacent defects, filed separately (not fixed here)

Generated by Claude Code


Generated by Claude Code

os-zhuangand others added 3 commits August 13, 2026 23:59
…utside the transaction
`publishPackageDrafts` (Studio's "publish whole app") wrote no
`sys_metadata_audit` rows at all — neither the allowed-outcome `publish`
rows nor a `denied` row for a refusal. It calls `promoteDraftForPublish`
directly, so #7748's row in `publishMetaItem` never ran for it.
Both outcomes are written in Phase 2, after the ADR-0067 D2 transaction
has closed: the allowed rows off `promoted[]`, the `denied` row from the
rollback `catch`. Inside the transaction the refusal's own row would roll
back with the batch it records, which is the defect #7748 closed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
…geset
`check:error-code-casing` correctly flags `code: 'batch_aborted'` in a code
position. Opted out per site with the gate's documented `adr0112-ok: D6b`
marker rather than exempting the whole file, so the rest stays guarded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@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 1:34am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-core, @objectstack/metadata-protocol.

4 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-core, @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/plugins/packages.mdx(via @objectstack/metadata-core)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)

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

  • content/docs/releases/v12.mdx(via @objectstack/metadata-core)
  • 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
…cope
The existing fixtures used ORG for the draft and for the publishing session,
so an audit row keyed on either would pass. These two differ: an env-wide
draft (organization_id IS NULL) published, and refused, by a non-null-org
caller — the #3115 shape listDrafts surfaces via its $or.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@os-zhuangClaude

Copy link
Copy Markdown
Contributor

A — it is already the draft's own scope. And you were right that the fixture did not prove it, so it does now.

__batchItem is stamped with d, the loop variable over ordered, which is built from repo.listDrafts({ packageId }). That mapper is:

organizationId: row.organization_id ?? null,

so d.organizationId is the draft's ownorganization_id — the identical source draftOrgId comes from one line earlier (const draftOrgId = d.organizationId ?? null). The two rows agree by construction, not by coincidence: same object, same field. request.organizationId reaches the batch only as orgId, which is used for recordPackageCommit and the side effects, and touches neither audit row.

So the asymmetry you were checking for does not exist. But your second point was the real one:

without it neither row's scope is actually pinned

Correct, and it was unpinned in both directions — ORG was the draft's org and the session's org in every fixture, so a row keyed on either would have passed. Two fixtures added, pushed as 518814dc2c, where the two values differ: an env-wide draft (organization_id IS NULL) published, and refused, by a non-null-org caller — the #3115 shape listDrafts surfaces through its $or.

And I ablated them rather than trusting a green run, since a new fixture that cannot fail is worth nothing:

Ablation 3 — key both rows on the caller's orgId instead of the draft's scope. Predicted 2 red / 8 green before running; measured 2 red / 8 green, no divergence:

× scope: an env-wide draft published by an org-scoped caller audits ENV-WIDE, not to the caller org
AssertionError: expected 'org_alpha' to be null
× scope: an env-wide draft REFUSED under an org-scoped caller audits ENV-WIDE too
AssertionError: expected 'org_alpha' to be null
Tests 2 failed | 8 passed (10)

expected 'org_alpha' to be null is precisely the caller's active org leaking into the row — the defect shape you asked about, now caught by a red test in both outcomes. The other 8 stay green because their draft org and caller org are the same value, which is exactly why they could not pin it.

Re-verified after the change on the merged tree: metadata-protocol 1271 tests passed (86 files, +2), check:type-check-debt re-measure still none above its recorded number / surplus: none, and nul-bytes, error-code-casing, test-source-alias, engine-double-contract, where-matcher, query-options-erasure, cross-package-test-inputs all green. Ablation reverted via git checkout of the branch; git status clean, zero ablation traces. No ledger growth, no stash, no force-push; #8594 and #8595 remain filed, not fixed here.

PR body updated with Ablation 3.

Generated by Claude Code


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 14, 2026 01:47
@os-zhuang
os-zhuang enabled auto-merge August 14, 2026 01:47
@os-zhuang
os-zhuang added this pull request to the merge queueAug 14, 2026
Merged via the queue into main with commit fda61e4Aug 14, 2026
32 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8400-publish-drafts-audit branch August 14, 2026 02:06
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.

publishPackageDrafts writes no audit rows — Studio's "publish whole app" leaves the audit trail empty

2 participants

@os-zhuang@claude