Skip to content

fix(metadata-protocol,metadata): revert reads the history row under the key the writer stored it with (#7559) - #7619

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-7559-revert-version-key-mismatch
Aug 11, 2026
Merged

fix(metadata-protocol,metadata): revert reads the history row under the key the writer stored it with (#7559)#7619
os-zhuang merged 2 commits into
mainfrom
claude/issue-7559-revert-version-key-mismatch

Conversation

@claude

@claudeclaudeBot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Fixes#7559

The card deliberately stopped at "the revert's lookup key disagrees with what the history writer stores" and asked for a measurement before a patch. This is that measurement, then the fix it selects.

The measurement — both sides of the same row

Driven against a realObjectQL engine and the realSysMetadataRepository behind the protocol: author an env-wide draft, publish the package, edit, publish again — with the publish and revert requests carrying an active organization, which is what every console request carries (resolveActiveOrganizationId puts one on all of them). Env-wide is what Studio / AI authoring writes, and SysMetadataRepository.listDrafts deliberately surfaces those drafts to a non-null-org caller through its $or.

Rows after the second publish:

sys_metadata type=view name=cases org=NULL state=active version=4 pkg=app.probe
sys_metadata_history v1 create org=NULL (draft save)
v2 publish org=NULL (publish #1)
v3 create org=NULL (draft save)
v4 publish org=NULL (publish #2)
sys_metadata_commit cmt_...a org=org_x items=[{view/cases, existedBefore:false, prevVersion:null}]
cmt_...b org=org_x items=[{view/cases, existedBefore:true, prevVersion:2}]
addressing keywhat the writer persistswhat the revert's lookup asks forverdict
version numbering basesys_metadata_history.version, the per-(org,type,name) lineage counter. Drafts consume numbers too, so publish #1 is v2 and the commit records prevVersion: 2restoreVersion(targetVersion) with exactly 2agrees
package_id / overlaysys_metadata_history has no package_id column at allthe version lookup does not filter on itagrees
parent idprevious_checksum on the history rownot consulted by the version lookupagrees
organization_idNULL — publish routes each draft to the draft's own scope, and captures prevVersion from the row in that scope (the #3115 rule)'org_x'getOverlayRepo(request.organizationId)DISAGREES

So the reader asks sys_metadata_history for (organization_id='org_x', type='view', name='cases', version=2), matches nothing, and answers:

revertCommit(org) => success:false, failed:[{ code: "VERSION_NOT_FOUND",
error: "[version_not_found] No history row at version 2 for view/cases." }]

— the card's symptom verbatim, version number included. Two controls isolate the key to organization_id alone:

  • same input, no active orgsuccess: true, restored.
  • an org-scoped item reverted by its own orgsuccess: true, restored.

historyMetaItem shows the same split from the read side: with no org it lists all four versions; with organizationId: 'org_x' it returns []. That is consistent with the card's "/history lists exactly that row" — the listing the QA read was not scoped to the org the revert was.

The sibling item-level revert has the identical defect, measured the same way and not previously reported: rollbackMetaItem({ type:'view', name:'cases', toVersion:2, organizationId:'org_x' }) threw VERSION_NOT_FOUND / 404 while the same call without the org succeeded.

Not a regression of #6215 — confirmed, as asked

Confirmed against the code, not from symptoms. #6215 scoped restoreVersion's put()parent lookup by package_id; its fix is present and intact (restoreVersion reads the raw active row and threads activePackageId into put). It is also structurally uninvolved here: this failure happens one step earlier, in the history version lookup, and that table carries no package_id column for the scoping to apply to. Different key (organization_id), different stage (before the row is located rather than after), different class (404 VERSION_NOT_FOUND rather than 409). Same family — the revert path's row addressing disagreeing with the writer's — as the card predicted.

The fix

One shared resolver, resolveMetaItemOrgScope, applied at both revert callers: resolve the scope the item's lineage actually lives in rather than assuming the caller's active org. Precedence is the ADR-0005 overlay order — the caller's own overlay first, env-wide second, and the caller's own scope unchanged when neither has a lineage, so a genuinely absent item still fails in the scope the caller asked about. No catch: a driver failure fails the revert rather than resolving to a scope nobody verified.

This is the read-side half of the rule the write side already states and publishPackageDrafts already follows. revertCommit resolves per item, because a batch legitimately mixes an env-wide artifact with an org overlay and a hoisted scope has to pick one and be wrong about the other.

Two latent bugs fall out with it: the #6602 registry heal and the #4636 package-binding read both received the request's org, so an org-scoped revert of an env-wide row skipped the heal while reporting success. The #6602 call site's own comment already said "the row's OWN scope, per item" while passing the request's org; the resolution is what makes that comment true.

Second half — the package-level revert 500

The route is unchanged, and that is the finding. The first reading of this card is that POST /packages/:id/revert needs its own catch. It does not: handlePackagesRequest wraps its entire body in one try { … } catch (e) { errorFromThrown(e, 500) }, so the throw was always classified. errorFromThrown reads status and code off the error and falls back to 500 only when it finds neither — and MetadataManager.revertPackage threw bare Errors carrying neither, for two perfectly ordinary refusals. The whole defect is the thrown shape.

I wrote the per-route catch first and reverse verification showed it inert — the envelope cases pass with the manager fixed and the route untouched — so it is not in this PR. The route file is byte-identical to main.

Both refusals now carry a declared envelope (ADR-0112): unknown package id → RESOURCE_NOT_FOUND / 404; never-published package → RESOURCE_CONFLICT / 409. Both come from the ADR-0112 standard catalog rather than the extension ledger, per the ledger's own rule that a generic condition (not found / conflict) uses the standard catalog instead of registering a synonym — so no ledger edit and no spec regeneration.

Not filed as a separate issue: it is on the same feature and inside this card's reach, which the dispatch asked for in preference to filing.

Tests

Positive identity is pinned first and asserts the restored body, not merely the absence of an error — a revert that "succeeds" while restoring the wrong version is one step away from this defect. Refusal cases assert codeandstatus; a bare rejects.toThrow() is green against an implementation that throws a naked Error.

New packages/objectql/src/protocol-revert-org-scope.test.ts (7 cases) — real engine, real repository, full publish→revert round trip. The existing ADR-0067 suite stubs repo.restoreVersion outright, so it pins the revert plan and is structurally unable to see whether the number in that plan resolves to a row; #7559 lived exactly in that gap.

  • revertCommit restores the pre-commit body for an env-wide item with an active org
  • the writer/reader addressing measurement, pinned as assertions
  • rollbackMetaItem restores the same item for an org caller
  • control: an org-scoped item is still reverted in its own scope, and nothing is written env-wide on its behalf
  • a version with genuinely no history row is still refused — VERSION_NOT_FOUND / 404
  • revertCommit reports a genuinely missing version per item, carrying the code
  • an unknown commit id — COMMIT_NOT_FOUND / 404

Also strengthened: the two revertPackage cases in metadata-service.test.ts (were rejects.toThrow(message), now assert code + status + message), and a new dispatcher case pinning that a declared refusal survives to the wire as its own status and code rather than being flattened to 500.

Reverse verification — direction predicted before running

Half 1, protocol.ts reverted to main: predicted 2 red / 5 green; got exactly that. The two positive-identity cases failed with VERSION_NOT_FOUND. The writer-measurement case, the org-scoped control and the three refusals stayed green by design — they do not depend on the fix, and saying so is the point: they are what stops the suite passing by refusing everything.

Half 2, manager reverted to main: 2 red (the envelope cases). The route case stayed green — the unpredicted direction that found the inert route change, reported above rather than papered over.

Local runs

@objectstack/metadata 30 files, 593 tests passed
@objectstack/metadata-protocol 72 files, 1062 tests passed
@objectstack/objectql 180 files, 3194 tests passed
@objectstack/runtime 124 files, 1992 tests passed
typecheck (objectql, runtime) Done
check:nul-bytes / check:error-code-casing /
check:engine-double-contract / check:durability-log-level all OK

@objectstack/metadata and @objectstack/metadata-protocol declare no typecheck script (ledger DEBT), so the typecheck above is the full coverage available for the four packages, not a subset I chose. Suites were run after pnpm --filter 'PKG^...' build in a fresh worktree; the gate farm beyond the four families named above is CI's.


Generated by Claude Code

…he key the writer stored it with (#7559)
Commit-revert answered `VERSION_NOT_FOUND: No history row at version 2`
over a row `GET .../history` lists, and `POST /packages/:id/revert`
answered 500.
Measured both sides of the same row, driving a real publish twice
through the real protocol and SysMetadataRepository:
writer sys_metadata_history.version = per-(org,type,name) lineage
counter; drafts consume numbers, so publish #1 is v2 and the
commit records prevVersion: 2 -- and every row lands at
organization_id = NULL, because publish routes each draft to
the draft's OWN scope (#3115).
reader restoreVersion asks for version 2 -- agrees; does not filter
package_id, and the history table has no such column --
agrees; scopes organization_id to the REQUEST's active org --
DISAGREES.
organization_id alone is the disagreeing key. Not a regression of
#6215: that one fails later, at restoreVersion's put() parent lookup,
with a 409, and its package_id scoping is intact and uninvolved here.
revertCommit and rollbackMetaItem now resolve the scope an item's
lineage actually lives in (caller's own overlay first, env-wide
second), per item for a batch. The resolved scope also reaches the
#6602 registry heal and the #4636 package-binding read, which an
org-scoped revert of an env-wide row was skipping while reporting
success.
Second half, a separate defect on the same feature: revertPackage threw
bare Errors with no code/status, so errorFromThrown had nothing to
classify and fell back to 500. Now RESOURCE_NOT_FOUND/404 and
RESOURCE_CONFLICT/409 per ADR-0112. The route itself is UNCHANGED: its
handler already wraps the whole body in one catch that calls
errorFromThrown, so the per-route catch this card's first reading
called for was inert -- reverse verification caught that, and it is not
in this fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0155v8vapVCt98zb9eWVhbLq
@vercel

vercelBot commented Aug 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 11, 2026 9:56am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-protocol, @objectstack/metadata.

6 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, @objectstack/metadata)
  • content/docs/kernel/cluster.mdx(via packages/metadata)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol, @objectstack/metadata)
  • content/docs/plugins/packages.mdx(via @objectstack/metadata)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol, @objectstack/metadata)
  • content/docs/protocol/kernel/metadata-service.mdx(via @objectstack/metadata)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/v12.mdx(via @objectstack/metadata)
  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol, @objectstack/metadata)

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.

…BT ledger
`check-type-check-coverage --re-measure` went red: objectql TEST_DEBT
records 355, tsc reported 356 (+1). The three errors the new test file
contributed, all invisible to the package's own `typecheck` script
because that config excludes `*.test.ts` while the ratchet measures with
the tests put back:
TS2554 x3 registerObject(schema) -- `packageId` is a REQUIRED second
parameter, not optional
TS2345 x3 the `Record<string, unknown>` spread in the field helper
put an index signature on every field, so the object was
not assignable to ServiceObject
TS2322 x5 `longtext` is not in the FieldType union; the spelling is
`textarea`
(The last two surfaced only once the one before it was fixed, which is
why the count moved 356 -> 356 -> 358 -> 353 rather than straight down.)
Measured the way the gate measures -- a sibling tsconfig that extends
the package's own with the test globs dropped from `exclude`, over a
FULLY BUILT closure. Without the build the same command reports 654, the
TS2307-plus-implicit-any cascade the script itself refuses to record.
objectql now measures 353 against a recorded 355. Ledger deliberately
UNCHANGED: the -2 is the gate's informational "can be lowered" line, and
lowering it -- or any other entry's -- is not this PR's business.
Behaviour unaffected: objectql 180 files / 3194 tests green, and reverse
verification still arms -- reverting the protocol fix under the new
fixtures still turns exactly the two positive-identity cases red with
VERSION_NOT_FOUND.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0155v8vapVCt98zb9eWVhbLq
@os-zhuang
os-zhuang marked this pull request as ready for review August 11, 2026 10:18
@os-zhuang
os-zhuang added this pull request to the merge queueAug 11, 2026
Merged via the queue into main with commit 52d1a7dAug 11, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-7559-revert-version-key-mismatch branch August 11, 2026 10:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

1 participant

@os-zhuang