feat(plugin): implement plugin infrastructure with Extism WASM loader#12
Merged
feat(plugin): implement plugin infrastructure with Extism WASM loader#12
Conversation
Add the plugin adapter layer implementing the PluginLoader and PluginReadRepository ports with Extism for WASM module loading, DashMap-backed registry, and notify-based hot-reload. - Manifest parser for plugin.toml (category, capabilities, version) - ExtismPluginLoader with atomic insert, 100MB size limit, panic capture - PluginRegistry with DashMap + Mutex<Plugin> for concurrent access - Hot-reload watcher via notify with tokio async event processing - CQRS handlers: Install, Uninstall, Enable, Disable, ListPlugins - Tauri IPC wiring with path traversal protection on plugin_install - Domain: Container/Notifier categories, PluginError variant - 39 new tests (267 total), clippy clean
github-actions
bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a WASM plugin subsystem: plugin.toml manifest parsing and wasm discovery, an Extism-based plugin loader with size/name/path safeguards, a concurrent DashMap-backed registry, a notify-based hot-reload watcher, CQRS commands/queries and Tauri IPC handlers for plugin lifecycle, and two new plugin categories (Container, Notifier). Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Tauri Frontend)
participant Tauri as Tauri IPC
participant CmdBus as Command Bus
participant Loader as ExtismPluginLoader
participant Registry as PluginRegistry
participant Event as Event Bus
Client->>Tauri: plugin_install(path)
Tauri->>Tauri: canonicalize & validate path
Tauri->>Tauri: parse_manifest(dir)
Tauri->>CmdBus: InstallPluginCommand(manifest)
CmdBus->>Loader: spawn_blocking load(manifest)
Loader->>Loader: find_wasm_file & validate name/size
Loader->>Loader: instantiate extism::Plugin
Loader->>Registry: try_insert LoadedPlugin (atomic)
Registry-->>Loader: OK / AlreadyExists
Loader-->>CmdBus: Result
CmdBus->>Event: publish PluginLoaded (on success)
CmdBus-->>Tauri: Result
Tauri-->>Client: Success/Error
Client->>Tauri: plugin_list()
Tauri->>CmdBus: ListPluginsQuery
CmdBus->>Registry: list_loaded()
Registry-->>CmdBus: Vec<PluginInfo>
CmdBus-->>Tauri: Vec<PluginViewDto>
Tauri-->>Client: Plugin list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces a complete WASM plugin infrastructure via the Extism SDK: manifest parsing, an in-memory DashMap registry, hot-reload file watching, five CQRS handlers, and five Tauri IPC commands. The architecture is well-layered and the test coverage is solid (39 new tests), but one security gap and two behavioural discrepancies need attention before merge.
Confidence Score: 4/5Safe to merge after fixing the non-canonicalized allowed_parent in plugin_install; remaining findings are P2. One P1 security finding (path traversal guard uses non-canonical prefix) keeps the score at 4. The two P2 issues (no-op enable/disable and shard-guard lifetime) are documented or low-risk in the current call graph. Everything else — manifest parsing, atomic registry, hot-reload watcher, CQRS handlers, 39 new tests — is well-implemented. src-tauri/src/adapters/driving/tauri_ipc.rs (plugin_install path guard), src-tauri/src/application/commands/toggle_plugin.rs (no-op enable/disable)
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driving/tauri_ipc.rs | Adds 5 plugin IPC commands; path traversal guard has a gap — allowed_parent is not canonicalized, so the starts_with check against the user-supplied canonical path is unreliable on symlinked config directories. |
| src-tauri/src/adapters/driven/plugin/registry.rs | DashMap-backed registry with atomic insert and enable/disable; call_plugin holds the shard read-guard through the full Mutex acquisition and WASM call, contrary to its own comment. |
| src-tauri/src/adapters/driven/plugin/extism_loader.rs | Implements PluginLoader with 100 MB size cap and TOCTOU-safe insert; load() re-parses the manifest from disk to locate the WASM file instead of using information already in the passed manifest. |
| src-tauri/src/adapters/driven/plugin/manifest.rs | Clean plugin.toml parser with full category validation, capability building, and well-covered tests. |
| src-tauri/src/adapters/driven/plugin/watcher.rs | Hot-reload watcher using notify + tokio; handles create/modify/remove events correctly and unloads on both plugin.toml and .wasm removal. |
| src-tauri/src/application/commands/toggle_plugin.rs | Enable/disable handlers validate plugin existence but do not call set_enabled — IPC returns Ok with no state change, documented as MVP but silently misleading to callers. |
| src-tauri/src/application/commands/install_plugin.rs | Clean install handler: loads via port and publishes PluginLoaded event; no event emitted on failure. |
| src-tauri/src/application/queries/list_plugins.rs | Simple query handler mapping PluginInfo to PluginViewDto via PluginReadRepository; well-tested. |
| src-tauri/src/domain/model/plugin.rs | Adds Container and Notifier variants to PluginCategory; clean domain model with builder-style PluginManifest. |
| src-tauri/src/domain/error.rs | Adds PluginError(String) variant to DomainError; Display impl and tests updated correctly. |
Sequence Diagram
sequenceDiagram
participant FE as Frontend
participant IPC as tauri_ipc (plugin_install)
participant CB as CommandBus
participant Loader as ExtismPluginLoader
participant Manifest as parse_manifest
participant Registry as PluginRegistry
participant EB as EventBus
participant Watcher as PluginWatcher
FE->>IPC: plugin_install(path)
IPC->>IPC: canonicalize(path)
IPC->>IPC: starts_with(allowed_parent) [guard]
IPC->>Manifest: parse_manifest(canonical)
Manifest-->>IPC: (PluginManifest, wasm_path)
IPC->>CB: handle_install_plugin(manifest)
CB->>Loader: load(manifest)
Loader->>Manifest: parse_manifest(plugin_dir) [re-parse for wasm_path]
Manifest-->>Loader: (_, wasm_path)
Loader->>Loader: fs::metadata + size check
Loader->>Loader: fs::read(wasm_path)
Loader->>Registry: try_insert(name, LoadedPlugin)
Registry-->>Loader: true / false
Loader-->>CB: Ok / AlreadyExists
CB->>EB: publish(PluginLoaded)
CB-->>IPC: Ok
IPC-->>FE: Ok
Note over Watcher,Registry: Concurrent hot-reload path
Watcher->>Loader: load(manifest) on Create/Modify
Watcher->>Loader: unload(name) on Remove
Reviews (1): Last reviewed commit: "feat(plugin): implement plugin infrastru..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+152
to
+161
| let allowed_parent = dirs::config_dir() | ||
| .unwrap_or_else(|| std::path::PathBuf::from(".")) | ||
| .join("vortex") | ||
| .join("plugins"); | ||
| if !canonical.starts_with(&allowed_parent) { | ||
| return Err(format!( | ||
| "plugin path must be under {}", | ||
| allowed_parent.display() | ||
| )); | ||
| } |
There was a problem hiding this comment.
allowed_parent undermines path-traversal guard
canonical is the fully-resolved, symlink-free path of the user input, but allowed_parent is built from dirs::config_dir() which is not canonicalized. On systems where the config directory is reached through symlinks (e.g. a custom $HOME or XDG_CONFIG_HOME pointing elsewhere), canonical.starts_with(&allowed_parent) can return false for legitimate paths or true for paths that are actually outside the intended prefix — depending on the direction of the symlink. Both sides of starts_with must be in the same resolved form for the check to be reliable.
let allowed_parent = dirs::config_dir()
.unwrap_or_else(|| std::path::PathBuf::from("."))
.join("vortex")
.join("plugins");
// Canonicalize allowed_parent too (create dir first if needed so canonicalize succeeds)
std::fs::create_dir_all(&allowed_parent)
.map_err(|e| format!("cannot create plugins dir: {e}"))?;
let allowed_parent = allowed_parent
.canonicalize()
.map_err(|e| format!("cannot resolve plugins dir: {e}"))?;
if !canonical.starts_with(&allowed_parent) {
Comment on lines
+75
to
+90
| pub fn call_plugin(&self, name: &str, func: &str, input: &str) -> Result<String, DomainError> { | ||
| // Get the Mutex reference, then drop the DashMap shard guard before locking. | ||
| // This prevents holding the shard during potentially slow WASM execution. | ||
| let entry = self | ||
| .plugins | ||
| .get(name) | ||
| .ok_or_else(|| DomainError::NotFound(name.to_string()))?; | ||
| let plugin_mutex = &entry.plugin; | ||
| let mut plugin = plugin_mutex | ||
| .lock() | ||
| .map_err(|_| DomainError::PluginError(format!("plugin '{name}' mutex poisoned")))?; | ||
| let result = plugin | ||
| .call::<&str, &str>(func, input) | ||
| .map_err(|e| DomainError::PluginError(format!("plugin call failed: {e}")))?; | ||
| Ok(result.to_string()) | ||
| } |
There was a problem hiding this comment.
The comment on line 76 says the shard guard is dropped before locking the Mutex, but this does not happen. plugin_mutex is a reference &entry.plugin, so entry (the DashMap read-guard) is kept alive until the end of the function — through the full duration of plugin.call(...). Any concurrent writer that hashes to the same DashMap shard will block for the entire WASM call duration. While an actual deadlock requires a WASM callback to trigger a shard write (unlikely here), the performance/correctness contract described in the comment is not met.
To actually release the shard guard first, store plugin as Arc<Mutex<extism::Plugin>> so it can be cloned out before entry is dropped:
// In LoadedPlugin:
pub plugin: Arc<Mutex<extism::Plugin>>,
// In call_plugin:
let plugin_mutex = self.plugins
.get(name)
.ok_or_else(|| DomainError::NotFound(name.to_string()))?
.plugin
.clone(); // clone the Arc, releases shard guard immediately
let mut plugin = plugin_mutex.lock()...
Comment on lines
+11
to
+38
| impl CommandBus { | ||
| pub async fn handle_enable_plugin( | ||
| &self, | ||
| cmd: super::EnablePluginCommand, | ||
| ) -> Result<(), AppError> { | ||
| let plugins = self.plugin_loader().list_loaded()?; | ||
| if !plugins.iter().any(|p| p.name() == cmd.name) { | ||
| return Err(AppError::NotFound(format!( | ||
| "plugin '{}' not loaded", | ||
| cmd.name | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn handle_disable_plugin( | ||
| &self, | ||
| cmd: super::DisablePluginCommand, | ||
| ) -> Result<(), AppError> { | ||
| let plugins = self.plugin_loader().list_loaded()?; | ||
| if !plugins.iter().any(|p| p.name() == cmd.name) { | ||
| return Err(AppError::NotFound(format!( | ||
| "plugin '{}' not loaded", | ||
| cmd.name | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
plugin_enable / plugin_disable IPC commands are silent no-ops
Both handlers confirm the plugin is loaded and return Ok(()) — they never call PluginRegistry::set_enabled. The resolve_url path in ExtismPluginLoader filters on info.is_enabled(), so disabled plugins will still respond to URL resolution until the process restarts. The file-level comment acknowledges this is MVP, but callers over IPC receive a success response with no observable effect, which is likely to surface as a confusing bug during integration testing.
Consider either wiring through a PluginManager port now (since PluginRegistry::set_enabled already exists), or returning a not-implemented error until the port is introduced, so callers know the operation had no effect.
Comment on lines
+36
to
+72
| fn load(&self, manifest: &PluginManifest) -> Result<(), DomainError> { | ||
| let name = manifest.info().name().to_string(); | ||
|
|
||
| let plugin_dir = self.plugins_dir.join(&name); | ||
| let (_, wasm_path) = parse_manifest(&plugin_dir)?; | ||
|
|
||
| const MAX_WASM_SIZE: u64 = 100 * 1024 * 1024; // 100 MB | ||
| let metadata = std::fs::metadata(&wasm_path).map_err(|e| { | ||
| DomainError::PluginError(format!("failed to stat wasm {}: {e}", wasm_path.display())) | ||
| })?; | ||
| if metadata.len() > MAX_WASM_SIZE { | ||
| return Err(DomainError::PluginError(format!( | ||
| "wasm file {} exceeds 100 MB limit ({} bytes)", | ||
| wasm_path.display(), | ||
| metadata.len() | ||
| ))); | ||
| } | ||
| let wasm_bytes = std::fs::read(&wasm_path).map_err(|e| { | ||
| DomainError::PluginError(format!("failed to read wasm {}: {e}", wasm_path.display())) | ||
| })?; | ||
|
|
||
| let extism_manifest = extism::Manifest::new([extism::Wasm::data(wasm_bytes)]); | ||
| let plugin = extism::Plugin::new(&extism_manifest, [], true) | ||
| .map_err(|e| DomainError::PluginError(format!("failed to load plugin: {e}")))?; | ||
|
|
||
| let loaded = LoadedPlugin { | ||
| manifest: manifest.clone(), | ||
| plugin: std::sync::Mutex::new(plugin), | ||
| enabled: true, | ||
| }; | ||
|
|
||
| // Atomic insert-if-absent via DashMap::entry() | ||
| if self.registry.try_insert(name.clone(), loaded) { | ||
| Ok(()) | ||
| } else { | ||
| Err(DomainError::AlreadyExists(name)) | ||
| } |
There was a problem hiding this comment.
load() silently ignores the passed manifest and re-parses from disk
load receives a PluginManifest (already validated by the IPC layer), then immediately calls parse_manifest(&plugin_dir) again to locate the .wasm file, discarding the result and only keeping wasm_path. If the plugin directory contains multiple .wasm files, the loaded binary is determined by filesystem iteration order (non-deterministic), which may not match the manifest's declared binary.
Consider storing the WASM file name in PluginManifest during parsing (e.g. wasm_file: PathBuf) and using that path in load() to guarantee the correct binary is loaded.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src-tauri/src/application/commands/uninstall_plugin.rs (1)
24-228: Extract shared command-bus test fixtures to reduce drift.Most mocks/fixture setup here are duplicated in
src-tauri/src/application/commands/install_plugin.rs. A shared test helper module would cut maintenance overhead and keep behavior changes synchronized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/uninstall_plugin.rs` around lines 24 - 228, Tests for uninstall_plugin.rs duplicate many mocks and the make_bus helper also used in install_plugin.rs; extract these shared fixtures into a common test helper module (e.g., tests::fixtures) and update both files to import and reuse it. Move MockDownloadRepo, MockDownloadEngine, MockEventBus, MockFileStorage, MockHttpClient, MockPluginLoader, MockConfigStore, MockCredentialStore, MockClipboardObserver, and make_bus into that shared module, keep their APIs identical, and change references in uninstall_plugin tests and install_plugin tests to use the shared fixtures (preserve types like CommandBus and PluginLoader in signatures).src-tauri/src/domain/error.rs (1)
60-62: Add a regression test for the newPluginErrordisplay path.Line 60 introduces a new formatted branch, but this module doesn’t currently assert its string output. A small unit test will lock this contract and prevent silent message drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/domain/error.rs` around lines 60 - 62, Add a unit test that exercises the Display implementation for DomainError::PluginError: construct DomainError::PluginError with a sample message, call format!("{}", error) (or error.to_string()), and assert the resulting string equals "Plugin error: <your message>" to lock the output contract of the fmt::Display branch handling PluginError.src-tauri/src/adapters/driven/plugin/extism_loader.rs (2)
82-98: Non-deterministic plugin resolution order.
DashMap::iter()does not guarantee iteration order. If multiple plugins can handle the same URL, the "winning" plugin will be inconsistent across invocations. This could lead to confusing behavior for users.Consider documenting this limitation or implementing a deterministic ordering strategy (e.g., by plugin name or a priority field in the manifest).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs` around lines 82 - 98, resolve_url currently iterates registry.list_info() which is non-deterministic (DashMap::iter), causing inconsistent plugin selection when multiple plugins can_handle the same URL; fix by collecting the PluginInfo items from self.registry.list_info(), filter by info.is_enabled(), then sort the Vec deterministically (e.g., by info.name() or a manifest priority field if available) before iterating and calling self.registry.call_plugin(&name, "can_handle", url); alternatively add a comment documenting the non-deterministic behavior if you prefer not to change ordering.
36-41: Consider avoiding redundant manifest parsing.The
manifestparameter is already parsed and passed in, butparse_manifestis called again on line 40 just to obtain thewasm_path. This results in re-reading and re-parsingplugin.tomlfrom disk unnecessarily.Consider either:
- Storing the
wasm_pathinPluginManifestso it's available without re-parsing- Deriving the wasm path directly:
plugin_dir.join(format!("{}.wasm", name))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs` around lines 36 - 41, The load method currently re-parses plugin.toml via parse_manifest just to get wasm_path; remove that redundant parse and derive the .wasm path from the provided PluginManifest instead. In the load(&self, manifest: &PluginManifest) implementation, stop calling parse_manifest(&plugin_dir) and either (A) read a new wasm_path field off PluginManifest (add a wasm path accessor/property to PluginManifest and use that), or (B) compute the path as plugin_dir.join(format!("{}.wasm", name)) using the existing name, then use that wasm_path for subsequent operations (remove the parse_manifest call and the unused tuple binding). Ensure references to parse_manifest, wasm_path, plugin_dir, plugins_dir, name, and load are updated accordingly.src-tauri/src/adapters/driven/plugin/registry.rs (1)
28-42: Markinsertmethod as test-only or document its purpose.The
insertmethod is only used within test code (lines 140, 153, 165, 172, 202). Sincetry_insertis the production API enforcing atomic duplicate-checking, consider either adding#[cfg(test)]toinsertor documenting why both methods are exposed publicly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/registry.rs` around lines 28 - 42, The public method insert on Registry is only used in tests while try_insert is the intended production API; either make insert test-only or document its purpose. Locate the insert method (fn insert(&self, name: String, loaded: LoadedPlugin)) and either add a test-only attribute so it’s compiled only for tests (#[cfg(test)]) or add a brief doc comment explaining why insert is exposed alongside try_insert (e.g., for non-atomic test setup), and ensure references to self.plugins and LoadedPlugin are preserved; update any test imports if you make it test-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Around line 109-120: The find_wasm_file function currently returns the first
.wasm from std::fs::read_dir which is non-deterministic; change find_wasm_file
to collect all entries whose extension is "wasm" (e.g., into a Vec<PathBuf>),
then if exactly one match return it, otherwise return a DomainError::PluginError
explaining either "no .wasm found" or "multiple .wasm files found" and include
the list of candidates to make the failure deterministic; update any callers to
handle this explicit error.
In `@src-tauri/src/adapters/driven/plugin/registry.rs`:
- Around line 75-90: The comment is wrong because the DashMap shard guard (the
`entry` from `self.plugins.get(name)`) is still held while `plugin_mutex.lock()`
and the WASM `call()` run; update `call_plugin` so the shard guard is dropped
before locking by extracting or cloning a reference to the mutex out of the
entry and letting `entry` go out of scope first—e.g., obtain the plugin handle
(an Arc/cloneable handle such as `Arc<Mutex<...>>`) from `entry.plugin`, drop
`entry`, then call `lock()` and `call()` on the cloned handle; if `LoadedPlugin`
does not currently store a cloneable handle, change `LoadedPlugin::plugin` to
`Arc<Mutex<extism::Plugin>>` to allow cloning the handle and releasing the
DashMap shard before performing the expensive WASM execution in `call_plugin`.
In `@src-tauri/src/adapters/driven/plugin/watcher.rs`:
- Around line 84-98: Unloading by deriving the plugin key from
path.parent().file_name() is brittle; instead persist a mapping from plugin file
path to its registered plugin name when the plugin is loaded (e.g., update the
loader/registry at load time), and use that exact mapping to look up the plugin
name during EventKind::Remove rather than inferring from the directory;
additionally, during install/parse (where the manifest is read) add a check to
either enforce directory-name == manifest.info().name() or reject the install
and surface a clear error so mismatched directories cannot produce a registry
entry that won't be found by name.
- Around line 49-81: The watcher currently only reacts when is_plugin_toml(path)
is true; update the EventKind::Create(_) | EventKind::Modify(_) branch to also
react when the changed path is a .wasm file (e.g., path.extension() ==
Some("wasm")). For .wasm events, compute the plugin_dir = path.parent() and then
attempt to parse_manifest(plugin_dir) (same as the plugin.toml branch) and
proceed with unloading/loading via loader.unload(&name) and
loader.load(&manifest); reuse the existing parse_manifest, loader.load, and
loader.unload logic and logging paths so that changes to either plugin.toml or
the plugin .wasm trigger the same reload sequence.
In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 152-156: The current guard falls back to "." when
dirs::config_dir() is missing and compares against a non-canonical path; change
this to fail closed: when dirs::config_dir() returns None return an Err (or
early error) instead of using "."; build the expected plugin root by taking the
config_dir() PathBuf, joining "vortex/plugins", then canonicalize that plugin
root (e.g., canonicalize the PathBuf used for allowed_parent) and compare
canonical.starts_with(&canonical_allowed_parent) rather than the raw joined path
so the path traversal check uses fully-resolved paths (referencing the existing
allowed_parent variable and the canonical variable in this file).
In `@src-tauri/src/application/commands/install_plugin.rs`:
- Around line 11-16: The call to plugin_loader().load(&cmd.manifest) inside
handle_install_plugin blocks on std::fs operations (see
ExtismPluginLoader::load) and should be moved off Tokio worker threads; wrap the
synchronous load call in tokio::task::spawn_blocking (or otherwise run it on a
blocking thread) and await the JoinHandle result, propagating any errors into
the existing Result<AppError>; specifically modify handle_install_plugin to call
spawn_blocking(|| self.plugin_loader().load(cmd.manifest.clone())) (or
equivalent closure capturing needed data) and handle the returned Result so the
blocking filesystem work no longer runs on the async runtime threads.
In `@src-tauri/src/application/commands/toggle_plugin.rs`:
- Around line 12-37: The handlers handle_enable_plugin and handle_disable_plugin
currently only check that the plugin is loaded but don't change its enabled
state; update each to call the registry mutator (via self.plugin_loader() or
PluginRegistry) to set the enabled flag—e.g., after verifying presence call
PluginRegistry::set_enabled(plugin_name, true) in handle_enable_plugin and
PluginRegistry::set_enabled(plugin_name, false) in handle_disable_plugin,
propagate any errors from set_enabled as AppError, and return Ok(()) on success
so the IPC commands actually mutate plugin state.
In `@src-tauri/src/lib.rs`:
- Around line 57-61: Handlers plugin_install, plugin_uninstall, plugin_enable,
plugin_disable, and plugin_list require State<'_, AppState> but run() never
registers AppState in setup(); register the AppState before exposing these IPC
commands by adding app.manage(AppState::new() or AppState::default()) (or
equivalent AppState construction) inside the setup() closure (or on the Tauri
builder) so the State<'_, AppState> is available to those handlers when invoked.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs`:
- Around line 82-98: resolve_url currently iterates registry.list_info() which
is non-deterministic (DashMap::iter), causing inconsistent plugin selection when
multiple plugins can_handle the same URL; fix by collecting the PluginInfo items
from self.registry.list_info(), filter by info.is_enabled(), then sort the Vec
deterministically (e.g., by info.name() or a manifest priority field if
available) before iterating and calling self.registry.call_plugin(&name,
"can_handle", url); alternatively add a comment documenting the
non-deterministic behavior if you prefer not to change ordering.
- Around line 36-41: The load method currently re-parses plugin.toml via
parse_manifest just to get wasm_path; remove that redundant parse and derive the
.wasm path from the provided PluginManifest instead. In the load(&self,
manifest: &PluginManifest) implementation, stop calling
parse_manifest(&plugin_dir) and either (A) read a new wasm_path field off
PluginManifest (add a wasm path accessor/property to PluginManifest and use
that), or (B) compute the path as plugin_dir.join(format!("{}.wasm", name))
using the existing name, then use that wasm_path for subsequent operations
(remove the parse_manifest call and the unused tuple binding). Ensure references
to parse_manifest, wasm_path, plugin_dir, plugins_dir, name, and load are
updated accordingly.
In `@src-tauri/src/adapters/driven/plugin/registry.rs`:
- Around line 28-42: The public method insert on Registry is only used in tests
while try_insert is the intended production API; either make insert test-only or
document its purpose. Locate the insert method (fn insert(&self, name: String,
loaded: LoadedPlugin)) and either add a test-only attribute so it’s compiled
only for tests (#[cfg(test)]) or add a brief doc comment explaining why insert
is exposed alongside try_insert (e.g., for non-atomic test setup), and ensure
references to self.plugins and LoadedPlugin are preserved; update any test
imports if you make it test-only.
In `@src-tauri/src/application/commands/uninstall_plugin.rs`:
- Around line 24-228: Tests for uninstall_plugin.rs duplicate many mocks and the
make_bus helper also used in install_plugin.rs; extract these shared fixtures
into a common test helper module (e.g., tests::fixtures) and update both files
to import and reuse it. Move MockDownloadRepo, MockDownloadEngine, MockEventBus,
MockFileStorage, MockHttpClient, MockPluginLoader, MockConfigStore,
MockCredentialStore, MockClipboardObserver, and make_bus into that shared
module, keep their APIs identical, and change references in uninstall_plugin
tests and install_plugin tests to use the shared fixtures (preserve types like
CommandBus and PluginLoader in signatures).
In `@src-tauri/src/domain/error.rs`:
- Around line 60-62: Add a unit test that exercises the Display implementation
for DomainError::PluginError: construct DomainError::PluginError with a sample
message, call format!("{}", error) (or error.to_string()), and assert the
resulting string equals "Plugin error: <your message>" to lock the output
contract of the fmt::Display branch handling PluginError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98506cfe-d4ac-4324-89f1-0b3cdd054baa
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
CHANGELOG.mdsrc-tauri/Cargo.tomlsrc-tauri/src/adapters/driven/filesystem/file_storage.rssrc-tauri/src/adapters/driven/mod.rssrc-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driven/plugin/mod.rssrc-tauri/src/adapters/driven/plugin/registry.rssrc-tauri/src/adapters/driven/plugin/watcher.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/install_plugin.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/toggle_plugin.rssrc-tauri/src/application/commands/uninstall_plugin.rssrc-tauri/src/application/queries/list_plugins.rssrc-tauri/src/application/queries/mod.rssrc-tauri/src/domain/error.rssrc-tauri/src/domain/model/plugin.rssrc-tauri/src/lib.rs
💤 Files with no reviewable changes (1)
- src-tauri/src/adapters/driven/filesystem/file_storage.rs
Comment on lines
+84
to
+98
| EventKind::Remove(_) => { | ||
| // Convention: plugin directory name must match the plugin's `name` field in | ||
| // plugin.toml. On removal, the toml may already be gone so we rely on dir name. | ||
| for path in &event.paths { | ||
| if (is_plugin_toml(path) || is_wasm_file(path)) | ||
| && let Some(plugin_dir) = path.parent() | ||
| && let Some(name) = plugin_dir.file_name().and_then(|n| n.to_str()) | ||
| && loader.registry().contains(name) | ||
| { | ||
| tracing::info!("plugin file removed, unloading '{name}'"); | ||
| if let Err(e) = loader.unload(name) { | ||
| tracing::warn!("failed to unload plugin '{name}': {e}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unloading by parent-directory name is brittle.
This path derives the registry key from path.parent().file_name(), but nothing in the parser enforces dir_name == manifest.info().name(). A plugin can load successfully from a differently named directory and then never unload on remove, leaving a stale registry entry behind. Persist a path→plugin-name mapping or reject mismatched directory names during install/parse.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/adapters/driven/plugin/watcher.rs` around lines 84 - 98,
Unloading by deriving the plugin key from path.parent().file_name() is brittle;
instead persist a mapping from plugin file path to its registered plugin name
when the plugin is loaded (e.g., update the loader/registry at load time), and
use that exact mapping to look up the plugin name during EventKind::Remove
rather than inferring from the directory; additionally, during install/parse
(where the manifest is read) add a check to either enforce directory-name ==
manifest.info().name() or reject the install and surface a clear error so
mismatched directories cannot produce a registry entry that won't be found by
name.
Comment on lines
+57
to
+61
| plugin_install, | ||
| plugin_uninstall, | ||
| plugin_enable, | ||
| plugin_disable, | ||
| plugin_list, |
There was a problem hiding this comment.
Wire AppState before exposing these IPC commands.
These handlers all require State<'_, AppState>, but run() still never registers an AppState in setup(). As added here, the entire plugin IPC surface will fail on first invoke instead of exercising the new infrastructure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/lib.rs` around lines 57 - 61, Handlers plugin_install,
plugin_uninstall, plugin_enable, plugin_disable, and plugin_list require
State<'_, AppState> but run() never registers AppState in setup(); register the
AppState before exposing these IPC commands by adding app.manage(AppState::new()
or AppState::default()) (or equivalent AppState construction) inside the setup()
closure (or on the Tauri builder) so the State<'_, AppState> is available to
those handlers when invoked.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driven/plugin/registry.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/registry.rs:78">
P2: The DashMap guard is held for the entire `plugin.call` because `entry` stays in scope, so registry operations on the same shard are blocked during WASM execution. Consider storing the plugin behind an `Arc<Mutex<_>>` (or similar) and cloning it so the guard can be dropped before the call.</violation>
</file>
<file name="src-tauri/src/adapters/driving/tauri_ipc.rs">
<violation number="1" location="src-tauri/src/adapters/driving/tauri_ipc.rs:156">
P1: Canonicalize `allowed_parent` before comparing with `starts_with` to prevent inevitable failures on Windows and symlinked filesystems.</violation>
</file>
<file name="src-tauri/src/adapters/driven/plugin/watcher.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/watcher.rs:51">
P2: Create/Modify events ignore `.wasm` updates, so hot-reload won’t trigger when the plugin binary changes. Include `.wasm` paths in the reload condition.</violation>
</file>
<file name="src-tauri/src/adapters/driven/plugin/extism_loader.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/extism_loader.rs:39">
P1: Validate the plugin name before using it to build a path. As written, a `plugin.toml` name containing `..` or path separators lets load() escape `plugins_dir` and read a different manifest/wasm than the one that was canonicalized.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Canonicalize allowed_parent in plugin_install IPC, fail closed when config_dir() is None (greptile P1, coderabbit, cubic) - Validate plugin name against path traversal (no ../ or separators) - Use Arc<Mutex<Plugin>> in registry, clone before WASM call to release DashMap shard guard during execution (cubic P2, coderabbit) - Require exactly one .wasm per plugin dir (deterministic loading) - Handle .wasm file changes in hot-reload watcher (not just plugin.toml) - Add PluginLoader::set_enabled port method, enable/disable commands now actually mutate plugin state via registry - Wrap plugin_loader().load() in spawn_blocking for async safety - Sort plugins by name in resolve_url for deterministic resolution - Mark registry.insert() as #[cfg(test)], try_insert is production API - Add PluginError display regression test - Remove redundant parse_manifest call in loader (use find_wasm_file)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src-tauri/src/adapters/driven/plugin/manifest.rs (1)
23-23: Consider renaming_licensetolicensewith#[serde(default)]or#[allow(dead_code)].The
_licensefield uses underscore prefix to suppress unused warnings, but this is unconventional for serde structs. If the field is intentionally parsed but unused, consider using#[allow(dead_code)]with a proper name, or add a comment explaining the intent.♻️ Suggested alternatives
#[derive(Deserialize)] struct RawPluginSection { name: String, version: String, category: String, author: String, description: String, - _license: Option<String>, + #[allow(dead_code)] + license: Option<String>, min_vortex_version: Option<String>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/manifest.rs` at line 23, The struct field named `_license` is prefixed with an underscore to silence unused warnings; rename it to `license` and annotate it with either #[serde(default)] if you want it deserialized with a default when missing, or add #[allow(dead_code)] above the `license` field if you intentionally parse but never use it; update any references to `_license` in the code (e.g., in the manifest struct and any uses) to the new `license` identifier and add a short comment clarifying why it’s present if it remains unused.src-tauri/src/application/commands/toggle_plugin.rs (1)
165-171: Strengthen tests by asserting theenabledflag value.Because
_enabledis ignored in the mock, tests can’t catch accidental inversion between enable/disable paths.✅ Suggested test-hardening diff
struct MockPluginLoader { plugins: Vec<PluginInfo>, + calls: std::sync::Mutex<Vec<(String, bool)>>, } impl MockPluginLoader { fn empty() -> Self { - Self { plugins: vec![] } + Self { + plugins: vec![], + calls: std::sync::Mutex::new(vec![]), + } } fn with_plugin(name: &str) -> Self { let info = PluginInfo::new( name.to_string(), @@ ); Self { plugins: vec![info], + calls: std::sync::Mutex::new(vec![]), } } } impl PluginLoader for MockPluginLoader { @@ - fn set_enabled(&self, name: &str, _enabled: bool) -> Result<(), DomainError> { + fn set_enabled(&self, name: &str, enabled: bool) -> Result<(), DomainError> { + self.calls + .lock() + .unwrap() + .push((name.to_string(), enabled)); if self.plugins.iter().any(|p| p.name() == name) { Ok(()) } else { Err(DomainError::NotFound(name.to_string())) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/application/commands/toggle_plugin.rs` around lines 165 - 171, The mock set_enabled currently ignores the _enabled parameter so tests can't detect inverted enable/disable logic; change set_enabled(&self, name: &str, _enabled: bool) to use the enabled argument (rename to enabled) and locate the matching plugin in self.plugins, then update or record that plugin's enabled state (or call its setter, e.g., Plugin::set_enabled or mutate the plugin struct) and return Ok(()); if no match, keep returning Err(DomainError::NotFound(name.to_string())). This ensures tests can assert the enabled flag value and catch inversion bugs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/watcher.rs`:
- Around line 61-67: The contains() check before unloading introduces a race;
instead, remove the conditional and call loader.unload(&name) unconditionally
(or perform a single atomic API if available) and handle the NotFound error case
explicitly; update the block referencing loader.registry().contains(&name) and
loader.unload(&name) so you always call unload and only log a warning for errors
other than NotFound to avoid spurious warnings during concurrent removals.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Line 23: The struct field named `_license` is prefixed with an underscore to
silence unused warnings; rename it to `license` and annotate it with either
#[serde(default)] if you want it deserialized with a default when missing, or
add #[allow(dead_code)] above the `license` field if you intentionally parse but
never use it; update any references to `_license` in the code (e.g., in the
manifest struct and any uses) to the new `license` identifier and add a short
comment clarifying why it’s present if it remains unused.
In `@src-tauri/src/application/commands/toggle_plugin.rs`:
- Around line 165-171: The mock set_enabled currently ignores the _enabled
parameter so tests can't detect inverted enable/disable logic; change
set_enabled(&self, name: &str, _enabled: bool) to use the enabled argument
(rename to enabled) and locate the matching plugin in self.plugins, then update
or record that plugin's enabled state (or call its setter, e.g.,
Plugin::set_enabled or mutate the plugin struct) and return Ok(()); if no match,
keep returning Err(DomainError::NotFound(name.to_string())). This ensures tests
can assert the enabled flag value and catch inversion bugs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40af634d-5155-4891-8b9c-24643f516cdc
📒 Files selected for processing (21)
src-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driven/plugin/registry.rssrc-tauri/src/adapters/driven/plugin/watcher.rssrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/command_bus.rssrc-tauri/src/application/commands/cancel_download.rssrc-tauri/src/application/commands/install_plugin.rssrc-tauri/src/application/commands/pause_all.rssrc-tauri/src/application/commands/pause_download.rssrc-tauri/src/application/commands/remove_download.rssrc-tauri/src/application/commands/resume_all.rssrc-tauri/src/application/commands/resume_download.rssrc-tauri/src/application/commands/retry_download.rssrc-tauri/src/application/commands/set_priority.rssrc-tauri/src/application/commands/start_download.rssrc-tauri/src/application/commands/toggle_plugin.rssrc-tauri/src/application/commands/uninstall_plugin.rssrc-tauri/src/domain/error.rssrc-tauri/src/domain/ports/driven/plugin_loader.rssrc-tauri/src/domain/ports/driven/tests.rs
✅ Files skipped from review due to trivial changes (3)
- src-tauri/src/application/commands/pause_download.rs
- src-tauri/src/application/commands/cancel_download.rs
- src-tauri/src/application/commands/retry_download.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/src/domain/error.rs
- src-tauri/src/application/commands/uninstall_plugin.rs
- src-tauri/src/adapters/driving/tauri_ipc.rs
- src-tauri/src/adapters/driven/plugin/extism_loader.rs
- Enforce dir name == plugin name in manifest parser (coderabbit) - Remove TOCTOU race: unload directly without contains() check in watcher - Fix collapsible_if clippy warning in manifest validation - Update test_parse_manifest_valid for dir name convention
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/plugin/manifest.rs (1)
23-23: Consider fixing or removing the_licensefield.The underscore-prefixed
_licensefield won't match alicensekey in TOML without#[serde(rename = "license")]. Since serde allows unknown fields by default, alicensefield inplugin.tomlwill simply be ignored, and this field serves no purpose.Either add the rename attribute if you want to parse (and ignore) the license field, or remove this dead field entirely.
♻️ Option 1: Add rename attribute to accept `license` key
- _license: Option<String>, + #[serde(rename = "license")] + _license: Option<String>,♻️ Option 2: Remove dead field
- _license: Option<String>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/manifest.rs` at line 23, The field `_license: Option<String>` in the manifest struct is a dead field because its underscore name prevents serde from mapping the `license` TOML key; either remove the `_license` field entirely from the struct (if you don't need to capture the license) or add a serde rename attribute to it (e.g., #[serde(rename = "license")] on the `_license` field) so that serde will parse the `license` key into that Option<String>; update the struct definition containing `_license` accordingly and run tests to ensure deserialization behaves as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Around line 219-238: The test test_parse_manifest_unknown_category is passing
for the wrong reason because it writes plugin files into tmp.path() (a
random-named dir) so parse_manifest fails on directory-name mismatch before
checking category; fix by creating a subdirectory named "weird" under the
TempDir, write_plugin_toml and write_dummy_wasm into that "weird" directory, and
call parse_manifest with that subdirectory path so parse_manifest will reach the
category parsing logic and return the expected DomainError::PluginError for an
unknown category.
- Around line 240-258: The test test_parse_manifest_missing_wasm fails for the
wrong reason because it writes the plugin TOML to tmp.path() while the plugin
name is "no-wasm", causing the directory-name mismatch to error before the
missing .wasm check; update the test to create a subdirectory matching the
plugin name (e.g., tmp.path().join("no-wasm")), write_plugin_toml into that
subdir, call parse_manifest on that subdir so parse_manifest (the function under
test) proceeds past the name check and fails due to the absent .wasm file as
intended.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Line 23: The field `_license: Option<String>` in the manifest struct is a dead
field because its underscore name prevents serde from mapping the `license` TOML
key; either remove the `_license` field entirely from the struct (if you don't
need to capture the license) or add a serde rename attribute to it (e.g.,
#[serde(rename = "license")] on the `_license` field) so that serde will parse
the `license` key into that Option<String>; update the struct definition
containing `_license` accordingly and run tests to ensure deserialization
behaves as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5756ef2-46bb-4332-a5dc-c85e8aade42e
📒 Files selected for processing (2)
src-tauri/src/adapters/driven/plugin/manifest.rssrc-tauri/src/adapters/driven/plugin/watcher.rs
Tests for unknown_category and missing_wasm were passing for the wrong reason — dir_name != plugin_name check fired before the intended error. Fixed by creating properly named subdirectories in each test. Added assertions on error message content to lock correct failure reason. Added test_parse_manifest_dir_name_mismatch regression test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/plugin/manifest.rs (1)
248-343: Add a regression test for multiple.wasmfiles.You enforce “exactly one
.wasm”, but there isn’t a test locking the multi-artifact failure path yet. Adding it would protect the deterministic behavior introduced in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/manifest.rs` around lines 248 - 343, Add a new unit test (e.g., fn test_parse_manifest_multiple_wasm()) that creates a TempDir plugin directory, writes a valid plugin TOML via write_plugin_toml and then creates two .wasm files with write_dummy_wasm (e.g., "a.wasm" and "b.wasm"), calls parse_manifest(&plugin_dir), asserts it returns Err, and checks the error message contains the expected multi-wasm failure text (the same substring your parse_manifest uses when more than one .wasm is found); reference parse_manifest and write_dummy_wasm in the test so the failure path for multiple .wasm files is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Around line 127-132: The loop in find_wasm_file currently collects any dir
entry with a ".wasm" extension, including non-regular entries; update the logic
to ensure only regular files are accepted by checking the entry/file type before
pushing to wasm_files (e.g., use entry.file_type() or path.is_file() and require
is_file() true), and propagate errors from file_type() as
DomainError::PluginError like other entry errors so invalid non-file artifacts
are excluded early.
---
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/manifest.rs`:
- Around line 248-343: Add a new unit test (e.g., fn
test_parse_manifest_multiple_wasm()) that creates a TempDir plugin directory,
writes a valid plugin TOML via write_plugin_toml and then creates two .wasm
files with write_dummy_wasm (e.g., "a.wasm" and "b.wasm"), calls
parse_manifest(&plugin_dir), asserts it returns Err, and checks the error
message contains the expected multi-wasm failure text (the same substring your
parse_manifest uses when more than one .wasm is found); reference parse_manifest
and write_dummy_wasm in the test so the failure path for multiple .wasm files is
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8365cce1-6c35-42db-be90-5b811edff677
📒 Files selected for processing (1)
src-tauri/src/adapters/driven/plugin/manifest.rs
Add is_file() check in find_wasm_file to reject directories or symlinks with .wasm extension, failing at parse time instead of at loader execution.
This was referenced Apr 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Adapter Layer (new:
adapters/driven/plugin/)plugin.tomlfiles, validates category/capabilities, locates.wasmfilePluginRegistrybacked by DashMap, implementsPluginReadRepositoryportExtismPluginLoaderimplementingPluginLoaderport (load/unload/resolve_url/list)notifycrate + tokio, handles Create/Modify/Remove eventsApplication Layer (new handlers)
PluginLoadedeventPluginUnloadedeventVec<PluginViewDto>viaPluginReadRepositorySecurity hardening (from adversarial review)
plugin_installIPC canonicalizes path and checks prefix against~/.config/vortex/plugins/std::fs::readentry()API to prevent TOCTOU race on duplicate loadDomain extensions
ContainerandNotifiervariants toPluginCategoryPluginError(String)variant toDomainErrorTest plan
cargo clippy --workspace -- -D warningscleancargo fmt --checkcleanplugin.toml+.wasmin~/.config/vortex/plugins/and verify hot-reloadSummary by cubic
Adds a WASM plugin system powered by
extism, with a manifest parser, in-memory registry, and hot-reload. Installs, uninstalls, enables, disables, and lists plugins via CQRS and Tauri IPC, meeting the plugin infrastructure requirements (Linear 13).New Features
plugin.tomlparser validates category/capabilities/version and locates exactly one.wasm.ExtismPluginLoader+PluginRegistry(DashMap+Arc<Mutex<extism::Plugin>>) for atomic load/unload/list with a 100 MB cap.notify+tokio.plugin_install,plugin_uninstall,plugin_enable,plugin_disable,plugin_list; domain addsContainer/NotifierandPluginError.Bug Fixes
dirsconfig dir is missing, validate plugin names (no separators/traversal).PluginLoader::set_enabled.loadruns inspawn_blocking; resolve order is deterministic (sort by name).Arc<Mutex<Plugin>>to releaseDashMapguards; unload directly to avoid TOCTOU; watcher reacts to.wasmchanges.plugin.nameand accept only a single regular.wasmfile (reject directories/symlinks).Written for commit ae6188e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores
Documentation