Review follow-up M: Grim Harvest fires in turn-based combat

The doc's item M claimed all three mage subclass spell hooks were dead on the
turn path because applyMageSubclassSpellHooks had one caller. It has three.
resolveTurnSpell has called it since 5cd343a, so Empowered Evocation and
Overchannel always worked -- they only move mods.SpellPreDamage, which
resolveTurnSpell returns as EnemyDamage.

Grim Harvest was the real defect, with a narrower cause: the hook wrote
mods.GrimHarvestSlot into a local CombatModifiers that resolveTurnSpell
discarded, because turnSpellOutcome had no field to carry it out. A Necromancy
Mage who killed with a spell in a manual fight never healed.

The stash can't ride on fight-start mods the way auto-resolve's does -- the
spell is cast mid-fight and the turn engine rebuilds combatants every round --
so it rides on the casting seat's ActorStatuses, like ArmedAbility. Each
damaging cast overwrites it; snapshotActor carries it across commit().

grimHarvestHeal also scanned for the *first* spell_cast event to ask whether
the spell landed the killing blow. Auto-resolve casts once, pre-combat, so
first == last there. A turn-based mage casts every round, so a non-lethal
opening cantrip vetoed the heal the killing spell had earned. Now scans for the
last spell_cast -- provably identical on the auto-resolve path, so the golden
corpus does not move.

Balance: a caster buff on the manual surface only, and the one the subclass was
written to have. Auto-resolve already paid it out.

Claude-Session: https://claude.ai/code/session_017mEwUmmS7aQTP2NQXj6rUa

internal/plugin/combat_cmd.go

@@ -602,6 +602,18 @@ func (p *AdventurePlugin) castActionForSeat(ct *combatTurn, seat int, args strin
 		if msg := p.chargeSpellCost(uid, spell, slotLevel); msg != "" {
 			return PlayerAction{}, noop, msg
 		}
+		// Park the Necromancy kill-heal stash on the casting seat. The
+		// auto-resolve path keeps it on the fight-start CombatModifiers, which
+		// a turn-based fight has nowhere to hold — it rebuilds its combatants
+		// every round. Only a damaging cast stashes (a miss leaves the slot at
+		// 0), and each one overwrites the last, so the stash always describes
+		// the seat's most recent landed spell — which is the only one that can
+		// have been lethal by the time the close-out reads it.
+		if out.GrimHarvestSlot > 0 {
+			as := ct.sess.actorStatusesPtr(seat)
+			as.GrimHarvestSlot = out.GrimHarvestSlot
+			as.GrimHarvestNecrotic = out.GrimHarvestNecrotic
+		}
 		eff = &turnActionEffect{
 			Label:       out.Desc,
 			Action:      "spell_cast",

internal/plugin/combat_engine.go

@@ -162,9 +162,11 @@ type CombatModifiers struct {
 	// ArcaneWardHP: flat HP buffer absorbed before player HP. Refilled at the
 	// start of each combat by Abjuration L5+ (2× Mage level, +prof at L7).
 	// Persists across rounds within a single combat; not refunded between fights.
-	// GrimHarvestSlot/Necrotic: snapshot of the queued spell stashed by
-	// applyPendingCast for the post-combat Grim Harvest hook (Necromancy L5+).
-	// Heal fires only if the spell event is what dropped the enemy to 0.
+	// GrimHarvestSlot/Necrotic: snapshot of the damaging spell stashed for the
+	// post-combat Grim Harvest hook (Necromancy L5+) — by applyPendingCast on
+	// the auto-resolve path, and by seatFightStartMods reading the seat's
+	// statuses on the turn-based one. Heal fires only if that spell's event is
+	// what dropped the enemy to 0.
 	ArcaneWardHP        int
 	GrimHarvestSlot     int
 	GrimHarvestNecrotic bool

internal/plugin/combat_grim_harvest_turn_test.go

@@ -0,0 +1,232 @@
+package plugin
+
+import (
+	"testing"
+
+	"maunium.net/go/mautrix/id"
+)
+
+// Grim Harvest on the turn-based surface.
+//
+// The auto-resolve path stashes the killing spell's slot level on the
+// fight-start CombatModifiers, where the close-out reads it. A turn-based fight
+// has nowhere to keep that: it rebuilds its combatants from the session row on
+// every !attack / !cast. resolveTurnSpell computed the stash into a local mod
+// set and dropped it on the floor, so a Necromancy Mage who killed with a spell
+// never healed — on a class that already trails.
+//
+// The stash now rides on the casting seat's ActorStatuses, and the close-out
+// asks whether the *last* spell_cast is the one that dropped the enemy to 0.
+//
+// (Empowered Evocation and Overchannel were never broken here: they only move
+// mods.SpellPreDamage, which resolveTurnSpell already returns as EnemyDamage.)
+
+// necromancer is a Mage who has reached L5 Grim Harvest.
+func necromancer(t *testing.T, uid id.UserID, level int) *DnDCharacter {
+	t.Helper()
+	if err := createAdvCharacter(uid, string(uid)); err != nil {
+		t.Fatal(err)
+	}
+	c := &DnDCharacter{
+		UserID: uid, Race: RaceHuman, Class: ClassMage, Subclass: SubclassNecromancy, Level: level,
+		STR: 8, DEX: 12, CON: 12, INT: 16, WIS: 10, CHA: 10,
+		HPMax: 30, HPCurrent: 10, ArmorClass: 12,
+	}
+	if err := SaveDnDCharacter(c); err != nil {
+		t.Fatal(err)
+	}
+	return c
+}
+
+// tough is a stat block a spell can be cast at without any of it mattering.
+func tough() CombatStats { return CombatStats{MaxHP: 40, AC: 10} }
+
+// ── the leak itself ──────────────────────────────────────────────────────────
+
+// resolveTurnSpell must hand the stash back to its caller. Before this, the
+// slot level lived and died inside the function.
+func TestResolveTurnSpell_CarriesTheGrimHarvestStashOut(t *testing.T) {
+	c := &DnDCharacter{Class: ClassMage, Subclass: SubclassNecromancy, Level: 5, INT: 16}
+
+	// magic_missile: auto-hit, force damage, upcast to a 3rd-level slot.
+	spell, _ := lookupSpell("magic_missile")
+	enemy := tough()
+	out, ok := resolveTurnSpell(c, spell, 3, &enemy)
+	if !ok {
+		t.Fatal("magic_missile should be a supported turn spell")
+	}
+	if out.GrimHarvestSlot != 3 {
+		t.Errorf("GrimHarvestSlot = %d, want 3 (the slot it was cast at)", out.GrimHarvestSlot)
+	}
+	if out.GrimHarvestNecrotic {
+		t.Error("magic_missile deals force damage — Necrotic should be false")
+	}
+
+	// blight: auto-hit, necrotic — the flag that triples the heal.
+	spell, _ = lookupSpell("blight")
+	enemy = tough()
+	out, _ = resolveTurnSpell(c, spell, 4, &enemy)
+	if out.GrimHarvestSlot != 4 || !out.GrimHarvestNecrotic {
+		t.Errorf("blight L4: slot=%d necrotic=%v, want 4/true", out.GrimHarvestSlot, out.GrimHarvestNecrotic)
+	}
+}
+
+// Nobody else stashes. A Mage below L5 has not learned the harvest, and an
+// Evocation Mage never will.
+func TestResolveTurnSpell_NoStashForAnyoneElse(t *testing.T) {
+	spell, _ := lookupSpell("magic_missile")
+	for _, c := range []*DnDCharacter{
+		{Class: ClassMage, Subclass: SubclassNecromancy, Level: 4, INT: 16},
+		{Class: ClassMage, Subclass: SubclassEvocation, Level: 12, INT: 16},
+		{Class: ClassSorcerer, Level: 12, CHA: 16},
+	} {
+		enemy := tough()
+		out, _ := resolveTurnSpell(c, spell, 3, &enemy)
+		if out.GrimHarvestSlot != 0 {
+			t.Errorf("%s/%s L%d stashed slot %d, want 0",
+				c.Class, c.Subclass, c.Level, out.GrimHarvestSlot)
+		}
+	}
+}
+
+// ── which cast counts ────────────────────────────────────────────────────────
+
+// The killing-blow check used to break on the first spell_cast it saw. In a
+// turn-based fight the mage casts every round, so the first one is an opening
+// cantrip that left the enemy standing — and it vetoed the heal the killing
+// spell had earned. It is the last cast that has to be lethal.
+func TestGrimHarvestHeal_ReadsTheLastCastNotTheFirst(t *testing.T) {
+	c := &DnDCharacter{Class: ClassMage, Subclass: SubclassNecromancy, Level: 5, INT: 16}
+	mods := CombatModifiers{GrimHarvestSlot: 2, GrimHarvestNecrotic: true}
+
+	result := CombatResult{PlayerWon: true, Events: []CombatEvent{
+		{Round: 1, Action: "spell_cast", EnemyHP: 22}, // opening cantrip, enemy stands
+		{Round: 2, Action: "enemy_attack", EnemyHP: 22},
+		{Round: 2, Action: "spell_cast", EnemyHP: 0}, // this one killed it
+	}}
+	if got := grimHarvestHeal(c, result, mods); got != 6 {
+		t.Errorf("heal = %d, want 6 (3× a 2nd-level necrotic slot)", got)
+	}
+}
+
+// The mirror: the mage softened it up with a spell, then finished it with a
+// weapon. No spell landed the blow, so no harvest.
+func TestGrimHarvestHeal_WeaponKillAfterASpellDoesNotHarvest(t *testing.T) {
+	c := &DnDCharacter{Class: ClassMage, Subclass: SubclassNecromancy, Level: 5, INT: 16}
+	mods := CombatModifiers{GrimHarvestSlot: 2, GrimHarvestNecrotic: true}
+
+	result := CombatResult{PlayerWon: true, Events: []CombatEvent{
+		{Round: 1, Action: "spell_cast", EnemyHP: 9},
+		{Round: 2, Action: "player_attack", EnemyHP: 0},
+	}}
+	if got := grimHarvestHeal(c, result, mods); got != 0 {
+		t.Errorf("heal = %d, want 0 — the sword landed the blow, not the spell", got)
+	}
+}
+
+// ── the seam through the session ─────────────────────────────────────────────
+
+// The stash has to survive the round-trip the fight puts it through: parked on
+// the seat by the cast, carried across commit()'s snapshot, read back by the
+// close-out.
+func TestSeatFightStartMods_ReadsTheStashOffTheSeat(t *testing.T) {
+	setupEmptyTestDB(t)
+	uid := id.UserID("@seatstash:example.org")
+	c := necromancer(t, uid, 5)
+
+	sess := &CombatSession{UserID: string(uid), Status: CombatStatusWon}
+	sess.Statuses.GrimHarvestSlot = 3
+	sess.Statuses.GrimHarvestNecrotic = true
+
+	mods := seatFightStartMods(sess, 0, c)
+	if mods.GrimHarvestSlot != 3 || !mods.GrimHarvestNecrotic {
+		t.Fatalf("seatFightStartMods lost the stash: %+v", mods)
+	}
+
+	// snapshotActor rebuilds ActorStatuses from combatState each commit; fields
+	// with no combatState counterpart must be carried over from the prior
+	// snapshot, the way ArmedAbility is.
+	kept := snapshotActor(&actor{}, sess.Statuses.ActorStatuses)
+	if kept.GrimHarvestSlot != 3 || !kept.GrimHarvestNecrotic {
+		t.Errorf("commit() dropped the stash: %+v", kept)
+	}
+}
+
+// End to end at the close-out: a necromancer who ended the fight at 10/30 HP
+// with a lethal 2nd-level necrotic spell walks away with 6 HP back.
+func TestPostCombatBookkeepingForSeat_GrimHarvestHealsOnASpellKill(t *testing.T) {
+	setupEmptyTestDB(t)
+	uid := id.UserID("@harvest:example.org")
+	necromancer(t, uid, 5)
+
+	sess := &CombatSession{
+		UserID: string(uid), Status: CombatStatusWon, Round: 3,
+		PlayerHP: 10, PlayerHPMax: 30, EnemyHP: 0,
+		TurnLog: []CombatEvent{
+			{Round: 1, Action: "spell_cast", EnemyHP: 14},
+			{Round: 3, Action: "spell_cast", EnemyHP: 0},
+		},
+	}
+	sess.Statuses.GrimHarvestSlot = 2
+	sess.Statuses.GrimHarvestNecrotic = true
+
+	(&AdventurePlugin{}).postCombatBookkeepingForSeat(sess, 0)
+
+	got, _ := LoadDnDCharacter(uid)
+	if got.HPCurrent != 16 {
+		t.Errorf("HPCurrent = %d after a turn-based spell kill, want 16 (10 + 3×2)", got.HPCurrent)
+	}
+}
+
+// A seat with no stash — the mage never cast, or missed every time — is left
+// exactly as the fight left them.
+func TestPostCombatBookkeepingForSeat_NoStashNoHeal(t *testing.T) {
+	setupEmptyTestDB(t)
+	uid := id.UserID("@nostash:example.org")
+	necromancer(t, uid, 5)
+
+	sess := &CombatSession{
+		UserID: string(uid), Status: CombatStatusWon, Round: 2,
+		PlayerHP: 10, PlayerHPMax: 30, EnemyHP: 0,
+		TurnLog: []CombatEvent{{Round: 2, Action: "player_attack", EnemyHP: 0}},
+	}
+	(&AdventurePlugin{}).postCombatBookkeepingForSeat(sess, 0)
+
+	got, _ := LoadDnDCharacter(uid)
+	if got.HPCurrent != 10 {
+		t.Errorf("HPCurrent = %d, want 10 — nothing was stashed", got.HPCurrent)
+	}
+}
+
+// Party fights: the stash is per-seat, so seat 1's harvest cannot heal seat 0.
+// This is the same class of bug P5 fixed for mid-fight buffs.
+func TestGrimHarvestStash_IsPerSeat(t *testing.T) {
+	setupEmptyTestDB(t)
+	leader, member := id.UserID("@leader:example.org"), id.UserID("@member:example.org")
+	fightTestChar(t, leader, 30)
+	necromancer(t, member, 5)
+
+	sess := &CombatSession{
+		UserID: string(leader), Status: CombatStatusWon, Round: 2,
+		PlayerHP: 20, PlayerHPMax: 30, EnemyHP: 0,
+		Participants: []CombatParticipant{{Seat: 1, UserID: string(member), HP: 10, HPMax: 30}},
+		TurnLog: []CombatEvent{
+			{Round: 2, Seat: 1, Action: "spell_cast", EnemyHP: 0},
+		},
+	}
+	sess.Participants[0].Statuses.GrimHarvestSlot = 2
+	sess.Participants[0].Statuses.GrimHarvestNecrotic = true
+
+	p := &AdventurePlugin{}
+	p.postCombatBookkeepingForSeat(sess, 0)
+	p.postCombatBookkeepingForSeat(sess, 1)
+
+	gotLeader, _ := LoadDnDCharacter(leader)
+	if gotLeader.HPCurrent != 30 {
+		t.Errorf("leader HPCurrent = %d, want 30 — the member's harvest is not theirs", gotLeader.HPCurrent)
+	}
+	gotMember, _ := LoadDnDCharacter(member)
+	if gotMember.HPCurrent != 16 {
+		t.Errorf("member HPCurrent = %d, want 16 (10 + 3×2)", gotMember.HPCurrent)
+	}
+}

internal/plugin/combat_session.go

@@ -95,6 +95,17 @@ type ActorStatuses struct {
 	// close-out know a rage fired. Empty when nothing was armed.
 	ArmedAbility string `json:"armed_ability,omitempty"`
 
+	// GrimHarvestSlot / GrimHarvestNecrotic snapshot the most recent damaging
+	// spell this seat cast, for the Necromancy Mage's post-combat kill-heal.
+	// The auto-resolve path carries the same pair on CombatModifiers, stashed
+	// once by applyPendingCast; the turn-based path can cast every round, so
+	// the stash lives here and each damaging cast overwrites it. Whether the
+	// heal actually fires is decided at close-out by grimHarvestHeal, which
+	// asks whether the *last* spell_cast event is the one that dropped the
+	// enemy to 0 — so a stale stash from an earlier, non-lethal cast is inert.
+	GrimHarvestSlot     int  `json:"grim_harvest_slot,omitempty"`
+	GrimHarvestNecrotic bool `json:"grim_harvest_necrotic,omitempty"`
+
 	// Once-per-fight class/race/subclass one-shots: the "already used" flags.
 	// Without persistence these reset every round on resume, letting a Halfling
 	// reroll a nat 1 or an Orc rage every single round of a turn-based fight.

internal/plugin/combat_session_build.go

@@ -181,22 +181,25 @@ func (p *AdventurePlugin) partyCombatantsForSession(sess *CombatSession) ([]*Com
 	return players, &enemy, nil
 }
 
-// seatFightStartMods re-derives the fight-start modifiers a finished fight's
-// close-out still needs — today only the Berserker's rage flag, which decides
-// whether the character owes a point of exhaustion.
+// seatFightStartMods re-derives the modifiers a finished fight's close-out still
+// needs: the Berserker's rage flag, which decides whether the character owes a
+// point of exhaustion, and the Necromancy Mage's Grim Harvest stash.
 //
 // It re-applies the seat's armed ability to an empty mod set rather than
 // rebuilding the whole combatant: no Apply writes to the character (they read
 // level and HP and write only mods), so this is pure, and the passive/equipment
 // layers a full build would add are not read by any post-combat hook.
 //
-// GrimHarvestSlot stays zero here, and that is not an oversight: the turn-based
-// path never runs applyPendingCast, so a Necromancy Mage's spell is never
-// stashed and Grim Harvest cannot fire on this surface at all. Wiring the mage
-// spell hooks into the turn-based `!cast` is a separate change.
+// The Grim Harvest pair is not re-derived but read back off the seat's
+// statuses, where castActionForSeat parked it: the spell that stashed it was
+// cast mid-fight, not at fight start, and nothing in the character sheet still
+// remembers which one it was.
 func seatFightStartMods(sess *CombatSession, seat int, c *DnDCharacter) CombatModifiers {
 	var mods CombatModifiers
-	applyAbilityByID(c, sess.actorStatusesForSeat(seat).ArmedAbility, &mods)
+	st := sess.actorStatusesForSeat(seat)
+	applyAbilityByID(c, st.ArmedAbility, &mods)
+	mods.GrimHarvestSlot = st.GrimHarvestSlot
+	mods.GrimHarvestNecrotic = st.GrimHarvestNecrotic
 	return mods
 }
 

internal/plugin/dnd_spell_combat.go

@@ -310,6 +310,13 @@ type turnSpellOutcome struct {
 	PlayerHeal  int
 	EnemySkip   bool
 	Desc        string
+
+	// GrimHarvestSlot / GrimHarvestNecrotic are the Necromancy Mage's kill-heal
+	// stash for this cast, zero for everyone else. Unlike the fields above they
+	// outlive the casting round: the caller parks them on the seat's
+	// ActorStatuses and the close-out decides whether the cast was lethal.
+	GrimHarvestSlot     int
+	GrimHarvestNecrotic bool
 }
 
 // resolveTurnSpell resolves a spell as a turn-based player action, reusing the
@@ -344,6 +351,8 @@ func resolveTurnSpell(c *DnDCharacter, spell SpellDefinition, slotLevel int, ene
 		out.EnemyDamage = mods.SpellPreDamage
 		out.EnemySkip = mods.SpellEnemySkipFirst
 		out.Desc = mods.SpellPreDamageDesc
+		out.GrimHarvestSlot = mods.GrimHarvestSlot
+		out.GrimHarvestNecrotic = mods.GrimHarvestNecrotic
 		if out.Desc == "" {
 			out.Desc = spell.Name
 		}

internal/plugin/dnd_subclass_combat.go

@@ -934,11 +934,22 @@ func grimHarvestHeal(c *DnDCharacter, result CombatResult, mods CombatModifiers)
 	if !result.PlayerWon || mods.GrimHarvestSlot <= 0 {
 		return 0
 	}
-	// The pre-combat spell killed the enemy iff the spell_cast event itself
-	// dropped EnemyHP to 0. If a later round-event finished the kill, no heal.
-	for _, ev := range result.Events {
-		if ev.Action == "spell_cast" {
-			if ev.EnemyHP > 0 {
+	// The spell killed the enemy iff the spell_cast event itself dropped EnemyHP
+	// to 0. If a later round-event (a weapon swing, the pet, a concentration
+	// aura re-tick) finished the kill, no heal.
+	//
+	// It is the *last* spell_cast that has to be lethal, not the first. The
+	// auto-resolve path casts once, pre-combat, so the two coincide there; the
+	// turn-based path can cast every round, and there the stash on the seat's
+	// statuses describes that last cast. Reading the first event would let a
+	// non-lethal opening cantrip veto a heal the killing spell had earned.
+	//
+	// No spell_cast event at all cannot happen while the stash is set — both
+	// surfaces emit one for every damaging cast — so the loop falling through
+	// is not a case, just an absence.
+	for i := len(result.Events) - 1; i >= 0; i-- {
+		if result.Events[i].Action == "spell_cast" {
+			if result.Events[i].EnemyHP > 0 {
 				return 0
 			}
 			break