Skip to content

Much better river handling for nations and tribes! 🏞️#3786

Merged
evanpelle merged 9 commits into
mainfrom
better-neighbr-finding
Apr 29, 2026
Merged

Much better river handling for nations and tribes! 🏞️#3786
evanpelle merged 9 commits into
mainfrom
better-neighbr-finding

Conversation

@FloPinguin
Copy link
Copy Markdown
Contributor

@FloPinguin FloPinguin commented Apr 27, 2026

Description:

In this example, the two nations DONT see each other as neighbors, but as ISLANDERS. Because they dont have a direct border connection, there is water in between.

image

This is a big problem, because most of the logic in AiAttackBehavior gets ignored. Only the "islander" strategy runs (late, because its a not-important strat).

Summary

  • PlayerImpl.neighbors() now includes cross-water neighbors: a new shoreReachableNeighbors() helper samples every 10th shore border tile and looks up to 5 tiles in each cardinal direction across water, finding land owners on the other side (covers rivers up to 4 tiles wide).

  • AiAttackBehavior.maybeAttack() extends the hasNonNukedTerraNullius check to also trigger on TN detected via player.neighbors(), so nations notice and pursue TN that is only reachable across a river.

  • sendAttack() uses a new hasLandBorderWithTerraNullius() land-only adjacency check to decide between a land attack and a boat attack for TN, rather than sharesBorderWith() which includes water tiles.

  • Added sendBoatAttackToNearbyTerraNullius(): when no TN land is directly adjacent, the AI scans its shore border tiles for unowned land across water and dispatches a transport ship.

Also works for Tribes!

Tribes can boat rivers now, really cool.

2026-04-27.23-49-24.mp4

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
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

Walkthrough

AI neighbor discovery widened (adds sparse shore reachability), neighbors() renamed to nearby(), and attack routing/refunds refactored: sendAttack, sendLandAttack, and sendBoatAttack now return booleans; TerraNullius handling uses land-only adjacency with shore-scan fallback for transports.

Changes

