Skip to content

Commit 00bdfbd

Browse files
feat: direct skills retrieval for write tools + improved best practice checker warnings with embedded skills responses (#1182) (#1448)
* feat(internal): add skill_loader utility with path-traversal guards Shared helper for resolving (skill, file) pairs from the bundled skills-vendor directory. Mirrors the symlink + path-traversal guards in ha_get_skill_guide's file read path, but silently skips bad files instead of raising so write tools can use it for response embedding without failing the operation. Refs #1182 * refactor(internal): server._get_skills_dir delegates to skill_loader Single source of truth for the skills-vendor path lookup. The new delegate matches the existing exists()-only check exactly — no behaviour change for ha_get_skill_guide or any other caller — and gives the write-tool include_skill parameter the same path resolver without holding a server reference. Also simplifies skill_loader._skills_dir_at to a bare existence check so the two paths stay byte-equivalent. Refs #1182 * refactor: best_practice_checker returns dataclass; warning text names 3 access routes check_automation_config / check_script_config now return BestPracticeCheckResult — a list[str] subclass that also exposes a .referenced_files set of skill file paths each warning points to. Existing call sites (and tests) that treat the return as a plain list keep working unchanged; new callers use .referenced_files to fetch file bodies via skill_loader and embed them in responses. Each warning's ' See ...' suffix now names all three skill-access routes when skills are enabled: See skill://... | call ha_get_skill_guide(skill=..., file=...) | or pass include_skill=True on this tool to receive the file in the next response automatically When skill_prefix=None (skills feature off server-wide), the suffix is suppressed entirely — matches historical behaviour because none of the three routes resolve when skills are off. Internal sweep: every warnings.append(... + _ref(...)) site replaced with _emit(warnings, ..., file_ref) so the referenced_files set stays in sync with the warning strings. Refs #1182 * feat: include_skill=True default on ha_config_set_automation Adds include_skill: bool = True parameter. When True (default), the response carries skill_content: {path: body} populated from the canonical mapping (automation-patterns.md + template-guidelines.md). Independent of include_skill, best-practice warnings auto-populate the same skill_content field with whichever reference files those warnings cited — so the LLM always gets the relevant guidance inline on the first wrong attempt, no follow-up call needed. bp_warnings storage typed as BestPracticeCheckResult so the referenced_files set is accessible to the response builder. Refs #1182 * refactor(internal): hoist build_skill_content to util_helpers Shared helper now lives in util_helpers.build_skill_content so each of the six write tools getting include_skill (automation, script, scene, helper, dashboard, yaml) imports the same implementation. Per-tool canonical mapping stays local to each module. Refs #1182 * feat: include_skill=True default on ha_config_set_script Mirrors the ha_config_set_automation pattern: response carries skill_content with automation-patterns.md + template-guidelines.md by default, auto-embeds referenced files on BP-checker warnings (works in both python_transform and full-replace modes). Refs #1182 * feat: include_skill=True default on ha_config_set_scene No scene-specific reference file exists. Returns the top-level home-assistant-best-practices SKILL.md doc by default, which links out to the relevant references for action/condition design (scenes share action syntax with automations/scripts). Refs #1182 * feat: include_skill=True default on ha_config_set_helper Adds include_skill: bool = True parameter. _attach_helper_skill() post-processes each of the 4 success return sites (simple-create, simple-update, config-store-update, and the flow-helper branch) to attach skill_content with references/helper-selection.md — the decision matrix for picking the right helper type (input_*, counter, timer, template, group, utility_meter, etc.). Refs #1182 * feat: include_skill=True default on ha_config_set_dashboard Returns dashboard-guide.md + dashboard-cards.md by default — layout patterns and card-type taxonomy. Single success-return site wraps via _attach_dashboard_skill helper. Refs #1182 * feat: include_skill=True default on ha_config_set_yaml Returns template-guidelines.md by default. YAML packages frequently include template sensors, command_line entities, and mqtt templates — exactly where template misuse causes the most subtle bugs. Refs #1182 * test: cover build_skill_content + per-tool canonical mappings Unit tests pin the shared assembly contract: canonical files attached when include_skill=True, suppressed when False; BP-warning referenced files always attach; canonical and referenced dedupe; missing canonical files silently skipped; missing skills-vendor degrades to no-op. Plus a pin per tool's canonical mapping so any future change to one of the six write tools' skill assignment is a deliberate edit caught by these assertions. Refs #1182 * feat: section-slice reactive auto-embed via #anchor (issue #1182 Q3) Best-practice warnings already point at specific markdown anchors (e.g. automation-patterns.md#native-conditions). Previously the auto-embed path shipped the whole 20 KB reference file. Now it ships just the matching section — typically 1-5 KB. Reactive content cost drops 5-15x with no information loss. Implementation: - skill_loader gets extract_section(body, anchor) — GH-style slugifier, fence-aware (a ``# yaml-comment`` inside ```yaml ... ``` no longer false-closes the surrounding section). - resolve_skill_files accepts "path#anchor" entries; reads each file at most once even when multiple sections are requested from it. - best_practice_checker._emit preserves the anchor in referenced_files instead of stripping to bare path. - util_helpers.build_skill_content dedupes bare-vs-anchored for the same file (full file in canonical supersedes a sliced section ref). Tests cover: anchor extraction, fence handling, ifthen-style slash slugs, missing anchor silently skipped, file-read dedup across multiple sections, bare-vs-anchored supersession at the right granularity. Refs #1182 * docs(internal): trim ha_config_set_automation docstring (-6.5 KB) Removed schema enumerations (REQUIRED FIELDS / OPTIONAL CONFIG FIELDS, TRIGGER/CONDITION/ACTION TYPES, blueprint vs regular type breakdown), extended worked examples (motion light, blueprint create/update), and the embedded PREFER NATIVE OVER TEMPLATES cheat-sheet. All of that now ships in the response under skill_content via automation-patterns.md + template-guidelines.md by default; the reactive checker additionally embeds the relevant section on warnings. Kept the action-verb summary, the when-NOT-to-use routing (scene/helper alternatives), the two-mode contract (config vs python_transform with config_hash), and the pointer to where the templating guidance lives now (skill_content / best_practice_warnings). 7,453 → 979 chars (-6,474). Catalog cost down by ~1.5K tokens for this single tool. The guidance is not lost — it arrives in the response on every write call (and just the relevant section on warnings) instead of riding along in the catalog forever. Refs #1182 * docs(internal): trim ha_config_set_script docstring (-3.9 KB) Mirrors the ha_config_set_automation trim. Removed schema field lists, 6 extended worked examples (delay, blink, parameters, blueprint create/update), and the embedded PREFER NATIVE OVER TEMPLATES cheat-sheet. All of that arrives in the response via skill_content (automation-patterns.md + template-guidelines.md by default; relevant section on BP warnings). Kept the action-verb summary, the when-NOT-to-use routing (use ha_config_set_automation for trigger-based work), the two-mode contract, the sequence-vs-use_blueprint requirement, and the pointer to where the templating guidance lives now. 4,762 → 869 chars (-3,893). Refs #1182 * docs(internal): trim ha_config_set_dashboard docstring (-3.0 KB) Removed python_transform worked examples, MODERN BEST PRACTICES list, DISCOVERING ENTITY IDs preamble, DOCUMENTATION cross-refs, and 4 verbose dashboard config examples (empty, sections, strategy, update). All of that ships via skill_content (dashboard-guide.md + dashboard-cards.md by default). Kept the two-mode contract, the index-shift caveat for chained python_transforms, the strategy-vs-custom note, the url_path naming rules, and pointers to entity-discovery tools. 4,058 → 1,047 chars (-3,011). Refs #1182 * docs(internal): trim ha_config_set_helper docstring (-2.0 KB) Removed the full SIMPLE/FLOW type enumerations (12 simple + 15 flow + config_subentry), the verbose Behavior notes preamble, and 4 worked examples (template sensor, group, tod, config_subentry). The helper-type decision matrix and worked examples now ship in skill_content via helper-selection.md by default. Kept the param-required-by-mode table, the action= disambiguation contract, the schema-discovery-on-first-error pattern, and the update-field-preservation note (all behavior contracts the LLM needs at call time, not reference docs). 3,282 → 1,225 chars (-2,057). Refs #1182 * docs(internal): trim ha_config_get_automation docstring (-0.3 KB) Examples and "use ha_get_skill_guide" pointer removed — single required parameter makes the call self-evident, and the matching set_automation call now ships skill_content automatically. Kept the return-shape contract (config_hash + automation_id resolution) the LLM needs at call time. 708 → 425 chars (-283). Refs #1182 * docs(internal): trim ha_config_get_script docstring (-0.4 KB) Examples and behavioral-parity-with-automations note removed; the prefix-strip and bare-key contract is now stated once, concisely. Refs #1182 * docs(internal): trim ha_config_get_scene + ha_config_set_scene docstrings Both shorted: examples removed, schema-shape reduced to one-liner, ha_get_skill_guide pointers dropped (skill_content ships in set_scene responses by default). get_scene: 455 → 220 (-235); set_scene: 1,322 → 850 (-472). Total -707. Refs #1182 * docs(internal): trim ha_config_list_helpers docstring (-0.8 KB) Removed the per-helper-type one-line descriptions (the same information is already enumerated by the Literal[...] on helper_type, visible to the LLM as the JSON-schema enum), the examples, and the skill_guide pointer. Kept the storage-vs-YAML scope note and the simple-vs-flow routing (use ha_search_entities for flow-based types). 1,318 → 538 chars (-780). Refs #1182 * docs(internal): trim ha_config_get_dashboard docstring (-0.9 KB) Examples (8 of them) and the search-workflow walk-through removed. Kept the three-mode contract (list/search/get) and the config_hash return-shape distinction (present in get/search, absent in list). 1,978 → 1,071 chars (-907). Refs #1182 * docs(internal): trim ha_call_service docstring (-0.8 KB) Removed 4 worked examples, the per-parameter Markdown bullet list (parameter descriptions are already on the Annotated[Field()] entries in the signature), and the skill_guide pointer. Kept the domain.service pattern, the omitted-entity_id targeting note, the return_response / wait contract, and the discovery-tool pointers. 1,593 → 711 chars (-882). Refs #1182 * docs(internal): trim ha_config_set_yaml docstring (-0.3 KB) Collapsed the per-tool routing bullet list into one sentence (the LLM already knows the alternatives from the catalog). Replaced the skill_guide pointer with a note that template-guidelines.md ships in this response by default. Kept the LAST RESORT warning, the YAML-only scope (allowed keys), the post_action reload-vs-restart note, and the comment/tag preservation guarantee. 1,240 → 1,036 chars (-204). Refs #1182 * fix: attach skill_content on ha_config_set_dashboard python_transform path The python_transform branch built and returned transform_result without calling _attach_dashboard_skill, so include_skill=True was a silent no-op on the recommended edit mode. Only the create/update branch wrapped. Refs #1182 * fix: attach skill_content on ha_config_set_helper config_subentry path The config_subentry branch returned set_config_subentry's response directly without calling _attach_helper_skill, so include_skill=True was a silent no-op on this fifth return site. The other four return sites (flow, simple-create, simple-update, config-update) already wrap. Refs #1182 * fix: attach skill_content on ha_config_set_scene python_transform path The python_transform branch built and returned response without ever calling build_skill_content, so include_skill=True was a silent no-op when editing an existing scene. Only the config-replacement branch wrapped. Refs #1182 * docs(internal): correct scene docstring + module comment (D1, D3) Three sites claimed scenes 'share action syntax with automations/scripts' or that SKILL.md links out to 'action/condition design' references. Factually wrong: scenes are pure state snapshots — only an entities dict, never triggers/conditions/actions. The validator at _validate_scene_config rejects list shape with exactly this distinction. Rewrote the module comment and include_skill Field description to state honestly what SKILL.md actually covers (entity-naming, safe-refactoring, helper-vs-template trade-offs) and why it's still relevant for scene authoring. Re-added the entities-dict shape with a concrete example to the public docstring — the prior trim dropped the only example, and no shipped skill file carries scene examples, so the LLM had no on-call-site reference for the first-write payload. Refs #1182 * docs(internal): drop stale helper count from ha_config_set_yaml (D2) Said '27 helper types' but Literal on set_helper.helper_type lists 28 (includes config_subentry, which the prior count missed). Switched to an enumeration of representative types instead of a count so future helper additions don't introduce drift again. Refs #1182 * docs(internal): align checker docstrings with skill_prefix=None impl (D4) Module docstring and check_automation_config arg doc both promised that when skill_prefix=None, the URI route would be omitted but the ha_get_skill_guide tool route and the include_skill parameter route would still be mentioned. The implementation (_three_route_suffix returns '' when skill_prefix falsy) suppresses the entire suffix — correctly, because skill_prefix=None signals skills are disabled server-wide, in which case none of the three routes can resolve. Updated both docstring sites to match the implementation rather than the other way around (the implementation is the right behaviour; the docstrings were aspirational). Refs #1182 * fix(internal): preserve referenced_files on copy/deepcopy (L1) BestPracticeCheckResult.__init__ resets referenced_files to an empty set, so the default list-subclass copy protocol (which re-enters __init__ with self as items) silently dropped the auto-embed payload. Added __copy__ and __deepcopy__ overrides that explicitly carry the set across the copy. No current caller uses copy/deepcopy on the result, so this is latent; fixing now avoids a debugging trap for any future consumer (e.g. a Transform layer that wants to forward the result). Refs #1182 * fix: slugify both sides in extract_section + cover edge cases (L2) The slugifier was applied to the heading from the file but NOT to the caller-provided anchor, so an asymmetric comparison silently missed on trailing whitespace, double-hash typos, mixed-case anchors, etc. All current _emit() sites pre-slugify, so the bug is latent — but the asymmetry was a footgun for any future change touching anchor strings. Now both sides are slugified before comparison. Tests added: - trailing/leading whitespace tolerance - mixed-case tolerance - ## / # typo absorption - last-heading section runs to EOF - first-match wins on heading-slug collision Refs #1182 * fix: surface skills-vendor-missing as a top-level warning (L3) Previously: include_skill=True on any of the 6 write tools silently omitted the skill_content field when the bundled skills-vendor submodule wasn't initialised. Asymmetric vs the read-side ha_get_skill_guide tool (server.py:200-208), which surfaces a structured degraded:True payload for the same condition. Operators on Docker / source installs who skipped --recurse-submodules got a silently degraded server. New shared helper util_helpers.attach_skill_content: attaches skill_content as before AND appends a top-level warnings[] entry when the caller requested skill content (include_skill=True OR referenced_files non-empty) AND the vendor is missing. Swept all six write tools to delegate through this helper: - automation/script/scene/yaml: inline `if skill_content: result[...]=` blocks replaced with attach_skill_content() calls. - helpers / dashboards: the per-tool _attach_*_skill wrappers now delegate to the shared helper (same single point for the degraded-warning behaviour). User-opted-out path (include_skill=False, no BP-warnings) stays silent — only requests-that-can't-be-fulfilled surface the warning. Refs #1182 * fix: escalate skill_loader log levels (O1) Every failure mode in _read_file_safely logged at DEBUG, which is below the default LOG_LEVEL=INFO — symlink rejects, path-traversal rejects, and missing files were all invisible in production. Combined with the write tools' silent-degrade contract, the operator had zero feedback on these conditions. Now WARNING for: - symlink rejection (security event) - path-traversal rejection (security event) - missing file / not-regular-file (caller bug or submodule drift) - OS errors during resolve/read Plus: missing anchor in extract_section was silently dropped with no log at all — now logs WARNING in resolve_skill_files since a missing anchor means a _emit() site typo or vendor submodule heading rename, both of which are real bugs. Refs #1182 * test: structural attach coverage + real-skill anchor resolution (T1, T2, T3) Two parametrized test classes that would have caught the bugs fixed earlier in this PR (the three missing attach_skill_content calls on python_transform / config_subentry branches) without needing the full fastmcp stack: * TestWriteToolAttachCoverage — AST-scans each of the six write tools, counts success-return paths, and asserts an attach-helper call exists for each. Pins the wrap-against-every-return-site contract structurally. * TestEveryEmittedAnchorResolves — extracts every literal anchor passed to _emit() in best_practice_checker, plus every per-tool canonical file mapping, and resolves each against the real bundled skills-vendor submodule. Catches submodule heading renames and _emit() typos that would otherwise produce silent empty skill_content with no test signal. Real-skill tests skip cleanly when the vendor submodule isn't initialised so a fresh clone doesn't fail collection. Refs #1182 * style: ruff SIM114 + PERF401 cleanup in skill_content_wiring test Combined the dual isinstance branches in _count_attach_calls into a single or-chained condition, and switched the inner-loop append in _canonical_files_mappings to list.extend with a generator. Behaviour-equivalent. Refs #1182 * docs(internal): restore full ha_call_service docstring from upstream ha_call_service has no skill_content, no best-practice checker, and no include_skill parameter — the original trim (commit 5694b3b9) plus the merge-conflict resolution dropped Basic Usage examples that nothing else replaces for this tool. Restored the full upstream post-#1447 docstring (Basic Usage + Key behavior + skill_guide pointer + Common patterns trailer) verbatim. Comment-analyzer agent flagged this as finding #4 earlier and I wrongly demoted it to LP4. Fixing now: this trim was asymmetric with the rest of the PR and the tool's high call frequency makes losing the docstring guidance especially costly. Refs #1182 * docs(internal): restore read-only tool docstrings — no skill_content path Five GET/LIST tools were trimmed under the same trim philosophy as the write tools, but they don't ship skill_content (only write tools do) and they're not best-practice-checker-gated either. The LLM has no alternate channel for the dropped guidance on these tools. Reverted the docstrings to their pre-trim verbatim shape: - ha_config_get_automation (was commit da9bc359) - ha_config_get_script (was commit b23b23e0) - ha_config_get_scene (was part of commit 46d49aa1; set_scene trim kept) - ha_config_list_helpers (was commit 639f8f0d) + flow-helper routing kept - ha_config_get_dashboard (was commit 7163e0c8) Addresses inline review comments on #1448 noting these trims were inappropriate for read-only tools that have no skill-fallback path. Refs #1182 * docs(internal): restore ha_config_set_automation docstring (move PREFER NATIVE to top) Addresses inline review on PR #1448. Per the reviewer's notes: - PREFER NATIVE OVER TEMPLATES block restored AND moved to the TOP of the docstring (was buried; a hallucinating LLM that's ignoring the skill content needs this front-and-centre to avoid reaching for Jinja first). - AUTOMATION TYPES + REQUIRED FIELDS (regular vs blueprint) restored — blueprint workflow has zero coverage in skill files. - OPTIONAL CONFIG FIELDS list restored — category / initial_state / variables aren't in skill content. - BASIC EXAMPLES + BLUEPRINT EXAMPLES restored — full worked configs for time-trigger, motion-light, update, blueprint create/update. - TRIGGER/CONDITION/ACTION TYPES enumeration restored — fills the zone-trigger / template-trigger / device-condition / parallel / delay gaps in automation-patterns.md. - TROUBLESHOOTING restored with the reviewer's requested clarification: ha_eval_template is for "IF you must use Jinja and have no native alternative" — frames it as a fallback, not a default. Kept the skill_content delivery note (auto-embed on warnings + canonical files via include_skill) since that's the new mechanism this PR adds. Refs #1182 * docs(internal): restore ha_config_set_script docstring (PREFER NATIVE at top, fields: + blueprints back) Per inline review: PREFER NATIVE OVER TEMPLATES moved to the top (reviewer marked line 434 'This absolutely needs to be left in, and needs to be at the very top.'). Restored sections: - SCRIPTS vs AUTOMATIONS routing kept high. - python_transform examples (delete/append/replace) restored — no skill file documents the syntax. - Required + optional config fields restored — script 'fields:' has zero skill coverage; its purpose (caller-supplied input with selector schema) is script-only. - Worked examples for delay, blink, parameterised backup, update, blueprint create/update restored — blueprint workflow has zero coverage in skill files. Refs #1182 * docs(internal): restore ha_config_set_scene docstring Per inline review (line 520): make sure worked example is in the docstring since SKILL.md (the only file set_scene ships) has no scene example. Restored the upstream WHEN TO USE / WHEN NOT TO USE / SCENE SHAPE / EXAMPLE structure verbatim. Kept the skill_content delivery note for the include_skill mechanism. Refs #1182 * docs(internal): restore ha_config_set_helper docstring Per inline review (line 2397): make sure most important + relevant info isn't excluded. Restored full upstream docstring covering: - SIMPLE vs FLOW vs CONFIG_SUBENTRY dispatch model with full type enumerations (helper-selection.md doesn't have a clean SIMPLE/FLOW table; 'trend' helper is in the FLOW list but missing from the skill file; subentry workflow has zero skill coverage). - All four required-params-by-mode rules. - Behavior notes (UPDATE preservation, action= disambiguation, silent-ignore + data_schema discovery, menu_options for menu-rooted types). - Worked tool-call examples for template / group / tod / config_subentry — these are first-call-payload-non-obvious shapes the LLM needs explicitly. The skill describes the menu flow in prose only; the tool-call shape was gone with no replacement. Refs #1182 * docs(internal): restore ha_config_set_dashboard docstring + clarify yaml-mode distinction Per inline review (lines 932, 1010, 559, 594, 576, 574): - python_transform examples (5 patterns: icon update, append, del, pattern loop, multi-op chain) restored — no skill file documents python_transform syntax for dashboards. - Strategy-based dashboard example + 'Take Control' caveat restored — strategy dashboards have zero skill coverage. - MODERN DASHBOARD BEST PRACTICES list restored. Dropped the stale '2024+' qualifier (it's 2026 now; this section is the current guidance, not a recent-add note). Reviewer flagged this should also be kept up to date in the skills repo separately. - title/icon/require_admin/show_in_sidebar update-alongside-config behavior note restored. - DISCOVERING ENTITY IDs section restored verbatim. - All 4 example shapes restored (empty / sections-tile / strategy / update). - NEW: STORAGE-MODE vs YAML-MODE DASHBOARDS section disambiguates the two YAML cases per reviewer's clarification at line 594 — dedicated .yaml file referenced from configuration.yaml, vs directly inlined under configuration.yaml's lovelace: key. Tool covers neither; pointer to ha_config_set_yaml for the latter. Refs #1182 * docs(internal): restore ha_config_set_yaml docstring (intact + skill_content note) Per inline review (line 182): leave docstring intact except the skill-access blurb. Restored full upstream LAST RESORT structure + routing bullets + intended-use scope + post_action / comment-tag / replace semantics. Helper-count corrected to 28 (was 27). Added the include_skill / skill_content delivery note for the new mechanism; kept the ha_get_skill_guide pointer for deeper guidance. Refs #1182 * docs(internal): minor wording revert to upstream verbatim on 3 docstrings Trimmed wording drift introduced during the partial restorations back to upstream-verbatim: - set_automation, set_script: 'before writing' (was 'BEFORE writing' — cosmetic case change reverted). - set_automation, set_script: integrated the skill_content auto-embed note as an additional sentence after the original 'will surface anything in a logic position' line rather than replacing that line — keeps the original wording intact. - set_script: restored 'Creates a new script or updates...' paragraph + 'fields:' wording + 'Create script with parameters:' header to upstream. SCRIPTS vs AUTOMATIONS moved back to its original position after Optional fields. - set_scene: SCENE SHAPE sentence reverted to upstream order ('Automations use a list of actions; scenes capture a snapshot of states as a dict'). Set_automation's PREFER NATIVE placement at the top, the ha_eval_template clarification, and the skill_content delivery notes are kept — those are the user-requested additions. Refs #1182 * feat: hide include_skill param from schema, teach opt-out via response hint BAT-confirmed regression: every LLM (Claude/GPT/Gemini variants) saw the default-on include_skill bool in the tool catalog and reflexively set it to False, defeating the proactive-skill-delivery design from PR #1448. Fix via FastMCP `exclude_args=["include_skill"]` on all six write-tool decorators. The JSON schema published to clients no longer lists the parameter, so LLMs can't pre-emptively disable what they can't see. The runtime still accepts include_skill=False when passed explicitly (Pydantic's default `additionalProperties: true`), so the opt-out path remains functional for callers who know about it. How the LLM learns about the (now-hidden) opt-out: when skill_content is actually delivered, `attach_skill_content` injects a sibling `skill_content_hint` field that names the parameter and tells the LLM to pass `include_skill=false` on subsequent calls if the content has already been received. The hint only ships alongside delivered content, so it can't be acted on before the model has seen what it's opting out of — eliminating the BAT-observed reflex-disable failure mode. Best-practice-checker route suffix becomes 2-route (skill:// URI + ha_get_skill_guide) — the previous 3rd route, "pass include_skill=True", is dropped because it pointed at a parameter no longer in the catalog. The auto-embed on warnings is unchanged; only the advertised routes are reduced. Tool docstrings drop the now-misleading "(see include_skill)" mentions on all six write tools — the param is hidden, so referring the LLM to it from the docstring was self-contradictory. Renamed `_three_route_suffix` → `_skill_route_suffix` and updated the module-level docs to describe the 2-route shape. Tests: - New `test_include_skill_is_hidden_from_tool_catalog` in test_skill_content_wiring.py — AST-pins exclude_args=["include_skill"] on every write tool's @tool / @mcp.tool decorator. - `test_happy_path_attaches_skill_content` now asserts the new `skill_content_hint` ships with delivered content. - `test_nothing_requested_is_silent` asserts the hint is absent when no content is delivered. - `TestThreeRouteWarningSuffix` updated: the include_skill-route test is replaced with `test_warning_does_not_mention_include_skill_param` (positive assertion that the hidden param is never named in warnings). Closes the BAT regression on PR #1448. Feature flag for the design is still issue #1182. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: hoist skill_content_hint to top of response, imperative wording BAT regression on the hidden-include_skill change: even Opus needed five tries to find and act on the opt-out hint when it trailed the ~25KB skill_content body; Sonnet/Haiku never found it. Cause is two-part — placement (LLMs process top-down and the hint was the last key after a giant payload) and voice (conditional "if ... then" reads as advisory, not actionable). Fix: reorder the response so skill_content_hint is the FIRST key and skill_content is the LAST, with the operation result fields (success, data, entity_id) sandwiched between. Reword the hint to imperative voice: "Pass `include_skill=false` on subsequent calls to this tool in this session to skip this content." Mutation is in place via response.clear() + re-insertion because the write-tool callers pass the dict by reference and expect their handle to keep pointing at the same response object. Test: new test_hint_appears_first_and_content_last in test_build_skill_content.py pins the key ordering contract and asserts the other response fields are preserved between hint and content. Existing test_happy_path_attaches_skill_content already asserts the hint value via the _SKILL_CONTENT_OPTOUT_HINT constant so the wording change is picked up there automatically. If smaller models still miss the hint after this, the fallback is to re-expose the param under an obscure name (visible-but-undescribed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: re-expose include_skill as opaque attach_skill_payload param BAT regression on the hidden-param + top-of-response-hint approach: no model ever opted out when the hint was visible at the top of the response, even after multiple calls in a session. Hidden visibility was working, but the LLM could not act on the hint because it couldn't see a callable param in the schema to set. Switching to the visible-but-opaque strategy: 1. Drop ``exclude_args=["attach_skill_payload"]`` from every write-tool decorator so the param appears in the published MCP schema again. 2. Rename ``include_skill`` → ``attach_skill_payload`` everywhere. The new name was chosen so the schema shows a default-True boolean with no obvious "this is the skill toggle, flip me to disable" semantic — "attach" reads as an internal data-shaping flag rather than a user-facing feature, "payload" suggests internal/advanced framing. 3. Strip the Pydantic Field description on the param across all 6 write tools (Field(default=True) only, no description=). The schema now publishes only the bare type + default, with no prose telling the model what the param does. 4. Remove the parameter's mention from every tool docstring (already done previously when the (see include_skill) parentheticals were dropped; the docstrings now describe only the by-default behaviour of skill_content, never the param). The opt-out hint (still imperative, still first key in response) becomes the ONLY surface that names the param and explains when to set it. A model that reflexively toggles unexplained schema parameters has nothing semantic to pull on; a model that needs to opt out reads the hint and now has a callable name plus a usage rule in the same sentence. Tests: - test_attach_skill_payload_is_visible_in_tool_catalog (NEW) — asserts the decorator does NOT carry exclude_args=["attach_skill_payload"], pinning the visibility decision so a future PR can't quietly re-hide the param and revert to the broken state. - test_attach_skill_payload_has_no_field_description (NEW) — asserts the Field on the param carries no description kwarg, pinning the opacity decision so a well-meaning future PR can't re-add a description that defeats the opacity. - All existing tests updated for the new param name (find/replace across test_build_skill_content.py, test_best_practice_checker.py, test_skill_content_wiring.py). Module docstrings (best_practice_checker.py, skill_loader.py, server.py) updated to reference the new name and the visible-opaque design rationale. If this fallback also fails BAT (small models don't pick up the hint), the next option is to leave behaviour alone but also embed the opt-out instruction inline at the START of the skill_content body itself so the LLM cannot avoid reading it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: rename attach_skill_payload to opaque enabled (BAT round 4) BAT round 3: Opus could still see ``attach_skill_payload`` had skill semantics in the name and reflex-disabled every try. The word "skill" in the param name was the giveaway — even with no description, the name itself told the model what it controlled. Renamed to ``enabled`` — completely generic, no domain semantic. A model scanning the schema sees a default-True boolean named "enabled" on a write tool and the natural read is "this gates whether the tool runs at all, don't touch it" — gatekeeper framing prevents the reflex-disable. The hint shipped inside the response when content is delivered clarifies that ``enabled=false`` only skips the skill_content payload, not the write itself. Hint text updated: "Pass ``enabled=false`` on subsequent calls to this tool in this session to skip this content." Still imperative, still first key in the response. Mechanical rename across 14 files (6 write tools + 2 internal helpers + util_helpers + best_practice_checker + skill_loader + server + 4 test files). No collisions with existing ``enabled`` usages (other occurrences are docstring text about automation enabled state, unrelated). If THIS still BATs poorly: - Opus picks up that "enabled=false" gates the write itself and stops using it → revisit with prior_skill_delivery (inverted polarity) reframe. - Models still don't opt out → fallback is to inline the directive at the top of the skill_content body itself (user's earlier ask), forcing the LLM to read it as part of the content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: rename enabled to MandatoryBPS, make REQUIRED (BAT round 5) BAT round 4: ``enabled`` worked as opacity for some models but Haiku just OMITTED the param entirely on every call (no default fallback to opt out, but also no signal it should opt in to skip content). Two changes this round: 1. Rename ``enabled`` → ``MandatoryBPS`` (Mandatory Best-Practice Skills). The capital-cased ``Mandatory`` prefix nudges models away from reflex-disabling — flipping a thing labelled "mandatory" to false reads as actively breaking something. Capital-cased to make the wordplay legible (Python convention is snake_case but the user's directive was explicit on the name). 2. Make the param REQUIRED at the schema level. Drop ``default=True`` from the Field, drop ``= True`` from the Python signature, and put a ``*,`` kwarg-only separator before it so Python's "non-default follows default" syntax rule doesn't fire. The MCP runtime always uses kwargs so no caller is broken. This forces the model to pass either true or false on every call — the previous "Haiku omits the param" failure mode is now impossible because FastMCP's Pydantic validator rejects the call without it. The default semantic remains "pass true if you have no opinion"; the response-side hint teaches when to pass false. Hint text auto-updated via the constant rename: "Pass ``MandatoryBPS=false`` on subsequent calls to this tool in this session to skip this content." Still imperative, still first key in the response. New structural test test_MandatoryBPS_is_required pins: * Field has no ``default`` / ``default_factory`` kwarg * Python signature has no default for MandatoryBPS (either no entry in kw_defaults for kwarg-only params, or non-positional placement) If Opus picks up that MandatoryBPS=false breaks something and refuses to ever set it false → revisit with reframed semantic (e.g. ``priorSkillReceived=false`` default which inverts polarity). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: revert MandatoryBPS to default=True (round 5 corrected) Misread the previous directive — user clarified the param should default to True (skill content delivered when omitted), not be required-without-default. With ``default=True``: - LLM passes true → content delivered - LLM passes false → content skipped - LLM omits → default kicks in, content delivered This matches the original safety semantic from when the param was ``include_skill``: omission is safe (content ships) rather than fail-loud. The opt-out hint in the response still teaches the LLM to pass ``MandatoryBPS=false`` when content is redundant. Changes: - Restored ``Field(default=True)`` on all 6 tool signatures. - Restored ``= True`` Python default on all 6 signatures. - Removed ``*,`` kwarg-only separator (no longer needed without the required-after-defaulted ordering problem). - Removed ``test_MandatoryBPS_is_required`` and its ``_MandatoryBPS_arg`` AST helper from test_skill_content_wiring.py — the required-contract no longer holds. - ``test_MandatoryBPS_has_no_field_description`` still pins the opacity contract (Field carries no description kwarg). - ``test_MandatoryBPS_is_visible_in_tool_catalog`` still pins visibility (no exclude_args). Hint constant unchanged: "Pass ``MandatoryBPS=false`` on subsequent calls to this tool in this session to skip this content." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(config): add ENABLE_MANDATORY_BPS master switch for skill_content delivery Adds an operator-controlled toggle for the write-tool skill_content feature (#1182) across all three configuration surfaces — env var, addon config, web UI — mirroring the enable_tool_search pattern. Setting sits ABOVE the per-call MandatoryBPS parameter as a server- wide master switch. When false, no skill_content goes out from any write tool regardless of the per-call param or BP-warning auto-embed. Default on — preserves the round-5 behaviour for existing operators. Wiring: - src/ha_mcp/config.py: ``enable_mandatory_bps`` Pydantic field with ``ENABLE_MANDATORY_BPS`` env alias, default True. Added to FEATURE_FLAG_FIELDS so the /api/settings/features endpoint advertises origin (env / addon / file / default) and validates writes the same way every other feature flag does. - src/ha_mcp/tools/util_helpers.py: master-switch check at the top of build_skill_content — returns empty dict when the setting is off, short-circuiting before the per-call canonical/referenced union and the I/O. - src/ha_mcp/settings_ui.py: FEATURE_META entry so the web UI surfaces label + help text matching the pattern of every other enable_* toggle. Add-on: - homeassistant-addon/config.yaml: ``enable_mandatory_bps: true`` in options + ``enable_mandatory_bps: bool?`` in schema. Visible in the stable addon Configuration tab. - homeassistant-addon-dev/config.yaml: same. - homeassistant-addon/translations/en.yaml: addon-tab name + description (kept in sync with web UI's help text per the comment above FEATURE_META). - homeassistant-addon-dev/translations/en.yaml: same. - homeassistant-addon/start.py: read raw value from /data/options.json with bool coercion (default True), emit ``ENABLE_MANDATORY_BPS`` env var alongside the other flags. Tests: - New ``test_master_switch_off_short_circuits`` in test_build_skill_content.py — patches get_global_settings to return a settings instance with enable_mandatory_bps=False and asserts build_skill_content returns empty even when the per-call MandatoryBPS=True or when referenced_files (BP-warning auto-embed) are supplied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: PR-toolkit findings — hint position, settings safety, UTF-8, stale docs Addresses real findings from the pr-review-toolkit batch run. Bug fixes: - build_skill_content + attach_skill_content now wrap get_global_settings in try/except → silently degrade to no skill_content. A cold-cache settings-load exception would otherwise bubble up, get re-mapped to INTERNAL_ERROR by the outer except-block of the write tool, and lead the agent to retry an already-committed mutation. - attach_skill_content's vendor-missing warning is suppressed when the master switch is off — the suppression cause is the operator config, not a missing submodule, so telling them to run `git submodule update --init` was misleading. - skill_loader._read_file_safely now catches UnicodeDecodeError separately from OSError. Invalid UTF-8 in a vendored skill file would otherwise propagate and fail a write the agent just committed. - ha_config_set_automation / _script / _scene config-update paths now call attach_skill_content AFTER building the outer return dict, so the hint-first response ordering survives. Previously the dict-spread ({"success": True, ..., **result}) pushed skill_content_hint to position 2-3 — the exact placement BAT showed small models can't find. dashboards / helpers / yaml / python_transform paths were already operating on the returned dict so they were unaffected. - homeassistant-addon/start.py now log_error's on an invalid (non-bool) enable_mandatory_bps value before falling back to default True, so the defensive coercion isn't silent. Stale comment/text fixes: - TestThreeRouteWarningSuffix → TestTwoRouteWarningSuffix; class docstring + inline comment + test docstring all stop claiming the param is "hidden via exclude_args" (it's visible). - test_build_skill_content's "hidden opt-out path" comment updated to "schema-visible-but-undescribed". - best_practice_checker.py module-docstring + _skill_route_suffix docstring no longer recite the param-design rationale verbatim; cross-reference util_helpers._SKILL_CONTENT_OPTOUT_HINT instead (eliminates the drift hazard between two copies). - tools_config_dashboards docstring's ha_config_set_yaml parenthetical now explicit that it only updates the registration entry, not the dashboard body in the referenced .yaml file. - util_helpers._SKILL_CONTENT_OPTOUT_HINT comment trimmed: no more five-round BAT history block in production code (per CLAUDE.md "don't reference fix history in code" — it rots and belongs in the PR description). One-paragraph "design rationale settled by BAT, don't tune casually" warning replaces the bullet list. Dead-code removal: - BestPracticeCheckResult.__copy__ / __deepcopy__ removed — zero callers in src or tests do copy.copy / copy.deepcopy on a result instance. Docstring updated to be honest that slicing / list() / copy.copy all drop the attribute and no call site exercises any of those paths. New tests: - test_master_off_with_vendor_missing_does_not_emit_warning — pins the suppression-cause-attribution fix above. - test_trailing_hash_resolves_to_whole_file — pins the (previously undefined) "references/foo.md#" trailing-empty-anchor behaviour so a refactor can't silently flip it. - test_enable_mandatory_bps_default_on + two parametrized coercion tests in tests/src/unit/test_config.py — pin Pydantic's bool accept/reject contract on the new env var. Skipped from the toolkit batch: - Loose structural attach-coverage inequality (would require fastmcp env to tighten with per-branch instance tests). - BestPracticeCheckResult dataclass refactor (works as-is; flagged as follow-up not blocker by type-design agent). - Duplicate import block in automations/scripts (isort-style nit; ruff passes; merging may not survive autoformat). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: auto-embed BP sections on errors + generic hint everywhere + skill_guide opt-out hint Closes the three design gaps surfaced by live BAT against the deployed PR. Before this commit, write-tool error responses dropped all skill_content — only success-path returns carried it, leaving the LLM without inline fix material on the exact code path where it needs it most (a write that just failed). Changes: 1. New helpers in util_helpers.py: - augment_error_dict_with_skill_content(error_dict, bp_warnings) mutates an error response in place to append the generic ha_get_skill_guide pointer to suggestions (idempotent) and, when bp_warnings has referenced_files, attach the matching section bodies under skill_content with skill_content_hint at the top. Canonical files NOT attached on errors (targeted section bodies are 1-5 KB; 25-37 KB canonical bundle would bloat errors without matching benefit). - augment_tool_error_with_skill_content(te, bp_warnings) wraps the dict mutator around a ToolError — used by each write tool's outer except handler. 2. Each of the 6 write tools' outer @tool method now wraps its body in: try: ... return response except ToolError as te: raise augment_tool_error_with_skill_content(te, bp_warnings) from None except Exception as e: error = exception_to_structured_error(..., raise_error=False) augment_error_dict_with_skill_content(error, bp_warnings) raise_tool_error(error) Six modifications instead of touching all 145 raise sites. The wrap captures every ToolError that bubbles to the outer handler, adds the generic hint, and embeds BP sections where bp_warnings has referenced_files. 3. ha_get_skill_guide Tier 3 (file content fetch) on the home-assistant-best-practices skill now prepends a skill_content_hint at the top of the response telling the LLM to pass MandatoryBPS=false on subsequent write-tool calls (avoids duplicate canonical delivery for smart clients that fetch skills proactively). Other skills (if any) unchanged. Tests added: - test_augment_error_adds_generic_hint_without_bp pins the no-BP-context case: every error gets the generic pointer. - test_augment_error_idempotent_on_re_raise pins that nested re-raise paths don't double-append the hint. - test_augment_error_embeds_bp_sections_when_referenced pins the with-BP case: section body inlined under skill_content with hint at top. Closes tasks #14, #15, #16. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(internal): prepend "MUST call ha_get_skill_guide first" on 6 write tools Front-loads the read-skills-then-write pattern on every write tool's docstring so an LLM scanning the tool catalog sees the directive before any of the substantive tool-specific guidance. Pairs with the existing ha_get_skill_guide Tier 3 response (commit 5bc31982) which prepends skill_content_hint telling the LLM to pass MandatoryBPS=false on subsequent write-tool calls — closing the read-then-write loop: 1. LLM sees write tool docstring → "MUST call ha_get_skill_guide first" 2. LLM calls ha_get_skill_guide → response top key is the opt-out hint 3. LLM calls write tool with MandatoryBPS=false → no duplicate content The single-line bare directive is deliberate (per maintainer request). The skill argument and which specific file to fetch are deferred to ha_get_skill_guide's own discovery flow (Tier 1 lists skills, Tier 2 lists files in a skill, Tier 3 reads content). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test+docs: address Patch76 PR review (P1-P3, P4 Option A, P6-P8) Test coverage additions: - test_augment_tool_error_wraps_dict_augmentation: pins the ToolError wrapper used by all 6 write tools' outer except handler. Decodes the JSON body, runs the dict augmentation (generic hint + section embed), re-encodes into a new ToolError. - test_augment_tool_error_falls_through_on_non_json_body: pins the defensive fall-through for non-JSON ToolError bodies. - test_settings_load_raises_returns_empty in test_build_skill_content: pins the broad-except graceful-degrade — get_global_settings() raising must short-circuit to {} so a settings-validation regression doesn't fail a write the agent already committed. - test_resolve_skill_files_oserror_during_resolve_returns_skipped in test_skill_loader: patches Path.resolve to raise OSError (e.g. ELOOP from a circular symlink), asserts silent-skip. - test_resolve_skill_files_invalid_utf8_returns_skipped in test_skill_loader: writes 0xff 0xfe bytes to a .md file, asserts the UnicodeDecodeError (subclass of ValueError, not OSError) is caught by the dedicated except clause and doesn't propagate. E2E test (tests/src/e2e/workflows/automation/test_skill_content_delivery.py): - test_default_mandatorybps_attaches_canonical_skill_content: real HA container + real FastMCP, asserts skill_content_hint is the FIRST key and skill_content contains the canonical files. - test_mandatorybps_false_suppresses_skill_content: explicit opt-out ships no skill_content / no hint. - test_bp_warning_auto_embeds_only_relevant_section: BP-checker fires on template-in-condition input + MandatoryBPS=False, response carries ONLY the section-anchored body (not the whole canonical file) — proves section-slicing works end-to-end through real FastMCP. Docstring tweak (P4 Option A, action-verb-first preserved): - All 6 write tools' first docstring line now reads "Create/Update <thing>. MUST call ha_get_skill_guide first." — satisfies the styleguide action-verb-first convention while keeping the BAT-tuned MUST directive on the same first line where catalog scanners see it. Previous structure put the MUST on its own first line above the action verb. Nits: - best_practice_checker.BestPracticeCheckResult: __slots__ added as one-line typo guard (prevents accidental attribute writes beyond referenced_files). - skill_loader.py: contract-stated comment replaces the "current _emit sites all pre-slugify" snapshot that would rot. - skill_loader.py module docstring: added caveat that top-level or near-EOF anchors return most of the file (the section runs to the next same/higher-level heading). Rejected: - P5 narrowing of `except Exception` around get_global_settings(). The broad except is deliberate graceful-degrade — narrowing to (OSError, ValidationError) would let a future AttributeError from Settings schema drift propagate and crash the write, which is the exact failure mode the broad except is designed to prevent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): patch get_global_settings at source + scope Path.resolve patch Two unit-test failures from 593bbefa, both caused by patch-target scope mistakes: 1. test_settings_load_raises_returns_empty was patching `ha_mcp.tools.util_helpers.get_global_settings` but the symbol isn't bound in that module's namespace — `build_skill_content` imports it function-locally via `from ..config import get_global_settings`. Patched at the source module instead. 2. test_resolve_skill_files_oserror_during_resolve_returns_skipped patched Path.resolve globally, which fired on the `skill_dir.resolve()` call in `resolve_skill_files` (line ~178) BEFORE reaching `_read_file_safely`. The outer call has no try/except so the OSError propagated and failed the test. Scoped the patch to only fire on `.md` suffixes, which is the only path that goes through `_read_file_safely`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test+fix: address Patch76 post-#1431 review (narrow except, 6-tool e2e, abs-path test) Patch76's #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> * chore(addon): restore dev addon version to upstream dev374 The merge left the dev addon config.yaml version at dev373 while upstream/master is at dev374, making the PR diff show a backwards version bump. The version line is release-pipeline-owned; restoring it to upstream's value zeroes the spurious diff so the PR only changes enable_mandatory_bps. No stable-version change (stable matches upstream at 7.6.0). Audit note (re #1486, "addon config isn't auto-synced between flavors"): verified enable_mandatory_bps is present on BOTH stable and dev addon config.yaml (options + schema) and translations, is NOT in BETA_FEATURE_FIELDS (so not beta-gated), and is written unconditionally in start.py — i.e. the skill_content feature is available on stable, not accidentally dev-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ebdc69b commit 00bdfbd

24 files changed

Lines changed: 3303 additions & 214 deletions

homeassistant-addon-dev/config.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ options:
3939
enable_custom_component_integration: false
4040
enable_code_mode: false
4141
enable_lite_docstrings: false
42+
enable_mandatory_bps: true
4243
enable_auto_backup: true
4344
auto_backup_throttle_minutes: 0
4445
auto_backup_retain_per_entity: 100
@@ -55,6 +56,7 @@ schema:
5556
enable_yaml_config_editing: bool?
5657
enable_code_mode: bool?
5758
enable_lite_docstrings: bool?
59+
enable_mandatory_bps: bool?
5860
enable_filesystem_tools: bool?
5961
enable_custom_component_integration: bool?
6062
enable_auto_backup: bool?

homeassistant-addon-dev/translations/en.yaml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,22 @@ configuration:
127127
features" toggle above (and in the web UI) to be on — otherwise
128128
this sub-flag is ignored at runtime regardless of its value
129129
here.
130+
enable_mandatory_bps:
131+
name: Attach best-practice skills on writes
132+
description: >-
133+
Master switch for the write-tool skill content delivery feature
134+
(issue #1182). When enabled (default), the six config write tools
135+
(automations, scripts, scenes, helpers, dashboards, raw YAML)
136+
attach the canonical Home Assistant best-practice reference files
137+
under `skill_content` on every successful write, plus auto-embed
138+
any reference sections cited by best-practice warnings. Each tool
139+
also exposes a per-call `MandatoryBPS` parameter the agent can
140+
set to false on subsequent calls once it has the content. When
141+
this master switch is off, NO skill_content goes out regardless
142+
of the per-call parameter or BP warnings. Leave on if your LLM
143+
benefits from inline guidance; turn off to minimise tokens when
144+
using an LLM that has the best-practice files indexed via skills
145+
or another retrieval path. Requires restart to take effect.
130146
enable_filesystem_tools:
131147
name: Enable filesystem tools (beta)
132148
description: >-

homeassistant-addon/config.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ options:
4545
enable_tool_search: false
4646
tool_search_max_results: 5
4747
enable_tool_security_policies: false
48+
enable_mandatory_bps: true
4849
disabled_tools: ""
4950
pinned_tools: ""
5051
enable_auto_backup: true
@@ -57,6 +58,7 @@ schema:
5758
enable_tool_search: bool?
5859
tool_search_max_results: int(2,10)?
5960
enable_tool_security_policies: bool?
61+
enable_mandatory_bps: bool?
6062
disabled_tools: str?
6163
pinned_tools: str?
6264
enable_auto_backup: bool?

homeassistant-addon/start.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,7 @@ def main() -> int:
282282
code_mode_in_config = False # presence flag
283283
enable_lite_docstrings = False # default
284284
lite_docstrings_in_config = False # presence flag
285+
enable_mandatory_bps = True # default (issue #1182 — on by default, non-beta)
285286
# Master beta toggle: present only in the dev addon's schema.
286287
# Default to False (stable behaviour); when
287288
# the dev schema-default merges in, ``beta_master_in_config``
@@ -358,6 +359,16 @@ def main() -> int:
358359
enable_lite_docstrings = (
359360
raw_lite_docstrings if isinstance(raw_lite_docstrings, bool) else False
360361
)
362+
raw_mandatory_bps = config.get("enable_mandatory_bps", True)
363+
if isinstance(raw_mandatory_bps, bool):
364+
enable_mandatory_bps = raw_mandatory_bps
365+
else:
366+
log_error(
367+
"enable_mandatory_bps must be bool, got "
368+
f"{type(raw_mandatory_bps).__name__}={raw_mandatory_bps!r}; "
369+
"using default True"
370+
)
371+
enable_mandatory_bps = True
361372
# Master beta toggle is present in the dev-addon schema.
362373
# Track presence separately so stable
363374
# add-on installs (where the key is absent from options.json)
@@ -435,6 +446,10 @@ def main() -> int:
435446
os.environ["ENABLE_TOOL_SECURITY_POLICIES"] = str(
436447
enable_tool_security_policies
437448
).lower()
449+
# ENABLE_MANDATORY_BPS is non-beta and default-ON, so it is written
450+
# unconditionally (like the stable core settings above) — never
451+
# presence-gated or beta-master-gated like the beta sub-flags below.
452+
os.environ["ENABLE_MANDATORY_BPS"] = str(enable_mandatory_bps).lower()
438453
# Beta sub-flags: only write env vars when the key is actually in
439454
# the addon's options.json. On stable addon,
440455
# none of these keys are in schema, so config.get(...) returned

homeassistant-addon/translations/en.yaml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,22 @@ configuration:
3737
and click Approve before the call proceeds. Per-tool rules with
3838
optional argument conditions are configured in the Tool Security
3939
Policies tab. Off by default. Requires restart to take effect.
40+
enable_mandatory_bps:
41+
name: Attach best-practice skills on writes
42+
description: >-
43+
Master switch for the write-tool skill content delivery feature
44+
(issue #1182). When enabled (default), the six config write tools
45+
(automations, scripts, scenes, helpers, dashboards, raw YAML)
46+
attach the canonical Home Assistant best-practice reference files
47+
under `skill_content` on every successful write, plus auto-embed
48+
any reference sections cited by best-practice warnings. Each tool
49+
also exposes a per-call `MandatoryBPS` parameter the agent can
50+
set to false on subsequent calls once it has the content. When
51+
this master switch is off, NO skill_content goes out regardless
52+
of the per-call parameter or BP warnings. Leave on if your LLM
53+
benefits from inline guidance; turn off to minimise tokens when
54+
using an LLM that has the best-practice files indexed via skills
55+
or another retrieval path. Requires restart to take effect.
4056
enable_auto_backup:
4157
name: Enable auto-backup of edits
4258
description: >-

src/ha_mcp/config.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,18 @@ class Settings(BaseSettings):
144144
# env-var users see the trade-off in their logs.
145145
enable_lite_docstrings: bool = Field(False, alias="ENABLE_LITE_DOCSTRINGS")
146146

147+
# Mandatory best-practice skills — server-side master switch for the
148+
# write-tool skill_content delivery feature (issue #1182). When True
149+
# (default), the six write tools (automations / scripts / scenes /
150+
# helpers / dashboards / yaml) attach the canonical best-practice
151+
# reference files under ``skill_content`` on every successful write,
152+
# plus auto-embed any sections cited by best-practice warnings. The
153+
# per-call ``MandatoryBPS`` parameter on each tool controls whether
154+
# the canonical files ship for that one call. This setting is the
155+
# master gate above that — when False, NO skill_content goes out
156+
# regardless of the per-call param or BP warnings. Default on.
157+
enable_mandatory_bps: bool = Field(True, alias="ENABLE_MANDATORY_BPS")
158+
147159
# Filesystem tools — read/write/delete/list under the HA config dir.
148160
# Previously gated by a direct ``os.getenv`` call in
149161
# ``tools/tools_filesystem.py`` so callers (and the settings UI)
@@ -412,6 +424,12 @@ class AdvancedField(NamedTuple):
412424
FeatureFlagField(
413425
"enable_tool_security_policies", "ENABLE_TOOL_SECURITY_POLICIES", bool
414426
),
427+
# Non-beta, default-ON master switch for write-tool skill_content
428+
# delivery (#1182). Grouped with the non-beta flags above the beta
429+
# run below; intentionally NOT in BETA_FEATURE_FIELDS (it must not be
430+
# gated by the beta master) nor in ADVANCED_SETTINGS_FIELDS (registries
431+
# are name-disjoint per _validate_registries()).
432+
FeatureFlagField("enable_mandatory_bps", "ENABLE_MANDATORY_BPS", bool),
415433
FeatureFlagField("enable_yaml_config_editing", "ENABLE_YAML_CONFIG_EDITING", bool),
416434
FeatureFlagField("enable_lite_docstrings", "ENABLE_LITE_DOCSTRINGS", bool),
417435
FeatureFlagField("enable_filesystem_tools", "HAMCP_ENABLE_FILESYSTEM_TOOLS", bool),

src/ha_mcp/server.py

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -200,10 +200,13 @@ def _get_skills_dir(self) -> Path | None:
200200
201201
Skills are vendored via a git submodule at resources/skills-vendor/.
202202
The actual skill directories live under the skills/ subdirectory
203-
within that repo.
203+
within that repo. Delegates to
204+
:func:`ha_mcp.utils.skill_loader.get_skills_dir` so the write-tool
205+
``MandatoryBPS`` parameter resolves the same path.
204206
"""
205-
skills_dir = Path(__file__).parent / "resources" / "skills-vendor" / "skills"
206-
return skills_dir if skills_dir.exists() else None
207+
from .utils.skill_loader import get_skills_dir
208+
209+
return get_skills_dir()
207210

208211
def _build_skills_instructions(self) -> str | None:
209212
"""Build server instructions from bundled skill frontmatter.
@@ -1434,13 +1437,28 @@ def _handle_skill_guide_call(
14341437
)
14351438
)
14361439

1437-
return {
1438-
"success": True,
1439-
"skill": skill,
1440-
"file": file,
1441-
"uri": f"skill://{skill}/{file}",
1442-
"content": content,
1443-
}
1440+
# Hint goes at the top of the response so the LLM sees it before
1441+
# parsing the (potentially large) content body. Scoped to the
1442+
# best-practice skill because that's the one the write-tool
1443+
# MandatoryBPS param gates; other skills (if any) are unrelated.
1444+
from .tools.util_helpers import (
1445+
_HA_BEST_PRACTICES_SKILL_NAME,
1446+
_SKILL_GUIDE_MANDATORYBPS_HINT,
1447+
)
1448+
1449+
response: dict[str, Any] = {}
1450+
if skill == _HA_BEST_PRACTICES_SKILL_NAME:
1451+
response["skill_content_hint"] = _SKILL_GUIDE_MANDATORYBPS_HINT
1452+
response.update(
1453+
{
1454+
"success": True,
1455+
"skill": skill,
1456+
"file": file,
1457+
"uri": f"skill://{skill}/{file}",
1458+
"content": content,
1459+
}
1460+
)
1461+
return response
14441462

14451463
# Helper methods required by EnhancedToolsMixin
14461464

src/ha_mcp/settings_ui.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2165,6 +2165,10 @@ def apply_tool_visibility(
21652165
label: "Enable Tool Security Policies",
21662166
help: "Opt-in middleware that gates high-stakes MCP tool calls behind user approval. When enabled, tools that match a rule in the Tool Security Policies tab require you to click Approve in the web UI before they run. Off by default. Per-tool rules with optional argument conditions are configured in the Tool Security Policies tab. Requires restart to take effect.",
21672167
},
2168+
enable_mandatory_bps: {
2169+
label: "Attach best-practice skills on writes",
2170+
help: "Master switch for the write-tool skill content delivery feature (issue #1182). When enabled (default), the six config write tools (automations, scripts, scenes, helpers, dashboards, raw YAML) attach the canonical Home Assistant best-practice reference files under skill_content on every successful write, plus auto-embed any reference sections cited by best-practice warnings. Each tool also exposes a per-call MandatoryBPS parameter the agent can set to false on subsequent calls once it has the content. When this master switch is off, NO skill_content goes out regardless of the per-call parameter or BP warnings. Leave on if your LLM benefits from inline guidance; turn off to minimise tokens when using an LLM that has the best-practice files indexed via skills or another retrieval path. Requires restart to take effect.",
2171+
},
21682172
// Master beta toggle — gates the 5 sub-flags below at runtime
21692173
// (see config.py:_apply_feature_flag_overrides master gate). UI
21702174
// dims sub-rows when this is off and re-renders live on flip.

0 commit comments

Comments
 (0)