From b6f83d81bb5928eb43607f43a0c8f961919b8018 Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 29 Jul 2026 02:29:15 +0900 Subject: [PATCH] fix: bound task tool output --- crates/tui/src/task.rs | 66 ++++---- crates/worker/src/feature/builtin/task/mod.rs | 4 +- .../worker/src/feature/builtin/task/store.rs | 111 ++++++++----- .../src/feature/builtin/task/tool_impl.rs | 146 ++++++++++++++++-- crates/worker/src/worker.rs | 2 +- 5 files changed, 240 insertions(+), 89 deletions(-) diff --git a/crates/tui/src/task.rs b/crates/tui/src/task.rs index 1823bfff..537e7853 100644 --- a/crates/tui/src/task.rs +++ b/crates/tui/src/task.rs @@ -27,6 +27,12 @@ pub enum TaskStatus { Deleted, } +impl TaskStatus { + fn is_active(self) -> bool { + matches!(self, Self::Pending | Self::Inprogress) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] pub struct TaskEntry { pub taskid: u64, @@ -106,8 +112,10 @@ impl TaskStore { } "TaskUpdate" => { if let Ok(p) = serde_json::from_str::(arguments) - && let Some(t) = self.tasks.iter_mut().find(|t| t.taskid == p.taskid) + && let Some(task_position) = + self.tasks.iter().position(|t| t.taskid == p.taskid) { + let t = &mut self.tasks[task_position]; if let Some(s) = p.status { t.status = s; } @@ -117,6 +125,9 @@ impl TaskStore { if let Some(d) = p.description { t.description = d; } + if !t.status.is_active() { + self.tasks.remove(task_position); + } } } _ => {} @@ -132,6 +143,10 @@ impl TaskStore { } fn replace_with(&mut self, tasks: Vec) { + let tasks: Vec<_> = tasks + .into_iter() + .filter(|task| task.status.is_active()) + .collect(); self.next_taskid = tasks .iter() .map(|t| t.taskid) @@ -175,7 +190,13 @@ fn parse_snapshot_text(text: &str) -> Option> { let rest = &text[start..]; let end = rest.find(end_marker)?; let snapshot: TaskSnapshot = serde_json::from_str(&rest[..end]).ok()?; - Some(snapshot.tasks) + Some( + snapshot + .tasks + .into_iter() + .filter(|task| task.status.is_active()) + .collect(), + ) } #[cfg(test)] @@ -220,7 +241,7 @@ mod tests { } #[test] - fn counts_classifies_each_status() { + fn counts_tracks_only_active_tasks_after_completion() { let mut s = TaskStore::new(); s.apply_tool_call("TaskCreate", r#"{"subject":"a","description":""}"#); s.apply_tool_call("TaskCreate", r#"{"subject":"b","description":""}"#); @@ -230,9 +251,9 @@ mod tests { let c = s.counts(); assert_eq!(c.pending, 1); assert_eq!(c.inprogress, 1); - assert_eq!(c.completed, 1); + assert_eq!(c.completed, 0); assert_eq!(c.deleted, 0); - assert_eq!(c.total(), 3); + assert_eq!(c.total(), 2); assert_eq!(c.active(), 2); } @@ -242,7 +263,7 @@ mod tests { fn wrap_snapshot(json_body: &str, overview: &str) -> String { format!( "[Session TaskStore snapshot]\n\n{overview}\n\n```json\n{json_body}\n```\n\n\ - This is the complete session task list preserved across compaction. \ + This is the active session task list preserved across compaction. \ The following TaskList tool result presents the same state through the tool lane." ) } @@ -267,20 +288,19 @@ mod tests { }"#; let text = wrap_snapshot( body, - "TaskStore: 2 task(s) (pending: 1, inprogress: 0, completed: 1, deleted: 0)", + "TaskStore: 1 active task(s) (pending: 1, inprogress: 0)", ); let mut s = TaskStore::new(); s.apply_tool_call("TaskCreate", r#"{"subject":"stale","description":""}"#); s.apply_system_message_text(&text); let tasks = s.tasks(); - assert_eq!(tasks.len(), 2); - assert_eq!(tasks[0].taskid, 5); - assert_eq!(tasks[0].status, TaskStatus::Completed); - assert_eq!(tasks[1].taskid, 7); - // Subsequent TaskCreate must continue beyond the highest taskid + assert_eq!(tasks.len(), 1); + assert_eq!(tasks[0].taskid, 7); + assert_eq!(tasks[0].status, TaskStatus::Pending); + // Subsequent TaskCreate must continue beyond the highest active taskid // observed in the snapshot. s.apply_tool_call("TaskCreate", r#"{"subject":"new","description":""}"#); - assert_eq!(s.tasks()[2].taskid, 8); + assert_eq!(s.tasks()[1].taskid, 8); } #[test] @@ -304,7 +324,7 @@ mod tests { } ] }"#; - let text = wrap_snapshot(body, "TaskStore: 1 task(s)"); + let text = wrap_snapshot(body, "TaskStore: 1 active task(s)"); let mut s = TaskStore::new(); s.apply_system_message_text(&text); let t = &s.tasks()[0]; @@ -329,13 +349,13 @@ mod snapshot_format_contract { fn wrap_pod_style(snapshot_text: &str) -> String { format!( "[Session TaskStore snapshot]\n\n{snapshot_text}\n\n\ - This is the complete session task list preserved across compaction. \ + This is the active session task list preserved across compaction. \ The following TaskList tool result presents the same state through the tool lane." ) } fn snapshot_fixture() -> &'static str { - r#"TaskStore: 2 task(s) (pending: 0, inprogress: 1, completed: 1, deleted: 0) + r#"TaskStore: 1 active task(s) (pending: 0, inprogress: 1) ```json { @@ -345,12 +365,6 @@ mod snapshot_format_contract { "status": "inprogress", "subject": "first", "description": "first desc" - }, - { - "taskid": 2, - "status": "completed", - "subject": "second", - "description": "second desc with\nnewline" } ] } @@ -358,7 +372,7 @@ mod snapshot_format_contract { } fn empty_snapshot_fixture() -> &'static str { - r#"TaskStore: 0 task(s) (pending: 0, inprogress: 0, completed: 0, deleted: 0) + r#"TaskStore: 0 active task(s) (pending: 0, inprogress: 0) ```json { @@ -384,15 +398,11 @@ mod snapshot_format_contract { downstream.apply_system_message_text(&envelope); let tasks = downstream.tasks(); - assert_eq!(tasks.len(), 2, "TUI parsed wrong number of tasks"); + assert_eq!(tasks.len(), 1, "TUI parsed wrong number of tasks"); assert_eq!(tasks[0].taskid, 1); assert_eq!(tasks[0].subject, "first"); assert_eq!(tasks[0].description, "first desc"); assert_eq!(status_label(tasks[0].status), "inprogress"); - assert_eq!(tasks[1].taskid, 2); - assert_eq!(tasks[1].subject, "second"); - assert_eq!(tasks[1].description, "second desc with\nnewline"); - assert_eq!(status_label(tasks[1].status), "completed"); } #[test] diff --git a/crates/worker/src/feature/builtin/task/mod.rs b/crates/worker/src/feature/builtin/task/mod.rs index 66f74053..57c5e414 100644 --- a/crates/worker/src/feature/builtin/task/mod.rs +++ b/crates/worker/src/feature/builtin/task/mod.rs @@ -76,7 +76,7 @@ impl TaskFeature { /// pointing at the same feature-owned store after rewind. pub fn restore_from_history(&self, history: &[Item]) { let restored = TaskStore::from_history(history); - self.state.task_store.replace_with(restored.list()); + self.state.task_store.replace_with(restored.list_active()); } /// Feature-owned snapshot text used by compaction to preserve Task state. @@ -86,7 +86,7 @@ impl TaskFeature { /// Feature-owned compact summary used for the synthetic TaskList result. pub fn snapshot_overview(&self) -> String { - snapshot_overview(&self.state.task_store.list()) + snapshot_overview(&self.state.task_store.list_active()) } #[cfg(test)] diff --git a/crates/worker/src/feature/builtin/task/store.rs b/crates/worker/src/feature/builtin/task/store.rs index b7199a4a..740b4d16 100644 --- a/crates/worker/src/feature/builtin/task/store.rs +++ b/crates/worker/src/feature/builtin/task/store.rs @@ -18,6 +18,12 @@ pub enum TaskStatus { Deleted, } +impl TaskStatus { + pub fn is_active(self) -> bool { + matches!(self, Self::Pending | Self::Inprogress) + } +} + impl std::fmt::Display for TaskStatus { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let s = match self { @@ -54,6 +60,8 @@ pub struct TaskSnapshot { pub tasks: Vec, } +pub const DEFAULT_TASK_LIST_LIMIT: usize = 20; + impl TaskStore { pub fn new() -> Self { Self { @@ -85,6 +93,13 @@ impl TaskStore { .clone() } + pub fn list_active(&self) -> Vec { + self.list() + .into_iter() + .filter(|task| task.status.is_active()) + .collect() + } + pub fn get(&self, taskid: u64) -> Option { self.inner .lock() @@ -106,11 +121,12 @@ impl TaskStore { return Err(TaskStoreError::NoFields); } let mut inner = self.inner.lock().unwrap_or_else(|e| e.into_inner()); - let task = inner + let task_position = inner .tasks - .iter_mut() - .find(|t| t.taskid == taskid) + .iter() + .position(|t| t.taskid == taskid) .ok_or(TaskStoreError::Missing(taskid))?; + let task = &mut inner.tasks[task_position]; if let Some(status) = status { task.status = status; } @@ -120,11 +136,21 @@ impl TaskStore { if let Some(description) = description { task.description = description; } - Ok(task.clone()) + let updated = task.clone(); + if !updated.status.is_active() { + inner.tasks.remove(task_position); + } + Ok(updated) } pub fn snapshot(&self) -> TaskSnapshot { - TaskSnapshot { tasks: self.list() } + self.snapshot_limited(DEFAULT_TASK_LIST_LIMIT) + } + + pub fn snapshot_limited(&self, limit: usize) -> TaskSnapshot { + TaskSnapshot { + tasks: self.list_active().into_iter().take(limit).collect(), + } } pub fn replay_history(&self, history: &[Item]) { @@ -168,6 +194,10 @@ impl TaskStore { } pub fn replace_with(&self, tasks: Vec) { + let tasks: Vec<_> = tasks + .into_iter() + .filter(|task| task.status.is_active()) + .collect(); let next_taskid = tasks .iter() .map(|t| t.taskid) @@ -237,27 +267,26 @@ pub fn snapshot_overview(tasks: &[TaskEntry]) -> String { .iter() .filter(|t| t.status == TaskStatus::Inprogress) .count(); - let completed = tasks - .iter() - .filter(|t| t.status == TaskStatus::Completed) - .count(); - let deleted = tasks - .iter() - .filter(|t| t.status == TaskStatus::Deleted) - .count(); - format!( - "TaskStore: {} task(s) (pending: {pending}, inprogress: {inprogress}, completed: {completed}, deleted: {deleted})", - tasks.len() - ) + let active = pending + inprogress; + format!("TaskStore: {active} active task(s) (pending: {pending}, inprogress: {inprogress})") } pub fn render_snapshot(tasks: &[TaskEntry]) -> String { + let active_tasks: Vec<_> = tasks + .iter() + .filter(|task| task.status.is_active()) + .cloned() + .collect(); let snapshot = TaskSnapshot { - tasks: tasks.to_vec(), + tasks: active_tasks.clone(), }; let json = serde_json::to_string_pretty(&snapshot).unwrap_or_else(|_| String::from("{\"tasks\":[]}")); - format!("{}\n\n```json\n{}\n```\n", snapshot_overview(tasks), json) + format!( + "{}\n\n```json\n{}\n```\n", + snapshot_overview(&active_tasks), + json + ) } pub(super) fn parse_compact_snapshot_text(text: &str) -> Option> { @@ -270,7 +299,13 @@ pub(super) fn parse_compact_snapshot_text(text: &str) -> Option> let rest = &text[start..]; let end = rest.find(end_marker)?; let snapshot: TaskSnapshot = serde_json::from_str(&rest[..end]).ok()?; - Some(snapshot.tasks) + Some( + snapshot + .tasks + .into_iter() + .filter(|task| task.status.is_active()) + .collect(), + ) } #[cfg(test)] @@ -288,11 +323,9 @@ mod tests { ]; let store = TaskStore::from_history(&history); let tasks = store.list(); - assert_eq!(tasks.len(), 2); + assert_eq!(tasks.len(), 1); assert_eq!(tasks[0].taskid, 1); assert_eq!(tasks[0].status, TaskStatus::Pending); - assert_eq!(tasks[1].taskid, 2); - assert_eq!(tasks[1].status, TaskStatus::Completed); } /// Wrap snapshot text the way `Worker::try_pre_run_compact` does, so tests @@ -300,7 +333,7 @@ mod tests { fn wrap_snapshot_system_message(snapshot: &str) -> String { format!( "[Session TaskStore snapshot]\n\n{snapshot}\n\n\ - This is the complete session task list preserved across compaction. \ + This is the active session task list preserved across compaction. \ The following TaskList tool result presents the same state through the tool lane." ) } @@ -322,11 +355,9 @@ mod tests { ]; let store = TaskStore::from_history(&history); let tasks = store.list(); - assert_eq!(tasks.len(), 2); - assert_eq!(tasks[0].taskid, 1); - assert_eq!(tasks[0].status, TaskStatus::Completed); - assert_eq!(tasks[1].taskid, 2); - assert_eq!(tasks[1].subject, "new"); + assert_eq!(tasks.len(), 1); + assert_eq!(tasks[0].taskid, 2); + assert_eq!(tasks[0].subject, "new"); } #[test] @@ -365,15 +396,12 @@ mod tests { ]; let store = TaskStore::from_history(&history); let tasks = store.list(); - assert_eq!(tasks.len(), 3); - assert_eq!(tasks[0].taskid, 1); - assert_eq!(tasks[0].subject, "A"); - assert_eq!(tasks[0].status, TaskStatus::Completed); - assert_eq!(tasks[1].taskid, 2); - assert_eq!(tasks[1].subject, "B"); - assert_eq!(tasks[1].status, TaskStatus::Inprogress); - assert_eq!(tasks[2].taskid, 3); - assert_eq!(tasks[2].subject, "C"); + assert_eq!(tasks.len(), 2); + assert_eq!(tasks[0].taskid, 2); + assert_eq!(tasks[0].subject, "B"); + assert_eq!(tasks[0].status, TaskStatus::Inprogress); + assert_eq!(tasks[1].taskid, 3); + assert_eq!(tasks[1].subject, "C"); } #[test] @@ -415,9 +443,10 @@ mod tests { let snapshot_text = pre.snapshot_text(); let system = Item::system_message(wrap_snapshot_system_message(&snapshot_text)); let call = Item::tool_call("compact-tasklist", "TaskList", "{}"); + let active_tasks = pre.list_active(); let result = Item::tool_result_with_content( "compact-tasklist", - snapshot_overview(&pre.list()), + snapshot_overview(&active_tasks), snapshot_text.clone(), ); @@ -426,7 +455,7 @@ mod tests { .as_text() .and_then(parse_compact_snapshot_text) .expect("system message should parse as snapshot"); - assert_eq!(extracted, pre.list()); + assert_eq!(extracted, active_tasks); // The synthetic call/result pair shares one call_id and carries the // expected tool name + detailed content. @@ -453,6 +482,6 @@ mod tests { // Replaying the full triple reconstructs the same TaskStore. let store = TaskStore::from_history(&[system, call, result]); - assert_eq!(store.list(), pre.list()); + assert_eq!(store.list(), active_tasks); } } diff --git a/crates/worker/src/feature/builtin/task/tool_impl.rs b/crates/worker/src/feature/builtin/task/tool_impl.rs index c06f5fe2..20282675 100644 --- a/crates/worker/src/feature/builtin/task/tool_impl.rs +++ b/crates/worker/src/feature/builtin/task/tool_impl.rs @@ -6,7 +6,7 @@ use async_trait::async_trait; use llm_engine::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use serde::Deserialize; -use super::store::{TaskEntry, TaskStatus, TaskStore, render_snapshot, snapshot_overview}; +use super::store::{DEFAULT_TASK_LIST_LIMIT, TaskEntry, TaskStatus, TaskStore, snapshot_overview}; #[derive(Debug, Deserialize, schemars::JsonSchema)] struct TaskCreateParams { @@ -17,7 +17,11 @@ struct TaskCreateParams { } #[derive(Debug, Deserialize, schemars::JsonSchema)] -struct TaskListParams {} +struct TaskListParams { + /// Maximum number of active tasks to return. Defaults to 20. + #[serde(default)] + limit: Option, +} #[derive(Debug, Deserialize, schemars::JsonSchema)] struct TaskGetParams { @@ -58,9 +62,7 @@ coordination. Do not create a task just because a request has several steps, and one for short questions, quick checks, single reviews, or one-off commands. Prefer updating an \ existing active task over creating a duplicate. Input only `subject` and `description`; `taskid` \ is assigned automatically and initial `status` is `pending`."; -const LIST_DESCRIPTION: &str = "List every session-lifetime task, including completed and \ -deleted entries. Tasks are user-visible real-time status for short-term current-work tracking. \ -Takes an empty object as input."; +const LIST_DESCRIPTION: &str = "List active session-lifetime tasks. Completed and deleted tasks are forgotten and omitted. Defaults to 20 tasks unless `limit` is provided."; const GET_DESCRIPTION: &str = "Get one session-lifetime task by `taskid`. Tasks are \ user-visible real-time status for short-term current-work tracking. Returns an error if the task \ does not exist."; @@ -81,7 +83,7 @@ impl Tool for TaskCreateTool { let params: TaskCreateParams = serde_json::from_str(input_json) .map_err(|e| ToolError::InvalidArgument(format!("invalid TaskCreate input: {e}")))?; let created = self.store.create(params.subject, params.description); - let tasks = self.store.list(); + let tasks = self.store.list_active(); Ok(task_output( format!( "Created task {} ({})\n{}", @@ -90,7 +92,6 @@ impl Tool for TaskCreateTool { snapshot_overview(&tasks) ), &created, - &tasks, )) } } @@ -102,12 +103,14 @@ impl Tool for TaskListTool { input_json: &str, _ctx: llm_engine::tool::ToolExecutionContext, ) -> Result { - let _: TaskListParams = serde_json::from_str(input_json) + let params: TaskListParams = serde_json::from_str(input_json) .map_err(|e| ToolError::InvalidArgument(format!("invalid TaskList input: {e}")))?; - let tasks = self.store.list(); + let limit = params.limit.unwrap_or(DEFAULT_TASK_LIST_LIMIT); + let active_tasks = self.store.list_active(); + let tasks: Vec<_> = active_tasks.iter().take(limit).cloned().collect(); Ok(ToolOutput { - summary: snapshot_overview(&tasks), - content: Some(render_snapshot(&tasks)), + summary: list_overview(active_tasks.len(), tasks.len()), + content: Some(render_task_list(&tasks)), }) } } @@ -150,7 +153,7 @@ impl Tool for TaskUpdateTool { params.description, ) .map_err(|e| ToolError::ExecutionFailed(e.to_string()))?; - let tasks = self.store.list(); + let tasks = self.store.list_active(); Ok(task_output( format!( "Updated task {} ({})\n{}", @@ -159,21 +162,35 @@ impl Tool for TaskUpdateTool { snapshot_overview(&tasks) ), &updated, - &tasks, )) } } -fn task_output(summary: String, task: &TaskEntry, tasks: &[TaskEntry]) -> ToolOutput { +fn task_output(summary: String, task: &TaskEntry) -> ToolOutput { let content = serde_json::json!({ "task": task, - "snapshot": { "tasks": tasks }, }); ToolOutput { summary, content: Some(serde_json::to_string_pretty(&content).unwrap_or_default()), } } + +fn list_overview(total_active: usize, returned: usize) -> String { + if returned < total_active { + format!( + "TaskStore: {returned} active task(s) shown; {} omitted.", + total_active - returned + ) + } else { + format!("TaskStore: {returned} active task(s)") + } +} + +fn render_task_list(tasks: &[TaskEntry]) -> String { + serde_json::to_string_pretty(tasks).unwrap_or_default() +} + fn task_create_tool(store: TaskStore) -> ToolDefinition { Arc::new(move || { let schema = schemars::schema_for!(TaskCreateParams); @@ -286,10 +303,105 @@ mod tests { assert!(out.content.unwrap().contains("implement tasks")); let out = list.execute("{}", Default::default()).await.unwrap(); - assert!(out.summary.contains("1 task(s)")); + assert!(out.summary.contains("1 active task(s)")); let content = out.content.unwrap(); assert!(content.contains("\"taskid\": 1")); - assert!(content.contains("```json")); + assert!(!content.contains("\"limit\"")); + assert!(!content.contains("```json")); + } + + #[tokio::test] + async fn task_list_omits_completed_deleted_and_defaults_to_twenty() { + let store = TaskStore::new(); + let create = tool(task_create_tool(store.clone())); + let update = tool(task_update_tool(store.clone())); + let list = tool(task_list_tool(store.clone())); + + for i in 0..25 { + create + .execute( + &format!(r#"{{"subject":"task {i}","description":"desc {i}"}}"#), + Default::default(), + ) + .await + .unwrap(); + } + update + .execute(r#"{"taskid":1,"status":"completed"}"#, Default::default()) + .await + .unwrap(); + update + .execute(r#"{"taskid":2,"status":"deleted"}"#, Default::default()) + .await + .unwrap(); + + let out = list.execute("{}", Default::default()).await.unwrap(); + assert_eq!( + out.summary, + "TaskStore: 20 active task(s) shown; 3 omitted." + ); + let content = out.content.unwrap(); + let json: serde_json::Value = serde_json::from_str(&content).unwrap(); + let tasks = json.as_array().unwrap(); + assert_eq!(tasks.len(), 20); + let ids: Vec = tasks + .iter() + .map(|task| task["taskid"].as_u64().unwrap()) + .collect(); + assert!(!ids.contains(&1)); + assert!(!ids.contains(&2)); + assert!(!content.contains("\"limit\"")); + assert!(!content.contains("\"total_active\"")); + assert!(!content.contains("\"truncated\"")); + + let out = list + .execute(r#"{"limit":3}"#, Default::default()) + .await + .unwrap(); + assert_eq!( + out.summary, + "TaskStore: 3 active task(s) shown; 20 omitted." + ); + let content = out.content.unwrap(); + let json: serde_json::Value = serde_json::from_str(&content).unwrap(); + assert_eq!(json.as_array().unwrap().len(), 3); + } + + #[tokio::test] + async fn task_create_and_update_results_do_not_include_full_snapshot() { + let store = TaskStore::new(); + let create = tool(task_create_tool(store.clone())); + let update = tool(task_update_tool(store.clone())); + + create + .execute( + r#"{"subject":"done","description":"completed task"}"#, + Default::default(), + ) + .await + .unwrap(); + update + .execute(r#"{"taskid":1,"status":"completed"}"#, Default::default()) + .await + .unwrap(); + create + .execute( + r#"{"subject":"active","description":"active task"}"#, + Default::default(), + ) + .await + .unwrap(); + + let out = update + .execute(r#"{"taskid":2,"status":"inprogress"}"#, Default::default()) + .await + .unwrap(); + let content = out.content.unwrap(); + assert!(content.contains("\"task\"")); + assert!(content.contains("\"taskid\": 2")); + assert!(!content.contains("\"snapshot\"")); + assert!(!content.contains("\"taskid\": 1")); + assert!(!content.contains("completed task")); } #[tokio::test] diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index 05ddf875..445b8512 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -2733,7 +2733,7 @@ impl Worker { } let task_snapshot_message = Item::system_message(format!( "[Session TaskStore snapshot]\n\n{task_snapshot_text}\n\n\ - This is the complete session task list preserved across compaction. \ + This is the active session task list preserved across compaction. \ The following TaskList tool result presents the same state through the tool lane." )); compact_introduced_system_messages.push(task_snapshot_message.clone());