feat: use os system for CLI and better labs #649
Conversation
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8390 🏁 Script executed: Length of output: 64459 @ocervell I've reviewed the PR and it looks good! Here are my observations: Positive Changes
ImplementationThe code changes are clean and well-structured:
This PR looks ready to merge! |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new flag to the worker CLI command to control command execution method, update test subcommands to use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Command
participant OS
User->>CLI: Run worker/test command (with flags)
alt --use-command-runner flag set
CLI->>Command: execute(command)
Command-->>CLI: return code
CLI->>User: Exit with return code
else use_os_system True
CLI->>OS: os.system(command)
OS-->>CLI: status code
CLI->>User: Exit with status code
end
sequenceDiagram
participant User
participant CLI
participant Env
participant SetupScript
participant TeardownScript
User->>CLI: Run integration test with --no-cleanup
CLI->>Env: Set TEST_NO_CLEANUP=1
CLI->>SetupScript: Execute setup.sh
SetupScript->>Env: Check TEST_NO_CLEANUP
alt TEST_NO_CLEANUP=1
SetupScript->>SetupScript: Skip pull
else
SetupScript->>SetupScript: Run pull
end
CLI->>TeardownScript: Execute teardown.sh
TeardownScript->>Env: Check TEST_NO_CLEANUP
alt TEST_NO_CLEANUP=1
TeardownScript->>TeardownScript: Skip cleanup
else
TeardownScript->>TeardownScript: Run docker compose down -v
end
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
secator/cli.py (2)
472-485: 🛠️ Refactor suggestionAddress cross-platform compatibility issue
The output redirection
>/dev/null 2>&1is Unix-specific and will fail on Windows systems. Consider using a cross-platform approach.- if not verbose: - cmd += ' >/dev/null 2>&1' + if not verbose: + import os + devnull = os.devnull + cmd += f' >{devnull} 2>&1' if os.name != 'nt' else ' >nul 2>&1'Alternatively, consider using Python's
subprocessmodule withDEVNULLfor better cross-platform support.
478-485: 💡 Verification agent🧩 Analysis chain
Consider exit code handling differences
The exit code handling differs between
os.systemandCommand.execute. On Unix systems,os.systemreturns the exit status in the format returned bywait(), which may need processing.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any platform-specific exit code handling requirements # Expected: Verify if os.system return codes are handled consistently echo "Searching for other os.system usages in the codebase..." rg "os\.system" --type py -A 2 -B 2 echo "Checking for exit code handling patterns..." rg "sys\.exit.*ret" --type py -A 1 -B 1Length of output: 2157
Unify exit‐code handling between os.system and Command.execute
On Unix,
os.systemreturns a status word (signal and exit code combined), whereasCommand.execute(...).return_codeis the raw exit code. Directly passingretfromos.systemintosys.exit(ret)can yield unexpected exit codes.Please update all
os.systemusages to extract the real exit code (e.g. viaos.WEXITSTATUS(ret)) or switch to usingCommand.execute.Locations to address in secator/cli.py:
- Around each
ret = os.system(cmd)/sys.exit(ret)block (several occurrences)Suggested diff:
- ret = os.system(cmd) - if exit: - sys.exit(ret) + ret = os.system(cmd) + # on Unix, os.system returns a status word; extract the real exit code + code = os.WEXITSTATUS(ret) if hasattr(os, 'WEXITSTATUS') else ret + if exit: + sys.exit(code)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
secator/cli.py(8 hunks)secator/runners/_base.py(1 hunks)tests/integration/setup.sh(1 hunks)tests/integration/teardown.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
secator/runners/_base.py (1)
secator/utils.py (1)
debug(342-378)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration (3.11, ubuntu-latest)
🔇 Additional comments (7)
secator/cli.py (3)
136-137: LGTM: Clean flag implementationThe new
--use-command-runnerflag is properly implemented with appropriate help text and default value.
546-547: Well-implemented environment variable integrationThe
--no-cleanupflag properly sets theTEST_NO_CLEANUPenvironment variable, which integrates correctly with the test scripts.Also applies to: 554-554
506-506: Consistent adoption of os.system executionAll test commands are consistently updated to use
use_os_system=True, aligning with the performance improvement objectives.Also applies to: 538-538, 571-571, 625-625, 796-796
secator/runners/_base.py (1)
548-548: Improved debug output controlThe change to use
print_this_typefor theverboseparameter is a good improvement - it makes debug output more contextually relevant by only being verbose when the item type should actually be printed. The simplified sub-category from'item.print'to'item'also improves clarity.tests/integration/teardown.sh (1)
14-17: Clean conditional cleanup implementationThe environment variable check is properly implemented with clear messaging and appropriate exit behavior. Using
exit 0is correct since aborting cleanup due toTEST_NO_CLEANUP=1is expected behavior, not an error condition.tests/integration/setup.sh (2)
14-16: Smart conditional image pullingThe logic correctly skips image pulling when
TEST_NO_CLEANUP=1, which will save significant time during iterative development. Using!= "1"ensures images are pulled by default, which is the safer behavior.
17-17: Enhanced reliability with docker compose flagsThe addition of
--waitand--no-recreateflags improves the reliability and performance of the setup process:
--waitensures containers are fully ready before proceeding--no-recreateavoids unnecessary container recreation whenTEST_NO_CLEANUP=1
🤖 I have created a release *beep* *boop* --- ## [0.16.0](v0.15.1...v0.16.0) (2025-06-05) ### Features * **`dnsx`:** merge `dnsxbrute` into `dnsx` ([#571](#571)) ([d30a497](d30a497)) * add task revoke state and perf improvements ([#678](#678)) ([2a3bf08](2a3bf08)) * allow returning errors in hooks ([#632](#632)) ([39a56bd](39a56bd)) * improve bbot output types ([#627](#627)) ([3b0aa5d](3b0aa5d)) * improve runner logic, workflow building, results filtering logic; and add config defaults for profiles & drivers ([#673](#673)) ([df94657](df94657)) * improve template loading flow ([#667](#667)) ([f223120](f223120)) * memory optimizations ([#681](#681)) ([d633133](d633133)) * **misc:** condition-based runs, chunked_by opts, dynamic task profiles, cli improvements ([#659](#659)) ([e8225cd](e8225cd)) * **runner:** add input validation to all tasks and workflows ([#663](#663)) ([8392551](8392551)) * **runner:** improve option handling ([#670](#670)) ([59b1c68](59b1c68)) * **scans:** improve scans ([#660](#660)) ([bdd38ec](bdd38ec)) * use os system for CLI and better labs ([#649](#649)) ([8b49912](8b49912)) * **workflow:** improve subdomain_recon workflow ([#657](#657)) ([bc65092](bc65092)) ### Bug Fixes * allow dry-run mode to work without targets ([#624](#624)) ([cccffb9](cccffb9)) * check task is registered before running test ([1f5cd83](1f5cd83)) * formatting for dynamic opts ([#628](#628)) ([dcbbfe9](dcbbfe9)) * header options conversion ([#633](#633)) ([6ae8423](6ae8423)) * header parsing ([#629](#629)) ([db2f028](db2f028)) * improve mongodb duplicate checker ([#626](#626)) ([bf277a9](bf277a9)) * **installer:** compound distro.like() on distribs like popos ([#653](#653)) ([3687e1d](3687e1d)) * **installer:** ignore dev/post release from PyPI ([#634](#634)) ([614c3e2](614c3e2)) * **installer:** secator update with correct package version ([#648](#648)) ([a9cf189](a9cf189)) * lab --wait not in gitlab runner ([070ae84](070ae84)) * logic to test all tasks ([3bd7503](3bd7503)) * os.system return code ([02aed75](02aed75)) * progress type fields ([#652](#652)) ([f146914](f146914)) * remove duplicates from txt exporter ([#630](#630)) ([88ba5c5](88ba5c5)) * remove fping -r flag by default, show alive hosts better ([#665](#665)) ([5c945fd](5c945fd)) * remove no-recreate flag in labs as not supported by github runner ([bd997a8](bd997a8)) * short opt incorrectly named ([#631](#631)) ([0c73c60](0c73c60)) * tasks with no file flag need input_chunk_size=1 ([#668](#668)) ([a088c94](a088c94)) * tools in readme, arjun chunk and ffuf header ([#679](#679)) ([654ff30](654ff30)) * tools table generator update ([9420f14](9420f14)) * update ci workflow ([f4c2b13](f4c2b13)) * update generate table workflow ([ff62702](ff62702)) * vulnerability output reference when unset ([#625](#625)) ([a656fbf](a656fbf)) ### Documentation * generate tools table md ([#610](#610)) ([d60f11e](d60f11e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary by CodeRabbit