Skip to content

Adopt the Whitaker Dylint suite in the lint gate and CI - #410

Merged
leynos merged 26 commits into
mainfrom
adopt-whitaker
Jul 9, 2026
Merged

Adopt the Whitaker Dylint suite in the lint gate and CI#410
leynos merged 26 commits into
mainfrom
adopt-whitaker

Conversation

@leynos

@leynos leynos commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

This branch adopts the Whitaker Dylint suite as part of the estate-wide lint rollout, following the pattern established in leynos/wireframe. The suite now runs clean over --all-targets --all-features, is enforced by make lint, and runs in CI on the stable matrix leg. Roughly 240 findings across four waves (build script → library → library tests → integration test crates) were fixed rather than suppressed, with two documented crate-level exclusions for deliberately ambient filesystem access.

The adoption also surfaced a Whitaker defect — no_std_fs_operations ignores in-source allow/expect attributes — filed as leynos/whitaker#270. Crate-granularity exclusion via dylint.toml is therefore the only working escape hatch, which shaped the design below.

Review walkthrough

  • Start with dylint.toml for the lint policy: each no_std_fs_operations exclusion carries its rationale (build script, the two boundary crates, and the fixture-staging integration test crates).
  • Then review ambient_fs/src/lib.rs, a new internal leaf crate confining the probes the which resolver genuinely cannot express through cap_std handles (a capability handle refuses to follow symlinks that leave its directory, which would break layouts such as /usr/bin/cc -> /etc/alternatives/cc). Its scope and reuse policy are documented in the crate docs. test_support/src/fs.rs is the analogous home for ambient test-fixture I/O, mirroring Whitaker's own whitaker_common exclusion.
  • src/manifest/mod.rs shows the main behavioural change in production code: open_manifest_workspace opens the workspace's cap_std handle first and reads the manifest through it, instead of an ambient fs::read_to_string.
  • src/stdlib/config.rs replaces the panicking Default for StdlibConfig with a fallible StdlibConfig::from_current_dir(); src/stdlib/command/config.rs switches the pipe limit tracker to saturating arithmetic so overflow trips the limit check rather than panicking.
  • src/stdlib/network/mod.rs and src/stdlib/network/cache.rs show the bumpy_road_function refactors and the resulting module split; src/ir/cycle.rs shows the largest module_max_lines split (682 → 377 lines, helpers in src/ir/cycle_support.rs).
  • The bulk of the diff is mechanical test-code compliance: fixtures return Result and tests consume them with ? (assertions become anyhow::ensure! to satisfy clippy::panic_in_result_fn), fixture I/O routes through test_support::fs, and inline test modules gain //! docs.
  • Finish with Makefile and .github/workflows/ci.yml for the gate wiring: make lint runs rustdoc, Clippy, and Whitaker; CI installs whitaker-installer 0.2.5 via a cached cargo binstall on the stable leg only, while the MSRV and nightly legs run the new make lint-clippy subset (Whitaker lints under its own pinned toolchain, so repeating it per leg duplicates identical work). Documentation updates live in docs/developers-guide.md, the vendored docs/whitaker-users-guide.md, and AGENTS.md.

Validation

  • make check-fmt: pass
  • make lint (rustdoc + Clippy + whitaker --all -- --all-targets --all-features, warnings denied): pass, zero Whitaker findings
  • make typecheck: pass
  • make test: pass (all targets, all features)
  • make test-workflow-contracts: 6 passed
  • make markdownlint: 70 files, 0 errors
  • make nixie: all Mermaid diagrams valid
  • mbake validate Makefile: valid

