Fix bots using skills they should not have access to

[eduardosmaniotto]

Summary

Fixes #943 — server-side bots were fighting with skills no legitimate character
in their situation could use (Impale on foot, Evil Spirit before its scroll
could drop, plus Fire Breath / Plasma Storm of the same kind).

What changed

• Loot-based skill learning (new BotSkillHandler): bots pick up orbs/scrolls
  teaching unknown, consumable skills (ItemPickupHandler) and consume them through
  the regular ItemConsumeAction on the navigator's equipment cadence — same gates
  a human faces. Level-ups grant no item skills anymore (BotSkillProgressionPlugIn).
• Plausible kit at generation (BotGenerator): backfilled skills additionally require
  granting-item obtainability (drop level + item requirements), so the fresh-start flag
  controls the starting kit — fresh level-1 bots start with almost nothing.
• Mount-bound skills off-limits (BotProgression, CombatHandler): Impale (47),
  Fire Breath (49) and Plasma Storm (76) are never learned, looted, or selected by bots;
  the combat skip is bot-only, so humans keep their mounted skills. No mount detection.
• Fresh starts spawn armorless (BotStartupProfile.EquipStarterArmor): weapon only,
  like a regular new character; veterans keep the basic set. Gear equipping extracted
  from BotGenerator into BotStarterGearEquipper (SRP; shared state as fields).
• Docs (bots.md) and new BotProgressionTests / BotStartupProfileTests cases.

Verification

• Bot + Offline suites green (117/117); full suite was 859/859 green before the gear
  refactor — please re-run once more before merging.
• Existing populations keep already-learned skills in the DB (nothing is deleted);
  mount-bound ones go silent immediately. Use Reset bots once for a fully clean
  population.

[sven-n]

Review

Reviewed c172d98 (12 files) statically — no .NET SDK in my environment, so I did not build or run the suites; everything below comes from reading the diff against master and the code it touches.

The direction is right: moving orb/scroll skills onto the loot-and-consume path via the real ItemConsumeAction/LearnablesConsumeHandlerPlugIn is the correct fix for #943, and gating BotSkillProgressionPlugIn on the whole item-granted set (rather than only those without own requirements) closes the actual hole. RequiresMount as a hard list also correctly covers already-learned skills at the combat-selection stage, and keeping the skip bot-only leaves humans alone. The BotStarterGearEquipper extraction is a clean move — the whole armor/weapon/potion block came over unchanged.

A few things I'd want addressed before merge.

1. A looted orb can be sold before it is ever consumed

BotShoppingHandler.GetSellableJunk wasn't updated. An orb or scroll is not a jewel, not a potion in HealingHandler's priority lists, not ammunition, and BotEquipmentHandler.IsUpgradeFor returns false for it (TryPlanSwap bails at IsWearableCandidate), so it falls straight through to junk.Add(item) (BotShoppingHandler.cs:244).

Concretely: a bot loots an Orb of Twisting Slash and, in the window before TryLearnSkillsAsync next runs on the equip cadence, ShouldGoShopping fires on slot pressure and the sell step runs at the merchant — the orb is sold, and an unsold one is discarded instead under slot pressure. The skill is silently lost and the bot has to find another orb. The window is short (EquipCheckInterval is 8s) but the structural protection is simply absent.

The gear path protects itself with exactly this guard ("selling a piece it picked up as an upgrade one tick before it puts it on is pure loss" — same comment applies verbatim here). One line in GetSellableJunk:

if (BotSkillHandler.WantsSkillItem(player, item))
{
    continue; // a skill orb/scroll waiting for the next learn pass.
}

2. GrantingItemsCache leaks the whole GameConfiguration graph, and buys nothing

BotProgression.cs:431 — a static ConcurrentDictionary<GameConfiguration, …>. GameConfiguration does not override Equals/GetHashCode, so this is reference-keyed and every instance ever passed in is pinned for the process lifetime. The XML doc acknowledges "a reload builds a new instance" — which means each reload permanently leaks the previous configuration's entire object graph (items, monsters, maps, skills), not just the small dictionary.

The stated justification doesn't hold either: IsGrantingItemObtainable has exactly one production call site, BotGenerator.LearnClassSkills (BotGenerator.cs:553), which runs once per generated character — not "the per-tick skill selection of hundreds of bots". The neighbouring GetItemGrantedSkillNumbers(config) is already recomputed per character with no cache at all.

I'd drop the static cache and hand the lookup in, built once per generation run alongside itemGrantedSkillNumbers — same shape as the existing code, no lifetime question. If a cache really is wanted later, ConditionalWeakTable would at least not pin the key. (Note the cache is also stale against in-place edits: the admin panel mutates the tracked GameConfiguration rather than replacing it, so a newly added item definition would never show up.)

3. IsGrantingItemObtainable covers wearable granting items, where both the gate and the premise are wrong

GetGrantingItems uses the same filter as GetItemGrantedSkillNumbers — any ItemDefinition with a Skill. That is not just orbs and scrolls: summoner books and skill weapons are in there too (see the explicit "Summoner Books … are in group 5 (staffs) and are the only items in the group which can have skill" branch in ItemExtensions.GetRequirement). Two consequences:

• Wrong requirement math. IsObtainableGrantingItem compares requirement.MinimumValue directly. That matches the real gate only for non-wearables, where GetRequirement returns (totalAttribute, requirement.MinimumValue) unchanged. For a wearable granting item the real value goes through CalculateRequirement (multiplier 3, or 4 for energy), CalculateBookEnergyRequirement, plus item-level and option scaling — so the check systematically under-estimates it. The comment "the same gate a human faces at the consume handler" is only true for groups 12/15.
• Wearable-granted skills shouldn't be backfilled at all. They aren't learned by humans: SkillList adds them from EquippedItems.Where(item => item.HasSkill) and drops them again on unequip (SkillList.cs:67-70, AddItemSkillAsync/RemoveItemSkillAsync). Writing them into LearnedSkills at generation gives the bot a permanent skill that survives replacing the weapon — the same shape of problem as Bots use skills they should not have access to (mount-bound skills, premature orb/scroll skills) #943.

BotSkillHandler.WantsSkillItem already gets this right by restricting to OrbGroup/ScrollGroup (BotSkillHandler.cs:47). The generation path should use the same restriction, and then the raw-MinimumValue comparison becomes correct by construction.

4. Existing populations keep over-granted scroll skills with no runtime guard

The mount gate has a belt-and-braces runtime skip in CombatHandler, so previously-learned Impale goes quiet immediately. The scroll case has no equivalent: a level-20 bot that already learned Evil Spirit from the old level-up code keeps casting it. The PR body's "use Reset bots once" is a fair answer, but the two cases are handled asymmetrically and it's worth saying so explicitly in the docs, since the low-level-Evil-Spirit symptom from #943 is the one people will look for after upgrading.

5. Smaller things

• BotSkillProgressionPlugIn.cs:168: itemGrantedSkillNumbers.Contains(skill.Number) || !IsBotLearnableSkill(skill, itemGrantedSkillNumbers) — the first clause subsumes the item-granted branch inside IsBotLearnableSkill, so the set is now doing two different jobs at one call site and the parameter is dead weight here. A named predicate (or dropping the parameter and doing the item-granted filtering at each call site) would make the two policies — "learnable on level-up" vs. "learnable at generation, if the item is obtainable" — readable from the call.
• BotProgression.cs:312-316: the "Worth learning if it adds damage of its own, hits more than once…" comment stayed behind in IsBotLearnableSkill, above return IsBotLootableSkill(skill);, while the code it describes moved into IsBotLootableSkill (which now has no comment). The MasterDefinition check is also duplicated across both methods.
• ItemPickupHandler.cs:146: every other branch in ShouldPickUp is behind a _config.PickXxx toggle; this one is unconditional for bots. Since level-up no longer grants item skills, a server with pickup toggles off now has bots that can never learn an orb/scroll skill at all. Intended? If so a line in bots.md would help; otherwise reuse an existing flag.
• BotGenerator.cs now ends with two blank lines before the class's closing brace (SA1508), and GrantingItemsCache sits between two methods in BotProgression (SA1201/SA1204: a field must not follow a method) — both in StyleCop-covered projects.
• BotProgressionTests.cs: NoItemGrantedSkills is followed directly by a doc comment with no blank line, and CreateEvilSpiritSetup + the nested TestGameConfiguration/TestItemDefinition types are declared mid-file with four more [Test] methods and another nested class after them. tests/ has no Directory.Build.props, so StyleCop won't complain, but it reads against the grain of the rest of the suite.
• Coverage: BotSkillHandler is the centre of this change and has no tests — the group 12/15 filter, the already-known short-circuit, the double class check, CompliesRequirements, and the ItemPickupHandler wiring are all untested. BotProgressionTests only exercises the pure predicates. Likewise BotStarterGearEquipper: the extraction looks faithful, but nothing asserts that a fresh profile ends up with a weapon and no armor while a veteran gets both — which is the behaviour change users will actually notice. Given your own note that the full suite was last green before the gear refactor, that re-run is worth doing.

