Repository navigation
Conversation
Establish the base of the lint-hardening PR stack: the test, fixture, helper, and assertion improvements, separated from the lint-config, formatting, and rename changes stacked above so each review stays scoped. - Fixtures and setup helpers become fallible and propagate errors; the test body performs the unwrap with a descriptive expect. rstest fixtures are consumed as `#[from(fixture)] fixture_res: Result<..>` so bindings keep their names. - Assertion blocks are decomposed into named helpers, pure queries, and single-purpose assertion macros; duplicated step logic is extracted (binstall value comparison, adjacency neighbour lookup, CLI default condition tables). - Test modules gain explanatory `//!` opening lines; self-named test module files move to `mod.rs` form. - `test_support` gains scoped environment helpers backed by `temp_env` alongside the existing guard; a new `whitaker_test_macros` crate provides `allow_fixture_expansion_lints` for fixture expansion noise. Symbols follow main's current API; renames and signature changes land in their own PRs above this one.
Follow-up pass over the base sweep, driven by a seven-category audit: - whitaker_sarif: collapse ~18 near-identical serde match-panic round-trip blocks into shared test-support helpers (JSON round-trip and serialize-then-check), keeping match-panic internally so the helpers stay clean under a future no-expect-outside-tests gate. - config_loading behaviour steps: extract the repeated shared-config lookup into one fallible helper feeding five thin then-steps. - behaviour_cli support assertions: decompose the monolithic assertion functions into fallible lookups, pure queries, and thin wrappers, following the toolchain_steps exemplar. - whitaker_clones_core: split assert_region's fallible computation from its equality assertion, and convert ten ast tests whose Result returns were decorative (every `?` on always-valid literal constructors) into plain asserting tests with descriptive expects.
Rename code symbols and prose to Oxford spelling (-ize/-yze for the -ise pairs; analyse-family words correctly keep -yse): normalize, localize, summarize, recognized, serialize, and the SARIF domain terms Artifact -> Artefact across whitaker_sarif and its consumers. The SARIF 2.1.0 wire format spells its properties `artifactLocation` and `artifacts`, so those fields carry explicit `#[serde(rename)]` pins with doc comments explaining that the wire name is fixed by the schema rather than derived from the field name. External identifiers (`CompilerArtifact`, cargo metadata types, GitHub Actions flags) keep the external system's spelling. Feature files and step definitions are renamed together, and the typos configuration is pruned from 255 ignore patterns to the residual set the corrected tree still needs, so the spelling gate now enforces the convention. A handful of self-named module files move to mod.rs form here because the renames were entangled with those moves.
Move the remaining self-named module files to mod.rs form (install_flow, toolchain) and extract oversized in-src test modules into sibling files (list, workspace, downloader, the non-UTF-8 boundary tests, the dependency-binary fakes, and the deps repository tests), each wired with the repo's `#[cfg(test)] #[path]` convention and opening with an explanatory `//!` line. The moves are content-preserving: extracted test bodies are this tree's code verbatim; only the module structure changes. This keeps every module inside the 400-line budget the lint configuration later in this stack will enforce, without entangling the structural review with lint fixes.
Behaviour-preserving refactors across common/, whitaker_sarif, whitaker_clones_core, the lint crates, the suite, and the root crate, clearing the violations a stricter lint configuration later in this stack will enforce, while staying green under the current gates: - panic-prone operations give way to descriptive errors or checked alternatives (indexing and slicing via get/split_at_checked, no unwrap/expect outside tests, no panicking closures in Result contexts); - shadowed bindings are renamed meaningfully; wildcard matches name their single variants; string pushes and format chains use their cheaper forms; - the Dylint registration entry point moves behind a new declare_dylint_register_entry! macro in common, so consumer crates contain no unsafe tokens of their own; - the environment-variable test guards give way to temp_env-backed scoped helpers, and the last EnvVarGuard consumer is ported, so the guard itself retires; - missing documentation is filled in rather than waived. Installer changes are deliberately absent; they form the next PR in the stack.
Complete the support-crate wave with the second half of the donor's
fixes, keeping them out of the installer PR above.
Each lint crate's `impl_late_lint!` invocation moves into a private
`declaration` module carrying one reasoned expectation: the macro
expands to items with no documentable source location, so `missing_docs`
cannot be satisfied at the invocation site, and an `#[expect]` on the
macro call itself does not propagate into the expansion. The same
treatment isolates `base_newtype!`, which emits an explicit `Clone` on a
`Copy` type. Each crate then re-exports an explicit `{LINT, LintPass}`
surface rather than a glob.
The remainder are the usual behaviour-preserving fixes: parameters taken
by reference where they were needlessly moved, checked indexing and
slicing, total matches in place of `unreachable!`, meaningful names for
shadowed bindings, and assertion messages where they were missing.
Clear the installer's share of the violations a stricter lint configuration later in this stack will enforce, while staying green under the current gates. Several API changes land here because the fixes require them: - `BaseDirs` methods drop their redundant suffixes (`home`, `executables`, `whitaker_data`); the external `UserDirs::home_dir` call inside the implementation keeps its own name; - `InstallArgs` regroups its flags into `lint_selection`, `execution`, and `skip` flatten structs, so no single struct carries an excess of booleans; - install metrics report integer permille rather than a float rate, so assertions compare exactly instead of within a tolerance; - test helpers that previously panicked (`LocalServer::start`, the fake-binary writers, `load_cargo_toml`, `extract_binstall_table`) return `Result` and let callers propagate; - parameters that were needlessly moved are taken by reference; - the repository-release install path moves to its own module, with its executor, tool, and dependency grouped into a `RepositoryInstallRequest` so the extraction stays within the argument budget; - the root check swaps `libc::geteuid` for `rustix`, removing the last `unsafe` block in the installer and the `libc` dependency with it. A `#[expect(clippy::cast_sign_loss)]` is removed rather than retained: the timestamp arithmetic it covered now uses `div_euclid`/`rem_euclid` and a checked conversion.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
![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.
Reformat the affected guidance and remove duplicate blank lines so the repository Markdown checks pass.
Adopt the estate rustfmt profile and reformat the workspace under it. The profile requires a nightly rustfmt: it enables crate-granular, std-external-crate-grouped imports, doc-comment code formatting, comment and string wrapping, single-line functions, and try shorthand. The Dylint UI fixtures under `examples/` are excluded explicitly. Their `.stderr` expectations match compiler output byte for byte, including line and column numbers, so reformatting them silently shifts those spans and breaks the suite. Fixtures under `ui/` are already immune because they are not Cargo targets. `fn_single_line` collapses short rstest fixtures onto one line, and the `#[fixture]` expansion then trips `unused_braces` on the collapsed body. The three affected fixtures carry the existing `allow_fixture_expansion_lints` attribute, which scopes the allowance to the generated code rather than to the surrounding module. This commit is mechanical: no behaviour changes accompany it, which is why it is isolated from the fix PRs below it in the stack.
9f08657 to
f3392ba
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- whitaker/suite/src/driver.rs is usually changed with: whitaker/suite/src/lints.rs
- whitaker/common/tests/i18n_behaviour.rs is usually changed with: whitaker/common/tests/i18n_quality/suite.rs
- whitaker/common/src/decomposition_advice/tests/mod.rs is usually changed with: whitaker/common/src/decomposition_advice/mod.rs
- whitaker/common/src/test_support/decomposition.rs is usually changed with: whitaker/common/src/decomposition_advice/mod.rs
- whitaker/installer/src/deps/tests.rs is usually changed with: whitaker/installer/src/deps.rs
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@coderabbitai are you satisfied that the introduced proof(s) (this PR introduces no new proofs; it reflows three existing Kani harness sources — Published head: Proof inventory and claims: the three files above, 18 Production correspondence: unchanged by this PR. The affected Assumptions, trusted boundaries, domain, and bounds: unchanged. The Verifier evidence: no Kani run is attached to this assessment. This PR does not alter any harness body, so no per-obligation outcome can have changed; I am not claiming a verifier result I did not obtain. Kani is not wired into Non-vacuity and limitations: my evidence is a token-level comparison rather than a verifier run. Normalizing comments, identifier internals, whitespace, and hex-digit case, and comparing the token multiset across base and head, gives zero differing tokens for Assessment-ID: PR370-PROOFS-20261016-01 Please echo the Assessment-ID and assessed base/head in your substantive reply. Check substance, rigour, foundations, and correspondence to production code. Identify each unresolved concern or missing inspection explicitly. Context: this PR is a rustfmt normalization of 298 files (297 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base=d6af16316b88db83b508c015a32fa90b351a811c
head=f3392ba8e05b04a106cd4b2dd15d19b777d95118
git rev-parse HEAD
git cat-file -t "$base"
git cat-file -t "$head"
git diff "$base" "$head" -- common/src/decomposition_advice/community_kani/propagate_labels.rs crates/whitaker_clones_core/src/ast/kani.rs crates/whitaker_clones_core/src/index/kani.rs .rustfmt.toml
for f in common/src/decomposition_advice/community_kani/propagate_labels.rs crates/whitaker_clones_core/src/ast/kani.rs crates/whitaker_clones_core/src/index/kani.rs; do
ast-grep outline "$f" --items all
done
fd -i 'kani|propagate_labels|shared|Cargo.toml|rust-toolchain' common crates/whitaker_clones_core .githubLength of output: 1132 🏁 Script executed: #!/bin/bash
gh api 'repos/leynos/whitaker/compare/d6af16316b88db83b508c015a32fa90b351a811c...f3392ba8e05b04a106cd4b2dd15d19b777d95118' --jq '{status, total_commits, files: [.files[] | select(.filename == "common/src/decomposition_advice/community_kani/propagate_labels.rs" or .filename == "crates/whitaker_clones_core/src/ast/kani.rs" or .filename == "crates/whitaker_clones_core/src/index/kani.rs" or .filename == ".rustfmt.toml") | {filename,status,patch}]}'
for f in common/src/decomposition_advice/community_kani/propagate_labels.rs crates/whitaker_clones_core/src/ast/kani.rs crates/whitaker_clones_core/src/index/kani.rs; do
ast-grep outline "$f" --items all
cat -n "$f"
done
fd -i 'kani|propagate_labels|shared|Cargo.toml|rust-toolchain' common crates/whitaker_clones_core .githubLength of output: 40239 🏁 Script executed: #!/bin/bash
ast-grep outline common/src/decomposition_advice --items all --match 'propagate_labels|bounded_iteration_count|minimal_feature_vector|label|iteration'
ast-grep outline crates/whitaker_clones_core/src/ast --items all
ast-grep outline crates/whitaker_clones_core/src/index --items all
ast-grep outline common/src/decomposition_advice/community_kani/shared.rs --items all
cat -n common/src/decomposition_advice/community_kani/shared.rs
rg -n -C3 'cfg\(.*kani|cfg_attr\(.*kani|KANI_|fn (propagate_labels_report|select_smallest_covering|kind_counts|from_checked_lane_seeds_for_kani|candidate_pair_summary_for_kani|bounded_iteration_count)\b' common/src/decomposition_advice crates/whitaker_clones_core/src
rg -n -i -C2 '\bkani\b|unwind|overflow' Makefile Cargo.toml common/Cargo.toml crates/whitaker_clones_core/Cargo.toml .githubLength of output: 51593 🏁 Script executed: #!/bin/bash
for f in common/src/decomposition_advice/community.rs crates/whitaker_clones_core/src/ast/features.rs crates/whitaker_clones_core/src/ast/tree.rs crates/whitaker_clones_core/src/ast/lowering.rs scripts/run-kani.sh; do
ast-grep outline "$f" --items all
done
sed -n '130,342p' common/src/decomposition_advice/community.rs
cat -n crates/whitaker_clones_core/src/ast/cover.rs
sed -n '1,65p;155,170p;220,265p' crates/whitaker_clones_core/src/ast/features.rs
sed -n '1,110p' crates/whitaker_clones_core/src/ast/tree.rs
sed -n '145,255p' crates/whitaker_clones_core/src/ast/lowering.rs
cat -n crates/whitaker_clones_core/src/index/types.rs
cat -n crates/whitaker_clones_core/src/index/lsh.rs
sed -n '1,155p;275,370p' crates/whitaker_clones_core/src/index/minhash.rs
cat -n scripts/run-kani.sh
cat crates/whitaker_clones_core/Cargo.toml
gh api 'repos/leynos/whitaker/compare/d6af16316b88db83b508c015a32fa90b351a811c...f3392ba8e05b04a106cd4b2dd15d19b777d95118' --jq '.files[] | select(.filename | test("^common/src/decomposition_advice/(community.rs|community_kani/shared.rs|vector.rs)$|^crates/whitaker_clones_core/src/(ast/(features|cover|tree|lowering).rs|index/(types|lsh|minhash).rs)$|^scripts/run-kani.sh$|Cargo.toml$")) | {filename,patch}'Length of output: 76533 🏁 Script executed: #!/bin/bash
sed -n '192,220p' crates/whitaker_clones_core/src/ast/features.rs
ast-grep run --pattern 'select_smallest_covering($$$)' --lang rust crates/whitaker_clones_core/src
ast-grep outline scripts/install-kani.sh --items all
sed -n '1,105p' scripts/install-kani.sh
rg -n -C2 'kani|unwind|overflow' .github scripts/run-kani.sh common/Cargo.toml crates/whitaker_clones_core/Cargo.toml Cargo.toml
python - <<'PY'
import pathlib,re
p=pathlib.Path('crates/whitaker_clones_core/src/index/minhash.rs')
s=p.read_text()
part=s[s.index('#[cfg(kani)]\nfn sketch_values'):s.index('/// Extracts sorted')]
indices=[int(x) for x in re.findall(r'minimum_mixed_hash\(seeds\[(\d+)\], unique_hashes\)',part)]
print('Kani MinHash lane sequence equals range(128):',indices==list(range(128)))
for name in ['common/src/decomposition_advice/community_kani/propagate_labels.rs','crates/whitaker_clones_core/src/ast/kani.rs','crates/whitaker_clones_core/src/index/kani.rs']:
print(name, pathlib.Path(name).read_text().count('#[kani::proof]'))
PYLength of output: 10107 🤖 web_search executed:
Length of output: 944
Assessment-ID: Preservation and substanceI confirmed the 4 + 4 + 10 inventory. I inspected the exact comparison through GitHub after the local checkout lacked the base object. The three harness-file diffs contain only import regrouping, whitespace, comment wrapping, and hex-digit case changes. The inspected production-helper and proof-adapter diffs also preserve their logic. The harnesses contain meaningful assertions:
Do not treat token-multiset equality as semantic-equivalence evidence by itself. A multiset discards token order. The actual diffs support your preservation claim more directly. Unresolved concerns
Verifier evidence and conclusionNo Kani run was supplied or performed. The configured unwind bounds and safety properties therefore remain unconfirmed. The workflow search found no Kani invocation. Ordinary tests do not close that evidence gap. Do not require a fresh Kani run solely because these sources were reformatted. Require a pinned Kani run before asserting that all 18 harnesses pass, particularly to settle the arithmetic and unwinding concerns above. The formatting PR preserves the existing assurance and its existing limitations. It does not introduce these concerns, and it does not resolve them. Reassess any changed proof dependencies after the rebase. I did not independently verify the stack-layer vendor defect or the claimed cross-branch byte identity. Thanks for clarifying the assessment scope. No formal review was triggered. ✏️ Learnings added
You are interacting with an AI system. |
|
Assessment Accepted and recorded. I am retracting the "all 18 harnesses establish their Your four concerns are all pre-existing and out of scope for this PR, and I
On the verifier-evidence question: no Kani run is claimed for this PR, and I One correction to my own earlier framing, for the record: the You noted you did not independently verify the stack-layer defect or the claimed |
|
Correction to my own evidence statement in the previous comment, for the record. I was wrong that the current head fails Hosted CI is green on the exact candidate. Run with The two failure classes are one chain, not two defects. This host routes The lock timeout is downstream of the same starvation rather than a separate
So The vendored The PR description has been corrected accordingly. |
This branch has not been deployed
4 participants
Summary
This branch adopts the estate rustfmt profile and reformats the workspace under it. The change is mechanical — no behaviour changes accompany it — which is exactly why it is isolated here rather than mixed into the fix PRs below it in the stack, where it would have buried them.
The profile requires a nightly rustfmt. It enables crate-granular imports grouped std-external-crate, doc-comment code formatting, comment and string wrapping, single-line functions, try shorthand, and field-init shorthand.
Sixth PR in the lint-hardening stack, based on
harden-lint-installer.Review walkthrough
ignorelist: the Dylint UI fixtures underexamples/are excluded because their.stderrexpectations match compiler output byte for byte, including line and column numbers — reformatting them silently shifts those spans and breaks the suite. Fixtures underui/need no entry, as they are not Cargo targets.fn_single_linecollapsed onto one line gained the existingallow_fixture_expansion_lintsattribute: the collapsed body tripsunused_bracesinside the#[fixture]expansion, and this attribute scopes the allowance to the generated code rather than to the surrounding module.Validation
cargo fmt --all -- --check: pass, and idempotent.make markdownlint: pass, 76 files, 0 errors.38934959335 is
successagainstf3392ba, and itslinux-fulljob(log)
covers the gates this PR is judged on. It was dispatched by
workflow_dispatch, because this branch cannot receive apull_requestrun — see the stack note below.
Check formatting— pass.Lint(make lint) — pass, 3m21s.make publish-checkvianextest run --profile ci: 1665 tests run: 1665 passed (5 slow),5 skipped, 0 failures.
Enforce en-GB-oxendict spelling,Nixie,Markdown lint,Workflow contract tests,Publish dry run— all pass.example-harness lock, which is where local runs are affected (see the
notes below).
format_stringscorruption: reformatting can split a string literal mid-escape, turning\ninto a literaln, which compiles cleanly and only surfaces in exact-match assertions. Both corruption signatures were swept across every reformatted file — a continuation backslash followed by an escape-body character, and a line ending in an escaped backslash. Five multi-line literals were flagged by the first sweep and all five are legitimate word-boundary continuations.Notes
The stack: test-quality sweep → spelling → module structure → support-crate lint fixes → installer lint fixes → rustfmt profile → lint configuration → whitaker self-linting gate.
This PR cannot receive
pull_requestCI, and that is a stack condition,not a property of the diff.
refs/pull/370/mergedoes not exist, unlikeevery other layer (Correct en-GB-oxendict spellings in code symbols and prose #363
347a9dd3a, Adopt directory-based module structure #3649cc02773f, Fix workspace-lint violations across support crates #3651f53b6486,Fix workspace-lint violations across the installer #369
c797fbaca, Harden workspace lint, format, and documentation gates #371ba9d750a5, Run the Whitaker suite over Whitaker's own sources #37239b4757f0). GitHub only recomputesa merge ref when it can attempt the merge, and the
harden-lint-*branchesare members of a GitHub native stack (stack
367, id723477, basemain,spanning Sweep test, fixture, and assertion quality #362–Run the Whitaker suite over Whitaker's own sources #372 plus Plan: Collect brain trust diagnostics into a SARIF emitter (6.5.1) #354 and Record an ADR formalizing the brain trust lint driver interfaces (6.1.3) #358; readable at
GET /repos/leynos/whitaker/stacks/367). The stackprimitive rejects base changes outright — re-asserting the current base
returns
422 Cannot change the base branch because the pull request is part of a stack— and it will not recompute a member's merge ref while a lowerlayer conflicts. The break is at Fix workspace-lint violations across support crates #365: it forked from
harden-lint-modulesat
ab2bc5195(2026-08-28) and its base has since gained 108 commits, so amerge produces 87 conflict records (75 content, 1 rename/delete, 11
add/add) and it reports
CONFLICTING/DIRTY. Correct en-GB-oxendict spellings in code symbols and prose #363 and Adopt directory-based module structure #364 areMERGEABLE/CLEAN; Fix workspace-lint violations across the installer #369, Standardize Rust formatting #370 and Run the Whitaker suite over Whitaker's own sources #372 all readUNKNOWN. The push at16:44:59Z and the close/reopen at 17:28Z should both have fired
synchronize/reopened, and neither did.ci.ymlhas no draft guard, sodraft status is not the cause. Fix workspace-lint violations across support crates #365's repair belongs to its own PR.
A local
make testcannot complete on this shared host, for one reason.This is not a property of this diff — the same commit is green in CI, where
the condition does not exist.
This host routes every
rustcinvocation through a sharedbuild-limitssupervisor with a small compile-slot pool. With ~176
build-limits-rustcprocesses queued by other agents, the Dylint UI tests could not build their
lint libraries: compile requests sat 75-265 deep and expired. That produces
four failures, and each one names the cause in its own panic message:
example_compiles_without_diagnostics::case_1tests/ui.rs:88example_harness_collects_call_site_evidencetests/ui.rs:105RunnerFailure→failed to build lint libraryui::aliased_test_crate_non_companion_…lib_ui_tests.rs:95LibraryBuildFailedsha2_0_11_pre_migration_patterns_…run.rs:63LibraryBuildFailedThe lock timeout is a consequence of the same starvation, not a separate
fault. The example-harness lock is deliberately held across the whole example
run — including the lint-library build — so a test that is stuck waiting for
a compile slot holds the lock the whole time. Observed directly: the lock
owner was a
uitest whose childcargo buildwas parked inbuild-limits-rustc(wchan=poll_schedule_timeout, 0 CPU ticks over 20 s),holding
/tmp/rstest-helper-example-harness.owner-lock. A sibling then spanthe 31-minute (
1860 s)ACQUIRE_TIMEOUTand panicked.The final local run logged
1631/1652 tests run: 1621 passed (1 slow), 10 failed, 5 skippedbefore theharness stopped it at 60 minutes with
whitaker_clones_corecases stillwaiting for slots; the 10 failures are retries of those same 4 tests. Treat
this run as inconclusive, and the hosted run above as authoritative.
Separately,
make lintfailed twice at its first step (cargo doc) on thesame starvation —
whitakersat at "0 ahead" for 270 s and admissionexpired, with
build-limits-statusunqueryable ([Errno 11]). Once the pooldrained, the same commit passed cleanly:
cargo doc22 s, then clippy--workspace --all-targets --all-features -- -D warningsin 2 m 16 s, exit 0.Why local and CI differ on the compiler wrapper. Upstream
dylint_testing-6.0.1recognises a compiler invocation by testing whether thefirst argument is
rustc(upstreamsrc/lib.rs:345). Locally,~/.cargo/config.tomlsetsrustc-wrapper, so Cargo reportsRunning `<wrapper> /path/rustc --crate-name …`, the first argument is thewrapper, and a local UI run dies with
Found no `rustc` invocations. CI setsRUSTC_WRAPPER: sccacheand has no repositoryrustc-wrapperconfiguration,so it does not reproduce this: the CI log for the run above contains zero
occurrences of either signature. The workspace carries a vendored patch for
this case, pushed on
harden-lint-support-crates(424cf7a); this PR's based6af163forked before it landed and so does not inherit it. That is astack-ordering fact, not a defect in the reformat.
This diff changes no manifest, lockfile, nextest configuration, or test
expectation:
.config/nextest.toml,Cargo.toml,Cargo.lock,crates/no_unwrap_or_else_panic/src/lib_ui_tests.rsandcrates/rstest_helper_should_be_fixture/tests/ui.rsare byte-identical to thebase. The one non-mechanical file in the diff,
tests/ui/harness_lock.rs,differs by a single collapsed accessor whose logic and constants are
unchanged.
References