fix/4745 - Fixes various bugs with SAM targeting, range, miscalculations, and nuke pathing.

[JB940]

Stop SAM from targeting detonation tile. Add heuristic to determine which missiles to focus on. Fix bug discarding extra distance traveled in parabola. Fix SAM Missile not moving for a tick. Make SAMExecution source of truth for interception - missiles confirm first they should not be intercepted. Add detection code for missiles landing in SAM range despite never being interceptable.

Before opening a PR: discuss new features on Discord first, and file bugs or small improvements as issues. You must be assigned to an approved issue — unsolicited PRs will be auto-closed.

Add approved & assigned issue number here:

Resolves #4745

Description:

This PR solves various edge case bugs that can consistently be abused. This pr fixes bullet point 1, and 3 through 8.

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

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

JB940

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 49471d99-3c65-4b45-bb24-5710514c7c7a

📥 Commits

Reviewing files that changed from the base of the PR and between 6915b28 and 57062f0.

📒 Files selected for processing (1)

• src/core/execution/NukeExecution.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/core/execution/NukeExecution.ts

---

Walkthrough

SAM interception now evaluates and launches multiple ordered targets, including last-moment and simultaneous interceptions. Missile construction carries its target explicitly. Nuke completion checks nearby interceptors, and curve sampling preserves spacing overflow. Tests cover these interception and path-length changes.

Changes

SAM interception flow

Layer / File(s)	Summary
Target selection and interception timing src/core/execution/SAMLauncherExecution.ts	Interception timing excludes post-explosion tiles, supports detonation-tile fallback, and returns sorted multiple targets.
Multi-target launch and missile contracts src/core/execution/SAMLauncherExecution.ts, src/core/execution/SAMMissileExecution.ts, src/core/game/Game.ts	SAM launchers fire ordered targets until cooldown, and missile construction requires its target unit.
Nuke completion and path spacing src/core/execution/NukeExecution.ts, src/core/utilities/Line.ts	Nukes check nearby enemy SAM missiles before detonation, and curve sampling preserves excess distance.
SAM and trajectory validation tests/core/executions/SAMLauncherExecution.test.ts, tests/core/pathfinding/UniversalPathFinding.Parabola.test.ts	Tests cover prioritization, simultaneous warheads, edge-range interception, and revised path lengths.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GameTick
  participant SAMTargetingSystem
  participant SAMLauncherExecution
  participant SAMMissileExecution
  participant NukeExecution

  GameTick->>SAMTargetingSystem: compute valid interception targets
  SAMTargetingSystem-->>SAMLauncherExecution: return sorted targets
  SAMLauncherExecution->>SAMMissileExecution: launch missiles with target units
  SAMMissileExecution-->>NukeExecution: intercept targeted nukes
  NukeExecution->>NukeExecution: check completion detonation condition

Loading

Possibly related PRs

• openfrontio/OpenFrontIO#4637: Related SAM target selection and multi-warhead launch behavior.
• openfrontio/OpenFrontIO#4638: Related multi-target SAM interception and cooldown handling.
• openfrontio/OpenFrontIO#4753: Related SAM targeting, missile targets, and nuke detonation timing.

Poem

Nukes arc through skies of blue,
SAMs find more than one to do.
Missiles chase each chosen flight,
Curve points keep their spacing right.
At the edge, timing wins the fight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Most #4745 goals are covered, but the PR explicitly skips keeping SAM missiles alive after SAM destruction.	Either implement SAM missile persistence after SAM destruction or split that gameplay change into a separate issue and update #4745 scope.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main SAM targeting and nuke pathing fixes in this PR.
Description check	✅ Passed	The description matches the changeset and describes the SAM and nuke behavior fixes.
Out of Scope Changes check	✅ Passed	The changes stay focused on SAM targeting, interception, and nuke pathing fixes from the linked issue.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

---

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

[JB940]

Please advice when reviewing this PR on the following:

1. it did not implement keeping missiles alive after SAM destruction, as that is a gameplay change, not just a fix. Should this be added?

2. Bullet point 2 has some problems. The SAMs do properly target mirv warheads and properly calculate how to hit them. this is okay for the vast majority of cases. However they can still go through due to the top/verticality of the map issues. The distance from MIRV missile explosion (where it spawns warheads) to spots within enemy territory are sometimes so close they are logically and realistically unable to be intercepted.

   • Can either be kept as is, shooting MIRVs vertically is just better and is a 'skill'.
   • we can make SAMs 'cheat' and prevent the missile from exploding - just pretend it's intercepted properly.
   • We can force "underhand throw" MIRVs at the top half of the map - thus they will rain from below upwards

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

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

70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove leftover debug log.

console.log("DONE!") looks like a debugging artifact left in the interception success path.

🧹 Proposed fix

       if (result.status === PathStatus.COMPLETE) {
-        console.log("DONE!");
         this.mg.displayMessage(

🤖 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/SAMMissileExecution.ts` at line 70, Remove the leftover
console.log("DONE!") statement from the interception success path in
SAMMissileExecution, leaving the surrounding execution behavior unchanged.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/core/execution/NukeExecution.ts`:
- Around line 263-276: In the COMPLETE-result handling of NukeExecution, only
call this.nuke.move(result.node) when detonation was skipped because a nearby
SAM missile is targeting it. Preserve detonate() for the !shouldBeDestroyed
branch, but prevent execution from falling through to move() after the nuke has
been deleted.

In `@tests/core/executions/SAMLauncherExecution.test.ts`:
- Around line 338-341: Strengthen the SAM interception tests to verify timing
rather than only final inactive state. In
tests/core/executions/SAMLauncherExecution.test.ts:338-341, retain the distant
nuke and assert the dangerous nuke is selected first; at 371-375, assert both
SAM missiles launch in the same tick; at 408-413, assert all MIRV targets are
attacked in the same tick; and at 431-434, verify interception occurs before
detonation, such as by asserting no blast effect appears on the target tile.

---

Nitpick comments:
In `@src/core/execution/SAMMissileExecution.ts`:
- Line 70: Remove the leftover console.log("DONE!") statement from the
interception success path in SAMMissileExecution, leaving the surrounding
execution behavior unchanged.

🪄 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 Plus

Run ID: 99b74c62-8971-4437-b71e-53cd13af94a3

📥 Commits

Reviewing files that changed from the base of the PR and between 02bf82d and 415795a.

📒 Files selected for processing (7)

• src/core/execution/NukeExecution.ts
• src/core/execution/SAMLauncherExecution.ts
• src/core/execution/SAMMissileExecution.ts
• src/core/game/Game.ts
• src/core/utilities/Line.ts
• tests/core/executions/SAMLauncherExecution.test.ts
• tests/core/pathfinding/UniversalPathFinding.Parabola.test.ts

[evanpelle]

From Claude:

• NukeExecution detonation-hold check: .length === 1 should be >= 1 — if two missiles ever target the same nuke, it detonates. It's only safe today via the targetedBySAM invariant, which nothing local enforces.
• Magic radius 10: the inbound missile heads to trajectory[len-2], which is up to nukeSpeed from dst (22 for MIRV warheads), and the missile can still be a full move (12) short of it — so a legit interceptor can be 20–34 tiles away and the nuke detonates anyway. Derive it from defaultSamMissileSpeed() + nukeSpeed(type) instead.
• Comment accuracy: "Nuke Execution happens before SAM execution" isn't true in general — a SAM launcher built before the launch ticks before the NukeExecution. The real invariant is that the SAMMissileExecution (appended at fire time) ticks after the nuke, which is what the len - 2 cutoff guards. Worth rewording.
• Test gaps: the detonation-hold branch is never exercised, and the "SAM range edge" test doesn't hit the new fallback (its in-flight tiles at x=65/53/41 are within samRange 70, so the normal loop finds the interception). A test where only the landing tile is in range would cover the new block. Also the new nukes have no NukeExecution so reachedTarget() is vacuously false — wasDestroyedByEnemy() is doing all the work.
• Nit: in the fallback, samTickToReach <= explosionTick and tickBeforeShooting >= 0 are the same condition (nukeTickToReach === explosionTick there) — keep one.
• Heads-up, not a blocker: hydro no longer categorically outranks atom (an atom landing on the silo beats a distant hydro), and detection radius ×2 = 4× scanned area per SAM per tick. Both seem fine, just flagging as deliberate changes.

[JB940]

From Claude:

* **`NukeExecution` detonation-hold check:** `.length === 1` should be `>= 1` — if two missiles ever target the same nuke, it detonates. It's only safe today via the `targetedBySAM` invariant, which nothing local enforces.

* **Magic radius `10`:** the inbound missile heads to `trajectory[len-2]`, which is up to `nukeSpeed` from dst (22 for MIRV warheads), and the missile can still be a full move (12) short of it — so a legit interceptor can be 20–34 tiles away and the nuke detonates anyway. Derive it from `defaultSamMissileSpeed() + nukeSpeed(type)` instead.

* **Comment accuracy:** "Nuke Execution happens before SAM execution" isn't true in general — a SAM launcher built before the launch ticks _before_ the NukeExecution. The real invariant is that the _SAMMissileExecution_ (appended at fire time) ticks after the nuke, which is what the `len - 2` cutoff guards. Worth rewording.

* **Test gaps:** the detonation-hold branch is never exercised, and the "SAM range edge" test doesn't hit the new fallback (its in-flight tiles at x=65/53/41 are within samRange 70, so the normal loop finds the interception). A test where only the landing tile is in range would cover the new block. Also the new nukes have no NukeExecution so `reachedTarget()` is vacuously false — `wasDestroyedByEnemy()` is doing all the work.

* **Nit:** in the fallback, `samTickToReach <= explosionTick` and `tickBeforeShooting >= 0` are the same condition (nukeTickToReach === explosionTick there) — keep one.

* **Heads-up, not a blocker:** hydro no longer categorically outranks atom (an atom landing on the silo beats a distant hydro), and detection radius ×2 = 4× scanned area per SAM per tick. Both seem fine, just flagging as deliberate changes.

@evanpelle

• NukeExecution check: I can change this, however originally was a deliberate choice. Nukes always first get the targetedBySam flag set by a SAM in any code before the actual SamMissile is fired, and SAMs always check for this. Either way this occuring would be a bug, but the missile being prevented is preferable - so agree.
• Magic radius: so a legit interceptor can be 20–34 tiles away and the nuke detonates anyway I do not believe this to be true. In this commit I've changed the NukeMissile tick function to move the nuke to its target location, even on the explosion tick. This means the nuke is on the tile it explodes at, and the SAM Missile can only ever be defaultSamMissileSpeed() away at maximum, otherwise the SAM has tried to shoot a nuke it could NOT prevent - which is a bug - but it should explode. Do agree with changing it to defaultSamMissileSpeed()
• Comments: Agree, brain lagged that one, should've been SamMissileExecution.
• In the test suites, the SAM range is set to 20. It uses a different config and as such the default range is not 70 - so this test does what it is supposed to. (Took a long time to figure out why this kept failing)
• Sure. Considered it but wasn't confident, thanks Claude.
• By design, the SAM should prioritize living over shooting down more dangerous items, as all the missiles disappear on destroy, and thus trying to shoot at the hydro accomplishes nothing.