Nothing here looks hard to fix, and points 1-3 are the ones I'd not merge without.

---

Generated by Claude Code

[sven-n]

Re-review (c547606)

Went through 57ea7a0 + c547606 and re-checked each earlier point against the code rather than the commit message. All five are genuinely addressed:

1. Sell race — guard added in GetSellableJunk at the right spot (after the potion/jewel continues, before the IsUpgradeFor fallthrough), so orbs actually reach it. It also covers the unsold-then-discarded path, as the comment says.
2. Static cache — gone. GetGrantingItems(config) now returns the whole dictionary and BotGenerator builds it once per generation run. No key pinned, nothing to go stale.
3. Wearable granting items — IsConsumableSkillGrant restricts the backfill to groups 12/15, and MayBackfillSkill returns false when a skill is only equipment/pet-granted. That's the right call given SkillList supplies those on equip. The comment in IsObtainableGrantingItem explaining why the raw MinimumValue comparison is sound because wearables never reach it is exactly the invariant that was missing.
4. Docs asymmetry — covered in bots.md, including the pickup toggle.
5. Naming/ordering/comments — IsBotLearnableSkill collapsed into IsBotLootableSkill, the stranded comment moved to the code it describes, duplicate MasterDefinition check gone, private helpers moved below the public surface, trailing blank lines removed, and the duplicated NoItemGrantedSkills field dropped from both test files.

Gating the pickup on PickUpgradeItems is a good answer to the toggle question — it puts skill loot under the same switch as gear loot, which is the consistent reading.

New tests look right where I could trace them: MayBackfillSkill_EquipmentGrantedOnly_ReturnsFalse and MayBackfillSkill_MixedGrantsWithObtainableOrb_ReturnsTrue pin the two new rules, CreateScrollGrant exercises the TotalEnergyRequirementValue → TotalEnergy normalisation, and the Mock<Character>/Mock<GameConfiguration> pattern in BotStarterGearEquipperTest matches PlayerTestHelper. I checked the one thing that worried me — Character.Name is non-virtual, so SetupAllProperties leaves it at its string.Empty initialiser rather than null, and IsPreferredWeaponGroup/GetStatWeights handle "" fine (class 4 → Dark Knight → str 62 → melee, so the fixture's group-0 sword takes the preferred branch, not the fallback).

One new thing, introduced by the fix itself.

NeedsShopping now reads the skill list from the navigator timer

WantsSkillItem calls player.SkillList.ContainsSkill(...), which is a lookup on SkillList._availableSkills — a plain IDictionary<ushort, SkillEntry>, not a concurrent one (SkillList.cs:35, :145).

GetSellableJunk is reached from two places, and they are not on the same clock:

• TryTradeAsync → SellJunkAsync → GetSellableJunk runs inside the MU Helper tick (as TradeAndUpgradeAsync's own doc comment states) — safe, same tick as DrainPendingBotActionsAsync.
• NeedsShopping → GetSellableJunk is called directly from BotNavigator.EvaluateAsync (BotNavigator.cs:824), i.e. on the navigator's Timer at EvaluationInterval (1s) — a different timer from OfflinePlayerMuHelper's 500ms PeriodicTimer.

Meanwhile the writer, TryLearnSkillsAsync → AddLearnedSkillAsync → _availableSkills.Add(...) (SkillList.cs:206), runs on the MU Helper tick. So a ContainsKey on the navigator tick can land during an Add on the helper tick — a resize can throw, or the read can come back wrong.

This is the hazard the codebase is careful about elsewhere; TickAsync's own comment ("Actions queued from outside the tick … run here, serialized with the combat handler - so nothing mutates the skill list while combat is enumerating it") is accurate for combat and for ItemPickupHandler, since both run inside that tick. NeedsShopping is the one path that doesn't. Before this PR nothing on the navigator path touched SkillList — IsUpgradeFor, HasSurplus and GetLowPotionKinds don't — so the read is new here.

Severity is low: SafeEvaluateAsync catches it, so the worst case is a logged error plus one skipped tick and an OnAiTickFailed(). But it's cheap to avoid — e.g. have the shopping decision skip the skill-list part and let the trade-time call (already on the safe tick) do the precise check, or maintain a small set from the learn pass that the navigator can read without touching the dictionary. Worth a look even if you conclude the read is benign in practice.

Smaller

• BotSkillHandlerTest.WantsSkillItem_RequirementsUnmet_ReturnsFalse puts Stats.TotalEnergy on the orb. Real orbs and scrolls use Stats.TotalEnergyRequirementValue, which is what RequirementAttributeMapping in GetRequirement keys on; with TotalEnergy the lookup misses and falls to the return (requirement.Attribute, requirement.MinimumValue) tail. The test passes, but it skips the mapping path production takes — and the one MayBackfillSkill deliberately mirrors. CreateScrollGrant already uses the realistic attribute, so it's just this fixture that's out of step.
• MayBackfillSkill_EquipmentGrantedOnly_ReturnsFalse uses Fire Breath (49), which is also in MountRequiredSkillNumbers. MayBackfillSkill has no mount check, so the test does isolate the equipment-grant rule — but a non-mount skill would make that obvious to the next reader instead of leaving them to verify it.
• EquipWeapon_EquipsClassWeaponAtStarterLevel: the fixture holds exactly one item in the weapon groups, so MinBy(DropLevel) and the build preference can't be told apart. Adding a staff at a lower DropLevel would prove the preference wins over availability, which is the interesting half of that method.
• Nothing asserts the end-to-end fresh-vs-veteran gear difference — BotStartupProfileTests covers the flag and BotStarterGearEquipperTest covers EquipArmorSet() in isolation, but not that CreateCharacter skips it for a fresh profile. Harder to reach through BotGenerator's persistence context, so fine to leave; just noting the seam.
• Now that GetGrantingItems exists, GetItemGrantedSkillNumbers is a second near-identical scan of config.Items, and it runs on the level-up path (per level-up, per bot) rather than the rare generation path. Pre-existing, not introduced here — but the keys of the new dictionary are the same set if you ever want to collapse them.
• IsBotLootableSkill is now the gate for looting, generation and level-up, and its doc opens with "whether the skill is one a bot may own at all". The name is the last thing still saying "loot"; something like MayBotOwnSkill would match what it became.

Still no .NET SDK here, so this remains a static review — I did not build or run the suites. Points 1-3 from my first pass are resolved as far as reading can establish; the skill-list read above is the only thing I'd want a decision on before merge.

---

Generated by Claude Code

[sven-n]

Merged as 3b88313.

a1acb3f5 resolves the skill-list race properly rather than papering over it: the guard moved out of GetSellableJunk into SellJunkAsync, so the only remaining GetSellableJunk callers are the two NeedsShopping count checks, which no longer touch SkillList. I checked that the discard path can't reintroduce it — ClearUnsoldAsync only ever sees the unsold list derived from the already-filtered junk, so a wanted orb can't be destroyed as unsellable either. MayBotOwnSkill is the right name for what that gate became, and the two test-fixture fixes do now prove what they claim (the added staff at a lower drop level makes EquipWeapon demonstrate that build preference beats availability, which the single-weapon fixture couldn't).

Merged on green CI at a1acb3f5: the Azure pipeline builds and runs dotnet test over tests/*Tests/*.csproj, which covers the new bot tests, so the full-suite re-run you asked for in the description is satisfied; Codacy and the docs site are clean too.

One cosmetic leftover, not worth holding the merge for. MayBackfillSkill_ConsumableGrantWithoutSkillRequirements_ReturnsFalse includes [TestCase((short)62, "Earthshake")] and builds a group-12 orb for it, but Earthshake has no orb in the configuration — Pets.cs:66 grants it from the Dark Horse, and SkillsInitializer.cs:615 ties its damage to Stats.HorseLevel. So that case asserts a scenario that can't occur, and it contradicts its own sibling MayBackfillSkill_EquipmentGrantedOnly_ReturnsFalse, which models Earthshake correctly as Dark-Horse-granted. Phoenix Shot (270) is in the same boat — the ItemOrWeaponBoundSkillNumbers comment says its granting item isn't modelled at all. The rule under test is right and both tests pass; it's only the fixture that will mislead the next reader. A skill that really is orb-granted with no requirements of its own (Twisting Slash 41, Fire Slash 55 and Ice Arrow 51 all check out against Orbs.cs) would carry the same case without the contradiction.

Worth confirming separately that Earthshake is genuinely safe in production, since it is mount-bound but absent from MountRequiredSkillNumbers: it can't be backfilled (MayBackfillSkill rejects a pet-only grant), can't be looted (group 13 fails the orb/scroll filter), and can't come from level-up (blanket item-granted exclusion) — it only ever appears via AddItemSkillAsync when a Dark Horse is actually equipped, and disappears on unequip. That's legitimate use, so the hardcoded list isn't missing it.

---

Generated by Claude Code