fix(plugins): avoid spurious npm spec error after metadata failure#96143
Conversation
3f5af26 to
d6c9fba
Compare
|
Codex review: needs maintainer review before merge. Reviewed June 25, 2026, 7:41 PM ET / 23:41 UTC. Summary PR surface: Source +4, Tests +101. Total +105 across 2 files. Reproducibility: yes. at source level: current main logs metadata failure and falls through to installer validation, while installer validation rejects range-style registry specs before another metadata lookup. I did not induce a live npmjs.org outage during this read-only review. Review metrics: 1 noteworthy metric.
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:
Next step before merge
Security Review detailsBest possible solution: Land the focused guard once maintainers accept the fallback tradeoff, and keep broader npm metadata retry/no-output handling as separate follow-up work if still desired. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main logs metadata failure and falls through to installer validation, while installer validation rejects range-style registry specs before another metadata lookup. I did not induce a live npmjs.org outage during this read-only review. Is this the best way to solve the issue? Yes for the targeted symptom: the PR routes only installer-rejected specs through centralized failure handling and keeps valid exact, tag, and bare npm specs on the existing installer fallback. Broader retry/no-output behavior is a separate policy choice. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0da26499dac2. Label changesLabel justifications:
Evidence reviewedPR surface: Source +4, Tests +101. Total +105 across 2 files. View PR surface stats
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 re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
# Conflicts: # src/plugins/update.test.ts
…law#96143) Co-authored-by: Dallin Romney <[email protected]>
…law#96143) Co-authored-by: Dallin Romney <[email protected]>
…law#96143) Co-authored-by: Dallin Romney <[email protected]> (cherry picked from commit dd0e4f6)
Closes #77616
Recreates and repairs #95581.
What Problem This Solves
Fixes an issue where a transient npm metadata lookup failure for a range-style installed plugin spec could produce a confusing second error about an unsupported npm spec.
Why This Change Was Made
When metadata probing fails for a registry spec that the installer cannot validate, the update path now records the metadata check failure through the existing failure handler and stops before invoking the installer. That preserves
disableOnFailurecleanup for post-core updates. Valid exact, tag, and bare registry specs keep the existing installer fallback behavior. The same change also normalizes managed extensions-root path comparison so updater tests pass consistently on Windows and POSIX paths.User Impact
Users see the real metadata/registry failure instead of a misleading follow-on npm spec error, and failed post-core plugin updates still disable and clean stale plugin policy/slot references when configured to do so.
Evidence
node scripts/run-vitest.mjs src/plugins/update.test.tspassed locally: 99 tests.git diff --checkpassed locally.blacksmith-testbox,tbx_01kvtjzeq8qprs80191nsek8y93f5af2643cae4622f980786426a05893e06badb0codex/recreate-95581-plugin-update-metadata-failuredirectly fromhttps://github.com/brokemac79/openclaw.gitonto a cleanorigin/mainworktree on the runner.could not resolve repo: could not get git remote URL: exit status 128). The Testbox proof avoids local sync by checking out the PR branch directly on the Testbox.d6c9fbadacafbdfa50ee01b825413e212f8bc20f:The Azure changed gate included guarded extension wildcard re-export checks, plugin-sdk wildcard re-export checks, dependency pin guard, npm shrinkwrap guard, runtime sidecar baseline/owner test, package patch guard, database-first legacy-store guard, media download helper guard, runtime sidecar loader guard, all changed-scope typecheck, lint shards, and runtime import cycle check.
python .agents/skills/autoreview/scripts/autoreview --mode commit --commit HEAD --codex-bin %TEMP%/codex-fast-autoreview.cmdreported clean, no accepted/actionable findings.Real behavior proof: plugin update metadata failure
Behavior or issue addressed: a range-style installed npm plugin spec that hits a transient metadata lookup failure should report the registry failure and stop, instead of falling through to the installer and adding a misleading unsupported npm spec error.
Real environment tested: Docker-backed Crabbox
provider=local-container, imagemcr.microsoft.com/playwright:v1.60.0-noble, leasecbx_32e734196d8b, slugquick-krill, PR headd6c9fbadacafbdfa50ee01b825413e212f8bc20f.Exact steps or command run after this patch:
The proof script installed dependencies, created an installed plugin record for
@martian-engineering/lossless-claw@^0.9.0, routed productionnpm viewsubprocess calls to a local fakenpmexecutable that returnedregistry timeout, then invoked the production post-core updater functionupdateNpmInstalledPluginswithdisableOnFailure: true. The fakenpmexecutable exits with a sentinel error for any install/add command, so the proof fails if the updater falls through to the installer path.Evidence after fix:
Observed result after fix: the updater called only
npm view, reportedFailed to check lossless-claw: npm view failed: registry timeout, applied thedisableOnFailurecleanup, did not attemptnpm install/npm add, and did not emit anyunsupported npm specfollow-on message.Supplemental validation from the same local-container run:
What was not tested: a live npm registry outage was not induced against npmjs.org; the proof uses a controlled failing
npm viewsubprocess to exercise the real OpenClaw post-core plugin updater branch deterministically.