fix(plugins): harden imported extension dependencies - #329
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical dependency-validation and credential-boundary issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR hardens imported extension dependency installation and source copying with registry-only npm execution, cleanup, unique plugin IDs, sensitive-file filtering, tests, and documentation updates.
Changes:
- Adds isolated npm installation, validation, timeout handling, and cleanup.
- Makes imported plugins atomic and uniquely identified.
- Updates tests, E2E expectations, English/Chinese documentation, and ADR 0244.
File summaries
| File | Reviewed changes |
|---|---|
README.zh-CN.md |
Documents npm prerequisites. |
README.md |
Documents npm prerequisites and registry-only behavior. |
docs/zh-CN/spec/07-plugins/16-trusted-extensions.md |
Updates Chinese import and dependency behavior; ADR 0244 status needs synchronization (Nit, 1 vote). |
docs/zh-CN/spec/07-plugins/06-plugin-packaging.md |
Documents the import exception. |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Updates dependency-install E2E expectations. |
docs/zh-CN/plugin-development.md |
Documents dependency installation; restore the omitted supported-members continuation (Nit, 1 vote). |
docs/spec/07-plugins/16-trusted-extensions.md |
Defines the hardened boundary; remove the duplicate heading (Nit, 3 votes) and synchronize the Chinese ADR status (Nit, 1 vote). |
docs/spec/07-plugins/06-plugin-packaging.md |
Documents the import exception. |
docs/spec/06-delivery/04-e2e-test-plan.md |
Updates E2E expectations. |
docs/plugin-development.md |
Documents dependency installation. |
docs/adr/README.md |
Registers ADR 0244; add its Chinese index row (Nit, 2 votes). |
docs/adr/0244-imported-extension-dependency-boundary.md |
Records the dependency-boundary decision. |
docs/adr/0215-agent-extensions-as-plugin-contribution.md |
Clarifies validation scope. |
apps/desktop/test/agent-extensions.test.mjs |
Adds boundary and cleanup tests; remove the unused rmSync import (Nit, 1 vote). |
apps/desktop/electron/main/npm-registry-proxy.ts |
Implements registry-only loopback proxying. |
apps/desktop/electron/main/npm-installer.ts |
Implements validation and isolated installation; address npm v1 requires maps (Critical, 2 votes) and pre-resolution transitive non-registry dependencies (Critical, 1 vote). |
apps/desktop/electron/main/agent-extensions.ts |
Implements atomic imports and unique IDs; expand credential filtering for .key, id_ed25519, .crt, and .cer files (Critical, 3 votes). |
Review details
Suppressed comments (4)
apps/desktop/test/agent-extensions.test.mjs:4
rmSyncis imported but never referenced anywhere in this test file. Remove the dead import; otherwise a no-unused-import lint configuration will reject the test.
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
docs/adr/README.md:266
- The new row duplicates the
active-turn-steeringentry that already exists on the next line. Remove the duplicate index entry.
| active-turn-steering | Bind Composer steering to the active durable turn | Accepted (active-turn-steering; issue #164) |
docs/spec/07-plugins/16-trusted-extensions.md:3
- Adding ADR 0244 to the English status requires the corresponding Chinese status line to be updated as well, but
docs/zh-CN/spec/07-plugins/16-trusted-extensions.mdstill lists only ADR 0214/0215. Please synchronize that counterpart so the two specification pages do not describe different decision sets.
> Status: Implemented v1.1 (D387 / D388, ADR 0214 / ADR 0215 / ADR 0244); implementation notes are marked "v1 note"
docs/zh-CN/plugin-development.md:638
- The Chinese page drops the continuation of the preceding “supported members” bullet, so it now ends at
在插件行and immediately starts the next bullet. Restore的详情里报告，绝不抛出。to keep this locale complete and semantically aligned with the English documentation.
生成的插件。若目录声明了生产或可选依赖，PI-Desktop 会先运行
- Files reviewed: 17/17 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+268
to
+271
| IMPORT_SENSITIVE_FILE_NAMES.has(name) || | ||
| name.startsWith(".env") || | ||
| name.startsWith("id_rsa") || | ||
| /\.(pem|p12|pfx|keystore)$/.test(name) |
Comment on lines
+30
to
+35
| const LOCKFILE_DEPENDENCY_FIELDS = new Set([ | ||
| "dependencies", | ||
| "optionalDependencies", | ||
| "devDependencies", | ||
| "peerDependencies", | ||
| ]); |
Comment on lines
+374
to
+376
| const resolvedLockfiles = sanitizeDependencyLockfiles(pluginDir); | ||
| if (resolvedLockfiles.removedUnsafe) { | ||
| return failed("npm dependency resolution produced a non-registry lockfile source"); |
| | 0241 | Ship the file view as a vendored, updatable plugin | Accepted (supersedes ADR 0105; issue #304) | | ||
| | 0242 | Delta-only coalesced streaming updates | Accepted (amends 0127 / 0130 / 0149 / 0153; issue #299) | | ||
| | 0243 | Skill market public-HTTPS catalog fetch | Accepted (amends 0009; issue #287 / PR #290) | | ||
| | 0244 | Bound dependency installation for imported extensions | Accepted | |
| the user's credentials and proxy settings. Importing a package does not promise | ||
| that every third-party extension dependency can execute. | ||
|
|
||
| ## 4. Loading and runtime |
Owner
Author
|
Merged as Follow-up review fixes included:
Validation on the integrated |
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
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.
Follow-up to merged PR #300.
Review findings fixed
Validation
pnpm build:jspnpm lintpnpm docs:check(77 pairs)pnpm -r --if-present test(1603 passed)cargo fmt --checkcargo test -p host-core --locked(378 passed)cargo clippy -p host-core --all-targets --lockedpnpm test:e2e(18/18 passed; 2 live-model cases skipped withoutPI_DESKTOP_TEST_API_KEY)left-padpassed and removed temporary cache.