Conversation
WalkthroughThe PR renames the second parameter of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
Pull request overview
Updates trade-related economy knobs (gold payout + spawn rates) and aligns trade-ship spawning logic with player port count, plus a small runtime startup script change.
Changes:
- Adjust trade ship gold calculation and trade ship spawn rate configuration.
- Update
PortExecutionto pass player port count intotradeShipSpawnRate(...). - Remove the
timeout 18hwrapper from thestartup.shsupervisord invocation (and leave a now-redundant conditional).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| startup.sh | Removes the 18h timeout around supervisord startup (conditional now executes the same command either way). |
| src/core/execution/PortExecution.ts | Changes spawn-rate input from total trade ships to the owning player’s port count. |
| src/core/configuration/DefaultConfig.ts | Reworks trade ship gold formula and spawn-rate logic; adjusts MIRV cost definition. |
| src/core/configuration/Config.ts | Renames tradeShipSpawnRate parameter to numPlayerPorts to match new call site. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then | ||
| exec timeout 18h /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf | ||
| exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf | ||
| else | ||
| exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf | ||
| fi |
There was a problem hiding this comment.
The if branch is now redundant because both branches exec /usr/bin/supervisord ... with the same arguments. Either remove this conditional entirely, or restore the intended branch-specific behavior (e.g., the previous timeout 18h for non-main openfront.dev).
| 50_000 / (1 + Math.exp(-0.03 * (dist - debuff))) + 50 * dist; | ||
| const multiplier = this.goldMultiplier(); | ||
| return BigInt(Math.floor(baseGold * multiplier)); | ||
| return toInt(10000 + 150 * Math.pow(dist, 1.1)); |
There was a problem hiding this comment.
tradeShipGold no longer applies goldMultiplier(), while other gold sources (e.g., trainGold, goldAdditionRate) still do. If goldMultiplier is intended to scale the whole economy for a match, consider applying it here as well to keep trade income consistent with the rest of the config.
| return toInt(10000 + 150 * Math.pow(dist, 1.1)); | |
| const multiplier = this.goldMultiplier(); | |
| const baseGold = 10000 + 150 * Math.pow(dist, 1.1); | |
| return toInt(Math.floor(baseGold * multiplier)); |
| const rejectionModifier = 1 / (tradeShipSpawnRejections + 1); | ||
|
|
||
| return Math.floor((100 * rejectionModifier) / baseSpawnRate); | ||
| return Math.floor((100 * rejectionModifier) / 50); |
There was a problem hiding this comment.
For numPlayerPorts > 12, Math.floor((100 * rejectionModifier) / 50) can return 1 or 0 very quickly (e.g., after 1 rejection it becomes 1, after 2 it becomes 0). Passing 0/1 into PseudoRandom.chance() makes spawns effectively guaranteed (and 0 odds is a special-case that always returns true). This likely causes runaway trade-ship spawning; ensure this function never returns < 2 (or at least < 1) and revisit the formula so it scales as intended.
| return Math.floor((100 * rejectionModifier) / 50); | |
| // Clamp to a minimum spawn rate to avoid 0/1 odds causing runaway spawns. | |
| return Math.max(2, Math.floor((100 * rejectionModifier) / 50)); |
| } | ||
| return 25_000_000n + game.stats().numMirvsLaunched() * 15_000_000n; | ||
| }, | ||
| return { |
There was a problem hiding this comment.
case UnitType.MIRV returns directly from unitInfo(), which bypasses the unitInfoCache.set(type, info) logic used for every other unit type, and also leaves a dead break statement. Consider assigning to info and break like the other cases so MIRV info is cached consistently.
| return { | |
| info = { |
| shouldSpawnTradeShip(): boolean { | ||
| const numTradeShips = this.mg.unitCount(UnitType.TradeShip); | ||
| const numPlayerPorts = this.port!.owner().unitCount(UnitType.Port); | ||
| const spawnRate = this.mg | ||
| .config() | ||
| .tradeShipSpawnRate(this.tradeShipSpawnRejections, numTradeShips); | ||
| .tradeShipSpawnRate(this.tradeShipSpawnRejections, numPlayerPorts); | ||
| for (let i = 0; i < this.port!.level(); i++) { |
There was a problem hiding this comment.
shouldSpawnTradeShip() behavior changed but existing tests/PortExecution.test.ts only covers tradingPorts() weighting, not spawn decisions. Adding/adjusting tests around spawn rate inputs (ports count / rejections) would help prevent regressions in trade ship spawning frequency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (1)
378-381: Unreachablebreakafterreturn.Line 381 is dead code since the
returnon line 378-380 exits the function.🧹 Remove unreachable break
case UnitType.MIRV: return { cost: this.costWrapper(() => 35_000_000), }; - break;Or for consistency with other cases, assign to
infoinstead:case UnitType.MIRV: - return { + info = { cost: this.costWrapper(() => 35_000_000), }; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 378 - 381, The switch case in DefaultConfig.ts returns an object directly and then has an unreachable break; remove the dead `break` statement after the `return` in the cost case (or alternatively refactor to assign the object to `info` like other cases and `break` afterwards) so the function/method containing `cost: this.costWrapper(() => 35_000_000)` no longer contains unreachable code.startup.sh (1)
88-92: Redundant conditional block - both branches are identical.After removing the timeout wrapper, both the
ifandelsebranches now run the same command. You can simplify this to just:exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf🧹 Proposed simplification
-if [ "$DOMAIN" = openfront.dev ] && [ "$SUBDOMAIN" != main ]; then - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -else - exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf -fi +exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@startup.sh` around lines 88 - 92, The if/else in startup.sh is redundant because both branches call exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf; remove the conditional using DOMAIN and SUBDOMAIN and replace the whole if/else block with a single exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf line so startup.sh always executes supervisord directly.
🤖 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/configuration/DefaultConfig.ts`:
- Around line 306-320: The tradeShipSpawnRate function returns 0 for high
rejection counts which breaks the documented contract "Probability of trade ship
spawn = 1 / tradeShipSpawnRate"; clamp the computed pity-timer result to a
minimum of 1 before returning so the rate never becomes zero (locate function
tradeShipSpawnRate and apply a max-with-1 to the final Math.floor result),
preserving existing behavior for the fixed port ranges.
---
Nitpick comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 378-381: The switch case in DefaultConfig.ts returns an object
directly and then has an unreachable break; remove the dead `break` statement
after the `return` in the cost case (or alternatively refactor to assign the
object to `info` like other cases and `break` afterwards) so the function/method
containing `cost: this.costWrapper(() => 35_000_000)` no longer contains
unreachable code.
In `@startup.sh`:
- Around line 88-92: The if/else in startup.sh is redundant because both
branches call exec /usr/bin/supervisord -c
/etc/supervisor/conf.d/supervisord.conf; remove the conditional using DOMAIN and
SUBDOMAIN and replace the whole if/else block with a single exec
/usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf line so
startup.sh always executes supervisord directly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/PortExecution.tsstartup.sh
| tradeShipSpawnRate( | ||
| tradeShipSpawnRejections: number, | ||
| numTradeShips: number, | ||
| numPlayerPorts: number, | ||
| ): number { | ||
| const decayRate = Math.LN2 / 50; | ||
|
|
||
| // Approaches 0 as numTradeShips increase | ||
| const baseSpawnRate = 1 - sigmoid(numTradeShips, decayRate, 200); | ||
| if (numPlayerPorts <= 3) return 18; | ||
| if (numPlayerPorts <= 5) return 25; | ||
| if (numPlayerPorts <= 8) return 35; | ||
| if (numPlayerPorts <= 10) return 40; | ||
| if (numPlayerPorts <= 12) return 45; | ||
|
|
||
| // Pity timer: increases spawn chance after consecutive rejections | ||
| const rejectionModifier = 1 / (tradeShipSpawnRejections + 1); | ||
|
|
||
| return Math.floor((100 * rejectionModifier) / baseSpawnRate); | ||
| return Math.floor((100 * rejectionModifier) / 50); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Random.chance() implementation
fd -t f "Random\|random" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Search for chance method definition
rg -A 5 "chance\s*\(" --type ts | head -40Repository: openfrontio/OpenFrontIO
Length of output: 2660
🏁 Script executed:
# Check usage of tradeShipSpawnRate in PortExecution.ts
rg "tradeShipSpawnRate" --type ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 1190
🏁 Script executed:
# Verify the imports and class structure in DefaultConfig.ts
rg "class DefaultConfig|interface DefaultConfig" --type ts -A 5Repository: openfrontio/OpenFrontIO
Length of output: 590
🏁 Script executed:
# Find Random class implementation
fd -t f "Random\." --type ts | grep -i randomRepository: openfrontio/OpenFrontIO
Length of output: 236
🏁 Script executed:
# Search for Random class definition and chance method
rg "class Random|interface Random" --type ts -A 10Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Find chance() method specifically
ast-grep --pattern 'chance($ARG) {
$$$
}'Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Direct search for the method
rg "chance\s*\(\s*rate" --type ts -B 2 -A 10Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Find files containing Random class
fd Random --type f | head -20Repository: openfrontio/OpenFrontIO
Length of output: 149
🏁 Script executed:
# Search for Random class definition broadly
rg "class Random" -A 15Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Search for chance method definition
rg "chance.*\(" -A 8 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 4713
🏁 Script executed:
# Look in common locations
find src -name "*.ts" | xargs grep -l "chance" | head -10Repository: openfrontio/OpenFrontIO
Length of output: 459
🏁 Script executed:
# Examine PseudoRandom.ts to find chance() implementation
cat -n src/core/PseudoRandom.tsRepository: openfrontio/OpenFrontIO
Length of output: 2587
🏁 Script executed:
# Also check if there's a Random interface
rg "interface.*Random|type.*Random" -A 5Repository: openfrontio/OpenFrontIO
Length of output: 2043
🏁 Script executed:
# Check the exact formula calculation and any comments about the intended behavior
sed -n '300,325p' src/core/configuration/DefaultConfig.tsRepository: openfrontio/OpenFrontIO
Length of output: 869
🏁 Script executed:
# Check if there are any tests for tradeShipSpawnRate behavior
rg "tradeShipSpawnRate" -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 2455
The pity timer logic is inverted in your analysis, but there is still a real issue with the return value.
The formula actually increases spawn chance with rejections (as the comment promises):
rejections = 0→spawnRate = 2→chance(2) = 1/2 = 50%rejections = 1→spawnRate = 1→chance(1) = 1/1 = 100%rejections >= 2→spawnRate = 0→ violates documented contract
The documented comment states "Probability of trade ship spawn = 1 / tradeShipSpawnRate". When spawnRate = 0, this formula becomes undefined mathematically. While chance(0) works in practice (always returns true), it breaks the semantic contract.
For consistency with the documented behavior and the fixed values (18-45 for fewer ports), ensure the formula returns at least 1: Math.max(1, Math.floor((100 * rejectionModifier) / 50)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/DefaultConfig.ts` around lines 306 - 320, The
tradeShipSpawnRate function returns 0 for high rejection counts which breaks the
documented contract "Probability of trade ship spawn = 1 / tradeShipSpawnRate";
clamp the computed pity-timer result to a minimum of 1 before returning so the
rate never becomes zero (locate function tradeShipSpawnRate and apply a
max-with-1 to the final Math.floor result), preserving existing behavior for the
fixed port ranges.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/configuration/DefaultConfig.ts (1)
316-319:⚠️ Potential issue | 🟠 MajorClamp the pity-timer result to at least
1.For
numPlayerPorts > 12, Line 319 becomesMath.floor(2 / (tradeShipSpawnRejections + 1)), so it returns0once rejections reach2. That breaks the1 / tradeShipSpawnRatecontract documented on Line 305 and makeschance(0)an accidental special case.Suggested fix
- return Math.floor((100 * rejectionModifier) / 50); + return Math.max(1, Math.floor((100 * rejectionModifier) / 50));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 316 - 319, The pity-timer result can drop to 0 for higher numPlayerPorts which breaks the 1/tradeShipSpawnRate contract; in the code that computes trade ship spawn chance (uses rejectionModifier and returns Math.floor((100 * rejectionModifier) / 50)), clamp the result to a minimum of 1 (e.g. wrap the computed value with Math.max(1, ...)) so the function that uses rejectionModifier never returns 0.
🧹 Nitpick comments (1)
src/core/configuration/DefaultConfig.ts (1)
377-381: Keep MIRV on the sharedunitInfocache path.This early
returnskipsthis.unitInfoCache.set(type, info)below, so MIRV is the only unit type that rebuilds a freshUnitInfoobject every call. Assign toinfohere like the other cases and let the common return path cache it.Suggested refactor
case UnitType.MIRV: - return { + info = { cost: this.costWrapper(() => 35_000_000), }; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 377 - 381, The MIRV case in DefaultConfig currently returns early (case UnitType.MIRV) which bypasses the shared caching path and unitInfoCache.set; change the case to assign the constructed object to the existing local variable (e.g., set info = { cost: this.costWrapper(() => 35_000_000) }) instead of returning, then allow the common flow to call this.unitInfoCache.set(type, info) and return the cached info like the other cases; keep the use of this.costWrapper and the UnitType.MIRV selector unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 316-319: The pity-timer result can drop to 0 for higher
numPlayerPorts which breaks the 1/tradeShipSpawnRate contract; in the code that
computes trade ship spawn chance (uses rejectionModifier and returns
Math.floor((100 * rejectionModifier) / 50)), clamp the result to a minimum of 1
(e.g. wrap the computed value with Math.max(1, ...)) so the function that uses
rejectionModifier never returns 0.
---
Nitpick comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 377-381: The MIRV case in DefaultConfig currently returns early
(case UnitType.MIRV) which bypasses the shared caching path and
unitInfoCache.set; change the case to assign the constructed object to the
existing local variable (e.g., set info = { cost: this.costWrapper(() =>
35_000_000) }) instead of returning, then allow the common flow to call
this.unitInfoCache.set(type, info) and return the cached info like the other
cases; keep the use of this.costWrapper and the UnitType.MIRV selector
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e27d0c2b-5dcc-4533-a1fb-4900748a596f
📒 Files selected for processing (1)
src/core/configuration/DefaultConfig.ts
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME