fix(quality): reduce cognitive complexity and fix shell script issues#136
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors a Bash audit script for stricter Bash idioms and safer file iteration; refactors agent runtime Rust to extract mission checkpoint processing, gateway comparison helpers, and a providers command handler into dedicated helper functions without changing external behavior. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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-04 to 2026-03-04 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/web-quality-audit/scripts/analyze.sh:
- Around line 10-12: The usage() function ends with both exit 1 and an
unreachable return 1; remove the redundant "return 1" so the function only calls
exit 1 (delete the "return 1" line inside usage()) to satisfy ShellCheck SC2317
and avoid unreachable code in the script.
- Around line 65-69: The directory scan uses `find "$TARGET" | while read -r
file; do analyze_html "$file"; done` which runs the loop in a subshell so
updates to `ISSUES` and `WARNINGS` inside `analyze_html` are lost; replace the
pipeline with process substitution so the while-loop runs in the parent shell
(e.g. feed `find` into the loop via `< <(...)`) and prefer safe filename
handling (use `find` with `-print0` and a `while IFS= read -r -d '' file; do
analyze_html "$file"; done < <(find ... -print0)`) so `analyze_html`, `ISSUES`,
and `WARNINGS` are updated correctly in the parent shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 673e5060-ee14-482f-a4d2-24c724323dac
📒 Files selected for processing (4)
.agents/skills/web-quality-audit/scripts/analyze.shclients/agent-runtime/src/agent/agent.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/main.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (7)
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/main.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/agent/agent.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/main.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/agent/agent.rs
clients/agent-runtime/src/main.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
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
Keep startup path lean and avoid heavy initialization in command parsing flow
Files:
clients/agent-runtime/src/main.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/main.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/agent/agent.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/agent/agent.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
🧠 Learnings (7)
📓 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: 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
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
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
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
📚 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/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/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/main.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/agent/agent.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/main.rsclients/agent-runtime/src/agent/agent.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/gateway/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/gateway/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/agent.rs
🪛 Shellcheck (0.11.0)
.agents/skills/web-quality-audit/scripts/analyze.sh
[info] 11-11: Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
(SC2317)
🔇 Additional comments (4)
clients/agent-runtime/src/main.rs (2)
669-672: Providers command extraction is clean and behavior-preserving.Delegating listing/printing to
handle_providers_commandsimplifies dispatch flow without altering CLI output semantics.Also applies to: 708-738
669-672: Please document Rust quality-gate results for this refactor.I don’t see evidence in this PR context that required checks were run (or explicitly skipped with reason). Please attach results for:
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo testBased on learnings: Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why.clients/agent-runtime/src/agent/agent.rs (1)
845-886: Checkpoint handling refactor is solid and keeps lifecycle transitions explicit.The extraction into
process_mission_checkpointimproves readability while preserving governance checks, accounting, and explicit completed/terminated mission outcomes.Also applies to: 888-1015
clients/agent-runtime/src/gateway/mod.rs (1)
445-453: Gateway/webhook comparison modularization looks correct and maintainable.The helper split keeps restart-required field detection explicit and preserves existing security-sensitive comparison behavior for gateway limits and webhook secret modes.
Also applies to: 456-527, 571-609
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Deploying corvus with
|
| Latest commit: |
21caeac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://83d4ce47.corvus-42x.pages.dev |
| Branch Preview URL: | https://fix-ss-qa.corvus-42x.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.agents/skills/web-quality-audit/scripts/analyze.sh (1)
64-67:⚠️ Potential issue | 🟡 MinorLimit directory scanning to regular files.
At Line [67],
findcan return non-file entries (e.g., directories named*.html), which can produce noisy/incorrect findings when passed toanalyze_html.Suggested fix
- done < <(find "$TARGET" \( -name "*.html" -o -name "*.htm" \) -print0) + done < <(find "$TARGET" -type f \( -name "*.html" -o -name "*.htm" \) -print0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/web-quality-audit/scripts/analyze.sh around lines 64 - 67, The find invocation that feeds analyze_html is returning non-regular entries; update the command that generates the file list (the subshell reading into the while loop) to restrict results to regular files only so analyze_html only receives true files (use find with the appropriate test like -type f targeting "$TARGET" for *.html and *.htm). Ensure you keep the existing while IFS= read -r -d '' file; do analyze_html "$file"; done loop and the TARGET variable, only altering the find predicate to include -type f so directories matching *.html/*.htm are excluded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/web-quality-audit/scripts/analyze.sh:
- Around line 79-87: The script injects raw ISSUE and WARNING strings into JSON
(echo -n " \"${ISSUES[$i]}\"" and echo -n " \"${WARNINGS[$i]}\"") which
can break JSON when values contain quotes, backslashes, or newlines; add a
helper function (e.g., escape_json_string) that replaces backslash -> \\\\,
double-quote -> \\\", newline -> \\n, tab -> \\t, carriage return -> \\r,
formfeed -> \\f (or use jq -R -s / printf with %q if available) and call that
function when emitting values from the ISSUES and WARNINGS arrays so you print
escaped strings instead of raw "${ISSUES[$i]}" and "${WARNINGS[$i]}".
---
Duplicate comments:
In @.agents/skills/web-quality-audit/scripts/analyze.sh:
- Around line 64-67: The find invocation that feeds analyze_html is returning
non-regular entries; update the command that generates the file list (the
subshell reading into the while loop) to restrict results to regular files only
so analyze_html only receives true files (use find with the appropriate test
like -type f targeting "$TARGET" for *.html and *.htm). Ensure you keep the
existing while IFS= read -r -d '' file; do analyze_html "$file"; done loop and
the TARGET variable, only altering the find predicate to include -type f so
directories matching *.html/*.htm are excluded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45397092-2323-457a-aa53-b25a545826e4
📒 Files selected for processing (1)
.agents/skills/web-quality-audit/scripts/analyze.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
🧠 Learnings (1)
📓 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: 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
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
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
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
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
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
🔇 Additional comments (1)
.agents/skills/web-quality-audit/scripts/analyze.sh (1)
13-15: Good Bash hardening in argument handling and explicit function completion.Switching to
[[ ... ]]and making control flow explicit improves robustness and readability.Also applies to: 59-60
| if [[ $i -gt 0 ]]; then echo ','; fi | ||
| echo -n " \"${ISSUES[$i]}\"" | ||
| done | ||
| echo '' | ||
| echo ' ],' | ||
| echo ' "warnings": [' | ||
| for i in "${!WARNINGS[@]}"; do | ||
| if [ $i -gt 0 ]; then echo ','; fi | ||
| if [[ $i -gt 0 ]]; then echo ','; fi | ||
| echo -n " \"${WARNINGS[$i]}\"" |
There was a problem hiding this comment.
Escape JSON string values before emitting output.
At Lines [80] and [87], raw strings are injected into JSON. Filenames containing quotes, backslashes, or newlines can produce invalid JSON.
Suggested fix
+json_escape() {
+ local s="$1"
+ s=${s//\\/\\\\}
+ s=${s//\"/\\\"}
+ s=${s//$'\n'/\\n}
+ s=${s//$'\r'/\\r}
+ s=${s//$'\t'/\\t}
+ printf '%s' "$s"
+}
+
# Output results as JSON
echo '{'
echo ' "issues": ['
for i in "${!ISSUES[@]}"; do
if [[ $i -gt 0 ]]; then echo ','; fi
- echo -n " \"${ISSUES[$i]}\""
+ printf ' "%s"' "$(json_escape "${ISSUES[$i]}")"
done
echo ''
echo ' ],'
echo ' "warnings": ['
for i in "${!WARNINGS[@]}"; do
if [[ $i -gt 0 ]]; then echo ','; fi
- echo -n " \"${WARNINGS[$i]}\""
+ printf ' "%s"' "$(json_escape "${WARNINGS[$i]}")"
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ $i -gt 0 ]]; then echo ','; fi | |
| echo -n " \"${ISSUES[$i]}\"" | |
| done | |
| echo '' | |
| echo ' ],' | |
| echo ' "warnings": [' | |
| for i in "${!WARNINGS[@]}"; do | |
| if [ $i -gt 0 ]; then echo ','; fi | |
| if [[ $i -gt 0 ]]; then echo ','; fi | |
| echo -n " \"${WARNINGS[$i]}\"" | |
| json_escape() { | |
| local s="$1" | |
| s=${s//\\/\\\\} | |
| s=${s//\"/\\\"} | |
| s=${s//$'\n'/\\n} | |
| s=${s//$'\r'/\\r} | |
| s=${s//$'\t'/\\t} | |
| printf '%s' "$s" | |
| } | |
| # Output results as JSON | |
| echo '{' | |
| echo ' "issues": [' | |
| for i in "${!ISSUES[@]}"; do | |
| if [[ $i -gt 0 ]]; then echo ','; fi | |
| printf ' "%s"' "$(json_escape "${ISSUES[$i]}")" | |
| done | |
| echo '' | |
| echo ' ],' | |
| echo ' "warnings": [' | |
| for i in "${!WARNINGS[@]}"; do | |
| if [[ $i -gt 0 ]]; then echo ','; fi | |
| printf ' "%s"' "$(json_escape "${WARNINGS[$i]}")" | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/web-quality-audit/scripts/analyze.sh around lines 79 - 87,
The script injects raw ISSUE and WARNING strings into JSON (echo -n "
\"${ISSUES[$i]}\"" and echo -n " \"${WARNINGS[$i]}\"") which can break JSON
when values contain quotes, backslashes, or newlines; add a helper function
(e.g., escape_json_string) that replaces backslash -> \\\\, double-quote ->
\\\", newline -> \\n, tab -> \\t, carriage return -> \\r, formfeed -> \\f (or
use jq -R -s / printf with %q if available) and call that function when emitting
values from the ISSUES and WARNINGS arrays so you print escaped strings instead
of raw "${ISSUES[$i]}" and "${WARNINGS[$i]}".
|
1 participant
This pull request introduces several improvements and refactorings across the codebase, focusing on modularizing comparison logic in the gateway module, refactoring mission checkpoint handling in the agent runtime, and improving shell script robustness. Additionally, CLI provider listing logic is refactored for clarity. The most important changes are grouped below:
Agent mission checkpoint handling refactor
Agentby extracting checkpoint processing into a new async methodprocess_mission_checkpoint, improving readability and error handling inclients/agent-runtime/src/agent/agent.rs. The new structure makes checkpoint advancement and error paths more explicit, and centralizes logic for checkpoint processing and mission completion. [1] [2] [3] [4] [5]MissionCheckpointto support the refactored logic.Gateway and webhook comparison modularization
compare_gateway_fieldsinto smaller, focused functions:compare_gateway_basic_fieldsandcompare_gateway_limits_fieldsinclients/agent-runtime/src/gateway/mod.rs. This improves maintainability and makes the code easier to test and extend. [1] [2] [3]compare_webhook_secret_fieldfromcompare_webhook_fieldsfor better modularity. [1] [2]CLI providers command refactor
handle_providers_commandto simplify the main CLI command handler inclients/agent-runtime/src/main.rs. [1] [2]Shell script robustness improvements
.agents/skills/web-quality-audit/scripts/analyze.shto use double-bracket[[ ... ]]conditionals for better Bash compatibility and added explicitreturnstatements in functions for clearer control flow. [1] [2] [3]