Skip to content

fix(plugins): skip update when npm metadata is unavailable instead of producing spurious spec error#95581

Closed
lsr911 wants to merge 9 commits into
openclaw:mainfrom
lsr911:fix/issue-77616-Bug-Plugin-update-emits-spu
Closed

fix(plugins): skip update when npm metadata is unavailable instead of producing spurious spec error#95581
lsr911 wants to merge 9 commits into
openclaw:mainfrom
lsr911:fix/issue-77616-Bug-Plugin-update-emits-spu

Conversation

@lsr911

@lsr911 lsr911 commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Plugin update check emits a confusing chain of two errors when npm view
fails transiently:

  1. "Could not check X before update; falling back to installer path:
    npm view produced incomplete package metadata"
  2. "Failed to update X: unsupported npm spec: use an exact version or
    dist-tag (ranges are not allowed)"

The second message is spurious — the spec validation rejects the version
range only because the metadata it depends on was never fetched. Users
see two errors for a single transient registry failure.

Why This Change Was Made

When npm metadata is unavailable (transient registry error, rate limit,
etc.), the update flow logged a warning and fell through to
installPluginFromNpmSpec, which calls validateRegistryNpmSpec. That
validator rejects version ranges with "unsupported npm spec", producing
a confusing second error.

The fix records the error and skips the plugin update entirely when
metadata is unavailable, avoiding the guaranteed-to-fail installer path.

Changes

  • src/plugins/update.ts — when fetchNpmMetadataForUpdate fails,
    record an error outcome and continue to the next plugin instead of
    falling through to spec validation

Evidence

  • This eliminates the double-error UX for a single transient failure
  • Existing tests at plugins/update.test.ts and install.npm-spec.test.ts
    cover the surrounding update and spec-validation logic

Related

Closes #77616

@lsr911
fix(plugins): skip update when npm metadata is unavailable instead of…
333c245 
… producing spurious spec error

When npm view fails (transient registry error, rate limit, etc.),
the update flow logged a warning and fell through to spec validation,
which rejected version ranges with "unsupported npm spec". This
produced two confusing error messages for a single transient failure.

Fix: when metadata is unavailable, record the error and skip the
plugin update instead of falling through to the guaranteed-to-fail
installer path.

Closes openclaw#77616
@clawsweeper

clawsweeper Bot commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 6:55 PM ET / 22:55 UTC.

Summary
The PR adds an early error outcome in src/plugins/update.ts when npm metadata lookup fails during npm plugin updates, skipping the installer path for that plugin.

PR surface: Source +11. Total +11 across 1 file.

Reproducibility: Source-reproducible, but not live-reproduced here: current main has the metadata-failure warning/fallback path and an adjacent mocked test for npm view failure. The exact live transient registry failure and after-fix behavior still need redacted real proof.

Review metrics: 1 noteworthy metric.

  • Fallback behavior changed: 1 npm metadata-failure fallback changed to immediate skip. This is shipped plugin update behavior used during post-core sync, so maintainers need upgrade-safety evidence before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #77616
Summary: This PR is a candidate fix for the open plugin update diagnostic issue, but the current patch is not yet a safe landing 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: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • Route metadata failures through centralized failure handling or preserve equivalent disableOnFailure cleanup.
  • Update the fallback warning and focused regression test for the selected skip, retry, or fallback behavior.
  • [P1] Add redacted terminal output, logs, recording, or a linked artifact showing the after-fix openclaw update or post-core plugin update path.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and comments do not include after-fix terminal output, logs, recording, or a linked artifact for the plugin update path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P2] Merging as-is changes shipped npm metadata-failure behavior from installer fallback/failure handling to immediate skip, so post-core updates can leave an old or incompatible plugin enabled until a later run.
  • [P1] The new direct outcome bypasses recordFailure, so disableOnFailure: true cleanup for allow/deny lists and selected slots will not run for this failure mode.
  • [P1] The contributor has not provided redacted terminal output, logs, recording, or an artifact showing the after-fix openclaw update or post-core plugin update path.

Maintainer options:

  1. Preserve centralized failure cleanup (recommended)
    Route metadata-failure skips through recordFailure or an equivalent helper so disableOnFailure and config cleanup still apply when post-core updates cannot safely continue.
  2. Accept skip semantics intentionally
    Maintainers can choose immediate skip-on-metadata-failure behavior, but should require upgrade-safety tests and real after-fix proof because this changes shipped fallback behavior.
  3. Pause for resolver retry work
    If the desired product behavior is retry or no-output timeout handling, pause this PR and keep the linked issue as the canonical repair target.

Next step before merge

  • [P1] Human review and contributor action are needed because the blockers combine compatibility-sensitive update behavior with missing real behavior proof that automation cannot provide for the contributor.

Security
Cleared: The diff only changes plugin update control flow and does not add dependencies, workflows, package resolution inputs, permissions, or secret handling.

Review findings

  • [P1] Preserve failure cleanup on metadata skips — src/plugins/update.ts:1586-1596
  • [P2] Align the diagnostic with the new skip path — src/plugins/update.ts:1586-1596
