feat(tools): file-change snapshot store with /undo, /rollback, and tool prompt hardening - #433
Merged
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
📝 WalkthroughWalkthroughThe PR adds session-scoped file snapshots, snapshot-aware file tools, ChangesSnapshot undo and rollback
Diff rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AishShell
participant SnapshotStore
participant FileSystem
AishShell->>SnapshotStore: select rollback target
SnapshotStore-->>AishShell: return restore actions
AishShell->>FileSystem: restore or remove files
FileSystem-->>AishShell: return disk result
AishShell->>SnapshotStore: commit successful rollback
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jexShain
force-pushed
the
feat/file-snapshot-undo
branch
from
August 6, 2026 03:43
f9f0542 to
535cdd3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/aish-tools/src/fs/snapshot_store.rs (1)
271-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider verifying
snapshot_idwhen committing an undo.
UndoResultcarriessnapshot_id, butcommit_undo_lastandcommit_undo_last_forignore it. They pop or remove whatever entry is newest at commit time. If any mutation lands between the peek and the commit, the wrong snapshot is consumed and the remembered tag is rewound to the wrong content.The module doc at lines 379-384 states that the shell drives tools sequentially, so this is currently latent. An id check makes the peek → disk IO → commit protocol self-verifying and removes the dependency on that ordering assumption.
♻️ Proposed id-checked commit variants
+ /// Commit the undo for a specific snapshot id. Returns `None` when the + /// id is no longer the entry that would be undone, so a stale peek is + /// rejected instead of consuming the wrong snapshot. + pub fn commit_undo(&mut self, snapshot_id: u64) -> Option<FileSnapshot> { + if self.history.last()?.id != snapshot_id { + return None; + } + self.commit_undo_last() + } + + /// Commit the undo for a specific path and snapshot id. + pub fn commit_undo_for(&mut self, path: &Path, snapshot_id: u64) -> Option<FileSnapshot> { + let key = normalize_path(path); + let idx = self.history.iter().rposition(|s| s.path == key)?; + if self.history[idx].id != snapshot_id { + return None; + } + let snap = self.history.remove(idx); + self.rewind_tag(&snap); + Some(snap) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/fs/snapshot_store.rs` around lines 271 - 287, Update commit_undo_last and commit_undo_last_for to accept the expected snapshot_id from UndoResult and verify the selected history entry’s snapshot_id before removing it. Return None without mutating history or rewinding tags when the ID does not match, preserving the existing successful-commit behavior.crates/aish-tools/src/undo_edit/undo_edit.rs (1)
102-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for path-scoped undo and for the failed-restore-retains-history guarantee.
The test suite covers the no-path overwrite case, the created-file deletion case, and the empty-history error case. It does not cover
peek_undo_last_for/commit_undo_last_for(thepathargument path), and it does not verify the documented guarantee that a failedapply_to_diskcall leaves the history entry intact for a retry.Add a test that makes the target path unwritable (or otherwise forces
apply_to_diskto fail) and asserts the snapshot is still present in the store afterward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/undo_edit/undo_edit.rs` around lines 102 - 156, Extend the undo_edit tests with path-scoped coverage using the path argument and the SnapshotStore APIs peek_undo_last_for and commit_undo_last_for, verifying the correct snapshot is selected and committed. Add a failed-restore test that forces apply_to_disk to fail, then assert the corresponding snapshot remains in the store so the undo can be retried.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-shell/src/theme.rs`:
- Around line 510-517: Update render_diff and its line_diff input handling so
edits beyond the 1,000-line resource bound are still detected when the bounded
prefixes match, while preserving the existing limit. Ensure the fallback
includes changed tail regions and renders them correctly, then add a regression
test covering more than 1,000 unchanged leading lines followed by a tail edit.
- Around line 601-624: The sequential truncation after building lines can drop
changed hunks and produce max_lines + 1 output. Update the region-rendering flow
around choose_regions and render_diff_line to account for elision-marker budget
while selecting or emitting regions, preserve all changed lines when they fit
within max_lines, and reserve capacity for the final omission marker when
content is dropped. Add tests covering distant changed hunks and asserting the
rendered output never exceeds max_lines.
In `@crates/aish-tools/src/write_file/write_file.rs`:
- Around line 89-128: Update the successful write response around skip_rollback
in the write_file implementation to append a localized
tools.fs.write_file.not_undoable marker whenever the write is untracked, while
preserving the existing output for tracked writes. Add the corresponding
translation key to every locale file with text explaining that the change is not
undoable because the prior content could not be snapshotted.
- Around line 195-231: Guard the Unix test
write_file_unreadable_prior_skips_rollback by detecting whether the effective
user is root and returning early when it is. Keep the existing permission setup
and assertions unchanged for non-root execution, using the platform-appropriate
UID check already available in the test environment.
---
Nitpick comments:
In `@crates/aish-tools/src/fs/snapshot_store.rs`:
- Around line 271-287: Update commit_undo_last and commit_undo_last_for to
accept the expected snapshot_id from UndoResult and verify the selected history
entry’s snapshot_id before removing it. Return None without mutating history or
rewinding tags when the ID does not match, preserving the existing
successful-commit behavior.
In `@crates/aish-tools/src/undo_edit/undo_edit.rs`:
- Around line 102-156: Extend the undo_edit tests with path-scoped coverage
using the path argument and the SnapshotStore APIs peek_undo_last_for and
commit_undo_last_for, verifying the correct snapshot is selected and committed.
Add a failed-restore test that forces apply_to_disk to fail, then assert the
corresponding snapshot remains in the store so the undo can be retried.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34094f83-8138-4fd8-b8ff-a9aadbebf1fe
📒 Files selected for processing (22)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-shell/src/app.rscrates/aish-shell/src/readline.rscrates/aish-shell/src/theme.rscrates/aish-shell/tests/slash_popup_commands.rscrates/aish-tools/src/bash/prompt.rscrates/aish-tools/src/edit_file/edit_file.rscrates/aish-tools/src/edit_file/prompt.rscrates/aish-tools/src/fs/snapshot_store.rscrates/aish-tools/src/lib.rscrates/aish-tools/src/python/prompt.rscrates/aish-tools/src/read_file/prompt.rscrates/aish-tools/src/read_file/read_file.rscrates/aish-tools/src/undo_edit/prompt.rscrates/aish-tools/src/undo_edit/undo_edit.rscrates/aish-tools/src/write_file/prompt.rscrates/aish-tools/src/write_file/write_file.rs
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…ol prompt hardening Add a per-process SnapshotStore recording write_file/edit_file mutations so users can recover AI file edits (AI-Shell-Team#205): - /undo [path]: undo the most recent file change (optional path filter) - /rollback: interactive panel to restore any prior checkpoint - undo_edit tool: lets the AI undo its own last edit - read_file stamps a content tag; edit_file enforces is_fresh server-side to reject edits against stale/drifted content (even without a model tag) Harden tool prompts so the AI uses dedicated file tools instead of bypassing the snapshot system (AI-Shell-Team#432): - bash/secure_bash: forbid redirections and file writes - python_exec: forbid open()/pathlib/shutil/os file I/O; print and use write_file instead write_file caps rollback memory: a prior larger than MAX_WRITE_BYTES is not tracked (mirrors the SIZE_LIMIT gate in read_file/edit_file); the write still succeeds, it just isn't undoable. Closes AI-Shell-Team#205, Refs AI-Shell-Team#432
…text Split render_diff output into hunks centered on changed lines, each surrounded by up to DIFF_CONTEXT (3) unchanged lines. Edits near the end of a large file stay fully visible — leading context no longer crowds them out. The max_lines cap sheds context first, dropping bare changed lines only on overflow.
jexShain
force-pushed
the
feat/file-snapshot-undo
branch
from
August 6, 2026 05:30
535cdd3 to
4ef62ad
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User-visible Changes
/undo [path]command: undo the most recent AI file change (optional path filter to undo a specific file)./rollbackcommand: interactive panel to review and restore any prior checkpoint.write_file/edit_fileresults carry a content tag; a non-undoable write (unreadable/oversized prior) now reports(not undoable: prior content unavailable).Compatibility
/undoand/rollbackdescriptions mark "this session").commit_undo_last/commit_undo_last_fornow require asnapshot_idargument (self-verifying peek→commit). All callers updated.Testing
cargo fmt --all -- --check✅cargo clippy --workspace --all-targets -- -D warnings✅cargo test --workspace✅ — incl. new regression tests:cargo build --release --target x86_64-unknown-linux-musl+aish --helpsmoke ✅Change Type
/undo,/rollback,undo_edittool, server-side drift detectionmax_lines+1and could drop later changed hunksScope
Implements #205 (检查点/快照/回退). Refs #432 (tool prompt hardening).
Snapshot & undo (#205)
/undo [path],/rollbackpanel,undo_edittool — all use peek → apply → commit-on-success withsnapshot_idverification (a stale peek is rejected rather than consuming the wrong snapshot).read_filestamps a content tag;edit_fileenforcesis_freshserver-side to reject edits against stale/drifted content even without a model-supplied tag.MAX_HISTORY=200FIFO;write_fileskips rollback for priors larger thanMAX_WRITE_BYTES(mirrors theSIZE_LIMITgate inread_file/edit_file).Tool prompt hardening (#432)
open()/pathlib/shutil/os file I/O; guide the AI toprintand usewrite_file/edit_file.Design notes
is_freshhard safety net (edit drift detection). Thewrite-creates-new-file path has nois_freshnet (no prior tag), so it relies on prompt guidance.Closes #205
Refs #432
Summary by CodeRabbit
New Features
/undoto reverse the latest file change./rollbackto review and restore earlier file changes.Documentation