fix: traitor bug when attacking immediately after initiating an alliance (#2044)

This PR fixes a critical race condition bug where players could
unintentionally receive the traitor debuff when alliance requests were
accepted mid-attack.

Critical Bug Fixes #1866

**Root Cause:**
Players could bypass UI alliance checks ( isFriendly() ) by accepting
alliances and immediately attacking after that, causing the server to
treat the attack as betrayal
Solution: Added server-side alliance validation in
AttackExecution.init()
This ensures attacks on allies are blocked at the server level.

- Once Bots and Nations decide to attack, they breaks the alliance. I
added maybeConsiderBetrayal(), which currently always returns true. I’ll
add proper logic for alliance-breaking soon on another PR; this didn’t
exist in the code before.

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced
regression is found:

abodcraft1

---------

Co-authored-by: evanpelle <evanpelle@gmail.com>

src/core/execution/AttackExecution.ts

@@ -16,8 +16,6 @@ import { FlatBinaryHeap } from "./utils/FlatBinaryHeap"; // adjust path if neede
 
 const malusForRetreat = 25;
 export class AttackExecution implements Execution {
-  private breakAlliance = false;
-  private wasAlliedAtInit = false; // Store alliance state at initialization
   private active: boolean = true;
   private toConquer = new FlatBinaryHeap();
 
@@ -62,6 +60,24 @@ export class AttackExecution implements Execution {
         ? mg.terraNullius()
         : mg.player(this._targetID);
 
+    if (this._owner === this.target) {
+      console.error(`Player ${this._owner} cannot attack itself`);
+      this.active = false;
+      return;
+    }
+
+    // ALLIANCE CHECK — block attacks on friendly (ally or same team)
+    if (this.target.isPlayer()) {
+      const targetPlayer = this.target as Player;
+      if (this._owner.isFriendly(targetPlayer)) {
+        console.warn(
+          `${this._owner.displayName()} cannot attack ${targetPlayer.displayName()} because they are friendly (allied or same team)`,
+        );
+        this.active = false;
+        return;
+      }
+    }
+
     if (this.target && this.target.isPlayer()) {
       const targetPlayer = this.target as Player;
       if (
@@ -70,15 +86,10 @@ export class AttackExecution implements Execution {
       ) {
         // Don't let bots embargo since they can't trade anyway.
         targetPlayer.addEmbargo(this._owner, true);
+        this.rejectIncomingAllianceRequests(targetPlayer);
       }
     }
 
-    if (this._owner === this.target) {
-      console.error(`Player ${this._owner} cannot attack itself`);
-      this.active = false;
-      return;
-    }
-
     if (this.target.isPlayer()) {
       if (
         this.mg.config().numSpawnPhaseTurns() +
@@ -148,11 +159,6 @@ export class AttackExecution implements Execution {
     }
 
     if (this.target.isPlayer()) {
-      // Store the alliance state at initialization time to prevent race conditions
-      this.wasAlliedAtInit = this._owner.isAlliedWith(this.target);
-      if (this.wasAlliedAtInit) {
-        this.breakAlliance = true;
-      }
       this.target.updateRelation(this._owner, -80);
     }
   }
@@ -221,20 +227,8 @@ export class AttackExecution implements Execution {
       return;
     }
 
-    const alliance = targetPlayer
-      ? this._owner.allianceWith(targetPlayer)
-      : null;
-    if (this.breakAlliance && alliance !== null) {
-      this.breakAlliance = false;
-      this._owner.breakAlliance(alliance);
-    }
-    if (
-      targetPlayer &&
-      this._owner.isAlliedWith(targetPlayer) &&
-      !this.wasAlliedAtInit
-    ) {
+    if (targetPlayer && this._owner.isFriendly(targetPlayer)) {
       // In this case a new alliance was created AFTER the attack started.
-      // We should retreat to avoid the attacker becoming a traitor.
       this.retreat();
       return;
     }
@@ -295,6 +289,15 @@ export class AttackExecution implements Execution {
     }
   }
 
+  private rejectIncomingAllianceRequests(target: Player) {
+    const request = this._owner
+      .incomingAllianceRequests()
+      .find((ar) => ar.requestor() === target);
+    if (request !== undefined) {
+      request.reject();
+    }
+  }
+
   private addNeighbors(tile: TileRef) {
     if (this.attack === null) {
       throw new Error("Attack not initialized");

src/core/execution/BotExecution.ts

@@ -69,6 +69,13 @@ export class BotExecution implements Execution {
     if (toAttack !== null) {
       const odds = this.bot.isFriendly(toAttack) ? 6 : 3;
       if (this.random.chance(odds)) {
+        // Check and break alliance before attacking if needed
+        const alliance = this.bot.allianceWith(toAttack);
+
+        if (alliance !== null) {
+          this.bot.breakAlliance(alliance);
+        }
+
         this.behavior.sendAttack(toAttack);
         return;
       }

src/core/execution/FakeHumanExecution.ts

@@ -161,6 +161,30 @@ export class FakeHumanExecution implements Execution {
     this.maybeAttack();
   }
 
+  /**
+   * TODO: Implement strategic betrayal logic
+   * Currently this just breaks alliances without strategic consideration.
+   * Future implementation should consider:
+   * - Relative strength (troop count, territory size) compared to target
+   * - Risk vs reward of betrayal
+   * - Potential impact on relations with other players
+   * - Timing (don't betray when already fighting other enemies)
+   * - Strategic value of target's territory
+   * - If target is distracted
+   */
+  private maybeConsiderBetrayal(target: Player): boolean {
+    if (this.player === null) throw new Error("not initialized");
+
+    const alliance = this.player.allianceWith(target);
+
+    if (!alliance) return false;
+
+    this.player.breakAlliance(alliance);
+
+    // Successfully broken an alliance
+    return true;
+  }
+
   private maybeAttack() {
     if (this.player === null || this.behavior === null) {
       throw new Error("not initialized");
@@ -208,6 +232,7 @@ export class FakeHumanExecution implements Execution {
     const toAttack = this.random.chance(2)
       ? enemies[0]
       : this.random.randElement(enemies);
+
     if (this.shouldAttack(toAttack)) {
       this.behavior.sendAttack(toAttack);
       return;
@@ -228,9 +253,17 @@ export class FakeHumanExecution implements Execution {
 
   private shouldAttack(other: Player): boolean {
     if (this.player === null) throw new Error("not initialized");
+
     if (this.player.isOnSameTeam(other)) {
       return false;
     }
+
+    // Consider betrayal for allies
+    if (this.player.isAlliedWith(other)) {
+      const canProceed = this.maybeConsiderBetrayal(other);
+      return canProceed;
+    }
+
     if (this.player.isFriendly(other)) {
       if (this.shouldDiscourageAttack(other)) {
         return this.random.chance(200);
@@ -396,7 +429,7 @@ export class FakeHumanExecution implements Execution {
 
   private maybeSendBoatAttack(other: Player) {
     if (this.player === null) throw new Error("not initialized");
-    if (this.player.isOnSameTeam(other)) return;
+    if (this.player.isFriendly(other)) return;
     const closest = closestTwoTiles(
       this.mg,
       Array.from(this.player.borderTiles()).filter((t) =>

src/core/execution/utils/BotBehavior.ts

@@ -230,7 +230,9 @@ export class BotBehavior {
   }
 
   sendAttack(target: Player | TerraNullius) {
-    if (target.isPlayer() && this.player.isOnSameTeam(target)) return;
+    // Skip attacking friendly targets (allies or teammates) - decision to break alliances should be made by caller
+    if (target.isPlayer() && this.player.isFriendly(target)) return;
+
     const maxTroops = this.game.config().maxTroops(this.player);
     const reserveRatio = target.isPlayer()
       ? this.reserveRatio
@@ -242,7 +244,7 @@ export class BotBehavior {
       new AttackExecution(
         troops,
         this.player,
-        target.isPlayer() ? target.id() : null,
+        target.isPlayer() ? target.id() : this.game.terraNullius().id(),
       ),
     );
   }

tests/Attack.test.ts

@@ -113,9 +113,22 @@ describe("Attack", () => {
   });
 });
 
+let playerA: Player;
+let playerB: Player;
+
+function addPlayerToGame(
+  playerInfo: PlayerInfo,
+  game: Game,
+  tile: TileRef,
+): Player {
+  game.addPlayer(playerInfo);
+  game.addExecution(new SpawnExecution(playerInfo, tile));
+  return game.player(playerInfo.id);
+}
+
 describe("Attack race condition with alliance requests", () => {
-  it("should not mark attacker as traitor when alliance is formed after attack starts", async () => {
-    const game = await setup("ocean_and_land", {
+  beforeEach(async () => {
+    game = await setup("ocean_and_land", {
       infiniteGold: true,
       instantBuild: true,
       infiniteTroops: true,
@@ -127,32 +140,22 @@ describe("Attack race condition with alliance requests", () => {
       null,
       "playerA_id",
     );
+    playerA = addPlayerToGame(playerAInfo, game, game.ref(0, 10));
+
     const playerBInfo = new PlayerInfo(
       "playerB",
       PlayerType.Human,
       null,
       "playerB_id",
     );
-
-    game.addPlayer(playerAInfo);
-    game.addPlayer(playerBInfo);
-
-    const playerA = game.player(playerAInfo.id);
-    const playerB = game.player(playerBInfo.id);
-
-    // Spawn both players
-    const spawnA = game.ref(0, 10);
-    const spawnB = game.ref(0, 15);
-
-    game.addExecution(
-      new SpawnExecution(playerAInfo, spawnA),
-      new SpawnExecution(playerBInfo, spawnB),
-    );
+    playerB = addPlayerToGame(playerBInfo, game, game.ref(0, 10));
 
     while (game.inSpawnPhase()) {
       game.executeNextTick();
     }
+  });
 
+  it("should not mark attacker as traitor when alliance is formed after attack starts", async () => {
     // Player A sends alliance request to Player B
     const allianceRequest = playerA.createAllianceRequest(playerB);
     expect(allianceRequest).not.toBeNull();
@@ -173,13 +176,14 @@ describe("Attack race condition with alliance requests", () => {
       playerA.id(),
       null,
     );
-    game.addExecution(counterAttackExecution);
 
     // Player B accepts the alliance request
     if (allianceRequest) {
       allianceRequest.accept();
     }
 
+    game.addExecution(counterAttackExecution);
+
     // Execute a few ticks to process the attacks
     for (let i = 0; i < 5; i++) {
       game.executeNextTick();
@@ -188,57 +192,25 @@ describe("Attack race condition with alliance requests", () => {
     // Player A should not be marked as traitor because the alliance was formed after the attack started
     expect(playerA.isTraitor()).toBe(false);
 
+    expect(playerA.isAlliedWith(playerB)).toBe(true);
+    expect(playerB.isAlliedWith(playerA)).toBe(true);
     // The attacks should have retreated due to the alliance being formed
     expect(playerA.outgoingAttacks()).toHaveLength(0);
     expect(playerB.outgoingAttacks()).toHaveLength(0);
   });
 
-  it("should mark attacker as traitor when alliance existed before attack", async () => {
-    const game = await setup("ocean_and_land", {
-      infiniteGold: true,
-      instantBuild: true,
-      infiniteTroops: true,
-    });
-
-    const playerAInfo = new PlayerInfo(
-      "playerA",
-      PlayerType.Human,
-      null,
-      "playerA_id",
-    );
-    const playerBInfo = new PlayerInfo(
-      "playerB",
-      PlayerType.Human,
-      null,
-      "playerB_id",
-    );
-
-    game.addPlayer(playerAInfo);
-    game.addPlayer(playerBInfo);
-
-    const playerA = game.player(playerAInfo.id);
-    const playerB = game.player(playerBInfo.id);
-
-    // Spawn both players
-    const spawnA = game.ref(0, 10);
-    const spawnB = game.ref(0, 15);
-
-    game.addExecution(
-      new SpawnExecution(playerAInfo, spawnA),
-      new SpawnExecution(playerBInfo, spawnB),
-    );
-
-    while (game.inSpawnPhase()) {
-      game.executeNextTick();
-    }
-
+  it("should prevent player from attacking allied player", async () => {
     // Create an alliance between Player A and Player B
     const allianceRequest = playerA.createAllianceRequest(playerB);
     if (allianceRequest) {
       allianceRequest.accept();
     }
 
-    // Player A attacks Player B (should break the alliance)
+    // Verify alliance exists
+    expect(playerA.isAlliedWith(playerB)).toBe(true);
+    expect(playerB.isAlliedWith(playerA)).toBe(true);
+
+    // Player A tries to attack Player B (should be blocked)
     const attackExecution = new AttackExecution(
       null,
       playerA,
@@ -252,7 +224,73 @@ describe("Attack race condition with alliance requests", () => {
       game.executeNextTick();
     }
 
-    // Player A should be marked as traitor because they attacked an ally
-    expect(playerA.isTraitor()).toBe(true);
+    // No ongoing attacks should exist for either side
+    expect(playerA.outgoingAttacks()).toHaveLength(0);
+    expect(playerB.outgoingAttacks()).toHaveLength(0);
+    expect(playerA.incomingAttacks()).toHaveLength(0);
+    expect(playerB.incomingAttacks()).toHaveLength(0);
+  });
+
+  test("should cancel alliance requests if the recipient attacks", async () => {
+    // Player A sends alliance request to Player B
+    const allianceRequest = playerA.createAllianceRequest(playerB);
+    expect(allianceRequest).not.toBeNull();
+    expect(playerB.incomingAllianceRequests()).toHaveLength(1);
+
+    // Player B attacks Player A
+    const attackExecution = new AttackExecution(
+      null,
+      playerB,
+      playerA.id(),
+      null,
+    );
+    game.addExecution(attackExecution);
+
+    // Execute a few ticks to process the attacks
+    for (let i = 0; i < 5; i++) {
+      game.executeNextTick();
+    }
+    // Alliance request should be denied since player B attacked
+    expect(playerA.outgoingAllianceRequests()).toHaveLength(0);
+    expect(playerB.incomingAllianceRequests()).toHaveLength(0);
+  });
+
+  test("should cancel the proper alliance request among many", async () => {
+    // Add a new player to have more alliance requests
+    const playerCInfo = new PlayerInfo(
+      "playerB",
+      PlayerType.Human,
+      null,
+      "playerB_id",
+    );
+    const playerC = addPlayerToGame(playerCInfo, game, game.ref(10, 10));
+
+    // Player A sends alliance request to Player B
+    const allianceRequestAtoB = playerA.createAllianceRequest(playerB);
+    expect(allianceRequestAtoB).not.toBeNull();
+
+    // Player C also sends alliance request to Player B
+    const allianceRequestCtoB = playerC.createAllianceRequest(playerB);
+    expect(allianceRequestCtoB).not.toBeNull();
+
+    expect(playerB.incomingAllianceRequests()).toHaveLength(2);
+
+    // Player B attacks Player A
+    const attackExecution = new AttackExecution(
+      null,
+      playerB,
+      playerA.id(),
+      null,
+    );
+    game.addExecution(attackExecution);
+
+    // Execute a few ticks to process the attacks
+    for (let i = 0; i < 5; i++) {
+      game.executeNextTick();
+    }
+    // Alliance request A->B should be denied since player B attacked
+    expect(playerA.outgoingAllianceRequests()).toHaveLength(0);
+    // However C->B should remain
+    expect(playerB.incomingAllianceRequests()).toHaveLength(1);
   });
 });

tests/BotBehavior.test.ts

@@ -227,3 +227,159 @@ describe("BotBehavior.handleAllianceExtensionRequests", () => {
     expect(mockGame.addExecution).not.toHaveBeenCalled();
   });
 });
+
+describe("BotBehavior Attack Behavior", () => {
+  let game: Game;
+  let bot: Player;
+  let human: Player;
+  let botBehavior: BotBehavior;
+
+  // Helper function for basic test setup
+  async function setupTestEnvironment() {
+    const testGame = await setup("big_plains", {
+      infiniteGold: true,
+      instantBuild: true,
+      infiniteTroops: true,
+    });
+
+    // Add players
+    const botInfo = new PlayerInfo(
+      "bot_test",
+      PlayerType.Bot,
+      null,
+      "bot_test",
+    );
+    const humanInfo = new PlayerInfo(
+      "human_test",
+      PlayerType.Human,
+      null,
+      "human_test",
+    );
+    testGame.addPlayer(botInfo);
+    testGame.addPlayer(humanInfo);
+
+    const testBot = testGame.player("bot_test");
+    const testHuman = testGame.player("human_test");
+
+    // Assign territories
+    let landTileCount = 0;
+    testGame.map().forEachTile((tile) => {
+      if (!testGame.map().isLand(tile)) return;
+      (landTileCount++ % 2 === 0 ? testBot : testHuman).conquer(tile);
+    });
+
+    // Add troops
+    testBot.addTroops(5000);
+    testHuman.addTroops(5000);
+
+    // Skip spawn phase
+    while (testGame.inSpawnPhase()) {
+      testGame.executeNextTick();
+    }
+
+    const behavior = new BotBehavior(
+      new PseudoRandom(42),
+      testGame,
+      testBot,
+      0.5,
+      0.5,
+      0.2,
+    );
+
+    return { testGame, testBot, testHuman, behavior };
+  }
+
+  // Helper functions for tile assignment
+  function assignAlternatingLandTiles(
+    game: Game,
+    players: Player[],
+    totalTiles: number,
+  ) {
+    let assigned = 0;
+    game.map().forEachTile((tile) => {
+      if (assigned >= totalTiles) return;
+      if (!game.map().isLand(tile)) return;
+      const player = players[assigned % players.length];
+      player.conquer(tile);
+      assigned++;
+    });
+  }
+
+  beforeEach(async () => {
+    const env = await setupTestEnvironment();
+    game = env.testGame;
+    bot = env.testBot;
+    human = env.testHuman;
+    botBehavior = env.behavior;
+  });
+
+  test("bot cannot attack allied player", () => {
+    // Form alliance (bot creates request to human)
+    const allianceRequest = bot.createAllianceRequest(human);
+    allianceRequest?.accept();
+
+    expect(bot.isAlliedWith(human)).toBe(true);
+
+    // Count attacks before attempting attack
+    const attacksBefore = bot.outgoingAttacks().length;
+
+    // Attempt attack (should be blocked)
+    botBehavior.sendAttack(human);
+
+    // Execute a few ticks to process the attacks
+    for (let i = 0; i < 5; i++) {
+      game.executeNextTick();
+    }
+
+    expect(bot.isAlliedWith(human)).toBe(true);
+    expect(human.incomingAttacks()).toHaveLength(0);
+    // Should be same number of attacks (no new attack created)
+    expect(bot.outgoingAttacks()).toHaveLength(attacksBefore);
+  });
+
+  test("nation cannot attack allied player", () => {
+    // Create nation
+    const nationInfo = new PlayerInfo(
+      "nation_test",
+      PlayerType.FakeHuman,
+      null,
+      "nation_test",
+    );
+    game.addPlayer(nationInfo);
+    const nation = game.player("nation_test");
+
+    // Use helper for tile assignment
+    assignAlternatingLandTiles(game, [bot, human, nation], 21); // 21 to ensure each gets 7 tiles
+
+    nation.addTroops(1000);
+
+    const nationBehavior = new BotBehavior(
+      new PseudoRandom(42),
+      game,
+      nation,
+      0.5,
+      0.5,
+      0.2,
+    );
+
+    // Alliance between nation and human
+    const allianceRequest = nation.createAllianceRequest(human);
+    allianceRequest?.accept();
+
+    expect(nation.isAlliedWith(human)).toBe(true);
+
+    const attacksBefore = nation.outgoingAttacks().length;
+    nation.addTroops(50_000);
+
+    // Nation tries to attack ally (should be blocked)
+    nationBehavior.sendAttack(human);
+
+    // Execute a few ticks to process the attacks
+    for (let i = 0; i < 5; i++) {
+      game.executeNextTick();
+    }
+
+    expect(nation.isAlliedWith(human)).toBe(true);
+    expect(nation.outgoingAttacks()).toHaveLength(attacksBefore);
+  });
+});

tests/core/game/GameImpl.test.ts

@@ -79,6 +79,12 @@ describe("GameImpl", () => {
     game.executeNextTick();
     game.executeNextTick();
 
+    // STEP 1: First betray (manually break alliance)
+    const alliance = attacker.allianceWith(defender);
+    expect(alliance).toBeTruthy();
+    attacker.breakAlliance(alliance!);
+
+    // STEP 2: Then attack after betrayal
     game.addExecution(new AttackExecution(100, attacker, defender.id()));
 
     do {
@@ -86,6 +92,7 @@ describe("GameImpl", () => {
     } while (attacker.outgoingAttacks().length > 0);
 
     expect(attacker.isTraitor()).toBe(false);
+    expect(attacker.allianceWith(defender)).toBeFalsy();
   });
 
   test("Do become traitor when betraying active player", async () => {
@@ -110,6 +117,13 @@ describe("GameImpl", () => {
     game.executeNextTick();
     game.executeNextTick();
 
+    // First betray (manually break alliance)
+    const alliance = attacker.allianceWith(defender);
+    expect(alliance).toBeTruthy();
+    attacker.breakAlliance(alliance!);
+
+    game.executeNextTick();
+
     game.addExecution(new AttackExecution(100, attacker, defender.id()));
 
     do {
@@ -117,5 +131,6 @@ describe("GameImpl", () => {
     } while (attacker.outgoingAttacks().length > 0);
 
     expect(attacker.isTraitor()).toBe(true);
+    expect(attacker.allianceWith(defender)).toBeFalsy();
   });
 });