fix: July 7 code-review remediation (M1–M3, L1–L3) #153

2026-07-07T18:09:21Z

funman300 commented

2026-07-07 18:09:21 +00:00

Fixes from today's full-project review, one commit per finding:

M2 fix(ci): cap test-build debuginfo (line-tables-only) — the runner was OOM-killing ld on the engine test binary (runs 447, 486)
M3 style: one mechanical cargo fmt under rustfmt 1.9 + a cargo fmt --check CI step so drift can't accumulate again
L2 fix(engine): user_theme_dir() no longer panics on a missing platform data dir — degrades to 'no user themes' with a one-shot warning (CLAUDE.md §2.3)
L3 fix(engine): difficulty seed cursor's Random arm returns a time seed instead of unreachable!
L1 docs(architecture): §8 documented a KlondikePile::Waste variant that doesn't exist upstream; states the real 'Stock = waste in pile-coordinate space' convention
M1 fix(data): token refreshes serialised behind a mutex with a stale-token double-check — overlapping 401 retries could spend the single-use rotated refresh token twice and force a spurious re-login

Gates: fmt clean, clippy --workspace --all-targets -D warnings clean, 26/26 test suites green.

L4 (client-side sanity bounds on server payloads) deliberately not addressed per review decision.

🤖 Generated with Claude Code

Fixes from today's full-project review, one commit per finding: - **M2** `fix(ci)`: cap test-build debuginfo (`line-tables-only`) — the runner was OOM-killing `ld` on the engine test binary (runs 447, 486) - **M3** `style`: one mechanical `cargo fmt` under rustfmt 1.9 + a `cargo fmt --check` CI step so drift can't accumulate again - **L2** `fix(engine)`: `user_theme_dir()` no longer panics on a missing platform data dir — degrades to 'no user themes' with a one-shot warning (CLAUDE.md §2.3) - **L3** `fix(engine)`: difficulty seed cursor's `Random` arm returns a time seed instead of `unreachable!` - **L1** `docs(architecture)`: §8 documented a `KlondikePile::Waste` variant that doesn't exist upstream; states the real 'Stock = waste in pile-coordinate space' convention - **M1** `fix(data)`: token refreshes serialised behind a mutex with a stale-token double-check — overlapping 401 retries could spend the single-use rotated refresh token twice and force a spurious re-login Gates: fmt clean, `clippy --workspace --all-targets -D warnings` clean, 26/26 test suites green. L4 (client-side sanity bounds on server payloads) deliberately not addressed per review decision. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

funman300 added 6 commits 2026-07-07 18:09:21 +00:00

fix(ci): cap test-build debuginfo so the linker fits runner memory 18bb1fa0be

Two runs (447, 486) died with 'ld terminated with signal 9' linking the
solitaire_engine test binary — the runner OOMs on full dev debuginfo for
the Bevy dependency graph. line-tables-only preserves file:line panic
backtraces at a fraction of the link footprint.

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

style: cargo fmt under rustfmt 1.9 and gate formatting in CI 113a933170

The repo was formatted under an older stable; rustfmt 1.9 (Rust 1.95)
wraps signatures and call sites differently, so every touched file was
picking up unrelated formatting hunks. One mechanical pass, and a
'cargo fmt --check' step in the test workflow (same pinned 1.95.0
toolchain) so drift can't accumulate again.

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

fix(engine): degrade gracefully when the platform data dir is missing 757c35e4a0

user_theme_dir() panicked on desktop when dirs::data_dir() returned
None (broken $HOME / $XDG_*). Return an empty path instead — exactly
what the wasm32 branch already did — so theme discovery reports 'no
user themes' and the bundled default keeps working. Warns once with
the set_user_theme_dir() workaround. CLAUDE.md §2.3: no panic! in
runtime logic.

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

fix(engine): remove latent unreachable! panic in difficulty seed cursor f61573513c

The Random arm was only unreachable because seeds_for(Random) returns
None in a different function — a future catalog change would turn it
into a shipped runtime panic. Returning a system-time seed is the
correct Random behaviour either way. CLAUDE.md §2.3.

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

docs(architecture): correct KlondikePile sketch — upstream has no Waste variant 8b09c51271

§8 documented a Waste variant that doesn't exist in klondike 0.4; the
real convention (Stock denotes the waste pile in pile-coordinate space)
is now stated where the enum is sketched.

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

fix(data): serialise token refreshes so overlapping 401s can't force a logout

Test / test (pull_request) Successful in 15m33s

Details

abf1312cf5

Refresh tokens are single-use since the rotation change (PR #136). Two
in-flight requests hitting 401 together (replay upload racing a manual
sync) both called /api/auth/refresh; the second presented an already-
consumed token, got rejected, and surfaced as a spurious 'session
expired' re-login prompt.

refresh_token() now takes the stale access token that earned the 401
and runs behind a tokio::sync::Mutex; a loser of the lock race that
finds the stored token already rotated returns Ok and retries with it
instead of spending the new refresh token.

Untested caveat: exercising the race in a unit test needs an in-memory
auth_tokens double (the real keyring is unavailable on headless Linux
runners) — noted for a future test-support addition.

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

funman300 added 1 commit 2026-07-07 19:25:37 +00:00

fix(engine): unify light/dark theme manifest URL resolution

Test / test (pull_request) Successful in 13m4s

Details

018b69285d

- set_theme resolved every id to themes:// — the bundled dark/classic
  themes live at embedded://, so switching to them via the public API
  would NotFound and silently keep the old theme. All three load paths
  now share one theme_manifest_url resolver.
- load_initial_theme's settings-absent fallback was "classic", a
  leftover from v0.33 when classic was the default; it now derives from
  Settings::default() so it can't drift from the data crate again, with
  a test pinning the default id to a bundled theme.
- settings.rs doc no longer claims "classic" is migrated to "dark";
  only the pre-rename "default" id is rewritten.

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

funman300 referenced this pull request

2026-07-07 20:13:05 +00:00

feat: in-game theme store — server catalog + verified downloads + install UI #154

funman300 merged commit a80547c514 into master

2026-07-07 20:16:20 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: funman300/Ferrous-Solitaire#153