perf(compose): optimize list item reuse and transformation allocations#131
Closed
yacosta738 wants to merge 4 commits into
Closed
perf(compose): optimize list item reuse and transformation allocations#131yacosta738 wants to merge 4 commits into
yacosta738 wants to merge 4 commits into
Conversation
- Added contentType to LazyColumn in ChatPanel to improve item recycling and scroll performance. - Wrapped PasswordVisualTransformation in remember to avoid redundant object allocations on every recomposition during user input. - Verified with :composeApp:check and updated the performance journal. Co-authored-by: yacosta738 <33158051+yacosta738@users.noreply.github.com>
Deploying corvus with
|
| Latest commit: |
e929494
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a34e5527.corvus-42x.pages.dev |
| Branch Preview URL: | https://perf-compose-optimizations-1.corvus-42x.pages.dev |
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
|
Caution Review failedFailed to post review comments 📝 WalkthroughSummary by CodeRabbit
WalkthroughLarge refactor/removal: the PR removes the mission/MCP subsystems and associated tests/docs, simplifies approval/dispatcher/policy logic, adjusts config/schema and observability, updates many package/version manifests, alters CI/workflows, and includes two small Jetpack Compose performance tweaks. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-04 to 2026-03-04 |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/journal/bolt-journal.md:
- Line 75: Update the journal heading "## 2025-02-18 - KMP - List & Input
Optimization" so the date matches this PR’s timeline (change to "2026-03-04" or
the correct PR date) and scan the same markdown entry for any other date strings
to keep the entry consistent with the PR creation date; edit the line containing
that heading text ("## 2025-02-18 - KMP - List & Input Optimization") to the
corrected date.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f67b7931-e5a0-427e-bd47-dcb7fe2618a2
📒 Files selected for processing (1)
.agents/journal/bolt-journal.md
📜 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: pr-checks
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-21T09:07:52.298Z
Learnt from: yacosta738
Repo: dallay/corvus PR: 62
File: .agents/journal/sentinnel-journal.md:1-1
Timestamp: 2026-02-21T09:07:52.298Z
Learning: Branding guideline: The intentional brand name for the security-first agent in the dallay/corvus repository is 'Sentinnel' (with double n). Do not treat it as a typo of 'Sentinel'. Ensure all agent-related docs and journals under .agents/journal consistently use 'Sentinnel' with double n.
Applied to files:
.agents/journal/bolt-journal.md
| **Benchmark:** | ||
| Incremental build time: ~1.8s. All checks passed. | ||
|
|
||
| ## 2025-02-18 - KMP - List & Input Optimization |
Contributor
There was a problem hiding this comment.
Fix journal entry date to match this PR’s timeline.
The heading date (2025-02-18) conflicts with this PR’s creation date (2026-03-04), which makes change history harder to trust.
✏️ Proposed doc fix
-## 2025-02-18 - KMP - List & Input Optimization
+## 2026-03-04 - KMP - List & Input Optimization🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/journal/bolt-journal.md at line 75, Update the journal heading "##
2025-02-18 - KMP - List & Input Optimization" so the date matches this PR’s
timeline (change to "2026-03-04" or the correct PR date) and scan the same
markdown entry for any other date strings to keep the entry consistent with the
PR creation date; edit the line containing that heading text ("## 2025-02-18 -
KMP - List & Input Optimization") to the corrected date.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Added contentType to LazyColumn in ChatWorkspace to improve item reuse efficiency. - Wrapped PasswordVisualTransformation in remember to avoid redundant allocations during typing. - Updated performance journal with the latest optimizations. Co-authored-by: yacosta738 <33158051+yacosta738@users.noreply.github.com>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 50
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/dashboard/src/App.vue (1)
455-474: 🧹 Nitpick | 🔵 TrivialUnused parameter renamed—clarify intent.
parsedis never used insidebuildObservabilityPayload. The prior_parsedname correctly signaled it was intentionally unused. Either restore the underscore prefix or, if there's missing logic that should referenceparsed, add it now.Option A: restore underscore prefix
function buildObservabilityPayload( - parsed: ParsedValues, + _parsed: ParsedValues, snapshot: InitialConfigSnapshot ): Record<string, unknown> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/web/apps/dashboard/src/App.vue` around lines 455 - 474, The parameter parsed in buildObservabilityPayload is unused and should either be marked intentionally unused by restoring the underscore prefix (_parsed) or actually used if logic was accidentally omitted; update the function signature in buildObservabilityPayload to rename parsed to _parsed to indicate it’s intentionally unused, or add the missing references to parsed where needed (e.g., within conditionallyAdd calls or payload construction) and remove the underscore if you depend on its values.clients/agent-runtime/src/gateway/admin.rs (1)
310-374:⚠️ Potential issue | 🟠 MajorAdd targeted regression tests for the new gateway restart checks.
This block expands restart gating for security-sensitive gateway fields, but there’s no focused test coverage for changed/unchanged behavior (including normalized max-key inputs). In this high-risk surface, that’s a release blocker for regression safety.
Proposed test additions
@@ + #[test] + fn test_restart_required_updates_gateway_flags_and_limits() { + let cfg = test_config(); + let mut patch = AdminConfigUpdateRequest { + default_provider: None, + default_model: None, + default_temperature: None, + memory_backend: None, + observability: None, + runtime: None, + autonomy: None, + scheduler: None, + gateway: Some(AdminGatewayPatch { + port: None, + host: Some("0.0.0.0".into()), + require_pairing: None, + allow_public_bind: None, + pair_rate_limit_per_minute: None, + webhook_rate_limit_per_minute: None, + trust_forwarded_headers: None, + rate_limit_max_keys: None, + idempotency_ttl_secs: None, + idempotency_max_keys: None, + }), + webhook: None, + }; + assert_eq!(restart_required_updates(&cfg, &patch), vec!["gateway.host"]); + + patch.gateway.as_mut().unwrap().host = None; + patch.gateway.as_mut().unwrap().require_pairing = Some(!cfg.gateway.require_pairing); + assert_eq!(restart_required_updates(&cfg, &patch), vec!["gateway.require_pairing"]); + + patch.gateway.as_mut().unwrap().require_pairing = None; + patch.gateway.as_mut().unwrap().allow_public_bind = Some(!cfg.gateway.allow_public_bind); + assert_eq!(restart_required_updates(&cfg, &patch), vec!["gateway.allow_public_bind"]); + + patch.gateway.as_mut().unwrap().allow_public_bind = None; + patch.gateway.as_mut().unwrap().trust_forwarded_headers = + Some(!cfg.gateway.trust_forwarded_headers); + assert_eq!( + restart_required_updates(&cfg, &patch), + vec!["gateway.trust_forwarded_headers"] + ); + }As per coding guidelines "
**/*: Security first, performance second. Validate input boundaries, auth/authz implications, and secret management. 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/gateway/admin.rs` around lines 310 - 374, Add focused regression tests exercising collect_restart_required_gateway: create unit tests that build a baseline Config and various AdminGatewayPatch inputs and assert the exact contents of the returned/updated fields vector for changed vs unchanged cases. Cover each gate: port, host (trim behavior), require_pairing, allow_public_bind, pair_rate_limit_per_minute, webhook_rate_limit_per_minute, trust_forwarded_headers, gateway.rate_limit_max_keys and idempotency_max_keys (use gateway::utils::normalize_max_keys to assert normalized inputs that should/shouldn't trigger restart), and idempotency_ttl_secs (include ttl==0 behavior that preserves existing TTL). Put tests beside the function (e.g., in a #[cfg(test)] mod in the same file or in its unit test module) to create Config and AdminGatewayPatch instances, call collect_restart_required_gateway, and assert fields contains or does not contain the expected "gateway.*" strings for each scenario.clients/web/build.gradle.kts (1)
151-157:⚠️ Potential issue | 🟠 MajorFail fast on missing dist output and make archive output deterministic
At Line 156, the zip source path is not validated, so an empty/missing dist directory can still produce a zip artifact. Also, reproducibility flags are not set explicitly for this new distribution artifact.
Proposed patch
tasks.register<Zip>("${appName}DistZip") { group = "web" description = "Zip ${appName} distribution" archiveFileName = "${appName}-dist.zip" destinationDirectory.set(file("${webRootDir}/build/distributions")) - from("${appDir}/${config.distDir}") + val distOutput = appDir.resolve(config.distDir) + from(distOutput) + isReproducibleFileOrder = true + isPreserveFileTimestamps = false + doFirst { + if (!distOutput.exists() || distOutput.list()?.isEmpty() != false) { + throw GradleException("Missing or empty dist output for '$appName' at ${distOutput.absolutePath}") + } + } dependsOn(appBuild) }Verify by checking this task block contains a dist guard and reproducibility flags:
#!/bin/bash set -euo pipefail rg -n -C3 'register<Zip>\("\$\{appName\}DistZip"\)|isReproducibleFileOrder|isPreserveFileTimestamps|doFirst|GradleException' clients/web/build.gradle.ktsAs per coding guidelines, "Review plugin/config changes for supply-chain and reproducibility risks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/web/build.gradle.kts` around lines 151 - 157, The zip task "${appName}DistZip" must fail fast if the source dist directory is missing and must set reproducibility flags; update the Zip task registered as tasks.register<Zip>("${appName}DistZip") to add a doFirst block that checks that file("${appDir}/${config.distDir}") exists and isNotEmpty (throw a GradleException with a clear message if not), and set reproducibility properties on the Zip task (isReproducibleFileOrder = true and isPreserveFileTimestamps = false or the Gradle-equivalent flags) so the produced archive is deterministic; ensure the task still dependsOn(appBuild).clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatComponents.kt (1)
147-147: 🧹 Nitpick | 🔵 TrivialAlign FunctionNaming suppression strategy across chat UI files for consistency.
ChatWorkspace.kt uses
@file:Suppress("FunctionNaming")while ChatComponents.kt does not, yet both follow idiomatic Compose naming conventions (PascalCase for@Composablefunctions). Either apply the suppression to ChatComponents.kt or configure detekt to ignore@Composable-annotatedfunctions project-wide. This is a stylistic inconsistency, not a functional issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatComponents.kt` at line 147, ChatComponents.kt defines Compose functions like ChatBubble using PascalCase but lacks the file-level suppression used in ChatWorkspace.kt; add the same `@file`:Suppress("FunctionNaming") directive at the top of ChatComponents.kt so Compose functions (e.g., ChatBubble) are consistently exempt from the detekt FunctionNaming rule, or alternatively apply a project-wide detekt rule to ignore `@Composable` functions—prefer adding the file-level suppression to mirror ChatWorkspace.kt for immediate consistency.clients/agent-runtime/src/providers/mod.rs (1)
302-330: 🛠️ Refactor suggestion | 🟠 MajorReplace per-call
Vecallocations with static slices.Line 302 currently allocates a new
Vecon every call toresolve_provider_credential. This is avoidable in a frequently-used path; use&[&str]/&[]to make candidate selection allocation-free.♻️ Proposed refactor
- let provider_env_candidates: Vec<&str> = match name { - "anthropic" => vec!["ANTHROPIC_OAUTH_TOKEN", "ANTHROPIC_API_KEY"], - "openrouter" => vec!["OPENROUTER_API_KEY"], - "openai" => vec!["OPENAI_API_KEY"], - "copilot" | "github-copilot" => vec!["GITHUB_TOKEN", "GH_TOKEN"], - "ollama" => vec!["OLLAMA_API_KEY"], - "venice" => vec!["VENICE_API_KEY"], - "groq" => vec!["GROQ_API_KEY"], - "mistral" => vec!["MISTRAL_API_KEY"], - "deepseek" => vec!["DEEPSEEK_API_KEY"], - "xai" | "grok" => vec!["XAI_API_KEY"], - "together" | "together-ai" => vec!["TOGETHER_API_KEY"], - "fireworks" | "fireworks-ai" => vec!["FIREWORKS_API_KEY"], - "perplexity" => vec!["PERPLEXITY_API_KEY"], - "cohere" => vec!["COHERE_API_KEY"], - name if is_moonshot_alias(name) => vec!["MOONSHOT_API_KEY"], - name if is_glm_alias(name) => vec!["GLM_API_KEY"], - name if is_minimax_alias(name) => vec!["MINIMAX_API_KEY"], - name if is_qianfan_alias(name) => vec!["QIANFAN_API_KEY"], - name if is_qwen_alias(name) => vec!["DASHSCOPE_API_KEY"], - name if is_zai_alias(name) => vec!["ZAI_API_KEY"], - "nvidia" | "nvidia-nim" | "build.nvidia.com" => vec!["NVIDIA_API_KEY"], - "synthetic" => vec!["SYNTHETIC_API_KEY"], - "opencode" | "opencode-zen" => vec!["OPENCODE_API_KEY"], - "vercel" | "vercel-ai" => vec!["VERCEL_API_KEY"], - "cloudflare" | "cloudflare-ai" => vec!["CLOUDFLARE_API_KEY"], - "astrai" => vec!["ASTRAI_API_KEY"], - _ => vec![], + let provider_env_candidates: &[&str] = match name { + "anthropic" => &["ANTHROPIC_OAUTH_TOKEN", "ANTHROPIC_API_KEY"], + "openrouter" => &["OPENROUTER_API_KEY"], + "openai" => &["OPENAI_API_KEY"], + "copilot" | "github-copilot" => &["GITHUB_TOKEN", "GH_TOKEN"], + "ollama" => &["OLLAMA_API_KEY"], + "venice" => &["VENICE_API_KEY"], + "groq" => &["GROQ_API_KEY"], + "mistral" => &["MISTRAL_API_KEY"], + "deepseek" => &["DEEPSEEK_API_KEY"], + "xai" | "grok" => &["XAI_API_KEY"], + "together" | "together-ai" => &["TOGETHER_API_KEY"], + "fireworks" | "fireworks-ai" => &["FIREWORKS_API_KEY"], + "perplexity" => &["PERPLEXITY_API_KEY"], + "cohere" => &["COHERE_API_KEY"], + name if is_moonshot_alias(name) => &["MOONSHOT_API_KEY"], + name if is_glm_alias(name) => &["GLM_API_KEY"], + name if is_minimax_alias(name) => &["MINIMAX_API_KEY"], + name if is_qianfan_alias(name) => &["QIANFAN_API_KEY"], + name if is_qwen_alias(name) => &["DASHSCOPE_API_KEY"], + name if is_zai_alias(name) => &["ZAI_API_KEY"], + "nvidia" | "nvidia-nim" | "build.nvidia.com" => &["NVIDIA_API_KEY"], + "synthetic" => &["SYNTHETIC_API_KEY"], + "opencode" | "opencode-zen" => &["OPENCODE_API_KEY"], + "vercel" | "vercel-ai" => &["VERCEL_API_KEY"], + "cloudflare" | "cloudflare-ai" => &["CLOUDFLARE_API_KEY"], + "astrai" => &["ASTRAI_API_KEY"], + _ => &[], }; - for env_var in provider_env_candidates { + for env_var in provider_env_candidates.iter().copied() {As per coding guidelines:
clients/agent-runtime/src/**/*.rs— “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/providers/mod.rs` around lines 302 - 330, The match currently builds a new Vec<&str> each call (provider_env_candidates) inside resolve_provider_credential; change provider_env_candidates to a static slice type (e.g. &'static [&'static str]) and return slice literals (e.g. &["OPENAI_API_KEY"]) from each match arm instead of vec![], so no per-call allocation is done; update any subsequent code that iterates or indexes provider_env_candidates to accept a slice (it should already work with &[&str]) and remove Vec-specific methods if present.clients/agent-runtime/src/cron/store.rs (1)
35-50:⚠️ Potential issue | 🟠 MajorAvoid forcing
delete_after_run = 0for all shell jobs.Line 40 hardcodes
delete_after_runto0, which removes configurable/implicit cleanup behavior for shell jobs at creation time. This is a behavioral regression risk for one-shot/at flows.Suggested fix
pub fn add_shell_job( config: &Config, name: Option<String>, schedule: Schedule, command: &str, ) -> Result<CronJob> { @@ + let delete_after_run = matches!(&schedule, Schedule::At { .. }); @@ - ) VALUES (?1, ?2, ?3, ?4, 'shell', NULL, ?5, 'isolated', NULL, 1, ?6, 0, ?7, ?8)", + ) VALUES (?1, ?2, ?3, ?4, 'shell', NULL, ?5, 'isolated', NULL, 1, ?6, ?7, ?8, ?9)", params![ id, expression, command, schedule_json, name, serde_json::to_string(&DeliveryConfig::default())?, + if delete_after_run { 1 } else { 0 }, now.to_rfc3339(), next_run.to_rfc3339(), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/cron/store.rs` around lines 35 - 50, The INSERT into cron_jobs inside with_connection currently hardcodes delete_after_run = 0 in the SQL, which removes per-job or DB-default cleanup behavior; change the INSERT in conn.execute (the cron_jobs INSERT used by the store) to use a parameter for delete_after_run (e.g., ?N) or omit the delete_after_run column so the DB default applies, and bind that parameter from the calling code (or the function argument) instead of always writing 0.clients/agent-runtime/src/tools/cron_add.rs (1)
45-46:⚠️ Potential issue | 🟠 Major
delete_after_runis silently ignored for shell jobs.You parse
delete_after_run, expose it in schema, then drop it in the shell branch (Line 131). Return an explicit error for shell jobs (or remove it from schema for shell) to avoid silent contract drift.Suggested guard
let delete_after_run = args .get("delete_after_run") .and_then(serde_json::Value::as_bool) .unwrap_or(default_delete_after_run); let result = match job_type { JobType::Shell => { + if args.get("delete_after_run").is_some() { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some("'delete_after_run' is not supported for shell jobs".to_string()), + }); + } let command = match args.get("command").and_then(serde_json::Value::as_str) {Based on learnings: “Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path” and “prefer explicit errors over silent fallback for unsupported critical paths”.Also applies to: 104-108, 110-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/cron_add.rs` around lines 45 - 46, The cron_add tool exposes the parameter delete_after_run in its schema but silently ignores it in the shell job branch; update the shell handling in cron_add.rs so that when job_kind == Shell (the shell branch around the current shell handling code) you validate params and either return an explicit error (e.g., a ToolResult/ToolError indicating delete_after_run is unsupported for shell jobs) if delete_after_run is present, or remove delete_after_run from the schema if shell jobs truly must not accept it; ensure validation happens before executing or constructing the cron entry and surface a clear structured error via the Tool trait return type rather than silently dropping the field.
🤖 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/firmware/corvus-nucleo/src/main.rs`:
- Around line 133-170: The code repeatedly calls has_cmd(&line_buf, ...) which
re-scans the buffer; instead, extract the command once (e.g., let cmd =
get_cmd_from(&line_buf) or parse the "cmd" field once) and replace the if/else
chain with a single match on that cmd byte/slice, then handle "ping",
"capabilities", "gpio_read", and "gpio_write" branches using the same
parse_arg(&line_buf, ...) calls for pin/value and reusing id_str, LED_PIN and
led.set_level as before; this centralizes command parsing, avoids repeated
full-buffer scans, and makes adding commands easier.
- Around line 155-163: The gpio_write branch currently parses value with
parse_arg(...).unwrap_or(0) and treats any non-zero as HIGH; change this to
enforce value must be exactly 0 or 1: after calling parse_arg(&line_buf,
b"value") (no silent default), validate that it returned a numeric value and
that that value is either 0 or 1; if not, write an error response into resp_buf
(using the same id_str and an ok:false/error message) and return, otherwise use
the validated value to call led.set_level(if value == 1 { Level::High } else {
Level::Low }). Keep the existing pin checks (LED_PIN/0..=13) and ensure you
reference parse_arg, has_cmd, led.set_level, LED_PIN, resp_buf, and id_str when
making the change.
In `@clients/agent-runtime/src/agent/agent.rs`:
- Around line 556-559: The approval-required output is using extracted_reason as
if it were the invoked target; update the code that builds the response (the
struct fields with output, success, tool_call_id and
DispatchAction::ApprovalRequired) so the output string includes both the tool
name and the extracted reason separately (e.g., "approval required to run
<tool_name>: <reason>") instead of only `{extracted_reason}`; use the tool
identifier available on call (call.tool_call_id or call.tool_name) and keep
DispatchAction::ApprovalRequired(extracted_reason) unchanged so the action still
carries the reason.
- Around line 399-401: The return path for unknown tools in the handler
currently returns (format!("Unknown tool: {}", call.name), false) without
recording an observer event; update the unknown-tool branch in the same function
that handles tool invocation to emit a ToolCall observer event (e.g., create and
record a ToolCall/ToolCallFailure event including call.name and the error
message) before returning so observer metrics include unknown-tool failures;
ensure you use the same observer API used elsewhere in this file (the
observer.record.../record_event/emit method and the ToolCall event type) and
keep the returned tuple unchanged.
In `@clients/agent-runtime/src/agent/tests.rs`:
- Around line 505-507: The assertion in the test inspects
ConversationMessage::ToolResults and checks for the exact phrase "approval
required before executing", which is fragile; update the predicate used in the
results.iter().any(...) to instead check for a broader stable signal such as
r.content.contains("approval required") && r.tool_id.contains("YOUR_TOOL_ID")
(or whichever tool identifier is present in the result structure) so the test
verifies that an approval was required for the specific tool without depending
on the full sentence wording. Ensure you reference
ConversationMessage::ToolResults, results, and r.content (and r.tool_id or
equivalent field) when making the change.
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 1042-1045: The system prompt currently hardcodes Discord-specific
lines via prompt.push_str for "- You are running as a Discord bot..." and "-
When someone messages you on Discord..." which are injected for all channels;
change this to be channel-agnostic by either removing those two hardcoded
Discord lines from the shared prompt or wrapping them in a runtime conditional
that only appends them when the active channel indicates Discord (e.g., check
the channel enum/parameter passed into the prompt builder). Locate the code that
constructs the shared system prompt (the variable named prompt and the
prompt.push_str calls) and add a channel check or use a generic line like "You
may send messages to the active channel when appropriate" so non-Discord
sessions don’t receive Discord-specific instructions.
In `@clients/agent-runtime/src/channels/telegram.rs`:
- Around line 1645-1649: Replace the raw error interpolation in the Telegram
polling error logs (the tracing::warn! calls invoked after
self.client.post(...).json(&body).send().await fails and the similar block at
the later occurrence) with a sanitized, non-sensitive message (e.g., "Telegram
poll failed, retrying") and, if you need structured details, record only
non-sensitive fields derived from the error (for example a boolean like
e.is_timeout() or an HTTP status from e.status() if present) rather than the
full error string or raw request/transport details; update the tracing::warn!
invocations to use that sanitized message and optional safe fields instead of
"{e}".
- Around line 823-829: The current loop around send_message_with_fallback in the
Telegram channel silently ignores errors (it only proceeds on Ok and continues
on Err), causing send to return Ok(()) even if all chunk sends failed; update
the send implementation that calls send_message_with_fallback to propagate
errors instead of swallowing them—specifically, in the loop around
send_message_with_fallback(&self.client, &api_url, body).await, handle the
Result by returning Err when send_message_with_fallback returns Err (or collect
and return the first/aggregate error if multiple chunks may fail) and only
continue to the next chunk on success; ensure the final send method signature
and semantics (in the impl of Channel for Telegram) return an appropriate Err
when any chunk fails and preserve the small sleep between successful chunk
sends.
- Around line 1704-1713: The sendChatAction call is passing msg.reply_target
directly as "chat_id", which can be a composite "chat_id:thread_id" and breaks
Telegram API expectations; update the sendChatAction logic to parse the reply
target using Self::parse_reply_target() (the same parsing used by
send_message/edit_message/delete_message), extract the chat_id and optional
message_thread_id, and include chat_id and, if present, message_thread_id in the
typing_body JSON instead of the raw reply_target so typing actions work for
forum threads.
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 2367-2369: The environment override calls for provider/model
(env_override_string("CORVUS_PROVIDER", "PROVIDER", &mut self.default_provider)
and env_override_string("CORVUS_MODEL", "MODEL", &mut self.default_model)) are
executed after the GLM/ZAI alias-specific API-key logic, causing alias
mismatches; move these two env_override_string calls so they execute before the
alias-detection / alias-specific key-selection block (the code that checks for
GLM/ZAI aliases and applies alias-specific API key overrides) so that
CORVUS_PROVIDER/CORVUS_MODEL are applied first and the subsequent alias-specific
credential selection uses the overridden values.
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 1553-1556: The JSON error payload currently emits only free-form
text in the "error" field; change it to a stable, machine-readable structure by
adding explicit denial fields (e.g., "status": "denied" or "denied": true and a
"denial_reason" or nested "denial": { "reason": ... }) while keeping the
existing "error" message and "session_id". Update the JSON construction around
the err variable in mod.rs (gateway module) so webhook clients can reliably
parse denial status and reason (e.g., include "status"/"denied" and
"denial_reason"/"denial.reason" alongside "session_id").
In `@clients/agent-runtime/src/main.rs`:
- Line 630: The match arm for Commands::Onboard currently calls unreachable!(),
which can panic; replace it with an explicit error return (e.g., use
anyhow::bail! or return Err(anyhow::anyhow!(...))) so the CLI returns a
controlled error. Update the Commands::Onboard match arm in main.rs to produce a
clear, user-facing error message (referencing the Commands::Onboard variant and
the surrounding match in main) and ensure the enclosing function's return type
supports anyhow::Result or a compatible error type.
- Around line 1149-1155: The callback capture Err branch currently prints errors
and returns Ok(()) which falsely reports success; update the Err(e) arm (the
callback capture/authorization handling code in main) to return a non-success
result instead of Ok(()): propagate or convert the error (e.g., return
Err(e.into()) or return an anyhow::Error with context that includes the profile
and the printed suggestion), so the CLI exits with failure when OAuth login is
incomplete and no tokens were stored.
- Around line 1229-1230: The PasteToken and SetupToken command handlers call
auth::normalize_provider(&provider)? then proceed to
auth::anthropic_token::detect_auth_kind(), but they lack an explicit provider
guard; add a check after normalize_provider to enforce provider == "anthropic"
and return an error (bail! or equivalent) with a clear message if not, mirroring
the pattern used in auth login/auth paste-redirect/auth refresh; update the
handlers for PasteToken and SetupToken to perform this guard before calling
auth::anthropic_token::detect_auth_kind().
In `@clients/agent-runtime/src/memory/snapshot.rs`:
- Around line 237-239: The current loop unconditionally drops lines where
trimmed.starts_with("*Created:") or trimmed == "---" which causes legitimate
content loss; update the logic in the snapshot hydration code (the loop handling
`trimmed` in memory/snapshot.rs, likely inside the hydrate/deserialize function)
to only treat those tokens as metadata when they match the exporter's trailing
footer pattern (e.g., detect the full footer block or that you are past the
content boundary) instead of globally skipping any such lines; preserve literal
content lines that merely start with "*Created:" or equal "---" and add a
regression test that writes and rehydrates an entry containing those exact
literals to ensure a lossless round-trip.
In `@clients/agent-runtime/src/memory/sqlite.rs`:
- Around line 752-779: The code is filtering by session_id in Rust after
applying LIMIT which can truncate session-specific results; update the SQL in
both branches inside the function using the prepared statements (the blocks that
call conn.prepare and stmt.query_map with row_mapper) to include session_id in
the WHERE clause when session_ref is Some(sid) instead of filtering in-memory:
i.e., for the category branch add "AND session_id = ?" (and pass sid as an extra
param before the LIMIT), and for the no-category branch add "WHERE session_id =
?" (again pass sid then LIMIT), remove the in-memory session_id checks inside
the for row loop, and adjust the params! order to supply the session id and
DEFAULT_LIST_LIMIT to stmt.query_map so results pushed into results already
respect session filtering.
In `@clients/agent-runtime/src/onboard/wizard.rs`:
- Around line 3407-3411: The parsing of allowed phone numbers into
allowed_numbers should ignore empty entries from users_str (e.g., trailing
commas); update the else branch that builds allowed_numbers to trim each split
segment and filter out empty strings before collecting into Vec<String> so
users_str and allowed_numbers only contain non-empty phone values (preserve the
existing special-case when users_str.trim() == "*").
- Around line 1862-2014: This match block creates a second hardcoded model
catalog for the variable models that diverges from the existing
curated_models_for_provider / default_model_for_provider logic; replace the
entire match on canonical_provider used to build models in onboard::wizard.rs
with a call to the shared catalog provider (e.g., invoke
curated_models_for_provider(canonical_provider) or map to
default_model_for_provider where a single default tuple is required), preserve
any alias normalization (like treating "google" / "google-gemini" as the
canonical key) before calling the shared function, and fall back to the shared
default ("default" or default_model_for_provider) so all onboarding flows use
the same canonical model lists and defaults.
- Around line 3365-3380: The spawn block uses reqwest::blocking::Client::new()
without timeouts which can hang; replace creation with a builder configured like
the rest of the code (use reqwest::blocking::Client::builder()/ClientBuilder)
and set timeout(Duration::from_secs(8)) and
connect_timeout(Duration::from_secs(4)) before build() so the thread won’t block
(affecting the code around thread_result / phone_number_id_clone /
access_token_clone). Also fix the allowed-phone-list split later by trimming
entries and filtering out empty strings (use
.split(',').map(str::trim).filter(|s| !s.is_empty())... when collecting the
allowed phone numbers) so blank entries like "123, ,456" are not stored.
- Line 4: Update the PR description (or add a short accompanying commit message)
for the changes touching clients/agent-runtime (e.g., the onboard module that
includes HeartbeatConfig, IMessageConfig, MatrixConfig, MemoryConfig,
ObservabilityConfig in wizard.rs) to state that you ran and the results of:
`cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and
`cargo test` (or explicitly note which were skipped and why); include pass/fail
output or a brief note that all passed so the reviewer can verify compliance
with the CONTRIBUTING.md checklist.
- Around line 3327-3361: Replace the visible Input::new() prompts for the secret
values with dialoguer's Password::new() and enforce non-empty, confirmed verify
token: for access_token (the variable access_token) replace
Input::new().with_prompt(...).interact_text() with
Password::new().with_prompt(...).interact() so the token is hidden; for
verify_token (the variable verify_token) remove the .default(...) and instead
use Password::new().with_prompt(...).with_confirmation("Confirm webhook verify
token", "Tokens do not match").allow_empty_password(false).interact() to require
a non-empty, confirmed value; keep the existing Err(_) => return false error
handling and leave phone_number_id logic unchanged.
In `@clients/agent-runtime/src/providers/copilot.rs`:
- Around line 386-403: The code currently parses message.content as JSON and
returns an ApiMessage with role "tool" even when tool_call_id or content are
missing or non-string; update the block in providers/copilot.rs so you validate
both parsed fields are present and are strings before returning a "tool"
ApiMessage (i.e., require tool_call_id.is_some() && content.is_some()); if
either is missing, do not return a "tool" ApiMessage (fall through to the
non-tool handling or return an error/None as appropriate), and ensure the
ApiMessage struct (ApiMessage) is only constructed with validated values to
avoid producing malformed provider requests.
- Around line 353-357: The code currently calls
serde_json::from_str::<serde_json::Value>(&message.content) to get an owned
Value then uses value.get("tool_calls") and
serde_json::from_value::<Vec<ProviderToolCall>>(tool_calls_value.clone()), which
clones the tool_calls subtree; instead, take ownership of the tool_calls Value
from the already-owned Value (e.g., use value.as_object_mut().and_then(|o|
o.remove("tool_calls")) or value.get_mut and remove the key) and pass that owned
Value directly into serde_json::from_value::<Vec<ProviderToolCall>> to eliminate
the tool_calls_value.clone() allocation while keeping the same deserialization
path (referenced symbols: message.content,
serde_json::from_str::<serde_json::Value>, value.get("tool_calls"),
tool_calls_value.clone(), serde_json::from_value::<Vec<ProviderToolCall>>).
In `@clients/agent-runtime/src/providers/reliable.rs`:
- Around line 222-226: The warning "Exhausted retries, trying next
provider/model" is logged unconditionally even when the loop broke early for a
non-retryable error; update the logic around the tracing::warn! call
(referencing provider_name, current_model) to only log when retries were truly
exhausted (e.g., guard it with an exhausted_retries boolean set to true only
when the loop falls through all attempts or check attempt == self.max_retries),
or otherwise check attempt == self.max_retries before emitting the warning so
short-circuit breaks do not produce the misleading message.
- Around line 187-193: The rotation code currently calls self.rotate_key() and
logs a suffix of the key but never applies the rotated credential to the
subsequent request, and it also leaks sensitive data; update the retry path so
the value returned by rotate_key() is used to replace the auth/context passed to
make_request (or whatever auth field/provider credential is used by the provider
request) before retrying, remove the tracing::info log that prints any portion
of the key (replace with a non-secret message like "rotated API key for
provider" using provider_name), and ensure the code path that constructs the
request (e.g., the call sites around make_request and the internal auth storage)
reads the updated key/index so rotation affects behavior at runtime.
In `@clients/agent-runtime/src/tools/cron_add.rs`:
- Around line 104-108: Change matches!(schedule, Schedule::At { .. }) to
matches!(&schedule, Schedule::At { .. }) so you borrow rather than move the
non-Copy enum `schedule`; keep using `schedule` later in the match arms. Also
reconcile the `delete_after_run` parameter: either remove it from the shell-job
schema or (preferred) update `add_shell_job` to accept a `delete_after_run:
bool` parameter and thread `delete_after_run` (parsed from `args`) through the
call sites (same way `add_agent_job` receives it) and use it in the shell job
creation logic so the parameter is not silently ignored. Ensure you update the
`add_shell_job` signature, its internal usage, and any call sites where
`add_shell_job` is invoked.
In `@clients/agent-runtime/src/tools/http_request.rs`:
- Around line 171-188: The response header logging in execute() currently only
masks "set-cookie" and is out of sync with the tested helper; update execute()
to use the existing redact_headers_for_display(headers: &[(String, String)]) ->
Vec<(String, String)> (or factor a single shared redaction function) when
formatting or emitting response headers so all sensitive keys (authorization,
api-key, apikey, token, secret, etc.) are consistently redacted; locate the code
path in execute() that prints or logs response headers and replace its ad-hoc
masking with a call to redact_headers_for_display and then emit the returned Vec
for output/logging.
In `@clients/agent-runtime/src/update/mod.rs`:
- Around line 676-678: The change removed "lark"/"dingtalk"/"qq" from
infer_authorized_sender causing authorized_sender to be None and widening
confirmation authority; restore those channels to the match arm in
infer_authorized_sender so that recipient strings for "lark", "dingtalk", and
"qq" return Some(recipient.to_string()) (matching how "telegram"/"signal"/...
are handled) ensuring authorized_sender remains set for those channels and
prevents unintended authz broadening during update execution.
- Around line 661-670: The sort key for targets currently uses only channel and
recipient (targets.sort_by) but deduplication compares channel, recipient, and
authorized_sender (targets.dedup_by), which can leave nondeterministic
duplicates; update the sort_by comparator to include authorized_sender in the
comparison chain (i.e., compare channel, then recipient, then authorized_sender)
so sorting and deduplication use the same canonical key.
In `@clients/web/apps/chat/package.json`:
- Line 3: The package.json "version" field was downgraded from 0.2.0 to 0.1.15
which is a semantic rollback; either revert the "version" key back to the prior
monotonic version (e.g., "0.2.0") or, if the rollback is intentional, add a
clear note in the PR and update any version-dependent automation, release notes,
and telemetry consumers to reflect the change; reference the "version" key in
package.json when making the change and ensure CI/release scripts and changelog
are updated accordingly.
In `@clients/web/apps/dashboard/package.json`:
- Line 3: The package.json "version" field was downgraded from 0.2.0 to 0.1.15
which can break tooling; restore a monotonic version (e.g., set the "version"
key back to 0.2.0 or a higher semver) or, if the downgrade is intentional, add
an explicit migration note in this PR and update CHANGELOG/release notes and any
consumers that depend on the previous version to avoid rollout/cache/telemetry
regressions.
In `@clients/web/apps/docs/package.json`:
- Around line 16-24: The devDependencies currently use floating "latest" specs
for `@astrojs/check` and typescript; replace those with pinned versions by
updating the package.json devDependencies entries for "@astrojs/check" and
"typescript" to specific version strings (e.g., replace "latest" with the exact
version you want) so CI lockfile regeneration cannot pull unreviewed upgrades;
ensure you run pnpm install to update the lockfile after changing the
"@astrojs/check" and "typescript" entries.
In
`@clients/web/apps/docs/src/content/docs/en/guides/architecture/diagrams/container/system-containers.puml`:
- Line 6: The include line currently pulls the C4_PlantUML library from the
master branch; update that URL to reference a specific tagged release (e.g.,
replace the raw github master URL pointing to C4_Container.puml with the same
raw URL but using a stable tag like v2.13.0 or the latest release tag) so the
diagram import is pinned to a specific version and rendering is reproducible.
In `@clients/web/apps/docs/src/content/docs/en/guides/gpg-setup.md`:
- Around line 75-76: The GPG export command currently uses "gpg
--export-secret-keys" which exports the entire secret key (including the primary
key); replace this with "gpg --export-secret-subkeys" so only the subkey is
exported (primary key remains a stub/offline), i.e., update the line that calls
"gpg --export-secret-keys --armor YOUR_SUBKEY_ID > private-subkey.asc" to use
the "--export-secret-subkeys" flag instead, ensuring the doc references
YOUR_SUBKEY_ID (the subkey) and the output file private-subkey.asc remain the
same.
- Line 25: Update the wording and GPG command in both English and Spanish docs:
change the line containing "**Key size:** `4096` bits (minimum required by Maven
Central)" to indicate 4096 is a "recommended" or "best practice" size (e.g.,
replace "minimum required by Maven Central" with "recommended best practice");
locate the GPG export command that uses the flag "--export-secret-keys" and
replace it with "--export-secret-subkeys" (and any example invocations that
export the entire secret keyring) so CI/CD guidance exports only subkeys and not
primary key material; apply these exact edits in the corresponding English and
Spanish sections where the key size line and the export command appear.
In
`@clients/web/apps/docs/src/content/docs/es/guides/architecture/diagrams/container/system-containers.puml`:
- Line 12: The actor label in the PlantUML line Person(endUser, "End User",
"Usuario final del agente") is in English even though this file is under the
Spanish docs; change the visible label to Spanish by replacing "End User" with
the appropriate Spanish text (e.g., "Usuario final") so the Person(endUser, ...)
declaration uses consistent Spanish localization across the file.
- Line 6: The PlantUML include uses the mutable "master" ref for
C4_Container.puml causing non-deterministic builds; update the include URL (the
line that imports C4_Container.puml) to pin a stable tag (e.g., replace "master"
with "v2.13.0") in both the Spanish and English diagram files so the diagrams
reference an immutable release of the C4-PlantUML library.
In `@clients/web/apps/docs/src/content/docs/es/guides/gpg-setup.md`:
- Line 159: The fenced code block in the markdown lacks a language tag which
fails lint and reduces readability; update the backtick fence in the
docs/guides/gpg-setup.md content (the triple backticks shown at the comment) to
include a language identifier (e.g., add "text" after the opening ``` so it
becomes ```text) to satisfy the linter and improve rendering.
- Line 25: La línea que declara "**Tamaño:** `4096` bits (mínimo requerido por
Maven Central)" es incorrecta; reemplaza ese texto por una formulación que lo
deje claro como recomendación, por ejemplo indicando "**Tamaño:** `4096` bits
(recomendado; Maven Central no exige un mínimo; los ejemplos oficiales usan RSA
3072)" para reflejar que 4096 es una práctica de seguridad y no un requisito
obligatorio; busca y modifica la cadena exacta "**Tamaño:** `4096` bits (mínimo
requerido por Maven Central)" en el documento para corregirla.
- Around line 75-76: Replace the gpg export command that uses
--export-secret-keys with --export-secret-subkeys to avoid exporting the master
key; update the line containing "gpg --export-secret-keys --armor TU_ID_SUBCLAVE
> private-subkey.asc" in the docs to use "gpg --export-secret-subkeys --armor
TU_ID_SUBCLAVE > private-subkey.asc" so only subkeys are exported for CI/CD
usage.
In `@clients/web/apps/marketing/package.json`:
- Line 3: The package.json "version" field was changed to "0.1.15" which appears
to be a downgrade; confirm whether this change was intentional and either
restore the previous version string in package.json (revert the "version" value
in clients/web/apps/marketing package.json to the prior release) or add a brief
commit message/PR note documenting the intentional version change and its reason
so release automation and version tracking aren't broken.
In `@gradle.properties`:
- Line 27: The VERSION property is set to 0.1.15 which is lower than the
published 0.2.0 and will block npm publish; either update the VERSION value (the
VERSION key in gradle.properties) to 0.2.0 or a higher new semver (e.g., 0.2.1
or 0.3.0) before publishing, or if this is an intentional rollback, add a
documented rollback section (risk assessment, affected versions, mitigation
steps, and explicit revert/publish procedure) to the release notes and changelog
so maintainers can follow the safe rollback process.
In `@openspec/specs/agent-loop/spec.md`:
- Around line 1-75: Add an explicit English/Spanish translation status note to
the "Agent Loop Specification" (title string) indicating ES parity is pending if
no Spanish version exists; place a short status line near the top under the
Purpose or at the end of the Requirements and link to a tracking issue (e.g.,
“ES translation pending — see issue #<num>”) or add a TODO with a concise
sentence about pending Spanish translation so readers know parity is not yet
available.
- Around line 21-27: The spec currently requires predictable stream events
("start, progress, and completion events") but doesn't define canonical event
names or payload fields; update the Agent Loop spec (around the "loop MUST emit
predictable stream events" and "Scenario: Standard Iteration Events" sections
and the referenced event phase lines) to include a concrete event schema:
enumerate stable event type strings (e.g., "tool.start", "tool.progress",
"tool.complete", "loop.iteration.start", "loop.iteration.end"), required payload
fields (id, timestamp, source, trace_id, status, data/result, error), expected
semantics for each field, and a version/token for backward-compatibility; add
one compact JSON example per event and a note mandating that all entry points
(CLI/channel/gateway) must implement this schema to prevent contract drift.
- Around line 71-75: Update the approval requirement so approvals are bound to
the exact tool invocation to prevent TOCTOU: when the dispatcher (the code
handling the "tool dispatched by the model" path) requests and records explicit
user approval, require the approval to include a cryptographic or unforgeable
approval token that encodes the tool identifier, action name, serialized args
(or a stable fingerprint), session id, and an expiry timestamp; before executing
the tool the dispatcher must verify the approval token matches the current
tool+args+session and is unexpired and valid (signature check) and reject reuse
for any differing action or expired token; represent this as an Approval object
(fields: tool, action, argsFingerprint, sessionId, expiry, signature) and
enforce verification in the dispatcher flow that previously only checked
"explicit authorization".
- Line 13: The markdown headings like "#### Scenario: Unified Loop Execution"
(and the other headings at the same locations referenced by the reviewer) are
missing a blank line beneath them which triggers markdownlint MD022; fix by
inserting a single blank line after each affected heading (e.g., the "####
Scenario: Unified Loop Execution" heading and the headings at the other
mentioned locations) so every heading is followed by an empty line, then run
markdownlint to confirm MD022 is resolved.
In `@SONARQUBE_ISSUES.md`:
- Around line 1-4: The document titled "# SonarQube Issues - Proyecto Corvus" is
Spanish-only; add an English parity indicator by either (a) creating an English
companion section or file with equivalent content, or (b) inserting a clear
notice at the top such as "EN: English translation pending" and a link/path to
the forthcoming EN version; update the header or add a short bilingual line
under "## Resumen" to reflect the chosen approach so readers know parity
expectations.
- Around line 28-40: The CRITICAL table incorrectly lists dashboard/App.vue
under the "Rust - agent-runtime" section; update SONARQUBE_ISSUES.md to remove
dashboard/App.vue from the Rust table and place it under the correct
frontend/category (e.g., a "Frontend - Vue" or "Dashboard" section) so ownership
and language metadata are accurate, ensuring the table header and any row counts
are adjusted accordingly and any other entries referencing "dashboard/App.vue"
are corrected; locate the "Rust - agent-runtime" table and the row containing
dashboard/App.vue to perform the change.
- Around line 121-123: Resolve the contradiction between Phase 1 ("Eliminar
tokens hardcodeados en gateway/mod.rs") and the resolved false-positive status
reported in lines 14–23: update the Phase 1 entry or the BLOCKER status so there
is a single source of truth (either mark the gateway/mod.rs token issue as
resolved/false-positive to match lines 14–23, or remove the resolved note and
keep the remediation requirement). Reference the Phase 1 heading and the
gateway/mod.rs token finding when making the change, and ensure the document
consistently reflects the final decision (resolved false positive or required
fix) across the section.
- Around line 18-21: Fix the Markdown heading spacing issues by ensuring there
is a blank line both before and after each heading; specifically update the
headings "Issue 1: Token Bearer en gateway/mod.rs línea 2442 ✅ RESUELTO" and
"Issue 2: Token Bearer en gateway/mod.rs línea 2507 ✅ RESUELTO" to have a blank
line above and below them, and apply the same blank-line-before/after fix to the
other heading pairs reported in the comment so all markdownlint MD022 warnings
are resolved.
---
Outside diff comments:
In `@clients/agent-runtime/src/cron/store.rs`:
- Around line 35-50: The INSERT into cron_jobs inside with_connection currently
hardcodes delete_after_run = 0 in the SQL, which removes per-job or DB-default
cleanup behavior; change the INSERT in conn.execute (the cron_jobs INSERT used
by the store) to use a parameter for delete_after_run (e.g., ?N) or omit the
delete_after_run column so the DB default applies, and bind that parameter from
the calling code (or the function argument) instead of always writing 0.
In `@clients/agent-runtime/src/gateway/admin.rs`:
- Around line 310-374: Add focused regression tests exercising
collect_restart_required_gateway: create unit tests that build a baseline Config
and various AdminGatewayPatch inputs and assert the exact contents of the
returned/updated fields vector for changed vs unchanged cases. Cover each gate:
port, host (trim behavior), require_pairing, allow_public_bind,
pair_rate_limit_per_minute, webhook_rate_limit_per_minute,
trust_forwarded_headers, gateway.rate_limit_max_keys and idempotency_max_keys
(use gateway::utils::normalize_max_keys to assert normalized inputs that
should/shouldn't trigger restart), and idempotency_ttl_secs (include ttl==0
behavior that preserves existing TTL). Put tests beside the function (e.g., in a
#[cfg(test)] mod in the same file or in its unit test module) to create Config
and AdminGatewayPatch instances, call collect_restart_required_gateway, and
assert fields contains or does not contain the expected "gateway.*" strings for
each scenario.
In `@clients/agent-runtime/src/providers/mod.rs`:
- Around line 302-330: The match currently builds a new Vec<&str> each call
(provider_env_candidates) inside resolve_provider_credential; change
provider_env_candidates to a static slice type (e.g. &'static [&'static str])
and return slice literals (e.g. &["OPENAI_API_KEY"]) from each match arm instead
of vec![], so no per-call allocation is done; update any subsequent code that
iterates or indexes provider_env_candidates to accept a slice (it should already
work with &[&str]) and remove Vec-specific methods if present.
In `@clients/agent-runtime/src/tools/cron_add.rs`:
- Around line 45-46: The cron_add tool exposes the parameter delete_after_run in
its schema but silently ignores it in the shell job branch; update the shell
handling in cron_add.rs so that when job_kind == Shell (the shell branch around
the current shell handling code) you validate params and either return an
explicit error (e.g., a ToolResult/ToolError indicating delete_after_run is
unsupported for shell jobs) if delete_after_run is present, or remove
delete_after_run from the schema if shell jobs truly must not accept it; ensure
validation happens before executing or constructing the cron entry and surface a
clear structured error via the Tool trait return type rather than silently
dropping the field.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatComponents.kt`:
- Line 147: ChatComponents.kt defines Compose functions like ChatBubble using
PascalCase but lacks the file-level suppression used in ChatWorkspace.kt; add
the same `@file`:Suppress("FunctionNaming") directive at the top of
ChatComponents.kt so Compose functions (e.g., ChatBubble) are consistently
exempt from the detekt FunctionNaming rule, or alternatively apply a
project-wide detekt rule to ignore `@Composable` functions—prefer adding the
file-level suppression to mirror ChatWorkspace.kt for immediate consistency.
In `@clients/web/apps/dashboard/src/App.vue`:
- Around line 455-474: The parameter parsed in buildObservabilityPayload is
unused and should either be marked intentionally unused by restoring the
underscore prefix (_parsed) or actually used if logic was accidentally omitted;
update the function signature in buildObservabilityPayload to rename parsed to
_parsed to indicate it’s intentionally unused, or add the missing references to
parsed where needed (e.g., within conditionallyAdd calls or payload
construction) and remove the underscore if you depend on its values.
In `@clients/web/build.gradle.kts`:
- Around line 151-157: The zip task "${appName}DistZip" must fail fast if the
source dist directory is missing and must set reproducibility flags; update the
Zip task registered as tasks.register<Zip>("${appName}DistZip") to add a doFirst
block that checks that file("${appDir}/${config.distDir}") exists and isNotEmpty
(throw a GradleException with a clear message if not), and set reproducibility
properties on the Zip task (isReproducibleFileOrder = true and
isPreserveFileTimestamps = false or the Gradle-equivalent flags) so the produced
archive is deterministic; ensure the task still dependsOn(appBuild).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fa7046e-ed61-48c3-a9b2-e4a0f01607fa
⛔ Files ignored due to path filters (3)
clients/agent-runtime/Cargo.lockis excluded by!**/*.lockclients/agent-runtime/firmware/corvus-nucleo/Cargo.lockis excluded by!**/*.lockclients/web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (117)
.agents/journal/scribe-journal.md.agents/skills/accessibility/SKILL.md.agents/skills/accessibility/references/WCAG.md.agents/skills/best-practices/SKILL.md.agents/skills/core-web-vitals/SKILL.md.agents/skills/core-web-vitals/references/LCP.md.agents/skills/performance/SKILL.md.agents/skills/seo/SKILL.md.agents/skills/web-quality-audit/SKILL.md.agents/skills/web-quality-audit/scripts/analyze.sh.github/codecov.yml.github/workflows/pull-request-check.yml.github/workflows/sonarqube-analysis.ymlSONARQUBE_ISSUES.mdclients/agent-runtime/Cargo.tomlclients/agent-runtime/build.gradle.ktsclients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/npm/corvus-cli/package.jsonclients/agent-runtime/npm/corvus-darwin-arm64/package.jsonclients/agent-runtime/npm/corvus-darwin-x64/package.jsonclients/agent-runtime/npm/corvus-linux-arm64/package.jsonclients/agent-runtime/npm/corvus-linux-x64/package.jsonclients/agent-runtime/npm/corvus-windows-arm64/package.jsonclients/agent-runtime/npm/corvus-windows-x64/package.jsonclients/agent-runtime/npm/corvus/package.jsonclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/agent/dispatcher.rsclients/agent-runtime/src/agent/mission.rsclients/agent-runtime/src/agent/mod.rsclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/approval/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/channels/telegram.rsclients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/cron/mod.rsclients/agent-runtime/src/cron/store.rsclients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/gateway/admin.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/memory/snapshot.rsclients/agent-runtime/src/memory/sqlite.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/observability/mod.rsclients/agent-runtime/src/observability/otel.rsclients/agent-runtime/src/observability/prometheus.rsclients/agent-runtime/src/observability/traits.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/providers/anthropic.rsclients/agent-runtime/src/providers/compatible.rsclients/agent-runtime/src/providers/copilot.rsclients/agent-runtime/src/providers/mod.rsclients/agent-runtime/src/providers/reliable.rsclients/agent-runtime/src/providers/traits.rsclients/agent-runtime/src/security/mod.rsclients/agent-runtime/src/security/policy.rsclients/agent-runtime/src/tools/cron_add.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/tools/mcp/adapter.rsclients/agent-runtime/src/tools/mcp/client.rsclients/agent-runtime/src/tools/mcp/mod.rsclients/agent-runtime/src/tools/mcp/normalize.rsclients/agent-runtime/src/tools/mod.rsclients/agent-runtime/src/tools/traits.rsclients/agent-runtime/src/update/mod.rsclients/agent-runtime/tests/legacy_loop_guard.rsclients/agent-runtime/tests/mcp_config_validation.rsclients/agent-runtime/tests/mcp_execution_limits.rsclients/agent-runtime/tests/mcp_native_regression.rsclients/agent-runtime/tests/mcp_policy_approval_parity.rsclients/agent-runtime/tests/mcp_registry_integration.rsclients/agent-runtime/tests/mcp_runtime_e2e.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/ChatComponents.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.ktclients/web/apps/chat/package.jsonclients/web/apps/chat/vite.config.tsclients/web/apps/dashboard/package.jsonclients/web/apps/dashboard/src/App.vueclients/web/apps/dashboard/vite.config.jsclients/web/apps/dashboard/vite.config.tsclients/web/apps/docs/package.jsonclients/web/apps/docs/src/content/docs/en/clients/agent-runtime/architecture.mdclients/web/apps/docs/src/content/docs/en/guides/architecture/diagrams/container/system-containers.pumlclients/web/apps/docs/src/content/docs/en/guides/cli-reference.mdclients/web/apps/docs/src/content/docs/en/guides/configuration.mdclients/web/apps/docs/src/content/docs/en/guides/gpg-setup.mdclients/web/apps/docs/src/content/docs/es/guides/architecture/diagrams/container/system-containers.pumlclients/web/apps/docs/src/content/docs/es/guides/cli-reference.mdclients/web/apps/docs/src/content/docs/es/guides/configuration.mdclients/web/apps/docs/src/content/docs/es/guides/gpg-setup.mdclients/web/apps/marketing/package.jsonclients/web/apps/marketing/src/layouts/MarketingLayout.astroclients/web/apps/marketing/src/pages/index.astroclients/web/apps/marketing/src/styles/global.cssclients/web/build.gradle.ktsclients/web/package.jsonclients/web/packages/shared/package.jsonclients/web/pnpm-workspace.yamlgradle.propertiesgradle/build-logic/gradle.propertiesopenspec/changes/agent-runtime-mission-layer/design.mdopenspec/changes/agent-runtime-mission-layer/proposal.mdopenspec/changes/agent-runtime-mission-layer/specs/agent-loop/spec.mdopenspec/changes/agent-runtime-mission-layer/tasks.mdopenspec/changes/archive/2026-03-03-support-mcps-agent-runtime/design.mdopenspec/changes/archive/2026-03-03-support-mcps-agent-runtime/proposal.mdopenspec/changes/archive/2026-03-03-support-mcps-agent-runtime/specs/mcp-runtime/spec.mdopenspec/changes/archive/2026-03-03-support-mcps-agent-runtime/tasks.mdopenspec/changes/archive/2026-03-03-support-mcps-agent-runtime/verify-report.mdopenspec/changes/archive/2026-03-04-agent-runtime-mission-layer/verify-report.mdopenspec/specs/agent-loop/spec.md
💤 Files with no reviewable changes (52)
- .github/codecov.yml
- .agents/skills/accessibility/SKILL.md
- clients/web/apps/docs/src/content/docs/es/guides/configuration.md
- clients/agent-runtime/src/providers/compatible.rs
- clients/agent-runtime/src/providers/anthropic.rs
- .agents/skills/seo/SKILL.md
- .agents/skills/accessibility/references/WCAG.md
- clients/web/apps/docs/src/content/docs/en/clients/agent-runtime/architecture.md
- openspec/changes/archive/2026-03-03-support-mcps-agent-runtime/design.md
- openspec/changes/agent-runtime-mission-layer/design.md
- clients/agent-runtime/src/tools/mcp/normalize.rs
- clients/agent-runtime/src/observability/log.rs
- clients/agent-runtime/tests/mission_entrypoint_parity.rs
- clients/agent-runtime/tests/mcp_runtime_e2e.rs
- clients/agent-runtime/src/providers/traits.rs
- clients/agent-runtime/tests/mission_security_parity.rs
- clients/agent-runtime/tests/mcp_registry_integration.rs
- openspec/changes/agent-runtime-mission-layer/proposal.md
- clients/agent-runtime/tests/mission_governance_integration.rs
- clients/agent-runtime/tests/mcp_policy_approval_parity.rs
- clients/agent-runtime/src/security/policy.rs
- openspec/changes/archive/2026-03-03-support-mcps-agent-runtime/tasks.md
- clients/agent-runtime/tests/mcp_config_validation.rs
- clients/web/apps/chat/vite.config.ts
- .agents/skills/core-web-vitals/references/LCP.md
- clients/agent-runtime/src/tools/traits.rs
- clients/agent-runtime/src/tools/mcp/mod.rs
- clients/agent-runtime/tests/mcp_execution_limits.rs
- .agents/skills/best-practices/SKILL.md
- openspec/changes/agent-runtime-mission-layer/tasks.md
- openspec/changes/archive/2026-03-04-agent-runtime-mission-layer/verify-report.md
- clients/agent-runtime/tests/mission_config_toggle.rs
- .agents/skills/core-web-vitals/SKILL.md
- openspec/changes/agent-runtime-mission-layer/specs/agent-loop/spec.md
- openspec/changes/archive/2026-03-03-support-mcps-agent-runtime/verify-report.md
- clients/agent-runtime/tests/mcp_native_regression.rs
- clients/agent-runtime/tests/mission_lifecycle_integration.rs
- clients/agent-runtime/src/tools/mcp/client.rs
- .agents/journal/scribe-journal.md
- clients/agent-runtime/src/agent/mod.rs
- clients/web/apps/docs/src/content/docs/es/guides/cli-reference.md
- clients/agent-runtime/src/observability/traits.rs
- openspec/changes/archive/2026-03-03-support-mcps-agent-runtime/specs/mcp-runtime/spec.md
- .agents/skills/web-quality-audit/SKILL.md
- clients/agent-runtime/src/tools/mcp/adapter.rs
- clients/web/apps/docs/src/content/docs/en/guides/cli-reference.md
- clients/web/apps/docs/src/content/docs/en/guides/configuration.md
- clients/web/apps/dashboard/vite.config.ts
- .agents/skills/performance/SKILL.md
- clients/agent-runtime/src/agent/mission.rs
- .agents/skills/web-quality-audit/scripts/analyze.sh
- openspec/changes/archive/2026-03-03-support-mcps-agent-runtime/proposal.md
Comment on lines
+133
to
+170
| if has_cmd(&line_buf, b"ping") { | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"pong\"}}", id_str); | ||
| } else if has_cmd(&line_buf, b"capabilities") { | ||
| let _ = write!( | ||
| resp_buf, | ||
| "{{\"id\":\"{}\",\"ok\":true,\"result\":\"{{\\\"gpio\\\":[0,1,2,3,4,5,6,7,8,9,10,11,12,13],\\\"led_pin\\\":13}}\"}}", | ||
| id_str | ||
| ); | ||
| } else if has_cmd(&line_buf, b"gpio_read") { | ||
| let pin = parse_arg(&line_buf, b"pin").unwrap_or(-1); | ||
| if pin == LED_PIN as i32 { | ||
| // Output doesn't support read; return 0 (LED state not readable) | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"0\"}}", id_str); | ||
| } else if pin >= 0 && pin <= 13 { | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"0\"}}", id_str); | ||
| } else { | ||
| let _ = write!( | ||
| resp_buf, | ||
| "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid pin {}\"}}", | ||
| id_str, pin | ||
| ); | ||
| } | ||
| } else if has_cmd(&line_buf, b"gpio_write") { | ||
| let pin = parse_arg(&line_buf, b"pin").unwrap_or(-1); | ||
| let value = parse_arg(&line_buf, b"value").unwrap_or(0); | ||
| if pin == LED_PIN as i32 { | ||
| led.set_level(if value != 0 { Level::High } else { Level::Low }); | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | ||
| } else if pin >= 0 && pin <= 13 { | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | ||
| } else { | ||
| let _ = write!( | ||
| resp_buf, | ||
| "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid pin {}\"}}", | ||
| id_str, pin | ||
| ); | ||
| } | ||
| } else { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parse cmd once to avoid repeated full-buffer scans.
has_cmd(&line_buf, ...) is called multiple times in sequence. Extracting cmd once and matching on it would reduce repeated scans and simplify future command additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/firmware/corvus-nucleo/src/main.rs` around lines 133 -
170, The code repeatedly calls has_cmd(&line_buf, ...) which re-scans the
buffer; instead, extract the command once (e.g., let cmd =
get_cmd_from(&line_buf) or parse the "cmd" field once) and replace the if/else
chain with a single match on that cmd byte/slice, then handle "ping",
"capabilities", "gpio_read", and "gpio_write" branches using the same
parse_arg(&line_buf, ...) calls for pin/value and reusing id_str, LED_PIN and
led.set_level as before; this centralizes command parsing, avoids repeated
full-buffer scans, and makes adding commands easier.
Comment on lines
+155
to
+163
| } else if has_cmd(&line_buf, b"gpio_write") { | ||
| let pin = parse_arg(&line_buf, b"pin").unwrap_or(-1); | ||
| let value = parse_arg(&line_buf, b"value").unwrap_or(0); | ||
| if pin == LED_PIN as i32 { | ||
| led.set_level(if value != 0 { Level::High } else { Level::Low }); | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | ||
| } else if pin >= 0 && pin <= 13 { | ||
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | ||
| } else { |
Contributor
There was a problem hiding this comment.
Validate gpio_write.value strictly (0/1) instead of coercing.
At Line 157, missing/invalid value silently becomes 0, and Line 159 treats any non-zero as HIGH. This accepts out-of-contract inputs and can mask caller bugs. Return an explicit error unless value is exactly 0 or 1.
Suggested fix
} else if has_cmd(&line_buf, b"gpio_write") {
let pin = parse_arg(&line_buf, b"pin").unwrap_or(-1);
- let value = parse_arg(&line_buf, b"value").unwrap_or(0);
- if pin == LED_PIN as i32 {
- led.set_level(if value != 0 { Level::High } else { Level::Low });
+ let value = parse_arg(&line_buf, b"value");
+ if pin < 0 || pin > 13 {
+ let _ = write!(
+ resp_buf,
+ "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid pin {}\"}}",
+ id_str, pin
+ );
+ } else if !matches!(value, Some(0 | 1)) {
+ let _ = write!(
+ resp_buf,
+ "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid value\"}}",
+ id_str
+ );
+ } else if pin == LED_PIN as i32 {
+ let v = value.unwrap_or(0);
+ led.set_level(if v == 1 { Level::High } else { Level::Low });
let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str);
} else if pin >= 0 && pin <= 13 {
let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str);
- } else {
- let _ = write!(
- resp_buf,
- "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid pin {}\"}}",
- id_str, pin
- );
}As per coding guidelines: "**/*: Security first, performance second. Validate input boundaries..."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if has_cmd(&line_buf, b"gpio_write") { | |
| let pin = parse_arg(&line_buf, b"pin").unwrap_or(-1); | |
| let value = parse_arg(&line_buf, b"value").unwrap_or(0); | |
| if pin == LED_PIN as i32 { | |
| led.set_level(if value != 0 { Level::High } else { Level::Low }); | |
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | |
| } else if pin >= 0 && pin <= 13 { | |
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | |
| } else { | |
| } else if has_cmd(&line_buf, b"gpio_write") { | |
| let pin = parse_arg(&line_buf, b"pin").unwrap_or(-1); | |
| let value = parse_arg(&line_buf, b"value"); | |
| if pin < 0 || pin > 13 { | |
| let _ = write!( | |
| resp_buf, | |
| "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid pin {}\"}}", | |
| id_str, pin | |
| ); | |
| } else if !matches!(value, Some(0 | 1)) { | |
| let _ = write!( | |
| resp_buf, | |
| "{{\"id\":\"{}\",\"ok\":false,\"result\":\"\",\"error\":\"Invalid value\"}}", | |
| id_str | |
| ); | |
| } else if pin == LED_PIN as i32 { | |
| let v = value.unwrap_or(0); | |
| led.set_level(if v == 1 { Level::High } else { Level::Low }); | |
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | |
| } else if pin >= 0 && pin <= 13 { | |
| let _ = write!(resp_buf, "{{\"id\":\"{}\",\"ok\":true,\"result\":\"done\"}}", id_str); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/firmware/corvus-nucleo/src/main.rs` around lines 155 -
163, The gpio_write branch currently parses value with
parse_arg(...).unwrap_or(0) and treats any non-zero as HIGH; change this to
enforce value must be exactly 0 or 1: after calling parse_arg(&line_buf,
b"value") (no silent default), validate that it returned a numeric value and
that that value is either 0 or 1; if not, write an error response into resp_buf
(using the same id_str and an ok:false/error message) and return, otherwise use
the validated value to call led.set_level(if value == 1 { Level::High } else {
Level::Low }). Keep the existing pin checks (LED_PIN/0..=13) and ensure you
reference parse_arg, has_cmd, led.set_level, LED_PIN, resp_buf, and id_str when
making the change.
Comment on lines
+505
to
+507
| ConversationMessage::ToolResults(results) => results | ||
| .iter() | ||
| .any(|r| r.content.contains("approval required before executing")), |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make denial assertion less phrase-fragile.
This check is tightly coupled to one exact sentence, so minor wording updates can cause false test failures. Prefer asserting a broader stable signal (e.g., “approval required” + tool identifier) instead of the full phrase.
Suggested test assertion hardening
- ConversationMessage::ToolResults(results) => results
- .iter()
- .any(|r| r.content.contains("approval required before executing")),
+ ConversationMessage::ToolResults(results) => results.iter().any(|r| {
+ let content = r.content.to_ascii_lowercase();
+ content.contains("approval required")
+ && content.contains("nonexistent_tool")
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ConversationMessage::ToolResults(results) => results | |
| .iter() | |
| .any(|r| r.content.contains("approval required before executing")), | |
| ConversationMessage::ToolResults(results) => results.iter().any(|r| { | |
| let content = r.content.to_ascii_lowercase(); | |
| content.contains("approval required") | |
| && content.contains("nonexistent_tool") | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/agent/tests.rs` around lines 505 - 507, The
assertion in the test inspects ConversationMessage::ToolResults and checks for
the exact phrase "approval required before executing", which is fragile; update
the predicate used in the results.iter().any(...) to instead check for a broader
stable signal such as r.content.contains("approval required") &&
r.tool_id.contains("YOUR_TOOL_ID") (or whichever tool identifier is present in
the result structure) so the test verifies that an approval was required for the
specific tool without depending on the full sentence wording. Ensure you
reference ConversationMessage::ToolResults, results, and r.content (and
r.tool_id or equivalent field) when making the change.
Comment on lines
+823
to
+829
| if let Ok(resp) = send_message_with_fallback(&self.client, &api_url, body).await { | ||
| if index < chunks.len() - 1 { | ||
| tokio::time::sleep(Duration::from_millis(100)).await; | ||
| } | ||
| let _ = resp; | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate chunk send failures instead of silently continuing.
This block drops send_message_with_fallback errors and returns Ok(()) even when all sends fail, which breaks send reliability semantics.
Suggested fix
- if let Ok(resp) = send_message_with_fallback(&self.client, &api_url, body).await {
- if index < chunks.len() - 1 {
- tokio::time::sleep(Duration::from_millis(100)).await;
- }
- let _ = resp;
- continue;
- }
-
- if index < chunks.len() - 1 {
+ send_message_with_fallback(&self.client, &api_url, body)
+ .await
+ .with_context(|| {
+ format!(
+ "Telegram sendMessage failed for chunk {}/{}",
+ index + 1,
+ chunks.len()
+ )
+ })?;
+
+ if index + 1 < chunks.len() {
tokio::time::sleep(Duration::from_millis(100)).await;
}Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/channels/telegram.rs` around lines 823 - 829, The
current loop around send_message_with_fallback in the Telegram channel silently
ignores errors (it only proceeds on Ok and continues on Err), causing send to
return Ok(()) even if all chunk sends failed; update the send implementation
that calls send_message_with_fallback to propagate errors instead of swallowing
them—specifically, in the loop around send_message_with_fallback(&self.client,
&api_url, body).await, handle the Result by returning Err when
send_message_with_fallback returns Err (or collect and return the
first/aggregate error if multiple chunks may fail) and only continue to the next
chunk on success; ensure the final send method signature and semantics (in the
impl of Channel for Telegram) return an appropriate Err when any chunk fails and
preserve the small sleep between successful chunk sends.
Comment on lines
+1645
to
+1649
| let resp = match self.client.post(&url).json(&body).send().await { | ||
| Ok(r) => r, | ||
| Err(e) => { | ||
| tracing::warn!("Telegram poll error: {e}"); | ||
| tokio::time::sleep(std::time::Duration::from_secs(5)).await; |
Contributor
There was a problem hiding this comment.
Do not log raw poll errors that may expose sensitive request context.
Logging {e} directly on poll failures can leak sensitive transport/request details into logs. Use sanitized messages (and optional non-sensitive structured fields) instead.
Suggested fix
- Err(e) => {
- tracing::warn!("Telegram poll error: {e}");
+ Err(_e) => {
+ tracing::warn!("Telegram poll request failed; retrying in 5s");
tokio::time::sleep(std::time::Duration::from_secs(5)).await;
continue;
}
};
let data: serde_json::Value = match resp.json().await {
- Ok(d) => d,
- Err(e) => {
- tracing::warn!("Telegram parse error: {e}");
+ Ok(d) => d,
+ Err(_e) => {
+ tracing::warn!("Telegram getUpdates response parse failed; retrying in 5s");
tokio::time::sleep(std::time::Duration::from_secs(5)).await;
continue;
}
};As per coding guidelines: clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements.
Also applies to: 1654-1658
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/channels/telegram.rs` around lines 1645 - 1649,
Replace the raw error interpolation in the Telegram polling error logs (the
tracing::warn! calls invoked after
self.client.post(...).json(&body).send().await fails and the similar block at
the later occurrence) with a sanitized, non-sensitive message (e.g., "Telegram
poll failed, retrying") and, if you need structured details, record only
non-sensitive fields derived from the error (for example a boolean like
e.is_timeout() or an HTTP status from e.status() if present) rather than the
full error string or raw request/transport details; update the tracing::warn!
invocations to use that sanitized message and optional safe fields instead of
"{e}".
Comment on lines
+71
to
+75
| - GIVEN a tool dispatched by the model that requires elevated privileges | ||
| - WHEN the dispatcher intercepts the tool call request | ||
| - THEN the system MUST evaluate the action against the current session's risk classification and approval policy | ||
| - AND the system MUST block the execution and request explicit user approval if the action exceeds the permitted risk threshold | ||
| - AND the system MUST NOT proceed until explicit authorization is granted or the request is aborted. |
Contributor
There was a problem hiding this comment.
Bind approvals to an exact action to prevent authorization TOCTOU gaps.
Line 74 requires explicit approval but does not require binding approval to the exact tool/action+args and expiry. Reused or stale approvals could authorize unintended calls.
Proposed security requirement addition
- AND the system MUST block the execution and request explicit user approval if the action exceeds the permitted risk threshold
+- AND any approval token/record MUST be bound to the exact tool name, normalized arguments, session_id, and a short expiration window
- AND the system MUST NOT proceed until explicit authorization is granted or the request is aborted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/specs/agent-loop/spec.md` around lines 71 - 75, Update the approval
requirement so approvals are bound to the exact tool invocation to prevent
TOCTOU: when the dispatcher (the code handling the "tool dispatched by the
model" path) requests and records explicit user approval, require the approval
to include a cryptographic or unforgeable approval token that encodes the tool
identifier, action name, serialized args (or a stable fingerprint), session id,
and an expiry timestamp; before executing the tool the dispatcher must verify
the approval token matches the current tool+args+session and is unexpired and
valid (signature check) and reject reuse for any differing action or expired
token; represent this as an Approval object (fields: tool, action,
argsFingerprint, sessionId, expiry, signature) and enforce verification in the
dispatcher flow that previously only checked "explicit authorization".
Comment on lines
+1
to
+4
| # SonarQube Issues - Proyecto Corvus | ||
|
|
||
| ## Resumen | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add EN/ES parity note or English companion
This doc is ES-only. Add an English version or an explicit “translation pending” note so parity expectations are clear.
As per coding guidelines, "**/*.{md,mdx}: For user-facing docs, check EN/ES parity or explicitly note pending translation gaps."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SONARQUBE_ISSUES.md` around lines 1 - 4, The document titled "# SonarQube
Issues - Proyecto Corvus" is Spanish-only; add an English parity indicator by
either (a) creating an English companion section or file with equivalent
content, or (b) inserting a clear notice at the top such as "EN: English
translation pending" and a link/path to the forthcoming EN version; update the
header or add a short bilingual line under "## Resumen" to reflect the chosen
approach so readers know parity expectations.
Comment on lines
+18
to
+21
| ### Issue 1: Token Bearer en gateway/mod.rs línea 2442 ✅ RESUELTO | ||
| - **Status**: Marcado como FALSE POSITIVE (es un test de seguridad) | ||
|
|
||
| ### Issue 2: Token Bearer en gateway/mod.rs línea 2507 ✅ RESUELTO |
Contributor
There was a problem hiding this comment.
Resolve markdown heading spacing warnings (MD022)
Multiple headings are missing required blank lines before/after; this currently fails markdownlint hygiene and hurts readability in rendered docs.
As per coding guidelines, "**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes."
Also applies to: 76-77, 84-85, 87-88, 90-91, 97-98, 101-102, 109-110, 121-122, 124-125, 131-132, 136-137
🧰 Tools
🪛 LanguageTool
[grammar] ~18-~18: Cambia la palabra o signo.
Context: ... en gateway/mod.rs línea 2442 ✅ RESUELTO - Status: Marcado como FALSE POSITIVE (e...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_SPACE)
[grammar] ~21-~21: Cambia la palabra o signo.
Context: ... en gateway/mod.rs línea 2507 ✅ RESUELTO - Status: Marcado como FALSE POSITIVE (e...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_SPACE)
🪛 markdownlint-cli2 (0.21.0)
[warning] 18-18: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SONARQUBE_ISSUES.md` around lines 18 - 21, Fix the Markdown heading spacing
issues by ensuring there is a blank line both before and after each heading;
specifically update the headings "Issue 1: Token Bearer en gateway/mod.rs línea
2442 ✅ RESUELTO" and "Issue 2: Token Bearer en gateway/mod.rs línea 2507 ✅
RESUELTO" to have a blank line above and below them, and apply the same
blank-line-before/after fix to the other heading pairs reported in the comment
so all markdownlint MD022 warnings are resolved.
Comment on lines
+28
to
+40
| ### Rust - agent-runtime | ||
|
|
||
| | Archivo | Línea | Complejidad | Máximo | Key | | ||
| |---------|-------|-------------|--------|-----| | ||
| | onboard/wizard.rs | 2770 | 151 | 15 | AZye4oPnIngfSClMZ_ec | | ||
| | wizard.rs | 1469 | 97 | 15 | AZye4oPnIngfSClMZ_ea | | ||
| | channels/irc.rs | 399 | 91 | 15 | AZye4oJFIngfSClMZ_dt | | ||
| | main.rs | 564 | 63 | 15 | AZye4oPLIngfSClMZ_eY | | ||
| | config/schema.rs | 2204 | 65 | 15 | AZye4oO8IngfSClMZ_eX | | ||
| | channels/mod.rs | 412 | 59 | 15 | AZye4oKCIngfSClMZ_dz | | ||
| | memory/sqlite.rs | 480 | 54 | 15 | AZye4oIIIngfSClMZ_dl | | ||
| | dashboard/App.vue | 344 | 51 | 15 | AZye4oHdIngfSClMZ_df | | ||
| | providers/reliable.rs | 154 | 46 | 15 | AZye4oNkIngfSClMZ_eL | |
Contributor
There was a problem hiding this comment.
Correct language/category mismatch in CRITICAL table
dashboard/App.vue appears under “Rust - agent-runtime”. That makes the report technically inaccurate and can misroute ownership during triage.
As per coding guidelines, "**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SONARQUBE_ISSUES.md` around lines 28 - 40, The CRITICAL table incorrectly
lists dashboard/App.vue under the "Rust - agent-runtime" section; update
SONARQUBE_ISSUES.md to remove dashboard/App.vue from the Rust table and place it
under the correct frontend/category (e.g., a "Frontend - Vue" or "Dashboard"
section) so ownership and language metadata are accurate, ensuring the table
header and any row counts are adjusted accordingly and any other entries
referencing "dashboard/App.vue" are corrected; locate the "Rust - agent-runtime"
table and the row containing dashboard/App.vue to perform the change.
Comment on lines
+121
to
+123
| ### Fase 1: CRÍTICO - Security | ||
| 1. Eliminar tokens hardcodeados en gateway/mod.rs | ||
|
|
Contributor
There was a problem hiding this comment.
Fix Phase 1 contradiction with resolved BLOCKER status
Phase 1 says to remove hardcoded tokens, but lines 14–23 already classify those findings as resolved false positives (security tests). Keep one truth source here, or this plan will drive incorrect remediation work.
Suggested edit
-### Fase 1: CRÍTICO - Security
-1. Eliminar tokens hardcodeados en gateway/mod.rs
+### Fase 1: CRÍTICO - Security
+1. Verificar y documentar la justificación de FALSE POSITIVE en `gateway/mod.rs` (tests de seguridad).
+2. Revisar únicamente nuevos hallazgos CRITICAL/BLOCKER activos en SonarQube.As per coding guidelines, "**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Fase 1: CRÍTICO - Security | |
| 1. Eliminar tokens hardcodeados en gateway/mod.rs | |
| ### Fase 1: CRÍTICO - Security | |
| 1. Verificar y documentar la justificación de FALSE POSITIVE en `gateway/mod.rs` (tests de seguridad). | |
| 2. Revisar únicamente nuevos hallazgos CRITICAL/BLOCKER activos en SonarQube. | |
🧰 Tools
🪛 LanguageTool
[grammar] ~121-~121: Cambia la palabra o signo.
Context: ...e Acción ### Fase 1: CRÍTICO - Security 1. Eliminar tokens hardcodeados en gateway/...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_SPACE)
🪛 markdownlint-cli2 (0.21.0)
[warning] 121-121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SONARQUBE_ISSUES.md` around lines 121 - 123, Resolve the contradiction
between Phase 1 ("Eliminar tokens hardcodeados en gateway/mod.rs") and the
resolved false-positive status reported in lines 14–23: update the Phase 1 entry
or the BLOCKER status so there is a single source of truth (either mark the
gateway/mod.rs token issue as resolved/false-positive to match lines 14–23, or
remove the resolved note and keep the remediation requirement). Reference the
Phase 1 heading and the gateway/mod.rs token finding when making the change, and
ensure the document consistently reflects the final decision (resolved false
positive or required fix) across the section.
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.
Implemented runtime performance optimizations in the Compose Multiplatform application:
contentTypeto the chat message list. This allows the Compose runtime to more efficiently recycle item compositions of the same type (User vs Assistant bubbles), reducing rendering overhead during scrolling.PasswordVisualTransformation()in arememberblock. Previously, a new transformation object was allocated on every recomposition (e.g., every keystroke in the config fields), increasing GC pressure.Verified functional correctness with
./gradlew :composeApp:checkand formatting with./gradlew spotlessApply. Updated.agents/journal/bolt-journal.mdwith the new entry.PR created automatically by Jules for task 13297058189744760976 started by @yacosta738