gogobee/gogobee_code_review_followups.md

Code review follow-ups — n1-restoration (2026-07-10)

Review scope: git diff main...HEAD, 99 files / ~16.7k lines (N1, N2, R1, R2, N3 P1–P7). Method: 8 finder angles → dedup → 1 verifier per correctness candidate. Baseline before and after: go build, go vet, go test ./internal/... all green.

Working-tree doc — do not commit (per project convention on gogobee_*.md).

Status. Fixes 1–5 shipped in 1f21156. Deferred items A and B fixed 2026-07-10 (d76c63b); A turned out to have a deeper root cause and surfaced three new findings, M, N and O. Item C fixed 2026-07-10 — its premise was wrong and the bug underneath it was worse (the seven-day resume window was never enforced; see below). Item M fixed 2026-07-10 — two of its three claims were wrong (the mage hooks do run on the turn path); the one real defect, Grim Harvest, had a different cause than recorded. Items D, I, L, N fixed. Item K verified 2026-07-10 — the anchor gap is a real, confirmed narrowing, left as an owner design call (no code change). Item H measured 2026-07-10 and declined — its "per-message cascade" premise was overstated, the hot path is already optimized, and the proposed MessageContext thread-through is disproportionate churn with a staleness regression surface.

All correctness work is committed. Everything remaining is deliberate, not a defect: E (intentional, roster-size death rule), F (grantAutorunGrace stopgap, deferred to R5 by design), G (party-combat DB chatter — a perf refactor whose only safe fix is a fight-lifetime roster cache, i.e. the same staleness surface H was declined for; deferred), H (declined, above), J (!sell anchor — PLAUSIBLE, presence == any chat, left per convention), K (owner design call, above), O (already fixed as a side-effect of A; only the sim re-baseline is pending, which is operational).

Method note. Both C and M were mis-diagnosed in the same way: the finder read one call site, concluded "this is the only caller", and wrote it down. Verify the callers before planning the fix.

Third pass (2026-07-10, 88c5fcd). /code-review high --fix over the pushed follow-up stack (git diff origin/main...HEAD, 7 commits). Five fixes shipped; they harden the item-C extraction work and the item-N Misty ordering rather than opening new ground. See "Third pass" below.

---

Before you commit: split the formatting from the fixes

A repo-wide gofmt -w ./internal ./cmd ran after the review. It reformatted 105 files that have nothing to do with these findings — they were already non-conforming on main (struct-field alignment, mostly), and gofmt is semantics-preserving, so nothing changed behaviourally.

Net state: 117 files dirty, only 13 carry review fixes. Commit them apart, or the fixes become unreviewable inside a wall of realignment.

The 13 fix-bearing files:

internal/plugin/adventure.go               *  internal/plugin/dnd_expedition_supplies.go  *
internal/plugin/adventure_arena_season.go     internal/plugin/dnd_zone_cmd.go             *
internal/plugin/combat_cmd.go                 internal/plugin/expedition_party.go
internal/plugin/combat_party_turn.go          internal/plugin/expedition_party_cmd.go
internal/plugin/dnd_expedition.go          *  internal/plugin/expedition_party_resolve.go
internal/plugin/dnd_expedition_combat.go      internal/plugin/zone_combat_party.go
internal/plugin/combat_engine_party_test.go

Plus one new file: internal/plugin/zone_combat_party_casualty_test.go.

The four marked * carry both a fix and gofmt realignment (adventure.go, dnd_expedition.go, dnd_expedition_supplies.go, dnd_zone_cmd.go), so a clean split needs git add -p on those rather than a straight per-file split. The other nine are fix-only and can be staged whole.

---

Fixed in this pass

#	File	Defect
1	combat_party_turn.go	Settle-to-terminal dropped the entire close-out
2	expedition_party_cmd.go	Party member permanently soft-locked on leader extract
3	expedition_party_cmd.go	Lost update on the shared supply pool
4	dnd_expedition_combat.go	Elite harvest interrupt granted no elite loot
5	adventure_arena_season.go	Transient failure permanently lost a season crown

Plus cleanups: deleted dead partySurvivors; collapsed the zoneCombatRoster alias into fightRoster; partyCasualtyLine now uses joinNames; consolidated four copies of the expedition column projection into expeditionSelectCols; corrected two false doc comments on applyDailyBurn/applyDailyBurnP; replyDM no longer sends a blank DM.

1. Close-out dropped when the settle turns lethal — the worst one

beginCombatTurn calls settleCombatSession to resolve a phase the engine owes (a fight parked mid enemy-turn after a restart). That settle can end the fight. The old code then saw !sess.IsActive() and returned "you're not in a fight."

The terminal status was already persisted, so nothing ever paid the party out: no XP, no loot, no markAdventureDead, no abandonZoneRun, no forceExtractExpeditionForRunLoss. The reaper cannot rescue it either — listExpiredCombatSessions filters status = 'active', and the session is now terminal. The win or the death simply evaporates.

The !fight start path and the reaper both already do settle-then-closePartyRound; beginCombatTurn was the one settle site that skipped it. Now it closes out and announces, matching them.

2. Member soft-locked when the leader extracts and walks away

seatedExpeditionFor (the guard) spans status IN ('active','extracting'). expeditionForMember (what !expedition leave resolved through) filtered status = 'active' only. During the leader's 7-day extracting limbo a member was therefore refused any new adventure by the guard, and told "No active expedition" by the very command the guard pointed them at. Nothing sweeps stale extracting rows, and only the leader can trigger the transition that clears them — so if the leader quit, the member was stuck forever with no self-service recovery.

!expedition leave now resolves the seat through seatedExpeditionFor, so the exit sees every state the gate sees. (seatedExpeditionFor already excludes leaders, so they still fall through to the !extract message.)

3. Lost update on the shared supply pool

updateSupplies rewrites supplies_json wholesale. expeditionCmdAccept folded a new member's packs onto an exp snapshot read ~60 lines earlier, with no lock. Handlers run one goroutine per event (mautrix AsyncHandlers, never overridden), so two invitees accepting at once genuinely interleave: both read pool P, one writes P+a, the other P+b — a member's packs vanish, or spent SU is resurrected by the day-burn tick.

Note advUserLock cannot fix this: it is keyed by sender, so two members take two different mutexes. Added advExpeditionLock(expID) and re-read the pool under it. This closes accept-vs-accept only — see deferred item B.

4. Elite harvest interrupt paid standard loot

runHarvestInterrupt computed elite := kind == InterruptElite, used it to pick an elite enemy and to pick elite narration, then passed a hardcoded false as isElite to closeOutZoneWin. Since dropZoneLoot gates on isBoss || isElite, beating an elite interrupt skipped the masterwork roll and used the 1.0 standard treasure weight instead of 2.0 — while the identical elite fought via !zone paid out correctly.

Now passes the live elite. The param only flows into dropZoneLoot, so this adds the masterwork roll and doubles treasure weight for surviving seats; threat, XP, and narration are untouched. A small reward buff on a fight players already win, consistent with lifting trailers rather than nerfing.

5. A failed season crown was lost permanently, not retried

arenaSeasonRollover logged-and-continued when recordArenaSeasonTitle failed, then called db.MarkJobCompleted unconditionally. JobCompleted short-circuits every future run for that quarter, so a transient SQLite BUSY meant the crown was never awarded — ever.

recordArenaSeasonTitle is idempotent (arena_season_titles has PRIMARY KEY (season, kind), insert is ON CONFLICT DO NOTHING) and a past season's data is frozen, so retrying is safe. The job is now marked complete only when no crown failed. The "no entrants" path still closes the season correctly.

---

Fixed in the second pass (2026-07-10) — items A and B

A. Turn-based combat skipped achievements and post-combat subclass state

The root cause was one level deeper than this doc originally recorded, and it is worth writing down because the surface symptom was misleading.

buildZoneCombatants used to call applyArmedAbility, which applies an ability's mods and then clears c.ArmedAbility and saves the sheet. The turn engine calls that same builder again on every !attack / !cast / !consume. So in a turn-based fight:

• Round 1's build fired the ability, spent the resource, cleared the arm flag.
• Round 2+ rebuilt the character, found ArmedAbility == "", and produced a combatant with none of the ability's mods.

A Berserker paid stamina and got exactly one round of BerserkerRage, RageMeleeDmg, PhysicalResistRage, FrenzyDmgBonus. Every entry in dndActiveAbilities had the same shape. mods.BerserkerRage was not merely unread at close-out — by then it no longer existed.

Fixed by splitting arming into its two halves:

• consumeArmedAbility(c) — the mutation. Disarms, saves, returns the id. Runs once, at fight start.
• applyAbilityByID(c, id, mods) — pure. No DB write, no disarm. Safe on every rebuild. (No ability's Apply writes to the character, so this is genuinely pure.)
• armAbilityForFight(c, mods) — consume + apply, for the auto-resolve callers that build and fight in one breath.

buildZoneCombatants now takes the already-consumed armed id and re-applies it. The id rides on ActorStatuses.ArmedAbility, seeded per seat at fight start (seedActorOneShots), so partyCombatantsForSession reproduces the ability on every rebuild and the close-out can still see that a rage fired.

On top of that, the close-out itself: postCombatBookkeeping(...) in combat_bridge.go now carries the achievements + subclass persistence, and all four close-outs route through it — runDungeonCombat, runZoneCombatRoster, finishCombatSession, finishPartyCombatSession. The turn-based pair reach it via postCombatBookkeepingForSeat, which rebuilds the CombatResult the hooks need from the persisted session (seatCombatResult) and re-derives the fight-start mods (seatFightStartMods).

It fires on every terminal status, not just a win — a Berserker who rages and loses is still exhausted, which is what auto-resolve always did.

Also fixed in passing: buildFightSeats and runZoneCombatRoster consumed the armed ability before the checks that could sit a seat out, so a downed member was disarmed for a fight they never joined. The refusals now run first.

Balance note. Turn-based Berserkers now keep rage for the whole fight instead of one round. That is a buff, but it is the buff the player already paid for. The class-balance corpus measures the auto-resolve path, so it is unmoved.

New tests: internal/plugin/combat_armed_ability_test.go (9 cases) — purity/repeatability of applyAbilityByID, once-only consume, rage surviving rebuilds, the sat-out member keeping their ability, per-seat statuses isolation, and exhaustion on both a manual win and a manual loss.

The Grim Harvest half of this item was closed separately — see item M.

B. The other six supply writers raced

All six now go through withExpeditionSupplies (dnd_expedition.go), which takes advExpeditionLock(expID), re-reads the expedition, hands the closure the fresh row, and persists what it returns:

dnd_expedition_cycle.go (nightRolloverBurn, forage + burn in one write), dnd_expedition_milestone.go (grantTwoWeeksCache), dnd_expedition_region_cmd.go (advanceToNextRegion transit burn), dnd_expedition_camp.go (campPitch), expedition_autocamp.go (pitchAutopilotCamp), expedition_ambient.go (pack-rat drain).

Fix #3's hand-rolled lock in expeditionCmdAccept was folded onto the same helper. Seven call sites, no nesting — verified none of the closure callees re-enter the lock (sync.Mutex is not reentrant).

expedition_sim.go:1421 is left alone on purpose: the sim is single-threaded and takes no plugin locks.

The atomic-json_set-delta alternative is still the better long-term shape (it would drop the lock entirely) but it couples updateSupplies to the blob format.

---

Third pass (2026-07-10, 88c5fcd) — /code-review high on the follow-up stack

Reviewed git diff origin/main...HEAD (the 7 pushed follow-up commits) with 8 finder angles → dedup → 1 verifier per candidate. The removed-behavior angle came back empty: the close-out refactor (item D), parseMenuIndex (item I), the GrimHarvest stash (item M), and the endRunOnLoss/applyOwnerWinEffects/ grantSeatWinXP extractions all verified behavior-preserving. Findings clustered in the new item-C extraction guard, plus one ordering nuance in the item-N Misty wiring. Five fixed:

1. The extraction guard fell open on a DB error. expeditionCmdStart's resumable-guard did switch n, err := partySize(pending.ID) and gated the party-blocking refusal on case err == nil && n > 1. A transient roster-read error skipped both cases, fell through, and started a new expedition on top of the still-seated party — the exact orphaning the guard exists to prevent, in the one situation (a DB hiccup on the roster read) where it matters. Now checks extractionLapsed first (pure, no DB call on the reap path) and treats a partySize error as "assume occupied → refuse". Also drops the wasted COUNT the old switch-init ran on the lapsed-reap path.

2. The start-path lapsed reap unseated members silently. When a leader with a lapsed party extraction ran !expedition start, the reap called completeExpedition(...Failed) — which frees the roster — with no DM, unlike the hourly sweeper and !expedition abandon, which both notify. Extracted reapLapsedExtraction(e) (reap + notify the audience) as the single reap-with-notify path; the sweeper and the start-path reap both route through it, so a member is never silently unseated by whichever path reaches the row first.

3. Misty's crowd swung before the concentration pulse. Item N's new stepRoundEnd seat-loop ran seatEndOfRound (crowd revenge, which can end the fight) before the pre-existing concentration-aura tick. Concentration is turn-engine-only (no auto-resolve counterpart), so this was a within-engine ordering call, not a cross-engine divergence — but a caster whose lingering aura would kill the enemy that round could be dropped by crowd revenge first, contradicting the concentration block's own "a lethal pulse settles the fight" intent. Moved the Misty seat-loop to after the concentration tick: a lethal pulse now wins via finish(CombatStatusWon) before the end-of-round crowd swing; on any round the pulse doesn't end the fight, both procs fire exactly as before. (Owner-approved reorder — it was reported as a balance call and the answer was "move it".)

4. Fourth copy of the event-log scan. seatCombatResult hand-rolled a misty_heal scan loop. Promoted the test-only hasAction(events, action) helper to production (combat_session_build.go), used it there, and removed the duplicate test definition.

5. New dnd_-prefixed file. Renamed dnd_expedition_extract_sweep_test.go → expedition_extract_sweep_test.go, per the no-new-dnd_-names rule (the diff's three other new test files already avoided the prefix).

Reported, not fixed (deliberate):

• Misty ordering was the only correctness-shaped finding; the other angles agreed the stack was clean. Two lower items were left: dnd_setup.go's respec/wipe disbands an extracted party with no member DM — but the disband is intended (item C hole #3) and the missing DM is outside the reviewed lines; and abandonExpedition re-queries the row the caller already resolved — the divergence a finder posited isn't reachable (the extracted-row branch only runs when no active row exists), so the extra indexed SELECT was left alone.

Full plugin suite green before and after. No auto-resolve path touched, so the class-balance corpus and combat_characterization.golden do not move.

---

Deferred — real, not fixed

M. Mage subclass spell hooks never run in turn-based combat — TWO-THIRDS MIS-STATED. Fixed 2026-07-10.

The premise was wrong. applyMageSubclassSpellHooks has three non-test callers, not one. resolveTurnSpell (dnd_spell_combat.go:341) has called it since Phase 13 shipped per-round casting (5cd343a, 2026-05-14).

So on the turn-based surface:

• Empowered Evocation (L7, +INT to evocation damage) — always worked.
• Evocation Overchannel (L10, ×1.5 on a 1st–5th-level slot) — always worked.

Both only move mods.SpellPreDamage, which resolveTurnSpell returns as out.EnemyDamage. Nothing was lost.

Grim Harvest was the only real defect, and its cause was narrower than "the hooks don't run": the hooks ran, wrote mods.GrimHarvestSlot — and resolveTurnSpell dropped that local CombatModifiers on the floor, 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 cannot ride on fight-start mods the way it does in auto-resolve: the spell is cast mid-fight, and the turn engine rebuilds its combatants every round. So it rides where every other mid-fight fact rides — the casting seat's ActorStatuses:

1. turnSpellOutcome gained GrimHarvestSlot / GrimHarvestNecrotic.
2. castActionForSeat parks them on actorStatusesPtr(seat) when a cast lands. snapshotActor starts from the prior snapshot, so they survive commit() untouched, exactly as ArmedAbility does.
3. seatFightStartMods reads them back at close-out, in place of the comment that used to explain why they were permanently zero.

And the killing-blow check was wrong for this surface. grimHarvestHeal scanned for the first spell_cast event and refused the heal if the enemy survived it. Auto-resolve casts once, pre-combat, so first == last there; a turn-based mage casts every round, so an opening cantrip that left the enemy standing vetoed the heal the round-3 killing spell had earned. It now scans for the last spell_cast — provably identical on the auto-resolve path, which is why the golden corpus does not move.

Each damaging cast overwrites the stash, so it always describes the seat's most recent landed spell — the only one that can have been lethal. A miss stashes nothing, and a stale stash is inert because the last spell_cast event then shows the enemy alive.

Balance note. This is a caster buff on the manual surface — 2×/3×/4× slot level in HP, once, on a spell kill, for L5+ Necromancy Mages. It is the buff the subclass was written to have and auto-resolve already paid out. Consistent with the standing "lift trailers" stance. The class-balance corpus measures auto-resolve and is unmoved.

New tests: internal/plugin/combat_grim_harvest_turn_test.go (7 cases) — the stash surviving resolveTurnSpell, nobody-else-stashes, last-cast-not-first, weapon-kill-after-a-spell, the snapshotActor round-trip, the end-to-end heal at postCombatBookkeepingForSeat, and per-seat isolation in a party fight.

N. The NPC procs do not exist in the turn engine (RE-VERIFIED 2026-07-10 — conclusion holds, evidence corrected, scope wider) — the debuff exploit fixed 2026-07-10.

Verified before planning, after items C and M both turned out to rest on a bad "only one caller" claim. This one survives, but two of its three sentences did not.

The CrowdRevengeProc exploit is now closed (see the fix note at the end of this item). Closing it pulled MistyHealProc in with it: both are round-end effects, and the honest hook runs the pair rather than the debuff alone — a one-sided hook would have been a second parallel sibling of the kind item D warns about. So Misty's heal is now live on the turn path too. That is a player-favourable discovery mechanic and fits the "lift trailers" stance, so it ships together; only SniperKillProc stays turn-dead, because it is a pre-combat one-shot with no round-end seam to hang on.

Correction 1 — the location was wrong. MistyHealProc is not read inside simulatePartyWithRNG. It is read at combat_engine_party.go:520 inside endOfRoundForSeat, a helper, which is exactly the shape that made C and M wrong. So the question had to be asked properly: endOfRoundForSeat has one caller, simulatePartyRound (:418), which is the auto-resolve round. The turn engine runs its own stepRoundEnd and never calls it. Conclusion stands. SniperKillProc (:102, genuinely in simulatePartyWithRNG, a pre-combat one-shot) has no turn-engine counterpart either.

Correction 2 — the procs are built, then ignored. buildZoneCombatants calls DerivePlayerStats (combat_session_build.go:61), which sets mods.MistyHealProc / mods.SniperKillProc (combat_stats.go:180,184) on every turn-based combatant. The mods are present on the struct every single round. No turn-engine code path reads either field. It is a dangling write, not a missing build.

Correction 3 — there is a third proc, and it cuts the other way. CrowdRevengeProc (:473) sits in the same endOfRoundForSeat and is equally absent from the turn engine. That one is the Misty debuff. So the asymmetry is not "manual fights are worse": a buffed player loses their buff by fighting manually, and a debuffed player escapes their debuff by doing the same. The debuff half is the one worth caring about, because it is exploitable and needs no discovery to exploit — just fight everything with !attack.

Not gaps — checked and explained, so nobody re-files them:

• PetAttackProc and SpiritWeaponProc are reimplemented in the turn engine (petStrike, spiritWeaponStrike), though they fire after a player action rather than at round end.
• EnvironmentProc is absent because turnCombatPhase is a single flat "Duel" phase with the proc at 0 — deliberate, not dropped.
• HealItem's auto-heal never fires, but the turn engine seeds and snapshots HealChargesLeft and gives the player !consume instead. Player-driven by design, not a leak. (Worth confirming the charges aren't double-counted across the two surfaces.)

The achievement claim held when written; the fix moved it. At the time, combat_sniper_kill and combat_misty_clutch were both unreachable from a manual kill and seatCombatResult hardcoded both flags false — so "six achievements is really four" was correct. Wiring the heal changed that: seatCombatResult now reads MistyHealed back off the misty_heal event on the log (the same way combat_pet_save has always been detected), so combat_misty_clutch is reachable turn-based now — it is five, not four. combat_sniper_kill stays unreachable. The other four were always reachable: combat_near_death (computed by seatCombatResult), combat_pet_save (the turn engine does call resolveEnemyAttack, which emits pet_whiff), combat_death_save (trySave in stepRoundEnd), and combat_consumable_used (combat_cmd.go:773).

(Keep this internal. Per project convention these procs are hidden discovery mechanics and must not surface in help text, player docs, or DMs.)

Fix (2026-07-10). Both Misty procs were hoisted out of endOfRoundForSeat into shared helpers — mistyCrowdRevenge and mistyHeal — and a seatEndOfRound hook that runs the pair in the order the auto-resolve engine does (crowd first, then, if the seat survived, the heal). endOfRoundForSeat now calls the helpers; the turn engine's stepRoundEnd calls seatEndOfRound per seat, after the poison tick so a heal can answer the round's damage. Both helpers short-circuit before touching the RNG when their proc is unarmed, so a character with no Misty history draws exactly the dice it drew before — the sim corpus and combat_characterization.golden do not move (full suite confirms).

renderAllySeatEvent gained a crowd_revenge case: an ally sees the damage land (silence would read as HP vanishing) but not Misty's name, matching the neighbouring misty_heal's neutral "recovers N" — the grudge stays the owner's own discovery.

New tests: combat_misty_turn_test.go (6 cases) — the crowd landing on the manual surface (the exploit), a lethal crowd ending a solo fight, the heal firing and capping, no heal for a seat the crowd just dropped, the no-RNG-when-unarmed property that protects the golden file, and an end-to-end wiring test through advanceCombatSession that reports PlayerHP=40 (the shipped exploit) if the stepRoundEnd call is removed.

O. trySimAutoArm used to re-arm every round — sim baselines will move (NEW)

Before item A's fix, trySimAutoArm lived inside buildZoneCombatants. On the turn-based path that meant: every rebuild re-armed the class-default ability, spent another point of the resource, and re-applied it. A simulated Fighter got a fresh Second Wind heal every single round until stamina ran dry; a Cleric, a Healing Word.

expedition-sim's autoDriveCombat drives handleAttackCmd, i.e. the turn-based path — so this inflated every turn-based sim result. It now arms once per fight, which is what simAutoArmEnabled's own doc comment says it models ("a competent real player who would !arm Second Wind before each fight").

Any expedition-sim corpus that involved turn-based combat is stale. The pending final L10/L12 re-baseline should be run after this change, not before. The pure auto-resolve corpus (SimulateCombat / simulateParty) is unaffected — that path always armed exactly once.

C. Members and leaders disagree about an expedition during extracting — MIS-STATED; the real bug was worse. Fixed 2026-07-10.

The premise was wrong. getActiveExpedition — the leader's lookup — also filters status = 'active'. During extracting the leader gets "no active expedition" from !expedition, !map, !camp &c. exactly as the member does. They already agree, and honestly so: nobody is standing in the dungeon. Widening expeditionForMember would have made the member the only player who can see a paused expedition. Not done, and it should not be.

Reading for it surfaced the real defect underneath.

The seven-day resume window was never enforced. releaseParty fires only on a terminal status, and dnd_expedition.go:428 justifies skipping it for extracting by asserting "the roster is cleared when the resume window lapses and the row flips to failed". Nothing did that. The only extracting → failed transition in the tree was inside handleResumeCmd — a lazy expiry that runs only when the leader personally types !resume.

So a leader who quit, forgot, or simply started a different expedition left the row extracting forever. releaseParty never ran, every member stayed seated, and assertNotAdventuring kept refusing them a run of their own. Fix #2 gave them !expedition leave as an escape, which is why this was a permanent nag rather than a permanent soft-lock — but a player should not have to find it.

Three holes, all closed:

1. No sweeper. sweepLapsedExtractions (dnd_expedition_extract.go) reaps every extracting row past completed_at + 7d through completeExpedition, which releases the roster, and DMs the audience. Hourly ticker (expeditionExtractionSweepTicker), plus a one-shot at Start() — a lapse that happened while the bot was down is blocking those members now. The audience is read before the close-out, since completeExpedition disbands the roster expeditionAudience reads. handleResumeCmd's lazy expiry stays, now sharing the extractionLapsed predicate (which treats a NULL completed_at as lapsed — it is the only clock the window has).

2. The leader could orphan their own party. !expedition start checked only getActiveExpedition, so a leader could start fresh on top of an extracted row. !resume resolves the newest extracting row, so the old one became unreachable — un-resumable, un-abandonable, roster still held. It now refuses, but only when that row has a roster (partySize > 1); a solo extraction strands nobody, so walking away from one stays a normal thing to do. A lapsed row is reaped in place and the start proceeds.

3. The leader had no way out but to pay. expeditionCmdAbandon resolved through activeExpeditionFor and so could not see the extracted row it owns — the leader had to buy a !resume just to abandon. Both it and abandonExpedition now span extracting via ownedLiveExpedition, which sorts active rows first so expeditionCmdStart's run-spawn rollback still tears down the row it just created rather than an older extracted one. This also fixes dnd_setup.go:394,449 for free: a leader who rerolled their character used to strand their whole party.

Abandoning now DMs the members too, and says the true thing when the party is standing in town rather than in the dungeon (supplies already spent, loot already banked — not "supplies are forfeit").

New tests: dnd_expedition_extract_sweep_test.go (6 cases) — the sweep releasing a stranded roster, leaving a live extraction alone, the NULL-completed_at predicate, abandon reaching an extracted row and freeing the party, abandon preferring the active row, and the no-live-row error.

Not covered: the two expeditionCmdStart / expeditionCmdAbandon refusals, which sit above the DM boundary. Still the dmSink gap.

D. Two turn-based win close-outs must be kept in sync by hand — effect-drift closed 2026-07-10.

finishPartyWin (combat_party_finish.go) was a hand-copied sibling of the CombatStatusWon block in finishCombatSession (combat_cmd.go): mood scan, kill record, threat, boss threat, XP + near-death, loot, TwinBee line, continue hint. finishPartyCombatSession routes solo back to finishCombatSession, so the two must stay equivalent forever, with nothing forcing it. Item A was the first drift.

Item A's fix already pulled the bookkeeping out of both into postCombatBookkeepingForSeat. This pass hoists the remaining duplicated effects into three shared helpers in combat_party_finish.go, and routes both close-outs through them:

• applyOwnerWinEffects(owner, enemyID, elite) bool — the zone-kill record, room threat, and boss-defeat threat drop; returns bossOnExpedition. Fires once, owner-scoped (a member owns neither row).
• grantSeatWinXP(uid, hp, hpMax, monster, tier) — near-death calc + XP grant. Per seat (solo calls it once with seat 0).
• endRunOnLoss(owner, runID, death) — mood event (death only) + abandonZoneRun
  • forceExtractExpeditionForRunLoss, shared by both the Lost and Fled paths.

What deliberately stays divergent, so this was not a full closeOutSeat merge: the player-facing text genuinely differs (solo "You finished at N/M HP" vs party "The party stands"; the party's 4-way boss/hint split vs the solo 2-way), and the death-on-win rule is roster-size-dependent by design — solo finishCombatSession never marks a winner dead, finishPartyWin marks a seat that won from 0 HP dead. That is item E and must not be unified. So the effect lists are now a single copy each; the text is not.

Full plugin suite green; no behavior change (helpers are extract-and-call).

Note this is the only place the party work chose a parallel sibling. Elsewhere the branch is a genuine N-body generalization, not a bolt-on: SimulateCombat → simulatePartyWithRNG, runCombatRound → runPartyCombatRound, runZoneCombat → runZoneCombatRoster, RenderTurnRound → RenderPartyTurnRound.

E. Death-on-win depends on roster size — intentional, leave it

closeOutZoneWin marks a downed seat dead on a win only when len(seated) > 1. A solo player who wins at 0 HP (retaliate aura kills the swinger on the killing blow) survives; a party member in the same spot dies.

This is deliberate and documented in place (zone_combat_party.go:183-188): unifying it would move the sim's outcome labels and force a re-baseline of testdata/combat_characterization.golden. Not a defect. Listed so the next person to find it doesn't "fix" it by accident.

F. grantAutorunGrace is a stopgap (already tracked as R5)

zone_revisit.go:172 stamps last_autorun_at to buy one autoRunCooldown window after a !revisit, because the autopilot ticker has no concept of position-vs-frontier and will otherwise march the party back out of the room they just backtracked to. The code says so itself and defers the real fix to R5. It is coupled to the exact value of autoRunCooldown and to the ticker cadence.

G. Party combat multiplies the known per-turn DB chatter

Already tracked as project_combat_session_cache_deferred, now worse:

• partyCombatantsForSession (combat_session_build.go:137) rebuilds every seat from scratch on every !attack/!cast/!consume (~6–8 SELECTs per seat). A 3-member, 5-round fight ≈ 270–360 SELECTs.
• rebuildRoster (combat_cmd.go:620) then rebuilds all seats again after a mid-fight buff, when only the casting seat changed.
• runZoneCombatRoster (zone_combat_party.go:73) loads char + equipment, then buildZoneCombatants loads both again and discards the first pair — once per seat per room, on the path that fights every room.
• nudgeStalledPartyTurns (combat_party_turn.go:281) does two session reads plus a full N-seat rebuild per stalled fight, serially, on the 1-minute ticker, while holding the fight lock.
• The enemy stat block is rebuilt once per seat and kept only for seat 0 (the code comments admit it). CPU-only, but pure waste.

Cheapest real fix: memoize the built roster for the fight's lifetime. Mid-fight buffs are already tracked as ActorStatuses deltas and re-applied by applySessionBuffs, so stats are reproducible without re-reading the DB.

H. Guard probes re-query per message — MEASURED, DECLINED 2026-07-10.

isPartyMember → activeExpeditionFor is up to 2 queries; activeZoneRunFor adds more. A solo player with nothing in flight costs 3 queries to conclude "not in a party". The original write-up said this was "repeated by each guard a handler consults" and proposed resolving seat/expedition/run once per message into MessageContext and threading it.

The premise was overstated, and the fix is disproportionate. Not done.

• No per-message guard cascade exists. OnMessage dispatch is strictly one-handler-per-command (if IsCommand(...) return handle(...)). Nothing runs these resolvers for every message; the redundancy is intra-handler only.
• The one hot inbound path is already optimized. zoneCmdAdvance / expeditionCmdRun call isPartyMember directly by design — the code comment spells out that this avoids re-resolving the run (which advanceOnce is about to resolve) and double-firing the §4.3 idle reap. The authors already closed the hot case.
• The remaining 2–3× callers are cold. A per-function scan found ≥2 resolver calls only in zoneCmdEnter, zoneCmdAbandon, expeditionCmdStart, the status impls (all one-shot commands), and runAutopilotWalk (5×, but it is the background ticker path, not an inbound message). In those the calls are typically a guard-check on one branch then a resolve on another — not the same read twice.
• The proposed fix has a real regression surface. MessageContext is a shared cross-plugin value type (movies, vibe, miniflux, …); the resolvers are free functions used at 60+ sites. Threading a resolved snapshot means changing the shared type and every call site, and carrying a memo that goes stale the moment a handler writes the row mid-flight — the exact staleness-bug class items A/M/N just fixed. All to save a few cheap indexed local SQLite reads on a low-volume bot.

Cost/benefit is upside-down: high churn + a staleness regression surface for a negligible perf gain on paths that are mostly already optimal. Left as-is deliberately. Revisit only if a profiler ever shows these reads on a hot path.

I. parseTemperIndex is the third copy of the same menu parser — Fixed 2026-07-10.

adventure_temper.go duplicated the "read leading digits, 1-indexed → 0-indexed, bounds-check" reply parser already inlined in resolveMagicEquipReply (magic_items_gameplay.go) and handleMasterworkEquipReply (adventure_masterwork.go). Promoted the temper copy to parseMenuIndex(reply, n) and routed the other two through it.

Behaviour-preserving on all three: the old parsed flag was redundant with the idx < 0 guard (no leading digit leaves idx=0, then idx-- → -1, rejected either way), so no input changes verdict. The retry disagreement the doc flagged was deliberately left as-is — this was a pure dedup, so magic-items and temper still re-store the pending interaction on a bad parse and masterwork still doesn't. Unifying that is a behaviour change and belongs in its own pass if wanted. Full plugin suite green; the existing parseMenuIndex table test moved with the rename.

J. !sell fires the event anchor even when nothing sold (PLAUSIBLE)

adventure.go:719 calls maybeFireAnchoredEvent unconditionally after handleSellCmd, so !sell nonexistentitem can burn the player's one daily event slot. Gating it needs advSellItem/advSellAll to return a sold bool.

Probably leave it. The anchor's stated design is presence-based — "a moment the player is present and reading" — and a player who typed !sell and read the reply is present. That matches the project's own rule that presence means any chat, not a successful command. Flagged only so the choice is explicit.

K. Confirm the A6 anchor set covers the player population — CONFIRMED REAL GAP 2026-07-10.

N1/A6 replaced the old "any daily-active player, any activity" mid-day event roll with three presence anchors: the expedition Night digest, !sell, and arena cashout. tryTriggerEvent's HasActedToday gate was dropped with it.

The code confirms the gap is real, not hypothetical. The three, and only three, maybeFireAnchoredEvent call sites are:

1. expedition_autorun.go:312 — the end-of-day digest, and it is gated on campDecision.Night. A Night camp only happens on a multi-day expedition; a single-day dungeon/!zone run never reaches one.
2. adventure.go:734 — after a !sell at Thom's.
3. adventure_arena.go:633 — after an arena cashout.

So the excluded population is concrete: a player whose daily loop is mining / foraging / fishing / babysit / single-day dungeon, who never sells at Thom's, never enters the arena, and never runs a multi-day expedition, receives zero mid-day events — the roll never even happens for them. That is exactly the core daily-grind loop, so this is plausibly a large fraction of players, not an edge case.

Fourth anchor drafted 2026-07-10 (uncommitted, pending owner review). The on-theme, farm-resistant home is a foreground single-day !zone clear — the climax DM the dungeon/harvest-via-walk player is demonstrably reading, and one they cannot spam (a clear costs a full walk). Implemented as:

• advEventChanceZoneClear = 0.08 in adventure_events.go (matches the digest; it is the day's climax moment, and it is the single tuning knob).
• advanceResult.zoneCleared set true only in the full-clear branch of advanceOnceWithOpts (not a mid-zone region clear, which continues the run and would fire per region).
• Rolled in zoneCmdAdvance after the clear DM streams, via the unchanged maybeFireAnchoredEvent. That is the foreground path only — autopilot (runAutopilotWalk) and party members (refused earlier by isPartyMember) never reach it. The per-day slot guard still caps everyone at one event/day.

Rates (see TestZoneClearAnchorRate): a zone-clear-only grind player at ~2 clears/day lands ~1.08 events/week — the same band as the fully-engaged digest+sell+arena player (~1.19/week, unchanged). The rare all-four-in-one-day player peaks at a ~1.65/week probability ceiling but still gets at most one actual event/day.

Two knobs remain the owner's call, hence "pending review": the 0.08 chance, and whether a single-day zone clear is even the right anchor for the grind loop vs. !resources (rejected here as farmable — repeated !resources would just re-roll the same daily slot) or leaving the grind loop event-free by design. Full plugin suite green; no existing behaviour moved (the three prior anchors and TestAnchoredEventWeeklyRate are untouched).

L. Dead helper: openParty — Verified and removed 2026-07-10.

Confirmed dead: a repo-wide grep found openParty only in expedition_party.go (the def) and expedition_party_test.go. The invite path goes joinParty → seatLeader, which runs the same INSERT ... role 'leader' ON CONFLICT DO NOTHING but reads the owner off the expedition row instead of trusting a passed-in id. Deleted openParty; the eleven test call sites now use a seatLeaderFixture(t, expID) helper that wraps seatLeader in a transaction (the owner comes from the seedExpedition row, which always matched the id the old calls passed). seatLeader's doc comment absorbed the idempotency note that justified openParty. Full plugin suite green.

---

Test gap worth closing — the DM seam CLOSED 2026-07-10

SendDM went straight through Base to a live client, with no fake/sink seam, so the handler-level paths behind fixes #1, #2 and #5 could not be unit-tested. That is why the party close-out bug survived a suite this thorough: every party test is unit-level and stops below the DM boundary.

Fixed 2026-07-10. Added a MessageSink interface and a Base.Sink field (plugin.go). When non-nil it captures every outbound message — both the DM route (SendDM/SendDMID) and the room route (SendMessage/SendMessageID, which is what the arena announcement actually uses) — and the live client is never touched. Nil in production, so behaviour is unchanged; the whole internal/... suite is green with the field added. One seam, no call-site churn: the 765 SendDM/replyDM/SendMessage sites are all downstream of these four methods.

New tests (message_sink_test.go): a captureSink test double + installSink helper; TestMessageSink_CapturesDMAndRoom proving both routes divert with the right target/text and that the *ID variants report back an id; and TestExpeditionCmdLeave_DMsBothEnds, which drives the real fix-#2 handler end to end and asserts both DMs land (member "turned back", leader notified) — a path that was literally a silent no-op in a unit test before. beginCombatTurn's terminal path and the arena rollover announcement are now reachable the same way (seam proven for both the DM and room routes); dedicated end-to-end tests for those two remain easy follow-ups if wanted.

Added this pass: TestPartyCasualtyLine_JoinsNamesLikeTheRestOfTheBot, and TestPartySurvivors_SplitsOnHPNotOnOutcome was rewritten as TestAnySurvivor_ReadsHPNotOutcome when its helper was deleted.

Added in the second pass (combat_armed_ability_test.go): the close-out bookkeeping is now reachable below the DM boundary via postCombatBookkeepingForSeat(sess, seat), which takes a plain *CombatSession and needs no client — so item A's behaviour is directly unit-tested even though finishCombatSession itself still isn't. (The dmSink seam that would have covered finishCombatSession itself now exists — see above.)

Not covered by tests: item B's locking. withExpeditionSupplies is exercised incidentally by the existing expedition suite, but nothing asserts the mutual exclusion. A t.Parallel accept-vs-burn race test under -race would.