Skip to content

Conversation

@justus-camp-microsoft
Copy link
Contributor

#2743 added a macro to use an integrated xshell::Shell as part of rust flowey steps, but no way to enforce usage. This is problematic because future nodes could use xshell::cmd! and escape the nix-shell --pure wrapping that's coming.

This adds a lint as part of house-rules to catch usages and require an allow comment for any new future xshell::cmd! invocations. This required a refactor/removal of the PackageManager struct in install_dist_pkg to a more functional style that can keep the runtime around.

@justus-camp-microsoft justus-camp-microsoft requested a review from a team as a code owner February 10, 2026 23:28
Copilot AI review requested due to automatic review settings February 10, 2026 23:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an xtask fmt --house-rules lint to prevent Flowey code from directly using xshell::cmd! / xshell::Shell::new() (to avoid bypassing future nix-shell --pure wrapping), and updates/annotates existing exceptions accordingly.

Changes:

  • Introduce a new house-rules check that flags bare xshell::cmd! / xshell::Shell::new() under flowey/ unless suppressed with // xtask-fmt allow-xshell <reason>.
  • Wire the new lint into xtask house-rules with a --skip-flowey-xshell escape hatch.
  • Refactor install_dist_pkg to use flowey::shell_cmd! / rt.sh instead of owning a standalone xshell::Shell, and add suppressions for remaining legitimate xshell uses.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
xtask/src/tasks/fmt/house_rules/flowey_xshell.rs New house-rules lint that detects bare xshell usage in flowey/ and enforces justification-based suppressions.
xtask/src/tasks/fmt/house_rules.rs Registers and runs the new lint; adds a CLI flag to skip it.
flowey/flowey_lib_common/src/run_cargo_doc.rs Adds suppression annotation for an allowed xshell::cmd! usage in a utility context.
flowey/flowey_lib_common/src/install_dist_pkg.rs Refactors package-install logic to use Flowey’s runtime shell (flowey::shell_cmd!) instead of creating a new xshell::Shell.
flowey/flowey_lib_common/src/_util/wslpath.rs Annotates existing direct xshell usage with suppressions.
flowey/flowey_core/src/node.rs Annotates core infrastructure xshell usage (runtime shell creation + macro wrapper) with suppressions.
flowey/flowey_cli/src/cli/regen.rs Annotates CLI-specific xshell usage with suppressions.

Comment on lines +61 to +62
let no_existing = installed_packages.insert(package.to_owned());
assert!(no_existing);
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert!(no_existing) can panic when the package query returns multiple lines that collapse to the same key after stripping the :arch suffix (e.g., multi-arch installs or glob matches from dpkg-query -W). Instead of asserting uniqueness, consider either keeping the full package identifier (including arch) or simply ignoring duplicates when inserting into the set.

Suggested change
let no_existing = installed_packages.insert(package.to_owned());
assert!(no_existing);
// Ignore duplicates after normalizing package names (e.g., multi-arch variants).
let _ = installed_packages.insert(package.to_owned());

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant