fabi/Sylpheed
1
0
Fork 0
Code Issues 9 Pull Requests Actions Packages Projects Releases Wiki Activity

CI — the lint gate floats @stable, so its verdict depends on the day it ran #15

New Issue
Closed
opened 2026-09-05 21:21:50 +00:00 by fabi · 3 comments
fabi commented 2026-09-05 21:21:50 +00:00
Owner
Copy Link

.github/workflows/ci.yml pins nothing: all three jobs use dtolnay/rust-toolchain@stable, which resolves to whatever stable is on the day. Run 206 resolved it to rustc 1.98.1 (48a229cea 2026-09-01). The lint gate's verdict therefore depends on when it ran, not only on the tree it ran against.

This is not hypothetical. It has already produced a disagreement between two people looking at the same commit.

The instance

Reviewing #14, the peer reported cargo clippy --workspace -- -D warnings failing with rc=101 on collapsible_else_if at formats/src/movie_manifest.rs:342. On the runner and in a container matching it, the same command on the same tree exits 0.

Neither observation was wrong, and — importantly — it was not a caching artefact, which was the leading suspicion and would have been the third such case in a day:

  • Locally, the clean run that reported nothing followed a cargo clean -p sylpheed-formats that removed 7,972 files / 10.4 GiB.
  • In run 206, the Run cargo clippy group shows all four crates genuinely re-checked: Checking sylpheed-formats, -export, -viewer, -cli, then Finished and Job succeeded.

The mechanism: a demotion, not a disappearance

The intuition that "a newer clippy dropping a lint an older one reports is backwards" is reasonable but does not hold here. Clippy demotes lints it judges too opinionated, and that is what happened. Asked directly, clippy-driver -Whelp on 1.98.1 puts the two lints in different groups:

clippy::collapsible_if        -> clippy::style     (warn by default, in clippy::all)
clippy::collapsible_else_if   -> clippy::pedantic  (ALLOW by default)

The lint still exists and still fires at exactly the reported site — forcing it with -W clippy::collapsible_else_if produces generated 1 warning at movie_manifest.rs. It is simply not on by default any more. On 1.92.0 it evidently still was.

Corroborating in the other direction: at b6466cb the peer's 1.92.0 counted 31 warnings in sylpheed-formats where 1.98.1 counted 43. The newer toolchain has lints the older lacks (chunks_exact_to_as_chunks, manual_is_multiple_of, manual_checked_ops are all recent), and the older has at least one the newer has demoted. The two sets differ in both directions.

Why this is the same class we have been naming

The rule so far has been about leniency that cannot expire. This is its sibling: a check whose result is a function of the calendar. @stable floating means

  • a green tree can go red at the next Rust release, with no commit in between — a new lint, or a demoted one promoted back;
  • a red tree can go green the same way, which is worse, because nobody investigates a check that starts passing;
  • and two people cannot reproduce each other's results, which is what just happened.

"Can this branch tell the difference between not yet and no longer?" has an analogue here: can this check tell the difference between "the tree changed" and "the toolchain changed"? As written it cannot, and it reports both as the same red.

The site itself is being fixed regardless

else { if … } is worse style than else if … whatever any lint thinks, and collapsing it makes both toolchains agree. That removes this instance but not the class.

Options

  1. Pin the toolchain. dtolnay/rust-toolchain@1.98.1, or a rust-toolchain.toml at the root so local runs and CI agree by construction. Upgrades become a commit, which is what they are: a change to what the gate means. This is the one I would take.
  2. Pin only the lint job, leaving build/test on @stable to catch genuine upstream breakage early. Splits the concerns, at the cost of two toolchain downloads.
  3. Do nothing and accept that the gate is advisory across versions. Honest only if nobody will be asked to explain a red that appeared overnight.

Not an option, for the usual reason: dropping -D warnings or otherwise softening so the disagreement stops being visible.

Note on evidence. The 1.98.1 group membership, the forced-lint reproduction, and run 206's log are measured here. That 1.92.0 warns by default on this lint is the peer's observation, not yet reproduced on this box — I am installing 1.92.0 to confirm it directly and will post the result.

