diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ab69f2c3..11882242e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,13 @@ on: types: [opened, synchronize, reopened] workflow_dispatch: +env: + # Single source of truth for the cargo-nextest pin. `make test` runs the + # non-doctest suite through nextest, so every job that installs it reads + # this value. Declared once at workflow scope so the documented + # AGENTS.md `sed` extraction yields exactly one version. + NEXTEST_VERSION: '0.9.133' + jobs: build-test: runs-on: ubuntu-latest @@ -18,9 +25,6 @@ jobs: # the dated nightly pinned in rust-toolchain.toml. NETSUKE_RUST_TOOLCHAIN: nightly-2026-06-25 WHITAKER_INSTALLER_VERSION: '0.2.7' - # Single source of truth for the cargo-nextest pin. `make test` runs the - # non-doctest suite through nextest, so the job installs it up front. - NEXTEST_VERSION: '0.9.133' steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -48,7 +52,6 @@ jobs: uses: leynos/shared-actions/.github/actions/setup-rust@8add2d99854a5b77548eae98cca59202e68fefc8 with: toolchain: ${{ env.NETSUKE_RUST_TOOLCHAIN }} - components: rustfmt, clippy # Preserve warnings-as-errors and Polonius through toolchain setup. rustflags: -D warnings -Zpolonius=next - name: Install cargo-nextest @@ -88,7 +91,10 @@ jobs: uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 with: python-version: '3.13' - enable-cache: true + # This Rust workspace has no Python dependency manifest. Cache the + # shared spelling dictionary below rather than a uv cache with no + # durable dependency key. + enable-cache: 'false' - name: Cache shared spelling dictionary uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: @@ -118,12 +124,105 @@ jobs: CS_ACCESS_TOKEN: ${{ secrets.CS_ACCESS_TOKEN }} uses: leynos/shared-actions/.github/actions/upload-codescene-coverage@8add2d99854a5b77548eae98cca59202e68fefc8 with: + path: lcov.info format: lcov mode: check project-url: https://api.codescene.io/v2/projects/69281 access-token: ${{ env.CS_ACCESS_TOKEN }} installer-checksum: ${{ vars.CODESCENE_CLI_SHA256 }} + build-test-windows: + # Gates merges: the `#[cfg(windows)]` tree is compiled, linted, and tested + # under `-D warnings` on this platform (see #518). + runs-on: windows-latest + permissions: + contents: read + env: + CARGO_TERM_COLOR: always + BUILD_PROFILE: debug + # The tree requires -Zpolonius=next (see + # docs/adr-006-adopt-polonius-nightly-toolchain.md), so CI builds with + # the dated nightly pinned in rust-toolchain.toml. + NETSUKE_RUST_TOOLCHAIN: nightly-2026-06-25 + WHITAKER_INSTALLER_VERSION: '0.2.7' + defaults: + run: + # The Makefile uses POSIX shell constructs throughout; Git Bash is + # preinstalled on windows-latest. GNU Make's default recipe shell on + # Windows is cmd.exe, so every make invocation overrides SHELL to bash. + shell: bash + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Install GNU Make + run: choco install make --yes --no-progress + - name: Setup Rust + uses: leynos/shared-actions/.github/actions/setup-rust@8add2d99854a5b77548eae98cca59202e68fefc8 + with: + toolchain: ${{ env.NETSUKE_RUST_TOOLCHAIN }} + # Preserve warnings-as-errors and Polonius through toolchain setup. + rustflags: -D warnings -Zpolonius=next + - name: Install Ninja + uses: seanmiddleditch/gha-setup-ninja@3b1f8f94a2f8254bd26914c4ab9474d4f0015f67 # v6 + - name: Install cargo-nextest + uses: taiki-e/install-action@18b1216eba7f8039b0f8d131d5473787f0edce68 # v2.85.3 + with: + tool: nextest@${{ env.NEXTEST_VERSION }} + - name: Show rustc version + run: | + rustup show + rustc --version + cargo --version + - name: Show Ninja version + run: ninja --version + - name: Format + run: make SHELL=bash check-fmt + - name: Lint (Clippy) + # Clippy and `cargo doc` over the whole workspace under `-D warnings`, + # including the `#[cfg(windows)]` arms. + run: make SHELL=bash lint-clippy + - name: Cache Whitaker installer + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: | + ~/.cargo/bin/whitaker-installer + ~/.cache/cargo-binstall + key: whitaker-installer-${{ runner.os }}-${{ runner.arch }}-${{ env.WHITAKER_INSTALLER_VERSION }} + - name: Install Whitaker + # Installs and runs on windows-latest (verified in #562); the same + # binstall-with-cargo-install-fallback path as the Linux job. On + # Windows the installer ships `whitaker` as a PowerShell wrapper + # (.ps1), which Git Bash cannot execute, so shim `whitaker` in the + # cargo bin directory (already on PATH) to run the wrapper through + # PowerShell. A failure here blocks the merge. + run: | + if ! command -v whitaker-installer >/dev/null 2>&1; then + if cargo binstall --version >/dev/null 2>&1; then + cargo binstall --no-confirm --locked "whitaker-installer@${WHITAKER_INSTALLER_VERSION}" + else + echo "cargo-binstall unavailable; building whitaker-installer from crates.io" + cargo install --locked whitaker-installer --version "${WHITAKER_INSTALLER_VERSION}" + fi + fi + whitaker-installer + if [ -f "${HOME}/.local/bin/whitaker.ps1" ]; then + printf '%s\n' \ + '#!/bin/bash' \ + 'exec powershell -NoProfile -ExecutionPolicy Bypass -File "${HOME}/.local/bin/whitaker.ps1" "$@"' \ + > "${CARGO_HOME:-$HOME/.cargo}/bin/whitaker" + chmod +x "${CARGO_HOME:-$HOME/.cargo}/bin/whitaker" + fi + - name: Lint (Whitaker) + # Whitaker/Dylint over the workspace under `-D warnings`, including + # the `#[cfg(windows)]` arms. A failure blocks the merge. + run: make SHELL=bash lint-whitaker + - name: Test + # cargo-nextest plus doctests under `-D warnings -Zpolonius=next`, + # compiling and running the `#[cfg(windows)]` test tree. A failure + # blocks the merge. + run: make SHELL=bash test + kani-smoke: if: github.event_name == 'pull_request' runs-on: ubuntu-latest @@ -145,6 +244,10 @@ jobs: toolchain: stable - name: Install uv uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 + with: + # The Kani job has no Python dependency manifest; its explicit cache + # below owns the Rust toolchain artefacts. + enable-cache: false - name: Cache Kani tools uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: diff --git a/.github/workflows/coverage-main.yml b/.github/workflows/coverage-main.yml index 2e3be88b6..8c86175e1 100644 --- a/.github/workflows/coverage-main.yml +++ b/.github/workflows/coverage-main.yml @@ -29,7 +29,6 @@ jobs: # Match rust-toolchain.toml: the tree needs -Zpolonius=next (see # docs/adr-006-adopt-polonius-nightly-toolchain.md). toolchain: nightly-2026-06-25 - components: rustfmt, clippy # Preserve warnings-as-errors and Polonius through toolchain setup; # cargo-llvm-cov appends its instrumentation flags to this value. rustflags: -D warnings -Zpolonius=next @@ -45,6 +44,7 @@ jobs: if: env.CS_ACCESS_TOKEN != '' uses: leynos/shared-actions/.github/actions/upload-codescene-coverage@8add2d99854a5b77548eae98cca59202e68fefc8 with: + path: lcov.info format: lcov access-token: ${{ env.CS_ACCESS_TOKEN }} installer-checksum: ${{ vars.CODESCENE_CLI_SHA256 }} diff --git a/Cargo.lock b/Cargo.lock index d55c08b97..8eca53c7d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1582,6 +1582,7 @@ dependencies = [ "clap_complete", "clap_mangen", "digest 0.11.3", + "dunce", "fluent-bundle", "fs4", "glob", diff --git a/Cargo.toml b/Cargo.toml index ef2e0405b..9496f4a69 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,6 +101,7 @@ cap-primitives = "3.4.4" cap-std = { version = "3.4.4", features = ["fs_utf8"] } fs4 = "1.1.0" camino = "1.2.0" +dunce = "1.0.5" semver = { version = "1", features = ["serde"] } anyhow = "1" indicatif = "0.18.4" diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 5a3034a6c..d29f704e2 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -429,14 +429,15 @@ action's `with.rustflags` input, and none of them may set a job-level value and silently drop the flag, so the tree would fail to borrow-check with a confusing `E0499` rather than an obvious configuration error. -Four workflows carry the contract: +Five CI jobs across four workflows carry the contract: -| Workflow | Job | Shared action | `with.rustflags` | -| --------------------------------------------------------------------- | ----------------- | -------------------- | ----------------------------- | -| [`ci.yml`](../.github/workflows/ci.yml) | `build-test` | `setup-rust` | `-D warnings -Zpolonius=next` | -| [`coverage-main.yml`](../.github/workflows/coverage-main.yml) | `coverage-upload` | `setup-rust` | `-D warnings -Zpolonius=next` | -| [`netsukefile-test.yml`](../.github/workflows/netsukefile-test.yml) | `netsukefile` | `setup-rust` | `-Zpolonius=next` | -| [`build-and-package.yml`](../.github/workflows/build-and-package.yml) | `build` | `rust-build-release` | `-Zpolonius=next` | +| Workflow | Job | Shared action | `with.rustflags` | +| --- | --- | --- | --- | +| [`ci.yml`](../.github/workflows/ci.yml) | `build-test` | `setup-rust` | `-D warnings -Zpolonius=next` | +| [`ci.yml`](../.github/workflows/ci.yml) | `build-test-windows` | `setup-rust` | `-D warnings -Zpolonius=next` | +| [`coverage-main.yml`](../.github/workflows/coverage-main.yml) | `coverage-upload` | `setup-rust` | `-D warnings -Zpolonius=next` | +| [`netsukefile-test.yml`](../.github/workflows/netsukefile-test.yml) | `netsukefile` | `setup-rust` | `-Zpolonius=next` | +| [`build-and-package.yml`](../.github/workflows/build-and-package.yml) | `build` | `rust-build-release` | `-Zpolonius=next` | CI and coverage add `-D warnings` because those jobs gate on a warning-free build; the Netsukefile and packaging jobs carry the Polonius flag alone, so a @@ -451,7 +452,7 @@ their toolchain through the action's own `toolchain` input and a second, independently edited pin would let the two disagree. [`tests/polonius_toolchain_contract.rs`](../tests/polonius_toolchain_contract.rs) -enforces all four callers. For each one it asserts: +enforces all five callers. For each one it asserts: - the job uses the expected shared-action reference — path *and* pinned revision, the latter derived from the checked workflows themselves rather @@ -1964,6 +1965,19 @@ is not obvious from the name: regular file with any execute bit set, and `false` for an absent or unreadable path. It is the inverse of `set_mode`, and exists for probing a sandbox `PATH` the way an executable lookup would. +- `canonicalize(path: &Utf8Path) -> io::Result` is the deliberate + ambient boundary for fixture paths. It delegates to `std::fs::canonicalize`: + `cap_std::fs::Dir::canonicalize` is scoped to a directory handle and returns + a relative path, so it cannot provide the absolute canonical spelling needed + for fixtures in an ambient temporary directory. The helper propagates the + underlying I/O error and returns `io::ErrorKind::InvalidData` when the + canonical path cannot be represented as UTF-8; callers must not hide that + failure with lossy conversion. Because the operation is host-native, tests + that compare native path identity should use this helper, including when + Windows short-name and long-name spellings refer to the same file; identity + follows the filesystem's canonical form rather than handwritten separator + or string normalization. Keep this exception in `test_support::fs`; production + code remains capability-scoped or uses its dedicated normalizer. - `copy(from, to) -> io::Result` forwards to `std::fs::copy`, returning the number of bytes copied and propagating its failure. The `dev_fast` release fixtures use it to place a built archive under its versioned name. @@ -2001,6 +2015,22 @@ claim to model arbitrary scheduler or filesystem interleavings. The fallible `test_support::fs::inspect_path` probe treats `NotFound` as absence and propagates every other metadata error. +### Temporary Ninja build files + +`runner::process::create_temp_ninja_file` writes, flushes, and synchronizes a +generated Ninja file, then converts the `NamedTempFile` into a +`tempfile::TempPath`. Returning `TempPath` is deliberate: it retains automatic +cleanup while releasing the writer before Ninja reopens the file by path. On +Windows, leaving the original writer open can make Ninja's read fail. Keep the +returned `TempPath` alive until the Ninja invocation completes; dropping it +removes the temporary file. + +The regression test +`create_temp_ninja_file_releases_writer_before_external_read` is the lifecycle +contract. It opens the returned path through an independent handle, reads it +back, and checks its contents, length, and `.ninja` suffix. Changes to the +helper must preserve that writer-release and path-lifetime behaviour. + ### Shared Makefile contract helpers `tests/support/makefile.rs` is a shared module for integration tests that @@ -2594,8 +2624,12 @@ Configuration merge helpers: diagnostics for the diagnostic and merge callers. - `push_discovered_file_layers(composer, errors, discovered) -> ()` transfers the retained layers and discovery errors into the full merge composition. -- `collect_file_layers_with_trace_and_env_source(directory, env_source)` runs - the one discovery pass and retains bounded project-scope trace metadata. +- `collect_file_layers_with_normalizer_and_trace(directory, normalizer, + env_source)` runs the one discovery pass with the injected path normalizer + and environment source, and retains bounded project-scope trace metadata for + deferred diagnostics. The normalizer canonicalizes comparison keys so + project layers are de-duplicated across equivalent path spellings; + `DiscoveryOutcome::emit_diagnostics()` is the production emission boundary. - `resolve_json_and_layers_outcome_with_env(cli, matches, env)` retains the `DiscoveryOutcome` so startup can emit diagnostics after tracing setup and @@ -2800,11 +2834,30 @@ signature again. Pinning both is what lets a behavioural test drive `which` and `tests/stdlib_which_pathext_tests.rs`, which is gated to Windows because `PATHEXT` governs resolution only there. -That gating has a cost worth stating: CI runs `make test` on `ubuntu-latest` -only, so a `#[cfg(windows)]` test does not gate a merge. Keep host-independent -rules — normalization, the fallback — in the `#[cfg(any(windows, test))]` unit -tests that the Linux suite executes, and reserve the Windows-gated suite for -behaviour that genuinely cannot run elsewhere. +That gating has a cost worth stating: the Windows-gated suite runs only on +`build-test-windows`, so keep host-independent rules — normalization, the +fallback — in the `#[cfg(any(windows, test))]` unit tests that every host +executes, and reserve the Windows-gated suite for behaviour that genuinely +cannot run elsewhere. + +The `build-test-windows` job in `.github/workflows/ci.yml` is a merge gate: it +compiles, lints (Clippy and Whitaker), and tests the `#[cfg(windows)]` suite on +`windows-latest` under `-D warnings`, so a Windows-gated test or lint finding +blocks a merge. The split still stands: host-independent rules stay in the +`#[cfg(any(windows, test))]` unit tests so every host — including a developer +on Unix — exercises them, while the Windows-gated suite covers the behaviour +that only exists there. + +The Windows job installs GNU Make through Chocolatey and Ninja through the +setup action, then runs every Make target through Git Bash with `SHELL=bash`. +That override is required because GNU Make otherwise selects `cmd.exe` on +Windows, while Netsuke's recipes use POSIX shell syntax. It installs the +workflow-pinned `cargo-nextest`; the shared Rust setup action supplies +`rustfmt` and Clippy. `whitaker-installer` produces a PowerShell wrapper on +Windows, so the job adds a Bash shim that invokes it through PowerShell before +running `make SHELL=bash lint-whitaker`. To reproduce the platform gate, use a +Windows environment with those tools provisioned and run the four Windows Make +commands from the workflow in that order. #### `PATHEXT` normalization @@ -2829,6 +2882,17 @@ Composition rules: empty result would mean Windows treats nothing as executable, so `which` would report every command missing. +The widening was reassessed when `build-test-windows` began compiling and +testing the `#[cfg(windows)]` arm directly (#518): the original motivation for +`#[cfg(any(windows, test))]` — reaching the pure string logic from a CI host +that never compiled Windows — is gone, but reverting to `#[cfg(windows)]` +would drop Unix-host coverage of `parse_pathext`'s normalization, +de-duplication, and fallback rules, which `src/stdlib/which/pathext_tests.rs` +pins on every host. There is no equivalent Unix-side test for a Windows-only +function, so the widening stays: the pure string logic is exercised on both +Linux and Windows, and a Windows-gated regression cannot hide from the Unix +suite. + The full normalization contract, which the property tests in `src/stdlib/which/pathext_tests.rs` pin: @@ -2943,17 +3007,22 @@ split diagnostics, path comparison, and tests out of the main discovery flow: - `discovery_diagnostics.rs` — bounded tracing helpers (`path_hash`, `short_hash`, `debug_config_path`, `debug_optional_config_path`, - `warn_explicit_config_load_failed`) and the `ConfigLoadFailureKind` enum used - to classify a load failure without retaining error text. + `debug_project_layer_deduplication`, `warn_explicit_config_load_failed`) and + the `ConfigLoadFailureKind` enum used to classify a load failure without + retaining error text. The de-duplication event records discovered, project, + and appended layer counts after filtering without exposing paths. - `discovery_paths.rs` — `normalized_path_key` resolves a path to a comparable, canonicalized form and returns canonicalization errors to its caller. The discovery-side `comparison_key` fallback uses the original path - literally when resolution fails, continues discovery, and emits only the - normal append debug event. This lets relative or symlinked `--directory` + literally when resolution fails, continues discovery, and emits a bounded + post-filter layer-count event. This lets relative or symlinked `--directory` values match OrthoConfig's canonicalized layer paths without making an - unresolved path fatal. `FsPathNormalizer` is confined to this comparison - boundary: selectors remain pure path queries, OrthoConfig supplies the layer - path, and tracing remains at the orchestration boundary. + unresolved path fatal. `FsPathNormalizer` uses `dunce::canonicalize` to + mirror OrthoConfig's native Windows identity (without UNC-prefix or + short-name divergence); on other platforms it follows + `std::fs::canonicalize`. Keep it confined to this comparison boundary: + selectors remain pure path queries, OrthoConfig supplies the layer path, and + tracing remains at the orchestration boundary. - `discovery_event_assertions.rs` — shared test-only helpers: `capture_events` runs a closure under a TRACE capturing subscriber, `find_event` locates one emitted event by substring, and `EventAssertion` diff --git a/docs/execplans/3-11-2-discover-configuration-files-in-project-and-user-scopes.md b/docs/execplans/3-11-2-discover-configuration-files-in-project-and-user-scopes.md index b5e77347d..652669b89 100644 --- a/docs/execplans/3-11-2-discover-configuration-files-in-project-and-user-scopes.md +++ b/docs/execplans/3-11-2-discover-configuration-files-in-project-and-user-scopes.md @@ -150,8 +150,9 @@ The implementation must finish by marking roadmap item 3.11.2 done only after precedence over user scope as expected (2026-04-03). - Stage A analysis confirms that no code changes are needed in `src/cli/config_merge.rs` for discovery semantics—the implementation is - correct. The work required is test coverage and documentation alignment - (2026-04-03). + correct. Later hardening was made in the discovery-layer path comparison, + fallback de-duplication, and deferred diagnostic replay without changing that + merge seam (2026-04-03). ## Decision Log @@ -175,19 +176,21 @@ The implementation must finish by marking roadmap item 3.11.2 done only after rules. Rationale: The design document should record architectural decisions permanently, while the user guide can link to or summarize that contract in user-friendly terms. Date/Author: 2026-04-03 / implementation agent. -- Decision: Stage B (adjusting `config_discovery()`) is unnecessary because the +- Decision: Stage B requires no change to `config_discovery()` because the current implementation correctly uses OrthoConfig's default discovery order, - which gives project scope precedence over user scope. Rationale: Analysis of - `config_discovery()` and OrthoConfig documentation confirms the - implementation matches the intended contract. Date/Author: 2026-04-03 / - implementation agent. + which gives project scope precedence over user scope. Separate hardening in + the discovery layers adds `dunce` comparison, fallback de-duplication, and + deferred bounded diagnostics without changing that seam. Date/Author: + 2026-04-03 / implementation agent. ## Outcomes & Retrospective -### Completion summary +### Completion summary (historical milestone record) Roadmap item 3.11.2 is complete as of 2026-04-03. The discovery contract was -documented, integration tests were added, and all validation gates pass. +documented, integration tests were added, and the then-current validation gates +passed. Later follow-up changes are described above and require current +revision evidence from the focused and complete gates. ### Search order (documented in netsuke-design.md § 8.4.1) @@ -209,15 +212,19 @@ leaving user-scope lookup unchanged. ### Code changes -**None required.** The existing `config_discovery()` implementation in -`src/cli/config_merge.rs` correctly uses OrthoConfig's default discovery order, -which already provides the intended project-over-user precedence. The -implementation was verified as correct and needed no adjustment. +The implementation now canonicalizes project-path comparisons through `dunce`, +matching OrthoConfig's Windows short-name and long-name handling. When +canonical comparison is unavailable, the project-scope fallback pass still +de-duplicates layers by the available path identity. `ProjectScopeTrace` retains +only bounded project-path metadata and the discovered, project, and appended +layer counts. The de-duplication event is deferred until the production +`DiscoveryOutcome::emit_diagnostics` boundary, so collection does not emit a +diagnostic before tracing is configured. ### Tests added -1. **Integration tests** (`tests/cli_tests/config_discovery.rs`, 8 tests, all - passing): +1. **Integration tests** (`tests/cli_tests/config_discovery_overrides.rs` and + `tests/cli_tests/config_discovery_scopes.rs`): - `project_scope_config_discovered_automatically` - `user_scope_config_discovered_when_no_project_config` - `project_config_takes_precedence_over_user_config` @@ -227,7 +234,19 @@ implementation was verified as correct and needed no adjustment. - `config_path_env_var_bypasses_automatic_discovery` - `list_fields_append_across_discovered_config_env_and_cli` -2. **BDD scenarios** (`tests/features/configuration_discovery.feature` and +2. **Discovery diagnostics and layer tests** (`src/cli/discovery_layer_tests.rs`, + `src/cli/discovery_tracing_tests.rs`, and + `src/cli/discovery_replay_proptests.rs`): + - Deferred replay covers the project-layer de-duplication event and its + bounded layer counts without repeating discovery or environment access + - Unix alias and normalization-fallback cases preserve one project layer; + Windows coverage exercises native path identity + +3. **Canonicalization tests** (`test_support/src/canonicalize.rs`): + - Dot-component, missing-path (`NotFound`), Unix symlink, and Unix + non-UTF-8 (`InvalidData`) cases are covered + +4. **BDD scenarios** (`tests/features/configuration_discovery.feature` and `tests/bdd/steps/configuration_discovery.rs`, COMPLETED): - Feature file with 5 scenarios covering discovery and precedence - Step definitions for config file creation and environment setup @@ -238,23 +257,39 @@ implementation was verified as correct and needed no adjustment. ### Platform-specific constraints -None. OrthoConfig handles platform differences (Unix XDG paths vs. Windows -AppData directories) transparently. The tests use platform-agnostic temp -directories and $HOME overrides for reproducibility. +OrthoConfig handles scope selection (Unix XDG paths versus Windows AppData +directories) transparently, but path identity requires an explicit boundary +policy. On Windows, a temporary directory may be spelled with a short path +while OrthoConfig records a long canonical path. Discovery therefore +canonicalizes both sides with `dunce` before comparing them, and the fallback +pass de-duplicates the loaded project layers by their canonical path. This +prevents one physical `.netsuke.toml` from entering the merge chain twice. +The three bounded layer-count fields (discovered, project, and appended) are +retained for deferred diagnostic replay and emitted only through +`DiscoveryOutcome::emit_diagnostics`, not during collection. + +Unix tests exercise equivalent `.` and symlink aliases; Windows CI exercises +the native short/long-path spelling. The property test generates additional +project-directory spellings and requires one canonical layer. ### Validation results +The following results are historical milestone evidence captured on 2026-04-03; +the suite counts reflect that earlier repository state and are not current +results for the follow-up: + - `make check-fmt`: **PASS** - `make lint`: **PASS** (all Clippy warnings resolved) -- `cargo test --test cli_tests`: **PASS** (all 54 tests pass, including 8 config - discovery tests) -- `cargo test --test bdd_tests`: **PASS** (all 202 BDD scenarios pass, including - 5 new configuration discovery scenarios) -- `make test`: **PASS** (all 377 unit tests, 202 BDD scenarios, and 54 - integration tests pass) +- `cargo test --test cli_tests`: **PASS** (the then-current suite passed, + including the configuration-discovery tests) +- `cargo test --test bdd_tests`: **PASS** (the then-current BDD suite passed, + including the configuration-discovery scenarios) +- `make test`: **PASS** (the then-current unit, BDD, and integration suites + passed) -All tests pass. Configuration discovery is fully implemented, tested, and -documented. +Validation for the follow-up must be reported from the focused and complete +gates run against the current revision; this historical record does not +substitute for that evidence. ### Documentation updates @@ -271,9 +306,10 @@ documented. ### Lessons learned -1. **Existing implementation was correct**: The discovery mechanism worked - correctly from the start. The milestone's value was in validation, - documentation, and test coverage rather than implementation changes. +1. **The discovery seam was correct**: The core `ConfigDiscovery` mechanism + worked correctly from the start. Later hardening addressed canonical path + comparison, fallback layer de-duplication, and deferred bounded diagnostics + without replacing that seam. 2. **BDD test complexity**: Integrating new BDD scenarios with existing test infrastructure requires careful coordination with existing step definitions. Future BDD work should either extend existing step files or provide complete @@ -553,4 +589,6 @@ Expected evidence: ## Completion acknowledgement This plan has been completed as indicated by the "Status: COMPLETED" header. -All stages have been implemented, tested, and integrated into the codebase. +The original stages were implemented, tested, and integrated into the codebase; +the historical validation record above must not be read as current follow-up +gate evidence. diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 86f1a5a8e..2bbcb0ce3 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2954,10 +2954,13 @@ Diagnostic-mode resolution uses `(OrthoResult, DiscoveryOutcome)` without emitting diagnostics. The composition boundary calls `DiscoveryOutcome::emit_diagnostics()` after tracing is configured, replaying the retained diagnostics without repeating environment -or filesystem access. `collect_file_layers_with_trace_and_env_source(...)` -performs the underlying discovery scan and retains bounded project-scope trace -metadata. `DiscoveryOutcome::into_layers()` transfers the same discovered -layers to `merge_with_cached_file_layers(...)`, which consumes them for the +or filesystem access. `collect_file_layers_with_normalizer_and_trace(directory, +normalizer, env_source)` performs the underlying discovery scan with the path +normalizer and environment source, retaining bounded project-scope trace +metadata. The normalizer canonicalizes comparison keys so equivalent project +path spellings de-duplicate to one layer. `DiscoveryOutcome::into_layers()` +transfers the same discovered layers to `merge_with_cached_file_layers(...)`, +which consumes them for the full merge and prevents a second discovery pass. The standalone `merge_with_config_and_env(...)` path performs discovery, emits diagnostics and delegates to `merge_with_cached_file_layers(...)`. diff --git a/docs/users-guide.md b/docs/users-guide.md index 5e2eeca51..078d6e30b 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -999,6 +999,11 @@ defaults. When it finds a candidate that cannot be loaded, such as malformed TOML or a file whose `extends` parent is missing, Netsuke reports the load error. A broken discovered configuration is therefore not treated as absent. +On Windows, Netsuke normalizes alternate spellings of a configuration path, +including short and long path forms, before comparing discovered layers. A +project `.netsuke.toml` therefore contributes one layer even when two spellings +refer to the same physical file. + ### Diagnose configuration selection Pass `--verbose` to see how Netsuke selected its configuration. Structured diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index a84684274..e38f6cd5d 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -32,7 +32,8 @@ mod trace; #[path = "discovery_telemetry.rs"] mod telemetry; use diagnostics::{BoundedConfigPath, ConfigLoadFailureKind, ConfigLoadWarning}; -use layers::collect_file_layers_with_trace_and_env_source; +use layers::collect_file_layers_with_normalizer_and_trace; +use paths::{FsPathNormalizer, PathNormalizer}; /// Record the discovery series for an already-timed phase at the boundary. pub use telemetry::record_discovery_outcome; use trace::{DiscoveryDiagnostics, DiscoveryTrace, FileLayerTrace}; @@ -121,12 +122,16 @@ impl DiscoveryOutcome { /// Discover configuration layers once through the injected environment. pub(crate) fn discover_file_layers(cli: &Cli, env: &impl EnvProvider) -> DiscoveryOutcome { - collect_outcome(cli, env) + discover_file_layers_with_normalizer(cli, env, &FsPathNormalizer) } -/// Run one discovery pass and retain its outcome, including deferred errors. -fn collect_outcome(cli: &Cli, env: &impl EnvProvider) -> DiscoveryOutcome { - let (trace, load_warning, outcome) = collect_file_layers_with_env(cli, env); +/// Discover configuration layers through one path-normalization policy. +fn discover_file_layers_with_normalizer( + cli: &Cli, + env: &impl EnvProvider, + normalizer: &impl PathNormalizer, +) -> DiscoveryOutcome { + let (trace, load_warning, outcome) = collect_file_layers_with_env(cli, env, normalizer); let diagnostics = DiscoveryDiagnostics::new(trace, load_warning); let layers = match outcome { Ok(discovered_layers) => { @@ -149,10 +154,7 @@ fn collect_outcome(cli: &Cli, env: &impl EnvProvider) -> DiscoveryOutcome { DiscoveryOutcome { layers } } -/// Add a discovered file layer result to a merge composition. -/// -/// Discovery errors join the merge error collection, retaining the normal -/// merge path's accumulated-error behaviour. +/// Add discovered layers and errors to the normal merge accumulation. pub(crate) fn push_discovered_file_layers( composer: &mut MergeComposer, errors: &mut Vec>, @@ -166,12 +168,11 @@ pub(crate) fn push_discovered_file_layers( } /// Load layers through the shared explicit-config precedence boundary. -/// -/// Normal merging and early JSON resolution both use this helper so they -/// select the same file layers while retaining their own error handling. +/// Normal merging and early JSON resolution use it to retain their error policy. fn collect_file_layers_with_env( cli: &Cli, env: &impl EnvProvider, + normalizer: &impl PathNormalizer, ) -> ( DiscoveryTrace, Option, @@ -180,8 +181,9 @@ fn collect_file_layers_with_env( let resolution = resolve_config_selector(cli.config.clone(), env); let (file_layers, load_warning, outcome) = resolution.path.as_deref().map_or_else( || { - let (project_scope, outcome) = collect_file_layers_with_trace_and_env_source( + let (project_scope, outcome) = collect_file_layers_with_normalizer_and_trace( cli.directory.as_deref(), + normalizer, discovery_env_source(env), ); (FileLayerTrace::Automatic { project_scope }, None, outcome) @@ -347,6 +349,9 @@ mod event_assertions; #[path = "discovery_tracing_tests.rs"] mod tracing_tests; +#[cfg(test)] +#[path = "discovery_layer_replay_tests.rs"] +mod layer_replay_tests; #[cfg(test)] #[path = "discovery_layer_tests.rs"] mod layer_tests; diff --git a/src/cli/discovery_diagnostics.rs b/src/cli/discovery_diagnostics.rs index e8129885d..4692ab419 100644 --- a/src/cli/discovery_diagnostics.rs +++ b/src/cli/discovery_diagnostics.rs @@ -22,6 +22,35 @@ pub(super) enum ConfigLoadFailureKind { LoadError, } +/// Emit the bounded outcome of project-scope layer de-duplication. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct ProjectLayerDeduplication { + discovered: usize, + project: usize, + appended: usize, +} + +impl ProjectLayerDeduplication { + /// Retain the bounded counts produced by one project-layer fallback pass. + pub(super) const fn new(discovered: usize, project: usize, appended: usize) -> Self { + Self { + discovered, + project, + appended, + } + } + + /// Emit the retained project-layer de-duplication outcome. + pub(super) fn emit(&self) { + debug!( + discovered_layer_count = self.discovered, + project_layer_count = self.project, + appended_layer_count = self.appended, + "resolved project-scope layer deduplication" + ); + } +} + /// Bounded warning metadata retained when an explicit config load fails. #[derive(Clone, Debug)] pub(super) struct ConfigLoadWarning { diff --git a/src/cli/discovery_helper_proptests.rs b/src/cli/discovery_helper_proptests.rs index 98622d2d6..961be8e3f 100644 --- a/src/cli/discovery_helper_proptests.rs +++ b/src/cli/discovery_helper_proptests.rs @@ -8,17 +8,17 @@ //! `DefaultHasher`'s algorithm is explicitly not stable across Rust releases, //! so nothing here asserts a specific hash value — only width, charset, and //! determinism within the run. - use super::MergeLayer; use super::diagnostics::{path_hash, short_hash}; use super::json::json_from_value; +use super::layers::collect_file_layers_with_normalizer; use super::paths::{FailingPathNormalizer, FsPathNormalizer, normalized_path_key}; use anyhow::{Context, Result, ensure}; use proptest::prelude::*; use rstest::rstest; use serde_json::{Map, Value}; use std::borrow::Cow; -use std::path::Path; +use std::path::{Path, PathBuf}; use tempfile::tempdir; /// Generate arbitrary byte strings, including empty and non-UTF-8 sequences. @@ -31,6 +31,15 @@ fn path_string() -> impl Strategy { "[A-Za-z0-9._/-]{0,64}" } +fn project_alias(temp: &Path, project_name: &str, spelling: u8) -> PathBuf { + let project = temp.join(project_name); + match spelling { + 0 => project, + 1 => project.join("."), + _ => temp.join(project_name).join("..").join(project_name), + } +} + /// Assert `hash` is the bounded correlation identifier the log fields rely on. fn ensure_bounded_hash(hash: &str) -> Result<()> { ensure!( @@ -100,6 +109,36 @@ proptest! { .expect("an already-resolved path must normalize again"); prop_assert_eq!(once, twice); } + + /// Every generated spelling of one project directory produces one layer. + #[test] + fn project_config_aliases_have_one_canonical_layer( + project_name in "[A-Za-z0-9][A-Za-z0-9_-]{0,31}", + spelling in 0_u8..3, + ) { + let temp = tempdir().expect("create temp dir for project alias"); + let project = temp.path().join(&project_name); + test_support::fs::create_dir(&project).expect("create generated project directory"); + let config = project.join(".netsuke.toml"); + test_support::fs::write(&config, "default_targets = [\"alpha\"]\n") + .expect("write generated project config"); + + let layers = collect_file_layers_with_normalizer( + Some(&project_alias(temp.path(), &project_name, spelling)), + &FsPathNormalizer, + ) + .expect("discover generated project config"); + let discovered = layers + .iter() + .filter_map(|layer| layer.path().map(|path| path.as_str().to_owned())) + .collect::>(); + let canonical = normalized_path_key(&FsPathNormalizer, &config.to_string_lossy()) + .expect("canonicalize generated project config") + .to_string_lossy() + .into_owned(); + + prop_assert_eq!(discovered, vec![canonical]); + } } /// One generated file-layer value with an optional `json` preference. diff --git a/src/cli/discovery_layer_replay_tests.rs b/src/cli/discovery_layer_replay_tests.rs new file mode 100644 index 000000000..9d73aec5c --- /dev/null +++ b/src/cli/discovery_layer_replay_tests.rs @@ -0,0 +1,96 @@ +//! Deferred-diagnostic replay tests for configuration file-layer discovery. +//! +//! These tests share discovery fixtures with `layer_tests` and verify the +//! composition boundary replays its retained, bounded events without another +//! environment lookup. + +use super::event_assertions::{EventAssertion, find_event}; +use super::layer_tests::{CountingEnv, LayerScenario, replay_events, scenario_cli}; +use super::*; +use anyhow::{Context, Result, ensure}; +use tempfile::tempdir; + +/// Cached explicit selection emits its branch without repeating discovery. +#[test] +fn replay_logs_explicit_config_branch_without_environment_access() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli = scenario_cli(LayerScenario::ExplicitConfig, &temp)?; + let env = CountingEnv::default(); + let discovered = collect_diag_file_layers_with_env(&cli, &env); + let events = replay_events(&discovered)?; + find_event(&events, "resolved config path")?; + let branch_event = find_event(&events, "using explicit config path")?; + + ensure!( + !discovered.layers().is_empty(), + "explicit layer should be retained for the merge" + ); + ensure!( + env.get_calls() == 0, + "explicit selection should not access the environment, including on replay" + ); + EventAssertion::new( + branch_event, + cli.config.as_deref().context("explicit config")?, + ) + .ensure_bounded_path_fields()?; + + Ok(()) +} + +/// Cached automatic discovery emits no project-scope trace without a project layer. +#[test] +fn replay_logs_discovery_without_project_scope_trace_without_environment_access() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli = scenario_cli(LayerScenario::Discovery, &temp)?; + let env = CountingEnv::default(); + let discovered = collect_diag_file_layers_with_env(&cli, &env); + let discovery_get_calls = env.get_calls(); + ensure!( + discovery_get_calls > 0, + "discovery should read the injected environment" + ); + + let events = replay_events(&discovered)?; + ensure!( + env.get_calls() == discovery_get_calls, + "replay must not access the environment again" + ); + find_event(&events, "read config path variable")?; + find_event(&events, "resolved config path")?; + find_event(&events, "using config discovery")?; + ensure!( + !events + .iter() + .any(|event| event.contains("project-scope layers")), + "discovery without a project layer must not report a project-scope outcome: {events:?}" + ); + + Ok(()) +} + +/// Cached automatic discovery replays its included project-scope decision. +#[test] +fn replay_logs_included_project_scope_without_environment_access() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + test_support::fs::write(temp.path().join(".netsuke.toml"), "jobs = 7\n") + .context("write project config")?; + let cli = scenario_cli(LayerScenario::Discovery, &temp)?; + let env = CountingEnv::default(); + let discovered = collect_diag_file_layers_with_env(&cli, &env); + let discovery_get_calls = env.get_calls(); + ensure!( + discovery_get_calls > 0, + "discovery should read the injected environment" + ); + + let events = replay_events(&discovered)?; + ensure!( + env.get_calls() == discovery_get_calls, + "replay must not access the environment again" + ); + find_event(&events, "using config discovery")?; + find_event(&events, "discovery included project-scope layers")?; + + Ok(()) +} diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index 74b64534c..dc01ecce8 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -3,6 +3,7 @@ //! These cover which branch the shared file-layer boundary takes — explicit path //! versus automatic discovery — and the project-scope second pass. Selector //! precedence and event-schema snapshots live in the tracing test module. +use super::paths::{FailingPathNormalizer, FsPathNormalizer, PathNormalizer, normalized_path_key}; use super::*; use crate::cli::test_support::TestEnv; use anyhow::{Context, Result, ensure}; @@ -11,9 +12,8 @@ use pretty_assertions::assert_eq; use rstest::rstest; use tempfile::{TempDir, tempdir}; -use super::event_assertions::{EventAssertion, capture_events, find_event}; +use super::event_assertions::{capture_events, find_event}; use super::layers::collect_file_layers_with_normalizer; -use super::paths::{FailingPathNormalizer, PathNormalizer}; use std::cell::Cell; use std::ffi::OsString; use std::path::{Path, PathBuf}; @@ -97,86 +97,6 @@ pub(super) fn replay_events(discovered: &DiscoveryOutcome) -> Result Ok(events) } -/// Cached explicit selection emits its branch without repeating discovery. -#[test] -fn replay_logs_explicit_config_branch_without_environment_access() -> Result<()> { - let temp = tempdir().context("create temp dir")?; - let cli = scenario_cli(LayerScenario::ExplicitConfig, &temp)?; - let env = CountingEnv::default(); - let discovered = collect_diag_file_layers_with_env(&cli, &env); - let events = replay_events(&discovered)?; - find_event(&events, "resolved config path")?; - let branch_event = find_event(&events, "using explicit config path")?; - - ensure!( - !discovered.layers().is_empty(), - "explicit layer should be retained for the merge" - ); - ensure!( - env.get_calls() == 0, - "explicit selection should not access the environment, including on replay" - ); - EventAssertion::new( - branch_event, - cli.config.as_deref().context("explicit config")?, - ) - .ensure_bounded_path_fields()?; - - Ok(()) -} - -/// Cached automatic discovery replays its appended project-scope decision. -#[test] -fn replay_logs_discovery_and_appended_project_scope_without_environment_access() -> Result<()> { - let temp = tempdir().context("create temp dir")?; - let cli = scenario_cli(LayerScenario::Discovery, &temp)?; - let env = CountingEnv::default(); - let discovered = collect_diag_file_layers_with_env(&cli, &env); - let discovery_get_calls = env.get_calls(); - ensure!( - discovery_get_calls > 0, - "discovery should read the injected environment" - ); - - let events = replay_events(&discovered)?; - ensure!( - env.get_calls() == discovery_get_calls, - "replay must not access the environment again" - ); - find_event(&events, "read config path variable")?; - find_event(&events, "resolved config path")?; - find_event(&events, "using config discovery")?; - find_event(&events, "appending project-scope layers")?; - - Ok(()) -} - -/// Cached automatic discovery replays its included project-scope decision. -#[test] -fn replay_logs_included_project_scope_without_environment_access() -> Result<()> { - let temp = tempdir().context("create temp dir")?; - test_support::fs::write(temp.path().join(".netsuke.toml"), "jobs = 7\n") - .context("write project config")?; - let cli = scenario_cli(LayerScenario::Discovery, &temp)?; - let env = CountingEnv::default(); - let discovered = collect_diag_file_layers_with_env(&cli, &env); - let discovery_get_calls = env.get_calls(); - ensure!( - discovery_get_calls > 0, - "discovery should read the injected environment" - ); - - let events = replay_events(&discovered)?; - ensure!( - env.get_calls() == discovery_get_calls, - "replay must not access the environment again" - ); - find_event(&events, "using config discovery")?; - find_event(&events, "discovery included project-scope layers")?; - - Ok(()) -} - /// Automatic discovery must use the injected XDG directory, not the host. #[test] fn injected_automatic_discovery_uses_xdg_config_home() -> Result<()> { @@ -200,7 +120,12 @@ fn injected_automatic_discovery_uses_xdg_config_home() -> Result<()> { .filter_map(|layer| layer.path().map(|path| path.as_str().to_owned())) .collect::>(); - assert_eq!(paths, vec![config_path.to_string_lossy().into_owned()]); + let expected_path = normalized_path_key(&FsPathNormalizer, &config_path.to_string_lossy()) + .context("canonicalise injected XDG config path")? + .to_string_lossy() + .into_owned(); + + assert_eq!(paths, vec![expected_path]); Ok(()) } @@ -249,12 +174,9 @@ fn discovered_project_config_retains_load_outcome( Ok(()) } -/// A project-scope layer already found by discovery is not appended again. +/// A non-canonical `--directory` must not append an already-discovered layer. /// -/// `OrthoConfig` records canonicalised layer paths, so a non-canonical -/// `--directory` (here one containing a `.` component, as a relative or -/// symlinked path would be) must still match. Appending twice would duplicate -/// the entries of every `merge_strategy = "append"` field in the file. +/// Duplicating the layer repeats every `merge_strategy = "append"` value. #[test] fn existing_project_scope_layer_is_not_appended_twice() -> Result<()> { let temp = tempdir().context("create temp dir")?; @@ -292,6 +214,42 @@ fn existing_project_scope_layer_is_not_appended_twice() -> Result<()> { Ok(()) } +/// A symlink alias must not append a project-scope layer twice: Windows short +/// and long names canonicalize to the one file recorded by discovery. +#[cfg(unix)] +#[test] +fn project_scope_layer_is_not_appended_twice_via_symlink_alias() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let project_dir = temp.path().join("project"); + test_support::fs::create_dir(&project_dir).context("create project dir")?; + test_support::fs::write( + project_dir.join(".netsuke.toml"), + "default_targets = [\"alpha\"]\n", + ) + .context("write project config")?; + + // An alternate spelling of `project_dir` that resolves to the same file. + let alias = temp.path().join("project-alias"); + test_support::fs::symlink(&project_dir, &alias).context("create project alias")?; + + let layers = + collect_file_layers_with_normalizer(Some(alias.as_path()), &paths::FsPathNormalizer)?; + + let project_layers = layers + .iter() + .filter(|layer| { + layer + .path() + .is_some_and(|path| path.as_str().ends_with(".netsuke.toml")) + }) + .count(); + ensure!( + project_layers == 1, + "project-scope layer should appear exactly once, found {project_layers}: {layers:?}" + ); + Ok(()) +} + /// Scanning stored canonical layer paths does not normalize each inherited layer. #[test] fn project_layer_scan_normalizes_only_the_project_key() -> Result<()> { @@ -322,12 +280,8 @@ fn project_layer_scan_normalizes_only_the_project_key() -> Result<()> { Ok(()) } -/// Normalization failure must not fail configuration discovery. -/// -/// A missing project `.netsuke.toml` or an unreadable directory makes -/// canonicalization fail, which is ordinary rather than exceptional. The -/// discovery-side policy compares such a path literally and carries on; only an -/// unmatched project layer results, never an error. +/// Canonicalization failure falls back to literal comparison without failing +/// discovery. #[test] fn normalization_failure_does_not_fail_discovery() -> Result<()> { let temp = tempdir().context("create temp dir")?; @@ -339,11 +293,47 @@ fn normalization_failure_does_not_fail_discovery() -> Result<()> { ) .context("write project config")?; - let layers = - collect_file_layers_with_normalizer(Some(project_dir.as_path()), &FailingPathNormalizer) - .context("discovery must succeed despite normalization failure")?; + // The dot component differs from OrthoConfig's canonical layer path, so + // the failing normalizer must take the fallback de-duplication branch. + let alias = project_dir.join("."); + let cli = Cli { + directory: Some(alias), + ..Cli::default() + }; + let env = CountingEnv::default(); + let (discovered, collection_events) = capture_events(|| { + Ok::<_, anyhow::Error>(discover_file_layers_with_normalizer( + &cli, + &env, + &FailingPathNormalizer, + )) + })?; - ensure!(layers.len() == 1, "expected one project layer: {layers:?}"); + ensure!( + !collection_events + .iter() + .any(|event| event.contains("resolved project-scope layer deduplication")), + "discovery must defer the project-layer count event until replay: {collection_events:?}" + ); + let discovery_get_calls = env.get_calls(); + let events = replay_events(&discovered)?; + + ensure!( + env.get_calls() == discovery_get_calls, + "replay must not access the environment again" + ); + ensure!( + discovered.layers().len() == 1, + "expected one project layer: {:?}", + discovered.layers() + ); + let deduplication = find_event(&events, "resolved project-scope layer deduplication")?; + ensure!( + deduplication.contains("discovered_layer_count=1") + && deduplication.contains("project_layer_count=1") + && deduplication.contains("appended_layer_count=0"), + "fallback de-duplication outcome should record its counts: {deduplication}" + ); Ok(()) } diff --git a/src/cli/discovery_layers.rs b/src/cli/discovery_layers.rs index 4e6785fc8..d96f4e660 100644 --- a/src/cli/discovery_layers.rs +++ b/src/cli/discovery_layers.rs @@ -11,15 +11,18 @@ use ortho_config::{ load_config_file_as_chain, }; use std::borrow::Cow; +use std::collections::HashSet; use std::path::{Path, PathBuf}; #[cfg(test)] use std::sync::Arc; use super::super::parser::Cli; use super::CONFIG_ENV_VAR; -use super::diagnostics::{BoundedConfigPath, debug_optional_config_path_from_fields}; +use super::diagnostics::{ + BoundedConfigPath, ProjectLayerDeduplication, debug_optional_config_path_from_fields, +}; use super::json::json_from_value; -use super::paths::{FsPathNormalizer, PathNormalizer, normalized_path_key}; +use super::paths::{PathNormalizer, normalized_path_key}; /// Preserve discovered layers while extracting their final JSON preference. /// @@ -53,7 +56,15 @@ pub(super) enum ProjectScopeTrace { /// The primary discovery scan already yielded the project configuration. Included(BoundedConfigPath), /// The project configuration was loaded by the second pass. - Appended(BoundedConfigPath), + Appended { + path: BoundedConfigPath, + deduplication: Option, + }, + /// The second pass found only layers already returned by discovery. + Deduplicated { + path: BoundedConfigPath, + deduplication: ProjectLayerDeduplication, + }, } impl ProjectScopeTrace { @@ -66,9 +77,25 @@ impl ProjectScopeTrace { path, ); } - Self::Appended(path) => { + Self::Appended { + path, + deduplication, + } => { + if let Some(counts) = deduplication { + counts.emit(); + } debug_optional_config_path_from_fields("appending project-scope layers", path); } + Self::Deduplicated { + path, + deduplication, + } => { + deduplication.emit(); + debug_optional_config_path_from_fields( + "project-scope layers already discovered", + path, + ); + } } } } @@ -87,18 +114,6 @@ fn config_discovery(directory: Option<&PathBuf>, env_source: SharedEnvSource) -> builder.build() } -/// Run discovery with the composition root's environment source and retain its -/// project-scope outcome for later replay. -pub(super) fn collect_file_layers_with_trace_and_env_source( - directory: Option<&Path>, - env_source: SharedEnvSource, -) -> ( - Option, - OrthoResult>>, -) { - collect_file_layers_with_normalizer_and_trace(directory, &FsPathNormalizer, env_source) -} - /// Return the key used to compare the expected project file against a layer. /// /// This is the discovery-side fallback policy for [`normalized_path_key`]. A @@ -106,8 +121,8 @@ pub(super) fn collect_file_layers_with_trace_and_env_source( /// frequently does not exist, or a directory the process cannot read — is /// compared literally with `OrthoConfig`'s already-canonicalized layer path /// rather than failing discovery. An exact textual match still identifies the layer; -/// otherwise the project-scope pass appends it and retains its normal debug -/// event for the composition boundary. +/// otherwise the project-scope pass de-duplicates loaded layers and retains +/// its bounded outcome for the composition boundary. fn comparison_key(normalizer: &impl PathNormalizer, path: &str) -> PathBuf { normalized_path_key(normalizer, path).unwrap_or_else(|_| PathBuf::from(path)) } @@ -130,7 +145,7 @@ pub(super) fn collect_file_layers_with_normalizer( } /// Build the discovery chain and project-scope trace using `normalizer`. -fn collect_file_layers_with_normalizer_and_trace( +pub(super) fn collect_file_layers_with_normalizer_and_trace( directory: Option<&Path>, normalizer: &impl PathNormalizer, env_source: SharedEnvSource, @@ -159,7 +174,7 @@ fn collect_file_layers_with_normalizer_and_trace( let has_project_layer = project_key.as_deref().is_some_and(|key| { file_layers.value.iter().any(|layer| { layer.path().is_some_and(|path| { - key == path.as_std_path() + key.as_os_str() == path.as_std_path().as_os_str() || lossy_project_key .as_deref() .is_some_and(|lossy_key| lossy_key == path.as_str()) @@ -174,17 +189,71 @@ fn collect_file_layers_with_normalizer_and_trace( ); } - let trace = ProjectScopeTrace::Appended(project_trace_path); - let result = project_scope_layers(project_file.as_deref()).map(|project_layers| { - file_layers - .value + merge_project_scope_layers( + file_layers.value, + project_file.as_deref(), + project_trace_path, + ) +} + +/// Load project layers, filter aliases already yielded by discovery, and trace the outcome. +fn merge_project_scope_layers( + discovered_layers: Vec>, + project_file: Option<&Path>, + project_trace_path: BoundedConfigPath, +) -> ( + Option, + OrthoResult>>, +) { + let error_trace = ProjectScopeTrace::Appended { + path: project_trace_path.clone(), + deduplication: None, + }; + let result = project_scope_layers(project_file).map(|project_layers| { + let discovered_paths = discovered_layers + .iter() + .filter_map(|layer| layer.path().map(camino::Utf8Path::as_str)) + .collect::>(); + let discovered_layer_count = discovered_paths.len(); + let project_layer_count = project_layers.len(); + let project_layers_to_append = project_layers .into_iter() - .chain(project_layers) - .collect() + .filter(|layer| { + layer + .path() + .is_none_or(|path| !discovered_paths.contains(path.as_str())) + }) + .collect::>(); + let appended_layer_count = project_layers_to_append.len(); + let deduplication = ProjectLayerDeduplication::new( + discovered_layer_count, + project_layer_count, + appended_layer_count, + ); + let trace = if project_layer_count == 0 { + None + } else if appended_layer_count == 0 { + Some(ProjectScopeTrace::Deduplicated { + path: project_trace_path, + deduplication, + }) + } else { + Some(ProjectScopeTrace::Appended { + path: project_trace_path, + deduplication: Some(deduplication), + }) + }; + let layers = discovered_layers + .into_iter() + .chain(project_layers_to_append) + .collect(); + (trace, layers) }); - (Some(trace), result) + match result { + Ok((trace, layers)) => (trace, Ok(layers)), + Err(err) => (Some(error_trace), Err(err)), + } } - fn project_scope_file(directory: Option<&Path>) -> Option { let root = directory .map(PathBuf::from) diff --git a/src/cli/discovery_paths.rs b/src/cli/discovery_paths.rs index 2e8b86853..92f2817c0 100644 --- a/src/cli/discovery_paths.rs +++ b/src/cli/discovery_paths.rs @@ -18,7 +18,13 @@ pub(super) struct FsPathNormalizer; impl PathNormalizer for FsPathNormalizer { fn normalize(&self, path: &Path) -> io::Result { - std::fs::canonicalize(path) + // `ortho_config` canonicalizes layer paths with `dunce` on Windows so + // diagnostics and comparisons stay free of UNC prefixes and short-name + // forms; mirror that here so the project-scope dedup key and the + // injected explicit config path compare equal to the recorded layer + // path. On other platforms `dunce` is a thin wrapper over + // `std::fs::canonicalize`, so the behaviour is unchanged. + dunce::canonicalize(path) } } diff --git a/src/cli/discovery_unit_tests.rs b/src/cli/discovery_unit_tests.rs index 42017e343..58633e54e 100644 --- a/src/cli/discovery_unit_tests.rs +++ b/src/cli/discovery_unit_tests.rs @@ -2,7 +2,7 @@ use super::*; use crate::cli::test_support::TestEnv; -use anyhow::ensure; +use anyhow::{Context, ensure}; use cap_std::{ambient_authority, fs::Dir}; use rstest::rstest; use std::path::PathBuf; @@ -60,7 +60,15 @@ fn collect_diag_file_layers_uses_injected_explicit_config() -> anyhow::Result<() let env = TestEnv::default().with_var(CONFIG_ENV_VAR, config_path.as_os_str()); let discovered = collect_diag_file_layers_with_env(&Cli::default(), &env); - let expected_path = config_path.to_string_lossy().into_owned(); + // `load_config_file_as_chain` canonicalises the layer path through the + // same normalizer discovery uses, so compare the injected path in that + // canonical form. On Windows this folds short-name and UNC-prefixed + // spellings into the long-name form the layer records. + let expected_path = + paths::normalized_path_key(&paths::FsPathNormalizer, &config_path.to_string_lossy()) + .context("canonicalise injected config path")? + .to_string_lossy() + .into_owned(); ensure!( discovered.layers().iter().any(|layer| layer diff --git a/src/cli/parser.rs b/src/cli/parser.rs index f54efd96c..5f27501b6 100644 --- a/src/cli/parser.rs +++ b/src/cli/parser.rs @@ -82,6 +82,7 @@ pub(super) fn validation_message( #[derive(Debug, Parser, Serialize, Deserialize)] #[command( name = "netsuke", + bin_name = "netsuke", author, version, about, diff --git a/src/cli/parser_tests.rs b/src/cli/parser_tests.rs index 4cb1a2c4c..93b4bf346 100644 --- a/src/cli/parser_tests.rs +++ b/src/cli/parser_tests.rs @@ -13,6 +13,12 @@ use insta::assert_snapshot; use rstest::rstest; use test_support::fluent::normalize_fluent_isolates; +/// Pins the public CLI name independently of the platform executable suffix. +#[test] +fn cli_command_uses_documented_binary_name() { + assert_eq!(Cli::command().get_bin_name(), Some("netsuke")); +} + /// Verifies localized long-help includes `--config ` and its /// Fluent-resolved description, then matches the complete output snapshot. #[rstest] diff --git a/src/manifest/glob/tests/capability.rs b/src/manifest/glob/tests/capability.rs index 7d808365d..c8ecaa5aa 100644 --- a/src/manifest/glob/tests/capability.rs +++ b/src/manifest/glob/tests/capability.rs @@ -1,8 +1,11 @@ //! Tests for the capability handle the glob metadata checks run through. -use super::super::walk::{literal_dir_prefix, open_root_dir}; +#[cfg(unix)] +use super::super::walk::literal_dir_prefix; +use super::super::walk::open_root_dir; use super::super::{GlobPattern, glob_paths}; use anyhow::{Context, Result, anyhow, ensure}; use camino::{Utf8Path, Utf8PathBuf}; +#[cfg(unix)] use minijinja::ErrorKind; use rstest::{fixture, rstest}; use tempfile::{TempDir, tempdir}; diff --git a/src/manifest/glob/tests/diagnostics.rs b/src/manifest/glob/tests/diagnostics.rs index c95234f1b..1191dbc8f 100644 --- a/src/manifest/glob/tests/diagnostics.rs +++ b/src/manifest/glob/tests/diagnostics.rs @@ -4,7 +4,9 @@ //! subscriber scoped to the call. The recorder and subscriber are both //! thread-local, so no test-wide lock is needed. -use super::super::{MAX_UNREACHABLE_SYMLINK_SAMPLES, expand_glob, glob_paths, record_expansion}; +#[cfg(unix)] +use super::super::MAX_UNREACHABLE_SYMLINK_SAMPLES; +use super::super::{expand_glob, glob_paths, record_expansion}; use anyhow::{Context, Result, ensure}; use metrics::SharedString; use metrics_util::{ diff --git a/src/manifest/glob/tests/expansion.rs b/src/manifest/glob/tests/expansion.rs index bbe5235cd..34309ded4 100644 --- a/src/manifest/glob/tests/expansion.rs +++ b/src/manifest/glob/tests/expansion.rs @@ -1,7 +1,13 @@ //! Tests for the match set [`glob_paths`] returns. +#[cfg(unix)] +use super::super::GlobPattern; +use super::super::glob_paths; +#[cfg(unix)] use super::super::walk::{GlobRoot, process_glob_entry}; -use super::super::{GlobPattern, glob_paths}; -use anyhow::{Context, Result, anyhow, ensure}; +#[cfg(unix)] +use anyhow::{Context, anyhow}; +use anyhow::{Result, ensure}; +#[cfg(unix)] use cap_std::{ambient_authority, fs::Dir}; use minijinja::ErrorKind; use rstest::rstest; diff --git a/src/manifest/glob/validate.rs b/src/manifest/glob/validate.rs index db3b28fec..c66c2794e 100644 --- a/src/manifest/glob/validate.rs +++ b/src/manifest/glob/validate.rs @@ -39,7 +39,11 @@ impl ValidationState { } #[cfg(not(unix))] - fn process_escape(&mut self, _ch: char) -> bool { + #[expect( + clippy::unused_self, + reason = "signature must mirror the Unix arm, which reads self.escaped" + )] + const fn process_escape(&mut self, _ch: char) -> bool { false } diff --git a/src/manifest/glob/walk.rs b/src/manifest/glob/walk.rs index 23d4b41c2..1d7af78dd 100644 --- a/src/manifest/glob/walk.rs +++ b/src/manifest/glob/walk.rs @@ -349,7 +349,7 @@ fn prefix_is_unopenable(err: &io::Error) -> bool { { /// `ERROR_DIRECTORY`: the path is not a directory. const ERROR_DIRECTORY: i32 = 267; - return err.raw_os_error() == Some(ERROR_DIRECTORY); + err.raw_os_error() == Some(ERROR_DIRECTORY) } #[cfg(not(windows))] false diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 8eba54b0b..cca906052 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -167,7 +167,7 @@ fn handle_build(cli: &Cli, args: &BuildArgs, context: &ExecutionContext<'_>) -> }; let build_file = process::create_temp_ninja_file(&ninja)?; - let build_path = build_file.path(); + let build_path: &Path = build_file.as_ref(); let ctx = || { format!( @@ -230,7 +230,7 @@ fn handle_ninja_tool( let ninja = NinjaContent::new(ninja_file); let tmp = process::create_temp_ninja_file(&ninja)?; - let build_path = tmp.path(); + let build_path: &Path = tmp.as_ref(); let ctx = || { format!( diff --git a/src/runner/process/configure.rs b/src/runner/process/configure.rs index 19984620f..26eaca2d4 100644 --- a/src/runner/process/configure.rs +++ b/src/runner/process/configure.rs @@ -106,7 +106,9 @@ mod tests { OsString::from("-j"), OsString::from("4"), OsString::from("-f"), - build_file.canonicalize()?.into_os_string(), + canonicalize_utf8_path(build_file)? + .into_std_path_buf() + .into_os_string(), ]) } diff --git a/src/runner/process/dyndep_retention.rs b/src/runner/process/dyndep_retention.rs index b10f02279..54fcfb327 100644 --- a/src/runner/process/dyndep_retention.rs +++ b/src/runner/process/dyndep_retention.rs @@ -16,6 +16,7 @@ use fs4::FileExt; use std::{ collections::{BTreeMap, HashSet}, io::ErrorKind, + path::Path, }; /// Maximum number of obsolete sidecars retained after one publication. @@ -144,7 +145,7 @@ fn prune_dyndep_sidecars_inner( } let current_paths = current .iter() - .map(|sidecar| sidecar.relative_path().as_str()) + .map(|sidecar| sidecar.relative_path().as_std_path()) .collect::>(); let mut summary = RetentionSummary::default(); retain_obsolete_sidecars(dir, ¤t_paths, policy, &mut summary)?; @@ -154,7 +155,7 @@ fn prune_dyndep_sidecars_inner( /// Select obsolete sidecars during one directory traversal with bounded memory. fn retain_obsolete_sidecars( dir: &Dir, - current_paths: &HashSet<&str>, + current_paths: &HashSet<&Path>, policy: RetentionPolicy, summary: &mut RetentionSummary, ) -> Result<()> { @@ -174,7 +175,7 @@ fn retain_obsolete_sidecars( /// Mutable state scoped to one leased directory traversal. struct RetentionPass<'current, 'summary> { - current_paths: &'current HashSet<&'current str>, + current_paths: &'current HashSet<&'current Path>, retained: RetentionSelection, summary: &'summary mut RetentionSummary, } @@ -190,7 +191,9 @@ fn retain_directory_entry( .file_name() .with_context(|| retention_error(Utf8Path::new(DYNDEP_DIR)))?; let path = Utf8Path::new(DYNDEP_DIR).join(name); - if path.as_str() == DYNDEP_LOCK || pass.current_paths.contains(path.as_str()) { + if Path::new(DYNDEP_LOCK) == path.as_std_path() + || pass.current_paths.contains(path.as_std_path()) + { return Ok(()); } if has_extension(&path, "tmp") { @@ -262,8 +265,8 @@ fn retain_or_remove_sidecar( Ok(()) } -fn is_obsolete_sidecar(path: &Utf8Path, current_paths: &HashSet<&str>) -> bool { - has_extension(path, "dd") && !current_paths.contains(path.as_str()) +fn is_obsolete_sidecar(path: &Utf8Path, current_paths: &HashSet<&Path>) -> bool { + has_extension(path, "dd") && !current_paths.contains(path.as_std_path()) } fn has_extension(path: &Utf8Path, extension: &str) -> bool { diff --git a/src/runner/process/dyndep_retention_tests.rs b/src/runner/process/dyndep_retention_tests.rs index 3ceec790d..d6b8b8f28 100644 --- a/src/runner/process/dyndep_retention_tests.rs +++ b/src/runner/process/dyndep_retention_tests.rs @@ -11,6 +11,7 @@ use anyhow::{Context, Result, ensure}; use camino::Utf8PathBuf; use cap_std::fs_utf8::Dir; use rstest::{fixture, rstest}; +use std::path::Path; #[fixture] fn dyndep_workspace() -> Result<(tempfile::TempDir, Dir)> { @@ -328,8 +329,10 @@ fn retention_cleanup_failure_has_localized_context( let Err(error) = result else { anyhow::bail!("retention must report an unremovable candidate"); }; + let native_failing_path = Path::new(DYNDEP_DIR).join("unremovable.dd"); + let native_failing_path_display = native_failing_path.to_string_lossy().into_owned(); let expected = localization::message(keys::RUNNER_IO_DYNDEP_RETENTION) - .with_arg("path", failing_path.as_str()) + .with_arg("path", native_failing_path_display) .to_string(); ensure!( format!("{error:#}").contains(&expected), @@ -337,3 +340,43 @@ fn retention_cleanup_failure_has_localized_context( ); Ok(()) } + +#[cfg(windows)] +#[test] +fn windows_retention_removes_stale_sidecars_by_native_path_identity() -> Result<()> { + // Regression for the Windows path-identity bug fixed by comparing + // sidecar paths as `Path` values rather than `str` spellings: pruning + // must preserve the bundle that produced the lease and remove a stale + // sidecar even when the stale path is addressed through the native + // Windows separator spelling. The assertion resolves both paths through + // the capability directory, so NTFS treats them as the files on disk. + let temp = tempfile::tempdir()?; + let dir = temporary_dir(&temp)?; + let current = sidecar(".netsuke/dyndep/current.dd", "current"); + let lease = materialize_dyndep_files(&dir, std::slice::from_ref(¤t))?; + + // A stale sidecar is materialised by the same writer, so spell its path + // the way the NTFS directory entry reports it: backslash separators. + let stale_native = ".netsuke\\dyndep\\stale-candidate.dd"; + dir.write(stale_native, "stale")?; + + prune_dyndep_sidecars( + &dir, + &lease, + std::slice::from_ref(¤t), + RetentionPolicy::new(0, 0), + )?; + + ensure!( + dir.open(current.relative_path()).is_ok(), + "retention must preserve the current bundle's sidecar under native path identity" + ); + let stale_open_error = dir + .open(stale_native) + .expect_err("retention must remove a stale sidecar reported with native separators"); + ensure!( + stale_open_error.kind() == std::io::ErrorKind::NotFound, + "stale sidecar must be absent rather than inaccessible: {stale_open_error}" + ); + Ok(()) +} diff --git a/src/runner/process/exit_status_tests.rs b/src/runner/process/exit_status_tests.rs index 6dcce99d9..eb53efca4 100644 --- a/src/runner/process/exit_status_tests.rs +++ b/src/runner/process/exit_status_tests.rs @@ -18,10 +18,10 @@ fn exit_status(code: i32) -> ExitStatus { } #[cfg(windows)] -fn exit_status(code: i32) -> ExitStatus { +fn exit_status(code: u32) -> ExitStatus { use std::os::windows::process::ExitStatusExt; - ExitStatus::from_raw(code as u32) + ExitStatus::from_raw(code) } fn command_log_context() -> CommandLogContext { diff --git a/src/runner/process/file_io.rs b/src/runner/process/file_io.rs index 98b3c0687..19e1cd68f 100644 --- a/src/runner/process/file_io.rs +++ b/src/runner/process/file_io.rs @@ -9,7 +9,7 @@ use cap_std::{ambient_authority, fs as cap_fs}; use std::io; use std::io::Write; use std::path::Path; -use tempfile::{Builder, NamedTempFile}; +use tempfile::{Builder, NamedTempFile, TempPath}; use tracing::info; /// Return `true` when `path` is the CLI sentinel indicating "write to stdout". @@ -18,7 +18,12 @@ pub fn is_stdout_path(path: &Path) -> bool { path.as_os_str() == "-" } -pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult { +/// Materialize `content` as a temporary Ninja file with no open writer handle. +/// +/// Returning [`TempPath`] retains automatic cleanup while releasing the writer +/// before Ninja reopens the file by path. Windows otherwise rejects Ninja's +/// read while the original `NamedTempFile` handle remains open. +pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult { let mut tmp = Builder::new() .prefix("netsuke.") .suffix(".ninja") @@ -29,8 +34,9 @@ pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult Result<()> { + fn create_temp_ninja_file_releases_writer_before_external_read() -> Result<()> { let content = NinjaContent::new(String::from("rule cc")); let file = create_temp_ninja_file(&content)?; + let path: &Path = file.as_ref(); - // Ninja reads the file by path, so reopening must succeed. - drop(file.reopen().context("reopen temp file")?); - let written = test_fs::read_to_string(file.path()).context("read temp file")?; + // Ninja reads the file by path, so no writer handle may remain open. + let parent = path.parent().context("find temporary file parent")?; + let name = path.file_name().context("find temporary file name")?; + let directory = cap_fs::Dir::open_ambient_dir(parent, ambient_authority()) + .context("open temporary file parent")?; + drop( + directory + .open(name) + .context("open temporary Ninja file through an external handle")?, + ); + let written = test_fs::read_to_string(path).context("read temp file")?; ensure!( written == content.as_str(), "reopened file contents '{written}' did not match '{expected}'", expected = content.as_str() ); - let observed_len = test_fs::file_len(file.path()).context("query temp file metadata")?; + let observed_len = test_fs::file_len(path).context("query temp file metadata")?; ensure!( observed_len == content.as_str().len() as u64, "expected size {} but observed {}", content.as_str().len(), observed_len ); - let temp_display = file.path().display().to_string(); - let has_ninja_ext = file - .path() + let temp_display = path.display().to_string(); + let has_ninja_ext = path .extension() .and_then(|ext| ext.to_str()) .is_some_and(|ext| ext.eq_ignore_ascii_case("ninja")); diff --git a/src/stdlib/command/execution.rs b/src/stdlib/command/execution.rs index 0cfbaba44..5e2890916 100644 --- a/src/stdlib/command/execution.rs +++ b/src/stdlib/command/execution.rs @@ -1,5 +1,7 @@ //! Shell execution helpers shared by `shell` and `grep` filters. +#[cfg(windows)] +use std::os::windows::process::CommandExt; use std::{ io::{self, Write}, process::{Child, Command, ExitStatus, Stdio}, @@ -103,7 +105,7 @@ pub(super) fn run_command( run_configured_command( SHELL, |cmd| { - cmd.args(SHELL_ARGS).arg(command); + configure_shell_command(cmd, command); }, ChildInvocation { input, @@ -113,6 +115,23 @@ pub(super) fn run_command( ) } +/// Configure the host shell to execute one already-composed command string. +/// +/// Windows `cmd.exe` parses its command argument itself, rather than using +/// the usual C-runtime argument rules. Passing a quoted command through +/// [`Command::arg`] would escape its quotes into literal backslashes, so pass +/// the shell text verbatim inside `cmd /C`'s required outer quote pair. +fn configure_shell_command(cmd: &mut Command, command: &str) { + #[cfg(windows)] + { + cmd.args(SHELL_ARGS).raw_arg(format!("\"{command}\"")); + } + #[cfg(not(windows))] + { + cmd.args(SHELL_ARGS).arg(command); + } +} + #[cfg(windows)] pub(super) fn run_program( program: &str, diff --git a/src/stdlib/command/quote.rs b/src/stdlib/command/quote.rs index 923770eb9..229f680f5 100644 --- a/src/stdlib/command/quote.rs +++ b/src/stdlib/command/quote.rs @@ -26,6 +26,10 @@ impl fmt::Display for QuoteError { } } +/// `QuoteError` crosses into `anyhow::Result` in the Windows quoting tests, +/// which requires the `std::error::Error` trait. +impl std::error::Error for QuoteError {} + #[cfg(windows)] pub(super) fn quote(arg: &str) -> Result { if arg.chars().any(|ch| matches!(ch, '\n' | '\r')) { @@ -117,7 +121,7 @@ mod tests { ("foo\"bar\"baz", "\"foo^\"bar^\"baz\""), ("!DELAYED!", "\"^!DELAYED^!\""), ("\"!VAR!\"", "\"^\"^!VAR^!^\"\""), - (r#"C:\\path\\\"ending"#, r#""C:\\path\^"ending""#), + (r#"C:\\path\\\"ending"#, r#""C:\\path\\\^"ending""#), ]; for (input, expected) in success_cases { diff --git a/src/stdlib/config/mod.rs b/src/stdlib/config/mod.rs index 08525f346..646f79a8f 100644 --- a/src/stdlib/config/mod.rs +++ b/src/stdlib/config/mod.rs @@ -12,7 +12,7 @@ pub use super::config_types::{ use super::{command, network::NetworkPolicy, which::WORKSPACE_SKIP_DIRS}; use crate::localization::{self, keys}; use anyhow::{anyhow, bail, ensure}; -use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; +use camino::{Utf8Path, Utf8PathBuf}; use cap_std::fs_utf8::Dir; use std::{ffi::OsString, num::NonZeroUsize, sync::Arc}; @@ -284,17 +284,36 @@ impl StdlibConfig { ); } - for component in relative.components() { - if matches!( + #[cfg(windows)] + let has_parent_directory = relative + .as_str() + .split(['/', '\\']) + .any(|component| component == ".."); + #[cfg(not(windows))] + let has_parent_directory = relative + .as_std_path() + .components() + .any(|component| matches!(component, std::path::Component::ParentDir)); + + let has_rooted_component = relative.as_std_path().components().any(|component| { + matches!( component, - Utf8Component::ParentDir | Utf8Component::Prefix(_) - ) { - bail!( - "{}", - localization::message(keys::STDLIB_FETCH_CACHE_ESCAPES) - .with_arg("path", relative.as_str()) - ); - } + std::path::Component::Prefix(_) | std::path::Component::RootDir + ) + }); + if has_rooted_component { + bail!( + "{}", + localization::message(keys::STDLIB_FETCH_CACHE_NOT_RELATIVE) + .with_arg("path", relative.as_str()) + ); + } + if has_parent_directory { + bail!( + "{}", + localization::message(keys::STDLIB_FETCH_CACHE_ESCAPES) + .with_arg("path", relative.as_str()) + ); } Ok(()) diff --git a/src/stdlib/config_tests.rs b/src/stdlib/config_tests.rs index 5a9303f3b..36b48be25 100644 --- a/src/stdlib/config_tests.rs +++ b/src/stdlib/config_tests.rs @@ -36,6 +36,22 @@ fn validate_cache_relative_rejects_invalid_inputs( assert_eq!(err.to_string(), expected); } +/// Windows accepts either separator, so both parent-directory spellings must +/// be rejected before a cache path can escape the workspace capability. +#[cfg(windows)] +#[rstest] +#[case("../escape")] +#[case(r"..\escape")] +fn validate_cache_relative_rejects_windows_parent_separator(#[case] spelling: &str) { + let path = Utf8Path::new(spelling); + let err = StdlibConfig::validate_cache_relative(path) + .expect_err("Windows parent-directory spelling should fail"); + let expected = localization::message(keys::STDLIB_FETCH_CACHE_ESCAPES) + .with_arg("path", path.as_str()) + .to_string(); + assert_eq!(err.to_string(), expected); +} + #[rstest] fn validate_cache_relative_accepts_workspace_relative_paths() { StdlibConfig::validate_cache_relative(Utf8Path::new("nested/cache")) diff --git a/src/stdlib/register.rs b/src/stdlib/register.rs index c95e89af2..c38a90ab5 100644 --- a/src/stdlib/register.rs +++ b/src/stdlib/register.rs @@ -254,7 +254,7 @@ fn is_fifo(ft: fs::FileType) -> bool { } #[cfg(not(unix))] -fn is_fifo(_ft: fs::FileType) -> bool { +const fn is_fifo(_ft: fs::FileType) -> bool { false } @@ -264,7 +264,7 @@ fn is_block_device(ft: fs::FileType) -> bool { } #[cfg(not(unix))] -fn is_block_device(_ft: fs::FileType) -> bool { +const fn is_block_device(_ft: fs::FileType) -> bool { false } @@ -274,7 +274,7 @@ fn is_char_device(ft: fs::FileType) -> bool { } #[cfg(not(unix))] -fn is_char_device(_ft: fs::FileType) -> bool { +const fn is_char_device(_ft: fs::FileType) -> bool { false } @@ -284,6 +284,6 @@ fn is_device(ft: fs::FileType) -> bool { } #[cfg(not(unix))] -fn is_device(_ft: fs::FileType) -> bool { +const fn is_device(_ft: fs::FileType) -> bool { false } diff --git a/src/stdlib/which/env.rs b/src/stdlib/which/env.rs index 6018eac66..36b034c9b 100644 --- a/src/stdlib/which/env.rs +++ b/src/stdlib/which/env.rs @@ -67,6 +67,13 @@ pub(super) struct EnvSnapshot { } impl EnvSnapshot { + /// Capture a snapshot without a `PATHEXT` override. + /// + /// This is the production capture entry on platforms without `PATHEXT` + /// semantics, where `capture_with_pathext` delegates to it. On Windows the + /// production entry is `capture_with_pathext` itself, so here the function + /// survives only for tests, which is why the gate admits `test`. + #[cfg(any(not(windows), test))] pub(super) fn capture( cwd_override: Option<&Utf8Path>, path_override: Option<&OsStr>, @@ -74,6 +81,12 @@ impl EnvSnapshot { Self::capture_with_env(cwd_override, path_override, &DefaultEnv) } + /// Capture with an injected environment provider. + /// + /// See [`Self::capture`] for why this is gated to non-Windows production + /// plus tests: on Windows the production path threads a `PATHEXT` override + /// and reaches `capture_impl` directly, so this chain is test-only there. + #[cfg(any(not(windows), test))] pub(super) fn capture_with_env( cwd_override: Option<&Utf8Path>, path_override: Option<&OsStr>, @@ -88,7 +101,11 @@ impl EnvSnapshot { /// concept of, so the two `capture_impl` arities diverge. Isolating the /// divergence in a pair of wrappers keeps `capture_with_env` free of a /// `cfg`-gated bare `return`, which reads as dead code on either target. - #[cfg(windows)] + /// + /// Reachable only from `capture_with_env`, which is itself test-only on + /// Windows (production enters through `capture_with_pathext`), so this + /// arm is compiled only under `test`. + #[cfg(all(windows, test))] fn capture_for_platform( cwd_override: Option<&Utf8Path>, path_override: Option<&OsStr>, diff --git a/src/stdlib/which/lookup/tests.rs b/src/stdlib/which/lookup/tests.rs index 49c01ea77..1bb2737a9 100644 --- a/src/stdlib/which/lookup/tests.rs +++ b/src/stdlib/which/lookup/tests.rs @@ -230,7 +230,7 @@ fn pathext_without_leading_dots_is_normalised_and_deduplicated( Some(std::ffi::OsStr::new("COM;EXE;EXE; .BAT ;bat")), )?; let mut pathexts = snapshot.pathext().to_vec(); - pathexts.sort_unstable_by(|a, b| a.to_lowercase().cmp(&b.to_lowercase())); + pathexts.sort_unstable_by_key(|ext| ext.to_lowercase()); let contains_ci = |needle: &str| pathexts.iter().any(|ext| ext.eq_ignore_ascii_case(needle)); @@ -286,7 +286,6 @@ fn direct_path_not_executable_raises_direct_not_found( #[cfg(windows)] #[rstest] fn resolve_direct_appends_pathext(workspace: Result) -> Result<()> { - use crate::stdlib::which::workspace_switch::WorkspaceSwitch; use test_support::exec::make_executable; let env = workspace?; @@ -297,16 +296,14 @@ fn resolve_direct_appends_pathext(workspace: Result) -> Result<() test_fs::create_dir_all(tools_dir.as_std_path()).context("mkdir tools")?; let exe = base.with_extension("bat"); test_fs::write(exe.as_std_path(), b"@echo off\r\n").context("write stub")?; - make_executable(&exe)?; - - let snapshot = EnvSnapshot { - cwd: env.root.clone(), - raw_path: None, - raw_pathext: Some(".bat".into()), - entries: vec![], - pathext: vec![".bat".into()], - workspace_switch: WorkspaceSwitch::Absent, - }; + make_executable(exe.as_std_path())?; + + let snapshot = EnvSnapshot::capture_with_pathext( + Some(env.root()), + None, + Some(std::ffi::OsStr::new(".bat")), + ) + .context("capture env for direct PATHEXT resolution")?; let matches = resolve_direct(".\\tools\\gradlew", &snapshot, &WhichOptions::default())?; diff --git a/src/stdlib/which/lookup/workspace/windows.rs b/src/stdlib/which/lookup/workspace/windows.rs index 9e777e0b2..e26a0809c 100644 --- a/src/stdlib/which/lookup/workspace/windows.rs +++ b/src/stdlib/which/lookup/workspace/windows.rs @@ -23,7 +23,7 @@ struct CollectionState { } impl CollectionState { - fn new(collect_all: bool) -> Self { + const fn new(collect_all: bool) -> Self { Self { matches: Vec::new(), collect_all, @@ -119,11 +119,11 @@ impl WorkspaceMatchContext { if !command_has_ext { let candidates = env::candidate_paths(Utf8Path::new(""), &command_lower, env.pathext()); - for candidate in candidates { - if let Some(name) = Utf8Path::new(candidate.as_str()).file_name() { - basenames.insert(name.to_ascii_lowercase()); - } - } + basenames.extend(candidates.into_iter().filter_map(|candidate| { + Utf8Path::new(candidate.as_str()) + .file_name() + .map(str::to_ascii_lowercase) + })); } Self { diff --git a/test_support/src/canonicalize.rs b/test_support/src/canonicalize.rs new file mode 100644 index 000000000..d6a37fd7b --- /dev/null +++ b/test_support/src/canonicalize.rs @@ -0,0 +1,163 @@ +//! Path canonicalization for fixtures staged in ambient temporary +//! directories. +//! +//! Split from `fs.rs` to keep that module within the Whitaker +//! `module_max_lines` cap; included from there via `#[path]` so the helper +//! stays a child module of `fs`. + +use camino::{Utf8Path, Utf8PathBuf}; + +/// Resolve `path` to the filesystem's canonical spelling. +/// +/// The helper returns a [`camino::Utf8PathBuf`] so fixture code never has to +/// convert an ambient `std::path::PathBuf` back into the `camino` world. The +/// underlying canonicalization still happens through the ambient boundary that +/// `fs` exists to provide: `cap_std`'s `Dir::canonicalize` resolves only +/// within a directory handle and returns a relative path, so it cannot +/// reproduce an absolute canonical path for a fixture staged in an ambient +/// temporary directory. +/// +/// # Errors +/// +/// Propagates the underlying `std::fs::canonicalize` failure, or +/// [`std::io::ErrorKind::InvalidData`] when the canonical path is not valid +/// UTF-8. +/// +/// # Examples +/// +/// ``` +/// use camino::Utf8Path; +/// +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let path = Utf8Path::from_path(dir.path()).expect("tempdir path is UTF-8"); +/// let canonical = test_support::fs::canonicalize(path).expect("canonicalize fixture"); +/// assert!(canonical.is_absolute()); +/// ``` +pub fn canonicalize(path: &Utf8Path) -> std::io::Result { + let canonical = std::fs::canonicalize(path)?; + Utf8PathBuf::from_path_buf(canonical).map_err(|non_utf8_path| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "canonical fixture path is not valid UTF-8: {}", + non_utf8_path.display() + ), + ) + }) +} + +#[cfg(test)] +mod tests { + //! Regression coverage for fixture-path canonicalization. + + use super::canonicalize; + use anyhow::{Context, Result, anyhow, ensure}; + use camino::{Utf8Path, Utf8PathBuf}; + use rstest::{fixture, rstest}; + use std::path::Path; + use tempfile::{TempDir, tempdir}; + + #[fixture] + fn temporary_fixture_directory() -> Result { + tempdir().context("create temporary fixture directory") + } + + fn utf8_path(path: &Path) -> Result<&Utf8Path> { + Utf8Path::from_path(path).context("fixture path must be valid UTF-8") + } + + fn filesystem_canonical_path(path: &Path) -> Result { + let canonical = std::fs::canonicalize(path).context("canonicalize fixture path")?; + Utf8PathBuf::from_path_buf(canonical).map_err(|resolved_path| { + anyhow!("fixture canonical path must be valid UTF-8: {resolved_path:?}") + }) + } + + #[rstest] + fn canonicalize_resolves_dot_component_in_fixture_path( + temporary_fixture_directory: Result, + ) -> Result<()> { + let temporary = temporary_fixture_directory?; + let fixture_directory = temporary.path().join("fixture"); + super::super::create_dir(&fixture_directory).context("create fixture directory")?; + let fixture = fixture_directory.join("config.toml"); + super::super::write(&fixture, "jobs = 1\n").context("write fixture")?; + + let dotted_fixture = fixture_directory.join(".").join("config.toml"); + let canonical = + canonicalize(utf8_path(&dotted_fixture)?).context("canonicalize fixture")?; + let expected = filesystem_canonical_path(&fixture)?; + ensure!( + canonical == expected, + "canonical fixture path {canonical} did not match filesystem spelling {expected}" + ); + + Ok(()) + } + + #[rstest] + fn canonicalize_reports_a_missing_utf8_fixture_path( + temporary_fixture_directory: Result, + ) -> Result<()> { + use std::io::ErrorKind; + + let temporary = temporary_fixture_directory?; + let missing_fixture = temporary.path().join("missing-config.toml"); + + let error = canonicalize(utf8_path(&missing_fixture)?) + .expect_err("a missing fixture path must not canonicalize"); + ensure!( + error.kind() == ErrorKind::NotFound, + "expected NotFound for a missing fixture path, got {error}" + ); + Ok(()) + } + + #[cfg(unix)] + #[rstest] + fn canonicalize_resolves_unix_symlink_alias_to_target( + temporary_fixture_directory: Result, + ) -> Result<()> { + let temporary = temporary_fixture_directory?; + let target = temporary.path().join("target.toml"); + super::super::write(&target, "jobs = 1\n").context("write target fixture")?; + let alias = temporary.path().join("alias.toml"); + super::super::symlink(&target, &alias).context("create fixture alias")?; + + let canonical = canonicalize(utf8_path(&alias)?).context("canonicalize alias")?; + let expected = filesystem_canonical_path(&target)?; + ensure!( + canonical == expected, + "canonical alias path {canonical} did not resolve to target {expected}" + ); + + Ok(()) + } + + #[cfg(unix)] + #[rstest] + fn canonicalize_rejects_non_utf8_resolved_unix_path( + temporary_fixture_directory: Result, + ) -> Result<()> { + use std::ffi::OsString; + use std::io::ErrorKind; + use std::os::unix::ffi::OsStringExt; + + let temporary = temporary_fixture_directory?; + let non_utf8_name = OsString::from_vec(b"fixture-\xFF.toml".to_vec()); + let non_utf8_fixture = temporary.path().join(non_utf8_name); + super::super::write(&non_utf8_fixture, "jobs = 1\n").context("write non-UTF-8 fixture")?; + let utf8_alias = temporary.path().join("utf8-alias.toml"); + super::super::symlink(&non_utf8_fixture, &utf8_alias) + .context("create UTF-8 alias to non-UTF-8 fixture")?; + + let error = canonicalize(utf8_path(&utf8_alias)?) + .expect_err("a non-UTF-8 resolved path must be rejected"); + ensure!( + error.kind() == ErrorKind::InvalidData, + "expected InvalidData for non-UTF-8 resolved path, got {error}" + ); + + Ok(()) + } +} diff --git a/test_support/src/check_ninja.rs b/test_support/src/check_ninja.rs index 571a201f1..4597806d6 100644 --- a/test_support/src/check_ninja.rs +++ b/test_support/src/check_ninja.rs @@ -72,11 +72,12 @@ impl ShellFlag { /// Shared plumbing for the fake-Ninja factories below: creates the temp /// directory, writes `script` via [`write_exec_with_content`], and returns /// both so callers keep the directory alive for the script's lifetime. +#[cfg(unix)] fn write_fake_ninja_script(script: &str, context: &str) -> Result<(TempDir, PathBuf)> { let dir = TempDir::new().with_context(|| format!("{context}: create temp dir"))?; write_fake_ninja_script_in_dir(script, context, dir) } - +#[cfg(unix)] fn write_fake_ninja_script_in_dir( script: &str, context: &str, @@ -131,7 +132,9 @@ pub(crate) fn fake_ninja_check_build_file_in( /// /// Returns an error if the temporary directory or fake executable cannot be created. pub fn fake_ninja_check_build_file() -> Result<(TempDir, PathBuf)> { - write_fake_ninja_script( + #[cfg(unix)] + let (name, content) = ( + "ninja", concat!( "#!/bin/sh\n", "if [ \"$1\" = \"-f\" ] && [ ! -f \"$2\" ]; then\n", @@ -140,8 +143,23 @@ pub fn fake_ninja_check_build_file() -> Result<(TempDir, PathBuf)> { "fi\n", "exit 0\n" ), - "fake_ninja_check_build_file", - ) + ); + #[cfg(windows)] + let (name, content) = ( + "ninja.cmd", + concat!( + "@echo off\r\n", + "if /I \"%~1\"==\"-f\" if not exist \"%~2\" (\r\n", + " echo missing build file: %~2 1>&2\r\n", + " exit /B 1\r\n", + ")\r\n", + "exit /B 0\r\n", + ), + ); + let dir = TempDir::new().context("fake_ninja_check_build_file: create temp dir")?; + let path = write_exec_with_content(dir.path(), name, content) + .context("fake_ninja_check_build_file: write executable")?; + Ok((dir, path)) } /// Create a fake Ninja that validates `-t ` was invoked with the expected tool name. @@ -316,12 +334,20 @@ pub fn fake_ninja_expect_tool_with_jobs( } /// Stub for non-Unix platforms that returns an error. +/// +/// # Errors +/// +/// Always returns an error: this factory is only supported on Unix platforms. #[cfg(not(unix))] pub fn fake_ninja_expect_tool(_expected_tool: ToolName) -> Result<(TempDir, PathBuf)> { anyhow::bail!("fake_ninja_expect_tool is only supported on Unix platforms") } /// Stub for non-Unix platforms that returns an error. +/// +/// # Errors +/// +/// Always returns an error: this factory is only supported on Unix platforms. #[cfg(not(unix))] pub fn fake_ninja_expect_tool_with_jobs( _expected_tool: ToolName, @@ -331,7 +357,7 @@ pub fn fake_ninja_expect_tool_with_jobs( anyhow::bail!("fake_ninja_expect_tool_with_jobs is only supported on Unix platforms") } -#[cfg(test)] +#[cfg(all(test, unix))] mod tests { //! Unit coverage for the fake-Ninja factories in this module: verifies the //! generated shell scripts validate `-t`, `-f`, `-j`, and `-C` invocations diff --git a/test_support/src/command_helper.rs b/test_support/src/command_helper.rs index 906da45b5..43fe00adf 100644 --- a/test_support/src/command_helper.rs +++ b/test_support/src/command_helper.rs @@ -199,7 +199,9 @@ fn rust_compiler(env: &impl Env) -> OsString { mod tests { //! Unit tests for compiler selection and helper compilation. - use super::{RustHelperSource, compile_rust_helper_with_env, rust_compiler}; + use super::rust_compiler; + #[cfg(unix)] + use super::{RustHelperSource, compile_rust_helper_with_env}; #[cfg(unix)] use crate::exec::write_exec_with_content; #[cfg(unix)] diff --git a/test_support/src/exec.rs b/test_support/src/exec.rs index d10e2a69e..cc1393ef3 100644 --- a/test_support/src/exec.rs +++ b/test_support/src/exec.rs @@ -87,7 +87,7 @@ pub fn make_executable(path: &Path) -> Result<()> { /// Never returns an error; the fallible signature matches the Unix variant so /// callers need no platform-specific handling. #[cfg(not(unix))] -pub fn make_executable(_path: &Path) -> Result<()> { +pub const fn make_executable(_path: &Path) -> Result<()> { Ok(()) } diff --git a/test_support/src/fs.rs b/test_support/src/fs.rs index a2afd5754..f07f7a360 100644 --- a/test_support/src/fs.rs +++ b/test_support/src/fs.rs @@ -21,6 +21,10 @@ use std::io; use std::path::Path; use std::time::SystemTime; +#[path = "canonicalize.rs"] +mod canonicalize; +pub use canonicalize::canonicalize; + /// The state observed when inspecting a filesystem path. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum PathState { @@ -127,6 +131,27 @@ pub fn exists(path: impl AsRef) -> bool { pub fn is_dir(path: impl AsRef) -> bool { fs::metadata(path).is_ok_and(|metadata| metadata.is_dir()) } +/// Read metadata without allowing a descendant of a regular file to alias it. +fn metadata(path: &Path) -> io::Result { + for ancestor in path + .ancestors() + .skip(1) + .filter(|ancestor| !ancestor.as_os_str().is_empty()) + { + match fs::metadata(ancestor) { + Ok(metadata) if !metadata.is_dir() => { + return Err(io::Error::new( + io::ErrorKind::NotADirectory, + format!("non-directory path ancestor: {}", ancestor.display()), + )); + } + Ok(_) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + } + fs::metadata(path) +} /// Inspect whether `path` is absent, a directory, or another target. /// @@ -157,7 +182,7 @@ pub fn is_dir(path: impl AsRef) -> bool { /// ); /// ``` pub fn inspect_path(path: impl AsRef) -> io::Result { - match fs::metadata(path) { + match metadata(path.as_ref()) { Ok(metadata) if metadata.is_dir() => Ok(PathState::Directory), Ok(_) => Ok(PathState::NonDirectory), Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(PathState::Absent), @@ -186,7 +211,7 @@ pub fn inspect_path(path: impl AsRef) -> io::Result { /// assert!(!test_support::fs::try_is_file(dir.path().join("absent")).expect("inspect absent")); /// ``` pub fn try_is_file(path: impl AsRef) -> io::Result { - match fs::metadata(path) { + match metadata(path.as_ref()) { Ok(metadata) => Ok(metadata.is_file()), Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(false), Err(error) => Err(error), diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 6de82758a..77f1088b8 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -54,7 +54,7 @@ pub use manifest::ensure_manifest_exists; pub use exec::{make_executable, write_exec, write_exec_with_content}; mod error; -#[cfg(test)] +#[cfg(all(test, unix))] mod tracing_capture; use anyhow::{Context, Result}; /// Format an error and its sources (outermost → root) using `Display`, joined diff --git a/test_support/src/manifest/tests.rs b/test_support/src/manifest/tests.rs index 523ab6925..0b5118085 100644 --- a/test_support/src/manifest/tests.rs +++ b/test_support/src/manifest/tests.rs @@ -61,7 +61,7 @@ fn non_directory_parent_propagates_target_inspection_error( let (temp, temp_path) = temp_manifest_workspace?; let parent = temp.path().join("parent"); fs::write(&parent, b"file").context("write placeholder parent file")?; - let manifest = parent.join("manifest.yml"); + let manifest = temp_path.join("parent/manifest.yml"); let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("parent/manifest.yml")) else { anyhow::bail!("non-directory parent should error"); @@ -71,10 +71,7 @@ fn non_directory_parent_propagates_target_inspection_error( "target inspection error should not be treated as absence: {err}" ); let msg = err.to_string(); - let manifest_str = manifest - .to_str() - .ok_or_else(|| anyhow::anyhow!("manifest path is not valid UTF-8"))?; - anyhow::ensure!(msg.contains(manifest_str), "message: {msg}"); + anyhow::ensure!(msg.contains(manifest.as_str()), "message: {msg}"); Ok(()) } diff --git a/test_support/src/netsuke.rs b/test_support/src/netsuke.rs index 43506ccc5..58c7fff53 100644 --- a/test_support/src/netsuke.rs +++ b/test_support/src/netsuke.rs @@ -381,7 +381,7 @@ mod tests { for expected in [ root.join("build/debug").join(binary_name()), target_dir.join("debug").join(binary_name()), - target_dir.join("build/debug").join(binary_name()), + target_dir.join("build").join("debug").join(binary_name()), ] { ensure!( message.contains(expected.as_str()), diff --git a/tests/bdd/steps/conditional_manifest.rs b/tests/bdd/steps/conditional_manifest.rs index 27e2d8851..8755c30be 100644 --- a/tests/bdd/steps/conditional_manifest.rs +++ b/tests/bdd/steps/conditional_manifest.rs @@ -103,7 +103,11 @@ fn mark_executable(path: &Path) -> Result<()> { } #[cfg(not(unix))] -fn mark_executable(_path: &Path) -> Result<()> { +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared call site needs no platform-specific handling" +)] +const fn mark_executable(_path: &Path) -> Result<()> { Ok(()) } diff --git a/tests/bdd/steps/process.rs b/tests/bdd/steps/process.rs index ef2f0f2cb..eefe0b43c 100644 --- a/tests/bdd/steps/process.rs +++ b/tests/bdd/steps/process.rs @@ -4,18 +4,16 @@ use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use anyhow::{Context, Result, anyhow, ensure}; use camino::{Utf8Path, Utf8PathBuf}; use mockable::{DefaultEnv, Env}; +#[cfg(unix)] use netsuke::output_prefs; use netsuke::runner::{self, BuildTargets, CommandEnv, NINJA_PROGRAM}; use rstest_bdd_macros::{given, then, when}; use std::fs; use std::path::{Path, PathBuf}; use tempfile::TempDir; -use test_support::{ - check_ninja::{self, ToolName}, - ensure_manifest_exists, - env::prepend_path_value, - fake_ninja, -}; +#[cfg(unix)] +use test_support::check_ninja::ToolName; +use test_support::{check_ninja, ensure_manifest_exists, env::prepend_path_value, fake_ninja}; // --------------------------------------------------------------------------- // Helper functions @@ -84,6 +82,7 @@ fn prepare_cli_with_directory(world: &TestWorld) -> Result<()> { } /// Prepares the CLI for execution with an absolute file path. +#[cfg(unix)] fn prepare_cli_with_absolute_file(world: &TestWorld) -> Result<()> { prepare_cli_with_directory(world)?; world diff --git a/tests/bdd/steps/progress_output.rs b/tests/bdd/steps/progress_output.rs index c6abd7c21..2ef3317c0 100644 --- a/tests/bdd/steps/progress_output.rs +++ b/tests/bdd/steps/progress_output.rs @@ -26,7 +26,11 @@ fn make_script_executable(path: &Path) -> Result<()> { } #[cfg(not(unix))] -fn make_script_executable(_path: &Path) -> Result<()> { +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared call site needs no platform-specific handling" +)] +const fn make_script_executable(_path: &Path) -> Result<()> { Ok(()) } diff --git a/tests/bdd/steps/stdlib/assertions.rs b/tests/bdd/steps/stdlib/assertions.rs index 87e9c858d..e4b361ff7 100644 --- a/tests/bdd/steps/stdlib/assertions.rs +++ b/tests/bdd/steps/stdlib/assertions.rs @@ -3,6 +3,7 @@ use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use anyhow::{Context, Result, bail, ensure}; +use camino::Utf8Path; use cap_std::{ambient_authority, fs_utf8::Dir}; use rstest_bdd_macros::then; use std::fs; @@ -112,8 +113,9 @@ pub(crate) fn assert_fetch_cache_present(world: &TestWorld) -> Result<()> { #[then("the stdlib output equals the workspace root")] pub(crate) fn assert_stdlib_output_is_root(world: &TestWorld) -> Result<()> { let (root, output) = stdlib_root_and_output(world)?; + let actual = Utf8Path::new(&output); ensure!( - output == root.as_str(), + actual == root.as_path(), "expected output to equal workspace root" ); Ok(()) @@ -123,8 +125,9 @@ pub(crate) fn assert_stdlib_output_is_root(world: &TestWorld) -> Result<()> { pub(crate) fn assert_stdlib_output_is_workspace_path(world: &TestWorld, path: &str) -> Result<()> { let (root, output) = stdlib_root_and_output(world)?; let expected = root.join(path); + let actual = Utf8Path::new(&output); ensure!( - output == expected.as_str(), + actual == expected.as_path(), "expected output '{}', got '{output}'", expected ); @@ -139,8 +142,9 @@ pub(crate) fn assert_stdlib_output_is_workspace_executable( let relative = camino::Utf8PathBuf::from(path); let (root, output) = stdlib_root_and_output(world)?; let expected = resolve_executable_path(&root, relative.as_path()); + let actual = Utf8Path::new(&output); ensure!( - output == expected.as_str(), + actual == expected.as_path(), "expected stdlib output '{expected}' but was '{output}'" ); Ok(()) diff --git a/tests/bdd/steps/stdlib/workspace.rs b/tests/bdd/steps/stdlib/workspace.rs index 4d3a4875e..db5a6cb9f 100644 --- a/tests/bdd/steps/stdlib/workspace.rs +++ b/tests/bdd/steps/stdlib/workspace.rs @@ -208,23 +208,25 @@ const fn executable_script() -> &'static [u8] { } } +#[cfg(unix)] fn mark_executable(path: &Utf8Path) -> Result<()> { - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let mut perms = fs::metadata(path.as_std_path()) - .with_context(|| format!("stat stdlib executable {path}"))? - .permissions(); - perms.set_mode(0o755); - fs::set_permissions(path.as_std_path(), perms) - .with_context(|| format!("chmod stdlib executable {path}"))?; - Ok(()) - } - #[cfg(not(unix))] - { - let _ = path; - Ok(()) - } + use std::os::unix::fs::PermissionsExt; + let mut perms = fs::metadata(path.as_std_path()) + .with_context(|| format!("stat stdlib executable {path}"))? + .permissions(); + perms.set_mode(0o755); + fs::set_permissions(path.as_std_path(), perms) + .with_context(|| format!("chmod stdlib executable {path}"))?; + Ok(()) +} + +#[cfg(not(unix))] +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared call site needs no platform-specific handling" +)] +const fn mark_executable(_path: &Utf8Path) -> Result<()> { + Ok(()) } // --------------------------------------------------------------------------- diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index 6367e2d92..21914b1c0 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -11,7 +11,9 @@ use anyhow::{Context, Result, ensure}; use netsuke::runner::CommandEnv; use proptest::prelude::*; -use rstest::{fixture, rstest}; +#[cfg(unix)] +use rstest::fixture; +use rstest::rstest; use std::{ ffi::{OsStr, OsString}, path::PathBuf, diff --git a/tests/logging_stderr/json.rs b/tests/logging_stderr/json.rs index 1a2d0cb3c..b216e45b6 100644 --- a/tests/logging_stderr/json.rs +++ b/tests/logging_stderr/json.rs @@ -1,7 +1,10 @@ //! JSON diagnostic, result-envelope, and standard stderr integration tests. -use super::support::{open_workspace, temp_with_minimal_manifest, write_fake_ninja_script}; +#[cfg(unix)] +use super::support::write_fake_ninja_script; +use super::support::{open_workspace, temp_with_minimal_manifest}; use anyhow::{Context, Result, ensure}; +#[cfg(unix)] use camino::Utf8Path; #[cfg(unix)] use netsuke::runner::NINJA_ENV; diff --git a/tests/logging_stderr/support.rs b/tests/logging_stderr/support.rs index 84be3730d..37fcadd47 100644 --- a/tests/logging_stderr/support.rs +++ b/tests/logging_stderr/support.rs @@ -23,7 +23,11 @@ fn make_script_executable(dir: &Dir, path: &Utf8Path) -> Result<()> { } #[cfg(not(unix))] -fn make_script_executable(_dir: &Dir, _path: &Utf8Path) -> Result<()> { +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared call site needs no platform-specific handling" +)] +const fn make_script_executable(_dir: &Dir, _path: &Utf8Path) -> Result<()> { Ok(()) } @@ -108,21 +112,28 @@ pub(super) fn path_containing(dir: &Path) -> Result { pub(super) fn run_verbose_build_with_ninja_env( current_dir: &Path, - path_env: std::ffi::OsString, + path_env: Option, ninja_env: Option<&Path>, ) -> Result { let mut command = assert_cmd::cargo::cargo_bin_cmd!("netsuke"); command .current_dir(current_dir) - .env("PATH", path_env) .env_remove(NINJA_ENV) .arg("--verbose") .arg("build"); + if let Some(path) = path_env { + command.env("PATH", path); + } if let Some(ninja) = ninja_env { command.env(NINJA_ENV, ninja); } let output = command.output().context("run verbose netsuke build")?; - ensure!(output.status.success(), "expected verbose build to succeed"); + ensure!( + output.status.success(), + "expected verbose build to succeed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr) + ); String::from_utf8(output.stderr).context("stderr should be valid UTF-8") } diff --git a/tests/logging_stderr/verbose.rs b/tests/logging_stderr/verbose.rs index b767d687f..ba76ffde1 100644 --- a/tests/logging_stderr/verbose.rs +++ b/tests/logging_stderr/verbose.rs @@ -58,14 +58,20 @@ fn run_verbose_build_with_fake_ninja_and_assert_log( NinjaOverride::FullPath => Some(ninja_path.as_path()), NinjaOverride::Name => Some(Path::new(&ninja_name)), }; - let stderr = run_verbose_build_with_ninja_env( - workspace, - path_containing(ninja_temp.path())?, - ninja_env, - )?; + // Windows resolves the default `ninja` program as an executable, while + // the deterministic fixture is a batch script. Inherit the CI-provisioned + // Ninja only for that fallback branch; explicit overrides still use the + // fake through the injected PATH. + let path_env = if cfg!(windows) && matches!(override_mode, NinjaOverride::Unset) { + None + } else { + Some(path_containing(ninja_temp.path())?) + }; + let stderr = run_verbose_build_with_ninja_env(workspace, path_env, ninja_env)?; let expected = match override_mode { - NinjaOverride::Unset | NinjaOverride::Name => format!("Executing command: {ninja_stem} "), + NinjaOverride::Unset => format!("Executing command: {ninja_stem} "), + NinjaOverride::Name => format!("Executing command: {ninja_name} "), NinjaOverride::FullPath => format!("Executing command: {} ", ninja_path.display()), }; ensure!(stderr.contains(&expected), "{description}, got:\n{stderr}"); diff --git a/tests/manifest_glob_tests.rs b/tests/manifest_glob_tests.rs index 486610880..443538de0 100644 --- a/tests/manifest_glob_tests.rs +++ b/tests/manifest_glob_tests.rs @@ -106,8 +106,8 @@ fn temp_dir() -> tempfile::TempDir { #[case(GlobTestCase { setup: TestFiles { files: &[("b.txt", "b"), ("a.txt", "a")], dirs: &[] }, pattern_suffix: "*.txt", - name_template: "{{ item | replace('{dir}/', '') | replace('{dir}\\\\', '') | replace('.txt', '.out') }}", - expected_partial: &["a.out", "b.out"], + name_template: "{{ item }}", + expected_partial: &["a.txt", "b.txt"], description: "expands and sorts matches", })] #[case(GlobTestCase { @@ -182,12 +182,20 @@ fn test_glob_behavior(temp_dir: tempfile::TempDir, #[case] case: GlobTestCase) - case.description ); } else { - let prefix_fwd = format!("{dir_fwd}/"); - let prefix_back = format!("{dir_str}\\"); let names: Vec<_> = target_names(&manifest)? .into_iter() - .map(|n| n.replace(&prefix_fwd, "").replace(&prefix_back, "")) - .collect(); + .map(|name| { + let path = Path::new(&name); + let relative = if path.is_absolute() { + path.strip_prefix(temp_dir.path()).with_context(|| { + format!("absolute glob target should be under temporary dir: {name}") + })? + } else { + path + }; + Ok(relative.to_string_lossy().replace('\\', "/")) + }) + .collect::>()?; ensure!(names == case.expected_partial, "{}", case.description); } Ok(()) @@ -318,12 +326,20 @@ fn glob_accepts_windows_path_separators(temp_dir: tempfile::TempDir) -> Result<( pattern = pattern, )); let manifest = manifest::from_str(&yaml)?; - let prefix_fwd = format!("{dir_fwd}/"); let names: Vec<_> = target_names(&manifest)? .into_iter() - .map(|n| n.replace(&prefix_fwd, "").replace(".txt", ".out")) - .collect(); - ensure!(names == ["a.out", "b.out"]); + .map(|name| { + let file_name = Path::new(&name) + .file_name() + .with_context(|| format!("glob target name should include a file name: {name}"))? + .to_string_lossy(); + Ok(file_name.replace(".txt", ".out")) + }) + .collect::>()?; + ensure!( + names == ["a.out", "b.out"], + "unexpected target names: {names:?}" + ); Ok(()) } diff --git a/tests/ninja_gen_command_list_integration_tests.rs b/tests/ninja_gen_command_list_integration_tests.rs index 155a44bc1..7d618ae15 100644 --- a/tests/ninja_gen_command_list_integration_tests.rs +++ b/tests/ninja_gen_command_list_integration_tests.rs @@ -1,9 +1,11 @@ -//! Real-Ninja regressions for command-list shell boundaries. +//! Real-Ninja regressions for POSIX command-list shell boundaries. //! //! These tests cover syntax which would escape a directly interpolated brace //! group and therefore require the generated command to evaluate each entry as //! a complete shell unit. +#![cfg(unix)] + use anyhow::{Context, Result, ensure}; use camino::Utf8PathBuf; use cap_std::{ambient_authority, fs_utf8::Dir}; diff --git a/tests/ninja_gen_command_list_process_integration_tests.rs b/tests/ninja_gen_command_list_process_integration_tests.rs index f19262b42..b10ade034 100644 --- a/tests/ninja_gen_command_list_process_integration_tests.rs +++ b/tests/ninja_gen_command_list_process_integration_tests.rs @@ -4,6 +4,8 @@ //! generated Ninja file, separately from the broader generator integration //! scenarios. +#![cfg(unix)] + use anyhow::{Context, Result, ensure}; use camino::Utf8PathBuf; use cap_std::{ambient_authority, fs_utf8::Dir}; diff --git a/tests/ninja_gen_integration_tests.rs b/tests/ninja_gen_integration_tests.rs index c84b7fce4..222ecee7f 100644 --- a/tests/ninja_gen_integration_tests.rs +++ b/tests/ninja_gen_integration_tests.rs @@ -6,22 +6,30 @@ use anyhow::{Context, Result, bail, ensure}; use camino::Utf8PathBuf; +#[cfg(unix)] use cap_std::{ambient_authority, fs_utf8::Dir}; use netsuke::ast::Recipe; use netsuke::ir::{Action, BuildEdge, BuildGraph}; use netsuke::ninja_gen::{NinjaGenError, generate, generate_into}; -use rstest::{fixture, rstest}; +#[cfg(unix)] +use rstest::fixture; +use rstest::rstest; +#[cfg(unix)] use std::process::Command; +#[cfg(unix)] use tempfile::TempDir; +#[cfg(unix)] use test_support::ninja_gen::{self, AssertionType, NinjaIntegrationCase}; /// Provide a temporary directory when Ninja is available, skipping otherwise. +#[cfg(unix)] #[fixture] fn ninja_integration_setup() -> Option { ninja_gen::ninja_integration_setup() } /// Integration scenarios to confirm Ninja executes commands correctly. +#[cfg(unix)] #[rstest] #[case::multiline_script_valid(NinjaIntegrationCase { action: Action { diff --git a/tests/ninja_snapshot_tests.rs b/tests/ninja_snapshot_tests.rs index 49de45e0d..097f30725 100644 --- a/tests/ninja_snapshot_tests.rs +++ b/tests/ninja_snapshot_tests.rs @@ -8,10 +8,16 @@ use anyhow::{Context, Result, ensure}; use cap_std::{ambient_authority, fs_utf8::Dir}; use insta::{Settings, assert_snapshot}; use netsuke::{ir::BuildGraph, manifest, ninja_gen, stdlib::StdlibConfig}; -use std::{fs, process::Command}; +#[cfg(unix)] +use std::process::Command; +#[cfg(unix)] use tempfile::tempdir; +#[cfg(unix)] use test_support::ensure_binaries_available; +#[cfg(unix)] +use test_support::fs; +#[cfg(unix)] fn run_ok(cmd: &mut Command) -> Result { let out = cmd.output().context("failed to spawn command")?; let status = out.status; @@ -25,6 +31,7 @@ fn run_ok(cmd: &mut Command) -> Result { } #[test] +#[cfg(unix)] fn touch_manifest_ninja_validation() -> Result<()> { if let Err(err) = ensure_binaries_available(&[("ninja", &["--version"]), ("python3", &["--version"])]) diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index ee823e51e..6e19f350f 100644 --- a/tests/packaging_smoke_tests.rs +++ b/tests/packaging_smoke_tests.rs @@ -70,14 +70,19 @@ fn packaged_manifest_retains_build_script_sources() { let packaged_manifest = String::from_utf8_lossy(&list_output.stdout); let packaged_paths = packaged_manifest .lines() - .map(str::trim) + .map(|path| normalize_packaged_path(path.trim())) .collect::>(); assert_required_paths_present(&packaged_paths); assert_forbidden_roots_absent(&packaged_paths); } -fn assert_required_paths_present(packaged_paths: &BTreeSet<&str>) { +/// Normalize Cargo's platform-native package-list separators for comparison. +fn normalize_packaged_path(path: &str) -> String { + path.replace('\\', "/") +} + +fn assert_required_paths_present(packaged_paths: &BTreeSet) { for required_path in REQUIRED_PACKAGED_FILES { assert!( packaged_paths.contains(required_path), @@ -93,7 +98,7 @@ fn assert_required_paths_present(packaged_paths: &BTreeSet<&str>) { } } -fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet<&str>) { +fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet) { for forbidden_root in FORBIDDEN_PACKAGED_ROOTS { // Name the offending entry: knowing only the forbidden root leaves the // reader grepping the packaged manifest by hand. @@ -109,7 +114,7 @@ fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet<&str>) { assert!( offender.is_none(), "packaged manifest should not contain `{forbidden_root}`, found `{}`", - offender.copied().unwrap_or_default() + offender.map(String::as_str).unwrap_or_default() ); } @@ -120,3 +125,11 @@ fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet<&str>) { "packaged manifest should not contain stale `ninja_env` paths" ); } + +#[test] +fn normalized_packaged_path_accepts_windows_separator_spelling() { + assert_eq!( + normalize_packaged_path("build_l10n_audit\\mod.rs"), + normalize_packaged_path("build_l10n_audit/mod.rs") + ); +} diff --git a/tests/polonius_toolchain_contract.rs b/tests/polonius_toolchain_contract.rs index 3d2632d1b..f70e0f8e1 100644 --- a/tests/polonius_toolchain_contract.rs +++ b/tests/polonius_toolchain_contract.rs @@ -44,6 +44,13 @@ const CI_WORKFLOW: WorkflowExpectation = WorkflowExpectation { rustflags: WARNINGS_POLONIUS_RUSTFLAGS, pins_toolchain_env: true, }; +const CI_WINDOWS_WORKFLOW: WorkflowExpectation = WorkflowExpectation { + path: ".github/workflows/ci.yml", + job: "build-test-windows", + action: SETUP_RUST_ACTION, + rustflags: WARNINGS_POLONIUS_RUSTFLAGS, + pins_toolchain_env: true, +}; const NETSUKEFILE_WORKFLOW: WorkflowExpectation = WorkflowExpectation { path: ".github/workflows/netsukefile-test.yml", job: "netsukefile", @@ -67,8 +74,9 @@ const PACKAGING_WORKFLOW: WorkflowExpectation = WorkflowExpectation { }; /// Every workflow under the shared-action toolchain contract. -const WORKFLOW_EXPECTATIONS: [WorkflowExpectation; 4] = [ +const WORKFLOW_EXPECTATIONS: [WorkflowExpectation; 5] = [ CI_WORKFLOW, + CI_WINDOWS_WORKFLOW, NETSUKEFILE_WORKFLOW, COVERAGE_WORKFLOW, PACKAGING_WORKFLOW, @@ -167,6 +175,7 @@ fn makefile_declares_the_polonius_flags_variable() -> Result<()> { #[rstest] #[case::ci(CI_WORKFLOW)] +#[case::ci_windows(CI_WINDOWS_WORKFLOW)] #[case::netsukefile(NETSUKEFILE_WORKFLOW)] #[case::coverage(COVERAGE_WORKFLOW)] #[case::packaging(PACKAGING_WORKFLOW)] diff --git a/tests/runner_cases/default_targets.rs b/tests/runner_cases/default_targets.rs index 6b604bc33..a858b8253 100644 --- a/tests/runner_cases/default_targets.rs +++ b/tests/runner_cases/default_targets.rs @@ -1,4 +1,9 @@ //! Unix-only runner tests covering CLI default-target execution. +//! +//! The whole crate is Unix-only: it drives a fake `ninja` shell script and +//! the Unix-only `FakeNinjaFixture`, neither of which exists on Windows. + +#![cfg(unix)] use anyhow::{Context, Result, ensure}; use netsuke::cli::{BuildArgs, Cli, Commands}; diff --git a/tests/runner_tool_subcommands_tests.rs b/tests/runner_tool_subcommands_tests.rs index 874c346ef..0f26327e1 100644 --- a/tests/runner_tool_subcommands_tests.rs +++ b/tests/runner_tool_subcommands_tests.rs @@ -3,6 +3,11 @@ //! Covers the `clean` subcommand which still invokes `ninja -t `. The //! `graph` subcommand renders in-process and is covered by //! `tests/runner_graph_tests.rs`. +//! +//! The whole crate is Unix-only: it drives a fake `ninja` shell script and the +//! Unix-only `check_ninja` factories, neither of which exists on Windows. + +#![cfg(unix)] use anyhow::{Context, Result, bail, ensure}; use netsuke::cli::{Cli, Commands}; diff --git a/tests/serial_dependency_cli_tests.rs b/tests/serial_dependency_cli_tests.rs index 13ede176b..30cd2a6c5 100644 --- a/tests/serial_dependency_cli_tests.rs +++ b/tests/serial_dependency_cli_tests.rs @@ -1,5 +1,7 @@ //! Public-CLI end-to-end tests for serial dependency sidecar publication. +#![cfg(unix)] + use anyhow::{Context, Result, anyhow, ensure}; use camino::{Utf8Path, Utf8PathBuf}; use cap_std::{ambient_authority, fs_utf8::Dir}; diff --git a/tests/serial_dependency_runtime_tests.rs b/tests/serial_dependency_runtime_tests.rs index f49465b95..87966920b 100644 --- a/tests/serial_dependency_runtime_tests.rs +++ b/tests/serial_dependency_runtime_tests.rs @@ -5,6 +5,8 @@ //! failure short-circuiting, and shared-work reuse. They deliberately assert //! observable behaviour rather than generated text. +#![cfg(unix)] + use anyhow::{Context, Result, ensure}; use camino::Utf8PathBuf; use cap_std::{ambient_authority, fs_utf8::Dir}; diff --git a/tests/snapshots/which_diagnostic_snapshot_tests__which_direct_not_found@windows.snap b/tests/snapshots/which_diagnostic_snapshot_tests__which_direct_not_found@windows.snap new file mode 100644 index 000000000..b44679411 --- /dev/null +++ b/tests/snapshots/which_diagnostic_snapshot_tests__which_direct_not_found@windows.snap @@ -0,0 +1,5 @@ +--- +source: tests/which_diagnostic_snapshot_tests.rs +expression: normalized +--- +invalid operation: netsuke::jinja::which::not_found: [netsuke::jinja::which::not_found] command '⁨./absent⁩' at '⁨[WORKSPACE]\./absent⁩' is missing or not executable. (in :1) diff --git a/tests/snapshots/which_diagnostic_snapshot_tests__which_not_found@windows.snap b/tests/snapshots/which_diagnostic_snapshot_tests__which_not_found@windows.snap new file mode 100644 index 000000000..60d785f52 --- /dev/null +++ b/tests/snapshots/which_diagnostic_snapshot_tests__which_not_found@windows.snap @@ -0,0 +1,5 @@ +--- +source: tests/which_diagnostic_snapshot_tests.rs +expression: normalized +--- +invalid operation: netsuke::jinja::which::not_found: [netsuke::jinja::which::not_found] command '⁨absent⁩' not found after checking ⁨0⁩ PATH entries. Preview: ⁨⁩. Set cwd_mode="always" to include the current directory. (in :1) diff --git a/tests/std_filter_tests/command_filters/grep_filter_tests.rs b/tests/std_filter_tests/command_filters/grep_filter_tests.rs index ba8c9fc03..618654b26 100644 --- a/tests/std_filter_tests/command_filters/grep_filter_tests.rs +++ b/tests/std_filter_tests/command_filters/grep_filter_tests.rs @@ -1,13 +1,18 @@ //! Grep filter behaviour tests. use anyhow::{Context, Result, bail, ensure}; +#[cfg(not(windows))] use cap_std::{ambient_authority, fs_utf8::Dir}; use minijinja::{ErrorKind, context}; use rstest::rstest; +#[cfg(not(windows))] use test_support::fluent::normalize_fluent_isolates; +#[cfg(not(windows))] use test_support::fs; -use super::{StdlibConfig, fallible, streaming_match_payload}; +use super::fallible; +#[cfg(not(windows))] +use super::{StdlibConfig, streaming_match_payload}; #[cfg(not(windows))] #[rstest] 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 d1159e427..cb07b62a6 100644 --- a/tests/std_filter_tests/command_filters/windows_filter_tests.rs +++ b/tests/std_filter_tests/command_filters/windows_filter_tests.rs @@ -9,12 +9,12 @@ use camino::Utf8PathBuf; use cap_std::{ambient_authority, fs_utf8::Dir}; use minijinja::context; use mockable::{DefaultEnv, Env}; -use rstest::{fixture, rstest}; +use rstest::rstest; use std::ffi::OsString; -use std::fs; use tempfile::tempdir; use test_support::command_helper::compile_rust_helper; use test_support::env::prepend_path_value; +use test_support::fs as test_fs; use super::{StdlibConfig, fallible, streaming_match_payload}; @@ -59,6 +59,7 @@ const ARGS_STUB: &str = concat!( ); /// Error context messages for Windows command helper setup. +#[derive(Copy, Clone)] struct WindowsSetupContext { tempdir: &'static str, root: &'static str, @@ -115,14 +116,14 @@ fn grep_on_windows_bypasses_shell() -> Result<()> { )?; let config = StdlibConfig::from_current_dir()?.with_command_path_override(path); - let (mut env, mut state) = fallible::stdlib_env_with_config(config)?; + let (mut env, state) = fallible::stdlib_env_with_config(config)?; state.reset_impure(); fallible::register_template( &mut env, "grep_win", - r#"{{ 'line1 + r"{{ 'line1 line2 -' | grep('^line2') | trim }}"#, +' | grep('^line2') | trim }}", )?; let template = env .get_template("grep_win") @@ -155,7 +156,7 @@ fn grep_streams_large_output_on_windows() -> Result<()> { .with_command_max_output_bytes(512)? .with_command_max_stream_bytes(200_000)? .with_command_path_override(path); - let (mut env, mut state) = fallible::stdlib_env_with_config(config)?; + let (mut env, state) = fallible::stdlib_env_with_config(config)?; state.reset_impure(); fallible::register_template( &mut env, @@ -173,15 +174,15 @@ fn grep_streams_large_output_on_windows() -> Result<()> { state.is_impure(), "grep streaming should mark template impure" ); - let path = camino::Utf8Path::new(rendered.as_str()); - let metadata = fs::metadata(path.as_std_path()) - .with_context(|| format!("stat streamed windows grep output {}", path))?; + let rendered_path = camino::Utf8Path::new(rendered.as_str()); + let rendered_len = test_fs::file_len(rendered_path.as_std_path()) + .with_context(|| format!("stat streamed windows grep output {rendered_path}"))?; ensure!( - metadata.len() >= payload.len() as u64, + rendered_len >= payload.len() as u64, "streamed grep output should retain payload size" ); - let contents = fs::read_to_string(path.as_std_path()) - .with_context(|| format!("read streamed windows grep output {}", path))?; + let contents = test_fs::read_to_string(rendered_path.as_std_path()) + .with_context(|| format!("read streamed windows grep output {rendered_path}"))?; ensure!( contents == payload, "streamed grep file should contain the helper payload" @@ -202,9 +203,9 @@ fn shell_preserves_cmd_meta_characters() -> Result<()> { ARGS_STUB, )?; - let command = format!("\"{}\" \"literal %%^!\"", exe); + let command = format!("\"{exe}\" \"literal %%^!\""); let config = StdlibConfig::from_current_dir()?.with_command_path_override(path); - let (mut env, mut state) = fallible::stdlib_env_with_config(config)?; + let (mut env, state) = fallible::stdlib_env_with_config(config)?; state.reset_impure(); fallible::register_template(&mut env, "shell_meta", "{{ '' | shell(cmd) }}")?; let template = env @@ -214,8 +215,8 @@ fn shell_preserves_cmd_meta_characters() -> Result<()> { .render(context!(cmd => command)) .context("render shell meta template")?; ensure!( - rendered.trim() == "literal %^!", - "expected literal %^! but rendered {rendered}" + rendered.trim() == "literal %%^!", + "expected literal %%^! but rendered {rendered}" ); ensure!( state.is_impure(), diff --git a/tests/std_filter_tests/path_filters.rs b/tests/std_filter_tests/path_filters.rs index 73c7ff948..3b09dc525 100644 --- a/tests/std_filter_tests/path_filters.rs +++ b/tests/std_filter_tests/path_filters.rs @@ -4,7 +4,9 @@ //! `with_suffix`, `realpath`, and `expanduser`. Each test validates filter //! behaviour with various inputs and error conditions. -use anyhow::{Context, Result, anyhow, bail, ensure}; +#[cfg(unix)] +use anyhow::anyhow; +use anyhow::{Context, Result, bail, ensure}; use camino::Utf8Path; use cap_std::{ambient_authority, fs_utf8::Dir}; use minijinja::{Environment, ErrorKind}; diff --git a/tests/std_filter_tests/which_filter_common.rs b/tests/std_filter_tests/which_filter_common.rs index 7294176e6..8fe627f12 100644 --- a/tests/std_filter_tests/which_filter_common.rs +++ b/tests/std_filter_tests/which_filter_common.rs @@ -110,13 +110,40 @@ pub(crate) fn write_tool(dir: &Utf8Path, name: &ToolName) -> Result Ok(path) } +/// Format a fixture path as the public `which` result renders it. +/// +/// This is deliberately limited to the shared `which` integration fixtures: +/// callers select whether they expect the resolver's raw or canonical mode, +/// while this helper preserves the output contract common to both modes. +pub(crate) fn expected_which_output_path(path: &Utf8Path) -> String { + #[cfg(windows)] + { + path.as_str().replace('\\', "/") + } + #[cfg(not(windows))] + { + path.as_str().to_owned() + } +} + +/// Canonicalize a fixture path and format it as a canonical `which` result. +pub(crate) fn expected_canonical_which_output_path(path: &Utf8Path) -> Result { + let canonical_path = + fs::canonicalize(path).with_context(|| format!("canonicalize fixture path {path}"))?; + Ok(expected_which_output_path(&canonical_path)) +} + #[cfg(unix)] fn mark_executable(path: &Utf8Path) -> Result<()> { fs::set_mode(path, 0o755).with_context(|| format!("chmod {path:?}")) } #[cfg(not(unix))] -fn mark_executable(_path: &Utf8Path) -> Result<()> { +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared call site needs no platform-specific handling" +)] +const fn mark_executable(_path: &Utf8Path) -> Result<()> { Ok(()) } diff --git a/tests/std_filter_tests/which_filter_tests.rs b/tests/std_filter_tests/which_filter_tests.rs index 807f90f00..c806f2d3d 100644 --- a/tests/std_filter_tests/which_filter_tests.rs +++ b/tests/std_filter_tests/which_filter_tests.rs @@ -34,9 +34,10 @@ fn test_cache_after_removal( .first() .context("cache-removal fixture should contain a tool path")? .clone(); + let expected_path = expected_which_output_path(&removed_path); ensure!( - first == removed_path.as_str(), - "initial which lookup should resolve {removed_path}, got {first}" + first == expected_path, + "initial which lookup should resolve {expected_path}, got {first}" ); fs::remove_file(&removed_path)?; @@ -55,8 +56,8 @@ fn test_cache_after_removal( } else { let second = second_result?; ensure!( - second == removed_path.as_str(), - "cached lookup should preserve {removed_path}, got {second}" + second == expected_path, + "cached lookup should preserve {expected_path}, got {second}" ); } @@ -92,10 +93,15 @@ fn test_duplicate_paths( let output = render_and_assert_pure(fixture, &template)?; let parts: Vec<&str> = output.split('|').collect(); - let expected_path = fixture + let fixture_path = fixture .paths .first() .context("duplicate-path fixture should contain a tool path")?; + let expected_output_path = if canonical { + expected_canonical_which_output_path(fixture_path)? + } else { + expected_which_output_path(fixture_path) + }; ensure!( parts.len() == expected_count, @@ -104,8 +110,8 @@ fn test_duplicate_paths( ); for part in &parts { ensure!( - *part == expected_path.as_str(), - "expected duplicate path {expected_path}, got {part}" + *part == expected_output_path, + "expected duplicate path {expected_output_path}, got {part}" ); } @@ -127,9 +133,10 @@ fn test_cwd_mode_resolution(cwd_mode_value: &str) -> Result<()> { "{{{{ which('local', cwd_mode='{cwd_mode_value}') }}}}" )); let output = render(&mut env, &template)?; + let expected_path = expected_which_output_path(&tool); ensure!( - output == tool.as_str(), - "cwd_mode {cwd_mode_value} should resolve {tool}, got {output}" + output == expected_path, + "cwd_mode {cwd_mode_value} should resolve {expected_path}, got {output}" ); Ok(()) } @@ -230,9 +237,10 @@ fn which_resolver_honours_workspace_root_override() -> Result<()> { let (mut env, _state) = fallible::stdlib_env_with_config(config.with_path_override(path.into_inner()))?; let output = render(&mut env, &Template::from("{{ 'helper' | which }}"))?; + let expected_path = expected_which_output_path(&tool); ensure!( - output == tool.as_str(), - "workspace override should resolve {tool}, got {output}" + output == expected_path, + "workspace override should resolve {expected_path}, got {output}" ); Ok(()) } diff --git a/tests/stderr_routing_tests.rs b/tests/stderr_routing_tests.rs index 9bb7df6da..a7105a848 100644 --- a/tests/stderr_routing_tests.rs +++ b/tests/stderr_routing_tests.rs @@ -15,6 +15,7 @@ use netsuke::runner::{ BuildTargets, CommandEnv, NinjaBuildRequest, NinjaProcessOptions, NinjaToolRequest, StderrMode, run_ninja_tool_with, run_ninja_with, }; +use rstest::rstest; use std::path::{Path, PathBuf}; use std::process::Command; use tempfile::{TempDir, tempdir}; @@ -58,6 +59,8 @@ fn run_routing_worker(job: &str, tool: bool, ninja: &Path, ran_file: &Path) -> R .env(RAN_FILE_ENV, ran_file); if tool { command.env(TOOL_ENV, "1"); + } else { + command.env_remove(TOOL_ENV); } Ok(command) } @@ -163,30 +166,13 @@ fn routing_worker() -> Result<()> { result.context("run Ninja invocation in routing worker") } -/// A build request carrying `stderr_mode=Forward`: the child markers must -/// reach the user's stdout and stderr. -#[test] -fn forward_request_routes_child_streams() -> Result<()> { - assert_routing_case(StderrMode::Forward, false) -} - -/// A tool request carrying `stderr_mode=Forward`: the child markers must -/// reach the user's stdout and stderr. -#[test] -fn forward_tool_request_routes_child_streams() -> Result<()> { - assert_routing_case(StderrMode::Forward, true) -} - -/// A build request carrying `stderr_mode=Suppress`: both child markers must be -/// drained, and the run marker proves the child really executed. -#[test] -fn suppress_request_drains_child_streams() -> Result<()> { - assert_routing_case(StderrMode::Suppress, false) -} - -/// A tool request carrying `stderr_mode=Suppress`: both child markers must be -/// drained, with the run marker proving the child executed. -#[test] -fn suppress_tool_request_drains_child_streams() -> Result<()> { - assert_routing_case(StderrMode::Suppress, true) +/// Each explicit stderr policy routes both build and tool request streams. +/// Each case proves the process boundary honours `stderr_mode` alone. +#[rstest] +#[case::forward_build(StderrMode::Forward, false)] +#[case::forward_tool(StderrMode::Forward, true)] +#[case::suppress_build(StderrMode::Suppress, false)] +#[case::suppress_tool(StderrMode::Suppress, true)] +fn request_routes_child_streams(#[case] stderr_mode: StderrMode, #[case] tool: bool) -> Result<()> { + assert_routing_case(stderr_mode, tool) } diff --git a/tests/stdlib_which_pathext_tests.rs b/tests/stdlib_which_pathext_tests.rs index d9590ac96..7dc98fa15 100644 --- a/tests/stdlib_which_pathext_tests.rs +++ b/tests/stdlib_which_pathext_tests.rs @@ -93,10 +93,10 @@ fn assert_which_resolves_to( expected: &Utf8Path, ) -> Result<()> { let rendered = env.render_str(&format!("{{{{ which('{command}') }}}}"), context! {})?; - let expected = rendered_form(expected); + let expected_form = rendered_form(expected); ensure!( - rendered == expected, - "expected which('{command}') to render {expected}, got {rendered}" + rendered == expected_form, + "expected which('{command}') to render {expected_form}, got {rendered}" ); Ok(()) } diff --git a/tests/stdlib_which_tests.rs b/tests/stdlib_which_tests.rs index 2e545fb0e..58b96997e 100644 --- a/tests/stdlib_which_tests.rs +++ b/tests/stdlib_which_tests.rs @@ -98,7 +98,11 @@ fn mark_executable(path: &Utf8Path) -> Result<()> { } #[cfg(not(unix))] -fn mark_executable(_path: &Utf8Path) -> Result<()> { +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared call site needs no platform-specific handling" +)] +const fn mark_executable(_path: &Utf8Path) -> Result<()> { Ok(()) } diff --git a/tests/which_diagnostic_snapshot_tests.rs b/tests/which_diagnostic_snapshot_tests.rs index 17866be9a..303a5f784 100644 --- a/tests/which_diagnostic_snapshot_tests.rs +++ b/tests/which_diagnostic_snapshot_tests.rs @@ -70,7 +70,11 @@ fn mark_executable(path: &Utf8Path) -> Result<()> { } #[cfg(not(unix))] -fn mark_executable(_path: &Utf8Path) -> Result<()> { +#[expect( + clippy::unnecessary_wraps, + reason = "the fallible signature must match the Unix variant so the shared write_tool call site needs no platform-specific handling" +)] +const fn mark_executable(_path: &Utf8Path) -> Result<()> { Ok(()) } @@ -85,6 +89,19 @@ fn normalize_error(message: &str, root: &Utf8Path) -> String { message.replace(root.as_str(), "[WORKSPACE]") } +#[cfg(windows)] +fn platform_snapshot_name(snapshot: &str) -> String { + match snapshot { + "which_not_found" | "which_direct_not_found" => format!("{snapshot}@windows"), + _ => snapshot.to_owned(), + } +} + +#[cfg(not(windows))] +const fn platform_snapshot_name(snapshot: &str) -> &str { + snapshot +} + #[rstest] #[case::not_found("which_not_found", "{{ 'absent' | which(cwd_mode='never') }}")] #[case::direct_not_found("which_direct_not_found", "{{ './absent' | which }}")] @@ -105,6 +122,6 @@ fn which_diagnostic_messages_match_baseline( let message = render_error(&env, template)?; let normalized = normalize_error(&message, &workspace_fixture.root); - insta::assert_snapshot!(snapshot, normalized); + insta::assert_snapshot!(platform_snapshot_name(snapshot), normalized); Ok(()) } diff --git a/tests/workflow_build_and_package.rs b/tests/workflow_build_and_package.rs index 0afd6bc57..254a67fee 100644 --- a/tests/workflow_build_and_package.rs +++ b/tests/workflow_build_and_package.rs @@ -5,6 +5,7 @@ mod common; use anyhow::{Context, Result, ensure}; use common::workflow_contents; use rstest::rstest; +use serde_yaml::Value as YamlValue; use std::{fs, path::PathBuf}; use toml::Value; @@ -22,6 +23,56 @@ fn goreleaser_contents() -> Result { .with_context(|| format!("read GoReleaser contents from {}", path.display())) } +fn goreleaser_config() -> Result { + serde_yaml::from_str(&goreleaser_contents()?).context("parse GoReleaser YAML") +} + +/// The fallback build whose `pre` hook maps GOOS/GOARCH to Rust target +/// triples. Identified by `skip_build: true` plus the `id`, because the +/// platform-defined build variables (`GOOS`/`GOARCH`) are only meaningful to +/// `GoReleaser` at packaging time — the very hook this contract pins. +fn fallback_build(config: &YamlValue) -> Result<&YamlValue> { + config + .get("builds") + .and_then(YamlValue::as_sequence) + .context("GoReleaser config should declare builds")? + .iter() + .find(|build| { + build.get("skip_build").and_then(YamlValue::as_bool) == Some(true) + && build.get("id").and_then(YamlValue::as_str) == Some("netsuke") + }) + .context("GoReleaser config should declare the netsuke fallback build") +} + +/// Count the builds declaring a non-empty build-scoped `pre` hook. +fn build_pre_hook_count(config: &YamlValue) -> usize { + config + .get("builds") + .and_then(YamlValue::as_sequence) + .iter() + .flat_map(|builds| builds.iter()) + .filter(|build| { + build + .get("hooks") + .and_then(|hooks| hooks.get("pre")) + .and_then(YamlValue::as_sequence) + .is_some_and(|hooks| !hooks.is_empty()) + }) + .count() +} + +/// The fallback build is the only build allowed to carry a build-scoped `pre` +/// hook. A second build-level hook anywhere in the file would satisfy a +/// whole-file line scan without the fallback hook being reachable. +fn ensure_only_fallback_build_has_pre_hook(config: &YamlValue) -> Result<()> { + let build_pre_hooks = build_pre_hook_count(config); + ensure!( + build_pre_hooks == 1, + "the netsuke fallback build should be the only build declaring a pre hook, found {build_pre_hooks}" + ); + Ok(()) +} + fn staging_config() -> Result { release_staging_contents()? .parse::() @@ -215,24 +266,96 @@ fn behavioural_build_and_package_validates_release_help_tooling() { #[test] fn goreleaser_fallback_uses_rust_target_triple_orthohelp_paths() -> Result<()> { - let contents = goreleaser_contents()?; + let config = goreleaser_config()?; + let fallback = fallback_build(&config)?; + let pre_hook = fallback + .get("hooks") + .and_then(|hooks| hooks.get("pre")) + .and_then(YamlValue::as_sequence) + .and_then(|hooks| hooks.first()) + .and_then(YamlValue::as_str) + .context("the netsuke fallback build should declare a pre hook")?; ensure!( - !contents.contains("target/orthohelp/${{GOOS}-${GOARCH}}") - && !contents.contains("target/orthohelp/${GOOS}-${GOARCH}"), + !pre_hook.contains("target/orthohelp/${{GOOS}-${GOARCH}}") + && !pre_hook.contains("target/orthohelp/${GOOS}-${GOARCH}"), "GoReleaser fallback must not use raw GOOS/GOARCH orthohelp paths" ); ensure!( - contents.contains("x86_64-unknown-linux-gnu"), + pre_hook.contains("x86_64-unknown-linux-gnu"), "GoReleaser fallback should map linux/amd64 to the Rust target triple" ); ensure!( - contents.contains("target/orthohelp/${RUST_TARGET}/release/man/man1/netsuke.1"), + pre_hook.contains("target/orthohelp/${RUST_TARGET}/release/man/man1/netsuke.1"), "GoReleaser fallback should resolve orthohelp output through RUST_TARGET" ); ensure!( - contents.contains(" hooks:\n pre:") && !contents.contains("\nbefore:\n hooks:"), - "GoReleaser fallback should run where GOOS and GOARCH are defined" + pre_hook.contains("${GOOS}/${GOARCH}"), + "GoReleaser fallback hook should branch on GOOS/GOARCH so it runs where those are defined" + ); + + ensure_only_fallback_build_has_pre_hook(&config)?; + + ensure!( + !config + .as_mapping() + .is_some_and(|root| root.contains_key(YamlValue::String("before".to_owned()))), + "GoReleaser config must not fall back to the deprecated global before hook" + ); + Ok(()) +} +#[test] +fn goreleaser_fallback_pre_hook_guards_an_unrelated_build_level_hook() -> Result<()> { + // Reconstruct the file as parsed, with a second build that carries its own + // build-level `pre` hook. A line-scanning assertion would see the second + // `hooks: pre:` pair and pass even if the fallback hook were missing; the + // structural contract must reject that configuration. + let config = goreleaser_config()?; + let mut builds = config + .get("builds") + .and_then(YamlValue::as_sequence) + .cloned() + .unwrap_or_default(); + let mut unrelated = serde_yaml::Mapping::new(); + unrelated.insert( + YamlValue::String("id".to_owned()), + YamlValue::String("unrelated-package".to_owned()), + ); + let mut hooks = serde_yaml::Mapping::new(); + hooks.insert( + YamlValue::String("pre".to_owned()), + YamlValue::Sequence(vec![YamlValue::String("echo unrelated".to_owned())]), + ); + unrelated.insert( + YamlValue::String("hooks".to_owned()), + YamlValue::Mapping(hooks), + ); + builds.push(YamlValue::Mapping(unrelated)); + let mut edited = serde_yaml::Mapping::new(); + for (key, value) in config + .as_mapping() + .context("GoReleaser config should be a mapping")? + { + edited.insert(key.clone(), value.clone()); + } + edited.insert( + YamlValue::String("builds".to_owned()), + YamlValue::Sequence(builds), + ); + let edited_config = YamlValue::Mapping(edited); + + ensure!( + build_pre_hook_count(&edited_config) == 2, + "the regression fixture should contain exactly two build-level pre hooks" + ); + let Err(error) = ensure_only_fallback_build_has_pre_hook(&edited_config) else { + anyhow::bail!("the structural guard should reject a second unrelated build-level pre hook"); + }; + ensure!( + error + .to_string() + .contains("only build declaring a pre hook"), + "the guard should diagnose the unrelated build-level hook, got: {error}" ); Ok(()) } diff --git a/tests/workflow_ci.rs b/tests/workflow_ci.rs index a73e07170..235145cd0 100644 --- a/tests/workflow_ci.rs +++ b/tests/workflow_ci.rs @@ -45,6 +45,16 @@ fn job<'a>(workflow: &'a Value, name: &'static str) -> Result<&'a Mapping> { value_mapping(job_value, name) } +fn workflow_env(workflow: &Value, key: YamlKey) -> Result<&str> { + let root = value_mapping(workflow, "workflow")?; + let env = mapping_get(root, YamlKey("env")) + .context("workflow should define a workflow-level env") + .and_then(|value| value_mapping(value, "workflow-level env"))?; + mapping_get(env, key) + .and_then(Value::as_str) + .with_context(|| format!("workflow-level env should define {}", key.0)) +} + fn steps(job: &Mapping) -> Result<&Vec> { mapping_get(job, YamlKey("steps")) .context("job should define steps") @@ -161,32 +171,39 @@ fn unit_recognizes_exact_versions() { fn behavioural_ci_workflow_installs_pinned_cargo_nextest() -> Result<()> { let contents = workflow_contents("ci.yml").expect("CI workflow should be readable"); let workflow: Value = serde_yaml::from_str(&contents).context("parse CI workflow YAML")?; - let build_test = job(&workflow, "build-test")?; - let version = job_env(build_test, YamlKey("NEXTEST_VERSION")) - .context("build-test job should pin NEXTEST_VERSION")?; + let version = workflow_env(&workflow, YamlKey("NEXTEST_VERSION")) + .context("workflow-level env should pin NEXTEST_VERSION")?; ensure!( is_exact_version(version), "NEXTEST_VERSION should pin an exact version, found {version:?}" ); - let steps = steps(build_test)?; - let install = named_step(steps, "Install cargo-nextest")?; - let uses = mapping_get(install, YamlKey("uses")) - .and_then(Value::as_str) - .context("Install cargo-nextest step should reference an action")?; - ensure!( - is_pinned_action_ref(uses, "taiki-e/install-action"), - "cargo-nextest installer should be pinned to a full commit SHA, found {uses:?}" - ); - ensure!( - step_input(install, YamlKey("tool")) == Some("nextest@${{ env.NEXTEST_VERSION }}"), - "cargo-nextest installer should resolve its pin from NEXTEST_VERSION" - ); - ensure!( - !install.contains_key(Value::String("if".to_owned())), - "cargo-nextest should install on every matrix leg because every leg runs make test" - ); + for job_name in ["build-test", "build-test-windows"] { + let build_test = job(&workflow, job_name)?; + ensure!( + job_env(build_test, YamlKey("NEXTEST_VERSION")).is_none(), + "{job_name} should not duplicate NEXTEST_VERSION at job scope" + ); + + let steps = steps(build_test)?; + let install = named_step(steps, "Install cargo-nextest")?; + let uses = mapping_get(install, YamlKey("uses")) + .and_then(Value::as_str) + .context("Install cargo-nextest step should reference an action")?; + ensure!( + is_pinned_action_ref(uses, "taiki-e/install-action"), + "cargo-nextest installer should be pinned to a full commit SHA, found {uses:?}" + ); + ensure!( + step_input(install, YamlKey("tool")) == Some("nextest@${{ env.NEXTEST_VERSION }}"), + "cargo-nextest installer should resolve its pin from NEXTEST_VERSION" + ); + ensure!( + !install.contains_key(Value::String("if".to_owned())), + "cargo-nextest should install on every matrix leg because every leg runs make test" + ); + } Ok(()) } @@ -206,6 +223,11 @@ fn behavioural_ci_workflow_runs_tests_through_the_make_target() -> Result<()> { step_index(steps, "Install cargo-nextest")? < step_index(steps, "Test")?, "cargo-nextest should be installed before make test runs" ); + let setup_uv = named_step(steps, "Setup uv")?; + ensure!( + step_input(setup_uv, YamlKey("enable-cache")) == Some("false"), + "the manifest-free spelling job must not enable uv's automatic cache" + ); Ok(()) } @@ -235,9 +257,12 @@ fn behavioural_ci_workflow_wires_kani_smoke_job() -> Result<()> { .iter() .find_map(|step| step_name(step, "Install uv")) .context("Kani smoke job should include the Install uv step")?; + let uv_cache_enabled = mapping_get(install_uv_step, YamlKey("with")) + .and_then(Value::as_mapping) + .and_then(|with| mapping_get(with, YamlKey("enable-cache"))); ensure!( - !install_uv_step.contains_key(Value::String("with".to_owned())), - "Install uv step should not include a with configuration" + uv_cache_enabled == Some(&Value::Bool(false)), + "Install uv step should disable automatic caching because Kani uses an explicit cache" ); let cache_step = steps diff --git a/tests/workflow_contracts/ci_lint_test.py b/tests/workflow_contracts/ci_lint_test.py index 3bb07a873..27d164062 100644 --- a/tests/workflow_contracts/ci_lint_test.py +++ b/tests/workflow_contracts/ci_lint_test.py @@ -25,10 +25,12 @@ import re from pathlib import Path +import pytest import yaml REPO_ROOT = Path(__file__).resolve().parents[2] WORKFLOW_PATH = REPO_ROOT / ".github" / "workflows" / "ci.yml" +COVERAGE_MAIN_WORKFLOW_PATH = REPO_ROOT / ".github" / "workflows" / "coverage-main.yml" MAKEFILE_PATH = REPO_ROOT / "Makefile" TEST_SHELL_STEP = "Install test shell dependencies" @@ -49,6 +51,15 @@ class _WorkflowLoader(yaml.SafeLoader): """ +def _is_string(value: object) -> bool: + """Return whether a parsed YAML key has the required string shape.""" + match value: + case str(): + return True + case _: + return False + + # Drop the inherited YAML 1.1 bool resolver, then reinstate the 1.2 word set. _WorkflowLoader.yaml_implicit_resolvers = { initial: [ @@ -65,7 +76,7 @@ class _WorkflowLoader(yaml.SafeLoader): ) -def _load() -> dict[str, object]: +def _load(workflow_path: Path = WORKFLOW_PATH) -> dict[str, object]: """Parse the workflow file, rejecting anything but a mapping root. ``yaml.safe_load`` happily returns ``None`` for an empty document, or a @@ -76,20 +87,44 @@ def _load() -> dict[str, object]: # `yaml.load` is safe here: `_WorkflowLoader` derives from `SafeLoader`, so # it constructs no arbitrary Python objects. match yaml.load( - WORKFLOW_PATH.read_text(encoding="utf-8"), Loader=_WorkflowLoader + workflow_path.read_text(encoding="utf-8"), Loader=_WorkflowLoader ): case dict() as workflow: pass case other: - raise AssertionError( + pytest.fail( "the workflow must parse to a mapping, " f"got {type(other).__name__}" ) - non_string_keys = sorted(repr(key) for key in workflow if not isinstance(key, str)) + non_string_keys = sorted(repr(key) for key in workflow if not _is_string(key)) if non_string_keys: - raise AssertionError( + pytest.fail( f"the workflow mapping must be string-keyed, got {non_string_keys}" ) + match workflow.get("jobs"): + case dict() as jobs: + pass + case _: + pytest.fail("the workflow must declare a jobs mapping") + for job_name, job in jobs.items(): + if not _is_string(job_name): + pytest.fail(f"the jobs mapping must be string-keyed, got {job_name!r}") + match job: + case dict() as job_mapping: + pass + case _: + pytest.fail(f"jobs.{job_name} must be a mapping") + if "env" not in job_mapping: + continue + match job_mapping["env"]: + case dict() as env: + pass + case _: + pytest.fail(f"jobs.{job_name}.env must be a mapping") + if "NEXTEST_VERSION" in env: + pytest.fail( + f"jobs.{job_name}.env must not redeclare NEXTEST_VERSION" + ) return workflow @@ -99,17 +134,73 @@ def _steps(workflow: dict[str, object]) -> list[dict[str, object]]: case dict() as jobs: pass case _: - raise AssertionError("the workflow must declare a jobs mapping") + pytest.fail("the workflow must declare a jobs mapping") match jobs.get("build-test"): case dict() as job: pass case _: - raise AssertionError("the workflow must declare a build-test job") + pytest.fail("the workflow must declare a build-test job") + match job.get("steps"): + case list() as steps: + return steps + case _: + pytest.fail("jobs.build-test.steps must be a list") + + +def _coverage_upload_steps(workflow: dict[str, object]) -> list[dict[str, object]]: + """Return the main-branch coverage-upload job's steps.""" + match workflow.get("jobs"): + case dict() as jobs: + pass + case _: + pytest.fail("the workflow must declare a jobs mapping") + match jobs.get("coverage-upload"): + case dict() as job: + pass + case _: + pytest.fail("the workflow must declare a coverage-upload job") match job.get("steps"): case list() as steps: return steps case _: - raise AssertionError("jobs.build-test.steps must be a list") + pytest.fail("jobs.coverage-upload.steps must be a list") + + +def _windows_job(workflow: dict[str, object]) -> dict[str, object]: + """Return the build-test-windows job.""" + match workflow.get("jobs"): + case dict() as jobs: + pass + case _: + pytest.fail("the workflow must declare a jobs mapping") + match jobs.get("build-test-windows"): + case dict() as job: + return job + case _: + pytest.fail( + "the workflow must declare a build-test-windows job" + ) + + +def _windows_steps(workflow: dict[str, object]) -> list[dict[str, object]]: + """Return the build-test-windows job's steps.""" + match _windows_job(workflow).get("steps"): + case list() as steps: + return steps + case _: + pytest.fail("jobs.build-test-windows.steps must be a list") + + +def _windows_step(name: str) -> dict[str, object]: + """Return the uniquely named step from the build-test-windows job.""" + matches = [ + step for step in _windows_steps(_load()) if step.get("name") == name + ] + assert len(matches) == 1, ( + f"expected exactly one build-test-windows step named {name!r}, " + f"found {len(matches)}" + ) + return matches[0] def _step(name: str) -> dict[str, object]: @@ -127,7 +218,7 @@ def _test_shell_script() -> str: case str() as run: return run case _: - raise AssertionError(f"{TEST_SHELL_STEP} must declare a run script") + pytest.fail(f"{TEST_SHELL_STEP} must declare a run script") def test_test_shell_step_installs_gawk() -> None: @@ -197,3 +288,349 @@ def test_makefile_clippy_flags_stay_workspace_wide() -> None: f"every workspace crate, target, and feature with warnings denied; " f"got {match.group(1).strip()!r}" ) + + +def test_nextest_version_declared_once_at_workflow_scope() -> None: + """NEXTEST_VERSION is declared once, at workflow scope. + + AGENTS.md documents a local-install recipe that extracts the pin with: + + sed -n "s/.*NEXTEST_VERSION: '\\(.*\\)'.*/\\1/p" \ + .github/workflows/ci.yml + + A job-scoped duplicate would make that command emit two newline-separated + values, which `cargo install --version "$NEXTEST_VERSION"` rejects. The pin + therefore lives in the workflow-level `env:` block — the only declaration + in the file — and both jobs read it via `${{ env.NEXTEST_VERSION }}`. + """ + text = WORKFLOW_PATH.read_text(encoding="utf-8") + declarations = re.findall(r"^\s*NEXTEST_VERSION:\s*'([^']+)'", text, re.MULTILINE) + assert declarations == ["0.9.133"], ( + "NEXTEST_VERSION must be declared exactly once at workflow scope " + f"with the pinned value, got {declarations!r}" + ) + + workflow = _load() + match workflow.get("env"): + case dict() as env: + pass + case _: + pytest.fail( + "the workflow must declare a workflow-level env mapping" + ) + assert env.get("NEXTEST_VERSION") == "0.9.133", ( + "NEXTEST_VERSION must be pinned at workflow scope, " + f"got {env.get('NEXTEST_VERSION')!r}" + ) + + match workflow.get("jobs"): + case dict() as jobs: + pass + case _: + pytest.fail("the workflow must declare a jobs mapping") + for job_name in ("build-test", "build-test-windows"): + match jobs.get(job_name): + case dict() as job: + pass + case _: + pytest.fail(f"the workflow must declare a {job_name} job") + installs = [ + step.get("with", {}).get("tool") + for step in job.get("steps", []) + if "nextest" in str(step.get("with", {}).get("tool", "")) + ] + assert installs == ["nextest@${{ env.NEXTEST_VERSION }}"], ( + f"{job_name} must install nextest via the workflow-scoped " + f"${{{{ env.NEXTEST_VERSION }}}}, got {installs!r}" + ) + + +def test_windows_job_runs_on_windows_latest() -> None: + """The Windows job must actually run on a Windows runner.""" + job = _windows_job(_load()) + assert job.get("runs-on") == "windows-latest", ( + "build-test-windows must run on windows-latest so the " + f"#[cfg(windows)] tree is compiled, got {job.get('runs-on')!r}" + ) + + +def test_windows_job_uses_git_bash_for_recipes() -> None: + """The job runs recipes under Git Bash, not cmd.exe. + + The Makefile uses POSIX shell constructs throughout, and GNU Make's + default recipe shell on Windows is cmd.exe, so the job must default every + run step to bash. + """ + job = _windows_job(_load()) + match job.get("defaults"): + case dict() as defaults: + pass + case _: + pytest.fail( + "build-test-windows must declare a defaults mapping" + ) + match defaults.get("run"): + case dict() as run: + pass + case _: + pytest.fail( + "build-test-windows must declare a defaults.run mapping" + ) + assert run.get("shell") == "bash", ( + "build-test-windows must run recipes under Git Bash " + f"(defaults.run.shell: bash), got {run.get('shell')!r}" + ) + + +def test_windows_setup_rust_keeps_warnings_and_polonius() -> None: + """The Windows toolchain setup preserves -D warnings and -Zpolonius=next. + + The `#[cfg(windows)]` tree must be compiled under `-D warnings` to surface + findings, and the tree requires the Polonius analysis, so the shared + setup-rust action must receive both flags through its `rustflags` input. + """ + step = _windows_step("Setup Rust") + assert "setup-rust" in step.get("uses", ""), ( + f"Setup Rust must use the shared setup-rust action, got {step.get('uses')!r}" + ) + match step.get("with"): + case dict() as with_: + pass + case _: + pytest.fail("Setup Rust must declare a with mapping") + assert with_.get("toolchain") == "${{ env.NETSUKE_RUST_TOOLCHAIN }}", ( + "Setup Rust must use the pinned NETSUKE_RUST_TOOLCHAIN, " + f"got {with_.get('toolchain')!r}" + ) + assert with_.get("rustflags") == "-D warnings -Zpolonius=next", ( + "Setup Rust must pass -D warnings -Zpolonius=next through rustflags " + f"so the #[cfg(windows)] tree compiles under warnings-as-errors, " + f"got {with_.get('rustflags')!r}" + ) + + +def test_setup_rust_does_not_pass_unsupported_components_input() -> None: + """No setup-rust invocation passes the unsupported `components` input. + + The shared `setup-rust` action installs `rustfmt` and `clippy` internally + through `actions-rust-lang/setup-rust-toolchain`; its declared inputs do not + include `components`, so passing one emits an "Unexpected input(s) + 'components'" warning on every run. The contract is that every `Setup Rust` + step uses the shared action and passes only supported inputs, so `check-fmt` + and `lint-clippy` still find the components the action installs. + """ + workflow = _load() + for job_name in ("build-test", "build-test-windows"): + match workflow.get("jobs"): + case dict() as jobs: + pass + case _: + pytest.fail("the workflow must declare a jobs mapping") + match jobs.get(job_name): + case dict() as job: + pass + case _: + pytest.fail(f"the workflow must declare a {job_name} job") + setup_steps = [ + step + for step in job.get("steps", []) + if "setup-rust" in str(step.get("uses", "")) + ] + assert setup_steps, f"{job_name} must use the shared setup-rust action" + for step in setup_steps: + match step.get("with"): + case dict() as with_: + pass + case _: + pytest.fail( + f"{job_name} Setup Rust must declare a with mapping" + ) + assert "components" not in with_, ( + f"{job_name} Setup Rust must not pass the unsupported " + f"'components' input, got {sorted(with_.keys())!r}" + ) + + +def test_windows_job_runs_check_fmt_lint_and_test() -> None: + """The Windows job runs check-fmt, lint, and test as merge gates. + + Every quality gate must run through the Makefile with `SHELL=bash` so the + POSIX-shell recipes execute under Git Bash on the Windows runner. + """ + runs = [step.get("run") for step in _windows_steps(_load())] + expected = [ + "make SHELL=bash check-fmt", + "make SHELL=bash lint-clippy", + "make SHELL=bash lint-whitaker", + "make SHELL=bash test", + ] + for command in expected: + assert runs.count(command) == 1, ( + f"build-test-windows must run {command!r} exactly once, " + f"got run steps: {runs!r}" + ) + + +def test_windows_job_does_not_duplicate_doc_and_audit_gates() -> None: + """The Windows job excludes platform-independent doc and audit gates. + + `make spelling`, `make markdownlint`, `make nixie`, coverage generation, + the CodeScene gate, and `make test-workflow-contracts` are already covered + on Linux; duplicating them on Windows buys nothing. + """ + runs = [step.get("run") for step in _windows_steps(_load())] + excluded = [ + "make spelling", + "make markdownlint", + "make nixie", + "make test-workflow-contracts", + ] + for command in excluded: + assert command not in runs, ( + f"build-test-windows must not run the platform-independent " + f"{command!r}, got run steps: {runs!r}" + ) + + action_steps = [ + str(step.get("uses", "")) for step in _windows_steps(_load()) + ] + excluded_actions = ( + "leynos/shared-actions/.github/actions/generate-coverage", + "leynos/shared-actions/.github/actions/upload-codescene-coverage", + ) + for action in excluded_actions: + assert all(action not in step for step in action_steps), ( + f"build-test-windows must not use the Linux-only audit action " + f"{action!r}, got action steps: {action_steps!r}" + ) + + +def test_windows_job_is_a_blocking_merge_gate() -> None: + """No step in the Windows job is allowed to fail silently. + + A `continue-on-error: true` on the job or any step would let a Windows + lint or test failure pass the merge, defeating the gate. + """ + job = _windows_job(_load()) + assert job.get("continue-on-error") is not True, ( + "build-test-windows must not set continue-on-error on the job" + ) + for step in _windows_steps(_load()): + assert step.get("continue-on-error") is not True, ( + f"build-test-windows step {step.get('name')!r} must not set " + "continue-on-error" + ) + + +def test_coverage_report_is_produced_before_codescene_check() -> None: + """The CodeScene gate consumes the report the coverage step produces. + + `generate-coverage` writes the report to `output-path` (lcov.info) and the + `upload-codescene-coverage` check step reads that exact path in lcov + format. If the report path, format, or step ordering drifts, CodeScene + reports "No valid coverage report found in the build pipeline". This pins + the wiring: the coverage step runs after `make test` and the CodeScene + check runs after the coverage step, both with `format: lcov`. + """ + workflow = _load() + steps = _steps(workflow) + names = [step.get("name") for step in steps] + + coverage_index = names.index("Test and Measure Coverage") + codescene_index = names.index("Check coverage against CodeScene gates") + test_index = names.index("Test") + assert test_index < coverage_index, ( + "coverage must be measured after the test run so the report reflects " + "the tested tree" + ) + assert coverage_index < codescene_index, ( + "the CodeScene check must run after the coverage step so the report " + "exists in the build pipeline" + ) + + coverage_step = steps[coverage_index] + match coverage_step.get("with"): + case dict() as with_: + pass + case _: + pytest.fail( + "Test and Measure Coverage must declare a with mapping" + ) + assert with_.get("output-path") == "lcov.info", ( + "coverage must be written to lcov.info, " + f"got {with_.get('output-path')!r}" + ) + assert with_.get("format") == "lcov", ( + "coverage must be measured in lcov format, " + f"got {with_.get('format')!r}" + ) + + codescene_step = steps[codescene_index] + match codescene_step.get("with"): + case dict() as with_: + pass + case _: + pytest.fail( + "Check coverage against CodeScene gates must declare a with " + "mapping" + ) + assert with_.get("format") == "lcov", ( + "the CodeScene check must consume lcov format, " + f"got {with_.get('format')!r}" + ) + assert with_.get("path") == "lcov.info", ( + "the CodeScene check must read the report generated at lcov.info, " + f"got {with_.get('path')!r}" + ) + assert with_.get("mode") == "check", ( + "the CodeScene check must run in check mode, " + f"got {with_.get('mode')!r}" + ) + + +def test_main_coverage_upload_reads_the_generated_lcov_report() -> None: + """Main uploads the LCOV report it produces before calling CodeScene.""" + steps = _coverage_upload_steps(_load(COVERAGE_MAIN_WORKFLOW_PATH)) + names = [step.get("name") for step in steps] + coverage_index = names.index("Test and Measure Coverage") + upload_index = names.index("Upload coverage data to CodeScene") + assert coverage_index < upload_index, ( + "main must generate coverage before uploading it to CodeScene" + ) + + coverage_step = steps[coverage_index] + upload_step = steps[upload_index] + assert str(coverage_step.get("uses", "")).startswith( + "leynos/shared-actions/.github/actions/generate-coverage@" + ), "main coverage production must use generate-coverage" + assert str(upload_step.get("uses", "")).startswith( + "leynos/shared-actions/.github/actions/upload-codescene-coverage@" + ), "main coverage upload must use upload-codescene-coverage" + + match coverage_step.get("with"): + case dict() as coverage_with: + pass + case _: + pytest.fail("main coverage production must declare a with mapping") + assert coverage_with.get("output-path") == "lcov.info", ( + "main coverage must write lcov.info, " + f"got {coverage_with.get('output-path')!r}" + ) + assert coverage_with.get("format") == "lcov", ( + "main coverage must use lcov format, " + f"got {coverage_with.get('format')!r}" + ) + + match upload_step.get("with"): + case dict() as upload_with: + pass + case _: + pytest.fail("main CodeScene upload must declare a with mapping") + assert upload_with.get("path") == "lcov.info", ( + "main CodeScene upload must read lcov.info, " + f"got {upload_with.get('path')!r}" + ) + assert upload_with.get("format") == "lcov", ( + "main CodeScene upload must consume lcov format, " + f"got {upload_with.get('format')!r}" + )