-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(claude-code): add permission prompt routing for approve mode #7420
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
Changes from all commits
b8bdad4
2b7502b
4d4e325
3078280
a4c6252
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -981,6 +981,10 @@ impl CliSession { | |
|
|
||
| if permission == Permission::Cancel { | ||
| output::render_text("Tool call cancelled. Returning to chat...", Some(Color::Yellow), true); | ||
| self.agent.handle_confirmation(id.clone(), PermissionConfirmation { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this need to be reflected in desktop or mobile etc?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| principal_type: PrincipalType::Tool, | ||
| permission: Permission::DenyOnce, | ||
| }).await; | ||
| let mut response_message = Message::user(); | ||
| response_message.content.push(MessageContent::tool_response( | ||
| id, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ use crate::mcp_utils::ToolResult; | |
| use crate::permission::permission_inspector::PermissionInspector; | ||
| use crate::permission::permission_judge::PermissionCheckResult; | ||
| use crate::permission::PermissionConfirmation; | ||
| use crate::providers::base::Provider; | ||
| use crate::providers::base::{PermissionRouting, Provider}; | ||
| use crate::providers::errors::ProviderError; | ||
| use crate::recipe::{Author, Recipe, Response, Settings}; | ||
| use crate::scheduler_trait::SchedulerTrait; | ||
|
|
@@ -846,11 +846,28 @@ impl Agent { | |
| request_id: String, | ||
| confirmation: PermissionConfirmation, | ||
| ) { | ||
| let provider = self.provider.lock().await.clone(); | ||
| if let Some(provider) = provider.as_ref() { | ||
| if provider.permission_routing() == PermissionRouting::ActionRequired | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you talk me throught what this is doing? I think I know...
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TL;DR: Confirmations from the user need to get back to whoever is waiting. For LLM providers, that's goose's tool executor. For agentic providers like claude-code, that's the provider's own stream loop. This dispatches to the right one. Recapping LLM providers not for you, but for me: LLM providers (Anthropic, OpenAI, etc): goose executes tools itself. When a tool needs approval, dispatch_tool_calls yields an ActionRequired message to the frontend, then blocks on an mpsc channel waiting for the user's decision. When the user allows or denies, Now, Agentic providers (claude-code, ACP) — the CLI process executes tools. Right now this is only used for Claude-code, but ACP will be similar: claude-code wants to run a tool in approve mode, it writes a can_use_tool JSON request to stdout via its permission-prompt-tool. The provider's So, all frontends (CLI, desktop, mobile, ACP) converge on |
||
| && provider | ||
| .handle_permission_confirmation(&request_id, &confirmation) | ||
| .await | ||
| { | ||
| return; | ||
| } | ||
| } | ||
| if let Err(e) = self.confirmation_tx.send((request_id, confirmation)).await { | ||
| error!("Failed to send confirmation: {}", e); | ||
| } | ||
| } | ||
|
|
||
| pub async fn supports_action_required_permissions(&self) -> bool { | ||
| if let Some(provider) = self.provider.lock().await.as_ref() { | ||
| return provider.permission_routing() == PermissionRouting::ActionRequired; | ||
| } | ||
| false | ||
| } | ||
|
|
||
| #[instrument( | ||
| skip(self, user_message, session_config), | ||
| fields(user_message, trace_input) | ||
|
|
@@ -2014,8 +2031,119 @@ impl Agent { | |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::permission::permission_confirmation::PrincipalType; | ||
| use crate::providers::base::PermissionRouting; | ||
| use crate::recipe::Response; | ||
|
|
||
| struct ActionRequiredProvider { | ||
| handled: tokio::sync::Mutex<Vec<(String, PermissionConfirmation)>>, | ||
| } | ||
|
|
||
| impl ActionRequiredProvider { | ||
| fn new() -> Self { | ||
| Self { | ||
| handled: tokio::sync::Mutex::new(Vec::new()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Debug for ActionRequiredProvider { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("ActionRequiredProvider").finish() | ||
| } | ||
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl crate::providers::base::Provider for ActionRequiredProvider { | ||
| fn get_name(&self) -> &str { | ||
| "test-action-required" | ||
| } | ||
| fn get_model_config(&self) -> crate::model::ModelConfig { | ||
| crate::model::ModelConfig::new("test").unwrap() | ||
| } | ||
| async fn stream( | ||
| &self, | ||
| _: &crate::model::ModelConfig, | ||
| _: &str, | ||
| _: &str, | ||
| _: &[crate::conversation::message::Message], | ||
| _: &[rmcp::model::Tool], | ||
| ) -> Result<crate::providers::base::MessageStream, crate::providers::errors::ProviderError> | ||
| { | ||
| unimplemented!() | ||
| } | ||
| fn permission_routing(&self) -> PermissionRouting { | ||
| PermissionRouting::ActionRequired | ||
| } | ||
| async fn handle_permission_confirmation( | ||
| &self, | ||
| request_id: &str, | ||
| confirmation: &PermissionConfirmation, | ||
| ) -> bool { | ||
| self.handled | ||
| .lock() | ||
| .await | ||
| .push((request_id.to_string(), confirmation.clone())); | ||
| request_id == "known" | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_handle_confirmation_routes_to_provider() { | ||
| let agent = Agent::new(); | ||
| let provider = Arc::new(ActionRequiredProvider::new()); | ||
| *agent.provider.lock().await = | ||
| Some(provider.clone() as Arc<dyn crate::providers::base::Provider>); | ||
|
|
||
| // Known request_id → provider handles it, confirmation_tx NOT called | ||
| agent | ||
| .handle_confirmation( | ||
| "known".to_string(), | ||
| PermissionConfirmation { | ||
| principal_type: PrincipalType::Tool, | ||
| permission: crate::permission::Permission::AllowOnce, | ||
| }, | ||
| ) | ||
| .await; | ||
| assert_eq!(provider.handled.lock().await.len(), 1); | ||
|
|
||
| // Unknown request_id → provider returns false, falls through to confirmation_tx | ||
| agent | ||
| .handle_confirmation( | ||
| "unknown".to_string(), | ||
| PermissionConfirmation { | ||
| principal_type: PrincipalType::Tool, | ||
| permission: crate::permission::Permission::DenyOnce, | ||
| }, | ||
| ) | ||
| .await; | ||
| assert_eq!(provider.handled.lock().await.len(), 2); | ||
| // Verify the fallthrough went to confirmation_rx | ||
| let mut rx = agent.confirmation_rx.lock().await; | ||
| let (id, conf) = rx.recv().await.unwrap(); | ||
| assert_eq!(id, "unknown"); | ||
| assert_eq!(conf.permission, crate::permission::Permission::DenyOnce); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_handle_confirmation_noop_provider() { | ||
| let agent = Agent::new(); | ||
| // No provider set → Noop routing, goes straight to confirmation_tx | ||
| agent | ||
| .handle_confirmation( | ||
| "any".to_string(), | ||
| PermissionConfirmation { | ||
| principal_type: PrincipalType::Tool, | ||
| permission: crate::permission::Permission::AllowOnce, | ||
| }, | ||
| ) | ||
| .await; | ||
|
|
||
| let mut rx = agent.confirmation_rx.lock().await; | ||
| let (id, _) = rx.recv().await.unwrap(); | ||
| assert_eq!(id, "any"); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_add_final_output_tool() -> Result<()> { | ||
| let agent = Agent::new(); | ||
|
|
||
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.
will this need to be reflected in desktop or mobile etc?
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.
No. This code is the CLI's permission prompt and collecting the user's answer.
Desktop/mobile already have their own ToolApprovalButtons POSTs to the
confirm_tool_actionserver endpoint, which calls the samehandle_confirmation()method this PR changed. So the routing automatically applies to desktop/mobile: nothing to do here.Note: When #7238 lands and CLI goes through goosed, this CLI-specific code path becomes dead code; the CLI would POST to
confirm_tool_actionjust like desktop does today.