chesslib integration and improvements#7
Conversation
fixed some bugs implemented LMR and NMP, features, etc. Passed STC: -------------------------------------------------- Results of new vs base (10+0.1, NULL, 16MB, UHO_Lichess_4852_v1.epd): Elo: 51.67 +/- 8.40, nElo: 92.31 +/- 14.79 LOS: 100.00 %, DrawRatio: 47.36 %, PairsRatio: 2.82 Games: 2120, Wins: 517, Losses: 204, Draws: 1399, Points: 1216.5 (57.38 %) Ptnml(0-2): [8, 138, 502, 357, 55], WL/DD Ratio: 0.11 LLR: 2.95 (100.2%) (-2.94, 2.94) [0.00, 2.00] -------------------------------------------------- Passed LTC: -------------------------------------------------- Results of new vs base (60+0.6, NULL, 16MB, UHO_Lichess_4852_v1.epd): Elo: 108.69 +/- 10.86, nElo: 199.39 +/- 18.67 LOS: 100.00 %, DrawRatio: 41.80 %, PairsRatio: 12.34 Games: 1330, Wins: 461, Losses: 58, Draws: 811, Points: 866.5 (65.15 %) Ptnml(0-2): [1, 28, 278, 283, 75], WL/DD Ratio: 0.11 LLR: 2.95 (100.1%) (-2.94, 2.94) [0.00, 2.00] --------------------------------------------------
Passed STC (elo1=2): https://github.com/winapiadmin/cppchess_engine/actions/runs/28851470047 (mostly) pass LTC (elo1=2): https://github.com/winapiadmin/cppchess_engine/actions/runs/28864852436 Passed STC (elo1=5): https://github.com/winapiadmin/cppchess_engine/actions/runs/28876353922/job/85652602141 Passed LTC (elo1=5): https://github.com/winapiadmin/cppchess_engine/actions/runs/28876032178 Improvement since beginning of the PR: -------------------------------------------------- Results of new vs base (10+0.1, NULL, 64MB, UHO_Lichess_4852_v1.epd): Elo: 111.48 +/- 22.41, nElo: 152.89 +/- 28.67 LOS: 100.00 %, DrawRatio: 30.14 %, PairsRatio: 4.05 Games: 564, Wins: 256, Losses: 81, Draws: 227, Points: 369.5 (65.51 %) Ptnml(0-2): [4, 35, 85, 98, 60], WL/DD Ratio: 0.81 LLR: 2.95 (100.3%) (-2.94, 2.94) [0.00, 5.00] --------- Co-authored-by: GitHub Actions <actions@github.com>
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR updates build and CI files, then replaces the legacy chess engine code with new ChangesBuild System and CI Workflows
Engine Core Rewrite
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49cf63a595
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
CMakeLists.txt-9-18 (1)
9-18: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPin dependency tags to specific commits for reproducible builds.
GIT_TAG main(chesslib, tbprobe) andGIT_TAG master(csv-parser) make builds non-reproducible — upstream changes can silently alter build behavior or introduce breakage. Pin to specific commit SHAs or release tags instead.🔒 Proposed fix
FetchContent_Declare( chesslib GIT_REPOSITORY https://github.com/winapiadmin/chesslib.git - GIT_TAG main + GIT_TAG <pinned-commit-sha> ) FetchContent_Declare( tbprobe GIT_REPOSITORY https://github.com/winapiadmin/tb_probing_tool.git - GIT_TAG main + GIT_TAG <pinned-commit-sha> )FetchContent_Declare( csv GIT_REPOSITORY https://github.com/vincentlaucsb/csv-parser.git - GIT_TAG master + GIT_TAG <pinned-commit-sha> )Also applies to: 70-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 9 - 18, The FetchContent_Declare entries for chesslib and tbprobe still use moving branch references, which makes builds non-reproducible. Update the GIT_TAG values in the FetchContent blocks (including the csv-parser declaration mentioned in the review) to fixed commit SHAs or immutable release tags, and keep the existing dependency names and repository URLs unchanged so the build always resolves the same source revision..github/workflows/games.yml-31-32 (1)
31-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove unnecessary
contents: writepermission.This workflow only builds and tests — it never pushes commits or creates releases. The
contents: writepermission is overly broad and unnecessary. Remove it or reduce tocontents: read.🔒 Proposed fix
permissions: - contents: write + contents: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/games.yml around lines 31 - 32, The workflow permissions are too broad for a build/test-only job; update the permissions block in the games workflow to remove the write access or reduce the existing contents permission to read. Locate the top-level permissions configuration in games.yml and keep it minimal since no step in this workflow needs repository write access.Source: Linters/SAST tools
.github/workflows/games.yml-113-127 (1)
113-127: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPrevent shell injection from workflow inputs.
workflow_dispatchinputs (output_exec,tc,rounds,elo0,elo1) are interpolated directly into shell commands via${{ github.event.inputs.* }}. While only users with write access can trigger the workflow, a crafted input could execute arbitrary commands in the CI environment, which has access to repository secrets. Pass these values through environment variables instead.🔒 Proposed fix
- name: Run fastchess + env: + OUTPUT_EXEC: ${{ github.event.inputs.output_exec }} + TC: ${{ github.event.inputs.tc }} + ROUNDS: ${{ github.event.inputs.rounds }} + ELO0: ${{ github.event.inputs.elo0 }} + ELO1: ${{ github.event.inputs.elo1 }} run: | chmod +x new-engine/engine - chmod +x base-engine/${{ github.event.inputs.output_exec }} + chmod +x "base-engine/$OUTPUT_EXEC" ./fastchess -recover \ -engine cmd=new-engine/engine name=new \ - -engine cmd=base-engine/${{ github.event.inputs.output_exec }} name=base \ + -engine cmd="base-engine/$OUTPUT_EXEC" name=base \ -openings file=UHO_Lichess_4852_v1.epd format=epd order=random \ - -each tc=${{ github.event.inputs.tc }} \ - -rounds ${{ github.event.inputs.rounds }} \ + -each tc="$TC" \ + -rounds "$ROUNDS" \ -concurrency $(nproc) \ -pgnout notation=san nodes=true file=games.pgn \ -repeat \ - -sprt elo0=${{ github.event.inputs.elo0 }} elo1=${{ github.event.inputs.elo1 }} alpha=0.05 beta=0.05 | tee results.txt + -sprt elo0="$ELO0" elo1="$ELO1" alpha=0.05 beta=0.05 | tee results.txt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/games.yml around lines 113 - 127, The Run fastchess workflow step is interpolating workflow_dispatch inputs directly into the shell command, which creates a shell injection risk. Update the fastchess invocation to read output_exec, tc, rounds, elo0, and elo1 from environment variables instead of embedding github.event.inputs.* inline, and reference the existing Run fastchess step plus the chmod +x setup that uses output_exec so the values are safely passed through the shell.Source: Linters/SAST tools
uci.cpp-88-93 (1)
88-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t consume later
golimits assearchmoves.After
searchmoves, every remaining token is pushed as a move. Inputs likego searchmoves e2e4 wtime 10000 btime 10000lose the clock limits and treatwtime/10000as moves, which can cause unintended long searches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uci.cpp` around lines 88 - 93, The UCI parser in the searchmoves handling is consuming every remaining token, so later go limits like wtime and btime are being misclassified as moves. Update the parsing logic in uci.cpp around the searchmoves loop to stop collecting moves once a recognized go limit token is encountered, or otherwise parse searchmoves only until the next option keyword, and keep the rest of the tokens available for the existing go-limit fields in the same parser.uci.cpp-138-140 (1)
138-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRound negative mate plies away from zero.
With C++ integer truncation,
mate.plies == -1formats asmate 0. Use the symmetric adjustment for negative plies.Proposed fix
[](Score::Mate mate) -> std::string { - auto m = (mate.plies > 0 ? (mate.plies + 1) : mate.plies) / 2; + auto m = (mate.plies > 0 ? mate.plies + 1 : mate.plies - 1) / 2; return std::string("mate ") + std::to_string(m); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uci.cpp` around lines 138 - 140, The mate formatting in the `overload` lambda inside `uci.cpp` currently uses integer division that truncates toward zero, so negative values like `Score::Mate{-1}` become `mate 0`. Update the `mate.plies` to move symmetrically away from zero before dividing by 2, using the same logic for both positive and negative plies, so the `std::to_string(m)` output rounds correctly for negative mate distances.ucioption.cpp-123-129 (1)
123-129: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject invalid spin values without throwing.
std::stoi(v)throws for non-numeric or out-of-range input, so a command likesetoption name Hash value abccan terminate the engine instead of being ignored.Proposed fix
Option &Option::operator=(const std::string &v) { assert(!type.empty()); + if (type == "spin") { + int parsed = 0; + std::size_t parsedChars = 0; + try { + parsed = std::stoi(v, &parsedChars); + } catch (...) { + return *this; + } + + if (parsedChars != v.size() || parsed < min || parsed > max) + return *this; + } + if ((type != "button" && type != "string" && v.empty()) || (type == "check" && v != "true" && v != "false") || - (type == "spin" && (std::stoi(v) < min || std::stoi(v) > max))) + false) return *this;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ucioption.cpp` around lines 123 - 129, In Option::operator=, the spin-value validation currently calls std::stoi(v) directly, which can throw on non-numeric or out-of-range input. Update the "spin" branch to validate the string safely before converting, or wrap the conversion in error handling so invalid values are simply rejected and the function returns without changing the option. Keep the existing behavior for button/string/check types intact.uci.cpp-219-292 (1)
219-292: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t stop search before dispatching every command.
Line 291 aborts and joins the search for any incoming command, so
isready,uci, or debug commands during search prematurely end the search. Movestop()into the commands that actually require it.Proposed fix
} else if (token == "ucinewgame") { + stop(); search::tt.clear(); break; } else if (token == "stop") { + stop(); break; } else if (token == "quit") { quit = true; return; @@ while (!quit && std::getline(std::cin, line)) { - stop(); execCmd(line); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uci.cpp` around lines 219 - 292, The command dispatcher in engine::loop currently calls stop() before every execCmd invocation, which incorrectly aborts search for non-interrupting commands like uci, isready, and debug/query handlers. Update the flow so stop() is only invoked inside the specific execCmd branches that must terminate or alter an active search (for example stop, quit, setoption, position-changing commands), and leave harmless commands to run without stopping the current search.tune_cmd.cpp-907-915 (1)
907-915: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftKeep reservoir memory bounded by
max_pos.Replacement appends the new FEN to
fen_bufand only updates the offset, leaving every replaced FEN in memory. Large training CSVs will grow memory with rows seen instead of sampled positions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 907 - 915, The reservoir replacement path in tune_cmd.cpp keeps growing fen_buf because it appends every newly selected FEN and only changes the offset. Update the replacement logic in the reservoir sampling branch so memory stays bounded by max_pos, either by reusing storage for the replaced sample or by avoiding retaining old FEN strings beyond the sampled set. Focus on the sampling code around the all.offsets/all.fen_buf/all.results updates and ensure the buffer size tracks only the current reservoir.tune_cmd.cpp-712-721 (1)
712-721: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInitialize
best_lossfrom holdout loss, not train loss.The loop compares
hold_loss < best_loss, butbest_lossstarts asbase_lossfrom the training set. If initial holdout loss is above train loss, later holdout improvements may never be saved.Proposed fix
double base_loss = eval_loss(all, train_idx); + double base_hold_loss = eval_loss(all, holdout_idx); std::cout << "info string texel_tune: dim=" << dim << " positions=" << n << " initial_loss=" << base_loss * train_idx.size() << std::endl; @@ - double best_loss = base_loss; + double best_loss = base_hold_loss;Also applies to: 852-858
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 712 - 721, The tuning loop in `tune_cmd.cpp` initializes `best_loss` from `base_loss`, but `best_loss` is compared against `hold_loss` later, so it should be seeded with the holdout loss instead of the training loss. Update the initialization near the `eval_loss(all, train_idx)` / `best_x` setup to compute the corresponding holdout loss first and assign that to `best_loss`, and apply the same change in the duplicated block referenced by the comment. Keep the existing `best_x` initialization unchanged, but ensure the initial best score always matches the metric used in the `hold_loss < best_loss` check.tune_cmd.cpp-352-378 (1)
352-378: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply phase factors to split MG/EG gradients.
For separate MG/EG weights, the derivative must include
phase_mgorphase_eg. Rook file, pawn-structure, passed-pawn, and king-shelter gradients currently update both phases at full strength.Example fix pattern
- gradient[it_mg->second] += common_factor * eff; + gradient[it_mg->second] += common_factor * eff * phase_mg; - gradient[it_eg->second] += common_factor * eff; + gradient[it_eg->second] += common_factor * eff * phase_eg;Also applies to: 407-459, 497-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 352 - 378, The gradient updates for split MG/EG weights in tune_cmd.cpp are missing phase scaling, so fix the rook file logic in the shown loop and the related passed-pawn and king-shelter sections by multiplying each MG update by phase_mg and each EG update by phase_eg. Use the existing symbols like rookOpenFileMg, rookOpenFileEg, rookSemiOpenFileMg, rookSemiOpenFileEg, and the corresponding gradient writes to ensure both phases are updated with their own phase factor instead of full strength.eval.cpp-695-714 (1)
695-714: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the engine/library insufficient-material check here too.
eval_components()only zeros KBKB/KNNK, so KK, KBK, and KNK can still get PST/material scores wheneval::eval()is called directly. This leaks into the max-ply fallback in search and into tuning loss evaluation.Proposed fix
// Draw detection: score 0 for positions where neither side can force a win + if (board.is_insufficient_material()) + return { 0, 0, 0 }; + int totalPieces = popcount(board.occ());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eval.cpp` around lines 695 - 714, Update eval_components() to rely on the engine/library’s insufficient-material detection instead of only special-casing KBKB and KNNK. The issue is that KK, KBK, and KNK can still accumulate PST/material terms when eval::eval() is called directly, so add the shared insufficient-material check in eval_components() and ensure it returns a zeroed evaluation for all drawn endgames it covers. Use the existing eval::eval() / eval_components() path and the board material helpers already present to keep the logic centralized.tune_cmd.cpp-743-746 (1)
743-746: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep tuning’s score calculation identical to
eval::eval().This duplicated formula misses
eval::eval()’smg == 0 && eg == 0early return, so draw/equal-component positions are trained astempoinstead of0. That also makestrain_lossinside the loop differ fromeval_loss().Proposed fix
auto comp = eval::eval_components(pos); const int sign = pos.side_to_move() == chess::WHITE ? 1 : -1; - Value score = (((comp.mg * comp.phase) + (comp.eg * (256 - comp.phase))) * sign) / 256 + eval::tempo; + Value score = (comp.mg == 0 && comp.eg == 0) + ? 0 + : (((comp.mg * comp.phase) + (comp.eg * (256 - comp.phase))) * sign) / 256 + eval::tempo;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune_cmd.cpp` around lines 743 - 746, The tuning score formula in tune_cmd.cpp must stay identical to eval::eval() and currently skips its mg == 0 && eg == 0 early return. Update the score calculation in the tuning loop to use the same logic as eval::eval() (or call the same helper) so equal-component positions evaluate to 0 instead of tempo, which will also keep train_loss aligned with eval_loss(). Use the existing eval::eval() and eval::eval_components() paths as the reference points when fixing the duplicated calculation.eval.cpp-31-228 (1)
31-228: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-enable the tuning registrations
This entireTUNE(...)block is commented out, so these eval weights never reachTune::get_list()andtune_cmd.cpphas nothing to export or tune. Move the registrations back into active code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eval.cpp` around lines 31 - 228, The eval tuning registrations are disabled because the entire TUNE block in eval.cpp is commented out, so Tune::get_list() and the tuning/export path in tune_cmd.cpp never see these parameters. Re-enable this block as active code in the evaluation setup so the existing TUNE entries for the eval weight symbols are registered again and available to the tuner.score.cpp-14-15 (1)
14-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle negative mate plies symmetrically in
uci.cpp:139score.cppcan produceMate{-1}, and the current formatter turns that intomate 0because negative odd plies truncate toward zero.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@score.cpp` around lines 14 - 15, The mate score formatting path is handling negative mate plies incorrectly, causing values like Mate{-1} to be rendered as mate 0. Update the formatter logic in the UCI score conversion path to treat positive and negative mate distances symmetrically, using the mate score symbol(s) involved in score.cpp and the formatter in uci.cpp so negative odd plies preserve the correct sign and count.timeman.h-16-18 (1)
16-18: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInitialize
startTimeinLimitsType.The constructor explicitly initializes the time-control fields but leaves
startTimeindeterminate; default-constructed limits outsideparse_limits()can feed undefined data intoTimeManagement::init().Proposed fix
LimitsType() { - time[chess::WHITE] = time[chess::BLACK] = inc[chess::WHITE] = inc[chess::BLACK] = movetime = TimePoint(0); + time[chess::WHITE] = time[chess::BLACK] = inc[chess::WHITE] = inc[chess::BLACK] = movetime = startTime = + TimePoint(0); movestogo = mate = perft = infinite = 0;Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@timeman.h` around lines 16 - 18, `LimitsType` leaves `startTime` uninitialized in its constructor, which can propagate undefined data into `TimeManagement::init()` when limits are default-constructed. Update the `LimitsType()` initialization in `timeman.h` to explicitly set `startTime` to a safe default alongside the other time-control fields, and ensure any other constructor or initializer that affects `LimitsType` follows the same pattern.timeman.cpp-79-81 (1)
79-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClamp computed budgets after overhead subtraction.
Line 81 can make
maximumTimenegative on very small clocks or largeMove Overhead, which makes hard-stop checks fire immediately or before the soft budget.Proposed fix
// Limit the maximum possible time for this move - optimumTime = TimePoint(optScale * timeLeft); - maximumTime = TimePoint(std::min(0.825179 * time - moveOverhead, maxScale * optimumTime)) - 10; + optimumTime = std::max(TimePoint(1), TimePoint(optScale * timeLeft)); + const TimePoint maxCandidate = TimePoint(std::min(0.825179 * time - moveOverhead, maxScale * optimumTime)) - 10; + maximumTime = std::max(optimumTime, maxCandidate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@timeman.cpp` around lines 79 - 81, The move budget calculation in the time management logic can go negative after subtracting overhead, so clamp the result to zero (or the minimum valid TimePoint) before assigning to maximumTime in timeman.cpp. Update the computation around optimumTime/maximumTime so the overhead subtraction cannot produce a negative hard limit on very small time controls or large Move Overhead values.timeman.cpp-26-33 (1)
26-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLet
movetimeoverride base-clock fields.Line 26 ignores
movetimewhenevertime[us]is also present, so a GUI command carrying both can fall into heuristic clock allocation instead of searching for the requested exact duration.Proposed fix
- if (limits.movetime != 0 && limits.time[us] == 0) { + if (limits.movetime != 0) { optimumTime = maximumTime = TimePoint(limits.movetime); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@timeman.cpp` around lines 26 - 33, The time-control handling in the logic around the movetime and base-clock checks should treat movetime as the highest priority override, even when limits.time[us] is also set. Update the condition sequence so the movetime case in the time-calculation code path is handled before any heuristic clock-allocation branch, ensuring a command with both fields still uses the exact requested duration; keep the fallback infinite-time case and other timing logic unchanged.tt.h-2-4 (1)
2-4: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude the standard headers this public header uses
tt.husesstd::nothrow,std::bad_alloc, and fixed-width types, but only includes<algorithm>,<cstring>, and<types.h>. Add the standard headers directly so the header stays self-contained.Proposed fix
`#pragma` once `#include` <algorithm> +#include <cstddef> +#include <cstdint> `#include` <cstring> +#include <new> `#include` <types.h>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tt.h` around lines 2 - 4, The public header tt.h is not self-contained because it relies on standard library and fixed-width types without including the proper standard headers. Update the includes at the top of tt.h so the symbols used by the header, including std::nothrow, std::bad_alloc, and fixed-width integer types, are declared directly from the standard headers rather than depending on transitive includes or non-standard headers; keep the change localized to the header’s existing include block.search.cpp-661-663 (1)
661-663: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the TT fallback move before reporting it as
bestmove.This path emits the TT move without checking root legality or the active
searchmovesfilter. If the stored move is stale/invalid for the root constraints, UCI can receive an illegalbestmove; reuse the legality check already used inextract_pvbefore reporting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@search.cpp` around lines 661 - 663, The TT fallback in search.cpp reports a stored move from tt.lookup via report without validating it against the root position or the active searchmoves constraints. Update the logic around the TTEntry/getMove path to reuse the same legality filtering used in extract_pv before emitting bestmove, and only report the move if it is legal for the current root board state and allowed by the current search filter.search.cpp-618-655 (1)
618-655: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReport a completed depth before breaking on time.
When
doSearchreturns a validscore_but time is now overoptimum()/maximum(), Lines 619-621 break before updatinglastPVand before emitting the completed-depth info. That can leavebestmovestuck on the previous iteration or the initial qsearch fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@search.cpp` around lines 618 - 655, In search.cpp’s doSearch iteration, the time-limit check is happening before the depth’s best PV and InfoFull report are finalized, so a completed score can be discarded and bestmove may stay stale. Reorder the logic around the score_/time check so the current iteration still updates lastPV from info.pv and calls report(info) for the completed depth before breaking; use the existing doSearch, lastPV, extract_pv, and report symbols to place the fix.movepick.cpp-82-92 (1)
82-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep SEE tied to the hypothetical occupancy
stmAttackersshould useocc, notboard.occ(stm), and the removed square should be cleared, not toggled. As written, a captured blocker can stay eligible for selection again, which can skew SEE and the capture pruning that depends on it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@movepick.cpp` around lines 82 - 92, The SEE loop in movepick.cpp is still using the live board occupancy instead of the hypothetical occupancy, which can let already-removed blockers be considered again. Update the attacker filter in the SEE routine to use the local occ state, and when removing the chosen square in the loop, clear that bit explicitly rather than toggling it so the simulation stays consistent. Keep the fix centered on the SEE/least_valuable_attacker flow that recomputes attackers after each removal.
🟡 Minor comments (8)
.github/workflows/games.yml-40-42 (1)
40-42: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSet
persist-credentials: falseon checkout steps.Neither build job pushes back to the repo, so the
GITHUB_TOKENshould not be persisted in.git/config. This reduces the risk of credential leakage through artifact or log exposure.🔒 Proposed fix
- uses: actions/checkout@v4 with: + persist-credentials: false path: new- uses: actions/checkout@v4 with: + persist-credentials: false ref: ${{ github.event.inputs.base_ref }} path: baseAlso applies to: 66-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/games.yml around lines 40 - 42, The checkout steps in the workflow are persisting the default token unnecessarily. Update each actions/checkout invocation in the game build jobs (the checkout step using the path new and the other checkout step referenced by the comment) to set persist-credentials to false so GITHUB_TOKEN is not written into .git/config; use the actions/checkout step configuration itself to make the change.Source: Linters/SAST tools
CMakeLists.txt-44-49 (1)
44-49: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
git rev-parsefailure gracefully.If the build runs outside a git repository (e.g., a downloaded tarball),
git rev-parse --short HEADwill fail. Unlike thegit describecall on line 55, this command lacksERROR_QUIET, so it will print an error to stderr. More importantly,GIT_SHAwill be empty, producing aBUILD_VERSIONof"debug-". AddERROR_QUIETand a fallback.🛡️ Proposed fix
execute_process( COMMAND git rev-parse --short HEAD WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} OUTPUT_VARIABLE GIT_SHA + ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE ) +if(GIT_SHA STREQUAL "") + set(GIT_SHA "unknown") +endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 44 - 49, Handle the `execute_process` call that populates `GIT_SHA` so it fails quietly outside a git repo and still produces a usable version string. Add `ERROR_QUIET` to the `git rev-parse --short HEAD` invocation, then add a fallback in the same `GIT_SHA`/`BUILD_VERSION` flow so an empty result does not yield `debug-` (for example, substitute a default revision or reuse the existing `git describe` fallback pattern).Makefile-30-34 (1)
30-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPin the dependency revisions and source list
git clonepulls the latest upstream HEAD, so the build can drift over time. Thedeps/chesslib/*.cppglob also picks up chesslib test sources, so make the dependency revisions and source files explicit to keep the Makefile reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 30 - 34, The Makefile dependency setup is non-reproducible because deps clones the latest upstream HEAD and SRCS uses broad globs that may include unwanted dependency test files. Update the deps target to fetch fixed revisions for chesslib and tbprobe instead of floating clones, and replace the deps/chesslib/*.cpp and deps/tbprobe/*.cpp patterns in SRCS with an explicit source list of only the required production files. Keep the changes localized around deps and SRCS so the build stays stable over time.ucioption.cpp-66-74 (1)
66-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep insertion order per
OptionsMapinstance.Line 68 uses a function-local static counter shared by every map. Temporary maps created for combo validation can advance it, and later options may get
idx >= options_map.size(), sooperator<<won’t print them.Proposed fix
void OptionsMap::add(const std::string &name, const Option &option) { if (!options_map.count(name)) { - static size_t insert_order = 0; - options_map[name] = option; options_map[name].parent = this; - options_map[name].idx = insert_order++; + options_map[name].idx = options_map.size() - 1; } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ucioption.cpp` around lines 66 - 74, The insertion index in OptionsMap::add is being tracked with a function-local static counter, so it is shared across all OptionsMap instances and can drift when temporary maps are created. Change the order tracking to be per-instance state on OptionsMap (or derive it from the current map size/entry count) so each map assigns contiguous idx values independently, and make sure operator<< continues to iterate over all inserted options using those per-map indices.main.cpp-14-14 (1)
14-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefine
BUILD_VERSIONin the Makefile build path.CMakeLists.txtsets it, butMakefilecompilesmain.cppdirectly without any-D BUILD_VERSION=..., somakewill fail on this line. Add a Makefile definition or a source fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.cpp` at line 14, The version print in main.cpp depends on BUILD_VERSION, but the Makefile build path does not define it, so make will fail there. Update the Makefile to pass a BUILD_VERSION definition during compilation, or add a safe fallback in main.cpp so the BUILD_VERSION reference in the main entrypoint always resolves regardless of build system.score.h-42-52 (1)
42-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize
Scoreto internal units
Score{}default-constructs the firststd::variantalternative, so it starts asMate{0}and can format asmate 0. If that isn’t intended, initializescoretoInternalUnits{VALUE_ZERO}or remove the default constructor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@score.h` around lines 42 - 52, The default construction of Score currently leaves the std::variant score as its first alternative, which means Score() behaves like Mate{0} instead of internal units. Update Score’s default initialization in the Score class so score starts as InternalUnits with VALUE_ZERO, or remove the default constructor if an uninitialized/default Score should not exist; keep the existing Score(Value v), is(), get(), and visit() behavior unchanged.tt.cpp-33-45 (1)
33-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an
intfor TT depth and clamp before packing.
int8_t depthcan wrap for deeper searches, so replacement decisions and stored depth can become wrong once the requested depth exceeds 127.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tt.cpp` around lines 33 - 45, The TranspositionTable::store depth handling currently uses int8_t, which can wrap for deep searches and break replacement logic; change the depth parameter in TranspositionTable::store to int, and clamp it to the valid packed range right before calling TTEntry::setPackedFields. Make sure any depth comparison against TTEntry::getDepth() still uses the unclamped int value, and keep the stored packed depth bounded so entries remain correct for depths above 127.timeman.h-2-4 (1)
2-4: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
<string>totimeman.hsearchmovesusesstd::string, so this header should include<string>directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@timeman.h` around lines 2 - 4, timeman.h is missing a direct include for std::string, which makes the header rely on indirect includes. Update the includes in timeman.h to add <string> alongside the existing standard headers so searchmoves and any other string-using declarations are self-contained and do not depend on transitive includes.
🧹 Nitpick comments (2)
.github/workflows/clang-format.yml (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the clang-format version for consistency.
Installing
clang-formatviaapt-getuses whatever version ships with the runner image, which may differ from developers' local versions. This can cause the workflow to reformat code differently than intended or produce spurious diffs. Consider installing a specific version (e.g., via pip:pip install clang-format==18.1.0) or using a versioned Docker action.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/clang-format.yml around lines 21 - 22, The clang-format install step is unpinned, so the workflow may use a different formatter version than local development. Update the install logic in the clang-format workflow job to use a fixed clang-format version instead of the runner’s default package; keep the change localized to the Install clang-format step and, if needed, the surrounding workflow job so the version is explicit and consistent.tt.h (1)
84-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDelete copy operations for
TranspositionTable. It ownstable, so the compiler-generated copy constructor/assignment would double-delete the buffer if this type is ever copied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tt.h` around lines 84 - 102, Delete the copy operations for TranspositionTable because it owns the table buffer and the default copy constructor/assignment would double-free it if copied. Update the TranspositionTable class definition to explicitly disable copying while keeping ownership semantics clear, and make sure any existing uses of TranspositionTable in code rely on move or reference semantics instead of copying.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Major comments:
In @.github/workflows/games.yml:
- Around line 31-32: The workflow permissions are too broad for a
build/test-only job; update the permissions block in the games workflow to
remove the write access or reduce the existing contents permission to read.
Locate the top-level permissions configuration in games.yml and keep it minimal
since no step in this workflow needs repository write access.
- Around line 113-127: The Run fastchess workflow step is interpolating
workflow_dispatch inputs directly into the shell command, which creates a shell
injection risk. Update the fastchess invocation to read output_exec, tc, rounds,
elo0, and elo1 from environment variables instead of embedding
github.event.inputs.* inline, and reference the existing Run fastchess step plus
the chmod +x setup that uses output_exec so the values are safely passed through
the shell.
In `@CMakeLists.txt`:
- Around line 9-18: The FetchContent_Declare entries for chesslib and tbprobe
still use moving branch references, which makes builds non-reproducible. Update
the GIT_TAG values in the FetchContent blocks (including the csv-parser
declaration mentioned in the review) to fixed commit SHAs or immutable release
tags, and keep the existing dependency names and repository URLs unchanged so
the build always resolves the same source revision.
In `@eval.cpp`:
- Around line 695-714: Update eval_components() to rely on the engine/library’s
insufficient-material detection instead of only special-casing KBKB and KNNK.
The issue is that KK, KBK, and KNK can still accumulate PST/material terms when
eval::eval() is called directly, so add the shared insufficient-material check
in eval_components() and ensure it returns a zeroed evaluation for all drawn
endgames it covers. Use the existing eval::eval() / eval_components() path and
the board material helpers already present to keep the logic centralized.
- Around line 31-228: The eval tuning registrations are disabled because the
entire TUNE block in eval.cpp is commented out, so Tune::get_list() and the
tuning/export path in tune_cmd.cpp never see these parameters. Re-enable this
block as active code in the evaluation setup so the existing TUNE entries for
the eval weight symbols are registered again and available to the tuner.
In `@movepick.cpp`:
- Around line 82-92: The SEE loop in movepick.cpp is still using the live board
occupancy instead of the hypothetical occupancy, which can let already-removed
blockers be considered again. Update the attacker filter in the SEE routine to
use the local occ state, and when removing the chosen square in the loop, clear
that bit explicitly rather than toggling it so the simulation stays consistent.
Keep the fix centered on the SEE/least_valuable_attacker flow that recomputes
attackers after each removal.
In `@score.cpp`:
- Around line 14-15: The mate score formatting path is handling negative mate
plies incorrectly, causing values like Mate{-1} to be rendered as mate 0. Update
the formatter logic in the UCI score conversion path to treat positive and
negative mate distances symmetrically, using the mate score symbol(s) involved
in score.cpp and the formatter in uci.cpp so negative odd plies preserve the
correct sign and count.
In `@search.cpp`:
- Around line 661-663: The TT fallback in search.cpp reports a stored move from
tt.lookup via report without validating it against the root position or the
active searchmoves constraints. Update the logic around the TTEntry/getMove path
to reuse the same legality filtering used in extract_pv before emitting
bestmove, and only report the move if it is legal for the current root board
state and allowed by the current search filter.
- Around line 618-655: In search.cpp’s doSearch iteration, the time-limit check
is happening before the depth’s best PV and InfoFull report are finalized, so a
completed score can be discarded and bestmove may stay stale. Reorder the logic
around the score_/time check so the current iteration still updates lastPV from
info.pv and calls report(info) for the completed depth before breaking; use the
existing doSearch, lastPV, extract_pv, and report symbols to place the fix.
In `@timeman.cpp`:
- Around line 79-81: The move budget calculation in the time management logic
can go negative after subtracting overhead, so clamp the result to zero (or the
minimum valid TimePoint) before assigning to maximumTime in timeman.cpp. Update
the computation around optimumTime/maximumTime so the overhead subtraction
cannot produce a negative hard limit on very small time controls or large Move
Overhead values.
- Around line 26-33: The time-control handling in the logic around the movetime
and base-clock checks should treat movetime as the highest priority override,
even when limits.time[us] is also set. Update the condition sequence so the
movetime case in the time-calculation code path is handled before any heuristic
clock-allocation branch, ensuring a command with both fields still uses the
exact requested duration; keep the fallback infinite-time case and other timing
logic unchanged.
In `@timeman.h`:
- Around line 16-18: `LimitsType` leaves `startTime` uninitialized in its
constructor, which can propagate undefined data into `TimeManagement::init()`
when limits are default-constructed. Update the `LimitsType()` initialization in
`timeman.h` to explicitly set `startTime` to a safe default alongside the other
time-control fields, and ensure any other constructor or initializer that
affects `LimitsType` follows the same pattern.
In `@tt.h`:
- Around line 2-4: The public header tt.h is not self-contained because it
relies on standard library and fixed-width types without including the proper
standard headers. Update the includes at the top of tt.h so the symbols used by
the header, including std::nothrow, std::bad_alloc, and fixed-width integer
types, are declared directly from the standard headers rather than depending on
transitive includes or non-standard headers; keep the change localized to the
header’s existing include block.
In `@tune_cmd.cpp`:
- Around line 907-915: The reservoir replacement path in tune_cmd.cpp keeps
growing fen_buf because it appends every newly selected FEN and only changes the
offset. Update the replacement logic in the reservoir sampling branch so memory
stays bounded by max_pos, either by reusing storage for the replaced sample or
by avoiding retaining old FEN strings beyond the sampled set. Focus on the
sampling code around the all.offsets/all.fen_buf/all.results updates and ensure
the buffer size tracks only the current reservoir.
- Around line 712-721: The tuning loop in `tune_cmd.cpp` initializes `best_loss`
from `base_loss`, but `best_loss` is compared against `hold_loss` later, so it
should be seeded with the holdout loss instead of the training loss. Update the
initialization near the `eval_loss(all, train_idx)` / `best_x` setup to compute
the corresponding holdout loss first and assign that to `best_loss`, and apply
the same change in the duplicated block referenced by the comment. Keep the
existing `best_x` initialization unchanged, but ensure the initial best score
always matches the metric used in the `hold_loss < best_loss` check.
- Around line 352-378: The gradient updates for split MG/EG weights in
tune_cmd.cpp are missing phase scaling, so fix the rook file logic in the shown
loop and the related passed-pawn and king-shelter sections by multiplying each
MG update by phase_mg and each EG update by phase_eg. Use the existing symbols
like rookOpenFileMg, rookOpenFileEg, rookSemiOpenFileMg, rookSemiOpenFileEg, and
the corresponding gradient writes to ensure both phases are updated with their
own phase factor instead of full strength.
- Around line 743-746: The tuning score formula in tune_cmd.cpp must stay
identical to eval::eval() and currently skips its mg == 0 && eg == 0 early
return. Update the score calculation in the tuning loop to use the same logic as
eval::eval() (or call the same helper) so equal-component positions evaluate to
0 instead of tempo, which will also keep train_loss aligned with eval_loss().
Use the existing eval::eval() and eval::eval_components() paths as the reference
points when fixing the duplicated calculation.
In `@uci.cpp`:
- Around line 88-93: The UCI parser in the searchmoves handling is consuming
every remaining token, so later go limits like wtime and btime are being
misclassified as moves. Update the parsing logic in uci.cpp around the
searchmoves loop to stop collecting moves once a recognized go limit token is
encountered, or otherwise parse searchmoves only until the next option keyword,
and keep the rest of the tokens available for the existing go-limit fields in
the same parser.
- Around line 138-140: The mate formatting in the `overload` lambda inside
`uci.cpp` currently uses integer division that truncates toward zero, so
negative values like `Score::Mate{-1}` become `mate 0`. Update the `mate.plies`
to move symmetrically away from zero before dividing by 2, using the same logic
for both positive and negative plies, so the `std::to_string(m)` output rounds
correctly for negative mate distances.
- Around line 219-292: The command dispatcher in engine::loop currently calls
stop() before every execCmd invocation, which incorrectly aborts search for
non-interrupting commands like uci, isready, and debug/query handlers. Update
the flow so stop() is only invoked inside the specific execCmd branches that
must terminate or alter an active search (for example stop, quit, setoption,
position-changing commands), and leave harmless commands to run without stopping
the current search.
In `@ucioption.cpp`:
- Around line 123-129: In Option::operator=, the spin-value validation currently
calls std::stoi(v) directly, which can throw on non-numeric or out-of-range
input. Update the "spin" branch to validate the string safely before converting,
or wrap the conversion in error handling so invalid values are simply rejected
and the function returns without changing the option. Keep the existing behavior
for button/string/check types intact.
---
Minor comments:
In @.github/workflows/games.yml:
- Around line 40-42: The checkout steps in the workflow are persisting the
default token unnecessarily. Update each actions/checkout invocation in the game
build jobs (the checkout step using the path new and the other checkout step
referenced by the comment) to set persist-credentials to false so GITHUB_TOKEN
is not written into .git/config; use the actions/checkout step configuration
itself to make the change.
In `@CMakeLists.txt`:
- Around line 44-49: Handle the `execute_process` call that populates `GIT_SHA`
so it fails quietly outside a git repo and still produces a usable version
string. Add `ERROR_QUIET` to the `git rev-parse --short HEAD` invocation, then
add a fallback in the same `GIT_SHA`/`BUILD_VERSION` flow so an empty result
does not yield `debug-` (for example, substitute a default revision or reuse the
existing `git describe` fallback pattern).
In `@main.cpp`:
- Line 14: The version print in main.cpp depends on BUILD_VERSION, but the
Makefile build path does not define it, so make will fail there. Update the
Makefile to pass a BUILD_VERSION definition during compilation, or add a safe
fallback in main.cpp so the BUILD_VERSION reference in the main entrypoint
always resolves regardless of build system.
In `@Makefile`:
- Around line 30-34: The Makefile dependency setup is non-reproducible because
deps clones the latest upstream HEAD and SRCS uses broad globs that may include
unwanted dependency test files. Update the deps target to fetch fixed revisions
for chesslib and tbprobe instead of floating clones, and replace the
deps/chesslib/*.cpp and deps/tbprobe/*.cpp patterns in SRCS with an explicit
source list of only the required production files. Keep the changes localized
around deps and SRCS so the build stays stable over time.
In `@score.h`:
- Around line 42-52: The default construction of Score currently leaves the
std::variant score as its first alternative, which means Score() behaves like
Mate{0} instead of internal units. Update Score’s default initialization in the
Score class so score starts as InternalUnits with VALUE_ZERO, or remove the
default constructor if an uninitialized/default Score should not exist; keep the
existing Score(Value v), is(), get(), and visit() behavior unchanged.
In `@timeman.h`:
- Around line 2-4: timeman.h is missing a direct include for std::string, which
makes the header rely on indirect includes. Update the includes in timeman.h to
add <string> alongside the existing standard headers so searchmoves and any
other string-using declarations are self-contained and do not depend on
transitive includes.
In `@tt.cpp`:
- Around line 33-45: The TranspositionTable::store depth handling currently uses
int8_t, which can wrap for deep searches and break replacement logic; change the
depth parameter in TranspositionTable::store to int, and clamp it to the valid
packed range right before calling TTEntry::setPackedFields. Make sure any depth
comparison against TTEntry::getDepth() still uses the unclamped int value, and
keep the stored packed depth bounded so entries remain correct for depths above
127.
In `@ucioption.cpp`:
- Around line 66-74: The insertion index in OptionsMap::add is being tracked
with a function-local static counter, so it is shared across all OptionsMap
instances and can drift when temporary maps are created. Change the order
tracking to be per-instance state on OptionsMap (or derive it from the current
map size/entry count) so each map assigns contiguous idx values independently,
and make sure operator<< continues to iterate over all inserted options using
those per-map indices.
---
Nitpick comments:
In @.github/workflows/clang-format.yml:
- Around line 21-22: The clang-format install step is unpinned, so the workflow
may use a different formatter version than local development. Update the install
logic in the clang-format workflow job to use a fixed clang-format version
instead of the runner’s default package; keep the change localized to the
Install clang-format step and, if needed, the surrounding workflow job so the
version is explicit and consistent.
In `@tt.h`:
- Around line 84-102: Delete the copy operations for TranspositionTable because
it owns the table buffer and the default copy constructor/assignment would
double-free it if copied. Update the TranspositionTable class definition to
explicitly disable copying while keeping ownership semantics clear, and make
sure any existing uses of TranspositionTable in code rely on move or reference
semantics instead of copying.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b54aa6d-4146-4915-9ab8-fdd8a44c9279
📒 Files selected for processing (40)
.clang-format.github/workflows/clang-format.yml.github/workflows/games.yml.gitignoreCMakeLists.txtMakefileREADME.mdWeights.hchess.hppeval.cppeval.heval.hppmain.cppmovepick.cppmovepick.hmovepick.hppscore.cppscore.hsearch.cppsearch.hsearch.hpptb.cpptb.htimeman.cpptimeman.htimeman.hpptt.cpptt.htt.hpptune.cpptune.htune_cmd.cpptune_cmd.huci.cppuci.huci.hppucioption.cppucioption.hucioptions.cppucioptions.hpp
💤 Files with no reviewable changes (8)
- uci.hpp
- search.hpp
- ucioptions.hpp
- eval.hpp
- ucioptions.cpp
- movepick.hpp
- timeman.hpp
- tt.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
33-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin dependency clones to a specific commit or tag for reproducibility and supply-chain integrity.
The
depstarget cloneschesslibandtb_probing_toolfrom their default branch HEAD without pinning to a specific commit, tag, or branch. This makes builds non-reproducible — an upstream push with breaking changes or a compromised repository would silently corrupt or compromise the build.Consider pinning to a specific commit:
deps: - test -d deps/chesslib || git clone https://github.com/winapiadmin/chesslib deps/chesslib - test -d deps/tbprobe || git clone https://github.com/winapiadmin/tb_probing_tool deps/tbprobe + test -d deps/chesslib || git clone https://github.com/winapiadmin/chesslib deps/chesslib && cd deps/chesslib && git checkout <pinned-sha> + test -d deps/tbprobe || git clone https://github.com/winapiadmin/tb_probing_tool deps/tbprobe && cd deps/tbprobe && git checkout <pinned-sha>At minimum, pin to a known-good commit SHA or tag so CI and local builds are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 37-42: `CHESSLIB_SRCS` and `SRCS` are being expanded too early in
the Makefile, so `wildcard` runs before the `deps` target clones `deps/chesslib`
and `deps/tbprobe`. Update the source discovery in the Makefile to defer
evaluation until build time (for example by using recursive assignment for
`CHESSLIB_SRCS` and anything derived from it, including `SRCS`/`OBJS`), so the
re-run after `deps` can pick up the cloned chesslib and tbprobe sources
automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
3 participants
tuff, see #6
Passed STC: https://github.com/winapiadmin/cppchess_engine/actions/runs/29003169121
Passed LTC: https://github.com/winapiadmin/cppchess_engine/actions/runs/28992367965
also fixes tons of crashes and timeouts by accident