From 49898359bee1b53375f1fbece6aee432fd9e9dbb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 25 Jul 2026 16:46:04 -1000 Subject: [PATCH] [store_yaml] Simplify: packages records remote sources, single tree walker, dict-style test packages --- esphome/components/api/api_connection.cpp | 26 ++-- esphome/components/packages/__init__.py | 41 +++++- esphome/components/store_yaml/__init__.py | 138 +++++++----------- .../store_yaml/test.bk72xx-ard.yaml | 3 +- .../components/store_yaml/test.esp32-idf.yaml | 3 +- .../store_yaml/test.esp8266-ard.yaml | 3 +- tests/components/store_yaml/test.host.yaml | 5 +- .../store_yaml/test.ln882x-ard.yaml | 3 +- .../store_yaml/test.rp2040-ard.yaml | 3 +- .../store_yaml/test.rtl87xx-ard.yaml | 3 +- tests/integration/conftest.py | 8 +- tests/integration/test_store_yaml_recovery.py | 8 +- .../unit_tests/components/test_store_yaml.py | 49 +++---- 13 files changed, 144 insertions(+), 149 deletions(-) diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index fa8ad3d4d5..288d45db5f 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -1220,19 +1220,19 @@ void APIConnection::on_get_yaml_request() { #ifdef USE_ESP8266 this->store_yaml_chunk_buf_ = std::make_unique(STORE_YAML_CHUNK_SIZE); #endif - // All responses — including the single data-less done=true frame for a - // missing/empty blob — go through the loop-driven retry below, so a full - // TX buffer at request time can't strand the client without a terminal frame. + // All responses go through the loop-driven retry below, so a full TX + // buffer at request time can't strand the client without a terminal frame. this->store_yaml_pos_ = 0; this->try_send_store_yaml_(); } // Caller guarantees: store_yaml_pos_ != SIZE_MAX (a request is in flight). void APIConnection::try_send_store_yaml_() { + // Every component's setup() completes before the app loop services API + // messages, and codegen always embeds a non-empty blob, so the component + // is present and total > 0 whenever a request is serviced. auto *comp = store_yaml::global_store_yaml; - // comp is only null if the request arrived before the component's setup(); - // treat that like an empty blob and send just the terminal frame. - const size_t total = comp == nullptr ? 0 : comp->get_size(); + const size_t total = comp->get_size(); #ifdef USE_ESP8266 const size_t chunk_size = STORE_YAML_CHUNK_SIZE; @@ -1250,19 +1250,13 @@ void APIConnection::try_send_store_yaml_() { const size_t to_send = std::min(remaining, chunk_size); GetYamlResponse resp; - if (to_send != 0) { #ifdef USE_ESP8266 - progmem_memcpy(this->store_yaml_chunk_buf_.get(), comp->get_data() + this->store_yaml_pos_, to_send); - resp.set_data(this->store_yaml_chunk_buf_.get(), to_send); + progmem_memcpy(this->store_yaml_chunk_buf_.get(), comp->get_data() + this->store_yaml_pos_, to_send); + resp.set_data(this->store_yaml_chunk_buf_.get(), to_send); #else - resp.set_data(comp->get_data() + this->store_yaml_pos_, to_send); + resp.set_data(comp->get_data() + this->store_yaml_pos_, to_send); #endif - } else { - // Terminal frame for an empty blob: a valid empty pointer keeps the - // forced `data` field's memcpy well-defined. - resp.set_data(reinterpret_cast(""), 0); - } - if (this->store_yaml_pos_ == 0 && total != 0) { + if (this->store_yaml_pos_ == 0) { resp.total_size = static_cast(total); resp.encoding = StringRef(store_yaml::ENCODING); } diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index 44a1ebf36e..50395e4dbd 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -1,5 +1,6 @@ from collections import UserDict from collections.abc import Callable +from dataclasses import dataclass, field from functools import reduce from pathlib import Path from typing import Any @@ -33,9 +34,43 @@ from esphome.const import ( CONF_VARS, __version__ as ESPHOME_VERSION, ) -from esphome.core import EsphomeError +from esphome.core import CORE, EsphomeError DOMAIN = CONF_PACKAGES + + +@dataclass(frozen=True) +class RemotePackageSource: + """A remote source a package was fetched from while processing the config.""" + + url: str + ref: str | None + + +@dataclass +class PackagesData: + """Per-run package state, keyed under DOMAIN in CORE.data.""" + + remote_sources: list[RemotePackageSource] = field(default_factory=list) + + +def _get_data() -> PackagesData: + if DOMAIN not in CORE.data: + CORE.data[DOMAIN] = PackagesData() + return CORE.data[DOMAIN] + + +def get_remote_package_sources() -> list[RemotePackageSource]: + """Remote sources fetched while processing this config, in fetch order. + + Consumers (e.g. store_yaml) use this to tell which parts of the config + came from remote repositories rather than local files. + """ + if (data := CORE.data.get(DOMAIN)) is None: + return [] + return data.remote_sources + + # Guard against infinite include chains (e.g. A includes B includes A). MAX_INCLUDE_DEPTH = 20 @@ -189,6 +224,10 @@ def _process_remote_package(config: dict[str, Any]) -> dict[str, Any]: username=config.get(CONF_USERNAME), password=config.get(CONF_PASSWORD), ) + source = RemotePackageSource(config[CONF_URL], config.get(CONF_REF)) + remote_sources = _get_data().remote_sources + if source not in remote_sources: + remote_sources.append(source) files: list[dict[str, Any]] = [] # ``repo_root`` is the directory containing ``.git`` and must be passed diff --git a/esphome/components/store_yaml/__init__.py b/esphome/components/store_yaml/__init__.py index 40982cf5dc..8aa49b4c0c 100644 --- a/esphome/components/store_yaml/__init__.py +++ b/esphome/components/store_yaml/__init__.py @@ -3,12 +3,12 @@ from __future__ import annotations from collections.abc import Generator from dataclasses import dataclass import logging -import os from pathlib import Path import struct from esphome import yaml_util import esphome.codegen as cg +from esphome.components import packages from esphome.components.api import CONF_ENCRYPTION import esphome.config_validation as cv from esphome.const import CONF_API, CONF_ID, CONF_RAW_DATA_ID @@ -27,9 +27,6 @@ CODEOWNERS = ["@bdraco"] DEPENDENCIES = ["api"] CONF_INCLUDE_SECRETS = "include_secrets" -# Avoid an `_api:` substring in the key name so the integration-test harness -# (which naively str-replaces `api:` to inject a port directive) doesn't -# clobber configs that opt into this escape hatch. CONF_ALLOW_UNENCRYPTED = "allow_unencrypted" store_yaml_ns = cg.esphome_ns.namespace("store_yaml") @@ -120,14 +117,11 @@ def _gather_files( entries: list[tuple[str, Path]] = [] secret_rels: set[str] = set() for path in discovered.files: - try: - rel_str = path.relative_to(root).as_posix() - except ValueError: - # Outside the project root (e.g. ../common.yaml or a secrets file in - # $HOME). Use a relative path with ".." components instead of just - # the basename so the include graph is preserved and files from - # different directories with the same basename don't collide. - rel_str = os.path.relpath(path, root).replace(os.sep, "/") + # Files outside the project root (e.g. ../common.yaml or a secrets file + # in $HOME) keep their ".." components so the include graph is preserved + # and files from different directories with the same basename don't + # collide. + rel_str = path.relative_to(root, walk_up=True).as_posix() if path in discovered.secrets: secret_rels.add(rel_str) @@ -151,26 +145,6 @@ def _read_files_verbatim(entries: list[tuple[str, Path]]) -> list[tuple[str, byt return files -def _iter_sensitive_values( - node: object, path: tuple[str, ...] = () -) -> Generator[tuple[tuple[str, ...], str]]: - """Yield (config_path, value) for every cv.sensitive value in a config tree.""" - if isinstance(node, yaml_util.SensitiveStr): - yield path, str(node) - elif isinstance(node, dict): - for key, value in node.items(): - yield from _iter_sensitive_values(value, (*path, str(key))) - elif isinstance(node, (list, tuple)): - for item in node: - yield from _iter_sensitive_values(item, path) - - -@dataclass -class _SensitiveValue: - secret_name: str - config_path: str # dotted path, for warnings (never log the value itself) - - def _iter_scalars( node: object, path: tuple[str, ...] = () ) -> Generator[tuple[tuple[str, ...], object]]: @@ -185,12 +159,27 @@ def _iter_scalars( yield path, node +def _iter_sensitive_values(node: object) -> Generator[tuple[tuple[str, ...], str]]: + """Yield (config_path, value) for every cv.sensitive value in a config tree.""" + for path, value in _iter_scalars(node): + if isinstance(value, yaml_util.SensitiveStr): + yield path, str(value) + + +@dataclass +class _SensitiveValue: + secret_name: str + config_path: str # dotted path, for warnings (never log the value itself) + + def _warn_sensitive_collisions(sensitive: dict[str, _SensitiveValue]) -> None: """Redaction is value-keyed: any scalar equal to a sensitive value is rewritten to its `!secret` reference, including unrelated ones (e.g. `platform: esp32` when a password is literally "esp32"). Filling in a different value during recovery would then silently rewrite those unrelated scalars too — warn so the trap is documented, not silent.""" + if not sensitive: + return for path, value in _iter_scalars(CORE.config): if isinstance(value, yaml_util.SensitiveStr): continue @@ -256,43 +245,19 @@ def _uncaptured_note( return (UNCAPTURED_NOTE_PATH, "".join(parts).encode("utf-8")) -def _find_remote_packages(entries: list[tuple[str, Path]]) -> list[str]: - """Describe every `packages:` entry that pulls content from a remote source. +def _remote_package_descriptions() -> list[str]: + """Describe every remote source packages were fetched from. - Remote packages are downloaded during validation, which the fresh parse - used for discovery never reaches, so their files cannot be embedded. The - entry file still records the source, so the config is re-fetchable; this - only makes the gap visible instead of silent. + Remote packages are downloaded while the config is processed; the packages + component records each source, and this formats that record. Their files + cannot be embedded, but the entry file still records the package config, so + the config is re-fetchable; this only makes the gap visible instead of + silent. """ - remote: list[str] = [] - for _, path in entries: - try: - tree = yaml_util.load_yaml(path, clear_secrets=False) - except EsphomeError: - # Discovery already loaded this file once; a failure here would - # have been reported as a load_error and failed the build. - continue - if not isinstance(tree, dict): - continue - packages = tree.get("packages") - if isinstance(packages, dict): - candidates = packages.items() - elif isinstance(packages, list): - candidates = ((None, item) for item in packages) - else: - continue - for name, value in candidates: - desc = None - if isinstance(value, dict) and "url" in value: - url = value.get("url") - ref = value.get("ref") - desc = f"{url}@{ref}" if ref else str(url) - elif isinstance(value, str) and "//" in value: - # Shorthand form, e.g. `github://org/repo/file.yaml@main` - desc = value - if desc is not None: - remote.append(f"{name}: {desc}" if name is not None else desc) - return remote + return [ + f"{source.url}@{source.ref}" if source.ref else source.url + for source in packages.get_remote_package_sources() + ] def _build_secrets_skeleton(keys: set[str]) -> bytes: @@ -421,26 +386,23 @@ def unpack_envelope(blob: bytes) -> dict[str, bytes]: raise EsphomeError("envelope must start with EHY1 magic") pos = 4 files: dict[str, bytes] = {} - try: - (count,) = struct.unpack_from(" len(blob): - raise EsphomeError("truncated envelope") - path = blob[pos : pos + path_len].decode("utf-8") - if path.startswith(("/", "\\")) or (len(path) >= 2 and path[1] == ":"): - raise EsphomeError(f"envelope contains non-relative path: {path}") - pos += path_len - (content_len,) = struct.unpack_from(" len(blob): - raise EsphomeError("truncated envelope") - files[path] = blob[pos : pos + content_len] - pos += content_len - except struct.error as err: - raise EsphomeError(f"truncated envelope: {err}") from err + + def take(n: int) -> bytes: + nonlocal pos + if pos + n > len(blob): + raise EsphomeError("truncated envelope") + chunk = blob[pos : pos + n] + pos += n + return chunk + + (count,) = struct.unpack("= 2 and path[1] == ":"): + raise EsphomeError(f"envelope contains non-relative path: {path}") + (content_len,) = struct.unpack(" None: files = _read_files_verbatim(entries) else: files = _generate_redacted_files(entries, secret_rels) - remote_packages = _find_remote_packages(entries) + remote_packages = _remote_package_descriptions() if remote_packages: _LOGGER.warning( "store_yaml: %d package(s) come from remote sources and cannot be " diff --git a/tests/components/store_yaml/test.bk72xx-ard.yaml b/tests/components/store_yaml/test.bk72xx-ard.yaml index 979236dc7a..1de35f3cc3 100644 --- a/tests/components/store_yaml/test.bk72xx-ard.yaml +++ b/tests/components/store_yaml/test.bk72xx-ard.yaml @@ -2,4 +2,5 @@ wifi: ssid: MySSID password: password1 -<<: !include common.yaml +packages: + store_yaml: !include common.yaml diff --git a/tests/components/store_yaml/test.esp32-idf.yaml b/tests/components/store_yaml/test.esp32-idf.yaml index 979236dc7a..1de35f3cc3 100644 --- a/tests/components/store_yaml/test.esp32-idf.yaml +++ b/tests/components/store_yaml/test.esp32-idf.yaml @@ -2,4 +2,5 @@ wifi: ssid: MySSID password: password1 -<<: !include common.yaml +packages: + store_yaml: !include common.yaml diff --git a/tests/components/store_yaml/test.esp8266-ard.yaml b/tests/components/store_yaml/test.esp8266-ard.yaml index 979236dc7a..1de35f3cc3 100644 --- a/tests/components/store_yaml/test.esp8266-ard.yaml +++ b/tests/components/store_yaml/test.esp8266-ard.yaml @@ -2,4 +2,5 @@ wifi: ssid: MySSID password: password1 -<<: !include common.yaml +packages: + store_yaml: !include common.yaml diff --git a/tests/components/store_yaml/test.host.yaml b/tests/components/store_yaml/test.host.yaml index 1ecafeab77..6c822b28ee 100644 --- a/tests/components/store_yaml/test.host.yaml +++ b/tests/components/store_yaml/test.host.yaml @@ -1,3 +1,4 @@ -<<: !include common.yaml - network: + +packages: + store_yaml: !include common.yaml diff --git a/tests/components/store_yaml/test.ln882x-ard.yaml b/tests/components/store_yaml/test.ln882x-ard.yaml index 979236dc7a..1de35f3cc3 100644 --- a/tests/components/store_yaml/test.ln882x-ard.yaml +++ b/tests/components/store_yaml/test.ln882x-ard.yaml @@ -2,4 +2,5 @@ wifi: ssid: MySSID password: password1 -<<: !include common.yaml +packages: + store_yaml: !include common.yaml diff --git a/tests/components/store_yaml/test.rp2040-ard.yaml b/tests/components/store_yaml/test.rp2040-ard.yaml index 979236dc7a..1de35f3cc3 100644 --- a/tests/components/store_yaml/test.rp2040-ard.yaml +++ b/tests/components/store_yaml/test.rp2040-ard.yaml @@ -2,4 +2,5 @@ wifi: ssid: MySSID password: password1 -<<: !include common.yaml +packages: + store_yaml: !include common.yaml diff --git a/tests/components/store_yaml/test.rtl87xx-ard.yaml b/tests/components/store_yaml/test.rtl87xx-ard.yaml index 979236dc7a..1de35f3cc3 100644 --- a/tests/components/store_yaml/test.rtl87xx-ard.yaml +++ b/tests/components/store_yaml/test.rtl87xx-ard.yaml @@ -2,4 +2,5 @@ wifi: ssid: MySSID password: password1 -<<: !include common.yaml +packages: + store_yaml: !include common.yaml diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index a9c9e0686f..1f3da14a75 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -10,6 +10,7 @@ import logging import os from pathlib import Path import platform +import re import signal import socket import subprocess @@ -178,10 +179,9 @@ async def yaml_config(request: pytest.FixtureRequest, unused_tcp_port: int) -> s loop = asyncio.get_running_loop() content = await loop.run_in_executor(None, fixture_path.read_text) - # Replace the port in the config if it contains api section - if "api:" in content: - # Add port configuration after api: - content = content.replace("api:", f"api:\n port: {unused_tcp_port}") + # Replace the port in the config if it contains an api section. Anchored to + # the start of a line so keys that merely end in "api:" are left alone. + content = re.sub(r"(?m)^api:", f"api:\n port: {unused_tcp_port}", content) # Add debug build flags for integration tests to enable assertions if "esphome:" in content and "platformio_options:" not in content: diff --git a/tests/integration/test_store_yaml_recovery.py b/tests/integration/test_store_yaml_recovery.py index 8470dbcdde..7629dfb23e 100644 --- a/tests/integration/test_store_yaml_recovery.py +++ b/tests/integration/test_store_yaml_recovery.py @@ -19,12 +19,8 @@ import contextlib import pytest -try: - from compression import zstd # type: ignore[import-not-found] -except ImportError: - from backports import zstd # type: ignore[import-not-found, no-redef] - -from esphome.components.store_yaml import unpack_envelope +# The component resolves the stdlib-vs-backport zstd import once; reuse it. +from esphome.components.store_yaml import unpack_envelope, zstd from esphome.yaml_util import find_secret_references from .types import RunCompiledFunction diff --git a/tests/unit_tests/components/test_store_yaml.py b/tests/unit_tests/components/test_store_yaml.py index 4d281527d7..3f2b1a9fdc 100644 --- a/tests/unit_tests/components/test_store_yaml.py +++ b/tests/unit_tests/components/test_store_yaml.py @@ -8,16 +8,17 @@ from pathlib import Path import pytest from esphome import yaml_util +from esphome.components import packages from esphome.components.store_yaml import ( CONF_ALLOW_UNENCRYPTED, SECRETS_SKELETON_HEADER, UNCAPTURED_NOTE_PATH, _final_validate, - _find_remote_packages, _gather_files, _generate_redacted_files, _pack_envelope, _read_files_verbatim, + _remote_package_descriptions, _uncaptured_note, unpack_envelope, ) @@ -290,40 +291,36 @@ def test_uncaptured_note_lists_remote_packages() -> None: """Remote packages that can't be captured are recorded with their source so the user knows to re-fetch them.""" rel, content = _uncaptured_note( - [], ["base: https://github.com/org/repo@main", "github://org/repo/file.yaml"] + [], ["https://github.com/org/repo@main", "https://github.com/org/other"] ) assert rel == UNCAPTURED_NOTE_PATH text = content.decode() - assert "# base: https://github.com/org/repo@main" in text - assert "# github://org/repo/file.yaml" in text + assert "# https://github.com/org/repo@main" in text + assert "# https://github.com/org/other" in text -def test_find_remote_packages_detects_url_and_shorthand(project: Path) -> None: - """`packages:` entries with a url (dict or shorthand string) are reported; - local `!include` packages are not.""" - (project / "entry.yaml").write_text( - "packages:\n" - " base:\n" - " url: https://github.com/org/repo\n" - " ref: main\n" - " files: [common.yaml]\n" - " shorthand: github://org/repo/file.yaml@main\n" - " local: !include wifi.yaml\n" - "esphome:\n name: test\n" +def test_remote_package_descriptions_read_packages_record( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Remote sources recorded by the packages component during config + processing are formatted as url@ref (url alone when ref is absent).""" + monkeypatch.delitem(CORE.data, packages.DOMAIN, raising=False) + data = packages._get_data() + data.remote_sources.append( + packages.RemotePackageSource("https://github.com/org/repo", "main") ) - discovered = _sources(project, "entry.yaml", "wifi.yaml") - entries, _ = _gather_files(discovered) - remote = _find_remote_packages(entries) - assert remote == [ - "base: https://github.com/org/repo@main", - "shorthand: github://org/repo/file.yaml@main", + data.remote_sources.append( + packages.RemotePackageSource("https://github.com/org/other", None) + ) + assert _remote_package_descriptions() == [ + "https://github.com/org/repo@main", + "https://github.com/org/other", ] -def test_find_remote_packages_ignores_local_only(project: Path) -> None: - discovered = _sources(project, "entry.yaml", "wifi.yaml") - entries, _ = _gather_files(discovered) - assert _find_remote_packages(entries) == [] +def test_remote_package_descriptions_empty_without_packages() -> None: + CORE.data.pop(packages.DOMAIN, None) + assert _remote_package_descriptions() == [] def test_redacted_skips_empty_sensitive_values(project: Path) -> None: