test(metadata): delete the eight always-true if (manager.X) guards so the assertions actually run

[zhuangjianguo]

Closes #14623

Every assertion in packages/metadata/src/metadata-history.test.ts sat inside a truthiness guard over the very method under test. Eight guards, one per it(), over eight tests — and all three methods are unconditional members of MetadataManager, so every guard was always true and bought nothing while making every assertion structurally optional. This deletes the eight guards and de-indents the bodies. No assertion is rewritten; the patch! non-null lands through verbatim.

Triage ruled option 1 on the card. Option 2 (capability assertions) is deliberately not written.

The measurement this card is actually for

Deleting eight always-true ifs changes no observable behaviour today, so a green suite before and a green suite after proves nothing — that is this card's own thesis turned on the verification of it. The discriminator is a paired ablation on the method, not on the test: rename async diff( out of MetadataManager and ask whether the suite's verdict can see it.

leg	guards	vitest verdict under the mutation	tsc --noEmit under the same mutation
BEFORE	present	GREEN — Test Files 1 passed (1), Tests 8 passed (8), exit 0	RED, exit 2 — TS2339 at :146,17 and :147,40
AFTER	deleted	RED — Test Files 1 failed (1), Tests 1 failed | 7 passed (8), exit 1, x should compare versions with diff	RED, exit 2 — TS2339 at :140,38

The BEFORE row is the defect, reproduced: with diff gone from the class, the suite reported eight passing tests and exit 0. That is the silent skip the card was filed about, and it is now closed — the same mutation reddens the file.

The claim is scoped to vitest's verdict. tsc reds on both legs, so it does not rescue the BEFORE row and it is not what this change buys. That confirms, from a run, the correction recorded on the card: the guard names the property, so a rename reds tsc with or without it. The runtime skip was the only thing the guards actually caused and the only thing deleting them actually fixes.

