pokebedrock/pokebedrock-showdown
4
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages Activity Actions

Huge AI Improvements #12

Merged
smell-of-curry merged 3 commits from huge-ai-improvements into master 2026-07-21 21:31:14 +00:00
Conversation 33 Commits 3 Files changed 16 +2286 -675
smell-of-curry commented 2026-06-10 18:46:53 +00:00 (Migrated from github.com)
Copy link

Stronger Strategic AI: ~65% winrate at d5 vs d3, fix randombattle crash

Summary

This PR overhauls the strategic-ai battle engine so higher difficulty levels are measurably stronger than lower ones, fixes a randombattle generation crash caused by host-side cosmetic skins, and adds a self-play harness so future AI changes can be validated empirically instead of by feel.

Headline numbers (1000-game seeded series, gen9randombattle, side-swapped):

Matchup Winrate (decisive)
d5 vs d3 65.2% (was ~50–58% pre-PR)
d5 vs d4 64.3%
d4 vs d2 59.3%

A clean monotonic difficulty gradient, with the top tier now clearly separating from the heuristic core.

What changed

1. Tracker is no longer starving the engines (the biggest single bug)

Before: BattleStateTracker only saw protocol log lines that flowed through PlayerAI.receiveLine. The PokeBedrock host bypasses that path (it has its own log interpreter), so engines were running blind to weather, hazards, boosts, status, revealed moves, and lastMoveFailed.

  • New PlayerAI.ingestLogLine(line) API: feed the tracker without dispatching choices.
  • New Battle.forwardLogLineToAI hook in pokebedrock-beh: routes every protocol line to its actor (with |split| privacy preserved).
  • EngineContext.lastMoveFailedByMon now mirrors tracker state, so e.g. Stomping Tantrum doubles BP correctly after a fizzled Earthquake into Levitate.

2. MCTS tier (d5) does real multi-turn search

MctsEngine was a one-ply sampler in disguise. It now:

  • Forks the live Battle via toJSON() / fromJSON() and plays out a few greedy turns against a determinized foe team.
  • Samples a fresh determinization per rollout (weighted, without replacement) from a wider inferred-move pool, so visits average over many plausible foe sets instead of anchoring on one guess.
  • Uses a mostly-greedy foe reply model at the root (real opponents — including our own d3 — play near-greedy).
  • Evaluates fork end-states with a richer scoring function: HP + alive bonus plus status penalty, net stat-boost stages, and entry-hazard pressure (rocks/spikes/tspikes/web).
  • Blends rollout score with the heuristic baseline (rather than overriding it) on a visit-count curve up to 60% rollout weight, so noisy rollouts can break ties but can't overturn a confident heuristic read.

3. MCTS now searches over switches, not just moves

MctsEngine.maybeSwitchOut wraps the heuristic switch gate with rollout verification:

  • Heuristic says switch → vetoed when staying clearly wins (e.g. "we KO first" lines the matchup math misses).
  • Heuristic says stay in a losing matchup → overridden when rollouts strongly favor the best bench mon.
  • Asymmetric margins (12 to veto, 18 to override) and the existing switch lock prevent ping-pong.

This was the missing structural piece — switch decisions used to be 100% heuristic regardless of difficulty.

4. Disciplined play at high difficulty

A diagnostic run (d5 with search disabled) only scored 52.4% vs d3, proving the heuristic core was injecting handicaps even at max difficulty:

  • 15% chance per turn of a random Protect probe — vetoed at d4–d5.
  • Forced anti-staleness rotation that demoted the best move when the second-best was within 90% — vetoed at d4–d5 (repeating Earthquake is usually correct).
  • d5 ladder cap dropped from 0.12 → 0 (fully greedy).

Lower tiers keep all three for human-feeling variety.

5. Pokerogue-style switch margins replacing a coin flip

The old switch gate was a (difficulty - 1) / 4 coin flip — even an "elite" trainer ignored half their correct switches at d3 and added pure noise at d4–d5. Now: a voluntary switch must beat the current matchup by a difficulty-scaled margin, raised further by a switchCount counter that climbs each time we switch and floors back down each time we commit to a move.

6. Move selection ladder replacing epsilon-uniform noise

selectByScoreLadder walks the descending-sorted move list and advances to the next-best with probability proportional to closeness, capped per difficulty. Variety is now strength-preserving: a close second is a frequent pick, a distant third is rare, a negative-scored move is never reached from a positive run.

7. OpponentInference upgrades

  • Move priors are now filtered by gen and level legality (parses learnset sources like 9L36 / 9M), so e.g. a level-50 Garchomp's prior pool no longer includes moves it can't yet have learned.
  • New inferredImmunities field on TrackedPokemon: when a foe's attack is logged as |-immune| against a type the dex didn't predict, the inference layer now uses that to upgrade ability/item guesses (Levitate, Storm Drain, Air Balloon, etc.).
  • Crit / SE / resisted log events feed the tracker's per-mon lastMoveFailed flag.

8. Damage calc gaps closed

  • Toxic Spikes: correct fraction (1/16 / 1/8) and Poison-immune absorb logic.
  • Weight-based BP (Low Kick, Grass Knot, Heavy Slam, Heat Crash) and speed-based BP (Gyro Ball, Electro Ball) now use exact tables instead of a sigmoid.
  • Last Respects scales with fainted party members.
  • Doubles spread reduction flagged through to the calc.

9. Mega/Tera matchup-aware transforms

TransformPolicy.considerMega now compares the current matchup score with the projected mega forme's score and defers transformation when significantly worse — no more "Mega evolved into a 4× weakness because the move score said attack."

10. Random-team generator no longer crashes on host-side skins

PokeBedrock event skins (Torterra-StPatrick, Sableye-Sculk, Mimikyu-Raichu, etc.) are registered as cosmeticFormes but have no dex entries/aliases. Vanilla Showdown's random team generator deliberately rolls a random cosmetic forme for variety, which previously crashed battle creation:

Error: Unidentified species: torterrastpatrick

getForme in gen8, gen9, and gen9baby/teams.ts now filters cosmetic forme samples through dex.species.get(forme).exists, so unresolvable host skins are silently dropped while real cosmetic formes (e.g. Pikachu-Original, Gastrodon-East) still rotate as before. Errored game count in self-play went from ~6% to 0%.

11. New: AI-vs-AI self-play harness

Added sim/tools/selfplay.ts. Pits two PlayerAI configurations against each other, side-swapped per game, with a worker_threads pool for parallelism (default: cores − 1).

Usage:

node dist/sim/tools/selfplay.js --a 5 --b 3 --games 1000 --quiet --seed 1234
node dist/sim/tools/selfplay.js --help

A 1000-game series now finishes in ~7 min on an M-series MacBook (was ~2h single-threaded). Per-game seeds are pre-derived from the run seed, so results are reproducible regardless of pool size or completion order. Includes a per-game wall-clock watchdog so a single deadlocked battle can't kill the series silently.

What players in pokebedrock-beh will notice

Once pokebedrock-beh picks up the new package, in-game AI behaviour should change visibly:

  • Trainer fights feel sharper at higher difficulties. Gym leaders, Elite Four, and other boss trainers (which now floor to high difficulty) will pick the best damaging move much more consistently, switch into resists at the right moments, and stop switching when staying is clearly correct (the old gate had them flip-flopping between two mons).
  • Fewer "wait, why did it click Protect?" moments. The 15% random Protect probe is gone at d4–d5; Protect is now only used when the move evaluator actually scores it well (e.g. scouting a Choice user, stalling poison, opposing partner setup).
  • Mega evolutions and Terastallizing happen at sane times. No more "Mega'd into the 4× weakness." The transform policy now checks the post-transform matchup score and defers when it's worse.
  • Harder boss/elite trainers via better team building. Boss trainer parties are difficulty-scaled (EVs, natures, competitive movesets, held items) and floor to a minimum difficulty.
  • No more Unidentified species crashes when a randombattle-format trainer rolls into a battle that would have previously selected a host skin.
  • Lower difficulties feel less random, not less smart. Difficulty 0–3 keeps human-feeling variety (move ladder, occasional Protect, anti-staleness rotation), but the variety now comes from picking among reasonable options rather than rolling dice on whether to ignore a correct switch.
  • Status/boost/hazard awareness. With the tracker now actually receiving log lines, AI decisions take into account weather/terrain, screens, hazards, opponent boosts, and burned/paralyzed mons — none of which were reaching the engines before.

The lower tiers were intentionally not hardened; players choosing easier opponents should still see plenty of suboptimal play, just suboptimal in a more intentional way.

Test plan

  • node build clean.
  • node dist/sim/tools/selfplay.js --a 5 --b 3 --games 1000 --quiet --seed 1234,5678,9012,3456 → 65.2% (target was ≥65%).
  • Difficulty gradient verified: d5 > d4 > d2 > d0 holds in 400-game runs.
  • No Unidentified species errors across 1000+ games of gen9randombattle.
  • Self-play watchdog: a deliberately deadlocked game is force-tied and the series completes.
  • Smoke-test in pokebedrock-beh against an Elite Four trainer (visual) — recommended before merge.

Risks / follow-ups

  • The MCTS fork path requires Battle.toJSON() / fromJSON() and a host that exposes the live battle to the AI; falls back to the tracker-only sampler when unavailable. (Bedrock host wires this up; the self-play harness does too.)
  • searchBudgetMs still defaults to 200ms at d5. Increasing to 500ms didn't change winrate within noise — the ceiling is rollout quality, not quantity. Future work: smarter rollout policy (use the real evaluator inside the fork instead of greedy max-BP).
  • Doubles MCTS search isn't modelled (slot pairing is non-trivial); doubles still uses the heuristic engine.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented Random Battles from selecting cosmetic formes that can’t be resolved in the in-game dex (including baby variants).

  • New Features

    • Added a deterministic AI self-play harness with a runnable CLI and optional worker-thread parallel execution.
    • Upgraded strategic AI with fork-based rollouts and ladder-style move selection, plus improved Mega/Ultra decision-making.

  • Improvements

    • Refined switching logic, move-failure tracking, and hazard/damage estimations.
    • Improved opponent inference using inferred immunities and level-accurate learnset availability.
# Stronger Strategic AI: ~65% winrate at d5 vs d3, fix randombattle crash ## Summary This PR overhauls the `strategic-ai` battle engine so higher difficulty levels are *measurably* stronger than lower ones, fixes a randombattle generation crash caused by host-side cosmetic skins, and adds a self-play harness so future AI changes can be validated empirically instead of by feel. Headline numbers (1000-game seeded series, `gen9randombattle`, side-swapped): | Matchup | Winrate (decisive) | |---|---| | **d5 vs d3** | **65.2%** (was ~50–58% pre-PR) | | d5 vs d4 | 64.3% | | d4 vs d2 | 59.3% | A clean monotonic difficulty gradient, with the top tier now clearly separating from the heuristic core. ## What changed ### 1. Tracker is no longer starving the engines (the biggest single bug) Before: `BattleStateTracker` only saw protocol log lines that flowed through `PlayerAI.receiveLine`. The PokeBedrock host bypasses that path (it has its own log interpreter), so engines were running blind to weather, hazards, boosts, status, revealed moves, and `lastMoveFailed`. - New `PlayerAI.ingestLogLine(line)` API: feed the tracker without dispatching choices. - New `Battle.forwardLogLineToAI` hook in `pokebedrock-beh`: routes every protocol line to its actor (with `|split|` privacy preserved). - `EngineContext.lastMoveFailedByMon` now mirrors tracker state, so e.g. Stomping Tantrum doubles BP correctly after a fizzled Earthquake into Levitate. ### 2. MCTS tier (d5) does real multi-turn search `MctsEngine` was a one-ply sampler in disguise. It now: - Forks the live `Battle` via `toJSON()` / `fromJSON()` and plays out a few greedy turns against a determinized foe team. - Samples a **fresh determinization per rollout** (weighted, without replacement) from a wider inferred-move pool, so visits average over many plausible foe sets instead of anchoring on one guess. - Uses a **mostly-greedy foe reply model** at the root (real opponents — including our own d3 — play near-greedy). - Evaluates fork end-states with a richer scoring function: HP + alive bonus *plus* status penalty, net stat-boost stages, and entry-hazard pressure (rocks/spikes/tspikes/web). - **Blends** rollout score with the heuristic baseline (rather than overriding it) on a visit-count curve up to 60% rollout weight, so noisy rollouts can break ties but can't overturn a confident heuristic read. ### 3. MCTS now searches over **switches**, not just moves `MctsEngine.maybeSwitchOut` wraps the heuristic switch gate with rollout verification: - Heuristic says **switch** → vetoed when staying clearly wins (e.g. "we KO first" lines the matchup math misses). - Heuristic says **stay** in a losing matchup → overridden when rollouts strongly favor the best bench mon. - Asymmetric margins (12 to veto, 18 to override) and the existing switch lock prevent ping-pong. This was the missing structural piece — switch decisions used to be 100% heuristic regardless of difficulty. ### 4. Disciplined play at high difficulty A diagnostic run (d5 with search disabled) only scored 52.4% vs d3, proving the heuristic core was injecting handicaps even at max difficulty: - 15% chance per turn of a **random Protect probe** — vetoed at d4–d5. - **Forced anti-staleness rotation** that demoted the best move when the second-best was within 90% — vetoed at d4–d5 (repeating Earthquake is usually correct). - d5 ladder cap dropped from `0.12` → **`0` (fully greedy)**. Lower tiers keep all three for human-feeling variety. ### 5. Pokerogue-style switch margins replacing a coin flip The old switch gate was a `(difficulty - 1) / 4` coin flip — even an "elite" trainer ignored half their correct switches at d3 and added pure noise at d4–d5. Now: a voluntary switch must beat the current matchup by a difficulty-scaled margin, raised further by a `switchCount` counter that climbs each time we switch and floors back down each time we commit to a move. ### 6. Move selection ladder replacing epsilon-uniform noise `selectByScoreLadder` walks the descending-sorted move list and advances to the next-best with probability proportional to *closeness*, capped per difficulty. Variety is now strength-preserving: a close second is a frequent pick, a distant third is rare, a negative-scored move is never reached from a positive run. ### 7. `OpponentInference` upgrades - Move priors are now filtered by **gen and level legality** (parses learnset sources like `9L36` / `9M`), so e.g. a level-50 Garchomp's prior pool no longer includes moves it can't yet have learned. - New `inferredImmunities` field on `TrackedPokemon`: when a foe's attack is logged as `|-immune|` against a type the dex didn't predict, the inference layer now uses that to upgrade ability/item guesses (Levitate, Storm Drain, Air Balloon, etc.). - Crit / SE / resisted log events feed the tracker's per-mon `lastMoveFailed` flag. ### 8. Damage calc gaps closed - **Toxic Spikes**: correct fraction (1/16 / 1/8) and Poison-immune absorb logic. - **Weight-based BP** (Low Kick, Grass Knot, Heavy Slam, Heat Crash) and **speed-based BP** (Gyro Ball, Electro Ball) now use exact tables instead of a sigmoid. - **Last Respects** scales with fainted party members. - **Doubles spread reduction** flagged through to the calc. ### 9. Mega/Tera matchup-aware transforms `TransformPolicy.considerMega` now compares the current matchup score with the projected mega forme's score and defers transformation when significantly worse — no more "Mega evolved into a 4× weakness because the move score said attack." ### 10. Random-team generator no longer crashes on host-side skins PokeBedrock event skins (`Torterra-StPatrick`, `Sableye-Sculk`, `Mimikyu-Raichu`, etc.) are registered as `cosmeticFormes` but have no dex entries/aliases. Vanilla Showdown's random team generator deliberately rolls a random cosmetic forme for variety, which previously crashed battle creation: ``` Error: Unidentified species: torterrastpatrick ``` `getForme` in `gen8`, `gen9`, and `gen9baby/teams.ts` now filters cosmetic forme samples through `dex.species.get(forme).exists`, so unresolvable host skins are silently dropped while real cosmetic formes (e.g. `Pikachu-Original`, `Gastrodon-East`) still rotate as before. Errored game count in self-play went from ~6% to 0%. ### 11. New: AI-vs-AI self-play harness Added `sim/tools/selfplay.ts`. Pits two `PlayerAI` configurations against each other, side-swapped per game, with a `worker_threads` pool for parallelism (default: cores − 1). Usage: ```bash node dist/sim/tools/selfplay.js --a 5 --b 3 --games 1000 --quiet --seed 1234 node dist/sim/tools/selfplay.js --help ``` A 1000-game series now finishes in ~7 min on an M-series MacBook (was ~2h single-threaded). Per-game seeds are pre-derived from the run seed, so results are reproducible regardless of pool size or completion order. Includes a per-game wall-clock watchdog so a single deadlocked battle can't kill the series silently. ## What players in `pokebedrock-beh` will notice Once `pokebedrock-beh` picks up the new package, in-game AI behaviour should change visibly: - **Trainer fights feel sharper at higher difficulties.** Gym leaders, Elite Four, and other boss trainers (which now floor to high difficulty) will pick the best damaging move much more consistently, switch into resists at the right moments, and *stop* switching when staying is clearly correct (the old gate had them flip-flopping between two mons). - **Fewer "wait, why did it click Protect?" moments.** The 15% random Protect probe is gone at d4–d5; Protect is now only used when the move evaluator actually scores it well (e.g. scouting a Choice user, stalling poison, opposing partner setup). - **Mega evolutions and Terastallizing happen at sane times.** No more "Mega'd into the 4× weakness." The transform policy now checks the *post-transform* matchup score and defers when it's worse. - **Harder boss/elite trainers via better team building.** Boss trainer parties are difficulty-scaled (EVs, natures, competitive movesets, held items) and floor to a minimum difficulty. - **No more `Unidentified species` crashes** when a randombattle-format trainer rolls into a battle that would have previously selected a host skin. - **Lower difficulties feel less random, not less smart.** Difficulty 0–3 keeps human-feeling variety (move ladder, occasional Protect, anti-staleness rotation), but the variety now comes from *picking among reasonable options* rather than rolling dice on whether to ignore a correct switch. - **Status/boost/hazard awareness.** With the tracker now actually receiving log lines, AI decisions take into account weather/terrain, screens, hazards, opponent boosts, and burned/paralyzed mons — none of which were reaching the engines before. The lower tiers were intentionally *not* hardened; players choosing easier opponents should still see plenty of suboptimal play, just suboptimal in a more intentional way. ## Test plan - [x] `node build` clean. - [x] `node dist/sim/tools/selfplay.js --a 5 --b 3 --games 1000 --quiet --seed 1234,5678,9012,3456` → 65.2% (target was ≥65%). - [x] Difficulty gradient verified: d5 > d4 > d2 > d0 holds in 400-game runs. - [x] No `Unidentified species` errors across 1000+ games of `gen9randombattle`. - [x] Self-play watchdog: a deliberately deadlocked game is force-tied and the series completes. - [ ] Smoke-test in `pokebedrock-beh` against an Elite Four trainer (visual) — recommended before merge. ## Risks / follow-ups - The MCTS fork path requires `Battle.toJSON()` / `fromJSON()` and a host that exposes the live battle to the AI; falls back to the tracker-only sampler when unavailable. (Bedrock host wires this up; the self-play harness does too.) - `searchBudgetMs` still defaults to 200ms at d5. Increasing to 500ms didn't change winrate within noise — the ceiling is rollout *quality*, not quantity. Future work: smarter rollout policy (use the real evaluator inside the fork instead of greedy max-BP). - Doubles MCTS search isn't modelled (slot pairing is non-trivial); doubles still uses the heuristic engine. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented Random Battles from selecting cosmetic formes that can’t be resolved in the in-game dex (including baby variants). * **New Features** * Added a deterministic AI self-play harness with a runnable CLI and optional worker-thread parallel execution. * Upgraded strategic AI with fork-based rollouts and ladder-style move selection, plus improved Mega/Ultra decision-making. * **Improvements** * Refined switching logic, move-failure tracking, and hazard/damage estimations. * Improved opponent inference using inferred immunities and level-accurate learnset availability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
coderabbitai[bot] commented 2026-06-10 18:47:16 +00:00 (Migrated from github.com)
Copy link

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9104939c-9012-43c2-a8d7-ac3f10fd0b9b

