From d3aedd24183e9fdf99fcbfc0b568d216b1c6b4eb Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Tue, 11 Aug 2026 08:46:41 +0000 Subject: [PATCH 1/2] fix(metadata-protocol,metadata): revert reads the history row under the 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 Claude-Session: https://claude.ai/code/session_0155v8vapVCt98zb9eWVhbLq --- .changeset/tame-moons-shave.md | 10 + packages/metadata-protocol/src/protocol.ts | 99 ++++- packages/metadata/src/metadata-manager.ts | 33 +- .../metadata/src/metadata-service.test.ts | 21 +- .../src/protocol-revert-org-scope.test.ts | 352 ++++++++++++++++++ packages/runtime/src/http-dispatcher.test.ts | 46 +++ 6 files changed, 549 insertions(+), 12 deletions(-) create mode 100644 .changeset/tame-moons-shave.md create mode 100644 packages/objectql/src/protocol-revert-org-scope.test.ts diff --git a/.changeset/tame-moons-shave.md b/.changeset/tame-moons-shave.md new file mode 100644 index 0000000000..dc8b28139a --- /dev/null +++ b/.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 = `: 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. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 14a1c03f28..338d20e428 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/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 { + 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[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[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 diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 9346e1fc01..e47ee9e557 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/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) { diff --git a/packages/metadata/src/metadata-service.test.ts b/packages/metadata/src/metadata-service.test.ts index 8a20d4760d..f11181e80d 100644 --- a/packages/metadata/src/metadata-service.test.ts +++ b/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'), + }); }); }); diff --git a/packages/objectql/src/protocol-revert-org-scope.test.ts b/packages/objectql/src/protocol-revert-org-scope.test.ts new file mode 100644 index 0000000000..d994020fd9 --- /dev/null +++ b/packages/objectql/src/protocol-revert-org-scope.test.ts @@ -0,0 +1,352 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7559 — the revert path's history lookup must ask under the key the history + * WRITER used, against a REAL {@link ObjectQL} engine and the REAL + * {@link SysMetadataRepository} behind the protocol. + * + * ## Why a real engine, and why the whole publish→revert round trip + * + * The existing ADR-0067 suites (`protocol-commit-history.test.ts`) stub + * `repo.restoreVersion` outright, so they pin the revert PLAN — created → + * soft-remove, edited → restoreVersion(prevVersion) — and are structurally + * unable to see whether the number in that plan resolves to a row. #7559 lived + * exactly in that gap: the plan was right and the lookup missed. + * + * ## The measurement this file pins (both sides of the same row) + * + * Drive a package publish twice with an active organization, over drafts + * authored env-wide (what Studio / AI authoring writes, and what + * `SysMetadataRepository.listDrafts`'s `$or` deliberately surfaces to a + * non-null-org caller): + * + * | | writer | reader (before the fix) | + * |---|---|---| + * | version base | `sys_metadata_history.version`, the per-(org,type,name) lineage counter — drafts consume numbers too, so publish #1 is v2 and publish #2 is v4, and the commit records `prevVersion: 2` | asks for version `2` — AGREES | + * | `package_id` | history carries no `package_id` column at all | not filtered — AGREES (so NOT the #6215 scoping) | + * | `organization_id` | `NULL` — publish routes each draft to the draft's OWN scope (#3115) | the REQUEST's active org — **DISAGREES** | + * + * so the revert answered `VERSION_NOT_FOUND: No history row at version 2` over + * a row the history endpoint lists. `organization_id` is the disagreeing key, + * alone; the other two agree, which is what makes this a distinct defect rather + * than a regression of #6215 (that one fails LATER, at `restoreVersion`'s + * `put()` parent lookup, with a 409). + * + * ## What each test is for + * + * The POSITIVE identity is pinned first and asserts the restored BODY, not just + * the absence of an error: a revert that "succeeds" while restoring the wrong + * version is the failure this card is one step away from. The refusal case then + * asserts `code` AND `status` (ADR-0112) — a bare `rejects.toThrow()` is green + * against an implementation that throws a naked `Error`, so it proves nothing + * about the envelope. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ObjectQL } from './engine.js'; + +const txt = (name: string, extra: Record = {}) => + ({ name, label: name, type: 'text' as const, ...extra }); +const num = (name: string) => ({ name, label: name, type: 'number' as const }); +const dt = (name: string) => ({ name, label: name, type: 'datetime' as const }); +const long = (name: string) => ({ name, label: name, type: 'longtext' as const }); + +const sysMetadataObject = { + name: 'sys_metadata', label: 'System Metadata', + fields: { + id: txt('id', { primaryKey: true }), + type: txt('type', { required: true }), name: txt('name', { required: true }), + organization_id: txt('organization_id'), package_id: txt('package_id'), + metadata: long('metadata'), checksum: txt('checksum', { maxLength: 71 }), + state: txt('state'), version: num('version'), + created_at: dt('created_at'), updated_at: dt('updated_at'), + created_by: txt('created_by'), updated_by: txt('updated_by'), + }, +}; + +const sysMetadataHistoryObject = { + name: 'sys_metadata_history', label: 'Metadata History', + fields: { + id: txt('id', { primaryKey: true }), event_seq: num('event_seq'), + type: txt('type', { required: true }), name: txt('name', { required: true }), + version: num('version'), operation_type: txt('operation_type'), + metadata: long('metadata'), checksum: txt('checksum', { maxLength: 71 }), + previous_checksum: txt('previous_checksum', { maxLength: 71 }), + change_note: long('change_note'), source: txt('source'), + organization_id: txt('organization_id'), recorded_by: txt('recorded_by'), + recorded_at: dt('recorded_at'), + }, +}; + +const sysMetadataCommitObject = { + name: 'sys_metadata_commit', label: 'Metadata Commit', + fields: { + id: txt('id', { primaryKey: true }), package_id: txt('package_id'), + operation: txt('operation'), message: long('message'), actor: txt('actor'), + ai_model: txt('ai_model'), parent_commit_id: txt('parent_commit_id'), + event_seq_start: num('event_seq_start'), event_seq_end: num('event_seq_end'), + items: long('items'), item_count: num('item_count'), + organization_id: txt('organization_id'), created_at: dt('created_at'), + }, +}; + +/** + * Minimal driver; equality-only WHERE. + * + * `$and` / `$or` are conjoined WITH their sibling keys, which is the one thing + * this stub may NOT get wrong here: `listDrafts` sends + * `{ state:'draft', package_id, $or:[{organization_id:ORG},{organization_id:null}] }`, + * and the short-circuiting shape some older stubs in this package use + * (`if ($or) return $or.some(...)`) drops the `state` and `package_id` filters + * and hands back rows no real driver would — which silently turns this whole + * scenario into a different one. + */ +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and') { + if (!(v as any[]).every((w) => matchesWhere(row, w))) return false; + continue; + } + if (k === '$or') { + if (!(v as any[]).some((w) => matchesWhere(row, w))) return false; + continue; + } + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + const a = row[k] === undefined ? null : row[k]; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(o: string, ast: any) { + return Array.from(storeFor(o).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(o: string, ast: any) { + for (const r of storeFor(o).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(o: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(o).set(id, row); + return row; + }, + async update(o: string, id: string, data: Record) { + const s = storeFor(o); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${o}/${id}`); + const u = { ...cur, ...data, id }; + s.set(id, u); + return u; + }, + async upsert(o: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(o).has(id)) return this.update(o, id, data); + return this.create(o, data); + }, + async delete(o: string, id: string) { return storeFor(o).delete(id); }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async bulkCreate(o: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(o, r))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver }; +} + +const PKG = 'app.revertscope'; +const ORG = 'org_x'; +const viewBody = (label: string) => ({ name: 'cases', type: 'grid', label, columns: ['id'] }); + +describe('#7559 — the revert reads the history row under the key the writer stored it with', () => { + let engine: ObjectQL; + let protocol: ObjectStackProtocolImplementation; + + beforeEach(async () => { + engine = new ObjectQL(); + const { driver } = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(sysMetadataObject); + engine.registry.registerObject(sysMetadataHistoryObject); + engine.registry.registerObject(sysMetadataCommitObject); + protocol = new ObjectStackProtocolImplementation(engine); + (protocol as any).ensureOverlayIndex = async () => {}; + }); + + /** The active row's stored body, read straight out of the table. */ + const activeBody = async (orgId: string | null) => { + const row = (await engine.findOne('sys_metadata', { + where: { organization_id: orgId, type: 'view', name: 'cases', state: 'active' }, + })) as any; + return row ? JSON.parse(row.metadata) : null; + }; + + /** + * The card's reproduction: two publishes of an env-wide draft, driven by a + * caller carrying an active org — which is every console request, since + * `resolveActiveOrganizationId` puts one on all of them. + */ + const publishTwiceEnvWideAsOrg = async () => { + await protocol.saveMetaItem({ + type: 'view', name: 'cases', item: viewBody('A'), packageId: PKG, mode: 'draft', + }); + await protocol.publishPackageDrafts({ + packageId: PKG, organizationId: ORG, message: 'publish 1', + }); + await protocol.saveMetaItem({ + type: 'view', name: 'cases', item: viewBody('B'), packageId: PKG, mode: 'draft', + }); + const p2 = await protocol.publishPackageDrafts({ + packageId: PKG, organizationId: ORG, message: 'publish 2', + }); + return p2; + }; + + // ── POSITIVE IDENTITY FIRST ────────────────────────────────────────── + // Pinned before any refusal so this suite cannot pass by refusing + // everything, and asserting the restored BODY so it cannot pass by + // reverting to the wrong version. + + it('revertCommit restores the pre-commit BODY for an env-wide item when the caller has an active org', async () => { + const p2 = await publishTwiceEnvWideAsOrg(); + expect(p2.success).toBe(true); + expect(p2.commitId).toBeTruthy(); + // Live state is publish #2's body before the revert. + expect(await activeBody(null)).toMatchObject({ label: 'B' }); + + const res = await protocol.revertCommit({ + commitId: p2.commitId!, organizationId: ORG, + }); + + expect(res.failed).toEqual([]); + expect(res.success).toBe(true); + expect(res.reverted).toEqual([{ type: 'view', name: 'cases', action: 'restored' }]); + // The identity that matters: publish #1's body is live again, in the + // env-wide scope it was published into. + expect(await activeBody(null)).toMatchObject({ label: 'A' }); + }); + + it('the writer and the reader address the SAME row — history is env-wide, and the commit asks for a version that is in it', async () => { + const p2 = await publishTwiceEnvWideAsOrg(); + + // WRITER: every history row landed env-wide, and the lineage counter + // numbers drafts too, so the pre-commit publish is version 2. + const hist = (await engine.find('sys_metadata_history', { where: {} })) as any[]; + expect(hist.map((r) => ({ v: r.version, org: r.organization_id ?? null }))).toEqual([ + { v: 1, org: null }, { v: 2, org: null }, { v: 3, org: null }, { v: 4, org: null }, + ]); + + // READER: the commit's revert plan asks for exactly that version. + const commits = await protocol.listCommits({ packageId: PKG, organizationId: ORG }); + const target = commits.find((c) => c.id === p2.commitId)!; + expect(target.items).toEqual([ + { type: 'view', name: 'cases', existedBefore: true, prevVersion: 2 }, + ]); + + // …and #6215's `package_id` scoping is NOT what is in play here: the + // history table the version lookup reads carries no such column. + expect(Object.keys(hist[0])).not.toContain('package_id'); + }); + + it('rollbackMetaItem — the sibling item-level revert — restores the same env-wide item for an org caller', async () => { + await publishTwiceEnvWideAsOrg(); + expect(await activeBody(null)).toMatchObject({ label: 'B' }); + + const res = await protocol.rollbackMetaItem({ + type: 'view', name: 'cases', toVersion: 2, organizationId: ORG, + }); + + expect(res.success).toBe(true); + expect(res.restoredFromVersion).toBe(2); + expect(await activeBody(null)).toMatchObject({ label: 'A' }); + }); + + it('an ORG-SCOPED item is still reverted in its OWN scope, not redirected env-wide', async () => { + // The control for the resolution's precedence: an org that has its own + // overlay row must keep reverting that row. Without it, "fall back to + // env-wide" could pass every test above while quietly hijacking the + // org-scoped case that already worked. + await protocol.saveMetaItem({ + type: 'view', name: 'cases', item: viewBody('ORG-A'), + packageId: PKG, mode: 'draft', organizationId: ORG, + }); + await protocol.publishPackageDrafts({ packageId: PKG, organizationId: ORG }); + await protocol.saveMetaItem({ + type: 'view', name: 'cases', item: viewBody('ORG-B'), + packageId: PKG, mode: 'draft', organizationId: ORG, + }); + const p2 = await protocol.publishPackageDrafts({ packageId: PKG, organizationId: ORG }); + + const res = await protocol.revertCommit({ + commitId: p2.commitId!, organizationId: ORG, + }); + + expect(res.failed).toEqual([]); + expect(res.success).toBe(true); + expect(await activeBody(ORG)).toMatchObject({ label: 'ORG-A' }); + // Nothing was written into the env-wide scope on this org's behalf. + expect(await activeBody(null)).toBeNull(); + }); + + // ── REFUSALS — `code` AND `status`, never a bare throw ──────────────── + + it('a version that genuinely has no history row is still refused, with the ADR-0112 envelope', async () => { + // The fix widens WHERE the lookup looks; it must not make a real miss + // resolve to something. Version 99 exists in no scope. + await publishTwiceEnvWideAsOrg(); + + await expect( + protocol.rollbackMetaItem({ + type: 'view', name: 'cases', toVersion: 99, organizationId: ORG, + }), + ).rejects.toMatchObject({ code: 'VERSION_NOT_FOUND', status: 404 }); + }); + + it('revertCommit reports a genuinely missing version per item, carrying the code', async () => { + await publishTwiceEnvWideAsOrg(); + // Hand-write a commit whose plan points at a version nobody ever wrote. + await engine.insert('sys_metadata_commit', { + id: 'cmt_bogus', package_id: PKG, operation: 'apply', + organization_id: ORG, item_count: 1, + items: JSON.stringify([ + { type: 'view', name: 'cases', existedBefore: true, prevVersion: 99 }, + ]), + created_at: '2026-08-11T00:00:00.000Z', + }); + + const res = await protocol.revertCommit({ commitId: 'cmt_bogus', organizationId: ORG }); + + expect(res.success).toBe(false); + expect(res.failedCount).toBe(1); + expect(res.failed[0]).toMatchObject({ + type: 'view', name: 'cases', code: 'VERSION_NOT_FOUND', + }); + }); + + it('an unknown commit id is refused with COMMIT_NOT_FOUND / 404', async () => { + await expect( + protocol.revertCommit({ commitId: 'cmt_nope', organizationId: ORG }), + ).rejects.toMatchObject({ code: 'COMMIT_NOT_FOUND', status: 404 }); + }); +}); diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 1218c47a45..57570a2606 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -1487,6 +1487,52 @@ describe('HttpDispatcher', () => { expect(mockMetadata.revertPackage).toHaveBeenCalledWith('com.acme.crm'); }); + // [#7559] ADR-0112 — a route that cannot serve the request answers a + // DECLARED 4xx, not a 500. The QA run saw a flat 500 from + // `POST /packages/:id/revert`. + // + // WHERE THAT 500 CAME FROM, measured rather than assumed: NOT from the + // route. `handlePackagesRequest` wraps its whole body in one + // `try { … } catch (e) { errorFromThrown(e, 500) }`, so the throw was + // always classified — `errorFromThrown` reads `status`/`code` off the + // error and falls back to 500 only when it finds neither, and + // `MetadataManager.revertPackage` threw bare `Error`s carrying neither. + // The whole defect is upstream, in the thrown shape; the first reading + // of this card (add a per-route `catch`) would have changed nothing, + // and reverse verification is what caught it — with the manager fixed + // and the route untouched, these cases already pass. + // + // So this pins the CHAIN, which is the part a manager-only unit test + // cannot see: a declared refusal survives the dispatcher as its own + // status AND code rather than being flattened. Both are asserted — + // status alone is green against a 404 with an empty envelope, code + // alone against a 500 that happens to carry one. + it.each([ + ['RESOURCE_NOT_FOUND', 404, "No metadata items found for package 'com.acme.crm'"], + ['RESOURCE_CONFLICT', 409, "Package 'com.acme.crm' has never been published"], + ])('POST /packages/:id/revert answers %s / %i, not 500', async (code, status, message) => { + const refusal = new Error(message) as Error & { code?: string; status?: number }; + refusal.code = code as string; + refusal.status = status as number; + const mockMetadata = { revertPackage: vi.fn().mockRejectedValue(refusal) }; + const mockRegistry = { + getAllPackages: vi.fn().mockReturnValue([]), + enablePackage: vi.fn(), + disablePackage: vi.fn(), + }; + (kernel as any).getService = vi.fn().mockImplementation((name: string) => { + if (name === 'metadata') return Promise.resolve(mockMetadata); + if (name === 'objectql') return Promise.resolve({ registry: mockRegistry }); + return null; + }); + + const result = await dispatcher.handlePackages('/com.acme.crm/revert', 'POST', {}, {}, PKG_ADMIN()); + + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(status); + expect(result.response?.body?.error?.code).toBe(code); + }); + it('should return 503 for publish when metadata service unavailable', async () => { const mockRegistry = { getAllPackages: vi.fn().mockReturnValue([]), From aa17597c608a309564eb38f21f6834cde5f52781 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Tue, 11 Aug 2026 09:55:49 +0000 Subject: [PATCH 2/2] fix(objectql): new #7559 test adds zero raw tsc errors to the TEST_DEBT 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` 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 Claude-Session: https://claude.ai/code/session_0155v8vapVCt98zb9eWVhbLq --- .../src/protocol-revert-org-scope.test.ts | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/packages/objectql/src/protocol-revert-org-scope.test.ts b/packages/objectql/src/protocol-revert-org-scope.test.ts index d994020fd9..530c85fee2 100644 --- a/packages/objectql/src/protocol-revert-org-scope.test.ts +++ b/packages/objectql/src/protocol-revert-org-scope.test.ts @@ -44,15 +44,25 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import type { ServiceObject } from '@objectstack/spec/data'; import { ObjectQL } from './engine.js'; -const txt = (name: string, extra: Record = {}) => +// `extra` is narrowed to the two keys these fixtures actually set. Typed as +// `Record` the spread adds an index signature to every field, +// which makes the whole object unassignable to `ServiceObject` (TS2345) — +// invisible to the package's own `typecheck` script, which excludes tests, and +// a raw error to the shrink-only type-check-coverage ratchet, which does not. +const txt = (name: string, extra: { primaryKey?: boolean; maxLength?: number; required?: boolean } = {}) => ({ name, label: name, type: 'text' as const, ...extra }); const num = (name: string) => ({ name, label: name, type: 'number' as const }); const dt = (name: string) => ({ name, label: name, type: 'datetime' as const }); -const long = (name: string) => ({ name, label: name, type: 'longtext' as const }); +// `textarea`, not `longtext`: the latter is not in the FieldType union at all +// (`packages/spec/src/data/field.zod.ts`). Several older fixtures in this +// package spell it `longtext` and it type-errors there too — part of what the +// TEST_DEBT ledger is counting. +const long = (name: string) => ({ name, label: name, type: 'textarea' as const }); -const sysMetadataObject = { +const sysMetadataObject: ServiceObject = { name: 'sys_metadata', label: 'System Metadata', fields: { id: txt('id', { primaryKey: true }), @@ -65,7 +75,7 @@ const sysMetadataObject = { }, }; -const sysMetadataHistoryObject = { +const sysMetadataHistoryObject: ServiceObject = { name: 'sys_metadata_history', label: 'Metadata History', fields: { id: txt('id', { primaryKey: true }), event_seq: num('event_seq'), @@ -79,7 +89,7 @@ const sysMetadataHistoryObject = { }, }; -const sysMetadataCommitObject = { +const sysMetadataCommitObject: ServiceObject = { name: 'sys_metadata_commit', label: 'Metadata Commit', fields: { id: txt('id', { primaryKey: true }), package_id: txt('package_id'), @@ -187,9 +197,14 @@ describe('#7559 — the revert reads the history row under the key the writer st const { driver } = makeStubDriver(); engine.registerDriver(driver, true); await engine.init(); - engine.registry.registerObject(sysMetadataObject); - engine.registry.registerObject(sysMetadataHistoryObject); - engine.registry.registerObject(sysMetadataCommitObject); + // `packageId` is REQUIRED by `registerObject(schema, packageId, …)`. + // Omitting it compiles under the package's own `typecheck` script (which + // excludes `*.test.ts`) but is a raw TS2554 to the type-check-coverage + // ratchet, which measures `tsc --noEmit` with the tests put back — + // TEST_DEBT is shrink-only, so a new test file has to add zero. + engine.registry.registerObject(sysMetadataObject, 'test'); + engine.registry.registerObject(sysMetadataHistoryObject, 'test'); + engine.registry.registerObject(sysMetadataCommitObject, 'test'); protocol = new ObjectStackProtocolImplementation(engine); (protocol as any).ensureOverlayIndex = async () => {}; });