Add skill data for passive tree Grants Skill nodes

[jay9297]

Summary

• Add Gems.lua entries for 8 Djinn command sub-skills (Navira's Fracturing/Well/Oasis, Ruzhan's Trap/Reckoning/Fury, Kelari's Deception/Judgment) so they resolve in gemIdLookup and produce correct ExtraSkill mods when their passive tree nodes are allocated
• Add ModParser patterns for "Grants Sands of Time" and "Grants Thaumaturgical Dynamism" display-only buff text so they no longer appear as unknown/unrecognized stats
• Update ModCache entries to reflect the resolved skill IDs

Closes #39

Details

The existing pipeline for Grants Skill: X passive tree nodes was already complete:

1. ModParser matches the pattern and calls grantedExtraSkill()
2. grantedExtraSkill() looks up the skill name in gemIdLookup
3. If found, emits mod("ExtraSkill", "LIST", { skillId = ..., level = 1 })
4. CalcSetup picks up ExtraSkill mods and creates socket groups

The issue was purely data-layer: 8 Djinn command skills existed in Skills/minion.lua (loaded into data.skills) but had no corresponding entries in Gems.lua, so they never appeared in gemIdLookup. Adding the gem entries completes the mapping.

For "Grants Sands of Time" and "Grants Thaumaturgical Dynamism", these are display-only stat description text (not actual skill grants). They now match as known no-op patterns instead of being flagged as unknown.

Test plan

• Allocate a Djinn ascendancy node that grants a command skill (e.g. Navira's Fracturing) and verify the skill appears in the Skills tab
• Allocate a Chronomancer node with "Grants Sands of Time" and verify no unknown stat warning
• Allocate a Gemling Legionnaire node with "Grants Thaumaturgical Dynamism" and verify no unknown stat warning
• Verify existing Grants Skill nodes (concoctions, Apocalypse, etc.) still work correctly

[jay9297]

Code Review — Complete ✓

Reviewed and rebased onto current dev (2 new upstream commits for trade query/OAuth work — no conflicts with this PR's files).

Findings

All code is correct. No bugs found.

• Djinn skill IDs (ChilledGroundBurstWaterDjinn, ESRechargeForceRestartWaterDjinn, ChilledGroundOasisConvertWaterDjinn, FireRuneFireDjinn, MeteorFireDjinn, MassFusilladeFireDjinn, ExplosiveTeleportSandDjinn, HandSlamSandDjinn) — all verified present in src/Data/Skills/minion.lua. ✓
• gameId field uses Metadata/Items/Gem/... (singular) — confirmed correct for ascendancy gems; matches all other existing ascendancy entries. ✓
• {{},nil} in ModCache for Grants Sands of Time and Grants Thaumaturgical Dynamism — this is the established pattern for "recognized, no-op mod" (dozens of existing entries use it). Correctly returns empty mod list without triggering the unsupported-stat writer. ✓
• ModParser patterns for grants sands of time / grants thaumaturgical dynamism — correctly placed, no regex conflict with ["grants skill: (%D+)"] (those patterns don't start with "grants skill:"). Needed for headless mode where ModCache preload is skipped. ✓
• grantedExtraSkill lookup — gemIdLookup is keyed by gemData.name:lower(), mod lines are lowercased before matching; lookup is consistent end-to-end. ✓

Status

Ready for testing. No code changes were needed — just rebased. Test plan in PR description covers the right cases.

[github-actions]

Now I have enough context to write a thorough review.

---

Code Review

1. Calculation Correctness

No changes touch src/Modules/Calcs/ or CalcSections.lua. The ModCache entries produce ExtraSkill mods with type="LIST", which routes into the existing skill-granting pipeline — no new math paths introduced. However, the new skillId strings (ChilledGroundBurstWaterDjinn, ESRechargeForceRestartWaterDjinn, etc.) must resolve against entries in the grantedEffects game data. Those backing entries are not visible in this diff. If they don't exist, the ExtraSkill mod will silently fail to grant the skill at runtime rather than erroring. Not a blocker, but worth verifying the grantedEffect data is present.

2. Modifier Parsing — ModParser.lua:3491–3492

["grants sands of time"] = { },
["grants thaumaturgical dynamism"] = { },

These are inserted into specialModList as empty tables. The dispatch path at line 6343–6346 is:

if type(specialMod) == "function" then
    return specialMod(tonumber(cap[1]), unpack(cap))
else
    return copyTable(specialMod)  -- returns {} for these entries
end

copyTable({}) returns an empty table, which correctly represents "recognised, no mods produced." The patterns get anchored to ^...$ at line 6093 before scan() runs, so there is no conflict with ["grants skill: (%D+)"]. The mechanics are sound.

FAIL — Missing tests. CLAUDE.md rule #2 is unambiguous:

Any change requires adding test cases for the new modifier strings in spec/System/TestModParser.lua.

spec/System/TestModParser.lua does not exist, and no existing spec file covers these new patterns. There are no new tests anywhere in spec/ for:

• "Grants Sands of Time" → empty mod list
• "Grants Thaumaturgical Dynamism" → empty mod list
• Eight new "Grants Skill: ..." strings (these exercise the pre-existing grantedExtraSkill path, but only if gemIdLookup resolves them)

3. Nil Safety

No new unguarded table accesses. Gems.lua uses literal numeric values (never indexing into tables). copyTable({}) on an empty table is safe. ModCache.lua is pure data. Pass.

4. Data Integrity

Gems.lua key vs gameId naming: All new keys use "Metadata/Items/Gems/..." (plural) while gameId uses "Metadata/Items/Gem/..." (singular). This matches every surrounding existing entry (SkillGemAscendancySummonWaterDjinn at line 9056/9059, etc.). Pass.

grantedEffectId ↔ skillId consistency: All eight are aligned:

Gem	grantedEffectId (Gems.lua)	skillId (ModCache.lua)
Navira's Fracturing	ChilledGroundBurstWaterDjinn	ChilledGroundBurstWaterDjinn ✓
Navira's Well	ESRechargeForceRestartWaterDjinn	ESRechargeForceRestartWaterDjinn ✓
Navira's Oasis	ChilledGroundOasisConvertWaterDjinn	ChilledGroundOasisConvertWaterDjinn ✓
Ruzhan's Trap	FireRuneFireDjinn	FireRuneFireDjinn ✓
Ruzhan's Reckoning	MeteorFireDjinn	MeteorFireDjinn ✓
Ruzhan's Fury	MassFusilladeFireDjinn	MassFusilladeFireDjinn ✓
Kelari's Deception	ExplosiveTeleportSandDjinn	ExplosiveTeleportSandDjinn ✓
Kelari's Judgment	HandSlamSandDjinn	HandSlamSandDjinn ✓

BUG — Missing physical tag on Kelari sub-skills. All Navira sub-skills have cold = true; all Ruzhan sub-skills have fire = true. The parent Sand Djinn (SkillGemAscendancySummonSandDjinn, line 9113) has physical = true. But Kelari's Deception and Kelari's Judgment both have no damage-type tag:

-- Kelari's Deception (Gems.lua, new entry)
tags = {
    grants_active_skill = true,
    spell = true,
    area = true,           -- no physical = true
},
tagString = "AoE",         -- no Physical

-- Kelari's Judgment (Gems.lua, new entry)  
tags = {
    grants_active_skill = true,
    spell = true,
    area = true,           -- no physical = true
},
tagString = "AoE",         -- no Physical

HandSlamSandDjinn is a hand slam and ExplosiveTeleportSandDjinn produces a sand explosion — both expected to deal physical damage. The omission breaks tag-based damage scaling (e.g. "increased Physical Damage" modifiers will not apply to these skills via the tag filter). Needs verification against game data, but the asymmetry with Navira/Ruzhan sub-skills is a clear red flag.

ModCache.lua {{},nil} format for "Grants Sands of Time" / "Grants Thaumaturgical Dynamism": The SaveModCache writer at Main.lua:293–313 serialises parseMod's return as {modList, extra}. For these patterns, parseMod returns copyTable({}) with no second return, so cache[line] = {{}, nil} is exactly what the serialiser would write. Consistent and correct.

5. Performance

No allocations inside any hot loop. All changes are to startup-time data tables and the parseModCache lookup, which is populated once. Pass.

6. Test Coverage

• spec/System/TestModParser.lua — does not exist; not created in this PR.
• No coverage in TestItemMods_spec.lua or TestPassiveSpec_spec.lua for:
  • The two new "silent" patterns
  • The eight new ExtraSkill ModCache entries
• CLAUDE.md rule Passive Nodes: Support Elemental Archon / Archon Buffs mods #2 is a hard requirement; this PR does not satisfy it.

7. Style

All new Gems.lua entries are structurally identical to neighbours (same field order, same indent). ModCache.lua entries maintain alphabetical order. ModParser additions sit correctly under the -- Extra skill/support comment block. Pass.

---

Summary of Issues

Severity	Location	Issue
Required	spec/	No test cases for new ModParser patterns or ExtraSkill mappings (CLAUDE.md rule #2)
Bug	Gems.lua new entries	Kelari's Deception and Kelari's Judgment missing physical = true / "Physical" in tagString
Verify	All 8 new gems	Confirm backing grantedEffect definitions exist in game data files

---

REQUEST_CHANGES — The missing physical tag on Kelari sub-skills will cause those skills to miss physical damage scaling modifiers at runtime, and the complete absence of modifier-parser tests violates an explicit project rule in CLAUDE.md.

[jay9297]

Now I have enough context to write a thorough review.

Code Review

1. Calculation Correctness

No changes touch src/Modules/Calcs/ or CalcSections.lua. The ModCache entries produce ExtraSkill mods with type="LIST", which routes into the existing skill-granting pipeline — no new math paths introduced. However, the new skillId strings (ChilledGroundBurstWaterDjinn, ESRechargeForceRestartWaterDjinn, etc.) must resolve against entries in the grantedEffects game data. Those backing entries are not visible in this diff. If they don't exist, the ExtraSkill mod will silently fail to grant the skill at runtime rather than erroring. Not a blocker, but worth verifying the grantedEffect data is present.

2. Modifier Parsing — ModParser.lua:3491–3492

["grants sands of time"] = { },
["grants thaumaturgical dynamism"] = { },

These are inserted into specialModList as empty tables. The dispatch path at line 6343–6346 is:

if type(specialMod) == "function" then
    return specialMod(tonumber(cap[1]), unpack(cap))
else
    return copyTable(specialMod)  -- returns {} for these entries
end

copyTable({}) returns an empty table, which correctly represents "recognised, no mods produced." The patterns get anchored to ^...$ at line 6093 before scan() runs, so there is no conflict with ["grants skill: (%D+)"]. The mechanics are sound.

FAIL — Missing tests. CLAUDE.md rule #2 is unambiguous:

Any change requires adding test cases for the new modifier strings in spec/System/TestModParser.lua.

spec/System/TestModParser.lua does not exist, and no existing spec file covers these new patterns. There are no new tests anywhere in spec/ for:

• "Grants Sands of Time" → empty mod list
• "Grants Thaumaturgical Dynamism" → empty mod list
• Eight new "Grants Skill: ..." strings (these exercise the pre-existing grantedExtraSkill path, but only if gemIdLookup resolves them)

3. Nil Safety

No new unguarded table accesses. Gems.lua uses literal numeric values (never indexing into tables). copyTable({}) on an empty table is safe. ModCache.lua is pure data. Pass.

4. Data Integrity

Gems.lua key vs gameId naming: All new keys use "Metadata/Items/Gems/..." (plural) while gameId uses "Metadata/Items/Gem/..." (singular). This matches every surrounding existing entry (SkillGemAscendancySummonWaterDjinn at line 9056/9059, etc.). Pass.

grantedEffectId ↔ skillId consistency: All eight are aligned:

Gem grantedEffectId (Gems.lua) skillId (ModCache.lua)
Navira's Fracturing ChilledGroundBurstWaterDjinn ChilledGroundBurstWaterDjinn ✓
Navira's Well ESRechargeForceRestartWaterDjinn ESRechargeForceRestartWaterDjinn ✓
Navira's Oasis ChilledGroundOasisConvertWaterDjinn ChilledGroundOasisConvertWaterDjinn ✓
Ruzhan's Trap FireRuneFireDjinn FireRuneFireDjinn ✓
Ruzhan's Reckoning MeteorFireDjinn MeteorFireDjinn ✓
Ruzhan's Fury MassFusilladeFireDjinn MassFusilladeFireDjinn ✓
Kelari's Deception ExplosiveTeleportSandDjinn ExplosiveTeleportSandDjinn ✓
Kelari's Judgment HandSlamSandDjinn HandSlamSandDjinn ✓
BUG — Missing physical tag on Kelari sub-skills. All Navira sub-skills have cold = true; all Ruzhan sub-skills have fire = true. The parent Sand Djinn (SkillGemAscendancySummonSandDjinn, line 9113) has physical = true. But Kelari's Deception and Kelari's Judgment both have no damage-type tag:

-- Kelari's Deception (Gems.lua, new entry)
tags = {
    grants_active_skill = true,
    spell = true,
    area = true,           -- no physical = true
},
tagString = "AoE",         -- no Physical

-- Kelari's Judgment (Gems.lua, new entry)  
tags = {
    grants_active_skill = true,
    spell = true,
    area = true,           -- no physical = true
},
tagString = "AoE",         -- no Physical

HandSlamSandDjinn is a hand slam and ExplosiveTeleportSandDjinn produces a sand explosion — both expected to deal physical damage. The omission breaks tag-based damage scaling (e.g. "increased Physical Damage" modifiers will not apply to these skills via the tag filter). Needs verification against game data, but the asymmetry with Navira/Ruzhan sub-skills is a clear red flag.

ModCache.lua {{},nil} format for "Grants Sands of Time" / "Grants Thaumaturgical Dynamism": The SaveModCache writer at Main.lua:293–313 serialises parseMod's return as {modList, extra}. For these patterns, parseMod returns copyTable({}) with no second return, so cache[line] = {{}, nil} is exactly what the serialiser would write. Consistent and correct.

5. Performance

No allocations inside any hot loop. All changes are to startup-time data tables and the parseModCache lookup, which is populated once. Pass.

6. Test Coverage

• spec/System/TestModParser.lua — does not exist; not created in this PR.

• No coverage in TestItemMods_spec.lua or TestPassiveSpec_spec.lua for:

  • The two new "silent" patterns
  • The eight new ExtraSkill ModCache entries

• CLAUDE.md rule Passive Nodes: Support Elemental Archon / Archon Buffs mods #2 is a hard requirement; this PR does not satisfy it.

7. Style

All new Gems.lua entries are structurally identical to neighbours (same field order, same indent). ModCache.lua entries maintain alphabetical order. ModParser additions sit correctly under the -- Extra skill/support comment block. Pass.

Summary of Issues

Severity Location Issue
Required spec/ No test cases for new ModParser patterns or ExtraSkill mappings (CLAUDE.md rule #2)
Bug Gems.lua new entries Kelari's Deception and Kelari's Judgment missing physical = true / "Physical" in tagString
Verify All 8 new gems Confirm backing grantedEffect definitions exist in game data files
REQUEST_CHANGES — The missing physical tag on Kelari sub-skills will cause those skills to miss physical damage scaling modifiers at runtime, and the complete absence of modifier-parser tests violates an explicit project rule in CLAUDE.md.

/fix

[github-actions]

Now I have everything I need. Here is the full review.

---

Code Review

1. Calculation Correctness

No changes touch src/Modules/Calcs/ or CalcSections.lua. The new entries are pure data registration, so there are no calculation math concerns.

The skillId values in ModCache.lua are consistent with the grantedEffectId values in Gems.lua across all 8 new skills:

Gem	grantedEffectId / skillId
Navira's Fracturing	ChilledGroundBurstWaterDjinn ✓
Navira's Well	ESRechargeForceRestartWaterDjinn ✓
Navira's Oasis	ChilledGroundOasisConvertWaterDjinn ✓
Ruzhan's Trap	FireRuneFireDjinn ✓
Ruzhan's Reckoning	MeteorFireDjinn ✓
Ruzhan's Fury	MassFusilladeFireDjinn ✓
Kelari's Deception	ExplosiveTeleportSandDjinn ✓
Kelari's Judgment	HandSlamSandDjinn ✓

All eight pairs are internally self-consistent. No issues.

---

2. Modifier Parsing — BLOCKING

CLAUDE.md states explicitly: "Any change [to ModParser.lua] requires adding test cases for the new modifier strings in spec/System/TestModParser.lua."

Two new entries were added at src/Modules/ModParser.lua:3490-3491:

["grants sands of time"] = { },
["grants thaumaturgical dynamism"] = { },

No test cases were added. The spec file has no coverage for "Grants Sands of Time" or "Grants Thaumaturgical Dynamism". This is a hard requirement per project policy — the PR must not merge without them.

The minimum expected test shape (matching the existing pattern in spec/System/TestItemMods_spec.lua) would be an it() case that applies these modifier strings and asserts the result produces no stat changes (i.e., modDB:Sum(...) for any stat returns 0 or base value).

Secondary concern — pattern placement. The two new entries are inserted between two related patterns:

["grants skill: (%D+)"] = function(_, skill) return grantedExtraSkill(skill, 1) end,
["grants sands of time"] = { },          -- inserted here
["grants thaumaturgical dynamism"] = { },
["grants skill: level (%d+) (.+)"] = function(...)

In Lua 5.1, specialModList iteration order is unspecified for hash-table keys, so order does not affect correctness. However, splitting the two "grants skill:" patterns with unrelated entries hurts readability. These should be grouped either before or after the "grants skill:" family.

---

3. Nil Safety

ModCache.lua — {{},nil} pattern change. Two entries changed from {nil, "...string..."} to {{},nil}:

-- before
c["Grants Sands of Time"]={nil,"Grants Sands of Time "}
-- after
c["Grants Sands of Time"]={{},nil}

This is correct. The {nil, string} format signals an unrecognized modifier; {{}, nil} signals "parsed successfully, zero mods generated." The change is consistent with how ModParser now handles these strings (returning {}), and the {{},nil} pattern has well-established precedent throughout ModCache.lua (e.g., line 739, 3507, 4542–4553, 4656). No nil safety issues.

The new ExtraSkill entries in ModCache.lua follow the identical structure of every other ExtraSkill entry in the file and introduce no new table-access patterns. No nil safety issues.

---

4. Data Integrity

Key vs. gameId path discrepancy is intentional. All 8 new entries follow the established convention: the table key uses Metadata/Items/Gems/ (plural) while gameId uses Metadata/Items/Gem/ (singular). This matches every existing ascendancy gem entry (confirmed at lines 9078–9142).

Tag concern — spell = true on physical-sounding skills. Two entries warrant verification:

• SkillGemAscendancyKelarisDeception (ExplosiveTeleportSandDjinn) — tagged spell = true, area = true. No movement tag. If this is a blink/teleport skill, the absence of a movement tag may cause incorrect modifiers to apply (or fail to apply). Verify against game data.
• SkillGemAscendancyKelarisJudgment (HandSlamSandDjinn) — tagged spell = true, area = true. A "hand slam" is typically a physical attack. If this skill deals physical damage, the absence of a physical damage type tag and presence of spell = true may produce incorrect damage calculations. Verify against game data.

These are not blocking if these skills truly deal no player-relevant damage (i.e., they are djinn abilities that PoB does not model in the stat calculation), but they should be explicitly confirmed.

tagString = "" for Navira's Well is consistent with spells that have no categorizable damage element (the skill restarts ES recharge). No issue.

---

5. Performance

All new allocations are in module-load-time data tables (Gems.lua, ModCache.lua). The specialModList additions in ModParser.lua are evaluated at parse time, not inside a Calc:Build* hot loop. No performance concerns.

---

6. Test Coverage

Code path	Covered?
"grants sands of time" in ModParser.lua	No
"grants thaumaturgical dynamism" in ModParser.lua	No
Grants Skill: Kelari's Deception ExtraSkill in ModCache.lua	No
Grants Skill: Navira's Fracturing ExtraSkill in ModCache.lua	No
(…same for all 8 new ExtraSkill entries)	No

The two ModParser.lua additions are the hard blocker per CLAUDE.md. The ModCache.lua and Gems.lua additions are data entries; there is no explicit test mandate for those, but snapshot/build tests (--tags builds) should be run to catch any regression from the new skill IDs resolving (or failing to resolve) at build time.

---

7. Style

• All 8 gem entries in Gems.lua follow the surrounding conventions: field order (name, baseTypeName, gameId, variantId, grantedEffectId, tags, gemType, tagString, reqStr/Dex/Int, Tier, naturalMaxLevel) matches adjacent entries. No issues.
• ModCache.lua entries match the surrounding line format exactly. No issues.
• ModParser.lua placement issue noted above in §2.

---

REQUEST_CHANGES — Missing test cases for both new ModParser.lua patterns ("grants sands of time", "grants thaumaturgical dynamism") are a hard project policy requirement per CLAUDE.md; additionally, the two physical-sounding Kelari skills should have their spell = true and missing element/movement tags verified against game data before merge.