Skip to content

refactor: unify rule loading through merge_rules - #57

Open
Adityakk9031 wants to merge 3 commits into
Corgea:mainfrom
Adityakk9031:#56
Open

refactor: unify rule loading through merge_rules#57
Adityakk9031 wants to merge 3 commits into
Corgea:mainfrom
Adityakk9031:#56

Conversation

@Adityakk9031

@Adityakk9031 Adityakk9031 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #56

Description

This PR fixes a potential process panic (called Option::unwrap() on a None value) in src/scanner/modes.rs when loading scan rules.

Previously, load_explicit_scan_rules and load_rules_for_detected_language called .unwrap() directly on all_rules.into_iter().next() when single-element rule lists were expected. If custom or file-based rules failed to yield matching definitions (e.g. when using --use-file-rules with an empty or non-matching rules directory), this resulted in an unhandled CLI thread panic.

This change replaces .unwrap() with safe .pop().ok_or_else(...) error handling that returns a clean anyhow::Result::Err to the user instead of panicking.

Changes Made

  • src/scanner/modes.rs: Replaced .next().unwrap() calls with .pop().ok_or_else(...) and explicit empty-vector checks in both load_explicit_scan_rules and load_rules_for_detected_language.
  • src/scanner/modes.rs: Added unit test load_explicit_scan_rules_handles_nonexistent_rules_gracefully to prevent future regressions.

Testing & Verification

  • Formatted with cargo fmt.
  • Ran cargo test: all 105 unit tests and doc tests passed successfully.

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@juangaitanv and @Ibrahimrahhal have a look

@yhoztak
yhoztak requested a review from juangaitanv July 23, 2026 23:36
Comment thread src/scanner/modes.rs Outdated
Comment thread src/scanner/modes.rs
@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@juangaitanv have a look

@juangaitanv

Copy link
Copy Markdown
Contributor

@juangaitanv have a look
@Adityakk9031 could you address this comment please #57 (comment)?

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@juangaitanv You're completely right all_rules was already guaranteed non-empty because invalid rule paths return an Err early at Rules::load_from_path(rules_path)?.

In commit 5ca93e7, we resolved this by removing the redundant .unwrap() branch entirely and delegating directly to Rules::merge_rules(all_rules), which cleanly handles both single and merged rule sets.

@juangaitanv

Copy link
Copy Markdown
Contributor

@juangaitanv You're completely right all_rules was already guaranteed non-empty because invalid rule paths return an Err early at Rules::load_from_path(rules_path)?.

In commit 5ca93e7, we resolved this by removing the redundant .unwrap() branch entirely and delegating directly to Rules::merge_rules(all_rules), which cleanly handles both single and merged rule sets.

🙏. could you address this comment pleaes #57 (comment)?

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@juangaitanv I've updated the PR with a regression unit test load_rules_for_detected_language_returns_none_when_no_rules_found in src/scanner/modes.rs.

This test configures --use-file-rules with an empty rules directory and verifies that load_rules_for_detected_language handles empty rule sets safely and returns Ok(None) cleanly.

@juangaitanv
juangaitanv self-requested a review August 7, 2026 13:49

@juangaitanv juangaitanv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The refactor is behavior-identical (merge_rules on a one-element vec reproduces the single-rules case, empty paths stay guarded) and the new tests pass.

One note on framing: the removed .unwrap() was provably safe under the len() == 1 guard, so this is a simplification, not a bug fix. Consider retitling the PR (e.g. "refactor: unify rule loading through merge_rules") so the history reflects that.

@Adityakk9031 Adityakk9031 changed the title fix(scanner): replace panic-prone iterator .unwrap() with safe Option… refactor: unify rule loading through merge_rules Aug 12, 2026
@Adityakk9031

Copy link
Copy Markdown
Contributor Author

Thanks @juangaitanv for the review and approval!

I've updated the PR title to refactor: unify rule loading through merge_rules as suggested to accurately reflect the refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Unhandled process panic in rule loading via direct .unwrap()

2 participants