Skip to content

Commit e590f86

Browse files
committed
fix(registry): address review findings + CI failures
CI fixes: - Rename normalizeCapabilities -> canonicalCapabilitiesForDriftCheck to avoid namespace clash with the existing capability normalizer exported from @emdash-cms/plugin-types via core's index. The old name shadowed plugin-types' helper at the top level of core's dist, which made the definePlugin() overload set look ambiguous to TS in plugins/forms and caused a typecheck cascade there. - [...seen].toSorted() instead of [...seen].sort() to clear the e18e/prefer-spread-syntax + unicorn/no-array-sort lint errors. Review findings (ask-bonk[bot]): - HIGH: drift check tripped on every install when the release record's extension was empty. The browser now omits acknowledgedDeclaredAccess when capabilities is empty, opting out of the server-side drift gate for the (currently common) case where publishers haven't filled in the extension block. The bundle's real capabilities are still bound to the checksum-verified bytes. - HIGH: DID-only publishers (no resolvable handle) could be linked from the browse grid but never installed because the server rejects handles without a '.'. Cards now render as non-interactive with a 'Publisher handle unresolved' badge; the detail page surfaces a matching warning and disables Install. - MEDIUM: registry-enabled sites were unconditionally routing existing marketplace plugin detail URLs to RegistryPluginDetail, breaking deep links. Detail-route selection now discriminates by param shape (pluginId.includes('/')) rather than the manifest flag. - MEDIUM: state-row write failure after storeBundleInR2 left orphan bundles. Best-effort cleanup in the catch via deleteBundleFromR2. - LOW: parseDurationSeconds runs on the user-supplied integration option per install (not the already-normalized manifest shape). Wrap in try/catch and surface as REGISTRY_POLICY_INVALID rather than letting it bubble to a generic INSTALL_FAILED. - LOW: validator-pattern doc drift in plugin-id.ts (already fixed in the prior commit).
1 parent ba6c25b commit e590f86

6 files changed

Lines changed: 167 additions & 60 deletions

File tree

packages/admin/src/components/RegistryBrowse.tsx

Lines changed: 53 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -154,43 +154,69 @@ interface RegistryPackageCardProps {
154154

155155
function RegistryPackageCard({ pkg, installed }: RegistryPackageCardProps) {
156156
const { t } = useLingui();
157-
const handle = pkg.handle ?? pkg.did;
157+
// Only domain-like handles are installable; the server's handle
158+
// validator rejects DID strings. Cards for packages whose handle the
159+
// aggregator couldn't resolve render the same content but without a
160+
// link, so the user understands the package isn't actionable.
161+
const installable = Boolean(pkg.handle && pkg.handle.includes("."));
162+
const handleDisplay = pkg.handle ?? pkg.did;
158163
// `profile` is a pass-through of the signed package profile record.
159164
// We duck-type minimal display fields out of it.
160165
const profile = pkg.profile as { name?: string; description?: string };
161166
const verified = (pkg.labels ?? []).some((l: { val?: string }) => l.val === "verified");
162167

168+
const inner = (
169+
<div className="flex items-start gap-3">
170+
<div className="mt-1 rounded-md bg-kumo-subtle p-2 text-kumo-subtle">
171+
<PuzzlePiece className="h-5 w-5" />
172+
</div>
173+
<div className="min-w-0 flex-1">
174+
<div className="flex items-center gap-2">
175+
<h2 className="truncate font-semibold">{profile.name ?? pkg.slug}</h2>
176+
{verified ? (
177+
<ShieldCheck
178+
className="h-4 w-4 shrink-0 text-kumo-brand"
179+
aria-label={t`Verified publisher`}
180+
/>
181+
) : null}
182+
</div>
183+
<p className="truncate text-xs text-kumo-subtle">{handleDisplay}</p>
184+
{profile.description ? (
185+
<p className="mt-2 line-clamp-2 text-sm text-kumo-default">{profile.description}</p>
186+
) : null}
187+
{installed ? (
188+
<div className="mt-3">
189+
<Badge variant="success">{t`Installed`}</Badge>
190+
</div>
191+
) : null}
192+
{!installable ? (
193+
<div className="mt-3">
194+
<Badge variant="outline">{t`Publisher handle unresolved`}</Badge>
195+
</div>
196+
) : null}
197+
</div>
198+
</div>
199+
);
200+
201+
if (!installable) {
202+
return (
203+
<div
204+
className="block rounded-md border border-kumo-border bg-kumo-surface p-4 opacity-60"
205+
aria-disabled="true"
206+
>
207+
{inner}
208+
</div>
209+
);
210+
}
211+
163212
return (
164213
<Link
165214
to="/plugins/marketplace/$pluginId"
166-
params={{ pluginId: `${pkg.handle ?? pkg.did}/${pkg.slug}` }}
215+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- installable check above ensures pkg.handle is defined
216+
params={{ pluginId: `${pkg.handle!}/${pkg.slug}` }}
167217
className="block rounded-md border border-kumo-border bg-kumo-surface p-4 transition-colors hover:bg-kumo-subtle focus:outline-none focus-visible:ring-2 focus-visible:ring-kumo-brand"
168218
>
169-
<div className="flex items-start gap-3">
170-
<div className="mt-1 rounded-md bg-kumo-subtle p-2 text-kumo-subtle">
171-
<PuzzlePiece className="h-5 w-5" />
172-
</div>
173-
<div className="min-w-0 flex-1">
174-
<div className="flex items-center gap-2">
175-
<h2 className="truncate font-semibold">{profile.name ?? pkg.slug}</h2>
176-
{verified ? (
177-
<ShieldCheck
178-
className="h-4 w-4 shrink-0 text-kumo-brand"
179-
aria-label={t`Verified publisher`}
180-
/>
181-
) : null}
182-
</div>
183-
<p className="truncate text-xs text-kumo-subtle">{handle}</p>
184-
{profile.description ? (
185-
<p className="mt-2 line-clamp-2 text-sm text-kumo-default">{profile.description}</p>
186-
) : null}
187-
{installed ? (
188-
<div className="mt-3">
189-
<Badge variant="success">{t`Installed`}</Badge>
190-
</div>
191-
) : null}
192-
</div>
193-
</div>
219+
{inner}
194220
</Link>
195221
);
196222
}

packages/admin/src/components/RegistryPluginDetail.tsx

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ import { Link } from "@tanstack/react-router";
2121
import * as React from "react";
2222

2323
import {
24+
canonicalCapabilitiesForDriftCheck,
2425
getLatestRegistryRelease,
2526
installRegistryPlugin,
26-
normalizeCapabilities,
2727
releasePassesPolicy,
2828
resolveRegistryPackage,
2929
type RegistryClientConfig,
@@ -84,22 +84,39 @@ export function RegistryPluginDetail({ pluginId, config }: RegistryPluginDetailP
8484
)?.[1];
8585

8686
const capabilities: string[] = Array.isArray(ext?.capabilities)
87-
? normalizeCapabilities(ext?.capabilities)
88-
: normalizeCapabilities(declaredAccessToCapabilityList(ext?.declaredAccess));
87+
? canonicalCapabilitiesForDriftCheck(ext?.capabilities)
88+
: canonicalCapabilitiesForDriftCheck(declaredAccessToCapabilityList(ext?.declaredAccess));
8989

9090
const profile = pkg?.profile as { name?: string; description?: string } | undefined;
9191
const verified = (pkg?.labels ?? []).some((l: { val?: string }) => l.val === "verified");
9292

9393
const policyOk =
9494
release && pkg ? releasePassesPolicy(release, { did: pkg.did, slug }, config.policy) : true;
95+
// Install requires a resolvable handle: the server validates handles
96+
// with at least one `.`, which DID strings (`did:plc:abc`) don't
97+
// satisfy. Publishers whose handle the aggregator couldn't resolve
98+
// (or who haven't claimed one yet) can't be installed today. Surface
99+
// the limitation in the UI rather than letting the user click into
100+
// `INVALID_HANDLE` from the server.
101+
const hasResolvableHandle = Boolean(pkg?.handle && pkg.handle.includes("."));
95102

96103
const installMutation = useMutation({
97104
mutationFn: () =>
98105
installRegistryPlugin({
99106
handle,
100107
slug,
101108
version: release?.version,
102-
acknowledgedDeclaredAccess: capabilities,
109+
// Only send the acknowledgement when the dialog had real
110+
// capability data to display. The server's drift check is
111+
// gated on `acknowledgedDeclaredAccess !== undefined`, so
112+
// omitting the field opts out of the check entirely --
113+
// correct behaviour for the (currently common) case where
114+
// the publisher's release record doesn't yet carry an
115+
// extension block. The bundle's actual capabilities are
116+
// still bound to the checksum-verified bytes; the drift
117+
// check is a UX sanity belt for already-displayed
118+
// consent, not an authorization gate.
119+
acknowledgedDeclaredAccess: capabilities.length > 0 ? capabilities : undefined,
103120
}),
104121
onSuccess: () => {
105122
setShowConsent(false);
@@ -173,14 +190,30 @@ export function RegistryPluginDetail({ pluginId, config }: RegistryPluginDetailP
173190
<div>
174191
<Button
175192
variant="primary"
176-
disabled={!release || !policyOk}
193+
disabled={!release || !policyOk || !hasResolvableHandle}
177194
onClick={() => setShowConsent(true)}
178195
>
179196
{t`Install`}
180197
</Button>
181198
</div>
182199
</div>
183200

201+
{/* Unresolvable handle notice */}
202+
{pkg && !hasResolvableHandle ? (
203+
<div
204+
className="flex items-start gap-3 rounded-md border border-kumo-warning bg-kumo-warning/10 p-4 text-kumo-warning"
205+
role="status"
206+
>
207+
<Warning className="mt-0.5 h-5 w-5 shrink-0" />
208+
<div>
209+
<p className="font-medium">{t`Publisher handle is not resolvable`}</p>
210+
<p className="mt-1 text-sm text-kumo-default">
211+
{t`This package's publisher hasn't claimed a handle the aggregator can resolve, so it can't be installed yet. The publisher needs to set up a handle (any domain they control) before this plugin is installable.`}
212+
</p>
213+
</div>
214+
</div>
215+
) : null}
216+
184217
{/* Policy holdback notice */}
185218
{release && !policyOk ? (
186219
<div

packages/admin/src/lib/api/registry.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,23 +208,23 @@ export function releasePassesPolicy(
208208
}
209209

210210
/**
211-
* Normalize a capabilities list for set-style comparison. Mirrors the
212-
* server-side helper of the same name in
211+
* Canonicalize a capabilities list for set-style comparison. Mirrors
212+
* the server-side helper `canonicalCapabilitiesForDriftCheck` in
213213
* `packages/core/src/registry/config.ts` -- both sides must produce
214214
* the same canonical shape so the install handler's drift check is
215215
* stable across reorderings, duplicates, and junk entries.
216216
*
217217
* Filters non-strings, deduplicates, and sorts lexically.
218218
*/
219-
export function normalizeCapabilities(value: unknown): string[] {
219+
export function canonicalCapabilitiesForDriftCheck(value: unknown): string[] {
220220
if (!Array.isArray(value)) return [];
221221
const seen = new Set<string>();
222222
for (const entry of value) {
223223
if (typeof entry === "string" && entry.length > 0) {
224224
seen.add(entry);
225225
}
226226
}
227-
return Array.from(seen).sort();
227+
return [...seen].toSorted();
228228
}
229229

230230
/**

packages/admin/src/router.tsx

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1336,10 +1336,14 @@ function MarketplaceDetailPage() {
13361336
return new Set(plugins.map((p) => p.id));
13371337
}, [plugins]);
13381338

1339-
// Registry detail when configured. The `pluginId` route param carries
1340-
// `${handle}/${slug}` in the registry case; the slash is encoded once
1341-
// by the router and decoded back here.
1342-
if (manifest?.registry) {
1339+
// Discriminate by param shape, not by the manifest flag. A registry
1340+
// pluginId is always `${handle}/${slug}` and contains exactly one `/`;
1341+
// a marketplace pluginId is a single segment with no `/`. This keeps
1342+
// deep links to marketplace-installed plugins working on sites that
1343+
// later opt into the registry, instead of unconditionally routing
1344+
// every visit to RegistryPluginDetail.
1345+
const looksLikeRegistryId = pluginId.includes("/");
1346+
if (manifest?.registry && looksLikeRegistryId) {
13431347
return <RegistryPluginDetail pluginId={pluginId} config={manifest.registry} />;
13441348
}
13451349

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

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ import type { PluginBundle } from "../../plugins/marketplace.js";
4848
import type { SandboxRunner } from "../../plugins/sandbox/types.js";
4949
import { PluginStateRepository } from "../../plugins/state.js";
5050
import {
51-
normalizeCapabilities,
51+
canonicalCapabilitiesForDriftCheck,
5252
parseDurationSeconds,
5353
releaseExemptFromMinimumAge,
5454
validateAggregatorUrl,
@@ -57,7 +57,7 @@ import { makeRegistryPluginId } from "../../registry/plugin-id.js";
5757
import { EmDashStorageError } from "../../storage/types.js";
5858
import type { Storage } from "../../storage/types.js";
5959
import type { ApiResult } from "../types.js";
60-
import { storeBundleInR2 } from "./marketplace.js";
60+
import { deleteBundleFromR2, storeBundleInR2 } from "./marketplace.js";
6161

6262
// ── Types ──────────────────────────────────────────────────────────
6363

@@ -690,9 +690,31 @@ export async function handleRegistryInstall(
690690
// signed createdAt from the publisher's PDS (deferred to the
691691
// follow-up that adds full MST verification). If the timestamp
692692
// is missing or malformed, we fail closed and reject the install.
693+
// `registryConfig` is the user-supplied integration option, not
694+
// the normalized manifest shape, so the duration parse runs once
695+
// per install. Catch a malformed value here -- normally caught at
696+
// `normalizeRegistryConfig` time, but a future config-mutation
697+
// path could re-enter with a bad value -- and surface it as a
698+
// structured error rather than letting it bubble out as a generic
699+
// 500.
693700
const minimumReleaseAge = registryConfig.policy?.minimumReleaseAge;
694-
const minimumReleaseAgeSeconds =
695-
minimumReleaseAge !== undefined ? parseDurationSeconds(minimumReleaseAge) : 0;
701+
let minimumReleaseAgeSeconds = 0;
702+
if (minimumReleaseAge !== undefined) {
703+
try {
704+
minimumReleaseAgeSeconds = parseDurationSeconds(minimumReleaseAge);
705+
} catch (err) {
706+
return {
707+
success: false,
708+
error: {
709+
code: "REGISTRY_POLICY_INVALID",
710+
message:
711+
err instanceof Error
712+
? err.message
713+
: "Invalid minimumReleaseAge value in registry config",
714+
},
715+
};
716+
}
717+
}
696718
if (minimumReleaseAgeSeconds > 0) {
697719
const exclude = registryConfig.policy?.minimumReleaseAgeExclude?.map((e) =>
698720
e.trim().toLowerCase(),
@@ -876,8 +898,8 @@ export async function handleRegistryInstall(
876898
// the runtime starts enforcing `declaredAccess` natively, this
877899
// comparison switches to that shape.
878900
if (input.acknowledgedDeclaredAccess !== undefined) {
879-
const acknowledged = normalizeCapabilities(input.acknowledgedDeclaredAccess);
880-
const actual = normalizeCapabilities(bundle.manifest.capabilities);
901+
const acknowledged = canonicalCapabilitiesForDriftCheck(input.acknowledgedDeclaredAccess);
902+
const actual = canonicalCapabilitiesForDriftCheck(bundle.manifest.capabilities);
881903
if (
882904
acknowledged.length !== actual.length ||
883905
acknowledged.some((cap, i) => cap !== actual[i])
@@ -901,14 +923,32 @@ export async function handleRegistryInstall(
901923
// (the signed record from the publisher's repo), not from the
902924
// bundle manifest -- the manifest carries the trust contract,
903925
// the profile carries the marketing copy.
926+
//
927+
// If the state-row write fails (DB error, PK race against a
928+
// concurrent install of the same package), clean up the R2 bundle
929+
// we just wrote so we don't leave orphans. The cleanup is
930+
// best-effort; if it also fails, the row failure still surfaces
931+
// to the caller.
904932
const profile = packageView.profile as { name?: string; description?: string };
905-
await stateRepo.upsert(pluginId, version, "active", {
906-
source: "registry",
907-
displayName: profile.name ?? slug,
908-
description: profile.description ?? undefined,
909-
registryPublisherDid: publisherDid,
910-
registrySlug: slug,
911-
});
933+
try {
934+
await stateRepo.upsert(pluginId, version, "active", {
935+
source: "registry",
936+
displayName: profile.name ?? slug,
937+
description: profile.description ?? undefined,
938+
registryPublisherDid: publisherDid,
939+
registrySlug: slug,
940+
});
941+
} catch (stateErr) {
942+
try {
943+
await deleteBundleFromR2(storage, pluginId, version, "registry");
944+
} catch (cleanupErr) {
945+
console.warn(
946+
`[registry-install] Failed to clean up R2 bundle for ${pluginId}@${version} after state-row write failure:`,
947+
cleanupErr,
948+
);
949+
}
950+
throw stateErr;
951+
}
912952

913953
return {
914954
success: true,

packages/core/src/registry/config.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,30 +38,34 @@ export interface ManifestRegistryConfig {
3838
}
3939

4040
/**
41-
* Normalize a capabilities list for set-style comparison.
41+
* Canonicalize a capabilities list for set-style comparison.
4242
*
4343
* Capabilities (the legacy declared-access shape used by the current
4444
* sandbox enforcer) are conceptually a *set*: order, duplicates, and
4545
* non-string entries don't carry meaning. The install handler's drift
4646
* check compares the admin's acknowledged set against the bundle
47-
* manifest's set; both sides pass through this normalizer first so
47+
* manifest's set; both sides pass through this canonicalizer first so
4848
* an aggregator-supplied array with unstable order or junk entries
4949
* can't cause a spurious drift rejection.
5050
*
51-
* Filters non-strings, deduplicates, and sorts lexically. Exported so
52-
* the same shape is produced by the browser before sending the
53-
* `acknowledgedDeclaredAccess` payload and by the server before
51+
* Filters non-strings, deduplicates, and sorts lexically. Named to
52+
* avoid shadowing `@emdash-cms/plugin-types`'s existing
53+
* `normalizeCapabilities` (which dedupes + applies the deprecated →
54+
* current alias map but does not filter junk or sort).
55+
*
56+
* Exported so the same shape is produced by the browser before sending
57+
* the `acknowledgedDeclaredAccess` payload and by the server before
5458
* comparing against the bundle.
5559
*/
56-
export function normalizeCapabilities(value: unknown): string[] {
60+
export function canonicalCapabilitiesForDriftCheck(value: unknown): string[] {
5761
if (!Array.isArray(value)) return [];
5862
const seen = new Set<string>();
5963
for (const entry of value) {
6064
if (typeof entry === "string" && entry.length > 0) {
6165
seen.add(entry);
6266
}
6367
}
64-
return Array.from(seen).sort();
68+
return [...seen].toSorted();
6569
}
6670

6771
/**

0 commit comments

Comments
 (0)