Skip to content

Enhance chess tutor with rich position analysis and improved prompts#1

Open
TheOneWhoBurns wants to merge 6 commits intomainfrom
claude/review-thesis-project-0O8h7
Open

Enhance chess tutor with rich position analysis and improved prompts#1
TheOneWhoBurns wants to merge 6 commits intomainfrom
claude/review-thesis-project-0O8h7

Conversation

@TheOneWhoBurns
Copy link
Owner

@TheOneWhoBurns TheOneWhoBurns commented Jan 29, 2026

Summary

This PR significantly improves the chess tutor's educational capabilities by adding comprehensive position analysis, better error handling, and more engaging conversational responses. The system now provides richer context to the LLM for better move commentary and explanations.

Key Changes

Configuration & Environment

  • Added .env.example with required API keys (Django secret, Anthropic API, HuggingFace token)
  • Expanded .gitignore with comprehensive Python, Django, IDE, and project-specific patterns

Core Chess Logic (ChessLogic.py)

  • Added position analysis: New _get_position_analysis() method integrates ChessAnalyzer for rich board context
  • Move quality assessment: Implemented _get_move_quality() to evaluate moves by comparing engine evaluations before/after
  • Improved error handling: Added logging throughout, graceful fallbacks when Maia engine unavailable
  • Better game flow: Enhanced game start/end messages with more engaging language
  • Refactored move handling: Cleaner castling notation parsing and improved error messages
  • Chat history management: Better context preservation across game sessions

Prompt Generation (PromptMaker.py)

  • Structured prompt templates: Completely redesigned prompts with clear sections (game state, analysis, chat history, task)
  • Position-aware responses: Prompts now include rich position analysis and move quality assessments
  • Better move commentary: Separate handling for blunders, mistakes, good moves, and brilliant moves
  • Top moves formatting: New format_top_moves() method to present engine suggestions clearly
  • Improved explanations: Enhanced explanation prompts that guide learning without giving away answers
  • Conversational tone: Prompts now emphasize friendly, peer-like teaching style with casual language

Notable Implementation Details

  • Type hints added throughout for better code clarity
  • Logging replaces print statements for production-ready error tracking
  • Engine evaluation changes are now calculated to assess move quality objectively
  • Fallback mechanisms ensure graceful degradation if engine unavailable
  • Chat history limited to recent exchanges to maintain context window efficiency
  • Move quality feedback integrated into LLM prompts to enhance educational value

Benefits

  • Better learning: Students get immediate, specific feedback on move quality
  • Richer analysis: Position context helps the tutor provide more relevant guidance
  • More engaging: Conversational improvements make interactions feel more natural
  • More robust: Improved error handling and logging for production reliability
  • Cleaner code: Type hints and documentation improve maintainability

https://claude.ai/code/session_015oMB6YTKQBXf3DZPmd6Poz

Summary by CodeRabbit

  • New Features

    • Player stats bar showing animated rating, opponent level, and W‑L‑D record; server now returns player_stats.
    • Adaptive opponent difficulty that updates based on player performance.
    • Richer tutoring: deeper position analysis, move‑quality feedback, improved explanations and chat responses.
  • Chores

    • Added environment template, dependency list, engine setup utility, and updated .gitignore for local development.

✏️ Tip: You can customize this high-level summary in your review settings.

…anup

Security fixes:
- Move secrets (Django key, HF token) to environment variables
- Add .env.example template for configuration
- Fix XSS vulnerability in chat.js by using textContent
- Fix chat.js to handle 'ignore' and 'error' status properly

New features:
- Add chess_analyzer.py with rich position analysis:
  - Threat detection (hanging pieces, attacks on high-value pieces)
  - Tactical pattern recognition (forks, pins, hanging pieces)
  - Material balance calculation with descriptions
  - Game phase detection (opening/middlegame/endgame)
  - King safety assessment
  - Center control evaluation
  - Move quality assessment (blunder/mistake/good/brilliant)
