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

.changeset/tame-moons-shave.md

@@ -0,0 +1,10 @@
+---
+'@objectstack/metadata-protocol': patch
+'@objectstack/metadata': patch
+---
+
+Fix commit-revert answering `VERSION_NOT_FOUND` over a row `/history` lists, and the package-level revert route answering 500
+
+**Revert (`revertCommit` / `rollbackMetaItem`).** Both revert callers resolved their overlay repository from the caller's *active organization*, while the publish that recorded the commit routes each draft to the draft's **own** scope (the ADR-0005 / #3115 rule `SysMetadataRepository.listDrafts` states, and `publishPackageDrafts` already follows). So an env-wide artifact — what Studio and AI authoring write — published from a console request carrying an active org stored its `sys_metadata_history` rows at `organization_id = NULL` and was then read back at `organization_id = <org>`: no match, and the revert answered `VERSION_NOT_FOUND: No history row at version 2` for a version the history endpoint lists. The revert now resolves the scope the item's lineage actually lives in (the caller's own overlay first, env-wide second), per item for a batch revert. The same resolution reaches the `#6602` registry heal and the `#4636` package-binding read, which an org-scoped revert of an env-wide row was previously skipping while reporting success.
+
+**`POST /packages/:id/revert`.** The route now answers a declared 4xx instead of 500 (ADR-0112). The cause was entirely in the thrown shape, not the route: `MetadataManager.revertPackage` threw bare `Error`s carrying no `code` or `status`, and `errorFromThrown` — which the route's handler already reaches through one enclosing `catch` — falls back to 500 only when it finds neither. An unknown package id now answers `RESOURCE_NOT_FOUND` / 404 and a never-published package `RESOURCE_CONFLICT` / 409; 500 remains only as the fallback for a genuinely unexpected throw.

packages/metadata-protocol/src/protocol.ts

@@ -3251,6 +3251,61 @@ export class ObjectStackProtocolImplementation implements
         return repo;
     }
 
