[store_yaml] Address review: retry terminal frame, fail on unredactable secrets, note uncaptured includes

esphome/components/api/api.proto

@@ -2758,7 +2758,7 @@ message GetYamlResponse {
   option (ifdef) = "USE_STORE_YAML";
 
   bytes data = 1 [(force) = true];
-  bool done = 2;
+  bool done = 2 [(force) = true];
   // Sent on the first chunk only — the client is expected to cache it.
   // (firmware bandwidth/flash is expensive; the client has gigabytes.)
   uint32 total_size = 3;

esphome/components/api/api_connection.cpp

@@ -1180,28 +1180,23 @@ static uint8_t store_yaml_chunk_buf[STORE_YAML_CHUNK_SIZE];
 #endif
 
 void APIConnection::on_get_yaml_request() {
-  auto *comp = store_yaml::global_store_yaml;
-  if (comp == nullptr || comp->get_size() == 0) {
-    // No component yet (request before setup()) or empty blob — send a single
-    // done=true response so the client always gets a terminal frame.
-    GetYamlResponse resp;
-    resp.done = true;
-    this->send_message(resp);
-    return;
-  }
+  // 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.
   this->store_yaml_pos_ = 0;
   this->try_send_store_yaml_();
 }
 
-// Caller guarantees: store_yaml_pos_ != SIZE_MAX (a stream is in progress), which
-// implies on_get_yaml_request() already saw the never-cleared global non-null.
+// Caller guarantees: store_yaml_pos_ != SIZE_MAX (a request is in flight).
 void APIConnection::try_send_store_yaml_() {
   auto *comp = store_yaml::global_store_yaml;
-  const size_t total = comp->get_size();
+  // 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();
 
   // Camera-style streaming: advance the position only after a successful send,
   // so a WOULD_BLOCK simply retries the same chunk on the next loop iteration.
-  while (this->store_yaml_pos_ < total) {
+  while (true) {
     if (!this->helper_->can_write_without_blocking())
       return;
 
@@ -1209,13 +1204,19 @@ void APIConnection::try_send_store_yaml_() {
     const size_t to_send = std::min(remaining, STORE_YAML_CHUNK_SIZE);
 
     GetYamlResponse resp;
+    if (to_send != 0) {
 #ifdef USE_ESP8266
-    progmem_memcpy(store_yaml_chunk_buf, comp->get_data() + this->store_yaml_pos_, to_send);
-    resp.set_data(store_yaml_chunk_buf, to_send);
+      progmem_memcpy(store_yaml_chunk_buf, comp->get_data() + this->store_yaml_pos_, to_send);
+      resp.set_data(store_yaml_chunk_buf, 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
-    if (this->store_yaml_pos_ == 0) {
+    } 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<const uint8_t *>(""), 0);
+    }
+    if (this->store_yaml_pos_ == 0 && total != 0) {
       resp.total_size = static_cast<uint32_t>(total);
       resp.encoding = StringRef(store_yaml::ENCODING);
     }
@@ -1225,9 +1226,11 @@ void APIConnection::try_send_store_yaml_() {
       return;  // retry on next loop, pos unchanged
 
     this->store_yaml_pos_ += to_send;
+    if (resp.done)
+      break;
   }
 
-  // Reached end successfully — final response (with done=true) already sent above.
+  // Final response (with done=true) sent successfully.
   this->store_yaml_pos_ = std::numeric_limits<size_t>::max();
 }
 #endif

esphome/components/api/api_pb2.cpp

@@ -4167,7 +4167,8 @@ uint8_t *GetYamlResponse::encode(ProtoWriteBuffer &buffer PROTO_ENCODE_DEBUG_PAR
   ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 10);
   ProtoEncode::encode_varint_raw(pos PROTO_ENCODE_DEBUG_ARG, this->data_len_);
   ProtoEncode::encode_raw(pos PROTO_ENCODE_DEBUG_ARG, this->data_ptr_, this->data_len_);
-  ProtoEncode::encode_bool(pos PROTO_ENCODE_DEBUG_ARG, 2, this->done);
+  ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 16);
+  ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, this->done ? 0x01 : 0x00);
   ProtoEncode::encode_uint32(pos PROTO_ENCODE_DEBUG_ARG, 3, this->total_size);
   ProtoEncode::encode_string(pos PROTO_ENCODE_DEBUG_ARG, 4, this->encoding);
   return pos;
@@ -4175,7 +4176,7 @@ uint8_t *GetYamlResponse::encode(ProtoWriteBuffer &buffer PROTO_ENCODE_DEBUG_PAR
 uint32_t GetYamlResponse::calculate_size() const {
   uint32_t size = 0;
   size += ProtoSize::calc_length_force(1, this->data_len_);
-  size += ProtoSize::calc_bool(1, this->done);
+  size += ProtoSize::calc_bool_force(1);
   size += ProtoSize::calc_uint32(1, this->total_size);
   size += !this->encoding.empty() ? 2 + this->encoding.size() : 0;
   return size;

esphome/components/store_yaml/__init__.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+from collections.abc import Generator
 from dataclasses import dataclass
 import logging
 import os
@@ -150,7 +151,9 @@ def _read_files_verbatim(entries: list[tuple[str, Path]]) -> list[tuple[str, byt
     return files
 
 
-def _iter_sensitive_values(node: object, path: tuple[str, ...] = ()):
+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)
@@ -201,7 +204,7 @@ def _build_secrets_skeleton(keys: set[str]) -> bytes:
 
 
 def _generate_redacted_files(
-    entries: list[tuple[str, Path]], secret_rels: set[str]
+    entries: list[tuple[str, Path]], secret_rels: set[str], unresolved: list[str]
 ) -> list[tuple[str, bytes]]:
     """Re-generate each captured file from its parse tree with cv.sensitive
     values emitted as `!secret <name>` references, and replace secrets files
@@ -228,17 +231,34 @@ def _generate_redacted_files(
     for text in texts.values():
         skeleton_keys |= yaml_util.find_secret_references(text)
 
-    for value, info in sensitive.items():
-        # After the context manager exits, only values loaded through a real
-        # `!secret` are still registered — those legitimately never appear
-        # inline, so only warn for inline values that were never swapped.
-        if yaml_util.is_secret(value) is None and info.secret_name not in skeleton_keys:
-            _LOGGER.warning(
-                "store_yaml: could not locate the sensitive value of '%s' in the "
-                "source YAML (built via substitutions?); it may still be "
-                "embedded verbatim",
-                info.config_path,
-            )
+    # After the context manager exits, only values loaded through a real
+    # `!secret` are still registered — those legitimately never appear
+    # inline. An inline value that was never swapped would ship verbatim in
+    # the blob, silently breaking the redaction promise — fail the build.
+    leaked = [
+        info.config_path
+        for value, info in sensitive.items()
+        if yaml_util.is_secret(value) is None and info.secret_name not in skeleton_keys
+    ]
+    if leaked:
+        raise EsphomeError(
+            "store_yaml: could not redact the sensitive value(s) of "
+            f"{', '.join(leaked)} (built via substitutions?). Reference them "
+            "with `!secret` in the YAML, or set `include_secrets: true` to "
+            "embed secrets deliberately."
+        )
+
+    if unresolved:
+        # Record the gap inside the recovered config itself, not just in a
+        # compile-time log line: substitution-pathed includes can't be
+        # captured, so the user must restore those files manually.
+        entry_rel = entries[0][0]
+        texts[entry_rel] = (
+            "# store_yaml: the following !include paths use substitutions and\n"
+            "# could not be captured; restore these files manually:\n"
+            + "".join(f"#   {inc}\n" for inc in unresolved)
+            + texts[entry_rel]
+        )
 
     skeleton = _build_secrets_skeleton(skeleton_keys)
     result = [
@@ -316,7 +336,7 @@ async def to_code(config: ConfigType) -> None:
     if config[CONF_INCLUDE_SECRETS]:
         files = _read_files_verbatim(entries)
     else:
-        files = _generate_redacted_files(entries, secret_rels)
+        files = _generate_redacted_files(entries, secret_rels, discovered.unresolved)
     envelope = _pack_envelope(files)
     compressed = zstd.compress(envelope, level=ZSTD_LEVEL)
 

tests/unit_tests/components/test_store_yaml.py

@@ -48,8 +48,8 @@ def _sources(
 
 
 def _gather_redacted(discovered: DiscoveredYamlFiles) -> dict[str, bytes]:
-    files, secret_rels = _gather_files(discovered)
-    return dict(_generate_redacted_files(files, secret_rels))
+    entries, secret_rels = _gather_files(discovered)
+    return dict(_generate_redacted_files(entries, secret_rels, discovered.unresolved))
 
 
 # ---------------------------------------------------------------------------
@@ -250,34 +250,37 @@ def test_redacted_reuses_existing_secret_name_for_duplicated_value(
     assert files["wifi.yaml"] == b"password: !secret 'api_key'\n"
 
 
-def test_redacted_warns_when_value_not_locatable(
-    project: Path, caplog: pytest.LogCaptureFixture
-) -> None:
+def test_redacted_raises_when_value_not_locatable(project: Path) -> None:
     """A sensitive value that never appears as a whole scalar (e.g. composed
-    via substitutions) produces a warning naming the config path, not the value."""
+    via substitutions) would ship verbatim — fail the build, naming the config
+    path but never the value."""
     CORE.config = {"wifi": [{"password": SensitiveStr("not_in_any_file")}]}
     discovered = _sources(project, "wifi.yaml")
-    with caplog.at_level("WARNING", logger="esphome.components.store_yaml"):
+    with pytest.raises(EsphomeError, match="wifi.password") as err:
         _gather_redacted(discovered)
-    assert any(
-        "wifi.password" in r.message and "could not locate" in r.message
-        for r in caplog.records
-    )
-    assert not any("not_in_any_file" in r.message for r in caplog.records)
+    assert "not_in_any_file" not in str(err.value)
 
 
-def test_redacted_does_not_warn_for_secret_only_values(
-    project: Path, caplog: pytest.LogCaptureFixture
-) -> None:
+def test_redacted_accepts_secret_only_values(project: Path) -> None:
     """A value that only exists via `!secret` legitimately never appears inline."""
     CORE.config = {"api": {"encryption": {"key": SensitiveStr("SUPER_SECRET")}}}
     yaml_util._SECRET_VALUES["SUPER_SECRET"] = "api_key"
     discovered = _sources(
         project, "entry.yaml", "secrets.yaml", secrets=("secrets.yaml",)
     )
-    with caplog.at_level("WARNING", logger="esphome.components.store_yaml"):
-        _gather_redacted(discovered)
-    assert not any("could not locate" in r.message for r in caplog.records)
+    files = _gather_redacted(discovered)
+    assert 'api_key: ""' in files["secrets.yaml"].decode()
+
+
+def test_redacted_records_unresolved_includes_in_entry_file(project: Path) -> None:
+    """Substitution-pathed includes that can't be captured are noted inside the
+    recovered entry file, not just in a compile-time log line."""
+    discovered = _sources(project, "entry.yaml", "secrets.yaml")
+    discovered.unresolved.append("${board}.yaml")
+    files = _gather_redacted(discovered)
+    text = files["entry.yaml"].decode()
+    assert text.startswith("# store_yaml: the following !include paths")
+    assert "#   ${board}.yaml" in text
 
 
 def test_redacted_skips_empty_sensitive_values(project: Path) -> None: