Skip to content

Commit 65b9129

Browse files
committed
fix(internal): post-merge cleanups in config + settings_ui
- _read_backup_override_file now WARNING-logs unreadable / corrupt / non-dict override files instead of returning {} silently, mirroring _read_feature_flag_override_file. A user who saved a backup-config edit but hit a permission error or wrote a corrupt file now gets a log line instead of a silent revert to defaults. - _atomic_write_json cleans up the .tmp file on OSError so a failed write doesn't leave a partial sibling next to the real file (parity with _save_feature_flags' inline tmp/rename block). - Drop the duplicate inline _reset_global_settings import in _save_feature_flags — it's already at module scope after the merge.
1 parent cf18ad9 commit 65b9129

2 files changed

Lines changed: 58 additions & 14 deletions

File tree

src/ha_mcp/config.py

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -599,28 +599,66 @@ def get_backup_setting_origin(env_name: str) -> str:
599599
def _read_backup_override_file() -> dict[str, object]:
600600
"""Return the contents of the auto-backup override file, or ``{}``.
601601
602-
Malformed JSON / missing file / unreadable file all return ``{}``
603-
silently; the override file is best-effort and a corrupt file
604-
should not break Settings loading. Reads are not cached — callers
605-
(Settings construction, the GET endpoint) hit disk each time, which
606-
is fine for a small JSON file behind a singleton-cached Settings.
602+
Best-effort: a corrupt file MUST NOT break Settings loading. The
603+
failure modes split into two categories that need different
604+
treatment (mirrors ``_read_feature_flag_override_file``):
605+
606+
* **Silent**: file does not exist. The override layer is opt-in;
607+
a missing file is the normal "user has never edited" state and
608+
should not log.
609+
* **Loud (WARNING)**: file exists but is unreadable
610+
(``PermissionError``, broken filesystem) or unparseable
611+
(``JSONDecodeError``). The user toggled something, the UI said
612+
"Saved", and the value is silently being ignored. Without a
613+
log line they have no diagnostic; with one, the sidecar/server
614+
log tells them exactly what to fix.
615+
616+
Reads are not cached — callers (Settings construction, the GET
617+
endpoint) hit disk each time, which is fine for a small JSON file
618+
behind a singleton-cached Settings.
607619
"""
620+
import json
608621
from pathlib import Path
609622

610-
from .utils.data_paths import get_data_dir
623+
try:
624+
from .utils.data_paths import get_data_dir
611625

612-
path: Path = get_data_dir() / _BACKUP_OVERRIDE_FILENAME
626+
path: Path = get_data_dir() / _BACKUP_OVERRIDE_FILENAME
627+
except (RuntimeError, OSError):
628+
# Couldn't resolve the data dir at all — user has no override
629+
# file by definition. Silent.
630+
return {}
613631
try:
614632
raw = path.read_text()
615-
except (FileNotFoundError, OSError):
633+
except FileNotFoundError:
634+
return {}
635+
except OSError:
636+
logger.warning(
637+
"Auto-backup override file at %s exists but is unreadable; "
638+
"falling back to defaults. Check filesystem permissions.",
639+
path,
640+
exc_info=True,
641+
)
616642
return {}
617643
try:
618-
import json
619-
620644
data = json.loads(raw)
621645
except ValueError:
646+
logger.warning(
647+
"Auto-backup override file at %s is not valid JSON; "
648+
"falling back to defaults. Delete or fix the file to "
649+
"re-enable persisted toggles.",
650+
path,
651+
)
622652
return {}
623-
return data if isinstance(data, dict) else {}
653+
if not isinstance(data, dict):
654+
logger.warning(
655+
"Auto-backup override file at %s is not a JSON object "
656+
"(got %s); falling back to defaults.",
657+
path,
658+
type(data).__name__,
659+
)
660+
return {}
661+
return data
624662

625663

626664
def _apply_backup_overrides(settings: "Settings") -> None:

src/ha_mcp/settings_ui.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -278,10 +278,17 @@ def _atomic_write_json(path: Path, payload: dict[str, Any]) -> None:
278278
279279
Raises OSError on filesystem failure — same surface as the previous
280280
``path.write_text`` so caller try/except shapes don't need updating.
281+
On failure, the ``.tmp`` file is cleaned up so a previous partial
282+
write does not accumulate next to the real file.
281283
"""
282284
tmp = path.with_suffix(path.suffix + ".tmp")
283-
tmp.write_text(json.dumps(payload, indent=2))
284-
os.replace(str(tmp), str(path))
285+
try:
286+
tmp.write_text(json.dumps(payload, indent=2))
287+
os.replace(str(tmp), str(path))
288+
except OSError:
289+
with contextlib.suppress(FileNotFoundError, OSError):
290+
tmp.unlink()
291+
raise
285292

286293

287294
def save_tool_config(config: dict[str, Any]) -> bool:
@@ -1924,7 +1931,6 @@ async def _save_feature_flags(request: Request) -> JSONResponse:
19241931
_FEATURE_FLAG_INT_BOUNDS,
19251932
_FEATURE_FLAG_OVERRIDE_FILENAME,
19261933
FEATURE_FLAG_FIELDS,
1927-
_reset_global_settings,
19281934
get_feature_flag_origin,
19291935
)
19301936
from .utils.data_paths import get_data_dir

0 commit comments

Comments
 (0)