From e7af2aaf9bb3b023e26bf368c17a210d9c734cc4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 12 May 2026 17:39:43 -0500 Subject: [PATCH] [core] Address review: secrets perms, mtime freshness, substitution regression, tests - save_compiled_config: write the cache with mode 0600 (the YAML contains resolved secrets) and always rewrite so mtime advances even when content is unchanged. Previously write_file_if_changed could leave mtime pinned behind a whitespace YAML edit, locking the fast path to 'stale'. - __main__: skip the fast path when -s overrides are passed. The cache was written against the previous substitution set; reusing it would silently ignore the new values. - Add tests for save_compiled_config (write + swallow-errors), the wizard-only sidecar fallback, and the substitution-skip branch. - Trim docstrings/comments throughout. --- esphome/__main__.py | 13 ++-- esphome/compiled_config.py | 55 +++++------------ esphome/storage_json.py | 13 ++-- esphome/writer.py | 5 +- tests/unit_tests/test_compiled_config.py | 78 +++++++++++++++++++++--- 5 files changed, 96 insertions(+), 68 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index d68dcffaf2..d5a9b69833 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -2419,15 +2419,12 @@ def run_esphome(argv): skip_external = args.command in ("logs", "clean") command_line_substitutions = dict(args.substitution) if args.substitution else {} - # Fast path for `upload` and `logs`: reuse the validated config the - # last compile cached on disk. Skips parse + schema validate + - # final-validate + external-component refresh, which is dead work - # for those subcommands -- the binary on disk is already known good - # and the caller already knows the device address. Falls back to a - # full `read_config()` when the cache is missing/stale/corrupt so a - # cold cache never produces a worse outcome than today. + # Fast path for upload/logs: reuse the validated-config cache the + # last compile wrote. Falls back to read_config when missing/stale. + # Skipped when -s overrides are passed, since the cache was written + # against the previous substitution set. config = None - if args.command in ("upload", "logs"): + if args.command in ("upload", "logs") and not command_line_substitutions: from esphome.compiled_config import load_compiled_config config = load_compiled_config(conf_path) diff --git a/esphome/compiled_config.py b/esphome/compiled_config.py index ac817bd906..92cbb7348a 100644 --- a/esphome/compiled_config.py +++ b/esphome/compiled_config.py @@ -1,20 +1,9 @@ -"""Validated-config cache for the ``upload`` / ``logs`` fast path. +"""Validated-config cache for the upload/logs fast path. -After every successful ``esphome compile``, the writer dumps the -validated config to ``/storage/.validated.yaml``. -The next ``esphome upload`` / ``esphome logs`` for that YAML reuses -the cache instead of re-running the full ``read_config()`` pipeline -(parse + schema validate + final-validate + external-component -refresh), which is dead work once the binary on disk is known good. - -The cache uses YAML (via ``yaml_util``) rather than JSON so all -esphome-specific tags (``!lambda``, ``!include``, ``ID`` instances, -paths) round-trip cleanly, and so the small ``StorageJSON`` sidecar -isn't bloated by configs that can grow past a megabyte. Staleness -is gated by an mtime check against the source YAML; the loader -falls back to ``None`` (and the caller to ``read_config()``) whenever -the cache is missing, stale, unparseable, or the companion -``StorageJSON`` sidecar can't be loaded. +compile dumps the validated config to /storage/.validated.yaml; +the next upload/logs for that YAML reuses it instead of running the full +read_config pipeline. YAML round-trip (yaml_util.dump/load_yaml) keeps +!lambda/!include/IDs/paths intact; mtime gates staleness. """ from __future__ import annotations @@ -23,7 +12,7 @@ import logging from pathlib import Path from esphome.core import CORE -from esphome.helpers import write_file_if_changed +from esphome.helpers import write_file from esphome.storage_json import StorageJSON, ext_storage_path from esphome.types import ConfigType @@ -44,21 +33,16 @@ def _cache_is_fresh(cache_path: Path, source_path: Path) -> bool: def save_compiled_config(config: ConfigType) -> None: - """Dump the validated config to its sidecar YAML file. + """Write the validated-config cache. Always-write so mtime stays fresh. - Called from the writer after every successful compile so the next - ``esphome upload`` / ``esphome logs`` for this YAML can skip - validation via :func:`load_compiled_config`. Failures here are - non-fatal: the worst case is that the fast path falls back to a - full ``read_config`` next time. + Mode 0600 because show_secrets=True resolves !secret inline. + Failures are non-fatal: the fast path falls back to read_config. """ from esphome import yaml_util try: - # show_secrets=True keeps the cache self-contained; it lives - # in the same trust zone as the rest of .esphome/storage/. rendered = yaml_util.dump(config, show_secrets=True) - write_file_if_changed(compiled_config_path(CORE.config_filename), rendered) + write_file(compiled_config_path(CORE.config_filename), rendered, private=True) except Exception as err: # pylint: disable=broad-except _LOGGER.debug("Skipping compiled config cache write: %s", err) @@ -66,15 +50,9 @@ def save_compiled_config(config: ConfigType) -> None: def load_compiled_config(conf_path: Path) -> ConfigType | None: """Load the cached validated config and apply storage metadata to CORE. - Single entry point for the ``upload`` / ``logs`` fast path: loads - the cache, applies the ``StorageJSON`` sidecar's platform / build - metadata to ``CORE``, and returns the config dict. Returns - ``None`` (so the caller falls back to ``read_config``) when the - cache is missing, older than the source YAML, unparseable, or - when the sidecar can't be loaded. The mtime check catches the - common "user edited the YAML and forgot to recompile" case; - deeper drift (an edited ``!include`` whose parent YAML mtime - didn't change) is the user's responsibility. + Returns None (caller falls back to read_config) when the cache is + missing, older than the source YAML, unparseable, or the sidecar + is incomplete. """ cache_path = compiled_config_path(conf_path.name) if not _cache_is_fresh(cache_path, conf_path): @@ -83,8 +61,6 @@ def load_compiled_config(conf_path: Path) -> ConfigType | None: from esphome import yaml_util try: - # clear_secrets=False keeps in-flight secret state intact; the - # cache is self-contained and resolves no !secret references. config = yaml_util.load_yaml(cache_path, clear_secrets=False) except Exception: # pylint: disable=broad-except return None @@ -92,9 +68,8 @@ def load_compiled_config(conf_path: Path) -> ConfigType | None: storage = StorageJSON.load(ext_storage_path(conf_path.name)) if storage is None: return None - # `apply_to_core` assumes the sidecar was written by `from_esphome_core` - # after a real compile, which always sets at least one of these. A - # wizard-only sidecar (no compile) can't drive upload/logs. + # apply_to_core assumes a real compile wrote the sidecar; wizard-only + # sidecars leave both of these unset and can't drive upload/logs. if not storage.core_platform and not storage.target_platform: return None storage.apply_to_core() diff --git a/esphome/storage_json.py b/esphome/storage_json.py index 0fd4a5cc0a..7d26b22f96 100644 --- a/esphome/storage_json.py +++ b/esphome/storage_json.py @@ -265,14 +265,11 @@ class StorageJSON: def apply_to_core(self) -> None: """Populate CORE with the metadata upload/logs read. - Inverse of :meth:`from_esphome_core`'s CORE→StorageJSON - projection. Keep paired -- a new CORE attribute the - ``upload`` / ``logs`` fast path needs has to be captured by - ``from_esphome_core`` too. Validators (``loaded_integrations``, - ``loaded_platforms``, ``friendly_name``) are deliberately not - restored: they're consumed by component validation, which the - fast path skips, and ``CORE.__init__`` already leaves them at - safe defaults. + Inverse of :meth:`from_esphome_core`. Keep paired -- a new + attribute upload/logs needs has to be captured there too. + Validator-only fields (loaded_integrations/platforms, + friendly_name) are skipped; the fast path doesn't run + validation and CORE.__init__ defaults them. """ CORE.name = self.name CORE.build_path = self.build_path diff --git a/esphome/writer.py b/esphome/writer.py index 54eb254b1b..cf04e4f8d2 100644 --- a/esphome/writer.py +++ b/esphome/writer.py @@ -111,10 +111,7 @@ def update_storage_json() -> None: old = StorageJSON.load(path) new = StorageJSON.from_esphome_core(CORE, old) - # Refresh the validated-config cache. `esphome upload` and - # `esphome logs` look for it next time they run and skip the full - # validation pipeline when it's present and fresh (mtime >= YAML - # mtime). Cheap to write, big win to read. + # Refresh the cache upload/logs read on the next call. if CORE.config is not None: save_compiled_config(CORE.config) diff --git a/tests/unit_tests/test_compiled_config.py b/tests/unit_tests/test_compiled_config.py index 5dc6df6944..1fda7bf5bd 100644 --- a/tests/unit_tests/test_compiled_config.py +++ b/tests/unit_tests/test_compiled_config.py @@ -10,7 +10,11 @@ from unittest.mock import patch import pytest from esphome.__main__ import run_esphome -from esphome.compiled_config import compiled_config_path, load_compiled_config +from esphome.compiled_config import ( + compiled_config_path, + load_compiled_config, + save_compiled_config, +) from esphome.const import ( CONF_API, CONF_ESPHOME, @@ -120,13 +124,6 @@ def test_load_compiled_config_happy_path(fresh_cache_files: Path) -> None: assert CORE.data[KEY_CORE][KEY_TARGET_PLATFORM] == "esp32" assert CORE.data[KEY_CORE][KEY_TARGET_FRAMEWORK] == "arduino" - # The validator-only attributes are deliberately left at their - # CORE.__init__ defaults. The fast path skips validation, so - # nothing reads these. - assert CORE.loaded_integrations == set() - assert CORE.loaded_platforms == set() - assert CORE.friendly_name is None - @pytest.mark.parametrize( "scenario", @@ -200,6 +197,24 @@ def test_run_esphome_upload_and_logs_fall_back_when_no_cache( mock_read.assert_called_once() +def test_run_esphome_upload_with_substitution_skips_cache( + fresh_cache_files: Path, +) -> None: + """`-s key value` forces a fresh validation -- the cache was written + against the prior substitution set, so reusing it would silently + ignore the override.""" + with ( + patch("esphome.__main__.read_config", return_value=None) as mock_read, + patch.dict( + "esphome.__main__.POST_CONFIG_ACTIONS", + {"upload": lambda args, config: 0}, + ), + ): + run_esphome(["esphome", "-s", "var", "val", "upload", str(fresh_cache_files)]) + + mock_read.assert_called_once() + + def test_run_esphome_compile_does_not_use_cache(fresh_cache_files: Path) -> None: """compile always re-validates -- it's what writes the cache.""" with ( @@ -212,3 +227,50 @@ def test_run_esphome_compile_does_not_use_cache(fresh_cache_files: Path) -> None run_esphome(["esphome", "compile", str(fresh_cache_files)]) mock_read.assert_called_once() + + +def test_save_compiled_config_writes_cache(tmp_path: Path) -> None: + """`save_compiled_config` writes the dumped YAML next to the sidecar.""" + CORE.config_path = tmp_path / "lite_test.yaml" + save_compiled_config({"esphome": {"name": "lite_test"}, "logger": {}}) + + cache_path = compiled_config_path("lite_test.yaml") + assert cache_path.is_file() + body = cache_path.read_text() + assert "name: lite_test" in body + assert "logger:" in body + + +def test_save_compiled_config_swallows_dump_errors( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Failures during the dump are non-fatal -- a bad cache just means + the next fast path falls back to read_config().""" + CORE.config_path = tmp_path / "lite_test.yaml" + with patch("esphome.yaml_util.dump", side_effect=RuntimeError("boom")): + save_compiled_config({"esphome": {"name": "lite_test"}}) + assert not compiled_config_path("lite_test.yaml").exists() + + +def test_load_compiled_config_rejects_wizard_only_sidecar(tmp_path: Path) -> None: + """A wizard-only sidecar (no compile -- no core_platform / target_platform) + can't drive upload/logs, so the fast path falls back.""" + yaml_path = tmp_path / "lite_test.yaml" + yaml_path.write_text("esphome:\n name: lite_test\n") + CORE.config_path = yaml_path + + storage_dir = tmp_path / ".esphome" / "storage" + storage_dir.mkdir(parents=True, exist_ok=True) + # StorageJSON with both core_platform and target_platform unset. + (storage_dir / "lite_test.yaml.json").write_text( + '{"storage_version": 1, "name": "lite_test", "friendly_name": null, ' + '"comment": null, "esphome_version": null, "src_version": 1, ' + '"address": null, "web_port": null, "esp_platform": null, ' + '"build_path": null, "firmware_bin_path": null, ' + '"loaded_integrations": [], "loaded_platforms": [], "no_mdns": false, ' + '"framework": null, "core_platform": null}' + ) + cache_path = _write_cache(storage_dir / "lite_test.yaml.validated.yaml") + _set_cache_mtime(cache_path, yaml_path, offset=5) + + assert load_compiled_config(yaml_path) is None