fix(pgn): add SAN disambiguation and check/checkmate suffixes [NCS-42] (#56)

Two bugs in move notation causing PGN import failures in LiChess:

1. Disambiguation: when two pieces of same type can reach same square,
   SAN requires file/rank/full-square prefix (e.g. "Ndf3" not "Nf3").
   Added disambiguate() in PgnExporter and disambiguatePiece() in
   GameEngine, both querying allLegalMoves to find competing pieces.

2. Check/checkmate suffix: "+" and "#" were never appended.
   PgnExporter now threads ctxAfter through moveToAlgebraic and
   calls DefaultRules.isCheck/isCheckmate. GameEngine passes
   PostMoveStatus to translateMoveToNotation for the same result.

Also removes dead notation code in executeMoveBody (result was never
used — not passed to MoveExecutedEvent).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Janis Eccarius <eccariusjanis@gmail.com>
Reviewed-on: #56

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

@@ -418,7 +418,6 @@ class GameEngine(
     val contextBefore = currentContext
     val nextContext   = ruleSet.applyMove(currentContext)(move)
     val captured      = computeCaptured(currentContext, move)
-    val notation      = translateMoveToNotation(move, contextBefore.board)
     currentContext = nextContext
 
     advanceClock(contextBefore.turn)
@@ -463,13 +462,18 @@ class GameEngine(
       redoStack = Nil
     else if status.isCheck then notifyObservers(CheckDetectedEvent(currentContext))
 
-  private def translateMoveToNotation(move: Move, boardBefore: Board): String =
-    move.moveType match
+  private def translateMoveToNotation(move: Move, ctxBefore: GameContext, status: PostMoveStatus): String =
+    val suffix =
+      if status.isCheckmate then "#"
+      else if status.isCheck then "+"
+      else ""
+    val base = move.moveType match
       case MoveType.CastleKingside    => "O-O"
       case MoveType.CastleQueenside   => "O-O-O"
       case MoveType.EnPassant         => enPassantNotation(move)
       case MoveType.Promotion(pp)     => promotionNotation(move, pp)
-      case MoveType.Normal(isCapture) => normalMoveNotation(move, boardBefore, isCapture)
+      case MoveType.Normal(isCapture) => normalMoveNotation(move, ctxBefore, isCapture)
+    base + suffix
 
   private def enPassantNotation(move: Move): String =
     s"${move.from.file.toString.toLowerCase}x${move.to}"
@@ -482,16 +486,31 @@ class GameEngine(
       case PromotionPiece.Knight => "N"
     s"${move.to}=$ppChar"
 
-  private[engine] def normalMoveNotation(move: Move, boardBefore: Board, isCapture: Boolean): String =
-    boardBefore.pieceAt(move.from).map(_.pieceType) match
+  private[engine] def normalMoveNotation(move: Move, ctxBefore: GameContext, isCapture: Boolean): String =
+    ctxBefore.board.pieceAt(move.from).map(_.pieceType) match
       case Some(PieceType.Pawn) =>
         if isCapture then s"${move.from.file.toString.toLowerCase}x${move.to}"
         else move.to.toString
       case Some(pt) =>
         val letter = pieceNotation(pt)
-        if isCapture then s"${letter}x${move.to}" else s"$letter${move.to}"
+        val d      = disambiguatePiece(move.from, move.to, pt, ctxBefore)
+        if isCapture then s"$letter${d}x${move.to}" else s"$letter$d${move.to}"
       case None => move.to.toString
 
+  private def disambiguatePiece(from: Square, to: Square, pieceType: PieceType, ctx: GameContext): String =
+    if pieceType == PieceType.King then ""
+    else
+      val competitors = ruleSet
+        .allLegalMoves(ctx)
+        .filter(m => m.to == to && m.from != from && ctx.board.pieceAt(m.from).exists(_.pieceType == pieceType))
+      if competitors.isEmpty then ""
+      else
+        val sameFile = competitors.exists(_.from.file == from.file)
+        val sameRank = competitors.exists(_.from.rank == from.rank)
+        if !sameFile then from.file.toString.toLowerCase
+        else if !sameRank then (from.rank.ordinal + 1).toString
+        else from.toString
+
   private[engine] def pieceNotation(pieceType: PieceType): String =
     pieceType match
       case PieceType.Knight => "N"
@@ -519,9 +538,10 @@ class GameEngine(
     if currentContext.moves.isEmpty then
       notifyObservers(InvalidMoveEvent(currentContext, InvalidMoveReason.NothingToUndo))
     else
-      val lastMove = currentContext.moves.last
-      val prevCtx  = replayContextFromMoves(currentContext.moves.dropRight(1))
-      val notation = translateMoveToNotation(lastMove, prevCtx.board)
+      val lastMove   = currentContext.moves.last
+      val prevCtx    = replayContextFromMoves(currentContext.moves.dropRight(1))
+      val postStatus = ruleSet.postMoveStatus(currentContext)
+      val notation   = translateMoveToNotation(lastMove, prevCtx, postStatus)
       redoStack = lastMove :: redoStack
       currentContext = prevCtx
       notifyObservers(MoveUndoneEvent(currentContext, notation))

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

@@ -1,6 +1,6 @@
 package de.nowchess.chess.engine
 
-import de.nowchess.api.board.{Board, Color, File, PieceType, Rank, Square}
+import de.nowchess.api.board.{Color, File, PieceType, Rank, Square}
 import de.nowchess.api.game.GameContext
 import de.nowchess.api.move.{Move, MoveType, PromotionPiece}
 import de.nowchess.chess.observer.{GameEvent, InvalidMoveEvent, InvalidMoveReason, Observer}
@@ -137,7 +137,7 @@ class GameEngineIntegrationTest extends AnyFunSuite with Matchers:
 
   test("normalMoveNotation handles missing source piece"):
     val engine = new GameEngine(ruleSet = DefaultRules)
-    val result = engine.normalMoveNotation(Move(sq("e3"), sq("e4")), Board.initial, isCapture = false)
+    val result = engine.normalMoveNotation(Move(sq("e3"), sq("e4")), GameContext.initial, isCapture = false)
 
     result shouldBe "e4"
 

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

@@ -106,7 +106,7 @@ class GameEngineNotationTest extends AnyFunSuite with Matchers:
     engine.undo()
 
     val evt = events.collect { case e: MoveUndoneEvent => e }.head
-    evt.pgnNotation shouldBe "e8=B"
+    evt.pgnNotation shouldBe "e8=B+"
 
   // ── King normal move notation (line 246) ───────────────────────────
 
@@ -134,3 +134,87 @@ class GameEngineNotationTest extends AnyFunSuite with Matchers:
     val evt = events.collect { case e: MoveUndoneEvent => e }.head
     evt.pgnNotation should startWith("K")
     evt.pgnNotation should include("f1")
+
+  // ── Disambiguation: two knights same rank (file suffix) ────────────
+
+  test("undo with two knights on same rank disambiguates by file"):
+    // White knights on b1 and f3, black pawn on h7 prevents draw, both knights can reach d2
+    val board = FenParser.parseBoard("k7/7p/8/8/8/5N2/8/1N5K").get
+    val ctx = GameContext.initial
+      .withBoard(board)
+      .withTurn(Color.White)
+      .withCastlingRights(de.nowchess.api.board.CastlingRights(false, false, false, false))
+
+    val engine = new GameEngine(ctx, DefaultRules)
+    val events = captureEvents(engine)
+
+    // Knight on b1 moves to d2; notation must be "Nbd2" to disambiguate from Nf3
+    engine.processUserInput("b1d2")
+    events.clear()
+    engine.undo()
+
+    val evt = events.collect { case e: MoveUndoneEvent => e }.head
+    evt.pgnNotation should startWith("Nb")
+    evt.pgnNotation should include("d2")
+
+  // ── Disambiguation: two knights same file (rank suffix) ────────────
+
+  test("undo with two knights on same file disambiguates by rank"):
+    // White knights on b1 and b3, both can reach d2 or c5; use b1->d2
+    val board = FenParser.parseBoard("k7/7p/8/8/8/1N6/8/1N5K").get
+    val ctx = GameContext.initial
+      .withBoard(board)
+      .withTurn(Color.White)
+      .withCastlingRights(de.nowchess.api.board.CastlingRights(false, false, false, false))
+
+    val engine = new GameEngine(ctx, DefaultRules)
+    val events = captureEvents(engine)
+
+    // Knight on b1 moves to d2; notation must be "N1d2" to disambiguate from b3
+    engine.processUserInput("b1d2")
+    events.clear()
+    engine.undo()
+
+    val evt = events.collect { case e: MoveUndoneEvent => e }.head
+    evt.pgnNotation should include("1")
+    evt.pgnNotation should include("d2")
+
+  // ── Check suffix (+) ───────────────────────────────────────────────
+
+  test("undo after move that gives check emits notation with + suffix"):
+    // White rook a1, white king h1, black king e8; Ra1-e1 gives check on e-file
+    val board = FenParser.parseBoard("4k3/8/8/8/8/8/8/R6K").get
+    val ctx = GameContext.initial
+      .withBoard(board)
+      .withTurn(Color.White)
+      .withCastlingRights(de.nowchess.api.board.CastlingRights(false, false, false, false))
+
+    val engine = new GameEngine(ctx, DefaultRules)
+    val events = captureEvents(engine)
+
+    engine.processUserInput("a1e1")
+    events.clear()
+    engine.undo()
+
+    val evt = events.collect { case e: MoveUndoneEvent => e }.head
+    evt.pgnNotation should endWith("+")
+
+  // ── Checkmate suffix (#) ──────────────────────────────────────────
+
+  test("undo after checkmate move emits notation with # suffix"):
+    // Fool's mate setup (before final move): 1.f3 e5 2.g4 -- black plays Qd8-h4#
+    val board = FenParser.parseBoard("rnbqkbnr/pppp1ppp/8/4p3/6P1/5P2/PPPPP2P/RNBQKBNR").get
+    val ctx = GameContext.initial
+      .withBoard(board)
+      .withTurn(Color.Black)
+      .withCastlingRights(de.nowchess.api.board.CastlingRights(true, true, true, true))
+
+    val engine = new GameEngine(ctx, DefaultRules)
+    val events = captureEvents(engine)
+
+    engine.processUserInput("d8h4")
+    events.clear()
+    engine.undo()
+
+    val evt = events.collect { case e: MoveUndoneEvent => e }.head
+    evt.pgnNotation should endWith("#")

modules/io/src/main/scala/de/nowchess/io/pgn/PgnExporter.scala

@@ -31,7 +31,9 @@ object PgnExporter extends GameContextExport:
       if moves.isEmpty then ""
       else
         val contexts = moves.scanLeft(GameContext.initial)((ctx, move) => DefaultRules.applyMove(ctx)(move))
-        val sanMoves = moves.zip(contexts).map { case (move, ctx) => moveToAlgebraic(move, ctx.board) }
+        val sanMoves = moves.zip(contexts).zip(contexts.tail).map { case ((move, ctxBefore), ctxAfter) =>
+          moveToAlgebraic(move, ctxBefore, ctxAfter)
+        }
 
         val groupedMoves = sanMoves.zipWithIndex.groupBy(_._2 / 2)
         val moveLines = for (moveNumber, movePairs) <- groupedMoves.toList.sortBy(_._1) yield
@@ -48,9 +50,24 @@ object PgnExporter extends GameContextExport:
     else if moveText.isEmpty then headerLines
     else s"$headerLines\n\n$moveText"
 
-  /** Convert a Move to Standard Algebraic Notation using the board state before the move. */
-  private def moveToAlgebraic(move: Move, boardBefore: Board): String =
-    move.moveType match
+  private def disambiguate(from: Square, to: Square, pieceType: PieceType, ctx: GameContext): String =
+    val competitors = DefaultRules
+      .allLegalMoves(ctx)
+      .filter(m => m.to == to && m.from != from && ctx.board.pieceAt(m.from).exists(_.pieceType == pieceType))
+    if competitors.isEmpty then ""
+    else
+      val sameFile = competitors.exists(_.from.file == from.file)
+      val sameRank = competitors.exists(_.from.rank == from.rank)
+      if !sameFile then from.file.toString.toLowerCase
+      else if !sameRank then (from.rank.ordinal + 1).toString
+      else from.toString
+
+  private def moveToAlgebraic(move: Move, ctxBefore: GameContext, ctxAfter: GameContext): String =
+    val suffix =
+      if DefaultRules.isCheckmate(ctxAfter) then "#"
+      else if DefaultRules.isCheck(ctxAfter) then "+"
+      else ""
+    val base = move.moveType match
       case MoveType.CastleKingside  => "O-O"
       case MoveType.CastleQueenside => "O-O-O"
       case MoveType.EnPassant       => s"${move.from.file.toString.toLowerCase}x${move.to}"
@@ -60,18 +77,19 @@ object PgnExporter extends GameContextExport:
           case PromotionPiece.Rook   => "=R"
           case PromotionPiece.Bishop => "=B"
           case PromotionPiece.Knight => "=N"
-        val isCapture = boardBefore.pieceAt(move.to).isDefined
+        val isCapture = ctxBefore.board.pieceAt(move.to).isDefined
         if isCapture then s"${move.from.file.toString.toLowerCase}x${move.to}$promSuffix"
         else s"${move.to}$promSuffix"
       case MoveType.Normal(isCapture) =>
         val dest   = move.to.toString
         val capStr = if isCapture then "x" else ""
-        boardBefore.pieceAt(move.from).map(_.pieceType).getOrElse(PieceType.Pawn) match
+        ctxBefore.board.pieceAt(move.from).map(_.pieceType).getOrElse(PieceType.Pawn) match
           case PieceType.Pawn =>
             if isCapture then s"${move.from.file.toString.toLowerCase}x$dest"
             else dest
-          case PieceType.Knight => s"N$capStr$dest"
-          case PieceType.Bishop => s"B$capStr$dest"
-          case PieceType.Rook   => s"R$capStr$dest"
-          case PieceType.Queen  => s"Q$capStr$dest"
+          case PieceType.Knight => s"N${disambiguate(move.from, move.to, PieceType.Knight, ctxBefore)}$capStr$dest"
+          case PieceType.Bishop => s"B${disambiguate(move.from, move.to, PieceType.Bishop, ctxBefore)}$capStr$dest"
+          case PieceType.Rook   => s"R${disambiguate(move.from, move.to, PieceType.Rook, ctxBefore)}$capStr$dest"
+          case PieceType.Queen  => s"Q${disambiguate(move.from, move.to, PieceType.Queen, ctxBefore)}$capStr$dest"
           case PieceType.King   => s"K$capStr$dest"
+    base + suffix

modules/io/src/test/scala/de/nowchess/io/pgn/PgnExporterTest.scala

@@ -112,3 +112,38 @@ class PgnExporterTest extends AnyFunSuite with Matchers:
     pgn should include("exf8=Q")
     pawnCapturePgn should include("exd3")
     quietPromotionPgn should include("e8=Q")
+
+  test("exportGame disambiguates when two knights can reach same square"):
+    // 1.Nf3 a6 2.d3 a5 3.Nfd2: d3 clears d2 so both Nb1 and Nf3 can reach d2; must emit "Nfd2"
+    val moves = List(
+      Move(sq("g1"), sq("f3")),
+      Move(sq("a7"), sq("a6")),
+      Move(sq("d2"), sq("d3")),
+      Move(sq("a6"), sq("a5")),
+      Move(sq("f3"), sq("d2")),
+    )
+    val pgn = PgnExporter.exportGame(Map.empty, moves)
+    pgn should include("Nfd2")
+
+  test("exportGame appends + after move that gives check"):
+    // 1.e4 e5 2.Qh5 Nc6 3.Qxf7+ — queen captures f7, gives check to black king on e8
+    val moves = List(
+      Move(sq("e2"), sq("e4")),
+      Move(sq("e7"), sq("e5")),
+      Move(sq("d1"), sq("h5")),
+      Move(sq("b8"), sq("c6")),
+      Move(sq("h5"), sq("f7"), MoveType.Normal(isCapture = true)),
+    )
+    val pgn = PgnExporter.exportGame(Map.empty, moves)
+    pgn should include("Qxf7+")
+
+  test("exportGame appends # after checkmate move"):
+    // Fool's mate: 1.f3 e5 2.g4 Qh4#
+    val moves = List(
+      Move(sq("f2"), sq("f3")),
+      Move(sq("e7"), sq("e5")),
+      Move(sq("g2"), sq("g4")),
+      Move(sq("d8"), sq("h4")),
+    )
+    val pgn = PgnExporter.exportGame(Map("Result" -> "*"), moves)
+    pgn should include("Qh4#")