Skip to content

fix(combat): participant ac field propagates through to attack resolver #57

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): participant ac field propagates through to attack resolver #57

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
e7b452a
fix(combat): participant ac field propagates through to attack resolver
Mnehmos Apr 18, 2026
3262e07
fix(combat): remove duplicate ac field on CombatParticipant
Mnehmos Apr 19, 2026
d8c41b1
fix(combat): persist ac/attackDamage/attackBonus on initial encounter…
Mnehmos Apr 19, 2026
4fbc410
fix(combat): persist resistances/vulnerabilities/immunities on initia…
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
6 changes: 5 additions & 1 deletion src/engine/combat/engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ export interface CombatParticipant {
isStabilized?: boolean; // Unconscious but won't die
isDead?: boolean; // Permanently defeated
// COMBAT STATS (Auto-resolution)
ac?: number; // Armor Class
/**
* Armor Class. Used by the attack resolver. If omitted, the engine falls
* back to a derived value (10 + initiativeBonus/2) for legacy compatibility.
*/
ac?: number;
attackDamage?: string; // Default attack damage (e.g., "1d6+2")
attackBonus?: number; // Default attack bonus used if none provided
}
Expand Down
7 changes: 6 additions & 1 deletion src/schema/encounter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@ export const TokenSchema = z.object({
attackBonus: z.number().optional().describe('Default attack bonus'),
// Lair-action ownership — must be persisted so loadState can rebuild the
// LAIR slot in turnOrder (see encounter.repo.ts loadState lookup).
hasLairActions: z.boolean().optional().describe('Whether this token owns lair actions')
hasLairActions: z.boolean().optional().describe('Whether this token owns lair actions'),
// Damage modifiers (HIGH-002) — must persist so post-load attack resolution
// continues to honor immunities/resistances/vulnerabilities.
resistances: z.array(z.string()).optional().describe('Damage types dealt at half damage'),
vulnerabilities: z.array(z.string()).optional().describe('Damage types dealt at double damage'),
immunities: z.array(z.string()).optional().describe('Damage types ignored entirely')
});

export type Token = z.infer<typeof TokenSchema>;
Expand Down
2 changes: 2 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 @@ -40,6 +40,8 @@ const ParticipantSchema = z.object({
initiativeBonus: z.number().int(),
hp: z.number().int().nonnegative(), // Allow 0 HP for dying characters
maxHp: z.number().int().positive(),
ac: z.number().int().min(0).optional()
.describe('Armor Class. If omitted, falls back to attacker-side derivation.'),
Comment on lines +43 to +44
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Disallow ac: 0 at validation time.

Line 43 currently accepts 0, but the combat resolver treats dc === 0 as “no AC provided”. That makes ac: 0 behave inconsistently with the rest of the pipeline. Please tighten this to min(1) here and mirror the same bound in the matching create_encounter and encounter token schemas.

Suggested fix 
-    ac: z.number().int().min(0).optional()
+    ac: z.number().int().min(1).optional()
         .describe('Armor Class. If omitted, falls back to attacker-side derivation.'),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ac: z.number().int().min(0).optional()
.describe('Armor Class. If omitted, falls back to attacker-side derivation.'),
ac: z.number().int().min(1).optional()
.describe('Armor Class. If omitted, falls back to attacker-side derivation.'),
🤖 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 43 - 44, The schema
for the AC field currently allows 0 via ac: z.number().int().min(0).optional(),
but 0 is treated as “no AC provided” elsewhere; update the validation to
disallow 0 by changing the constraint to min(1) (i.e., ac:
z.number().int().min(1).optional()) in the combat resolver schema (the ac
property in the schema defined in combat-manage.ts) and apply the identical
change to the corresponding AC fields in the create_encounter and encounter
token schemas so all validation consistently requires AC >= 1 when provided.

isEnemy: z.boolean().optional(),
/**
* Convenience alias for `isEnemy`. Values "enemy" / "hostile" map to
Expand Down
20 changes: 18 additions & 2 deletions src/server/handlers/combat-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,8 @@ Example (use real UUID from context for player character!):
isEnemy: z.boolean().optional().describe('Whether this is an enemy (auto-detected if not set)'),
hasLairActions: z.boolean().optional()
.describe('Adds a LAIR slot at initiative 20 to the turn order'),
ac: z.number().int().min(0).optional()
.describe('Armor Class (used by attack resolution; defaults to attacker-side derivation if omitted)'),
conditions: z.array(z.string()).default([]),
position: z.object({ x: z.number(), y: z.number(), z: z.number().optional() }).optional()
.describe('CRIT-003: Spatial position for movement (x, y coordinates)'),
Expand Down Expand Up @@ -1102,7 +1104,10 @@ export async function handleCreateEncounter(args: unknown, ctx: SessionContext)
resistances: p.resistances,
vulnerabilities: p.vulnerabilities,
immunities: p.immunities,
...extraStats
...extraStats,
// Caller-supplied AC wins over the preset's default so explicit
// overrides (e.g., a goblin in chain mail) take effect.
...(p.ac !== undefined ? { ac: p.ac } : {})
Comment on lines +1107 to +1110
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 19, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect every engine attack-resolution path and how target AC is chosen.
rg -n -C3 "targetAC|executeOpportunityAttack|autoResolveAttack|target\.ac" src/engine/combat/engine.ts

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 1705

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check the structure/type of target object to confirm ac property exists
echo "=== Looking for target object type definition and ac property ==="
rg -n "interface.*Target|type.*Target|\.ac\s*:" src/engine/combat/ --max-count=20

echo ""
echo "=== Search for all targetAC assignments in engine ==="
rg -n "targetAC\s*=" src/engine/combat/

echo ""
echo "=== Check for other auto-resolve or attack patterns ==="
rg -n "autoResolve|auto-resolve|Resolve.*[Aa]ttack" src/engine/combat/

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 132

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search for Participant type definition ==="
rg -n "interface Participant|type Participant|class Participant" --type ts

echo ""
echo "=== Get full executeOpportunityAttack function context ==="
sed -n '1322,1395p' src/engine/combat/engine.ts

echo ""
echo "=== Search for all targetAC or similar AC computation ==="
rg -n "targetAC|target\.ac" src/engine/combat/engine.ts

echo ""
echo "=== Search for other attack methods ==="
rg -n "executeAttack|resolveAttack|checkAttack" src/engine/combat/engine.ts

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 113

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== List repository structure ==="
ls -la src/ 2>/dev/null || echo "src/ not found"
ls -la src/engine/ 2>/dev/null || echo "src/engine/ not found"
ls -la src/engine/combat/ 2>/dev/null || echo "src/engine/combat/ not found"

echo ""
echo "=== Find all TypeScript files in the repo ==="
find . -name "*.ts" -type f | head -20

echo ""
echo "=== Check if combat-handlers.ts exists ==="
find . -name "combat-handlers.ts" -type f

echo ""
echo "=== Check if engine.ts exists ==="
find . -name "engine.ts" -type f

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 2495

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Get executeOpportunityAttack function and surrounding context ==="
sed -n '1319,1400p' src/engine/combat/engine.ts

echo ""
echo "=== Search for all mentions of targetAC in engine.ts ==="
grep -n "targetAC" src/engine/combat/engine.ts

echo ""
echo "=== Check if target object has an ac property being used elsewhere ==="
grep -n "target\.ac" src/engine/combat/engine.ts

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 3951

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check combat-handlers.ts at lines 1104-1107 to verify ac assignment ==="
sed -n '1100,1110p' src/server/handlers/combat-handlers.ts

echo ""
echo "=== Search for Participant type/interface in entire codebase ==="
grep -rn "interface Participant\|type Participant\|class.*Participant" src/ --include="*.ts" | head -20

echo ""
echo "=== Search for where ac property is defined or used in any file ==="
grep -rn "\.ac\s*=" src/ --include="*.ts" | head -30

echo ""
echo "=== Check if there are other attack resolution methods ==="
grep -n "handle.*[Aa]ttack\|execute.*[Aa]ttack\|resolve.*[Aa]ttack" src/engine/combat/engine.ts

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 666

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search for CombatParticipant type definition ==="
grep -rn "interface CombatParticipant\|type CombatParticipant\|class CombatParticipant" src/ --include="*.ts"

echo ""
echo "=== Search for where CombatParticipant is defined ==="
grep -rn "CombatParticipant" src/ --include="*.ts" -l

echo ""
echo "=== Check engine.ts imports and type definitions at top ==="
head -100 src/engine/combat/engine.ts | grep -n "interface\|type\|import\|CombatParticipant"

echo ""
echo "=== Check if other handler methods use target.ac ==="
grep -n "\.ac" src/server/handlers/combat-handlers.ts | head -20

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 2304

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Get full CombatParticipant interface definition ==="
sed -n '22,80p' src/engine/combat/engine.ts

echo ""
echo "=== Get context around line 1315-1316 in combat-handlers.ts to see how normal attacks use target.ac ==="
sed -n '1310,1325p' src/server/handlers/combat-handlers.ts

echo ""
echo "=== Check if there are other attack methods besides executeOpportunityAttack ==="
grep -n "execute.*[Aa]ttack\|resolve.*[Aa]ttack" src/engine/combat/engine.ts

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 3933

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Get executeAttack method to see how it handles target.ac ==="
sed -n '550,650p' src/engine/combat/engine.ts

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 4344

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search for all methods that compute AC/DC and might have the same issue ==="
grep -n "10 + \|targetAC\|derive.*ac\|ac.*=" src/engine/combat/engine.ts | head -30

echo ""
echo "=== Get full context of any other attack methods that might ignore target.ac ==="
grep -B5 -A15 "const.*AC.*=" src/engine/combat/engine.ts | head -80

Repository: Mnehmos/mnehmos.rpg.mcp

Length of output: 3171

Fix executeOpportunityAttack to respect caller-supplied AC.

The opportunity attack path still derives targetAC from initiativeBonus instead of checking target.ac first. While the handler correctly propagates and sets ac on participants (lines 1104–1107), and the normal attack flow respects it (lines 1315–1316 in handlers), executeOpportunityAttack at line 1340 ignores it, creating an inconsistency:

-const targetAC = 10 + (target.initiativeBonus > 0 ? Math.floor(target.initiativeBonus / 2) : 0);
+const targetAC =
+    target.ac ??
+    (10 + (target.initiativeBonus > 0 ? Math.floor(target.initiativeBonus / 2) : 0));

This ensures opportunity attacks use the same AC preference logic as regular attacks.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/server/handlers/combat-handlers.ts` around lines 1104 - 1107,
executeOpportunityAttack computes targetAC from initiativeBonus instead of
honoring a caller-supplied target.ac; update the target AC resolution inside
executeOpportunityAttack to first use target.ac if defined (the same preference
used when spreading ...(p.ac !== undefined ? { ac: p.ac } : {}) in participant
construction and as used in the normal attack flow), and only fall back to the
current initiativeBonus-derived calculation when target.ac is undefined.

} as CombatParticipant;

return participant;
Expand All @@ -1125,7 +1130,10 @@ export async function handleCreateEncounter(args: unknown, ctx: SessionContext)
const db = getDb(process.env.NODE_ENV === 'test' ? ':memory:' : 'rpg.db');
const repo = new EncounterRepository(db);

// Create the encounter record first (with initiative and isEnemy)
// Create the encounter record first (with initiative and isEnemy).
// PR #57 follow-up: persist ac/attackDamage/attackBonus too — those drive
// attack resolution, and omitting them on initial create made loadState()
// (before any saveState) drop back to the default-AC-10 fallback.
repo.create({
id: encounterId,
tokens: state.participants.map(p => ({
Expand All @@ -1139,6 +1147,14 @@ export async function handleCreateEncounter(args: unknown, ctx: SessionContext)
maxHp: p.maxHp,
conditions: p.conditions,
abilityScores: p.abilityScores,
// Combat stats used by the attack resolver
ac: p.ac,
attackDamage: p.attackDamage,
attackBonus: p.attackBonus,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Damage modifiers — drop them and post-load attacks lose half/2x/immune behavior
resistances: p.resistances,
vulnerabilities: p.vulnerabilities,
immunities: p.immunities,
// Spatial visualization data
position: p.position,
movementSpeed: p.movementSpeed ?? 30,
Expand Down
74 changes: 74 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 @@ -166,6 +166,80 @@ describe('combat_manage consolidated tool', () => {
const data = parseResult(result);
expect(data.success).toBe(true);
});

// PR #57 follow-up: damage modifiers must also survive the initial
// create -> loadState cycle. Dropping resistances/immunities/etc.
// changes damage resolution after a cold load.
it('persists resistances/immunities/vulnerabilities into the initial encounter row', async () => {
const { EncounterRepository } = await import('../../../src/storage/repos/encounter.repo.js');
const createResult = await handleCombatManage({
action: 'create',
seed: 'damage-mods-cold-load',
participants: [
{
id: 'fire-elem',
name: 'Fire Elemental',
initiativeBonus: 0,
hp: 30,
maxHp: 30,
isEnemy: true,
resistances: ['bludgeoning', 'piercing', 'slashing'],
immunities: ['fire'],
vulnerabilities: ['cold']
}
]
}, ctx);
const encounterId = parseResult(createResult).encounterId;

const repo = new EncounterRepository(getDb(':memory:'));
const loaded = repo.loadState(encounterId);
const elem = loaded.participants.find((p: { id: string }) => p.id === 'fire-elem');
expect(elem?.resistances).toEqual(['bludgeoning', 'piercing', 'slashing']);
expect(elem?.immunities).toEqual(['fire']);
expect(elem?.vulnerabilities).toEqual(['cold']);
});

// PR #57 follow-up: ensure ac survives an initial create -> loadState
// round-trip even before any saveState() is called.
it('persists ac into the initial encounter row (no loss on cold load)', async () => {
const { EncounterRepository } = await import('../../../src/storage/repos/encounter.repo.js');
const createResult = await handleCombatManage({
action: 'create',
seed: 'ac-persistence-cold-load',
participants: [
{ id: 'tanky', name: 'Tank', initiativeBonus: 0, hp: 30, maxHp: 30, ac: 18, isEnemy: false }
]
}, ctx);
const encounterId = parseResult(createResult).encounterId;

const repo = new EncounterRepository(getDb(':memory:'));
const loaded = repo.loadState(encounterId);
expect(loaded).not.toBeNull();
const tank = loaded.participants.find((p: { id: string; ac?: number }) => p.id === 'tanky');
expect(tank?.ac).toBe(18);
});

// Regression for issue #47: participant `ac` was being silently dropped
// by the consolidated schema and never reached the attack resolver. All
// attacks resolved vs AC 10 regardless of the supplied value.
it('honors participant `ac` in encounter state', async () => {
const result = await handleCombatManage({
action: 'create',
seed: 'ac-persistence-test',
participants: [
{ id: 'tanky-pc', name: 'Tank', initiativeBonus: 0, hp: 38, maxHp: 38, ac: 18, isEnemy: false, position: { x: 0, y: 0 } },
{ id: 'squishy-enemy', name: 'Bandit', initiativeBonus: 0, hp: 10, maxHp: 10, ac: 11, isEnemy: true, position: { x: 1, y: 0 } }
]
}, ctx);
const data = parseResult(result);
expect(data.success).toBe(true);

const byId = Object.fromEntries(
(data.participants as Array<{ id: string; ac?: number }>).map((p) => [p.id, p.ac])
);
expect(byId['tanky-pc']).toBe(18);
expect(byId['squishy-enemy']).toBe(11);
});
});

describe('get action', () => {
Expand Down
Loading