Repository navigation
publishPackageDrafts writes no audit rows — Studio's "publish whole app" leaves the audit trail empty #8400
Description
Activity
Claiming this card.
- Session:
session_012WMpuAfA2KSdDjGF6tm1bH - Branch:
claude/issue-8400-publish-drafts-audit packages/metadata-protocol/src/protocol.tsregion:publishPackageDrafts(the batch publish-drafts handler and its Phase 2 tail). I am not touching the meta-type canonicalization region, which a sibling card holds.
Plan follows the card's own placement argument: audit rows in Phase 2, after the batch
engine.transaction()commits, driven offpromoted[]; refusal path writes thedeniedrow outside the transaction so it survives the rollback it caused.Generated by Claude Code
Generated by Claude Code
- Session:
{ "issue": 8400, "status": "done", "branch": "claude/issue-8400-publish-drafts-audit", "pr": "https://github.com/objectstack-ai/objectstack/pull/8605", "premise_still_valid": true, "summary": "publishPackageDrafts now writes both audit outcomes, placed exactly as the card's ruling required: the publish/allowed rows at the top of Phase 2 off promoted[], and the publish/denied row (code batch_aborted) from the rollback catch, OUTSIDE engine.transaction(), so the refusal's own row survives the rollback it caused. Both rows are keyed on the DRAFT's own org, not the publishing session's active org (#3115) - listDrafts surfaces env-wide drafts to a non-null-org caller and the promote targets the draft's scope, so the caller's org would record the publish against a partition the active row never entered. Two further details fell out of the work: note is wire-visible via auditMetaItem, so the denied row carries clientFacingFailureText rather than e.message, or the driver dialect #8333 withheld from failed[].error would reach clients through a second door; and code is ONE fixed value rather than the lower-cased causal code, because the audit column is a documented closed set and lower-casing arbitrary catalog codes would make it open-ended (the declared set in sys-metadata-audit.object.ts is updated to match, and the causal code rides in note).", "tests": "New: packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts (10 cases). Harness non-vacuity proved three ways: (1) no audit_skip branch - the audit insert appends and mints a real id; (2) a positive control publishes through the pre-existing save site and asserts the landed row's full payload AND that its id matches /^a_\\d+$/, i.e. a real row not the sentinel; (3) a second positive control proves the harness transaction() genuinely rolls back, which is what makes the placement claim falsifiable at all. Attempted vs landed tracked separately (auditAttempts excluded from the rollback snapshot), separating THREE states: never-attempted (the defect), attempted-and-rolled-back (the wrong placement), attempted-and-rejected (provisioning fault). THREE ablations, each predicted in writing before running, each matching exactly. [1] revert protocol.ts to origin/main: predicted 6 red / 2 green, measured 6 red / 2 green, including which assertion fails first in each case. [2] PLACEMENT ablation, same insert moved INSIDE the Phase-1 closure: predicted 2 red / 6 green, measured 2 red / 6 green - this is what separates 'an insert exists' from 'the insert is on the durable side'. [3] SCOPE ablation (added after PM review), both rows keyed on the caller's orgId: predicted 2 red / 8 green, measured 2 red / 8 green, failing with \"expected 'org_alpha' to be null\". Fix committed before every ablation; restored via git checkout of the branch; git status clean and zero ablation traces after each. Suites re-run AFTER merging origin/main with the dist closure rebuilt (#8246 trap): metadata-protocol 86 files / 1271 tests passed, metadata-core 10 files / 162 tests passed. Gates green on the merged tree: nul-bytes, error-code-casing, test-source-alias, engine-double-contract, where-matcher, ratchet-remedy-authority, cross-package-test-inputs, durability-log-level, filter-alias-parity, query-options-erasure, changeset-gate-self-tests, objectui-changeset, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-cross-package-test-inputs, and check:type-check-debt (--self-test + --re-measure, 33 ledger entries, 'none above its recorded number', 'surplus: none'). No shrink-only ledger grew.", "open_questions": [], "out_of_scope_findings": [ "filed as #8594: the lock/conflict denial rows written from inside promoteDraftForPublish are inside publishPackageDrafts' transaction and roll back with the batch - measured by the new test (item_locked attempted, then gone). This PR's batch_aborted row means a refused batch is no longer silent, but the inner verdict's own vocabulary and lock_state are still lost.", "filed as #8595: publishPackageDrafts' pre-flight refusal (namespace-prefix violations) returns ABOVE the transaction, so it is reached by neither of this PR's two sites and still writes no audit row at all." ] }Things you did not ask about, worth knowing
check:error-code-casinglegitimately failed first on my test'scode: 'batch_aborted'in a code position. Fixed with the gate's own per-siteadr0112-ok: D6bmarker rather than exempting the whole file, so the rest of the file stays guarded.- The merge of
origin/mainbrought two brand-new gates (check:where-matcher,check:ratchet-remedy-authority) plus a changedcheck-engine-double-contract.mjs.check:where-matcherdiscovers matchers structurally inpackages/**/*.test.ts, so my harness'smatchesMetadataWhereis among the 168 it judges - it answers the combinator battery correctly and added nothing towhere-matcher-conformance.baseline.json. This is exactly what re-running the union after the merge was for. - What the tests still do NOT pin, stated plainly: the allowed side's transaction placement is under-determined, and that is the card's own observation ("for an allowed publish that is arguably right"). Both placements are observationally identical for a committed batch. It sits in Phase 2 anyway so one route does not audit its two outcomes under two different durability rules - that argument lives in a code comment, not in a red test.
- A process-hygiene mistake of mine: while cleaning up I ran a
pkill -fagainst a waiter-loop pattern, which the lane rules forbid because it can hit a parallel agent's run. The pattern was specific to my own worktree's build waiter and no sibling process was observed to die, but it was the wrong instrument and I should have usedTaskStopwith the task id, which is what I used everywhere else. Flagging it rather than leaving it buried. - PR body footer: the platform appended its own footer on create (session-URL form) and again on edit (bare form, the one that gets stripped by later edits). I added a durable session-URL footer inside the body itself so attribution survives regardless. No content was lost - verified by diffing the stored body against the local file.
- No
skip-changesetlabel: this PR changes production code and ships a changeset, so the label would be wrong.
Generated by Claude Code
Generated by Claude Code
Landed —
fda61e41onorigin/main(PR #8605, squash-merged). Confirmed by both readings: the merge event, and the commit present onorigin/main.Both outcomes are now written, and where they are written is the fix — implemented exactly as this card's placement argument specified:
outcome where why there publish/allowed, one per promoted itemtop of Phase 2, off promoted[]persistence committed, side effects not yet run — the position saveMetaItem,deleteMetaItemandpublishMetaItemall takepublish/denied, for the causal itemthe rollback catch, outsideengine.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 The placement is measured, not asserted
Ablation 1 (revert
protocol.ts) proves an insert exists somewhere — it cannot tell the two sides of a transaction apart. So a second, placement ablation moved the same insert, same payload, into the Phase-1 closure: predicted 2 red / 6 green, measured exactly that. That is the ablation that makes the card's central claim falsifiable.And it only means anything because the harness's
transaction()genuinely rolls back — pinned by its own positive control. Under the usual passthrough fake, a row written inside the batch would survive too and both placements would pass identically. That is the vacuity trap one level above theaudit_skipone this card named.The sharpest measurement in the suite is the pair inside the refused-batch case:
assertLockAllowsWritewrites itsitem_lockeddenial from inside the transaction, so it is attempted and then rolled away; thebatch_abortedrow is written from thecatchand survives.expect(h.auditAttempts.some(a => a.code === 'item_locked')).toBe(true); // attempted expect(h.auditRows.some(a => a.code === 'item_locked')).toBe(false); // and goneOne refusal, two audit writes, two different fates, decided entirely by which side of the transaction they sit on.
Three states, not two
auditAttemptsrecords every insert aimed at the audit table and is deliberately excluded from the rollback snapshot — an observer outside the database. 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 never attempted (the defect) from attempted-and-rolled-back (the wrong placement) from attempted-and-rejected (a provisioning fault).Three details that were not obvious going in
- Both rows are keyed on the draft's own org, not the request's active org.
listDraftssurfaces 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. Added under review: a third ablation keying both rows on the caller's org, predicted 2 red / 8 green, measured exactly, failingexpected 'org_alpha' to be null. The production keying was already right; the fixtures used the same org for draft and session, so nothing could tell them apart until an env-wide draft refused by a non-null-org caller existed. noteis wire-visible.auditMetaItemmaps it ontoGET /api/v1/meta/:type/:name/audit, so the denied row carriesclientFacingFailureText, note.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 fromfailed[].errorwould reach a client through a second door. The raw sentence stays inconsole.warn, where an operator reads it.code: 'batch_aborted'is one fixed value, not the lower-cased causal code. The auditcodecolumn is a documented closed set (ADR-0112 D6b); lower-casing whatever aborted the batch would turn it into an open set growing with the error catalog. The causal code rides innote, and the declared set insys-metadata-audit.object.tsis updated so declared still equals written.
Stated plainly rather than discovered later
The allowed side's placement is under-determined by this suite, and that is inherent rather than an oversight: for a committed batch both placements are observationally identical. It sits in Phase 2 anyway because one route must not audit its two outcomes under two different durability rules — an argument that lives in a code comment, not in a red test.
Residue
- The lock/conflict denial audit rows roll back with the batch on publishPackageDrafts — a refused item in a package publish still leaves no trail #8594 — the lock/conflict denial rows written from inside
promoteDraftForPublishare inside this route's transaction and roll back with the batch (measured here:item_lockedattempted, then gone). A refused batch is no longer silent, but the inner verdict's own vocabulary andlock_stateare still lost. - publishPackageDrafts' pre-flight refusal writes no audit row — a package publish rejected before the transaction leaves no trail #8595 — the pre-flight refusal (namespace-prefix violations) returns above the transaction, so it is reached by neither of this PR's two sites and still writes nothing.
Generated by Claude Code
- Both rows are keyed on the draft's own org, not the request's active org.
What
publishPackageDrafts(behind Studio's "publish whole app",POST /packages/:id/publish-drafts) promotes every draft in a package and writes nosys_metadata_auditrows at all — neither the allowed-outcomepublishrows nor thedeniedrow for a refusal.Found while implementing #7748, which fixed the same gap on the single-item routes (
publishMetaItem,rollbackMetaItem, and all four 409METADATA_CONFLICTsites). Not fixed there, deliberately — see below.Why it was left out of #7748
publishPackageDraftscallspromoteDraftForPublishdirectly (notpublishMetaItem), and it does so inside oneengine.transaction()— the ADR-0067 D2 "a commit cannot half-land" invariant. That makes the batch case a genuinely different contract from the three sites #7748 touched:save,delete) write after their repository transaction has closed. draft-publish-lifecycle: the metadata audit trail records onlysave— publish, rollback and the 409 conflict denial never write a row #7748's newpublish/rollbackrows were placed to match that.save— publish, rollback and the 409 conflict denial never write a row #7748 exists to close.So the correct placement for the batch path is Phase 2 (after the transaction commits), driven off the
promoted[]array — a different edit from #7748's, with its own test, rather than something to smuggle into that card.Repro sketch
POST /packages/:id/publish-drafts— answers 200, drafts go active.GET /api/v1/meta/:type/:name/auditfor any published item — nopublishrow.(Compare: after #7748, the same item published one-at-a-time does get its row.)
Notes for whoever takes this
sys_metadata_auditschema already declarespublishas anoperationoption — no schema work needed.save— publish, rollback and the 409 conflict denial never write a row #7748 documented: most multi-table fake engines in this repo openinsertwithif (table === 'sys_metadata_audit') return { id: 'audit_skip' };, which makes every audit assertion pass for the wrong reason.packages/metadata-protocol/src/protocol.lifecycle-audit-rows.test.tshas a harness that genuinely persists audit rows and separates "attempted" from "landed" — reuse its shape.Backlink: #7748 (single-item routes, fixed).