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

[jay9297]

Summary

Fixes all issues identified in the adversarial review of PR #60. Changes are scoped strictly to the export feature branch (feature/poe2-build-export-improved).

Fixes applied

CRITICAL

• nodeNotes lost on undo/tree-copy (PassiveSpec.lua): Added nodeNotes = copyTable(self.nodeNotes) to CreateUndoState and self.nodeNotes = state.nodeNotes or {} in RestoreUndoState. Without this, every Ctrl+Z after setting a node note silently erased it, and copying a passive tree lost all notes.
• io.popen('ls -d ...') crash + injection (BuildExportPoE2.lua): Replaced with os.rename(path, path) — a POSIX-guaranteed no-op when the path exists. No shell is spawned, no path content is interpolated into a command string. Eliminates both the Windows crash risk and the shell injection vector.

HIGH

• warnedNoStringId module global (BuildExportPoE2.lua): Removed the per-session warn-once guard. The warning is now returned as a third value from buildPassives → BuildTable → Export → WriteFile, so every call site gets fresh state. ImportTab.DoPoE2Export now displays the warning as a colorCodes.WARNING status instead of silently succeeding.
• Non-deterministic JSON output (BuildExportPoE2.lua): table.sort(order) added in buildPassives before emitting; buildItems now iterates a sorted copy of SlotMap keys. Eliminates pairs()-over-hash-table ordering surprises.

MEDIUM

• PresetNextLevels anyHas checks only levelMax (BuildExportPoE2.lua): Changed to levelMin or levelMax and guarded the maxLvl update with entry.levelMax and — prevents double-seeding an entry that already has only levelMin set.
• autoBracket gives hi < lo for n > 99 (BuildExportPoE2.lua): Changed hi formula to use m_max(lo, m_floor(i/n * 99)), guaranteeing hi >= lo for any n.
• SkillsTab clampLvl fallback uses global math.floor (SkillsTab.lua): Added local m_floor = math.floor at module top; fallback now uses the local alias consistently with the rest of the file.
• self.build.modFlag vs self.modFlag (SkillsTab.lua): Level-bracket edit callbacks now correctly set self.modFlag (tab-level dirty flag) instead of self.build.modFlag.

LOW

• Note popup fires on unallocated nodes (PassiveTreeView.lua): Gated both the Shift+Right-Click popup and the tooltip hint on hoverNode.alloc / node.alloc. The tooltip now shows existing note content prominently when present, with the edit hint below.
• Misleading gemIdFor comment (BuildExportPoE2.lua): Replaced the factually incorrect "typo in gameId" claim with an accurate description of why gemId is preferred over gemData.gameId.

Test plan

• Run full suite: docker compose run --rm busted-tests
• Verify new PresetNextLevels test case (levelMin-only entry) passes
• Verify new Export warning-return test passes
• Manually: allocate a passive node, Shift+Right-Click to add a note, press Ctrl+Z — note should survive undo
• Manually: copy a passive tree — notes should be preserved in the copy
• Manually: export a build and confirm the status label shows the warning when tree.lua lacks stringIds

https://claude.ai/code/session_01EvGnnyxkoMawLmWWqCTxxX

---

Generated by Claude Code

[github-actions]

🔄 Cycle 1, attempt 1/3

src/Classes/ImportTab.lua:405 — path:gsub("%^", "^") replaces ^ with ^, a no-op. safePath is always identical to path. If the intent is to escape caret characters for the color-code renderer (^x33FF77 prefix), the replacement must be "^^".

src/Modules/BuildExportPoE2.lua (dirExists) — err:find("No such file") and err:find("does not exist") are locale-sensitive; on non-English libc locales ENOENT carries a different message, causing the function to return true for a missing directory and silently attempt writes into a non-existent path.

Missing test — No test exercises the passiveWarning-non-nil path (the sawMissingStringId and not sawStringId branch in buildPassives); the new test only validates the nil case.

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: REQUEST_CHANGES. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
SkillsTab.lua:1459,1468 — self.modFlag = true is correct here; Build.lua:1253,1374 explicitly reads skillsTab.modFlag to track unsaved state and the rest of the file uses the same pattern. The original self.build.modFlag was the bug.

PassiveSpec.lua:2396 — copyTable(self.nodeNotes) is safe; nodeNotes is always initialized to {} at line 104 of the constructor.

BuildExportPoE2.lua:68 — The m_max(lo, ...) guard in autoBracket fixes a real underflow (e.g., n≥100, i=1 gives m_floor(1/100*99)=0 < lo=1), but no test covers this edge case.

✅ Agent succeeded (cycle 1, attempt 1)

[github-actions]

Verdict: COMMENT. See review comment above.

[github-actions]

🔄 Cycle 1, attempt 1/3
All clean. self.modFlag on SkillsTab is properly wired into build:ResetModFlags() and the unsaved check at Build.lua:1374.

✅ Agent succeeded (cycle 1, attempt 1)