fix(update): fail closed on managed plugin pin conflicts#87304
fix(update): fail closed on managed plugin pin conflicts#87304luoyanglang wants to merge 10 commits into
Conversation
|
Codex review: needs changes before merge. Reviewed June 28, 2026, 3:38 AM ET / 07:38 UTC. Summary PR surface: Source +56, Tests +436. Total +492 across 4 files. Reproducibility: yes. Source inspection shows current update installs can write the release-managed dependency spec into managed npm package.json before npm install, and the PR's own tests/proof exercise the legacy-root conflict path. Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Keep the fail-closed policy, but inspect all existing managed roots for the package before choosing or writing the install target and add a regression for an active generation-root newer pin. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current update installs can write the release-managed dependency spec into managed npm package.json before npm install, and the PR's own tests/proof exercise the legacy-root conflict path. Is this the best way to solve the issue? No, not yet. The fail-closed boundary is the right layer, but the implementation must cover existing generation roots as well as the selected destination and legacy project root. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9c95abd49d45. Label changesLabel justifications:
Evidence reviewedPR surface: Source +56, Tests +436. Total +492 across 4 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Clockwork Signal Puff Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
Addressed the ClawSweeper P2 finding in head 965efbc. The preserved-newer-pin path now re-resolves metadata for the preserved spec when lockfile metadata is missing, so version/integrity/shasum stay aligned instead of carrying stale 0.10.0 metadata into a 0.11.2 install target. Added regression coverage for an existing 0.11.2 managed root pin with missing lockfile metadata, plus current-head mutating managed npm root proof using real npm registry versions for @martian-engineering/lossless-claw 0.10.0 -> 0.11.2. The PR body now includes the redacted proof output. Local validation passed:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Updated the Real behavior proof section with the required structured fields (behavior, environment, steps, evidence, observedResult, notTested) while preserving the source-level and mutating managed npm root evidence. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
Adjusted the Real behavior proof to match scripts/github/real-behavior-proof-policy.mjs exactly: one Real behavior proof section with recognized field lines (Behavior or issue addressed, Real environment tested, Exact steps or command run after this patch, Evidence after fix, Observed result after fix, What was not tested). @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
Addressed the latest P2 finding in head 88b1b9f. The preserved-newer-pin path now fails closed if preserved-version npm metadata cannot be resolved, and it only reuses an installed preserved dependency directly when lock metadata includes integrity; otherwise it uses the normal npm install + integrity verification path. Added a regression covering metadata lookup failure with zero npm install calls, reran focused tests, check-changed, and current-head mutating managed npm root proof. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
88b1b9f to
5bfdb85
Compare
|
Rebased onto current origin/main (20eab65) and refreshed the current-head managed npm root proof for head 5bfdb85. Targeted plugin and infra tests still pass. One CI guard is currently failing on the upstream shrinkwrap baseline ([email protected] absent from pnpm-lock.yaml), which reproduces locally via node scripts/generate-npm-shrinkwrap.mjs --all --check and is unrelated to this PR's files. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review Refreshed the Real behavior proof for current head Proof rerun summary:
Validation rerun on this head:
|
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
@clawsweeper re-review Resolved the current New head: Conflict resolution summary:
Refreshed real behavior proof for the new head:
Validation on the new head:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…newer-plugin-pins
…newer-plugin-pins
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
@clawsweeper re-review Refreshed this PR on head What changed:
Validation:
Note: full staged merge scanning reported inherited |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Follow-up for the failed Root cause from the Actions log matched the #84975 failure: Fix on head
Validation:
|
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
…newer-plugin-pins # Conflicts: # test/scripts/plugin-sdk-surface-report.test.ts
…newer-plugin-pins
|
@clawsweeper re-review Refreshed this PR on head Maintainer alignment: this matches @steipete's preferred near-term shape. In update mode, when the existing What changed in this refresh:
Net diff surface:
Local validation (Mac, Node 22.22.3):
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Re: the P1 generation-root scan finding ( Rolling back to an older version while a newer active generation exists is structurally identical to the #85184 downgrade case at the install layer: same Distinguishing a stuck user pin from OpenClaw's own managed newer generation needs an "explicit user downgrade vs bulk Could you confirm the preferred direction:
I'm happy to implement (2) if that's the desired direction; I held off because it changes update UX semantics rather than being a narrow install-layer guard. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Still current, |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Closing due to inactivity. |
openclaw updatecould silently downgrade a user-pinned managed plugin direct dependency. After the maintainer policy note, this PR now implements the conflict/fail-closed behavior instead of preserving the newer pin automatically.Fixes #85184.
Affected surface
src/plugins/install.tsinstallPluginFromManagedNpmRootsrc/infra/npm-managed-root.tsupsertManagedNpmRootDependencysrc/plugins/install.npm-spec.test.tssrc/infra/npm-managed-root.test.tsScope
npm install, including the case where the package payload is missing andresolveEffectiveInstallMode()would otherwise fall back from update to install.Real behavior proof
5f1d39b844b2dbcc03ebbe84bca6ba8599347d82, real OpenClaw source imported withnode --import tsx, real npm metadata lookup for@martian-engineering/[email protected], temporary managed npm root outside the repository.package.jsonpinning@martian-engineering/lossless-clawto0.11.3; runinstallPluginFromNpmSpec(..., { spec: "@martian-engineering/[email protected]", mode: "update" }); verify the result fails closed, the package.json pin remains0.11.3, and no npm install artifacts are created. This refresh uses0.11.2/0.11.3because the older proof spec0.10.0is now rejected earlier by the current runtime plugin API compatibility check, before this managed-root policy branch.{ "proof": "openclaw-pr87304-managed-root-policy", "head": "5f1d39b844b2dbcc03ebbe84bca6ba8599347d82", "packageName": "@martian-engineering/lossless-claw", "requestedUpdateSpec": "@martian-engineering/[email protected]", "existingPackageJsonPin": "0.11.3", "result": { "ok": false, "error": "Managed plugin dependency conflict for @martian-engineering/lossless-claw: current package.json pin is 0.11.3 but OpenClaw release target is 0.11.2. Non-interactive update is refusing to choose automatically. Use the release-managed version, keep the user pin explicitly, or abort." }, "finalPackageJsonPin": "0.11.3", "npmInstallArtifacts": { "packageLockExists": false, "nodeModulesExists": false }, "npmInstallRan": false, "tempRootKind": "mktemp /tmp/openclaw-pr87304-policy-proof-*" }Local validation after the current-main merge conflict resolution:
Coverage note
This remains the open PR for #85184. Codegraph-reported overlap with #86122, #87477, and #87573 is in the shared plugin install/update functions; the policy behavior above should remain the compatibility point for those edits.