fix: atomic write, tag preservation, invalid preset error, additional tests

conftest.py

@@ -0,0 +1,5 @@
+# conftest.py
+import sys
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).parent / "scripts"))

scripts/merge_prefs.py

@@ -8,7 +8,9 @@ Usage:
                            --preset cc|cs6
 """
 import argparse
+import os
 import shutil
+import tempfile
 from pathlib import Path
 from xml.etree import ElementTree as ET
 
@@ -24,14 +26,19 @@ PRESET_OVERRIDES = {
 }
 
 
-def _find_or_create(parent: ET.Element, id_val: str) -> ET.Element:
-    """Return child with matching id, or create and append a new one."""
-    for child in parent:
-        if child.get("id") == id_val:
-            return child
-    child = ET.SubElement(parent, "group")
-    child.set("id", id_val)
-    return child
+def _atomic_write(tree: ET.ElementTree, path: Path) -> None:
+    """Write XML tree atomically — prevents corruption on interrupt."""
+    fd, tmp = tempfile.mkstemp(dir=path.parent, suffix=".tmp")
+    try:
+        with os.fdopen(fd, "w", encoding="utf-8") as fh:
+            tree.write(fh, encoding="unicode", xml_declaration=True)
+        os.replace(tmp, path)
+    except Exception:
+        try:
+            os.unlink(tmp)
+        except OSError:
+            pass
+        raise
 
 
 def _merge_nodes(live_parent: ET.Element, patch_parent: ET.Element) -> None:
@@ -40,24 +47,30 @@ def _merge_nodes(live_parent: ET.Element, patch_parent: ET.Element) -> None:
         child_id = patch_child.get("id")
         if child_id is None:
             continue
-
-        live_child = _find_or_create(live_parent, child_id)
-
-        # Merge attributes: only set attrs present in patch; preserve unknown attrs
+        live_child = next(
+            (c for c in live_parent if c.get("id") == child_id), None
+        )
+        if live_child is None:
+            live_child = ET.SubElement(live_parent, patch_child.tag)
+            live_child.set("id", child_id)
         for attr, value in patch_child.attrib.items():
             if attr != "id":
                 live_child.set(attr, value)
-
-        # Recurse into children
         _merge_nodes(live_child, patch_child)
 
 
 def _apply_preset(root: ET.Element, preset: str) -> None:
     """Apply preset-specific overrides (active keys file, document units)."""
+    if preset not in PRESET_OVERRIDES:
+        raise ValueError(f"Unknown preset {preset!r}. Valid: {list(PRESET_OVERRIDES)}")
     for id_path, attrs in PRESET_OVERRIDES[preset].items():
         node = root
         for id_val in id_path:
-            node = _find_or_create(node, id_val)
+            child = next((c for c in node if c.get("id") == id_val), None)
+            if child is None:
+                child = ET.SubElement(node, "group")
+                child.set("id", id_val)
+            node = child
         for attr, value in attrs.items():
             node.set(attr, value)
 
@@ -73,7 +86,7 @@ def merge(prefs_path: Path, patch_path: Path, preset: str) -> None:
         shutil.copy(patch_path, prefs_path)
         live_tree = ET.parse(prefs_path)
         _apply_preset(live_tree.getroot(), preset)
-        live_tree.write(str(prefs_path), encoding="unicode", xml_declaration=True)
+        _atomic_write(live_tree, prefs_path)
         return
 
     live_tree = ET.parse(prefs_path)  # raises ParseError on malformed XML

tests/test_merge_prefs.py

@@ -1,12 +1,10 @@
 """Tests for scripts/merge_prefs.py"""
 import shutil
-import sys
 from pathlib import Path
 from xml.etree import ElementTree as ET
 
 import pytest
 
-sys.path.insert(0, str(Path(__file__).parent.parent / "scripts"))
 import merge_prefs
 
 FIXTURES = Path(__file__).parent / "fixtures"
@@ -138,3 +136,19 @@ def test_merge_raises_on_missing_patch(tmp_path):
     shutil.copy(FIXTURES / "prefs-fresh.xml", prefs)
     with pytest.raises(FileNotFoundError):
         merge_prefs.merge(prefs, tmp_path / "nonexistent-patch.xml", "cc")
+
+
+def test_merge_raises_on_malformed_patch(tmp_path):
+    prefs = tmp_path / "preferences.xml"
+    shutil.copy(FIXTURES / "prefs-fresh.xml", prefs)
+    bad_patch = tmp_path / "bad-patch.xml"
+    bad_patch.write_text("not xml at all")
+    with pytest.raises(ET.ParseError):
+        merge_prefs.merge(prefs, bad_patch, "cc")
+
+
+def test_merge_raises_on_invalid_preset(tmp_path):
+    prefs = tmp_path / "preferences.xml"
+    shutil.copy(FIXTURES / "prefs-fresh.xml", prefs)
+    with pytest.raises(ValueError, match="Unknown preset"):
+        merge_prefs.merge(prefs, FIXTURES / "patch-minimal.xml", "bad-preset")