Skip to content

fix(metadata-protocol): uninstall no longer orphans env-wide sys_metadata rows (#7705) - #7771

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-7705-delete-package-org-scope
Aug 11, 2026
Merged

fix(metadata-protocol): uninstall no longer orphans env-wide sys_metadata rows (#7705)#7771
os-zhuang merged 2 commits into
mainfrom
claude/issue-7705-delete-package-org-scope

Conversation

@claude

@claudeclaudeBot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Fixes#7705
Part of #7557

What was measured, and what it falsified

The card dispatched this diagnose-first, because its named mechanism might not be the mechanism. Two candidates were live:

  • (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 sys_metadata registration, or a visibility rule — so an identicalwhere returns different rows on the two seams.

(b) is falsified.findData — the GET /api/v1/data/sys_metadata path that returned 3 rows — issues this.engine.find(object, options) (protocol.ts:6803) on the very same engine instance deletePackage uses. 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 (measured — the driver saw exactly {package_id} and {package_id, organization_id}, nothing added). So the $or here is not a workaround over a deeper defect, and PD#5 is satisfied.

(a) is the mechanism, measured end-to-end on a real ObjectQL engine over a real SqlDriver/SQLite, seeded through the real saveMetaItem write path:

uninstallrows selectedenv-wide rows afterreceipt
{packageId, organizationId}before1 of 43 survive (orphaned)deletedCount: 1, success: true
{packageId, organizationId}after4 of 40 survivedeletedCount: 4, success: true

Note the receipt is worse than the card's success:false — a nonzero deletedCount and success: true reported over surviving rows.

Why the miss is the common case, not 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; 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:782, whose persisted: envelope the issue quotes — is the one a user with an active session hits. The other door (packages/rest/src/package-routes.ts:440) passes no org at all.

The fix

packages/metadata-protocol/src/protocol.ts — one line of behaviour, in deletePackage:

if(request.organizationId){where.$or=[{organization_id: request.organizationId},{organization_id: null},];}

Deliberately the same shape as the #3115 "orphaned draft" fix in this package (sys-metadata-repository.ts:896-899), and the same shape the SQL driver's own implicit tenant wall already uses (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 half of the #3115 shape). The direct-mount REST door passes no organizationId, so restricting that branch to env-wide rows would orphan every org-scoped row instead — the same bug, re-created on the other door.

The pin

packages/runtime/src/package-uninstall-org-scope.integration.test.ts (new, 4 cases).

{success: true, deletedCount: 0} against a package with no rows is indistinguishable from this bug, so a call-shaped assertion proves nothing here — and that is exactly what both existing deletePackage suites are (protocol-package-lifecycle.test.ts, durable-package.test.ts stub engine.find to hand back the rows the test wants and mock deleteMetaItem so nothing is ever deleted). Neither could have caught this.

This pin therefore uses a real engine and a real driver — 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() — seeds rows through the real saveMetaItem, runs the real uninstall, and asserts which rows survive in SQLite afterwards:

  1. env-wide rows are removed, and deletedCount equals what was seeded in scope (4);
  2. another organization's rows for the same package survive;
  3. another package's rows survive;
  4. a no-org uninstall still clears the whole package (guards the other door against a future over-narrowing).

Cases 2–4 are the control: a fix that over-widens the predicate deletes data that should have stayed, which is worse than the orphaning this closes.

Reverse verification — direction predicted BEFORE the revert

Predicted: restoring the strict equality turns only case 1 red (deletedCount 4 → 1, the three env-wide rows surviving) and leaves 2–4 green, because strict equality is narrower than the $or — it cannot reach another org's or another package's rows, and it does not touch the no-org branch.

Measured on revert: exactly that. 1 failed | 3 passed, with ['reprob_a','reprob_b','reprob_foreign','reprob_v'] surviving where ['reprob_foreign'] was expected. Prediction held.

Scope held

  • protocol.ts:12085 (listCommits, sys_metadata_commit) left untouched — it carries the byte-identical strict-equality pattern and the measured mechanism does implicate it (same predicate, same engine, same NULL semantics). Reported, not fixed here, per the card's fence — it wants its own card. Different table, different symptom (missing commit history, not orphaned rows).
  • MetadataFacade.unregisterPackage removes only object contributors — every non-object item the package shipped stays registered #7221 (in-memory MetadataFacade.unregisterPackage) untouched — this is the persisted layer only.
  • No content/docs/releases/ edits; a .changeset/ entry is included instead. No docs/adr/** changes.

Observation, not fixed here

An uninstall with no organization (the REST door) matches {package_id} alone and therefore deletes every organization's rows for that package — measured: 5 of 5, including a foreign org's row. That is pre-existing and unchanged by this PR (and case 4 pins it deliberately, since narrowing it would re-create the orphaning). Whether a full uninstall should be cross-tenant is a product question, not this card's defect. Flagging for a separate card. Related: resolveActiveOrganizationId (http-dispatcher.ts:1687) reads authService?.auth?.api ?? authService?.api with no getApi() fallback and is already carried as an open finding (#4127 batch 5) — it decides which of the two behaviours a given deployment gets.

Gates

gateresult
@objectstack/metadata-protocol✅ 1066
@objectstack/objectql✅ 3237
@objectstack/runtime✅ 2033 (130 files)
@objectstack/rest✅ 1462
pnpm build (closure)✅ 71/71
pnpm check:type-check-debt✅ none above recorded — see note
pnpm check:query-options-erasure✅ 67 unswept, none new
pnpm check:tenant-chokepoint✅ 19 bindings, all reads scoped
pnpm check:engine-double-contract✅ 150 pinned
pnpm check:driver-memory-census✅ (no new declaration — pin uses the real SqlDriver)
pnpm check:nul-bytes / check:error-code-casing / changeset gates
eslint (changed files)

Ratchet note, recorded rather than paid around: the first draft of the pin raised @objectstack/runtime TEST_DEBT 227 → 228 (a single TS2554registry.registerObject requires packageId). The ledger was not raised; the error was fixed by passing the platform package id explicitly, which the object registration wanted anyway — the objects must not be owned by the package under test, or deletePackage's registry unregistration would tear the table out from under the post-uninstall assertions. Re-measured at 227, the recorded number. Note this is invisible to packages/runtime's own typecheck script, whose tsconfig excludes *.test.ts; it only appears under the ratchet's raw tsc --noEmit.

CI has not converged yet — opened as a draft immediately per #6644 L2; the reviewer reads CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GKUiYq4A42J7Aa1QfkKyiX


Generated by Claude Code

…data rows (#7705)
`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GKUiYq4A42J7Aa1QfkKyiX
@vercel

vercelBot commented Aug 11, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 11, 2026 2:20pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol.

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

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)

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

  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol)

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.

protocol.deletePackage finds zero sys_metadata rows the data plane finds 3 of — uninstall leaves orphaned rows (persistence half of #7557)

2 participants

@os-zhuang@claude