refactor: address code audit findings#64
Conversation
Bug fixes: - main.rs: .or() → .and() to check both terminal resume/leave results - copy.rs: fix off-by-one in recursion depth check (>= instead of >) - helpers.rs: .metadata() → .symlink_metadata() to avoid following symlinks Refactoring: - Extract shutdown_job() helper in main.rs (DRY) - Extract suffixed_path() in temp.rs - Extract match_wildcard_simple() in pattern.rs (keep matches() <100 lines) - Refactor delete.rs const blocks → define_critical_lists! macro - Optimize pattern.rs: pre-compile needle_bytes, add prefix_str/suffix_str fast paths - Optimize watcher.rs: flush_expired via retain() instead of collect+remove - Optimize list_picker.rs: eliminate Vec<ListItem> intermediate allocation Safety & correctness: - job_runner.rs: wrap thread::spawn in catch_unwind to prevent double-panic - app_state.rs: fallback to temp_dir() instead of hardcoded / - path.rs: fallback to . instead of / - move_ops.rs: add symlink_metadata() existence check before move - common.rs: document strip_prefix invariant with expect Code quality: - Extract magic numbers to constants (SPINNER_TICK_INTERVAL, WATCH_CHANNEL_CAPACITY, BYTES_PER_UNIT, HEX_OFFSET_PREFIX_WIDTH) - Rename restore_ok → already_restored in shell.rs - panel.rs: &Vec<PathBuf> → &[PathBuf], HashMap::with_capacity optimization - Add clarifying comments throughout (TOCTOU, platform-specific code, intentional fallbacks) - Standardize test setup: AppState::default() → AppState::new() Reviewed by 13 parallel agents. All 964 tests pass, clippy clean.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leszek3737, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![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 various optimizations, refactorings, and platform-specific improvements across the codebase, including case-insensitive search optimizations, safer fallback directory handling, and cleaner terminal state recovery. However, two issues were identified in the review: first, removing the is_symlink check in remove_any introduces a regression when deleting symlinks pointing to directories, as is_dir traverses symlinks; second, using catch_unwind to handle thread spawning failures in job_runner.rs is non-idiomatic and should be replaced with std::thread::Builder::new().spawn().
| if path.is_dir() { | ||
| std::fs::remove_dir_all(path) | ||
| } else { | ||
| std::fs::remove_file(path) |
There was a problem hiding this comment.
Removing the path.is_symlink() check introduces a regression when deleting symlinks pointing to directories. In Rust, Path::is_dir() traverses symlinks to query the target. Consequently, if path is a symlink to a directory, path.is_dir() will return true, causing remove_any to call std::fs::remove_dir_all(path) instead of std::fs::remove_file(path). On Unix-like systems, calling remove_dir_all on a symlink to a directory will fail with ENOTDIR (Not a directory). Restoring the is_symlink() check ensures that symlinks themselves are correctly removed without traversing them.
if path.is_symlink() {
std::fs::remove_file(path)
} else if path.is_dir() {
std::fs::remove_dir_all(path)
} else {
std::fs::remove_file(path)| let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | ||
| std::thread::spawn(move || { | ||
| let (tx, rx) = mpsc::channel(); | ||
| let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | ||
| let _ = std::thread::spawn(move || { | ||
| let result = handle.join(); | ||
| let _ = tx.send(result); | ||
| }); | ||
| })); | ||
| match rx.recv_timeout(Duration::from_secs(5)) { | ||
| Ok(Err(e)) => { | ||
| debug_log!("worker thread panicked during tear-down: {:?}", e); | ||
| } | ||
| Err(_) => { | ||
| debug_log!( | ||
| "worker thread did not finish within 5 s — abandoning reaper" | ||
| ); | ||
| } | ||
| Ok(Ok(())) => {} | ||
| } | ||
| Ok(Ok(())) => {} | ||
| } | ||
| }); | ||
| }); | ||
| })); |
There was a problem hiding this comment.
Using std::panic::catch_unwind to guard against thread spawning failures is verbose and non-idiomatic. In Rust, std::thread::spawn panics if the OS fails to create a thread, but std::thread::Builder::new().spawn() returns a Result instead of panicking. Using Builder::spawn allows you to handle thread creation failures safely and cleanly without needing catch_unwind or AssertUnwindSafe.
let _ = std::thread::Builder::new().spawn(move || {
let (tx, rx) = mpsc::channel();
let _ = std::thread::Builder::new().spawn(move || {
let result = handle.join();
let _ = tx.send(result);
});
match rx.recv_timeout(Duration::from_secs(5)) {
Ok(Err(e)) => {
debug_log!("worker thread panicked during tear-down: {:?}", e);
}
Err(_) => {
debug_log!(
"worker thread did not finish within 5 s — abandoning reaper"
);
}
Ok(Ok(())) => {}
}
});
Greptile SummaryA broad code-quality and safety pass touching 32 files: three bug fixes (
Confidence Score: 4/5Safe to merge with minor follow-up; the only substantive concerns are in All three stated bug fixes are correct and well-targeted. The refactoring in src/ops/file_ops/common.rs — both the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[delete_dir_recursive_with_cancel] --> B{is symlink?}
B -- yes --> C[remove_symlink leaf only]
B -- no --> D[canonicalize path]
D --> E{is root?}
E -- yes --> F[PermissionDenied]
E -- no --> G{in CRITICAL_DIRS exact match?}
G -- yes --> H[PermissionDenied]
G -- no --> I{is_under_temp?}
I -- no --> J{in CRITICAL_DIR_PREFIXES starts_with?}
J -- yes --> K[PermissionDenied]
J -- no --> L[delete_dir_contents]
I -- yes --> L
L --> M[remove_dir leaf]
subgraph remove_any
N[path] --> O{is_dir? follows symlinks}
O -- yes --> P[remove_dir_all]
O -- no --> Q[remove_file]
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[allow(clippy::expect_used)] | ||
| let suffix = path | ||
| .strip_prefix(ancestor) | ||
| .unwrap_or_else(|_| Path::new("")); | ||
| .expect("ancestor must be a parent of path"); |
There was a problem hiding this comment.
#[allow(clippy::expect_used)] outside a test block
AGENTS.md explicitly forbids this annotation except inside mod tests blocks: "No #[allow(...)] or #[expect(...)] to suppress lints — fix the underlying issue instead." expect_used should not be suppressed in production paths. Since the invariant is documented in the comment and guaranteed by construction, one valid resolution is to restructure the loop so strip_prefix is never called with a non-parent ancestor (e.g., pass the suffix by construction instead of re-deriving it here), or keep the original unwrap_or_else(|_| Path::new("")) fallback which clippy accepts without an attribute.
Context Used: CLAUDE.md (source)
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!
- common.rs: restore is_symlink() check in remove_any() to prevent remove_dir_all on symlinks to directories (Gemini) - job_runner.rs: replace catch_unwind with thread::Builder::spawn() which returns Result instead of panicking (Gemini) - common.rs: revert expect to unwrap_or_else to avoid #[allow(clippy::expect_used)] in production code (Greptile)
1 participant
Bug fixes:
Refactoring:
Safety & correctness:
Code quality: