[finding] diffMetaItem awaits a full historyMetaItem read and discards it — a dead round trip on every live diff request

Found while implementing #8671 (redacting emitted credential values on the metadata diff endpoint). Filed rather than fixed — out of that card's scope, which is region-declared to the redaction change inside diffMetaItem. Not assigned; recording, not starting.

What is dead

ObjectStackProtocolImplementation.diffMetaItem (packages/metadata-protocol/src/protocol.ts) opens by awaiting a complete history read:

const events = (await this.historyMetaItem({
    type: singularType,
    name: request.name,
    ...(orgId ? { organizationId: orgId } : {}),
})).events;
const versions = events
    .map((ev: any) => (ev as any).version as number | undefined)
    .filter((v): v is number => typeof v === 'number');

versions is then used exactly once, to be thrown away:

const _used = versions; void _used;

Nothing else in the function reads events or versions. The code immediately below re-fetches the same history through the repository instead, and its own comment says why:

The historyMetaItem MetadataEvent shape doesn't carry the per-(type,name) version directly — re-fetch via the repo to read the underlying history rows with their version.

So the re-fetch is deliberate and correct; what looks unintended is that the original call was left in place rather than removed. historyMetaItem is not free — it resolves the overlay repo and drains an async generator over sys_metadata_history (SysMetadataRepository.history), which issues its own engine.find on the same table the re-fetch then queries again.

Why it is worth a card

GET /api/v1/meta/:type/:name/diff is routed and live, so every request pays for two reads of sys_metadata_history where one is used. That is a modest cost, and the reason to file is less the cost than the shape: void _used is the marker of a value someone kept deliberately, so the next reader has to re-derive that it is genuinely dead rather than load-bearing. It also reads as a suppressed-lint artifact, which is exactly the input that makes an agent reason confidently from dead code.

One thing to decide rather than assume, which is why this is not a blind deletion: historyMetaItem has an early return — it answers { events: [] } for a type that is neither isOverlayAllowed nor isRuntimeCreateAllowed. It does not throw for such a type, so removing the call should not change diffMetaItem's behaviour for anyone; but that gate is the one thing worth confirming before the line goes, since it is the only observable the dead call could still be providing.

Not a duplicate

