Add buttons for more outcomes and handle tools that don't need

Ben Brandt and Antonio Scandurra created

authorization

Co-authored-by: Antonio Scandurra <me@as-cii.com>

Change summary

crates/acp/src/acp.rs         |  95 +++++----
crates/acp/src/server.rs      |  47 ----
crates/acp/src/thread_view.rs | 359 ++++++++++++++++++++++++++++++++----
3 files changed, 376 insertions(+), 125 deletions(-)

Detailed changes

crates/acp/src/acp.rs 🔗

@@ -130,8 +130,8 @@ pub struct ToolCall {
 #[derive(Debug)]
 pub enum ToolCallStatus {
     WaitingForConfirmation {
-        description: Entity<Markdown>,
-        respond_tx: oneshot::Sender<bool>,
+        confirmation: acp::ToolCallConfirmation,
+        respond_tx: oneshot::Sender<acp::ToolCallConfirmationOutcome>,
     },
     // todo! Running?
     Allowed {
@@ -269,24 +269,40 @@ impl AcpThread {
         );
     }
 
-    pub fn push_tool_call(
+    pub fn request_tool_call(
         &mut self,
         title: String,
-        description: String,
-        confirmation_tx: Option<oneshot::Sender<bool>>,
+        confirmation: acp::ToolCallConfirmation,
+        cx: &mut Context<Self>,
+    ) -> ToolCallRequest {
+        let (tx, rx) = oneshot::channel();
+
+        let status = ToolCallStatus::WaitingForConfirmation {
+            confirmation,
+            respond_tx: tx,
+        };
+
+        let id = self.insert_tool_call(title, status, cx);
+        ToolCallRequest { id, outcome: rx }
+    }
+
+    pub fn push_tool_call(&mut self, title: String, cx: &mut Context<Self>) -> ToolCallId {
+        let status = ToolCallStatus::Allowed {
+            status: acp::ToolCallStatus::Running,
+            content: None,
+        };
+
+        self.insert_tool_call(title, status, cx)
+    }
+
+    fn insert_tool_call(
+        &mut self,
+        title: String,
+        status: ToolCallStatus,
         cx: &mut Context<Self>,
     ) -> ToolCallId {
         let language_registry = self.project.read(cx).languages().clone();
 
-        let description = cx.new(|cx| {
-            Markdown::new(
-                description.into(),
-                Some(language_registry.clone()),
-                None,
-                cx,
-            )
-        });
-
         let entry_id = self.push_entry(
             AgentThreadEntryContent::ToolCall(ToolCall {
                 // todo! clean up id creation
@@ -294,17 +310,7 @@ impl AcpThread {
                 tool_name: cx.new(|cx| {
                     Markdown::new(title.into(), Some(language_registry.clone()), None, cx)
                 }),
-                status: if let Some(respond_tx) = confirmation_tx {
-                    ToolCallStatus::WaitingForConfirmation {
-                        description,
-                        respond_tx,
-                    }
-                } else {
-                    ToolCallStatus::Allowed {
-                        status: acp::ToolCallStatus::Running,
-                        content: Some(description),
-                    }
-                },
+                status,
             }),
             cx,
         );
@@ -312,7 +318,12 @@ impl AcpThread {
         ToolCallId(entry_id)
     }
 
-    pub fn authorize_tool_call(&mut self, id: ToolCallId, allowed: bool, cx: &mut Context<Self>) {
+    pub fn authorize_tool_call(
+        &mut self,
+        id: ToolCallId,
+        outcome: acp::ToolCallConfirmationOutcome,
+        cx: &mut Context<Self>,
+    ) {
         let Some(entry) = self.entry_mut(id.0) else {
             return;
         };
@@ -322,19 +333,19 @@ impl AcpThread {
             return;
         };
 
-        let new_status = if allowed {
+        let new_status = if outcome == acp::ToolCallConfirmationOutcome::Reject {
+            ToolCallStatus::Rejected
+        } else {
             ToolCallStatus::Allowed {
                 status: acp::ToolCallStatus::Running,
                 content: None,
             }
-        } else {
-            ToolCallStatus::Rejected
         };
 
         let curr_status = mem::replace(&mut call.status, new_status);
 
         if let ToolCallStatus::WaitingForConfirmation { respond_tx, .. } = curr_status {
-            respond_tx.send(allowed).log_err();
+            respond_tx.send(outcome).log_err();
         } else {
             debug_panic!("tried to authorize an already authorized tool call");
         }
@@ -422,6 +433,11 @@ impl AcpThread {
     }
 }
 
+pub struct ToolCallRequest {
+    pub id: ToolCallId,
+    pub outcome: oneshot::Receiver<acp::ToolCallConfirmationOutcome>,
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -506,7 +522,7 @@ mod tests {
             let AgentThreadEntryContent::ToolCall(ToolCall {
                 id,
                 tool_name,
-                status: ToolCallStatus::WaitingForConfirmation { description, .. },
+                status: ToolCallStatus::Allowed { .. },
             }) = &thread.entries().last().unwrap().content
             else {
                 panic!();
@@ -516,18 +532,19 @@ mod tests {
                 assert_eq!(md.source(), "read_file");
             });
 
-            description.read_with(cx, |md, _cx| {
-                assert!(
-                    md.source().contains("foo"),
-                    "Expected description to contain 'foo', but got {}",
-                    md.source()
-                );
-            });
+            // todo!
+            // description.read_with(cx, |md, _cx| {
+            //     assert!(
+            //         md.source().contains("foo"),
+            //         "Expected description to contain 'foo', but got {}",
+            //         md.source()
+            //     );
+            // });
             *id
         });
 
         thread.update(cx, |thread, cx| {
-            thread.authorize_tool_call(tool_call_id, true, cx);
+            thread.authorize_tool_call(tool_call_id, acp::ToolCallConfirmationOutcome::Allow, cx);
             assert!(matches!(
                 thread.entries().last().unwrap().content,
                 AgentThreadEntryContent::ToolCall(ToolCall {

crates/acp/src/server.rs 🔗

@@ -1,9 +1,8 @@
-use crate::{AcpThread, ThreadEntryId, ThreadId, ToolCallId};
+use crate::{AcpThread, ThreadEntryId, ThreadId, ToolCallId, ToolCallRequest};
 use agentic_coding_protocol as acp;
 use anyhow::{Context as _, Result};
 use async_trait::async_trait;
 use collections::HashMap;
-use futures::channel::oneshot;
 use gpui::{App, AppContext, AsyncApp, Context, Entity, Task, WeakEntity};
 use parking_lot::Mutex;
 use project::Project;
@@ -182,52 +181,18 @@ impl acp::Client for AcpClientDelegate {
         &self,
         request: acp::RequestToolCallConfirmationParams,
     ) -> Result<acp::RequestToolCallConfirmationResponse> {
-        let (tx, rx) = oneshot::channel();
-
         let cx = &mut self.cx.clone();
-        let entry_id = cx
+        let ToolCallRequest { id, outcome } = cx
             .update(|cx| {
                 self.update_thread(&request.thread_id.into(), cx, |thread, cx| {
-                    // todo! Should we pass through richer data than a description?
-                    let description = match request.confirmation {
-                        acp::ToolCallConfirmation::Edit {
-                            file_name,
-                            file_diff,
-                        } => {
-                            // todo! Nicer syntax/presentation based on file extension? Better way to communicate diff?
-                            format!("Edit file `{file_name}` with diff:\n```\n{file_diff}\n```")
-                        }
-                        acp::ToolCallConfirmation::Execute {
-                            command,
-                            root_command: _,
-                        } => {
-                            format!("Execute command `{command}`")
-                        }
-                        acp::ToolCallConfirmation::Mcp {
-                            server_name,
-                            tool_name: _,
-                            tool_display_name,
-                        } => {
-                            format!("MCP: {server_name} - {tool_display_name}")
-                        }
-                        acp::ToolCallConfirmation::Info { prompt, urls } => {
-                            format!("Info: {prompt}\n{urls:?}")
-                        }
-                    };
-                    thread.push_tool_call(request.title, description, Some(tx), cx)
+                    thread.request_tool_call(request.title, request.confirmation, cx)
                 })
             })?
             .context("Failed to update thread")?;
 
-        let outcome = if rx.await? {
-            // todo! Handle other outcomes
-            acp::ToolCallConfirmationOutcome::Allow
-        } else {
-            acp::ToolCallConfirmationOutcome::Reject
-        };
         Ok(acp::RequestToolCallConfirmationResponse {
-            id: entry_id.into(),
-            outcome,
+            id: id.into(),
+            outcome: outcome.await?,
         })
     }
 
@@ -239,7 +204,7 @@ impl acp::Client for AcpClientDelegate {
         let entry_id = cx
             .update(|cx| {
                 self.update_thread(&request.thread_id.into(), cx, |thread, cx| {
-                    thread.push_tool_call(request.title, request.description, None, cx)
+                    thread.push_tool_call(request.title, cx)
                 })
             })?
             .context("Failed to update thread")?;

crates/acp/src/thread_view.rs 🔗

@@ -2,7 +2,7 @@ use std::path::Path;
 use std::rc::Rc;
 use std::time::Duration;
 
-use agentic_coding_protocol::{self as acp};
+use agentic_coding_protocol::{self as acp, ToolCallConfirmation};
 use anyhow::Result;
 use editor::{Editor, MultiBuffer};
 use gpui::{
@@ -191,12 +191,17 @@ impl AcpThreadView {
         });
     }
 
-    fn authorize_tool_call(&mut self, id: ToolCallId, allowed: bool, cx: &mut Context<Self>) {
+    fn authorize_tool_call(
+        &mut self,
+        id: ToolCallId,
+        outcome: acp::ToolCallConfirmationOutcome,
+        cx: &mut Context<Self>,
+    ) {
         let Some(thread) = self.thread() else {
             return;
         };
         thread.update(cx, |thread, cx| {
-            thread.authorize_tool_call(id, allowed, cx);
+            thread.authorize_tool_call(id, outcome, cx);
         });
         cx.notify();
     }
@@ -289,48 +294,9 @@ impl AcpThreadView {
         };
 
         let content = match &tool_call.status {
-            ToolCallStatus::WaitingForConfirmation { description, .. } => v_flex()
-                .border_color(cx.theme().colors().border)
-                .border_t_1()
-                .px_2()
-                .py_1p5()
-                .child(MarkdownElement::new(
-                    description.clone(),
-                    default_markdown_style(window, cx),
-                ))
-                .child(
-                    h_flex()
-                        .justify_end()
-                        .gap_1()
-                        .child(
-                            Button::new(("allow", tool_call.id.as_u64()), "Allow")
-                                .icon(IconName::Check)
-                                .icon_position(IconPosition::Start)
-                                .icon_size(IconSize::Small)
-                                .icon_color(Color::Success)
-                                .on_click(cx.listener({
-                                    let id = tool_call.id;
-                                    move |this, _, _, cx| {
-                                        this.authorize_tool_call(id, true, cx);
-                                    }
-                                })),
-                        )
-                        .child(
-                            Button::new(("reject", tool_call.id.as_u64()), "Reject")
-                                .icon(IconName::X)
-                                .icon_position(IconPosition::Start)
-                                .icon_size(IconSize::Small)
-                                .icon_color(Color::Error)
-                                .on_click(cx.listener({
-                                    let id = tool_call.id;
-                                    move |this, _, _, cx| {
-                                        this.authorize_tool_call(id, false, cx);
-                                    }
-                                })),
-                        ),
-                )
-                .into_any()
-                .into(),
+            ToolCallStatus::WaitingForConfirmation { confirmation, .. } => {
+                Some(self.render_tool_call_confirmation(tool_call.id, confirmation, cx))
+            }
             ToolCallStatus::Allowed { content, .. } => content.clone().map(|content| {
                 div()
                     .border_color(cx.theme().colors().border)
@@ -372,6 +338,309 @@ impl AcpThreadView {
             )
             .children(content)
     }
+
+    fn render_tool_call_confirmation(
+        &self,
+        tool_call_id: ToolCallId,
+        confirmation: &ToolCallConfirmation,
+        cx: &Context<Self>,
+    ) -> AnyElement {
+        match confirmation {
+            ToolCallConfirmation::Edit {
+                file_name,
+                file_diff,
+            } => v_flex()
+                .border_color(cx.theme().colors().border)
+                .border_t_1()
+                .px_2()
+                .py_1p5()
+                // todo! nicer rendering
+                .child(file_name.clone())
+                .child(file_diff.clone())
+                .child(
+                    h_flex()
+                        .justify_end()
+                        .gap_1()
+                        .child(
+                            Button::new(("allow", tool_call_id.as_u64()), "Always Allow Edits")
+                                .icon(IconName::CheckDouble)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Success)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::AlwaysAllow,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        )
+                        .child(
+                            Button::new(("allow", tool_call_id.as_u64()), "Allow")
+                                .icon(IconName::Check)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Success)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Allow,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        )
+                        .child(
+                            Button::new(("reject", tool_call_id.as_u64()), "Reject")
+                                .icon(IconName::X)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Error)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Reject,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        ),
+                )
+                .into_any(),
+            ToolCallConfirmation::Execute {
+                command,
+                root_command,
+            } => v_flex()
+                .border_color(cx.theme().colors().border)
+                .border_t_1()
+                .px_2()
+                .py_1p5()
+                // todo! nicer rendering
+                .child(command.clone())
+                .child(
+                    h_flex()
+                        .justify_end()
+                        .gap_1()
+                        .child(
+                            Button::new(
+                                ("allow", tool_call_id.as_u64()),
+                                format!("Always Allow {root_command}"),
+                            )
+                            .icon(IconName::CheckDouble)
+                            .icon_position(IconPosition::Start)
+                            .icon_size(IconSize::Small)
+                            .icon_color(Color::Success)
+                            .on_click(cx.listener({
+                                let id = tool_call_id;
+                                move |this, _, _, cx| {
+                                    this.authorize_tool_call(
+                                        id,
+                                        acp::ToolCallConfirmationOutcome::AlwaysAllow,
+                                        cx,
+                                    );
+                                }
+                            })),
+                        )
+                        .child(
+                            Button::new(("allow", tool_call_id.as_u64()), "Allow")
+                                .icon(IconName::Check)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Success)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Allow,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        )
+                        .child(
+                            Button::new(("reject", tool_call_id.as_u64()), "Reject")
+                                .icon(IconName::X)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Error)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Reject,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        ),
+                )
+                .into_any(),
+            ToolCallConfirmation::Mcp {
+                server_name,
+                tool_name: _,
+                tool_display_name,
+            } => v_flex()
+                .border_color(cx.theme().colors().border)
+                .border_t_1()
+                .px_2()
+                .py_1p5()
+                // todo! nicer rendering
+                .child(format!("{server_name} - {tool_display_name}"))
+                .child(
+                    h_flex()
+                        .justify_end()
+                        .gap_1()
+                        .child(
+                            Button::new(
+                                ("allow", tool_call_id.as_u64()),
+                                format!("Always Allow {server_name}"),
+                            )
+                            .icon(IconName::CheckDouble)
+                            .icon_position(IconPosition::Start)
+                            .icon_size(IconSize::Small)
+                            .icon_color(Color::Success)
+                            .on_click(cx.listener({
+                                let id = tool_call_id;
+                                move |this, _, _, cx| {
+                                    this.authorize_tool_call(
+                                        id,
+                                        acp::ToolCallConfirmationOutcome::AlwaysAllowMcpServer,
+                                        cx,
+                                    );
+                                }
+                            })),
+                        )
+                        .child(
+                            Button::new(
+                                ("allow", tool_call_id.as_u64()),
+                                format!("Always Allow {tool_display_name}"),
+                            )
+                            .icon(IconName::CheckDouble)
+                            .icon_position(IconPosition::Start)
+                            .icon_size(IconSize::Small)
+                            .icon_color(Color::Success)
+                            .on_click(cx.listener({
+                                let id = tool_call_id;
+                                move |this, _, _, cx| {
+                                    this.authorize_tool_call(
+                                        id,
+                                        acp::ToolCallConfirmationOutcome::AlwaysAllowTool,
+                                        cx,
+                                    );
+                                }
+                            })),
+                        )
+                        .child(
+                            Button::new(("allow", tool_call_id.as_u64()), "Allow")
+                                .icon(IconName::Check)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Success)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Allow,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        )
+                        .child(
+                            Button::new(("reject", tool_call_id.as_u64()), "Reject")
+                                .icon(IconName::X)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Error)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Reject,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        ),
+                )
+                .into_any(),
+            ToolCallConfirmation::Info { prompt, urls: _ } => v_flex()
+                .border_color(cx.theme().colors().border)
+                .border_t_1()
+                .px_2()
+                .py_1p5()
+                // todo! nicer rendering
+                .child(prompt.clone())
+                .child(
+                    h_flex()
+                        .justify_end()
+                        .gap_1()
+                        .child(
+                            Button::new(("allow", tool_call_id.as_u64()), "Always Allow")
+                                .icon(IconName::CheckDouble)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Success)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::AlwaysAllow,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        )
+                        .child(
+                            Button::new(("allow", tool_call_id.as_u64()), "Allow")
+                                .icon(IconName::Check)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Success)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Allow,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        )
+                        .child(
+                            Button::new(("reject", tool_call_id.as_u64()), "Reject")
+                                .icon(IconName::X)
+                                .icon_position(IconPosition::Start)
+                                .icon_size(IconSize::Small)
+                                .icon_color(Color::Error)
+                                .on_click(cx.listener({
+                                    let id = tool_call_id;
+                                    move |this, _, _, cx| {
+                                        this.authorize_tool_call(
+                                            id,
+                                            acp::ToolCallConfirmationOutcome::Reject,
+                                            cx,
+                                        );
+                                    }
+                                })),
+                        ),
+                )
+                .into_any(),
+        }
+    }
 }
 
 impl Focusable for AcpThreadView {