hot reloading engine using plugin architecture - #150
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds plugin infrastructure: an NPM webhook endpoint that enqueues plugin-install events, a PluginManager for install/require with migration dispatch and rollback, a MigrationWorker that fans out or runs per-tenant migrations, a Piscina-backed ExecutionWorker and worker entrypoint for running piece triggers, startup piece-loading integration, module wiring, tests, and registry package/test config updates. ChangesPlugin Installation and Execution System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/modules/plugins/plugins.controller.spec.ts`:
- Around line 18-46: Add a unit test that verifies
PluginsController.handleNpmWebhook rejects unauthenticated requests when
NPM_WEBHOOK_SECRET is not configured: update the existing test setup by
overriding the configService.get mock to return undefined for
'NPM_WEBHOOK_SECRET', call controller.handleNpmWebhook with a sample
WebhookPayloadDto, assert it throws UnauthorizedException, and ensure
installPieceMock (the mocked PluginManagerService.installPiece) is not called;
reference controller.handleNpmWebhook, configService.get, and installPieceMock
to locate where to add the test.
- Around line 49-52: The test helper generateValidSignature currently computes
the HMAC from JSON.stringify(payload), which mirrors the controller bug; update
generateValidSignature (and tests that call it) to compute the HMAC over the
exact raw request body string that the webhook receiver will see (i.e., the
serialized JSON text sent over the wire) using TEST_SECRET, not the JS object;
modify the helper signature to accept the rawBody string (or derive the exact
serialized string used by tests) and use that raw string when calling
crypto.createHmac(...) so signatures match the controller once it is fixed to
validate against the raw request body.
In `@apps/api/src/modules/plugins/plugins.controller.ts`:
- Around line 86-107: The webhook handler currently awaits
pluginManager.installPiece(...) which can block the HTTP response; change it to
an acknowledge-then-process pattern: return the success response immediately
after validating the request and then invoke
pluginManager.installPiece(packageName, version) asynchronously
(fire-and-forget) so installation runs in background (ensure installPiece
already catches/logs its own errors or wrap the async call in a try/catch inside
a background task to log failures). Also add a short HTTP timeout/guard in the
controller (e.g., limit work done before response) and ensure any retryable
failures are surfaced via logs/metrics so external webhook retries or a retry
queue can be used later. Ensure you modify the webhook handler method (the
function invoking pluginManager.installPiece) to stop awaiting the installer and
to immediately respond.
- Around line 52-67: The current NPM webhook handling (uses NPM_WEBHOOK_SECRET,
webhookSecret and signature in plugins.controller.ts) only validates signatures
when webhookSecret is present, allowing unauthenticated requests if the env var
is missing; make signature validation mandatory by failing requests when
NPM_WEBHOOK_SECRET is not set or empty (throw UnauthorizedException or return
401 in the webhook handler) and/or add a startup check in the PluginsController
or app bootstrap that logs an error and prevents startup when NPM_WEBHOOK_SECRET
is missing; update the logic around webhookSecret/signature to always require a
signature match against the HMAC digest and reference the existing variables
(webhookSecret, signature, digest) and the UnauthorizedException path to
implement the enforced behavior.
- Around line 52-67: The HMAC is being computed over JSON.stringify(payload)
which uses the parsed object and can change byte ordering/whitespace; change to
compute the HMAC over the raw request body bytes instead: add a RawBody
createParamDecorator (e.g., RawBody) that reads request.rawBody, configure the
application JSON parser (in main.ts) with a verify hook to set req.rawBody =
buf.toString('utf8'), then update the controller code that currently uses
webhookSecret, signature and payload to use the injected raw body value
(rawBody) when creating the HMAC digest so the signature comparison uses the
exact original request bytes.
- Line 59: Replace the direct string comparison of signature and digest with a
constant-time comparison: convert both signature and digest to Buffers of the
same length (e.g., Buffer.from(..., 'utf8' or 'hex' as appropriate), ensure
equal length by failing early if not, then use crypto.timingSafeEqual(bufSig,
bufDigest) to validate the HMAC; update the conditional around the variables
signature and digest in the plugins controller to use this check and handle
failures consistently (throw/return error) instead of using !==.
In `@apps/api/src/modules/plugins/plugins.module.ts`:
- Line 6: The PluginsModule currently imports the bare PiecesModule which has no
providers, causing PluginManagerService injection to fail; change the import to
use PiecesModule.forRoot() (or remove the import if PiecesModule.forRoot() is
already imported globally in AppModule) so the providers (including
PluginManagerService) are registered and PluginsController can resolve its
dependency. Update the imports array in PluginsModule to reference
PiecesModule.forRoot() (or remove the explicit import) and ensure PluginsModule
still declares/providers for PluginsController as before.
In `@packages/pieces/platform/registry/package.json`:
- Around line 27-30: Update the vulnerable dependencies in
packages/pieces/platform/registry/package.json: bump "drizzle-orm" from ^0.45.1
to at least ^0.45.2 (first patched version) and update devDependency "vitest"
from 2.1.9 to at least 4.1.0 (first patched version); ensure package.json
reflects these new versions and then run the package manager install and tests
to verify no breaking changes in code paths that use drizzle-orm and vitest
(search for usages of "drizzle-orm" and test scripts referencing "vitest" to
confirm compatibility).
In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts`:
- Around line 27-31: The handler currently acknowledges any message that lacks
plugin fields, which can drop unrelated TenantProvisionQueue events; add a
discriminant check and only process/ack plugin migration messages: implement an
isPluginMigrationEvent type guard that verifies event.pluginLocation and
event.pieceName (or a new event.type === 'plugin_migration') and call
runBackgroundMigrations only when that guard passes; for non-plugin messages
either send them to a dedicated PluginMigrationQueue or nack/requeue/forward the
original payload so other consumers (e.g., TenantProvisionQueue handlers) can
handle them instead of being silently removed.
- Around line 74-87: The current stub helpers getActiveTenants and
getTenantDbConnection are unsafe for production and will run migrations against
the wrong DB; change them to fail-fast until a real implementation is provided:
make getActiveTenants throw or return an empty list (or read from a real tenant
store) when the hardcoded array is present, and make getTenantDbConnection throw
an explicit error rather than returning this.globalDb so migrations cannot
proceed; reference and update getActiveTenants and getTenantDbConnection in
migration-worker.service.ts and add a clear error message indicating that tenant
resolution/connection management must be implemented before running migrations.
In `@packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts`:
- Around line 27-31: Replace the brittle string checks around resolvedPath
(isLocalWorkspace / isGlobalPlugins) with strict canonical-root validation:
resolve and canonicalize resolvedPath (fs.realpathSync or async) and
canonicalize each allowed root (workspace package roots and
PluginManagerService.PLUGINS_PATH), then ensure the canonical resolvedPath has
one of those canonical roots as a strict path-prefix using boundary-aware checks
(e.g., compare prefix + path.sep or use path.relative to verify it is not
escaping the root) before throwing the sandbox error; update the logic in the
piece-execution.worker code that references resolvedPath, isLocalWorkspace, and
isGlobalPlugins to use the canonical allowed roots array and robust prefix
checks so configurable PLUGINS_PATH is honored and path traversal cannot bypass
validation.
- Line 37: The worker uses require(resolvedPath) which crashes in ESM because
require is not defined; fix by importing createRequire from 'node:module' and
creating a scoped require with createRequire(import.meta.url), then use that
local require to load the piece (replace the direct require(resolvedPath) call
in piece-execution.worker.ts so the variable piece is assigned via the created
require function).
In `@packages/pieces/platform/registry/src/pieces/piece-loader.service.ts`:
- Around line 53-77: The current local-development fallback (the try/catch that
calls pluginManager.ensurePiece, pluginManager.requirePiece and then does the
resolvedPath / import fallback using anchorUrl/import.meta.url) runs
unconditionally and can load unintended local code in production; change the
catch(downloadErr) block so that it only attempts the local workspace resolution
when an explicit dev flag is set (e.g., this.isDev or this.config.devMode) and
otherwise re-throws or returns the original error after logging; specifically,
inside the catch for pluginManager.ensurePiece / pluginManager.requirePiece, log
the warning as now but if not in dev mode do not execute the
resolvedPath/hostRequire/import fallback logic (instead throw downloadErr or a
new Error), and keep the existing resolvedPath/anchorUrl/import.meta.url code
path behind that dev check so only development runs it.
In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts`:
- Around line 18-23: The default pluginsPath falls back to os.tmpdir(), which is
insecure for executable artifacts; change the default in the PluginManager
initialization to an app-owned persistent directory (e.g., under the app
home/data like ~/.soopa/plugins or a directory derived from a dedicated
APP_DATA_DIR env var) and only use os.tmpdir() when explicitly in dev mode
(e.g., NODE_ENV === 'development' or an explicit DEV_MODE flag). Ensure the code
that sets up pluginsPath (the this.pluginsPath assignment used when constructing
new PluginManager) creates the directory if missing and applies restrictive
permissions/ownership (e.g., mode 0o700) so executable plugins are stored in a
persistent, private location rather than /tmp.
In `@packages/pieces/platform/registry/vitest.config.mts`:
- Around line 30-54: The vitest.config.mts 'exclude' array contains many
app-level patterns that don't exist in this registry package (e.g.,
'src/main.ts', 'src/modules/ai/**', 'src/db/reset-e2e.ts', 'src/db/db-cli.ts');
open the exclude array in vitest.config.mts and remove or replace entries that
don't match this package's layout, keeping only registry-relevant patterns (for
example: 'node_modules/**', 'dist/**', '**/*.d.ts', '**/*.spec.ts' /
'**/*.e2e-spec.ts', 'test/**', and any actual local build/test artifact
patterns), and validate against the package's src structure so the list only
excludes real, relevant paths.
- Around line 18-20: Update the Vitest server.deps.external list to remove the
stale '`@nexiom/auth`' and '`@nexiom/identity`' entries: open the
server.deps.external array (symbol: server.deps.external) in vitest.config.mts,
delete those two '`@nexiom/`*' strings, and either add the correct '`@soopa/`*'
package names that appear in this package's package.json or leave them out if no
externals are required; ensure the array matches the actual dependencies in
package.json.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d50416f-b7dc-48da-b0cd-3de1bea75b68
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
apps/api/src/app/app.module.tsapps/api/src/modules/plugins/plugins.controller.spec.tsapps/api/src/modules/plugins/plugins.controller.tsapps/api/src/modules/plugins/plugins.module.tspackages/pieces/platform/quickbooks/package.jsonpackages/pieces/platform/registry/package.jsonpackages/pieces/platform/registry/src/index.tspackages/pieces/platform/registry/src/pieces/execution-worker.service.spec.tspackages/pieces/platform/registry/src/pieces/execution-worker.service.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.spec.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.tspackages/pieces/platform/registry/src/pieces/piece-execution.worker.tspackages/pieces/platform/registry/src/pieces/piece-loader.service.spec.tspackages/pieces/platform/registry/src/pieces/piece-loader.service.tspackages/pieces/platform/registry/src/pieces/pieces.module.tspackages/pieces/platform/registry/src/pieces/plugin-manager.service.spec.tspackages/pieces/platform/registry/src/pieces/plugin-manager.service.tspackages/pieces/platform/registry/vitest.config.mtspackages/pieces/platform/salesforce/package.jsonpackages/queue/src/events/plugin-migration.event.tspackages/queue/src/index.ts
💤 Files with no reviewable changes (2)
- packages/pieces/platform/quickbooks/package.json
- packages/pieces/platform/salesforce/package.json
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 10 file(s) based on 15 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 10 file(s) based on 15 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts (2)
86-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispatch migrations when
ensurePiece()installs a missing package.The miss path bypasses
installPiece()and callsthis.manager.install()directly, so startup sync downloads new code without publishing theTenantProvisionQueueevent.PieceLoaderServicethen immediately requires that module on startup, which can bring a piece online before its tenant migrations have run.Suggested minimal fix
this.logger.log(`Startup Sync: Downloading missing piece ${packageName}...`); - const pluginInfo = await this.manager.install(packageName, version); - this.logger.log(`Startup Sync: Installed ${packageName} to ${pluginInfo.location}`); - return pluginInfo; + return this.installPiece(packageName, version);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts` around lines 86 - 96, ensurePiece currently calls this.manager.install(...) directly which skips the migration/publishing flow; change ensurePiece to delegate to the existing installPath that triggers tenant migrations and the TenantProvisionQueue event (e.g., call this.installPiece(packageName, version) instead of this.manager.install, or if keeping manager.install then immediately publish the TenantProvisionQueue event and invoke the migration dispatch used by installPiece). Update references in ensurePiece (function ensurePiece) so newly installed pieces go through the same migration/publish logic as installPiece and avoid loading a piece before migrations run.
56-65:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't expose a plugin install unless the migration job is durably recorded.
Line 56 installs the package before Line 65 enqueues the
PluginMigrationEvent. IfqueueService.send()fails, the method rethrows, but the new code is already on disk and can still be loaded bypackages/pieces/platform/registry/src/pieces/piece-loader.service.tsLines 60-63 without the tenant migrations that make it safe to run. This needs an atomic recovery path: persist the event before exposing the install, or roll the install back on dispatch failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts` around lines 56 - 65, The install is exposed before the migration job is durably recorded; change the flow so the migration event is guaranteed or the install is rolled back: either (preferred) persist/enqueue the PluginMigrationEvent via queueService.send (or a durable DB record) before calling this.logger.log or returning, or (if enqueue-before-install isn't possible) catch failures from queueService.send and call this.manager.uninstall(packageName, pluginInfo.version) to roll back the install (and only log/return on successful enqueue); update the code around this.manager.install, PluginMigrationEvent construction, and queueService.send to implement the chosen approach and ensure errors are rethrown after rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/modules/plugins/plugins.controller.ts`:
- Around line 113-126: The controller currently calls
pluginManager.installPiece(packageName, version) and immediately returns
accepted, risking lost installs if the process dies; instead persist a durable
install job/outbox record before responding (e.g., create an InstallJob or
Outbox entry with packageName, version, request metadata) and enqueue/emit the
job ID; then have a separate worker/process pick up that job and call
pluginManager.installPiece(job.packageName, job.version). Replace the direct
fire-and-forget installPiece() invocation in the webhook handler with a
persistent enqueue operation (and optionally still fire-and-forget the job
dispatch), and return the accepted response referencing the persisted job ID.
In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts`:
- Around line 93-107: The migration worker currently throws because the stubbed
helpers getActiveTenants and getTenantDbConnection are unimplemented and
plugin-manager.service enqueues TenantProvisionQueue events without tenantId, so
the worker always hits these stubs; fix by either implementing tenant resolution
and connection management used by migration-worker.service (provide a real
implementation for getActiveTenants to return active tenant IDs for a given
pieceName and for getTenantDbConnection to return a tenant-scoped DB client from
your TenantDatabaseManager/pool) or by disabling migration dispatch/consumer
registration until those helpers exist (guard the code path that
enqueues/consumes TenantProvisionQueue events and the migration registration
behind a feature flag or a check that ensures tenant handlers are available).
Ensure you update or remove the tests that currently spy on the private helpers
once real implementations or guards are in place.
In `@packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts`:
- Around line 31-45: The sandbox root computation in piece-execution.worker
(variables pluginsPath and allowedRoots) is incompatible with
PluginManagerService; change it to reuse the same plugin-root helper/contract
used by PluginManagerService (or duplicate its logic) so the default paths match
(/tmp/soopa-plugins in dev and ~/.soopa/plugins in prod when PLUGINS_PATH is
unset), and ensure you only call fs.realpathSync on paths that exist (use
fs.existsSync or try/catch around realpathSync) and include the workspace
package roots fallback (the current process.cwd() packages/pieces/* entries)
when building allowedRoots; update references to pluginsPath, allowedRoots and
the code that computes realpathSync accordingly.
---
Outside diff comments:
In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts`:
- Around line 86-96: ensurePiece currently calls this.manager.install(...)
directly which skips the migration/publishing flow; change ensurePiece to
delegate to the existing installPath that triggers tenant migrations and the
TenantProvisionQueue event (e.g., call this.installPiece(packageName, version)
instead of this.manager.install, or if keeping manager.install then immediately
publish the TenantProvisionQueue event and invoke the migration dispatch used by
installPiece). Update references in ensurePiece (function ensurePiece) so newly
installed pieces go through the same migration/publish logic as installPiece and
avoid loading a piece before migrations run.
- Around line 56-65: The install is exposed before the migration job is durably
recorded; change the flow so the migration event is guaranteed or the install is
rolled back: either (preferred) persist/enqueue the PluginMigrationEvent via
queueService.send (or a durable DB record) before calling this.logger.log or
returning, or (if enqueue-before-install isn't possible) catch failures from
queueService.send and call this.manager.uninstall(packageName,
pluginInfo.version) to roll back the install (and only log/return on successful
enqueue); update the code around this.manager.install, PluginMigrationEvent
construction, and queueService.send to implement the chosen approach and ensure
errors are rethrown after rollback.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d098710-440a-41aa-b1e9-9b4cb5a290c1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/api/src/modules/plugins/plugins.controller.spec.tsapps/api/src/modules/plugins/plugins.controller.tsapps/api/src/modules/plugins/plugins.module.tspackages/pieces/platform/registry/package.jsonpackages/pieces/platform/registry/src/pieces/migration-worker.service.tspackages/pieces/platform/registry/src/pieces/piece-execution.worker.tspackages/pieces/platform/registry/src/pieces/piece-loader.service.tspackages/pieces/platform/registry/src/pieces/plugin-manager.service.tspackages/pieces/platform/registry/vitest.config.mts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 10 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 10 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/pieces/platform/registry/src/pieces/migration-worker.service.ts (1)
62-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
tenantIdin the runtime guard too.
consume()deliversunknown, and the queue contract saystenantIdisstring | undefined. Right now{ pluginLocation, pieceName, tenantId: 123 }passes the guard and enters the single-tenant path with an invalid identifier.Proposed fix
private isPluginMigrationEvent(event: any): event is PluginMigrationEvent { return ( typeof event === 'object' && event !== null && typeof event.pluginLocation === 'string' && - typeof event.pieceName === 'string' + typeof event.pieceName === 'string' && + (event.tenantId === undefined || typeof event.tenantId === 'string') ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts` around lines 62 - 68, The runtime type guard isPluginMigrationEvent currently doesn't validate tenantId, so malformed payloads like tenantId: 123 slip through; update isPluginMigrationEvent to also check that event.tenantId is either a string or undefined (i.e. typeof event.tenantId === 'string' || typeof event.tenantId === 'undefined') so consume()'s unknown payloads are rejected when tenantId is an invalid type; keep references to PluginMigrationEvent and the consume() entry point when adding the check.packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts (2)
23-29: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEliminate duplicated plugins path logic.
The plugins path computation is duplicated between the static
PLUGINS_PATHproperty (lines 14-18) and the constructor (lines 24-29). This violates DRY and creates a maintenance burden—if the logic needs to change, both locations must be updated identically.♻️ Refactor to use the static property
constructor( `@Inject`(QUEUE_SERVICE) private readonly queueService: IQueueService ) { - // Determine plugins path based on environment - const isDev = process.env.NODE_ENV === 'development' || process.env.DEV_MODE === 'true'; - const appDataDir = process.env.APP_DATA_DIR || (isDev ? process.cwd() : path.join(os.homedir(), '.soopa')); - - // Default to app-owned persistent directory, only use tmpdir in development - this.pluginsPath = process.env.PLUGINS_PATH || - (isDev ? path.join(os.tmpdir(), 'soopa-plugins') : path.join(appDataDir, 'plugins')); + this.pluginsPath = PluginManagerService.PLUGINS_PATH; this.logger.log(`Initializing Live Plugin Manager at: ${this.pluginsPath}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts` around lines 23 - 29, The plugins path logic is duplicated between the static PLUGINS_PATH property and the constructor; remove the duplication by having the constructor use the already-computed static PLUGINS_PATH instead of recomputing it. Update the constructor to assign this.pluginsPath = PluginManagerService.PLUGINS_PATH (or call a static getter if you prefer lazy evaluation), and delete the repeated env/path logic from the constructor so the single source of truth remains the static PLUGINS_PATH symbol.
34-40: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider async filesystem operations during module initialization.
The constructor uses synchronous filesystem operations (
existsSync,mkdirSync,chmodSync) which block the Node.js event loop during service instantiation. While this is generally acceptable in a constructor, for improved startup performance and non-blocking behavior, consider moving this logic to an asynconModuleInitlifecycle hook and using the async variants (fs.promises.mkdir,fs.promises.chmod).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts` around lines 34 - 40, Move the synchronous filesystem calls out of the constructor into an async onModuleInit lifecycle method: remove existsSync/mkdirSync/chmodSync usage in the constructor and implement async onModuleInit() (and implement OnModuleInit) that uses fs.promises (e.g., fs.promises.stat or fs.promises.access to check existence, fs.promises.mkdir(this.pluginsPath, { recursive: true, mode: 0o700 }) to create, and fs.promises.chmod(this.pluginsPath, 0o700) to apply permissions) and await these operations; preserve the logger calls (logger.log) and add try/catch to log and rethrow or handle errors as appropriate so startup errors are surfaced.packages/pieces/platform/registry/src/pieces/plugin-manager.service.spec.ts (1)
94-107: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrevent test pollution from environment variable manipulation.
Tests directly mutate
process.env.ENABLE_PLUGIN_MIGRATIONS(lines 95, 106, 110, 128, 139) and delete it inline. If a test throws an exception before reaching the cleanupdeletestatement, the environment variable remains set and can pollute subsequent tests, especially in parallel execution.♻️ Centralize env var cleanup in lifecycle hooks
describe('PluginManagerService', () => { let service: PluginManagerService; let queueService: Mocked<IQueueService>; + let originalEnableMigrations: string | undefined; beforeEach(() => { + originalEnableMigrations = process.env.ENABLE_PLUGIN_MIGRATIONS; queueService = { send: vi.fn(), consume: vi.fn(), } as unknown as Mocked<IQueueService>; service = new PluginManagerService(queueService); }); afterEach(() => { + if (originalEnableMigrations === undefined) { + delete process.env.ENABLE_PLUGIN_MIGRATIONS; + } else { + process.env.ENABLE_PLUGIN_MIGRATIONS = originalEnableMigrations; + } vi.clearAllMocks(); }); // In tests, set the env var without manual cleanup: it('should install piece and immediately dispatch a migration event to SQS', async () => { process.env.ENABLE_PLUGIN_MIGRATIONS = 'true'; // ... test body ... - delete process.env.ENABLE_PLUGIN_MIGRATIONS; });Also applies to: 109-118, 127-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.spec.ts` around lines 94 - 107, Tests mutate process.env.ENABLE_PLUGIN_MIGRATIONS inline which can leak state if a test throws; instead, capture the original value and restore it in a test lifecycle hook: store const originalEnablePluginMigrations = process.env.ENABLE_PLUGIN_MIGRATIONS in a beforeEach (or at top of the describe) and in an afterEach reset process.env.ENABLE_PLUGIN_MIGRATIONS = originalEnablePluginMigrations (or delete it if originally undefined); update tests that call service.installPiece and spyOn (e.g., the test using (service as any).manager.install and QueueName.TenantProvisionQueue assertions) to remove inline delete statements so environment cleanup is centralized and robust.apps/api/src/modules/plugins/plugins.controller.ts (1)
70-73:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix NPM webhook HMAC to hash the raw request body bytes (not
JSON.stringify(payload))
apps/api/src/modules/plugins/plugins.controller.tsstill computes the digest overJSON.stringify(payload)(the Nest-parsed DTO) at lines 70–73. npm signatures are generated over the raw, unmodified HTTP body bytes, so re-serialization can change the byte sequence and cause legitimate requests to fail auth.
apps/api/src/main.tsalready enables raw-body capture (rawBody: true), so the controller should inject it (e.g.,@RawBody() rawBody: Buffer) and usehmac.update(rawBody)instead. Updateplugins.controller.spec.tsaccordingly (it currently signsJSON.stringify(payload), masking the mismatch with real npm webhook verification).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/plugins/plugins.controller.ts` around lines 70 - 73, The webhook HMAC is being computed over JSON.stringify(payload) instead of the raw HTTP body bytes; update the webhook handler in plugins.controller.ts to accept the raw body (inject the raw Buffer via `@RawBody`() rawBody: Buffer or the project's RawBody decorator) and compute the digest using hmac.update(rawBody) rather than the parsed DTO, and then verify against the signature header; also update plugins.controller.spec.ts to sign the exact rawBody Buffer used in tests (not JSON.stringify(payload)) so the test matches real npm webhook signing.
♻️ Duplicate comments (3)
packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts (1)
1-3:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBare
require('node:os')reintroduces an ESM crash here.Line 33 and Line 34 use
require(...)before the scoped require is created. In this ESM worker, that throwsReferenceError: require is not definedon every execution.Proposed fix
+import * as os from 'node:os'; import * as path from 'node:path'; import * as fs from 'node:fs'; import { createRequire } from 'node:module'; @@ - const appDataDir = process.env.APP_DATA_DIR || (isDev ? process.cwd() : path.join(require('node:os').homedir(), '.soopa')); - const pluginsPath = process.env.PLUGINS_PATH || (isDev ? path.join(require('node:os').tmpdir(), 'soopa-plugins') : path.join(appDataDir, 'plugins')); + const appDataDir = process.env.APP_DATA_DIR || (isDev ? process.cwd() : path.join(os.homedir(), '.soopa')); + const pluginsPath = process.env.PLUGINS_PATH || (isDev ? path.join(os.tmpdir(), 'soopa-plugins') : path.join(appDataDir, 'plugins'));#!/bin/bash set -euo pipefail echo "== registry package module type ==" jq -r '.type // "(none)"' packages/pieces/platform/registry/package.json echo echo "== worker import style ==" sed -n '1,8p' packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts echo echo "== bare require usage in the changed block ==" nl -ba packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts | sed -n '31,36p'Also applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts` around lines 1 - 3, The code currently calls bare require('node:os') before a scoped require exists, causing ReferenceError in this ESM worker; fix by removing the bare require and either (a) use an ES import (e.g. import * as os from 'node:os') at the top, or (b) create a scoped require via createRequire(import.meta.url) (e.g. const requireScoped = createRequire(import.meta.url)) before any require calls and replace require('node:os') with requireScoped('node:os'); update the occurrences that reference the OS module accordingly (look for the require usage around the piece-execution.worker code and the existing createRequire import).packages/pieces/platform/registry/src/pieces/migration-worker.service.ts (1)
21-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
areTenantHandlersAvailable()still enables a guaranteed-failing consumer.This check only mirrors
ENABLE_PLUGIN_MIGRATIONS, but Line 120 and Line 128 still throw unconditionally. Combined withpackages/pieces/platform/registry/src/pieces/plugin-manager.service.ts:53-79, setting the flag registers the consumer and starts enqueueing jobs that can only fail. Keep this path hard-disabled until real tenant lookup/DB resolution is injected, or make the readiness check reflect concrete implementations instead of the env flag.Also applies to: 55-57, 119-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts` around lines 21 - 29, The current readiness check (areTenantHandlersAvailable) only mirrors ENABLE_PLUGIN_MIGRATIONS and still leads to a guaranteed-failing consumer because code later (in onModuleInit and the consumer start/enqueue paths) throws unconditionally when tenant handlers are not truly implemented; change the readiness logic to detect real implementations (e.g., verify injected handlers are not the default stubs by checking the actual methods/closures for getActiveTenants and getTenantDbConnection on the injected service) and return early from onModuleInit so the migration consumer is not registered or started unless those concrete handlers exist, and remove or guard the unconditional throws in the tenant-resolution/enqueue code paths so plugin-manager.service does not register/enqueue jobs when tenant handlers are absent.apps/api/src/modules/plugins/plugins.controller.spec.ts (1)
112-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest signature validation masks the controller's raw body bug.
The test helper
generateValidSignaturecorrectly computes HMAC over arawBodystring parameter. However, the tests create this raw body viaJSON.stringify(payload)and then pass both the computed signature and the samepayloadobject to the controller. The controller then re-serializes the payload withJSON.stringify(payload).In these simple test cases, both JSON serializations produce identical output, so the signatures match. This masks the controller bug where real webhook payloads (serialized by the NPM registry) will not match the controller's re-serialization.
Once the controller is fixed to use the raw request body for HMAC validation, these tests will need to be updated to pass the raw body string to the controller method and verify that the signature validation actually compares against that raw body.
This issue is a consequence of the critical controller bug flagged in
plugins.controller.tsat lines 70-73.Also applies to: 129-130, 146-147, 177-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/plugins/plugins.controller.spec.ts` around lines 112 - 113, The test currently masks the controller bug by computing signature over rawBody (const rawBody = JSON.stringify(payload); const signature = generateValidSignature(rawBody)) but then passing the payload object into the controller; update the spec to pass the rawBody string into the controller webhook handler and/or set the request to expose the raw body string (so the handler validates HMAC against the exact rawBody), i.e. use generateValidSignature(rawBody) and supply rawBody as the incoming request body (instead of payload) when invoking the controller method under test; also update the other occurrences mentioned (lines near 129-130, 146-147, 177-178) so all tests feed the exact rawBody string to the controller rather than a re-serialized object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/pieces/platform/registry/src/pieces/migration-worker.service.spec.ts`:
- Around line 34-41: Save the original process.env.ENABLE_PLUGIN_MIGRATIONS in
the beforeEach hook (e.g., const originalEnablePluginMigrations =
process.env.ENABLE_PLUGIN_MIGRATIONS) before setting it to 'true', and in
afterEach restore it (set process.env.ENABLE_PLUGIN_MIGRATIONS =
originalEnablePluginMigrations) or delete it only if the original was undefined;
update the beforeEach/afterEach around the existing hooks in
migration-worker.service.spec.ts to use these names so existing tests still
enable migrations but the original environment value is preserved.
In `@packages/queue/src/constants.ts`:
- Line 15: The init-localstack.sh script's QUEUES array is missing the queue
defined by PluginInstallQueue (and its DLQ PluginInstallQueueDLQ), so update the
QUEUES=(...) list in scripts/init-localstack.sh to include
"plugin-install-queue" (the DLQ name will be auto-derived as
"plugin-install-queue-dlq"); ensure the exact string matches the constant
PluginInstallQueue from packages/queue/src/constants.ts so the queue and its DLQ
are created during LocalStack initialization.
---
Outside diff comments:
In `@apps/api/src/modules/plugins/plugins.controller.ts`:
- Around line 70-73: The webhook HMAC is being computed over
JSON.stringify(payload) instead of the raw HTTP body bytes; update the webhook
handler in plugins.controller.ts to accept the raw body (inject the raw Buffer
via `@RawBody`() rawBody: Buffer or the project's RawBody decorator) and compute
the digest using hmac.update(rawBody) rather than the parsed DTO, and then
verify against the signature header; also update plugins.controller.spec.ts to
sign the exact rawBody Buffer used in tests (not JSON.stringify(payload)) so the
test matches real npm webhook signing.
In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts`:
- Around line 62-68: The runtime type guard isPluginMigrationEvent currently
doesn't validate tenantId, so malformed payloads like tenantId: 123 slip
through; update isPluginMigrationEvent to also check that event.tenantId is
either a string or undefined (i.e. typeof event.tenantId === 'string' || typeof
event.tenantId === 'undefined') so consume()'s unknown payloads are rejected
when tenantId is an invalid type; keep references to PluginMigrationEvent and
the consume() entry point when adding the check.
In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.spec.ts`:
- Around line 94-107: Tests mutate process.env.ENABLE_PLUGIN_MIGRATIONS inline
which can leak state if a test throws; instead, capture the original value and
restore it in a test lifecycle hook: store const originalEnablePluginMigrations
= process.env.ENABLE_PLUGIN_MIGRATIONS in a beforeEach (or at top of the
describe) and in an afterEach reset process.env.ENABLE_PLUGIN_MIGRATIONS =
originalEnablePluginMigrations (or delete it if originally undefined); update
tests that call service.installPiece and spyOn (e.g., the test using (service as
any).manager.install and QueueName.TenantProvisionQueue assertions) to remove
inline delete statements so environment cleanup is centralized and robust.
In `@packages/pieces/platform/registry/src/pieces/plugin-manager.service.ts`:
- Around line 23-29: The plugins path logic is duplicated between the static
PLUGINS_PATH property and the constructor; remove the duplication by having the
constructor use the already-computed static PLUGINS_PATH instead of recomputing
it. Update the constructor to assign this.pluginsPath =
PluginManagerService.PLUGINS_PATH (or call a static getter if you prefer lazy
evaluation), and delete the repeated env/path logic from the constructor so the
single source of truth remains the static PLUGINS_PATH symbol.
- Around line 34-40: Move the synchronous filesystem calls out of the
constructor into an async onModuleInit lifecycle method: remove
existsSync/mkdirSync/chmodSync usage in the constructor and implement async
onModuleInit() (and implement OnModuleInit) that uses fs.promises (e.g.,
fs.promises.stat or fs.promises.access to check existence,
fs.promises.mkdir(this.pluginsPath, { recursive: true, mode: 0o700 }) to create,
and fs.promises.chmod(this.pluginsPath, 0o700) to apply permissions) and await
these operations; preserve the logger calls (logger.log) and add try/catch to
log and rethrow or handle errors as appropriate so startup errors are surfaced.
---
Duplicate comments:
In `@apps/api/src/modules/plugins/plugins.controller.spec.ts`:
- Around line 112-113: The test currently masks the controller bug by computing
signature over rawBody (const rawBody = JSON.stringify(payload); const signature
= generateValidSignature(rawBody)) but then passing the payload object into the
controller; update the spec to pass the rawBody string into the controller
webhook handler and/or set the request to expose the raw body string (so the
handler validates HMAC against the exact rawBody), i.e. use
generateValidSignature(rawBody) and supply rawBody as the incoming request body
(instead of payload) when invoking the controller method under test; also update
the other occurrences mentioned (lines near 129-130, 146-147, 177-178) so all
tests feed the exact rawBody string to the controller rather than a
re-serialized object.
In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts`:
- Around line 21-29: The current readiness check (areTenantHandlersAvailable)
only mirrors ENABLE_PLUGIN_MIGRATIONS and still leads to a guaranteed-failing
consumer because code later (in onModuleInit and the consumer start/enqueue
paths) throws unconditionally when tenant handlers are not truly implemented;
change the readiness logic to detect real implementations (e.g., verify injected
handlers are not the default stubs by checking the actual methods/closures for
getActiveTenants and getTenantDbConnection on the injected service) and return
early from onModuleInit so the migration consumer is not registered or started
unless those concrete handlers exist, and remove or guard the unconditional
throws in the tenant-resolution/enqueue code paths so plugin-manager.service
does not register/enqueue jobs when tenant handlers are absent.
In `@packages/pieces/platform/registry/src/pieces/piece-execution.worker.ts`:
- Around line 1-3: The code currently calls bare require('node:os') before a
scoped require exists, causing ReferenceError in this ESM worker; fix by
removing the bare require and either (a) use an ES import (e.g. import * as os
from 'node:os') at the top, or (b) create a scoped require via
createRequire(import.meta.url) (e.g. const requireScoped =
createRequire(import.meta.url)) before any require calls and replace
require('node:os') with requireScoped('node:os'); update the occurrences that
reference the OS module accordingly (look for the require usage around the
piece-execution.worker code and the existing createRequire import).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e734aed-e5b6-4974-8c71-15ce3480edfd
📒 Files selected for processing (12)
apps/api/src/modules/plugins/plugins.controller.spec.tsapps/api/src/modules/plugins/plugins.controller.tsapps/api/src/modules/plugins/plugins.module.tspackages/pieces/platform/registry/src/index.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.spec.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.tspackages/pieces/platform/registry/src/pieces/piece-execution.worker.tspackages/pieces/platform/registry/src/pieces/plugin-manager.service.spec.tspackages/pieces/platform/registry/src/pieces/plugin-manager.service.tspackages/queue/src/constants.tspackages/queue/src/events/plugin-install.event.tspackages/queue/src/index.ts
This was referenced Jun 9, 2026
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
1 participant
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 by CodeRabbit
New Features
Tests