Skip to content

Fix/double tap full send #4397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Katokoda wants to merge 7 commits into from
Closed

Fix/double tap full send #4397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f4c07cb
Makes tests self-contained
Katokoda Jun 17, 2026
799ee08
The execution Manager now checks for multiple troop-ratio-intents for…
Katokoda Jun 18, 2026
18ffe46
Return to previous donate-troop behavior
Katokoda Jun 19, 2026
4275211
Lowers quantity of maps check and separates the ratio computation in …
Katokoda Jun 23, 2026
100aa73
lint
Katokoda Jun 24, 2026
90f2375
Adds tests for the new Execution Manager Merging Behavior
Katokoda Jun 24, 2026
2ef3c05
format
Katokoda Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/client/ClientGameRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,8 @@ export class ClientGameRunner {
this.eventBus.emit(
new SendAttackIntentEvent(
this.gameView.owner(tile).id(),
this.myPlayer!.troops() * this.renderer.uiState.attackRatio,
this.renderer.uiState.attackRatio,
this.myPlayer!.troops(),
),
);
} else if (this.canAutoBoat(actions.buildableUnits, tile)) {
Expand Down Expand Up @@ -1102,7 +1103,8 @@ export class ClientGameRunner {
this.eventBus.emit(
new SendAttackIntentEvent(
this.gameView.owner(tile).id(),
this.myPlayer!.troops() * this.renderer.uiState.attackRatio,
this.renderer.uiState.attackRatio,
this.myPlayer!.troops(),
),
);
}
Expand Down Expand Up @@ -1136,12 +1138,14 @@ export class ClientGameRunner {
mostRecentAttack.attackerID,
) as PlayerView;
if (!attacker) return;

const counterTroops = Math.min(
mostRecentAttack.troops,
this.renderer.uiState.attackRatio * this.myPlayer.troops(),
this.eventBus.emit(
new SendAttackIntentEvent(
attacker.id(),
this.renderer.uiState.attackRatio,
this.myPlayer.troops(),
mostRecentAttack.troops,
),
);
this.eventBus.emit(new SendAttackIntentEvent(attacker.id(), counterTroops));
}

private doRequestAllianceUnderCursor(): void {
Expand Down Expand Up @@ -1226,7 +1230,8 @@ export class ClientGameRunner {
this.eventBus.emit(
new SendBoatAttackIntentEvent(
tile,
this.myPlayer.troops() * this.renderer.uiState.attackRatio,
this.renderer.uiState.attackRatio,
this.myPlayer.troops(),
),
);
}
Expand Down
13 changes: 9 additions & 4 deletions src/client/Transport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,17 @@ export class SendSpawnIntentEvent implements GameEvent {
export class SendAttackIntentEvent implements GameEvent {
constructor(
public readonly targetID: PlayerID | null,
public readonly troops: number,
public readonly troopRatio: number,
public readonly troopCount: number,
public readonly maxTroopCount: number | null = null,
) {}
Comment on lines +74 to 77

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 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).

}

export class SendBoatAttackIntentEvent implements GameEvent {
constructor(
public readonly dst: TileRef,
public readonly troops: number,
public readonly troopRatio: number,
public readonly troopCount: number,
) {}
}

Expand Down Expand Up @@ -484,14 +487,16 @@ export class Transport {
this.sendIntent({
type: "attack",
targetID: event.targetID,
troops: event.troops,
troopRatio: event.troopRatio,
troopCount: event.troopCount,
});
}

private onSendBoatAttackIntent(event: SendBoatAttackIntentEvent) {
this.sendIntent({
type: "boat",
troops: event.troops,
troopRatio: event.troopRatio,
troopCount: event.troopCount,
dst: event.dst,
});
}
Expand Down
12 changes: 7 additions & 5 deletions src/client/hud/layers/AttacksDisplay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,14 @@ export class AttacksDisplay extends LitElement implements Controller {

const myPlayer = this.game.myPlayer();
if (!myPlayer) return;

const counterTroops = Math.min(
attack.troops,
this.uiState.attackRatio * myPlayer.troops(),
this.eventBus.emit(
new SendAttackIntentEvent(
attacker.id(),
this.uiState.attackRatio,
myPlayer.troops(),
attack.troops,
),
);
this.eventBus.emit(new SendAttackIntentEvent(attacker.id(), counterTroops));
}

private renderIncomingAttacks() {
Expand Down
6 changes: 4 additions & 2 deletions src/client/hud/layers/PlayerActionHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ export class PlayerActionHandler {
this.eventBus.emit(
new SendAttackIntentEvent(
targetId,
this.uiState.attackRatio * player.troops(),
this.uiState.attackRatio,
player.troops(),
),
);
}
Expand All @@ -36,7 +37,8 @@ export class PlayerActionHandler {
this.eventBus.emit(
new SendBoatAttackIntentEvent(
targetTile,
this.uiState.attackRatio * player.troops(),
this.uiState.attackRatio,
player.troops(),
),
);
}
Expand Down
8 changes: 6 additions & 2 deletions src/core/Schemas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,10 @@ export const AllianceExtensionIntentSchema = z.object({
export const AttackIntentSchema = z.object({
type: z.literal("attack"),
targetID: ID.nullable(),
troops: z.number().nonnegative().nullable(),
troopCount: z.number().nonnegative(),
troopRatio: z.number().gt(0).max(1),
// maxTroopSent present only for retaliation.
maxTroopSent: z.number().nonnegative().optional(),
Comment on lines +362 to +365

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ 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.ts

Repository: 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 -n

Repository: 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.ts

Repository: 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 -50

Repository: 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.ts

Repository: 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 -20

Repository: 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.ts

Repository: 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 ts

Repository: 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 -40

Repository: 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.

});

export const SpawnIntentSchema = z.object({
Expand All @@ -369,7 +372,8 @@ export const SpawnIntentSchema = z.object({

export const BoatAttackIntentSchema = z.object({
type: z.literal("boat"),
troops: z.number().nonnegative(),
troopCount: z.number().nonnegative(),
troopRatio: z.number().gt(0).max(1),
dst: z.number(),
});

Expand Down
63 changes: 59 additions & 4 deletions src/core/execution/ExecutionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,57 @@ export class Executor {
this.random = new PseudoRandom(simpleHash(gameID) + 1);
}

private computeRatio(
remainingTroopRatio: number,
totalRatioUsage: number,
): number {
return (1 - remainingTroopRatio) / totalRatioUsage;
}
Comment on lines +45 to +50

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 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


createExecs(turn: Turn): Execution[] {
return turn.intents.map((i) => this.createExec(i));
// In the rare case a client sends multiple troopRatio-orders,
// we need to "merge" their orders instead of executing them in parallel.
// (two 60% attacks should be one 84% attack, not one 120% attack)
// But, they may be of different types/on different targets
// (hence we do two (84/120)*60% = 42% attacks).
const remainingTroopRatio_perClientID = new Map<ClientID, number>();
const totalRatioUsage_perClientID = new Map<ClientID, number>();
for (const intent of turn.intents) {
switch (intent.type) {
case "boat":
case "attack": {
remainingTroopRatio_perClientID.set(
intent.clientID,
(remainingTroopRatio_perClientID.get(intent.clientID) ?? 1) *
(1 - intent.troopRatio),
);
totalRatioUsage_perClientID.set(
intent.clientID,
(totalRatioUsage_perClientID.get(intent.clientID) ?? 0) +
intent.troopRatio,
);
}
}
}

return turn.intents.map((intent) => {
switch (intent.type) {
case "boat":
case "attack":
return this.createExec(
intent,
this.computeRatio(
remainingTroopRatio_perClientID.get(intent.clientID)!,
totalRatioUsage_perClientID.get(intent.clientID)!,
),
);
default:
return this.createExec(intent, undefined);
}
});
}

createExec(intent: StampedIntent): Execution {
createExec(intent: StampedIntent, troopRatioFactor?: number): Execution {

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 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/ -A2

Repository: openfrontio/OpenFrontIO

Length of output: 1223

🏁 Script executed:

# Also search the entire codebase for createExec calls
rg --type ts 'createExec' -C 2

Repository: openfrontio/OpenFrontIO

Length of output: 161

🏁 Script executed:

# Read the ExecutionManager.ts file to see the complete context
cat -n src/core/execution/ExecutionManager.ts

Repository: 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.ts

Repository: 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.

const player = this.mg.playerByClientID(intent.clientID);
if (!player) {
console.warn(`player with clientID ${intent.clientID} not found`);
Expand All @@ -57,7 +103,12 @@ export class Executor {
switch (intent.type) {
case "attack": {
return new AttackExecution(
intent.troops,
Math.floor(
Math.min(
troopRatioFactor! * intent.troopRatio * intent.troopCount,
intent.maxTroopSent ?? intent.troopCount,
),
),
player,
intent.targetID,
null,
Expand All @@ -72,7 +123,11 @@ export class Executor {
case "spawn":
return new SpawnExecution(this.gameID, player.info(), intent.tile);
case "boat":
return new TransportShipExecution(player, intent.dst, intent.troops);
return new TransportShipExecution(
player,
intent.dst,
Math.floor(troopRatioFactor! * intent.troopRatio * intent.troopCount),
);
case "allianceRequest":
return new AllianceRequestExecution(player, intent.recipient);
case "allianceReject":
Expand Down
12 changes: 6 additions & 6 deletions tests/Attack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ let defender: Player;
let defenderSpawn: TileRef;
let attackerSpawn: TileRef;

function sendBoat(target: TileRef, troops: number) {
game.addExecution(new TransportShipExecution(defender, target, troops));
}

const immunityPhaseTicks = 10;
function waitForImmunityToEnd() {
for (let i = 0; i < immunityPhaseTicks + 1; i++) {
Expand Down Expand Up @@ -110,7 +106,9 @@ describe("Attack", () => {
constructionExecution(game, defender, 1, 1, UnitType.MissileSilo);
expect(defender.units(UnitType.MissileSilo)).toHaveLength(1);

sendBoat(game.ref(15, 8), 100);
game.addExecution(
new TransportShipExecution(defender, game.ref(15, 8), 100),
);

constructionExecution(game, defender, 0, 15, UnitType.AtomBomb, 3);
const nuke = defender.units(UnitType.AtomBomb)[0];
Expand All @@ -129,7 +127,9 @@ describe("Attack", () => {
const player_start_troops = defender.troops();
const boat_troops = player_start_troops * 0.5;

sendBoat(game.ref(15, 8), boat_troops);
game.addExecution(
new TransportShipExecution(defender, game.ref(15, 8), boat_troops),
);

game.executeNextTick();

Expand Down
Loading
Loading