Skip to content

fix: preserve null gold in DonateGoldExecution so auto-donate works (BUG-03) #3944

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

Open
berkelmali wants to merge 2 commits into
base: main
Choose a base branch
from
+70 −2
Open

fix: preserve null gold in DonateGoldExecution so auto-donate works (BUG-03) #3944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9520c4
fix: preserve null gold in DonateGoldExecution so auto-donate works (…
berkelmali May 16, 2026
7f8f820
test: add null goldNum auto-donate regression test (BUG-03)
berkelmali May 17, 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
4 changes: 2 additions & 2 deletions src/core/execution/DonateGoldExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {

export class DonateGoldExecution implements Execution {
private recipient: Player;
private gold: Gold;
private gold: Gold | null;

private mg: Game;
private random: PseudoRandom;
Expand All @@ -30,7 +30,7 @@ export class DonateGoldExecution implements Execution {
private recipientID: PlayerID,
goldNum: number | null,
) {
this.gold = toInt(goldNum ?? 0);
this.gold = goldNum === null ? null : toInt(goldNum);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 16, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Search for tests covering DonateGoldExecution with null gold amount

# Find test files that import or reference DonateGoldExecution
rg -l 'DonateGoldExecution' --glob '*test*' --glob '*spec*'

# Search for test cases that might cover null gold or auto-donate behavior
rg -n -C3 'DonateGold.*null|auto.*donate|goldNum.*null' --glob '*test*' --glob '*spec*'

Repository: openfrontio/OpenFrontIO

Length of output: 119

🏁 Script executed:

# Check test files for DonateGoldExecution coverage
head -100 tests/Donate.test.ts

# Check the source file to understand the context
cat -n src/core/execution/DonateGoldExecution.ts | head -60

Repository: openfrontio/OpenFrontIO

Length of output: 4882

🏁 Script executed:

# Find the line where DonateGoldExecution is instantiated in the test
rg -n 'new DonateGoldExecution' tests/Donate.test.ts -A 2

# Also check the AllianceDonation test file
rg -n 'new DonateGoldExecution' tests/AllianceDonation.test.ts -A 2

Repository: openfrontio/OpenFrontIO

Length of output: 433

🏁 Script executed:

# Search all test files for any DonateGoldExecution calls with null or without goldNum param
rg 'DonateGoldExecution\(' tests/ -A 1 | grep -v '^\s*[0-9]' | head -50

# Count total DonateGoldExecution calls
rg 'new DonateGoldExecution' tests/ | wc -l

# Check if there's a test specifically for auto-donate or null goldNum
rg -i 'auto.*donate|goldNum.*null|gold.*null' tests/

Repository: openfrontio/OpenFrontIO

Length of output: 605

Add test for auto-donate scenario (null goldNum).

The fix correctly preserves null at line 33 to enable the auto-donate fallback at line 49. However, no test cases cover the scenario where goldNum is null. Per coding guidelines, all changes to src/core/ must include tests. Please add a test case that verifies: when DonateGoldExecution is instantiated with goldNum = null, the donation amount defaults to sender.gold() / 3n during initialization.

🤖 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/DonateGoldExecution.ts` at line 33, The change preserved
null for this.gold in DonateGoldExecution to enable the auto-donate fallback,
but there is no test covering goldNum = null; add a unit test that constructs
DonateGoldExecution with goldNum = null and a mock sender whose gold() returns a
known BigInt, then assert that after initialization (or after running the
initializer method used in the class) the actual donation amount equals
sender.gold() / 3n; reference DonateGoldExecution, the constructor behavior that
sets this.gold, and the fallback calculation sender.gold() / 3n when writing the
test.

}

init(mg: Game, ticks: number): void {
Expand Down
68 changes: 68 additions & 0 deletions tests/Donate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,74 @@ describe("Donate troops to a non ally", () => {
});
});

describe("Donate gold with null goldNum (auto-donate fallback)", () => {
it("Should auto-donate 1/3 of sender gold when goldNum is null", async () => {
const game = await setup("ocean_and_land", {
infiniteGold: false,
donateGold: true,
});
const gameID: GameID = "game_id";

const donorInfo = new PlayerInfo(
"donor",
PlayerType.Human,
null,
"donor_id",
);
const recipientInfo = new PlayerInfo(
"recipient",
PlayerType.Human,
null,
"recipient_id",
);

game.addPlayer(donorInfo);
game.addPlayer(recipientInfo);

const donor = game.player(donorInfo.id);
const recipient = game.player(recipientInfo.id);

// Spawn both players
const spawnA = game.ref(0, 10);
const spawnB = game.ref(0, 15);

game.addExecution(
new SpawnExecution(gameID, donorInfo, spawnA),
new SpawnExecution(gameID, recipientInfo, spawnB),
);

// donor sends alliance request to recipient
const allianceRequest = donor.createAllianceRequest(recipient);
expect(allianceRequest).not.toBeNull();

// recipient accepts the alliance request
if (allianceRequest) {
allianceRequest.accept();
}
game.executeNextTick();

// Give donor a known amount of gold
donor.addGold(9000n);
const donorGoldBefore = donor.gold();
const recipientGoldBefore = recipient.gold();

// Pass null as goldNum — should trigger auto-donate of sender.gold() / 3n
game.addExecution(new DonateGoldExecution(donor, recipientInfo.id, null));

for (let i = 0; i < 5; i++) {
game.executeNextTick();
}

// Donor should have lost gold
expect(donor.gold() < donorGoldBefore).toBe(true);
// Recipient should have received gold
expect(recipient.gold() > recipientGoldBefore).toBe(true);
// Check donor lost roughly 1/3 of their gold
const donorLost = donorGoldBefore - donor.gold();
expect(donorLost).toBeGreaterThan(0n);
Comment on lines +248 to +250
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the expected auto-donate amount, not just > 0n.

This regression can still pass if donation is incorrect but positive. Please assert against donorGoldBefore / 3n (or at least a strong lower bound tied to it) so BUG-03 is truly locked.

Suggested tightening 
-    // Check donor lost roughly 1/3 of their gold
+    // Check null path donates one-third of sender gold
+    const expectedAutoDonate = donorGoldBefore / 3n;
     const donorLost = donorGoldBefore - donor.gold();
+    const recipientGained = recipient.gold() - recipientGoldBefore;
-    expect(donorLost).toBeGreaterThan(0n);
+    expect(donorLost).toBeGreaterThan(0n);
+    expect(recipientGained).toBeGreaterThanOrEqual(expectedAutoDonate);
📝 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.

Suggested change
// Check donor lost roughly 1/3 of their gold
const donorLost = donorGoldBefore - donor.gold();
expect(donorLost).toBeGreaterThan(0n);
// Check null path donates one-third of sender gold
const expectedAutoDonate = donorGoldBefore / 3n;
const donorLost = donorGoldBefore - donor.gold();
const recipientGained = recipient.gold() - recipientGoldBefore;
expect(donorLost).toBeGreaterThan(0n);
expect(recipientGained).toBeGreaterThanOrEqual(expectedAutoDonate);
🤖 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/Donate.test.ts` around lines 248 - 250, Replace the weak positivity
check with a precise assertion against the expected auto-donate amount: compute
expectedLoss = donorGoldBefore / 3n (or a lower-bound adjusted if rounding is
desired) and replace expect(donorLost).toBeGreaterThan(0n) with
expect(donorLost).toBeGreaterThanOrEqual(expectedLoss); reference the existing
variables donorGoldBefore, donor.gold(), and donorLost when making the change so
the test verifies the ~1/3 deduction exactly.

});
});

describe("Donate Gold to a non ally", () => {
it("Gold should not be donated", async () => {
const game = await setup("ocean_and_land", {
Expand Down
Loading