feat(advisor): native deterministic build-advisor tab (Epic #85)

[jay9297]

Epic #85 — native deterministic build-advisor tab

Implements the full Advisor epic (#85, sub-issues #77–#84): a native, fully-offline
build-advisor tab. It reads the live, already-calculated build and surfaces rule-based
improvement findings. No LLM, no network — pure deterministic Lua over PoB's own
engine output, so every number it cites matches what PoB shows.

This branch bundles the 8 sub-issues, each developed on its own branch and merged into the
epic with a clean first-parent history.

What's included

• src/Modules/Advisor.lua — pure logic (Advisor.analyze(build) -> findings), unit-testable headlessly. 14 checks across Survivability, Skills, Attributes, Spirit, and Tree.
• src/Classes/AdvisorTab.lua — UI tab: findings grouped by category, severity filters + "High only", collapsible headers with counts, click-to-jump, live auto-refresh on recalc.
• src/Modules/Build.lua — 3 small additive hunks: tab instantiation, mode button + high-severity badge, draw dispatch.
• Tests — spec/Modules/Advisor_spec.lua (20 unit tests, every check) + spec/System/TestAdvisorSmoke_spec.lua (2 headless smoke tests that run every check unguarded on a real engine-calculated build).
• Docs — ADVISOR.md (check catalogue + how to add a check) and ADVISOR-DEV.md (headless/test commands, upstream rebase workflow, touched-files list).

Check catalogue (id → severity)

• Survivability: res.<elem>.uncapped (high/med), res.<elem>.surplus (info), ehp.low (med), recovery.none (med), mitigation.none (med), maxhit.weakest (info)
• Skills: support.inapplicable.<name> (high), support.empty.<skill> (info), support.duplicate.<id> (med)
• Attributes: attr.unmet.<Attr>.<gem> (high)
• Spirit: spirit.over (high), spirit.unused (info)
• Tree: tree.adjacent.<id> (info), tree.deadend.<id> (med), tree.floating (med)

The support-applicability check uses the engine's own calcLib.doesTypeExpressionMatch
against the active skill's tags, so it never relies on a re-derived tag table.

Upstream surface

Only src/Modules/Build.lua is modified (3 additive hunks); everything else is new,
self-contained files. This keeps upstream rebases cheap.

Test plan

• Unit suite green locally: 503 successes / 0 failures / 0 errors (--exclude-tags builds,data)
• Advisor unit + smoke specs: 22 successes / 0 failures
• Every check syntax-checked (LuaJIT) and exercised on a real build (smoke)
• No src/Data/, ModParser, or ModCache changes → no cache regen needed
• CI builds and data matrices (verified by GitHub Actions on this PR)

🤖 Generated with Claude Code

[github-actions]

🔄 Cycle 1, attempt 1/3
src/Modules/Advisor.lua:274 — calcLib.doesTypeExpressionMatch is called as a bare global without any require or nil guard; if calcLib is not in the global environment when this module loads, the support.inapplicable check pcall-catches silently and produces no finding, making the check a silent no-op. The module's own comment says "pure logic, no UI references — unit-testable headlessly" but this is an implicit dependency on the full calc environment.

src/Modules/Advisor.lua:323–324 — gem[a.req] accesses gem.reqStr, gem.reqDex, gem.reqInt as top-level fields on the gem object; in real PoB builds, gem requirements are stored on gem.gemData.grantedEffect (or level-list entries), not on the outer gem table. The unit test works only because it manually sets gem.reqStr = 200 directly, masking the mismatch; on a real build the check would never fire. The smoke test won't catch this because no crash occurs — it just silently produces no findings.

src/Modules/Advisor.lua:300–305 — when the same support gameId appears three or more times in a group, seen[gid] stays true and a support.duplicate.<gameId> finding is emitted for each additional copy beyond the first, producing multiple findings with the same id and duplicate rows in the UI.

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: REQUEST_CHANGES. See review comment above.

[github-actions]

Verdict: REQUEST_CHANGES. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
addSkillTypes pre-population diverges from engine (Advisor.lua:207-215): The check adds addSkillTypes from every support gem into types before evaluating any support's applicability, whereas CalcActiveSkill.lua:183-208 uses a multi-pass loop that only adds addSkillTypes from already-accepted supports; the Advisor over-enriches types and will silently miss inapplicable support findings when a rejected support's addSkillTypes is what made another support appear to match.

Dead-end check false positive for ClassStart-adjacent nodes (Advisor.lua:370): PAYOFF_TYPES contains Notable, Keystone, and Socket but not ClassStart or AscendClassStart, so a Normal leaf node whose sole allocated neighbour is a class-start node (allocDegree=1, neighbour.type="ClassStart") is incorrectly flagged as a dead end; add both start types to PAYOFF_TYPES.

calcLib-nil guard defaults to false positive (Advisor.lua:221-222): When calcLib is nil and requireSkillTypes is non-empty, (not req or not req[1] or (calcLib and ...)) evaluates to nil (falsy), so applies is falsy and a finding fires for every typed support—the safe default when the evaluator is unavailable should be "applies" (no finding); guard the entire check with if not calcLib then return end or flip the nil-branch to true.

excludeSkillTypes branch has no unit test (Advisor.lua:222): the exc and exc[1] and calcLib.doesTypeExpressionMatch(exc, types) branch is exercised by real gem data (e.g., act_str.lua:526) but spec/Modules/Advisor_spec.lua has no case where a support's excludeSkillTypes matches the active skill's types, leaving the exclusion logic untested.

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: REQUEST_CHANGES. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
Advisor.lua:221-222 — calcLib nil path inverts to false positive
When calcLib is nil, (not req or not req[1] or (calcLib and ...)) short-circuits to false for any support with a non-empty requireSkillTypes, making applies=false and flagging every such support as inapplicable. The intent is to skip the check when the engine isn't available; the fix is not calcLib or calcLib.doesTypeExpressionMatch(...).

Advisor.lua:195-240 — support applicability check diverges from engine
The check reimplements calcLib.canGrantedEffectSupportActiveSkill (CalcTools.lua:85) but omits the grantedEffect.supportGemsOnly and grantedEffect.cannotBeSupported guards that the engine uses. Both flags exist in live game data (grep confirms 6 occurrences across sup_str.lua and other.lua). This produces false negatives for supports that the engine rejects via those flags, and means a skill with cannotBeSupported=true will have its supports evaluated anyway. The Advisor should delegate to canGrantedEffectSupportActiveSkill (or at minimum mirror all three early-return conditions).

Advisor.lua:370 — PAYOFF_TYPES missing "Mastery"
PoE2 has Mastery nodes (PassiveTree.lua:399, PassiveSpec.lua:353). A Normal leaf node that leads exclusively to an allocated Mastery is flagged tree.deadend because "Mastery" is absent from PAYOFF_TYPES. No test covers this. Add Mastery = true to the table.

Advisor.lua:381 / tree check overlap — missing test
A Normal leaf node adjacent to an unallocated Notable gets both tree.deadend (its one allocated neighbor is the start, not a payoff) and tree.adjacent for the same Notable. No spec exercises this case; without it, the overlap is invisible. A test with: Start → A(Normal) → B(Notable, not allocated) with only Start and A allocated should document the expected behaviour.

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: REQUEST_CHANGES. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
Advisor.lua — addSkillTypes accumulation ignores gem.enabled

The loop that builds the effective skill-type set from support gems' addSkillTypes (the first for _, gem in ipairs(group.gemList) loop inside the inapplicable-support check, ~line 246–252) does not guard gem.enabled ~= false. A disabled support gem can silently widen the active skill's type set and suppress a valid inapplicable finding on an enabled support in the same group.

Advisor.lua — supportGemsOnly guard is dead code (~line 263)

and not (ge.supportGemsOnly and not activeGem.gemData)

groupActiveEffect only returns a gem when gem.gemData is truthy, so not activeGem.gemData is always false. The supportGemsOnly exemption never fires; any support gem with that flag will be checked for skill-type compatibility with the active skill and may produce false-positive inapplicable findings.

spec/Modules/Advisor_spec.lua — no coverage for addSkillTypes broadening

There is no test that places a support gem with addSkillTypes = { SkillType.SomeType } alongside an active skill and verifies the broadened type set is respected (i.e., a support that is applicable only after the type expansion is not flagged).

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: REQUEST_CHANGES. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
Advisor.lua:335 — not calcLib short-circuits applies = true; when calcLib is absent the entire inapplicable-support check produces zero findings with no log output, silently undermining the feature without any pcall catch or ConPrintf warning.

Advisor.lua:330 — for _, st in pairs(ge.addSkillTypes) do types[st] = true end iterates values; if real game data stores addSkillTypes as a key-indexed hash like grantedEffect.skillTypes ({ [SkillType.X] = true }), st becomes true and broadening silently does nothing — verify the format in src/Data/Skills/*.lua before merge.

spec/Modules/Advisor_spec.lua — support.inapplicable and support.empty checks have no test for group.enabled = false; only attr.unmet exercises that guard ("does not flag unmet attribute requirements for gems in a disabled group").

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: COMMENT. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
The addSkillTypes format and evasion key both checked out fine against real data. I confirmed:

• addSkillTypes in real gem files is an array ({ SkillType.X }), so ipairs is correct.
• output.EvadeChance is always initialized in CalcDefence.lua line 1440, so the fallback or out.MeleeEvadeChance is harmless dead code but not wrong.

After full review of all logic, nil-safety, tests, and integration with Build.lua:

• Resistance / EHP / recovery / mitigation / spirit / tree checks: correct logic, well nil-guarded.
• Support applicability: groupActiveEffect, addSkillTypes expansion, requireSkillTypes short-circuit, duplicate dedup sentinel pattern — all correct.
• Attribute check uses out.Str/out.Dex/out.Int: matches standard PoB output key conventions.
• AdvisorTab auto-refresh on reference change, scroll clamping, click-to-jump — all correct.
• Build.lua integration is additive and clean.
• Test coverage addresses all documented check IDs including edge cases (disabled groups, minionSkillTypes, CI, three-copy dup, floating tree nodes).

The fix field is populated in every finding but never rendered in AdvisorTab:Draw (only title and detail are shown). This is a UX gap, not a logic bug, and appears intentional for the MVP.

✅ Agent succeeded (cycle 1, attempt 1)