fix(export): address adversarial review findings in PR #60 PoE2 build export (#76)

* fix(export): address adversarial review findings in PoE2 build export

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

* 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

* 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

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: jay9297

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,37 @@ 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)
+
+		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)
+
+		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()

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")

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
 

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

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."

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; 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)
@@ -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