feat: Code Agent Specialist#182
Merged
Merged
Conversation
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 code-session support (types, parsing, CLI, config, delegated session execution), audit logging for code sessions, a new optional Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Agent as Agent
participant DelegateTool as DelegateTool
participant SubAgent as SubAgent
participant CodeSessionResult as CodeSessionResult
participant Observer as Observer
participant AuditLogger as AuditLogger
User->>Agent: request code task / CLI Code
Agent->>DelegateTool: execute(prompt, delegate config)
alt DelegateExecutionMode::Session
DelegateTool->>DelegateTool: run_session (spawn child with derived config)
DelegateTool->>SubAgent: start sub-agent (delegated = true)
SubAgent->>SubAgent: run turns, invoke tools, produce output (FINAL RESULT)
SubAgent-->>DelegateTool: final output
DelegateTool->>CodeSessionResult: parse_from_output(output, session_id)
CodeSessionResult-->>DelegateTool: structured result
DelegateTool-->>Agent: ToolResult{ structured: CodeSessionResult.to_structured() }
else OneShot
DelegateTool->>SubAgent: delegate one-shot
SubAgent-->>DelegateTool: result
DelegateTool-->>Agent: ToolResult
end
Agent->>Observer: emit ObserverEvent::CodeSessionCompleted(...)
Observer->>AuditLogger: log_code_session_event(...)
AuditLogger-->>Observer: ack
Observer-->>Agent: recorded
Agent-->>User: return completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-10 to 2026-03-10 |
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
clients/agent-runtime/src/tools/hardware_memory_map.rs (1)
84-93:⚠️ Potential issue | 🟠 MajorPopulate
structuredon both return paths.Leaving
structured: Nonehere means this tool still only exposes formatted text, so downstream callers cannot consume the memory map or error details through the new machine-readableToolResultcontract. Return JSON on both paths, e.g.{ code, message }for configuration errors and{ board, source, regions }for successful lookups, while keepingoutputas the human-readable rendering.Proposed direction
if self.boards.is_empty() { return Ok(ToolResult { success: false, output: String::new(), error: Some( "No peripherals configured. Add boards to config.toml [peripherals.boards]." .into(), ), - structured: None, + structured: Some(json!({ + "code": "no_boards_configured", + "message": "No peripherals configured. Add boards to config.toml [peripherals.boards]." + })), }); } @@ + let structured = json!({ + "board": board, + "source": if probe_ok { "probe-rs" } else { "datasheet" }, + "regions": [] + }); + Ok(ToolResult { success: true, output, error: None, - structured: None, + structured: Some(structured), })Based on learnings: "Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path".Also applies to: 141-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/hardware_memory_map.rs` around lines 84 - 93, The early-return branch that checks self.boards.is_empty() currently sets ToolResult.structured = None; update that branch (the method containing the self.boards.is_empty() check) to populate structured with a JSON-serializable object such as { "code": "no_peripherals", "message": "No peripherals configured. Add boards to config.toml [peripherals.boards]." }, keeping output as the human-readable string; likewise update the successful-return branch later in this file (the other ToolResult return around lines 141-145) to set structured to a JSON object like { "board": <board_id>, "source": <source>, "regions": <regions_array> } so downstream callers can consume machine-readable data while preserving the human-readable output. Ensure you use the ToolResult struct's structured field and produce serializable types (e.g., serde_json::Value or a small serde-annotated struct) rather than leaving it None.Makefile (1)
15-22: 🧹 Nitpick | 🔵 TrivialWindows bash assumption has a bootstrap dependency.
The Makefile assumes
bashis in PATH on Windows (line 18), with validation deferred tocheck-tools.sh. However,check-tools.shitself requires bash to execute. If bash is missing, the error message will be cryptic (command not found).Consider adding a comment or fallback error for Windows users without bash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 15 - 22, The Makefile assumes bash exists on Windows by setting SHELL when DETECTED_OS is Windows but gives a cryptic failure if bash is missing because check-tools.sh requires bash; add an explicit bootstrap check: when DETECTED_OS is Windows_NT, test for bash availability (e.g., via command -v/which) and if not present emit a clear make error (using $(error ...)) explaining that bash is required to run check-tools.sh and how to install it, and also add a short inline comment above the SHELL assignment documenting this bootstrap dependency and why bash is required.clients/agent-runtime/src/tools/memory_forget.rs (1)
31-48:⚠️ Potential issue | 🟡 MinorReject blank memory keys.
"key": ""and whitespace-only values pass both the schema and the runtime check today. Please trim and require a non-empty key here, and mirror that in the schema withminLength: 1.Suggested fix
fn parameters_schema(&self) -> serde_json::Value { json!({ "type": "object", "properties": { "key": { "type": "string", + "minLength": 1, "description": "The key of the memory to forget" } }, "required": ["key"] }) } async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> { let key = args .get("key") .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("Missing 'key' parameter"))?; + .map(str::trim) + .filter(|key| !key.is_empty()) + .ok_or_else(|| anyhow::anyhow!("Missing or empty 'key' parameter"))?;Based on learnings: "Applies to clients/agent-runtime/src/tools/**/*.rs : Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/memory_forget.rs` around lines 31 - 48, The parameters_schema() currently allows empty or whitespace-only "key" values; update parameters_schema to include "minLength": 1 for the "key" property, and in execute(&self, args: serde_json::Value) trim the extracted key string (call .trim()) and reject empty strings by returning an anyhow error (e.g., Missing or empty 'key' parameter) before proceeding; reference the functions parameters_schema and execute in memory_forget.rs and ensure the runtime validation mirrors the schema so whitespace-only values are rejected and a structured ToolResult error is returned rather than panicking.clients/agent-runtime/src/peripherals/arduino_upload.rs (1)
63-149:⚠️ Potential issue | 🟠 MajorDo not run
arduino-clisynchronously on the async runtime.
Command::output()blocks insideasync fn execute, and the compile/upload calls have no timeout. A hungarduino-cliinvocation can pin a Tokio worker indefinitely and stall the agent. Move these calls totokio::process::Commandorspawn_blocking, and wrap compile/upload in explicit timeouts.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/peripherals/arduino_upload.rs` around lines 63 - 149, The code calls blocking std::process::Command::output() (e.g., the initial version check and the compile/upload invocations captured in variables compile and upload inside async fn execute) which can block the Tokio runtime; replace these with non-blocking alternatives and add timeouts: either use tokio::process::Command for the version, compile and upload calls (await their .output().await) or run the blocking calls inside tokio::task::spawn_blocking, and wrap each compile/upload await in tokio::time::timeout with a sensible duration; ensure errors from timed-out or spawn_blocking failures are mapped into the existing ToolResult error/output paths (including cleaning up temp_dir and preserving self.port in the upload call).clients/agent-runtime/src/peripherals/serial.rs (1)
86-98: 🧹 Nitpick | 🔵 TrivialPreserve non-string serial replies in
ToolResult.structured.This path already has parsed JSON in
resp["result"], but it stringifies object/array responses and hard-codesstructured: None. That drops the type information the new field was added for, especially for capability-style responses.Suggested refactor
- let ok = resp["ok"].as_bool().unwrap_or(false); - let result = resp["result"] - .as_str() - .map(String::from) - .unwrap_or_else(|| resp["result"].to_string()); + let ok = resp["ok"].as_bool().unwrap_or(false); + let raw_result = resp.get("result").cloned().unwrap_or(Value::Null); + let result = raw_result + .as_str() + .map(String::from) + .unwrap_or_else(|| raw_result.to_string()); let error = resp["error"].as_str().map(String::from); + let structured = match raw_result { + Value::Null | Value::String(_) => None, + other => Some(other), + }; Ok(ToolResult { success: ok, output: result, error, - structured: None, + structured, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/peripherals/serial.rs` around lines 86 - 98, The current code converts all resp["result"] values to a string and always sets ToolResult.structured to None, losing type info for non-string JSON results; change it so that if resp["result"] is a JSON string you keep output as that string and structured = None, otherwise keep output = resp["result"].to_string() but set structured = Some(resp["result"].clone()) (i.e., compute let structured = if resp["result"].is_string() { None } else { Some(resp["result"].clone()) } and use that when constructing ToolResult, referencing resp, result, error, and ToolResult).clients/agent-runtime/src/tools/pushover.rs (1)
53-84: 🧹 Nitpick | 🔵 TrivialBlocking file I/O in async context.
std::fs::read_to_stringon line 55 blocks the async runtime. Consider usingtokio::fs::read_to_stringfor consistency with other tools and to avoid blocking the executor.♻️ Suggested fix
- fn get_credentials(&self) -> anyhow::Result<(String, String)> { + async fn get_credentials(&self) -> anyhow::Result<(String, String)> { let env_path = self.workspace_dir.join(".env"); - let content = std::fs::read_to_string(&env_path) + let content = tokio::fs::read_to_string(&env_path).await .map_err(|e| anyhow::anyhow!("Failed to read {}: {}", env_path.display(), e))?;Then update the call site at line 169:
- let (token, user_key) = self.get_credentials()?; + let (token, user_key) = self.get_credentials().await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/pushover.rs` around lines 53 - 84, The get_credentials function is performing blocking file I/O with std::fs::read_to_string; change it to async by making get_credentials async (async fn get_credentials(&self) -> anyhow::Result<(String, String)>) and replace std::fs::read_to_string(&env_path) with tokio::fs::read_to_string(&env_path).await (preserving the current error mapping). Keep the rest of the parsing logic (lines referencing workspace_dir, parse_env_value, PUSHOVER_TOKEN / PUSHOVER_USER_KEY) unchanged and update all call sites (e.g., the call at the previously noted site) to .await the async get_credentials call. Ensure the function signature and callers use the appropriate async context and propagate anyhow::Result as before.clients/agent-runtime/src/tools/hardware_board_info.rs (1)
90-126:⚠️ Potential issue | 🟠 MajorReject
boardvalues that are not explicitly configured.Right now any caller can pass an arbitrary board name, and the tool will return static info for it; with
probeenabled, it can also trigger probing for an unconfigured target. Validate the requested board againstself.boardsbefore continuing.Suggested change
let board = board.as_deref().unwrap_or("unknown"); if self.boards.is_empty() { return Ok(ToolResult { success: false, @@ structured: None, }); } + + if !self.boards.iter().any(|configured| configured == board) { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some(format!("Board '{board}' is not configured")), + structured: None, + }); + } let mut output = String::new();As per coding guidelines: "Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/hardware_board_info.rs` around lines 90 - 126, The code currently accepts any caller-supplied board string and proceeds to use it (variable board) and potentially call probe_board_info on untrusted values; validate that the requested board is one of the configured entries in self.boards before proceeding: after deriving board (board/as_deref/unwrap_or), check membership (e.g., if !self.boards.contains(board) ) and return a ToolResult error indicating the board is unknown/unconfigured; only allow the probe_board_info call and any board-specific logic for values present in self.boards to prevent probing or returning info for arbitrary inputs.clients/agent-runtime/src/tools/browser.rs (1)
749-771:⚠️ Potential issue | 🔴 CriticalDo not forward unchecked params to the computer-use sidecar.
This path forwards every key except
actionto the sidecar. Local validation only looks at a few action-specific fields, so callers can smuggle extra sidecar parameters that bypass Corvus policy. Buildparamsfrom a per-action allowlist instead of cloning the whole input object.As per coding guidelines: "Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/browser.rs` around lines 749 - 771, The code currently clones the entire args object and strips only "action", which lets callers forward unchecked keys to the computer-use sidecar; instead, change the construction of params in the browser invocation (in browser.rs where validate_computer_use_action is called) to build a new serde_json::Map by selecting only a per-action allowlist of keys (e.g., define an allowlist match on action -> &[&str] or a helper function like allowed_params_for_action(action) and then copy only those keys from args.as_object()). Keep calling self.validate_computer_use_action(action, ¶ms) but ensure params contains only allowed keys before creating the payload so no extra filesystem/network parameters are forwarded to the sidecar.clients/agent-runtime/src/tools/schedule.rs (1)
203-217: 🛠️ Refactor suggestion | 🟠 MajorPreserve the already-built JSON in
structured.
handle_getconstructsdetailas JSON, then pretty-prints it intooutput, but Line 217 still drops the machine-readable payload withstructured: None. That forces downstream callers to re-parse text and defeats the newToolResultsurface.Suggested change
Ok(ToolResult { success: true, output: serde_json::to_string_pretty(&detail)?, error: None, - structured: None, + structured: Some(detail), })Based on learnings: "Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/schedule.rs` around lines 203 - 217, The constructed JSON `detail` in handle_get is being pretty-printed into `output` but not returned in `structured`; change the ToolResult construction to return the machine-readable payload by setting `structured: Some(detail)` (or Some(detail.clone()) if needed for ownership) instead of `None`, keeping `output: serde_json::to_string_pretty(&detail)?` as-is so callers get both human and structured representations; update the ToolResult fields accordingly in the handle_get function.clients/agent-runtime/src/tools/git_operations.rs (1)
497-518:⚠️ Potential issue | 🔴 CriticalReject checkout targets that start with
-.
sanitize_git_argsdoes not block option-like branch names, so inputs like--orphanor-reachgit checkoutas flags/special shorthand instead of a literal branch. Treat leading-dash values as invalid before invoking git.Suggested change
- if branch_name.contains('@') || branch_name.contains('^') || branch_name.contains('~') { + if branch_name.starts_with('-') + || branch_name.contains('@') + || branch_name.contains('^') + || branch_name.contains('~') + { anyhow::bail!("Branch name contains invalid characters"); }Based on learnings: "Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/git_operations.rs` around lines 497 - 518, The sanitized branch token can still start with '-' (e.g., "--orphan" or "-") and will be treated as a git option; after calling sanitize_git_args and extracting branch_name, add a validation that rejects any branch_name that starts_with('-') (or equals "-") and return an error via anyhow::bail! with a clear message; update the checkout path that calls run_git_command(&["checkout", branch_name]) to only run when this new check passes so ToolResult is never produced for option-like inputs (refer to sanitize_git_args, branch_name, run_git_command and the ToolResult return).clients/agent-runtime/src/tools/http_request.rs (1)
345-366:⚠️ Potential issue | 🟠 MajorEnforce the response-size limit while reading, not after buffering.
Line 345 loads the full body into memory with
response.text().await, somax_response_sizeonly affects display. A large response can still blow past memory and latency limits beforetruncate_responseruns. Stream the body and stop once the byte budget is reached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/http_request.rs` around lines 345 - 366, The code currently calls response.text().await (in the block around ToolResult construction) which buffers the entire body before truncating; instead stream the response (e.g., response.bytes_stream()) and read chunks into a byte buffer until the configured max_response_size is reached, stop requesting more data, and mark the body as truncated; convert the collected bytes to a String (using UTF-8 fallback like from_utf8_lossy) and pass that to truncate_response or skip truncate_response if you already enforced the limit; replace the response.text().await branch with this streaming logic, handle chunk read errors similarly to the current Err(e) case, and ensure the output, response_text variable, and any truncated marker are set accordingly when building the ToolResult.clients/agent-runtime/src/tools/mcp/adapter.rs (1)
53-70:⚠️ Potential issue | 🟡 MinorPotential panic on line 69 if
max_bodyisn't a UTF-8 char boundary.The fallback
output[..max_body].to_string()uses byte indexing. Ifmax_bodylands in the middle of a multi-byte character, this will panic at runtime.The
scan()logic above handles this correctly, but theunwrap_or_elsefallback can be reached when the first character's byte length exceedsmax_body. In that case, slicing atmax_bodywill panic unless it's 0.Proposed fix
.last() .map(|end| output[..end].to_string()) - .unwrap_or_else(|| output[..max_body].to_string()) + .unwrap_or_default() };If no characters fit within
max_body, returning an empty string before appending the marker is safer than risking a panic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/mcp/adapter.rs` around lines 53 - 70, The fallback in the `truncated` computation can panic because `unwrap_or_else(|| output[..max_body].to_string())` slices at a byte index that may split a UTF-8 char; change the fallback to return an empty string instead of slicing (i.e., replace that `unwrap_or_else` branch so it yields `""` when no valid char boundary fits within `max_body`), keeping the existing `scan` logic and using the same `output`, `max_body`, and `truncated` variables.clients/agent-runtime/src/peripherals/rpi.rs (1)
76-86:⚠️ Potential issue | 🟠 MajorValidate GPIO inputs before coercing them.
pin as u8silently wraps oversized inputs, andgpio_writecurrently treats any non-zerovalueasHigh. On a real board that can toggle the wrong pin/state instead of rejecting bad input. Add schema bounds and fail fast inexecute()for out-of-range pins and values other than0/1.🔧 Suggested fix
fn parameters_schema(&self) -> Value { json!({ "type": "object", "properties": { "pin": { "type": "integer", - "description": "BCM GPIO pin number (e.g. 17, 27)" + "description": "BCM GPIO pin number (e.g. 17, 27)", + "minimum": 0, + "maximum": 255 } }, "required": ["pin"] }) } @@ - let pin_u8 = pin as u8; + let pin_u8 = u8::try_from(pin).map_err(|_| anyhow::anyhow!("'pin' must fit in u8"))?; @@ fn parameters_schema(&self) -> Value { json!({ "type": "object", "properties": { "pin": { "type": "integer", - "description": "BCM GPIO pin number" + "description": "BCM GPIO pin number", + "minimum": 0, + "maximum": 255 }, "value": { "type": "integer", - "description": "0 for low, 1 for high" + "description": "0 for low, 1 for high", + "enum": [0, 1] } }, "required": ["pin", "value"] }) } @@ - let pin_u8 = pin as u8; - let level = match value { - 0 => rppal::gpio::Level::Low, - _ => rppal::gpio::Level::High, - }; + let pin_u8 = u8::try_from(pin).map_err(|_| anyhow::anyhow!("'pin' must fit in u8"))?; + let level = match value { + 0 => rppal::gpio::Level::Low, + 1 => rppal::gpio::Level::High, + _ => anyhow::bail!("'value' must be 0 or 1"), + };As per coding guidelines:
**/*: Security first, performance second. Validate input boundaries, auth/authz implications, and secret management.Also applies to: 89-95, 128-142, 145-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/peripherals/rpi.rs` around lines 76 - 86, The parameters_schema currently lacks bounds and execute silently coerces invalid GPIO inputs (using pin as u8 and treating any non-zero value as High); update parameters_schema to set valid integer bounds for "pin" (e.g. min 0, max 27 or board-specific max) and constrain "value" to enum [0,1] where applicable, and in the execute() implementations (the GPIO write/read handlers that call gpio_write/gpio_read) perform strict validation: reject out-of-range pin numbers before casting (do an explicit range check and only then cast to u8) and reject any value other than exactly 0 or 1 with an error result (do not treat non-zero as High); apply the same validation pattern to the other GPIO handler blocks that call gpio_write/gpio_read so invalid inputs fail fast.
🤖 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/src/agent/agent.rs`:
- Around line 758-760: The function code_session_id falls back to a generated
UUID when the CORVUS_SESSION_ID env var is absent but this behavior is
undocumented; add a brief doc comment on the code_session_id function (and
update any relevant README or environment/ops docs) explaining that
CORVUS_SESSION_ID, if set, is used to correlate sessions across orchestrators
and that a UUID is generated otherwise, so callers and operators know how to
provide/override session IDs.
- Around line 845-858: The code currently swallows audit logging failures by
only emitting a tracing::warn inside the block that calls
self.audit_logger.log_code_session_event(CodeSessionAuditLog { ... }); change
this to support a configurable strict mode: add or use a boolean flag on the
agent (e.g., self.strict_audit or self.fail_on_audit_error) and, inside the
audit_logger block, if logger.log_code_session_event(...) returns Err then
either (a) propagate/return the error (convert to the function's error type)
when strict mode is enabled, or (b) keep the tracing::warn when strict mode is
disabled; locate the code around audit_logger, log_code_session_event,
CodeSessionAuditLog and replace the unconditional tracing::warn with a
conditional that enforces strict behavior and returns the error in strict mode.
- Around line 357-374: The function audit_logger_from_config currently uses
AuditConfig::default() instead of the audit settings provided in Config; update
it to read the audit configuration from the passed Config (e.g., use
config.audit or the actual field name that holds audit settings) and pass that
AuditConfig into AuditLogger::new, preserving the corvus_dir logic and existing
error handling in audit_logger_from_config; ensure you remove the hardcoded
AuditConfig::default() and reference the Config's audit field when constructing
the logger.
In `@clients/agent-runtime/src/agent/code_session.rs`:
- Around line 221-228: The status parser in the parse_from_output block using
parse_field currently maps "completed", "completed_with_warnings", and "blocked"
but omits the "success" and "failed" strings declared in the prompt; update the
match arm inside the parse_field(...).map(|s| match s.trim() { ... }) to also
handle "success" (map to CodeSessionStatus::Completed) and "failed" (map to
CodeSessionStatus::Failed), keeping the existing mappings for
"completed_with_warnings" -> CodeSessionStatus::CompletedWithWarnings and
"blocked" -> CodeSessionStatus::Blocked and leaving the fallback as
CodeSessionStatus::Failed.
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 137-145: Ensure deserialization/validation fails on invalid
delegate override values by adding checks in validate_for_runtime() (or
deserialization hooks) for the Delegate config: require max_iterations when Some
to be >0 (reject 0), require timeout_ms when Some to be >0 (reject 0), and
reject empty validation command lists/empty strings used for code-session
validation; also validate DelegateExecutionMode if there are unsupported modes.
Update validation logic references around the fields execution_mode,
max_iterations, timeout_ms and any code-session validation command fields so
invalid configs return an error at startup instead of being accepted.
In `@clients/agent-runtime/src/observability/otel.rs`:
- Around line 274-296: The span created in ObserverEvent::CodeSessionCompleted
sets attributes but never sets a Status, so update the handling in the
CodeSessionCompleted arm to set the span status based on the local `status`
variable (e.g., map success/ok to Status::Ok and failures to Status::Error with
a message). After building the span with tracer.build(...) and before calling
span.end(), call the span status API (the opentelemetry span status setter used
elsewhere in this file, matching the pattern from ToolCall or LlmResponse) to
set an appropriate StatusCode/Status message derived from `status`, then end the
span.
In `@clients/agent-runtime/src/security/audit.rs`:
- Around line 189-201: The CodeSessionAuditLog struct is only derived Debug and
Clone but should implement serde serialization for structured logging; add
#[derive(Serialize)] to CodeSessionAuditLog and import serde::Serialize (or use
serde's prelude) so the struct can be serialized consistently like
CodeSessionAudit, ensuring any fields (e.g., session_id, status, summary,
changed_files, commands, validations, blockers, pending_work, delegated) are
serializable types.
In `@clients/agent-runtime/src/tools/delegate.rs`:
- Around line 158-165: The timeout conversion currently does integer division
(ms / 1000) causing values like 500ms to become 0s; update the logic in
delegate.rs around agent_config.timeout_ms /
config.agent.code_session.timeout_ms / DELEGATE_TIMEOUT_SECS so you compute a
proper Duration from milliseconds (e.g. use Duration::from_millis for the chosen
ms value or perform ceiling division ((ms + 999) / 1000) and ensure a minimum
nonzero timeout) and then pass that Duration into tokio::time::timeout when
calling agent.turn(full_prompt); keep references to agent_config.timeout_ms,
config.agent.code_session.timeout_ms, DELEGATE_TIMEOUT_SECS and
agent.turn(full_prompt) to locate and update the code.
In `@clients/web/apps/docs/src/styles/custom.css`:
- Around line 388-398: Wrap the global transition rules for the selectors a,
button, .card, input, and summary inside a prefers-reduced-motion media query so
they only apply when the user has not requested reduced motion; update the
comment to reflect this change and keep the rule opt-in for third-party
components (i.e., move the existing
transition-property/transition-duration/transition-timing-function block into
`@media` (prefers-reduced-motion: no-preference) and ensure no unconditional
global transitions remain).
In `@clients/web/apps/marketing/package.json`:
- Around line 8-13: The npm script "copy-install" blindly runs cp on
../../../../scripts/install.sh which will fail cryptically if the file is
missing; change the "copy-install" script in package.json (the "copy-install"
entry) to first test for the file's existence (e.g., check -f or test -e on
../../../../scripts/install.sh) and either emit a clear error and exit non‑zero
or skip the copy with an informative message, then run cp only if the test
passes so the failure mode is explicit and developer-friendly.
In `@clients/web/apps/marketing/public/install`:
- Around line 44-64: The detect_existing_install function uses RUN_ONBOARD with
inverted semantics (RUN_ONBOARD="1" means skip), which is confusing; rename
RUN_ONBOARD to SKIP_ONBOARD (and CORVUS_NO_ONBOARD to match if needed) and
update all references and initialization (where RUN_ONBOARD is set from
CORVUS_NO_ONBOARD) as well as the checks in detect_existing_install (checks
against FORCE_ONBOARD remain) so that setting SKIP_ONBOARD="1" clearly means
skip onboarding; alternatively, if you prefer to keep the RUN_ONBOARD name,
invert its usage everywhere in detect_existing_install and its initialization so
RUN_ONBOARD="1" actually means run onboarding, and update any other code that
reads RUN_ONBOARD or CORVUS_NO_ONBOARD (e.g., FORCE_ONBOARD, the initial
assignment at line 21) to the new variable name/semantics to keep behavior
consistent.
In `@clients/web/apps/marketing/src/styles/global.css`:
- Around line 847-849: The reduced-motion media rule using the universal
selector isn’t overriding higher-specificity class rules (e.g., .btn,
.terminal-card, .copy-button.is-copied) because it lacks !important; update the
`@media` (prefers-reduced-motion: reduce) block so the properties
transition-duration, animation-duration and animation-iteration-count are
suffixed with !important (e.g., transition-duration: 0.01ms !important;) so the
reduced-motion overrides reliably suppress motion for those class-based
selectors.
In `@Makefile`:
- Around line 98-104: The Makefile target 'doctor' (alias 'check-tools')
currently has a body longer than 5 lines which trips Checkmake; move the
diagnostic logic into a separate executable script (e.g., scripts/doctor.sh)
containing the multi-line checks and make the Makefile 'doctor' target a short
wrapper that simply invokes that script (or collapse the multiple commands into
a single-line shell invocation if you prefer not to add a file). Update the
Makefile to call the new script from the 'doctor' target (referencing the
'doctor'/'check-tools' target names) so the target body is <=5 lines and all
diagnostic output remains unchanged.
In `@openspec/changes/code-agent-specialist/tasks.md`:
- Line 30: The checklist item "4.4 Execute Rust validation gates for
`clients/agent-runtime/**/*.rs`" incorrectly marks the gates complete without
recording which commands ran or were skipped; update the tasks.md entry for that
4.4 item to leave the box unchecked until you append the actual executed
commands and their outcomes (e.g., `cargo fmt --all -- --check`, `cargo clippy
--all-targets -- -D warnings`, `cargo test` and any focused test runs for
`clients/agent-runtime/src/config/schema.rs`, `.../agent/prompt.rs`,
`.../agent/code_session.rs`, `.../tools/delegate.rs`, `.../security/policy.rs`,
`.../approval/mod.rs`), or explicitly list any skipped checks with the reason
why, then only check the box after those results are recorded.
In `@scripts/install.sh`:
- Around line 692-701: The nested trap that re-establishes a parent trap via the
string variable parent_return_trap (registered on RETURN around the staged_path
cleanup) is fragile; replace this pattern by introducing a single global cleanup
function (e.g., cleanup_staged_path) that explicitly tracks temp files via
staged_path and performs removal, and register that one cleanup function with
trap RETURN once at script initialization instead of re-setting traps via
parent_return_trap strings; remove the code paths that attempt to re-evaluate
parent_return_trap and ensure staged_path is only checked/removed inside the
single cleanup function so special characters or changed trap logic cannot break
restoration.
- Around line 21-22: The variable RUN_ONBOARD is misnamed because its value is
inverted (1 means "skip"), so rename it to SKIP_ONBOARD and the env var
CORVUS_NO_ONBOARD to CORVUS_SKIP_ONBOARD to make semantics clear; change the
assignment RUN_ONBOARD="${CORVUS_NO_ONBOARD:-0}" to
SKIP_ONBOARD="${CORVUS_SKIP_ONBOARD:-0}" (keep FORCE_ONBOARD as-is) and then
update every usage of RUN_ONBOARD to SKIP_ONBOARD and every reference to
CORVUS_NO_ONBOARD to CORVUS_SKIP_ONBOARD so conditional checks and logic (e.g.,
where the script skips onboarding) continue to work with the clarified name.
- Around line 746-749: The trap currently expands $download_path and
$extract_dir immediately (via the install_cleanup_trap string), which breaks if
paths contain spaces/special chars; replace the string-based trap with a cleanup
function (e.g., cleanup_install) that references the variables ($download_path,
$extract_dir) at execution time and then set trap 'cleanup_install' RETURN (or
EXIT) so values are handled safely, or if you must keep a string trap build an
escaped command using printf %q for each path before assigning
install_cleanup_trap to avoid premature expansion and word-splitting; update
references to install_cleanup_trap and trap accordingly.
---
Outside diff comments:
In `@clients/agent-runtime/src/peripherals/arduino_upload.rs`:
- Around line 63-149: The code calls blocking std::process::Command::output()
(e.g., the initial version check and the compile/upload invocations captured in
variables compile and upload inside async fn execute) which can block the Tokio
runtime; replace these with non-blocking alternatives and add timeouts: either
use tokio::process::Command for the version, compile and upload calls (await
their .output().await) or run the blocking calls inside
tokio::task::spawn_blocking, and wrap each compile/upload await in
tokio::time::timeout with a sensible duration; ensure errors from timed-out or
spawn_blocking failures are mapped into the existing ToolResult error/output
paths (including cleaning up temp_dir and preserving self.port in the upload
call).
In `@clients/agent-runtime/src/peripherals/rpi.rs`:
- Around line 76-86: The parameters_schema currently lacks bounds and execute
silently coerces invalid GPIO inputs (using pin as u8 and treating any non-zero
value as High); update parameters_schema to set valid integer bounds for "pin"
(e.g. min 0, max 27 or board-specific max) and constrain "value" to enum [0,1]
where applicable, and in the execute() implementations (the GPIO write/read
handlers that call gpio_write/gpio_read) perform strict validation: reject
out-of-range pin numbers before casting (do an explicit range check and only
then cast to u8) and reject any value other than exactly 0 or 1 with an error
result (do not treat non-zero as High); apply the same validation pattern to the
other GPIO handler blocks that call gpio_write/gpio_read so invalid inputs fail
fast.
In `@clients/agent-runtime/src/peripherals/serial.rs`:
- Around line 86-98: The current code converts all resp["result"] values to a
string and always sets ToolResult.structured to None, losing type info for
non-string JSON results; change it so that if resp["result"] is a JSON string
you keep output as that string and structured = None, otherwise keep output =
resp["result"].to_string() but set structured = Some(resp["result"].clone())
(i.e., compute let structured = if resp["result"].is_string() { None } else {
Some(resp["result"].clone()) } and use that when constructing ToolResult,
referencing resp, result, error, and ToolResult).
In `@clients/agent-runtime/src/tools/browser.rs`:
- Around line 749-771: The code currently clones the entire args object and
strips only "action", which lets callers forward unchecked keys to the
computer-use sidecar; instead, change the construction of params in the browser
invocation (in browser.rs where validate_computer_use_action is called) to build
a new serde_json::Map by selecting only a per-action allowlist of keys (e.g.,
define an allowlist match on action -> &[&str] or a helper function like
allowed_params_for_action(action) and then copy only those keys from
args.as_object()). Keep calling self.validate_computer_use_action(action,
¶ms) but ensure params contains only allowed keys before creating the
payload so no extra filesystem/network parameters are forwarded to the sidecar.
In `@clients/agent-runtime/src/tools/git_operations.rs`:
- Around line 497-518: The sanitized branch token can still start with '-'
(e.g., "--orphan" or "-") and will be treated as a git option; after calling
sanitize_git_args and extracting branch_name, add a validation that rejects any
branch_name that starts_with('-') (or equals "-") and return an error via
anyhow::bail! with a clear message; update the checkout path that calls
run_git_command(&["checkout", branch_name]) to only run when this new check
passes so ToolResult is never produced for option-like inputs (refer to
sanitize_git_args, branch_name, run_git_command and the ToolResult return).
In `@clients/agent-runtime/src/tools/hardware_board_info.rs`:
- Around line 90-126: The code currently accepts any caller-supplied board
string and proceeds to use it (variable board) and potentially call
probe_board_info on untrusted values; validate that the requested board is one
of the configured entries in self.boards before proceeding: after deriving board
(board/as_deref/unwrap_or), check membership (e.g., if
!self.boards.contains(board) ) and return a ToolResult error indicating the
board is unknown/unconfigured; only allow the probe_board_info call and any
board-specific logic for values present in self.boards to prevent probing or
returning info for arbitrary inputs.
In `@clients/agent-runtime/src/tools/hardware_memory_map.rs`:
- Around line 84-93: The early-return branch that checks self.boards.is_empty()
currently sets ToolResult.structured = None; update that branch (the method
containing the self.boards.is_empty() check) to populate structured with a
JSON-serializable object such as { "code": "no_peripherals", "message": "No
peripherals configured. Add boards to config.toml [peripherals.boards]." },
keeping output as the human-readable string; likewise update the
successful-return branch later in this file (the other ToolResult return around
lines 141-145) to set structured to a JSON object like { "board": <board_id>,
"source": <source>, "regions": <regions_array> } so downstream callers can
consume machine-readable data while preserving the human-readable output. Ensure
you use the ToolResult struct's structured field and produce serializable types
(e.g., serde_json::Value or a small serde-annotated struct) rather than leaving
it None.
In `@clients/agent-runtime/src/tools/http_request.rs`:
- Around line 345-366: The code currently calls response.text().await (in the
block around ToolResult construction) which buffers the entire body before
truncating; instead stream the response (e.g., response.bytes_stream()) and read
chunks into a byte buffer until the configured max_response_size is reached,
stop requesting more data, and mark the body as truncated; convert the collected
bytes to a String (using UTF-8 fallback like from_utf8_lossy) and pass that to
truncate_response or skip truncate_response if you already enforced the limit;
replace the response.text().await branch with this streaming logic, handle chunk
read errors similarly to the current Err(e) case, and ensure the output,
response_text variable, and any truncated marker are set accordingly when
building the ToolResult.
In `@clients/agent-runtime/src/tools/mcp/adapter.rs`:
- Around line 53-70: The fallback in the `truncated` computation can panic
because `unwrap_or_else(|| output[..max_body].to_string())` slices at a byte
index that may split a UTF-8 char; change the fallback to return an empty string
instead of slicing (i.e., replace that `unwrap_or_else` branch so it yields `""`
when no valid char boundary fits within `max_body`), keeping the existing `scan`
logic and using the same `output`, `max_body`, and `truncated` variables.
In `@clients/agent-runtime/src/tools/memory_forget.rs`:
- Around line 31-48: The parameters_schema() currently allows empty or
whitespace-only "key" values; update parameters_schema to include "minLength": 1
for the "key" property, and in execute(&self, args: serde_json::Value) trim the
extracted key string (call .trim()) and reject empty strings by returning an
anyhow error (e.g., Missing or empty 'key' parameter) before proceeding;
reference the functions parameters_schema and execute in memory_forget.rs and
ensure the runtime validation mirrors the schema so whitespace-only values are
rejected and a structured ToolResult error is returned rather than panicking.
In `@clients/agent-runtime/src/tools/pushover.rs`:
- Around line 53-84: The get_credentials function is performing blocking file
I/O with std::fs::read_to_string; change it to async by making get_credentials
async (async fn get_credentials(&self) -> anyhow::Result<(String, String)>) and
replace std::fs::read_to_string(&env_path) with
tokio::fs::read_to_string(&env_path).await (preserving the current error
mapping). Keep the rest of the parsing logic (lines referencing workspace_dir,
parse_env_value, PUSHOVER_TOKEN / PUSHOVER_USER_KEY) unchanged and update all
call sites (e.g., the call at the previously noted site) to .await the async
get_credentials call. Ensure the function signature and callers use the
appropriate async context and propagate anyhow::Result as before.
In `@clients/agent-runtime/src/tools/schedule.rs`:
- Around line 203-217: The constructed JSON `detail` in handle_get is being
pretty-printed into `output` but not returned in `structured`; change the
ToolResult construction to return the machine-readable payload by setting
`structured: Some(detail)` (or Some(detail.clone()) if needed for ownership)
instead of `None`, keeping `output: serde_json::to_string_pretty(&detail)?`
as-is so callers get both human and structured representations; update the
ToolResult fields accordingly in the handle_get function.
In `@Makefile`:
- Around line 15-22: The Makefile assumes bash exists on Windows by setting
SHELL when DETECTED_OS is Windows but gives a cryptic failure if bash is missing
because check-tools.sh requires bash; add an explicit bootstrap check: when
DETECTED_OS is Windows_NT, test for bash availability (e.g., via command
-v/which) and if not present emit a clear make error (using $(error ...))
explaining that bash is required to run check-tools.sh and how to install it,
and also add a short inline comment above the SHELL assignment documenting this
bootstrap dependency and why bash is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 139ca0d4-b35f-4ffd-907e-7a4bba354d85
📒 Files selected for processing (80)
.agents/journal/bolt-journal.md.agents/journal/scribe-journal.md.agents/journal/sentinnel-journal.md.agents/skills/release/SKILL.md.github/renovate.jsonMakefileclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/agent/mod.rsclients/agent-runtime/src/agent/prompt.rsclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/bootstrap/mod.rsclients/agent-runtime/src/channels/dingtalk.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/observability/prometheus.rsclients/agent-runtime/src/observability/traits.rsclients/agent-runtime/src/observability/verbose.rsclients/agent-runtime/src/peripherals/arduino_upload.rsclients/agent-runtime/src/peripherals/capabilities_tool.rsclients/agent-runtime/src/peripherals/rpi.rsclients/agent-runtime/src/peripherals/serial.rsclients/agent-runtime/src/peripherals/uno_q_bridge.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/security/mod.rsclients/agent-runtime/src/security/policy.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/browser_open.rsclients/agent-runtime/src/tools/composio.rsclients/agent-runtime/src/tools/cron_add.rsclients/agent-runtime/src/tools/cron_list.rsclients/agent-runtime/src/tools/cron_remove.rsclients/agent-runtime/src/tools/cron_run.rsclients/agent-runtime/src/tools/cron_runs.rsclients/agent-runtime/src/tools/cron_update.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/file_read.rsclients/agent-runtime/src/tools/file_write.rsclients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/image_info.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/memory_recall.rsclients/agent-runtime/src/tools/memory_store.rsclients/agent-runtime/src/tools/mod.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/screenshot.rsclients/agent-runtime/src/tools/shell.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/tools/web_search_tool.rsclients/agent-runtime/tests/agent_e2e.rsclients/agent-runtime/tests/agent_loop_integration.rsclients/agent-runtime/tests/legacy_loop_guard.rsclients/agent-runtime/tests/mcp_execution_limits.rsclients/agent-runtime/tests/mission_config_toggle.rsclients/agent-runtime/tests/mission_entrypoint_parity.rsclients/agent-runtime/tests/mission_governance_integration.rsclients/agent-runtime/tests/mission_lifecycle_integration.rsclients/agent-runtime/tests/mission_security_parity.rsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.ktclients/web/apps/docs/src/content/docs/en/guides/release.mdclients/web/apps/docs/src/content/docs/es/guides/release.mdclients/web/apps/docs/src/styles/custom.cssclients/web/apps/marketing/README.mdclients/web/apps/marketing/package.jsonclients/web/apps/marketing/public/installclients/web/apps/marketing/src/styles/global.cssopenspec/changes/code-agent-specialist/tasks.mdscripts/check-tools.shscripts/install.shscripts/sync-version-with-tag.sh
💤 Files with no reviewable changes (4)
- .agents/journal/scribe-journal.md
- .agents/journal/bolt-journal.md
- clients/agent-runtime/src/channels/dingtalk.rs
- .agents/journal/sentinnel-journal.md
Deploying corvus with
|
| Latest commit: |
d449f67
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e82fd70c.corvus-42x.pages.dev |
| Branch Preview URL: | https://code-agent.corvus-42x.pages.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
clients/web/apps/marketing/src/styles/global.css (1)
853-865:⚠️ Potential issue | 🟡 MinorRemove this redundant reduced-motion block.
This duplicates the block at lines 26-38, but without
!important—making it both redundant and broken. The earlier block already handles reduced-motion correctly. Keeping this risks reintroducing the accessibility regression if the first block is ever removed or refactored.Proposed fix: delete the duplicate block
-@media (prefers-reduced-motion: reduce) { - html { - scroll-behavior: auto; - } - - *, - *::before, - *::after { - transition-duration: 0.01ms; - animation-duration: 0.01ms; - animation-iteration-count: 1; - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/web/apps/marketing/src/styles/global.css` around lines 853 - 865, Remove the duplicate reduced-motion media query block (the "@media (prefers-reduced-motion: reduce)" rule that targets html and the selectors *, *::before, *::after) — delete this redundant block that sets scroll-behavior and tiny transition/animation durations without !important so the original block (the earlier reduced-motion rule) remains the single source of truth and accessibility behavior.clients/agent-runtime/src/tools/pushover.rs (1)
289-411:⚠️ Potential issue | 🔴 CriticalSix tests will fail to compile: sync tests calling async
get_credentialswithout.await.The
get_credentialsmethod is async, but these tests call it without.awaitin synchronous#[test]functions, causing type mismatches:
credentials_parsed_from_env_file(line 303)credentials_fail_without_env_file(line 318)credentials_fail_without_token(line 333)credentials_fail_without_user_key(line 348)credentials_ignore_comments(line 363)credentials_support_export_and_quoted_values(line 405)Convert each to async tests by changing
#[test]to#[tokio::test], the function signature toasync fn, and adding.awaitto theget_credentials()calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/pushover.rs` around lines 289 - 411, Tests call the async method get_credentials without awaiting it, causing compile errors; update each failing test (credentials_parsed_from_env_file, credentials_fail_without_env_file, credentials_fail_without_token, credentials_fail_without_user_key, credentials_ignore_comments, credentials_support_export_and_quoted_values) by changing #[test] to #[tokio::test], making the fn signature async fn, and appending .await to the get_credentials() call so it awaits the future returned by PushoverTool::get_credentials().clients/agent-runtime/src/peripherals/mod.rs (2)
115-126:⚠️ Potential issue | 🟠 MajorReject empty paths before saving the peripheral.
After trimming,
path = ""still gets persisted as a serial board. Startup will then skip that entry viavalidate_board_config(), socorvus peripheral add <board> " "reports success but leaves a dead config behind.Proposed fix
fn handle_add_command(board: String, path: String) -> Result<()> { let board = board.trim().to_string(); if board.is_empty() { anyhow::bail!("Peripheral board name cannot be empty"); } let path = path.trim().to_string(); + if path.is_empty() { + anyhow::bail!("Peripheral path cannot be empty"); + } let transport = if path == "native" { "native" } else { "serial" };As per coding guidelines, "Security first, performance second. Validate input boundaries..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/peripherals/mod.rs` around lines 115 - 126, In handle_add_command, after trimming path, validate it and reject empty strings before computing transport/path_opt: add a check like if path.is_empty() { anyhow::bail!("Peripheral path cannot be empty"); } so that a trimmed input of "" (e.g., " ") is not accepted and saved as a dead serial entry; keep the existing transport determination (if path == "native" { "native" } else { "serial" }) and path_opt logic unchanged but only run them after this validation.
162-190:⚠️ Potential issue | 🟠 MajorRegister static hardware tools from
board_names, not from successful connections.Phase B still checks
!tools.is_empty(). That means a valid but temporarily unreachable board never getshardware_memory_map,hardware_board_info, orhardware_memory_read, even though these tool constructors only need the validated board list. This removes diagnostics exactly when startup is degraded.Proposed fix
- // Phase B: Add hardware tools when any boards configured - if !tools.is_empty() { + // Phase B: Add hardware tools when any valid boards are configured + if !board_names.is_empty() { tools.push(Box::new(HardwareMemoryMapTool::new(board_names.clone()))); tools.push(Box::new(crate::tools::HardwareBoardInfoTool::new( board_names.clone(), ))); tools.push(Box::new(crate::tools::HardwareMemoryReadTool::new( board_names, ))); }As per coding guidelines, "Look for behavioral regressions, missing tests, and contract breaks across modules."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/peripherals/mod.rs` around lines 162 - 190, The code currently gates registration of HardwareMemoryMapTool, HardwareBoardInfoTool, and HardwareMemoryReadTool on tools being non-empty, which excludes validated-but-unreachable boards; change the condition to check board_names (use !board_names.is_empty()) so these static hardware tools are always registered when there are validated boards. Locate the block using board_names and tools and replace the if !tools.is_empty() check with if !board_names.is_empty(), leaving the existing pushes that call HardwareMemoryMapTool::new(board_names.clone()), crate::tools::HardwareBoardInfoTool::new(board_names.clone()), and crate::tools::HardwareMemoryReadTool::new(board_names) intact so constructors still receive the validated board list.clients/agent-runtime/src/tools/memory_forget.rs (1)
31-38:⚠️ Potential issue | 🟠 MajorKeep missing/blank
keyinputs insideToolResult.
minLength: 1still advertises" "as acceptable, and this branch turns that case into ananyhowerror instead of a normal failedToolResult. That makes schema-valid bad input look like an internal tool failure. Returnsuccess: falsehere and publish the same non-empty contract in the schema.Based on learnings: Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime pathAlso applies to: 45-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/memory_forget.rs` around lines 31 - 38, The parameters_schema currently allows whitespace-only "key" and the runtime branch converts that into an internal error; update the handler (in memory_forget.rs — the parameters_schema function and the tool execution method that returns ToolResult) to keep the non-empty contract in the JSON schema but validate and sanitize the input by trimming the "key" value, and if the trimmed key is empty return a ToolResult with success: false and a clear output/error message (no anyhow panic or internal error). Ensure the same validation logic appears in the execution method that currently maps inputs to the forgetting logic (the function that produces the ToolResult around lines 45–51) so whitespace-only keys yield a structured failure, not an internal error.clients/agent-runtime/src/tools/schedule.rs (1)
213-227:⚠️ Potential issue | 🟡 MinorKeep
geton the same structured schema aslistandcreate.
job_to_structured()now includesdelete_after_run, buthandle_get()hand-builds a smaller object here. The same job therefore has different machine-readable shapes depending on which action fetched it.Suggested fix
- let detail = json!({ - "id": job.id, - "expression": job.expression, - "command": job.command, - "next_run": job.next_run.to_rfc3339(), - "last_run": job.last_run.map(|value| value.to_rfc3339()), - "last_status": job.last_status, - "enabled": job.enabled, - "one_shot": matches!(job.schedule, cron::Schedule::At { .. }), - }); + let detail = job_to_structured(&job);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/schedule.rs` around lines 213 - 227, The structured JSON returned by handle_get currently hand-builds a smaller object and omits fields like delete_after_run, causing mismatch with job_to_structured() used by list/create; update handle_get (the block building detail and returning ToolResult) to produce the same structured schema as job_to_structured() — either call job_to_structured(&job) and use its output for both output and structured, or add the missing delete_after_run (and any other fields present in job_to_structured) to the hand-built detail so the machine-readable shape is identical across get/list/create.clients/agent-runtime/src/tools/hardware_memory_map.rs (1)
157-165:⚠️ Potential issue | 🟠 MajorReturn a failure when no map exists for the configured board.
When probe lookup fails and
static_map_for_board()returnsNone,map_textstaysNonebut this still emitssuccess: truewith"map": null. That makes machine consumers treat “no memory map” as a valid answer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/hardware_memory_map.rs` around lines 157 - 165, The current ToolResult always sets success: true even when static_map_for_board(...) returns None (map_text stays None), which misleads consumers; update the logic around static_map_for_board / map_text in the function building ToolResult so that if map_text is None you return a failure ToolResult (success: false) with a descriptive error message in error (e.g. "no memory map for board <board>") and structured should either omit "map" or set it to null explicitly, ensuring consumers can detect the missing map. Use the existing symbols ToolResult, static_map_for_board, map_text, output and structured to locate and modify the code path.
♻️ Duplicate comments (1)
clients/agent-runtime/src/config/schema.rs (1)
2889-2905:⚠️ Potential issue | 🟠 MajorKeep rejecting zero-valued code-session limits.
validate_code_session_config()still acceptsagent.code_session.max_iterations = 0,agent.code_session.timeout_ms = 0, andvalidation_commands[*].timeout_ms = 0. Those values turn sessions or validations into immediate no-ops/timeouts instead of failing fast at startup.As per coding guidelines: `clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs`: Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicableSuggested fix
fn validate_code_session_config(&self) -> Result<()> { let code_session = &self.agent.code_session; + if code_session.max_iterations == 0 { + anyhow::bail!("agent.code_session.max_iterations must be greater than zero"); + } + if code_session.timeout_ms == 0 { + anyhow::bail!("agent.code_session.timeout_ms must be greater than zero"); + } if code_session.enabled && code_session.validation_commands.is_empty() { anyhow::bail!( "agent.code_session.validation_commands must be non-empty when code_session is enabled" ); } for (idx, validation) in code_session.validation_commands.iter().enumerate() { if validation.command.trim().is_empty() { anyhow::bail!( "agent.code_session.validation_commands[{idx}].command must be non-empty" ); } + if validation.timeout_ms == 0 { + anyhow::bail!( + "agent.code_session.validation_commands[{idx}].timeout_ms must be greater than zero" + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/config/schema.rs` around lines 2889 - 2905, validate_code_session_config currently allows zero-valued limits which effectively disables/short-circuits sessions; update validate_code_session_config to reject zero (non-positive) values by adding checks that agent.code_session.max_iterations > 0 and agent.code_session.timeout_ms > 0 and that each validation_commands[*].timeout_ms > 0, and call anyhow::bail(...) with clear messages (including the index for per-command errors) when any of those fields are zero or non-positive so startup fails fast (refer to function validate_code_session_config, struct fields agent.code_session.max_iterations, agent.code_session.timeout_ms, and validation_commands[*].timeout_ms).
🤖 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/src/agent/code_session.rs`:
- Around line 242-249: The mapping currently sets every
ExecutedCommandSummary.success = true for entries coming from commands_raw (in
the commands variable), which misrepresents failed commands from the
commands_run field; update the parsing in the map that builds
ExecutedCommandSummary to detect and set success based on the command text
(e.g., parse common prefixes/suffixes like "FAILED", "SUCCESS", "[FAIL]", or a
"status: ..." token) or change the upstream representation so commands_raw
carries a structured status, then use that structured field to populate success
and risk_level accordingly; specifically modify the closure that constructs
ExecutedCommandSummary (referencing commands_raw and ExecutedCommandSummary) to
extract a success boolean from the raw entry instead of hardcoding true, or if
no status is available, add a comment/documentation near this mapping noting the
assumption that commands_run contains only successful commands.
In `@clients/agent-runtime/src/peripherals/mod.rs`:
- Around line 202-218: validate_board_config currently rejects "websocket" but
the schema comment for PeripheralBoardConfig still lists websocket and tests use
it; update the schema comment to remove "websocket" and add "bridge" so docs
match validation, and make the change explicit by updating tests: in the
arduino_flash.rs test that uses "websocket" switch it to a supported transport
(e.g., "bridge" or "serial") or assert the validation error if you intend to
keep failing behavior; ensure PeripheralBoardConfig documentation (schema.rs)
and any related comments reflect the new allowed set ("serial", "native",
"bridge") and that no code silently ignores unsupported transports.
In `@clients/agent-runtime/src/security/audit.rs`:
- Around line 255-267: log_code_session_event is currently persisting sensitive
fields (summary, commands, validations) directly into the AuditEvent by building
a CodeSessionAudit from the incoming CodeSessionAuditLog; instead create a
redacted DTO or sanitize those fields before constructing CodeSessionAudit
(e.g., implement a redact_code_session or RedactedCodeSession type and call it
from log_code_session_event), ensure you replace summary, commands, and
validations with scrubbed values (mask tokens/credentials and remove
known-secret patterns) and use the redacted object when calling
AuditEvent::with_code_session so no raw secrets are written to audit.log.
In `@clients/agent-runtime/src/tools/browser.rs`:
- Around line 951-993: The runtime rejects valid schema calls because
allowed_params_for_action/validate_action_params enforce per-action-only fields
while parameters_schema() publishes a flat union; fix by making the schema
action-discriminated or by relaxing validation: update parameters_schema() to
return per-action discriminated schemas (via an "action" enum + per-action
properties) OR change validate_action_params (and the same logic around lines
referenced) to ignore/strip irrelevant keys instead of returning an error (i.e.,
only enforce presence and types of required keys from allowed_params_for_action
and silently accept extra fields), keeping error creation via
Self::failed_tool_result and avoiding panics; apply the same change to the
related checks at the other occurrence (lines ~1141-1143).
In `@clients/agent-runtime/src/tools/delegate.rs`:
- Around line 174-183: The current classification block that sets
CodeSessionStatus based on substring checks of error_text (in the agent.turn()
error handling) is fragile; change it to inspect a typed error instead: update
agent.turn() to return or wrap errors as a typed enum (e.g., AgentError with
variants like IterationBudgetExceeded, Timeout, ApprovalBlocked, etc.), then
replace the string.contains checks with a match on the error enum to map
variants to CodeSessionStatus::BudgetExceeded, ::Blocked, or ::Error;
alternatively, add a helper function (e.g.,
map_agent_error_to_code_session_status(error: &AgentError) -> CodeSessionStatus)
and call that from the location using error_text to make the mapping explicit
and robust.
In `@clients/agent-runtime/src/tools/git_operations.rs`:
- Around line 511-518: The code validates branch syntax with git
check-ref-format but then uses git checkout which can silently operate on paths
instead of branches; change the branch checkout call in
run_git_command(&["checkout", branch_name]) to use git switch
(run_git_command(&["switch", branch_name])) so the operation only targets
branches and fails if the branch doesn't exist, and additionally replace or
augment the syntax-only check-ref-format step with an existence check (e.g.,
run_git_command(&["show-ref", "--verify", format!("refs/heads/{}", branch_name)
or use rev-parse --verify refs/heads/<branch>]) before switching) to ensure the
branch actually exists; locate these changes around the calls referencing
branch_name and the run_git_command helper.
In `@clients/agent-runtime/src/tools/hardware_memory_map.rs`:
- Around line 88-92: The code currently falls back to self.boards[0] when args
is missing or board is malformed; instead validate args strictly: in the method
that reads args (the block using args.get("board") to produce board) first
ensure args.as_object() is Some, then if the "board" key exists require it to be
a string and return a structured ToolResult error (use the project's ToolResult
error variant) when it's not a string or when args is not an object; only when
"board" is absent should you pick a default from self.boards, and avoid silently
accepting malformed inputs—update the logic around args, board, self.boards, and
the Tool implementation to return validation errors rather than falling back.
In `@clients/agent-runtime/src/tools/hardware_memory_read.rs`:
- Around line 75-91: The code currently auto-selects self.boards[0] when
args.get("board") is missing; change this so that when the "board" parameter is
omitted and self.boards.len() > 1 you return a ToolResult error instructing the
caller to specify a board, otherwise (len == 1) you may safely use that single
board; specifically update the logic around args.get("board") /
.unwrap_or_else(...) in hardware_memory_read.rs to first check
self.boards.len(), only default to the single configured board when there is
exactly one, and keep the subsequent validation that the chosen board exists in
self.boards before returning success/failure ToolResult (refer to
args.get("board"), self.boards, and ToolResult).
In `@clients/agent-runtime/src/tools/http_request.rs`:
- Around line 205-239: The tests still call the removed helper
truncate_response, causing compile failures; either restore a small
compatibility wrapper named truncate_response that forwards to the new async
read_response_body (matching the original public signature used by tests) or
update the tests to call read_response_body directly and adapt to its async
return type and parameters; locate functions read_response_body and any test
references to truncate_response and ensure the wrapper or tests use the same
signature/visibility so cargo test compiles.
In `@clients/agent-runtime/src/tools/traits.rs`:
- Around line 55-62: The code currently swallows schema validation failures by
replacing the parameters with an empty permissive schema; instead fail-closed:
when SchemaCleanr::validate(¶meters) returns Err, propagate or return that
error (do not set parameters to {"type":"object","properties":{}}), and ensure
callers of the function (and the tool registration path) handle a Result/Err so
the tool registration aborts; update any Tool implementations in src/tools/ to
provide a valid strict parameters_schema() and sanitize inputs so they conform
before registration. Ensure you reference and change the logic around
SchemaCleanr::validate, self.parameters_schema(), and the registration path that
consumes the validated schema so malformed schemas cause registration failure
rather than silent fallback.
In `@clients/web/apps/docs/src/styles/custom.css`:
- Around line 388-398: The comment claiming transitions are "opt-in" is
misleading because the rule targeting a, button, .card, input, and summary
applies globally and will be inherited by third-party components unless they
override it; update the comment above this block in custom.css to state that
these transitions are applied broadly to those elements (and may affect
third-party components) and note that components can override or opt out via
their own transition or transition-property rules (the prefers-reduced-motion
block already handles accessibility).
In `@Makefile`:
- Around line 106-108: The Makefile target doctor is incorrectly gated by
check-tools which prevents scripts/doctor.sh from running when development tools
are missing; update the doctor target to depend only on bootstrap-bash (remove
check-tools as a prerequisite) so make doctor will always invoke
scripts/doctor.sh and let that script report missing Java/Node/pnpm/Rust; adjust
the doctor target definition (referencing the doctor target name, check-tools
prerequisite, bootstrap-bash prerequisite, and scripts/doctor.sh) accordingly so
the make recipe runs unconditionally and environment checks are handled inside
the script.
In `@openspec/changes/code-agent-specialist/tasks.md`:
- Around line 5-29: The tasks checklist in
openspec/changes/code-agent-specialist/tasks.md incorrectly marks items 1.1–4.3
as complete; revert the premature checkmarks by changing the checked boxes ([x])
back to unchecked ([ ]) for all entries referencing the unimplemented tests and
code changes (items 1.1 through 4.3), and add a brief note or comment line next
to the top of the checklist explaining that these items remain TODO until the
implementation PR lands so the spec tracker and docs remain accurate; locate and
edit the lines containing the checklist entries in that file to update the boxes
and add the explanatory note.
In `@scripts/doctor.sh`:
- Around line 12-28: The version probes currently abort the whole script on
non-zero exits (due to set -e); modify each probe so failures are downgraded to
warnings instead of exiting: keep the existing command existence checks (command
-v docker / pnpm / rustup) and when running docker --version, pnpm --version,
and rustup show active-toolchain, append a safe failure handler (e.g., capture
the command's stderr/exit status and echo a clear "warning" message including
the error output) or use a construct that prevents non-zero exit propagation
(e.g., run the version command followed by || { echo "Warning: <tool> version
check failed: <error>"; } ), ensuring the script continues to run remaining
checks and still reports useful diagnostic output referencing the exact commands
docker --version, pnpm --version, and rustup show active-toolchain.
In `@scripts/install.sh`:
- Around line 895-918: The restart_daemon_if_available function currently
restarts the Corvus service but returns success without verifying health; after
calling "$resolved_cmd" service restart (in restart_daemon_if_available) add a
post-restart health probe using the Corvus health command (e.g., "$resolved_cmd
doctor" or the internal health check) and only print success when that probe
reports healthy; if the probe fails or reports degraded/unhealthy, print a clear
error/warn message and return a non-zero status so callers know the update did
not fully succeed.
- Around line 858-875: The post_install_steps function currently prefers command
-v corvus over the freshly installed binary and may call an unquoted
resolved_cmd; change the resolver to prefer INSTALLED_BINARY_PATH when set and
executable (use INSTALLED_BINARY_PATH as resolved_cmd before falling back to
command -v corvus), ensure resolved_cmd is assigned safely, and invoke
"$resolved_cmd" quoted when computing resolved_version; apply the identical fix
to the other resolver block referenced around lines 895-902 so both places
consistently prefer INSTALLED_BINARY_PATH, set resolved_cmd accordingly, and
quote the invocation.
- Around line 44-64: detect_existing_install() currently treats any corvus on
PATH as a binary install by setting EXISTING_CMD_PATH/EXISTING_INSTALL_DIR and
ALREADY_INSTALLED, which leads select_install_method() to force a binary install
and resolve_install_dir() to overwrite shims/wrappers; change
detect_existing_install() to detect package-manager shims/wrappers (e.g., if
EXISTING_CMD_PATH is a symlink, lives under node_modules/.bin, pnpm/bun/npm shim
content, or is a JS script with a node shebang) and in those cases either
preserve and record the original install method (set a variable like
EXISTING_INSTALL_METHOD) or treat it as “non-replaceable” so you do not force
binary install; update select_install_method() to respect
EXISTING_INSTALL_METHOD (avoid forcing "binary") and update
resolve_install_dir() to only reuse EXISTING_INSTALL_DIR when the file is a real
native binary or when an installer-owned marker/version check confirms it is
safe to update.
- Around line 776-779: The "yarn" case branch currently uses the removed Yarn 2+
command pattern `yarn global add "$CLI_PACKAGE"`, which fails on Yarn Berry;
update the "yarn" case to detect Yarn major version and either use `yarn dlx` to
run the CLI temporarily for Yarn>=2 or keep `yarn global add "$CLI_PACKAGE"` for
Yarn 1.x — i.e., add a Yarn version check (via the Yarn version command) inside
the yarn case label and branch to use `yarn dlx "$CLI_PACKAGE"` for modern Yarn
or fallback to the existing global install for classic Yarn, ensuring you still
reference the same "$CLI_PACKAGE" variable.
---
Outside diff comments:
In `@clients/agent-runtime/src/peripherals/mod.rs`:
- Around line 115-126: In handle_add_command, after trimming path, validate it
and reject empty strings before computing transport/path_opt: add a check like
if path.is_empty() { anyhow::bail!("Peripheral path cannot be empty"); } so that
a trimmed input of "" (e.g., " ") is not accepted and saved as a dead serial
entry; keep the existing transport determination (if path == "native" { "native"
} else { "serial" }) and path_opt logic unchanged but only run them after this
validation.
- Around line 162-190: The code currently gates registration of
HardwareMemoryMapTool, HardwareBoardInfoTool, and HardwareMemoryReadTool on
tools being non-empty, which excludes validated-but-unreachable boards; change
the condition to check board_names (use !board_names.is_empty()) so these static
hardware tools are always registered when there are validated boards. Locate the
block using board_names and tools and replace the if !tools.is_empty() check
with if !board_names.is_empty(), leaving the existing pushes that call
HardwareMemoryMapTool::new(board_names.clone()),
crate::tools::HardwareBoardInfoTool::new(board_names.clone()), and
crate::tools::HardwareMemoryReadTool::new(board_names) intact so constructors
still receive the validated board list.
In `@clients/agent-runtime/src/tools/hardware_memory_map.rs`:
- Around line 157-165: The current ToolResult always sets success: true even
when static_map_for_board(...) returns None (map_text stays None), which
misleads consumers; update the logic around static_map_for_board / map_text in
the function building ToolResult so that if map_text is None you return a
failure ToolResult (success: false) with a descriptive error message in error
(e.g. "no memory map for board <board>") and structured should either omit "map"
or set it to null explicitly, ensuring consumers can detect the missing map. Use
the existing symbols ToolResult, static_map_for_board, map_text, output and
structured to locate and modify the code path.
In `@clients/agent-runtime/src/tools/memory_forget.rs`:
- Around line 31-38: The parameters_schema currently allows whitespace-only
"key" and the runtime branch converts that into an internal error; update the
handler (in memory_forget.rs — the parameters_schema function and the tool
execution method that returns ToolResult) to keep the non-empty contract in the
JSON schema but validate and sanitize the input by trimming the "key" value, and
if the trimmed key is empty return a ToolResult with success: false and a clear
output/error message (no anyhow panic or internal error). Ensure the same
validation logic appears in the execution method that currently maps inputs to
the forgetting logic (the function that produces the ToolResult around lines
45–51) so whitespace-only keys yield a structured failure, not an internal
error.
In `@clients/agent-runtime/src/tools/pushover.rs`:
- Around line 289-411: Tests call the async method get_credentials without
awaiting it, causing compile errors; update each failing test
(credentials_parsed_from_env_file, credentials_fail_without_env_file,
credentials_fail_without_token, credentials_fail_without_user_key,
credentials_ignore_comments, credentials_support_export_and_quoted_values) by
changing #[test] to #[tokio::test], making the fn signature async fn, and
appending .await to the get_credentials() call so it awaits the future returned
by PushoverTool::get_credentials().
In `@clients/agent-runtime/src/tools/schedule.rs`:
- Around line 213-227: The structured JSON returned by handle_get currently
hand-builds a smaller object and omits fields like delete_after_run, causing
mismatch with job_to_structured() used by list/create; update handle_get (the
block building detail and returning ToolResult) to produce the same structured
schema as job_to_structured() — either call job_to_structured(&job) and use its
output for both output and structured, or add the missing delete_after_run (and
any other fields present in job_to_structured) to the hand-built detail so the
machine-readable shape is identical across get/list/create.
In `@clients/web/apps/marketing/src/styles/global.css`:
- Around line 853-865: Remove the duplicate reduced-motion media query block
(the "@media (prefers-reduced-motion: reduce)" rule that targets html and the
selectors *, *::before, *::after) — delete this redundant block that sets
scroll-behavior and tiny transition/animation durations without !important so
the original block (the earlier reduced-motion rule) remains the single source
of truth and accessibility behavior.
---
Duplicate comments:
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 2889-2905: validate_code_session_config currently allows
zero-valued limits which effectively disables/short-circuits sessions; update
validate_code_session_config to reject zero (non-positive) values by adding
checks that agent.code_session.max_iterations > 0 and
agent.code_session.timeout_ms > 0 and that each
validation_commands[*].timeout_ms > 0, and call anyhow::bail(...) with clear
messages (including the index for per-command errors) when any of those fields
are zero or non-positive so startup fails fast (refer to function
validate_code_session_config, struct fields agent.code_session.max_iterations,
agent.code_session.timeout_ms, and validation_commands[*].timeout_ms).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d04611b-385c-4489-91fe-42902e6fa815
📒 Files selected for processing (29)
Makefileclients/agent-runtime/README.mdclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/peripherals/mod.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/traits.rsclients/web/apps/docs/src/styles/custom.cssclients/web/apps/marketing/package.jsonclients/web/apps/marketing/public/installclients/web/apps/marketing/src/styles/global.cssclippy.tomlopenspec/changes/code-agent-specialist/tasks.mdscripts/bootstrap-bash.shscripts/doctor.shscripts/install.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (8)
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
scripts/bootstrap-bash.shclients/web/apps/marketing/package.jsonclients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/web/apps/marketing/public/installscripts/doctor.shclients/agent-runtime/src/tools/traits.rsclients/web/apps/docs/src/styles/custom.cssclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsMakefileclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/tools/http_request.rsscripts/install.shclients/agent-runtime/src/agent/code_session.rsclippy.tomlclients/agent-runtime/README.mdclients/agent-runtime/src/tools/hardware_board_info.rsopenspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/web/apps/marketing/src/styles/global.cssclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
clients/agent-runtime/src/tools/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path
Files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/tools/hardware_board_info.rsclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
clients/agent-runtime/README.mdopenspec/changes/code-agent-specialist/tasks.md
🧠 Learnings (12)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/hardware_memory_map.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/tools/hardware_board_info.rsopenspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/src/tools/hardware_memory_read.rsclients/agent-runtime/src/tools/memory_forget.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/agent/code_session.rsclients/agent-runtime/src/tools/hardware_board_info.rsopenspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/tools/pushover.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/schedule.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/tools/git_operations.rsclients/agent-runtime/src/tools/delegate.rsclients/agent-runtime/src/tools/browser.rsscripts/doctor.shclients/agent-runtime/src/security/audit.rsMakefileclients/agent-runtime/src/agent/code_session.rsopenspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/providers/**/*.rs : Implement `Provider` trait in `src/providers/` and register in `src/providers/mod.rs` factory when adding a new provider
Applied to files:
clients/agent-runtime/src/tools/delegate.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/tools/browser.rsclients/agent-runtime/src/security/audit.rsclients/agent-runtime/README.mdopenspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/peripherals/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/security/audit.rsopenspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/tools/mcp/adapter.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
openspec/changes/code-agent-specialist/tasks.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
openspec/changes/code-agent-specialist/tasks.mdclients/agent-runtime/src/config/schema.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/peripherals/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/agent-runtime/src/config/schema.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/tools/mcp/adapter.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Applied to files:
clients/agent-runtime/src/agent/agent.rs
🪛 checkmake (0.2.2)
Makefile
[warning] 371-371: Missing required phony target "all"
(minphony)
[warning] 371-371: Missing required phony target "test"
(minphony)
🔇 Additional comments (21)
clients/web/apps/marketing/src/styles/global.css (1)
26-38: Reduced-motion block correctly implemented with!important.This properly overrides higher-specificity class rules (
.btn,.terminal-card,.copy-button.is-copied, etc.) ensuring motion is suppressed for users who need it.clients/web/apps/docs/src/styles/custom.css (2)
400-407: LGTM!Proper use of
aria-current="true"for TOC navigation state. The transition is covered by the reduced-motion safeguard below.
559-573: Good accessibility safeguard.The
prefers-reduced-motion: reduceblock correctly nullifies all animations and transitions for users who request reduced motion. Using0.01msinstead of0msavoids edge cases where some browsers skip zero-duration transitions entirely.This addresses the accessibility gap from the previous review, even though it uses the "apply globally + override for reduce" pattern rather than "conditional apply."
clients/agent-runtime/src/tools/pushover.rs (2)
53-57: LGTM: Async I/O conversion is correct.Using
tokio::fs::read_to_stringaligns with the async runtime and avoids blocking the executor. Error context is preserved.
126-131: LGTM:structured: Noneadditions align with ToolResult contract.All ToolResult instantiations now include the required
structuredfield. Per the trait definition, this field uses#[serde(skip_serializing_if = "Option::is_none")], so None values are cleanly omitted during serialization.Also applies to: 134-141, 156-164, 199-206, 212-229
clients/agent-runtime/src/observability/otel.rs (2)
274-301: LGTM! Span status is now properly set based on session outcome.The handler correctly maps success-like statuses to
Status::Okand others toStatus::error. The selective field destructuring via..is consistent withverbose.rs.
485-495: Test coverage for CodeSessionCompleted is appropriate.The test exercises all required fields and validates the event can be recorded without panic.
clients/agent-runtime/src/tools/delegate.rs (4)
1-28: Clear module documentation with security and rollback notes.The doc comments properly document execution modes, security boundary (child sessions use same
SecurityPolicy), and rollback instructions. Good for maintainability.
158-163: Timeout handling now uses milliseconds correctly.The previous integer division issue is resolved - now using
Duration::from_millis(timeout_ms)with.max(1)ensuring a minimum 1ms timeout. This avoids the truncation problem where sub-second values became 0.
114-127: Config cloning and credential propagation looks correct.The child config is built from
base_configclone with provider/model overrides. API key is set fromagent_config.api_keyorfallback_credential- no logging of credentials.
393-398: Session mode dispatch correctly branches before OneShot path.The security check at line 350-360 runs before this branch, so both modes enforce the same
SecurityPolicy. Depth limit is also checked earlier (line 336).clients/agent-runtime/src/agent/code_session.rs (4)
6-41: CodeSessionStatus enum is well-defined with comprehensive variants.The
#[default]onSuccessand case-insensitiveas_str()mapping are appropriate. The enum covers all expected session outcomes.
221-233: Parser now handles all documented status values including "success" and "failed".The
to_ascii_lowercase()ensures case-insensitive matching, addressing the previous review concern.
256-262: Unprefixed validation entries default to success.If a validation entry lacks
pass:orfail:prefix, it's treated as passing. This is a reasonable fallback given the prompt specifies the prefix format.
196-198: to_structured() safely handles serialization failure.Using
unwrap_or(Value::Null)prevents panics if serialization fails, which aligns with the "no panics in runtime path" guideline.clients/agent-runtime/src/agent/agent.rs (6)
367-388: Audit logger initialization now honors config settings.The function correctly uses
config.security.auditinstead of the hardcoded default, and respectsaudit_config.strictto fail-fast when initialization fails in strict mode. This addresses the previous review concern.
772-776: CORVUS_SESSION_ID fallback is now documented.The doc comment explains the environment variable usage and UUID fallback behavior, addressing the previous review suggestion.
861-878: Audit logging respects strict mode.When
audit_strictis true, audit failures now bail with an error instead of silently continuing. This addresses the previous review concern about security-critical deployments.
804-845: Sensitive data is redacted before logging.Commands, blockers, pending_work, and summary are all passed through
redact_observer_payload()before being included in observer events and audit logs. This aligns with the "never log secrets" guideline.
737-755: Turn flow correctly wraps session handling.Code-mode sessions capture session_id before calling
turn_inner, then parse/record results regardless of success/error. The error path usescode_session_result_from_errorfor proper status classification.
29-55: Agent struct expansion for code-session support is clean.New fields (
audit_logger,audit_strict,code_mode,code_session_delegated) are appropriately typed. The#[allow(clippy::struct_excessive_bools)]is reasonable given these are independent flags.
Comment on lines
+202
to
+218
| fn validate_board_config(board: &PeripheralBoardConfig) -> Result<(), String> { | ||
| let board_name = board.board.trim(); | ||
| if board_name.is_empty() { | ||
| return Err("board name is empty".to_string()); | ||
| } | ||
|
|
||
| match board.transport.as_str() { | ||
| "serial" => { | ||
| if board.path.as_deref().map(str::trim).unwrap_or("").is_empty() { | ||
| return Err("serial transport requires a path".to_string()); | ||
| } | ||
| } | ||
| "native" | "bridge" => {} | ||
| other => { | ||
| return Err(format!( | ||
| "unsupported transport '{other}' (use: serial, native, bridge)" | ||
| )); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Schema/doc references for peripheral transports:"
rg -n -C2 '\b(websocket|bridge|serial|native)\b' clients/agent-runtime/src/config/schema.rs
echo
echo "Runtime handling of peripheral transports:"
rg -n -C3 '\b(websocket|bridge|serial|native)\b' clients/agent-runtime/src/peripherals clients/agent-runtime/src/tools clients/agent-runtime/src/configRepository: dallay/corvus
Length of output: 48020
🏁 Script executed:
sed -n '155,185p' clients/agent-runtime/src/peripherals/arduino_flash.rsRepository: dallay/corvus
Length of output: 1151
🏁 Script executed:
# Check how validate_board_config is called in the flow
rg -n "validate_board_config" clients/agent-runtime/src/peripherals/mod.rsRepository: dallay/corvus
Length of output: 204
🏁 Script executed:
sed -n '155,175p' clients/agent-runtime/src/peripherals/mod.rsRepository: dallay/corvus
Length of output: 886
🏁 Script executed:
# Check if there's any websocket implementation or if it was never implemented
rg -i "websocket" clients/agent-runtime/src/peripherals/ | head -20Repository: dallay/corvus
Length of output: 159
🏁 Script executed:
# Get the full context of the validation and warning flow
sed -n '155,220p' clients/agent-runtime/src/peripherals/mod.rsRepository: dallay/corvus
Length of output: 2287
Update schema and reconsider silent fallback for breaking transport changes.
validate_board_config() now rejects websocket transport (lines 216–218), but PeripheralBoardConfig still documents it (clients/agent-runtime/src/config/schema.rs:582). Configs using that value silently skip with a warning rather than failing explicitly.
Additionally, the new bridge transport is accepted (line 214) but missing from the schema comment. Update schema.rs:582 to remove websocket, add bridge, and either:
- Fail validation with an error (preferred), or
- If silent skip is intentional, explicitly document the fallback behavior in the schema and add a corresponding test.
A test in arduino_flash.rs (line 175–181) already uses websocket transport and should be updated per the chosen approach.
Per learnings: "Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/peripherals/mod.rs` around lines 202 - 218,
validate_board_config currently rejects "websocket" but the schema comment for
PeripheralBoardConfig still lists websocket and tests use it; update the schema
comment to remove "websocket" and add "bridge" so docs match validation, and
make the change explicit by updating tests: in the arduino_flash.rs test that
uses "websocket" switch it to a supported transport (e.g., "bridge" or "serial")
or assert the validation error if you intend to keep failing behavior; ensure
PeripheralBoardConfig documentation (schema.rs) and any related comments reflect
the new allowed set ("serial", "native", "bridge") and that no code silently
ignores unsupported transports.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
clients/agent-runtime/src/tools/http_request.rs (1)
205-239:⚠️ Potential issue | 🟠 MajorMid-stream body read failures still masked as synthetic success output.
The error path at lines 213-215 returns a formatted string rather than propagating the error. A 200 response with a mid-stream failure will still yield
success: truewith body"[Failed to read response body: ...]". This hides transport/decode errors from callers.Consider returning
anyhow::Result<String>and mapping errors to a failureToolResultinexecute.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/http_request.rs` around lines 205 - 239, The read_response_body function currently masks mid-stream read errors by returning a formatted string; change its signature to return anyhow::Result<String> (or Result<String, anyhow::Error>) and return Err(error.into()) on chunk read failures instead of a synthetic string; preserve the truncation behavior by returning Ok(text) or Ok(truncated_text) when successful; then update the caller (execute) to handle the Err case and convert it into a failing ToolResult (map the error into ToolResult::failure or equivalent) so transport/decoding errors are propagated rather than treated as success; use existing symbols read_response_body, execute, ToolResult, and max_response_size to locate and update the code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@clients/agent-runtime/src/tools/http_request.rs`:
- Around line 205-239: The read_response_body function currently masks
mid-stream read errors by returning a formatted string; change its signature to
return anyhow::Result<String> (or Result<String, anyhow::Error>) and return
Err(error.into()) on chunk read failures instead of a synthetic string; preserve
the truncation behavior by returning Ok(text) or Ok(truncated_text) when
successful; then update the caller (execute) to handle the Err case and convert
it into a failing ToolResult (map the error into ToolResult::failure or
equivalent) so transport/decoding errors are propagated rather than treated as
success; use existing symbols read_response_body, execute, ToolResult, and
max_response_size to locate and update the code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0aa00f60-0d12-4566-ae09-da771d3bd7c5
📒 Files selected for processing (2)
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (7)
clients/agent-runtime/src/tools/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
🧠 Learnings (6)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/pushover.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/tools/http_request.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/tools/http_request.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/tools/http_request.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/tools/http_request.rs
🔇 Additional comments (5)
clients/agent-runtime/src/tools/http_request.rs (2)
241-249: Test compatibility wrapper restores compilation.The
#[cfg(test)]truncate_responsehelper preserves backward compatibility for existing unit tests while production code uses the streamingread_response_body. This addresses the previous concern about broken tests.
272-279:structured: Noneadditions align with ToolResult schema extension.Both
tool_error_resultand the success path inexecutenow initialize the newstructuredfield. Consistent with the broader PR changes toToolResult.Also applies to: 389-398
clients/agent-runtime/src/tools/pushover.rs (3)
53-57: Good: async credential loading avoids blocking the runtime.Switching from
std::fs::read_to_stringtotokio::fs::read_to_stringprevents blocking the async executor during file I/O. Based on learnings: "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency."
124-231: AllToolResultpaths consistently initializestructured: None.Read-only block, rate-limit block, invalid-priority error, HTTP failure, and success/failure API responses all set the new field. Aligns with the broader
ToolResultschema extension.
289-411: Tests properly converted to async.All credential-related tests now use
#[tokio::test]and.awaitonget_credentials(). The test setup usingstd::fs::writeis acceptable since test setup is not on the hot path.
|
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.
This pull request contains a series of improvements and maintenance updates across project tooling, developer workflow, and documentation. The most significant changes include improvements to the
Makefilefor better cross-platform compatibility and developer experience, stricter Renovate configuration to control dependency update PRs, and various documentation and journal cleanups.Developer Tooling and Workflow Improvements:
Makefileto improve Windows compatibility by assumingbashis in thePATHand delegating toolchain checks to a newscripts/check-tools.shscript, simplifying environment detection and setup. Added a newdoctortarget for quick diagnostics and improved output consistency for cleaning commands. [1] [2] [3] [4] [5] [6] [7] [8] [9]web-check-all,test-all,check-all,clean-web, andclean-pnpmtargets to streamline multi-module development and maintenance. Thedevtarget now includes setup by default for a smoother onboarding experience. [1] [2] [3] [4] [5]Dependency Management:
.github/renovate.jsonto strictly limit Renovate's concurrent PRs, hourly PRs, and concurrent branches to 1 each, reducing noise and risk in dependency updates. Removed post-update deduplication steps to avoid unnecessary changes. [1] [2]Documentation and Journal Maintenance:
bolt-journal.md,scribe-journal.md,sentinnel-journal.md) to declutter the repository. [1] [2] [3].agents/skills/release/SKILL.mdto reference the correct location of thesync-version-with-tag.shscript.These changes collectively improve project maintainability, reduce developer friction, and ensure a cleaner repository.
Closes: #180