From 4596008b7342a0120c01ab0ac94560b7e533937a Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Sat, 18 Apr 2026 09:26:37 -0700 Subject: [PATCH 1/3] fix(combat): dash actually extends the enforced movement budget The consolidated dash handler was a stub: it returned a success message saying "Movement speed doubled" but never touched the engine, so the follow-up move action still enforced the original 30ft budget and rejected with "Insufficient movement." Observed in live session when Rurk tried to dash + move to close the aisle. - CombatEngine.applyDash(): adds a fresh baseSpeed allotment on top of any existing movementRemaining, flags hasDashed, and consumes the main action (matches 5e dash rules). - Consolidated dash handler calls applyDash and reports the new movementRemaining so callers can see their budget. Closes #50 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/combat/engine.ts | 21 ++++++++++++++++ src/server/consolidated/combat-action.ts | 24 +++++++++++++++--- .../server/consolidated/combat-action.test.ts | 25 +++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/engine/combat/engine.ts b/src/engine/combat/engine.ts index d8471da..dd3cd93 100644 --- a/src/engine/combat/engine.ts +++ b/src/engine/combat/engine.ts @@ -995,6 +995,27 @@ export class CombatEngine { participant.movementRemaining = participant.movementSpeed ?? 30; } + /** + * Apply the Dash action for a participant. Adds a fresh movement + * allotment on top of whatever they already have left, and flags + * hasDashed so the budget-check in move actions sees the doubled + * window. Consumes the main action. + */ + applyDash(participantId: string): { ok: true; movementRemaining: number } | { ok: false; error: string } { + if (!this.state) return { ok: false, error: 'No active combat' }; + const participant = this.state.participants.find((p) => p.id === participantId); + if (!participant) return { ok: false, error: `Participant ${participantId} not found` }; + if (participant.hasDashed) return { ok: false, error: 'Already dashed this turn' }; + + const baseSpeed = participant.movementSpeed ?? 30; + const currentRemaining = participant.movementRemaining ?? baseSpeed; + participant.movementRemaining = currentRemaining + baseSpeed; + participant.hasDashed = true; + participant.actionUsed = true; + + return { ok: true, movementRemaining: participant.movementRemaining }; + } + /** * Process start-of-turn condition effects */ diff --git a/src/server/consolidated/combat-action.ts b/src/server/consolidated/combat-action.ts index 548c42f..109cb4c 100644 --- a/src/server/consolidated/combat-action.ts +++ b/src/server/consolidated/combat-action.ts @@ -9,6 +9,7 @@ import { createActionRouter, ActionDefinition, McpResponse } from '../../utils/a import { SessionContext } from '../types.js'; import { RichFormatter } from '../utils/formatter.js'; import { handleExecuteCombatAction } from '../handlers/combat-handlers.js'; +import { getCombatManager } from '../state/combat-manager.js'; // ═══════════════════════════════════════════════════════════════════════════ // CONSTANTS @@ -188,13 +189,30 @@ const definitions: Record = { schema: DashSchema, handler: async (params: z.infer) => { if (!currentContext) throw new Error('No session context'); - // Dash doubles movement speed for the turn + const engine = getCombatManager().get(`${currentContext.sessionId}:${params.encounterId}`); + if (!engine) { + return { + error: true, + actionType: 'dash', + message: `Encounter ${params.encounterId} not found.` + }; + } + const result = engine.applyDash(params.actorId); + if (!result.ok) { + return { + error: true, + actionType: 'dash', + actorId: params.actorId, + message: result.error + }; + } return { success: true, actionType: 'dash', actorId: params.actorId, - effect: 'Movement speed doubled for this turn', - message: `${params.actorId} takes the Dash action, doubling movement speed.` + movementRemaining: result.movementRemaining, + effect: `Movement speed doubled for this turn (budget now ${result.movementRemaining}ft)`, + message: `${params.actorId} takes the Dash action. Movement doubled; ${result.movementRemaining}ft remaining.` }; }, aliases: ['sprint', 'run', 'hustle'] diff --git a/tests/server/consolidated/combat-action.test.ts b/tests/server/consolidated/combat-action.test.ts index ce2ac3f..a526b23 100644 --- a/tests/server/consolidated/combat-action.test.ts +++ b/tests/server/consolidated/combat-action.test.ts @@ -282,6 +282,31 @@ describe('combat_action consolidated tool', () => { // Alias resolves to dash expect(data.actionType).toBe('dash'); }); + + // Regression for issue #50: dash was a stub that returned a success + // message without actually extending movementRemaining, so the next + // move call still enforced the base 30ft budget. + it('dash actually doubles the enforced move budget', async () => { + const dashResult = await handleCombatAction({ + action: 'dash', + encounterId: testEncounterId, + actorId: 'hero-1' + }, ctx); + const dashData = parseResult(dashResult); + expect(dashData.success).toBe(true); + expect(dashData.movementRemaining).toBe(60); + + // Move 9 tiles diagonally (~45ft at 5ft/sq). Without dash this + // would exceed the 30ft budget; with dash (60ft) it fits. + const moveResult = await handleCombatAction({ + action: 'move', + encounterId: testEncounterId, + actorId: 'hero-1', + targetPosition: { x: 14, y: 14 } + }, ctx); + const moveData = parseResult(moveResult); + expect(moveData.success).toBe(true); + }); }); describe('dodge action', () => { From ebf6fe97954f71121f2c417d8d00066381867888 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Sun, 19 Apr 2026 12:23:48 -0700 Subject: [PATCH 2/3] fix(combat): dash validates action economy and auto-loads encounter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewers (PR #60) flagged two gaps: 1. applyDash only checked hasDashed before consuming the action, so a participant who had already attacked could still dash for a free 60ft of movement. Dash IS the action in 5e — calling validateActionEconomy(participantId, 'action') first rejects the illegal sequence. 2. The dash handler short-circuited with "Encounter not found" when the engine wasn't in memory, unlike other combat actions which auto-load from EncounterRepository. After a process restart or manager eviction, dash failed even though attack/move still worked. Adds matching auto-load path in the consolidated dash handler and regression tests for both behaviors. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/engine/combat/engine.ts | 8 ++++ src/server/consolidated/combat-action.ts | 23 ++++++++++- .../server/consolidated/combat-action.test.ts | 38 +++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/engine/combat/engine.ts b/src/engine/combat/engine.ts index dd3cd93..a3c57f7 100644 --- a/src/engine/combat/engine.ts +++ b/src/engine/combat/engine.ts @@ -1007,6 +1007,14 @@ export class CombatEngine { if (!participant) return { ok: false, error: `Participant ${participantId} not found` }; if (participant.hasDashed) return { ok: false, error: 'Already dashed this turn' }; + // Dash IS the action. Reject if the participant already burned their + // main action this turn (5e action economy). Without this guard, a + // caller could attack and then dash for a free 60ft of movement. + const econ = this.validateActionEconomy(participantId, 'action'); + if (!econ.valid) { + return { ok: false, error: econ.error || 'Action already used this turn' }; + } + const baseSpeed = participant.movementSpeed ?? 30; const currentRemaining = participant.movementRemaining ?? baseSpeed; participant.movementRemaining = currentRemaining + baseSpeed; diff --git a/src/server/consolidated/combat-action.ts b/src/server/consolidated/combat-action.ts index 109cb4c..8795004 100644 --- a/src/server/consolidated/combat-action.ts +++ b/src/server/consolidated/combat-action.ts @@ -10,6 +10,9 @@ import { SessionContext } from '../types.js'; import { RichFormatter } from '../utils/formatter.js'; import { handleExecuteCombatAction } from '../handlers/combat-handlers.js'; import { getCombatManager } from '../state/combat-manager.js'; +import { getDb } from '../../storage/index.js'; +import { EncounterRepository } from '../../storage/repos/encounter.repo.js'; +import { CombatEngine } from '../../engine/combat/engine.js'; // ═══════════════════════════════════════════════════════════════════════════ // CONSTANTS @@ -189,7 +192,25 @@ const definitions: Record = { schema: DashSchema, handler: async (params: z.infer) => { if (!currentContext) throw new Error('No session context'); - const engine = getCombatManager().get(`${currentContext.sessionId}:${params.encounterId}`); + + const sessionKey = `${currentContext.sessionId}:${params.encounterId}`; + let engine = getCombatManager().get(sessionKey); + + // Auto-load from DB if the engine isn't in memory (matches the + // pattern in handleExecuteCombatAction). Without this, dash + // returned "not found" after a process restart even when the + // encounter still existed and other actions worked. + if (!engine) { + const db = getDb(process.env.NODE_ENV === 'test' ? ':memory:' : 'rpg.db'); + const repo = new EncounterRepository(db); + const persisted = repo.loadState(params.encounterId); + if (persisted) { + engine = new CombatEngine(params.encounterId); + engine.loadState(persisted); + getCombatManager().create(sessionKey, engine); + } + } + if (!engine) { return { error: true, diff --git a/tests/server/consolidated/combat-action.test.ts b/tests/server/consolidated/combat-action.test.ts index a526b23..cbcd756 100644 --- a/tests/server/consolidated/combat-action.test.ts +++ b/tests/server/consolidated/combat-action.test.ts @@ -283,6 +283,44 @@ describe('combat_action consolidated tool', () => { expect(data.actionType).toBe('dash'); }); + // Reviewer follow-ups on PR #60: + // - dash must consume the action economy slot (else attack-then-dash). + // - dash must auto-load the engine from DB (matches other actions). + it('dash refuses when the actor already used their main action this turn', async () => { + // hero-1 attacks (consumes action) then tries to dash. + await handleCombatAction({ + action: 'attack', + encounterId: testEncounterId, + actorId: 'hero-1', + targetId: 'goblin-1', + attackBonus: 5, + damage: 4 + }, ctx); + + const dashResult = await handleCombatAction({ + action: 'dash', + encounterId: testEncounterId, + actorId: 'hero-1' + }, ctx); + const dashData = parseResult(dashResult); + expect(dashData.error).toBe(true); + expect(dashData.message).toMatch(/already used|action/i); + }); + + it('dash auto-loads the encounter from DB when engine is evicted', async () => { + const { getCombatManager } = await import('../../../src/server/state/combat-manager.js'); + getCombatManager().clear(); + + const dashResult = await handleCombatAction({ + action: 'dash', + encounterId: testEncounterId, + actorId: 'hero-1' + }, ctx); + const dashData = parseResult(dashResult); + expect(dashData.success).toBe(true); + expect(dashData.movementRemaining).toBe(60); + }); + // Regression for issue #50: dash was a stub that returned a success // message without actually extending movementRemaining, so the next // move call still enforced the base 30ft budget. From fa46dfc1b05a4d1538dc0d6f466a54a088c24362 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Sun, 19 Apr 2026 12:53:45 -0700 Subject: [PATCH 3/3] fix(combat): race-safe engine restore in dash auto-load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewers (PR #60, Minor) noted that two concurrent dash requests hitting the auto-load path could both see the engine missing, both load from DB, and both call CombatManager.create — the loser would throw "Encounter X already exists". Adopts the get-or-create pattern: re-check the manager after loading from DB, and wrap create in a try/get fallback so the loser of the race adopts the winner's engine instead of crashing. Regression test fires two dash requests in parallel against an evicted engine; one must succeed and neither may throw 'already exists'. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/server/consolidated/combat-action.ts | 23 +++++++++++-- .../server/consolidated/combat-action.test.ts | 32 +++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/server/consolidated/combat-action.ts b/src/server/consolidated/combat-action.ts index 8795004..3b6d6f8 100644 --- a/src/server/consolidated/combat-action.ts +++ b/src/server/consolidated/combat-action.ts @@ -200,14 +200,31 @@ const definitions: Record = { // pattern in handleExecuteCombatAction). Without this, dash // returned "not found" after a process restart even when the // encounter still existed and other actions worked. + // + // Race-safe restore (PR #60 reviewer ask): two concurrent + // requests can both find the engine missing and both load from + // DB. CombatManager.create throws if the key already exists, so + // wrap the create in a try/get fallback — the loser of the race + // adopts the winner's engine. if (!engine) { const db = getDb(process.env.NODE_ENV === 'test' ? ':memory:' : 'rpg.db'); const repo = new EncounterRepository(db); const persisted = repo.loadState(params.encounterId); if (persisted) { - engine = new CombatEngine(params.encounterId); - engine.loadState(persisted); - getCombatManager().create(sessionKey, engine); + // Re-check in case another concurrent request restored it + // between our initial get() and now. + engine = getCombatManager().get(sessionKey); + if (!engine) { + const candidate = new CombatEngine(params.encounterId); + candidate.loadState(persisted); + try { + getCombatManager().create(sessionKey, candidate); + engine = candidate; + } catch { + // Lost the race — adopt the engine the winner created. + engine = getCombatManager().get(sessionKey); + } + } } } diff --git a/tests/server/consolidated/combat-action.test.ts b/tests/server/consolidated/combat-action.test.ts index cbcd756..54930ea 100644 --- a/tests/server/consolidated/combat-action.test.ts +++ b/tests/server/consolidated/combat-action.test.ts @@ -307,6 +307,38 @@ describe('combat_action consolidated tool', () => { expect(dashData.message).toMatch(/already used|action/i); }); + // Reviewer follow-up on PR #60 (Minor): two concurrent dash calls + // hitting the auto-load path could both try to register the same + // engine key, throwing "Encounter X already exists" on the loser. + it('dash survives two concurrent auto-load requests without throwing', async () => { + const { getCombatManager } = await import('../../../src/server/state/combat-manager.js'); + getCombatManager().clear(); + + const [a, b] = await Promise.all([ + handleCombatAction({ + action: 'dash', + encounterId: testEncounterId, + actorId: 'hero-1' + }, ctx), + handleCombatAction({ + action: 'dash', + encounterId: testEncounterId, + actorId: 'hero-1' + }, ctx) + ]); + + const da = parseResult(a); + const db = parseResult(b); + // One must succeed (the first) and the other must reject due to + // hasDashed/economy — but neither may throw "already exists". + expect([da, db].some((d) => d.success === true)).toBe(true); + for (const d of [da, db]) { + if (d.error) { + expect(String(d.message ?? '')).not.toMatch(/already exists/i); + } + } + }); + it('dash auto-loads the encounter from DB when engine is evicted', async () => { const { getCombatManager } = await import('../../../src/server/state/combat-manager.js'); getCombatManager().clear();