fix(file_ops): safety hardening — TOCTOU docs, error propagation, dedup, tests#77
Conversation
…up, tests - Coordination: centralize MSG_DEST_EXISTS, MSG_CRITICAL_DIR, MSG_SYMLINK_CHMOD into common.rs; add is_dir_reparse() helper; gate chmod on #[cfg(unix)] - WS-A: reject_same_file canonicalize fallback fixed (and_then + is_ok_and instead of ok().zip().is_some_and); strip_prefix debug_log on invariant violation; remove_any non-adversarial contract doc; collapse check_canceled/check_optional_canceled; error source chain doc - WS-B: TOCTOU race in publish_temp_dir (overwrite=false) documented; validate_copy_targets extracted; preserve_timestamps on top-level dir after publish_temp_dir; TEMP_DIR_COUNTER — AtomicU64, atomic rename sequence documented in 3 functions; reservation patterns doc - WS-C: .map(|_bytes| ()) comment; create_dir_all rationale doc; MSG_SYMLINK_CHMOD dedup; rename_entry source/dest inode guard - WS-D: is_canceled single post-loop check; magic const rationales in chunk_copy.rs; #[cfg(unix)] on chmod re-export - Code review: root guard comment corrected (Unix only); /tmp added to CRITICAL_DIRS; same-inode symlink error clarified - Tests: 11 new — overwrite:true copy, canceled+errors format_summary, move cancel, error kind assertions, normalize_suffix (5 subtests), root directory rejection, overwrite.rs conflict tests
Reviewer's GuideHardens file/batch operations against TOCTOU and error-handling issues by centralizing shared messages/helpers, refining delete/move/copy semantics (including symlink and critical-dir handling), tightening rename/chmod behavior, and expanding regression tests for overwrite, cancellation, and error reporting. Flow diagram for classify_entry-based copy/delete dispatchflowchart LR
SrcPath[Path] --> Classify[classify_entry]
Classify --> KindSymlink{EntryKind::Symlink}
Classify --> KindDir{EntryKind::Dir}
Classify --> KindFile{EntryKind::File}
KindSymlink --> CopySymlink[copy_entry: file_ops::copy_symlink]
KindDir --> CopyDir[copy_entry: copy_dir_recursive_with_progress]
KindFile --> CopyFile[copy_entry: copy_file_with_progress]
KindSymlink --> DelSymlink[batch_delete: file_ops::delete_file]
KindDir --> DelDir[batch_delete: delete_dir_recursive _cancelable]
KindFile --> DelFile[batch_delete: file_ops::delete_file]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several refactorings, platform-specific compatibility improvements, and robust error handling across file operations. Key changes include classifying file entries using a new EntryKind enum, refining cancellation checks in batch operations, standardizing error messages, and adding extensive unit tests for overwrite conflicts and suffix normalization. Additionally, Windows-specific directory metadata checks and macOS-specific chmod error mappings were improved. Feedback on these changes highlights that treating timestamp preservation failures as fatal during directory copies could cause operations to fail on unsupported filesystems (e.g., FAT32/exFAT); it is recommended to log a warning instead of returning an error.
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.
| )); | ||
| } | ||
| publish_temp_dir(guard.path(), dest, overwrite, src_perms)?; | ||
| preserve_timestamps(dest, &fs::metadata(src)?)?; |
There was a problem hiding this comment.
Treating preserve_timestamps as a fatal error can cause copy operations to fail completely on filesystems that do not support setting timestamps (such as some FAT32/exFAT USB drives or network mounts), even though the directory and all its files were successfully copied and published. Making timestamp preservation failures non-fatal (by logging a warning instead of returning an error) makes the copy operation much more robust.
| preserve_timestamps(dest, &fs::metadata(src)?)?; | |
| if let Err(e) = preserve_timestamps(dest, &fs::metadata(src)?) { | |
| crate::debug_log!("warning: failed to preserve timestamps for {}: {}", dest.display(), e); | |
| } |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that
classify_entryandremove_anyboth encapsulate similar file-type dispatch logic (symlink/dir/file), consider reusing one from the other (or sharing a single helper) to avoid subtle divergence in behavior over time. - The
MSG_DEST_EXISTSusage is inconsistent across call sites (e.g.,copy_fileincludes the destination path whilerename_entrydoes not); for easier debugging, consider standardizing the error format to always include the destination path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `classify_entry` and `remove_any` both encapsulate similar file-type dispatch logic (symlink/dir/file), consider reusing one from the other (or sharing a single helper) to avoid subtle divergence in behavior over time.
- The `MSG_DEST_EXISTS` usage is inconsistent across call sites (e.g., `copy_file` includes the destination path while `rename_entry` does not); for easier debugging, consider standardizing the error format to always include the destination path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR hardens the file-operations layer across four workstreams: TOCTOU documentation and narrow-window re-checks in copy/temp, deduplication of shared string constants into
Confidence Score: 4/5Safe to merge with one fix: The rest of the PR — TOCTOU docs, constant deduplication, src/ops/batch.rs — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[execute_batch_generic] --> B{top-of-loop\nis_canceled?}
B -- yes --> C[state.canceled = true\nbreak]
B -- no --> D[process_batch_entry]
D --> E{Interrupted +\nis_canceled?}
E -- yes --> F[state.canceled = true]
E -- no --> G[record result]
F --> H{state.canceled?}
G --> H
H -- yes --> I[break]
H -- no --> B
I --> J[post-loop final check]
J -- is_canceled? --> K[state.canceled = true]
J --> L[return BatchReport]
K --> L
M[batch_delete] --> N{top-of-loop\nis_canceled?}
N -- yes --> O[canceled = true / break]
N -- no --> P[classify + delete]
P --> Q{Interrupted +\nis_canceled?}
Q -- yes --> R[canceled = true]
Q -- no --> S[record result]
R --> T{canceled?}
S --> T
T -- yes --> U[break]
T -- no --> N
U --> V[return BatchReport]
N -. last item .-> V
style J fill:#90EE90
style K fill:#90EE90
style V fill:#FFB6C1
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Ok(_) => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::AlreadyExists, | ||
| format!("destination already exists: {}", dest.display()), | ||
| )); | ||
| } |
There was a problem hiding this comment.
MSG_DEST_EXISTS constant not used here, unlike every other callsite in the module. If the constant text changes, this message will silently diverge.
| Ok(_) => { | |
| return Err(io::Error::new( | |
| io::ErrorKind::AlreadyExists, | |
| format!("destination already exists: {}", dest.display()), | |
| )); | |
| } | |
| Ok(_) => { | |
| return Err(io::Error::new( | |
| io::ErrorKind::AlreadyExists, | |
| format!("{MSG_DEST_EXISTS}: {}", dest.display()), | |
| )); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /// Renames a filesystem entry within its parent directory. | ||
| /// | ||
| /// Validates that `new_name` is a single filename component (no separators, no `..`). | ||
| /// Detects same-inode renames (e.g., case-only rename on case-insensitive FS). | ||
| /// On POSIX, the `try_exists` guard is best-effort — `fs::rename` atomically | ||
| /// replaces the destination; true atomic no-replace requires `RENAME_NOREPLACE` | ||
| /// or `renamex_np`, which are out of stdlib. | ||
| fn validate_entry_name(new_name: &str) -> io::Result<()> { |
There was a problem hiding this comment.
rename_entry, not the private validate_entry_name helper extracted from it. The comment should sit on the public function.
| /// Renames a filesystem entry within its parent directory. | |
| /// | |
| /// Validates that `new_name` is a single filename component (no separators, no `..`). | |
| /// Detects same-inode renames (e.g., case-only rename on case-insensitive FS). | |
| /// On POSIX, the `try_exists` guard is best-effort — `fs::rename` atomically | |
| /// replaces the destination; true atomic no-replace requires `RENAME_NOREPLACE` | |
| /// or `renamex_np`, which are out of stdlib. | |
| fn validate_entry_name(new_name: &str) -> io::Result<()> { | |
| fn validate_entry_name(new_name: &str) -> io::Result<()> { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
All tests pass, clippy is clean, and formatting is compliant. Here's my comprehensive review: PR Review: Safety Hardening for File OperationsOverall: This is a well-intentioned hardening PR with good documentation, useful refactoring ( Bugs / Behavioral Concerns1. When the backup restore fails in the error path, the error is only Consider propagating the restore error (wrapped with context) so the caller can surface it, or at minimum document this behavior in the doc comment. 2. The PR correctly added a post-loop // batch_delete — current code after the loop (line 793):
// (no final is_canceled check)
// execute_batch_generic — current code after the loop (line 504-508):
if !state.canceled && is_canceled(cancel) {
state.canceled = true;
}This should be made consistent. Code Quality / Convention Issues3. This function is defined with 4. Missed message deduplication in The PR's stated goal is to "Centralize MSG_DEST_EXISTS... into common.rs", but format!("destination already exists: {}", dest.display()),instead of the centralized 5. Dead branch in if meta.is_symlink() && is_dir_meta(&meta) {On non-Windows, 6. Overly broad visibility on constants (
Minor Issues7. Test name doesn't match behavior (
8. The test's progress callback sets 9. Error kind change in When both cancel and rollback fail, the error kind changes from Positive Aspects
|
…cel check - copy.rs: preserve_timestamps failure now non-fatal (warn+continue) for FAT32/exFAT compatibility (gemini-code-assist) - temp.rs: use MSG_DEST_EXISTS constant instead of raw string (greptile) - entry_ops.rs: move rename_entry doc comment to correct function (greptile) - common.rs: remove dead is_dir_reparse() (greptile — YAGNI + lint policy) - common.rs: clarify Windows-only branch in remove_any (greptile) - batch.rs: add missing post-loop cancel check to batch_delete (greptile)
|
I've completed my thorough review. Here's my analysis: PR Review: Safety Hardening for File OperationsCI Status: All green — Important ContextThis PR has two commits. The second commit (
Remaining Concerns1. The base branch propagated the restore error with a descriptive message: fs::rename(&backup.entry, dest).map_err(|restore_err| {
io::Error::other(format!("failed to restore destination {} ...", ...))
})?;This PR changed it to a bare Notably, 2. Test doesn't exercise the final-check fix (batch/tests.rs:757-778) — Low
3.
Verified Correct
VerdictSolid PR. The refactoring, documentation, and safety improvements are well-executed. The one concern worth addressing before merge is the |
1 participant
Summary by Sourcery
Harden file operations and batch processing against edge cases and TOCTOU concerns while improving error messaging and cancellation handling.
Bug Fixes:
Enhancements:
Tests: