fix(plugins): localize bundled runtime deps to extensions#67099
fix(plugins): localize bundled runtime deps to extensions#67099vincentkoc merged 10 commits intomainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49e3c0b054
ℹ️ 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".
49e3c0b to
2ff3f19
Compare
Greptile SummaryThis PR isolates staged bundled plugin builds into
Confidence Score: 4/5Safe to merge for current plugins, but has a latent logic bug in the optional-only deps path of the new install refactor. One P1 finding: the node_modules existence guard fires unconditionally even when only optional specs were attempted and swallowed — contradicting the comment at line 737. No current staged plugin has only optional runtime deps (all have at least one dependencies entry), so the bug is latent today but would manifest if any future plugin or a current plugin update lands in that state. scripts/stage-bundled-plugin-runtime-deps.mjs lines 741–746 Prompt To Fix All With AIThis is a comment left during a code review.
Path: scripts/stage-bundled-plugin-runtime-deps.mjs
Line: 741-746
Comment:
**Optional-only deps can still block staging**
When a plugin has only `optionalDependencies` (no `dependencies`) and `stageRuntimeDependencies: true`, the required-install block is skipped entirely, the optional install silently swallows failures (line 736–738), and then this check fires — throwing an error even though the intent is that optional deps must not block staging.
```suggestion
const stagedNodeModulesDir = path.join(tempInstallDir, "node_modules");
if (requiredSpecs.length > 0 && !fs.existsSync(stagedNodeModulesDir)) {
throw new Error(
`failed to stage bundled runtime deps for ${pluginId}: explicit npm install produced no node_modules directory`,
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/qqbot/src/slash-commands.ts
Line: 17-43
Comment:
**`readPluginVersion` duplicated within qqbot**
The same `PACKAGE_JSON_CANDIDATES` + `readPluginVersion()` helper is defined independently in both `extensions/qqbot/src/api.ts` and here. Since both files are in the same package, extracting it to a shared local helper (e.g. `utils/plugin-version.ts`) would eliminate the duplication. The same pattern was also necessary in `extensions/feishu/src/client.ts`, but that's in a different package.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "Merge branch 'main' into pr-65478-securi..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4631cb8117
ℹ️ 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".
🔒 Aisle Security AnalysisWe found 4 potential security issue(s) in this PR:
1. 🟠 Symlink traversal allows arbitrary filesystem delete/overwrite during runtime deps staging
DescriptionThe runtime dependency staging script operates on plugin directories discovered under
If an attacker can supply a workspace where Vulnerable flow:
RecommendationHarden path handling by ensuring the entire directory chain is within the expected repo and contains no symlink/junction components before any destructive operation. Recommended approach:
const distRoot = path.join(repoRoot, "dist");
const extensionsRoot = path.join(distRoot, "extensions");
const realExtensionsRoot = fs.realpathSync(extensionsRoot);
const realRepoRoot = fs.realpathSync(repoRoot);
if (!realExtensionsRoot.startsWith(realRepoRoot + path.sep)) {
throw new Error(`extensionsRoot escapes repo via symlink: ${extensionsRoot}`);
}
const realPluginDir = fs.realpathSync(pluginDir);
if (!realPluginDir.startsWith(realExtensionsRoot + path.sep)) throw new Error("...");This prevents a symlinked 2. 🟠 Supply-chain exposure: published package omits bundled extension node_modules causing postinstall to fetch & execute unpinned deps
DescriptionThe published On install,
Relevant changed packaging line: "files": [
...,
"!dist/extensions/*/node_modules/**",
...
]Postinstall sink (for context): npmArgs: ["install", "--omit=dev", "--no-save", "--package-lock=false", "--legacy-peer-deps", ...missingSpecs]RecommendationAvoid dynamic network installs during end-user Options:
Example hardening (in npmArgs: [
"install",
"--omit=dev",
"--no-save",
"--package-lock=false",
"--ignore-scripts",
"--silent",
...missingSpecsPinnedToExactVersions
]Also validate that 3. 🟡 Symlink-based workspace root confusion allows staging runtime deps from outside repository
Description
If
Vulnerable code: installedWorkspaceRoot = path.dirname(fs.realpathSync(nodeModulesDir));
...
const installedPluginRoot = path.join(installedWorkspaceRoot, "extensions", pluginId);
if (fs.existsSync(path.join(installedPluginRoot, "package.json"))) {
return installedPluginRoot;
}RecommendationConstrain the resolved workspace root to the current repository and/or refuse symlinked Suggested hardening: const nodeModulesDir = path.join(repoRoot, "node_modules");
// Refuse symlinked node_modules at the repo root.
if (fs.existsSync(nodeModulesDir) && fs.lstatSync(nodeModulesDir).isSymbolicLink()) {
return currentPluginRoot;
}
const realNodeModules = fs.realpathSync(nodeModulesDir);
const installedWorkspaceRoot = path.dirname(realNodeModules);
const realRepoRoot = fs.realpathSync(repoRoot);
// Ensure installedWorkspaceRoot is the repoRoot (or within it, if desired)
if (installedWorkspaceRoot !== realRepoRoot) {
return currentPluginRoot;
}Additionally, when using 4. 🔵 DoS via invalid npm dependency specifiers not rejected in runtime deps pinning
DescriptionThe fallback runtime dependency pinning logic allows certain npm dependency specifiers that are not semver ranges (e.g., npm alias
Vulnerable code paths: function isSafeRuntimeDependencySpec(spec) {
// ... does not reject `npm:`, `github:`, `latest`, etc.
}
function dependencyVersionSatisfied(spec, installedVersion) {
return semverSatisfies(installedVersion, spec, { includePrerelease: false });
}RecommendationTreat non-semver specs as invalid before calling Suggested hardening: import semverValidRange from "semver/ranges/valid.js";
function assertSafeRuntimeDependencySpec(depName, spec) {
if (!isSafeRuntimeDependencySpec(spec)) {
throw new Error(`disallowed runtime dependency spec for ${depName}: ${spec}`);
}
// Reject npm alias / non-semver specifiers
if (spec.startsWith("npm:")) {
throw new Error(`disallowed npm alias spec for ${depName}: ${spec}`);
}
if (!semverValidRange(spec)) {
throw new Error(`runtime dependency spec must be a semver range for ${depName}: ${spec}`);
}
}
function dependencyVersionSatisfied(spec, installedVersion) {
try {
return semverSatisfies(installedVersion, spec, { includePrerelease: false });
} catch {
return false;
}
}This prevents unexpected exceptions and ensures the pinning logic only operates on semver ranges it can interpret safely. Analyzed PR: #67099 at commit Last updated on: 2026-04-15T10:57:28Z |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc4d0b8ac8
ℹ️ 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".
| optionalDependencies: resolvePinnedRuntimeDependencyGroup( | ||
| runtimeGroups.optionalDependencies, | ||
| params, |
There was a problem hiding this comment.
Skip unresolved optional runtime deps when pinning
This now routes optionalDependencies through the same exact-version resolver as required deps, so staging fails whenever an optional package is not present in the root install graph. In practice, optional native deps (for example @discordjs/opus) are often skipped on unsupported hosts or when native build prerequisites are missing; with this code path, stageBundledPluginRuntimeDeps throws and aborts runtime-postbuild instead of proceeding without that optional package.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7b3b2880
ℹ️ 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".
| const IMPORT_PATTERNS = [ | ||
| /\bfrom\s*["']([^"']+)["']/g, | ||
| /\bimport\s*\(\s*["']([^"']+)["']\s*\)/g, | ||
| /\brequire\s*\(\s*["']([^"']+)["']\s*\)/g, | ||
| /\b(?:require|[_$A-Za-z][\w$]*require[\w$]*)\.resolve\s*\(\s*["']([^"']+)["']\s*\)/gi, |
There was a problem hiding this comment.
Match side-effect imports in dependency ownership scan
collectModuleSpecifiers only matches from, dynamic import(...), and require(...), so bare side-effect imports like import "pkg"; are never counted. This makes collectRootDependencyOwnershipAudit underreport real usage and can misclassify required root deps as unreferenced/localizable, which is risky because this script is used to drive dependency-removal decisions. Add a static bare-import pattern (as used in scripts/lib/bundled-plugin-root-runtime-mirrors.mjs) so these imports are included.
Useful? React with 👍 / 👎.
…7099) * fix(plugins): localize bundled runtime deps to extensions * fix(plugins): move staged runtime deps out of root * fix(packaging): harden prepack and runtime dep staging * fix(packaging): preserve optional runtime dep staging * Update CHANGELOG.md * fix(packaging): harden runtime staging filesystem writes * fix(docker): ship preinstall warning in bootstrap layers * fix(packaging): exclude staged plugin node_modules from npm pack
…7099) * fix(plugins): localize bundled runtime deps to extensions * fix(plugins): move staged runtime deps out of root * fix(packaging): harden prepack and runtime dep staging * fix(packaging): preserve optional runtime dep staging * Update CHANGELOG.md * fix(packaging): harden runtime staging filesystem writes * fix(docker): ship preinstall warning in bootstrap layers * fix(packaging): exclude staged plugin node_modules from npm pack
…tall When `openclaw update` runs `openclaw doctor` after replacing the package files, bundled plugin runtime deps (grammy etc.) haven't been installed yet — `ensureBundledPluginRuntimeDeps` only runs later during plugin loader initialisation. The static `import * as grammy from 'grammy'` at the top of allowed-updates.ts is pulled in transitively before the installer has a chance to run: doctor → channel-doctor → bundled channel registry → telegram/channel.ts → telegram/monitor.ts → telegram/allowed-updates.ts ← static ESM import grammy 💥 The fix replaces the top-level import with a lazy `createRequire()` call so grammy is only resolved when `resolveTelegramAllowedUpdates()` / the module constant is first evaluated, by which point the installer has already placed grammy under dist/extensions/telegram/node_modules/. Fallback constants (already present) ensure correct behaviour when grammy is genuinely absent. Reproduces with: rm -rf dist/extensions/telegram/node_modules openclaw doctor --non-interactive # → Error: Cannot find module 'grammy' Fixes the regression introduced by openclaw#67099 (localize bundled plugin runtime deps).
…7099) * fix(plugins): localize bundled runtime deps to extensions * fix(plugins): move staged runtime deps out of root * fix(packaging): harden prepack and runtime dep staging * fix(packaging): preserve optional runtime dep staging * Update CHANGELOG.md * fix(packaging): harden runtime staging filesystem writes * fix(docker): ship preinstall warning in bootstrap layers * fix(packaging): exclude staged plugin node_modules from npm pack
…7099) * fix(plugins): localize bundled runtime deps to extensions * fix(plugins): move staged runtime deps out of root * fix(packaging): harden prepack and runtime dep staging * fix(packaging): preserve optional runtime dep staging * Update CHANGELOG.md * fix(packaging): harden runtime staging filesystem writes * fix(docker): ship preinstall warning in bootstrap layers * fix(packaging): exclude staged plugin node_modules from npm pack
…7099) * fix(plugins): localize bundled runtime deps to extensions * fix(plugins): move staged runtime deps out of root * fix(packaging): harden prepack and runtime dep staging * fix(packaging): preserve optional runtime dep staging * Update CHANGELOG.md * fix(packaging): harden runtime staging filesystem writes * fix(docker): ship preinstall warning in bootstrap layers * fix(packaging): exclude staged plugin node_modules from npm pack
Summary
Describe the problem and fix in 2–5 bullets:
stageRuntimeDependencieswere still effectively root-owned because rootdist/chunks imported extension runtime deps like@buape/carbon, and staged plugin package scopes broke some plugin-local runtime loading paths.npm install/packaging stayed heavier than necessary, extension dependency ownership was wrong, and bundled plugin runtime isolation was brittle.dist/extensions/<id>graphs, Discord-owned deps moved out of the root package, plugin-local package version lookups were hardened for staged layouts, and the plugin loader now keepsdist/extensions/*modules on the aliased Jiti path soopenclaw/plugin-sdk/*still resolves inside nested plugin package scopes.pi-embedded-runnerworktree changes were left untouched.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
dist/emitted extension-owned chunks and root dependency ownership could not be reduced safely. Once plugins were split into isolated staged graphs, two hidden assumptions surfaced: some plugin code assumed../package.jsonfromsrc/*would still be valid after bundling, and the native-loader fast path bypassed plugin-sdk alias resolution inside nesteddist/extensions/<id>/package.jsonscopes.openclaw/plugin-sdk/*imports under nested package scopes.Regression Test Plan (if applicable)
src/channels/plugins/contracts/directory.registry-backed.contract.test.ts,src/plugins/contracts/package-manifest.contract.test.ts,src/plugins/sdk-alias.test.ts,test/scripts/root-dependency-ownership-audit.test.tsdist/extensions/<id>without reaching back into root-owned runtime deps, plugin-local package manifests still resolve, and bundled plugin dist modules still resolveopenclaw/plugin-sdk/*through the loader alias boundary.User-visible / Behavior Changes
npm pack/ staged bundled plugin packaging no longer requires Discord runtime deps to remain mirrored at the root package surface.pnpm buildis green again after narrowing OpenAI Responses reasoning-effort typing.Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
dist/extensions/<id>artifacts.Expected
openclaw/plugin-sdk/*imports.pnpm buildpasses.Actual
dist/still imported Discord-owned deps, staged plugin dist paths broke some../package.jsonlookups, and native loading ofdist/extensions/*bypassed plugin-sdk alias resolution.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm build;pnpm test src/plugins/sdk-alias.test.ts;pnpm exec vitest run --config test/vitest/vitest.contracts.config.ts src/channels/plugins/contracts/directory.registry-backed.contract.test.ts --reporter=verbose;pnpm exec vitest run --config test/vitest/vitest.contracts.config.ts src/plugins/contracts/package-manifest.contract.test.ts --reporter=verbose;pnpm test test/scripts/root-dependency-ownership-audit.test.ts;pnpm test test/release-check.test.ts test/openclaw-npm-postpublish-verify.test.ts;pnpm test src/agents/openai-transport-stream.test.ts -t 'defaults OpenAI Responses reasoning effort to high when unset'dist/extensions/<id>/package.jsonscopes; root ownership audit after build only still flagshono.pnpm test; external npm publish/install smoke beyond local packaging/runtime checks.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.src/plugins/sdk-alias.test.tsand exercised by the bundled directory contract suite.