Fix review findings: cancelled turns, approval visibility

index c99e90d..ad39789 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -98,6 +98,14 @@ pub trait InferenceBackend: Send + Sync { sink: Option<&TokenSink>, ) -> Result<TurnResult, BackendError>; + /// Record tool results in the conversation history *without* asking the + /// model to continue the turn. Used when a turn is abandoned mid-round + /// (the user cancelled at the permission gate): by then the assistant + /// message carrying the tool calls is already in history, and leaving them + /// unanswered makes strict OpenAI-compatible endpoints reject every later + /// request in the session. + async fn record_cancelled_tool_results(&self, results: Vec<ToolResult>); + /// Whether inference runs over the network (a configured provider) rather /// than on-device. Drives UI labelling so the displayed model can't claim a /// local model while requests actually go to the cloud. @@ -195,6 +203,13 @@ impl InferenceBackend for LocalBackend { Ok(onde_result_to_turn(result)) } + async fn record_cancelled_tool_results(&self, _results: Vec<ToolResult>) { + // onde's public API cannot append tool-result history entries without + // running another inference round, so the dangling tool call stays in + // its history. The chat template replays it as-is, which local models + // tolerate — worst case the model re-issues the call next turn. + } + fn is_remote(&self) -> bool { false } @@ -562,6 +577,17 @@ impl InferenceBackend for OpenAiBackend { self.complete(tools, sink).await } + async fn record_cancelled_tool_results(&self, results: Vec<ToolResult>) { + let mut history = self.history.lock().await; + for result in results { + history.push(serde_json::json!({ + "role": "tool", + "tool_call_id": result.tool_call_id, + "content": result.content, + })); + } + } + fn is_remote(&self) -> bool { true } @@ -727,6 +753,36 @@ mod tests { ); } + #[tokio::test] + async fn cancelled_tool_results_close_out_history() { + let backend = OpenAiBackend::new("http://localhost", "", "test-model", None); + backend + .history + .lock() + .await + .push(streamed_assistant_history( + "", + &[ToolCall { + id: "call_9".to_string(), + name: "run_command".to_string(), + arguments: r#"{"command":"ls"}"#.to_string(), + }], + )); + + backend + .record_cancelled_tool_results(vec![ToolResult { + tool_call_id: "call_9".to_string(), + content: "cancelled by the user".to_string(), + }]) + .await; + + let history = backend.history.lock().await; + let last = history.last().unwrap(); + assert_eq!(last["role"], "tool"); + assert_eq!(last["tool_call_id"], "call_9"); + assert_eq!(last["content"], "cancelled by the user"); + } + #[test] fn assistant_message_with_tool_calls_round_trips() { let message = ResponseMessage {

index 2038ebc..638badf 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -161,6 +161,8 @@ mod tui { /// `reply`; the user answers with y (once) / a (session) / n (deny) ApprovalRequest { tool: String, + /// arguments preview so the user can see what they are approving + args: String, reply: oneshot::Sender<ApprovalChoice>, }, } @@ -1689,6 +1691,7 @@ mod tui { let _ = tx .send(InferenceUpdate::ApprovalRequest { tool: tc.name.clone(), + args: permissions::approval_preview(&tc.arguments), reply: reply_tx, }) .await; @@ -1928,9 +1931,14 @@ mod tui { app.stop_thinking(); app.messages.push(ChatMessage::system(format!("error: {msg}"))); } - Some(InferenceUpdate::ApprovalRequest { tool, reply }) => { + Some(InferenceUpdate::ApprovalRequest { tool, args, reply }) => { + let call = if args.is_empty() { + tool.clone() + } else { + format!("{tool}({args})") + }; app.messages.push(ChatMessage::system(format!( - "⚠ permission — allow {tool}? [y]es · [a]lways this session · [n]o" + "⚠ permission — allow {call}? [y]es · [a]lways this session · [n]o" ))); app.pending_approval = Some((tool, reply)); }

index 2d719f3..293bdb0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1295,7 +1295,7 @@ impl SiGitAgent { let mut tool_results = Vec::new(); - for tc in &result.tool_calls { + for (call_index, tc) in result.tool_calls.iter().enumerate() { log::info!( " → {}({})", tc.name, @@ -1326,6 +1326,23 @@ impl SiGitAgent { } PermissionVerdict::TurnCancelled => { log::info!("prompt({}) cancelled at permission gate", session_id); + // The assistant message carrying these tool + // calls is already in the backend history; + // leaving any of them unanswered makes strict + // OpenAI-compatible endpoints reject every + // later request in the session. Close out this + // call and the ones this round never reached. + for pending in &result.tool_calls[call_index..] { + tool_results.push(BackendToolResult { + tool_call_id: pending.id.clone(), + content: format!( + "`{}` was not executed: the user cancelled the turn \ + at the permission prompt.", + pending.name + ), + }); + } + backend.record_cancelled_tool_results(tool_results).await; return Ok(PromptResponse::new(StopReason::Cancelled)); } } @@ -1409,12 +1426,18 @@ impl SiGitAgent { tool_name: &str, arguments: &str, ) -> PermissionVerdict { - let args_preview: String = arguments.chars().take(120).collect(); + // The user decides from this dialog, so show the arguments with any + // truncation flagged (a silently clipped command could hide its tail + // from the person approving it). The full arguments also travel as + // `raw_input` for clients that render it. + let args_preview = permissions::approval_preview(arguments); let title = if args_preview.is_empty() { tool_name.to_string() } else { format!("{tool_name}({args_preview})") }; + let raw_input: serde_json::Value = serde_json::from_str(arguments) + .unwrap_or_else(|_| serde_json::Value::String(arguments.to_string())); let request = RequestPermissionRequest::new( session_id.clone(), @@ -1423,7 +1446,8 @@ impl SiGitAgent { ToolCallUpdateFields::new() .title(title) .kind(tool_kind_for(tool_name)) - .status(ToolCallStatus::Pending), + .status(ToolCallStatus::Pending) + .raw_input(raw_input), ), vec![ PermissionOption::new("allow_once", "Allow once", PermissionOptionKind::AllowOnce),

index 172b6ee..ce9e796 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -113,6 +113,20 @@ pub fn user_denial(tool_name: &str) -> String { ) } +/// Render a tool call's arguments for an approval prompt. The person deciding +/// must be able to see what they are approving, so the cap is generous and any +/// cut is marked with how much is hidden — silently truncating could hide the +/// tail of a command from the user who is about to allow it. +pub fn approval_preview(arguments: &str) -> String { + const MAX_CHARS: usize = 500; + let total = arguments.chars().count(); + if total <= MAX_CHARS { + return arguments.to_string(); + } + let shown: String = arguments.chars().take(MAX_CHARS).collect(); + format!("{shown}… [+{} more chars]", total - MAX_CHARS) +} + /// Policy check for one tool call. See the module docs for the layering. pub fn decision_for(session: &str, tool_name: &str) -> Decision { if classify(tool_name) == ToolRisk::ReadOnly { @@ -272,6 +286,23 @@ mod tests { assert_ne!(decision_for(session, "edit_file"), Decision::Allow); } + #[test] + fn approval_preview_shows_short_arguments_in_full() { + let args = r#"{"command":"cargo test"}"#; + assert_eq!(approval_preview(args), args); + } + + #[test] + fn approval_preview_marks_truncation_explicitly() { + let args = format!(r#"{{"command":"echo {}; rm -rf /"}}"#, "x".repeat(600)); + let preview = approval_preview(&args); + assert!(preview.chars().count() < args.chars().count()); + assert!( + preview.contains("more chars]"), + "hidden content must be flagged, got: {preview}" + ); + } + #[test] fn plan_mode_outranks_session_grant() { let _guard = env_guard();