fix(combat): surface off-turn advisory when actorId isn't on turn#59
fix(combat): surface off-turn advisory when actorId isn't on turn#59
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAn off-turn action advisory and optional lair-action flag were added to combat handling. When an action is executed by a non-active participant (including special handling for an initiative-20 LAIR slot), a warning line is appended to the action response output; actions are not rejected. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
combat_action accepts any actorId and resolves the action regardless of whose turn it is. In the live session this let Marisol attack during Brokk's turn and then attack again on her own turn — two actions for one turn slot. Rather than hard-fail (which would retroactively break 39 existing tests that pre-assume initiative order), this PR emits an `off_turn_action: <actorId> acting during <name>'s turn` warning in the response text. Production callers see the misuse, while existing test flows remain intact. The guard only fires when the supplied actorId is a real participant — unknown actors still produce the specific downstream error. Follow-up (not in this PR): promote to a hard reject once the test fixtures that depend on initiative-order guessing are updated. Closes #49 Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56dc58e88e
ℹ️ 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".
| const activeId = liveState.turnOrder[liveState.currentTurnIndex]; | ||
| if ( | ||
| activeId && | ||
| activeId !== 'LAIR' && |
There was a problem hiding this comment.
Remove LAIR bypass from off-turn warning check
The new activeId !== 'LAIR' condition suppresses the advisory during initiative-20 lair turns, so in encounters with hasLairActions a participant can still execute combat_action out of turn without any off_turn_action signal. That recreates the silent misuse path this patch is trying to expose (the actor can still act again on their normal turn later in the round), just in lair-enabled combats.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/server/consolidated/combat-action.test.ts (1)
106-118: Make the off-turn test independent of assumed initiative order.Line 106-Line 108 hard-codes that
hero-1is on turn. Prefer deriving the current-turn actor from encounter state and selecting the opposite actor dynamically, so this regression test stays stable if initiative mechanics evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/consolidated/combat-action.test.ts` around lines 106 - 118, The test currently assumes hero-1 is on turn; change it to read the encounter's current actor from the encounter state and pick the other actor dynamically before calling handleCombatAction. Specifically, fetch the encounter by testEncounterId (using the same test helpers/context as other tests), get encounter.currentActor or equivalent field, compute offTurnActor = currentActor === 'hero-1' ? 'goblin-1' : 'hero-1', and pass offTurnActor as actorId (and the opposite as targetId) into handleCombatAction so the expect on result.content[0].text.match(/off_turn_action/) remains valid regardless of initiative ordering.
🤖 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 1254-1264: The current check excludes LAIR turns from off-turn
detection by requiring activeId !== 'LAIR'; remove that clause so actions during
a LAIR turn are treated the same as other turns. In the block using activeId,
parsed.actorId, liveState.participants and turnWarning, delete the `activeId !==
'LAIR'` condition and keep the existing actor lookup/optional name fallback
(actor?.name ?? activeId) to safely format the advisory for LAIR as well as
normal participants.
---
Nitpick comments:
In `@tests/server/consolidated/combat-action.test.ts`:
- Around line 106-118: The test currently assumes hero-1 is on turn; change it
to read the encounter's current actor from the encounter state and pick the
other actor dynamically before calling handleCombatAction. Specifically, fetch
the encounter by testEncounterId (using the same test helpers/context as other
tests), get encounter.currentActor or equivalent field, compute offTurnActor =
currentActor === 'hero-1' ? 'goblin-1' : 'hero-1', and pass offTurnActor as
actorId (and the opposite as targetId) into handleCombatAction so the expect on
result.content[0].text.match(/off_turn_action/) remains valid regardless of
initiative ordering.
🪄 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: f8130432-1fa7-4213-b110-8007128635b1
📒 Files selected for processing (2)
src/server/handlers/combat-handlers.tstests/server/consolidated/combat-action.test.ts
Reviewers (PR #59) flagged that the activeId !== 'LAIR' bypass let a participant act mid-LAIR-turn without any off_turn signal — defeating the very misuse this PR was meant to expose. - Drop the LAIR bypass; treat LAIR as a non-participant turn slot. - When the active slot is LAIR, the warning labels it 'LAIR action's turn' so callers know to use the lair_action tool instead. - Threads hasLairActions through the inner create_encounter schema and the participant builder so the regression test can construct a LAIR-bearing encounter. - Test forces currentTurnIndex to the LAIR slot and asserts a PC attack still produces an off_turn_action warning. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/server/handlers/combat-handlers.ts (1)
1245-1251: Keep the advisory comment aligned with supported actions.Line 1246-Line 1248 mentions
dash/dodge/help/ready, but those actions are not currently accepted by the schema enum. This is just comment drift, but it can mislead future edits.✏️ Suggested comment-only cleanup
- // handler (attack / cast_spell / move / dash / dodge / help / heal / - // disengage / ready) is an on-turn action. If actorId doesn't match the + // handler (attack / cast_spell / move / heal / disengage) is an on-turn + // action. If actorId doesn't match the🤖 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 1245 - 1251, Update the advisory comment in src/server/handlers/combat-handlers.ts so it accurately lists only the actions currently accepted by the schema enum for the on-turn handler (replace the outdated "dash/dodge/help/ready" entries). Locate the comment block above the on-turn action handler (the turn-identity advisory referencing attack / cast_spell / move / dash / dodge / help / heal / disengage / ready) and remove or replace the unsupported actions with the exact action names present in the schema enum, keeping the rest of the advisory text (warning instead of throwing) unchanged.
🤖 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`:
- Line 1099: The TokenSchema is missing hasLairActions and the repo.create()
token mapping omits it, so persists never store that flag; add hasLairActions:
z.boolean().optional() to the TokenSchema definition and include hasLairActions
in the token fields passed to repo.create() (the same object where other token
props are mapped) so that participants loaded later can be found by the existing
participants.find((p: any) => p.hasLairActions) lookup and lair action turn
order is preserved.
---
Nitpick comments:
In `@src/server/handlers/combat-handlers.ts`:
- Around line 1245-1251: Update the advisory comment in
src/server/handlers/combat-handlers.ts so it accurately lists only the actions
currently accepted by the schema enum for the on-turn handler (replace the
outdated "dash/dodge/help/ready" entries). Locate the comment block above the
on-turn action handler (the turn-identity advisory referencing attack /
cast_spell / move / dash / dodge / help / heal / disengage / ready) and remove
or replace the unsupported actions with the exact action names present in the
schema enum, keeping the rest of the advisory text (warning instead of throwing)
unchanged.
🪄 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: bcb9e739-235a-4733-a868-5c94adbac5d7
📒 Files selected for processing (2)
src/server/handlers/combat-handlers.tstests/server/consolidated/combat-action.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/server/consolidated/combat-action.test.ts
| initiative: 0, // Will be rolled | ||
| initiativeBonus: p.initiativeBonus ?? 0, | ||
| isEnemy: p.isEnemy ?? false, | ||
| hasLairActions: p.hasLairActions ?? false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate encounter repository implementation:"
fd -i 'encounter.repo.ts' src
echo
echo "Inspect create/load/save mappings for lair-related fields:"
rg -n -C4 'create\\(|loadState\\(|saveState\\(|hasLairActions|lairOwnerId|turnOrder|activeTokenId|tokens' src/server src/storageRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, check the file size and read lines around 1099 and 1131-1145
echo "=== Reading combat-handlers.ts around line 1099 ==="
wc -l src/server/handlers/combat-handlers.ts
echo
echo "=== Context around line 1099 ==="
sed -n '1090,1110p' src/server/handlers/combat-handlers.ts
echo
echo "=== Context around lines 1131-1145 (persistence mapping) ==="
sed -n '1125,1150p' src/server/handlers/combat-handlers.tsRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 2048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the encounter repository to understand persistence
echo "=== Reading encounter.repo.ts ==="
wc -l src/storage/repos/encounter.repo.ts
echo
echo "=== Search for create method and persistence mappings ==="
rg -n "create\(" src/storage/repos/encounter.repo.ts -A 20
echo
echo "=== Search for loadState method ==="
rg -n "loadState" src/storage/repos/encounter.repo.ts -A 15
echo
echo "=== Search for hasLairActions in storage layer ==="
rg -n "hasLairActions" src/storage src/serverRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 3477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full loadState method in encounter.repo.ts ==="
sed -n '130,190p' src/storage/repos/encounter.repo.ts
echo
echo "=== Check token schema/interface definition ==="
rg -n "interface.*Token|type.*Token" src/storage src/server --type-list | head -20
rg -n "tokens:" src/storage/repos/encounter.repo.ts -B5 -A5Repository: Mnehmos/mnehmos.rpg.mcp
Length of output: 2904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify what token fields are actually expected in schema ==="
rg -n "EncounterSchema\|TokenSchema" src/storage -A 30 | head -60Repository: Mnehmos/mnehmos.rpg.mcp
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for token/encounter schema definitions ==="
rg -n "type.*Token|interface.*Token|Encounter.*=" src --type ts -A 10 | head -80
echo
echo "=== Look for Zod schema definitions ==="
rg -n "z\.object|EncounterSchema\|TokenSchema" src/storage -A 20 | head -100
echo
echo "=== Check what CombatParticipant interface includes ==="
rg -n "interface CombatParticipant|type CombatParticipant" src -A 20Repository: Mnehmos/mnehmos.rpg.mcp
Length of output: 6078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read full TokenSchema definition ==="
sed -n '80,120p' src/schema/encounter.ts
echo
echo "=== Read full EncounterSchema ==="
sed -n '140,165p' src/schema/encounter.ts
echo
echo "=== Check if hasLairActions is in TokenSchema ==="
rg -n "hasLairActions" src/schema/encounter.tsRepository: Mnehmos/mnehmos.rpg.mcp
Length of output: 2470
Add hasLairActions to token persistence mapping.
Line 1099 sets hasLairActions on in-memory participants, but the TokenSchema (src/schema/encounter.ts) does not define this field, and the create-time persistence mapping (lines 1131–1145) omits it. When loadState() reloads the encounter, it searches for participants.find((p: any) => p.hasLairActions) (encounter.repo.ts:160), which will always be undefined since the field was never persisted. This causes lair turn order to be lost across session reloads, breaking lair action functionality.
Add hasLairActions: z.boolean().optional() to TokenSchema and include it in the token persistence mapping in the repo.create() call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/handlers/combat-handlers.ts` at line 1099, The TokenSchema is
missing hasLairActions and the repo.create() token mapping omits it, so persists
never store that flag; add hasLairActions: z.boolean().optional() to the
TokenSchema definition and include hasLairActions in the token fields passed to
repo.create() (the same object where other token props are mapped) so that
participants loaded later can be found by the existing participants.find((p:
any) => p.hasLairActions) lookup and lair action turn order is preserved.
…trip Reviewers (PR #59) caught that hasLairActions was set on participants but never persisted. TokenSchema didn't define it and repo.create()'s token mapping omitted it. After a restart, encounter.repo.loadState searched for participants.find(p => p.hasLairActions) and always got undefined, dropping the LAIR slot from turnOrder and silently disabling the lair-action mechanic. - Add hasLairActions: z.boolean().optional() to TokenSchema. - Include hasLairActions in repo.create() token mapping. - Regression test creates a LAIR-bearing encounter, reloads via EncounterRepository.loadState, and asserts both that turnOrder still contains 'LAIR' and that the bearer's hasLairActions flag survived. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@coderabbitai both findings are stale regenerations against the same commit that addresses them — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 56245
My earlier comments were stale — apologies for the noise. I'll resolve them now. [resolve] |
Summary
combat_actionresolved actions regardless of whose turn it was — letting a caller fire an attack as an off-turn participant, then fire again when their real turn comes up. Live session observed Marisol attacking twice in a round.Approach
A hard reject broke 39 existing tests that rely on initiative-order guessing (they'd need individual fixes). This PR chooses the smaller-blast-radius path: emit an
off_turn_action: <actorId> acting during <name>'s turnadvisory in the response. Production callers see the misuse; existing flows remain intact.Scope
actorIdis a real participant that is not the current-turn participant.Future work
Promote to a hard reject once the test fixtures that depend on initiative-order guessing are fixed. Tracked implicitly — if the team wants, I can open a follow-up.
Test plan
off_turn_actiontext.Closes #49
🤖 Generated with Claude Code
Summary by CodeRabbit