Skip to content

feat(registry): uninstall, update, update-check handlers + admin lifecycle#1114

Merged
ascorbic merged 5 commits into
mainfrom
feat/registry-lifecycle
May 20, 2026
Merged

feat(registry): uninstall, update, update-check handlers + admin lifecycle#1114
ascorbic merged 5 commits into
mainfrom
feat/registry-lifecycle

Conversation

@ascorbic
Copy link
Copy Markdown
Collaborator

What does this PR do?

Implements #1036 (next child of the registry roadmap umbrella #1026, after #1029/#1112): full lifecycle for registry-source plugins — uninstall, update with capability + public-route re-consent, and update-check folded into the existing /plugins/updates endpoint. Closes the "uninstall is not yet available" band-aid in PluginManager.

Server

  • POST /_emdash/api/admin/plugins/registry/:id/uninstall — deletes the R2 bundle, drops the _plugin_state row, optionally clears the plugin's _plugin_storage rows (deleteData: true). Refuses non-registry sources so a marketplace plugin sharing the id namespace can't be trashed.
  • POST /_emdash/api/admin/plugins/registry/:id/update — re-runs the install pipeline at a newer version, diffing capabilities + public-route visibility against the currently installed bundle. CAPABILITY_ESCALATION and ROUTE_VISIBILITY_ESCALATION gate widened permissions behind confirmCapabilityChanges / confirmRouteVisibilityChanges, mirroring marketplace exactly.
  • GET /_emdash/api/admin/plugins/updates is now cross-source: runs marketplace + registry checks in parallel, isolates failures, logs structured errors, returns one merged list wrapped in the standard { data: { items } } envelope.
  • Exported marketplace's diffCapabilities / diffRouteVisibility so the registry handler can reuse them without duplication.

Admin

  • updateRegistryPlugin / uninstallRegistryPlugin client functions.
  • PluginManager.tsx dispatches the update/uninstall mutations by plugin.source so registry plugins flow through the new endpoints. Uninstall button enabled for any sandboxed source. Band-aid removed.

Folded in from #1112's deferred LOW

  • Install/update/update-check handlers now classify aggregator failures as AGGREGATOR_RESPONSE_INVALID (ClientValidationError) and AGGREGATOR_HTTP_ERROR (ClientResponseError) instead of subsuming both under INSTALL_FAILED.

Backfilled from #1011's deferred tests

  • makeRegistryPluginId: format, determinism, distinctness across publishers + slugs, 10 000-pair collision check.
  • verifyChecksum: hex + multibase + algorithm-mismatch + malformed paths.
  • New lifecycle handler error-path tests.

Closes #1036.

Discussion: #296

Adversarial review

Two-round adversarial review (Claude Opus 4.7 sub-agent). Resolved in this PR:

  • HIGH: /plugins/updates was returning the wrong API envelope — fixed to { data: { items } } to match the rest of the admin API.
  • LOW: structured (non-thrown) registry-check failures were swallowed without telemetry — now logged with their error.code / error.message.

Inherited from marketplace, out of scope (will fix together)

  • The fire-and-forget deleteBundleFromR2(oldVersion) after stateRepo.upsert can, in a concurrent update + downgrade race, delete the now-current bundle. handleMarketplaceUpdate has the same shape — the registry handler faithfully mirrors it. Fixing both together needs either an await plus state-re-read or a sweeper; tracked as follow-up.
  • The update consent dialog architecturally bypasses the server escalation gate: the mutation mutationFn pre-confirms (confirmCapabilityChanges: true), and the dialog renders the already-granted caps with newCapabilities: [] (TODO at PluginManager.tsx:519: "WS3 will populate this from the diff"). The server-side gate is correct and reachable from API callers; the admin UI just never triggers it. Same shape in marketplace today; tracked as follow-up.

Scope-limited

  • handleRegistryUpdate happy-path tests (full DiscoveryClient + fetch mocking) are deferred to a follow-up — only error paths have unit coverage in this PR. The handler's logic is a tight mirror of handleMarketplaceUpdate's well-tested shape.
  • assertSafeArtifactUrl artifact-specific SSRF edges deferred — the underlying SSRF logic is already covered in tests/unit/import/ssrf.test.ts.

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes (touched files clean; pre-existing baseline of 56 unrelated diagnostics unchanged)
  • pnpm test passes (core 3375, admin 879, plugin-cli 264, registry-client 37)
  • pnpm format has been run
  • I have added/updated tests for my changes (5 uninstall + 5 update error-path + 5 plugin-id + 7 checksum = 22 new)
  • User-visible strings in the admin UI are wrapped for translation (Lingui t)
  • I have added a changeset (@emdash-cms/admin + emdash, both minor)
  • New features link to an approved Discussion: Marketplace Discussion #296

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: Claude Opus 4.7 (Claude Code)

Screenshots / test output

core:            Test Files 216 passed (216)   Tests 3375 passed (3375)
admin:           Test Files  62 passed  (62)   Tests  879 passed  (879)
plugin-cli:      Test Files  16 passed  (16)   Tests  264 passed  (264)
registry-client: Test Files   3 passed   (3)   Tests   37 passed   (37)

…cycle

Implements #1036: registry plugins now have a full lifecycle parallel
to marketplace, removing the 'uninstall not yet available' band-aid in
PluginManager.

Server:
- handleRegistryUninstall: deletes the R2 bundle, drops _plugin_state,
  optionally clears _plugin_storage rows. Refuses non-registry sources.
- handleRegistryUpdate: re-runs the install pipeline at a newer
  version, diffing capabilities + public-route visibility against the
  currently installed bundle. CAPABILITY_ESCALATION and
  ROUTE_VISIBILITY_ESCALATION gate widened permissions behind
  confirmCapabilityChanges / confirmRouteVisibilityChanges, mirroring
  marketplace exactly.
- handleRegistryUpdateCheck: scans installed registry plugins, queries
  the aggregator's getLatestRelease for each, returns the version diff.
  Per-plugin aggregator failures don't blank the list.
- POST /_emdash/api/admin/plugins/registry/:id/uninstall
- POST /_emdash/api/admin/plugins/registry/:id/update
- GET  /_emdash/api/admin/plugins/updates is now cross-source: runs
  marketplace + registry checks in parallel, isolates failures, and
  logs structured errors. Wraps the merged items in the standard
  { data: { items } } envelope.
- Marketplace's diffCapabilities + diffRouteVisibility are now
  exported so the registry handler can reuse them without duplication.

Admin:
- updateRegistryPlugin / uninstallRegistryPlugin client functions.
- PluginManager dispatches mutationFn by plugin.source so registry
  plugins flow through the new endpoints; uninstall button enabled
  for any sandboxed source.

Deferred from #1112 (folded in):
- Install handler now classifies aggregator failures as
  AGGREGATOR_RESPONSE_INVALID (ClientValidationError) and
  AGGREGATOR_HTTP_ERROR (ClientResponseError) instead of folding both
  into INSTALL_FAILED. The same classification applies to update and
  update-check.

Deferred from #1011 (backfilled):
- makeRegistryPluginId: format, determinism, distinctness across
  publishers + slugs, and a 10 000-pair collision check.
- verifyChecksum: hex + multibase paths, algorithm-mismatch, malformed.
- Lifecycle handler error paths.

Out of scope (inherited from marketplace, will fix together):
- Concurrent update + downgrade race where a fire-and-forget cleanup
  of the previous version can delete the now-current bundle.
- Update consent dialog architecturally bypasses the server's
  escalation gate (mutationFn pre-confirms; the dialog shows
  already-granted caps with newCapabilities: []). The server gate
  is correct; the client never reaches it. Same shape in marketplace.

Refs #1036
Copilot AI review requested due to automatic review settings May 20, 2026 05:09
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 20, 2026

🦋 Changeset detected

Latest commit: e367f5a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@emdash-cms/admin Minor
emdash Minor
@emdash-cms/cloudflare Minor
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/auth Minor
@emdash-cms/blocks Minor
@emdash-cms/gutenberg-to-portable-text Minor
@emdash-cms/x402 Minor
create-emdash Minor
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 20, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-perf-coordinator e367f5a May 20 2026, 07:45 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 20, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-i18n e367f5a May 20 2026, 07:45 PM

@github-actions
Copy link
Copy Markdown
Contributor

Scope check

This PR changes 1,266 lines across 14 files. Large PRs are harder to review and more likely to be closed without review.

If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs.

See CONTRIBUTING.md for contribution guidelines.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 20, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
docs e367f5a May 20 2026, 07:45 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 20, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-cache e367f5a May 20 2026, 07:47 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 20, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-playground e367f5a May 20 2026, 07:46 PM

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 20, 2026

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@1114

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@1114

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@1114

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@1114

emdash

npm i https://pkg.pr.new/emdash@1114

create-emdash

npm i https://pkg.pr.new/create-emdash@1114

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@1114

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@1114

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@1114

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@1114

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@1114

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@1114

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@1114

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@1114

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@1114

commit: e367f5a

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds full lifecycle support for experimental registry-sourced plugins (uninstall/update/update-check) and wires those flows through the admin UI, while aligning update-check behavior across plugin sources.

Changes:

  • Adds registry uninstall/update endpoints + core handler implementations (including capability/route-visibility escalation gates) and exports shared diff helpers from marketplace.
  • Extends /admin/plugins/updates to merge marketplace + registry update checks without one source failure blanking the other.
  • Backfills unit tests for registry plugin ID generation, checksum verification, and registry lifecycle handler error paths.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pnpm-lock.yaml Locks new dependency versions (incl. @atcute/client).
packages/core/package.json Adds @atcute/client dependency needed for typed aggregator error handling.
packages/core/src/api/handlers/marketplace.ts Exports diffCapabilities / diffRouteVisibility for reuse by registry update handler.
packages/core/src/api/handlers/registry.ts Adds uninstall/update/update-check handlers; exports verifyChecksum / assertSafeArtifactUrl; classifies aggregator errors with dedicated codes.
packages/core/src/api/handlers/index.ts Re-exports new registry lifecycle handlers and types.
packages/core/src/astro/routes/api/admin/plugins/updates.ts Makes update-check endpoint cross-source and merges results into { data: { items } }.
packages/core/src/astro/routes/api/admin/plugins/registry/[id]/update.ts Adds registry update route wrapper (body parsing + auth + sync).
packages/core/src/astro/routes/api/admin/plugins/registry/[id]/uninstall.ts Adds registry uninstall route wrapper (body parsing + auth + sync).
packages/core/tests/unit/registry/plugin-id.test.ts Adds tests for makeRegistryPluginId determinism/format/collision resistance.
packages/core/tests/unit/registry/checksum.test.ts Adds tests for verifyChecksum (hex + multibase-multihash).
packages/core/tests/unit/api/registry-handlers.test.ts Adds handler tests (currently uninstall + update error paths).
packages/admin/src/lib/api/registry.ts Adds admin client functions for registry update/uninstall.
packages/admin/src/components/PluginManager.tsx Wires update/uninstall mutations by plugin.source; enables uninstall for registry plugins.
.changeset/registry-lifecycle.md Declares minor releases for admin + emdash and documents feature surface.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 236 to +243
const updateMutation = useMutation({
mutationFn: () => updateMarketplacePlugin(plugin.id, { confirmCapabilities: true }),
mutationFn: () =>
isRegistry
? updateRegistryPlugin(plugin.id, {
confirmCapabilityChanges: true,
confirmRouteVisibilityChanges: true,
})
: updateMarketplacePlugin(plugin.id, { confirmCapabilities: true }),
Comment on lines +1159 to +1164
try {
await db.deleteFrom("_plugin_storage").where("plugin_id", "=", pluginId).execute();
dataDeleted = true;
} catch {
// No plugin_storage rows for this plugin; nothing to delete.
}
* `marketplace-handlers.test.ts`):
* - Uninstall (handleRegistryUninstall)
* - Update (handleRegistryUpdate)
* - Update check (handleRegistryUpdateCheck)
@ascorbic
Copy link
Copy Markdown
Collaborator Author

/review

uninstallMarketplacePlugin,
type PluginUpdateInfo,
} from "../lib/api/marketplace.js";
import { uninstallRegistryPlugin, updateRegistryPlugin } from "../lib/api/registry.js";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: "Check for updates" button is gated on hasMarketplacePlugins only

Category: Logic Errors
Severity: MEDIUM

Referencing unchanged code at line 122 (hasMarketplacePlugins = plugins?.some((p) => p.source === "marketplace")) and line 147 ({hasMarketplacePlugins && <Button ... onClick={refetchUpdates}>{t\Check for updates`}}`).

This PR teaches /plugins/updates to return registry plugins, but the UI button that triggers it is still rendered only when at least one marketplace plugin is installed. A site whose only plugins are registry-source -- exactly the scenario the registry rollout targets -- has no way to trigger the update check from the Plugins page.

Trigger: install a registry plugin, no marketplace plugins. Open Plugins page. "Check for updates" is never shown, so registry update availability never gets surfaced.

Fix: update the gate alongside this PR -- plugins?.some((p) => p.source === "marketplace" || p.source === "registry"), or rename the derived flag to hasUpdatableSources.

dataDeleted = true;
} catch {
// No plugin_storage rows for this plugin; nothing to delete.
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: bare catch silently turns a real DB error into a successful "didn't delete" response

Category: Error Handling / Data Integrity
Severity: MEDIUM

The inline comment claims "No plugin_storage rows for this plugin; nothing to delete" -- but a DELETE ... WHERE plugin_id = ? with zero matches does NOT throw in SQLite or Postgres. It just affects 0 rows. So this catch will only ever fire on a real error (missing table, schema drift, permission issue, dialect bug, transaction conflict). When it does fire, dataDeleted stays false, the next line still calls stateRepo.delete(pluginId), and the handler returns { success: true, data: { pluginId, dataDeleted: false } }.

The caller explicitly asked for deleteData: true. The server reports success but lies about whether the data was deleted, and leaves orphaned _plugin_storage rows tied to a now-deleted state row -- exactly the contract violation deleteData exists to avoid. No telemetry either; the error vanishes.

Trigger: any DB-level failure on the _plugin_storage delete (e.g., _plugin_storage table missing in an old deployment, deadlock on a busy site, dialect error). The admin clicks "Uninstall + delete data" and gets a toast saying success, while the plugin's stored data persists.

Fix: remove the inner try/catch entirely (let real errors propagate to the outer catch and surface as UNINSTALL_FAILED), or narrow it to a known benign condition via utils/db-errors.isMissingTableError() only. Bare catch {} is wrong here.

},
};
}
if (err instanceof SsrfError) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: dead instanceof SsrfError branch

Category: Logic Errors
Severity: LOW

assertSafeArtifactUrl (lines 370-377) catches SsrfError and re-throws it as a plain new Error("Artifact URL rejected: ..."). So no caller of assertSafeArtifactUrl ever sees an SsrfError -- including this update handler, which calls it directly at lines 1378 and 1382.

Result: SSRF rejections from those direct calls fall through to the UPDATE_FAILED branch below instead of getting the intended UNSAFE_ARTIFACT_URL code, and the wrapped "Artifact URL rejected: ..." message gets echoed to the client. The catch reads like SSRF gets a structured code; it doesn't.

Trigger: aggregator returns a release whose artifacts.package.url points at a private IP / localhost / a hostname that resolves to one. The update fails with code: "UPDATE_FAILED", message: "Artifact URL rejected: ..." instead of code: "UNSAFE_ARTIFACT_URL".

