feat:add headless arg to help #629
Conversation
WalkthroughA new debugging configuration named "Safety --help" has been added to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
safety/scan/constants.py (1)
19-25: LGTM! Clear and well-structured help text.The updated help text effectively communicates both standard and headless authentication options with clear examples.
Consider adding the
[nhc](no highlight color) tags around the URL for consistency with other help texts in the file:CLI_AUTH_COMMAND_HELP = ( "Authenticate with Safety CLI to perform scans.\n" - "If already authenticated, your default browser will automatically open to https://platform.safetycli.com." + "If already authenticated, your default browser will automatically open to [nhc]https://platform.safetycli.com[/nhc]." "\nExample: safety auth login\n" "For headless authentication, you will receive a URL to paste into an external browser." "\nExample: safety auth login --headless" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.vscode/launch.json(1 hunks)safety/scan/constants.py(1 hunks)
🔇 Additional comments (2)
.vscode/launch.json (1)
4-13: LGTM! The debug configuration is well-structured.
The new "Safety --help" configuration follows the established pattern and maintains consistency with other configurations in the file.
safety/scan/constants.py (1)
19-25: Verify documentation and usage consistency.
Let's ensure the headless authentication is properly documented across the codebase.
✅ Verification successful
Documentation and implementation are consistent with headless authentication
The changes to CLI_AUTH_COMMAND_HELP are properly reflected across the codebase:
- The headless feature is documented in CHANGELOG.md (version 3.1.0)
- The auth CLI implementation in
safety/auth/cli.pyincludes the headless flag - The README.md provides basic auth instructions without being too detailed, which is appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related documentation and usage of auth commands
# Test 1: Look for documentation files that might need updates
echo "Checking documentation files..."
rg -l "auth.*login" --type md
# Test 2: Look for test files that might need updates for headless mode
echo "Checking test files..."
rg -l "CLI_AUTH.*HELP" --type py
# Test 3: Look for command implementation files
echo "Checking command implementations..."
rg -l "auth.*login.*headless" --type py
Length of output: 409
Script:
#!/bin/bash
# Let's check the content of these files to verify consistency
# Check README.md content around auth login
echo "=== README.md auth login section ==="
rg -A 5 -B 5 "auth.*login" README.md
# Check CHANGELOG.md for related changes
echo -e "\n=== CHANGELOG.md relevant entries ==="
rg -A 2 -B 2 "auth.*login|headless" CHANGELOG.md
# Check auth CLI implementation
echo -e "\n=== auth/cli.py implementation ==="
rg -A 10 -B 2 "CLI_AUTH.*HELP|auth.*login.*headless" safety/auth/cli.py
Length of output: 3500
There was a problem hiding this comment.
Hey @SafetyQuincyF this is great! I was thinking we should add a line to our options section of the help:
SafetyQuincyF
force-pushed
the
feat-add-headless-to-help
branch
2 times, most recently
from
82a2222 to
7ff780b
Compare
|
I am benching this due to complexity adding --headless to options would require a rewrite of how auth login is done. |
SafetyQuincyF
force-pushed
the
feat-add-headless-to-help
branch
from
e6d685c to
9b204de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
safety/auth/cli_utils.py (1)
14-14: Clean up unused imports.Remove the unused
CLI_STAGE_HELPimport. Additionally, verify if theStageimport is still needed since the stage parameter has been removed fromauth_options.-from safety.scan.constants import CLI_HEADLESS_AUTH_HELP, CLI_KEY_HELP, CLI_PROXY_HOST_HELP, CLI_PROXY_PORT_HELP, CLI_PROXY_PROTOCOL_HELP, CLI_STAGE_HELP +from safety.scan.constants import CLI_HEADLESS_AUTH_HELP, CLI_KEY_HELP, CLI_PROXY_HOST_HELP, CLI_PROXY_PORT_HELP, CLI_PROXY_PROTOCOL_HELP🧰 Tools
🪛 Ruff
14-14:
safety.scan.constants.CLI_STAGE_HELPimported but unusedRemove unused import:
safety.scan.constants.CLI_STAGE_HELP(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.vscode/launch.json(1 hunks)safety/auth/cli_utils.py(2 hunks)safety/cli.py(3 hunks)safety/scan/constants.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .vscode/launch.json
- safety/scan/constants.py
🧰 Additional context used
🪛 Ruff
safety/auth/cli_utils.py
14-14: safety.scan.constants.CLI_STAGE_HELP imported but unused
Remove unused import: safety.scan.constants.CLI_STAGE_HELP
(F401)
🔇 Additional comments (3)
safety/auth/cli_utils.py (1)
139-153:
Critical: Incomplete implementation of headless authentication.
The PR objective mentions that implementing the headless option requires rewriting the authentication login process. However, the current changes only add the CLI option without:
- Propagating the headless flag through the auth flow
- Updating the
inject_sessionfunction to handle headless authentication - Removing the
stageparameter frominject_session's inner function
Let's verify the auth flow implementation:
Would you like assistance in implementing the complete headless authentication flow?
Also applies to: 167-238
safety/cli.py (2)
470-470: LGTM! Consistent with previous auth_options change
The removal of stage=False parameter is consistent with the change in the check command, maintaining uniformity in the authentication logic.
281-281: Verify removal of stage parameter from auth_options
The removal of stage=False parameter from auth_options decorator appears to be part of authentication logic simplification.
✅ Verification successful
Removal of stage parameter from auth_options is consistent
The verification shows that:
- The
stageparameter has been completely removed from theauth_optionsdecorator implementation insafety/auth/cli_utils.py - All usages of
auth_optionsin the codebase have been updated to remove thestageparameter - Git history confirms this was a deliberate change, removing
stage=Falsefrom all command decorators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of stage parameter in auth_options
# Test: Search for auth_options with stage parameter. Expect: No occurrences.
rg -A 2 "@auth_options\(.*stage.*\)"
Length of output: 38
Script:
#!/bin/bash
# Let's first check all usages of auth_options decorator to understand the pattern
rg -B 1 -A 2 "@auth_options"
# Also check the implementation of auth_options to understand the parameters
fd "auth_options" --exec cat {}
Length of output: 786
Script:
#!/bin/bash
# Let's check the auth_options implementation in the codebase
rg -B 5 -A 10 "def auth_options"
# Also check for any historical usage of stage parameter
git log -p | rg -B 3 -A 3 "auth_options.*stage"
Length of output: 2842
safety/auth/cli_utils.py
Outdated
| func = click.option("--headless", | ||
| help=CLI_HEADLESS_AUTH_HELP)(func) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotation and default value for the headless option.
The --headless option should include a type annotation and default value for consistency with other CLI options.
- func = click.option("--headless",
- help=CLI_HEADLESS_AUTH_HELP)(func)
+ func = click.option("--headless", is_flag=True, default=False,
+ help=CLI_HEADLESS_AUTH_HELP)(func)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func = click.option("--headless", | |
| help=CLI_HEADLESS_AUTH_HELP)(func) | |
| func = click.option("--headless", is_flag=True, default=False, | |
| help=CLI_HEADLESS_AUTH_HELP)(func) |
safety/cli.py
Outdated
| @@ -703,8 +703,7 @@ def configure(ctx, proxy_protocol, proxy_host, proxy_port, proxy_timeout, | |||
| for key, value in proxy_config.items(): | |||
| config.set(PROXY_SECTION_NAME, key, value) | |||
|
|
|||
| if stage: | |||
| config['host'] = {'stage': "development" if stage == "dev" else stage} | |||
|
|
|||
There was a problem hiding this comment.
Critical: Remove unused stage parameter from configure function
The stage parameter is referenced in the function signature but was removed from the parameters list. This could lead to runtime errors.
Apply this diff to fix the issue:
def configure(ctx, proxy_protocol, proxy_host, proxy_port, proxy_timeout,
- proxy_required, organization_id, organization_name, stage,
+ proxy_required, organization_id, organization_name,
save_to_system):Committable suggestion skipped: line range outside the PR's diff.
SafetyQuincyF
force-pushed
the
feat-add-headless-to-help
branch
from
9b204de to
622c909
Compare
Description
Add --headless to help option for safety.
Type of Change
Testing
Checklist
Summary by CodeRabbit
New Features
Documentation