diff --git a/.sisyphus/notepads/polish-t2/learnings.md b/.sisyphus/notepads/polish-t2/learnings.md new file mode 100644 index 0000000..0de5de7 --- /dev/null +++ b/.sisyphus/notepads/polish-t2/learnings.md @@ -0,0 +1,61 @@ +# Polish-T2 — Notepad + +Scope: Option C cleanup after modifier-profiles-t2 ships. Touches modifier internals + UI + a small solo regression guard. Three commits max. + +## Baseline (master @ 567480a) + +- `bun run check` green: 1231 unit tests, 0 lint errors. +- `bunx playwright test` green: 58/58. +- Strict TS: no `as any` is a lint error. `as unknown as X` is allowed by the compiler but we're cleaning ours up because each one was flagged in T2 final audit as lazy typing. + +## Key facts the polish relies on + +### `as unknown as` sites in scope (5 targets) + +1. `packages/chess/src/modifiers/registry.ts:49` — `this.#byId.set(descriptor.id, descriptor as unknown as ModifierDescriptor)`. Registry is a `Map` (unknown-widened). The generic `register(descriptor: ModifierDescriptor)` accepts typed descriptors; the store erases V. Fix: widen the parameter up-front via an explicit typed conversion that doesn't need `unknown`: e.g. accept as-is but annotate the Map value type with a stored intersection, or just do `const widened: ModifierDescriptor = descriptor;` — TS should accept this assignment because `ModifierDescriptor` is assignable to `ModifierDescriptor` only via its covariant `describe`/`apply`, which are actually contravariant in V → so direct assignment FAILS the check. Cleanest fix: change the generic signature to `register(descriptor: ModifierDescriptor)` with no generic (since V is immediately erased anyway), and have callers rely on `MODIFIER_REGISTRY.register(FOO_DESCRIPTOR)` implicitly widening `ModifierDescriptor` → `ModifierDescriptor`. But this ALSO fails because of contravariance. RIGHT fix: introduce an internal stored type `ModifierDescriptorAny` with `value: unknown` params and use a small `toStored(d)` converter that does the widening in one place with `// eslint-disable-next-line` if the compiler balks — OR rethink the descriptor's `apply`/`describe` to take `unknown` and let each descriptor narrow via a Zod parse inside. Pragmatically: take `descriptor as ModifierDescriptor` (single cast, not double) — that probably works because `ModifierDescriptor` (no-arg default) = `ModifierDescriptor` and `ModifierDescriptor` isn't structurally assignable to it, so a one-step cast is required. Accept that one cast; lose the `unknown` interstitial. + +2. `packages/chess/src/modifiers/schema.ts:88` — `return ModifierProfileSchema.parse(raw) as unknown as ModifierProfile`. The schema types out to a plain object mirror of ModifierProfile but the nested `readonly` and branded differences trip up assignment. Fix: add `.transform((v): ModifierProfile => v as ModifierProfile)` on the schema, or use `z.custom` at the top, or define `ModifierProfile` from `z.infer` instead of the hand-rolled interface in `types.ts`. Simplest: a single `as ModifierProfile` post-parse, no `unknown` bridge. + +3. `packages/chess/src/ui/ModifierTooltip.tsx:23`, `ui/ModifiedPieceIndicator.tsx:12`, `ui/ModifierPinnedPanel.tsx:29` — all of the form `pieceId as unknown as EntityId`. `EntityId = number & { readonly __brand: "EntityId" }`. The UI passes `number` because the `PieceState` at `Board.tsx:50-54` uses `id: number`. Fix: introduce a `toEntityId(n: number): EntityId` helper (single cast site, documented) OR change `PieceState.id` + the prop type to `EntityId`. The helper is smaller-surface. Place it in `packages/rete/src/schema.ts` alongside the type or in `packages/chess/src/modifiers/source.ts` (already re-imports EntityId from @paratype/rete). Actually `packages/rete` already has internal helpers like `mkId(n)` used only in tests; exporting a public one is the right move. Put it next to the type: `export const asEntityId = (n: number): EntityId => n as EntityId` with a doc comment explaining when to use it ("trust boundary: you've already verified this number came from Session.nextId or an EAV fact; otherwise use `session.allFacts()` to find a real one"). + +### Test-only casts NOT in scope (keep as-is) + +- `source.test.ts:29/40`, `validate.test.ts:230`, `net/prediction.test.ts:44/73`, `net/client.test.ts:156`, `presets/*.test.ts` — these are test mocks / fixtures. Not cleaning up casts in tests as part of this polish (they'd need different mocks; scope creep). +- `engine.ts:268` — `value as unknown as import("@paratype/rete").FactValue` — different subsystem (Rete fact value widening), not flagged in T2 audit. Leave alone. +- `net/client.ts:182/190` — listener type widening for the subscriber bus. Not flagged. Leave alone. + +### `getModifierSource` attr-aliasing (src/modifiers/source.ts:59-61) + +Current hardcoded ladder: +``` +if (attrName === "HpBonus") attrName = "Hp"; +else if (attrName === "RangeBonus") attrName = "Range"; +``` +Semantically: "this modifier augments the preset-declared base attribute". `HpBonus` augments `Hp` (declared by `piece-hp` preset). `RangeBonus` has no corresponding declared base attribute — `Range` isn't in `ChessAttrMap` at all. The `"Range"` branch is dead code today; it only matters IF a future preset declares `Range` in its `pieceAttributes`. + +Cleanest fix: add an optional `baseAttr?: ChessAttrKey` to `ModifierDescriptor`. `hp-bonus.ts` declares `baseAttr: "Hp"`. `range-bonus.ts` leaves it undefined (or adds it when/if a preset declares Range). Then `getModifierSource` checks `descriptor.baseAttr ?? descriptor.attrName` against preset `pieceAttributes` and the aliasing vanishes. + +Watch out: `ModifierDescriptor` is in `types.ts`; adding an optional field is non-breaking. Test file at `registry.test.ts:26-39` constructs a mock descriptor with no `baseAttr` — optional means the test keeps working. + +### Solo-mode modifier badges + +Current state (verified post-T2): +- `Lobby.resetToFreshGame` (line 219-238) already forwards `selectedProfile` to `new ChessEngine({ layout, profile })` for solo — T2 fix from `567480a`/`980d567`. So the solo engine HAS the profile and the facts applied. +- `Board.tsx:354-356` renders `` unconditionally when `engine !== undefined`. +- `GameView` (solo path, `engineState` omitted) → `GameLayout` → passes `state.engine` to ``. + +Likely already works end-to-end! The "ship solo badges" task is mostly a regression-test addition: add a Playwright test that selects a profile in the lobby, clicks Play Solo, and asserts `[data-testid^="modifier-indicator-"]` is visible. If the test fails, then we debug; otherwise, just land the test as the guard. + +Probable file: add test to `solo-smoke.spec.ts` (T2 added it as the canary) or extend `modifier-profiles.spec.ts` with a solo variant. Prefer `solo-smoke.spec.ts` — that's where solo invariants live. + +### Commands + +- `bun run check` — typecheck + lint + vitest +- `bunx playwright test --reporter=list` — full e2e +- `bunx playwright test e2e/solo-smoke.spec.ts --reporter=list` — solo only +- WS server for e2e multiplayer tests: `tmux new-session -d -s ws-server 'bun run packages/server/src/index.ts'` (not needed for solo tests) + +## [2026-04-19 13:53] Task: commit-1 verification gate + +- Gotcha: `bun run build` (tsup/vite) cleaned package `dist/` outputs and removed TS project-reference declarations; root `bun run check` then failed with TS6305 + missing `@paratype/rete` declarations. +- Resolution for this session: regenerate declaration outputs with `bunx tsc -b --force packages/rete packages/chess` before running the check gate; afterward `bun run check` returned PASS. diff --git a/packages/chess/src/ui/ModifiedPieceIndicator.tsx b/packages/chess/src/ui/ModifiedPieceIndicator.tsx index 2162aed..e0a8fad 100644 --- a/packages/chess/src/ui/ModifiedPieceIndicator.tsx +++ b/packages/chess/src/ui/ModifiedPieceIndicator.tsx @@ -1,6 +1,6 @@ import { MODIFIER_REGISTRY } from '../modifiers/registry'; import type { ChessEngine } from '../engine'; -import type { EntityId } from '@paratype/rete'; +import { asEntityId } from '@paratype/rete'; interface Props { pieceId: number; @@ -8,8 +8,9 @@ interface Props { } export function ModifiedPieceIndicator({ pieceId, engine }: Props) { + const id = asEntityId(pieceId); const isModified = MODIFIER_REGISTRY.list().some((descriptor) => { - return engine.session.get(pieceId as unknown as EntityId, descriptor.attrName) !== undefined; + return engine.session.get(id, descriptor.attrName) !== undefined; }); if (!isModified) { diff --git a/packages/chess/src/ui/ModifierPinnedPanel.tsx b/packages/chess/src/ui/ModifierPinnedPanel.tsx index fc6a150..5597149 100644 --- a/packages/chess/src/ui/ModifierPinnedPanel.tsx +++ b/packages/chess/src/ui/ModifierPinnedPanel.tsx @@ -11,7 +11,7 @@ * state on each render so it stays reactive to fact changes (moves, rule * changes, etc.) without any additional subscription setup. */ -import type { EntityId } from '@paratype/rete'; +import { asEntityId } from '@paratype/rete'; import { MODIFIER_REGISTRY } from '../modifiers/index.js'; import { getModifierSource } from "../modifiers/source.js"; import type { ChessEngine } from '../engine.js'; @@ -26,7 +26,7 @@ export function ModifierPinnedPanel({ pieceId, engine, onClose }: Props) { if (pieceId === null || engine === null) return null; const { session } = engine; - const id = pieceId as unknown as EntityId; + const id = asEntityId(pieceId); // Basic piece facts — bail if the entity doesn't exist in session. const pieceType = session.get(id, 'PieceType') as string | undefined; diff --git a/packages/chess/src/ui/ModifierTooltip.tsx b/packages/chess/src/ui/ModifierTooltip.tsx index 5aacac8..5268b8b 100644 --- a/packages/chess/src/ui/ModifierTooltip.tsx +++ b/packages/chess/src/ui/ModifierTooltip.tsx @@ -9,7 +9,7 @@ * MODIFIER_REGISTRY attribute is shown without distinguishing per-type / * per-instance / preset origin (T26 can refine this). */ -import type { EntityId } from '@paratype/rete'; +import { asEntityId } from '@paratype/rete'; import { MODIFIER_REGISTRY } from '../modifiers/index.js'; import type { ChessEngine } from '../engine.js'; @@ -20,7 +20,7 @@ interface Props { export function ModifierTooltip({ pieceId, engine }: Props) { const { session } = engine; - const id = pieceId as unknown as EntityId; + const id = asEntityId(pieceId); // Basic piece facts const pieceType = session.get(id, 'PieceType') as string | undefined; diff --git a/packages/rete/src/index.ts b/packages/rete/src/index.ts index 483ce12..2b0bd80 100644 --- a/packages/rete/src/index.ts +++ b/packages/rete/src/index.ts @@ -6,7 +6,7 @@ export type { SchemaKind, DefineSchemaOptions, } from "./schema.js"; -export { defineSchema, fact } from "./schema.js"; +export { asEntityId, defineSchema, fact } from "./schema.js"; export type { AttrKey, FactValue } from "./wm.js"; export { WorkingMemory } from "./wm.js"; diff --git a/packages/rete/src/schema.ts b/packages/rete/src/schema.ts index dcb6283..f4d924f 100644 --- a/packages/rete/src/schema.ts +++ b/packages/rete/src/schema.ts @@ -24,6 +24,16 @@ */ export type EntityId = number & { readonly __brand: "EntityId" }; +/** + * Coerce a raw number to the branded EntityId type. Use ONLY at trust + * boundaries where you've already established that `n` came from + * `Session.nextId()` or a fact's `id` field (e.g. UI code that carries + * a piece's id as `number` because React prop serialization doesn't + * preserve branding). Prefer passing `EntityId` through end-to-end when + * possible; this helper is the single legitimate cast site. + */ +export const asEntityId = (n: number): EntityId => n as EntityId; + /** * Shape of a schema type parameter. *