fix(plugins): align install/discovery paths and add legacy migration - #7549
Conversation
`zeroclaw plugin install` wrote to `config.data_dir/plugins` while the runtime,
gateway, and skill discovery scanned `config.plugins.plugins_dir`, so CLI-installed
WASM plugins were invisible to the agent ("Loaded 0 WASM plugin tools").
Single-source the directory via `PluginsConfig::resolved_plugins_dir()` +
`PluginHost::from_plugins_dir` so install and discovery agree (also fixing custom
`plugins_dir` values that the old `parent()` logic ignored), and add an opt-in
`plugin migrate` command plus a detection hint to relocate plugins stranded in the
legacy locations.
Closes zeroclaw-labs#6254.
github-actions
Bot
added
core
CI clippy runs with `-D warnings`, which flags the `Config::default()` + field-reassignment pattern in the `config_with_dirs` test helper added for the plugin legacy-dir detection tests. Build the struct with update syntax instead.
singlerider
left a comment
singlerider
left a comment
There was a problem hiding this comment.
Full disclosure first: I have a draft PR (#7413) attacking the same divergence. Having read this one, it's the better shape: it makes the configured plugins_dir the single truth rather than imposing a new location, and the migration is opt-in with a visible nudge instead of automatic. I'll rework my draft down to whatever doesn't overlap. Approving on merits.
🟢 The fix removes the bug class, not just the bug
PluginsConfig::resolved_plugins_dir() as the one resolver, plus PluginHost::from_plugins_dir taking the already-resolved path, deletes three hand-rolled tilde expanders and the parent()-then-re-append dance (which silently broke any custom plugins_dir not ending in plugins/, a latent bug this PR fixes in passing and documents). CLI, runtime tools, skills, and gateway now provably resolve through one function.
🟢 The round-trip test encodes the original complaint
install_then_discover_round_trip_uses_same_dir is literally #6254 as a test: install via one host, discover via a fresh host over the same resolved dir. And new_still_appends_plugins_subdir pins the legacy constructor contract so existing callers of new/with_security can't be silently rebased onto the new semantics.
🟢 Migration is conservative in every direction
Never clobbers an existing target plugin, rename with copy+remove fallback for cross-filesystem moves (remove only after the copy succeeds), missing/empty source is a no-op, and a directory only counts as legacy when it both differs from the target and actually contains a manifest.toml. The legacy_plugin_dirs_skips_dir_equal_to_target test covers the operator who pointed plugins_dir at the old location, where migration would otherwise eat its own tail.
🟢 Stranded plugins are loud now
The WARN at tool-discovery time plus the plugin list stderr note means the failure mode this PR fixes (silently invisible plugins) can't recur silently even for users who never run migrate. Fluent keys for all three new messages, en-fallback safe.
One observation, no action needed: legacy_plugin_dirs_with_entries doing filesystem probes from zeroclaw-config is mildly unusual for that crate, but it's the natural home given Config owns both candidate paths, and it's read-only.
WareWolf-MoonWall
left a comment
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
The architecture here is exactly right — PluginsConfig::resolved_plugins_dir() as a single source of truth, everything else calling it, no hand-rolled tilde expanders, and no path re-derivation. One localization gap to close before merge.
🔴 Blocking — Three new Fluent keys are English-only
crates/zeroclaw-runtime/locales/en/cli.ftl adds three new user-facing keys:
cli-plugin-legacy-detectedcli-plugin-migratedcli-plugin-migrate-none
The other four locale files (es/cli.ftl, fr/cli.ftl, ja/cli.ftl, zh-CN/cli.ftl) were not touched. All four already contain the existing plugin locale keys (cli-plugin-installed-from, cli-plugin-removed, cli-plugin-not-found, etc.), so the missing entries are not a "never translated" category — they're an omission. Per AGENTS.md, all user-facing CLI output must use fl!() / Fluent strings, and the Fluent catalog sync expectation is that new English keys get corresponding entries in all maintained locales. Add the three keys to all four non-English files. Placeholder values matching the English copy are acceptable if translation is not yet available.
🟢 What looks good — SOT design is correct and well-exercised
PluginsConfig::resolved_plugins_dir() is pure (no I/O), documented as the canonical source, and every consuming site in api_plugins.rs, skills/mod.rs, tools/mod.rs, and src/main.rs resolves through it. The PluginHost::from_plugins_dir constructor eliminates the old parent()-then-reappend dance — the bug where a custom plugins_dir not ending in plugins was silently misrouted is now structurally impossible.
🟢 What looks good — test coverage is thorough and honest
Five new schema tests cover absolute-path passthrough, tilde expansion, both legacy-location variants, and the edge case where the configured dir equals the legacy location. Five new host tests cover the from_plugins_dir behavior, the preserved new()/with_security() contract, the install-then-discover round-trip (the exact regression for #6254), and the migrate function's collision avoidance and no-op behavior. Reproducing the bug before and after in the Validation Evidence section, with actual log output, is strong evidence.
🟡 Warning — setup.bat parity not confirmed
The PR body notes Windows setup.bat was not tested (no Windows host). The install.sh path is well-verified; the batch parity is a reasonable extension of the same pattern. Low actual risk since plugin support is behind the plugins-wasm feature flag and the bat change mirrors the verified shell logic, but worth noting for the record.
Audacity88
left a comment
Audacity88
left a comment
There was a problem hiding this comment.
Checked current head b51c11f against #6254, the superseded #6746 / #7413 path-contract work, the prior reviews, the current diff, and visible CI. This is a comment review, not an approval, because @WareWolf-MoonWall's CHANGES_REQUESTED review is still active and should be cleared by that reviewer.
✅ Resolved — Locale catalog gap is addressed on current head
The latest i18n commit adds the three new CLI Fluent keys to all maintained runtime CLI locale catalogs:
cli-plugin-legacy-detectedcli-plugin-migratedcli-plugin-migrate-none
Those keys are now present in es, fr, ja, and zh-CN, so the blocking localization gap from the previous review is addressed in the current diff.
🟢 What looks good — Plugin directory resolution is now one path contract
PluginsConfig::resolved_plugins_dir() is the single resolver, and the CLI, runtime tool discovery, plugin skill discovery, and gateway plugin listing all route through PluginHost::from_plugins_dir / from_plugins_dir_with_security instead of deriving a parent path and appending plugins again. That fits the single-source-of-truth rule and fixes the exact #6254 split where plugin install and runtime discovery disagreed about where installed WASM plugins lived.
The regression coverage is also pointed at the right behavior: the install-then-discover round trip covers the user-visible failure, and the migration tests cover the legacy directories without clobbering existing target plugins.
🟡 Warning — Windows batch parity is still unproven locally
The PR body is transparent that setup.bat was not tested on a Windows host. I do not see that as a blocker for this PR because the verified path contract is behind plugins-wasm, CI builds Windows successfully, and the changed batch logic mirrors the shell setup path. It is still worth keeping in mind if follow-up installer testing turns up Windows-specific quoting or path behavior.
WareWolf-MoonWall
left a comment
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Re-review at b51c11f — converting my prior CHANGES_REQUESTED to an approval. My sole blocking finding is addressed; no new issues in the fixup commit.
✅ Resolved — Locale catalog gap is closed
The three new CLI Fluent keys are now present in all four non-English locale files:
cli-plugin-legacy-detectedcli-plugin-migratedcli-plugin-migrate-none
Each locale (es, fr, ja, zh-CN) carries a real translation, not an English-copy placeholder — the Spanish, French, Japanese, and Simplified Chinese strings all read as idiomatic localizations of the English source. The variable placeholders ({$path}, {$target}, {$count}) are consistent with the English keys and the surrounding catalog entries. That's the right way to close a catalog gap.
🟢 What looks good — nothing regressed in the fixup
The i18n commit touches only the four locale .ftl files. The path-contract changes from the original commits are untouched. CI is still green.
The 🟡 warning about setup.bat Windows parity from my prior review remains — @Audacity88 echoed it in their comment review. Neither of us considers it a blocker; I'm not re-raising it as a new finding here. It stands as prior art if follow-up installer testing surfaces a Windows-specific quoting issue.
Approving. The fix is correct, the locale coverage is complete, and the test suite encodes the original bug report.
singlerider
left a comment
singlerider
left a comment
There was a problem hiding this comment.
Re-affirming at head b51c11f3c. My 06-13 approval was on 286c22bd6; the only changes since are the locale commit that resolved @WareWolf-MoonWall's blocker (the three plugin-migrate Fluent keys are now present in all four non-English locales — confirmed es/fr/ja/zh-CN) and a master merge. Neither touches the architecture I approved.
The SOT design is unchanged and still correct: PluginsConfig::resolved_plugins_dir() as the single canonical (pure, no-I/O) resolver, every consumer (api_plugins.rs, skills/mod.rs, tools/mod.rs, src/main.rs) routing through it, and PluginHost::from_plugins_dir eliminating the old parent()-then-reappend dance that misrouted custom plugins_dir values — the #6254 regression is structurally impossible now. The schema/host test coverage (tilde/absolute/legacy-variants, install-then-discover round-trip, migrate collision/no-op) is thorough.
Both reviewers have approved at this head and the branch is MERGEABLE / CLEAN. Approval carries forward. (The setup.bat Windows parity remains untested per the body, but it's low-risk behind the plugins-wasm feature flag and mirrors the verified shell logic.)
|
@alanpjohn make sure your setup.bat changes will not interfere with what's on current upstream/master. We're generating a lot of the file at time of release now. |
|
@singlerider this PR doesn't touch |
4 participants
Summary
masterzeroclaw plugin installwrote toconfig.data_dir/plugins, but the agent runtime, gateway, and skill discovery scannedconfig.plugins.plugins_dir. The two never matched, so CLI-installed WASM plugins were silently invisible to the agent (Loaded 0 WASM plugin tools) even thoughplugin list/infoshowed them installed. Reproduced on currentmaster.PluginsConfig::resolved_plugins_dir()is now the one source of truth; the CLI, runtime, skills, and gateway all resolve through it via a newPluginHost::from_plugins_dir. This also fixes a latent bug where a customplugins_dirnot ending inpluginswas silently ignored by the oldparent()-then-re-append dance, and removes three hand-rolled tilde expanders.zeroclaw plugin migratecommand plus a detection warning (onplugin listand at agent startup) to relocate plugins stranded in the old locations.plugins.plugins_dirconfig schema/default, or per-request discovery behavior. No automatic/at-startup file mutation — migration is user-invoked only.GET …/pluginslisting — all behind theplugins-wasmbuild feature and gated onplugins.enabled. No database, network, auth, or channel changes.bug,config,plugins,runtime,gateway,core,size: M,risk: low(maintainers to confirm; rollback fields filled below in caserisk: mediumis preferred).Validation Evidence
$ cargo fmt --all -- --check # exit 0 — no diffs--features plugins-wasm):…/data/plugins; runtime scans…/plugins(nonexistent); agent startup logszc_attrs={"count":0}→ "Loaded 0 WASM plugin tools" whileplugin listshows the plugin.…/plugins; agent logszc_attrs={"count":1}→ "Loaded 1 WASM plugin tools".…/data/plugins,plugin listprints the legacy hint and agent startup logs the legacy WARN;plugin migrate→ "Moved 1 plugin(s) …"; re-run → "Nothing to migrate." (idempotent); the relocated plugin then loads.Security & Privacy Impact
plugin migratereads the legacy plugin dirs and writes to the configured plugin dir, all within the existing ZeroClaw data/config tree thatplugin install/removealready read and write.Compatibility
zeroclaw plugin migratesubcommand. No config schema change;plugins.plugins_dir(default~/.zeroclaw/plugins) is unchanged but is now actually honored for custom values.…/data/plugins(or pre-rename…/workspace/plugins) and were already invisible to the agent. Runzeroclaw plugin migrateonce to relocate them (orzeroclaw plugin install <source>again). Bothplugin listand agent startup now print a hint when stranded plugins are detected.Rollback
git revert 464f1538fplugins-wasmbuild feature and gated onplugins.enabled; no runtime toggle for the fix itself.Loaded 0 WASM plugin toolsat agent startup despitezeroclaw plugin listshowing installed plugins; or aWARN … run \zeroclaw plugin migrate`` that persists after migrating.