Address review follow-ups: TUI cancel, state pruning, ACP test

index 4a3a7bf..6bb5de4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,10 +63,10 @@ Run a single test: `cargo test <test_name>`. ## Critical platform constraint: `#[cfg(unix)]` dead code -The interactive client, the `InferenceBackend` seam (`backend.rs`), and provider resolution -(`provider.rs`) are wired up **only** through `#[cfg(unix)]` code paths. On Windows the binary -runs ACP-only and drives `onde` directly, so much of `backend.rs` and `provider.rs` is -legitimately unused there and the dead-code lint is suppressed *on non-Unix targets only*. +The interactive client is `#[cfg(unix)]`-only. The `InferenceBackend` seam (`backend.rs`) and +provider resolution (`provider.rs`) are consumed by both the interactive client and the ACP +server, but several of their items are reached only through the Unix-only interactive paths, so +the dead-code lint is suppressed *on non-Unix targets only*. Consequence: code can pass clippy on macOS/Linux but fail on the Windows target (or vice versa). When touching `backend.rs`, `provider.rs`, or the interactive path, keep the `cfg` gates intact —

index ad39789..a004c11 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -11,11 +11,11 @@ //! The trait exposes neither `onde` nor OpenAI types, so the loop does not depend //! on a specific backend. //! -//! The whole backend seam is wired up only through the interactive client, which -//! is `#[cfg(unix)]` (see `run_interactive` in `main.rs` and `mod tui` in -//! `chat.rs`). On non-Unix targets the binary runs ACP-only and drives `onde` -//! directly, so every item here is legitimately unused there. Suppress the -//! dead-code lint on those targets only — Unix builds still get full coverage. +//! The seam is consumed by both surfaces: the interactive client (`#[cfg(unix)]`, +//! see `run_interactive` in `main.rs` and `mod tui` in `chat.rs`) and the ACP +//! server's prompt loop. Some items are still reached only through the +//! Unix-only interactive paths, so the dead-code lint stays suppressed on +//! non-Unix targets only — Unix builds keep full coverage. #![cfg_attr(not(unix), allow(dead_code))] use std::sync::Arc;

index 638badf..77a5476 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1606,6 +1606,27 @@ mod tui { specs } + /// Close out a cancelled round in backend history: the results of tools + /// that already ran this round, plus cancellation notes for `unreached` + /// calls. Leaving a round's tool calls unanswered breaks strict + /// OpenAI-compatible endpoints on the session's next request. + async fn abandon_round( + backend: &dyn InferenceBackend, + mut tool_results: Vec<ToolResult>, + unreached: &[crate::backend::ToolCall], + ) { + for pending in unreached { + tool_results.push(ToolResult { + tool_call_id: pending.id.clone(), + content: format!( + "`{}` was not executed: the user cancelled the turn.", + pending.name + ), + }); + } + backend.record_cancelled_tool_results(tool_results).await; + } + /// run the tool-calling loop off the main thread, posting updates via `tx`. /// dropping `tx` signals completion to the event loop. async fn run_inference_task( @@ -1667,7 +1688,16 @@ mod tui { let mut tool_results = Vec::new(); - for tc in &result.tool_calls { + for (call_index, tc) in result.tool_calls.iter().enumerate() { + // The UI drops the receiver on Ctrl+C or quit. Stop the turn + // at the next boundary instead of burning model rounds (and + // possibly running granted tools) in the background. + if tx.is_closed() { + log::info!("turn cancelled by the user — stopping the tool loop"); + abandon_round(&*backend, tool_results, &result.tool_calls[call_index..]).await; + return; + } + log::info!( " → {}({})", tc.name, @@ -1703,12 +1733,26 @@ mod tui { permissions::grant_for_session(TUI_SESSION, &tc.name); crate::tools::execute_tool(&tc.name, &tc.arguments).await } - // An explicit "no", or the UI dropped the channel - // (cancel/quit) — either way, do not run the tool. - Ok(ApprovalChoice::Deny) | Err(_) => { + Ok(ApprovalChoice::Deny) => { log::info!(" ✗ {} denied by user", tc.name); permissions::user_denial(&tc.name) } + // The UI dropped the reply channel (Ctrl+C or + // quit): the whole turn is over, not just this + // call. Close out the round and stop instead of + // continuing rounds in the background. + Err(_) => { + log::info!( + "turn cancelled at the approval prompt — stopping the tool loop" + ); + abandon_round( + &*backend, + tool_results, + &result.tool_calls[call_index..], + ) + .await; + return; + } } } }; @@ -1720,6 +1764,14 @@ mod tui { }); } + // Cancelled while the round's tools ran: record what executed and + // stop before paying for another model round nobody will see. + if tx.is_closed() { + log::info!("turn cancelled by the user — skipping the next model round"); + abandon_round(&*backend, tool_results, &[]).await; + return; + } + // on the last round, pass no tools so the model must produce text — // that's also the round we can stream on-device. let next_tools = if round < MAX_TOOL_ROUNDS {

index 293bdb0..bccf236 100644 --- a/src/main.rs +++ b/src/main.rs @@ -949,9 +949,10 @@ impl SiGitAgent { *guard = Some(args.cwd.clone()); } - // A reloaded session starts fresh: grants and plan mode from the - // previous life of this session id must not carry over. - permissions::reset_session(&args.session_id.to_string()); + // A session boundary: grants and plan mode from the previous life of + // this session id must not carry over — and since one shared engine + // means one live conversation, state for every other id is dead too. + permissions::reset_all(); // tool calls use relative paths, so we need to match the editor's cwd if args.cwd.is_dir() @@ -988,6 +989,9 @@ impl SiGitAgent { args: ForkSessionRequest, ) -> agent_client_protocol::Result<ForkSessionResponse> { let new_id = SessionId::new(uuid::Uuid::new_v4().to_string()); + // Session boundary: permission grants and plan mode never cross it + // (see handle_load_session), so a fork starts with a clean slate. + permissions::reset_all(); log::info!( "fork_session: from={} new={new_id}, cwd={}, additional_directories={:?}", args.session_id, @@ -1035,6 +1039,9 @@ impl SiGitAgent { args: NewSessionRequest, ) -> agent_client_protocol::Result<NewSessionResponse> { let session_id = SessionId::new(uuid::Uuid::new_v4().to_string()); + // Session boundary: permission grants and plan mode never cross it + // (see handle_load_session), so stale ids stop accumulating state. + permissions::reset_all(); log::info!( "new_session: id={session_id}, cwd={}, additional_directories={:?}", args.cwd.display(), @@ -2846,6 +2853,29 @@ async fn run_acp_server() -> anyhow::Result<()> { needs_download, )); + // Honor the explicit provider override (OPENAI_BASE_URL/OPENAI_API_KEY or + // an active providers.toml profile) in ACP mode too — the interactive + // client already does. Without this the override was silently ignored here + // and prompts insisted on a local model. It is also what lets the ACP + // integration test drive the agent against a scripted endpoint + // (tests/acp_permissions.rs). The model picker still shows the local + // selection; overrides are a power-user escape hatch, not a tier. + if let Some(cfg) = provider::active_provider() { + log::info!( + "inference: using {} (model {}) at {}", + cfg.display_name, + cfg.model, + cfg.base_url + ); + let override_backend: Arc<dyn InferenceBackend> = Arc::new(OpenAiBackend::new( + cfg.base_url, + cfg.api_key, + cfg.model, + Some(system_prompt_for_model(true).to_string()), + )); + *state.backend.lock().await = override_backend; + } + let stdin = tokio::io::stdin().compat(); let stdout = tokio::io::stdout().compat_write(); let transport = ByteStreams::new(stdout, stdin);

index ce9e796..cf1e6c3 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -184,6 +184,18 @@ pub fn reset_session(session: &str) { map.remove(session); } +/// Drop the recorded state for *every* session. Called at ACP session +/// boundaries (new/load/fork): the agent drives one shared engine, so only one +/// conversation is live at a time and grants must never cross a boundary. This +/// also keeps the map from accumulating entries for session ids that will +/// never be used again. +pub fn reset_all() { + let mut map = sessions() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + map.clear(); +} + /// One-line status summary for `/permissions` and `/status`. pub fn describe(session: &str) -> String { let plan = if plan_mode(session) { "on" } else { "off" };

index debd292..84f8cf6 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -8,11 +8,11 @@ //! endpoint and tier are built in, and the session token is the credential. //! 3. On-device: no login and no override, so inference runs locally. //! -//! Provider resolution is consumed only by the interactive client, which is -//! `#[cfg(unix)]`. The display helpers (`cloud_tier_label`, `CLOUD_TIERS`) are -//! still used cross-platform by `/models`, but the resolution path is dead on -//! non-Unix targets, where the binary runs ACP-only and on-device. Suppress the -//! dead-code lint there only — Unix builds keep full coverage. +//! Provider resolution runs at startup in both modes: the interactive client +//! picks its whole backend from it, and `run_acp_server` applies the explicit +//! override (env or `providers.toml`) the same way. Some items are still wired +//! up only through `#[cfg(unix)]` interactive paths, so the dead-code lint +//! stays suppressed on non-Unix targets only — Unix builds keep full coverage. #![cfg_attr(not(unix), allow(dead_code))] use std::path::PathBuf;

new file mode 100644 index 0000000..3e11d23 --- /dev/null +++ b/tests/acp_permissions.rs @@ -0,0 +1,353 @@ +//! End-to-end ACP permission round-trip against the real binary. +//! +//! Spawns `sigit` in ACP mode (stdin piped, so not a TTY) wired to a scripted +//! OpenAI-compatible SSE endpoint via the `OPENAI_BASE_URL` override, then +//! drives newline-delimited JSON-RPC over stdio. The scripted model calls +//! `run_command` — a mutating tool — so the agent must send +//! `session/request_permission` mid-turn (the exact path the spawned-handler / +//! `turn_lock` design exists for). The test answers it twice: +//! +//! 1. `cancelled` — the prompt must stop with `stopReason: "cancelled"`, and +//! the *next* request to the endpoint must show the abandoned round closed +//! out with `role: "tool"` results, or a strict OpenAI-compatible endpoint +//! would reject the whole session. +//! 2. `selected: allow_once` — the tool must actually execute and its output +//! travel back to the endpoint as a tool result. + +use std::collections::VecDeque; +use std::io::{BufRead, BufReader, Read, Write}; +use std::net::TcpListener; +use std::process::{Child, ChildStdin, Command, Stdio}; +use std::sync::mpsc::{Receiver, channel}; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use serde_json::{Value, json}; + +const TIMEOUT: Duration = Duration::from_secs(60); + +// ── Scripted OpenAI-compatible endpoint ───────────────────────────────────── + +fn sse_body(events: &[Value]) -> String { + let mut body = String::new(); + for event in events { + body.push_str("data: "); + body.push_str(&event.to_string()); + body.push_str("\n\n"); + } + body.push_str("data: [DONE]\n\n"); + body +} + +fn sse_tool_call(id: &str, name: &str, arguments: &str) -> String { + sse_body(&[json!({ + "choices": [{"delta": {"tool_calls": [{ + "index": 0, + "id": id, + "function": {"name": name, "arguments": arguments}, + }]}}] + })]) +} + +fn sse_text(text: &str) -> String { + sse_body(&[json!({"choices": [{"delta": {"content": text}}]})]) +} + +/// Serves one scripted SSE response per request and records each request body. +struct FakeEndpoint { + port: u16, + requests: Arc<Mutex<Vec<Value>>>, +} + +fn start_fake_endpoint(responses: Vec<String>) -> FakeEndpoint { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind fake endpoint"); + let port = listener.local_addr().unwrap().port(); + let requests: Arc<Mutex<Vec<Value>>> = Arc::default(); + let recorded = Arc::clone(&requests); + let queue = Mutex::new(VecDeque::from(responses)); + + std::thread::spawn(move || { + // `connection: close` below means one request per connection, so the + // serial accept loop matches the agent's serial completion requests. + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + let mut reader = BufReader::new(match stream.try_clone() { + Ok(clone) => clone, + Err(_) => continue, + }); + let mut content_length = 0usize; + loop { + let mut line = String::new(); + if reader.read_line(&mut line).unwrap_or(0) == 0 { + break; + } + let line = line.trim(); + if line.is_empty() { + break; + } + if let Some(length) = line.to_ascii_lowercase().strip_prefix("content-length:") { + content_length = length.trim().parse().unwrap_or(0); + } + } + let mut body = vec![0u8; content_length]; + if reader.read_exact(&mut body).is_err() { + continue; + } + if let Ok(request) = serde_json::from_slice::<Value>(&body) { + recorded.lock().unwrap().push(request); + } + let payload = queue + .lock() + .unwrap() + .pop_front() + .unwrap_or_else(|| sse_text("out of scripted responses")); + let response = format!( + "HTTP/1.1 200 OK\r\ncontent-type: text/event-stream\r\n\ + content-length: {}\r\nconnection: close\r\n\r\n{}", + payload.len(), + payload + ); + let _ = stream.write_all(response.as_bytes()); + } + }); + + FakeEndpoint { port, requests } +} + +// ── ACP client over the binary's stdio ────────────────────────────────────── + +struct AgentUnderTest { + child: Child, + stdin: ChildStdin, + incoming: Receiver<Value>, + next_id: u64, +} + +fn spawn_agent(port: u16, config_dir: &std::path::Path) -> AgentUnderTest { + let mut child = Command::new(env!("CARGO_BIN_EXE_sigit")) + .env("OPENAI_BASE_URL", format!("http://127.0.0.1:{port}")) + .env("OPENAI_API_KEY", "test-key") + .env("SIGIT_MODEL", "scripted-model") + .env("SIGIT_CONFIG_DIR", config_dir) + .env("SIGIT_MCP", "off") + // A fresh config dir means the default permission mode, `ask` — make + // sure the environment can't turn the gate off underneath the test. + .env_remove("SIGIT_PERMISSIONS") + .env_remove("SIGIT_LOCAL_INFERENCE") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sigit in ACP mode"); + + let stdout = child.stdout.take().unwrap(); + let (message_tx, incoming) = channel(); + std::thread::spawn(move || { + for line in BufReader::new(stdout).lines() { + let Ok(line) = line else { break }; + if let Ok(message) = serde_json::from_str::<Value>(&line) + && message_tx.send(message).is_err() + { + break; + } + } + }); + + let stdin = child.stdin.take().unwrap(); + AgentUnderTest { + child, + stdin, + incoming, + next_id: 0, + } +} + +impl AgentUnderTest { + fn send(&mut self, message: Value) { + let mut line = message.to_string(); + line.push('\n'); + self.stdin + .write_all(line.as_bytes()) + .expect("write to agent stdin"); + self.stdin.flush().expect("flush agent stdin"); + } + + fn request(&mut self, method: &str, params: Value) -> u64 { + self.next_id += 1; + let id = self.next_id; + self.send(json!({"jsonrpc": "2.0", "id": id, "method": method, "params": params})); + id + } + + fn respond(&mut self, id: Value, result: Value) { + self.send(json!({"jsonrpc": "2.0", "id": id, "result": result})); + } + + /// Skip notifications and unrelated traffic until `matches` is satisfied. + fn wait_for(&mut self, what: &str, matches: impl Fn(&Value) -> bool) -> Value { + let deadline = Instant::now() + TIMEOUT; + loop { + let remaining = deadline.saturating_duration_since(Instant::now()); + match self.incoming.recv_timeout(remaining) { + Ok(message) if matches(&message) => return message, + Ok(_) => continue, + Err(_) => panic!("timed out waiting for {what}"), + } + } + } + + /// The response to one of *our* requests (has our id, no `method`). + fn wait_for_response(&mut self, id: u64) -> Value { + let response = self.wait_for(&format!("response to request {id}"), |message| { + message["id"] == id && message.get("method").is_none() + }); + assert!( + response.get("error").is_none(), + "request {id} failed: {response}" + ); + response + } + + /// A request *from* the agent (has a `method` and its own id). + fn wait_for_agent_request(&mut self, method: &str) -> Value { + self.wait_for(&format!("agent request {method}"), |message| { + message["method"] == method && message.get("id").is_some() + }) + } +} + +impl Drop for AgentUnderTest { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +// ── The round-trip ────────────────────────────────────────────────────────── + +#[test] +fn permission_round_trip_cancel_then_allow() { + let endpoint = start_fake_endpoint(vec![ + sse_tool_call("call_1", "run_command", r#"{"command":"echo sigit-first"}"#), + sse_tool_call( + "call_2", + "run_command", + r#"{"command":"echo sigit-approved"}"#, + ), + sse_text("done"), + ]); + + let scratch = std::env::temp_dir().join(format!("sigit_acp_perm_{}", std::process::id())); + let config_dir = scratch.join("config"); + let cwd = scratch.join("cwd"); + std::fs::create_dir_all(&config_dir).unwrap(); + std::fs::create_dir_all(&cwd).unwrap(); + + let mut agent = spawn_agent(endpoint.port, &config_dir); + + let id = agent.request( + "initialize", + json!({"protocolVersion": 1, "clientCapabilities": {}}), + ); + agent.wait_for_response(id); + + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []})); + let session_id = agent.wait_for_response(id)["result"]["sessionId"] + .as_str() + .expect("session id") + .to_string(); + + // ── Turn 1: cancel at the permission gate ─────────────────────────── + let prompt_id = agent.request( + "session/prompt", + json!({ + "sessionId": session_id, + "prompt": [{"type": "text", "text": "run the first command"}], + }), + ); + + let permission = agent.wait_for_agent_request("session/request_permission"); + let params = &permission["params"]; + assert_eq!(params["sessionId"], session_id.as_str()); + let title = params["toolCall"]["title"].as_str().expect("title"); + assert!( + title.contains("run_command") && title.contains("echo sigit-first"), + "the dialog must show the tool and its arguments, got: {title}" + ); + assert_eq!( + params["toolCall"]["rawInput"]["command"], "echo sigit-first", + "full arguments must travel as rawInput" + ); + let option_ids: Vec<&str> = params["options"] + .as_array() + .expect("options") + .iter() + .map(|option| option["optionId"].as_str().unwrap_or_default()) + .collect(); + assert_eq!(option_ids, ["allow_once", "allow_session", "reject_once"]); + + agent.respond( + permission["id"].clone(), + json!({"outcome": {"outcome": "cancelled"}}), + ); + + let response = agent.wait_for_response(prompt_id); + assert_eq!(response["result"]["stopReason"], "cancelled"); + + // ── Turn 2: history must be repaired; then approve once ───────────── + let prompt_id = agent.request( + "session/prompt", + json!({ + "sessionId": session_id, + "prompt": [{"type": "text", "text": "run the second command"}], + }), + ); + + let permission = agent.wait_for_agent_request("session/request_permission"); + agent.respond( + permission["id"].clone(), + json!({"outcome": {"outcome": "selected", "optionId": "allow_once"}}), + ); + + let response = agent.wait_for_response(prompt_id); + assert_eq!(response["result"]["stopReason"], "end_turn"); + + // ── What the endpoint saw ──────────────────────────────────────────── + let requests = endpoint.requests.lock().unwrap(); + assert_eq!(requests.len(), 3, "expected exactly three completions"); + + // Request 2 replays the full history: the cancelled round's tool call + // must be answered by a `role: "tool"` message, not left dangling. + let messages = requests[1]["messages"].as_array().expect("messages"); + let call_position = messages + .iter() + .position(|message| message["tool_calls"][0]["id"] == "call_1") + .expect("cancelled turn's assistant tool call in replayed history"); + let repair = &messages[call_position + 1]; + assert_eq!(repair["role"], "tool", "dangling tool call not closed out"); + assert_eq!(repair["tool_call_id"], "call_1"); + assert!( + repair["content"] + .as_str() + .unwrap_or_default() + .contains("cancelled"), + "repair message should say the turn was cancelled: {repair}" + ); + + // Request 3 carries the approved call's real output. + let messages = requests[2]["messages"].as_array().expect("messages"); + let result = messages + .iter() + .find(|message| message["role"] == "tool" && message["tool_call_id"] == "call_2") + .expect("tool result for the approved call"); + assert!( + result["content"] + .as_str() + .unwrap_or_default() + .contains("sigit-approved"), + "the approved command's output should reach the endpoint: {result}" + ); + + drop(agent); + let _ = std::fs::remove_dir_all(&scratch); +}