[packages] Improve error messages with include stack and fix missing path propagation (#15844)

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: J. Nick Koston <nick@home-assistant.io>
Co-authored-by: Jesse Hills <3060199+jesserockz@users.noreply.github.com>

esphome/components/packages/__init__.py

@@ -8,7 +8,9 @@ from typing import Any
 from esphome import git, yaml_util
 from esphome.components.substitutions import (
     ContextVars,
+    ErrList,
     push_context,
+    raise_first_undefined,
     resolve_include,
     resolve_substitutions_block,
     substitute,
@@ -360,12 +362,19 @@ def _substitute_package_definition(
     if isinstance(package_config, str) or (
         isinstance(package_config, dict) and is_remote_package(package_config)
     ):
+        # Collect undefined-variable errors (rather than raising strict) so the
+        # path walked through a remote-package dict is preserved and the user
+        # sees which field (url / path / ref / ...) referenced the undefined
+        # variable.
+        errors: ErrList = []
         package_config = substitute(
             item=package_config,
             path=[],
             parent_context=context_vars or ContextVars(),
             strict_undefined=False,
+            errors=errors,
         )
+        raise_first_undefined(errors, package_config, "package definition")
     return package_config
 
 

esphome/components/substitutions/__init__.py

@@ -30,6 +30,56 @@ ErrList = list[tuple[UndefinedError, SubstitutionPath, Any]]
 jinja = Jinja()
 
 
+def raise_first_undefined(
+    errors: ErrList,
+    source: Any,
+    context_label: str,
+) -> None:
+    """If *errors* is non-empty, raise ``cv.Invalid`` for the first undefined variable.
+
+    The raised error names the missing variable, the path walked into *source*
+    (for nested dicts, e.g. ``url`` or ``ref``), and the YAML source location
+    when *source* carries one. Only the first error is surfaced; the user will
+    re-run after fixing it and any remaining undefined variables will be
+    reported then.
+
+    ``context_label`` is the noun describing where the undefined variable
+    appeared (e.g. ``"package definition"``).
+    """
+    if not errors:
+        return
+    err, err_path, err_value = errors[0]
+    if len(errors) > 1:
+        # Log any further undefined variables so debug-level output covers
+        # the full set, even though only the first is surfaced to the user.
+        extras = ", ".join(
+            f"{e.message} at '{'->'.join(str(p) for p in p_path)}'"
+            for e, p_path, _ in errors[1:]
+        )
+        _LOGGER.debug("Additional undefined variables in %s: %s", context_label, extras)
+    # Prefer the location of the offending scalar (e.g. the `url:` value) over
+    # the enclosing package-definition dict so the message points at the exact
+    # line/column that carries the undefined variable.
+    location_node = (
+        err_value
+        if isinstance(err_value, ESPHomeDataBase) and err_value.esp_range is not None
+        else source
+    )
+    location = ""
+    if (
+        isinstance(location_node, ESPHomeDataBase)
+        and location_node.esp_range is not None
+    ):
+        mark = location_node.esp_range.start_mark
+        # DocumentLocation.line/column are 0-based (from the YAML Mark). Render
+        # as 1-based to match config.line_info() and editor line numbering.
+        location = f" (in {mark.document} {mark.line + 1}:{mark.column + 1})"
+    field = f" at '{'->'.join(str(p) for p in err_path)}'" if err_path else ""
+    raise cv.Invalid(
+        f"Undefined variable in {context_label}{field}: {err.message}{location}"
+    )
+
+
 def validate_substitution_key(value: Any) -> str:
     """Validate and normalize a substitution key, stripping a leading ``$`` if present."""
     value = cv.string(value)

tests/component_tests/packages/test_packages.py

@@ -2,18 +2,20 @@
 
 import logging
 from pathlib import Path
+import re
 from unittest.mock import MagicMock, patch
 
 import pytest
 
 from esphome.components.packages import (
     CONFIG_SCHEMA,
+    _substitute_package_definition,
     _walk_packages,
     do_packages_pass,
     is_package_definition,
     merge_packages,
 )
-from esphome.components.substitutions import do_substitution_pass
+from esphome.components.substitutions import ContextVars, do_substitution_pass
 import esphome.config as config_module
 from esphome.config import resolve_extend_remove
 from esphome.config_helpers import Extend, Remove
@@ -44,7 +46,7 @@ from esphome.const import (
 )
 from esphome.core import CORE
 from esphome.util import OrderedDict
-from esphome.yaml_util import IncludeFile, add_context
+from esphome.yaml_util import IncludeFile, add_context, load_yaml
 
 # Test strings
 TEST_DEVICE_NAME = "test_device_name"
@@ -1399,3 +1401,85 @@ def test_raw_config_contains_merged_esphome_from_package(tmp_path) -> None:
         "CORE.raw_config should contain esphome section after package merge"
     )
     assert CORE.raw_config[CONF_ESPHOME][CONF_NAME] == TEST_DEVICE_NAME
+
+
+# ---------------------------------------------------------------------------
+# _substitute_package_definition
+# ---------------------------------------------------------------------------
+
+
+def test_substitute_package_definition_local_dict_returned_unchanged() -> None:
+    """A plain local config dict is not substituted and is returned as-is."""
+    pkg = {CONF_WIFI: {CONF_SSID: "test"}}
+    result = _substitute_package_definition(pkg, ContextVars())
+    assert result is pkg
+
+
+def test_substitute_package_definition_string_resolved_with_context() -> None:
+    """A string package definition has its variables substituted."""
+    ctx = ContextVars({"variant": "esp32"})
+    result = _substitute_package_definition("device-${variant}.yaml", ctx)
+    assert result == "device-esp32.yaml"
+
+
+def test_substitute_package_definition_undefined_in_string() -> None:
+    """An undefined variable in a package URL string raises cv.Invalid."""
+    with pytest.raises(cv.Invalid, match="Undefined variable in package definition"):
+        _substitute_package_definition(
+            "github://org/repo/${undefined_var}/pkg.yaml", ContextVars()
+        )
+
+
+def test_substitute_package_definition_undefined_in_remote_dict_field() -> None:
+    """An undefined variable inside a remote-dict field names the offending field."""
+    with pytest.raises(cv.Invalid) as exc_info:
+        _substitute_package_definition(
+            {CONF_URL: "github://${typo}/repo"}, ContextVars()
+        )
+    err = str(exc_info.value)
+    assert "'typo' is undefined" in err
+    assert CONF_URL in err
+
+
+def test_substitute_package_definition_undefined_in_remote_dict_non_first_field() -> (
+    None
+):
+    """The field path joins correctly for non-first dict fields (e.g. ``ref``)."""
+    with pytest.raises(cv.Invalid) as exc_info:
+        _substitute_package_definition(
+            {
+                CONF_URL: "github://org/repo",
+                CONF_REF: "branch-${branch_typo}",
+            },
+            ContextVars(),
+        )
+    err = str(exc_info.value)
+    assert "'branch_typo' is undefined" in err
+    assert CONF_REF in err
+
+
+def test_substitute_package_definition_includes_source_location(tmp_path: Path) -> None:
+    """A package loaded from YAML surfaces file/line/col in the cv.Invalid message.
+
+    Line/column are rendered 1-based (matching config.line_info() and editor
+    line numbering) and point at the offending scalar, not the enclosing dict.
+    """
+    yaml_file = tmp_path / "main.yaml"
+    yaml_file.write_text(
+        "packages:\n  broken: github://org/repo/${undefined_var}/pkg.yaml\n"
+    )
+    config = load_yaml(yaml_file)
+    package_config = config[CONF_PACKAGES]["broken"]
+
+    with pytest.raises(cv.Invalid) as exc_info:
+        _substitute_package_definition(package_config, ContextVars())
+
+    err = str(exc_info.value)
+    assert "main.yaml" in err
+    # The offending value lives on line 2 (1-based). Column depends on the YAML
+    # loader, so we only pin line and check that a 1-based column is present.
+    match = re.search(r"main\.yaml (\d+):(\d+)", err)
+    assert match, err
+    line, col = int(match.group(1)), int(match.group(2))
+    assert line == 2, f"expected 1-based line 2, got {line} (err={err!r})"
+    assert col >= 1, f"expected 1-based column ≥ 1, got {col} (err={err!r})"

tests/unit_tests/test_substitutions.py

@@ -14,6 +14,7 @@ from esphome.components.packages import (
     do_packages_pass,
     merge_packages,
 )
+from esphome.components.substitutions.jinja import UndefinedError
 from esphome.config import resolve_extend_remove
 from esphome.config_helpers import Extend, merge_config
 import esphome.config_validation as cv
@@ -675,6 +676,39 @@ def test_include_filename_substitution_undefined_var(tmp_path: Path) -> None:
         substitutions.do_substitution_pass(config)
 
 
+def test_raise_first_undefined_logs_extras_at_debug(
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """Only the first undefined error is raised; extras are logged at debug."""
+    errors: substitutions.ErrList = [
+        (UndefinedError("'a' is undefined"), ["url"], None),
+        (UndefinedError("'b' is undefined"), ["ref"], None),
+        (UndefinedError("'c' is undefined"), ["path"], None),
+    ]
+
+    with (
+        caplog.at_level(logging.DEBUG, logger="esphome.components.substitutions"),
+        pytest.raises(cv.Invalid) as exc_info,
+    ):
+        substitutions.raise_first_undefined(errors, None, "package definition")
+
+    # First error is surfaced as the cv.Invalid message.
+    raised = str(exc_info.value)
+    assert "'a' is undefined" in raised
+    assert "'b' is undefined" not in raised
+    assert "'c' is undefined" not in raised
+
+    # Remaining errors are captured via debug logging for troubleshooting.
+    assert "Additional undefined variables in package definition" in caplog.text
+    assert "'b' is undefined at 'ref'" in caplog.text
+    assert "'c' is undefined at 'path'" in caplog.text
+
+
+def test_raise_first_undefined_noop_on_empty() -> None:
+    """An empty errors list is a no-op — no exception, no log."""
+    substitutions.raise_first_undefined([], None, "package definition")
+
+
 def test_do_substitution_pass_included_substitutions_must_be_mapping(
     tmp_path: Path,
 ) -> None: