fix(cli): finalize JSONC parsing with safe trailing commas and secure fallback logic

Author: RbBtSn0w

src/specify_cli/__init__.py

@@ -685,9 +685,9 @@ def strip_json_comments(content: str) -> str:
     
     This implementation handles VSCode settings.json files by:
     1. Skipping comments inside "string literals"
-    2. Correctlly handling escaped quotes by counting backslashes
+    2. Correctly handling escaped quotes by counting backslashes
     3. Removing single-line (//) and multi-line (/* */) comments
-    4. Handling trailing commas safely (only outside strings)
+    4. Handling trailing commas safely (only outside strings/comments)
     """
     import re
 
@@ -745,22 +745,28 @@ def strip_json_comments(content: str) -> str:
                 i = length # End of content
             continue
 
+        # Handle trailing commas (safe because we're outside strings and comments)
+        if char == ',':
+            # Peek ahead to find the next non-whitespace character
+            next_i = i + 1
+            is_trailing = False
+            while next_i < length:
+                next_char = content[next_i]
+                if next_char in (' ', '\t', '\n', '\r'):
+                    next_i += 1
+                    continue
+                if next_char in (']', '}'):
+                    is_trailing = True
+                break
+            
+            if is_trailing:
+                i += 1 # Skip the comma
+                continue
+
         result.append(char)
         i += 1
 
-    clean_content = "".join(result)
-    
-    # Handle trailing commas: find a comma followed by closing brace/bracket
-    # This is now safer as we've already stripped comments and identified strings
-    # However, to be 100% safe against commas in strings, we should have handled this 
-    # during the character loop. Let's refine the regex to be very specific to 
-    # JSON structure (comma followed by whitespace and closing char).
-    # Since we've already joined the result, we can apply a final cleanup.
-    # To truly avoid commas in strings, we'd need to mark string ranges.
-    # For now, this regex is standard for basic JSONC to JSON conversion.
-    clean_content = re.sub(r',\s*([\]}])', r'\1', clean_content)
-    
-    return clean_content
+    return "".join(result)
 
 def merge_json_files(existing_path: Path, new_content: dict, verbose: bool = False) -> dict:
     """Merge new JSON content into existing JSON file.
@@ -779,32 +785,37 @@ def merge_json_files(existing_path: Path, new_content: dict, verbose: bool = Fal
     Returns:
         Merged JSON content as dict
     """
-    if not isinstance(new_content, dict):
-        if verbose:
-            console.print(f"[yellow]Warning: Template content for {existing_path.name} is not a dictionary. Skipping merge.[/yellow]")
-        return {}
-
-    try:
-        if existing_path.exists():
+    # Load existing content first to have a safe fallback
+    existing_content = {}
+    exists = existing_path.exists()
+    
+    if exists:
+        try:
             with open(existing_path, 'r', encoding='utf-8') as f:
                 raw_content = f.read().lstrip('\ufeff')
                 # Handle comments (JSONC)
                 clean_content = strip_json_comments(raw_content)
                 existing_content = json.loads(clean_content)
-                
-            if not isinstance(existing_content, dict):
-                if verbose:
-                    console.print(f"[yellow]Warning: Existing JSON in {existing_path.name} is not an object, using template instead[/yellow]")
-                return new_content
-        else:
-            return new_content
-             
-    except (FileNotFoundError, json.JSONDecodeError) as e:
-        # If file is invalid, just use new content
-        if verbose and not isinstance(e, FileNotFoundError):
-            console.print(f"[yellow]Warning: Could not parse existing JSON in {existing_path.name} ({e}), using template instead[/yellow]")
+        except (FileNotFoundError, json.JSONDecodeError) as e:
+            if verbose and not isinstance(e, FileNotFoundError):
+                console.print(f"[yellow]Warning: Could not parse existing JSON in {existing_path.name} ({e}).[/yellow]")
+            # fallback to empty dict for merging
+
+    # Validate template content
+    if not isinstance(new_content, dict):
+        if verbose:
+            console.print(f"[yellow]Warning: Template content for {existing_path.name} is not a dictionary. Preserving existing settings.[/yellow]")
+        return existing_content if isinstance(existing_content, dict) else {}
+
+    if not exists:
         return new_content
 
+    # If existing content parsed but is not a dict, skip merge to avoid data loss
+    if not isinstance(existing_content, dict):
+        if verbose:
+            console.print(f"[yellow]Warning: Existing JSON in {existing_path.name} is not an object. Skipping merge to avoid data loss.[/yellow]")
+        return existing_content
+
     def deep_merge_polite(base: dict, update: dict) -> dict:
         """Recursively merge update dict into base dict, preserving base values."""
         result = base.copy()

tests/test_merge.py

@@ -49,6 +49,21 @@ def test_strip_json_comments_trailing_commas_advanced():
     assert parsed["list"][0]["id"] == 1
     assert parsed["metadata"]["tags"] == ["a", "b"]
 
+def test_strip_json_comments_safe_commas_in_strings():
+    """Verify that commas followed by closing characters INSIDE strings are not removed."""
+    jsonc = """
+    {
+        "tricky_string": "value, ]",
+        "another_one": "nested, }",
+        "normal": [1, 2, ]
+    }
+    """
+    stripped = strip_json_comments(jsonc)
+    parsed = json.loads(stripped)
+    assert parsed["tricky_string"] == "value, ]"
+    assert parsed["another_one"] == "nested, }"
+    assert parsed["normal"] == [1, 2]
+
 def test_strip_json_comments_whitespace():
     """Ensure stripping works with various whitespace and empty values."""
     jsonc = "{   \n\n   \"a\": 1\n, \n \"b\": 2 // comment \n }"
@@ -161,9 +176,9 @@ def test_merge_json_files_with_bom(tmp_path):
     assert merged == {"a": 1, "b": 2}
 
 def test_merge_json_files_not_a_dictionary_template(tmp_path):
-    """If for some reason new_content is not a dict, return empty dict as safety."""
+    """If for some reason new_content is not a dict, PRESERVE existing settings instead of wiping."""
     existing_file = tmp_path / "ok.json"
     existing_file.write_text('{"a": 1}')
 
-    # In the updated implementation, if new_content is not a dict, we return {}
-    assert merge_json_files(existing_file, ["not", "a", "dict"]) == {}
+    # Secure fallback: return existing settings to avoid clobbering
+    assert merge_json_files(existing_file, ["not", "a", "dict"]) == {"a": 1}