diff --git a/HANDOFF.md b/HANDOFF.md new file mode 100644 index 000000000..cdb57955e --- /dev/null +++ b/HANDOFF.md @@ -0,0 +1,194 @@ +# Handoff: cantrip/spell selection regression fix + +Branch: `claude/fix-cantrips-selection-bug-CSwVv` +Status: phase 1 shipped; phase 2 + phase 3 + relink UI planned, not yet implemented +Last updated by previous agent at end of design conversation. Read this fully before touching code. + +## The bug + +Homebrew Cleric/Druid replacements lost cantrip/spell selections after a recent UX change. Saved entity still had `:cleric-cantrips-known`; UI showed nothing selected. Setting orcbrew source name to blank temporarily "fixed" it. + +Root cause: `spell_subs.cljs:475-478` (the `::classes5e/plugin-classes` sub) mutated class `:name` to `"Cleric (Source)"` for display. Downstream consumers re-derived identity via `common/name-to-kw` from the mutated `:name`. Saved characters using the canonical `:cleric-cantrips-known` key were orphaned because the new template produced `:cleric-source-cantrips-known`. + +Full case study in `docs/kb/key-vs-name-separation.md` including the four leak sites. + +## What's shipped (phase 1) + +Commits on the branch: + +- `0a4f262` — Fix cantrip/spell selection regression + add source-suffix toggle + - Reverted the `:name` mutation in `spell_subs.cljs` + - Plumbed `:plugin-source` through `option-cfg` as a distinct `::plugin-source` slot + - Added user-pref toggle `::show-class-source-suffix` (default off, pure display) + - Added `class-option-display-name` helper in `character_builder.cljs` + - Added `reconcile-spell-selection-keys` in `content_reconciliation.cljs` (load-time auto-rebind for unambiguous orphans) + - Wired into `:set-character` in `events.cljs` + - Tests in `content_reconciliation_test.cljs` +- `a77d0a1` — kb doc capturing the design rule + four leak sites + plugin-load race verification +- `46310f3` — three source comments pointing at the kb doc at speculation-prone sites +- `af5e6fe` — `console.info` on `:rewrote` for field debugging + +## The architectural pivot mid-thread + +Phase 1 only stopped the *active* leak. It defended the design rule by convention, not by architecture. The deeper question — should identity ever derive from `:name` at all outside the editor? — surfaced during review and led to the current plan. + +The user's frame: there are two valid architectures, and the codebase has to commit to one. + +1. **`:name` is mutable display.** Identity must not depend on it. Any code, anywhere, can mutate `:name` freely without risk. +2. **`:name` is locked. Display goes in a separate field.** Identity *can* depend on `:name` because `:name` is structurally protected from mutation. + +Phase 1 left us at neither — `:name` is unmutated by convention but identity still derives from it via `(name-to-kw title)` at `options.cljc:469`. Anyone who mutates `:name` again replays the bug. The user (correctly) does not want a convention-only defense. + +**Direction chosen: architecture #1.** Identity flows from `class-key`, not `class-name`/`:name`. `:name` is allowed to mutate freely; nothing identity-bearing depends on it outside the editor. + +## What's planned next + +### Phase 2 — switch kw derivation to class-key + +**The surgical rule:** wherever a value sourced from `:name` ends up at `name-to-kw` and produces a key, replace the input with `class-key` at that terminus. Display chains (`class-name → title → user-facing label`) stay untouched — those legitimately use `:name`. + +Three sites to change: + +1. **`options.cljc:469`** in `spell-selection`: `kw (common/name-to-kw title)` → derive `kw` from `class-key` (already in the kwarg destructure). `title` stays for display. +2. **`template_base.cljc:275`** in `?prepare-spell-count`: takes `class-name` string today. Change to take `class-key` directly. Callers adjust. +3. **`options.cljc:635`** `class-key-name` fallback: latent today (callers pass `class-key`), but the fallback derivation is wrong. Switch to class-key. + +**Sites that stay alone:** +- `events.cljs:544` `reg-save-homebrew`: `key (common/name-to-kw name)` — this is the class editor's save path. Editor-mediated rename mechanism. Editor owns key changes. **Do not touch.** +- `template.cljc:option-cfg`/`selection-cfg` constructors: operating on destructured kwargs at template-build time. They mint keys; they're not downstream consumers of mutable name fields. + +### Phase 3 — import-rename hardening + +Today (`import_validation.cljs:1388-1394`), import-conflict-rename appends the full slugified source name to `:key` only. `:name` is untouched. Combined with `events.cljs:544` regenerating `:key` from `:name` on every editor save, **the import-disambiguation lasts only until the user opens the class in the editor**. First save obliterates it. + +Fix: + +- On import-conflict, compute a source abbreviation: + - 1–3 words: first+last letter of each word (e.g., `"Kibbles' Tasty"` → `KsTy`, `"Kindly Townsfolk"` → `KyTk`) + - 4+ words: first letter of each (e.g., `"Tasha's Cauldron of Everything"` → `TCoE`, `"Mythic Odysseys of Theros"` → `MOoT`) + - Numeric tie-breaker only on detected collision (`KsTy` already taken → `KsTy2`) +- Append the abbreviation to both `:key` AND `:name` (e.g., `:key :cleric-kt`, `:name "Cleric (KT)"`). +- Editor save now regenerates `:key` from `:name`; because `:name` includes `(KT)`, the regenerated key remains `:cleric-kt`. Disambiguation survives edits. +- Override case: a user wanting to intentionally override a built-in opens the class in the editor and deletes `(KT)` from `:name`. Save regenerates as `:cleric`. Override active. The reconciler then handles the orphan rebind on next character load (auto if unambiguous, surface to UI otherwise). + +**Stored abbreviation field:** undecided. The user is on the fence. Pros (deterministic source-of-truth, enables collision-resolution UI, decouples abbr from source rename) vs. cons (schema bloat, two-place state, recompute is deterministic anyway). Default plan: do not store unless the collision-resolution UI gets built. + +### Reconciler — updates and additions + +The shipped reconciler at `content_reconciliation.cljs` stays as infrastructure but needs three updates: + +- `class->expected-spell-keys` computes from `class-key` (not `:name`) to match the new derivation. Same suffix-match logic, different input. +- **Drop `:parked`** accumulator from `reconcile-class-entry-options` and `reconcile-spell-selection-keys`. It was scaffolding for a UI that consumes empty data — `:parked` cannot fire today (class expected-keys always has 2 keys with distinct suffixes, so candidates is always 0 or 1; single-survivor always rewrites). Pure vapor surface area. +- **Add `:unbound-classes`** accumulator. For each character class entry whose `::entity/key` doesn't match any currently-loaded class (built-ins + plugins), emit `{:class-key K :inferred-name N :candidates [...]}`. Feeds the relink UI. + +New reconciler return shape: `{:character ... :rewrote [...] :unbound-classes [...]}`. + +**Add subclass-mismatch detection.** For each subclass entry on the character, check its `:class` reference. If it doesn't match any class entry on the same character, flag it. Auto-rebind if unambiguous (one candidate matches); surface in the relink UI otherwise. + +### Relink UI + +Inline on the character builder, modeled on the existing missing-content banner at `character_builder.cljs:1940-1972`. Same surface, same expand/collapse pattern, adds an action. + +Sections: +- **Unbound classes:** "Class `:cleric-kt` from this character isn't currently loaded. Pick one: [list of loaded candidates ranked by name match / prefix share]. [Rebind] [Leave as-is]" +- **Subclass mismatch:** similar shape, but the candidate list is the character's own class entries. + +Picking rebinds via an event handler that mutates `::entity/key` on the entry (class) or `:class` field (subclass), then re-dispatches `:set-character` to refresh. Dismiss leaves the orphan; banner reappears next load. + +**Modal vs. inline:** inline. Matches existing missing-content banner precedent; lets user read both without losing context. + +### Reconciler input — use existing aggregation + +The reconciler today receives `loaded-plugin-classes` (plugin classes only — built-ins are not in the list). For `:unbound-classes` to be accurate, it needs to know about built-ins too. + +**Do not build a new helper.** The app already aggregates via `::classes5e/classes` at `spell_subs.cljs:937-960` — that's what the dropdown consumes. The reconciler should consume the same source. For event-handler context, either replicate the aggregation logic directly from `db` (built-in keys are source-code constants in `class5e`; plugin keys come from `(:plugins db)`) or call the same helper functions the sub uses. + +With key-based derivation, the reconciler's needs simplify further — for the spell-selection rewrite path it only needs "set of known class keys" (built-ins ∪ plugins). No `:name` lookups. Class names matter only for the relink UI's candidate ranking. + +## Why these decisions + +This was a multi-hour conversation that took several wrong turns. Documenting so the next agent doesn't repeat them. + +### Why phase 1 wasn't enough + +The shipped fix reverted the active leak but left identity still flowing through `:name`. The defense was "don't mutate `:name`" — a convention, enforced by docs and comments. The user pointed out this is exactly the kind of convention-only defense that fails the moment someone forgets, and identity hardening should be architectural, not procedural. Hence the pivot to class-key derivation. + +### Why `:parked` is vapor and was removed from the plan + +The reconciler returned `{:rewrote [...] :parked [...]}`. `:parked` was designed to feed a "manual rebind" UI for orphans the auto-rewrite couldn't resolve. Two failure modes were imagined: +- Multiple candidates within a class entry (ambiguous suffix match) — **unreachable** because `class->expected-spell-keys` emits exactly 2 keys with distinct suffixes ("cantrips-known", "spells-known"); per-orphan, candidates is always 0 or 1. +- Zero candidates — only fires if the orphan has a malformed suffix the regex would already filter out. + +The "originating class not loaded" case the parking UI was meant to surface is a class-level orphan, not a spell-selection orphan. It's covered by the existing "Missing Content" banner at `character_builder.cljs:1940-1972` (which subscribes to `::char5e/missing-content-report`). + +**Replacement:** `:unbound-classes` actually fires when a class isn't loaded — that's real data. The relink UI consumes that, not `:parked`. + +### Why the plugin-load race is a non-issue (verified) + +A reviewer worried that `:set-character` might fire before plugins hydrate, leaving the reconciler with empty expected-keys. **Not possible** — `::e5/plugins` is registered via `reg-local-store-cofx` at `db.cljs:302` (synchronous localStorage read). `:initialize-db` (`events.cljs:208`) injects both plugins and character cofx and writes them atomically into the same `db`. Subsequent `:set-character` dispatches see fully-hydrated plugins. The "user opened a character on a machine without the plugin" case is real but distinct — handled by the missing-content path / soon the relink UI, not by a race fix. + +### Why import-rename is fragile (the editor save problem) + +The user spotted this mid-conversation. `import_validation.cljs:1388` only renames `:key`. `events.cljs:544` always regenerates `:key` from `:name` on save. So conflict-renamed homebrew keys (`:artificer-kibbles-tasty`) are transient — first edit collapses them back to `:artificer`. This is what phase 3 fixes by getting the abbreviation into `:name`. + +### Why we keep the suffix toggle + +Phase 1 introduced `::show-class-source-suffix`. Originally framed as "decouple display from identity"; under the new architecture (key-based derivation) it's now purely a display preference. No architectural debt, low complexity. Keep. + +### What we are deliberately not doing + +- **Editor-side rebind trigger on rename.** Reconciler runs at next character load (lazy, per-character). Editor-mediated trigger is more immediate but adds plumbing. Defer. +- **Subclass-imported-in-separate-orcbrew-from-parent linking.** Cross-orcbrew-file subclass-to-class link breakage. Real but narrow blast radius (one subclass file referencing a renamed parent). Punt to `docs/TODO.md` or the agents-repo TODO. +- **Generalize the relink mechanism to all content types (race, feat, subrace).** Same pattern would apply but each needs its own audit. Spell-selection / class / subclass coverage is the scope of this PR. + +## Files to know + +### Modified or created in phase 1 (already shipped) +- `src/cljs/orcpub/dnd/e5/spell_subs.cljs` (`:467-485` — mutation revert; new comment above the sub) +- `src/cljc/orcpub/template.cljc` (`:74-88` — `::plugin-source` slot on `option-cfg`) +- `src/cljc/orcpub/dnd/e5/options.cljc` (`:2860-2887` — `class-option` pass-through; `:680-681` dead-binding cleanup) +- `src/cljs/orcpub/character_builder.cljs` (`:197-222` — toggle UI + display helper) +- `src/cljs/orcpub/dnd/e5/db.cljs` (`:283` — pref spec) +- `src/cljs/orcpub/dnd/e5/events.cljs` (`:1213-1230` — reconciler wiring + `console.info`; toggle handler elsewhere) +- `src/cljs/orcpub/dnd/e5/subs.cljs` — pref subscription +- `src/cljs/orcpub/dnd/e5/content_reconciliation.cljs` — `reconcile-spell-selection-keys` + helpers +- `test/cljs/orcpub/dnd/e5/content_reconciliation_test.cljs` — reconciler tests +- `docs/kb/key-vs-name-separation.md` + `docs/kb/README.md` + +### To modify in phase 2 + 3 +- `src/cljc/orcpub/dnd/e5/options.cljc` (`:469`, `:611-620`, `:680-687`, `:635`, `:1919-1928`) — kw derivation switch + caller adjustments +- `src/cljc/orcpub/dnd/e5/template_base.cljc` (`:275`) — `?prepare-spell-count` takes class-key +- `src/cljs/orcpub/dnd/e5/content_reconciliation.cljs` — drop `:parked`, add `:unbound-classes`, add subclass-mismatch detection, update `class->expected-spell-keys` +- `src/cljs/orcpub/dnd/e5/import_validation.cljs` (`:1388-1394` and surrounding) — abbreviation rule, name-suffixing, numeric tie-breaker +- `src/cljs/orcpub/dnd/e5/events.cljs` — broaden `loaded-plugin-classes` to include built-ins, or call the same aggregation the sub uses; add relink event handlers +- `src/cljs/orcpub/dnd/e5/subs.cljs` — subscription for unbound-classes / subclass-mismatch from reconciler state +- `src/cljs/orcpub/character_builder.cljs` — relink UI section (model after existing missing-content banner at `:1940-1972`) +- `src/cljs/orcpub/dnd/e5/views.cljs` — relink UI shared components if any +- `test/cljs/orcpub/dnd/e5/content_reconciliation_test.cljs` — new tests for key-based expected, unbound-classes detection, subclass-mismatch +- `docs/kb/key-vs-name-separation.md` — rewrite the "design rule defended by convention" framing into "rule enforced by architecture" +- `docs/TODO.md` — add cross-file subclass-linking note + +## Reasoning traps the previous agent fell into + +If you find yourself doing any of these, **stop and re-read this doc.** + +- Conflating `class-name` (a function parameter) with `:name` (a field on class config). They're synonymous *by convention* — every caller of `spell-selection` populates `:class-name` from `(:name cls-cfg)`. But the parameter slot itself is arbitrary. +- Suggesting we change `title` derivation. `title` is downstream of `class-name`. The fix targets the upstream input wherever `:name`-derived strings reach `name-to-kw`, not the downstream display string. +- Proposing UI for `:parked` orphans. They don't exist in real data. Don't build for vapor. +- Inventing a parallel helper for "loaded classes" when the dropdown already has access to the aggregated set via `::classes5e/classes`. Use what exists. +- Claiming the plugin-load race is real. It isn't. The cofx is synchronous; both plugins and character land in `db` in the same `:initialize-db` atomic write. +- Treating the half-fix (revert `:name` mutation, leave identity name-derived) as a complete solution. It addresses the active regression but leaves the architecture defended by convention only. The user explicitly rejected this as a stopping point. + +## Open questions + +- Should the stored abbreviation field be added? (Pros/cons in the conversation; user undecided.) +- Modal vs. inline for relink UI? (Tentatively inline, matching missing-content banner precedent. Confirm before building.) +- Candidate ranking for unbound-class relink: exact name match → prefix share → everything else. Sensible default, confirm. + +## References + +- Design rule and case study: `docs/kb/key-vs-name-separation.md` +- Existing missing-content banner (UI precedent for relink): `src/cljs/orcpub/character_builder.cljs:1940-1972` +- Existing aggregated classes subscription: `src/cljs/orcpub/dnd/e5/spell_subs.cljs:937-960` (`::classes5e/classes`) +- Existing import-rename logic: `src/cljs/orcpub/dnd/e5/import_validation.cljs:1388-1394` (`generate-new-key`) +- Existing class editor save (do not touch): `src/cljs/orcpub/dnd/e5/events.cljs:534-563` (`reg-save-homebrew`) diff --git a/docs/kb/README.md b/docs/kb/README.md index dad7d896f..9ee639445 100644 --- a/docs/kb/README.md +++ b/docs/kb/README.md @@ -9,6 +9,7 @@ direct inspection of code, logs, or authoritative references. Speculation is mar | Document | Topic | Source quality | |----------|-------|---------------| | [datomic-crash-analysis.md](datomic-crash-analysis.md) | Datomic transactor crashes — root cause, frequency, fix options | High — direct log analysis from `logs/datomic.{1,2,3}.log` | +| [key-vs-name-separation.md](key-vs-name-separation.md) | Class config `:key`/`:name` separation, cantrip regression case study, `name-to-kw(:name)` audit pattern, plugin-load race verification | High — direct code trace | ## Contribution rules diff --git a/docs/kb/key-vs-name-separation.md b/docs/kb/key-vs-name-separation.md new file mode 100644 index 000000000..114be60fc --- /dev/null +++ b/docs/kb/key-vs-name-separation.md @@ -0,0 +1,87 @@ +# Class config: `:key` is identity, `:name` is display + +**Date:** 2026-05-04 +**Source:** Direct code analysis of `spell_subs.cljs:467-485`, `options.cljc:469`, `template_base.cljc:275`, `options.cljc:635, 1060, 2904`, `template.cljc:74-88`, `db.cljs:267-305`, `events.cljs:208-228`, `content_reconciliation.cljs`, plus session reasoning trail dated 2026-05-04. + +## TL;DR design rule + +`:key` is identity. `:name` is display. Display strings never bleed into keys. + +- *Inside the editor:* renaming a class moves the key with the name (e.g. `gambit` → `charlatan`). Editor flow owns key changes and the rebinding of dependent selections. +- *Outside the editor:* `:key` is read, never re-derived. `name-to-kw(:name)` outside the editor/import path is the bug pattern — apparent in the cantrip regression case study below. +- Built-in classes (`:cleric`, `:wizard`, ...) are source-code constants — never rename, never collide. + +## Case study: cantrip/spell selection regression (2026) + +### Symptom +Homebrew Cleric/Druid replacements lose cantrip/spell selections after a UX change to suffix the source name onto class display labels. Saved entity still has `:cleric-cantrips-known`; UI shows nothing selected. Setting the orcbrew source name to blank temporarily "fixes" it. + +### Root cause +`spell_subs.cljs:475-478` (`::classes5e/plugin-classes` sub) mutated class `:name` to `"Cleric (Source)"` for display. Class `:key` was untouched. The mutation contract on `:479-480` claimed *"`:name` is display-only"* — but downstream code violated that contract by re-deriving identity via `common/name-to-kw` from the mutated `:name`. + +### The four downstream leak sites + +| # | Site | What leaked | +|---|------|-------------| +| 1 | `options.cljc:469` (`spell-selection`) | mutated `name` → `name-to-kw` → `::t/key` (the cantrip bug) | +| 2 | `template_base.cljc:275` (`?prepare-spell-count`) | mutated `class-name` → `name-to-kw` → lookup miss against canonical `?spell-slot-factors` | +| 3 | `options.cljc:2951` (`mods/map-mod ?prepares-spells`) | feeds #2; same root | +| 4 | `options.cljc:635` (`class-key-name` fallback) | latent; current callers pass `class-key`, but the fallback is wrong | + +### Fix +Reverted the `:name` mutation. Plumbed `:plugin-source` through `option-cfg` as a distinct `::plugin-source` slot. Display suffix computed at the dropdown render site, gated by a new `::show-class-source-suffix` user pref. All four leaks heal from the single revert. + +A load-time reconciler (`reconcile-spell-selection-keys` in `content_reconciliation.cljs`) heals regression-window orphans on character load when a single suffix-match candidate exists. + +## When `name-to-kw(:name)` is benign vs. a leak + +Common false-alarm pattern during reviews — `name-to-kw(:name)` shapes can look identical but differ in safety. + +**Benign (constructor / minting time):** +- `template.cljc:option-cfg` / `:selection-cfg` — destructured `name` kwarg, derived once at template-build time +- `options.cljc:feat-option` (`:1060`) — operates on feat configs whose `:name` is not mutated upstream +- `common.cljc:add-keys` — applied to static seed data (equipment, weapons, conditions, monsters, spells) +- Import path slugification (`import_validation.cljs`) +- Homebrew duplicate-detection (`views.cljs:6404+`) — operates on user-typed names in the editor + +The shared property: `:name` is stable at the time of derivation. The key gets minted once from a canonical name. + +**Leak shape:** +- `:name` is *mutated upstream* of the identity derivation (e.g., a sub appends a source label to `:name` for display, then a downstream consumer derives a key from it) +- The derived key drifts away from the canonical key the rest of the pipeline expects +- Saved data referencing the canonical key is orphaned + +**Audit heuristic:** for any `name-to-kw(:name)` site, trace `:name`'s upstream. If any code rewrites `:name` for display purposes, you have a leak. If `:name` is the canonical name from source code or stable config, you don't. + +## Plugin-load race: verified non-issue + +**✅ VERIFIED 2026-05-04 — direct trace of `db.cljs` and `events.cljs`.** + +A reviewer worried that `:set-character` could fire before plugins hydrate, leaving any reconciliation logic with empty `:plugins` in `db`. Confirmed not the case: + +- `::e5/plugins` is registered via `reg-local-store-cofx` at `db.cljs:302` — synchronous localStorage read. +- `:initialize-db` at `events.cljs:208` injects `:local-store-character` and `::e5/plugins` as cofx in the same handler. +- The handler writes both atomically into `db` via a single `cond->` (`events.cljs:222-228`). +- Subsequent `:set-character` dispatches see `db` with plugins already populated. + +The "user opened a character on a machine without the originating plugin imported" case is real but distinct — that's correctly the missing-content / parked-orphan case, not a race. Reconciler treats it conservatively (no silent rewrite). + +## Reconciler approach: transient parked state + +The reconciler returns `{:rewrote [...] :parked [...]}`: +- `:rewrote` — orphan keys with a single unambiguous canonical candidate; rewritten in place at character load. +- `:parked` — orphans with multiple candidates (multiclass overlap) or zero candidates (originating class not loaded). Data preserved on the entity; held in transient db state for UI banner. + +**Design call:** parked entries are NOT persisted to the entity as a separate field. They're re-derived every character load from the entity's orphan keys × currently-loaded plugins. This avoids a schema slot that could drift from the underlying data. + +Resolution UI lives in `views.cljs` (banner + resolution view); resolution event mutates the entity and re-runs the reconciler. + +## Source comments worth leaving + +For sites where future reviewers are most likely to speculate about behavior: + +1. **`spell_subs.cljs` `::classes5e/plugin-classes`** — note that `:name` was historically mutated for display and the mutation was reverted; refer to this kb doc. +2. **`content_reconciliation.cljs:reconcile-class-entry-options`** — note that the "multiple candidates within one class entry" branch is unreachable today (single suffix per class) but the cross-class accumulator in `reconcile-spell-selection-keys` populates `:parked` for multiclass overlap. +3. **`events.cljs` reconciler invocation site** — one-liner that the reconciler trusts `db` plugins to be hydrated because `::e5/plugins` is a sync cofx at `:initialize-db`. + +Comments should be one-liners pointing here, not duplications of this doc. diff --git a/src/cljc/orcpub/dnd/e5/classes.cljc b/src/cljc/orcpub/dnd/e5/classes.cljc index 8e4656529..7c6ae534a 100644 --- a/src/cljc/orcpub/dnd/e5/classes.cljc +++ b/src/cljc/orcpub/dnd/e5/classes.cljc @@ -27,6 +27,13 @@ (spec/def ::homebrew-boon (spec/keys :req-un [::name ::key ::option-pack])) +(def base-class-keys + "SRD built-in class keys. Source-code constants; never rename, never collide. + Canonical location for this set; other namespaces should reference it from + here rather than redefining." + #{:barbarian :bard :cleric :druid :fighter :monk + :paladin :ranger :rogue :sorcerer :warlock :wizard}) + (defn class-level [levels class-kw] (get-in levels [class-kw :class-level])) diff --git a/src/cljc/orcpub/dnd/e5/options.cljc b/src/cljc/orcpub/dnd/e5/options.cljc index 6b6c5ef01..87d52c605 100644 --- a/src/cljc/orcpub/dnd/e5/options.cljc +++ b/src/cljc/orcpub/dnd/e5/options.cljc @@ -461,12 +461,26 @@ #(memoized-spell-option spells-map spellcasting-ability class-name % prepend-level? qualifier) (sort spells))) -(defn spell-level-title [class-name level] +(defn spell-level-title + "Display title for a class's spell selection at a given level." + [class-name level] (str class-name (if (and level (zero? level)) " Cantrips Known" (str " Spells Known" (when level (str " " level)))))) +(defn spell-selection-key + "Identity-derived selection key for a class's spell selection at a given + level. Mirrors the shape spell-level-title produces but rooted in + :class-key, not :name. See docs/kb/key-vs-name-separation.md." + [class-key level] + (keyword (str (name class-key) + (if (and level (zero? level)) + "-cantrips-known" + (str "-spells-known" (when level (str "-" level))))))) + (defn spell-selection [spell-lists spells-map {:keys [title class-key level spellcasting-ability class-name num prepend-level? spell-keys options min max exclude-ref? ref]}] + ;; Identity (kw) derives from :class-key. :class-name still feeds title + ;; for display. See docs/kb/key-vs-name-separation.md. (let [title (or title (spell-level-title class-name level)) - kw (common/name-to-kw title) + kw (spell-selection-key class-key level) ref (or ref (when (not exclude-ref?) [:class class-key kw]))] (t/selection-cfg {:name title @@ -629,14 +643,6 @@ spell-keys) spell-keys)) -(defn class-key-name [cls-key cls-nm] - (if cls-key - (name cls-key) - (common/name-to-kw cls-nm))) - -(defn spell-selection-key [cls-key-nm] - (keyword (str cls-key-nm "-spells-known"))) - (defn spells-known-selections [spell-lists spells-map @@ -677,17 +683,14 @@ filtered-keys))) all-spells))] (assoc m cls-lvl - [(let [cls-key-nm (class-key-name (:key cls-cfg) (:name cls-cfg)) - kw (spell-selection-key cls-key-nm) - cls-nm (:name cls-cfg)] - (spell-selection - spell-lists - spells-map - {:class-key class-key - :class-name cls-nm - :min num - :max (when (not acquire?) num) - :options options}))]))) + [(spell-selection + spell-lists + spells-map + {:class-key class-key + :class-name (:name cls-cfg) + :min num + :max (when (not acquire?) num) + :options options})]))) {} spells-known)) @@ -2867,6 +2870,7 @@ help hit-die plugin? + plugin-source profs levels ability-increase-levels @@ -2904,6 +2908,7 @@ (t/option-cfg {:name name :key kw + :plugin-source plugin-source :help [:div.p-t-5.p-l-10.p-r-10 (class-help hit-die save-profs weapon-profs armor-profs) [:div.m-t-10 help]] diff --git a/src/cljc/orcpub/dnd/e5/template_base.cljc b/src/cljc/orcpub/dnd/e5/template_base.cljc index 636e67a2c..4ae7ee467 100644 --- a/src/cljc/orcpub/dnd/e5/template_base.cljc +++ b/src/cljc/orcpub/dnd/e5/template_base.cljc @@ -272,6 +272,14 @@ (?class-level class-kw) (get ?spell-slot-factors class-kw))) ?prepare-spell-count (fn [class-name] + ;; Latent name-to-kw on class-name. Active risk is + ;; bounded — callers pass class-name from spell entries' + ;; :class field (sourced from canonical hardcoded source + ;; strings or class config :name, which phase 1 keeps + ;; canonical). Propagating class-key through spell-data + ;; and the ~100 mod5e/spells-known call sites is its own + ;; PR; tracked in docs/TODO.md. See + ;; docs/kb/key-vs-name-separation.md. (let [class-kw (common/name-to-kw class-name) slot-factor (get ?spell-slot-factors class-kw) spell-mods ?spell-modifiers diff --git a/src/cljc/orcpub/template.cljc b/src/cljc/orcpub/template.cljc index b8ddffc80..891262f8d 100644 --- a/src/cljc/orcpub/template.cljc +++ b/src/cljc/orcpub/template.cljc @@ -71,7 +71,7 @@ ::prereq-fn func ::hide-if-fail? hide-if-fail?}) -(defn option-cfg [{:keys [:db/id name key help selections modifiers associated-options prereqs order ui-fn icon select-fn edit-event] :as cfg}] +(defn option-cfg [{:keys [:db/id name key help selections modifiers associated-options prereqs order ui-fn icon select-fn edit-event plugin-source] :as cfg}] {::id id ::name name ::key (or key (common/name-to-kw name)) @@ -84,7 +84,8 @@ ::ui-fn ui-fn ::select-fn select-fn ::icon icon - ::edit-event edit-event}) + ::edit-event edit-event + ::plugin-source plugin-source}) (declare make-modifier-map-from-selections) (declare make-plugin-map-from-selections) diff --git a/src/cljs/orcpub/character_builder.cljs b/src/cljs/orcpub/character_builder.cljs index 42f7b790e..d1f0519e3 100644 --- a/src/cljs/orcpub/character_builder.cljs +++ b/src/cljs/orcpub/character_builder.cljs @@ -194,12 +194,18 @@ (def levels-selection #(when (= :levels (::t/key %)) %)) +(defn class-option-display-name [name plugin-source show-suffix?] + (if (and show-suffix? plugin-source) + (str name " (" plugin-source ")") + name)) + (defn class-level-selector [] (let [expanded? (r/atom false)] (fn [i key selected-class options unselected-classes-set built-char] (let [options-map (make-options-map options) class-template-option (options-map key) - path [:class-levels key]] + path [:class-levels key] + show-suffix? @(subscribe [::subs5e/show-class-source-suffix])] [:div.m-b-5 {:class (when @expanded? "b-1 b-rad-5 p-5")} [:div.flex.align-items-c @@ -208,13 +214,14 @@ :on-change (set-class i options-map)} (doall (map - (fn [{:keys [::t/key ::t/name] :as option}] + (fn [{:keys [::t/key ::t/name ::t/plugin-source] :as option}] (let [failed-prereqs (when (pos? i) (prereq-failures option built-char))] ^{:key key} [:option.builder-dropdown-item {:value key :disabled (seq failed-prereqs)} - (str name (when (seq failed-prereqs) (str " (" (s/join ", " failed-prereqs) ")")))])) + (str (class-option-display-name name plugin-source show-suffix?) + (when (seq failed-prereqs) (str " (" (s/join ", " failed-prereqs) ")")))])) (sort-by ::t/name (filter @@ -270,6 +277,7 @@ (let [options (::t/options selection) built-char @(subscribe [:built-character]) selected-classes @(subscribe [::char5e/levels]) + show-suffix? @(subscribe [::subs5e/show-class-source-suffix]) unselected-classes (remove (set (keys selected-classes)) (map ::t/key options)) @@ -281,6 +289,9 @@ (entity/meets-prereqs? option built-char))) options)] [:div + [:div.m-b-5 + {:on-click #(dispatch [::events5e/toggle-class-source-suffix])} + [views5e/labeled-checkbox "Show homebrew source on class names" show-suffix?]] [:div (doall (map-indexed diff --git a/src/cljs/orcpub/dnd/e5/content_reconciliation.cljs b/src/cljs/orcpub/dnd/e5/content_reconciliation.cljs index c42b64da0..7f5550488 100644 --- a/src/cljs/orcpub/dnd/e5/content_reconciliation.cljs +++ b/src/cljs/orcpub/dnd/e5/content_reconciliation.cljs @@ -10,7 +10,8 @@ the entire options tree generically." (:require [clojure.string :as str] [orcpub.entity :as entity] - [orcpub.common :as common])) + [orcpub.common :as common] + [orcpub.dnd.e5.classes :as class5e])) ;; ============================================================================ ;; Content Type Definitions @@ -160,10 +161,6 @@ ;; Only SRD content belongs here. Non-SRD PHB content (Battle Master, ;; Folk Hero, etc.) comes from plugins and SHOULD be flagged when removed. -(def ^:private builtin-classes - #{:barbarian :bard :cleric :druid :fighter :monk - :paladin :ranger :rogue :sorcerer :warlock :wizard}) - (def ^:private builtin-races #{:dwarf :elf :halfling :human :dragonborn :gnome :half-elf :half-orc :tiefling}) @@ -201,7 +198,7 @@ "True if this key is SRD built-in content that won't appear in plugin subs." [k content-type] (case content-type - :class (contains? builtin-classes k) + :class (contains? class5e/base-class-keys k) :subclass (contains? builtin-subclasses k) :race (contains? builtin-races k) :subrace (contains? builtin-subraces k) @@ -258,3 +255,117 @@ {:has-missing? (boolean (seq missing)) :missing-count (count missing) :items (vec missing)})) + +;; ============================================================================ +;; Spell Selection Key Reconciliation +;; ============================================================================ +;; +;; During a regression window, the plugin-classes sub mutated class :name +;; to "Cleric (Source)", which leaked into spell-selection :key derivation +;; (selection keys are computed via name-to-kw of the class name + suffix). +;; Characters built or re-saved during the window have selection keys like +;; :cleric-source-cantrips-known. After reverting the mutation, the template +;; uses the canonical :cleric-cantrips-known again, leaving the saved key +;; orphaned and the selections invisible. +;; +;; This reconciler heals unambiguous orphans at character load. + +(def ^:private spell-selection-suffix-re + #"^.+?-(cantrips-known|spells-known)$") + +(defn- spell-selection-suffix + "Returns the trailing suffix (\"cantrips-known\" or \"spells-known\") + for a spell-selection-shaped key, else nil." + [k] + (when (keyword? k) + (when-let [match (re-matches spell-selection-suffix-re (name k))] + (second match)))) + +(defn- class->expected-spell-keys + "Set of canonical spell-selection keys for a class entry with this :key. + Mirrors options/spell-selection-key. See docs/kb/key-vs-name-separation.md." + [class-key] + (when class-key + #{(keyword (str (name class-key) "-cantrips-known")) + (keyword (str (name class-key) "-spells-known"))})) + +(defn- reconcile-class-entry-options + "Walk one class entry's option map. Returns {:options reconciled :rewrote [...]}. + Orphan spell-selection keys with a single suffix-match candidate in the + expected set are rewritten in place; everything else passes through + unchanged. The existing missing-content banner surfaces class-level + orphans (entries whose class isn't loaded)." + [class-key options expected-keys] + (reduce-kv + (fn [acc k v] + (let [suffix (spell-selection-suffix k)] + (cond + (nil? suffix) + (update acc :options assoc k v) + + (contains? expected-keys k) + (update acc :options assoc k v) + + :else + (let [candidates (filter #(= suffix (spell-selection-suffix %)) + expected-keys)] + (if (= 1 (count candidates)) + (-> acc + (update :options assoc (first candidates) v) + (update :rewrote conj + {:class-key class-key + :from k + :to (first candidates)})) + (update acc :options assoc k v)))))) + {:options {} :rewrote []} + options)) + +(defn reconcile-spell-selection-keys + "Heal regression-window + migration-window spell-selection key orphans on a + character. Runs at :set-character (lazy, per-character-on-view). + + With key-based kw derivation, the canonical spell-selection key for a class + entry is :{class-key}-cantrips-known / :{class-key}-spells-known. Saved + characters bound to the older :name-derived shape (e.g. + :artificer-cantrips-known under a class entry whose :key is + :artificer-kibbles-tasty) get auto-rewritten via suffix match. + + Args: + - character: character entity (with ::entity/options) + - loaded-class-keys: collection of class keys the system knows about right + now (built-ins + plugins; same source the class dropdown consumes via + ::classes5e/classes). + + For each class entry in the character whose :key is in the loaded set, + walk its options and rewrite spell-selection-shaped keys to the canonical + class-key-derived form when there's a single suffix-match candidate. + Class entries whose :key is NOT loaded pass through unchanged — the + existing missing-content banner surfaces them for user-driven relink. + + Returns: + {:character reconciled-character + :rewrote [{:class-key K :from K1 :to K2} ...]}" + [character loaded-class-keys] + (let [known-keys (set loaded-class-keys) + class-entries (get-in character [::entity/options :class])] + (if (sequential? class-entries) + (let [{:keys [entries rewrote]} + (reduce + (fn [acc class-entry] + (let [class-key (::entity/key class-entry) + expected (when (contains? known-keys class-key) + (class->expected-spell-keys class-key))] + (if (empty? expected) + (update acc :entries conj class-entry) + (let [opts (or (::entity/options class-entry) {}) + {:keys [options rewrote]} + (reconcile-class-entry-options class-key opts expected)] + (-> acc + (update :entries conj + (assoc class-entry ::entity/options options)) + (update :rewrote into rewrote)))))) + {:entries [] :rewrote []} + class-entries)] + {:character (assoc-in character [::entity/options :class] entries) + :rewrote rewrote}) + {:character character :rewrote []}))) diff --git a/src/cljs/orcpub/dnd/e5/db.cljs b/src/cljs/orcpub/dnd/e5/db.cljs index 51f8d50ee..37cf935e2 100644 --- a/src/cljs/orcpub/dnd/e5/db.cljs +++ b/src/cljs/orcpub/dnd/e5/db.cljs @@ -280,8 +280,9 @@ (spec/def ::theme string?) (spec/def ::patron string?) ; patron (spec/def ::patron-tier string?) ; patron-tier +(spec/def ::show-class-source-suffix boolean?) (spec/def ::user-data (spec/keys :req-un [::username ::email])) -(spec/def ::user (spec/keys :opt-un [::user-data ::token ::theme ::patron ::patron-tier])) +(spec/def ::user (spec/keys :opt-un [::user-data ::token ::theme ::patron ::patron-tier ::show-class-source-suffix])) (reg-local-store-cofx :local-store-user diff --git a/src/cljs/orcpub/dnd/e5/events.cljs b/src/cljs/orcpub/dnd/e5/events.cljs index 98bea8c97..b02bbe2ce 100644 --- a/src/cljs/orcpub/dnd/e5/events.cljs +++ b/src/cljs/orcpub/dnd/e5/events.cljs @@ -29,6 +29,7 @@ [orcpub.dnd.e5.magic-items :as mi] [orcpub.dnd.e5.event-handlers :as event-handlers] [orcpub.dnd.e5.character.equipment :as char-equip5e] + [orcpub.dnd.e5.content-reconciliation :as content-recon] [orcpub.dnd.e5.db :refer [default-value character->local-store user->local-store @@ -1209,8 +1210,29 @@ :url (backend-url path) :on-success [:unfollow-user-success]}}))) +(defn- loaded-class-keys + "Set of class keys currently known to the system — SRD built-ins plus + enabled plugin classes from db :plugins. Same source the class dropdown + consumes via ::classes5e/classes." + [db] + (into class5e/base-class-keys + (for [[_ plugin-data] (:plugins db) + :when (and (map? plugin-data) (not (:disabled? plugin-data))) + [class-key class-data] (::e5/classes plugin-data) + :when (and (map? class-data) (not (:disabled? class-data)))] + class-key))) + (defn set-character [db [_ character]] - (assoc db :character character :loading false)) + ;; db :plugins are already hydrated here — ::e5/plugins is a sync cofx at + ;; :initialize-db, so the reconciler can trust loaded-class-keys. + ;; See docs/kb/key-vs-name-separation.md. + (let [{:keys [character rewrote]} + (content-recon/reconcile-spell-selection-keys + character + (loaded-class-keys db))] + (when (seq rewrote) + (js/console.info "Reconciled spell-selection keys:" (clj->js rewrote))) + (assoc db :character character :loading false))) (reg-event-db :toggle-character-expanded @@ -2640,6 +2662,12 @@ "dark-theme" "light-theme"))))) +(reg-event-db + ::toggle-class-source-suffix + [user->local-store-interceptor] + (fn [db _] + (update-in db [:user-data :show-class-source-suffix] not))) + #_ ;; never dispatched from UI (reg-event-db ::mi/set-builder-item @@ -4659,16 +4687,12 @@ (fn [db _] (assoc-in db [::char5e/delete-plugin-confirmation-shown?] false))) -;; Base class keys that are always available (not from plugins) -(def base-class-keys - #{:barbarian :bard :cleric :druid :fighter :monk :paladin :ranger :rogue :sorcerer :warlock :wizard}) - (defn remove-plugin-classes "Removes classes from character that aren't base classes. If no classes remain, sets to Barbarian. Preserves all other character data." [character] (let [current-classes (get-in character [::entity/options :class]) - valid-classes (vec (filter #(base-class-keys (::entity/key %)) current-classes))] + valid-classes (vec (filter #(class5e/base-class-keys (::entity/key %)) current-classes))] (if (seq valid-classes) ;; Keep only valid base classes (assoc-in character [::entity/options :class] valid-classes) diff --git a/src/cljs/orcpub/dnd/e5/spell_subs.cljs b/src/cljs/orcpub/dnd/e5/spell_subs.cljs index 8d6b530bf..9ef031abb 100644 --- a/src/cljs/orcpub/dnd/e5/spell_subs.cljs +++ b/src/cljs/orcpub/dnd/e5/spell_subs.cljs @@ -451,6 +451,10 @@ :when (and (map? subclass-data) (not (:disabled? subclass-data)))] [source-name subclass-key subclass-data])))) +;; :name on plugin classes is canonical; do not mutate it to embed source +;; (the source label is carried separately as :plugin-source). Mutating :name +;; causes downstream name-to-kw to derive shifted keys; see +;; docs/kb/key-vs-name-separation.md. (reg-sub ::classes5e/plugin-classes :<- [::e5/plugins-with-sources] @@ -459,26 +463,14 @@ :<- [::selections5e/selection-map] (fn [[plugins-with-sources spell-lists spells-map selection-map]] ;; Defensive handling: skip malformed classes rather than breaking - ;; Also includes source name for disambiguation when multiple sources - ;; have classes with the same name (e.g., two different "Artificer" classes) + ;; Source name carried as :plugin-source for display; never folded into :name. (keep (fn [[source-name class-key class]] (try (when (and (map? class) class-key) - (let [;; Ensure the class has its key set (the map key is the authoritative key) - class-with-key (assoc class :key class-key) - levels (make-levels spell-lists spells-map selection-map class-with-key) - ;; Add source name to class name for disambiguation - ;; Only if source name is meaningful (not default) - display-name (if (and source-name - (not= source-name "Default Option Source")) - (str (:name class) " (" source-name ")") - (:name class))] + (let [class-with-key (assoc class :key class-key) + levels (make-levels spell-lists spells-map selection-map class-with-key)] (assoc class-with-key - :name display-name - ;; :name is display-only (may include source suffix). - ;; All internal lookups use :key, never :name. - :original-name (:name class) :plugin-source source-name :modifiers (opt5e/plugin-modifiers (:props class) class-key) diff --git a/src/cljs/orcpub/dnd/e5/subs.cljs b/src/cljs/orcpub/dnd/e5/subs.cljs index ae6570f5a..ce69e3912 100644 --- a/src/cljs/orcpub/dnd/e5/subs.cljs +++ b/src/cljs/orcpub/dnd/e5/subs.cljs @@ -1076,6 +1076,11 @@ (fn [db _] (get-in db [:user-data :theme]))) +(reg-sub + ::show-class-source-suffix + (fn [db _] + (boolean (get-in db [:user-data :show-class-source-suffix])))) + (reg-sub ::mi5e/builder-item (fn [db _] diff --git a/test/cljs/orcpub/dnd/e5/content_reconciliation_test.cljs b/test/cljs/orcpub/dnd/e5/content_reconciliation_test.cljs index f6bfa3f7e..347b03e43 100644 --- a/test/cljs/orcpub/dnd/e5/content_reconciliation_test.cljs +++ b/test/cljs/orcpub/dnd/e5/content_reconciliation_test.cljs @@ -206,3 +206,149 @@ "all 3 feats should be flagged as missing") (is (= #{:blade-mastery :brawny :metabolic-control} (set (map :key missing-feats))))))) + +;; ============================================================================ +;; Spell Selection Key Reconciliation +;; ============================================================================ +;; +;; Regression-window characters had spell-selection keys derived from a +;; mutated class :name (e.g., :cleric-source-cantrips-known instead of the +;; canonical :cleric-cantrips-known). After reverting the mutation, the +;; loaded plugin-class data has its canonical :name back, so the canonical +;; selection keys can be reconstructed and used to heal orphaned saves. + +(defn- spell-selection-fixture + "Build a one-class character with a spell-selection options map under + the class entry. Returns the entity." + [class-key spell-options] + {::entity/options + {:class [{::entity/key class-key + ::entity/options spell-options}]}}) + +(deftest test-reconcile-rewrites-name-derived-orphan-to-key-derived + (testing "Pre-phase-2 saved selection key (name-derived) gets rewritten to + the key-derived shape when the class entry's key disambiguates. + Example: conflict-renamed homebrew :artificer-kibbles-tasty had + saved selections under :artificer-cantrips-known (slug from :name + 'Artificer'); phase 2 derives expected from class-key, producing + :artificer-kibbles-tasty-cantrips-known." + (let [character (spell-selection-fixture + :artificer-kibbles-tasty + {:artificer-cantrips-known + [{::entity/key :prestidigitation} + {::entity/key :guidance}]}) + loaded #{:artificer-kibbles-tasty} + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character loaded) + new-options (-> character ::entity/options :class first ::entity/options)] + (is (= 1 (count rewrote))) + (is (= {:class-key :artificer-kibbles-tasty + :from :artificer-cantrips-known + :to :artificer-kibbles-tasty-cantrips-known} + (first rewrote))) + (is (contains? new-options :artificer-kibbles-tasty-cantrips-known)) + (is (not (contains? new-options :artificer-cantrips-known))) + (is (= 2 (count (:artificer-kibbles-tasty-cantrips-known new-options))))))) + +(deftest test-reconcile-leaves-orphan-when-class-not-loaded + (testing "Class entry whose :key isn't in the loaded set is passed through + untouched. The existing missing-content banner handles the + user-facing alert; no silent rewriting against arbitrary classes." + (let [character (spell-selection-fixture + :artificer-kibbles-tasty + {:artificer-cantrips-known + [{::entity/key :guidance}]}) + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character #{}) + new-options (-> character ::entity/options :class first ::entity/options)] + (is (empty? rewrote)) + (is (contains? new-options :artificer-cantrips-known) + "orphan data preserved unchanged")))) + +(deftest test-reconcile-leaves-healthy-key-alone + (testing "Healthy canonical (key-derived) selection key is not touched" + (let [character (spell-selection-fixture + :artificer-kibbles-tasty + {:artificer-kibbles-tasty-cantrips-known + [{::entity/key :guidance}]}) + loaded #{:artificer-kibbles-tasty} + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character loaded) + new-options (-> character ::entity/options :class first ::entity/options)] + (is (empty? rewrote)) + (is (contains? new-options :artificer-kibbles-tasty-cantrips-known))))) + +(deftest test-reconcile-leaves-builtin-cleric-untouched + (testing "Built-in class with canonical keys is healthy and stays untouched" + (let [character (spell-selection-fixture + :cleric + {:cleric-cantrips-known + [{::entity/key :guidance}]}) + loaded #{:cleric} + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character loaded) + new-options (-> character ::entity/options :class first ::entity/options)] + (is (empty? rewrote)) + (is (contains? new-options :cleric-cantrips-known))))) + +(deftest test-reconcile-preserves-non-spell-selection-keys + (testing "Non-spell-selection keys are passed through unchanged" + (let [character (spell-selection-fixture + :artificer-kibbles-tasty + {:divine-domain {::entity/key :life-domain} + :skill-proficiency [{::entity/key :medicine}] + :artificer-cantrips-known [{::entity/key :guidance}]}) + loaded #{:artificer-kibbles-tasty} + {:keys [character]} + (reconcile/reconcile-spell-selection-keys character loaded) + new-options (-> character ::entity/options :class first ::entity/options)] + (is (= {::entity/key :life-domain} (:divine-domain new-options))) + (is (= [{::entity/key :medicine}] (:skill-proficiency new-options))) + (is (contains? new-options :artificer-kibbles-tasty-cantrips-known)) + (is (not (contains? new-options :artificer-cantrips-known)))))) + +(deftest test-reconcile-rewrites-spells-known-suffix + (testing "spells-known suffix follows the same rebind path as cantrips-known" + (let [character (spell-selection-fixture + :wizard-kibbles-tasty + {:wizard-spells-known + [{::entity/key :magic-missile} + {::entity/key :shield}]}) + loaded #{:wizard-kibbles-tasty} + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character loaded) + new-options (-> character ::entity/options :class first ::entity/options)] + (is (= :wizard-kibbles-tasty-spells-known (:to (first rewrote)))) + (is (contains? new-options :wizard-kibbles-tasty-spells-known)) + (is (= 2 (count (:wizard-kibbles-tasty-spells-known new-options))))))) + +(deftest test-reconcile-handles-character-with-no-classes + (testing "Character without :class entries returns unchanged result" + (let [character {::entity/options {}} + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character #{})] + (is (empty? rewrote)) + (is (= {::entity/options {}} character))))) + +(deftest test-reconcile-multiclass-each-class-isolated + (testing "Two classes each reconcile against their own expected keys. + Conflict-renamed homebrew with a saved name-derived orphan + rewrites; built-in alongside it stays healthy." + (let [character {::entity/options + {:class [{::entity/key :artificer-kibbles-tasty + ::entity/options + {:artificer-cantrips-known + [{::entity/key :guidance}]}} + {::entity/key :wizard + ::entity/options + {:wizard-cantrips-known + [{::entity/key :fire-bolt}]}}]}} + loaded #{:artificer-kibbles-tasty :wizard} + {:keys [character rewrote]} + (reconcile/reconcile-spell-selection-keys character loaded) + classes (-> character ::entity/options :class)] + (is (= 1 (count rewrote))) + (is (= :artificer-kibbles-tasty (:class-key (first rewrote)))) + (is (contains? (-> classes first ::entity/options) :artificer-kibbles-tasty-cantrips-known)) + (is (contains? (-> classes second ::entity/options) :wizard-cantrips-known) + "built-in class entry untouched")))) diff --git a/web-handoff.md b/web-handoff.md new file mode 100644 index 000000000..bd0625763 --- /dev/null +++ b/web-handoff.md @@ -0,0 +1,120 @@ +# Web handoff: cantrip/spell-selection regression + identity architecture + +Branch: `claude/fix-cantrips-selection-bug-CSwVv` +Audience: agent picking up the work in a fresh web session. Read this BEFORE making any claims about codebase state — local checkouts can be stale relative to the branch HEAD. Verify with `git log claude/fix-cantrips-selection-bug-CSwVv -20` and `git status` first. + +## The bug + +Homebrew Cleric/Druid replacements lose cantrip/spell selections after a recent UX change. Saved entity has `:cleric-cantrips-known`; UI shows nothing selected. Setting orcbrew source name to blank temporarily "fixes" it, proving the source label leaks into the selection key. + +Root cause: `spell_subs.cljs:475-478` (the `::classes5e/plugin-classes` sub) mutated class `:name` to `"Cleric (Source)"` for display. Downstream consumers re-derived identity via `common/name-to-kw` from the mutated `:name`. Saved characters bound to the canonical key got orphaned because the new template produced a different shifted key. + +## What's shipped on this branch (verify with git log) + +- `0a4f262` Phase 1 — revert `:name` mutation in `spell_subs.cljs`, plumb `:plugin-source` as a distinct `::plugin-source` slot through `option-cfg`, add `::show-class-source-suffix` user-pref toggle, add load-time reconciler `reconcile-spell-selection-keys` in `content_reconciliation.cljs`, wire into `:set-character`. Tests included. +- `a77d0a1` Kb doc: `docs/kb/key-vs-name-separation.md` — design rule, four-leak-site case study, plugin-load race verification. +- `46310f3` Three source comments pointing to the kb doc at speculation-prone sites. +- `af5e6fe` `console.info` on `:rewrote` results for field debugging. +- `dd4144d` Dense `HANDOFF.md` at repo root (older, denser sibling of this doc). +- `251e1a7` **Phase 2 — switch kw derivation from `class-name`-based to `class-key`-based.** Adds `spell-selection-key` helper in `options.cljc` (the dead one was repurposed). Deletes the actually-dead `class-key-name` fallback. Updates the reconciler so `class->expected-spell-keys` computes from `class-key`, drops the `:parked` accumulator (which was unreachable scaffolding for a UI we're not building). Adds a comment at `template_base.cljc:275` (`?prepare-spell-count`) explicitly deferring the propagation fix to a follow-up. Reconciler input changes shape from "seq of class data" to "set of known class keys"; events.cljs adjusted accordingly. Test fixtures rewritten to model the post-phase-2 migration shape. +- `1c24a8e` Dedupe `base-class-keys` — canonical home is now `classes.cljc`; the duplicate in `events.cljs:4691` is removed, and `content_reconciliation.cljs`'s `builtin-classes` set is folded back to reference the canonical one. + +## Architectural decision + +The hardest decision of the conversation: phase 1's revert defended the design rule by convention only — anyone re-mutating `:name` would replay the bug. The user explicitly rejected convention-only as a stopping point. Phase 2 commits to: + +> Outside the class editor, identity flows from `class-key`. `class-name` (i.e. `:name` on class config) is display only. Display chains stay; identity chains get rewired to use `class-key` directly. +> +> Inside the class editor, `(name-to-kw name)` at `events.cljs:534-563` (`reg-save-homebrew`) IS the legitimate `:name → :key` rename mechanism. The editor owns key changes intentionally. This is the carve-out exception to the rule. + +The bug class is now structurally impossible at the fixed site (`spell-selection`), not just convention-defended. + +## Verified findings + +- **Plugin-load race is not real.** Reviewer-raised concern that `:set-character` could fire before plugins hydrate, leaving the reconciler with empty expected-keys. **Verified false.** `::e5/plugins` is registered via `reg-local-store-cofx` at `db.cljs:302` — synchronous localStorage read. `:initialize-db` (`events.cljs:208`) injects both plugins and character cofx and writes them atomically into the same `db` value. Subsequent `:set-character` dispatches see fully-hydrated plugins. The "user opens character on a machine without the originating plugin" case is real but distinct — handled by the existing missing-content banner. + +- **Editor save regenerates `:key` from `:name`** at `events.cljs:534-563` on every homebrew save. Combined with import-rename only changing `:key` (not `:name`), this means import disambiguation is destroyed by any subsequent edit. Phase 3 (planned, not yet shipped) addresses this. + +- **Import-rename only changes `:key`** at `import_validation.cljs:1388-1394` (`generate-new-key`). `:name` is untouched. This is why disambiguation is fragile. + +- **`class->expected-spell-keys`'s `:parked` accumulator was vapor.** Verified by tracing the candidate count — `class->expected-spell-keys` always emits 2 keys with distinct suffixes, so candidates is always 0 or 1; single-survivor always rewrites; the multi-candidate parking branch was unreachable. Dropped in phase 2. + +- **`base-class-keys` was duplicated.** `events.cljs:4691` and `content_reconciliation.cljs:163` (`builtin-classes`) both defined the same 12-class SRD set. Canonical home is now `classes.cljc`; the duplicates were removed in `1c24a8e`. + +- **There is no lightweight runtime registry of built-in content.** Built-in classes are 12 imperative source-code functions (`barbarian-option`, `cleric-option`, etc. in `classes.cljc`). Their KEYS only exist inside the option-cfgs those functions return when called with real args. No data-shaped enumeration exists; the `base-class-keys` set is a hand-maintained shadow. Same pattern repeats for `builtin-races`, `builtin-subraces`, `builtin-subclasses`, `builtin-backgrounds`, `builtin-feats` — all currently shimmed via hardcoded sets in `content_reconciliation.cljs`. This is real architectural debt; see "open architectural concern" below. + +## What's NOT yet shipped (decisions made, code pending) + +### Phase 3 — import-rename hardening +Decision: append a short source ABBREVIATION (not the full slug) to both `:key` AND `:name` on import-conflict. +- Abbreviation rule: 1–3 words → first+last letter of each word (`"Kibbles' Tasty"` → `KsTy`); 4+ words → first letter only (`"Tasha's Cauldron of Everything"` → `TCoE`). Matches community-standard D&D abbreviation conventions. +- Numeric tie-breaker on actual collision (`KsTy` already taken → `KsTy2`). +- Override case: user wanting to override a built-in opens the class in the editor and removes `(KT)` from `:name`; save regenerates `:key` to `:cleric` (clean), override becomes active. The reconciler then handles orphan rebind on next character load. +- **Stored abbreviation field:** user undecided. Not adding to schema unless a collision-resolution UI gets built. + +### Relink UI +Decision: extend the existing missing-content banner at `character_builder.cljs:1940-1972`, do NOT build new detection. +- The existing `::char5e/missing-content-report` sub already detects unloaded classes/subclasses/races/etc. +- The existing `find-similar-content` at `content_reconciliation.cljs:132` already produces scored candidate suggestions. +- Extend the banner with rebind buttons that consume those existing `:suggestions`. Inline placement (matches existing precedent), not modal. +- New event handler `relink-class-entry` rewrites `::entity/key` on the chosen class entry and re-dispatches `:set-character`. + +### Subclass-mismatch detection +Two flavors discussed: +- (a) Subclass not loaded — already covered by missing-content-report. +- (b) Subclass IS loaded but linked to wrong class on the character — **deferred to follow-up**. Detection requires new logic (cross-reference subclass `:class` field against character's class entries). + +## Scope explicitly excluded from this PR + +- **`?prepare-spell-count` propagation** at `template_base.cljc:275`. Clean fix requires threading `:class-key` through `spell-data`, `spells-known` modifier, and ~100 `mod5e/spells-known` callsites scattered across `classes.cljc`. Latent risk; active behavior is correct under phase 1's revert. Code comment + TODO added. +- **Refactoring how base class option-fns are written** (e.g. into multimethods, registries, or compile-time scanned macros). User explicitly rejected. Stays as imperative source-code functions. +- **Subclass-cross-file linking** (subclass imported in a separate orcbrew from its parent class). Real but narrow blast radius. Punt to `docs/TODO.md`. +- **Generalizing relink to feats / races / subraces / backgrounds.** Same pattern would apply; each needs its own audit. + +## Open architectural concern + +Every built-in content type has a hardcoded "shim shadow" enumeration in `content_reconciliation.cljs` (`builtin-races`, `builtin-subraces`, `builtin-subclasses`, `builtin-backgrounds`, `builtin-feats`) plus the `base-class-keys` in `classes.cljc`. These exist because built-in content is defined as imperative option-fns and plugin content is data; the two don't unify cheaply. + +User is justifiably frustrated by this — the app obviously knows about its content (every dropdown lists them), but there's no clean way to query that knowledge without either calling the heavy option-fns or maintaining manual shadow sets. + +Minimum-invasive fix proposed but not yet committed: an app-init event that runs the existing heavyweight subs once at startup, extracts keys for each content type, caches at db path `[:content/known-keys]`. All `builtin-*` sets read from the cache instead of being hand-maintained lists. Eliminates the shim pattern without refactoring how option-fns are written. + +User asked whether Clojure/cljs offers something better. Surveyed: `ns-publics` (brittle under cljs `:advanced` compilation), macros (workable, add complexity), multimethods (idiomatic but requires changing how option-fns are written — rejected), var metadata (same compilation concern). The app-init cache pattern is the realistic fit. + +**Open question:** fold the architectural cleanup into this PR, or file as a separate high-priority TODO? + +## Reasoning traps from prior sessions (do not repeat) + +- **Conflating `class-name` (a function parameter) with `:name` (a class config field).** They are synonymous *by convention only* — callers populate the parameter from `:name`. The parameter slot itself is arbitrary. +- **"Dead code" claims without thorough verification.** A grep that misses one usage in a related file leads to incorrect deletion plans. Use multiple search patterns (`\bfoo\b`, `foo/`, `qualified.ns/foo`) and check both src/ and test/ before assuming a symbol is unused. +- **Trusting local-checkout state when the branch has remote commits.** This handoff itself was originally written against a stale local clone that didn't have any branch commits visible. Always `git fetch origin ` and `git log ` before claiming anything about the branch state. +- **Inventing reconciler outputs for UIs that don't exist.** The `:parked` accumulator was a hook into vapor. Verify the producer fires in real data before consuming. +- **Proposing UI components without checking for existing precedent.** The missing-content banner already does most of what the relink UI needs. +- **Trusting reviewer claims without re-verifying.** Plugin-load race was claimed real; it isn't. +- **Treating phase 1's revert as a complete solution.** Phase 1 stops the active bleed but defends only by convention. The user explicitly rejected this as a stopping point. Phase 2's architectural switch is mandatory. + +## Where to start + +1. Verify branch state via `git fetch origin claude/fix-cantrips-selection-bug-CSwVv && git log -20`. Reconcile against any state the user mentions; trust the remote over any stale local checkout. +2. Pending decisions to confirm with the user before writing code: + - Should the architectural cleanup (shim-shadow cache) fold into this PR or be a separate follow-up? + - Confirm relink UI inline placement and event-handler shape. + - Confirm stored-abbreviation field stays deferred. +3. The natural next implementation step is phase 3 (`import_validation.cljs` abbreviation logic + `:name` suffixing). Phase 2 is already shipped. + +## Key files + +- `src/cljs/orcpub/dnd/e5/spell_subs.cljs:454-483` — `::classes5e/plugin-classes` sub; historic mutation site, now reverted +- `src/cljc/orcpub/dnd/e5/options.cljc:469` — `spell-selection-key` helper (phase 2's class-key-based derivation) +- `src/cljc/orcpub/dnd/e5/options.cljc:478-488` — `spell-selection` using the helper for `kw` +- `src/cljc/orcpub/dnd/e5/template_base.cljc:275` — `?prepare-spell-count`; deferred site with code comment +- `src/cljs/orcpub/dnd/e5/events.cljs:534-563` — `reg-save-homebrew`; the editor save path (carve-out exception to the design rule) +- `src/cljc/orcpub/dnd/e5/classes.cljc` — `base-class-keys` canonical home (post-dedupe) +- `src/cljs/orcpub/dnd/e5/content_reconciliation.cljs:132` — `find-similar-content`; reuse for relink UI candidate ranking +- `src/cljs/orcpub/dnd/e5/content_reconciliation.cljs:163+` — remaining `builtin-*` shim sets (the architectural-debt site) +- `src/cljs/orcpub/character_builder.cljs:1940-1972` — existing missing-content banner; extend, don't duplicate +- `src/cljs/orcpub/dnd/e5/import_validation.cljs:1388-1394` — `generate-new-key`; phase 3 target +- `src/cljs/orcpub/dnd/e5/spell_subs.cljs:937-983` — `::classes5e/classes` + `::classes5e/class-map` (heavyweight unified sub) +- `src/cljs/orcpub/dnd/e5/subs.cljs:1435-1467` — `::char5e/available-content` + `::char5e/missing-content-report` (existing detection) +- `docs/kb/key-vs-name-separation.md` — durable case study + design rule +- `HANDOFF.md` — older sibling doc with more granular file:line references