Skip to content

feat: Implement user confirmation mode, request confirmation when running bash/python code in this mode#2774

Merged
amanape merged 15 commits intoOpenHands:mainfrom
invariantlabs-ai:confirmation_mode
Jul 11, 2024
Merged

feat: Implement user confirmation mode, request confirmation when running bash/python code in this mode#2774
amanape merged 15 commits intoOpenHands:mainfrom
invariantlabs-ai:confirmation_mode

Conversation

@adrgs
Copy link
Copy Markdown
Contributor

@adrgs adrgs commented Jul 3, 2024

What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?
Implements #2308 enhancement

Give a brief summary of what the PR does, explaining any non-trivial design decisions

  • Adds a new boolean setting called CONFIRMATION_MODE, which is false by default
  • Adds a new toggle option in SettingsForm
  • Implements 3 new states for the agents: AWAITING_USER_CONFIRMATION, ACTION_CONFIRMED, ACTION_REJECTED
  • In confirmation mode, the bash/python actions will have confirm/reject buttons directly in the chat log
  • Updated tests

Other references

Screenshots: Screenshot 2024-07-03 at 17 02 46 Screenshot 2024-07-03 at 18 28 17 Screenshot 2024-07-03 at 17 02 59 Screenshot 2024-07-03 at 17 04 17 Screenshot 2024-07-03 at 18 12 00 Screenshot 2024-07-03 at 18 12 08

Copy link
Copy Markdown
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton for this contribution!!
Backend part LGTM 👍 ! Can anyone from the front side take a look?

plus: right now, confirmation mode has to be set at the beginning of the conv, and there's no way to switch them on and off during an interaction session. I guess the next step of improvement would be to support "switch them on/off on the fly" - let's open an issue to track this once this PR is merged :D

@tobitege tobitege requested review from amanape and iFurySt July 4, 2024 21:55
@iFurySt
Copy link
Copy Markdown
Collaborator

iFurySt commented Jul 5, 2024

LGTM! @adrgs , could you please fix the lint and the failed unit tests?

Copy link
Copy Markdown
Contributor

@amanape amanape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, don't forget to lint your code in order to merge changes!

@adrgs
Copy link
Copy Markdown
Contributor Author

adrgs commented Jul 5, 2024

Thanks everyone for the great feedback! I've added a new commit with the changes.

Let me know if there is anything else to be done. Thank you

@enyst
Copy link
Copy Markdown
Collaborator

enyst commented Jul 9, 2024

I think CmdKillAction no longer exists, merge issue?

@adrgs adrgs requested a review from amanape July 10, 2024 09:37
Copy link
Copy Markdown
Contributor

@amanape amanape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Sorry for the delay. It seems your package-lock.json and package.json are not in sync. Try removing the package-lock.json and running npm install again.

@adrgs adrgs force-pushed the confirmation_mode branch 2 times, most recently from 1d836f9 to 0bff2fa Compare July 10, 2024 18:04
@adrgs
Copy link
Copy Markdown
Contributor Author

adrgs commented Jul 10, 2024

Added, sorry for that.

@adrgs adrgs requested a review from amanape July 10, 2024 18:12
@amanape amanape enabled auto-merge (squash) July 10, 2024 19:07
@amanape
Copy link
Copy Markdown
Contributor

amanape commented Jul 10, 2024

It looks that one of the Python tests are failing. I'm not sure if it is due to flakiness or due to the changes you have introduced.

@enyst Could you confirm if it is due to the change they made to agent.py?

@yufansong
Copy link
Copy Markdown
Collaborator

yufansong commented Jul 11, 2024

I guess it should be flakiness. Rerun the test to see the result.

@yufansong yufansong disabled auto-merge July 11, 2024 04:27
@adrgs
Copy link
Copy Markdown
Contributor Author

adrgs commented Jul 11, 2024

Added missing is_confirmed in test outputs

Copy link
Copy Markdown
Contributor

@amanape amanape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this cool feature! Awesome stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants