fix: resolve all SonarCloud code smells and improve test coverage#351
Conversation
yacosta738
commented
Mar 28, 2026
yacosta738
commented
- Reduce cognitive complexity in mcp/mod.rs (104→<15) by extracting 5 helpers
- Reduce cognitive complexity in agent/tests.rs (16→<15) with helper function
- Reduce cognitive complexity in corvus-nucleo/main.rs (17→<15) with accumulate_byte
- Fix parameter count in ChatWorkspace.kt (8→5) via BridgeActions data class
- Replace typeof checks with direct undefined comparison in useChat.ts/useConfig.ts
- Add 59 new tests across detect.rs, log.rs, integrations/mod.rs, mcp/mod.rs
- Reduce cognitive complexity in mcp/mod.rs (104→<15) by extracting 5 helpers - Reduce cognitive complexity in agent/tests.rs (16→<15) with helper function - Reduce cognitive complexity in corvus-nucleo/main.rs (17→<15) with accumulate_byte - Fix parameter count in ChatWorkspace.kt (8→5) via BridgeActions data class - Replace typeof checks with direct undefined comparison in useChat.ts/useConfig.ts - Add 59 new tests across detect.rs, log.rs, integrations/mod.rs, mcp/mod.rs
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized MCP capability discovery and newline-buffering logic, consolidated bridge callbacks into a new Kotlin Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-28 to 2026-03-28 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt (1)
244-252: 🛠️ Refactor suggestion | 🟠 MajorPipeline failure:
ChatPanelexceeds Detekt'sLongParameterListthreshold.Detekt flags 6 parameters as exceeding the threshold. Consider consolidating into a state holder similar to the
BridgeActionspattern:♻️ Suggested refactor
+data class ChatPanelState( + val workspaceState: ChatWorkspaceState, + val bridgeState: MobileBridgeUiState, + val messages: List<ChatMessage>, + val query: String, +) + `@Composable` private fun ChatPanel( - state: ChatWorkspaceState, - bridgeState: MobileBridgeUiState, - messages: List<ChatMessage>, - query: String, + panelState: ChatPanelState, actions: ChatWorkspaceActions, modifier: Modifier = Modifier, ) {🤖 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/ChatWorkspace.kt` around lines 244 - 252, ChatPanel has too many parameters (exceeds Detekt LongParameterList); consolidate them into a single holder (e.g., ChatPanelProps or ChatPanelState) and/or an actions object (following the BridgeActions pattern) so ChatPanel signature becomes simpler. Create a data class that contains state: ChatWorkspaceState, bridgeState: MobileBridgeUiState, messages: List<ChatMessage>, query: String and actions: ChatWorkspaceActions (keep modifier separate or include if preferred), update ChatPanel to accept that holder and update all callers to construct and pass the new holder instead of individual args.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (1)
24-26:⚠️ Potential issue | 🟡 MinorPipeline failures: Detekt
FunctionNamingmisconfigured for Compose.The
FunctionNamingerrors on lines 26, 57, 76 are false positives. Jetpack Compose conventions require@Composablefunctions to use PascalCase. Configure Detekt to exclude composable functions or add a suppression:`@Suppress`("FunctionNaming") `@Composable` fun App(...) { ... }Alternatively, update
detekt.ymlto allow the pattern for composable functions.🤖 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/App.kt` around lines 24 - 26, Detekt is flagging PascalCase Compose functions (false positives) — add a suppression or adjust config: for the Composable functions such as App (and the other `@Composable` functions flagged on the same file), add `@Suppress`("FunctionNaming") directly above the `@Composable` annotation (e.g., annotate App with `@Suppress`("FunctionNaming") and do the same for the other flagged composables), or alternatively update the detekt.yml rule for FunctionNaming to exclude composable functions (match by annotation or name pattern) so PascalCase Compose functions are allowed.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.kt (1)
52-53: 🛠️ Refactor suggestion | 🟠 MajorPipeline failure:
ConfigSettingsListexceeds Detekt'sLongMethodthreshold (121 lines vs 60 max).Extract the
item { }contents into separate composables to reduce method length:♻️ Suggested extraction
`@Composable` private fun BridgeLinkingHeader(bridgeState: MobileBridgeUiState) { ... } `@Composable` private fun OnboardingStatusItem(bridgeState: MobileBridgeUiState) { ... } `@Composable` private fun ActionButtonsRow( onboardingState: MobileOnboardingState, bridgeState: MobileBridgeUiState, actions: ChatWorkspaceActions, ) { ... }Then call these from
ConfigSettingsList'sitem { }blocks.🤖 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/ConfigPanel.kt` around lines 52 - 53, ConfigSettingsList is too long; extract the large item { } blocks into smaller composables: create private `@Composable` functions BridgeLinkingHeader(bridgeState: MobileBridgeUiState), OnboardingStatusItem(bridgeState: MobileBridgeUiState) and ActionButtonsRow(onboardingState: MobileOnboardingState, bridgeState: MobileBridgeUiState, actions: ChatWorkspaceActions) and move the UI currently inside the corresponding item { } blocks into these functions, then replace each item { ... } body with a call to the new composable; ensure state and parameters referenced in the original item (e.g., bridgeState, onboardingState, actions) are forwarded to the new functions and imports remain correct.
🤖 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 223-231: The current accumulate_byte function can clear the buffer
on overflow but then accepts the remainder of the same physical frame, allowing
a truncated tail command to be processed; fix by changing accumulate_byte to
also accept a mutable overflow/discard flag (e.g., frame_discarding: &mut bool)
and on push error set *frame_discarding = true and clear the buffer, then while
*frame_discarding is true ignore all bytes (return false) until a terminator is
seen, at which point clear *frame_discarding and return false (do not treat the
terminated data as a valid line); update the caller(s) that handle terminators
(the code that checks accumulate_byte's return at the termination site) to pass
and maintain this discard flag so oversized frames are dropped completely.
In `@clients/agent-runtime/src/integrations/mod.rs`:
- Around line 272-341: Multiple unit tests repeatedly call show_integration_info
with different integration names and only assert is_ok(); replace them with a
single parameterized/table-driven test that iterates over a list of integration
names (e.g.,
["Telegram","Discord","Slack","OpenRouter","Ollama","iMessage","GitHub","Browser","Cron","Webhooks"]),
constructs Config::default(), calls show_integration_info(&config, name) for
each, and asserts result.is_ok(); update or remove the individual tests
(functions like show_integration_info_telegram_prints_setup, etc.) and keep one
consolidated test function (e.g., show_integration_info_all_prints_setup) to
reduce duplication while preserving the same assertions.
- Around line 373-379: The test integration_category_copy_and_clone currently
assigns let cloned = cat; which only copies the value again instead of
exercising the Clone implementation; update the test to call
IntegrationCategory::clone (e.g., let cloned = cat.clone();) so the Clone trait
is actually invoked and then assert_eq!(cat, cloned); ensure the test still
checks the Copy behavior with copied and the Clone behavior with cloned.
In `@clients/agent-runtime/src/observability/log.rs`:
- Around line 294-485: Create a small test helper to avoid repeating
LogObserver::new() by adding a function like fn make_log_observer() ->
LogObserver or a helper that accepts a closure (e.g., fn
with_log_observer<F:FnOnce(&LogObserver)>(f: F)) in the tests module and replace
the repeated let obs = LogObserver::new(); lines with calls to
make_log_observer() or with_log_observer(|obs| ...); update tests that reference
LogObserver::new() (e.g., log_observer_tool_call_start_no_panic,
log_observer_llm_response_success_no_panic, log_observer_image_ingress_no_panic,
log_observer_as_any_downcasts, etc.) to use the helper to reduce duplication and
centralize construction.
- Around line 293-500: Add a short note to the PR description or commit message
confirming that formatting and lint/test checks passed for this change: run and
include results (or note skips + reasons) for "cargo fmt --all -- --check",
"cargo clippy --all-targets -- -D warnings", and "cargo test" as they apply to
the changes around LogObserver (tests like
log_observer_extreme_duration_no_panic,
log_observer_on_image_ingress_delegates_to_record_event, etc.) so reviewers can
verify the clients/agent-runtime crate passed CI/local checks.
In `@clients/agent-runtime/src/security/detect.rs`:
- Around line 203-244: Update the three tests
(firejail_backend_falls_back_gracefully,
bubblewrap_backend_falls_back_gracefully, docker_backend_falls_back_gracefully)
to assert the actual sandbox backend outcome instead of only
sandbox.is_available(): call create_sandbox(&config) and then check if the probe
indicates unavailability (e.g., !sandbox.is_available() or a probe failure path)
assert that sandbox.name() == "none", otherwise assert sandbox.name() == the
expected backend string (e.g., "firejail", "bubblewrap", "docker"); use the
existing create_sandbox, SandboxBackend, sandbox.is_available() and
sandbox.name() symbols so the test fails if a wrong backend is selected. Ensure
same pattern is applied to similar tests under src/security/, src/gateway/, and
src/tools/ as these are high-risk surfaces.
- Around line 188-200: The test landlock_backend_falls_back_on_non_linux is not
platform-gated and can run on Linux with incorrect semantics; gate it with
#[cfg(not(target_os = "linux"))] (or rename to indicate non-linux) and change
the assertion to explicitly check the fallback backend by asserting
sandbox.name() == "none" (instead of just is_available()); locate the test
around fn landlock_backend_falls_back_on_non_linux and update the function
attribute and assertion accordingly so the test only runs on non-Linux targets
and verifies the noop fallback.
In `@clients/agent-runtime/src/tools/mcp/mod.rs`:
- Around line 615-628: The test
discover_server_prompts_adapter_creation_failure_skips_prompt currently uses two
valid prompts so it never exercises the McpPromptAdapter::from_manifest failure
path; either rename the test to reflect the all-success scenario or modify the
mock payload so one manifest is invalid (e.g., give one prompt malformed args or
a missing required field that McpPromptAdapter::from_manifest will reject) and
then assert discover_capabilities(&config) yields only the good prompt
(assert_eq!(result.len(), 1)). Locate the test function and adjust the payload
passed to mock_server (or change the assertion/name) and ensure the failing
manifest triggers McpPromptAdapter::from_manifest rejection so the failure
branch is covered.
- Around line 147-180: The code currently inserts canonical into seen_names
before building the adapter, which reserves the ID even when
McpPromptAdapter::from_manifest fails; instead, check for collisions using
seen_names.contains(&canonical) and bail with
collision_error_message(&canonical) if present, but defer inserting into
seen_names until after prompt_adapter::McpPromptAdapter::from_manifest(...)
succeeds; then insert the canonical and push the adapter (tools.push(...)) so
IDs are only reserved for successfully created adapters.
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 85-117: BridgeActions is being reconstructed on every
recomposition which breaks identity-based memoization in ChatWorkspace; fix by
memoizing the BridgeActions instance with remember while ensuring lambdas use
the latest values via rememberUpdatedState: create remembered references (e.g.,
currentBridgeSnapshot = rememberUpdatedState(bridgeSnapshot) and
currentOnBridgeSnapshotChange = rememberUpdatedState(onBridgeSnapshotChange))
and then wrap BridgeActions(...) in a remember { ... } so its object identity is
stable but its lambdas read currentBridgeSnapshot.value and invoke
currentOnBridgeSnapshotChange.value to apply updates.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt`:
- Around line 155-163: The problem is that bridgeActions passed into
ChatWorkspace is recreated every recomposition because BridgeActions is
constructed inline in App.kt and its lambdas capture bridgeSnapshot, so
remember(bridgeActions) in ChatWorkspace is ineffective; fix by stabilizing the
captured values in App.kt using rememberUpdatedState for bridgeSnapshot and any
callbacks (e.g., onBridgeSnapshotChange/onRetryBridge), then wrap the
BridgeActions construction in remember so BridgeActions itself is stable across
recompositions (e.g., use remember { BridgeActions(...) } but have the lambdas
reference the rememberUpdatedState-backed values); this will make the
remember(bridgeActions) in ChatWorkspace work as intended.
---
Outside diff comments:
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 24-26: Detekt is flagging PascalCase Compose functions (false
positives) — add a suppression or adjust config: for the Composable functions
such as App (and the other `@Composable` functions flagged on the same file), add
`@Suppress`("FunctionNaming") directly above the `@Composable` annotation (e.g.,
annotate App with `@Suppress`("FunctionNaming") and do the same for the other
flagged composables), or alternatively update the detekt.yml rule for
FunctionNaming to exclude composable functions (match by annotation or name
pattern) so PascalCase Compose functions are allowed.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt`:
- Around line 244-252: ChatPanel has too many parameters (exceeds Detekt
LongParameterList); consolidate them into a single holder (e.g., ChatPanelProps
or ChatPanelState) and/or an actions object (following the BridgeActions
pattern) so ChatPanel signature becomes simpler. Create a data class that
contains state: ChatWorkspaceState, bridgeState: MobileBridgeUiState, messages:
List<ChatMessage>, query: String and actions: ChatWorkspaceActions (keep
modifier separate or include if preferred), update ChatPanel to accept that
holder and update all callers to construct and pass the new holder instead of
individual args.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.kt`:
- Around line 52-53: ConfigSettingsList is too long; extract the large item { }
blocks into smaller composables: create private `@Composable` functions
BridgeLinkingHeader(bridgeState: MobileBridgeUiState),
OnboardingStatusItem(bridgeState: MobileBridgeUiState) and
ActionButtonsRow(onboardingState: MobileOnboardingState, bridgeState:
MobileBridgeUiState, actions: ChatWorkspaceActions) and move the UI currently
inside the corresponding item { } blocks into these functions, then replace each
item { ... } body with a call to the new composable; ensure state and parameters
referenced in the original item (e.g., bridgeState, onboardingState, actions)
are forwarded to the new functions and imports remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6fc2d3c-0b84-44b6-95be-f109cc66a781
📒 Files selected for processing (11)
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.ktclients/web/apps/chat/src/composables/useChat.tsclients/web/apps/dashboard/src/composables/useConfig.ts
📜 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). (3)
- GitHub Check: pr-checks
- GitHub Check: sonar
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (8)
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/web/apps/dashboard/src/composables/useConfig.tsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.ktclients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/web/apps/chat/src/composables/useChat.tsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/tools/mcp/mod.rsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Enforce null safety (no !!), structured concurrency, and non-blocking suspend code.
Prefer idiomatic Kotlin (expression bodies, sealed types, value classes when justified).
Verify tests follow TDD intent and use backtick test names where applicable.
Files:
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/tools/mcp/mod.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/tools/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Inspect existing module and adjacent tests before editing; define scope boundary with one concern per PR and avoid mixed feature+refactor+infra patches
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rsclients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/observability/log.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/providers/**/*.rs : Implement `Provider` trait in `src/providers/` and register in `src/providers/mod.rs` factory when adding a new provider
Applied to files:
clients/agent-runtime/src/integrations/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/agent-runtime/src/integrations/mod.rs
🪛 GitHub Actions: Scan with Detekt
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.kt
[error] 53-53: detekt [LongMethod]: The function ConfigSettingsList is too long (121). The maximum length is 60.
[error] 29-29: detekt [FunctionNaming]: Function names should match the pattern: [a-z][a-zA-Z0-9]*.
[error] 53-53: detekt [FunctionNaming]: Function names should match the pattern: [a-z][a-zA-Z0-9]*.
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
[error] 26-26: detekt [FunctionNaming]: Function names should match the pattern: [a-z][a-zA-Z0-9]*.
[error] 57-57: detekt [FunctionNaming]: Function names should match the pattern: [a-z][a-zA-Z0-9]*.
[error] 76-76: detekt [FunctionNaming]: Function names should match the pattern: [a-z][a-zA-Z0-9]*.
[error] 144-144: detekt [MagicNumber]: This expression contains a magic number. Consider defining it to a well named constant.
[error] 144-144: detekt [MagicNumber]: This expression contains a magic number. Consider defining it to a well named constant.
[error] 144-144: detekt [MagicNumber]: This expression contains a magic number. Consider defining it to a well named constant.
[error] 144-144: detekt [MagicNumber]: This expression contains a magic number. Consider defining it to a well named constant.
[error] 144-144: detekt [MagicNumber]: This expression contains a magic number. Consider defining it to a well named constant.
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt
[error] 245-245: detekt [LongParameterList]: The function ChatPanel(...) has too many parameters. The current threshold is set to 6.
🔇 Additional comments (9)
clients/agent-runtime/src/security/detect.rs (1)
158-185: Good deny-by-default coverage for explicit disable/none backend.These tests correctly lock in that
enabled: Some(false)andbackend: Noneboth force noop sandbox behavior.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.clients/agent-runtime/src/integrations/mod.rs (1)
175-176: Provide evidence that Rust validation checks passedNo outputs from
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, orcargo testare included. Per guidelines forclients/agent-runtime/**/*.rs, either include command outputs showing all checks passed or document which were skipped and why.clients/agent-runtime/src/observability/log.rs (1)
294-499: Strong coverage expansion for observer event paths.These tests substantially improve confidence across mission/image/LLM/overflow branches and delegation/downcast behavior. Nice improvement in regression safety.
clients/web/apps/dashboard/src/composables/useConfig.ts (1)
611-611: This change is safe and appropriate for a browser-only composable.The guard at line 611 checks for window availability before accessing browser APIs (location.hash, URLSearchParams, history). Since
useConfigis used exclusively in the dashboard web application—a client-side composable with no SSR usage—bothtypeof globalThis.window === "undefined"andglobalThis.window === undefinedbehave identically. Thetypeofoperator adds unnecessary defensiveness here sinceglobalThisalways exists and the code is browser-only.clients/web/apps/chat/src/composables/useChat.ts (1)
108-108: No actionable changes needed. Bothtypeof globalThis.window === "undefined"andglobalThis.window === undefinedare functionally equivalent. The direct comparison is idiomatic JavaScript and safe across all environments—in Node.js,globalThis.windowis simplyundefined, not a thrown error. The guard clauses and try-catch blocks (lines 112-116, 124-128, 136-140) provide proper protection against sessionStorage access failures. This is a safe simplification.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt (2)
83-89: LGTM!The
@Stableannotation correctly signals to Compose that equality implies unchanged semantics, enabling recomposition skipping. Good consolidation of bridge callbacks.
91-97: LGTM!Clean separation of bridge-specific actions into a nested
BridgeActionsfield while keeping workspace-level callbacks at the top level.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.kt (1)
103-154: LGTM!All bridge action callbacks correctly routed through
actions.bridge.*to match the refactoredChatWorkspaceActionsstructure. The button logic and conditional rendering remain unchanged.clients/agent-runtime/src/agent/tests.rs (1)
418-437: Nice simplification.The helper keeps the MCP denial test focused while still asserting the structured payload contract on
codeandtool.Also applies to: 625-627
- Fix accumulate_byte overflow vulnerability with discard flag (critical) - Defer prompt ID reservation until after adapter creation in MCP discovery - Memoize BridgeActions with rememberUpdatedState to prevent recomposition churn - Use cat.clone() to actually test Clone trait in integrations test - Platform-gate Landlock fallback test and verify sandbox backend names - Rename misleading prompt adapter test to match actual behavior
Deploying corvus with
|
| Latest commit: |
bfeb044
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4bbc426a.corvus-42x.pages.dev |
| Branch Preview URL: | https://fix-sonarcloud-quality-gate.corvus-42x.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (1)
27-27:⚠️ Potential issue | 🟠 MajorUnblock Detekt by reconciling
FunctionNamingwith Compose conventions.Detekt fails on Line 27, Line 58, and Line 77. These PascalCase
@Composablenames are idiomatic for UI composables, but your current rule requires lowerCamelCase and blocks CI.♻️ Proposed targeted fix in this file
-@Composable +@Suppress("FunctionNaming") +@Composable `@Preview` fun App(platformOverride: Platform? = null, initialBridgeSnapshot: MobileBridgeSnapshot? = null) { -@Composable +@Suppress("FunctionNaming") +@Composable private fun AppOnboardingContent( -@Composable +@Suppress("FunctionNaming") +@Composable private fun AppChatContent(As per coding guidelines, "Prefer idiomatic Kotlin (expression bodies, sealed types, value classes when justified)."
Also applies to: 58-58, 77-77
🤖 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/App.kt` at line 27, Detekt flags PascalCase composable function names; fix by adding a suppression for the FunctionNaming rule to the Compose functions in this file—annotate the top-level App function and the two other PascalCase `@Composable` functions (the ones at the other reported locations) with `@Suppress`("FunctionNaming") so you keep idiomatic Compose names while satisfying Detekt; apply the annotation immediately above each function declaration (preserve existing annotations like `@Composable`) to limit scope to these functions only.
♻️ Duplicate comments (1)
clients/agent-runtime/src/integrations/mod.rs (1)
272-341: 🧹 Nitpick | 🔵 TrivialConsolidate known-integration success tests into one table-driven test.
These tests are structurally identical (
Config::default()+assert!(result.is_ok())) and add maintenance noise.Refactor sketch
-#[test] -fn show_integration_info_telegram_prints_setup() { - let config = Config::default(); - let result = show_integration_info(&config, "Telegram"); - assert!(result.is_ok()); -} -// ... similar tests for Discord/Slack/OpenRouter/Ollama/iMessage/GitHub/Browser/Cron/Webhooks +#[test] +fn show_integration_info_known_integrations_return_ok() { + let config = Config::default(); + let names = [ + "Telegram", "Discord", "Slack", "OpenRouter", "Ollama", + "iMessage", "GitHub", "Browser", "Cron", "Webhooks", + ]; + for name in names { + assert!( + show_integration_info(&config, name).is_ok(), + "expected integration {name} to resolve" + ); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/integrations/mod.rs` around lines 272 - 341, Replace the many duplicate tests that each call show_integration_info(&Config::default(), "<Name>") and assert!(result.is_ok()) with a single table-driven test: create a Vec (or array) of integration names (e.g., "Telegram", "Discord", "Slack", "OpenRouter", "Ollama", "iMessage", "GitHub", "Browser", "Cron", "Webhooks"), iterate over them in one #[test] function, call show_integration_info(&Config::default(), name) for each, and assert!(result.is_ok()); remove the individual test functions (e.g., show_integration_info_telegram_prints_setup, show_integration_info_discord_prints_setup, etc.) so behavior is preserved but duplication is eliminated.
🤖 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 219-243: Add a unit test that exercises accumulate_byte to ensure
an oversized frame sets discarding and drops bytes until a terminator, then
accepts the next command: instantiate a heapless::Vec<u8, N> (use a small N in
the test to force overflow quickly), call accumulate_byte repeatedly with bytes
that exceed capacity (assert discarding becomes true and line_buf is empty),
continue feeding bytes including a terminator (assert discarding becomes false
and buffer still empty), then feed a valid command terminated by '\n' or '\r'
(assert accumulate_byte returns true and line_buf contains the expected
command); reference the accumulate_byte function, the discarding flag, and the
line_buf buffer when adding the test.
In `@clients/agent-runtime/src/integrations/mod.rs`:
- Around line 273-349: The tests named like
show_integration_info_telegram_prints_setup currently only assert that
show_integration_info(&config, "<Integration>") returns Ok(()), so they don't
verify printed guidance; update each test (or a subset) to capture and assert
the printed output contains the expected setup/help text for the integration
(for example by capturing stdout/stderr around show_integration_info)
referencing the show_integration_info function and the test functions (e.g.,
show_integration_info_telegram_prints_setup,
show_integration_info_discord_prints_setup, etc.); alternatively, if you
intentionally only want to assert success, rename the tests to
show_integration_info_<integration>_returns_ok to reflect the actual contract.
Ensure the chosen approach is applied consistently across all listed tests
(Telegram, Discord, Slack, OpenRouter, Ollama, iMessage, GitHub, Browser, Cron,
Webhooks, Spotify).
- Around line 344-349: The test
show_integration_info_coming_soon_prints_track_message currently hard-codes
"Spotify"; instead call the integration registry/status function to locate an
integration whose status is ComingSoon and use that integration's name when
calling show_integration_info. Concretely: inside
show_integration_info_coming_soon_prints_track_message, replace the literal
"Spotify" with code that queries the registry/status (e.g., the function that
returns the list of integrations and their statuses), find an entry where status
== ComingSoon, take its name, and pass that to show_integration_info(&config,
name); also handle the case where no ComingSoon entry exists by failing the test
with a clear assertion or skipping. Ensure you reference show_integration_info
and Config::default in the updated test.
- Around line 227-388: Run the required verification commands locally and/or in
CI and attach their outputs to the PR: run cargo fmt --all -- --check, cargo
clippy --all-targets -- -D warnings, and cargo test; if they pass, paste the
terminal output or CI job log snippets into the PR description (or link the CI
run), and if any were intentionally skipped document which check was skipped and
why (include the exact command, reason, and a plan to address failures). Ensure
to mention the repository/branch/commit SHA and reference the test files (e.g.,
mod.rs integration tests) so reviewers can correlate the results.
In `@clients/agent-runtime/src/tools/mcp/mod.rs`:
- Around line 636-657: Update these tests to assert the warning was actually
emitted: mark each test with the tracing_test::traced_test attribute (or
otherwise install a tracing subscriber that captures logs) and after calling
warn_unadvertised_capability(&server, "...") assert that tracing_test::logs()
(or your capture buffer) contains the expected warning message/fields about the
unadvertised capability. Reference the existing tests
warn_unadvertised_capability_fires_for_mock_server_with_resources and
warn_unadvertised_capability_fires_for_mock_server_with_prompts, the mock_server
helper, and the warn_unadvertised_capability function when adding the
traced_test attribute and assertions.
- Around line 67-79: The warning for failed normalization when calling
adapter::McpToolAdapter::from_manifest currently logs only server and the
redacted error; update the tracing::warn call inside the manifests loop to
include the failing manifest's identifying fields (e.g., manifest.name and, when
present, manifest.uri for resource manifests) and change the log message to
reflect the actual failure (e.g., "MCP tool manifest failed to normalize;
skipping manifest"). Do the same update for the equivalent failure-handling
sites in the other normalization loops (the blocks analogous to lines 105-121
and 161-173) so each tracing::warn includes manifest.name and manifest.uri plus
the redacted error for clear context.
- Around line 54-64: The code currently swallows failures from
client.list_tools(), client.list_resources(), and client.list_prompts() by
logging a warning and returning Ok(()), which breaks the discover_tools()
contract expected by its caller; change those Err branches so they propagate the
error instead of converting it to success (either return Err(error.into()) or
map to the discover_tools() error type used in this module), or convert the
function to return a structured partial-result/error type and return that
structured error; update the match arms around client.list_tools(),
client.list_resources(), and client.list_prompts() (the Err => { ... } arms) to
return the propagated error rather than Ok(()) so callers can distinguish
discovery failure from “no MCP capabilities.”
- Around line 752-763: The test
redact_error_message_replaces_sensitive_env_values mutates global env vars
without synchronization; wrap the set_var/remove_var sequence in the same global
test mutex pattern used elsewhere (define a static LOCK: OnceLock<Mutex<()>> and
lock it for the duration of the test) or replace the manual set/remove with a
scoped env helper that restores the previous value on drop; specifically modify
the redact_error_message_replaces_sensitive_env_values test to acquire the LOCK
before calling std::env::set_var and hold it until after std::env::remove_var
(or use the scoped helper) so the process-global environment changes are
serialized like other tests.
---
Outside diff comments:
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Line 27: Detekt flags PascalCase composable function names; fix by adding a
suppression for the FunctionNaming rule to the Compose functions in this
file—annotate the top-level App function and the two other PascalCase
`@Composable` functions (the ones at the other reported locations) with
`@Suppress`("FunctionNaming") so you keep idiomatic Compose names while satisfying
Detekt; apply the annotation immediately above each function declaration
(preserve existing annotations like `@Composable`) to limit scope to these
functions only.
---
Duplicate comments:
In `@clients/agent-runtime/src/integrations/mod.rs`:
- Around line 272-341: Replace the many duplicate tests that each call
show_integration_info(&Config::default(), "<Name>") and assert!(result.is_ok())
with a single table-driven test: create a Vec (or array) of integration names
(e.g., "Telegram", "Discord", "Slack", "OpenRouter", "Ollama", "iMessage",
"GitHub", "Browser", "Cron", "Webhooks"), iterate over them in one #[test]
function, call show_integration_info(&Config::default(), name) for each, and
assert!(result.is_ok()); remove the individual test functions (e.g.,
show_integration_info_telegram_prints_setup,
show_integration_info_discord_prints_setup, etc.) so behavior is preserved but
duplication is eliminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68c756fb-c220-44d1-b166-7076fc066f42
📒 Files selected for processing (5)
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
📜 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). (3)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (8)
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Enforce null safety (no !!), structured concurrency, and non-blocking suspend code.
Prefer idiomatic Kotlin (expression bodies, sealed types, value classes when justified).
Verify tests follow TDD intent and use backtick test names where applicable.
Files:
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/tools/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
🧠 Learnings (11)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/firmware/corvus-nucleo/src/main.rsclients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/providers/**/*.rs : Implement `Provider` trait in `src/providers/` and register in `src/providers/mod.rs` factory when adding a new provider
Applied to files:
clients/agent-runtime/src/integrations/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/security/detect.rsclients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Include threat/risk notes and rollback strategy for security, runtime, and gateway changes; add or update tests for boundary checks and failure modes
Applied to files:
clients/agent-runtime/src/security/detect.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions
Applied to files:
clients/agent-runtime/src/security/detect.rs
🪛 GitHub Actions: Scan with Detekt
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
[error] 27-27: detekt: Function name does not match pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
[error] 58-58: detekt: Function name does not match pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
[error] 77-77: detekt: Function name does not match pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]
🔇 Additional comments (6)
clients/agent-runtime/src/security/detect.rs (3)
187-201: Good platform-gated Landlock fallback test.This now correctly scopes non-Linux behavior and validates explicit fallback backend selection (
"none"), which tightens the security contract.
203-257: Fallback backend assertions are now robust.Checking concrete backend names (
requestedor"none") is a solid improvement over availability-only assertions and better guards against selection regressions.
259-285: Useful coverage for auto/default sandbox behavior.These tests add good safety nets for default config and auto-detection invariants (
is_available+ non-empty backend name).clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (1)
82-115:BridgeActionsstabilization looks correct.Using
rememberUpdatedState+rememberhere keepsBridgeActionsidentity stable while still applying updates with the latest snapshot/callback values. This matchesChatWorkspace’sbridgeActions: BridgeActionscontract and avoids recomposition churn.Also applies to: 121-121
clients/agent-runtime/firmware/corvus-nucleo/src/main.rs (1)
259-267: Main-loop integration looks good.Keeping
discardingalongsideline_bufpreserves parser state across bytes without adding extra allocation or cloning in the hot path.clients/agent-runtime/src/integrations/mod.rs (1)
373-380: Nice fix: Clone is now explicitly exercised.Using
cat.clone()here correctly validates Clone behavior (separate from Copy).
… in test - Add @file:Suppress("FunctionNaming") to App.kt for Compose PascalCase convention - Wrap env::set_var/remove_var in mutex + unsafe block for thread-safe test execution
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (1)
149-154:⚠️ Potential issue | 🟡 MinorExtract UUID segment lengths to a named constant to fix Detekt MagicNumber violation.
The numbers
8, 4, 4, 4, 12represent UUID segment lengths and should be defined as a private constant:Proposed fix
+private val UUID_SEGMENT_LENGTHS = listOf(8, 4, 4, 4, 12) + private fun generateSessionId(): String = - listOf(8, 4, 4, 4, 12).joinToString("-") { segmentLength -> + UUID_SEGMENT_LENGTHS.joinToString("-") { segmentLength -> buildString(segmentLength) { repeat(segmentLength) { append(HEX_DIGITS[Random.nextInt(HEX_DIGITS.length)]) } } }
sessionIdis used for UI state tracking and debug logging, not authentication or authorization. Non-cryptographicRandomis appropriate for this use case.🤖 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/App.kt` around lines 149 - 154, Extract the magic numbers in generateSessionId into a private constant (e.g., UUID_SEGMENT_LENGTHS = listOf(8, 4, 4, 4, 12)) and use that constant in the generateSessionId function instead of the inline list; update any imports/usages if necessary and keep HEX_DIGITS and Random usage unchanged. This removes the Detekt MagicNumber violation while preserving behavior of generateSessionId and its references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/tools/mcp/mod.rs`:
- Around line 147-159: Add a one-line comment explaining the intent of the
normalization check: that normalize::normalize_prompt_name(&server.name,
&manifest.name) is called only to validate the prompt name before creating the
adapter and the resulting value is intentionally unused (stored in _canonical).
Place the comment above the let _canonical = match ... block and reference
server and manifest to clarify this is a guard/validation step prior to adapter
building.
- Around line 751-769: The test
redact_error_message_replaces_sensitive_env_values currently sets and removes an
env var manually which can leak if the test panics; create a small RAII guard
(e.g., TempEnv or EnvVarGuard) that, in its constructor, sets the env var and in
Drop removes it, then replace the unsafe std::env::set_var/remove_var calls with
instantiation of that guard inside the ENV_LOCK critical section; keep using
ENV_LOCK and reference the test function name and redact_error_message so
reviewers can locate the change.
---
Outside diff comments:
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 149-154: Extract the magic numbers in generateSessionId into a
private constant (e.g., UUID_SEGMENT_LENGTHS = listOf(8, 4, 4, 4, 12)) and use
that constant in the generateSessionId function instead of the inline list;
update any imports/usages if necessary and keep HEX_DIGITS and Random usage
unchanged. This removes the Detekt MagicNumber violation while preserving
behavior of generateSessionId and its references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3bef312-fc78-439a-939a-163e836a0394
📒 Files selected for processing (2)
clients/agent-runtime/src/tools/mcp/mod.rsclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
📜 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). (3)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (8)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Enforce null safety (no !!), structured concurrency, and non-blocking suspend code.
Prefer idiomatic Kotlin (expression bodies, sealed types, value classes when justified).
Verify tests follow TDD intent and use backtick test names where applicable.
Files:
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/tools/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Tooltrait insrc/tools/with strict parameter schema, validate and sanitize all inputs, and return structuredToolResultwithout panics in runtime path
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/tools/mcp/mod.rs
🧠 Learnings (6)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/tools/mcp/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Applied to files:
clients/agent-runtime/src/tools/mcp/mod.rs
🪛 GitHub Actions: Scan with Detekt
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
[error] 150-150: detekt: This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
🔇 Additional comments (10)
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (3)
1-2: LGTM – Appropriate suppression for Compose naming conventions.PascalCase for
@Composablefunctions is idiomatic; file-level suppression avoids repetitive per-function annotations.
84-117: Correctly addresses recomposition instability.
rememberUpdatedState+rememberensures stableBridgeActionsidentity while lambdas read the latestbridgeSnapshot/onBridgeSnapshotChangeon each invocation.
119-124: LGTM – Call site matchesChatWorkspaceAPI contract.clients/agent-runtime/src/tools/mcp/mod.rs (7)
54-64: List-failure-to-Ok() contract concern remains unaddressed.This branch converts
client.list_tools()errors toOk(()), preventing callers from distinguishing "server unreachable" from "server has no tools." The caller atclients/agent-runtime/src/tools/mod.rs:220-260relies onErrto log discovery failures distinctly. Same pattern at lines 92-102 and 133-143.
71-79: Adapter-failure warning still lacks manifest identifier.When
from_manifestfails, the warning logs onlyserverand the redacted error—on a server with many tools this makes triage difficult. Includetool = %manifest.name(and similarly for resources at 114-118 and prompts at 167-170).
636-657: Tests still don't assert the warning is emitted.Both
warn_unadvertised_capability_fires_for_*tests verify no panic but not that thetracing::warn!actually fired. Usetracing_test::traced_test(or capture logs manually) and assert the warning message/fields so removing the warning would fail the test.
35-46: LGTM: Clean collision-check helper.Centralizing the insert-then-push pattern eliminates duplicate code and ensures collision detection happens consistently after adapter creation succeeds.
181-196: LGTM: Mock-only diagnostic helper.Early-return guard for non-mock servers keeps production code paths clean. JSON parse failure is silently ignored, which is appropriate for a diagnostic-only helper.
709-747: LGTM: Collision-check unit test is thorough.Inline
FakeToolstruct is appropriate for isolated testing ofregister_with_collision_check. The test correctly verifies both the success path and duplicate-rejection path.
528-632: Good coverage for failure-isolation paths.Tests for disabled config/server, list failures, and adapter failures exercise the warn-and-continue branches. This matches the PR objective of improving test coverage.
- Add EnvVarGuard RAII struct for panic-safe env var cleanup in tests - Add clarifying comment on prompt name validation before adapter creation - Extract UUID_SEGMENT_LENGTHS constant to fix Detekt MagicNumber violation
|
1 participant
