Merge pull request #3713 from rommapp/posthog-code/fix-config-parse-error-surfacing

fix: surface config.yml parse errors instead of silently using defaults

Author: gantoine

backend/config/config_manager.py

@@ -107,6 +107,7 @@ class NetplayICEServer(TypedDict):
 class Config:
     CONFIG_FILE_MOUNTED: bool
     CONFIG_FILE_WRITABLE: bool
+    CONFIG_FILE_PARSE_ERROR: str | None
     EXCLUDED_PLATFORMS: list[str]
     EXCLUDED_SINGLE_EXT: list[str]
     EXCLUDED_SINGLE_FILES: list[str]
@@ -174,6 +175,7 @@ class ConfigManager:
     _raw_config: dict = {}
     _config_file_mounted: bool = False
     _config_file_writable: bool = False
+    _config_file_parse_error: str | None = None
 
     def __new__(cls, *args, **kwargs):
         if cls._self is None:
@@ -208,13 +210,18 @@ def _safe_load_yaml(self, cf) -> dict:
         """Load YAML, falling back to an empty config on syntax errors so the
         app can still boot with defaults rather than crashing."""
         try:
-            return yaml.load(cf, Loader=SafeLoader) or {}
+            config = yaml.load(cf, Loader=SafeLoader) or {}
+            self._config_file_parse_error = None
+            return config
         except yaml.YAMLError as exc:
             log.critical(
                 f"Failed to parse {hl(self.config_file, BLUE)}: {exc}. "
                 "Falling back to default configuration, fix the YAML "
                 "syntax to apply your settings."
             )
+            # Keep the error around so it can be surfaced in the UI, since the
+            # whole config (not just the broken part) is discarded here.
+            self._config_file_parse_error = str(exc)
             return {}
 
     def _create_missing_config_file(self) -> None:
@@ -230,6 +237,7 @@ def _create_missing_config_file(self) -> None:
             # Reset any previously loaded singleton state so parsing reflects
             # the newly created empty config file.
             self._raw_config = {}
+            self._config_file_parse_error = None
             self._config_file_mounted = True
             self._config_file_writable = os.access(self.config_file, os.W_OK)
         except PermissionError:
@@ -287,6 +295,7 @@ def _parse_config(self):
         self.config = Config(
             CONFIG_FILE_MOUNTED=self._config_file_mounted,
             CONFIG_FILE_WRITABLE=self._config_file_writable,
+            CONFIG_FILE_PARSE_ERROR=self._config_file_parse_error,
             EXCLUDED_PLATFORMS=sorted(
                 {
                     *DEFAULT_EXCLUDED_DIRS,
@@ -723,6 +732,8 @@ def get_config(self) -> Config:
                 self._raw_config = self._safe_load_yaml(config_file)
         except FileNotFoundError:
             log.debug("Config file not found!")
+            # No file to parse, so clear any stale parse error from a prior load.
+            self._config_file_parse_error = None
 
         self._parse_config()
         self._validate_config()

backend/endpoints/configs.py

@@ -42,6 +42,10 @@ def get_config(request: Request) -> ConfigResponse:
     return ConfigResponse(
         CONFIG_FILE_MOUNTED=cfg.CONFIG_FILE_MOUNTED,
         CONFIG_FILE_WRITABLE=cfg.CONFIG_FILE_WRITABLE,
+        # Raw parser error may leak the config file path, so only send when authenticated
+        CONFIG_FILE_PARSE_ERROR=(
+            cfg.CONFIG_FILE_PARSE_ERROR if request.user.is_authenticated else None
+        ),
         EXCLUDED_PLATFORMS=cfg.EXCLUDED_PLATFORMS,
         EXCLUDED_SINGLE_EXT=cfg.EXCLUDED_SINGLE_EXT,
         EXCLUDED_SINGLE_FILES=cfg.EXCLUDED_SINGLE_FILES,

backend/endpoints/responses/config.py

@@ -6,6 +6,7 @@
 class ConfigResponse(TypedDict):
     CONFIG_FILE_MOUNTED: bool
     CONFIG_FILE_WRITABLE: bool
+    CONFIG_FILE_PARSE_ERROR: str | None
     EXCLUDED_PLATFORMS: list[str]
     EXCLUDED_SINGLE_EXT: list[str]
     EXCLUDED_SINGLE_FILES: list[str]

backend/tests/config/test_config_loader.py

@@ -160,6 +160,74 @@ def test_malformed_yaml_falls_back_to_defaults():
     assert loader.config.ROMS_FOLDER_NAME == "roms"
     assert loader.config.FIRMWARE_FOLDER_NAME == "bios"
     assert loader.config.SCAN_MEDIA == ["box2d", "screenshot", "manual"]
+    # The parse error is surfaced so the UI can warn the user their whole
+    # config (not just the broken part) was discarded.
+    assert loader.config.CONFIG_FILE_PARSE_ERROR is not None
+
+
+def test_valid_config_has_no_parse_error():
+    """A syntactically valid config should not report a parse error."""
+    loader = ConfigManager(
+        os.path.join(Path(__file__).resolve().parent, "fixtures", "config/config.yml")
+    )
+
+    assert loader.config.CONFIG_FILE_PARSE_ERROR is None
+
+
+def test_mixed_gamelist_syntax_surfaces_parse_error(tmp_path):
+    """Regression for #3708: mixing the old (`scan.export`) and new
+    (`scan.gamelist.export`) syntax produces invalid YAML. The whole config is
+    discarded and defaults are used, so the parse error must be surfaced rather
+    than silently swallowed."""
+    config_file = tmp_path / "config.yml"
+    config_file.write_text(
+        "scan:\n"
+        "  gamelist:\n"
+        "    export: true\n"
+        "  - gamelist_xml: true\n"
+        "  media:\n"
+        "    - box2d\n"
+        "    - video\n"
+        "    - manual\n"
+    )
+
+    loader = ConfigManager(str(config_file))
+
+    assert loader.config.CONFIG_FILE_PARSE_ERROR is not None
+    # The user's scan.media list is lost, falling back to defaults.
+    assert loader.config.SCAN_MEDIA == ["box2d", "screenshot", "manual"]
+
+
+def test_parse_error_is_cleared_when_config_is_missing(tmp_path):
+    """A stale parse error from a malformed config must not persist once the
+    file is gone. The loader is a singleton, so a later reload that skips YAML
+    parsing (e.g. the file was deleted) has to clear the flag."""
+    config_file = tmp_path / "config.yml"
+    config_file.write_text("scan:\n  - broken: true\n  media: [box2d]\n")
+
+    loader = ConfigManager(str(config_file))
+    assert loader.config.CONFIG_FILE_PARSE_ERROR is not None
+
+    # get_config's FileNotFoundError branch must clear the stale error.
+    config_file.unlink()
+    loader.get_config()
+    assert loader.config.CONFIG_FILE_PARSE_ERROR is None
+
+
+def test_parse_error_is_cleared_when_config_is_recreated(tmp_path):
+    """Reloading via a missing file goes through _create_missing_config_file,
+    which must also clear a stale parse error from a prior malformed load."""
+    broken_file = tmp_path / "config.yml"
+    broken_file.write_text("scan:\n  - broken: true\n  media: [box2d]\n")
+
+    ConfigManager(str(broken_file))
+
+    # Reusing the singleton with a missing path exercises the __init__
+    # FileNotFoundError -> _create_missing_config_file recreate path.
+    broken_file.unlink()
+    loader = ConfigManager(str(broken_file))
+
+    assert loader.config.CONFIG_FILE_PARSE_ERROR is None
 
 
 def test_config_updates_serialize_gamelist_media_as_plain_strings(tmp_path):

backend/tests/endpoints/test_config.py

@@ -31,6 +31,28 @@ def test_config(client):
     assert config.get("GAMELIST_MEDIA_IMAGE") == "screenshot"
 
 
+def test_config_parse_error_gated_by_auth(client, access_token: str):
+    # The raw parser error can leak the config file path, so it is only
+    # returned to authenticated users.
+    cfg = cm.get_config()
+    cfg.CONFIG_FILE_PARSE_ERROR = 'in "/data/config.yml", line 2'
+
+    with patch.object(cm, "get_config", return_value=cfg):
+        anon = client.get("/api/config")
+        assert anon.status_code == status.HTTP_200_OK
+        assert anon.json().get("CONFIG_FILE_PARSE_ERROR") is None
+
+        auth = client.get(
+            "/api/config",
+            headers={"Authorization": f"Bearer {access_token}"},
+        )
+        assert auth.status_code == status.HTTP_200_OK
+        assert (
+            auth.json().get("CONFIG_FILE_PARSE_ERROR")
+            == 'in "/data/config.yml", line 2'
+        )
+
+
 def test_add_platform_binding_payload_shape(client, access_token: str):
     with patch.object(cm, "add_platform_binding") as add_platform_binding:
         response = client.post(

frontend/src/__generated__/models/ConfigResponse.ts

@@ -43,6 +43,8 @@
   "config-file-not-mounted-title": "Конфигурационният файл не е монтиран!",
   "config-file-not-writable-desc": "Файлът config.yml не е записваем. Промените в конфигурацията няма да се запазят след рестартиране на приложението.",
   "config-file-not-writable-title": "Конфигурационният файл не е записваем!",
+  "config-file-parse-error-desc": "The config.yml file has a syntax error, so RomM is running with default settings and ignoring your configuration. Fix the error and restart RomM to apply your settings. Details: {error}",
+  "config-file-parse-error-title": "Configuration file could not be parsed!",
   "config-tab": "Конфигурация",
   "confirm-delete-mapping": "Потвърди?",
   "continue-playing-as-grid": "Продължи игра като решетка",

frontend/src/locales/bg_BG/settings.json

@@ -43,6 +43,8 @@
   "config-file-not-mounted-title": "Konfigurační soubor není připojen!",
   "config-file-not-writable-desc": "Soubor config.yml nelze zapisovat. Veškeré změny provedené v konfiguraci se po restartování aplikace neuchovají.",
   "config-file-not-writable-title": "Konfigurační soubor nelze zapisovat!",
+  "config-file-parse-error-desc": "The config.yml file has a syntax error, so RomM is running with default settings and ignoring your configuration. Fix the error and restart RomM to apply your settings. Details: {error}",
+  "config-file-parse-error-title": "Configuration file could not be parsed!",
   "config-tab": "Konfigurace",
   "confirm-delete-mapping": "Potvrzujete?",
   "continue-playing-as-grid": "Pokračovat ve hraní jako mřížka",

frontend/src/locales/cs_CZ/settings.json

@@ -43,6 +43,8 @@
   "config-file-not-mounted-title": "Konfigurationsdatei nicht eingebunden!",
   "config-file-not-writable-desc": "Die config.yml-Datei ist nicht beschreibbar. Alle an der Konfiguration vorgenommenen Änderungen werden nach dem Neustart der Anwendung nicht beibehalten.",
   "config-file-not-writable-title": "Konfigurationsdatei nicht beschreibbar!",
+  "config-file-parse-error-desc": "The config.yml file has a syntax error, so RomM is running with default settings and ignoring your configuration. Fix the error and restart RomM to apply your settings. Details: {error}",
+  "config-file-parse-error-title": "Configuration file could not be parsed!",
   "config-tab": "Konfiguration",
   "confirm-delete-mapping": "Bestätigen Sie?",
   "continue-playing-as-grid": "Weiterspielen als Raster",

frontend/src/locales/de_DE/settings.json

@@ -43,6 +43,8 @@
   "config-file-not-mounted-title": "Configuration file not mounted!",
   "config-file-not-writable-desc": "The config.yml file is not writable. Any changes made to the configuration will not persist after the application restarts.",
   "config-file-not-writable-title": "Configuration file not writable!",
+  "config-file-parse-error-desc": "The config.yml file has a syntax error, so RomM is running with default settings and ignoring your configuration. Fix the error and restart RomM to apply your settings. Details: {error}",
+  "config-file-parse-error-title": "Configuration file could not be parsed!",
   "config-tab": "Config",
   "confirm-delete-mapping": "Do you confirm?",
   "continue-playing-as-grid": "Continue playing as grid",