Add strategic AI framework and PlayerAI #10
No reviewers
Labels
No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
pokebedrock/pokebedrock-showdown!10
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ai-update"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Introduce a new strategic-AI system and replace the legacy RandomPlayerAI surface with a unified PlayerAI. Key additions: an Engine interface and EngineContext, multiple engine implementations (RandomEngine, HeuristicEngine, MctsEngine, OnePlySearchEngine, LightHeuristicEngine), mechanics/evaluators (DamageCalc, MoveEvaluator, SwitchEvaluator, TargetPicker, TransformPolicy), policy and state modules (DifficultyPolicy, BattleStateTracker, LogParser, OpponentInference), and tests for the new modules. The PlayerAI class now hosts engines, plumbs protocol logs into BattleStateTracker, and forwards ChoiceRequests to selected engines; BattlePlayer.receiveRequest's signature was broadened to allow returning a choice string. Runners and examples were updated to use PlayerAI (defaulting to the random engine for compatibility), exhaustive-runner wires a CoordinatedRandomEngine to share pools, and the old sim/tools/random-player-ai.ts was removed. Migration note: callers importing RandomPlayerAI should switch to PlayerAI (pass engine: 'random' to retain prior behavior) and adjust for the altered receiveRequest return semantics.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
📝 Walkthrough
Walkthrough
Modular strategic-AI replaces legacy Random/Strong heuristics with a new PlayerAI host, a pluggable Engine interface (Random/Light/Heuristic/OnePly/MCTS), battle-state tracking, damage/target/switch/transform mechanics, opponent inference, widened BattlePlayer.receiveRequest signature, and runner/example/test updates.
Changes
sim/battle-stream.ts,sim/index.tsBattlePlayer.receiveRequest(request: ChoiceRequest)return type tostring | undefined | void; replacedRandomPlayerAI/StrongHeuristicsAIexports withPlayerAIand exportedPlayerAIOptions.sim/tools/player-ai.tsPlayerAIandPlayerAIOptions: PRNG/difficulty init, lazy BattleStateTracker creation, buffered log parsing, error-handling for disabled moves, and request→engine dispatch.sim/tools/strategic-ai/engines/Engine.ts,sim/tools/strategic-ai/policy/DifficultyPolicy.ts,sim/tools/strategic-ai/index.tsEngineinterface andEngineContext, noise/info-forgetting knobs, difficulty→engine mapping (pickEngine,knobsForDifficulty,applyKnobs), and a barrel export for strategic-ai.sim/tools/strategic-ai/engines/RandomEngine.ts,.../LightHeuristicEngine.ts,.../HeuristicEngine.ts,.../OnePlySearchEngine.ts,.../MctsEngine.tssim/tools/strategic-ai/mechanics/DamageCalc.ts,.../SwitchEvaluator.ts,.../TargetPicker.ts,.../TransformPolicy.ts,.../MoveEvaluator.tssim/tools/strategic-ai/state/BattleStateTracker.ts,.../LogParser.ts,.../OpponentInference.tssim/tools/runner.ts,sim/tools/exhaustive-runner.tsBattleStreams.BattlePlayer; default AI construction switched toPlayerAIwith random engine; exhaustive runner wired toCoordinatedRandomEngineandfixMoveId.sim/tools/random-player-ai.ts,sim/tools/strategic-player-ai.tsRandomPlayerAIandStrongHeuristicsAIimplementations (functionality replaced by RandomEngine + PlayerAI-hosted engines).sim/examples/battle-stream-example.ts,test/sim/tools/strategic-ai/*PlayerAIwith{ engine: "random" }; added tests for DamageCalc, LogParser, SwitchEvaluator, and a difficulty-gradient harness.sim/tools/strategic-ai/...,sim/side.tssim/side.tsadjusted multi-battle null-active handling and refined choice parsing.Sequence Diagram(s)
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
ai-updateThanks 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 helpto get the list of available commands and usage tips.@ -0,0 +1,234 @@/**CodeQL / Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with '[] ' and with many repetitions of '] a'.
This regular expression that depends on library input may run slow on strings starting with '[] : ' and with many repetitions of ': a'.
This regular expression that depends on library input may run slow on strings starting with '[] ' and with many repetitions of '] a'.
This regular expression that depends on library input may run slow on strings starting with '[] : ' and with many repetitions of ': a'.
This regular expression that depends on library input may run slow on strings starting with '[] ' and with many repetitions of '] a'.
This regular expression that depends on library input may run slow on strings starting with '[] : ' and with many repetitions of ': a'.
Show more details
CodeQL / Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with 'a's ' and with many repetitions of 'a's a'.
This regular expression that depends on library input may run slow on strings starting with 'a's ' and with many repetitions of 'a's a'.
This regular expression that depends on library input may run slow on strings starting with 'a's ' and with many repetitions of 'a's a'.
Show more details
@ -0,0 +1,929 @@/**CodeQL / Prototype-polluting assignment
This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.
Show more details
@ -0,0 +500,4 @@this.sides[event.pokemon.side].fainted += 1;const slotIdx = event.pokemon.position;if (isValidActivePosition(slotIdx) && this.active[event.pokemon.side][slotIdx] === mon.id) {this.active[event.pokemon.side][slotIdx] = "";CodeQL / Prototype-polluting assignment
This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.
Show more details
Pull request overview
This PR introduces a new strategic AI framework with a unified
PlayerAIhost that routes decision-making to pluggable engines (random → heuristic → search), while replacing/removing the legacyRandomPlayerAIandStrongHeuristicsAIsurfaces. It also adds a battle state tracker + protocol log parser so higher-tier engines can reason over hazards, revealed moves, boosts, items, and field state.Changes:
PlayerAIhost shell + engine interface (Engine,EngineContext) and difficulty policy mapping difficulty→engine/knobs.PlayerAI(defaulting to random engine) and added tests for core strategic-AI modules.Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
PlayerAI.request+ log events to maintain rich battle state.RandomPlayerAI.PlayerAIhost that plumbs logs into tracker and dispatches to engines.PlayerAI(engine: 'random').RandomEnginesubclass.PlayerAIinstead of legacy AIs.PlayerAI(engine: 'random').BattlePlayer.receiveRequestreturn type to support returning a choice string.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -0,0 +1,234 @@/**receiveErrorstores disabled moves under a${side}|${name}key, but the engines keydisabledMovesByMonby the monId returned frommonIdForSlot(which prefersreq.uuid). SincePokemonSwitchRequestData.uuidis present, disabled moves will never be filtered out, causing the AI to potentially repeat invalid choices. Consider resolving the UUID from the latest request/tracker and storing the disabled move under that UUID key (optionally also under the${side}|${name}fallback).@ -0,0 +1,226 @@/**This doc comment includes an absolute local filesystem path (
D:\bedrock-server\...) inside a markdown link. That won’t resolve for other contributors and is likely to end up stale. Prefer a repo-relative link (or plain file reference likesim/battle.ts:L327-L333) instead.@ -0,0 +111,4 @@if (chosenSlots.includes(benchSlot)) return false;const fainted = benchPoke.condition.endsWith(" fnt");if (fainted && !benchPoke.reviving) return false;return true;forceSwitchfilters outbenchSlot <= mustSwitchFlags.lengthinstead of excluding currently-active Pokémon viabenchPoke.active. SinceSide.getRequestData()preserves team order (active mons are not guaranteed to be the first N entries), this can both (a) wrongly exclude valid switch-ins (e.g. slot 1) and (b) allow switching to an active slot if the active mon isn't in the first N positions. UsebenchPoke.active(andchosenSlots) to filter actives rather than relying on list position.@ -0,0 +1,929 @@/**hazardDamageFraction's Spikes grounding check ignores field-wide grounding effects like Gravity. Under Gravity, Flying/Levitate/Air Balloon mons should be treated as grounded for Spikes damage, but this code will still exempt them. Consider incorporatingthis.field.gravityinto thegroundedcalculation (or reusingisGroundedlogic where feasible).Typo in comment: "faiunted" → "fainted".
The
enditemhandler has a conditional intended to treat item swaps differently, but both branches setmon.item = ""and the comment suggests behavior that isn’t implemented. This is misleading and can be simplified; if special-casing is needed for certainevent.fromvalues, implement it explicitly, otherwise remove the dead conditional/comment.@ -0,0 +1,268 @@/**The module header comment says unrevealed moves are sampled from the species' learnset "intersected with the format's legal moves", but
legalMoves()currently just returns all learnset moves it finds inDex.data.Learnsetsand doesn't take a format/ruleset into account. Either implement the format-legal intersection (by passing format/rule context into inference) or adjust the comment to match current behavior to avoid overstating inference correctness.Actionable comments posted: 11
🟡 Minor comments (16)
🧹 Nitpick comments (14)
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
7eed26c0-a7eb-4116-b63b-eaf23b9c308b📥 Commits
Reviewing files that changed from the base of the PR and between
62ea003476and457d8c7ba4.📒 Files selected for processing (28)
sim/battle-stream.tssim/examples/battle-stream-example.tssim/index.tssim/tools/exhaustive-runner.tssim/tools/player-ai.tssim/tools/random-player-ai.tssim/tools/runner.tssim/tools/strategic-ai/engines/Engine.tssim/tools/strategic-ai/engines/HeuristicEngine.tssim/tools/strategic-ai/engines/LightHeuristicEngine.tssim/tools/strategic-ai/engines/MctsEngine.tssim/tools/strategic-ai/engines/OnePlySearchEngine.tssim/tools/strategic-ai/engines/RandomEngine.tssim/tools/strategic-ai/index.tssim/tools/strategic-ai/mechanics/DamageCalc.tssim/tools/strategic-ai/mechanics/MoveEvaluator.tssim/tools/strategic-ai/mechanics/SwitchEvaluator.tssim/tools/strategic-ai/mechanics/TargetPicker.tssim/tools/strategic-ai/mechanics/TransformPolicy.tssim/tools/strategic-ai/policy/DifficultyPolicy.tssim/tools/strategic-ai/state/BattleStateTracker.tssim/tools/strategic-ai/state/LogParser.tssim/tools/strategic-ai/state/OpponentInference.tssim/tools/strategic-player-ai.tstest/sim/tools/strategic-ai/damage-calc.jstest/sim/tools/strategic-ai/difficulty-gradient.jstest/sim/tools/strategic-ai/log-parser.jstest/sim/tools/strategic-ai/switch-evaluator.js💤 Files with no reviewable changes (2)
@ -0,0 +365,4 @@let defStat = (isPhysical ? def.stats?.def : def.stats?.spd) ?? 100;atkStat *= boostMul(atk.boosts?.[isPhysical ? "atk" : "spa"] || 0);defStat *= boostMul(def.boosts?.[isPhysical ? "def" : "spd"] || 0);const ability = (atk.ability || atk.baseAbility || "").toLowerCase();⚠️ Potential issue | 🟠 Major
simpleDamagewill throw when foestatsis undefined.PokemonSwitchRequestData.statsis regularly absent for foes whose set hasn't been revealed (the rest of this file already guards withme.stats?.spe ?? 0/foe.stats?.spe ?? 0). Lines 349–352 accessatk.stats.atk,atk.stats.spa,def.stats.def,def.stats.spddirectly, so the very first damage estimate against an unrevealed foe will throwTypeError: Cannot read properties of undefined, which propagates out ofdecideForSlotand breaks the engine for that turn.🛡️ Guard with optional chaining + sane defaults
Pick a more principled default if you have a species-based estimator handy (e.g.
estimateMaxHp-style stats), but at minimum stop dereferencing throughundefined.📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +284,4 @@}}return chosenMove;});⚠️ Potential issue | 🟠 Major
Mega/Dynamax/Terastallize exclusivity isn't enforced across active slots.
canMegaEvo,canDynamax,canTerastallize,canUltraBurstare declared inside the.map(active, i)callback, so each active slot has its own copy. In doubles bothactive[0].canMegaEvoandactive[1].canMegaEvocan betrue(the request just signals "either mon is eligible"); each iteration independently rollsdoTransformagainstmegaProband can emit... mega(ordynamax/terastallize) for both slots. Showdown only allows one mega/dynamax/tera per battle, so the second command will be rejected or silently dropped — corrupting test runs and confusing replays generated by the runners.🔧 Lift transformation flags out of the per-slot callback
🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +1,943 @@/**⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 50392
Tera-on-existing-STAB never gets its 2.0×/2.25× multiplier.
Callers (e.g.
TransformPolicy.teraOffensiveValue, line 131) setattacker.types = [teraType]andterastallized: truewhen terastallizing. With that, line 287'smoveTypeMatchesUserTypereturns true whenevermoveType === teraType, and the block at 288–291 applies 1.5× (or 2.0× with Adaptability). The dedicated Tera bonus at line 293 then short-circuits because!stabAppliesis false.The actual gen 9 rule for Tera-into-original-STAB is 2.0× (and 2.25× with Adaptability). In its current form, this calc undercounts that case by ~33% (or ~12.5% with Adaptability), which directly biases
teraOffensiveValueand KO-conversion checks inTransformPolicy.The CalcPokemon shape doesn't carry the original types post-Tera, so a clean fix likely needs an extra field (e.g.
originalTypes) or for callers to keep the original types and only setteraType+terastallized: true. Worth fixing now while there's only one caller.🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +707,4 @@if (useStage > 0) value = Math.floor(value * (2 + useStage) / 2);else if (useStage < 0) value = Math.floor(value * 2 / (2 - useStage));return value;}⚠️ Potential issue | 🟠 Major
Crit ignores stat stages in the wrong direction (defender) and not at all (attacker).
Showdown's crit rules are: ignore the defender's positive defensive stages and the attacker's negative offensive stages — they're the boosts that would otherwise help the side hit by the crit. The current code does the opposite for the defender and skips it entirely for the attacker:
computeDefensiveStat(line 647) callseffectiveStat(defender, stat, !!forceCrit). WithignoreNegativeBoost=true,effectiveStatclampsstage < 0to 0 (line 669). That zeroes the defender's negative defensive stages — which actually helped us — and leaves positive stages intact, so a +6 Def defender keeps full +6 against our crit.computeOffensiveStat(line 612) never passesforceCrit, so an attacker with -2 Atk from Intimidate still has its negative stage applied during a crit.This will materially over-/under-count expected damage in any rollout that uses
forceCrit: true, and the MCTS/OnePly engines do rely on those numbers.Suggested fix
…and update the call sites:
(Body Press / Foul Play call sites need the same treatment for the stat they actually read.)
✅ Addressed in commit
ed4d2de@ -0,0 +1,466 @@/**⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 3090
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 131
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 893
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 61
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 474
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 494
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 493
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 416
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 954
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 529
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 2396
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 1626
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 531
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 710
Foe-target debuff moves are misclassified as self-boosts.
The condition
move.boosts || move.self?.boostsgates intoscoreBoostMove, which does not distinguish between self-boosts and opponent debuffs. Moves likegrowl(atk: -1, target: "allAdjacentFoes"),charm(atk: -2, target: "normal"), andtailwhip(def: -1, target: "allAdjacentFoes") have negative stat changes inmove.boostswith non-self targets. When routed throughscoreBoostMove, they produce negative scores (e.g., Growl → -1 * 9 = -9), making them worse than unknownStatus (score 2), so the AI avoids them.Restrict the boost branch to genuine self-boosts by checking
move.target === "self"alongsidemove.self?.boosts, and route foe-target debuffs to a dedicated debuff scorer.As a minor follow-up: the selfdestruct check at line 124 includes
m.selfdestruct === true, which is dead code. Move data only uses string literals ("always", "ifHit"), never boolean true.🛠 Suggested fix
…and a companion scorer that inverts the sign and applies diminishing returns against the defender's current boosts.
🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +1,280 @@/**⚠️ Potential issue | 🟠 Major
Side state is swapped when scoring foe → us damage.
bestAttackingDamagehardcodesattackerSide: tracker.sides[tracker.mySide]anddefenderSide: tracker.sides[tracker.foeSide](lines 215–216). That works for the us → foe call on line 89, but for the foe → us call on line 87 the attacker is the foe and the defender is us, so the sides are inverted:defenderSideis consumed inDamageCalcfor Reflect / Light Screen / Aurora Veil (seesim/tools/strategic-ai/mechanics/DamageCalc.tslines 336–339). With it pointing atfoeSide, the foe's screens get applied to its own attacks against us, while our screens get ignored entirely.attackerSideis documented to carry Tailwind-style state and would be similarly wrong.Net effect: matchup scores systematically misweigh
foeDealFractionwhenever either side has screens / tailwind up — exactly the situations switch decisions are most sensitive to.Suggested fix: thread the side ids through to the calc
…and update the two call sites in
evaluateMatchup:Also applies to: 209-217
🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +1,148 @@/**⚠️ Potential issue | 🟠 Major
Follow Me / Rage Powder penalty likely applied to the wrong slot.
Follow Me and Rage Powder redirect every single-target move on our side to the user, regardless of which foe slot we explicitly target — picking the other foe doesn't avoid the redirection. As written, lines 119–121 penalize the redirector, biasing us to target the non-redirector even though the actual hit will still land on the redirector. The net effect is "we lie to ourselves about the target" with no mechanical benefit.
If the intent is "don't waste a kill move on the support mon," that's the opposite signal of what this produces. If the intent is "approximate that we can ignore the support mon," the penalty should be applied to the other foes (so the redirector becomes the highest-scoring target, since the move ends up there). A reasonable rewrite:
🔧 Apply redirection penalty to non-redirector slots
Same comment applies to
stormdrain/lightningrodon the foe side at lines 114–118 — those are full redirections too, not partial ones. Consider unifying the redirection model.🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +673,4 @@mon.baseAbility = mon.ability;}return;}⚠️ Potential issue | 🟠 Major
baseAbilityis incorrectly inferred from Trace/Skill Swap/Imposter events.The handler treats every
|-ability|event as a base-ability reveal:if (!mon.baseAbility) mon.baseAbility = mon.ability;runs even whenevent.fromisability: Trace,ability: Imposter,move: Skill Swap, etc. The first time we observe such a mon, itsbaseAbilityis locked to the acquired ability and persists across subsequent switches — which will mislead any consumer that usesbaseAbilityto reason about the foe's set (e.g. switch evaluators projecting hazard interactions on switch-in).Proposed fix — only seed `baseAbility` on a non-derived reveal
🤖 Prompt for AI Agents
@ -0,0 +823,4 @@else if (ss.spikes >= 3) damage += 1 / 4;}return damage;}⚠️ Potential issue | 🔴 Critical
hazardDamageFractionreimplements (incorrectly) the grounded check fromisGrounded.The inline grounded check on lines 779-781 only considers Flying type, Levitate, and Air Balloon. It misses every other relevant condition that
isGrounded(lines 751-762) handles, plus a coupleisGroundeditself omits:Since this method drives switch evaluation (
SwitchEvaluatorper the AI summary), wrong hazard estimates will translate directly into bad switch decisions. Please extract a single mon-keyed groundedness helper and reuse it from bothisGroundedandhazardDamageFraction.Proposed extraction
Also: the
isGroundeddocstring promises to handle Roost (lines 746-750) but the code does not check theroostvolatile. Either drop the mention or add the check (Roost suppresses Flying type for the turn — for a mid-turn EQ it matters; for switch-in hazards it doesn't, hence harmless to omit fromhazardDamageFraction).🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +223,4 @@dist.set("leftovers", leftoversScore * scale);dist.set("assaultvest", avScore * scale);dist.set("", rest);return dist;⚠️ Potential issue | 🟠 Major
Item distribution can exceed 1.0; violates the
Distributioninvariant.The
Distributiontype contract (line 39) says values sum to ~1.0, and downstream MCTS/search rollouts will treat this as a probability. But the heuristic mass added ininferItemsis unbounded — e.g. an NFE foe that's been on the field through hazards while same-move-streaking once getsbootsScore 0.5 + evioliteScore 0.6 + choiceScore 0.6 + leftovers/av/sash 0.15 ≈ 1.85, andrest = max(0, 1 - totals) = 0clamps the unknown bucket but does not renormalize. The map values are then written directly (lines 199–208), so the distribution sums to ~1.85.Suggested fix: normalize before writing
🤖 Prompt for AI Agents
✅ Addressed in commit
ed4d2de@ -0,0 +1,215 @@'use strict';⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 3260
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 5518
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 29261
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 11389
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 3527
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 95
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-showdown
Length of output: 1257
The test fixture
parseLine('|win|p2')does not match the actual Showdown protocol, which emits|win|<player name>, and applyEvent has no handler for win events.Real battle logs send
|win|<player name>(e.g.,|win|Bot 1), not|win|p2. The parser's regex/^p[1-4]$/rejects player names, causing parseLine to return{ kind: 'win', side: undefined }. Additionally,BattleStateTracker.applyEvent()contains no case handler forwinortieevents, so even if the parser produced the correct format, the event would silently fall through to the default case and be discarded. This means real battle-end signals are lost during integration withBattleStateTracker. The test should either use the actual protocol format or be renamed to clarify it tests a synthetic subset.🤖 Prompt for AI Agents
🧹 Nitpick comments (3)
🤖 Prompt for all review comments with AI agents
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
e7e05e7d-454e-44f0-8b88-6b7737b831db📥 Commits
Reviewing files that changed from the base of the PR and between
457d8c7ba4and3f75be5180.📒 Files selected for processing (1)
sim/side.ts@cursor review
@ -0,0 +486,4 @@isSideId(event.pokemon.side) &&isValidActivePosition(event.pokemon.position)) {this.active[event.pokemon.side][event.pokemon.position] = mon.id;CodeQL / Prototype-polluting assignment
This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.
Show more details
@bugbot review
Actionable comments posted: 13
♻️ Duplicate comments (1)
🧹 Nitpick comments (1)
🤖 Prompt for all review comments with AI agents
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
95dde378-8c60-49e1-a970-340d1e0bfaa1📥 Commits
Reviewing files that changed from the base of the PR and between
3f75be5180anded4d2de93b.📒 Files selected for processing (13)
sim/tools/player-ai.tssim/tools/strategic-ai/engines/LightHeuristicEngine.tssim/tools/strategic-ai/engines/MctsEngine.tssim/tools/strategic-ai/engines/RandomEngine.tssim/tools/strategic-ai/mechanics/DamageCalc.tssim/tools/strategic-ai/mechanics/MoveEvaluator.tssim/tools/strategic-ai/mechanics/SwitchEvaluator.tssim/tools/strategic-ai/mechanics/TargetPicker.tssim/tools/strategic-ai/mechanics/TransformPolicy.tssim/tools/strategic-ai/state/BattleStateTracker.tssim/tools/strategic-ai/state/LogParser.tssim/tools/strategic-ai/state/OpponentInference.tstest/sim/tools/strategic-ai/log-parser.js@ -0,0 +1,234 @@/**⚠️ Potential issue | 🟡 Minor
Disabled-move parsing fails for Pokémon names containing apostrophes.
Line 181 uses
^([^']+)'s ..., which breaks on names likeFarfetch'd, sodisabledMovesByMonmay not be updated.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +202,4 @@}return target ? `move ${best.idx} ${target}` : `move ${best.idx}`;}case "adjacentAlly":⚠️ Potential issue | 🔴 Critical
Don't hard-code foe target
1in doubles.For targetable foe moves, this always emits
move N 1. If the only live foe is in slot2, or slot1is empty after a KO, the command is illegal and the turn fails. The engine can stay "light" and still choose the first live foe slot instead of a fixed constant.✅ Addressed in commits
7929f94to45f24bd@ -0,0 +155,4 @@const canUltraBurst = !!active.canUltraBurst && !transformsUsed.ultra;let canZMove = !!active.canZMove;const canDynamax = !!active.canDynamax && !transformsUsed.dynamax;const canTerastallize = !!active.canTerastallize && !transformsUsed.tera;⚠️ Potential issue | 🔴 Critical
Don't look up the acting mon by team order here.
This reintroduces the indexing bug the force-switch path already avoids:
request.active[i]is active-slot order, butside.pokemon[i]is team order. When the active mons are not the firstNentries, Lines 150-158 read the wrong Pokémon'scondition/commanding/ transform flags and can emitpassor a move for the wrong slot.🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +258,4 @@choiceStr += ` -${i + 1}`;}}}⚠️ Potential issue | 🔴 Critical
Pick from live foe slots instead of hard-coding
{1, 2}.For
normal/any/adjacentFoemoves, Line 234 chooses1or2blindly. After a KO, that can point at an empty or fainted foe slot, so the simulator rejects an otherwise legal move.Suggested fix
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +302,4 @@// overwriting `types` with `[teraType]`.const teraOnOriginalStab = teraMatchesMove && !!attacker.originalTypes &&attacker.originalTypes.includes(moveType);if (teraOnOriginalStab) {⚠️ Potential issue | 🟠 Major
fromTrackeddoesn’t populateoriginalTypes, so tera-on-existing-STAB logic is usually inert.The STAB branch depends on
attacker.originalTypes(Lines 303-305), butfromTrackednever sets it, so tracker-based evaluations miss the boosted Tera-existing-STAB case.Suggested fix
Also applies to: 916-930
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +75,4 @@}if (m.category === "Status") {return evaluateStatus(m, moveId, ctx);}⚠️ Potential issue | 🟠 Major
rapidspin/mortalspinhazard-removal scoring is unreachable in current control flow.evaluateStatusis only called for Status-category moves (Line 76), butrapidspinandmortalspinare damaging moves, so their hazard-removal bonus here never executes.Suggested fix
Also applies to: 207-219
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +175,4 @@if (attacker.status === "tox" || attacker.status === "psn") score -= 5;rationale = "recover";return { moveId, score, rationale };}⚠️ Potential issue | 🟠 Major
wishis currently scored by the generic recovery path, making the dedicatedwishbranch dead.Because
isRecoveryMoveincludes"wish"(Line 324), execution returns at Line 161 and never reaches the customwishlogic at Lines 284-287.Suggested fix
Also applies to: 283-287, 311-325
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +1,280 @@/**⚠️ Potential issue | 🟠 Major
Gate Swift Swim on rain before doubling Speed.
scaledSpeed()currently doubles Speed for everyswiftswimmon, even in clear weather. That skewscomputeSpeedDelta()and can easily push the switch heuristic toward the wrong candidate for the whole battle.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +76,4 @@) {return null;}if (foeSlots.length === 0) return null;⚠️ Potential issue | 🔴 Critical
Handle ally/self target classes before scoring foe slots.
adjacentAllyandadjacentAllyOrSelffall through into the foe-target path, but this function only ever returns positive foe indices. In doubles that yields illegal commands for ally-targeted moves like Helping Hand or Coaching instead of the negative indices thatTargetIndexalready models.🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +1,303 @@/**⚠️ Potential issue | 🟠 Major
Avoid auto-consuming Z on status moves.
This branch always returns
zmovefor status moves once a matching Z slot exists, which can burn the one-shot resource on routine setup turns and preempt Mega/Dynamax by priority ordering.Suggested minimal fix
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +1,929 @@/**⚠️ Potential issue | 🟠 Major
Item updates can remain stale when the request reports an empty item.
Line 368 (
data.item || tracked.item) drops valid empty-string updates, so removed/consumed items can persist incorrectly in tracker state.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +1,478 @@/**⚠️ Potential issue | 🟡 Minor
Don't coerce malformed numeric fields into valid state.
|turn|fooand|teamsize|p1|foocurrently become{kind: 'turn', turn: 0}/{kind: 'teamsize', size: 0}becauseparseInt(...) || 0treatsNaNas falsy. That fabricates tracker state instead of returningnull, which is the contract documented at the top of this parser.Suggested fix
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +133,4 @@for (const id of unseen) {dist.set(id, (dist.get(id) || 0) + per);}return dist;⚠️ Potential issue | 🟠 Major
Normalize the move distribution when no unseen moves remain.
If
legalMoves()returns nothing, or every legal move is already revealed, this returns a distribution whose total mass isrevealedShareinstead of1. That breaks theDistributioncontract and underweights fully-known move sets for any consumer that treats these values as probabilities.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commits
7929f94to45f24bd@ -0,0 +155,4 @@const canUltraBurst = !!active.canUltraBurst && !transformsUsed.ultra;let canZMove = !!active.canZMove;const canDynamax = !!active.canDynamax && !transformsUsed.dynamax;const canTerastallize = !!active.canTerastallize && !transformsUsed.tera;Skipping this fix: Showdown maintains the invariant that
side.pokemonkeeps the active Pokemon at the first N indices. SeeBattle#swap(sim/battle.ts:1561-1562) which mirrors swaps intoside.pokemon. The request data is built directly fromside.pokemon(sim/side.ts:355-357), somyPokemon[i]fori < activeSlots.lengthis the same Pokemon asrequest.active[i]. The legacyRandomPlayerAIhas relied on this invariant in production. Resolving.@ -0,0 +155,4 @@const canUltraBurst = !!active.canUltraBurst && !transformsUsed.ultra;let canZMove = !!active.canZMove;const canDynamax = !!active.canDynamax && !transformsUsed.dynamax;const canTerastallize = !!active.canTerastallize && !transformsUsed.tera;🧩 Analysis chain
🏁 Script executed:
Length of output: 1158
@smell-of-curry, confirmed — the source code backs up your explanation.Battle#swapkeepsside.pokemonandside.activein sync (lines 1561–1564 ofsim/battle.ts), and the request serializer insim/side.tsiteratesthis.pokemondirectly in that same order. SomyPokemon[i]fori < activeSlots.lengthis guaranteed to be the same object asrequest.active[i]. My original concern was unfounded. Thanks for the detailed pointer!✏️ Learnings added
@ -0,0 +181,4 @@// Lazy `(.+?)` for the owner (allows apostrophes in names like// `Farfetch'd`), anchored end + literal ` is disabled` keep the// match bounded so this doesn't backtrack pathologically.const m2 = /^(.+?)'s (.+) is disabled$/.exec(message);CodeQL / Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with 'a's ' and with many repetitions of 'a's a'.
This regular expression that depends on library input may run slow on strings starting with 'a's ' and with many repetitions of 'a's a'.
This regular expression that depends on library input may run slow on strings starting with 'a's ' and with many repetitions of 'a's a'.
Show more details