@sigitcodereview / sigit / commits / 4cda4cb

Render write_todos as an ACP plan update

Zed and other clients render session/update plans with proper progress UI, so route the model's write_todos tool calls there instead of showing them as a generic tool-call card. Co-Authored-By: siGit Code <noreply@sigit.si>

Seto Elkahfi committed Sep 7, 2026 at 07:51 UTC 4cda4cb58f3ae14d3a73e86242bb2e97c7f6102b

2 files changed +298 -38

src/main.rs

+95 -38

@@ -75,10 +75,11 @@ use agent_client_protocol::schema::v1::{

75 ConfigOptionUpdate, ContentBlock, ContentChunk, EmbeddedResourceResource, ForkSessionRequest,

76 ForkSessionResponse, Implementation, InitializeRequest, InitializeResponse, LoadSessionRequest,

77 LoadSessionResponse, Meta, NewSessionRequest, NewSessionResponse, PermissionOption,

78 - PermissionOptionKind, PromptRequest, PromptResponse, RequestPermissionOutcome,

79 - RequestPermissionRequest, SessionAdditionalDirectoriesCapabilities, SessionCapabilities,

80 - SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption,

81 - SessionConfigValueId, SessionForkCapabilities, SessionId, SessionNotification, SessionUpdate,

78 + PermissionOptionKind, Plan, PlanEntry, PlanEntryPriority, PlanEntryStatus, PromptRequest,

79 + PromptResponse, RequestPermissionOutcome, RequestPermissionRequest,

80 + SessionAdditionalDirectoriesCapabilities, SessionCapabilities, SessionConfigOption,

81 + SessionConfigOptionCategory, SessionConfigSelectOption, SessionConfigValueId,

82 + SessionForkCapabilities, SessionId, SessionNotification, SessionUpdate,

83 SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, StopReason, ToolCall,

84 ToolCallStatus, ToolCallUpdate, ToolCallUpdateFields, ToolKind, UnstructuredCommandInput,

85 };

@@ -348,6 +349,35 @@ fn tool_kind_for(tool_name: &str) -> ToolKind {

349 }

350 }

351

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

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

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

355 + if todos.is_empty() {

356 + return None;

357 + }

358 +

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

360 + for todo in todos {

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

362 + if content.is_empty() {

363 + return None;

364 + }

365 + // Unknown and absent statuses both fall back to pending, matching how

366 + // `exec_write_todos` renders them. The two have to agree: the executor

367 + // reports success for an off-enum status, so rejecting the plan here

368 + // would leave a stale list on screen while the model believes it

369 + // reported progress.

370 + let status = match todo.get("status").and_then(serde_json::Value::as_str) {

371 + Some("completed") => PlanEntryStatus::Completed,

372 + Some("in_progress") => PlanEntryStatus::InProgress,

373 + _ => PlanEntryStatus::Pending,

374 + };

375 + entries.push(PlanEntry::new(content, PlanEntryPriority::Medium, status));

376 + }

377 +

378 + Some(Plan::new(entries))

379 +}

380 +

381 /// Shown when a siGit Code Cloud tier is selected without a signed-in account.

382 const CLOUD_LOGIN_PROMPT: &str = "siGit Code Cloud needs an account. Sign in with \

383 `/login <email> <password>` (or the Authenticate button), then pick the tier again. \

@@ -876,6 +906,18 @@ impl SiGitAgent {

906 cx.send_notification(SessionNotification::new(session_id, update))

907 }

908

909 + fn send_plan_update(

910 + &self,

911 + cx: &ConnectionTo<Client>,

912 + session_id: SessionId,

913 + plan: Plan,

914 + ) -> agent_client_protocol::Result<()> {

915 + cx.send_notification(SessionNotification::new(

916 + session_id,

917 + SessionUpdate::Plan(plan),

918 + ))

919 + }

920 +

921 /// Advertise siGit's slash commands to the client. Editors like Zed parse

922 /// `/`-prefixed input and only forward commands they've been told about, so

923 /// without this `/login`, `/models`, etc. are rejected client-side.

@@ -1609,29 +1651,42 @@ impl SiGitAgent {

1651 tc.arguments.chars().take(120).collect::<String>()

1652 );

1653

1612 - // Model tool calls are agent actions in ACP too, not merely an

1613 - // implementation detail of the completion loop. Announce each

1614 - // one before it runs and close it afterwards so clients such as

1615 - // Zed can render activity while the next inference round is in

1616 - // flight. Previously only model loading emitted ToolCall events,

1617 - // leaving an otherwise working tool round visually indistinct

1618 - // from a stalled response.

1619 - let raw_input: serde_json::Value = serde_json::from_str(&tc.arguments)

1620 - .unwrap_or_else(|e| {

1621 - log::warn!("malformed JSON in tool arguments for '{}': {e}", tc.name);

1622 - serde_json::Value::String(tc.arguments.clone())

1623 - });