Review details

Best possible solution:

Preserve centralized failure handling or implement bounded metadata retry/no-output handling, align diagnostics and regression tests with the chosen behavior, and require redacted real update-path proof before merge.

Do we have a high-confidence way to reproduce the issue?

Source-reproducible, but not live-reproduced here: current main has the metadata-failure warning/fallback path and an adjacent mocked test for npm view failure. The exact live transient registry failure and after-fix behavior still need redacted real proof.

Is this the best way to solve the issue?

No, not as written; early skip is a plausible mitigation, but the best fix must preserve centralized failure handling or implement bounded metadata retry behavior with matching tests and proof.

Full review comments:

  • [P1] Preserve failure cleanup on metadata skips — src/plugins/update.ts:1586-1596
    The added branch records an error and continues before the installer and recordFailure path can run. In post-core update callers with disableOnFailure: true, a transient metadata outage can now leave an old plugin enabled and skip the allow/slot cleanup that current failed updates apply.
    Confidence: 0.91
  • [P2] Align the diagnostic with the new skip path — src/plugins/update.ts:1586-1596
    This branch no longer falls back to the installer, but the warning immediately before it still says falling back to installer path; the adjacent test also asserts the old installer call. Please align the diagnostic and regression coverage with the selected skip, retry, or fallback behavior.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is an intermittent plugin update diagnostic and reliability fix with limited blast radius but real user confusion.
  • merge-risk: 🚨 compatibility: The diff changes shipped npm plugin update fallback and disable-on-failure behavior when registry metadata is unavailable.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body and comments do not include after-fix terminal output, logs, recording, or a linked artifact for the plugin update path. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +11. Total +11 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 11 0 +11
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 11 0 +11

What I checked:

  • PR diff adds early skip: The PR pushes an error outcome and continues immediately after npm metadata lookup fails, before the normal installer and failure-handling path can run. (src/plugins/update.ts:1586, 2ac4aa23269b)
  • Central failure handling: Current main routes update failures through recordFailure, which applies disableOnFailure, emits the disabled warning, mutates config, and records skipped/error outcomes. (src/plugins/update.ts:1263, a39e548ede22)
  • Cleanup that would be bypassed: disablePluginConfigEntry removes the failed plugin from allow/deny lists and resets memory/context-engine slots before marking the entry disabled. (src/plugins/update.ts:1152, a39e548ede22)
  • Post-core caller opts into cleanup: The post-core update path calls updateNpmInstalledPlugins with disableOnFailure: true, so metadata failure behavior affects upgrade recovery. (src/cli/update-cli/update-command.ts:1880, a39e548ede22)
  • Existing test locks current fallback behavior: The current update test suite asserts that metadata probing failure logs the fallback-to-installer warning and calls the npm installer once. (src/plugins/update.test.ts:1862, a39e548ede22)
  • Installer validates before metadata resolution: installPluginFromNpmSpec validates the registry spec first, then calls resolveNpmSpecMetadata, which explains why falling through can surface validation-layer errors. (src/plugins/install.ts:2846, a39e548ede22)

Likely related people:

  • vincentkoc: Current blame and recent GitHub history tie this handle to the central plugin update failure path and post-core update recovery work. (role: recent area contributor; confidence: high; commits: d17045db6fa2, 767e8280ac7c, 62fad3da8606; files: src/plugins/update.ts, src/plugins/update.test.ts, src/cli/update-cli/update-command.ts)
  • steipete: Recent history shows repeated work on npm metadata utilities, plugin install/update diagnostics, and update warning behavior around this flow. (role: install/update infrastructure contributor; confidence: high; commits: 97d1f5fd153c, 2eaf8ad7126b, 738f94b29022; files: src/infra/install-source-utils.ts, src/plugins/install.ts, src/plugins/update.ts)
  • IWhatsskill: Authored the recent failed-update cleanup change in the same updater path that this PR currently bypasses. (role: adjacent cleanup contributor; confidence: high; commits: 5214f16e2959; files: src/plugins/update.ts, src/plugins/update.test.ts)
  • BKF-Gitty: Merged history shows work on plugin channel fallback messaging and failed fallback metadata in the same update flow. (role: recent update fallback contributor; confidence: medium; commits: c96a12d3c887; files: src/plugins/update.ts, src/plugins/update.test.ts, src/cli/update-cli/update-command.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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 21, 2026
@lsr911

lsr911 commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Closing after 7 days with no review activity. This PR fixed spurious npm spec error after transient metadata fetch failure. Will re-open if reviewer feedback becomes available.

@lsr911 lsr911 closed this Jun 21, 2026
@lsr911
lsr911 deleted the fix/issue-77616-Bug-Plugin-update-emits-spu branch June 22, 2026 00:46
@brokemac79 brokemac79 mentioned this pull request Jun 23, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Plugin update emits spurious 'unsupported npm spec' error after transient npm view failure

1 participant

@lsr911