fix(core): gate all client spawn intents on the spawn phase (follow-up to #4657)#4660
fix(core): gate all client spawn intents on the spawn phase (follow-up to #4657)#4660iiamlewis wants to merge 2 commits into
Conversation
…p to #4657) #4657 blocked an already-spawned player from re-spawning after the spawn phase (the teleport exploit), but a player could still spawn for the first time once the phase had ended. Tighten the rule so any client "spawn" intent is rejected once the spawn phase is over — a player can neither spawn nor re-spawn mid-game. A naive `!inSpawnPhase()` gate would break internal spawns: nations, bots, and random-spawn placement queue a SpawnExecution during the spawn phase, but it can execute a tick later (a singleplayer human's own spawn ends the phase early). So SpawnExecution takes a `fromIntent` flag — true only for client "spawn" intents (ExecutionManager) — and the phase gate applies to those alone. Trusted internal spawns are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughSpawn executions now distinguish client spawn intents from internal spawns. Client intents are blocked after the spawn phase, while trusted internal spawns remain allowed. Tests cover relocation prevention, repeated spawning, never-spawned players, execution-manager wiring, and random spawn behavior. ChangesSpawn intent gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionManager
participant SpawnExecution
participant GameState
Client->>ExecutionManager: Submit spawn intent
ExecutionManager->>SpawnExecution: Create with fromIntent=true
SpawnExecution->>GameState: Check spawn phase
SpawnExecution->>GameState: Apply or ignore spawn
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 73-78: Add a focused test for the stamped "spawn" intent branch in
ExecutionManager.createExec, verifying it returns a SpawnExecution with
fromIntent set to true and passes the expected game, player, and tile data. Keep
the existing direct SpawnExecution tests 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e79eefcf-e848-4f41-b83f-c06b314c8ba1
📒 Files selected for processing (3)
src/core/execution/ExecutionManager.tssrc/core/execution/SpawnExecution.tstests/core/execution/SpawnExecution.test.ts
| return new SpawnExecution( | ||
| this.gameID, | ||
| player.info(), | ||
| intent.tile, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any existing test exercising Executor.createExec with a "spawn" intent.
rg -nP -C3 '\bcreateExec(s)?\s*\(' --type=ts -g '*.test.ts'
rg -nP -C3 '"spawn"' --type=ts -g '*.test.ts'Repository: openfrontio/OpenFrontIO
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Relevant files\n'
git ls-files 'src/core/execution/*' 'tests/core/execution/*' 'tests/core/*' | sed -n '1,200p'
printf '\n## Find Executor.createExec and spawn handling\n'
rg -n -C 3 'createExec|spawn|fromIntent' src/core/execution tests/core/execution tests/core --type ts
printf '\n## Show ExecutionManager.ts around spawn branch\n'
sed -n '1,140p' src/core/execution/ExecutionManager.ts
printf '\n## Show SpawnExecution tests\n'
sed -n '1,220p' tests/core/execution/SpawnExecution.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file list ---'
git ls-files 'src/core/execution/*' 'tests/core/execution/*' 'tests/core/*' | sed -n '1,200p'
echo
echo '--- createExec / spawn / fromIntent references ---'
rg -n -C 3 'createExec|spawn|fromIntent' src/core/execution tests/core/execution tests/core --type ts || true
echo
echo '--- ExecutionManager.ts ---'
sed -n '1,160p' src/core/execution/ExecutionManager.ts
echo
echo '--- SpawnExecution.test.ts ---'
sed -n '1,240p' tests/core/execution/SpawnExecution.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests mentioning ExecutionManager / StampedIntent / createExec ---'
rg -n -C 2 'ExecutionManager|StampedIntent|createExec|createExecs|fromIntent' tests src/core --type ts | sed -n '1,220p'
echo
echo '--- ExecutionManager.ts spawn branch only ---'
sed -n '45,82p' src/core/execution/ExecutionManager.ts
echo
echo '--- SpawnExecution.test.ts spawn-intent tests only ---'
sed -n '120,250p' tests/core/execution/SpawnExecution.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 11590
Add a small test for the "spawn" intent path.
tests/core/execution/SpawnExecution.test.ts covers fromIntent = true by building SpawnExecution directly, but it does not exercise ExecutionManager.createExec for a stamped "spawn" intent. Add one test there so this wiring cannot regress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/execution/ExecutionManager.ts` around lines 73 - 78, Add a focused
test for the stamped "spawn" intent branch in ExecutionManager.createExec,
verifying it returns a SpawnExecution with fromIntent set to true and passes the
expected game, player, and tile data. Keep the existing direct SpawnExecution
tests unchanged.
Source: Coding guidelines
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Add a focused test that Executor.createExec builds a SpawnExecution for a "spawn" intent, carries the intent tile, and — because it originated from a client intent — is a no-op when executed after the spawn phase. Guards against the fromIntent flag being dropped from the ExecutionManager wiring. Addresses review feedback on #4660. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 33-35: Update the assertion for playerByClientID("client_id") so
it first verifies that the returned player exists, then separately asserts that
the existing player's hasSpawned() value is false. Remove the nullish fallback
to ensure a missing player causes the test to fail.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4658dafa-1b40-4346-a543-bace938429a8
📒 Files selected for processing (1)
tests/core/execution/ExecutionManager.test.ts
| expect(game.playerByClientID("client_id")?.hasSpawned() ?? false).toBe( | ||
| false, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not mask a missing player in this assertion.
?? false makes the test pass when playerByClientID("client_id") returns undefined, so it may not verify the intended player at all. Assert that the player exists, then check hasSpawned().
Suggested fix
- expect(game.playerByClientID("client_id")?.hasSpawned() ?? false).toBe(
- false,
- );
+ const player = game.playerByClientID("client_id");
+ expect(player).toBeDefined();
+ expect(player!.hasSpawned()).toBe(false);📝 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.
| expect(game.playerByClientID("client_id")?.hasSpawned() ?? false).toBe( | |
| false, | |
| ); | |
| const player = game.playerByClientID("client_id"); | |
| expect(player).toBeDefined(); | |
| expect(player!.hasSpawned()).toBe(false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/execution/ExecutionManager.test.ts` around lines 33 - 35, Update
the assertion for playerByClientID("client_id") so it first verifies that the
returned player exists, then separately asserts that the existing player's
hasSpawned() value is false. Remove the nullish fallback to ensure a missing
player causes the test to fail.
1 participant
Follow-up to #4657.
Problem
#4657 blocked an already-spawned player from re-spawning after the spawn phase (the teleport exploit), using
!inSpawnPhase() && player.hasSpawned(). But that still lets a never-spawned player spawn for the first time once the phase has ended — spawning fresh, mid-game, at a chosen tile.The rule should be: once the spawn phase is over, a player can neither spawn nor re-spawn.
Why not just
!inSpawnPhase()A naive gate on
!inSpawnPhase()alone regresses legitimate spawning. Nations, bots, and random-spawn placement queue aSpawnExecutionduring the spawn phase, but it can execute a tick or two later — e.g. a singleplayer human's own spawn callsendSpawnPhase()immediately, so AI entities that spawn afterwards would be wrongly rejected. (NationExecutiononly enqueues its spawn whileinSpawnPhase(), then it lands on a later tick.)Fix
SpawnExecutiontakes afromIntentflag, set true only for clientspawnintents (inExecutionManager). The phase gate applies to those alone:Every internal construction (
NationExecution,TribeSpawner,PlayerSpawner) leavesfromIntentfalse and is unaffected.Tests
tests/core/execution/SpawnExecution.test.ts:Full suite green (2094 tests), including the singleplayer nation-spawn ordering tests.
🤖 Generated with Claude Code