Notes

  • In-source suppression (cfg_attr(dylint_lib = …, expect(…))) is non-functional in Whitaker v0.2.5 (no_std_fs_operations ignores in-source allow/expect attributes whitaker#270); the cfg(dylint_lib, values(any())) check-cfg allowlist entry is retained so attribute-based suppression can replace crate exclusions once the lint honours it.
  • New integration test crates that touch std::fs will fail the lint until deliberately added to dylint.toml, making that policy decision visible in review.
  • Adding the ambient_fs crate required a matching .github/dependabot.yml entry (enforced by dependabot_config_tests).

References

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3896ee51-9225-4467-a391-c4099073b299

📥 Commits

Reviewing files that changed from the base of the PR and between 12a44a2 and 4d951a0.

📒 Files selected for processing (15)
  • ambient_fs/src/lib.rs
  • docs/developers-guide.md
  • locales/en-US/messages.ftl
  • locales/es-ES/messages.ftl
  • src/graph_view/tests_property.rs
  • src/localization/keys.rs
  • src/manifest/mod.rs
  • src/manifest/tests/workspace.rs
  • src/stdlib/network/mod.rs
  • src/stdlib/network/tests_support.rs
  • src/stdlib/time/mod.rs
  • src/stdlib/which/lookup/tests.rs
  • test_support/src/lib.rs
  • tests/bdd/steps/manifest_command_helpers.rs
  • tests/cli_tests/config_discovery_scopes.rs
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • leynos/rstest-bdd (auto-detected)
  • leynos/ortho-config (auto-detected)
  • leynos/shared-actions (auto-detected) → reviewed against open PR #329 adopt-whitaker instead of the default branch

Adopted the Whitaker Dylint lint suite across the project’s linting and CI workflow: make lint now runs rustdoc-with-warnings-denied, Clippy for the full targets/features matrix when Whitaker isn’t available, and otherwise runs the Whitaker installer with --all-targets --all-features (also pinned in stable CI). Updated contributor guidance and added a Whitaker users’ guide covering installation/configuration and lint catalogue.

Added an ambient_fs leaf crate plus test_support::fs helpers to allow intentional ambient filesystem access while keeping no_std_fs_operations compliance via a new dylint.toml crate exclusion set. Refactored stdlib which, network fetch/caching, command tempfile handling, and related tests to use the capability-scoped helpers instead of std::fs where appropriate.

Refactored StdlibConfig to construct from the current directory (from_current_dir) with new localised diagnostics for workspace-root/CWD resolution failures, plus updated tests/fixtures to use fallible Result-based setup. Further split and reorganised internals and test support (manifest ingestion via capability-scoped dirs, network cache module extraction, cycle-detection helpers, and shared test helpers), and converted multiple test suites to Result/anyhow assertions while tightening filesystem/cache assertions and adding targeted tracing.

Also included CI/documentation wiring and mechanical test cleanups (e.g., typed BDD step helpers, config-discovery test refactors, and duplicate-structure test consolidation) to support the lint gate running broadly.

Walkthrough

Add Whitaker linting and docs, introduce ambient_fs plus shared test filesystem helpers, move manifest/network/command filesystem access onto scoped handles, and split or harden several test modules and helpers.

Changes

Whitaker, filesystem, and test refactor changes

Layer / File(s) Summary
Whitaker lint flow and docs
.github/workflows/ci.yml, Makefile, AGENTS.md, Cargo.toml, dylint.toml, docs/contents.md, docs/developers-guide.md, docs/whitaker-users-guide.md, .github/dependabot.yml, locales/*, src/localization/keys.rs, tests/cli_tests/config_discovery.rs
Pin and cache the Whitaker installer in CI, split lint execution by toolchain, update lint documentation and policy, add the Whitaker guide and localisation keys, and extend Dependabot coverage for ambient_fs.
ambient_fs and shared fs helpers
ambient_fs/Cargo.toml, ambient_fs/src/lib.rs, test_support/src/fs.rs, test_support/src/lib.rs, src/stdlib/which/lookup/*, src/runner/process/file_io.rs, src/stdlib/which/cache.rs
Add ambient filesystem helpers and shared test filesystem wrappers, then switch executable probing, path canonicalisation, directory-entry checks, and selected runner/test fixture filesystem calls onto those helpers.
Scoped config, manifest, command, and network cache
src/stdlib/config.rs, src/stdlib/config_tests.rs, src/manifest/mod.rs, src/manifest/tests/workspace.rs, src/manifest/glob/tests.rs, src/manifest/tests/stages.rs, src/stdlib/command/*, src/stdlib/network/*, tests/std_filter_tests/*
Make stdlib configuration fallible, read manifests through capability-scoped directories, move command tempfile and network cache handling onto scoped helpers, and update the affected tests to use the new constructors and shared support modules.

Test suite hardening and module splits

Layer / File(s) Summary
Graph, IR, manifest, and status test splits
src/graph_view/*, src/ir/cycle.rs, src/ir/cycle_support.rs, src/ir/cycle_tests.rs, src/diagnostic_json_tests.rs, src/manifest/expand_test_cases/*, src/manifest/expand_tests.rs, src/manifest/diagnostics/*, src/manifest/render.rs, src/runner/process/*, src/status_tests.rs, tests/workflow_shared_actions_pins.rs
Move graph and IR helpers/tests into separate modules, make several render and status tests Result-based, recover from poisoned tracing locks, and harden empty-input or missing-state helper paths.
BDD helper extraction and config discovery
tests/bdd/steps/cli*.rs, tests/bdd/steps/manifest_command*.rs, tests/bdd/types*.rs, tests/bdd/steps/configuration_preferences.rs, tests/cli_tests/config_discovery*.rs
Extract CLI and manifest BDD helpers into support modules, add typed Ninja BDD values, make enum-name conversion fallible, and split config-discovery coverage into scope and override modules.

Poem

A linting rabbit hopped in bright,
With Whitaker humming through the night. 🐇
Scoped dirs, safe caches, tests that ensure!,
Fewer panics, paths more sure.
New helpers bloom, the modules split —
tidy crates now neatly knit.

Possibly related PRs

  • leynos/netsuke#335: Refactors the same which executable-probing path that now delegates to ambient_fs::is_executable_file.
  • leynos/netsuke#392: Touches the same cycle canonicalisation code split into cycle_support.rs here.
  • leynos/wireframe#514: Updates the same Whitaker CI and Makefile lint wiring.

Suggested reviewers: codescene-delta-analysis, codescene-access


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 2 warnings)

Check name Status Explanation Resolution
Testing (Overall) ❌ Error sync_file is only checked by a same-process reopen/read test, so a no-op sync would still pass; cache_key_hashes_url is also length-only. Add a post-close or separate-process oracle for temp-file durability, and assert known SHA-256 vectors for cache-key/hash helpers instead of length-only checks.
Unit Architecture ❌ Error test_support::fs::exists hides filesystem fallibility by returning bool from metadata(...).is_ok(), so a read path still masks I/O and permission errors. Return io::Result<bool> (or drop the wrapper) so tests handle existence checks at the boundary instead of swallowing failures.
Observability ⚠️ Warning Fetch/cache and current-dir/tempfile changes add logs only; no bounded metrics or spans cover the new cache behaviour or startup failure boundaries. Add cache-hit/miss/error counters and trace spans or events for workspace open/resolution and tempfile creation/failure, with bounded labels.
Concurrency And State ⚠️ Warning The fetch cache mutates a shared on-disk entry via create+truncate/write with no lock or atomic rename, and tests only cover sequential success/failure. Serialise cache writes with a lock or temp-file+atomic rename protocol, and add a contention test that runs two fetches for the same key concurrently.
✅ Passed checks (16 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the PR’s main change: adopting Whitaker in lint gating and CI.
Description check ✅ Passed The description directly matches the changeset and clearly describes the Whitaker lint rollout and CI wiring.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
User-Facing Documentation ✅ Passed docs/whitaker-users-guide.md documents Whitaker install/use/configure, and docs/contents.md links it as the guide for the make lint suite.
Developer Documentation ✅ Passed PASS: The guide and design docs cover the new lint gate and architectural boundaries, and en-US/es-ES locale keys are in sync (331/331, no diff).
Module-Level Documentation ✅ Passed Heuristic scan over 269 Rust files found 0 modules lacking a top-level doc comment; the new leaf crates and refactored modules explain purpose and relationships clearly.
Testing (Unit And Behavioural) ✅ Passed PASS: The PR adds integration and regression tests across file, HTTP, CLI, workflow, and config boundaries, with edge/error cases and real I/O, not just private helper seams.
Testing (Property / Proof) ✅ Passed PASS: The PR adds proptest invariance checks and Kani proofs for cycle canonicalisation/detection, matching the requested testing strategy.
Testing (Compile-Time / Ui) ✅ Passed PASS: compile-time Kani cfg behaviour is covered by trybuild/rustc UI tests, and structured renderer output is snapshot-gated with insta.
Domain Architecture ✅ Passed Ambient filesystem, cache, and tempfile work stay in boundary crates/modules; manifest/which code uses explicit adapters and typed errors, not raw infra leakage.
Security And Privacy ✅ Passed PASS: The diff keeps secrets out, logs only host/hash rather than full URLs, and confines ambient filesystem access to explicit helper crates and validated paths.
Performance And Resource Use ✅ Passed PASS: New work stays bounded and linear (e.g. max_depth 6, cache reads capped at limit+1), with no unbounded collections or hot-path blowups.
Architectural Complexity And Maintainability ✅ Passed PASS: New abstractions are thin extractions with explicit scope/reuse docs, and the added crates/modules stay leaf-level; no speculative layers, registries, or hidden hooks.
Rust Compiler Lint Integrity ✅ Passed PASS: No active dead_code/unused suppressions or fake anchors were added; new helpers are referenced, and the only new clones are intentional snapshots/Arc clones.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch adopt-whitaker

Comment @coderabbitai help to get the list of available commands.

codescene-access[bot]

This comment was marked as outdated.

@leynos

leynos commented Jul 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

src/stdlib/network/tests.rs

Comment on file

        Arc,
        atomic::{AtomicBool, Ordering},
    },
use std::sync::{

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: fetch_rejects_disallowed_scheme,fetch_rejects_not_allowlisted_host

@leynos

leynos commented Jul 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

tests/cli_tests/config_discovery_scopes.rs

Comment on lines +252 to +295

fn project_config_takes_precedence_over_user_config() -> Result<()> {
    let _env_lock = EnvLock::acquire();
    let cwd_guard = CwdGuard::acquire().context("capture current working directory")?;

    let temp_project = tempdir().context("create temporary project directory")?;
    let temp_appdata = tempdir().context("create temporary APPDATA directory")?;

    // Create sandboxed Windows user-scope config at %APPDATA%\netsuke\config.toml
    let netsuke_config_dir = temp_appdata.path().join("netsuke");
    fs::create_dir_all(&netsuke_config_dir).context("create netsuke config directory")?;
    fs::write(
        netsuke_config_dir.join("config.toml"),
        PRECEDENCE_USER_CONFIG_CONTENT,
    )
    .context("write user config.toml in APPDATA")?;

    // Project config: overrides theme; does NOT set colour_policy.
    fs::write(
        temp_project.path().join(".netsuke.toml"),
        PRECEDENCE_PROJECT_CONFIG_CONTENT,
    )
    .context("write project .netsuke.toml")?;

    let _appdata_guard = EnvVarGuard::set("APPDATA", temp_appdata.path().as_os_str());
    let _localappdata_guard = EnvVarGuard::remove("LOCALAPPDATA");
    let _config_guard = EnvVarGuard::remove("NETSUKE_CONFIG");
    let _config_path_guard = EnvVarGuard::remove("NETSUKE_CONFIG_PATH");
    let _theme_guard = EnvVarGuard::remove("NETSUKE_THEME");
    let _jobs_guard = EnvVarGuard::remove("NETSUKE_JOBS");
    let _colour_guard = EnvVarGuard::remove("NETSUKE_COLOUR_POLICY");

    std::env::set_current_dir(&temp_project).context("change to project directory")?;

    let localizer = Arc::from(cli_localization::build_localizer(None));
    let (cli, matches) = netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer)
        .context("parse CLI for precedence test")?;
    let merged = netsuke::cli::merge_with_config(&cli, &matches)
        .context("merge configs")?
        .with_default_command();

    let result = assert_project_precedence_applied(&merged);
    drop(cwd_guard);
    result
}

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: project_config_takes_precedence_over_user_config,project_config_takes_precedence_over_user_config

@coderabbitai

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

codescene-access[bot]

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants