Uh oh!
There was an error while loading. Please reload this page.
fix(metadata-protocol): fold publish Phase 2's object-table lookup at the producer - #8867
Conversation
… the producer `runPublishSideEffects` took both a folded `singularType` and an unfolded `requestType`, and `ensureObjectStorage` was the sole consumer of the unfolded one — a spelling-tolerant lookup one layer below a boundary that already folds, the shape `canonicalMetaType`'s header has rejected since #4432. Pass `args.singularType` and drop `requestType` from the parameter list entirely, so no future consumer can reach for the unfolded value. Behaviour is unchanged: `type` is read only by `ensureObjectStorage`'s guard and never reaches `syncObjectSchema`, which takes `name` alone. Also corrects the in-tree comment that claimed the `'objects'` limb was unreachable because "both of its call sites stand behind a fold". That reason was false — the second call site sits inside `runPublishSideEffects`, whose batch caller does not fold. The conclusion held for an unstated reason: a plural row at rest cannot be promoted (`whereFor` emits the folded singular with no at-rest fallback), so the batch aborts with `NO_DRAFT` before Phase 2. That is now measured rather than asserted. Adds the batch publish path's first coverage of the table-DDL side effect. Part of #8820 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8
…ure-object-storage-objects-limb
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
…ure-object-storage-objects-limb
`protocol-publish-package-drafts.test.ts` pinned the args object
`publishPackageDrafts` hands `runPublishSideEffects`, asserting
`requestType: 'object'`. That field was removed in this branch —
`ensureObjectStorage` was its only consumer and now reads the folded
`singularType` every other consumer in the helper already read.
The draft in this case is spelled singular either way, so the assertion
pins the same fact under the surviving field name. The case stubs
`runPublishSideEffects` out entirely (`mockResolvedValue({})`), so it
pins the ARGS seam and never exercised the table-DDL side effect.
Part of #8820
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8Uh oh!
There was an error while loading. Please reload this page.
Fixes#8820
runPublishSideEffects(publish Phase 2) took two type parameters — a foldedsingularTypeand an unfoldedrequestType— andensureObjectStoragewas the sole consumer of the unfolded one. That is a spelling-tolerant lookup one layer below a boundary that already folds, the shapecanonicalMetaType's header has rejected since #4432.This passes
args.singularTypeand removesrequestTypefrom the parameter list entirely, so no future consumer can reach for the unfolded value.This is a refactor, not a fix — and the card's stated reason was wrong
ensureObjectStoragereadstypeonly in its guard; it never reachessyncObjectSchema, which takesnamealone. Before:'objects'passes limb 2, callssyncObjectSchema(name). After:'object'passes limb 1, callssyncObjectSchema(name). Byte-identical side effect, no public surface touched (the method is private,requestTypewas a private args field).The card proposed deleting
ensureObjectStorage's&& type !== 'objects'limb on the premise that "both call sites stand behind the/metafold". That premise is false, and an in-tree comment asserted it too. The helper's second call site sits insiderunPublishSideEffects, which has two callers:publishMetaItem— folded at the/metaboundary.publishPackageDrafts— not folded. It passes the draft row's storedtype, andlistDraftsapplies no fold (sys-metadata-repository.tsmapstype: row.typestraight through).Counting the helper's own call sites is not the reachability question.
Why the conclusion nevertheless held — measured, not argued
A draft row stored under a plural
typecannot be promoted at all.promoteDraftForPublishaddresses it by the folded singular, andSysMetadataRepository.whereForemits that spelling verbatim with no at-rest fallback, so the promote raisesNO_DRAFTand the all-or-nothing batch (ADR-0067 D2) aborts before Phase 2 ever runs. That is now pinned rather than assumed.So the limb was unreachable — but nothing in the type system, the call graph, or any comment said why, and the helper's parameter list advertised the opposite. The safety of the arrangement rested on an unstated invariant three frames away, in a different file. This removes the parameter instead of restating the invariant.
The coverage gap is the sharper claim
The batch publish path had no coverage of the table-DDL side effect at all. Measured at
fd6bdf89f: deleting the'objects'limb and running the whole package gave 98 files / 1444 tests green — before and after. A green suite could not tell the two states apart, so "the existing publish/save coverage stays green" could not have licensed the deletion. The new test file closes that gap.Precise about one thing:
objectql'sprotocol-publish-package-drafts.test.tsdid already touch this seam, but it stubsrunPublishSideEffectsout wholesale (mockResolvedValue({})) and pins the args, so it never exercised the DDL step. The claim above is about the side effect, not the seam.Ablations — direction predicted BEFORE each run
'objects'limb deleted'objects'Ablation 2 is the one that matters for follow-up: with the producer folding, the limb is genuinely dead, so its deletion becomes a true no-op provable by this file. Ablation 3 is the non-vacuity control — it fails as
expected [] to deeply equal [ 'invoice', 'ticket' ], i.e. the tests really do observe this helper drivingsyncObjectSchemaunder the folded spelling, rather than passing for want of an assertion.⛔ Deleting the limb is deliberately NOT in this PR. It is a separate, now-provable follow-up, so each step stands on its own claim. The
dropObjectStoragetwin at:11542carries the identical limb and is untouched here.One consumer followed the rename
packages/objectql/src/protocol-publish-package-drafts.test.tsassertedtoMatchObject({ requestType: 'object', name: 'course' })on the args seam.requestTypeno longer exists, so that assertion now readssingularType. The draft in that case is spelled singular either way, so it pins the same fact under the surviving field name — the expectation did not move, only the field name it reads.This was a real red on
Test Core (2/3), not a flake: my first verification ran only--filter @objectstack/metadata-protocol, while that shard carriesobjectql, a consumer of the changed package. The scoping error was mine; the consumer sweep below is the corrected verification.Verification — union re-run at
e1a06ad8f(current head, no commits since)Consumer sweep (downstream direction — every package whose tests exercise
ObjectStackProtocolImplementation), all green:metadata-protocolobjectqlrestruntimespecclientplugin-approvalsplugin-authplugin-emaildogfooddogfoodresolves the code under test fromdist, so it ran against a rebuiltmetadata-protocol— verified directly (requestTypeoccurs 0 times indist/index.js, built after the source commit).metadata-protocoldeclares notypecheckscript (it is a ledger package), so a green typecheck would say nothing here. Ratchet measured directly on a built workspace:pnpm check:type-check-debt --re-measure— 33 ledger entries re-measured, 1926 raw tsc errors, none above its recorded number. No ceiling raised. The one surplus it reports (@objectstack/lint, -1) is pre-existing and another package's ledger; left alone.scripts/pm/dispatch-gates.mjs, all green:check:cross-package-test-inputs,check:durability-log-level,check:filter-alias-parity,check:query-options-erasure,check:type-check-coverage,check:nul-bytes.check:engine-double-contract— this PR adds a fake engine, which no path derivation can name. Green at "264 pinned"; the double routesupdate/deletethroughassertEngineUpdateDispatch/assertEngineDeleteDispatch, so it is pinned and needed no ledger entry.No changeset
No user-visible change: a private method's parameter list, a comment, and two test files. The PR declares no release of its own, which is the changeset gate's own stated criterion for exemption. The
skip-changesetlabel was applied by the PM seat.Out-of-scope findings filed while measuring, both unassigned: #8858 (batch audit row records the raw stored spelling) and #8862 (a third tolerance in this file that consumes the plural rather than only guarding on it — reachability explicitly unmeasured, and it overlaps #8819's live region).
Generated by Claude Code