fix(objectql): a refused package uninstall mutates nothing (#7970) (#8105)

* wip: reorder uninstallPackage refusal ahead of mutations (#7970)

* changeset

* test: pin refusal message identity under multiple extended objects (#7970)

---------

Co-authored-by: Claude <support@objectstack.ai>

Author: os-zhuang

.changeset/uninstall-refuses-before-mutating.md

@@ -0,0 +1,35 @@
+---
+"@objectstack/objectql": patch
+---
+
+fix(objectql): a refused package uninstall now mutates nothing (#7970)
+
+`SchemaRegistry.uninstallPackage` has one step that can **refuse**:
+`unregisterObjectsByPackage`, which throws when the package owns an object
+another package `extend`s (ADR-0029 — the refusal that tells an operator to
+uninstall the extenders first). That guard exists to keep a registry whole, and
+it was reached **through** mutations, so exercising it half-tore down the very
+package it was protecting. Two limbs were exposed:
+
+- **The namespace.** `uninstallPackage` released it before calling the refusing
+  verb. A refused uninstall therefore left the package installed — record,
+  objects and items all intact — while its namespace no longer resolved, for the
+  life of the process. The invariant was already written three lines below the
+  defect ("a refused uninstall must remove nothing at all"); the code above it
+  did the opposite.
+- **The package's other objects.** `unregisterObjectsByPackage` decided the
+  refusal one object at a time, inside the walk that removes them, so a package
+  owning `account` (free) and `contact` (extended) lost `account` on its way to
+  refusing over `contact`.
+
+Both are fixed by ordering, not by a transaction or a second probe: the refusing
+verb runs first in `uninstallPackage`, and the verb itself now decides the
+refusal across every object before removing any. A throw from
+`unregisterObjectsByPackage` is a no-op, which is what lets its callers run it
+ahead of their own mutations.
+
+Unchanged: the refusal's message and the object it names, the `force: true` path
+(which skips the refusal and removes exactly what it removed before), and the
+successful uninstall's observable outcome. Grade is **latent** — no in-tree
+caller reaches the refusal path today, so no shipped behaviour was
+observably broken; this restores the invariant before one does.

packages/objectql/src/registry.test.ts

@@ -357,6 +357,60 @@ describe('SchemaRegistry', () => {
                 registry.unregisterObjectsByPackage('com.owner', true);
             }).not.toThrow();
         });
+
+        it('[#7970] refuses before removing anything — a free sibling object survives', () => {
+            // `free` is walked FIRST (Map insertion order) and used to be spliced
+            // out on the way to refusing over `important`, so the refusal that
+            // exists to keep the registry whole half-tore it down instead.
+            registry.registerObject({ name: 'free', fields: {} }, 'com.owner', 'base', 'own');
+            registry.registerObject({ name: 'important', fields: {} }, 'com.owner', 'base', 'own');
+            registry.registerObject({ name: 'important', fields: {} }, 'com.ext', undefined, 'extend');
+
+            expect(() => {
+                registry.unregisterObjectsByPackage('com.owner');
+            }).toThrow(/object "important" is extended by com\.ext/);
+
+            expect(registry.getObject('free')).toBeDefined();
+            expect(registry.getObject('important')).toBeDefined();
+        });
+
+        /**
+         * [#7970] MESSAGE IDENTITY, the half the reorder must not disturb. The
+         * refusal pass replaces an inline check, so it must name the SAME object
+         * and the SAME extenders as before — with two refusable objects the
+         * first one walked still wins, and an object's extenders are still
+         * listed in registration order. This test is deliberately written to
+         * pass BOTH before and after the fix: run it against the pre-fix
+         * `registry.ts` and it stays green, which is what proves the message did
+         * not move (only the mutations that used to precede it are gone).
+         */
+        it('[#7970] the refusal still names the first refusable object and all its extenders', () => {
+            registry.registerObject({ name: 'alpha', fields: {} }, 'com.owner', 'base', 'own');
+            registry.registerObject({ name: 'beta', fields: {} }, 'com.owner', 'base', 'own');
+            registry.registerObject({ name: 'alpha', fields: {} }, 'com.ext1', undefined, 'extend');
+            registry.registerObject({ name: 'alpha', fields: {} }, 'com.ext2', undefined, 'extend');
+            registry.registerObject({ name: 'beta', fields: {} }, 'com.ext3', undefined, 'extend');
+
+            expect(() => registry.unregisterObjectsByPackage('com.owner')).toThrow(
+                'Cannot uninstall package "com.owner": object "alpha" is extended by ' +
+                'com.ext1, com.ext2. Uninstall extenders first.',
+            );
+        });
+
+        it('[#7970] force still removes the owner even with a free sibling ahead of it', () => {
+            registry.registerObject({ name: 'free', fields: {} }, 'com.owner', 'base', 'own');
+            registry.registerObject({ name: 'important', fields: {} }, 'com.owner', 'base', 'own');
+            registry.registerObject({ name: 'important', fields: {} }, 'com.ext', undefined, 'extend');
+
+            registry.unregisterObjectsByPackage('com.owner', true);
+
+            // The refusal pass is skipped under `force`, and the mutation pass is
+            // unchanged: both of the package's contributions are gone, and the
+            // extender's own contribution is left where it was.
+            expect(registry.getObject('free')).toBeUndefined();
+            expect(registry.getObjectOwner('important')).toBeUndefined();
+            expect(registry.getObjectContributors('important')).toHaveLength(1);
+        });
     });
 
     // ==========================================
@@ -427,6 +481,64 @@ describe('SchemaRegistry', () => {
             expect(registry.getNamespaceOwner('test')).toBeUndefined();
         });
 
+        /**
+         * [#7970] The uninstall's ONE refusable step is `unregisterObjectsByPackage`
+         * (ADR-0029: you may not uninstall the owner of an object another package
+         * extends). It now runs before every mutation, so reaching that refusal
+         * costs nothing. The namespace is the limb that measured this: the release
+         * used to run FIRST, so a refused uninstall left the package installed —
+         * record, objects and items all intact — while its namespace no longer
+         * resolved, for the life of the process. No test refused and then inspected
+         * the namespace, which is exactly why the defect was invisible.
+         *
+         * Latent by grade: no in-tree caller reaches the refusal path today.
+         */
+        it('[#7970] a refused uninstall leaves the namespace still resolving', () => {
+            registry.installPackage({ id: 'com.crm', name: 'CRM', namespace: 'crm', version: '1.0.0' } as any);
+            registry.registerObject({ name: 'contact', fields: {} }, 'com.crm', 'crm', 'own');
+            registry.registerObject({ name: 'contact', fields: {} }, 'com.analytics', undefined, 'extend');
+
+            expect(registry.getNamespaceOwner('crm')).toBe('com.crm');
+
+            expect(() => registry.uninstallPackage('com.crm')).toThrow(
+                /Cannot uninstall package "com\.crm".*extended by com\.analytics/,
+            );
+
+            // The assertion the card names: refused ⇒ the namespace still resolves.
+            expect(registry.getNamespaceOwner('crm')).toBe('com.crm');
+            expect(registry.getNamespaceOwners('crm')).toEqual(['com.crm']);
+        });
+
+        it('[#7970] a refused uninstall leaves the whole package intact, not just the namespace', () => {
+            registry.installPackage({ id: 'com.crm', name: 'CRM', namespace: 'crm', version: '1.0.0' } as any);
+            // Registered ahead of the extended object, so the object walk reaches
+            // this one before it can refuse.
+            registry.registerObject({ name: 'account', fields: {} }, 'com.crm', 'crm', 'own');
+            registry.registerObject({ name: 'contact', fields: {} }, 'com.crm', 'crm', 'own');
+            registry.registerObject({ name: 'contact', fields: {} }, 'com.analytics', undefined, 'extend');
+            registry.registerItem('page', { name: 'home' }, 'name', 'com.crm');
+
+            expect(() => registry.uninstallPackage('com.crm')).toThrow(/extended by com\.analytics/);
+
+            // Every limb `uninstallPackage` mutates, in the order it mutates them.
+            expect(registry.getNamespaceOwner('crm')).toBe('com.crm');
+            expect(registry.getObject('account')).toBeDefined();
+            expect(registry.getObject('contact')).toBeDefined();
+            expect(registry.getItem('page', 'home')).toMatchObject({ name: 'home' });
+            expect(registry.getPackage('com.crm')).toBeDefined();
+        });
+
+        it('[#7970] the successful path still releases the namespace after the object verb', () => {
+            registry.installPackage({ id: 'com.crm', name: 'CRM', namespace: 'crm', version: '1.0.0' } as any);
+            registry.registerObject({ name: 'contact', fields: {} }, 'com.crm', 'crm', 'own');
+
+            expect(registry.uninstallPackage('com.crm')).toBe(true);
+
+            expect(registry.getNamespaceOwner('crm')).toBeUndefined();
+            expect(registry.getObject('contact')).toBeUndefined();
+            expect(registry.getPackage('com.crm')).toBeUndefined();
+        });
+
         it('updatePackageManifest merges editable fields, preserving lifecycle state', () => {
             registry.installPackage({ id: 'com.test', name: 'Old', version: '1.0.0' } as any);
             registry.disablePackage('com.test'); // lifecycle state that must survive an edit

packages/objectql/src/registry.ts

@@ -1838,28 +1838,53 @@ export class SchemaRegistry {
 
   /**
    * Unregister all objects contributed by a package.
-   * 
+   *
+   * [#7970] **Refuses before it mutates.** If any object this package owns is
+   * extended by another package (ADR-0029), the call throws having removed
+   * nothing — the refusal is decided across every object first. Callers may
+   * therefore treat a throw as a no-op, which is what lets
+   * {@link uninstallPackage} run this verb ahead of its own mutations.
+   *
    * @throws Error if trying to uninstall an owner that has extenders
    */
   unregisterObjectsByPackage(packageId: string, force: boolean = false): void {
+    // [#7970] REFUSAL PASS — the whole decision, taken before a single
+    // contribution is removed. This check used to live inline in the mutation
+    // walk below, one object at a time, so a package owning `account` (free)
+    // and `contact` (extended by another package) lost `account` on the way to
+    // refusing over `contact`: the guard that exists to keep a registry whole
+    // was itself reached through a mutation, and nothing rolled it back. Same
+    // predicate and same iteration order as the inline check it replaces, so
+    // the same object still refuses with the same message — what changed is
+    // only that no removal precedes the throw.
+    if (!force) {
+      for (const [fqn, contributors] of this.objectContributors.entries()) {
+        const ownedHere = contributors.some(
+          c => c.packageId === packageId && c.ownership === 'own'
+        );
+        if (!ownedHere) continue;
+        // Extenders from other packages
+        const otherExtenders = contributors.filter(
+          c => c.packageId !== packageId && c.ownership === 'extend'
+        );
+        if (otherExtenders.length > 0) {
+          throw new Error(
+            `Cannot uninstall package "${packageId}": object "${fqn}" is extended by ` +
+            `${otherExtenders.map(c => c.packageId).join(', ')}. Uninstall extenders first.`
+          );
+        }
+      }
+    }
+
+    // MUTATION PASS — carries no refusal of its own; the pass above already
+    // proved every removal below is allowed. Keep it that way: a second copy of
+    // the predicate here is the two-places-that-must-agree shape this ordering
+    // fix was chosen over.
     for (const [fqn, contributors] of this.objectContributors.entries()) {
       // Find this package's contributions
       const packageContribs = contributors.filter(c => c.packageId === packageId);
-      
-      for (const contrib of packageContribs) {
-        if (contrib.ownership === 'own' && !force) {
-          // Check if there are extenders from other packages
-          const otherExtenders = contributors.filter(
-            c => c.packageId !== packageId && c.ownership === 'extend'
-          );
-          if (otherExtenders.length > 0) {
-            throw new Error(
-              `Cannot uninstall package "${packageId}": object "${fqn}" is extended by ` +
-              `${otherExtenders.map(c => c.packageId).join(', ')}. Uninstall extenders first.`
-            );
-          }
-        }
 
+      for (const contrib of packageContribs) {
         // Remove contribution
         const idx = contributors.indexOf(contrib);
         if (idx !== -1) {
@@ -2704,14 +2729,23 @@ export class SchemaRegistry {
       return false;
     }
 
+    // [#7970] Unregister objects FIRST — this is the one step that can REFUSE
+    // (ADR-0029: the package owns an object another package `extend`s), so
+    // every mutation below is downstream of the refusal point and a refused
+    // uninstall removes nothing at all. The namespace release used to sit
+    // ABOVE this line, which meant reaching the guard that exists to keep a
+    // registry whole cost the namespace on the way in: the package stayed
+    // installed with its objects and its record intact, while its namespace no
+    // longer resolved, for the life of the process. Safe as the first step
+    // because the verb reads `objectContributors` only — it depends on nothing
+    // the steps below establish.
+    this.unregisterObjectsByPackage(id);
+
     // Unregister namespace
     if (pkg.manifest.namespace) {
       this.unregisterNamespace(pkg.manifest.namespace, id);
     }
 
-    // Unregister objects (will throw if extenders exist)
-    this.unregisterObjectsByPackage(id);
-
     // [#7221] …and everything else the package shipped. The object verb above
     // reaches `objectContributors` only, so without this an uninstall dropped
     // the package record while its `page`/`view`/`flow`/`app`/`api` entries