1624 - self.send_tool_call_update(

1625 - cx,

1626 - session_id.clone(),

1627 - SessionUpdate::ToolCall(

1628 - ToolCall::new(tc.id.clone(), tc.name.clone())

1629 - .kind(tool_kind_for(&tc.name))

1630 - .status(ToolCallStatus::InProgress)

1631 - .raw_input(raw_input),

1632 - ),

1633 - )

1634 - .ok();

1654 + // Only treat the call as a plan once its arguments have actually

1655 + // produced one. Keying off the tool name alone means a

1656 + // `write_todos` the converter rejects sends no plan *and* skips

1657 + // the tool-call card below, so the call disappears from the

1658 + // client while the model is told the list was updated.

1659 + let plan = (tc.name == "write_todos")

1660 + .then(|| todos_arguments_to_plan(&tc.arguments))

1661 + .flatten();

1662 + let render_as_plan = plan.is_some();

1663 + if let Some(plan) = plan {

1664 + self.send_plan_update(cx, session_id.clone(), plan).ok();

1665 + } else {

1666 + // Model tool calls are agent actions in ACP too, not merely an

1667 + // implementation detail of the completion loop. Announce each

1668 + // one before it runs and close it afterwards so clients such as

1669 + // Zed can render activity while the next inference round is in

1670 + // flight. Previously only model loading emitted ToolCall events,

1671 + // leaving an otherwise working tool round visually indistinct

1672 + // from a stalled response.

1673 + let raw_input: serde_json::Value = serde_json::from_str(&tc.arguments)

1674 + .unwrap_or_else(|e| {

1675 + log::warn!("malformed JSON in tool arguments for '{}': {e}", tc.name);

1676 + serde_json::Value::String(tc.arguments.clone())

1677 + });

1678 + self.send_tool_call_update(

1679 + cx,

1680 + session_id.clone(),

1681 + SessionUpdate::ToolCall(

1682 + ToolCall::new(tc.id.clone(), tc.name.clone())

1683 + .kind(tool_kind_for(&tc.name))

1684 + .status(ToolCallStatus::InProgress)

1685 + .raw_input(raw_input),

1686 + ),

1687 + )

1688 + .ok();

1689 + }

1690

1691 let signature = format!("{}\n{}", tc.name, tc.arguments);

1692 let repeat_count = repeated_tool_calls

@@ -1727,17 +1782,19 @@ impl SiGitAgent {

1782

1783 log::info!(" ← {} chars", output.len());

1784

1730 - self.send_tool_call_update(

1731 - cx,

1732 - session_id.clone(),

1733 - SessionUpdate::ToolCallUpdate(ToolCallUpdate::new(

1734 - tc.id.clone(),

1735 - ToolCallUpdateFields::new()

1736 - .status(ToolCallStatus::Completed)

1737 - .raw_output(serde_json::Value::String(output.clone())),

1738 - )),

1739 - )

1740 - .ok();

1785 + if !render_as_plan {

1786 + self.send_tool_call_update(

1787 + cx,

1788 + session_id.clone(),

1789 + SessionUpdate::ToolCallUpdate(ToolCallUpdate::new(

1790 + tc.id.clone(),

1791 + ToolCallUpdateFields::new()

1792 + .status(ToolCallStatus::Completed)

1793 + .raw_output(serde_json::Value::String(output.clone())),

1794 + )),

1795 + )

1796 + .ok();

1797 + }

1798

1799 tool_results.push(BackendToolResult {

1800 tool_call_id: tc.id.clone(),

tests/acp_permissions.rs

+203

@@ -283,6 +283,24 @@ impl AgentUnderTest {

283 }

284 }

285

286 + fn wait_for_prompt_updates(&mut self, id: u64) -> (Value, Vec<Value>) {

287 + let mut updates = Vec::new();

288 + let deadline = Instant::now() + TIMEOUT;

289 + loop {

290 + let remaining = deadline.saturating_duration_since(Instant::now());

291 + let Ok(message) = self.incoming.recv_timeout(remaining) else {

292 + panic!("timed out waiting for the prompt response");

293 + };

294 + if message["method"] == "session/update" {

295 + updates.push(message["params"]["update"].clone());

296 + }

297 + if message["id"] == id && message.get("method").is_none() {

298 + assert!(message.get("error").is_none(), "prompt failed: {message}");

299 + return (message, updates);

300 + }

301 + }

302 + }

303 +

304 /// A request *from* the agent (has a `method` and its own id).

305 fn wait_for_agent_request(&mut self, method: &str) -> Value {

306 self.wait_for(&format!("agent request {method}"), |message| {

@@ -662,3 +680,188 @@ fn a_tool_call_emitted_as_text_is_executed_rather_than_rendered() {

680 drop(agent);

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

682 }

683 +

684 +#[test]

685 +fn write_todos_reaches_the_client_as_an_acp_plan() {

686 + let endpoint = start_fake_endpoint(vec![

687 + sse_tool_call(

688 + "call_1",

689 + "write_todos",

690 + &json!({

691 + "todos": [

692 + {"content": "Fetch issue screenshot", "status": "completed"},

693 + {"content": "Implement ACP styling", "status": "in_progress"},

694 + {"content": "Build and verify", "status": "pending"}

695 + ]

696 + })

697 + .to_string(),

698 + ),

699 + sse_text("Continuing."),

700 + ]);

701 +

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

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

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

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

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

707 +

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

709 +

710 + let id = agent.request(

711 + "initialize",

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

713 + );

714 + agent.wait_for_response(id);

715 +

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

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

718 + .as_str()

719 + .expect("session id")

720 + .to_string();

721 +

722 + let prompt_id = agent.request(

723 + "session/prompt",

724 + json!({

725 + "sessionId": session_id,

726 + "prompt": [{"type": "text", "text": "fix the ACP display"}],

727 + }),

728 + );

729 + let (response, updates) = agent.wait_for_prompt_updates(prompt_id);

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

731 +

732 + let plan = updates

733 + .iter()

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

735 + .expect("write_todos should be rendered as an ACP plan update");

736 + assert_eq!(plan["entries"][0]["content"], "Fetch issue screenshot");

737 + assert_eq!(plan["entries"][0]["status"], "completed");

738 + assert_eq!(plan["entries"][1]["status"], "in_progress");

739 + assert_eq!(plan["entries"][2]["status"], "pending");

740 +

741 + assert!(

742 + !updates.iter().any(|update| {

743 + update["sessionUpdate"] == "tool_call" && update["title"] == "write_todos"

744 + }),

745 + "write_todos must not show up as a generic tool-call card: {updates:?}"

746 + );

747 +

748 + drop(agent);

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

750 +}

751 +

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

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

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

755 +#[test]

756 +fn write_todos_with_an_unknown_status_still_reaches_the_client() {

757 + let endpoint = start_fake_endpoint(vec![

758 + sse_tool_call(

759 + "call_1",

760 + "write_todos",

761 + &json!({

762 + "todos": [

763 + {"content": "Fetch issue screenshot", "status": "completed"},

764 + {"content": "Ship it", "status": "cancelled"}

765 + ]

766 + })

767 + .to_string(),

768 + ),

769 + sse_text("Continuing."),

770 + ]);

771 +

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

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

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

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

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

777 +

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

779 +

780 + let id = agent.request(

781 + "initialize",

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

783 + );

784 + agent.wait_for_response(id);

785 +

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

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

788 + .as_str()

789 + .expect("session id")

790 + .to_string();

791 +

792 + let prompt_id = agent.request(

793 + "session/prompt",

794 + json!({

795 + "sessionId": session_id,

796 + "prompt": [{"type": "text", "text": "plan the work"}],

797 + }),

798 + );

799 + let (_response, updates) = agent.wait_for_prompt_updates(prompt_id);

800 +

801 + let plan = updates

802 + .iter()

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

804 + .expect("an unknown status must not drop the plan");

805 + assert_eq!(plan["entries"][1]["content"], "Ship it");

806 + assert_eq!(plan["entries"][1]["status"], "pending");

807 +

808 + drop(agent);

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

810 +}

811 +

812 +/// Arguments the converter cannot turn into a plan fall back to the ordinary

813 +/// tool-call card. Without that fallback the call is announced nowhere at all.

814 +#[test]

815 +fn unconvertible_write_todos_falls_back_to_a_tool_call_card() {

816 + let endpoint = start_fake_endpoint(vec![

817 + // `todos` is absent, so there is no plan to build. `exec_write_todos`

818 + // answers with an error string, and the client still has to see the call.

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

820 + sse_text("Continuing."),

821 + ]);

822 +

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

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

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

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

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

828 +

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

830 +

831 + let id = agent.request(

832 + "initialize",

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

834 + );

835 + agent.wait_for_response(id);

836 +

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

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

839 + .as_str()

840 + .expect("session id")

841 + .to_string();

842 +

843 + let prompt_id = agent.request(

844 + "session/prompt",

845 + json!({

846 + "sessionId": session_id,

847 + "prompt": [{"type": "text", "text": "plan the work"}],

848 + }),

849 + );

850 + let (_response, updates) = agent.wait_for_prompt_updates(prompt_id);

851 +

852 + assert!(

853 + !updates

854 + .iter()

855 + .any(|update| update["sessionUpdate"] == "plan"),

856 + "there is no plan to send: {updates:?}"

857 + );

858 + assert!(

859 + updates.iter().any(|update| {

860 + update["sessionUpdate"] == "tool_call" && update["title"] == "write_todos"

861 + }),

862 + "the call must still be announced as a tool call: {updates:?}"

863 + );

864 +

865 + drop(agent);

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

867 +}