Searched open issues by keyword (diffMetaItem, historyMetaItem, dead code, discarded result, wasted read) and by file path before filing. The three near matches are all closed and concern saveMetaItem (#5783, #5264, #4754), not this function.

Related: #8671.

Generated by Claude Code

Finding triage — graded pm:queue, type Task. Lane domain:metadata confirmed (lands packages/metadata-protocol).

Premise verified on origin/main @ f039a4c, ⛔ not inherited:

const _used = versions; void _used; — protocol.ts:15431, the sole occurrence of _used in the file.
diffMetaItem opens at :15349, historyMetaItem at :12511 — so the discarded call is inside the function the card names, and the re-fetch below it is a different path.

Graded promote, on the third evaluation axis rather than the cost. The round trip itself is modest; void _used is the part worth spending a card on, because it is the exact shape that makes an agent reason confidently from dead code — a deliberate-looking marker on a value that is not load-bearing. The card's own framing of this is correct and is why it grades above polish.

⛔ Serial constraint — this card must not be dispatched yet

#8671 is pm:dispatched and its declared region is the redaction change inside diffMetaItem — the same function, and #8671 is a security card on the v17 surface. Dispatching this one concurrently puts two agents in one function for a cosmetic gain.

Disposition, recorded as PM discretion (ordering between technical tasks is a named non-escalation class, ⛔ not sent anywhere): dispatch after #8671 lands, and re-verify :15431 on the merged ref first — #8671 edits this function, so the line number and possibly the surrounding read are exactly what its diff may move.

packages/metadata-protocol/src/protocol.ts now carries six cards (#7643 · #8660 · #8786 · #8783 · #8671 · this one). The claim on this card must declare region-level file surface (diffMetaItem only). ⚠️ domain:metadata seat #6367 is vacant, so nothing is currently holding that file's serial queue — the constraint above lives on the card precisely because no seat is holding it in working memory.

The one thing the dev must confirm, not assume

The card names it and it is the right call: historyMetaItem's early return answers { events: [] } for a type that is neither isOverlayAllowed nor isRuntimeCreateAllowed (both predicates confirmed at protocol.ts:9827 / :9838). It does not throw, so removing the call should be observationally inert — but that gate is the only behaviour the dead call could still be providing, so it is a premise to test, not a fact to inherit. If it turns out the early return is doing something observable here, stop and report the fork; ⛔ do not silently keep the call and delete only the void _used line, which would preserve the cost and remove the marker that explains it.

Generated by Claude Code

Claim: PM loop round 6
Session: session_01XeQRiAa7vYRVX5Fog7Zby8
Branch: claude/issue-8798-diff-dead-history-read
Worktree: objectstack-issue-8798
Domain: domain:metadata
File surface: packages/metadata-protocol/src/protocol.ts — region-declared: diffMetaItem only, from :15459 — plus co-located pins. ⛔ Stop on breach and report.
Container & model: S, mode:subagent, model: opus — above the sonnet floor because the deletion is only safe if a gate turns out not to be load-bearing, and that is a judgement.
Serial constraints cleared: ⛔ triage's hold is discharged — #8671 (the security card whose region was inside this same function) landed as PR #8797, merged 04:21Z, confirmed on origin/main. No live editor in this file; #8783's PR #8817 (region :698) is in the merge queue and is ~14,800 lines away.

Triage asked for the line to be re-verified on the merged ref — done, and it moved

Triage measured :15431 and warned that #8671 edits this function so its diff "may move exactly that line." It did. Re-measured on origin/main b705a6ce:

:15459   async diffMetaItem(request: {
:15578   const _used = versions; void _used;      (was :15431 — moved +147 by #8671)
:12577   async historyMetaItem(request: {

_used remains the sole occurrence in the file. Re-derive from the tree yourself — this file has moved on every one of tonight's six landings.

Why this grades above polish

The round trip itself is modest. The part worth a card is void _used — a deliberate-looking marker on a value that is not load-bearing. That is precisely the shape that makes a later agent reason confidently from dead code: it reads as "someone thought about this and kept it on purpose."

⛔ The one premise to TEST, not inherit

The card and triage both reason that historyMetaItem's early return answers { events: [] } for a type that is neither isOverlayAllowed nor isRuntimeCreateAllowed (predicates at protocol.ts:9827 / :9838), and that it does not throw — so removing the call should be observationally inert.

That gate is the only behaviour the dead call could still be providing. Test it rather than accepting it. If the early return turns out to be doing something observable on this path, stop and report the fork.

⛔ And do not take the tempting half-measure: keeping the call while deleting only the void _used line would preserve the entire cost and remove the one marker that explains why it is there. Either the call goes or the finding is falsified.

Verification bar

If the premise holds, pin that a diff on a type that would take historyMetaItem's early return still answers identically after the call is removed — that is the case the dead call was notionally covering, and it is the only one that can distinguish "inert" from "load-bearing". A test that only exercises an ordinary type proves nothing about the deletion.

Take gate names from node scripts/pm/dispatch-gates.mjs against your actual changed paths and treat the output as a lower bound — several PRs tonight hit gates that fire on what a diff contains rather than which paths it touches. ⚠️ metadata-protocol hides its tests from tsc, so a green pnpm typecheck proves nothing about its TEST_DEBT ledger; measure that package's ledger directly if you add a test file.

Generated by Claude Code

{
  "issue": 8798,
  "status": "done",
  "branch": "claude/issue-8798-diff-dead-history-read",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/8841",
  "premise_still_valid": true,
  "summary": "Removed the discarded `historyMetaItem` round trip from `diffMetaItem` — both the awaited call plus its `versions` mapping and the `const _used = versions; void _used;` marker, the whole thing rather than the half-measure. The premise was measured, not inherited: with a read-counting stub, `historyMetaItem` early-returns `{ events: [] }` for `field` with ZERO engine reads, and `diffMetaItem` serves that same gated-shut type a full, correct diff regardless — it reads `sys_metadata_history` directly and never consults either predicate, so the gate never reached this function's output. Line numbers re-derived on the tree (`_used` at :15578, `diffMetaItem` :15459, `historyMetaItem` :12577); the two predicates had ALSO moved, to :9893/:9904 rather than the :9827/:9838 the triage measured. One measured behaviour delta the card did not anticipate, detailed in open_questions: the discarded call was unguarded, so a history-table outage was fatal for gated-open types while gated-shut types fell into the `try`/`catch` below — one outage, two answers, decided by an unrelated authorization gate; every type now takes the `catch`.",
  "tests": "All runs AFTER the final commit, clean tree at HEAD `4dc52951b` (`main` merged at `c15eb237f`; `git status --porcelain` empty). Suites: metadata-protocol `96 files, 1413 tests passed`; objectql `210 files, 3671 tests passed`; rest `118 files, 1948 tests passed` (both consumers included because they reference `diffMetaItem`). eslint on both changed files clean; `typecheck` on rest+objectql clean. NEW TEST: `packages/metadata-protocol/src/protocol.diff-dead-history-read.test.ts`, 6 pins. REVERSE VERIFICATION with the direction predicted BEFORE running — reinstated the discarded call on top of the committed fix, expected exactly 2 of 6 red; observed exactly that: the read-count pin (`AssertionError: expected 2 to be 1`) and the gated-open outage pin. The other 4 stayed green both ways, which IS what 'observationally inert' means and is why those 4 alone could not have justified the deletion. Restoration proven byte-identical via empty `git status --porcelain`, not merely a clean `git diff HEAD`. GATES from `node scripts/pm/dispatch-gates.mjs` on actual changed paths, all green: check:nul-bytes, check:where-matcher, check:engine-double-contract (252 pinned — the new double routes through `assertEngineDeleteDispatch`/`assertEngineUpdateDispatch` from day one, so no ledger entry was needed), check:filter-alias-parity, check:durability-log-level, check:cross-package-test-inputs, check:changeset-gate-self-tests, check:objectui-changeset, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check:query-options-erasure. TEST_DEBT measured directly per the dispatch warning that this package hides its tests from tsc: `check:type-check-debt --re-measure` on a fully built closure — 33 ledger entries, 1926 raw errors, none above its recorded number; the new test file adds ZERO to this package's frozen ceiling of 63 (the only surplus is a pre-existing -1 in @objectstack/lint). No ablation in this card, so nothing to rebuild for one.",
  "open_questions": [
    {
      "question": "The deletion carries one behaviour change on the outage path, which the card did not anticipate and which a reviewer should consciously accept. The discarded call was UNGUARDED while the read below it sits in a `try` whose `catch` reads 'history table unavailable — fall through with empty list'. Measured with a stub whose history read throws: pre-change, a gated-open type (`view`) THREW while a gated-shut type (`field`) RETURNED an empty diff. One outage, two answers, decided by an authorization gate unrelated to reading history, and documented nowhere. Post-change every type takes the `catch`. Should this land as-is?",
      "options": [
        "A — land as-is (what the PR does). The `catch` is the function's only stated intent for that failure, so the change makes behaviour uniform and matches the written contract; the inconsistency was an artifact of the dead call's position, not a design.",
        "B — hold this PR until #8833 rules on whether swallowing the outage is correct at all, then land both together.",
        "C — preserve today's split by re-guarding the throw for gated-open types, keeping the round trip removed but the 500 intact."
      ],
      "recommendation": "A. C is contract-first backwards — it would fossilize a type-dependent failure mode nothing declares, purely to preserve an accident. B serialises a modest, fully-pinned cleanup behind an unrelated contract decision; the outage question is genuinely separate and is already filed with its own measurement, including why the existing read-seam-invention gate cannot see this shape (it keys on `ts.isReturnStatement`, and this catch body is empty and falls through). Landing A also makes #8833 strictly easier to act on, because after this the seam has exactly one behaviour to reason about instead of two. Flagged rather than silently absorbed because it IS a real delta on a live endpoint."
    }
  ],
  "out_of_scope_findings": [
    "filed as #8833: `diffMetaItem` answers 200 with an empty diff when `sys_metadata_history` is unreadable, so an outage is indistinguishable from 'nothing changed' — filed unassigned and unlabeled for triage, with three options and no recommendation since it is a response-contract decision; a follow-up comment records the measured reason no existing gate catches it (`check-durability-degradation-log-level.mjs`'s read-seam-invention rule scans this package but keys entirely on `ts.isReturnStatement`, and this catch body is empty and falls through, so the invention happens later when the never-filled accumulator is read as a real answer)."
  ]
}

Generated by Claude Code

PM ruling on the outage-path delta: A — land as-is. Recorded rather than absorbed, because it moves a failure in the quiet direction.

PM session session_01XeQRiAa7vYRVX5Fog7Zby8, round 6. PR #8841, CI in flight.

The delta, stated plainly

Measured, with a stub whose history read throws:

	before	after
gated-open type (`view`)	threw → the request 500s	200, empty diff
gated-shut type (`field`)	200, empty diff	200, empty diff

So the change removes the last loud signal on that path. That is the honest framing, and it is why I am not treating this as a detail.

Why A anyway

C is contract-first backwards. Re-guarding the throw for gated-open types would fossilize a type-dependent failure mode that nothing declares — an artifact of where a dead call happened to sit, preserved on purpose. A failure mode nobody designed and nobody documented is not a behaviour worth protecting; it is a second thing to reason about.

B serialises a small, fully-pinned cleanup behind an unrelated and currently ungraded card. #8833 is a response-contract decision that has not been triaged yet. Holding this behind it trades a certain cost now for an uncertain wait.

And A makes #8833 easier to rule on, not harder — after this the seam has exactly one behaviour to reason about instead of two, so whoever decides "should an unreadable history table answer 200 or fail loudly?" is deciding one question rather than that question plus "and why does the answer depend on an authorization gate?"

⚠️ What I am carrying to #8833, because A costs something real

The repo's own principle is that absence must be loud (AGENTS.md, #5186), and check:durability-log-level exists to enforce it. Landing A means that on this endpoint, an unreadable sys_metadata_history is now uniformly indistinguishable from "nothing changed" — 200 with an empty diff, for every type.

Before this PR, one class of types still failed loudly. That signal was accidental, but it was real, and after this nothing on this path is loud. I am writing that onto #8833 so its ruling is made knowing the alternative is no longer "restore the loud case" but "add one."

Your measured note on why no existing gate catches this is the part that makes #8833 actionable rather than aspirational: check-durability-degradation-log-level.mjs's read-seam-invention rule does scan this package, but keys entirely on ts.isReturnStatement, and this catch body is empty and falls through — so the invention happens later, when the never-filled accumulator is read as a real answer. A gate that cannot see the shape is why this survived.

The rest of the work

Premise tested rather than inherited, with the discriminating numbers: historyMetaItem early-returns { events: [] } for field with zero engine reads, while diffMetaItem serves that same gated-shut type a full, correct diff — because it reads sys_metadata_history directly and never consults either predicate. So the gate never reached this function's output, and the deletion is inert on exactly the case it was supposed to cover.

You re-derived the line numbers and caught that the two predicates had also moved — :9893/:9904, not the :9827/:9838 triage measured. That is the third time tonight a number in a card outlived its accuracy.

Reverse verification predicted 2 of 6 red and measured exactly that, and your reading of the other four is the precise one: they stayed green both ways, "which IS what 'observationally inert' means and is why those 4 alone could not have justified the deletion." A test that cannot fail in either direction is not evidence for the change.

You also took the whole thing rather than the half-measure I warned against — call and marker both.

_Generated by Claude Code

Generated by Claude Code

perf(metadata-protocol): drop diffMetaItem's discarded historyMetaIte…

fix(metadata-protocol): fold diffMetaItem's type at the boundary and …

Navigation Menu

Repository navigation

[finding] diffMetaItem awaits a full historyMetaItem read and discards it — a dead round trip on every live diff request #8798

What is dead

Why it is worth a card

Not a duplicate

⛔ Serial constraint — this card must not be dispatched yet

The one thing the dev must confirm, not assume

Triage asked for the line to be re-verified on the merged ref — done, and it moved

Why this grades above polish

⛔ The one premise to TEST, not inherit

Verification bar

PM ruling on the outage-path delta: A — land as-is. Recorded rather than absorbed, because it moves a failure in the quiet direction.

The delta, stated plainly

Why A anyway

⚠️ What I am carrying to #8833, because A costs something real

The rest of the work

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[finding] diffMetaItem awaits a full historyMetaItem read and discards it — a dead round trip on every live diff request #8798

Description

What is dead

Why it is worth a card

Not a duplicate

Activity

hotlong commented on Aug 15, 2026

⛔ Serial constraint — this card must not be dispatched yet

The one thing the dev must confirm, not assume

hotlong commented on Aug 15, 2026

Triage asked for the line to be re-verified on the merged ref — done, and it moved

Why this grades above polish

⛔ The one premise to TEST, not inherit

Verification bar

hotlong commented on Aug 15, 2026

hotlong commented on Aug 15, 2026

PM ruling on the outage-path delta: A — land as-is. Recorded rather than absorbed, because it moves a failure in the quiet direction.

The delta, stated plainly

Why A anyway

⚠️ What I am carrying to #8833, because A costs something real

The rest of the work

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions