Skip to content

fix(combat): spawn_quick_enemy appends to existing encounter when encounterId given #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Mnehmos merged 4 commits into from
Apr 20, 2026
Merged

fix(combat): spawn_quick_enemy appends to existing encounter when encounterId given #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e9619f
fix(combat): spawn_quick_enemy appends to existing encounter when enc…
Mnehmos Apr 18, 2026
2b874fa
fix(combat): spawn_quick_enemy auto-loads from DB, persists, and read…
Mnehmos Apr 19, 2026
4c70f50
fix(combat): spawn_quick_enemy errors loudly when encounterId is unknown
Mnehmos Apr 19, 2026
5cd7b1a
fix(combat): roll back in-memory append when persistence fails on spawn
Mnehmos Apr 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/engine/combat/engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,54 @@ export class CombatEngine {
*
* If any participant has hasLairActions=true, adds 'LAIR' to turn order at initiative 20
*/
/**
* Add new participants to an existing encounter. Rolls initiative for
* each, resorts the turn order, and keeps currentTurnIndex pointing at
* the same actor (so an insertion ahead of the active turn doesn't skip it).
*/
addParticipants(newParticipants: CombatParticipant[]): CombatState {
if (!this.state) throw new Error('No active combat');

const withInit = newParticipants.map(p => ({
...p,
initiative: this.rng.d20(p.initiativeBonus),
isEnemy: p.isEnemy ?? this.detectIsEnemy(p.id, p.name),
movementRemaining: p.movementSpeed ?? 30,
actionUsed: false,
bonusActionUsed: false,
spellsCast: {},
reactionUsed: false,
hasDashed: false,
hasDisengaged: false
}));

const currentId = this.state.turnOrder[this.state.currentTurnIndex];
const merged = [...this.state.participants, ...withInit];

merged.sort((a, b) => {
const ai = a.initiative ?? 0;
const bi = b.initiative ?? 0;
if (bi !== ai) return bi - ai;
return a.id.localeCompare(b.id);
});

// Rebuild turn order, preserving any LAIR slot at its initiative-20 position.
const newTurnOrder: string[] = merged.map(p => p.id);
if (this.state.hasLairActions) {
const lairIndex = merged.findIndex(p => (p.initiative ?? 0) <= 20);
if (lairIndex === -1) newTurnOrder.push('LAIR');
else newTurnOrder.splice(lairIndex, 0, 'LAIR');
}

// Keep the active participant's turn anchored after the resort.
const newIndex = newTurnOrder.indexOf(currentId);
this.state.participants = merged;
this.state.turnOrder = newTurnOrder;
if (newIndex >= 0) this.state.currentTurnIndex = newIndex;

return this.state;
}

startEncounter(participants: CombatParticipant[]): CombatState {
// Roll initiative for each participant and store the value
const participantsWithInitiative = participants.map(p => {
Expand Down
100 changes: 100 additions & 0 deletions src/server/consolidated/combat-manage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import {
import { expandCreatureTemplate, listAllTemplates } from '../../data/creature-presets.js';
import { getDb } from '../../storage/index.js';
import { CombatActionLogRepository } from '../../storage/repos/combat-action-log.repo.js';
import { EncounterRepository } from '../../storage/repos/encounter.repo.js';
import { CombatEngine } from '../../engine/combat/engine.js';
import { getCombatManager } from '../state/combat-manager.js';

// ═══════════════════════════════════════════════════════════════════════════
// CONSTANTS
Expand Down Expand Up @@ -269,6 +272,9 @@ const definitions: Record<CombatManageAction, ActionDefinition> = {
initiativeBonus: Math.floor((preset.stats.dex - 10) / 2),
hp: preset.hp,
maxHp: preset.maxHp,
ac: preset.ac,
attackDamage: preset.defaultAttack?.damage,
attackBonus: preset.defaultAttack?.toHit,
isEnemy: true,
conditions: [],
position: pos,
Expand All @@ -278,6 +284,100 @@ const definitions: Record<CombatManageAction, ActionDefinition> = {
});
}

// If encounterId is supplied, append the new enemies to that
// encounter. Auto-loads from the database when the engine isn't
// in memory (mirroring handleGetEncounterState / handleExecute*),
// and persists the new state back so a subsequent restart still
// sees the spawned enemies. Only falls back to creating a fresh
// encounter when the id genuinely doesn't exist anywhere.
if (params.encounterId) {
const sessionKey = `${currentContext.sessionId}:${params.encounterId}`;
let engine = getCombatManager().get(sessionKey);
let loadedFromDb = false;

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);
loadedFromDb = true;
}
}

if (engine) {
// Snapshot for rollback before mutating in-memory state.
const beforeIds = new Set(engine.getState()?.participants.map((p) => p.id) ?? []);
const state = engine.addParticipants(
participants as unknown as Parameters<typeof engine.addParticipants>[0]
);

// Persist the appended state so a restart doesn't lose the
// newly spawned enemies. PR #58 reviewer ask: don't return
// success if persistence fails — that splits in-memory and
// DB state. Roll back the in-memory addParticipants and
// surface an explicit error.
try {
const db = getDb(process.env.NODE_ENV === 'test' ? ':memory:' : 'rpg.db');
const repo = new EncounterRepository(db);
repo.saveState(params.encounterId, state);
} catch (err) {
// Roll back: drop the just-added participants so memory
// matches DB. Use the engine's state directly since we
// know the schema.
const live = engine.getState();
if (live) {
live.participants = live.participants.filter((p) => beforeIds.has(p.id));
live.turnOrder = live.turnOrder.filter((id) => id === 'LAIR' || beforeIds.has(id));
}
return {
error: true,
actionType: 'spawn_quick_enemy',
encounterId: params.encounterId,
message: `Failed to persist appended encounter state: ${(err as Error).message}. In-memory append rolled back.`,
rolledBack: true
};
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +326 to +342

@coderabbitai coderabbitai Bot Apr 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Incomplete rollback: currentTurnIndex is not restored.

The rollback correctly restores participants and turnOrder, but addParticipants() (see context snippet 1) also modifies currentTurnIndex to anchor the active turn in the re-sorted order. After rollback, the index may point to the wrong participant or be out of bounds.

Example: if turnOrder was [A, B, C] with index 1 (turn=B), after adding a high-initiative enemy X, it becomes [X, A, B, C] with index 2 (still B). After rollback, turnOrder is [A, B, C] but index remains 2, pointing to C instead of B.

🐛 Proposed fix: capture and restore currentTurnIndex 
                 if (engine) {
                     // Snapshot for rollback before mutating in-memory state.
                     const beforeIds = new Set(engine.getState()?.participants.map((p) => p.id) ?? []);
+                    const beforeTurnIndex = engine.getState()?.currentTurnIndex ?? 0;
                     const state = engine.addParticipants(
                         participants as unknown as Parameters<typeof engine.addParticipants>[0]
                     );
                     // ... persistence attempt ...
                     } catch (err) {
                         // Roll back: drop the just-added participants so memory
                         // matches DB. Use the engine's state directly since we
                         // know the schema.
                         const live = engine.getState();
                         if (live) {
                             live.participants = live.participants.filter((p) => beforeIds.has(p.id));
                             live.turnOrder = live.turnOrder.filter((id) => id === 'LAIR' || beforeIds.has(id));
+                            live.currentTurnIndex = beforeTurnIndex;
                         }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/server/consolidated/combat-manage.ts` around lines 304 - 320, The
rollback restores participants and turnOrder but misses restoring
currentTurnIndex; capture the pre-append currentTurnIndex (from
engine.getState().currentTurnIndex) before calling addParticipants(), and in the
catch block set live.currentTurnIndex = previousIndex (with a safety clamp to
0..live.turnOrder.length-1) so the active turn is correctly restored when you
revert live.participants and live.turnOrder; update the catch handler that
references engine.getState() and live to also restore currentTurnIndex.


return {
success: true,
actionType: 'spawn_quick_enemy',
encounterId: params.encounterId,
creature: params.creature,
spawnedCount: count,
appendedToExisting: true,
loadedFromDb,
enemies: participants.map(p => ({
id: p.id,
name: p.name,
hp: p.hp,
maxHp: p.maxHp,
ac: preset.ac,
position: p.position,
attack: preset.defaultAttack
})),
turnOrder: state.turnOrder,
// currentTurnIndex indexes turnOrder, NOT participants —
// those arrays can diverge when LAIR is in the order.
currentTurn: state.turnOrder[state.currentTurnIndex],
readyForCombat: true,
hint: `Added ${count} ${preset.name}(s) to existing encounter. Initiative re-sorted.`
};
}
// encounterId given but neither in memory nor in DB — return
// an explicit error rather than silently creating a new
// encounter with the spawned enemies. Silent fallback hides
// typos and stale ids from the caller (PR #58 reviewer ask).
return {
error: true,
actionType: 'spawn_quick_enemy',
message: `Encounter ${params.encounterId} not found in memory or DB. Omit encounterId to create a new encounter.`,
requestedEncounterId: params.encounterId
};
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Create encounter with these participants
const seed = params.seed || `quick-${Date.now()}`;
const createParams = {
Expand Down
157 changes: 157 additions & 0 deletions tests/server/consolidated/combat-manage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,163 @@ describe('combat_manage consolidated tool', () => {
expect(p?.isEnemy).toBe(false);
});

// Regression for issue #48: spawn_quick_enemy with encounterId was
// ignoring the id and creating a fresh encounter, leaving PCs without
// opponents in the original.
it('spawn_quick_enemy appends to existing encounter when encounterId is set', async () => {
// Create encounter with PCs only
const createResult = await handleCombatManage({
action: 'create',
seed: 'spawn-append-test',
participants: [
{ id: 'pc-hero', name: 'Hero', initiativeBonus: 3, hp: 30, maxHp: 30, isEnemy: false, position: { x: 0, y: 0 } }
]
}, ctx);
const originalId = parseResult(createResult).encounterId;

// Spawn goblins into the same encounter
const spawnResult = await handleCombatManage({
action: 'spawn_quick_enemy',
encounterId: originalId,
creature: 'goblin',
count: 2,
position: { x: 10, y: 10 }
}, ctx);
const spawnData = parseResult(spawnResult);

expect(spawnData.success).toBe(true);
expect(spawnData.appendedToExisting).toBe(true);
expect(spawnData.encounterId).toBe(originalId);
expect(spawnData.spawnedCount).toBe(2);
// Original encounter now has PC + 2 goblins = 3 participants
expect(spawnData.turnOrder.length).toBe(3);
});

// Reviewer follow-ups on PR #58:
// - currentTurn must come from turnOrder index, not participants[i]?.id.
// - Auto-load from DB when the in-memory engine is gone.
it('spawn_quick_enemy currentTurn comes from turnOrder index', async () => {
const createResult = await handleCombatManage({
action: 'create',
seed: 'spawn-currentTurn-test',
participants: [
{ id: 'pc-hero', name: 'Hero', initiativeBonus: 5, hp: 30, maxHp: 30, isEnemy: false, position: { x: 0, y: 0 } }
]
}, ctx);
const originalId = parseResult(createResult).encounterId;

const spawnResult = await handleCombatManage({
action: 'spawn_quick_enemy',
encounterId: originalId,
creature: 'goblin',
count: 1
}, ctx);
const spawnData = parseResult(spawnResult);

// currentTurn must remain anchored to the pre-existing actor.
// (Asserting turnOrder[0] is brittle to initiative re-sorting.)
expect(spawnData.currentTurn).toBe('pc-hero');
expect(spawnData.turnOrder).toContain(spawnData.currentTurn);
});

it('spawn_quick_enemy auto-loads from DB when engine is evicted from memory', async () => {
const { getCombatManager } = await import('../../../src/server/state/combat-manager.js');
const createResult = await handleCombatManage({
action: 'create',
seed: 'spawn-autoload-test',
participants: [
{ id: 'pc-hero', name: 'Hero', initiativeBonus: 5, hp: 30, maxHp: 30, isEnemy: false, position: { x: 0, y: 0 } }
]
}, ctx);
const originalId = parseResult(createResult).encounterId;

// Simulate process restart / context eviction.
getCombatManager().clear();

const spawnResult = await handleCombatManage({
action: 'spawn_quick_enemy',
encounterId: originalId,
creature: 'goblin',
count: 1
}, ctx);
const spawnData = parseResult(spawnResult);

expect(spawnData.success).toBe(true);
expect(spawnData.appendedToExisting).toBe(true);
expect(spawnData.loadedFromDb).toBe(true);
expect(spawnData.encounterId).toBe(originalId);
expect(spawnData.turnOrder.length).toBe(2);
});

// Reviewer follow-up on PR #58: when persistence fails after an
// in-memory append, we must NOT return success — that splits memory
// and DB state. Roll back the in-memory addParticipants.
it('spawn_quick_enemy rolls back in-memory append when persistence fails', async () => {
const { getCombatManager } = await import('../../../src/server/state/combat-manager.js');
const createResult = await handleCombatManage({
action: 'create',
seed: 'spawn-rollback-test',
participants: [
{ id: 'pc-hero', name: 'Hero', initiativeBonus: 5, hp: 30, maxHp: 30, isEnemy: false, position: { x: 0, y: 0 } }
]
}, ctx);
const eid = parseResult(createResult).encounterId;
const engine = getCombatManager().get(`${ctx.sessionId}:${eid}`)!;
const beforeCount = engine.getState()!.participants.length;

// Simulate persistence failure by stubbing saveState to throw.
const repoMod = await import('../../../src/storage/repos/encounter.repo.js');
const originalSave = repoMod.EncounterRepository.prototype.saveState;
repoMod.EncounterRepository.prototype.saveState = function () {
throw new Error('disk full');
};

try {
const result = await handleCombatManage({
action: 'spawn_quick_enemy',
encounterId: eid,
creature: 'goblin',
count: 1
}, ctx);
const data = parseResult(result);
expect(data.error).toBe(true);
expect(data.rolledBack).toBe(true);
expect(data.message).toMatch(/persist/i);
// In-memory state must match what it was before the attempt.
expect(engine.getState()!.participants.length).toBe(beforeCount);
} finally {
repoMod.EncounterRepository.prototype.saveState = originalSave;
}
});

// Reviewer follow-up on PR #58: when an encounterId is supplied but
// doesn't exist anywhere, return an explicit error. Silent fallback
// to creating a fresh encounter hides typos / stale ids.
it('spawn_quick_enemy errors when encounterId is unknown to memory and DB', async () => {
const spawnResult = await handleCombatManage({
action: 'spawn_quick_enemy',
encounterId: 'encounter-does-not-exist-anywhere',
creature: 'goblin',
count: 1
}, ctx);
const data = parseResult(spawnResult);
expect(data.error).toBe(true);
expect(data.message).toMatch(/not found/i);
expect(data.requestedEncounterId).toBe('encounter-does-not-exist-anywhere');
});

it('spawn_quick_enemy still creates a new encounter when encounterId is omitted', async () => {
const result = await handleCombatManage({
action: 'spawn_quick_enemy',
creature: 'goblin',
count: 1
}, ctx);
const data = parseResult(result);
expect(data.success).toBe(true);
expect(data.appendedToExisting).toBeUndefined();
expect(data.encounterId).toBeDefined();
});

it('should accept "start" alias', async () => {
const result = await handleCombatManage({
action: 'start',
Expand Down
Loading