From 8b9549e58842ed38e8da19689a2c7b37aea2f5e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:14:05 +0000 Subject: [PATCH] fix(metadata-protocol): uninstall no longer orphans env-wide sys_metadata rows (#7705) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `protocol.deletePackage` selected the rows to remove with a strict `organization_id` equality, which matches nothing against rows stored env-wide (`organization_id IS NULL`). An uninstall issued by a session with an active organization therefore removed only whichever rows happened to be org-scoped and left every env-wide row behind, while reporting a nonzero `deletedCount` and `success: true` over the survivors. Measured, not assumed. The card offered two candidates and the second is falsified: `findData` — the `GET /api/v1/data/sys_metadata` path that returned three rows — issues `this.engine.find` on the same engine instance, and the engine injects no org predicate of its own, so the protocol's engine is NOT scoped differently from the data plane's. On a real ObjectQL engine over SQLite, an org-scoped uninstall of a package holding three env-wide rows and one org-scoped row deleted 1 of 4. An org-scoped uninstall now matches its own organization OR env-wide — the `$or` shape this package already uses for the #3115 orphaned-draft fix, and the shape the SQL driver's own tenant wall uses (#2734). Both directions that must not widen are unchanged and pinned: another organization's rows for the same package stay out of scope, and another package's rows are never touched. The no-org branch stays package-wide on purpose — the direct-mount REST door passes no organization, so narrowing it to env-wide-only would orphan every org-scoped row instead. The pin uses a real engine and a real driver and asserts the CONSEQUENCE: `{success: true, deletedCount: 0}` against a package with no rows is indistinguishable from this bug, so it seeds rows through the real save path, runs the real uninstall, and asserts which rows survive in SQLite afterwards. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GKUiYq4A42J7Aa1QfkKyiX --- .../uninstall-env-wide-metadata-orphans.md | 42 +++ packages/metadata-protocol/src/protocol.ts | 33 ++- ...ge-uninstall-org-scope.integration.test.ts | 260 ++++++++++++++++++ 3 files changed, 334 insertions(+), 1 deletion(-) create mode 100644 .changeset/uninstall-env-wide-metadata-orphans.md create mode 100644 packages/runtime/src/package-uninstall-org-scope.integration.test.ts diff --git a/.changeset/uninstall-env-wide-metadata-orphans.md b/.changeset/uninstall-env-wide-metadata-orphans.md new file mode 100644 index 0000000000..d07eb77237 --- /dev/null +++ b/.changeset/uninstall-env-wide-metadata-orphans.md @@ -0,0 +1,42 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): uninstall no longer orphans a package's env-wide `sys_metadata` rows (#7705) + +`protocol.deletePackage` selected the rows to remove with a strict +`organization_id` equality: + +```ts +const where = { package_id: request.packageId }; +if (request.organizationId) where.organization_id = request.organizationId; +``` + +Against rows stored **env-wide** (`organization_id IS NULL`) that predicate +matches nothing, so an uninstall issued by a session with an active +organization removed only whichever rows happened to be org-scoped and left +every env-wide row behind — while reporting a nonzero `deletedCount` and +`success: true` over the survivors. The package's metadata stayed in +`sys_metadata` after its uninstall "succeeded", and a reinstall then collided +with the rows that were never removed. + +Env-wide is where a package's metadata normally lands, which is why this was +the common case rather than a corner: the REST `PUT /meta/:type/:name` save +path does not thread the session's active organization, and AI-authored +metadata is written env-wide too. Measured on a real engine over SQLite, an +org-scoped uninstall of a package holding three env-wide rows and one +org-scoped row deleted **1 of 4** and reported success. + +An org-scoped uninstall now matches its own organization **or** env-wide, the +same `$or [{organization_id: oid}, {organization_id: null}]` shape this package +already uses for the #3115 "orphaned draft" fix, and the same shape the SQL +driver's own implicit tenant wall uses (`field = :tenant OR field IS NULL`, +#2734). + +Scoping is unchanged in both directions that must not widen: another +organization's rows for the same package are still out of scope for an +org-scoped uninstall, and another package's rows are never touched. An +uninstall issued with **no** organization is also unchanged — it stays +package-wide, because the direct-mount REST door passes no organization at all +and narrowing that branch to env-wide-only would orphan every org-scoped row +instead. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index b544036247..c4bc93572f 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -11624,7 +11624,38 @@ export class ObjectStackProtocolImplementation implements cleanups: UninstallCleanupOutcome[]; }> { const where: Record = { package_id: request.packageId }; - if (request.organizationId) where.organization_id = request.organizationId; + // [#7705] Surface BOTH org-scoped rows and env-wide (`organization_id + // IS NULL`) rows to an org-scoped uninstall. A strict + // `organization_id = ` equality silently dropped every env-wide + // row, and env-wide is where a package's metadata normally LANDS: the + // REST `PUT /meta/:type/:name` save path does not thread the session's + // active org, and AI-authored metadata is written env-wide too. So an + // uninstall issued by a session that HAS an active org (the dispatcher + // door, `packages/runtime/src/domains/packages.ts`, is the one that + // resolves and passes `organizationId`) selected only the handful of + // rows that happened to be org-scoped and left the rest behind — + // reporting `deletedCount` > 0 and `success: true` while the package's + // rows demonstrably survived (the orphaned-uninstall bug). + // + // Same defect and same remedy as the #3115 "orphaned draft" bug one + // file over ({@link SysMetadataRepository.listDrafts}), and the shape + // is deliberately identical to it. The driver's own implicit tenant + // wall already reads this way (`field = :tenant OR field IS NULL`, + // #2734); only author-supplied predicates are strict, which is what + // made this silent. + // + // The no-org branch is deliberately NOT narrowed to `organization_id + // IS NULL`: the other door of this route (the direct-mount REST + // registrar, `packages/rest/src/package-routes.ts`) passes no + // `organizationId` at all, and restricting it to env-wide rows would + // orphan every org-scoped row — the same bug, re-created on the other + // door. Absent an org, a full uninstall stays package-wide. + if (request.organizationId) { + where.$or = [ + { organization_id: request.organizationId }, + { organization_id: null }, + ]; + } const rows = (await this.engine.find('sys_metadata', { where })) as any[]; const dropStorage = request.keepData !== true; diff --git a/packages/runtime/src/package-uninstall-org-scope.integration.test.ts b/packages/runtime/src/package-uninstall-org-scope.integration.test.ts new file mode 100644 index 0000000000..f3e01bda3e --- /dev/null +++ b/packages/runtime/src/package-uninstall-org-scope.integration.test.ts @@ -0,0 +1,260 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Real-engine regression for #7705 — `protocol.deletePackage` found ZERO +// `sys_metadata` rows the data plane found three of, and uninstall left them +// orphaned (the persistence half of #7557; PR #7700 shipped the envelope half +// and deliberately did not patch this from the consumer side). + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { + SysMetadataObject, + SysMetadataHistoryObject, + SysMetadataAuditObject, +} from '@objectstack/metadata-core'; + +/** + * The mechanism, MEASURED (not the one the card guessed at first). + * + * `deletePackage` selected its rows with a strict `organization_id` equality: + * + * const where = { package_id: request.packageId }; + * if (request.organizationId) where.organization_id = request.organizationId; + * + * Two candidates were live when this was dispatched, and the second is + * FALSIFIED by measurement, so it is recorded here rather than left implied: + * + * (a) the caller supplies an `organizationId` and strict equality drops rows + * stored env-wide (`organization_id IS NULL`); + * (b) the protocol's `this.engine` is scoped differently from the data + * plane's — an org-injecting wrapper, a separate registration for + * `sys_metadata`, or a visibility rule — so an IDENTICAL `where` returns + * different rows on the two seams. + * + * (b) is false. `findData` — the `GET /api/v1/data/sys_metadata` path that + * returned three rows — issues `this.engine.find(object, options)` on the very + * same engine instance this method uses, and the engine injects no org + * predicate of its own: a bare `new ObjectQL()` carries zero middlewares, and + * the driver receives the author-supplied `where` verbatim. So (a) is the + * mechanism, and it is what these tests pin. + * + * Why the miss is the COMMON case rather than a corner: env-wide is where a + * package's metadata normally lands (the REST `PUT /meta/:type/:name` save + * path does not thread the session's active org, and AI-authored metadata is + * written env-wide too), while the door that resolves an org and passes it — + * the dispatcher twin at `packages/runtime/src/domains/packages.ts`, the door + * whose `persisted:` envelope the issue quotes — is the one users hit with an + * active session. So the uninstall selected only whichever rows happened to be + * org-scoped and reported `success: true` over the survivors. + * + * The remedy is the shape this codebase already uses for exactly this defect + * class: `$or [{organization_id: oid}, {organization_id: null}]`, from the + * #3115 "orphaned draft" fix in `SysMetadataRepository.listDrafts` + * (`packages/metadata-protocol/src/sys-metadata-repository.ts`). The SQL + * driver's own implicit tenant wall already reads this way too (`field = + * :tenant OR field IS NULL`, #2734) — only author-supplied predicates are + * strict, which is what made this silent. + * + * --------------------------------------------------------------------------- + * Why this suite uses the REAL engine and the REAL driver + * --------------------------------------------------------------------------- + * `{success: true, deletedCount: 0}` against a package that has no rows is + * indistinguishable from this bug, so an assertion on the CALL — "was + * `deleteMetaItem` invoked with these arguments" — proves nothing here. Both + * existing `deletePackage` suites are call-shaped for that reason and neither + * could have caught this: they stub `engine.find` to hand back the rows the + * test wants and mock `deleteMetaItem` so nothing is ever deleted. This suite + * therefore SEEDS rows through the real `saveMetaItem` write path, runs the + * real uninstall, and asserts on WHICH ROWS SURVIVE in SQLite afterwards. + * A hand-built double is specifically what cannot answer this: the whole + * question is whether `organization_id = 'org'` matches a NULL column, which + * is a property of the driver's SQL, not of a stub's `filter()`. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE the revert was run + * --------------------------------------------------------------------------- + * Restoring `where.organization_id = request.organizationId` in place of the + * `$or` was predicted to turn ONLY the org-scoped-uninstall case red — + * `deletedCount` 4 → 1 with the three env-wide rows surviving — and to leave + * every negative case green, because strict equality is NARROWER than the + * `$or`: it cannot reach another org's rows or another package's rows, and it + * does not touch the no-org branch at all. Measured on revert: exactly that. + * `deletedCount` came back 1, `reprob_a` / `reprob_b` / `reprob_v` survived, + * and the three negative cases stayed green. The negatives are the control + * that keeps a future "fix" from over-widening the predicate — deleting rows + * that should have stayed is worse than the orphaning this closes. + */ + +const PKG = 'com.repro.b'; +const OTHER_PKG = 'com.other'; +const PLATFORM_PKG = '@objectstack/platform-objects'; +const ACTIVE_ORG = 'org_active'; +const OTHER_ORG = 'org_other'; + +let cleanup: Array<() => void> = []; +afterEach(() => { + for (const c of cleanup) c(); + cleanup = []; +}); + +/** REAL ObjectQL wired to a REAL SqlDriver over on-disk better-sqlite3. */ +async function boot() { + const dir = mkdtempSync(join(tmpdir(), 'os-7705-')); + cleanup.push(() => rmSync(dir, { recursive: true, force: true })); + + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + // `sys_metadata` is the table under test; the history/audit tables are the + // ones `saveMetaItem` and `deleteMetaItem` write through on the real path. + const objects = [SysMetadataObject, SysMetadataHistoryObject, SysMetadataAuditObject] as any[]; + await driver.initObjects(objects); + + const engine = new ObjectQL(); + engine.registerDriver(driver as any, true); + await engine.init(); + // Registered under the PLATFORM package, which must not be the package under + // test: `deletePackage` also unregisters its package from the live registry, + // and owning `sys_metadata` from `PKG` would tear the table out from under + // the post-uninstall assertions that read the surviving rows back. + for (const o of objects) engine.registry.registerObject(o, PLATFORM_PKG); + cleanup.push(() => { void engine.destroy(); }); + + // `'package-author'` is the genuine control-plane assembly's channel — the + // #4463 runtime authoring gate is for environment-channel writes and would + // otherwise refuse the seeding saves below. + const protocol = new ObjectStackProtocolImplementation(engine as any, undefined, undefined, 'package-author'); + return { engine, protocol }; +} + +const viewBody = (name: string) => ({ + name, + label: name, + type: 'grid', + data: { provider: 'object', object: 'anything' }, + columns: ['id'], +}); + +/** + * Seed through the REAL write path so every row carries the checksum and + * history the real uninstall reads back. Views (not objects) so the assertions + * stay on row survival rather than on physical-table teardown, which + * `deleteMetaItem` handles separately and which #7705 is not about. + */ +async function seed(protocol: any) { + const save = (name: string, packageId: string, organizationId?: string) => + protocol.saveMetaItem({ + type: 'view', + name, + item: viewBody(name), + packageId, + ...(organizationId ? { organizationId } : {}), + }); + + // The suspected — and confirmed — miss: env-wide rows, `organization_id IS NULL`. + await save('reprob_a', PKG); + await save('reprob_b', PKG); + await save('reprob_v', PKG); + // Same package, the caller's OWN org: the only rows the strict equality found. + await save('reprob_own', PKG, ACTIVE_ORG); + // Negative 1 — same package, ANOTHER org. Must survive an org-scoped uninstall. + await save('reprob_foreign', PKG, OTHER_ORG); + // Negative 2 — ANOTHER package, env-wide. Must survive either way. + await save('other_a', OTHER_PKG); +} + +/** Every surviving row, as `name[pkg,org]`, read straight back out of SQLite. */ +async function survivors(engine: any): Promise { + const rows = (await engine.find('sys_metadata', { where: {} })) as any[]; + return rows.map((r) => `${r.name}[${r.package_id},${r.organization_id ?? 'ENV'}]`).sort(); +} + +const namesFor = async (engine: any, packageId: string): Promise => { + const rows = (await engine.find('sys_metadata', { where: { package_id: packageId } })) as any[]; + return rows.map((r) => r.name).sort(); +}; + +describe('#7705 — org-scoped uninstall must not orphan env-wide sys_metadata rows', () => { + it('removes the env-wide rows too, and counts them (was: found 1 of 4, left 3 orphaned)', async () => { + const { engine, protocol } = await boot(); + await seed(protocol); + + // Precondition: the rows the uninstall is supposed to remove really exist, + // so a passing assertion below cannot be the vacuous "nothing was there". + expect(await namesFor(engine, PKG)).toEqual( + ['reprob_a', 'reprob_b', 'reprob_foreign', 'reprob_own', 'reprob_v'], + ); + + const res: any = await (protocol as any).deletePackage({ + packageId: PKG, + organizationId: ACTIVE_ORG, + }); + + // The CONSEQUENCE: no row of this package survives in the caller's scope + // (its own org + env-wide). Before the fix `reprob_a`, `reprob_b` and + // `reprob_v` were all still here. + expect(await namesFor(engine, PKG)).toEqual(['reprob_foreign']); + + // …and the receipt matches what was actually seeded in that scope — 3 + // env-wide + 1 own-org. It reported 1 before, while claiming success. + expect(res.deletedCount).toBe(4); + expect(res.failedCount).toBe(0); + expect(res.success).toBe(true); + expect(res.deleted.map((d: any) => d.name).sort()).toEqual( + ['reprob_a', 'reprob_b', 'reprob_own', 'reprob_v'], + ); + + // The complete post-state, so nothing else moved either way. + expect(await survivors(engine)).toEqual([ + `other_a[${OTHER_PKG},ENV]`, + `reprob_foreign[${PKG},${OTHER_ORG}]`, + ]); + }); + + it('does NOT sweep up another organization’s rows', async () => { + const { engine, protocol } = await boot(); + await seed(protocol); + + await (protocol as any).deletePackage({ packageId: PKG, organizationId: ACTIVE_ORG }); + + // `reprob_foreign` belongs to a different tenant and was never in scope. + // Over-widening the predicate to catch the env-wide rows would delete data + // that should have stayed — worse than the bug being closed here. + const rows = (await engine.find('sys_metadata', { + where: { package_id: PKG, organization_id: OTHER_ORG }, + })) as any[]; + expect(rows.map((r: any) => r.name)).toEqual(['reprob_foreign']); + }); + + it('does NOT sweep up another package’s rows', async () => { + const { engine, protocol } = await boot(); + await seed(protocol); + + await (protocol as any).deletePackage({ packageId: PKG, organizationId: ACTIVE_ORG }); + + expect(await namesFor(engine, OTHER_PKG)).toEqual(['other_a']); + }); + + it('an uninstall with NO org still clears the whole package (the other door)', async () => { + const { engine, protocol } = await boot(); + await seed(protocol); + + // The direct-mount REST registrar (`packages/rest/src/package-routes.ts`) + // calls `deletePackage({ packageId })` with no org at all. Narrowing THAT + // branch to `organization_id IS NULL` — the other half of the #3115 shape + // — would orphan every org-scoped row instead, i.e. re-create this bug on + // the other door. This case pins that the no-org branch stays package-wide. + const res: any = await (protocol as any).deletePackage({ packageId: PKG }); + + expect(await namesFor(engine, PKG)).toEqual([]); + expect(res.deletedCount).toBe(5); + expect(await namesFor(engine, OTHER_PKG)).toEqual(['other_a']); + }); +});