Cohort / File(s) Summary
AI Attack Logic
src/core/execution/utils/AiAttackBehavior.ts
Refactored attack flow: maybeAttack merges direct-border owners with player.nearby(); attack strategy helpers and sendAttack now return boolean. TerraNullius uses land-only adjacency; shore-scan may enqueue TransportShipExecution when transport preconditions pass.
Neighbor Detection (API + Impl)
src/core/game/Game.ts, src/core/game/PlayerImpl.ts
Public method renamed neighbors()nearby(); PlayerImpl.nearby() adds shore-reachable owners by sampling border tiles (every 10th), checking shore+cardinal water then probing 5 tiles out for land owners and merging results.
Tribe / Execution Flow
src/core/execution/TribeExecution.ts
maybeAttack() now treats sendAttack(...) as boolean-returning and only early-exits on true; uses nearby() for TerraNullius detection so fallbacks run when attacks fail.
Alliance / Nation Behaviors
src/core/execution/alliance/BreakAllianceExecution.ts, src/core/execution/nation/NationAllianceBehavior.ts, src/core/execution/nation/NationEmojiBehavior.ts, src/core/execution/nation/NationStructureBehavior.ts
Switched calls from neighbors() to nearby() across alliance, emoji, station connectivity, and defense-post logic; existing filters and logic unchanged.
Tests & Misc
tests/NationStructureBehavior.test.ts, ...
Test mocks updated to expose nearby() instead of neighbors(); minor call-site and control-flow updates to handle boolean attack returns.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant AI as "AI Decision"
    participant Player as "PlayerImpl"
    participant Map as "World/TileMap"
    participant Builder as "TransportBuilder"
    participant Exec as "ExecutionEngine"

    AI->>Player: request border tiles + nearby()
    Player->>Map: sample border tiles (every 10th), identify shore tiles
    Player->>Map: for each shore tile check cardinal water, probe 5 tiles for land owner
    Map-->>Player: return owners (including TN acceptance)
    Player-->>AI: merged neighbor set (direct + shoreReachable)

    AI->>AI: maybeAttack() -> build borderingPlayers, choose target
    AI->>AI: call sendAttack(target) -> returns boolean
    alt Player or land-adjacent target
        AI->>Exec: sendLandAttack -> enqueue AttackExecution
        Exec-->>AI: ack (true)
    else shore-only TN target reachable by transport
        AI->>Builder: shore-scan -> can build transport?
        Builder-->>AI: buildable? (true/false)
        alt buildable & preconds pass
            AI->>Exec: sendBoatAttack / enqueue TransportShipExecution
            Exec-->>AI: ack (true)
        else
            AI-->>AI: sendAttack returned false (no attack)
        end
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🌊 Scouts step ten, peering past the foam,
Shores whisper owners far from home,
Borders join and choices turn to fleets,
Logic returns true only when order meets,
Small probes, clear strikes — the map learns new beats. ⚓

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Much better river handling for nations and tribes!' clearly and directly describes the main feature: improved river crossing mechanics for AI gameplay.
Description check ✅ Passed The description explains the problem (water-separated nations seen as islanders), the solution (cross-water neighbor detection), implementation details, and includes confirmations of testing and translation updates.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 81-84: The current hasNonNukedTerraNullius boolean is incorrectly
true when playerNeighbors contains a nuked (fallout) TerraNullius because
playerNeighbors.some(n => !n.isPlayer()) doesn't check fallout; replace this
with a helper that checks land tiles reachable across rivers are ownerless AND
not fallout. Implement a private hasRiverReachableNonNukedTerraNullius() that
reuses the existing river-scan logic but returns true only when tile satisfies
!this.game.hasOwner(tile) && !this.game.hasFallout(tile), then use that helper
in place of playerNeighbors.some(...) when computing hasNonNukedTerraNullius
(keeping the original border.some(...) clause intact).
- Around line 799-818: The current scan in AiAttackBehavior iterates directions
but only checks the land tile at exactly distance 5 (nx,ny) and so misses TNs
that appear at distances 2–4; modify the inner loop in the border/directions
scan so that for each direction (dx,dy) you iterate step from 1 through 5,
compute candidate coords (bx+dx*step, by+dy*step), validate isValidCoord/isWater
for intermediate tiles as appropriate, and when you encounter the first land
tile use the same checks (this.game.isLand, this.game.hasOwner,
canBuildTransportShip(this.game, this.player, tile)) and act on it (break out of
the step loop) instead of only testing the exact distance 5 position; ensure you
still skip directions when intermediate tiles are invalid or not water as
previously intended.

In `@src/core/game/PlayerImpl.ts`:
- Around line 355-399: shoreReachableNeighbors currently jumps directly to the
coordinate 5 steps from a shore tile which can skip the first land tile
encountered across a river; change the logic in shoreReachableNeighbors (inside
the loop over directions after confirming the adjacent tile is water) to iterate
distance d from 1 up to 5, compute nx = bx + dx * d and ny = by + dy * d, skip
invalid coords, and on the first tile where map.isLand(tile) is true treat that
tile as the target (get owner via map.ownerID(tile), skip if owner ===
this.smallID(), and add this.mg.playerBySmallID(owner) to ns); stop the loop
once you find the first land tile or reach d==5 so you no longer skip earlier
land patches.
🪄 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: ef20a650-f2fc-4c4d-b3dd-020e0436890e

📥 Commits

Reviewing files that changed from the base of the PR and between 4aa726c and 6cbfd5d.

📒 Files selected for processing (2)
  • src/core/execution/utils/AiAttackBehavior.ts
  • src/core/game/PlayerImpl.ts

Comment thread src/core/execution/utils/AiAttackBehavior.ts
Comment thread src/core/execution/utils/AiAttackBehavior.ts Outdated
Comment thread src/core/game/PlayerImpl.ts
@github-project-automation github-project-automation Bot moved this from Triage to Development in OpenFront Release Management Apr 27, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 770-779: hasLandBorderWithTerraNullius currently returns true for
any unowned land including fallout; update it to mirror the maybeAttack gate by
excluding fallout — i.e., inside the nested loops (iterating
this.player.borderTiles() and this.game.neighbors(border)), only return true
when this.game.isLand(neighbor) && !this.game.hasOwner(neighbor) &&
!this.game.hasFallout(neighbor). This keeps hasLandBorderWithTerraNullius
consistent with hasNonNukedTerraNullius and the maybeAttack branch selection.
🪄 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: 573c6782-8e00-4769-82f5-5be1de9142b1