+    /**
+     * [#7559] ADR-0005 / #3115 — resolve the org scope an item's lineage
+     * ACTUALLY lives in, for a caller whose active org may not be that scope.
+     *
+     * This is the read-side half of the rule {@link SysMetadataRepository.listDrafts}
+     * states on the write side: a non-null-org caller sees BOTH its own overlay
+     * rows and the env-wide (`organization_id IS NULL`) ones, "so consumers that
+     * then act on a draft MUST route the write to THIS scope, not the caller's
+     * active org, or they 404 on the env-wide row they can never match".
+     * {@link publishPackageDrafts} learned it — it promotes each draft through
+     * `getOverlayRepo(d.organizationId)` and captures `prevVersion` from the
+     * row in the draft's OWN scope. The two revert callers did not, and read
+     * back under `getOverlayRepo(request.organizationId)` instead.
+     *
+     * Measured on `origin/main` (#7559): an env-wide `view` published twice from
+     * a console request carrying an active org lands its `sys_metadata` and
+     * `sys_metadata_history` rows at `organization_id = NULL` while the commit
+     * records `prevVersion: 2`; `revertCommit` with that same active org then
+     * asks `sys_metadata_history` for `(organization_id='org_x', version=2)`,
+     * matches nothing, and answers `VERSION_NOT_FOUND: No history row at
+     * version 2` — over a row `GET …/history` lists. Same input with no active
+     * org succeeds, and an org-scoped item reverted by its own org succeeds:
+     * the disagreement is `organization_id` alone.
+     *
+     * NOT the `package_id` scoping #6215 fixed — that one is a step later, in
+     * {@link SysMetadataRepository.restoreVersion}'s `put()` parent lookup, and
+     * is intact and uninvolved here (the history table carries no `package_id`
+     * column at all).
+     *
+     * Precedence is the ADR-0005 overlay order — the caller's own org shadows
+     * env-wide — so an org that has its own overlay row reverts THAT row, and
+     * only an org with no overlay of its own falls through to the env-wide
+     * lineage it was already publishing into. When neither scope has a lineage
+     * the caller's own scope is returned unchanged, so a genuinely absent item
+     * still fails in the scope the caller asked about.
+     *
+     * Deliberately NO `catch`: a driver failure here must fail the revert, not
+     * resolve to a scope nobody verified (AGENTS.md read-seam invention rule).
+     */
+    private async resolveMetaItemOrgScope(
+        singularType: string,
+        name: string,
+        requestOrgId: string | null,
+    ): Promise<string | null> {
+        if (requestOrgId === null) return null;
+        const inOrg = await this.engine.findOne('sys_metadata_history', {
+            where: { organization_id: requestOrgId, type: singularType, name },
+        });
+        if (inOrg) return requestOrgId;
+        const inEnv = await this.engine.findOne('sys_metadata_history', {
+            where: { organization_id: null, type: singularType, name },
+        });
+        return inEnv ? null : requestOrgId;
+    }
+
     /**
      * One-time guard for ensuring the overlay-uniqueness UNIQUE INDEXes exist
      * on `sys_metadata`. ADR-0005 (revised 2026-05) + ADR-0048: per-env DBs
@@ -12025,7 +12080,6 @@ export class ObjectStackProtocolImplementation implements
             throw err;
         }
         const items = this.parseCommitItems(row.items);
-        const repo = this.getOverlayRepo(orgId);
         // #4556 — threaded into repo.put/delete → `recorded_by`; NULL when the
         // revert carries no human actor.
         const actor = request.actor ?? null;
@@ -12035,7 +12089,22 @@ export class ObjectStackProtocolImplementation implements
         // Reverse apply order so artifacts that depend on others (e.g. a view on
         // a new object) are removed before the thing they reference.
         for (const it of [...items].reverse()) {
-            const ref = { type: it.type, name: it.name, org: orgId ?? 'env' } as unknown as Parameters<typeof repo.get>[0];
+            // [#7559] PER ITEM, and from the ROW rather than from the request —
+            // the same shape {@link publishPackageDrafts} already uses when it
+            // promotes each draft in the draft's OWN scope and captures
+            // `prevVersion` there. A batch legitimately mixes an env-wide
+            // artifact with an org overlay, so a hoisted `orgId` has to pick one
+            // and be wrong about the other — which is exactly how a commit whose
+            // items are env-wide answered `VERSION_NOT_FOUND` for every item
+            // when reverted by a caller with an active org. See
+            // {@link resolveMetaItemOrgScope} for the measurement.
+            const itemOrgId = await this.resolveMetaItemOrgScope(
+                PLURAL_TO_SINGULAR[it.type] ?? it.type,
+                it.name,
+                orgId,
+            );
+            const repo = this.getOverlayRepo(itemOrgId);
+            const ref = { type: it.type, name: it.name, org: itemOrgId ?? 'env' } as unknown as Parameters<typeof repo.get>[0];
             try {
                 const current = await repo.get(ref, { state: 'active' });
                 if (!it.existedBefore) {
@@ -12133,7 +12202,11 @@ export class ObjectStackProtocolImplementation implements
                     // all three of its own call sites. The gate moved to the
                     // choke point every caller shares; the pin below this
                     // comment is unchanged and still covers the batch path.
-                    await this.restoreArtifactRegistryView(it.type, it.name, orgId);
+                    // [#7559] The ITEM's resolved scope, not the request's — the
+                    // #6602 gate this parameter carries asks "is this row
+                    // env-wide?", and an env-wide row reverted by an org-scoped
+                    // caller skipped the heal entirely while answering success.
+                    await this.restoreArtifactRegistryView(it.type, it.name, itemOrgId);
                     reverted.push({ type: it.type, name: it.name, action: 'removed' });
                 } else if (it.prevVersion !== null && it.prevVersion !== undefined) {
                     // Edited an existing artifact → restore the pre-commit body.
@@ -12179,7 +12252,7 @@ export class ObjectStackProtocolImplementation implements
                     // fallible query downstream of a write that already succeeded —
                     // the shape that ends in a `catch {}` swallowing a real outage
                     // (#4867). Per ITEM, because a batch mixes bindings.
-                    const restorePackageId = await this.resolveOverlayPackageBinding(it.type, it.name, orgId);
+                    const restorePackageId = await this.resolveOverlayPackageBinding(it.type, it.name, itemOrgId);
                     const restored = await repo.restoreVersion(ref, it.prevVersion, {
                         actor,
                         source: 'protocol.revertCommit',
@@ -12215,7 +12288,10 @@ export class ObjectStackProtocolImplementation implements
                         // is refused by {@link hydrateOverlayIntoRegistry} and never
                         // reaches the registry every org in this process shares —
                         // inherited, not re-decided here.
-                        organizationId: orgId,
+                        // [#7559] …and now that is what it actually IS. This line
+                        // said "the row's OWN scope" while passing the REQUEST's
+                        // org; the resolution above is what makes the comment true.
+                        organizationId: itemOrgId,
                     });
                     reverted.push({ type: it.type, name: it.name, action: 'restored' });
                 }
@@ -12356,7 +12432,18 @@ export class ObjectStackProtocolImplementation implements
         });
         if (_rollbackLockErr) throw _rollbackLockErr;
         await this.ensureOverlayIndex();
-        const orgId = request.organizationId ?? null;
+        // [#7559] The scope the item's lineage actually lives in, not the
+        // caller's active org. Measured on `origin/main`: an env-wide `view`
+        // rolled back by a caller with an active org threw `VERSION_NOT_FOUND`
+        // (404) at exactly the version its own history endpoint lists, while
+        // the identical call with no active org succeeded — the same
+        // disagreement {@link revertCommit} showed, one caller over. See
+        // {@link resolveMetaItemOrgScope}.
+        const orgId = await this.resolveMetaItemOrgScope(
+            singularType,
+            request.name,
+            request.organizationId ?? null,
+        );
         const repo = this.getOverlayRepo(orgId);
         const artifactBacked = this.isArtifactBacked(singularType, request.name);
         const intent: 'override-artifact' | 'runtime-only' = artifactBacked

packages/metadata/src/metadata-manager.ts

@@ -1758,14 +1758,43 @@ export class MetadataManager implements IMetadataService {
       }
     }
 
+    // [#7559] ADR-0112 — both refusals below carry a DECLARED `code` + `status`.
+    // They are the ordinary answers to an ordinary request (revert a package id
+    // that this environment has nothing for, or has never published), and a
+    // route that cannot serve the request must answer a declared 4xx.
+    //
+    // This is the WHOLE cause of the 500 the QA run saw from
+    // `POST /packages/:id/revert`, measured rather than assumed: that route's
+    // handler already wraps its entire body in one
+    // `try { … } catch (e) { errorFromThrown(e, 500) }`, and `errorFromThrown`
+    // reads `status` / `code` off the error — falling back to 500 only when it
+    // finds neither, which is exactly what a bare `Error` offers. Nothing was
+    // wrong with the route; the thrown shape was. (The first reading of #7559
+    // was that the route needed its own `catch`; reverse verification showed
+    // that change was inert, so it is not in this fix.)
+    //
+    // Both codes come from the ADR-0112 STANDARD catalog rather than the
+    // extension ledger: the ledger's own rule is that a generic condition (not
+    // found / conflict) uses the standard catalog instead of registering a
+    // synonym.
     if (packageItems.length === 0) {
-      throw new Error(`No metadata items found for package '${packageId}'`);
+      const err = new Error(
+        `No metadata items found for package '${packageId}'`,
+      ) as Error & { code?: string; status?: number };
+      err.code = 'RESOURCE_NOT_FOUND';
+      err.status = 404;
+      throw err;
     }
 
     // Check that at least one item has a published snapshot
     const hasPublished = packageItems.some(item => item.data.publishedDefinition !== undefined);
     if (!hasPublished) {
-      throw new Error(`Package '${packageId}' has never been published`);
+      const err = new Error(
+        `Package '${packageId}' has never been published`,
+      ) as Error & { code?: string; status?: number };
+      err.code = 'RESOURCE_CONFLICT';
+      err.status = 409;
+      throw err;
     }
 
     for (const item of packageItems) {

packages/metadata/src/metadata-service.test.ts

@@ -909,16 +909,29 @@ describe('MetadataManager — IMetadataService Contract', () => {
       expect(reverted.metadata).toEqual(reverted.publishedDefinition);
     });
 
-    it('should throw for non-existent package', async () => {
-      await expect(manager.revertPackage('nonexistent')).rejects.toThrow('No metadata items found');
+    // [#7559] Both refusals assert `code` AND `status`, not just the message.
+    // `rejects.toThrow('…')` was green against the naked `Error` these sites
+    // used to throw — the throw was never the defect; the missing ADR-0112
+    // envelope was, and it is what made `POST /packages/:id/revert` answer 500
+    // for two perfectly ordinary refusals.
+    it('should refuse a non-existent package with RESOURCE_NOT_FOUND / 404', async () => {
+      await expect(manager.revertPackage('nonexistent')).rejects.toMatchObject({
+        code: 'RESOURCE_NOT_FOUND',
+        status: 404,
+        message: expect.stringContaining('No metadata items found'),
+      });
     });
 
-    it('should throw for never-published package', async () => {
+    it('should refuse a never-published package with RESOURCE_CONFLICT / 409', async () => {
       await manager.register('object', 'new_item', {
         name: 'new_item', packageId: 'com.acme.new',
       });
 
-      await expect(manager.revertPackage('com.acme.new')).rejects.toThrow('has never been published');
+      await expect(manager.revertPackage('com.acme.new')).rejects.toMatchObject({
+        code: 'RESOURCE_CONFLICT',
+        status: 409,
+        message: expect.stringContaining('has never been published'),
+      });
     });
   });