diff --git a/internal/plugin/dnd_cast.go b/internal/plugin/dnd_cast.go index 560a479..aae3ddd 100644 --- a/internal/plugin/dnd_cast.go +++ b/internal/plugin/dnd_cast.go @@ -80,8 +80,7 @@ func (p *AdventurePlugin) handleDnDCastCmd(ctx MessageContext, args string) erro } if !isSpellcaster(c) { return p.SendDM(ctx.Sender, fmt.Sprintf( - "%s isn't a caster class. `!cast` is for Mage, Cleric, Ranger, and Arcane Trickster Rogues (L5+).", - titleClass(c.Class))) + "%s doesn't cast spells.", titleClass(c.Class))) } if args == "" { @@ -363,8 +362,10 @@ func (p *AdventurePlugin) dndCastDrop(ctx MessageContext, c *DnDCharacter) error // Refund the queued slot (if any) — voluntary drop is the player's // choice; restore the slot rather than forfeit it. + slotRefunded := false if pc, ok := decodePendingCast(c.PendingCast); ok && pc.SlotLevel > 0 { _ = refundSpellSlot(ctx.Sender, pc.SlotLevel) + slotRefunded = true } dropped := []string{} @@ -381,7 +382,11 @@ func (p *AdventurePlugin) dndCastDrop(ctx MessageContext, c *DnDCharacter) error if err := SaveDnDCharacter(c); err != nil { return p.SendDM(ctx.Sender, "Couldn't drop: "+err.Error()) } - return p.SendDM(ctx.Sender, "Dropped: "+strings.Join(dropped, ", ")+".") + msg := "Dropped: " + strings.Join(dropped, ", ") + "." + if slotRefunded { + msg += " _(slot refunded)_" + } + return p.SendDM(ctx.Sender, msg) } // ── !spells command ───────────────────────────────────────────────────────── @@ -487,7 +492,8 @@ func renderSpellsList(c *DnDCharacter) string { func (p *AdventurePlugin) handleSpellsLearn(ctx MessageContext, c *DnDCharacter, raw string) error { if c.Class != ClassMage { - return p.SendDM(ctx.Sender, "`!spells learn` is for Mage. Cleric uses `!prepare`; Ranger spells are auto-known.") + return p.SendDM(ctx.Sender, "`!spells learn` is for Mage. "+spellRouteHintFor(c)+ + " Run `!spells` to see what you already know.") } if raw == "" { return p.SendDM(ctx.Sender, "Usage: `!spells learn `") @@ -561,7 +567,8 @@ func (p *AdventurePlugin) handleDnDPrepareCmd(ctx MessageContext, args string) e return p.SendDM(ctx.Sender, "Couldn't load your Adv 2.0 sheet.") } if c.Class != ClassCleric { - return p.SendDM(ctx.Sender, "`!prepare` is for Cleric. Mage uses `!spells learn`; Ranger spells are auto-known.") + return p.SendDM(ctx.Sender, "`!prepare` is for Cleric. "+spellRouteHintFor(c)+ + " Run `!spells` to see what you already know.") } args = strings.TrimSpace(args) if args == "" { @@ -611,7 +618,8 @@ func (p *AdventurePlugin) handleDnDPrepareCmd(ctx MessageContext, args string) e } if prepCount+1 > cap { return p.SendDM(ctx.Sender, fmt.Sprintf( - "Prep cap is %d. Unprepare a spell first: `!prepare clear `.", cap)) + "Your prepared list is full (%d/%d). Unprepare one first: `!prepare clear `.", + prepCount, cap)) } if err := setSpellPrepared(ctx.Sender, spell.ID, true); err != nil { return p.SendDM(ctx.Sender, "Couldn't prepare: "+err.Error()) @@ -640,6 +648,30 @@ func renderClericPrepStatus(c *DnDCharacter) string { prepCount, cap) } +// spellRouteHintFor returns a one-sentence pointer telling the player which +// command actually advances their spellbook, scoped to their class. Used by +// the !spells learn / !prepare wrong-class error messages so the player isn't +// left guessing after we reject them. +func spellRouteHintFor(c *DnDCharacter) string { + if c == nil { + return "" + } + switch c.Class { + case ClassMage: + return "Mage learns spells with `!spells learn `." + case ClassCleric: + return "Cleric picks today's spells with `!prepare `." + case ClassRogue: + if c.Subclass == SubclassArcaneTrickster { + return "Arcane Trickster learns spells with `!spells learn ` once it unlocks." + } + return "" + case ClassDruid, ClassBard, ClassSorcerer, ClassWarlock, ClassPaladin, ClassRanger: + return "Your class learns spells automatically as you level — nothing to prepare." + } + return "" +} + // ── helpers ────────────────────────────────────────────────────────────────── func renderCastHelp(c *DnDCharacter) string { diff --git a/internal/plugin/dnd_prepare_test.go b/internal/plugin/dnd_prepare_test.go index 024356e..f3ab2ba 100644 --- a/internal/plugin/dnd_prepare_test.go +++ b/internal/plugin/dnd_prepare_test.go @@ -72,7 +72,7 @@ func TestPrepareCap(t *testing.T) { // Add 7 known leveled spells, all unprepared. leveled := []string{ - "cure_wounds", "healing_word_spell", "bless", "guiding_bolt", + "cure_wounds", "healing_word", "bless", "guiding_bolt", "shield_of_faith", "spiritual_weapon", "aid", } for _, sid := range leveled { diff --git a/internal/plugin/dnd_spells.go b/internal/plugin/dnd_spells.go index a569d77..356f3e1 100644 --- a/internal/plugin/dnd_spells.go +++ b/internal/plugin/dnd_spells.go @@ -86,18 +86,49 @@ type SpellDefinition struct { // dndSpellRegistry merges the vendored Open5e SRD dump with the hand-authored // spell list. SRD loads first as the broad baseline; buildSpellList() overlays // it and wins on any ID collision — the hand-authored entries carry the tuned -// Effect/DamageDice/Upcast values, the SRD entry is just the fallback shape. +// Effect/DamageDice/Upcast values, the SRD entry is the fallback shape. +// +// One special-case on collision: the Classes slice is *unioned* across both +// sources rather than replaced. The hand-authored tables were written before +// the Open5e caster scaffolds (Druid/Bard/Sorcerer/Warlock/Paladin) landed, +// so most entries are tagged Mage-only; the SRD knows the broader 5e class +// list. Unioning means a Sorcerer can actually cast a `magic_missile` they +// were auto-granted, without us having to hand-fix every overlay entry. var dndSpellRegistry = func() map[string]SpellDefinition { out := make(map[string]SpellDefinition, 320) for _, s := range buildSRDSpellList() { out[s.ID] = s } for _, s := range buildSpellList() { + if prev, ok := out[s.ID]; ok { + s.Classes = mergeClassList(prev.Classes, s.Classes) + } out[s.ID] = s } return out }() +// mergeClassList returns the union of two class slices, preserving the order +// of the first (SRD baseline) and appending any classes only in the second. +// Order-stability matters for any callers that iterate Classes deterministically. +func mergeClassList(a, b []DnDClass) []DnDClass { + seen := make(map[DnDClass]bool, len(a)+len(b)) + out := make([]DnDClass, 0, len(a)+len(b)) + for _, c := range a { + if !seen[c] { + seen[c] = true + out = append(out, c) + } + } + for _, c := range b { + if !seen[c] { + seen[c] = true + out = append(out, c) + } + } + return out +} + func lookupSpell(id string) (SpellDefinition, bool) { s, ok := dndSpellRegistry[strings.ToLower(strings.TrimSpace(id))] return s, ok @@ -698,13 +729,17 @@ func defaultKnownSpells(class DnDClass, level int) []string { case ClassMage: out := []string{"fire_bolt", "minor_illusion", "mending"} if level >= 1 { - out = append(out, "magic_missile", "mage_armor", "shield", "burning_hands", "detect_magic") + // No `shield` here — it's a reaction (EffectReaction) and combat + // has no reaction window yet, so an auto-granted shield is just + // a dead entry in the player's spellbook. Reinstated when SP-Reactions ships. + out = append(out, "magic_missile", "mage_armor", "burning_hands", "detect_magic", "sleep") } if maxSlot >= 2 { out = append(out, "scorching_ray", "misty_step", "mirror_image") } if maxSlot >= 3 { - out = append(out, "fireball", "counterspell") + // `counterspell` deferred for the same reason as `shield`. + out = append(out, "fireball", "fly") } if maxSlot >= 4 { out = append(out, "ice_storm", "greater_invisibility") @@ -716,7 +751,7 @@ func defaultKnownSpells(class DnDClass, level int) []string { case ClassCleric: out := []string{"sacred_flame", "guidance", "mending"} if level >= 1 { - out = append(out, "cure_wounds", "healing_word_spell", "bless", "guiding_bolt", "shield_of_faith") + out = append(out, "cure_wounds", "healing_word", "bless", "guiding_bolt", "shield_of_faith") } if maxSlot >= 2 { out = append(out, "spiritual_weapon", "lesser_restoration", "aid") @@ -752,7 +787,10 @@ func defaultKnownSpells(class DnDClass, level int) []string { // gives the class a damage option, control, and a heal/buff where the // class fantasy expects one. case ClassDruid: - out := []string{"produce_flame", "shillelagh", "guidance", "mending"} + // `shillelagh` deferred — the overlay flags it ranger/cleric only and + // it has no real attack profile in the engine yet (BuffSelf, no damage + // dice). Reinstated when weapon-bonus casts wire up. + out := []string{"produce_flame", "guidance", "mending"} if level >= 1 { out = append(out, "cure_wounds", "healing_word", "faerie_fire", "thunderwave", "entangle") } @@ -790,13 +828,15 @@ func defaultKnownSpells(class DnDClass, level int) []string { case ClassSorcerer: out := []string{"fire_bolt", "ray_of_frost", "shocking_grasp", "mending"} if level >= 1 { - out = append(out, "magic_missile", "burning_hands", "mage_armor", "shield", "thunderwave") + // `shield` skipped — reaction spell, no usable window in current combat. + out = append(out, "magic_missile", "burning_hands", "mage_armor", "thunderwave", "sleep") } if maxSlot >= 2 { out = append(out, "scorching_ray", "mirror_image", "misty_step", "hold_person") } if maxSlot >= 3 { - out = append(out, "fireball", "counterspell", "lightning_bolt", "haste") + // `counterspell` skipped — reaction. + out = append(out, "fireball", "lightning_bolt", "haste", "fly") } if maxSlot >= 4 { out = append(out, "ice_storm", "greater_invisibility") @@ -808,13 +848,17 @@ func defaultKnownSpells(class DnDClass, level int) []string { case ClassWarlock: out := []string{"eldritch_blast", "chill_touch", "minor_illusion"} if level >= 1 { - out = append(out, "hellish_rebuke", "command", "charm_person", "hideous_laughter") + // `hellish_rebuke` skipped — reaction spell, no usable window + // in current combat. Substituting `burning_hands` as the L1 + // damage staple (SRD: Mage/Cleric/Sorcerer/Warlock). + out = append(out, "burning_hands", "command", "charm_person", "hideous_laughter") } if maxSlot >= 2 { out = append(out, "scorching_ray", "misty_step", "hold_person", "invisibility") } if maxSlot >= 3 { - out = append(out, "counterspell", "fly", "vampiric_touch", "hypnotic_pattern") + // `counterspell` skipped — reaction. + out = append(out, "fly", "vampiric_touch", "hypnotic_pattern") } if maxSlot >= 4 { out = append(out, "banishment", "greater_invisibility") diff --git a/internal/plugin/dnd_spells_data.go b/internal/plugin/dnd_spells_data.go index d75a7f4..b5565cd 100644 --- a/internal/plugin/dnd_spells_data.go +++ b/internal/plugin/dnd_spells_data.go @@ -132,11 +132,10 @@ func buildSpellList() []SpellDefinition { Classes: cleric, Effect: EffectBuffAlly, CastTime: CastBonusAction, Concentration: true, Description: "+2 AC to one target. 10 min."}, - {ID: "healing_word_spell", Name: "Healing Word", Level: 1, School: "evocation", - Classes: cleric, Effect: EffectSpellHeal, CastTime: CastBonusAction, - DamageDice: "1d4", - Description: "1d4 + WIS mod HP. Range 60 ft.", - Upcast: "+1d4 per slot above 1st"}, + // `healing_word_spell` was a hand-authored alias for `healing_word` + // that collided on display name with the SRD entry. Removed to make + // parseSpell("healing word") deterministic; Cleric default now points + // at the canonical SRD `healing_word` (cleric/druid/bard tagged). {ID: "command", Name: "Command", Level: 1, School: "enchantment", Classes: cleric, Effect: EffectControl, CastTime: CastAction, SaveStat: "WIS", diff --git a/internal/plugin/dnd_spells_defaults_s2_test.go b/internal/plugin/dnd_spells_defaults_s2_test.go new file mode 100644 index 0000000..d8b6230 --- /dev/null +++ b/internal/plugin/dnd_spells_defaults_s2_test.go @@ -0,0 +1,111 @@ +package plugin + +// UX session S2 acceptance tests. Cover: +// - B2: no EffectReaction in any class's default known-spell list (and +// every default is actually castable by that class — the overlay-narrow +// bug used to make Sorcerer/Warlock/Bard/Druid lists silently broken). +// - R15: parseSpell("healing word") resolves deterministically to the +// SRD `healing_word`, and the legacy `healing_word_spell` alias is gone. +// - B3: !cast non-caster error contains no class enumerations. + +import ( + "fmt" + "strings" + "testing" +) + +func TestDefaultKnownSpellsHaveNoReactions(t *testing.T) { + classes := []DnDClass{ + ClassMage, ClassCleric, ClassRanger, + ClassDruid, ClassBard, ClassSorcerer, ClassWarlock, ClassPaladin, + } + for _, class := range classes { + for _, lvl := range []int{1, 2, 3, 5, 9, 13, 20} { + for _, sid := range defaultKnownSpells(class, lvl) { + s, ok := lookupSpell(sid) + if !ok { + t.Errorf("default for %s L%d: unknown spell %q", class, lvl, sid) + continue + } + if s.Effect == EffectReaction { + t.Errorf("default for %s L%d includes reaction spell %q — combat has no reaction window yet", + class, lvl, sid) + } + } + } + } +} + +func TestDefaultKnownSpellsAreCastableByClass(t *testing.T) { + // Every default-granted spell must list the granting class in its + // Classes slice; otherwise the !cast classOK gate rejects a spell the + // player was auto-granted. This used to fail silently because the + // hand-authored overlay was narrowing class lists down to [Mage]. + classes := []DnDClass{ + ClassMage, ClassCleric, ClassRanger, + ClassDruid, ClassBard, ClassSorcerer, ClassWarlock, ClassPaladin, + } + for _, class := range classes { + for _, lvl := range []int{1, 3, 5, 9, 13, 20} { + for _, sid := range defaultKnownSpells(class, lvl) { + s, ok := lookupSpell(sid) + if !ok { + continue // covered by the existence test + } + ok = false + for _, c := range s.Classes { + if c == class { + ok = true + break + } + } + if !ok { + t.Errorf("%s default L%d grants %q but Classes=%v doesn't include %s", + class, lvl, sid, s.Classes, class) + } + } + } + } +} + +func TestHealingWordResolvesDeterministically(t *testing.T) { + for _, in := range []string{"healing word", "Healing Word", "HEALING WORD", "healing-word", "healing_word"} { + s, ok := parseSpell(in) + if !ok { + t.Fatalf("parseSpell(%q): not found", in) + } + if s.ID != "healing_word" { + t.Errorf("parseSpell(%q): id=%q, want healing_word (legacy alias should be gone)", + in, s.ID) + } + } + if _, ok := lookupSpell("healing_word_spell"); ok { + t.Errorf("healing_word_spell alias should have been removed; still in registry") + } +} + +func TestCastNonCasterErrorHasNoClassEnumeration(t *testing.T) { + // B3: the non-caster error message used to enumerate "Mage, Cleric, + // Ranger, and Arcane Trickster Rogues (L5+)" — stale list that now + // omits Druid/Bard/Sorcerer/Warlock/Paladin. The replacement should + // not mention specific class names. + // + // We exercise the prefix the !cast handler emits via the public + // titleClass()/isSpellcaster() shape. Direct invocation of the handler + // would need a full plugin/db harness; we settle for asserting on the + // composed prefix that the production line uses. + for _, cls := range []DnDClass{ClassFighter, ClassRogue} { + msg := fmt.Sprintf("%s doesn't cast spells.", titleClass(cls)) + lower := strings.ToLower(msg) + for _, banned := range []string{"mage", "cleric", "druid", "bard", "sorcerer", "warlock", "paladin", "ranger", "trickster"} { + if strings.Contains(lower, banned) { + // titleClass() may legitimately put the *speaker's* class in; + // the failure case is enumerating *other* classes. + if !strings.EqualFold(string(cls), banned) { + t.Errorf("non-caster error mentions class %q for speaker %s: %q", + banned, cls, msg) + } + } + } + } +}