Skip to content

feat(/tools): update trustall UX, /trust and /untrust capability #1200

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

Merged
merged 1 commit into from
Apr 21, 2025

Conversation

hayemaxi
Copy link
Contributor

@hayemaxi hayemaxi commented Apr 11, 2025

#1157

  • Make trustall/acceptall warning more verbose.
  • Trustall/acceptall warning will show if you start chat with -a or --acceptall or --trust-all-tools
  • /trust and /untrust takes an unlimited amount of tools at a time
  • /trust and /untrust will display an error if you specify a tool name that does not exist

image
image

image
image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@hayemaxi hayemaxi requested a review from mschrage April 11, 2025 17:37
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 76.47059% with 16 lines in your changes missing coverage. Please review.

Project coverage is 14.06%. Comparing base (72cb299) to head (b501ee5).
Report is 158 commits behind head on main.

Files with missing lines Patch % Lines
crates/q_cli/src/cli/chat/mod.rs 70.83% 13 Missing and 1 partial ⚠️
crates/q_cli/src/cli/chat/command.rs 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1200      +/-   ##
==========================================
- Coverage   14.07%   14.06%   -0.01%     
==========================================
  Files        2366     2366              
  Lines      205628   205665      +37     
  Branches   185992   186029      +37     
==========================================
- Hits        28936    28934       -2     
- Misses     175256   175295      +39     
  Partials     1436     1436              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hayemaxi hayemaxi marked this pull request as ready for review April 11, 2025 18:10
@hayemaxi hayemaxi requested a review from a team April 11, 2025 18:10
Comment on lines 206 to 244
const TRUST_ALL_TEXT: &str = color_print::cstr! {"\n<green!>All tools are now trusted (<red!>!</red!>). This <bold>disables</bold> acceptance prompting.\
\nAgents can sometimes do unexpected things so understand the risks.</green!>"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe say ".... Q will execute tools without asking for confirmation."

it's shorter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I received feedback that warning was too short 😔

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh then maybe replace "This disables...prompting." with "Q will execute tools without asking for confirmation." Because "acceptance prompting" might be hard to understand

- Make trustall/acceptall warning more verbose.
- Trustall/acceptall warning will show if you start chat with -a or --acceptall or --trust-all-tools
- /trust and /untrust takes an unlimited amount of tools at a time
- /trust and /untrust will display an error if you specify a tool name that does not exist
@GoodluckH
Copy link
Contributor

I wonder if we can remove that text but for the user input indicator, we can show ![all tools trusted]> initially

Then for the subsequent inputs we just show !>

We assume our users are smarter than us, so if they somehow figured out how to trust all tools, they should already know the risk.

We just need some text indicating what ! means

Copy link
Contributor

@GoodluckH GoodluckH left a comment

Choose a reason for hiding this comment

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

See comments.

@hayemaxi
Copy link
Contributor Author

I wonder if we can remove that text but for the user input indicator
...
so if they somehow figured out how to trust all tools, they should already know the risk.

We have gotten some feedback that trust doesn't fully explain the implications of using it, which prompted this PR to make it more verbose (at least for the most dangerous case: /acceptall).

... we can show ![all tools trusted]> initially

Then for the subsequent inputs we just show !>

IMO ![all tools trusted]> is too cluttered even for the first message, and if they start with a profile it will look even worse ![all tools trusted][my-usual-profile]>.

Instead, what if we just remove !>?

@GoodluckH
Copy link
Contributor

GoodluckH commented Apr 18, 2025

We have gotten some feedback that trust doesn't fully explain the implications of using it

Interesting, that means we are not communicating that properly when they run /tools trustall. If they can find that command, I think they pretty much know what they're doing. Maybe we can just add a confirmation dialog when they run /tools trustall, and put verbose stuff there instead of on the greeting screen.

Or if confirmation dialog is too much code change, we can just add that risk disclaimer as well as the (!) and put it in a different color.

Maybe we can replace ! with 🤖 or something to look not too punishing...

@hayemaxi hayemaxi self-assigned this Apr 18, 2025
@hayemaxi hayemaxi linked an issue Apr 18, 2025 that may be closed by this pull request
Copy link
Contributor

@GoodluckH GoodluckH left a comment

Choose a reason for hiding this comment

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

Approving for now to unblock release. we can iterate on UI as we go

@hayemaxi hayemaxi merged commit 8e3c5b5 into aws:main Apr 21, 2025
11 checks passed
This was referenced Apr 30, 2025
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.

Add startup message for -a, --acceptall, --trust-all-tools options
3 participants