@sigitcode-old / sigit / commits / 257aeaf

Fork 1

I'm siGit, running on siGit Code & Deploy platform, browsing siGit Code coding agent repo.

Isolate model roots and todo state per session

Two process-global stores made the CLI unreliable when several sessions share one process. setup.rs built its model store by mutating HF_HUB_CACHE, HF_HOME and HOME from inside tests, guarded by ENV_TEST_LOCK. The model picker reads those variables on other threads, so the lock never isolated the values and setup::tests failed intermittently. Resolve the roots once with ModelRoots::from_env() for production callers and pass them into the internals, so a test points the readers at a scratch directory instead of aiming process-global variables at it. tools.rs kept the todo list in a single process-global slot shared by every thread the process serves. Key it by session, like permissions::sessions(), and clear it when the session ends, so a plan cannot leak into another conversation. The TUI now carries a session key too (permissions::TUI_SESSION) rather than the session-less "" key, so its background tasks, plan and permission grants are torn down on exit; the now-unused TODOS_TEST_LOCK and reset_todos are gone. Co-Authored-By: siGit Code v1.7.0-acp <noreply@sigit.si>

Seto Elkahfi committed Oct 16, 2026 at 21:33 UTC 257aeafd7bcddf3919274727c80e3419d80e69a8

4 files changed +294 -189

crates/cli/src/chat.rs

@@ -1634,6 +1634,10 @@ mod tui {

1634 };

