fix: convert rtk ls from reimplementation to native proxy#38
Conversation
BREAKING CHANGE: Removes --depth, --format (tree/flat/json) flags Before: `rtk ls` was a reimplementation using WalkBuilder that didn't support native ls flags like -l, -a, -h, -R. After: `rtk ls` proxies to native ls, supporting all flags: - `rtk ls -la` works - `rtk ls -alh` works - `rtk ls -R src/` works - Default: `-la` when no args Token optimization: filters "total X" line from output. This aligns with rtk philosophy: proxy and filter, don't reimplement. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Converts rtk ls from a custom directory traversal implementation to a native ls proxy while applying minimal output filtering for token savings.
Changes:
- Replaces
rtk ls’s custom traversal/formatting with a directlssubprocess call. - Updates CLI parsing for
rtk lsto pass through arbitrarylsflags/args and default to-lawhen none are provided. - Adds unit tests for the
lsoutput filter (removingtotal Xlines).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main.rs | Updates rtk ls CLI interface and wires the subcommand to the new ls::run(&args, verbose) signature. |
| src/ls.rs | Implements native ls proxy execution, filters output, adds verbose reduction stats, and introduces filter-focused tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn run(args: &[String], verbose: u8) -> Result<()> { | ||
| let mut cmd = Command::new("ls"); | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s.to_lowercase().as_str() { | ||
| "tree" => Ok(OutputFormat::Tree), | ||
| "flat" => Ok(OutputFormat::Flat), | ||
| "json" => Ok(OutputFormat::Json), | ||
| _ => Err(format!("Unknown format: {}", s)), | ||
| // Default to -la if no args (common case for LLM context) | ||
| if args.is_empty() { | ||
| cmd.args(["-la"]); |
There was a problem hiding this comment.
Command::new("ls") will fail on Windows, but the project docs explicitly claim cross-platform support (e.g., ARCHITECTURE.md notes macOS/Linux/Windows). Add a Windows implementation (e.g., proxy to cmd /C dir or PowerShell Get-ChildItem, with equivalent filtering), or gracefully fall back when ls isn't available and return a clear error.
| let output = cmd.output().context("Failed to run ls")?; | ||
|
|
||
| // Skip the root itself | ||
| if entry_path == path { | ||
| continue; | ||
| } | ||
|
|
||
| let depth = entry_path.components().count() - base_depth; | ||
| let name = entry_path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().to_string()) | ||
| .unwrap_or_default(); | ||
|
|
||
| let is_dir = entry.file_type().map(|ft| ft.is_dir()).unwrap_or(false); | ||
|
|
||
| entries.push(DirEntry { | ||
| name, | ||
| path: entry_path.display().to_string(), | ||
| is_dir, | ||
| depth, | ||
| }); | ||
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| eprint!("{}", stderr); | ||
| std::process::exit(output.status.code().unwrap_or(1)); | ||
| } |
There was a problem hiding this comment.
When ls exits non-zero, this returns early and only prints stderr, discarding any stdout that ls may have produced (e.g., ls existing missing prints valid listings + an error). To preserve native behavior, print the (filtered) stdout even on failure, print stderr as well, and then exit with the same status code.
| print!("{}", filtered); | ||
| tracking::track("ls", "rtk ls", &raw, &filtered); | ||
|
|
There was a problem hiding this comment.
tracking::track should include the actual command+args being proxied (and the effective default when no args are provided) so tracking remains accurate/consistent with other commands (e.g., src/cargo_cmd.rs:41-46 uses formatted command strings). Right now it always records original_cmd as "ls" and omits args, which will skew token-savings analytics.
The previous PR rtk-ai#38 contained BREAKING CHANGE in the commit message, which triggered an unintended major version bump to 1.0.0. This commit: - Updates manifest to 0.7.1 - Updates Cargo.toml to 0.7.1 After merging, close PR rtk-ai#39 without merging to skip the 1.0.0 release. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fix: convert rtk ls from reimplementation to native proxy
The previous PR rtk-ai#38 contained BREAKING CHANGE in the commit message, which triggered an unintended major version bump to 1.0.0. This commit: - Updates manifest to 0.7.1 - Updates Cargo.toml to 0.7.1 After merging, close PR rtk-ai#39 without merging to skip the 1.0.0 release. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
rtk lsfrom a WalkBuilder reimplementation to a nativelsproxy-l,-a,-h,-R, etc.-lawhen no args providedProblem
rtk ls -alfailed because-lwas not a recognized flag. The previous implementation reimplemented directory traversal instead of proxying to native ls, which is inconsistent with rtk's philosophy (proxy and filter, don't reimplement).Breaking Changes
Removes the following flags:
--depth/-d--format(tree/flat/json)--all/-a(replaced by native-a)These are replaced by native ls flags.
Test Plan
rtk ls -laworksrtk ls -alhworksrtk lsdefaults to-lartk ls -R src/works (recursive)🤖 Generated with Claude Code