- Enhanced PromptMaker with detailed analysis context for LLM
- LLM now receives comprehensive position info to give better feedback

Code quality:
- Add requirements.txt for dependency management
- Replace print statements with proper logging
- Add type hints and docstrings throughout
- Clean up .gitignore
- Improve error messages for users

https://claude.ai/code/session_015oMB6YTKQBXf3DZPmd6Poz
Upgrade from claude-3-5-haiku-20241022 to claude-sonnet-4-20250514
for better reasoning and response quality in chess tutoring.

https://claude.ai/code/session_015oMB6YTKQBXf3DZPmd6Poz
- Add scripts/setup_maia.py for easy dependency setup:
  - Auto-downloads Maia weights from official CSSLab repo
  - Checks for LC0 installation with helpful install instructions
  - Supports all Maia skill levels (1100-1900 Elo)
  - Progress bar for downloads

- Improve maia_engine.py:
  - Better error messages when LC0 or weights are missing
  - Configurable skill level (1100-1900 Elo)
  - set_level() method to change difficulty at runtime
  - Proper logging instead of silent failures
  - Type hints and documentation

https://claude.ai/code/session_015oMB6YTKQBXf3DZPmd6Poz
New features:
- skill_estimator.py: Tracks player performance and estimates Elo
  - Records move quality (blunders, mistakes, good moves)
  - Tracks game results against different Maia levels
  - Adjusts estimated Elo based on performance
  - Recommends appropriate Maia level (rounds up for challenge)

- Adaptive Maia difficulty:
  - Maia automatically adjusts to match player skill
  - Level changes between games based on performance
  - Starts at 1200, adjusts up/down based on play quality

- New UI stats bar showing:
  - Player's estimated Elo rating (with animations on change)
  - Current Maia opponent level
  - Win/Loss/Draw record
  - Styled dark header bar above the board

- Backend updates:
  - ChessLogic now tracks move quality for skill estimation
  - Game results recorded for Elo calculation
  - Views return player_stats in API response

https://claude.ai/code/session_015oMB6YTKQBXf3DZPmd6Poz
@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Adds environment/config templates and dependencies; introduces a ChessLogicUnit orchestrator, MaiaEngine wrapper, ChessAnalyzer, SkillEstimator, expanded PromptMaker and models, API view integration, frontend player-stats UI and animations, a Maia setup script, and .gitignore/requirements updates.

Changes

Cohort / File(s) Summary
Configuration & Dependencies
\.env.example, \.gitignore, chess_tutor/settings.py, requirements.txt
Adds .env example, dotenv-driven SECRET_KEY/DEBUG/HF_TOKEN, expands .gitignore patterns, and adds a pinned requirements.txt.
Core Orchestrator & API
chess_tutor/ChessLogic.py, chess_tutor/views.py
Introduces ChessLogicUnit centralizing game flow, intent handling, Maia integration, move evaluation and new public APIs; views wire requests to it and return structured JSON including player stats.
Engine Integration & Setup
chess_tutor/maia_engine.py, scripts/setup_maia.py
Adds MaiaEngine wrapper with leveled weights, evaluation/top-move APIs, move-quality classification and runtime level switching; CLI script to download/verify Maia weights and LC0.
Analysis & Prompting
chess_tutor/chess_analyzer.py, chess_tutor/PromptMaker.py
New ChessAnalyzer produces structured PositionAnalysis and LLM-ready text; PromptMaker enriched to format move/chat history and build move/explanation/chat prompts including top-moves and move-quality context.
Skill Tracking
chess_tutor/skill_estimator.py
Adds SkillEstimator with MoveRecord/GameRecord, dynamic Elo estimation, per-move/game recording, Maia level recommendation, and a global instance.
Models & LLM config
chess_tutor/models.py
Switches LLM model to claude-sonnet-4-20250514, reduces max_tokens, shortens system prompt, and replaces prints with logger usage.
Frontend UI & Assets
templates/chat.html, static/css/main.css, static/js/chat.js
Adds Player Stats Bar UI, CSS for stats and Elo animations (noted duplicated blocks), and JS updatePlayerStats with safe DOM updates and animation toggles; integrates player_stats in responses.
Miscellaneous
static/css/..., other small tweaks
Minor logging/error-handling adjustments and duplicated CSS insertions across stylesheet locations.

Sequence Diagram

sequenceDiagram
    participant User
    participant View as API View
    participant Classifier as Intent Classifier
    participant Logic as ChessLogicUnit
    participant Engine as MaiaEngine
    participant Analyzer as ChessAnalyzer
    participant Estimator as SkillEstimator
    participant Prompter as PromptMaker
    participant LLM as Claude LLM

    User->>View: POST message
    View->>Classifier: Classify intent with board context
    Classifier-->>View: Intent result
    View->>Logic: handle_message(intent_result)
    
    alt Move Intent
        Logic->>Engine: get_best_move(board)
        Engine-->>Logic: Maia move
        Logic->>Analyzer: analyze_position(board)
        Analyzer-->>Logic: Position analysis
        Logic->>Estimator: record_move(quality, eval_change)
        Estimator-->>Logic: Updated stats
        Logic->>Prompter: create_move_prompt(...)
        Prompter-->>Logic: Prompt text
        Logic->>LLM: Request tutoring response with prompt
        LLM-->>Logic: Tutoring response
    else Explanation Intent
        Logic->>Analyzer: analyze_position(board)
        Analyzer-->>Logic: Position analysis
        Logic->>Prompter: create_explanation_prompt(...)
        Prompter-->>Logic: Prompt text
        Logic->>LLM: Request explanation
        LLM-->>Logic: Explanation
    else Chat Intent
        Logic->>Analyzer: analyze_position(board)
        Analyzer-->>Logic: Position analysis
        Logic->>Prompter: create_chat_prompt(...)
        Prompter-->>Logic: Prompt text
        Logic->>LLM: Request conversational response
        LLM-->>Logic: Chat response
    end
    
    Logic-->>View: Response dict (message, stats, moves, fen)
    View-->>User: JSON response
    User->>User: Update UI and stats bar
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 With whiskers twitching, I hopped through each part,
Engines and analyzers stitched close to the heart.
Prompts, stats, and levels in a tidy parade,
Maia learns softly while lessons are made,
A nibble of code—now let’s play a smart game! ♟️✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Enhance chess tutor with rich position analysis and improved prompts' directly summarizes the main changes: adding position analysis and improving prompts, which are core to the PR's objectives.
Docstring Coverage ✅ Passed Docstring coverage is 82.76% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@chess_tutor/chess_analyzer.py`:
- Around line 409-418: The castling detection using board.is_castling(move) on
moves from board.move_stack is unreliable because it checks against the current
board state and the unused variable piece_moved should be removed; fix it by
creating a fresh temporary Board(), iterate through board.move_stack, and for
each move call temp.is_castling(move) before temp.push(move) to decide if the
king castled for that color (set is_castled accordingly), then remove the unused
piece_moved assignment; reference symbols: is_castled, board.move_stack,
board.is_castling, temp.push, and piece_moved.

In `@chess_tutor/PromptMaker.py`:
- Around line 210-222: The loop building move lines in PromptMaker.py can raise
TypeError when move_info has 'evaluation': None; change the retrieval in the
block that computes eval_pawns (inside the for over top_moves in the method that
constructs lines) to coerce None to 0 before dividing (e.g. read evaluation into
a local like eval_raw = move_info.get('evaluation'); if eval_raw is None:
eval_raw = 0; then eval_pawns = eval_raw / 100) so division always receives a
number; keep the existing mate handling and san lookup unchanged.
🧹 Nitpick comments (19)
scripts/setup_maia.py (3)

77-83: Remove unnecessary f-string prefix and consider narrowing the exception type.

Line 79 has an f-string without placeholders. The broad Exception catch is acceptable for download error handling but could be narrowed to urllib.error.URLError for more precise error handling.

🧹 Proposed fix
     try:
         urllib.request.urlretrieve(url, weights_file, _download_progress)
-        print(f"\n  ✓ Downloaded successfully!")
+        print("\n  ✓ Downloaded successfully!")
         return True
     except Exception as e:
         print(f"\n  ✗ Download failed: {e}")
         return False

110-114: Remove unnecessary f-string prefixes.

These strings contain no placeholders.

🧹 Proposed fix
     if weights_ok:
-        print(f"  ✓ Maia weights present")
+        print("  ✓ Maia weights present")
     else:
-        print(f"  ✗ Maia weights missing")
+        print("  ✗ Maia weights missing")

153-155: Remove unused variable lc0_ok.

The result of check_lc0_installed() is assigned but never used.

🧹 Proposed fix
     # Check LC0
-    lc0_ok = check_lc0_installed()
+    check_lc0_installed()
     print()
chess_tutor/skill_estimator.py (3)

67-80: Consider annotating mutable class attributes with ClassVar.

The static analysis tool flags MOVE_QUALITY_ADJUSTMENTS and RESULT_ADJUSTMENTS as mutable class attributes that should be annotated with typing.ClassVar to clarify they're not instance attributes.

🧹 Proposed fix
+from typing import ClassVar, Dict, List, Optional
-from typing import List, Optional
 
 ...
 
+    MOVE_QUALITY_ADJUSTMENTS: ClassVar[Dict[str, int]] = {
-    MOVE_QUALITY_ADJUSTMENTS = {
         "Blunder": -15,
         "Mistake": -8,
         "Normal": 0,
         "Good": +5,
         "Excellent": +12,
     }
 
+    RESULT_ADJUSTMENTS: ClassVar[Dict[str, int]] = {
-    RESULT_ADJUSTMENTS = {
         "win": +25,
         "draw": +5,
         "loss": -15,
     }

236-238: Avoid calling __init__ from reset().

Calling self.__init__() directly is an anti-pattern that can cause issues with subclassing and doesn't clearly communicate intent. Consider explicitly resetting attributes instead.

♻️ Proposed refactor
     def reset(self):
         """Reset all statistics (for testing or new player)."""
-        self.__init__(self.DEFAULT_ELO)
+        self.estimated_elo = self.DEFAULT_ELO
+        self.game_history.clear()
+        self.current_game_moves.clear()
+        self.recent_moves.clear()
+        self.total_moves = 0
+        self.total_blunders = 0
+        self.total_mistakes = 0
+        self.total_good_moves = 0
+        self.games_played = 0
+        self.wins = 0
+        self.losses = 0
+        self.draws = 0

241-242: Global singleton may complicate testing and multi-user scenarios.

The module-level skill_estimator instance creates shared state. This works for single-user scenarios but may cause issues if the application needs to support multiple concurrent users or isolated test cases. Consider using dependency injection or session-scoped instances if this becomes a concern.

chess_tutor/ChessLogic.py (4)

39-41: Unused _last_eval attribute.

_last_eval is initialized but never read or updated anywhere in this file. Only _pre_move_eval is used for move quality assessment. Consider removing it if it's not needed.

🧹 Proposed fix
         # Track evaluation for move quality assessment
-        self._last_eval: Optional[int] = None
         self._pre_move_eval: Optional[int] = None

49-54: Use logger.exception to preserve stack traces.

When catching exceptions and logging errors, use logger.exception() instead of logger.error() to automatically include the stack trace, which aids debugging.

♻️ Proposed fix
         try:
             self.maia_engine = MaiaEngine(self.project_dir, level=level)
             logger.info(f"Maia engine initialized at level {level}")
         except Exception as e:
-            logger.error(f"Failed to initialize Maia engine: {e}")
+            logger.exception("Failed to initialize Maia engine")
             self.maia_engine = None

This pattern should also be applied at lines 68-69, 100-101, 129-130, 138-139, 392, 438, and 464.


104-131: Unused move parameter and reliance on private methods.

  1. The move parameter is declared but never used—the method operates on self.board directly.
  2. The method accesses private methods _classify_move_quality and _assess_last_move from other classes, which couples this code to internal implementation details.
♻️ Proposed fix for unused parameter
-    def _evaluate_and_record_move(self, move: chess.Move) -> tuple[Optional[str], Optional[int]]:
+    def _evaluate_and_record_move(self) -> tuple[Optional[str], Optional[int]]:

Update the call site at line 321-323:

-        player_move_quality, eval_change = self._evaluate_and_record_move(
-            self.board.move_stack[-1] if self.board.move_stack else None
-        )
+        player_move_quality, _eval_change = self._evaluate_and_record_move()

Consider exposing classify_move_quality and assess_last_move as public methods, or move the classification logic into SkillEstimator where it's used.


320-323: Unused eval_change variable.

The eval_change value is unpacked but never used. Prefix it with an underscore to indicate it's intentionally ignored.

🧹 Proposed fix
         # Evaluate and record the player's move
-        player_move_quality, eval_change = self._evaluate_and_record_move(
+        player_move_quality, _eval_change = self._evaluate_and_record_move(
             self.board.move_stack[-1] if self.board.move_stack else None
         )
chess_tutor/maia_engine.py (1)

166-174: Simplify key existence check.

The if "pv" in pv and pv["pv"] pattern can be simplified using dict.get().

🧹 Proposed fix
         for pv in analysis:
-            if "pv" in pv and pv["pv"]:
+            pv_moves = pv.get("pv")
+            if pv_moves:
                 score = pv["score"].white()
                 moves.append({
-                    "move": pv["pv"][0],
-                    "san": board.san(pv["pv"][0]),
+                    "move": pv_moves[0],
+                    "san": board.san(pv_moves[0]),
                     "evaluation": score.score(mate_score=10000),
                     "mate": score.mate()
                 })
static/css/main.css (1)

47-52: Consider improving label contrast for accessibility.

The .stat-label uses #95a5a6 on the dark gradient background (#2c3e50), which has approximately 3.5:1 contrast ratio. WCAG AA requires 4.5:1 for small text (11px). Consider using a lighter gray like #b8c6ce or #c0c8ce to improve readability.

🎨 Proposed fix
 .stat-label {
     font-size: 11px;
-    color: `#95a5a6`;
+    color: `#bdc3c7`;
     text-transform: uppercase;
     letter-spacing: 0.5px;
 }
chess_tutor/models.py (1)

60-62: Use logger.exception for error logging.

Same as noted in ChessLogic.py—use logger.exception() instead of logger.error() to capture the full stack trace. This applies here and at line 106.

♻️ Proposed fix
         except Exception as e:
-            logger.error(f"Error initializing models: {e}")
+            logger.exception("Error initializing models")
             raise
chess_tutor/views.py (2)

63-69: Default argument is always evaluated.

On line 68, chess_logic.get_player_stats() is evaluated even when "player_stats" exists in response. This is a minor inefficiency since Python evaluates default arguments before dict.get() checks the key.

Suggested fix using conditional expression
         return JsonResponse({
             'response': response["message"],
             'status': response["status"],
             'moves': response["moves"],
             'fen': chess_logic.get_current_position(),
-            'player_stats': response.get("player_stats", chess_logic.get_player_stats())
+            'player_stats': response.get("player_stats") or chess_logic.get_player_stats()
         })

71-78: Use logging.exception for automatic traceback inclusion.

For the JSONDecodeError handler, consider whether a traceback is needed. For the general exception handler (line 80), logging.exception is preferred over logging.error with exc_info=True as it's more concise and always includes the traceback.

Suggested fix
     except Exception as e:
-        logger.error(f"Error in send_message: {e}", exc_info=True)
+        logger.exception(f"Error in send_message: {e}")
         return JsonResponse({
chess_tutor/chess_analyzer.py (3)

362-372: Unused board parameter.

The board parameter is not used in _summarize_tactics. Consider removing it if not needed for future enhancements.

Suggested fix
-    def _summarize_tactics(self, tactics: List[TacticalPattern], board: chess.Board) -> str:
+    def _summarize_tactics(self, tactics: List[TacticalPattern]) -> str:
         """Create tactical summary"""

Note: This requires updating the call site on line 116.


533-538: Unused board parameter in _create_position_assessment.

The board parameter is passed but not used in this method.


582-607: Remove extraneous f-string prefixes.

Multiple strings have f prefix but contain no placeholders (lines 582, 589, 594, 598, 602, 607). This is harmless but adds noise.

Suggested fix for one example
-        sections.append(f"=== POSITION ANALYSIS ===")
+        sections.append("=== POSITION ANALYSIS ===")

Apply similarly to lines 589, 594, 598, 602, 607.

chess_tutor/PromptMaker.py (1)

18-25: Move formatting is correct but could include standard spacing.

The logic correctly produces alternating move numbers for white moves. Standard PGN notation includes a space after the period: 1. e4 rather than 1.e4.

Suggested fix
     def _format_move_history(self, move_history: List[str]) -> str:
         """Format move history in standard chess notation"""
         if not move_history:
             return "No moves yet."
         return " ".join(
-            f"{i//2 + 1}.{move}" if i % 2 == 0 else move
+            f"{i//2 + 1}. {move}" if i % 2 == 0 else move
             for i, move in enumerate(move_history)
         )

Address CodeRabbit review: detect castling by king's two-square move
pattern instead of unreliable board.is_castling on historical moves,
and guard against None evaluation values in PromptMaker.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@chess_tutor/chess_analyzer.py`:
- Around line 336-358: The pin detection incorrectly iterates a full ray (which
can extend beyond the king); change the logic in the block that builds
pieces_between to use chess.SquareSet.between(square, king_sq) instead of
chess.SquareSet.ray(square, king_sq), iterate those returned squares (no need to
exclude square or king), collect pieces as before, and keep the existing check
that if len(pieces_between) == 1 you create the pinned_sq/pinned_piece and
append the TacticalPattern (symbols: chess.SquareSet.ray -> replace with
chess.SquareSet.between, pieces_between, pinned_sq, pinned_piece,
patterns.append(TacticalPattern(...))).

In `@chess_tutor/PromptMaker.py`:
- Around line 27-35: In _format_chat_history, avoid KeyError from malformed
message dicts by defensively accessing keys: inside the _format_chat_history
function (and the list comprehension that builds the returned string), replace
direct indexing msg['role']/msg['content'] with msg.get('role', 'user' or a safe
default) and msg.get('content', '<no content>'), and optionally filter out or
skip non-dict entries (e.g., ensure isinstance(msg, dict)) so the function
returns stable output even when chat_history contains malformed items.
🧹 Nitpick comments (3)
chess_tutor/chess_analyzer.py (3)

87-92: extended_center is defined but never used.

The extended_center list is initialized but not referenced anywhere in the class. Consider removing it or implementing its intended usage.

🧹 Proposed fix
     def __init__(self):
         # Center squares for control assessment
         self.center_squares = [chess.D4, chess.D5, chess.E4, chess.E5]
-        self.extended_center = [
-            chess.C3, chess.C4, chess.C5, chess.C6,
-            chess.D3, chess.D4, chess.D5, chess.D6,
-            chess.E3, chess.E4, chess.E5, chess.E6,
-            chess.F3, chess.F4, chess.F5, chess.F6
-        ]

362-362: Unused board parameter.

The board parameter is declared but not used. If it's reserved for future use, consider prefixing with underscore (_board) to suppress warnings.


581-606: Remove extraneous f prefix from string literals.

Several strings use f-string syntax but contain no placeholders. Remove the f prefix for clarity.

🧹 Proposed fix
-        sections.append(f"=== POSITION ANALYSIS ===")
+        sections.append("=== POSITION ANALYSIS ===")
         sections.append(f"Game Phase: {analysis.game_phase.upper()}")
         sections.append(f"Position Assessment: {analysis.who_is_winning}")
         sections.append(f"Material: {analysis.material_description}")

         # Last move quality (if available)
         if analysis.last_move_assessment:
-            sections.append(f"\n=== LAST MOVE ===")
+            sections.append("\n=== LAST MOVE ===")
             sections.append(analysis.last_move_assessment)

         # Threats and tactics
         if analysis.threats_summary != "No immediate tactical threats.":
-            sections.append(f"\n=== THREATS ===")
+            sections.append("\n=== THREATS ===")
             sections.append(analysis.threats_summary)

         if analysis.tactics_summary != "No major tactical patterns detected.":
-            sections.append(f"\n=== TACTICS ===")
+            sections.append("\n=== TACTICS ===")
             sections.append(analysis.tactics_summary)

         # King safety (only if noteworthy)
-        sections.append(f"\n=== KING SAFETY ===")
+        sections.append("\n=== KING SAFETY ===")
         sections.append(f"White: {analysis.white_king_safety}")
         sections.append(f"Black: {analysis.black_king_safety}")

         # Center control
-        sections.append(f"\n=== POSITIONAL ===")
+        sections.append("\n=== POSITIONAL ===")
         sections.append(analysis.center_control)

Comment on lines +336 to +358
# Check if there's exactly one piece between attacker and king
ray = chess.SquareSet.ray(square, king_sq)
if ray:
pieces_between = []
for ray_sq in ray:
if ray_sq != square and ray_sq != king_sq:
p = board.piece_at(ray_sq)
if p:
pieces_between.append((ray_sq, p))

# If exactly one piece of the target color is between
if len(pieces_between) == 1:
pinned_sq, pinned_piece = pieces_between[0]
if pinned_piece.color == color:
attacker_name = PIECE_NAMES[piece.piece_type]
pinned_name = PIECE_NAMES[pinned_piece.piece_type]
attacker_color = "White" if opponent == chess.WHITE else "Black"
pinned_color = "White" if color == chess.WHITE else "Black"
patterns.append(TacticalPattern(
pattern_type="pin",
description=f"{pinned_color}'s {pinned_name} on {chess.square_name(pinned_sq)} is pinned to the king by {attacker_color}'s {attacker_name}!",
squares_involved=[square, pinned_sq, king_sq]
))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

python-chess SquareSet.ray documentation

💡 Result:

In python-chess, SquareSet.ray(a, b) is a classmethod that returns a SquareSet containing all squares on the same rank, file, or diagonal that contains both squares a and bbut only if a and b are aligned. If they are not aligned, it returns an empty set (bitboard 0). [1][2]

Example (from the docs):

import chess
print(chess.SquareSet.ray(chess.E2, chess.B5))

This prints the diagonal “ray” through E2 and B5 (including both endpoints). [1]

Docs/source: See the SquareSet.ray entry in the “Core” docs and its implementation in chess.py. [1][2]


🌐 Web query:

python-chess SquareSet.between documentation

💡 Result:

chess.SquareSet.between(a, b) is a class method that returns a SquareSet containing all squares on the same rank, file, or diagonal strictly between squares a and b (endpoints excluded). If a and b are not aligned, it returns an empty set. [1][2]

Example from the docs:

import chess
print(chess.SquareSet.between(chess.E2, chess.B5))

This yields the squares between E2 and B5 along the diagonal (C4 and D3). [1]

Sources: python-chess “Core” docs (latest) [1] and the generated module docs showing the method docstring/implementation [2].


🏁 Script executed:

# Find and inspect the _find_pins method in context
fd chess_analyzer.py | head -1 | xargs -I {} sh -c 'wc -l "{}" && sed -n "310,365p" "{}"'

Repository: TheOneWhoBurns/Chess-LLM-Tutor

Length of output: 3136


Pin detection incorrectly includes squares beyond the king.

chess.SquareSet.ray(square, king_sq) returns all squares on the rank, file, or diagonal through both points—extending beyond the king on that line. The current code manually excludes only the attacker and king squares with if ray_sq != square and ray_sq != king_sq, but this leaves squares beyond the king that should never be considered. For example, with a bishop on A1 and king on E5, the ray includes F6, G7, and H8; any piece there would be incorrectly identified as pinned.

Use chess.SquareSet.between(square, king_sq) instead, which returns only squares strictly between the two points:

🔧 Proposed fix
-                        ray = chess.SquareSet.ray(square, king_sq)
-                        if ray:
+                        between = chess.SquareSet.between(square, king_sq)
+                        if between:
                             pieces_between = []
-                            for ray_sq in ray:
-                                if ray_sq != square and ray_sq != king_sq:
-                                    p = board.piece_at(ray_sq)
-                                    if p:
-                                        pieces_between.append((ray_sq, p))
+                            for sq in between:
+                                p = board.piece_at(sq)
+                                if p:
+                                    pieces_between.append((sq, p))
📝 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.

Suggested change
# Check if there's exactly one piece between attacker and king
ray = chess.SquareSet.ray(square, king_sq)
if ray:
pieces_between = []
for ray_sq in ray:
if ray_sq != square and ray_sq != king_sq:
p = board.piece_at(ray_sq)
if p:
pieces_between.append((ray_sq, p))
# If exactly one piece of the target color is between
if len(pieces_between) == 1:
pinned_sq, pinned_piece = pieces_between[0]
if pinned_piece.color == color:
attacker_name = PIECE_NAMES[piece.piece_type]
pinned_name = PIECE_NAMES[pinned_piece.piece_type]
attacker_color = "White" if opponent == chess.WHITE else "Black"
pinned_color = "White" if color == chess.WHITE else "Black"
patterns.append(TacticalPattern(
pattern_type="pin",
description=f"{pinned_color}'s {pinned_name} on {chess.square_name(pinned_sq)} is pinned to the king by {attacker_color}'s {attacker_name}!",
squares_involved=[square, pinned_sq, king_sq]
))
# Check if there's exactly one piece between attacker and king
between = chess.SquareSet.between(square, king_sq)
if between:
pieces_between = []
for sq in between:
p = board.piece_at(sq)
if p:
pieces_between.append((sq, p))
# If exactly one piece of the target color is between
if len(pieces_between) == 1:
pinned_sq, pinned_piece = pieces_between[0]
if pinned_piece.color == color:
attacker_name = PIECE_NAMES[piece.piece_type]
pinned_name = PIECE_NAMES[pinned_piece.piece_type]
attacker_color = "White" if opponent == chess.WHITE else "Black"
pinned_color = "White" if color == chess.WHITE else "Black"
patterns.append(TacticalPattern(
pattern_type="pin",
description=f"{pinned_color}'s {pinned_name} on {chess.square_name(pinned_sq)} is pinned to the king by {attacker_color}'s {attacker_name}!",
squares_involved=[square, pinned_sq, king_sq]
))
🤖 Prompt for AI Agents
In `@chess_tutor/chess_analyzer.py` around lines 336 - 358, The pin detection
incorrectly iterates a full ray (which can extend beyond the king); change the
logic in the block that builds pieces_between to use
chess.SquareSet.between(square, king_sq) instead of chess.SquareSet.ray(square,
king_sq), iterate those returned squares (no need to exclude square or king),
collect pieces as before, and keep the existing check that if
len(pieces_between) == 1 you create the pinned_sq/pinned_piece and append the
TacticalPattern (symbols: chess.SquareSet.ray -> replace with
chess.SquareSet.between, pieces_between, pinned_sq, pinned_piece,
patterns.append(TacticalPattern(...))).

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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