📥 Commits

Reviewing files that changed from the base of the PR and between 6cbfd5d and 0ea05ce.

📒 Files selected for processing (2)
  • src/core/execution/utils/AiAttackBehavior.ts
  • src/core/game/PlayerImpl.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/core/game/PlayerImpl.ts

Comment thread src/core/execution/utils/AiAttackBehavior.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/core/execution/utils/AiAttackBehavior.ts (1)

81-84: ⚠️ Potential issue | 🟠 Major

Filter fallout out of the TN neighbor gate again.

Lines 82-84 treat any non-player from player.neighbors() as “non-nuked” TerraNullius, but neighbors() also includes direct unowned land with fallout. That means a nuked border tile can still make hasNonNukedTerraNullius true and send the AI down the TN path again.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/utils/AiAttackBehavior.ts` around lines 81 - 84, The
hasNonNukedTerraNullius boolean wrongly treats any non-player neighbor from
playerNeighbors as non-nuked TN even when that neighbor tile has fallout; update
the second predicate to also exclude fallout (i.e. require the neighbor is not a
player AND the neighbor's tile is not fallout according to
this.game.hasFallout). Locate the hasNonNukedTerraNullius computation that uses
border and playerNeighbors.some((n) => !n.isPlayer()) and change the
playerNeighbors check to verify both !n.isPlayer() and
!this.game.hasFallout(...) for the neighbor's tile/reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 85-87: The code currently short-circuits maybeAttack() whenever
hasNonNukedTerraNullius is true by calling sendAttack(this.game.terraNullius())
and returning unconditionally; because the new river TN path can legitimately do
nothing (boats disabled, at cap, no launch path) this causes the nation to skip
further attack logic. Change sendAttack (and the helper
sendBoatAttackToNearbyTerraNullius) to return a boolean indicating whether an
action was actually queued/sent, and in maybeAttack() only return when
sendAttack(...) returned true; otherwise continue the attack logic. Ensure
sendBoatAttackToNearbyTerraNullius returns false on the "do nothing" cases
(boats disabled, at cap, no path) and true when it enqueues an attack so
maybeAttack() only short-circuits on successful queueing.

---

Duplicate comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 81-84: The hasNonNukedTerraNullius boolean wrongly treats any
non-player neighbor from playerNeighbors as non-nuked TN even when that neighbor
tile has fallout; update the second predicate to also exclude fallout (i.e.
require the neighbor is not a player AND the neighbor's tile is not fallout
according to this.game.hasFallout). Locate the hasNonNukedTerraNullius
computation that uses border and playerNeighbors.some((n) => !n.isPlayer()) and
change the playerNeighbors check to verify both !n.isPlayer() and
!this.game.hasFallout(...) for the neighbor's tile/reference.
🪄 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: cb46dde5-e18d-4d16-9374-ace5b75d74e0

📥 Commits

Reviewing files that changed from the base of the PR and between 0ea05ce and 23f0e64.

📒 Files selected for processing (1)
  • src/core/execution/utils/AiAttackBehavior.ts

Comment thread src/core/execution/utils/AiAttackBehavior.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 737-755: The new sendAttack(target): boolean contract is not
propagated at callers in TribeExecution.ts; locate the three sendAttack(...)
call sites in src/core/execution/TribeExecution.ts and stop unconditionally
returning there—check the boolean result and only short-circuit/return when
sendAttack(...) returns true (success); when it returns false, allow the method
to continue to follow-up logic or return false to propagate failure as
appropriate so queued-execution behavior is not skipped.
🪄 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: 9c5a1c54-6605-48ba-9021-7968ebb899d1

📥 Commits

Reviewing files that changed from the base of the PR and between 23f0e64 and 7bc6e33.

📒 Files selected for processing (1)
  • src/core/execution/utils/AiAttackBehavior.ts

Comment thread src/core/execution/utils/AiAttackBehavior.ts
coderabbitai[bot]
coderabbitai Bot previously approved these changes Apr 27, 2026
@PatrickPlaysBadly
Copy link
Copy Markdown
Contributor

Nice addition!

Copy link
Copy Markdown
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

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

review from claude:

Verdict

REQUEST CHANGES

Must fix before merge:

  • Finding #1 (PlayerImpl.ts:389) — likely typo in fallout filter; one-line fix.
  • Finding #2 / #13 (Game.ts:740) — decide between scoping the cross-river reach to a dedicated method or auditing+documenting the five unrelated callers of neighbors(). Either is fine; the silent contract change is not.

Strongly recommended:

  • Finding #8 — at least gate assistAllies's side effects on the return value.
  • Finding #12 — add the regression test for #1; without it, a future refactor will reintroduce the typo.

Optional / follow-up:

  • Performance findings #3, #4, #7 (cache neighbors(), hoist directions, drop Array.from allocations) — material on large nations; can be batched into a follow-up.
  • Findings #9, #10 — minor cleanups.

Comment thread src/core/game/PlayerImpl.ts Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe we should rename this to "nearbyPlayers", because it's not strictly neighbors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went with just "nearby" because the method can also return TerraNullius...

Btw, I dont understand what claude means in that summary, fallout filter seems fine and all callers of neighbors (now "nearby") are related (the additional players from the river check are no problem for the callers, all callers are NPC logic)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 (3)
src/core/execution/utils/AiAttackBehavior.ts (3)

679-711: ⚠️ Potential issue | 🟠 Major

Only return from attackRandomTarget() after a real attack was queued.

return this.sendAttack(...) now stops this method even when sendAttack() returns false. One failed target can skip the rest of the fallback logic for the tick.

Suggested fix
   const incomingAttackPlayer = this.findIncomingAttackPlayer();
   if (incomingAttackPlayer) {
-    this.sendAttack(incomingAttackPlayer, true);
-    return;
+    if (this.sendAttack(incomingAttackPlayer, true)) return;
   }
@@
     if (toAttack !== null) {
       if (this.random.chance(3)) {
-        return this.sendAttack(toAttack);
+        if (this.sendAttack(toAttack)) return;
       }
     }
@@
-      return this.sendAttack(neighbor);
+      if (this.sendAttack(neighbor)) return;
     }

Also applies to: 737-755

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/utils/AiAttackBehavior.ts` around lines 679 - 711, In
attackRandomTarget(), avoid returning immediately on the result of sendAttack()
so a false return doesn't abort remaining fallback logic; call sendAttack(...)
and only return if it succeeds (true), otherwise continue execution to try other
targets or logic paths (apply the same change for the traitor branch using
getNeighborTraitorToAttack() and the neighbor loop over player.nearby() with
random.shuffleArray(), and ensure the incoming retaliation via
findIncomingAttackPlayer() still returns immediately on success).

467-491: ⚠️ Potential issue | 🟠 Major

assistAllies() still treats a failed sendAttack() as success.

After this contract change, assistAllies() should only lower relation, send EMOJI_ASSIST_ACCEPT, and return true if sendAttack(target) actually queued something. Right now a blocked river/boat assist can still stop later strategies and send the wrong emoji.

Suggested fix
-        this.player.updateRelation(ally, -20);
-        this.sendAttack(target);
-        this.emojiBehavior.sendEmoji(ally, EMOJI_ASSIST_ACCEPT);
-        return true;
+        if (!this.sendAttack(target)) continue;
+        this.player.updateRelation(ally, -20);
+        this.emojiBehavior.sendEmoji(ally, EMOJI_ASSIST_ACCEPT);
+        return true;

Also applies to: 737-755

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/utils/AiAttackBehavior.ts` around lines 467 - 491,
assistAllies() currently treats sendAttack(target) as always succeeding; change
the flow so that you only decrement relation (player.updateRelation), send
EMOJI_ASSIST_ACCEPT via emojiBehavior.sendEmoji(ally, EMOJI_ASSIST_ACCEPT), and
return true when sendAttack(target) actually queued/returned success; if
sendAttack fails, do not lower relation (or revert the -20) and do not send
EMOJI_ASSIST_ACCEPT or return true — instead continue checking other
allies/targets or return false. Apply the same fix to the other identical block
around the assistAllies-equivalent code at lines ~737-755 (use the same checks
around sendAttack(), updateRelation(), and EMOJI_ASSIST_ACCEPT).

412-419: ⚠️ Potential issue | 🟠 Major

attackBots() now reaches river bots without a pending boat budget.

nearby() can include short cross-water bots, so this loop can hit sendBoatAttack() several times in one tick. sendBoatAttack() sizes each ship from this.player.troops() / 5, and its boat-cap check only sees already-existing ships, not boats that were just queued. On Hard/Impossible, that can queue more boats and troops than intended.

If river bots are intentional here, please add pending troop/boat tracking for boat attacks too; otherwise keep this bot list land-only.

Also applies to: 893-930

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/utils/AiAttackBehavior.ts` around lines 412 - 419, The bot
filter in attackBots() uses nearby() which can include short cross-water (river)
bots and causes multiple sendBoatAttack() calls that ignore queued boats/troops;
either (A) restrict the bots list to land-only (e.g., change the filter in
attackBots() to exclude bots across water by checking pathability/reachability
such as a canReach/isOnSameLand check for each Player n, keeping the
PlayerType.Bot condition), or (B) if river-boat attacks are intended, add
pending-boat and pending-troop tracking used by sendBoatAttack() (introduce
pendingBoatCount and pendingTroopReserve on this.player or AiAttackBehavior,
increment when queueing a boat and decrement when it launches, and have
sendBoatAttack() consult pending counts plus existing ships and troops via
this.player.troops() before queuing) so multiple sendBoatAttack() calls in one
tick honor caps; apply the same change pattern where attackBots logic repeats
(also around the 893-930 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/execution/nation/NationStructureBehavior.ts`:
- Line 881: The test fixtures for Player were not updated after the API rename:
NationStructureBehavior.ts now calls player.nearby(), but the Player fixture
stubs still expose the old method name, causing CI failures; update the Player
fixtures used by NationStructureBehavior (and any related stubs referenced
around the other occurrence) to implement a nearby() method that mirrors the
prior API behavior (or add a small adapter that forwards nearby() to the old
method name) so player.nearby() exists during tests — search for the Player test
fixture and the NationStructureBehavior usages to add nearby() or an adapter
function to the fixture.

In `@src/core/game/PlayerImpl.ts`:
- Around line 361-371: The loop currently samples every 10th tile from
borderTiles() before checking map.isShore(), which can skip all shore tiles on
small territories and prevent shoreReachableNeighbors()/nearby() from probing
the river; change the logic to first filter the border list for shore tiles (use
borderTiles() then .filter(b => map.isShore(b))) and then apply the i += 10
stride to that shores array (or alternatively move the map.isShore() check above
the stride so you only count/stride among shore tiles) so
shoreReachableNeighbors() is guaranteed to see river-facing tiles.

---

Outside diff comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 679-711: In attackRandomTarget(), avoid returning immediately on
the result of sendAttack() so a false return doesn't abort remaining fallback
logic; call sendAttack(...) and only return if it succeeds (true), otherwise
continue execution to try other targets or logic paths (apply the same change
for the traitor branch using getNeighborTraitorToAttack() and the neighbor loop
over player.nearby() with random.shuffleArray(), and ensure the incoming
retaliation via findIncomingAttackPlayer() still returns immediately on
success).
- Around line 467-491: assistAllies() currently treats sendAttack(target) as
always succeeding; change the flow so that you only decrement relation
(player.updateRelation), send EMOJI_ASSIST_ACCEPT via
emojiBehavior.sendEmoji(ally, EMOJI_ASSIST_ACCEPT), and return true when
sendAttack(target) actually queued/returned success; if sendAttack fails, do not
lower relation (or revert the -20) and do not send EMOJI_ASSIST_ACCEPT or return
true — instead continue checking other allies/targets or return false. Apply the
same fix to the other identical block around the assistAllies-equivalent code at
lines ~737-755 (use the same checks around sendAttack(), updateRelation(), and
EMOJI_ASSIST_ACCEPT).
- Around line 412-419: The bot filter in attackBots() uses nearby() which can
include short cross-water (river) bots and causes multiple sendBoatAttack()
calls that ignore queued boats/troops; either (A) restrict the bots list to
land-only (e.g., change the filter in attackBots() to exclude bots across water
by checking pathability/reachability such as a canReach/isOnSameLand check for
each Player n, keeping the PlayerType.Bot condition), or (B) if river-boat
attacks are intended, add pending-boat and pending-troop tracking used by
sendBoatAttack() (introduce pendingBoatCount and pendingTroopReserve on
this.player or AiAttackBehavior, increment when queueing a boat and decrement
when it launches, and have sendBoatAttack() consult pending counts plus existing
ships and troops via this.player.troops() before queuing) so multiple
sendBoatAttack() calls in one tick honor caps; apply the same change pattern
where attackBots logic repeats (also around the 893-930 region).
🪄 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: ef0626bc-b2cd-4065-93bf-03df379d2964

📥 Commits

Reviewing files that changed from the base of the PR and between 7bc6e33 and e6b35fb.

📒 Files selected for processing (8)
  • src/core/execution/TribeExecution.ts
  • src/core/execution/alliance/BreakAllianceExecution.ts
  • src/core/execution/nation/NationAllianceBehavior.ts
  • src/core/execution/nation/NationEmojiBehavior.ts
  • src/core/execution/nation/NationStructureBehavior.ts
  • src/core/execution/utils/AiAttackBehavior.ts
  • src/core/game/Game.ts
  • src/core/game/PlayerImpl.ts

Comment thread src/core/execution/nation/NationStructureBehavior.ts
Comment thread src/core/game/PlayerImpl.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/core/execution/utils/AiAttackBehavior.ts (1)

81-87: Minor observation on hasNonNukedTerraNullius detection.

The second condition playerNeighbors.some((n) => !n.isPlayer()) can be true when nearby() returns TerraNullius from direct borders that have fallout (since nearby() only filters fallout for river-reachable tiles). This could cause sendAttack(terraNullius()) to be called when no valid TN target exists.

However, this is handled gracefully:

  • hasLandBorderWithTerraNullius() filters fallout (line 761)
  • sendBoatAttackToNearbyTerraNullius() filters fallout (line 808)
  • Both return false when no valid target exists

So the attack path fails safely and the method continues. This is inefficient but not incorrect. Acceptable for now given the added complexity of filtering fallout in nearby() itself.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/utils/AiAttackBehavior.ts` around lines 81 - 87, The
hasNonNukedTerraNullius check can be a false positive because
playerNeighbors.some(n => !n.isPlayer()) may include terra nullius tiles that
actually have fallout; update the condition to only mark hasNonNukedTerraNullius
true when a non-nuked TN target is actually reachable—e.g., replace or augment
the playerNeighbors check by consulting the existing filtering helpers (call
hasLandBorderWithTerraNullius() or reuse the logic from
sendBoatAttackToNearbyTerraNullius()) so that
sendAttack(this.game.terraNullius()) is only attempted when a valid non-fallout
terra nullius target exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 81-87: The hasNonNukedTerraNullius check can be a false positive
because playerNeighbors.some(n => !n.isPlayer()) may include terra nullius tiles
that actually have fallout; update the condition to only mark
hasNonNukedTerraNullius true when a non-nuked TN target is actually
reachable—e.g., replace or augment the playerNeighbors check by consulting the
existing filtering helpers (call hasLandBorderWithTerraNullius() or reuse the
logic from sendBoatAttackToNearbyTerraNullius()) so that
sendAttack(this.game.terraNullius()) is only attempted when a valid non-fallout
terra nullius target exists.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1419300c-b856-4242-a92a-51b72b8c28aa

📥 Commits

Reviewing files that changed from the base of the PR and between e6b35fb and 9102ffa.

📒 Files selected for processing (3)
  • src/core/execution/utils/AiAttackBehavior.ts
  • src/core/game/PlayerImpl.ts
  • tests/NationStructureBehavior.test.ts

@github-project-automation github-project-automation Bot moved this from Development to Final Review in OpenFront Release Management Apr 29, 2026
@evanpelle evanpelle merged commit 7654537 into main Apr 29, 2026
13 checks passed
@evanpelle evanpelle deleted the better-neighbr-finding branch April 29, 2026 00:25
@github-project-automation github-project-automation Bot moved this from Final Review to Complete in OpenFront Release Management Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants