fix(combat): participant ac field propagates through to attack resolver#57
fix(combat): participant ac field propagates through to attack resolver#57
ac field propagates through to attack resolver#57Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOptional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Engine
participant Repo
Client->>Handler: POST create encounter (participants [+ optional ac, modifiers])
Handler->>Engine: derive stats (initiative, defaults)
Handler->>Handler: if p.ac defined -> override derived AC
Handler->>Repo: repo.create(encounter with persisted ac, attackBonus, attackDamage, resistances/vulnerabilities/immunities)
Repo-->>Handler: created encounter id / stored rows
Handler-->>Client: return created encounter (participants include ac & modifiers)
Client->>Repo: repo.loadState(encounterId)
Repo-->>Client: stored encounter state (participant.ac and modifiers)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c196d8dd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Armor Class. Used by attack resolution. If omitted, the engine falls | ||
| * back to a derived value (10 + initiativeBonus/2) for legacy compatibility. | ||
| */ | ||
| ac?: number; |
There was a problem hiding this comment.
Remove duplicate
ac field in CombatParticipant
Adding ac?: number here introduces a duplicate property name in the same CombatParticipant interface (there is already an ac declaration in the // COMBAT STATS section below), which causes TypeScript to fail with TS2300: Duplicate identifier 'ac' during tsc --noEmit. Any CI or local workflow that runs type-checking will fail until one of the declarations is removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engine/combat/engine.ts`:
- Around line 30-34: The CombatParticipant interface currently declares the
optional property ac?: number twice, causing a TS2300 duplicate identifier
error; remove the redundant second declaration so only the documented ac?:
number (the one that explains the legacy fallback) remains in the
CombatParticipant interface and verify there are no other duplicate ac
declarations in that interface or nearby types.
In `@src/server/handlers/combat-handlers.ts`:
- Around line 1104-1107: The repo.create token mapping is missing the combat
fields added to state.participants (ac, attackDamage, attackBonus), causing the
initial persisted record to lack those values; update the token object built for
repo.create (the mapping that converts state.participants into the token passed
to repo.create) to include p.ac, p.attackDamage, and p.attackBonus (respecting
the existing override behavior where present) so the initial DB record matches
the in-memory participant shape used by combat resolution and subsequent
saveState().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fce6117a-d81a-47f5-abd8-c550e33b0d39
📒 Files selected for processing (4)
src/engine/combat/engine.tssrc/server/consolidated/combat-manage.tssrc/server/handlers/combat-handlers.tstests/server/consolidated/combat-manage.test.ts
Reviewers (PR #57) caught that this branch declared ac?: number twice on CombatParticipant — once in my new addition near the top of the interface and once in the existing COMBAT STATS section. Even though tsc happened to compile, that's a TS2300 risk and confuses readers. Drops the duplicate at the top, keeps the existing one in COMBAT STATS, and lifts the doc comment so the meaning isn't lost. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
… create Reviewers (PR #57) caught that repo.create()'s token mapping dropped ac/attackDamage/attackBonus, so loadState() on a freshly-created encounter (before any saveState) would serve a tokenized state without combat stats — the attack resolver would then fall back to AC 10 and heuristic damage. TokenSchema already accepts the fields (ac/attackDamage/attackBonus were present), this just wires them into the create path. Adds a cold-load test that creates an encounter and immediately reads it back through EncounterRepository.loadState to confirm ac survives. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/handlers/combat-handlers.ts`:
- Around line 1146-1149: The initial token snapshot only persists ac,
attackDamage, and attackBonus; add the missing combat modifier fields
resistances, vulnerabilities, and immunities to the projection that builds the
participant/token snapshot (the same place where ac/attackDamage/attackBonus are
set in combat-handlers.ts) so they survive a create→load cycle, and update the
encounter token schema/validation used by the repository to accept those three
fields as valid properties so they are not stripped during save/load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81aa9c0b-87b3-4829-8f3a-5139cb6ba6f1
📒 Files selected for processing (2)
src/server/handlers/combat-handlers.tstests/server/consolidated/combat-manage.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/server/consolidated/combat-manage.test.ts
…l create Reviewers (PR #57 round 3) caught the same persistence omission for damage modifiers as for combat stats — repo.create()'s token mapping dropped resistances/vulnerabilities/immunities, so a cold load (create -> loadState before saveState) would lose half/double/immune behavior on the next attack. - Add the three fields to TokenSchema (encounter.ts). - Include them in repo.create()'s token mapping. - Cold-load regression test asserts a fire elemental's resistance/immunity/vulnerability triples survive load. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/handlers/combat-handlers.ts (1)
1134-1163:⚠️ Potential issue | 🟠 MajorInitial encounter persistence still drops terrain.
handleCreateEncounter()putsparsed.terrainontostate, but the firstrepo.create()snapshot never writesterrain. A create → load before the nextsaveState()will therefore lose obstacles/difficult terrain/water and change movement/pathing. Cross-file note: the encounter terrain schema also needs awaterfield, otherwise that property will still be stripped during validation.💡 Proposed fix
repo.create({ id: encounterId, tokens: state.participants.map(p => ({ id: p.id, name: p.name, initiativeBonus: p.initiativeBonus, initiative: p.initiative, // Store rolled initiative isEnemy: p.isEnemy, // Store enemy flag hp: p.hp, 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, // 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, size: p.size ?? 'medium' })), round: state.round, activeTokenId: state.turnOrder[state.currentTurnIndex], status: 'active', + terrain: state.terrain, createdAt: new Date().toISOString(), updatedAt: new Date().toISOString() });And extend
TerrainSchemato retainwaterduring repository validation.🤖 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 1134 - 1163, The initial encounter snapshot created in handleCreateEncounter omits terrain, so include the terrain from state (e.g., parsed.terrain/state.terrain) in the repo.create payload alongside round/activeTokenId/etc. to ensure obstacles/difficult terrain/water are persisted on first save, and update the TerrainSchema used by the repository validation to include a water field (and any related subfields) so the validation does not strip the water property on create/load; reference handleCreateEncounter and TerrainSchema when making these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/handlers/combat-handlers.ts`:
- Around line 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.
---
Outside diff comments:
In `@src/server/handlers/combat-handlers.ts`:
- Around line 1134-1163: The initial encounter snapshot created in
handleCreateEncounter omits terrain, so include the terrain from state (e.g.,
parsed.terrain/state.terrain) in the repo.create payload alongside
round/activeTokenId/etc. to ensure obstacles/difficult terrain/water are
persisted on first save, and update the TerrainSchema used by the repository
validation to include a water field (and any related subfields) so the
validation does not strip the water property on create/load; reference
handleCreateEncounter and TerrainSchema when making these edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e822c94-d65d-4fc1-825d-c02053ac2d54
📒 Files selected for processing (3)
src/schema/encounter.tssrc/server/handlers/combat-handlers.tstests/server/consolidated/combat-manage.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/server/consolidated/combat-manage.test.ts
| ...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 } : {}) |
There was a problem hiding this comment.
🧩 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.tsRepository: 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.tsRepository: 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 fRepository: 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.tsRepository: 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.tsRepository: 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 -20Repository: 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.tsRepository: 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.tsRepository: 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 -80Repository: 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.
The consolidated ParticipantSchema lacked an `ac` field, so Zod dropped any caller-supplied AC. Even when it reached the internal handleCreateEncounter, the participant-construction site didn't carry `p.ac` into the CombatParticipant record. Result: every attack resolved vs AC 10 (the attacker-side fallback). - Add `ac` to CombatParticipant interface (optional). - Accept `ac` on both the consolidated ParticipantSchema and the inner create_encounter schema. - Propagate `p.ac` through the participant builder, letting caller values override the creature-preset AC when both are supplied. - Attack resolver already reads `target.ac` first, so no engine change needed — this fix just gets the value there. Closes #47 Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Reviewers (PR #57) caught that this branch declared ac?: number twice on CombatParticipant — once in my new addition near the top of the interface and once in the existing COMBAT STATS section. Even though tsc happened to compile, that's a TS2300 risk and confuses readers. Drops the duplicate at the top, keeps the existing one in COMBAT STATS, and lifts the doc comment so the meaning isn't lost. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
… create Reviewers (PR #57) caught that repo.create()'s token mapping dropped ac/attackDamage/attackBonus, so loadState() on a freshly-created encounter (before any saveState) would serve a tokenized state without combat stats — the attack resolver would then fall back to AC 10 and heuristic damage. TokenSchema already accepts the fields (ac/attackDamage/attackBonus were present), this just wires them into the create path. Adds a cold-load test that creates an encounter and immediately reads it back through EncounterRepository.loadState to confirm ac survives. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…l create Reviewers (PR #57 round 3) caught the same persistence omission for damage modifiers as for combat stats — repo.create()'s token mapping dropped resistances/vulnerabilities/immunities, so a cold load (create -> loadState before saveState) would lose half/double/immune behavior on the next attack. - Add the three fields to TokenSchema (encounter.ts). - Include them in repo.create()'s token mapping. - Cold-load regression test asserts a fire elemental's resistance/immunity/vulnerability triples survive load. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
d37446b to
4fbc410
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/consolidated/combat-manage.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: def8c050-8089-4e52-a9c0-9ebf92e6fcac
📒 Files selected for processing (5)
src/engine/combat/engine.tssrc/schema/encounter.tssrc/server/consolidated/combat-manage.tssrc/server/handlers/combat-handlers.tstests/server/consolidated/combat-manage.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/server/consolidated/combat-manage.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/engine/combat/engine.ts
- src/schema/encounter.ts
| ac: z.number().int().min(0).optional() | ||
| .describe('Armor Class. If omitted, falls back to attacker-side derivation.'), |
There was a problem hiding this comment.
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.
| 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.
Summary
Caller-supplied
acon participants was silently dropped by the consolidated combat_manage schema and never reachedCombatParticipant. Attacks resolved against AC 10 regardless of the value passed.Fix
ac?: numberadded toCombatParticipantinterface.ParticipantSchema(consolidated) and the innercreate_encounterinput schema.p.ac, with caller values overriding creature-preset AC when both are present.target.acwas already the preferred source in the attack handler — this PR just ensures the value is actually set.Test plan
ac: 18andac: 11round-trip through to encounter state.Closes #47
🤖 Generated with Claude Code
Summary by CodeRabbit