fix: address SonarCloud quality gate issues#789
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces a dispatcher-based streaming path in the agent gateway, refactors client-side streaming parsing, hardens skill path handling and recursive copy safety, improves a Dream test for transient lock contention, and updates release/test tooling and a Docker builder step. ChangesAgent Runtime — Streaming Dispatcher
Agent Runtime — Skills & Memory Safety
Web Dashboard — Streaming Pipeline Refactor
Release Tooling, Tests & Docker
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant Gateway as Agent Gateway
participant Dispatcher as Webhook Dispatcher
participant SSE as SSE Stream
Client->>Gateway: POST /chat/stream
activate Gateway
alt Dispatcher Enabled
Gateway->>Gateway: Build StreamDispatcherRequest
Gateway->>Dispatcher: execute_stream_dispatcher()
activate Dispatcher
Dispatcher->>Dispatcher: Run webhook dispatch logic
Dispatcher-->>Gateway: WebhookTurnResult
deactivate Dispatcher
Gateway->>Gateway: Map to StreamProcessingOutcome
else Dispatcher Disabled
Gateway->>Gateway: Legacy HTTP ingress handling
end
Gateway->>SSE: Emit SSE events (chunk/done/error)
SSE-->>Client: Stream events
Gateway->>Gateway: Update session activity
deactivate Gateway
sequenceDiagram
participant App as useChat.ts
participant Reader as readStreamEvents
participant Decoder as TextDecoder
participant Parser as parseStreamEvent
participant Callback as handleChunk/handleDone
App->>Reader: readStreamEvents(stream, callbacks)
activate Reader
loop for each incoming chunk
Reader->>Decoder: decode chunk
Decoder-->>Reader: text
Reader->>Reader: consumeStreamLine() → consumeBufferedStreamEvent()
activate Parser
Parser->>Parser: parseStreamEvent (chunk/done/error)
Parser-->>Reader: StreamLineResult
deactivate Parser
Reader->>Reader: applyStreamLineResult(result, callbacks)
Reader->>Callback: invoke onChunk or store doneEvent
end
Reader-->>App: StreamReadResult (captures doneEvent)
deactivate Reader
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![semgrep-code-dallay[bot]](https://avatars.githubusercontent.com/u/47160086?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| let content = std::fs::read_to_string(&canonical_skill_md_path)?; |
There was a problem hiding this comment.
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
🥳 Fixed in commit 0a966cb 🥳
There was a problem hiding this comment.
/fp validate_and_parse_skill_md canonicalizes the resolved SKILL.md path, verifies the canonical path remains under the canonical skill directory, and only then reads/parses it. That boundary check covers .. traversal and symlink escapes before filesystem access.
|
|
||
| std::fs::create_dir_all(dest)?; | ||
| for entry in std::fs::read_dir(src)? { | ||
| for entry in std::fs::read_dir(canonical_src)? { |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
Why this might be safe to ignore:
The code canonicalizes the path before use and explicitly validates that the canonicalized path stays within the canonical_root boundary, refusing to proceed if it escapes, which prevents path traversal attacks.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>clients/agent-runtime/src/skills/mod.rs</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/dallay/corvus/blob/de98462584c8eb3e26e7f47bf3adb4d561ce219a/clients/agent-runtime/src/skills/mod.rs#L468 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 468] canonical_src</a>"]
end
%% Intermediate
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/dallay/corvus/blob/de98462584c8eb3e26e7f47bf3adb4d561ce219a/clients/agent-runtime/src/skills/mod.rs#L468 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 468] canonical_src</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
File0:::invis
%% Connections
Source --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by tainted-path.
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
/fp copy_dir_recursive_checked validates both the canonical source directory and each canonical entry against the canonical source root before copying. Escaping symlinks/path traversal bail out before copy; latest patch also copies through staging so failed validation does not leave a partial destination.
| ); | ||
| } | ||
|
|
||
| let content = std::fs::read_to_string(&canonical_path)?; |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
Why this might be safe to ignore:
The path is canonicalized before use and validated to ensure it stays within the canonical_skill_dir boundary, preventing path traversal. The input comes from local filesystem directory entries during skill loading, not from untrusted user input.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>clients/agent-runtime/src/skills/mod.rs</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/dallay/corvus/blob/de98462584c8eb3e26e7f47bf3adb4d561ce219a/clients/agent-runtime/src/skills/mod.rs#L287 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 287] path</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/dallay/corvus/blob/de98462584c8eb3e26e7f47bf3adb4d561ce219a/clients/agent-runtime/src/skills/mod.rs#L287 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 287] canonical_path</a>"]
end
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/dallay/corvus/blob/de98462584c8eb3e26e7f47bf3adb4d561ce219a/clients/agent-runtime/src/skills/mod.rs#L296 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 296] &canonical_path</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by tainted-path.
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
/fp load_skill_md canonicalizes dir and path, checks canonical_path.starts_with(canonical_skill_dir), then reads from the canonicalized path. That prevents traversal/symlink escape outside the skill directory before the read occurs.
Deploying corvus with
|
| Latest commit: |
6fab139
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e6d0424b.corvus-42x.pages.dev |
| Branch Preview URL: | https://fix-sonarcloud-quality-gate-j6ik.corvus-42x.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clients/rook/Dockerfile (1)
7-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin apt package versions in builder stage for reproducible builds
Lines 7–11 install build dependencies without version pinning. While the final release image uses distroless (which excludes these packages), the builder stage has unpinned versions, creating supply-chain drift across builds. This is inconsistent with the pinned base images (rust:1.95, busybox:1.36.1, distroless/cc-debian13:nonroot). Keep
--no-install-recommendsand add version pins to clang, libclang-dev, and pkg-config for deterministic builds.Suggested change
RUN apt-get update && apt-get install -y --no-install-recommends \ - clang \ - libclang-dev \ - pkg-config \ + clang=<version> \ + libclang-dev=<version> \ + pkg-config=<version> \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/rook/Dockerfile` around lines 7 - 11, The RUN that installs build deps (clang, libclang-dev, pkg-config) in the builder stage must pin package versions for reproducible builds; update the RUN in the builder stage that contains "apt-get update && apt-get install -y --no-install-recommends \ clang \ libclang-dev \ pkg-config" to specify explicit version pins (e.g. clang=<version>, libclang-dev=<version>, pkg-config=<version>) matching the Debian base used, keep --no-install-recommends and the apt-get update, and ensure the rm -rf /var/lib/apt/lists/* cleanup remains; use the same unique package names (clang, libclang-dev, pkg-config) in the pins so the change is easy to locate.clients/agent-runtime/src/skills/mod.rs (2)
1031-1077:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHeads-up:
canonicalizeon broken symlinks aborts the whole install.If
skill_dirhappens to be (or contain) a dangling symlink,skill_dir.canonicalize()?at Line 1034 /skill_md_path.canonicalize()?at Line 1045 returnsErr, and the function bubbles up before theremove_dir_allcleanup branches run — so a half-installed skill directory can be left on disk. Consider mapping these errors to a cleanup-then-bail path so failure modes converge with the other validation branches in this function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/agent-runtime/src/skills/mod.rs` around lines 1031 - 1077, The canonicalize calls in validate_and_parse_skill_md (on skill_dir and on skill_md_path) can fail for dangling symlinks and currently propagate the error before remove_dir_all runs; change both usages to explicitly handle Err by attempting std::fs::remove_dir_all(skill_dir) and then returning an anyhow::Error (i.e., replace the `...?` with a match or map_err closure that calls remove_dir_all and then anyhow::bail! / Err(anyhow::Error::msg(...)) with a clear message including the original error), so all failure paths perform the same cleanup before bailing.
454-488:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSymlinks are still silently dereferenced — and a symlink loop inside the source root will recurse forever.
The "escape" check uses
canonicalize, so a symlink that resolves insidecanonical_rootpasses the guard. Two consequences:
- Silent deref: internal symlinks are followed and the target's contents are copied as a regular file/dir. Skill layouts that intentionally contain symlinks lose that semantic at install time — and the copy can pull in significantly more data than the user expects.
- Loop hazard: a self-referential symlink that resolves to an ancestor still inside
canonical_root(e.g.src/loop -> src) will passstarts_with(canonical_root)and trigger unbounded recursion viaread_dir→canonicalize→copy_dir_recursive_checked→ … withcreate_dir_all/std::fs::copyhappily overwriting on each pass. The newcopy_dir_recursive_rejects_symlink_escapetest only covers the escape case, not the loop case.Detecting symlinks via
entry.file_type()(orsymlink_metadata) and refusing/skipping them before anycanonicalizecall closes both holes and is also one fewer syscall per entry.🛡️ Suggested guard
for entry in std::fs::read_dir(canonical_src)? { let entry = entry?; + let file_type = entry.file_type()?; + if file_type.is_symlink() { + anyhow::bail!( + "Refusing to copy skill entry '{}' because it is a symlink", + entry.path().display(), + ); + } let src_path = entry.path(); let canonical_entry_path = src_path.canonicalize()?;A regression test with
src/loop -> src(orsrc/loop -> .) would lock this in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/agent-runtime/src/skills/mod.rs` around lines 454 - 488, The copy_dir_recursive_checked function currently canonicalizes every entry which silently dereferences symlinks and allows symlink loops; change the logic to call entry.file_type() or std::fs::symlink_metadata on each DirEntry (before any canonicalize) to detect symlinks and either refuse or skip them (update behavior consistently), only call canonicalize for non-symlink entries, and then proceed with the existing starts_with and recursion/copy; also add a regression test that creates a self-referential symlink (e.g., src/loop -> src or -> .) to ensure loops are rejected/skipped and do not recurse forever.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 2499-2543: execute_stream_dispatcher is requesting SSE frames
(include_sse_frames: true) but stream_outcome_from_dispatch_result ignores
result.event_frames, causing needless buffering; either stop requesting frames
or propagate them: change execute_stream_dispatcher (include_sse_frames) to
false if the stream contract only needs final text, or update
stream_outcome_from_dispatch_result and the StreamProcessingOutcome type to
accept and prefer result.event_frames (thread event_frames through
StreamProcessingOutcome and emit those instead of falling back to
response_text), adjusting callers of StreamProcessingOutcome accordingly.
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 2132-2149: Add a sibling unit test that ensures copy_dir_recursive
(and by extension copy_dir_recursive_checked) rejects symlink loops: create a
temp src dir with a marker file, create a symlink inside src that points back to
src (or to "."), call copy_dir_recursive(src.path(), &dst.path().join("copy")),
assert the call returns an Err (or does not hang) and assert that dst does not
contain duplicated or escaped entries from the loop (e.g., dst/.../marker file
should not exist or should not be recursively copied); name the test something
like copy_dir_recursive_rejects_symlink_loop and mirror the style of the
existing copy_dir_recursive_rejects_symlink_escape test.
In `@clients/web/apps/dashboard/src/composables/useChat.ts`:
- Around line 270-297: readStreamEvents currently returns void and relies on the
onDone callback to stash the final StreamDoneEvent into a StreamReadResult
wrapper; change readStreamEvents to return the captured StreamDoneEvent (or null
if none) directly by updating its signature from Promise<void> to
Promise<StreamDoneEvent | null>, collect the final doneEvent inside the
loop/after-buffer processing and return it instead of only calling onDone, and
then remove the mutable wrapper usage at call sites and fold any session-update
side-effect into a small caller helper that applies the returned doneEvent and
updates session state; refer to readStreamEvents, StreamDoneEvent, processLine,
and onDone to locate where to capture and return the final event.
- Around line 683-696: The read flow currently can exit early or throw from
readStreamEvents without releasing the ReadableStreamDefaultReader, so update
the logic around reader and readStreamEvents (the reader used by
readStreamEvents, the streamReadResult / handleDoneEvent callback, and post-read
handling that throws fallbackMessage) to always tear down the reader: wrap the
await readStreamEvents(...) and the subsequent doneEvent check in a try/finally
(or catch then finally) where in the finally you call reader.cancel() (or at
minimum reader.releaseLock()) to ensure the stream is cancelled/released when
readStreamEvents throws or when doneEvent is missing; keep setSessionReady usage
unchanged inside handleDoneEvent.
In `@scripts/release-contract.test.mjs`:
- Around line 24-33: readTomlSection currently looks for a section header
followed by a literal "\n", which breaks on CRLF checkouts; update
readTomlSection to be newline-agnostic by locating the sectionHeader
(`[${sectionName}]`) regardless of whether it's followed by "\n" or "\r\n",
compute bodyStart after the matched newline sequence, and find nextSectionStart
by searching for the next section marker that also tolerates CRLF vs LF (e.g.,
look for "\n[" or "\r\n[" or use a regex with \r?\n\[) so the returned slice of
the section body is correct on both Windows and Unix.
In `@scripts/resolve-release-components.mjs`:
- Around line 87-105: The loop in expandTransitiveComponents uses Array.some
which short-circuits and only records the first satisfied dependsOnReleaseOf
reason; replace the .some-based logic with explicit iteration so every
dependency for each component is evaluated: in expandTransitiveComponents
iterate over Object.entries(graph.components) and then for each component
iterate all component.dependsOnReleaseOf (for...of or forEach) and call
addTransitiveComponent for each dependency, updating the changed flag with
logical OR when any call returns true so all transitive reasons (added by
ensureReasonBucket/reasons push inside addTransitiveComponent) are preserved.
---
Outside diff comments:
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 1031-1077: The canonicalize calls in validate_and_parse_skill_md
(on skill_dir and on skill_md_path) can fail for dangling symlinks and currently
propagate the error before remove_dir_all runs; change both usages to explicitly
handle Err by attempting std::fs::remove_dir_all(skill_dir) and then returning
an anyhow::Error (i.e., replace the `...?` with a match or map_err closure that
calls remove_dir_all and then anyhow::bail! / Err(anyhow::Error::msg(...)) with
a clear message including the original error), so all failure paths perform the
same cleanup before bailing.
- Around line 454-488: The copy_dir_recursive_checked function currently
canonicalizes every entry which silently dereferences symlinks and allows
symlink loops; change the logic to call entry.file_type() or
std::fs::symlink_metadata on each DirEntry (before any canonicalize) to detect
symlinks and either refuse or skip them (update behavior consistently), only
call canonicalize for non-symlink entries, and then proceed with the existing
starts_with and recursion/copy; also add a regression test that creates a
self-referential symlink (e.g., src/loop -> src or -> .) to ensure loops are
rejected/skipped and do not recurse forever.
In `@clients/rook/Dockerfile`:
- Around line 7-11: The RUN that installs build deps (clang, libclang-dev,
pkg-config) in the builder stage must pin package versions for reproducible
builds; update the RUN in the builder stage that contains "apt-get update &&
apt-get install -y --no-install-recommends \ clang \ libclang-dev \ pkg-config"
to specify explicit version pins (e.g. clang=<version>, libclang-dev=<version>,
pkg-config=<version>) matching the Debian base used, keep
--no-install-recommends and the apt-get update, and ensure the rm -rf
/var/lib/apt/lists/* cleanup remains; use the same unique package names (clang,
libclang-dev, pkg-config) in the pins so the change is easy to locate.
🪄 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: 059178c9-9e01-4b75-86e8-355008723b1d
📒 Files selected for processing (8)
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/memory/dream.rsclients/agent-runtime/src/skills/mod.rsclients/rook/Dockerfileclients/web/apps/dashboard/src/composables/useChat.tsscripts/release-contract.test.mjsscripts/resolve-release-components.mjsscripts/resolve-release-from-tag.mjs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (6)
**/*
⚙️ 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/rook/Dockerfileclients/agent-runtime/src/memory/dream.rsscripts/resolve-release-components.mjsscripts/resolve-release-from-tag.mjsscripts/release-contract.test.mjsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/skills/mod.rsclients/web/apps/dashboard/src/composables/useChat.ts
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/memory/dream.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/skills/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/memory/dream.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/skills/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/memory/dream.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/skills/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/gateway/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/gateway/mod.rs
🪛 Hadolint (2.14.0)
clients/rook/Dockerfile
[warning] 7-7: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🔇 Additional comments (2)
clients/agent-runtime/src/memory/dream.rs (1)
710-711: LGTM — retry wrapper + lock assertion are the right fix.The replacement of a bare
run_if_triggered(...).unwrap().unwrap()withrun_if_triggered_after_transient_busyplusassert_eq!(first_report.lock_state, DreamLockState::Acquired)correctly guards against the test hitting transient busy state left by a parallel test runner and makes the expected post-condition explicit. No behavioral change to production code paths.One heads-up on the helper:
run_after_transient_busycaps retries at 10 × 10 ms = 100 ms. If the host is starved longer than that, the finallast_report.expect(...)either panics (allNonereturns) or hands back aBusyreport, which would cause theAcquiredassertion here to fail loudly. That's the right failure mode — just worth knowing if this test becomes intermittently flaky on slow CI agents.clients/agent-runtime/src/skills/mod.rs (1)
285-319: LGTM — and the canonicalizeddir_namematchesinstall_local_skill's naming.
canonical_skill_dir.file_name()resolves through the install-time symlink, butinstall_local_skillalready derives the destination name fromcanonical_src.file_name()(Line 1269), so the two stay consistent and frontmatter-name validation still works as intended. Path-escape guard beforeread_to_stringis the right ordering.
…nup' into fix/sonarcloud-quality-gate-cleanup
coderabbitai
Bot
added
risk:high
and removed
area:rust
risk:security
area:ci
area:web
labels
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clients/web/apps/dashboard/src/composables/useChat.ts (1)
238-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-JSON SSE
errorpayloads without failing parsing.
parseStreamEventassumeserrordata is JSON, but backend SSE currently emitserror_data.to_string()inclients/agent-runtime/src/gateway/mod.rs:2530-2558. A plain-text error will throw here and mask the real server message.As per coding guidelines, "Look for behavioral regressions, missing tests, and contract breaks across modules."Suggested fix
if (eventName === "error") { - const errorEvt = parseStreamEventJson<StreamErrorEvent>(eventData, fallbackMessage); - return { type: "error", message: errorEvt.message || fallbackMessage }; + try { + const parsed = JSON.parse(eventData) as StreamErrorEvent | string; + if (typeof parsed === "string") { + return { type: "error", message: parsed.trim() || fallbackMessage }; + } + return { type: "error", message: parsed.message?.trim() || fallbackMessage }; + } catch { + return { type: "error", message: eventData.trim() || fallbackMessage }; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/web/apps/dashboard/src/composables/useChat.ts` around lines 238 - 240, The error handler assumes SSE error payloads are JSON; change the logic in the eventName === "error" branch (where parseStreamEventJson<StreamErrorEvent>(eventData, fallbackMessage) is called) to try JSON parsing but gracefully handle plain-text payloads: attempt to parse eventData as JSON into StreamErrorEvent, and if parsing fails or yields no message fall back to using the raw eventData string (or fallbackMessage) for the returned message; ensure parseStreamEventJson usage is replaced or wrapped with this safe-parse behavior so non-JSON SSE `error` emissions from the backend do not throw and the real server message is preserved.scripts/release-contract.test.mjs (1)
1167-1176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
releaseVersionis not regex-escaped before use innew RegExp.Line 1175 constructs
new RegExp(^version = "${releaseVersion}"$, "m"). Semver strings like1.2.3contain.which is a metacharacter and matches any character —1X2Y3would also pass. UseString.rawescaping or a literal string comparison instead.🛡️ Proposed fix
- assert.match(packageStanza, new RegExp(`^version = "${releaseVersion}"$`, "m")); + const escapedVersion = releaseVersion.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + assert.match(packageStanza, new RegExp(`^version = "${escapedVersion}"$`, "m"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release-contract.test.mjs` around lines 1167 - 1176, The test uses new RegExp(`^version = "${releaseVersion}"$`, "m") which treats dots in releaseVersion as regex metacharacters; update the assertion to either escape releaseVersion before constructing the RegExp or avoid RegExp entirely by extracting the packageStanza value and doing a strict string or equality check against `version = "<releaseVersion>"`; modify the test named "cargo publish contract keeps local cerebro path and release version aligned" to use an escaped releaseVersion (e.g. escapeRegExp(releaseVersion)) or compare packageStanza lines literally instead of using new RegExp.clients/rook/Dockerfile (1)
3-3: 🧹 Nitpick | 🔵 TrivialBase images are not pinned by digest — supply chain risk.
rust:1.95-slim-trixieandgcr.io/distroless/cc-debian13:nonrootare mutable tags; a compromised or accidentally-retagged image would silently affect every build.Pin by digest for the images you do not control (at minimum the
releasestage, which ships):-FROM gcr.io/distroless/cc-debian13:nonroot AS release +FROM gcr.io/distroless/cc-debian13:nonroot@sha256:<digest> AS releaseRun
docker buildx imagetools inspect gcr.io/distroless/cc-debian13:nonrootto get the current digest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/rook/Dockerfile` at line 3, The Dockerfile uses mutable tags; replace the unpinned bases with digest-pinned references: update the "FROM rust:1.95-slim-trixie AS builder" line to use the rust image digest and update the release stage "FROM gcr.io/distroless/cc-debian13:nonroot" to use its `@sha256`:... digest. Obtain the exact digests with "docker buildx imagetools inspect <image>:<tag>" (e.g., inspect rust:1.95-slim-trixie and gcr.io/distroless/cc-debian13:nonroot) and then substitute the tags with the returned `@sha256` digests so both the builder and release stages are pinned.
♻️ Duplicate comments (2)
scripts/resolve-release-components.mjs (1)
87-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve all transitive reasons even after a component is already marked affected.
The current guard short-circuits once
componentIdis already inaffectedComponents, so later satisfied dependencies never get recorded inreasons.As per coding guidelines, "Look for behavioral regressions, missing tests, and contract breaks across modules."Suggested fix
function addTransitiveComponent(affectedComponents, reasons, componentId, dependency) { - if (!affectedComponents.has(dependency) || affectedComponents.has(componentId)) { + if (!affectedComponents.has(dependency)) { return false; } - affectedComponents.add(componentId); - ensureReasonBucket(reasons, componentId).push(`depends-on-release-of:${dependency}`); - return true; + const bucket = ensureReasonBucket(reasons, componentId); + const reason = `depends-on-release-of:${dependency}`; + if (!bucket.includes(reason)) { + bucket.push(reason); + } + + if (affectedComponents.has(componentId)) { + return false; + } + + affectedComponents.add(componentId); + return true; }Also applies to: 104-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/resolve-release-components.mjs` around lines 87 - 94, The current addTransitiveComponent function returns early when componentId is already in affectedComponents, which prevents recording additional transitive reasons; change the logic so you always call ensureReasonBucket(reasons, componentId).push(...) to record the new `depends-on-release-of:${dependency}` reason even if affectedComponents already contains componentId, and only add to affectedComponents when it wasn't present; apply the same fix to the other identical guard further down the file where ensureReasonBucket is used.clients/web/apps/dashboard/src/composables/useChat.ts (1)
689-700:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel the stream reader on teardown, not only release the lock.
reader.releaseLock()alone does not terminate an in-flight body when parsing throws mid-stream. Cancel first to avoid dangling network reads on error paths.As per coding guidelines, "Security first, performance second."Suggested fix
} finally { + await reader.cancel().catch(() => {}); reader.releaseLock(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/web/apps/dashboard/src/composables/useChat.ts` around lines 689 - 700, The teardown currently only calls reader.releaseLock(), which doesn’t cancel an in-flight stream and can leave network reads dangling; update the finally block in the function surrounding readStreamEvents so you call reader.cancel() (or await reader.cancel() if appropriate) before calling reader.releaseLock(), ensuring any in-flight parsing errors trigger stream cancellation; reference the reader variable and the readStreamEvents/applyStreamDoneEvent flow to locate the try/finally around the stream handling and add the cancel step there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/agent-runtime/src/gateway/mod.rs`:
- Around line 2524-2527: The terminal webhook log call in
log_webhook_terminal_outcome uses runtime_path="stream_dispatcher" which is
inconsistent with the earlier log_webhook_runtime_path() value
("dispatcher_agent"); update the log_webhook_terminal_outcome invocation so it
uses the same runtime_path used by log_webhook_runtime_path (e.g.,
"dispatcher_agent") to keep labels consistent for the same request (reference
symbols: log_webhook_terminal_outcome, log_webhook_runtime_path,
webhook_outcome_label, request.session_id, result.outcome).
In `@clients/agent-runtime/src/skills/mod.rs`:
- Around line 449-452: The copy_dir_recursive function must detect and reject
when the destination is inside the source tree to avoid infinite recursion:
first canonicalize dest (like canonical_src) and check whether
canonical_dest.starts_with(&canonical_src) (or otherwise is an
ancestor/descendant) and return an Err before calling
copy_dir_recursive_checked; mention the symbols to change: copy_dir_recursive,
src, dest, canonical_src, and call to copy_dir_recursive_checked so the check
happens prior to traversal and directory creation.
In `@clients/rook/Dockerfile`:
- Around line 8-10: The Dockerfile currently pins clang=1:19.0-63,
libclang-dev=1:19.0-63 and pkg-config=1.8.1-4 which will break when
trixie-security/trixie-updates publishes patch releases; update the Dockerfile
to use flexible constraints (replace the pinned package entries clang,
libclang-dev, pkg-config) or, if exact pins are required, add a RUN apt-cache
policy clang libclang-dev pkg-config step in your CI/Docker build to log
available versions so version drift is visible (target the package names shown:
clang, libclang-dev, pkg-config).
In `@scripts/release-contract.test.mjs`:
- Around line 24-35: The RegExp in readTomlSection interpolates sectionName
directly, so names with regex metacharacters break the pattern; fix by escaping
sectionName before building the RegExp (add an escape utility or inline replace
that escapes characters like . * + ? ^ $ { } ( ) | [ ] \ ) and then use the
escaped string in the existing new RegExp(`^\\[${escapedSectionName}\\]\\r?\\n`,
"m") call so the function safely handles arbitrary section names.
- Around line 128-137: trustedExecutableDirs() is a trivial wrapper that just
returns trustedExecutableDirectoryPaths; remove the unnecessary indirection by
deleting the trustedExecutableDirs() function and replace all call sites that
use trustedExecutableDirs() with the constant trustedExecutableDirectoryPaths so
callers reference the array directly.
---
Outside diff comments:
In `@clients/rook/Dockerfile`:
- Line 3: The Dockerfile uses mutable tags; replace the unpinned bases with
digest-pinned references: update the "FROM rust:1.95-slim-trixie AS builder"
line to use the rust image digest and update the release stage "FROM
gcr.io/distroless/cc-debian13:nonroot" to use its `@sha256`:... digest. Obtain the
exact digests with "docker buildx imagetools inspect <image>:<tag>" (e.g.,
inspect rust:1.95-slim-trixie and gcr.io/distroless/cc-debian13:nonroot) and
then substitute the tags with the returned `@sha256` digests so both the builder
and release stages are pinned.
In `@clients/web/apps/dashboard/src/composables/useChat.ts`:
- Around line 238-240: The error handler assumes SSE error payloads are JSON;
change the logic in the eventName === "error" branch (where
parseStreamEventJson<StreamErrorEvent>(eventData, fallbackMessage) is called) to
try JSON parsing but gracefully handle plain-text payloads: attempt to parse
eventData as JSON into StreamErrorEvent, and if parsing fails or yields no
message fall back to using the raw eventData string (or fallbackMessage) for the
returned message; ensure parseStreamEventJson usage is replaced or wrapped with
this safe-parse behavior so non-JSON SSE `error` emissions from the backend do
not throw and the real server message is preserved.
In `@scripts/release-contract.test.mjs`:
- Around line 1167-1176: The test uses new RegExp(`^version =
"${releaseVersion}"$`, "m") which treats dots in releaseVersion as regex
metacharacters; update the assertion to either escape releaseVersion before
constructing the RegExp or avoid RegExp entirely by extracting the packageStanza
value and doing a strict string or equality check against `version =
"<releaseVersion>"`; modify the test named "cargo publish contract keeps local
cerebro path and release version aligned" to use an escaped releaseVersion (e.g.
escapeRegExp(releaseVersion)) or compare packageStanza lines literally instead
of using new RegExp.
---
Duplicate comments:
In `@clients/web/apps/dashboard/src/composables/useChat.ts`:
- Around line 689-700: The teardown currently only calls reader.releaseLock(),
which doesn’t cancel an in-flight stream and can leave network reads dangling;
update the finally block in the function surrounding readStreamEvents so you
call reader.cancel() (or await reader.cancel() if appropriate) before calling
reader.releaseLock(), ensuring any in-flight parsing errors trigger stream
cancellation; reference the reader variable and the
readStreamEvents/applyStreamDoneEvent flow to locate the try/finally around the
stream handling and add the cancel step there.
In `@scripts/resolve-release-components.mjs`:
- Around line 87-94: The current addTransitiveComponent function returns early
when componentId is already in affectedComponents, which prevents recording
additional transitive reasons; change the logic so you always call
ensureReasonBucket(reasons, componentId).push(...) to record the new
`depends-on-release-of:${dependency}` reason even if affectedComponents already
contains componentId, and only add to affectedComponents when it wasn't present;
apply the same fix to the other identical guard further down the file where
ensureReasonBucket is used.
🪄 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: 55fc7455-4c3e-491e-a1fa-f1cc8c5830cb
📒 Files selected for processing (6)
clients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/skills/mod.rsclients/rook/Dockerfileclients/web/apps/dashboard/src/composables/useChat.tsscripts/release-contract.test.mjsscripts/resolve-release-components.mjs
📜 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: semgrep-cloud-platform/scan
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (6)
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
scripts/resolve-release-components.mjsclients/rook/Dockerfileclients/web/apps/dashboard/src/composables/useChat.tsclients/agent-runtime/src/skills/mod.rsclients/agent-runtime/src/gateway/mod.rsscripts/release-contract.test.mjs
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/skills/mod.rsclients/agent-runtime/src/gateway/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/skills/mod.rsclients/agent-runtime/src/gateway/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/skills/mod.rsclients/agent-runtime/src/gateway/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/gateway/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/gateway/mod.rs
🔇 Additional comments (6)
scripts/release-contract.test.mjs (3)
53-66: LGTM —process.execPath+ trusted PATH propagation is the right hardening.Switching all child-process helpers from the string
"node"toprocess.execPathand replacing an inherited, potentially attacker-influencedPATHwithtrustedExecutableDirectoryPaths.join(path.delimiter)is a clean, effective fix for the SonarCloud security hotspot. No concerns.Also applies to: 68-78, 92-102, 660-673, 675-691, 784-796
24-35: Past review comment is resolved.The CRLF-agnostic rewrite of
readTomlSectionusing\r?\nin both the section-header regex and the next-section search correctly handles bothLFandCRLFcheckouts. The previous brittleindexOf("\n[")approach is gone.
1197-1208: Cargo stderr normalization is correct.Buffer → string → empty-string fallback chain handles all known
execFileSyncerror shapes cleanly. The network-access skip guard is a sensible CI adaptation.clients/rook/Dockerfile (1)
7-7:--no-install-recommendsis the right call.Reduces the builder-layer attack surface by excluding optional packages. LGTM.
clients/agent-runtime/src/skills/mod.rs (2)
286-315: Nice path-confinement hardening.Canonicalizing both the skill directory and
SKILL.mdbefore reading closes symlink and traversal escapes, and anchoringlocationto the canonical manifest path keeps later checks pointed at the real file.Also applies to: 1057-1097
958-972: Good atomic-copy coverage.The staging-dir cleanup in
copy_dir_atomic, plus the new loop/escape tests, materially reduces the chance of leaving half-copied skill contents behind on failure.Also applies to: 2173-2231
- gateway: use consistent runtime_path label 'dispatcher_agent' in terminal outcome log - skills: add dest-inside-src check before copying, handle non-existent dest gracefully - rook: pin base images by digest, use flexible package version constraints - dashboard: handle plain-text SSE error payloads gracefully, add reader.cancel() before releaseLock() - release-contract: escape regex metacharacters in TOML section parsing and version matching - release-contract: remove unnecessary trustedExecutableDirs wrapper, use constant directly - resolve-release-components: always record transitive dependency reasons even when component already in set
- dashboard: add test for plain-text SSE error fallback in stream parsing - release-contract: add test verifying multiple transitive reasons are recorded
|
1 participant
Related Issues
SonarCloud quality gate: https://sonarcloud.io/project/overview?id=dallay_corvus
Summary
Tested Information
cargo clippy --manifest-path "clients/agent-runtime/Cargo.toml" --all-targets -- -D warningscargo test --manifest-path "clients/agent-runtime/Cargo.toml" --lib gateway::cargo test --manifest-path "clients/agent-runtime/Cargo.toml" --lib memory::dream::tests::suppresses_duplicate_triggers_by_session_id -- --exactnode --test "scripts/release-contract.test.mjs"pnpm --filter @corvus/dashboard run checkReview path: start with
scripts/release-contract.test.mjsand the release resolver changes, then review the stream parsing/dispatch helper extraction.Documentation Impact
Breaking Changes
None.
Checklist