From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: lucascouts Date: Mon, 12 Oct 2026 08:43:30 -0300 Subject: [PATCH] agent: show a task's output, open its log, and keep the log with its record The tasks card of 0037 said that a background shell was running and for how long, never what it was printing. The output lived only in the agent's /tmp/claude-/.../tasks/.output, on a tmpfs, so a reboot took it; the Agent Tasks history of 0035 remembered that the task existed, not what it printed. Story 041. What it adds, on top of the adapter's _claude/tasks contract version 2 (@lucascouts/claude-agent-acp-plus >= 0.26.0): - A card row expands (keyboard-reachable "Expand task"/"Collapse task"): what the task runs -- its tool call's title, else its description -- its progress details, and for a shell task the last lines of output in a monospace block, or "No output yet". A watched row is expanded; every row starts collapsed after a restart. - "Open Log" on a shell row fetches the output, saves it as the task's log and opens it read-only in an editor tab; "Open Thread" on a subagent row shows the subagent's own thread when its parent's conversation has it loaded, else opens the parent at the tool call through 0041's path. A failure reads "Could not open the log: ". - When a shell task ends its output is copied into Zeo's data directory and belongs to the history record: the panel's rows offer "Open Log" ("Log no longer available" when the file is gone), and Delete, Delete All Archived and 0053's Delete Selected delete the log with the row. Loading the history sweeps every log no record owns. - Open Log is refused in a remote project, card and panel alike, with task_log::REMOTE_PROJECT_NOTICE ("Open Log is not available in remote projects"): a log is a local file, and a remote path would resolve on the other host. Contract (_claude/tasks v2, advertised as agentCapabilities._meta ["_claude/tasks"] = { "version": 2 }; every v1 field and method kept): - snapshot keys Zeo reads -- required: id, type, description, background, depth, status, startedAt; nullable (absent parses as None): toolCallId, endedAt, usage, lastTool, summary; defaulted: hasOutput (false), tail (none). A tail that is not a list of strings keeps the previous task list and logs a warning naming the session. - _claude/tasks/output { sessionId, taskId, maxBytes: 1048576 } answers { content, totalBytes, truncated }. An agent advertising a version below 2 gets no request: Zeo answers "unsupported" itself. - The fixtures claude_tasks_snapshot_v2.json and claude_tasks_output.json are byte-identical to the adapter's claude-tasks-snapshot.json and claude-tasks-output.json; status.sh step 6 checks both pairs. File-mode rules (task_log.rs): logs live at /agent_task_logs, scoped by release channel, as /.log, each id mapped to a separator-free file-name component. The directories are made 0700 and tightened to it when they already exist; a log file is 0600. A save writes a temporary file in the log's own directory, sync_all()s it and renames it over the previous log, so an interrupted save leaves the old log whole -- never via /tmp, another filesystem. A log keeps at most the last 1 MiB, cut at a line boundary under "[earlier output truncated: N bytes]". Log lines (agent_task_log event=saved|deleted|swept|failed) carry ids, sizes, modes and outcomes, never output text. The cut searches bytes for that line boundary, so a cap that lands inside a multibyte character advances to a char boundary, never panics. Migration: HISTORY_MIGRATIONS step 4 (the next free step; 0053-0055 add none), ALTER TABLE agent_task_history ADD COLUMN log_bytes INTEGER; NULL means no log, so a database written before this patch keeps every record, each without a log. Steps 1-3 are unchanged (append-only). No new dependency; Cargo.lock is unchanged. Rides claude-agent-acp-plus BY DEPENDENCY after 0053, the last task patch: it extends 0037's card, 0035's panel and store, 0041's open-at-task path and 0042's delete path. a4b39c38 review: (2026-11-01, story 072) Checked against upstream #66255/#66101/#66102. This patch adds no arm to AcpThread::handle_session_update beyond 0034's, and it does not touch the SessionMessage arm or subagent_association. The output and log surfaces read the same task entries as at a5c4ca8b. Story 064 (2026-11-02): `_claude/tasks` contract version 3 -- an entry may name the ACP session of the subagent it describes (`childSessionId`), parsed into AgentTask::child_session_id. It is read as a raw value so a malformed one is dropped on its own entry with a debug log, never failing the snapshot the way a wrong-typed `tail` does; absent in versions 1 and 2. It sits beside this patch's version 2 fields, which is why it is here and not in 0034. 3 tests (acp_thread_tasks_child_session_tests.rs). --- diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index e104f7b..f25720a 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -3998,6 +3998,23 @@ pub struct AgentTask { pub usage: Option, pub last_tool: Option, pub summary: Option, + /// `_claude/tasks` version 2: the agent can serve this task's output + /// (`AcpThread::fetch_task_output`). Always `false` from a version 1 agent. + pub has_output: bool, + /// `_claude/tasks` version 2: a running shell's last lines, sanitized by the + /// agent. `None` for every other task, and from a version 1 agent. + pub tail: Option>, + /// `_claude/tasks` version 3: the ACP session of the subagent this entry describes, so + /// the task surfaces show one row per child whichever source names it (story 064). + pub child_session_id: Option, +} + +/// The end of a task's output, as `_claude/tasks/output` answers it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TaskOutput { + pub content: String, + pub total_bytes: u64, + pub truncated: bool, } #[derive(Deserialize)] @@ -4021,6 +4038,16 @@ struct WireTask { usage: Option, last_tool: Option, summary: Option, + // Version 2. `default` is what lets a version 1 snapshot, which has neither, + // still parse; a wrong-typed value still fails the whole snapshot. + #[serde(default)] + has_output: bool, + #[serde(default)] + tail: Option>, + // Version 3. Read as a raw value so a malformed one is dropped on its own entry instead + // of failing the whole snapshot, which is what a wrong-typed `tail` does. + #[serde(default)] + child_session_id: Option, } #[derive(Deserialize)] @@ -4050,7 +4077,20 @@ fn system_time_from_millis(millis: u64) -> SystemTime { } impl From for AgentTask { - fn from(task: WireTask) -> Self { + fn from(mut task: WireTask) -> Self { + let child_session_id = task.child_session_id.take().and_then(|value| match value { + serde_json::Value::String(child) if !child.is_empty() => { + Some(acp_v2::SessionId::new(child)) + } + serde_json::Value::Null => None, + other => { + log::debug!( + "ignoring malformed childSessionId on task {}: {other}", + task.id + ); + None + } + }); AgentTask { id: task.id.into(), tool_call_id: task.tool_call_id.map(acp_v2::ToolCallId::new), @@ -4074,6 +4114,11 @@ impl From for AgentTask { }), last_tool: task.last_tool.map(SharedString::from), summary: task.summary.map(SharedString::from), + has_output: task.has_output, + tail: task + .tail + .map(|lines| lines.into_iter().map(SharedString::from).collect()), + child_session_id, } } } @@ -5423,6 +5468,17 @@ impl AcpThread { .stop_task(self.session_id(), task_id, cx) } + /// The end of a task's output, read from the agent (`_claude/tasks` version 2). + pub fn fetch_task_output( + &self, + task_id: &AgentTaskId, + cx: &mut App, + ) -> Task> { + self.connection + .clone() + .fetch_task_output(self.session_id(), task_id, cx) + } + /// Resolves to `false` when the agent had nothing to move to the background. pub fn background_task(&self, tool_call_id: &str, cx: &mut App) -> Task> { self.connection @@ -5694,10 +5750,14 @@ impl AcpThread { { match serde_json::from_value::(payload.clone()) { Ok(snapshot) => self.apply_tasks_snapshot(snapshot, cx), + // Category and position only: serde's message quotes the + // offending value, which for `tail` is the shell's output. Err(err) => log::warn!( - "malformed _claude/tasks for session {}: {}", + "malformed _claude/tasks for session {}: {:?} error at line {} column {}", self.session_id(), - err + err.classify(), + err.line(), + err.column() ), } } @@ -27949,4 +28009,7 @@ mod tests { include!("acp_thread_tasks_tests.rs"); include!("acp_thread_tasks_review_tests.rs"); + include!("acp_thread_tasks_v2_tests.rs"); + include!("acp_thread_tasks_contract_v2_tests.rs"); + include!("acp_thread_tasks_child_session_tests.rs"); } diff --git a/crates/acp_thread/src/acp_thread_tasks_child_session_tests.rs b/crates/acp_thread/src/acp_thread_tasks_child_session_tests.rs new file mode 100644 index 0000000..8e098f2 --- /dev/null +++ b/crates/acp_thread/src/acp_thread_tasks_child_session_tests.rs @@ -0,0 +1,153 @@ +// Story 064, task 2.1 (feed half) — `childSessionId` on a `_claude/tasks` entry (contract v3, story +// 065): parsed into `AgentTask::child_session_id` when present, absent from v2 and earlier, and a +// malformed value is ignored, never an error (R3.1, R3.2). +// +// Included at the end of `mod tests` in `acp_thread.rs`, AFTER the contract file: +// +// include!("acp_thread_tasks_v2_tests.rs"); +// include!("acp_thread_tasks_contract_v2_tests.rs"); +// include!("acp_thread_tasks_child_session_tests.rs"); +// +// so it shares story 012's helpers (`tasks_thread`, `tasks_send`, `tasks_snapshot`, +// `tasks_wire_task`, `tasks_ids`) and `FakeAgentConnection`. Every name carries +// `tasks_child_session` (filter `cargo test -p acp_thread tasks_child_session`). +// +// Contract (design.md §Data Models): `AgentTask` gains `child_session_id: Option` +// (`acp_v2::SessionId`), parsed from the feed's `childSessionId` when present. Task 2.1's +// objective: a malformed `childSessionId` is ignored, never an error. +// Assumed beyond design.md: an empty string names no session and reads `None`, as an empty +// `toolCallId` names no entry (`TaskRecord::new`). +// +// Hostile halves first: a malformed `childSessionId` must neither reject the snapshot (as a +// malformed `tail` does) nor invent a child; two entries naming two children keep each its own; +// then the benign v3 entry and the v2 entry without the key. + +fn tasks_child_session_entry(id: &str, child: serde_json::Value) -> serde_json::Value { + let mut task = tasks_wire_task(id, "running", 1_000); + task["type"] = serde_json::json!("subagent"); + task["childSessionId"] = child; + task +} + +fn tasks_child_session_of( + thread: &Entity, + id: &str, + cx: &mut TestAppContext, +) -> Option { + thread.read_with(cx, |thread, _| { + thread + .tasks() + .iter() + .find(|task| task.id.as_ref() == id) + .unwrap_or_else(|| panic!("task {id} is listed")) + .child_session_id + .as_ref() + .map(|child| child.to_string()) + }) +} + +// A malformed value is ignored on its own entry: the snapshot is accepted whole, the entry is +// listed with no child, and the well-formed entry beside it keeps its child. +#[gpui::test] +async fn test_tasks_child_session_malformed_value_is_ignored_not_an_error(cx: &mut TestAppContext) { + for bad in [ + serde_json::json!(42), + serde_json::json!(true), + serde_json::json!({ "id": "c-1" }), + serde_json::json!(["c-1"]), + serde_json::json!(""), + ] { + let (thread, events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + tasks_send( + &thread, + tasks_snapshot(vec![ + tasks_child_session_entry("bad", bad.clone()), + tasks_child_session_entry("good", serde_json::json!("c-good")), + ]), + cx, + ); + assert_eq!( + tasks_ids(&thread, cx), + vec![ + ("bad".to_string(), "running"), + ("good".to_string(), "running") + ], + "a malformed childSessionId never rejects the snapshot: {bad}" + ); + assert_eq!( + events.borrow().updated, + 1, + "the snapshot is applied and notified once: {bad}" + ); + assert_eq!( + tasks_child_session_of(&thread, "bad", cx), + None, + "a malformed childSessionId names no child: {bad}" + ); + assert_eq!( + tasks_child_session_of(&thread, "good", cx), + Some("c-good".to_string()), + "the entry beside it keeps its child: {bad}" + ); + } +} + +// Identity is the child's id, not the entry's shape: two entries with the same description name +// two children, and an entry whose own id equals another child's id names only its own child. +#[gpui::test] +async fn test_tasks_child_session_each_entry_keeps_its_own_child(cx: &mut TestAppContext) { + let (thread, _events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + let mut first = tasks_child_session_entry("t1", serde_json::json!("c-a")); + let mut second = tasks_child_session_entry("c-a", serde_json::json!("c-b")); + first["description"] = serde_json::json!("Explore"); + second["description"] = serde_json::json!("Explore"); + tasks_send(&thread, tasks_snapshot(vec![first, second]), cx); + + assert_eq!( + tasks_child_session_of(&thread, "t1", cx), + Some("c-a".to_string()) + ); + assert_eq!( + tasks_child_session_of(&thread, "c-a", cx), + Some("c-b".to_string()), + "an entry id that looks like a child id is still only an entry id" + ); +} + +// Benign: a v3 entry carries its child; a v2 entry (no key) and `null` carry none, and the rest of +// the entry parses as before. +#[gpui::test] +async fn test_tasks_child_session_v3_value_parses_and_v2_entry_has_none(cx: &mut TestAppContext) { + let (thread, _events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + let v2_entry = tasks_wire_task("v2", "completed", 2_000); + assert!( + v2_entry.get("childSessionId").is_none(), + "precondition: a version 2 entry has no childSessionId key" + ); + tasks_send( + &thread, + tasks_snapshot(vec![ + tasks_child_session_entry("v3", serde_json::json!("c-native")), + tasks_child_session_entry("null", serde_json::json!(null)), + v2_entry, + ]), + cx, + ); + + assert_eq!( + tasks_child_session_of(&thread, "v3", cx), + Some("c-native".to_string()) + ); + assert_eq!(tasks_child_session_of(&thread, "null", cx), None); + assert_eq!(tasks_child_session_of(&thread, "v2", cx), None); + thread.read_with(cx, |thread, _| { + let v3 = thread + .tasks() + .iter() + .find(|task| task.id.as_ref() == "v3") + .expect("v3 is listed"); + assert_eq!(v3.kind.as_ref(), "subagent"); + assert_eq!(v3.description.as_ref(), "Task v3"); + assert_eq!(v3.status, AgentTaskStatus::Running); + }); +} diff --git a/crates/acp_thread/src/acp_thread_tasks_contract_v2_tests.rs b/crates/acp_thread/src/acp_thread_tasks_contract_v2_tests.rs new file mode 100644 index 0000000..b4d0a51 --- /dev/null +++ b/crates/acp_thread/src/acp_thread_tasks_contract_v2_tests.rs @@ -0,0 +1,114 @@ +// Story 041, task 8.1 — the data-contract check between the adapter's fixtures and Zeo's wire +// structs (R7.5, R3.1, R3.4; Q12). `scripts/status.sh` step 6 proves the two copies byte-identical; +// this file proves the copy Zeo holds exercises every field Zeo reads, so a fixture that silently +// lost a field (and still parsed, because the field defaults) cannot pass for a contract. +// +// Included at the end of `mod tests` in `acp_thread.rs`, after the version 2 file: +// +// include!("acp_thread_tasks_v2_tests.rs"); +// include!("acp_thread_tasks_contract_v2_tests.rs"); +// +// so the private `WireTasksSnapshot` is reachable. Names carry `tasks_contract` (filter +// `cargo test -p acp_thread tasks_contract`). +// +// The key lists below are what `WireTask` (camelCase) and the `_claude/tasks/output` parser read; +// task 8.1 re-derives them from the code and records them in its commit body. If a field is added +// to either reader, add it here, and to both fixtures. +// +// Hostile halves first: removing any REQUIRED key from a fixture task must make the snapshot fail +// to parse (the fixture really carries it); the two DEFAULTED keys (`hasOutput`, `tail`) parse when +// absent, so their presence in the version 2 fixture is asserted explicitly instead. + +const TASKS_CONTRACT_SNAPSHOT: &str = include_str!("test_data/claude_tasks_snapshot_v2.json"); +const TASKS_CONTRACT_OUTPUT: &str = include_str!("test_data/claude_tasks_output.json"); + +// Re-derived from `WireTask` by task 8.1 (Run mode, 2026-10-12): the non-`Option` fields are +// required; the five `Option` fields parse as `None` when absent (serde's rule for `Option`), so +// they are nullable, not required; `hasOutput` and `tail` carry `#[serde(default)]`. +const TASKS_CONTRACT_REQUIRED: &[&str] = &[ + "id", + "type", + "description", + "background", + "depth", + "status", + "startedAt", +]; +const TASKS_CONTRACT_NULLABLE: &[&str] = &["toolCallId", "endedAt", "usage", "lastTool", "summary"]; +const TASKS_CONTRACT_DEFAULTED: &[&str] = &["hasOutput", "tail"]; +const TASKS_CONTRACT_OUTPUT_KEYS: &[&str] = &["content", "totalBytes", "truncated"]; + +fn tasks_contract_doc() -> serde_json::Value { + serde_json::from_str(TASKS_CONTRACT_SNAPSHOT).expect("the snapshot fixture is JSON") +} + +#[test] +fn tasks_contract_every_fixture_task_carries_every_key_zeo_reads() { + let doc = tasks_contract_doc(); + for task in doc["tasks"].as_array().expect("tasks is a list") { + let object = task.as_object().expect("a task is an object"); + for key in TASKS_CONTRACT_REQUIRED + .iter() + .chain(TASKS_CONTRACT_NULLABLE) + .chain(TASKS_CONTRACT_DEFAULTED) + { + assert!(object.contains_key(*key), "{} lacks {key}", task["id"]); + } + let mut keys: Vec<&str> = object.keys().map(String::as_str).collect(); + keys.sort_unstable(); + let mut expected: Vec<&str> = TASKS_CONTRACT_REQUIRED + .iter() + .chain(TASKS_CONTRACT_NULLABLE) + .chain(TASKS_CONTRACT_DEFAULTED) + .copied() + .collect(); + expected.sort_unstable(); + assert_eq!(keys, expected, "no key Zeo does not read"); + } +} + +// Hostile: each required key, removed from one task, breaks the parse. +#[test] +fn tasks_contract_removing_a_required_key_breaks_the_parse() { + assert!(serde_json::from_value::(tasks_contract_doc()).is_ok()); + for key in TASKS_CONTRACT_REQUIRED { + let mut doc = tasks_contract_doc(); + doc["tasks"][0].as_object_mut().unwrap().remove(*key); + assert!( + serde_json::from_value::(doc).is_err(), + "the parse must need {key}" + ); + } + // Converse: the nullable and defaulted keys are optional on the wire (version 1 + // compatibility, R3.2); their presence in the fixture is asserted by the test above. + for key in TASKS_CONTRACT_NULLABLE + .iter() + .chain(TASKS_CONTRACT_DEFAULTED) + { + let mut doc = tasks_contract_doc(); + doc["tasks"][0].as_object_mut().unwrap().remove(*key); + assert!( + serde_json::from_value::(doc).is_ok(), + "{key} defaults" + ); + } +} + +#[test] +fn tasks_contract_output_fixture_has_exactly_the_keys_the_parser_reads() { + let output: serde_json::Value = + serde_json::from_str(TASKS_CONTRACT_OUTPUT).expect("the output fixture is JSON"); + let mut keys: Vec<&str> = output + .as_object() + .expect("an object") + .keys() + .map(String::as_str) + .collect(); + keys.sort_unstable(); + let mut expected = TASKS_CONTRACT_OUTPUT_KEYS.to_vec(); + expected.sort_unstable(); + assert_eq!(keys, expected); + assert!(output["content"].is_string()); + assert!(output["totalBytes"].is_u64()); + assert!(output["truncated"].is_boolean()); +} diff --git a/crates/acp_thread/src/acp_thread_tasks_v2_tests.rs b/crates/acp_thread/src/acp_thread_tasks_v2_tests.rs new file mode 100644 index 0000000..bc94905 --- /dev/null +++ b/crates/acp_thread/src/acp_thread_tasks_v2_tests.rs @@ -0,0 +1,280 @@ +// Story 041, task 4.2 — `_claude/tasks` version 2 in `AcpThread`: `tail` and `has_output` on +// `AgentTask`, the version 2 and version 1 fixtures, the malformed tail (R3.1, R3.2, R3.3, R7.5; +// Q12). Also the trait default of `fetch_task_output` (task 4.3's R3.5, which lives in this crate). +// +// Included at the end of `mod tests` in `acp_thread.rs`, AFTER `acp_thread_tasks_tests.rs`: +// +// include!("acp_thread_tasks_tests.rs"); +// include!("acp_thread_tasks_review_tests.rs"); +// include!("acp_thread_tasks_v2_tests.rs"); +// +// so it shares that module's imports, `FakeAgentConnection`, and story 012's helpers +// (`tasks_thread`, `tasks_send`, `tasks_snapshot`, `tasks_update`, `tasks_wire_task`, +// `tasks_ids`). Every test name carries `tasks` (filter `cargo test -p acp_thread tasks`). +// +// Contract (design.md §`AcpThread` task model): `AgentTask::has_output: bool`, +// `AgentTask::tail: Option>`; `AcpThread::fetch_task_output(&self, task_id: +// &AgentTaskId, cx) -> Task>`. Fixtures under `src/test_data/`: +// `claude_tasks_snapshot_v2.json` (version 2, new in this patch, byte-identical to the adapter's +// `claude-tasks-snapshot.json`) and `claude_tasks_snapshot.json` (version 1, as patch 0034 created it). +// +// Hostile halves first: a malformed `tail` or `hasOutput` must keep the previous list (and not +// read as "no tail"); a version 1 snapshot must still parse; a repeated identical tail must not +// notify again while a changed one must. + +const TASKS_V2_FIXTURE: &str = include_str!("test_data/claude_tasks_snapshot_v2.json"); +const TASKS_V1_FIXTURE: &str = include_str!("test_data/claude_tasks_snapshot.json"); + +fn tasks_v2_tail( + thread: &Entity, + id: &str, + cx: &mut TestAppContext, +) -> Option> { + thread.read_with(cx, |thread, _| { + let task = thread + .tasks() + .iter() + .find(|task| task.id.as_ref() == id) + .unwrap_or_else(|| panic!("task {id} is listed")); + task.tail + .as_ref() + .map(|lines| lines.iter().map(|line| line.to_string()).collect()) + }) +} + +fn tasks_v2_has_output(thread: &Entity, id: &str, cx: &mut TestAppContext) -> bool { + thread.read_with(cx, |thread, _| { + thread + .tasks() + .iter() + .find(|task| task.id.as_ref() == id) + .unwrap_or_else(|| panic!("task {id} is listed")) + .has_output + }) +} + +fn tasks_v2_shell( + id: &str, + tail: serde_json::Value, + has_output: serde_json::Value, +) -> serde_json::Value { + let mut task = tasks_wire_task(id, "running", 1_000); + task["tail"] = tail; + task["hasOutput"] = has_output; + task +} + +// ---- R3.3 — hostile first: a malformed field keeps the previous list ---------------------- + +#[gpui::test] +async fn test_tasks_v2_malformed_tail_keeps_the_previous_list(cx: &mut TestAppContext) { + let (thread, events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + tasks_send( + &thread, + tasks_snapshot(vec![tasks_v2_shell( + "b1", + serde_json::json!(["line 1"]), + serde_json::json!(true), + )]), + cx, + ); + assert_eq!(events.borrow().updated, 1); + + for bad in [ + tasks_v2_shell("b1", serde_json::json!("line 2"), serde_json::json!(true)), + tasks_v2_shell("b1", serde_json::json!([1, 2]), serde_json::json!(true)), + tasks_v2_shell( + "b1", + serde_json::json!(["ok", null]), + serde_json::json!(true), + ), + tasks_v2_shell( + "b1", + serde_json::json!({ "0": "x" }), + serde_json::json!(true), + ), + tasks_v2_shell( + "b1", + serde_json::json!(["line 2"]), + serde_json::json!("yes"), + ), + ] { + tasks_send(&thread, tasks_snapshot(vec![bad.clone()]), cx); + assert_eq!( + tasks_v2_tail(&thread, "b1", cx), + Some(vec!["line 1".to_string()]), + "a malformed snapshot is rejected whole: {bad}" + ); + assert!(tasks_v2_has_output(&thread, "b1", cx)); + } + assert_eq!( + events.borrow().updated, + 1, + "a rejected snapshot notifies nobody" + ); +} + +// Converse: `null` and an empty list are well-formed tails. +#[gpui::test] +async fn test_tasks_v2_null_and_empty_tails_are_accepted(cx: &mut TestAppContext) { + let (thread, _events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + tasks_send( + &thread, + tasks_snapshot(vec![tasks_v2_shell( + "b1", + serde_json::json!(null), + serde_json::json!(false), + )]), + cx, + ); + assert_eq!(tasks_v2_tail(&thread, "b1", cx), None); + assert!(!tasks_v2_has_output(&thread, "b1", cx)); + + tasks_send( + &thread, + tasks_snapshot(vec![tasks_v2_shell( + "b1", + serde_json::json!([]), + serde_json::json!(true), + )]), + cx, + ); + assert_eq!(tasks_v2_tail(&thread, "b1", cx), Some(Vec::new())); + assert!(tasks_v2_has_output(&thread, "b1", cx)); +} + +// ---- R3.2 — a version 1 snapshot still parses ----------------------------------------------- + +#[gpui::test] +async fn test_tasks_v2_version_1_snapshot_parses_with_no_tail_and_no_output( + cx: &mut TestAppContext, +) { + let (thread, events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + let payload: serde_json::Value = + serde_json::from_str(TASKS_V1_FIXTURE).expect("the version 1 fixture is JSON"); + for task in payload["tasks"].as_array().expect("tasks is a list") { + assert!( + task.get("hasOutput").is_none() && task.get("tail").is_none(), + "{task}" + ); + } + tasks_send(&thread, tasks_update(payload.clone()), cx); + + let ids: Vec = payload["tasks"] + .as_array() + .unwrap() + .iter() + .map(|task| task["id"].as_str().unwrap().to_string()) + .collect(); + assert_eq!( + tasks_ids(&thread, cx) + .into_iter() + .map(|(id, _)| id) + .collect::>(), + ids, + "every version 1 task is held" + ); + for id in &ids { + assert_eq!(tasks_v2_tail(&thread, id, cx), None); + assert!(!tasks_v2_has_output(&thread, id, cx)); + } + assert_eq!(events.borrow().updated, 1); +} + +// ---- R3.1 — the version 2 fixture and the fields on the model ------------------------------ + +#[gpui::test] +async fn test_tasks_v2_contract_fixture_parses_with_tail_and_output(cx: &mut TestAppContext) { + let (thread, events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + let payload: serde_json::Value = + serde_json::from_str(TASKS_V2_FIXTURE).expect("the version 2 fixture is JSON"); + tasks_send(&thread, tasks_update(payload.clone()), cx); + assert_eq!(events.borrow().updated, 1); + + let mut roles = (0, 0, 0); + for task in payload["tasks"].as_array().expect("tasks is a list") { + let id = task["id"].as_str().unwrap(); + let expected_tail: Option> = task["tail"].as_array().map(|lines| { + lines + .iter() + .map(|line| line.as_str().unwrap().to_string()) + .collect() + }); + assert_eq!(tasks_v2_tail(&thread, id, cx), expected_tail, "{id}"); + assert_eq!( + tasks_v2_has_output(&thread, id, cx), + task["hasOutput"] + .as_bool() + .expect("hasOutput is a boolean in version 2"), + "{id}" + ); + match ( + task["type"].as_str().unwrap(), + task["status"].as_str().unwrap(), + ) { + ("shell", "running") => { + roles.0 += 1; + assert!(expected_tail.is_some_and(|t| !t.is_empty())); + } + ("shell", _) => roles.1 += 1, + ("subagent", _) => { + roles.2 += 1; + assert!(!task["hasOutput"].as_bool().unwrap()); + } + _ => {} + } + } + assert_eq!( + roles, + (1, 1, 1), + "one running shell, one ended shell, one subagent" + ); +} + +// Hostile: the same tail again is no change; a new line is. +#[gpui::test] +async fn test_tasks_v2_a_changed_tail_notifies_and_the_same_tail_does_not(cx: &mut TestAppContext) { + let (thread, events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + let snapshot = |tail: serde_json::Value| { + tasks_snapshot(vec![tasks_v2_shell("b1", tail, serde_json::json!(true))]) + }; + tasks_send(&thread, snapshot(serde_json::json!(["line 1"])), cx); + tasks_send(&thread, snapshot(serde_json::json!(["line 1"])), cx); + assert_eq!( + events.borrow().updated, + 1, + "an identical snapshot is not news" + ); + + tasks_send( + &thread, + snapshot(serde_json::json!(["line 1", "line 2"])), + cx, + ); + assert_eq!(events.borrow().updated, 2, "a tail that grew is news"); + assert_eq!( + tasks_v2_tail(&thread, "b1", cx), + Some(vec!["line 1".to_string(), "line 2".to_string()]) + ); +} + +// ---- R3.5 (task 4.3) — the trait default answers "unsupported" ------------------------------ + +#[gpui::test] +async fn test_tasks_v2_fetch_output_is_unsupported_for_an_agent_without_it( + cx: &mut TestAppContext, +) { + // FakeAgentConnection does not override `fetch_task_output`: the trait default answers. + let (thread, _events) = tasks_thread(Rc::new(FakeAgentConnection::new()), cx).await; + let error = thread + .update(cx, |thread, cx| { + thread.fetch_task_output(&AgentTaskId::from("b1".to_string()), cx) + }) + .await + .expect_err("reading output is unsupported"); + let text = error.to_string().to_lowercase(); + assert!( + text.contains("not supported") || text.contains("unsupported"), + "{text}" + ); +} diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 5b0f0b2..ad80e1a 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -1,4 +1,4 @@ -use crate::{AcpThread, ElicitationStore, ReceiptSubmission}; +use crate::{AcpThread, ElicitationStore, ReceiptSubmission, TaskOutput}; use agent_client_protocol::{schema::MaybeUndefined, v1::schema as acp_v1, v2::schema as acp_v2}; use anyhow::Result; use chrono::{DateTime, Utc}; @@ -164,6 +164,19 @@ pub trait AgentConnection { Task::ready(Err(anyhow::Error::msg("Closing sessions is not supported"))) } + /// Read the end of one of the session's tasks' output (`_claude/tasks/output`, + /// version 2). Unsupported unless the connection overrides it. + fn fetch_task_output( + self: Rc, + _session_id: &acp_v2::SessionId, + _task_id: &str, + _cx: &mut App, + ) -> Task> { + Task::ready(Err(anyhow::Error::msg( + "Reading agent task output is not supported", + ))) + } + /// Ask the agent to stop one of the session's tasks (`AcpThread::tasks`). fn stop_task( self: Rc, diff --git a/crates/acp_thread/src/test_data/claude_tasks_output.json b/crates/acp_thread/src/test_data/claude_tasks_output.json new file mode 100644 index 0000000..98b9872 --- /dev/null +++ b/crates/acp_thread/src/test_data/claude_tasks_output.json @@ -0,0 +1,5 @@ +{ + "content": "line 2\nline 3\n", + "totalBytes": 2097152, + "truncated": true +} diff --git a/crates/acp_thread/src/test_data/claude_tasks_snapshot_v2.json b/crates/acp_thread/src/test_data/claude_tasks_snapshot_v2.json new file mode 100644 index 0000000..63eb65f --- /dev/null +++ b/crates/acp_thread/src/test_data/claude_tasks_snapshot_v2.json @@ -0,0 +1,56 @@ +{ + "tasks": [ + { + "id": "b7k2m9x1q", + "toolCallId": "toolu_01ShellSleep20", + "type": "shell", + "description": "Run sleep 20 in the background", + "background": true, + "depth": 0, + "status": "running", + "startedAt": 1759500000000, + "endedAt": null, + "usage": null, + "lastTool": null, + "summary": null, + "hasOutput": true, + "tail": ["line 1", "line 2", "line 3"] + }, + { + "id": "a3f8c2d4e5b6", + "toolCallId": "toolu_01ExploreFeed", + "type": "subagent", + "description": "Explore the task feed", + "background": false, + "depth": 1, + "status": "running", + "startedAt": 1759500001000, + "endedAt": null, + "usage": { + "tokens": 18234, + "toolUses": 7, + "durationMs": 42000 + }, + "lastTool": "Grep", + "summary": "Reading the SDK task message types", + "hasOutput": false, + "tail": null + }, + { + "id": "c9d1e2f3g", + "toolCallId": "toolu_01RunLinter", + "type": "shell", + "description": "Run the linter", + "background": true, + "depth": 0, + "status": "completed", + "startedAt": 1759500002000, + "endedAt": 1759500005000, + "usage": null, + "lastTool": null, + "summary": "Background command \"Run the linter\" completed (exit code 0)", + "hasOutput": true, + "tail": null + } + ] +} diff --git a/crates/agent_servers/src/acp/v1.rs b/crates/agent_servers/src/acp/v1.rs index bf421ab..ea4ee5e 100644 --- a/crates/agent_servers/src/acp/v1.rs +++ b/crates/agent_servers/src/acp/v1.rs @@ -58,6 +58,9 @@ const PARAMETERIZED_MODEL_PICKER_META_KEY: &str = "parameterizedModelPicker"; const CLAUDE_TASKS_CAPABILITY: &str = "_claude/tasks"; const CLAUDE_TASKS_STOP_METHOD: &str = "_claude/tasks/stop"; const CLAUDE_TASKS_BACKGROUND_METHOD: &str = "_claude/tasks/background"; +const CLAUDE_TASKS_OUTPUT_METHOD: &str = "_claude/tasks/output"; +/// The most `_claude/tasks/output` serves; the adapter clamps to the same value. +const CLAUDE_TASKS_OUTPUT_MAX_BYTES: u64 = 1_048_576; async fn close_session_and_drain( connection: &ConnectionTo, session_id: acp_v2::SessionId, @@ -95,6 +98,31 @@ async fn send_task_request( .map_err(failed) } +/// Reads a `_claude/tasks/output` result strictly: a missing or wrong-typed field is an +/// error naming it, never a default value. +fn parse_task_output(response: &serde_json::Value) -> Result { + let field = |name: &str| { + response + .get(name) + .ok_or_else(|| anyhow!("the response has no `{name}`")) + }; + let content = field("content")? + .as_str() + .ok_or_else(|| anyhow!("`content` is not a string"))? + .to_string(); + let total_bytes = field("totalBytes")? + .as_u64() + .ok_or_else(|| anyhow!("`totalBytes` is not a non-negative integer"))?; + let truncated = field("truncated")? + .as_bool() + .ok_or_else(|| anyhow!("`truncated` is not a boolean"))?; + Ok(acp_thread::TaskOutput { + content, + total_bytes, + truncated, + }) +} + pub struct AcpConnection { id: AgentId, telemetry_id: SharedString, @@ -968,14 +996,21 @@ impl AcpConnection { /// Keyed on the capability alone: the entry id is whatever the user named it, and /// the agent's self-reported name differs between the fork and the published package. - fn agent_supports_tasks(&self) -> bool { + /// + /// Version 1 carries the feed, stop and background; version 2 (story 041) adds + /// `hasOutput`, `tail` and `_claude/tasks/output`. 0 when nothing is advertised. + fn agent_tasks_version(&self) -> u64 { self.agent_capabilities .meta .as_ref() .and_then(|meta| meta.get(CLAUDE_TASKS_CAPABILITY)) .and_then(|capability| capability.get("version")) .and_then(serde_json::Value::as_u64) - .is_some_and(|version| version >= 1) + .unwrap_or(0) + } + + fn agent_supports_tasks(&self) -> bool { + self.agent_tasks_version() >= 1 } /// `rpc_call` gets a receiver that resolves once its response has been applied, and @@ -2146,6 +2181,38 @@ impl AgentConnection for AcpConnection { .spawn(async move { request.await.map(|_| ()) }) } + fn fetch_task_output( + self: Rc, + session_id: &acp_v2::SessionId, + task_id: &str, + cx: &mut App, + ) -> Task> { + let version = self.agent_tasks_version(); + if version < 2 { + return Task::ready(Err(anyhow!( + "Reading agent task output is not supported: the agent advertises \ + {CLAUDE_TASKS_CAPABILITY} version {version}" + ))); + } + let params = serde_json::json!({ + "sessionId": session_id, + "taskId": task_id, + "maxBytes": CLAUDE_TASKS_OUTPUT_MAX_BYTES, + }); + let request = send_task_request( + self.connection.clone(), + CLAUDE_TASKS_OUTPUT_METHOD, + params, + task_id.to_string(), + ); + let task_id = task_id.to_string(); + cx.foreground_executor().spawn(async move { + let response = request.await?; + parse_task_output(&response) + .with_context(|| format!("{CLAUDE_TASKS_OUTPUT_METHOD} answered for {task_id}")) + }) + } + fn background_task( self: Rc, session_id: &acp_v2::SessionId, @@ -2531,6 +2598,17 @@ pub mod test_support { .background_task(session_id, tool_call_id, cx) } + fn fetch_task_output( + self: Rc, + session_id: &acp_v2::SessionId, + task_id: &str, + cx: &mut App, + ) -> Task> { + self.inner + .clone() + .fetch_task_output(session_id, task_id, cx) + } + fn request_elicitations(&self) -> Option> { self.inner.request_elicitations() } @@ -10489,6 +10567,7 @@ exit 7 } include!("../acp_task_actions_tests.rs"); + include!("../acp_task_output_tests.rs"); } fn mcp_servers_for_project(project: &Entity, cx: &App) -> Vec { diff --git a/crates/agent_servers/src/acp_task_output_tests.rs b/crates/agent_servers/src/acp_task_output_tests.rs new file mode 100644 index 0000000..46ca0c4 --- /dev/null +++ b/crates/agent_servers/src/acp_task_output_tests.rs @@ -0,0 +1,248 @@ +// Story 041, task 4.3 — `AcpConnection::fetch_task_output` against a fake ACP peer: the version +// gate, the request it sends, the strict response parse, the agent's error (R3.4, R3.5; Q4, Q12). +// +// Included at the end of `mod tests` in `acp.rs`, AFTER the story 012 file whose fake peer it +// reuses: +// +// include!("acp_task_actions_tests.rs"); +// include!("acp_task_output_tests.rs"); +// +// so `connect_task_actions_peer`, `task_actions_session`, `task_actions_requests`, +// `tasks_capability_v`, `PLUS_ENTRY` and `PLUS_NAME` are in scope. Every test name carries +// `tasks` and `task_output` (filter `cargo test -p agent_servers tasks`). +// +// Contract (design.md §`AgentConnection::fetch_task_output`): `fn fetch_task_output(self: Rc, +// session_id: &acp_v2::SessionId, task_id: &str, cx: &mut App) -> Task>`, the +// same shape as story 012's `stop_task`; `acp_thread::TaskOutput { content: String, total_bytes: +// u64, truncated: bool }`; output needs `_claude/tasks` version >= 2 while stop keeps >= 1. +// If the trait method's argument types differ, adapt `fetch` below only. +// +// Hostile halves first: a version 1 agent — one that stop and background still work with — must +// be sent NOTHING for output; a response missing any one field must be an error naming it, never a +// TaskOutput with a default value. + +const TASK_OUTPUT_FIXTURE: &str = + include_str!("../../acp_thread/src/test_data/claude_tasks_output.json"); + +async fn fetch( + peer: &TaskActionsPeer, + session_id: &acp_v2::SessionId, + task_id: &str, + cx: &mut gpui::TestAppContext, +) -> anyhow::Result { + cx.update(|cx| { + peer.connection + .clone() + .fetch_task_output(session_id, task_id, cx) + }) + .await +} + +fn task_output_reply(value: serde_json::Value) -> TaskActionsReply { + Arc::new(move |_method, _params| Ok(value.clone())) +} + +fn task_output_fixture() -> serde_json::Value { + serde_json::from_str(TASK_OUTPUT_FIXTURE).expect("the output fixture is JSON") +} + +// ---- R3.5 — hostile first: below version 2, nothing is sent -------------------------------- + +#[gpui::test] +async fn test_tasks_task_output_unsupported_below_version_2_and_sends_nothing( + cx: &mut gpui::TestAppContext, +) { + for capability in [None, tasks_capability_v(0), tasks_capability_v(1)] { + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + capability.clone(), + task_output_reply(task_output_fixture()), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + + let error = fetch(&peer, &session_id, "b1", cx) + .await + .expect_err("output is unsupported below version 2"); + let text = format!("{error:#}").to_lowercase(); + assert!(text.contains("not supported"), "{capability:?}: {text}"); + assert!( + task_actions_requests(&peer).is_empty(), + "{capability:?}: nothing reached the agent" + ); + } +} + +#[gpui::test] +async fn test_tasks_task_output_unsupported_names_the_advertised_version( + cx: &mut gpui::TestAppContext, +) { + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(1), + task_output_reply(task_output_fixture()), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + let text = format!( + "{:#}", + fetch(&peer, &session_id, "b1", cx).await.unwrap_err() + ); + assert!(text.contains("version 1"), "{text}"); +} + +// Converse: the version 1 gate of stop is unchanged by the new method. +#[gpui::test] +async fn test_tasks_task_output_version_1_agent_still_stops(cx: &mut gpui::TestAppContext) { + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(1), + task_actions_ok(), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + cx.update(|cx| peer.connection.clone().stop_task(&session_id, "b1", cx)) + .await + .expect("stop works for a version 1 agent"); + assert_eq!(task_actions_requests(&peer).len(), 1); +} + +// ---- R3.4 — the request and the response --------------------------------------------------- + +#[gpui::test] +async fn test_tasks_task_output_sends_one_request_and_reads_the_fixture( + cx: &mut gpui::TestAppContext, +) { + for version in [2, 3] { + let fixture = task_output_fixture(); + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(version), + task_output_reply(fixture.clone()), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + + let output = fetch(&peer, &session_id, "b1", cx) + .await + .unwrap_or_else(|error| panic!("v{version}: {error:#}")); + + assert_eq!(output.content, fixture["content"].as_str().unwrap()); + assert_eq!(output.total_bytes, fixture["totalBytes"].as_u64().unwrap()); + assert_eq!(output.truncated, fixture["truncated"].as_bool().unwrap()); + assert_eq!( + task_actions_requests(&peer), + vec![( + "_claude/tasks/output".to_string(), + serde_json::json!({ + "sessionId": session_id.to_string(), + "taskId": "b1", + "maxBytes": 1_048_576, + }), + )] + ); + } +} + +#[gpui::test] +async fn test_tasks_task_output_returns_the_agents_error_naming_method_and_task( + cx: &mut gpui::TestAppContext, +) { + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(2), + Arc::new(|_method, _params| { + Err(agent_client_protocol::Error::new( + -32603, + "output of task b1 is no longer available (ENOENT)", + )) + }), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + + let text = format!( + "{:#}", + fetch(&peer, &session_id, "b1", cx).await.unwrap_err() + ); + assert!( + text.contains("_claude/tasks/output failed for b1"), + "{text}" + ); + assert!(text.contains("no longer available (ENOENT)"), "{text}"); +} + +// Hostile: each missing or mistyped field is an error naming it, never a default. +#[gpui::test] +async fn test_tasks_task_output_strict_response_names_the_missing_field( + cx: &mut gpui::TestAppContext, +) { + let full = serde_json::json!({ "content": "x\n", "totalBytes": 2, "truncated": false }); + for field in ["content", "totalBytes", "truncated"] { + let mut partial = full.clone(); + partial.as_object_mut().unwrap().remove(field); + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(2), + task_output_reply(partial), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + let text = format!( + "{:#}", + fetch(&peer, &session_id, "b1", cx).await.unwrap_err() + ); + assert!(text.contains(field), "missing {field}: {text}"); + } + for bad in [ + serde_json::json!({ "content": 5, "totalBytes": 2, "truncated": false }), + serde_json::json!({ "content": "x", "totalBytes": -1, "truncated": false }), + serde_json::json!({ "content": "x", "totalBytes": "2", "truncated": false }), + serde_json::json!({ "content": "x", "totalBytes": 2, "truncated": "no" }), + ] { + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(2), + task_output_reply(bad.clone()), + cx, + ) + .await; + let (_thread, session_id) = task_actions_session(&peer, cx).await; + assert!(fetch(&peer, &session_id, "b1", cx).await.is_err(), "{bad}"); + } +} + +// The thread delegates to its connection. +#[gpui::test] +async fn test_tasks_task_output_thread_delegates_to_the_connection(cx: &mut gpui::TestAppContext) { + let peer = connect_task_actions_peer( + PLUS_ENTRY, + PLUS_NAME, + tasks_capability_v(2), + task_output_reply(task_output_fixture()), + cx, + ) + .await; + let (thread, _session_id) = task_actions_session(&peer, cx).await; + let output = thread + .update(cx, |thread, cx| { + thread.fetch_task_output(&acp_thread::AgentTaskId::from("b1".to_string()), cx) + }) + .await + .expect("the thread's fetch reaches the agent"); + assert!(output.truncated); + assert_eq!(task_actions_requests(&peer).len(), 1); +} diff --git a/crates/agent_tasks_panel/src/history_db.rs b/crates/agent_tasks_panel/src/history_db.rs index 88626c8..9dc81c8 100644 --- a/crates/agent_tasks_panel/src/history_db.rs +++ b/crates/agent_tasks_panel/src/history_db.rs @@ -44,7 +44,7 @@ pub struct AgentTaskHistoryDb(ThreadSafeConnection); /// The migration list lives in a `static` so that the list registered with `inventory` and /// `AgentTaskHistoryDb::MIGRATIONS` are one allocation rather than two copies of a `const`. -static HISTORY_MIGRATIONS: [&str; 3] = [ +static HISTORY_MIGRATIONS: [&str; 4] = [ sql!( CREATE TABLE IF NOT EXISTS agent_task_history( session_id TEXT NOT NULL, @@ -69,6 +69,8 @@ static HISTORY_MIGRATIONS: [&str; 3] = [ // Step 3 (story 023): the tool call that started the task, NULL when the feed gave none or // the record predates the column (R2.1, R2.2). sql!(ALTER TABLE agent_task_history ADD COLUMN tool_call_id TEXT;), + // Step 4 (story 041): the size of the task's saved log, NULL when it has none (R6.9). + sql!(ALTER TABLE agent_task_history ADD COLUMN log_bytes INTEGER;), ]; impl Domain for AgentTaskHistoryDb { @@ -90,6 +92,8 @@ pub struct HistoryEntry { pub archived: bool, /// The user's label, already normalized; `None` shows the description. pub label: Option, + /// The size of the task's saved log; `None` when it has none. + pub log_bytes: Option, } // The only delete is the user's, of records they archived first (story 025): every other record @@ -245,22 +249,36 @@ impl AgentTaskHistoryDb { /// Deletes a batch of archived tasks in one transaction: when any row fails, every row of /// the batch is kept. A key that is not archived is left in place, so a running or listed /// task is never deleted. Only the history row goes; the thread is never touched. - pub async fn delete(&self, keys: &[(acp_v2::SessionId, AgentTaskId)]) -> Result<()> { + /// Returns the indices into `keys` of the rows it actually removed: a key whose row is not + /// archived in the database (an archive write that failed) is not deleted, and its caller + /// must not delete that record's log either (story 041). + pub async fn delete(&self, keys: &[(acp_v2::SessionId, AgentTaskId)]) -> Result> { let keys: Vec<(String, String)> = keys .iter() .map(|(session_id, task_id)| (session_id.to_string(), task_id.to_string())) .collect(); self.write(move |connection| { connection.with_savepoint("agent_task_history_delete", || { + let mut archived = connection.select_row_bound::<(&str, &str), i64>(sql!( + SELECT 1 FROM agent_task_history + WHERE session_id = ?1 AND task_id = ?2 AND archived = 1 + ))?; let mut delete = connection.exec_bound(sql!( DELETE FROM agent_task_history WHERE session_id = ?1 AND task_id = ?2 AND archived = 1 ))?; - for (session_id, task_id) in &keys { - delete((session_id.as_str(), task_id.as_str())) - .with_context(|| format!("deleting {session_id}/{task_id}"))?; + let mut removed = Vec::new(); + for (index, (session_id, task_id)) in keys.iter().enumerate() { + let ids = (session_id.as_str(), task_id.as_str()); + let present = archived(ids) + .with_context(|| format!("reading {session_id}/{task_id}"))? + .is_some(); + delete(ids).with_context(|| format!("deleting {session_id}/{task_id}"))?; + if present { + removed.push(index); + } } - Ok(()) + Ok(removed) }) }) .await @@ -285,6 +303,25 @@ impl AgentTaskHistoryDb { .with_context(|| format!("labelling {ids}")) } + /// Sets the size of one task's saved log, or clears it (`None`) when the task has no log. + /// Only that `(session_id, task_id)` row is written. + pub async fn set_log_bytes( + &self, + session_id: &acp_v2::SessionId, + task_id: &AgentTaskId, + bytes: Option, + ) -> Result<()> { + let ids = ids_of(session_id, task_id); + let bindings = (bytes, session_id.to_string(), task_id.to_string()); + self.write(move |connection| { + connection.exec_bound(sql!( + UPDATE agent_task_history SET log_bytes = ?1 WHERE session_id = ?2 AND task_id = ?3 + ))?(bindings) + }) + .await + .with_context(|| format!("recording the log size of {ids}")) + } + /// Every record, archived or not, newest first. /// /// A row this build cannot read -- a status a newer Zeo wrote, then opened by an older @@ -294,7 +331,8 @@ impl AgentTaskHistoryDb { pub async fn load_all(&self) -> Result> { let rows = self.select::(sql!( SELECT session_id, task_id, thread_title, project, kind, description, background, - status, started_at, ended_at, tokens, tool_uses, archived, label, tool_call_id + status, started_at, ended_at, tokens, tool_uses, archived, label, tool_call_id, + log_bytes FROM agent_task_history ORDER BY started_at DESC, session_id, task_id ))?() @@ -334,6 +372,7 @@ impl Column for HistoryRow { let (archived, next): (bool, i32) = Column::column(statement, next)?; let (label, next): (Option, i32) = Column::column(statement, next)?; let (tool_call_id, next): (Option, i32) = Column::column(statement, next)?; + let (log_bytes, next): (Option, i32) = Column::column(statement, next)?; let status = match parse_status(&status) { Ok(status) => status, @@ -364,6 +403,7 @@ impl Column for HistoryRow { record, archived, label: label.map(Into::into), + log_bytes, })), next, )) @@ -420,3 +460,11 @@ mod label; #[cfg(test)] #[path = "history_db/tool_call_id_tests.rs"] mod tool_call_id; + +#[cfg(test)] +#[path = "history_db/log_bytes_tests.rs"] +mod log_bytes; + +#[cfg(test)] +#[path = "history_db/delete_removed_tests.rs"] +mod delete_removed; diff --git a/crates/agent_tasks_panel/src/history_db/delete_removed_tests.rs b/crates/agent_tasks_panel/src/history_db/delete_removed_tests.rs new file mode 100644 index 0000000..cb09284 --- /dev/null +++ b/crates/agent_tasks_panel/src/history_db/delete_removed_tests.rs @@ -0,0 +1,38 @@ +// Story 041, group 5 tech review (S1): `delete` reports only the rows it removed, so a record +// whose archive write failed (still unarchived in the database) keeps its log file. + +use acp_thread::{AgentTaskId, AgentTaskStatus}; +use gpui::TestAppContext; + +use crate::AgentTaskHistoryDb; +use crate::test_helpers::*; + +#[gpui::test] +async fn history_db_delete_reports_only_the_archived_rows_it_removed(_cx: &mut TestAppContext) { + let db = AgentTaskHistoryDb::open_test_db("history_db_delete_reports_removed").await; + for (index, task) in ["archived", "still_open"].iter().enumerate() { + let at = 1_000 + index as i64 * 100; + db.insert_started(&record("s1", task, AgentTaskStatus::Running, at)) + .await + .unwrap(); + db.update_ended(&record("s1", task, AgentTaskStatus::Completed, at)) + .await + .unwrap(); + } + let key = |task: &str| (sid("s1"), AgentTaskId::from(task.to_string())); + db.set_archived(&[key("archived")], true).await.unwrap(); + + let removed = db + .delete(&[key("archived"), key("still_open")]) + .await + .unwrap(); + + assert_eq!( + removed, + vec![0], + "only the archived row is removed and reported" + ); + let left = db.load_all().await.unwrap(); + assert!(find(&left, "s1", "still_open").is_some()); + assert!(find(&left, "s1", "archived").is_none()); +} diff --git a/crates/agent_tasks_panel/src/history_db/log_bytes_tests.rs b/crates/agent_tasks_panel/src/history_db/log_bytes_tests.rs new file mode 100644 index 0000000..0a87f60 --- /dev/null +++ b/crates/agent_tasks_panel/src/history_db/log_bytes_tests.rs @@ -0,0 +1,196 @@ +// Story 041, task 5.2 (database half) — the history records each task's log size: one appended +// migration step, `set_log_bytes`, `HistoryEntry::log_bytes` (R6.1, R6.9; Q14). Module path: +// `history_db::log_bytes`. Wired in `src/history_db.rs` as +// +// #[cfg(test)] +// #[path = "history_db/log_bytes_tests.rs"] +// mod log_bytes; +// +// Validation filter: `cargo test -p agent_tasks_panel` (names carry `history_db_log_bytes`). +// +// Contract (design.md §Task history): the LAST step of `HISTORY_MIGRATIONS` is `ALTER TABLE +// agent_task_history ADD COLUMN log_bytes INTEGER;` (NULL = no log); `AgentTaskHistoryDb:: +// set_log_bytes(session_id, task_id, bytes: Option)`; `HistoryEntry::log_bytes: Option`. +// The step's index is not pinned: a batch sibling may append before this story, so the tests pin +// the three shipped steps and the last one, never the length. Argument types are adapted in `set`. +// +// Hostile halves first: a database written by the SHIPPED steps, holding records, must open with +// the new list and keep every record with no log; a size set on (s1, t1) must not land on +// (s1, t10) or (s2, t1). + +use acp_thread::{AgentTaskId, AgentTaskStatus}; +use db::sqlez::{connection::Connection, domain::Domain}; +use db::sqlez_macros::sql; +use gpui::TestAppContext; + +use crate::test_helpers::*; +use crate::{AgentTaskHistoryDb, HistoryEntry}; + +/// Step 0 exactly as patch 0035 shipped it (story 023's test carries the same copy). +const SHIPPED_STEP_0: &str = sql!( + CREATE TABLE IF NOT EXISTS agent_task_history( + session_id TEXT NOT NULL, + task_id TEXT NOT NULL, + thread_title TEXT, + project TEXT NOT NULL, + kind TEXT NOT NULL, + description TEXT NOT NULL, + background INTEGER NOT NULL, + status TEXT NOT NULL, + started_at INTEGER NOT NULL, + ended_at INTEGER, + tokens INTEGER, + tool_uses INTEGER, + archived INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY(session_id, task_id) + ) STRICT; + CREATE INDEX IF NOT EXISTS agent_task_history_started ON agent_task_history(started_at); +); +const SHIPPED_STEP_1: &str = sql!(ALTER TABLE agent_task_history ADD COLUMN label TEXT;); +const SHIPPED_STEP_2: &str = sql!(ALTER TABLE agent_task_history ADD COLUMN tool_call_id TEXT;); +const LOG_BYTES_STEP: &str = sql!(ALTER TABLE agent_task_history ADD COLUMN log_bytes INTEGER;); + +fn migrate_all(connection: &Connection) { + connection + .migrate( + AgentTaskHistoryDb::NAME, + AgentTaskHistoryDb::MIGRATIONS, + &mut |_, _, _| false, + ) + .expect("the history migrations should apply"); +} + +/// One fixed SQL statement of this test (sqlez, not a process). +fn run_sql_now(connection: &Connection, statement: &str) { + let mut run = connection.exec(statement).expect("statement prepares"); + run().expect("statement runs"); +} + +async fn set(db: &AgentTaskHistoryDb, session: &str, task: &str, bytes: Option) { + db.set_log_bytes(&sid(session), &AgentTaskId::from(task.to_string()), bytes) + .await + .expect("set_log_bytes should succeed"); +} + +fn log_bytes_of(rows: &[HistoryEntry], session: &str, task: &str) -> Option { + find(rows, session, task) + .unwrap_or_else(|| panic!("{session}/{task} is recorded")) + .log_bytes +} + +async fn seeded(name: &'static str, keys: &[(&str, &str)]) -> AgentTaskHistoryDb { + let db = AgentTaskHistoryDb::open_test_db(name).await; + for (index, (session, task)) in keys.iter().enumerate() { + let at = 1_000 + index as i64 * 100; + db.insert_started(&record(session, task, AgentTaskStatus::Running, at)) + .await + .unwrap(); + db.update_ended(&record(session, task, AgentTaskStatus::Completed, at)) + .await + .unwrap(); + } + db +} + +// ── Migration (R6.9, Q14) ────────────────────────────────────────────────── + +// Hostile: a pre-story database with records opens, keeps every record, each with no log. +#[test] +fn history_db_log_bytes_pre_story_database_keeps_every_record_without_a_log() { + let connection = Connection::open_memory(Some("history_db_log_bytes_pre_story")); + connection + .migrate( + AgentTaskHistoryDb::NAME, + &[SHIPPED_STEP_0, SHIPPED_STEP_1, SHIPPED_STEP_2], + &mut |_, _, _| false, + ) + .expect("the shipped steps apply"); + for (session, task) in [("s-old", "t1"), ("s-old", "t2"), ("s-other", "t1")] { + run_sql_now( + &connection, + &format!( + "INSERT INTO agent_task_history \ + (session_id, task_id, thread_title, project, kind, description, background, \ + status, started_at, ended_at, archived, label, tool_call_id) \ + VALUES ('{session}', '{task}', 'Old', 'zeo', 'shell', 'Old task', 1, \ + 'completed', 1000, 2000, 1, 'kept label', 'toolu_x')" + ), + ); + } + + migrate_all(&connection); + migrate_all(&connection); + + let rows = connection + .select::<(String, String, Option, Option, Option)>( + "SELECT session_id, task_id, label, tool_call_id, log_bytes FROM agent_task_history \ + ORDER BY session_id, task_id", + ) + .expect("select")() + .expect("run"); + assert_eq!(rows.len(), 3, "every record survives"); + for (session, task, label, tool_call_id, log_bytes) in rows { + assert_eq!(label.as_deref(), Some("kept label"), "{session}/{task}"); + assert_eq!(tool_call_id.as_deref(), Some("toolu_x"), "{session}/{task}"); + assert_eq!(log_bytes, None, "{session}/{task} has no log"); + } +} + +#[test] +fn history_db_log_bytes_is_appended_after_the_shipped_steps() { + let steps = AgentTaskHistoryDb::MIGRATIONS; + assert_eq!(steps[0], SHIPPED_STEP_0, "step 0 is never edited"); + assert!( + steps[1] == SHIPPED_STEP_1, + "story 022's step is left as shipped" + ); + assert!( + steps[2] == SHIPPED_STEP_2, + "story 023's step is left as shipped" + ); + assert!( + *steps.last().unwrap() == LOG_BYTES_STEP, + "this story's step is the last one" + ); + assert_eq!( + steps.iter().filter(|step| **step == LOG_BYTES_STEP).count(), + 1, + "appended exactly once" + ); +} + +// ── set_log_bytes and load_all (R6.1) ────────────────────────────────────── + +#[gpui::test] +async fn history_db_log_bytes_fresh_records_have_no_log(_cx: &mut TestAppContext) { + let db = seeded("history_db_log_bytes_fresh", &[("s1", "t1")]).await; + assert_eq!(log_bytes_of(&load(&db).await, "s1", "t1"), None); +} + +// Hostile (wrong collapse): only (s1, t1) carries the size. +#[gpui::test] +async fn history_db_log_bytes_set_touches_only_its_own_row(_cx: &mut TestAppContext) { + let db = seeded( + "history_db_log_bytes_own_row", + &[("s1", "t1"), ("s1", "t10"), ("s2", "t1")], + ) + .await; + + set(&db, "s1", "t1", Some(4_096)).await; + + let rows = load(&db).await; + assert_eq!(log_bytes_of(&rows, "s1", "t1"), Some(4_096)); + assert_eq!(log_bytes_of(&rows, "s1", "t10"), None); + assert_eq!(log_bytes_of(&rows, "s2", "t1"), None); +} + +#[gpui::test] +async fn history_db_log_bytes_replaces_and_clears(_cx: &mut TestAppContext) { + let db = seeded("history_db_log_bytes_replace", &[("s1", "t1")]).await; + set(&db, "s1", "t1", Some(10)).await; + set(&db, "s1", "t1", Some(1_048_620)).await; + assert_eq!(log_bytes_of(&load(&db).await, "s1", "t1"), Some(1_048_620)); + + set(&db, "s1", "t1", None).await; + assert_eq!(log_bytes_of(&load(&db).await, "s1", "t1"), None); +} diff --git a/crates/agent_tasks_panel/src/history_db/tool_call_id_tests.rs b/crates/agent_tasks_panel/src/history_db/tool_call_id_tests.rs index 5c7fc30..c27a730 100644 --- a/crates/agent_tasks_panel/src/history_db/tool_call_id_tests.rs +++ b/crates/agent_tasks_panel/src/history_db/tool_call_id_tests.rs @@ -176,9 +176,16 @@ fn history_db_tool_call_id_is_one_step_appended_after_the_shipped_one() { == sql!(ALTER TABLE agent_task_history ADD COLUMN label TEXT;), "the label step before it is left as shipped" ); + // Later stories append after this step (story 041's `log_bytes`), so the total length is + // not pinned; this story's step must still appear exactly once. assert_eq!( - AgentTaskHistoryDb::MIGRATIONS.len(), - 3, + AgentTaskHistoryDb::MIGRATIONS + .iter() + .filter(|step| { + **step == sql!(ALTER TABLE agent_task_history ADD COLUMN tool_call_id TEXT;) + }) + .count(), + 1, "exactly one step is appended" ); assert!( diff --git a/crates/agent_tasks_panel/src/panel.rs b/crates/agent_tasks_panel/src/panel.rs index b68d452..3ec5bbc 100644 --- a/crates/agent_tasks_panel/src/panel.rs +++ b/crates/agent_tasks_panel/src/panel.rs @@ -15,7 +15,7 @@ use acp_thread::{AgentTask, AgentTaskId, AgentTaskStatus}; use agent_client_protocol::v2::schema as acp_v2; use agent_ui::{ AgentPanel, DEFAULT_THREAD_TITLE, OpenAgentSessionOptions, OpenedAgentSession, TaskRecord, - TasksCard, collapse_label, display_label, open_agent_session, + TasksCard, collapse_label, display_label, open_agent_session, task_log, thread_metadata_store::ThreadMetadataStore, }; use anyhow::{Result, anyhow}; @@ -44,6 +44,9 @@ use crate::{Availability, HistoryEntry, TaskHistoryStore, TaskKey}; /// no id was recorded, or the call belongs to a nested subagent (story 023 R3.3). const NO_ENTRY_NOTICE: &str = "Opened the thread; this task has no entry to jump to"; +/// What a row says when its task's saved log file is gone (story 041 R5.8). +const LOG_GONE_NOTICE: &str = "Log no longer available"; + actions!( agent_tasks_panel, [ @@ -197,6 +200,8 @@ pub struct PanelRow { pub can_stop: bool, /// The task has ended and is not archived yet (R3.4). pub can_archive: bool, + /// The task has a saved log, so "Open Log" is offered (story 041 R5.7). + pub can_open_log: bool, /// The row is in the panel's selection (story 034 R5). pub selected: bool, /// Why the last click on the row opened nothing (R2.7). @@ -226,6 +231,7 @@ enum ListEntry { struct RowFocus { open: FocusHandle, rename: FocusHandle, + open_log: FocusHandle, stop: FocusHandle, /// Archive, or Unarchive on an archived row: the two share the row's last slot. archive: FocusHandle, @@ -238,6 +244,7 @@ impl RowFocus { Self { open: control_focus_handle(cx), rename: control_focus_handle(cx), + open_log: control_focus_handle(cx), stop: control_focus_handle(cx), archive: control_focus_handle(cx), delete: control_focus_handle(cx), @@ -825,6 +832,107 @@ impl AgentTasksPanel { } } + /// Opens `key`'s saved log read-only in an editor tab of this panel's workspace (story 041 + /// R5.9). A log whose file is gone opens nothing: the row says so instead (R5.8). + /// + /// The existence check reads the disk off the UI thread, and the open runs after it, so it + /// never runs inside the caller's update of this panel. + pub fn open_log(&mut self, key: &TaskKey, window: &mut Window, cx: &mut Context) { + let (session_id, task_id) = key; + // "Not local" covers both SSH and collab projects, and runs before any filesystem check: + // the log is a local file the remote host would resolve instead (R5.11). + let remote = self + .workspace + .read_with(cx, |workspace, cx| !workspace.project().read(cx).is_local()) + .unwrap_or(false); + if remote { + log::info!( + "agent_task_log event=refused session_id={session_id} task_id={task_id} \ + reason=remote_project source=agent_tasks_panel" + ); + self.set_notice(key, task_log::REMOTE_PROJECT_NOTICE, cx); + return; + } + // The logs root is resolved here, on the foreground: tests point it elsewhere through a + // global the background executor cannot read. + let path = task_log::log_path( + &task_log::logs_root_in(cx), + &session_id.to_string(), + task_id, + ); + let key = key.clone(); + let workspace = self.workspace.clone(); + cx.spawn_in(window, async move |this, cx| { + let exists = cx + .background_spawn({ + let path = path.clone(); + async move { path.is_file() } + }) + .await; + if !exists { + log::info!( + "agent_task_log event=missing session_id={} task_id={} path={}", + key.0, + key.1, + path.display() + ); + this.update(cx, |panel, cx| panel.set_notice(&key, LOG_GONE_NOTICE, cx)) + .ok(); + return; + } + let opened = match workspace.update_in(cx, |workspace, window, cx| { + agent_ui::open_abs_path_at_point_task(workspace, path.clone(), None, window, cx) + }) { + Ok(open) => match open.await { + Ok(Some(item)) => Ok(item), + Ok(None) => Err(anyhow!("no file at {}", path.display())), + Err(error) => Err(error), + }, + Err(error) => Err(error.context("this panel's workspace is gone")), + }; + let opened = opened.and_then(|item| { + item.downcast::().ok_or_else(|| { + anyhow!("the log did not open in an editor, so it is not read-only") + }) + }); + if let Ok(editor) = &opened { + editor.update(cx, |editor, cx| { + editor.set_read_only(true); + cx.notify(); + }); + } + // A panel closed meanwhile has no row left to show the outcome on. + this.update(cx, |panel, cx| panel.finish_open_log(&key, opened, cx)) + .ok(); + }) + .detach(); + } + + /// Shows how opening `key`'s log ended: a failure on the task's row, with a WARN line naming + /// the session and the task; an open clears an earlier notice. + fn finish_open_log( + &mut self, + key: &TaskKey, + opened: Result>, + cx: &mut Context, + ) { + let (session_id, task_id) = key; + match opened { + Ok(_) => { + if self.notices.remove(key).is_some() { + cx.notify(); + } + } + Err(error) => { + log::warn!( + "agent_task_log event=failed session_id={session_id} task_id={task_id} \ + reason=open_tab source=agent_tasks_panel error={error:#}" + ); + self.set_notice(key, format!("Could not open the log: {error:#}"), cx); + } + } + } + /// Stops `key`'s running task through the card that holds it live, so the panel's Stop /// is the card's: the same confirmation, and an error kept on the card's row, which the /// panel reads back (R2.6). @@ -1481,7 +1589,8 @@ fn render_header( } /// `row`'s line: its label (the user's, else the description) and its progress or outcome — -/// together the control that opens its thread (R2.5) — then Stop while it runs (R2.6), or +/// together the control that opens its thread (R2.5) — then Open Log when the task has a saved +/// log (story 041 R5.7), Stop while it runs (R2.6), or /// Archive once it has ended (R3.1, R3.4), or Unarchive and Delete when archived (R3.3; story /// 025). Watched or /// expanded detail stays on the card, so every row keeps the one-line height `uniform_list` @@ -1622,6 +1731,29 @@ fn render_row( } })), ); + let open_log_button = row.can_open_log.then(|| { + div() + .debug_selector({ + let task_id = record.task_id.clone(); + move || format!("agent-tasks-open-log-{task_id}") + }) + .child( + action_button( + control_id("open-log"), + IconName::FileTextOutlined, + format!("Open log: {shown_label}"), + "Open Log", + &focus.open_log, + ) + .on_click(cx.listener({ + let key = key.clone(); + move |panel, _, window, cx| { + cx.stop_propagation(); + panel.open_log(&key, window, cx) + } + })), + ) + }); let stop_button = row.can_stop.then(|| { action_button( control_id("stop"), @@ -1689,6 +1821,7 @@ fn render_row( .gap_1() .child(div().min_w_0().flex_1().child(open_or_rename)) .child(rename_button) + .children(open_log_button) .children(stop_button) .children(archive_button) .children(delete_button) @@ -1891,6 +2024,7 @@ fn panel_row( PanelRow { can_stop, can_archive: !entry.archived && !entry.record.status.is_running(), + can_open_log: entry.log_bytes.is_some(), selected, notice, error, @@ -2247,3 +2381,11 @@ mod delete_selected; #[cfg(test)] #[path = "panel/header_archive_filter_tests.rs"] mod header_archive_filter; + +#[cfg(test)] +#[path = "panel/open_log_tests.rs"] +mod open_log; + +#[cfg(test)] +#[path = "panel/open_log_remote_tests.rs"] +mod open_log_remote; diff --git a/crates/agent_tasks_panel/src/panel/open_log_remote_tests.rs b/crates/agent_tasks_panel/src/panel/open_log_remote_tests.rs new file mode 100644 index 0000000..5faaada --- /dev/null +++ b/crates/agent_tasks_panel/src/panel/open_log_remote_tests.rs @@ -0,0 +1,211 @@ +// Story 041, task 7.2 — "Open Log" on a history row is refused when the panel's project is remote +// (R5.11; Q5, Q16). Module path: `panel::open_log_remote`. Wired in `src/panel.rs`, beside 7.1's +// `open_log` module: +// +// #[cfg(test)] +// #[path = "panel/open_log_remote_tests.rs"] +// mod open_log_remote; +// +// Validation filter: `cargo test -p agent_tasks_panel open_log` (every test name below contains +// `open_log`). +// +// The remote seam, stated plainly: a real `Project` switched into its non-local client state by +// `Project::mark_as_collab_for_testing()` (project test-support, already used by +// `agent_panel.rs`'s own tests). After it `is_local()` is false and `is_remote()` true, through +// the project's real client state, not a stubbed predicate. It is NOT the SSH shape +// (`is_via_remote_server()` stays false): this crate has no `proto` dependency to answer the +// mock transport's connect `Ping`, and the card-side file +// (`agent_ui/src/tasks_card/open_log_remote_tests.rs`) already proves the SSH shape over a real +// `Project::remote`. Together they pin the guard as design.md words it — "checks whether the +// workspace's project is local" — so a guard written as `is_via_remote_server()` alone fails here. +// If Run mode adds `proto` as a dev-dependency, an SSH-shaped variant of test 1 may be added; do +// not replace this one. +// +// Contract (design.md §Agent Tasks panel, "Remote projects (R5.11)"): when the panel's workspace +// project is remote, `open_log` calls `set_notice(key, "Open Log is not available in remote +// projects", cx)` and opens nothing, BEFORE any filesystem check. Beyond it, this file assumes +// what 7.1's `open_log_tests.rs` assumes: `AgentTasksPanel::open_log(&key, window, cx)`, +// `PanelRow::can_open_log`, `agent_ui::task_log::{LogsRootOverride, log_path, write_log}`, and +// the `test_helpers` (`init_base`, `record`, `open_panels`, `find_row`, `notice_of`, `key`, `sid`). +// +// Order: hostile halves first — +// 1. remote, log file present: the notice, no tab (the "wrongly opens" half); +// 2. remote, log file GONE: still the remote notice, never "Log no longer available" — the +// check comes before the filesystem is consulted (a guard placed after `is_file()` collapses +// the two notices into the wrong one); +// then the converse — +// 3. local: the same row opens its log read-only with no notice. +// +// Red, before 7.2: tests 1 and 2 fail on the notice assertion (the panel opens the log, or says +// "Log no longer available"). Test 3 is the guard against an over-broad fix and may already pass +// after 7.1. + +use std::path::{Path, PathBuf}; + +use acp_thread::{AgentTaskId, AgentTaskStatus}; +use agent_ui::task_log::{LogsRootOverride, log_path, write_log}; +use gpui::TestAppContext; + +use crate::test_helpers::*; + +const REMOTE_NOTICE: &str = "Open Log is not available in remote projects"; + +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-panel-log-remote-{name}-{}-{nanos}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } + fn root(&self) -> PathBuf { + self.0.join("agent_task_logs") + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// Records (s1, t1) ended, with a saved log of `text`, before the panel loads. +async fn seed(root: &Path, text: &str, cx: &mut TestAppContext) -> PathBuf { + let db = init_base(cx); + let root_owned = root.to_path_buf(); + cx.update(|cx| cx.set_global(LogsRootOverride(root_owned))); + db.insert_started(&record("s1", "t1", AgentTaskStatus::Running, 1_000)) + .await + .unwrap(); + db.update_ended(&record("s1", "t1", AgentTaskStatus::Completed, 1_000)) + .await + .unwrap(); + let path = log_path(root, "s1", "t1"); + let bytes = write_log(&path, text).expect("seed the log"); + db.set_log_bytes( + &sid("s1"), + &AgentTaskId::from("t1".to_string()), + Some(bytes), + ) + .await + .unwrap(); + path +} + +/// The workspace opens through its project's FakeFs: seed a copy there too, so an unguarded +/// open in tests 1 and 3 really can show the text. +async fn seed_fake_copy(panels: &mut Panels, path: &Path, text: &str) { + let fs = panels.cx.update(|_, cx| ::global(cx)); + fs.create_dir(path.parent().unwrap()).await.unwrap(); + fs.as_fake() + .insert_file(path, text.as_bytes().to_vec()) + .await; +} + +fn make_project_remote(panels: &mut Panels) { + let workspace = panels.workspace.clone(); + let project = workspace.update(&mut panels.cx, |workspace, _| workspace.project().clone()); + project.update(&mut panels.cx, |project, _| { + project.mark_as_collab_for_testing() + }); + project.update(&mut panels.cx, |project, _| { + assert!( + !project.is_local(), + "the rig must make the panel's project non-local" + ); + }); +} + +fn active_editor(panels: &mut Panels) -> Option<(bool, String)> { + let workspace = panels.workspace.clone(); + workspace.update(&mut panels.cx, |workspace, cx| { + workspace + .active_item_as::(cx) + .map(|editor| { + let editor = editor.read(cx); + (editor.read_only(cx), editor.text(cx)) + }) + }) +} + +fn open_log(panels: &mut Panels) { + let panel = panels.tasks_panel.clone(); + let key = key("s1", "t1"); + panel.update_in(&mut panels.cx, |panel, window, cx| { + panel.open_log(&key, window, cx) + }); + panels.cx.run_until_parked(); +} + +fn notice(panels: &mut Panels) -> Option { + let panel = panels.tasks_panel.clone(); + notice_of(&panel, &sid("s1"), "t1", &mut panels.cx).map(|notice| notice.to_string()) +} + +// ---- 1. Hostile: a remote project, the log present — open nothing, say why ---------------- + +#[gpui::test] +async fn open_log_in_a_remote_project_shows_the_notice_and_opens_nothing(cx: &mut TestAppContext) { + let scratch = Scratch::new("remote"); + let path = seed(&scratch.root(), "REMOTE-PANEL-LOG\n", cx).await; + let mut panels = open_panels(cx).await; + seed_fake_copy(&mut panels, &path, "REMOTE-PANEL-LOG\n").await; + make_project_remote(&mut panels); + let before = active_editor(&mut panels); + + open_log(&mut panels); + + assert_eq!(notice(&mut panels).as_deref(), Some(REMOTE_NOTICE)); + assert_eq!(active_editor(&mut panels), before, "no tab opened"); + assert!(path.is_file(), "refusing never touches the saved log"); +} + +// ---- 2. Hostile: a remote project, the log gone — the remote check comes first ------------- + +#[gpui::test] +async fn open_log_in_a_remote_project_refuses_before_checking_the_file(cx: &mut TestAppContext) { + let scratch = Scratch::new("remote-missing"); + let path = seed(&scratch.root(), "gone\n", cx).await; + let mut panels = open_panels(cx).await; + make_project_remote(&mut panels); + std::fs::remove_file(&path).unwrap(); + let before = active_editor(&mut panels); + + open_log(&mut panels); + + let notice = notice(&mut panels); + assert_ne!( + notice.as_deref(), + Some("Log no longer available"), + "the remote check runs before any filesystem check" + ); + assert_eq!(notice.as_deref(), Some(REMOTE_NOTICE)); + assert_eq!(active_editor(&mut panels), before, "no tab opened"); +} + +// ---- 3. Converse: the same row in a local project opens its log ---------------------------- + +#[gpui::test] +async fn open_log_in_a_local_project_still_opens_the_log_read_only(cx: &mut TestAppContext) { + let scratch = Scratch::new("local"); + let path = seed(&scratch.root(), "LOCAL-PANEL-LOG\n", cx).await; + let mut panels = open_panels(cx).await; + seed_fake_copy(&mut panels, &path, "LOCAL-PANEL-LOG\n").await; + let workspace = panels.workspace.clone(); + let project = workspace.update(&mut panels.cx, |workspace, _| workspace.project().clone()); + project.update(&mut panels.cx, |project, _| assert!(project.is_local())); + + open_log(&mut panels); + + let (read_only, text) = active_editor(&mut panels).expect("an editor tab opened"); + assert!(read_only, "the saved log opens read-only"); + assert_eq!(text, "LOCAL-PANEL-LOG\n"); + assert_eq!(notice(&mut panels), None, "a local project gets no notice"); +} diff --git a/crates/agent_tasks_panel/src/panel/open_log_tests.rs b/crates/agent_tasks_panel/src/panel/open_log_tests.rs new file mode 100644 index 0000000..9060c3a --- /dev/null +++ b/crates/agent_tasks_panel/src/panel/open_log_tests.rs @@ -0,0 +1,168 @@ +// Story 041, task 7.1 — "Open Log" on a history row of the Agent Tasks panel (R5.7, R5.8; Q16). +// Module path: `panel::open_log`. Wired in `src/panel.rs` as +// +// #[cfg(test)] +// #[path = "panel/open_log_tests.rs"] +// mod open_log; +// +// Validation filter: `cargo test -p agent_tasks_panel open_log`. +// +// Contract (design.md §Agent Tasks panel) and what this file assumes beyond it: +// - `AgentTasksPanel::open_log(&key, window, cx)`, the same shape as story 023's `open_task`; +// - `PanelRow::can_open_log: bool`, true exactly when the entry has `log_bytes`, beside story +// 014's `can_stop` / `can_archive`; +// - the log root seam shared with the card and store tests: `agent_ui::task_log::LogsRootOverride` +// as a gpui global, read through `task_log::logs_root_in(cx)`; +// - the existence check reads the real disk (`is_file()`), while the workspace opens through its +// project's FakeFs: so each test writes the real file AND seeds a FakeFs copy at the same path. +// +// Hostile halves first: a record without a log is not offered "Open Log"; with (s1, t1) logged +// and (s1, t10) not, opening t1 shows t1's text; a record whose file is gone says "Log no longer +// available" and opens nothing. + +use std::path::{Path, PathBuf}; + +use acp_thread::{AgentTaskId, AgentTaskStatus}; +use agent_ui::task_log::{LogsRootOverride, log_path, write_log}; +use gpui::TestAppContext; + +use crate::test_helpers::*; + +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-panel-log-{name}-{}-{nanos}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } + fn root(&self) -> PathBuf { + self.0.join("agent_task_logs") + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// Records (s1, t1) with a log of `t1_text` and (s1, t10) without one, before the panel loads. +async fn seed(root: &Path, t1_text: &str, cx: &mut TestAppContext) -> PathBuf { + let db = init_base(cx); + let root_owned = root.to_path_buf(); + cx.update(|cx| cx.set_global(LogsRootOverride(root_owned))); + for (index, task) in ["t1", "t10"].iter().enumerate() { + let at = 1_000 + index as i64 * 100; + db.insert_started(&record("s1", task, AgentTaskStatus::Running, at)) + .await + .unwrap(); + db.update_ended(&record("s1", task, AgentTaskStatus::Completed, at)) + .await + .unwrap(); + } + let path = log_path(root, "s1", "t1"); + let bytes = write_log(&path, t1_text).expect("seed the log"); + db.set_log_bytes( + &sid("s1"), + &AgentTaskId::from("t1".to_string()), + Some(bytes), + ) + .await + .unwrap(); + path +} + +async fn seed_fake_copy(panels: &mut Panels, path: &Path, text: &str) { + let fs = panels.cx.update(|_, cx| ::global(cx)); + // FakeFs::insert_file needs the parent directory to exist (Run-mode fix, story 041 7.1). + fs.create_dir(path.parent().expect("a log has a parent dir")) + .await + .unwrap(); + fs.as_fake() + .insert_file(path, text.as_bytes().to_vec()) + .await; +} + +fn row(panels: &mut Panels, task: &str) -> crate::PanelRow { + find_row(panels, "s1", task).unwrap_or_else(|| panic!("s1/{task} is listed")) +} + +fn active_editor(panels: &mut Panels) -> Option<(bool, String)> { + let workspace = panels.workspace.clone(); + workspace.update(&mut panels.cx, |workspace, cx| { + workspace + .active_item_as::(cx) + .map(|editor| { + let editor = editor.read(cx); + (editor.read_only(cx), editor.text(cx)) + }) + }) +} + +fn open_log(panels: &mut Panels, task: &str) { + let panel = panels.tasks_panel.clone(); + let key = key("s1", task); + panel.update_in(&mut panels.cx, |panel, window, cx| { + panel.open_log(&key, window, cx) + }); + panels.cx.run_until_parked(); +} + +// Hostile first: only a record with a log is offered the action. +#[gpui::test] +async fn open_log_is_offered_only_for_a_record_with_a_log(cx: &mut TestAppContext) { + let scratch = Scratch::new("offered"); + seed(&scratch.root(), "t1 output\n", cx).await; + let mut panels = open_panels(cx).await; + + assert!(row(&mut panels, "t1").can_open_log); + assert!( + !row(&mut panels, "t10").can_open_log, + "no log_bytes, no Open Log" + ); +} + +#[gpui::test] +async fn open_log_opens_the_records_own_log_read_only(cx: &mut TestAppContext) { + let scratch = Scratch::new("open"); + let path = seed(&scratch.root(), "t1 output\nline 2\n", cx).await; + let mut panels = open_panels(cx).await; + seed_fake_copy(&mut panels, &path, "t1 output\nline 2\n").await; + + open_log(&mut panels, "t1"); + + let (read_only, text) = active_editor(&mut panels).expect("an editor tab opened"); + assert!(read_only, "the saved log opens read-only"); + assert_eq!(text, "t1 output\nline 2\n"); + let panel = panels.tasks_panel.clone(); + assert_eq!(notice_of(&panel, &sid("s1"), "t1", &mut panels.cx), None); +} + +// Hostile: the file is gone — say so, open nothing. +#[gpui::test] +async fn open_log_of_a_missing_file_says_so_and_opens_nothing(cx: &mut TestAppContext) { + let scratch = Scratch::new("missing"); + let path = seed(&scratch.root(), "t1 output\n", cx).await; + let mut panels = open_panels(cx).await; + // A FakeFs copy exists, so only the real-disk check can refuse. + seed_fake_copy(&mut panels, &path, "t1 output\n").await; + std::fs::remove_file(&path).unwrap(); + let before = active_editor(&mut panels); + + open_log(&mut panels, "t1"); + + let panel = panels.tasks_panel.clone(); + assert_eq!( + notice_of(&panel, &sid("s1"), "t1", &mut panels.cx).as_deref(), + Some("Log no longer available") + ); + assert_eq!(active_editor(&mut panels), before, "nothing opened"); +} diff --git a/crates/agent_tasks_panel/src/store.rs b/crates/agent_tasks_panel/src/store.rs index fc6329b..cdbb4a8 100644 --- a/crates/agent_tasks_panel/src/store.rs +++ b/crates/agent_tasks_panel/src/store.rs @@ -1,9 +1,13 @@ //! `TaskHistoryStore`: the agent task history in memory, and the single writer of its domain. //! //! The store is fed only by the `TasksCard`s registered with it (one per workspace's agent -//! panel): their `TaskActivity` events, and their `ThreadTitleChanged` events, which move every -//! record of a thread to its newest title. Every change updates memory first and then the -//! database, so a failed write leaves this session's view intact (R1.6). +//! panel): their `TaskActivity` events, their `ThreadTitleChanged` events, which move every +//! record of a thread to its newest title, and their `TaskLogSaved` events, which record the +//! size of a task's saved log. Every change updates memory first and then the database, so a +//! failed write leaves this session's view intact (R1.6). +//! +//! A task's log file follows its record (story 041): deleting a record deletes its file once the +//! row is gone, and the first load sweeps every file that belongs to no record. //! //! Startup order matters (R1.5): no task outlives the agent process that ran it, so every row //! still `running` is swept to `interrupted` before anything else touches the domain. Activity @@ -11,19 +15,25 @@ //! them — otherwise a task started in the first frame would be swept, or overwritten by the //! load. -use std::{cmp, sync::atomic::Ordering}; +use std::{ + cmp, + path::{Path, PathBuf}, + sync::atomic::Ordering, + time::SystemTime, +}; use acp_thread::{AgentTask, AgentTaskId}; use agent_client_protocol::v2::schema as acp_v2; use agent_ui::{ - TaskActivity, TaskLabels, TaskRecord, TasksCard, ThreadTitleChanged, normalize_user_label, + TaskActivity, TaskLabels, TaskRecord, TasksCard, TasksCardEvent, ThreadTitleChanged, + normalize_user_label, task_log, }; use anyhow::Result; use collections::{HashMap, HashSet}; use futures::{StreamExt as _, channel::mpsc}; use gpui::{ - App, AppContext as _, BorrowAppContext as _, Context, Entity, EntityId, Global, SharedString, - Subscription, Task, WeakEntity, + App, AppContext as _, AsyncApp, BorrowAppContext as _, Context, Entity, EntityId, Global, + SharedString, Subscription, Task, WeakEntity, }; use crate::{AgentTaskHistoryDb, HistoryEntry}; @@ -63,6 +73,7 @@ struct RegisteredCard { card: WeakEntity, _activity: Subscription, _titles: Subscription, + _card_events: Subscription, _changes: Subscription, } @@ -70,6 +81,15 @@ struct RegisteredCard { enum Heard { Activity(TaskActivity), Title(ThreadTitleChanged), + /// A task's log was saved (`Some(size)`), or the task keeps none (`None`). + LogSaved { + key: TaskKey, + bytes: Option, + logs_root: PathBuf, + /// When the event ARRIVED: one heard while the history loads waits in `pending`, and + /// stamping it when applied would make a stale "no log" look newer than a later save. + heard_at: SystemTime, + }, } /// A task's key in the history: the pair, since a task id is unique only within its session. @@ -85,9 +105,10 @@ enum HistoryWrite { keys: Vec, archived: bool, }, - /// Archived records the user deleted (story 025). + /// Archived records the user deleted (story 025), and then their log files (story 041). Deleted { keys: Vec, + logs_root: PathBuf, }, Titled(ThreadTitleChanged), Labeled { @@ -99,15 +120,38 @@ enum HistoryWrite { key: TaskKey, tool_call_id: SharedString, }, + /// The size of a task's saved log; `None` clears it, then deletes the file (story 041). + LogSaved { + key: TaskKey, + bytes: Option, + logs_root: PathBuf, + /// When the store heard it: a `None` never deletes a file saved after this. + heard_at: SystemTime, + }, } impl HistoryWrite { - async fn run(self, db: &AgentTaskHistoryDb) { + async fn run(self, db: &AgentTaskHistoryDb, cx: &AsyncApp) { let result = match &self { Self::Started(record) => db.insert_started(record).await, Self::Ended(record) => db.update_ended(record).await, Self::Archived { keys, archived } => db.set_archived(keys, *archived).await, - Self::Deleted { keys } => db.delete(keys).await, + // Row first, then the file: a crash in between leaves an orphan file, which the next + // load sweeps, never a record pointing at a missing log. A failed row delete keeps + // the record in the database, so it keeps its file too. + // Only the rows the database really removed lose their file: a record whose archive + // write had failed is still unarchived there, survives the delete, and keeps its log. + Self::Deleted { keys, logs_root } => match db.delete(keys).await { + Ok(removed) => { + let removed: Vec = removed + .into_iter() + .map(|index| keys[index].clone()) + .collect(); + delete_log_files(&removed, logs_root, cx).await; + Ok(()) + } + Err(error) => Err(error), + }, Self::Titled(change) => { db.update_thread_title(&change.session_id, &change.title) .await @@ -120,6 +164,18 @@ impl HistoryWrite { db.fill_tool_call_id(session_id, task_id, tool_call_id) .await } + Self::LogSaved { + key: (session_id, task_id), + bytes, + logs_root, + heard_at, + } => { + let result = db.set_log_bytes(session_id, task_id, *bytes).await; + if result.is_ok() && bytes.is_none() { + delete_stale_log_file(session_id, task_id, logs_root, *heard_at, cx).await; + } + result + } }; if let Err(error) = result { log::error!( @@ -148,7 +204,7 @@ impl HistoryWrite { }; format!("the {action} of {}", describe_keys(keys)) } - Self::Deleted { keys } => format!("the deletion of {}", describe_keys(keys)), + Self::Deleted { keys, .. } => format!("the deletion of {}", describe_keys(keys)), Self::Titled(change) => format!("the new title of session {}", change.session_id), Self::Labeled { key: (session_id, task_id), @@ -160,10 +216,67 @@ impl HistoryWrite { } => format!( "the tool call {tool_call_id} that started task {task_id} in session {session_id}" ), + Self::LogSaved { + key: (session_id, task_id), + .. + } => format!("the log size of task {task_id} in session {session_id}"), } } } +/// Deletes the log file of every key, off the foreground thread. A failure is logged with its +/// ids and leaves an orphan file for the next load's sweep. +/// A "no log" event's file delete, which leaves a file saved after the event: a later Open Log +/// may already have written a fresh copy while this write waited in the queue. +async fn delete_stale_log_file( + session_id: &acp_v2::SessionId, + task_id: &AgentTaskId, + logs_root: &Path, + heard_at: SystemTime, + cx: &AsyncApp, +) { + let (session_id, task_id) = (session_id.to_string(), task_id.to_string()); + let logs_root = logs_root.to_path_buf(); + cx.background_spawn(async move { + match task_log::delete_log_unless_newer(&logs_root, &session_id, &task_id, heard_at) { + Ok(true) => { + log::info!("agent_task_log event=deleted session_id={session_id} task_id={task_id}") + } + Ok(false) => log::info!( + "agent_task_log event=kept session_id={session_id} task_id={task_id} \ + reason=newer_than_event" + ), + Err(error) => log::warn!( + "agent_task_log event=failed session_id={session_id} task_id={task_id} \ + reason=delete error={error:#}" + ), + } + }) + .await; +} + +async fn delete_log_files(keys: &[TaskKey], logs_root: &Path, cx: &AsyncApp) { + let keys: Vec<(String, String)> = keys + .iter() + .map(|(session_id, task_id)| (session_id.to_string(), task_id.to_string())) + .collect(); + let logs_root = logs_root.to_path_buf(); + cx.background_spawn(async move { + for (session_id, task_id) in keys { + match task_log::delete_log(&logs_root, &session_id, &task_id) { + Ok(()) => log::info!( + "agent_task_log event=deleted session_id={session_id} task_id={task_id}" + ), + Err(error) => log::warn!( + "agent_task_log event=failed session_id={session_id} task_id={task_id} \ + reason=delete error={error:#}" + ), + } + } + }) + .await; +} + /// Every key of a batch, naming its session and task ids. fn describe_keys(keys: &[TaskKey]) -> String { keys.iter() @@ -212,6 +325,9 @@ impl TaskHistoryStore { cx: &mut Context, ) -> Self { let (writes, mut queue) = mpsc::unbounded::(); + // A log saved after this instant is newer than the history being loaded: the sweep + // keeps it even when its record is not among the loaded ones. + let loading_began = SystemTime::now(); let writer = cx.spawn(async move |this, cx| { let loaded = if on_fallback { Err(SharedString::from( @@ -224,13 +340,15 @@ impl TaskHistoryStore { .map_err(|error| format!("{error:#}").into()) }; if this - .update(cx, |store, cx| store.finish_loading(loaded, cx)) + .update(cx, |store, cx| { + store.finish_loading(loaded, loading_began, cx) + }) .is_err() { return; } while let Some(write) = queue.next().await { - write.run(&db).await; + write.run(&db, cx).await; } }); Self { @@ -257,6 +375,27 @@ impl TaskHistoryStore { let titles = cx.subscribe(card, |store, _card, change: &ThreadTitleChanged, cx| { store.hear(Heard::Title(change.clone()), cx); }); + let card_events = cx.subscribe(card, |store, _card, event: &TasksCardEvent, cx| { + if let TasksCardEvent::TaskLogSaved { + session_id, + task_id, + bytes, + } = event + { + // Resolved here, on the foreground: background code never reads the real root. + let logs_root = task_log::logs_root_in(cx); + let key = (session_id.clone(), task_id.clone()); + store.hear( + Heard::LogSaved { + key, + bytes: *bytes, + logs_root, + heard_at: SystemTime::now(), + }, + cx, + ); + } + }); // Progress changes no record, but the panels of every window show it live (R2.3), so // they repaint on this store's notifications for any card's change. let changes = cx.observe(card, |_, _, cx| cx.notify()); @@ -271,6 +410,7 @@ impl TaskHistoryStore { card: card.downgrade(), _activity: activity, _titles: titles, + _card_events: card_events, _changes: changes, }, ); @@ -387,7 +527,11 @@ impl TaskHistoryStore { }); } self.deleted.extend(deleted.iter().cloned()); - self.queue_write(HistoryWrite::Deleted { keys: deleted }); + let logs_root = task_log::logs_root_in(cx); + self.queue_write(HistoryWrite::Deleted { + keys: deleted, + logs_root, + }); cx.notify(); } @@ -410,8 +554,10 @@ impl TaskHistoryStore { fn finish_loading( &mut self, loaded: Result, SharedString>, + loading_began: SystemTime, cx: &mut Context, ) { + let swept = loaded.is_ok(); match loaded { Ok(records) => { self.records = records; @@ -438,12 +584,67 @@ impl TaskHistoryStore { self.availability = Availability::Unavailable(cause); } } - for heard in self.pending.take().unwrap_or_default() { + let pending = self.pending.take().unwrap_or_default(); + // Saved while loading: kept by the sweep even if the store has no record for it. + let mut keep: std::collections::HashSet<(String, String)> = pending + .iter() + .filter_map(|heard| match heard { + Heard::LogSaved { + key: (session_id, task_id), + .. + } => Some((session_id.to_string(), task_id.to_string())), + Heard::Activity(_) | Heard::Title(_) => None, + }) + .collect(); + for heard in pending { self.apply(heard); } + if swept { + // Every loaded record, plus every one recorded since loading began. + keep.extend(self.records.iter().map(|entry| { + ( + entry.record.session_id.to_string(), + entry.record.task_id.to_string(), + ) + })); + self.sweep_orphan_logs(keep, loading_began, cx); + } cx.notify(); } + /// Removes every log file that belongs to no record (R6.6). Skipped on the in-memory + /// fallback: its history is empty, and the sweep would read that as "no record keeps a log". + fn sweep_orphan_logs( + &self, + keep: std::collections::HashSet<(String, String)>, + newer_than: SystemTime, + cx: &mut Context, + ) { + if !logs_may_be_swept( + db::ALL_FILE_DB_FAILED.load(Ordering::Acquire), + zed_stateless(), + self.availability == Availability::Available, + ) { + return; + } + let logs_root = task_log::logs_root_in(cx); + cx.background_spawn(async move { + match task_log::sweep(&logs_root, &keep, newer_than) { + Ok(report) => log::info!( + "agent_task_log event=swept removed={} kept={} other={}", + report.removed, + report.kept, + report.other + ), + Err(error) => log::warn!( + "agent_task_log event=failed reason=sweep root={} error={error:#}", + logs_root.display() + ), + } + }) + .detach(); + } + fn hear(&mut self, heard: Heard, cx: &mut Context) { if let Some(pending) = self.pending.as_mut() { pending.push(heard); @@ -458,9 +659,37 @@ impl TaskHistoryStore { Heard::Activity(TaskActivity::Started(record)) => self.record_started(record), Heard::Activity(TaskActivity::Ended(record)) => self.record_ended(record), Heard::Title(change) => self.record_title(change), + Heard::LogSaved { + key, + bytes, + logs_root, + heard_at, + } => self.record_log_saved(key, bytes, logs_root, heard_at), } } + /// Records the size of a task's saved log, or that it has none (R6.1, R6.7). Memory first, + /// then the write; a task with no record (deleted this session, or never recorded) is left + /// alone, and a file the card wrote for it is swept at the next load. + fn record_log_saved( + &mut self, + key: TaskKey, + bytes: Option, + logs_root: PathBuf, + heard_at: SystemTime, + ) { + let Some(index) = self.position(&key.0, &key.1) else { + return; + }; + self.records[index].log_bytes = bytes; + self.queue_write(HistoryWrite::LogSaved { + key, + bytes, + logs_root, + heard_at, + }); + } + /// A task seen for the first time is recorded with whatever it carries — its outcome too, /// when first seen ended (R1.1, R1.8). A key already recorded is left alone: a repeated /// snapshot (R1.3), or a re-opened session replaying `Started`, never rewrites a record nor @@ -484,6 +713,7 @@ impl TaskHistoryStore { record: record.clone(), archived: false, label: None, + log_bytes: None, }); self.queue_write(HistoryWrite::Started(record)); } @@ -609,6 +839,35 @@ fn newest_first(a: &TaskRecord, b: &TaskRecord) -> cmp::Ordering { .then_with(|| a.task_id.cmp(&b.task_id)) } +/// Whether the orphan sweep may run. An in-memory history — the file database failed, or Zeo +/// runs with `ZED_STATELESS`, which opens in-memory databases WITHOUT setting +/// `ALL_FILE_DB_FAILED` — holds no record of the logs on disk, and sweeping against it would +/// delete every one of them. +fn logs_may_be_swept(all_file_db_failed: bool, stateless: bool, available: bool) -> bool { + !all_file_db_failed && !stateless && available +} + +/// `ZED_STATELESS` as `zed_env_vars` reads it (set and non-empty), without depending on that crate. +fn zed_stateless() -> bool { + std::env::var_os("ZED_STATELESS").is_some_and(|value| !value.is_empty()) +} + +#[cfg(test)] +mod sweep_guard_tests { + use super::logs_may_be_swept; + + #[test] + fn store_log_sweep_runs_only_against_a_persistent_available_history() { + assert!(logs_may_be_swept(false, false, true)); + assert!( + !logs_may_be_swept(true, false, true), + "file database failed" + ); + assert!(!logs_may_be_swept(false, true, true), "ZED_STATELESS"); + assert!(!logs_may_be_swept(false, false, false), "not available"); + } +} + #[cfg(test)] #[path = "store/lifecycle_tests.rs"] mod lifecycle; @@ -640,3 +899,8 @@ mod tool_call_id; #[cfg(test)] #[path = "store/delete_tests.rs"] mod delete; + +#[cfg(test)] +#[path = "store/log_tests.rs"] +// Not `mod log`: that would shadow the `log` crate for this file's `log::` macros. +mod log_files; diff --git a/crates/agent_tasks_panel/src/store/log_tests.rs b/crates/agent_tasks_panel/src/store/log_tests.rs new file mode 100644 index 0000000..a64f9e2 --- /dev/null +++ b/crates/agent_tasks_panel/src/store/log_tests.rs @@ -0,0 +1,292 @@ +// Story 041, task 5.2 (store half) — a task's log follows its history record: the size recorded +// on `TaskLogSaved`, the file deleted with the record, orphans swept when the history loads, and +// no sweep on the in-memory fallback (R6.1, R6.5, R6.6; Q4). Module path: `store::log`. Wired in +// `src/store.rs` as +// +// #[cfg(test)] +// #[path = "store/log_tests.rs"] +// mod log; +// +// Every test name starts with `log_` (filter `cargo test -p agent_tasks_panel log_`). +// +// Contract (design.md §Task history, §`task_log`): `TasksCardEvent::TaskLogSaved { session_id, +// task_id, bytes: u64 }`; `HistoryEntry::log_bytes`; `delete_archived` / `delete_all_archived` +// delete the row, then the file; `finish_loading` sweeps with every loaded key kept, and skips the +// sweep on the fallback. `agent_ui::task_log::{log_path, write_log}` re-exported for this crate. +// +// Contract assumed beyond design.md (the test seam, also used by the card and the panel tests): +// the log root is read through `agent_ui::task_log::logs_root_in(cx)`, which returns the +// `agent_ui::task_log::LogsRootOverride(PathBuf)` gpui global when set and `logs_root()` otherwise. +// Production never sets it; tests set it so nothing touches the operator's data directory. If the +// implementation picks another seam, change `use_scratch_logs_root` only. +// +// Hostile halves first: deleting (s1, t1) must leave (s1, t10)'s and (s2, t1)'s files; a delete +// whose row write FAILS must delete no file; the sweep must keep every loaded record's file while +// removing orphans, and must remove NOTHING when the history is the in-memory fallback — an empty +// fallback history would otherwise delete every log. + +use std::path::{Path, PathBuf}; + +use acp_thread::{AgentTaskId, AgentTaskStatus}; +use agent_ui::TasksCardEvent; +use agent_ui::task_log::{LogsRootOverride, log_path, write_log}; +use gpui::{AppContext as _, Entity, TestAppContext, VisualTestContext}; + +use crate::test_helpers::*; +use crate::{AgentTaskHistoryDb, HistoryEntry, TaskHistoryStore}; + +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-store-log-{name}-{}-{nanos}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } + fn root(&self) -> PathBuf { + self.0.join("agent_task_logs") + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn use_scratch_logs_root(root: &Path, cx: &mut TestAppContext) { + let root = root.to_path_buf(); + cx.update(|cx| cx.set_global(LogsRootOverride(root))); +} + +fn put_log(root: &Path, session: &str, task: &str) -> PathBuf { + let path = log_path(root, session, task); + write_log(&path, "log text\n").expect("seed log"); + path +} + +/// A store over `name`'s database holding `finished` tasks, the ones in `archived` archived, and +/// registered with the rig's real card. `on_fallback` stands for `db::ALL_FILE_DB_FAILED`. +async fn seeded_store( + name: &'static str, + finished: &[(&str, &str)], + archived: &[(&str, &str)], + on_fallback: bool, + root: &Path, + cx: &mut TestAppContext, +) -> (AgentTaskHistoryDb, Entity, CardRig) { + let mut rig = card_rig(cx).await; + use_scratch_logs_root(root, cx); + let db = AgentTaskHistoryDb::open_test_db(name).await; + for (index, (session, task)) in finished.iter().enumerate() { + let at = 1_000 + index as i64 * 100; + db.insert_started(&record(session, task, AgentTaskStatus::Running, at)) + .await + .unwrap(); + db.update_ended(&record(session, task, AgentTaskStatus::Completed, at)) + .await + .unwrap(); + } + let keys: Vec<_> = archived.iter().map(|(s, t)| key(s, t)).collect(); + db.set_archived(&keys, true).await.unwrap(); + let store = rig + .cx + .new(|cx| TaskHistoryStore::with_fallback_flag(db.clone(), on_fallback, cx)); + rig.cx.run_until_parked(); + let card = rig.card.clone(); + store.update(&mut rig.cx, |store, cx| store.register_card(&card, cx)); + rig.cx.run_until_parked(); + (db, store, rig) +} + +fn entries(store: &Entity, cx: &VisualTestContext) -> Vec { + store.read_with(cx, |store, _| store.records().to_vec()) +} + +fn saved( + card: &Entity, + session: &str, + task: &str, + bytes: u64, + cx: &mut VisualTestContext, +) { + card.update(cx, |_, cx| { + cx.emit(TasksCardEvent::TaskLogSaved { + session_id: sid(session), + task_id: AgentTaskId::from(task.to_string()), + bytes: Some(bytes), + }) + }); + cx.run_until_parked(); +} + +// ── TaskLogSaved (R6.1) ──────────────────────────────────────────────────── + +#[gpui::test] +async fn log_saved_records_the_size_in_memory_and_in_the_database(cx: &mut TestAppContext) { + let scratch = Scratch::new("saved"); + let (db, store, mut rig) = seeded_store( + "store_log_saved", + &[("s1", "t1"), ("s1", "t10")], + &[], + false, + &scratch.root(), + cx, + ) + .await; + let card = rig.card.clone(); + + saved(&card, "s1", "t1", 4_242, &mut rig.cx); + + let view = entries(&store, &rig.cx); + assert_eq!(find(&view, "s1", "t1").unwrap().log_bytes, Some(4_242)); + assert_eq!( + find(&view, "s1", "t10").unwrap().log_bytes, + None, + "only its own record" + ); + let stored = load(&db).await; + assert_eq!(find(&stored, "s1", "t1").unwrap().log_bytes, Some(4_242)); +} + +// ── Delete (R6.5) ────────────────────────────────────────────────────────── + +// Hostile (wrong collapse): deleting (s1, t1) leaves the look-alikes' files. +#[gpui::test] +async fn log_delete_archived_removes_the_records_file_and_only_that_file(cx: &mut TestAppContext) { + let scratch = Scratch::new("delete"); + let root = scratch.root(); + let (_db, store, mut rig) = seeded_store( + "store_log_delete", + &[("s1", "t1"), ("s1", "t10"), ("s2", "t1")], + &[("s1", "t1"), ("s1", "t10"), ("s2", "t1")], + false, + &root, + cx, + ) + .await; + let own = put_log(&root, "s1", "t1"); + let sibling = put_log(&root, "s1", "t10"); + let other_session = put_log(&root, "s2", "t1"); + + store.update(&mut rig.cx, |store, cx| { + store.delete_archived(&[key("s1", "t1")], cx) + }); + rig.cx.run_until_parked(); + + assert!(find(&entries(&store, &rig.cx), "s1", "t1").is_none()); + assert!(!own.exists(), "the deleted record's log is gone"); + assert!(sibling.exists()); + assert!(other_session.exists()); +} + +#[gpui::test] +async fn log_delete_all_archived_removes_every_archived_records_file(cx: &mut TestAppContext) { + let scratch = Scratch::new("delete-all"); + let root = scratch.root(); + let (_db, store, mut rig) = seeded_store( + "store_log_delete_all", + &[("s1", "t1"), ("s1", "t2"), ("s1", "kept")], + &[("s1", "t1"), ("s1", "t2")], + false, + &root, + cx, + ) + .await; + let archived = [put_log(&root, "s1", "t1"), put_log(&root, "s1", "t2")]; + let listed = put_log(&root, "s1", "kept"); + + store.update(&mut rig.cx, |store, cx| store.delete_all_archived(cx)); + rig.cx.run_until_parked(); + + for path in &archived { + assert!(!path.exists(), "{path:?}"); + } + assert!( + listed.exists(), + "a record that is not archived keeps its log" + ); +} + +// Hostile: the row write fails — the record stays, so its file must stay too. +#[gpui::test] +async fn log_failed_row_delete_deletes_no_file(cx: &mut TestAppContext) { + let scratch = Scratch::new("delete-fails"); + let root = scratch.root(); + let (db, store, mut rig) = seeded_store( + "store_log_delete_fails", + &[("s1", "t1")], + &[("s1", "t1")], + false, + &root, + cx, + ) + .await; + let own = put_log(&root, "s1", "t1"); + drop_history_table(&db).await; + + store.update(&mut rig.cx, |store, cx| { + store.delete_archived(&[key("s1", "t1")], cx) + }); + rig.cx.run_until_parked(); + + assert!( + own.exists(), + "a delete that did not reach the database leaves the file" + ); +} + +// ── Sweep at load (R6.6) ─────────────────────────────────────────────────── + +#[gpui::test] +async fn log_load_sweeps_orphans_and_keeps_every_loaded_records_log(cx: &mut TestAppContext) { + let scratch = Scratch::new("sweep"); + let root = scratch.root(); + let kept = [put_log(&root, "s1", "t1"), put_log(&root, "s/2", "a/b")]; + let orphans = [put_log(&root, "s1", "gone"), put_log(&root, "s9", "t1")]; + + let (_db, _store, _rig) = seeded_store( + "store_log_sweep", + &[("s1", "t1"), ("s/2", "a/b")], + &[("s/2", "a/b")], // archived records are loaded records too + false, + &root, + cx, + ) + .await; + + for path in &kept { + assert!(path.exists(), "{path:?} belongs to a record"); + } + for path in &orphans { + assert!(!path.exists(), "{path:?} belongs to no record"); + } +} + +// Hostile: the in-memory fallback loads nothing; the sweep must not read that as "no records". +#[gpui::test] +async fn log_no_sweep_on_the_in_memory_fallback(cx: &mut TestAppContext) { + let scratch = Scratch::new("fallback"); + let root = scratch.root(); + let logs = [put_log(&root, "s1", "t1"), put_log(&root, "s2", "t2")]; + + let (_db, store, rig) = seeded_store("store_log_fallback", &[], &[], true, &root, cx).await; + + assert!( + store.read_with(&rig.cx, |store, _| matches!( + store.availability(), + super::Availability::Unavailable(_) + )), + "the store is on the fallback" + ); + for path in &logs { + assert!(path.exists(), "{path:?} survives a fallback session"); + } +} diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index e6bfe88..3b14c12 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -1580,6 +1580,30 @@ impl AgentPanel { window, cx, ), + TasksCardEvent::OpenLog { + session_id, + task_id, + path, + } => panel.open_log_from_card( + session_id.clone(), + task_id.clone(), + path.clone(), + window, + cx, + ), + TasksCardEvent::OpenSubagent { + session_id, + task_id, + tool_call_id, + } => panel.open_subagent_from_card( + session_id.clone(), + task_id.clone(), + tool_call_id.as_ref(), + window, + cx, + ), + // The task history records it; the panel has nothing to do. + TasksCardEvent::TaskLogSaved { .. } => {} }, ); @@ -1836,6 +1860,16 @@ impl AgentPanel { else { return; }; + self.show_card_open_failure(session_id, task_id, error, cx); + } + + fn show_card_open_failure( + &mut self, + session_id: acp_v2::SessionId, + task_id: acp_thread::AgentTaskId, + error: anyhow::Error, + cx: &mut Context, + ) { let message = format!("{error:#}"); log::error!( "agent panel: opening task {task_id} of session {session_id} from the tasks card \ @@ -1846,6 +1880,184 @@ impl AgentPanel { }); } + /// Shows a subagent task's own thread from the tasks card (R5.5). When the root + /// thread names no thread of its own for the task's tool call, opens the root + /// thread at that call through `open_task_from_card`'s path and says so on the + /// row (R5.6); a failure is shown on the row instead. + fn open_subagent_from_card( + &mut self, + session_id: acp_v2::SessionId, + task_id: acp_thread::AgentTaskId, + tool_call_id: Option<&acp_v2::ToolCallId>, + window: &mut Window, + cx: &mut Context, + ) { + match self.show_subagent_thread(&session_id, tool_call_id, window, cx) { + Ok(Some(subagent_session_id)) => { + log::info!( + "agent panel: opened subagent session {subagent_session_id} of task \ + {task_id} in session {session_id} from the tasks card" + ); + return; + } + Ok(None) => {} + Err(error) => { + self.show_card_open_failure(session_id, task_id, error, cx); + return; + } + } + if let Err(error) = self.open_thread_at_tool_call(&session_id, tool_call_id, window, cx) { + self.show_card_open_failure(session_id, task_id, error, cx); + return; + } + log::info!( + "agent panel: task {task_id} of session {session_id} has no subagent thread to \ + show; opened its parent thread at tool call {tool_call_id:?}" + ); + self.tasks_card.update(cx, |card, cx| { + card.set_row_error( + (session_id, task_id), + SUBAGENT_WITHOUT_THREAD_NOTICE.into(), + cx, + ) + }); + } + + /// Brings forward the thread the root thread of `session_id` names on its tool + /// call `tool_call_id` as the subagent's own, and returns that thread's session. + /// `Ok(None)` when there is none to show: no tool call id, no such root entry, + /// no session named on it, or a named session this conversation never loaded. + fn show_subagent_thread( + &mut self, + session_id: &acp_v2::SessionId, + tool_call_id: Option<&acp_v2::ToolCallId>, + window: &mut Window, + cx: &mut Context, + ) -> Result> { + let Some(tool_call_id) = tool_call_id else { + return Ok(None); + }; + let Some(conversation) = self.held_conversation_view(session_id, cx) else { + // `open_thread_at_tool_call` reports a thread this panel does not hold. + return Ok(None); + }; + // Only the task's own tool call names its session: a subagent without one + // never borrows another call's (R5.6). + let Some(subagent_session_id) = conversation.read(cx).root_thread(cx).and_then(|root| { + let root = root.read(cx); + let (_, call) = root.tool_call(tool_call_id)?; + Some(call.subagent_session_info.as_ref()?.session_id.clone()) + }) else { + return Ok(None); + }; + if conversation + .read(cx) + .thread_view(&subagent_session_id) + .is_none() + { + log::debug!( + "agent panel: subagent session {subagent_session_id} of tool call \ + {tool_call_id} in session {session_id} is not loaded in its conversation" + ); + return Ok(None); + } + self.open_thread_by_session(session_id.clone(), window, cx); + let conversation = self + .active_conversation_view() + .filter(|active| **active == conversation) + .cloned() + .with_context(|| format!("the thread of session {session_id} did not come forward"))?; + conversation.update(cx, |conversation, cx| { + conversation.navigate_to_thread(subagent_session_id.clone(), window, cx) + }); + Ok(Some(subagent_session_id)) + } + + /// Opens a task's log, just saved by the tasks card, read-only in this panel's + /// workspace (R5.2); a failure is shown on the task's row. + fn open_log_from_card( + &mut self, + session_id: acp_v2::SessionId, + task_id: acp_thread::AgentTaskId, + path: PathBuf, + window: &mut Window, + cx: &mut Context, + ) { + let card = self.tasks_card.downgrade(); + let fail = move |error: anyhow::Error, + session_id: acp_v2::SessionId, + task_id: acp_thread::AgentTaskId, + cx: &mut App| { + log::warn!( + "agent_task_log event=failed session_id={session_id} task_id={task_id} \ + reason=open_tab error={error:#}" + ); + card.update(cx, |card, cx| { + card.set_row_error( + (session_id, task_id), + format!("Could not open the log: {error:#}").into(), + cx, + ) + }) + .ok(); + }; + let Some(workspace) = self.workspace.upgrade() else { + fail( + anyhow!("the workspace is no longer open"), + session_id, + task_id, + cx, + ); + return; + }; + // "Not local" covers both SSH and collab projects: in either, the open would resolve + // the local log path on another host (R5.10). + if !workspace.read(cx).project().read(cx).is_local() { + log::info!( + "agent_task_log event=refused session_id={session_id} task_id={task_id} \ + reason=remote_project" + ); + self.tasks_card.update(cx, |card, cx| { + card.set_row_error( + (session_id, task_id), + crate::task_log::REMOTE_PROJECT_NOTICE.into(), + cx, + ) + }); + return; + } + let open = workspace.update(cx, |workspace, cx| { + crate::open_abs_path_at_point_task(workspace, path.clone(), None, window, cx) + }); + cx.spawn(async move |_, cx| { + let item = match open.await { + Ok(Some(item)) => item, + Ok(None) => { + let error = anyhow!("no file at {}", path.display()); + cx.update(|cx| fail(error, session_id, task_id, cx)); + return; + } + Err(error) => { + cx.update(|cx| fail(error, session_id, task_id, cx)); + return; + } + }; + let Some(editor) = item.downcast::() else { + log::warn!( + "agent_task_log event=failed session_id={session_id} task_id={task_id} \ + reason=open_tab error=the log did not open in an editor, so it is not \ + read-only" + ); + return; + }; + editor.update(cx, |editor, cx| { + editor.set_read_only(true); + cx.notify(); + }); + }) + .detach(); + } + fn open_thread_at_tool_call( &mut self, session_id: &acp_v2::SessionId, @@ -7382,6 +7594,11 @@ impl AgentPanel { } } +/// What a subagent row says when its thread was asked for and its parent's opened +/// instead (R5.6). A notice, not an error, shown in the row's message slot. +const SUBAGENT_WITHOUT_THREAD_NOTICE: &str = + "Opened the parent thread; this subagent has no thread of its own to show"; + /// Scrolls `conversation`'s root thread to the entry of `tool_call_id` and /// returns its index, or `None` when the root thread has no such entry: a nested /// subagent's tool call lives in its own thread, not among the root's entries. diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index 64cf53f..4b98f0a 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -95,7 +95,11 @@ pub use crate::message_editor::MessageEditorEvent; pub use crate::task_label::{ TASK_LABEL_MAX_CHARS, TaskLabels, collapse_label, display_label, normalize_user_label, }; -pub use crate::tasks_card::{TaskActivity, TaskKey, TaskRecord, TasksCard, ThreadTitleChanged}; +/// Story 041: where a task's saved log lives; the Agent Tasks panel and its store use it too. +pub use crate::tasks_card::task_log; +pub use crate::tasks_card::{ + TaskActivity, TaskKey, TaskRecord, TasksCard, TasksCardEvent, ThreadTitleChanged, +}; pub use crate::thread_metadata_store::ThreadId; pub use agent_diff::{AgentDiffPane, AgentDiffToolbar}; pub use conversation_view::open_markdown_in_workspace; @@ -214,53 +218,60 @@ pub(crate) fn open_abs_path_at_point( window: &mut Window, cx: &mut Context, ) { + open_abs_path_at_point_task(workspace, abs_path, point, window, cx).detach_and_log_err(cx); +} + +/// Like [`open_abs_path_at_point`], for a caller that needs the opened item: it resolves +/// to `None` when nothing was opened (no file outside every worktree, or the workspace +/// went away), and to an error when opening failed. +pub fn open_abs_path_at_point_task( + workspace: &mut Workspace, + abs_path: PathBuf, + point: Option, + window: &mut Window, + cx: &mut Context, +) -> gpui::Task>>> { let project_path = workspace.project().read_with(cx, |project, cx| { project_path_for_file_link(project, &abs_path, cx) }); let project = workspace.project().downgrade(); let workspace = cx.weak_entity(); - window - .spawn(cx, async move |cx| { - let task = if let Some(project_path) = project_path { - workspace - .update_in(cx, |workspace, window, cx| { - workspace.open_path(project_path, None, true, window, cx) - }) - .ok() - } else { - let path = abs_path - .to_str() - .context("file link path is not valid UTF-8")?; - let Ok(task) = - project.update(cx, |project, cx| project.resolve_abs_file_link(path, cx)) - else { - return Ok(()); - }; - let Some(resolved_path) = task - .await - .with_context(|| format!("resolving agent file link {abs_path:?}"))? - else { - log::warn!( - "Could not resolve agent file link to {abs_path:?}: no matching file" - ); - return Ok(()); - }; - workspace - .update_in(cx, |workspace, window, cx| { - workspace.open_resolved_path(resolved_path, window, cx) - }) - .ok() - }; - let Some(task) = task else { - return Ok(()); - }; - let item = task.await?; - let Some(point) = point else { - return Ok(()); + window.spawn(cx, async move |cx| { + let task = if let Some(project_path) = project_path { + workspace + .update_in(cx, |workspace, window, cx| { + workspace.open_path(project_path, None, true, window, cx) + }) + .ok() + } else { + let path = abs_path + .to_str() + .context("file link path is not valid UTF-8")?; + let Ok(task) = + project.update(cx, |project, cx| project.resolve_abs_file_link(path, cx)) + else { + return Ok(None); }; - let Some(editor) = item.downcast::() else { - return Ok(()); + let Some(resolved_path) = task + .await + .with_context(|| format!("resolving agent file link {abs_path:?}"))? + else { + log::warn!("Could not resolve agent file link to {abs_path:?}: no matching file"); + return Ok(None); }; + workspace + .update_in(cx, |workspace, window, cx| { + workspace.open_resolved_path(resolved_path, window, cx) + }) + .ok() + }; + let Some(task) = task else { + return Ok(None); + }; + let item = task.await?; + if let Some(point) = point + && let Some(editor) = item.downcast::() + { editor .update_in(cx, |editor, window, cx| { if let Some(buffer) = editor.buffer().read(cx).as_singleton() { @@ -273,9 +284,9 @@ pub(crate) fn open_abs_path_at_point( } }) .ok(); - anyhow::Ok(()) - }) - .detach_and_log_err(cx); + } + Ok(Some(item)) + }) } pub const DEFAULT_THREAD_TITLE: &str = "New Agent Thread"; diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index 02c974e..718025b 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -2751,6 +2751,19 @@ impl ConversationView { .detach_and_log_err(cx); } + /// Loads a subagent's session the way opening its parent's thread does. Only the native + /// agent announces a subagent while a turn runs (`SubagentSpawned` needs a registered + /// child), so a fixture that adds the spawning tool call to a live thread loads it here. + #[cfg(test)] + pub(crate) fn load_subagent_session_for_test( + &mut self, + subagent_id: acp_v2::SessionId, + parent_session_id: acp_v2::SessionId, + cx: &mut Context, + ) { + self.load_subagent_session(subagent_id, parent_session_id, cx); + } + fn spawn_external_agent_login( login: task::SpawnInTerminal, workspace: Entity, diff --git a/crates/agent_ui/src/tasks_card.rs b/crates/agent_ui/src/tasks_card.rs index babff64..12858c7 100644 --- a/crates/agent_ui/src/tasks_card.rs +++ b/crates/agent_ui/src/tasks_card.rs @@ -13,6 +13,7 @@ use std::{ cmp::Reverse, + path::PathBuf, time::{Duration, Instant, SystemTime, UNIX_EPOCH}, }; @@ -32,6 +33,7 @@ use crate::DEFAULT_THREAD_TITLE; use crate::conversation_view::ConversationView; use crate::task_label::{TaskLabels, collapse_label, display_label}; +pub mod task_log; mod tray; #[cfg(test)] @@ -73,6 +75,22 @@ mod background; #[path = "tasks_card/row_error_tests.rs"] mod row_error; +#[cfg(test)] +#[path = "tasks_card/expand_tests.rs"] +mod expand; + +#[cfg(test)] +#[path = "tasks_card/open_log_tests.rs"] +mod open_log; + +#[cfg(test)] +#[path = "tasks_card/open_log_remote_tests.rs"] +mod open_log_remote; + +#[cfg(test)] +#[path = "tasks_card/open_subagent_tests.rs"] +mod open_subagent; + #[cfg(test)] #[path = "tasks_card/open_tests.rs"] mod open; @@ -137,6 +155,29 @@ pub enum TasksCardEvent { task_id: AgentTaskId, tool_call_id: Option, }, + /// A task's log was saved (`Some(size in bytes)`), or its save failed and the task keeps + /// no log (`None`). The task history records the size, and on `None` deletes the file. + TaskLogSaved { + session_id: acp_v2::SessionId, + task_id: AgentTaskId, + bytes: Option, + }, + /// Open the task's log, just saved at `path`, read-only (R5.2). A failure comes + /// back through `set_row_error` on the task's row. + OpenLog { + session_id: acp_v2::SessionId, + task_id: AgentTaskId, + path: PathBuf, + }, + /// Show a subagent task's own thread, which the root thread names on the tool + /// call that spawned it (R5.5). When it names none, the task's thread opens at + /// that tool call instead and the row says so (R5.6); a failure comes back + /// through `set_row_error` on the task's row. + OpenSubagent { + session_id: acp_v2::SessionId, + task_id: AgentTaskId, + tool_call_id: Option, + }, } impl EventEmitter for TasksCard {} @@ -235,6 +276,11 @@ pub struct ThreadGroup { pub struct TaskRow { pub task: AgentTask, pub watched: bool, + /// Opened with its disclosure, or watched: a watched row is always expanded (R4.6). + pub expanded: bool, + /// The label of the root thread's tool call that started the task, else its + /// description (R4.1). + pub running_what: SharedString, pub error: Option, } @@ -245,10 +291,15 @@ pub struct TasksCard { minimized: bool, /// In memory only, so a restart starts with nothing watched (R3.3). watched: HashSet, + /// Rows opened with their disclosure; in memory only, like `watched` (R4.8). + expanded: HashSet, /// The final copy of each task whose ending reached the card, until it expires. ended: HashMap, /// The last stop or background error of a row (R4.3, R4.7). row_errors: HashMap, + /// Tasks whose log this card saved: a later failed copy on end must tell the + /// history to drop that log, which is older than the task's end (R6.7). + logs_saved: HashSet, /// Counts endings as they reach the card, to order ended-only groups. endings_seen: u64, /// Endings already weighed for the tray. A thread held through two views @@ -288,10 +339,29 @@ pub struct TasksCard { /// A per-row control that takes keyboard focus. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] enum RowControl { + Expand, Watch, Stop, Background, Open, + OpenLog, + OpenThread, +} + +/// Why a task's log is being saved: the operator asked to open it, or its task ended. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum LogSave { + Open, + CopyOnEnd, +} + +impl LogSave { + fn reason(self) -> &'static str { + match self { + LogSave::Open => "open_log", + LogSave::CopyOnEnd => "copy_on_end", + } + } } /// An error shown on a row, with the task as it was when the error arrived: the @@ -339,8 +409,10 @@ impl TasksCard { views: Vec::new(), minimized: false, watched: HashSet::default(), + expanded: HashSet::default(), ended: HashMap::default(), row_errors: HashMap::default(), + logs_saved: HashSet::default(), endings_seen: 0, tray_weighed: HashSet::default(), recorded: HashMap::default(), @@ -437,7 +509,7 @@ impl TasksCard { let mut rows: Vec = running .into_iter() .chain(ended.into_iter().map(|ended| &ended.task)) - .map(|task| self.row(session_id, task.clone())) + .map(|task| self.row(thread, task.clone(), cx)) .collect(); let order = match (oldest_running, latest_ending) { @@ -522,6 +594,17 @@ impl TasksCard { &self.pill_focus_handle } + #[cfg(test)] + pub(crate) fn expand_focus_handle( + &self, + session_id: &acp_v2::SessionId, + task_id: &AgentTaskId, + ) -> Option { + self.row_focus_handles + .get(&((session_id.clone(), task_id.clone()), RowControl::Expand)) + .cloned() + } + #[cfg(test)] pub(crate) fn watch_focus_handle( &self, @@ -591,6 +674,21 @@ impl TasksCard { cx.notify(); } + /// Expands or collapses one thread's task row (R4.5). A watched row stays + /// expanded whatever this says, until it is unwatched (R4.6). + pub fn toggle_expanded( + &mut self, + session_id: acp_v2::SessionId, + task_id: AgentTaskId, + cx: &mut Context, + ) { + let key = (session_id, task_id); + if !self.expanded.remove(&key) { + self.expanded.insert(key); + } + cx.notify(); + } + /// Asks before stopping one thread's running task, naming it, and stops it only /// on an explicit Stop (R4.1, R4.2). A prompt dropped unanswered — its window /// closed — is a Cancel, never a Stop. @@ -783,6 +881,139 @@ impl TasksCard { }); } + /// Asks the hosting panel to show a subagent task's own thread (R5.5), or its + /// parent thread at the tool call that spawned it when it has none (R5.6). + pub fn open_subagent( + &mut self, + session_id: acp_v2::SessionId, + task_id: AgentTaskId, + _window: &mut Window, + cx: &mut Context, + ) { + let tool_call_id = self + .task_on_show(&(session_id.clone(), task_id.clone()), cx) + .and_then(|task| task.tool_call_id.clone()); + cx.emit(TasksCardEvent::OpenSubagent { + session_id, + task_id, + tool_call_id, + }); + } + + /// Fetches one thread's task output, saves it as the task's log, and asks the + /// hosting panel to open it read-only (R5.2). A failed fetch or save shows on + /// the row and opens nothing (R5.3). + pub fn open_log( + &mut self, + session_id: acp_v2::SessionId, + task_id: AgentTaskId, + _window: &mut Window, + cx: &mut Context, + ) { + let key = (session_id, task_id); + let Some(thread) = self.held_thread(&key.0) else { + log::warn!( + "agent_task_log event=failed session_id={} task_id={} reason=open_log \ + error=the thread is not held by the tasks card", + key.0, + key.1 + ); + self.set_row_error( + key, + "Could not open the log: the thread is no longer open".into(), + cx, + ); + return; + }; + self.save_log(key, &thread, LogSave::Open, cx); + } + + /// Fetches the task's output from its thread, then writes the log off the UI + /// thread; `finish_log_save` tells what came of it. + fn save_log( + &mut self, + key: TaskKey, + thread: &Entity, + purpose: LogSave, + cx: &mut Context, + ) { + let path = task_log::log_path(&task_log::logs_root_in(cx), &key.0.to_string(), &key.1); + let fetch = thread.update(cx, |thread, cx| thread.fetch_task_output(&key.1, cx)); + cx.spawn(async move |card, cx| { + let saved = match fetch.await { + Ok(output) => { + cx.background_spawn({ + let path = path.clone(); + async move { task_log::write_log(&path, &task_log::render_log(&output)) } + }) + .await + } + Err(error) => Err(error), + }; + card.update(cx, |card, cx| { + card.finish_log_save(key, path, purpose, saved, cx) + }) + .ok(); + }) + .detach(); + } + + fn finish_log_save( + &mut self, + key: TaskKey, + path: PathBuf, + purpose: LogSave, + saved: anyhow::Result, + cx: &mut Context, + ) { + let (session_id, task_id) = key.clone(); + let reason = purpose.reason(); + let bytes = match saved { + Ok(bytes) => bytes, + Err(error) => { + log::warn!( + "agent_task_log event=failed session_id={session_id} task_id={task_id} \ + reason={reason} error={error:#}" + ); + match purpose { + LogSave::Open => self.set_row_error( + key, + format!("Could not open the log: {error:#}").into(), + cx, + ), + // Nobody asked for this copy, so the row says nothing (R6.7). + LogSave::CopyOnEnd => { + if self.logs_saved.remove(&key) { + cx.emit(TasksCardEvent::TaskLogSaved { + session_id, + task_id, + bytes: None, + }); + } + } + } + return; + } + }; + log::info!( + "agent_task_log event=saved session_id={session_id} task_id={task_id} \ + bytes={bytes} mode=0600 reason={reason}" + ); + self.logs_saved.insert(key); + cx.emit(TasksCardEvent::TaskLogSaved { + session_id: session_id.clone(), + task_id: task_id.clone(), + bytes: Some(bytes), + }); + if purpose == LogSave::Open { + cx.emit(TasksCardEvent::OpenLog { + session_id, + task_id, + path, + }); + } + } + /// Shows `message` on a row until a snapshot changes its task (R4.3, R4.7). pub(crate) fn set_row_error( &mut self, @@ -795,15 +1026,21 @@ impl TasksCard { cx.notify(); } - fn row(&self, session_id: &acp_v2::SessionId, task: AgentTask) -> TaskRow { - let key = (session_id.clone(), task.id.clone()); + fn row(&self, thread: &AcpThread, task: AgentTask, cx: &App) -> TaskRow { + let key = (thread.session_id().clone(), task.id.clone()); TaskRow { watched: self.watched.contains(&key), + expanded: self.is_expanded(&key), + running_what: running_what(thread, &task, cx), error: self.row_errors.get(&key).map(|error| error.message.clone()), task, } } + fn is_expanded(&self, key: &TaskKey) -> bool { + self.expanded.contains(key) || self.watched.contains(key) + } + fn hold_view(view: &Entity, cx: &mut Context) -> HeldView { let view_id = view.entity_id(); let thread = view @@ -919,6 +1156,7 @@ impl TasksCard { task_id: &AgentTaskId, cx: &mut Context, ) { + let thread_entity = thread.clone(); let thread = thread.read(cx); let session_id = thread.session_id().clone(); let Some(task) = thread @@ -934,6 +1172,9 @@ impl TasksCard { return; }; let status = task.status; + // Read now: the adapter drops the ended task from its next snapshot, and the + // card drops its ended row `ENDED_ROW_TTL` later (R6.1). + let copies_log = task.kind == SHELL_KIND && task.has_output; let datagram = tray::datagram_for( &task, &session_id, @@ -948,6 +1189,8 @@ impl TasksCard { // its next snapshot. let record = TaskRecord::new(thread, &task); let key = (session_id, task.id.clone()); + // One copy per ending, however many views of the session deliver it. + let first_ending = !self.activity_ended.contains(&key); let now = cx.background_executor().now(); self.endings_seen += 1; self.ended.insert( @@ -958,8 +1201,11 @@ impl TasksCard { sequence: self.endings_seen, }, ); - self.tell_tray(key, status, datagram, cx); + self.tell_tray(key.clone(), status, datagram, cx); self.record_ended(record, cx); + if copies_log && first_ending { + self.save_log(key, &thread_entity, LogSave::CopyOnEnd, cx); + } self.schedule_expiry(cx); cx.notify(); } @@ -1107,6 +1353,7 @@ impl TasksCard { // A task that went back to running keeps its watch and its error. if !self.is_running(&key, cx) { self.watched.remove(&key); + self.expanded.remove(&key); self.row_errors.remove(&key); } } @@ -1165,6 +1412,13 @@ impl TasksCard { self.views.iter().filter_map(|held| held.thread.as_ref()) } + /// A live thread of `session_id` that the card holds, whatever its tasks. + fn held_thread(&self, session_id: &acp_v2::SessionId) -> Option> { + self.held_threads() + .filter(|held| &held.session_id == session_id) + .find_map(|held| held.thread.upgrade()) + } + fn is_running(&self, key: &TaskKey, cx: &App) -> bool { self.thread_running(key, cx).is_some() } @@ -1227,8 +1481,12 @@ impl TasksCard { .retain(|(session_id, _), _| held.contains(session_id)); self.watched .retain(|(session_id, _)| held.contains(session_id)); + self.expanded + .retain(|(session_id, _)| held.contains(session_id)); self.row_errors .retain(|(session_id, _), _| held.contains(session_id)); + self.logs_saved + .retain(|(session_id, _)| held.contains(session_id)); self.tray_weighed .retain(|(session_id, _)| held.contains(session_id)); // Only here, after the panel's deferred sync: activation removes a view and @@ -1475,6 +1733,8 @@ impl TasksCard { let TaskRow { task, watched, + expanded, + running_what, error, } = row; let task_id = task.id.clone(); @@ -1563,6 +1823,37 @@ impl TasksCard { ) }; + let expand_focus_handle = self.row_focus_handle(&key, RowControl::Expand, cx); + let (expand_icon, expand_text) = if expanded { + (IconName::ChevronDown, "Collapse task") + } else { + (IconName::ChevronRight, "Expand task") + }; + let expand_button = IconButton::new( + SharedString::from(format!("tasks-card-expand-{element_key}")), + expand_icon, + ) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .aria_label(row_aria_label(expand_text.to_string())) + .tooltip(card_tooltip( + format!("expand-{task_id}").into(), + expand_text.into(), + )) + .track_focus(&expand_focus_handle) + .on_click(cx.listener({ + let session_id = session_id.clone(); + let task_id = task_id.clone(); + move |card, _, _, cx| card.toggle_expanded(session_id.clone(), task_id.clone(), cx) + })); + let expand_button = div() + .debug_selector({ + let task_id = task_id.clone(); + move || format!("tasks-card-expand-{task_id}") + }) + .flex_none() + .child(expand_button); + let watch_focus_handle = self.row_focus_handle(&key, RowControl::Watch, cx); let (watch_tooltip, watch_aria) = if watched { ("Stop Watching This Task", "Stop watching") @@ -1654,6 +1945,70 @@ impl TasksCard { .flex_none() .child(open_button); + // Only a shell the agent can serve the output of has a log to open (R5.1). + let open_log_button = (task.kind == SHELL_KIND && task.has_output).then(|| { + let open_log_focus_handle = self.row_focus_handle(&key, RowControl::OpenLog, cx); + let button = IconButton::new( + SharedString::from(format!("tasks-card-open-log-{element_key}")), + IconName::FileTextOutlined, + ) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .aria_label(row_aria_label(format!("Open log: {shown_label}"))) + .tooltip(card_tooltip( + format!("open-log-{task_id}").into(), + "Open Log".into(), + )) + .track_focus(&open_log_focus_handle) + .on_click(cx.listener({ + let session_id = session_id.clone(); + let task_id = task_id.clone(); + move |card, _, window, cx| { + card.open_log(session_id.clone(), task_id.clone(), window, cx) + } + })); + div() + .debug_selector({ + let task_id = task_id.clone(); + move || format!("tasks-card-open-log-{task_id}") + }) + .flex_none() + .child(button) + }); + + // Only a subagent has a thread of its own to show (R5.4). + let open_thread_button = (task.kind == SUBAGENT_KIND).then(|| { + let open_thread_focus_handle = self.row_focus_handle(&key, RowControl::OpenThread, cx); + let button = IconButton::new( + SharedString::from(format!("tasks-card-open-thread-{element_key}")), + IconName::Thread, + ) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .aria_label(row_aria_label(format!( + "Open subagent thread: {shown_label}" + ))) + .tooltip(card_tooltip( + format!("open-thread-{task_id}").into(), + "Open Thread".into(), + )) + .track_focus(&open_thread_focus_handle) + .on_click(cx.listener({ + let session_id = session_id.clone(); + let task_id = task_id.clone(); + move |card, _, window, cx| { + card.open_subagent(session_id.clone(), task_id.clone(), window, cx) + } + })); + div() + .debug_selector({ + let task_id = task_id.clone(); + move || format!("tasks-card-open-thread-{task_id}") + }) + .flex_none() + .child(button) + }); + let offers_background = task.status.is_running() && background_target(&task).is_some(); let background_button = offers_background.then(|| { let background_focus_handle = self.row_focus_handle(&key, RowControl::Background, cx); @@ -1732,28 +2087,38 @@ impl TasksCard { ) .children(timing) .child(open_button) + .children(open_log_button) + .children(open_thread_button) .child(watch_button) .children(background_button) .children(stop_button); v_flex() .id(SharedString::from(format!("tasks-card-row-{element_key}"))) - .debug_selector({ - let task_id = task_id.clone(); - move || format!("tasks-card-row-{task_id}") - }) + .debug_selector(move || format!("tasks-card-row-{task_id}")) .pl_2() .py_0p5() - .child(line) - .children(progress_line) - .when(watched, |this| this.child(render_details(&task, &task_id))) - .when_some(error, |this, error| { - this.child( - Label::new(error) - .size(LabelSize::XSmall) - .color(Color::Error), - ) - }) + .child( + // The disclosure sits in a gutter of its own, so the progress line and the + // expanded body stay aligned under the description as before. + h_flex().items_start().gap_1().child(expand_button).child( + v_flex() + .flex_1() + .min_w_0() + .child(line) + .children(progress_line) + .when(expanded, |this| { + this.child(render_expanded_body(&task, running_what, cx)) + }) + .when_some(error, |this, error| { + this.child( + Label::new(error) + .size(LabelSize::XSmall) + .color(Color::Error), + ) + }), + ), + ) .into_any_element() } @@ -1954,7 +2319,95 @@ fn running_count(groups: &[ThreadGroup]) -> usize { .count() } -/// A watched row's progress details (R3.2): tokens, tool uses, last tool, summary. +/// What an expanded row shows under its line (R4.1–R4.4): what the task runs, its +/// progress details, and for a running shell its tail or "No output yet". +fn render_expanded_body(task: &AgentTask, running_what: SharedString, cx: &App) -> AnyElement { + let task_id = &task.id; + let running_what = div() + .debug_selector({ + let task_id = task_id.clone(); + move || format!("tasks-card-running-what-{task_id}") + }) + .pl_1() + .min_w_0() + .child( + Label::new(collapse_label(&running_what)) + .size(LabelSize::XSmall) + .color(Color::Muted) + .single_line() + .truncate(), + ); + let shows_tail = task.kind == SHELL_KIND && task.status.is_running(); + v_flex() + .min_w_0() + .gap_0p5() + .child(running_what) + .child(render_details(task, task_id)) + .when(shows_tail, |this| this.child(render_tail(task, cx))) + .into_any_element() +} + +/// A running shell's tail, one entry per line in a monospace block, or "No output yet" +/// before the agent has sent any (R4.3, R4.4). +fn render_tail(task: &AgentTask, cx: &App) -> AnyElement { + let task_id = task.id.clone(); + let Some(lines) = task.tail.as_deref().filter(|tail| !tail.is_empty()) else { + return div() + .debug_selector(move || format!("tasks-card-no-output-{task_id}")) + .pl_1() + .child( + Label::new("No output yet") + .size(LabelSize::XSmall) + .color(Color::Muted), + ) + .into_any_element(); + }; + v_flex() + .debug_selector(move || format!("tasks-card-tail-{task_id}")) + .min_w_0() + .overflow_hidden() + .px_1() + .rounded_sm() + .bg(cx.theme().colors().editor_background) + .children(lines.iter().map(|line| { + Label::new(tail_line(line)) + .size(LabelSize::XSmall) + .buffer_font(cx) + .single_line() + .truncate() + })) + .into_any_element() +} + +/// The task kind whose rows show a tail (R4.3). +const SHELL_KIND: &str = "shell"; + +/// The task kind that runs in a thread of its own (R5.4). +const SUBAGENT_KIND: &str = "subagent"; + +/// A tail entry on one row of the block: `single_line` stops soft wrapping, but a line break +/// inside the text would still start a new line and draw over the next entry. +fn tail_line(line: &SharedString) -> SharedString { + if line.contains(['\n', '\r']) { + line.replace(['\n', '\r'], " ").into() + } else { + line.clone() + } +} + +/// The label of the root thread's tool call that started `task` — the command line for a +/// Bash call — else the task's description (R4.1). Only the task's own tool call counts. +fn running_what(thread: &AcpThread, task: &AgentTask, cx: &App) -> SharedString { + task.tool_call_id + .as_ref() + .and_then(|id| thread.tool_call(id)) + .map(|(_, tool_call)| tool_call.label.read(cx).source()) + .filter(|label| !label.trim().is_empty()) + .map(|label| SharedString::from(label.to_string())) + .unwrap_or_else(|| task.description.clone()) +} + +/// An expanded row's progress details (R3.2, R4.2): tokens, tool uses, last tool, summary. fn render_details(task: &AgentTask, task_id: &AgentTaskId) -> AnyElement { let usage = match &task.usage { Some(usage) => format!("{} tokens · {} tool uses", usage.tokens, usage.tool_uses), diff --git a/crates/agent_ui/src/tasks_card/expand_tests.rs b/crates/agent_ui/src/tasks_card/expand_tests.rs new file mode 100644 index 0000000..551e8f2 --- /dev/null +++ b/crates/agent_ui/src/tasks_card/expand_tests.rs @@ -0,0 +1,314 @@ +// Story 041, task 6.1 — a card row expands: what the task runs, its progress details, its tail +// or "No output yet"; watched rows stay expanded; nothing persists; the control is labelled and +// reachable from the keyboard (R4.1–R4.8; Q16). Filter: `tasks_card::expand`. +// +// Wired in `src/tasks_card.rs` beside the other card test modules: +// +// #[cfg(test)] +// #[path = "tasks_card/expand_tests.rs"] +// mod expand; +// +// Contract (design.md §`TasksCard` changes) and what this file assumes beyond it, in the card's +// existing style: +// - `TasksCard::toggle_expanded(session_id, task_id, cx)`; `TaskRow::expanded: bool` (true when +// toggled open OR watched) and `TaskRow::running_what: SharedString`; +// - `TasksCard::expand_focus_handle(&session_id, &task_id) -> Option`, like +// `watch_focus_handle`; +// - debug selectors `tasks-card-expand-{id}`, `tasks-card-running-what-{id}`, +// `tasks-card-tail-{id}`, `tasks-card-no-output-{id}`, and story 013's +// `tasks-card-details-{id}`; +// - the disclosure's screen-reader label lands in `RENDERED_ARIA_LABELS` like the row's other +// controls. +// +// Hostile halves first: a task whose tool call is NOT in the thread must not borrow another +// tool call's title; expanding A's `t1` must not expand B's `t1`; a subagent row expanded shows +// no tail block and no "No output yet". + +use acp_thread::AcpThread; +use agent_client_protocol::v1::schema as acp; +use gpui::{AppContext as _, Entity, FocusHandle, TestAppContext, VisualTestContext}; +use serde_json::json; + +use super::RENDERED_ARIA_LABELS; +use super::test_helpers::*; + +fn draw(cx: &mut VisualTestContext) { + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); +} + +fn painted(selector: &'static str, cx: &mut VisualTestContext) -> bool { + draw(cx); + cx.debug_bounds(selector).is_some() +} + +fn shell(id: &str, tail: Option<&[&str]>, started_at: u64) -> serde_json::Value { + let mut task = wire_task(id, "running", true, started_at); + task["tail"] = tail.map_or(json!(null), |lines| json!(lines)); + task["hasOutput"] = json!(tail.is_some()); + task +} + +fn push_tool_call(thread: &Entity, id: &str, title: &str, cx: &mut VisualTestContext) { + thread.update(cx, |thread, cx| { + thread + .handle_session_update( + acp::SessionUpdate::ToolCall(acp::ToolCall::new(id.to_string(), title.to_string())), + cx, + ) + .expect("tool call applies"); + }); + cx.run_until_parked(); +} + +fn aria_labels() -> Vec { + RENDERED_ARIA_LABELS.with(|labels| labels.borrow().iter().map(|l| l.to_string()).collect()) +} + +fn reachable_by_tab(handle: &FocusHandle, cx: &mut VisualTestContext) -> bool { + for _ in 0..200 { + cx.update(|window, cx| window.focus_next(cx)); + draw(cx); + if cx.update(|window, _| handle.is_focused(window)) { + return true; + } + } + false +} + +// ---- R4.1 — what the task runs ------------------------------------------------------------- + +// Hostile: t2's tool call is not in the thread; its row must fall back to its description, not +// take toolu_t1's title. +#[gpui::test] +async fn test_expand_running_what_comes_from_its_own_tool_call_or_the_description( + cx: &mut TestAppContext, +) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + push_tool_call(&a, "toolu_t1", "sleep 30 && echo done", &mut cx); + send_tasks( + &a, + vec![shell("t1", None, 1_000), shell("t2", None, 2_000)], + &mut cx, + ); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + let t1 = find_row(&card, &sa, "t1", &mut cx).unwrap(); + let t2 = find_row(&card, &sa, "t2", &mut cx).unwrap(); + assert_eq!(t1.running_what.as_ref(), "sleep 30 && echo done"); + assert_eq!( + t2.running_what.as_ref(), + "Task t2", + "no tool call of its own: its description" + ); +} + +// ---- R4.2–R4.5 — expand and collapse -------------------------------------------------------- + +#[gpui::test] +async fn test_expand_shows_what_runs_details_and_tail_then_collapses(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + send_tasks( + &a, + vec![shell("t1", Some(&["line 1", "line 2"]), 1_000)], + &mut cx, + ); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + assert!( + !find_row(&card, &sa, "t1", &mut cx).unwrap().expanded, + "collapsed by default" + ); + assert!(!painted("tasks-card-tail-t1", &mut cx)); + assert!(!painted("tasks-card-details-t1", &mut cx)); + + card.update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "t1".into(), cx) + }); + assert!(find_row(&card, &sa, "t1", &mut cx).unwrap().expanded); + assert!(painted("tasks-card-running-what-t1", &mut cx)); + assert!(painted("tasks-card-details-t1", &mut cx)); + assert!(painted("tasks-card-tail-t1", &mut cx)); + assert!(!painted("tasks-card-no-output-t1", &mut cx)); + + // R4.3: updated with each snapshot. + send_tasks( + &a, + vec![shell("t1", Some(&["line 1", "line 2", "line 3"]), 1_000)], + &mut cx, + ); + let row = find_row(&card, &sa, "t1", &mut cx).unwrap(); + let tail: Vec = row + .task + .tail + .as_ref() + .unwrap() + .iter() + .map(|l| l.to_string()) + .collect(); + assert_eq!(tail, vec!["line 1", "line 2", "line 3"]); + assert!(row.expanded, "a snapshot does not collapse the row"); + + card.update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "t1".into(), cx) + }); + assert!(!find_row(&card, &sa, "t1", &mut cx).unwrap().expanded); + assert!(!painted("tasks-card-tail-t1", &mut cx)); +} + +#[gpui::test] +async fn test_expand_shell_without_tail_says_no_output_yet(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", None, 1_000)], &mut cx); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + card.update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "t1".into(), cx) + }); + assert!(painted("tasks-card-no-output-t1", &mut cx)); + assert!(!painted("tasks-card-tail-t1", &mut cx)); +} + +// Hostile: a subagent is not a shell; it has neither block. +#[gpui::test] +async fn test_expand_subagent_shows_details_but_no_tail_block(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + let mut sub = wire_task("s1", "running", false, 1_000); + sub["type"] = json!("subagent"); + sub["hasOutput"] = json!(false); + sub["tail"] = json!(null); + send_tasks(&a, vec![sub], &mut cx); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + card.update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "s1".into(), cx) + }); + assert!(painted("tasks-card-details-s1", &mut cx)); + assert!(!painted("tasks-card-tail-s1", &mut cx)); + assert!(!painted("tasks-card-no-output-s1", &mut cx)); +} + +// Hostile (wrong collapse): the same task id in two threads is two rows. +#[gpui::test] +async fn test_expand_one_threads_t1_leaves_the_others_collapsed(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", None, 1_000)], &mut cx); + let b = open_titled_thread(&panel, &connection, "B", &mut cx); + send_tasks(&b, vec![shell("t1", None, 2_000)], &mut cx); + let (sa, sb) = (session_of(&a, &cx), session_of(&b, &cx)); + let card = card(&panel, &cx); + + card.update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "t1".into(), cx) + }); + assert!(find_row(&card, &sa, "t1", &mut cx).unwrap().expanded); + assert!(!find_row(&card, &sb, "t1", &mut cx).unwrap().expanded); +} + +// ---- R4.6, R4.8 — watched rows, and nothing persists --------------------------------------- + +#[gpui::test] +async fn test_expand_a_watched_row_is_expanded_until_unwatched(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", Some(&["x"]), 1_000)], &mut cx); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + card.update(&mut cx, |card, cx| { + card.toggle_watch(sa.clone(), "t1".into(), cx) + }); + assert!( + find_row(&card, &sa, "t1", &mut cx).unwrap().expanded, + "watched means expanded" + ); + assert!(painted("tasks-card-tail-t1", &mut cx)); + + card.update(&mut cx, |card, cx| { + card.toggle_watch(sa.clone(), "t1".into(), cx) + }); + assert!(!find_row(&card, &sa, "t1", &mut cx).unwrap().expanded); +} + +#[gpui::test] +async fn test_expand_state_is_not_carried_into_a_new_card(cx: &mut TestAppContext) { + let (panel, workspace, mut cx) = setup_card_panel_in_workspace(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", None, 1_000)], &mut cx); + let sa = session_of(&a, &cx); + card(&panel, &cx).update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "t1".into(), cx) + }); + + // A restart builds a fresh panel and card; the held thread is handed over again. + let fresh = workspace.update_in(&mut cx, |workspace, window, cx| { + cx.new(|cx| crate::AgentPanel::new(workspace, window, cx)) + }); + let view = panel.read_with(&cx, |panel, _| { + panel.active_conversation_view().cloned().unwrap() + }); + let fresh_card = card(&fresh, &cx); + fresh_card.update(&mut cx, |card, cx| { + card.sync_threads([&view].into_iter(), cx) + }); + cx.run_until_parked(); + + assert!(!find_row(&fresh_card, &sa, "t1", &mut cx).unwrap().expanded); +} + +// ---- R4.7, Q16 — labelled and reachable ---------------------------------------------------- + +#[gpui::test] +async fn test_expand_control_is_labelled_by_its_state_and_reachable_by_tab( + cx: &mut TestAppContext, +) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", None, 1_000)], &mut cx); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + draw(&mut cx); + assert!(painted("tasks-card-expand-t1", &mut cx)); + assert!( + aria_labels().iter().any(|l| l == "Expand task"), + "{:?}", + aria_labels() + ); + assert!(!aria_labels().iter().any(|l| l == "Collapse task")); + + card.update(&mut cx, |card, cx| { + card.toggle_expanded(sa.clone(), "t1".into(), cx) + }); + draw(&mut cx); + assert!( + aria_labels().iter().any(|l| l == "Collapse task"), + "{:?}", + aria_labels() + ); + + let handle = card + .read_with(&cx, |card, _| card.expand_focus_handle(&sa, &"t1".into())) + .expect("the painted row has a focus handle for its disclosure"); + assert!( + reachable_by_tab(&handle, &mut cx), + "the disclosure is a tab stop" + ); +} diff --git a/crates/agent_ui/src/tasks_card/open_log_remote_tests.rs b/crates/agent_ui/src/tasks_card/open_log_remote_tests.rs new file mode 100644 index 0000000..a919ef9 --- /dev/null +++ b/crates/agent_ui/src/tasks_card/open_log_remote_tests.rs @@ -0,0 +1,514 @@ +// Story 041, task 7.2 — "Open Log" on a card row is refused in a remote project (R5.10; Q5, Q16). +// Filter: `cargo test -p agent_ui tasks_card::open_log` (the module path is +// `tasks_card::open_log_remote::…`, so the 6.2 filter selects these tests too). +// +// Wired in `src/tasks_card.rs`, beside 6.2's `open_log` module: +// +// #[cfg(test)] +// #[path = "tasks_card/open_log_remote_tests.rs"] +// mod open_log_remote; +// +// The remote seam: a REAL remote project, not a predicate. `Project::remote` over +// `remote::RemoteClient::fake_server` + `connect_mock` — the mock transport `terminal_view`'s +// `init_remote_test` already uses — so `is_via_remote_server()` is true and `is_local()` false +// through the same code a live SSH project takes. No `remote_server::HeadlessProject`: the server +// side is a bare session with two handlers, `Ping` (the client pings once on connect) and +// `GetPathMetadata`, which RECORDS every path the client asks about and answers "exists, a file" +// — the impostor design.md names: "an unrelated remote file at the same path would open +// instead". An unguarded open resolves the log path on the remote host through exactly that +// request (`Project::resolve_abs_file_path_canonical`), so "the log path never crossed the wire" +// is the observable proof that nothing was read or opened remotely. +// +// Dependencies this file uses, all already in agent_ui's Cargo.toml (checked 2026-10-12): +// `remote` (test-support), `proto`, `workspace` (test-support, for `AppState::test`), `fs`, +// `release_channel`, `semver`. No Cargo.toml change is needed. +// +// Contract (design.md §`TasksCard` changes, "Remote projects (R5.10)"; story.md R5.10, R6.1 and +// Out of Scope "the copy on end still saves the log"), and what this file assumes beyond it — +// all of it already true in the 6.2 build: +// - `TasksCard::open_log(session_id, task_id, window, cx)`, emitting `OpenLog`, handled by +// `AgentPanel` (`open_log_from_card`); +// - the row's message is read back through `TaskRow::error`; +// - `task_log::{LogsRootOverride, log_path, render_log}`. +// The refusal text is pinned exactly: "Open Log is not available in remote projects". +// Whether the card still fetches and saves before the panel refuses is NOT pinned. +// +// Order: hostile halves first — +// 1. remote (wrong "open"): the row says the notice, no tab opens, the log path is never sent +// to the remote host, and a sibling row is not marked; +// 2. remote (wrong over-reach): the refusal must not swallow the copy on end — an ended shell's +// log is still written locally, with no row message; +// then the converse — +// 3. local: the same row opens its log read-only with no message. +// +// Red, before 7.2: test 1 fails on its first assertion — the unguarded panel resolves the path on +// the remote host (the recorder sees it) and the row shows "Could not open the log: …" or +// nothing, never the notice. Tests 2 and 3 are guards against an over-broad fix and may already +// pass; Red for 7.2 is test 1. + +use std::{ + any::Any, + collections::HashMap, + path::{Path, PathBuf}, + rc::Rc, + sync::{Arc, Mutex}, +}; + +use acp_thread::{AcpThread, AgentConnection, StubAgentConnection, TaskOutput}; +use agent_client_protocol::v1::schema as acp; +use agent_client_protocol::v2::schema as acp_v2; +use anyhow::{Result, anyhow}; +use gpui::{App, AppContext as _, Entity, SharedString, Task, TestAppContext, VisualTestContext}; +use project::{AgentId, Project}; +use serde_json::json; +use workspace::{MultiWorkspace, PathList, Workspace}; + +use super::task_log::{LogsRootOverride, log_path, render_log}; +use super::test_helpers::*; +use crate::AgentPanel; +use crate::conversation_view::tests::init_test; +use crate::test_support::open_thread_with_custom_connection; + +const REMOTE_NOTICE: &str = "Open Log is not available in remote projects"; + +// ---- A connection that answers `fetch_task_output` (a trimmed copy of 6.2's) ------------- + +#[derive(Clone)] +struct OutputConnection { + stub: StubAgentConnection, + next_session: Arc>, + answers: Arc>>, +} + +impl OutputConnection { + fn new() -> Self { + Self { + stub: StubAgentConnection::new(), + next_session: Arc::new(Mutex::new(0)), + answers: Arc::default(), + } + } + + fn answer(&self, session: &acp_v2::SessionId, task: &str, output: TaskOutput) { + self.answers + .lock() + .unwrap() + .insert((session.to_string(), task.to_string()), output); + } +} + +impl AgentConnection for OutputConnection { + fn agent_id(&self) -> AgentId { + self.stub.agent_id() + } + + fn telemetry_id(&self) -> SharedString { + "open-log-remote-test".into() + } + + fn new_session( + self: Rc, + project: Entity, + work_dirs: PathList, + cx: &mut App, + ) -> Task>> { + let session_id = { + let mut next = self.next_session.lock().unwrap(); + *next += 1; + acp_v2::SessionId::new(format!("open-log-remote-session-{}", *next)) + }; + let action_log = cx.new(|_| action_log::ActionLog::new(project.clone())); + let thread = cx.new(|cx| { + AcpThread::new( + None, + None, + Some(work_dirs), + self.clone(), + project, + action_log, + session_id, + watch::Receiver::constant(acp_v2::PromptCapabilities::new()), + cx, + ) + }); + Task::ready(Ok(thread)) + } + + fn auth_methods(&self) -> &[acp_v2::AuthMethod] { + &[] + } + + fn authenticate(&self, _method: acp_v2::AuthMethodId, _cx: &mut App) -> Task> { + Task::ready(Ok(())) + } + + fn prompt( + &self, + _params: acp_v2::PromptRequest, + _cx: &mut App, + ) -> Task> { + Task::ready(Ok(acp::PromptResponse::new(acp::StopReason::EndTurn))) + } + + fn cancel(&self, _session_id: &acp_v2::SessionId, _cx: &mut App) -> Result<()> { + Ok(()) + } + + fn fetch_task_output( + self: Rc, + session_id: &acp_v2::SessionId, + task_id: &str, + _cx: &mut App, + ) -> Task> { + let key = (session_id.to_string(), task_id.to_string()); + let answer = self.answers.lock().unwrap().get(&key).cloned(); + Task::ready(answer.ok_or_else(|| { + anyhow!("_claude/tasks/output failed for {task_id}: no answer scripted") + })) + } + + fn into_any(self: Rc) -> Rc { + self + } +} + +// ---- Rig ----------------------------------------------------------------------------------- + +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-open-log-remote-{name}-{}-{nanos}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } + fn root(&self) -> PathBuf { + self.0.join("agent_task_logs") + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// The remote host's side: every path the client asked `GetPathMetadata` about. The `anchor` +/// owns the handlers; drop it and the server stops answering. +struct RemoteHost { + asked: Arc>>, + _anchor: Entity<()>, +} + +impl RemoteHost { + fn asked(&self) -> Vec { + self.asked.lock().unwrap().clone() + } +} + +/// A visible agent panel in a workspace window whose project is a real remote-server project +/// over the mock transport. +async fn setup_remote_card_panel( + cx: &mut TestAppContext, + server_cx: &mut TestAppContext, +) -> ( + Entity, + Entity, + RemoteHost, + VisualTestContext, +) { + init_test(cx); + cx.update(|cx| { + agent::ThreadStore::init_global(cx); + language_model::LanguageModelRegistry::test(cx); + }); + server_cx.update(|cx| release_channel::init(semver::Version::new(0, 0, 0), cx)); + // The client, user store, languages and node runtime for `Project::remote`, as + // `terminal_view`'s `init_remote_test` takes them. `AppState::test` installs its own FakeFs + // as the global; it is the one the project and the tests use. + let params = cx.update(workspace::AppState::test); + + let (opts, server_session, connect_guard) = remote::RemoteClient::fake_server(cx, server_cx); + let anchor = server_cx.new(|_| ()); + server_session + .add_request_handler::(anchor.downgrade(), |_, _, _| async { + Ok(proto::Ack {}) + }); + let asked: Arc>> = Arc::default(); + let recorder = asked.clone(); + server_session.add_request_handler::( + anchor.downgrade(), + move |_, envelope, _| { + let recorder = recorder.clone(); + async move { + let path = envelope.payload.path; + recorder.lock().unwrap().push(path.clone()); + // The impostor: the remote host claims a file at whatever path is asked. + Ok(proto::GetPathMetadataResponse { + exists: true, + path, + is_dir: false, + }) + } + }, + ); + drop(connect_guard); + + let remote_client = remote::RemoteClient::connect_mock(opts, cx).await; + let project = cx.update(|cx| { + Project::remote( + remote_client, + params.client.clone(), + params.node_runtime.clone(), + params.user_store.clone(), + params.languages.clone(), + params.fs.clone(), + false, + cx, + ) + }); + project.read_with(cx, |project, _| { + assert!( + project.is_via_remote_server() && !project.is_local(), + "the rig must build a remote-server project" + ); + }); + + // `AppState::test` already registered a `WorkspaceStore` on `params.client`, and + // `MultiWorkspace::test_new` would register a second one on the project's (same) client — + // "registered handler for the same message proto::Follow twice". Build the workspace on + // `params`, as `terminal_view`'s `init_remote_test` does. + let multi_workspace = cx.add_window({ + // Run-mode fix, story 041 7.2 + let params = params.clone(); // Run-mode fix, story 041 7.2 + move |window, cx| { + // Run-mode fix, story 041 7.2 + window.activate_window(); // Run-mode fix, story 041 7.2 + let workspace = // Run-mode fix, story 041 7.2 + cx.new(|cx| Workspace::new(None, project, params, window, cx)); // Run-mode fix, story 041 7.2 + MultiWorkspace::new(workspace, window, cx) // Run-mode fix, story 041 7.2 + } // Run-mode fix, story 041 7.2 + }); // Run-mode fix, story 041 7.2 + let workspace = multi_workspace + .read_with(cx, |multi_workspace, _cx| { + multi_workspace.workspace().clone() + }) + .unwrap(); + let mut cx = VisualTestContext::from_window(multi_workspace.into(), cx); + cx.simulate_resize(gpui::size(gpui::px(1200.), gpui::px(800.))); + let panel = workspace.update_in(&mut cx, |workspace, window, cx| { + let panel = cx.new(|cx| AgentPanel::new(workspace, window, cx)); + workspace.add_panel(panel.clone(), window, cx); + workspace.focus_panel::(window, cx); + panel + }); + cx.run_until_parked(); + ( + panel, + workspace, + RemoteHost { + asked, + _anchor: anchor, + }, + cx, + ) +} + +fn use_scratch_logs_root(root: &Path, cx: &mut VisualTestContext) { + let root = root.to_path_buf(); + cx.update(|_, cx| cx.set_global(LogsRootOverride(root))); +} + +fn open_thread( + panel: &Entity, + connection: &OutputConnection, + title: &str, + cx: &mut VisualTestContext, +) -> Entity { + open_thread_with_custom_connection(panel, connection.clone(), cx); + let thread = panel.read_with(cx, |panel, cx| panel.active_agent_thread(cx).unwrap()); + send_info(&thread, acp::SessionInfoUpdate::new().title(title), cx); + thread +} + +fn shell(id: &str, status: &str) -> serde_json::Value { + let mut task = wire_task(id, status, true, 1_000); + task["hasOutput"] = json!(true); + task["tail"] = json!(null); + task +} + +fn out(content: &str) -> TaskOutput { + TaskOutput { + content: content.to_string(), + total_bytes: content.len() as u64, + truncated: false, + } +} + +fn active_editor( + workspace: &Entity, + cx: &mut VisualTestContext, +) -> Option<(bool, String)> { + workspace.update(cx, |workspace, cx| { + workspace + .active_item_as::(cx) + .map(|editor| { + let editor = editor.read(cx); + (editor.read_only(cx), editor.text(cx)) + }) + }) +} + +fn open_log( + card: &Entity, + session: &acp_v2::SessionId, + task: &str, + cx: &mut VisualTestContext, +) { + let task: acp_thread::AgentTaskId = task.to_string().into(); + card.update_in(cx, |card, window, cx| { + card.open_log(session.clone(), task, window, cx) + }); + cx.run_until_parked(); +} + +// ---- 1. Hostile: a remote project opens nothing and says why ------------------------------- + +#[gpui::test] +async fn test_open_log_in_a_remote_project_shows_the_notice_and_opens_nothing( + cx: &mut TestAppContext, + server_cx: &mut TestAppContext, +) { + let (panel, workspace, host, mut cx) = setup_remote_card_panel(cx, server_cx).await; + let scratch = Scratch::new("remote"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks( + &a, + vec![shell("t1", "running"), shell("t2", "running")], + &mut cx, + ); + let sa = session_of(&a, &cx); + connection.answer(&sa, "t1", out("REMOTE-PROJECT-LOG\n")); + connection.answer(&sa, "t2", out("sibling\n")); + let card = card(&panel, &cx); + let path = log_path(&scratch.root(), &sa.to_string(), "t1"); + let before = active_editor(&workspace, &mut cx); + + open_log(&card, &sa, "t1", &mut cx); + + let error = find_row(&card, &sa, "t1", &mut cx) + .expect("t1 is listed") + .error + .map(|error| error.to_string()); + assert_eq!( + error.as_deref(), + Some(REMOTE_NOTICE), + "the row says Open Log is refused, not an open failure" + ); + let path_text = path.to_string_lossy().to_string(); + let root_text = scratch.root().to_string_lossy().to_string(); + let leaked: Vec = host + .asked() + .into_iter() + .filter(|asked| asked == &path_text || asked.starts_with(&root_text)) + .collect(); + assert!( + leaked.is_empty(), + "the local log path must never be resolved on the remote host: {leaked:?}" + ); + assert_eq!(active_editor(&workspace, &mut cx), before, "no tab opened"); + assert!( + find_row(&card, &sa, "t2", &mut cx) + .expect("t2 is listed") + .error + .is_none(), + "the notice lands on the row that asked, not on its sibling" + ); +} + +// ---- 2. Hostile: the refusal does not reach the copy on end -------------------------------- + +#[gpui::test] +async fn test_open_log_remote_project_still_copies_an_ended_shell_log( + cx: &mut TestAppContext, + server_cx: &mut TestAppContext, +) { + let (panel, workspace, _host, mut cx) = setup_remote_card_panel(cx, server_cx).await; + let scratch = Scratch::new("remote-copy"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running")], &mut cx); + let sa = session_of(&a, &cx); + let output = out("ended in a remote project\n"); + connection.answer(&sa, "t1", output.clone()); + let card = card(&panel, &cx); + let before = active_editor(&workspace, &mut cx); + + send_tasks(&a, vec![shell("t1", "completed")], &mut cx); + send_tasks(&a, vec![], &mut cx); + + let path = log_path(&scratch.root(), &sa.to_string(), "t1"); + assert_eq!( + std::fs::read_to_string(&path).ok().as_deref(), + Some(render_log(&output).as_str()), + "the copy on end still saves the log locally in a remote project" + ); + assert_eq!( + active_editor(&workspace, &mut cx), + before, + "the copy opens nothing" + ); + if let Some(row) = find_row(&card, &sa, "t1", &mut cx) { + assert!(row.error.is_none(), "nobody asked, so the row says nothing"); + } +} + +// ---- 3. Converse: the same row in a local project opens its log ---------------------------- + +#[gpui::test] +async fn test_open_log_in_a_local_project_still_opens_the_log_read_only(cx: &mut TestAppContext) { + let (panel, workspace, mut cx) = setup_card_panel_in_workspace(cx).await; + let project = workspace.update(&mut cx, |workspace, _| workspace.project().clone()); + project.read_with(&cx, |project, _| assert!(project.is_local())); + let scratch = Scratch::new("local"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running")], &mut cx); + let sa = session_of(&a, &cx); + let output = out("LOCAL-PROJECT-LOG\n"); + connection.answer(&sa, "t1", output.clone()); + let path = log_path(&scratch.root(), &sa.to_string(), "t1"); + let rendered = render_log(&output); + // The workspace opens through its FakeFs, the card writes the real disk (as in 6.2). + let fs = cx.update(|_, cx| ::global(cx)); + fs.create_dir(path.parent().unwrap()).await.unwrap(); + fs.as_fake() + .insert_file(&path, rendered.as_bytes().to_vec()) + .await; + let card = card(&panel, &cx); + + open_log(&card, &sa, "t1", &mut cx); + + let (read_only, text) = active_editor(&workspace, &mut cx).expect("an editor tab opened"); + assert!(read_only, "the log opens read-only"); + assert_eq!(text, rendered); + let error = find_row(&card, &sa, "t1", &mut cx) + .expect("t1 is listed") + .error; + assert!( + error.is_none(), + "a local project is never told Open Log is unavailable: {error:?}" + ); +} diff --git a/crates/agent_ui/src/tasks_card/open_log_tests.rs b/crates/agent_ui/src/tasks_card/open_log_tests.rs new file mode 100644 index 0000000..a104e02 --- /dev/null +++ b/crates/agent_ui/src/tasks_card/open_log_tests.rs @@ -0,0 +1,562 @@ +// Story 041, task 6.2 — "Open Log" on a card row, and the copy of a shell's log when it ends +// (R5.1, R5.2, R5.3, R6.1, R6.4, R6.7; Q4, Q16). Filter: `tasks_card::open_log`. +// +// Wired in `src/tasks_card.rs`: +// +// #[cfg(test)] +// #[path = "tasks_card/open_log_tests.rs"] +// mod open_log; +// +// Contract (design.md §`TasksCard` changes, §`task_log`) and what this file assumes beyond it: +// - `TasksCard::open_log(session_id, task_id, window, cx)`, public like `open_task`; +// - `TasksCardEvent::TaskLogSaved { session_id, task_id, bytes: Option }` then `TasksCardEvent::OpenLog +// { session_id, task_id, path }`; the agent panel opens `path` read-only; +// - debug selector `tasks-card-open-log-{id}`; +// - the test seam shared with the store and panel tests: the card reads the log root through +// `task_log::logs_root_in(cx)`, which honours the `task_log::LogsRootOverride(PathBuf)` gpui +// global (never set in production). Change `use_scratch_logs_root` only if the seam differs. +// +// The workspace in these tests runs on a FakeFs while `write_log` writes the real disk (the +// design's std::fs code). So the opened tab is checked against a FakeFs copy seeded at the +// path the card is expected to write, BEFORE the click; the real file is checked on disk. The +// GUI proof (task 8.2) covers the real filesystem end to end. +// +// Hostile halves first: a shell without output and a subagent get no "Open Log"; a failed fetch +// or write opens nothing and says why on the row; two threads' `t1` never share a log; an ended +// shell without output, or a subagent, is never fetched; a failed copy on end shows nothing on +// the row (nobody asked) and records nothing. + +use std::{ + any::Any, + cell::RefCell, + collections::HashMap, + path::{Path, PathBuf}, + rc::Rc, + sync::{Arc, Mutex}, +}; + +use acp_thread::{AcpThread, AgentConnection, StubAgentConnection, TaskOutput}; +use agent_client_protocol::v1::schema as acp; +use agent_client_protocol::v2::schema as acp_v2; +use anyhow::{Result, anyhow}; +use gpui::{App, AppContext as _, Entity, SharedString, Task, TestAppContext, VisualTestContext}; +use project::{AgentId, Project}; +use serde_json::json; +use workspace::PathList; + +use super::task_log::{LogsRootOverride, log_path, render_log}; +use super::test_helpers::*; +use super::{TasksCard, TasksCardEvent}; +use crate::test_support::open_thread_with_custom_connection; + +// ---- A connection that answers `fetch_task_output` as scripted ------------------------------ + +#[derive(Clone)] +struct OutputConnection { + stub: StubAgentConnection, + next_session: Arc>, + /// task id -> the answer; a missing id answers an error. + answers: Arc>>>, + fetches: Arc>>, +} + +impl OutputConnection { + fn new() -> Self { + Self { + stub: StubAgentConnection::new(), + next_session: Arc::new(Mutex::new(0)), + answers: Arc::default(), + fetches: Arc::default(), + } + } + + fn answer( + &self, + session: &acp_v2::SessionId, + task: &str, + answer: std::result::Result, + ) { + self.answers + .lock() + .unwrap() + .insert((session.to_string(), task.to_string()), answer); + } + + fn fetches(&self) -> Vec<(String, String)> { + self.fetches.lock().unwrap().clone() + } +} + +impl AgentConnection for OutputConnection { + fn agent_id(&self) -> AgentId { + self.stub.agent_id() + } + + fn telemetry_id(&self) -> SharedString { + "open-log-test".into() + } + + fn new_session( + self: Rc, + project: Entity, + work_dirs: PathList, + cx: &mut App, + ) -> Task>> { + let session_id = { + let mut next = self.next_session.lock().unwrap(); + *next += 1; + acp_v2::SessionId::new(format!("open-log-session-{}", *next)) + }; + let action_log = cx.new(|_| action_log::ActionLog::new(project.clone())); + let thread = cx.new(|cx| { + AcpThread::new( + None, + None, + Some(work_dirs), + self.clone(), + project, + action_log, + session_id, + watch::Receiver::constant(acp_v2::PromptCapabilities::new()), + cx, + ) + }); + Task::ready(Ok(thread)) + } + + fn auth_methods(&self) -> &[acp_v2::AuthMethod] { + &[] + } + + fn authenticate(&self, _method: acp_v2::AuthMethodId, _cx: &mut App) -> Task> { + Task::ready(Ok(())) + } + + fn prompt( + &self, + _params: acp_v2::PromptRequest, + _cx: &mut App, + ) -> Task> { + Task::ready(Ok(acp::PromptResponse::new(acp::StopReason::EndTurn))) + } + + fn cancel(&self, _session_id: &acp_v2::SessionId, _cx: &mut App) -> Result<()> { + Ok(()) + } + + fn fetch_task_output( + self: Rc, + session_id: &acp_v2::SessionId, + task_id: &str, + _cx: &mut App, + ) -> Task> { + let key = (session_id.to_string(), task_id.to_string()); + self.fetches.lock().unwrap().push(key.clone()); + let answer = self.answers.lock().unwrap().get(&key).cloned(); + Task::ready(match answer { + Some(Ok(output)) => Ok(output), + Some(Err(message)) => Err(anyhow!( + "_claude/tasks/output failed for {task_id}: {message}" + )), + None => Err(anyhow!( + "_claude/tasks/output failed for {task_id}: no answer scripted" + )), + }) + } + + fn into_any(self: Rc) -> Rc { + self + } +} + +// ---- Rig --------------------------------------------------------------------------------- + +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-open-log-{name}-{}-{nanos}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } + fn root(&self) -> PathBuf { + self.0.join("agent_task_logs") + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn use_scratch_logs_root(root: &Path, cx: &mut VisualTestContext) { + let root = root.to_path_buf(); + cx.update(|_, cx| cx.set_global(LogsRootOverride(root))); +} + +fn open_thread( + panel: &Entity, + connection: &OutputConnection, + title: &str, + cx: &mut VisualTestContext, +) -> Entity { + open_thread_with_custom_connection(panel, connection.clone(), cx); + let thread = panel.read_with(cx, |panel, cx| panel.active_agent_thread(cx).unwrap()); + send_info(&thread, acp::SessionInfoUpdate::new().title(title), cx); + thread +} + +fn shell(id: &str, status: &str, has_output: bool) -> serde_json::Value { + let mut task = wire_task(id, status, true, 1_000); + task["hasOutput"] = json!(has_output); + task["tail"] = json!(null); + task +} + +fn subagent(id: &str, status: &str) -> serde_json::Value { + let mut task = wire_task(id, status, false, 1_000); + task["type"] = json!("subagent"); + task["hasOutput"] = json!(false); + task["tail"] = json!(null); + task +} + +fn out(content: &str, truncated: bool) -> TaskOutput { + TaskOutput { + content: content.to_string(), + total_bytes: if truncated { + 3 * 1_048_576 + } else { + content.len() as u64 + }, + truncated, + } +} + +/// Every event the card emits, as text: `saved::`, `open::`. +fn record_events(card: &Entity, cx: &mut VisualTestContext) -> Rc>> { + let events = Rc::new(RefCell::new(Vec::new())); + let sink = events.clone(); + cx.update(|_, cx| { + cx.subscribe(card, move |_, event: &TasksCardEvent, _| { + let text = match event { + TasksCardEvent::TaskLogSaved { task_id, bytes, .. } => format!( + "saved:{task_id}:{}", + bytes.map_or("none".to_string(), |b| b.to_string()) + ), + TasksCardEvent::OpenLog { task_id, path, .. } => { + format!("open:{task_id}:{}", path.display()) + } + _ => return, + }; + sink.borrow_mut().push(text); + }) + .detach() + }); + events +} + +fn draw(cx: &mut VisualTestContext) { + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); +} + +fn mode(path: &Path) -> u32 { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(path).unwrap().permissions().mode() & 0o777 +} + +async fn seed_fake_copy(path: &Path, text: &str, cx: &mut VisualTestContext) { + let fs = cx.update(|_, cx| ::global(cx)); + // FakeFs::insert_file needs the parent directory to exist (Run-mode fix, story 041 6.2). + fs.create_dir(path.parent().unwrap()).await.unwrap(); + fs.as_fake() + .insert_file(path, text.as_bytes().to_vec()) + .await; +} + +fn active_editor( + workspace: &Entity, + cx: &mut VisualTestContext, +) -> Option<(bool, String)> { + workspace.update(cx, |workspace, cx| { + workspace + .active_item_as::(cx) + .map(|editor| { + let editor = editor.read(cx); + (editor.read_only(cx), editor.text(cx)) + }) + }) +} + +// ---- R5.1 — where "Open Log" is offered --------------------------------------------------- + +// Hostile first: no output, or not a shell, means no button. +#[gpui::test] +async fn test_open_log_is_offered_only_on_shell_rows_with_output(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks( + &a, + vec![ + shell("with", "running", true), + shell("without", "running", false), + subagent("sub", "running"), + ], + &mut cx, + ); + draw(&mut cx); + assert!(cx.debug_bounds("tasks-card-open-log-with").is_some()); + assert!(cx.debug_bounds("tasks-card-open-log-without").is_none()); + assert!(cx.debug_bounds("tasks-card-open-log-sub").is_none()); +} + +// ---- R5.2, R6.4 — open, save, read-only --------------------------------------------------- + +#[gpui::test] +async fn test_open_log_fetches_saves_0600_and_opens_read_only(cx: &mut TestAppContext) { + let (panel, workspace, mut cx) = setup_card_panel_in_workspace(cx).await; + let scratch = Scratch::new("open"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running", true)], &mut cx); + let sa = session_of(&a, &cx); + let output = out("line 1\nline 2\n", false); + connection.answer(&sa, "t1", Ok(output.clone())); + let path = log_path(&scratch.root(), &sa.to_string(), "t1"); + let rendered = render_log(&output); + seed_fake_copy(&path, &rendered, &mut cx).await; + let card = card(&panel, &cx); + let events = record_events(&card, &mut cx); + + card.update_in(&mut cx, |card, window, cx| { + card.open_log(sa.clone(), "t1".into(), window, cx) + }); + cx.run_until_parked(); + + assert_eq!( + connection.fetches(), + vec![(sa.to_string(), "t1".to_string())] + ); + assert_eq!(std::fs::read_to_string(&path).unwrap(), rendered); + assert_eq!(mode(&path), 0o600); + assert_eq!( + *events.borrow(), + vec![ + format!("saved:t1:{}", rendered.len()), + format!("open:t1:{}", path.display()), + ] + ); + let (read_only, text) = active_editor(&workspace, &mut cx).expect("an editor tab opened"); + assert!(read_only, "the log opens read-only"); + assert_eq!(text, rendered); + assert!(find_row(&card, &sa, "t1", &mut cx).unwrap().error.is_none()); + + // Opening again refreshes the one copy. + let newer = out("line 1\nline 2\nline 3\n", false); + connection.answer(&sa, "t1", Ok(newer.clone())); + card.update_in(&mut cx, |card, window, cx| { + card.open_log(sa.clone(), "t1".into(), window, cx) + }); + cx.run_until_parked(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), render_log(&newer)); +} + +// ---- R5.3 — failures open nothing and say why --------------------------------------------- + +#[gpui::test] +async fn test_open_log_fetch_failure_shows_the_reason_and_opens_nothing(cx: &mut TestAppContext) { + let (panel, workspace, mut cx) = setup_card_panel_in_workspace(cx).await; + let scratch = Scratch::new("fetch-fails"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running", true)], &mut cx); + let sa = session_of(&a, &cx); + connection.answer( + &sa, + "t1", + Err("output of task t1 is no longer available (ENOENT)".into()), + ); + let card = card(&panel, &cx); + let events = record_events(&card, &mut cx); + let before = active_editor(&workspace, &mut cx); + + card.update_in(&mut cx, |card, window, cx| { + card.open_log(sa.clone(), "t1".into(), window, cx) + }); + cx.run_until_parked(); + + let error = find_row(&card, &sa, "t1", &mut cx) + .unwrap() + .error + .expect("the row says why"); + assert!(error.starts_with("Could not open the log: "), "{error}"); + assert!(error.contains("no longer available (ENOENT)"), "{error}"); + assert!(events.borrow().is_empty(), "nothing saved, nothing opened"); + assert!(!log_path(&scratch.root(), &sa.to_string(), "t1").exists()); + assert_eq!(active_editor(&workspace, &mut cx), before); +} + +#[gpui::test] +async fn test_open_log_write_failure_shows_the_reason_and_opens_nothing(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let scratch = Scratch::new("write-fails"); + // The log root is occupied by a file, so no directory can be created under it. + std::fs::write(scratch.root(), "not a directory").unwrap(); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running", true)], &mut cx); + let sa = session_of(&a, &cx); + connection.answer(&sa, "t1", Ok(out("SECRET-OUTPUT\n", false))); + let card = card(&panel, &cx); + let events = record_events(&card, &mut cx); + + card.update_in(&mut cx, |card, window, cx| { + card.open_log(sa.clone(), "t1".into(), window, cx) + }); + cx.run_until_parked(); + + let error = find_row(&card, &sa, "t1", &mut cx) + .unwrap() + .error + .expect("the row says why"); + assert!(error.starts_with("Could not open the log: "), "{error}"); + assert!(!error.contains("SECRET-OUTPUT"), "{error}"); + assert!(events.borrow().is_empty()); +} + +// Hostile (wrong collapse): two threads' t1 each get their own log. +#[gpui::test] +async fn test_open_log_two_threads_t1_never_share_a_log(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let scratch = Scratch::new("two-threads"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running", true)], &mut cx); + let b = open_thread(&panel, &connection, "B", &mut cx); + send_tasks(&b, vec![shell("t1", "running", true)], &mut cx); + let (sa, sb) = (session_of(&a, &cx), session_of(&b, &cx)); + connection.answer(&sa, "t1", Ok(out("from A\n", false))); + connection.answer(&sb, "t1", Ok(out("from B\n", false))); + let card = card(&panel, &cx); + + for session in [&sa, &sb] { + card.update_in(&mut cx, |card, window, cx| { + card.open_log(session.clone(), "t1".into(), window, cx) + }); + cx.run_until_parked(); + } + + let pa = log_path(&scratch.root(), &sa.to_string(), "t1"); + let pb = log_path(&scratch.root(), &sb.to_string(), "t1"); + assert_ne!(pa, pb); + assert_eq!(std::fs::read_to_string(pa).unwrap(), "from A\n"); + assert_eq!(std::fs::read_to_string(pb).unwrap(), "from B\n"); +} + +// ---- R6.1, R6.7 — the copy on end --------------------------------------------------------- + +#[gpui::test] +async fn test_open_log_an_ended_shell_with_output_is_copied_without_opening( + cx: &mut TestAppContext, +) { + let (panel, workspace, mut cx) = setup_card_panel_in_workspace(cx).await; + let scratch = Scratch::new("copy-on-end"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running", true)], &mut cx); + let sa = session_of(&a, &cx); + let output = out("line 2\nline 3\n", true); + connection.answer(&sa, "t1", Ok(output.clone())); + let card = card(&panel, &cx); + let events = record_events(&card, &mut cx); + let before = active_editor(&workspace, &mut cx); + + send_tasks(&a, vec![shell("t1", "completed", true)], &mut cx); + // The adapter drops the ended task from its next snapshot: the copy must not depend on it. + send_tasks(&a, vec![], &mut cx); + + let path = log_path(&scratch.root(), &sa.to_string(), "t1"); + let rendered = render_log(&output); + assert_eq!(std::fs::read_to_string(&path).unwrap(), rendered); + assert!(rendered.starts_with("[earlier output truncated: ")); + assert_eq!( + *events.borrow(), + vec![format!("saved:t1:{}", rendered.len())], + "no OpenLog" + ); + assert_eq!(active_editor(&workspace, &mut cx), before, "nothing opened"); +} + +// Hostile: no output, or a subagent, is never fetched at its end. +#[gpui::test] +async fn test_open_log_ended_tasks_without_output_are_not_fetched(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let scratch = Scratch::new("no-copy"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks( + &a, + vec![shell("quiet", "running", false), subagent("sub", "running")], + &mut cx, + ); + send_tasks( + &a, + vec![ + shell("quiet", "completed", false), + subagent("sub", "completed"), + ], + &mut cx, + ); + assert!(connection.fetches().is_empty()); + assert!( + !scratch.root().exists(), + "no log directory is created for nothing" + ); +} + +#[gpui::test] +async fn test_open_log_failed_copy_on_end_records_nothing_and_marks_no_row( + cx: &mut TestAppContext, +) { + let (panel, mut cx) = setup_card_panel(cx).await; + let scratch = Scratch::new("copy-fails"); + use_scratch_logs_root(&scratch.root(), &mut cx); + let connection = OutputConnection::new(); + let a = open_thread(&panel, &connection, "A", &mut cx); + send_tasks(&a, vec![shell("t1", "running", true)], &mut cx); + let sa = session_of(&a, &cx); + connection.answer(&sa, "t1", Err("gone".into())); + let card = card(&panel, &cx); + let events = record_events(&card, &mut cx); + + send_tasks(&a, vec![shell("t1", "completed", true)], &mut cx); + + assert_eq!( + connection.fetches(), + vec![(sa.to_string(), "t1".to_string())] + ); + assert!( + events.borrow().is_empty(), + "no TaskLogSaved for a failed copy" + ); + assert!( + find_row(&card, &sa, "t1", &mut cx).unwrap().error.is_none(), + "nobody asked" + ); +} diff --git a/crates/agent_ui/src/tasks_card/open_subagent_tests.rs b/crates/agent_ui/src/tasks_card/open_subagent_tests.rs new file mode 100644 index 0000000..c6f9be0 --- /dev/null +++ b/crates/agent_ui/src/tasks_card/open_subagent_tests.rs @@ -0,0 +1,197 @@ +// Story 041, task 6.3 — "Open Thread" on a subagent row: the subagent's own thread when the parent +// knows it, else the parent thread at the subagent's tool call with a notice (R5.4, R5.5, R5.6; +// Q16). Filter: `tasks_card::open_subagent`. +// +// Wired in `src/tasks_card.rs`: +// +// #[cfg(test)] +// #[path = "tasks_card/open_subagent_tests.rs"] +// mod open_subagent; +// +// Contract (design.md §`TasksCard` changes) and what this file assumes beyond it: +// - `TasksCard::open_subagent(session_id, task_id, window, cx)`, public like `open_task`, emitting +// `TasksCardEvent::OpenSubagent { session_id, task_id, tool_call_id }`; the agent panel resolves +// the subagent session from the root thread's tool call `subagent_session_info` and calls +// `ConversationView::navigate_to_thread`; +// - the parent thread learns a subagent session from the tool call's `_meta` under +// `acp_thread::SUBAGENT_SESSION_INFO_META_KEY` (the key `subagent_session_info_from_meta` reads; +// made `pub` if it is not); +// - debug selector `tasks-card-open-thread-{id}`; +// - the R5.6 notice is shown in the row's message slot (`TaskRow::error`, the card's only per-row +// text); if the implementation adds a separate notice field, read it in `row_message` only; +// - `crate::test_support::active_session_id` reports the session of the thread the panel shows. +// +// Hostile halves first: a shell row gets no "Open Thread"; a subagent whose tool call carries no +// session info must not navigate to some other thread — it opens the parent at its own call and +// says so; with two subagents, each opens its own session, never the other's. + +use acp_thread::AcpThread; +use agent_client_protocol::v1::schema as acp; +use agent_client_protocol::v2::schema as acp_v2; +use gpui::{Entity, TestAppContext, VisualTestContext}; +use serde_json::json; + +use super::TaskRow; +use super::test_helpers::*; +use crate::AgentPanel; +use crate::test_support::active_session_id; + +const NO_OWN_THREAD: &str = + "Opened the parent thread; this subagent has no thread of its own to show"; + +fn subagent(id: &str, tool_call_id: &str) -> serde_json::Value { + let mut task = wire_task(id, "running", false, 1_000); + task["type"] = json!("subagent"); + task["toolCallId"] = json!(tool_call_id); + task["hasOutput"] = json!(false); + task["tail"] = json!(null); + task +} + +/// The Agent tool call that spawned a subagent; `subagent_session` fills its session info. +fn push_agent_call( + panel: &Entity, + thread: &Entity, + id: &str, + subagent_session: Option<&str>, + cx: &mut VisualTestContext, +) { + let mut call = acp::ToolCall::new(id.to_string(), format!("Agent {id}")); + if let Some(session) = subagent_session { + let mut meta = serde_json::Map::new(); + meta.insert( + acp_thread::SUBAGENT_SESSION_INFO_META_KEY.to_string(), + json!({ "session_id": session, "message_start_index": 0 }), + ); + call = call.meta(meta); + } + thread.update(cx, |thread, cx| { + thread + .handle_session_update(acp::SessionUpdate::ToolCall(call), cx) + .expect("tool call applies"); + }); + // Run-mode fix, story 041 6.3: load the subagent's session into the parent's conversation. + if let Some(session) = subagent_session { + let parent = thread.read_with(cx, |thread, _| thread.session_id().clone()); + let conversation = panel + .read_with(cx, |panel, _| panel.active_conversation_view().cloned()) + .expect("the parent's conversation is active"); + conversation.update(cx, |view, cx| { + view.load_subagent_session_for_test(acp_v2::SessionId::new(session), parent, cx) + }); + } + cx.run_until_parked(); +} + +fn row_message(row: &TaskRow) -> Option { + row.error.as_ref().map(|message| message.to_string()) +} + +fn draw(cx: &mut VisualTestContext) { + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); +} + +// ---- R5.4 — where "Open Thread" is offered ------------------------------------------------ + +#[gpui::test] +async fn test_open_subagent_is_offered_only_on_subagent_rows(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + let mut sh = wire_task("sh", "running", true, 1_000); + sh["hasOutput"] = json!(true); + sh["tail"] = json!(["x"]); + send_tasks(&a, vec![sh, subagent("sub", "toolu_sub")], &mut cx); + draw(&mut cx); + assert!(cx.debug_bounds("tasks-card-open-thread-sub").is_some()); + assert!(cx.debug_bounds("tasks-card-open-thread-sh").is_none()); +} + +// ---- R5.6 — hostile first: no session info, no wrong navigation --------------------------- + +#[gpui::test] +async fn test_open_subagent_without_its_session_opens_the_parent_and_says_so( + cx: &mut TestAppContext, +) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + // Another subagent call WITH a session sits in the same thread; it must not be borrowed. + push_agent_call( + &panel, + &a, + "toolu_other", + Some("other-subagent-session"), + &mut cx, + ); + push_agent_call(&panel, &a, "toolu_sub", None, &mut cx); + send_tasks(&a, vec![subagent("sub", "toolu_sub")], &mut cx); + let sa = session_of(&a, &cx); + let _b = open_titled_thread(&panel, &connection, "B", &mut cx); + + let card = card(&panel, &cx); + card.update_in(&mut cx, |card, window, cx| { + card.open_subagent(sa.clone(), "sub".into(), window, cx) + }); + cx.run_until_parked(); + + assert_eq!( + active_session_id(&panel, &cx), + sa, + "the parent thread is brought forward" + ); + assert_ne!( + active_session_id(&panel, &cx), + acp_v2::SessionId::new("other-subagent-session") + ); + let row = find_row(&card, &sa, "sub", &mut cx).expect("the row stays listed"); + assert_eq!(row_message(&row).as_deref(), Some(NO_OWN_THREAD)); +} + +// ---- R5.5 — the subagent's own thread ----------------------------------------------------- + +#[gpui::test] +async fn test_open_subagent_shows_its_own_thread_in_this_window(cx: &mut TestAppContext) { + let (panel, mut cx) = setup_card_panel(cx).await; + let connection = TaskActionsConnection::new(); + let a = open_titled_thread(&panel, &connection, "A", &mut cx); + push_agent_call( + &panel, + &a, + "toolu_one", + Some("subagent-session-one"), + &mut cx, + ); + push_agent_call( + &panel, + &a, + "toolu_two", + Some("subagent-session-two"), + &mut cx, + ); + send_tasks( + &a, + vec![subagent("one", "toolu_one"), subagent("two", "toolu_two")], + &mut cx, + ); + let sa = session_of(&a, &cx); + let card = card(&panel, &cx); + + card.update_in(&mut cx, |card, window, cx| { + card.open_subagent(sa.clone(), "two".into(), window, cx) + }); + cx.run_until_parked(); + + assert_eq!( + active_session_id(&panel, &cx), + acp_v2::SessionId::new("subagent-session-two"), + "its own session, not the other subagent's" + ); + let row = find_row(&card, &sa, "two", &mut cx).expect("the row stays listed"); + assert_eq!( + row_message(&row), + None, + "no notice when the thread was shown" + ); +} diff --git a/crates/agent_ui/src/tasks_card/task_log.rs b/crates/agent_ui/src/tasks_card/task_log.rs new file mode 100644 index 0000000..6323643 --- /dev/null +++ b/crates/agent_ui/src/tasks_card/task_log.rs @@ -0,0 +1,349 @@ +//! Where an agent task's saved log lives, and how it is written (story 041). +//! +//! A shell task's output is copied here when it ends, and when the operator opens its log, so the +//! log outlives the agent's temporary file. A log can hold secrets, so it is written mode 0600 +//! inside directories of mode 0700, replaced atomically, capped at [`LOG_MAX_BYTES`], deleted with +//! its task record and swept when its record is gone. No log line carries its content. + +use std::collections::HashSet; +use std::fs; +use std::io::Write as _; +use std::path::{Path, PathBuf}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use acp_thread::TaskOutput; +use anyhow::{Context as _, Result}; +use gpui::{App, Global}; + +/// The most output a log keeps; an agent that sends more is cut again at a line boundary. +pub const LOG_MAX_BYTES: usize = 1_048_576; + +/// What a row says when "Open Log" is asked in a remote project (R5.10, R5.11): a log is a local +/// file, and opening it there would resolve its path on the remote host instead. +pub const REMOTE_PROJECT_NOTICE: &str = "Open Log is not available in remote projects"; + +const LOG_EXTENSION: &str = "log"; +const TEMP_MARKER: &str = ".log.tmp-"; + +/// `/agent_task_logs`. +pub fn logs_root() -> PathBuf { + paths::data_dir().join("agent_task_logs") +} + +/// Points [`logs_root_in`] somewhere else. Tests only: production never sets it. +pub struct LogsRootOverride(pub PathBuf); + +impl Global for LogsRootOverride {} + +/// The logs root every caller uses: [`LogsRootOverride`] when a test set one, else +/// [`logs_root`] scoped to this build's release channel. +/// +/// Scoped by channel because the task history is: its database lives under +/// `db/0-`, and the orphan sweep keeps only the logs of the records ONE history holds. +/// A shared root would let a dev or preview build sweep the stable build's logs. +/// +/// Test builds never resolve the real data directory: without an override they get a +/// per-process directory under the system temp dir, so a test that builds a history store +/// cannot sweep the operator's logs. +pub fn logs_root_in(cx: &App) -> PathBuf { + if let Some(root) = cx.try_global::() { + return root.0.clone(); + } + #[cfg(any(test, feature = "test-support"))] + { + std::env::temp_dir().join(format!("zeo-agent-task-logs-test-{}", std::process::id())) + } + #[cfg(not(any(test, feature = "test-support")))] + { + logs_root().join(release_channel::RELEASE_CHANNEL.dev_name()) + } +} + +/// `root//.log`, with each id mapped to a file-name component that holds no +/// separator and no `..`, and that no other id maps to. +pub fn log_path(root: &Path, session_id: &str, task_id: &str) -> PathBuf { + root.join(component(session_id)) + .join(format!("{}.{LOG_EXTENSION}", component(task_id))) +} + +/// An id as a file-name component. Three forms that cannot meet: +/// - a plain id (`^[A-Za-z0-9_-]{1,128}$`) is kept as it is, and never contains `@`; +/// - any other id is `@` + the lowercase hex of its bytes, when that is at most 200 characters; +/// - a longer one is `@h` + its FNV-1a 128-bit hash (`h` is not a hex digit). +/// +/// The first two forms are injective. The hash form is not collision-resistant: two ids over +/// 100 bytes crafted to collide would share a file. Ids come from the agent the operator runs, +/// so this is accepted; nothing is ever cut, which would make collisions trivial. +fn component(id: &str) -> String { + let plain = !id.is_empty() + && id.len() <= 128 + && id + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_' || byte == b'-'); + if plain { + return id.to_string(); + } + let hex: String = id.bytes().map(|byte| format!("{byte:02x}")).collect(); + if hex.len() <= 200 { + format!("@{hex}") + } else { + format!("@h{:032x}", fnv1a_128(id.as_bytes())) + } +} + +fn fnv1a_128(bytes: &[u8]) -> u128 { + const OFFSET_BASIS: u128 = 0x6c62272e07bb014262b821756295c58d; + const PRIME: u128 = 0x0000000001000000000000000000013b; + bytes.iter().fold(OFFSET_BASIS, |hash, byte| { + (hash ^ u128::from(*byte)).wrapping_mul(PRIME) + }) +} + +/// The log text for `output`: the content, capped at [`LOG_MAX_BYTES`] on a line boundary, under +/// a `[earlier output truncated: N bytes]` header whenever bytes of the output are not in it. +pub fn render_log(output: &TaskOutput) -> String { + let content = output.content.as_str(); + let (body, cut) = if content.len() > LOG_MAX_BYTES { + (cap_at_line_boundary(content), true) + } else { + (content, false) + }; + if !output.truncated && !cut { + return body.to_string(); + } + let total = output.total_bytes.max(content.len() as u64); + let dropped = total.saturating_sub(body.len() as u64); + format!("[earlier output truncated: {dropped} bytes]\n{body}") +} + +/// The last `LOG_MAX_BYTES` of `content`, starting on a line (never inside a line or a UTF-8 +/// sequence). A window with no newline keeps whatever whole characters fit. +fn cap_at_line_boundary(content: &str) -> &str { + let start = content.len() - LOG_MAX_BYTES; + if content.as_bytes()[start - 1] == b'\n' { + return &content[start..]; + } + // `start` is a byte offset and may fall inside a multibyte character (U+FFFD from the + // adapter's lossy decode is three bytes), so the newline is searched in bytes: the byte + // after a `\n` always begins a character. + match content.as_bytes()[start..] + .iter() + .position(|byte| *byte == b'\n') + { + Some(newline) => &content[start + newline + 1..], + None => { + let mut start = start; + while !content.is_char_boundary(start) { + start += 1; + } + &content[start..] + } + } +} + +/// Writes `text` to `path` atomically, mode 0600, its directories tightened to 0700. Returns the +/// bytes written. On failure the temporary file is removed and whatever was at `path` is left as +/// it was; the error names the path, never the text. +pub fn write_log(path: &Path, text: &str) -> Result { + write_log_inner(path, text).with_context(|| format!("writing task log {}", path.display())) +} + +fn write_log_inner(path: &Path, text: &str) -> Result { + let session_dir = path + .parent() + .context("a task log path has a session directory")?; + let root = session_dir + .parent() + .context("a task log path has a root directory")?; + create_private_dirs(session_dir)?; + tighten(root)?; + tighten(session_dir)?; + + let file_name = path + .file_name() + .context("a task log path has a file name")? + .to_string_lossy(); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|elapsed| elapsed.as_nanos()) + .unwrap_or_default(); + // Same directory as the target: `rename` is atomic only within one filesystem. + let temporary = session_dir.join(format!( + "{}{}{}-{nanos}", + file_name.trim_end_matches(".log"), + TEMP_MARKER, + std::process::id() + )); + let result = write_and_rename(&temporary, path, text.as_bytes()); + if result.is_err() { + fs::remove_file(&temporary).ok(); + } + result +} + +fn write_and_rename(temporary: &Path, path: &Path, bytes: &[u8]) -> Result { + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + let mut file = options + .open(temporary) + .context("creating the temporary log file")?; + file.write_all(bytes) + .context("writing the temporary log file")?; + file.sync_all().context("syncing the temporary log file")?; + drop(file); + fs::rename(temporary, path).context("moving the log into place")?; + Ok(bytes.len() as u64) +} + +/// Creates the directories at mode 0700 from the start, so none is ever wider in between. +fn create_private_dirs(dir: &Path) -> Result<()> { + let mut builder = fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt as _; + builder.mode(0o700); + } + builder.create(dir).context("creating the log directories") +} + +/// A directory that already existed may be wider than 0700. A symlink is refused rather than +/// followed: `set_permissions` would change the mode of whatever it points to. +fn tighten(dir: &Path) -> Result<()> { + let metadata = + fs::symlink_metadata(dir).with_context(|| format!("reading {}", dir.display()))?; + anyhow::ensure!( + !metadata.file_type().is_symlink(), + "{} is a symlink", + dir.display() + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + fs::set_permissions(dir, fs::Permissions::from_mode(0o700)) + .with_context(|| format!("restricting {} to 0700", dir.display()))?; + } + Ok(()) +} + +/// Deletes the log of `(session_id, task_id)`. A missing file is success; a session directory +/// left empty is removed. +pub fn delete_log(root: &Path, session_id: &str, task_id: &str) -> Result<()> { + let path = log_path(root, session_id, task_id); + match fs::remove_file(&path) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + return Err(error).with_context(|| format!("deleting task log {}", path.display())); + } + } + if let Some(session_dir) = path.parent() + && fs::read_dir(session_dir).is_ok_and(|mut entries| entries.next().is_none()) + { + fs::remove_dir(session_dir).ok(); + } + Ok(()) +} + +/// Like [`delete_log`], but leaves a file modified after `heard_at`: a "no log" decided before +/// a newer successful save must not delete that save's file. +pub fn delete_log_unless_newer( + root: &Path, + session_id: &str, + task_id: &str, + heard_at: SystemTime, +) -> Result { + let path = log_path(root, session_id, task_id); + let newer = fs::metadata(&path) + .and_then(|metadata| metadata.modified()) + .is_ok_and(|modified| modified > heard_at); + if newer { + return Ok(false); + } + delete_log(root, session_id, task_id).map(|()| true) +} + +/// What [`sweep`] did. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct SweepReport { + pub removed: usize, + pub kept: usize, + pub other: usize, +} + +/// Removes every log and temporary log under `root` that belongs to no key in `keep`, unless it +/// was modified after `newer_than` (a log saved while the history was loading). Files with any +/// other name are left alone and counted. A missing root is nothing to sweep. +pub fn sweep( + root: &Path, + keep: &HashSet<(String, String)>, + newer_than: SystemTime, +) -> Result { + let mut report = SweepReport::default(); + let sessions = match fs::read_dir(root) { + Ok(sessions) => sessions, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(report), + Err(error) => { + return Err(error).with_context(|| format!("reading {}", root.display())); + } + }; + // Forward mapping: a hashed name cannot be mapped back to its id. + let kept_paths: HashSet = keep + .iter() + .map(|(session, task)| log_path(root, session, task)) + .collect(); + for session in sessions.filter_map(|entry| entry.ok()) { + let session_dir = session.path(); + // `DirEntry::file_type` does not follow symlinks: a link planted under the root must + // not lead the sweep into another directory. + if !session.file_type().is_ok_and(|kind| kind.is_dir()) { + report.other += 1; + continue; + } + let Ok(files) = fs::read_dir(&session_dir) else { + continue; + }; + for file in files.filter_map(|entry| entry.ok()) { + let path = file.path(); + let name = file.file_name().to_string_lossy().into_owned(); + let is_file = file.file_type().is_ok_and(|kind| kind.is_file()); + let is_log = is_file && (name.ends_with(".log") || name.contains(TEMP_MARKER)); + if !is_log { + report.other += 1; + continue; + } + let fresh = file + .metadata() + .and_then(|metadata| metadata.modified()) + .is_ok_and(|modified| modified > newer_than); + if kept_paths.contains(&path) || fresh { + report.kept += 1; + continue; + } + match fs::remove_file(&path) { + Ok(()) => report.removed += 1, + Err(error) => log::warn!( + "agent_task_log event=failed path={} reason=sweep error={error:#}", + path.display() + ), + } + } + if fs::read_dir(&session_dir).is_ok_and(|mut entries| entries.next().is_none()) { + fs::remove_dir(&session_dir).ok(); + } + } + Ok(report) +} + +#[cfg(test)] +#[path = "task_log_tests.rs"] +mod tests; + +#[cfg(test)] +#[path = "task_log_review_tests.rs"] +mod review_tests; diff --git a/crates/agent_ui/src/tasks_card/task_log_review_tests.rs b/crates/agent_ui/src/tasks_card/task_log_review_tests.rs new file mode 100644 index 0000000..47bb9ef --- /dev/null +++ b/crates/agent_ui/src/tasks_card/task_log_review_tests.rs @@ -0,0 +1,107 @@ +// Story 041, group 5 tech review — regressions for what the review found in `task_log`: +// B1 (a test build must never resolve the real data directory), S2 (a stale "no log" must not +// delete a newer save), N1 (symlinks under the root are neither followed nor tightened). + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::PathBuf; +use std::time::{Duration, SystemTime}; + +use gpui::TestAppContext; + +use super::*; + +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|elapsed| elapsed.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-task-log-review-{name}-{}-{nanos}", + std::process::id() + )); + fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + fs::remove_dir_all(&self.0).ok(); + } +} + +// B1: without an override, a test build gets a private temp root, never `/…`. +#[gpui::test] +fn task_log_review_test_build_never_resolves_the_real_data_dir(cx: &mut TestAppContext) { + cx.update(|cx| { + let root = logs_root_in(cx); + assert!(root.starts_with(std::env::temp_dir()), "{root:?}"); + assert!(!root.starts_with(paths::data_dir()), "{root:?}"); + + cx.set_global(LogsRootOverride(PathBuf::from("/override"))); + assert_eq!(logs_root_in(cx), PathBuf::from("/override")); + }); +} + +// S2: a "no log" heard before a newer save leaves that save; one heard after deletes it. +#[test] +fn task_log_review_stale_no_log_keeps_a_newer_save_and_deletes_an_older_one() { + let scratch = Scratch::new("stale"); + let root = scratch.0.join("logs"); + let path = log_path(&root, "s1", "t1"); + write_log(&path, "fresh\n").unwrap(); + + let before_the_save = SystemTime::now() - Duration::from_secs(60); + assert!(!delete_log_unless_newer(&root, "s1", "t1", before_the_save).unwrap()); + assert!(path.exists(), "a save newer than the event is kept"); + + let after_the_save = SystemTime::now() + Duration::from_secs(60); + assert!(delete_log_unless_newer(&root, "s1", "t1", after_the_save).unwrap()); + assert!(!path.exists()); +} + +// N1: a session entry that is a symlink to another directory is not swept into. +#[test] +fn task_log_review_sweep_does_not_follow_a_symlinked_session_directory() { + let scratch = Scratch::new("sweep-link"); + let root = scratch.0.join("logs"); + let outside = scratch.0.join("outside"); + fs::create_dir_all(&root).unwrap(); + fs::create_dir_all(&outside).unwrap(); + fs::write(outside.join("precious.log"), "not ours").unwrap(); + std::os::unix::fs::symlink(&outside, root.join("planted")).unwrap(); + + sweep( + &root, + &HashSet::new(), + SystemTime::now() + Duration::from_secs(3600), + ) + .unwrap(); + + assert!(outside.join("precious.log").exists()); +} + +// N1: a session directory that is a symlink is refused, and its target's mode is left alone. +#[test] +fn task_log_review_write_refuses_a_symlinked_session_directory() { + let scratch = Scratch::new("write-link"); + let root = scratch.0.join("logs"); + let outside = scratch.0.join("outside"); + fs::create_dir_all(&root).unwrap(); + fs::create_dir_all(&outside).unwrap(); + fs::set_permissions(&outside, fs::Permissions::from_mode(0o755)).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("s1")).unwrap(); + + let error = write_log(&log_path(&root, "s1", "t1"), "x\n").expect_err("a symlink is refused"); + assert!(format!("{error:#}").contains("symlink"), "{error:#}"); + let mode = fs::metadata(&outside).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o755); + assert!( + fs::read_dir(&outside).unwrap().next().is_none(), + "nothing written there" + ); +} diff --git a/crates/agent_ui/src/tasks_card/task_log_tests.rs b/crates/agent_ui/src/tasks_card/task_log_tests.rs new file mode 100644 index 0000000..76acc2a --- /dev/null +++ b/crates/agent_ui/src/tasks_card/task_log_tests.rs @@ -0,0 +1,431 @@ +// Story 041, task 5.1 — `task_log`: where a task's log lives, how it is written (mode 0600 in +// 0700 directories, replaced atomically), the 1 MiB cap, delete and the orphan sweep (R6.2, R6.3, +// R6.4, R6.6, R6.7; Q4, Q15, Q18). +// +// Wired in `src/tasks_card/task_log.rs` as +// +// #[cfg(test)] +// #[path = "task_log_tests.rs"] +// mod tests; +// +// (a `#[path]` in the non-mod-rs file `tasks_card/task_log.rs` resolves next to it), so every test +// path is `tasks_card::task_log::tests::…` (filter `cargo test -p agent_ui task_log`). +// +// Contract (design.md §`task_log`): `LOG_MAX_BYTES`, `logs_root()`, `log_path(root, session_id, +// task_id) -> PathBuf`, `render_log(&TaskOutput) -> String`, `write_log(path, text) -> +// Result`, `delete_log(root, session_id, task_id) -> Result<()>`, `sweep(root, &HashSet<(String, +// String)>) -> Result`. Plain `#[test]`s over a private temp directory; nothing here +// touches `paths::data_dir()` except the one `logs_root` assertion, which only computes a path. +// +// Hostile halves first, and one of them is aimed at a value the design DERIVES: `component()` +// keeps an id that matches `^[A-Za-z0-9_-]{1,128}$` and writes any other as `x` + lowercase hex, +// cut to 200 characters. A third id can reach the same rendered name by another route: +// - `..` escapes to `x2e2e`, and the id `x2e2e` is a valid bare name that renders as itself; +// - two 129-character ids differing only in their last character both escape to 259 +// characters and are cut to the same 200. +// Two different ids must never share a file — the record of one would open, or delete, the log +// of the other. These tests fail against design.md's `component()` as written; that is the point. + +use std::collections::HashSet; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::time::{Duration, SystemTime}; + +/// A `newer_than` after every file a test writes, so the age guard keeps nothing by itself. +fn later() -> SystemTime { + SystemTime::now() + Duration::from_secs(3600) +} + +use acp_thread::TaskOutput; + +use super::*; + +/// A private directory removed on drop (no extra dev-dependency). +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let dir = std::env::temp_dir().join(format!( + "zeo-task-log-{name}-{}-{nanos}", + std::process::id() + )); + fs::create_dir_all(&dir).expect("scratch dir"); + Scratch(dir) + } + + fn root(&self) -> PathBuf { + self.0.join("agent_task_logs") + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +fn mode(path: &Path) -> u32 { + fs::metadata(path).expect("metadata").permissions().mode() & 0o777 +} + +fn names_in(dir: &Path) -> Vec { + let mut names: Vec = fs::read_dir(dir) + .map(|entries| { + entries + .filter_map(|entry| entry.ok()) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .collect() + }) + .unwrap_or_default(); + names.sort(); + names +} + +fn output(content: &str, total_bytes: u64, truncated: bool) -> TaskOutput { + TaskOutput { + content: content.to_string(), + total_bytes, + truncated, + } +} + +// ---- log_path: ids never collide, never escape the root ------------------------------------ + +fn assert_inside(root: &Path, path: &Path) { + let relative = path + .strip_prefix(root) + .unwrap_or_else(|_| panic!("{path:?} is outside {root:?}")); + let parts: Vec<_> = relative.components().collect(); + assert_eq!(parts.len(), 2, "root//.log, got {path:?}"); + for part in parts { + let text = part.as_os_str().to_string_lossy(); + assert!( + !text.contains('/') && text != ".." && text != ".", + "{path:?}" + ); + } + assert!(path.to_string_lossy().ends_with(".log"), "{path:?}"); +} + +// Hostile (derived value): a third id must not reach another id's rendered name. +#[test] +fn task_log_ids_that_look_like_each_others_escape_get_distinct_files() { + let root = PathBuf::from("/logs"); + let long = |last: char| format!("{}{last}", "a".repeat(128)); + let ids: Vec = vec![ + "..".into(), + "x2e2e".into(), // the hex escape of ".." spelled as a valid bare id + ".".into(), + "x2e".into(), // the hex escape of "." + "a/b".into(), + "a_b".into(), + "a%2Fb".into(), + "x612f62".into(), // the hex escape of "a/b" + "tâche".into(), + "".into(), + long('b'), // 129 characters: escaped, then cut + long('c'), + format!("{}/1", "z".repeat(150)), + format!("{}/2", "z".repeat(150)), + ]; + let mut seen: HashSet = HashSet::new(); + for id in &ids { + let path = log_path(&root, "session", id); + assert_inside(&root, &path); + assert!(seen.insert(path.clone()), "{id:?} collides at {path:?}"); + } + // The same for session ids: the directory level must not collide either. + let mut sessions: HashSet = HashSet::new(); + for id in &ids { + let path = log_path(&root, id, "t1"); + assert_inside(&root, &path); + assert!( + sessions.insert(path.parent().unwrap().to_path_buf()), + "session {id:?} collides" + ); + } +} + +#[test] +fn task_log_path_is_deterministic_and_plain_ids_stay_readable() { + let root = PathBuf::from("/logs"); + let path = log_path(&root, "6f1c2d3e-aaaa-bbbb", "b7k2m9x1q"); + assert_eq!(path, root.join("6f1c2d3e-aaaa-bbbb").join("b7k2m9x1q.log")); + assert_eq!(path, log_path(&root, "6f1c2d3e-aaaa-bbbb", "b7k2m9x1q")); +} + +#[test] +fn task_log_root_is_agent_task_logs_in_the_data_dir() { + assert_eq!(logs_root(), paths::data_dir().join("agent_task_logs")); +} + +// ---- render_log: the cap (R6.2) ------------------------------------------------------------ + +#[test] +fn task_log_render_keeps_untruncated_output_verbatim() { + assert_eq!(render_log(&output("a\nb\n", 4, false)), "a\nb\n"); +} + +#[test] +fn task_log_render_names_the_dropped_bytes_on_its_first_line() { + let content = "line 2\nline 3\n"; + let rendered = render_log(&output(content, 2_097_152, true)); + let dropped = 2_097_152 - content.len() as u64; + assert_eq!( + rendered, + format!("[earlier output truncated: {dropped} bytes]\n{content}") + ); +} + +// Hostile: an agent that sends more than the cap is cut again, at a line boundary. +#[test] +fn task_log_render_caps_an_oversized_answer_at_a_line_boundary() { + let line = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdé\n"; + let content = line.repeat((3 * LOG_MAX_BYTES) / line.len()); + let total = content.len() as u64; + let rendered = render_log(&output(&content, total, false)); + + let (header, body) = rendered.split_once('\n').expect("a header line"); + assert!( + header.starts_with("[earlier output truncated: "), + "{header}" + ); + let dropped = total - body.len() as u64; + assert_eq!( + header, + format!("[earlier output truncated: {dropped} bytes]") + ); + assert!(body.len() <= LOG_MAX_BYTES); + assert!( + body.starts_with(line), + "the kept part starts at a line boundary" + ); + assert!(content.ends_with(body)); + assert_eq!(LOG_MAX_BYTES, 1_048_576); +} + +// Hostile: the raw cut `len - LOG_MAX_BYTES` lands inside a two-byte character and no newline +// follows it, so only whole characters may be kept. +#[test] +fn task_log_cap_without_a_newline_keeps_whole_multibyte_characters() { + let content = "é".repeat(LOG_MAX_BYTES / 2 + 1) + "x"; + assert!(!content.is_char_boundary(content.len() - LOG_MAX_BYTES)); + + let body = cap_at_line_boundary(&content); + + assert_eq!(body.len(), LOG_MAX_BYTES - 1); + assert!(body.starts_with('é')); + assert!(content.ends_with(body)); +} + +// Hostile: the adapter decodes invalid bytes as U+FFFD (three bytes each), so a capped log can +// exceed LOG_MAX_BYTES with the raw cut inside a replacement character. The kept part must +// still start on the next line. +#[test] +fn task_log_cap_inside_a_replacement_character_starts_on_the_next_line() { + let line = "\u{FFFD}".repeat(9) + "\n"; + let content = line.repeat((2 * LOG_MAX_BYTES) / line.len()) + "\u{FFFD}\u{FFFD}"; + assert!(!content.is_char_boundary(content.len() - LOG_MAX_BYTES)); + + let body = cap_at_line_boundary(&content); + + assert!(body.len() <= LOG_MAX_BYTES); + assert!(body.starts_with(&line)); + assert!(content.ends_with(body)); + let cut = content.len() - body.len(); + assert_eq!(&content[cut - 1..cut], "\n"); + assert!(cut - (content.len() - LOG_MAX_BYTES) < line.len()); +} + +#[test] +fn task_log_cap_of_ascii_lines_is_unchanged() { + let line = "0123456789\n"; + let content = "x".repeat(5) + &line.repeat((2 * LOG_MAX_BYTES) / line.len()); + let start = content.len() - LOG_MAX_BYTES; + let expected = &content[start + content[start..].find('\n').map_or(0, |i| i + 1)..]; + assert_eq!(cap_at_line_boundary(&content), expected); + + let aligned = "y".repeat(LOG_MAX_BYTES) + "\n" + &"z".repeat(LOG_MAX_BYTES - 1) + "\n"; + assert_eq!( + cap_at_line_boundary(&aligned), + "z".repeat(LOG_MAX_BYTES - 1) + "\n" + ); +} + +// ---- write_log: modes, replace, failure (R6.3, R6.4, Q18) --------------------------------- + +#[test] +fn task_log_write_creates_0600_in_0700_directories_and_returns_its_size() { + let scratch = Scratch::new("modes"); + let root = scratch.root(); + let path = log_path(&root, "s1", "t1"); + + let bytes = write_log(&path, "hello\n").expect("write succeeds"); + + assert_eq!(bytes, 6); + assert_eq!(fs::read_to_string(&path).unwrap(), "hello\n"); + assert_eq!(mode(&path), 0o600); + assert_eq!(mode(path.parent().unwrap()), 0o700); + assert_eq!(mode(&root), 0o700); +} + +// Hostile: a root that already exists world-readable is tightened, not trusted. +#[test] +fn task_log_write_tightens_an_existing_root() { + let scratch = Scratch::new("tighten"); + let root = scratch.root(); + fs::create_dir_all(root.join("s1")).unwrap(); + fs::set_permissions(&root, fs::Permissions::from_mode(0o755)).unwrap(); + fs::set_permissions(root.join("s1"), fs::Permissions::from_mode(0o755)).unwrap(); + + write_log(&log_path(&root, "s1", "t1"), "x\n").expect("write succeeds"); + assert_eq!(mode(&root), 0o700); + assert_eq!(mode(&root.join("s1")), 0o700); +} + +#[test] +fn task_log_write_again_replaces_the_one_file_and_leaves_no_temporary() { + let scratch = Scratch::new("replace"); + let root = scratch.root(); + let path = log_path(&root, "s1", "t1"); + write_log(&path, "first\n").unwrap(); + write_log(&path, "second\n").unwrap(); + + assert_eq!(fs::read_to_string(&path).unwrap(), "second\n"); + assert_eq!(names_in(path.parent().unwrap()), vec!["t1.log".to_string()]); + assert_eq!(mode(&path), 0o600); +} + +// Q18: the write cannot complete (the target is occupied by a non-empty directory, so the final +// rename fails after the temporary file was written). The error names the ids, carries no text, +// the temporary file is gone, and what occupied the target is untouched — the target is never +// written in place. +#[test] +fn task_log_failed_write_leaves_the_target_untouched_and_no_temporary() { + let scratch = Scratch::new("failed"); + let root = scratch.root(); + let path = log_path(&root, "s1", "t1"); + fs::create_dir_all(&path).unwrap(); + fs::write(path.join("previous"), "previous content").unwrap(); + + let error = write_log(&path, "SECRET-OUTPUT\n").expect_err("the rename cannot succeed"); + let text = format!("{error:#}"); + assert!(text.contains("s1") && text.contains("t1"), "{text}"); + assert!(!text.contains("SECRET-OUTPUT"), "{text}"); + assert_eq!( + fs::read_to_string(path.join("previous")).unwrap(), + "previous content" + ); + assert_eq!( + names_in(path.parent().unwrap()), + vec!["t1.log".to_string()], + "no temporary file is left behind" + ); +} + +// ---- delete_log (R6.5's file half) -------------------------------------------------------- + +// Hostile: deleting (s1, t1) must not touch (s1, t10) or (s2, t1). +#[test] +fn task_log_delete_removes_only_its_own_file() { + let scratch = Scratch::new("delete"); + let root = scratch.root(); + for (session, task) in [("s1", "t1"), ("s1", "t10"), ("s2", "t1")] { + write_log(&log_path(&root, session, task), "x\n").unwrap(); + } + + delete_log(&root, "s1", "t1").expect("delete succeeds"); + + assert!(!log_path(&root, "s1", "t1").exists()); + assert!(log_path(&root, "s1", "t10").exists()); + assert!(log_path(&root, "s2", "t1").exists()); +} + +#[test] +fn task_log_delete_of_a_missing_file_succeeds_and_an_emptied_session_dir_goes() { + let scratch = Scratch::new("delete-empty"); + let root = scratch.root(); + delete_log(&root, "s1", "never-written").expect("NotFound is success"); + + write_log(&log_path(&root, "s1", "t1"), "x\n").unwrap(); + delete_log(&root, "s1", "t1").unwrap(); + assert!( + !root.join("s1").exists(), + "the empty session directory is removed" + ); +} + +// ---- sweep (R6.6) ------------------------------------------------------------------------- + +#[test] +fn task_log_sweep_keeps_kept_keys_and_removes_orphans_and_their_temporaries() { + let scratch = Scratch::new("sweep"); + let root = scratch.root(); + let keep_ids = [("s1", "t1"), ("s/2", "a/b")]; // the second pair is stored escaped + for (session, task) in keep_ids.iter().chain([("s1", "t2"), ("s3", "t1")].iter()) { + write_log(&log_path(&root, session, task), "x\n").unwrap(); + } + let orphan_tmp = log_path(&root, "s1", "t9").with_extension("log.tmp-1-2"); + fs::write(&orphan_tmp, "partial").unwrap(); + let stranger = root.join("s1").join("README"); + fs::write(&stranger, "not a log").unwrap(); + + let keep: HashSet<(String, String)> = keep_ids + .iter() + .map(|(s, t)| (s.to_string(), t.to_string())) + .collect(); + sweep(&root, &keep, later()).expect("sweep succeeds"); + + assert!(log_path(&root, "s1", "t1").exists()); + assert!( + log_path(&root, "s/2", "a/b").exists(), + "an escaped kept key is mapped back" + ); + assert!(!log_path(&root, "s1", "t2").exists()); + assert!(!log_path(&root, "s3", "t1").exists()); + assert!(!orphan_tmp.exists()); + assert!(stranger.exists(), "a name it cannot map is left alone"); +} + +#[test] +fn task_log_sweep_of_a_missing_root_does_nothing() { + let scratch = Scratch::new("sweep-missing"); + let root = scratch.root(); + sweep(&root, &HashSet::new(), later()).expect("nothing to sweep is not an error"); + assert!(!root.exists(), "the sweep creates nothing"); +} + +// Hostile (M1): a log saved while the history was loading belongs to no loaded key yet, but it is +// newer than the load, so the sweep must leave it; an equally unkept older file still goes. +#[test] +fn task_log_sweep_keeps_a_file_written_after_the_load_began() { + let scratch = Scratch::new("sweep-newer"); + let root = scratch.root(); + write_log(&log_path(&root, "s1", "old"), "x\n").unwrap(); + let old = log_path(&root, "s1", "old"); + let past = SystemTime::now() - Duration::from_secs(600); + fs::File::options() + .write(true) + .open(&old) + .unwrap() + .set_modified(past) + .unwrap(); + let loaded_at = SystemTime::now() - Duration::from_secs(60); + write_log(&log_path(&root, "s1", "fresh"), "x\n").unwrap(); + + sweep(&root, &HashSet::new(), loaded_at).expect("sweep succeeds"); + + assert!( + !old.exists(), + "an unkept file older than the load is an orphan" + ); + assert!( + log_path(&root, "s1", "fresh").exists(), + "a file saved during the load is kept" + ); +} diff --git a/crates/agent_ui/src/tasks_card/test_helpers.rs b/crates/agent_ui/src/tasks_card/test_helpers.rs index 0208066..c0d1b0c 100644 --- a/crates/agent_ui/src/tasks_card/test_helpers.rs +++ b/crates/agent_ui/src/tasks_card/test_helpers.rs @@ -180,6 +180,39 @@ impl AgentConnection for TaskActionsConnection { Task::ready(Ok(thread)) } + // Loads a subagent's own session the way `new_session` builds a thread, so the + // conversation can hold and show a subagent thread (story 041, R5.5). + fn supports_load_session(&self) -> bool { + true + } + + fn load_session( + self: Rc, + session_id: acp_v2::SessionId, + project: Entity, + work_dirs: PathList, + _title: Option, + cx: &mut App, + ) -> Task>> { + let action_log = cx.new(|_| action_log::ActionLog::new(project.clone())); + let thread = cx.new(|cx| { + AcpThread::new( + None, + None, + Some(work_dirs), + self.clone(), + project, + action_log, + session_id, + watch::Receiver::constant( + agent_client_protocol::v2::schema::PromptCapabilities::new(), + ), + cx, + ) + }); + Task::ready(Ok(thread)) + } + fn auth_methods(&self) -> &[agent_client_protocol::v2::schema::AuthMethod] { &[] } diff --git a/crates/agent_ui/src/test_support.rs b/crates/agent_ui/src/test_support.rs index 70af3e2..3f454d9 100644 --- a/crates/agent_ui/src/test_support.rs +++ b/crates/agent_ui/src/test_support.rs @@ -294,10 +294,13 @@ pub fn type_draft_prompt(panel: &Entity, text: &str, cx: &mut Visual cx.run_until_parked(); } +/// The session of the thread the panel shows: a subagent's own when one is on +/// screen, else the conversation's root thread. pub fn active_session_id(panel: &Entity, cx: &VisualTestContext) -> acp::SessionId { panel.read_with(cx, |panel, cx| { - let thread = panel.active_agent_thread(cx).unwrap(); - thread.read(cx).session_id().clone() + let conversation = panel.active_conversation_view().unwrap().read(cx); + let view = conversation.active_thread().unwrap(); + view.read(cx).thread.read(cx).session_id().clone() }) }