feat(cli): load rules database from file - #78
Conversation
|
@alexsmolya Welcome, and thanks for the contribution! Nice to have you here. 🙌 I went through the implementation, the tests, the contributing guide and the docs. The change is nicely scoped: it stays at the CLI boundary, fails before the scan starts if the file is bad, and keeps stdout clean in JSON mode. Test coverage for embedded / valid / missing / malformed is good to see. Left a few comments inline. Only the docs one really blocks, the rest we can talk through. Once that's sorted I'm happy to approve and merge. |
|
|
||
| ### Added | ||
|
|
||
| - `diskern scan --rules <file>` to test scans with an external rules database |
There was a problem hiding this comment.
Good to have the entry here. Could you also add a row for --rules <file> to the flags table in crates/diskern-cli/README.md? It's user facing, so it should sit with --top, --verdict and --json rather than only in the CHANGELOG.
There was a problem hiding this comment.
Fixed: added --rules <file> to the CLI flags table in crates/diskern-cli/README.md.
| path.display() | ||
| ) | ||
| })?; | ||
| serde_json::from_slice(&contents) |
There was a problem hiding this comment.
One thing worth guarding: rules::compile throws away patterns that aren't valid globs and only reports it through tracing::warn!, and the CLI doesn't install a subscriber. So a typo in a pattern matches nothing and the scan still exits 0.
Could we compile the patterns in load_rules and error out early? diskern-core keeps the shipped db honest the same way in every_shipped_pattern_is_a_valid_glob.
There was a problem hiding this comment.
Implemented: external rule patterns are compiled and invalid globs are rejected before scanning, with regression coverage.
| verdict, | ||
| rules, | ||
| } => { | ||
| let external_rules = rules.as_deref(); |
There was a problem hiding this comment.
Because the external file replaces the whole db, the protected rules go with it. A rules file with a safe rule for **/*.msi would list C:/Windows/Installer/*.msi under "Safe to remove", which is the case the rule ordering in base.json is there to prevent.
Would you be open to keeping the embedded protected rules in front of the external ones, or warning when the loaded db has none? Happy to go either way.
There was a problem hiding this comment.
Implemented: embedded protected rules are evaluated before external rules, with regression coverage preventing external rules from overriding them.
| } | ||
| } | ||
|
|
||
| fn load_rules(path: Option<&std::path::Path>) -> Result<RulesDb> { |
There was a problem hiding this comment.
Optional: version is read but never checked, so a file written for a later schema would load quietly with today's semantics. A small check that bails on an unknown version would make the field worth carrying.
There was a problem hiding this comment.
The schema version is intentionally unchanged: the repository defines no compatibility or rejection policy for schema versions, so introducing a check here would invent policy beyond this change.
|
On the docs point, this is the table I meant: diskern/crates/diskern-cli/README.md Lines 51 to 55 in 60d5f1b A row right under | |
2 participants
What & why
Fixes #68
diskern scancan now accept an optional external rules database:Without
--rules, the CLI continues to use the embedded rules database. A supplied file is deserialized through the existingRulesDbmodel, with explicit errors for unreadable files and malformed JSON. Human-readable output identifies the external rules source, while JSON output remains valid on stdout. The change stays at the CLI boundary and does not move classification logic out ofdiskern-core.The user-visible feature is documented in
CHANGELOG.md.Validation
cargo fmt --allcargo +1.96.0 clippy -p diskern-core -p diskern-cli --all-targets -- -D warningscargo +1.96.0 test -p diskern-core -p diskern-cligit diff --checkAll passed. The focused suite includes regression coverage for embedded rules, valid external rules, missing files, and malformed JSON.
Checklist
cargo fmt --alland the required core/CLI clippy validation are clean