fix(metadata-protocol): four read seams that failed no longer answer from an empty accumulator

[os-zhuang]

Fixes#8896

Four reads in @objectstack/metadata-protocol sat behind a bare catch that fell through — or, in one case, jumped — above a value the read was supposed to fill. Each handed its caller an answer indistinguishable from a legitimate one, with nothing logged and no field saying the answer was incomplete (ADR-0110 D3).

Each catch is discriminated by error type, never removed, through the shared isMissingTableError predicate (@objectstack/metadata/errors) — the same call DatabaseLoader, SysMetadataRepository and cascadeDeleteRelations (#8895 / PR #9006) already make. No new error code and no new response field, per the maintainer's #8833 ruling for this family.

The four seams, and what each now does

seam	the invented answer	benign case kept	everything else
seed-loader.tsloadExistingRecords	empty Map = "no existing rows" = write these rows	table not provisioned	propagates
protocol.tssearchAll	object dropped, totalHits / truncated still reported as a complete sweep	table not provisioned	propagates
protocol.tsfindReferencesToMeta	source type dropped from "what would break if I delete this"	inherited from getMetaItems	propagates (503)
protocol.tspublishPackageDrafts	fabricated revert-plan entry	sys_metadata not provisioned	propagates, publish refused

1. loadExistingRecords — the highest-severity one

The map is not a cache; it IS the write decision, in all three callers, and "empty" means write these rows. The upsert pre-load turns every update into an INSERT — and bulkWrite's attempt 2+ recheck, the only thing standing between an at-least-once retry and a duplicate of every row the first attempt already committed (framework#3149), is silently disarmed.

Measured pre-fix, both shapes, in the pins' ablation:

upsert pre-load: resolved { inserted: 1, updated: 0, errored: 0 } ← existing row invisible
retry recheck: expected [ …(2) ] to have a length of 1 but got 2 ← the duplicate

The swallowed comment ("Object may not have records yet") also named a case that cannot reach it: an object that merely has no rows answers find with [], it does not throw.

2. searchAll — and a comment that named a failure mode I could not find

The old comment read "RBAC denial or driver hiccup — skip silently per object". Measured on this tree, RBAC denial is not a failure mode of this seam: object-level authorization is enforced at the REST door (enforceAuth) beforesearchAll is reached, row-level security narrows find's result set rather than throwing, and nothing in-repo registers a beforeFind hook that denies by throwing. So the one real benign case is the unprovisioned table — routine here, since the registry lists every declared object whether or not a deployment provisioned it. Were a throwing permission hook added later, the ruling still applies: a read that could not run must not be answered "no matches here".

3. findReferencesToMeta — the one seam that gets no predicate of its own

This is where measuring per seam changed the answer rather than confirming it. This read goes through getMetaItems, which already performs exactly this discrimination (rethrowUnlessMetadataStoreUnprovisioned, #5532): the benign case returns normally, a real outage becomes a 503 SERVICE_UNAVAILABLE carrying the driver error as cause. A source type this deployment does not declare is not an error at all — listItems answers [].

So the only thing the catch { return; } could swallow was the 503 raised deliberately one line below it. The catch is simply gone; adding a second isMissingTableError here would be a second vocabulary of "benign", the exact debt @objectstack/metadata/errors exists to retire. Promise.all makes propagation right-shaped: the first rejection rejects the whole scan, so no half-scanned list reaches a caller — which matters because this list is the admin UI's "Used by" panel, and a short list reads as "nothing depends on it, safe to delete".

4. publishPackageDrafts — the comment/code contradiction, decided

The card asks which is wrong. Both were, in different directions:

• The code never omitted — it pushed { existedBefore: false, prevVersion: null }, the literal opposite of the healthy branch's existedBefore: !!activeRow. existedBefore: false means "revert = soft-remove", so reverting that commit DELETES an artifact whose previous version was supposed to be restored. A read that failed was answered with a value, and the value chosen was the destructive one.
• The comment's described behaviour would not have been correct either: an item omitted from the plan is simply not reverted, so the revert silently leaves the newly published version live while reporting the turn undone.

Both are one defect — a revert plan derived from a read that did not happen — so neither was the survivor. The comment now describes the discrimination the code does.

With sys_metadata unprovisioned there genuinely is no active row for anything, so existedBefore: false IS the truth and that push is kept byte-for-byte. Everything else propagates, and the loop's position is what makes that safe: the capture pass runs BEFORE Phase 1's transaction, so a refusal leaves the draft pending, no active row, no commit recorded. Verified on the post-#8986 tree.

⭐ What this fix made visible: 10 tests that were green on a path that never ran

This is the strongest argument for the change, and it was found by CI on the first push rather than by the seam analysis.

@objectstack/objectql owns the orchestration tests for publishPackageDrafts (it depends on @objectstack/metadata-protocol). Their fixtures built a protocol over an engine that never implemented findOne — so the ADR-0067 pre-publish capture threw TypeError: this.engine.findOne is not a function on every item of every case, the bare catch swallowed it, and the fabricated { existedBefore: false, prevVersion: null } was pushed in its place.

The consequence: no test in the repo had ever exercised the real revert-plan capture.existedBefore was false everywhere, not because a fixture said so but because the read crashed and the crash was hidden — including in the cases whose names claim end-to-end coverage (publishes every draft…, all-or-nothing…, wraps the batch in ONE engine transaction…). The existedBefore: true branch, which decides whether a revert restores or deletes, had never once been reached. A regression turning every revert into a deletion would have kept that suite green.

Discriminating the catch is what surfaced it: isMissingTableError(TypeError) is false, so the broken engine stops being invisible. The production behaviour is right and the fixtures were lying, so the repair is in the fixtures:

• makeProtocol now installs a real capture double — findOne answers from a seedable set of active rows and returns nullexplicitly for "no active row" (the two are now distinguishable, and only one is truthful), and insert records the commit row so the revert plan is observable instead of being swallowed a second time by recordCommit's own catch (recordCommit swallows a failed sys_metadata_commit write — the publish reports success and the turn is silently not revertible #9066).
• The four sites that replaced protocol.engine wholesale now spread the double instead — replacing it takes findOne away again and re-arms the same vacuity.
• A new case asserts both of the capture's real answers in one batch: an artifact with a pre-existing active row is recorded existedBefore: true, prevVersion: 4, its new sibling existedBefore: false, prevVersion: null, with the capture reads themselves asserted (first N, in draft order, each in the draft's own scope) so the values are evidence rather than defaults.

Verified not to be vacuity in a new form: with the double's answer ablated to always-null, that case goes red on the existedBefore: true entry; restored byte-identical afterwards.

Tests

Three new pin files in metadata-protocol, 23 cases, plus the objectql fixture repair. Every expectation is written against literals — the exact injected error object, its literal message and code, the literal 503 envelope, literal row counts, the existedBefore / prevVersion values read out of the stored commit row. Each failure assertion is paired with anti-vacuity controls in the same describe, and every benign branch carries proof that the injected throw actually fired. Both Postgres and SQLite phrasings, plus the column "x" of relation "y" does not exist superstring case that must stay loud.

Reverse verification, direction predicted before running: ordinary red, 8 of 23. Observed exactly that, with the predicted per-seam split (3 / 2 / 1 / 2) — every benign case and every positive control stayed green. Source restored byte-identical and re-run green.

Gates — union re-run at 6366de753, after the final commit

Suites: pnpm --filter @objectstack/objectql test — 212 files, 3730 tests, all passing (this is the suite that caught the fixture defect; a package-local metadata-protocol run structurally could not, because these tests live in the consumer's package). pnpm --filter @objectstack/metadata-protocol test — 111 files, 1555 tests, all passing.

Green: check:cross-package-test-inputs (+ the ci.yml script form) · check:durability-log-level · check:filter-alias-parity · check:changeset-gate-self-tests · check:objectui-changeset · check-adr-0087-registration · check-changeset-no-major · check-empty-changeset · check:nul-bytes · check:engine-double-contract · check:where-matcher · check:query-options-erasure · check:type-check-coverage · check:type-check-debt.

Re-deriving with scripts/pm/dispatch-gates.mjs against the actual diff added nine families the dispatch list did not name, and two caught real defects in my own test code:

• check:where-matcher — my publish fixture's matchesWhere implemented $or but read $and as a field name. Repaired by refusing the combinators the double does not implement, the convention 147 of 246 discovered matchers already follow. Baseline unchanged.
• check:type-check-debt — the new seed-loader pin's extensionless './seed-loader' import added a TS2835, drifting @objectstack/metadata-protocol 63 → 64. Fixed at the source with the nodenext-explicit specifier. The ledger was not raised — re-measure reports every entry at its recorded number.

check:durability-log-level was expected to be blind here, and the honest measurement is more specific than "byte-identical": its read-seam census moved 67 → 66, and the type-discriminated benign branch bucket stayed at 7 in both. The one seam that left the census is the deleted catch in findReferencesToMeta — there is no longer a seam there to count — while the three isMissingTableError discriminations stayed invisible to it exactly as predicted, because they return no expression. Neither number is certification, and the gate was not touched (#8845 deliberately did not extend it).

One measurement trap worth recording: after merging origin/main, protocol.driver-text-disclosure.test.ts failed in my worktree. It was stale dist/, not a regression — the merge brought #9030's MySQL leak-predicate change into @objectstack/types while my worktree still resolved the pre-merge build. Rebuilding the closure returned it to green; nothing in this PR touches it.

Scope

Carve-outs respected: getMetaDiagnostics (#8855, landed), diffMetaItem (#8833, landed), checkGovernance (#8906, domain:engine-core, not this lane). No edits to migrateStoredMetadata, recordMetadataAudit or applyObjectRegistryMutation. File surface extended by the PM this round to the two objectql fixture files, which are the consumer face of this change; no other objectql file is touched.

One out-of-scope finding filed unassigned as #9066: recordCommit's bare catch swallows a failed sys_metadata_commit write, so a publish reports success while the turn is silently not revertible. That is the write half of the same neighbourhood and a different function — searched first, no open duplicate.

---

Generated by Claude Code

[github-actions]

📓 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.

[os-zhuang]

🔴 CI red — Test Core (2/3). Diagnosis in progress; ⛔ NOT being called flaky.

PM, session session_01NTKPDRoynY8i3HmdSFUxFj. ⛔ The PR stays draft and out of the queue until this is understood. Posting what is measured, and explicitly what is not.

What is established

• The failing package is @objectstack/objectql, not metadata-protocol — Failed: @objectstack/objectql#test, 56/63 turbo tasks successful. ⛔ The job name is not the failure; this is read from the log body.
• main is green. Two completed CI runs on main at 2a20b99ad (08:04Z) and ced5c4912 (07:45Z) both success. ⇒ this is not a pre-existing red, and "it's red on main too" is not available as an answer.
• The diff can physically reach it.@objectstack/objectqldepends on@objectstack/metadata-protocol, and 8 objectql source files import it (engine.ts, registry.ts, metadata-facade.ts, …). objectql's test files don't import it directly, but that is irrelevant — the path runs through the source. ⇒ mechanism is open; this cannot be dismissed on reachability.
• The test step ran to completion (check-test-completeness: OK — 9 packages, 6020 tests declared and all 6020 accounted for), so this is not a job that died before test bodies ran. ⛔ A plain re-run is therefore not a legitimate response.

⛔ A hypothesis I formed and then killed, recorded so nobody re-walks it

The log is full of no such table: sys_metadata, and this PR deletesfindReferencesToMeta's per-matcher catch — so "the deleted catch now lets an outage propagate into packages/objectql/src/protocol-references.test.ts" looked compelling.

It does not hold.getMetaItems routes through rethrowUnlessMetadataStoreUnprovisioned, which treats an unprovisioned table as benign and returns empty — it raises the 503 only for other failures. So an unprovisioned sys_metadata is exactly the case that does not propagate, and those log lines are ordinary driver noise from tests that expect them.

That was pattern-matching a symptom into a story. Recording it because the story was plausible enough to have shipped a wrong "fix".

What is NOT yet known

The name of the failing test. I have not identified it, and I am not going to name a cause without it. A local reproduction is running against the exact PR head (43f0902fa) in a clean worktree — install, dependency-closure build, then the full @objectstack/objectql suite.

Next

Whichever way it lands, this ends with a pushed fix or a stated blocker, ⛔ never with a silent re-queue:

• reproduces locally ⇒ root-cause it, fix at source, re-verify, push;
• does not reproduce ⇒ that is a result, not a licence to re-run: it points at cross-package or ordering interference under the sharded runner, and I will say so with the evidence rather than shipping on a green retry.

⛔ No ledger will be raised, no test skipped or quarantined, and the PR does not go back in the queue on a re-run alone.

---

Generated by Claude Code

[os-zhuang]

🔬 Root-caused — reproduced locally, and it is a REAL regression, not a flake

PM, session session_01NTKPDRoynY8i3HmdSFUxFj. Reproduced at this PR's exact head 43f0902fa in a clean worktree (install → dependency-closure build → full suite). ⛔ No re-run was attempted; ⛔ nothing is being called flaky.

Test Files 2 failed | 210 passed (212)
Tests 10 failed | 3718 passed (3728)
TypeError: this.engine.findOne is not a function
at _ObjectStackProtocolImplementation.publishPackageDrafts (../metadata-protocol/src/protocol.ts:14217:54)

Failing: packages/objectql/src/protocol-publish-package-drafts.test.ts (9) and src/build-probes.test.ts (1) — all one cause, all in seam 4, the one this PR changed.

The change is CORRECT. The fixtures were wrong.

Those tests build a stub engine that never implemented findOne. Before this PR the bare catch swallowed the resulting TypeError and pushed the fabricated {existedBefore:false, prevVersion:null} — so the stub "worked" by accident. The discriminated catch now asks isMissingTableError(TypeError) → false → rethrows. A TypeError from a broken engine is precisely the class the #8833 ruling says must surface.

⛔ The discrimination will not be softened, TypeError will not join the benign set, and no test will be skipped or quarantined to get green. The fix is to the doubles.

⭐ What the red actually revealed — the more valuable half

All 10 of those tests were vacuous on this path. Because the swallow caught the missing findOne on every item of every run, existedBefore was fabricated false every time — so no test in this repository has ever exercised the real revert-plan capture, including the ones whose names claim to: "publishes every draft of the package and reports success", "all-or-nothing (ADR-0067 D2)", "wraps the batch in ONE engine transaction and rolls it back on failure".

They were green because of the defect this card fixes. That is a stronger argument for the change than the seam analysis alone, and it is exactly the shape this lane keeps re-learning: a passing test is not evidence its subject works.

⚠️ Why the dev's own verification could not have caught it — and why that is a structural fact, not negligence

The dev ran @objectstack/metadata-protocol (111 files / 1555 tests, green) and the reverse verification, both correctly. The tests for publishPackageDrafts — a metadata-protocol function — live in packages/objectql/. No amount of diligence inside the edited package reaches them.

⇒ The lane's standing lesson applies verbatim: "ran a downstream consumer" is not "covered the consumer face" — and here the consumer face contains the subject's own tests. ⚠️check:cross-package-test-inputs is green on this PR despite exactly this shape; whether it is meant to see it is being checked, and becomes a finding if it structurally cannot.

Disposition

Patch round dispatched to the same dev (context preserved, ⛔ not a re-dispatch). Declared file surface amended by me to include those two objectql test files — that is the consumer face of this change and is required to land, ⛔ not scope creep. The doubles must gain a real findOne, plus at least one assertion that the capture actually happened (an item with a pre-existing active row recorded existedBefore: true with its prevVersion) — otherwise the vacuity is merely restored in a new form.

⛔ The PR stays draft and out of the queue until @objectstack/objectql is green.

---

Generated by Claude Code