fix: address sonar quality findings#769
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR contains coordinated updates to CI tooling (SonarQube exclusions, Rust verification), refactors the streaming and chat event handling for readability via extracted helper functions, updates the sandbox Node.js installation method, and reorganizes release component validation logic into dedicated helper functions. ChangesSonar Exclusions Expansion
Rust Toolchain Verification
Code Refactoring
Development Environment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clients/web/apps/dashboard/src/composables/useChat.ts (1)
612-614:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPre-existing:
readeris not cancelled on error paths.When
processLinethrows (e.g., on a server-sent"error"event) the exception bubbles throughprocessStreamBufferand thewhile(true)loop, landing in the outercatch. Thefinallyblock resetssending/streamingand clears the timeout, butreader.cancel()is never called. TheReadableStreamstays locked until GC.While browsers tolerate this, it delays resource release and can leave the server streaming into a zombie connection. Adding a
reader.cancel()call infinallyis low-risk.🛡️ Suggested fix
- const reader = response.body?.getReader(); + let reader: ReadableStreamDefaultReader<Uint8Array> | undefined; + reader = response.body?.getReader(); if (!reader) { throw new Error(t("chat.streamUnavailable")); }} finally { clearTimeout(timeoutId); + reader?.cancel().catch(() => void 0); sending.value = false; streaming.value = false; }Also applies to: 662-677
🤖 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 612 - 614, The reader obtained from response.body?.getReader() is not cancelled on error paths, so ensure reader.cancel() is always called: inside the async function that reads the stream (the while(true) loop that uses processStreamBuffer and calls processLine), add a guard that captures the reader variable and invoke await reader.cancel() in the outer finally block (or a finally adjacent to the try/catch that wraps the streaming loop) so the ReadableStream is explicitly released on success or any thrown errors from processLine; reference the reader local created from response.body?.getReader(), the processStreamBuffer loop/while(true), and the finally block that currently resets sending/streaming/timeout to locate where to add the await reader.cancel() call.dev/sandbox/Dockerfile (1)
11-27: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUnpinned
apt-get installbreaks reproducible builds.Every rebuild may silently pull different package versions (e.g., a new
gitminor version), making the image non-deterministic. Pin the packages you care most about for a stable sandbox baseline.💡 Example: pinning key packages
- build-essential \ - curl \ - git \ - nodejs \ - npm \ + build-essential=12.* \ + curl=8.* \ + git=1:2.* \ + nodejs=18.* \ # or the NodeSource equivalent + npm=9.* \Run
apt-cache policy <package>inside the image to get exact candidate versions to pin.🤖 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 `@dev/sandbox/Dockerfile` around lines 11 - 27, The apt-get install line in the Dockerfile should pin package versions to make builds reproducible: determine exact candidate versions with apt-cache policy for each important package (e.g., git, nodejs, python3, npm) and replace bare package names in the RUN apt-get install ... command with explicit pins (package=version), use --no-install-recommends to avoid non-deterministic deps, and keep the surrounding steps to update/clean (apt-get update; apt-get install -y --no-install-recommends <pinned-packages>; apt-get clean && rm -rf /var/lib/apt/lists/*) so the RUN block in the Dockerfile becomes deterministic.
🤖 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 `@dev/sandbox/Dockerfile`:
- Around line 7-20: The Dockerfile currently installs nodejs and npm from Ubuntu
apt (the RUN apt-get install ... list), which yields Node 18; remove nodejs and
npm from that apt install list and instead add steps to configure NodeSource's
signed apt repository for Node.js LTS (v22 or v20 as desired) using their GPG
key and a proper sources.list.d entry, run apt-get update, then apt-get install
nodejs from that trusted repo; locate the existing RUN that contains
"build-essential ... nodejs npm" and replace the plain apt install of nodejs/npm
with the NodeSource repository setup followed by installing the nodejs package
so the image uses the specified supported LTS version.
In `@scripts/release-components.mjs`:
- Around line 71-74: Add a unit test in release-contract.test.mjs that calls
validateGraphShape with an invalid root value (e.g., null, array, or non-object)
and asserts that it throws a TypeError specifically (not a generic Error);
reference the validateGraphShape function to locate the behavior under test and
include separate assertions for at least one null/undefined case and one Array
case to cover both branches of the guard.
---
Outside diff comments:
In `@clients/web/apps/dashboard/src/composables/useChat.ts`:
- Around line 612-614: The reader obtained from response.body?.getReader() is
not cancelled on error paths, so ensure reader.cancel() is always called: inside
the async function that reads the stream (the while(true) loop that uses
processStreamBuffer and calls processLine), add a guard that captures the reader
variable and invoke await reader.cancel() in the outer finally block (or a
finally adjacent to the try/catch that wraps the streaming loop) so the
ReadableStream is explicitly released on success or any thrown errors from
processLine; reference the reader local created from response.body?.getReader(),
the processStreamBuffer loop/while(true), and the finally block that currently
resets sending/streaming/timeout to locate where to add the await
reader.cancel() call.
In `@dev/sandbox/Dockerfile`:
- Around line 11-27: The apt-get install line in the Dockerfile should pin
package versions to make builds reproducible: determine exact candidate versions
with apt-cache policy for each important package (e.g., git, nodejs, python3,
npm) and replace bare package names in the RUN apt-get install ... command with
explicit pins (package=version), use --no-install-recommends to avoid
non-deterministic deps, and keep the surrounding steps to update/clean (apt-get
update; apt-get install -y --no-install-recommends <pinned-packages>; apt-get
clean && rm -rf /var/lib/apt/lists/*) so the RUN block in the Dockerfile becomes
deterministic.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 8d9aa578-d17b-4eb5-9a80-eb21c88eb127
📒 Files selected for processing (7)
.github/workflows/core-check.yml.github/workflows/sonarqube-analysis.ymlclients/rook/src/gateway/streaming.rsclients/web/apps/dashboard/src/composables/useChat.tsdev/sandbox/Dockerfilescripts/release-components.mjsscripts/sonar.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pr-checks
- GitHub Check: sonar
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ 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/sonar.shdev/sandbox/Dockerfileclients/rook/src/gateway/streaming.rsscripts/release-components.mjsclients/web/apps/dashboard/src/composables/useChat.ts
**/*.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/rook/src/gateway/streaming.rs
🪛 Hadolint (2.14.0)
dev/sandbox/Dockerfile
[warning] 11-11: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 11-11: Avoid additional packages by specifying --no-install-recommends
(DL3015)
🔇 Additional comments (11)
clients/web/apps/dashboard/src/composables/useChat.ts (1)
625-640: Refactor is behaviorally equivalent — LGTM for the changed lines.The
switch→if-chain conversion correctly covers all fourStreamLineResultvariants:"chunk"and"done"have explicit early returns,"error"throws, and"continue"falls through implicitly with no action. TypeScript discriminated-union narrowing works identically in both styles, soresult.doneEventandresult.chunkare correctly typed in their respective branches.dev/sandbox/Dockerfile (1)
28-38: LGTM — user setup and version-check pattern are solid.The three-way UID conflict guard (
id -u developer,getent passwd 1000) is well-structured, the0440sudoers permission is correct, and thenode --version && npm --versionat the end of theRUNlayer gives a fast-fail signal during the build..github/workflows/sonarqube-analysis.yml (1)
136-136: LGTM — exclusion patterns matchscripts/sonar.shexactly.The two new Cargo.toml wildcard patterns are identical to those added in
scripts/sonar.sh, keeping local and hosted scan configurations in sync. No concerns on this change.scripts/sonar.sh (1)
100-100: ⚡ Quick winNo action needed—both sonar.exclusions sources are in sync.
The new exclusion patterns (
**/clients/agent-runtime/crates/**/Cargo.tomland**/clients/agent-runtime/firmware/**/Cargo.toml) are syntactically correct for SonarQube, and the scripts/sonar.sh and CI workflow YAML are consistent. Asonar-project.propertiesfile does not exist in the repository, so no third location requires updating.scripts/release-components.mjs (3)
28-38: Clean shape guard.The null/array/primitive guard at line 29 before the field loop is correct — no issues.
85-125: LGTM — correct extractions.
validateComponentandvalidateSharedInfraPathboth correctly threadgraphthrough for cross-reference checks, and the validation ordering within each helper is sound.
127-146: LGTM — delegation order is correct.
validateGraphShaperuns first, guaranteeinggraph.componentsandgraph.sharedInfraPathsare objects before any reference lookups in the downstream helpers. The guard against an empty components map (lines 133–135) correctly fires aftervalidateInternalReleaseDependenciessince that step only requires the components map to exist, not to be non-empty.clients/rook/src/gateway/streaming.rs (3)
164-172:run_completion_once— correct and clean.
Option::take()at the call site ensures the callback can never be invoked twice. The guard inside the function gracefully handles the already-Nonecase without panicking.
174-179:pop_pending_eventcorrectly threads theis_donesignal through the state.Emitting
[DONE]to the client in the current step and settingterminated = truefor the next unfold iteration preserves the required event-before-completion ordering thathandlers.rsdepends on.
144-199: Refactoring is behaviorally correct — the three extracted helpers cleanly decompose the original loop.The unfold state transitions are sound:
- Pending-first drain before upstream fetch avoids a spurious upstream poll when buffered events remain.
terminatedis only set after[DONE]is emitted (not before), so the sentinel reaches the client before the completion callback fires.on_complete.take()in the termination branch is the only extraction point, eliminating any double-call risk..github/workflows/core-check.yml (1)
62-77: ⚡ Quick winThe Rust verification step in core-check.yml is likely unnecessary —
agent-core-kmpis pure Kotlin and does not compile Rust code.The
🦀 Verify Rust toolchainstep (lines 62–72) checks forcargoandrustc, but:agent-core-kmp:checkis a pure Kotlin Multiplatform build with no Rust compilation. The RustCliBridgeTest uses mock shell executables, not the actual Rust binaries. If this step exists only to verify toolchain presence without being consumed by the Gradle task, consider removing it.If runtime invocation of a Rust binary is required during tests, the corvus executable should be explicitly provided as part of the test environment or via a setup action—not implicitly expected from the GitHub runner image.
> Likely an incorrect or invalid review comment.
| # - Node.js/npm: from Ubuntu's signed package repositories | ||
| # - Core: curl, git, vim, build-essential (gcc, make) | ||
| # - Python: python3, pip | ||
| # - Network: ping, dnsutils | ||
| RUN apt-get update && apt-get install -y curl && \ | ||
| curl -fsSL https://deb.nodesource.com/setup_20.x | bash - && \ | ||
| apt-get install -y \ | ||
| RUN apt-get update && apt-get install -y \ | ||
| build-essential \ | ||
| curl \ | ||
| dnsutils \ | ||
| git \ | ||
| iputils-ping \ | ||
| nano \ | ||
| net-tools \ | ||
| nodejs \ | ||
| npm \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the current Dockerfile
cat dev/sandbox/DockerfileRepository: dallay/corvus
Length of output: 1195
🏁 Script executed:
# Check git history for the Dockerfile to see what changed
cd dev/sandbox && git log --oneline -20 Dockerfile 2>/dev/null || echo "Git history not available"Repository: dallay/corvus
Length of output: 101
🏁 Script executed:
# Search for any NodeSource references in git history
git log -p dev/sandbox/Dockerfile 2>/dev/null | grep -A5 -B5 "nodesource\|NodeSource" | head -50 || echo "No NodeSource history found"Repository: dallay/corvus
Length of output: 39
🌐 Web query:
Node.js 18 end of life EOL date April 2025
💡 Result:
Node.js 18 end of life (EOL) date was April 30, 2025. Evidence: The official Node.js End-Of-Life page lists “v18 (Hydrogen)” with the last updated/EOL date as Mar 27, 2025, and the EOL end date is shown as April 30, 2025 on Node’s official release plan table for v18.x (End-of-life: 2025-04-30).
Citations:
🏁 Script executed:
# Get more detailed git history to find the previous version
git log --all --oneline dev/sandbox/Dockerfile | head -10Repository: dallay/corvus
Length of output: 433
🏁 Script executed:
# Try to find the previous commit before the sonar fix
git show 7c73d95^:dev/sandbox/Dockerfile 2>/dev/null | grep -A 20 "Node.js"Repository: dallay/corvus
Length of output: 39
🏁 Script executed:
# Search for any mention of nodesource or Node.js version in recent commits
git log --all -p dev/sandbox/Dockerfile 2>/dev/null | grep -i "nodesource\|node.*20\|v20" | head -20Repository: dallay/corvus
Length of output: 241
nodejs and npm from Ubuntu 24.04 apt default to v18, which is EOL—a regression from the previously installed v20.
Ubuntu 24.04's default apt repositories ship Node.js v18, not v20. This is a downgrade from the prior NodeSource v20 installation, and Node.js 18 reached end-of-life on April 30, 2025. The sandbox will now run an EOL Node.js version without security patches—the opposite of this PR's security intent.
To maintain v20 (or upgrade to v22 LTS) without the curl | bash risk, use NodeSource's modern gpg-verified method:
🔒 Proposed fix: install Node.js v22 LTS via gpg-verified NodeSource apt repo
-# - Node.js/npm: from Ubuntu's signed package repositories
+# - Node.js/npm: from NodeSource apt repo (gpg-verified, no curl|bash)
# - Core: curl, git, vim, build-essential (gcc, make)
# - Python: python3, pip
# - Network: ping, dnsutils
-RUN apt-get update && apt-get install -y \
+RUN apt-get update && apt-get install -y --no-install-recommends \
build-essential \
ca-certificates \
curl \
dnsutils \
git \
gnupg \
iputils-ping \
nano \
net-tools \
python3 \
python3-pip \
sudo \
unzip \
vim \
wget \
zip \
+ && mkdir -p /etc/apt/keyrings \
+ && curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key \
+ | gpg --dearmor -o /etc/apt/keyrings/nodesource.gpg \
+ && echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_22.x nodistro main" \
+ > /etc/apt/sources.list.d/nodesource.list \
+ && apt-get update && apt-get install -y --no-install-recommends nodejs \
&& if id -u developer >/dev/null 2>&1; then \
true; \
elif getent passwd 1000 >/dev/null 2>&1; then \🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 11-11: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 11-11: Avoid additional packages by specifying --no-install-recommends
(DL3015)
🤖 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 `@dev/sandbox/Dockerfile` around lines 7 - 20, The Dockerfile currently
installs nodejs and npm from Ubuntu apt (the RUN apt-get install ... list),
which yields Node 18; remove nodejs and npm from that apt install list and
instead add steps to configure NodeSource's signed apt repository for Node.js
LTS (v22 or v20 as desired) using their GPG key and a proper sources.list.d
entry, run apt-get update, then apt-get install nodejs from that trusted repo;
locate the existing RUN that contains "build-essential ... nodejs npm" and
replace the plain apt install of nodejs/npm with the NodeSource repository setup
followed by installing the nodejs package so the image uses the specified
supported LTS version.
| function validateGraphShape(graph) { | ||
| if (!graph || typeof graph !== "object" || Array.isArray(graph)) { | ||
| throw new Error("release component graph must be an object"); | ||
| throw new TypeError("release component graph must be an object"); | ||
| } |
There was a problem hiding this comment.
TypeError is semantically correct but the error path has no test coverage.
TypeError represents "an error when a value is not of the expected type" and is a subclass of Error, so all current callers that catch generically remain compatible. However, none of the tests in release-contract.test.mjs exercise the invalid-root-graph path to assert that a TypeError (rather than a plain Error) is thrown, meaning the intended contract change is not verified by the test suite.
🧪 Suggested test assertion
// In scripts/release-contract.test.mjs (new test)
+test("loadReleaseComponents throws TypeError when graph root is not an object", async () => {
+ // Point RELEASE_COMPONENTS_PATH at a temp file containing a non-object (e.g. `[]` or `"string"`)
+ // then assert:
+ assert.throws(() => validateGraph([]), { instanceOf: TypeError });
+ assert.throws(() => validateGraph("string"), { instanceOf: TypeError });
+});🤖 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-components.mjs` around lines 71 - 74, Add a unit test in
release-contract.test.mjs that calls validateGraphShape with an invalid root
value (e.g., null, array, or non-object) and asserts that it throws a TypeError
specifically (not a generic Error); reference the validateGraphShape function to
locate the behavior under test and include separate assertions for at least one
null/undefined case and one Array case to cover both branches of the guard.
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: A pull request for this branch has been added to a merge queue. Branches that |
2 participants
Related Issues
Related to SonarCloud quality findings for
dallay_corvus.Summary
Tested Information
cargo fmt --checkinclients/rookcargo test -p rook gateway::streaminginclients/rooknode --check scripts/release-components.mjsnode --test scripts/release-contract.test.mjsbash -n scripts/sonar.shdocker build -t corvus-sandbox-sonar-check dev/sandboxgit diff --checkgit push -u origin fix/sonar-quality-remediationpre-push hook ran web lint/checks and passedDocumentation Impact
Breaking Changes
None.
Checklist