Fix: either change assertSafeArtifactUrl to throw SsrfError directly (and update install's call sites to handle it), or drop this dead branch and accept that SSRF-shaped failures will get the catch-all code.

* `marketplace-handlers.test.ts`):
* - Uninstall (handleRegistryUninstall)
* - Update (handleRegistryUpdate)
* - Update check (handleRegistryUpdateCheck)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: file header advertises coverage that doesn't exist

Category: Misleading code
Severity: LOW

The docstring lists "Update check (handleRegistryUpdateCheck)" as a covered surface, but neither the import block nor any describe block touches handleRegistryUpdateCheck. The PR description itself calls out update-check happy-path coverage as scope-limited; the file comment should agree.

Fix: drop the update-check bullet from the header, or add a stub describe.skip("handleRegistryUpdateCheck", ...) to make the gap explicit.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 20, 2026

Review posted successfully (id 4325391940). Posted 4 inline comments:

  • MEDIUM (PluginManager.tsx): "Check for updates" button stays gated on hasMarketplacePlugins — registry-only sites can't trigger the new merged update check.
  • MEDIUM (registry.ts:1164): Bare catch on _plugin_storage delete swallows real DB errors but still reports success: true, dataDeleted: false. Caller asked to delete data, server silently lies.
  • LOW (registry.ts:1505): Dead instanceof SsrfError branch — assertSafeArtifactUrl always rewraps as plain Error, so SSRF rejections actually fall through to UPDATE_FAILED instead of UNSAFE_ARTIFACT_URL.
  • LOW (registry-handlers.test.ts:8): File header advertises handleRegistryUpdateCheck coverage that doesn't exist.

github run

- PluginManager: extend the 'Check for updates' button gate from
  hasMarketplacePlugins to hasUpdatableSources so registry-only sites
  can actually trigger the merged update check.
- handleRegistryUninstall: drop the bare try/catch around the
  _plugin_storage delete. A DELETE with zero matches doesn't throw,
  so the bare catch only ever masked real DB errors while still
  reporting dataDeleted: false. Real errors now propagate to the
  outer catch and surface as UNINSTALL_FAILED.
- handleRegistryUpdate: remove the dead 'err instanceof SsrfError'
  branch from the catch (assertSafeArtifactUrl wraps SsrfError in a
  plain Error before rethrowing, so the branch was unreachable).
- registry-handlers test header: drop the update-check coverage claim
  to match the actual test surface.

Reworks the update-consent flow to actually consult the server's
escalation gate instead of pre-confirming:

- apiError / unwrapResult now plumb error.details through to the
  response body. The CAPABILITY_ESCALATION and ROUTE_VISIBILITY_
  ESCALATION responses now carry their diff to the client.
- New RegistryUpdateEscalationError carries the diff. updateRegistryPlugin
  parses CAPABILITY_ESCALATION / ROUTE_VISIBILITY_ESCALATION 403s and
  throws it instead of a generic Error.
- PluginManager preflights the registry update without confirm flags;
  on escalation the consent dialog opens populated with the real
  capabilityChanges.added and newlyPublicRoutes; the user's confirm
  re-calls with confirmCapabilityChanges + confirmRouteVisibilityChanges
  set. Iterative escalations (capability then route) re-open the
  dialog with the new diff.
- CapabilityConsentDialog: new newlyPublicRoutes prop renders the
  public-route diff alongside the capability diff.

Marketplace's update path is unchanged in this PR (it still
pre-confirms; same WS3 TODO it has always had). Registry no longer
inherits that bypass.
@ascorbic
Copy link
Copy Markdown
Collaborator Author

/review again

