Skip to content

Conversation

@justus-camp-microsoft
Copy link
Contributor

@justus-camp-microsoft justus-camp-microsoft commented Feb 10, 2026

#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 disallows xshell::Shell::new() and xshell::cmd! in our clippy.toml.

@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.
@smalis-msft
Copy link
Contributor

This may be better served by using clippy's disallowed lints rather than something custom, check out our clippy.toml.

@smalis-msft
Copy link
Contributor

It may need some centralization/cleanup in other crates too, but then you can stick an #[expect(disallowed_methods)] on the few construction sites and add it to the forbidden list for everywhere else.

@smalis-msft
Copy link
Contributor

How many of these instances can we centralize? Like can xtask gain one new fn new_xtask_shell or something that has the expects and that everyone else calls? Can flowey do something similar?

@github-actions
Copy link

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.

2 participants