Repository navigation
Conversation
|
Important Review skippedToo many files! This PR contains 428 files, which is 128 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (428)
You can disable this status message by setting the Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
Reviewer's GuideThis PR hardens lint, formatting, and documentation gates across the entire Whitaker workspace by centralizing lint configuration at the workspace level, enforcing stricter Clippy and Rust lints (including forbidding unsafe code), adding a shared rustfmt configuration, extending test tooling to run doctests with warnings denied, and ensuring the toolchain includes rust-analyzer. Sequence diagram for hardened test and documentation lint gatesequenceDiagram
actor Developer
participant Makefile
participant Cargo
Developer->>Makefile: make test
Makefile->>Cargo: test (RUSTFLAGS="-C prefer-dynamic ...")
Makefile->>Cargo: test --workspace --doc --all-features (RUSTFLAGS="$(RUST_FLAGS)")
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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. common/tests/decomposition_adjacency_behaviour.rs Comment on file .neighbours_of(node)
.ok_or_else(|| format!("node {node} is out of bounds"))?;
let is_sorted = neighbours.windows(2).all(|pair| pair[0].0 <= pair[1].0);
let is_sorted = neighbours.is_sorted_by_key(|neighbour| neighbour.0);❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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. Prefer assertion macros with pure queries and fallible commands if needed. 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. Comment on lines +187 to +190 assert!(!args.lint_selection.individual_lints);
assert!(!args.lint_selection.experimental);
assert!(!args.execution.cranelift);
assert!(!args.skip.skip_deps);❌ New issue: Large Assertion Blocks |
|
@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. installer/tests/behaviour_binstall.rs Comment on lines +107 to +115 fn then_pkg_url_present(world: &mut BinstallWorld) -> Result<(), String> {
let binstall = binstall_table(world)?;
let pkg_url = table_str(binstall, "pkg-url")?;
if pkg_url != PKG_URL_TEMPLATE {
return Err(format!(
"pkg-url mismatch: expected {PKG_URL_TEMPLATE}, got {pkg_url}"
));
}
Ok(())❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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 in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationinstaller/tests/behaviour_workflows.rs: What lead to degradation?The module contains 6 functions with similar structure: then_cli_exits_successfully,then_installation_succeeds_or_is_skipped,then_no_update_is_true,then_output_includes_library_path_instructions and 2 more functions Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@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 in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationinstaller/tests/toolchain_steps/mod.rs: What lead to degradation?The module contains 4 functions with similar structure: then_error_includes_toolchain_name,then_error_mentions_install_failure,then_install_message_shown,then_no_install_message Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@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 in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationinstaller/tests/behaviour_cli/support_assertions.rs: What lead to degradation?The module contains 2 functions with similar structure: assert_experimental_lint_opt_in_message_is_shown,assert_unknown_lint_message_is_shown Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
2c5cd42 to
9aa9e45
Compare
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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 in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Overall Code Complexitycrates/module_must_have_inner_docs/src/driver/inner_attr.rs: What lead to degradation?This module has a mean cyclomatic complexity of 4.14 across 7 functions. The mean complexity threshold is 4 Why does this problem occur?Overall Code Complexity is measured by the mean cyclomatic complexity across all functions in the file. The lower the number, the better. How to fix it?You address the overall cyclomatic complexity by a) modularizing the code, and b) abstract away the complexity. Let's look at some examples: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b758d198b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai assertions should not be used in given or when steps that return a result. The assertions should be performed in the then step. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
Promote the root package's clippy policy to `[workspace.lints]` so every member crate — including the rustc proxy shims and support crates — is held to the same standard, and extend it: - add `disallowed_methods` and `missing_assert_message` to the clippy set; - forbid `unsafe_code` workspace-wide; - add `[lints] workspace = true` to every member crate, removing the now-redundant local `expect_used`/`unwrap_used` and `unexpected_cfgs` tables; - introduce `.rustfmt.toml` with the estate's unstable nightly formatting options; - run workspace doctests (`cargo test --workspace --doc --all-features`) from the `test` target with warnings denied; - request the `rust-analyzer` component in `rust-toolchain.toml`. The tree is brought into compliance by the preparatory stack beneath this branch; this commit records the policy itself.
Bring the tree toward compliance with the hardened `[workspace.lints]` policy, fixing findings at source rather than suppressing them: - apply the machine-applicable clippy fixes across the workspace (`const fn`, owned-string clones, `use_self`, import granularity, format-args interpolation); - restructure panic-prone indexing in the decomposition-advice, LCOM4, clone-detection, and signal modules with iterator and `get`-based rewrites that preserve behaviour; - replace the unsafe `EnvVarGuard`/`LocaleOverride` environment guards with scoped `temp_env`-backed helpers (`with_env_var`, `with_env_var_removed`, `with_locale`) and rework the UI runner and all call sites onto them, satisfying `unsafe_code = "forbid"`; - declare the suite's Dylint entry point through a new `declare_dylint_register_entry!` macro in whitaker-common so the `#[unsafe(no_mangle)]` attribute expands from an external macro; - replace the installer's `libc::geteuid()` root probe with the safe `rustix::process::geteuid()` and drop the now-unused libc dependency; - document struct fields, `# Errors`, and `# Panics` contracts; rename shadowed bindings; add assertion messages; make non-test helpers fallible instead of panicking; - scope the one sanctioned `#[expect(clippy::float_arithmetic)]` to the bumpy-road signal functions whose weights are user-configured floats; - split `lint` into `lint-clippy` and `lint-whitaker` so the Whitaker suite lints the support crates in the gate and CI installs the pinned whitaker-installer; - refresh the spelling exemptions for moved SARIF identifier lines and correct two non-oxendict spellings.
Run `cargo fmt --all` under the new `.rustfmt.toml` (imports granularity, single-line functions, wrapped comments, formatted strings and doc examples). No behavioural changes.
Rename the `-ise`/`-isation` identifiers, call sites, documentation and Gherkin step text to the Oxford `-ize`/`-ization` forms the shared spelling policy requires, rather than exempting each line: - `normalise_*` -> `normalize_*`, `summarise_context*` -> `summarize_context*`, `localised_messages` -> `localized_messages`, and the matching test, fixture and step-function names; - `capitalise_segment`, `pluralisation`, `deserialises_*`, `recognises_*`, `finalised`, `parametrised` and `synthesised` likewise; - SARIF `Artifact`/`ArtifactLocation` -> `Artefact`/`ArtefactLocation`. The `artifactLocation` and `artifacts` property names are fixed by the OASIS SARIF 2.1.0 schema, so the fields pin their wire names with `serde(rename)` and the two literals remain exempt as serialized values; - the en-GB diagnostic note in `no_expect_outside_tests` now reads "recognized", with the byte-exact `.stderr` fixtures updated in step so the localized message and its fallback agree. Delete the unused deprecated `rasterise_signal` alias instead of renaming it; the workspace treats these as internal APIs, so no compatibility shim is needed. Prune the 205 now-dead `typos.local.toml` ignore patterns left behind. The 50 that remain cover genuinely external names only: Gaelic and Welsh locale prose, `cargo_metadata::CompilerArtifact`, GitHub Actions artifact actions, `act`'s `--artifact-server-path`, dylint UI fixture sources, and the two SARIF wire-format properties. Also repair the `#[path]` test declarations in `module_must_have_inner_docs`, which resolved relative to `src/driver/` after that module moved to `driver/mod.rs`, and adopt the shared `allow_fixture_expansion_lints` attribute for single-line rstest fixtures across the workspace.
Satisfy `self_named_module_files` for `install_flow`, `tests`, `toolchain`, and `toolchain::tests::failure_mocks`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename shadowed bindings, borrow needlessly-owned parameters, replace slice indexing with slice patterns, drop the vestigial `Result` around prebuilt installation, and extract wrapper-script assertion helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace slice indexing with fallible lookups and slice patterns, rename shadowed bindings, name wildcard match variants explicitly, elide a redundant lifetime, inline format arguments, compare archive extensions case-insensitively, and reshape the resolve-crates test case struct to drop its bool cluster. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bring the lint crates, the root crate, and the installer's directory abstraction into compliance with the hardened policy: - isolate `dylint_linting::impl_late_lint!` in a `mod declaration` per lint crate and re-export only the documented lint surface, so the macro's undocumentable ABI entry point and `get_lints` accessor carry one fulfilled, reasoned `#![expect(missing_docs)]` instead of the policy being disabled crate-wide; - isolate `newt_hype::base_newtype!` likewise for the explicit `Clone` impl it emits on a `Copy` type; - rename the `BaseDirs` trait methods to `home`/`executables`/ `whitaker_data` so the mockall-generated struct no longer shares a uniform field postfix, fixing `struct_field_names` at source; - replace the two `float_cmp` assertions with `to_bits()` comparisons, which is what "the configured value round-tripped unchanged" actually asserts; - add the missing assertion messages in the root crate's harness tests. Scope the Clippy gate to `--lib --bins --tests --benches` rather than `--all-targets`. Every file under a lint crate's `examples/` directory is a `fail_*`/`pass_*` Dylint UI fixture that deliberately contains the anti-patterns the suite detects; a fixture proving `expect_used` fires cannot itself forbid `expect_used`. They live in `examples/` rather than `ui/` because `dylint_testing::Test::example` builds them with each crate's dev-dependencies. `typecheck` still compiles them and the UI harness still lints them.
Three test failures traced to the new formatting profile rather than to any behavioural change: - `cargo fmt --all` formats `examples/` because those files are Cargo targets, and `format_strings` split a long `reason = "..."` across lines. That shifted the diagnostics one line down, so the byte-exact `.stderr` expectations no longer matched. Fixtures under `ui/` were never affected because they are not targets. Add a `.rustfmt.toml` `ignore` list so the `examples/` fixtures are equally immune, and restore the reformatted files; they now differ from `main` only by the two deliberate spelling corrections. - `cargo package -p whitaker-common` failed because the new `whitaker_test_macros` dev-dependency carried a version requirement while the crate is `publish = false`. Make the workspace entry path-only: Cargo strips versionless dev-dependencies when packaging. Also clear the two findings the Whitaker suite raised against its own sources, now that `make lint` runs it: - move the `#[must_use]` attributes in `i18n/diagnostics.rs` back after their doc comments, where a mechanical `clippy --fix` pass had wedged them between the summary line and the rest of the comment (`function_attrs_follow_docs`); - recover the poisoned lock in `env_test_guard` with `PoisonError::into_inner` instead of panicking (`no_unwrap_or_else_panic`). The mutex guards `()`, so a poisoned lock carries no corrupted state -- only the record that an earlier test panicked while holding the serialization token.
`format_strings = true` broke a long literal across lines *inside* an escape sequence: the backslash of `\n` became the end-of-line continuation, leaving the `n` as literal text on the next line. The assertion in the example-harness UI test was then searching for `fixture-localnfingerprint=...` and failing. This is content corruption rather than reformatting, and it is silent -- the code still compiles and only a test comparing exact output caught it. Rewrite the literal with `concat!()`, which `AGENTS.md` already prescribes over backslash-escaped newlines and which the formatter cannot split mid-escape.
`make lint` now runs the Whitaker suite over the workspace's own support crates, and this clears what it found there: - `module_must_have_inner_docs`: give every `mod tests`/`mod unix` an explanatory `//!` opening line; - `no_expect_outside_tests`: fixtures and helpers are not tests, so make them fallible and let the `#[test]`/`#[rstest]` body do the unwrapping. Two helpers became macros instead -- `source_span!` because it is used inside `#[case(...)]` where a `Result` cannot be propagated, and `utf8_path!` so the fallible conversion inlines into each test body; - `no_unwrap_or_else_panic`: propagate errors rather than panicking in the closure, converting the enclosing rstest-bdd steps to return `Result<(), String>`; - `no_std_fs_operations`: add the `i18n_packaging` integration-test crate to `dylint.toml`. That test shells out to `cargo package` and walks `$CARGO_TARGET_DIR`, so it is ambient by construction, and in-source suppression provably does not work for this lint. Note that integration-test targets compile as their own crate named after the file, so the existing `whitaker_common` entry did not cover it; - `module_max_lines`: extract `workspace.rs`'s tests to `workspace_tests.rs` (440 lines to 209 + 231); - `bumpy_road_function`: extract `attempt_repository_install`, then group its six parameters into a documented `RepositoryInstallRequest` so the extraction does not simply trade one denied lint for `too_many_arguments`. Also rename two `let entry = entry?` rebinds and hoist an import above the statements that the fixture conversions introduced ahead of it.
`make lint` now passes end to end: rustdoc, Clippy, and the Whitaker suite over the workspace's own support crates. - `no_expect_outside_tests`: convert the installer's fixtures and test helpers to return `Result`, with the `#[test]`/`#[rstest]` body doing the unwrapping. rstest fixtures are consumed as `#[from(fixture)] fixture_res: Result<..>` so the conversion does not trade the lint for `shadow_reuse` on a `let x = x?` rebind. - `no_unwrap_or_else_panic`: propagate rather than panic in the closure. - `module_max_lines`: split `list`, `downloader`, `workspace`, `dependency_binary_helpers` and `deps::tests` into sibling modules, each keeping an explanatory `//!` opening line. - `bumpy_road_function`: extract the complexity clusters, grouping parameters into named structs where an extraction would otherwise exceed the four-argument budget. - `no_std_fs_operations`: add the integration-test crates that stage fixtures ambiently to `dylint.toml`. In-source suppression does not work for this lint, and these targets compile as their own crate named after the file, so they are not covered by the package-level entries.
`make test` now runs the workspace doctests, which surfaced two things. Doctests compile as their own crate and do not inherit the lib's `#![cfg_attr(feature = "dylint-driver", feature(rustc_private))]`, so under `--all-features` every Dylint driver crate fails to link `rustc_driver`. Their examples are exercised by the unit and UI suites instead, so exclude them, mirroring the existing `nextest` exclusions. Also repair two doc examples in `installer/src/dirs.rs` left stale by the `BaseDirs` method rename. `leta rename` updates code references but not doc comments, and `cargo doc` only validates intra-doc links, so nothing caught these until the examples were actually compiled -- which is the point of adding the gate.
`then_node_has_no_neighbours` and `then_neighbours_of_node_are_sorted` each repeated the same `neighbours_of` lookup and out-of-bounds conversion before their differing assertion, which CodeScene flagged as duplication. Add `with_neighbours_of_node` alongside `with_report` and layer it on top, so the successful-build handling stays in one place and each step is left with only its own assertion. Behaviour is unchanged: the step strings, the bounds-error text, the assertion messages, and the `is_sorted_by_key` check are all preserved verbatim.
CodeScene flagged four runs of adjacent bare assertions in the CLI default tests as large assertion blocks. Each was also silent about which field had drifted: a failure reported only the line number. Add `assert_default_conditions`, which takes `(name, condition)` pairs and names the offending field in the panic message, and express each group as a table of named pure boolean queries. The checks themselves are unchanged, including the `verbosity == 0` comparison, which becomes a boolean rather than an `assert_eq!` so it fits the same table. No `PartialEq`/`Eq` derives were added and the production CLI types are untouched; the tests still read each field individually.
`then_pkg_url_present`, `then_bin_dir_present` and `then_default_pkg_fmt` each repeated the same fetch-key-and-compare shape, which CodeScene flagged as duplication. Add `assert_binstall_value_equals` beside `table_str` and reduce each step to a single call. The helper reports a mismatch by returning `Err` rather than asserting. These steps return `Result`, so an `assert_eq!` here trips `clippy::panic_in_result_fn`, and returning the message matches how every other step in the file reports failure. The message text is unchanged, so existing failures read exactly as before. `then_windows_override_pkg_fmt` is left alone: it reads through the `overrides` table and carries its own error strings, so it does not fit this helper.
Extract shared output assertions so related BDD steps retain their existing conditions while avoiding duplicated control flow.
Return diagnostic failures from the fallible integration test so the test keeps its detailed failure output without panicking.
Build the documented filename from the public platform prefix and extension helpers so the example works on Windows, macOS, and Linux.
Resolve the target branch's lockfile conflict from the manifest and preserve the current compatible dependency graph.
9b758d1 to
afb1e4a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Superseded by a stack of eight PRs
This branch carried the whole lint-hardening effort as a single 429-file change, which is not reviewable. It has been decomposed into a stack of focused, individually green PRs, each based on the one below it:
mainThe fixes land before the configuration that demands them, so every PR in the stack passes its own gates rather than only the final tip. The two mechanical changes — the rustfmt reformat and the spelling renames — are isolated so they do not bury the substantive diffs.
The stack is not merely a re-slicing of this branch: it also carries a seven-category test-quality sweep that this branch lacked, and turning the gates on in their own PR surfaced four findings that were invisible here, because several crates were shielded by local lint tables or were inheriting from a workspace that had no clippy table at all.
Closing in favour of the stack.
References