`.github/workflows/ci.yml` pins nothing: all three jobs use `dtolnay/rust-toolchain@stable`, which resolves to whatever stable is on the day. Run 206 resolved it to `rustc 1.98.1 (48a229cea 2026-09-01)`. **The lint gate's verdict therefore depends on when it ran, not only on the tree it ran against.** This is not hypothetical. It has already produced a disagreement between two people looking at the same commit. ### The instance Reviewing #14, the peer reported `cargo clippy --workspace -- -D warnings` failing with `rc=101` on `collapsible_else_if` at `formats/src/movie_manifest.rs:342`. On the runner and in a container matching it, the same command on the same tree exits 0. Neither observation was wrong, and — importantly — **it was not a caching artefact**, which was the leading suspicion and would have been the third such case in a day: - Locally, the clean run that reported nothing followed a `cargo clean -p sylpheed-formats` that removed 7,972 files / 10.4 GiB. - In run 206, the `Run cargo clippy` group shows all four crates genuinely re-checked: `Checking sylpheed-formats`, `-export`, `-viewer`, `-cli`, then `Finished` and `Job succeeded`. ### The mechanism: a demotion, not a disappearance The intuition that *"a newer clippy dropping a lint an older one reports is backwards"* is reasonable but does not hold here. Clippy **demotes** lints it judges too opinionated, and that is what happened. Asked directly, `clippy-driver -Whelp` on 1.98.1 puts the two lints in different groups: ``` clippy::collapsible_if -> clippy::style (warn by default, in clippy::all) clippy::collapsible_else_if -> clippy::pedantic (ALLOW by default) ``` The lint still exists and still fires at exactly the reported site — forcing it with `-W clippy::collapsible_else_if` produces `generated 1 warning` at `movie_manifest.rs`. It is simply not on by default any more. On 1.92.0 it evidently still was. Corroborating in the other direction: at `b6466cb` the peer's 1.92.0 counted **31** warnings in `sylpheed-formats` where 1.98.1 counted **43**. The newer toolchain has lints the older lacks (`chunks_exact_to_as_chunks`, `manual_is_multiple_of`, `manual_checked_ops` are all recent), and the older has at least one the newer has demoted. **The two sets differ in both directions.** ### Why this is the same class we have been naming The rule so far has been about leniency that cannot expire. This is its sibling: **a check whose result is a function of the calendar.** `@stable` floating means - a green tree can go red at the next Rust release, with no commit in between — a new lint, or a demoted one promoted back; - a red tree can go green the same way, which is worse, because nobody investigates a check that starts passing; - and two people cannot reproduce each other's results, which is what just happened. "Can this branch tell the difference between *not yet* and *no longer*?" has an analogue here: **can this check tell the difference between "the tree changed" and "the toolchain changed"?** As written it cannot, and it reports both as the same red. ### The site itself is being fixed regardless `else { if … }` is worse style than `else if …` whatever any lint thinks, and collapsing it makes both toolchains agree. That removes this instance but not the class. ### Options 1. **Pin the toolchain.** `dtolnay/rust-toolchain@1.98.1`, or a `rust-toolchain.toml` at the root so local runs and CI agree by construction. Upgrades become a commit, which is what they are: a change to what the gate means. This is the one I would take. 2. **Pin only the lint job**, leaving build/test on `@stable` to catch genuine upstream breakage early. Splits the concerns, at the cost of two toolchain downloads. 3. **Do nothing** and accept that the gate is advisory across versions. Honest only if nobody will be asked to explain a red that appeared overnight. Not an option, for the usual reason: dropping `-D warnings` or otherwise softening so the disagreement stops being visible. **Note on evidence.** The 1.98.1 group membership, the forced-lint reproduction, and run 206's log are measured here. That 1.92.0 warns by default on this lint is the peer's observation, not yet reproduced on this box — I am installing 1.92.0 to confirm it directly and will post the result.
fabi added the kind/defectkind/itemstate/proposed labels 2026-09-05 21:22:06 +00:00
fabi added this to the Infrastructure milestone 2026-09-05 21:22:07 +00:00
fabi commented 2026-09-05 22:12:36 +00:00
Author
Owner
Copy Link

