Trade route capture blacklist rebase

[scamiv]

rebase of #3600

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:

DISCORD_USERNAME

[coderabbitai]

Walkthrough

This PR introduces a trade-route blocking mechanism to prevent trading between specific ports when a trade ship is captured. The system tracks blocked routes by port pair and expiry tick, allowing port execution logic to exclude blocked routes from trade candidate selection.

Changes

Cohort / File(s)	Summary
Trade Route Control Interface src/core/game/Game.ts	Added two new methods to the Game interface: blockTradeRouteUntil() to schedule a block and isTradeRouteBlocked() to query active blocks.
Trade Route State Management src/core/game/GameImpl.ts	Implemented trade-route blocking via tradeRouteBlockedUntil map, block expiry pruning, hash incorporation, and exported TRADE_ROUTE_BLOCK_DURATION_TICKS constant (100 ticks).
Execution Logic Updates src/core/execution/PortExecution.ts, src/core/execution/TradeShipExecution.ts	Port execution now filters out blocked routes; trade ship execution captures source/destination port IDs and blocks the route when captured by a different owner.
Test Coverage tests/PortExecution.test.ts, tests/core/executions/TradeShipExecution.test.ts	Added comprehensive tests validating trade-route blocking on port selection, expiry behavior, game state hashing, and capture-triggered blocking.

Sequence Diagram

sequenceDiagram
    participant TradeShip as TradeShip Execution
    participant Game as GameImpl
    participant Port as Port Execution

    Note over TradeShip,Port: Normal Trade Route Active
    Port->>Game: isTradeRouteBlocked(src, dst, tick)?
    Game-->>Port: false
    Port->>Port: Include port in trading candidates

    Note over TradeShip,Port: Ship Captured Event
    TradeShip->>Game: blockTradeRouteUntil(src, dst, tick + 100)
    Game->>Game: Store block in tradeRouteBlockedUntil map

    Note over TradeShip,Port: Route Now Blocked
    Port->>Game: isTradeRouteBlocked(src, dst, tick)?
    Game-->>Port: true
    Port->>Port: Skip port from trading candidates

    Note over TradeShip,Port: Block Expires
    Port->>Game: isTradeRouteBlocked(src, dst, tick + 101)?
    Game->>Game: Prune expired block
    Game-->>Port: false
    Port->>Port: Include port in candidates again

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A trade ship falls, its path now sealed 🚢⛓️
For hundred ticks, no deals are wheeled
Port execution learns to see
Which routes are blocked, which stay free
Blocking builds a safer spree ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description references the original PR and mentions the feature scope, but contains unchecked required items and lacks detail about the actual implementation changes made.	Update the description to confirm which checklist items were completed and provide a brief summary of the key implementation details added in this rebase.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Trade route capture blacklist rebase' directly describes the main technical change: implementing a trade route blocking mechanism when ships are captured.
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.}

---

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.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

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

125-143: ⚠️ Potential issue | 🟡 Minor

Blocked ports still influence proximity weighting.

After Line 126-Line 134 skips blocked routes, Line 142 still uses i and ports.length from the unfiltered list. This can accidentally remove proximity bonus from valid routes when earlier entries are blocked.

💡 Suggested fix

