feat: cargo build/test/clippy with compact output#29
feat: cargo build/test/clippy with compact output#29pszymkowiak merged 2 commits intortk-ai:masterfrom
Conversation
- Make compact_diff pub(crate) in git.rs for cross-module use - Extract filter_json_string() from json_cmd.rs for reuse - Add ok_confirmation() to utils.rs for write operation confirmations - Add detect_package_manager() and package_manager_exec() to utils.rs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- cargo build: strip Compiling/Downloading lines, show errors + summary - cargo test: show failures only + summary line - cargo clippy: group warnings by lint rule with locations New module: src/cargo_cmd.rs with 6 unit tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new rtk cargo command group that runs cargo build/test/clippy and post-processes their output into a more compact, LLM-friendly format.
Changes:
- Introduce
src/cargo_cmd.rswithCargoCommandplus output filters for build/test/clippy (with unit tests). - Wire
rtk cargo build|test|clippyinto the CLI insrc/main.rs. - Expose/extend shared helpers used across commands (JSON string filtering, compact diff visibility, confirmation + package manager helpers).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cargo_cmd.rs | New cargo subcommand runner + output compaction filters and unit tests. |
| src/main.rs | Adds Commands::Cargo and dispatch to cargo_cmd::run. |
| src/utils.rs | Adds ok_confirmation, detect_package_manager, package_manager_exec and tests. |
| src/json_cmd.rs | Extracts filter_json_string() and updates run() to use it. |
| src/git.rs | Makes compact_diff pub(crate) for cross-module reuse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if in_failure_section { | ||
| if line.starts_with("test result:") { | ||
| in_failure_section = false; | ||
| summary_lines.push(line.to_string()); |
There was a problem hiding this comment.
In filter_cargo_test, the test result: line is pushed into summary_lines inside the in_failure_section branch, and then it can be pushed again by the later if !in_failure_section && line.starts_with("test result:") block in the same loop iteration. This duplicates the summary line for failing test runs. Consider continue-ing after handling test result: in the failure-section branch, or refactor so the summary capture happens in exactly one place.
| summary_lines.push(line.to_string()); | |
| summary_lines.push(line.to_string()); | |
| continue; |
| .with_context(|| format!("Failed to parse JSON: {}", file.display()))?; | ||
|
|
||
| let schema = extract_schema(&value, 0, max_depth); | ||
| let schema = filter_json_string(&content, max_depth)?; |
There was a problem hiding this comment.
json_cmd::run used to include the filename in the JSON parse error context (e.g. "Failed to parse JSON: "). After switching to filter_json_string, parse failures lose that file context. Consider wrapping the filter_json_string call with with_context(|| format!("Failed to parse JSON: {}", file.display())) so file-based usage remains debuggable while keeping the generic error inside filter_json_string for piped input.
| let schema = filter_json_string(&content, max_depth)?; | |
| let schema = filter_json_string(&content, max_depth) | |
| .with_context(|| format!("Failed to parse JSON: {}", file.display()))?; |
| #[test] | ||
| fn test_detect_package_manager_default() { | ||
| // In the test environment (rtk repo), there's no JS lockfile | ||
| // so it should default to "npm" | ||
| let pm = detect_package_manager(); | ||
| assert!(["pnpm", "yarn", "npm"].contains(&pm)); |
There was a problem hiding this comment.
test_detect_package_manager_default doesn’t actually verify the intended default behavior: it asserts the return value is one of ["pnpm", "yarn", "npm"], which will always pass regardless of lockfiles. To make this test meaningful/stable, run it in a temp directory without lockfiles (e.g., using tempfile + set_current_dir) and assert it returns "npm".
| /// Format a confirmation message: "ok <action> <detail>" | ||
| /// Used for write operations (merge, create, comment, edit, etc.) | ||
| /// |
There was a problem hiding this comment.
PR description lists only src/cargo_cmd.rs and src/main.rs as changed, but this PR also modifies src/utils.rs, src/json_cmd.rs, and src/git.rs in the diff shown here. If this is due to the stacked PR dependency on #28, it would help to explicitly call that out (or rebase onto the merged base PR) so reviewers can distinguish the incremental changes from the shared-infra changes.
Summary
rtk cargo build: strip Compiling/Downloading lines, show errors + summaryrtk cargo test: show failures only + summary linertk cargo clippy: group warnings by lint rule with locationsFiles changed
src/cargo_cmd.rs— new module withCargoCommandenum + 3 filter functionssrc/main.rs—Commands::Cargo,CargoCommandsenum, match armsTest plan
cargo buildcompiles cleancargo clippy— no new warningsDependencies
Depends on #28 (shared infrastructure). Stacked PR series: PR 2/6
🤖 Generated with Claude Code