test+fix: address Patch76 post-homeassistant-ai#1431 review (narrow except, 6-tool e2e, abs-path test)

Patch76's homeassistant-ai#1431-aware re-review (verdict: mechanism "solid... ready").
Three non-blocking items addressed:

1. Narrow the settings-lookup except (util_helpers.py). build_skill_content
   and attach_skill_content wrapped get_global_settings() in a bare
   `except Exception`, masking programming bugs (AttributeError/ImportError)
   the same as a genuine config issue. Narrowed both to
   `except ValidationError` (the realistic Settings() config-load failure
   named in the existing comment) so real bugs now surface, per the repo's
   narrow-except convention. Also flipped the attach_skill_content fallback
   from `master_on = True` to `False`: on a settings-fetch failure we can't
   know the master state, so suppress the vendor-missing warning rather
   than emit a misleading one whose true cause was the settings fetch.

2. e2e skill_content delivery now covers all six write tools:
   - test_skill_content_delivery.py: parametrized on/off coverage for
     script / scene / helper / dashboard (hint-is-first-key + canonical
     files on default; suppression on MandatoryBPS=False). Automation keeps
     its explicit tests incl. the BP-warning section-slice.
   - test_yaml_config.py: TestYamlConfigSkillContentDelivery (on/off) using
     the module's mcp_client_with_yaml_config fixture (ha_config_set_yaml is
     feature-flag + component gated, can't share the automation-dir fixtures).
   The six tools have distinct success-return shapes where an ordering or
   wrong-dict bug would slip past the structural AST test.

3. test_skill_loader.py: test_resolve_skill_files_rejects_absolute_path
   (`/etc/passwd` passed directly) — guards the traversal check against a
   future refactor to a naive prefix match.

Test bookkeeping for #1: test_settings_load_raises_returns_empty now feeds a
real pydantic.ValidationError (via _make_validation_error()) instead of a
plain ValueError, and test_settings_load_propagates_unexpected_error pins
that a non-config error (AttributeError) is NOT swallowed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: kingpanther13

src/ha_mcp/tools/util_helpers.py

@@ -13,6 +13,7 @@
 from typing import Any, overload
 
 from fastmcp.exceptions import ToolError
+from pydantic import ValidationError
 
 from ..client.rest_client import (
     HomeAssistantAPIError,
@@ -2018,10 +2019,14 @@ def build_skill_content(
     # a successful write into a tool-level INTERNAL_ERROR (which would
     # then prompt the agent to retry, double-applying the mutation).
     # Silent degrade to "no skill_content" is the documented contract.
+    # Narrowed to ValidationError (the realistic config-load failure from
+    # Settings()) so genuine bugs (AttributeError/ImportError/etc.) still
+    # surface instead of being masked on every write — per the repo's
+    # narrow-except convention.
     try:
         if not get_global_settings().enable_mandatory_bps:
             return {}
-    except Exception:
+    except ValidationError:
         logger.warning("skill_content settings lookup failed; omitting", exc_info=True)
         return {}
 
@@ -2146,10 +2151,14 @@ def attach_skill_content(
     #    Benign — return silently.
     # 3. Something was requested but the vendor submodule is missing.
     #    Degraded — append a warning so operators notice.
+    # Narrowed to ValidationError (see build_skill_content). On a settings
+    # lookup failure we can't know the master state, so default master_on
+    # to False — that suppresses the vendor-missing warning rather than
+    # emitting a misleading one whose real cause was the settings fetch.
     try:
         master_on = get_global_settings().enable_mandatory_bps
-    except Exception:
-        master_on = True
+    except ValidationError:
+        master_on = False
     requested_anything = MandatoryBPS or referenced_files
     if master_on and requested_anything and get_skills_dir() is None:
         response.setdefault("warnings", []).append(_SKILLS_VENDOR_MISSING_WARNING)

tests/src/e2e/workflows/automation/test_skill_content_delivery.py

@@ -175,3 +175,128 @@ async def test_bp_warning_auto_embeds_only_relevant_section(
         assert section_body.lstrip().startswith("##"), (
             "section body should start with a markdown heading"
         )
+
+    # ------------------------------------------------------------------
+    # Coverage across the other write tools (script / scene / helper /
+    # dashboard). Automation is covered explicitly above; yaml — which is
+    # feature-flagged and custom-component-gated — is covered in
+    # tests/src/e2e/workflows/filesystem/test_yaml_config.py, which owns
+    # the mcp_client_with_yaml_config fixture. Together these exercise all
+    # six write tools that expose the MandatoryBPS parameter, each of
+    # which has a distinct success-return shape (spread dicts, helper
+    # subentry paths, dashboard metadata) where an ordering or wrong-dict
+    # bug would slip past the structural AST test.
+    # ------------------------------------------------------------------
+
+    def _build_create(self, tool: str, light: str, suffix: str):
+        """Return (tool_name, args, expected_skill_substrings) for a
+        minimal valid create on each write tool."""
+        if tool == "script":
+            return (
+                "ha_config_set_script",
+                {
+                    "script_id": f"e2e_skill_script_{suffix}",
+                    "config": {
+                        "alias": f"E2E Skill Script {suffix}",
+                        "sequence": [
+                            {
+                                "service": "light.turn_on",
+                                "target": {"entity_id": light},
+                            }
+                        ],
+                    },
+                },
+                ["automation-patterns.md", "template-guidelines.md"],
+            )
+        if tool == "scene":
+            return (
+                "ha_config_set_scene",
+                {
+                    "scene_id": f"e2e_skill_scene_{suffix}",
+                    "config": {
+                        "name": f"E2E Skill Scene {suffix}",
+                        "entities": {light: {"state": "on"}},
+                    },
+                },
+                ["SKILL.md"],
+            )
+        if tool == "helper":
+            return (
+                "ha_config_set_helper",
+                {
+                    "helper_type": "input_boolean",
+                    "name": f"E2E Skill Helper {suffix}",
+                },
+                ["helper-selection.md"],
+            )
+        if tool == "dashboard":
+            return (
+                "ha_config_set_dashboard",
+                {
+                    "url_path": f"e2e-skill-dashboard-{suffix}",
+                    "title": f"E2E Skill Dash {suffix}",
+                    "config": {"views": [{"title": "V", "cards": []}]},
+                },
+                ["dashboard-guide.md", "dashboard-cards.md"],
+            )
+        raise AssertionError(f"unknown tool {tool}")
+
+    def _track(self, cleanup_tracker, tool: str, args: dict, result: dict) -> None:
+        """Best-effort cleanup registration for the created entity."""
+        entity_id = result.get("entity_id")
+        if tool == "script":
+            cleanup_tracker.track("script", entity_id or f"script.{args['script_id']}")
+        elif tool == "scene":
+            cleanup_tracker.track("scene", entity_id or f"scene.{args['scene_id']}")
+        elif tool == "helper":
+            if entity_id:
+                cleanup_tracker.track("input_boolean", entity_id)
+        elif tool == "dashboard":
+            cleanup_tracker.track("dashboard", args["url_path"])
+
+    @pytest.mark.parametrize("tool", ["script", "scene", "helper", "dashboard"])
+    async def test_default_on_attaches_skill_content_all_tools(
+        self, mcp_client, cleanup_tracker, tool
+    ):
+        """Default MandatoryBPS=True attaches canonical skill_content with
+        the hint as the FIRST response key, for every write tool — not
+        just automation."""
+        light = await self._find_test_light_entity(mcp_client)
+        tool_name, args, expected = self._build_create(tool, light, "on")
+        result = await safe_call_tool(mcp_client, tool_name, args)
+        assert result.get("success"), f"{tool} create failed: {result}"
+        self._track(cleanup_tracker, tool, args, result)
+
+        keys = list(result.keys())
+        assert keys[0] == "skill_content_hint", (
+            f"{tool}: skill_content_hint must be the first response key, got {keys}"
+        )
+        skill_content = result.get("skill_content") or {}
+        assert skill_content, f"{tool}: skill_content must be non-empty"
+        joined = "\n".join(skill_content.keys())
+        for sub in expected:
+            assert sub in joined, (
+                f"{tool}: expected {sub!r} among skill_content keys "
+                f"{list(skill_content.keys())}"
+            )
+
+    @pytest.mark.parametrize("tool", ["script", "scene", "helper", "dashboard"])
+    async def test_mandatorybps_false_suppresses_all_tools(
+        self, mcp_client, cleanup_tracker, tool
+    ):
+        """Explicit MandatoryBPS=False suppresses both skill_content and the
+        hint, for every write tool."""
+        light = await self._find_test_light_entity(mcp_client)
+        tool_name, args, _ = self._build_create(tool, light, "off")
+        result = await safe_call_tool(
+            mcp_client, tool_name, {**args, "MandatoryBPS": False}
+        )
+        assert result.get("success"), f"{tool} create failed: {result}"
+        self._track(cleanup_tracker, tool, args, result)
+
+        assert "skill_content" not in result, (
+            f"{tool}: MandatoryBPS=False must suppress skill_content"
+        )
+        assert "skill_content_hint" not in result, (
+            f"{tool}: MandatoryBPS=False must suppress skill_content_hint"
+        )

tests/src/e2e/workflows/filesystem/test_yaml_config.py

@@ -1059,3 +1059,68 @@ async def test_write_read_delete_dashboard_yaml_file(
             {"path": path, "confirm": True},
         )
         assert delete_data.get("success") is True, delete_data
+
+
+@pytest.mark.filesystem
+class TestYamlConfigSkillContentDelivery:
+    """E2E: ha_config_set_yaml participates in the write-tool skill_content
+    delivery feature (#1182) — the sixth write tool exposing MandatoryBPS.
+
+    Lives here (not in workflows/automation/test_skill_content_delivery.py
+    with the other five tools) because ha_config_set_yaml is feature-flag
+    + custom-component gated and needs the mcp_client_with_yaml_config
+    fixture owned by this module.
+    """
+
+    _TEMPLATE_SENSOR = "- sensor:\n    - name: E2E Skill Sensor\n      state: 'ok'"
+
+    async def test_default_on_attaches_skill_content(self, mcp_client_with_yaml_config):
+        """MandatoryBPS defaults to True → template-guidelines.md ships
+        under skill_content with the hint as the first response key."""
+        result = await safe_call_tool(
+            mcp_client_with_yaml_config,
+            TOOL_NAME,
+            {
+                "yaml_path": "template",
+                "action": "add",
+                "content": self._TEMPLATE_SENSOR,
+                "file": "packages/_e2e_skill_bps_on.yaml",
+                "backup": False,
+            },
+        )
+        assert result.get("success") is True, f"yaml add failed: {result}"
+
+        keys = list(result.keys())
+        assert keys[0] == "skill_content_hint", (
+            f"skill_content_hint must be the first response key, got {keys}"
+        )
+        skill_content = result.get("skill_content") or {}
+        assert skill_content, "skill_content must be non-empty"
+        assert "template-guidelines.md" in "\n".join(skill_content.keys()), (
+            f"expected template-guidelines.md among {list(skill_content.keys())}"
+        )
+
+    async def test_mandatorybps_false_suppresses_skill_content(
+        self, mcp_client_with_yaml_config
+    ):
+        """Explicit MandatoryBPS=False suppresses both skill_content and the
+        hint on the yaml tool."""
+        result = await safe_call_tool(
+            mcp_client_with_yaml_config,
+            TOOL_NAME,
+            {
+                "yaml_path": "template",
+                "action": "add",
+                "content": self._TEMPLATE_SENSOR,
+                "file": "packages/_e2e_skill_bps_off.yaml",
+                "backup": False,
+                "MandatoryBPS": False,
+            },
+        )
+        assert result.get("success") is True, f"yaml add failed: {result}"
+        assert "skill_content" not in result, (
+            "MandatoryBPS=False must suppress skill_content"
+        )
+        assert "skill_content_hint" not in result, (
+            "MandatoryBPS=False must suppress skill_content_hint"
+        )

tests/src/unit/test_build_skill_content.py

@@ -11,6 +11,7 @@
 from unittest.mock import patch
 
 import pytest
+from pydantic import BaseModel, ValidationError
 
 from ha_mcp.tools.util_helpers import (
     _SKILL_CONTENT_OPTOUT_HINT,
@@ -23,6 +24,21 @@
 )
 
 
+def _make_validation_error() -> ValidationError:
+    """Construct a real ``pydantic.ValidationError`` — the realistic
+    config-load failure ``Settings()`` raises, which the narrowed
+    ``except ValidationError`` in build/attach_skill_content degrades on."""
+
+    class _M(BaseModel):
+        x: int
+
+    try:
+        _M(x="not-an-int")  # type: ignore[arg-type]
+    except ValidationError as exc:
+        return exc
+    raise AssertionError("expected ValidationError")  # pragma: no cover
+
+
 @pytest.fixture
 def fake_skills_dir(tmp_path: Path) -> Path:
     """Build a fake home-assistant-best-practices skill with reference files
@@ -374,20 +390,24 @@ def test_augment_tool_error_falls_through_on_non_json_body(self):
 
     def test_settings_load_raises_returns_empty(self, patched_get_skills_dir):
         """build_skill_content must silently degrade to {} when
-        get_global_settings() raises — the documented contract is "skill
-        content is opportunistic; never fail the surrounding write".
-        Without this defensive wrap, a settings-validation regression
-        would propagate, the outer except in each tool would re-map to
-        INTERNAL_ERROR, and the agent would retry an already-committed
-        mutation."""
+        get_global_settings() raises a config-validation error — the
+        documented contract is "skill content is opportunistic; never
+        fail the surrounding write". Without this guard, a settings-
+        validation regression would propagate, the outer except in each
+        tool would re-map to INTERNAL_ERROR, and the agent would retry
+        an already-committed mutation.
+
+        The except is narrowed to ``pydantic.ValidationError`` (the
+        realistic Settings() config-load failure), so a genuine config
+        problem degrades gracefully here."""
         # ``get_global_settings`` is imported INSIDE ``build_skill_content``
         # via ``from ..config import get_global_settings``, so the symbol
         # isn't bound in ``util_helpers``'s module namespace. Patch at the
         # source module instead — that's where the function-local import
         # resolves the name on every call.
         with patch(
             "ha_mcp.config.get_global_settings",
-            side_effect=ValueError("settings broken"),
+            side_effect=_make_validation_error(),
         ):
             result = build_skill_content(
                 MandatoryBPS=True,
@@ -396,6 +416,24 @@ def test_settings_load_raises_returns_empty(self, patched_get_skills_dir):
             )
         assert result == {}
 
+    def test_settings_load_propagates_unexpected_error(self, patched_get_skills_dir):
+        """A NON-config error (programming bug: AttributeError/ImportError/
+        etc.) must NOT be swallowed — the except is deliberately narrowed
+        to ValidationError so real bugs surface instead of being masked on
+        every write (Patch76 review; repo narrow-except convention)."""
+        with (
+            patch(
+                "ha_mcp.config.get_global_settings",
+                side_effect=AttributeError("real bug, not a config issue"),
+            ),
+            pytest.raises(AttributeError),
+        ):
+            build_skill_content(
+                MandatoryBPS=True,
+                canonical_files=("references/automation-patterns.md",),
+                referenced_files=None,
+            )
+
     def test_trailing_hash_resolves_to_whole_file(self, patched_get_skills_dir):
         """A trailing ``#`` with empty anchor (``"path#"``) must produce
         defined behaviour — currently resolves to the whole file under

tests/src/unit/test_skill_loader.py

@@ -103,6 +103,23 @@ def test_resolve_skill_files_rejects_traversal(fake_skills_dir: Path) -> None:
     assert result == {}
 
 
+def test_resolve_skill_files_rejects_absolute_path(fake_skills_dir: Path) -> None:
+    """An absolute path passed directly is refused.
+
+    ``skill_dir / "/etc/passwd"`` collapses to ``/etc/passwd`` under
+    pathlib (absolute RHS wins), which then fails the
+    ``is_relative_to(skill_root)`` guard. Guards against a future
+    refactor to a naive string-prefix check that would let an absolute
+    path slip through. Input is hardcoded today, so this is
+    defense-in-depth — but cheap to pin."""
+    result = skill_loader.resolve_skill_files(
+        fake_skills_dir,
+        "home-assistant-best-practices",
+        ["/etc/passwd"],
+    )
+    assert result == {}
+
+
 def test_resolve_skill_files_rejects_unknown_skill(fake_skills_dir: Path) -> None:
     """A skill name with no corresponding directory returns empty."""
     result = skill_loader.resolve_skill_files(