Skip to content

fix(plugins): keep load-path plugins with installed index#99409

Closed
qingminglong wants to merge 3 commits into
openclaw:mainfrom
qingminglong:codex/99185-plugin-load-paths
Closed

fix(plugins): keep load-path plugins with installed index#99409
qingminglong wants to merge 3 commits into
openclaw:mainfrom
qingminglong:codex/99185-plugin-load-paths

Conversation

@qingminglong

@qingminglong qingminglong commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #99185

What Problem This Solves

Fixes an issue where users with workspace plugins configured through plugins.load.paths would lose those plugins after a managed npm plugin populated the installed plugin index. The dropped workspace plugins then surfaced as stale plugin not found config warnings even though their files were still present.

Why This Change Was Made

The installed-index manifest registry reconstruction now merges configured plugins.load.paths discovery candidates with the persisted installed-index candidates instead of treating the index as the only candidate source. The configured load-path scanner is shared with normal discovery so the same load-path validation and bundled-path diagnostics apply without broadening the installed-index path to bundled/global auto-discovery.

User Impact

Users can install or enable managed npm plugins without breaking existing workspace plugins loaded from plugins.load.paths. Gateway startup and CLI config validation should continue to see both managed installed plugins and configured workspace plugins.

Evidence

Current head: a9a484b5dd20609fdd757842b759ba6ff8c014d3.

  • Claim proved: installed-index registry reconstruction keeps explicitly configured plugins.load.paths workspace plugins alongside installed-index plugins, preserves scoped lookups, and keeps normal bundled-load-path filtering behavior.
  • Related PR quality gate: fix(plugins): discover config load.paths plugins when index has entries #99196 is an open proof-positive competing PR for Installing a managed npm plugin breaks resolution of all plugins.load.paths workspace plugins ("plugin not found: stale config entry ignored") #99185. This branch now includes the same critical behavior coverage points so maintainers can compare the implementations directly.
  • Focused validation:
    • node scripts/run-vitest.mjs src/plugins/manifest-registry-installed.test.ts
    • Result: passed, 1 file / 20 passed / 1 skipped.
    • git diff --check
    • Result: clean.
  • Behavior covered by the focused registry test suite:
    • Installed-index plugin plus plugins.load.paths workspace plugin both appear in the reconstructed registry.
    • Empty plugins.load.paths stays a no-op and returns only the installed-index plugin.
    • pluginIds: ["workspace"] scoped lookup returns the configured workspace plugin without leaking unrelated load-path plugins or installed-index plugins.
    • Redundant load paths pointing at OpenClaw bundled plugin directories are skipped with the existing ignored plugins.load.paths entry warning.
  • Earlier supporting evidence from this PR:
    • node_modules\.bin\oxlint --tsconfig config/tsconfig/oxlint.core.json src/plugins/manifest-registry-installed.ts src/plugins/manifest-registry-installed.test.ts passed.
    • node scripts/run-vitest.mjs src/plugins/manifest-registry-installed.test.ts previously passed: 17 passed, 1 skipped.
    • node scripts/run-vitest.mjs src/config/config.plugin-validation.test.ts completed all assertions with 57 passed, 1 skipped, then failed during Windows temp SQLite cleanup with EBUSY; no test assertion failed after the code fix.
  • Not tested / proof gaps:

@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: #99196 is an open, non-draft, mergeable, proof-positive canonical PR for the same installed-index plus plugins.load.paths regression, while this PR has similar source/test changes but still lacks real behavior proof and no unique merge-worthy remainder.

Root-cause cluster
Relationship: superseded
Canonical: #99196
Summary: This PR, #99196, and the linked issue all target the same installed-index plus plugins.load.paths regression; the sibling PR is the proof-positive canonical landing path.

Members:

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

Canonical path: Use #99196 as the canonical landing path for #99185 and close this duplicate branch.

So I’m closing this here and keeping the remaining discussion on #99196 and #99185.

Review details

Best possible solution:

Use #99196 as the canonical landing path for #99185 and close this duplicate branch.

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

Yes at source level: current main enters installed-index manifest reconstruction when the index has rows, and that caller supplies only index candidates to a callee that skips normal discovery when candidates are present. I did not run the full managed npm install plus gateway restart flow in this read-only review.

Is this the best way to solve the issue?

No for this PR as the landing path: the code shape is plausible, but #99196 is the better canonical solution because it covers the same issue with accepted real behavior proof. The best maintainer path is to review or land the canonical PR, not keep duplicate branches open.

Security review:

Security review cleared: No concrete security or supply-chain concern was found; the diff touches plugin discovery/registry TypeScript and colocated tests only.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md and the scoped plugin guide were read; plugin registry/config loading is compatibility-sensitive and reviews must check callers, callees, sibling paths, tests, and proof. (AGENTS.md:1, 776b34a4e642)
  • Current-main bug path: Current main reconstructs an installed-index manifest registry from params.index.plugins candidates only, so configured load-path plugins absent from the index are not rediscovered there. (src/plugins/manifest-registry-installed.ts:621, 776b34a4e642)
  • Runtime entry point: Plugin metadata snapshot loading chooses loadPluginManifestRegistryForInstalledIndex whenever the normalized installed index has plugin rows, matching the managed-plugin index branch reported in the linked issue. (src/plugins/plugin-metadata-snapshot.ts:699, 776b34a4e642)
  • Callee contract: loadPluginManifestRegistry treats supplied candidates as complete and calls normal discovery only when candidates are absent, so the installed-index caller owns adding load-path candidates. (src/plugins/manifest-registry.ts:958, 776b34a4e642)
  • This PR diff: This PR factors configured load-path scanning into discoverConfiguredPluginLoadPaths, calls it from installed-index reconstruction, and adds focused registry tests; the PR body reports test/lint proof but explicitly says no full live npm registry install plus gateway restart was run. (src/plugins/manifest-registry-installed.ts:614, a9a484b5dd20)
  • Canonical sibling PR: Live GitHub data shows fix(plugins): discover config load.paths plugins when index has entries #99196 is open, non-draft, mergeable, labeled proof: sufficient, and its body includes copied after-fix terminal output for mixed installed-index/load-path discovery and scoped lookup behavior. (79009941c2d2)

Likely related people:

  • steipete: Current blame for the installed-index registry function and configured load-path discovery block points to commit bb72443, and live PR metadata shows this commit came from and was merged through feat: session-first sidebar, compact context ring, and warm light theme for the Control UI #99289 by this handle. (role: recent area contributor and merger; confidence: high; commits: bb724436150e; files: src/plugins/manifest-registry-installed.ts, src/plugins/discovery.ts)
  • vincentkoc: Git history for the installed-index registry and discovery symbols shows release-history commit e085fa1 carrying these paths before the current-main merge, so this handle is adjacent for shipped/current behavior context. (role: release-history adjacent contributor; confidence: medium; commits: e085fa1a3ffd; files: src/plugins/manifest-registry-installed.ts, src/plugins/discovery.ts, src/plugins/plugin-metadata-snapshot.ts)

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

@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 Jul 3, 2026
@qingminglong

qingminglong commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

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.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 3, 2026
@qingminglong

qingminglong commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 3, 2026 •
edited
Loading

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.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 4, 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: M 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.

Installing a managed npm plugin breaks resolution of all plugins.load.paths workspace plugins ("plugin not found: stale config entry ignored")

1 participant

@qingminglong