diff --git a/apps/maple-agent/CLAUDE.md b/apps/maple-agent/CLAUDE.md index 7e04621de..8d271ea11 100644 --- a/apps/maple-agent/CLAUDE.md +++ b/apps/maple-agent/CLAUDE.md @@ -9,6 +9,12 @@ GPUI desktop app for Maple. Workspace crates: `app` (package `maple-agent-app`, `crates/pi-agent-core`, `crates/pi-coding-agent`, which must not depend on Maple crates. See `README.md` for the layout, prerequisites, and command line modes. +Keep `crates/pi-*` unchanged for Maple product work. Implement policy and UI +behavior through the existing host, extension, and operations interfaces in +`crates/maple-agent` and `app`. Exceptions must address a critical need and +explain why existing interfaces are insufficient; see `crates/UPSTREAM.md` for +upstream review work. + Packaged Dev and Prod OAuth use their compiled Auth origins and a one-attempt loopback listener in `app/src/hosted_oauth.rs`; unpackaged builds retain the provider callback paste flow. Keep the listener bound to `127.0.0.1` through diff --git a/apps/maple-agent/README.md b/apps/maple-agent/README.md index 9ea121c5a..d6c106946 100644 --- a/apps/maple-agent/README.md +++ b/apps/maple-agent/README.md @@ -70,6 +70,24 @@ agent runtime; its desktop Agent page points users to Maple Work. attachments (picker, paste, or drag and drop), a per-message Copy button, and a context-window indicator. Every tool call runs without asking for approval. +- Shell tool calls start in the task's saved folder on the execution computer. + A `cd` inside a command does not affect later calls. Repository searches + should name the intended checkout/worktree with explicit absolute paths. + Maple supplies a 30-minute default deadline to allow slow builds, and the + call can request a longer + `timeout` explicitly. Search guidance asks for a short timeout (normally + 30 seconds) and a narrower scope if it expires; that shorter limit applies + only when the call requests it. + Quiet commands report their starting folder, elapsed time, time since output and + deadline every five seconds, including on collapsed tool cards. Stop and + deadlines use Pi's process-group cleanup on Unix (process-tree cleanup on Windows). + Maple bounds the whole operation, including setup and stdin delivery, and allows + up to one second for cooperative cancellation before dropping a stuck operation. + The execution policy and progress reporting live in Maple; the Pi crates are unchanged. + These are execution safeguards, not a filesystem sandbox: repository searches + must still choose the correct folder and bounded scope, and deliberately + detached processes can escape a Unix process group. `head` bounds output, + not runtime. - Slash commands in the composer: `/btw` asks a side question the task never sees, plus `/compact`, `/new`, `/pin`, `/web`, `/model`, and `/help`. Skills and prompt templates appear in the same list. diff --git a/apps/maple-agent/app/src/ui/chat/cache.rs b/apps/maple-agent/app/src/ui/chat/cache.rs index 21467f469..97eceaf67 100644 --- a/apps/maple-agent/app/src/ui/chat/cache.rs +++ b/apps/maple-agent/app/src/ui/chat/cache.rs @@ -264,6 +264,8 @@ pub(super) struct ItemDerived { pub(super) attachments: Vec, /// Readable tool output for the expanded card. pub(super) output_text: Option, + /// Shell execution status stays visible even on a collapsed card. + pub(super) shell_status: Option, /// `input: {json}` for the expanded card. pub(super) input_line: Option, /// +/- lines of an edit or write tool, capped at `MAX_DIFF_LINES`. @@ -290,6 +292,7 @@ impl ItemDerived { }) .collect(), output_text, + shell_status: super::transcript::shell_execution_status(item).map(SharedString::from), input_line: tool_input_line(item).map(SharedString::from), diff_lines: Arc::new(diff_lines_for(item)), external_agent: external_agent_activity(item).map(Arc::new), diff --git a/apps/maple-agent/app/src/ui/chat/tests.rs b/apps/maple-agent/app/src/ui/chat/tests.rs index cbfb7272d..863fce822 100644 --- a/apps/maple-agent/app/src/ui/chat/tests.rs +++ b/apps/maple-agent/app/src/ui/chat/tests.rs @@ -10688,8 +10688,14 @@ mod state_tests { chat.booting = false; chat.replace_timeline(vec![ AgentTimelineItem { - status: Some("completed".to_string()), + status: Some("running".to_string()), title: Some("Terminal: cargo test".to_string()), + output: Some(serde_json::json!({"structuredContent": {"shellExecution": { + "cwd": "/fixture/worktree", + "elapsedSeconds": 65, + "outputIdleSeconds": 60, + "timeoutSeconds": 120, + }}})), ..item("tool-1", "tool", None) }, AgentTimelineItem { @@ -10711,6 +10717,19 @@ mod state_tests { cx.simulate_resize(gpui::size(px(1200.), px(800.))); let card = cx.debug_bounds("tool-card").expect("the tool card renders"); + assert!( + cx.debug_bounds("shell-execution-status").is_some(), + "shell status remains visible on a collapsed card" + ); + chat.read_with(cx, |chat, _| { + let status = + super::super::transcript::shell_execution_status(&chat.timeline[0]).unwrap(); + assert!(status.contains("/fixture/worktree")); + assert!(status.contains("Running 65s")); + assert!(status.contains("no output for 60s")); + assert!(status.contains("deadline 120s")); + assert!(status.contains("Stop cancels command")); + }); let tool_title = cx .debug_bounds("tool-title") .expect("the tool card title renders"); diff --git a/apps/maple-agent/app/src/ui/chat/transcript.rs b/apps/maple-agent/app/src/ui/chat/transcript.rs index 392313a77..02d9c54ec 100644 --- a/apps/maple-agent/app/src/ui/chat/transcript.rs +++ b/apps/maple-agent/app/src/ui/chat/transcript.rs @@ -1130,7 +1130,7 @@ fn render_tool( let raw = item.title.as_deref().unwrap_or(&item.item_type); SharedString::from(tool_label_title(raw).to_string()) }); - let card = div() + let mut card = div() .id(gpui::SharedString::from(format!("tool-toggle-{item_id}"))) .debug_selector(|| "tool-card".to_string()) .role(gpui::Role::DisclosureTriangle) @@ -1201,8 +1201,20 @@ fn render_tool( theme::text_muted(), )), ); - // Collapsed: the header alone; the summary takes over the title when - // it lands. Only expanded cards pay for the derived payload strings. + if running + && shell_execution(item).is_some() + && let Some(status) = &transcript.derived.get(item, revision).shell_status + { + card = card.child( + div() + .text_xs() + .text_color(gpui::rgb(theme::text_secondary())) + .debug_selector(|| "shell-execution-status".to_string()) + .child(status.clone()), + ); + } + // Collapsed: the header and live shell status; the summary takes over the title when + // it lands. Expanded cards also show the cached arguments and output. if !details { return div().child(card); } @@ -1390,6 +1402,30 @@ pub(super) fn tool_output_markdown(item: &AgentTimelineItem) -> Option { (!trimmed.is_empty()).then(|| trimmed.to_string()) } +fn shell_execution(item: &AgentTimelineItem) -> Option<&serde_json::Value> { + item.output + .as_ref()? + .get("structuredContent")? + .get("shellExecution") +} + +pub(super) fn shell_execution_status(item: &AgentTimelineItem) -> Option { + let execution = shell_execution(item)?; + let cwd = execution.get("cwd")?.as_str()?; + let elapsed = execution.get("elapsedSeconds")?.as_u64()?; + let idle = execution.get("outputIdleSeconds")?.as_u64()?; + let deadline = execution + .get("timeoutSeconds") + .and_then(serde_json::Value::as_f64) + .map_or_else( + || "no deadline".to_string(), + |seconds| format!("deadline {seconds}s"), + ); + Some(format!( + "Started in {cwd}\nRunning {elapsed}s · no output for {idle}s · {deadline} · Stop cancels command" + )) +} + fn extract_output_text(value: &serde_json::Value) -> Option { match value { serde_json::Value::String(text) => Some(text.clone()), diff --git a/apps/maple-agent/crates/maple-agent/src/agent/runs.rs b/apps/maple-agent/crates/maple-agent/src/agent/runs.rs index 1f3a65f27..fd59b85af 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/runs.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/runs.rs @@ -41,7 +41,9 @@ use super::{ }; mod live; +mod shell_progress; use live::LiveSnapshot; +use shell_progress::ShellProgress; const AGENT_RUN_EVENT_CAPACITY: usize = 256; const MAX_DESKTOP_QUEUE_ITEMS: usize = 16; @@ -1588,7 +1590,11 @@ impl AgentRuntime { } = run; let attachments = AgentAttachmentStore::new(Arc::clone(self.store.store()), self.store.account()); - let watch = RunWatch::default(); + let watch = RunWatch { + state: Arc::default(), + shells: ShellProgress::new(session.extension_context().cwd()), + }; + let shell_status = watch.shells.start(events.clone()); let listener = session.subscribe({ let events = events.clone(); let watch = watch.clone(); @@ -1749,6 +1755,7 @@ impl AgentRuntime { run.accepting = false; } session.unsubscribe(listener); + shell_status.stop().await; let cancelled = stopped.is_cancelled(); if cancelled || failure.is_some() || deferred.failure.is_some() { @@ -1982,9 +1989,10 @@ struct DrivenRun { /// What a run's listener saw: the usage of its replies and whether Pi /// retried. -#[derive(Clone, Default)] +#[derive(Clone)] struct RunWatch { state: Arc>, + shells: Arc, } #[derive(Default)] @@ -2030,6 +2038,7 @@ impl RunWatch { } else { let tool = match event { pi_agent_core::AgentEvent::ToolExecutionStart { tool_call_id, .. } + | pi_agent_core::AgentEvent::ToolExecutionUpdate { tool_call_id, .. } | pi_agent_core::AgentEvent::ToolExecutionEnd { tool_call_id, .. } => { Some(tool_call_id.as_str()) } @@ -2037,9 +2046,7 @@ impl RunWatch { }; (projected, tool, false) }; - for row in rows { - events.publish_row(AgentRunEvent::TimelineItem(row), tool, completed); - } + self.shells.publish(event, rows, tool, completed, events); } AgentSessionEvent::EntryAppended { entry } => events.note_source(entry), AgentSessionEvent::RetryStart { .. } => lock(&self.state).retried = true, diff --git a/apps/maple-agent/crates/maple-agent/src/agent/runs/shell_progress.rs b/apps/maple-agent/crates/maple-agent/src/agent/runs/shell_progress.rs new file mode 100644 index 000000000..b1c3e464a --- /dev/null +++ b/apps/maple-agent/crates/maple-agent/src/agent/runs/shell_progress.rs @@ -0,0 +1,275 @@ +//! Run-owned shell status. Pi's partial output contract stays unchanged. + +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use pi_agent_core::AgentEvent; +use pi_ai::Content; +use pi_coding_agent::SessionMessage; +use serde_json::json; +use tokio::sync::Notify; + +use super::{AgentRunEvent, AgentTimelineItem, RunEvents, lock}; +use crate::agent::tools::shell::timeout_seconds; + +const STATUS_INTERVAL: Duration = Duration::from_secs(5); + +pub(super) struct ShellProgress { + cwd: Option, + calls: Mutex>, + wake: Notify, +} + +struct RunningShell { + row: AgentTimelineItem, + started: Instant, + last_output: Instant, + timeout: f64, +} + +impl RunningShell { + fn decorate(&self, row: &mut AgentTimelineItem, cwd: &std::path::Path) { + let output = row.output.get_or_insert_with(|| json!({})); + let details = &mut output["structuredContent"]; + if !details.is_object() { + *details = json!({}); + } + details["shellExecution"] = json!({ + "cwd": cwd.to_string_lossy(), + "timeoutSeconds": self.timeout, + "elapsedSeconds": self.started.elapsed().as_secs(), + "outputIdleSeconds": self.last_output.elapsed().as_secs(), + }); + } +} + +impl ShellProgress { + pub(super) fn new(cwd: Option) -> Arc { + Arc::new(Self { + cwd, + calls: Mutex::new(HashMap::new()), + wake: Notify::new(), + }) + } + + pub(super) fn publish( + &self, + event: &AgentEvent, + mut rows: Vec, + tool: Option<&str>, + completed: bool, + events: &RunEvents, + ) { + // Hold this through publication: a heartbeat and End cannot overtake + // each other between checking ownership and sending their rows. + let mut calls = lock(&self.calls); + match event { + AgentEvent::ToolExecutionStart { + tool_call_id, + tool_name, + args, + } if matches!(tool_name.as_str(), "bash" | "powershell") => { + if self.cwd.is_some() + && let Some(row) = rows.first() + { + let now = Instant::now(); + calls.insert( + tool_call_id.clone(), + RunningShell { + row: row.clone(), + started: now, + last_output: now, + timeout: timeout_seconds(args.get("timeout").and_then(|v| v.as_f64())), + }, + ); + self.wake.notify_one(); + } + } + AgentEvent::ToolExecutionUpdate { + tool_call_id, + partial_result, + tool_name, + .. + } => { + // Projection and publication are separate steps. An update + // projected before End may arrive here after it; fence that + // stale row using the same lock as terminal publication. + if matches!(tool_name.as_str(), "bash" | "powershell") + && self.cwd.is_some() + && !calls.contains_key(tool_call_id) + { + return; + } + if let Some(call) = calls.get_mut(tool_call_id) + && partial_result.content.iter().any( + |content| matches!(content, Content::Text(text) if !text.text.is_empty()), + ) + { + call.last_output = Instant::now(); + } + } + AgentEvent::ToolExecutionEnd { tool_call_id, .. } => { + calls.remove(tool_call_id); + } + _ => {} + } + for row in &mut rows { + if let Some(call) = tool.and_then(|tool| calls.get_mut(tool)) + && let Some(cwd) = &self.cwd + { + call.decorate(row, cwd); + call.row = row.clone(); + } + events.publish_row(AgentRunEvent::TimelineItem(row.clone()), tool, completed); + } + } + + pub(super) fn start(self: &Arc, events: RunEvents) -> ShellStatusTask { + let progress = Arc::clone(self); + ShellStatusTask(tokio::spawn(async move { + loop { + progress.wake.notified().await; + loop { + tokio::time::sleep(STATUS_INTERVAL).await; + if !progress.heartbeat(&events) { + break; + } + } + } + })) + } + + fn heartbeat(&self, events: &RunEvents) -> bool { + let calls = lock(&self.calls); + if let Some(cwd) = &self.cwd { + for (id, call) in calls.iter() { + let mut row = call.row.clone(); + call.decorate(&mut row, cwd); + events.publish_row(AgentRunEvent::TimelineItem(row), Some(id), false); + } + } + !calls.is_empty() + } +} + +pub(super) struct ShellStatusTask(tokio::task::JoinHandle<()>); + +impl ShellStatusTask { + pub(super) async fn stop(mut self) { + self.0.abort(); + let _ = (&mut self.0).await; + } +} + +impl Drop for ShellStatusTask { + fn drop(&mut self) { + self.0.abort(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::agent::{AgentEventDispatcher, tests::Recorder, timeline}; + use pi_agent_core::AgentToolResult; + + #[test] + fn heartbeat_keeps_output_and_truncation_but_never_revives_a_finished_call() { + let dir = tempfile::tempdir().unwrap(); + let progress = ShellProgress::new(Some(dir.path().to_path_buf())); + let (events, mut receiver) = RunEvents::new( + AgentEventDispatcher::new(Arc::new(Recorder::default())), + "task", + "run", + ); + for id in ["first", "second"] { + let mut row = timeline::tool_result_item(id.into(), &[], None, false, 0); + row.status = Some("running".into()); + progress.publish( + &AgentEvent::ToolExecutionStart { + tool_call_id: id.into(), + tool_name: "bash".into(), + args: json!({"command": "fixture"}), + }, + vec![row], + Some(id), + false, + &events, + ); + } + let partial = AgentToolResult::text("retained output") + .with_details(json!({"fullOutputPath": "fixture.log"})); + let mut row = timeline::tool_result_item( + "second".into(), + &partial.content, + partial.details.as_ref(), + false, + 0, + ); + row.status = Some("running".into()); + progress.publish( + &AgentEvent::ToolExecutionUpdate { + tool_call_id: "second".into(), + tool_name: "bash".into(), + args: json!({}), + partial_result: partial, + }, + vec![row], + Some("second"), + false, + &events, + ); + progress.publish( + &AgentEvent::ToolExecutionEnd { + tool_call_id: "first".into(), + tool_name: "bash".into(), + result: AgentToolResult::text("done"), + is_error: false, + }, + vec![timeline::tool_result_item( + "first".into(), + &[Content::text("done")], + None, + false, + 0, + )], + Some("first"), + false, + &events, + ); + while receiver.try_recv().is_ok() {} + let mut stale = + timeline::tool_result_item("first".into(), &[Content::text("late")], None, false, 0); + stale.status = Some("running".into()); + progress.publish( + &AgentEvent::ToolExecutionUpdate { + tool_call_id: "first".into(), + tool_name: "bash".into(), + args: json!({}), + partial_result: AgentToolResult::text("late"), + }, + vec![stale], + Some("first"), + false, + &events, + ); + assert!( + receiver.try_recv().is_err(), + "a previously projected update cannot overtake End" + ); + assert!(progress.heartbeat(&events)); + let AgentRunEvent::TimelineItem(row) = receiver.try_recv().unwrap() else { + panic!("heartbeat row") + }; + assert_eq!(row.id, "second"); + let output = row.output.unwrap(); + assert_eq!(output["text"], "retained output"); + assert_eq!(output["structuredContent"]["fullOutputPath"], "fixture.log"); + assert!( + receiver.try_recv().is_err(), + "the finished call emits no heartbeat" + ); + } +} diff --git a/apps/maple-agent/crates/maple-agent/src/agent/runtime.rs b/apps/maple-agent/crates/maple-agent/src/agent/runtime.rs index 020328c28..f0f94000d 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/runtime.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/runtime.rs @@ -793,7 +793,11 @@ impl AgentRuntime { /// delivered together, the saved thinking default, and the host's opening /// instructions after Pi's own. fn settings(&self) -> Settings { - let appended = self.host.harness_instructions().trim().to_string(); + let appended = format!( + "{}\n\n{}", + tools::shell::GUIDANCE, + self.host.harness_instructions().trim() + ); Settings { retry: RETRY, // The transport times out a response that does not start or diff --git a/apps/maple-agent/crates/maple-agent/src/agent/shell_tests.rs b/apps/maple-agent/crates/maple-agent/src/agent/shell_tests.rs new file mode 100644 index 000000000..afa033466 --- /dev/null +++ b/apps/maple-agent/crates/maple-agent/src/agent/shell_tests.rs @@ -0,0 +1,135 @@ +//! Execution-host folder selection and quiet shell progress, without remote +//! accounts, live services or the real application's store. +#![cfg(unix)] + +use super::*; + +#[tokio::test] +async fn saved_task_worktree_survives_a_different_runtime_default_and_cold_load() { + let mut harness = Harness::new().await; + let worktree = tempfile::tempdir().unwrap(); + std::fs::write(worktree.path().join("window.rs"), "worktree-fixture").unwrap(); + let task = harness + .handle + .create_session(Some(AgentCreateSessionRequest { + project_root: Some(worktree.path().to_string_lossy().into_owned()), + title: None, + model: None, + context_limit: None, + mcp_server_names: Some(Vec::new()), + })) + .await + .unwrap() + .session + .id; + for cold in [false, true] { + if cold { + harness.restart().await; + harness.start().await; + } + harness.faux.push_message(vec![faux_tool_call( + "bash", + json!({"command": "pwd; cat ./window.rs"}), + )]); + harness.faux.push_text("Done."); + let mut run = harness.send(&task, "Read the fixture").await; + assert_eq!(finished(&mut run).await, AgentRunTerminal::Completed); + let rows = harness.recorder.live_rows(&run.run_id); + let row = rows + .iter() + .rev() + .find(|row| row.item_type == "tool") + .unwrap(); + let output = row.output.as_ref().unwrap()["text"].as_str().unwrap(); + let mut lines = output.lines(); + assert_eq!( + std::fs::canonicalize(lines.next().unwrap()).unwrap(), + worktree.path().canonicalize().unwrap() + ); + assert_eq!(lines.next(), Some("worktree-fixture")); + let detail = harness.handle.load_session(task.clone()).await.unwrap(); + assert_eq!( + Path::new(&detail.session.project_root) + .canonicalize() + .unwrap(), + worktree.path().canonicalize().unwrap() + ); + } + harness.handle.stop().await.unwrap(); +} + +#[tokio::test] +async fn quiet_shell_reports_effective_folder_deadline_and_idle_time_until_stop() { + let harness = Harness::new().await; + let worktree = tempfile::tempdir().unwrap(); + harness.faux.push_message(vec![faux_tool_call( + "bash", + json!({ + "command": "printf fixture-output; sleep 30 # fixture-private-command", + }), + )]); + let task = harness + .handle + .create_session(Some(AgentCreateSessionRequest { + project_root: Some(worktree.path().to_string_lossy().into_owned()), + title: None, + model: None, + context_limit: None, + mcp_server_names: Some(Vec::new()), + })) + .await + .unwrap() + .session + .id; + let mut run = harness.send(&task, "Wait for cancellation").await; + let run_id = run.run_id.clone(); + eventually(|| { + harness.recorder.run_events(&run_id).iter().any(|event| { + let AgentRunEvent::TimelineItem(row) = event else { + return false; + }; + let Some(progress) = row + .output + .as_ref() + .and_then(|output| output.get("structuredContent")) + .and_then(|details| details.get("shellExecution")) + else { + return false; + }; + progress["elapsedSeconds"] + .as_u64() + .is_some_and(|seconds| seconds >= 5) + }) + }) + .await; + let rows = harness.recorder.live_rows(&run_id); + let tool = rows.iter().find(|row| row.item_type == "tool").unwrap(); + assert_eq!(tool.status.as_deref(), Some("running")); + assert_eq!(tool.output.as_ref().unwrap()["text"], "fixture-output"); + let progress = &tool.output.as_ref().unwrap()["structuredContent"]["shellExecution"]; + assert_eq!(progress["cwd"], json!(worktree.path())); + assert_eq!(progress["timeoutSeconds"], 1800.0); + assert!(progress["outputIdleSeconds"].as_u64().unwrap() >= 4); + assert!(!progress.to_string().contains("fixture-private-command")); + let identity = tool.live.clone(); + assert!(identity.is_some()); + harness + .handle + .cancel_desktop_run(run_id.clone()) + .await + .unwrap(); + assert_eq!(finished(&mut run).await, AgentRunTerminal::Cancelled); + // Partial results replace the original tool occurrence and never settle it + // early or create an extra row. The final cancellation removes live status. + let rows = harness.recorder.live_rows(&run_id); + let tools: Vec<_> = rows.iter().filter(|row| row.item_type == "tool").collect(); + assert_eq!(tools.len(), 1); + assert_eq!(tools[0].live, identity); + assert_ne!(tools[0].status.as_deref(), Some("running")); + assert!( + tools[0].output.as_ref().unwrap()["structuredContent"] + .get("shellExecution") + .is_none() + ); + harness.handle.stop().await.unwrap(); +} diff --git a/apps/maple-agent/crates/maple-agent/src/agent/tests.rs b/apps/maple-agent/crates/maple-agent/src/agent/tests.rs index 344667d07..c4dddd751 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/tests.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/tests.rs @@ -16,6 +16,9 @@ use crate::maple_api::test_maple_api_session; #[path = "image_persistence_tests.rs"] mod image_persistence_tests; +#[path = "shell_tests.rs"] +mod shell_tests; + const USER: &str = "slice@example.com"; #[derive(Default)] diff --git a/apps/maple-agent/crates/maple-agent/src/agent/timeline.rs b/apps/maple-agent/crates/maple-agent/src/agent/timeline.rs index de6aa273b..93787e7f8 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/timeline.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/timeline.rs @@ -608,17 +608,37 @@ impl LiveTimeline { merge: "replace".to_string(), }] } + AgentEvent::ToolExecutionUpdate { + tool_call_id, + partial_result, + .. + } => { + let Some(id) = self.tool_rows.get(tool_call_id) else { + return Vec::new(); + }; + let mut item = tool_result_item( + id.clone(), + &partial_result.content, + partial_result.details.as_ref(), + false, + pi_ai::now_ms(), + ); + item.status = Some("running".into()); + vec![item] + } AgentEvent::ToolExecutionEnd { tool_call_id, result, is_error, .. } => { - let Some(id) = self.tool_rows.get(tool_call_id) else { + // Retire the occurrence so a delayed partial result cannot + // overwrite its final status or output while other tools run. + let Some(id) = self.tool_rows.remove(tool_call_id) else { return Vec::new(); }; vec![tool_result_item( - id.clone(), + id, &result.content, result.details.as_ref(), *is_error, diff --git a/apps/maple-agent/crates/maple-agent/src/agent/timeline/tests.rs b/apps/maple-agent/crates/maple-agent/src/agent/timeline/tests.rs index 5973b6614..fbd106fbf 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/timeline/tests.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/timeline/tests.rs @@ -87,6 +87,60 @@ fn shown(items: &[AgentTimelineItem]) -> Vec { .collect() } +#[test] +fn late_tool_progress_cannot_replace_a_final_result_while_another_tool_runs() { + for (text, is_error) in [ + ("finished", false), + ("Command aborted", true), + ("Command timed out after 30 seconds", true), + ] { + let mut live = LiveTimeline::default(); + let mut reply = AssistantMessage::empty(&FauxProvider::new().model()); + reply.content = vec![ + faux_tool_call("bash", json!({"command": "first"})), + faux_tool_call("bash", json!({"command": "second"})), + ]; + let calls: Vec<_> = reply.tool_calls().cloned().collect(); + let mut rows = live.rows(&AgentEvent::MessageEnd { + message: SessionMessage::Llm(Message::Assistant(reply)), + }); + let update = |call: &ToolCall| AgentEvent::ToolExecutionUpdate { + tool_call_id: call.id.clone(), + tool_name: call.name.clone(), + args: Value::Object(call.arguments.clone()), + partial_result: AgentToolResult::text("still running"), + }; + for call in &calls { + for row in live.rows(&AgentEvent::ToolExecutionStart { + tool_call_id: call.id.clone(), + tool_name: call.name.clone(), + args: Value::Object(call.arguments.clone()), + }) { + merge_into(&mut rows, row); + } + assert_eq!(live.rows(&update(call)).len(), 1); + } + for row in live.rows(&AgentEvent::ToolExecutionEnd { + tool_call_id: calls[0].id.clone(), + tool_name: calls[0].name.clone(), + result: AgentToolResult::text(text), + is_error, + }) { + merge_into(&mut rows, row); + } + // An already-running reporter can deliver a partial result after End. + assert!(live.rows(&update(&calls[0])).is_empty()); + let active = live.rows(&update(&calls[1])); + assert_eq!(active.len(), 1); + assert_eq!(active[0].status.as_deref(), Some("running")); + assert_eq!( + rows[0].status.as_deref(), + Some(if is_error { "failed" } else { "completed" }) + ); + assert_eq!(rows[0].output.as_ref().unwrap()["text"], text); + } +} + #[tokio::test] async fn live_rows_are_the_rows_a_reload_shows() { let faux = FauxProvider::new(); diff --git a/apps/maple-agent/crates/maple-agent/src/agent/tools/mod.rs b/apps/maple-agent/crates/maple-agent/src/agent/tools/mod.rs index 445fbf239..60c8c4146 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/tools/mod.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/tools/mod.rs @@ -13,6 +13,7 @@ mod desktop; mod read_image; +pub(super) mod shell; pub(crate) mod web; pub(super) use desktop::parse_user_questions; @@ -24,7 +25,8 @@ use std::sync::Arc; use pi_coding_agent::AgentSession; use pi_coding_agent::extensions::{Extension, RegisteredTool}; use pi_coding_agent::tools::{ - BashSpawnContext, BashSpawnHook, PowerShellToolOptions, ToolsOptions, set_env_var, + BashSpawnContext, BashSpawnHook, LocalShellOperations, PowerShellToolOptions, ToolsOptions, + set_env_var, }; use super::questions::QuestionBroker; @@ -67,9 +69,13 @@ pub(crate) fn task_tools(task: TaskToolsFor) -> TaskTools { .map(PathBuf::from); let hook = spawn_hook(task.session_id.clone(), task.login_path); let mut options = ToolsOptions::default(); + options.bash.operations = Some(shell::bounded(LocalShellOperations::bash( + shell_path.clone(), + ))); options.bash.shell_path = shell_path.clone(); options.bash.spawn_hook = Some(hook.clone()); options.powershell = PowerShellToolOptions { + operations: Some(shell::bounded(LocalShellOperations::powershell())), spawn_hook: Some(hook), ..PowerShellToolOptions::default() }; diff --git a/apps/maple-agent/crates/maple-agent/src/agent/tools/shell.rs b/apps/maple-agent/crates/maple-agent/src/agent/tools/shell.rs new file mode 100644 index 000000000..3f7026034 --- /dev/null +++ b/apps/maple-agent/crates/maple-agent/src/agent/tools/shell.rs @@ -0,0 +1,90 @@ +//! Maple's execution policy around Pi's unchanged shell operations. + +use std::path::Path; +use std::sync::Arc; +use std::time::Duration; + +use async_trait::async_trait; +use pi_coding_agent::tools::{BashOperations, ExecError, ExecOptions, LocalShellOperations}; + +pub(crate) const DEFAULT_TIMEOUT_SECONDS: f64 = 30.0 * 60.0; +const CLEANUP_GRACE: Duration = Duration::from_secs(1); + +pub(crate) const GUIDANCE: &str = "Maple shell policy: bash and powershell have a 30-minute default runtime deadline, overriding the generic tool description's no-default behavior. Request a longer timeout explicitly for slow builds. Each command starts in the task's saved folder; cd does not persist between calls. For repository searches, use explicit absolute paths to the intended checkout/worktree. Verify the folder and never guess ../../ from a temporary folder. Avoid recursive searches of filesystem roots, /proc, /sys and /dev. head limits output, not runtime: request a short timeout (normally 30 seconds) for searches and narrow the scope if it expires."; + +pub(super) fn bounded(inner: LocalShellOperations) -> Arc { + Arc::new(BoundedShell { + inner: Arc::new(inner), + default_timeout: DEFAULT_TIMEOUT_SECONDS, + }) +} + +struct BoundedShell { + inner: Arc, + default_timeout: f64, +} + +pub(crate) fn timeout_seconds(requested: Option) -> f64 { + requested.unwrap_or(DEFAULT_TIMEOUT_SECONDS) +} + +#[async_trait] +impl BashOperations for BoundedShell { + async fn exec( + &self, + command: &str, + cwd: &Path, + mut options: ExecOptions, + ) -> Result, ExecError> { + let seconds = options.timeout.unwrap_or(self.default_timeout); + let duration = Duration::try_from_secs_f64(seconds) + .ok() + .filter(|duration| !duration.is_zero()) + .ok_or_else(|| { + ExecError::Failed( + "Invalid timeout: must be a positive finite number of seconds".into(), + ) + })?; + let cancel = options.cancel.clone(); + if cancel.is_cancelled() { + return Err(ExecError::Aborted); + } + let execution_cancel = cancel.child_token(); + options.cancel = execution_cancel.clone(); + options.timeout = Some(seconds); + // Bound setup and command delivery as well as process waiting. In + // particular, Pi's legacy stdin transport can block before its timer. + let execution = async { + if !cwd.is_absolute() { + return Err(ExecError::Failed( + "Working directory must be an absolute path".into(), + )); + } + if !tokio::fs::metadata(cwd) + .await + .is_ok_and(|metadata| metadata.is_dir()) + { + return Err(ExecError::Failed( + "Working directory is unavailable or is not a directory".into(), + )); + } + self.inner.exec(command, cwd, options).await + }; + tokio::pin!(execution); + let reason = tokio::select! { + biased; + _ = cancel.cancelled() => ExecError::Aborted, + result = &mut execution => return result, + _ = tokio::time::sleep(duration) => ExecError::TimedOut(seconds), + }; + // Let Pi reap the shell and clean up remaining pipe writers normally. + // If setup/stdin cannot respond, dropping the future invokes Pi's own + // process-tree guard. Never wait indefinitely for that cooperation. + execution_cancel.cancel(); + let _ = tokio::time::timeout(CLEANUP_GRACE, &mut execution).await; + Err(reason) + } +} + +#[cfg(test)] +mod tests; diff --git a/apps/maple-agent/crates/maple-agent/src/agent/tools/shell/tests.rs b/apps/maple-agent/crates/maple-agent/src/agent/tools/shell/tests.rs new file mode 100644 index 000000000..0957e5d01 --- /dev/null +++ b/apps/maple-agent/crates/maple-agent/src/agent/tools/shell/tests.rs @@ -0,0 +1,270 @@ +use super::*; +use std::fs; +use std::sync::atomic::{AtomicBool, Ordering}; +use tokio_util::sync::CancellationToken; + +fn options() -> ExecOptions { + ExecOptions { + on_data: Arc::new(|_| {}), + cancel: CancellationToken::new(), + timeout: None, + env: None, + } +} + +#[tokio::test] +async fn unavailable_or_relative_folders_fail_before_execution() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("file"), "fixture").unwrap(); + let shell = bounded(LocalShellOperations::bash(None)); + for cwd in [ + dir.path().join("missing"), + dir.path().join("file"), + "../../".into(), + ] { + assert!(matches!( + shell.exec("must-not-execute", &cwd, options()).await, + Err(ExecError::Failed(_)) + )); + } +} + +struct InspectTimeout; +#[async_trait] +impl BashOperations for InspectTimeout { + async fn exec( + &self, + _: &str, + _: &Path, + options: ExecOptions, + ) -> Result, ExecError> { + assert_eq!(options.timeout, Some(DEFAULT_TIMEOUT_SECONDS)); + Ok(Some(0)) + } +} + +#[tokio::test] +async fn maple_supplies_a_default_deadline_to_the_existing_operations_interface() { + let dir = tempfile::tempdir().unwrap(); + let shell = BoundedShell { + inner: Arc::new(InspectTimeout), + default_timeout: DEFAULT_TIMEOUT_SECONDS, + }; + assert_eq!( + shell.exec("fixture", dir.path(), options()).await.unwrap(), + Some(0) + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn explicit_deadlines_override_the_default_and_calls_keep_the_saved_folder() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("window.rs"), "fixture").unwrap(); + let shell = BoundedShell { + inner: Arc::new(LocalShellOperations::bash(None)), + default_timeout: 0.05, + }; + assert_eq!( + shell.exec("sleep 30", dir.path(), options()).await, + Err(ExecError::TimedOut(0.05)) + ); + assert_eq!( + shell + .exec( + "sleep 0.1", + dir.path(), + ExecOptions { + timeout: Some(5.0), + ..options() + } + ) + .await + .unwrap(), + Some(0) + ); + shell.exec("cd ..", dir.path(), options()).await.unwrap(); + assert_eq!( + shell + .exec("test -f ./window.rs", dir.path(), options()) + .await + .unwrap(), + Some(0) + ); +} + +struct BlockedDelivery(Arc, Arc); +struct Dropped(Arc); +impl Drop for Dropped { + fn drop(&mut self) { + self.0.store(true, Ordering::SeqCst); + } +} +#[async_trait] +impl BashOperations for BlockedDelivery { + async fn exec(&self, _: &str, _: &Path, _: ExecOptions) -> Result, ExecError> { + let _dropped = Dropped(self.0.clone()); + self.1.notify_one(); + // An operation stuck before its own cancellation/deadline select, as + // with command delivery to a shell that does not read its stdin. + std::future::pending().await + } +} + +#[tokio::test] +async fn uncooperative_command_delivery_is_bounded_and_dropped() { + let dir = tempfile::tempdir().unwrap(); + let dropped = Arc::new(AtomicBool::new(false)); + let shell = BoundedShell { + inner: Arc::new(BlockedDelivery(dropped.clone(), Arc::default())), + default_timeout: 0.01, + }; + let parent_cancel = CancellationToken::new(); + let result = tokio::time::timeout( + Duration::from_secs(5), + shell.exec( + "fixture", + dir.path(), + ExecOptions { + cancel: parent_cancel.clone(), + ..options() + }, + ), + ) + .await + .unwrap(); + assert_eq!(result, Err(ExecError::TimedOut(0.01))); + assert!(dropped.load(Ordering::SeqCst)); + assert!( + !parent_cancel.is_cancelled(), + "a command deadline must not cancel its caller" + ); +} + +#[tokio::test] +async fn uncooperative_command_delivery_is_cancelled_and_dropped() { + let dir = tempfile::tempdir().unwrap(); + let dropped = Arc::new(AtomicBool::new(false)); + let entered = Arc::new(tokio::sync::Notify::new()); + let shell = BoundedShell { + inner: Arc::new(BlockedDelivery(dropped.clone(), entered.clone())), + default_timeout: DEFAULT_TIMEOUT_SECONDS, + }; + let cancel = CancellationToken::new(); + let (result, ()) = tokio::time::timeout(Duration::from_secs(5), async { + tokio::join!( + shell.exec( + "fixture", + dir.path(), + ExecOptions { + cancel: cancel.clone(), + ..options() + } + ), + async { + entered.notified().await; + cancel.cancel(); + } + ) + }) + .await + .unwrap(); + assert_eq!(result, Err(ExecError::Aborted)); + assert!(dropped.load(Ordering::SeqCst)); +} + +/// A finite, disposable substitute for a kernel file that waits indefinitely for +/// new data. The held writer prevents EOF; neither side of the pipeline produces +/// output, so head cannot bound its runtime. +#[cfg(unix)] +#[tokio::test] +async fn blocked_pipeline_is_stopped_by_cancel_deadline_and_dropped_future() { + use std::ffi::CString; + use std::os::unix::fs::OpenOptionsExt; + + for end in ["cancel", "deadline", "drop"] { + let dir = tempfile::tempdir().unwrap(); + fs::create_dir_all(dir.path().join("scratch/tmp")).unwrap(); + let fifo = CString::new(dir.path().join("blocked").as_os_str().as_encoded_bytes()).unwrap(); + // SAFETY: a valid, NUL-terminated path inside a disposable test directory. + assert_eq!(unsafe { libc::mkfifo(fifo.as_ptr(), 0o600) }, 0); + let _reader = fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NONBLOCK) + .open(dir.path().join("blocked")) + .unwrap(); + let _writer = fs::OpenOptions::new() + .write(true) + .custom_flags(libc::O_NONBLOCK) + .open(dir.path().join("blocked")) + .unwrap(); + let tool = Arc::new(BoundedShell { + inner: Arc::new(LocalShellOperations::bash(None)), + default_timeout: DEFAULT_TIMEOUT_SECONDS, + }); + let cwd = dir.path().to_path_buf(); + let cancel = CancellationToken::new(); + let call = tokio::spawn({ + let (tool, cancel) = (tool.clone(), cancel.clone()); + async move { + tool.exec( + "printf '%s' $$ > shell.pid; cd scratch/tmp && sh -c 'printf %s $$ > ../../reader.pid; exec cat ../../blocked' | sh -c 'printf %s $$ > ../../head.pid; exec head -c 1'", + &cwd, + ExecOptions { timeout: Some(if end == "deadline" { 0.5 } else { 30.0 }), cancel, ..options() }, + ).await + } + }); + let mut pids = Vec::new(); + for name in ["shell.pid", "reader.pid", "head.pid"] { + let pid = tokio::time::timeout(Duration::from_secs(5), async { + loop { + if let Some(pid) = fs::read_to_string(dir.path().join(name)) + .ok() + .and_then(|text| text.parse::().ok()) + { + break pid; + } + tokio::time::sleep(Duration::from_millis(5)).await; + } + }) + .await + .expect("pipeline member records its pid"); + pids.push(pid); + } + match end { + "cancel" => cancel.cancel(), + "drop" => call.abort(), + _ => {} + } + let result = tokio::time::timeout(Duration::from_secs(5), call) + .await + .expect("blocked call ends"); + match end { + "drop" => assert!(result.unwrap_err().is_cancelled()), + "cancel" => assert_eq!(result.unwrap().unwrap_err(), ExecError::Aborted), + _ => assert_eq!(result.unwrap().unwrap_err(), ExecError::TimedOut(0.5)), + } + // Check every member, not just the shell. Linux PID 1 need not reap an + // orphan immediately, so a zombie counts as terminated, never as alive. + for pid in pids { + tokio::time::timeout(Duration::from_secs(5), async { + loop { + // SAFETY: signal zero only checks existence. + if unsafe { libc::kill(pid, 0) } != 0 { + break; + } + #[cfg(target_os = "linux")] + if fs::read_to_string(format!("/proc/{pid}/stat")).is_ok_and(|stat| { + stat.rsplit_once(") ") + .is_some_and(|(_, rest)| rest.starts_with("Z ")) + }) { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("pipeline member is terminated"); + } + } +} diff --git a/apps/maple-agent/crates/maple-agent/src/agent/tools/tests.rs b/apps/maple-agent/crates/maple-agent/src/agent/tools/tests.rs index 8f0a29a96..0434c3f47 100644 --- a/apps/maple-agent/crates/maple-agent/src/agent/tools/tests.rs +++ b/apps/maple-agent/crates/maple-agent/src/agent/tools/tests.rs @@ -128,6 +128,8 @@ fn the_model_gets_read_a_shell_edit_and_write() { }; assert_eq!(tools.builtin, ["read", shell, "edit", "write"]); assert!(tools.options.powershell.spawn_hook.is_some()); + assert!(tools.options.bash.operations.is_some()); + assert!(tools.options.powershell.operations.is_some()); } fn tool(tools: &TaskTools, name: &str) -> Arc {