Fix: Bots NEVER attacked someone if they had water access

[FloPinguin]

Description:

Bots always attacked Terra Nullius if they shared a border with Terra Nullius.
But water is Terra Nullius...
So I changed that condition to this.bot.neighbors().some((n) => !n.isPlayer()).

Please complete the following:

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

Please put your Discord username so you can be contacted if a bug or regression is found:

FloPinguin

[coderabbitai]

Walkthrough

The maybeAttack method in BotExecution was modified to change how it detects terra nullius neighbors. Instead of checking if the bot shares a border with terra nullius, it now checks whether any neighbor is a non-player entity, triggering the attack logic accordingly.

Changes

Cohort / File(s)	Summary
Attack neighbor detection logic src/core/execution/BotExecution.ts	Changed terra nullius detection from sharesBorderWith(this.mg.terraNullius()) to this.bot.neighbors().some(n => !n.isPlayer()). Condition now checks for existence of non-player neighbors rather than border proximity, adjusting when terra nullius attacks are initiated.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A bot surveys its neighbors near,
No longer checks the border's tear—
If strangers lurk without a name,
Terra nullius feels the flame! 🔥⚔️

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main bug fix: bots no longer incorrectly attack when they have water access because the trigger condition was changed from border-sharing with Terra Nullius to checking for non-player neighbors.
Description check	✅ Passed	The description is directly related to the changeset, explaining the bug (bots attacking Terra Nullius/water), the root cause (water being Terra Nullius), and the solution (checking for non-player neighbors instead).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24774b4 and 7459940.

📒 Files selected for processing (1)

• src/core/execution/BotExecution.ts

🧰 Additional context used

🧠 Learnings (11)

📓 Common learnings

Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.

Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.

📚 Learning: 2025-10-18T17:54:01.311Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-12-13T14:58:29.645Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-10-20T20:15:28.858Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-10-20T11:02:16.969Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-08-23T07:48:19.060Z

Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-10-27T09:47:26.395Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-11-01T00:24:33.860Z

Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2026-01-13T20:16:05.535Z

Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2025-08-29T16:16:11.309Z

Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.

Applied to files:

• src/core/execution/BotExecution.ts

📚 Learning: 2026-01-12T21:37:01.156Z

Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.

Applied to files:

• src/core/execution/BotExecution.ts

🔇 Additional comments (1)

src/core/execution/BotExecution.ts (1)

103-109: Good fix for the neighbor detection logic.

The change from sharesBorderWith(terraNullius()) to neighbors().some((n) => !n.isPlayer()) correctly excludes water tiles. Now bots only expand into actual unowned land, allowing them to attack other players when no land expansion is possible.

One note: the neighborsTerraNullius flag is a one-way latch. Once set to false (line 108), bots will never expand into new Terra Nullius even if it appears later (e.g., after a player dies). This is pre-existing behavior, so not blocking, but worth knowing.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}