Skip to content

Harden UCI/FEN parsing, fix engine bugs, upgrade toolchain and dependencies - #322

Merged
kirillbobyrev merged 3 commits into
mainfrom
claude/codebase-review-0hfehw
Jul 20, 2026
Merged

Harden UCI/FEN parsing, fix engine bugs, upgrade toolchain and dependencies#322
kirillbobyrev merged 3 commits into
mainfrom
claude/codebase-review-0hfehw

Conversation

@kirillbobyrev

Copy link
Copy Markdown
Owner

Summary

A codebase review pass: bug fixes in UCI/FEN handling and game-state tracking, plus a toolchain/dependency upgrade.

Bug fixes (fix: commit)

  • UCI go time controls were parsed as microseconds — the UCI protocol specifies milliseconds, so the engine would have been given 1000× less time than intended once time management lands.
  • Panics on untrusted UCI input, replaced with graceful errors:
    • non-ASCII move strings panicked on byte slicing (&uci[..2] on a multi-byte char boundary);
    • unknown promotion characters hit unreachable!();
    • e2e2-style moves tripped a debug assertion in Move::new;
    • setoption name Hash value (trailing value with no argument) panicked with an out-of-bounds index.
  • go argument scanning skipped two tokens after any unrecognized keyword, so valueless tokens (ponder, unknown extensions) swallowed the keyword that followed them (go ponder wtime 1000 lost wtime).
  • Unsound transmute in Move::promotion() — transmuting a raw u8 to Option<Promotion> relies on an unspecified niche layout; replaced with an explicit match.
  • FEN validation now rejects positions where the side not to move is in check (including adjacent kings), closing the "probe opposite checks" TODO. Such positions break move-generator invariants downstream.
  • Halfmove clock overflow: u8 incremented without bound panicked in debug builds past 255 plies without capture/pawn move; now saturates.
  • RepetitionTable::record returned false for the 4th+ occurrence of a position (== 3>= 3), which could un-flag a threefold draw.
  • Engine loop robustness: input read errors and invalid moves in position commands now propagate as errors instead of panic!/unreachable!().
  • Fuzz target fix: generate_moves called is_check() on a Result and did not compile.

Upgrades (chore: commit)

  • MSRV → 1.95 (required by shakmaty 0.30); the build workflow's MSRV job was pinned to 1.80.0, which cannot build the 2024 edition, and now matches rust-version.
  • itertools 0.13→0.15, rand 0.9→0.10 (+ rand_distr 0.5→0.6; small_rng is built-in now and random() moved to RngExt), shadow-rs 1→2, shakmaty 0.27→0.30, shakmaty-syzygy 0.25→0.28, indicatif 0.17→0.18, criterion 0.5→0.8, plus a full cargo update. Fuzz crate deps aligned.
  • Fixed the new clippy::useless_borrows_in_formatting perf lint (denied via clippy::perf) surfaced by clippy 1.97.

Testing

  • cargo test: 102 tests + doctests pass.
  • cargo test --release -- --ignored: all 12 expensive perft / differential tests against shakmaty pass (move generation exactness verified after the movegen-adjacent changes).
  • cargo clippy --all-targets clean (only pre-existing dead-code warnings in search stubs).
  • New regression tests: malformed UCI moves, setoption/go parsing edge cases, opposite-check and adjacent-king FEN rejection.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk


Generated by Claude Code

claude added 2 commits July 20, 2026 17:48
- Reject malformed UCI moves gracefully instead of panicking:
  non-ASCII input caused byte-slicing panics, unknown promotion pieces
  hit unreachable!(), and equal source/target squares tripped a
  debug assertion. Promotion now implements TryFrom<char>.
- Replace the unsound transmute in Move::promotion() with an explicit
  match: the niche layout of Option<Promotion> is not guaranteed.
- Parse UCI 'go' time controls as milliseconds (per the UCI spec)
  instead of microseconds.
- Fix out-of-bounds panic on 'setoption name Hash value' (trailing
  'value' with no argument).
- Fix 'go' argument scanning so that valueless tokens (ponder,
  infinite) no longer swallow the keyword that follows them.
- Propagate errors in the engine loop instead of panicking on input
  errors and unreachable!() on invalid moves in 'position' commands.
- Validate that the side not to move is not in check (also rejects
  adjacent kings) when parsing FEN, closing a long-standing TODO.
- Saturate the halfmove clock instead of overflowing u8 in long games.
- Report threefold repetition for four and more repetitions as well.
- Fix bit-rotted generate_moves fuzz target (is_check() was called on
  a Result and could not compile).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk
- Bump MSRV to 1.95 (required by shakmaty 0.30) and update the stale
  1.80.0 pin in the build workflow, which could not build the 2024
  edition anyway.
- itertools 0.13 -> 0.15, rand 0.9 -> 0.10 (small_rng feature is now
  built-in, random() moved to RngExt), rand_distr 0.5 -> 0.6,
  shadow-rs 1 -> 2, shakmaty 0.27 -> 0.30, shakmaty-syzygy
  0.25 -> 0.28, indicatif 0.17 -> 0.18, criterion 0.5 -> 0.8.
- Fix new clippy::useless_borrows_in_formatting perf lint (denied via
  clippy::perf) surfaced by the newer toolchain.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk
@kirillbobyrev
kirillbobyrev marked this pull request as ready for review July 20, 2026 17:51
Fixes the Lint CI job: cargo +nightly fmt --check flagged formatting
(mostly pre-existing) in position.rs and attacks.rs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GVrKvkMe4xDmkRAksacBzk
@kirillbobyrev
kirillbobyrev merged commit 065dd7d into main Jul 20, 2026
13 checks passed
@kirillbobyrev
kirillbobyrev deleted the claude/codebase-review-0hfehw branch July 20, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants