Drop CountKingSafetyDefects from board-representation migration plan

Descope CountKingSafetyDefects entirely, per discussion after landing
GetAttacks's half of section 3: the two functions no longer share
enough to justify one plan. CountKingSafetyDefects (eval.c:2325) turns
out to do no ray-walk/blocker check at all -- it's an unblocked
CHECK_VECTOR proximity heuristic, not a true attack query -- so
_WhoAttacksSquareBB's blocker-aware result isn't a value-identical
drop-in for it; making it bitboard-backed would be a real behavior
change (needing eval re-tuning/re-gating), not a reimplementation, and
a materially different, riskier project than this one.

eval.c is untouched. If CountKingSafetyDefects work happens later, it
should be a new, separate migration document starting from its actual
(unblocked heuristic) behavior, not a resumption of this one.

Also updates section 4-7 status notes to reflect what's actually done
vs. still blocked on section 6's toggle (GetAttacks alone, no longer
entangled with a king-safety timeline).

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Jntky4yGUTyQVaGCXms4F2

1 files changed, 147 insertions, 131 deletions

diff --git a/src/board_representation/MIGRATION.md b/src/board_representation/MIGRATION.md
index f13aa9b..f8bccb1 100644
--- a/src/board_representation/MIGRATION.md
+++ b/src/board_representation/MIGRATION.md
@@ -1,13 +1,24 @@
-# Migration plan: bitboard-backed `GetAttacks` and `CountKingSafetyDefects`
+# Migration plan: bitboard-backed `GetAttacks`
 
-**Status: sections 1-2 (bbPieces) and section 3's `GetAttacks` half
-(bbPieces/bbPawns-backed `_WhoAttacksSquareBB`/`_GetAttacksBB`, not yet
-wired into the `GetAttacks` macro) implemented and verified, ready to
-commit. `CountKingSafetyDefects`'s half of section 3 deliberately not
-started -- see its own status note below for why it needs a different
-design, not just a second consumer of the same primitive. Sections 4
-(partially done, folded into section 3's own testing)-8 otherwise not
-started.** See the per-section status notes below for specifics.
+**Scope note (post-section-3): `CountKingSafetyDefects` has been
+removed from this plan.** It was originally included as a second
+consumer of the same shared primitive, but turned out not to fit --
+see the "Why `CountKingSafetyDefects` was dropped" section right after
+this one for the full reasoning. Its code is untouched and left
+exactly as it was; if it's ever revisited, that should be a new, separate
+migration document, not a resumption of this one -- the two functions
+no longer have enough in common (beyond both reading `bbPieces`) to
+justify sharing a plan, a toggle strategy, or a retirement checklist.
+Every section below has been edited to drop `CountKingSafetyDefects`
+references accordingly; where useful, dropped content is preserved
+inline as a note rather than deleted, so a future session designing
+the separate migration doesn't have to rediscover it from git history.
+
+**Status: sections 1-2 (`bbPieces`) and section 3 (`bbPieces`/`bbPawns`-backed
+`_WhoAttacksSquareBB`/`_GetAttacksBB`, not yet wired into the
+`GetAttacks` macro) implemented, verified, and committed. Sections 4
+(partially done, folded into section 3's own testing)-8 not started.**
+See the per-section status notes below for specifics.
 
 Context: a prior pass of this session added occupancy-bitboard-driven
 mobility/attack-presence bitboards (`bbPawnAttacks`, `bbMinorAttacks`,
@@ -24,29 +35,76 @@ both representations and nothing was ever removed to pay for it.
 
 That result reframed the question: the win isn't "add more bitboards to
 Eval," it's picking the *right* bitboards in the *right* place. This
-document is the plan for that -- two functions sharing one new bitboard
-primitive:
+document is the plan for that, focused on one function:
+
+- **`GetAttacks`** (the SEE support routine in `see.c` / `x86.asm` /
+  `x64.asm`), called far more often per node than `Eval()` -- every
+  capture move considered in move ordering, at every node, via
+  `generate.c`'s 5 call sites and `searchsup.c:365`. It does a manual
+  mailbox ray-walk over every one of a side's non-pawn pieces (with a
+  `CHECK_VECTOR` geometry-table lookup to quickly reject misaligned
+  ones) to answer "does this piece attack the move's destination
+  square" -- O(pieces) candidates x O(ray length) per call, regardless
+  of how close any given piece actually is to the target square. A
+  strong candidate for a bitboard-backed rewrite because it reduces to
+  a simple, well-defined query: "which of this side's pieces attack
+  square X."
+
+## Why `CountKingSafetyDefects` was dropped
+
+`CountKingSafetyDefects` (`eval.c:2408`) was originally included here
+too, on the assumption that it does the same kind of ray-walk as
+`GetAttacks` against a different target square (the king's, instead of
+a move's destination) -- called not just from `Eval()`
+(`eval.c:7386-7387`, `eval.c:2549-2550`) but directly from search-tree
+pruning/extension decisions (`search.c:1252`, `searchsup.c:484`,
+`searchsup.c:902,905`), making it if anything a *more* pervasive case
+of the same problem.
+
+That assumption turned out to be wrong once the actual function body
+(`eval.c:2325`) was read carefully while implementing `GetAttacks`'s
+half of section 3: `CountKingSafetyDefects` does **no ray-walk and no
+blocker/occlusion check at all**. It's a `CHECK_VECTOR_WITH_INDEX`
+table lookup testing whether each enemy piece's *type* geometrically
+aligns with one of three squares near the king (`cKing-1, cKing,
+cKing+1`), regardless of what's in between -- a deliberately
+approximate proximity/alignment heuristic, not a true "does this piece
+attack this square" query.
+
+That mismatch matters for two reasons this plan's shared-primitive
+premise depended on:
 
-1. **`GetAttacks`** (the SEE support routine in `see.c` / `x86.asm` /
-   `x64.asm`), called far more often per node than `Eval()` -- every
-   capture move considered in move ordering, at every node, via
-   `generate.c`'s 5 call sites and `searchsup.c:365`.
-2. **`CountKingSafetyDefects`** (`eval.c:2408`), which turns out to be an
-   even more pervasive case of the same problem: it's called not just
-   from `Eval()` (`eval.c:7386-7387`, `eval.c:2549-2550`) but directly
-   from search-tree pruning/extension decisions -- `search.c:1252`,
-   `searchsup.c:484`, `searchsup.c:902,905` -- meaning it runs at search
-   nodes regardless of whether `Eval()` is even reached at that node.
+1. **No shared code to share.** `_WhoAttacksSquareBB` (built for
+   `GetAttacks`) is blocker-aware by construction -- that's the whole
+   point of a ray-walk-replacement primitive. Feeding
+   `CountKingSafetyDefects` a blocker-aware result would make it *more
+   accurate* than today, not a value-identical reimplementation. A
+   correct bitboard version of `CountKingSafetyDefects` would need to
+   stay unblocked (just reformulate the existing `cNonPawns` array walk
+   as a `bbPieces` bit walk), which shares `bbPieces` as a data source
+   but not `_WhoAttacksSquareBB` as a function -- there is no longer a
+   single primitive both functions consume, so this plan's core
+   framing ("two functions sharing one new bitboard primitive") doesn't
+   hold.
+2. **Different, harder-to-satisfy correctness/gating shape.** Because
+   the two functions would no longer share an implementation, testing
+   them "jointly" (as section 4 originally proposed) stops being
+   representative of anything -- each needs its own correctness sweep,
+   its own benchmark, its own curated-suite/`match_play.py` gate, on
+   its own schedule. And because `CountKingSafetyDefects` feeds an
+   `Eval()` score term in addition to pruning gates, any *intentional*
+   change to its approximation (e.g. making it blocker-aware on
+   purpose, as a real improvement rather than an accidental one) is an
+   eval-tuning-shaped change, not a reimplementation -- a materially
+   different, riskier project than this one.
 
-Both do the exact same thing algorithmically: walk every one of a side's
-non-pawn pieces with a geometry-table lookup (`CHECK_VECTOR`) and, for
-sliders, a manual mailbox ray-walk, to answer "does this piece attack (or
-point at) this particular square" -- `GetAttacks`'s target is the move's
-destination square, `CountKingSafetyDefects`'s target is the king's
-square. O(pieces) candidates x O(ray length), every call, regardless of
-how close any given piece actually is to the target square. Both are
-strong candidates for the same fix, because both reduce to the same
-underlying query: "which of this side's pieces attack square X."
+Given that, bundling it into this plan added risk and complexity to
+the `GetAttacks` work for no shared benefit. It's out of scope now;
+`eval.c`'s `CountKingSafetyDefects` is untouched. A future migration
+for it should start from scratch with its actual behavior (the
+unblocked proximity heuristic above) as the documented baseline, not
+from this document's original "shares a primitive with `GetAttacks`"
+premise.
 
 ## -1. Current repo state: what's committed, what's stashed
 
@@ -111,7 +169,13 @@ leave it in the stash unless it's wanted on its own merits later.
 
 In scope: add incrementally-maintained per-color, per-piece-type location
 bitboards to `POSITION`, and a bitboard-driven "who attacks square X"
-primitive that both `GetAttacks` and `CountKingSafetyDefects` consume.
+primitive that `GetAttacks` consumes.
+
+Explicitly not in scope (see "Why `CountKingSafetyDefects` was
+dropped" above): reimplementing `CountKingSafetyDefects`. It was
+in scope in an earlier version of this document; removed once
+implementing `GetAttacks`'s half revealed the two functions don't
+actually share enough to justify one plan.
 
 Explicitly not in scope: changing `MOVE`'s `cFrom:8`/`cTo:8` encoding or
 `COOR`'s `0x88` numbering. That would shrink `g_HistoryCounters`,
@@ -206,9 +270,7 @@ behavioral risk, exactly as planned. **Not yet committed** -- sitting on
 disk, verified, ready to commit whenever section 1 is considered a good
 checkpoint.
 
-## 3. New `_WhoAttacksSquareBB` primitive -- `GetAttacks` half DONE, `CountKingSafetyDefects` half not started
-
-### `GetAttacks` half -- DONE, verified correct, verified faster than asm
+## 3. New `_WhoAttacksSquareBB` primitive -- DONE, verified correct, verified faster than asm
 
 Implemented in `see.c`: `_WhoAttacksSquareBB(pos, cSquare, uSide,
 bbOccupied)` returns a bitboard of every uSide knight/bishop/rook/
@@ -352,34 +414,10 @@ actually run (not just to pass)**, both pre-existing, both unrelated to
    is unrelated scope to this migration and risks masking whether a
    real regression exists if conflated with this work's own testing.
 
-### `CountKingSafetyDefects` half -- NOT STARTED, needs a different design than planned
-
-While implementing the `GetAttacks` half, re-reading the actual
-`CountKingSafetyDefects` body (`eval.c:2325`) revealed the original
-plan's premise for this half is wrong: the plan assumed both functions
-do "a mailbox ray-walk... to answer 'does this piece attack this
-square'", making them two consumers of one shared blocked-attack
-primitive. In fact `CountKingSafetyDefects` does **no ray-walk and no
-blocker/occlusion check at all** -- it's a `CHECK_VECTOR_WITH_INDEX`
-table lookup testing whether each enemy piece's *type* geometrically
-aligns with one of three squares near the king (`cKing-1, cKing,
-cKing+1`), regardless of what's in between. It's a deliberately
-approximate proximity/alignment heuristic, not a true attack query.
-
-Feeding it `_WhoAttacksSquareBB`'s real, blocker-aware result would
-silently make it *more accurate* than today -- a behavior change (and
-one that would need eval re-tuning + re-gating), not a value-identical
-reimplementation, which breaks section 4's "identical behavior" testing
-premise for this half specifically. Decided direction (not yet
-implemented): two separate functions instead of one shared code path --
-`_WhoAttacksSquareBB` stays `GetAttacks`-only; a `_CountKingSafetyDefectsBB`
-would keep the existing unblocked `CHECK_VECTOR` logic bit-for-bit,
-just iterate `bbPieces[xside][*]` bits instead of walking `cNonPawns[
-xside][]` to get the same enemy-piece-square list -- same "share
-`bbPieces` as the substrate, not a literal ray-walk" relationship as
-`GetAttacks`, just without a shared primitive function. Not started;
-next session should implement this shape directly rather than
-revisiting the original one-primitive premise.
+(`CountKingSafetyDefects` was originally planned as a second consumer
+of this primitive -- see "Why `CountKingSafetyDefects` was dropped"
+near the top of this document for why that's no longer part of this
+plan.)
 
 ## 4. Correctness verification
 
@@ -402,16 +440,15 @@ sequence.
    every-square x both-colors sweep already in place
    (`GenerateRandomLegalPosition`). This is the primary correctness
    gate -- cheap to run (`gmake TEST=1`), already wired into the batch
-   self-test startup path.
-3. **Add an equivalent random-position test for
-   `_CountKingSafetyDefectsBB`**, same 20,000-position generator, both
-   colors, comparing its returned defect count *and* the
-   `uPiecesPointingAtKing` side-effect against `CountKingSafetyDefects`.
-   No existing harness to extend here (unlike `GetAttacks`), so this is
-   new test code in the `TEST=1` build, following `TestGetAttacks`'s
-   shape.
-4. **`precommit_check.sh`** as always, for the crash/assert layer.
-5. **SEE-value diffing at the position level**: since `SEE()` returns a
+   self-test startup path. **DONE**, folded into section 3's own work
+   above -- also fixed a latent `bbPieces`/`bbPawns` maintenance gap in
+   `GenerateRandomLegalPosition` found while running this.
+3. **`precommit_check.sh`** as always, for the crash/assert layer.
+   **DONE** for `_GetAttacksBB`'s current (not-yet-wired-in) state --
+   clean. Should be re-run once section 6's toggle actually flips
+   `GetAttacks` to the bitboard implementation, since nothing has
+   exercised that live yet.
+4. **SEE-value diffing at the position level**: since `SEE()` returns a
    single integer per move (not a board eval), the equivalent of the
    Eval work's "150-position score-identity diff" is a diagnostic
    command (`seescores`, analogous to the batch `eval` dumps used
@@ -419,34 +456,23 @@ sequence.
    ECM/random positions, run once per `GetAttacksBB` toggle state,
    diffed. Catches any behavioral drift `TestGetAttacks`'s raw-list
    comparison might miss once results feed into `_MinLegalPiece`'s
-   exchange simulation. Do the same for `CountKingSafetyDefects`: a
-   `kingdefects` diagnostic command dumping the defect count for both
-   kings across the same position batch, diffed the same way -- this one
-   matters more than it sounds, since the count feeds directly into
-   `Eval()`'s king-safety score term (`eval.c:2549-2550`), not just
-   pruning gates.
-6. **Full-suite behavioral check is mandatory here, not optional** --
+   exchange simulation. Not started.
+5. **Full-suite behavioral check is mandatory here, not optional** --
    this is the one place this migration is *riskier* than the Eval
    constant work: `GetAttacks` feeds move ordering and pruning decisions
-   (`generate.c`'s 5 call sites, `searchsup.c:365`), and
-   `CountKingSafetyDefects` feeds pruning/extension decisions directly
-   (`search.c:1252`, `searchsup.c:484,902,905`) *in addition to* an
-   Eval() score term -- so even value-for-value-identical results can
-   shift which move gets tried first or which lines get pruned/extended,
-   changing node counts and occasionally search results at the margins.
-   Run all three curated suites (`ecm_ringers`, `ecm_confident_quick`,
-   `ecm_hard_quick`) at `sd10` against `head_reference/`, and don't treat
-   a clean `TestGetAttacks`/`_CountKingSafetyDefectsBB` unit-level pass
-   as sufficient sign-off by itself -- `CountKingSafetyDefects`'s direct
-   search-code call sites make this the higher-risk of the two changes,
-   despite being the simpler one to implement.
-7. **`match_play.py` gate** (`LOWER95 >= 0.5`) before calling this done
+   directly (`generate.c`'s 5 call sites, `searchsup.c:365`), so even
+   value-for-value-identical results can shift which move gets tried
+   first or which lines get pruned/extended, changing node counts and
+   occasionally search results at the margins. Run all three curated
+   suites (`ecm_ringers`, `ecm_confident_quick`, `ecm_hard_quick`) at
+   `sd10` against `head_reference/`, and don't treat a clean
+   `TestGetAttacks` unit-level pass as sufficient sign-off by itself.
+   Not started -- requires section 6's toggle to exist first, so there's
+   something to flip on for the suite runs.
+6. **`match_play.py` gate** (`LOWER95 >= 0.5`) before calling this done
    -- same reasoning: this is closer to a search-behavior change than a
    pure eval-magnitude change, so the existing eval-tuning gate criteria
-   apply. Run once with both migrations landed together (they share the
-   same underlying primitive, so testing them jointly is representative
-   of how they'll actually ship) plus, if the combined match shows a
-   problem, one more isolating each toggle independently to attribute it.
+   apply. Not started, same section-6 dependency as above.
 
 ## 5. Microbenchmarking
 
@@ -456,16 +482,17 @@ because integration costs (dual maintenance, extra indirection) aren't
 visible in isolation. Two tiers, both required before wiring in:
 
 1. **Isolated cycles/call**, same `SystemReadTimeStampCounter` pattern
-   as `evalcycles`/`RunEvalRookAB`: a `getattacksbench [iterations]`
-   command looping old vs. new `GetAttacks` on the *current*
-   position/square, and a `kingdefectsbench [iterations]` command doing
-   the same for `CountKingSafetyDefects`, interleaved (old/new/old/new)
-   to cancel shared-box noise, run across a battery of positions spanning
-   piece density -- opening (~30 pieces), middlegame (~20), endgame
-   (~8) -- since the algorithmic win should scale *with* piece count, and
-   a flat or inverted result across that spectrum is a red flag before
-   proceeding further, exactly like the rook/bishop Eval win that didn't
-   hold up once integrated.
+   as `evalcycles`/`RunEvalRookAB`: interleaved (old/new/old/new, to
+   cancel shared-box noise) comparison across a battery of positions
+   spanning piece density -- opening (~30 pieces), middlegame (~20),
+   endgame (~8) -- since the algorithmic win should scale *with* piece
+   count, and a flat or inverted result across that spectrum is a red
+   flag before proceeding further, exactly like the rook/bishop Eval
+   win that didn't hold up once integrated. **DONE**, folded into
+   `TestGetAttacks`'s own benchmark addition rather than a separate
+   `getattacksbench` command -- see section 3 for the results (0.53x
+   asm opening, 0.55x middlegame, 0.89x endgame). Consistent win across
+   the spectrum, no red flag.
 2. **Whole-engine cycles-per-node / NPS**, not just the isolated call:
    `sd`-fixed-depth comparison against `head_reference` on the three
    curated suites, using the engine's own self-reported `Searched for N
@@ -473,11 +500,8 @@ visible in isolation. Two tiers, both required before wiring in:
    earlier sessions). This is the check that would have caught the Eval
    regression earlier if it had been run before the reader-migration
    work proceeded -- don't skip straight from isolated-cycles-looks-good
-   to production wiring again. Given `CountKingSafetyDefects`'s search-code
-   call sites, this whole-engine number is where its win (or lack of one)
-   will actually show up -- its isolated per-call cycle count matters
-   less than `GetAttacks`'s does, precisely because it's called from more
-   places than just move ordering.
+   to production wiring again. Not started -- requires section 6's
+   toggle to exist first.
 
 ## 6. Dual-support / toggle strategy
 
@@ -486,38 +510,30 @@ Same `#define` pattern already established for the Eval occupancy work
 implementation `chess.h`'s `GetAttacks` macro/prototype resolves to
 (alongside the existing `CROUTINES` old/new(asm) switch -- this becomes a
 three-way choice: `SlowGetAttacks` (C mailbox), asm `GetAttacks` (x86/x64
-mailbox), `_GetAttacksBB` (new)). A second, independent toggle,
-`KINGSAFETY_BITBOARD`, does the same for `CountKingSafetyDefects` vs.
-`_CountKingSafetyDefectsBB` -- independent so each can be measured,
-gated, and (if needed) rolled back on its own, even though they share the
-underlying `_WhoAttacksSquareBB` primitive.
+mailbox), `_GetAttacksBB` (new)). Not started -- `_GetAttacksBB` is
+still a side-by-side PoC, called only from the test/bench harness, not
+reachable from the `GetAttacks` macro yet.
 
-`bbPieces` maintenance itself is **always on** regardless of either
-toggle -- it's cheap enough that gating it adds complexity for no
+`bbPieces`/`bbPawns` maintenance itself is **always on** regardless of
+this toggle -- it's cheap enough that gating it adds complexity for no
 benefit, and other future work (see section 8) can build on it once
 trusted.
 
-## 7. Retirement criteria for the old mailbox implementations
+## 7. Retirement criteria for the old mailbox implementation
 
 Only delete `SlowGetAttacks`/asm `GetAttacks` after **all** of:
 
 - Extended `TestGetAttacks` clean across the 20,000-position sweep.
+  **DONE.**
 - Isolated cycles/call shows a consistent win across the piece-density
-  spectrum (not just one favorable position).
+  spectrum (not just one favorable position). **DONE.**
 - Whole-engine `sd10` on all three curated suites shows no solve-count
-  regression vs. `head_reference`.
-- `match_play.py` gate clears `LOWER95 >= 0.5`.
+  regression vs. `head_reference`. Not started -- needs section 6's
+  toggle first.
+- `match_play.py` gate clears `LOWER95 >= 0.5`. Not started, same
+  dependency.
 - `head_reference/` rebuilt as the new baseline once landed.
 
-Only delete the old `CountKingSafetyDefects` mailbox body after the same
-five criteria, evaluated against *its* test/bench additions -- do not
-retire it just because `GetAttacks`'s migration cleared its own bar; they
-share a primitive but are independently gated per section 6, and
-`CountKingSafetyDefects`'s wider call-site footprint (search-code pruning
-gates, not just move ordering) makes it the one more likely to surface a
-problem only visible in full search behavior, not in the unit-level
-correctness test.
-
 One easy win to check first, independent of all this: confirm whether
 `x86.asm`'s `GetAttacks` is even compiled into the `SIXTYFOUR=1` release
 profile at all (`GNUmakefile`) -- if it's already dead on this 64-bit-only