📥 Commits

Reviewing files that changed from the base of the PR and between 479d35216b09d6658646e6b4b7a65146a3f940d3 and 05d5879e43.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (15)
  • data/random-battles/gen8/teams.ts
  • data/random-battles/gen9/teams.ts
  • data/random-battles/gen9baby/teams.ts
  • package.json
  • sim/tools/player-ai.ts
  • sim/tools/selfplay.ts
  • sim/tools/strategic-ai/engines/Engine.ts
  • sim/tools/strategic-ai/engines/HeuristicEngine.ts
  • sim/tools/strategic-ai/engines/MctsEngine.ts
  • sim/tools/strategic-ai/mechanics/DamageCalc.ts
  • sim/tools/strategic-ai/mechanics/MoveEvaluator.ts
  • sim/tools/strategic-ai/mechanics/TransformPolicy.ts
  • sim/tools/strategic-ai/policy/DifficultyPolicy.ts
  • sim/tools/strategic-ai/state/BattleStateTracker.ts
  • sim/tools/strategic-ai/state/OpponentInference.ts
📝 Walkthrough

Walkthrough

This PR filters random-battle cosmetic formes, updates strategic AI state, inference, damage, transformation, heuristic, and MCTS behavior, adds PlayerAI log ingestion, and introduces deterministic self-play execution with worker and CLI support.

Changes

Strategic AI runtime, inference, and self-play

Layer / File(s) Summary
Dex-validated cosmetic forme sampling
data/random-battles/gen8/teams.ts, data/random-battles/gen9/teams.ts, data/random-battles/gen9baby/teams.ts, package.json		 Cosmetic forme sampling now excludes dex-unresolvable entries, and the package version is updated to 0.1.21.
Engine context and difficulty knob contracts
sim/tools/strategic-ai/engines/Engine.ts, sim/tools/strategic-ai/policy/DifficultyPolicy.ts		 Noise controls are replaced by ladder and switch-margin controls, with optional live-battle access and updated difficulty mappings.
Tracker immunity signals and opponent inference updates
sim/tools/strategic-ai/state/BattleStateTracker.ts, sim/tools/strategic-ai/state/OpponentInference.ts		 Tracking records unexplained immunities and failure state, Toxic Spikes receive modeled cost, and inference uses generation- and level-gated learnsets plus immunity-aware priors.
PlayerAI log ingestion and battle accessor wiring
sim/tools/player-ai.ts		 PlayerAI adds delayed protocol ingestion, shared tracker processing, failure-state synchronization, buffered replay, and live Battle access.
Move evaluation, damage calc, and transform policy
sim/tools/strategic-ai/mechanics/MoveEvaluator.ts, sim/tools/strategic-ai/mechanics/DamageCalc.ts, sim/tools/strategic-ai/mechanics/TransformPolicy.ts		 Move evaluation carries turn damage context, several damage formulas are updated, and Mega or Ultra transformation can be deferred after matchup comparison.
Heuristic ladder picks and switch gating
sim/tools/strategic-ai/engines/HeuristicEngine.ts		 Move selection uses score ladders, switch pressure affects gating, Protect probing updates pressure, and doubles decisions share turn-plan state.
MCTS fork rollout scoring and fork-aware switch validation
sim/tools/strategic-ai/engines/MctsEngine.ts		 MCTS snapshots battles, runs seeded fork rollouts with determinization and greedy continuation, blends rollout rewards, and evaluates switch choices with fork results.
Self-play types and per-game execution flow
sim/tools/selfplay.ts		 Self-play defines public configuration and result types, retries rejected choices, runs seeded games, handles timeouts, and retries early failures.
Worker pool and CLI entrypoints
sim/tools/selfplay.ts		 Self-play aggregates series results, schedules worker lanes with recovery, processes worker messages, parses CLI options, and prints summaries.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Host
  participant PlayerAI
  participant Tracker as BattleStateTracker
  participant EngineCtx as EngineContext
  Host->>PlayerAI: ingestLogLine(line)
  PlayerAI->>PlayerAI: applyTrackedLine(line)
  PlayerAI->>Tracker: applyEvent(event)
  PlayerAI->>PlayerAI: syncLastMoveFailed()
  PlayerAI->>EngineCtx: update lastMoveFailedByMon

sequenceDiagram
  participant MctsEngine
  participant Battle
  participant ForkRollout
  participant ForkState
  MctsEngine->>Battle: takeBattleSnapshot()
  MctsEngine->>ForkRollout: run forkRollout(candidate)
  ForkRollout->>ForkRollout: determinizeMoves()
  ForkRollout->>ForkRollout: playGreedyTurn()
  ForkRollout->>ForkState: evaluateForkState()
  ForkState-->>MctsEngine: rollout reward

sequenceDiagram
  participant CLI
  participant Runner as runSelfPlay
  participant Pool as worker pool
  participant Game as playGame
  CLI->>Runner: parse options and start run
  Runner->>Pool: schedule WorkerTask items
  Pool->>Game: playGameWithRetries()
  Game-->>Pool: GameResult
  Pool-->>Runner: aggregated results
  Runner-->>CLI: summary output

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is too vague and generic to convey the main change in this PR. Use a specific title that names the key changes, such as strategic AI overhaul, randombattle crash fix, and self-play harness.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch huge-ai-improvements

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.

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.

<!-- This is an auto-generated comment: summarize by coderabbit.ai --> <!-- review_stack_entry_start --> [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/smell-of-curry/pokebedrock-showdown/pull/12?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- This is an auto-generated comment: failure by coderabbit.ai --> > [!CAUTION] > ## Review failed > > The pull request is closed. <!-- end of auto-generated comment: failure by coderabbit.ai --> <details> <summary>ℹ️ Recent review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `9104939c-9012-43c2-a8d7-ac3f10fd0b9b` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 479d35216b09d6658646e6b4b7a65146a3f940d3 and 05d5879e43a2fbdeae5f07b38b8b6a3fb878fef5. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (15)</summary> * `data/random-battles/gen8/teams.ts` * `data/random-battles/gen9/teams.ts` * `data/random-battles/gen9baby/teams.ts` * `package.json` * `sim/tools/player-ai.ts` * `sim/tools/selfplay.ts` * `sim/tools/strategic-ai/engines/Engine.ts` * `sim/tools/strategic-ai/engines/HeuristicEngine.ts` * `sim/tools/strategic-ai/engines/MctsEngine.ts` * `sim/tools/strategic-ai/mechanics/DamageCalc.ts` * `sim/tools/strategic-ai/mechanics/MoveEvaluator.ts` * `sim/tools/strategic-ai/mechanics/TransformPolicy.ts` * `sim/tools/strategic-ai/policy/DifficultyPolicy.ts` * `sim/tools/strategic-ai/state/BattleStateTracker.ts` * `sim/tools/strategic-ai/state/OpponentInference.ts` </details> </details> --- <!-- walkthrough_start --> <details> <summary>📝 Walkthrough</summary> ## Walkthrough This PR filters random-battle cosmetic formes, updates strategic AI state, inference, damage, transformation, heuristic, and MCTS behavior, adds PlayerAI log ingestion, and introduces deterministic self-play execution with worker and CLI support. ### Changes **Strategic AI runtime, inference, and self-play** |Layer / File(s)|Summary| |---|---| |**Dex-validated cosmetic forme sampling** <br> `data/random-battles/gen8/teams.ts`, `data/random-battles/gen9/teams.ts`, `data/random-battles/gen9baby/teams.ts`, `package.json`|Cosmetic forme sampling now excludes dex-unresolvable entries, and the package version is updated to `0.1.21`.| |**Engine context and difficulty knob contracts** <br> `sim/tools/strategic-ai/engines/Engine.ts`, `sim/tools/strategic-ai/policy/DifficultyPolicy.ts`|Noise controls are replaced by ladder and switch-margin controls, with optional live-battle access and updated difficulty mappings.| |**Tracker immunity signals and opponent inference updates** <br> `sim/tools/strategic-ai/state/BattleStateTracker.ts`, `sim/tools/strategic-ai/state/OpponentInference.ts`|Tracking records unexplained immunities and failure state, Toxic Spikes receive modeled cost, and inference uses generation- and level-gated learnsets plus immunity-aware priors.| |**PlayerAI log ingestion and battle accessor wiring** <br> `sim/tools/player-ai.ts`|PlayerAI adds delayed protocol ingestion, shared tracker processing, failure-state synchronization, buffered replay, and live Battle access.| |**Move evaluation, damage calc, and transform policy** <br> `sim/tools/strategic-ai/mechanics/MoveEvaluator.ts`, `sim/tools/strategic-ai/mechanics/DamageCalc.ts`, `sim/tools/strategic-ai/mechanics/TransformPolicy.ts`|Move evaluation carries turn damage context, several damage formulas are updated, and Mega or Ultra transformation can be deferred after matchup comparison.| |**Heuristic ladder picks and switch gating** <br> `sim/tools/strategic-ai/engines/HeuristicEngine.ts`|Move selection uses score ladders, switch pressure affects gating, Protect probing updates pressure, and doubles decisions share turn-plan state.| |**MCTS fork rollout scoring and fork-aware switch validation** <br> `sim/tools/strategic-ai/engines/MctsEngine.ts`|MCTS snapshots battles, runs seeded fork rollouts with determinization and greedy continuation, blends rollout rewards, and evaluates switch choices with fork results.| |**Self-play types and per-game execution flow** <br> `sim/tools/selfplay.ts`|Self-play defines public configuration and result types, retries rejected choices, runs seeded games, handles timeouts, and retries early failures.| |**Worker pool and CLI entrypoints** <br> `sim/tools/selfplay.ts`|Self-play aggregates series results, schedules worker lanes with recovery, processes worker messages, parses CLI options, and prints summaries.| **Estimated code review effort:** 5 (Critical) | ~120 minutes ### Sequence Diagram(s) ```mermaid sequenceDiagram participant Host participant PlayerAI participant Tracker as BattleStateTracker participant EngineCtx as EngineContext Host->>PlayerAI: ingestLogLine(line) PlayerAI->>PlayerAI: applyTrackedLine(line) PlayerAI->>Tracker: applyEvent(event) PlayerAI->>PlayerAI: syncLastMoveFailed() PlayerAI->>EngineCtx: update lastMoveFailedByMon ``` ```mermaid sequenceDiagram participant MctsEngine participant Battle participant ForkRollout participant ForkState MctsEngine->>Battle: takeBattleSnapshot() MctsEngine->>ForkRollout: run forkRollout(candidate) ForkRollout->>ForkRollout: determinizeMoves() ForkRollout->>ForkRollout: playGreedyTurn() ForkRollout->>ForkState: evaluateForkState() ForkState-->>MctsEngine: rollout reward ``` ```mermaid sequenceDiagram participant CLI participant Runner as runSelfPlay participant Pool as worker pool participant Game as playGame CLI->>Runner: parse options and start run Runner->>Pool: schedule WorkerTask items Pool->>Game: playGameWithRetries() Game-->>Pool: GameResult Pool-->>Runner: aggregated results Runner-->>CLI: summary output ``` **Possibly related PRs** - [smell-of-curry/pokebedrock-showdown#10](https://github.com/smell-of-curry/pokebedrock-showdown/pull/10): Adds the strategic PlayerAI and tracker architecture extended by this PR. - [smell-of-curry/pokebedrock-showdown#11](https://github.com/smell-of-curry/pokebedrock-showdown/pull/11): Introduces related `lastMoveFailed` state used by this PR’s tracker and decision changes. </details> <!-- walkthrough_end --> <!-- pre_merge_checks_walkthrough_start --> <details> <summary>🚥 Pre-merge checks | ✅ 4 | ❌ 1</summary> ### ❌ Failed checks (1 inconclusive) | Check name | Status | Explanation | Resolution | | :---------: | :------------- | :----------------------------------------------------------------------- | :----------------------------------------------------------------------------------------------------------------------------- | | Title check | ❓ Inconclusive | The title is too vague and generic to convey the main change in this PR. | Use a specific title that names the key changes, such as strategic AI overhaul, randombattle crash fix, and self-play harness. | <details> <summary>✅ Passed checks (4 passed)</summary> | Check name | Status | Explanation | | :------------------------: | :------- | :--------------------------------------------------------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | | Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. | | 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. | </details> </details> <!-- pre_merge_checks_walkthrough_end --> <!-- finishing_touch_checkbox_start --> <details> <summary>✨ Finishing Touches</summary> <details> <summary>📝 Generate docstrings</summary> - [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR - [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch </details> <details> <summary>🧪 Generate unit tests (beta)</summary> - [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests - [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `huge-ai-improvements` </details> </details> <!-- finishing_touch_checkbox_end --> <!-- This is an auto-generated comment: all tool run failures by coderabbit.ai --> > [!WARNING] > There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. > > <details> > <summary>🔧 ESLint</summary> > > > If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration. > > ESLint install timed out. The project may have too many dependencies for the sandbox. > > > > </details> <!-- end of auto-generated comment: all tool run failures by coderabbit.ai --> <!-- tips_start --> --- Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=smell-of-curry/pokebedrock-showdown&utm_content=12)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> <sub>Comment `@coderabbitai help` to get the list of available commands.</sub> <!-- tips_end -->
github-advanced-security[bot] (Migrated from github.com) reviewed 2026-06-10 18:49:54 +00:00
sim/tools/strategic-ai/state/BattleStateTracker.ts
@ -604,0 +753,4 @@
return;
}
case "item": {
const mon = this.resolveByRef(event.pokemon);
github-advanced-security[bot] (Migrated from github.com) commented 2026-06-10 18:49:54 +00:00
Copy link

CodeQL / Prototype-polluting assignment

This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.
This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.

Show more details

## CodeQL / Prototype-polluting assignment This assignment may alter Object.prototype if a malicious '__proto__' string is injected from [library input](1). This assignment may alter Object.prototype if a malicious '__proto__' string is injected from [library input](2). [Show more details](https://github.com/smell-of-curry/pokebedrock-showdown/security/code-scanning/39)
sim/tools/strategic-ai/state/BattleStateTracker.ts
@ -604,0 +773,4 @@
return;
}
case "primal": {
const mon = this.resolveByRef(event.pokemon);
github-advanced-security[bot] (Migrated from github.com) commented 2026-06-10 18:49:54 +00:00
Copy link

CodeQL / Prototype-polluting assignment

This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.
This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.

Show more details

## CodeQL / Prototype-polluting assignment This assignment may alter Object.prototype if a malicious '__proto__' string is injected from [library input](1). This assignment may alter Object.prototype if a malicious '__proto__' string is injected from [library input](2). [Show more details](https://github.com/smell-of-curry/pokebedrock-showdown/security/code-scanning/40)
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2026-06-10 18:51:38 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy link

Pull request overview

This PR substantially strengthens the sim/tools/strategic-ai subsystem by improving battle-state tracking fidelity, making high-difficulty play more consistently “best-move/best-switch” (including deeper MCTS via battle forking), and adding a parallel self-play harness to measure difficulty gradients. It also hardens random-battle team generation against unresolved host cosmetic formes that previously caused battle creation crashes.

Changes:

  • Expanded tracker + inference to incorporate more battle log signals (e.g., inferred immunities, last-move-failed propagation) and improved legality-aware move priors.
  • Reworked high-tier decision logic: move selection ladder + switch margin gating, plus MCTS fork rollouts (and switch verification) when a live Battle is available.
  • Added sim/tools/selfplay.ts to run reproducible, parallel AI-vs-AI winrate series; and filtered random cosmetic formes to prevent Unidentified species crashes.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
sim/tools/strategic-ai/state/OpponentInference.ts Adds gen/level-legal move priors and folds inferred immunities into ability/item distributions.
sim/tools/strategic-ai/state/BattleStateTracker.ts Tracks inferred immunities from `
sim/tools/strategic-ai/policy/DifficultyPolicy.ts Replaces epsilon noise with ladder cap + switch margin knobs and updates tier mapping.
sim/tools/strategic-ai/mechanics/TransformPolicy.ts Defers mega/ultra when projected matchup worsens, using matchup evaluation.
sim/tools/strategic-ai/mechanics/MoveEvaluator.ts Threads doubles-only “defender already took damage” signal into Assurance scoring.
sim/tools/strategic-ai/mechanics/DamageCalc.ts Improves BP tables (weight/speed), Last Respects scaling, and various formatting/logic cleanups.
sim/tools/strategic-ai/engines/MctsEngine.ts Implements true fork-based multi-turn rollouts, determinization, blended scoring, and switch verification.
sim/tools/strategic-ai/engines/HeuristicEngine.ts Adds switch margin + switch-frequency damping, disciplined-tier behavior, and move-score ladder selection.
sim/tools/strategic-ai/engines/Engine.ts Replaces epsilon noise helper with selectByScoreLadder and extends EngineContext knobs (plus optional getBattle).
sim/tools/selfplay.ts New parallel self-play harness with per-game seed derivation, retries, and hang watchdogs.
sim/tools/player-ai.ts Adds ingestLogLine and tracker-to-engineCtx synchronization, and wires optional live-battle access for MCTS forking.
data/random-battles/gen9baby/teams.ts Filters cosmetic forme sampling through dex existence to avoid unresolved skins.
data/random-battles/gen9/teams.ts Same cosmetic forme existence filter as above for gen9 teams.
data/random-battles/gen8/teams.ts Same cosmetic forme existence filter as above for gen8 teams.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview This PR substantially strengthens the `sim/tools/strategic-ai` subsystem by improving battle-state tracking fidelity, making high-difficulty play more consistently “best-move/best-switch” (including deeper MCTS via battle forking), and adding a parallel self-play harness to measure difficulty gradients. It also hardens random-battle team generation against unresolved host cosmetic formes that previously caused battle creation crashes. **Changes:** - Expanded tracker + inference to incorporate more battle log signals (e.g., inferred immunities, last-move-failed propagation) and improved legality-aware move priors. - Reworked high-tier decision logic: move selection ladder + switch margin gating, plus MCTS fork rollouts (and switch verification) when a live `Battle` is available. - Added `sim/tools/selfplay.ts` to run reproducible, parallel AI-vs-AI winrate series; and filtered random cosmetic formes to prevent `Unidentified species` crashes. ### Reviewed changes Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | sim/tools/strategic-ai/state/OpponentInference.ts | Adds gen/level-legal move priors and folds inferred immunities into ability/item distributions. | | sim/tools/strategic-ai/state/BattleStateTracker.ts | Tracks inferred immunities from `|-immune|`, improves last-move-failed handling, and prices Toxic Spikes as expected HP cost. | | sim/tools/strategic-ai/policy/DifficultyPolicy.ts | Replaces epsilon noise with ladder cap + switch margin knobs and updates tier mapping. | | sim/tools/strategic-ai/mechanics/TransformPolicy.ts | Defers mega/ultra when projected matchup worsens, using matchup evaluation. | | sim/tools/strategic-ai/mechanics/MoveEvaluator.ts | Threads doubles-only “defender already took damage” signal into Assurance scoring. | | sim/tools/strategic-ai/mechanics/DamageCalc.ts | Improves BP tables (weight/speed), Last Respects scaling, and various formatting/logic cleanups. | | sim/tools/strategic-ai/engines/MctsEngine.ts | Implements true fork-based multi-turn rollouts, determinization, blended scoring, and switch verification. | | sim/tools/strategic-ai/engines/HeuristicEngine.ts | Adds switch margin + switch-frequency damping, disciplined-tier behavior, and move-score ladder selection. | | sim/tools/strategic-ai/engines/Engine.ts | Replaces epsilon noise helper with `selectByScoreLadder` and extends `EngineContext` knobs (plus optional `getBattle`). | | sim/tools/selfplay.ts | New parallel self-play harness with per-game seed derivation, retries, and hang watchdogs. | | sim/tools/player-ai.ts | Adds `ingestLogLine` and tracker-to-engineCtx synchronization, and wires optional live-battle access for MCTS forking. | | data/random-battles/gen9baby/teams.ts | Filters cosmetic forme sampling through dex existence to avoid unresolved skins. | | data/random-battles/gen9/teams.ts | Same cosmetic forme existence filter as above for gen9 teams. | | data/random-battles/gen8/teams.ts | Same cosmetic forme existence filter as above for gen8 teams. | </details> --- 💡 <a href="/smell-of-curry/pokebedrock-showdown/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
sim/tools/strategic-ai/engines/MctsEngine.ts
@ -202,6 +416,341 @@ function sampleRollout(
return reward;
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-06-10 18:51:38 +00:00
Copy link

determinizeMoves currently consults foeActive.moveSlots (which contains the battle’s real hidden moves) via present = new Set(foeActive.moveSlots.map(...)), and it stops filling when fills runs out. Both behaviors can leak full-information into fork rollouts: the determinization is influenced by the true hidden moves, and any remaining unrevealed slots keep their real moves. This breaks the “no full-info cheating” guarantee of the fork search.

`determinizeMoves` currently consults `foeActive.moveSlots` (which contains the battle’s *real* hidden moves) via `present = new Set(foeActive.moveSlots.map(...))`, and it stops filling when `fills` runs out. Both behaviors can leak full-information into fork rollouts: the determinization is influenced by the true hidden moves, and any remaining unrevealed slots keep their real moves. This breaks the “no full-info cheating” guarantee of the fork search.
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
@ -2,19 +2,25 @@
* Strategic-AI difficulty policy.
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2026-06-10 18:51:38 +00:00
Copy link

The difficulty/knobs mapping table is now out of sync with the actual knobs: difficulty 5 is documented as ladder cap 0.12, but knobsForDifficulty(5) returns ladderAdvanceCap: 0 (fully greedy). This can mislead future tuning/benchmarking since the comment is treated as the authoritative tier spec.

The difficulty/knobs mapping table is now out of sync with the actual knobs: difficulty 5 is documented as ladder cap 0.12, but `knobsForDifficulty(5)` returns `ladderAdvanceCap: 0` (fully greedy). This can mislead future tuning/benchmarking since the comment is treated as the authoritative tier spec.
coderabbitai[bot] (Migrated from github.com) reviewed 2026-06-10 19:02:51 +00:00
coderabbitai[bot] (Migrated from github.com) left a comment
Copy link

Actionable comments posted: 11

🤖 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 `@sim/tools/selfplay.ts`:
- Around line 490-505: The CLI currently uses parseInt directly when building
the SelfPlayOptions object (see the options construction and the call to
runSelfPlay()), which allows NaN and out-of-range values through; validate each
numeric flag after parsing (args.a, args.b, args.games, budget-a, budget-b,
max-turns, game-timeout, jobs) and reject invalid or out-of-range values with a
clear usage error before calling runSelfPlay(); enforce difficulty bounds for
a/b (e.g., allowed range), require non-negative integers for
games/maxTurns/jobs/budgets/timeouts, and treat parseInt failures (NaN) as
invalid input, returning a user-facing error (throw or exit with message) rather
than passing bad values into runSelfPlay().
- Around line 294-309: runOne currently only listens for worker "message" and
"error" so if the worker emits "exit" without an "error" the Promise never
settles; add an "exit" handler in runOne (alongside onMessage and onError) that
calls the same cleanup and rejects with an Error indicating the worker exited
(include exit code and signal if available), and ensure the new onExit is
removed in cleanup and attached with worker.on("exit", onExit) so in-flight
tasks are marked errored and can trigger worker respawn logic.
- Around line 186-187: The empty .catch handlers on p1.start() and p2.start()
swallow start failures; remove those .catch(() => {}) calls so start() promise
rejections surface to the caller, and ensure playGameWithRetries() (the harness)
awaits or otherwise handles the returned start promises so a crashed AI loop
immediately rejects the game attempt instead of hanging/retrying.

In `@sim/tools/strategic-ai/engines/HeuristicEngine.ts`:
- Around line 104-109: TurnPlan.partnerAttacks is too coarse for Assurance
scoring; replace the single boolean with a target-aware structure (e.g.,
partnerAttacksByTarget: Record<targetId, boolean> or two booleans for foeA/foeB)
in the TurnPlan interface and propagate that change to any places that read or
write partnerAttacks (notably the scoring logic around Assurance and the code
paths referenced at the original comment locations). When an earlier slot
chooses a damaging move, mark the specific target in partnerAttacksByTarget
(respecting redirection/redirect events) and change the Assurance check to only
double damage if partnerAttacksByTarget[targetId] is true for the same target
the current move targets. Update all code that set/checked partnerAttacks
(including the locations mentioned in the comment) to use the new per-target
keys and ensure initialization of the TurnPlan uses the new structure.

In `@sim/tools/strategic-ai/mechanics/DamageCalc.ts`:
- Around line 581-586: The Gyro Ball BP calculation in the "gyroball" case
currently adds +1; update it to use floor(25 * targetSpe / userSpe) with a
minimum of 1 and a maximum cap of 150 instead of adding 1. Locate the "gyroball"
case in DamageCalc.ts (variables: userSpe, targetSpe, bp, effectiveStat,
input.attacker, input.defender) and change the bp assignment to compute
Math.max(1, Math.floor((25 * targetSpe) / userSpe)) and then cap it with
Math.min(150, ...), removing the +1 adjustment.
- Around line 490-497: The current logic only runs the ability-to-type mapping
when type === "Normal", so ability "normalize" never converts non-Normal moves;
update the DamageCalc handling so that the "normalize" case on the ability is
applied unconditionally (or handled in a separate branch) to convert any move
type to "Normal" before effectiveness and BP-boost logic runs; specifically
adjust the switch that currently checks type === "Normal" (in DamageCalc.ts) to
either move the "normalize" case out of that conditional or check ability ===
"normalize" earlier so Normalize consistently forces type = "Normal" while
leaving other -ate abilities handled as before.

In `@sim/tools/strategic-ai/policy/DifficultyPolicy.ts`:
- Around line 11-23: The documentation table is out of sync with the actual
tuning for difficulty 5: update the row for difficulty 5 in DifficultyPolicy.ts
to reflect knobsForDifficulty(5) which uses ladderAdvanceCap: 0 (not 0.12) and
update the descriptive text from “near-greedy” to “greedy” (or equivalent) so
the ladder cap column and the trailing sentence about difficulty 5 match the
code; keep other columns (engine MCTS, info forgetting 0.00, switch margin 4)
as-is and ensure any mention of selectByScoreLadder/ladderAdvanceCap aligns with
the new value.
- Around line 110-115: In applyKnobs(EngineContext, difficulty) the current
conditional "if (knobs.searchBudgetMs) ctx.searchBudgetMs =
knobs.searchBudgetMs" preserves prior context values and prevents setting a
legitimate 0; change it to always assign the knob value to ctx.searchBudgetMs
(i.e., remove the truthy check) so knobsForDifficulty's searchBudgetMs
overwrites the existing EngineContext value even when it's 0 or falsy.

In `@sim/tools/strategic-ai/state/OpponentInference.ts`:
- Around line 285-287: The code uses the global Dex.gen when building learnset
keys, but it should use the battle's actual generation from the tracker; update
the functions that reference Dex.gen (notably learnableMoves and the similar
occurrences around lines 329-334) to use the tracker/battle generation (e.g.,
this.tracker.gen or whatever instance `tracker` is in scope) when computing the
key and any gen-dependent checks so the learnset and level gates reflect the
real battle gen rather than Dex.gen.
- Around line 173-180: The loop handling mon.inferredImmunities builds a
distribution that sums to 0.9 when every legal ability is in candidates; change
the logic in the block that computes per/rest so the distribution sums to 1.0:
compute candidates = IMMUNITY_ABILITIES[type]?.filter(a => choices.includes(a));
if candidates exists and rest = choices.filter(c => !candidates.includes(c)) has
length 0, set per = 1.0 / candidates.length and assign that to each candidates
id in dist; otherwise keep the current behavior (per = 0.9 / candidates.length
and 0.1 / rest.length for rest) and then return dist. Ensure you still use the
same symbols (mon.inferredImmunities, IMMUNITY_ABILITIES, choices, dist,
candidates, rest) and return dist after assigning.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 08196a42-9d63-4fe4-8622-66e97ddb5e82

📥 Commits

Reviewing files that changed from the base of the PR and between 5d45910c73 and af190cbba06609c299f18d6dd3a6128e1e140de9.

📒 Files selected for processing (14)
  • data/random-battles/gen8/teams.ts
  • data/random-battles/gen9/teams.ts
  • data/random-battles/gen9baby/teams.ts
  • sim/tools/player-ai.ts
  • sim/tools/selfplay.ts
  • sim/tools/strategic-ai/engines/Engine.ts
  • sim/tools/strategic-ai/engines/HeuristicEngine.ts
  • sim/tools/strategic-ai/engines/MctsEngine.ts
  • sim/tools/strategic-ai/mechanics/DamageCalc.ts
  • sim/tools/strategic-ai/mechanics/MoveEvaluator.ts
  • sim/tools/strategic-ai/mechanics/TransformPolicy.ts
  • sim/tools/strategic-ai/policy/DifficultyPolicy.ts
  • sim/tools/strategic-ai/state/BattleStateTracker.ts
  • sim/tools/strategic-ai/state/OpponentInference.ts
**Actionable comments posted: 11** <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` 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 `@sim/tools/selfplay.ts`: - Around line 490-505: The CLI currently uses parseInt directly when building the SelfPlayOptions object (see the options construction and the call to runSelfPlay()), which allows NaN and out-of-range values through; validate each numeric flag after parsing (args.a, args.b, args.games, budget-a, budget-b, max-turns, game-timeout, jobs) and reject invalid or out-of-range values with a clear usage error before calling runSelfPlay(); enforce difficulty bounds for a/b (e.g., allowed range), require non-negative integers for games/maxTurns/jobs/budgets/timeouts, and treat parseInt failures (NaN) as invalid input, returning a user-facing error (throw or exit with message) rather than passing bad values into runSelfPlay(). - Around line 294-309: runOne currently only listens for worker "message" and "error" so if the worker emits "exit" without an "error" the Promise never settles; add an "exit" handler in runOne (alongside onMessage and onError) that calls the same cleanup and rejects with an Error indicating the worker exited (include exit code and signal if available), and ensure the new onExit is removed in cleanup and attached with worker.on("exit", onExit) so in-flight tasks are marked errored and can trigger worker respawn logic. - Around line 186-187: The empty .catch handlers on p1.start() and p2.start() swallow start failures; remove those .catch(() => {}) calls so start() promise rejections surface to the caller, and ensure playGameWithRetries() (the harness) awaits or otherwise handles the returned start promises so a crashed AI loop immediately rejects the game attempt instead of hanging/retrying. In `@sim/tools/strategic-ai/engines/HeuristicEngine.ts`: - Around line 104-109: TurnPlan.partnerAttacks is too coarse for Assurance scoring; replace the single boolean with a target-aware structure (e.g., partnerAttacksByTarget: Record<targetId, boolean> or two booleans for foeA/foeB) in the TurnPlan interface and propagate that change to any places that read or write partnerAttacks (notably the scoring logic around Assurance and the code paths referenced at the original comment locations). When an earlier slot chooses a damaging move, mark the specific target in partnerAttacksByTarget (respecting redirection/redirect events) and change the Assurance check to only double damage if partnerAttacksByTarget[targetId] is true for the same target the current move targets. Update all code that set/checked partnerAttacks (including the locations mentioned in the comment) to use the new per-target keys and ensure initialization of the TurnPlan uses the new structure. In `@sim/tools/strategic-ai/mechanics/DamageCalc.ts`: - Around line 581-586: The Gyro Ball BP calculation in the "gyroball" case currently adds +1; update it to use floor(25 * targetSpe / userSpe) with a minimum of 1 and a maximum cap of 150 instead of adding 1. Locate the "gyroball" case in DamageCalc.ts (variables: userSpe, targetSpe, bp, effectiveStat, input.attacker, input.defender) and change the bp assignment to compute Math.max(1, Math.floor((25 * targetSpe) / userSpe)) and then cap it with Math.min(150, ...), removing the +1 adjustment. - Around line 490-497: The current logic only runs the ability-to-type mapping when type === "Normal", so ability "normalize" never converts non-Normal moves; update the DamageCalc handling so that the "normalize" case on the ability is applied unconditionally (or handled in a separate branch) to convert any move type to "Normal" before effectiveness and BP-boost logic runs; specifically adjust the switch that currently checks type === "Normal" (in DamageCalc.ts) to either move the "normalize" case out of that conditional or check ability === "normalize" earlier so Normalize consistently forces type = "Normal" while leaving other -ate abilities handled as before. In `@sim/tools/strategic-ai/policy/DifficultyPolicy.ts`: - Around line 11-23: The documentation table is out of sync with the actual tuning for difficulty 5: update the row for difficulty 5 in DifficultyPolicy.ts to reflect knobsForDifficulty(5) which uses ladderAdvanceCap: 0 (not 0.12) and update the descriptive text from “near-greedy” to “greedy” (or equivalent) so the ladder cap column and the trailing sentence about difficulty 5 match the code; keep other columns (engine MCTS, info forgetting 0.00, switch margin 4) as-is and ensure any mention of selectByScoreLadder/ladderAdvanceCap aligns with the new value. - Around line 110-115: In applyKnobs(EngineContext, difficulty) the current conditional "if (knobs.searchBudgetMs) ctx.searchBudgetMs = knobs.searchBudgetMs" preserves prior context values and prevents setting a legitimate 0; change it to always assign the knob value to ctx.searchBudgetMs (i.e., remove the truthy check) so knobsForDifficulty's searchBudgetMs overwrites the existing EngineContext value even when it's 0 or falsy. In `@sim/tools/strategic-ai/state/OpponentInference.ts`: - Around line 285-287: The code uses the global Dex.gen when building learnset keys, but it should use the battle's actual generation from the tracker; update the functions that reference Dex.gen (notably learnableMoves and the similar occurrences around lines 329-334) to use the tracker/battle generation (e.g., this.tracker.gen or whatever instance `tracker` is in scope) when computing the key and any gen-dependent checks so the learnset and level gates reflect the real battle gen rather than Dex.gen. - Around line 173-180: The loop handling mon.inferredImmunities builds a distribution that sums to 0.9 when every legal ability is in candidates; change the logic in the block that computes per/rest so the distribution sums to 1.0: compute candidates = IMMUNITY_ABILITIES[type]?.filter(a => choices.includes(a)); if candidates exists and rest = choices.filter(c => !candidates.includes(c)) has length 0, set per = 1.0 / candidates.length and assign that to each candidates id in dist; otherwise keep the current behavior (per = 0.9 / candidates.length and 0.1 / rest.length for rest) and then return dist. Ensure you still use the same symbols (mon.inferredImmunities, IMMUNITY_ABILITIES, choices, dist, candidates, rest) and return dist after assigning. ``` </details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `08196a42-9d63-4fe4-8622-66e97ddb5e82` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5d45910c73daea1ed349720a0e50d67b9c660142 and af190cbba06609c299f18d6dd3a6128e1e140de9. </details> <details> <summary>📒 Files selected for processing (14)</summary> * `data/random-battles/gen8/teams.ts` * `data/random-battles/gen9/teams.ts` * `data/random-battles/gen9baby/teams.ts` * `sim/tools/player-ai.ts` * `sim/tools/selfplay.ts` * `sim/tools/strategic-ai/engines/Engine.ts` * `sim/tools/strategic-ai/engines/HeuristicEngine.ts` * `sim/tools/strategic-ai/engines/MctsEngine.ts` * `sim/tools/strategic-ai/mechanics/DamageCalc.ts` * `sim/tools/strategic-ai/mechanics/MoveEvaluator.ts` * `sim/tools/strategic-ai/mechanics/TransformPolicy.ts` * `sim/tools/strategic-ai/policy/DifficultyPolicy.ts` * `sim/tools/strategic-ai/state/BattleStateTracker.ts` * `sim/tools/strategic-ai/state/OpponentInference.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
sim/tools/selfplay.ts
@ -0,0 +1,558 @@
/**
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:48 +00:00
Copy link

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Surface PlayerAI.start() failures instead of converting them into timeout hangs.

These are the only promises that can tell this harness an AI loop crashed. Swallowing both rejections means a deterministic failure just sits until the force-tie/hung timers fire, and playGameWithRetries() can then burn three more full retries on the same broken setup.

Suggested direction 
- void p1.start().catch(() => {});
- void p2.start().catch(() => {});
+ const aiFailure = Promise.race([
+   p1.start().then(() => null, err => ({side: "p1", err})),
+   p2.start().then(() => null, err => ({side: "p2", err})),
+ ]);
...
- const raced = await Promise.race([consume(), hung]);
+ const raced = await Promise.race([consume(), hung, aiFailure]);
+ if (raced && raced !== "hung") {
+   errored = true;
+   errorMessage = `${raced.side} AI failed: ${raced.err.message}`;
+ }
🤖 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 `@sim/tools/selfplay.ts` around lines 186 - 187, The empty .catch handlers on
p1.start() and p2.start() swallow start failures; remove those .catch(() => {})
calls so start() promise rejections surface to the caller, and ensure
playGameWithRetries() (the harness) awaits or otherwise handles the returned
start promises so a crashed AI loop immediately rejects the game attempt instead
of hanging/retrying.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Surface `PlayerAI.start()` failures instead of converting them into timeout hangs.** These are the only promises that can tell this harness an AI loop crashed. Swallowing both rejections means a deterministic failure just sits until the force-tie/hung timers fire, and `playGameWithRetries()` can then burn three more full retries on the same broken setup. <details> <summary>Suggested direction</summary> ```diff - void p1.start().catch(() => {}); - void p2.start().catch(() => {}); + const aiFailure = Promise.race([ + p1.start().then(() => null, err => ({side: "p1", err})), + p2.start().then(() => null, err => ({side: "p2", err})), + ]); ... - const raced = await Promise.race([consume(), hung]); + const raced = await Promise.race([consume(), hung, aiFailure]); + if (raced && raced !== "hung") { + errored = true; + errorMessage = `${raced.side} AI failed: ${raced.err.message}`; + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/selfplay.ts` around lines 186 - 187, The empty .catch handlers on p1.start() and p2.start() swallow start failures; remove those .catch(() => {}) calls so start() promise rejections surface to the caller, and ensure playGameWithRetries() (the harness) awaits or otherwise handles the returned start promises so a crashed AI loop immediately rejects the game attempt instead of hanging/retrying. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:f7e49ff96a67fc536252df58 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/selfplay.ts
@ -0,0 +327,4 @@
worker.on("message", onMessage);
worker.on("error", onError);
worker.on("exit", onExit);
worker.postMessage(task);
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:48 +00:00
Copy link

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle worker exit as well as error for in-flight tasks.

runOne() never settles if a worker dies before replying but doesn't emit error first. In that case the lane hangs forever instead of marking the game errored and respawning the worker as intended.

Suggested fix 
 const runOne = (worker: Worker, task: WorkerTask) => new Promise<GameResult>((resolve, reject) => {
   const onMessage = (msg: { index: number, result: GameResult }) => {
     cleanup();
     resolve(msg.result);
   };
   const onError = (err: Error) => {
     cleanup();
     reject(err);
   };
+  const onExit = (code: number) => {
+    cleanup();
+    reject(new Error(`worker exited before replying (code ${code})`));
+  };
   const cleanup = () => {
     worker.off("message", onMessage);
     worker.off("error", onError);
+    worker.off("exit", onExit);
   };
   worker.on("message", onMessage);
   worker.on("error", onError);
+  worker.on("exit", onExit);
   worker.postMessage(task);
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

const runOne = (worker: Worker, task: WorkerTask) => new Promise<GameResult>((resolve, reject) => {
	const onMessage = (msg: { index: number, result: GameResult }) => {
		cleanup();
		resolve(msg.result);
	};
	const onError = (err: Error) => {
		cleanup();
		reject(err);
	};
	const onExit = (code: number) => {
		cleanup();
		reject(new Error(`worker exited before replying (code ${code})`));
	};
	const cleanup = () => {
		worker.off("message", onMessage);
		worker.off("error", onError);
		worker.off("exit", onExit);
	};
	worker.on("message", onMessage);
	worker.on("error", onError);
	worker.on("exit", onExit);
	worker.postMessage(task);
});
🤖 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 `@sim/tools/selfplay.ts` around lines 294 - 309, runOne currently only listens
for worker "message" and "error" so if the worker emits "exit" without an
"error" the Promise never settles; add an "exit" handler in runOne (alongside
onMessage and onError) that calls the same cleanup and rejects with an Error
indicating the worker exited (include exit code and signal if available), and
ensure the new onExit is removed in cleanup and attached with worker.on("exit",
onExit) so in-flight tasks are marked errored and can trigger worker respawn
logic.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Handle worker `exit` as well as `error` for in-flight tasks.** `runOne()` never settles if a worker dies before replying but doesn't emit `error` first. In that case the lane hangs forever instead of marking the game errored and respawning the worker as intended. <details> <summary>Suggested fix</summary> ```diff const runOne = (worker: Worker, task: WorkerTask) => new Promise<GameResult>((resolve, reject) => { const onMessage = (msg: { index: number, result: GameResult }) => { cleanup(); resolve(msg.result); }; const onError = (err: Error) => { cleanup(); reject(err); }; + const onExit = (code: number) => { + cleanup(); + reject(new Error(`worker exited before replying (code ${code})`)); + }; const cleanup = () => { worker.off("message", onMessage); worker.off("error", onError); + worker.off("exit", onExit); }; worker.on("message", onMessage); worker.on("error", onError); + worker.on("exit", onExit); worker.postMessage(task); }); ``` </details> <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion const runOne = (worker: Worker, task: WorkerTask) => new Promise<GameResult>((resolve, reject) => { const onMessage = (msg: { index: number, result: GameResult }) => { cleanup(); resolve(msg.result); }; const onError = (err: Error) => { cleanup(); reject(err); }; const onExit = (code: number) => { cleanup(); reject(new Error(`worker exited before replying (code ${code})`)); }; const cleanup = () => { worker.off("message", onMessage); worker.off("error", onError); worker.off("exit", onExit); }; worker.on("message", onMessage); worker.on("error", onError); worker.on("exit", onExit); worker.postMessage(task); }); ``` </details> <!-- suggestion_end --> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/selfplay.ts` around lines 294 - 309, runOne currently only listens for worker "message" and "error" so if the worker emits "exit" without an "error" the Promise never settles; add an "exit" handler in runOne (alongside onMessage and onError) that calls the same cleanup and rejects with an Error indicating the worker exited (include exit code and signal if available), and ensure the new onExit is removed in cleanup and attached with worker.on("exit", onExit) so in-flight tasks are marked errored and can trigger worker respawn logic. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:d82aebe45f3c3bf5173edb59 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/selfplay.ts
@ -0,0 +541,4 @@
maxTurns: intArg(args["max-turns"], "max-turns", 1),
gameTimeoutMs: intArg(args["game-timeout"], "game-timeout", 0),
jobs: intArg(args.jobs, "jobs", 1),
verbose: !args.quiet,
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:48 +00:00
Copy link

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject invalid numeric CLI flags up front.

parseInt can yield NaN or out-of-range values here (--games foo, --a 9, --max-turns -1, etc.), and those flow straight into runSelfPlay(). The result is confusing behavior—empty runs, immediate timeouts, or invalid difficulty configs—instead of a clear usage error.

🤖 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 `@sim/tools/selfplay.ts` around lines 490 - 505, The CLI currently uses
parseInt directly when building the SelfPlayOptions object (see the options
construction and the call to runSelfPlay()), which allows NaN and out-of-range
values through; validate each numeric flag after parsing (args.a, args.b,
args.games, budget-a, budget-b, max-turns, game-timeout, jobs) and reject
invalid or out-of-range values with a clear usage error before calling
runSelfPlay(); enforce difficulty bounds for a/b (e.g., allowed range), require
non-negative integers for games/maxTurns/jobs/budgets/timeouts, and treat
parseInt failures (NaN) as invalid input, returning a user-facing error (throw
or exit with message) rather than passing bad values into runSelfPlay().

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Reject invalid numeric CLI flags up front.** `parseInt` can yield `NaN` or out-of-range values here (`--games foo`, `--a 9`, `--max-turns -1`, etc.), and those flow straight into `runSelfPlay()`. The result is confusing behavior—empty runs, immediate timeouts, or invalid difficulty configs—instead of a clear usage error. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/selfplay.ts` around lines 490 - 505, The CLI currently uses parseInt directly when building the SelfPlayOptions object (see the options construction and the call to runSelfPlay()), which allows NaN and out-of-range values through; validate each numeric flag after parsing (args.a, args.b, args.games, budget-a, budget-b, max-turns, game-timeout, jobs) and reject invalid or out-of-range values with a clear usage error before calling runSelfPlay(); enforce difficulty bounds for a/b (e.g., allowed range), require non-negative integers for games/maxTurns/jobs/budgets/timeouts, and treat parseInt failures (NaN) as invalid input, returning a user-facing error (throw or exit with message) rather than passing bad values into runSelfPlay(). ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:fc2e0004f8ceff0fccd0c5b0 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/strategic-ai/engines/HeuristicEngine.ts
@ -99,0 +124,4 @@
*/
interface TurnPlan {
/** An earlier slot chose Helping Hand this turn. */
helpingHand: boolean;
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:48 +00:00
Copy link

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

partnerAttacks is too coarse for Assurance.

This boolean only says that an earlier ally picked some damaging move. Assurance doubles only if the same target has already taken damage, so later-slot scoring is inflated whenever the partner attacks the other foe, gets redirected, or commits to a non-matching hit pattern. This needs target-aware turn-plan state, not a single flag.

Also applies to: 330-335, 360-361

🤖 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 `@sim/tools/strategic-ai/engines/HeuristicEngine.ts` around lines 104 - 109,
TurnPlan.partnerAttacks is too coarse for Assurance scoring; replace the single
boolean with a target-aware structure (e.g., partnerAttacksByTarget:
Record<targetId, boolean> or two booleans for foeA/foeB) in the TurnPlan
interface and propagate that change to any places that read or write
partnerAttacks (notably the scoring logic around Assurance and the code paths
referenced at the original comment locations). When an earlier slot chooses a
damaging move, mark the specific target in partnerAttacksByTarget (respecting
redirection/redirect events) and change the Assurance check to only double
damage if partnerAttacksByTarget[targetId] is true for the same target the
current move targets. Update all code that set/checked partnerAttacks (including
the locations mentioned in the comment) to use the new per-target keys and
ensure initialization of the TurnPlan uses the new structure.
_⚠️ Potential issue_ | _🟠 Major_ | _🏗️ Heavy lift_ **`partnerAttacks` is too coarse for Assurance.** This boolean only says that an earlier ally picked some damaging move. Assurance doubles only if the *same target* has already taken damage, so later-slot scoring is inflated whenever the partner attacks the other foe, gets redirected, or commits to a non-matching hit pattern. This needs target-aware turn-plan state, not a single flag. Also applies to: 330-335, 360-361 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/engines/HeuristicEngine.ts` around lines 104 - 109, TurnPlan.partnerAttacks is too coarse for Assurance scoring; replace the single boolean with a target-aware structure (e.g., partnerAttacksByTarget: Record<targetId, boolean> or two booleans for foeA/foeB) in the TurnPlan interface and propagate that change to any places that read or write partnerAttacks (notably the scoring logic around Assurance and the code paths referenced at the original comment locations). When an earlier slot chooses a damaging move, mark the specific target in partnerAttacksByTarget (respecting redirection/redirect events) and change the Assurance check to only double damage if partnerAttacksByTarget[targetId] is true for the same target the current move targets. Update all code that set/checked partnerAttacks (including the locations mentioned in the comment) to use the new per-target keys and ensure initialization of the TurnPlan uses the new structure. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:61a8cae05505c32971567012 --> <!-- This is an auto-generated reply by CodeRabbit -->
sim/tools/strategic-ai/engines/MctsEngine.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:48 +00:00
Copy link

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Fork rollouts still read the opponent’s hidden bench.

determinizeMoves only rewrites the current foe active, but the fork keeps the real unrevealed backline and evaluateForkState scores every side.pokemon. After a KO or forced switch, these rollouts can therefore branch on and score the opponent’s actual hidden switch-in/species/HP, which turns the d5 fork path into a full-information search outside self-play. Redact/determinize unrevealed foe bench state before forking, or keep fork evaluation constrained to tracker-visible state only.

Also applies to: 711-729

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟠 Major_ | _🏗️ Heavy lift_ **Fork rollouts still read the opponent’s hidden bench.** `determinizeMoves` only rewrites the current foe active, but the fork keeps the real unrevealed backline and `evaluateForkState` scores every `side.pokemon`. After a KO or forced switch, these rollouts can therefore branch on and score the opponent’s actual hidden switch-in/species/HP, which turns the d5 fork path into a full-information search outside self-play. Redact/determinize unrevealed foe bench state before forking, or keep fork evaluation constrained to tracker-visible state only. Also applies to: 711-729 <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:3c2972a2bdfdd7550672b476 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/strategic-ai/mechanics/DamageCalc.ts
@ -503,22 +505,22 @@ function abilityImmunityBlocks(moveType: string, move: Move, input: DamageCalcIn
const ability = toID(input.defender.ability);
if (move.category === "Status") return false;
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:49 +00:00
Copy link

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Normalize never converts non-Normal moves here.

Because this switch only runs inside if (type === "Normal"), a Normalize user firing a non-Normal move keeps its original type. That breaks both effectiveness and the later Normalize BP boost path.

Suggested fix 
 function resolveMoveType(move: Move, input: DamageCalcInput): string {
 	const id = toID(move.id || (move as { name?: string }).name || "");
 	let type = move.type || "Normal";
 	const ability = toID(input.attacker.ability);
+	if (ability === "normalize") return "Normal";
 	if (id === "terablast" && input.attacker.terastallized && input.attacker.teraType) {
 		type = input.attacker.teraType;
 	}
 	...
 	if (type === "Normal") {
 		switch (ability) {
 		case "aerilate": return "Flying";
 		case "pixilate": return "Fairy";
 		case "refrigerate": return "Ice";
 		case "galvanize": return "Electric";
-		case "normalize": return "Normal"; // No-op on Normal; -ate boost still applies.
 		}
 	}
 	return type;
 }
🤖 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 `@sim/tools/strategic-ai/mechanics/DamageCalc.ts` around lines 490 - 497, The
current logic only runs the ability-to-type mapping when type === "Normal", so
ability "normalize" never converts non-Normal moves; update the DamageCalc
handling so that the "normalize" case on the ability is applied unconditionally
(or handled in a separate branch) to convert any move type to "Normal" before
effectiveness and BP-boost logic runs; specifically adjust the switch that
currently checks type === "Normal" (in DamageCalc.ts) to either move the
"normalize" case out of that conditional or check ability === "normalize"
earlier so Normalize consistently forces type = "Normal" while leaving other
-ate abilities handled as before.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`Normalize` never converts non-Normal moves here.** Because this switch only runs inside `if (type === "Normal")`, a `Normalize` user firing a non-Normal move keeps its original type. That breaks both effectiveness and the later Normalize BP boost path. <details> <summary>Suggested fix</summary> ```diff function resolveMoveType(move: Move, input: DamageCalcInput): string { const id = toID(move.id || (move as { name?: string }).name || ""); let type = move.type || "Normal"; const ability = toID(input.attacker.ability); + if (ability === "normalize") return "Normal"; if (id === "terablast" && input.attacker.terastallized && input.attacker.teraType) { type = input.attacker.teraType; } ... if (type === "Normal") { switch (ability) { case "aerilate": return "Flying"; case "pixilate": return "Fairy"; case "refrigerate": return "Ice"; case "galvanize": return "Electric"; - case "normalize": return "Normal"; // No-op on Normal; -ate boost still applies. } } return type; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/mechanics/DamageCalc.ts` around lines 490 - 497, The current logic only runs the ability-to-type mapping when type === "Normal", so ability "normalize" never converts non-Normal moves; update the DamageCalc handling so that the "normalize" case on the ability is applied unconditionally (or handled in a separate branch) to convert any move type to "Normal" before effectiveness and BP-boost logic runs; specifically adjust the switch that currently checks type === "Normal" (in DamageCalc.ts) to either move the "normalize" case out of that conditional or check ability === "normalize" earlier so Normalize consistently forces type = "Normal" while leaving other -ate abilities handled as before. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:1719f4166502ceb0eb54781d --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/strategic-ai/mechanics/DamageCalc.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:49 +00:00
Copy link

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Gyro Ball base power is off by one.

The formula is floor(25 * targetSpe / userSpe) with a minimum of 1, not ... + 1. This overstates every Gyro Ball roll.

Suggested fix 
 	case "gyroball": {
 		// BP = 25 × (target speed / user speed) + 1, capped at 150.
 		const userSpe = Math.max(1, effectiveStat(input.attacker, "spe"));
 		const targetSpe = effectiveStat(input.defender, "spe");
-		bp = Math.min(150, Math.floor((25 * targetSpe) / userSpe) + 1);
+		bp = Math.min(150, Math.max(1, Math.floor((25 * targetSpe) / userSpe)));
 		break;
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	case "gyroball": {
		// BP = 25 × (target speed / user speed) + 1, capped at 150.
		const userSpe = Math.max(1, effectiveStat(input.attacker, "spe"));
		const targetSpe = effectiveStat(input.defender, "spe");
		bp = Math.min(150, Math.max(1, Math.floor((25 * targetSpe) / userSpe)));
		break;
	}
🤖 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 `@sim/tools/strategic-ai/mechanics/DamageCalc.ts` around lines 581 - 586, The
Gyro Ball BP calculation in the "gyroball" case currently adds +1; update it to
use floor(25 * targetSpe / userSpe) with a minimum of 1 and a maximum cap of 150
instead of adding 1. Locate the "gyroball" case in DamageCalc.ts (variables:
userSpe, targetSpe, bp, effectiveStat, input.attacker, input.defender) and
change the bp assignment to compute Math.max(1, Math.floor((25 * targetSpe) /
userSpe)) and then cap it with Math.min(150, ...), removing the +1 adjustment.
_⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`Gyro Ball` base power is off by one.** The formula is `floor(25 * targetSpe / userSpe)` with a minimum of 1, not `... + 1`. This overstates every `Gyro Ball` roll. <details> <summary>Suggested fix</summary> ```diff case "gyroball": { // BP = 25 × (target speed / user speed) + 1, capped at 150. const userSpe = Math.max(1, effectiveStat(input.attacker, "spe")); const targetSpe = effectiveStat(input.defender, "spe"); - bp = Math.min(150, Math.floor((25 * targetSpe) / userSpe) + 1); + bp = Math.min(150, Math.max(1, Math.floor((25 * targetSpe) / userSpe))); break; } ``` </details> <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion case "gyroball": { // BP = 25 × (target speed / user speed) + 1, capped at 150. const userSpe = Math.max(1, effectiveStat(input.attacker, "spe")); const targetSpe = effectiveStat(input.defender, "spe"); bp = Math.min(150, Math.max(1, Math.floor((25 * targetSpe) / userSpe))); break; } ``` </details> <!-- suggestion_end --> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/mechanics/DamageCalc.ts` around lines 581 - 586, The Gyro Ball BP calculation in the "gyroball" case currently adds +1; update it to use floor(25 * targetSpe / userSpe) with a minimum of 1 and a maximum cap of 150 instead of adding 1. Locate the "gyroball" case in DamageCalc.ts (variables: userSpe, targetSpe, bp, effectiveStat, input.attacker, input.defender) and change the bp assignment to compute Math.max(1, Math.floor((25 * targetSpe) / userSpe)) and then cap it with Math.min(150, ...), removing the +1 adjustment. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:da5fa3c2cf007aee909593ac --> <!-- This is an auto-generated reply by CodeRabbit -->
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:49 +00:00
Copy link

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the difficulty-5 tuning table.

Line 17 still documents a 0.12 ladder cap and Lines 20-23 describe tier 5 as “near-greedy”, but knobsForDifficulty(5) is now fully greedy with ladderAdvanceCap: 0. Leaving the table stale makes future tuning harder to reason about.

🤖 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 `@sim/tools/strategic-ai/policy/DifficultyPolicy.ts` around lines 11 - 23, The
documentation table is out of sync with the actual tuning for difficulty 5:
update the row for difficulty 5 in DifficultyPolicy.ts to reflect
knobsForDifficulty(5) which uses ladderAdvanceCap: 0 (not 0.12) and update the
descriptive text from “near-greedy” to “greedy” (or equivalent) so the ladder
cap column and the trailing sentence about difficulty 5 match the code; keep
other columns (engine MCTS, info forgetting 0.00, switch margin 4) as-is and
ensure any mention of selectByScoreLadder/ladderAdvanceCap aligns with the new
value.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix the difficulty-5 tuning table.** Line 17 still documents a `0.12` ladder cap and Lines 20-23 describe tier 5 as “near-greedy”, but `knobsForDifficulty(5)` is now fully greedy with `ladderAdvanceCap: 0`. Leaving the table stale makes future tuning harder to reason about. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/policy/DifficultyPolicy.ts` around lines 11 - 23, The documentation table is out of sync with the actual tuning for difficulty 5: update the row for difficulty 5 in DifficultyPolicy.ts to reflect knobsForDifficulty(5) which uses ladderAdvanceCap: 0 (not 0.12) and update the descriptive text from “near-greedy” to “greedy” (or equivalent) so the ladder cap column and the trailing sentence about difficulty 5 match the code; keep other columns (engine MCTS, info forgetting 0.00, switch margin 4) as-is and ensure any mention of selectByScoreLadder/ladderAdvanceCap aligns with the new value. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:0d3c4e0c42c51d3601852883 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
@ -102,7 +109,11 @@ function autoFor(difficulty: number): Exclude<EngineName, "auto"> {
*/
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:49 +00:00
Copy link

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Overwrite searchBudgetMs instead of conditionally preserving it.

Line 115 only writes truthy budgets. If this helper is reapplied to a reused EngineContext, tiers without a budget inherit the previous tier’s value, and a legitimate 0 budget could never be applied.

Suggested fix 
 export function applyKnobs(ctx: EngineContext, difficulty: number): void {
 	const knobs = knobsForDifficulty(difficulty);
 	ctx.ladderAdvanceCap = knobs.ladderAdvanceCap;
 	ctx.infoForgetting = knobs.infoForgetting;
 	ctx.switchMargin = knobs.switchMargin;
-	if (knobs.searchBudgetMs) ctx.searchBudgetMs = knobs.searchBudgetMs;
+	ctx.searchBudgetMs = knobs.searchBudgetMs;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

export function applyKnobs(ctx: EngineContext, difficulty: number): void {
	const knobs = knobsForDifficulty(difficulty);
	ctx.ladderAdvanceCap = knobs.ladderAdvanceCap;
	ctx.infoForgetting = knobs.infoForgetting;
	ctx.switchMargin = knobs.switchMargin;
	ctx.searchBudgetMs = knobs.searchBudgetMs;
}
🤖 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 `@sim/tools/strategic-ai/policy/DifficultyPolicy.ts` around lines 110 - 115, In
applyKnobs(EngineContext, difficulty) the current conditional "if
(knobs.searchBudgetMs) ctx.searchBudgetMs = knobs.searchBudgetMs" preserves
prior context values and prevents setting a legitimate 0; change it to always
assign the knob value to ctx.searchBudgetMs (i.e., remove the truthy check) so
knobsForDifficulty's searchBudgetMs overwrites the existing EngineContext value
even when it's 0 or falsy.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Overwrite `searchBudgetMs` instead of conditionally preserving it.** Line 115 only writes truthy budgets. If this helper is reapplied to a reused `EngineContext`, tiers without a budget inherit the previous tier’s value, and a legitimate `0` budget could never be applied. <details> <summary>Suggested fix</summary> ```diff export function applyKnobs(ctx: EngineContext, difficulty: number): void { const knobs = knobsForDifficulty(difficulty); ctx.ladderAdvanceCap = knobs.ladderAdvanceCap; ctx.infoForgetting = knobs.infoForgetting; ctx.switchMargin = knobs.switchMargin; - if (knobs.searchBudgetMs) ctx.searchBudgetMs = knobs.searchBudgetMs; + ctx.searchBudgetMs = knobs.searchBudgetMs; } ``` </details> <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion export function applyKnobs(ctx: EngineContext, difficulty: number): void { const knobs = knobsForDifficulty(difficulty); ctx.ladderAdvanceCap = knobs.ladderAdvanceCap; ctx.infoForgetting = knobs.infoForgetting; ctx.switchMargin = knobs.switchMargin; ctx.searchBudgetMs = knobs.searchBudgetMs; } ``` </details> <!-- suggestion_end --> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/policy/DifficultyPolicy.ts` around lines 110 - 115, In applyKnobs(EngineContext, difficulty) the current conditional "if (knobs.searchBudgetMs) ctx.searchBudgetMs = knobs.searchBudgetMs" preserves prior context values and prevents setting a legitimate 0; change it to always assign the knob value to ctx.searchBudgetMs (i.e., remove the truthy check) so knobsForDifficulty's searchBudgetMs overwrites the existing EngineContext value even when it's 0 or falsy. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:d9d90afd7c202c06d795e11c --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/strategic-ai/state/OpponentInference.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:49 +00:00
Copy link

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize the immunity-ability prior when there is no remainder bucket.

If every legal ability is in candidates, this branch returns a distribution summing to 0.9 instead of 1.0. That breaks the Distribution contract and underweights the only legal outcomes for single-ability species.

Suggested fix 
 	for (const type of mon.inferredImmunities) {
 		const candidates = IMMUNITY_ABILITIES[type]?.filter(a => choices.includes(a));
 		if (!candidates?.length) continue;
-		const per = 0.9 / candidates.length;
-		for (const id of candidates) dist.set(id, per);
 		const rest = choices.filter(c => !candidates.includes(c));
-		for (const id of rest) dist.set(id, 0.1 / rest.length);
+		if (rest.length === 0) {
+			for (const id of candidates) dist.set(id, 1 / candidates.length);
+		} else {
+			for (const id of candidates) dist.set(id, 0.9 / candidates.length);
+			for (const id of rest) dist.set(id, 0.1 / rest.length);
+		}
 		return dist;
 	}
🤖 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 `@sim/tools/strategic-ai/state/OpponentInference.ts` around lines 173 - 180,
The loop handling mon.inferredImmunities builds a distribution that sums to 0.9
when every legal ability is in candidates; change the logic in the block that
computes per/rest so the distribution sums to 1.0: compute candidates =
IMMUNITY_ABILITIES[type]?.filter(a => choices.includes(a)); if candidates exists
and rest = choices.filter(c => !candidates.includes(c)) has length 0, set per =
1.0 / candidates.length and assign that to each candidates id in dist; otherwise
keep the current behavior (per = 0.9 / candidates.length and 0.1 / rest.length
for rest) and then return dist. Ensure you still use the same symbols
(mon.inferredImmunities, IMMUNITY_ABILITIES, choices, dist, candidates, rest)
and return dist after assigning.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Normalize the immunity-ability prior when there is no remainder bucket.** If every legal ability is in `candidates`, this branch returns a distribution summing to `0.9` instead of `1.0`. That breaks the `Distribution` contract and underweights the only legal outcomes for single-ability species. <details> <summary>Suggested fix</summary> ```diff for (const type of mon.inferredImmunities) { const candidates = IMMUNITY_ABILITIES[type]?.filter(a => choices.includes(a)); if (!candidates?.length) continue; - const per = 0.9 / candidates.length; - for (const id of candidates) dist.set(id, per); const rest = choices.filter(c => !candidates.includes(c)); - for (const id of rest) dist.set(id, 0.1 / rest.length); + if (rest.length === 0) { + for (const id of candidates) dist.set(id, 1 / candidates.length); + } else { + for (const id of candidates) dist.set(id, 0.9 / candidates.length); + for (const id of rest) dist.set(id, 0.1 / rest.length); + } return dist; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/state/OpponentInference.ts` around lines 173 - 180, The loop handling mon.inferredImmunities builds a distribution that sums to 0.9 when every legal ability is in candidates; change the logic in the block that computes per/rest so the distribution sums to 1.0: compute candidates = IMMUNITY_ABILITIES[type]?.filter(a => choices.includes(a)); if candidates exists and rest = choices.filter(c => !candidates.includes(c)) has length 0, set per = 1.0 / candidates.length and assign that to each candidates id in dist; otherwise keep the current behavior (per = 0.9 / candidates.length and 0.1 / rest.length for rest) and then return dist. Ensure you still use the same symbols (mon.inferredImmunities, IMMUNITY_ABILITIES, choices, dist, candidates, rest) and return dist after assigning. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:d134a50673af4b2cdb03ce28 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
sim/tools/strategic-ai/state/OpponentInference.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-06-10 19:02:49 +00:00
Copy link

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Key learnset legality off the battle’s actual gen, not Dex.gen.

Line 286 uses the global default dex generation, but the tracker already records the real battle generation. In Gen 8 or older formats this builds Gen 9 move pools and level gates, so the rollout priors are wrong before search even starts.

Also applies to: 329-334

🤖 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 `@sim/tools/strategic-ai/state/OpponentInference.ts` around lines 285 - 287,
The code uses the global Dex.gen when building learnset keys, but it should use
the battle's actual generation from the tracker; update the functions that
reference Dex.gen (notably learnableMoves and the similar occurrences around
lines 329-334) to use the tracker/battle generation (e.g., this.tracker.gen or
whatever instance `tracker` is in scope) when computing the key and any
gen-dependent checks so the learnset and level gates reflect the real battle gen
rather than Dex.gen.

✅ Addressed in commits f4568cf to d768907

_⚠️ Potential issue_ | _🟠 Major_ | _🏗️ Heavy lift_ **Key learnset legality off the battle’s actual gen, not `Dex.gen`.** Line 286 uses the global default dex generation, but the tracker already records the real battle generation. In Gen 8 or older formats this builds Gen 9 move pools and level gates, so the rollout priors are wrong before search even starts. Also applies to: 329-334 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/state/OpponentInference.ts` around lines 285 - 287, The code uses the global Dex.gen when building learnset keys, but it should use the battle's actual generation from the tracker; update the functions that reference Dex.gen (notably learnableMoves and the similar occurrences around lines 329-334) to use the tracker/battle generation (e.g., this.tracker.gen or whatever instance `tracker` is in scope) when computing the key and any gen-dependent checks so the learnset and level gates reflect the real battle gen rather than Dex.gen. ``` </details> <!-- fingerprinting:phantom:medusa:grasshopper --> <!-- cr-comment:v1:f7ac49e093141a285e575092 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 18:44:25 +00:00
coderabbitai[bot] (Migrated from github.com) left a comment
Copy link

Actionable comments posted: 1

🤖 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 `@sim/tools/strategic-ai/mechanics/DamageCalc.ts`:
- Around line 691-693: The Normalize handling in DamageCalc is incomplete and
the `|| true` check is dead code. Update `resolveMoveType` so Normalize actually
converts eligible moves to Normal before damage is calculated, then keep the
boost logic in the `normalize` case using the resolved move type from
`DamageCalc`. Remove the tautology and make sure the `moveType`/`move.type`
check in the `normalize` branch only applies to moves that are truly Normal
after resolution.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7184dd53-0f60-4f61-946f-d21060dd901f

📥 Commits

Reviewing files that changed from the base of the PR and between af190cbba06609c299f18d6dd3a6128e1e140de9 and 479d35216b09d6658646e6b4b7a65146a3f940d3.

📒 Files selected for processing (14)
  • data/random-battles/gen8/teams.ts
  • data/random-battles/gen9/teams.ts
  • data/random-battles/gen9baby/teams.ts
  • sim/tools/player-ai.ts
  • sim/tools/selfplay.ts
  • sim/tools/strategic-ai/engines/Engine.ts
  • sim/tools/strategic-ai/engines/HeuristicEngine.ts
  • sim/tools/strategic-ai/engines/MctsEngine.ts
  • sim/tools/strategic-ai/mechanics/DamageCalc.ts
  • sim/tools/strategic-ai/mechanics/MoveEvaluator.ts
  • sim/tools/strategic-ai/mechanics/TransformPolicy.ts
  • sim/tools/strategic-ai/policy/DifficultyPolicy.ts
  • sim/tools/strategic-ai/state/BattleStateTracker.ts
  • sim/tools/strategic-ai/state/OpponentInference.ts
🚧 Files skipped from review as they are similar to previous changes (11)
  • data/random-battles/gen9baby/teams.ts
  • sim/tools/strategic-ai/mechanics/MoveEvaluator.ts
  • data/random-battles/gen8/teams.ts
  • sim/tools/strategic-ai/policy/DifficultyPolicy.ts
  • sim/tools/strategic-ai/mechanics/TransformPolicy.ts
  • sim/tools/strategic-ai/engines/Engine.ts
  • sim/tools/strategic-ai/engines/MctsEngine.ts
  • sim/tools/player-ai.ts
  • sim/tools/strategic-ai/engines/HeuristicEngine.ts
  • sim/tools/selfplay.ts
  • data/random-battles/gen9/teams.ts
**Actionable comments posted: 1** <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` 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 `@sim/tools/strategic-ai/mechanics/DamageCalc.ts`: - Around line 691-693: The Normalize handling in DamageCalc is incomplete and the `|| true` check is dead code. Update `resolveMoveType` so Normalize actually converts eligible moves to Normal before damage is calculated, then keep the boost logic in the `normalize` case using the resolved move type from `DamageCalc`. Remove the tautology and make sure the `moveType`/`move.type` check in the `normalize` branch only applies to moves that are truly Normal after resolution. ``` </details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `7184dd53-0f60-4f61-946f-d21060dd901f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between af190cbba06609c299f18d6dd3a6128e1e140de9 and 479d35216b09d6658646e6b4b7a65146a3f940d3. </details> <details> <summary>📒 Files selected for processing (14)</summary> * `data/random-battles/gen8/teams.ts` * `data/random-battles/gen9/teams.ts` * `data/random-battles/gen9baby/teams.ts` * `sim/tools/player-ai.ts` * `sim/tools/selfplay.ts` * `sim/tools/strategic-ai/engines/Engine.ts` * `sim/tools/strategic-ai/engines/HeuristicEngine.ts` * `sim/tools/strategic-ai/engines/MctsEngine.ts` * `sim/tools/strategic-ai/mechanics/DamageCalc.ts` * `sim/tools/strategic-ai/mechanics/MoveEvaluator.ts` * `sim/tools/strategic-ai/mechanics/TransformPolicy.ts` * `sim/tools/strategic-ai/policy/DifficultyPolicy.ts` * `sim/tools/strategic-ai/state/BattleStateTracker.ts` * `sim/tools/strategic-ai/state/OpponentInference.ts` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (11)</summary> * data/random-battles/gen9baby/teams.ts * sim/tools/strategic-ai/mechanics/MoveEvaluator.ts * data/random-battles/gen8/teams.ts * sim/tools/strategic-ai/policy/DifficultyPolicy.ts * sim/tools/strategic-ai/mechanics/TransformPolicy.ts * sim/tools/strategic-ai/engines/Engine.ts * sim/tools/strategic-ai/engines/MctsEngine.ts * sim/tools/player-ai.ts * sim/tools/strategic-ai/engines/HeuristicEngine.ts * sim/tools/selfplay.ts * data/random-battles/gen9/teams.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
sim/tools/strategic-ai/mechanics/DamageCalc.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 18:44:24 +00:00
Copy link

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant section in DamageCalc.ts
wc -l sim/tools/strategic-ai/mechanics/DamageCalc.ts
sed -n '660,720p' sim/tools/strategic-ai/mechanics/DamageCalc.ts

# Find resolveMoveType and Normalize-related logic
rg -n "resolveMoveType|normalize|Normalize|moveType === \"Normal\"|move.type !== \"Normal\" \|\| true" sim/tools/strategic-ai/mechanics -S

Repository: smell-of-curry/pokebedrock-showdown

Length of output: 3981

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the move-type resolver and nearby comments
sed -n '450,510p' sim/tools/strategic-ai/mechanics/DamageCalc.ts

# Inspect the later usages that depend on resolveMoveType for Normalize-sensitive behavior
sed -n '730,760p' sim/tools/strategic-ai/mechanics/DamageCalc.ts
sed -n '1036,1090p' sim/tools/strategic-ai/mechanics/DamageCalc.ts

Repository: smell-of-curry/pokebedrock-showdown

Length of output: 5753

Normalize only boosts already-Normal moves
moveType === "Normal" is already the whole check here; the || true is dead code. The real issue is that resolveMoveType still leaves non-Normal moves unchanged under Normalize, so the 20% boost never reaches the converted moves. Fix the resolver and drop the tautology.

🤖 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 `@sim/tools/strategic-ai/mechanics/DamageCalc.ts` around lines 691 - 693, The
Normalize handling in DamageCalc is incomplete and the `|| true` check is dead
code. Update `resolveMoveType` so Normalize actually converts eligible moves to
Normal before damage is calculated, then keep the boost logic in the `normalize`
case using the resolved move type from `DamageCalc`. Remove the tautology and
make sure the `moveType`/`move.type` check in the `normalize` branch only
applies to moves that are truly Normal after resolution.

✅ Addressed in commits f4568cf to d768907

_🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ <details> <summary>🧩 Analysis chain</summary> 🏁 Script executed: ```shell #!/bin/bash set -euo pipefail # Inspect the relevant section in DamageCalc.ts wc -l sim/tools/strategic-ai/mechanics/DamageCalc.ts sed -n '660,720p' sim/tools/strategic-ai/mechanics/DamageCalc.ts # Find resolveMoveType and Normalize-related logic rg -n "resolveMoveType|normalize|Normalize|moveType === \"Normal\"|move.type !== \"Normal\" \|\| true" sim/tools/strategic-ai/mechanics -S ``` Repository: smell-of-curry/pokebedrock-showdown Length of output: 3981 --- 🏁 Script executed: ```shell #!/bin/bash set -euo pipefail # Read the move-type resolver and nearby comments sed -n '450,510p' sim/tools/strategic-ai/mechanics/DamageCalc.ts # Inspect the later usages that depend on resolveMoveType for Normalize-sensitive behavior sed -n '730,760p' sim/tools/strategic-ai/mechanics/DamageCalc.ts sed -n '1036,1090p' sim/tools/strategic-ai/mechanics/DamageCalc.ts ``` Repository: smell-of-curry/pokebedrock-showdown Length of output: 5753 --- </details> **Normalize only boosts already-Normal moves** `moveType === "Normal"` is already the whole check here; the `|| true` is dead code. The real issue is that `resolveMoveType` still leaves non-Normal moves unchanged under Normalize, so the 20% boost never reaches the converted moves. Fix the resolver and drop the tautology. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sim/tools/strategic-ai/mechanics/DamageCalc.ts` around lines 691 - 693, The Normalize handling in DamageCalc is incomplete and the `|| true` check is dead code. Update `resolveMoveType` so Normalize actually converts eligible moves to Normal before damage is calculated, then keep the boost logic in the `normalize` case using the resolved move type from `DamageCalc`. Remove the tautology and make sure the `moveType`/`move.type` check in the `normalize` branch only applies to moves that are truly Normal after resolution. ``` </details> <!-- fingerprinting:phantom:medusa:capybara --> <!-- cr-indicator-types:potential_issue --> <!-- cr-comment:v1:38e526970fd899cef313e623 --> <!-- This is an auto-generated reply by CodeRabbit --> ✅ Addressed in commits f4568cf to d768907
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:40 +00:00
sim/tools/strategic-ai/state/BattleStateTracker.ts
@ -604,0 +753,4 @@
return;
}
case "item": {
const mon = this.resolveByRef(event.pokemon);
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:40 +00:00
Copy link

False positive. this.active is typed Record<SideId, MonId[]>, so this is an array-element write with an integer index (isValidActivePosition restricts it to 0..2) into the array at a SideId that isSideId validates against a strict "p1" | "p2" | "p3" | "p4" allowlist. The written key is a small bounded integer, not an attacker-controlled string, so __proto__/constructor can never reach the assignment target. No prototype-pollution vector here — resolving.

False positive. `this.active` is typed `Record<SideId, MonId[]>`, so this is an **array-element write with an integer index** (`isValidActivePosition` restricts it to `0..2`) into the array at a `SideId` that `isSideId` validates against a strict `"p1" | "p2" | "p3" | "p4"` allowlist. The written key is a small bounded integer, not an attacker-controlled string, so `__proto__`/`constructor` can never reach the assignment target. No prototype-pollution vector here — resolving.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:41 +00:00
sim/tools/strategic-ai/state/BattleStateTracker.ts
@ -604,0 +773,4 @@
return;
}
case "primal": {
const mon = this.resolveByRef(event.pokemon);
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:41 +00:00
Copy link

False positive. this.active is typed Record<SideId, MonId[]>, so this is an array-element write with an integer index (isValidActivePosition restricts it to 0..2) into the array at a SideId that isSideId validates against a strict "p1" | "p2" | "p3" | "p4" allowlist. The written key is a small bounded integer, not an attacker-controlled string, so __proto__/constructor can never reach the assignment target. No prototype-pollution vector here — resolving.

False positive. `this.active` is typed `Record<SideId, MonId[]>`, so this is an **array-element write with an integer index** (`isValidActivePosition` restricts it to `0..2`) into the array at a `SideId` that `isSideId` validates against a strict `"p1" | "p2" | "p3" | "p4"` allowlist. The written key is a small bounded integer, not an attacker-controlled string, so `__proto__`/`constructor` can never reach the assignment target. No prototype-pollution vector here — resolving.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:42 +00:00
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
@ -2,19 +2,25 @@
* Strategic-AI difficulty policy.
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:42 +00:00
Copy link

Fixed in d768907a1 — the tier-5 row now reads ladder cap 0.00 and "fully greedy", matching knobsForDifficulty(5) (ladderAdvanceCap: 0).

Fixed in d768907a1 — the tier-5 row now reads ladder cap `0.00` and "fully greedy", matching `knobsForDifficulty(5)` (`ladderAdvanceCap: 0`).
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:43 +00:00
sim/tools/strategic-ai/engines/MctsEngine.ts
@ -202,6 +416,341 @@ function sampleRollout(
return reward;
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:43 +00:00
Copy link

Partially addressed in d768907a1: determinizeMoves now seeds its exclusion set from revealed moves only, so the sampled fills are no longer biased away from the foe's real (hidden) moveset. The deeper concern — fully redacting the unrevealed backline — is tracked on the related thread as a follow-up (see reply there).

Partially addressed in d768907a1: `determinizeMoves` now seeds its exclusion set from **revealed moves only**, so the sampled fills are no longer biased away from the foe's real (hidden) moveset. The deeper concern — fully redacting the unrevealed backline — is tracked on the related thread as a follow-up (see reply there).
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:44 +00:00
sim/tools/selfplay.ts
@ -0,0 +1,558 @@
/**
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:44 +00:00
Copy link

Fixed in d768907a1 — a crashed PlayerAI.start() now surfaces through the game Promise.race and is mapped to an errored GameResult, instead of being swallowed and left to the force-tie/hang timers. A successful start() (which resolves at battle end) is deliberately mapped to a never-settling promise so it can't win the race before consume() reads |win|.

Fixed in d768907a1 — a crashed `PlayerAI.start()` now surfaces through the game `Promise.race` and is mapped to an errored `GameResult`, instead of being swallowed and left to the force-tie/hang timers. A *successful* `start()` (which resolves at battle end) is deliberately mapped to a never-settling promise so it can't win the race before `consume()` reads `|win|`.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:47 +00:00
sim/tools/selfplay.ts
@ -0,0 +327,4 @@
worker.on("message", onMessage);
worker.on("error", onError);
worker.on("exit", onExit);
worker.postMessage(task);
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:46 +00:00
Copy link

Fixed in d768907a1 — added an exit listener alongside error that rejects the in-flight task with a clear message, so a worker dying without emitting error errors the game and lets the lane respawn instead of hanging forever.

Fixed in d768907a1 — added an `exit` listener alongside `error` that rejects the in-flight task with a clear message, so a worker dying without emitting `error` errors the game and lets the lane respawn instead of hanging forever.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:48 +00:00
sim/tools/selfplay.ts
@ -0,0 +541,4 @@
maxTurns: intArg(args["max-turns"], "max-turns", 1),
gameTimeoutMs: intArg(args["game-timeout"], "game-timeout", 0),
jobs: intArg(args.jobs, "jobs", 1),
verbose: !args.quiet,
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:48 +00:00
Copy link

Fixed in d768907a1 — numeric flags now go through a validating intArg(raw, name, min, max) helper that rejects NaN/non-integer/out-of-range values with a clear usage error and process.exit(1) (difficulty 0..5, games/max-turns >= 1, budgets/timeout >= 0).

Fixed in d768907a1 — numeric flags now go through a validating `intArg(raw, name, min, max)` helper that rejects `NaN`/non-integer/out-of-range values with a clear usage error and `process.exit(1)` (difficulty `0..5`, games/max-turns `>= 1`, budgets/timeout `>= 0`).
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:49 +00:00
sim/tools/strategic-ai/engines/HeuristicEngine.ts
@ -99,0 +124,4 @@
*/
interface TurnPlan {
/** An earlier slot chose Helping Hand this turn. */
helpingHand: boolean;
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:49 +00:00
Copy link

Won't fix — this is an intentional heuristic approximation, not a correctness bug. The heuristic engine models the foe side as a single tracker.foeActive defender for every slot (defender: foeMon = tracker.foeActive); doubles support here is deliberately coarse, and the precise multi-target search (MCTS) doesn't run doubles at all. Within that single-foe model, partnerAttacks correctly means "the modeled foe already took damage this turn." A target-aware partnerAttacksByTarget would require per-foe-slot modeling this engine doesn't do, and the only impact today is a minor over-valuation of Assurance in doubles. Resolving as a documented limitation.

Won't fix — this is an intentional heuristic approximation, not a correctness bug. The heuristic engine models the foe side as a **single** `tracker.foeActive` defender for every slot (`defender: foeMon = tracker.foeActive`); doubles support here is deliberately coarse, and the precise multi-target search (MCTS) doesn't run doubles at all. Within that single-foe model, `partnerAttacks` correctly means "the modeled foe already took damage this turn." A target-aware `partnerAttacksByTarget` would require per-foe-slot modeling this engine doesn't do, and the only impact today is a minor over-valuation of Assurance in doubles. Resolving as a documented limitation.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:50 +00:00
sim/tools/strategic-ai/engines/MctsEngine.ts
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:50 +00:00
Copy link

Valid, but a known limitation rather than a quick fix. d768907a1 stops determinizeMoves from leaking the foe active's real moves, but fully honest rollouts require redacting/resampling the entire unrevealed foe team (species/sets/HP), which is a substantial redesign of the fork/eval path. That would change rollout outcomes and invalidate the empirically-tuned win-rate, so it needs its own self-play re-validation. In self-play the leak is symmetric, and MCTS (difficulty 5) is the intended hardest tier. Tracking as a follow-up instead of churning the validated engine in this PR — resolving the thread.

Valid, but a known limitation rather than a quick fix. d768907a1 stops `determinizeMoves` from leaking the foe active's real moves, but fully honest rollouts require redacting/resampling the **entire** unrevealed foe team (species/sets/HP), which is a substantial redesign of the fork/eval path. That would change rollout outcomes and invalidate the empirically-tuned win-rate, so it needs its own self-play re-validation. In self-play the leak is symmetric, and MCTS (difficulty 5) is the intended hardest tier. Tracking as a follow-up instead of churning the validated engine in this PR — resolving the thread.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:52 +00:00
sim/tools/strategic-ai/mechanics/DamageCalc.ts
@ -503,22 +505,22 @@ function abilityImmunityBlocks(moveType: string, move: Move, input: DamageCalcIn
const ability = toID(input.defender.ability);
if (move.category === "Status") return false;
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:52 +00:00
Copy link

Fixed in d768907a1 — resolveMoveType now handles Normalize before the type === "Normal" block (if (ability === "normalize") return "Normal";), so a Normalize user's non-Normal move is correctly converted to Normal for both effectiveness and the BP-boost path.

Fixed in d768907a1 — `resolveMoveType` now handles Normalize **before** the `type === "Normal"` block (`if (ability === "normalize") return "Normal";`), so a Normalize user's non-Normal move is correctly converted to Normal for both effectiveness and the BP-boost path.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:54 +00:00
sim/tools/strategic-ai/mechanics/DamageCalc.ts
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:54 +00:00
Copy link

Not a bug — the +1 is correct. Showdown's own gyroball basePowerCallback (data/moves.ts) is Math.floor(25 * target.getStat('spe') / pokemon.getStat('spe')) + 1, capped at 150, which is exactly what this code computes (userSpe is floored at 1 so the min-1 is implicit). Resolving without change.

Not a bug — the `+1` is correct. Showdown's own `gyroball` `basePowerCallback` (data/moves.ts) is `Math.floor(25 * target.getStat('spe') / pokemon.getStat('spe')) + 1`, capped at 150, which is exactly what this code computes (`userSpe` is floored at 1 so the min-1 is implicit). Resolving without change.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:55 +00:00
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:55 +00:00
Copy link

Fixed in d768907a1 — the tier-5 row now reads ladder cap 0.00 and "fully greedy", matching knobsForDifficulty(5) (ladderAdvanceCap: 0).

Fixed in d768907a1 — the tier-5 row now reads ladder cap `0.00` and "fully greedy", matching `knobsForDifficulty(5)` (`ladderAdvanceCap: 0`).
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:56 +00:00
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
@ -102,7 +109,11 @@ function autoFor(difficulty: number): Exclude<EngineName, "auto"> {
*/
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:56 +00:00
Copy link

Fixed in d768907a1 — applyKnobs now unconditionally assigns ctx.searchBudgetMs = knobs.searchBudgetMs. The explicit options.searchBudgetMs override is re-applied by the caller on the next line, and search engines read ctx.searchBudgetMs ?? DEFAULT_BUDGET_MS, so a reused EngineContext can no longer inherit a prior tier's budget and a legitimate 0 is applied.

Fixed in d768907a1 — `applyKnobs` now unconditionally assigns `ctx.searchBudgetMs = knobs.searchBudgetMs`. The explicit `options.searchBudgetMs` override is re-applied by the caller on the next line, and search engines read `ctx.searchBudgetMs ?? DEFAULT_BUDGET_MS`, so a reused `EngineContext` can no longer inherit a prior tier's budget and a legitimate `0` is applied.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:57 +00:00
sim/tools/strategic-ai/state/OpponentInference.ts
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:57 +00:00
Copy link

Fixed in d768907a1 — when every legal ability is an immunity candidate (empty remainder bucket), the full probability mass is now spread over the candidates (1 / candidates.length) so the distribution sums to 1 instead of 0.9.

Fixed in d768907a1 — when every legal ability is an immunity candidate (empty remainder bucket), the full probability mass is now spread over the candidates (`1 / candidates.length`) so the distribution sums to 1 instead of 0.9.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:32:58 +00:00
sim/tools/strategic-ai/state/OpponentInference.ts
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:58 +00:00
Copy link

Fixed in d768907a1 — learnableMoves/legalMoves now take the battle generation, threaded from tracker.gen, instead of the global Dex.gen. So current-gen source gating and level gates match the actual battle gen. The learnset cache key already includes gen, so per-gen caching stays correct. Applies to both the 285-287 and 329-334 sites.

Fixed in d768907a1 — `learnableMoves`/`legalMoves` now take the battle generation, threaded from `tracker.gen`, instead of the global `Dex.gen`. So current-gen source gating and level gates match the actual battle gen. The learnset cache key already includes gen, so per-gen caching stays correct. Applies to both the 285-287 and 329-334 sites.
smell-of-curry (Migrated from github.com) reviewed 2026-07-05 19:33:00 +00:00
sim/tools/strategic-ai/mechanics/DamageCalc.ts
smell-of-curry (Migrated from github.com) commented 2026-07-05 19:32:59 +00:00
Copy link

Fixed in d768907a1 — dropped the || true tautology; the 1.2x now applies only to moves Normalize actually converted (if (move.type !== "Normal")), matching Showdown's typeChangerBoosted rule so already-Normal moves aren't double-counted. Paired with the resolveMoveType fix above so non-Normal moves are converted to Normal first.

Fixed in d768907a1 — dropped the `|| true` tautology; the 1.2x now applies only to moves Normalize actually converted (`if (move.type !== "Normal")`), matching Showdown's `typeChangerBoosted` rule so already-Normal moves aren't double-counted. Paired with the `resolveMoveType` fix above so non-Normal moves are converted to Normal first.
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:00 +00:00
sim/tools/selfplay.ts
@ -0,0 +1,558 @@
/**
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:00 +00:00
Copy link

@smell-of-curry Thanks for the detailed fix — mapping successful start() resolutions to a never-settling promise is a good touch, since it keeps consume() as the authoritative source for the actual |win| outcome while still letting a crash in either AI short-circuit the race immediately. That addresses the concern about failures silently degrading into timeout hangs.

`@smell-of-curry` Thanks for the detailed fix — mapping successful `start()` resolutions to a never-settling promise is a good touch, since it keeps `consume()` as the authoritative source for the actual `|win|` outcome while still letting a crash in either AI short-circuit the race immediately. That addresses the concern about failures silently degrading into timeout hangs. <!-- <review_comment_addressed> --> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:02 +00:00
sim/tools/selfplay.ts
@ -0,0 +327,4 @@
worker.on("message", onMessage);
worker.on("error", onError);
worker.on("exit", onExit);
worker.postMessage(task);
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:02 +00:00
Copy link

@smell-of-curry, thanks for confirming — that resolves the hang scenario cleanly. Appreciate the quick turnaround!

(ᵔ◡ᵔ)

`@smell-of-curry`, thanks for confirming — that resolves the hang scenario cleanly. Appreciate the quick turnaround! (ᵔ◡ᵔ) <!-- <review_comment_addressed> --> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:05 +00:00
sim/tools/selfplay.ts
@ -0,0 +541,4 @@
maxTurns: intArg(args["max-turns"], "max-turns", 1),
gameTimeoutMs: intArg(args["game-timeout"], "game-timeout", 0),
jobs: intArg(args.jobs, "jobs", 1),
verbose: !args.quiet,
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:05 +00:00
Copy link

@smell-of-curry, that closes the gap nicely — the intArg helper with explicit bounds and clear usage errors is exactly what was needed here. Thanks for the fix!

ヽ(•‿•)ノ

`@smell-of-curry`, that closes the gap nicely — the `intArg` helper with explicit bounds and clear usage errors is exactly what was needed here. Thanks for the fix! ヽ(•‿•)ノ <!-- <review_comment_addressed> --> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:08 +00:00
sim/tools/strategic-ai/mechanics/DamageCalc.ts
@ -503,22 +505,22 @@ function abilityImmunityBlocks(moveType: string, move: Move, input: DamageCalcIn
const ability = toID(input.defender.ability);
if (move.category === "Status") return false;
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:08 +00:00
Copy link

@smell-of-curry Confirmed, that fix correctly handles the ordering issue — checking ability === "normalize" before the type === "Normal" guard ensures non-Normal moves are properly converted for both type effectiveness and the BP-boost logic. Thanks for the quick fix!

(‿ˠ‿)

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry` Confirmed, that fix correctly handles the ordering issue — checking `ability === "normalize"` before the `type === "Normal"` guard ensures non-Normal moves are properly converted for both type effectiveness and the BP-boost logic. Thanks for the quick fix! (‿ˠ‿) <!-- <review_comment_addressed> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:08 +00:00
sim/tools/strategic-ai/engines/MctsEngine.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:08 +00:00
Copy link

@smell-of-curry, that's a reasonable tradeoff — re-validating win-rates after a rollout-behavior change shouldn't happen incidentally inside this PR. The partial fix in d768907a1 (redacting the foe active's unrevealed moves) already closes the highest-leverage leak, and the symmetric-leak argument for self-play is fair since both sides get the same treatment during evaluation.

Would you like me to open a follow-up issue to track the full foe-bench redaction (species/sets/HP) for the fork rollout path, so it doesn't get lost?

(°ヘ°) noted — tracking it as a follow-up sounds good to me.

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry`, that's a reasonable tradeoff — re-validating win-rates after a rollout-behavior change shouldn't happen incidentally inside this PR. The partial fix in d768907a1 (redacting the foe active's unrevealed moves) already closes the highest-leverage leak, and the symmetric-leak argument for self-play is fair since both sides get the same treatment during evaluation. Would you like me to open a follow-up issue to track the full foe-bench redaction (species/sets/HP) for the fork rollout path, so it doesn't get lost? (°ヘ°) noted — tracking it as a follow-up sounds good to me. <!-- <review_comment_withdrawn> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:10 +00:00
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:10 +00:00
Copy link

@smell-of-curry Confirmed — the file now shows the tier-5 row as 0.00 ladder cap with "fully greedy" wording, matching knobsForDifficulty(5)'s ladderAdvanceCap: 0. Thanks for the fix!

(=^･ω･^=)

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry` Confirmed — the file now shows the tier-5 row as `0.00` ladder cap with "fully greedy" wording, matching `knobsForDifficulty(5)`'s `ladderAdvanceCap: 0`. Thanks for the fix! (=^･ω･^=) <!-- <review_comment_addressed> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:11 +00:00
sim/tools/strategic-ai/engines/HeuristicEngine.ts
@ -99,0 +124,4 @@
*/
interface TurnPlan {
/** An earlier slot chose Helping Hand this turn. */
helpingHand: boolean;
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:11 +00:00
Copy link

@smell-of-curry, that makes sense — since evalCtx.defender is always tracker.foeActive for every slot in this engine, there's no per-foe-slot state to key partnerAttacks against in the first place, so a target-aware map wouldn't add real precision here. Thanks for the clarification on the doubles model scope; withdrawing this one.

✏️ Learnings added 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 12
File: sim/tools/strategic-ai/engines/HeuristicEngine.ts:104-109
Timestamp: 2026-07-05T19:33:05.225Z
Learning: In `sim/tools/strategic-ai/engines/HeuristicEngine.ts`, the `HeuristicEngine`'s doubles/triples support is intentionally coarse: `decideForSlot` always evaluates against a single `tracker.foeActive` defender for every active slot (not per-foe-slot state), so `TurnPlan.partnerAttacks` (a single boolean meaning "the modeled foe already took damage this turn") is correct within that single-foe model. Precise multi-target doubles modeling is out of scope for this engine — the MCTS search engine does not run doubles at all. Don't flag `partnerAttacks` as needing to be target-aware (e.g., `partnerAttacksByTarget`) unless the engine's foe model is changed to track per-slot foes.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry`, that makes sense — since `evalCtx.defender` is always `tracker.foeActive` for every slot in this engine, there's no per-foe-slot state to key `partnerAttacks` against in the first place, so a target-aware map wouldn't add real precision here. Thanks for the clarification on the doubles model scope; withdrawing this one. <!-- <review_comment_withdrawn> --> --- <details> <summary>✏️ Learnings added</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 12 File: sim/tools/strategic-ai/engines/HeuristicEngine.ts:104-109 Timestamp: 2026-07-05T19:33:05.225Z Learning: In `sim/tools/strategic-ai/engines/HeuristicEngine.ts`, the `HeuristicEngine`'s doubles/triples support is intentionally coarse: `decideForSlot` always evaluates against a single `tracker.foeActive` defender for every active slot (not per-foe-slot state), so `TurnPlan.partnerAttacks` (a single boolean meaning "the modeled foe already took damage this turn") is correct within that single-foe model. Precise multi-target doubles modeling is out of scope for this engine — the MCTS search engine does not run doubles at all. Don't flag `partnerAttacks` as needing to be target-aware (e.g., `partnerAttacksByTarget`) unless the engine's foe model is changed to track per-slot foes. ``` > Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure [review instructions](https://docs.coderabbit.ai/guides/review-instructions). You can manage existing learnings in the [app](https://app.coderabbit.ai/learnings). </details> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:12 +00:00
sim/tools/strategic-ai/policy/DifficultyPolicy.ts
@ -102,7 +109,11 @@ function autoFor(difficulty: number): Exclude<EngineName, "auto"> {
*/
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:12 +00:00
Copy link

@smell-of-curry Confirmed — the file content shows applyKnobs now unconditionally assigns ctx.searchBudgetMs = knobs.searchBudgetMs, which resolves the stale-budget/legitimate-0 issue I flagged. Thanks for the fix!

(´｡• ᵕ •｡`) ♡

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry` Confirmed — the file content shows `applyKnobs` now unconditionally assigns `ctx.searchBudgetMs = knobs.searchBudgetMs`, which resolves the stale-budget/legitimate-`0` issue I flagged. Thanks for the fix! (´｡• ᵕ •｡`) ♡ <!-- <review_comment_addressed> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:14 +00:00
sim/tools/strategic-ai/mechanics/DamageCalc.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:14 +00:00
Copy link

@smell-of-curry Thanks for the correction — you're right, Showdown's actual basePowerCallback for Gyro Ball does include the + 1, and the Math.max(1, ...) floor on userSpe already prevents division issues. My original suggestion was incorrect. Withdrawing that comment.

(๑•̀ㅁ•́๑)✧ my bad, thanks for double-checking against the source!

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry` Thanks for the correction — you're right, Showdown's actual `basePowerCallback` for Gyro Ball does include the `+ 1`, and the `Math.max(1, ...)` floor on `userSpe` already prevents division issues. My original suggestion was incorrect. Withdrawing that comment. <!-- <review_comment_withdrawn> --> (๑•̀ㅁ•́๑)✧ my bad, thanks for double-checking against the source! --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:15 +00:00
sim/tools/strategic-ai/state/OpponentInference.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:15 +00:00
Copy link

@smell-of-curry Confirmed — the current file shows learnableMoves(species, gen) and legalMoves(mon, gen) both threading gen through (sourced from tracker.gen via inferMon), and the cache key ${toID(species)}|${gen} correctly scopes the cache per generation. This resolves the issue at both call sites. Thanks for the fix!

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry` Confirmed — the current file shows `learnableMoves(species, gen)` and `legalMoves(mon, gen)` both threading `gen` through (sourced from `tracker.gen` via `inferMon`), and the cache key `${toID(species)}|${gen}` correctly scopes the cache per generation. This resolves the issue at both call sites. Thanks for the fix! <!-- <review_comment_addressed> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:15 +00:00
sim/tools/strategic-ai/state/OpponentInference.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:15 +00:00
Copy link

@smell-of-curry Confirmed — the updated code in sim/tools/strategic-ai/state/OpponentInference.ts now branches on rest.length === 0 and spreads full mass (1 / candidates.length) across candidates in that case, otherwise keeping the 0.9/0.1 split. This correctly normalizes the distribution to sum to 1.0 in both cases. Thanks for the quick fix!

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry` Confirmed — the updated code in `sim/tools/strategic-ai/state/OpponentInference.ts` now branches on `rest.length === 0` and spreads full mass (`1 / candidates.length`) across candidates in that case, otherwise keeping the `0.9`/`0.1` split. This correctly normalizes the distribution to sum to 1.0 in both cases. Thanks for the quick fix! <!-- <review_comment_addressed> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
coderabbitai[bot] (Migrated from github.com) reviewed 2026-07-05 19:33:16 +00:00
sim/tools/strategic-ai/mechanics/DamageCalc.ts
coderabbitai[bot] (Migrated from github.com) commented 2026-07-05 19:33:16 +00:00
Copy link

@smell-of-curry, confirmed — that resolves the issue correctly and aligns with Showdown's typeChangerBoosted semantics. Thanks for the fix!

🐇✨

🧠 Learnings used 
Learnt from: smell-of-curry
Repo: smell-of-curry/pokebedrock-showdown PR: 10
File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158
Timestamp: 2026-04-28T23:02:52.579Z
Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index.
`@smell-of-curry`, confirmed — that resolves the issue correctly and aligns with Showdown's `typeChangerBoosted` semantics. Thanks for the fix! 🐇✨ <!-- <review_comment_addressed> --> --- <details> <summary>🧠 Learnings used</summary> ``` Learnt from: smell-of-curry Repo: smell-of-curry/pokebedrock-showdown PR: 10 File: sim/tools/strategic-ai/engines/RandomEngine.ts:149-158 Timestamp: 2026-04-28T23:02:52.579Z Learning: When building requests/decisions using `activeSlots.map` inside the strategic AI, treat `myPokemon[i]` (or the source array used for active slots) as the active-slot–ordered Pokémon. This is correct because the simulator maintains the invariant that active Pokémon occupy the first `activeSlots.length` indices of `side.pokemon`, and the request serializer iterates `side.pokemon` in index order to produce `request.active[i]` for `i < activeSlots.length`. Therefore, avoid applying a “team order vs active-slot order” mismatch inside `activeSlots.map` callbacks; `i` refers to the active-slot index. ``` </details> <!-- This is an auto-generated reply by CodeRabbit -->
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something isn't working

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
pokebedrock/pokebedrock-showdown!12
No description provided.