feat(agent-runtime): activate surreal graphs plugin memory runtime#68
Merged
Conversation
Deploying corvus-plugins with
|
| Latest commit: |
11ccc44
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3dd29c2d.corvus-plugins.pages.dev |
| Branch Preview URL: | https://feature-memory-surreal-graph.corvus-plugins.pages.dev |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a WASM-based SurrealDB-backed memory plugin and host executor, exposes FFI plugin entrypoints, integrates strict ontology-based memory validation (validate → correct → re-validate) into agent flows and channels, registers a Plugin Release Manager skill, and updates runtime dependencies and plugin manifests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Agent
participant Provider as Provider/LLM
participant MemoryPlugin as Memory Plugin (WASM)
participant SurrealDB
User->>Agent: user query
Agent->>Provider: generate draft response
Provider-->>Agent: draft response
Agent->>MemoryPlugin: validate_response(query, draft)
MemoryPlugin->>SurrealDB: execute ontology/rules
SurrealDB-->>MemoryPlugin: violations / result
MemoryPlugin-->>Agent: ValidationResult {valid, violations}
alt valid
Agent-->>User: return draft
else invalid
Agent->>Provider: system prompt -> fix violations
Provider-->>Agent: corrected response
Agent->>MemoryPlugin: validate_response(query, corrected)
MemoryPlugin->>SurrealDB: re-check rules
MemoryPlugin-->>Agent: ValidationResult
alt corrected valid
Agent-->>User: return corrected response
else
Agent-->>User: return ontology-failure message
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-02-22 to 2026-02-22 |
![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: 7
🧹 Nitpick comments (10)
.agents/skills/plugin-release/SKILL.md (2)
47-48: Replacebatwithcatfor portability.
batis a third-party pager that is not installed in most CI runners or fresh developer environments. Using it here creates an unnecessary dependency for what is purely a read-only verification step.✏️ Proposed fix
-bat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml +cat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/plugin-release/SKILL.md around lines 47 - 48, The command in the documentation uses the non-portable pager "bat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml"; update that command to use the standard, universally available "cat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml" so CI and fresh developer environments won’t depend on a third-party tool — edit the SKILL.md entry that contains the "bat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml" command and replace "bat" with "cat".
67-69: Document tag recovery steps alongside Common Failure Case#1.The skill describes a tag/version mismatch as the most likely failure (line 111), but there are no instructions for recovering from a pushed tag that must be retracted. A mismatched tag that already triggered
publish-plugins.ymlcan cause confusing partial artifacts.Consider adding a short "Recovery" subsection after the tag step, e.g.:
✏️ Proposed addition
git push origin plugin/<plugin-id>/vX.Y.Z + +> **Recovery (tag/version mismatch):** If the workflow fails due to a mismatch, +> delete the tag locally and remotely before retrying: +> ```bash +> git tag -d plugin/<plugin-id>/vX.Y.Z +> git push origin --delete plugin/<plugin-id>/vX.Y.Z +> ``` +> Fix the version in `Cargo.toml`, commit, then re-run steps 2–4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/plugin-release/SKILL.md around lines 67 - 69, Document recovery steps for retracting a pushed tag after the tag creation step by adding a short "Recovery" subsection (placed after the git tag/push lines and referenced from Common Failure Case `#1`). Explain the two-step recovery: delete the local tag with "git tag -d plugin/<plugin-id>/vX.Y.Z", delete the remote tag with "git push origin --delete plugin/<plugin-id>/vX.Y.Z", then fix the version in Cargo.toml, commit, and re-run the publish steps (the workflow publish-plugins.yml may need to be re-triggered). Reference the tag pattern plugin/<plugin-id>/vX.Y.Z and publish-plugins.yml so readers can locate the relevant workflow.clients/agent-runtime/src/security/pairing.rs (1)
145-147:.any()short-circuits — not a fully constant-time scanThe change from
tokens.contains(&hashed)toiter().any(constant_time_eq)makes the timing-safe intent explicit and is a net improvement. However,.any()short-circuits on the firsttrue, so execution time still varies depending on where the matching hash lands in theHashSet's non-deterministic iteration order.In practice this is a negligible risk (iteration order is non-predictable by an attacker, token sets are tiny, and both sides are fixed-length SHA-256 hex so standard equality would already be near-constant), but if you want to eliminate the theoretical concern entirely, a non-short-circuiting fold avoids the early exit:
♻️ Constant-time fold over all stored tokens
- tokens - .iter() - .any(|stored| constant_time_eq(stored, &hashed)) + tokens + .iter() + .fold(false, |found, stored| found | constant_time_eq(stored, &hashed))As a side note, switching from
HashSet::contains(O(1)) to a linear scan (O(n)) is a minor performance trade-off, although negligible for the handful of tokens expected here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/security/pairing.rs` around lines 145 - 147, Replace the short-circuiting .any(...) scan with a non-short-circuiting reduction so every stored token is compared to hashed; in pairing.rs change the tokens.iter().any(|stored| constant_time_eq(stored, &hashed)) usage to a fold (or explicit loop) that XORs/ORs (accumulates) the result of constant_time_eq for each stored token and returns the final boolean, ensuring constant_time_eq is called for every entry (refer to the tokens variable and constant_time_eq and hashed identifiers when making the change).clients/agent-runtime/src/channels/mod.rs (1)
127-211: Deduplicate strict validation helper to prevent drift.
The same logic now exists inagent.rs,agent/loop_.rs, and here. Consider extracting a shared helper (e.g., in anagentormemorymodule) so behavior and messaging stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/mod.rs` around lines 127 - 211, The enforce_strict_memory_validation function duplicates validation/correction flow found in agent.rs and agent/loop_.rs; extract that logic into a shared helper (e.g., memory::enforce_strict_validation or agent::validation::enforce_strict_validation) and have enforce_strict_memory_validation, the agent.rs and agent/loop_.rs callers delegate to it; move the calls to mem.validate_response and provider.chat_with_system plus the standardized log and return messages (including the exact warning strings and user-facing failure strings) into the single helper so behavior and messages remain identical across callers.clients/agent-runtime/src/plugins/mod.rs (1)
1935-1994: Cross‑checkruntime_apibetween lockfile and manifest.
You validate id/version, but a mismatched runtime_api could indicate tampering or stale metadata. Failing fast here avoids surprising runtime mismatches.♻️ Suggested check
if manifest.version != locked.version { bail!( "Plugin manifest version mismatch for '{}': lockfile='{}', manifest='{}'", plugin_id, locked.version, manifest.version ); } + + if manifest.runtime_api != locked.runtime_api { + bail!( + "Plugin manifest runtime_api mismatch for '{}': lockfile='{}', manifest='{}'", + plugin_id, + locked.runtime_api, + manifest.runtime_api + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/plugins/mod.rs` around lines 1935 - 1994, Add a runtime_api consistency check in resolve_memory_plugin_runtime: after deserializing PluginManifest into manifest and after validate_plugin_version(...) compare manifest.runtime_api with the locked.runtime_api (the value on the locked struct returned by resolve_memory_plugin) and bail with a clear error if they differ (similar to the existing id/version mismatch branches); include plugin_id, locked.runtime_api and manifest.runtime_api in the error message and keep returning Err via bail! so mismatched runtime_api fails fast before constructing ResolvedMemoryPlugin.clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs (1)
340-358: Cache schema initialization to avoid repeated SQL on every request.
ensure_schemaruns multipleDEFINEstatements per call; a one‑time guard avoids needless DB round‑trips and reduces latency.♻️ Possible approach
fn ensure_schema() -> std::result::Result<(), String> { - let statements = [ - "DEFINE TABLE memory_entries SCHEMALESS;", - "DEFINE TABLE memory_edges SCHEMALESS;", - "DEFINE TABLE ontology_terms SCHEMALESS;", - "DEFINE TABLE ontology_rules SCHEMALESS;", - "DEFINE INDEX idx_memory_entries_key ON TABLE memory_entries COLUMNS key UNIQUE;", - "DEFINE INDEX idx_memory_entries_category ON TABLE memory_entries COLUMNS category;", - "DEFINE INDEX idx_memory_entries_session ON TABLE memory_entries COLUMNS session_id;", - "DEFINE INDEX idx_memory_edges_relation ON TABLE memory_edges COLUMNS relation_type;", - "DEFINE INDEX idx_memory_edges_to ON TABLE memory_edges COLUMNS to_id;", - ]; - - for statement in statements { - let _ = sql_query(statement, None, Some(3_000)); - } - - Ok(()) + static SCHEMA_INIT: std::sync::OnceLock<()> = std::sync::OnceLock::new(); + SCHEMA_INIT.get_or_try_init(|| { + let statements = [ + "DEFINE TABLE memory_entries SCHEMALESS;", + "DEFINE TABLE memory_edges SCHEMALESS;", + "DEFINE TABLE ontology_terms SCHEMALESS;", + "DEFINE TABLE ontology_rules SCHEMALESS;", + "DEFINE INDEX idx_memory_entries_key ON TABLE memory_entries COLUMNS key UNIQUE;", + "DEFINE INDEX idx_memory_entries_category ON TABLE memory_entries COLUMNS category;", + "DEFINE INDEX idx_memory_entries_session ON TABLE memory_entries COLUMNS session_id;", + "DEFINE INDEX idx_memory_edges_relation ON TABLE memory_edges COLUMNS relation_type;", + "DEFINE INDEX idx_memory_edges_to ON TABLE memory_edges COLUMNS to_id;", + ]; + + for statement in statements { + let _ = sql_query(statement, None, Some(3_000)); + } + + Ok(()) + })?; + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 340 - 358, ensure_schema currently issues the DEFINE statements on every call; change it to run the block only once by adding a thread-safe one-time guard (e.g., std::sync::Once or once_cell::sync::OnceCell) around the loop that calls sql_query so subsequent calls return immediately; keep the existing statements array and sql_query calls inside the one-time closure and propagate or log any error from the initial run so failures are visible (reference ensure_schema and sql_query to locate where to add the Once/OnceCell guard).clients/agent-runtime/src/memory/traits.rs (1)
16-21: Avoid a misleadingDefaultforMemoryValidationResult.
#[derive(Default)]makesvalid = false, which can accidentally mark responses invalid ifMemoryValidationResult::default()is used. Consider a custom Default returningvalid: trueor remove Default to avoid misuse.♻️ Suggested change
-#[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct MemoryValidationResult { pub valid: bool, #[serde(default)] pub violations: Vec<String>, } + +impl Default for MemoryValidationResult { + fn default() -> Self { + Self { + valid: true, + violations: Vec::new(), + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/traits.rs` around lines 16 - 21, The derived Default for MemoryValidationResult is misleading because it yields valid = false; update the struct by removing Default from the derive list and provide an explicit impl Default for MemoryValidationResult that returns valid: true and violations: empty Vec (or alternatively drop Default entirely to avoid accidental use); locate the MemoryValidationResult definition and replace the derive(Default) with a custom impl Default that sets valid = true and violations = Vec::new() so callers using MemoryValidationResult::default() get a valid result.clients/agent-runtime/src/memory/plugin.rs (2)
925-967: Tests cover the essential helpers but lack coverage for the core WASM execution path.The category roundtrip, endpoint normalization, security validation, and status packing tests are good. Consider adding integration tests (even with a minimal stub WASM module) for
invoke_syncin a follow-up to validate the full alloc → call → dealloc lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 925 - 967, Tests currently exercise helpers but miss the WASM execution lifecycle; add an integration test that exercises invoke_sync to validate alloc → call → dealloc and result handling. Create a minimal stub WASM module (or embed a tiny WASM binary) that exports alloc, dealloc, and the target function, then in the test call invoke_sync with that module, verify the alloc returns a pointer, the call returns expected output, and dealloc frees memory without leaks or panics; target functions and helpers to touch are invoke_sync, alloc, dealloc, and any memory marshalling helpers used by invoke_sync so the full roundtrip is covered.
566-612:host_sql_v1— the synchronous blocking HTTP call is safe here but consider logging at warn level for transport errors.Since this runs inside
spawn_blocking, the blocking HTTP call is acceptable. However, the transport error on line 532-541 is silently returned as a JSON error to the plugin. Atracing::warn!here would help operators diagnose connectivity issues with SurrealDB.💡 Add warn-level logging for SQL transport errors
Err(error) => { + tracing::warn!("SurrealDB SQL transport error: {error}"); return json!({ "ok": false, "status": 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 566 - 612, In host_sql_v1, when the closure returns Err(error) you should emit a warn-level log with the error details to aid diagnosing SurrealDB transport/connectivity issues; add a tracing::warn! (or warn!(...) via use tracing::warn) in the Err(error) arm—e.g. tracing::warn!(error = %error, "host_sql_v1 transport error contacting SurrealDB")—before building the JSON error payload so operators see the transport error context; keep the existing JSON/error response behavior unchanged.clients/agent-runtime/src/memory/mod.rs (1)
113-147: Consider surfacing the fallback more visibly when the user explicitly configuredsurreal-graphs.The
tracing::warn!on line 126-128 is appropriate, but when a user explicitly setsbackend: "surreal-graphs", silently degrading to markdown could cause confusion (e.g., no graph ontology features, data not persisted to SurrealDB). Consider logging aterrorlevel or including a startup banner hint so the fallback is harder to miss.The overall approach — resolve plugin runtime, attempt init, fallback on failure — is sound and well-structured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/mod.rs` around lines 113 - 147, The current fallback to Markdown when MemoryBackendKind::SurrealGraphs fails is only traced at warn level; change the fallback logging in the SurrealGraphs match arm (the branches handling Ok(Some(plugin_runtime)) -> Err(error), Ok(None), and Err(error)) to a more prominent error or startup-visible message so users who explicitly configured surreal-graphs notice the degradation. Update the tracing messages around PluginBackedMemory::new and the branches referencing crate::plugins::OFFICIAL_SURREAL_GRAPHS_PLUGIN_ID and MarkdownMemory::new to use tracing::error! (or emit an additional startup banner/hint) that clearly states the backend fell back to markdown, warns that SurrealDB features/persistence will not be available, and suggests installing/trusting the plugin ID. Ensure the messages include the plugin id and the originating error details where available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/Cargo.toml`:
- Around line 83-84: The wasmi dependency in Cargo.toml is using default
features which bloats binaries; update the wasmi entry (identify the wasmi
dependency line) to disable default features and explicitly enable only needed
features (e.g., set default-features = false and features = ["std"]) and
remove/avoid the "wat" feature unless the codebase actually parses WAT text;
ensure the dependency line matches the project's pattern used for
prost/surrealdb.
In `@clients/agent-runtime/plugins/memory-surreal-graphs/Cargo.toml`:
- Around line 24-28: Update the Cargo.toml dependency entries for serde and
serde_json to disable default features and enable only what's needed for WASM
size; specifically set serde = { version = "1.0", default-features = false,
features = ["derive", "alloc"] } and serde_json = { version = "1.0",
default-features = false, features = ["alloc"] } so the json! macro and basic
(de)serialization remain available while removing preserve_order,
arbitrary_precision, float_roundtrip, unbounded_depth and raw_value.
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 159-197: In dispatch, before calling alloc_v1, check the encoded
response size (encoded.len()) against the plugin manifest's max_output_bytes (or
a defined MAX_OUTPUT_BYTES constant) and return a non-zero status if it exceeds
the limit; this prevents allocating oversized responses from execute() for
functions like list/recall—so compute encoded (or better: compute/serialize to a
size-limited buffer), compare to max_output_bytes, and only call
alloc_v1(resp_len_i32) when within the limit, returning an appropriate error
code and avoiding dealloc_v1 calls when allocation was never performed.
In `@clients/agent-runtime/src/agent/agent.rs`:
- Around line 355-444: Add unit tests exercising
Agent::enforce_strict_memory_validation covering: (1) initial
memory.validate_response returning invalid with violations and
provider.chat_with_system supplying a corrected string that then validates true
(assert the method returns the corrected text), (2) provider.chat_with_system
error path (assert the fallback error message contains the original
violations_text), (3) post-correction validation returning still-invalid (assert
returned message includes checked violations), and (4) memory.validate_response
errors on initial or post-correction calls (assert the function returns the
appropriate unavailable/ontology-failed messages). Use mocks/stubs for
memory.validate_response and provider.chat_with_system to simulate each scenario
and assert exact returned strings from enforce_strict_memory_validation to lock
behavior; reference the enforce_strict_memory_validation method,
Memory::validate_response, and Provider::chat_with_system when locating code to
test.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 461-470: SurrealSqlClient's auto-derived Debug exposes sensitive
fields (password, token); replace #[derive(Debug)] with a manual impl of
std::fmt::Debug for SurrealSqlClient that prints non-sensitive fields (sql_url,
namespace, database, username) but redacts or omits password and token (e.g.,
show "<redacted>" or "REDACTED"); update/remove the Debug derive on any upstream
types (HostState, PluginWasmExecutorInner) if they still derive Debug
transitively from SurrealSqlClient so that only non-sensitive data is printed
when those types are formatted.
- Around line 778-800: The validate_endpoint_security function currently accepts
"ws" and "wss" unconditionally, letting endpoints that will later normalize to
http/https bypass the HTTP loopback restriction; update
validate_endpoint_security (checking scheme, host, allow_http_loopback) to treat
"ws" like "http" and "wss" like "https" when allow_http_loopback is relevant: if
scheme == "ws" or "wss" and allow_http_loopback is false, enforce
is_loopback_host(host) (bail with the same messages used for "http" when
non-loopback), while preserving the existing behavior for secure schemes;
reference the scheme, host, allow_http_loopback, is_loopback_host, and
normalize_http_endpoint logic when making the change.
- Around line 342-350: The code currently compiles the WASM module on every
invoke_sync by calling Engine::new/Module::new with self.inner.wasm_bytes;
instead, compile once in PluginWasmExecutor::new and store the resulting Engine
and Module on PluginWasmExecutorInner (e.g., add engine: Engine and module:
Module fields), then in invoke_sync only create a fresh Store (mutable
per-invocation) and instantiate from the cached module; update
PluginWasmExecutor::new to build WasmiConfig/Engine and Module::new(&engine,
&self.inner.wasm_bytes) once and remove the per-invoke compilation in
invoke_sync.
---
Nitpick comments:
In @.agents/skills/plugin-release/SKILL.md:
- Around line 47-48: The command in the documentation uses the non-portable
pager "bat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml"; update that
command to use the standard, universally available "cat
clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml" so CI and fresh developer
environments won’t depend on a third-party tool — edit the SKILL.md entry that
contains the "bat clients/agent-runtime/plugins/<plugin-dir>/Cargo.toml" command
and replace "bat" with "cat".
- Around line 67-69: Document recovery steps for retracting a pushed tag after
the tag creation step by adding a short "Recovery" subsection (placed after the
git tag/push lines and referenced from Common Failure Case `#1`). Explain the
two-step recovery: delete the local tag with "git tag -d
plugin/<plugin-id>/vX.Y.Z", delete the remote tag with "git push origin --delete
plugin/<plugin-id>/vX.Y.Z", then fix the version in Cargo.toml, commit, and
re-run the publish steps (the workflow publish-plugins.yml may need to be
re-triggered). Reference the tag pattern plugin/<plugin-id>/vX.Y.Z and
publish-plugins.yml so readers can locate the relevant workflow.
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 340-358: ensure_schema currently issues the DEFINE statements on
every call; change it to run the block only once by adding a thread-safe
one-time guard (e.g., std::sync::Once or once_cell::sync::OnceCell) around the
loop that calls sql_query so subsequent calls return immediately; keep the
existing statements array and sql_query calls inside the one-time closure and
propagate or log any error from the initial run so failures are visible
(reference ensure_schema and sql_query to locate where to add the Once/OnceCell
guard).
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 127-211: The enforce_strict_memory_validation function duplicates
validation/correction flow found in agent.rs and agent/loop_.rs; extract that
logic into a shared helper (e.g., memory::enforce_strict_validation or
agent::validation::enforce_strict_validation) and have
enforce_strict_memory_validation, the agent.rs and agent/loop_.rs callers
delegate to it; move the calls to mem.validate_response and
provider.chat_with_system plus the standardized log and return messages
(including the exact warning strings and user-facing failure strings) into the
single helper so behavior and messages remain identical across callers.
In `@clients/agent-runtime/src/memory/mod.rs`:
- Around line 113-147: The current fallback to Markdown when
MemoryBackendKind::SurrealGraphs fails is only traced at warn level; change the
fallback logging in the SurrealGraphs match arm (the branches handling
Ok(Some(plugin_runtime)) -> Err(error), Ok(None), and Err(error)) to a more
prominent error or startup-visible message so users who explicitly configured
surreal-graphs notice the degradation. Update the tracing messages around
PluginBackedMemory::new and the branches referencing
crate::plugins::OFFICIAL_SURREAL_GRAPHS_PLUGIN_ID and MarkdownMemory::new to use
tracing::error! (or emit an additional startup banner/hint) that clearly states
the backend fell back to markdown, warns that SurrealDB features/persistence
will not be available, and suggests installing/trusting the plugin ID. Ensure
the messages include the plugin id and the originating error details where
available.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 925-967: Tests currently exercise helpers but miss the WASM
execution lifecycle; add an integration test that exercises invoke_sync to
validate alloc → call → dealloc and result handling. Create a minimal stub WASM
module (or embed a tiny WASM binary) that exports alloc, dealloc, and the target
function, then in the test call invoke_sync with that module, verify the alloc
returns a pointer, the call returns expected output, and dealloc frees memory
without leaks or panics; target functions and helpers to touch are invoke_sync,
alloc, dealloc, and any memory marshalling helpers used by invoke_sync so the
full roundtrip is covered.
- Around line 566-612: In host_sql_v1, when the closure returns Err(error) you
should emit a warn-level log with the error details to aid diagnosing SurrealDB
transport/connectivity issues; add a tracing::warn! (or warn!(...) via use
tracing::warn) in the Err(error) arm—e.g. tracing::warn!(error = %error,
"host_sql_v1 transport error contacting SurrealDB")—before building the JSON
error payload so operators see the transport error context; keep the existing
JSON/error response behavior unchanged.
In `@clients/agent-runtime/src/memory/traits.rs`:
- Around line 16-21: The derived Default for MemoryValidationResult is
misleading because it yields valid = false; update the struct by removing
Default from the derive list and provide an explicit impl Default for
MemoryValidationResult that returns valid: true and violations: empty Vec (or
alternatively drop Default entirely to avoid accidental use); locate the
MemoryValidationResult definition and replace the derive(Default) with a custom
impl Default that sets valid = true and violations = Vec::new() so callers using
MemoryValidationResult::default() get a valid result.
In `@clients/agent-runtime/src/plugins/mod.rs`:
- Around line 1935-1994: Add a runtime_api consistency check in
resolve_memory_plugin_runtime: after deserializing PluginManifest into manifest
and after validate_plugin_version(...) compare manifest.runtime_api with the
locked.runtime_api (the value on the locked struct returned by
resolve_memory_plugin) and bail with a clear error if they differ (similar to
the existing id/version mismatch branches); include plugin_id,
locked.runtime_api and manifest.runtime_api in the error message and keep
returning Err via bail! so mismatched runtime_api fails fast before constructing
ResolvedMemoryPlugin.
In `@clients/agent-runtime/src/security/pairing.rs`:
- Around line 145-147: Replace the short-circuiting .any(...) scan with a
non-short-circuiting reduction so every stored token is compared to hashed; in
pairing.rs change the tokens.iter().any(|stored| constant_time_eq(stored,
&hashed)) usage to a fold (or explicit loop) that XORs/ORs (accumulates) the
result of constant_time_eq for each stored token and returns the final boolean,
ensuring constant_time_eq is called for every entry (refer to the tokens
variable and constant_time_eq and hashed identifiers when making the change).
…ut identity regex
Deploying corvus with
|
| Latest commit: |
11ccc44
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://52274bd5.corvus-42x.pages.dev |
| Branch Preview URL: | https://feature-memory-surreal-graph.corvus-42x.pages.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
.agents/skills/plugin-release/SKILL.md (2)
21-42: Add a clean working-tree precondition.The preconditions and the "Verify local state" step lack an explicit requirement that the working tree must be clean before tagging. Without this guard, an agent could create a tag over uncommitted or unstaged changes, resulting in a release whose
Cargo.tomlversion differs from what was actually built.✏️ Proposed additions
Add to the Preconditions list:
2. Plugin version in `Cargo.toml` matches tag semver. 3. Plugin builds for `wasm32-wasip1`. +4. Working tree is clean (`git status` shows no uncommitted changes). -4. Catalog deployment secrets are configured in GitHub: +5. Catalog deployment secrets are configured in GitHub:And tighten Step 1:
### 1) Verify local state ```bash git status git log --oneline -10
+Stop if
git statusreports any uncommitted or staged changes.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/plugin-release/SKILL.md around lines 21 - 42, Update the
SKILL.md preconditions to require a clean working tree before tagging by adding
a bullet under "Preconditions" that the git working tree must have no
uncommitted or staged changes, and modify the "Verify local state" step (the git
status/git log block) to include an explicit directive: "Stop ifgit status
reports any uncommitted or staged changes." Reference the "Preconditions"
section and the "Verify local state" step when making the edits so the guard is
clearly visible to agents following the release flow.</details> --- `131-137`: **Consider adding a verification command for the runtime install check.** Given that this PR enforces signature verification for remote plugins (per the commit notes), the last two checklist items could be more actionable with concrete commands: <details> <summary>✏️ Proposed addition</summary> ```diff -- [ ] Runtime can install plugin via `corvus plugins install <plugin-id>` -- [ ] Runtime verification passes for the installed plugin +- [ ] Runtime can install plugin via `corvus plugins install <plugin-id>` +- [ ] Runtime verification passes: `corvus plugins verify <plugin-id>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/plugin-release/SKILL.md around lines 131 - 137, Update the two checklist lines "Runtime can install plugin via corvus plugins install <plugin-id>" and "Runtime verification passes for the installed plugin" to include a concrete verification command sequence: instruct the releaser to run the install command for the plugin ID and then run the runtime's verification command (e.g., corvus plugins verify <plugin-id> or equivalent) and check for a zero exit code and expected verification output; reference the checklist lines verbatim so the text is replaced/extended with the concrete install + verify commands and an instruction to assert the signature verification succeeded.clients/agent-runtime/Cargo.toml (2)
83-84: Gatewasmibehind an optional feature flag, consistent with other specialized backends.Every other heavy or environment-specific dependency in this file is optional and feature-gated (
memory-surreal,probe,rag-pdf,browser-native,hardware).wasmi— a full WASM interpreter — is currently unconditional, meaning it's compiled into every build including minimal/size-optimized ones, working against the project'sopt-level = "z"/lto = "fat"release philosophy.Consider introducing a
memory-pluginfeature (mirroringmemory-surreal):♻️ Proposed refactor
-# WASM execution for plugin-backed memory backends -wasmi = { version = "0.40", default-features = false, features = ["std"] } +# WASM execution for plugin-backed memory backends (optional; enable with --features memory-plugin) +wasmi = { version = "0.40", optional = true, default-features = false, features = ["std"] }And in
[features]:# memory-surreal = SurrealDB-backed memory backend memory-surreal = ["dep:surrealdb"] +# memory-plugin = WASM plugin-backed memory backend +memory-plugin = ["dep:wasmi"]Based on learnings: "Preserve release-size profile assumptions in
Cargo.tomland avoid adding heavy dependencies unless clearly justified" and "Do not add heavy dependencies for minor convenience; justify new crate additions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/Cargo.toml` around lines 83 - 84, The wasmi dependency should be made optional and moved behind a feature flag (suggested name: memory-plugin) similar to other backends; change the wasmi entry to include optional = true and remove unconditional compile, then add a new feature in the [features] table (e.g., "memory-plugin" = ["wasmi"]) and ensure any crates referencing wasmi (search for references to wasmi or the WASM execution plugin code) are conditionalized with cfg(feature = "memory-plugin") or moved behind the feature so builds without memory-plugin don’t compile wasmi.
198-198: Alignwatversion with the project's exact-pin convention for dev-dependencies.The existing dev-deps (
tempfile = "=3.25.0",criterion = { version = "=0.5.1", ... }) use exact version pins for reproducibility.wat = "1"uses a broad range and may silently pick up minor updates.♻️ Proposed fix
-wat = "1" +wat = "=1.219.1" # pin to the version resolved in Cargo.lock for reproducibility🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/Cargo.toml` at line 198, The dev-dependency "wat" is using a permissive range (wat = "1") whereas the repo pins dev-dependencies to exact versions; change the "wat" entry in Cargo.toml from wat = "1" to an exact-pin form (e.g., wat = "=X.Y.Z") to match the project's convention—use the exact version found in Cargo.lock or the desired exact release instead of a range so it aligns with other dev-deps like "tempfile" and "criterion".clients/agent-runtime/src/agent/mod.rs (1)
8-8: Preferpub(crate)forvalidationunless external crates need the module.Since
enforce_strict_validationispub(crate)anyway, making the modulepub(crate)keeps the API surface tighter without reducing in-crate usability.Proposed change
-pub mod validation; +pub(crate) mod validation;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/agent/mod.rs` at line 8, The module declaration `validation` is exported publicly but its key item `enforce_strict_validation` is already `pub(crate)`, so tighten the API by changing the module visibility from public to crate-only: replace the `pub mod validation;` declaration with a `pub(crate) mod validation;` and update any external usages if they exist so only in-crate code relies on `validation::enforce_strict_validation` (no change needed for internal callers).clients/agent-runtime/src/memory/traits.rs (1)
99-113: UseMemoryValidationResult::default()in the trait default implementation.
Avoids repeating the same “valid + empty violations” construction.Proposed change
async fn validate_response( &self, _user_query: &str, _response: &str, _session_id: Option<&str>, ) -> anyhow::Result<MemoryValidationResult> { - Ok(MemoryValidationResult { - valid: true, - violations: Vec::new(), - }) + Ok(MemoryValidationResult::default()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/traits.rs` around lines 99 - 113, The default implementation of validate_response currently constructs MemoryValidationResult manually; replace that manual construction in the validate_response default method with MemoryValidationResult::default() so it returns Ok(MemoryValidationResult::default()) instead of building { valid: true, violations: Vec::new() }, updating the async fn validate_response in the trait to use the default constructor.clients/agent-runtime/src/memory/plugin.rs (3)
469-480: Minor:usernameis not redacted inDebugoutput.Line 475 prints
usernamein the clear. Depending on the deployment, database usernames may be considered sensitive (e.g., shared or personal accounts). Consider redacting it consistently withpasswordandtoken, or leave as-is if usernames are considered non-secret in this context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 469 - 480, The Debug implementation for SurrealSqlClient exposes the username field in cleartext; update the impl of std::fmt::Debug for SurrealSqlClient to redact the username like password and token (e.g., replace the .field("username", &self.username) with a redacted placeholder), ensuring username is not printed while keeping sql_url, namespace, and database as before.
209-225:validate_responsedefaults tovalid=truewhen the plugin omits the field — fail-open by design?Line 212:
unwrap_or(true)means a plugin that returns an RPC response without a"valid"key will be treated as passing validation. If this is intentional (graceful degradation), atracing::debug!when the field is absent would help during troubleshooting. If validation should be strict,unwrap_or(false)or an explicit error would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 209 - 225, The code currently treats a missing "valid" field as true via let valid = result.get("valid").and_then(Value::as_bool).unwrap_or(true), which causes a fail-open behavior; change this to fail-closed by using unwrap_or(false) (or return an explicit error) so missing validation is not treated as passing, and update the surrounding logic that constructs MemoryValidationResult to reflect this; if you intend graceful degradation instead, detect the absence of the "valid" key (e.g., check result.get("valid").is_none()) and emit a tracing::debug! noting the missing field before defaulting, while keeping the MemoryValidationResult { valid, violations } construction intact.
314-332: Timeout onspawn_blockingdoes not cancel the in-flight task.When
tokio::time::timeoutfires (Line 323), thespawn_blockingtask keeps running — Tokio cannot cancel blocking threads. If the WASM plugin makes a long-running SQL call, the thread stays occupied until completion. This is an inherent limitation ofspawn_blockingand not easily fixable, but be aware that under sustained timeouts you could exhaust the blocking thread pool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 314 - 332, The current use of tokio::task::spawn_blocking in invoke_with_entrypoint with tokio::time::timeout does not cancel the underlying blocking thread when the timeout fires (spawn_blocking / join handle cannot stop a blocking syscall), so long-running WASM/SQL calls can exhaust the blocking thread pool; to fix, refactor the blocking work out of spawn_blocking by making invoke_sync async (or adding an async wrapper like invoke_async) and use tokio::spawn (or an async-compatible plugin call) so the timed-out task can be aborted, or move plugin execution to an external process that can be killed; update invoke_with_entrypoint to call the new async entrypoint (e.g., invoke_async / invoke_entrypoint_async) and await it with tokio::time::timeout instead of using spawn_blocking, and remove or document the spawn_blocking usage to avoid threadpool exhaustion.clients/agent-runtime/src/memory/mod.rs (1)
320-328: Duplicate test coverage for surreal-graphs fallback.
factory_surreal_graphs_without_installed_plugin_falls_back_to_markdown(Line 320) andfactory_surreal_graphs_fallback_when_plugin_missing(Line 414) exercise the same code path with identical setup and assertions. Consider removing one.Also applies to: 414-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/mod.rs` around lines 320 - 328, Two tests duplicate the same coverage: factory_surreal_graphs_without_installed_plugin_falls_back_to_markdown and factory_surreal_graphs_fallback_when_plugin_missing both construct a MemoryConfig with backend "surreal-graphs", call create_memory(...), and assert mem.name() == "markdown"; remove one of them (or consolidate into a single test) to eliminate duplicate coverage and keep the clearer name (pick one of the two function names) while leaving the remaining test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/plugin-release/SKILL.md:
- Around line 64-85: Move the sentence "This triggers `publish-plugins.yml`." so
it immediately follows the git push command in the "Create release tag" section
(right after the two lines that run `git tag -a plugin/<plugin-id>/vX.Y.Z -m
"Release <plugin-id> vX.Y.Z"` and `git push origin plugin/<plugin-id>/vX.Y.Z`)
instead of remaining at the end of the recovery block; update SKILL.md in the
"Create release tag" subsection to place that sentence before the "#### Recovery
(if tag pushed with wrong version)" heading so the workflow trigger is clearly
associated with the initial push.
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 115-136: alloc_v1/dealloc_v1 currently allocate a Vec with
capacity=size but dealloc_v1 uses a parameter named len which is misleading and
must equal the original capacity; update the ABI and implementation so
dealloc_v1's second parameter is clearly the original capacity (e.g., rename len
-> cap) and validate it matches expectations before calling
Vec::from_raw_parts(ptr as *mut u8, 0, cap as usize); ensure alloc_v1's
allocation and returned pointer semantics remain capacity-based and document the
contract in the function comments so callers pass the same cap used at
allocation time.
- Around line 728-816: In sql_query, before calling unsafe {
slice::from_raw_parts(response_ptr as *const u8, len) } ensure you validate that
len <= capacity (and that response_ptr > 0) to avoid UB if the host returns more
bytes than allocated; if len > capacity, deallocate the buffer
(dealloc_v1(response_ptr, i32::try_from(capacity).unwrap_or(i32::MAX))) and
return an Err with a clear message (e.g., "host sql response length exceeds
capacity"), or treat it as a resize signal similar to status=1, but always
perform the dealloc and proper error/resize handling in the sql_query function
to prevent unsafe reads.
- Around line 636-697: validate_response currently swallows DB/query errors by
calling sql_rows(...).unwrap_or_default(), which makes validation silently pass
when the rules lookup fails; change validate_response (and its callers if
necessary) to propagate query errors instead of defaulting: call
sql_rows(&rules_query, None, Some(5_000)) and map any Err into the function Err
return (using anyhow or thiserror as our project convention), returning an Err
with context like "failed querying ontology_rules" rather than treating it as an
empty rule set; update the function's Result error type accordingly and ensure
callers handle the propagated error.
- Around line 461-501: The expansion query builds ids as a Value::Array of
Value::String (expanded_ids -> id_list) which produces plain strings like
"memory_entries:abc" that SurrealDB 2.x won’t match against the record id
column; change the construction of the IN list used by expanded_query so each id
is emitted as a proper record literal or record type (e.g., convert each
expanded_id into a record literal like r"memory_entries:abc" or use
type::record(...) syntax) instead of plain strings — inspect
surreal_string_literal() for a helper or add a helper for record literals and
use it when building expanded_query (the code paths to change are where
expanded_ids is mapped into id_list and where expanded_query is formatted,
leaving sql_rows, row_to_entry and CandidateEntry usage unchanged).
In `@clients/agent-runtime/src/agent/loop_.rs`:
- Around line 1595-1604: The assistant message pushed by run_tool_call_loop is
being validated afterwards by enforce_strict_memory_validation, so history
(e.g., the interactive history object referenced as history or mem) still
contains the unvalidated response; update the same history entry after
validation by replacing the original assistant message with the
validated/corrected response (the value returned by
enforce_strict_memory_validation, currently assigned to response/final_output)
before returning from run_tool_call_loop so subsequent turns use the corrected
text (ensure you locate the push in run_tool_call_loop and replace or update
that specific assistant message rather than appending a new one).
In `@clients/agent-runtime/src/agent/validation.rs`:
- Around line 12-86: Several tracing::warn! calls in the error branches for
mem.validate_response, provider.chat_with_system, and the post-correction
validation are interpolating the raw error ({error}) which can leak sensitive
provider/memory details; replace those warn logs with generic messages that do
not include the error string (e.g., "Memory validation failed" / "Ontology
correction pass failed" / "Post-correction ontology validation failed") and, if
you need diagnostic info, send a redacted or non-sensitive representation to
tracing::debug! or use a helper like redact_error(error) before logging; update
the tracing::warn! invocations around mem.validate_response,
provider.chat_with_system, and the final match arm accordingly so no raw error
content is logged.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 993-997: The test
reject_non_loopback_wss_when_http_loopback_is_disabled is asserting that a wss
endpoint is rejected, but per the updated validation wss (TLS) should be
accepted; change the test to call parse_endpoint("wss://example.com/rpc") and
then assert validate_endpoint_security(&endpoint, false) returns Ok (e.g., use
.unwrap() or assert!(...is_ok())) instead of unwrap_err and the "non-loopback"
assertion so it expects success from validate_endpoint_security.
- Around line 809-830: The "wss" match arm incorrectly treats wss as insecure —
update the scheme handling so "wss" is accepted the same as "https": remove the
extra non-loopback restriction in the "wss" arm and ensure it returns Ok(())
unconditionally (matching the "https" arm), and fix the error message text if
any remaining checks reference "http" for a TLS scheme; update or delete the
test reject_non_loopback_wss_when_http_loopback_is_disabled accordingly. Use the
existing symbols allow_http_loopback and is_loopback_host and modify the "wss"
match arm in the scheme validation block to mirror the "https" behavior.
---
Nitpick comments:
In @.agents/skills/plugin-release/SKILL.md:
- Around line 21-42: Update the SKILL.md preconditions to require a clean
working tree before tagging by adding a bullet under "Preconditions" that the
git working tree must have no uncommitted or staged changes, and modify the
"Verify local state" step (the git status/git log block) to include an explicit
directive: "Stop if `git status` reports any uncommitted or staged changes."
Reference the "Preconditions" section and the "Verify local state" step when
making the edits so the guard is clearly visible to agents following the release
flow.
- Around line 131-137: Update the two checklist lines "Runtime can install
plugin via corvus plugins install <plugin-id>" and "Runtime verification passes
for the installed plugin" to include a concrete verification command sequence:
instruct the releaser to run the install command for the plugin ID and then run
the runtime's verification command (e.g., corvus plugins verify <plugin-id> or
equivalent) and check for a zero exit code and expected verification output;
reference the checklist lines verbatim so the text is replaced/extended with the
concrete install + verify commands and an instruction to assert the signature
verification succeeded.
In `@clients/agent-runtime/Cargo.toml`:
- Around line 83-84: The wasmi dependency should be made optional and moved
behind a feature flag (suggested name: memory-plugin) similar to other backends;
change the wasmi entry to include optional = true and remove unconditional
compile, then add a new feature in the [features] table (e.g., "memory-plugin" =
["wasmi"]) and ensure any crates referencing wasmi (search for references to
wasmi or the WASM execution plugin code) are conditionalized with cfg(feature =
"memory-plugin") or moved behind the feature so builds without memory-plugin
don’t compile wasmi.
- Line 198: The dev-dependency "wat" is using a permissive range (wat = "1")
whereas the repo pins dev-dependencies to exact versions; change the "wat" entry
in Cargo.toml from wat = "1" to an exact-pin form (e.g., wat = "=X.Y.Z") to
match the project's convention—use the exact version found in Cargo.lock or the
desired exact release instead of a range so it aligns with other dev-deps like
"tempfile" and "criterion".
In `@clients/agent-runtime/src/agent/mod.rs`:
- Line 8: The module declaration `validation` is exported publicly but its key
item `enforce_strict_validation` is already `pub(crate)`, so tighten the API by
changing the module visibility from public to crate-only: replace the `pub mod
validation;` declaration with a `pub(crate) mod validation;` and update any
external usages if they exist so only in-crate code relies on
`validation::enforce_strict_validation` (no change needed for internal callers).
In `@clients/agent-runtime/src/memory/mod.rs`:
- Around line 320-328: Two tests duplicate the same coverage:
factory_surreal_graphs_without_installed_plugin_falls_back_to_markdown and
factory_surreal_graphs_fallback_when_plugin_missing both construct a
MemoryConfig with backend "surreal-graphs", call create_memory(...), and assert
mem.name() == "markdown"; remove one of them (or consolidate into a single test)
to eliminate duplicate coverage and keep the clearer name (pick one of the two
function names) while leaving the remaining test unchanged.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 469-480: The Debug implementation for SurrealSqlClient exposes the
username field in cleartext; update the impl of std::fmt::Debug for
SurrealSqlClient to redact the username like password and token (e.g., replace
the .field("username", &self.username) with a redacted placeholder), ensuring
username is not printed while keeping sql_url, namespace, and database as
before.
- Around line 209-225: The code currently treats a missing "valid" field as true
via let valid = result.get("valid").and_then(Value::as_bool).unwrap_or(true),
which causes a fail-open behavior; change this to fail-closed by using
unwrap_or(false) (or return an explicit error) so missing validation is not
treated as passing, and update the surrounding logic that constructs
MemoryValidationResult to reflect this; if you intend graceful degradation
instead, detect the absence of the "valid" key (e.g., check
result.get("valid").is_none()) and emit a tracing::debug! noting the missing
field before defaulting, while keeping the MemoryValidationResult { valid,
violations } construction intact.
- Around line 314-332: The current use of tokio::task::spawn_blocking in
invoke_with_entrypoint with tokio::time::timeout does not cancel the underlying
blocking thread when the timeout fires (spawn_blocking / join handle cannot stop
a blocking syscall), so long-running WASM/SQL calls can exhaust the blocking
thread pool; to fix, refactor the blocking work out of spawn_blocking by making
invoke_sync async (or adding an async wrapper like invoke_async) and use
tokio::spawn (or an async-compatible plugin call) so the timed-out task can be
aborted, or move plugin execution to an external process that can be killed;
update invoke_with_entrypoint to call the new async entrypoint (e.g.,
invoke_async / invoke_entrypoint_async) and await it with tokio::time::timeout
instead of using spawn_blocking, and remove or document the spawn_blocking usage
to avoid threadpool exhaustion.
In `@clients/agent-runtime/src/memory/traits.rs`:
- Around line 99-113: The default implementation of validate_response currently
constructs MemoryValidationResult manually; replace that manual construction in
the validate_response default method with MemoryValidationResult::default() so
it returns Ok(MemoryValidationResult::default()) instead of building { valid:
true, violations: Vec::new() }, updating the async fn validate_response in the
trait to use the default constructor.
…e allocation handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
clients/agent-runtime/src/memory/plugin.rs (4)
756-763:category_to_stringduplicates theDisplayimpl onMemoryCategory.
MemoryCategory::Displayintraits.rs(Lines 46-55) produces identical output. You could replace calls tocategory_to_string(&cat)withcat.to_string()and remove this function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 756 - 763, category_to_string duplicates the Display impl for MemoryCategory; remove the category_to_string function and replace its call sites with the existing Display implementation by calling to_string() on MemoryCategory (e.g., replace category_to_string(&cat) with cat.to_string()); ensure no remaining references to category_to_string exist and run cargo build/tests to confirm compilation after deleting the function in plugin.rs.
871-888: Consider the same bounds check inread_memory_bytesfor defense-in-depth.While callers of
read_memory_bytesalready validateresp_lenagainstmax_output_bytes, adding a linear-memory bounds check here would guard against future call sites that might skip the external validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 871 - 888, Add an explicit linear-memory bounds check inside read_memory_bytes to defend against out-of-range reads: after validating ptr and len non-negative and converting len to usize, compute the end offset (ptr as usize + len_usize) and compare it against the plugin memory's current byte length (use wasmi Memory API such as memory.size/store page size conversion or the appropriate method to get current memory size) and return an error/bail!("out-of-bounds memory access") if the end offset exceeds memory length; keep the existing read call and error context intact.
309-317: Unnecessary string clones on every invocation for entrypoint names.
self.inner.memory_entrypoint.clone()andself.inner.health_entrypoint.clone()allocate on every call. Since the executor isClone(viaArc<Inner>), you could pass theArcinto the blocking task and read the entrypoint frominnerinside. Alternatively, store entrypoints asArc<str>for cheap cloning.As per coding guidelines: "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 309 - 317, The current invoke_memory and invoke_health methods clone the entrypoint strings every call (self.inner.memory_entrypoint.clone(), self.inner.health_entrypoint.clone()); instead avoid per-call allocations by either passing the Arc<Inner> into invoke_with_entrypoint and reading inner.memory_entrypoint / inner.health_entrypoint inside the blocking task, or change the stored types to Arc<str> so cloning is cheap; update invoke_memory and invoke_health to stop calling clone() on memory_entrypoint/health_entrypoint and instead forward the Arc<Inner> (self.inner) or use Arc<str> fields, and adjust invoke_with_entrypoint signature/usage accordingly to read the entrypoint from Inner at execution time (reference: methods invoke_memory, invoke_health, invoke_with_entrypoint and field inner.memory_entrypoint / inner.health_entrypoint).
591-625:host_sql_v1error response includes rawerror.to_string()sent back to the WASM plugin.Lines 613 and 619-621: the raw error (which may include hostname/port from connection failures) is both logged and included in the JSON payload returned to the plugin. Since plugins must be trusted (per the resolution flow in
mod.rs), and are sandboxed without network access, this is low risk — but worth noting for defense-in-depth.As per coding guidelines: "
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 591 - 625, host_sql_v1 currently includes the raw error.to_string() in the JSON payload and logs it directly; remove raw error material from both the transport response and high-level logs. Change the Err branch that builds payload and tracing::warn to: (1) log only a non-sensitive, generic message via tracing::warn (or include a sanitized/hashed error id produced by a new helper like sanitize_error(error) if you need correlation), and (2) populate the JSON payload with a generic error.message (e.g. "internal server error" or the error code "HOST_SQL_ERROR") instead of error.to_string(); keep detailed error details only in a secure/internal debug log or a scrubbed/sanitized form if required. Ensure you update the construction site around host_sql_v1, the outcome Err arm, and the serde_json::to_vec call that creates the response.clients/agent-runtime/src/agent/validation.rs (1)
36-41: Consider bounding the correction prompt size.If
candidateorviolations_textis very large, the correction prompt could exceed provider token limits or become expensive. Consider truncating them to a reasonable ceiling before formatting the prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/agent/validation.rs` around lines 36 - 41, The correction_prompt construction can blow up if candidate or violations_text are huge; modify the code around correction_prompt to truncate candidate and violations_text to a safe ceiling before formatting (e.g., implement a helper like truncate_to_max_chars_or_tokens and apply it to candidate and violations_text), keeping user_query intact or also capped if needed, and then build correction_prompt from the truncated strings; ensure the truncation preserves the end or start of the text as appropriate and document the chosen max length constant.clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs (2)
251-338: Destructurerequestbefore thematchto avoid the sixargs.clone()calls.Each arm calls
request.args.clone()andrequest.request_id.clone(). Destructuring first lets the arms takeargsandrequest_idby move.♻️ Proposed refactor
- let result = match request.op.as_str() { - "store" => { - let args: StoreArgs = - serde_json::from_value(request.args.clone()).map_err(|error| { - ( - request.request_id.clone(), - "INVALID_ARGS".to_string(), - format!("store args invalid: {error}"), - ) - })?; + let PluginRequest { request_id, op, args } = request; + let result = match op.as_str() { + "store" => { + let args: StoreArgs = serde_json::from_value(args).map_err(|error| { + ( + request_id.clone(), + "INVALID_ARGS".to_string(), + format!("store args invalid: {error}"), + ) + })?; // ... repeat for the other arms, replacing request.args.clone() → args // and request.request_id.clone() → request_id.clone()Based on learnings: avoid unnecessary allocations and clones to maintain performance and efficiency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 251 - 338, Destructure the incoming Request before the match to avoid repeatedly cloning; e.g., bind let Request { op, args, request_id, .. } = request; (or similar) and then match on op (or op.as_str()) using the moved args and request_id so each branch uses args and request_id by value instead of calling request.args.clone() and request.request_id.clone() in the "store", "recall", "get", "list", "forget", and "validate_response" arms; ensure any further uses that need owned values take ownership or clone only when truly necessary.
600-602: Hard-codedLIMIT 1000can silently hitMAX_OUTPUT_BYTESand return error code 8.
ListArgshas nolimitfield. With content-heavy entries, 1000 rows easily exceeds 256 KB, and callers receive a bare integer error code with no message. Consider adding an optionallimit(defaulting to a safe constant ≤MAX_RECALL_LIMIT) toListArgs, similar toRecallArgs.♻️ Proposed refactor
#[derive(Debug, Deserialize)] struct ListArgs { #[serde(default)] category: Option<String>, #[serde(default)] session_id: Option<String>, + #[serde(default)] + limit: Option<usize>, }- let query = - format!("SELECT * FROM memory_entries{where_clause} ORDER BY updated_at DESC LIMIT 1000;"); + let limit = args.limit.unwrap_or(MAX_RECALL_LIMIT).clamp(1, MAX_RECALL_LIMIT); + let query = + format!("SELECT * FROM memory_entries{where_clause} ORDER BY updated_at DESC LIMIT {limit};");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 600 - 602, The query uses a hard-coded "LIMIT 1000" which can exceed MAX_OUTPUT_BYTES and cause silent error code 8; add an optional limit field to ListArgs (like RecallArgs) with a default safe constant (<= MAX_RECALL_LIMIT), validate/cap incoming limits against MAX_RECALL_LIMIT, and replace the hard-coded "LIMIT 1000" in the query construction (the `query` string that selects from `memory_entries`) with the validated limit; ensure callers that omit the new ListArgs.limit get the default and keep using `sql_rows(&query, None, Some(6_000))?` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/plugin-release/SKILL.md:
- Around line 94-101: Clarify whether the <version> placeholder in the artifact
path artifacts/<plugin-id>/<version>/<plugin-id>.wasm uses the v-prefixed tag
(v<semver>, e.g., v0.2.0) or the bare semver from Cargo.toml (e.g., 0.2.0);
update SKILL.md to explicitly state which format the release workflow produces
(match the tag format used in the repository’s release/tagging) and give one
concrete example using artifacts/<plugin-id>/... with the correct form so
operators know whether to look for artifacts/<plugin-id>/v0.2.0/... or
artifacts/<plugin-id>/0.2.0/....
- Line 95: Clarify the ambiguity in the artifact path
`artifacts/<plugin-id>/<version>/<plugin-id>.wasm` by stating whether
`<version>` includes the `v` prefix used in the tag format `v<semver>` or the
bare semver from `Cargo.toml`; update SKILL.md near the listed artifact path
(and optionally add a parenthetical example) to explicitly say that `<version>`
is the bare semver (e.g., `0.2.0`) and not the tag-prefixed form `v0.2.0`.
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 627-640: The forget_memory function currently ignores errors from
the two SQL deletions (the sql_query calls for delete_entry_query and
delete_edges_query) and always returns Ok(true); change this so you capture and
handle the Result from each sql_query: call sql_query for delete_entry_query and
propagate errors (using ? or matching) or return Ok(false) when deletions report
failure, then do the same for the delete_edges_query; ensure the function
returns an error or false on failure instead of unconditionally Ok(true) so
callers get accurate deletion status.
- Around line 224-228: The current sequence calls write_i32(resp_ptr_out,
resp_ptr) then write_i32(resp_len_out, resp_len_i32) and if the second fails the
host has already received resp_ptr; instead, pre-validate both output slots
before writing: add/use a small helper to assert both resp_ptr_out and
resp_len_out are writable (e.g., validate_write_i32_slot or similar) and if
validation fails dealloc_v1(resp_ptr, resp_len_i32) and return the error,
otherwise perform both write_i32 calls; reference: write_i32, resp_ptr_out,
resp_len_out, dealloc_v1, resp_ptr, resp_len_i32.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 920-937: The function read_caller_memory_bytes currently allocates
bytes from the untrusted len before validating against the caller's linear
memory; change it to first get the caller memory size via
memory.data_size(caller), verify ptr as usize <= data_size (bail if out of
bounds), compute available = data_size - ptr as usize, cap len_usize =
len_usize.min(available) (or bail if you prefer zero-length), then allocate
vec![0u8; len_usize] and proceed to memory.read; apply the same defensive cap to
read_memory_bytes as well.
---
Duplicate comments:
In @.agents/skills/plugin-release/SKILL.md:
- Line 74: The placement of the line "This triggers `publish-plugins.yml`." is
correct — it should remain immediately after the `git push` command and before
the Recovery block; no code change required, just mark the review comment as
resolved and remove the duplicate reviewer tag or duplicate comment entry if
present in the review thread referencing that exact string.
---
Nitpick comments:
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 251-338: Destructure the incoming Request before the match to
avoid repeatedly cloning; e.g., bind let Request { op, args, request_id, .. } =
request; (or similar) and then match on op (or op.as_str()) using the moved args
and request_id so each branch uses args and request_id by value instead of
calling request.args.clone() and request.request_id.clone() in the "store",
"recall", "get", "list", "forget", and "validate_response" arms; ensure any
further uses that need owned values take ownership or clone only when truly
necessary.
- Around line 600-602: The query uses a hard-coded "LIMIT 1000" which can exceed
MAX_OUTPUT_BYTES and cause silent error code 8; add an optional limit field to
ListArgs (like RecallArgs) with a default safe constant (<= MAX_RECALL_LIMIT),
validate/cap incoming limits against MAX_RECALL_LIMIT, and replace the
hard-coded "LIMIT 1000" in the query construction (the `query` string that
selects from `memory_entries`) with the validated limit; ensure callers that
omit the new ListArgs.limit get the default and keep using `sql_rows(&query,
None, Some(6_000))?` unchanged.
In `@clients/agent-runtime/src/agent/validation.rs`:
- Around line 36-41: The correction_prompt construction can blow up if candidate
or violations_text are huge; modify the code around correction_prompt to
truncate candidate and violations_text to a safe ceiling before formatting
(e.g., implement a helper like truncate_to_max_chars_or_tokens and apply it to
candidate and violations_text), keeping user_query intact or also capped if
needed, and then build correction_prompt from the truncated strings; ensure the
truncation preserves the end or start of the text as appropriate and document
the chosen max length constant.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 756-763: category_to_string duplicates the Display impl for
MemoryCategory; remove the category_to_string function and replace its call
sites with the existing Display implementation by calling to_string() on
MemoryCategory (e.g., replace category_to_string(&cat) with cat.to_string());
ensure no remaining references to category_to_string exist and run cargo
build/tests to confirm compilation after deleting the function in plugin.rs.
- Around line 871-888: Add an explicit linear-memory bounds check inside
read_memory_bytes to defend against out-of-range reads: after validating ptr and
len non-negative and converting len to usize, compute the end offset (ptr as
usize + len_usize) and compare it against the plugin memory's current byte
length (use wasmi Memory API such as memory.size/store page size conversion or
the appropriate method to get current memory size) and return an
error/bail!("out-of-bounds memory access") if the end offset exceeds memory
length; keep the existing read call and error context intact.
- Around line 309-317: The current invoke_memory and invoke_health methods clone
the entrypoint strings every call (self.inner.memory_entrypoint.clone(),
self.inner.health_entrypoint.clone()); instead avoid per-call allocations by
either passing the Arc<Inner> into invoke_with_entrypoint and reading
inner.memory_entrypoint / inner.health_entrypoint inside the blocking task, or
change the stored types to Arc<str> so cloning is cheap; update invoke_memory
and invoke_health to stop calling clone() on memory_entrypoint/health_entrypoint
and instead forward the Arc<Inner> (self.inner) or use Arc<str> fields, and
adjust invoke_with_entrypoint signature/usage accordingly to read the entrypoint
from Inner at execution time (reference: methods invoke_memory, invoke_health,
invoke_with_entrypoint and field inner.memory_entrypoint /
inner.health_entrypoint).
- Around line 591-625: host_sql_v1 currently includes the raw error.to_string()
in the JSON payload and logs it directly; remove raw error material from both
the transport response and high-level logs. Change the Err branch that builds
payload and tracing::warn to: (1) log only a non-sensitive, generic message via
tracing::warn (or include a sanitized/hashed error id produced by a new helper
like sanitize_error(error) if you need correlation), and (2) populate the JSON
payload with a generic error.message (e.g. "internal server error" or the error
code "HOST_SQL_ERROR") instead of error.to_string(); keep detailed error details
only in a secure/internal debug log or a scrubbed/sanitized form if required.
Ensure you update the construction site around host_sql_v1, the outcome Err arm,
and the serde_json::to_vec call that creates the response.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
clients/agent-runtime/src/agent/validation.rs (2)
25-32: Error details are fully discarded — safe but considertracing::debug!for diagnostics.All three error branches correctly avoid logging sensitive content per the coding guidelines. However, binding the error and logging it at
debug!level would preserve diagnostic value for local troubleshooting without exposing it in production warn-level logs.This is a minor observability tradeoff and fine to leave as-is if the team prefers minimal log surface.
Also applies to: 71-78, 98-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/agent/validation.rs` around lines 25 - 32, Bind the error returned by mem.validate_response (replace Err(_) with Err(err)) and emit the error details at debug level (e.g., tracing::debug!("Memory validation failed: {:?}", err)); keep the existing tracing::warn! message and the same user-facing return string intact. Apply the same change to the other validate_response match sites referenced (the blocks around the other Err(_) cases) so all three places log diagnostics at debug level while still not exposing error details at warn level.
38-47: Duplicated violations formatting logic.The pattern of formatting
Vec<String>violations into a bulleted list (check if empty → map with"- "prefix → join with"\n") appears twice. A small helper would reduce duplication.Proposed helper
+fn format_violations(violations: &[String], fallback: &str) -> String { + if violations.is_empty() { + format!("- {fallback}") + } else { + violations.iter().map(|v| format!("- {v}")).collect::<Vec<_>>().join("\n") + } +}Then use it in both call sites:
- let violations_text = if validation.violations.is_empty() { - "- unknown ontology violation".to_string() - } else { - validation - .violations - .iter() - .map(|item| format!("- {item}")) - .collect::<Vec<_>>() - .join("\n") - }; + let violations_text = format_violations(&validation.violations, "unknown ontology violation");Also applies to: 82-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/agent/validation.rs` around lines 38 - 47, Extract the repeated "check empty → prefix each item with \"- \" → join with \"\n\"" logic into a small helper function (e.g. format_bulleted_violations or format_violations) and use it wherever you currently build violations_text from validation.violations; specifically replace the logic that constructs violations_text (which inspects validation.violations and maps to "- {item}" then joins) and the similar block used later (the other call site that formats validation.violations) with calls to the new helper so the formatting is centralized.clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs (4)
505-545:extract_terms(query)is called twice with the same input inrecall_memory.The first call drives the ontology expansion (line 505) and the second builds the term set for scoring (line 545). Both operate on the same
querystring. Computing once and reusing avoids the redundant allocation.♻️ Proposed refactor
+ let query_terms = extract_terms(query); + let mut expanded_ids = HashSet::new(); - for term in extract_terms(query) { + for term in &query_terms { ... } ... - let terms = extract_terms(query); + let terms = query_terms; let mut ranked: Vec<EntryOut> = candidates🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 505 - 545, The function recall_memory currently calls extract_terms(query) twice — once for ontology expansion and again later for scoring — causing redundant allocation; compute let terms = extract_terms(query) once before the first loop and reuse that same terms variable in both the ontology-expansion loop (the for term in ... block that builds term_hash/term_record_id and runs edge_lookup) and the later scoring logic instead of calling extract_terms(query) again, ensuring you remove the second call and reference the precomputed terms wherever needed (look for extract_terms usages inside recall_memory to replace).
233-237:write_i32error branch (lines 233–237) is unreachable after the pre-validation above.
validate_write_i32_slot(lines 226–231) already rejectsptr <= 0;write_i32checks the same condition before performingcopy_nonoverlapping, which traps rather than returning anErrin WASM when the address is out-of-bounds. The second dealloc + return block is dead code. Removing it reduces noise and makes it clear that the allocation is only conditionally freed.♻️ Proposed cleanup
- if write_i32(resp_ptr_out, resp_ptr).is_err() || write_i32(resp_len_out, resp_len_i32).is_err() - { - dealloc_v1(resp_ptr, resp_len_i32); - return 7; - } + // Safety: both slot pointers validated above. + let _ = write_i32(resp_ptr_out, resp_ptr); + let _ = write_i32(resp_len_out, resp_len_i32);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 233 - 237, The error-handling branch that calls dealloc_v1(resp_ptr, resp_len_i32) and returns 7 after attempting write_i32 is dead/unnecessary because validate_write_i32_slot already rejects non-positive pointers and write_i32 will trap on out-of-bounds rather than return Err; remove that unreachable if-branch and its dealloc+return so the allocation free is only done in the existing conditional paths and the code relies on validate_write_i32_slot and write_i32 behavior; update/remove references to resp_ptr/resp_len_out in that branch and keep a single clear allocation-free path.
433-442: Prefer parameterized queries viavarsover string-interpolated literals throughout.
HostSqlRequestalready carries avars: Option<Value>field, but every call-site passesvars: Noneand builds queries by interpolatingsurreal_string_literal(…)values. While the JSON-based escaping is safe against injection, parameterized queries are the idiomatic approach: they remove the escaping surface entirely, produce cleaner SQL, and make queries easier to test and inspect. The same pattern applies to allsql_query/sql_rowscall sites in this file (recall_memory,forget_memory, etc.).Example for
store_memory:♻️ Illustrative refactor for one query
- let upsert_query = format!( - "UPSERT type::thing(\"memory_entries\", {id}) CONTENT {{ key: {key}, ... }};", - id = surreal_string_literal(&key_hash), - key = surreal_string_literal(key), - ... - ); - sql_query(&upsert_query, None, Some(5_000))?; + let upsert_query = "UPSERT type::thing('memory_entries', $id) CONTENT { key: $key, content: $content, category: $category, timestamp: $ts, updated_at: $ts, session_id: $sid };"; + sql_query(upsert_query, Some(json!({ + "id": key_hash, + "key": key, + "content": content, + "category": category, + "ts": now, + "sid": args.session_id, + })), Some(5_000))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 433 - 442, The upsert in store_memory (the formatted upsert_query using surreal_string_literal) should be converted to a parameterized query: replace the interpolated values with named placeholders (e.g., $id, $key, $content, $category, $timestamp, $updated_at, $session_id), build a serde_json::Value map for these variables and pass it as HostSqlRequest.vars: Some(vars) instead of vars: None, and stop calling surreal_string_literal for those fields; apply the same refactor pattern to the other sql_query/sql_rows call sites in this file (recall_memory, forget_memory, etc.) so all queries use vars and placeholders and avoid string interpolation.
391-415: Schema initialisation failure is permanent for the lifetime of the WASM instance.
OnceLock::get_or_initruns the closure exactly once and caches the outcome. If the DB is transiently unavailable during the first request, every subsequent request for this instance returnsSCHEMA_INIT_FAILEDwithout retrying — the only recovery path is to restart (or reload) the WASM instance.Consider documenting this restart-required failure mode explicitly in the plugin manifest or in the error message returned to the caller, so the host can detect it and evict/reload the instance rather than retrying indefinitely on the same broken one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 391 - 415, The schema init currently caches any failure permanently via SCHEMA_INIT_RESULT.get_or_init in ensure_schema, so update ensure_schema (and the closure passed to get_or_init) to wrap and return errors that explicitly state this is a permanent WASM-instance-level initialization failure and recommend host eviction/reload (e.g., include text like "schema bootstrap failed; this instance must be restarted/evicted"), and ensure sql_query error branches (the closure used by get_or_init) propagate that augmented message so callers of ensure_schema can detect the restart-required condition; reference ensure_schema, SCHEMA_INIT_RESULT, get_or_init, and sql_query when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/plugin-release/SKILL.md:
- Around line 76-88: Update the "Recovery (if tag pushed with wrong version)"
section in SKILL.md to include cleanup steps for the race window when
publish-plugins.yml ran before the tag was deleted: instruct operators to
inspect the GitHub Actions run for the wrong tag, remove any published
Cloudflare Pages artifact at the immutable path (e.g.,
artifacts/<plugin-id>/<wrong-version>/) via the Cloudflare Pages dashboard or
Pages API, and revert any catalog.json or manifest upsert that points to the
wrong version before re-tagging and republishing; place this note adjacent to
the existing git tag delete instructions so it's visible during recovery.
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 361-370: read_input currently allows req_ptr == 0 which leads to
unsafe reads from address 0; make its null-pointer check consistent with other
functions by rejecting ptr <= 0. Update read_input to return Err when req_ptr <=
0 (matching write_i32 and validate_write_i32_slot behavior) and include the same
error message format ("negative request pointer/length" or similar) so callers
of read_input, write_i32, and validate_write_i32_slot behave consistently when
given a zero or negative pointer.
- Around line 527-531: In recall_memory, expanded_query is missing the session
filter so ontology-expanded ids can return entries from other sessions; modify
the expanded_query construction (the format call that builds "SELECT * FROM
memory_entries WHERE id IN [{ids}] LIMIT {limit};") to include the same
session_filter predicate used by keyword_query (and incorporate session_id when
present) — e.g., append the session_filter condition to the WHERE clause or
inject the session predicate into the format args so expanded_query enforces the
session constraint just like keyword_query does.
- Around line 714-715: The SELECT used to build rules_query in validate_response
currently appends session_filter which excludes global rules (those without
session_id) because NONE = "…" is false; update the query construction in
validate_response (the rules_query/ session_filter logic) so the WHERE clause
allows either session_id IS NONE OR session_id = "<session>" while still
requiring enabled = true (i.e., build WHERE enabled = true AND (session_id IS
NONE OR session_id = "<session>") LIMIT 200) so global ontology_rules are
returned alongside per-session rules.
- Around line 458-465: store_memory currently issues a CREATE for memory_edges
which inserts a new auto-ID row each call; change the edge creation so it
upserts a deterministic edge ID (or does a SELECT-and-CREATE-if-not-exists) to
prevent duplicate edges. In the store_memory logic that builds edge_query (the
variable named edge_query near the sql_query call) compute a deterministic ID
from the edge key components (e.g., concatenate relation_type, from_id, to_id
and session_id using surreal_string_literal/option_literal helpers), then emit
an UPSERT (or UPSERT type::thing(deterministic_id) / SELECT ... THEN CREATE IF
NOT EXISTS) for memory_edges with that deterministic ID and the same CONTENT
payload, and keep the existing sql_query call and timeout. Reference
functions/vars: store_memory, edge_query, memory_edges, surreal_string_literal,
option_literal, recall_memory (ensure recall still works with dedup).
- Around line 858-863: In the status==1 (resize) path you're setting capacity =
len even when len == 0 which causes repeated zero-length allocations and
exhausting retries; change the resize logic so that if len == 0 you pick a
non-zero capacity (e.g. capacity = std::cmp::max(1, len) or capacity =
capacity.saturating_add(1)) before continuing, and only dealloc_v1(response_ptr,
...) when a non-zero capacity was previously allocated; update the block that
touches response_ptr, dealloc_v1, capacity, and len so capacity never becomes
zero on a resize retry.
In `@clients/agent-runtime/src/agent/validation.rs`:
- Around line 49-57: The correction prompt currently truncates candidate and
violations_text but not user_query, which can let very large queries bloat the
prompt; add a new truncation limit constant (e.g., MAX_PROMPT_USER_QUERY_CHARS)
and apply truncate_for_prompt to user_query (create a truncated_user_query
variable) and use that truncated_user_query in the format! call that builds
correction_prompt (refer to truncate_for_prompt and the correction_prompt
construction in validation.rs).
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 325-343: The semaphore permit (obtained via
invocation_gate.acquire_owned()) is being moved into the spawn_blocking closure
(let _permit = permit) so it is held until the blocking task finishes even if
tokio::time::timeout returns; remove moving the permit into the closure so the
permit stays in the async scope and is dropped on timeout. Concretely, in the
code around invocation_gate.acquire_owned(), keep the permit variable in the
outer async scope and do not reference or move it into the closure passed to
tokio::task::spawn_blocking (remove or avoid the line let _permit = permit
inside that closure); leave only the executor and request moved into the closure
so timeout will drop the permit on error while the blocking invoke_sync call
(executor.invoke_sync(...)) continues independently.
---
Duplicate comments:
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 827-836: The error messages in the "ws" match arm of the SurrealDB
URL validation reuse text referring to "http", which is misleading for WebSocket
URLs; update the bail!() messages inside the "ws" arm (in
clients/agent-runtime/src/memory/plugin.rs) to reference "ws" or "WebSocket"
instead of "http" — e.g., change "http is disabled" to "ws is disabled" (or
"WebSocket is disabled") and adjust the non-loopback message to say "refusing
insecure SurrealDB URL over ws for non-loopback host '{host}'" so both checks
correctly describe the ws scheme.
---
Nitpick comments:
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 505-545: The function recall_memory currently calls
extract_terms(query) twice — once for ontology expansion and again later for
scoring — causing redundant allocation; compute let terms = extract_terms(query)
once before the first loop and reuse that same terms variable in both the
ontology-expansion loop (the for term in ... block that builds
term_hash/term_record_id and runs edge_lookup) and the later scoring logic
instead of calling extract_terms(query) again, ensuring you remove the second
call and reference the precomputed terms wherever needed (look for extract_terms
usages inside recall_memory to replace).
- Around line 233-237: The error-handling branch that calls dealloc_v1(resp_ptr,
resp_len_i32) and returns 7 after attempting write_i32 is dead/unnecessary
because validate_write_i32_slot already rejects non-positive pointers and
write_i32 will trap on out-of-bounds rather than return Err; remove that
unreachable if-branch and its dealloc+return so the allocation free is only done
in the existing conditional paths and the code relies on validate_write_i32_slot
and write_i32 behavior; update/remove references to resp_ptr/resp_len_out in
that branch and keep a single clear allocation-free path.
- Around line 433-442: The upsert in store_memory (the formatted upsert_query
using surreal_string_literal) should be converted to a parameterized query:
replace the interpolated values with named placeholders (e.g., $id, $key,
$content, $category, $timestamp, $updated_at, $session_id), build a
serde_json::Value map for these variables and pass it as HostSqlRequest.vars:
Some(vars) instead of vars: None, and stop calling surreal_string_literal for
those fields; apply the same refactor pattern to the other sql_query/sql_rows
call sites in this file (recall_memory, forget_memory, etc.) so all queries use
vars and placeholders and avoid string interpolation.
- Around line 391-415: The schema init currently caches any failure permanently
via SCHEMA_INIT_RESULT.get_or_init in ensure_schema, so update ensure_schema
(and the closure passed to get_or_init) to wrap and return errors that
explicitly state this is a permanent WASM-instance-level initialization failure
and recommend host eviction/reload (e.g., include text like "schema bootstrap
failed; this instance must be restarted/evicted"), and ensure sql_query error
branches (the closure used by get_or_init) propagate that augmented message so
callers of ensure_schema can detect the restart-required condition; reference
ensure_schema, SCHEMA_INIT_RESULT, get_or_init, and sql_query when making the
change.
In `@clients/agent-runtime/src/agent/validation.rs`:
- Around line 25-32: Bind the error returned by mem.validate_response (replace
Err(_) with Err(err)) and emit the error details at debug level (e.g.,
tracing::debug!("Memory validation failed: {:?}", err)); keep the existing
tracing::warn! message and the same user-facing return string intact. Apply the
same change to the other validate_response match sites referenced (the blocks
around the other Err(_) cases) so all three places log diagnostics at debug
level while still not exposing error details at warn level.
- Around line 38-47: Extract the repeated "check empty → prefix each item with
\"- \" → join with \"\n\"" logic into a small helper function (e.g.
format_bulleted_violations or format_violations) and use it wherever you
currently build violations_text from validation.violations; specifically replace
the logic that constructs violations_text (which inspects validation.violations
and maps to "- {item}" then joins) and the similar block used later (the other
call site that formats validation.violations) with calls to the new helper so
the formatting is centralized.
…r query truncation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
clients/agent-runtime/src/memory/plugin.rs (3)
488-510: Credential redaction inDebugimpl looks good.Manual
Debugimplementation correctly redactspasswordandtoken. Note thatusername(line 505) is also redacted — typically usernames aren't considered secrets. Consider showing the actual username value (likesql_url,namespace,database) to improve debuggability, while keepingpasswordandtokenredacted.♻️ Optional: show username for better diagnostics
impl std::fmt::Debug for SurrealSqlClient { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("SurrealSqlClient") .field("sql_url", &self.sql_url) .field("namespace", &self.namespace) .field("database", &self.database) - .field("username", &"<redacted>") + .field("username", &self.username) .field("password", &"<redacted>") .field("token", &"<redacted>") .finish_non_exhaustive() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 488 - 510, The Debug implementation for SurrealSqlClient currently redacts the username like password/token; update the impl for SurrealSqlClient (the fmt method) to print the actual username value instead of the literal "<redacted>" (e.g., change .field("username", &"<redacted>") to .field("username", &self.username)) while keeping password and token redacted and preserving finish_non_exhaustive().
933-960: Same silent-truncation concern applies toread_caller_memory_bytes.The
requested_len.min(available)on line 954 silently truncates plugin-provided request data. A plugin claiming a request length larger than its own linear memory is a programming error — failing explicitly would surface bugs faster.♻️ Bail on out-of-bounds instead of silent truncation
#[allow(clippy::cast_sign_loss)] let requested_len = len as usize; let available = data_size - ptr_usize; - let len_usize = requested_len.min(available); + if requested_len > available { + bail!( + "host import request length ({requested_len}) exceeds available memory ({available}) at offset {ptr_usize}" + ); + } + let len_usize = requested_len; let mut bytes = vec![0u8; len_usize];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 933 - 960, The function read_caller_memory_bytes silently truncates when a plugin requests more bytes than remain in its linear memory; change the logic to detect requested_len > available and return an error (bail!) instead of using requested_len.min(available). Specifically, in read_caller_memory_bytes (and where ptr_usize and requested_len are computed) replace the min-based truncation with an explicit check that bails with a clear message like "requested out-of-bounds read" when requested_len exceeds available, so the subsequent memory.read call only runs with a verified in-bounds length.
874-901:read_memory_bytessilently truncates whenrequested_len > available.If the plugin reports a response length larger than the remaining linear memory from
ptr, this function silently returns fewer bytes (line 895). Downstream JSON deserialization will fail with a confusing parse error rather than a clear bounds-violation message. Consider bailing instead of truncating, since a mismatch here signals a buggy/corrupt plugin.♻️ Bail on out-of-bounds instead of silent truncation
#[allow(clippy::cast_sign_loss)] let requested_len = len as usize; let available = data_size - ptr_usize; - let len_usize = requested_len.min(available); + if requested_len > available { + bail!( + "plugin response length ({requested_len}) exceeds available memory ({available}) at offset {ptr_usize}" + ); + } + let len_usize = requested_len; let mut bytes = vec![0u8; len_usize];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/memory/plugin.rs` around lines 874 - 901, The function read_memory_bytes currently silently truncates when the plugin's requested_len exceeds available bytes; update read_memory_bytes to detect if requested_len (len as usize) > available and return an explicit error (bail!) instead of using requested_len.min(available) and returning fewer bytes. Locate the ptr_usize/requested_len/available logic in read_memory_bytes and replace the min-based truncation with a bounds check that bails with a clear out-of-bounds message (include requested and available sizes for clarity) before calling memory.read.clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs (2)
639-684:forget_memoryedge-delete failure blocks the overall operation.The entry delete (line 661-668) correctly checks success. However, edge deletion (lines 674-681) also returns
Ok(false)on failure, even though the entry itself has already been deleted. This is stricter than necessary — orphaned edges from a past-deleted entry are harmless (they won't match anything inrecall_memory), and a transient DB hiccup during edge cleanup would cause the caller to seedeleted=falsedespite the entry being gone.Consider making edge cleanup best-effort:
♻️ Make edge cleanup best-effort
let delete_edges_query = format!( "DELETE memory_edges WHERE from_id = {id} OR to_id = {id};", id = surreal_string_literal(&entry_record_id), ); - let delete_edges_payload = sql_query(&delete_edges_query, None, Some(4_000))?; - if !delete_edges_payload - .get("ok") - .and_then(Value::as_bool) - .unwrap_or(false) - { - return Ok(false); - } + // Edge cleanup is best-effort; orphaned edges are harmless. + let _ = sql_query(&delete_edges_query, None, Some(4_000)); Ok(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 639 - 684, In forget_memory, the edge deletion (delete_edges_query / delete_edges_payload via sql_query) currently aborts and returns Ok(false) if the edge-delete response isn't "ok", which incorrectly makes the whole operation fail even after the entry has been removed; change this to best-effort: after running sql_query for delete_edges_query, do not return Ok(false) on a non-ok payload — instead log the failure (or ignore it) and proceed to return Ok(true) since the primary entry deletion (delete_entry_query) already succeeded. Ensure you update the handling around delete_edges_payload so failures do not cause the function to report the deletion as unsuccessful.
913-938:extract_termsexcludes all tokens shorter than 4 characters.The minimum-length filter on line 924 drops potentially important short domain terms (e.g., "API", "SQL", "GPU", "RSS", "JWT"). If the ontology contains such terms, they'll never match during recall or validation.
A lower bound of 3 might be more appropriate, or consider allowing short terms if they appear in the ontology.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs` around lines 913 - 938, The extract_terms function currently filters out any token shorter than 4, which drops valid short domain tokens; change the length check from if normalized.len() < 4 to if normalized.len() < 3 to allow 3-char tokens, and optionally extend extract_terms to accept a whitelist of ontology terms (e.g., add a parameter ontology_terms: &HashSet<String>) and move the length check after a whitelist membership test so tokens present in ontology_terms are preserved even if shorter than 3; keep references to MAX_TERMS, stop_words, and the unique HashSet behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/plugin-release/SKILL.md:
- Around line 100-103: The docs in SKILL.md list signature and certificate
filenames without their artifact directories, which is misleading; update the
two entries that currently show `<plugin-id>.wasm.sig` and
`<plugin-id>.wasm.pem` to include the full generated paths
`artifacts/<plugin-id>/<version>/<plugin-id>.wasm.sig` and
`artifacts/<plugin-id>/<version>/<plugin-id>.wasm.pem` so they match the actual
workflow output, ensuring any mention of those filenames elsewhere in the file
is similarly updated.
---
Nitpick comments:
In `@clients/agent-runtime/plugins/memory-surreal-graphs/src/lib.rs`:
- Around line 639-684: In forget_memory, the edge deletion (delete_edges_query /
delete_edges_payload via sql_query) currently aborts and returns Ok(false) if
the edge-delete response isn't "ok", which incorrectly makes the whole operation
fail even after the entry has been removed; change this to best-effort: after
running sql_query for delete_edges_query, do not return Ok(false) on a non-ok
payload — instead log the failure (or ignore it) and proceed to return Ok(true)
since the primary entry deletion (delete_entry_query) already succeeded. Ensure
you update the handling around delete_edges_payload so failures do not cause the
function to report the deletion as unsuccessful.
- Around line 913-938: The extract_terms function currently filters out any
token shorter than 4, which drops valid short domain tokens; change the length
check from if normalized.len() < 4 to if normalized.len() < 3 to allow 3-char
tokens, and optionally extend extract_terms to accept a whitelist of ontology
terms (e.g., add a parameter ontology_terms: &HashSet<String>) and move the
length check after a whitelist membership test so tokens present in
ontology_terms are preserved even if shorter than 3; keep references to
MAX_TERMS, stop_words, and the unique HashSet behavior unchanged.
In `@clients/agent-runtime/src/memory/plugin.rs`:
- Around line 488-510: The Debug implementation for SurrealSqlClient currently
redacts the username like password/token; update the impl for SurrealSqlClient
(the fmt method) to print the actual username value instead of the literal
"<redacted>" (e.g., change .field("username", &"<redacted>") to
.field("username", &self.username)) while keeping password and token redacted
and preserving finish_non_exhaustive().
- Around line 933-960: The function read_caller_memory_bytes silently truncates
when a plugin requests more bytes than remain in its linear memory; change the
logic to detect requested_len > available and return an error (bail!) instead of
using requested_len.min(available). Specifically, in read_caller_memory_bytes
(and where ptr_usize and requested_len are computed) replace the min-based
truncation with an explicit check that bails with a clear message like
"requested out-of-bounds read" when requested_len exceeds available, so the
subsequent memory.read call only runs with a verified in-bounds length.
- Around line 874-901: The function read_memory_bytes currently silently
truncates when the plugin's requested_len exceeds available bytes; update
read_memory_bytes to detect if requested_len (len as usize) > available and
return an explicit error (bail!) instead of using requested_len.min(available)
and returning fewer bytes. Locate the ptr_usize/requested_len/available logic in
read_memory_bytes and replace the min-based truncation with a bounds check that
bails with a clear out-of-bounds message (include requested and available sizes
for clarity) before calling memory.read.
yuniel-acosta
approved these changes
Feb 22, 2026
This was referenced Mar 3, 2026
Merged
This was referenced Mar 28, 2026
This was referenced Apr 9, 2026
Merged
Merged
This was referenced Apr 11, 2026
Merged
This was referenced Apr 19, 2026
This was referenced Apr 29, 2026
8 tasks
8 tasks
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
2 participants
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.
This pull request introduces strict ontology validation and correction for agent responses, ensuring that outputs comply with domain-specific rules before being returned to users. It also adds a new plugin release management skill, improves plugin-backed memory initialization, and updates dependencies for WebAssembly plugin support. Below are the most important changes grouped by theme:
Strict Ontology Validation and Correction:
Introduced
enforce_strict_memory_validationlogic across agent, CLI, and channel flows to validate responses against memory ontologies, attempt automatic correction using the LLM if violations are found, and provide user-friendly fallback messages if validation fails. [1] [2] [3] [4] [5] [6] [7] [8] [9]Added a new
MemoryValidationResultstruct to standardize ontology validation results, including validity and a list of violations.Plugin-backed Memory Improvements:
Improved plugin-backed memory backend initialization to use the new
resolve_memory_plugin_runtimefunction, handle initialization errors gracefully, and fall back to markdown memory if plugin loading fails.Updated dependencies for the
memory-surreal-graphsplugin to includehex,serde,serde_json, andsha2for better WASM support.Added the
wasmicrate to the agent runtime for WASM execution in plugin-backed memory backends.Documentation and Release Workflow:
plugin-releaseskill with detailed documentation for releasing Corvus runtime plugins, covering tagging, artifact publishing, signing, and Cloudflare Pages deployment. [1] [2]Minor Updates:
Updated the README to fix the Tailwind CSS documentation link.
Exposed
MemoryValidationResultin thememorymodule's public API.Summary by CodeRabbit
New Features
Documentation