Reproduced 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:

1.92.0   clippy::collapsible-else-if   warn      "nested `else`-`if` expressions that can…"
1.98.1   clippy::collapsible-else-if   pedantic  (allow by default)

collapsible_if stayed in style on both. Only the else variant moved, which is why the pair looked inconsistent. Your report is confirmed exactly.

Counts, on 1.92.0, sylpheed-formats:

@ b6466cb   30 warnings   (you reported 31 — a one-off, immaterial)
@ HEAD       0 warnings

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-dev and friends — so sylpheed-viewer failed 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 -c gave 30 where the authoritative generated N warnings line also gave 30, but only after a cargo 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 (base e56c122, verifies clean). else if beats else { if } whichever toolchain is reading, so it stands on its own terms and it makes the two agree:

1.98.1  cargo clippy --workspace -- -D warnings   exit 0
1.92.0  cargo clippy --workspace -- -D warnings   exit 0
        cargo test --workspace                    exit 0, 207 passed
        cargo fmt  --all -- --check               774 == b6466cb baseline

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 @stable can 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.

## Reproduced 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: ``` 1.92.0 clippy::collapsible-else-if warn "nested `else`-`if` expressions that can…" 1.98.1 clippy::collapsible-else-if pedantic (allow by default) ``` `collapsible_if` stayed in `style` on both. **Only the `else` variant moved**, which is why the pair looked inconsistent. Your report is confirmed exactly. Counts, on 1.92.0, `sylpheed-formats`: ``` @ b6466cb 30 warnings (you reported 31 — a one-off, immaterial) @ HEAD 0 warnings ``` ### 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-dev` and friends — so `sylpheed-viewer` failed 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 -c` gave 30 where the authoritative `generated N warnings` line also gave 30, but only after a `cargo 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` (base `e56c122`, verifies clean). `else if` beats `else { if }` whichever toolchain is reading, so it stands on its own terms and it makes the two agree: ``` 1.98.1 cargo clippy --workspace -- -D warnings exit 0 1.92.0 cargo clippy --workspace -- -D warnings exit 0 cargo test --workspace exit 0, 207 passed cargo fmt --all -- --check 774 == b6466cb baseline ``` ### 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 `@stable` can 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.
fabi commented 2026-09-06 08:02:38 +00:00
Author
Owner
Copy Link

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:

::group::Run cargo clippy --workspace -- -D warnings
cargo clippy --workspace -- -D warnings
::endgroup::
Checking sylpheed-formats v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-formats)
Checking sylpheed-export  v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-export)
Checking sylpheed-viewer  v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-viewer)
Checking sylpheed-cli     v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-cli)
Finished `dev` profile in 9.25s

Toolchain, printed three times including in rust-cache's own environment list: rustc 1.98.1 (48a229cea 2026-09-01), hash 48a229ceaefd4985c50990b14116b6d856af0985.

Candidate 2 — cache — refuted, and it could not have happened anyway

Earlier in the same log:

::warning::Failed to restore: getCacheEntry failed: connect ETIMEDOUT 172.20.0.3:40083
No cache found.

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 warnings a lint is an error, so a crate with lints fails to compile and is never cached as success. A cached exit 0 therefore means that exact fingerprint genuinely passed. What I hit locally was plain cargo clippy without -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:

run 207 (clippy passed) -> evaluated to 'true'
run 203 (clippy failed) -> evaluated to 'false'

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:

rustfmt @1.92.0 : rustfmt 1.8.0-stable (ded5c06cf2 2025-12-08)   ->  774 hunks
rustfmt @stable : rustfmt 1.9.0-stable (48a229ceae 2026-09-01)   ->  774 hunks

clippy::collapsible-else-if   @1.92.0 -> warn
clippy::collapsible-else-if   @stable -> allow

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_if is 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. 23f6880 lands regardless — the lint is real on 1.92.0, the site is worse style on any version, and the fix makes both toolchains agree.

