fix: update move validation to check for king safety (#13)

Reviewed-on: #13

.claude/agents/test-writer.md

@@ -20,5 +20,4 @@ When invoked BEFORE scala-implementer (no implementation exists yet):
 
 When invoked AFTER scala-implementer (implementation exists):
   Run python3 jacoco-reporter/jacoco_coverage_gaps.py modules/{service-name}/build/reports/jacoco/test/jacocoTestReport.xml --output agent
-  Use the jacoco-coverage-gaps skill — close coverage gaps revealed by the report.
   To regenerate the report run the tests first.

modules/core/src/main/scala/de/nowchess/chess/controller/GameController.scala

@@ -63,8 +63,7 @@ object GameController:
       case PromotionPiece.Bishop => PieceType.Bishop
       case PromotionPiece.Knight => PieceType.Knight
     val newBoard    = boardAfterMove.updated(to, Piece(turn, promotedPieceType))
-    // Promotion is always a pawn move → clock resets
-    val newHistory = history.addMove(from, to, None, Some(piece), wasPawnMove = true)
+    val newHistory  = history.addMove(from, to, None, Some(piece))
     toMoveResult(newBoard, newHistory, captured, turn)
 
   // ---------------------------------------------------------------------------
@@ -76,7 +75,7 @@ object GameController:
       case None                              => MoveResult.NoPiece
       case Some(piece) if piece.color != turn => MoveResult.WrongColor
       case Some(_) =>
-        if !MoveValidator.isLegal(board, history, from, to) then MoveResult.IllegalMove
+        if !GameRules.legalMoves(board, history, turn).contains(from -> to) then MoveResult.IllegalMove
         else if MoveValidator.isPromotionMove(board, from, to) then
           MoveResult.PromotionRequired(from, to, board, history, board.pieceAt(to), turn)
         else applyNormalMove(board, history, turn, from, to)
@@ -92,9 +91,7 @@ object GameController:
           val capturedSq = EnPassantCalculator.capturedPawnSquare(to, turn)
           (b.removed(capturedSq), board.pieceAt(capturedSq))
         else (b, cap)
-    val wasPawnMove = board.pieceAt(from).exists(_.pieceType == PieceType.Pawn)
-    val wasCapture  = captured.isDefined
-    val newHistory  = history.addMove(from, to, castleOpt, wasPawnMove = wasPawnMove, wasCapture = wasCapture)
+    val newHistory = history.addMove(from, to, castleOpt)
     toMoveResult(newBoard, newHistory, captured, turn)
 
   private def toMoveResult(newBoard: Board, newHistory: GameHistory, captured: Option[Piece], turn: Color): MoveResult =

modules/core/src/main/scala/de/nowchess/chess/engine/GameEngine.scala

@@ -67,19 +67,6 @@ class GameEngine(
       case "redo" =>
         performRedo()
 
-      case "draw" =>
-        if currentHistory.halfMoveClock >= 100 then
-          currentBoard   = Board.initial
-          currentHistory = GameHistory.empty
-          currentTurn    = Color.White
-          invoker.clear()
-          notifyObservers(DrawClaimedEvent(currentBoard, currentHistory, currentTurn))
-        else
-          notifyObservers(InvalidMoveEvent(
-            currentBoard, currentHistory, currentTurn,
-            "Draw cannot be claimed: the 50-move rule has not been triggered."
-          ))
-
       case "" =>
         val event = InvalidMoveEvent(
           currentBoard,
@@ -122,8 +109,6 @@ class GameEngine(
                 invoker.execute(updatedCmd)
                 updateGameState(newBoard, newHistory, newTurn)
                 emitMoveEvent(from.toString, to.toString, captured, newTurn)
-                if currentHistory.halfMoveClock >= 100 then
-                  notifyObservers(FiftyMoveRuleAvailableEvent(currentBoard, currentHistory, currentTurn))
 
               case MoveResult.MovedInCheck(newBoard, newHistory, captured, newTurn) =>
                 // Move succeeded with check
@@ -132,8 +117,6 @@ class GameEngine(
                 updateGameState(newBoard, newHistory, newTurn)
                 emitMoveEvent(from.toString, to.toString, captured, newTurn)
                 notifyObservers(CheckDetectedEvent(currentBoard, currentHistory, currentTurn))
-                if currentHistory.halfMoveClock >= 100 then
-                  notifyObservers(FiftyMoveRuleAvailableEvent(currentBoard, currentHistory, currentTurn))
 
               case MoveResult.Checkmate(winner) =>
                 // Move resulted in checkmate

modules/core/src/main/scala/de/nowchess/chess/logic/GameHistory.scala

@@ -11,36 +11,21 @@ case class HistoryMove(
   promotionPiece: Option[PromotionPiece] = None
 )
 
-/** Complete game history: ordered list of moves plus the half-move clock for the 50-move rule.
- *
- *  @param moves         moves played so far, oldest first
- *  @param halfMoveClock plies since the last pawn move or capture (FIDE 50-move rule counter)
- */
-case class GameHistory(moves: List[HistoryMove] = List.empty, halfMoveClock: Int = 0):
-
-  /** Add a raw HistoryMove record. Clock increments by 1.
-   *  Use the coordinate overload when you know whether the move is a pawn move or capture.
-   */
+/** Complete game history: ordered list of moves. */
+case class GameHistory(moves: List[HistoryMove] = List.empty):
   def addMove(move: HistoryMove): GameHistory =
-    GameHistory(moves :+ move, halfMoveClock + 1)
+    GameHistory(moves :+ move)
+
+  def addMove(from: Square, to: Square): GameHistory =
+    addMove(HistoryMove(from, to, None))
 
-  /** Add a move by coordinates.
-   *
-   *  @param wasPawnMove true when the moving piece is a pawn — resets the clock to 0
-   *  @param wasCapture  true when a piece was captured (including en passant) — resets the clock to 0
-   *
-   *  If neither flag is set the clock increments by 1.
-   */
   def addMove(
     from: Square,
     to: Square,
     castleSide: Option[CastleSide] = None,
-    promotionPiece: Option[PromotionPiece] = None,
-    wasPawnMove: Boolean = false,
-    wasCapture: Boolean = false
+    promotionPiece: Option[PromotionPiece] = None
   ): GameHistory =
-    val newClock = if wasPawnMove || wasCapture then 0 else halfMoveClock + 1
-    GameHistory(moves :+ HistoryMove(from, to, castleSide, promotionPiece), newClock)
+    addMove(HistoryMove(from, to, castleSide, promotionPiece))
 
 object GameHistory:
   val empty: GameHistory = GameHistory()

modules/core/src/main/scala/de/nowchess/chess/notation/PgnExporter.scala

@@ -22,8 +22,7 @@ object PgnExporter:
         if blackMoveStr.isEmpty then s"$moveNum. $whiteMoveStr"
         else s"$moveNum. $whiteMoveStr $blackMoveStr"
 
-      val termination = headers.getOrElse("Result", "*")
-      moveLines.mkString(" ") + s" $termination"
+      moveLines.mkString(" ") + " *"
 
     if headerLines.isEmpty then moveText
     else if moveText.isEmpty then headerLines

modules/core/src/main/scala/de/nowchess/chess/observer/Observer.scala

@@ -67,20 +67,6 @@ case class BoardResetEvent(
   turn: Color
 ) extends GameEvent
 
-/** Fired after any move where the half-move clock reaches 100 — the 50-move rule is now claimable. */
-case class FiftyMoveRuleAvailableEvent(
-  board: Board,
-  history: GameHistory,
-  turn: Color
-) extends GameEvent
-
-/** Fired when a player successfully claims a draw under the 50-move rule. */
-case class DrawClaimedEvent(
-  board: Board,
-  history: GameHistory,
-  turn: Color
-) extends GameEvent
-
 /** Observer trait: implement to receive game state updates. */
 trait Observer:
   def onGameEvent(event: GameEvent): Unit

modules/core/src/test/scala/de/nowchess/chess/controller/GameControllerTest.scala

@@ -43,6 +43,30 @@ class GameControllerTest extends AnyFunSuite with Matchers:
     // White pawn at E2 cannot jump three squares to E5
     processMove(Board.initial, GameHistory.empty, Color.White, "e2e5") shouldBe MoveResult.IllegalMove
 
+  test("processMove: move that leaves own king in check returns IllegalMove"):
+    // White King E1 is in check from Black Rook E8. Moving the D2 pawn is
+    // geometrically legal but does not resolve the check — must be rejected.
+    val b = Board(Map(
+      sq(File.E, Rank.R1) -> Piece.WhiteKing,
+      sq(File.D, Rank.R2) -> Piece.WhitePawn,
+      sq(File.E, Rank.R8) -> Piece.BlackRook,
+      sq(File.A, Rank.R8) -> Piece.BlackKing
+    ))
+    processMove(b, GameHistory.empty, Color.White, "d2d4") shouldBe MoveResult.IllegalMove
+
+  test("processMove: move that resolves check is allowed"):
+    // White King E1 is in check from Black Rook E8 along the E-file.
+    // White Rook A5 interposes at E5 — resolves the check, no new check on Black King A8.
+    val b = Board(Map(
+      sq(File.E, Rank.R1) -> Piece.WhiteKing,
+      sq(File.A, Rank.R5) -> Piece.WhiteRook,
+      sq(File.E, Rank.R8) -> Piece.BlackRook,
+      sq(File.A, Rank.R8) -> Piece.BlackKing
+    ))
+    processMove(b, GameHistory.empty, Color.White, "a5e5") match
+      case _: MoveResult.Moved => succeed
+      case other => fail(s"Expected Moved, got $other")
+
   test("processMove: legal pawn move returns Moved with updated board and flipped turn"):
     processMove(Board.initial, GameHistory.empty, Color.White, "e2e4") match
       case MoveResult.Moved(newBoard, newHistory, captured, newTurn) =>
@@ -464,39 +488,3 @@ class GameControllerTest extends AnyFunSuite with Matchers:
       PromotionPiece.Knight, Color.White
     )
     result should be (MoveResult.Stalemate)
-
-  // ──── half-move clock propagation ────────────────────────────────────
-
-  test("processMove: non-pawn non-capture increments halfMoveClock"):
-    // g1f3 is a knight move — not a pawn, not a capture
-    processMove(Board.initial, GameHistory.empty, Color.White, "g1f3") match
-      case MoveResult.Moved(_, newHistory, _, _) =>
-        newHistory.halfMoveClock shouldBe 1
-      case other => fail(s"Expected Moved, got $other")
-
-  test("processMove: pawn move resets halfMoveClock to 0"):
-    processMove(Board.initial, GameHistory.empty, Color.White, "e2e4") match
-      case MoveResult.Moved(_, newHistory, _, _) =>
-        newHistory.halfMoveClock shouldBe 0
-      case other => fail(s"Expected Moved, got $other")
-
-  test("processMove: capture resets halfMoveClock to 0"):
-    // White pawn on e5, Black pawn on d6 — exd6 is a capture
-    val board = Board(Map(
-      sq(File.E, Rank.R5) -> Piece.WhitePawn,
-      sq(File.D, Rank.R6) -> Piece.BlackPawn,
-      sq(File.E, Rank.R1) -> Piece.WhiteKing,
-      sq(File.E, Rank.R8) -> Piece.BlackKing
-    ))
-    val history = GameHistory(halfMoveClock = 10)
-    processMove(board, history, Color.White, "e5d6") match
-      case MoveResult.Moved(_, newHistory, _, _) =>
-        newHistory.halfMoveClock shouldBe 0
-      case other => fail(s"Expected Moved, got $other")
-
-  test("processMove: clock carries from previous history on non-pawn non-capture"):
-    val history = GameHistory(halfMoveClock = 5)
-    processMove(Board.initial, history, Color.White, "g1f3") match
-      case MoveResult.Moved(_, newHistory, _, _) =>
-        newHistory.halfMoveClock shouldBe 6
-      case other => fail(s"Expected Moved, got $other")

modules/core/src/test/scala/de/nowchess/chess/engine/GameEngineTest.scala

@@ -3,7 +3,7 @@ package de.nowchess.chess.engine
 import scala.collection.mutable
 import de.nowchess.api.board.{Board, Color}
 import de.nowchess.chess.logic.GameHistory
-import de.nowchess.chess.observer.{Observer, GameEvent, MoveExecutedEvent, CheckDetectedEvent, BoardResetEvent, InvalidMoveEvent, FiftyMoveRuleAvailableEvent, DrawClaimedEvent}
+import de.nowchess.chess.observer.{Observer, GameEvent, MoveExecutedEvent, CheckDetectedEvent, BoardResetEvent, InvalidMoveEvent}
 import org.scalatest.funsuite.AnyFunSuite
 import org.scalatest.matchers.should.Matchers
 
@@ -302,47 +302,6 @@ class GameEngineTest extends AnyFunSuite with Matchers:
     engine.board shouldBe boardAfterSecondMove
     engine.turn shouldBe Color.White
 
-  // ──── 50-move rule ───────────────────────────────────────────────────
-
-  test("GameEngine: 'draw' rejected when halfMoveClock < 100"):
-    val engine = new GameEngine()
-    val observer = new MockObserver()
-    engine.subscribe(observer)
-    engine.processUserInput("draw")
-    observer.events.size shouldBe 1
-    observer.events.head shouldBe a[InvalidMoveEvent]
-
-  test("GameEngine: 'draw' accepted and fires DrawClaimedEvent when halfMoveClock >= 100"):
-    val engine = new GameEngine(initialHistory = GameHistory(halfMoveClock = 100))
-    val observer = new MockObserver()
-    engine.subscribe(observer)
-    engine.processUserInput("draw")
-    observer.events.size shouldBe 1
-    observer.events.head shouldBe a[DrawClaimedEvent]
-
-  test("GameEngine: state resets to initial after draw claimed"):
-    val engine = new GameEngine(initialHistory = GameHistory(halfMoveClock = 100))
-    engine.processUserInput("draw")
-    engine.board shouldBe Board.initial
-    engine.history shouldBe GameHistory.empty
-    engine.turn shouldBe Color.White
-
-  test("GameEngine: FiftyMoveRuleAvailableEvent fired when move brings clock to 100"):
-    // Start at clock 99; a knight move (non-pawn, non-capture) increments to 100
-    val engine = new GameEngine(initialHistory = GameHistory(halfMoveClock = 99))
-    val observer = new MockObserver()
-    engine.subscribe(observer)
-    engine.processUserInput("g1f3")  // knight move on initial board
-    // Should receive MoveExecutedEvent AND FiftyMoveRuleAvailableEvent
-    observer.events.exists(_.isInstanceOf[FiftyMoveRuleAvailableEvent]) shouldBe true
-
-  test("GameEngine: FiftyMoveRuleAvailableEvent not fired when clock is below 100 after move"):
-    val engine = new GameEngine(initialHistory = GameHistory(halfMoveClock = 5))
-    val observer = new MockObserver()
-    engine.subscribe(observer)
-    engine.processUserInput("g1f3")
-    observer.events.exists(_.isInstanceOf[FiftyMoveRuleAvailableEvent]) shouldBe false
-
   // Mock Observer for testing
   private class MockObserver extends Observer:
     val events = mutable.ListBuffer[GameEvent]()

modules/core/src/test/scala/de/nowchess/chess/logic/GameHistoryTest.scala

@@ -69,36 +69,3 @@ class GameHistoryTest extends AnyFunSuite with Matchers:
     newHistory.moves should have length 1
     newHistory.moves.head.castleSide should be (None)
     newHistory.moves.head.promotionPiece should be (Some(PromotionPiece.Queen))
-
-  // ──── half-move clock ────────────────────────────────────────────────
-
-  test("halfMoveClock starts at 0"):
-    GameHistory.empty.halfMoveClock shouldBe 0
-
-  test("halfMoveClock increments on a non-pawn non-capture move"):
-    val h = GameHistory.empty.addMove(sq(File.G, Rank.R1), sq(File.F, Rank.R3))
-    h.halfMoveClock shouldBe 1
-
-  test("halfMoveClock resets to 0 on a pawn move"):
-    val h = GameHistory.empty.addMove(sq(File.E, Rank.R2), sq(File.E, Rank.R4), wasPawnMove = true)
-    h.halfMoveClock shouldBe 0
-
-  test("halfMoveClock resets to 0 on a capture"):
-    val h = GameHistory.empty.addMove(sq(File.E, Rank.R5), sq(File.D, Rank.R6), wasCapture = true)
-    h.halfMoveClock shouldBe 0
-
-  test("halfMoveClock resets to 0 when both wasPawnMove and wasCapture are true"):
-    val h = GameHistory.empty.addMove(sq(File.E, Rank.R5), sq(File.D, Rank.R6), wasPawnMove = true, wasCapture = true)
-    h.halfMoveClock shouldBe 0
-
-  test("halfMoveClock carries across multiple moves"):
-    val h = GameHistory.empty
-      .addMove(sq(File.G, Rank.R1), sq(File.F, Rank.R3))          // +1 → 1
-      .addMove(sq(File.G, Rank.R8), sq(File.F, Rank.R6))          // +1 → 2
-      .addMove(sq(File.E, Rank.R2), sq(File.E, Rank.R4), wasPawnMove = true)  // reset → 0
-      .addMove(sq(File.B, Rank.R1), sq(File.C, Rank.R3))          // +1 → 1
-    h.halfMoveClock shouldBe 1
-
-  test("GameHistory can be initialised with a non-zero halfMoveClock"):
-    val h = GameHistory(halfMoveClock = 42)
-    h.halfMoveClock shouldBe 42

modules/core/src/test/scala/de/nowchess/chess/notation/FenExporterTest.scala

@@ -67,22 +67,3 @@ class FenExporterTest extends AnyFunSuite with Matchers:
     )
     val fen = FenExporter.gameStateToFen(gameState)
     fen shouldBe "rnbqkbnr/pp1ppppp/8/2pP4/8/8/PPPP1PPP/RNBQKBNR w KQkq c6 2 3"
-
-  test("halfMoveClock round-trips through FEN export and import"):
-    import de.nowchess.chess.logic.GameHistory
-    import de.nowchess.chess.notation.FenParser
-    val history = GameHistory(halfMoveClock = 42)
-    val gameState = GameState(
-      piecePlacement  = FenExporter.boardToFen(de.nowchess.api.board.Board.initial),
-      activeColor     = Color.White,
-      castlingWhite   = CastlingRights.Both,
-      castlingBlack   = CastlingRights.Both,
-      enPassantTarget = None,
-      halfMoveClock   = history.halfMoveClock,
-      fullMoveNumber  = 1,
-      status          = GameStatus.InProgress
-    )
-    val fen    = FenExporter.gameStateToFen(gameState)
-    FenParser.parseFen(fen) match
-      case Some(gs) => gs.halfMoveClock shouldBe 42
-      case None     => fail("FEN parsing failed")

modules/core/src/test/scala/de/nowchess/chess/notation/PgnExporterTest.scala

@@ -100,15 +100,3 @@ class PgnExporterTest extends AnyFunSuite with Matchers:
     pgn should include ("e2e4")
     pgn should not include ("=")
   }
-
-  test("exportGame uses Result header as termination marker"):
-    val history = GameHistory()
-      .addMove(HistoryMove(Square(File.E, Rank.R2), Square(File.E, Rank.R4), None))
-    val pgn = PgnExporter.exportGame(Map("Result" -> "1/2-1/2"), history)
-    pgn should endWith("1/2-1/2")
-
-  test("exportGame with no Result header still uses * as default"):
-    val history = GameHistory()
-      .addMove(HistoryMove(Square(File.E, Rank.R2), Square(File.E, Rank.R4), None))
-    val pgn = PgnExporter.exportGame(Map.empty, history)
-    pgn shouldBe "1. e2e4 *"