From 1702eec79ca3884dc30fb54e5f803d704821a2f5 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Sun, 27 Sep 2026 01:40:28 -0700 Subject: [PATCH 1/2] fix(tui): scope /undo to the paths the undone step changed (#6644) Cause: `/undo` picked the newest current-session `tool:`/`pre-turn:` snapshot that differed from the workspace and ran `SnapshotRepo::restore`, a whole-tree checkout. Edits made after that snapshot were reverted too, including the user's own edits to other files. Candidates came from `list(100)`, so an older restore point still in the store looked absent, and a fork could not use the snapshots of the turns it inherited because they carry the source session's tag. Fix: reuse the Runtime's path-scoped contract from #6645. A step runs from its restore point to the next restore point the conversation owns (or the workspace now, for the newest open step); only the paths `changed_paths_between` reports for it are restored, via `restore_path_plan` with a pre-restore safety snapshot and a preflight re-check. A path that changed since (matches neither the step's end nor an older restore point an earlier /undo walked it back to) is refused and nothing is changed. Lookup is uncapped and resolved by tree id. A fork owns its source's snapshots up to the fork time, following the saved-session lineage (`parent_session_id`, new metadata-only `SessionManager::load_session_metadata_by_id`). Tool-by-tool stepping (#384) is kept. `snapshot_diff_stat` lost its only caller and is removed. Tests: five regression tests in commands/groups/debug/tests.rs (path scope, changed-since refusal, tool-by-tool stepping, >100 snapshots, fork inheritance); all five fail on the previous implementation (test result: FAILED. 0 passed; 5 failed). - cargo test -p codewhale-tui --lib -- commands::groups::debug::tests: test result: ok. 65 passed; 0 failed - cargo test -p codewhale-tui --lib -- snapshot::: test result: ok. 76 passed; 0 failed - cargo clippy -p codewhale-tui --lib --tests (CI flags): clean Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 + crates/tui/CHANGELOG.md | 6 + crates/tui/src/commands/groups/debug/tests.rs | 212 +++++++++++ crates/tui/src/commands/groups/debug/undo.rs | 360 ++++++++++++++---- crates/tui/src/session_manager.rs | 5 + crates/tui/src/snapshot/repo.rs | 63 --- 6 files changed, 518 insertions(+), 134 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e398bd7f9c..08c1719da7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,12 @@ quieter, and Fleet runs can be checked before they spend anything. ### Fixed +- TUI `/undo` now restores only the files the undone tool call or turn + changed, and refuses, changing nothing, when one of them changed since. + Before, it checked out the whole snapshot tree, which also reverted later + edits to other files. It also finds restore points older than the newest + 100 snapshots, and a forked session can undo the turns it inherited + ([#6644](https://github.com/Hmbown/Codewhale/issues/6644)). - A top-level `base_url` or `api_key` in `config.toml` now means one thing everywhere. Every reader used its own rule for which routes inherited it, which is how a DeepSeek endpoint became the Xiaomi MiMo route's and failed diff --git a/crates/tui/CHANGELOG.md b/crates/tui/CHANGELOG.md index dcc56ff648..9f966b42f0 100644 --- a/crates/tui/CHANGELOG.md +++ b/crates/tui/CHANGELOG.md @@ -51,6 +51,12 @@ quieter, and Fleet runs can be checked before they spend anything. ### Fixed +- TUI `/undo` now restores only the files the undone tool call or turn + changed, and refuses, changing nothing, when one of them changed since. + Before, it checked out the whole snapshot tree, which also reverted later + edits to other files. It also finds restore points older than the newest + 100 snapshots, and a forked session can undo the turns it inherited + ([#6644](https://github.com/Hmbown/Codewhale/issues/6644)). - A top-level `base_url` or `api_key` in `config.toml` now means one thing everywhere. Every reader used its own rule for which routes inherited it, which is how a DeepSeek endpoint became the Xiaomi MiMo route's and failed diff --git a/crates/tui/src/commands/groups/debug/tests.rs b/crates/tui/src/commands/groups/debug/tests.rs index b346bb6139..4ea13c914d 100644 --- a/crates/tui/src/commands/groups/debug/tests.rs +++ b/crates/tui/src/commands/groups/debug/tests.rs @@ -2170,3 +2170,215 @@ fn test_undo_reports_that_files_were_not_reverted_when_the_repo_is_unavailable() "the reason must travel with the fallback: {message}" ); } + +/// Isolated HOME + workspace for the `/undo` restore tests (#6644). +// Fields drop in order: the env guards restore before the lock releases. +struct UndoFixture { + workspace: PathBuf, + repo: crate::snapshot::SnapshotRepo, + _tmp: tempfile::TempDir, + _codewhale_home: crate::test_support::EnvVarGuard, + _profile: crate::test_support::EnvVarGuard, + _home: crate::test_support::EnvVarGuard, + _lock: crate::test_support::TestEnvLock, +} + +impl UndoFixture { + fn new() -> Self { + use crate::test_support::{EnvVarGuard, lock_test_env}; + let lock = lock_test_env(); + let tmp = tempfile::tempdir().unwrap(); + let home = EnvVarGuard::set("HOME", tmp.path()); + let profile = EnvVarGuard::set("USERPROFILE", tmp.path()); + let codewhale_home = EnvVarGuard::remove("CODEWHALE_HOME"); + let workspace = tmp.path().join("ws"); + std::fs::create_dir_all(&workspace).unwrap(); + let repo = crate::snapshot::SnapshotRepo::open_or_init(&workspace).unwrap(); + Self { + workspace, + repo, + _tmp: tmp, + _codewhale_home: codewhale_home, + _profile: profile, + _home: home, + _lock: lock, + } + } + + fn write(&self, name: &str, body: &str) { + std::fs::write(self.workspace.join(name), body).unwrap(); + } + + fn read(&self, name: &str) -> String { + std::fs::read_to_string(self.workspace.join(name)).unwrap() + } + + fn snapshot(&self, label: &str, session: &str) { + self.repo.take_snapshot(label, Some(session)).unwrap(); + } + + fn app(&self, session: &str) -> App { + let mut app = create_test_app(); + app.workspace = self.workspace.clone(); + app.yolo = true; + app.current_session_id = Some(session.to_string()); + app + } +} + +/// `/undo` restores only the paths the undone turn changed: an edit the user +/// made to another file after the turn survives. The whole-tree restore +/// reverted it too. +#[test] +fn patch_undo_restores_only_the_paths_the_undone_step_changed() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.write("b.txt", "b0"); + fx.snapshot("pre-turn:1", "s1"); + fx.write("a.txt", "a1"); + fx.write("new.txt", "created by the turn"); + fx.snapshot("post-turn:1", "s1"); + // The user's own edit after the turn, and a file they created. + fx.write("b.txt", "b-user"); + fx.write("mine.txt", "user file"); + + let mut app = fx.app("s1"); + let result = patch_undo(&mut app); + + assert!(!result.is_error, "{:?}", result.message); + assert_eq!(fx.read("a.txt"), "a0"); + assert!(!fx.workspace.join("new.txt").exists()); + assert_eq!(fx.read("b.txt"), "b-user"); + assert_eq!(fx.read("mine.txt"), "user file"); + let message = result.message.unwrap_or_default(); + assert!( + message.contains("modified a.txt") && message.contains("removed new.txt"), + "{message}" + ); +} + +/// A path the undone step changed that changed again since is refused, not +/// overwritten, and nothing else is touched. +#[test] +fn patch_undo_refuses_when_a_changed_path_changed_since() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.write("b.txt", "b0"); + fx.snapshot("pre-turn:1", "s1"); + fx.write("a.txt", "a1"); + fx.write("b.txt", "b1"); + fx.snapshot("post-turn:1", "s1"); + fx.write("a.txt", "a-user"); + + let mut app = fx.app("s1"); + let result = super::dispatch(&mut app, "undo", None).expect("registered command"); + + let message = result.message.unwrap_or_default(); + assert!( + message.contains("Refusing to undo snapshot") && message.contains("a.txt"), + "{message}" + ); + assert_eq!(fx.read("a.txt"), "a-user"); + assert_eq!(fx.read("b.txt"), "b1"); +} + +/// `/undo` keeps stepping back one tool call at a time (#384), each step +/// restoring only what that call changed. +#[test] +fn patch_undo_steps_back_one_tool_call_at_a_time() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.snapshot("pre-turn:1", "s1"); + fx.snapshot("tool:call-1", "s1"); + fx.write("a.txt", "a1"); + fx.snapshot("tool:call-2", "s1"); + fx.write("a.txt", "a2"); + fx.write("b.txt", "b2"); + fx.snapshot("post-turn:1", "s1"); + + let mut app = fx.app("s1"); + let first = patch_undo(&mut app); + assert!(!first.is_error, "{:?}", first.message); + assert_eq!(fx.read("a.txt"), "a1"); + assert!(!fx.workspace.join("b.txt").exists()); + + let second = patch_undo(&mut app); + assert!(!second.is_error, "{:?}", second.message); + assert_eq!(fx.read("a.txt"), "a0"); + + let third = patch_undo(&mut app); + assert!( + third + .message + .as_deref() + .is_some_and(|m| m.starts_with("No undoable snapshot")), + "{:?}", + third.message + ); +} + +/// Restore points older than the newest 100 snapshots are still found. +#[test] +fn patch_undo_finds_restore_points_beyond_the_newest_hundred_snapshots() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.snapshot("pre-turn:1", "s1"); + fx.write("a.txt", "a1"); + fx.snapshot("post-turn:1", "s1"); + for i in 0..101 { + fx.repo + .take_snapshot(&format!("tool:other-{i}"), Some("other-session")) + .unwrap(); + } + + let mut app = fx.app("s1"); + let result = patch_undo(&mut app); + + assert!(!result.is_error, "{:?}", result.message); + assert_eq!(fx.read("a.txt"), "a0", "{:?}", result.message); +} + +/// A fork owns the restore points of the turns it inherited, up to the fork, +/// and none its source took afterwards. +#[test] +fn patch_undo_restores_turns_a_fork_inherited() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.snapshot("pre-turn:1", "source"); + fx.write("a.txt", "a1"); + fx.snapshot("post-turn:1", "source"); + + let fork = |created_at: chrono::DateTime| { + let mut app = fx.app("fork"); + let mut metadata = + crate::session_manager::create_saved_session(&[], "model", &fx.workspace, 0, None) + .metadata; + metadata.id = "fork".to_string(); + metadata.parent_session_id = Some("source".to_string()); + metadata.created_at = created_at; + app.current_session_metadata = Some(metadata); + app + }; + + let owners = super::undo::snapshot_owners(&fork(chrono::Utc::now())); + assert_eq!(owners.len(), 2); + assert_eq!(owners[1].session_id, "source"); + + // Forked before the source took these snapshots: they are not the fork's. + let mut early = fork(chrono::Utc::now() - chrono::Duration::hours(1)); + let refused = patch_undo(&mut early); + assert!( + refused + .message + .as_deref() + .is_some_and(|m| m.starts_with("No undoable snapshot")), + "{:?}", + refused.message + ); + assert_eq!(fx.read("a.txt"), "a1"); + + let mut app = fork(chrono::Utc::now() + chrono::Duration::seconds(5)); + let result = patch_undo(&mut app); + assert!(!result.is_error, "{:?}", result.message); + assert_eq!(fx.read("a.txt"), "a0", "{:?}", result.message); +} diff --git a/crates/tui/src/commands/groups/debug/undo.rs b/crates/tui/src/commands/groups/debug/undo.rs index 5419b3f771..e9aac80ba7 100644 --- a/crates/tui/src/commands/groups/debug/undo.rs +++ b/crates/tui/src/commands/groups/debug/undo.rs @@ -4,6 +4,7 @@ use crate::dependencies::{ExternalTool, Git}; use crate::tui::app::{App, AppAction}; use crate::tui::history::HistoryCell; use codewhale_models::ContentBlock; +use std::path::PathBuf; use super::CommandResult; @@ -146,12 +147,243 @@ fn tool_result_id(block: &ContentBlock) -> Option<&String> { } } +/// Deepest fork chain [`snapshot_owners`] follows. A chain this long is +/// already unusual; the bound only stops a corrupt lineage from looping. +const MAX_FORK_ANCESTORS: usize = 32; + +/// A session whose restore points this conversation owns. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(in crate::commands) struct SnapshotOwner { + /// Session tag the snapshots carry. + pub(in crate::commands) session_id: String, + /// Newest snapshot time (Unix seconds) owned from this session: `None` + /// for the current session, the fork time for a session it was forked + /// from. The source keeps working after the fork, and its later + /// snapshots are not the fork's. + pub(in crate::commands) until: Option, +} + +impl SnapshotOwner { + fn owns(&self, snapshot: &crate::snapshot::Snapshot) -> bool { + snapshot.session_id.as_deref() == Some(self.session_id.as_str()) + && self.until.is_none_or(|until| snapshot.timestamp <= until) + } +} + +/// The sessions whose restore points `/undo` may use: the current session, +/// and for a fork each session it was forked from, up to the fork. A fork +/// copies its source's turns, so the snapshots those turns took (tagged +/// with the source's id) are the fork's too, as the Runtime's thread-owned +/// restore points are (#6621). +/// +/// Lineage the saved sessions cannot prove ends the chain: fewer owners +/// means fewer restorable steps, never someone else's. +pub(in crate::commands) fn snapshot_owners(app: &App) -> Vec { + let Some(current) = app.current_session_id.clone() else { + return Vec::new(); + }; + let manager = crate::session_manager::SessionManager::default_location().ok(); + let load = |id: &str| { + manager + .as_ref() + .and_then(|manager| manager.load_session_metadata_by_id(id).ok()) + }; + let mut metadata = app + .current_session_metadata + .clone() + .filter(|metadata| metadata.id == current) + .or_else(|| load(¤t)); + let mut owners = vec![SnapshotOwner { + session_id: current, + until: None, + }]; + while let Some(child) = metadata.take() { + let Some(parent) = child.parent_session_id.clone() else { + break; + }; + if owners.len() > MAX_FORK_ANCESTORS + || owners.iter().any(|owner| owner.session_id == parent) + { + break; + } + let forked_at = child.created_at.timestamp(); + let until = owners + .last() + .and_then(|owner| owner.until) + .map_or(forked_at, |child_until| child_until.min(forked_at)); + metadata = load(&parent); + owners.push(SnapshotOwner { + session_id: parent, + until: Some(until), + }); + } + owners +} + +/// Labels a `/undo` step starts at: before one tool call, or before a turn. +fn is_undo_step_label(label: &str) -> bool { + label.starts_with("tool:") || label.starts_with("pre-turn:") +} + +/// Labels of the restore points an engine takes for a turn. A step runs from +/// one of them to the next one the conversation owns. +fn is_restore_point_label(label: &str) -> bool { + is_undo_step_label(label) || label.starts_with("post-tool:") || label.starts_with("post-turn:") +} + +/// One `/undo` step, planned but not applied. +pub(in crate::commands) struct UndoStep { + /// Restore point the step started at. + pub(in crate::commands) target: crate::snapshot::Snapshot, + /// Tree the step ended at: the next restore point this conversation + /// owns, or, for the newest step, a snapshot of the workspace as it is + /// now. Trees, not commit ids, because a prune rewrites commit ids. + pub(in crate::commands) end: crate::snapshot::SnapshotId, + /// The paths the step changed that are still as it left them. + pub(in crate::commands) restore: Vec, +} + +/// Why no step can be undone. +pub(in crate::commands) enum UndoRefusal { + /// Nothing this conversation owns is left to undo. + Nothing(String), + /// A step is there, but undoing it would clobber later work, or the + /// snapshot repo failed; nothing was changed. + Refused(Box), +} + +/// Find the newest step of `snapshots` (newest first) that `owners` own and +/// that is not undone yet, and the paths undoing it restores. +/// +/// A step is scoped to the paths that changed between its restore point and +/// the next one: edits to any other file (the user's, another session's) +/// are never touched. A path the step changed that changed again since is +/// refused rather than overwritten. A step whose paths are all back at its +/// restore point is already undone, so `/undo` walks back one tool call (or +/// turn) at a time (#384). +/// +/// Known limits: the TUI records no per-tool receipts (the Runtime's +/// `post-tool:` spans and declared write paths), so a step owns everything +/// that changed between its restore point and the next one this +/// conversation owns, including a write another session made in that window. +/// The newest step, when no later restore point exists yet, ends at the +/// workspace as it is now. +pub(in crate::commands) fn plan_undo_step( + repo: &crate::snapshot::SnapshotRepo, + snapshots: Vec, + owners: &[SnapshotOwner], +) -> Result { + let owned: Vec = snapshots + .into_iter() + .filter(|snapshot| is_restore_point_label(&snapshot.label)) + .filter(|snapshot| owners.iter().any(|owner| owner.owns(snapshot))) + .collect(); + if !owned + .iter() + .any(|snapshot| is_undo_step_label(&snapshot.label)) + { + return Err(UndoRefusal::Nothing( + "No undoable snapshots for the current session — nothing to revert.".to_string(), + )); + } + + let compare_failed = |error: std::io::Error| { + UndoRefusal::Refused(Box::new( + if error.kind() == std::io::ErrorKind::InvalidInput { + CommandResult::message(format!( + "A path the undone step changed cannot be restored file by file: {error}. \ + Nothing was changed; use /restore for a whole-workspace rollback." + )) + } else { + CommandResult::error(format!("Failed to compare snapshot: {error}")) + }, + )) + }; + + for (index, target) in owned.iter().enumerate() { + if !is_undo_step_label(&target.label) { + continue; + } + let end = match index.checked_sub(1) { + Some(newer) => owned[newer].tree.clone(), + // The newest step has no later restore point (the turn is still + // running, stopped early, or its post-turn snapshot has not + // landed): the workspace now is the only record of its end. + None => { + if repo + .work_tree_matches_snapshot(&target.tree) + .map_err(compare_failed)? + { + continue; + } + let short = &target.id.as_str()[..target.id.as_str().len().min(12)]; + repo.take_snapshot(&format!("pre-restore:{short}"), None) + .map_err(|error| { + UndoRefusal::Refused(Box::new(CommandResult::error(format!( + "Failed to snapshot the workspace before undo: {error}" + )))) + })? + .tree + } + }; + let changed = repo + .changed_paths_between(&target.tree, &end) + .map_err(compare_failed)?; + let mut restore = Vec::new(); + let mut changed_since = Vec::new(); + 'paths: for path in changed { + if repo + .path_matches_snapshot(&end, &path) + .map_err(compare_failed)? + { + restore.push(path); + continue; + } + // Back at the step's start, or at an older restore point that an + // earlier `/undo` walked it back to: already undone. + for older in &owned[index..] { + if repo + .path_matches_snapshot(&older.tree, &path) + .map_err(compare_failed)? + { + continue 'paths; + } + } + changed_since.push(path.display().to_string()); + } + if !changed_since.is_empty() { + return Err(UndoRefusal::Refused(Box::new(CommandResult::message( + format!( + "Refusing to undo snapshot '{}': {} changed after it, and undoing would overwrite \ + that change. Nothing was changed; revert those files yourself, or use /restore \ + for a whole-workspace rollback.", + target.label, + changed_since.join(", ") + ), + )))); + } + if restore.is_empty() { + // Already undone, or the step changed nothing: keep walking back. + continue; + } + return Ok(UndoStep { + target: target.clone(), + end, + restore, + }); + } + Err(UndoRefusal::Nothing( + "No undoable snapshot differs from the current workspace — nothing to revert.".to_string(), + )) +} + /// Revert the most recent write tool (apply_patch/edit_file/write_file) or turn. /// -/// Opens the side-git snapshot repo and finds the most recent snapshot, -/// preferring per-tool snapshots (`tool:*`) over pre-turn snapshots -/// (`pre-turn:*`). Restores files from that snapshot and shows a diff -/// summary. Falls back to conversation undo when no snapshots exist. +/// Opens the side-git snapshot repo and finds the newest `tool:*` or +/// `pre-turn:*` restore point this conversation owns (see +/// [`snapshot_owners`]) whose step is not undone yet, then restores only the +/// files that step changed (see [`plan_undo_step`]). Falls back to +/// conversation undo when no snapshots exist. /// /// Posts a `HistoryCell::System` entry so the user can see what was /// reverted in the transcript. @@ -168,7 +400,9 @@ pub fn patch_undo(app: &mut App) -> CommandResult { } }; - let snapshots = match repo.list(100) { + // The whole store: an older restore point that is still stored must not + // be mistaken for a pruned one. + let snapshots = match repo.list(usize::MAX) { Ok(s) => s, Err(e) => { return CommandResult::error(format!("Failed to list snapshots: {e}")); @@ -183,49 +417,23 @@ pub fn patch_undo(app: &mut App) -> CommandResult { // Untagged legacy snapshots and snapshots from another conversation may // describe unrelated user work in this same workspace, so fail closed // and let the command dispatcher fall back to conversation-only undo. - let Some(current_session) = app.current_session_id.as_deref() else { + let owners = snapshot_owners(app); + if owners.is_empty() { return CommandResult::message( "No undoable snapshot is tagged for the current session — nothing to revert.", ); - }; - let candidates: Vec = snapshots - .into_iter() - .filter(|s| s.label.starts_with("tool:") || s.label.starts_with("pre-turn:")) - .filter(|s| s.session_id.as_deref() == Some(current_session)) - .collect(); - - if candidates.is_empty() { - return CommandResult::message( - "No undoable snapshots for the current session — nothing to revert.", - ); - } - - // Pick the newest current-session candidate whose tree differs from the - // workspace. Skipping identical snapshots makes repeated `/undo` walk - // backward only inside the proven session boundary. - let mut target = None; - for snapshot in &candidates { - match repo.work_tree_matches_snapshot(&snapshot.id) { - Ok(false) => { - target = Some(snapshot); - break; - } - Ok(true) => {} - Err(error) => { - return CommandResult::error(format!("Failed to compare snapshot: {error}")); - } - } } - let Some(target) = target else { - return CommandResult::message( - "No undoable snapshot differs from the current workspace — nothing to revert.", - ); + let step = match plan_undo_step(&repo, snapshots, &owners) { + Ok(step) => step, + Err(UndoRefusal::Nothing(message)) => return CommandResult::message(message), + Err(UndoRefusal::Refused(result)) => return *result, }; + let target = &step.target; // Restoring workspace files is a mutation. Apply the trust gate only - // after finding a real, current-session target so chat-only `/undo` can - // still fall back to conversation history in ordinary mode. + // after finding a real, owned step so chat-only `/undo` can still fall + // back to conversation history in ordinary mode. if !(app.yolo || app.trust_mode) { return CommandResult::message( "Refusing to undo workspace files outside trusted mode.\n\ @@ -233,22 +441,35 @@ pub fn patch_undo(app: &mut App) -> CommandResult { ); } - // Capture what this restore is about to change *before* it runs: after the - // checkout the work tree matches the snapshot and the diff is empty by - // construction. Computed in the side repo, not the user's — the user's - // `git diff --stat` reports their own uncommitted work, which is not what - // the undo changed. - let diff_stat = match repo.snapshot_diff_stat(&target.id) { - Ok(stat) => stat, - Err(e) => { - tracing::warn!(target: "snapshot", "diff stat for the undo summary failed: {e}"); - None - } - }; - - if let Err(e) = repo.restore(&target.id) { - return CommandResult::error(format!("Restore failed: {e}")); - } + let plan: Vec<(PathBuf, crate::snapshot::SnapshotId)> = step + .restore + .iter() + .map(|path| (path.clone(), target.tree.clone())) + .collect(); + let backup_short = &target.id.as_str()[..target.id.as_str().len().min(12)]; + let outcomes = + match repo.restore_path_plan(&plan, &format!("pre-restore:{backup_short}"), true, || { + // Re-verify after the safety snapshot, immediately before the + // first write: a change that landed meanwhile is refused. + for path in &step.restore { + if !repo.path_matches_snapshot(&step.end, path)? { + return Err(std::io::Error::new( + std::io::ErrorKind::WouldBlock, + format!( + "'{}' changed while the undo was being prepared; nothing was changed.", + path.display() + ), + )); + } + } + Ok(()) + }) { + Ok(outcomes) => outcomes, + Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + return CommandResult::message(e.to_string()); + } + Err(e) => return CommandResult::error(format!("Restore failed: {e}")), + }; if let Some(tool_id) = target.label.strip_prefix("tool:") { prune_undone_tool_context(app, tool_id); @@ -257,25 +478,22 @@ pub fn patch_undo(app: &mut App) -> CommandResult { } let short = &target.id.as_str()[..target.id.as_str().len().min(8)]; - let summary = match diff_stat { - Some(ref stat) => { - format!( - "Restored snapshot '{}' ({}). Files affected:\n{stat}", - target.label, short - ) - } - None => { - format!( - "Restored snapshot '{}' ({}). No diff changes detected.", - target.label, short - ) - } - }; + let lines: Vec = outcomes + .iter() + .map(|outcome| format!("{} {}", outcome.action.as_str(), outcome.path.display())) + .collect(); + let summary = format!( + "Restored {} file(s) to snapshot '{}' ({}):\n{}", + outcomes.len(), + target.label, + short, + lines.join("\n") + ); // Post a system cell so the reverted state is visible in the transcript. app.push_history_cell(HistoryCell::System { content: format!( - "/undo reverted workspace to snapshot '{}' ({})", + "/undo reverted workspace files to snapshot '{}' ({})", target.label, short ), }); diff --git a/crates/tui/src/session_manager.rs b/crates/tui/src/session_manager.rs index 79cf086c12..cdaf570f19 100644 --- a/crates/tui/src/session_manager.rs +++ b/crates/tui/src/session_manager.rs @@ -1288,6 +1288,11 @@ impl SessionManager { Ok(trimmed) } + /// Metadata of saved session `id`, read without loading its transcript. + pub fn load_session_metadata_by_id(&self, id: &str) -> std::io::Result { + Self::load_session_metadata(&self.validated_session_path(id)?) + } + fn validated_session_path(&self, id: &str) -> std::io::Result { let trimmed = self.validated_session_id(id)?; Ok(self.sessions_dir.join(format!("{trimmed}.json"))) diff --git a/crates/tui/src/snapshot/repo.rs b/crates/tui/src/snapshot/repo.rs index 75f96f0832..cc4f71bc21 100644 --- a/crates/tui/src/snapshot/repo.rs +++ b/crates/tui/src/snapshot/repo.rs @@ -1203,41 +1203,6 @@ impl SnapshotRepo { Ok(outcomes) } - /// `git diff --stat` between snapshot `id` and the current working tree, - /// computed inside the side repo. - /// - /// This is what restoring `id` *would* change, so it must be captured - /// before the restore runs — afterwards the work tree matches the snapshot - /// and the diff is empty by construction. - /// - /// It deliberately runs against the side repo rather than the user's. The - /// previous summary ran `git diff --stat` in the workspace with the user's - /// `.git`, which reports the user's own uncommitted work: it listed files - /// the restore had not touched, and reported nothing when that work - /// happened to be committed. Returns `None` when nothing differs. - pub fn snapshot_diff_stat(&self, id: &SnapshotId) -> io::Result> { - let diff = run_git( - &self.git_dir, - &self.work_tree, - &[ - "diff", - "--stat", - "--end-of-options", - id.as_str(), - "--", - ":/", - ], - )?; - if !diff.status.success() { - return Err(io_other(format!( - "git diff --stat failed: {}", - String::from_utf8_lossy(&diff.stderr).trim() - ))); - } - let stat = String::from_utf8_lossy(&diff.stdout).trim().to_string(); - Ok((!stat.is_empty()).then_some(stat)) - } - /// Return whether the current workspace matches the given snapshot's /// tracked file content. /// @@ -2200,34 +2165,6 @@ mod tests { ); } - #[test] - fn snapshot_diff_stat_describes_what_a_restore_would_change() { - let tmp = tempdir().unwrap(); - let (repo, _home) = make_repo(tmp.path()); - let changed = repo.work_tree().join("changed.txt"); - let untouched = repo.work_tree().join("untouched.txt"); - - std::fs::write(&changed, b"v1").unwrap(); - std::fs::write(&untouched, b"stable").unwrap(); - let id = repo.snapshot("pre-turn:1").expect("snapshot"); - - std::fs::write(&changed, b"v2").unwrap(); - - let stat = repo - .snapshot_diff_stat(&id) - .expect("diff stat") - .expect("the snapshot differs, so something must be reported"); - assert!(stat.contains("changed.txt"), "got: {stat}"); - // The stat describes the restore's effect, not the workspace's whole - // uncommitted state — a file the restore will not touch must not appear. - assert!(!stat.contains("untouched.txt"), "got: {stat}"); - - // After restoring, the two sides agree: nothing left to report. (Which - // is why the caller must capture this *before* the restore runs.) - repo.restore(&id).expect("restore"); - assert_eq!(repo.snapshot_diff_stat(&id).expect("diff stat"), None); - } - #[test] fn restore_paths_removes_a_file_created_after_the_snapshot() { let tmp = tempdir().unwrap(); From 29faeaa8f08876ddb3d36f3e2d89f9881dc2a5f2 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Sun, 27 Sep 2026 02:00:13 -0700 Subject: [PATCH 2/2] fix(tui): /undo waits for the post-turn snapshot and skips non-regular paths (#6644) Follow-up to the path-scoped /undo, from review findings: - Post-turn race. The TUI takes its post-turn snapshot fire-and-forget after TurnComplete (#234), so an immediate /undo planned the newest step against the live workspace. Both then ran `git add -A` on the side repo's index, and the late post-turn snapshot recorded the reverted workspace as the turn's end. The engine now reserves the snapshot before TurnComplete (`snapshot::PendingPostTurnSnapshot`), and /undo waits up to 10s for it. If it is still pending, /undo changes nothing and says to retry. - Non-regular paths. A symlink, directory or submodule change made path comparison return InvalidInput, which refused the whole /undo and every older step, with no conversation fallback. These paths are now left in place and reported ("Left in place ..."). Regular paths are still restored, and older steps stay reachable. - Writes while refusing. Planning the newest step snapshotted the workspace before the trust gate, so an untrusted /undo still wrote a commit and could trigger a size-pressure prune. Planning now refuses before that snapshot outside trusted mode. The planning snapshot is reused as the restore's safety backup (`SnapshotRepo::restore_path_plan_with_backup`), so no second snapshot or prune runs between planning and checkout. - revert_turn remains a whole-tree rollback. Its module doc and tool description now state that it overwrites later edits. Routing it through the scoped plan is not part of this change. Tests: three new regression tests in commands/groups/debug/tests.rs. With each fix neutralized: test result: FAILED. 0 passed; 3 failed. cargo test -p codewhale-tui --lib -- snapshot:: commands::groups::debug::tests tools::revert_turn: test result: ok. 149 passed; 0 failed. cargo clippy -p codewhale-tui --lib --tests -D warnings (CI allows): clean. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 +- crates/tui/CHANGELOG.md | 6 +- crates/tui/src/commands/groups/debug/tests.rs | 108 +++++++++ crates/tui/src/commands/groups/debug/undo.rs | 209 ++++++++++++------ crates/tui/src/core/engine.rs | 44 +++- crates/tui/src/snapshot/mod.rs | 54 +++++ crates/tui/src/snapshot/repo.rs | 47 +++- crates/tui/src/tools/revert_turn.rs | 9 +- 8 files changed, 408 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08c1719da7..e7bc2752f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,7 +55,11 @@ quieter, and Fleet runs can be checked before they spend anything. changed, and refuses, changing nothing, when one of them changed since. Before, it checked out the whole snapshot tree, which also reverted later edits to other files. It also finds restore points older than the newest - 100 snapshots, and a forked session can undo the turns it inherited + 100 snapshots, and a forked session can undo the turns it inherited. It + waits for a post-turn snapshot that is still being written, leaves a + changed symlink or other non-regular path in place (and says so) instead + of refusing the whole undo, and writes nothing to the snapshot store when + it refuses outside trusted mode ([#6644](https://github.com/Hmbown/Codewhale/issues/6644)). - A top-level `base_url` or `api_key` in `config.toml` now means one thing everywhere. Every reader used its own rule for which routes inherited it, diff --git a/crates/tui/CHANGELOG.md b/crates/tui/CHANGELOG.md index 9f966b42f0..70387f4d11 100644 --- a/crates/tui/CHANGELOG.md +++ b/crates/tui/CHANGELOG.md @@ -55,7 +55,11 @@ quieter, and Fleet runs can be checked before they spend anything. changed, and refuses, changing nothing, when one of them changed since. Before, it checked out the whole snapshot tree, which also reverted later edits to other files. It also finds restore points older than the newest - 100 snapshots, and a forked session can undo the turns it inherited + 100 snapshots, and a forked session can undo the turns it inherited. It + waits for a post-turn snapshot that is still being written, leaves a + changed symlink or other non-regular path in place (and says so) instead + of refusing the whole undo, and writes nothing to the snapshot store when + it refuses outside trusted mode ([#6644](https://github.com/Hmbown/Codewhale/issues/6644)). - A top-level `base_url` or `api_key` in `config.toml` now means one thing everywhere. Every reader used its own rule for which routes inherited it, diff --git a/crates/tui/src/commands/groups/debug/tests.rs b/crates/tui/src/commands/groups/debug/tests.rs index 4ea13c914d..9987830cea 100644 --- a/crates/tui/src/commands/groups/debug/tests.rs +++ b/crates/tui/src/commands/groups/debug/tests.rs @@ -2382,3 +2382,111 @@ fn patch_undo_restores_turns_a_fork_inherited() { assert!(!result.is_error, "{:?}", result.message); assert_eq!(fx.read("a.txt"), "a0", "{:?}", result.message); } + +/// `/undo` typed while the post-turn snapshot is still being written waits +/// for it (#6644). Before, the two raced on the side repo: the post-turn +/// snapshot landed after the undo and recorded the reverted workspace as +/// the turn's end, and the undone step ended at "now". +#[test] +fn patch_undo_waits_for_a_pending_post_turn_snapshot() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.snapshot("pre-turn:1", "s1"); + fx.snapshot("tool:call-1", "s1"); + fx.write("a.txt", "a1"); + + // The turn has completed; its post-turn snapshot is still in flight. + let pending = crate::snapshot::PendingPostTurnSnapshot::reserve(); + let writer = { + let repo = crate::snapshot::SnapshotRepo::open_or_init(&fx.workspace).unwrap(); + std::thread::spawn(move || { + std::thread::sleep(std::time::Duration::from_millis(500)); + let taken = repo.take_snapshot("post-turn:1", Some("s1")).unwrap(); + drop(pending); + taken + }) + }; + + let mut app = fx.app("s1"); + let result = patch_undo(&mut app); + let post_turn = writer.join().unwrap(); + + assert!(!result.is_error, "{:?}", result.message); + assert_eq!(fx.read("a.txt"), "a0", "{:?}", result.message); + let tool = fx + .repo + .list(usize::MAX) + .unwrap() + .into_iter() + .find(|snapshot| snapshot.label == "tool:call-1") + .unwrap(); + assert_eq!( + fx.repo + .changed_paths_between(&tool.tree, &post_turn.tree) + .unwrap(), + vec![PathBuf::from("a.txt")], + "the post-turn snapshot must record the turn's end, not the undone workspace" + ); +} + +/// A step that changed a symlink restores its regular files and reports the +/// symlink, and it does not block older steps. +#[cfg(unix)] +#[test] +fn patch_undo_skips_non_regular_paths_without_blocking_older_steps() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.snapshot("pre-turn:1", "s1"); + fx.write("a.txt", "a1"); + fx.snapshot("pre-turn:2", "s1"); + fx.write("a.txt", "a2"); + std::os::unix::fs::symlink("a.txt", fx.workspace.join("current")).unwrap(); + fx.snapshot("post-turn:2", "s1"); + + let mut app = fx.app("s1"); + let first = patch_undo(&mut app); + assert!(!first.is_error, "{:?}", first.message); + assert_eq!(fx.read("a.txt"), "a1"); + let message = first.message.unwrap_or_default(); + assert!( + message.contains("Left in place") && message.contains("current"), + "{message}" + ); + assert!( + std::fs::symlink_metadata(fx.workspace.join("current")) + .unwrap() + .file_type() + .is_symlink() + ); + + let second = patch_undo(&mut app); + assert!(!second.is_error, "{:?}", second.message); + assert_eq!(fx.read("a.txt"), "a0", "{:?}", second.message); +} + +/// Outside trusted mode `/undo` refuses before writing anything: planning +/// the newest step does not add a snapshot to the side repo. +#[test] +fn patch_undo_outside_trusted_mode_writes_no_snapshot() { + let fx = UndoFixture::new(); + fx.write("a.txt", "a0"); + fx.snapshot("pre-turn:1", "s1"); + fx.write("a.txt", "a1"); + let before = fx.repo.list(usize::MAX).unwrap().len(); + + let mut app = fx.app("s1"); + app.yolo = false; + app.trust_mode = false; + let result = patch_undo(&mut app); + + assert!( + result + .message + .as_deref() + .is_some_and(|m| m.starts_with("Refusing to undo workspace files outside trusted mode")), + "{:?}", + result.message + ); + assert_eq!(fx.repo.list(usize::MAX).unwrap().len(), before); + assert_eq!(fx.read("a.txt"), "a1"); +} diff --git a/crates/tui/src/commands/groups/debug/undo.rs b/crates/tui/src/commands/groups/debug/undo.rs index e9aac80ba7..8d0501d96b 100644 --- a/crates/tui/src/commands/groups/debug/undo.rs +++ b/crates/tui/src/commands/groups/debug/undo.rs @@ -241,6 +241,13 @@ pub(in crate::commands) struct UndoStep { pub(in crate::commands) end: crate::snapshot::SnapshotId, /// The paths the step changed that are still as it left them. pub(in crate::commands) restore: Vec, + /// Changed paths `/undo` leaves in place because they are not regular + /// files (a symlink, a directory, a submodule), in this step or in a + /// newer one it walked past. + pub(in crate::commands) skipped: Vec, + /// The `pre-restore:` snapshot planning took of the workspace, when the + /// step ends now; the restore reuses it as its safety backup. + pub(in crate::commands) backup: Option, } /// Why no step can be undone. @@ -260,18 +267,28 @@ pub(in crate::commands) enum UndoRefusal { /// are never touched. A path the step changed that changed again since is /// refused rather than overwritten. A step whose paths are all back at its /// restore point is already undone, so `/undo` walks back one tool call (or -/// turn) at a time (#384). +/// turn) at a time (#384). A changed path that is not a regular file is +/// left in place and reported (file-scoped restore never writes symlinks or +/// directories); it does not block the step's other paths or older steps. +/// +/// Planning writes nothing, except when the newest step ends now: the +/// workspace is then snapshotted, and only when `trusted`, since `/undo` +/// outside trusted mode refuses to touch files anyway. /// /// Known limits: the TUI records no per-tool receipts (the Runtime's /// `post-tool:` spans and declared write paths), so a step owns everything /// that changed between its restore point and the next one this /// conversation owns, including a write another session made in that window. -/// The newest step, when no later restore point exists yet, ends at the -/// workspace as it is now. +/// The newest step, when no later restore point exists yet (the turn is still +/// running, or its post-turn snapshot failed), ends at the workspace as it +/// is now, so an edit made since the step's restore point counts as the +/// step's. [`patch_undo`] first waits for a post-turn snapshot this process +/// is still taking, so this is not the case right after a turn. pub(in crate::commands) fn plan_undo_step( repo: &crate::snapshot::SnapshotRepo, snapshots: Vec, owners: &[SnapshotOwner], + trusted: bool, ) -> Result { let owned: Vec = snapshots .into_iter() @@ -288,22 +305,20 @@ pub(in crate::commands) fn plan_undo_step( } let compare_failed = |error: std::io::Error| { - UndoRefusal::Refused(Box::new( - if error.kind() == std::io::ErrorKind::InvalidInput { - CommandResult::message(format!( - "A path the undone step changed cannot be restored file by file: {error}. \ - Nothing was changed; use /restore for a whole-workspace rollback." - )) - } else { - CommandResult::error(format!("Failed to compare snapshot: {error}")) - }, - )) + UndoRefusal::Refused(Box::new(CommandResult::error(format!( + "Failed to compare snapshot: {error}" + )))) }; + // `InvalidInput` from a path comparison: the path is not a regular file + // (or not a safe workspace path) on one side, so it is left alone. + let unrestorable = |error: &std::io::Error| error.kind() == std::io::ErrorKind::InvalidInput; + let mut skipped: Vec = Vec::new(); for (index, target) in owned.iter().enumerate() { if !is_undo_step_label(&target.label) { continue; } + let mut backup = None; let end = match index.checked_sub(1) { Some(newer) => owned[newer].tree.clone(), // The newest step has no later restore point (the turn is still @@ -316,14 +331,19 @@ pub(in crate::commands) fn plan_undo_step( { continue; } + if !trusted { + return Err(UndoRefusal::Refused(Box::new(untrusted_refusal()))); + } let short = &target.id.as_str()[..target.id.as_str().len().min(12)]; - repo.take_snapshot(&format!("pre-restore:{short}"), None) + let taken = repo + .take_snapshot(&format!("pre-restore:{short}"), None) .map_err(|error| { UndoRefusal::Refused(Box::new(CommandResult::error(format!( "Failed to snapshot the workspace before undo: {error}" )))) - })? - .tree + })?; + backup = Some(taken.id); + taken.tree } }; let changed = repo @@ -332,21 +352,38 @@ pub(in crate::commands) fn plan_undo_step( let mut restore = Vec::new(); let mut changed_since = Vec::new(); 'paths: for path in changed { - if repo - .path_matches_snapshot(&end, &path) - .map_err(compare_failed)? - { - restore.push(path); - continue; + match repo.path_matches_snapshot(&end, &path) { + // Still as the step left it. Comparing the step's start too + // proves it holds a regular file (or nothing) to restore. + Ok(true) => match repo.path_same_in_snapshots(&target.tree, &end, &path) { + Ok(_) => { + restore.push(path); + continue; + } + Err(error) if unrestorable(&error) => { + skipped.push(path); + continue; + } + Err(error) => return Err(compare_failed(error)), + }, + Ok(false) => {} + Err(error) if unrestorable(&error) => { + skipped.push(path); + continue; + } + Err(error) => return Err(compare_failed(error)), } // Back at the step's start, or at an older restore point that an // earlier `/undo` walked it back to: already undone. for older in &owned[index..] { - if repo - .path_matches_snapshot(&older.tree, &path) - .map_err(compare_failed)? - { - continue 'paths; + match repo.path_matches_snapshot(&older.tree, &path) { + Ok(true) => continue 'paths, + Ok(false) => {} + Err(error) if unrestorable(&error) => { + skipped.push(path); + continue 'paths; + } + Err(error) => return Err(compare_failed(error)), } } changed_since.push(path.display().to_string()); @@ -363,13 +400,18 @@ pub(in crate::commands) fn plan_undo_step( )))); } if restore.is_empty() { - // Already undone, or the step changed nothing: keep walking back. + // Already undone, changed nothing, or changed only paths `/undo` + // cannot restore: keep walking back. continue; } + skipped.sort(); + skipped.dedup(); return Ok(UndoStep { target: target.clone(), end, restore, + skipped, + backup, }); } Err(UndoRefusal::Nothing( @@ -377,6 +419,16 @@ pub(in crate::commands) fn plan_undo_step( )) } +/// How long `/undo` waits for a post-turn snapshot still being written. +const POST_TURN_SNAPSHOT_WAIT: std::time::Duration = std::time::Duration::from_secs(10); + +fn untrusted_refusal() -> CommandResult { + CommandResult::message( + "Refusing to undo workspace files outside trusted mode.\n\ + Run `/trust on` or select Full Access with Shift+Tab, then re-run `/undo`.", + ) +} + /// Revert the most recent write tool (apply_patch/edit_file/write_file) or turn. /// /// Opens the side-git snapshot repo and finds the newest `tool:*` or @@ -400,6 +452,16 @@ pub fn patch_undo(app: &mut App) -> CommandResult { } }; + // A post-turn snapshot this process is still taking is the newest step's + // end: without it, every edit since the step's restore point would count + // as the step's. + if !crate::snapshot::wait_for_pending_post_turn_snapshots(POST_TURN_SNAPSHOT_WAIT) { + return CommandResult::message( + "The last turn's workspace snapshot is still being written; nothing was changed. \ + Run /undo again in a moment.", + ); + } + // The whole store: an older restore point that is still stored must not // be mistaken for a pruned one. let snapshots = match repo.list(usize::MAX) { @@ -424,21 +486,19 @@ pub fn patch_undo(app: &mut App) -> CommandResult { ); } - let step = match plan_undo_step(&repo, snapshots, &owners) { + // Restoring workspace files is a mutation. Apply the trust gate only + // after finding a real, owned step so chat-only `/undo` can still fall + // back to conversation history in ordinary mode; planning itself writes + // nothing outside trusted mode. + let trusted = app.yolo || app.trust_mode; + let step = match plan_undo_step(&repo, snapshots, &owners, trusted) { Ok(step) => step, Err(UndoRefusal::Nothing(message)) => return CommandResult::message(message), Err(UndoRefusal::Refused(result)) => return *result, }; let target = &step.target; - - // Restoring workspace files is a mutation. Apply the trust gate only - // after finding a real, owned step so chat-only `/undo` can still fall - // back to conversation history in ordinary mode. - if !(app.yolo || app.trust_mode) { - return CommandResult::message( - "Refusing to undo workspace files outside trusted mode.\n\ - Run `/trust on` or select Full Access with Shift+Tab, then re-run `/undo`.", - ); + if !trusted { + return untrusted_refusal(); } let plan: Vec<(PathBuf, crate::snapshot::SnapshotId)> = step @@ -446,30 +506,43 @@ pub fn patch_undo(app: &mut App) -> CommandResult { .iter() .map(|path| (path.clone(), target.tree.clone())) .collect(); - let backup_short = &target.id.as_str()[..target.id.as_str().len().min(12)]; - let outcomes = - match repo.restore_path_plan(&plan, &format!("pre-restore:{backup_short}"), true, || { - // Re-verify after the safety snapshot, immediately before the - // first write: a change that landed meanwhile is refused. - for path in &step.restore { - if !repo.path_matches_snapshot(&step.end, path)? { - return Err(std::io::Error::new( - std::io::ErrorKind::WouldBlock, - format!( - "'{}' changed while the undo was being prepared; nothing was changed.", - path.display() - ), - )); - } + // Re-verify after the safety snapshot, immediately before the first + // write: a change that landed meanwhile is refused. + let preflight = || { + for path in &step.restore { + if !repo.path_matches_snapshot(&step.end, path)? { + return Err(std::io::Error::new( + std::io::ErrorKind::WouldBlock, + format!( + "'{}' changed while the undo was being prepared; nothing was changed.", + path.display() + ), + )); } - Ok(()) - }) { - Ok(outcomes) => outcomes, - Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { - return CommandResult::message(e.to_string()); - } - Err(e) => return CommandResult::error(format!("Restore failed: {e}")), - }; + } + Ok(()) + }; + let restored = match &step.backup { + // Planning already snapshotted the workspace (and `preflight` proves + // every planned path is still as that snapshot holds it). + Some(backup) => repo.restore_path_plan_with_backup(&plan, backup, true, preflight), + None => { + let backup_short = &target.id.as_str()[..target.id.as_str().len().min(12)]; + repo.restore_path_plan( + &plan, + &format!("pre-restore:{backup_short}"), + true, + preflight, + ) + } + }; + let outcomes = match restored { + Ok(outcomes) => outcomes, + Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + return CommandResult::message(e.to_string()); + } + Err(e) => return CommandResult::error(format!("Restore failed: {e}")), + }; if let Some(tool_id) = target.label.strip_prefix("tool:") { prune_undone_tool_context(app, tool_id); @@ -482,13 +555,25 @@ pub fn patch_undo(app: &mut App) -> CommandResult { .iter() .map(|outcome| format!("{} {}", outcome.action.as_str(), outcome.path.display())) .collect(); - let summary = format!( + let mut summary = format!( "Restored {} file(s) to snapshot '{}' ({}):\n{}", outcomes.len(), target.label, short, lines.join("\n") ); + if !step.skipped.is_empty() { + let skipped: Vec = step + .skipped + .iter() + .map(|path| path.display().to_string()) + .collect(); + summary.push_str(&format!( + "\nLeft in place (not a regular file, which /undo does not restore; use /restore \ + for a whole-workspace rollback): {}", + skipped.join(", ") + )); + } // Post a system cell so the reverted state is visible in the transcript. app.push_history_cell(HistoryCell::System { diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index c206576667..44c2f35d38 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -2165,6 +2165,7 @@ impl Engine { } self.post_turn_snapshot_before_complete(&snapshot_prompt) .await; + let pending_post_turn = self.reserve_post_turn_snapshot(); drop(turn_control); let _ = self .tx_event @@ -2179,7 +2180,11 @@ impl Engine { }) .await; - self.post_turn_snapshot_after_complete("post-shell-turn-snapshot", snapshot_prompt); + self.post_turn_snapshot_after_complete( + "post-shell-turn-snapshot", + snapshot_prompt, + pending_post_turn, + ); } /// Take one workspace snapshot for the running turn and report it as an @@ -2249,14 +2254,29 @@ impl Engine { .await; } - /// Without [`EngineConfig::record_restore_points`], take the post-turn - /// snapshot fire-and-forget: `TurnComplete` is already emitted, so the UI - /// is unblocked and the user can type / select / paste immediately - /// (#234). The git work proceeds on the blocking pool. - fn post_turn_snapshot_after_complete(&self, task: &'static str, prompt: String) { - if !self.config.snapshots_enabled || self.config.record_restore_points { + /// Without [`EngineConfig::record_restore_points`], reserve the post-turn + /// snapshot [`Self::post_turn_snapshot_after_complete`] takes. Called + /// before `TurnComplete`, so a `/undo` the user types as soon as the + /// turn ends waits for that snapshot instead of racing it (#6644). + fn reserve_post_turn_snapshot(&self) -> Option { + (self.config.snapshots_enabled && !self.config.record_restore_points) + .then(crate::snapshot::PendingPostTurnSnapshot::reserve) + } + + /// Take the post-turn snapshot reserved by + /// [`Self::reserve_post_turn_snapshot`] fire-and-forget: `TurnComplete` + /// is already emitted, so the UI is unblocked and the user can type / + /// select / paste immediately (#234). The git work proceeds on the + /// blocking pool, and the reservation is released once it is done. + fn post_turn_snapshot_after_complete( + &self, + task: &'static str, + prompt: String, + pending: Option, + ) { + let Some(pending) = pending else { return; - } + }; let post_workspace = self.session.workspace.clone(); let post_seq = self.turn_counter; let post_cap = self.config.snapshots_max_workspace_bytes; @@ -2269,6 +2289,7 @@ impl Engine { Some(&prompt), Some(&post_sid), ); + drop(pending); }); } @@ -5848,6 +5869,7 @@ impl Engine { } self.post_turn_snapshot_before_complete(&snapshot_prompt_post) .await; + let pending_post_turn = self.reserve_post_turn_snapshot(); drop(turn_control); // `event_sent` means the TurnComplete event reached the UI channel — // never that the user saw model output. (#6184: the old `delivered` @@ -5875,7 +5897,11 @@ impl Engine { // Post-turn snapshot, unless it was already taken before // TurnComplete (see `EngineConfig::record_restore_points`). - self.post_turn_snapshot_after_complete("post-turn-snapshot", snapshot_prompt_post); + self.post_turn_snapshot_after_complete( + "post-turn-snapshot", + snapshot_prompt_post, + pending_post_turn, + ); // ── Background advisor watcher (#3982) ──────────────────────────── // Fire-and-forget: TurnComplete is already emitted. The advisor diff --git a/crates/tui/src/snapshot/mod.rs b/crates/tui/src/snapshot/mod.rs index beff6229f7..c861759c0b 100644 --- a/crates/tui/src/snapshot/mod.rs +++ b/crates/tui/src/snapshot/mod.rs @@ -157,3 +157,57 @@ impl WorkspaceSnapshotRef { && snapshot.label.starts_with(self.kind.label_prefix()) } } + +/// Post-turn snapshots this process has promised but not written yet. +/// +/// An interactive engine takes its post-turn snapshot after `TurnComplete`, +/// off the input path (#234), so `/undo` typed right after a turn can run +/// before it lands. Without it the newest step would end at the workspace as +/// it is now, taking every edit made since the turn's last restore point for +/// the step's own, and both `git add -A` runs would race on the side repo's +/// index (#6644). The engine reserves the snapshot before `TurnComplete` and +/// `/undo` waits for it with [`wait_for_pending_post_turn_snapshots`]. +/// +/// In-process only: a post-turn snapshot another process is taking is not +/// seen here. +static PENDING_POST_TURN: (std::sync::Mutex, std::sync::Condvar) = + (std::sync::Mutex::new(0), std::sync::Condvar::new()); + +/// A reserved post-turn snapshot; dropping it (after the snapshot is written, +/// failed, or abandoned) releases the reservation. +#[must_use = "the reservation is released when this is dropped"] +pub struct PendingPostTurnSnapshot(()); + +impl PendingPostTurnSnapshot { + pub fn reserve() -> Self { + let (count, _) = &PENDING_POST_TURN; + *count + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) += 1; + Self(()) + } +} + +impl Drop for PendingPostTurnSnapshot { + fn drop(&mut self) { + let (count, released) = &PENDING_POST_TURN; + let mut count = count + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + *count = count.saturating_sub(1); + released.notify_all(); + } +} + +/// Wait up to `timeout` for every reserved post-turn snapshot to be written. +/// Returns whether none is still pending. +pub fn wait_for_pending_post_turn_snapshots(timeout: std::time::Duration) -> bool { + let (count, released) = &PENDING_POST_TURN; + let count = count + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let (count, _) = released + .wait_timeout_while(count, timeout, |pending| *pending > 0) + .unwrap_or_else(std::sync::PoisonError::into_inner); + *count == 0 +} diff --git a/crates/tui/src/snapshot/repo.rs b/crates/tui/src/snapshot/repo.rs index cc4f71bc21..e2dacd1c65 100644 --- a/crates/tui/src/snapshot/repo.rs +++ b/crates/tui/src/snapshot/repo.rs @@ -106,6 +106,12 @@ impl PathRestoreAction { } } +/// The safety snapshot a path restore writes first, or one already taken. +enum RestoreBackup<'a> { + Take(&'a str), + Existing(&'a SnapshotId), +} + /// Report of what [`SnapshotRepo::restore_paths`] did to one path. #[derive(Debug, Clone)] pub struct PathRestoreOutcome { @@ -1106,6 +1112,42 @@ impl SnapshotRepo { backup_label: &str, prune_emptied_dirs: bool, preflight: impl FnOnce() -> io::Result<()>, + ) -> io::Result> { + self.restore_path_plan_backed_up( + plan, + RestoreBackup::Take(backup_label), + prune_emptied_dirs, + preflight, + ) + } + + /// [`Self::restore_path_plan`] with a safety snapshot the caller already + /// took (`backup`, a commit id) instead of a new one. `preflight` must + /// prove every planned path is still as `backup` holds it, so the backup + /// is as good as one taken now; reusing it keeps a second snapshot, and + /// the prune that comes with it, out of the window between planning and + /// the first write. + pub fn restore_path_plan_with_backup( + &self, + plan: &[(PathBuf, SnapshotId)], + backup: &SnapshotId, + prune_emptied_dirs: bool, + preflight: impl FnOnce() -> io::Result<()>, + ) -> io::Result> { + self.restore_path_plan_backed_up( + plan, + RestoreBackup::Existing(backup), + prune_emptied_dirs, + preflight, + ) + } + + fn restore_path_plan_backed_up( + &self, + plan: &[(PathBuf, SnapshotId)], + backup: RestoreBackup<'_>, + prune_emptied_dirs: bool, + preflight: impl FnOnce() -> io::Result<()>, ) -> io::Result> { if plan.is_empty() { return Ok(Vec::new()); @@ -1121,7 +1163,10 @@ impl SnapshotRepo { // A durable backup is required for this destructive API. Ignored // files cannot be removed/overwritten if the snapshot cannot retain them. - let backup = self.snapshot_with_session(backup_label, None)?; + let backup = match backup { + RestoreBackup::Take(label) => self.snapshot_with_session(label, None)?, + RestoreBackup::Existing(id) => id.clone(), + }; for (rel, _, _, in_work) in &pre_state { if *in_work && !self.snapshot_contains_regular_file(&backup, rel)? { return Err(io_other( diff --git a/crates/tui/src/tools/revert_turn.rs b/crates/tui/src/tools/revert_turn.rs index f981d9043b..2a59134e46 100644 --- a/crates/tui/src/tools/revert_turn.rs +++ b/crates/tui/src/tools/revert_turn.rs @@ -7,6 +7,12 @@ //! index, so the model doesn't have to count entries. //! //! Approval is `Required` because this mutates the workspace. +//! +//! Known limit: like `/restore`, this is a whole-tree rollback. It restores +//! every path to the pre-turn snapshot, so an edit made after that turn (the +//! user's, or another session's) is overwritten too; it does not have the +//! path scoping, changed-since refusal, uncapped lookup or fork-inherited +//! restore points of `/undo` (#6644). use async_trait::async_trait; use serde_json::{Value, json}; @@ -35,7 +41,8 @@ impl ToolSpec for RevertTurnTool { Use when the user explicitly asks to undo, revert, or roll back the most recent edits. \ `turn_offset` is 1-based: 1 reverts the most recent turn, 2 reverts the previous one, \ and so on (max 50). Conversation history is NOT modified — only working-tree files are \ - restored from the side-git snapshot repo." + restored from the side-git snapshot repo. This restores the whole workspace, so any \ + edit made after that turn, including the user's own, is overwritten as well." } fn input_schema(&self) -> Value {