Skip to content

chore(desktop-backend): scope the unwrap_used deny to production code#10128

Open
aryanorastar wants to merge 4 commits into
BasedHardware:mainfrom
aryanorastar:chore/rust-test-clippy-clean
Open

chore(desktop-backend): scope the unwrap_used deny to production code#10128
aryanorastar wants to merge 4 commits into
BasedHardware:mainfrom
aryanorastar:chore/rust-test-clippy-clean

Conversation

@aryanorastar

@aryanorastar aryanorastar commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

cargo clippy --all-targets on the desktop Rust backend reported 275 unwrap_used errors — every single one inside #[cfg(test)] code (proxy tests 176, chat tests 91, scattered singles). The crate-level unwrap_used = "deny" predates any --all-targets run: CI's plain cargo clippy never lints test targets, so the debt was invisible in CI while screaming in every contributor's editor — rust-analyzer runs clippy with --all-targets by default, burying real diagnostics under 275 false alarms.

Fix: each test module carries an inner #![allow(clippy::unwrap_used)] with a one-line rationale. Production code keeps the crate-level deny untouched.

Named as out-of-scope debt in #10101; this closes it.

Why allow, not 275 expect()s

Tests may unwrap — a test failing on unwrap is the test doing its job, and the codebase's test authors clearly practice that (#9851's chat split merged with 91 of them). Rewriting 275 call sites into expect("...") messages nobody reads is churn that fights the practiced convention; scoping the deny to the code it was written for matches intent with five attribute lines.

Product invariants affected

none (verified with scripts/pr-preflight --suggest)

Validation

  • cargo clippy --all-targets0 errors (was 275).
  • Plain cargo clippy → unchanged (was already clean; production deny still active — a production unwrap() still fails the build).
  • cargo test376 passed.
  • rustfmt --edition 2021 applied; make preflight (local lane, this PR body) → all selected checks pass.
  • proxy.rs frozen-size baseline raised 2803 → 2806 (+3, the test-module allow attribute) with justification, per the ratchet's documented raise path.

Out of scope, named

cargo clippy --all-targets reported 275 unwrap_used errors, every one
inside #[cfg(test)] code (proxy 176, chat tests 91, and scattered
singles): the crate-level deny predates any --all-targets run, and CI's
plain cargo clippy never lints test targets, so the debt was invisible
in CI while screaming in every contributor's editor — rust-analyzer runs
clippy with --all-targets by default, burying real diagnostics under 275
false alarms.

Tests may unwrap: a test failing on unwrap is the test doing its job,
and the test authors (including BasedHardware#9851's chat split, merged with these
unwraps) clearly practice that. Each test module now carries an inner
allow with that rationale; production code keeps the crate-level deny
untouched.

Verification: cargo clippy --all-targets → 0 errors (was 275);
cargo test → 376 passed; plain cargo clippy unchanged (was already
clean). BasedHardware#10101's new services::http test module carries its own 4
unwraps and gets the same allow when the branches meet.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Git-on-my-level Git-on-my-level added workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior candidate-clean-pr Small, safe, well-scoped, well-tested contribution; model clean PR labels Jul 20, 2026
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks for the focused cleanup here. Scoping clippy::unwrap_used allows to the #[cfg(test)] modules keeps the production deny in place while removing the --all-targets/rust-analyzer noise from existing tests, and the touched Rust files look limited to test-module attributes plus the ratchet baseline justification.

I’m leaving this as a positive signal rather than a formal approval because this run is gated by the workflow/tooling-sensitive ratchet-baseline path and the PR should still have maintainer sign-off on the lint policy choice before merge. The key thing I’d want a maintainer to verify is that the latest CI/local validation matches the stated cargo clippy --all-targets and cargo test results.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

@Git-on-my-level I've pushed a fix for the pre-existing Backend unit suite timebomb that was failing CI. CI should be green shortly. Thanks for the review!

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Confirmed on this head: cargo clippy --all-targets → 0 errors (was 275), plain cargo clippy unchanged (the production unwrap_used deny stays active — a production unwrap() still fails the build), cargo test → 376 passed. Backend unit suite and Rust Backend both green in CI here. Thanks for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

candidate-clean-pr Small, safe, well-scoped, well-tested contribution; model clean PR workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants