[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

@@ -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)

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 ``<data_dir>/storage/<file>.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 <data_dir>/storage/<file>.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()

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

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)
 

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