feat(checkpoint): opt-in pre-edit file backups for one-step undo - #53
Conversation
The agent edited files unattended with no rollback: a bad multi-edit had to be reconstructed by hand. Adds an opt-in checkpoint layer so an edit can be undone. Design — back up only the files the agent *touches*, not the whole tree. Before `edit_file`/`write_file` mutates a file, its prior content (or its absence, for a newly-created file) is saved under `.system_generated/checkpoints/<id>/`. Datasets and model weights in an ML workspace are never snapshotted, so this is safe at any workspace size — the reason a whole-tree shadow-git snapshot is impractical here. - `src/checkpoint.rs`: `CheckpointStore` (snapshot / list / restore) behind a process-wide `OnceLock`, like the other startup-config singletons. `snapshot_before` is a no-op when disabled and best-effort (a backup failure is logged, never breaks the edit). Restoring a creation removes the file; restoring an edit rewrites the prior bytes. - **Restore is symlink/TOCTOU-safe**: in restricted mode it refuses a target whose final component became a symlink after the snapshot and re-resolves the write through the file tools' own `resolve_path` (canonicalizing, so a swapped-in symlinked parent that resolves outside the sandbox is rejected) — closing an arbitrary-write-outside-sandbox vector that a lexical-only check would miss. - Retention: keep the most recent `MAX_CHECKPOINTS` (200), pruning the oldest on each snapshot, so disk can't grow without bound. - `checkpoint` tool (list/restore), registered only when enabled. - `edit_file` / `write_file` call `checkpoint::snapshot_before` before writing. - Config: `checkpoint_enabled` (default false); `main` initialises the store (restore confined to the sandbox in restricted mode) and registers the tool. Tests: snapshot→edit→restore recovers content; restoring a created file removes it; restore refuses a meta.path outside base and a symlinked target (no out-of-sandbox write); bad ids rejected; list newest-first.
Independent code review — UPSTREAM-READYSecurity review of the pre-edit checkpoint/undo. No remaining CRITICAL/HIGH.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a pre-edit file backup and checkpointing system to support one-step undo functionality for file modifications. It adds a new checkpoint module, configures a checkpoint_enabled setting, registers the CheckpointTool, and integrates snapshotting before file writes and edits. The review feedback highlights two key areas for improvement: a security vulnerability where file deletion is susceptible to symlink/TOCTOU directory traversal attacks (which can be resolved by safely resolving parent directories), and a performance bottleneck in the prune function that can be optimized by sorting directory entries by modification time instead of parsing metadata JSON files on every snapshot.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // The snapshotted edit created the file; undo = remove it. `remove_file` unlinks a | ||
| // symlink itself (not its target), so a lexical containment check is sufficient here. | ||
| if let Some(base) = &self.base { | ||
| if !target.starts_with(base) { | ||
| return Err( | ||
| "checkpoint target is outside the workspace; refusing to restore".to_string(), | ||
| ); | ||
| } | ||
| } | ||
| match std::fs::remove_file(&target) { | ||
| Ok(()) => Ok(format!( | ||
| "Removed {} (it was created after checkpoint {}).", | ||
| m.path, id | ||
| )), | ||
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => { | ||
| Ok(format!("{} is already absent.", m.path)) | ||
| } | ||
| Err(e) => Err(format!("restore remove: {e}")), | ||
| } |
There was a problem hiding this comment.
The lexical check target.starts_with(base) is insufficient to prevent symlink/TOCTOU directory traversal attacks when deleting files. While std::fs::remove_file does not follow a symlink at the final component, it will follow symlinks in any parent directory components. If an attacker swaps a parent directory with a symlink pointing outside the sandbox, remove_file will delete files outside the sandbox. To prevent this, we should safely resolve the parent directory of the target using resolve_path before performing the deletion.
// The snapshotted edit created the file; undo = remove it. To prevent symlink/TOCTOU
// attacks via parent directories, we safely resolve the parent directory first.
let safe_target = if let Some(base) = &self.base {
let parent = target.parent().ok_or_else(|| "invalid target path".to_string())?;
let safe_parent = crate::tools::builtin::resolve_path(&parent.to_string_lossy(), base, true)
.map_err(|e| format!("checkpoint target parent rejected: {e}"))?;
let file_name = target.file_name().ok_or_else(|| "invalid target filename".to_string())?;
safe_parent.join(file_name)
} else {
target
};
match std::fs::remove_file(&safe_target) {
Ok(()) => Ok(format!(
"Removed {} (it was created after checkpoint {}).",
m.path, id
)),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
Ok(format!("{} is already absent.", m.path))
}
Err(e) => Err(format!("restore remove: {e}")),
}| fn prune(&self) { | ||
| let entries = self.list(); // newest first | ||
| for e in entries.iter().skip(MAX_CHECKPOINTS) { | ||
| let _ = std::fs::remove_dir_all(self.root.join(&e.id)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of prune calls self.list(), which reads and parses the meta.json file for every single checkpoint in the store. Since prune is called on every snapshot (before every file write or edit), this introduces a significant performance bottleneck as the number of checkpoints grows. We can optimize this by sorting the directory entries by their modification time using metadata, avoiding any file reads or JSON parsing.
fn prune(&self) {
let Ok(rd) = std::fs::read_dir(&self.root) else {
return;
};
let mut dirs = Vec::new();
for entry in rd.flatten() {
let path = entry.path();
let modified = entry.metadata()
.and_then(|m| m.modified())
.unwrap_or(std::time::SystemTime::UNIX_EPOCH);
dirs.push((path, modified));
}
if dirs.len() <= MAX_CHECKPOINTS {
return;
}
dirs.sort_by_key(|x| x.1); // oldest first
let to_remove = dirs.len() - MAX_CHECKPOINTS;
for (path, _) in dirs.into_iter().take(to_remove) {
let _ = std::fs::remove_dir_all(path);
}
}|
Addressed both:
All 7 checkpoint tests green. |
|
Heads-up: this PR was merged before the two code-review fixes landed (the parent-symlink/TOCTOU hardening on the delete path — security-HIGH — and the prune mtime optimization). I've opened them as a follow-up in #56 against current |
…nk-toctou fix(checkpoint): parent-symlink TOCTOU on delete + prune by mtime (follow-up to #53)
Summary
The agent edited files unattended with no rollback — a bad multi-edit had to be reconstructed by hand. This adds an opt-in checkpoint layer for one-step undo.
Design — back up only the files the agent touches, not the whole tree. Before
edit_file/write_filemutates a file, its prior content (or its absence, for a newly-created file) is saved under.system_generated/checkpoints/<id>/. Datasets and model weights in an ML workspace are never snapshotted, so this is safe at any workspace size — which is exactly why a whole-tree shadow-git snapshot is impractical here.Changes
src/checkpoint.rs:CheckpointStore(snapshot / list / restore) behind a process-wideOnceLock.snapshot_beforeis a no-op when disabled and best-effort (a backup failure is logged, never breaks the edit). Restoring a creation removes the file; restoring an edit rewrites the prior bytes.resolve_path(canonicalizing, so a swapped-in symlinked parent that resolves outside the sandbox is rejected) — closing an arbitrary-write-outside-sandbox vector that a lexical check would miss.MAX_CHECKPOINTS(200), pruning the oldest on each snapshot, so disk can't grow without bound.checkpointtool (list/restore), registered only when enabled.checkpoint_enabled(default false);maininitialises the store (restore confined to the sandbox in restricted mode) and registers the tool.Testing
cargo check --all-targets,cargo clippy --release --all-targets(no new warnings),cargo test --lib→ 329 passed, 0 failed.Notes
Per-edit (not per-turn) granularity; the model restores specific ids. Follow-ups: group a turn's edits, and a snapshot-side symlink check for content-fidelity (low).
Branch merges cleanly into current
main; independently reviewed before upstreaming (see review comment).