Skip to content

fix(update): fail closed on managed plugin pin conflicts#87304

Closed
luoyanglang wants to merge 10 commits into
openclaw:mainfrom
luoyanglang:wolf/update-preserve-newer-plugin-pins
Closed

fix(update): fail closed on managed plugin pin conflicts#87304
luoyanglang wants to merge 10 commits into
openclaw:mainfrom
luoyanglang:wolf/update-preserve-newer-plugin-pins

Conversation

@luoyanglang

@luoyanglang luoyanglang commented May 27, 2026

Copy link
Copy Markdown
Contributor

openclaw update could 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.ts
    • installPluginFromManagedNpmRoot
    • managed npm update-mode conflict handling
  • src/infra/npm-managed-root.ts
    • upsertManagedNpmRootDependency
    • release-managed dependency spec writing
  • Tests:
    • src/plugins/install.npm-spec.test.ts
    • src/infra/npm-managed-root.test.ts

Scope

  • Release-managed plugin versions remain authoritative for the managed npm root write path.
  • In explicit update mode, if the managed npm project already has a newer direct dependency pin than the release-managed target, the update fails closed instead of downgrading or silently preserving it.
  • The conflict is detected before npm install, including the case where the package payload is missing and resolveEffectiveInstallMode() would otherwise fall back from update to install.
  • The error names the package, current package.json pin, release target, and the operator choices: use release-managed version, keep the user pin explicitly, or abort.
  • Existing install mode, equal versions, older managed versions, peer repair, and unrelated dependency updates keep the existing behavior.

Real behavior proof

  • Behavior or issue addressed: An update-mode managed npm plugin install now refuses to automatically choose between a user-pinned newer direct dependency and the release-managed target.
  • Real environment tested: macOS local development machine, current PR head 5f1d39b844b2dbcc03ebbe84bca6ba8599347d82, real OpenClaw source imported with node --import tsx, real npm metadata lookup for @martian-engineering/[email protected], temporary managed npm root outside the repository.
  • Exact steps or command run after this patch: Create a temporary managed npm project with package.json pinning @martian-engineering/lossless-claw to 0.11.3; run installPluginFromNpmSpec(..., { spec: "@martian-engineering/[email protected]", mode: "update" }); verify the result fails closed, the package.json pin remains 0.11.3, and no npm install artifacts are created. This refresh uses 0.11.2/0.11.3 because the older proof spec 0.10.0 is now rejected earlier by the current runtime plugin API compatibility check, before this managed-root policy branch.
  • Evidence after fix:
{
  "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:

$ node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts
Test Files  1 passed (1)
Tests  56 passed (56)

$ node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts
Test Files  1 passed (1)
Tests  19 passed (19)

$ ./node_modules/.bin/oxfmt --check src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts
All matched files use the correct format.

$ git diff --check
(exit 0)

$ node scripts/check-no-conflict-markers.mjs
(exit 0)

$ gitleaks protect --staged --redact
no leaks found
  • Observed result after fix: The update returns an actionable conflict error, leaves the user pin unchanged, and does not run npm install.
  • What was not tested: I did not implement an interactive chooser in this patch. The current install boundary is non-interactive, so this PR establishes the fail-closed behavior and error shape that an interactive update flow can later use.

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.

@openclaw-barnacle openclaw-barnacle Bot added size: M triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 28, 2026, 3:38 AM ET / 07:38 UTC.

Summary
The PR adds a managed npm update conflict check that fails closed when a newer direct dependency pin conflicts with the release-managed plugin target, plus focused regression tests.

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.

  • Fail-Closed Update Gate: 1 added. The diff adds one new pre-install policy branch that changes non-interactive update behavior before npm install runs.
  • Conflict Root Scope: 2 roots checked. The current PR checks only the selected destination root and legacy project root, which matters because current main can store active installs in generation roots.

Stored data model
Persistent data-model change detected: serialized state: src/infra/npm-managed-root.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #85184
Summary: This PR is the active candidate fix for the canonical managed direct dependency downgrade issue; adjacent managed npm install PRs do not replace this remaining policy/fix path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🐚 platinum hermit
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Extend the conflict check to all existing managed roots for the package, including generation roots.
  • [P2] Add a regression where an active generation root has a newer package.json pin and the release target is older.

Risk before merge

  • [P1] The new conflict guard can miss newer pins in existing generation roots, so merging as-is can still silently switch an update to an older release-managed package in that state.
  • [P1] Even after the root-coverage bug is fixed, this intentionally changes non-interactive managed plugin updates from rewriting to failing closed, so unattended update workflows may stop until an operator chooses a policy.

Maintainer options:

  1. Fix Generation-Root Coverage (recommended)
    Extend the conflict check to include existing package generation roots, then prove a newer active generation pin fails closed before dry-run or npm install.
  2. Accept Only With Explicit Upgrade Risk
    Maintainers could choose to land the current scope knowingly, but that leaves a documented downgrade gap in generated-root installs.
  3. Pause For Broader Update UX
    Maintainers can pause this PR until the interactive chooser or explicit keep-pin/release-managed flag is designed if fail-closed alone is not enough.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Extend managed pin conflict detection to inspect existing managed npm project/generation roots for the package before choosing or writing the install root; add regression coverage for a newer pin in an active generation root and keep the fail-closed behavior before npm install and dry-run.

Next step before merge

  • [P1] A narrow repair can extend the conflict scan to existing generation roots and add targeted regression coverage without changing the maintainer-approved fail-closed policy.

Security
Cleared: The diff changes managed npm install policy and tests without adding dependencies, workflows, secret handling, or new executable supply-chain sources.

Review findings

  • [P1] Check existing generation roots before installing — src/plugins/install.ts:1417
Review details

Best 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:

  • [P1] Check existing generation roots before installing — src/plugins/install.ts:1417
    The new conflict guard only checks the destination root and the legacy project root. Current main can keep the active installed package in an existing generation root, while an update to an older release target computes a different fresh generation root; in that state this array never reads the newer active package.json pin, so the update can still install/switch to the older package instead of failing closed. Include the existing generation roots for the package in the conflict check and add a regression for that case.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9c95abd49d45.

Label changes

Label justifications:

  • P1: The PR addresses a user-facing update path that can silently downgrade managed plugin dependency state and reintroduce fixed plugin bugs.
  • merge-risk: 🚨 compatibility: Merging changes non-interactive managed npm update behavior and can stop unattended updates when a newer direct pin conflicts with the release-managed target.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🐚 platinum hermit and patch quality is 🧂 unranked krab.
  • status: 🛠️ actively grinding: The PR author has acted after the latest ClawSweeper review and work remains. Sufficient (live_output): The PR discussion includes live-output style proof from a real source run with real npm metadata for the legacy/project-root conflict path, though the finding above identifies an unproven generation-root edge.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR discussion includes live-output style proof from a real source run with real npm metadata for the legacy/project-root conflict path, though the finding above identifies an unproven generation-root edge.
Evidence reviewed

PR surface:

Source +56, Tests +436. Total +492 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 56 0 +56
Tests 2 439 3 +436
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 495 3 +492

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts -t "newer user pin|generation".
  • [P1] node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts src/plugins/update.test.ts.
  • [P1] node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts.
  • [P1] pnpm exec oxfmt --check --threads=1 src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts.
  • [P1] git diff --check.

What I checked:

  • Repository policy applied: Root policy treats plugin update fail-closed behavior and fallback changes as compatibility-sensitive merge risk, and the scoped plugin guide requires preserving loader/install contracts. (AGENTS.md:35, 9c95abd49d45)
  • Scoped plugin policy read: The plugin boundary guide was read fully and applies because the diff changes plugin install/update behavior. (src/plugins/AGENTS.md:1, 9c95abd49d45)
  • Current main uses generated npm roots: Current main enumerates both legacy and generation package dirs, then uses that set to decide update generation behavior, so existing active state can live outside the legacy project root. (src/plugins/install.ts:1164, 9c95abd49d45)
  • PR conflict scope is too narrow: The PR diff passes only the selected install root and the legacy project root into the new conflict check; it does not include other existing generation roots for the package. (src/plugins/install.ts:1417, bc5d533ccc08)
  • Current main still rewrites dependency specs before install: The managed npm writer sets package.json dependencies to the requested dependency spec, which is the mutation this PR is trying to guard before npm install runs. (src/infra/npm-managed-root.ts:335, 9c95abd49d45)
  • Maintainer policy note supports fail-closed direction: A maintainer comment says silently rewriting a user-maintained pin is not acceptable and prefers interactive choice or non-interactive fail-closed behavior, which supports this PR's intended policy.

Likely related people:

  • steipete: Posted the fail-closed policy direction on this PR and has recent commits in the same managed npm install/root area. (role: policy reviewer and recent area contributor; confidence: high; commits: ea682182d05a, a326faa10cf8, 4491232874d8; files: src/plugins/install.ts, src/infra/npm-managed-root.ts)
  • vincentkoc: Recent current-main commits touched plugin install security, required package repair, and managed npm root behavior near this update path. (role: recent area contributor; confidence: high; commits: 97ce204d972c, ccf83ace38f2, 5d557171b3cd; files: src/plugins/install.ts, src/infra/npm-managed-root.ts)
  • ooiuuii: Authored the merged fix for npm plugin updates preserving running gateway imports, which introduced the generated root behavior this PR must account for. (role: introduced adjacent generation-root behavior; confidence: medium; commits: dd055c4f7c3a; files: src/plugins/install.ts, src/plugins/install.npm-spec.test.ts)
  • amknight: Authored the merged managed npm root override and peer pin reconciliation change in the same managed root writer area. (role: adjacent managed-root contributor; confidence: medium; commits: 2cafbd07746d; files: src/infra/npm-managed-root.ts, src/infra/npm-managed-root.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 🥚 common Clockwork Signal Puff

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: hums during re-review.
Image traits: location merge queue dock; accessory release bell; palette rose quartz and slate; mood curious; pose stepping out of a freshly hatched shell; shell woven fiber shell; lighting subtle sparkle highlights; background subtle branch markers.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Clockwork Signal Puff in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@luoyanglang

Copy link
Copy Markdown
Contributor Author

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:

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts
  • pnpm exec oxfmt --check src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts
  • pnpm lint --threads=8
  • node scripts/check-changed.mjs --base origin/main
  • git diff --check origin/main..HEAD && git diff --check
  • node scripts/check-no-conflict-markers.mjs
  • gitleaks protect --staged --redact

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@luoyanglang

Copy link
Copy Markdown
Contributor Author

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

@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@luoyanglang

Copy link
Copy Markdown
Contributor Author

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

@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 27, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 27, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 27, 2026
@luoyanglang

Copy link
Copy Markdown
Contributor Author

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

@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@luoyanglang
luoyanglang force-pushed the wolf/update-preserve-newer-plugin-pins branch from 88b1b9f to 5bfdb85 Compare May 27, 2026 15:20
@luoyanglang

Copy link
Copy Markdown
Contributor Author

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

@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 27, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 3, 2026
@luoyanglang

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Refreshed the Real behavior proof for current head c3a909dd791b5b0c2c0d2170b511ff6a16d75b7c.

Proof rerun summary:

  • Real npm metadata lookup for @martian-engineering/[email protected].
  • Temporary managed npm root pre-pinned @martian-engineering/lossless-claw to 0.11.3.
  • installPluginFromNpmSpec(..., mode: "update") failed closed with the managed dependency conflict.
  • The package.json pin stayed 0.11.3; no package-lock.json or node_modules was created.

Validation rerun on this head:

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts (56 passed)
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts (19 passed)
  • ./node_modules/.bin/oxfmt --check src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts
  • git diff --check origin/main..HEAD
  • node scripts/check-no-conflict-markers.mjs
  • gitleaks detect --redact --log-opts origin/main..HEAD

@clawsweeper

clawsweeper Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

@luoyanglang

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Resolved the current origin/main merge conflict by merging current main into this PR.

New head: 5f1d39b844b2dbcc03ebbe84bca6ba8599347d82.

Conflict resolution summary:

  • Kept this PR's managed dependency update-conflict check before dryRun.
  • Kept current main's npm install policy preflight before dryRun.
  • The combined order is now availability check -> managed dependency conflict check -> policy preflight -> dry-run/install path.

Refreshed real behavior proof for the new head:

  • Real npm metadata lookup for @martian-engineering/[email protected].
  • Temporary managed npm root pre-pinned @martian-engineering/lossless-claw to 0.11.3.
  • installPluginFromNpmSpec(..., mode: "update") failed closed with the managed dependency conflict.
  • The package.json pin stayed 0.11.3; no package-lock.json or node_modules was created.

Validation on the new head:

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts (56 passed)
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts (19 passed)
  • ./node_modules/.bin/oxfmt --check src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts
  • git diff --check
  • node scripts/check-no-conflict-markers.mjs
  • gitleaks protect --staged --redact

@clawsweeper

clawsweeper Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@openclaw-barnacle

Copy link
Copy Markdown

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

@luoyanglang

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Refreshed this PR on head bcad98f16c against current origin/main (3848b9619f).

What changed:

  • Resolved the src/plugins/install.npm-spec.test.ts conflict by preserving both main's replaceExisting mock option and this PR's viewFailure mock option.
  • Fixed the managed pin conflict check after main's generated npm root changes: update mode now checks both the selected install root and the legacy/project managed npm root before writing or installing, so an existing newer user pin is still caught before generation-root install paths can overwrite or bypass it.
  • Preserved the fail-closed policy: non-interactive update refuses to choose between the release-managed target and a newer user-maintained pin.

Validation:

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts -t "newer user pin|newer user-pinned|lockfile and node_modules disagree" passed: 6 tests.
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts src/plugins/update.test.ts passed: 2 files, 159 tests.
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts passed: 28 tests.
  • pnpm exec oxfmt --check --threads=1 src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/plugins/update.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts passed.
  • git diff --check origin/main...HEAD passed.
  • node scripts/check-no-conflict-markers.mjs src/plugins/install.ts src/plugins/install.npm-spec.test.ts src/plugins/update.test.ts src/infra/npm-managed-root.ts src/infra/npm-managed-root.test.ts passed.
  • gitleaks git --redact --log-opts origin/main..HEAD passed with no leaks.

Note: full staged merge scanning reported inherited generic-api-key matches from current-main lmstudio/UI i18n test fixtures, but the PR range scan above passed and the flagged files are not part of this PR diff.

@clawsweeper

clawsweeper Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@luoyanglang

Copy link
Copy Markdown
Contributor Author

Follow-up for the failed checks-node-compact-small-whole-3 check on head bcad98f16c.

Root cause from the Actions log matched the #84975 failure: ui/src/ui/chat/slash-commands.browser-import.test.ts expected the shared command registry imports in the pre-format order, while the merged branch uses the formatted order with normalization imports before shared/fast-mode.js.

Fix on head 4685b4c1b5:

  • Aligned the browser import guard expectation with the formatted import order.
  • No runtime behavior changed.

Validation:

  • node scripts/run-vitest.mjs run ui/src/ui/chat/slash-commands.browser-import.test.ts passed: 2 tests.
  • pnpm exec oxfmt --check --threads=1 src/auto-reply/commands-registry.shared.ts ui/src/ui/chat/slash-commands.browser-import.test.ts passed.
  • git diff --check origin/main...HEAD passed.
  • node scripts/check-no-conflict-markers.mjs src/auto-reply/commands-registry.shared.ts ui/src/ui/chat/slash-commands.browser-import.test.ts passed.
  • gitleaks git --redact --log-opts origin/main..HEAD passed with no leaks.

@openclaw-barnacle

Copy link
Copy Markdown

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

…newer-plugin-pins

# Conflicts:
#	test/scripts/plugin-sdk-surface-report.test.ts
@luoyanglang

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Refreshed this PR on head bc5d533cc against current origin/main (119dc4bd82).

Maintainer alignment: this matches @steipete's preferred near-term shape. In update mode, when the existing package.json pin is newer than the release-managed target, the install path now returns a hard conflict error (fail closed) naming the package, current pin, and target version, with the three operator options (use release version / keep pin explicitly / abort). It does not silently preserve the newer pin. The check spans both the selected install root and the legacy/project managed npm root.

What changed in this refresh:

  • Merged current origin/main. The only conflict was test/scripts/plugin-sdk-surface-report.test.ts; resolved by taking main's version (this PR has no independent change there).
  • Restored all unrelated import-order/format churn back to origin/main. The net PR diff is now exactly the plugin-pin change: 4 files, +495/-3.

Net diff surface:

  • src/plugins/install.ts
  • src/infra/npm-managed-root.ts
  • src/plugins/install.npm-spec.test.ts
  • src/infra/npm-managed-root.test.ts

Local validation (Mac, Node 22.22.3):

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/install.npm-spec.test.ts src/plugins/update.test.ts — 168 tests pass
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.infra.config.ts src/infra/npm-managed-root.test.ts — 28 tests pass
  • pnpm exec oxfmt --check --threads=1 on the 4 touched files
  • git diff --check origin/main...HEAD
  • gitleaks git --redact --log-opts origin/main..HEAD — no leaks

@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle

Copy link
Copy Markdown

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

@luoyanglang

Copy link
Copy Markdown
Contributor Author

Re: the P1 generation-root scan finding (src/plugins/install.ts:1417) — I prototyped extending the conflict scan to all active artifact-generation roots (with a regression test) and confirmed it reproduces the gap, but it regresses the retained-generation rollback flow covered by does not mutate a retained generation when an exact rollback reuses its artifact key.

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 generationUse: "update" (the rollback allocates a fresh generation, not a retained-install), same explicit-version spec, same mode: "update". So neither generationUse nor "explicit version requested" can separate the two — and the existing fails closed when update finds a newer user-pinned managed dependency test already uses an explicit @0.10.0 spec while expecting fail-closed, so an explicit-version bypass would break that contract too.

Distinguishing a stuck user pin from OpenClaw's own managed newer generation needs an "explicit user downgrade vs bulk openclaw update auto-resolve" intent signal that lives in the update command layer, not in installPluginFromManagedNpmRoot. The current two-root scope (selected install root + legacy project root) appears intentional for that reason: the newer-pin-in-the-selected/legacy-root case fails closed, while a newer generation that is not the install target is left to the rollback lifecycle.

Could you confirm the preferred direction:

  1. Keep the current two-root scope (treat the finding as covered by the rollback contract), or
  2. Accept a larger cross-layer change that threads update-intent into the managed pin conflict guard (and adjusts the explicit-version test contract accordingly)?

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.

@openclaw-barnacle

Copy link
Copy Markdown

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

@luoyanglang

Copy link
Copy Markdown
Contributor Author

Still current, mergeable CLEAN against latest main. The one open item is the ClawSweeper P1 to extend the pin-conflict scan to all active generation roots — I prototyped that (with a repro test), but it regresses the legitimate retained-generation rollback flow (does not mutate a retained generation when an exact rollback reuses its artifact key, structurally the #85184 downgrade case). This needs a maintainer scope call: (A) keep the current narrow fail-closed fix, or (B) broaden the scan with a new way to tell a legitimate rollback apart from a real conflict. Happy to implement whichever you prefer.

@openclaw-barnacle

Copy link
Copy Markdown

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

@openclaw-barnacle

Copy link
Copy Markdown

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M stale Marked as stale due to inactivity status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openclaw update silently downgrades and rewrites pinned direct-dependency versions

5 participants