fix(package-registry): strip ansi codes from outputs #11
Conversation
📝 WalkthroughWalkthroughThis PR introduces centralized error handling with Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant mntn as mntn CLI
participant Git as Git
participant Display as Output Display
User->>mntn: mntn sync --diff
mntn->>Git: git diff (unstaged changes)
Git-->>mntn: Diff output (bytes)
mntn->>Display: Print unstaged diff block
Display-->>User: Show formatted diff
mntn->>Git: git diff --cached (staged changes)<br/>or fallback for older git
Git-->>mntn: Diff output (bytes)
mntn->>Display: Print staged diff block
Display-->>User: Show formatted diff
mntn->>Git: Proceed with sync if not --diff mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
3-3:⚠️ Potential issue | 🟡 MinorPackage version not bumped to match CHANGELOG.
Cargo.tomlstill hasversion = "2.2.0"whileCHANGELOG.mddocuments av2.3.0release. This should be updated to"2.3.0"before release.Proposed fix
-version = "2.2.0" +version = "2.3.0"
🤖 Fix all issues with AI agents
In `@src/tasks/sync.rs`:
- Around line 256-271: run_staged_diff_with_fallback currently swallows all
errors when run_git_diff_bytes fails and always retries with "--cached"; change
it to only fallback when the error indicates an unsupported/unrecognized flag by
inspecting the error (e.g., stderr text or a specific error kind) returned from
run_git_diff_bytes, otherwise propagate the error; if you keep the fallback
path, log a warning (via your existing logger) that you are retrying with
"--cached" and include the original error details; update references in the
function run_staged_diff_with_fallback and use the error value returned by
run_git_diff_bytes to decide whether to retry.
- Around line 273-292: The function run_git_diff_bytes duplicates error handling
from run_cmd_impl and returns a std::io::Error-based error; instead, add a
run_cmd_bytes variant in src/utils/system.rs that mirrors run_cmd_impl but
returns Result<Vec<u8>, AppError> (reusing AppError::CommandFailure for non-zero
exit and UTF-8 stderr handling), then replace the body of run_git_diff_bytes to
call that new run_cmd_bytes helper so all command failures use
AppError::CommandFailure consistently.
🧹 Nitpick comments (2)
src/tasks/backup.rs (1)
174-174: Consider caching the compiledRegexinstrip_ansi_codes.The underlying
strip_ansi_codes(insrc/utils/system.rs) recompiles the regex on every invocation. Since this runs inside apar_iter, it compiles once per package manager — tolerable for now but aLazyLock/OnceLockstatic would be cleaner.src/tasks/sync.rs (1)
294-295: Redundant inner import.
std::io::Writeis already imported at Line 9 (use std::io::Write;). The inneruseon Line 295 is unnecessary.♻️ Remove redundant import
fn print_diff_block(title: &str, content: &[u8]) -> anyhow::Result<()> { - use std::io::Write; - println!("{}", title);
| fn run_staged_diff_with_fallback(staged_args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> { | ||
| match run_git_diff_bytes(staged_args, mntn_dir) { | ||
| Ok(output) => Ok(output), | ||
| Err(_) => { | ||
| let mut cached_args = Vec::with_capacity(staged_args.len()); | ||
| for arg in staged_args { | ||
| if *arg == "--staged" { | ||
| cached_args.push("--cached"); | ||
| } else { | ||
| cached_args.push(*arg); | ||
| } | ||
| } | ||
| run_git_diff_bytes(&cached_args, mntn_dir) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
run_staged_diff_with_fallback swallows all errors, not just unsupported-flag errors.
The Err(_) match arm catches every failure (permission denied, corrupt repo, disk errors, etc.) and silently retries with --cached. This could mask real problems. Consider matching only on errors likely caused by an unrecognized --staged flag, or at least logging a warning when falling back.
🛡️ Suggested approach: inspect stderr before falling back
fn run_staged_diff_with_fallback(staged_args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> {
match run_git_diff_bytes(staged_args, mntn_dir) {
Ok(output) => Ok(output),
- Err(_) => {
+ Err(e) => {
+ // Only fall back if the error looks like an unrecognized option
+ let msg = e.to_string();
+ if !msg.contains("unknown option") && !msg.contains("unrecognized") {
+ return Err(e);
+ }
let mut cached_args = Vec::with_capacity(staged_args.len());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_staged_diff_with_fallback(staged_args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> { | |
| match run_git_diff_bytes(staged_args, mntn_dir) { | |
| Ok(output) => Ok(output), | |
| Err(_) => { | |
| let mut cached_args = Vec::with_capacity(staged_args.len()); | |
| for arg in staged_args { | |
| if *arg == "--staged" { | |
| cached_args.push("--cached"); | |
| } else { | |
| cached_args.push(*arg); | |
| } | |
| } | |
| run_git_diff_bytes(&cached_args, mntn_dir) | |
| } | |
| } | |
| } | |
| fn run_staged_diff_with_fallback(staged_args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> { | |
| match run_git_diff_bytes(staged_args, mntn_dir) { | |
| Ok(output) => Ok(output), | |
| Err(e) => { | |
| // Only fall back if the error looks like an unrecognized option | |
| let msg = e.to_string(); | |
| if !msg.contains("unknown option") && !msg.contains("unrecognized") { | |
| return Err(e); | |
| } | |
| let mut cached_args = Vec::with_capacity(staged_args.len()); | |
| for arg in staged_args { | |
| if *arg == "--staged" { | |
| cached_args.push("--cached"); | |
| } else { | |
| cached_args.push(*arg); | |
| } | |
| } | |
| run_git_diff_bytes(&cached_args, mntn_dir) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/tasks/sync.rs` around lines 256 - 271, run_staged_diff_with_fallback
currently swallows all errors when run_git_diff_bytes fails and always retries
with "--cached"; change it to only fallback when the error indicates an
unsupported/unrecognized flag by inspecting the error (e.g., stderr text or a
specific error kind) returned from run_git_diff_bytes, otherwise propagate the
error; if you keep the fallback path, log a warning (via your existing logger)
that you are retrying with "--cached" and include the original error details;
update references in the function run_staged_diff_with_fallback and use the
error value returned by run_git_diff_bytes to decide whether to retry.
| fn run_git_diff_bytes(args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> { | ||
| let output = Command::new("git") | ||
| .args(args) | ||
| .current_dir(mntn_dir) | ||
| .output()?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr_len = output.stderr.len(); | ||
| let stderr_msg = String::from_utf8(output.stderr) | ||
| .unwrap_or_else(|_| format!("<non-UTF-8 stderr data: {} bytes>", stderr_len)); | ||
| return Err(std::io::Error::other(format!( | ||
| "Command 'git' failed with status {:?}: {}", | ||
| output.status.code(), | ||
| stderr_msg | ||
| )) | ||
| .into()); | ||
| } | ||
|
|
||
| Ok(output.stdout) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicated error handling and inconsistent error type.
run_git_diff_bytes reimplements the error-handling pattern from run_cmd_impl in src/utils/system.rs (Lines 36–60), but uses std::io::Error::other(...) instead of the new AppError::CommandFailure introduced in this PR. This defeats the purpose of centralized error handling.
Consider adding a run_cmd_bytes variant to src/utils/system.rs that returns Result<Vec<u8>> and reuses AppError::CommandFailure, then call it from here.
♻️ Quick fix: at minimum, use AppError::CommandFailure for consistency
if !output.status.success() {
let stderr_len = output.stderr.len();
let stderr_msg = String::from_utf8(output.stderr)
.unwrap_or_else(|_| format!("<non-UTF-8 stderr data: {} bytes>", stderr_len));
- return Err(std::io::Error::other(format!(
- "Command 'git' failed with status {:?}: {}",
- output.status.code(),
- stderr_msg
- ))
- .into());
+ return Err(crate::error::AppError::CommandFailure {
+ cmd: "git".to_string(),
+ status: output.status.code(),
+ stderr: stderr_msg,
+ }.into());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_git_diff_bytes(args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> { | |
| let output = Command::new("git") | |
| .args(args) | |
| .current_dir(mntn_dir) | |
| .output()?; | |
| if !output.status.success() { | |
| let stderr_len = output.stderr.len(); | |
| let stderr_msg = String::from_utf8(output.stderr) | |
| .unwrap_or_else(|_| format!("<non-UTF-8 stderr data: {} bytes>", stderr_len)); | |
| return Err(std::io::Error::other(format!( | |
| "Command 'git' failed with status {:?}: {}", | |
| output.status.code(), | |
| stderr_msg | |
| )) | |
| .into()); | |
| } | |
| Ok(output.stdout) | |
| } | |
| fn run_git_diff_bytes(args: &[&str], mntn_dir: &Path) -> anyhow::Result<Vec<u8>> { | |
| let output = Command::new("git") | |
| .args(args) | |
| .current_dir(mntn_dir) | |
| .output()?; | |
| if !output.status.success() { | |
| let stderr_len = output.stderr.len(); | |
| let stderr_msg = String::from_utf8(output.stderr) | |
| .unwrap_or_else(|_| format!("<non-UTF-8 stderr data: {} bytes>", stderr_len)); | |
| return Err(crate::error::AppError::CommandFailure { | |
| cmd: "git".to_string(), | |
| status: output.status.code(), | |
| stderr: stderr_msg, | |
| }.into()); | |
| } | |
| Ok(output.stdout) | |
| } |
🤖 Prompt for AI Agents
In `@src/tasks/sync.rs` around lines 273 - 292, The function run_git_diff_bytes
duplicates error handling from run_cmd_impl and returns a std::io::Error-based
error; instead, add a run_cmd_bytes variant in src/utils/system.rs that mirrors
run_cmd_impl but returns Result<Vec<u8>, AppError> (reusing
AppError::CommandFailure for non-zero exit and UTF-8 stderr handling), then
replace the body of run_git_diff_bytes to call that new run_cmd_bytes helper so
all command failures use AppError::CommandFailure consistently.
There was a problem hiding this comment.
Pull request overview
This PR improves output stability/cleanliness and modernizes error handling across the CLI by introducing a central AppError and switching task execution to anyhow::Result, while also adding a mntn sync --diff preview mode.
Changes:
- Add ANSI-stripping utility and apply it to package manager backup outputs.
- Introduce
AppError+Result<T>and migrate utility/registry APIs to use it; migrate tasks toanyhow::Result<()>. - Add
mntn sync --diff(and documentation/changelog updates) to show unstaged + staged git diffs.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/system.rs | Adds strip_ansi_codes, migrates command helpers to AppError/Result<T>, updates related tests. |
| src/tasks/backup.rs | Strips ANSI escape codes from captured package manager outputs before writing to files. |
| src/error.rs | Introduces centralized application error enum (AppError) and Result<T> alias. |
| src/main.rs | Wires the new error module into the crate module tree. |
| src/registry.rs | Converts registry load/save and enable/disable to use AppError/Result<T> (typed errors). |
| src/tasks/core.rs | Updates Task::execute() to return anyhow::Result<()>. |
| src/tasks/sync.rs | Adds --diff support and implements combined staged/unstaged diff output with fallback behavior. |
| src/cli.rs | Adds mntn sync --diff CLI flag. |
| README.md | Documents mntn sync --status and mntn sync --diff. |
| CHANGELOG.md | Adds v2.3.0 notes for --diff and ANSI stripping fix. |
| Cargo.toml | Adds anyhow and thiserror dependencies. |
| Cargo.lock | Updates lockfile for new dependencies and version bumps. |
| src/encryption.rs | Migrates encryption functions to anyhow::Result and adds context-rich errors. |
| src/tasks/use_profile.rs | Switches to anyhow::Result and uses bail! for missing profile. |
| src/tasks/validate.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/restore.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/purge.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/package_registry.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/migrate.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/install.rs | Switches task execution signature to anyhow::Result across install helpers. |
| src/tasks/encrypted_configs_registry.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/delete.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/configs_registry.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/clean.rs | Switches task execution signature to anyhow::Result. |
| src/tasks/biometric_sudo.rs | Switches task execution signature to anyhow::Result and converts error propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(_) => { | ||
| let mut cached_args = Vec::with_capacity(staged_args.len()); | ||
| for arg in staged_args { | ||
| if *arg == "--staged" { | ||
| cached_args.push("--cached"); | ||
| } else { | ||
| cached_args.push(*arg); | ||
| } | ||
| } | ||
| run_git_diff_bytes(&cached_args, mntn_dir) |
There was a problem hiding this comment.
run_staged_diff_with_fallback falls back to --cached on any error from git diff --staged, which can hide real failures and drops the original error details. Consider only retrying when stderr/exit status indicates --staged is unsupported, and otherwise returning the original error.
| Err(_) => { | |
| let mut cached_args = Vec::with_capacity(staged_args.len()); | |
| for arg in staged_args { | |
| if *arg == "--staged" { | |
| cached_args.push("--cached"); | |
| } else { | |
| cached_args.push(*arg); | |
| } | |
| } | |
| run_git_diff_bytes(&cached_args, mntn_dir) | |
| Err(e) => { | |
| let msg = e.to_string(); | |
| let staged_unsupported = | |
| (msg.contains("unknown option") || msg.contains("unrecognized option")) | |
| && msg.contains("--staged"); | |
| if staged_unsupported { | |
| let mut cached_args = Vec::with_capacity(staged_args.len()); | |
| for arg in staged_args { | |
| if *arg == "--staged" { | |
| cached_args.push("--cached"); | |
| } else { | |
| cached_args.push(*arg); | |
| } | |
| } | |
| run_git_diff_bytes(&cached_args, mntn_dir) | |
| } else { | |
| Err(e) | |
| } |
| /// - Captures stdout and returns it if the command succeeds and output is valid UTF-8. | ||
| /// - Returns an `io::Error` if the command cannot be run. | ||
| /// - Returns an error if the command exits with non-zero status. | ||
| /// - Returns a `FromUtf8Error` if stdout isn't valid UTF-8. | ||
| pub fn run_cmd(cmd: &str, args: &[&str]) -> Result<String, Box<dyn std::error::Error>> { | ||
| pub fn run_cmd(cmd: &str, args: &[&str]) -> Result<String> { |
There was a problem hiding this comment.
The rustdoc above run_cmd still describes returning io::Error / FromUtf8Error, but the function now returns crate::error::Result<String> (an AppError). Update the docs to reflect the new error type/variants so callers know what to handle.
| pub fn strip_ansi_codes(input: &str) -> String { | ||
| // Matches CSI (Control Sequence Introducer) ANSI escape codes. | ||
| let re = Regex::new(r"\x1B\[[0-?]*[ -/]*[@-~]").unwrap(); | ||
| re.replace_all(input, "").to_string() | ||
| } |
There was a problem hiding this comment.
strip_ansi_codes recompiles the regex on every call, which is avoidable overhead (and this runs in a loop when backing up multiple package managers). Consider compiling the regex once (e.g., static via std::sync::LazyLock/once_cell) and reusing it.
| fn test_get_current_git_branch_returns_branch_name() { | ||
| if run_cmd("git", &["--version"]).is_err() { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This test exits early (silently passes) when git --version fails and later repeats that pattern for other git commands. That can mask real regressions on systems where git exists but one step fails; prefer skipping only when git is unavailable, and otherwise asserting so failures are visible.
1 participant
Summary by CodeRabbit
New Features
mntn sync --diffcommand to display git diffs before syncing, with fallback support for older git versions.Bug Fixes
Documentation
mntn sync --statusandmntn sync --diffcommands in the Git Integration and Sync Guide.