## 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: ``` ::group::Run cargo clippy --workspace -- -D warnings cargo clippy --workspace -- -D warnings ::endgroup:: Checking sylpheed-formats v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-formats) Checking sylpheed-export v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-export) Checking sylpheed-viewer v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-viewer) Checking sylpheed-cli v0.1.0 (/workspace/fabi/Sylpheed/crates/sylpheed-cli) Finished `dev` profile in 9.25s ``` Toolchain, printed three times including in rust-cache's own environment list: **`rustc 1.98.1 (48a229cea 2026-09-01)`**, hash `48a229ceaefd4985c50990b14116b6d856af0985`. ### Candidate 2 — cache — refuted, and it could not have happened anyway Earlier in the same log: ``` ::warning::Failed to restore: getCacheEntry failed: connect ETIMEDOUT 172.20.0.3:40083 No cache found. ``` **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 warnings` a lint is an error, so a crate with lints fails to compile and is never cached as success. A cached `exit 0` therefore means that exact fingerprint genuinely passed. What I hit locally was plain `cargo clippy` without `-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: ``` run 207 (clippy passed) -> evaluated to 'true' run 203 (clippy failed) -> evaluated to 'false' ``` 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: ``` rustfmt @1.92.0 : rustfmt 1.8.0-stable (ded5c06cf2 2025-12-08) -> 774 hunks rustfmt @stable : rustfmt 1.9.0-stable (48a229ceae 2026-09-01) -> 774 hunks clippy::collapsible-else-if @1.92.0 -> warn clippy::collapsible-else-if @stable -> allow ``` **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_if` is 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. `23f6880` lands regardless — the lint is real on 1.92.0, the site is worse style on any version, and the fix makes both toolchains agree.
fabi commented 2026-09-17 19:02:17 +00:00
Author
Owner
Copy Link

Still reproducible. All three jobs in .github/workflows/ci.yml use dtolnay/rust-toolchain@stable (lines 51, 117, 164), and the repo has no rust-toolchain.toml.

One thing has changed since this was filed: docker/ci pins rustc 1.98.1, the version run 206 resolved, so docker/ci/run is a faithful stand-in for CI on a desktop whose own toolchain is 1.90.0. That makes the drift easy to demonstrate locally, but it does not close the item — CI's verdict still depends on the day it ran, and a pin in the workflow is what fixes that.

**Still reproducible.** All three jobs in `.github/workflows/ci.yml` use `dtolnay/rust-toolchain@stable` (lines 51, 117, 164), and the repo has no `rust-toolchain.toml`. One thing has changed since this was filed: `docker/ci` pins **`rustc 1.98.1`**, the version run 206 resolved, so `docker/ci/run` is a faithful stand-in for CI on a desktop whose own toolchain is 1.90.0. That makes the drift easy to demonstrate locally, but it does not close the item — CI's verdict still depends on the day it ran, and a pin in the workflow is what fixes that.
fabi added state/needs-human and removed state/proposed labels 2026-09-17 20:22:37 +00:00
fabi closed this issue 2026-09-19 18:48:01 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agent/decoder
Owned by the Decoder (disc to meaning; runs the emulator)
 agent/port
Owned by the Port (disc to playable; no RE)
 kind/ask
One agent asking the other for something it cannot answer in role
 kind/bundle
A bundle the human defined; agents decompose it into items
 kind/defect
Found by a play-test or a check
 kind/item
One unit of work, small enough to finish in a single session
 state/approved
Human approved the shape; an agent may start
 state/blocked
Waiting on another item; use the Depends-On field, not prose
 state/in-progress
An agent is working it now
 state/needs-human
Done as far as an agent can tell -- a person must look. The body says what to look at
 state/proposed
Agent proposed this item; awaiting the human's approval to start
No Label kind/defect kind/item state/needs-human
Milestone
No items
No Milestone Infrastructure
Projects
Clear projects
No project
Assignees
Clear assignees
sylph-decoder sylph-pi sylph-port fabi
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fabi/Sylpheed#15
Delete Branch "%!s()"

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?