1635 if app.history_pending_delete.as_deref() == Some(id.as_str()) {

1636 crate::session_store::delete(&id);

1637 + // A plan must not outlive its conversation. The id is

1638 + // the live session's key when the user deletes the

1639 + // thread the TUI is currently running.

1640 + crate::tools::clear_todos(&id);

1641 app.refresh_history();

1642 app.history_notice = Some(format!("Deleted session '{id}'."));

1643 } else {

@@ -2837,6 +2841,8 @@ mod tui {

2841 crate::permissions::reset_session(crate::permissions::TUI_SESSION);

2842 // The saved session must not resurrect what the user just wiped.

2843 crate::session_store::delete(TUI_STORE_SESSION);

2844 + // The plan belongs to the conversation, so the wipe drops it too.

2845 + crate::tools::clear_todos(crate::permissions::TUI_SESSION);

2846 app.context_tokens_used = None;

2847 app.messages.push(ChatMessage::system(format!(

2848 "Cleared {cleared} turn(s). History is empty.",

crates/cli/src/main.rs

+52 -21

@@ -743,11 +743,12 @@ fn todos_arguments_to_plan(arguments: &str) -> Option<Plan> {

743 Some(Plan::new(entries))

744 }

745

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

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

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

749 -fn persisted_todos_to_plan() -> Option<Plan> {

750 - let todos = tools::get_todos()?;

746 +/// Rebuilds an ACP [`Plan`] from the session's persisted todo list in

747 +/// `tools::get_todos()`. Used to re-emit the plan on error turns so the client

748 +/// panel stays populated when the model never got to (or forgot to) call

749 +/// `write_todos` this turn.

750 +fn persisted_todos_to_plan(session: &SessionId) -> Option<Plan> {

751 + let todos = tools::get_todos(&session.to_string())?;

752 let entries = todos

753 .into_iter()

754 .map(|todo| {

@@ -3011,10 +3012,11 @@ impl SiGitAgent {

3012 ///

3013 /// Drops everything this process holds for it: the parked state (and with

3014 /// it the MCP servers the client named), its permission grants and plan

3014 - /// mode, and the background commands it started. The saved history stays,

3015 - /// so the thread still lists and reopens. The caller has already signalled

3016 - /// the session's running turn (see `begin_close`) and holds the session's

3017 - /// lock and `workspace_lock`, so that turn has ended by the time this runs.

3015 + /// mode, its todo list, and the background commands it started. The saved

3016 + /// history stays, so the thread still lists and reopens. The caller has

3017 + /// already signalled the session's running turn (see `begin_close`) and

3018 + /// holds the session's lock and `workspace_lock`, so that turn has ended by

3019 + /// the time this runs.

3020 ///

3021 /// An id this process does not hold is answered with success. The spec

3022 /// allows an error there, but the client wants the session gone and it is.

@@ -3050,6 +3052,10 @@ impl SiGitAgent {

3052 self.announced_titles.lock().unwrap().remove(&key);

3053 let killed = tools::kill_session_tasks(&key);

3054 tools::forget_abort(&key);

3055 + // The plan is the conversation's, so it goes with the conversation:

3056 + // an id that comes back later starts with an empty panel rather than

3057 + // the previous life's list.

3058 + tools::clear_todos(&key);

3059 self.closing_sessions.lock().unwrap().remove(&key);

3060

3061 log::info!(

@@ -3599,7 +3605,7 @@ impl SiGitAgent {

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

3606 // empty when the initial inference call fails before the model

3607 // can call `write_todos`.

3602 - if let Some(plan) = persisted_todos_to_plan() {

3608 + if let Some(plan) = persisted_todos_to_plan(&session_id) {

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

3610 }

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

@@ -3996,7 +4002,7 @@ impl SiGitAgent {

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

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

4004 // empty when a tool round fails mid-turn.

3999 - if let Some(plan) = persisted_todos_to_plan() {

4005 + if let Some(plan) = persisted_todos_to_plan(&session_id) {

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

4007 }

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

@@ -5492,6 +5498,13 @@ async fn exec_slash_acp(

5498 permissions::reset_session(&session_key);

5499 // The saved session must not resurrect what the user just wiped.

5500 session_store::delete(&session_key);

5501 + // The plan belongs to the conversation, so the wipe drops it too.

5502 + // The empty plan is what clears the client's panel: without it the

5503 + // editor keeps showing the list of the thread that no longer exists.

5504 + tools::clear_todos(&session_key);

5505 + agent

5506 + .send_plan_update(cx, session_id.clone(), Plan::new(Vec::new()))

5507 + .ok();

5508 // The wipe drops the session's Permissions override too; push the

5509 // refreshed chip so it doesn't keep showing a stale pick.

5510 let config_options = {

@@ -5922,6 +5935,12 @@ fn init_logging(is_tty: bool) {

5935 /// LeaveAlternateScreen (ratatui 0.29 hides `writer_mut()`).

5936 #[cfg(unix)]

5937 async fn run_interactive(tty: std::fs::File, mut cleanup_tty: std::fs::File) -> anyhow::Result<()> {

5938 + // The TUI is one long-lived conversation and shares the key the permission

5939 + // and session stores already use, so its tool calls are scoped to it —

5940 + // background task ownership and the todo list — instead of the session-less

5941 + // `""` key, which no teardown path can name.

5942 + tools::set_active_session(Some(permissions::TUI_SESSION));

5943 +

5944 let engine = Arc::new(ChatEngine::new());

5945

5946 let startup_selection = setup::startup_model_selection();

@@ -6042,6 +6061,18 @@ async fn run_interactive(tty: std::fs::File, mut cleanup_tty: std::fs::File) ->

6061 }

6062 }

6063

6064 + // The conversation is over, however it ended (`/exit`, Ctrl+C, a panic in

6065 + // the event loop). Drop what it owned — background tasks, the plan, the

6066 + // permission grants and the abort slot — and detach the active session, so

6067 + // nothing outlives the conversation. The saved history deliberately stays:

6068 + // it is what `/resume` brings back on the next launch.

6069 + let killed = tools::kill_session_tasks(permissions::TUI_SESSION);

6070 + tools::clear_todos(permissions::TUI_SESSION);

6071 + tools::forget_abort(permissions::TUI_SESSION);

6072 + permissions::reset_session(permissions::TUI_SESSION);

6073 + tools::set_active_session(None);

6074 + log::info!("tui exit: {killed} background task(s) stopped");

6075 +

6076 chat_result

6077 }

6078

@@ -6896,7 +6927,7 @@ mod tests {

6927 async fn unknown_session_is_rejected_instead_of_borrowing_live_cwd() {

6928 let agent = SiGitAgent::new(

6929 Arc::new(ChatEngine::new()),

6899 - default_local_model_config(),

6930 + GgufModelConfig::qwen25_3b(),

6931 Arc::new(AtomicBool::new(true)),

6932 Arc::new(AtomicBool::new(false)),

6933 Arc::new(std::sync::Mutex::new(None)),

@@ -6917,7 +6948,7 @@ mod tests {

6948 async fn a_turn_leaves_no_cancellation_behind_however_it_returns() {

6949 let agent = SiGitAgent::new(

6950 Arc::new(ChatEngine::new()),

6920 - default_local_model_config(),

6951 + GgufModelConfig::qwen25_3b(),

6952 Arc::new(AtomicBool::new(true)),

6953 Arc::new(AtomicBool::new(false)),

6954 Arc::new(std::sync::Mutex::new(None)),

@@ -6956,7 +6987,7 @@ mod tests {

6987 async fn a_finished_turn_does_not_remove_its_successors_cancellation() {

6988 let agent = SiGitAgent::new(

6989 Arc::new(ChatEngine::new()),

6959 - default_local_model_config(),

6990 + GgufModelConfig::qwen25_3b(),

6991 Arc::new(AtomicBool::new(true)),

6992 Arc::new(AtomicBool::new(false)),

6993 Arc::new(std::sync::Mutex::new(None)),

@@ -6982,7 +7013,7 @@ mod tests {

7013 async fn cancel_signals_only_the_matching_active_prompt() {

7014 let agent = SiGitAgent::new(

7015 Arc::new(ChatEngine::new()),

6985 - default_local_model_config(),

7016 + GgufModelConfig::qwen25_3b(),

7017 Arc::new(AtomicBool::new(true)),

7018 Arc::new(AtomicBool::new(false)),

7019 Arc::new(std::sync::Mutex::new(None)),

@@ -7034,7 +7065,7 @@ mod tests {

7065 std::env::set_var("SIGIT_LOCAL_INFERENCE", "on");

7066 }

7067

7037 - let local = default_local_model_config();

7068 + let local = GgufModelConfig::qwen25_3b();

7069 let agent = SiGitAgent::new(

7070 Arc::new(ChatEngine::new()),

7071 local.clone(),

@@ -7131,7 +7162,7 @@ mod tests {

7162

7163 let agent = SiGitAgent::new(

7164 Arc::new(ChatEngine::new()),

7134 - default_local_model_config(),

7165 + GgufModelConfig::qwen25_3b(),

7166 Arc::new(AtomicBool::new(true)),

7167 Arc::new(AtomicBool::new(false)),

7168 Arc::new(std::sync::Mutex::new(None)),

@@ -7287,7 +7318,7 @@ mod tests {

7318

7319 let agent = SiGitAgent::new(

7320 Arc::new(ChatEngine::new()),

7290 - default_local_model_config(),

7321 + GgufModelConfig::qwen25_3b(),

7322 Arc::new(AtomicBool::new(true)),

7323 Arc::new(AtomicBool::new(false)),

7324 Arc::new(std::sync::Mutex::new(None)),

@@ -7378,7 +7409,7 @@ mod tests {

7409

7410 let agent = SiGitAgent::new(

7411 Arc::new(ChatEngine::new()),

7381 - default_local_model_config(),

7412 + GgufModelConfig::qwen25_3b(),

7413 Arc::new(AtomicBool::new(true)),

7414 Arc::new(AtomicBool::new(false)),

7415 Arc::new(std::sync::Mutex::new(None)),

@@ -7477,7 +7508,7 @@ mod tests {

7508 .unwrap_or_else(|poisoned| poisoned.into_inner());

7509 let agent = Arc::new(SiGitAgent::new(

7510 Arc::new(ChatEngine::new()),

7480 - default_local_model_config(),

7511 + GgufModelConfig::qwen25_3b(),

7512 Arc::new(AtomicBool::new(true)),

7513 Arc::new(AtomicBool::new(false)),

7514 Arc::new(std::sync::Mutex::new(None)),

@@ -7525,7 +7556,7 @@ mod tests {

7556 .unwrap_or_else(|poisoned| poisoned.into_inner());

7557 let agent = SiGitAgent::new(

7558 Arc::new(ChatEngine::new()),

7528 - default_local_model_config(),

7559 + GgufModelConfig::qwen25_3b(),

7560 Arc::new(AtomicBool::new(true)),

7561 Arc::new(AtomicBool::new(false)),

7562 Arc::new(std::sync::Mutex::new(None)),

crates/cli/src/setup.rs

+117 -102

@@ -109,27 +109,70 @@ pub enum ModelCacheHealth {

109 NotDownloaded,

110 }

111

112 +/// Where the model store lives, resolved once and handed to the readers.

113 +///

114 +/// Resolving it from the environment is a global decision, so a caller that

115 +/// needs a store of its own — a test pointing at a scratch directory — passes

116 +/// the roots in rather than mutating `HF_HUB_CACHE`, `HF_HOME` and `HOME` out

117 +/// from under whatever else is running in the same process.

118 +#[derive(Debug, Clone, Default)]

119 +struct ModelRoots {

120 + /// Onde App Group model hub, when the signed app has run at least once.

121 + app_group: Option<PathBuf>,

122 + /// `HF_HUB_CACHE`, or `HF_HOME/hub`, when set and present.

123 + configured_cache: Option<PathBuf>,

124 + /// `~/.cache/huggingface/hub`, when present.

125 + default_cache: Option<PathBuf>,

126 + /// File holding the persisted selection.

127 + selected_model_file: Option<PathBuf>,

128 +}

129 +

130 +impl ModelRoots {

131 + fn from_env() -> Self {

132 + Self {

133 + app_group: app_group_models_root(),

134 + configured_cache: hf_cache_root(),

135 + default_cache: default_hf_cache_root(),

136 + selected_model_file: selected_model_file_path(),

137 + }

138 + }

139 +

140 + /// One cache dir and one selection file, with no environment lookups at all.

141 + #[cfg(test)]

142 + fn in_dir(cache_root: &Path, selected_model_file: &Path) -> Self {

143 + Self {

144 + configured_cache: Some(cache_root.to_path_buf()),

145 + selected_model_file: Some(selected_model_file.to_path_buf()),

146 + ..Self::default()

147 + }

148 + }

149 +}

150 +

151 /// find all GGUF models on disk. checks Onde app group first, then HF cache.

152 pub fn discover_local_models() -> Vec<DiscoveredModel> {

153 + discover_local_models_in(&ModelRoots::from_env())

154 +}

155 +

156 +fn discover_local_models_in(roots: &ModelRoots) -> Vec<DiscoveredModel> {

157 let mut models = Vec::new();

158 let mut seen_roots = Vec::new();

159

117 - if let Some(app_group_models) = app_group_models_root() {

160 + if let Some(app_group_models) = roots.app_group.as_ref() {

161 seen_roots.push(app_group_models.clone());

119 - collect_models_from_cache_root(&app_group_models, true, &mut models);

162 + collect_models_from_cache_root(app_group_models, true, &mut models);

163 }

164

122 - if let Some(hf_cache) = hf_cache_root()

123 - && !seen_roots.iter().any(|root| root == &hf_cache)

165 + if let Some(hf_cache) = roots.configured_cache.as_ref()

166 + && !seen_roots.iter().any(|root| root == hf_cache)

167 {

168 seen_roots.push(hf_cache.clone());

126 - collect_models_from_cache_root(&hf_cache, false, &mut models);

169 + collect_models_from_cache_root(hf_cache, false, &mut models);

170 }

171

129 - if let Some(default_hf_cache) = default_hf_cache_root()

130 - && !seen_roots.iter().any(|root| root == &default_hf_cache)

172 + if let Some(default_hf_cache) = roots.default_cache.as_ref()

173 + && !seen_roots.iter().any(|root| root == default_hf_cache)

174 {

132 - collect_models_from_cache_root(&default_hf_cache, false, &mut models);

175 + collect_models_from_cache_root(default_hf_cache, false, &mut models);

176 }

177

178 models.sort_by(|left, right| {

@@ -300,7 +343,10 @@ fn default_hf_cache_root() -> Option<PathBuf> {

343 }

344

345 pub fn load_selected_model() -> Option<SelectedModel> {

303 - let path = selected_model_file_path()?;

346 + load_selected_model_from(&selected_model_file_path()?)

347 +}

348 +

349 +fn load_selected_model_from(path: &Path) -> Option<SelectedModel> {

350 let contents = std::fs::read_to_string(path).ok()?;

351 let trimmed = contents.trim();

352 if trimmed.is_empty() {

@@ -333,9 +379,16 @@ pub fn load_selected_model_name() -> Option<String> {

379 /// pick a model for startup: saved selection > first local model > none.

380 /// if we fall back to a local model, persist it so ACP and TUI agree next time.

381 pub fn startup_model_selection() -> Option<StartupModelSelection> {

336 - let discovered = discover_local_models();

382 + startup_model_selection_in(&ModelRoots::from_env())

383 +}

384

338 - if let Some(saved_model) = load_selected_model()

385 +fn startup_model_selection_in(roots: &ModelRoots) -> Option<StartupModelSelection> {

386 + let discovered = discover_local_models_in(roots);

387 +

388 + if let Some(saved_model) = roots

389 + .selected_model_file

390 + .as_deref()

391 + .and_then(load_selected_model_from)

392 && let Some(model) = discovered.iter().find(|model| {

393 saved_model.matches(model) && model.cache_health == ModelCacheHealth::Complete

394 })

@@ -351,7 +404,9 @@ pub fn startup_model_selection() -> Option<StartupModelSelection> {

404 .find(|model| model.cache_health == ModelCacheHealth::Complete)

405 .map(|model| {

406 let selected_model = SelectedModel::from_discovered(&model);

354 - let _ = save_selected_model(&selected_model);

407 + if let Some(path) = roots.selected_model_file.as_deref() {

408 + let _ = save_selected_model_to(path, &selected_model);

409 + }

410 StartupModelSelection {

411 display_name: model.display_name.clone(),

412 selected_model: Some(selected_model),

@@ -363,6 +418,10 @@ pub fn save_selected_model(selected_model: &SelectedModel) -> Result<(), String>

418 let path = selected_model_file_path()

419 .ok_or_else(|| "Could not determine where to store the selected model.".to_string())?;

420

421 + save_selected_model_to(&path, selected_model)

422 +}

423 +

424 +fn save_selected_model_to(path: &Path, selected_model: &SelectedModel) -> Result<(), String> {

425 if let Some(parent) = path.parent()

426 && !parent.exists()

427 {

@@ -375,7 +434,7 @@ pub fn save_selected_model(selected_model: &SelectedModel) -> Result<(), String>

434 selected_model.model_id, selected_model.gguf_file

435 );

436

378 - std::fs::write(&path, contents)

437 + std::fs::write(path, contents)

438 .map_err(|error| format!("Could not save selected model: {error}"))

439 }

440

@@ -431,22 +490,32 @@ fn resolve_shared_container() -> Option<PathBuf> {

490 #[cfg(test)]

491 mod tests {

492 use super::*;

434 - use std::sync::Mutex;

493

436 - /// The crate-wide lock, not one of this module's own: the model picker

437 - /// reads this cache from other modules' tests, and they must not see it

438 - /// pointed at a scratch directory halfway through.

439 - fn env_lock() -> &'static Mutex<()> {

440 - &crate::ENV_TEST_LOCK

494 + /// A cache dir and selection file owned by this test. The model store is

495 + /// resolved from `HF_HUB_CACHE`/`HF_HOME`/`HOME`, which are process-global

496 + /// and read by the picker on other threads, so tests hand their roots in

497 + /// instead of aiming those variables at a scratch directory.

498 + fn test_roots(cache_root: &Path, selection_dir: &Path) -> ModelRoots {

499 + ModelRoots::in_dir(cache_root, &selection_dir.join(SELECTED_MODEL_FILE_NAME))

500 }

501

502 + /// A scratch directory no other test in this run can land on. The clock

503 + /// alone is not enough: its resolution is coarse on some platforms and

504 + /// parallel test threads can read the same tick, so the pid and a

505 + /// process-local counter are mixed in.

506 fn unique_temp_dir(name: &str) -> PathBuf {

507 + static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);

508 +

509 let nanos = std::time::SystemTime::now()

510 .duration_since(std::time::UNIX_EPOCH)

511 .expect("system time before unix epoch")

512 .as_nanos();

513 + let seq = COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed);

514

449 - std::env::temp_dir().join(format!("sigit-setup-tests-{name}-{nanos}"))

515 + std::env::temp_dir().join(format!(

516 + "sigit-setup-tests-{name}-{}-{seq}-{nanos}",

517 + std::process::id()

518 + ))

519 }

520

521 fn create_snapshot(

@@ -474,48 +543,11 @@ mod tests {

543 snapshot_dir

544 }

545

477 - fn with_test_env<T>(hf_hub_cache: &Path, hf_home: &Path, f: impl FnOnce() -> T) -> T {

478 - let _guard = env_lock().lock().expect("lock env");

479 -

480 - let old_hf_hub_cache = std::env::var_os("HF_HUB_CACHE");

481 - let old_hf_home = std::env::var_os("HF_HOME");

482 - let old_home = std::env::var_os("HOME");

483 -

484 - // SAFETY: tests serialize environment mutation with a process-wide mutex.

485 - unsafe {

486 - std::env::set_var("HF_HUB_CACHE", hf_hub_cache);

487 - std::env::set_var("HF_HOME", hf_home);

488 - std::env::set_var("HOME", hf_home);

489 - }

490 -

491 - let result = f();

492 -

493 - // SAFETY: tests serialize environment mutation with a process-wide mutex.

494 - unsafe {

495 - match old_hf_hub_cache {

496 - Some(value) => std::env::set_var("HF_HUB_CACHE", value),

497 - None => std::env::remove_var("HF_HUB_CACHE"),

498 - }

499 - match old_hf_home {

500 - Some(value) => std::env::set_var("HF_HOME", value),

501 - None => std::env::remove_var("HF_HOME"),

502 - }

503 - match old_home {

504 - Some(value) => std::env::set_var("HOME", value),

505 - None => std::env::remove_var("HOME"),

506 - }

507 - }

508 -

509 - result

510 - }

511 -

546 #[test]

547 fn discover_local_models_marks_complete_and_incomplete_snapshots() {

548 let root = unique_temp_dir("discover-health");

549 let cache_root = root.join("hf-cache");

516 - let hf_home = root.join("hf-home");

550 std::fs::create_dir_all(&cache_root).expect("create cache root");

518 - std::fs::create_dir_all(&hf_home).expect("create hf home");

551

552 create_snapshot(

553 &cache_root,

@@ -532,7 +564,7 @@ mod tests {

564 false,

565 );

566

535 - let models = with_test_env(&cache_root, &hf_home, discover_local_models);

567 + let models = discover_local_models_in(&test_roots(&cache_root, &root));

568

569 assert_eq!(models.len(), 2);

570

@@ -557,9 +589,7 @@ mod tests {

589 fn startup_model_selection_skips_saved_incomplete_model_and_picks_complete_one() {

590 let root = unique_temp_dir("startup-selection");

591 let cache_root = root.join("hf-cache");

560 - let hf_home = root.join("hf-home");

592 std::fs::create_dir_all(&cache_root).expect("create cache root");

562 - std::fs::create_dir_all(&hf_home).expect("create hf home");

593

594 create_snapshot(

595 &cache_root,

@@ -576,20 +606,18 @@ mod tests {

606 true,

607 );

608

579 - let selection = with_test_env(&cache_root, &hf_home, || {

580 - let selected_path = selected_model_file_path().expect("selected model path");

581 - if let Some(parent) = selected_path.parent() {

582 - std::fs::create_dir_all(parent).expect("create selected model parent");

583 - }

609 + let roots = test_roots(&cache_root, &root);

610 + let selected_path = roots

611 + .selected_model_file

612 + .clone()

613 + .expect("selected model path");

614 + std::fs::write(

615 + &selected_path,

616 + "bartowski/Qwen2.5-Coder-3B-Instruct-GGUF\nQwen2.5-Coder-3B-Instruct-Q4_K_M.gguf\n",

617 + )

618 + .expect("write selected model");

619

585 - std::fs::write(

586 - &selected_path,

587 - "bartowski/Qwen2.5-Coder-3B-Instruct-GGUF\nQwen2.5-Coder-3B-Instruct-Q4_K_M.gguf\n",

588 - )

589 - .expect("write selected model");

590 -

591 - startup_model_selection().expect("startup selection")

592 - });

620 + let selection = startup_model_selection_in(&roots).expect("startup selection");

621

622 assert_eq!(

623 selection.display_name,

@@ -606,11 +634,9 @@ mod tests {

634 fn discover_empty_cache_returns_no_models() {

635 let root = unique_temp_dir("discover-empty");

636 let cache_root = root.join("hf-cache");

609 - let hf_home = root.join("hf-home");

637 std::fs::create_dir_all(&cache_root).expect("create cache root");

611 - std::fs::create_dir_all(&hf_home).expect("create hf home");

638

613 - let models = with_test_env(&cache_root, &hf_home, discover_local_models);

639 + let models = discover_local_models_in(&test_roots(&cache_root, &root));

640 assert!(models.is_empty());

641

642 std::fs::remove_dir_all(root).expect("remove temp dir");

@@ -620,9 +646,7 @@ mod tests {

646 fn complete_models_sort_before_incomplete() {

647 let root = unique_temp_dir("sort-order");

648 let cache_root = root.join("hf-cache");

623 - let hf_home = root.join("hf-home");

649 std::fs::create_dir_all(&cache_root).expect("create cache root");

625 - std::fs::create_dir_all(&hf_home).expect("create hf home");

650

651 create_snapshot(

652 &cache_root,

@@ -639,7 +663,7 @@ mod tests {

663 true,

664 );

665

642 - let models = with_test_env(&cache_root, &hf_home, discover_local_models);

666 + let models = discover_local_models_in(&test_roots(&cache_root, &root));

667 assert_eq!(models.len(), 2);

668 assert_eq!(models[0].cache_health, ModelCacheHealth::Complete);

669 assert_eq!(models[1].cache_health, ModelCacheHealth::Incomplete);

@@ -650,21 +674,19 @@ mod tests {

674 #[test]

675 fn load_selected_model_roundtrip() {

676 let root = unique_temp_dir("persistence-roundtrip");

653 - let hf_home = root.join("hf-home");

654 - std::fs::create_dir_all(&hf_home).expect("create hf home");

677 + let path = root.join(SELECTED_MODEL_FILE_NAME);

678

656 - with_test_env(&hf_home, &hf_home, || {

657 - let original = SelectedModel {

658 - model_id: "bartowski/Qwen_Qwen3-4B-GGUF".to_string(),

659 - gguf_file: "Qwen_Qwen3-4B-Q4_K_M.gguf".to_string(),

660 - };

679 + let original = SelectedModel {

680 + model_id: "bartowski/Qwen_Qwen3-4B-GGUF".to_string(),

681 + gguf_file: "Qwen_Qwen3-4B-Q4_K_M.gguf".to_string(),

682 + };

683

662 - save_selected_model(&original).expect("save");

663 - let loaded = load_selected_model().expect("load");

684 + // The parent dir does not exist yet: saving has to create it.

685 + save_selected_model_to(&path, &original).expect("save");

686 + let loaded = load_selected_model_from(&path).expect("load");

687

665 - assert_eq!(loaded.model_id, original.model_id);

666 - assert_eq!(loaded.gguf_file, original.gguf_file);

667 - });

688 + assert_eq!(loaded.model_id, original.model_id);

689 + assert_eq!(loaded.gguf_file, original.gguf_file);

690

691 std::fs::remove_dir_all(root).expect("remove temp dir");

692 }

@@ -672,18 +694,11 @@ mod tests {

694 #[test]

695 fn load_selected_model_empty_file_returns_none() {

696 let root = unique_temp_dir("persistence-empty");

675 - let hf_home = root.join("hf-home");

676 - std::fs::create_dir_all(&hf_home).expect("create hf home");

697 + let path = root.join(SELECTED_MODEL_FILE_NAME);

698 + std::fs::create_dir_all(&root).expect("create root");

699 + std::fs::write(&path, "").expect("write empty");

700

678 - with_test_env(&hf_home, &hf_home, || {

679 - let path = selected_model_file_path().expect("path");

680 - if let Some(parent) = path.parent() {

681 - std::fs::create_dir_all(parent).expect("create parent");

682 - }

683 - std::fs::write(&path, "").expect("write empty");

684 -

685 - assert!(load_selected_model().is_none());

686 - });

701 + assert!(load_selected_model_from(&path).is_none());

702

703 std::fs::remove_dir_all(root).expect("remove temp dir");

704 }

crates/cli/src/tools.rs

+119 -66

@@ -577,7 +577,9 @@ fn execute_sync_tool(name: &str, arguments: &str, owner: Option<&str>) -> ToolOu

577 "read_website" => exec_read_website(arguments),

578 "create_directory" => exec_create_directory(arguments),

579 "glob" => exec_glob(arguments),

580 - "write_todos" => exec_write_todos(arguments),

580 + // A missing owner is a session-less call (the headless runner sets

581 + // one too), and its list is scoped to that same empty key.

582 + "write_todos" => exec_write_todos(arguments, owner.unwrap_or("")),

583 "remember" => exec_remember(arguments),

584 "delete_file" => exec_delete_file(arguments),

585 "run_command" => exec_run_command(arguments, owner),

@@ -2081,43 +2083,58 @@ impl TodoStatus {

2083 }

2084 }

2085

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

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

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

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

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

2089 -/// help.

2090 -static TODOS: RwLock<Option<Vec<Todo>>> = RwLock::new(None);

2091 -

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

2093 -/// call has succeeded yet (or the list was cleared).

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

2095 - TODOS

2096 - .read()

2097 - .unwrap_or_else(|poisoned| poisoned.into_inner())

2098 - .clone()

2086 +/// Per-session todo store. The model owns the list conceptually, but when it

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

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

2089 +/// the last-known list so `main.rs` can re-emit it (`todos_arguments_to_plan` +

2090 +/// `SessionUpdate::Plan`) without the model's help.

2091 +///

2092 +/// Keyed by session, like `permissions::sessions()`: one process serves every

2093 +/// thread an editor has open, and a plan belonging to one conversation must

2094 +/// never surface in another.

2095 +fn todo_sessions() -> &'static Mutex<HashMap<String, Vec<Todo>>> {

2096 + static SESSIONS: OnceLock<Mutex<HashMap<String, Vec<Todo>>>> = OnceLock::new();

2097 + SESSIONS.get_or_init(|| Mutex::new(HashMap::new()))

2098 }

2099

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

2102 -/// this, parallel `cargo test` runs clobber each other's list.

2103 -#[cfg(test)]

2104 -static TODOS_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

2100 +/// Read the session's persisted todo list, if any. Returns `None` when no

2101 +/// `write_todos` call has succeeded yet (or the list was cleared).

2102 +pub fn get_todos(session: &str) -> Option<Vec<Todo>> {

2103 + todo_sessions()

2104 + .lock()

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

2106 + .get(session)

2107 + .cloned()

2108 +}

2109

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

2107 -/// next — the store is process-global.

2108 -#[cfg(test)]

2109 -fn reset_todos() {

2110 - let mut guard = TODOS

2111 - .write()

2110 +/// Store the session's list, or drop the key when `None`. Dropping it rather

2111 +/// than storing an empty list keeps "no plan" a single state the map doesn't

2112 +/// accumulate entries for.

2113 +fn set_todos(session: &str, todos: Option<Vec<Todo>>) {

2114 + let mut map = todo_sessions()

2115 + .lock()

2116 .unwrap_or_else(|poisoned| poisoned.into_inner());

2113 - *guard = None;

2117 + match todos {

2118 + Some(todos) => {

2119 + map.insert(session.to_string(), todos);

2120 + }

2121 + None => {

2122 + map.remove(session);

2123 + }

2124 + }

2125 +}

2126 +

2127 +/// Forget a session's list (session teardown, a wiped conversation) so a plan

2128 +/// never outlives the conversation it belongs to.

2129 +pub fn clear_todos(session: &str) {

2130 + set_todos(session, None);

2131 }

2132

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

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

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

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

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

2137 +fn exec_write_todos(arguments: &str, session: &str) -> String {

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

2139 Ok(v) => v,

2140 Err(err) => return format!("Error: failed to parse arguments: {err}"),

@@ -2132,10 +2149,7 @@ fn exec_write_todos(arguments: &str) -> String {

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

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

2151 if todos.is_empty() {

2135 - let mut guard = TODOS

2136 - .write()

2137 - .unwrap_or_else(|poisoned| poisoned.into_inner());

2138 - *guard = None;

2152 + set_todos(session, None);

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

2154 }

2155

@@ -2172,10 +2186,7 @@ fn exec_write_todos(arguments: &str) -> String {

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

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

2188 // fallback `get_todos()` reads.

2175 - let mut guard = TODOS

2176 - .write()

2177 - .unwrap_or_else(|poisoned| poisoned.into_inner());

2178 - *guard = Some(persisted);

2189 + set_todos(session, Some(persisted));

2190

2191 format!(

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

@@ -3569,7 +3580,7 @@ mod tests {

3580 ]

3581 })

3582 .to_string();

3572 - let result = exec_write_todos(&args);

3583 + let result = exec_write_todos(&args, "todos-test-render");

3584 assert!(result.contains("1/3 done"), "{result}");

3585 assert!(result.contains("[x] Read code"), "{result}");

3586 assert!(result.contains("[~] Make change"), "{result}");

@@ -3578,7 +3589,7 @@ mod tests {

3589

3590 #[test]

3591 fn test_write_todos_accepts_an_empty_list_to_clear() {

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

3592 + let result = exec_write_todos(r#"{"todos":[]}"#, "todos-test-empty-clear");

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

3594 }

3595

@@ -5378,15 +5389,14 @@ mod tests {

5389 }

5390

5391 // ── write_todos persistence ───────────────────────────────────────────

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

5382 - // and calls `reset_todos()` first or it inherits whatever the previous

5383 - // test left behind.

5392 + // Each test owns a distinct session key, so the store's per-session

5393 + // scoping is what keeps parallel `cargo test` runs out of each other's

5394 + // lists — no shared lock needed.

5395

5396 #[test]

5397 fn write_todos_persists_list() {

5387 - let _guard = TODOS_TEST_LOCK.lock().unwrap();

5388 - reset_todos();

5389 - assert!(get_todos().is_none());

5398 + let session = "todos-test-persists";

5399 + assert!(get_todos(session).is_none());

5400

5401 let output = exec_write_todos(

5402 r#"{"todos":[

@@ -5394,10 +5404,11 @@ mod tests {

5404 {"content":"step two","status":"in_progress"},

5405 {"content":"step three","status":"pending"}

5406 ]}"#,

5407 + session,

5408 );

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

5410

5400 - let todos = get_todos().expect("list should be persisted");

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

5412 assert_eq!(todos.len(), 3);

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

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

@@ -5406,63 +5417,105 @@ mod tests {

5417 }

5418

5419 #[test]

5409 - fn get_todos_returns_none_after_reset() {

5410 - let _guard = TODOS_TEST_LOCK.lock().unwrap();

5411 - reset_todos();

5412 - assert!(get_todos().is_none());

5420 + fn write_todos_is_scoped_to_one_session() {

5421 + exec_write_todos(

5422 + r#"{"todos":[{"content":"mine","status":"pending"}]}"#,

5423 + "todos-test-scope-a",

5424 + );

5425 +

5426 + // A second session starts empty rather than inheriting the first

5427 + // session's plan, and writing there leaves the first untouched.

5428 + assert!(get_todos("todos-test-scope-b").is_none());

5429 + exec_write_todos(

5430 + r#"{"todos":[{"content":"theirs","status":"completed"}]}"#,

5431 + "todos-test-scope-b",

5432 + );

5433 +

5434 + let mine = get_todos("todos-test-scope-a").expect("first session keeps its list");

5435 + assert_eq!(mine.len(), 1);

5436 + assert_eq!(mine[0].content, "mine");

5437 + let theirs = get_todos("todos-test-scope-b").expect("second session has its own");

5438 + assert_eq!(theirs[0].content, "theirs");

5439 + }

5440 +

5441 + #[test]

5442 + fn get_todos_returns_none_for_unknown_session() {

5443 + assert!(get_todos("todos-test-unknown").is_none());

5444 + }

5445 +

5446 + #[test]

5447 + fn clear_todos_forgets_only_that_session() {

5448 + exec_write_todos(

5449 + r#"{"todos":[{"content":"gone","status":"pending"}]}"#,

5450 + "todos-test-clear",

5451 + );

5452 + exec_write_todos(

5453 + r#"{"todos":[{"content":"kept","status":"pending"}]}"#,

5454 + "todos-test-clear-other",

5455 + );

5456 +

5457 + clear_todos("todos-test-clear");

5458 +

5459 + assert!(get_todos("todos-test-clear").is_none());

5460 + assert!(get_todos("todos-test-clear-other").is_some());

5461 }

5462

5463 #[test]

5464 fn write_todos_empty_list_clears_store() {

5417 - let _guard = TODOS_TEST_LOCK.lock().unwrap();

5465 + let session = "todos-test-empty";

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

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

5420 - assert!(get_todos().is_some());

5467 + exec_write_todos(

5468 + r#"{"todos":[{"content":"seed","status":"pending"}]}"#,

5469 + session,

5470 + );

5471 + assert!(get_todos(session).is_some());

5472

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

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

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

5424 - assert!(get_todos().is_none());

5475 + assert!(get_todos(session).is_none());

5476 }

5477

5478 #[test]

5479 fn write_todos_unknown_status_falls_back_to_pending() {

5429 - let _guard = TODOS_TEST_LOCK.lock().unwrap();

5430 - reset_todos();

5431 - exec_write_todos(r#"{"todos":[{"content":"weird","status":"bogus"}]}"#);

5432 - let todos = get_todos().expect("list should be persisted");

5480 + let session = "todos-test-unknown-status";

5481 + exec_write_todos(

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

5483 + session,

5484 + );

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

5486 assert_eq!(todos.len(), 1);

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

5488 }

5489

5490 #[test]

5491 fn write_todos_missing_status_defaults_to_pending() {

5439 - let _guard = TODOS_TEST_LOCK.lock().unwrap();

5440 - reset_todos();

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

5442 - let todos = get_todos().expect("list should be persisted");

5492 + let session = "todos-test-missing-status";

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

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

5495 assert_eq!(todos.len(), 1);

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

5497 }

5498

5499 #[test]

5500 fn write_todos_overwrites_previous_list() {

5449 - let _guard = TODOS_TEST_LOCK.lock().unwrap();

5450 - reset_todos();

5501 + let session = "todos-test-overwrite";

5502 exec_write_todos(

5503 r#"{"todos":[

5504 {"content":"a","status":"pending"},

5505 {"content":"b","status":"pending"}

5506 ]}"#,

5507 + session,

5508 );

5457 - assert_eq!(get_todos().unwrap().len(), 2);

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

5510

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

5512 exec_write_todos(

5513 r#"{"todos":[

5514 {"content":"only","status":"completed"}

5515 ]}"#,

5516 + session,

5517 );

5465 - let todos = get_todos().unwrap();

5518 + let todos = get_todos(session).unwrap();

5519 assert_eq!(todos.len(), 1);

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

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