Repository navigation
fix(cloud-connection): an install-local uninstall runs the protocol's registered uninstall cleanups - #21512
Conversation
…ission set or grant behind Reproduces the defect at the public door on both orders of events (hot install -> DELETE -> restart; install -> restart -> DELETE -> restart). Red on main until the uninstall runs the registered cleanups. Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
… registered uninstall cleanups DELETE /api/v1/marketplace/install-local/:manifestId removed the ledger entry and nothing else, so the package's managed_by: package permission sets and every grant of them outlived the uninstall. The door now calls the protocol's uninstall-cleanup runner (the registry deletePackage runs) once the ledger entry is gone, with the manifest id and no organization, and answers each outcome as cleanups. No second revocation path lives in cloud-connection. The runner itself (protocol.runUninstallCleanups) is a metadata-protocol edit sequenced separately; until it lands this door reports one failed outcome naming it rather than an empty list. Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
…ll cleanups Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
…anups Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 2 package(s): 19 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 292fffcece61c7fa6c02ae8aff7fba06b534fa84 && git checkout 292fffcece61c7fa6c02ae8aff7fba06b534fa84
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 6dd99b82c38cd68b51c86241d53c5b7dda670a08 cd5cabce34aff75de8855ca4aa501c18d3a55020 && git checkout -B drift-repro 6dd99b82c38cd68b51c86241d53c5b7dda670a08 && git merge --no-ff cd5cabce34aff75de8855ca4aa501c18d3a55020
node scripts/docs-audit/affected-docs.mjs --json 6dd99b82c38cd68b51c86241d53c5b7dda670a08
|
…stall-local-uninstall-cleanups
…runUninstallCleanups deletePackage's step-7 loop over the registerUninstallCleanup registry moves verbatim into a public runUninstallCleanups method, and deletePackage calls it. The only change inside the loop is the warn tag, now [protocol.runUninstallCleanups]. install-local's uninstall door calls the same runner, so both doors run one registry through one runner. The new test pins the args each cleanup receives, failure as an outcome with driver text withheld, and the control: deletePackage reports exactly the runner's outcomes. Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
…anups; Clause-② is yes The extracted runner is a new public method on the exported ObjectStackProtocolImplementation, an additive widening of @objectstack/metadata-protocol's public surface, so that package is graded minor and the declaration line reads Clause-②: yes. cloud-connection stays a patch. Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
A third order of events in the uninstall pin: hot install, DELETE, then a hot install of another package, then restart. The DELETE leaves the package registered until the restart, and the second install's metadata:reloaded re-runs plugin-security's declared-permission seeding over it, so the uninstalled package's set is re-projected as a fresh managed_by: package row and survives the restart as an orphan. Its grant stays revoked; that half is a plain assertion. The two set readings are it.fails, measured red and reported for filing, not fixed here. Claude-Session: https://claude.ai/code/session_016GiHYRmLSNWTfbX9gVQkpz Co-Authored-By: Claude <noreply@anthropic.com>
…stall-local-uninstall-cleanups
Contract reviewServed-tier: Inputs: card #21490 (body and all 7 comments, triage through ACCEPT), PR #21512 (body, 9-file list, net diff against ① Derived judgments
Nothing in the diff was judged wrong. ② Semver level
③ Boundary flagsOpen question, round 1 (Clause-② A or B for the protocol edit) — answered by the seat ( Open questions, round 2 — none declared; none found. Round-1 deviations, each resolved at the head:
Round-2 deviations:
Out-of-scope findings:
Check-runs on the head: every latest run per name is Escalated: nothing. Implemented-by: VERDICT: PASS Generated by Claude Code |
…is not offered; the admin read says why (objectstack-ai#21580) Part of objectstack-ai#21476 Clause-②: no This delivers the doors half and the administrator's read half of the triage ruling (`5962758813`). The publish half is left open on purpose: its contract-faithful channel sits behind `packages/metadata-protocol/src/protocol.ts`, which open PRs hold. The call sites are under "Not in this PR". The keyword is `Part of`, so the card stays open after this merges, while that half waits for a decision. ## What On a walled tenancy posture, a public form bound to an object walled by an organization column is no longer offered to anonymous visitors. An anonymous submission carries no organization. On a walled posture the engine refuses an insert without one into such an object (`resolveSystemInsertOrganization`). So the form was served (`GET /forms/contact-us` 200), and then every submit answered `500 ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED`. This was reproduced on `origin/main` (`6dd99b82c3`) with `bootStack(showcaseStack, { multiTenant: 'posture-only' })`. - **One predicate.** `anonymousFormIntakeUnavailability` in `packages/rest/src/rest-server.ts` returns null when the form can take intake, otherwise why it cannot. It reads two facts and restates neither: - the tenancy service's in-force `posture`. This is the value SecurityPlugin hands the engine (`setTenancyPostureProvider`), so a degraded walled request reads `single` there, and the engine derives the install's organization. - the wall column, from `@objectstack/metadata-core`'s existing `resolveRecordWallOrganizationField`, read over the served object schema (which carries the injected `organization_id`). The object is read only once a wall is in force, so single-posture deployments pay nothing new. - **Both doors read it in one place.** It is called inside `resolveFormBySlug`, the one resolution both `GET /forms/:slug` and `POST /forms/:slug/submit` already call. An unavailable form resolves to `null`, which is exactly the withdrawn form's `404 FORM_NOT_FOUND`, byte for byte. Anonymous callers learn nothing about the tenancy, and there is no second check per door. - **The administrator's read names why.** `GET /meta/view/:name` puts one warning in `item._diagnostics.warnings` per unavailable open form. It sits on both arms (cached and uncached), is located at the form's `sharing` (`config.sharing`, `formViews.KEY.sharing` or `form.sharing`), and names the slug, the object, the wall column, the posture and the remedy (`tenancy: { enabled: false }` when the rows belong to no organization). The reason depends on facts the protocol's validator never hashes, so on the cached arm a `view`'s If-None-Match is compared in REST against an ETag with the reason folded in (the ADR-0106 D3 shape). With no reason, the ETag and the 304 are byte-identical to before (pinned). ### Placement: the predicate lives in `rest`, not `metadata-core` The dispatch assumed `metadata-core`'s `anonymous-form-intake.ts`. I measured that first: any new export there enlarges `@objectstack/metadata-core`'s published index (`export *`), which is a `Clause-②: yes (widening)` change by objectstack-ai#21566's own grading. The dispatch pins `Clause-②: no`. Every reader of the predicate (two doors, one admin read) is in `rest-server.ts`. The predicate composes two rules that are already shared (`postureEnforcesWall` from spec, `resolveRecordWallOrganizationField` from metadata-core), so no rule gains a second spelling. It moves into `metadata-core` on the day a reader outside `rest` exists, for example the publish half's option A below. ## Pins - `packages/rest/src/public-form-intake-availability.test.ts` (16 tests): - The doors are enumerated off the registered routes. The set under `/forms/` must be exactly the two doors, and each door on each walled posture (`isolated`, `group`) is its own row: it answers the withdrawn form's answer byte for byte, and `createData` is never called. - Controls, all accepted: a `tenancy: { enabled: false }` object, the single posture (where the object is not even read), a degraded walled request, and no tenancy service. - Admin read on both arms: the located warning appears on the walled cases, and `_diagnostics` is untouched on the controls. - Validator: the bare protocol ETag revalidates into the reason, the folded ETag gives 304, and the control's ETag is unchanged and still gives 304. - `packages/qa/dogfood/test/showcase-public-form-walled-intake.dogfood.test.ts` (real walled showcase boot): - both doors' raw answers equal the same form's answers once withdrawn env-wide on the same boot, and no `showcase_inquiry` row lands; - the admin read names the reason at `config.sharing`. - `public-form-withdrawal-walled.dogfood.test.ts`: the dogfood control. A tenancy-disabled object on the walled boot still accepts intake, and its admin read now also asserts that no intake warning is present. ## Ablation (one-shot, not kept) Each mutation was applied with `scripts/ablation-replace.mjs` against `packages/rest/src/rest-server.ts`, whose HEAD blob is `239ac2d6` at both `8a8839f9ab` and the final `66ee5294a6`. The direction was predicted before each run. - **A, unit leg: the doors stop reading the predicate.** `return unavailable ? null : { ...match, organizationId };` became `return { ...match, organizationId };` (anchor 1 to 0, blob `239ac2d6` to `43fdf709`). - Predicted: exactly the 4 door rows red, everything else green. - Observed: 4 failed, 12 passed. The GET rows received 200 and the POST rows 201 where 404 was expected; the admin-read rows stayed green because they reach the predicate from their own call site. - Restore: blob equals HEAD and `git diff HEAD` is empty. - **A, dist leg (dogfood).** - The first attempt was a no-op. The same replacement left `unavailable` unused, and the DTS build refused it (`noUnusedLocals`) after the JS bundle had already been emitted, so no test ran. I rebuilt from the restored source, and `ablation-dist-preflight` confirmed the guard present in `dist/index.js` and `dist/index.cjs` with a clean tree. - Re-run with `return unavailable && false ? null : { ...match, organizationId };` (blob `2b2e9c9f`), rebuilt; the preflight found the plant marker in 2 built files. - Predicted: 1 red. Observed: 1 failed (`expected 200 to be 404` on the doors row), 8 passed. - Restore: blob equals HEAD, rebuilt; the preflight found the guard in 2 files, the mutation absent from all 6, and a clean tree; the dogfood run went back to 9/9. - **B: the predicate itself answers "available".** `return tenantField === null ? null : ...` became `return tenantField === null || true ? null : ...` (blob `25ef3c8e`). - Predicted: 7 red (4 door rows, plus the 3 walled admin-read rows: uncached, cached, validator) and 9 green. - Observed: 7 failed, 9 passed. Restore: blob equals HEAD and the tree is clean. The pins asked for an ablation "on one door". There is no per-door read site to ablate: the predicate is read once, in the resolution both doors call. Ablation A removes that one read, and each door's own row goes red. ## Tests (at `66ee5294a6`, after merging `origin/main` `44072fc2b9`) - `@objectstack/rest` full suite (`--project local`): 258 files passed; 4883 tests passed, 326 skipped. `typecheck`: `tsc --noEmit` clean, and `check:test-typecheck` OK. - `@objectstack/metadata-core`: 17 files / 311 passed; `typecheck` clean (it is unchanged). - `@objectstack/dogfood` `typecheck` clean. Public-form dogfood files (walled intake, walled withdrawal, showcase withdrawal, showcase public form, read-back masking): 5 files / 20 passed. - `@objectstack/runtime` `/meta` parity census, run because it reads `rest-server.ts` source and `rest`'s `dist/`. The four files `meta-list-projection-parity`, `meta-item-read-gate-parity`, `meta-read-org-scope-parity` and `meta-item-envelope` gave 780 passed. - `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands`: 68 commands; 67 exit 0. `check:dual-build-cjs-loads` answered PREREQUISITE NOT MET (it needs a full workspace build), so it is NOT MEASURED and left to CI. The `--ran` reconciliation accounted for 68 of 68: 67 run, 1 NOT MEASURED, 0 unrun. - eslint, narrowed to the 4 changed `.ts` files (`--no-inline-config --format json`): 4 files, 0 errors, 0 warnings, none ignored. The fifth changed path is a changeset `.md`. The narrowing is sound because `eslint.config.mjs` never enables type-aware linting (no `parserOptions.project`), so this diff cannot move any untouched file's verdict. The full `pnpm lint` is CI's. ## Not in this PR: the publish half The ruling asks the publish surfaces (`PUT /meta/view/:name`, `POST /meta/view/:name/publish`) to say why too. The only located, structured channel those responses carry is `advisories`, and both `SaveMetaItemResponseSchema` and `PublishMetaItemResponseSchema` declare the runtime authoring gate as its producer. The places that would have to change: - `packages/metadata-protocol/src/runtime-authoring-gate.ts`: a gate-local rule would sit beside `findPlatformScheduleOrgGaps`, around line 300. - `packages/metadata-protocol/src/protocol.ts:5612`: the gate is fed `orgWallEnforced: this.orgWallEnforced()`. - `protocol.ts:5938`: `orgWallEnforced()` reads the requested posture (`postureEnforcesWall(resolveTenancyPosture())`), which disagrees with the doors' in-force reading on a degraded deployment. - `protocol.ts:18673` and `protocol.ts:19612`: the attach sites. `protocol.ts` is held by open PRs objectstack-ai#21545 and objectstack-ai#21512, so this PR stops there. The options and a recommendation are in the report on the card. ## Acceptance notes - **Boundary of the predicate.** It reads declarations. The engine also passes a federated (`external`) object, a platform object its inventory has not admitted, and a row a `beforeInsert` hook stamped. A form bound to one of those that also carries a wall column is withheld here although the engine would accept it, which is the fail-closed direction. No shipped hook stamps `organization_id` (`git grep` over the CRM and showcase hooks: exit 1, with a `beforeInsert` control at exit 0). - **New failure mode on walled postures.** An object-metadata read failure now fails both doors closed (GET `500 FORM_RESOLVE_FAILED`; submit through `mapDataError`). Single-posture deployments make no new read. - **Cached arm.** For every `view` read, If-None-Match is now compared in REST rather than in the protocol. Server work is unchanged, because `getMetaItemCached` already delegates to `getMetaItem`. Response bytes, the ETag and the 304 are identical when there is no reason. - **Not stamped by this PR.** The list read (`GET /meta/view`), `/layers`, and the runtime HTTP dispatcher's `/meta` item read. The dispatcher serves no `/forms/*` door, so a dispatcher-only composition has no intake that could be unavailable. - **CRM.** `app-crm`'s lead form (`/forms/contact-us`) is the same class on a walled CRM deployment. Not booted here. - **Console.** Whether the console renders `_diagnostics.warnings`: NOT MEASURED (no `objectui` checkout in this container). --- _Generated by [Claude Code](https://claude.ai/code/session_01DiCSbmJrkzNhuEAier4VoJ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Fixes #21490
Clause-②: yes
Status: both halves are in this diff; draft. This PR now carries the install-local door and the protocol runner the door calls. The runner is
runUninstallCleanups, a new public method on the exportedObjectStackProtocolImplementation. That widens@objectstack/metadata-protocol's public surface, so the seat ruledClause-②: yeson the card, and that package takes aminorchangeset. An at-tier contract review is owed before this PR enqueues.The defect, measured at the public door
On
main9ff7428, with an emptyos start, I installed a package that declares a permission set throughos package install. I granted the set to the operator through the data door, sentDELETE /api/v1/marketplace/install-local/com.example.tasksapp, and probed over REST.managed_by: package)handleUninstallremoved the ledger entry and nothing else.plugin-securityregisters the revocation (security.package-permissions) with the protocol's uninstall-cleanup registry, and before this change onlydeletePackageran that registry.Why not call an existing protocol door (measured)
DELETE /api/v1/packages/:idrefuses an install-local package outright:422 WRITABLE_PACKAGE_REQUIRED. The set survives, and the ledger keeps the package across a restart.deletePackage, measured over an install-local package's shape (registry entry only, nosys_packagesrow, nosys_metadatarows):400 TENANT_SCOPE_REQUIRED).allTenants: trueit answerssuccess: false(0 rows deleted).sys_packagesdelete and callsregistry.uninstallPackage, which withdraws the package from the running kernel. The door never did that, and it would leave the package's bound handlers and flows behind. It would also delete anysys_metadataoverlay rows bound to the package, dropping their tables by default.deletePackageand this door both call (the triage direction: one registration seam, both doors run it).What this PR changes
@objectstack/metadata-protocol(minor)runUninstallCleanups({ packageId, organizationId?, actor? })sits right afterregisterUninstallCleanup. Its request type isDeletePackageRequestpicked down to those three keys, and it answers oneUninstallCleanupOutcomeper registered cleanup.deletePackage's step-7 loop, moved verbatim. The only change inside the loop is the warn tag, now[protocol.runUninstallCleanups]instead of[protocol.deletePackage].deletePackagecalls it in place of the loop:const cleanups = await this.runUninstallCleanups(request);. There is no other change todeletePackage, and its existing suites are the control (see Tests).protocol.uninstall-cleanups-runner.test.ts. It pins the arguments each cleanup receives (with and without an organization and actor), failure as an outcome with the driver text withheld while the remaining cleanups still run, and the control:deletePackagecalls the runner once with its own request and reports exactly the runner's outcomes.@objectstack/cloud-connection(patch)handleUninstallruns the protocol's uninstall cleanups once the ledger entry is gone, throughprotocol.runUninstallCleanups.manifest.id, and a cloud install's ledgerpackageIdis the catalog id. It carries no organization, because an install-local package is installed for the whole runtime.actoris the admitted operator.cloud-connection.data.cleanups, the waydeletePackagereports them.warnwith its remedy: install the package again, then uninstall it again. The ledger entry is gone, so retrying the DELETE answers 404.protocolservice means no registry exists, socleanups: [].protocol.runUninstallCleanups, so "nothing to revoke" and "the revocation never ran" read differently.protocolslot is uncontracted, so this door narrows it per consumer: the verb name is this file's, and the request and outcome types are the producer's (DeletePackageRequest,UninstallCleanupOutcome). This is the same shapePackagesDomainProtocoluses fordeletePackageinpackages/runtime.@objectstack/cloud-connectiontherefore declares@objectstack/metadata-protocol, which it already received through@objectstack/runtime(check:undeclared-dep-imports).Tests
All at head
cd5cabce34(this branch after mergingorigin/main6dd99b82c3), on real built packages:turbo run build --filter=@objectstack/cli^...built every dependency,@objectstack/metadata-protocoland@objectstack/cloud-connectionincluded, andablation-dist-preflightread the runner's own warn tag in both of@objectstack/metadata-protocol's runtime bundles. No dist overlay.packages/cli/test/package-install-local-uninstall-cleanups.integration.test.ts,--project integration): 10 passed, 2 expected fail (12).security.package-permissionsreportedsuccess: true, no set and no grant right after, and no set, no grant and object 404 after a restart. The previous round read this green only through a temporary overlay of the built runner; it now reads green on the real build.main(ebf0b8e329, test only) the same 8 cases read 6 failed, 2 passed. That is the reproduction.protocol.uninstall-cleanups-runner,durable-package,protocol.driver-text-disclosure,protocol.marked-refusal-classification,protocol.package-delete-refusal): 5 files, 54/54.pnpm --filter @objectstack/metadata-protocol typecheckgreen, and the full suite: 207 files passed, 3 skipped; 3192 tests passed, 19 skipped. The new test is in the tsc program (--listFiles, count 1).pnpm --filter @objectstack/cloud-connection typecheck(both programs) green, and the full suite: 33 files, 414/414. The unit pinmarketplace-install-local-uninstall-cleanups.test.tsis 8/8 in it.pnpm --filter @objectstack/cli exec vitest run --project unit: 250 files passed. The 2 that failed,published-subpath-console.pinandpublished-subpath-hook-body.pin, refused at collection becausepackages/cliitself had nodist/(packages/cli is not built). That is a prerequisite, not a reading. Afterpnpm --filter @objectstack/cli buildthey passed, 29/29.test/vitest-tiers-partition.test.tsis in the unit run.The re-seed window (dispatch A4): measured RED, pinned as
it.fails, not fixed hereOrder 3 in the integration pin: hot install of the package, grant its set, DELETE it, then a hot install of a DIFFERENT package (
com.example.notesapp), then restart.managed_by: package,package_id: com.example.tasksappWhy: this DELETE leaves the package registered in the running kernel until the next restart (the response's note says so). The other install announces
metadata:reloaded, and plugin-security's subscriber re-runsbootstrapDeclaredPermissionsover every package the kernel still holds. That re-projects the uninstalled package's set. After the restart nothing selects it again: the package is gone, the row is not. The grant does not come back, because the cleanup deleted the binding and the seeding writes none.In the pin, the precondition and the "grant stays revoked, object gone" reading are plain assertions, both green. The two set readings are
it.fails: each turns red the day its half is fixed, which is the cue to promote it to a plain assertion. The finding goes to the seat for filing (see the report on the card); this PR does not fix it.Reverse verification
Both legs ran through
scripts/ablation-replace.mjs(WRAP mode, with a planted marker), from the committed headea93d2754e.marketplace-install-local-plugin.ts, the anchorconst cleanups = await this.runUninstallCleanups(ctx, manifestId, admission.userId);(1 hit) was replaced with an empty list plus the markerABLATION-21490-DOOR. The tool reported "anchor 1 → 0, blob f9929f2f700a → 1771b1b9b2f3".ablation-dist-preflightfound the marker in both runtime bundles,index.jsandindex.cjs. The package's DTS step failed on the now-unused private helper (TS6133), but the JS bundles the suites load were emitted and carried the marker.git diff HEADis empty". Rebuilt (exit 0).ablation-dist-preflight --absent: the marker is absent from all 6 built files, and the working tree is clean against HEAD.protocol.ts, the anchorfor (const [name, cleanup] of this.uninstallCleanups) {(1 hit, now only insiderunUninstallCleanups) was replaced with a loop over an empty Map plus the markerABLATION-21490-RUNNER. The tool reported "anchor 1 → 0, blob 4308b1f47cce → 721cc1b5c8b8". The build exited 0, and the marker was present inindex.jsandindex.cjs.deletePackagesuites went red, which showsdeletePackagereally runs through the runner.git diff HEADis empty". Rebuilt (exit 0).--absent: the marker is absent from all 24 built files, and the tree is clean.Gates
cd5cabce34,node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackderived 77 commands, and all 77 exited 0.--ranreconciles to "77 derived, 77 run, 0 NOT-MEASURED, 0 UNRUN".pnpm check:dual-build-cjs-loadsfirst answeredPREREQUISITE NOT MET(exit 3: 9 packages had nodist/). Later gates in the same sweep built them, and the re-run exited 0.pnpm --filter @objectstack/spec check:generatedafter the merge: all 15 generated artifacts are up to date.pnpm lint(eslint . --no-inline-config) exited 0 atcd5cabce34.Acceptance notes
it.failsand reported for filing, not fixed here. A fix belongs where the package outlives its uninstall: in the running registry, or in the seeding's selection of packages.SchemaRegistry.uninstallPackageexists anddeletePackageuses it. The wording is kept here because withdrawing the package from the running kernel is outside this card. That withdrawal is also what would close the re-seed window.sys_packagesrow is gone. The warn names the remedy that works: install the package again, then uninstall it again.Generated by Claude Code