Repository navigation
Clear the clippy backlog by hand — 121 warnings the -D warnings gate will fail on - #145
Merged
Merged
Conversation
…pampa, neural-debug
Every edit here was made by hand and reviewed. `cargo clippy --fix` was
deliberately not used: its `unused_*` machinery deletes code the author
may still want, and a lint pass is not the place to decide that.
Behaviour-preserving rewrites only:
- `collapsible_if` -> edition-2024 let-chains (9 sites; none had an `else`
on either arm, so the merge cannot change control flow).
- `get(k).unwrap()` -> `&map[k]` in tests (14 sites). Same panic-on-missing
behaviour, different message.
- `needless_borrows_for_generic_args` -> dropped the `&` in `assert!`,
`panic!` and `println!` format arguments (19 sites).
- `redundant_closure` -> pass the fn item directly to `spawn_blocking`.
- `.map_or(false, f)` -> `.is_some_and(f)`; `.ok().is_some_and(f)` ->
`.is_ok_and(f)`.
- `Iterator::last` on a `DoubleEndedIterator` -> `.next_back()`.
- `let ... else { return None }` -> `?` where the fn already returns Option.
- Unused imports removed (`header`, `ShaderDispatch`, `CallStats`, `Header`);
unused `State` params renamed to `_state`; an unnecessary `mut` dropped.
- `unneeded_late_init`/`needless_return` in the `cfg(not(instrument))` arms
of neural-debug: each `return` was that arm's sole statement and therefore
already the function's tail expression.
- One unreadable literal regrouped (`0x00AB_12u32` -> `0x0000_AB12_u32`,
identical value).
- `doc_lazy_continuation` in osm_features: a wrapped `>=95%` was opening an
accidental rustdoc blockquote. Escaped it (`\>=95%`) rather than marking
five continuation lines as quoted prose they were never meant to be.
Deliberately NOT touched, and reported instead: ~30 `never used` /
`never read` / `never constructed` findings across cockpit-server,
osm_artifact_manager, osm_tiles and codebook. Deleting them is a product
decision, not a lint fix.
Verified: `cargo clippy -p neural-debug -p aiwar-ingest --all-targets` is
clean. The heavier crates are verified in the follow-up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGVLyRZNEKKBSfBDJfbY3V
Same discipline as the previous commit — every edit by hand, no
`clippy --fix`, nothing deleted on a "never used" finding.
- Unused imports removed (`InferenceType`, `nars_deduction`,
`nars_induction`, `nars_abduction`, `PlanResult`, `HashMap`, `Instant`);
each verified to have no remaining use in its file.
- `collapsible_if` -> let-chain in `execute_graph_query` (no `else` on the
outer arm, so the fallthrough to the graph echo is unchanged).
- `sort_by(|a, b| b.len().cmp(&a.len()))` -> `sort_by_key(Reverse(len))`.
- `.map(|s| s.clone())` -> `.cloned()`; `if let Some(x) = r.ok()` ->
`if let Ok(x) = r`; manual `Iterator::find` loop -> `.find(..)`.
- `if calls > 0 { total / calls / 1000 } else { 0 }` ->
`checked_div(calls).map(|q| q / 1000).unwrap_or(0)`.
- `revise_edges(&mut Vec<TruthEdge>)` -> `&mut [TruthEdge]`. The body only
iterates; `grep` over the whole tree finds no caller, and `&mut Vec`
coerces at any future one.
One clippy suggestion was NOT applied verbatim, because it is not
behaviour-preserving. `manual_clamp` rewrites `w.min(1.0).max(0.0)` as
`w.clamp(0.0, 1.0)`, but the two disagree on NaN: `f64::min` returns the
non-NaN operand, so the old chain turned a NaN weight into 1.0 — maximum
confidence — while `clamp` propagates NaN into the truth value. The weight
comes from `w.parse::<f64>()` on a graph column and Rust's float parser
accepts "NaN", so it is reachable. Kept `clamp` and added
`.filter(|w| w.is_finite())` ahead of it, so a non-finite weight falls back
to the default frequency instead of becoming certainty. Comment records why.
Reported, not fixed: `RawNode::node_type` never read; `compute` takes 11
arguments; five `new()`-without-`Default` suggestions. This crate mirrors an
upstream API surface, so adding `Default` impls is a product decision.
`rustfmt` deliberately not run on these files: they carry 1986/564/297/205/
92/192/79 lines of pre-existing drift at HEAD, which would bury seven lint
fixes in ~3400 lines of unrelated churn.
Verification of `-p notebook-query` is still running at commit time; any
finding it reports lands as a follow-up on this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGVLyRZNEKKBSfBDJfbY3V
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 75 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (30)
Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_80126e8e-092d-4408-a60d-c256888aabd7) |
`cargo clippy -p notebook-query --all-targets` came back clean apart from the five items the previous commit deliberately left for a decision. The gate runs `-D warnings`, so "reported, not fixed" still fails it — these are now closed, each in the direction that removes nothing. - `Default` for `StyleTopology`, `OsintCounter`, `OsintRegistry`: implemented as `Self::new()`. Purely additive; the lint fires precisely because each `new()` takes no arguments. - `GraphSensorium::compute` (11 args): `#[expect(clippy::too_many_arguments)]` with a reason. The arity mirrors the upstream API this stub stands in for, so grouping the counters into a parameter struct would make the stub diverge from the thing it exists to mirror. - `RawNode::node_type` never read: kept, with `#[expect(dead_code)]` and a comment. The struct is deserialize-only and documents the shape of `aiwar_graph.json`; deleting the field would leave a type that is a partial and misleading record of the file it parses. `#[expect]` rather than `#[allow]` deliberately — it warns if the lint ever stops firing, so neither suppression can outlive its reason unnoticed. Verified: `cargo clippy -p notebook-query --all-targets` is now clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AGVLyRZNEKKBSfBDJfbY3V
Same closeout as the notebook-query commit, for the other stub crate: `Session::new()` and `SessionContext::new()` both take no arguments, which is exactly what makes clippy ask for `Default`. Implemented as `Self::new()` — additive, nothing removed. Verified: `cargo clippy -p graph-flow --all-targets` is clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AGVLyRZNEKKBSfBDJfbY3V
AdaWorldAPI
marked this pull request as ready for review
September 6, 2026 08:32
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Sep 6, 2026
AdaWorldAPI
pushed a commit
that referenced
this pull request
Sep 6, 2026
…pit build CodeRabbit flagged `mime_from_path`'s `#[expect(dead_code)]` as unfulfilled under `--features embed-cockpit`. It was right, and chasing it found something bigger sitting underneath: with that feature enabled, cockpit-server did not compile at all. ## The regression, and it is mine `25f4b940` (the #145 lint sweep) removed `use axum::http::{StatusCode, header}` down to `StatusCode` alone. `header::CONTENT_TYPE` is used at five sites — 614, 670, 712, 745, 757 — and every one of them is inside a `#[cfg(feature = "embed-cockpit")]` block. In a default build those blocks are compiled out, so the import genuinely looks unused and clippy said so. I removed it. With the feature on: error[E0433]: cannot find module or crate `header` x5 CI never enables `embed-cockpit`, so nothing caught it. The Dockerfile does (`cargo build -p cockpit-server --features embed-cockpit`), so this has been broken on the production build path since #145 merged. Fix: restore the import gated to match its usage, rather than merging it back into the braced form that caused the problem — a single `use` cannot be half-gated: use axum::http::StatusCode; // both configurations #[cfg(feature = "embed-cockpit")] use axum::http::header; // only where it is used ## The lint finding that led here `mime_from_path` keeps its annotation but as `#[allow]`, not `#[expect]`: with the feature enabled `static_handler` calls it, so no expectation holds in every configuration. Same structural rule this PR already applied to 18 other items — I simply failed to apply it here, because my gate only ever ran the default feature set. That was the real hole: the classification was sound, the measurement behind it was too narrow. main.rs now carries 1 `expect` (the `jsonrpc` wire field, dead in every configuration) and 6 `allow`. ## Verification — both configurations this time default, workspace, -D warnings 0 findings --features embed-cockpit, cockpit-server 0 errors cargo fmt --all -- --check clean The feature build needs `cockpit/dist` to exist for `include_dir!`; a stub was created locally to verify and removed afterwards (it is gitignored either way). ## Deliberately NOT fixed here The feature build still reports 6 style warnings — 3 `needless_return`, 3 `collapsible_if`, at main.rs 629/668/678/710/720/755 — in code that until this commit could not be compiled, so nothing had ever linted it. They block neither CI (which does not enable the feature) nor `cargo build`. Left alone because the `needless_return` fix turns on cfg-dependent tail-expression semantics: those `return`s are only redundant because the paired `cfg(not(...))` block disappears, and getting that wrong would re-break the build path this commit repairs. Worth its own change, not a drive-by on a lint PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AGVLyRZNEKKBSfBDJfbY3V
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #144. That PR fixed only what the 1.98 toolchain move broke —
the two
chunks_exact_to_as_chunkssites and the twoiter_kv_mapsites thatturned the
cargo xtask verifygate red. This PR clears the rest of thebacklog the same sweep surfaced — the warnings the gate's
-D warningsturnsinto errors — across 28 files in 9 crates.
Measured end state:
aiwar-ingest,neural-debug,notebook-queryandgraph-floware clippy-clean.cockpit-serverhas 36 findings left,and what they are is the one open question on this PR (see below).
Based on
main(post-#144 merge), not stacked on the merged branch.Method
Every edit was made by hand.
cargo clippy --fixwas run once, inspected,and reverted before any of this work: its
unused_*machinery rewrites codewhose value is a product decision, and a lint pass is not where that decision
belongs. The captured autofix diff was 749 lines.
Fan-out was five workers on disjoint file sets (a shared file is a
lost-write race), each briefed to edit only — no
cargo, nogit, noworktree — with compilation and linting done centrally afterwards. Each was
told explicitly not to delete anything reported "never used" / "never
read" / "never constructed", and to report it instead.
What changed
Behaviour-preserving rewrites:
collapsible_ifelseon either armneedless_borrows_for_generic_args&inassert!/panic!/println!format argsget_unwrapget(k).unwrap()→&map[k]in tests — same panic, different messageredundant_closurespawn_blockingunused_importsneedless_returncfg(not(instrument))arms; eachreturnwas already the tail expression.map_or(false, f)→.is_some_and(f);.ok().is_some_and(f)→.is_ok_and(f);Iterator::last→.next_back();let ... else { return None }→?;.map(|s| s.clone())→.cloned();sort_by(cmp)→sort_by_key(Reverse(..)); manualfindloop →.find(..); unusedStateparams →_state; one unnecessarymut; one unreadable literal regrouped (identical value)Two places the clippy suggestion was not taken verbatim
manual_clampis not behaviour-preserving here. It rewritesw.min(1.0).max(0.0)asw.clamp(0.0, 1.0), and the two disagree on NaN:f64::minreturns the non-NaN operand, so the old chain turned a NaN weightinto
1.0— maximum confidence — whileclamppropagates NaN into the truthvalue. The weight comes from
w.parse::<f64>()on a graph column, and Rust'sfloat parser accepts
"NaN", so it is reachable. Keptclampand added.filter(|w| w.is_finite())ahead of it, so a non-finite weight falls back tothe default frequency instead of becoming certainty. A comment records why.
doc_lazy_continuationinosm_features.rsfired because a wrapped>=95%was opening an accidental rustdoc blockquote mid-sentence. Clippy'ssuggestion is to mark the five continuation lines as quoted; that silences the
lint but renders five lines of ordinary prose as a blockquote they were never
meant to be. Escaped the
>instead (\>=95%), which removes the blockquoteentirely and the lint with it.
Reported, deliberately not fixed
About 30
never used/never read/never constructedfindings acrosscockpit-server(main.rs,codebook.rs,openai.rs,graph_engine.rs,scene_player.rs),osm_artifact_manager.rs,osm_tiles.rsandnotebook-query. Deleting them is a product decision, not a lint fix, and thestubs/crates deliberately mirror an upstream API surface. Also left alone:five
new()-without-Defaultsuggestions instubs/for the same reason, acomputewith 11 arguments, and a "very complex type" ondecode_tile_bin—all design-level.
One genuine smell surfaced and not silenced:
osm_features.rs:2374'sassert!(GEOMETRY_CITY_BUDGET > GEOMETRY_OVERVIEW_BUDGET)compares two
consts, so it folds at compile time and can never fail atruntime. Flagged rather than removed.
Formatting
rustfmtwas not run onaiwar-ingest,neural-debugorstubs/. Thosefiles carry substantial pre-existing drift at HEAD (measured: 27, 52, 77, 133,
1986, 564, 297, 205, 92, 192 and 79 lines respectively), so formatting them
would bury the lint fixes in several thousand lines of unrelated churn. Files
that were already
rustfmt-clean stayed clean.Verification
Run locally,
CARGO_PROFILE_DEV_DEBUG=0 CARGO_INCREMENTAL=0, all--all-targets:neural-debugaiwar-ingestnotebook-querygraph-flowcockpit-servercargo build --workspaceandcargo nextest run --workspaceare not runnablein this sandbox (
nextestis absent, and the tree links v8,aws-lc-sysanddatafusion against a ~1.7 GB remaining disk budget).
Worth stating plainly, because it changes what "wait for CI" means here.
.github/workflows/test-suite.yml:29reads:ts-test-suite.yml,hub-client-e2e.yml,release.ymlandcleanup-caches.ymlcarry the same guard. That job is the one holdingcargo fmt --all -- --check,cargo xtask lintand the clippy-D warningsgate — so on
AdaWorldAPI/q2every one of them reportsskipped, on this PRand on every fork PR before it. The green checkmark on #144 was three skipped
jobs.
Two consequences:
gets. Not a formality — please read them as the gate.
cargo fmt --all -- --checkcurrently reports 189 files needing formatting —aiwar-ingest,neural-debug,osint-bake,cpic,stubs/*,cockpit-server— none ofwhich exist upstream, so upstream's green gate says nothing about them. It
would fail on formatting before ever reaching clippy. (It also walks into
the sibling
openstreetmap-website-rscheckout through a path dep, which isits own problem for a CI runner.)
That is why this PR deliberately does not run
rustfmton the driftedfiles: doing it here would mix a 189-file reformat into a lint cleanup. It is
worth its own PR, and worth deciding whether the fork should un-gate the test
suite so the question stops being invisible.
The 36 that remain, and why I did not touch them
33 of the 36 are
dead_code—never used/never read/never constructed. They are not a lint problem, they are a question aboutwhat this crate is for, and the answer is not mine to assume:
resolve_garmin_{map,scene,drape,contours},garmin_scene_slugs,morton_interleave,morton_deinterleave,stride_for,mime_from_path,publish_lifecycle,publish_artifacttoken_to_index,tokens_to_indices,extract_cypher_identifiers,default_distance_table,CODEBOOK_SIZE,CYPHER_KEYWORDS,MAX_IDENTIFIERS_PER_CALL,NAMESPACE(×6)openai.rsrequest/response structs,ErrorResp,ErrorObj,scene_player'ssource/timestamp,main.rs'sjsonrpctruth_f,truth_cThere are two defensible dispositions and they differ per group. The DTO group
is the same case as
RawNode::node_typein this PR, which I closed with#[expect(dead_code)]and a comment: the struct documents a wire schema, anddeleting the field leaves a type that is a partial and misleading record of the
protocol. The unwired-surface groups are different — code written ahead of its
caller is a real signal, and blanket-suppressing it buries that signal rather
than resolving it.
So: say which way you want each group to go and I will push it. What I will
not do unprompted is delete them, which is exactly what
clippy --fixdoes.The remaining 3 are not
dead_code:osm_features.rs:2374—assert!(GEOMETRY_CITY_BUDGET > GEOMETRY_OVERVIEW_BUDGET, ...)compares two
consts, so it folds at compile time and can never fail atruntime. A genuine smell, flagged rather than silenced.
osint_gotham.rs:704—osint_node_rowsis more public thanBasinPlan,the type it takes.
osm_features.rs:2246—decode_tile_bin's return type is flagged"very complex".
All three are design-level and would widen this PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AGVLyRZNEKKBSfBDJfbY3V