Release cargo-bless 0.1.1 with code audit#22
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (21)
WalkthroughThis pull request introduces a comprehensive code-audit system to cargo-bless that scans Rust projects for suspicious patterns and structural issues. A new code-audit module uses tree-sitter for parsing and detects patterns like Arc, excessive unwrap calls, and complexity heuristics. CLI support includes a dedicated Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Entry Point
participant Policy as Policy Loader
participant Audit as Code Audit Scanner
participant Parser as Tree-Sitter Parser
participant Output as Output Renderer
CLI->>Policy: Load policy from bless.toml/path
Policy-->>CLI: CodeAuditConfig (suppressions/settings)
CLI->>Audit: scan_project(manifest_path, config)
Audit->>Parser: Parse Rust files with tree-sitter
Parser-->>Audit: AST + masked comments/strings
Audit->>Audit: Detect patterns (Arc<RwLock>, unwrap, etc.)
Audit->>Audit: Filter alerts by config & dedup
Audit-->>CLI: CodeAuditReport (alerts by kind)
CLI->>Output: render_code_audit_report(report)
Output-->>CLI: Terminal output with heat score & findings
alt JSON mode
CLI->>Output: render_json_report(suggestions, audit)
Output-->>CLI: Unified JSON {dependency_suggestions, code_audit}
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 35 minutes and 30 seconds.Comment |
Ruffian-L
force-pushed
the
release/0.1.1-code-audit
branch
from
e1f8ff9 to
e69311f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
155-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
--fail-oninto the exit path.
BlessOpts.fail_onis parsed and documented, but nothing in this command checks it before returningOk(()).cargo bless --fail-on=highwill still exit 0 even when matching findings are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 155 - 169, The command currently returns Ok(()) without considering opts.fail_on; add a check before the final Ok(()) that evaluates whether any findings meet the threshold specified by opts.fail_on (for code audit results in report when run_code_audit is true, and for lint/format suggestions in suggestions), and if so return an error or non-zero exit (e.g., propagate a failure Result) so cargo bless exits non‑zero when matching findings are present; locate the check near the end of main where report, suggestions, run_code_audit, and opts are in scope and use opts.fail_on to decide the exit outcome.
🧹 Nitpick comments (1)
tests/real_project_test.rs (1)
155-170: ⚡ Quick winMake the JSON contract test fail on non-JSON output.
The fallback branch explicitly passes when
--jsonprints arbitrary text, so regressions in the JSON path won't be caught. Since this PR wires--jsoninsrc/main.rs, this test should always parse stdout after the command succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/real_project_test.rs` around lines 155 - 170, The test currently allows non-JSON text when "--json" is used; change it so any non-empty stdout must be valid JSON by removing the fallback pass and always attempting to parse the output. In tests/real_project_test.rs, after calling run_bless(&project_dir, &["--json"]) on the output variable, call serde_json::from_str::<serde_json::Value>(trimmed) and expect a successful parse (i.e., fail the test if parsing errors) instead of only checking trimmed.starts_with('{') or '[' and returning early; ensure the assertion triggers whenever the command succeeds and stdout is non-empty so regressions in the JSON path are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/suggestions.json`:
- Around line 123-127: The mapping that replaces "proc-macro-error" with
"thiserror" is incorrect; update the suggestions.json entry for the object with
"pattern": "proc-macro-error" (currently "replacement": "thiserror", "kind":
"ModernAlternative") so it either (A) uses a correct compile-time diagnostic
alternative such as "proc-macro-error2" (or leaves replacement empty/null) and
adjusts "reason" to explain that proc-macro diagnostics require compile-time
APIs, or (B) remove the mapping entirely; ensure you modify the "replacement",
"kind", and "reason" fields for the entry referencing "proc-macro-error" rather
than introducing runtime-error crates like "thiserror".
In `@src/cli.rs`:
- Around line 41-47: Add a parse-time rejection for contradictory flags by
marking the two fields as conflicting in the CLI derive: annotate the audit_code
and no_audit_code fields with clap conflict attributes (e.g. add
#[arg(conflicts_with = "no_audit_code")] to the audit_code field and/or
#[arg(conflicts_with = "audit_code")] to the no_audit_code field) so the parser
will error if both --audit-code and --no-audit-code are passed; alternatively,
implement an early validation after parsing that checks opts.audit_code &&
opts.no_audit_code and returns a parse error—refer to the audit_code and
no_audit_code fields in the CLI struct to locate where to add this
check/attribute.
In `@src/code_audit.rs`:
- Around line 121-145: When diff_filter is set, we should skip files not in the
diff before reading and scanning to avoid full-workspace costs and inflated
files_scanned; modify the loop over files so that after the
is_ignored_path(file, config) check you test the diff_filter and skip the file
early if it is not part of the diff (i.e., call the appropriate include-check on
diff_filter for the current file path before fs::read_to_string), then only call
scan_code(&code, file, config) and increment the count that becomes
files_scanned for files you actually read/scan; keep existing behavior of
filtering alerts with filter.includes(alert) but ensure files_scanned reflects
only scanned files.
In `@src/fix.rs`:
- Around line 221-231: The current loop over
["dependencies","dev-dependencies","build-dependencies"] removes the crate and
returns on the first hit (using deps.remove(crate_name) and early return),
leaving other occurrences untouched; change the logic in the removal helper to
iterate all sections, call deps.remove(crate_name) for each, collect which
sections were modified (e.g., into a Vec<String>), and after the loop return a
single success message listing all modified sections (or bail if none were
removed); apply the same fix to the analogous helper mentioned at lines 237-248
so both functions remove the crate from every matching section instead of
stopping at the first.
- Around line 267-301: The code removes extra_crate from the manifest before
verifying main_crate exists, which can leave a partial edit; modify
apply_feature_opt() to first locate both crates (search doc for main_crate and
extra_crate across the sections array) and only perform the mutation if
main_crate is found, or if you prefer minimal change: record the removed
entry/value when you remove extra_crate and defer committing it—so if you later
fail to find main_crate, restore the removed entry into the same deps table;
reference the existing symbols doc, sections, extra_crate, main_crate,
extra_removed_section and use add_feature_to_dep only after confirming
main_crate exists (or after successfully committing both removal and
feature-add).
- Around line 144-179: The cargo check post-write currently only prints warnings
but doesn't signal failure to the caller, so make the cargo check result cause
the fix operation to fail: in the cargo check block in src/fix.rs (the
check_status match using check_status) return an Err (or propagate a non-ok
Result) when Ok(s) where s.success() is false or when Err(e) occurs, instead of
just printing a warning; ensure the apply() caller in src/main.rs receives that
error and causes the process to exit non-zero (or have apply() return a Result
that main treats as failure).
In `@src/main.rs`:
- Around line 20-23: The CLI scope flags (--workspace, --package, --all-targets)
are currently ignored because calls always pass only manifest; update the calls
to cargo_bless::parser::get_deps and cargo_bless::parser::scan_project (and any
other parser invocations around the opts.json branch and the other noted
locations) to forward the resolved scope from the CLI (e.g. fields on opts such
as workspace, package, all_targets) so the parser receives the intended scope;
adjust the argument list or construct a scope/config struct and pass it into
get_deps(manifest, scope) and scan_project(manifest, scope, ...), and update
cargo_bless::parser signatures if needed to accept and honor these scope flags.
In `@src/parser.rs`:
- Around line 80-86: The variable named `enabled_features` in `get_deps` (where
`MetadataCommand::features(CargoOpt::AllFeatures)` is used) is misleading
because it represents features from an all-features build; rename it to
something explicit like `all_features_build` or `features_with_all_enabled`,
update its documentation to state it reflects the all-features resolved plan
(not defaults), and adjust any other occurrences (including the similar block
around lines 132-140) to use the new name and docstring; ensure references to
`resolve.nodes[].features` and any downstream logic treating these as "actually
enabled in the resolved build" are updated to clarify the all-features context.
In `@src/policy.rs`:
- Around line 107-110: Change load_policy so it returns a Result<Option<Policy>,
E> instead of swallowing all errors into None: call fs::read_to_string(path) and
match the Err case to return Err unless the io::Error.kind() is NotFound (in
which case return Ok(None)); then call toml_edit::de::from_str(&content) and
return Err on parse errors (do not map them to None). Update the function
signature (load_policy) and callers accordingly and prefer a concrete error type
(or anyhow/Box<dyn Error>) so read and TOML deserialization failures are
propagated while only missing files produce Ok(None).
- Around line 123-126: The filter currently uses substring matching
(s.current.contains(p)) which erroneously suppresses packages by partial
matches; update the check in the filter closure that references
policy.ignore_packages and s.current to compare package names exactly by
splitting s.current on '+' (the same tokenization used elsewhere) and returning
false only if any token == p, i.e. perform an exact-name match against each
ignore_packages entry instead of contains.
In `@tests/fixtures/old-rust-project/src/main.rs`:
- Around line 6-10: The lazy_static! block has an invalid `static ref`
declaration for CONFIG; change the declaration inside lazy_static! so it uses
the correct syntax `static ref CONFIG: String = EXPR;` (i.e., add the `=` and
terminating `;`) and assign the initializer `"default_config".to_string()` to
CONFIG; locate the lazy_static! macro and the `static ref CONFIG: String` symbol
to update the declaration accordingly.
In `@tests/real_project_test.rs`:
- Around line 13-40: The test uses a brittle hardcoded bless_bin path and
ignores command exit status; replace bless_bin() to return
PathBuf::from(env!("CARGO_BIN_EXE_cargo-bless")) (or inline that env! call in
run_bless) and modify run_bless(project_dir, args) to check the Command output
status (output.status.success()) and panic or assert with output.stderr/stdout
on failure so tests fail on non-zero exit instead of silently returning stdout.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 155-169: The command currently returns Ok(()) without considering
opts.fail_on; add a check before the final Ok(()) that evaluates whether any
findings meet the threshold specified by opts.fail_on (for code audit results in
report when run_code_audit is true, and for lint/format suggestions in
suggestions), and if so return an error or non-zero exit (e.g., propagate a
failure Result) so cargo bless exits non‑zero when matching findings are
present; locate the check near the end of main where report, suggestions,
run_code_audit, and opts are in scope and use opts.fail_on to decide the exit
outcome.
---
Nitpick comments:
In `@tests/real_project_test.rs`:
- Around line 155-170: The test currently allows non-JSON text when "--json" is
used; change it so any non-empty stdout must be valid JSON by removing the
fallback pass and always attempting to parse the output. In
tests/real_project_test.rs, after calling run_bless(&project_dir, &["--json"])
on the output variable, call serde_json::from_str::<serde_json::Value>(trimmed)
and expect a successful parse (i.e., fail the test if parsing errors) instead of
only checking trimmed.starts_with('{') or '[' and returning early; ensure the
assertion triggers whenever the command succeeds and stdout is non-empty so
regressions in the JSON path are caught.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab1a038a-aa4a-4645-96f6-4e120d4f2db6
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktests/fixtures/old-rust-project/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.gitignoreCargo.tomlREADME.mddata/suggestions.jsonsrc/cli.rssrc/code_audit.rssrc/fix.rssrc/lib.rssrc/main.rssrc/output.rssrc/parser.rssrc/policy.rssrc/suggestions.rssrc/updater.rstests/fixtures/old-rust-project/Cargo.tomltests/fixtures/old-rust-project/src/main.rstests/integration.rstests/real_project_test.rs
Ruffian-L
force-pushed
the
release/0.1.1-code-audit
branch
2 times, most recently
from
b51eef1 to
cf789a8
Compare
Ruffian-L
force-pushed
the
release/0.1.1-code-audit
branch
from
cf789a8 to
79ad343
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification