@sigitcodereview / sigit / commits / 2c43c83

Let write_todos clear the plan with an empty list

write_todos rejected an empty list, so the model had no way to clear a finished checklist. Every valid call had to restate at least one step, and the last plan stayed in the editor until the session ended. An empty list now returns "Task list cleared." and goes to the client as an ACP plan with no entries. Plan updates replace the whole plan, so that clears it. The tool description tells the model it can do this. Fixes #98

Seto Elkahfi committed Sep 16, 2026 at 21:36 UTC 2c43c832f9dcb294301ff275ce5be4af28bd77ba

3 files changed +81 -7

src/main.rs

+8 -3

@@ -565,10 +565,9 @@ fn history_replay_updates(history: &[serde_json::Value]) -> Vec<SessionUpdate> {

565 fn todos_arguments_to_plan(arguments: &str) -> Option<Plan> {

566 let args: serde_json::Value = serde_json::from_str(arguments).ok()?;

567 let todos = args.get("todos")?.as_array()?;

568 - if todos.is_empty() {

569 - return None;

570 - }

568

569 + // An empty list still yields a plan: ACP plan updates replace the whole

570 + // plan, so an update with no entries is what clears it in the client.

571 let mut entries = Vec::with_capacity(todos.len());

572 for todo in todos {

573 let content = todo.get("content")?.as_str()?.trim();

@@ -5028,6 +5027,12 @@ mod tests {

5027 }

5028 }

5029

5030 + #[test]

5031 + fn an_empty_todo_list_becomes_an_empty_plan_that_clears_the_client() {

5032 + let plan = todos_arguments_to_plan(r#"{"todos":[]}"#).expect("empty list is a plan");

5033 + assert!(plan.entries.is_empty());

5034 + }

5035 +

5036 #[test]

5037 fn history_replay_shows_a_placeholder_for_a_call_with_no_recorded_output() {

5038 // A session file can carry a tool call whose result was never saved

src/tools.rs

+16 -4

@@ -336,13 +336,14 @@ pub fn all_tools() -> Vec<AgentTool> {

336 call it once up front with all the steps as `pending`, then call it \

337 again whenever a step's status changes. Mark exactly one step \

338 `in_progress` at a time and `completed` as soon as it is done. \

339 - Keep the list short and outcome-focused.",

339 + Keep the list short and outcome-focused. Pass an empty list to \

340 + clear the checklist once the task is finished.",

341 parameters_schema: json!({

342 "type": "object",

343 "properties": {

344 "todos": {

345 "type": "array",

345 - "description": "The full, current checklist (replaces any previous list).",

346 + "description": "The full, current checklist (replaces any previous list). An empty list clears it.",

347 "items": {

348 "type": "object",

349 "properties": {

@@ -1795,10 +1796,15 @@ fn exec_write_todos(arguments: &str) -> String {

1796 };

1797

1798 let todos = match args.get("todos").and_then(Value::as_array) {

1798 - Some(t) if !t.is_empty() => t,

1799 - Some(_) => return "Error: \"todos\" must contain at least one item".to_string(),

1799 + Some(t) => t,

1800 None => return "Error: missing required parameter \"todos\"".to_string(),

1801 };

1802 + // An empty list is how the model clears a finished checklist. Rejecting

1803 + // it left the last plan on screen for good, since every other call has to

1804 + // restate at least one step.

1805 + if todos.is_empty() {

1806 + return "Task list cleared.".to_string();

1807 + }

1808

1809 let mut lines = Vec::with_capacity(todos.len());

1810 let mut completed = 0usize;

@@ -2786,6 +2792,12 @@ mod tests {

2792 assert!(result.contains("[ ] Run tests"), "{result}");

2793 }

2794

2795 + #[test]

2796 + fn test_write_todos_accepts_an_empty_list_to_clear() {

2797 + let result = exec_write_todos(r#"{"todos":[]}"#);

2798 + assert_eq!(result, "Task list cleared.");

2799 + }

2800 +

2801 #[test]

2802 fn test_remember_appends_to_instruction_file() {

2803 let dir = std::env::temp_dir().join("sigit_test_remember");

tests/acp_permissions.rs

+57

@@ -1205,6 +1205,63 @@ fn write_todos_reaches_the_client_as_an_acp_plan() {

1205 let _ = std::fs::remove_dir_all(&scratch);

1206 }

1207

1208 +/// An empty `write_todos` is how the model clears a finished checklist. It has

1209 +/// to reach the client as a plan with no entries, since ACP plan updates

1210 +/// replace the whole plan, rather than as an error card that leaves the old

1211 +/// list on screen.

1212 +#[test]

1213 +fn an_empty_write_todos_clears_the_acp_plan() {

1214 + let endpoint = start_fake_endpoint(vec![

1215 + sse_tool_call("call_1", "write_todos", &json!({"todos": []}).to_string()),

1216 + sse_text("Checklist cleared."),

1217 + ]);

1218 +

1219 + let scratch = std::env::temp_dir().join(format!("sigit_acp_plan_clear_{}", std::process::id()));

1220 + let config_dir = scratch.join("config");

1221 + let cwd = scratch.join("cwd");

1222 + std::fs::create_dir_all(&config_dir).unwrap();

1223 + std::fs::create_dir_all(&cwd).unwrap();

1224 +

1225 + let mut agent = spawn_agent(endpoint.port, &config_dir);

1226 +

1227 + let id = agent.request(

1228 + "initialize",

1229 + json!({"protocolVersion": 1, "clientCapabilities": {}}),

1230 + );

1231 + agent.wait_for_response(id);

1232 +

1233 + let id = agent.request("session/new", json!({"cwd": cwd, "mcpServers": []}));

1234 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]

1235 + .as_str()

1236 + .expect("session id")

1237 + .to_string();

1238 +

1239 + let prompt_id = agent.request(

1240 + "session/prompt",

1241 + json!({

1242 + "sessionId": session_id,

1243 + "prompt": [{"type": "text", "text": "clear the checklist"}],

1244 + }),

1245 + );

1246 + let (response, updates) = agent.wait_for_response_with_updates(prompt_id);

1247 + assert_eq!(response["result"]["stopReason"], "end_turn");

1248 +

1249 + let plan = updates

1250 + .iter()

1251 + .find(|update| update["sessionUpdate"] == "plan")

1252 + .expect("an empty write_todos should still send a plan update");

1253 + assert_eq!(plan["entries"], json!([]));

1254 + assert!(

1255 + !updates

1256 + .iter()

1257 + .any(|update| update["sessionUpdate"] == "tool_call"),

1258 + "clearing the plan must not show a tool-call card: {updates:?}"

1259 + );

1260 +

1261 + drop(agent);

1262 + let _ = std::fs::remove_dir_all(&scratch);

1263 +}

1264 +

1265 /// An off-enum status must not make the whole call vanish. `exec_write_todos`

1266 /// renders it as pending and reports success, so the plan has to carry it the

1267 /// same way; dropping the plan here would leave the client showing a stale list.