-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Add --always-approve and --llm-approve CLI arguments for confirmation mode #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…mation mode This commit adds two new command-line arguments to allow users to specify the confirmation mode when launching the OpenHands CLI: - --always-approve: Enables always approve mode, where all actions are approved by the user before execution - --llm-approve: Enables LLM-based security analyzer mode, where only HIGH-risk actions require user confirmation Changes: - Added mutually exclusive --always-approve and --llm-approve arguments to main_parser.py with updated help examples - Updated simple_main.py to parse and pass confirmation_mode to run_cli_entry() - Modified agent_chat.py to accept confirmation_mode parameter and display the mode status on startup - Enhanced setup.py to support different confirmation modes with appropriate policies (AlwaysConfirm, ConfirmRisky, or NeverConfirm) - Added comprehensive tests for the new CLI arguments and confirmation modes Fixes #107
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
| confirmation_group.add_argument( | ||
| "--llm-approve", | ||
| action="store_true", | ||
| help="Enable LLM-based security analyzer (only confirm high-risk actions)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| help="Enable LLM-based security analyzer (only confirm high-risk actions)", | |
| help="Enable LLM-based security analyzer (only confirm LLM-predicted high-risk actions)", |
openhands_cli/simple_main.py
Outdated
| if args.always_approve: | ||
| confirmation_mode = "always" | ||
| elif args.llm_approve: | ||
| confirmation_mode = "llm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto - we should use the type we defined here
openhands_cli/setup.py
Outdated
| conversation.set_security_analyzer(LLMSecurityAnalyzer()) | ||
| conversation.set_confirmation_policy(ConfirmRisky(threshold=SecurityRisk.HIGH)) | ||
| else: | ||
| # Legacy support for include_security_analyzer parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we use include_security_analyzer here? Can we refactor it so we can get rid of the else branch here
|
@OpenHands review all comments in this PR and address them |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
- Updated --llm-approve help text for clarity - Added ConfirmationMode Literal type for type safety - Refactored setup.py to remove legacy include_security_analyzer parameter - Updated all affected code and tests to use confirmation_mode parameter - All tests passing (155/155) - All lint checks passing
SummaryI've successfully reviewed and addressed all 4 PR comments on PR #110. The changes have been committed and pushed to update the PR. ✅ PR Review Comments Addressed
🔧 Supporting Changes
✅ Verification
The PR is now updated and ready for re-review! |
… values Changed ConfirmationMode type from Literal['always', 'llm'] to Literal['always-approve', 'llm-approve'] to match the CLI argument names (--always-approve and --llm-approve) for better clarity and consistency. This makes the internal representation align with the user-facing CLI argument names, reducing confusion and improving code readability. Updated files: - openhands_cli/user_actions/types.py: Changed ConfirmationMode type - openhands_cli/simple_main.py: Use 'always-approve' and 'llm-approve' - openhands_cli/setup.py: Update confirmation mode checks - openhands_cli/agent_chat.py: Update confirmation mode display logic - openhands_cli/runner.py: Update toggle_confirmation_mode to use 'always-approve' - tests/test_main.py: Update test expectations - tests/test_confirmation_mode.py: Update test expectations All tests passing and linting checks pass. Co-authored-by: openhands <[email protected]>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
@OpenHands please fix the failing actions on PR #110 at branch |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
…ation-mode-cli-args
…ays' The confirmation mode values were refactored to use 'always-approve' and 'llm-approve' instead of 'always' and 'llm'. This commit updates the test expectations in test_confirm_command.py to match the actual values being used in the code.
✅ Successfully Fixed Failing Actions on PR #110I've successfully fixed the failing tests on PR #110 and merged the latest changes from main. Here's what was done: 🔧 Issue IdentifiedThe unit tests were failing because:
🛠️ Changes Made
✅ Verification
📝 CommitThe fix has been committed and pushed to branch
PR #110 is now ready with all checks passing! 🎉 View the PR: #110 |
|
@OpenHands please merge from main and resolve conflicts |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
- Merged main branch changes including --task and --file CLI arguments - Resolved conflicts in main_parser.py, agent_chat.py, simple_main.py, and test_main.py - Updated run_cli_entry() to accept both confirmation_mode and queued_inputs parameters - Merged examples in CLI help text to show both confirmation mode and task/file options - Updated tests to check both parameters - Fixed linting issues (line length in help examples) - All tests passing ✅
✅ Successfully Merged from Main and Resolved All ConflictsI have successfully merged the latest changes from the Summary of ChangesConflicts Resolved:
Additional Changes:
Testing Results:
What Was Merged from MainThe merge brought in the following new features from
Branch StatusThe branch
You can view the updated PR at: #110 |
|
@OpenHands pls merge from main and resolve conflicts |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
Resolved conflicts: - openhands_cli/argparsers/main_parser.py: Kept confirmation mode arguments and updated examples with ACP subcommand - uv.lock: Used main's version for regeneration Co-authored-by: openhands <[email protected]>
✅ Successfully Merged from Main and Resolved ConflictsI've successfully merged the latest changes from the 🔧 Conflicts Resolved
📊 Changes Merged from MainThe merge brought in significant updates from main (9 commits), including:
✅ Verification
The PR #110 has been updated with the merge commit and is now up-to-date with main while preserving all the confirmation mode functionality. |
openhands_cli/setup.py
Outdated
| else: | ||
| # Handle confirmation mode | ||
| if confirmation_mode == "always-approve": | ||
| # Always ask for confirmation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands always-approve should be "NEVER ASK FOR confirmation"
Maybe we should include "always-ask" into Confimration Mode, which will set confirmation policy to be AlwaysConfirm.
Please add some docs into the CLI description that by default it runs in "always-ask" mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I've successfully addressed the PR comment to fix the confirmation mode semantics. Here's what was changed:
✅ Changes Made
-
Fixed
--always-approvesemantics- Now means "auto-approve all actions without asking for confirmation"
- Uses
NeverConfirmpolicy with no security analyzer - Updated help text to clearly state this behavior
-
Added new
--always-askflag- Means "always ask for confirmation before executing actions"
- Uses
AlwaysConfirmpolicy withLLMSecurityAnalyzer - Marked as "(default)" in help text
-
Documented default behavior
- CLI epilog now states: "By default, OpenHands runs in CLI mode with 'always-ask' confirmation mode"
setup_conversation()defaults to "always-ask" when no mode is specified- Confirmation mode display shows the effective mode (defaulting to "always-ask")
-
Updated all related code
types.py: UpdatedConfirmationModetype to include"always-ask"setup.py: Implemented correct policy mapping (always-ask → AlwaysConfirm, always-approve → NeverConfirm, llm-approve → ConfirmRisky)runner.py: Fixed toggle logic (enable → "always-ask", disable → "always-approve")simple_main.py: Added parsing for all three confirmation modesagent_chat.py: Updated mode display with descriptions
-
Updated all tests
- Added test for
--always-askflag - Fixed existing tests to use correct mode names
- Added mutually exclusive tests for the new flag
- All 247 tests pass ✅
- All lint checks pass ✅
- Added test for
📊 Verification
- ✅ All tests pass (247 passing)
- ✅ Lint checks pass (ruff, pycodestyle, pyright)
- ✅ Changes committed and pushed to PR feat: Add --always-approve and --llm-approve CLI arguments for confirmation mode #110
The PR has been updated with these changes. The confirmation mode semantics are now correct and properly documented.
openhands_cli/runner.py
Outdated
| self.set_confirmation_policy(AlwaysConfirm()) | ||
| # Enable confirmation mode: recreate conversation with always mode | ||
| self.conversation = setup_conversation( | ||
| self.conversation.id, confirmation_mode="always-approve" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like maybe this could also be a Policy?
- --always-approve now means auto-approve all (never ask for confirmation) - --always-ask now means always ask for confirmation before executing actions - Document that default behavior is 'always-ask' mode - Update setup.py: always-ask uses AlwaysConfirm, always-approve uses NeverConfirm - Update runner.py toggle logic to use correct mode names - Update all tests to reflect new semantics - Update CLI help text with correct descriptions and shortened examples - All 247 tests pass and lint checks pass Addresses PR review comment about confirmation mode semantics.
Description
This PR adds two new command-line arguments to allow users to specify the confirmation mode when launching the OpenHands CLI:
--always-approve: Enables always approve mode, where all actions are approved by the user before execution--llm-approve: Enables LLM-based security analyzer mode, where only HIGH-risk actions require user confirmationFixes #107
Changes
Command-Line Arguments
--always-approveand--llm-approvearguments tomain_parser.pyCore Functionality
simple_main.pyto parse and passconfirmation_modetorun_cli_entry()agent_chat.pyto acceptconfirmation_modeparameter and display the mode status on startupsetup.pyto support different confirmation modes with appropriate policies:None: Default behavior (NeverConfirm policy)"always": AlwaysConfirm policy with LLMSecurityAnalyzer"llm": ConfirmRisky policy (HIGH risk threshold) with LLMSecurityAnalyzerTesting
test_main.pytest_confirmation_mode.pyconfirmation_modeparameterUsage Examples
Testing
To verify the implementation:
Checklist
--always-approveand--llm-approve@xingyaoww can click here to continue refining the PR
🚀 Try this PR