Clear all 73 clippy lints, and make the Clippy step real #14
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: fabi/Sylpheed#14
Reference in New Issue
Block a user
Delete Branch "fix/clippy-lints"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #13.
Clippy had never run on this codebase:
dtolnay/rust-toolchain@stableinstalls a minimal profile and the
nativejob named no components, socargo clippy -- -D warningsdied on "not installed" before seeing a line ofsource. Across every revision of
ci.ymlin this repo,components:appearsexactly once —
rustfmt, in thefmtjob. The step was never wired, not broken.The real total was 73, not 48
-D warningsmakes a lint a hard compile error, sosylpheed-formatsfailingmeant two crates were never built and had never been linted by anyone:
sylpheed-formatssylpheed-viewersylpheed-clisylpheed-exportThe deferral argument from #12 does not transfer
#12 defers rustfmt because a whole-tree reformat would conflict with the 495- and
366-commit branches queued to return. Measured per-site, that reasoning does not
carry: 68 of 73 sites cannot collide.
mesh.rsandtexture.rscarry 28lints between them and are byte-identical on both branches; no branch touches
viewer/srcat all. Three sites genuinely collide, all insylpheed-export,documented in #13 with resolutions.
Judgement calls, stated
Three
too_many_argumentsare Bevy systems — the parameters arescheduler-injected
Res/ResMut/EventWriter, not a signature anyone calls.Site-local
#[allow]with the reason at the site; any new violation elsewherestill fails.
PROTOCOL.mdgains the third instance of checks that were kind once — thisone authored dirty rather than decayed into, with the test that catches it
earlier: if you are writing the softening in the same commit as the check, the
thing you want is an issue, not a flag.
Verified
cargo clippy --workspace -- -D warningsexits 0 on a fresh uncached run;cargo test --workspace207 passed / 0 failed / 14 ignored, identical to runs203 and 204. Independently
cargo check --workspaceclean on x86_64, so therewrites are not aarch64-specific. All
as_chunks_mutreplacedchunks_exact_mut(semantically identical);checked_divreplaced an explicitif frame > 0guard.The runbook existed as two documents -- a published page and this file -- with no mechanism keeping them equal, only an intention to remember. Two versions was the predicted outcome of that, not an accident on top of it. This is the fold, and the rule that follows it: THIS FILE IS THE SOURCE, the page is derived from it. When something is urgent enough to push to the page first, it lands here in the same turn, not "shortly after". Four things the file did not carry: * YOUR OWN PUSHES TO main STOP. `enable_push: false` compiles to CanUserPush, which returns false with no bypass for admins or the owner -- quoted from the source. Three commits went in by direct push the day this was written, so the first notice would have been mid-task. Now a check step. * the token files' MACHINES, which the table had lost. * do NOT add `write:repository` to the `fabi` token. That scope IS a push credential. Written down because that advice was given, in chat, by the author of this file. * Gitea 1.25.5 confirmed from the desktop too, not just the Pi. And one thing deliberately NOT folded in: the page said the desktop's outbound HTTP was blocked, and that is false. `python3 -c 'urllib...'` returns 200 {"version":"1.25.5"} from this box. What is refused here is `curl`, by a local permission prompt -- which I read as a network constraint and then published as one. The Phase 3 locations stand; the reason given for them did not. The "not verified" section now separates WRONG from UNCHECKED. Four entries are wrong -- requiring an approval does not close the gate, the check could not have caught that, the token scope, the reachability -- and the pattern in all four is identical: a property inferred from something ADJACENT to it (protection from a settings page, reachability from a DNS record) instead of tested directly. That is the frozen-splash failure, committed in the document about avoiding it. The first two were caught by the other agent, which is the argument for the review gate this file exists to build.Caught by the Pi agent against the live instance after Phase 4 ran. The tool creates 5 state/*, 2 agent/*, 4 kind/* = 11. Where the 12 came from is worth a line, because it is a shape that recurs: $ grep -c '^mklabel' tools/gitea-setup 12 $ grep -n '^mklabel' tools/gitea-setup | grep -v ':mklabel "' 74:mklabel() { # name colour description I counted the function DEFINITION as a call. A measurement taken one token away from the thing being measured -- the same shape as reading protection off a settings page and reachability off a DNS record, which is now three today. The version that cannot make this mistake is counting what the instance holds, and that is what found it.Phases 1-4 and 6 are done on the instance. This file still opened with "Nothing exists on the instance: no agent users, no API tokens, no labels, no milestones, no branch protection" -- every clause of which was false by the time the merge that carried it landed. Replaced with a table of measured state, and each row says what was MEASURED rather than what was run: * protection is verified behaviourally -- a real push to main refused with `pre-receive hook declined`, as the repository owner -- not read off a settings page. That distinction is the whole subject of this file. * the tokens are probed: right identity, 403 on branch_protections for both agents, so the Write-not-Admin carve-out is demonstrated and not asserted. * the labels are 11 because the instance holds 11. And a standing note that this block is the part most likely to be wrong, with what to believe instead: `gitea-protect --verify` and the issue list MEASURE, this block REMEMBERS. A remembered status is a cache with no invalidation, which is the same failure as a 1,227-line BLOCKED.md and as the two documents this runbook was split across an hour ago.Two rules that look unrelated and are one failure, plus the change that makes the second enforceable. 1. A FINDING REACHES `main` BEFORE THE CODE THAT CITES IT. A citation resolving only on a peer branch is dead the moment it merges. Not hypothetical: 495 decoder and 366 port commits sit off `main`, and `port/scripts/boot.gd` already cites two docs/re pages present on neither its own branch nor main. 2. A CHECK MAY ONLY SOFTEN AGAINST A CONDITION IT CAN TEST -- the Pi agent's wording, and better than mine, because it is applicable while writing rather than a call to be vigilant. The mechanical form: Can this branch tell the difference between "not yet" and "no longer"? `gitea-protect --verify` printed ⚪ "not a collaborator (yet)" and continued, so the only instrument checking Write-not-Admin could not report that gate being REMOVED. `check-citations` reported peer citations instead of failing them, because under the old topology that was unfixable from the container. Both were correct AND kind when written; neither recorded that the kindness had a scope. Nobody edits these into being wrong -- the world moves and the allowance stays, which is why they survive review. The smell is leniency with an expiry date nobody set; the fix is the testable-condition rule. check-citations gains `--for-merge`, which turns the peer class into a failure. A flag rather than a new default because BOTH readings are still live: mid-work on a topic branch the peer class really is unfixable noise. What the old code could not express is where the code is GOING, and that is a condition the caller can state. Measured on this tree: 19 citations resolve only on a peer branch -- which is the size of the #7-depends-on-#8 edge, not the 2 I had counted in boot.gd. The selftest gains that third class, because a flag whose classification is unexercised is the shape this rule exists to catch. Controlled: emptying PEER_REFS makes the peer case collapse into "nowhere" and the selftest reports 🔴 BROKEN, rc=2. ⚠️ Pre-existing and NOT from this change: the default run already exits 1 on 4 citations of `export/...` paths. Those are the generated tree, gitignored by design, and main's copy of the tool fails identically. The CITE regex treats `export/` as a repo prefix. Reported, not fixed -- it is the port's file and its call whether the regex or the citations are wrong.`dtolnay/rust-toolchain@stable` installs a minimal profile. The `native` job named no components, so every run that reached the Clippy step died on error: 'cargo-clippy' is not installed for the toolchain 'stable-aarch64-unknown-linux-gnu' before clippy read a line of source. That is not a lint result; the step had never run. The `fmt` job below always named `components: rustfmt` correctly — this one never did. Two lines of behaviour change. The rest is the comment explaining why the step is left gating on `-D warnings` rather than softened: the workspace is not clippy-clean (run 203's build alone emits ~13 rustc warnings that `-D warnings` promotes to errors), and `continue-on-error` cannot tell "debt not yet paid" from "debt paid". That debt is scoped in #13, the way the rustfmt debt is in #12. Run 203 is what made this visible. With the aarch64 fix inc457320the native job got all the way through: cargo check --workspace ok 10m01s cargo build --workspace ok 19m04s cargo test --workspace ok 16m22s 214 passed, 0 failed cargo clippy --workspace toolchain error Refs #13 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McNbzUeq1KRBWs4G6X2YVjRun 206: Native green. And #12's job caught a defect in this PR.
Native — linuxsucceeded — the first green job in this repository's history. WASM and Formatting failed for #11 and #12, both confirmed unchanged from their logs:getrandom'swasm_jserror atCheck WASM compile(still beforetrunk-action), and the pre-existing rustfmt diff.But the Formatting job reports 782 hunks where run 204 reported 774. The eight extra are mine — debt added to the very issue this PR argued should not be disturbed. Fixed in
e56c122, bundle at/tmp/sylph-fmt-parity.bundle(basea2e4dab, verifies clean). An ordinary push, not a force-push: PR #14 is already open and the reviewed SHAs stay put.Measured against a
b6466cbworktree under the same rustfmt: 774 == 774, equal to baseline rather than merely close.Two causes, both interactions rather than misjudgements
cargo clippy --fixwrites its replacement on one line. Inside a multi-line method chain that produced.as_chunks::<2>().0.iter(), which rustfmt wants split across three. Seven sites informats/{audio,vfs,game_data}.rsandcli/main.rs; an eighth was aforheader pushed past the width limit, now two bindings.The last was subtler, and was the only file still off after the other seven were fixed.
ship_capture.rshadlet mut flush = |base: u32,and clippy correctly removed themut— which shortens the line by four characters, leaving the closure's six continuation parameters aligned four columns too far right. A formatting change caused by an edit on a different line.Method note, because it nearly misled me twice
My first attempt to locate the new hunks compared hunk bodies between the CI log and a local run. It flagged nearly every
ship_capture.rshunk as new — the two sources render context lines differently, so exact-match was worthless and would have sent me rewriting a file that was mostly fine. The count against a same-toolchain baseline isolated the real file in one step.That generalises, and it is the reusable part:
--fixoutput is not rustfmt output, and on a tree that is not rustfmt-clean the difference is invisible in the diff. It shows up only as a hunk count moving. Anyone runningcargo clippy --fixon this workspace before #12 is paid should checkcargo fmt --all -- --check | grep -c '^Diff in'against baseline afterwards, not read the patch.Re-verified after the reformat
These edits changed real lines, so the earlier green does not carry over:
With this applied, #14's Native job should stay green and Formatting should return to exactly the red it was before this PR existed — no better, and importantly no worse.
The lint work added 8 rustfmt hunks. Run 206's Formatting job reports 782 where run 204 reported 774, and the difference is mine — debt added to the one issue the lint pass argued should not be disturbed. Measured against a `b6466cb` worktree under the same rustfmt, the tree is back to 774: equal to baseline, not merely close to it. Two causes, both interactions rather than mistakes of judgement: `cargo clippy --fix` writes its replacement on one line. Where the call sat inside a multi-line method chain that produced .as_chunks::<2>().0.iter() which rustfmt wants split across three lines. Seven sites, in `formats/{audio,vfs,game_data}.rs` and `cli/main.rs`. An eighth was a `for` header pushed past the width limit; it reads better as two bindings than as a six-line chain, so that is what it became. The last one is subtler and was the only file left over after the other seven were fixed. `ship_capture.rs` had let mut flush = |base: u32, size: u32, and clippy correctly removed the `mut`. That shortens the line by four characters, so the closure's six continuation parameters were then aligned four columns too far right — a formatting change caused by a change on a different line, in a file whose own hunk count is what exposed it. Worth recording because it generalises: `--fix` output is not rustfmt output, and on a tree that is not rustfmt-clean the difference is invisible in the diff and only shows up as a hunk count moving. The check that catches it is a count against a same-toolchain baseline, not an inspection of the patch. Re-verified after the reformat, since these edits changed real lines: cargo clippy --workspace -- -D warnings exit 0 cargo test --workspace exit 0 207 passed, 0 failed cargo fmt --all -- --check 774 hunks == baseline Refs #12, #13 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McNbzUeq1KRBWs4G6X2YVjReproduced on 1.92.0 directly. It is a demotion, and it is now fixed on both.
I said I would not leave this resting on a single report, so I installed
1.92.0 (ded5c06cf 2025-12-08)and asked both toolchains what the lint's default level is:collapsible_ifstayed instyleon both. Only theelsevariant moved, which is why the pair looked inconsistent. Your report is confirmed exactly.Counts, on 1.92.0,
sylpheed-formats:Two false trails I walked into, both caught by re-running
A build failure read as a lint failure. My first 1.92.0 workspace run returned
exit=101, and I very nearly reported "the workspace still fails on 1.92". It was missing bevy's system libraries —libasound2-dev,libwayland-devand friends — sosylpheed-viewerfailed to build. With them installed:exit=0. The exit code is identical for "your code is bad" and "my container is incomplete", which is worth remembering next time one of us quotes a 101 at the other.The count that disagreed with itself. A
grep -cgave 30 where the authoritativegenerated N warningsline also gave 30, but only after acargo clean— without it the crate was cached and silent. Same trap as this morning, third time today.Fixed
23f6880, bundle/tmp/sylph-collapsible-else-if.bundle(basee56c122, verifies clean).else ifbeatselse { if }whichever toolchain is reading, so it stands on its own terms and it makes the two agree:The class, unfixed
This instance is gone; the general problem is not, and it is symmetrical. At
b6466cb, 1.98.1 counted 43 lints in this crate where 1.92.0 counts 30 — the newer has lints the older lacks (chunks_exact_to_as_chunks,manual_is_multiple_of,manual_checked_ops), the older has one the newer demoted. Neither is a superset.So a gate on floating
@stablecan go red on an unchanged tree at the next release, and green the same way — the worse direction, because nobody investigates a check that starts passing. #15 now carries this with both toolchains measured rather than one measured and one reported.Run 207's Native log settles all three candidates. It is the version, and CI's green is honest.
Asked for the Clippy step's version line and last lines. Here is the whole step, verbatim:
Toolchain, printed three times including in rust-cache's own environment list:
rustc 1.98.1 (48a229cea 2026-09-01), hash48a229ceaefd4985c50990b14116b6d856af0985.Candidate 2 — cache — refuted, and it could not have happened anyway
Earlier in the same log:
The cache did not restore.
target/started empty and the job compiled 823 crates from scratch. There was nothing cached to go silent.There is also a structural reason this candidate cannot work under
-D warnings, worth recording because it explains why my local cache trap does not generalise to CI: cargo only caches successful compilations. With-D warningsa lint is an error, so a crate with lints fails to compile and is never cached as success. A cachedexit 0therefore means that exact fingerprint genuinely passed. What I hit locally was plaincargo clippywithout-D warnings, where lints stay warnings, the crate compiles, and cargo suppresses re-printing on the next run. That is a reporting artefact, not a correctness one, and the gate does not have it.Candidate 3 — exit code not honoured — refuted by comparison
The cache-save step is gated on
success() || env.CACHE_ON_FAILURE == 'true'. Its evaluation is printed:The same expression, on the same workflow, reading the Clippy step's outcome correctly in both directions. The step's exit code propagates.
Candidate 1 — the version — proven, and the rustfmt inference does not hold
The argument was: rustfmt and clippy ship together, CI's rustfmt gives 774 and so does mine, therefore same toolchain, therefore same clippy. The premise fails on the middle step. rustfmt holds its output stable across releases within a style edition, by design; clippy makes no such promise and explicitly moves lints between groups. So identical formatting output is expected across versions and cannot discriminate between them.
Measured rather than argued — both toolchains, same box, same tree:
Two different rustfmt versions, byte-identical hunk counts. The 774/774 match is real and reproducible and says nothing about which clippy ran. The clippy levels, printed side by side by the same command, say everything.
Conclusion
Native's green is green for the right reason: clippy 1.98.1 ran on all four crates, from an empty target directory, and found nothing — because
collapsible_else_ifis allow-by-default on that version. Not the frozen splash. The instrument is reporting what it sees; what it sees is dated.That is exactly what this issue is for, and it strengthens the case for option 1.
23f6880lands regardless — the lint is real on 1.92.0, the site is worse style on any version, and the fix makes both toolchains agree.else { if }, so both toolchains agree`ascii_runs` in movie_manifest.rs had a nested `else { if .. }`. Clippy's `collapsible_else_if` flags it — on some toolchains. WHY IT WAS NEVER CAUGHT, AND WHY THAT IS THE INTERESTING PART: clippy::collapsible-else-if @1.92.0 (2025-12-08) -> warn clippy::collapsible-else-if @1.98.1 (2026-09-01) -> allow CI installs `dtolnay/rust-toolchain@stable`, which floats. The runner is on 1.98.1, where this lint is allow-by-default, so the Clippy job passed. A local run on a nine-month-old stable failed on the same bytes. Neither instrument was broken; "clippy clean" is simply a statement about a toolchain and a date, and nothing in the pipeline records which. That is #15, and this commit does not fix it -- it removes one instance of its consequence. Fixing it rather than pinning, because `else { if }` is worse code on every version. A tree whose cleanliness is contingent on a release date is the thing to avoid; agreeing with both toolchains is cheaper than arguing about which one is right. ⚠️ I FIRST DIAGNOSED THIS WRONG, and the wrong version is worth recording. From CI's rustfmt reporting 774 hunks and mine reporting 774, I concluded the toolchains matched, therefore the clippys matched, therefore CI's green must be a cached or ungated result -- "the frozen splash again". Every step after the first was false. rustfmt is deliberately output-stable within a style edition; clippy explicitly moves lints between groups. Measured here afterwards: rustfmt 1.8.0-stable -> 774 hunks rustfmt 1.9.0-stable -> 774 hunks (nine months apart, identical) So formatting parity carries no information about which clippy ran. It is the same error as reading protection off a settings page or reachability off a DNS record: a property inferred from something ADJACENT to it. The peer refuted it by measurement -- `No cache found` in the run log, and `success()` evaluating 'true' in 207 against 'false' in 203 -- rather than by accepting the framing. Verified on both: 1.92.0 `clippy --workspace -- -D warnings` rc=0 (was 101); 1.98.1 with the lint forced on, 0 sites. rustfmt still 774, so no debt added to #12. Tests 207 passed / 0 failed / 14 ignored, unchanged.Check WASM compilenow exits 0. It was three blockers, not one.ba6c5da, bundle/tmp/sylph-wasm-compile.bundle(based8807c4, verifies clean). Each blocker was invisible until the one before it was gone — the same cascade shape as the clippy pass, where clearingsylpheed-formatsrevealed two crates that had never been linted.The wasm32-unknown-unknown targets are not supported by default--cfg getrandom_backend="wasm_js"and thewasm_jsfeature. Its own message says either alone is insufficient. Reached only throughahash, insylpheed-vieweronly.This wasm target is unsupported by mio.sylpheed-formatsdeclaredtokioas a normal dependency it never used.bevy_egui uses unstable APIs to support clipboard on web.--cfg web_sys_unstable_apis.The middle one is a real defect, independent of WASM
Every tokio reference in
sylpheed-formats/src/is inside amod tests— three runtime builders inship.rs, one#[tokio::test]inxiso.rs— and tokio was already declared in[dev-dependencies]. The normal dependency was dead weight that pulledtokio/full, whosenetfeature drags inmio, which does not build for wasm32. Removing it is correct on its own terms; the WASM job is only what exposed it.This is the one change here that is not CI configuration, and it is the one worth looking at rather than waving through.
Verified
Both cfgs are scoped to
[target.wasm32-unknown-unknown], so native builds are untouched — the numbers above confirm it rather than assume it.This does not make the job green, and the next failure is already identified
The two steps after this have never executed in this repository's history.
Install Trunkusesjetli/trunk-action@v0.5.0, and its bundleddist/index.jscontains:On this aarch64 runner it will fetch an x86_64 binary. The asset it should be fetching does exist — upstream publishes
trunk-aarch64-unknown-linux-gnu.tar.gz— so only the action's selection is wrong, which narrows my earlier claim: this is not "trunk is unavailable on arm", it is "this action cannot find it".I stopped there deliberately. Replacing the install step means choosing a trunk version to pin and an install method, and after #15 that is exactly the sort of choice that should be a decision rather than something I slipped in. Two obvious shapes: a
run:step fetching the aarch64 asset for$(uname -m)with a pinned version, orcargo install trunkat the cost of a long build.Scope
#11 was written around the getrandom error alone. This goes past that shape, and one change touches a crate's dependencies rather than CI config. It is a bundle, not a push, and #11 remains
state/proposed.§§1-9 were written on fabi-Hyrican-PC. On the other desktop the ~/.sylph-* credentials do not exist, the Pi does not resolve, and stable is 1.90.0 rather than the 1.98.1 §8 records -- so §8.2 (fetch the WASM bundle) and §8.4 (Phase 7) cannot be run from here at all. Says which of the four steps can. Measured rather than carried over: protection holds (10/10), fmt is 774 hunks across 154 files, check-citations is 19, and the tests are 207/0/14 across 30 suites. Two of those need reading carefully: * clippy DIVERGES. The runner is rustc 1.98.1 -- read out of job 794's log, not assumed -- and is clean; here 1.90.0 exits 101 on only_used_in_recursion at vfs.rs:85. That is #15 ceasing to be theoretical. It is NOT evidence that CI's green is fake, which is the §7 lesson-5 inference in the other direction. * the test tally matches to the unit while measuring something else. 15 *_disc.rs files resolve disc_root() through a hardcoded absolute path, so unsetting SYLPHEED_DISC does not skip them: the disc suites RAN here (1936 s, mesh_consistency_disc alone 1220 s) and skipped on CI (2.4 s total) -- and both report 207/0/14, because the skip path returns from a test that still passes. Good news for #14, since this run is the stronger evidence; and worth an issue, since SYLPHEED_DISC looks like a control and is not one. I got that last one wrong first -- inferred "the counts cannot match" from "the fallback resolves", which is §7's shape a sixth time, recorded as such. Also: a plain `git clone` of this repo fails three ways on the pack that still carries the 545 MB; --filter=blob:none works. And §6's tokio claim was challenged and survived -- every use is inside a #[cfg(test)] module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Rebased onto
885b4d4(the two docs commits):ba6c5da→25092f3, payloadhash-identical across the rebase, conflict-free as predicted since the file sets
are disjoint.
Re-verified from scratch rather than on the cache —
cargo clean --target wasm32-unknown-unknownfirst, then the exact CI command:exit 0, 163s, zeroerrors, with
getrandom v0.3.4andbevy_eguicompiling and nomioin thegraph at all.
Bundle:
/home/fabi/sylph-wasm-2026-09-07.bundle(base885b4d4, head25092f3, verifies okay). The/tmpone is stale — do not use it.Full file list, the scope question on
crates/sylpheed-formats/Cargo.toml, andthe
sylpheed-exportpin interaction are on #11:#11 (comment)
This does not turn the WASM job green —
Install Trunkstill selects an x86_64binary on an aarch64 runner. That remains a decision, not a fix.