feat(backend): Cheat Detection Engine#696
Conversation
- Add CheatDetectionEngine in backend/modules/chess/src/cheat_detection.rs - Heuristic analysis: move time consistency, accuracy, complexity speed, blunder avoidance — all O(1) per move, CPU-efficient - Fully serde-serialisable API (MoveEntry, HeuristicResult, RiskLevel) - 21 unit tests covering: risk classification, think-time variance, high-quality move detection, colour separation, blunder avoidance scoring, std-dev edge cases, and reset behaviour - Fix pre-existing rating.rs DbErr wrapping errors - Expose module via chess::lib.rs public re-export Closes NOVUS-X#537
|
@playmaker410 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new cheat detection engine to the chess backend, exposes it from the crate root, adds serde derive support, and adjusts a SeaORM error-wrapping expression in the rating module. ChangesCheat Detection Engine
Rating Error Handling Change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
backend/modules/chess/src/cheat_detection.rs (2)
96-98: Validateentry.color(and consider using an enum) before recording.
record_moveaccepts any string forcolor, but every downstream filter (moves_for_color,analyse) only matches exactly"w"or"b". A typo from a caller ("W","white","B") silently disappears intoself.movesand produces empty/insufficient-data reports forever. Either reject invalid input here or replacecolor: StringonMoveEntrywithshakmaty::Color(and serialize as a string at the API boundary).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/chess/src/cheat_detection.rs` around lines 96 - 98, record_move currently accepts MoveEntry with a freeform String color which allows invalid values to be stored and later filtered out; update this by (preferred) changing MoveEntry.color from String to shakmaty::Color (and handle serde serialization/deserialization at the API boundary) so all callers supply a validated enum, or (alternate) keep the String but validate inside record_move by normalizing/parsing the string to only accept "w" or "b" (or mapping to shakmaty::Color) and return a Result/Error when invalid so callers cannot push bad colors; reference the record_move method, the MoveEntry type, and downstream consumers moves_for_color / analyse when making the change.
314-342:count_blundersis expensive and contradicts the "O(1) per move" design claim.For every recorded move, this function parses a FEN, constructs a
Chessposition, parses a SAN, clones the position, plays the move, then iterateslegal_moves()to check for captures. While chess bounds these operations by a constant, the constant is large (up to ~218 legal moves enumerated per position), and you do it for every move on everyanalyse()call.Worth considering:
- Cache parsed
Chesspositions or compute blunders incrementally duringrecord_move()to makeanalyse()cheaper.- Replace the
legal_moves()scan withpos.board.attacks_to(dest, attacking_color, occupied)to check attackers via bitboard queries instead of materializing the full move list.- The "hanging piece" detector ignores defenders — a captured piece that's defended isn't a blunder. Either document this as a "loose piece" heuristic only, or compare attacker/defender counts and piece values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/chess/src/cheat_detection.rs` around lines 314 - 342, count_blunders is expensive because it reparses FEN/SAN, reconstructs Chess positions, clones and enumerates legal_moves for every analyzed move; fix by (1) moving parsing/work into record_move so you store a parsed Fen/Chess position or the destination square and a precomputed occupied bitboard when moves are recorded, (2) replace the legal_moves() scan in count_blunders with a bitboard attack query (use the Chess.position.board.attacks_to(dest, attacking_color, occupied) or equivalent API instead of iterating legal_moves), and (3) if you want true blunder detection consider comparing attacker vs defender counts/values (or document it as a "loose piece" heuristic) so count_blunders only checks immediate attackers against stored occupancy and cached position data rather than rebuilding positions per call.backend/modules/chess/src/lib.rs (1)
5-8: Prefer an explicit re-export list over a wildcard.
pub use cheat_detection::*;silently re-exports every public symbol the module ever adds, which makes the crate's public surface implicit and prone to accidentally leaking helpers in future commits. The other modules in this file already follow the explicit-list convention.✏️ Optional tightening
-pub use cheat_detection::*; +pub use cheat_detection::{ + CheatDetectionEngine, HeuristicDetails, HeuristicResult, MoveEntry, RiskLevel, +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/modules/chess/src/lib.rs` around lines 5 - 8, Replace the wildcard re-export of the cheat_detection module with an explicit list of the public symbols you intend to expose: inspect the cheat_detection module for its pub items (types, structs, enums, functions, traits — e.g., any public names like CheatDetector, detect_cheat, CheatEvent, etc.), then change pub use cheat_detection::*; to pub use cheat_detection::{NameA, NameB, NameC}; listing each public symbol explicitly (and update this list when intentionally adding new public APIs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/modules/chess/src/cheat_detection.rs`:
- Around line 187-196: compute_think_times currently computes wall‑clock gaps
between consecutive moves of the same color (because caller prefilters with
moves_for_color), which mixes the opponent's intervening think time into the
metric; change compute_think_times to accept the full move list (e.g.,
&self.moves) plus the target color instead of a prefiltered slice, iterate the
full sequence and for each move of the target color subtract the immediately
preceding move's timestamp to get the true think time, filter out non‑positive
or >300_000 values as before, and update callers (and the time_consistency /
avg_think_time / think_time_std_dev calculations) to use the new signature so
reported stats reflect only the player's own think times.
- Around line 262-290: The function score_complexity_speed incorrectly pairs
think_times with player_moves by using the same index; think_times[k] is the
time taken to play move k+1, so for a complex move at player_moves[i] you must
use think_times[i-1] (and skip i==0). Update score_complexity_speed to compute
fast by mapping each complex index i to think_index = i - 1, only counting those
with i >= 1 and think_index < think_times.len(), i.e. filter complex_indices for
i>0 then check think_times[i-1] < FAST_MOVE_MS; this preserves the correct
correlation and avoids silently dropping/invalid indexing of the last move.
- Around line 248-253: The current condition treats any early knight/bishop move
as high-quality due to the tautology `from_square != to_square`; replace that
check with one that verifies the piece actually left its initial back-rank
starting square (i.e., a developing move). Update the conditional using the
existing fields entry.piece_role, entry.move_number, entry.from_square (and
entry.color or side if available) so it only returns true when piece_role is "n"
or "b", move_number <= 10, and entry.from_square is one of the role-specific
initial squares (knight: b1/g1 for White, b8/g8 for Black; bishop: c1/f1 for
White, c8/f8 for Black) — or encapsulate that logic in a small helper like
is_initial_backrank_square(entry.piece_role, entry.from_square, entry.color) and
use it in the branch.
In `@backend/modules/chess/src/rating.rs`:
- Around line 66-75: Replace the manual wrapping of database errors into
DbErr::Custom(String) and explicit ApiError::DatabaseError(...) calls with
direct propagation of sea_orm::DbErr using the ? operator so the existing
From<DbErr> for ApiError conversion is used; specifically update the
db.begin().await error handling, the txn.commit().await handling, and any other
spots in this file that currently do map_err(|e|
ApiError::DatabaseError(sea_orm::DbErr::Custom(format!(...)))) (e.g., around
update_ratings_in_transaction, commit, and related DB calls) to instead return
the DbErr via ? or, if you need to add context, wrap using DbErr::Custom once
(not converting into a plain String) before propagating; also apply the same fix
in backend/modules/service/src/games.rs where ApiError::DatabaseError is being
created from format!(...) so those spots use ? or DbErr::Custom(...) to preserve
the original DbErr variants.
---
Nitpick comments:
In `@backend/modules/chess/src/cheat_detection.rs`:
- Around line 96-98: record_move currently accepts MoveEntry with a freeform
String color which allows invalid values to be stored and later filtered out;
update this by (preferred) changing MoveEntry.color from String to
shakmaty::Color (and handle serde serialization/deserialization at the API
boundary) so all callers supply a validated enum, or (alternate) keep the String
but validate inside record_move by normalizing/parsing the string to only accept
"w" or "b" (or mapping to shakmaty::Color) and return a Result/Error when
invalid so callers cannot push bad colors; reference the record_move method, the
MoveEntry type, and downstream consumers moves_for_color / analyse when making
the change.
- Around line 314-342: count_blunders is expensive because it reparses FEN/SAN,
reconstructs Chess positions, clones and enumerates legal_moves for every
analyzed move; fix by (1) moving parsing/work into record_move so you store a
parsed Fen/Chess position or the destination square and a precomputed occupied
bitboard when moves are recorded, (2) replace the legal_moves() scan in
count_blunders with a bitboard attack query (use the
Chess.position.board.attacks_to(dest, attacking_color, occupied) or equivalent
API instead of iterating legal_moves), and (3) if you want true blunder
detection consider comparing attacker vs defender counts/values (or document it
as a "loose piece" heuristic) so count_blunders only checks immediate attackers
against stored occupancy and cached position data rather than rebuilding
positions per call.
In `@backend/modules/chess/src/lib.rs`:
- Around line 5-8: Replace the wildcard re-export of the cheat_detection module
with an explicit list of the public symbols you intend to expose: inspect the
cheat_detection module for its pub items (types, structs, enums, functions,
traits — e.g., any public names like CheatDetector, detect_cheat, CheatEvent,
etc.), then change pub use cheat_detection::*; to pub use
cheat_detection::{NameA, NameB, NameC}; listing each public symbol explicitly
(and update this list when intentionally adding new public APIs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4954fbf3-81dc-4fe1-88e7-9b6a31bb9984
⛔ Files ignored due to path filters (1)
backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
backend/modules/chess/Cargo.tomlbackend/modules/chess/src/cheat_detection.rsbackend/modules/chess/src/lib.rsbackend/modules/chess/src/rating.rs
| fn compute_think_times(&self, player_moves: &[&MoveEntry]) -> Vec<u64> { | ||
| let mut times = Vec::new(); | ||
| for window in player_moves.windows(2) { | ||
| let elapsed = window[1].timestamp.saturating_sub(window[0].timestamp); | ||
| if elapsed > 0 && elapsed < 300_000 { | ||
| times.push(elapsed); | ||
| } | ||
| } | ||
| times | ||
| } |
There was a problem hiding this comment.
compute_think_times measures wall-clock between same-color moves, not actual think time.
After moves_for_color(color) filters to a single side, consecutive timestamps are separated by both the player's think time and the opponent's intervening think time. The time_consistency heuristic (and avg_think_time / think_time_std_dev reported in the result) therefore conflates two players' clocks, which significantly weakens the "robotic pacing" signal you're trying to detect — e.g. a human alternating with a slow opponent will look more consistent than they really are.
The fix needs the full move list (so you can subtract the immediately preceding opponent timestamp). Consider passing &self.moves plus the target color to this helper instead of pre-filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/chess/src/cheat_detection.rs` around lines 187 - 196,
compute_think_times currently computes wall‑clock gaps between consecutive moves
of the same color (because caller prefilters with moves_for_color), which mixes
the opponent's intervening think time into the metric; change
compute_think_times to accept the full move list (e.g., &self.moves) plus the
target color instead of a prefiltered slice, iterate the full sequence and for
each move of the target color subtract the immediately preceding move's
timestamp to get the true think time, filter out non‑positive or >300_000 values
as before, and update callers (and the time_consistency / avg_think_time /
think_time_std_dev calculations) to use the new signature so reported stats
reflect only the player's own think times.
| if (entry.piece_role == "n" || entry.piece_role == "b") | ||
| && entry.move_number <= 10 | ||
| && entry.from_square != entry.to_square | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Tautological condition from_square != to_square.
A legal chess move always has distinct from/to squares, so this branch unconditionally classifies every knight or bishop move played on or before move 10 as "high-quality". That over-counts ordinary opening moves (and even retreats) as evidence of accuracy, biasing score_accuracy upward for any normal opening.
Most likely you meant a developing move — i.e. the piece leaves its initial back-rank square:
🐛 Suggested fix
- if (entry.piece_role == "n" || entry.piece_role == "b")
- && entry.move_number <= 10
- && entry.from_square != entry.to_square
- {
- return true;
- }
+ if (entry.piece_role == "n" || entry.piece_role == "b")
+ && entry.move_number <= 10
+ {
+ let back_rank = if entry.color == "w" { '1' } else { '8' };
+ if entry.from_square.ends_with(back_rank) {
+ return true;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (entry.piece_role == "n" || entry.piece_role == "b") | |
| && entry.move_number <= 10 | |
| && entry.from_square != entry.to_square | |
| { | |
| return true; | |
| } | |
| if (entry.piece_role == "n" || entry.piece_role == "b") | |
| && entry.move_number <= 10 | |
| { | |
| let back_rank = if entry.color == "w" { '1' } else { '8' }; | |
| if entry.from_square.ends_with(back_rank) { | |
| return true; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/chess/src/cheat_detection.rs` around lines 248 - 253, The
current condition treats any early knight/bishop move as high-quality due to the
tautology `from_square != to_square`; replace that check with one that verifies
the piece actually left its initial back-rank starting square (i.e., a
developing move). Update the conditional using the existing fields
entry.piece_role, entry.move_number, entry.from_square (and entry.color or side
if available) so it only returns true when piece_role is "n" or "b", move_number
<= 10, and entry.from_square is one of the role-specific initial squares
(knight: b1/g1 for White, b8/g8 for Black; bishop: c1/f1 for White, c8/f8 for
Black) — or encapsulate that logic in a small helper like
is_initial_backrank_square(entry.piece_role, entry.from_square, entry.color) and
use it in the branch.
| fn score_complexity_speed(player_moves: &[&MoveEntry], think_times: &[u64]) -> u32 { | ||
| if player_moves.len() < MIN_MOVES { | ||
| return 0; | ||
| } | ||
| let complex_indices: Vec<usize> = player_moves | ||
| .iter() | ||
| .enumerate() | ||
| .filter(|(_, m)| Self::count_pieces(&m.fen_before) >= COMPLEX_PIECE_THRESHOLD) | ||
| .map(|(i, _)| i) | ||
| .collect(); | ||
|
|
||
| if complex_indices.len() < 3 { | ||
| return 0; | ||
| } | ||
|
|
||
| let fast = complex_indices | ||
| .iter() | ||
| .filter(|&&i| i < think_times.len() && think_times[i] < FAST_MOVE_MS) | ||
| .count(); | ||
|
|
||
| let rate = fast as f64 / complex_indices.len() as f64; | ||
| if rate >= 0.5 { | ||
| (rate * 120.0).round().min(90.0) as u32 | ||
| } else if rate >= 0.3 { | ||
| (30.0 + (rate - 0.3) * 200.0).round() as u32 | ||
| } else { | ||
| (rate * 100.0).round() as u32 | ||
| } | ||
| } |
There was a problem hiding this comment.
Off-by-one when correlating think_times with player_moves.
think_times[k] is the gap between player_moves[k] and player_moves[k+1], so it is the time the player took to play move k+1, not move k. Indexing by the same i you use to identify a complex position attributes the wrong gap to that position — and the very last complex move (which has no think_times[i] because windows(2) produces len - 1 entries) is silently dropped via the i < think_times.len() guard.
🐛 Suggested fix
- let fast = complex_indices
- .iter()
- .filter(|&&i| i < think_times.len() && think_times[i] < FAST_MOVE_MS)
- .count();
+ // think_times[k] = time spent on player_moves[k+1]; index 0 has no preceding gap.
+ let fast = complex_indices
+ .iter()
+ .filter(|&&i| i > 0 && i - 1 < think_times.len() && think_times[i - 1] < FAST_MOVE_MS)
+ .count();(Note: this also depends on compute_think_times actually representing the player's think time — see the separate comment on that helper.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/chess/src/cheat_detection.rs` around lines 262 - 290, The
function score_complexity_speed incorrectly pairs think_times with player_moves
by using the same index; think_times[k] is the time taken to play move k+1, so
for a complex move at player_moves[i] you must use think_times[i-1] (and skip
i==0). Update score_complexity_speed to compute fast by mapping each complex
index i to think_index = i - 1, only counting those with i >= 1 and think_index
< think_times.len(), i.e. filter complex_indices for i>0 then check
think_times[i-1] < FAST_MOVE_MS; this preserves the correct correlation and
avoids silently dropping/invalid indexing of the last move.
| let txn = db.begin().await | ||
| .map_err(|e| ApiError::DatabaseError(format!("Failed to start transaction: {}", e)))?; | ||
| .map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!("Failed to start transaction: {}", e))))?; | ||
|
|
||
| let result = Self::update_ratings_in_transaction(&txn, game_id, config).await; | ||
|
|
||
| match result { | ||
| Ok(ratings) => { | ||
| // Commit the transaction if everything succeeded | ||
| txn.commit().await | ||
| .map_err(|e| ApiError::DatabaseError(format!("Failed to commit transaction: {}", e)))?; | ||
| .map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!("Failed to commit transaction: {}", e))))?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any remaining call sites that pass a String/format! into ApiError::DatabaseError.
rg -nP --type=rust -C2 'ApiError::DatabaseError\(\s*format!'Repository: NOVUS-X/XLMate
Length of output: 2232
🏁 Script executed:
# First, find the ApiError definition
find . -type f -name "*.rs" | xargs grep -l "struct ApiError\|enum ApiError" | head -5Repository: NOVUS-X/XLMate
Length of output: 95
🏁 Script executed:
# Check the ApiError::DatabaseError variant definition
rg -A5 "enum ApiError|DatabaseError" backend/modules/error/src/error.rs | head -40Repository: NOVUS-X/XLMate
Length of output: 1078
🏁 Script executed:
# Check if From<DbErr> impl actually exists
rg "impl From<.*DbErr" backend/modules/error/src/error.rs -A5Repository: NOVUS-X/XLMate
Length of output: 170
Propagate DbErr directly instead of wrapping in DbErr::Custom.
The ApiError::DatabaseError variant expects a DbErr, not a String. Wrapping every DbErr in DbErr::Custom(format!(...)) collapses the original error variant (e.g., RecordNotFound, ConnectionAcquire) into an opaque string. Per the existing impl From<DbErr> for ApiError in backend/modules/error/src/error.rs, use ? to propagate directly and let the conversion happen automatically:
- let txn = db.begin().await
- .map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!("Failed to start transaction: {}", e))))?;
+ let txn = db.begin().await?;Applies to all rewritten sites in this file: lines 67, 75, 96, 121, 127, 153, 156, 219, and 241.
Blocking issue: backend/modules/service/src/games.rs still passes format!(...) (a String) directly to ApiError::DatabaseError, which now expects a DbErr. Lines 93, 99, 114, 123, and 144 in that file will fail to compile. These must be updated using the same ? pattern above or wrapped in DbErr::Custom if context messages are required.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let txn = db.begin().await | |
| .map_err(|e| ApiError::DatabaseError(format!("Failed to start transaction: {}", e)))?; | |
| .map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!("Failed to start transaction: {}", e))))?; | |
| let result = Self::update_ratings_in_transaction(&txn, game_id, config).await; | |
| match result { | |
| Ok(ratings) => { | |
| // Commit the transaction if everything succeeded | |
| txn.commit().await | |
| .map_err(|e| ApiError::DatabaseError(format!("Failed to commit transaction: {}", e)))?; | |
| .map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!("Failed to commit transaction: {}", e))))?; | |
| let txn = db.begin().await?; | |
| let result = Self::update_ratings_in_transaction(&txn, game_id, config).await; | |
| match result { | |
| Ok(ratings) => { | |
| // Commit the transaction if everything succeeded | |
| txn.commit().await | |
| .map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!("Failed to commit transaction: {}", e))))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/modules/chess/src/rating.rs` around lines 66 - 75, Replace the manual
wrapping of database errors into DbErr::Custom(String) and explicit
ApiError::DatabaseError(...) calls with direct propagation of sea_orm::DbErr
using the ? operator so the existing From<DbErr> for ApiError conversion is
used; specifically update the db.begin().await error handling, the
txn.commit().await handling, and any other spots in this file that currently do
map_err(|e| ApiError::DatabaseError(sea_orm::DbErr::Custom(format!(...))))
(e.g., around update_ratings_in_transaction, commit, and related DB calls) to
instead return the DbErr via ? or, if you need to add context, wrap using
DbErr::Custom once (not converting into a plain String) before propagating; also
apply the same fix in backend/modules/service/src/games.rs where
ApiError::DatabaseError is being created from format!(...) so those spots use ?
or DbErr::Custom(...) to preserve the original DbErr variants.
|
Kindly Review |
1 participant
Summary
Implements the backend cheat detection engine as described in issue #537.
What's Changed
backend/modules/chess/src/cheat_detection.rs—CheatDetectionEnginestruct with full heuristic pipelinelow | moderate | high | criticalMoveEntry,HeuristicResult,HeuristicDetails,RiskLevel)Test Results
Acceptance Criteria
cargo test -p chess --lib)chess::lib.rsCloses #537
Summary by CodeRabbit