Fix elevated command parsing

[shgew]

Fixes #421 by changing how the elevated command is converted fromsubproces::Exec to std::process::Command. It now uses shlex to correctly parse the command.

Sanity Checking

• I have updated the changelog as per my changes
• I have tested, and self-reviewed my code
• Style and consistency
  • I ran nix fmt to format my Nix code
  • I ran cargo fmt to format my Rust code
  • I have added appropriate documentation to new code
  • My changes are consistent with the rest of the codebase
• Correctness
  • I ran cargo clippy and fixed any new linter warnings.
• If new changes are particularly complex:
  • My code includes comments in particularly complex areas to explain the
    logic
  • I have documented the motive for those changes in the PR body or commit
    description.
• Tested on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin

---

Add a 👍 reaction to pull requests you find important.

Summary by CodeRabbit

• Bug Fixes
  • Corrected handling of spaces and quoted arguments in elevated commands; quotes and escapes are preserved so paths or flags containing spaces are handled correctly.
• Documentation
  • Clarified Unreleased notes formatting and added a Fixed entry describing the improved elevated command parsing.
• Chores
  • Added shlex dependency to project dependencies.

[coderabbitai]

Walkthrough

Replaces whitespace-based tokenization with shlex parsing when constructing elevated commands; adds shlex dependency; updates error handling and argument forwarding in command elevation; updates CHANGELOG with a Fixed entry describing correct handling of spaces and quoted arguments.

Changes

Cohort / File(s)	Summary
Documentation (Changelog) CHANGELOG.md	Added an Unreleased "Fixed" entry documenting use of shlex-based parsing for elevated commands and correct handling of spaces/quoted arguments; minor formatting tweaks.
Dependency Update Cargo.toml	Added dependency: shlex = "1.3.0".
Command Elevation Parsing src/commands.rs	Replaced whitespace splitting with shlex::split; on parse failure returns error "Failed to parse sudo command"; constructs std::process::Command from &parts[0] and forwards remaining tokens via iterator mapping to &str; tests updated to validate shlex parsing and allow sudo token anywhere in the command line.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant CommandBuilder
  participant Shlex as shlex::split
  participant Process as std::process::Command

  Caller->>CommandBuilder: build_elevated(cmdline)
  CommandBuilder->>Shlex: shlex::split(cmdline)
  alt parse succeeds
    Shlex-->>CommandBuilder: Vec<String> parts
    CommandBuilder->>Process: new(&parts[0])
    CommandBuilder->>Process: args(parts[1..].iter().map(|s| s.as_str()))
    Process-->>Caller: Command ready
  else parse fails
    Shlex-->>CommandBuilder: None / Err
    CommandBuilder-->>Caller: error("Failed to parse sudo command")
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title “Fix elevated command parsing” succinctly and accurately describes the main change of replacing whitespace‐based splitting with shlex based parsing for elevated commands, clearly reflecting the intent and scope of the changeset.
Linked Issues Check	✅ Passed	The changes correctly address the linked issue #421 by replacing naive whitespace splitting with shlex parsing, adding error handling, and updating tests to ensure elevated commands with spaces are handled properly, satisfying the bug fix requirements.
Out of Scope Changes Check	✅ Passed	All modifications in this pull request are directly related to fixing command parsing via shlex, updating tests and documentation, and necessary project upkeep such as formatting and linting, with no unrelated or extraneous changes detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2779c8b and a45c244.

📒 Files selected for processing (2)

• CHANGELOG.md (1 hunks)
• src/commands.rs (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• CHANGELOG.md
• src/commands.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Test NH on Darwin
• GitHub Check: Test NH on Linux
• GitHub Check: treewide-checks
• GitHub Check: Build NH on Darwin
• GitHub Check: Build NH on Linux

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

CHANGELOG.md (1)

20-21: Grammar/typo: “supports” and “column”.

• Change “now support” → “now supports”.
• Fix “coloumn” → “column”.

Apply this diff:

- - `nh os info` now support `--fields` to select which field(s) to display;
-  also add a per-generation "Closure Size" coloumn.
+ - `nh os info` now supports `--fields` to select which field(s) to display;
+  also add a per-generation "Closure Size" column.

src/commands.rs (2)

428-431: Non‑UTF‑8 args are lost.

Even if keeping shlex, building from Strings drops non‑UTF‑8. Prefer args_os() end‑to‑end.

---

973-978: Test is fine; small robustness nit.

Parsing via shlex is okay here, but this test still depends on the formatting of to_cmdline_lossy. If you adopt direct Command construction, consider asserting presence of “-A”/“env”/assignments via a helper that builds argv rather than relying on a pretty-printed line.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8bf3234 and 68181e3.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• CHANGELOG.md (1 hunks)
• Cargo.toml (1 hunks)
• src/commands.rs (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Test NH on Darwin
• GitHub Check: Test NH on Linux
• GitHub Check: treewide-checks
• GitHub Check: Build NH on Linux
• GitHub Check: Build NH on Darwin

🔇 Additional comments (2)

CHANGELOG.md (1)

24-30: Changelog entry reads well; aligns with the fix.

Accurately documents the switch to shlex-based parsing for elevated command construction.

Cargo.toml (1)

52-52: Do not move shlex to dev-dependencies — it's used at runtime.
ripgrep shows shlex:: calls in src/commands.rs (lines 417–419 and 974–976); keep shlex in [dependencies] or remove/replace those runtime usages.

Likely an incorrect or invalid review comment.

[NotAShelf]

I'm not exactly sure if I really want to introduce a crate for this? This should be doable with just the stdlib by parsing the command beforehand, but I'm talking based on what I remember from the top of my head.

[shgew]

I'm not exactly sure if I really want to introduce a crate for this? This should be doable with just the stdlib by parsing the command beforehand, but I'm talking based on what I remember from the top of my head.

Yeah, it would be nice to avoid it. I will check if there's something.

However, should more effort be put into this approach? I think it's best to focus on trying to decouple the logic of building a command from both process::Command and subprocess::Exec to avoid parsing altogether, as I have mentioned in the issue.

[shgew]

Update: couldn't find anything in the standard library to avoid adding a crate. Please let me know if I missed something.