kernel convergence: safety scan rename, agent loop gate routing, provider status CLI#256
Merged
kernel convergence: safety scan rename, agent loop gate routing, provider status CLI#256
Conversation
…_tool() Agent loop tool calls now get taint tracking (sink checks + output labeling) and input safety checks, matching the MCP server path. Removes duplicated output safety checks and metrics recording. Closes #249
…ction Shows resolved providers, wrappers (retry/fallback), and quota usage. Redacts API keys in output. Closes #250
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant AL as Agent Loop
participant KG as Kernel Gate
participant TR as Tool Registry
participant SL as Safety Layer
participant TE as Taint Engine
rect rgba(220,240,220,0.5)
Note over AL,TR: Old flow (pre-change)
AL->>TR: execute_with_context(...)
TR-->>AL: tool_result
AL->>SL: check_tool_output(output)
end
rect rgba(200,220,255,0.5)
Note over AL,KG: New unified flow (post-change)
AL->>KG: execute_tool(request, taint?)
KG->>SL: scan(input_text, CheckDirection::Input)
SL-->>KG: safety_decision
KG->>TE: taint_check(input)
TE-->>KG: taint_state
KG->>TR: execute_with_context(...)
TR-->>KG: tool_result
KG->>SL: scan(output_text, CheckDirection::Output)
SL-->>KG: safety_decision
KG->>TE: taint_check(output)
TE-->>KG: taint_state
KG-->>AL: (for_llm, success, Some(output))
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kernel/gate.rs (1)
74-80:⚠️ Potential issue | 🔴 CriticalApply sanitized/redacted scan output before returning tool output.
Line [84] gets
SafetyResult, but non-blocking modifications are discarded. This can leak content that safety intended to redact/sanitize.🔧 Proposed fix
- let output = match registry.execute_with_context(name, input, ctx).await { + let mut output = match registry.execute_with_context(name, input, ctx).await { @@ if let Some(safety_layer) = safety { let result = safety_layer.scan(&output.for_llm, CheckDirection::Output); if result.blocked { @@ result.warnings.join("; ") ))); } + if result.was_modified { + output.for_llm = result.content; + } }Also applies to: 83-93, 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernel/gate.rs` around lines 74 - 80, The tool output from registry.execute_with_context (used in the gate.rs flow) is currently returned before applying SafetyResult modifications, risking leakage of content that should be redacted; update the code paths that handle the Ok(output) case (including the branches around metrics.record_tool_call and the subsequent returns) to run the output through the sanitizer/ SafetyResult application function (apply or merge the sanitized/redacted fields from SafetyResult into the tool output) and only return the sanitized output (also ensure errors still record metrics.record_tool_call with the appropriate success flag). Locate usages of registry.execute_with_context, the variable named output, and any SafetyResult handling and make sure non-blocking modifications from SafetyResult are applied to output prior to returning.
🧹 Nitpick comments (1)
src/safety/mod.rs (1)
114-123: UseCheckDirectionto avoid misleading scan diagnostics.Line [114] ignores
CheckDirection, while Line [122] always reports"Output truncated..."even for input scans. This will produce confusing warnings for kernel input checks.🔧 Proposed fix
-pub fn scan(&self, text: &str, _direction: CheckDirection) -> SafetyResult { +pub fn scan(&self, text: &str, direction: CheckDirection) -> SafetyResult { @@ + let direction_label = match direction { + CheckDirection::Input => "Input", + CheckDirection::Output => "Output", + }; let content = if text.len() > self.config.max_output_length { @@ - "Output truncated from {} to {} bytes", + "{} truncated from {} to {} bytes", + direction_label, text.len(), self.config.max_output_length, ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/safety/mod.rs` around lines 114 - 123, The scan method currently ignores the CheckDirection parameter (it’s named _direction) and always pushes "Output truncated..." which mislabels input scans; remove the underscore so the parameter is used (change _direction to direction) and branch on direction (e.g., if direction == CheckDirection::Output then set was_modified and push "Output truncated from {} to {} bytes", else for CheckDirection::Input either push "Input truncated..." or omit the truncation warning as appropriate); update any references in scan to use the direction variable and ensure the warnings message uses the correct label when comparing text.len() to self.config.max_output_length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/loop.rs`:
- Around line 1171-1177: The branch handling kernel::execute_tool() currently
treats any Ok(output) as success; update the Ok arm to detect ToolOutput::error
(match the ToolOutput enum or use an is_error helper) and treat it as a failure:
if output is an error, produce an error LLM message (e.g. format!("Error: {}",
...)) and return None for the ToolOutput so the caller treats it like a failure
instead of transitioning to ToolDone/after_tool; apply the same change to the
similar block(s) later (around the 1183-1236 region) so all
Ok(ToolOutput::error(...)) cases are handled as failures rather than successes.
In `@src/cli/provider.rs`:
- Around line 47-49: The status printing currently outputs s.api_base raw (see
the use of s.api_base in the status code); change this to parse the URL and
redact sensitive components before printing: detect and remove or mask userinfo
(username:password@) and mask any key-bearing query parameters or values (e.g.,
tokens, api_key, access_token) in the URL’s query string, then print the
redacted URL instead of the raw s.api_base string; implement the redaction logic
in a small helper (e.g., redact_api_base or redact_url) and call it where
s.api_base is printed to keep provider output safe and consistent.
- Around line 10-13: The provider status command runs blocking disk I/O on the
async runtime; wrap the synchronous calls (Config::load and
QuotaStore::load_or_default) inside tokio::task::spawn_blocking and await their
results from cmd_provider (or inside print_provider_status if that’s where they
live) so the async executor isn't blocked; locate uses of Config::load and
QuotaStore::load_or_default referenced when handling ProviderSubcommand::Status
/ print_provider_status, move those sync loads into spawn_blocking closures,
propagate any errors back to cmd_provider and handle them as before.
In `@src/safety/mod.rs`:
- Around line 119-127: The current truncation takes a byte slice
&text[..self.config.max_output_length] which can panic if max_output_length
splits a UTF-8 codepoint; change it to find the nearest valid UTF-8 boundary
before slicing. In the block referencing text and self.config.max_output_length,
compute a safe_cut starting at max_output_length and move it down until
text.is_char_boundary(safe_cut) (handle the zero case) and then slice
&text[..safe_cut]; keep the existing was_modified/warnings logic but ensure the
warning still reports the original byte lengths if desired.
---
Outside diff comments:
In `@src/kernel/gate.rs`:
- Around line 74-80: The tool output from registry.execute_with_context (used in
the gate.rs flow) is currently returned before applying SafetyResult
modifications, risking leakage of content that should be redacted; update the
code paths that handle the Ok(output) case (including the branches around
metrics.record_tool_call and the subsequent returns) to run the output through
the sanitizer/ SafetyResult application function (apply or merge the
sanitized/redacted fields from SafetyResult into the tool output) and only
return the sanitized output (also ensure errors still record
metrics.record_tool_call with the appropriate success flag). Locate usages of
registry.execute_with_context, the variable named output, and any SafetyResult
handling and make sure non-blocking modifications from SafetyResult are applied
to output prior to returning.
---
Nitpick comments:
In `@src/safety/mod.rs`:
- Around line 114-123: The scan method currently ignores the CheckDirection
parameter (it’s named _direction) and always pushes "Output truncated..." which
mislabels input scans; remove the underscore so the parameter is used (change
_direction to direction) and branch on direction (e.g., if direction ==
CheckDirection::Output then set was_modified and push "Output truncated from {}
to {} bytes", else for CheckDirection::Input either push "Input truncated..." or
omit the truncation warning as appropriate); update any references in scan to
use the direction variable and ensure the warnings message uses the correct
label when comparing text.len() to self.config.max_output_length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dee9c67-5394-4b51-a36c-b559651e35bd
📒 Files selected for processing (11)
AGENTS.mdCLAUDE.mdsrc/agent/loop.rssrc/cli/common.rssrc/cli/mod.rssrc/cli/provider.rssrc/kernel/gate.rssrc/kernel/mod.rssrc/mcp_server/handler.rssrc/safety/mod.rssrc/safety/taint.rs
Comment on lines
1171
to
1177
| Ok(output) => { | ||
| let elapsed = tool_start.elapsed(); | ||
| let latency_ms = elapsed.as_millis() as u64; | ||
| debug!(tool = %name, latency_ms = latency_ms, "Tool executed successfully"); | ||
| hooks.after_tool(&name, &output.for_llm, elapsed, channel_name, chat_id); | ||
| if let Some(tx) = tool_feedback_tx.read().await.as_ref() { | ||
| let _ = tx.send(ToolFeedback { | ||
| tool_name: name.clone(), | ||
| phase: ToolFeedbackPhase::Done { elapsed_ms: latency_ms }, | ||
| }); | ||
| } | ||
| #[cfg(feature = "panel")] | ||
| if let Some(bus) = &event_bus { | ||
| bus.send(crate::api::events::PanelEvent::ToolDone { | ||
| tool: name.clone(), | ||
| duration_ms: latency_ms, | ||
| }); | ||
| } | ||
| // Send to user if tool opted in | ||
| if let Some(ref user_msg) = output.for_user { | ||
| let mut outbound = crate::bus::OutboundMessage::new( | ||
| ctx.channel.as_deref().unwrap_or(""), | ||
| ctx.chat_id.as_deref().unwrap_or(""), | ||
| user_msg, | ||
| ); | ||
| // Propagate routing metadata (e.g. telegram_thread_id) | ||
| if let Some(tid) = inbound_meta.get("telegram_thread_id") { | ||
| outbound.metadata.insert("telegram_thread_id".to_string(), tid.clone()); | ||
| } | ||
| let _ = bus_for_tools.publish_outbound(outbound).await; | ||
| } | ||
| (output.for_llm, !output.is_error) | ||
| let for_llm = output.for_llm.clone(); | ||
| (for_llm, Some(output)) | ||
| } | ||
| Err(e) => { | ||
| let elapsed = tool_start.elapsed(); | ||
| let latency_ms = elapsed.as_millis() as u64; | ||
| error!(tool = %name, latency_ms = latency_ms, error = %e, "Tool execution failed"); | ||
| hooks.on_error(&name, &e.to_string(), channel_name, chat_id); | ||
| if let Some(metrics) = usage_metrics.as_ref() { | ||
| metrics.record_error(); | ||
| } | ||
| if let Some(tx) = tool_feedback_tx.read().await.as_ref() { | ||
| let _ = tx.send(ToolFeedback { | ||
| tool_name: name.clone(), | ||
| phase: ToolFeedbackPhase::Failed { | ||
| elapsed_ms: latency_ms, | ||
| error: e.to_string(), | ||
| }, | ||
| }); | ||
| } | ||
| #[cfg(feature = "panel")] | ||
| if let Some(bus) = &event_bus { | ||
| bus.send(crate::api::events::PanelEvent::ToolFailed { | ||
| tool: name.clone(), | ||
| error: e.to_string(), | ||
| }); | ||
| } | ||
| (format!("Error: {}", e), false) | ||
| (format!("Error: {}", e), None) | ||
| } |
There was a problem hiding this comment.
Treat ToolOutput::error as failure in the non-streaming branch.
kernel::execute_tool() can return Ok(ToolOutput::error(...)), but this branch marks any Some(output) as success (ToolDone, after_tool). That misreports blocked/error tool executions.
🐛 Proposed fix
- let (result, tool_output) = {
+ let (result, success, tool_output) = {
let tools_guard = tools.read().await;
match crate::kernel::execute_tool(
&tools_guard,
&name,
args,
&ctx,
safety.as_ref().map(|s| s.as_ref()),
&metrics_collector,
taint.as_ref().map(|t| t.as_ref()),
)
.await
{
Ok(output) => {
+ let success = !output.is_error;
let for_llm = output.for_llm.clone();
- (for_llm, Some(output))
+ (for_llm, success, Some(output))
}
Err(e) => {
- (format!("Error: {}", e), None)
+ (format!("Error: {}", e), false, None)
}
}
};
@@
- if let Some(output) = tool_output {
+ if success {
debug!(tool = %name, latency_ms = latency_ms, "Tool executed successfully");
hooks.after_tool(&name, &result, elapsed, channel_name, chat_id);
@@
- if let Some(ref user_msg) = output.for_user {
+ if let Some(output) = tool_output.as_ref() {
+ if let Some(ref user_msg) = output.for_user {
let mut outbound = crate::bus::OutboundMessage::new(
ctx.channel.as_deref().unwrap_or(""),
ctx.chat_id.as_deref().unwrap_or(""),
user_msg,
);
@@
- let _ = bus_for_tools.publish_outbound(outbound).await;
+ let _ = bus_for_tools.publish_outbound(outbound).await;
+ }
}
} else {Also applies to: 1183-1236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/loop.rs` around lines 1171 - 1177, The branch handling
kernel::execute_tool() currently treats any Ok(output) as success; update the Ok
arm to detect ToolOutput::error (match the ToolOutput enum or use an is_error
helper) and treat it as a failure: if output is an error, produce an error LLM
message (e.g. format!("Error: {}", ...)) and return None for the ToolOutput so
the caller treats it like a failure instead of transitioning to
ToolDone/after_tool; apply the same change to the similar block(s) later (around
the 1183-1236 region) so all Ok(ToolOutput::error(...)) cases are handled as
failures rather than successes.
Comment on lines
+10
to
+13
| pub(crate) fn cmd_provider(action: ProviderSubcommand) -> Result<()> { | ||
| match action { | ||
| ProviderSubcommand::Status => print_provider_status(), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the provider.rs file to see current implementation
echo "=== src/cli/provider.rs content ==="
wc -l src/cli/provider.rs
cat -n src/cli/provider.rsRepository: qhkm/zeptoclaw
Length of output: 4310
🏁 Script executed:
#!/bin/bash
# Check src/cli/mod.rs for provider command handling
echo "=== Searching for provider command handling in src/cli/mod.rs ==="
rg -n "Commands::Provider|cmd_provider" src/cli/mod.rs -A 3 -B 1Repository: qhkm/zeptoclaw
Length of output: 326
🏁 Script executed:
#!/bin/bash
# Verify if Config::load and QuotaStore::load_or_default are synchronous
echo "=== Checking Config::load definition ==="
fd -e rs | xargs rg -l "impl Config" | head -5
rg -n "fn load" --type rs | grep -i config | head -10
echo "=== Checking QuotaStore::load_or_default definition ==="
fd -e rs | xargs rg -l "QuotaStore" | head -5
rg -n "load_or_default" --type rs | head -10Repository: qhkm/zeptoclaw
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Check the context of where cmd_provider is called - is it in an async function?
echo "=== Context around line 623 in src/cli/mod.rs ==="
sed -n '600,650p' src/cli/mod.rs | cat -nRepository: qhkm/zeptoclaw
Length of output: 2126
🏁 Script executed:
#!/bin/bash
# Check Config::load definition
echo "=== Config::load definition ==="
rg -n "fn load" src/config/mod.rs -A 5 | head -20Repository: qhkm/zeptoclaw
Length of output: 590
🏁 Script executed:
#!/bin/bash
# Check QuotaStore::load_or_default definition
echo "=== QuotaStore::load_or_default definition ==="
rg -n "load_or_default" src/providers/quota.rs -A 5 | head -20Repository: qhkm/zeptoclaw
Length of output: 572
Move provider status disk I/O off the async runtime thread.
Config::load() and QuotaStore::load_or_default() are synchronous I/O operations running on the async runtime. Wrap them with tokio::task::spawn_blocking to prevent blocking the async executor, as required by the async-first design guidelines.
Proposed fix
-pub(crate) fn cmd_provider(action: ProviderSubcommand) -> Result<()> {
+pub(crate) async fn cmd_provider(action: ProviderSubcommand) -> Result<()> {
match action {
- ProviderSubcommand::Status => print_provider_status(),
+ ProviderSubcommand::Status => print_provider_status().await,
}
}
-fn print_provider_status() -> Result<()> {
- let config = Config::load()?;
+async fn print_provider_status() -> Result<()> {
+ let config = tokio::task::spawn_blocking(Config::load)
+ .await
+ .map_err(|e| anyhow::anyhow!("Config loader task panicked: {e}"))??;
let selections = resolve_runtime_providers(&config);
...
- let store = QuotaStore::load_or_default();
+ let store = tokio::task::spawn_blocking(QuotaStore::load_or_default)
+ .await
+ .map_err(|e| anyhow::anyhow!("Quota store loader task panicked: {e}"))?;--- a/src/cli/mod.rs
+++ b/src/cli/mod.rs
@@ -621,7 +621,7 @@
}
Some(Commands::Provider { action }) => {
- provider::cmd_provider(action)?;
+ provider::cmd_provider(action).await?;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/provider.rs` around lines 10 - 13, The provider status command runs
blocking disk I/O on the async runtime; wrap the synchronous calls (Config::load
and QuotaStore::load_or_default) inside tokio::task::spawn_blocking and await
their results from cmd_provider (or inside print_provider_status if that’s where
they live) so the async executor isn't blocked; locate uses of Config::load and
QuotaStore::load_or_default referenced when handling ProviderSubcommand::Status
/ print_provider_status, move those sync loads into spawn_blocking closures,
propagate any errors back to cmd_provider and handle them as before.
Comment on lines
+47
to
+49
| if let Some(ref base) = s.api_base { | ||
| println!(" api_base: {}", base); | ||
| } |
There was a problem hiding this comment.
Redact sensitive parts of api_base before printing.
api_base is printed raw, which can leak embedded tokens/keys. The provider status output should redact key-bearing query/userinfo values.
🔒 Proposed fix
+fn redact_api_base(api_base: &str) -> String {
+ let Some((base, query)) = api_base.split_once('?') else {
+ return api_base.to_string();
+ };
+ let redacted_query = query
+ .split('&')
+ .map(|kv| {
+ let (k, v) = kv.split_once('=').unwrap_or((kv, ""));
+ let key = k.to_ascii_lowercase();
+ let sensitive = matches!(
+ key.as_str(),
+ "api_key" | "apikey" | "key" | "token" | "access_token"
+ );
+ if sensitive && !v.is_empty() {
+ format!("{k}=****")
+ } else {
+ kv.to_string()
+ }
+ })
+ .collect::<Vec<_>>()
+ .join("&");
+ format!("{base}?{redacted_query}")
+}
...
- if let Some(ref base) = s.api_base {
- println!(" api_base: {}", base);
+ if let Some(ref base) = s.api_base {
+ println!(" api_base: {}", redact_api_base(base));
}Based on learnings: "Keep provider wiring consistent across config, onboarding, status output, and runtime behavior in Rust".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(ref base) = s.api_base { | |
| println!(" api_base: {}", base); | |
| } | |
| fn redact_api_base(api_base: &str) -> String { | |
| let Some((base, query)) = api_base.split_once('?') else { | |
| return api_base.to_string(); | |
| }; | |
| let redacted_query = query | |
| .split('&') | |
| .map(|kv| { | |
| let (k, v) = kv.split_once('=').unwrap_or((kv, "")); | |
| let key = k.to_ascii_lowercase(); | |
| let sensitive = matches!( | |
| key.as_str(), | |
| "api_key" | "apikey" | "key" | "token" | "access_token" | |
| ); | |
| if sensitive && !v.is_empty() { | |
| format!("{k}=****") | |
| } else { | |
| kv.to_string() | |
| } | |
| }) | |
| .collect::<Vec<_>>() | |
| .join("&"); | |
| format!("{base}?{redacted_query}") | |
| } | |
| if let Some(ref base) = s.api_base { | |
| println!(" api_base: {}", redact_api_base(base)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/provider.rs` around lines 47 - 49, The status printing currently
outputs s.api_base raw (see the use of s.api_base in the status code); change
this to parse the URL and redact sensitive components before printing: detect
and remove or mask userinfo (username:password@) and mask any key-bearing query
parameters or values (e.g., tokens, api_key, access_token) in the URL’s query
string, then print the redacted URL instead of the raw s.api_base string;
implement the redaction logic in a small helper (e.g., redact_api_base or
redact_url) and call it where s.api_base is printed to keep provider output safe
and consistent.
Comment on lines
+119
to
127
| let content = if text.len() > self.config.max_output_length { | ||
| was_modified = true; | ||
| warnings.push(format!( | ||
| "Output truncated from {} to {} bytes", | ||
| input.len(), | ||
| text.len(), | ||
| self.config.max_output_length, | ||
| )); | ||
| &input[..self.config.max_output_length] | ||
| &text[..self.config.max_output_length] | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the exact byte slicing pattern mentioned
rg -nP '&text\[\.\.self\.config\.max_output_length\]' src/safety/mod.rs -C 2Repository: qhkm/zeptoclaw
Length of output: 225
🏁 Script executed:
# Check the overall structure and relevant lines around 119-127
fd "mod.rs" src/safety/ -x wc -l {}Repository: qhkm/zeptoclaw
Length of output: 79
🏁 Script executed:
# Read the safety/mod.rs file to see actual implementation
head -150 src/safety/mod.rs | tail -50Repository: qhkm/zeptoclaw
Length of output: 1882
🏁 Script executed:
# Search for SafetyLayer struct definition and config
ast-grep --pattern 'struct SafetyLayer { $$$ }'Repository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Check if there's any existing UTF-8 safe truncation
rg -n 'is_char_boundary|truncate|utf' src/safety/mod.rsRepository: qhkm/zeptoclaw
Length of output: 276
🏁 Script executed:
# Search for SafetyLayerConfig struct to understand max_output_length
ast-grep --pattern 'struct SafetyLayerConfig { $$$ }'Repository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Get more context around the SafetyLayer definition
rg -n 'struct SafetyLayer' src/safety/mod.rs -A 10Repository: qhkm/zeptoclaw
Length of output: 381
🏁 Script executed:
# Search for where max_output_length is defined and used
rg -n 'max_output_length' src/safety/mod.rs -B 2 -A 2Repository: qhkm/zeptoclaw
Length of output: 1710
🏁 Script executed:
# Check if there are tests that validate the truncation behavior
rg -n 'truncat' src/safety/mod.rs -B 3 -A 3Repository: qhkm/zeptoclaw
Length of output: 1990
🏁 Script executed:
# Check config initialization to understand max_output_length semantics
rg -n 'max_output_length.*=' src/ -B 1 -A 1Repository: qhkm/zeptoclaw
Length of output: 266
🏁 Script executed:
# Check the test more carefully to see what input is used
sed -n '319,335p' src/safety/mod.rsRepository: qhkm/zeptoclaw
Length of output: 629
🏁 Script executed:
# Look for how tool outputs are passed to scan - check for any pre-processing
rg -n 'scan\(' src/ --type rs -B 2 -A 1 | head -40Repository: qhkm/zeptoclaw
Length of output: 84
🏁 Script executed:
# Check if there are any real-world tool outputs with multibyte chars being tested
rg -n 'multibyte|utf|char|encoding' src/safety/mod.rsRepository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Verify the actual behavior - test with a string containing multibyte UTF-8
python3 << 'EOF'
# Simulate what happens with the current code
text = "Hello 世界 test" # Contains multibyte UTF-8 characters
max_output_length = 8
print(f"Text: {text}")
print(f"Text bytes: {text.encode('utf-8')}")
print(f"Text len: {len(text)}")
print(f"Text byte len: {len(text.encode('utf-8'))}")
print(f"max_output_length: {max_output_length}")
# This is what Rust does with &text[..n]
# It treats the string as bytes
try:
# In Rust, this would be the equivalent operation
text_bytes = text.encode('utf-8')
truncated = text_bytes[:max_output_length].decode('utf-8')
print(f"Truncated safely: {truncated}")
except UnicodeDecodeError as e:
print(f"ERROR - Would panic in Rust: {e}")
EOFRepository: qhkm/zeptoclaw
Length of output: 282
🏁 Script executed:
# Check where tool outputs come from and if they could contain UTF-8
rg -n 'scan\(' src/ -B 2 -A 1 | grep -v BinaryRepository: qhkm/zeptoclaw
Length of output: 15218
🏁 Script executed:
# Look at the tools module to understand what kind of output is generated
fd "tools" src/ -type dRepository: qhkm/zeptoclaw
Length of output: 228
🏁 Script executed:
# Check if there are examples of tool outputs with non-ASCII
rg -n 'output|result' src/tools -l | head -5Repository: qhkm/zeptoclaw
Length of output: 159
🏁 Script executed:
# Verify the is_char_boundary check exists in std library
python3 << 'EOF'
# Confirm Rust's is_char_boundary semantics
text = "Hello 世界"
text_bytes = text.encode('utf-8')
print(f"String: {text}")
print(f"Bytes: {text_bytes}")
print(f"Char boundaries in Rust would be at positions where is_char_boundary(pos) == true")
print(f"Valid positions: 0 (start of 'H'), 5 (start of '世 at byte pos 5), etc.")
print()
print("For byte position 8 (middle of multibyte char):")
print(f" is_char_boundary(8) would be False - CANNOT slice here")
print(f" This causes panic in Rust")
EOFRepository: qhkm/zeptoclaw
Length of output: 402
🏁 Script executed:
# Check the actual comment about why max_output_length is in bytes
sed -n '36,37p' src/safety/mod.rsRepository: qhkm/zeptoclaw
Length of output: 154
🏁 Script executed:
# See if there's any sanitization of tool outputs before they reach scan
rg -n 'tool.*output|execute.*tool' src/ -lRepository: qhkm/zeptoclaw
Length of output: 358
🏁 Script executed:
# Check what happens to tool outputs before they reach scan()
# Look at how tools.rs or types.rs handles output
head -100 src/tools/types.rsRepository: qhkm/zeptoclaw
Length of output: 3653
🏁 Script executed:
# Check the gate.rs more carefully to see if there's pre-processing
sed -n '40,90p' src/kernel/gate.rsRepository: qhkm/zeptoclaw
Length of output: 1965
🏁 Script executed:
# Look for any base64 stripping or pre-processing mentioned in learnings
rg -n 'base64|strip|sanitize' src/tools/ -A 2Repository: qhkm/zeptoclaw
Length of output: 19457
🏁 Script executed:
# Check if tool outputs are converted to string safely before reaching scan
rg -n 'for_llm|to_string' src/tools/types.rs -B 2 -A 2Repository: qhkm/zeptoclaw
Length of output: 3437
🏁 Script executed:
# Verify what kind of data tool outputs contain - are they JSON, text, base64?
sed -n '1,50p' src/tools/types.rsRepository: qhkm/zeptoclaw
Length of output: 1838
🏁 Script executed:
# Verify that tool outputs can contain non-ASCII UTF-8
rg -n 'for_llm.*=' src/tools/ --type-list | head -20Repository: qhkm/zeptoclaw
Length of output: 447
🏁 Script executed:
# Check if there are any existing UTF-8 safety mechanisms
rg -n 'char_boundary|is_valid_utf8|utf8' src/safety/mod.rsRepository: qhkm/zeptoclaw
Length of output: 40
🏁 Script executed:
# Confirm the test only uses ASCII
sed -n '319,332p' src/safety/mod.rsRepository: qhkm/zeptoclaw
Length of output: 570
🏁 Script executed:
# Verify the fix approach (is_char_boundary exists in Rust std)
python3 << 'EOF'
# Verify the truncate_utf8 function logic
def truncate_utf8(s, max_bytes):
"""Simulate the proposed fix"""
if len(s.encode('utf-8')) <= max_bytes:
return s
text_bytes = s.encode('utf-8')
end = max_bytes
# Find the previous valid char boundary
while end > 0:
try:
text_bytes[:end].decode('utf-8')
return text_bytes[:end].decode('utf-8')
except UnicodeDecodeError:
end -= 1
return ""
# Test with real scenario
test_cases = [
("Hello 世界 world", 8), # Cuts mid-character
("你好世界", 5), # 3-byte chars
("Normal ASCII text here", 15),
]
for text, limit in test_cases:
result = truncate_utf8(text, limit)
print(f"Input: {text!r}, Limit: {limit}")
print(f" Original would panic/fail")
print(f" Fixed result: {result!r} ({len(result.encode('utf-8'))} bytes)")
print()
EOFRepository: qhkm/zeptoclaw
Length of output: 355
Prevent UTF-8 panic when truncating scanned text.
Line 126 slices a &str by raw byte length without validating UTF-8 boundaries. If max_output_length lands inside a multibyte character, this will panic at runtime. Tool outputs regularly contain non-ASCII text (JSON, HTML, web content), making this a real risk.
🔧 Proposed fix
+fn truncate_utf8(s: &str, max_bytes: usize) -> &str {
+ if s.len() <= max_bytes {
+ return s;
+ }
+ let mut end = max_bytes;
+ while end > 0 && !s.is_char_boundary(end) {
+ end -= 1;
+ }
+ &s[..end]
+}
+
pub fn scan(&self, text: &str, _direction: CheckDirection) -> SafetyResult {
@@
- &text[..self.config.max_output_length]
+ truncate_utf8(text, self.config.max_output_length)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let content = if text.len() > self.config.max_output_length { | |
| was_modified = true; | |
| warnings.push(format!( | |
| "Output truncated from {} to {} bytes", | |
| input.len(), | |
| text.len(), | |
| self.config.max_output_length, | |
| )); | |
| &input[..self.config.max_output_length] | |
| &text[..self.config.max_output_length] | |
| } else { | |
| fn truncate_utf8(s: &str, max_bytes: usize) -> &str { | |
| if s.len() <= max_bytes { | |
| return s; | |
| } | |
| let mut end = max_bytes; | |
| while end > 0 && !s.is_char_boundary(end) { | |
| end -= 1; | |
| } | |
| &s[..end] | |
| } | |
| let content = if text.len() > self.config.max_output_length { | |
| was_modified = true; | |
| warnings.push(format!( | |
| "Output truncated from {} to {} bytes", | |
| text.len(), | |
| self.config.max_output_length, | |
| )); | |
| truncate_utf8(text, self.config.max_output_length) | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/safety/mod.rs` around lines 119 - 127, The current truncation takes a
byte slice &text[..self.config.max_output_length] which can panic if
max_output_length splits a UTF-8 codepoint; change it to find the nearest valid
UTF-8 boundary before slicing. In the block referencing text and
self.config.max_output_length, compute a safe_cut starting at max_output_length
and move it down until text.is_char_boundary(safe_cut) (handle the zero case)
and then slice &text[..safe_cut]; keep the existing was_modified/warnings logic
but ensure the warning still reports the original byte lengths if desired.
…n path kernel::execute_tool() returns Ok(ToolOutput::error(...)) for taint blocks, safety blocks, and tool-not-found. The non-streaming path treated all Ok returns as success, logging "Tool executed successfully" and calling hooks.after_tool() for blocked tools. Now branches on output.is_error, matching the streaming path pattern. Also fixes scan() doc comment to reflect Input+Output directions and replaces spawn_error variable with retry counter to avoid clippy warning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/safety/mod.rs (1)
124-132:⚠️ Potential issue | 🔴 CriticalCritical: UTF-8 boundary panic risk in truncation path.
Line 131 slices
&strby raw byte count. Ifmax_output_lengthlands inside a multibyte character, this can panic at runtime.Proposed fix
+fn truncate_utf8_prefix(s: &str, max_bytes: usize) -> &str { + if s.len() <= max_bytes { + return s; + } + let mut end = max_bytes; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + &s[..end] +} + // 1. Length check / truncation let content = if text.len() > self.config.max_output_length { was_modified = true; warnings.push(format!( "Output truncated from {} to {} bytes", text.len(), self.config.max_output_length, )); - &text[..self.config.max_output_length] + truncate_utf8_prefix(text, self.config.max_output_length) } else { text };#!/bin/bash # Verify unsafe truncation is present and UTF-8 boundary guard is absent/present. rg -n '^\s*&text\[\.\.self\.config\.max_output_length\]' src/safety/mod.rs -C 2 rg -n 'is_char_boundary|truncate_utf8_prefix' src/safety/mod.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/safety/mod.rs` around lines 124 - 132, The truncation currently takes a raw byte slice &text[..self.config.max_output_length] which can panic if max_output_length falls inside a UTF-8 multibyte character; update the truncation in the block that sets content/was_modified/warnings to compute a safe byte index by checking text.is_char_boundary(end) (where end = self.config.max_output_length.min(text.len())) and, if not a boundary, move to the previous valid boundary (e.g., use text.char_indices() to find the last index < end) before slicing; ensure warnings still report original/trimmed sizes and use the safe slice for content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/safety/mod.rs`:
- Around line 113-116: The warning message for truncated content is static and
always says "Output truncated..." even though the scan function accepts a
direction parameter; update the truncation warning in the scan implementation to
be direction-aware by using the existing direction parameter (e.g., match on
direction or call direction.as_str()) to produce "Input truncated..." when
direction indicates input and "Output truncated..." when it indicates output,
and replace both occurrences of the static string in the scan function (and any
helper it calls) so the label matches the actual direction.
---
Duplicate comments:
In `@src/safety/mod.rs`:
- Around line 124-132: The truncation currently takes a raw byte slice
&text[..self.config.max_output_length] which can panic if max_output_length
falls inside a UTF-8 multibyte character; update the truncation in the block
that sets content/was_modified/warnings to compute a safe byte index by checking
text.is_char_boundary(end) (where end =
self.config.max_output_length.min(text.len())) and, if not a boundary, move to
the previous valid boundary (e.g., use text.char_indices() to find the last
index < end) before slicing; ensure warnings still report original/trimmed sizes
and use the safe slice for content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29293db4-1325-465e-b8f8-378951089242
📒 Files selected for processing (3)
src/agent/loop.rssrc/safety/mod.rssrc/tools/binary_plugin.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tools/binary_plugin.rs
- src/agent/loop.rs
Comment on lines
+113
to
+116
| /// The `direction` parameter indicates whether we are scanning tool input | ||
| /// (before execution) or tool output (after execution). Currently both | ||
| /// directions run the same pipeline; the parameter is reserved for future | ||
| /// differentiation. |
There was a problem hiding this comment.
Use direction-aware truncation warning text.
scan now supports input/output directions, but the warning string still always says "Output truncated...". This will mislabel input scans.
Proposed fix
-pub fn scan(&self, text: &str, _direction: CheckDirection) -> SafetyResult {
+pub fn scan(&self, text: &str, direction: CheckDirection) -> SafetyResult {
let mut warnings: Vec<String> = Vec::new();
let mut was_modified = false;
@@
+ let direction_label = match direction {
+ CheckDirection::Input => "Input",
+ CheckDirection::Output => "Output",
+ };
let content = if text.len() > self.config.max_output_length {
was_modified = true;
warnings.push(format!(
- "Output truncated from {} to {} bytes",
+ "{} truncated from {} to {} bytes",
+ direction_label,
text.len(),
self.config.max_output_length,
));Also applies to: 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/safety/mod.rs` around lines 113 - 116, The warning message for truncated
content is static and always says "Output truncated..." even though the scan
function accepts a direction parameter; update the truncation warning in the
scan implementation to be direction-aware by using the existing direction
parameter (e.g., match on direction or call direction.as_str()) to produce
"Input truncated..." when direction indicates input and "Output truncated..."
when it indicates output, and replace both occurrences of the static string in
the scan function (and any helper it calls) so the label matches the actual
direction.
…s dedup test_soft_error_paths_set_is_error_true: verifies tool-not-found, taint-blocked, and safety-blocked all return Ok with is_error=true. The agent loop branches on this flag for hooks/feedback/panel events; a regression would cause blocked tools to report as successful. test_metrics_recorded_exactly_once: verifies execute_tool records metrics exactly once per call, preventing the double-counting that existed before kernel convergence. Part of #249 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernel/gate.rs`:
- Around line 372-379: Replace the conditional check with a deterministic
assertion so the test always verifies the safety-block path: change the if +
assert into a single assert that both result.for_llm.contains("blocked by
safety") and result.is_error are true (e.g.,
assert!(result.for_llm.contains("blocked by safety") && result.is_error,
"...")), referencing the existing result.for_llm and result.is_error variables
in gate.rs so the regression fails if the safety-block message or the is_error
flag is not set.
Comment on lines
+372
to
+379
| // Safety may block or warn depending on pattern match confidence. | ||
| // If blocked, is_error must be true. | ||
| if result.for_llm.contains("blocked by safety") { | ||
| assert!( | ||
| result.is_error, | ||
| "safety-blocked must set is_error=true; agent loop branches on this" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Make the safety-block regression assertion deterministic.
Line 374 makes the safety-block assertion conditional, so this test can pass without exercising the safety-block soft-error path. That weakens the regression guarantee promised by this test.
Suggested fix
- // Safety may block or warn depending on pattern match confidence.
- // If blocked, is_error must be true.
- if result.for_llm.contains("blocked by safety") {
- assert!(
- result.is_error,
- "safety-blocked must set is_error=true; agent loop branches on this"
- );
- }
+ assert!(
+ result.for_llm.contains("blocked by safety"),
+ "expected safety input to be blocked to exercise soft-error safety path"
+ );
+ assert!(
+ result.is_error,
+ "safety-blocked must set is_error=true; agent loop branches on this"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Safety may block or warn depending on pattern match confidence. | |
| // If blocked, is_error must be true. | |
| if result.for_llm.contains("blocked by safety") { | |
| assert!( | |
| result.is_error, | |
| "safety-blocked must set is_error=true; agent loop branches on this" | |
| ); | |
| } | |
| assert!( | |
| result.for_llm.contains("blocked by safety"), | |
| "expected safety input to be blocked to exercise soft-error safety path" | |
| ); | |
| assert!( | |
| result.is_error, | |
| "safety-blocked must set is_error=true; agent loop branches on this" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/kernel/gate.rs` around lines 372 - 379, Replace the conditional check
with a deterministic assertion so the test always verifies the safety-block
path: change the if + assert into a single assert that both
result.for_llm.contains("blocked by safety") and result.is_error are true (e.g.,
assert!(result.for_llm.contains("blocked by safety") && result.is_error,
"...")), referencing the existing result.for_llm and result.is_error variables
in gate.rs so the regression fails if the safety-block message or the is_error
flag is not set.
This was referenced Mar 7, 2026
This was referenced Mar 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:\n- rename safety API check_tool_output to scan with CheckDirection\n- route agent-loop tool execution through kernel::execute_tool for shared safety, taint, and metrics behavior\n- add zeptoclaw provider status CLI for provider chain introspection\n- update CLAUDE.md and AGENTS.md snapshots\n\nValidation:\n- cargo fmt -- --check\n- cargo clippy -- -D warnings\n- cargo test --lib -- safety:: kernel::gate agent::r#loop\n- cargo test --bin zeptoclaw cli::provider\n\nIssues:\nCloses #251\nCloses #249\nCloses #250
Summary by CodeRabbit
New Features
zeptoclaw provider statusCLI to inspect providers, wrappers, and quotas.Bug Fixes / Reliability
Documentation