Fix/double tap full send#4397
Conversation
… any client, and merges their troop consumption instead of adding them. Required to convey separately troopRatio and troopCount for these Intents
|
Hi @Katokoda, thanks for the contribution. This PR was automatically closed because it doesn't fit our contribution workflow:
To contribute to OpenFront:
If you believe this was closed in error, please reach out on our Discord or comment below. See CONTRIBUTING.md for the full contribution process. — Automated PR gate. Source. |
github-actions
Bot
added
the
auto-closed-needs-issue
github-project-automation
Bot
moved this from Triage
to Complete
in OpenFront Release Management
WalkthroughAttack intent payloads are restructured so clients send ChangesServer-side troop ratio computation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Transport.ts (1)
486-493: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
maxTroopCountfrom the retaliate path is dropped here.
SendAttackIntentEventcarriesmaxTroopCount(set by the retaliate path inAttacksDisplay.handleRetaliatefromattack.troops), and the schema accepts an optionalmaxTroopSent. But this serializer only sendstroopRatioandtroopCount, somaxTroopSentis alwaysundefinedon the server. The result: the retaliation troop cap never takes effect, and the server usestroopCountas the cap instead of the intendedattack.troops.Note
maxTroopCountisnumber | null, while the schema field is optional (undefined), so mapnulltoundefined.🐛 Send the cap over the wire
private onSendAttackIntent(event: SendAttackIntentEvent) { this.sendIntent({ type: "attack", targetID: event.targetID, troopRatio: event.troopRatio, troopCount: event.troopCount, + maxTroopSent: event.maxTroopCount ?? undefined, }); }🤖 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/client/Transport.ts` around lines 486 - 493, The onSendAttackIntent method is not including the maxTroopCount from SendAttackIntentEvent when constructing the intent object, causing the maxTroopSent field to always be undefined on the server. Add maxTroopCount from the event parameter to the intent object being sent via sendIntent, mapping it to the maxTroopSent field in the schema. Since maxTroopCount is number or null while the schema field is optional (undefined), convert null values to undefined when setting maxTroopSent to ensure the retaliation troop cap is properly communicated to the server.
🧹 Nitpick comments (2)
src/core/execution/ExecutionManager.ts (2)
45-50: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFloating-point math in
src/core/— confirm this stays deterministic.
computeRatio(and thefactor * troopRatio * troopCountcomputations) introduce float divide/multiply into the core simulation. JavaScript doubles are deterministic across engines, so this is likely fine in practice, but the core guideline asks to avoid floating-point math for determinism. Worth a quick confirmation that this matches the project's determinism expectations, since previously the troop total arrived pre-computed.As per coding guidelines: "Ensure deterministic behavior in
src/core/by using seeded PRNG and avoiding floating-point math".🤖 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 45 - 50, The computeRatio method and its usage with troopRatio and troopCount calculations introduce floating-point division and multiplication into the core simulation logic, which conflicts with the project's determinism guideline to avoid floating-point math in src/core/. Review whether this floating-point behavior is acceptable for the project's determinism model, and if not, refactor the computeRatio method to use integer-based or fixed-point arithmetic calculations instead of floating-point division to maintain deterministic behavior across all execution contexts.Source: Coding guidelines
95-130: 🩺 Stability & Availability | 🔵 TrivialConsider making
troopRatioFactornon-optional with a default value of1for safer public API.While
createExecis public and the non-null assertions on lines 108 and 129 assume a factor is always supplied forattackandboatintents, the current codebase only callscreateExecthroughcreateExecs, which always provides the factor for these branches. However, to prevent future fragility if someone callscreateExecdirectly, default the factor to1:Suggested improvement
- createExec(intent: StampedIntent, troopRatioFactor?: number): Execution { + createExec(intent: StampedIntent, troopRatioFactor: number = 1): Execution {Then remove the
!non-null assertions on lines 108 and 129.🤖 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 95 - 130, The createExec method has an optional troopRatioFactor parameter with non-null assertions in the attack and boat cases, which could be fragile if the method is called directly without providing a value. Change the method signature to give troopRatioFactor a default value of 1 instead of making it optional (change troopRatioFactor?: number to troopRatioFactor: number = 1). Then remove the non-null assertion operators (!) after troopRatioFactor in both the attack case and the boat case where it is used in the Math.floor calculations.
🤖 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 15-23: Replace the stub game variable declaration (currently
assigned as undefined as any) with a real game instance created using the
setup() helper function imported from tests/util/Setup.ts. In the beforeEach
hook, instead of passing the undefined stub to the Executor constructor, pass
the real game instance created by setup(). This ensures the Executor is
initialized with a genuine game state rather than a bypass stub, allowing the
test to exercise the actual simulation logic and catch potential type or
contract regressions. Also update any intent building logic in the test to use
real game state from the setup instance.
- Around line 72-77: The boat intent object in the test is using an invalid
payload shape with the field targetID, but the actual Executor.createExec method
expects a dst field instead. Replace targetID with dst in the boat intent
payload to match the actual contract. Additionally, remove the as
StampedIntent[] type cast on line 85 that is masking this payload shape
mismatch, ensuring the test properly validates that the intent object matches
the expected StampedIntent type structure without type assertions that hide
errors.
---
Outside diff comments:
In `@src/client/Transport.ts`:
- Around line 486-493: The onSendAttackIntent method is not including the
maxTroopCount from SendAttackIntentEvent when constructing the intent object,
causing the maxTroopSent field to always be undefined on the server. Add
maxTroopCount from the event parameter to the intent object being sent via
sendIntent, mapping it to the maxTroopSent field in the schema. Since
maxTroopCount is number or null while the schema field is optional (undefined),
convert null values to undefined when setting maxTroopSent to ensure the
retaliation troop cap is properly communicated to the server.
---
Nitpick comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 45-50: The computeRatio method and its usage with troopRatio and
troopCount calculations introduce floating-point division and multiplication
into the core simulation logic, which conflicts with the project's determinism
guideline to avoid floating-point math in src/core/. Review whether this
floating-point behavior is acceptable for the project's determinism model, and
if not, refactor the computeRatio method to use integer-based or fixed-point
arithmetic calculations instead of floating-point division to maintain
deterministic behavior across all execution contexts.
- Around line 95-130: The createExec method has an optional troopRatioFactor
parameter with non-null assertions in the attack and boat cases, which could be
fragile if the method is called directly without providing a value. Change the
method signature to give troopRatioFactor a default value of 1 instead of making
it optional (change troopRatioFactor?: number to troopRatioFactor: number = 1).
Then remove the non-null assertion operators (!) after troopRatioFactor in both
the attack case and the boat case where it is used in the Math.floor
calculations.
🪄 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: 5a275a38-867f-4e55-a20f-24fe19225890
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
| const game: Game = undefined as any; | ||
| let executor: Executor; | ||
| const gameID: GameID = "test_game"; | ||
| const clientID: ClientID = "test_client"; | ||
| const mockPlayer: any = 7; | ||
|
|
||
| beforeEach(() => { | ||
| executor = new Executor(game, gameID, clientID); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use setup() instead of undefined as any game stubs in tests.
Lines 15-23 and Line 31 bypass real simulation setup, so this test can miss contract/type regressions (as seen with the casted intents). Please create the game with setup() and build intents from real game state.
As per coding guidelines, tests/**/*.test.ts: Use the setup() helper from tests/util/Setup.ts to create test game instances and exercise core simulation directly.
Also applies to: 31-31
🤖 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 15 - 23, Replace
the stub game variable declaration (currently assigned as undefined as any) with
a real game instance created using the setup() helper function imported from
tests/util/Setup.ts. In the beforeEach hook, instead of passing the undefined
stub to the Executor constructor, pass the real game instance created by
setup(). This ensures the Executor is initialized with a genuine game state
rather than a bypass stub, allowing the test to exercise the actual simulation
logic and catch potential type or contract regressions. Also update any intent
building logic in the test to use real game state from the setup instance.
Source: Coding guidelines
| type: "boat", | ||
| clientID: "client3", | ||
| troopRatio: 0.1, | ||
| troopCount: 1000, | ||
| targetID: "target2", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Boat intent payload shape is invalid in this test.
Line 76 uses targetID for a "boat" intent, but Executor.createExec reads intent.dst. The as StampedIntent[] cast on Line 85 hides this mismatch and makes the test pass with an invalid contract.
Suggested fix
- {
- type: "boat",
- clientID: "client3",
- troopRatio: 0.1,
- troopCount: 1000,
- targetID: "target2",
- },
+ {
+ type: "boat",
+ clientID: "client3",
+ troopRatio: 0.1,
+ troopCount: 1000,
+ dst: /* valid TileRef from setup() game/ref helper */,
+ },Also applies to: 85-85
🤖 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 72 - 77, The boat
intent object in the test is using an invalid payload shape with the field
targetID, but the actual Executor.createExec method expects a dst field instead.
Replace targetID with dst in the boat intent payload to match the actual
contract. Additionally, remove the as StampedIntent[] type cast on line 85 that
is masking this payload shape mismatch, ensuring the test properly validates
that the intent object matches the expected StampedIntent type structure without
type assertions that hide errors.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/core/execution/ExecutionManager.test.ts (2)
15-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
setup()instead ofundefined as anyfor the game fixture.This test bypasses real simulation state, so it can miss integration regressions in
Executor.createExecs.Suggested minimal fix
+import { setup } from "../../util/Setup"; ... describe("Executor", () => { - const game: Game = undefined as any; + let game: Game; let executor: Executor; ... beforeEach(() => { + ({ game } = setup()); executor = new Executor(game, gameID, clientID); });As per coding guidelines,
tests/**/*.test.ts: "Use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly."🤖 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 15 - 23, Replace the game fixture that uses undefined as any with a proper test fixture created by the setup() helper function from tests/util/Setup.ts. In the beforeEach hook, call setup() to create a real game instance instead of using the undefined as any assignment, and assign the result to the game constant. This ensures the Executor is tested with actual simulation state and will catch integration regressions in Executor.createExecs.Source: Coding guidelines
72-77: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t hide intent-shape errors behind
as StampedIntent[].The cast can mask invalid wire-contract fields (for example boat intent field names), which weakens exactly what this test should validate.
Suggested minimal fix
- intents: [ + const intents: StampedIntent[] = [ ... - ] as StampedIntent[], + ]; + intents,Also applies to: 85-85
🤖 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 72 - 77, Remove the `as StampedIntent[]` type cast that is masking type validation errors in the test. Instead of casting the intent objects to StampedIntent[], let TypeScript's type system properly validate that each intent object has the correct field names and structure. This way, invalid field names (like incorrect boat intent fields) will surface as compilation errors rather than being silently accepted by the cast, ensuring the test properly validates the wire-contract structure.
🤖 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/client/Transport.ts`:
- Around line 74-77: The maxTroopCount parameter in the constructor is not being
serialized when building the attack intent, which causes retaliate actions to
lose the troop cap constraint from incoming attacks. Add serialization for the
maxTroopCount field to the attack intent serialization logic, ensuring the field
is only included in the output when it has a non-null value. Apply the same fix
to the similar code also mentioned in the comment (at lines 486-492).
In `@src/core/execution/ExecutionManager.ts`:
- Around line 45-50: The computeRatio method and related troop calculations at
lines 106-111 and 126-130 use floating-point arithmetic in the core execution
logic, which conflicts with determinism requirements. Refactor these
calculations to use integer arithmetic instead, or if floating-point operations
are necessary, ensure they are immediately floored to integers before any
comparisons or storage. Verify that no float intermediate values are persisted
in state or used in conditional logic, and confirm all values passed to the
simulation are integers. Check the troopRatioFactor multiplication with
intent.troopRatio and intent.troopCount to ensure the results are floored and
only the integer output reaches downstream simulation logic.
- Line 95: The createExec method has an optional troopRatioFactor parameter that
is required for attack and boat intent types, but the non-null assertions on
troopRatioFactor in the attack branch (line 108) and boat branch (line 129)
could fail silently if called directly without this parameter. Add a runtime
guard at the beginning of the createExec method that checks if troopRatioFactor
is undefined when the intent type is attack or boat, and throw a descriptive
error in that case. This will prevent NaN values from flowing through the troop
calculation logic and alert developers to the missing required parameter.
In `@src/core/Schemas.ts`:
- Around line 362-365: The troopCount and maxTroopSent fields in the attack
intent schemas currently allow fractional values, but troop counts must be
discrete integers. Update the troopCount field in the schema around line 362 to
use .int().nonnegative() instead of just .nonnegative(), and apply the same
change to the maxTroopSent field in the same schema block. Then apply identical
changes to the corresponding troopCount and maxTroopSent fields in the
BoatAttackIntentSchema around line 375 to enforce integer validation
consistently across all attack intent schemas.
---
Duplicate comments:
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 15-23: Replace the game fixture that uses undefined as any with a
proper test fixture created by the setup() helper function from
tests/util/Setup.ts. In the beforeEach hook, call setup() to create a real game
instance instead of using the undefined as any assignment, and assign the result
to the game constant. This ensures the Executor is tested with actual simulation
state and will catch integration regressions in Executor.createExecs.
- Around line 72-77: Remove the `as StampedIntent[]` type cast that is masking
type validation errors in the test. Instead of casting the intent objects to
StampedIntent[], let TypeScript's type system properly validate that each intent
object has the correct field names and structure. This way, invalid field names
(like incorrect boat intent fields) will surface as compilation errors rather
than being silently accepted by the cast, ensuring the test properly validates
the wire-contract structure.
🪄 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: 36bf7100-6ee9-479f-bdd3-c32544e07e42
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
| public readonly troopRatio: number, | ||
| public readonly troopCount: number, | ||
| public readonly maxTroopCount: number | null = null, | ||
| ) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward the retaliation cap in the attack intent.
The fourth constructor argument is never serialized, so retaliate actions drop the maxTroopSent cap and can send more troops than the incoming attack allows. Match the schema field and only include it when present.
Proposed fix
- public readonly maxTroopCount: number | null = null,
+ public readonly maxTroopSent: number | null = null, this.sendIntent({
type: "attack",
targetID: event.targetID,
troopRatio: event.troopRatio,
troopCount: event.troopCount,
+ ...(event.maxTroopSent === null
+ ? {}
+ : { maxTroopSent: event.maxTroopSent }),
});Also applies to: 486-492
🤖 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/client/Transport.ts` around lines 74 - 77, The maxTroopCount parameter in
the constructor is not being serialized when building the attack intent, which
causes retaliate actions to lose the troop cap constraint from incoming attacks.
Add serialization for the maxTroopCount field to the attack intent serialization
logic, ensuring the field is only included in the output when it has a non-null
value. Apply the same fix to the similar code also mentioned in the comment (at
lines 486-492).
| private computeRatio( | ||
| remainingTroopRatio: number, | ||
| totalRatioUsage: number, | ||
| ): number { | ||
| return (1 - remainingTroopRatio) / totalRatioUsage; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Floating-point math in deterministic core.
computeRatio and the troop formulas (troopRatioFactor * intent.troopRatio * intent.troopCount) introduce float division and multiplication inside src/core/. The results are floored to integers, and JS number arithmetic is itself deterministic, so the practical desync risk is low. Still, this conflicts with the core determinism rule, so please confirm the floored output is the only value that reaches the simulation and that no float intermediate is stored or compared.
As per coding guidelines: "Ensure deterministic behavior in src/core/ by using seeded PRNG and avoiding floating-point math".
Also applies to: 106-111, 126-130
🤖 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 45 - 50, The
computeRatio method and related troop calculations at lines 106-111 and 126-130
use floating-point arithmetic in the core execution logic, which conflicts with
determinism requirements. Refactor these calculations to use integer arithmetic
instead, or if floating-point operations are necessary, ensure they are
immediately floored to integers before any comparisons or storage. Verify that
no float intermediate values are persisted in state or used in conditional
logic, and confirm all values passed to the simulation are integers. Check the
troopRatioFactor multiplication with intent.troopRatio and intent.troopCount to
ensure the results are floored and only the integer output reaches downstream
simulation logic.
Source: Coding guidelines
| } | ||
|
|
||
| createExec(intent: StampedIntent): Execution { | ||
| createExec(intent: StampedIntent, troopRatioFactor?: number): Execution { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of createExec to confirm attack/boat callers always pass a factor.
rg -nP --type=ts -C3 '\.createExec\s*\('Repository: openfrontio/OpenFrontIO
Length of output: 161
🏁 Script executed:
# First, let's verify the file exists and get its structure
ls -la src/core/execution/ExecutionManager.ts
# Use simpler rg syntax to find createExec calls (avoid -P flag)
rg --type ts 'createExec' src/core/execution/ -A2Repository: openfrontio/OpenFrontIO
Length of output: 1223
🏁 Script executed:
# Also search the entire codebase for createExec calls
rg --type ts 'createExec' -C 2Repository: openfrontio/OpenFrontIO
Length of output: 161
🏁 Script executed:
# Read the ExecutionManager.ts file to see the complete context
cat -n src/core/execution/ExecutionManager.tsRepository: openfrontio/OpenFrontIO
Length of output: 8918
🏁 Script executed:
# Search more broadly for createExec calls across the entire repo
find . -name "*.ts" -type f -exec grep -l "createExec" {} \;Repository: openfrontio/OpenFrontIO
Length of output: 275
🏁 Script executed:
# Check GameRunner.ts for how it calls createExec
cat -n src/core/GameRunner.ts | grep -A 5 -B 5 "createExec"Repository: openfrontio/OpenFrontIO
Length of output: 501
🏁 Script executed:
# Also check the test file to understand expected usage patterns
cat -n tests/core/execution/ExecutionManager.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 5696
Guard against direct calls to createExec with attack/boat intents but missing troopRatioFactor.
The method is public and troopRatioFactor is optional, but the attack (line 108) and boat (line 129) branches use troopRatioFactor! with non-null assertions. Currently, all code paths are safe: createExecs always computes and passes the factor for attack/boat intents, and passes undefined only for other intent types. However, the design is fragile—a developer calling createExec directly with an attack/boat intent and no factor would receive undefined * troopRatio * troopCount = NaN, flowing into Math.floor(NaN) as NaN troops.
Either add a runtime guard (throw error if troopRatioFactor is undefined for these types) or use a narrower type at the call site to make the requirement clear.
🤖 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` at line 95, The createExec method has
an optional troopRatioFactor parameter that is required for attack and boat
intent types, but the non-null assertions on troopRatioFactor in the attack
branch (line 108) and boat branch (line 129) could fail silently if called
directly without this parameter. Add a runtime guard at the beginning of the
createExec method that checks if troopRatioFactor is undefined when the intent
type is attack or boat, and throw a descriptive error in that case. This will
prevent NaN values from flowing through the troop calculation logic and alert
developers to the missing required parameter.
| troopCount: z.number().nonnegative(), | ||
| troopRatio: z.number().gt(0).max(1), | ||
| // maxTroopSent present only for retaliation. | ||
| maxTroopSent: z.number().nonnegative().optional(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, find and inspect the relevant file
fd "Schemas.ts" src/core/
# Then read the specific lines mentioned in the review
sed -n '360,380p' src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 881
🏁 Script executed:
# Check if there are other numeric troop/unit count fields in the schemas file
rg "troopCount|maxTroopSent|soldierCount|unitCount" src/core/Schemas.ts -n -A 1 -B 1
# Also search for similar patterns in other intent schemas to see if there's a pattern
rg "z\.number\(\)\.nonnegative" src/core/Schemas.ts -nRepository: openfrontio/OpenFrontIO
Length of output: 851
🏁 Script executed:
# Let's search for any existing use of .int() in the schemas to see if it's used elsewhere
rg "\.int\(\)" src/core/Schemas.ts -n
# Also check the imports to understand what version of Zod is being used
head -30 src/core/Schemas.tsRepository: openfrontio/OpenFrontIO
Length of output: 1311
🏁 Script executed:
# Check the context around the troop intent schemas to understand their purpose
sed -n '355,380p' src/core/Schemas.ts
# Also search for where troopCount is used to see if it's ever expected to be fractional
rg "troopCount" src/core/ -t ts --no-heading -B 2 -A 2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 3645
🏁 Script executed:
# Let's check how these values are actually used in the execution to confirm they should be integers
sed -n '1,50p' src/core/execution/ExecutionManager.tsRepository: openfrontio/OpenFrontIO
Length of output: 2469
🏁 Script executed:
# Let's verify the actual calculation to see if fractional troop counts would make sense
sed -n '100,120p' src/core/execution/ExecutionManager.ts | grep -A 30 "troopCount"
# Also check if there are any tests for these intent schemas
fd ".*test.*" src/core/ -t f | head -20Repository: openfrontio/OpenFrontIO
Length of output: 578
🏁 Script executed:
# Let's look at the full context of how the intent is processed to understand data flow
sed -n '140,180p' src/core/execution/ExecutionManager.tsRepository: openfrontio/OpenFrontIO
Length of output: 1611
🏁 Script executed:
# Let's find the attack case handler
sed -n '100,150p' src/core/execution/ExecutionManager.ts | grep -B 20 -A 20 "case \"attack\""Repository: openfrontio/OpenFrontIO
Length of output: 940
🏁 Script executed:
# Check if there are any other usages of Math.floor on troopCount calculations
rg "Math\.floor.*troopCount|troopCount.*Math\.floor" src/core/ -t tsRepository: openfrontio/OpenFrontIO
Length of output: 281
🏁 Script executed:
# Let's check if there's any documentation or comments about why troopCount might be fractional
rg "troopCount|troop.*count" src/core/Schemas.ts -B 3 -A 3
# Also search for any boat attack intent usage
rg "BoatAttackIntent" src/core/ -t ts -B 2 -A 2 | head -40Repository: openfrontio/OpenFrontIO
Length of output: 1747
Validate troop counts as safe integers in attack intents.
Troop counts are discrete units, not fractional quantities. troopCount and maxTroopSent should use .int().nonnegative() to enforce this at the wire boundary. This matches the pattern already used elsewhere in the schemas file for similar count fields.
Proposed schema changes
troopCount: z.number().nonnegative(),
+ troopCount: z.number().int().nonnegative(),
troopRatio: z.number().gt(0).max(1),
// maxTroopSent present only for retaliation.
maxTroopSent: z.number().nonnegative().optional(),
+ maxTroopSent: z.number().int().nonnegative().optional(),Apply the same change to BoatAttackIntentSchema (line 375):
troopCount: z.number().nonnegative(),
+ troopCount: z.number().int().nonnegative(),🤖 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/Schemas.ts` around lines 362 - 365, The troopCount and maxTroopSent
fields in the attack intent schemas currently allow fractional values, but troop
counts must be discrete integers. Update the troopCount field in the schema
around line 362 to use .int().nonnegative() instead of just .nonnegative(), and
apply the same change to the maxTroopSent field in the same schema block. Then
apply identical changes to the corresponding troopCount and maxTroopSent fields
in the BoatAttackIntentSchema around line 375 to enforce integer validation
consistently across all attack intent schemas.
|
I panicked and created a new PR (#4398), because this auto-closed and I could not trigger reevaluation of the issue reference, and I did not manage to add commits to it. |
1 participant
Add approved & assigned issue number here:
Resolves #4237
Description:
The PR changes the Intent Events structure using the attack-ratio to compute sent troop such that it do send to the Execution Manager both the attack ratio and the total troopCount to ratio from.
Then, the Execution Manager collects all those numbers for each ClientID sending many of those intents events in the same tick, and adjust the computation such that the total sent number of troop is equal (up to roundings) to the total troops that would be sent if the attacks where computed one after the other (the order is not important because of math).
More precisely, the "Remaining troops" after attacks of ratio 0.6, 0.3 and 0.2 is the multiplication of their non-attacks ratio, 0.40.70.8 = 0.224. The actual attack ratio for each of these same-tick attacks is then adapted accordingly BUT in a way that do not depend at all from their order or size, because the reduction is applied uniformingly.
Also, this PR adds a new test that tests various situations and compares them with the mocked-up still buggy version.
Please complete the following:
I also tested that the game is yes fixed by forcing the radial-menu attack to always send two attacks (easy way to reproduce the bug) and confirmed it is fixed.
Please put your Discord username so you can be contacted if a bug or regression is found:
Katokoda