Skip to content

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

Merged
Mnehmos merged 4 commits intomainfrom
fix/47-participant-ac-field
Apr 20, 2026
Merged

fix(combat): participant ac field propagates through to attack resolver#57
Mnehmos merged 4 commits intomainfrom
fix/47-participant-ac-field

Conversation

@Mnehmos
Copy link
Copy Markdown
Owner

@Mnehmos Mnehmos commented Apr 18, 2026

Summary

Caller-supplied ac on participants was silently dropped by the consolidated combat_manage schema and never reached CombatParticipant. Attacks resolved against AC 10 regardless of the value passed.

Fix

  • ac?: number added to CombatParticipant interface.
  • Accepted on both ParticipantSchema (consolidated) and the inner create_encounter input schema.
  • Participant builder reads p.ac, with caller values overriding creature-preset AC when both are present.
  • No engine change: target.ac was already the preferred source in the attack handler — this PR just ensures the value is actually set.

Test plan

  • Regression test asserts ac: 18 and ac: 11 round-trip through to encounter state.
  • combat-manage suite: 22/22 pass
  • combat-action suite: 25/25 pass
  • Full suite: 1890 passed, 6 skipped, 0 failed

Closes #47

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Participants may include an optional Armor Class (AC); supplied AC overrides derived AC and is persisted. Participant damage modifiers (resistances, vulnerabilities, immunities), attack bonus, and attack damage are persisted on creation.
  • Bug Fixes
    • Ensures supplied AC and combat stats are retained after encounter creation and load.
  • Tests
    • Added regression tests verifying persistence and returned participant AC and damage modifiers.
  • Documentation
    • Clarified AC semantics and fallback behavior.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 18, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Optional ac was added to participant types and schemas; the create-encounter handler now accepts and persists caller-supplied ac (overriding derived fallback), token damage modifier arrays were added to the token schema and persisted at create time, and tests verify these fields survive a create → load round-trip.

Changes

Cohort / File(s) Summary
Engine Interface
src/engine/combat/engine.ts
Expanded inline doc for CombatParticipant.ac?: number to state it is consumed by the attack resolver and that omission triggers the legacy fallback (10 + initiativeBonus/2).
Schema
src/schema/encounter.ts
Added optional persistence fields to TokenSchema: resistances, vulnerabilities, immunities (arrays of strings).
Consolidated API Schema
src/server/consolidated/combat-manage.ts
Extended internal ParticipantSchema to accept optional ac (z.number().int().min(0).optional()) with descriptive text about fallback derivation.
Handler Logic & Persistence
src/server/handlers/combat-handlers.ts
create_encounter input validation accepts optional ac; handleCreateEncounter conditionally sets ac from input (if provided) and persists ac, attackDamage, attackBonus, and damage modifier arrays (resistances,vulnerabilities,immunities) during initial repo.create.
Consolidated Management Tests
tests/server/consolidated/combat-manage.test.ts
Added regression tests asserting supplied participant ac and damage modifier arrays are present in the create response and after an immediate repo.loadState round-trip.

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)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Poem

🐰 I hopped through schemas, code, and tests tonight,
I tucked each AC snug so attacks find true sight,
No longer defaulted to ten in the fray,
Armor stays with its owner, come battle or play,
A joyful nibble — carrots for all, huzzah! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: the PR fixes participant ac field propagation to the attack resolver, which is the core objective.
Linked Issues check ✅ Passed All acceptance criteria from #47 are met: ac field accepted via schema [combat-manage.ts, combat-handlers.ts], attack resolver reads target AC from participant [engine.ts], and regression tests verify correct hit/miss behavior with AC 18 [combat-manage.test.ts].
Out of Scope Changes check ✅ Passed All changes directly support AC field propagation. The resistance/vulnerability/immunity field additions to TokenSchema [encounter.ts] are necessary for participant stat persistence and directly support the PR objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/47-participant-ac-field

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 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".

Comment thread src/engine/combat/engine.ts Outdated
* Armor Class. Used by attack resolution. If omitted, the engine falls
* back to a derived value (10 + initiativeBonus/2) for legacy compatibility.
*/
ac?: number;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2d9ddb9 and 8c196d8.

📒 Files selected for processing (4)
  • src/engine/combat/engine.ts
  • src/server/consolidated/combat-manage.ts
  • src/server/handlers/combat-handlers.ts
  • tests/server/consolidated/combat-manage.test.ts

Comment thread src/engine/combat/engine.ts Outdated
Mnehmos added a commit that referenced this pull request Apr 19, 2026
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]>
Mnehmos added a commit that referenced this pull request Apr 19, 2026
… 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]>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between a811352 and cf2cc2c.

📒 Files selected for processing (2)
  • src/server/handlers/combat-handlers.ts
  • tests/server/consolidated/combat-manage.test.ts
✅ Files skipped from review due to trivial changes (1)
  • tests/server/consolidated/combat-manage.test.ts

Comment thread src/server/handlers/combat-handlers.ts
Mnehmos added a commit that referenced this pull request Apr 19, 2026
…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]>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 | 🟠 Major

Initial encounter persistence still drops terrain.

handleCreateEncounter() puts parsed.terrain onto state, but the first repo.create() snapshot never writes terrain. A create → load before the next saveState() will therefore lose obstacles/difficult terrain/water and change movement/pathing. Cross-file note: the encounter terrain schema also needs a water field, 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 TerrainSchema to retain water during 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

📥 Commits

Reviewing files that changed from the base of the PR and between cf2cc2c and d37446b.

📒 Files selected for processing (3)
  • src/schema/encounter.ts
  • src/server/handlers/combat-handlers.ts
  • tests/server/consolidated/combat-manage.test.ts
✅ Files skipped from review due to trivial changes (1)
  • tests/server/consolidated/combat-manage.test.ts

Comment on lines +1104 to +1107
...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 } : {})
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

🧩 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.

Mnehmos and others added 4 commits April 20, 2026 07:30
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]>
@Mnehmos Mnehmos force-pushed the fix/47-participant-ac-field branch from d37446b to 4fbc410 Compare April 20, 2026 14:31
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between d37446b and 4fbc410.

📒 Files selected for processing (5)
  • src/engine/combat/engine.ts
  • src/schema/encounter.ts
  • src/server/consolidated/combat-manage.ts
  • src/server/handlers/combat-handlers.ts
  • tests/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

Comment on lines +43 to +44
ac: z.number().int().min(0).optional()
.describe('Armor Class. If omitted, falls back to attacker-side derivation.'),
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 | 🟡 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.

@Mnehmos Mnehmos merged commit ece90b6 into main Apr 20, 2026
4 checks passed
@Mnehmos Mnehmos deleted the fix/47-participant-ac-field branch April 20, 2026 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

combat_manage create: participant "ac" field ignored, all attacks resolve vs AC 10

1 participant