Skip to content

Enable strictest practical linting across the workspace - #15

Merged
rominf merged 7 commits into
mainfrom
strict-linting
Jun 18, 2026
Merged

Enable strictest practical linting across the workspace#15
rominf merged 7 commits into
mainfrom
strict-linting

Conversation

@rominf

@rominf rominf commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Tightens linting to the strictest practical level across every language in the repo, and wires prek + CI to enforce it.

  • Rust: enable Clippy all + pedantic + nursery (warn locally, denied in CI via -D warnings), inherited by every crate through lints.workspace = true. A small, documented allow-list in the root Cargo.toml turns off the high-noise / low-value / risky lints (e.g. assigning_clones, needless_pass_by_value, the numeric-cast family, doc/metadata lints) so the signal stays high. All remaining findings are fixed.
  • unsafe_code: changed from forbid to deny. It was previously declared in [workspace.lints.rust] but no crate opted in, so it was inert — and the workspace genuinely needs platform FFI (libc / Win32 / Cosmopolitan, std::env::set_var in edition 2024). It's now enforced everywhere, with narrow per-function #[allow(unsafe_code)] (each with a reason) at the real FFI sites.
  • Python: strict Ruff rule set (E,W,F,I,B,UP,SIM,C4,RUF) for the helper scripts and engine workers; all findings fixed.
  • Shell: ShellCheck --enable=all (excluding SC2310/SC2312, which need risky set -e restructuring of the setup scripts).
  • PowerShell: PSScriptAnalyzer with a settings file (Write-Host allowed — these are user-facing console installers); approved-verb renames and dead-code removal.
  • Enforcement: prek hooks and a new CI lint job run the same checks so local and CI results match.

Why

Linting was minimal and partly inert: no [workspace.lints.clippy] at all, lints.workspace never set, and CI only denied the default Clippy set. This raises what the existing hooks enforce, catching a much broader class of correctness and quality issues.

Non-obvious decisions

  • unreachable_pub is intentionally not enabled — this workspace is dominated by binary crates where every pub item is unreachable, so it would only add noise.
  • The allow-list is centralized in the root Cargo.toml with a one-line rationale per lint, so any entry can be re-enabled and burned down later.
  • A short rationale for the approach lives in plans/strict-linting-plan.md.

Risk

Low. No runtime behavior changes — tooling/quality-gate only. The diff is large but mostly mechanical (Clippy/Ruff autofixes).

Test plan

  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --all --check — clean
  • cargo test --workspace — passing (see note)
  • ruff check + ruff format --check on scripts/ + engines/ — clean
  • shellcheck --enable=all --exclude=SC2310,SC2312 on all scripts — clean
  • PSScriptAnalyzer with the bundled settings — clean

Note: tui::tests::assistant_comfyui_start_result_shows_url_models_and_quit_prompt is a pre-existing flake under the full parallel cargo test run (it fails on a clean main too); it passes in isolation and single-threaded. Unrelated to this change.

Turn on clippy all+pedantic+nursery (denied in CI via -D warnings),
wired into every crate with lints.workspace = true, plus a documented
allow-list for the high-noise/low-value or risky lints. unsafe_code is
denied (not forbid) with narrow per-function #[allow] at the platform
FFI sites. Fix all remaining findings.

Tighten the other languages to match: a strict ruff rule set for the
Python helpers, shellcheck --enable=all (minus the set -e suppression
checks), and PSScriptAnalyzer for the PowerShell scripts. Wire prek and
CI so local and CI enforce the same levels.
The rocm-dash merge added DashboardConfig/DashboardTuiConfig/DashboardDaemonConfig
without knowing about the stricter workspace lints introduced in this PR.

- Make three f64-returning default fns `const`
- Add `Eq` to `DashboardTuiConfig` (all fields are `Eq`)
- Add `#[must_use]` to the three builder-style `with_*` methods
- Allow `clippy::float_cmp` on two tests that compare exact constant defaults

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens and standardizes linting across the workspace (Rust/Clippy, Python/Ruff, Shell/ShellCheck, and PowerShell/PSScriptAnalyzer) and wires those checks into prek hooks and CI so local and CI enforcement align.

Changes:

  • Enable workspace-wide Rust lint inheritance ([lints] workspace = true) and configure strict-but-pragmatic workspace Clippy/Rust lints in the root Cargo.toml.
  • Expand Python linting via a stricter ruff.toml rule selection and apply compatible autofixes/refactors across scripts/workers.
  • Add/enhance ShellCheck and PSScriptAnalyzer enforcement via prek and a new CI lint job.

Reviewed changes

Copilot reviewed 51 out of 52 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Cargo.toml Defines strict workspace Rust/Clippy lint configuration and allow-list.
xtask/Cargo.toml Enables workspace lint inheritance for the xtask crate.
apps/rocm/Cargo.toml Enables workspace lint inheritance for the rocm app crate.
apps/rocm/src/main.rs Mechanical refactors to satisfy stricter Clippy lints (Option/Result helpers, formatting, matches).
apps/rocm/src/therock.rs Mechanical refactors and small formatting improvements for stricter Clippy.
apps/rocm/src/providers.rs Minor refactors, duration constant tweak, and formatting cleanups.
apps/rocm/src/provider_keys.rs Minor refactors (const fn) to satisfy stricter Clippy.
apps/rocm/src/comfyui.rs Refactors for stricter Clippy; some timeout constant rewrites.
apps/rocmd/Cargo.toml Enables workspace lint inheritance for the rocmd app crate.
apps/rocmd/src/lib.rs Refactors and small behavioral-preserving rewrites to satisfy stricter Clippy.
crates/rocm-core/Cargo.toml Enables workspace lint inheritance for rocm-core.
crates/rocm-core/src/lib.rs Adds targeted unsafe_code allows for real FFI sites and applies Clippy-driven refactors.
crates/rocm-core/src/runtime.rs Makes many helpers const fn, adds targeted unsafe_code allow annotations, and refactors Option handling.
crates/rocm-core/src/uv.rs Refactors for stricter Clippy and adds a targeted allow for a filename-extension lint.
crates/rocm-engine-protocol/Cargo.toml Enables workspace lint inheritance for rocm-engine-protocol.
crates/rocm-engine-protocol/src/lib.rs Small refactor to remove unnecessary clone in tests.
engines/atom/Cargo.toml Enables workspace lint inheritance for the atom engine crate.
engines/atom/src/lib.rs Mechanical refactors for stricter Clippy (Option helpers, const fn).
engines/lemonade/Cargo.toml Enables workspace lint inheritance for the lemonade engine crate.
engines/lemonade/src/lib.rs Refactors for stricter Clippy; minor control-flow simplifications and timeout literal rewrites.
engines/llama-cpp/Cargo.toml Enables workspace lint inheritance for the llama-cpp engine crate.
engines/llama-cpp/src/lib.rs Mechanical refactors for stricter Clippy (Option helpers, early-return patterns, const fn).
engines/pytorch/Cargo.toml Enables workspace lint inheritance for the pytorch engine crate.
engines/pytorch/src/lib.rs Mechanical refactors for stricter Clippy (const fn, Option helpers, formatting, minor restructuring).
engines/pytorch/src/python_worker.py Updates typing imports for modern Python conventions (collections.abc).
engines/sglang/Cargo.toml Enables workspace lint inheritance for the sglang engine crate.
engines/sglang/src/lib.rs Mechanical refactors for stricter Clippy (Option helpers, const fn).
engines/vllm/Cargo.toml Enables workspace lint inheritance for the vllm engine crate.
engines/vllm/src/lib.rs Mechanical refactors for stricter Clippy (Option helpers, const fn).
install.sh ShellCheck-driven quoting/brace tightening and safer variable expansions.
install.ps1 PSScriptAnalyzer-driven function renames and call-site updates.
.pre-commit-config.yaml Strengthens hooks: ShellCheck --enable=all and adds a PSScriptAnalyzer hook.
.github/workflows/ci.yml Adds a dedicated lint job running Ruff, ShellCheck, and PSScriptAnalyzer in CI.
ruff.toml Switches to a stricter, explicit Ruff rule selection and keeps E501 ignored.
PSScriptAnalyzerSettings.psd1 Adds repo-wide PSScriptAnalyzer configuration (including Write-Host exception).
plans/strict-linting-plan.md Documents the rationale, approach, and tradeoffs for strict linting across languages.
scripts/acceptance-install-upgrade-tui-uninstall.ps1 Renames helper function to satisfy PSScriptAnalyzer conventions.
scripts/atom_therock_gpu_test.py Removes now-unneeded lint suppression while retaining behavior.
scripts/build_single_exe_release.py Minor Python refactor (multi-context with) consistent with stricter linting.
scripts/comfyui_therock_gpu_test.py Python typing import cleanup and boolean-expression clarity tweak.
scripts/cosmopolitan_feasibility.py Python subprocess capture simplification (capture_output=True).
scripts/llama_cpp_therock_gpu_test.py Adds contextlib usage for cleaner exception suppression and removes lint suppression.
scripts/local_assistant_therock_gpu_test.py Uses contextlib.suppress and simplifies a nested condition.
scripts/pytorch_therock_gpu_test.py Removes broad-except suppression and improves exit exception chaining.
scripts/release_readiness.py Small Python cleanups (encoding default, dict.fromkeys).
scripts/rust_cosmopolitan_spike.py Python typing import cleanup, subprocess capture simplification, and safer string formatting.
scripts/setup-cosmocc.sh ShellCheck-driven quoting/brace tightening and safer variable expansions.
scripts/sglang_therock_gpu_test.py Removes now-unneeded lint suppression while retaining behavior.
scripts/single_exe_release_gate.py Uses contextlib.suppress and improves exception chaining on exit.
scripts/tui_e2e_smoke.py Python typing import cleanup and exception chaining on exit.
scripts/vllm_therock_gpu_test.py Removes now-unneeded lint suppression while retaining behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Supply-chain hardening per Copilot review: ludeeus/action-shellcheck@master
is a moving ref; pin to immutable commit SHA (v2.0.0). PSScriptAnalyzer
without -RequiredVersion can silently change rule behavior; pin to 1.25.0.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

