feat(plugins): example plugin publishing & route convention enforcement#164
Merged
seanhanca merged 6 commits intoMar 4, 2026
Merged
Conversation
Contributor
|
@seanhanca is attempting to deploy a commit to the Livepeer Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds example-plugin publishing (frontend UI, new registry endpoints, auto-install), enforces plugin route namespace and reserved-path validation, normalizes discovered plugin routes and originalRoutes, updates registry sync/cleanup, adds a dashboard catch-all plugin loader, and supporting package/export and script changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Plugin Publisher UI
participant API as Registry API
participant DB as Database
participant Storage as CDN/Build Storage
User->>UI: Click "Publish Example"
UI->>API: POST /registry/examples/:name/publish
API->>Storage: Verify bundle/artifacts exist
Storage-->>API: Bundle exists
API->>DB: Begin transaction (upsert package, version, workflow, deployment)
DB-->>API: Transaction committed
API->>DB: Upsert tenant/user install (auto-install, non-fatal)
DB-->>API: Install recorded
API-->>UI: Return package + version
UI->>User: Show published status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+726
to
+849
| router.post('/registry/examples/:name/publish', async (req: Request, res: Response) => { | ||
| try { | ||
| if (!(await isExamplePublishingEnabled())) { | ||
| return res.status(403).json({ error: 'Example plugin publishing is not enabled' }); | ||
| } | ||
|
|
||
| const userId = await getUserIdFromRequest(req); | ||
| if (!userId) return res.status(401).json({ error: 'Authentication required' }); | ||
|
|
||
| const { name: pluginName } = req.params; | ||
|
|
||
| // Discover examples and validate the requested name exists (path traversal safe) | ||
| const examples = discoverFromDir(MONOREPO_ROOT, 'examples'); | ||
| const example = examples.find((e: DiscoveredPlugin) => e.name === pluginName); | ||
| if (!example) { | ||
| return res.status(404).json({ | ||
| error: 'Example plugin not found', | ||
| available: examples.map((e: DiscoveredPlugin) => e.name), | ||
| }); | ||
| } | ||
|
|
||
| // Verify the plugin has been built | ||
| const bundlePath = path.join( | ||
| MONOREPO_ROOT, 'dist', 'plugins', example.dirName, | ||
| example.version, `${example.dirName}.js`, | ||
| ); | ||
| if (!fs.existsSync(bundlePath)) { | ||
| return res.status(400).json({ | ||
| error: `Plugin "${example.dirName}" must be built first`, | ||
| hint: `Run: bin/build-plugins.sh --plugin ${example.dirName}`, | ||
| }); | ||
| } | ||
|
|
||
| // Atomic publish: package + version + workflow + deployment + install in a transaction | ||
| const result = await db.$transaction(async (tx: any) => { | ||
| const pkgData = toPluginPackageData(example, PLUGIN_CDN_URL); | ||
| const pkg = await tx.pluginPackage.upsert({ | ||
| where: { name: example.name }, | ||
| update: { ...pkgData, publishStatus: 'published' }, | ||
| create: pkgData, | ||
| }); | ||
|
|
||
| const versionData = toPluginVersionData(example, pkg.id, PLUGIN_CDN_URL); | ||
| const version = await tx.pluginVersion.upsert({ | ||
| where: { packageId_version: { packageId: pkg.id, version: example.version } }, | ||
| update: { frontendUrl: versionData.frontendUrl, manifest: versionData.manifest as any }, | ||
| create: versionData, | ||
| }); | ||
|
|
||
| const workflowData = toWorkflowPluginData(example, PLUGIN_CDN_URL, MONOREPO_ROOT); | ||
| const existingWP = await tx.workflowPlugin.findUnique({ | ||
| where: { name: example.name }, | ||
| select: { metadata: true }, | ||
| }); | ||
| const mergedMetadata = { | ||
| ...((existingWP?.metadata as Record<string, unknown>) || {}), | ||
| originalRoutes: example.originalRoutes, | ||
| }; | ||
| await tx.workflowPlugin.upsert({ | ||
| where: { name: example.name }, | ||
| update: { ...workflowData, metadata: mergedMetadata }, | ||
| create: { ...workflowData, metadata: mergedMetadata }, | ||
| }); | ||
|
|
||
| const deployment = await tx.pluginDeployment.upsert({ | ||
| where: { packageId: pkg.id }, | ||
| update: { | ||
| versionId: version.id, | ||
| status: 'running', | ||
| frontendUrl: getBundleUrl(PLUGIN_CDN_URL, example.dirName, example.version), | ||
| deployedAt: new Date(), | ||
| healthStatus: 'healthy', | ||
| }, | ||
| create: { | ||
| packageId: pkg.id, | ||
| versionId: version.id, | ||
| status: 'running', | ||
| frontendUrl: getBundleUrl(PLUGIN_CDN_URL, example.dirName, example.version), | ||
| deployedAt: new Date(), | ||
| healthStatus: 'healthy', | ||
| activeInstalls: 0, | ||
| }, | ||
| }); | ||
|
|
||
| // Auto-install for the publishing user so the plugin appears in their | ||
| // sidebar, settings, and marketplace as "Installed". | ||
| const existingInstall = await tx.tenantPluginInstall.findFirst({ | ||
| where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } }, | ||
| }); | ||
| if (!existingInstall) { | ||
| await tx.tenantPluginInstall.create({ | ||
| data: { | ||
| userId, | ||
| deploymentId: deployment.id, | ||
| status: 'active', | ||
| enabled: true, | ||
| }, | ||
| }); | ||
| await tx.pluginDeployment.update({ | ||
| where: { id: deployment.id }, | ||
| data: { activeInstalls: { increment: 1 } }, | ||
| }); | ||
| } | ||
|
|
||
| await tx.userPluginPreference.upsert({ | ||
| where: { userId_pluginName: { userId, pluginName: example.name } }, | ||
| update: { enabled: true }, | ||
| create: { userId, pluginName: example.name, enabled: true, order: 0, pinned: false }, | ||
| }); | ||
|
|
||
| return { package: pkg, version }; | ||
| }); | ||
|
|
||
| await lifecycleService.audit({ | ||
| action: 'plugin.publish', resource: 'plugin', resourceId: example.name, | ||
| userId, details: { version: example.version, source: 'example' }, | ||
| }); | ||
|
|
||
| res.status(201).json({ success: true, ...result }); | ||
| } catch (error) { | ||
| console.error('Publish example error:', error); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Contributor
Author
There was a problem hiding this comment.
Fixed in commit 2390c01. Added apiLimiter middleware to the publish route: router.post('/registry/examples/:name/publish', apiLimiter, ...).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/marketplace/frontend/src/pages/Marketplace.tsx (1)
546-569:⚠️ Potential issue | 🟠 MajorReset installation state on failed refresh to avoid stale “installed” UI
At Line 547+,
setInstallationsonly runs on success. On non-2xx or thrown errors, old state is retained, so team-switch failures can show incorrect installed badges and actions.Suggested fix
const response = await fetch(url, { credentials: 'include' }); if (response.ok) { const json = await response.json(); const plugins = json.data?.plugins || json.plugins || []; const map = new Map<string, PluginInstallation>(); if (activeTeamId) { // Team context: every plugin returned is team-installed (or core) plugins.forEach((p: { name: string; installId?: string; id?: string }) => { map.set(p.name, { id: p.installId || p.id || '', packageId: '', status: 'active' }); }); } else { // Personal context: only include plugins explicitly installed by the user plugins .filter((p: { installed?: boolean }) => p.installed === true) .forEach((p: { name: string; id?: string }) => { map.set(p.name, { id: p.id || '', packageId: '', status: 'active' }); }); } setInstallations(map); + } else { + setInstallations(new Map()); } } catch (err) { console.error('Failed to load installations:', err); + setInstallations(new Map()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx` around lines 546 - 569, The fetch block that populates installations only calls setInstallations on success, leaving stale state on non-2xx responses or exceptions; update the logic around the fetch/response handling (the block using fetch(url, ...), response.ok, and setInstallations) to ensure setInstallations(new Map()) is called when response.ok is false or inside the catch, so installations are cleared on failure (preserve the existing success path that builds the Map using activeTeamId and plugins).
🧹 Nitpick comments (3)
services/base-svc/src/services/__tests__/publishVerification.test.ts (1)
222-285: Add regression tests for"/plugins/"and plugin-name namespace mismatch.The new route-validation suite is good, but it should also cover
"/plugins/"and routes under another plugin name (for example, manifestmy-pluginwith route"/plugins/other-plugin").🧪 Suggested additions
describe('route validation', () => { @@ it('should reject /admin as reserved', () => { @@ }); + it('should reject bare /plugins/ route', () => { + const result = validatePublishManifest( + validManifest(['/plugins/']) + ); + expect(result.valid).toBe(false); + expect(result.errors.some(e => e.code === 'ROUTE_RESERVED_PATH' || e.code === 'ROUTE_NAMESPACE_VIOLATION')).toBe(true); + }); + + it('should reject routes outside plugin-owned namespace', () => { + const result = validatePublishManifest( + validManifest(['/plugins/other-plugin']) + ); + expect(result.valid).toBe(false); + expect(result.errors.some(e => e.code === 'ROUTE_NAMESPACE_VIOLATION')).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/base-svc/src/services/__tests__/publishVerification.test.ts` around lines 222 - 285, Add two regression tests to the existing "route validation" suite that use the validatePublishManifest helper: one asserting that a manifest named "my-plugin" with a route exactly "/plugins/" is rejected (expect result.valid false and an error code like 'ROUTE_NAMESPACE_VIOLATION' or 'ROUTE_RESERVED_PATH' depending on validation rules), and another asserting that a manifest named "my-plugin" with a route under a different plugin namespace (e.g., "/plugins/other-plugin" or "/plugins/other-plugin/*") is rejected with a namespace-mismatch error (check for code 'ROUTE_NAMESPACE_VIOLATION'); reuse the validManifest(routes: string[]) helper to construct the manifests and follow the same expect patterns (result.valid and result.errors.some(e => e.code === ...)) as the surrounding tests.bin/sync-plugin-registry.ts (1)
195-195: Consider using the sharedtoKebabCaseutility.The kebab-case conversion here duplicates the logic in
packages/database/src/plugin-discovery.ts. Since this file already imports from that module, consider importingtoKebabCasefor consistency.♻️ Proposed fix
Update the import at line 25-31:
import { discoverPlugins, toWorkflowPluginData, toPluginPackageData, toPluginVersionData, getBundleUrl, + toKebabCase, } from '../packages/database/src/plugin-discovery.js';Then at line 195:
- const kebabName = wp.name.replace(/([A-Z])/g, '-$1').toLowerCase().replace(/^-/, ''); + const kebabName = toKebabCase(wp.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/sync-plugin-registry.ts` at line 195, Replace the inline kebab-case conversion that sets kebabName (const kebabName = wp.name.replace(/([A-Z])/g, '-$1').toLowerCase().replace(/^-/, '');) with the shared utility toKebabCase: import toKebabCase from the module that defines it (packages/database/src/plugin-discovery.ts) at the top of the file, then call toKebabCase(wp.name) where kebabName is computed; remove the duplicated regex logic and ensure the import name matches the exported symbol.apps/web-next/src/app/(dashboard)/[...slug]/page.tsx (1)
61-61: Consider extracting globalName derivation to a shared utility.The globalName computation logic here duplicates similar logic in
plugin-discovery.ts. While functional, if the naming convention changes, both locations would need updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(dashboard)/[...slug]/page.tsx at line 61, Extract the globalName derivation into a shared utility function (e.g., createPluginGlobalName or formatPluginGlobalName) and replace the inline logic in page.tsx (the globalName: plugin.globalName || `NaapPlugin${...}` expression referencing pluginName) with a call to that utility; update the other usage in plugin-discovery.ts to call the same utility so both locations use a single source of truth for the naming convention. Ensure the new utility accepts pluginName and optional prefix (default "NaapPlugin") and preserves the current split/capitalize/join behavior, export it for reuse, and add unit tests or a small test case to cover edge cases (hyphens/underscores/empty names).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 34: The dependency "@rollup/rollup-darwin-arm64" is a platform-specific
binary for darwin/arm64 and must be moved out of "dependencies" to
"optionalDependencies" to avoid EBADPLATFORM install failures on non-macOS
CI/runners; update package.json by removing the "@rollup/rollup-darwin-arm64"
entry from the "dependencies" block and add the same entry (with the same
version spec "^4.59.0") under "optionalDependencies", and then regenerate
lockfile (npm install / npm ci) so the lockfile reflects the change.
In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx`:
- Around line 649-650: The fetch call that builds the personal uninstall path
uses raw pkg.name which can contain characters (/, ?, #, ..) that break routing;
update the code where fetch(`/api/v1/installations/${pkg.name}`...) is
constructed (in Marketplace.tsx) to URL-encode the package name using
encodeURIComponent(pkg.name) so the name is treated as a safe path segment, and
apply the same encoding wherever pkg.name is interpolated into URLs (e.g., in
any other fetch or link construction) to prevent routing errors.
In `@plugins/plugin-publisher/frontend/src/lib/api.ts`:
- Around line 489-514: The publishExamplePlugin function currently returns the
full backend response (which includes success) but its declared return type is {
package, version }; update publishExamplePlugin to parse the JSON body (await
res.json()), and return only the result payload by extracting and returning the
package and version (e.g., const json = await res.json(); return { package:
json.package, version: json.version } or return json as the expected shape if
already unpacked), preserving the existing error handling for non-ok responses;
ensure you do not return the top-level success field so the returned value
matches the declared type for publishExamplePlugin.
In `@services/base-svc/src/routes/registry.ts`:
- Line 726: The POST route handler for '/registry/examples/:name/publish' needs
rate limiting: add the existing apiLimiter middleware to the route signature
(i.e., call router.post('/registry/examples/:name/publish', apiLimiter, async
(req: Request, res: Response) => { ... })). If apiLimiter is not already
imported in this module, import the apiLimiter symbol where other routes use it
so the middleware is available.
In `@services/base-svc/src/services/publishVerification.ts`:
- Around line 270-283: Normalize route into basePath by removing trailing
wildcard and also any trailing slash so "/plugins/" becomes "/plugins"; then
reject if basePath === "/plugins" (push ROUTE_NAMESPACE_VIOLATION) and enforce
ownership by requiring basePath to either equal `/plugins/${manifest.name}` or
startWith(`/plugins/${manifest.name}/`) — otherwise push
ROUTE_NAMESPACE_VIOLATION; keep the RESERVED_PATHS.has(basePath) check but run
it against the normalized basePath; update checks around the symbols route,
basePath, manifest.name, and RESERVED_PATHS so plugins cannot claim other
plugins' namespaces or slip through with "/plugins/".
---
Outside diff comments:
In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx`:
- Around line 546-569: The fetch block that populates installations only calls
setInstallations on success, leaving stale state on non-2xx responses or
exceptions; update the logic around the fetch/response handling (the block using
fetch(url, ...), response.ok, and setInstallations) to ensure
setInstallations(new Map()) is called when response.ok is false or inside the
catch, so installations are cleared on failure (preserve the existing success
path that builds the Map using activeTeamId and plugins).
---
Nitpick comments:
In `@apps/web-next/src/app/`(dashboard)/[...slug]/page.tsx:
- Line 61: Extract the globalName derivation into a shared utility function
(e.g., createPluginGlobalName or formatPluginGlobalName) and replace the inline
logic in page.tsx (the globalName: plugin.globalName || `NaapPlugin${...}`
expression referencing pluginName) with a call to that utility; update the other
usage in plugin-discovery.ts to call the same utility so both locations use a
single source of truth for the naming convention. Ensure the new utility accepts
pluginName and optional prefix (default "NaapPlugin") and preserves the current
split/capitalize/join behavior, export it for reuse, and add unit tests or a
small test case to cover edge cases (hyphens/underscores/empty names).
In `@bin/sync-plugin-registry.ts`:
- Line 195: Replace the inline kebab-case conversion that sets kebabName (const
kebabName = wp.name.replace(/([A-Z])/g, '-$1').toLowerCase().replace(/^-/, '');)
with the shared utility toKebabCase: import toKebabCase from the module that
defines it (packages/database/src/plugin-discovery.ts) at the top of the file,
then call toKebabCase(wp.name) where kebabName is computed; remove the
duplicated regex logic and ensure the import name matches the exported symbol.
In `@services/base-svc/src/services/__tests__/publishVerification.test.ts`:
- Around line 222-285: Add two regression tests to the existing "route
validation" suite that use the validatePublishManifest helper: one asserting
that a manifest named "my-plugin" with a route exactly "/plugins/" is rejected
(expect result.valid false and an error code like 'ROUTE_NAMESPACE_VIOLATION' or
'ROUTE_RESERVED_PATH' depending on validation rules), and another asserting that
a manifest named "my-plugin" with a route under a different plugin namespace
(e.g., "/plugins/other-plugin" or "/plugins/other-plugin/*") is rejected with a
namespace-mismatch error (check for code 'ROUTE_NAMESPACE_VIOLATION'); reuse the
validManifest(routes: string[]) helper to construct the manifests and follow the
same expect patterns (result.valid and result.errors.some(e => e.code === ...))
as the surrounding tests.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (17)
apps/web-next/prisma/seed.tsapps/web-next/src/app/(dashboard)/[...slug]/page.tsxbin/pre-push-validate.shbin/sync-plugin-registry.tspackage.jsonpackages/database/package.jsonpackages/database/src/plugin-discovery.tsplugins/marketplace/frontend/src/pages/Marketplace.tsxplugins/plugin-publisher/frontend/src/App.tsxplugins/plugin-publisher/frontend/src/lib/api.tsplugins/plugin-publisher/frontend/src/pages/Dashboard.tsxplugins/plugin-publisher/frontend/src/pages/ExamplePlugins.tsxservices/base-svc/src/routes/registry.tsservices/base-svc/src/server.tsservices/base-svc/src/services/__tests__/pluginLifecycle.integration.test.tsservices/base-svc/src/services/__tests__/publishVerification.test.tsservices/base-svc/src/services/publishVerification.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/base-svc/src/routes/registry.ts (1)
726-726:⚠️ Potential issue | 🟠 MajorAdd
apiLimiterto the example publish route.
POST /registry/examples/:name/publishstill does filesystem work without rate limiting, while the other publish endpoints are limited. Please addapiLimitermiddleware here too.🛡️ Proposed fix
- router.post('/registry/examples/:name/publish', async (req: Request, res: Response) => { + router.post('/registry/examples/:name/publish', apiLimiter, async (req: Request, res: Response) => {Use this read-only check to confirm parity with the other publish routes:
#!/bin/bash rg -n -C2 "router\.post\('/registry/examples/:name/publish'" services/base-svc/src/routes/registry.ts rg -n -C2 "router\.post\('/registry/publish'" services/base-svc/src/routes/registry.ts rg -n -C2 "router\.post\('/registry/publish/token'" services/base-svc/src/routes/registry.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/base-svc/src/routes/registry.ts` at line 726, The POST route handler defined at router.post('/registry/examples/:name/publish') is missing the apiLimiter middleware and should be protected like the other publish endpoints; update the route registration to include apiLimiter as middleware before the async handler (same ordering as router.post('/registry/publish' and router.post('/registry/publish/token')) so incoming requests are rate-limited while the route still performs filesystem work.
🧹 Nitpick comments (2)
.gitignore (2)
99-99: Remove redundant pattern.This pattern is already covered by line 78's
**/tsconfig.tsbuildinfo, which matchestsconfig.tsbuildinfoat any directory depth.🧹 Proposed cleanup
-.migration-complete packages/types/src/*.js packages/types/src/*.d.ts packages/types/src/*.d.ts.map -apps/workflows/*/tsconfig.tsbuildinfo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 99, Remove the redundant ignore entry "apps/workflows/*/tsconfig.tsbuildinfo" from .gitignore because it's already covered by the more general "**/tsconfig.tsbuildinfo" pattern; locate and delete the specific "apps/workflows/*/tsconfig.tsbuildinfo" line so only the general "**/tsconfig.tsbuildinfo" remains.
80-90: Consider a more maintainable approach for ignoring example plugin symlinks.This hardcoded list requires manual updates whenever a new example plugin is added. Consider one of these alternatives:
- Use a naming convention (e.g., prefix with
example-) and a pattern likeplugins/example-*- Add a comment reminding developers to update this list when adding new examples
- Document this requirement in contributing guidelines
A pattern-based approach would reduce maintenance burden, though care must be taken to avoid accidentally ignoring legitimate plugins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 80 - 90, The .gitignore currently lists example plugin symlinks explicitly (plugins/hello-world, plugins/todo-list, plugins/my-wallet, etc.), which is brittle; replace the hardcoded entries with a pattern-based rule (e.g., adopt a naming convention like prefixing example plugins and use a glob such as plugins/example-*) or, if naming cannot change, add a clear comment on the .gitignore near the plugins entries and document the update requirement in CONTRIBUTING.md so future contributors know to update the list when adding examples; ensure the chosen pattern does not accidentally match real plugins before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@services/base-svc/src/routes/registry.ts`:
- Line 726: The POST route handler defined at
router.post('/registry/examples/:name/publish') is missing the apiLimiter
middleware and should be protected like the other publish endpoints; update the
route registration to include apiLimiter as middleware before the async handler
(same ordering as router.post('/registry/publish' and
router.post('/registry/publish/token')) so incoming requests are rate-limited
while the route still performs filesystem work.
---
Nitpick comments:
In @.gitignore:
- Line 99: Remove the redundant ignore entry
"apps/workflows/*/tsconfig.tsbuildinfo" from .gitignore because it's already
covered by the more general "**/tsconfig.tsbuildinfo" pattern; locate and delete
the specific "apps/workflows/*/tsconfig.tsbuildinfo" line so only the general
"**/tsconfig.tsbuildinfo" remains.
- Around line 80-90: The .gitignore currently lists example plugin symlinks
explicitly (plugins/hello-world, plugins/todo-list, plugins/my-wallet, etc.),
which is brittle; replace the hardcoded entries with a pattern-based rule (e.g.,
adopt a naming convention like prefixing example plugins and use a glob such as
plugins/example-*) or, if naming cannot change, add a clear comment on the
.gitignore near the plugins entries and document the update requirement in
CONTRIBUTING.md so future contributors know to update the list when adding
examples; ensure the chosen pattern does not accidentally match real plugins
before committing.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Comment on lines
+726
to
+862
| router.post('/registry/examples/:name/publish', apiLimiter, async (req: Request, res: Response) => { | ||
| try { | ||
| if (!(await isExamplePublishingEnabled())) { | ||
| return res.status(403).json({ error: 'Example plugin publishing is not enabled' }); | ||
| } | ||
|
|
||
| const userId = await getUserIdFromRequest(req); | ||
| if (!userId) return res.status(401).json({ error: 'Authentication required' }); | ||
|
|
||
| const { name: pluginName } = req.params; | ||
|
|
||
| // Discover examples and validate the requested name exists (path traversal safe) | ||
| const examples = discoverFromDir(MONOREPO_ROOT, 'examples'); | ||
| const example = examples.find((e: DiscoveredPlugin) => e.name === pluginName); | ||
| if (!example) { | ||
| return res.status(404).json({ | ||
| error: 'Example plugin not found', | ||
| available: examples.map((e: DiscoveredPlugin) => e.name), | ||
| }); | ||
| } | ||
|
|
||
| // Verify the plugin has been built | ||
| const bundlePath = path.join( | ||
| MONOREPO_ROOT, 'dist', 'plugins', example.dirName, | ||
| example.version, `${example.dirName}.js`, | ||
| ); | ||
| if (!fs.existsSync(bundlePath)) { | ||
| return res.status(400).json({ | ||
| error: `Plugin "${example.dirName}" must be built first`, | ||
| hint: `Run: bin/build-plugins.sh --plugin ${example.dirName}`, | ||
| }); | ||
| } | ||
|
|
||
| // Atomic publish: package + version + workflow + deployment + install in a transaction | ||
| const result = await db.$transaction(async (tx: any) => { | ||
| const pkgData = toPluginPackageData(example, PLUGIN_CDN_URL); | ||
| const pkg = await tx.pluginPackage.upsert({ | ||
| where: { name: example.name }, | ||
| update: { ...pkgData, publishStatus: 'published' }, | ||
| create: pkgData, | ||
| }); | ||
|
|
||
| const versionData = toPluginVersionData(example, pkg.id, PLUGIN_CDN_URL); | ||
| const version = await tx.pluginVersion.upsert({ | ||
| where: { packageId_version: { packageId: pkg.id, version: example.version } }, | ||
| update: { frontendUrl: versionData.frontendUrl, manifest: versionData.manifest as any }, | ||
| create: versionData, | ||
| }); | ||
|
|
||
| const workflowData = toWorkflowPluginData(example, PLUGIN_CDN_URL, MONOREPO_ROOT); | ||
| const existingWP = await tx.workflowPlugin.findUnique({ | ||
| where: { name: example.name }, | ||
| select: { metadata: true }, | ||
| }); | ||
| const mergedMetadata = { | ||
| ...((existingWP?.metadata as Record<string, unknown>) || {}), | ||
| originalRoutes: example.originalRoutes, | ||
| }; | ||
| await tx.workflowPlugin.upsert({ | ||
| where: { name: example.name }, | ||
| update: { ...workflowData, metadata: mergedMetadata }, | ||
| create: { ...workflowData, metadata: mergedMetadata }, | ||
| }); | ||
|
|
||
| const deployment = await tx.pluginDeployment.upsert({ | ||
| where: { packageId: pkg.id }, | ||
| update: { | ||
| versionId: version.id, | ||
| status: 'running', | ||
| frontendUrl: getBundleUrl(PLUGIN_CDN_URL, example.dirName, example.version), | ||
| deployedAt: new Date(), | ||
| healthStatus: 'healthy', | ||
| }, | ||
| create: { | ||
| packageId: pkg.id, | ||
| versionId: version.id, | ||
| status: 'running', | ||
| frontendUrl: getBundleUrl(PLUGIN_CDN_URL, example.dirName, example.version), | ||
| deployedAt: new Date(), | ||
| healthStatus: 'healthy', | ||
| activeInstalls: 0, | ||
| }, | ||
| }); | ||
|
|
||
| // Auto-install for the publishing user so the plugin appears in their | ||
| // sidebar, settings, and marketplace as "Installed". | ||
| const existingInstall = await tx.tenantPluginInstall.findFirst({ | ||
| where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } }, | ||
| }); | ||
| if (!existingInstall) { | ||
| await tx.tenantPluginInstall.create({ | ||
| data: { | ||
| userId, | ||
| deploymentId: deployment.id, | ||
| status: 'active', | ||
| enabled: true, | ||
| }, | ||
| }); | ||
| await tx.pluginDeployment.update({ | ||
| where: { id: deployment.id }, | ||
| data: { activeInstalls: { increment: 1 } }, | ||
| }); | ||
| } | ||
|
|
||
| await tx.userPluginPreference.upsert({ | ||
| where: { userId_pluginName: { userId, pluginName: example.name } }, | ||
| update: { enabled: true }, | ||
| create: { userId, pluginName: example.name, enabled: true, order: 0, pinned: false }, | ||
| }); | ||
|
|
||
| return { package: pkg, version }; | ||
| }); | ||
|
|
||
| // Symlink examples/{name} → plugins/{name} so start.sh and | ||
| // sync-plugin-registry discover it like any regular plugin. | ||
| const symlinkTarget = path.join(MONOREPO_ROOT, 'plugins', example.dirName); | ||
| const symlinkSource = path.join(MONOREPO_ROOT, 'examples', example.dirName); | ||
| if (!fs.existsSync(symlinkTarget) && fs.existsSync(symlinkSource)) { | ||
| try { | ||
| fs.symlinkSync(symlinkSource, symlinkTarget, 'dir'); | ||
| console.log(`[registry] Symlinked plugins/${example.dirName} → examples/${example.dirName}`); | ||
| } catch (linkErr) { | ||
| console.warn(`[registry] Could not create symlink for ${example.dirName}:`, linkErr); | ||
| } | ||
| } | ||
|
|
||
| await lifecycleService.audit({ | ||
| action: 'plugin.publish', resource: 'plugin', resourceId: example.name, | ||
| userId, details: { version: example.version, source: 'example' }, | ||
| }); | ||
|
|
||
| res.status(201).json({ success: true, ...result }); | ||
| } catch (error) { | ||
| console.error('Publish example error:', error); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
services/base-svc/src/services/__tests__/publishVerification.test.ts (1)
319-321: Optional: centralize repeated publish input fixtures inverifyPublishtests.This block repeats the same manifest/frontendUrl shape several times; a shared fixture helper will make future policy updates easier and less error-prone.
♻️ Proposed refactor
describe('verifyPublish', () => { beforeEach(() => { mockFetch.mockReset(); }); + + const basePublishInput = { + manifest: { + name: 'my-plugin', + version: '1.0.0', + frontend: { + entry: './frontend/dist/production/plugin.js', + routes: ['/plugins/my-plugin'], + }, + }, + frontendUrl: 'https://cdn.example.com/plugins/my-plugin/1.0.0/my-plugin.js', + }; it('should pass all checks for valid publish', async () => { mockFetch.mockResolvedValueOnce({ ok: true }); - const result = await verifyPublish({ - manifest: { - name: 'my-plugin', - version: '1.0.0', - displayName: 'My Plugin', - description: 'Test', - frontend: { entry: './frontend/dist/production/plugin.js', routes: ['/plugins/my-plugin'] }, - }, - frontendUrl: 'https://cdn.example.com/plugins/my-plugin/1.0.0/my-plugin.js', - }); + const result = await verifyPublish({ + ...basePublishInput, + manifest: { + ...basePublishInput.manifest, + displayName: 'My Plugin', + description: 'Test', + }, + });Also applies to: 337-339, 370-370, 384-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/base-svc/src/services/__tests__/publishVerification.test.ts` around lines 319 - 321, Tests in publishVerification.test.ts repeat the same publish input object (manifest with frontend: { entry, routes } and frontendUrl) across multiple verifyPublish tests; create a shared fixture function (e.g., makePublishInput or defaultPublishFixture) that returns the common object and update the verifyPublish tests to use that helper instead of inlining the manifest and frontendUrl literals (references: verifyPublish tests, the manifest's frontend field and frontendUrl values) so future policy changes require a single update.services/base-svc/src/routes/registry.ts (2)
678-679: Consider lazy initialization forMONOREPO_ROOT.The path is computed once at module load time using
process.cwd(). If the working directory ever changes during runtime, this could cause issues. While unlikely in production, consider computing this lazily or documenting the assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/base-svc/src/routes/registry.ts` around lines 678 - 679, MONOREPO_ROOT is computed at module load using path.resolve(process.cwd(), process.env.MONOREPO_ROOT || '../..'), which can be stale if cwd changes; change to lazy initialization by replacing the constant with a getter function (e.g., getMonorepoRoot()) that computes and returns path.resolve(process.cwd(), process.env.MONOREPO_ROOT || '../..') on demand, update all references to use getMonorepoRoot(), and document the assumption if you prefer to keep the existing constant.
687-723: Consider adding rate limiting to GET /registry/examples.While this is a read-only endpoint, it performs filesystem discovery via
discoverFromDir. If this operation is expensive, consider adding light rate limiting to prevent abuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/base-svc/src/routes/registry.ts` around lines 687 - 723, Add light rate limiting middleware to the GET /registry/examples route to protect the expensive discoverFromDir call; wrap the route handler (router.get('/registry/examples', ...)) with an express-compatible rate limiter (e.g., express-rate-limit or existing project limiter) configured for a modest requests-per-minute/window and apply it before the async handler, or create and reuse a middleware function (e.g., examplesRateLimiter) and insert it as the second argument to router.get so the filesystem discovery and DB lookup are throttled.plugins/marketplace/frontend/src/pages/Marketplace.tsx (2)
652-653: URL encoding fix looks good; consider applying the same to team uninstall fallback.The
encodeURIComponent(pkg.name)fix properly addresses the previous review comment.However, line 647 (team uninstall) still has an unencoded fallback:
response = await fetch(`${BASE_URL}/api/v1/teams/${teamId}/plugins/${installId || pkg.name}`, ...If
installIdis falsy,pkg.nameis interpolated raw. For consistency and safety, consider:- response = await fetch(`${BASE_URL}/api/v1/teams/${teamId}/plugins/${installId || pkg.name}`, { + response = await fetch(`${BASE_URL}/api/v1/teams/${teamId}/plugins/${installId || encodeURIComponent(pkg.name)}`, {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx` around lines 652 - 653, The team-uninstall fallback currently interpolates pkg.name raw into the URL in the fetch call (the template using BASE_URL and `${teamId}/plugins/${installId || pkg.name}`); update that expression to encode the package name when installId is falsy by wrapping pkg.name with encodeURIComponent so the constructed URL is safe and consistent with the personal-uninstall call; ensure the change is applied in the same fetch/uninstall code path in Marketplace.tsx that builds the team uninstall URL.
566-571: Resetting installations on failure may cause jarring UX.When
loadInstallationsencounters a non-2xx response or exception, the installations state is cleared. If the user has plugins installed and there's a transient network issue, all "Installed" badges will momentarily disappear.Consider preserving the existing state on failure and optionally surfacing an error toast, so the UI doesn't flash unexpectedly:
} else { - setInstallations(new Map()); + console.warn('Failed to fetch installations, keeping existing state'); + // Optionally: setError('Could not refresh installation status'); } } catch (err) { console.error('Failed to load installations:', err); - setInstallations(new Map()); + // Preserve existing state to avoid UI flicker on transient errors }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx` around lines 566 - 571, The catch path in loadInstallations currently clears the UI state by calling setInstallations(new Map()), causing badges to disappear on transient failures; change the error handling so that on non-2xx responses or exceptions you do NOT call setInstallations(new Map()) and instead preserve the existing installations state, surface an error (e.g., via an error toast or setting an error state) to inform the user, and only update setInstallations(...) when the fetch/response is successful inside loadInstallations; update the catch block where setInstallations is called and ensure any callers rely on loadInstallations throwing or returning a success flag rather than clearing state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/sync-plugin-registry.ts`:
- Around line 193-203: The code assumes wp.routes is a string[] and calls
wp.routes.some(), which can throw if routes is null or not an array; before
using .some() coerce/validate the shape (e.g., check Array.isArray(wp.routes)
and that entries are strings) and assign a safe routes array (like const routes
= Array.isArray(wp.routes) ? wp.routes as string[] : []) so hasStaleRoutes uses
that safe value; when updating via prisma.workflowPlugin.update also preserve
originalRoutes by storing the raw wp.routes value into metadata.originalRoutes
only when a non-empty original exists (use the validated routes fallback) and
keep existing metadata handling (wp.metadata) intact.
In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx`:
- Around line 613-617: The personal install uses
`${BASE_URL}/api/v1/tenant/installations` while the personal uninstall hits
`/api/v1/installations/[name]`, which target different data models and can
orphan records; update Marketplace.tsx so the install and uninstall are
symmetric: either point both operations to the same consolidated endpoint/data
model (e.g., use a single tenant installation API for both create and delete) or
modify the uninstall flow (the function that calls
`/api/v1/installations/[name]`) to also call the tenant installation cleanup
endpoint (`/api/v1/tenant/installations` or equivalent) to remove the backend
tenant installation record as well as the Next.js UserPluginPreference, ensuring
both resources are cleaned up. Include BASE_URL and the exact endpoint strings
in your changes so both create and delete operate on the same model.
In `@services/base-svc/src/routes/registry.ts`:
- Around line 626-660: The auto-install block has a TOCTOU race: the separate
find/create calls for pluginDeployment, tenantPluginInstall, and the
activeInstalls increment can race under concurrency; replace the
check-then-create pattern with a single db.$transaction that uses upsert (or an
atomic create-with-conflict-handling) for pluginDeployment, an upsert for
tenantPluginInstall that only increments pluginDeployment.activeInstalls when a
new install is created, and an upsert for userPluginPreference, so all three
operations (pluginDeployment upsert, tenantPluginInstall upsert with conditional
activeInstalls increment, and userPluginPreference.upsert) occur atomically
inside the transaction to prevent duplicate deployments/over-incrementing;
target the pluginDeployment, tenantPluginInstall, pluginDeployment.update
(activeInstalls increment) and userPluginPreference.upsert calls in the current
block.
---
Nitpick comments:
In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx`:
- Around line 652-653: The team-uninstall fallback currently interpolates
pkg.name raw into the URL in the fetch call (the template using BASE_URL and
`${teamId}/plugins/${installId || pkg.name}`); update that expression to encode
the package name when installId is falsy by wrapping pkg.name with
encodeURIComponent so the constructed URL is safe and consistent with the
personal-uninstall call; ensure the change is applied in the same
fetch/uninstall code path in Marketplace.tsx that builds the team uninstall URL.
- Around line 566-571: The catch path in loadInstallations currently clears the
UI state by calling setInstallations(new Map()), causing badges to disappear on
transient failures; change the error handling so that on non-2xx responses or
exceptions you do NOT call setInstallations(new Map()) and instead preserve the
existing installations state, surface an error (e.g., via an error toast or
setting an error state) to inform the user, and only update
setInstallations(...) when the fetch/response is successful inside
loadInstallations; update the catch block where setInstallations is called and
ensure any callers rely on loadInstallations throwing or returning a success
flag rather than clearing state.
In `@services/base-svc/src/routes/registry.ts`:
- Around line 678-679: MONOREPO_ROOT is computed at module load using
path.resolve(process.cwd(), process.env.MONOREPO_ROOT || '../..'), which can be
stale if cwd changes; change to lazy initialization by replacing the constant
with a getter function (e.g., getMonorepoRoot()) that computes and returns
path.resolve(process.cwd(), process.env.MONOREPO_ROOT || '../..') on demand,
update all references to use getMonorepoRoot(), and document the assumption if
you prefer to keep the existing constant.
- Around line 687-723: Add light rate limiting middleware to the GET
/registry/examples route to protect the expensive discoverFromDir call; wrap the
route handler (router.get('/registry/examples', ...)) with an express-compatible
rate limiter (e.g., express-rate-limit or existing project limiter) configured
for a modest requests-per-minute/window and apply it before the async handler,
or create and reuse a middleware function (e.g., examplesRateLimiter) and insert
it as the second argument to router.get so the filesystem discovery and DB
lookup are throttled.
In `@services/base-svc/src/services/__tests__/publishVerification.test.ts`:
- Around line 319-321: Tests in publishVerification.test.ts repeat the same
publish input object (manifest with frontend: { entry, routes } and frontendUrl)
across multiple verifyPublish tests; create a shared fixture function (e.g.,
makePublishInput or defaultPublishFixture) that returns the common object and
update the verifyPublish tests to use that helper instead of inlining the
manifest and frontendUrl literals (references: verifyPublish tests, the
manifest's frontend field and frontendUrl values) so future policy changes
require a single update.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bin/sync-plugin-registry.tspackage.jsonplugins/marketplace/frontend/src/pages/Marketplace.tsxplugins/plugin-publisher/frontend/src/lib/api.tsservices/base-svc/src/routes/registry.tsservices/base-svc/src/services/__tests__/publishVerification.test.tsservices/base-svc/src/services/publishVerification.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- services/base-svc/src/services/publishVerification.ts
- package.json
- plugins/plugin-publisher/frontend/src/lib/api.ts
Comment on lines
+193
to
+203
| const routes = wp.routes as string[]; | ||
| const hasStaleRoutes = routes.some((r) => !r.startsWith('/plugins/')); | ||
| if (hasStaleRoutes) { | ||
| const kebabName = toKebabCase(wp.name); | ||
| const meta = (wp.metadata as Record<string, unknown>) || {}; | ||
| await prisma.workflowPlugin.update({ | ||
| where: { name: wp.name }, | ||
| data: { | ||
| routes: [`/plugins/${kebabName}`, `/plugins/${kebabName}/*`], | ||
| metadata: { ...meta, originalRoutes: meta.originalRoutes || wp.routes }, | ||
| }, |
There was a problem hiding this comment.
Guard wp.routes shape before calling .some() to avoid sync crashes.
Line 193 assumes wp.routes is always string[]; if a record has null/non-array JSON, Line 194 throws and aborts the whole sync job.
Suggested hardening patch
- const routes = wp.routes as string[];
- const hasStaleRoutes = routes.some((r) => !r.startsWith('/plugins/'));
+ const routes = Array.isArray(wp.routes)
+ ? wp.routes.filter((r): r is string => typeof r === 'string')
+ : [];
+ if (routes.length === 0) continue;
+ const hasStaleRoutes = routes.some((r) => !r.startsWith('/plugins/'));
if (hasStaleRoutes) {
const kebabName = toKebabCase(wp.name);
- const meta = (wp.metadata as Record<string, unknown>) || {};
+ const meta =
+ wp.metadata && typeof wp.metadata === 'object' && !Array.isArray(wp.metadata)
+ ? (wp.metadata as Record<string, unknown>)
+ : {};
await prisma.workflowPlugin.update({
where: { name: wp.name },
data: {
routes: [`/plugins/${kebabName}`, `/plugins/${kebabName}/*`],
- metadata: { ...meta, originalRoutes: meta.originalRoutes || wp.routes },
+ metadata: { ...meta, originalRoutes: meta.originalRoutes || routes },
},
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/sync-plugin-registry.ts` around lines 193 - 203, The code assumes
wp.routes is a string[] and calls wp.routes.some(), which can throw if routes is
null or not an array; before using .some() coerce/validate the shape (e.g.,
check Array.isArray(wp.routes) and that entries are strings) and assign a safe
routes array (like const routes = Array.isArray(wp.routes) ? wp.routes as
string[] : []) so hasStaleRoutes uses that safe value; when updating via
prisma.workflowPlugin.update also preserve originalRoutes by storing the raw
wp.routes value into metadata.originalRoutes only when a non-empty original
exists (use the validated routes fallback) and keep existing metadata handling
(wp.metadata) intact.
Comment on lines
+613
to
+617
| // Personal install via tenant installations endpoint | ||
| response = await fetch(`${BASE_URL}/api/v1/tenant/installations`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| credentials: 'include', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, understand the repository structure
echo "=== Repository structure (top-level) ==="
ls -la
echo ""
echo "=== Checking for backend service routes related to tenant installations ==="
find . -type f -name "*.ts" -o -name "*.js" | xargs grep -l "tenant/installations" 2>/dev/null | head -10
echo ""
echo "=== Searching for installations endpoint patterns ==="
find . -type f \( -name "*.ts" -o -name "*.js" \) | xargs grep -l "api/v1/installations" 2>/dev/null | head -10
echo ""
echo "=== Looking for Next.js API routes with 'installations' ==="
find . -path "*/app/api/*" -o -path "*/pages/api/*" | grep installationsRepository: livepeer/naap
Length of output: 3716
🏁 Script executed:
#!/bin/bash
# Check the marketplace frontend file to understand both operations
echo "=== Personal uninstall endpoint at line 653 ==="
sed -n '645,665p' plugins/marketplace/frontend/src/pages/Marketplace.tsx
echo ""
echo "=== Check if there's a Next.js API route for installations ==="
find . -type f \( -name "*.ts" -o -name "*.js" \) -path "*/api/*" | xargs grep -l "installations" 2>/dev/null | grep -v node_modules | head -10Repository: livepeer/naap
Length of output: 1596
🏁 Script executed:
#!/bin/bash
# Search for the backend tenant/installations endpoint
echo "=== Search for 'tenant/installations' in backend routes ==="
rg "tenant/installations" -t ts -g '!**/node_modules/**' -B 2 -A 5
echo ""
echo "=== Search for installations DELETE/uninstall handlers ==="
rg "installations.*DELETE|DELETE.*installations" -t ts -g '!**/node_modules/**' -B 2 -A 5Repository: livepeer/naap
Length of output: 34396
🏁 Script executed:
#!/bin/bash
# Check the Next.js tenant installations endpoint implementation
echo "=== Next.js tenant installations POST route ==="
cat -n apps/web-next/src/app/api/v1/tenant/installations/route.ts | head -50
echo ""
echo "=== Check for comments explaining the routing strategy ==="
grep -B5 -A10 "tenant/installations" plugins/marketplace/frontend/src/pages/Marketplace.tsx | head -40Repository: livepeer/naap
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Check if there's a Next.js route that mirrors the backend endpoint
echo "=== Search for any installations POST endpoint ==="
find apps/web-next/src/app/api/v1 -name "route.ts" -exec grep -l "POST.*installations" {} \;
echo ""
echo "=== Check what loadInstallations uses ==="
grep -B5 -A10 "loadInstallations" plugins/marketplace/frontend/src/pages/Marketplace.tsx | head -30Repository: livepeer/naap
Length of output: 1342
🏁 Script executed:
#!/bin/bash
# Check the Next.js installations DELETE implementation
echo "=== Next.js DELETE /api/v1/installations/[name] route ==="
cat -n apps/web-next/src/app/api/v1/installations/\[name\]/route.ts
echo ""
echo "=== Check for UserPluginPreference references ==="
rg "UserPluginPreference" --type ts -B 2 -A 5 | head -50Repository: livepeer/naap
Length of output: 7077
Refactor personal install/uninstall to use consistent endpoints and data models.
Personal install at line 614 uses ${BASE_URL}/api/v1/tenant/installations (creates a backend "tenant installation"), while personal uninstall at line 653 uses /api/v1/installations/[name] (deletes UserPluginPreference in Next.js). These operations target different data models in different systems—the install doesn't appear to create a UserPluginPreference, and the uninstall doesn't clean up the backend tenant installation. This risks orphaning records.
Either consolidate both operations to a single endpoint and data model, or ensure the uninstall properly cleans up both the UserPluginPreference AND the backend tenant installation record.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/marketplace/frontend/src/pages/Marketplace.tsx` around lines 613 -
617, The personal install uses `${BASE_URL}/api/v1/tenant/installations` while
the personal uninstall hits `/api/v1/installations/[name]`, which target
different data models and can orphan records; update Marketplace.tsx so the
install and uninstall are symmetric: either point both operations to the same
consolidated endpoint/data model (e.g., use a single tenant installation API for
both create and delete) or modify the uninstall flow (the function that calls
`/api/v1/installations/[name]`) to also call the tenant installation cleanup
endpoint (`/api/v1/tenant/installations` or equivalent) to remove the backend
tenant installation record as well as the Next.js UserPluginPreference, ensuring
both resources are cleaned up. Include BASE_URL and the exact endpoint strings
in your changes so both create and delete operate on the same model.
Comment on lines
+626
to
+660
| // Auto-install for the publisher: ensure a deployment + TenantPluginInstall | ||
| // exist so the plugin is immediately visible in their sidebar and settings. | ||
| try { | ||
| let deployment = await db.pluginDeployment.findUnique({ where: { packageId: pkg.id } }); | ||
| if (!deployment) { | ||
| deployment = await db.pluginDeployment.create({ | ||
| data: { | ||
| packageId: pkg.id, versionId: version.id, status: 'running', | ||
| frontendUrl: frontendUrl || '', deployedAt: new Date(), | ||
| healthStatus: 'healthy', activeInstalls: 0, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const existingInstall = await db.tenantPluginInstall.findFirst({ | ||
| where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } }, | ||
| }); | ||
| if (!existingInstall) { | ||
| await db.tenantPluginInstall.create({ | ||
| data: { userId, deploymentId: deployment.id, status: 'active', enabled: true }, | ||
| }); | ||
| await db.pluginDeployment.update({ | ||
| where: { id: deployment.id }, | ||
| data: { activeInstalls: { increment: 1 } }, | ||
| }); | ||
| } | ||
|
|
||
| await db.userPluginPreference.upsert({ | ||
| where: { userId_pluginName: { userId, pluginName: manifest.name } }, | ||
| update: { enabled: true }, | ||
| create: { userId, pluginName: manifest.name, enabled: true, order: 0, pinned: false }, | ||
| }); | ||
| } catch (installErr) { | ||
| console.warn('Auto-install after publish failed (non-fatal):', installErr); | ||
| } |
There was a problem hiding this comment.
TOCTOU race condition in auto-install logic.
The check-then-create pattern outside a transaction is susceptible to race conditions. Two concurrent publish requests for the same package could:
- Both find no deployment and race to create one (one fails with unique constraint, masked by catch)
- Both find no install and both create installs, over-incrementing
activeInstalls
The example publish flow (lines 760-837) correctly uses $transaction with upsert. Consider applying the same pattern here.
🔧 Proposed fix using upsert within a transaction
try {
- let deployment = await db.pluginDeployment.findUnique({ where: { packageId: pkg.id } });
- if (!deployment) {
- deployment = await db.pluginDeployment.create({
- data: {
- packageId: pkg.id, versionId: version.id, status: 'running',
- frontendUrl: frontendUrl || '', deployedAt: new Date(),
- healthStatus: 'healthy', activeInstalls: 0,
- },
- });
- }
-
- const existingInstall = await db.tenantPluginInstall.findFirst({
- where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } },
- });
- if (!existingInstall) {
- await db.tenantPluginInstall.create({
- data: { userId, deploymentId: deployment.id, status: 'active', enabled: true },
- });
- await db.pluginDeployment.update({
- where: { id: deployment.id },
- data: { activeInstalls: { increment: 1 } },
- });
- }
-
- await db.userPluginPreference.upsert({
- where: { userId_pluginName: { userId, pluginName: manifest.name } },
- update: { enabled: true },
- create: { userId, pluginName: manifest.name, enabled: true, order: 0, pinned: false },
- });
+ await db.$transaction(async (tx: any) => {
+ const deployment = await tx.pluginDeployment.upsert({
+ where: { packageId: pkg.id },
+ update: {},
+ create: {
+ packageId: pkg.id, versionId: version.id, status: 'running',
+ frontendUrl: frontendUrl || '', deployedAt: new Date(),
+ healthStatus: 'healthy', activeInstalls: 0,
+ },
+ });
+
+ const existingInstall = await tx.tenantPluginInstall.findFirst({
+ where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } },
+ });
+ if (!existingInstall) {
+ await tx.tenantPluginInstall.create({
+ data: { userId, deploymentId: deployment.id, status: 'active', enabled: true },
+ });
+ await tx.pluginDeployment.update({
+ where: { id: deployment.id },
+ data: { activeInstalls: { increment: 1 } },
+ });
+ }
+
+ await tx.userPluginPreference.upsert({
+ where: { userId_pluginName: { userId, pluginName: manifest.name } },
+ update: { enabled: true },
+ create: { userId, pluginName: manifest.name, enabled: true, order: 0, pinned: false },
+ });
+ });
} catch (installErr) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Auto-install for the publisher: ensure a deployment + TenantPluginInstall | |
| // exist so the plugin is immediately visible in their sidebar and settings. | |
| try { | |
| let deployment = await db.pluginDeployment.findUnique({ where: { packageId: pkg.id } }); | |
| if (!deployment) { | |
| deployment = await db.pluginDeployment.create({ | |
| data: { | |
| packageId: pkg.id, versionId: version.id, status: 'running', | |
| frontendUrl: frontendUrl || '', deployedAt: new Date(), | |
| healthStatus: 'healthy', activeInstalls: 0, | |
| }, | |
| }); | |
| } | |
| const existingInstall = await db.tenantPluginInstall.findFirst({ | |
| where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } }, | |
| }); | |
| if (!existingInstall) { | |
| await db.tenantPluginInstall.create({ | |
| data: { userId, deploymentId: deployment.id, status: 'active', enabled: true }, | |
| }); | |
| await db.pluginDeployment.update({ | |
| where: { id: deployment.id }, | |
| data: { activeInstalls: { increment: 1 } }, | |
| }); | |
| } | |
| await db.userPluginPreference.upsert({ | |
| where: { userId_pluginName: { userId, pluginName: manifest.name } }, | |
| update: { enabled: true }, | |
| create: { userId, pluginName: manifest.name, enabled: true, order: 0, pinned: false }, | |
| }); | |
| } catch (installErr) { | |
| console.warn('Auto-install after publish failed (non-fatal):', installErr); | |
| } | |
| // Auto-install for the publisher: ensure a deployment + TenantPluginInstall | |
| // exist so the plugin is immediately visible in their sidebar and settings. | |
| try { | |
| await db.$transaction(async (tx: any) => { | |
| const deployment = await tx.pluginDeployment.upsert({ | |
| where: { packageId: pkg.id }, | |
| update: {}, | |
| create: { | |
| packageId: pkg.id, versionId: version.id, status: 'running', | |
| frontendUrl: frontendUrl || '', deployedAt: new Date(), | |
| healthStatus: 'healthy', activeInstalls: 0, | |
| }, | |
| }); | |
| const existingInstall = await tx.tenantPluginInstall.findFirst({ | |
| where: { userId, deploymentId: deployment.id, status: { not: 'uninstalled' } }, | |
| }); | |
| if (!existingInstall) { | |
| await tx.tenantPluginInstall.create({ | |
| data: { userId, deploymentId: deployment.id, status: 'active', enabled: true }, | |
| }); | |
| await tx.pluginDeployment.update({ | |
| where: { id: deployment.id }, | |
| data: { activeInstalls: { increment: 1 } }, | |
| }); | |
| } | |
| await tx.userPluginPreference.upsert({ | |
| where: { userId_pluginName: { userId, pluginName: manifest.name } }, | |
| update: { enabled: true }, | |
| create: { userId, pluginName: manifest.name, enabled: true, order: 0, pinned: false }, | |
| }); | |
| }); | |
| } catch (installErr) { | |
| console.warn('Auto-install after publish failed (non-fatal):', installErr); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/base-svc/src/routes/registry.ts` around lines 626 - 660, The
auto-install block has a TOCTOU race: the separate find/create calls for
pluginDeployment, tenantPluginInstall, and the activeInstalls increment can race
under concurrency; replace the check-then-create pattern with a single
db.$transaction that uses upsert (or an atomic create-with-conflict-handling)
for pluginDeployment, an upsert for tenantPluginInstall that only increments
pluginDeployment.activeInstalls when a new install is created, and an upsert for
userPluginPreference, so all three operations (pluginDeployment upsert,
tenantPluginInstall upsert with conditional activeInstalls increment, and
userPluginPreference.upsert) occur atomically inside the transaction to prevent
duplicate deployments/over-incrementing; target the pluginDeployment,
tenantPluginInstall, pluginDeployment.update (activeInstalls increment) and
userPluginPreference.upsert calls in the current block.
|
This PR has conflicts with the base branch. Please rebase to resolve them: git fetch origin
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThe |
5 tasks
Add a feature-flagged UI in Plugin Publisher that lets users browse bundled example plugins and one-click publish them to the marketplace. Gated behind `enableExamplePublishing` feature flag (default: off). - Add `enableExamplePublishing` FeatureFlag to database seed - Add `discoverFromDir()` to plugin-discovery (refactor discoverPlugins to delegate) - Add `GET /registry/examples` and `POST /registry/examples/:name/publish` API endpoints - Add ExamplePlugins page with card grid, publish/published states, and disabled-feature handling - Add "Example Plugins" quick action to Plugin Publisher Dashboard - Export `@naap/database/plugin-discovery` subpath for clean imports - Atomic publish via `$transaction` (PluginPackage + Version + Workflow + Deployment) - Path traversal protection: validate plugin name against discovered list, not filesystem Made-with: Cursor
- Normalize non-core plugin routes to /plugins/{name} convention
- Add catch-all route in Next.js shell for dynamic plugin routing
- Add route namespace validation in publish verification
- Preserve originalRoutes in metadata for core plugin migration
- Add pre-push safety check to prevent plugin-discovery in Next.js runtime
- Update sync-plugin-registry to auto-normalize stale top-level routes
- Add comprehensive route validation tests
Made-with: Cursor
normalizePluginRoutes was assigning /plugins/{name} routes to all
non-core plugins, breaking headless providers like dashboard-provider-mock
that rely on empty routes to be loaded by BackgroundPluginLoader.
Made-with: Cursor
When an example plugin is published, create a symlink from plugins/{name}
to examples/{name} so start.sh discovers and starts its backend like any
regular plugin. Add gitignore entries for example plugin symlinks.
Made-with: Cursor
- Move @rollup/rollup-darwin-arm64 to optionalDependencies to fix EBADPLATFORM on non-macOS CI runners - Add rate limiting (apiLimiter) to POST /registry/examples/:name/publish - URL-encode plugin name in uninstall path to prevent routing issues - Reset installation state on failed API response to avoid stale UI - Harden route validation: reject bare /plugins/, enforce plugin-owned namespace to prevent cross-plugin route claiming - Extract response payload in publishExamplePlugin to match return type - Use shared toKebabCase utility in sync-plugin-registry - Fix test URLs from http:// to https:// matching HTTPS enforcement - Add regression tests for /plugins/ and namespace mismatch routes Made-with: Cursor
- CRITICAL: Narrow CSRF bypass scope — remove broad `/examples/` match from server.ts CSRF skip (example-publish uses JWT auth, CSRF protection should apply) - CRITICAL: Wrap user-publish auto-install in db.$transaction() to prevent TOCTOU race on concurrent publishes (matches the pattern already used in example-publish) - CRITICAL: Add authentication to GET /registry/examples endpoint (was unauthenticated despite being behind feature flag) - MAJOR: Add runtime type validation for wp.routes before casting to string[] in sync-plugin-registry.ts (prevents crash on null/ non-array JSON values) - MAJOR: Validate dirName against safe pattern before symlink creation in example-publish (defense-in-depth against path traversal) - MINOR: Remove dead code normalizeName function from catch-all plugin page Made-with: Cursor
seanhanca
force-pushed
the
feat/example-plugin-publishing
branch
from
2390c01 to
8871b71
Compare
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enableExamplePublishingfeature flag) that lets users browse shipped example plugins and publish them to the marketplace with one click. This surfaces plugins that are already bundled with the platform but previously hidden from end users./plugins/{name}routes/plugins/namespaceplugin-discoveryin Next.js runtime (Vercel safety)sync-plugin-registryauto-normalizes stale top-level routes for non-core pluginsdashboard-provider-mock) so theBackgroundPluginLoadercorrectly identifies and loads them for EventBus handler registration.Changes
packages/database/src/plugin-discovery.tsservices/base-svc/src/routes/registry.ts,services/base-svc/src/server.tsservices/base-svc/src/services/publishVerification.tsbin/sync-plugin-registry.tsapps/web-next/src/app/(dashboard)/[...slug]/page.tsx(new)bin/pre-push-validate.shplugins/marketplace/frontend/src/pages/Marketplace.tsxpublishVerification.test.ts,pluginLifecycle.integration.test.tsTest plan
enableExamplePublishingfeature flag on — verify example plugin list appears in Plugin Publisher/plugins/{name}routes after sync/plugins/namespaceMade with Cursor
Summary by CodeRabbit
New Features
Improvements
Bug Fixes