From fc0238460061002f0f94d72cb08b6fdb6a6befaa Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 00:01:45 +0000 Subject: [PATCH 1/3] fix(export): address adversarial review findings in PoE2 build export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL - PassiveSpec: include nodeNotes in CreateUndoState/RestoreUndoState so Ctrl+Z and tree copy no longer silently erase per-node notes - BuildExportPoE2: replace io.popen('ls -d ...') in dirExists with os.rename(path,path) — eliminates the Windows crash risk and the shell injection vector from unescaped path content HIGH - BuildExportPoE2: remove warnedNoStringId module-level global; surface the "no stringId in tree.lua" warning as a third return value from Export/WriteFile so each call site gets fresh state and the UI can display it prominently (ImportTab now shows colorCodes.WARNING status) - BuildExportPoE2: sort `order` before emitting in buildPassives and sort SlotMap keys before iterating in buildItems to produce deterministic JSON output regardless of LuaJIT hash table iteration order MEDIUM - BuildExportPoE2: PresetNextLevels anyHas now checks `levelMin or levelMax` (was levelMax-only), preventing double-seeding of an entry that already has only levelMin set - BuildExportPoE2: autoBracket uses m_max(lo, ...) for hi so hi >= lo is guaranteed even for n > 99 sets - SkillsTab: add local m_floor alias; fix clampLvl fallback to use it instead of the global math.floor; fix level-bracket callbacks to set self.modFlag instead of self.build.modFlag LOW - PassiveTreeView: gate the per-node note popup and tooltip on hoverNode.alloc — notes only make sense on allocated nodes; tooltip shows existing note content when present - BuildExportPoE2: replace factually-incorrect "typo in gameId" comment on gemIdFor with an accurate description Tests: add cases for levelMin-only anyHas, Export warning return value https://claude.ai/code/session_01EvGnnyxkoMawLmWWqCTxxX --- spec/System/TestBuildExportPoE2_spec.lua | 18 +++++++ src/Classes/ImportTab.lua | 9 +++- src/Classes/PassiveSpec.lua | 4 +- src/Classes/PassiveTreeView.lua | 17 +++--- src/Classes/SkillsTab.lua | 7 +-- src/Modules/BuildExportPoE2.lua | 68 +++++++++++++----------- 6 files changed, 79 insertions(+), 44 deletions(-) diff --git a/spec/System/TestBuildExportPoE2_spec.lua b/spec/System/TestBuildExportPoE2_spec.lua index 08ae97fab5..3008715a89 100644 --- a/spec/System/TestBuildExportPoE2_spec.lua +++ b/spec/System/TestBuildExportPoE2_spec.lua @@ -96,6 +96,17 @@ describe("TestBuildExportPoE2", function() assert.are.equals(1, newEntry.levelMin) assert.are.equals(30, newEntry.levelMax) end) + + it("Treats levelMin-only entry as having a level set", function() + local existing = { { id = 1, levelMin = 1 } } + local newEntry = { id = 2 } + BuildExportPoE2.PresetNextLevels(existing, newEntry) + -- anyHas=true but maxLvl=0 (no levelMax), so new entry gets [1,30] + assert.are.equals(1, newEntry.levelMin) + assert.are.equals(30, newEntry.levelMax) + -- Must NOT re-seed the existing entry (it already had levelMin set) + assert.is_nil(existing[1].levelMax) + end) end) describe("NextLoadoutBracket", function() @@ -251,6 +262,13 @@ describe("TestBuildExportPoE2", function() local json2 = BuildExportPoE2.Export(build) assert.are.equals(json1, json2) end) + + it("Returns nil warning for an empty build", function() + local _, err, warning = BuildExportPoE2.Export(build) + assert.is_nil(err) + -- An empty build has no passives so no stringId check fires. + assert.is_nil(warning) + end) end) describe("WriteFile", function() diff --git a/src/Classes/ImportTab.lua b/src/Classes/ImportTab.lua index 7516f9fc46..76085866fc 100644 --- a/src/Classes/ImportTab.lua +++ b/src/Classes/ImportTab.lua @@ -400,9 +400,14 @@ end function ImportTabClass:DoPoE2Export(Exporter, path) local function doWrite() - local ok, err = Exporter.WriteFile(self.build, path) + local ok, err, warning = Exporter.WriteFile(self.build, path) if ok then - self.poe2ExportStatus = colorCodes.POSITIVE .. "Saved to " .. path + local safePath = path:gsub("%^", "^") + if warning then + self.poe2ExportStatus = colorCodes.WARNING .. "Saved (warning): " .. warning + else + self.poe2ExportStatus = colorCodes.POSITIVE .. "Saved to " .. safePath + end else self.poe2ExportStatus = colorCodes.NEGATIVE .. (err or "Export failed") main:OpenMessagePopup("Export Failed", err or "Unknown error") diff --git a/src/Classes/PassiveSpec.lua b/src/Classes/PassiveSpec.lua index 11f5318298..c699107861 100644 --- a/src/Classes/PassiveSpec.lua +++ b/src/Classes/PassiveSpec.lua @@ -2392,7 +2392,8 @@ function PassiveSpecClass:CreateUndoState() weaponSets = weaponSets, hashOverrides = copyTable(self.hashOverrides, true), masteryEffects = selections, - treeVersion = self.treeVersion + treeVersion = self.treeVersion, + nodeNotes = copyTable(self.nodeNotes), } end @@ -2408,6 +2409,7 @@ function PassiveSpecClass:RestoreUndoState(state, treeVersion) end end self:ImportFromNodeList(nil, classId, ascendClassId, state.secondaryAscendClassId, state.hashList, state.weaponSets, state.hashOverrides, state.masteryEffects, treeVersion or state.treeVersion) + self.nodeNotes = state.nodeNotes or {} self:SetWindowTitleWithBuildClass() end diff --git a/src/Classes/PassiveTreeView.lua b/src/Classes/PassiveTreeView.lua index 824d8501d1..cf6c7496ee 100644 --- a/src/Classes/PassiveTreeView.lua +++ b/src/Classes/PassiveTreeView.lua @@ -539,9 +539,9 @@ function PassiveTreeViewClass:Draw(build, viewPort, inputEvents) elseif treeClick == "RIGHT" then -- User right-clicked on a node if hoverNode then - if IsKeyDown("SHIFT") then - -- Shift+Right-Click: open a popup to edit the per-node author note - -- (consumed by the PoE2 .build export as the node's additional_text). + if IsKeyDown("SHIFT") and hoverNode.alloc then + -- Shift+Right-Click on an allocated node: edit the per-node author + -- note emitted into the PoE2 .build export as additional_text. local nodeId = hoverNode.id local title = "Note: " .. (hoverNode.dn or hoverNode.name or "Passive") main:OpenNoteEditPopup(title, spec.nodeNotes[nodeId], function(text) @@ -1999,13 +1999,16 @@ function PassiveTreeViewClass:AddNodeTooltip(tooltip, node, build, incSmallPassi tooltip:AddLine(14, colorCodes.TIP.."Tip: Hold Ctrl to hide this tooltip.") tooltip:AddLine(14, colorCodes.TIP.."Tip: Press Ctrl+C to copy this node's text.") end - -- Per-node author note (Shift+Right-Click to set/edit) emitted into the PoE2 .build export. - if node.id and build.spec and build.spec.nodeNotes then + -- Per-node author note (Shift+Right-Click on an allocated node to set/edit). + if node.id and node.alloc and build.spec and build.spec.nodeNotes then local existing = build.spec.nodeNotes[node.id] - tooltip:AddSeparator(10) - tooltip:AddLine(14, colorCodes.TIP.."Shift + Right-Click to add a build note (PoE2 .build export)") if existing and existing ~= "" then + tooltip:AddSeparator(10) tooltip:AddLine(14, "^7Note: "..existing) + tooltip:AddLine(14, colorCodes.TIP.."Shift + Right-Click to edit (PoE2 .build export)") + else + tooltip:AddSeparator(10) + tooltip:AddLine(14, colorCodes.TIP.."Shift + Right-Click to add a build note (PoE2 .build export)") end end end diff --git a/src/Classes/SkillsTab.lua b/src/Classes/SkillsTab.lua index ae8165f5fc..839fa8aa08 100644 --- a/src/Classes/SkillsTab.lua +++ b/src/Classes/SkillsTab.lua @@ -7,6 +7,7 @@ local pairs = pairs local ipairs = ipairs local t_insert = table.insert local t_remove = table.remove +local m_floor = math.floor local m_min = math.min local m_max = math.max @@ -1441,7 +1442,7 @@ function SkillsTabClass:OpenSkillSetManagePopup() return function(buf) local n = tonumber(buf) if not n then return nil end - n = math.floor(n) + n = m_floor(n) if n < 0 then n = 0 end if n > 100 then n = 100 end return n @@ -1455,7 +1456,7 @@ function SkillsTabClass:OpenSkillSetManagePopup() local set = selectedSet() if set then set.levelMin = clampLvl(buf) - self.build.modFlag = true + self.modFlag = true end end) controls.lvlMin.tooltipText = "Lowest character level this skill set applies to in the exported .build (1-100). Leave blank to auto-split across skill sets." @@ -1464,7 +1465,7 @@ function SkillsTabClass:OpenSkillSetManagePopup() local set = selectedSet() if set then set.levelMax = clampLvl(buf) - self.build.modFlag = true + self.modFlag = true end end) controls.lvlMax.tooltipText = "Highest character level this skill set applies to in the exported .build (1-100). Leave blank to auto-split across skill sets." diff --git a/src/Modules/BuildExportPoE2.lua b/src/Modules/BuildExportPoE2.lua index 0b1e80114a..571928180e 100644 --- a/src/Modules/BuildExportPoE2.lua +++ b/src/Modules/BuildExportPoE2.lua @@ -68,7 +68,7 @@ local function autoBracket(i, n) return presetBrackets[n][i] end local lo = (i == 1) and 1 or (m_floor((i - 1) / n * 99) + 1) - local hi = (i == n) and 100 or m_floor(i / n * 99) + local hi = (i == n) and 100 or m_max(lo, m_floor(i / n * 99)) return { lo, hi } end @@ -152,9 +152,9 @@ function M.PresetNextLevels(existingEntries, newEntry) local maxLvl = 0 local anyHas = false for _, entry in ipairs(existingEntries or {}) do - if entry.levelMax then + if entry.levelMin or entry.levelMax then anyHas = true - if entry.levelMax > maxLvl then maxLvl = entry.levelMax end + if entry.levelMax and entry.levelMax > maxLvl then maxLvl = entry.levelMax end end end if not anyHas then @@ -178,11 +178,13 @@ local POE2_APP_ID = "2694490" local POE2_RELATIVE = "Documents" .. "/" .. "My Games" .. "/" .. "Path of Exile 2" .. "/" .. "BuildPlanner" local function dirExists(path) - local handle = io.popen('ls -d "' .. path .. '" 2>/dev/null') - if not handle then return false end - local result = handle:read("*a") - handle:close() - return result and result:match("%S") ~= nil + -- os.rename(x, x) is a POSIX-guaranteed no-op when x exists; the returned + -- error distinguishes ENOENT ("No such file") from other failures (e.g. + -- EBUSY, EACCES) that still indicate the path exists. No shell is spawned + -- and no path content is ever interpolated into a command string. + local ok, err = os.rename(path, path) + if ok then return true end + return err ~= nil and not err:find("No such file") and not err:find("does not exist") end local function tryProtonPath(baseSteam) @@ -227,8 +229,9 @@ end -- and should not be emitted into the PassiveSkills table the loader looks up. local MAX_NORMAL_NODE_ID = 65536 -local warnedNoStringId = false - +-- buildPassives returns (outTable, warningString|nil). +-- warningString is non-nil when tree.lua lacks stringId fields, meaning the +-- in-game BuildPlanner may not recognise the numeric passive ids. local function buildPassives(build, brackets) local specList = build.treeTab and build.treeTab.specList if not specList then return {} end @@ -278,15 +281,12 @@ local function buildPassives(build, brackets) end end end - -- The PoE2 BuildPlanner expects PassiveSkills.Id strings (e.g. "projectiles18", - -- "AscendancyMercenary2Notable5"). PoB's tree.lua only carries numeric ids - -- until src/Export/Scripts/passivetree.lua is re-run against GGPK to emit -- cspell:ignore passivetree - -- the stringId field. Warn once when that hasn't happened — the file will - -- still write but the loader probably won't recognise the passive ids. - if sawMissingStringId and not sawStringId and not warnedNoStringId then - warnedNoStringId = true - ConPrintf("[PoE2Export] tree.lua has no stringId fields; passive ids will be numeric and may not be recognised by the in-game BuildPlanner. Regenerate tree.lua via src/Export/Scripts/passivetree.lua to fix.") + local passiveWarning = nil + if sawMissingStringId and not sawStringId then + passiveWarning = "tree.lua has no stringId fields — passive ids are numeric and may not be recognised by the in-game BuildPlanner. Regenerate tree.lua via Export/Scripts/passivetree.lua to fix." + ConPrintf("[PoE2Export] " .. passiveWarning) end + table.sort(order) local out = {} for _, nodeId in ipairs(order) do local m = merged[nodeId] @@ -301,12 +301,11 @@ local function buildPassives(build, brackets) t_insert(out, entry) end end - return out + return out, passiveWarning end --- The auto-generated Gems.lua has a typo in ~486 entries' `gameId` field --- ("Metadata/Items/Gem/" vs the canonical "Metadata/Items/Gems/"). The table --- KEY is correct; `gemInstance.gemId` stores that key, so prefer it. +-- Prefer the Gems.lua table key (gemId) over the gemData.gameId field; +-- the key is always authoritative whereas gameId may differ in some entries. local function gemIdFor(gem) if gem and gem.gemId then return gem.gemId end return gem and gem.gemData and gem.gemData.gameId or nil @@ -470,7 +469,11 @@ local function buildItems(build, brackets) local itemSet = itemsTab.itemSets[setId] local interval = brackets and brackets[setIdx] or nil if itemSet then - for pobSlotName, mapping in pairs(M.SlotMap) do + local slotNames = {} + for k in pairs(M.SlotMap) do t_insert(slotNames, k) end + table.sort(slotNames) + for _, pobSlotName in ipairs(slotNames) do + local mapping = M.SlotMap[pobSlotName] local slotEntry = itemSet[pobSlotName] if slotEntry and slotEntry.selItemId and slotEntry.selItemId ~= 0 then local item = itemsTab.items[slotEntry.selItemId] @@ -535,28 +538,31 @@ function M.BuildTable(build) itemBrackets = bracketsFor(build.itemsTab.itemSetOrderList, function(id) return getItemSet(build.itemsTab, id) end) end - root.passives = buildPassives(build, treeBrackets) + local passiveWarning + root.passives, passiveWarning = buildPassives(build, treeBrackets) root.skills = buildSkills(build, skillBrackets) root.items = buildItems(build, itemBrackets) - return root + return root, passiveWarning end ---- Returns (jsonString, nil) on success, or (nil, errorMessage) on failure. +--- Returns (jsonString, nil, warning) on success, or (nil, errorMessage) on failure. +--- warning is a non-nil string when the export succeeded but passive ids are +--- numeric (tree.lua has no stringId fields) and may not be recognised in-game. function M.Export(build) - local root = M.BuildTable(build) + local root, passiveWarning = M.BuildTable(build) -- Force array-ness on the three top-level lists even when empty so the -- loader sees `[]` instead of `{}`. local state = { indent = true, level = 0 } local json, err = dkjson.encode(root, state) if not json then return nil, "JSON encode failed: " .. tostring(err) end - return json + return json, nil, passiveWarning end -- Writes the exported build to disk. Caller should confirm overwrite with the -- user before calling this — no existing-file check is performed here. ---- Returns (path, nil) on success. +--- Returns (path, nil, warning) on success; (nil, errMsg) on failure. function M.WriteFile(build, path) - local json, err = M.Export(build) + local json, err, warning = M.Export(build) if not json then return nil, err end -- Best-effort: ensure the target directory exists. local dir = path:match("^(.*[/\\])") @@ -565,7 +571,7 @@ function M.WriteFile(build, path) if not f then return nil, "Couldn't open '" .. path .. "': " .. tostring(fileErr) end f:write(json) f:close() - return path + return path, nil, warning end return M From cbca9444ea8313496d2cfb21475b167ab9262be7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 00:06:00 +0000 Subject: [PATCH 2/3] fix(export): address ai-review follow-up findings on PR #76 - ImportTab: fix safePath gsub no-op ('^' -> '^^' to actually escape caret characters for PoB's color-code renderer) - BuildExportPoE2: make dirExists locale-independent by checking only ok == true from os.rename; error strings are locale-sensitive and cannot reliably distinguish ENOENT from EACCES - Tests: add Export test covering the passiveWarning non-nil path (allocated node with no stringId triggers the degraded-export warning) https://claude.ai/code/session_01EvGnnyxkoMawLmWWqCTxxX --- spec/System/TestBuildExportPoE2_spec.lua | 9 +++++++++ src/Classes/ImportTab.lua | 2 +- src/Modules/BuildExportPoE2.lua | 14 +++++++------- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/spec/System/TestBuildExportPoE2_spec.lua b/spec/System/TestBuildExportPoE2_spec.lua index 3008715a89..ed09c3c9ba 100644 --- a/spec/System/TestBuildExportPoE2_spec.lua +++ b/spec/System/TestBuildExportPoE2_spec.lua @@ -269,6 +269,15 @@ describe("TestBuildExportPoE2", function() -- An empty build has no passives so no stringId check fires. assert.is_nil(warning) end) + + it("Returns warning when allocated nodes lack stringId", function() + -- Simulate a node without stringId to trigger the degraded-export path. + build.treeTab.specList[1].allocNodes = { [1] = {} } + local _, err, warning = BuildExportPoE2.Export(build) + assert.is_nil(err) + assert.is_not_nil(warning) + assert.is_truthy(warning:find("stringId")) + end) end) describe("WriteFile", function() diff --git a/src/Classes/ImportTab.lua b/src/Classes/ImportTab.lua index 76085866fc..2a054bccbb 100644 --- a/src/Classes/ImportTab.lua +++ b/src/Classes/ImportTab.lua @@ -402,7 +402,7 @@ function ImportTabClass:DoPoE2Export(Exporter, path) local function doWrite() local ok, err, warning = Exporter.WriteFile(self.build, path) if ok then - local safePath = path:gsub("%^", "^") + local safePath = path:gsub("%^", "^^") if warning then self.poe2ExportStatus = colorCodes.WARNING .. "Saved (warning): " .. warning else diff --git a/src/Modules/BuildExportPoE2.lua b/src/Modules/BuildExportPoE2.lua index 571928180e..fa240a40a2 100644 --- a/src/Modules/BuildExportPoE2.lua +++ b/src/Modules/BuildExportPoE2.lua @@ -178,13 +178,13 @@ local POE2_APP_ID = "2694490" local POE2_RELATIVE = "Documents" .. "/" .. "My Games" .. "/" .. "Path of Exile 2" .. "/" .. "BuildPlanner" local function dirExists(path) - -- os.rename(x, x) is a POSIX-guaranteed no-op when x exists; the returned - -- error distinguishes ENOENT ("No such file") from other failures (e.g. - -- EBUSY, EACCES) that still indicate the path exists. No shell is spawned - -- and no path content is ever interpolated into a command string. - local ok, err = os.rename(path, path) - if ok then return true end - return err ~= nil and not err:find("No such file") and not err:find("does not exist") + -- os.rename(x, x) is a POSIX-guaranteed no-op when x exists; we only + -- trust the true-return because error strings are locale-dependent and + -- cannot be used to distinguish ENOENT from EACCES reliably. False + -- negatives (directory exists but isn't renameable) are acceptable here + -- since this function only determines a suggested default path. + local ok = os.rename(path, path) + return ok == true end local function tryProtonPath(baseSteam) From 99c9ea5caf9ef4bcbf6367ea33fdf9fc50e4b12f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 00:09:38 +0000 Subject: [PATCH 3/3] test(export): cover autoBracket hi>=lo invariant for n>=100 sets Adds an Export test that places 100 passive specs with one allocated node in the first spec, then asserts the emitted level_interval has lo <= hi. Regresses the underflow that existed before the m_max fix (n=100, i=1 gave hi=floor(1/100*99)=0 < lo=1). https://claude.ai/code/session_01EvGnnyxkoMawLmWWqCTxxX --- spec/System/TestBuildExportPoE2_spec.lua | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/System/TestBuildExportPoE2_spec.lua b/spec/System/TestBuildExportPoE2_spec.lua index ed09c3c9ba..c70c02cbd5 100644 --- a/spec/System/TestBuildExportPoE2_spec.lua +++ b/spec/System/TestBuildExportPoE2_spec.lua @@ -278,6 +278,21 @@ describe("TestBuildExportPoE2", function() assert.is_not_nil(warning) assert.is_truthy(warning:find("stringId")) end) + + it("autoBracket never produces hi < lo for n >= 100 specs", function() + -- With n=100, i=1: old code gave hi=floor(1/100*99)=0 < lo=1. + -- The m_max fix ensures hi >= lo. + build.treeTab.specList[1].allocNodes = { [1] = {} } + for i = 2, 100 do + build.treeTab.specList[i] = { id = i, allocNodes = {} } + end + local root = BuildExportPoE2.BuildTable(build) + local first = root.passives[1] + if type(first) == "table" and first.level_interval then + assert.is_true(first.level_interval[1] <= first.level_interval[2], + "autoBracket produced hi < lo: " .. tostring(first.level_interval[1]) .. " > " .. tostring(first.level_interval[2])) + end + end) end) describe("WriteFile", function()