feat: complete option behavior runtime and e2e coverage in one pass#41
feat: complete option behavior runtime and e2e coverage in one pass#41
Conversation
There was a problem hiding this comment.
Pull request overview
This PR closes a CLI help/runtime behavior gap for rc find by wiring up the previously no-op --print and --exec options, and adds integration coverage to prevent regressions.
Changes:
- Implement
find --printto render matches using full remote paths (alias/bucket/key). - Implement
find --execto run a user-provided command per match (with{}substituted by the full remote path) and reject--execcombined with--json. - Add unit + integration tests for the new behaviors and extend the integration workflow smoke list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crates/cli/src/commands/find.rs |
Adds --exec execution, --print full-path rendering, JSON incompatibility check, plus a helper for full remote paths. |
crates/cli/tests/integration.rs |
Adds 3 e2e integration tests covering --print, --exec, and --exec --json rejection. |
.github/workflows/integration.yml |
Adds the new find --print integration test to the smoke test list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for m in &matches { | ||
| let object_path = full_object_path(&alias_name, &bucket, &m.key); | ||
| let command_text = exec_template.replace("{}", &object_path); | ||
| let output = match run_shell_command(&command_text) { | ||
| Ok(output) => output, |
There was a problem hiding this comment.
--exec builds a shell command string by substituting the (potentially untrusted) object key into exec_template and then runs it via sh -c / cmd /C. Because the substituted path is not escaped/quoted, object keys containing shell metacharacters (e.g. ;, &&, backticks, quotes, newlines, $()) can lead to unintended command execution. Prefer executing without a shell by parsing --exec into a program + args (e.g. Vec<String>) and replacing {} at the argv level, or shell-escape the substituted path (with clear docs about limitations).
There was a problem hiding this comment.
Addressed in d00f741. --exec no longer routes through sh -c / cmd /C. It now parses the template with shlex, replaces {} at argv level, and executes via Command::new(program).args(args), so object keys are not interpreted by a shell.
crates/cli/src/commands/find.rs
Outdated
| formatter.error(&format!( | ||
| "Command failed for {}: {}", | ||
| object_path, command_text |
There was a problem hiding this comment.
When the executed command exits non-zero, the error message doesn’t include the exit status (code/signal), which makes failures harder to debug—especially since command_text may be long and stderr may be empty. Consider including output.status (and/or output.status.code()) in the error, and optionally distinguishing between spawn failures vs. non-zero exit.
| formatter.error(&format!( | |
| "Command failed for {}: {}", | |
| object_path, command_text | |
| let status_desc = format!("{}", output.status); | |
| let status_code = output.status.code().unwrap_or(-1); | |
| formatter.error(&format!( | |
| "Command failed for {} (status {} [{}]): {}", | |
| object_path, status_desc, status_code, command_text |
There was a problem hiding this comment.
Addressed in d00f741. Non-zero exits now include process status in the error message: Command failed for <path> (status <status>): <rendered command>.
crates/cli/src/commands/find.rs
Outdated
| } | ||
|
|
||
| fn full_object_path(alias: &str, bucket: &str, key: &str) -> String { | ||
| format!("{alias}/{bucket}/{key}") |
There was a problem hiding this comment.
full_object_path() duplicates logic that already exists in rc_core::RemotePath::to_full_path() (and the existing helper handles empty keys without producing a trailing slash). Using RemotePath::new(alias, bucket, key).to_full_path() here would reduce duplication and keep remote path formatting consistent across the CLI.
| format!("{alias}/{bucket}/{key}") | |
| RemotePath::new(alias, bucket, key).to_full_path() |
There was a problem hiding this comment.
Addressed in d00f741. full_object_path() now delegates to RemotePath::new(...).to_full_path() to keep path formatting consistent and avoid duplication.
crates/cli/src/commands/find.rs
Outdated
| let display_matches: Vec<MatchInfo> = matches | ||
| .iter() | ||
| .map(|m| MatchInfo { | ||
| key: if args.print { | ||
| full_object_path(&alias_name, &bucket, &m.key) | ||
| } else { | ||
| m.key.clone() | ||
| }, | ||
| size_bytes: m.size_bytes, | ||
| size_human: m.size_human.clone(), | ||
| last_modified: m.last_modified.clone(), | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Calculate totals | ||
| let total_count = matches.len(); | ||
| let total_size: i64 = matches.iter().filter_map(|m| m.size_bytes).sum(); |
There was a problem hiding this comment.
display_matches is always rebuilt by cloning every MatchInfo field (including key, size_human, last_modified) even when --print is not set. For large listings this adds avoidable allocations. Consider reusing/mutating the existing matches vec in-place (e.g., only rewrite key when --print is set) so the common path doesn’t clone every entry.
| let display_matches: Vec<MatchInfo> = matches | |
| .iter() | |
| .map(|m| MatchInfo { | |
| key: if args.print { | |
| full_object_path(&alias_name, &bucket, &m.key) | |
| } else { | |
| m.key.clone() | |
| }, | |
| size_bytes: m.size_bytes, | |
| size_human: m.size_human.clone(), | |
| last_modified: m.last_modified.clone(), | |
| }) | |
| .collect(); | |
| // Calculate totals | |
| let total_count = matches.len(); | |
| let total_size: i64 = matches.iter().filter_map(|m| m.size_bytes).sum(); | |
| let display_matches: Vec<MatchInfo> = if args.print { | |
| matches | |
| .into_iter() | |
| .map(|mut m| { | |
| let full_key = full_object_path(&alias_name, &bucket, &m.key); | |
| m.key = full_key; | |
| m | |
| }) | |
| .collect() | |
| } else { | |
| matches | |
| }; | |
| // Calculate totals | |
| let total_count = display_matches.len(); | |
| let total_size: i64 = display_matches | |
| .iter() | |
| .filter_map(|m| m.size_bytes) | |
| .sum(); |
There was a problem hiding this comment.
Addressed in d00f741. display_matches now reuses the existing vector and rewrites keys in-place only when --print is enabled, avoiding per-entry cloning in the common path.
overtrue
changed the title
Summary
This PR is a one-pass completion for the remaining command-option behavior gaps, with runtime fixes and e2e coverage added together.
Core Fixes
findoptions (from the previous commit in this PR):--printnow outputs full remote path (alias/bucket/key)--execnow executes per match with{}substitution--exec --jsonis now explicitly rejected with usage errormirror --parallel(previously effectively no-op):--parallelmust be> 0Test Completion (e2e)
Added and validated behavior-level integration tests for option semantics (not only help contract):
test_find_print_outputs_full_remote_pathtest_find_exec_runs_command_with_full_remote_pathtest_find_exec_rejects_json_outputtest_find_maxdepth_excludes_deeper_matchestest_mirror_remove_with_parallel_synchronizes_destinationtest_mirror_parallel_zero_returns_usage_errortest_tree_option_combination_filters_expected_nodestest_diff_diff_only_excludes_same_entriestest_share_rejects_expiration_over_seven_daysTotal option behavior tests in
option_behavior_operations: 15.CI / Smoke updates
Updated
integration.ymlsmoke list to include additional fast option-regression guards:option_behavior_operations::test_find_exec_rejects_json_outputoption_behavior_operations::test_mirror_parallel_zero_returns_usage_errorValidation
All required checks passed locally:
cargo fmt --all --checkcargo clippy --workspace -- -D warningscargo test --workspaceReal RustFS backend validation (
TEST_S3_ENDPOINT=http://localhost:9000):cargo test --package rustfs-cli --test integration --features integration option_behavior_operations:: -- --test-threads=1Scope