feat(commands): granular permissions with /tools #1054

hayemaxi · 2025-04-02T22:37:46Z

Implements #921 which describes trusting and untrusting specific tools for the current session.

Start with default permissions. Users can change them from command line or within chat via /tools
/acceptall and --accept-all were deprecated in favor of --trust-all-tools and /tools trustall. They will continue to work, but display a notice and activation functionality of the new commands.
- Command::AcceptAll no longer exists
UI has been reword a little, see screenshots.
report_issue will include trust override settings in the report.
Chat supports multiple tool use requests from Q at once. I have refactored the tool flow to allow asking permissions for individual tool requests at a time
- Now, ExecuteTools (checks for acceptance one tool at a time) -> PromptUser (ask for acceptance) -> HandleInput (handle acceptance) -> ExecuteTools (find next tool that needs acceptance OR trigger execution if none remaining).
- I was not able to find a prompt that made Q send multiple tool_uses in a single request coming from Q. It seems to do them iteratively even when asked to do them in parallel.
- In any case, that original logic to support parallel requests is preserved with this new structure.

Risks: Likely conflicts with #999

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

codecov-commenter · 2025-04-02T22:39:54Z

Codecov Report

Attention: Patch coverage is 34.27762% with 232 lines in your changes missing coverage. Please review.

Project coverage is 13.68%. Comparing base (3eb6def) to head (48feeb2).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/q_cli/src/cli/chat/mod.rs	37.91%	106 Missing and 7 partials ⚠️
crates/q_cli/src/cli/chat/command.rs	6.84%	67 Missing and 1 partial ⚠️
crates/q_cli/src/cli/chat/tools/mod.rs	20.51%	31 Missing ⚠️
crates/q_cli/src/cli/mod.rs	75.00%	13 Missing ⚠️
crates/q_cli/src/cli/chat/tools/gh_issue.rs	0.00%	4 Missing ⚠️
crates/q_cli/src/cli/chat/conversation_state.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
- Coverage   13.83%   13.68%   -0.15%     
==========================================
  Files        2361     2361              
  Lines      204895   203623    -1272     
  Branches   185259   183987    -1272     
==========================================
- Hits        28348    27872     -476     
+ Misses     175052   174365     -687     
+ Partials     1495     1386     -109

☔ 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.

Implements aws#921 which describes trusting and untrusting specific tools for the current session. - Start with default permissions. Users can change them from command line or within chat via /tools - `/acceptall` and `--accept-all` were deprecated in favor of `--trust-all-tools` and `/tools trustall`. They will continue to work, but display a notice and activation functionality of the new commands. - Command::AcceptAll no longer exists - UI has been reword a little, see screenshots. - `report_issue` will include trust override settings in the report. - Chat supports multiple tool use requests from Q at once. I have refactored the tool flow to allow asking permissions for individual tool requests at a time - Now, ExecuteTools (checks for acceptance one tool at a time) -> PromptUser (ask for acceptance) -> HandleInput (handle acceptance) -> ExecuteTools (find next tool that needs acceptance OR trigger execution if none remaining). - I was not able to find a prompt that made Q send multiple tool_uses in a single request coming from Q. It seems to do them iteratively even when asked to do them in parallel. - In any case, that original logic to support parallel requests is preserved with this new structure. Risks: May conflict with aws#999

dingfeli · 2025-04-03T18:52:55Z

crates/q_cli/src/cli/chat/tools/mod.rs

+pub struct ToolPermission {
+    pub trusted: bool,
+}


Are we future proofing ourselves here, just curious. What are some of the other things we might be adding to this in the future?

Yes, instead of blanket trust bool, we can expand this to account for things like sub commands and directories.

dingfeli · 2025-04-03T20:19:38Z

crates/q_cli/src/cli/chat/conversation_state.rs

@@ -350,12 +353,12 @@ impl ConversationState {
    }

    /// Sets the next user message with "cancelled" tool results.
-    pub fn abandon_tool_use(&mut self, tools_to_be_abandoned: Vec<(String, super::tools::Tool)>, deny_input: String) {
+    pub fn abandon_tool_use(&mut self, tools_to_be_abandoned: Vec<QueuedTool>, deny_input: String) {


If the user accepts a partial list of tools, and without running it, interrupts it the workflow.
Do we now accept the permission granted in this interaction? Or do we revoke the permissions granted?
Right now it looks like what's granted would be remembered without running as well.

Yes, for trust only. Future calls on accepted tools will still request it. IMO this is desirable because the user is declaring that they trust the tool regardless of future attempts. Maybe they trust fs_writes, but once they see that it will run execute_bash, they cancel. That doesn't mean that that they don't trust fs_writes afterall.

That makes sense. Just wanted to make sure. The tool level permission should be largely context free.

dingfeli · 2025-04-03T20:30:49Z

crates/q_cli/src/cli/chat/mod.rs

+                        self.conversation_state
+                            .tools
+                            .iter()
+                            .for_each(|FigTool::ToolSpecification(spec)| {
+                                self.tool_permissions.trust_tool(spec.name.as_str());
+                            });


Is there an advantage to explicitly marking every tool as trusted as opposed to checking a flag.
i.e. is there a source of truth that will allow us to know if trustall has been enabled?
If someone is to add a new tool during a session, is the expectation here that the new tool is trusted? Or do we need to prompt the user again?

Not a big deal now though. We can always change this later.

Is there an advantage to explicitly marking every tool as trusted as opposed to checking a flag.
i.e. is there a source of truth that will allow us to know if trustall has been enabled?

We still need it for non-trust-all instances, but yes we could skip iteration of we had a trust-all flag. I didn't add this because:

Most if not all cases the there will only be one tool to iterate for. We can remove complexity of maintaining a trust-all flag since we already have the iteration code.
It doesn't seem that we can add tools mid-session at this time, but if we could in the future, we may want to re-evaluate trustall for that tool.

If someone is to add a new tool during a session, is the expectation here that the new tool is trusted? Or do we need to prompt the user again?

Currently the expectation is that the user needs to trust the new tool once it is added. I am open to discussing this, but it makes sense to me. Especially if the tool is added via some automation.

dingfeli

Seems mostly aligned with the RFC referenced.
Implementation seems sound to me.
Feel free to request reviews from people who are involved in the RFC discussion.

hayemaxi force-pushed the tool-trust-final branch from bc196a7 to 0ba142d Compare April 2, 2025 22:57

hayemaxi requested a review from dingfeli April 2, 2025 23:05

hayemaxi marked this pull request as ready for review April 2, 2025 23:05

hayemaxi requested a review from a team as a code owner April 2, 2025 23:05

hayemaxi mentioned this pull request Apr 3, 2025

Add visual indication that /acceptall is enabled #1042

Open

hayemaxi added 4 commits April 3, 2025 11:46

typo

288d8e2

reword help cli

251513e

/tools help subcommand explanation reword

24e413b

format

48feeb2

GoodluckH linked an issue Apr 3, 2025 that may be closed by this pull request

Add visual indication that /acceptall is enabled #1042

Open

dingfeli reviewed Apr 3, 2025

View reviewed changes

dingfeli approved these changes Apr 3, 2025

View reviewed changes

hayemaxi removed a link to an issue Apr 3, 2025

Add visual indication that /acceptall is enabled #1042

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(commands): granular permissions with /tools #1054

feat(commands): granular permissions with /tools #1054

hayemaxi commented Apr 2, 2025 •

edited

Loading

codecov-commenter commented Apr 2, 2025 •

edited

Loading

dingfeli Apr 3, 2025

hayemaxi Apr 3, 2025

dingfeli Apr 3, 2025

hayemaxi Apr 3, 2025

dingfeli Apr 3, 2025

dingfeli Apr 3, 2025 •

edited by hayemaxi

Loading

hayemaxi Apr 3, 2025

dingfeli left a comment

feat(commands): granular permissions with /tools #1054

Are you sure you want to change the base?

feat(commands): granular permissions with /tools #1054

Conversation

hayemaxi commented Apr 2, 2025 • edited Loading

codecov-commenter commented Apr 2, 2025 • edited Loading

Codecov Report

dingfeli Apr 3, 2025

Choose a reason for hiding this comment

hayemaxi Apr 3, 2025

Choose a reason for hiding this comment

dingfeli Apr 3, 2025

Choose a reason for hiding this comment

hayemaxi Apr 3, 2025

Choose a reason for hiding this comment

dingfeli Apr 3, 2025

Choose a reason for hiding this comment

dingfeli Apr 3, 2025 • edited by hayemaxi Loading

Choose a reason for hiding this comment

hayemaxi Apr 3, 2025

Choose a reason for hiding this comment

dingfeli left a comment

Choose a reason for hiding this comment

hayemaxi commented Apr 2, 2025 •

edited

Loading

codecov-commenter commented Apr 2, 2025 •

edited

Loading

dingfeli Apr 3, 2025 •

edited by hayemaxi

Loading