feat: go idiomatic refactor - #331
Open
lakinwecker wants to merge 87 commits into
Open
Conversation
Drop dead setBoard, move toBoard/goDiagram off the abstract seam, and make goBoardFromFen private[go] so the Position contract no longer leaks joansala-specific types.
Record the architecture decisions behind the pure-Scala Go engine: parallel scala/joansala variants, the Api.Position engine seam, the flat-array board with union-find chains, incremental Zobrist hashing for positional superko, immutable Api with interior mutability, area scoring / stone-selection flow FEN compatibility, and prioritizing rules correctness over joansala parity.
New strategygames.go.engine package: immutable GoState with union-find chain tracking, incremental Zobrist hashing, capture/suicide/simple-ko handling, and legal move generation in the engine-move-int encoding. Zero joansala imports, no Api.scala dependency.
Adds area (Chinese) scoring with dead-stone removal, GoGame turn/result wrapper, and FEN emit/parse byte-identical to the existing format (initial, handicap, and legacy 9-field FENs).
…cala engine Introduce go9x9Scala/go13x13Scala/go19x19Scala (ids 5/6/7, perfIds 503/504/505) as new Go variants that dispatch to the pure-Scala engine via ScalaPosition instead of the JoanSala native engine. Variant.usesScalaEngine flags the dispatch point in Api.position/positionFromVariantNameAndFEN. Extract per-size setup (initialFen, komi, handicap fens) into shared Go9x9Setup/Go13x13Setup/Go19x19Setup traits so both the existing and new variants for a given board size share one definition. Hoist winner/ specialEnd/specialDraw up to Variant as shared defaults, replacing the per-variant overrides duplicated across Go9x9/Go13x13/Go19x19.
Replay.initialFenToSituation and gameMoveToActionStrs called Api.positionFromStartingFenAndMoves / Forsyth.<< without threading the variant through, so new-variant fens were silently replayed against the old JoanSala engine instead of the pure-Scala one. Route both through the variant-aware entry points (positionFromVariantStartingFenAndMoves, Forsyth.<<@) so replay dispatches to the correct engine.
Add GoScalaVariantTest and GoScalaVariantIsometryTest exercising the new go9x9Scala/go13x13Scala/go19x19Scala variants against the pure-Scala engine, plus GoScalaWrapperRoundTripTest verifying the strategygames wrapper layer round-trips fen/moves correctly for the new variants.
Ports 17 of 19 upstream rules tests to the scala engine; the remaining 2 are re-expressed with corrected expectations, tagged where upstream behavior diverges from the ported engine.
Replays 19 games (~1490 plies total, including generated fuzz games and targeted ko/superko/pass scenarios) through the joansala-backed and pure-scala engines, asserting per-ply contract equality. Divergences (empty-region scoring, raw fen capture/pass fields, superko timing, positional vs situational superko) are allowlisted explicitly rather than silently skipped; see docs/adr/0008-0014.
Records seven divergences surfaced by the differential suite: empty-region scoring, raw fen capture/pass reporting, fen fields that don't round-trip, superko timing (forbidden up front vs reported after the move), multi-digit board-row run handling, post-end action replay, and the positional-vs-situational superko decision for the scala engine.
Incidental scalafmt wrapping picked up by sbt bench/scalafmt while working on the go benchmarks; no behavioral change (whitespace/line-wrap only, verified with git diff -w).
Non-JMH GoSmokeTiming for a quick (<90s) sanity check of the go engine replay/applyDrop/legalDrops paths across board sizes and both engines, logging results to CSV. Cheaper than JMH for iterating locally.
GoEngineBenchmark covers replay / applyDrop / legalDrops across the joansala and pure-Scala go engines at 9x9/13x13/19x19, using Level.Invocation for legalDrops so cached lazy-val movegen doesn't mask per-call cost. README documents the workloads and run commands. Baseline (JMH, quick run): legalDrops 3.45-4.80x, applyDrop 1.36-1.51x, replay 2.81-3.76x, new (pure Scala) vs old (joansala).
Score access previously round-tripped through FEN render+reparse on every drop. fenScore is shared by both engines (values identical by construction), so consumers read it directly instead of paying the render/reparse cost. Wires into Board.afterDrop, Replay.replaySelectSquares, and the variant winner/specialDraw checks.
File.of/apply and Rank.of/apply recomputed all.size (an O(19) List walk) on every call. Cache the count once and reuse it.
pieceMap previously rebuilt via a full board scan on every access. Add a precomputed Array[Pos] grid table (posAtGridIndex) for O(1) index-to-Pos lookup, and thread a ParentStones thunk through positionAfter so a child position can inherit its parent's pieceMap and apply a single incremental update instead of rescanning — the parent is dropped once the map is forced. legalDrops now derives from legalActions via Arrays.copyOf minus the trailing pass slot, instead of a full array filter.
GoState.capturedMovesOnLastPlacement replaces the lastCaptured ko var, tracking every stone removed by the last placement instead of only the last chain. ScalaPosition's incremental pieceMap inheritance now extends to capturing drops, removing captured stones from the inherited map instead of falling back to a full board rescan (13x13 applyDrop 14.3->3.7us). Movegen: legalDrops is now a drops-first array exposed uncopied via a private[go] accessor, walked by row/column instead of div/mod, with an inlined emptiness test, a neighbor bitmask that fast-paths points with no enemy contact, and a pseudoLiberties<=4 guard on the capture check (legalDrops scala 1.14/2.50/5.89 -> 0.72/1.62/3.77us at 9x9/13x13/19x19). Final JMH new-vs-old: legalDrops 5.98x/6.99x/7.23x, replay 7.44x/10.1x/12.9x, applyDrop 9.3-10.3x -- >=5x target met on all cells.
Adds scaladoc contracts to the public surface of the pure-Scala go engine (GoState, GoGame, GoFen, Zobrist, AreaScore, ScalaPosition) and NOTE comments at the non-obvious points: XOR-predicted superko hashing, pseudo-liberty exactness for capture detection, the uncopied legalDropsShared array, and the parent-reference drop in ko tracking. Adds docs/go-engine.md as an overview of the engine, linking ADRs 0001-0014. Variant.usesScalaEngine documents the two-engine split; CLAUDE.md's go/ section now names both engine families. bench/README.md documents GoSmokeTiming alongside GoEngineBenchmark. Doc/comment-only: sbt compile clean, strategygames.go.* 300/300 passing.
…and enforce seam invariants The RED bug: a dead-stone selection or `s@` drop naming a coordinate outside this variant's board (e.g. a 19x19-alphabet key on a 9x9 board) aliased onto a real point via Api.uciToMove's modular wraparound, silently lifting or placing a stone the caller never named and corrupting score/winner. Dead stone keys naming no square are now filtered to joansala parity; keys that are not coordinates at all now error. `engineMoveOf` refuses any drop naming no square of the variant rather than aliasing it onto one. Seam invariants enforced alongside, since they live in the same call paths: - GoState.legalDrops now clones (was legalDropsShared, a shared mutable array handed to callers), per ADR 0005. - ScalaPosition.afterLegalUci refuses any action on an already-ended game, per ADR 0013. - A simple ko point loaded from a FEN is now consulted by GoState.isLegal and legal-move generation, not just round-tripped: a state rebuilt from a FEN has no superko history, so the ko field is the only thing left protecting that recapture (consequence of ADR 0010). - GoGame.winningPlayer now derives from gameOutcome's sign rather than comparing gameScore, so it can't disagree with the outcome the aliasing fix above corrects. - gameOutcome/gameScore/p1Score/p2Score move from lazy vals to defs (on both the Api.Position trait and ScalaPosition) so a setKomi after construction cannot leave a stale cached score behind. Api.positionFromStartingFenAndMoves is deprecated: a go FEN carries no variant, so it can only ever rebuild a joansala position, silently switching the ruleset out from under a scala one.
- GoScalaVariantIsometryTest: assert the isometry holds on the valid prefix of the triple-ko sequence, not just that the full sequence is invalid. - GoFenCodecTest: match the specific GoFenError variant instead of a bare isLeft, and assert the loaded ko point is both refused by isLegal and absent from legalMoves. - GoDifferentialTest: add a four-stone-handicap capture case and an off-board-dead-stone-key case (differential coverage for the RED bug fix). - GoStateTest: pin positionHashHistory's zobrist hash to a literal rather than only comparing two computations of it to each other. - GoScalaVariantTest: assert the full key -> perfId map instead of a positional perfId list, and cover the off-board/unparsable dead-stone-key and drop-key behaviors. - GoGameTest/AreaScoreTest/GoScalaWrapperRoundTripTest: fixture and assertion cleanups (error-out on unparsable FENs in test setup instead of falling back to a default, drop a redundant contested-score case, pin the expected UciDump join string).
…rors - GoSmokeTiming: default output path is now bench/target/go-smoke-results.csv instead of a hardcoded scratchpad path from this machine; rewritten to consume GoBoardSize.all (via the new GoCorpusGame helper) instead of its own duplicated SizeFixture/corpora list. - GoSmokeTiming: write with a plain BufferedWriter instead of wrapping it in a PrintWriter, which swallows every IO error and would leave `main` reporting results it never actually wrote. - CorpusFixture: error out when a corpus file's declared turn count disagrees with the number of turns it actually carries, instead of silently truncating or padding. - GoEngineBenchmark: force pieceMap on the parent board explicitly so the drop-under-test isn't charged for a full board scan on first access.
- ADR 0010: note that a loaded ko point is now honoured by GoState.isLegal, since a FEN-rebuilt state has no superko history to fall back on. - go-engine.md: document that a go FEN names only its board size, never its engine, so positionFromFen/FEN.variant always answer joansala and positionFromStartingFenAndMoves is deprecated for the same reason; downstream callers of the variant-less forms need auditing before scala variants go live.
…sing engineMoveOf now matches on shape: pass, a valid on-board placement, or refusal. A token that is neither previously fell through to Api.uciToMove and got silently aliased onto a real point instead of being rejected. Adds a test asserting garbage tokens are refused.
Chain discarded matchers into single expectations in GoStateTest and EngineMoveEncodingTest so a failure on an earlier line is no longer silently absorbed. Rebuild AreaScoreTest's dead-stone case around withoutStones instead of hand-rolled ownership wiring. Rename the isometry test class to match its filename.
…ment playbook Zobrist.tableForSize now signals an unsupported size with require rather than a raw throw. Comments in Api, GoFen, GoState, and Zobrist are corrected to match current behavior, and ADR 0004/0010/0013 are amended where the fixups changed what they described. Adds ADR 0015 (Proposed) recording the open question of variant identity once joansala retires, and a "Retiring joansala" delete-day playbook plus an add-a-size map and the error-handling rule in docs/go-engine.md. Fixes a stale default-path line in bench/README.md.
Records the decision: the pure-Scala go engine takes over canonical ids 1/2/4 (go9x9/go13x13/go19x19); joansala is parked transitionally as go*Joansala (ids 5/6/7, perfIds 503-505) as a differential oracle, then deleted entirely in a single removal commit (including the build.sbt com.joansala go-engine dependency) per the retirement playbook in docs/go-engine.md. Amends 0001, 0007, 0010, and 0014 to point at the flip: every recorded joansala/scala divergence now applies to the canonical variants, and the 0010 round-trip loss on the ko field closes — GoFen.render emits the live simple-ko coordinate once the joansala literal-`-` regex is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ked as oracle Flips go9x9/go13x13/go19x19 (ids 1/2/4) onto the pure-Scala engine (usesScalaEngine=true). The former Go*Scala.scala variants are gone; joansala moves to the new Go*Joansala.scala (ids 5/6/7, perfIds 503-505), which survives only as the differential/benchmark oracle until the removal commit per ADR 0015. Variant.valid now dispatches Api.validateFEN per-variant instead of always validating against joansala's dialect: the scala path parses through GoFen.parse against a regex that accepts ko coordinates, the joansala path is unchanged. The single-arg validateFEN is kept but deprecated. GoFen.render now emits the live simple-ko coordinate when a simple ko is active instead of the joansala-forced literal `-`, closing the ADR 0010 round-trip loss; GoFen.parse reads it back as this engine's only recapture protection when replaying from a bare FEN with no position history. This is a retroactive rules change for stored games under ids 1/2/4 per the 0007/0011/0014 divergence table (superko enforced up front, no repetition flag, real capture/pass counts, etc. — see ADR 0015's amendments) but keeps existing ids, urls, and rating history intact; no app-side migration. Verified: sbt "testOnly strategygames.go.*" 312/312. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-points the go test ledger onto the flipped canonical/joansala keys: - GoScala* suites (GoScalaVariantTest, GoScalaVariantIsometryTest, GoScalaWrapperRoundTripTest) now target the canonical go9x9/go13x13/go19x19 keys instead of the deleted go9x9Scala/etc. - Joansala-oracle suites are tagged "JOANSALA ORACLE" and re-pointed to the parked go*Joansala keys: GoVariantTest's isometry check, GoLongGameTest examples 1-3, and five GoApiTest groups. - GoApiTest's Issue#490, triple-ko, and ko-FEN-fossil cases move to the scala-engine expectations required by ADRs 0011/0014. - GoSituationTest pins raw-fen passCount expectations per ADR 0009 instead of the joansala hardcoded zero. - GoReplayTest's malformed-fen fixture is fixed to a proper 8-field fen. - GoDifferentialTest's canonical/parked mapping swaps (canonical=scala vs parked=joansala) and the ko field comparison is normalised for the new live-ko dialect. - GoFenCodecTest gains a live-ko round-trip case. Verified: sbt "testOnly strategygames.go.*" 312/312 (GoDifferentialTest 34 examples, 0 failures). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The scala engine is now the only go engine. Removes the com.joansala go-engine dependency (aalina is retained for samurai/oware); deletes the parked oracle variants (Go9x9/13x13/19x19 Joansala, ids 5/6/7) and the differential test suite that compared against them — that comparison's record lives in ADRs 0008-0015. Benches switch from differential to absolute-timing mode. Executes ADR 0015.
The fifteen ADRs written during the engine's development described intermediate states (parallel variants, the flip sequencing) that no longer exist at the tip. They are replaced by one ADR recording only the decisions that matter at merge time: 15 files become 1, roughly 800 lines removed. References in docs/go-engine.md, bench/README.md, and the go source and test comments are re-pointed to the new ADR or to the divergence table in docs/go-engine.md, which now carries the per-divergence specifics without per-row ADR citations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Forsyth no longer asks the engine for a position. The board field, ko, scores, komi, pass count, turn and full move number all come from the Board, and FEN.pieces reads the board field back. Three consequences worth naming: - The exported score is variant.areaScore(board), not history.score, which is Score(0, 0) until a drop is played (ADR 0002). A freshly loaded handicap fen scores 810 55 again rather than 0. - History.halfMoveClock now counts every ply rather than only P2's. It is the fen's move counter and the parity the turn field is read from, matching the engine's own plyCount, and it is seeded from the fen. - Variant.createSelectSquares now lifts the named stones from board.pieces (controller ruling R4). GoReplayTest's settled ply moves from 12 stones on the board to 11: the fen always said 11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
validDrops, canDrop, boardAfterPass and boardAfterSelectSquares now answer from Chain, History.positionHashes and the board's own position state. Drop, Pass and SelectSquares derive `after` as a lazy val from situationBefore, so NextBoard, ExplicitBoardAfter and LazyBoardAfter are gone, as are Board.afterDrop, Board.uciMoves, Board.position and Board.apiPosition (controller ruling R11). Superko is probed only where a placement captures: a placement that captures nothing cannot recreate an earlier arrangement. Capture and legality come from a single Chain.capturesUnlessSuicide call per candidate point (R8), so a player in atari can still recapture. Situation.isRepetition is therefore permanently false. Behaviour that changed, deliberately: - The four pass auto settlement now applies on every path, not only the interactive one. A game auto settled in play used to replay as still ongoing. The oracle's four-pass-no-ss game records that divergence at ply 4; its line is corrected by hand, every field checked, and no other game moves. - Every action after a settlement is refused, on both replay paths (R7). The per ply path used to refuse a pass, because the engine did, and accept a drop, because nothing checked. - The batch replay path applies actions through the rules, so it keeps the superko history the live game keeps (R10). The gap example in GoPositionHashTest, written to fail when the gap closed, is deleted. Replay keeps its +1 settlement capture adjustment on top of boardAfterSelectSquares, which does not touch captures (R1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebuilding an engine position per prefix made the comparison quadratic in engine work as well as in rules work. positionsFromVariantStartingFen AndMoves already yields one position per ply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Api's fen helpers move onto Forsyth, FEN and Variant so the engine seam can go. Forsyth gains validate, boardRows and removeDeadStones; Sgf reads its handicap stones from FEN.pieces; Variant.valid becomes the cheap structural invariant instead of a fen round trip through a regex. Forsyth.<<@ now returns None for a fen it cannot read rather than a position that is quietly wrong. A board part whose rank count or row width disagrees with the variant, an unknown stone symbol, a turn field that names no player, a ko point the board size has not, a field count outside the grammar and a non numeric count are all refused. That makes Replay.plyAtFen's invalid fen guard live again. GoForsythTest ports every rejection case GoFenCodecTest pins, and differentially checks validate, FEN.pieces, boardRows, removeDeadStones and the initial fen lookup against the engine backed Api while both exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Delete the batch/per-ply duality. The second path existed only because applying one ply through the engine seam forced a movegen and a pieceMap rebuild; Variant.boardAfter does neither, so the ordinary recursive replay is the only path worth keeping. Gone with it: gameFromUciStringsSlow, gameFromUciStringsPerPly, gameFromBatchedActions, situationAfterAction, legalDropAt, recursiveGamesFromUci and the differential spec that watched the two paths for disagreement. gameFromUciStrings now runs through gameWithActionWhileValid, so every go loader threads History.positionHashes and refuses a superko violation exactly as a live game does. Pinned in GoSuperkoTest. Delete go's plyAtFen, whose body Parser.sans has always made unreachable, and answer at the wrapper with the invalid it always produced. Turn go FEN.variant into an Option so Forsyth.<< and <<< refuse an unsupported board size rather than throwing from inside sys.error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The go rules now live in go/variant/Variant.scala, go/Chain.scala and on Board itself, so Api, ScalaPosition and go/engine have no callers left. Delete them, along with the tests whose subject was the seam, and re-home onto the public API every behavioural fact those tests carried. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The engine and the Api seam are gone, so the benchmarks that measured them are gone with them. What survives measures the production surface: full-game replay, one placement, the legal-drop list, area scoring, and FEN parse and render. Fold them into one GoRulesBenchmark named for its subject, lift the corpus fixtures into GoCorpus, and give GoSmokeTiming its movegen workload back off Situation.dropsAsDrops. Regenerate docs/go-speed-results.md against the joansala baseline. The idiomatic rules replay a full game 2.7x / 3.1x / 3.1x faster than joansala at 9x9 / 13x13 / 19x19, and 37x / 64x / 111x slower than the batch-replay seam they replaced. Roughly 53% / 70% / 79% of that is the strict History.score flood fill, which is the design's own ruling showing up where it was predicted to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… History Go's area score is a flood fill over the whole board, not an accumulated count. It belongs with the other derived caches on go.Board — actors, posMap, piecesOnBoardCount, playerPiecesOnBoardCount — rather than as a strict field on go.History alongside genuinely accumulated captures. Variant.boardAfter and boardAfterSelectSquares stop computing it; winner, specialDraw and the FEN export read board.areaScore. The wrapper takes the score from the board it describes: History.Go(h, areaScore), constructed as History.Go(b.history, b.areaScore). The goHistory implicit conversion goes, because a go.History alone no longer determines a score; it had no callers. ADR 0002's pass invariant now holds structurally: a passed board has the same stones and komi, so it computes the same score. Its Score(0, 0) clause for drop-less games does not survive, and the oracle fixture's 91 pre-scoring plies (of 16,207) still record it — GoOracleTest is red on those cells alone, pending sign-off to regenerate. Details in the task 12b report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The area score is now derived from the board, so the plies before a game's first scoring-bearing action report the position's true area score instead of a placeholder zero. 91 of 16,207 plies change and no others. Only scoreP1 and scoreP2 move; no fen, digest, drop, capture, end or winner field differs anywhere. The old value is 0|0 in all 91, and the new value equals that ply's own fen score in all 91. 85 are ply 0, one per game; the rest are the drop-less prefixes of four-pass-no-ss and pass-drop-pass-settled, the two games curated for the clause this supersedes. history.score and the exported fen now agree at these plies, where Task 9's move of the fen export onto the position had left them disagreeing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
strategygames.Board took its history as a strict constructor val, so Board.Go(b) built History.Go(b.history, b.areaScore) eagerly and every wrapper board flood filled the whole go board whether or not anyone wanted the score. Replay.gameWithUciWhileValid materialises one wrapper game per ply, so wrapper replay paid it 204 times over a 204 ply game. Taking the history by name and holding it in a lazy val defers that to the first read. A 19x19 upstream replay through strategygames.Replay: before 26.23 ms ignoring the score, 25.02 ms reading every ply's score after 4.46 ms ignoring the score, 23.01 ms reading every ply's score 5.9x when the score is not asked for, unchanged when it is. No game logic changes behaviour: constructing a wrapped history is pure, and all nine subclasses pass their expression positionally and are untouched. The other eight gain the same deferral of their own wrapper construction, which for chess also defers two list maps over lastTurn and currentTurn. Reading any other field of a go board's history still forces the score, because History.Go takes it as a strict case class parameter. Removing that too needs either a by-name parameter, which costs History.Go its case class, or a board-carrying History.Go, which the lila History.apply factory cannot construct. Both are larger than this change and neither is attempted here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deriving the area score on Board made the two replay paths cost different things, so one replay number would now hide more than it tells. Measure four: the go package path and the wrapper path, each with the score read and unread. Correct the go section of the bench README, which still named the benchmarks this task renamed and claimed the go corpora were committed, and record two traps that produce a plausible table rather than an error: a second sbt in the same directory corrupts a run's forks, and sbt exits 0 even when JMH dropped the configurations they were running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The go package path replays a full game 9.3x / 16.4x / 29.7x faster than joansala at 9x9 / 13x13 / 19x19, and boardAfter costs 1.44us at 19x19 against joansala's 60.9. Moving the area score onto Board bought 3.4x / 5.2x / 9.5x on that path and 42x on a single placement. Report four replay numbers rather than one. The go package path scores only when something reads Board.areaScore; the wrapper path builds a Game per ply, and reading any history field on one forces that ply's flood fill, which costs 4.7x at 19x19. One number would hide that. Record that two earlier runs were discarded for contamination, so a reader who finds their JSON knows they were rejected rather than lost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 85-game, 16,207-ply fixture was migration scaffolding for verifying the rules rewrite against the engine it was replacing. The engine is gone, so the fixture has nothing left to verify against. Two of its four consumers were pure breadth and are deleted: GoForsythTest's validate sweep and GoScoringTest's recompute sweep both compared values this same code had produced, the latter tautologically since the area score became a derived lazy val. Two carried a cross-check worth keeping and are re-homed onto scripted games covering capture, double capture, ko cycles, pass runs and settlement: GoPositionHashTest's incremental-hash-equals-recompute sweep, and GoBoardStateTest's agreement between board position state and the action log. Both keep their original mechanism and their prefix/swept double replay; only the corpus changed. What is lost is corpus breadth, not any rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comments across the new go code, written to what the names and structure cannot carry: the ko rule's three conditions, why superko is probed only on capturing placements, what Chain.requireVacant prevents, the frontier dedup that no test can observe, why the area score is a lazy val on Board, and each of the four preserved quirks with what fixing it would take. ADR 0003 records the design. ADR 0001 is marked superseded in its structural half only, its rules decisions standing; ADR 0002 is superseded outright, carrying forward its resume ruling, strengthening its pass ruling, and recording that its Score(0,0) clause fell to evidence. docs/go-engine.md is rewritten around the rules rather than the engine, the speed dive is frozen as historical, and CLAUDE.md no longer points at a seam that does not exist. docs/go-refactor.md is the write-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
go-speed-results.md was re-measured at 3e9d0435 after the area score moved off History onto Board, and the shape moved with it: replay is 9.3x/16.4x/29.7x joansala rather than ~3x, 11.7x the deleted batch path rather than 37-111x, and applyDrop at 19x19 is 1.44 us rather than 59.9. areaScore has dropped out of the profile entirely, so the remaining cost is PieceMap/Set[Pos] hashing under the chain walk plus the superko scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Task 8 put the post-settlement refusal on Replay.gameWithActionWhileValid only. pgn.Reader.makeReplayWithActionStrs is the second loader over the same input: its drop branch was safe by accident (Replay.replayDrop goes through Variant.drop, which checks !situation.end) but replayPass and replaySelectSquares built their actions directly, so a pass or a second ss: after a settlement loaded fine there and threw everywhere else. Route both through Situation.pass / Situation.selectSquares, the way replayDrop already routes through Situation.drop, so all three action kinds share one legality decision. GoPostSettlementTest pins the refusal on all five loader entry points. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured on `s@a1 s@e5 pass pass ss:a1`, Go9x9: gameFromUciStrings and pgn.Reader give Score(2,0); Replay.apply(List[Uci]) and situationsFromUci give Score(0,0), as does a game played live. The split is by mechanism — folding action strings versus handing each Uci to a Situation — not by played versus loaded, which is how the comment, both documents and the test all described it. Behaviour is unchanged and stays preserved. GoSettlementCaptureTest now names all four entry points and which group each belongs to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three corrections, all documentation: - Record the fifth instance of "one behaviour, several entry points, one updated" — the post-settlement refusal that reached gameWithActionWhileValid and not pgn.Reader — and narrow the "a fifth would have to be written on purpose" claim to what the structure actually buys. Two instances were introduced during the refactor, both in tasks about the defect itself. - Refusals are not uniformly Validated. The Uci loaders answer Validated.invalid; the action-string loaders sys.error, including past pgn.Reader's own Result.Incomplete channel. An integrator needs Try, not fold. Preserved behaviour, wrongly documented. - Retract the recorded stacked-assertion limitation. Measured on this project: an example whose first statement is `1 === 2` and whose last is `3 === 3` fails and reports `1 != 2` at the first statement's line. specs2 mutable throws, so the limitation does not exist. Also splits the replay section of go-engine.md into its two real shapes, which is where the Uci-list overload had been going unmentioned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…otes - positionHashes goes from permanently empty to load-bearing. If lila does not carry the array through History.apply, every restored go game gets an empty superko history and permits placements a replay refuses — the defect this branch closed inside Replay, relocated to the lila boundary. Stated first, as a precondition, above the source-breaking items. With it: go's 8-byte entries versus the generic Hash.size of 3, which makes strategygames.History.toString meaningless for go, and ~3.2 KB on a 400-ply 19x19 game. - halfMoveClock changed meaning: P2-plies to all plies, so the raw field roughly doubles for the same game. Exported FEN unaffected. - Board.Go.withHistory and copy(history) discard the score they are handed, so strategygames.History.score is unsettable on the go path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
M1: go.format.Sgf.initialMoves called Api.pieceMapFromFen, which required variant.boardSize.height == fen.gameSize and sys.errored otherwise. The replacement, initialFen.pieces, reads the FEN's own row count and uses the global Pos space, so rendering a 19x19 FEN on Go9x9 emitted coordinates for points the board does not have. The patch notes tell lila callers to validate first; this in-repo caller was not updated. Guard restored, GoSgfTest pins both sides. M3: Variant.recreatesAnEarlierPosition's comment read as a proof from a stone-count argument that does not carry it — the count is not monotone across a game. Reworded: the restriction is inherited from the deleted engine and kept for cost, the exhaustiveness argument is named as an argument, and nothing rests on it. M4: record the three-pass FEN round trip as a known limitation. Measured, Go9x9, `pass pass pass`: source board has consecutivePasses 3 and canSelectSquares false; its own exported FEN reloads to 2 and true, and needs two further passes to settle where the source needed one. Preserved, same blocker as the four quirks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Row 4 said "Replay's three loaders vs pgn.Reader", which reads as four loaders total now that quirk 2 names the Uci-list loaders as a separate group that never carried the adjustment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
exportBoard(board) rendered a whole ten-field FEN, turn field included, derived from a bare Board — the last shape inherited from the deleted Api.Position seam, and the only reason Board.playerToMove and Situation.unary_!'s board rewrite existed. It now renders the board part plus the fields that belong to the board (ko, scores, captures, komi, pass count), as togyzkumalak and backgammon do, and >>(game) composes the full FEN from game.situation.player and the board's ply count. The wire format is unchanged: every FEN >> emits is byte-identical, verified by diffing a 478-line corpus dump (initial positions, seven scripted games at every ply, nine handicap games, the eight upstream 19x19 games, each as >>(game), >>(situation), >>(!situation) and a reload round trip) taken before and after. Board.playerToMove and Board.withPlayerToMove are deleted; unary_! is copy(player = !player), as it is in all eight sibling logics. The three EcopeningDB keys move to the board-only shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b6ba4a67 renamed exportBoardFen to exportBoard and narrowed it to the board-only fields, returning a String rather than a FEN. bench is not compiled by sbt test at the root, so the rename left GoRulesBenchmark uncompilable and every go benchmark unrunnable. fenRenderMidGame keeps measuring what it measured, the board's own rendering, now through exportBoard. GoMidGame.fenOf cannot: exportBoard emits eight fields and <<@ requires the turn field and nine or ten, so the mid-game fixture fen now comes from Forsyth.>>(game), which is the full fen the mid-game replay already had in hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it per ply strategygames.Board took its pieces as a strict constructor val, so every Board.Go(b) rebuilt the whole go PieceMap into wrapper Pos and Piece whether or not anyone read it. Replay.gameWithUciWhileValid materialises one wrapper game per ply, so a 19x19 wrapper replay rebuilt a 361 entry map 400 times over a game whose replay path never reads pieces at all. Taking the pieces by name and holding them in a lazy val defers that to the first read, exactly as 1d658123 did for the wrapped history. A 19x19 replay through strategygames.Replay: wrapperReplay 8300.2 -> 3205.6 us/op wrapperReplayReadingEveryScore 38413.9 -> 32947.3 us/op 2.59x when nothing reads the pieces, and 5.5 ms comes off the path that reads a score every ply, which does not read pieces either. No game logic changes behaviour. Board is a sealed abstract class, not a case class, so the by-name parameter costs no subclass its case-class-ness and all nine still pattern match as before. All nine pass their expression positionally and are untouched; each is a pure map over an immutable pieces map into Pos and Piece wrappers, so deferring it can change neither the value nor where an exception could arise. The other eight logics gain the same deferral of their own wrapper construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…claims The lila patch was missing its most dangerous entry: exportBoard and boardAndPlayer return eight and nine fields where they returned ten and eleven, with no compile error at the call site. States what the string now holds, that Forsyth.>>(game) is the unchanged full FEN, that go.format.FEN's accessors misread a board-only string, and why the change was made -- go returning a whole FEN from a bare Board was the anomaly among nine logics, and removing it is what let exportBoardFen, Board.playerToMove and Board.withPlayerToMove go. The performance sections now lead with the wrapper figures. lila runs the wrapper path, so 11.3x / 2.4x against joansala describe production and 29.7x measures the rules alone; which of the two wrapper numbers is live depends on whether lila reads a score per ply, which nobody here can answer. The two wrapper causes are named as the follow-up branch's first two items. Claims corrected against the tree: - the halfMoveClock entry named Board.playerToMove and a private Forsyth.playerToMove, both deleted; only Forsyth.fullMovePart still derives from the field - "nothing outside go/Board.scala assigns position state" -- Forsyth.<<@ sets ko, pass count and settlement when it builds a board from a FEN - the settlement capture group is every action-string loader, not two: gameWithUciWhileValid and Replay.apply(actionStrs) carry it too, the latter discarding it through the first-action state bug - Api.stonePocketData was private[go] and Option-wrapped - legal-drop generation costs 17-21x the seam's lazy list; the old wording read as faster - GoPostSettlementTest names five loaders, not the played path - komi: Variant.komi is only where a fresh board starts Recorded as known issues: Replay.apply(actionStrs).state returns the first action's game, identically in seven game logics; a class-init deadlock between Hex5$/Hex6$ and BoardType$ that hung a suite run for 45 minutes; and Ecopening.fromGame being dead for go behind the Parser.sans stub. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a3dc35a2 landed while the previous commit was being written and closes the largest wrapper cost both documents had just named as open: strategygames.Board took its pieces as a strict constructor argument, so every Board.Go(b) rebuilt the whole go piece map per ply. Its own before/after pair at 19x19 is 8300.2 -> 3205.6 us/op unread and 38413.9 -> 32947.3 us/op reading a score every ply. Every row that moved is marked as pre-a3dc35a2 rather than restated, because its baselines come from a different run than the dd250768 table: both wrapperReplay rows, the wrapperReplay allocation row, and the wrapper-path profile. The go package rows are untouched -- the change is one line in strategygames/Board.scala. The production figures are retrued to about 21x and 2.8x joansala, marked as arithmetic over a3dc35a2's numbers rather than a measurement, and the spread between them is 7.5x rather than 4.7x: removing a fixed per-ply cost shrinks the cheaper path proportionally more, so the open question of whether lila reads a score per ply matters more now, not less. The follow-up list drops to one item, History.Go's strict score, with the note that Board could take a by-name parameter cheaply because it is not a case class and History.Go cannot. c3ad23ac is recorded too: fenRenderMidGame now renders the eight-field board-only string rather than a ten-field FEN, so that row is not comparable across it, and b6ba4a67 had left GoRulesBenchmark uncompilable because bench is not built by sbt test at the root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Folding the branch's fixup commits replayed task 9's Forsyth changes over task 8's, and task 8's fix for review finding I2 was lost in the conflict resolution: fen.ply fell back to 0 again, silently disagreeing with the fen's own turn field where it should fail. The gate task 9 added to <<@ makes this unreachable for any fen that parses, so no behaviour changes - but a silent fallback is what the review objected to and an unreachable invariant should still say what it assumes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
879d0977's commit message carried an "after" pair that was written before it was measured. Both documents took it from there. The measured pair, read from the paired run's result JSON, is 8300.2 -> 3205.6 us/op on wrapperReplay at 19x19 (2.59x) and 38413.9 -> 32947.3 on wrapperReplayReadingEveryScore (1.17x). The true numbers change what the documents say, not only their digits. wrapperReplay at 19x19 now measures 3205.6 +/- 25.3 against the go package's own replay at 3243.2 +/- 156.3, so the wrapper's overhead on a replay that reads no score is gone rather than reduced, and the 5.25 ms the tables charged to the wrapper was this one map rebuild. Production against joansala moves to 30.1x on the score-unread row and 2.9x on the score-read row, both recomputed and both stated. The spread between the two wrapper rows is 10.3x, which is now entirely History.Go's strict area score and the only follow-up item left. bench is aggregated by no root task, so 7c69c51's rename of exportBoardFen left every go benchmark uncompilable until 8904ce5 with nothing red. Recorded in bench/README.md, which had the same rename stale in its own table, and in the follow-up notes, since the performance branch will live there. Every commit hash the documents cite was a pre-rebase hash that still resolves through unreferenced objects while being unreachable from HEAD. All five now point at the commits on the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The independent conformance review of src/main/scala/go/ against togyzkumalak, backgammon, abalone and dameo lived under .superpowers/sdd/, which is gitignored, so a reviewer of this branch could never see it. Published verbatim under docs/ and linked from docs/go-refactor.md. The review was taken at the tree of 34b36e5 and thirteen commits have landed since, six of them code, so every one of its 12 residue items and 5 justified deviations was re-checked against be190b3 and annotated with its status. Annotations are blockquotes; the reviewer's text is untouched. R1 - exportBoard exporting a whole ten-field FEN from a bare Board, and the Board.playerToMove / withPlayerToMove / bespoke Situation.unary_! that hung off it - is fixed by 7c69c51. One half of its prescription was not taken: the full-move field still comes from board.history.halfMoveClock rather than game.fullTurnCount, and the annotation says so. The other eleven residue items are open, each re-verified at its current line. JD5 widened rather than closed: b79b20b added a second by-name parameter to strategygames.Board, so the idiom is no longer go-shaped, but the History.Go arity asymmetry the reviewer called the worse half is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.