@sigitcodereview / sigit / commits / 8c88aeb

feat(tools): persist write_todos state across model omissions and error turns

write_todos currently lives only in the model's tool-call stream: if the model omits the call or the turn errors out before it runs, the client's plan panel goes empty with no way to recover the last known state. Add a process-global persistence layer (RwLock<Option<Vec<Todo>>>) in tools.rs, mirroring the existing ACTIVE_SESSION pattern. exec_write_todos now stores the parsed list on every successful call and clears the store when the list is empty. Expose get_todos() so main.rs can read persisted state outside the tool-call path. In main.rs, add persisted_todos_to_plan() to convert Vec<Todo> into an ACP Plan, and call it on both DrainTurnError::Backend paths (initial inference failure and tool-round failure) to re-emit the last persisted plan before returning the error. The client panel now stays populated even when the model never reaches write_todos. Six new tests cover persistence, retrieval, clear-on-empty, overwrite, unknown-status fallback, and missing-status default. Co-Authored-By: siGit Code v1.5.13-acp <noreply@sigit.si>

Seto Elkahfi committed Oct 4, 2026 at 21:07 UTC 8c88aeb35300445cf4aae82d324757339929b940

2 files changed +208 -7

src/main.rs

+30

@@ -634,6 +634,25 @@ fn todos_arguments_to_plan(arguments: &str) -> Option<Plan> {

634 Some(Plan::new(entries))

635 }

636

637 +/// Rebuilds an ACP [`Plan`] from the persisted todo list in `tools::get_todos()`.

638 +/// Used to re-emit the plan on error turns so the client panel stays populated

639 +/// when the model never got to (or forgot to) call `write_todos` this turn.

640 +fn persisted_todos_to_plan() -> Option<Plan> {

641 + let todos = tools::get_todos()?;

642 + let entries = todos

643 + .into_iter()

644 + .map(|todo| {

645 + let status = match todo.status {

646 + tools::TodoStatus::Completed => PlanEntryStatus::Completed,

647 + tools::TodoStatus::InProgress => PlanEntryStatus::InProgress,

648 + tools::TodoStatus::Pending => PlanEntryStatus::Pending,

649 + };

650 + PlanEntry::new(todo.content, PlanEntryPriority::Medium, status)

651 + })

652 + .collect();

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

654 +}

655 +

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

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

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

@@ -2251,6 +2270,12 @@ impl SiGitAgent {

2270 }

2271 Err(DrainTurnError::Backend(error)) => {

2272 log::error!("send_message_with_tools failed: {error}");

2273 + // Re-emit the persisted plan so the client panel doesn't go

2274 + // empty when the initial inference call fails before the model

2275 + // can call `write_todos`.

2276 + if let Some(plan) = persisted_todos_to_plan() {

2277 + let _ = self.send_plan_update(cx, session_id.clone(), plan);

2278 + }

2279 // Verbatim, no prefix: the backend has already turned this into

2280 // something worth reading (see `describe_api_error`), and it is

2281 // what the editor puts in its error banner. The context a

@@ -2532,6 +2557,11 @@ impl SiGitAgent {

2557 }

2558 Err(DrainTurnError::Backend(error)) => {

2559 log::error!("send_tool_results failed: {error}");

2560 + // Re-emit the persisted plan so the client panel doesn't go

2561 + // empty when a tool round fails mid-turn.

2562 + if let Some(plan) = persisted_todos_to_plan() {

2563 + let _ = self.send_plan_update(cx, session_id.clone(), plan);

2564 + }

2565 self.finish_prompt(&session_id, &cancellation);

2566 return Err(agent_client_protocol::Error::new(-32603, error));

2567 }

src/tools.rs

+178 -7

@@ -1793,9 +1793,68 @@ fn glob_walk(root: &Path, dir: &Path, re: &Regex, out: &mut Vec<(std::time::Syst

1793

1794 // ── write_todos ──────────────────────────────────────────────────────────────

1795

1796 +/// A single todo item as the model sent it, kept around so the panel doesn't

1797 +/// go stale when a later turn omits `write_todos` or errors out before

1798 +/// reaching it.

1799 +#[derive(Clone, Debug, PartialEq, Eq)]

1800 +pub struct Todo {

1801 + pub content: String,

1802 + pub status: TodoStatus,

1803 +}

1804 +

1805 +#[derive(Clone, Copy, Debug, PartialEq, Eq)]

1806 +pub enum TodoStatus {

1807 + Pending,

1808 + InProgress,

1809 + Completed,

1810 +}

1811 +

1812 +impl TodoStatus {

1813 + fn from_str(s: &str) -> Self {

1814 + match s {

1815 + "completed" => Self::Completed,

1816 + "in_progress" => Self::InProgress,

1817 + _ => Self::Pending,

1818 + }

1819 + }

1820 +}

1821 +

1822 +/// Process-global todo store. The model owns the list conceptually, but when

1823 +/// it forgets to re-send the full list — an error turn, a context shift, a

1824 +/// simple omission — the panel goes empty with nothing to fall back on. This

1825 +/// keeps the last-known list so `main.rs` can re-emit it.

1826 +/// (`todos_arguments_to_plan` + `SessionUpdate::Plan`) without the model's

1827 +/// help.

1828 +static TODOS: RwLock<Option<Vec<Todo>>> = RwLock::new(None);

1829 +

1830 +/// Read the persisted todo list, if any. Returns `None` when no `write_todos`

1831 +/// call has succeeded yet (or the list was cleared).

1832 +pub fn get_todos() -> Option<Vec<Todo>> {

1833 + TODOS

1834 + .read()

1835 + .unwrap_or_else(|poisoned| poisoned.into_inner())

1836 + .clone()

1837 +}

1838 +

1839 +/// Serializes tests that touch the process-global `TODOS` store. Without

1840 +/// this, parallel `cargo test` runs clobber each other's list.

1841 +#[cfg(test)]

1842 +static TODOS_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

1843 +

1844 +/// Reset the store. Used by tests so one test's list doesn't leak into the

1845 +/// next — the store is process-global.

1846 +#[cfg(test)]

1847 +fn reset_todos() {

1848 + let mut guard = TODOS

1849 + .write()

1850 + .unwrap_or_else(|poisoned| poisoned.into_inner());

1851 + *guard = None;

1852 +}

1853 +

1854 /// Renders the model's task checklist back as the tool result so the surface

1797 -/// (TUI / ACP client) can show live progress. Pure presentation — the list is

1798 -/// owned by the model, not persisted here.

1855 +/// (TUI / ACP client) can show live progress. Also persists the list so the

1856 +/// panel survives model omissions and error turns — the list is still owned

1857 +/// by the model, but the last-known state is kept as a fallback.

1858 fn exec_write_todos(arguments: &str) -> String {

1859 let args: Value = match serde_json::from_str(arguments) {

1860 Ok(v) => v,

@@ -1808,35 +1867,54 @@ fn exec_write_todos(arguments: &str) -> String {

1867 };

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

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

1811 - // restate at least one step.

1870 + // restate at least one step. Clear the store too so a later omission

1871 + // doesn't bring the old list back from the dead.

1872 if todos.is_empty() {

1873 + let mut guard = TODOS

1874 + .write()

1875 + .unwrap_or_else(|poisoned| poisoned.into_inner());

1876 + *guard = None;

1877 return "Task list cleared.".to_string();

1878 }

1879

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

1881 let mut completed = 0usize;

1882 + let mut persisted = Vec::with_capacity(todos.len());

1883

1884 for (idx, todo) in todos.iter().enumerate() {

1885 let content = match todo.get("content").and_then(Value::as_str) {

1886 Some(c) => c.trim(),

1887 None => return format!("Error: todo #{} is missing \"content\"", idx + 1),

1888 };

1824 - let status = todo

1889 + let status_str = todo

1890 .get("status")

1891 .and_then(Value::as_str)

1892 .unwrap_or("pending");

1893 + let status = TodoStatus::from_str(status_str);

1894

1895 let marker = match status {

1830 - "completed" => {

1896 + TodoStatus::Completed => {

1897 completed += 1;

1898 "[x]"

1899 }

1834 - "in_progress" => "[~]",

1835 - _ => "[ ]",

1900 + TodoStatus::InProgress => "[~]",

1901 + TodoStatus::Pending => "[ ]",

1902 };

1903 lines.push(format!("{marker} {content}"));

1904 + persisted.push(Todo {

1905 + content: content.to_string(),

1906 + status,

1907 + });

1908 }

1909

1910 + // Persist the parsed list so the panel survives a later turn that omits

1911 + // `write_todos` or errors out before reaching it. The store is the

1912 + // fallback `get_todos()` reads.

1913 + let mut guard = TODOS

1914 + .write()

1915 + .unwrap_or_else(|poisoned| poisoned.into_inner());

1916 + *guard = Some(persisted);

1917 +

1918 format!(

1919 "Task list updated ({completed}/{} done):\n{}",

1920 todos.len(),

@@ -4564,4 +4642,97 @@ mod tests {

4642 );

4643 assert!(err.contains("read-only set"), "message explains why: {err}");

4644 }

4645 +

4646 + // ── write_todos persistence ───────────────────────────────────────────

4647 + // The store is process-global, so every test acquires `TODOS_TEST_LOCK`

4648 + // and calls `reset_todos()` first or it inherits whatever the previous

4649 + // test left behind.

4650 +

4651 + #[test]

4652 + fn write_todos_persists_list() {

4653 + let _guard = TODOS_TEST_LOCK.lock().unwrap();

4654 + reset_todos();

4655 + assert!(get_todos().is_none());

4656 +

4657 + let output = exec_write_todos(

4658 + r#"{"todos":[

4659 + {"content":"step one","status":"completed"},

4660 + {"content":"step two","status":"in_progress"},

4661 + {"content":"step three","status":"pending"}

4662 + ]}"#,

4663 + );

4664 + assert!(output.contains("Task list updated (1/3 done)"));

4665 +

4666 + let todos = get_todos().expect("list should be persisted");

4667 + assert_eq!(todos.len(), 3);

4668 + assert_eq!(todos[0].content, "step one");

4669 + assert_eq!(todos[0].status, TodoStatus::Completed);

4670 + assert_eq!(todos[1].status, TodoStatus::InProgress);

4671 + assert_eq!(todos[2].status, TodoStatus::Pending);

4672 + }

4673 +

4674 + #[test]

4675 + fn get_todos_returns_none_after_reset() {

4676 + let _guard = TODOS_TEST_LOCK.lock().unwrap();

4677 + reset_todos();

4678 + assert!(get_todos().is_none());

4679 + }

4680 +

4681 + #[test]

4682 + fn write_todos_empty_list_clears_store() {

4683 + let _guard = TODOS_TEST_LOCK.lock().unwrap();

4684 + // Seed the store so we can prove the empty call wipes it.

4685 + exec_write_todos(r#"{"todos":[{"content":"seed","status":"pending"}]}"#);

4686 + assert!(get_todos().is_some());

4687 +

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

4689 + assert_eq!(output, "Task list cleared.");

4690 + assert!(get_todos().is_none());

4691 + }

4692 +

4693 + #[test]

4694 + fn write_todos_unknown_status_falls_back_to_pending() {

4695 + let _guard = TODOS_TEST_LOCK.lock().unwrap();

4696 + reset_todos();

4697 + exec_write_todos(

4698 + r#"{"todos":[{"content":"weird","status":"bogus"}]}"#,

4699 + );

4700 + let todos = get_todos().expect("list should be persisted");

4701 + assert_eq!(todos.len(), 1);

4702 + assert_eq!(todos[0].status, TodoStatus::Pending);

4703 + }

4704 +

4705 + #[test]

4706 + fn write_todos_missing_status_defaults_to_pending() {

4707 + let _guard = TODOS_TEST_LOCK.lock().unwrap();

4708 + reset_todos();

4709 + exec_write_todos(r#"{"todos":[{"content":"no status"}]}"#);

4710 + let todos = get_todos().expect("list should be persisted");

4711 + assert_eq!(todos.len(), 1);

4712 + assert_eq!(todos[0].status, TodoStatus::Pending);

4713 + }

4714 +

4715 + #[test]

4716 + fn write_todos_overwrites_previous_list() {

4717 + let _guard = TODOS_TEST_LOCK.lock().unwrap();

4718 + reset_todos();

4719 + exec_write_todos(

4720 + r#"{"todos":[

4721 + {"content":"a","status":"pending"},

4722 + {"content":"b","status":"pending"}

4723 + ]}"#,

4724 + );

4725 + assert_eq!(get_todos().unwrap().len(), 2);

4726 +

4727 + // A second call with a different list replaces, not appends.

4728 + exec_write_todos(

4729 + r#"{"todos":[

4730 + {"content":"only","status":"completed"}

4731 + ]}"#,

4732 + );

4733 + let todos = get_todos().unwrap();

4734 + assert_eq!(todos.len(), 1);

4735 + assert_eq!(todos[0].content, "only");

4736 + assert_eq!(todos[0].status, TodoStatus::Completed);

4737 + }

4738 }