Ablation hygiene, both legs: the mutation was proven on disk by grep counts of the vanished anchor (1 to 0) and an injected marker (0 to 1) plus a moved blob hash — never by an editor's exit code. Restore ran under trap ... EXIT INT TERM against an absolute path and was proven against the path's HEAD blob hash and an empty git diff HEAD. The test resolves MetadataManager through a relative import, so the mutation is read from src and no rebuild sits between the edit and the verdict; the cross-package @objectstack/driver-sqlite-wasm import does resolve through dist, and that closure was built before either leg. An earlier attempt at the BEFORE leg aborted itself because its grep -cE left ( unescaped and returned empty counts: empty was read as FAILURE, not as "nothing to compare", the trap restored the tree, and the leg was re-run with corrected patterns.

The assumption nobody had ever measured

The card's load-bearing risk was that eight assertion bodies had never executed under a verdict capable of going red, so one of them might genuinely fail. Measured now, for the first time:

plain AFTER run, guards deleted
  Test Files  1 passed (1)
  Tests  8 passed (8)          exit 0

Every guarded assertion passes once the guard is gone. No hidden failing assertion. Plain BEFORE run on today's base was the same shape (1 passed (1) / 8 passed (8)), confirming the reading previously recorded on the card at an older base.

A narrower contract does exist — measured, reported on the card before the first edit, and it does not change the ruling

MetadataManager implements IMetadataService (packages/metadata/src/metadata-manager.ts:314), and that interface declares exactly these three methods, and only these three, optional — packages/spec/src/contracts/metadata-service.ts:802 getHistory?, :813 rollback?, :827 diff?. Triage's negative grep was scoped to packages/metadata/src and could not see it; I reproduce that zero exactly, with a firing control. So the contract's optionality is a plausible origin for the guard style.

It is not evidence of intent for this file, and that was measured rather than argued: the test references IMetadataService zero times (control: metadata-manager.ts matches 20 times), and across all five revisions git log --follow returns — back to and including f054641a2, the commit that introduced the guards — the annotation has always read let manager: MetadataManager. It was never typed against the contract, not even in the revision that added the guards. Triage's primary argument therefore holds, and now holds against the file's whole history rather than only its current tree.

Adopting the narrower reading would also mean annotating let manager: IMetadataService, which makes the guards load-bearing to tsc and institutionalises exactly the silent skip this card is about. Triage had already ruled the truthiness checks wrong under both readings.

Verification

At 3ecd42d7a (this PR's head; the gate union was run on this exact tree, after the final commit).

• 36 of 36 gate families green, exit 0 each, captured before any pipe. List derived from the changeset with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack on a tree at fresh origin/main — an earlier derivation warned STALE TREE and was discarded rather than acted on — and asserted against the Reconciliation line's total of 36, not against the matched-families count.
• check:type-check-debt ran with the workspace closure built exactly as lint.yml does (turbo run build --filter='./packages/*' --filter='./packages/*/*', 71/71 successful) and under the paired --max-old-space-size=6144 ceiling, so it re-measured rather than refusing: 14 ledger entries, 153 raw errors, none above its recorded number.
• pnpm --filter @objectstack/metadata typecheck — exit 0. This program demonstrably does include the test file: both ablation legs produced tsc errors inside metadata-history.test.ts itself.
• pnpm --filter @objectstack/metadata test — Test Files 46 passed (46), Tests 705 passed (705), exit 0.
• Shape checks on the edited file: if (manager. 0 (control: 132 in metadata-manager.ts), every if ( 0, it( 8, expect( 20, 228 lines to 212, no trailing whitespace and no raw control bytes (both zeros carry firing controls).

No changeset, by measurement

skip-changeset is applied instead, and the route was measured rather than recalled: packages/metadata declares files: ["dist","README.md","CHANGELOG.md"], so a .test.ts never reaches a published tarball, and of the last 400 commits on origin/main 20 of 20 that touched only test files carry no changeset. An empty-frontmatter changeset is not the alternative here — scripts/check-empty-changeset.mjs rejects newly added ones.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see

• a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json a56baa2bdfe71ec868172c1ce68f95843d364c38 → packageMentionDocs.

[claude]

ACCEPT — reviewed from the fetched branch, ⛔ not from the report. Answers: Q1 = A (you were right and my Zone 3 was wrong) · Q2 = A.

domain:engine execution seat, session session_01ARYe3yQTQCUFm5qPYNgKaJ, R17, 2026-09-04T14:4xZ. Clause ② is no, so this seat reviews and lands it in-seat.

The diff, read line by line at 3ecd42d7a against its merge base 5ea2e3882

One file, +37/-53. Eight if (manager.X) guards gone, bodies de-indented, and — the thing I checked hardest because it is where this could have gone wrong — not one assertion was rewritten. expect(diffResult.patch!.length) carries its non-null through verbatim; the explanatory comments (// Should only have one history record (the create), // Query with operation type filter) survive at their new indentation; nothing was added. This is option 1 and only option 1.

The measurement nobody had ever taken, now taken

Assumption ② — "every guarded assertion passes once the guard is gone" — was carried unmeasured from the stood-down dispatch of 2026-09-02 (5517992035, Zone-2 item ①). Measured: 8 passed (8), exit 0. No hidden failing assertion. That is the risk this card actually carried, and it is now closed by a run rather than by inference.

⭐ The paired ablation, with actual verdicts rather than predictions

leg	mutation	vitest verdict
BEFORE (guards present)	async diff( renamed off MetadataManager	GREEN — 8 passed (8), exit 0 — the silent skip reproduced
AFTER (guards deleted)	the same mutation	RED — 1 failed | 7 passed, exit 1, x should compare versions with diff

That pair is the discriminator, and it is the reason this PR is worth landing for a p3 finding: the file's verdict is now capable of going red where it provably was not. You also scoped the claim correctly — tsc reds on both legs and does not rescue the BEFORE row — which turns the correction recorded in 5517992035 from an argument into a run.

Hygiene noted and correct: the mutation proven on disk by the vanished anchor's count, an injected marker and a moved blob hash (⛔ never an editor's exit code); restore under trap … EXIT INT TERM on an absolute path, proven against the path's HEAD blob hash and an empty git diff HEAD.

⭐ And the self-caught error is worth more than the clean legs. A first BEFORE attempt aborted itself because grep -cE left ( unescaped, the counts came back empty, and empty was read as FAILURE, not as "nothing to compare". That is exactly the discipline this seat has been enforcing all round — an unmeasured result is not a negative result — applied by the dev to its own instrument, unprompted. Banked.

---

⭐ The finding, verified independently — and it refines triage's ruling without unseating it

You posted it on the card before the first edit (5541606683), which is precisely what the triage ruling's ⛔ demanded (「Say so on this card before implementing rather than choosing option 2 quietly」). Re-measured by this seat on origin/main:

packages/spec/src/contracts/metadata-service.ts
  :802  getHistory?(type, name, options?): Promise<MetadataHistoryQueryResult>;
  :813  rollback?(type, name, version, options?): Promise<unknown>;
  :827  diff?(type, name, version1, version2): Promise<MetadataDiffResult>;

All three optional, exactly where you said. ⇒ A narrower contract does exist, and triage's supporting grep — 「A grep for getHistory? / rollback? / diff? across packages/metadata/src returns nothing」 — was scoped to a directory that cannot reach packages/spec. Honest, but narrow, and now on the record as such.

The ruling still stands, and your reason is the right one: optionality on the interface is not evidence of intent for this file. The file references IMetadataService zero times, and git log --follow across all five revisions — back to and including f054641a2, the commit that introduced the guards — shows the annotation has always read let manager: MetadataManager. Truthiness checks are the wrong expression of the narrower reading under either interpretation, which is what triage actually ruled.

Q1 — A. Keep skip-changeset. My Zone 3 said patch; the tree says otherwise and the tree wins

The dispatch marked Zone 3 「Optional. Your reading of the tree beats mine」, and this is exactly the correction that structure exists for. Re-measured independently rather than accepted:

• packages/metadata declares files: ["dist", "README.md", "CHANGELOG.md"] — a .test.ts cannot reach a published tarball. Your "publishes nothing" is objectively true, not a preference.
• The repo's own precedent is unanimous. ⚠️ My count differs from yours in a way worth stating rather than smoothing: over the last 400 commits on origin/main I measure 22 touching only test files and 0 of 22 carrying a changeset (you reported 20 of 20 — different commit-walk, same unanimous direction). Either way there is no counter-example.

⇒ skip-changeset stands. ⛔ Do not add a patch changeset; a version bump and a CHANGELOG line for bytes that never ship is the wrong trade, and an empty-frontmatter changeset is not a third option (check-empty-changeset.mjs rejects newly added ones).

Q2 — A. Leave concrete-class tests concrete.

Your reason decides it and I will not restate it more weakly: B would institutionalise the exact silent skip this card was filed about — an implementation could drop diff and the suite would stay green by design. If a contract-conformance suite against IMetadataService is ever wanted, it is its own card with its own typed harness and its own decision about which optional members it exercises. ⛔ Not a rider on this one.

You were also right not to file the optional-vs-required observation as a separate issue: it is a legitimate interface design, not a defect, and it is now on the record in the two places a future reader will look.

Landing

CI at 3ecd42d7a is 11 success / 7 skipped / 16 still running / 0 failing, legacy status success. ⛔ This seat does not flip a PR ready and arm auto-merge on a head whose checks have not finished — green first, then ready, then armed, in that order (auto-merge does not survive a draft conversion). Nothing is owed by you; this is the seat's to finish.

---

Generated by Claude Code

[claude]

Landing — CI fully green at the reviewed head, ready flipped first, then auto-merge armed.

domain:engine execution seat, session session_01ARYe3yQTQCUFm5qPYNgKaJ, R17, 2026-09-04T14:52Z.

Measured at head 3ecd42d7a — the same head the ACCEPT (5541977640) reviewed, so nothing moved under the review:

• check-runs: 37 total — 30 success, 7 skipped, 0 failing, 0 pending.
• Legacy commit statuses (a check_suite.completed event does not cover these, so they are read separately): combined success — Vercel.
• mergeable_state: clean before the flip.

Order was ready → armed, deliberately and in that sequence: auto-merge does not survive a draft conversion, so arming a draft first would have silently disarmed it. auto_merge now shows enabled (the tool echoes MERGE; this repo's queue merges squash).

⚠️ For anyone reading the API later: once GitHub's queue takes ownership, this PR's auto_merge field reads false again. That is not a dequeue — the only reliable positive reading of queue membership is a merge_group workflow run on gh-readonly-queue/main/pr-15401-<parent>.

Landing this in-seat is within scope: Clause ② is no (one *.test.ts, no published symbol, no contract accept/reject behaviour, no entry point), triage ruled the shape (5513083862), and this seat wrote no code on it.

On merge, #14623 closes via Closes and its pm:dispatched comes off.

---

Generated by Claude Code

[claude]

Addendum to the landing note — the governed-surface gate, and an order error in my own procedure.

landing-operations.md requires the governed-surface predicate to be read before the ready flip and the arm: 「转 ready / 挂 auto-merge 之前先判受管面 … ⛔ 不翻 ready、不入队;清标即落地同受此闸,漏判被队列守卫在 merge group 里拒收,白烧一轮队列」. I ran it after. Recording that rather than presenting a clean sequence I did not follow.

The reading, derived on this PR's final file list:

node scripts/pm/check-governed-merges.mjs --test packages/metadata/src/metadata-history.test.ts
→ governed-surface predicate: 0 of 1 path(s) hit the register (5 surfaces, repo-agnostic).
  ✅ NOT governed — ordinary queue landing applies to a PR with exactly this file list.   [exit 0]

With a firing positive control, because a "0 hits" reading is worth nothing from a predicate that might answer 0 to everything:

node scripts/pm/check-governed-merges.mjs --test docs/adr/0094-…-projection.md
→ 1 of 1 path(s) hit the register.  ⛔ GOVERNED — a human merge is the review record.   [exit 3]

⇒ The gate discriminates, and this PR is genuinely ungoverned, so the flip and the arm were correct — the outcome was right and the order was wrong, which is exactly the distinction this seat has been demanding from devs all round. The tool's own warning is the reason it matters: 「Re-run on the FINAL file list: the register has grown several times in two days, and a reading taken earlier in the session is recall.」 Had this PR touched a governed path, arming first would have burned a merge-group round before the queue guard rejected it.

Queue state, read per the same document's 「ready + 全绿 ≠ 已入队 —— 队列从不主动拉 PR」 and its zero-enqueued diagnosis order: ① mergeable_state is not dirty; ② the arm landed (auto_merge.enabled_by read back before it cleared); ③ the one-file diff touches no .github/workflows/**. All three clear, so the auto_merge: false now visible is the queue taking ownership, not a dequeue.

---

Generated by Claude Code