feat(smitebot): add working doctor command (checks + --json + docs)#77
feat(smitebot): add working doctor command (checks + --json + docs)#77Ashish-Kumar-Dash wants to merge 11 commits into
Conversation
3561e70 to
60c0eb9
Compare
|
made the required changes after the review & squashed the commits to a single one |
NishantBansal2003
left a comment
There was a problem hiding this comment.
Just skimming through the changes (haven’t reviewed them deeply yet) but one main comment I wanted to add is that the code lacks Rust docs for each function, as well as some inline comments.
Also, I think the implementation could be simplified quite a bit and made more readable by following more idiomatic Rust patterns. Right now, I see quite a bit of redundancy in the code, such as repeated if/else patterns, duplicated check names like "Docker daemon reachable" etc etc.
I also think using proper error types would be better than returning strings explicitly.
You can also check https://github.com/dergoegge/fuzzamoto/tree/master/fuzzamoto-cli for some project/code structure ideas.
|
Refactored |
NishantBansal2003
left a comment
There was a problem hiding this comment.
I haven't reviewed the tests yet, but I think we can add more unit tests. I know there are some cases we can't test, but wherever possible, it would be great to add them, for example, tests for helper functions, error cases, and error display as well (since that is the most user visible part)
5a4e352 to
37627ef
Compare
|
Refactored Also moved repeated tool/script/dockerfile lists into constants and added more helper/error-display tests(now 11) |
NishantBansal2003
left a comment
There was a problem hiding this comment.
Some more comments (feel free to ignore some nits if you disagree)
I still think the test module can be improved. Also, could you add inline comments in the tests wherever some workaround is used to achieve a particular check case? I think that would make it easier to understand what is being done and why.
f3e9834 to
a4dfca6
Compare
|
ping @NishantBansal2003 , I split smitebot into a more modular CLI layout like the fuzzamoto CLI: main.rs is now just clap wiring, the |
74105fc to
a44d67e
Compare
… neccessary unit tests
NishantBansal2003
left a comment
There was a problem hiding this comment.
I think once these comments are addressed (in a fixup), this PR will be ready for Matt’s review
2f5b01f to
0307320
Compare
NishantBansal2003
left a comment
There was a problem hiding this comment.
I think this is ready for @morehouse review:
Some points I would like to discuss are:
- Currently,
aflpp_pathis optional and defaults to$PATH. Since we explicitly pass the AFL++ path during fuzzing, I think we should make it explicit here as well instead of implicitly taking it from$PATH - I couldn’t find anywhere that
libnyx.sois used viaLD_LIBRARY_PATH. From a high-level view of the AFL++ code, I only seelibnyx.sobeing resolved via$AFL_PATH/libnyx.so, so I don’t think checking viaLD_LIBRARY_PATHadds much value. I may be wrong though, so could you point me to the place that checks forlibnyx.soviaLD_LIBRARY_PATH? - If we default to using
$PATHforaflpp_path, then I thinkcheck_libnyxwill be incorrect. For a system-installed AFL++, afl-fuzz would typically be in/usr/local/bin/, while libnyx.so would be in/usr/local/lib(or similar), so this check would always fail (I think the same issue will happen withcheck_nyx_hgetas well)
agreed, AFL++ tools, Nyx hget, and libnyx.so are now validated only under the explicit --aflpp-path. PATH is still used only for host tools like docker,python,bash
That LD_LIBRARY_PATH check came from my prev assumption that libnyx.so might be loaded through the dynamic linker search path. After going through the repo again, I couldn’t find a Smite dependency on LD_LIBRARY_PATH for libnyx.so, and your point about AFL++ resolving it via AFL_PATH makes sense. I’ve removed that check and now validate libnyx.so only under the explicit --aflpp-path. |
|
Once you approve these changes, I'll go ahead with asking for Matt's review |
|
@morehouse you may take a look, I think the PR is ready to be reviewed |
Summary
This PR introduces the first working
smitebotcommand:doctor.It adds
smitebotcrate and implements prerequisite checks needed before running Smite campaigns, with both human and machine readable(JSON) output.What’s included
smitebotas a workspace member.smitebotcrate dependencies (clap,serde,serde_json,thiserror).smitebot doctorcommand (no stub-only command surface).--jsonoutput for CI/machine parsing.smitebot doctorusage.Checks implemented
x86_64architecture/dev/kvmaccessibledocker info)afl-fuzzafl-cminafl-tminafl-whatsuphgetpresencelibnyx.sodiscoverable viaLD_LIBRARY_PATHValidation
cargo fmt --all --check cargo clippy -p smitebot --all-targets -- -D warnings cargo test -p smitebot cargo run -p smitebot -- doctor cargo run -p smitebot -- doctor --jsonping @morehouse @NishantBansal2003