-    const weightedPorts: Unit[] = [];
-
-    for (const [i, otherPort] of ports.entries()) {
-      if (
-        this.mg.isTradeRouteBlocked(
-          this.port.id(),
-          otherPort.id(),
-          this.mg.ticks(),
-        )
-      ) {
-        continue;
-      }
+    const eligiblePorts = ports.filter(
+      (otherPort) =>
+        !this.mg.isTradeRouteBlocked(
+          this.port.id(),
+          otherPort.id(),
+          this.mg.ticks(),
+        ),
+    );
+
+    const weightedPorts: Unit[] = [];
+    for (const [i, otherPort] of eligiblePorts.entries()) {
       const expanded = new Array(otherPort.level()).fill(otherPort);
       weightedPorts.push(...expanded);
       const tooClose =
         this.mg.manhattanDist(this.port!.tile(), otherPort.tile()) <
         this.mg.config().tradeShipShortRangeDebuff();
       const closeBonus =
-        i < this.mg.config().proximityBonusPortsNb(ports.length);
+        i < this.mg.config().proximityBonusPortsNb(eligiblePorts.length);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/PortExecution.ts` around lines 125 - 143, The loop in
PortExecution.ts uses the original index `i` and `ports.length` even after
skipping blocked routes, which causes proximity bonuses to be calculated against
the unfiltered list; fix by computing proximity using only non-blocked ports:
either pre-filter `ports` with `this.mg.isTradeRouteBlocked(...)` into a new
array and iterate that, or maintain a separate counter (e.g., `validIndex`) that
increments only when a port is not blocked and use that plus the filtered length
for the `closeBonus`/proximity calculation; update references to `i`,
`ports.length`, and `proximityBonusPortsNb` accordingly so `tooClose` and
`closeBonus` are evaluated against the filtered ordering.

🧹 Nitpick comments (3)

tests/core/executions/TradeShipExecution.test.ts (1)

161-173: Prefer public behavior checks over private map access.

This test depends on (game as any).tradeRouteBlockedUntil, which makes it fragile to internal renames. A spy on game.blockTradeRouteUntil keeps the test stable while validating the same “only once” behavior.

💡 Suggested test style

   it("does not add a second blacklist event when the ship is recaptured", () => {
-    const routeKey = `${srcPort.id()}:${dstPort.id()}`;
+    const blockSpy = vi.spyOn(game, "blockTradeRouteUntil");
     tradeShip.owner = vi.fn(() => pirate);
     tradeShipExecution.tick(1);
-    const blockedUntil = (game as any).tradeRouteBlockedUntil.get(routeKey);

     tradeShip.owner = vi.fn(() => origOwner);
     tradeShipExecution.tick(2);

-    expect((game as any).tradeRouteBlockedUntil.get(routeKey)).toBe(
-      blockedUntil,
-    );
+    expect(blockSpy).toHaveBeenCalledTimes(1);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/core/executions/TradeShipExecution.test.ts` around lines 161 - 173, The
test is asserting internal state via (game as any).tradeRouteBlockedUntil;
instead, spy on the public method game.blockTradeRouteUntil and verify it is
only invoked once for the same route when the ship is recaptured: set a spy/mock
on game.blockTradeRouteUntil before calling tradeShipExecution.tick(1), capture
the routeKey argument or call count, then change tradeShip.owner and call
tradeShipExecution.tick(2) and assert the spy was not called a second time (or
was called exactly once with the expected routeKey). Locate uses of
tradeShipExecution.tick, tradeShip.owner, and the routeKey computation to
implement the spy-based assertions.

tests/PortExecution.test.ts (1)

156-159: Avoid mutating private _ticks directly in tests.

Directly changing _ticks makes these tests less realistic and more brittle. Prefer advancing time through executeNextTick() so normal game-side effects remain in play.

💡 Suggested helper

+const advanceTicks = (game: Game, count: number) => {
+  for (let i = 0; i < count; i++) {
+    game.executeNextTick();
+  }
+};

-    (game as any)._ticks += 1;
+    advanceTicks(game, 1);

-    (game as any)._ticks += 100;
+    advanceTicks(game, 100);

Also applies to: 181-183, 199-200

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/PortExecution.test.ts` around lines 156 - 159, Tests are directly
mutating the private field _ticks (e.g., (game as any)._ticks += 1) which is
brittle; instead advance the game clock via the public tick mechanism by calling
executeNextTick() (or add a small test helper that calls executeNextTick() N
times) before asserting game.isTradeRouteBlocked(port.id(), blockedPort.id(),
game.ticks()); update all occurrences (including the ones noted around lines
181–183 and 199–200) to use executeNextTick() so side effects and invariants are
preserved.

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

10-10: Reduce coupling: avoid importing constants from GameImpl here.

TradeShipExecution mostly targets the Game interface; pulling this constant from GameImpl ties execution code to a concrete implementation. Consider moving the constant to src/core/game/Game.ts (or a small shared constants module).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/execution/TradeShipExecution.ts` at line 10, TradeShipExecution is
importing TRADE_ROUTE_BLOCK_DURATION_TICKS from GameImpl which couples execution
logic to a concrete implementation; move the constant into the shared Game
interface or a small constants module and update references. Specifically,
define TRADE_ROUTE_BLOCK_DURATION_TICKS in src/core/game/Game.ts (or a new
src/core/game/constants.ts) and then change the import in TradeShipExecution to
import from the Game interface module (or constants module) instead of GameImpl;
ensure any other modules that used the constant are updated to the new location
and exported from the Game interface module if needed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/core/execution/PortExecution.ts`:
- Around line 125-143: The loop in PortExecution.ts uses the original index `i`
and `ports.length` even after skipping blocked routes, which causes proximity
bonuses to be calculated against the unfiltered list; fix by computing proximity
using only non-blocked ports: either pre-filter `ports` with
`this.mg.isTradeRouteBlocked(...)` into a new array and iterate that, or
maintain a separate counter (e.g., `validIndex`) that increments only when a
port is not blocked and use that plus the filtered length for the
`closeBonus`/proximity calculation; update references to `i`, `ports.length`,
and `proximityBonusPortsNb` accordingly so `tooClose` and `closeBonus` are
evaluated against the filtered ordering.

---

Nitpick comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Line 10: TradeShipExecution is importing TRADE_ROUTE_BLOCK_DURATION_TICKS from
GameImpl which couples execution logic to a concrete implementation; move the
constant into the shared Game interface or a small constants module and update
references. Specifically, define TRADE_ROUTE_BLOCK_DURATION_TICKS in
src/core/game/Game.ts (or a new src/core/game/constants.ts) and then change the
import in TradeShipExecution to import from the Game interface module (or
constants module) instead of GameImpl; ensure any other modules that used the
constant are updated to the new location and exported from the Game interface
module if needed.

In `@tests/core/executions/TradeShipExecution.test.ts`:
- Around line 161-173: The test is asserting internal state via (game as
any).tradeRouteBlockedUntil; instead, spy on the public method
game.blockTradeRouteUntil and verify it is only invoked once for the same route
when the ship is recaptured: set a spy/mock on game.blockTradeRouteUntil before
calling tradeShipExecution.tick(1), capture the routeKey argument or call count,
then change tradeShip.owner and call tradeShipExecution.tick(2) and assert the
spy was not called a second time (or was called exactly once with the expected
routeKey). Locate uses of tradeShipExecution.tick, tradeShip.owner, and the
routeKey computation to implement the spy-based assertions.

In `@tests/PortExecution.test.ts`:
- Around line 156-159: Tests are directly mutating the private field _ticks
(e.g., (game as any)._ticks += 1) which is brittle; instead advance the game
clock via the public tick mechanism by calling executeNextTick() (or add a small
test helper that calls executeNextTick() N times) before asserting
game.isTradeRouteBlocked(port.id(), blockedPort.id(), game.ticks()); update all
occurrences (including the ones noted around lines 181–183 and 199–200) to use
executeNextTick() so side effects and invariants are preserved.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 40b51943-5536-4072-917e-5ec2a3de3553

📥 Commits

Reviewing files that changed from the base of the PR and between 1ebac8e and 9e2695d.

📒 Files selected for processing (6)

• src/core/execution/PortExecution.ts
• src/core/execution/TradeShipExecution.ts
• src/core/game/Game.ts
• src/core/game/GameImpl.ts
• tests/PortExecution.test.ts
• tests/core/executions/TradeShipExecution.test.ts