fix(core): guard against division by zero on 100% fallout coverage in WinCheckExecution - #4891
fix(core): guard against division by zero on 100% fallout coverage in WinCheckExecution#4891berkelmali wants to merge 2 commits into
Conversation
… WinCheckExecution
WalkthroughFFA and team territory-win checks now require at least one non-fallout land tile before calculating ownership percentages. Regression tests cover all-fallout maps in both modes. ChangesTerritory Win Guard
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
the
small-fix
![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/WinCheckExecution.ts`:
- Around line 107-110: Update the denominator handling in checkWinnerTeam() and
the corresponding FFA win-check path to retain the actual non-fallout tile count
instead of clamping it with Math.max. Guard each percentage-based territory
condition so it is evaluated only when that denominator is positive, while
keeping timer conditions independent; add FFA and team coverage for zero
non-fallout tiles with positive ownership.
🪄 Autofix
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 Plus
Run ID: 84cb042c-36ac-46b2-88f1-ac55008059bc
📒 Files selected for processing (1)
src/core/execution/WinCheckExecution.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/WinCheckExecution.ts`:
- Around line 107-112: Update the territory-win comparisons in the current
execution method and checkWinnerTeam() to avoid floating-point division: compare
scaled integer tile counts using a fixed-point percentage scale, converting
percentageTilesOwnedToWin() to that scale first when fractional values are
supported. Preserve the existing positive-denominator guard and apply the
equivalent scaled comparison to max[1].
In `@tests/core/executions/WinCheckExecution.test.ts`:
- Around line 85-95: Rewrite both regression tests around checkWinnerFFA to use
setup() and the real game state instead of partial mg mocks and vi.fn methods.
Configure the simulation to retain zero non-fallout tiles while a player has
positive territory ownership, invoke the core winner check, and assert that the
real game state has no winner.
🪄 Autofix
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 Plus
Run ID: 8acbe31f-bec6-427f-bb34-30c0281a67c3
📒 Files selected for processing (2)
src/core/execution/WinCheckExecution.tstests/core/executions/WinCheckExecution.test.ts
| const numTilesWithoutFallout = | ||
| this.mg.numLandTiles() - this.mg.numTilesWithFallout(); | ||
| if ( | ||
| const isTerritoryWin = | ||
| numTilesWithoutFallout > 0 && | ||
| (max.numTilesOwned() / numTilesWithoutFallout) * 100 > | ||
| this.mg.config().percentageTilesOwnedToWin() || | ||
| this.mg.config().percentageTilesOwnedToWin(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the territory comparison in fixed-point arithmetic.
The positive-denominator guard is correct. Both changed conditions still use floating-point division. Compare scaled integer tile counts instead. If percentageTilesOwnedToWin() supports fractional percentages, store the threshold in a fixed integer scale first.
Suggested direction
- const isTerritoryWin =
- numTilesWithoutFallout > 0 &&
- (max.numTilesOwned() / numTilesWithoutFallout) * 100 >
- this.mg.config().percentageTilesOwnedToWin();
+ const percentageThreshold =
+ this.mg.config().percentageTilesOwnedToWin();
+ const isTerritoryWin =
+ numTilesWithoutFallout > 0 &&
+ max.numTilesOwned() * 100 >
+ numTilesWithoutFallout * percentageThreshold;Apply the equivalent comparison to max[1] in checkWinnerTeam().
As per coding guidelines, src/core/**/*.ts must avoid floating-point math and keep the simulation deterministic.
Also applies to: 168-173
🤖 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/WinCheckExecution.ts` around lines 107 - 112, Update the
territory-win comparisons in the current execution method and checkWinnerTeam()
to avoid floating-point division: compare scaled integer tile counts using a
fixed-point percentage scale, converting percentageTilesOwnedToWin() to that
scale first when fractional values are supported. Preserve the existing
positive-denominator guard and apply the equivalent scaled comparison to max[1].
Source: Coding guidelines
| it("should not set territory winner in FFA when non-fallout tiles is zero", () => { | ||
| const player = { | ||
| numTilesOwned: vi.fn(() => 10), | ||
| name: vi.fn(() => "P1"), | ||
| }; | ||
| mg.players = vi.fn(() => [player]); | ||
| mg.numLandTiles = vi.fn(() => 100); | ||
| mg.numTilesWithFallout = vi.fn(() => 100); | ||
| winCheck.checkWinnerFFA(); | ||
| expect(mg.setWinner).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use setup() and real game state for both regression tests.
These tests replace mg methods with vi.fn() and use a partial mock. Rewrite them with setup() and drive the core simulation directly. Keep the zero non-fallout tile and positive ownership scenario, then assert the real game has no winner.
As per coding guidelines, tests under tests/**/*.test.ts must use setup() and exercise the core simulation directly instead of using mocks.
Also applies to: 97-115
🤖 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/executions/WinCheckExecution.test.ts` around lines 85 - 95,
Rewrite both regression tests around checkWinnerFFA to use setup() and the real
game state instead of partial mg mocks and vi.fn methods. Configure the
simulation to retain zero non-fallout tiles while a player has positive
territory ownership, invoke the core winner check, and assert that the real game
state has no winner.
Source: Coding guidelines
1 participant
PR 3:
fix(core): guard against division by zero on 100% fallout coverage in WinCheckExecutionDescription:
In
WinCheckExecution.ts, bothcheckWinnerFFA()andcheckWinnerTeam()calculate non-fallout land tiles using:If 100% of the land tiles become covered in fallout (e.g. during heavy late-game nuclear strikes),
numTilesWithoutFalloutbecomes0. This caused(max.numTilesOwned() / numTilesWithoutFallout) * 100to evaluate toInfinity, triggering an instant win condition for the leading player regardless of actual tile percentage owned.This PR guards
numTilesWithoutFalloutwithMath.max(1, ...)in both FFA and Team win check methods to prevent division by zero (Infinity).Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires