Conversation
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) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for older in &owned[index..] { | ||
| if repo | ||
| .path_matches_snapshot(&older.tree, &path) | ||
| .map_err(compare_failed)? | ||
| { | ||
| continue 'paths; | ||
| } |
There was a problem hiding this comment.
🟡 Later edits bypass changed-file refusal
When a later edit restores an older snapshot's bytes, plan_undo_step treats that path as already undone. It then restores other paths and skips the edited file, leaving a partial undo.
Learn more
The planner compares the current bytes of each path changed in a step against that step's end. If they differ, it also compares against every older restore point, assuming any match means an earlier undo reverted the path. A manual edit can produce exactly those bytes, so the planner cannot distinguish it from an undo. prune_undone_tool_context only changes transcript context, not a durable undo cursor.
Example: tool:1 changes a.txt from A to B; tool:2 changes it from B to C and also creates new.txt. The user edits a.txt back to A. /undo for tool:2 skips a.txt because A matches tool:1's snapshot, removes new.txt, and claims the step was restored, even though its target had B.
Recommended fix: Record the undo cursor or completed step/path restoration explicitly. Only treat an older match as prior undo when a recorded successful undo produced that state; otherwise refuse a mismatch with the step's end.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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), | ||
| }); |
There was a problem hiding this comment.
🔴 Fork inherits snapshots beyond its conversation
When /fork-from copies a saved parent, snapshot_owners admits every parent snapshot before creation. If the parent has unsaved turns, the fork can undo work absent from its transcript.
Learn more
fork_from builds its transcript from a saved source session, but the live source can take later snapshots without saving that transcript. Ownership is then assigned by snapshot time alone, so the fork receives source restore points for turns that were never copied. The forked_from_message_count field cannot identify these snapshots reliably without turn-bound snapshot receipts.
Example: The parent was saved after turn 1. It then runs turn 2, changing b.txt, without saving again. /fork-from copies the saved transcript containing only turn 1, but its creation time comes after turn 2's snapshots. /undo in the fork chooses turn 2's restore point and rolls back b.txt.
Recommended fix: Bound inherited restore points to turns actually copied into the fork, using recorded turn-to-snapshot references or a durable snapshot boundary captured with the saved session.
Was this helpful? React with 👍 or 👎 to provide feedback.
| - 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)). |
There was a problem hiding this comment.
🔍 Move changelog entries to merge time
The contribution workflow reserves changelog edits for a batched commit on main. These hunks touch both reserved changelogs and can conflict with other PRs.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1702eec79c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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)); |
There was a problem hiding this comment.
For a saved session produced by RuntimeThreads::fork_at_user_turn, the child document retains only the selected prefix (runtime_threads.rs:9642-9745), but this cutoff grants ownership of every parent snapshot created before the fork's wall-clock time. Consequently, opening that child in the TUI and running /undo can select a newer parent turn that the child deliberately dropped and restore its files while pruning unrelated child conversation context. Ownership needs to be derived from the retained turns' snapshot receipts rather than created_at; the seconds-only comparison also cannot distinguish source snapshots taken just after a fork within the same second.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
| if repo | ||
| .work_tree_matches_snapshot(&target.tree) | ||
| .map_err(compare_failed)? | ||
| { | ||
| continue; |
There was a problem hiding this comment.
When the newest step has no later restore point and it only created new files, work_tree_matches_snapshot returns true because its underlying git diff --quiet <tree> does not report untracked paths. This continue therefore reports no snapshot-level change, and the /undo dispatcher falls back to removing the conversation while leaving the files created by the interrupted or unfinished turn in place. Take the current tree snapshot first and compare the two trees, or otherwise include untracked paths in this check.
Useful? React with 👍 / 👎.
| return Err(UndoRefusal::Nothing( | ||
| "No undoable snapshots for the current session — nothing to revert.".to_string(), |
There was a problem hiding this comment.
The newly added /undo refusal, no-op, trust-gate, and success messages are constructed as English literals, so localized TUI sessions display untranslated output. Add MessageId entries and build these messages with tr(locale, MessageId::...) rather than introducing additional hard-coded user-visible prose.
AGENTS.md reference: crates/tui/AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
| for older in &owned[index..] { | ||
| if repo | ||
| .path_matches_snapshot(&older.tree, &path) | ||
| .map_err(compare_failed)? |
There was a problem hiding this comment.
When a changed path matches neither the step's end nor an older snapshot, this loop invokes path_matches_snapshot once per retained restore point, and that helper launches git ls-tree plus git hash-object; with many paths and the now-uncapped snapshot list, a changed-since refusal can synchronously spawn thousands of processes from the TUI command path and freeze input/rendering. Run the planning and Git work under spawn_blocking or otherwise batch the historical comparisons instead of performing this nested scan inline.
AGENTS.md reference: AGENTS.md:L164-L170
Useful? React with 👍 / 👎.
…r 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) <noreply@anthropic.com>
| if repo | ||
| .work_tree_matches_snapshot(&target.tree) | ||
| .map_err(compare_failed)? | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Created files survive undo after snapshot failure
When a turn only creates a file and its post-turn snapshot fails, work_tree_matches_snapshot reports no tracked changes. The planner skips that turn, so /undo removes the conversation but leaves the file.
Learn more
The newest step uses the live workspace when no later restore point exists, including when the post-turn snapshot failed. work_tree_matches_snapshot uses git diff against tracked paths and cannot see a newly created, untracked file. Skipping before taking a snapshot prevents the changed-path calculation from detecting that file. The dispatcher then falls back to conversation undo.
Example: A pre-turn snapshot has no new.txt; the turn creates new.txt, and the post-turn snapshot fails. /undo sees no tracked changes, drops the conversation, and retains new.txt instead of removing it.
Recommended fix: For a newest step lacking a later restore point, capture the current tree before deciding it is unchanged, then compare trees to detect added and deleted paths. Keep the existing trust gate ahead of snapshot writes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if restore.is_empty() { | ||
| // Already undone, changed nothing, or changed only paths `/undo` | ||
| // cannot restore: keep walking back. | ||
| continue; |
There was a problem hiding this comment.
🔴 Symlink-only turn silently loses its conversation
When a turn changes only a symlink, restore stays empty and the planner returns Nothing. /undo then drops the turn's conversation without reporting that the symlink remains.
Learn more
The planner records non-regular changed paths in skipped but only returns them when another regular path produces a restore plan. With a symlink-only turn, the loop finishes with UndoRefusal::Nothing, which dispatch treats as permission to undo the conversation. The symlink remains in the workspace and its omission never reaches the summary.
Example: A turn creates only the symlink current -> a.txt. /undo cannot restore that symlink, but removes the turn's messages and says nothing about the leftover current.
Recommended fix: Carry skipped paths into the no-restorable-step result and report them explicitly; avoid silently discarding the conversation for a turn whose file effects remain. Keep stepping back only when that behavior is reflected accurately in the message and history.
Was this helpful? React with 👍 or 👎 to provide feedback.
| static PENDING_POST_TURN: (std::sync::Mutex<usize>, std::sync::Condvar) = | ||
| (std::sync::Mutex::new(0), std::sync::Condvar::new()); |
There was a problem hiding this comment.
🟡 Unrelated snapshots stall workspace undo
When another workspace writes a post-turn snapshot, wait_for_pending_post_turn_snapshots makes /undo wait for it. The global counter can block the current workspace's command for ten seconds.
Learn more
Every engine reserves the same process-wide counter, regardless of which workspace its snapshot uses. patch_undo waits synchronously for the counter to reach zero, so an unrelated slow writer blocks the interactive command even when this workspace's snapshot is complete.
Example: Session A in workspace /alpha has already finished snapshotting. Session B in /beta has a snapshot writer blocked for 12 seconds. A's /undo waits ten seconds and refuses, although /alpha has no pending snapshot.
Recommended fix: Track reservations by workspace snapshot-repo identity, and wait only for the workspace being undone. Use a bounded wait that does not freeze unrelated UI work.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # CHANGELOG.md # crates/tui/CHANGELOG.md
Each fix below reconciles two PRs that merged textually but not semantically. - snapshot/repo.rs, receipts.rs: #6645/#6682 added SnapshotRepo::changed_paths_between(from, to) -> Vec<PathBuf> (undo, turn artifacts) and #6591 added a different changed_paths_between(from, to, limit) -> (Vec<SnapshotPathChange>, bool) (receipts). Duplicate definition; #6591's is renamed path_changes_between and its one caller (receipts) updated. - core/engine/turn_loop.rs: #6673's repl-fence approval match did not cover #6601's ApprovalResult::TimedOut. A timed-out card now refunds the tool-call budget slot and reports a timeout, like direct and code-mode calls; the audit line records "timeout" instead of "denied". - runtime_api/sessions.rs: #6640's session-owner 409 predates #6645's ApiError.code field; code: None. - tools/verifier.rs: #6671's env-scrub test called run_gate(gate) without the session_id argument run_gate takes on main (#6508). - skills/install.rs + integration harness: #6679 made install.rs read downloads through crate::utils::read_response_body_capped, but the integration harness #[path]-includes install.rs and has no utils module, so the integration test target did not compile (also on the #6679 branch). The capped reader moves to utils/response_body.rs (re-exported from utils, unchanged API) and the harness includes just that file as crate::utils. - Test files where an add-only conflict was auto-resolved by concatenating both sides lost the shared closing lines of the first test: commands/groups/debug/tests.rs (#6682 + #6591), tui/ui/tests.rs (#6635 + main), tools/shell/tests.rs (#6674 + #6679), and runtime_api/tests.rs (#6645 merge in round 1). Restored the missing `}` / `);` so each test is whole again; no assertions were dropped. - tools/shell/tests.rs: #6637's executor test expected `sort * | cat` to be admitted and then fail at run time; with #6675's rule ported into the #6637 lexer (see the #6675 merge), a word-leading unquoted `*` is refused before anything runs. The test now asserts that refusal and still checks the sentinel and option-named files are untouched. - core/engine/tests.rs -> tui/history/tests.rs: #6601's engine test asserted crate::tui::history on the trust warning, raising the runtime->UI test reference ratchet 40 -> 41 (check-command-crate- boundaries FAIL). That assertion moved to a tui::history test on workspace_trust_runtime_message, so the ratchet is back at 40. - scripts/check-blocking-calls-budget.json: runtime_api/git.rs 5 -> 6. #6648 justified this budget in its PR body (working-tree fingerprint reads in sync fns reached only from spawn_blocking); its own branch already has six such sites (the File::open used for hashing), so the recorded 5 was stale. subagent/worktree.rs tightened 7 -> 6. Checks: cargo check --workspace --tests clean (no warnings); cargo test -p codewhale-execpolicy 221+1+5+7+1 passed; cargo test -p codewhale-tui --test integration 187 passed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Stacked on #6645 (base
fix/6621-thread-snapshot-ownership); review and merge that first.Closes #6644
Problem
The TUI
/undo(commands/groups/debug/undo.rs::patch_undo) still used the older selection:SnapshotRepo::restoreon the newest differingtool:/pre-turn:snapshot, so edits made after it (the user's own edits to other files, another session's writes) were reverted too.list(100)cap: an older restore point still in the store was treated as absent.Fix
It now uses the same path-scoped contract as the Runtime's
patch-undofrom #6645:changed_paths_betweenreports for the step are restored, throughrestore_path_planwith apre-restore:safety snapshot and a preflight re-check before the first write./undowalked it back to),/undorefuses with a clear message and changes nothing.list(usize::MAX)) and uses tree ids, which survive the commit-id rewrite a prune does.parent_session_id), using a new metadata-onlySessionManager::load_session_metadata_by_id. Lineage that cannot be loaded ends the chain, so a failure means fewer steps can be undone, never someone else's.SnapshotRepo::snapshot_diff_statlost its only caller, so it is removed.Review follow-up (second commit):
TurnComplete(Post-turn UI freeze: terminal unresponsive for seconds after stream ends before copy/paste/selection works #234). The engine now reserves that snapshot beforeTurnComplete(snapshot::PendingPostTurnSnapshot), and/undowaits up to 10s for it. Previously, an immediate/undoplanned against the live workspace and raced the snapshot on the side repo's index, and the late snapshot recorded the reverted workspace as the turn's end./undoand blocked every older step./undoadds no commit and triggers no prune. The planning snapshot is reused as the restore's safety backup (SnapshotRepo::restore_path_plan_with_backup).Known limits:
post-tool:spans or declared write paths), so a step owns everything that changed between its restore point and the next one it owns. That includes an edit the user or another session made while the turn ran. It is documented onplan_undo_step.revert_turntool is still a whole-tree rollback, like/restore. Its doc and tool description now say that it overwrites later edits. Routing it through the scoped plan is not part of this PR.Evidence
commands/groups/debug/tests.rs: path scope, changed-since refusal, tool-by-tool stepping, more than 100 snapshots, and fork inheritance. All five fail against the previous implementation:test result: FAILED. 0 passed; 5 failed.cargo test -p codewhale-tui --lib -- commands::groups::debug::testsgivestest result: ok. 65 passed; 0 failed.test result: FAILED. 0 passed; 3 failed.cargo test -p codewhale-tui --lib -- snapshot:: commands::groups::debug::tests tools::revert_turngivestest result: ok. 149 passed; 0 failed, and clippy with CI's flags is clean.cargo test -p codewhale-tui --lib -- snapshot::givestest result: ok. 76 passed; 0 failed.cargo clippy -p codewhale-tui --lib --tests -- -D warnings(with CI's allows) is clean.cargo fmt --all -- --checkis clean.scripts/check-blocking-calls-budget.pyreports it is within budget.I have not dogfooded this manually in a live TUI session.
🤖 Generated with Claude Code