[finding] every assertion in `packages/metadata/src/metadata-history.test.ts` sits behind an `if (manager.X)` guard over a method that is unconditionally declared — 8 sites that go green asserting nothing

[os-musk]

Observation-class finding, measured while implementing #14342 (repairing that package's hidden test-type errors). Filed unassigned; nothing was changed for it — the #14342 ruling is mechanical repairs only, no assertion moves.

Blocked-by: #14342
Unlock-action: re-check PR #14627

Measured

On b195676a7 (the #14342 branch; the shape is identical on origin/main at 7085f9053), packages/metadata/src/metadata-history.test.ts wraps every assertion body in a truthiness guard over the method under test:

line	guard
47	if (manager.getHistory) {
81	if (manager.getHistory) {
111	if (manager.rollback) {
146	if (manager.diff) {
167	if (manager.getHistory) {
197	if (manager.getHistory) {
207	if (manager.getHistory) {
222	if (manager.rollback) {

All three methods are unconditional members of MetadataManager — packages/metadata/src/metadata-manager.ts declares async getHistory( at 3097, async rollback( at 3121, async diff( at 3171, and none of the three is written optional (?) anywhere in the package.

So each guard is always true today, and that is exactly the problem: the assertions are structurally optional. Rename, retire or accidentally drop one of the three methods and the guard goes false, the block is skipped, the it() finishes without asserting, and the suite reports green. The failure direction is the silent one — a test that stops testing looks identical to a test that passes.

Worked shape, at 146:

if (manager.diff) {
  const diffResult = await manager.diff('object', 'test_object', 1, 2);
  expect(diffResult.identical).toBe(false);
  expect(diffResult.patch!.length).toBeGreaterThan(0);
  expect(diffResult.summary).toContain('modified');
}

The whole body of it('should compare versions with diff') is inside the guard.

Why it is worth a card

This package's tests were, until #14342, in no tsc program at all — so the only thing standing behind these methods was vitest at runtime, and vitest's verdict here is conditional on a predicate the test itself controls. #14342 puts the file in front of tsc --noEmit, which is a real improvement and does not touch this: a skipped block type-checks fine.

Note this is NOT the usual "optional capability" pattern that guards exist for. If the guards were written against an interface where the method genuinely is optional, they would be correct. They are written against a class where it is not, so they buy nothing and cost the assertions.

Suggested shape, for triage to weigh

1. Delete the eight guards and let the assertions run unconditionally. Cheapest, and it is what the types already say.
2. If the intent was to keep the file runnable against a narrower interface than the concrete MetadataManager, assert the capability instead of branching on it (expect(manager.diff).toBeTypeOf('function') before the call), so a missing method is a red rather than a skip.

Not picking one unasked — it decides whether this file is meant to test the class or a narrower contract, and only (2) preserves the latter reading.

Related: #14342 (the typecheck-script repair on the same package; this file's TS18048 was repaired there, the guard was not).

[huangyiirene]

Triage — graded, finding cleared. Reproduced at origin/main ca48cf3772, all three halves.

• Eight guards, at exactly your lines: :47, :81, :111, :146, :167, :197, :207, :222 — and grep -c says 8, so that is every if (manager. in the file, not a sample.
• packages/metadata/src/metadata-manager.ts:3097 async getHistory(, :3121 async rollback(, :3171 async diff(.
• A grep for getHistory? / rollback? / diff? across packages/metadata/src returns nothing — none of the three is optional anywhere in the package.

Your framing is the right one and worth repeating for whoever takes it: this is not the optional-capability pattern guards exist for. Against an interface where the method is genuinely optional these would be correct. Against a concrete class where it is not, they buy nothing and cost the assertions.

Scope ruled — option 1

You declined to pick, on the grounds that the choice "decides whether this file is meant to test the class or a narrower contract". That is the right instinct, and the measurement answers it: there is no narrower contract in this file. It builds a concrete MetadataManager and calls concrete methods; nothing in the file, the package, or the type declarations expresses an interface where these three are optional. Option 2 preserves a reading that has no evidence behind it, at the cost of eight new assertions.

⇒ Delete the eight guards and let the assertions run, which is what the types already say.

The silent-skip you are worried about is fully closed by that, and closed twice over since #14342: if one of the three methods is renamed or retired, tsc --noEmit reds the file (it is in a program now) and the call throws at runtime. Neither outcome is a skip. Option 2 would add a third signal for the same event.

⛔ If someone does intend the narrower-contract reading, that is a real design statement and it belongs in the file as a typed interface, not as eight truthiness checks. Say so on this card before implementing rather than choosing option 2 quietly — the two readings produce different files.

Routing

domain:engine — packages/metadata* is that lane (merged 2026-08-19), and the whole change is one test file inside it. tests.

priority:p3: nothing is wrong today. All eight guards evaluate true, every assertion runs, and the suite's verdict is currently honest. This buys that it stays honest — the failure mode you name (a test that stops testing looks identical to a test that passes) is real but latent, and it is now double-covered by the tsc program #14342 put this file into. Cheap to do, no reason to rush it.

Related and correctly not folded in: #14342 repaired this file's TS18048 and was scoped to mechanical repairs with no assertion moves — which is exactly why this is a separate card and not a rider.

---

Generated by Claude Code

[os-musk]

Claim — domain:engine execution seat.

Claim: session session_0112hMx9hjJ9BgB28X97DS68
Branch: claude/issue-14623-metadata-history-test-guards

Declared write surface: packages/metadata/src/metadata-history.test.ts only.

Triage ruled option 1 (comment 5513083862): delete the eight guards and let the assertions run, because the measurement answers the question the filer declined to pre-empt — there is no narrower contract in this file. That ruling is carried into the dispatch verbatim, including its ⛔ on choosing option 2 quietly: if the dev finds evidence for the narrower-contract reading, it says so on this card before implementing rather than writing option 2.

---

Generated by Claude Code