Repository navigation
fix(id): confine REPL match-derived download paths (#2307) - #4472
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 56 minutes. View limit details
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. |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a8dc51db
ℹ️ 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".
| // Explicit user path stays the user's choice. | ||
| Some(path) => path.to_owned(), | ||
| // Match-derived default: confine via the shared CLI ruling. | ||
| None => download_output_path(None, &m.name)?, |
There was a problem hiding this comment.
When a match is named nested/-, find nested/- --file now reduces its destination to -. ReplContext::get forwards that value to export_blob, whose output == "-" branch (pkgs/id/src/lib.rs:569) writes the content to stdout instead of saving a file. Previously, nested/- was treated as a literal file path. The same regression affects search --file and interactive match-derived file outputs. Represent the derived filename as ./- or otherwise distinguish it from the stdout sentinel, while preserving explicit user-supplied - behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b61edc3: download_output_path now returns ./- for a match-derived bare - (no-dir case), so derived filenames never collide with the stdout sentinel in export_blob; explicit user-supplied - is untouched. Regression test test_download_output_path_dash_name_is_not_the_stdout_sentinel covers -, nested/-, and the dir case.
| if let Some(previous) = destinations.insert(output_path.clone(), m.name.as_str()) { | ||
| println!( | ||
| "refusing bulk download: destination collision between matches {:?} and {:?} at {:?}", | ||
| previous, m.name, output_path | ||
| ); | ||
| return; |
There was a problem hiding this comment.
When overlapping queries return the same match, selecting both displayed entries now refuses the entire download even though they reference identical content and the same name. For example, find report txt --dir downloads can list report.txt twice because collect_matches appends results independently for each query and apply_search_options does not deduplicate them; selecting both entries reaches this collision branch and writes nothing. Previously that selection succeeded, and the updated --all path already avoids this problem by deduplicating (hash, name) before checking destinations. Apply that deduplication to interactive directory and default-file output too, while continuing to reject collisions between distinct matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b61edc3: both interactive paths (output_selected_matches_to_dir and the interactive --file preflight) now deduplicate identical (hash, name) matches before the destination-collision check, matching the --all path; collisions between distinct matches are still refused as a batch. Regression test test_interactive_dir_duplicate_identical_selections_fetch_once covers selecting the same match twice.
Summary
download_output_pathhelper inhelpers.rs(the CLI's existing final-component ruling from id(find/search): a remote node's match name becomes the download path verbatim — ../ and absolute names overwrite arbitrary files #1614/id(find/search): PR #1622's final-component confinement makes distinct --all --dir matches silently overwrite each other #2178) and use it for every REPL match-derived download destination inrepl/runner.rs;commands/find.rsnow uses the same helper instead of its private copy.--diroutput, interactive selected-to-dir, and selected-to-file paths all derive from the match name's final path component only; names with no final component (..,/, trailing separator, empty) are refused and nothing is written.-) semantics are unchanged.Closes #2307
Verification
cargo test -p id --lib repl::runner::tests— result recorded on the PR / task 5238 checkpoint (fleet laptop load has repeatedly timed this compile out; see issue id(repl): find/search download paths use the match name verbatim — the #1614 confinement never reached the REPL copies (../ and absolute names overwrite arbitrary files) #2307 notes).Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.