fix: replace .unwrap() with .expect() in non-test scan paths (refs #275)#279
Conversation
- Add Cargo seeds: k256, secp256k1, libsecp256k1, ed448-goldilocks (tier 1), openssl (tier 2) - Add pip seeds: pyjwt, authlib, python-jose, jwcrypto (0.8), m2crypto (0.6) - Fix fabric: drop misleading RSA algorithm (wraps paramiko, no crypto itself)
- semgrep_compat: refactor is_none()+unwrap() to let-else
- semgrep_compat: expect("checked len == 1") on single-positive path
- semgrep_taint: expect on find('.') guarded by prior rfind
- scanner: expect on regex group 0 and Mutex::lock
- terminal: expect on last_mut after push
- foxguard_mcp: expect on json!() serialization
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR improves error handling throughout the codebase by replacing generic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rules/manifest.rs (1)
296-317:⚠️ Potential issue | 🟠 MajorAdd deterministic tie-breaker for equal confidence values in best-seed selection.
HashMap::values()iteration order is non-deterministic. When multiple seeds have equal confidence (e.g., 0.9), the selectedbestseed varies across runs. Add seed name as a secondary sort criterion to ensure stable behavior.Suggested fix
let best = reached_seeds .values() + .copied() .max_by(|a, b| { a.confidence .total_cmp(&b.confidence) + .then_with(|| a.name.cmp(b.name)) }) .unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/manifest.rs` around lines 296 - 317, The best-seed selection can be non-deterministic when two SeedEntry items have equal confidence; update the update logic in the reached_seeds insertion (the block using reached_seeds.entry(entry.name).and_modify(...).or_insert(entry)) to use a deterministic tie-breaker: replace the existing entry when entry.confidence > existing.confidence OR when entry.confidence == existing.confidence AND entry.name is lexicographically less (or otherwise deterministic) than existing.name; keep using SeedEntry, reached_seeds, seed_map and entry.name to locate and compare entries so ties break consistently by seed name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/rules/manifest.rs`:
- Around line 296-317: The best-seed selection can be non-deterministic when two
SeedEntry items have equal confidence; update the update logic in the
reached_seeds insertion (the block using
reached_seeds.entry(entry.name).and_modify(...).or_insert(entry)) to use a
deterministic tie-breaker: replace the existing entry when entry.confidence >
existing.confidence OR when entry.confidence == existing.confidence AND
entry.name is lexicographically less (or otherwise deterministic) than
existing.name; keep using SeedEntry, reached_seeds, seed_map and entry.name to
locate and compare entries so ties break consistently by seed name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aeb4c395-d056-461b-9e75-7f02db223d11
📒 Files selected for processing (7)
src/bin/foxguard_mcp.rssrc/engine/scanner.rssrc/report/sarif.rssrc/report/terminal.rssrc/rules/manifest.rssrc/rules/semgrep_compat.rssrc/rules/semgrep_taint.rs
Greptile SummaryThe seven targeted Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/transparency concerns with no runtime defects. Every src/rules/manifest.rs and src/report/sarif.rs — contain undocumented behavioral changes that deserve explicit mention in the PR description. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["scan_files()"] -->|"lock().expect('lock poisoned')"| B["Mutex warnings"]
A --> C["parse_block_comment_ignore()"]
C -->|"captures.get(0).expect('group 0 always present')"| D["Regex group 0"]
E["compile_pattern()"] -->|"if let Some(dot) = rfind('.')"| F{"dot found?"}
F -->|"yes: find('.').expect('rfind guarantees dot')"| G["Split root / field"]
F -->|no| H["return None"]
I["build_matcher()"] -->|"positives.len()==1 guard"| J["next().expect('checked len==1')"]
K["match_single_pattern()"] -->|"let Some(pat_node) = ... else"| L["return results"]
M["print_findings_full()"] -->|"push() then last_mut().expect('just pushed')"| N["by_file Vec"]
O["format_scan_result()"] -->|"to_string().expect('json!() always serializable')"| P["MCP JSON response"]
subgraph manifest_rs ["manifest.rs (undocumented extras)"]
Q["Vec reached_seeds"] -->|"changed to"| R["HashMap reached_seeds"]
R -->|"deduplicates by seed name"| S["values().max_by().unwrap() — guarded"]
T["fabric crypto_algorithm"] -->|"Some('RSA') → None"| U["no algo attributed"]
V["5 new Cargo seeds + 4 new PIP packages"] -.-> R
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let best = reached_seeds | ||
| .iter() | ||
| .values() | ||
| .max_by(|a, b| a.confidence.total_cmp(&b.confidence)) | ||
| .unwrap(); |
There was a problem hiding this comment.
.unwrap() inconsistent with PR goal
Line 332 still contains an un-replaced .unwrap() on max_by(...):
let best = reached_seeds
.values()
.max_by(|a, b| a.confidence.total_cmp(&b.confidence))
.unwrap();It is safe (guarded by the is_empty() check at line 324), but it is inconsistent with the stated goal of the PR. A simple .expect("reached_seeds non-empty: guarded by is_empty check above") would make the invariant explicit and match the style applied to every other site in this diff.
| let best = reached_seeds | |
| .iter() | |
| .values() | |
| .max_by(|a, b| a.confidence.total_cmp(&b.confidence)) | |
| .unwrap(); | |
| let best = reached_seeds | |
| .values() | |
| .max_by(|a, b| a.confidence.total_cmp(&b.confidence)) | |
| .expect("reached_seeds non-empty: guarded by is_empty check above"); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Approved — 7 |
2 participants
Summary
.unwrap()calls in non-test code with.expect()orlet-elseFiles changed
semgrep_compat.rs—let-elserefactor +expecton checked lengthsemgrep_taint.rs—expectonfind('.')guarded by priorrfindscanner.rs—expecton regex group 0 + Mutex lockterminal.rs—expectonlast_mutafter pushfoxguard_mcp.rs—expectonjson!()serializationTest plan
cargo test— all tests pass (1 pre-existing failure on main unrelated to this PR)cargo fmt --check— cleanSummary by CodeRabbit
Bug Fixes
Refactor