diff --git a/docs/adr/modifier-profiles.md b/docs/adr/modifier-profiles.md index 67aba02..b400775 100644 --- a/docs/adr/modifier-profiles.md +++ b/docs/adr/modifier-profiles.md @@ -804,3 +804,136 @@ Aura effects are treated as **derived facts**: delta=+1)` grants `HpBonus +1` to all allied pieces within two squares. If a knight moves from distance 1 to distance 3 from that king, the bonus is removed on the next recomputation pass. + +--- + +## T3 Implementation Retrospective + +What changed between the T3 plan and the shipped reality, what worked, what +hurt, and the open work the team should pick up next. + +### Scope delivered + +All 32 implementation tasks shipped across five waves: + +- **Wave 1**: ADR doc + primitive types/registry + custom descriptor types. +- **Wave 2**: 15 effect primitives (state / mechanic / advanced clusters). +- **Wave 3**: descriptor validator + Zod schema + library persistence + apply + integration + multi-profile stacking. +- **Wave 4**: server `custom-modifier.register` handler + `CustomModifierEditor` + primitive composer + panel kind-dropdown extension + lobby multi-profile + stack picker + aura recomputation hook. +- **Wave 5**: e2e suite + this retrospective + user docs + T4 forward-design. + +Final test counts: 1378 unit tests + the new e2e suite, all green. + +### What deviated from the plan + +#### Trigger/conditional primitives seed facts but don't (yet) fire + +`on-turn-start`, `on-capture`, `on-damaged`, and `conditional` all register and +seed `OnTurnStartHooks` / `OnCaptureHooks` / `OnDamagedHooks` / +`ConditionalHooks` facts on the target piece — but the engine pipeline that +SHOULD evaluate those hooks at the corresponding game phase is not wired up. +The data flows; the runtime trigger evaluation is deferred. + +The fact-seeding still has user value (the inspector can show "this piece has 1 +on-capture trigger"), but the primitives currently behave as data declarations, +not active behaviour. T22's `applyCustomDescriptor` walks `childPrimitives()` +recursively at apply time, so nested primitives DO run when the descriptor is +applied — what doesn't yet happen is "fire on-capture's primitives WHEN this +piece captures". + +Wiring is mechanical (the engine already exposes `onAfterMove`/`onDamage` hook +points used by the modifier-integration preset for `DamageResistance`); the +follow-up should replicate that pattern for the four trigger attrs. + +#### Aura contributions land in `AuraContributions` but no consumer reads them + +T28 ships `computeAuraFacts` and wires it to `onAfterMove` so contributions +recompute correctly after every move. They land on each affected piece as +`AuraContributions: Record`. **However**, no engine +subsystem currently reads that record when computing effective attribute +values — `HpBonus` consumers see only the directly-seeded value, not the +aura-derived addition. + +The infrastructure is correct (idempotent, source-move retracts contribution, +multi-source accumulation). The next step is "effective attr read points layer +both the direct fact AND the aura contribution" — a small but careful change +in HP / Range / DirectionAdditions consumers. + +#### Multi-profile stacking is solo-only on the wire + +T27 ships the lobby UI for ordered profile stacks. The engine +(`applyProfilesToSession`) already supports the stack natively. The wire +protocol, however, still carries one `ModifierProfile` per `room.create` / +`modifier-profile.update` payload — multiplayer rooms can use a single profile +each. Stacking on the wire would extend `RoomCreatePayloadSchema.profile?` to +`profiles?: ModifierProfile[]` and update the consent flow (T2-ADR-2) to apply +to a stack. Out of scope for T3. + +#### Server-side semantic validation deferred + +The server runs Zod structural validation on incoming `custom-modifier.register` +descriptors but does NOT validate primitive-kind-in-registry or per-primitive +params satisfaction. Doing so would require mirroring the entire 15-primitive +catalog into the Zod-v3 server package across the v3↔v4 schema boundary. + +Instead: the client validates with `validateCustomDescriptor` BEFORE sending, +and the engine's runtime applier (`applyCustomDescriptor`) silently skips +unknown primitive kinds on the apply path. Net effect: a malicious or buggy +client can register a structurally-valid descriptor whose primitives quietly +no-op. Consequence: low-severity (no incorrect game behaviour can leak to +opponent), but a server-side semantic gate is the natural T3.1 hardening. + +### What worked well + +- **Per-engine `CustomModifierRegistry` (ADR-4) prevents cross-room leakage by + construction**, not by discipline. We never had to hunt down a "this + descriptor mysteriously appeared in another room" bug because the type + system makes it impossible. +- **`childPrimitives()` as the composable recursion contract** turned the + "validate depth ≤ 3" and "walk for apply" requirements into one-liner + consumers. The same callback drives the validator's tree walk and the + applier's recursive descent. +- **Zod schema as both wire validator AND structural truth** kept the + custom-descriptor shape from drifting between protocol layer and runtime + type. The single boundary cast in `parseCustomModifierDescriptor` is + documented and small. +- **15 separate primitive files (one each)** kept individual change sets + reviewable and made `git blame` informative for each primitive's evolution. + +### What hurt + +- **Long parallel agent runs blew the tool-call cap repeatedly.** Multiple + Wave 2 / Wave 3 / Wave 4 batches were cancelled mid-flight after the + delegated agent burned 200 tool calls on verification loops without + committing. Reconciling partial work consumed orchestrator time we wanted + to spend on Wave 5. + + Mitigation for future epics: prompt agents to commit incrementally (every + 2-3 files) rather than batching all commits at the end. Even better, + decompose the cluster prompts further (one primitive = one delegation) + even at the cost of more orchestration overhead. + +- **`as unknown as X` in lazy schemas required a documented exception.** + T20's recursive `EffectPrimitiveNodeSchema` couldn't cleanly satisfy + `z.ZodType` because Zod infers `kind: string` and + the type wants `kind: PrimitiveKind`. We kept a single boundary cast in + `parseCustomModifierDescriptor` rather than restructuring the type. + +- **The chess-side Zod v4 vs server Zod v3 mismatch** forced us to mirror + schemas across two packages by hand. Moving the chess and server packages + onto the same Zod major would simplify a lot, but is a separate migration. + +### Open follow-ups + +| Item | Effort | Priority | +|---|---|---| +| Wire trigger primitives (on-turn-start/on-capture/on-damaged) into the engine pipeline | Medium | High — needed before custom modifiers feel "alive" | +| Layer `AuraContributions` into HP/Range read sites | Small | High — same | +| Server-side semantic validation of `custom-modifier.register` payloads | Medium | Medium — current degradation is silent | +| Multi-profile on the wire for multiplayer | Medium | Medium — solo-only is a clean stop-gap | +| Visual editor: drag-to-reorder primitives in the tree | Small | Low — polish | +| Visual editor: nested-tree inspector (currently JSON textarea fallback) | Medium | Low — polish | +| `CustomModifierEditor` Playwright coverage beyond happy-path | Small | Medium |