- Add comprehensive test suite (20 tests) for tactic detection - Fix pawn detection threshold bug (200 -> 100 centipawns) - Add error handling to uciToSan for invalid moves - Integrate tactical data into Tutor component for real-time feedback - Pass missedTactics from ChessGame to Tutor via props - Update LLM prompt to explain missed tactical opportunities - Fix Jest configuration to handle react-markdown ESM issues - All 50 tests passing
5.9 KiB
Tactic Recognition Module - Technical Analysis
Branch: codex/add-tactic-recognition-module
Executive Summary
The tactic recognition module has been partially implemented with good foundational code, but has critical gaps that prevent it from being merge-ready:
- ✅ Core detection logic is implemented - All required tactic types are detected
- ✅ Data structure matches requirements - Output format is correct
- ✅ Integration in ChessGame component - Tactics are detected and stored in move history
- ❌ NOT integrated with LLM pipeline - Tactic data is NOT passed to the Tutor/LLM for real-time feedback
- ❌ NO test coverage - Zero tests for the tactic detection module
- ⚠️ Only used in post-game analysis - Not available during gameplay
Detailed Analysis
1. Implementation Quality ✅
File: src/lib/tacticDetection.ts (366 lines)
The implementation is well-structured and covers all required tactic types:
- ✅ Material capture (win_piece, win_pawn)
- ✅ Pin detection
- ✅ Fork detection
- ✅ Skewer detection
- ✅ Check detection
- ✅ Hanging piece detection
- ✅ Conservative approach (filters false positives)
Strengths:
- Clean, readable code with helper functions
- Proper use of chess.js library
- Conservative detection (e.g., checks if captured piece can be recaptured)
- Correct piece value assignments
- Proper handling of edge cases (no best move, same move, etc.)
Minor Issues:
- Line 200: Threshold of 200cp for "win_piece" vs "win_pawn" seems arbitrary (should be 100 for pawn)
- No configuration options exposed (thresholds are hardcoded)
2. Integration Status ⚠️
ChessGame.tsx Integration:
// Lines 337-358: Tactic detection IS called
const missedTactics = detectMissedTactics({
fen: fenP0,
playerColor,
playerMoveSan: moveResult.result.san,
bestMoveUci: evalP0.bestMove,
cpLoss,
});
// Stored in move history
const completeHistoryItem = {
// ... other fields
missedTactics,
};
✅ Tactics ARE detected after each player move
✅ Tactics ARE stored in moveHistory state
✅ Tactics ARE available in GameOverModal for post-game analysis
GameOverModal.tsx Integration:
// Lines 145-160: Tactics are formatted for LLM in post-game analysis
const describeTactics = (tactics?: DetectedTactic[]) => {
// Formats tactics as text for LLM
};
✅ Tactics ARE used in post-game analysis LLM prompt
3. CRITICAL GAP: Real-time LLM Integration ❌
Tutor.tsx Analysis:
The Tutor component (which provides real-time feedback during the game) does NOT receive or use tactic data:
// Lines 17-32: TutorProps interface
interface TutorProps {
game: Chess;
currentFen: string;
userMove: Move | null;
computerMove: Move | null;
stockfish: Stockfish | null;
evalP0: StockfishEvaluation | null;
evalP2: StockfishEvaluation | null;
openingData: OpeningMetadata | null;
// ❌ NO missedTactics prop!
// ...
}
// Lines 185-202: LLM prompt construction
const prompt = `
[SYSTEM TRIGGER: move_exchange]
User (${playerColorName}) Move: ${userMove.san}
My (${tutorColorName}) Reply: ${computerMove.san}
My Internal Thoughts (Data):
- Pre-Eval (Before User Move): ${preScore} cp
- Post-Eval (After My Reply): ${postScore} cp
- Delta: ${delta} cp
// ❌ NO tactic information included!
`;
Impact: The AI tutor cannot provide tactical feedback during the game (e.g., "You missed a fork with Nf3!").
4. Test Coverage ❌
Status: ZERO tests for tactic detection module
Required tests:
- Unit tests for each tactic type detection
- Edge case tests (empty board, no tactics, multiple tactics)
- Integration tests with chess.js
- UCI to SAN conversion tests
- False positive prevention tests
5. Requirements Compliance
| Requirement | Status | Notes |
|---|---|---|
| Detect material capture | ✅ | Implemented with safety check |
| Detect pins | ✅ | Sliding piece logic correct |
| Detect forks | ✅ | Multi-target detection works |
| Detect skewers | ✅ | Value comparison correct |
| Detect checks | ✅ | Uses chess.js inCheck() |
| Detect hanging pieces | ✅ | Attack/defense counting |
| Conservative approach | ✅ | Multiple safety filters |
| Integrate before LLM | ❌ | Only in post-game, not real-time |
| No engine calls | ✅ | Uses provided data only |
| Configurable thresholds | ⚠️ | Hardcoded, not exposed |
| Structured output | ✅ | Matches spec exactly |
Recommendations
MUST HAVE (Before Merge):
-
Add comprehensive test suite (CRITICAL)
- Create
src/lib/__tests__/tacticDetection.test.ts - Test each tactic type with known positions
- Test edge cases and false positive prevention
- Aim for >80% code coverage
- Create
-
Integrate with real-time Tutor (CRITICAL - per requirements)
- Add
missedTacticsprop toTutorProps - Pass tactic data from ChessGame to Tutor
- Include tactic information in LLM prompt
- Format tactics in a way the LLM can explain naturally
- Add
SHOULD HAVE (Quality improvements):
-
Fix piece value threshold
- Line 200: Change threshold from 200 to 100 for pawn detection
-
Add configuration options
- Expose
evalLossThresholdas a prop - Allow customization based on player skill level
- Expose
-
Add documentation
- JSDoc comments for public functions
- Usage examples in README
Conclusion
Recommendation: DO NOT MERGE YET
The implementation is solid but incomplete. The module works well for post-game analysis but fails the primary requirement: providing tactical information to the LLM during gameplay for real-time feedback.
Estimated work to make merge-ready:
- Test suite: 4-6 hours
- Real-time integration: 2-3 hours
- Minor fixes: 1 hour
- Total: ~8 hours of work
The code quality is good and the foundation is strong. With the additions above, this will be a valuable feature.