crates/rocm-core/src/runtime.rs:23

  • RuntimePlatform::current (and the runtime_* helpers above it) is now const fn, but under cfg(target_vendor = "cosmo") it calls cosmo_hostos_has, which is a non-const function reading an extern static. This will fail to compile for Cosmopolitan targets. Consider either reverting these helpers to non-const or splitting into #[cfg(target_vendor = "cosmo")] non-const implementations and #[cfg(not(target_vendor = "cosmo"))] const implementations.
    pub const fn current() -> Self {
        #[cfg(target_vendor = "cosmo")]
        {
            if cosmo_hostos_has(COSMO_HOST_WINDOWS) {
                return Self::Windows;
            }
            if cosmo_hostos_has(COSMO_HOST_LINUX) {
                return Self::Linux;
            }
        }

Comment thread plans/strict-linting-plan.md Outdated
rominf added 2 commits June 17, 2026 11:00
Add `[lints] workspace = true` to rocm-dash-collectors, rocm-dash-core,
rocm-dash-daemon, and rocm-dash-tui so they inherit the same strict
clippy (all+pedantic+nursery) and deny-unsafe rules as the rest of the
workspace. Fix all resulting findings:

- `const fn` on small pure functions
- `#[must_use]` on builder-style `-> Self` methods
- Doc paragraph breaks (`too_long_first_doc_paragraph`)
- Numeric literal readability (`_` separators)
- `map(f).unwrap_or(a)` → `map_or(a, f)` on Option/Result
- `let…else` rewrites where clippy suggests it
- `implicit_hasher`: generalize `HashMap` params over the hasher
- `cast_possible_wrap`: annotate known-safe usize→isize casts in tests
- `float_cmp`: annotate exact-constant float comparisons in tests
- `struct_field_names` on VllmLogSlicer (`_re` suffix is meaningful)
- `future_not_send` on internal transport helpers
- Redundant `continue` removal

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 124 out of 125 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/rocm-core/src/runtime.rs:22

  • RuntimePlatform::current is now a const fn but (when target_vendor = "cosmo") it calls cosmo_hostos_has which reads an extern static (__hostos) via unsafe. This isn’t const-evaluable, so Cosmopolitan builds will fail to compile with “cannot call non-const fn in const fn” / “cannot access extern static in const”. Consider reverting these runtime-detection helpers (RuntimePlatform::current, RuntimeHost::current, and the runtime_* wrappers) back to non-const, or gating the const versions behind #[cfg(not(target_vendor = "cosmo"))].
    pub const fn current() -> Self {
        #[cfg(target_vendor = "cosmo")]
        {
            if cosmo_hostos_has(COSMO_HOST_WINDOWS) {
                return Self::Windows;
            }
            if cosmo_hostos_has(COSMO_HOST_LINUX) {
                return Self::Linux;
            }

@rominf
rominf enabled auto-merge June 17, 2026 11:34
@rominf
rominf requested review from r0x0r and volen-silo June 17, 2026 11:35
On Windows, SystemTime has ~100ns granularity so parallel tests that
both call tempdir() in the same window get the same path, causing
directory collision and data corruption between tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 124 out of 125 changed files in this pull request and generated no new comments.

@rominf
rominf added this pull request to the merge queue Jun 18, 2026
Merged via the queue into main with commit b38b7f8 Jun 18, 2026
8 checks passed
@rominf
rominf deleted the strict-linting branch June 22, 2026 11:41
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.

3 participants