if (isRegistry) {
updateMutation.mutate({
confirmCapabilityChanges: true,
confirmRouteVisibilityChanges: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: dialog consents to route-visibility escalation the user was never shown

Category: Security / UX
Severity: HIGH

handleUpdateConfirm sets confirmCapabilityChanges: true and confirmRouteVisibilityChanges: true in the same call, regardless of which escalation the user actually saw. Combined with the server's gate ordering, this means a plugin update with both new capabilities and new public routes silently slips the public-route diff past the user.

Walk the flow for a plugin where v2 adds a new capability and a new public route:

  1. handleUpdateClick -> mutate({}). No confirm flags.
  2. handleRegistryUpdate (registry.ts:1424-1435) checks capability escalation first. It returns CAPABILITY_ESCALATION with details: { capabilityChanges } -- only the capability diff, because the early return at 1434 happens before diffRouteVisibility is even called at 1437.
  3. Client's parseEscalation extracts capabilityChanges, routeVisibilityChanges is undefined.
  4. Dialog renders with newCapabilities = capabilityChanges.added and newlyPublicRoutes = []. User sees only the capability change.
  5. User clicks Confirm -> handleUpdateConfirm -> mutate({ confirmCapabilityChanges: true, confirmRouteVisibilityChanges: true }).
  6. Server: capability check now passes (confirmed); route check at 1439 also passes (also confirmed). Update proceeds. The user has consented to a new public, unauthenticated route they never saw.

The commit message claims iterative escalation re-opens the dialog with the new diff, but iteration never actually happens because the second confirm flag is pre-set on the first user confirmation. This is the same architectural bypass the PR description called out as inherited-from-marketplace and out of scope -- registry was supposed to no longer inherit it, and the body of the fix-commit message claims as much. It still does.

Trigger: any registry update where the new manifest both declares a new capability and flips a route to public: true. The route-exposure diff is what ROUTE_VISIBILITY_ESCALATION exists to surface; this code path silently auto-confirms it.

Fix: either (a) handleUpdateConfirm sets only the flag for the escalation the dialog actually displayed -- e.g. confirmCapabilityChanges: true when registryEscalation?.code === 'CAPABILITY_ESCALATION' -- and relies on the next server roundtrip to surface the route diff, letting onError re-open the dialog (this matches the "iterative escalations re-open the dialog" promise in the commit message); or (b) the server's CAPABILITY_ESCALATION branch at registry.ts:1432 eagerly computes diffRouteVisibility and includes it in details so the first dialog can show both at once. Option (a) is the smaller change and is what the existing onError plumbing already supports.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 20, 2026

Review posted (id 4331561511) with 1 inline comment.

Summary:

  • HIGH (PluginManager.tsx:286): handleUpdateConfirm always sets both confirmCapabilityChanges: true and confirmRouteVisibilityChanges: true on a single user confirmation. Because the server's gate at registry.ts:1432 returns CAPABILITY_ESCALATION with only the capability diff (route diff isn't computed until after that early-return), a plugin that adds both a new capability and a new public route shows the user only the capability diff, then silently auto-confirms the unseen route exposure. The commit message promises iterative escalation re-opens the dialog with the new diff; the wiring doesn't deliver it.

The four prior-round findings (hasMarketplacePlugins gate, bare catch on _plugin_storage delete, dead SsrfError branch, misleading test-file header) were all addressed in commit 7d594a6.

github run

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)

packages/core/src/api/handlers/registry.ts:1500

  • AGGREGATOR_RESPONSE_INVALID / AGGREGATOR_HTTP_ERROR returned here will inherit unwrapResult()’s default status mapping (400 for non-*_ERROR, 500 for *_ERROR) unless mapErrorStatus() is updated. If these are meant to represent upstream/aggregator failures, consider mapping them explicitly (commonly 502/503) so clients can distinguish configuration/user errors from aggregator outages.
		if (err instanceof ClientValidationError) {
			return {
				success: false,
				error: {
					code: "AGGREGATOR_RESPONSE_INVALID",
					message: `Aggregator returned a response that does not conform to its lexicon (${err.target})`,
				},
			};
		}
		if (err instanceof ClientResponseError) {
			return {
				success: false,
				error: {
					code: "AGGREGATOR_HTTP_ERROR",
					message: `Aggregator returned ${err.status}: ${err.error}`,
				},
			};
		}

packages/core/src/api/handlers/registry.ts:1620

  • Same as other registry handlers: AGGREGATOR_RESPONSE_INVALID currently maps to 400 via mapErrorStatus() default behavior, which can make aggregator failures look like a client request error. Consider adding explicit status mappings for these aggregator-specific codes so update-check failures surface with an appropriate 5xx (e.g. 502).
		if (err instanceof ClientValidationError) {
			return {
				success: false,
				error: {
					code: "AGGREGATOR_RESPONSE_INVALID",
					message: `Aggregator returned a response that does not conform to its lexicon (${err.target})`,
				},
			};
		}
		if (err instanceof ClientResponseError) {
			return {
				success: false,
				error: {
					code: "AGGREGATOR_HTTP_ERROR",
					message: `Aggregator returned ${err.status}: ${err.error}`,
				},
			};
		}

Comment on lines +1159 to +1160
await db.deleteFrom("_plugin_storage").where("plugin_id", "=", pluginId).execute();
dataDeleted = true;
Comment on lines +1072 to +1088
if (err instanceof ClientValidationError) {
return {
success: false,
error: {
code: "AGGREGATOR_RESPONSE_INVALID",
message: `Aggregator returned a response that does not conform to its lexicon (${err.target})`,
},
};
}
if (err instanceof ClientResponseError) {
return {
success: false,
error: {
code: "AGGREGATOR_HTTP_ERROR",
message: `Aggregator returned ${err.status}: ${err.error}`,
},
};
Comment on lines +7 to +8
* regardless of source mix; consumers tell sources apart by pluginId
* (registry ids are `r_*` per `REGISTRY_PLUGIN_ID_PATTERN`).
ascorbic added 2 commits May 20, 2026 20:35
- HIGH (consent): handleUpdateConfirm now only sets the confirm flag
  that matches the escalation the dialog actually displayed. Sending
  both unconditionally auto-confirmed route-visibility changes the
  user was never shown when capability escalation came first. The
  iterative re-open promised in the previous commit now actually
  happens: cap confirm → server returns ROUTE_VISIBILITY_ESCALATION →
  onError repopulates the dialog with the route diff → user confirms
  the new view → second roundtrip sends both flags.
- handleRegistryUninstall: restore a narrow try/catch around the
  _plugin_storage cleanup, logging the error and continuing with the
  state-row delete. Without it, a transient DB failure during the
  optional cleanup orphaned the state row pointing at an already-
  deleted bundle. Honest dataDeleted=false plus telemetry instead of
  swallowed silence.
- errors.ts: AGGREGATOR_RESPONSE_INVALID and AGGREGATOR_HTTP_ERROR now
  map to 502 Bad Gateway (added to ErrorCode and to mapErrorStatus).
- updates.ts: reword the header — the pluginId prefix is not a
  reliable source discriminator; consumers correlate via the plugin
  list's  field.
MED — uninstall ordering trades retriability for orphans (regression
introduced last commit). Reordered the uninstall steps so the most
failure-prone work runs first: _plugin_storage cleanup → bundle
delete → state row delete. A transient DB error during the optional
storage cleanup now cascades to the outer catch with state row and
bundle intact, so the admin can retry safely (bundle delete is
idempotent on misses). Replaces the previous swallow-and-continue,
which orphaned _plugin_storage rows forever when the state row got
deleted underneath them.

LOW — split AGGREGATOR_HTTP_ERROR's 502-everywhere mapping. A user
typo (publisher/slug doesn't exist) was surfacing as a 502 with a
"upstream broken" semantic, firing on the operator's 5xx alerting.
The three handler catches now map err.status === 404 to a new
AGGREGATOR_NOT_FOUND code (404), leaving AGGREGATOR_HTTP_ERROR (502)
for the actually-upstream-broken case.
@ascorbic ascorbic merged commit 9a30607 into main May 20, 2026
37 checks passed
@ascorbic ascorbic deleted the feat/registry-lifecycle branch May 20, 2026 20:04
@emdashbot emdashbot Bot mentioned this pull request May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Registry: uninstall, update, update-check handlers + admin wiring

2 participants