From 7a735d858530b004ce5cc6b1bce5ca17d1b2a827 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 2 Aug 2026 10:55:38 +0200 Subject: [PATCH 1/7] Enforce the environment mandate with clippy disallowed-methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md forbids ambient environment access, but nothing stopped the next contributor writing `std::env::var`. #494 proposed a dylint rule or a CI grep gate; Clippy's `disallowed-methods` is a better fit. It runs inside `make lint`, which already gates every commit. It resolves paths rather than matching text, so it distinguishes `std::env::var` from `Command::env`, from a re-exported alias, and from the string appearing in a comment. And the reason string surfaces in the diagnostic, so a contributor who trips it is told what to do instead: warning: use of a disallowed method `std::env::var` --> src/hasher.rs:67:47 = note: inject an environment reader Every existing site is annotated with `#[expect]` rather than `allow`. That is the load-bearing choice: an expectation goes unfulfilled — and warns — once the site stops tripping the lint, so a migrated file fails the gate until its annotation is removed. The backlog cannot rot silently, which `allow` would have permitted. Three dispositions, each with a reason naming why: composition roots in `src/` keep permanent site-level expectations naming the seam they supply; build scripts and artefact discovery keep permanent module-level ones, since they read what Cargo reports and have no seam to inject; and pending test files carry module-level expectations naming #492 or #493, removed as each migrates. `test_support` is excluded from the workspace, so it carries its own copy of the list. Verified by deliberate violation: `make lint` exits 2 with an unannotated `std::env::var` present and 0 once removed. Closes #504. Refs #494, #496. Co-Authored-By: Claude Opus 5 (1M context) --- build.rs | 5 +++ clippy.toml | 15 +++++++ docs/developers-guide.md | 47 +++++++++++++++++++++ src/cli/discovery.rs | 4 ++ src/locale_resolution.rs | 4 ++ src/manifest/mod.rs | 4 ++ src/output_mode.rs | 4 ++ src/output_prefs.rs | 8 ++++ src/runner/process/ninja_program.rs | 4 ++ src/stdlib/path/path_utils.rs | 8 ++++ src/stdlib/which/env.rs | 4 ++ src/stdlib/which/lookup/workspace/mod.rs | 4 ++ test_support/clippy.toml | 14 ++++++ tests/bdd/helpers/env_mutation.rs | 5 +++ tests/bdd/steps/conditional_manifest.rs | 5 +++ tests/bdd/steps/fs.rs | 5 +++ tests/bdd/steps/manifest/mod.rs | 5 +++ tests/bdd/steps/manifest_command_helpers.rs | 5 +++ tests/bdd/steps/stdlib/workspace.rs | 5 +++ tests/documentation_examples_e2e_tests.rs | 4 ++ tests/env_path_tests.rs | 5 +++ tests/env_restore_tests.rs | 5 +++ tests/kani_cfg_ui_tests.rs | 5 +++ tests/ninja_env_tests.rs | 5 +++ tests/packaging_smoke_tests.rs | 5 +++ tests/release_help/mod.rs | 5 +++ 26 files changed, 189 insertions(+) create mode 100644 test_support/clippy.toml diff --git a/build.rs b/build.rs index 8c8801471..4b6b1d550 100644 --- a/build.rs +++ b/build.rs @@ -1,3 +1,8 @@ +#![expect( + clippy::disallowed_methods, + reason = "build scripts read CARGO_* and OUT_DIR from the environment Cargo provides; there is no seam to inject and no test to isolate" +)] + //! Build script for Netsuke. //! //! This script performs two main tasks: diff --git a/clippy.toml b/clippy.toml index effc60402..bbe70becd 100644 --- a/clippy.toml +++ b/clippy.toml @@ -5,3 +5,18 @@ too-many-lines-threshold = 70 # default is 100 excessive-nesting-threshold = 4 # default is off allow-expect-in-tests = true + +# Enforce the AGENTS.md environment mandate. The reason strings surface in the +# diagnostic, so a contributor who trips one is told what to do instead. +# +# Sanctioned sites carry `#[expect(clippy::disallowed_methods, reason = "..")]` +# rather than `allow`, so the expectation goes unfulfilled — and warns — once the +# site is migrated. The backlog removes itself instead of rotting. +disallowed-methods = [ + { path = "std::env::var", reason = "inject an environment reader" }, + { path = "std::env::var_os", reason = "inject an environment reader" }, + { path = "std::env::vars", reason = "inject an environment reader" }, + { path = "std::env::vars_os", reason = "inject an environment reader" }, + { path = "std::env::set_var", reason = "use a stub environment in tests" }, + { path = "std::env::remove_var", reason = "use a stub environment in tests" }, +] diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 39507b852..0c0865ddd 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1408,6 +1408,53 @@ still safe to use. See the [locale-pinned snapshot tests](snapshot-testing-in-netsuke-using-insta.md#locale-pinned-snapshot-tests) section for the fixture's intended usage. +### Enforcing the environment mandate + +`clippy.toml` disallows the six process-environment entry points, so +`make lint` rejects a new one: + +```toml +disallowed-methods = [ + { path = "std::env::var", reason = "inject an environment reader" }, + { path = "std::env::set_var", reason = "use a stub environment in tests" }, + # ... var_os, vars, vars_os, remove_var +] +``` + +The reason string appears in the diagnostic, so a contributor who trips the +lint is told what to do instead, not merely that they may not. `test_support` +is excluded from the workspace, so it carries its own copy of the list. + +#### Annotating a sanctioned site + +Use `#[expect]`, never `allow`: + +```rust +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the read_env seam" +)] +pub fn resolve(no_emoji: Option) -> OutputPrefs { + resolve_with(no_emoji, |key| env::var(key).ok()) +} +``` + +`expect` becomes *unfulfilled* — and warns — once the site stops tripping the +lint. A migrated file therefore fails the gate until its annotation is removed, +so the backlog cannot rot silently. `allow` would go stale invisibly. + +Three dispositions are in use: + +- **Composition roots** in `src/` keep a permanent site-level expectation naming + the seam they supply. These are the sanctioned ambient boundary. +- **Build scripts and artefact discovery** keep a permanent module-level + expectation: they read what Cargo reports, and there is no seam to inject. +- **Pending migrations** in `tests/` carry a module-level expectation naming the + tracking issue, removed as each file migrates. + +Scope an expectation as tightly as the site allows — a function where one call +is involved, a module only where the whole file is pending migration. + ### `EnvLock` `test_support::env_lock::EnvLock` is a global mutex that serializes all diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index d2be80145..dd118d1d3 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -44,6 +44,10 @@ pub trait EnvProvider { #[derive(Debug, Default, Clone, Copy)] pub struct StdEnvProvider; +#[expect( + clippy::disallowed_methods, + reason = "composition root: StdEnvProvider is the process-backed adapter behind the EnvProvider seam" +)] impl EnvProvider for StdEnvProvider { fn get(&self, key: &str) -> Option { std::env::var_os(key) diff --git a/src/locale_resolution.rs b/src/locale_resolution.rs index 74083f530..347a3c62a 100644 --- a/src/locale_resolution.rs +++ b/src/locale_resolution.rs @@ -25,6 +25,10 @@ pub trait EnvProvider { #[derive(Debug, Default, Copy, Clone)] pub struct SystemEnv; +#[expect( + clippy::disallowed_methods, + reason = "composition root: SystemEnv is the process-backed adapter behind the EnvProvider seam" +)] impl EnvProvider for SystemEnv { fn var(&self, key: &str) -> Option { std::env::var(key).ok() diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index 4dca09bee..e8ab81981 100644 --- a/src/manifest/mod.rs +++ b/src/manifest/mod.rs @@ -81,6 +81,10 @@ pub enum ManifestLoadStage { /// assert_eq!(env("FOO").unwrap(), "bar"); /// // guard restores prior value on drop /// ``` +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the env() Jinja helper" +)] fn env_var(name: &str) -> std::result::Result { match std::env::var(name) { Ok(val) => Ok(val), diff --git a/src/output_mode.rs b/src/output_mode.rs index 2b22d7362..ff2618f84 100644 --- a/src/output_mode.rs +++ b/src/output_mode.rs @@ -49,6 +49,10 @@ impl OutputMode { /// assert_eq!(resolve(Some(false), None), OutputMode::Standard); /// ``` #[must_use] +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the read_env seam" +)] pub fn resolve(explicit: Option, colour_policy: Option) -> OutputMode { resolve_with(explicit, colour_policy, |key| env::var(key).ok()) } diff --git a/src/output_prefs.rs b/src/output_prefs.rs index dde7e8401..9d9650749 100644 --- a/src/output_prefs.rs +++ b/src/output_prefs.rs @@ -186,6 +186,10 @@ impl OutputPrefs { /// assert!(!prefs.emoji_allowed()); /// ``` #[must_use] +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the read_env seam" +)] pub fn resolve_from_theme(theme: Option, context: ThemeContext) -> OutputPrefs { resolve_from_theme_with(theme, context, |key| env::var(key).ok()) } @@ -227,6 +231,10 @@ where /// assert!(resolve_with(Some(false), |_| None).emoji_allowed()); /// ``` #[must_use] +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the read_env seam" +)] pub fn resolve(no_emoji: Option) -> OutputPrefs { resolve_with(no_emoji, |key| env::var(key).ok()) } diff --git a/src/runner/process/ninja_program.rs b/src/runner/process/ninja_program.rs index 606730ee3..aeb9ca4e0 100644 --- a/src/runner/process/ninja_program.rs +++ b/src/runner/process/ninja_program.rs @@ -64,6 +64,10 @@ where /// Resolve the configured Ninja executable as a UTF-8 path. #[must_use] +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the ninja program resolver seam" +)] pub fn resolve_ninja_program_utf8() -> Utf8PathBuf { resolve_ninja_program_utf8_with(|key| env::var_os(key)) } diff --git a/src/stdlib/path/path_utils.rs b/src/stdlib/path/path_utils.rs index 0e9a0f296..ac90cfbce 100644 --- a/src/stdlib/path/path_utils.rs +++ b/src/stdlib/path/path_utils.rs @@ -152,6 +152,10 @@ fn current_dir_utf8() -> Result { } #[cfg(windows)] +#[expect( + clippy::disallowed_methods, + reason = "composition root: home resolution is the path stdlib's ambient boundary; the injected ladders are tracked in the environment meta issue" +)] fn home_from_env() -> Option { env::var("HOME") .or_else(|_| env::var("USERPROFILE")) @@ -165,6 +169,10 @@ fn home_from_env() -> Option { } #[cfg(not(windows))] +#[expect( + clippy::disallowed_methods, + reason = "composition root: home resolution is the path stdlib's ambient boundary; the injected ladders are tracked in the environment meta issue" +)] fn home_from_env() -> Option { env::var("HOME").or_else(|_| env::var("USERPROFILE")).ok() } diff --git a/src/stdlib/which/env.rs b/src/stdlib/which/env.rs index 2a76b7d65..484c157fe 100644 --- a/src/stdlib/which/env.rs +++ b/src/stdlib/which/env.rs @@ -21,6 +21,10 @@ pub(super) struct EnvSnapshot { } impl EnvSnapshot { + #[expect( + clippy::disallowed_methods, + reason = "composition root: PATH and PATHEXT capture is the which resolver's ambient boundary; injection is tracked in the environment meta issue" + )] pub(super) fn capture( cwd_override: Option<&Utf8Path>, path_override: Option<&OsStr>, diff --git a/src/stdlib/which/lookup/workspace/mod.rs b/src/stdlib/which/lookup/workspace/mod.rs index 055214789..9e3129840 100644 --- a/src/stdlib/which/lookup/workspace/mod.rs +++ b/src/stdlib/which/lookup/workspace/mod.rs @@ -106,6 +106,10 @@ pub(super) fn should_visit_entry(entry: &walkdir::DirEntry, skip_dirs: &Workspac !skip_dirs.contains(&name) } +#[expect( + clippy::disallowed_methods, + reason = "composition root: supplies the process environment to the workspace fallback seam" +)] fn workspace_fallback_enabled() -> bool { match env::var(WORKSPACE_FALLBACK_ENV) { Ok(value) => { diff --git a/test_support/clippy.toml b/test_support/clippy.toml new file mode 100644 index 000000000..cc4374c81 --- /dev/null +++ b/test_support/clippy.toml @@ -0,0 +1,14 @@ +# Enforce the AGENTS.md environment mandate. The reason strings surface in the +# diagnostic, so a contributor who trips one is told what to do instead. +# +# Sanctioned sites carry `#[expect(clippy::disallowed_methods, reason = "..")]` +# rather than `allow`, so the expectation goes unfulfilled — and warns — once the +# site is migrated. The backlog removes itself instead of rotting. +disallowed-methods = [ + { path = "std::env::var", reason = "inject an environment reader" }, + { path = "std::env::var_os", reason = "inject an environment reader" }, + { path = "std::env::vars", reason = "inject an environment reader" }, + { path = "std::env::vars_os", reason = "inject an environment reader" }, + { path = "std::env::set_var", reason = "use a stub environment in tests" }, + { path = "std::env::remove_var", reason = "use a stub environment in tests" }, +] diff --git a/tests/bdd/helpers/env_mutation.rs b/tests/bdd/helpers/env_mutation.rs index 4645c0f8e..bedb083b0 100644 --- a/tests/bdd/helpers/env_mutation.rs +++ b/tests/bdd/helpers/env_mutation.rs @@ -3,6 +3,11 @@ //! Provides shared utilities for safely mutating process-global environment //! variables within BDD scenarios using the `EnvLock` serialization mechanism. +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] + use crate::bdd::fixtures::TestWorld; use crate::bdd::types::EnvVarKey; use anyhow::{Result, ensure}; diff --git a/tests/bdd/steps/conditional_manifest.rs b/tests/bdd/steps/conditional_manifest.rs index 26596ba65..7c293ef43 100644 --- a/tests/bdd/steps/conditional_manifest.rs +++ b/tests/bdd/steps/conditional_manifest.rs @@ -10,6 +10,11 @@ //! still letting later assertions inspect the command outputs and environment //! mutations created by the manifest under test. +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] + use crate::bdd::fixtures::TestWorld; use anyhow::{Context, Result}; use rstest_bdd_macros::given; diff --git a/tests/bdd/steps/fs.rs b/tests/bdd/steps/fs.rs index ce4929f46..ea21e28d0 100644 --- a/tests/bdd/steps/fs.rs +++ b/tests/bdd/steps/fs.rs @@ -1,5 +1,10 @@ //! Steps for preparing file-system fixtures used in Jinja tests (Unix only). +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] + use crate::bdd::fixtures::TestWorld; use anyhow::{Context, Result, anyhow, ensure}; use camino::Utf8PathBuf; diff --git a/tests/bdd/steps/manifest/mod.rs b/tests/bdd/steps/manifest/mod.rs index 42438ed20..1527b098f 100644 --- a/tests/bdd/steps/manifest/mod.rs +++ b/tests/bdd/steps/manifest/mod.rs @@ -5,6 +5,11 @@ //! - `helpers.rs` - Typed assertion utilities and target accessor functions //! - `targets.rs` - Target-specific assertion steps +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] + mod helpers; mod targets; diff --git a/tests/bdd/steps/manifest_command_helpers.rs b/tests/bdd/steps/manifest_command_helpers.rs index 657b365b0..638f30d91 100644 --- a/tests/bdd/steps/manifest_command_helpers.rs +++ b/tests/bdd/steps/manifest_command_helpers.rs @@ -3,6 +3,11 @@ //! Split from `manifest_command.rs` so both files stay within the module size //! budget; the step definitions in the parent module call these helpers. +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] + use super::OutputType; use crate::bdd::fixtures::TestWorld; use crate::bdd::helpers::assertions::{assert_slot_contains, normalize_fluent_isolates}; diff --git a/tests/bdd/steps/stdlib/workspace.rs b/tests/bdd/steps/stdlib/workspace.rs index 6352ec268..575818c22 100644 --- a/tests/bdd/steps/stdlib/workspace.rs +++ b/tests/bdd/steps/stdlib/workspace.rs @@ -1,6 +1,11 @@ //! Helpers for preparing stdlib workspaces during BDD scenarios, wiring //! up temporary directories, fixtures, and environment overrides for tests. +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] + use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use crate::bdd::types::{FileContents, HelperName, HttpResponseBody, PathEntries}; use anyhow::{Context, Result, anyhow}; diff --git a/tests/documentation_examples_e2e_tests.rs b/tests/documentation_examples_e2e_tests.rs index f4f4af4f9..7ec001ce1 100644 --- a/tests/documentation_examples_e2e_tests.rs +++ b/tests/documentation_examples_e2e_tests.rs @@ -1,5 +1,9 @@ //! End-to-end contracts for user-facing build examples. +#![expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] #![cfg(unix)] mod documentation_examples; diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index 1bacc8f8d..967881377 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -1,6 +1,11 @@ //! Tests for scoped manipulation of `PATH` via `prepend_dir_to_path` and //! `PathGuard`. +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] + use anyhow::{Context, Result, ensure}; use mockable::Env; use rstest::rstest; diff --git a/tests/env_restore_tests.rs b/tests/env_restore_tests.rs index 36b515fa5..7625df66a 100644 --- a/tests/env_restore_tests.rs +++ b/tests/env_restore_tests.rs @@ -4,6 +4,11 @@ //! `EnvLock`. Tests verify that set variables are restored, absent variables //! are removed, and empty snapshots are handled gracefully. +#![expect( + clippy::disallowed_methods, + reason = "covers the guards being retired; deleted under #493 (integration-binary migration)" +)] + use anyhow::{Result, ensure}; use rstest::rstest; use std::collections::HashMap; diff --git a/tests/kani_cfg_ui_tests.rs b/tests/kani_cfg_ui_tests.rs index 0f4ada031..413f42743 100644 --- a/tests/kani_cfg_ui_tests.rs +++ b/tests/kani_cfg_ui_tests.rs @@ -5,6 +5,11 @@ //! misspelled cfg names should still be rejected when the same check-cfg policy //! is applied. +#![expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] + use std::{ io, path::{Path, PathBuf}, diff --git a/tests/ninja_env_tests.rs b/tests/ninja_env_tests.rs index f2eeab9f2..61161a284 100644 --- a/tests/ninja_env_tests.rs +++ b/tests/ninja_env_tests.rs @@ -1,5 +1,10 @@ //! Tests for overriding the `NINJA_ENV` variable via a mock environment. +#![expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] + use anyhow::{Context, Result, ensure}; use mockable::MockEnv; use netsuke::runner::NINJA_ENV; diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index 92f482fd6..cef477587 100644 --- a/tests/packaging_smoke_tests.rs +++ b/tests/packaging_smoke_tests.rs @@ -4,6 +4,11 @@ //! build-script sources remain in its manifest, where an omission would //! otherwise fail only during release. +#![expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] + use std::collections::BTreeSet; use std::env; use std::path::Path; diff --git a/tests/release_help/mod.rs b/tests/release_help/mod.rs index 51acfdaa4..ca8147c2a 100644 --- a/tests/release_help/mod.rs +++ b/tests/release_help/mod.rs @@ -1,5 +1,10 @@ //! Shared fixtures for release-help generation script tests. +#![expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] + #[cfg(test)] mod script_functions; From 936031d598ccf4ab2cdd5e88053473a365c9e20b Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 3 Aug 2026 22:33:05 +0200 Subject: [PATCH 2/7] Narrow the mixed-content environment expectations to functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit: module-level expectations can suppress future unannotated `std::env` calls across a whole test target, which weakens the gate this PR exists to add. Correct where a file's environment access is incidental to its subject. Four files narrowed to the single function that reads the environment: `path_with_fake_cargo_orthohelp`, `rustc`, `executable_path`, and `packaged_manifest_retains_build_script_sources`. Each locates a build artefact Cargo reports through the environment; the rest of those files has nothing to do with it, so a target-wide exemption was too broad. Module scope is kept where the whole file *is* the pending migration unit — the BDD step modules, `ninja_env_tests`, `env_restore_tests`, `env_path_tests` — with the reason naming #492 or #493. Splitting six site-level attributes across a file that #493 rewrites wholesale would add noise without narrowing anything real, since every test in it mutates the environment. `build.rs` likewise reads only what Cargo supplies. That lint passes under `-D warnings` confirms each new expectation covers its call: a misplaced one would itself warn as unfulfilled. Also reverts markdown reflow carried into five unrelated documents; #512 fixes that at the source. Refs #504, #496. Co-Authored-By: Claude Opus 5 (1M context) --- ...dr-006-adopt-polonius-nightly-toolchain.md | 37 ++--- docs/netsuke-design.md | 26 ++-- docs/polonius.md | 128 +++++++++--------- ...snapshot-testing-in-netsuke-using-insta.md | 6 +- docs/users-guide.md | 17 +-- tests/documentation_examples_e2e_tests.rs | 8 +- tests/kani_cfg_ui_tests.rs | 9 +- tests/packaging_smoke_tests.rs | 9 +- tests/release_help/mod.rs | 9 +- 9 files changed, 126 insertions(+), 123 deletions(-) diff --git a/docs/adr-006-adopt-polonius-nightly-toolchain.md b/docs/adr-006-adopt-polonius-nightly-toolchain.md index b8040c8a5..1618dd215 100644 --- a/docs/adr-006-adopt-polonius-nightly-toolchain.md +++ b/docs/adr-006-adopt-polonius-nightly-toolchain.md @@ -19,8 +19,8 @@ several of these shapes, so the natural borrow-returning form of an accessor can compile where NLL rejected it. Adopting those borrow-centric designs binds the source tree to a -Polonius-enabled compiler, which is nightly-only until the analysis stabilizes. -That conflicts with three standing policies: +Polonius-enabled compiler, which is nightly-only until the analysis +stabilizes. That conflicts with three standing policies: - `rust-toolchain.toml` pinned stable `1.89.0`; - `Cargo.toml` declared `rust-version = "1.89.0"` as a minimum supported Rust @@ -52,20 +52,20 @@ Adopt Polonius now, as a nightly-only source tree: nightly requirement there, and advertising `1.89.0` would misstate the contract; `rust-toolchain.toml` is now the single source of truth. -Every borrow-centric rewrite that depends on the flag is verified both with and -without `-Zpolonius=next` and recorded in +Every borrow-centric rewrite that depends on the flag is verified both with +and without `-Zpolonius=next` and recorded in [polonius migration notes](polonius.md), including refusals where owned style remains correct. ## Rationale - **Design over deployment breadth.** Netsuke ships binaries, not a library - API. Consumers install packaged artefacts or build from source; the toolchain - pin costs contributors one `rustup` fetch, whereas NLL-era double lookups and - key clones cost every call site, forever. + API. Consumers install packaged artefacts or build from source; the + toolchain pin costs contributors one `rustup` fetch, whereas NLL-era + double lookups and key clones cost every call site, forever. - **Reproducibility.** A dated nightly behaves like a release: the same - compiler bits build the tree everywhere. `rustup` provisions it automatically - from `rust-toolchain.toml`. + compiler bits build the tree everywhere. `rustup` provisions it + automatically from `rust-toolchain.toml`. - **Coherent tooling.** Putting the flag in `.cargo/config.toml` keeps rust-analyzer, Clippy, Whitaker (whose Dylint driver is nightly-based), and Kani borrow-checking the same dialect, avoiding phantom editor errors on @@ -81,18 +81,19 @@ remains correct. `rust-toolchain.toml` and `.cargo/config.toml` (and Cargo would not apply them to a registry build anyway), so a bare `cargo install netsuke` of a Polonius-dependent release fails borrow checking on the user's default - toolchain. Registry installs must select the pinned nightly and pass the flag - explicitly - (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`); the - README and users' guide document this command and a contract test pins it. - Source installs from a checkout are unaffected because the pinned toolchain - and workspace configuration apply there. + toolchain. Registry installs must select the pinned nightly and pass the + flag explicitly + (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`); + the README and users' guide document this command and a contract test pins + it. Source installs from a checkout are unaffected because the pinned + toolchain and workspace configuration apply there. - Release packaging builds from the pinned nightly. Binary artefacts are - unaffected: the borrow checker changes what compiles, not what is generated. + unaffected: the borrow checker changes what compiles, not what is + generated. - Dependabot-style toolchain drift is impossible; moving the pin is a deliberate act. Move it forward periodically (and especially once Polonius - stabilizes), re-running the full gate suite, and update this ADR's references - when doing so. + stabilizes), re-running the full gate suite, and update this ADR's + references when doing so. - Sites that genuinely require Polonius are tagged `POLONIUS(...)` in source and must not be rewritten into NLL-era defensive forms; `AGENTS.md` and [polonius migration notes](polonius.md) carry the anti-regression guidance. diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index d2d80526a..bd009c313 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2584,8 +2584,8 @@ flowchart LR ``` Netsuke configuration discovery is implemented in `src/cli/discovery.rs`. -Explicit file selection is handled by `explicit_config_path_with_env(...)`, -which applies the precedence `--config` > `NETSUKE_CONFIG`. Layer loading and +Explicit file selection is handled by `explicit_config_path_with_env(...)`, which +applies the precedence `--config` > `NETSUKE_CONFIG`. Layer loading and automatic discovery are handled by `push_file_layers(...)`, which also applies the `-C/--directory` flag as the project-discovery root. @@ -2702,10 +2702,11 @@ manual flag repetition. override, relying on OrthoConfig's platform-specific defaults for standard directory resolution. - Netsuke-owned environment reads for explicit config selection and early JSON - resolution go through the `EnvProvider` port in `src/cli/discovery.rs`. - Production code uses `StdEnvProvider`; tests can inject a map-backed provider - instead of mutating the process environment. OrthoConfig discovery remains an - external boundary and may still read platform environment variables directly. + resolution go through the `EnvProvider` port in + `src/cli/discovery.rs`. Production code uses `StdEnvProvider`; tests can + inject a map-backed provider instead of mutating the process environment. + OrthoConfig discovery remains an external boundary and may still read + platform environment variables directly. - Configuration files use TOML format by default. JSON5 (`.json`, `.json5`) and YAML (`.yaml`, `.yml`) formats are supported when the corresponding Cargo features are enabled. @@ -2942,12 +2943,13 @@ selected for this project and the rationale for their inclusion. Netsuke compiles with the Polonius alpha borrow-checking analysis (`-Zpolonius=next`) on the dated nightly toolchain pinned in -`rust-toolchain.toml` ([ADR-006](adr-006-adopt-polonius-nightly-toolchain.md)). -Internal APIs follow a borrow-centric design contract: lookups and registries -return references (`&mut V` accessors with clone-on-miss keys), mutation -happens in place, and error context is built lazily on the failure path. -Owned-value style is reserved for genuine constraints — aliasing, suspension -points, thread and process boundaries, and persistent identity — and each such +`rust-toolchain.toml` +([ADR-006](adr-006-adopt-polonius-nightly-toolchain.md)). Internal APIs +follow a borrow-centric design contract: lookups and registries return +references (`&mut V` accessors with clone-on-miss keys), mutation happens in +place, and error context is built lazily on the failure path. Owned-value +style is reserved for genuine constraints — aliasing, suspension points, +thread and process boundaries, and persistent identity — and each such refusal is recorded in the [polonius migration notes](polonius.md) alongside the sites that depend on the analysis. diff --git a/docs/polonius.md b/docs/polonius.md index 93578d36f..f845ff17c 100644 --- a/docs/polonius.md +++ b/docs/polonius.md @@ -3,41 +3,41 @@ Netsuke compiles with the Polonius alpha borrow-checking analysis (`-Zpolonius=next`) on the dated nightly pinned in `rust-toolchain.toml`. [ADR-006](adr-006-adopt-polonius-nightly-toolchain.md) records the toolchain -policy; this document records the audit that motivated it, the API evolutions -it enabled, and the refusals that bound it. Issue +policy; this document records the audit that motivated it, the API +evolutions it enabled, and the refusals that bound it. Issue [#465](https://github.com/leynos/netsuke/issues/465) tracked the migration. ## Method -The migration ran the `nll-to-polonius` two-pass audit with the compiler as the -oracle: +The migration ran the `nll-to-polonius` two-pass audit with the compiler as +the oracle: 1. **Workaround scan** — mechanical sweep for local non-lexical-lifetimes (NLL) workaround shapes: double lookups, `entry()` with unconditionally - cloned keys, re-lookup after insert, index-returning finders, borrow-killing - `drop()` calls, and eager error context. + cloned keys, re-lookup after insert, index-returning finders, + borrow-killing `drop()` calls, and eager error context. 2. **Design-pressure scan** — structural sweep for owned lookup results, id/index indirection, clone-modify-writeback, snapshot-collect loops, and per-module clone hotspots. Every change was compiled twice on `nightly-2026-06-25`: once with -`-Zpolonius=next` (must pass) and once without. The no-flag compile exists only -to classify the individual change: a failure proves the design genuinely -depends on Polonius and the site is tagged `POLONIUS(...)`; success means the -old form was habit rather than necessity and the improvement carries no -toolchain caveat. The complete behavioural test suite runs under -`-Zpolonius=next` — the tree's only supported configuration — and was required -to pass unchanged after every change. +`-Zpolonius=next` (must pass) and once without. The no-flag compile exists +only to classify the individual change: a failure proves the design +genuinely depends on Polonius and the site is tagged `POLONIUS(...)`; +success means the old form was habit rather than necessity and the +improvement carries no toolchain caveat. The complete behavioural test +suite runs under `-Zpolonius=next` — the tree's only supported +configuration — and was required to pass unchanged after every change. ## Polonius-dependent sites -| Site | Tag | Verification | -| ------------------------------------------------------------- | ------------------ | ------------------------------------------------------------------------------- | +| Site | Tag | Verification | +| --- | --- | --- | | `src/graph_view/mod.rs` — `NodePathRegistry::ensure_node_mut` | `POLONIUS(case-3)` | Passes with `-Zpolonius=next`; rejected by NLL with E0499 on nightly-2026-06-25 | `ensure_node_mut` is the get-or-insert accessor behind graph projection: it -returns `&mut NodeKind`, performs a single lookup on the hit path, and clones -the path only on insertion. It replaced three +returns `&mut NodeKind`, performs a single lookup on the hit path, and +clones the path only on insertion. It replaced three `entry(path.clone()).or_insert(NodeKind::Source)` sites that cloned every input, implicit-dependency, and order-only path on every registration. The `get_mut` loan escapes only via the early return, which is the canonical @@ -51,24 +51,24 @@ well — the owned style was habit, so they carry no toolchain caveat: - `src/stdlib/collections.rs` — `group_by_filter` consumed its resolved key in `entry(key_value)` instead of cloning it first. - `src/ir/cycle.rs` — `detect_targets` snapshots borrowed - `&'targets Utf8Path` keys for its deterministic sort instead of cloning every - target path per analysis. The snapshot exists for sorting, not to end a - borrow, so it stays. + `&'targets Utf8Path` keys for its deterministic sort instead of cloning + every target path per analysis. The snapshot exists for sorting, not to + end a borrow, so it stays. - `src/stdlib/which/env.rs` — `EnvSnapshot::resolved_dirs` returns - `Vec<&Utf8Path>` borrowed from the snapshot; the search loop reads borrowed - directories and the paths are copied into the owned `ResolveError::NotFound` - only at the error boundary. + `Vec<&Utf8Path>` borrowed from the snapshot; the search loop reads + borrowed directories and the paths are copied into the owned + `ResolveError::NotFound` only at the error boundary. ## Refusals -Owned style retained deliberately. The constraint, not the borrow checker, is -load-bearing; each site carries the matching source tag: +Owned style retained deliberately. The constraint, not the borrow checker, +is load-bearing; each site carries the matching source tag: -| Site | Tag | Constraint | -| -------------------------------------------------------- | --------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `src/ir/from_manifest_support.rs` — `register_action` | `POLONIUS-REFUSED(id-is-data)` | The action hash is persistent IR identity: stored on every `BuildEdge` and named in the generated Ninja file. Remains owned unless callers demonstrate a need for the canonical interned value. | -| `src/stdlib/which/cache.rs` — `WhichResolver::try_cache` | `POLONIUS-REFUSED(lock-boundary)` | Cache hits are cloned out of the LRU because references cannot outlive the `MutexGuard`; the resolver is shared across evaluation sites. | -| `src/stdlib/collections.rs` — `GroupedValues::new` | `POLONIUS-REFUSED(miss-dominant)` | First-wins string-key registration almost always inserts, so the owned-key `entry` form pays nothing on the rare hit. | +| Site | Tag | Constraint | +| --- | --- | --- | +| `src/ir/from_manifest_support.rs` — `register_action` | `POLONIUS-REFUSED(id-is-data)` | The action hash is persistent IR identity: stored on every `BuildEdge` and named in the generated Ninja file. Remains owned unless callers demonstrate a need for the canonical interned value. | +| `src/stdlib/which/cache.rs` — `WhichResolver::try_cache` | `POLONIUS-REFUSED(lock-boundary)` | Cache hits are cloned out of the LRU because references cannot outlive the `MutexGuard`; the resolver is shared across evaluation sites. | +| `src/stdlib/collections.rs` — `GroupedValues::new` | `POLONIUS-REFUSED(miss-dominant)` | First-wins string-key registration almost always inserts, so the owned-key `entry` form pays nothing on the rare hit. | ## Non-candidates reviewed and cleared @@ -88,58 +88,60 @@ Scanner suspects that turned out not to be NLL residue: - Test-suite `drop()` calls (environment guards, HTTP fixture teardown) are semantic Drop effects, not borrow appeasement. -The plumbing itself is contract-tested: `tests/polonius_toolchain_contract.rs` -pins the dated-nightly channel, the `.cargo/config.toml` `build.rustflags` -entry, the `POLONIUS_FLAGS` default and every RUSTFLAGS-setting Makefile -recipe, and the `RUSTFLAGS` and toolchain presets in the CI, Netsukefile, -coverage, and packaging workflows. +The plumbing itself is contract-tested: +`tests/polonius_toolchain_contract.rs` pins the dated-nightly channel, the +`.cargo/config.toml` `build.rustflags` entry, the `POLONIUS_FLAGS` default +and every RUSTFLAGS-setting Makefile recipe, and the `RUSTFLAGS` and +toolchain presets in the CI, Netsukefile, coverage, and packaging +workflows. ## Harness consequences -Tooling that rebuilds the crate with its own flags must propagate the Polonius -flag or avoid compiling the crate: +Tooling that rebuilds the crate with its own flags must propagate the +Polonius flag or avoid compiling the crate: - **trybuild** discards ambient `RUSTFLAGS` and workspace `build.rustflags`, replacing them via `--config` on its scratch project, and it always builds the host crate as a fixture dependency. The Kani cfg policy fixture is therefore compiled and run directly with the workspace `rustc` - (`tests/kani_cfg_ui_tests.rs`); do not reintroduce trybuild cases that depend - on the `netsuke` crate while the tree is Polonius-only. + (`tests/kani_cfg_ui_tests.rs`); do not reintroduce trybuild cases that + depend on the `netsuke` crate while the tree is Polonius-only. - **Kani** and **Whitaker** run under their own toolchains but read the workspace `.cargo/config.toml` or the Makefile `RUSTFLAGS`, so they borrow-check with `-Zpolonius=next` and need no special handling. - **CI setup actions**: `actions-rust-lang/setup-rust-toolchain` exports `RUSTFLAGS="-D warnings"` into the job environment when the variable is - unset, which shadows `.cargo/config.toml` for every later step. The workflows - therefore pre-set `RUSTFLAGS` (including `-Zpolonius=next`) at job level — - the action defers to an existing value — and the Makefile recipes append - `POLONIUS_FLAGS` to any ambient `RUSTFLAGS` as a second line of defence. - `cargo-llvm-cov` appends its instrumentation flags to the ambient value, so - coverage inherits the flag from the job environment. + unset, which shadows `.cargo/config.toml` for every later step. The + workflows therefore pre-set `RUSTFLAGS` (including `-Zpolonius=next`) at + job level — the action defers to an existing value — and the Makefile + recipes append `POLONIUS_FLAGS` to any ambient `RUSTFLAGS` as a second + line of defence. `cargo-llvm-cov` appends its instrumentation flags to + the ambient value, so coverage inherits the flag from the job + environment. - **Registry installs**: the crates.io package excludes `rust-toolchain.toml` and `.cargo/config.toml`, and registry builds run outside the checkout, so `cargo install netsuke` must select the pinned nightly and pass the flag explicitly - (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`). The - README and users' guide document the command and + (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`). + The README and users' guide document the command and `tests/documentation_examples_tests.rs` pins it. - **cargo-mutants** (scheduled, informational) runs through the shared - `mutation-cargo.yml` workflow, which controls its own environment; if those - runs regress with E0499 at tagged sites, the shared workflow needs the same - `RUSTFLAGS` treatment. + `mutation-cargo.yml` workflow, which controls its own environment; if + those runs regress with E0499 at tagged sites, the shared workflow needs + the same `RUSTFLAGS` treatment. ## Clone counts -Measured with `rg --count '\.clone\(\)'` over `src/` (tests included where they -live in `src/`): +Measured with `rg --count '\.clone\(\)'` over `src/` (tests included where +they live in `src/`): -| Scope | Before | After | -| ---------------------------- | ------ | ----- | -| `src/` total | 158 | 151 | -| `src/graph_view/mod.rs` | 17 | 14 | -| `src/ir/cycle.rs` (non-test) | 1 | 0 | -| `src/stdlib/which/env.rs` | 4 | 1 | -| `src/stdlib/collections.rs` | 4 | 3 | +| Scope | Before | After | +| --- | --- | --- | +| `src/` total | 158 | 151 | +| `src/graph_view/mod.rs` | 17 | 14 | +| `src/ir/cycle.rs` (non-test) | 1 | 0 | +| `src/stdlib/which/env.rs` | 4 | 1 | +| `src/stdlib/collections.rs` | 4 | 3 | The scanner's clone-modify-writeback section was empty before and after the migration. The remaining graph_view clones construct owned keys for the two @@ -158,8 +160,8 @@ When `-Zpolonius=next` (or its successor) reaches stable Rust: ## Anti-regression guidance -The contract for new code and reviews (also summarized in `AGENTS.md` and the -[developers' guide](developers-guide.md)): +The contract for new code and reviews (also summarized in `AGENTS.md` and +the [developers' guide](developers-guide.md)): - Do not rewrite `POLONIUS(...)` sites into double lookups, `entry(key.clone())`, or `contains_key` guards — the direct form is @@ -169,7 +171,7 @@ The contract for new code and reviews (also summarized in `AGENTS.md` and the borrow-returning form compiles under the project toolchain. - Respect `POLONIUS-REFUSED(...)` tags: the named constraint (identity, locks, aliasing, suspension points, thread boundaries) is permanent, and - "simplifying" those sites into reference-returning forms will not compile or - will break the design. + "simplifying" those sites into reference-returning forms will not compile + or will break the design. - Classify any new borrow-centric API by compiling with and without the flag, then record it here. diff --git a/docs/snapshot-testing-in-netsuke-using-insta.md b/docs/snapshot-testing-in-netsuke-using-insta.md index 17bda652b..775a78637 100644 --- a/docs/snapshot-testing-in-netsuke-using-insta.md +++ b/docs/snapshot-testing-in-netsuke-using-insta.md @@ -281,9 +281,9 @@ function, serializing locale state across the test suite. > In this repository the canonical runner is cargo-nextest: `make test`, or > `cargo nextest run --test ninja_snapshot_tests` for a focused run. See > [Test execution](developers-guide.md#test-execution). `cargo insta` needs to -> be told which runner to drive, so use -> `cargo insta test --test-runner nextest` and `cargo insta review` when -> accepting changes. The `cargo test` invocations below are the generic form. +> be told which runner to drive, so use `cargo insta test --test-runner +> nextest` and `cargo insta review` when accepting changes. The `cargo test` +> invocations below are the generic form. To execute the snapshot tests, run `cargo test`. All tests (including our new snapshot tests) will run. On the first run (or whenever a snapshot differs from diff --git a/docs/users-guide.md b/docs/users-guide.md index 324907b3a..bf6117a46 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -11,14 +11,15 @@ change before 1.0. Pin the Netsuke version in automated workflows. ## Install Netsuke Netsuke requires [Ninja](https://ninja-build.org/) on `PATH`. A source build -also requires the dated Rust nightly toolchain pinned in `rust-toolchain.toml`, -because Netsuke builds with the Polonius borrow checker (`-Zpolonius=next`); -`rustup` installs it automatically inside a checkout. +also requires the dated Rust nightly toolchain pinned in +`rust-toolchain.toml`, because Netsuke builds with the Polonius borrow +checker (`-Zpolonius=next`); `rustup` installs it automatically inside a +checkout. Netsuke v0.1.0 is available from crates.io. Where -[`cargo binstall`](https://github.com/cargo-bins/cargo-binstall) is available, -prefer it: it fetches a prebuilt release binary and avoids the toolchain -requirement below. +[`cargo binstall`](https://github.com/cargo-bins/cargo-binstall) is +available, prefer it: it fetches a prebuilt release binary and avoids the +toolchain requirement below. @@ -27,8 +28,8 @@ cargo binstall netsuke ``` Building from the registry instead runs outside a repository checkout, so -neither the pinned toolchain nor the Polonius flag is picked up automatically; -supply both explicitly: +neither the pinned toolchain nor the Polonius flag is picked up +automatically; supply both explicitly: diff --git a/tests/documentation_examples_e2e_tests.rs b/tests/documentation_examples_e2e_tests.rs index 7ec001ce1..5dd056a09 100644 --- a/tests/documentation_examples_e2e_tests.rs +++ b/tests/documentation_examples_e2e_tests.rs @@ -1,9 +1,5 @@ //! End-to-end contracts for user-facing build examples. -#![expect( - clippy::disallowed_methods, - reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" -)] #![cfg(unix)] mod documentation_examples; @@ -20,6 +16,10 @@ use test_support::fs as test_fs; use test_support::netsuke::{NetsukeRun, run_netsuke_in_with_env}; use test_support::{ninja::ninja_integration_workspace, write_exec, write_exec_with_content}; +#[expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] fn executable_path(stub_directory: &Utf8Path) -> Result { let host_path = std::env::var("PATH").context("read host PATH")?; Ok(format!("{stub_directory}:{host_path}")) diff --git a/tests/kani_cfg_ui_tests.rs b/tests/kani_cfg_ui_tests.rs index 413f42743..69705f062 100644 --- a/tests/kani_cfg_ui_tests.rs +++ b/tests/kani_cfg_ui_tests.rs @@ -5,11 +5,6 @@ //! misspelled cfg names should still be rejected when the same check-cfg policy //! is applied. -#![expect( - clippy::disallowed_methods, - reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" -)] - use std::{ io, path::{Path, PathBuf}, @@ -103,6 +98,10 @@ fn compile_ui_fixture(source: &str, output_path: &Path) -> io::Result { .output() } +#[expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] fn rustc() -> PathBuf { std::env::var_os("RUSTC").map_or_else(|| Path::new("rustc").to_path_buf(), PathBuf::from) } diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index cef477587..e945e106b 100644 --- a/tests/packaging_smoke_tests.rs +++ b/tests/packaging_smoke_tests.rs @@ -4,11 +4,6 @@ //! build-script sources remain in its manifest, where an omission would //! otherwise fail only during release. -#![expect( - clippy::disallowed_methods, - reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" -)] - use std::collections::BTreeSet; use std::env; use std::path::Path; @@ -23,6 +18,10 @@ const REQUIRED_PACKAGED_FILES: [&str; 5] = [ ]; #[test] +#[expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] fn packaged_manifest_retains_build_script_sources() { let cargo_binary = env::var_os("CARGO").unwrap_or_else(|| "cargo".into()); let publish_output = Command::new(&cargo_binary) diff --git a/tests/release_help/mod.rs b/tests/release_help/mod.rs index ca8147c2a..5d33b19cc 100644 --- a/tests/release_help/mod.rs +++ b/tests/release_help/mod.rs @@ -1,10 +1,5 @@ //! Shared fixtures for release-help generation script tests. -#![expect( - clippy::disallowed_methods, - reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" -)] - #[cfg(test)] mod script_functions; @@ -130,6 +125,10 @@ esac "# } +#[expect( + clippy::disallowed_methods, + reason = "locating build artefacts Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] pub fn path_with_fake_cargo_orthohelp(fixture: &ScriptFixture) -> Result { let existing_path = std::env::var_os("PATH").unwrap_or_default(); let mut entries = vec![fixture.fake_bin_dir.clone()]; From 6ad6484d39dc5a526f8182ed76293edda1f6a064 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 4 Aug 2026 03:27:21 +0200 Subject: [PATCH 3/7] Restore main's wrapping in untouched documents The rebase reinstated pre-#512 line wrapping and table padding across five documents this branch never meant to change. The churn carries no prose difference, so restore main's versions and keep the branch scoped to the clippy environment mandate. --- ...dr-006-adopt-polonius-nightly-toolchain.md | 37 +++-- docs/netsuke-design.md | 26 ++-- docs/polonius.md | 128 +++++++++--------- ...snapshot-testing-in-netsuke-using-insta.md | 6 +- docs/users-guide.md | 17 ++- 5 files changed, 104 insertions(+), 110 deletions(-) diff --git a/docs/adr-006-adopt-polonius-nightly-toolchain.md b/docs/adr-006-adopt-polonius-nightly-toolchain.md index 1618dd215..b8040c8a5 100644 --- a/docs/adr-006-adopt-polonius-nightly-toolchain.md +++ b/docs/adr-006-adopt-polonius-nightly-toolchain.md @@ -19,8 +19,8 @@ several of these shapes, so the natural borrow-returning form of an accessor can compile where NLL rejected it. Adopting those borrow-centric designs binds the source tree to a -Polonius-enabled compiler, which is nightly-only until the analysis -stabilizes. That conflicts with three standing policies: +Polonius-enabled compiler, which is nightly-only until the analysis stabilizes. +That conflicts with three standing policies: - `rust-toolchain.toml` pinned stable `1.89.0`; - `Cargo.toml` declared `rust-version = "1.89.0"` as a minimum supported Rust @@ -52,20 +52,20 @@ Adopt Polonius now, as a nightly-only source tree: nightly requirement there, and advertising `1.89.0` would misstate the contract; `rust-toolchain.toml` is now the single source of truth. -Every borrow-centric rewrite that depends on the flag is verified both with -and without `-Zpolonius=next` and recorded in +Every borrow-centric rewrite that depends on the flag is verified both with and +without `-Zpolonius=next` and recorded in [polonius migration notes](polonius.md), including refusals where owned style remains correct. ## Rationale - **Design over deployment breadth.** Netsuke ships binaries, not a library - API. Consumers install packaged artefacts or build from source; the - toolchain pin costs contributors one `rustup` fetch, whereas NLL-era - double lookups and key clones cost every call site, forever. + API. Consumers install packaged artefacts or build from source; the toolchain + pin costs contributors one `rustup` fetch, whereas NLL-era double lookups and + key clones cost every call site, forever. - **Reproducibility.** A dated nightly behaves like a release: the same - compiler bits build the tree everywhere. `rustup` provisions it - automatically from `rust-toolchain.toml`. + compiler bits build the tree everywhere. `rustup` provisions it automatically + from `rust-toolchain.toml`. - **Coherent tooling.** Putting the flag in `.cargo/config.toml` keeps rust-analyzer, Clippy, Whitaker (whose Dylint driver is nightly-based), and Kani borrow-checking the same dialect, avoiding phantom editor errors on @@ -81,19 +81,18 @@ remains correct. `rust-toolchain.toml` and `.cargo/config.toml` (and Cargo would not apply them to a registry build anyway), so a bare `cargo install netsuke` of a Polonius-dependent release fails borrow checking on the user's default - toolchain. Registry installs must select the pinned nightly and pass the - flag explicitly - (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`); - the README and users' guide document this command and a contract test pins - it. Source installs from a checkout are unaffected because the pinned - toolchain and workspace configuration apply there. + toolchain. Registry installs must select the pinned nightly and pass the flag + explicitly + (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`); the + README and users' guide document this command and a contract test pins it. + Source installs from a checkout are unaffected because the pinned toolchain + and workspace configuration apply there. - Release packaging builds from the pinned nightly. Binary artefacts are - unaffected: the borrow checker changes what compiles, not what is - generated. + unaffected: the borrow checker changes what compiles, not what is generated. - Dependabot-style toolchain drift is impossible; moving the pin is a deliberate act. Move it forward periodically (and especially once Polonius - stabilizes), re-running the full gate suite, and update this ADR's - references when doing so. + stabilizes), re-running the full gate suite, and update this ADR's references + when doing so. - Sites that genuinely require Polonius are tagged `POLONIUS(...)` in source and must not be rewritten into NLL-era defensive forms; `AGENTS.md` and [polonius migration notes](polonius.md) carry the anti-regression guidance. diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index bd009c313..d2d80526a 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2584,8 +2584,8 @@ flowchart LR ``` Netsuke configuration discovery is implemented in `src/cli/discovery.rs`. -Explicit file selection is handled by `explicit_config_path_with_env(...)`, which -applies the precedence `--config` > `NETSUKE_CONFIG`. Layer loading and +Explicit file selection is handled by `explicit_config_path_with_env(...)`, +which applies the precedence `--config` > `NETSUKE_CONFIG`. Layer loading and automatic discovery are handled by `push_file_layers(...)`, which also applies the `-C/--directory` flag as the project-discovery root. @@ -2702,11 +2702,10 @@ manual flag repetition. override, relying on OrthoConfig's platform-specific defaults for standard directory resolution. - Netsuke-owned environment reads for explicit config selection and early JSON - resolution go through the `EnvProvider` port in - `src/cli/discovery.rs`. Production code uses `StdEnvProvider`; tests can - inject a map-backed provider instead of mutating the process environment. - OrthoConfig discovery remains an external boundary and may still read - platform environment variables directly. + resolution go through the `EnvProvider` port in `src/cli/discovery.rs`. + Production code uses `StdEnvProvider`; tests can inject a map-backed provider + instead of mutating the process environment. OrthoConfig discovery remains an + external boundary and may still read platform environment variables directly. - Configuration files use TOML format by default. JSON5 (`.json`, `.json5`) and YAML (`.yaml`, `.yml`) formats are supported when the corresponding Cargo features are enabled. @@ -2943,13 +2942,12 @@ selected for this project and the rationale for their inclusion. Netsuke compiles with the Polonius alpha borrow-checking analysis (`-Zpolonius=next`) on the dated nightly toolchain pinned in -`rust-toolchain.toml` -([ADR-006](adr-006-adopt-polonius-nightly-toolchain.md)). Internal APIs -follow a borrow-centric design contract: lookups and registries return -references (`&mut V` accessors with clone-on-miss keys), mutation happens in -place, and error context is built lazily on the failure path. Owned-value -style is reserved for genuine constraints — aliasing, suspension points, -thread and process boundaries, and persistent identity — and each such +`rust-toolchain.toml` ([ADR-006](adr-006-adopt-polonius-nightly-toolchain.md)). +Internal APIs follow a borrow-centric design contract: lookups and registries +return references (`&mut V` accessors with clone-on-miss keys), mutation +happens in place, and error context is built lazily on the failure path. +Owned-value style is reserved for genuine constraints — aliasing, suspension +points, thread and process boundaries, and persistent identity — and each such refusal is recorded in the [polonius migration notes](polonius.md) alongside the sites that depend on the analysis. diff --git a/docs/polonius.md b/docs/polonius.md index f845ff17c..93578d36f 100644 --- a/docs/polonius.md +++ b/docs/polonius.md @@ -3,41 +3,41 @@ Netsuke compiles with the Polonius alpha borrow-checking analysis (`-Zpolonius=next`) on the dated nightly pinned in `rust-toolchain.toml`. [ADR-006](adr-006-adopt-polonius-nightly-toolchain.md) records the toolchain -policy; this document records the audit that motivated it, the API -evolutions it enabled, and the refusals that bound it. Issue +policy; this document records the audit that motivated it, the API evolutions +it enabled, and the refusals that bound it. Issue [#465](https://github.com/leynos/netsuke/issues/465) tracked the migration. ## Method -The migration ran the `nll-to-polonius` two-pass audit with the compiler as -the oracle: +The migration ran the `nll-to-polonius` two-pass audit with the compiler as the +oracle: 1. **Workaround scan** — mechanical sweep for local non-lexical-lifetimes (NLL) workaround shapes: double lookups, `entry()` with unconditionally - cloned keys, re-lookup after insert, index-returning finders, - borrow-killing `drop()` calls, and eager error context. + cloned keys, re-lookup after insert, index-returning finders, borrow-killing + `drop()` calls, and eager error context. 2. **Design-pressure scan** — structural sweep for owned lookup results, id/index indirection, clone-modify-writeback, snapshot-collect loops, and per-module clone hotspots. Every change was compiled twice on `nightly-2026-06-25`: once with -`-Zpolonius=next` (must pass) and once without. The no-flag compile exists -only to classify the individual change: a failure proves the design -genuinely depends on Polonius and the site is tagged `POLONIUS(...)`; -success means the old form was habit rather than necessity and the -improvement carries no toolchain caveat. The complete behavioural test -suite runs under `-Zpolonius=next` — the tree's only supported -configuration — and was required to pass unchanged after every change. +`-Zpolonius=next` (must pass) and once without. The no-flag compile exists only +to classify the individual change: a failure proves the design genuinely +depends on Polonius and the site is tagged `POLONIUS(...)`; success means the +old form was habit rather than necessity and the improvement carries no +toolchain caveat. The complete behavioural test suite runs under +`-Zpolonius=next` — the tree's only supported configuration — and was required +to pass unchanged after every change. ## Polonius-dependent sites -| Site | Tag | Verification | -| --- | --- | --- | +| Site | Tag | Verification | +| ------------------------------------------------------------- | ------------------ | ------------------------------------------------------------------------------- | | `src/graph_view/mod.rs` — `NodePathRegistry::ensure_node_mut` | `POLONIUS(case-3)` | Passes with `-Zpolonius=next`; rejected by NLL with E0499 on nightly-2026-06-25 | `ensure_node_mut` is the get-or-insert accessor behind graph projection: it -returns `&mut NodeKind`, performs a single lookup on the hit path, and -clones the path only on insertion. It replaced three +returns `&mut NodeKind`, performs a single lookup on the hit path, and clones +the path only on insertion. It replaced three `entry(path.clone()).or_insert(NodeKind::Source)` sites that cloned every input, implicit-dependency, and order-only path on every registration. The `get_mut` loan escapes only via the early return, which is the canonical @@ -51,24 +51,24 @@ well — the owned style was habit, so they carry no toolchain caveat: - `src/stdlib/collections.rs` — `group_by_filter` consumed its resolved key in `entry(key_value)` instead of cloning it first. - `src/ir/cycle.rs` — `detect_targets` snapshots borrowed - `&'targets Utf8Path` keys for its deterministic sort instead of cloning - every target path per analysis. The snapshot exists for sorting, not to - end a borrow, so it stays. + `&'targets Utf8Path` keys for its deterministic sort instead of cloning every + target path per analysis. The snapshot exists for sorting, not to end a + borrow, so it stays. - `src/stdlib/which/env.rs` — `EnvSnapshot::resolved_dirs` returns - `Vec<&Utf8Path>` borrowed from the snapshot; the search loop reads - borrowed directories and the paths are copied into the owned - `ResolveError::NotFound` only at the error boundary. + `Vec<&Utf8Path>` borrowed from the snapshot; the search loop reads borrowed + directories and the paths are copied into the owned `ResolveError::NotFound` + only at the error boundary. ## Refusals -Owned style retained deliberately. The constraint, not the borrow checker, -is load-bearing; each site carries the matching source tag: +Owned style retained deliberately. The constraint, not the borrow checker, is +load-bearing; each site carries the matching source tag: -| Site | Tag | Constraint | -| --- | --- | --- | -| `src/ir/from_manifest_support.rs` — `register_action` | `POLONIUS-REFUSED(id-is-data)` | The action hash is persistent IR identity: stored on every `BuildEdge` and named in the generated Ninja file. Remains owned unless callers demonstrate a need for the canonical interned value. | -| `src/stdlib/which/cache.rs` — `WhichResolver::try_cache` | `POLONIUS-REFUSED(lock-boundary)` | Cache hits are cloned out of the LRU because references cannot outlive the `MutexGuard`; the resolver is shared across evaluation sites. | -| `src/stdlib/collections.rs` — `GroupedValues::new` | `POLONIUS-REFUSED(miss-dominant)` | First-wins string-key registration almost always inserts, so the owned-key `entry` form pays nothing on the rare hit. | +| Site | Tag | Constraint | +| -------------------------------------------------------- | --------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/ir/from_manifest_support.rs` — `register_action` | `POLONIUS-REFUSED(id-is-data)` | The action hash is persistent IR identity: stored on every `BuildEdge` and named in the generated Ninja file. Remains owned unless callers demonstrate a need for the canonical interned value. | +| `src/stdlib/which/cache.rs` — `WhichResolver::try_cache` | `POLONIUS-REFUSED(lock-boundary)` | Cache hits are cloned out of the LRU because references cannot outlive the `MutexGuard`; the resolver is shared across evaluation sites. | +| `src/stdlib/collections.rs` — `GroupedValues::new` | `POLONIUS-REFUSED(miss-dominant)` | First-wins string-key registration almost always inserts, so the owned-key `entry` form pays nothing on the rare hit. | ## Non-candidates reviewed and cleared @@ -88,60 +88,58 @@ Scanner suspects that turned out not to be NLL residue: - Test-suite `drop()` calls (environment guards, HTTP fixture teardown) are semantic Drop effects, not borrow appeasement. -The plumbing itself is contract-tested: -`tests/polonius_toolchain_contract.rs` pins the dated-nightly channel, the -`.cargo/config.toml` `build.rustflags` entry, the `POLONIUS_FLAGS` default -and every RUSTFLAGS-setting Makefile recipe, and the `RUSTFLAGS` and -toolchain presets in the CI, Netsukefile, coverage, and packaging -workflows. +The plumbing itself is contract-tested: `tests/polonius_toolchain_contract.rs` +pins the dated-nightly channel, the `.cargo/config.toml` `build.rustflags` +entry, the `POLONIUS_FLAGS` default and every RUSTFLAGS-setting Makefile +recipe, and the `RUSTFLAGS` and toolchain presets in the CI, Netsukefile, +coverage, and packaging workflows. ## Harness consequences -Tooling that rebuilds the crate with its own flags must propagate the -Polonius flag or avoid compiling the crate: +Tooling that rebuilds the crate with its own flags must propagate the Polonius +flag or avoid compiling the crate: - **trybuild** discards ambient `RUSTFLAGS` and workspace `build.rustflags`, replacing them via `--config` on its scratch project, and it always builds the host crate as a fixture dependency. The Kani cfg policy fixture is therefore compiled and run directly with the workspace `rustc` - (`tests/kani_cfg_ui_tests.rs`); do not reintroduce trybuild cases that - depend on the `netsuke` crate while the tree is Polonius-only. + (`tests/kani_cfg_ui_tests.rs`); do not reintroduce trybuild cases that depend + on the `netsuke` crate while the tree is Polonius-only. - **Kani** and **Whitaker** run under their own toolchains but read the workspace `.cargo/config.toml` or the Makefile `RUSTFLAGS`, so they borrow-check with `-Zpolonius=next` and need no special handling. - **CI setup actions**: `actions-rust-lang/setup-rust-toolchain` exports `RUSTFLAGS="-D warnings"` into the job environment when the variable is - unset, which shadows `.cargo/config.toml` for every later step. The - workflows therefore pre-set `RUSTFLAGS` (including `-Zpolonius=next`) at - job level — the action defers to an existing value — and the Makefile - recipes append `POLONIUS_FLAGS` to any ambient `RUSTFLAGS` as a second - line of defence. `cargo-llvm-cov` appends its instrumentation flags to - the ambient value, so coverage inherits the flag from the job - environment. + unset, which shadows `.cargo/config.toml` for every later step. The workflows + therefore pre-set `RUSTFLAGS` (including `-Zpolonius=next`) at job level — + the action defers to an existing value — and the Makefile recipes append + `POLONIUS_FLAGS` to any ambient `RUSTFLAGS` as a second line of defence. + `cargo-llvm-cov` appends its instrumentation flags to the ambient value, so + coverage inherits the flag from the job environment. - **Registry installs**: the crates.io package excludes `rust-toolchain.toml` and `.cargo/config.toml`, and registry builds run outside the checkout, so `cargo install netsuke` must select the pinned nightly and pass the flag explicitly - (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`). - The README and users' guide document the command and + (`RUSTFLAGS=-Zpolonius=next cargo +nightly-2026-06-25 install netsuke`). The + README and users' guide document the command and `tests/documentation_examples_tests.rs` pins it. - **cargo-mutants** (scheduled, informational) runs through the shared - `mutation-cargo.yml` workflow, which controls its own environment; if - those runs regress with E0499 at tagged sites, the shared workflow needs - the same `RUSTFLAGS` treatment. + `mutation-cargo.yml` workflow, which controls its own environment; if those + runs regress with E0499 at tagged sites, the shared workflow needs the same + `RUSTFLAGS` treatment. ## Clone counts -Measured with `rg --count '\.clone\(\)'` over `src/` (tests included where -they live in `src/`): +Measured with `rg --count '\.clone\(\)'` over `src/` (tests included where they +live in `src/`): -| Scope | Before | After | -| --- | --- | --- | -| `src/` total | 158 | 151 | -| `src/graph_view/mod.rs` | 17 | 14 | -| `src/ir/cycle.rs` (non-test) | 1 | 0 | -| `src/stdlib/which/env.rs` | 4 | 1 | -| `src/stdlib/collections.rs` | 4 | 3 | +| Scope | Before | After | +| ---------------------------- | ------ | ----- | +| `src/` total | 158 | 151 | +| `src/graph_view/mod.rs` | 17 | 14 | +| `src/ir/cycle.rs` (non-test) | 1 | 0 | +| `src/stdlib/which/env.rs` | 4 | 1 | +| `src/stdlib/collections.rs` | 4 | 3 | The scanner's clone-modify-writeback section was empty before and after the migration. The remaining graph_view clones construct owned keys for the two @@ -160,8 +158,8 @@ When `-Zpolonius=next` (or its successor) reaches stable Rust: ## Anti-regression guidance -The contract for new code and reviews (also summarized in `AGENTS.md` and -the [developers' guide](developers-guide.md)): +The contract for new code and reviews (also summarized in `AGENTS.md` and the +[developers' guide](developers-guide.md)): - Do not rewrite `POLONIUS(...)` sites into double lookups, `entry(key.clone())`, or `contains_key` guards — the direct form is @@ -171,7 +169,7 @@ the [developers' guide](developers-guide.md)): borrow-returning form compiles under the project toolchain. - Respect `POLONIUS-REFUSED(...)` tags: the named constraint (identity, locks, aliasing, suspension points, thread boundaries) is permanent, and - "simplifying" those sites into reference-returning forms will not compile - or will break the design. + "simplifying" those sites into reference-returning forms will not compile or + will break the design. - Classify any new borrow-centric API by compiling with and without the flag, then record it here. diff --git a/docs/snapshot-testing-in-netsuke-using-insta.md b/docs/snapshot-testing-in-netsuke-using-insta.md index 775a78637..17bda652b 100644 --- a/docs/snapshot-testing-in-netsuke-using-insta.md +++ b/docs/snapshot-testing-in-netsuke-using-insta.md @@ -281,9 +281,9 @@ function, serializing locale state across the test suite. > In this repository the canonical runner is cargo-nextest: `make test`, or > `cargo nextest run --test ninja_snapshot_tests` for a focused run. See > [Test execution](developers-guide.md#test-execution). `cargo insta` needs to -> be told which runner to drive, so use `cargo insta test --test-runner -> nextest` and `cargo insta review` when accepting changes. The `cargo test` -> invocations below are the generic form. +> be told which runner to drive, so use +> `cargo insta test --test-runner nextest` and `cargo insta review` when +> accepting changes. The `cargo test` invocations below are the generic form. To execute the snapshot tests, run `cargo test`. All tests (including our new snapshot tests) will run. On the first run (or whenever a snapshot differs from diff --git a/docs/users-guide.md b/docs/users-guide.md index bf6117a46..324907b3a 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -11,15 +11,14 @@ change before 1.0. Pin the Netsuke version in automated workflows. ## Install Netsuke Netsuke requires [Ninja](https://ninja-build.org/) on `PATH`. A source build -also requires the dated Rust nightly toolchain pinned in -`rust-toolchain.toml`, because Netsuke builds with the Polonius borrow -checker (`-Zpolonius=next`); `rustup` installs it automatically inside a -checkout. +also requires the dated Rust nightly toolchain pinned in `rust-toolchain.toml`, +because Netsuke builds with the Polonius borrow checker (`-Zpolonius=next`); +`rustup` installs it automatically inside a checkout. Netsuke v0.1.0 is available from crates.io. Where -[`cargo binstall`](https://github.com/cargo-bins/cargo-binstall) is -available, prefer it: it fetches a prebuilt release binary and avoids the -toolchain requirement below. +[`cargo binstall`](https://github.com/cargo-bins/cargo-binstall) is available, +prefer it: it fetches a prebuilt release binary and avoids the toolchain +requirement below. @@ -28,8 +27,8 @@ cargo binstall netsuke ``` Building from the registry instead runs outside a repository checkout, so -neither the pinned toolchain nor the Polonius flag is picked up -automatically; supply both explicitly: +neither the pinned toolchain nor the Polonius flag is picked up automatically; +supply both explicitly: From d4eb5eb4417e6ed26e55477ce5ba32804bf0e133 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 4 Aug 2026 13:56:49 +0200 Subject: [PATCH 4/7] Narrow the disallowed-methods expectations to individual items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine files carried a crate-level `#![expect(clippy::disallowed_methods)]`. A file-wide waiver silences the lint for code that has not been written yet, so the backlog it is meant to track cannot grow visibly: a new ambient read added to any of these files would have compiled silently. Move each expectation onto the item that actually reads the environment, preserving the existing reason text and issue references (#492 for the rstest-bdd migration, #493 for the integration-binary migration). `build.rs` gets one expectation per function rather than one for the crate, each naming the specific Cargo-supplied variables it reads and why no seam can exist for them: `SOURCE_DATE_EPOCH` is the reproducible-builds contract, `TARGET`/`PROFILE` are known only to the build script, and the `CARGO_*`/`OUT_DIR` set describes the crate being compiled. `mutate_env_var_handles_various_operations` needed a different shape. `rstest` generates one function per case and the attribute does not survive that expansion, so the two read-backs now go through a `read_back` helper that carries the expectation — narrower than the module-wide waiver the alternative would have required. `windows_command_setup` was never linted at all: it is `#[cfg(windows)]`, so no Linux run ever saw its `std::env::var_os("PATH")`. It reads the inherited PATH because the code under test resolves commands through the real process environment, so it gets an item-level expectation rather than a migration. Verified by mutation: an unannotated `std::env::var_os` added to `tests/env_restore_tests.rs` now fails `make lint`, and passes once removed. Addresses CodeRabbit findings on #505. --- build.rs | 17 ++++++++---- tests/bdd/helpers/env_mutation.rs | 26 ++++++++++++++----- tests/bdd/steps/conditional_manifest.rs | 9 +++---- tests/bdd/steps/fs.rs | 9 +++---- tests/bdd/steps/manifest/mod.rs | 9 +++---- tests/bdd/steps/manifest_command_helpers.rs | 9 +++---- tests/bdd/steps/stdlib/workspace.rs | 9 +++---- tests/env_path_tests.rs | 17 ++++++++---- tests/env_restore_tests.rs | 21 +++++++++++---- tests/ninja_env_tests.rs | 13 ++++++---- .../command_filters/windows_filter_tests.rs | 4 +++ 11 files changed, 91 insertions(+), 52 deletions(-) diff --git a/build.rs b/build.rs index 4b6b1d550..ef9d85ca4 100644 --- a/build.rs +++ b/build.rs @@ -1,8 +1,3 @@ -#![expect( - clippy::disallowed_methods, - reason = "build scripts read CARGO_* and OUT_DIR from the environment Cargo provides; there is no seam to inject and no test to isolate" -)] - //! Build script for Netsuke. //! //! This script performs two main tasks: @@ -73,6 +68,10 @@ mod theme; mod build_l10n_audit; +#[expect( + clippy::disallowed_methods, + reason = "SOURCE_DATE_EPOCH is the reproducible-builds contract: the build system supplies it to the build script's process, so there is no seam to inject it through" +)] fn manual_date() -> String { let Ok(raw) = env::var("SOURCE_DATE_EPOCH") else { return FALLBACK_DATE.into(); @@ -100,6 +99,10 @@ fn manual_date() -> String { }) } +#[expect( + clippy::disallowed_methods, + reason = "TARGET and PROFILE are set by Cargo for the build script alone; nothing else knows the triple and profile being built, so they cannot be passed in" +)] fn out_dir_for_target_profile() -> PathBuf { let target = env::var("TARGET").unwrap_or_else(|_| "unknown-target".into()); let profile = env::var("PROFILE").unwrap_or_else(|_| "unknown-profile".into()); @@ -137,6 +140,10 @@ fn emit_rerun_directives() { println!("cargo:rerun-if-changed=locales/es-ES/messages.ftl"); } +#[expect( + clippy::disallowed_methods, + reason = "CARGO_BIN_NAME, CARGO_PKG_NAME, CARGO_PKG_VERSION and OUT_DIR are Cargo's own build-script inputs; they describe the crate being compiled and Cargo provides them only through the environment" +)] fn generate_man_page(out_dir: &Path) -> Result<(), Box> { let cmd = cli::Cli::command(); let name = cmd diff --git a/tests/bdd/helpers/env_mutation.rs b/tests/bdd/helpers/env_mutation.rs index bedb083b0..9486a94ca 100644 --- a/tests/bdd/helpers/env_mutation.rs +++ b/tests/bdd/helpers/env_mutation.rs @@ -3,11 +3,6 @@ //! Provides shared utilities for safely mutating process-global environment //! variables within BDD scenarios using the `EnvLock` serialization mechanism. -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #492 (rstest-bdd migration)" -)] - use crate::bdd::fixtures::TestWorld; use crate::bdd::types::EnvVarKey; use anyhow::{Result, ensure}; @@ -32,6 +27,10 @@ use std::ffi::OsString; /// /// Returns an error if the environment variable name is empty, contains '=', /// or contains `'\0'`, or if `new_value` contains `'\0'`. +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] pub fn mutate_env_var(world: &TestWorld, key: EnvVarKey, new_value: Option<&str>) -> Result<()> { ensure!( !key.as_str().is_empty(), @@ -85,6 +84,19 @@ mod tests { expect_present: bool, } + /// Read a variable back to confirm the mutation under test took effect. + /// + /// The expectation lives here rather than on the test because `rstest` + /// generates one function per case and the attribute does not survive that + /// expansion; a single narrow helper is tighter than a module-wide waiver. + #[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" + )] + fn read_back(key: &str) -> Result { + std::env::var(key) + } + #[rstest] #[case::empty_key(MutationTestCase { key: "", new_value: None, expect_error: true, expect_present: false })] #[case::key_with_equals(MutationTestCase { key: "KEY=VALUE", new_value: Some("test"), expect_error: true, expect_present: false })] @@ -121,7 +133,7 @@ mod tests { if tc.expect_present { assert_eq!( - std::env::var(tc.key).ok().as_deref(), + read_back(tc.key).ok().as_deref(), tc.new_value, "variable should be set to expected value" ); @@ -130,7 +142,7 @@ mod tests { .expect("cleanup should succeed"); } else if !tc.key.is_empty() { assert!( - std::env::var(tc.key).is_err(), + read_back(tc.key).is_err(), "variable should have been removed" ); } diff --git a/tests/bdd/steps/conditional_manifest.rs b/tests/bdd/steps/conditional_manifest.rs index 7c293ef43..ce621796a 100644 --- a/tests/bdd/steps/conditional_manifest.rs +++ b/tests/bdd/steps/conditional_manifest.rs @@ -10,11 +10,6 @@ //! still letting later assertions inspect the command outputs and environment //! mutations created by the manifest under test. -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #492 (rstest-bdd migration)" -)] - use crate::bdd::fixtures::TestWorld; use anyhow::{Context, Result}; use rstest_bdd_macros::given; @@ -111,6 +106,10 @@ fn mark_executable(_path: &Path) -> Result<()> { Ok(()) } +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] fn prepend_path_for_child(world: &TestWorld, dir: &Path) -> Result<()> { let mut entries = vec![dir.to_path_buf()]; if let Some(host_path) = std::env::var_os("PATH") { diff --git a/tests/bdd/steps/fs.rs b/tests/bdd/steps/fs.rs index ea21e28d0..5e5e5b7c9 100644 --- a/tests/bdd/steps/fs.rs +++ b/tests/bdd/steps/fs.rs @@ -1,10 +1,5 @@ //! Steps for preparing file-system fixtures used in Jinja tests (Unix only). -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #492 (rstest-bdd migration)" -)] - use crate::bdd::fixtures::TestWorld; use anyhow::{Context, Result, anyhow, ensure}; use camino::Utf8PathBuf; @@ -102,6 +97,10 @@ fn create_device_with_fallback(config: DeviceConfig<'_>) -> Result Ok(fallback) } +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] fn setup_environment_variables( world: &TestWorld, root: &Utf8PathBuf, diff --git a/tests/bdd/steps/manifest/mod.rs b/tests/bdd/steps/manifest/mod.rs index 1527b098f..828905382 100644 --- a/tests/bdd/steps/manifest/mod.rs +++ b/tests/bdd/steps/manifest/mod.rs @@ -5,11 +5,6 @@ //! - `helpers.rs` - Typed assertion utilities and target accessor functions //! - `targets.rs` - Target-specific assertion steps -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #492 (rstest-bdd migration)" -)] - mod helpers; mod targets; @@ -120,6 +115,10 @@ fn assert_parsed(world: &TestWorld) -> Result<()> { // Environment variable helpers // --------------------------------------------------------------------------- +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] fn parse_env_token(chars: &mut std::iter::Peekable) -> String where I: Iterator, diff --git a/tests/bdd/steps/manifest_command_helpers.rs b/tests/bdd/steps/manifest_command_helpers.rs index 638f30d91..f459e77b8 100644 --- a/tests/bdd/steps/manifest_command_helpers.rs +++ b/tests/bdd/steps/manifest_command_helpers.rs @@ -3,11 +3,6 @@ //! Split from `manifest_command.rs` so both files stay within the module size //! budget; the step definitions in the parent module call these helpers. -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #492 (rstest-bdd migration)" -)] - use super::OutputType; use crate::bdd::fixtures::TestWorld; use crate::bdd::helpers::assertions::{assert_slot_contains, normalize_fluent_isolates}; @@ -144,6 +139,10 @@ pub(super) fn netsuke_executable() -> Result { /// - Controlled `PATH` variable /// /// Returns the configured command ready for execution. +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] pub(super) fn build_netsuke_command( world: &TestWorld, args: &[&str], diff --git a/tests/bdd/steps/stdlib/workspace.rs b/tests/bdd/steps/stdlib/workspace.rs index 575818c22..73bc5293c 100644 --- a/tests/bdd/steps/stdlib/workspace.rs +++ b/tests/bdd/steps/stdlib/workspace.rs @@ -1,11 +1,6 @@ //! Helpers for preparing stdlib workspaces during BDD scenarios, wiring //! up temporary directories, fixtures, and environment overrides for tests. -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #492 (rstest-bdd migration)" -)] - use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use crate::bdd::types::{FileContents, HelperName, HttpResponseBody, PathEntries}; use anyhow::{Context, Result, anyhow}; @@ -290,6 +285,10 @@ pub(crate) fn stdlib_path_entries(world: &TestWorld, entries: &str) -> Result<() } #[given("HOME points to the stdlib workspace root")] +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #492 (rstest-bdd migration)" +)] pub(crate) fn home_points_to_stdlib_root(world: &TestWorld) -> Result<()> { let root = ensure_workspace(world)?; let os_root = OsStr::new(root.as_str()); diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index 967881377..157e4d8f2 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -1,11 +1,6 @@ //! Tests for scoped manipulation of `PATH` via `prepend_dir_to_path` and //! `PathGuard`. -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #493 (integration-binary migration)" -)] - use anyhow::{Context, Result, ensure}; use mockable::Env; use rstest::rstest; @@ -15,6 +10,10 @@ use test_support::env::{VarGuard, mocked_path_env, prepend_dir_to_path, system_e #[rstest] #[serial] +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] fn prepend_dir_to_path_sets_and_restores() -> Result<()> { let env = mocked_path_env(); let original = env.raw("PATH").context("mock PATH should be set")?; @@ -42,6 +41,10 @@ fn prepend_dir_to_path_sets_and_restores() -> Result<()> { #[rstest] #[serial] +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] fn prepend_dir_to_path_handles_empty_path() -> Result<()> { let _path_guard = VarGuard::set("PATH", OsStr::new("")); let env = system_env(); @@ -66,6 +69,10 @@ fn prepend_dir_to_path_handles_empty_path() -> Result<()> { #[rstest] #[serial] +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] fn prepend_dir_to_path_handles_missing_path() -> Result<()> { let _path_guard = VarGuard::unset("PATH"); let env = system_env(); diff --git a/tests/env_restore_tests.rs b/tests/env_restore_tests.rs index 7625df66a..da76f393e 100644 --- a/tests/env_restore_tests.rs +++ b/tests/env_restore_tests.rs @@ -4,11 +4,6 @@ //! `EnvLock`. Tests verify that set variables are restored, absent variables //! are removed, and empty snapshots are handled gracefully. -#![expect( - clippy::disallowed_methods, - reason = "covers the guards being retired; deleted under #493 (integration-binary migration)" -)] - use anyhow::{Result, ensure}; use rstest::rstest; use std::collections::HashMap; @@ -21,6 +16,10 @@ fn test_var(suffix: &str) -> String { } #[rstest] +#[expect( + clippy::disallowed_methods, + reason = "covers the guards being retired; deleted under #493 (integration-binary migration)" +)] fn restore_many_restores_previously_set_variable() -> Result<()> { let key = test_var("SET"); let _previous = set_var(&key, std::ffi::OsStr::new("original")); @@ -47,6 +46,10 @@ fn restore_many_restores_previously_set_variable() -> Result<()> { } #[rstest] +#[expect( + clippy::disallowed_methods, + reason = "covers the guards being retired; deleted under #493 (integration-binary migration)" +)] fn restore_many_removes_variable_when_prior_value_is_none() -> Result<()> { let key = test_var("NONE"); let _ = set_var(&key, std::ffi::OsStr::new("transient")); @@ -70,6 +73,10 @@ fn restore_many_handles_empty_map() { } #[rstest] +#[expect( + clippy::disallowed_methods, + reason = "covers the guards being retired; deleted under #493 (integration-binary migration)" +)] fn restore_many_restores_multiple_variables() -> Result<()> { let key_a = test_var("MULTI_A"); let key_b = test_var("MULTI_B"); @@ -101,6 +108,10 @@ fn restore_many_restores_multiple_variables() -> Result<()> { } #[rstest] +#[expect( + clippy::disallowed_methods, + reason = "covers the guards being retired; deleted under #493 (integration-binary migration)" +)] fn restore_many_mixed_set_and_remove() -> Result<()> { let key_set = test_var("MIX_SET"); let key_remove = test_var("MIX_REMOVE"); diff --git a/tests/ninja_env_tests.rs b/tests/ninja_env_tests.rs index 61161a284..cb4710df9 100644 --- a/tests/ninja_env_tests.rs +++ b/tests/ninja_env_tests.rs @@ -1,10 +1,5 @@ //! Tests for overriding the `NINJA_ENV` variable via a mock environment. -#![expect( - clippy::disallowed_methods, - reason = "pending migration under #493 (integration-binary migration)" -)] - use anyhow::{Context, Result, ensure}; use mockable::MockEnv; use netsuke::runner::NINJA_ENV; @@ -20,6 +15,10 @@ fn ninja_tmp() -> PathBuf { #[rstest] #[serial] +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] fn override_ninja_env_sets_and_restores(ninja_tmp: PathBuf) -> Result<()> { let before = std::env::var_os(NINJA_ENV); let original = before @@ -48,6 +47,10 @@ fn override_ninja_env_sets_and_restores(ninja_tmp: PathBuf) -> Result<()> { #[rstest] #[serial] +#[expect( + clippy::disallowed_methods, + reason = "pending migration under #493 (integration-binary migration)" +)] fn override_ninja_env_unset_removes_variable(ninja_tmp: PathBuf) -> Result<()> { let before = std::env::var_os(NINJA_ENV); let mut env = MockEnv::new(); diff --git a/tests/std_filter_tests/command_filters/windows_filter_tests.rs b/tests/std_filter_tests/command_filters/windows_filter_tests.rs index 0387c81bb..db28d9e59 100644 --- a/tests/std_filter_tests/command_filters/windows_filter_tests.rs +++ b/tests/std_filter_tests/command_filters/windows_filter_tests.rs @@ -81,6 +81,10 @@ impl WindowsSetupContext { } } +#[expect( + clippy::disallowed_methods, + reason = "the code under test resolves commands through the real process PATH, so the helper directory must be prepended to the inherited value rather than to an injected one; pending migration under #493 (integration-binary migration)" +)] fn windows_command_setup( ctx: WindowsSetupContext, helper_name: &str, From ec0c232e63e85e2f66f975227f17211943c84c2f Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 4 Aug 2026 13:56:49 +0200 Subject: [PATCH 5/7] Say what a source checkout supplies to the build The install section named the pinned nightly toolchain but not the `RUSTFLAGS=-Zpolonius=next` that `.cargo/config.toml` also supplies, so a reader could not tell why the registry install needs both settings spelled out while `cargo install --path .` needs neither. State both inheritances explicitly, and make the source-install example self-contained by saying why it passes no toolchain or flags. Drop the comma before "because". Addresses a CodeRabbit finding on #505. --- docs/users-guide.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/users-guide.md b/docs/users-guide.md index 324907b3a..78d071339 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -11,9 +11,12 @@ change before 1.0. Pin the Netsuke version in automated workflows. ## Install Netsuke Netsuke requires [Ninja](https://ninja-build.org/) on `PATH`. A source build -also requires the dated Rust nightly toolchain pinned in `rust-toolchain.toml`, -because Netsuke builds with the Polonius borrow checker (`-Zpolonius=next`); -`rustup` installs it automatically inside a checkout. +also requires the dated Rust nightly toolchain pinned in `rust-toolchain.toml` +because Netsuke builds with the Polonius borrow checker (`-Zpolonius=next`). + +Inside a checkout both settings are inherited automatically: `rustup` installs +the pinned toolchain, and the repository's `.cargo/config.toml` supplies +`RUSTFLAGS=-Zpolonius=next`. Neither has to be passed on the command line. Netsuke v0.1.0 is available from crates.io. Where [`cargo binstall`](https://github.com/cargo-bins/cargo-binstall) is available, @@ -67,7 +70,9 @@ licence files. Installer packages do not have checksum sidecars in v0.1.0. Windows PowerShell help files are published beside each MSI as sidecar artefacts rather than embedded in the installer. -To install the current source checkout with Cargo: +To install the current source checkout with Cargo. The clone supplies both the +pinned nightly toolchain and `RUSTFLAGS=-Zpolonius=next`, so neither is given +here — unlike the registry install above, which runs outside a checkout: From d181d56f415538ec6a84677b902b9ade949c7b0a Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 4 Aug 2026 14:43:24 +0200 Subject: [PATCH 6/7] State the Windows PATH requirement in the expectation reason The reason now says plainly what the annotation licences: the test must read the inherited Windows PATH to prepend its temporary helper directory, because the code under test resolves commands through the real process environment and would never consult an injected value. The lint gate for this code cannot be run here or in CI. The module is `#[cfg(windows)]`, cross-checking it needs a Windows C toolchain that `ring`'s build script requires and this host lacks, and the CI lint job runs on ubuntu-latest only. That Ubuntu-only coverage is exactly why the unannotated lookup survived until now. Addresses a CodeRabbit finding on #505. --- tests/std_filter_tests/command_filters/windows_filter_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std_filter_tests/command_filters/windows_filter_tests.rs b/tests/std_filter_tests/command_filters/windows_filter_tests.rs index db28d9e59..1808d6fe9 100644 --- a/tests/std_filter_tests/command_filters/windows_filter_tests.rs +++ b/tests/std_filter_tests/command_filters/windows_filter_tests.rs @@ -83,7 +83,7 @@ impl WindowsSetupContext { #[expect( clippy::disallowed_methods, - reason = "the code under test resolves commands through the real process PATH, so the helper directory must be prepended to the inherited value rather than to an injected one; pending migration under #493 (integration-binary migration)" + reason = "the test must read the inherited Windows PATH so it can prepend its temporary helper directory: the code under test resolves commands through the real process environment, so an injected value would not be consulted; pending migration under #493 (integration-binary migration)" )] fn windows_command_setup( ctx: WindowsSetupContext, From 5580a5b12f2b7b4ae731263b4d5cd48d9b5dbc80 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 4 Aug 2026 20:26:35 +0200 Subject: [PATCH 7/7] Annotate the sanctioned environment sites in test_support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #478 added a second Clippy invocation for `test_support`, which the root workspace run cannot reach. That closed the enforcement gap this PR's `test_support/clippy.toml` was written for — and exposed the 27 sanctioned sites the file had never been able to see. These are the crate's process-environment boundary, in the same sense that `test_support::fs` is its filesystem boundary: `env`, `env_guard`, and `env_var_guard` exist to set and restore real variables so subprocess and legacy in-process tests can run, which AGENTS.md sanctions. `netsuke` and `command_helper` read the inherited environment to build a subprocess invocation; `sandbox::which` resolves through the real PATH because that is what the code under test must observe; `http::duration_from_env` reads an ambient tuning value with no seam to reach it. Each site takes an item-level `#[expect]` naming why no seam applies, following this PR's own policy: composition roots get a permanent site-level expectation, not a module-level one. `expect` rather than `allow`, so a site that later gains a seam fails the gate until its annotation is removed. Refs #504. --- test_support/src/command_helper.rs | 4 ++++ test_support/src/dev_fast/sandbox.rs | 4 ++++ test_support/src/env.rs | 32 ++++++++++++++++++++++++++++ test_support/src/env_guard.rs | 8 +++++++ test_support/src/env_var_guard.rs | 8 +++++++ test_support/src/http.rs | 4 ++++ test_support/src/netsuke.rs | 8 +++++++ 7 files changed, 68 insertions(+) diff --git a/test_support/src/command_helper.rs b/test_support/src/command_helper.rs index 9705c100e..a10288195 100644 --- a/test_support/src/command_helper.rs +++ b/test_support/src/command_helper.rs @@ -120,6 +120,10 @@ pub fn compile_large_output_helper( /// .expect("compile helper"); /// assert!(exe.as_std_path().exists()); /// ``` +#[expect( + clippy::disallowed_methods, + reason = "reads the inherited environment to invoke rustc for a helper binary; the compiler must see the real toolchain" +)] pub fn compile_rust_helper( dir: &Dir, root: &Utf8PathBuf, diff --git a/test_support/src/dev_fast/sandbox.rs b/test_support/src/dev_fast/sandbox.rs index 2b4a0c95b..ad29ae33e 100644 --- a/test_support/src/dev_fast/sandbox.rs +++ b/test_support/src/dev_fast/sandbox.rs @@ -275,6 +275,10 @@ pub fn real_utility(utility: &str) -> Result { /// sourceable `env` shell fragment into `~/.local/bin`, so a plain file probe /// selects a non-executable file and every sandboxed `env` invocation then /// fails with a permission error. +#[expect( + clippy::disallowed_methods, + reason = "resolves a tool through the real PATH, which is what the sandbox under test must observe" +)] fn which(utility: &str) -> Result { let path = std::env::var_os("PATH").context("read PATH")?; for dir in std::env::split_paths(&path) { diff --git a/test_support/src/env.rs b/test_support/src/env.rs index dcc9979e9..a645216f0 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -43,12 +43,20 @@ pub trait EnvMut: Env { } impl EnvMut for DefaultEnv { + #[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" + )] unsafe fn set_var(&self, key: &str, value: &OsStr) { unsafe { std::env::set_var(key, value) }; } } impl EnvMut for MockEnv { + #[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" + )] unsafe fn set_var(&self, key: &str, value: &OsStr) { unsafe { std::env::set_var(key, value) }; } @@ -59,6 +67,10 @@ impl EnvMut for MockEnv { /// Returns a `MockEnv` that yields the current `PATH` when queried. Tests can /// modify the real environment while the mock continues to expose the initial /// value. +#[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" +)] pub fn mocked_path_env() -> MockEnv { let original = std::env::var("PATH").unwrap_or_default(); let mut env = MockEnv::new(); @@ -72,6 +84,10 @@ pub fn mocked_path_env() -> MockEnv { /// /// The mutation is `unsafe` in Rust 2024 as it alters process state. The /// unsafety is scoped by acquiring [`EnvLock`]. +#[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" +)] pub fn set_var(key: &str, value: &OsStr) -> Option { let _lock = EnvLock::acquire(); let previous = std::env::var_os(key); @@ -81,6 +97,10 @@ pub fn set_var(key: &str, value: &OsStr) -> Option { } /// Set an environment variable while the caller already holds [`EnvLock`]. +#[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" +)] pub fn set_var_locked(lock: &EnvLock, key: &str, value: &OsStr) -> Option { let _ = lock; let previous = std::env::var_os(key); @@ -93,6 +113,10 @@ pub fn set_var_locked(lock: &EnvLock, key: &str, value: &OsStr) -> Option Option { let _lock = EnvLock::acquire(); let previous = std::env::var_os(key); @@ -136,6 +160,10 @@ pub fn restore_many(vars: HashMap>) { /// # Safety /// /// Caller must hold [`EnvLock`] for the duration of this call. +#[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" +)] pub unsafe fn restore_many_locked(vars: HashMap>) { for (key, val) in vars { if let Some(v) = val { @@ -156,6 +184,10 @@ pub struct VarGuard { /// Run `action` with `PATH` temporarily set to `value`, restoring on drop and /// serialising mutations with `EnvLock`. +#[expect( + clippy::disallowed_methods, + reason = "test_support::env is the crate's process-environment boundary: these guards exist to set and restore real variables for subprocess and legacy in-process tests, which AGENTS.md sanctions" +)] pub fn with_isolated_path(value: &OsStr, action: impl FnOnce() -> T) -> T { let _lock = EnvLock::acquire(); let original = std::env::var_os("PATH"); diff --git a/test_support/src/env_guard.rs b/test_support/src/env_guard.rs index 890210c65..845e4d4bc 100644 --- a/test_support/src/env_guard.rs +++ b/test_support/src/env_guard.rs @@ -33,10 +33,18 @@ pub trait Environment { pub struct StdEnv; impl Environment for StdEnv { + #[expect( + clippy::disallowed_methods, + reason = "the guard's whole purpose is to mutate and restore a real process variable; injecting here would leave nothing to guard" + )] unsafe fn set_var(&mut self, key: &str, value: &OsStr) { unsafe { std::env::set_var(key, value) }; } + #[expect( + clippy::disallowed_methods, + reason = "the guard's whole purpose is to mutate and restore a real process variable; injecting here would leave nothing to guard" + )] unsafe fn remove_var(&mut self, key: &str) { unsafe { std::env::remove_var(key) }; } diff --git a/test_support/src/env_var_guard.rs b/test_support/src/env_var_guard.rs index 157d797f1..8a6032f37 100644 --- a/test_support/src/env_var_guard.rs +++ b/test_support/src/env_var_guard.rs @@ -37,6 +37,10 @@ impl EnvVarGuard { /// Mutating process-global state is `unsafe` in Rust 2024. Callers must hold /// an [`EnvLock`](crate::env_lock::EnvLock) to serialise mutations. #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "the guard's whole purpose is to mutate and restore a real process variable; injecting here would leave nothing to guard" + )] pub fn set(name: impl Into>, val: impl AsRef) -> Self { let name = name.into(); let prev = std::env::var_os(&*name); @@ -54,6 +58,10 @@ impl EnvVarGuard { /// Callers must hold an [`EnvLock`](crate::env_lock::EnvLock) to serialise /// mutations of the process environment. #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "the guard's whole purpose is to mutate and restore a real process variable; injecting here would leave nothing to guard" + )] pub fn remove(name: impl Into>) -> Self { let name = name.into(); let prev = std::env::var_os(&*name); diff --git a/test_support/src/http.rs b/test_support/src/http.rs index 27b50b941..b8110af92 100644 --- a/test_support/src/http.rs +++ b/test_support/src/http.rs @@ -276,6 +276,10 @@ fn write_response(stream: &mut TcpStream, body: &str) { let _ = stream.write_all(response.as_bytes()); } +#[expect( + clippy::disallowed_methods, + reason = "reads an ambient tuning variable for the stub HTTP server; the fixture is configured by the environment it runs in, and no seam reaches it" +)] fn duration_from_env(var: &str, default: Duration) -> Duration { match env::var(var) { Ok(value) => { diff --git a/test_support/src/netsuke.rs b/test_support/src/netsuke.rs index 444860822..212b903a1 100644 --- a/test_support/src/netsuke.rs +++ b/test_support/src/netsuke.rs @@ -13,6 +13,10 @@ use std::path::PathBuf; /// /// Prefer `CARGO_BIN_EXE_netsuke` when available, otherwise fall back to a /// `target/(debug|release)`-derived path based on the current test binary. +#[expect( + clippy::disallowed_methods, + reason = "reads the inherited environment to build the subprocess invocation; assert_cmd subprocess isolation is the sanctioned exemption in AGENTS.md" +)] fn netsuke_executable() -> Result { if let Some(path) = std::env::var_os("CARGO_BIN_EXE_netsuke") { return Ok(path.into()); @@ -90,6 +94,10 @@ pub fn run_netsuke_in(current_dir: &Path, args: &[&str]) -> Result { /// /// Returns an error when `netsuke` cannot be located or the process cannot be /// spawned. +#[expect( + clippy::disallowed_methods, + reason = "reads the inherited environment to build the subprocess invocation; assert_cmd subprocess isolation is the sanctioned exemption in AGENTS.md" +)] pub fn run_netsuke_in_with_env( current_dir: &Path, args: &[&str],