Skip to content

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

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-7970-uninstall-order
Aug 12, 2026
Merged

fix(objectql): a refused package uninstall mutates nothing (#7970)#8105
os-zhuang merged 4 commits into
mainfrom
claude/issue-7970-uninstall-order

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#7970

SchemaRegistry.uninstallPackage has exactly one step that can refuse:
unregisterObjectsByPackage, which throws when the package owns an object another
package extends (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 the protection half-tore down the very package it
was protecting.

This is a consistency fix, not new policy — the invariant was already written into
this function, three lines below the defect, by #7221:

Runs AFTER the object verb because that one can refuse (ADR-0029 extenders): a refused
uninstall must remove nothing at all.

The unregisterItemsByPackage call that comment describes obeyed it. The
unregisterNamespace call directly above it did the opposite.

Every mutation in uninstallPackage, and where the refusal sat

Enumerated rather than assumed, because the card names only the namespace:

stepbeforeafter
unregisterNamespacebefore the refusalafter
unregisterObjectsByPackage (the refusing verb)2nd1st
unregisterItemsByPackageafterafter
package record deleteafterafter

Only the namespace was exposed at this level — the other two were already correctly
placed. The verb is safe as the first step because it reads objectContributors only; it
depends on nothing the steps below establish, so a straight hoist restores the ordering
with no compensating change.

The second limb: the refusing verb was itself partially mutating

Hoisting only the namespace would have left the card's own headline defect — "a
rejected uninstall leaves the package half-mutated"
— alive in a second place.
unregisterObjectsByPackage decided the refusal inline, one object at a time, inside
the walk that removes them
. A package owning account (free) and contact (extended
by another package) therefore lost account on its way to refusing over contact. The
comment above says "remove nothing at all", not "nothing except the objects we got
through before we noticed".

Fixed with the same tool one level down: a refusal pass over every object, then a
mutation pass. Same predicate, same iteration order — so the same object still refuses
with the same message, and only the removals that used to precede the throw are gone.
Deliberately not a second public canUninstall(id) probe (that leaves two places
that must agree) and not a transactional collect-and-apply (no journal, no rollback,
nothing to unwind) — both were refused by name in triage; neither is here.

unregisterObjectsByPackage now documents the guarantee its callers depend on: a throw
is a no-op, which is what makes it safe to run ahead of a caller's own mutations. That
also covers its other callers (MetadataFacade.unregisterPackage,
ObjectQL.unregisterObject), which were exposed to the same partial walk.

Tests

Six, in packages/objectql/src/registry.test.ts. Three witness the defect and go red
when registry.ts is reverted; three stay green under both versions, which is what
makes them regression pins rather than restatements of the fix:

Red on revert:

  • a refused uninstall leaves the namespace still resolvingthe assertion the card
    names.
    No existing test refused and then inspected the namespace, which is exactly
    why the defect was invisible to a fully green suite.
  • refuses before removing anything — a free sibling object survivesthe second-limb
    pin
    , and the only multi-object case at the verb level. A single-object fixture passes
    under the namespace-only fix too; this one does not.
  • a refused uninstall leaves the whole package intact, not just the namespace — the
    same multi-object shape through uninstallPackage, asserting every limb the function
    mutates (namespace, both objects, the item half, the package record).

Green under both (measured, by running them against the pre-fix registry.ts):

  • the refusal still names the first refusable object and all its extendersmessage
    identity, checked rather than claimed.
    With two refusable objects and three
    extenders, the first object walked still wins and its extenders are still listed in
    registration order, so the full message string is byte-identical to the pre-fix one.
  • force still removes the owner even with a free sibling ahead of it — the force: true
    path skips the refusal pass and removes exactly what it removed before.
  • the successful path still releases the namespace after the object verb.

Reverse verification was run from a committed state, against the exact pre-fix
registry.ts this branch merged (021622ada^2): 3 failed, 3 passed, and the other 93
tests in the file were unmoved — so nothing in the existing suite depended on the old
ordering.

Grade: latent

No in-tree caller reaches the refusal path today, so no shipped behaviour was observably
broken. This restores the invariant before one does. Not an incident.

One note for the next person to locate this defect

It had three different addresses in one day: triage measured registry.ts:2586-2608,
my branch point (f78dd23) had it at :2636-2667, and after merging origin/main it
sits at :2725. registry.ts took two unrelated merges in that window while being 3,000
lines long. Locate this seam by symbol — uninstallPackage and
unregisterObjectsByPackage — never by the line numbers in a card.

Verification

  • pnpm --filter @objectstack/objectql test — 190 files, 3382 tests, all passing
  • pnpm --filter @objectstack/objectql typecheck — clean
  • pnpm lint on both changed files — clean
  • pnpm check:durability-log-level, pnpm check:engine-double-contract,
    pnpm check:query-options-erasure, pnpm check:type-check-debt --re-measure
    (on a full built closure), node scripts/check-engine-split-ratio.mjs,
    pnpm check:nul-bytes — all exit 0, no baseline raised, no ledger touched

Generated by Claude Code

@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 3:37pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/objectql.

15 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/objectql)
  • content/docs/data-modeling/formulas.mdx(via packages/objectql)
  • content/docs/deployment/migration-from-objectql.mdx(via @objectstack/objectql)
  • content/docs/deployment/vercel.mdx(via @objectstack/objectql)
  • content/docs/kernel/contracts/data-engine.mdx(via @objectstack/objectql)
  • content/docs/kernel/runtime-services/examples.mdx(via packages/objectql)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/objectql)
  • content/docs/kernel/services.mdx(via @objectstack/objectql)
  • content/docs/permissions/authentication.mdx(via @objectstack/objectql)
  • content/docs/permissions/system-context.mdx(via packages/objectql)
  • content/docs/plugins/index.mdx(via @objectstack/objectql)
  • content/docs/plugins/packages.mdx(via @objectstack/objectql)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/objectql)
  • content/docs/protocol/objectql/query-syntax.mdx(via packages/objectql)
  • content/docs/protocol/objectql/state-machine.mdx(via @objectstack/objectql)

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

  • content/docs/releases/implementation-status.mdx(via @objectstack/objectql)

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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] uninstallPackage unregisters the namespace BEFORE the verb that can refuse — a rejected uninstall leaves the package half-mutated

1 participant

@os-zhuang