fix: prevent silent failures on invalid placeholder syntax (#237)

* fix #233: strict syntax validation for configuration placeholders

Signed-off-by: sushant-suse <[email protected]>

* fix #233: ruff issues

Signed-off-by: sushant-suse <[email protected]>

* fix: removed irrelevant files

Signed-off-by: sushant-suse <[email protected]>

---------

Signed-off-by: sushant-suse <[email protected]>

Author: sushant-suse

changelog.d/233.bugfix.rst

@@ -0,0 +1 @@
+Added strict syntax validation for configuration placeholders. The application will now explicitly report an error and halt when encountering malformed placeholders (e.g., missing or incorrectly ordered curly braces like ``{server.name``) instead of silently leaving them unresolved.

src/docbuild/config/app.py

@@ -24,6 +24,12 @@ class CircularReferenceError(ValueError):
     pass
 
 
+class PlaceholderSyntaxError(ValueError):
+    """Exception raised when a placeholder has invalid syntax."""
+
+    pass
+
+
 class PlaceholderResolver:
     """Handles placeholder resolution in configuration data."""
 
@@ -86,17 +92,43 @@ def _resolve_placeholder(self, match: re.Match) -> str:
                 f"key '{placeholder}' not found in current section.",
             )
 
+    def validate_brace_syntax(self, text: str, original_text: str) -> None:
+        """Validate that curly braces are balanced and properly ordered."""
+        brace_level = 0
+        for char in text:
+            if char == "{":
+                brace_level += 1
+            elif char == "}":
+                brace_level -= 1
+
+            # If it ever drops below 0, we've hit a '}' before a '{'
+            if brace_level < 0:
+                break
+
+        # If the level isn't exactly 0 at the end, the syntax is broken
+        if brace_level != 0:
+            container_name = self._get_container_name()
+
+            if "{" in text and "}" not in text:
+                msg = f"Missing end curly brace in placeholder in value: '{original_text}'"
+            elif "}" in text and "{" not in text:
+                msg = f"Missing start curly brace in placeholder in value: '{original_text}'"
+            else:
+                msg = f"Invalid placeholder syntax in value: '{original_text}'"
+
+            raise PlaceholderSyntaxError(f"In configuration key '{container_name}': {msg}")
+
     def _resolve_string_placeholders(self, text: str) -> str:
         """Resolve all placeholders in a string with recursion protection."""
+        original_text = text
         count = 0
         cls = self.__class__
+
+        # 1. Resolve valid placeholders
         while count < self.max_recursion_depth:
             new_text = cls.PLACEHOLDER_PATTERN.sub(self._resolve_placeholder, text)
-
             if new_text == text:
-                # No more changes, we're done
                 break
-
             text = new_text
             count += 1
 
@@ -106,7 +138,10 @@ def _resolve_string_placeholders(self, text: str) -> str:
                 f"Too many nested placeholder expansions in key '{key_name}'."
             )
 
-        # Replace escaped braces with literal ones
+        # 2. Syntax Validation using the helper method
+        self.validate_brace_syntax(text, original_text)
+
+        # 3. Final cleanup of escapes
         return text.replace("{{", "{").replace("}}", "}")
 
     def _get_container_name(self) -> str:
@@ -134,6 +169,7 @@ def replace(self) -> dict[str, Any]:
         :return: The configuration with all placeholders resolved.
         :raises PlaceholderResolutionError: If a placeholder cannot be resolved.
         :raises CircularReferenceError: If a circular reference is detected.
+        :raises PlaceholderSyntaxError: If a placeholder has invalid syntax.
         """
         # Use a stack to process all items iteratively
         # Stack items: (container, key, context) where container can be dict or list
@@ -183,6 +219,7 @@ def replace_placeholders(
     :return: A new dictionary with placeholders replaced.
     :raises PlaceholderResolutionError: If a placeholder cannot be resolved.
     :raises CircularReferenceError: If a circular reference is detected.
+    :raises PlaceholderSyntaxError: If a placeholder has invalid syntax.
     """
     if not isinstance(config, dict):
         return config

src/docbuild/models/config/app.py

@@ -10,6 +10,7 @@
 from docbuild.config.app import (
     CircularReferenceError,
     PlaceholderResolutionError,
+    PlaceholderSyntaxError,
     replace_placeholders,
 )
 from docbuild.logging import DEFAULT_LOGGING_CONFIG
@@ -235,7 +236,12 @@ def _resolve_placeholders(cls, data: dict[str, Any]) -> dict[str, Any] | None:
         if isinstance(data, dict):
             try:
                 return replace_placeholders(deepcopy(data))
-            except (PlaceholderResolutionError, CircularReferenceError) as e:
+            except (
+                PlaceholderResolutionError,
+                CircularReferenceError,
+                PlaceholderSyntaxError
+            ) as e:
+                # This wraps the error so Pydantic reports it as a validation failure
                 raise ValueError(f"Configuration placeholder error: {e}") from e
         return data
 

src/docbuild/models/config/env.py

@@ -17,6 +17,7 @@
 from ...config.app import (
     CircularReferenceError,
     PlaceholderResolutionError,
+    PlaceholderSyntaxError,
     replace_placeholders,
 )
 from ..language import LanguageCode
@@ -407,7 +408,11 @@ def _resolve_placeholders(cls, data: dict[str, Any]) -> dict[str, Any] | None:
         if isinstance(data, dict):
             try:
                 return replace_placeholders(deepcopy(data))
-            except (PlaceholderResolutionError, CircularReferenceError) as e:
+            except (
+                PlaceholderResolutionError,
+                CircularReferenceError,
+                PlaceholderSyntaxError
+            ) as e:
                 raise ValueError(f"Configuration placeholder error: {e}") from e
         return data
 

tests/config/app/test_app_replace_placeholders.py

@@ -8,6 +8,7 @@
     CircularReferenceError,
     PlaceholderResolutionError,
     PlaceholderResolver,
+    PlaceholderSyntaxError,
     replace_placeholders,
 )
 from docbuild.models.config.app import AppConfig
@@ -331,3 +332,24 @@ def test_nested_list_processing_appends_stack_items():
     out = replace_placeholders(config)
     assert out["seq"][0]["inner"] == "VAL"
     assert out["seq"][1] == "VAL"
+
+
+def test_placeholder_syntax_missing_end():
+    """Test orphaned opening brace."""
+    resolver = PlaceholderResolver({"key": "{server.name"})
+    with pytest.raises(PlaceholderSyntaxError, match="Missing end curly brace"):
+        resolver.replace()
+
+
+def test_placeholder_syntax_missing_start():
+    """Test orphaned closing brace."""
+    resolver = PlaceholderResolver({"key": "server.name}"})
+    with pytest.raises(PlaceholderSyntaxError, match="Missing start curly brace"):
+        resolver.replace()
+
+
+def test_placeholder_syntax_swapped_escapes():
+    """Test the reviewer's edge case: }} hello {{."""
+    resolver = PlaceholderResolver({"key": "}} hello {{"})
+    with pytest.raises(PlaceholderSyntaxError, match="Invalid placeholder syntax"):
+        resolver.replace()

tests/models/config/test_env.py

@@ -295,3 +295,22 @@ def test_envconfig_with_non_dict_input():
     # Pydantic will then raise a ValidationError because the input is not a dict.
     with pytest.raises(ValidationError, match="Input should be a valid dictionary"):
         EnvConfig.model_validate("not a dict")
+
+
+def test_env_config_invalid_placeholder_syntax():
+    """Ensure EnvConfig raises ValueError for malformed placeholders."""
+    invalid_data = {
+        "server": {"name": "test", "role": "production", "host": "127.0.0.1", "enable_mail": False},
+        "config": {"default_lang": "en-us", "languages": ["en-us"], "canonical_url_domain": "https://example.com"},
+        "paths": {
+            # ... other required paths ...
+            "base_server_cache_dir": "{base_cache_dir}/{server.name" # THE BUG
+        },
+        "build": {
+            "daps": {"command": "daps", "meta": "daps metadata"},
+            "container": {"container": "some-image"}
+        }
+    }
+    # We expect a ValueError because our model validator wraps PlaceholderSyntaxError
+    with pytest.raises(ValueError, match="Configuration placeholder error"):
+        EnvConfig.from_dict(invalid_data)