fix: plan managed npm peer pins with npm#81450
Conversation
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Source inspection of PR head shows the fallback map copies prior managed peers without excluding Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Fix the fallback host-peer filtering with focused regression coverage, then land this PR after maintainer review and use it to supersede #81632 if the folded active-host guard remains included. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection of PR head shows the fallback map copies prior managed peers without excluding Is this the best way to solve the issue? No. Using npm's lockfile planner is a good direction, but the fallback needs to apply the same host-peer exclusion invariant before this is the narrowest maintainable fix. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 697103604367. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 971654813b
ℹ️ 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".
| let result = await command(npmPeerPlanArgs, npmPlanOptions); | ||
| if (result.code !== 0 && isHostPeerResolutionFailure(result)) { | ||
| const hydrateResult = await command( |
There was a problem hiding this comment.
Handle non-openclaw peer failures in peer-plan step
The new peer-pin planner now aborts on any npm resolution error unless the stderr/stdout happens to mention openclaw@, because only that case enters the retry path. This is a regression for plugins with conflicting or unavailable other peer dependencies: npm install --package-lock-only --omit=peer still validates peer resolution, so the plan step can fail even though the real managed-root install path intentionally uses --legacy-peer-deps and would otherwise proceed. In those cases, plugin install now fails before install/retry logic runs, instead of producing peer pins from the manifest tree as before.
Useful? React with 👍 / 👎.
94014ff to
4456242
Compare
|
Thanks @steipete. I rebased this on current What changed:
Verification:
|
4456242 to
593c8f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 555a56ae16
ℹ️ 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".
| for (const packageName of previousManagedPeerDependencies) { | ||
| const dependencySpec = dependencies[packageName]; | ||
| if (dependencySpec) { | ||
| pins[packageName] = dependencySpec; | ||
| } |
There was a problem hiding this comment.
Exclude host peer from fallback peer pins
When npm peer planning exits non-zero, syncManagedNpmRootPeerDependencies uses this fallback map, but the fallback copies prior managed peers verbatim. That means a stale openclaw managed peer is preserved instead of being pruned, so any planning failure (for example, unrelated ERESOLVE) can keep the managed root owning the host package even though this change is intended to keep host peers out of managed ownership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fda4001730
ℹ️ 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".
| for (const [location, value] of Object.entries(params.lockfile.packages).toSorted( | ||
| ([left], [right]) => left.localeCompare(right), | ||
| )) { | ||
| if ( | ||
| readLockPackageName(location, value) !== params.packageName || |
There was a problem hiding this comment.
Limit peer version pinning to root lockfile entries
The fallback loop in findLockPackageVersion now selects the first matching package version from any lockfile path, so when a peer package is missing at node_modules/<peer> but exists only as a nested transitive (for example node_modules/a/node_modules/<peer>), that nested version is pinned into the managed root. In that case we can pin an incompatible version (e.g. nested 1.x for a peer range requiring ^2) and then syncManagedNpmRootPeerDependencies preserves that bad pin across install passes, which regresses the previous root-only lookup behavior and can leave plugins installed with unsatisfied peers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ad834e3a
ℹ️ 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".
| if (location === "" || !isRecord(value) || isDevOnlyLockPackage(value)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Skip optional-only lock packages when deriving peer pins
collectNpmLockPeerDependencyPins now harvests peers from every non-dev lockfile entry, but this filter does not exclude optional-only packages that may appear in package-lock.json even when they are not materialized on the current OS/CPU. In that case, peers from a platform-specific optional package can be promoted into managed root dependencies and then enforced by later install passes, creating unnecessary or failing peer installs that did not occur with the previous installed-tree scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ab1e219ad
ℹ️ 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".
| } | ||
|
|
||
| async function scrubHostPeerFromTempPackageLock(lockPath: string): Promise<void> { | ||
| const parsed = await readJsonIfExists<unknown>(lockPath); |
There was a problem hiding this comment.
Tolerate malformed lockfiles during peer-plan setup
The new peer-planning path unconditionally parses the copied package-lock.json via readJsonIfExists before running npm, and any JSON parse error now aborts syncManagedNpmRootPeerDependencies (which then fails plugin install/rollback in install.ts). This is a regression from the prior node_modules scan path, which did not require a valid lockfile to proceed; a stale/truncated lockfile should be treated as recoverable (skip scrub/copy and let npm regenerate) rather than a hard failure.
Useful? React with 👍 / 👎.
Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com>
Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com>
e97fc88 to
a3ef462
Compare
|
Verification for current head Local / Codex proof:
GitHub / remote proof:
Current GitHub state at verification time:
Known proof gaps:
PR #81632's active-host guard/test has been folded into this branch. Contributor credit is preserved with |
Summary
node_modulesopenclawhost peers out of managed root ownership, including temp-lock scrub/retry for optional host peer metadataReal behavior proof
Behavior addressed: Managed npm plugin installs now use npm's lockfile planner for peer pins, avoid promoting nested transitive versions into root peer deps, and preserve the active root-managed
node_modules/openclawhost package instead of uninstalling it.Real environment tested: Local macOS source checkout using real npm-managed-root install paths plus the repo static npm registry e2e; no production credentials or live user state.
Exact steps or command run after this patch:
OPENCLAW_VITEST_MAX_WORKERS=1 node scripts/test-projects.mjs src/plugins/install.npm-spec.e2e.test.ts -- --reporter=verboseEvidence after fix: The npm install e2e shard passed 11 tests, including managed root host-peer cleanup and sibling peer relink behavior, after commit
3ab1e219ad50015d5637a0a9eececafc632dacf5.Observed result after fix: Managed plugin install leaves the root
node_modules/openclawabsent for normal peer materialization cleanup, relinks plugin-localnode_modules/openclawpeers, and the folded active-host regression keeps an existing root-managed active host package instead of runningnpm uninstall openclaw.What was not tested: No live third-party registry install against a real user
~/.openclawdirectory; no package acceptance Docker lane was run for this PR.Verification
pnpm check:test-typespnpm exec oxfmt --check --threads=1 src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts src/plugins/install.npm-spec.test.ts CHANGELOG.md && git diff --checknode scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts src/plugins/install.npm-spec.test.ts src/plugins/install.tsOPENCLAW_VITEST_MAX_WORKERS=1 pnpm test src/infra/npm-managed-root.test.ts src/plugins/install.npm-spec.test.ts -- --reporter=verboseOPENCLAW_VITEST_MAX_WORKERS=1 node scripts/test-projects.mjs src/plugins/install.npm-spec.e2e.test.ts -- --reporter=verbosecodex-review --full-access: clean, no accepted/actionable findingsFollow-up to #81435. Folds the active-host preservation guard/test from #81632.