feat(plugins): add wasm plugin foundation and hook scaffolding (part 1/2) (RMN-270)#1363
Merged
theonlyhennygod merged 11 commits intozeroclaw-labs:mainfrom Mar 1, 2026
Conversation
|
Thanks for contributing to ZeroClaw. For faster review, please ensure:
See |
github-actions
bot
added
docs
github-actions
bot
removed
gateway
Contributor
Contributor
Contributor
Collaborator
|
Maintainer scan (2026-02-27):
Unblock path:
|
Contributor
Author
Code Review: WASM Plugin Foundation (Part 1/2)Reviewed commit range: CRITICAL (Must Fix)1. Compilation error — missing
|
Contributor
Author
Review Issues ResolvedAll issues from the code review have been addressed in 5 commits ( Critical (Fixed)
Important (Fixed)
Suggestions (Fixed)
Validation
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/mod.rs (1)
451-734:⚠️ Potential issue | 🟠 MajorAdd boundary/failure-mode tests for the new plugin registration path.
The new
config.plugins.enabledbranch is untested. Please add at least: (1) enabled path includes registry tools, (2) disabled path excludes them, and (3) duplicate-name handling behavior.As per coding guidelines:
src/{security,runtime,gateway,tools}/**/*.rs: "For security/runtime/gateway/tools changes, include at least one boundary/failure-mode validation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mod.rs` around lines 451 - 734, Add three unit tests inside the existing #[cfg(test)] mod tests in src/tools/mod.rs that exercise the new config.plugins.enabled branch: (1) a test that sets Config.plugins.enabled = true and a sample plugin registry entry (with a unique tool name) then calls all_tools(...) and asserts the registry tool name is present in the returned tool names; (2) a test that sets Config.plugins.enabled = false with the same registry entry and asserts the registry tool name is NOT present; and (3) a duplicate-name test that registers two plugin entries with the same tool name while plugins.enabled = true, calls all_tools(...), and asserts there is only one tool with that name (no duplicate entries) to validate duplicate-name handling; reuse the existing test helpers/patterns (TempDir, MemoryConfig, BrowserConfig, all_tools, default Config creation) and check names via tools.iter().map(|t| t.name()) as in the other tests.
🧹 Nitpick comments (8)
src/tools/shell.rs (1)
20-29: Add direct tests for UTF-8 truncation boundary behavior.The helper is now the truncation authority for both stdout/stderr paths, but there isn’t a focused unit test for multibyte boundary handling and no-op behavior.
🧪 Proposed test additions
#[cfg(test)] mod tests { use super::*; + #[test] + fn truncate_utf8_to_max_bytes_preserves_char_boundary() { + let mut s = "🙂🙂🙂".to_string(); + truncate_utf8_to_max_bytes(&mut s, 5); + assert_eq!(s, "🙂"); + } + + #[test] + fn truncate_utf8_to_max_bytes_noop_within_limit() { + let mut s = "abc".to_string(); + truncate_utf8_to_max_bytes(&mut s, 3); + assert_eq!(s, "abc"); + }Based on learnings: For security/runtime/gateway/tools changes, include threat/risk notes and rollback strategy; add/update tests or validation evidence for failure modes and boundaries; keep observability useful but non-sensitive.
Also applies to: 211-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/shell.rs` around lines 20 - 29, The truncate_utf8_to_max_bytes helper lacks focused unit tests for multibyte-character boundary behavior and no-op behavior when input is within max_bytes; add unit tests that (1) pass a string with multi-byte UTF-8 characters and assert truncation does not split a codepoint (test exact boundary at a byte index that falls inside a codepoint), (2) pass a string shorter than max_bytes and assert it remains unchanged, and (3) test edge cases: max_bytes = 0 and max_bytes equal to a character boundary; reference the truncate_utf8_to_max_bytes function in tests and also add brief threat/risk notes and a rollback strategy to the PR description or changelog noting potential log truncation impacts and how to revert the change if truncation causes issues in observability.docs/plans/2026-02-22-wasm-plugin-runtime.md (1)
413-415: Consider rewording the repeated “Ensure …” checklist lines.Small readability nit: varying sentence starts will make the final checklist scan faster.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-02-22-wasm-plugin-runtime.md` around lines 413 - 415, The three checklist lines all start with the word “Ensure,” which is repetitive; reword them to vary sentence starts for better scanability by replacing the first line with an active phrasing focused on hook determinism (refer to the checklist item about hook ordering and cancellation semantics), rewrite the second line to emphasize fallback behavior for providers/tools (the provider/tool fallback behavior item), and rephrase the third to highlight non-fatal, reversible hot-reload behavior (the hot-reload failures item), keeping meaning identical but using varied sentence starters (e.g., verbs or nouns) for each line.src/providers/mod.rs (1)
1317-1330: Add a focused test for the new plugin-provider fail-fast branch.This branch introduces new factory behavior and user-facing error text, so it should be covered explicitly to prevent silent regressions during Part 2 integration.
As per coding guidelines:
src/providers/**/*.rsshould “add focused tests for factory wiring and error paths.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/mod.rs` around lines 1317 - 1330, Add a focused unit test that exercises the branch guarded by plugins::runtime::current_registry() / registry.has_provider(name): simulate or stub the registry to return true for a plugin provider name and invoke the provider factory entry point that resolves providers (the function that eventually hits the match arm checking registry.has_provider(name)); assert it returns an error and that the error message contains "Plugin providers are not yet supported (requires Part 2). Provider" and does not fall through to the "Unknown provider" message—this ensures the fail-fast plugin-provider path is covered and prevents regressions.src/plugins/bridge/observer.rs (1)
64-70: Test name says forwarding, but forwarding is not asserted.
bridge_forwards_eventscurrently passes even ifrecord_eventdelegation breaks.💡 Suggested test hardening
#[test] fn bridge_forwards_events() { - let inner: Arc<dyn Observer> = Arc::new(DummyObserver::default()); - let bridge = ObserverBridge::new(Arc::clone(&inner)); + let inner = Arc::new(DummyObserver::default()); + let bridge = ObserverBridge::new(inner.clone()); bridge.record_event(&ObserverEvent::HeartbeatTick); + assert_eq!(*inner.events.lock(), 1); assert_eq!(bridge.name(), "observer-bridge"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/bridge/observer.rs` around lines 64 - 70, The test bridge_forwards_events only checks the bridge name but never verifies that ObserverBridge::record_event delegated to the inner observer; update the test to assert delegation by making DummyObserver expose its received events (e.g., a shared Arc<Mutex<Vec<ObserverEvent>>> or a last_event field) and after creating let inner: Arc<dyn Observer> = Arc::new(DummyObserver::default()) and let bridge = ObserverBridge::new(Arc::clone(&inner)), call bridge.record_event(&ObserverEvent::HeartbeatTick) and then lock/read the DummyObserver's recorded events to assert it contains ObserverEvent::HeartbeatTick (or the expected count/state), ensuring the test fails if record_event stops delegating to the inner observer.src/plugins/runtime.rs (1)
109-141: Add boundary tests for initialization failure/retry and concurrent calls.Current tests validate happy-path loading, but they don’t lock in behavior for concurrent
initialize_from_configcallers or retry-after-failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/runtime.rs` around lines 109 - 141, The test suite only covers the successful path for PluginRuntime::load_registry_from_config and misses boundary tests for initialization failures, retry-after-failure, and concurrent initialize_from_config callers; add new tests that (1) simulate initialization failing (e.g., by pointing PluginsConfig.dirs at a bad/missing manifest) and assert that initialize_from_config returns Err, then fix the source (or call path) to allow a subsequent successful retry and assert that a follow-up initialize_from_config succeeds and produces the expected registry, and (2) spawn concurrent callers that invoke PluginRuntime::initialize_from_config (or load_registry_from_config) simultaneously and assert only one initialization runs (or that all callers get a consistent result) to lock-in concurrency behavior; reference PluginRuntime, initialize_from_config, load_registry_from_config, PluginsConfig, and the registry methods (len, tools, has_provider) when adding these tests so they exercise failure/retry and concurrent-call handling.src/hooks/traits.rs (1)
31-87: Add direct default-contract tests for the new hook methods.
capabilities,before_compaction,after_compaction, andtool_result_persistare now part of the public hook contract; asserting their default pass-through behavior in tests will prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/traits.rs` around lines 31 - 87, Add unit tests that directly assert the default contract for the new hook methods: write tests calling capabilities() on a default hook impl and assert it returns an empty slice (&[]); call before_compaction(Vec<ChatMessage>) and assert it returns HookResult::Continue with the same Vec; call after_compaction(String) and assert HookResult::Continue returns the same String; call tool_result_persist(ToolResult) and assert HookResult::Continue returns the identical ToolResult. Use the concrete hook implementation (the type that implements these defaults) and the HookResult::Continue variant for comparisons so these defaults are covered and protected from regressions.src/hooks/runner.rs (2)
618-736: Good capability-gating test coverage; consider adding the cancel-with-capability case.The tests thoroughly verify that unauthorized modifications and cancellations are blocked. For completeness, consider adding a test that verifies
Cancelis honored when the hook does haveModifyToolResultscapability.🧪 Optional test to add
/// Hook with capability that cancels. struct CappedResultCanceller; #[async_trait] impl HookHandler for CappedResultCanceller { fn name(&self) -> &str { "capped_canceller" } fn capabilities(&self) -> &[PluginCapability] { &[PluginCapability::ModifyToolResults] } async fn tool_result_persist( &self, _tool: String, _result: ToolResult, ) -> HookResult<ToolResult> { HookResult::Cancel("authorized_block".into()) } } #[tokio::test] async fn tool_result_persist_allows_cancel_with_capability() { let mut runner = HookRunner::new(); runner.register(Box::new(CappedResultCanceller)); let result = runner .run_tool_result_persist("shell".into(), sample_tool_result()) .await; match result { HookResult::Cancel(reason) => { assert_eq!(reason, "authorized_block"); } HookResult::Continue(_) => panic!("cancel with capability should be honored"), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/runner.rs` around lines 618 - 736, Add a new test and hook that verifies Cancel is honored when the hook has the ModifyToolResults capability: implement a CappedResultCanceller struct implementing HookHandler (name() -> "capped_canceller", capabilities() -> &[PluginCapability::ModifyToolResults]) whose tool_result_persist returns HookResult::Cancel("authorized_block".into()), then add an async test tool_result_persist_allows_cancel_with_capability that registers Box::new(CappedResultCanceller) on a new HookRunner, calls run_tool_result_persist("shell".into(), sample_tool_result()).await and matches the result to assert it is HookResult::Cancel(reason) with reason == "authorized_block" (and panic if it returns Continue).
340-355: AddPartialEqderive toToolResultfor safer capability-gated modification detection.The modification detection at lines 340–343 explicitly compares
success,output, anderrorfields. This creates maintenance coupling: ifToolResultgains additional fields in the future, the comparison will silently allow those modifications without capability gating.Derive
PartialEqonToolResultand replace the field-by-field check withnext_result != result. This ensures all fields—current and future—are automatically included in the comparison.♻️ Suggested refactor
In
src/tools/traits.rs, addPartialEqto the derives:-#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ToolResult { pub success: bool, pub output: String, pub error: Option<String>, }In
src/hooks/runner.rsat lines 340–343, replace the check:- if next_result.success != result.success - || next_result.output != result.output - || next_result.error != result.error - { + if next_result != result { if has_modify_cap { result = next_result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/runner.rs` around lines 340 - 355, ToolResult equality is currently checked by comparing success/output/error fields manually; derive PartialEq for ToolResult (where it's defined) so equality covers all fields, then in the runner replace the field-by-field comparison with a single inequality check (use next_result != result) where has_modify_cap, next_result, and result are used to decide whether to accept the modification or log the warning; ensure you update the derive list for the ToolResult struct (e.g., add PartialEq) and swap the conditional in the hook runner to use the new != comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-02-22-wasm-plugin-runtime.md`:
- Around line 31-35: The test plugins_config_defaults_safe currently asserts
cfg.enabled which contradicts the deny-by-default goal; update the test to
assert the default is disabled (e.g., assert that cfg.enabled is false) or, if
switching to the planned PluginConfig type, create PluginConfig::default() and
assert its enabled flag is false; ensure you reference the
HooksConfig/PluginConfig default constructor used in the test
(plugins_config_defaults_safe) and change the assertion accordingly.
In `@src/agent/loop_.rs`:
- Around line 874-880: In the HookResult::Cancel arm, don’t embed the raw reason
into outputs/history—create a sanitized string (e.g. let safe_reason =
scrub_credentials(&reason)) and use safe_reason for outcome.output,
outcome.error_reason and any fields written to history like
tool_result_obj.output and tool_result_obj.error; replace direct uses of reason
in those assignments with the sanitized value so sensitive content is never
written into tool output/history.
In `@src/agent/loop_/history.rs`:
- Around line 115-126: After hooks.run_after_compaction(...) may return an
oversized summary, re-apply the COMPACTION_MAX_SUMMARY_CHARS cap to the summary
before calling apply_compaction_summary so hooks cannot bypass the size limit;
specifically, after handling crate::hooks::HookResult::Continue(next_summary)
(and before apply_compaction_summary(history, start, compact_end, &summary)),
truncate or ellipsize next_summary to COMPACTION_MAX_SUMMARY_CHARS (preserving
Unicode boundaries) and use that trimmed value as summary; update any variable
flow around run_after_compaction and apply_compaction_summary to use the trimmed
summary.
In `@src/channels/mod.rs`:
- Around line 3052-3054: The current code logs-and-continues when
crate::plugins::runtime::initialize_from_config(&config.plugins) returns Err,
which yields a fail-open behavior when config.plugins.enabled is true; change
the logic to check config.plugins.enabled and, if true and
initialize_from_config returns Err, propagate a hard failure (return Err(...) or
call bail! with a clear message and the underlying error) instead of
tracing::warn so startup fails fast; locate the call to
crate::plugins::runtime::initialize_from_config in mod.rs and replace the warn
branch with an error return that includes the original error context.
In `@src/config/schema.rs`:
- Around line 2087-2090: New boolean hook fields session_memory and boot_script
(and the similar fields around lines 2097-2099) were added as required fields
which breaks deserialization for existing configs; update the schema to make
these keys optional with safe defaults by annotating the struct fields (e.g.,
session_memory, boot_script, and the other new hook bools) to use serde defaults
(or change them to Option<bool> with a documented default) so existing
[hooks.builtin] entries without these keys still parse; ensure the struct name
where these fields live (e.g., the BuiltinHooks or HooksBuiltin struct) is
updated and update the public docs/comments to state the default values and
migration note.
- Around line 2103-2115: Add fail-fast validation inside Config::validate():
check PluginLimitsConfig.invoke_timeout_ms, memory_limit_bytes, and
max_concurrency and bail! with clear errors if any are zero (or otherwise
out-of-range), and validate the capability list (e.g., the config field that
holds plugin capability entries) by rejecting empty/blank strings (trim entries
and bail! if any are empty) and optionally reject entirely empty capability
lists; reference PluginLimitsConfig::invoke_timeout_ms, ::memory_limit_bytes,
::max_concurrency and the capability list field in your Config struct when
adding these checks so invalid runtime-critical values are rejected at
config-load time.
- Around line 2141-2149: The PluginsConfig struct requires the hot_reload field
to be present which breaks partial [plugins] sections; add a serde default for
that field (e.g., add #[serde(default)] to the hot_reload field in
PluginsConfig) so missing hot_reload deserializes to false, ensuring minimal
configs like "enabled = true" succeed; keep existing #[serde(default)] on dirs
and do not add new config keys.
In `@src/gateway/mod.rs`:
- Around line 332-334: The plugin initialization currently swallows errors
(tracing::warn!) which can silently disable plugin behavior; replace the
warn-and-continue with a hard failure: when
crate::plugins::runtime::initialize_from_config(&config.plugins) returns
Err(error), propagate a startup error (use bail! or return Err(anyhow::Error)
with a clear message including the error) so the caller of this function fails
fast instead of continuing with plugins disabled; update the surrounding
function signature to return Result if needed and add/adjust tests or startup
validation to assert plugin init failure surfaces, documenting the rollback
strategy (do not auto-disable config.plugins.enabled).
In `@src/plugins/bridge/observer.rs`:
- Around line 33-35: Both BroadcastObserver::as_any and ObserverBridge::as_any
currently return self, breaking the Observer trait's downcast contract; change
both implementations to delegate to the wrapped observer by returning
self.inner.as_any() (i.e., update the as_any method in BroadcastObserver and
ObserverBridge to call inner.as_any()). Also update the bridge_forwards_events
test to assert that
wrapped_observer.as_any().downcast_ref::<PrometheusObserver>() (or the concrete
inner type used) succeeds to ensure delegation works.
In `@src/plugins/manifest.rs`:
- Around line 76-88: The validation currently checks manifest.tools and
manifest.providers for empty names but doesn't require corresponding capability
declarations; update the validation to also require appropriate entries in
manifest.capabilities whenever manifest.tools or manifest.providers are
non-empty. Specifically, in the same validation area that iterates
manifest.tools and manifest.providers, add checks that manifest.capabilities
contains the expected capability token(s) (e.g., a "tools" or "providers"
capability or whatever canonical capability names your manifest uses) and call
anyhow::bail! with a clear message like "declared tools require corresponding
'tools' capability" or "declared providers require corresponding 'providers'
capability" if the capability is missing. Ensure you reference and validate
manifest.capabilities alongside the existing checks for manifest.tools and
manifest.providers so a manifest cannot declare tools/providers without opt-in
capabilities.
- Around line 58-64: validate_manifest currently allows empty or unsafe
module_path values, deferring failures to runtime; update validate_manifest to
fail fast by validating PluginManifest.module_path: check it's non-empty
(trimmed) and reject unsafe values (e.g., absolute paths starting with '/' or
drive letters, or any path segments containing ".." or path traversal
characters). Add the check alongside existing id/version checks in
validate_manifest so invalid module_path causes an immediate anyhow::bail! with
a clear message referencing module_path.
In `@src/plugins/registry.rs`:
- Around line 13-19: The register method is not idempotent because
re-registering a PluginManifest leaves stale entries in self.tools and
self.providers; modify register (the function named register) to insert the new
manifest into self.manifests first (or replace the existing), then rebuild the
derived indexes by clearing self.tools and self.providers and iterating over all
manifests in self.manifests to repopulate tools (extend/collect from
manifest.tools) and providers (insert provider.trim().to_string()), ensuring
tools/providers always reflect the current manifests.
In `@src/plugins/runtime.rs`:
- Around line 34-37: The code currently uses entries.flatten() which swallows
per-entry I/O errors; change the loop to iterate over the ReadDir results and
propagate/contextualize errors instead of flattening them: replace for entry in
entries.flatten() with for entry_res in entries { let entry =
entry_res.with_context(|| format!("failed to read entry in plugin directory {}",
path.display()))?; let path = entry.path(); } so any failure reading a directory
entry or plugin manifest surfaces (use the existing with_context/bail error
patterns around the entries variable and entry_res to fail fast and include path
details).
- Around line 76-87: The current initialize_from_config allows a race where two
threads pass the initial load check; replace the simple load check with an
atomic compare-and-swap: call INITIALIZED.compare_exchange(false, true,
std::sync::atomic::Ordering::Acquire, std::sync::atomic::Ordering::Relaxed) at
the start of initialize_from_config and if it fails return Ok(()) to skip
duplicate init; after successfully swapping, proceed to create PluginRuntime and
call PluginRuntime::load_registry_from_config and write into registry_cell(); if
load_registry_from_config or any subsequent step returns an Err, ensure you
reset INITIALIZED.store(false, std::sync::atomic::Ordering::Release) before
returning the error so failed inits don't leave INITIALIZED set.
In `@src/tools/mod.rs`:
- Around line 436-447: The plugin tool registration loop in src/tools/mod.rs
appends every plugin into tool_arcs without checking for duplicate names,
causing ambiguous dispatch; modify the loop that iterates registry.tools() to
check existing entries in tool_arcs for a matching name (compare the incoming
tool.name against the already-registered tool names held in the Arc entries) and
only create/push a new PluginManifestTool::new(ToolSpec { ... }) when no
existing name is found; on collision, skip (or log a warning) to avoid replacing
built-ins or other plugins and ensure uniqueness of names before pushing into
tool_arcs.
---
Outside diff comments:
In `@src/tools/mod.rs`:
- Around line 451-734: Add three unit tests inside the existing #[cfg(test)] mod
tests in src/tools/mod.rs that exercise the new config.plugins.enabled branch:
(1) a test that sets Config.plugins.enabled = true and a sample plugin registry
entry (with a unique tool name) then calls all_tools(...) and asserts the
registry tool name is present in the returned tool names; (2) a test that sets
Config.plugins.enabled = false with the same registry entry and asserts the
registry tool name is NOT present; and (3) a duplicate-name test that registers
two plugin entries with the same tool name while plugins.enabled = true, calls
all_tools(...), and asserts there is only one tool with that name (no duplicate
entries) to validate duplicate-name handling; reuse the existing test
helpers/patterns (TempDir, MemoryConfig, BrowserConfig, all_tools, default
Config creation) and check names via tools.iter().map(|t| t.name()) as in the
other tests.
---
Nitpick comments:
In `@docs/plans/2026-02-22-wasm-plugin-runtime.md`:
- Around line 413-415: The three checklist lines all start with the word
“Ensure,” which is repetitive; reword them to vary sentence starts for better
scanability by replacing the first line with an active phrasing focused on hook
determinism (refer to the checklist item about hook ordering and cancellation
semantics), rewrite the second line to emphasize fallback behavior for
providers/tools (the provider/tool fallback behavior item), and rephrase the
third to highlight non-fatal, reversible hot-reload behavior (the hot-reload
failures item), keeping meaning identical but using varied sentence starters
(e.g., verbs or nouns) for each line.
In `@src/hooks/runner.rs`:
- Around line 618-736: Add a new test and hook that verifies Cancel is honored
when the hook has the ModifyToolResults capability: implement a
CappedResultCanceller struct implementing HookHandler (name() ->
"capped_canceller", capabilities() -> &[PluginCapability::ModifyToolResults])
whose tool_result_persist returns HookResult::Cancel("authorized_block".into()),
then add an async test tool_result_persist_allows_cancel_with_capability that
registers Box::new(CappedResultCanceller) on a new HookRunner, calls
run_tool_result_persist("shell".into(), sample_tool_result()).await and matches
the result to assert it is HookResult::Cancel(reason) with reason ==
"authorized_block" (and panic if it returns Continue).
- Around line 340-355: ToolResult equality is currently checked by comparing
success/output/error fields manually; derive PartialEq for ToolResult (where
it's defined) so equality covers all fields, then in the runner replace the
field-by-field comparison with a single inequality check (use next_result !=
result) where has_modify_cap, next_result, and result are used to decide whether
to accept the modification or log the warning; ensure you update the derive list
for the ToolResult struct (e.g., add PartialEq) and swap the conditional in the
hook runner to use the new != comparison.
In `@src/hooks/traits.rs`:
- Around line 31-87: Add unit tests that directly assert the default contract
for the new hook methods: write tests calling capabilities() on a default hook
impl and assert it returns an empty slice (&[]); call
before_compaction(Vec<ChatMessage>) and assert it returns HookResult::Continue
with the same Vec; call after_compaction(String) and assert HookResult::Continue
returns the same String; call tool_result_persist(ToolResult) and assert
HookResult::Continue returns the identical ToolResult. Use the concrete hook
implementation (the type that implements these defaults) and the
HookResult::Continue variant for comparisons so these defaults are covered and
protected from regressions.
In `@src/plugins/bridge/observer.rs`:
- Around line 64-70: The test bridge_forwards_events only checks the bridge name
but never verifies that ObserverBridge::record_event delegated to the inner
observer; update the test to assert delegation by making DummyObserver expose
its received events (e.g., a shared Arc<Mutex<Vec<ObserverEvent>>> or a
last_event field) and after creating let inner: Arc<dyn Observer> =
Arc::new(DummyObserver::default()) and let bridge =
ObserverBridge::new(Arc::clone(&inner)), call
bridge.record_event(&ObserverEvent::HeartbeatTick) and then lock/read the
DummyObserver's recorded events to assert it contains
ObserverEvent::HeartbeatTick (or the expected count/state), ensuring the test
fails if record_event stops delegating to the inner observer.
In `@src/plugins/runtime.rs`:
- Around line 109-141: The test suite only covers the successful path for
PluginRuntime::load_registry_from_config and misses boundary tests for
initialization failures, retry-after-failure, and concurrent
initialize_from_config callers; add new tests that (1) simulate initialization
failing (e.g., by pointing PluginsConfig.dirs at a bad/missing manifest) and
assert that initialize_from_config returns Err, then fix the source (or call
path) to allow a subsequent successful retry and assert that a follow-up
initialize_from_config succeeds and produces the expected registry, and (2)
spawn concurrent callers that invoke PluginRuntime::initialize_from_config (or
load_registry_from_config) simultaneously and assert only one initialization
runs (or that all callers get a consistent result) to lock-in concurrency
behavior; reference PluginRuntime, initialize_from_config,
load_registry_from_config, PluginsConfig, and the registry methods (len, tools,
has_provider) when adding these tests so they exercise failure/retry and
concurrent-call handling.
In `@src/providers/mod.rs`:
- Around line 1317-1330: Add a focused unit test that exercises the branch
guarded by plugins::runtime::current_registry() / registry.has_provider(name):
simulate or stub the registry to return true for a plugin provider name and
invoke the provider factory entry point that resolves providers (the function
that eventually hits the match arm checking registry.has_provider(name)); assert
it returns an error and that the error message contains "Plugin providers are
not yet supported (requires Part 2). Provider" and does not fall through to the
"Unknown provider" message—this ensures the fail-fast plugin-provider path is
covered and prevents regressions.
In `@src/tools/shell.rs`:
- Around line 20-29: The truncate_utf8_to_max_bytes helper lacks focused unit
tests for multibyte-character boundary behavior and no-op behavior when input is
within max_bytes; add unit tests that (1) pass a string with multi-byte UTF-8
characters and assert truncation does not split a codepoint (test exact boundary
at a byte index that falls inside a codepoint), (2) pass a string shorter than
max_bytes and assert it remains unchanged, and (3) test edge cases: max_bytes =
0 and max_bytes equal to a character boundary; reference the
truncate_utf8_to_max_bytes function in tests and also add brief threat/risk
notes and a rollback strategy to the PR description or changelog noting
potential log truncation impacts and how to revert the change if truncation
causes issues in observability.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
docs/plans/2026-02-22-wasm-plugin-runtime-design.mddocs/plans/2026-02-22-wasm-plugin-runtime.mdsrc/agent/loop_.rssrc/agent/loop_/history.rssrc/channels/mod.rssrc/config/mod.rssrc/config/schema.rssrc/gateway/mod.rssrc/hooks/builtin/boot_script.rssrc/hooks/builtin/mod.rssrc/hooks/builtin/session_memory.rssrc/hooks/runner.rssrc/hooks/traits.rssrc/lib.rssrc/main.rssrc/memory/hygiene.rssrc/onboard/wizard.rssrc/plugins/bridge/mod.rssrc/plugins/bridge/observer.rssrc/plugins/hot_reload.rssrc/plugins/manifest.rssrc/plugins/mod.rssrc/plugins/registry.rssrc/plugins/runtime.rssrc/plugins/traits.rssrc/providers/mod.rssrc/tools/mod.rssrc/tools/screenshot.rssrc/tools/shell.rswit/zeroclaw/hooks/v1/hooks.witwit/zeroclaw/providers/v1/providers.witwit/zeroclaw/tools/v1/tools.wit
Comment on lines
+31
to
+35
| fn plugins_config_defaults_safe() { | ||
| let cfg = HooksConfig::default(); | ||
| // replace with PluginConfig once added | ||
| assert!(cfg.enabled); | ||
| } |
Contributor
There was a problem hiding this comment.
Default-safety example currently asserts the opposite behavior.
The sample test says “safe defaults” but asserts enabled == true, which conflicts with the deny-by-default objective described below.
🛠️ Proposed fix
#[test]
fn plugins_config_defaults_safe() {
- let cfg = HooksConfig::default();
- // replace with PluginConfig once added
- assert!(cfg.enabled);
+ let cfg = PluginsConfig::default();
+ assert!(!cfg.enabled);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn plugins_config_defaults_safe() { | |
| let cfg = HooksConfig::default(); | |
| // replace with PluginConfig once added | |
| assert!(cfg.enabled); | |
| } | |
| fn plugins_config_defaults_safe() { | |
| let cfg = PluginsConfig::default(); | |
| assert!(!cfg.enabled); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-02-22-wasm-plugin-runtime.md` around lines 31 - 35, The test
plugins_config_defaults_safe currently asserts cfg.enabled which contradicts the
deny-by-default goal; update the test to assert the default is disabled (e.g.,
assert that cfg.enabled is false) or, if switching to the planned PluginConfig
type, create PluginConfig::default() and assert its enabled flag is false;
ensure you reference the HooksConfig/PluginConfig default constructor used in
the test (plugins_config_defaults_safe) and change the assertion accordingly.
Comment on lines
+874
to
+880
| crate::hooks::HookResult::Cancel(reason) => { | ||
| outcome.success = false; | ||
| outcome.error_reason = Some(scrub_credentials(&reason)); | ||
| outcome.output = format!("Tool result blocked by hook: {reason}"); | ||
| tool_result_obj.success = false; | ||
| tool_result_obj.error = Some(reason); | ||
| tool_result_obj.output = outcome.output.clone(); |
Contributor
There was a problem hiding this comment.
Sanitize hook cancel reasons before writing them into tool output/history.
reason is currently embedded directly into outcome.output, which can leak sensitive content or inject untrusted text into subsequent model context.
🛡️ Suggested fix
crate::hooks::HookResult::Cancel(reason) => {
+ let safe_reason = scrub_credentials(&reason);
outcome.success = false;
- outcome.error_reason = Some(scrub_credentials(&reason));
- outcome.output = format!("Tool result blocked by hook: {reason}");
+ outcome.error_reason = Some(safe_reason.clone());
+ outcome.output = format!("Tool result blocked by hook: {safe_reason}");
tool_result_obj.success = false;
- tool_result_obj.error = Some(reason);
+ tool_result_obj.error = Some(safe_reason);
tool_result_obj.output = outcome.output.clone();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| crate::hooks::HookResult::Cancel(reason) => { | |
| outcome.success = false; | |
| outcome.error_reason = Some(scrub_credentials(&reason)); | |
| outcome.output = format!("Tool result blocked by hook: {reason}"); | |
| tool_result_obj.success = false; | |
| tool_result_obj.error = Some(reason); | |
| tool_result_obj.output = outcome.output.clone(); | |
| crate::hooks::HookResult::Cancel(reason) => { | |
| let safe_reason = scrub_credentials(&reason); | |
| outcome.success = false; | |
| outcome.error_reason = Some(safe_reason.clone()); | |
| outcome.output = format!("Tool result blocked by hook: {safe_reason}"); | |
| tool_result_obj.success = false; | |
| tool_result_obj.error = Some(safe_reason); | |
| tool_result_obj.output = outcome.output.clone(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/loop_.rs` around lines 874 - 880, In the HookResult::Cancel arm,
don’t embed the raw reason into outputs/history—create a sanitized string (e.g.
let safe_reason = scrub_credentials(&reason)) and use safe_reason for
outcome.output, outcome.error_reason and any fields written to history like
tool_result_obj.output and tool_result_obj.error; replace direct uses of reason
in those assignments with the sanitized value so sensitive content is never
written into tool output/history.
Comment on lines
+115
to
126
| let summary = if let Some(hooks) = hooks { | ||
| match hooks.run_after_compaction(summary).await { | ||
| crate::hooks::HookResult::Continue(next_summary) => next_summary, | ||
| crate::hooks::HookResult::Cancel(reason) => { | ||
| tracing::info!(%reason, "post-compaction summary cancelled by hook"); | ||
| return Ok(false); | ||
| } | ||
| } | ||
| } else { | ||
| summary | ||
| }; | ||
| apply_compaction_summary(history, start, compact_end, &summary); |
Contributor
There was a problem hiding this comment.
Re-apply summary length cap after run_after_compaction.
A hook can return an oversized summary and bypass COMPACTION_MAX_SUMMARY_CHARS, which can grow history entries beyond the intended bound.
💡 Suggested fix
let summary = truncate_with_ellipsis(&summary_raw, COMPACTION_MAX_SUMMARY_CHARS);
let summary = if let Some(hooks) = hooks {
match hooks.run_after_compaction(summary).await {
crate::hooks::HookResult::Continue(next_summary) => next_summary,
crate::hooks::HookResult::Cancel(reason) => {
tracing::info!(%reason, "post-compaction summary cancelled by hook");
return Ok(false);
}
}
} else {
summary
};
+ let summary = truncate_with_ellipsis(&summary, COMPACTION_MAX_SUMMARY_CHARS);
apply_compaction_summary(history, start, compact_end, &summary);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/loop_/history.rs` around lines 115 - 126, After
hooks.run_after_compaction(...) may return an oversized summary, re-apply the
COMPACTION_MAX_SUMMARY_CHARS cap to the summary before calling
apply_compaction_summary so hooks cannot bypass the size limit; specifically,
after handling crate::hooks::HookResult::Continue(next_summary) (and before
apply_compaction_summary(history, start, compact_end, &summary)), truncate or
ellipsize next_summary to COMPACTION_MAX_SUMMARY_CHARS (preserving Unicode
boundaries) and use that trimmed value as summary; update any variable flow
around run_after_compaction and apply_compaction_summary to use the trimmed
summary.
Comment on lines
+3052
to
+3054
| if let Err(error) = crate::plugins::runtime::initialize_from_config(&config.plugins) { | ||
| tracing::warn!("plugin registry initialization skipped: {error}"); | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast when plugins are enabled and registry init fails.
At Line 3052, startup always logs-and-continues on plugin init errors. When config.plugins.enabled is true, this creates a fail-open path where plugin-dependent behavior silently disappears.
Suggested fix
- if let Err(error) = crate::plugins::runtime::initialize_from_config(&config.plugins) {
- tracing::warn!("plugin registry initialization skipped: {error}");
- }
+ if let Err(error) = crate::plugins::runtime::initialize_from_config(&config.plugins) {
+ if config.plugins.enabled {
+ return Err(error)
+ .context("plugin registry initialization failed while plugins are enabled");
+ }
+ tracing::debug!("plugin registry initialization skipped (plugins disabled): {error}");
+ }As per coding guidelines: **/*.rs: Keep error paths obvious and localized; fail fast with explicit bail!/errors for unsupported or unsafe states.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Err(error) = crate::plugins::runtime::initialize_from_config(&config.plugins) { | |
| tracing::warn!("plugin registry initialization skipped: {error}"); | |
| } | |
| if let Err(error) = crate::plugins::runtime::initialize_from_config(&config.plugins) { | |
| if config.plugins.enabled { | |
| return Err(error) | |
| .context("plugin registry initialization failed while plugins are enabled"); | |
| } | |
| tracing::debug!("plugin registry initialization skipped (plugins disabled): {error}"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/mod.rs` around lines 3052 - 3054, The current code
logs-and-continues when
crate::plugins::runtime::initialize_from_config(&config.plugins) returns Err,
which yields a fail-open behavior when config.plugins.enabled is true; change
the logic to check config.plugins.enabled and, if true and
initialize_from_config returns Err, propagate a hard failure (return Err(...) or
call bail! with a clear message and the underlying error) instead of
tracing::warn so startup fails fast; locate the call to
crate::plugins::runtime::initialize_from_config in mod.rs and replace the warn
branch with an error return that includes the original error context.
Comment on lines
+76
to
+88
| for tool in &manifest.tools { | ||
| if tool.name.trim().is_empty() { | ||
| anyhow::bail!("plugin tool name cannot be empty"); | ||
| } | ||
| if tool.description.trim().is_empty() { | ||
| anyhow::bail!("plugin tool description cannot be empty"); | ||
| } | ||
| } | ||
| for provider in &manifest.providers { | ||
| if provider.trim().is_empty() { | ||
| anyhow::bail!("plugin provider name cannot be empty"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce capability declarations for declared tools/providers.
A manifest with empty capabilities but non-empty tools/providers currently passes validation. That broadens plugin surface area without explicit opt-in capability checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/manifest.rs` around lines 76 - 88, The validation currently
checks manifest.tools and manifest.providers for empty names but doesn't require
corresponding capability declarations; update the validation to also require
appropriate entries in manifest.capabilities whenever manifest.tools or
manifest.providers are non-empty. Specifically, in the same validation area that
iterates manifest.tools and manifest.providers, add checks that
manifest.capabilities contains the expected capability token(s) (e.g., a "tools"
or "providers" capability or whatever canonical capability names your manifest
uses) and call anyhow::bail! with a clear message like "declared tools require
corresponding 'tools' capability" or "declared providers require corresponding
'providers' capability" if the capability is missing. Ensure you reference and
validate manifest.capabilities alongside the existing checks for manifest.tools
and manifest.providers so a manifest cannot declare tools/providers without
opt-in capabilities.
Comment on lines
+34
to
+37
| let entries = std::fs::read_dir(path) | ||
| .with_context(|| format!("failed to read plugin directory {}", path.display()))?; | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); |
Contributor
There was a problem hiding this comment.
entries.flatten() hides manifest discovery/read failures.
Per-entry filesystem errors are silently dropped, which can produce partial registry state without any startup signal.
🔧 Suggested fix
- for entry in entries.flatten() {
- let path = entry.path();
+ for entry in entries {
+ let entry = entry.with_context(|| {
+ format!("failed to read plugin directory entry in {}", path.display())
+ })?;
+ let path = entry.path();
if !path.is_file() {
continue;
}As per coding guidelines **/*.rs: “Keep error paths obvious and localized; fail fast with explicit bail!/errors for unsupported or unsafe states.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/runtime.rs` around lines 34 - 37, The code currently uses
entries.flatten() which swallows per-entry I/O errors; change the loop to
iterate over the ReadDir results and propagate/contextualize errors instead of
flattening them: replace for entry in entries.flatten() with for entry_res in
entries { let entry = entry_res.with_context(|| format!("failed to read entry in
plugin directory {}", path.display()))?; let path = entry.path(); } so any
failure reading a directory entry or plugin manifest surfaces (use the existing
with_context/bail error patterns around the entries variable and entry_res to
fail fast and include path details).
Comment on lines
+76
to
+87
| pub fn initialize_from_config(config: &PluginsConfig) -> Result<()> { | ||
| if INITIALIZED.load(std::sync::atomic::Ordering::Acquire) { | ||
| tracing::debug!("plugin registry already initialized, skipping re-init"); | ||
| return Ok(()); | ||
| } | ||
| let runtime = PluginRuntime::new(); | ||
| let registry = runtime.load_registry_from_config(config)?; | ||
| let mut guard = registry_cell() | ||
| .write() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| *guard = registry; | ||
| INITIALIZED.store(true, std::sync::atomic::Ordering::Release); |
Contributor
There was a problem hiding this comment.
Initialization guard is not atomic across concurrent callers.
Two callers can pass the initial check and both write to the global registry, causing nondeterministic overwrite of startup state.
🔒 Suggested fix
pub fn initialize_from_config(config: &PluginsConfig) -> Result<()> {
- if INITIALIZED.load(std::sync::atomic::Ordering::Acquire) {
+ use std::sync::atomic::Ordering;
+ if INITIALIZED
+ .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire)
+ .is_err()
+ {
tracing::debug!("plugin registry already initialized, skipping re-init");
return Ok(());
}
let runtime = PluginRuntime::new();
- let registry = runtime.load_registry_from_config(config)?;
+ let registry = match runtime.load_registry_from_config(config) {
+ Ok(registry) => registry,
+ Err(error) => {
+ INITIALIZED.store(false, Ordering::Release);
+ return Err(error);
+ }
+ };
let mut guard = registry_cell()
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
*guard = registry;
- INITIALIZED.store(true, std::sync::atomic::Ordering::Release);
Ok(())
}As per coding guidelines **/*.rs: “Keep error paths obvious and localized; fail fast with explicit bail!/errors for unsupported or unsafe states.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn initialize_from_config(config: &PluginsConfig) -> Result<()> { | |
| if INITIALIZED.load(std::sync::atomic::Ordering::Acquire) { | |
| tracing::debug!("plugin registry already initialized, skipping re-init"); | |
| return Ok(()); | |
| } | |
| let runtime = PluginRuntime::new(); | |
| let registry = runtime.load_registry_from_config(config)?; | |
| let mut guard = registry_cell() | |
| .write() | |
| .unwrap_or_else(std::sync::PoisonError::into_inner); | |
| *guard = registry; | |
| INITIALIZED.store(true, std::sync::atomic::Ordering::Release); | |
| pub fn initialize_from_config(config: &PluginsConfig) -> Result<()> { | |
| use std::sync::atomic::Ordering; | |
| if INITIALIZED | |
| .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) | |
| .is_err() | |
| { | |
| tracing::debug!("plugin registry already initialized, skipping re-init"); | |
| return Ok(()); | |
| } | |
| let runtime = PluginRuntime::new(); | |
| let registry = match runtime.load_registry_from_config(config) { | |
| Ok(registry) => registry, | |
| Err(error) => { | |
| INITIALIZED.store(false, Ordering::Release); | |
| return Err(error); | |
| } | |
| }; | |
| let mut guard = registry_cell() | |
| .write() | |
| .unwrap_or_else(std::sync::PoisonError::into_inner); | |
| *guard = registry; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/runtime.rs` around lines 76 - 87, The current
initialize_from_config allows a race where two threads pass the initial load
check; replace the simple load check with an atomic compare-and-swap: call
INITIALIZED.compare_exchange(false, true, std::sync::atomic::Ordering::Acquire,
std::sync::atomic::Ordering::Relaxed) at the start of initialize_from_config and
if it fails return Ok(()) to skip duplicate init; after successfully swapping,
proceed to create PluginRuntime and call
PluginRuntime::load_registry_from_config and write into registry_cell(); if
load_registry_from_config or any subsequent step returns an Err, ensure you
reset INITIALIZED.store(false, std::sync::atomic::Ordering::Release) before
returning the error so failed inits don't leave INITIALIZED set.
Comment on lines
+436
to
+447
| // Add declared plugin tools from the active plugin registry. | ||
| if config.plugins.enabled { | ||
| let registry = plugins::runtime::current_registry(); | ||
| for tool in registry.tools() { | ||
| tool_arcs.push(Arc::new(PluginManifestTool::new(ToolSpec { | ||
| name: tool.name.clone(), | ||
| description: tool.description.clone(), | ||
| parameters: tool.parameters.clone(), | ||
| }))); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Guard plugin tool registration against name collisions.
Line 439 currently appends every declared plugin tool without checking whether that name already exists in tool_arcs. That can create ambiguous dispatch (built-in vs plugin, or plugin vs plugin).
🔧 Proposed fix (dedupe before push)
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
@@
if config.plugins.enabled {
let registry = plugins::runtime::current_registry();
+ let mut existing_names: HashSet<String> =
+ tool_arcs.iter().map(|tool| tool.name().to_string()).collect();
for tool in registry.tools() {
+ let tool_name = tool.name.trim();
+ if tool_name.is_empty() || !existing_names.insert(tool_name.to_string()) {
+ tracing::warn!("plugins: skipping duplicate tool registration for '{tool_name}'");
+ continue;
+ }
tool_arcs.push(Arc::new(PluginManifestTool::new(ToolSpec {
- name: tool.name.clone(),
+ name: tool_name.to_string(),
description: tool.description.clone(),
parameters: tool.parameters.clone(),
})));
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/mod.rs` around lines 436 - 447, The plugin tool registration loop
in src/tools/mod.rs appends every plugin into tool_arcs without checking for
duplicate names, causing ambiguous dispatch; modify the loop that iterates
registry.tools() to check existing entries in tool_arcs for a matching name
(compare the incoming tool.name against the already-registered tool names held
in the Arc entries) and only create/push a new PluginManifestTool::new(ToolSpec
{ ... }) when no existing name is found; on collision, skip (or log a warning)
to avoid replacing built-ins or other plugins and ensure uniqueness of names
before pushing into tool_arcs.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/agent/loop_.rs (1)
874-881:⚠️ Potential issue | 🟠 MajorIncomplete sanitization: raw
reasonstill embedded inoutcome.outputandtool_result_obj.error.Line 876 correctly sanitizes the reason for
outcome.error_reason, but lines 877 and 879 embed the rawreasondirectly intooutcome.outputandtool_result_obj.error. This inconsistency could leak sensitive content into tool output or subsequent model context.🛡️ Suggested fix for consistent sanitization
crate::hooks::HookResult::Cancel(reason) => { + let safe_reason = scrub_credentials(&reason); outcome.success = false; - outcome.error_reason = Some(scrub_credentials(&reason)); - outcome.output = format!("Tool result blocked by hook: {reason}"); + outcome.error_reason = Some(safe_reason.clone()); + outcome.output = format!("Tool result blocked by hook: {safe_reason}"); tool_result_obj.success = false; - tool_result_obj.error = Some(reason); + tool_result_obj.error = Some(safe_reason); tool_result_obj.output = outcome.output.clone(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/loop_.rs` around lines 874 - 881, The Cancel branch of crate::hooks::HookResult is embedding the raw reason into outcome.output and tool_result_obj.error; instead create a sanitized_reason = scrub_credentials(&reason) (you already use it for outcome.error_reason) and use sanitized_reason when setting outcome.output and tool_result_obj.error, and set tool_result_obj.output = outcome.output.clone() as before—update the block handling HookResult::Cancel to consistently use scrub_credentials(&reason) for all exposed fields (outcome.error_reason, outcome.output, tool_result_obj.error, tool_result_obj.output).
🧹 Nitpick comments (3)
src/agent/loop_.rs (1)
1081-1083: Plugin initialization duplicated across multiple entry points.
initialize_from_configis called inrun()(line 1081),process_message()(line 1573), andAgent::from_config()(agent.rs). While the fingerprint-based idempotency prevents redundant work, this scattered initialization could benefit from consolidation into a single startup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/loop_.rs` around lines 1081 - 1083, The plugin initialization (crate::plugins::runtime::initialize_from_config) is duplicated across run(), process_message(), and Agent::from_config(); consolidate it so initialization happens exactly once at startup: either keep the current call in run() and remove the calls from process_message() and Agent::from_config(), or extract a single init_plugins(config.plugins) helper that calls initialize_from_config(&config.plugins) and invoke that helper only from the main startup path (run()). Update references to the fingerprint/idempotency if needed and ensure config.plugins is passed into the chosen single entry point so no other entrypoints perform plugin initialization.src/plugins/registry.rs (1)
22-24: Consider addingis_empty()to satisfy clippy'slen_without_is_emptylint.Structs that expose a
len()method conventionally also provideis_empty()for idiomatic Rust usage and clippy compliance.♻️ Proposed addition
pub fn len(&self) -> usize { self.manifests.len() } + + pub fn is_empty(&self) -> bool { + self.manifests.is_empty() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/registry.rs` around lines 22 - 24, Add an is_empty(&self) -> bool method to the same impl that defines pub fn len(&self) -> usize so the type (Registry) satisfies clippy's len_without_is_empty; implement it by delegating to self.manifests.is_empty() (or return self.len() == 0) and place it next to the existing pub fn len(&self) -> usize in the impl block for the struct that contains manifests.src/plugins/runtime.rs (1)
82-112: Fingerprint check has a TOCTOU window allowing redundant initialization.Two concurrent callers can both pass the fingerprint comparison (lines 85-93), then both proceed to load the registry and overwrite the global state. While benign (both produce equivalent registries), this causes redundant I/O and computation.
For startup-only use this is acceptable. If hot-reload becomes common, consider using a mutex or
compare_exchangepattern to serialize initialization attempts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/runtime.rs` around lines 82 - 112, The current initialize_from_config has a TOCTOU race: two threads can read init_fingerprint_cell (via init_fingerprint_cell()) and both proceed to create registries concurrently; fix by serializing the check+init: acquire the write lock on init_fingerprint_cell (or otherwise a dedicated Mutex) before comparing the fingerprint so only one thread performs initialization, and re-check the stored fingerprint after acquiring the write lock to avoid redundant work; update initialize_from_config to use the write-guard path from init_fingerprint_cell (or a compare_exchange-style guard) to perform the check, then call PluginRuntime::new() and runtime.load_registry_from_config(config) and store the registry via registry_cell() and the fingerprint only once the initializing thread completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/loop_.rs`:
- Around line 874-881: The Cancel branch of crate::hooks::HookResult is
embedding the raw reason into outcome.output and tool_result_obj.error; instead
create a sanitized_reason = scrub_credentials(&reason) (you already use it for
outcome.error_reason) and use sanitized_reason when setting outcome.output and
tool_result_obj.error, and set tool_result_obj.output = outcome.output.clone()
as before—update the block handling HookResult::Cancel to consistently use
scrub_credentials(&reason) for all exposed fields (outcome.error_reason,
outcome.output, tool_result_obj.error, tool_result_obj.output).
---
Nitpick comments:
In `@src/agent/loop_.rs`:
- Around line 1081-1083: The plugin initialization
(crate::plugins::runtime::initialize_from_config) is duplicated across run(),
process_message(), and Agent::from_config(); consolidate it so initialization
happens exactly once at startup: either keep the current call in run() and
remove the calls from process_message() and Agent::from_config(), or extract a
single init_plugins(config.plugins) helper that calls
initialize_from_config(&config.plugins) and invoke that helper only from the
main startup path (run()). Update references to the fingerprint/idempotency if
needed and ensure config.plugins is passed into the chosen single entry point so
no other entrypoints perform plugin initialization.
In `@src/plugins/registry.rs`:
- Around line 22-24: Add an is_empty(&self) -> bool method to the same impl that
defines pub fn len(&self) -> usize so the type (Registry) satisfies clippy's
len_without_is_empty; implement it by delegating to self.manifests.is_empty()
(or return self.len() == 0) and place it next to the existing pub fn len(&self)
-> usize in the impl block for the struct that contains manifests.
In `@src/plugins/runtime.rs`:
- Around line 82-112: The current initialize_from_config has a TOCTOU race: two
threads can read init_fingerprint_cell (via init_fingerprint_cell()) and both
proceed to create registries concurrently; fix by serializing the check+init:
acquire the write lock on init_fingerprint_cell (or otherwise a dedicated Mutex)
before comparing the fingerprint so only one thread performs initialization, and
re-check the stored fingerprint after acquiring the write lock to avoid
redundant work; update initialize_from_config to use the write-guard path from
init_fingerprint_cell (or a compare_exchange-style guard) to perform the check,
then call PluginRuntime::new() and runtime.load_registry_from_config(config) and
store the registry via registry_cell() and the fingerprint only once the
initializing thread completes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/plans/2026-02-22-wasm-plugin-runtime.md (1)
31-35:⚠️ Potential issue | 🟡 MinorDefault-safety example still contradicts deny-by-default behavior.
Line 34 currently asserts enabled-by-default, which conflicts with the stated safe default posture and can steer implementation/tests the wrong way.
Suggested doc fix
#[test] fn plugins_config_defaults_safe() { - let cfg = HooksConfig::default(); - // replace with PluginConfig once added - assert!(cfg.enabled); + let cfg = PluginsConfig::default(); + assert!(!cfg.enabled); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-02-22-wasm-plugin-runtime.md` around lines 31 - 35, The test/example in function plugins_config_defaults_safe() asserts that HooksConfig (to be replaced by PluginConfig) is enabled by default, which contradicts the deny-by-default safe posture; update the example to assert that the default config is disabled (e.g., assert!(!cfg.enabled)) and adjust the inline comment to state that default is deny-by-default so future implementers/tests follow the safe default behavior when using HooksConfig/PluginConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-02-22-wasm-plugin-runtime.md`:
- Around line 378-379: The planned change narrows i18n follow-through to only
fr/vi but policy requires updating all supported locales or explicitly deferring
them; update the line "Modify: locale docs where equivalents exist (`fr`, `vi`
minimum for config/commands/troubleshooting)" to either list and apply changes
for all supported locales (en, zh-CN, ja, ru, fr, vi, el) or replace it with a
clear deferral statement and add a PR-level TODO describing which locales are
deferred and why; make sure to reference the shared doc IDs you changed so
translators can find them and include the deferral note in the PR description if
you choose not to update locale files now.
---
Duplicate comments:
In `@docs/plans/2026-02-22-wasm-plugin-runtime.md`:
- Around line 31-35: The test/example in function plugins_config_defaults_safe()
asserts that HooksConfig (to be replaced by PluginConfig) is enabled by default,
which contradicts the deny-by-default safe posture; update the example to assert
that the default config is disabled (e.g., assert!(!cfg.enabled)) and adjust the
inline comment to state that default is deny-by-default so future
implementers/tests follow the safe default behavior when using
HooksConfig/PluginConfig.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/plans/2026-02-22-wasm-plugin-runtime-design.mddocs/plans/2026-02-22-wasm-plugin-runtime.md
Comment on lines
+378
to
+379
| - Modify: locale docs where equivalents exist (`fr`, `vi` minimum for | ||
| config/commands/troubleshooting) |
Contributor
There was a problem hiding this comment.
Locale follow-through scope is too narrow for shared docs updates.
Line 378-379 limits localization to fr/vi, but the repo policy requires follow-through for supported locales or explicit deferral notes in the PR when not done.
Suggested doc fix
-- Modify: locale docs where equivalents exist (`fr`, `vi` minimum for
- config/commands/troubleshooting)
+- Modify: locale docs where equivalents exist for supported locales
+ (`en`, `zh-CN`, `ja`, `ru`, `fr`, `vi`, `el`) for
+ config/commands/troubleshooting, or explicitly document deferral in PR notes.As per coding guidelines, "If a change touches docs IA, runtime-contract references, or user-facing wording in shared docs, perform i18n follow-through for currently supported locales (en, zh-CN, ja, ru, fr, vi, el) in the same PR".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-02-22-wasm-plugin-runtime.md` around lines 378 - 379, The
planned change narrows i18n follow-through to only fr/vi but policy requires
updating all supported locales or explicitly deferring them; update the line
"Modify: locale docs where equivalents exist (`fr`, `vi` minimum for
config/commands/troubleshooting)" to either list and apply changes for all
supported locales (en, zh-CN, ja, ru, fr, vi, el) or replace it with a clear
deferral statement and add a PR-level TODO describing which locales are deferred
and why; make sure to reference the shared doc IDs you changed so translators
can find them and include the deferral note in the PR description if you choose
not to update locale files now.
…init idempotent - Add HookRunner::from_config() factory that encapsulates hook construction from HooksConfig, replacing 3 duplicated blocks in agent/loop_, gateway, and channels modules. - Make plugin registry initialize_from_config() idempotent: skip re-init if already initialized, log debug message instead of silently overwriting. - Add capability gating for tool_result_persist hook modifications.
Add `capabilities()` method to HookHandler trait so the runner can check whether a hook has ModifyToolResults permission before allowing it to mutate tool results. Without this, any registered hook could flip success, rewrite output, or suppress errors with no gate.
This was referenced Mar 1, 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
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
Describe this PR in 2-5 bullets:
devfor normal contributions;mainonly fordevpromotion):devpluginsmodule, manifests/registry/runtime skeleton), hook lifecycle extension points, startup integration plumbing, and Rust 1.87 compatibility fixes needed for this layer.Label Snapshot (required)
risk: low|medium|high):risk: highsize: XS|S|M|L|XL, auto-managed/read-only):size: XLcore|agent|channel|config|cron|daemon|doctor|gateway|health|heartbeat|integration|memory|observability|onboard|provider|runtime|security|service|skillforge|skills|tool|tunnel|docs|dependencies|ci|tests|scripts|dev, comma-separated):core, provider, tool, docs, dependencies<module>: <component>, for examplechannel: telegram,provider: kimi,tool: shell):agent: loop, channel: core, gateway: core, onboard: wizard, memory: hygiene, config: coretrusted contributor|experienced contributor|principal contributor|distinguished contributor, auto-managed/read-only; author merged PRs >=5/10/20/50): auto-managedChange Metadata
bug|feature|refactor|docs|security|chore):featureruntime|provider|channel|memory|security|ci|docs|multi):multiLinked Issue
Supersede Attribution (required when
Supersedes #is used)#<pr> by @<author>, one per line):#1361 by @gh-xjCo-authored-bytrailers added for materially incorporated contributors? (Yes/No):NoNo, explain why (for example: inspiration-only, no direct code/design carry-over): same author; no third-party carry-over requiring attribution trailers.\n): (Pass/Fail):PassValidation Evidence (required)
Commands and result summary:
cargo fmt --all -- --check cargo clippy --all-targets -- -D warnings cargo testcargo fmt --all -- --check: passcargo clippy --all-targets -- -D warnings: not used as release gate for this PR because repository currently has unrelated pre-existing-D warningsdebt outside this diff; CI strict-delta remains authoritative.cargo test: pass locally (full suite)Security Impact (required)
Yes/No):YesYes/No):NoYes/No):NoYes/No):YesYes, describe risk and mitigation: plugin capability surface is opt-in via config, with manifest validation and explicit runtime wiring; no new default-open behavior.Privacy and Data Hygiene (required)
pass|needs-follow-up):passCompatibility / Migration
Yes/No):YesYes/No):YesYes/No):Noi18n Follow-Through (required when docs or user-facing wording changes)
Yes/No):NoYes, locale navigation parity updated inREADME*,docs/README*, anddocs/SUMMARY.mdfor supported locales (en,zh-CN,ja,ru,fr,vi)? (Yes/No)Yes, localized runtime-contract docs updated where equivalents exist (minimum forfr/vi:commands-reference,config-reference,troubleshooting)? (Yes/No/N.A.)Yes, Vietnamese canonical docs underdocs/i18n/vi/**synced and compatibility shims underdocs/*.vi.mdvalidated? (Yes/No/N.A.)No/N.A., link follow-up issue/PR and explain scope decision: N/AHuman Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
plugins,hooks,agent,gateway,channels,providers,tools,onboard,config.Agent Collaboration Notes (recommended)
gh, local cargo checks/tests, targeted patch/edit flow.AGENTS.md+CONTRIBUTING.md): yes.Rollback Plan (required)
plugins.enabled.Risks and Mitigations
List real risks in this PR (or write
None).Summary by CodeRabbit
New Features
Documentation