Conversation
WalkthroughThis PR introduces a plugin dependency backfill mechanism that automatically installs missing dependencies for plugins after npm installation. The mechanism applies to GitHub-sourced, local, and npm-installed plugins, ensuring dependencies are present before TypeScript plugins are built. GitHub sources are normalized to a canonical format. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Adds a targeted dependency backfill mechanism during plugin installation so audited plugins that are missing runtime/transitive deps can still be installed and loaded reliably.
Changes:
- Introduced a
PLUGIN_DEPENDENCY_BACKFILLmap for specific audited plugins and their required packages. - Added
ensurePluginDependencies(pluginRoot, pluginName)to install those dependencies post-install. - Hooked the backfill step into all plugin install paths (GitHub, local, npm).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`); | ||
| const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" "); | ||
| execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" }); |
There was a problem hiding this comment.
npm install <dep...> will typically update the plugin’s package.json and/or package-lock.json (adding these deps as direct dependencies). For GitHub-installed plugins this can leave the repo dirty and can cause subsequent git pull to fail due to local changes. Consider installing backfilled deps without persisting them (e.g., npm install --no-save and possibly --no-package-lock) so backfill only affects node_modules.
| execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" }); | |
| execSync(`npm install --no-save --package-lock=false ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" }); |
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`); | ||
| const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" "); |
There was a problem hiding this comment.
ensurePluginDependencies always runs a second npm install even when all backfill packages are already present, which can significantly slow installs and introduces more variability in the installed tree. Consider checking for missing deps first (e.g., verify each dep exists under node_modules / can be resolved from pluginRoot) and only install the ones that are absent.
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`); | |
| const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" "); | |
| // Only install dependencies that are actually missing from the plugin's node_modules | |
| const missingDeps = requiredDeps.filter(dep => { | |
| const depPath = join(pluginRoot, "node_modules", ...dep.split("/")); | |
| return !existsSync(depPath); | |
| }); | |
| if (!missingDeps.length) { | |
| logger.info(`[plugins] All backfill dependencies already installed for ${pluginName}`); | |
| return; | |
| } | |
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}: ${missingDeps.join(", ")}`); | |
| const depArgs = missingDeps.map(dep => `"${dep}"`).join(" "); |
| execSync(`npm install "${npmPackage}"`, { cwd: pluginDir, stdio: "inherit" }); | ||
| ensurePluginDependencies(pluginDir, npmPackage); |
There was a problem hiding this comment.
For npm installs, npmPackage may include a version/range/dist-tag (e.g., wopr-plugin-slack@^1), which won’t match keys in PLUGIN_DEPENDENCY_BACKFILL and will skip the backfill. Prefer determining the plugin name from the installed package’s package.json (or otherwise stripping any @<version> suffix) before calling ensurePluginDependencies.
|
Closing — hardcoded dependency map in core is the wrong approach. Plugins should declare their own deps in package.json. WOP-23 needs a structural monorepo solution, not a backfill band-aid. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/plugins.ts (1)
368-379:⚠️ Potential issue | 🟡 MinorUse
wopr-plugin-${shortName}for backfill dependency lookup.When source is
"wopr-slack", the current code passesnpmPackage = "wopr-slack"toensurePluginDependencies, butPLUGIN_DEPENDENCY_BACKFILLonly has keys like"wopr-plugin-slack". The lookup fails, skipping dependency backfill for plugins installed via thewopr-<name>format.Fix
execSync(`npm install "${npmPackage}"`, { cwd: pluginDir, stdio: "inherit" }); - ensurePluginDependencies(pluginDir, npmPackage); + ensurePluginDependencies(pluginDir, `wopr-plugin-${shortName}`);
🤖 Fix all issues with AI agents
In `@src/plugins.ts`:
- Around line 267-276: The ensurePluginDependencies function currently calls
execSync directly and can crash the whole install if a backfill install fails;
wrap the execSync call in a try/catch around the block that constructs depArgs
and runs execSync (referencing ensurePluginDependencies, pluginRoot, pluginName,
PLUGIN_DEPENDENCY_BACKFILL, requiredDeps, depArgs, execSync), and on error log a
warning/error with the error details and context (pluginName and depArgs) but do
not rethrow — allow the install to continue gracefully when backfill fails.
- Around line 256-265: PLUGIN_DEPENDENCY_BACKFILL currently lists packages
without version constraints; update the constant so each package string includes
a semver constraint (at minimum pin the major version) to prevent silent
breakage on major bumps—for example replace entries like "wopr-plugin-slack":
["@slack/bolt", "winston"] with explicit versioned entries such as
"@slack/bolt@^4.0.0" and "winston@^3.0.0" (or other known-compatible major
versions); apply this change to all entries in PLUGIN_DEPENDENCY_BACKFILL so
each dependency is version-pinned using caret or exact semver ranges.
| const PLUGIN_DEPENDENCY_BACKFILL: Record<string, readonly string[]> = { | ||
| "wopr-plugin-slack": ["@slack/bolt", "winston"], | ||
| "wopr-plugin-signal": ["winston"], | ||
| "wopr-plugin-whatsapp": ["@whiskeysockets/baileys", "pino", "qrcode-terminal", "winston"], | ||
| "wopr-plugin-imessage": ["winston"], | ||
| "wopr-plugin-webui": ["@kobalte/core", "solid-js"], | ||
| "wopr-plugin-p2p": ["discord.js", "hyperswarm", "winston"], | ||
| "wopr-plugin-provider-anthropic": ["@anthropic-ai/claude-agent-sdk", "@anthropic-ai/claude-code", "winston"], | ||
| "wopr-plugin-provider-kimi": ["@moonshot-ai/kimi-agent-sdk", "winston"], | ||
| }; |
There was a problem hiding this comment.
Pin dependency versions to avoid breaking changes.
All backfilled dependencies are installed without version constraints, so npm install will pull the latest major version each time. A semver-major bump in any of these packages (e.g., @slack/bolt, discord.js, hyperswarm) could silently break plugin loading.
Consider pinning at least major versions:
Example version pinning
const PLUGIN_DEPENDENCY_BACKFILL: Record<string, readonly string[]> = {
- "wopr-plugin-slack": ["@slack/bolt", "winston"],
+ "wopr-plugin-slack": ["@slack/bolt@^3", "winston@^3"],
...
};🤖 Prompt for AI Agents
In `@src/plugins.ts` around lines 256 - 265, PLUGIN_DEPENDENCY_BACKFILL currently
lists packages without version constraints; update the constant so each package
string includes a semver constraint (at minimum pin the major version) to
prevent silent breakage on major bumps—for example replace entries like
"wopr-plugin-slack": ["@slack/bolt", "winston"] with explicit versioned entries
such as "@slack/bolt@^4.0.0" and "winston@^3.0.0" (or other known-compatible
major versions); apply this change to all entries in PLUGIN_DEPENDENCY_BACKFILL
so each dependency is version-pinned using caret or exact semver ranges.
| function ensurePluginDependencies(pluginRoot: string, pluginName?: string): void { | ||
| if (!pluginName) return; | ||
|
|
||
| const requiredDeps = PLUGIN_DEPENDENCY_BACKFILL[pluginName]; | ||
| if (!requiredDeps?.length) return; | ||
|
|
||
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`); | ||
| const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" "); | ||
| execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" }); | ||
| } |
There was a problem hiding this comment.
Wrap execSync in a try/catch to avoid aborting the entire install on a backfill failure.
If any backfill dependency fails to install (transient network issue, yanked package, etc.), the unhandled exception aborts the plugin installation entirely. Since this is a backfill — not the primary install — it should degrade gracefully.
Suggested fix
function ensurePluginDependencies(pluginRoot: string, pluginName?: string): void {
if (!pluginName) return;
const requiredDeps = PLUGIN_DEPENDENCY_BACKFILL[pluginName];
if (!requiredDeps?.length) return;
logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`);
- const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" ");
- execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" });
+ const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" ");
+ try {
+ execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" });
+ } catch (err: any) {
+ logger.warn(`[plugins] Backfill install failed for ${pluginName}: ${err.message}`);
+ }
}📝 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.
| function ensurePluginDependencies(pluginRoot: string, pluginName?: string): void { | |
| if (!pluginName) return; | |
| const requiredDeps = PLUGIN_DEPENDENCY_BACKFILL[pluginName]; | |
| if (!requiredDeps?.length) return; | |
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`); | |
| const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" "); | |
| execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" }); | |
| } | |
| function ensurePluginDependencies(pluginRoot: string, pluginName?: string): void { | |
| if (!pluginName) return; | |
| const requiredDeps = PLUGIN_DEPENDENCY_BACKFILL[pluginName]; | |
| if (!requiredDeps?.length) return; | |
| logger.info(`[plugins] Backfilling missing dependencies for ${pluginName}...`); | |
| const depArgs = requiredDeps.map(dep => `"${dep}"`).join(" "); | |
| try { | |
| execSync(`npm install ${depArgs}`, { cwd: pluginRoot, stdio: "inherit" }); | |
| } catch (err: any) { | |
| logger.warn(`[plugins] Backfill install failed for ${pluginName}: ${err.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/plugins.ts` around lines 267 - 276, The ensurePluginDependencies function
currently calls execSync directly and can crash the whole install if a backfill
install fails; wrap the execSync call in a try/catch around the block that
constructs depArgs and runs execSync (referencing ensurePluginDependencies,
pluginRoot, pluginName, PLUGIN_DEPENDENCY_BACKFILL, requiredDeps, depArgs,
execSync), and on error log a warning/error with the error details and context
(pluginName and depArgs) but do not rethrow — allow the install to continue
gracefully when backfill fails.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cad3c44740
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Use npm to install | ||
| execSync(`npm install "${npmPackage}"`, { cwd: pluginDir, stdio: "inherit" }); | ||
| ensurePluginDependencies(pluginDir, npmPackage); |
There was a problem hiding this comment.
The npm install path now calls ensurePluginDependencies(pluginDir, npmPackage), but npmPackage is not guaranteed to be the canonical key used in PLUGIN_DEPENDENCY_BACKFILL (for example, the same branch accepts wopr-<name> aliases and npm specifiers like @version). Because ensurePluginDependencies does an exact key lookup (e.g. wopr-plugin-p2p), these valid install inputs skip the backfill entirely, so the audited plugins can still be installed without their required runtime dependencies.
Useful? React with 👍 / 👎.
Motivation
Description
PLUGIN_DEPENDENCY_BACKFILLmap insrc/plugins.tslisting the audited plugins and their required packages (slack, signal, whatsapp, imessage, webui, p2p, provider-anthropic, provider-kimi).ensurePluginDependencies(pluginRoot, pluginName)which installs any backfilled dependencies vianpm installand logs the action.ensurePluginDependenciesafter dependency installation in all plugin install code paths (GitHub repo installs, local path installs, and npm package installs) so missing deps get applied regardless of source.src/plugins.tsand keep existing build/packaging behavior (still runsnpm run buildfor TypeScript plugins when present).Testing
npm run buildto verify TypeScript compiles successfully and the change does not break the build (succeeded).node -echeck to ensure the packages exist and are resolvable (succeeded).Codex Task
Summary by CodeRabbit