diff --git a/.sisyphus/notepads/rule-variants/learnings.md b/.sisyphus/notepads/rule-variants/learnings.md index a111d83..5d54f1d 100644 --- a/.sisyphus/notepads/rule-variants/learnings.md +++ b/.sisyphus/notepads/rule-variants/learnings.md @@ -194,3 +194,53 @@ section). - `session.allFacts()` automatically carries the new fact across serialization boundaries (snapshot/restore) without protocol changes — no server-side work needed for the A.3 addition. + +## [2026-04-20 20:17] Task: A.4 — overridePieceMoves hook + +**Shipped**: +- `overridePieceMoves` hook on `PresetDef` + (`packages/chess/src/presets/registry.ts`). Signature: + `(engine, pieceId) => readonly LegalMove[] | undefined`. +- `engine.getAllLegalMoves` restructured for per-piece dispatch: + 1. Poll every scoped preset's `overridePieceMoves`. First + non-undefined wins. + 2. If override wins → replace moves entirely. SKIP: default + type-registry generator, `transformMoveGenerator` chain, + `getExtraMoves` for this piece, en-passant/castling/promotion + synthesis (preset owns those mechanics). + 3. If no override → unchanged default path (type registry + + transforms + en-passant + castling + promotion + + `getExtraMoves`). + 4. `filterMoves` runs REGARDLESS (layering stays useful). + 5. Collision: second non-undefined return triggers + `console.warn` (dev only — suppressed when `NODE_ENV=production`). + First winner stands. +- New test file: `override-piece-moves.test.ts` (7 tests). Covers: + baseline knight count unchanged, lame-knight prototype, undefined- + return fallthrough, collision warning fires, production-mode + suppression, `filterMoves` composes on top of overrides, unrelated + piece types unaffected. + +**Verification**: `bun run check` green. 1448 tests (was 1441, +7). + +**Gotchas**: +- `engine.getPieceAt` is private. Test presets need occupancy + checks → use `engine.session.allFacts().find(f => f.attr === "Position" && f.value === sq)`. + Same friction every future preset test will hit. Flagged for + Phase A.5 docs consideration but leaving the private as-is per + "don't refactor during features". +- Override collisions are a guardrail, not opt-in. The design + choice is "warn, don't throw" because the incompatibleWith graph + is the authoritative contract — crashing on collision would + make it harder to iteratively develop two competing overrides. + +## Phase A status at end of 20:17 + +All 4 hooks landed on master: +- A.1: 4d05473 — getRoyalPieces + engine royal dispatch +- A.2: db8145f — filterLegalMoves +- A.3: f9475e9 — shouldAdvanceTurn + HalfMovesThisTurn +- A.4: (next commit) — overridePieceMoves + +Remaining in Phase A: A.5 — verification gate + PRESET-API.md +hook-ordering doc + phase summary. diff --git a/packages/chess/src/engine.ts b/packages/chess/src/engine.ts index fb569a2..dab6bf4 100644 --- a/packages/chess/src/engine.ts +++ b/packages/chess/src/engine.ts @@ -915,77 +915,125 @@ export class ChessEngine { .filter(p => p.type !== undefined); for (const piece of pieces) { - const baseGetter = lookupMoveGenerator(piece.type); - // Fold the transformMoveGenerator chain over all active presets - // (scope-filtered for `color`). Each preset's wrapper receives the - // OUTPUT of the previous — composing cleanly. We seed with the - // type-registry generator, or a degenerate empty-move generator if - // the piece type is unknown (keeps later wrappers well-defined). const scopedPresets = this.activePresets.getForColor(color); - const seedGetter: MoveGetter = - baseGetter ?? ((_s: Session, _id: EntityId) => [] as LegalMove[]); - const getter: MoveGetter = scopedPresets - .filter(p => p.transformMoveGenerator) - .reduce( - (gen, preset) => preset.transformMoveGenerator!(this, piece.id, gen), - seedGetter, - ); - // Skip pieces with no known type AND no transform — same semantics - // as the original "unknown type ⇒ immobile" guard, but now a - // transform preset can still produce moves for an otherwise unknown - // type. - if (!baseGetter && scopedPresets.every(p => !p.transformMoveGenerator)) { - continue; - } - let pieceMoves = getter(this.session, piece.id); - - // Add en passant for pawns - if (piece.type === "pawn") { - pieceMoves = [ - ...pieceMoves, - ...getEnPassantMoves(this.session, piece.id), - ]; - } - - // Add castling for kings - if (piece.type === "king") { - const enemyColor: PieceColor = color === "white" ? "black" : "white"; - const castling = getCastlingMoves( - this.session, - piece.id, - (s, sq) => isSquareAttacked(s, sq, enemyColor), - ); - pieceMoves = [...pieceMoves, ...castling]; - } - - // Replace raw promotion-rank pawn moves with promotion-tagged variants - if (piece.type === "pawn") { - const promotions = getPromotionMoves(this.session, piece.id); - if (promotions.length > 0) { - const promotionTos = new Set(promotions.map(m => m.to)); - pieceMoves = pieceMoves.filter(m => !promotionTos.has(m.to)); - pieceMoves = [...pieceMoves, ...promotions]; + // Phase A.4: check for overridePieceMoves BEFORE the default + // generator + transform chain + getExtraMoves. First + // non-undefined wins; later collisions emit a dev-mode + // console.warn but are otherwise ignored (the first winner's + // set is used as-is). The returned override REPLACES the + // type-registry moves; en-passant / castling / promotion + // synthesis below is SKIPPED because any preset redefining a + // pawn or king assumes responsibility for those mechanics. + let overrideMoves: readonly LegalMove[] | undefined = undefined; + let overrideSource: string | undefined = undefined; + for (const preset of scopedPresets) { + if (preset.overridePieceMoves === undefined) continue; + const candidate = preset.overridePieceMoves(this, piece.id); + if (candidate === undefined) continue; + if (overrideMoves === undefined) { + overrideMoves = candidate; + overrideSource = preset.id; + } else { + // Second non-undefined return → collision. Dev-mode + // warning only; first winner keeps its claim. + const env = + (globalThis as { readonly process?: { readonly env?: { readonly NODE_ENV?: string } } }) + .process?.env?.NODE_ENV; + if (env !== "production") { + console.warn( + `[overridePieceMoves] collision on piece ${String(piece.id)} ` + + `(type=${piece.type}): preset "${overrideSource}" already ` + + `overrode; "${preset.id}" also returned a non-undefined ` + + `result (ignored). Declare incompatibleWith between these ` + + `presets to catch this at config time.`, + ); + } } } - // Apply active preset rules for the color whose turn it is. The - // per-color filter implements `scope=white` / `scope=black` — a - // white-only preset never contributes moves while black is on move. - // - // Order matters — `getExtraMoves` contributes to the set that - // `filterMoves` operates on, so every active preset sees the full - // aggregated set (including prior presets' additions). - const activePresets = scopedPresets; - for (const preset of activePresets) { - if (preset.getExtraMoves) { + let pieceMoves: LegalMove[]; + if (overrideMoves !== undefined) { + pieceMoves = [...overrideMoves]; + } else { + const baseGetter = lookupMoveGenerator(piece.type); + // Fold the transformMoveGenerator chain over all active + // presets (scope-filtered for `color`). Each preset's wrapper + // receives the OUTPUT of the previous — composing cleanly. + // We seed with the type-registry generator, or a degenerate + // empty-move generator if the piece type is unknown (keeps + // later wrappers well-defined). + const seedGetter: MoveGetter = + baseGetter ?? ((_s: Session, _id: EntityId) => [] as LegalMove[]); + const getter: MoveGetter = scopedPresets + .filter(p => p.transformMoveGenerator) + .reduce( + (gen, preset) => preset.transformMoveGenerator!(this, piece.id, gen), + seedGetter, + ); + // Skip pieces with no known type AND no transform — same + // semantics as the original "unknown type ⇒ immobile" guard, + // but now a transform preset can still produce moves for an + // otherwise unknown type. + if (!baseGetter && scopedPresets.every(p => !p.transformMoveGenerator)) { + continue; + } + + pieceMoves = getter(this.session, piece.id); + + // Add en passant for pawns + if (piece.type === "pawn") { pieceMoves = [ ...pieceMoves, - ...preset.getExtraMoves(this, piece.id), + ...getEnPassantMoves(this.session, piece.id), ]; } + + // Add castling for kings + if (piece.type === "king") { + const enemyColor: PieceColor = color === "white" ? "black" : "white"; + const castling = getCastlingMoves( + this.session, + piece.id, + (s, sq) => isSquareAttacked(s, sq, enemyColor), + ); + pieceMoves = [...pieceMoves, ...castling]; + } + + // Replace raw promotion-rank pawn moves with promotion-tagged + // variants + if (piece.type === "pawn") { + const promotions = getPromotionMoves(this.session, piece.id); + if (promotions.length > 0) { + const promotionTos = new Set(promotions.map(m => m.to)); + pieceMoves = pieceMoves.filter(m => !promotionTos.has(m.to)); + pieceMoves = [...pieceMoves, ...promotions]; + } + } + + // Apply active preset rules for the color whose turn it is. + // The per-color filter implements `scope=white` / + // `scope=black` — a white-only preset never contributes moves + // while black is on move. + // + // Order matters — `getExtraMoves` contributes to the set + // that `filterMoves` operates on, so every active preset + // sees the full aggregated set (including prior presets' + // additions). + for (const preset of scopedPresets) { + if (preset.getExtraMoves) { + pieceMoves = [ + ...pieceMoves, + ...preset.getExtraMoves(this, piece.id), + ]; + } + } } - for (const preset of activePresets) { + + // filterMoves ALWAYS runs — even when overridePieceMoves + // produced the set. Lets presets layer per-piece filters on top + // of overrides. + for (const preset of scopedPresets) { if (preset.filterMoves) { pieceMoves = preset.filterMoves(pieceMoves, this, piece.id); } diff --git a/packages/chess/src/presets/override-piece-moves.test.ts b/packages/chess/src/presets/override-piece-moves.test.ts new file mode 100644 index 0000000..550eb70 --- /dev/null +++ b/packages/chess/src/presets/override-piece-moves.test.ts @@ -0,0 +1,288 @@ +/** + * Tests for the `overridePieceMoves` hook (Phase A.4 of the + * rule-variants epic). + * + * Semantics under test: + * - Override runs BEFORE the type-registry default generator. + * - First non-undefined return wins. Subsequent matching presets + * are ignored BUT emit a dev-mode console.warn naming the + * colliding presets. + * - Override SKIPS the default generator, transformMoveGenerator + * chain, and getExtraMoves contributions on THAT piece. The + * engine will NOT synthesize en-passant / castling / promotion + * on the overridden set — the preset owns those mechanics. + * - `filterMoves` and `filterLegalMoves` STILL run on the + * overridden set (documented contract — layering stays useful). + * - Returning undefined declines; the engine falls through to the + * next preset or the default. + * + * Coverage: + * - "Lame knight" prototype replaces knight moves with single-square + * orthogonal steps. Proves override semantics. + * - Non-matching return (undefined) leaves default generator intact. + * - Collision: two presets both overriding → first wins, dev-mode + * warning fires. Production-mode suppression is covered via an + * explicit env toggle. + * - `filterMoves` still runs on the overridden set. + * - Unrelated pieces are unaffected (override on pawns doesn't + * touch knights). + */ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import "./index.js"; +import { ChessEngine } from "../engine.js"; +import { PRESET_REGISTRY } from "./registry.js"; +import type { LegalMove } from "../rules/types.js"; +import { clearBoard, placePiece } from "./test-utils.js"; + +const LAME_KNIGHT_ID = "test-lame-knight"; +const NEVER_OVERRIDE_ID = "test-never-override"; +const RIVAL_LAME_KNIGHT_ID = "test-rival-lame-knight"; +const KNIGHT_FILTER_ID = "test-knight-filter"; + +beforeEach(() => { + PRESET_REGISTRY.register({ + id: LAME_KNIGHT_ID, + name: "Lame knight (test)", + description: + "Replaces knight moves with single-square orthogonal steps.", + incompatibleWith: [], + requires: [], + overridePieceMoves(engine, pieceId) { + const type = engine.session.get(pieceId, "PieceType") as string | undefined; + if (type !== "knight") return undefined; + const from = engine.session.get(pieceId, "Position") as number; + const out: LegalMove[] = []; + const dirs = [-8, 8, -1, 1]; // N, S, W, E + for (const d of dirs) { + const to = from + d; + if (to < 0 || to > 63) continue; + // Avoid wrapping across file edges on lateral moves. + if (d === -1 && (from % 8) === 0) continue; + if (d === 1 && (from % 8) === 7) continue; + // Inline occupancy check via session facts (getPieceAt is + // private on the engine). + let isCapture = false; + for (const f of engine.session.allFacts()) { + if (f.attr === "Position" && f.value === to) { + isCapture = true; + break; + } + } + out.push({ pieceId, from, to, isCapture }); + } + return out; + }, + }); + + PRESET_REGISTRY.register({ + id: NEVER_OVERRIDE_ID, + name: "Never override (test)", + description: "Always returns undefined from the override hook.", + incompatibleWith: [], + requires: [], + overridePieceMoves() { + return undefined; + }, + }); + + PRESET_REGISTRY.register({ + id: RIVAL_LAME_KNIGHT_ID, + name: "Rival lame knight (test)", + description: + "Also overrides knights — triggers collision with LAME_KNIGHT_ID.", + incompatibleWith: [], + requires: [], + overridePieceMoves(engine, pieceId) { + const type = engine.session.get(pieceId, "PieceType") as string | undefined; + if (type !== "knight") return undefined; + // Return an empty set — different from LAME_KNIGHT_ID's output, + // so if this preset wins we can tell. + return []; + }, + }); + + PRESET_REGISTRY.register({ + id: KNIGHT_FILTER_ID, + name: "Knight filter (test)", + description: + "Drops knight moves to squares on rank 3 (index 16-23). Proves filterMoves runs on override output.", + incompatibleWith: [], + requires: [], + filterMoves(moves, engine, pieceId) { + const type = engine.session.get(pieceId, "PieceType") as string | undefined; + if (type !== "knight") return moves; + return moves.filter(m => m.to < 16 || m.to > 23); + }, + }); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("overridePieceMoves — default (no preset)", () => { + it("baseline: FIDE opening produces 4 knight moves (2 per knight × 2 knights)", () => { + const engine = new ChessEngine(); + const moves = engine.getAllLegalMoves(); + const knightMoves = moves.filter(m => { + const type = engine.session.get(m.pieceId, "PieceType") as string; + return type === "knight"; + }); + // Each knight has 2 legal moves from the back rank; 2 knights → 4. + expect(knightMoves).toHaveLength(4); + }); +}); + +describe("overridePieceMoves — single-preset override", () => { + it("lame-knight replaces knight moves with single-square orthogonal steps", () => { + const engine = new ChessEngine(); + engine.setActivePresets([ + { id: LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + ]); + const moves = engine.getAllLegalMoves(); + const knightMoves = moves.filter(m => { + const type = engine.session.get(m.pieceId, "PieceType") as string; + return type === "knight"; + }); + // FIDE opening: each white knight (b1=1, g1=6) is blocked on 3 of + // 4 orthogonal squares (own pieces + board edge). Only "forward" + // to rank 2 is open BUT that's occupied by pawns. So every lame- + // knight move gets self-check-filtered? No — they're all blocked + // by friendly pieces (pawns on b2, g2; back-rank neighbours a1/c1, + // f1/h1). Expect 0 legal moves. + // + // Actually the lame-knight override generates moves INCLUDING + // captures of friendlies (we didn't filter). The preset doesn't + // distinguish friend vs enemy — the `isCapture` flag is set iff + // occupied. Then `filterMoves` (none registered in this setup) + // leaves them; `filterSelfCheckMoves` eventually drops friendly- + // capture moves? Actually NO — it only checks self-check after + // applying. Friendly-capture would leave the mover on the same + // square as a friendly piece. No rule in check.ts catches that. + // + // So we get the raw unfiltered count. Knight on b1 (1): N=-8 + // (invalid, <0); S=9 (occupied, friendly pawn — isCapture=true); + // W=0 (occupied, friendly rook); E=2 (occupied, friendly + // bishop). That's 3 "moves". Knight on g1 (6): N=-2 (invalid); + // S=14 (pawn); W=5 (bishop); E=7 (rook). 3 moves. Total: 6. + // + // The self-check filter would drop any move that leaves the king + // attacked. None of these expose the king (pieces stay on their + // starting structure). So we expect 6 lame-knight moves. + expect(knightMoves).toHaveLength(6); + for (const m of knightMoves) { + // All lame-knight moves are single-square orthogonal. + const delta = Math.abs(m.to - m.from); + expect([1, 8]).toContain(delta); + } + }); + + it("non-knight pieces are unaffected (override declined via undefined)", () => { + const engine = new ChessEngine(); + engine.setActivePresets([ + { id: LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + ]); + const moves = engine.getAllLegalMoves(); + const pawnMoves = moves.filter(m => { + const type = engine.session.get(m.pieceId, "PieceType") as string; + return type === "pawn"; + }); + // FIDE opening has 16 pawn moves (8 single-pushes + 8 double-pushes). + expect(pawnMoves).toHaveLength(16); + }); +}); + +describe("overridePieceMoves — undefined return falls through", () => { + it("a preset returning undefined leaves default behaviour intact", () => { + const engine = new ChessEngine(); + engine.setActivePresets([ + { id: NEVER_OVERRIDE_ID, scope: "both", turnsRemaining: null }, + ]); + // Default: 20 opening moves. If NEVER_OVERRIDE_ID broke the + // fallback, the count would change. + expect(engine.getAllLegalMoves()).toHaveLength(20); + }); +}); + +describe("overridePieceMoves — collision + dev-mode warning", () => { + it("first match wins; second match emits console.warn", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const engine = new ChessEngine(); + engine.setActivePresets([ + { id: LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + { id: RIVAL_LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + ]); + const moves = engine.getAllLegalMoves(); + const knightMoves = moves.filter(m => { + const type = engine.session.get(m.pieceId, "PieceType") as string; + return type === "knight"; + }); + // First preset (LAME_KNIGHT_ID) wins → 6 moves (see prior test). + // If RIVAL_LAME_KNIGHT_ID had won we'd see 0. + expect(knightMoves).toHaveLength(6); + + // Collision warning fires once per colliding piece (2 knights). + expect(warnSpy).toHaveBeenCalled(); + const firstCallArg = warnSpy.mock.calls[0]?.[0]; + expect(typeof firstCallArg).toBe("string"); + expect(firstCallArg as string).toContain("[overridePieceMoves] collision"); + expect(firstCallArg as string).toContain(LAME_KNIGHT_ID); + expect(firstCallArg as string).toContain(RIVAL_LAME_KNIGHT_ID); + }); + + it("collision warning suppressed when NODE_ENV=production", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + + try { + const engine = new ChessEngine(); + engine.setActivePresets([ + { id: LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + { id: RIVAL_LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + ]); + engine.getAllLegalMoves(); + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + if (originalEnv === undefined) { + delete process.env.NODE_ENV; + } else { + process.env.NODE_ENV = originalEnv; + } + } + }); +}); + +describe("overridePieceMoves — filterMoves still runs on override output", () => { + it("knight-filter preset drops rank-3 destinations from lame-knight moves", () => { + const engine = new ChessEngine(); + // Clear pawns so lame-knight has unblocked rank-3 candidates. + clearBoard(engine, { preserveKings: false }); + placePiece(engine, "king", "white", "e1"); + placePiece(engine, "king", "black", "e8"); + // Knight on a3 (16): N=24 (b4? no, 24=a4, rank 4), S=8 (a2), + // W=invalid (file edge), E=17 (b3, rank 3). Without filter: 3 + // moves. With KNIGHT_FILTER_ID dropping rank-3 destinations + // (16-23), "E=17" is dropped AND the starting square 16 doesn't + // matter (filter checks `to`). Expected surviving: 2 moves. + placePiece(engine, "knight", "white", "a3"); + + engine.setActivePresets([ + { id: LAME_KNIGHT_ID, scope: "both", turnsRemaining: null }, + { id: KNIGHT_FILTER_ID, scope: "both", turnsRemaining: null }, + ]); + + const moves = engine.getAllLegalMoves(); + const knightMoves = moves.filter(m => { + const type = engine.session.get(m.pieceId, "PieceType") as string; + return type === "knight"; + }); + // Every surviving move lands OUTSIDE rank 3 (16-23). + for (const m of knightMoves) { + expect(m.to < 16 || m.to > 23).toBe(true); + } + // At least one move survived (sanity). + expect(knightMoves.length).toBeGreaterThan(0); + }); +}); diff --git a/packages/chess/src/presets/registry.ts b/packages/chess/src/presets/registry.ts index ef04027..cd4f6b5 100644 --- a/packages/chess/src/presets/registry.ts +++ b/packages/chess/src/presets/registry.ts @@ -597,6 +597,47 @@ export interface PresetDef { ctx: TurnAdvanceContext, ) => boolean | undefined; + /** + * REPLACE the default move generator for a specific piece. + * + * Runs BEFORE `PIECE_TYPE_REGISTRY.get(type).generateMoves` in the + * engine's per-piece dispatch. First non-undefined return wins — + * the returned array becomes the piece's moves, and the engine + * SKIPS: + * - the default type-registry generator + * - `transformMoveGenerator` chain on this piece + * - `getExtraMoves` from other presets for this piece + * + * But STILL runs: + * - `filterMoves` (each preset per piece) — the returned set can + * still be filtered per-preset. + * - the self-check filter (unless opted out via + * `shouldFilterSelfCheck`). + * - `filterLegalMoves` (aggregate stage). + * - en-passant / castling / promotion handling (those run from + * piece-type switches in the engine, so if you override a pawn + * generator you MUST also synthesize promotion-tagged moves + * yourself; document the choice in-preset). + * + * Return undefined to decline — the engine moves to the next + * preset, and ultimately falls back to the default generator. + * + * COLLISIONS: If two presets both return non-undefined for the + * same piece, the FIRST WINS (registration order via + * `activePresets.list()`). The engine emits a dev-mode + * `console.warn` naming the two preset ids. This is a guardrail, + * not an opt-in: conflicts should be caught at config time via + * `incompatibleWith`. The warning is suppressed in production + * (`globalThis.process?.env?.NODE_ENV === "production"`). + * + * Canonical users: `berolina-pawns` (redefines pawn moves — + * forward-diagonal push + forward-orthogonal capture). + */ + readonly overridePieceMoves?: ( + engine: ChessEngine, + pieceId: EntityId, + ) => readonly LegalMove[] | undefined; + /** * Phase hook: fires AFTER the move has been confirmed legal but * BEFORE any state mutation. Return `{ cancel: true, reason }` to