Skip to content

Commit 2e52516

Browse files
feat: always-on skills; rename list/read resource tools with ha_ prefix (#1136)
* feat: drop ENABLE_SKILLS / ENABLE_SKILLS_AS_TOOLS toggles, rename to ha_list_resources / ha_read_resource The two skills toggles were redundant once #960 (web settings UI) shipped: the original justifications — "avoid local-skill conflicts" (skills served via skill:// URIs and locally-installed skills under ~/.claude/skills/ live in separate namespaces) and "don't burn extra context" (resources are not auto-injected; list_resources only returns metadata) — don't survive scrutiny. Per-tool visibility is now the single mechanism; users who want ha_list_resources or ha_read_resource off can disable them from the web settings UI like any other tool. Changes: - Drop enable_skills, enable_skills_as_tools, _skills_dependency validator from src/ha_mcp/config.py. - Always register SkillsDirectoryProvider and the ResourcesAsTools transform in server._register_skills(); always include the skills hint in _build_skills_instructions and the search-tool description. - Rename the transform-generated tools list_resources / read_resource to ha_list_resources / ha_read_resource via a HaResourcesAsTools subclass so they follow the project's ha_<verb>_<noun> convention. - Surface the renamed pair in the settings UI: extend _get_tool_metadata with TRANSFORM_GENERATED_TOOLS stub injection, so users can disable them per-tool (FastMCP's local_provider doesn't see transform-appended tools). - Drop add-on toggles: enable_skills / enable_skills_as_tools removed from homeassistant-addon{,-dev}/config.yaml schema + options, the env-var setup and the .skills_as_tools_default_migration_v1 migration helper removed from homeassistant-addon/start.py, and the supervisor labels removed from homeassistant-addon-dev/translations/en.yaml (prod translations sync at the next biweekly release). - Simplify best_practice_checker: drop get_skill_prefix() and its callers; always reference skill:// URIs. - Tests: replace the obsolete enable_skills* assertions with field-removal assertions, drop TestSkillsAsToolsMigration, update unit tests for unconditional registration, add e2e coverage that ha_list_resources / ha_read_resource appear (and unprefixed names don't), add a unit test that _get_tool_metadata injects stubs when local_provider omits them. - README and homeassistant-addon-dev/DOCS.md: remove the toggle rows; document the per-tool visibility path. Migration note: existing users with ENABLE_SKILLS_AS_TOOLS=false lose the env-var path. Equivalent opt-out is available per-tool in the web settings UI, documented in DOCS.md and README. Same outcome is achievable via the alternate mechanism, so this is not a breaking change per AGENTS.md's definition. Closes #1133 * fix: address PR review — harden rename, fix lingering read_resource refs, add coverage Review feedback from the pr-review-toolkit agents (code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer): - Update LLM-facing strings still mentioning the unprefixed `read_resource` in `_register_skill_guidance_tools` (tool description + handler `how_to_use`) and the `_register_skill_guidance_tools` docstring; also update the `tests/uat/stories/catalog/s13_dashboard_update_existing.yaml` expected-tools list. - Replace the `result[-2:]` slicing in `HaResourcesAsTools.list_tools` with a name-based scan over the full upstream sequence and a `_RENAMES` class mapping. Log a warning if the matched count is not exactly two so a future fastmcp regression that drops or reorders the appended tools surfaces loudly at boot instead of silently leaking the unprefixed names. - Use `HaResourcesAsTools.LIST_TOOL_NAME` / `READ_TOOL_NAME` constants for the pinned-tools list and the search-tool description text in `server.py` so the rename has a single source of truth. - Add an upgrade-fragility note to the `HaResourcesAsTools` docstring flagging the dependency on fastmcp's `_make_*_tool` private factories. - Tighten the `best_practice_checker` module docstring: the `skill_prefix` kwarg note now says "any URL prefix (e.g., a GitHub mirror)" instead of implying a canonical alternative still exists in the module. - Document the cross-module `TRANSFORM_GENERATED_TOOLS` ↔ `HaResourcesAsTools` constant invariant and add `test_transform_generated_tool_names_match_class_constants`. - New unit test file `tests/src/unit/test_ha_resources_as_tools.py`: rename happy-path for `list_tools`/`get_tool`, fall-through for unprefixed and unrelated names, and a drift-warning test that monkey-patches the base class to drop one of the appended tools. - New e2e tests `test_ha_list_resources_invocation` and `test_ha_read_resource_invocation` in `tests/src/e2e/tools/test_skills_resources.py`: actually invoke the renamed tools via `mcp_client.call_tool(...)` to confirm the rename doesn't break dispatch routing (catalog presence is necessary but not sufficient). Note on Gemini's two inline comments: both are based on a non-existent fastmcp API (`Transform.call_tool` / `CallToolNext`) — fastmcp's transform protocol exposes `list_tools` and `get_tool`, and tool dispatch in `FastMCP.call_tool` runs through `get_tool` (which this subclass overrides) before invoking `tool.run`. The new e2e invocation tests verify this end-to-end. * fix: address remaining review items — marker cleanup, skill summary log, ToolStub typing Three follow-ups from the pr-review-toolkit feedback that were initially deferred but on reconsideration belong in this PR: - ``cleanup_stale_migration_marker`` in ``homeassistant-addon/start.py`` removes ``/data/.skills_as_tools_default_migration_v1`` on next boot. The marker was created by the previous version's ``migrate_skills_as_tools_default`` (deleted in this PR's first commit); leaving it on disk forever is permanent ``/data`` litter for every existing add-on install. ``unlink(missing_ok=True)`` plus best-effort error handling. - ``HomeAssistantSmartMCPServer._register_skills`` now tracks per-phase status (provider / transform / guidance_tools count) and emits one summary log line at the end via ``_log_skill_registration_summary`` — ``info`` when both provider and transform succeeded, ``warning`` otherwise. Without the toggle, every install runs this code path on every boot, so a single line operators can grep for is more useful than reconstructing state from scattered ``logger.exception`` calls. Per-phase exception logs remain for stack traces. - ``ToolStub`` ``TypedDict`` defined in ``settings_ui.py`` and applied to both ``TRANSFORM_GENERATED_TOOLS`` and ``FEATURE_GATED_TOOLS``, with ``NotRequired`` keys for ``disabled_by``, ``readOnlyHint``, ``destructiveHint``. Annotation values converted from ``"true"`` strings to actual ``bool``. ``_render_stub`` helper collapses the previously-duplicated stub-build loops into one function so a misspelled key now fails type-checking instead of silently producing an entry with the wrong shape. Behavioral output is unchanged. --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
1 parent 08b69db commit 2e52516

19 files changed

Lines changed: 656 additions & 543 deletions

README.md

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -225,12 +225,9 @@ An MCP server can create automations, helpers, and dashboards, but it has no opi
225225

226226
### Bundled Skills (built-in)
227227

228-
Skills from `homeassistant-ai/skills` are bundled and served as [MCP resources](https://modelcontextprotocol.io/docs/concepts/resources) via `skill://` URIs. Any MCP client that supports resources can discover them automatically — no manual installation needed.
228+
Skills from `homeassistant-ai/skills` are bundled and served as [MCP resources](https://modelcontextprotocol.io/docs/concepts/resources) via `skill://` URIs. Any MCP client that supports resources can discover them automatically — no manual installation needed. For tool-only clients, the same skills are also exposed as `ha_list_resources` / `ha_read_resource` tools. Resources are not auto-injected into context — clients must explicitly request them, so idle context cost is just the metadata listing.
229229

230-
| Setting | Default | Description |
231-
|---------|---------|-------------|
232-
| `ENABLE_SKILLS` | `true` | Serve skills as MCP resources. Resources are not auto-injected into context — clients must explicitly request them. |
233-
| `ENABLE_SKILLS_AS_TOOLS` | `true` | Expose skills and doc resources via `list_resources`/`read_resource` tools. Resource-capable clients can set to `false` to reduce tool count. |
230+
If you want to hide either tool from the catalog, disable it from the web settings UI like any other tool.
234231

235232
Skills can still be installed manually for clients that prefer local skill files — see the [skills repo](https://github.com/homeassistant-ai/skills) for instructions.
236233

homeassistant-addon-dev/DOCS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@ The dev add-on uses the same configuration as the stable version. See the main a
1414
|--------|-------------|---------|
1515
| `backup_hint` | Backup strength preference | `normal` |
1616
| `secret_path` | Custom secret path (optional) | auto-generated |
17-
| `enable_skills` | Serve bundled HA best-practice skills as MCP resources | `true` |
18-
| `enable_skills_as_tools` | Expose skills via list_resources/read_resource tools | `true` |
1917
| `enable_tool_search` | Replace full tool catalog with search-based discovery (~46K → ~5K tokens). ⚠️ Do NOT enable for Claude Sonnet/Opus — their built-in tool search conflicts with ha-mcp's. Disable one or the other. | `false` |
2018
| `enable_yaml_config_editing` *(beta)* | Enables `ha_config_set_yaml` for editing `configuration.yaml` directly. Requires `ha_mcp_tools` custom component. | `false` |
2119
| `enable_filesystem_tools` *(beta)* | Enables file read/write tools (`ha_list_files`, `ha_read_file`, `ha_write_file`, `ha_delete_file`). Requires `ha_mcp_tools` custom component. | `false` |
@@ -25,6 +23,8 @@ The dev add-on uses the same configuration as the stable version. See the main a
2523
| `pinned_tools` | Comma-separated list of tool names to pin when tool search is enabled (seed value; web UI is primary) | empty |
2624
| `verify_ssl` | Verify the HA server's TLS certificate. Disable for self-signed certs or hostname mismatches. Weakens security — leave on unless needed. | `true` |
2725

26+
*Removed in 7.4.x:* `enable_skills` *and* `enable_skills_as_tools`*. Bundled skills are now always served; to hide* `ha_list_resources` *or* `ha_read_resource` *from the catalog, disable them from the Tool Settings Web UI (see below).*
27+
2828
Beta options are hidden under "Show unused optional configuration options" in the add-on Configuration tab. See [beta.md](https://github.com/homeassistant-ai/ha-mcp/blob/master/docs/beta.md) for details.
2929

3030
### Permissions

homeassistant-addon-dev/config.yaml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ host_network: true
2323
image: "ghcr.io/homeassistant-ai/ha-mcp-addon-dev-{arch}"
2424
options:
2525
backup_hint: "normal"
26-
enable_skills: true
27-
enable_skills_as_tools: true
2826
enable_tool_search: false
2927
enable_yaml_config_editing: false
3028
tool_search_max_results: 5
@@ -34,8 +32,6 @@ options:
3432
schema:
3533
backup_hint: list(strong|normal|weak|auto)
3634
secret_path: str?
37-
enable_skills: bool?
38-
enable_skills_as_tools: bool?
3935
enable_tool_search: bool?
4036
enable_yaml_config_editing: bool?
4137
enable_filesystem_tools: bool?

homeassistant-addon-dev/translations/en.yaml

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,6 @@ configuration:
77
name: Secret path override
88
description: |
99
Optional custom HTTP path for the MCP server. Leave empty to use the auto-generated secure path.
10-
enable_skills:
11-
name: Enable skills
12-
description: >-
13-
Serve bundled Home Assistant best-practice skills as MCP resources.
14-
Skills provide automation patterns, helper selection guides, and device
15-
control best practices. Clients must explicitly request them.
16-
enable_skills_as_tools:
17-
name: Enable skills as tools
18-
description: >-
19-
Expose skills via list_resources/read_resource tools for MCP clients
20-
that don't support resources natively. Adds 3 extra tools.
2110
enable_tool_search:
2211
name: Enable tool search
2312
description: >-

homeassistant-addon/config.yaml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,11 @@ image: "ghcr.io/homeassistant-ai/ha-mcp-addon-{arch}"
3030
# from homeassistant-addon-dev/config.yaml without reading that doc first.
3131
options:
3232
backup_hint: "normal"
33-
enable_skills: true
34-
enable_skills_as_tools: true
3533
enable_tool_search: false
3634
verify_ssl: true
3735
schema:
3836
backup_hint: list(strong|normal|weak|auto)
3937
secret_path: str?
40-
enable_skills: bool?
41-
enable_skills_as_tools: bool?
4238
enable_tool_search: bool?
4339
verify_ssl: bool?
4440
advanced_debug_logging: bool?

homeassistant-addon/start.py

Lines changed: 19 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -179,84 +179,26 @@ def resolve_bool_option(config: dict[str, Any], key: str, default: bool) -> bool
179179
return raw if isinstance(raw, bool) else default
180180

181181

182-
SKILLS_AS_TOOLS_MIGRATION_MARKER = ".skills_as_tools_default_migration_v1"
183-
184-
185-
def migrate_skills_as_tools_default(
186-
data_dir: Path,
187-
config_file: Path,
188-
stored_value: bool,
189-
config_read_ok: bool,
190-
) -> bool:
191-
"""One-time migration to force enable_skills_as_tools=true for existing users.
192-
193-
The Pydantic default in src/ha_mcp/config.py was flipped to True in
194-
#806, but the add-on's config.yaml was never updated at the same time.
195-
For add-on installs the env var is written from options.json before
196-
ha-mcp reads its Pydantic settings, so the new Python default never
197-
took effect for existing users. This runs exactly once per install
198-
(guarded by a marker file in /data) and forces the flag on for users
199-
who still have False stored, then persists the new value to
200-
options.json so the supervisor UI reflects it. On subsequent boots the
201-
marker is present and the stored value is respected, so users who
202-
deliberately toggle it off will not be re-forced.
203-
204-
config_read_ok must be False when the caller could not load
205-
options.json (file unreadable or malformed JSON). In that case the
206-
marker is not created, so the migration can run again on a later
207-
boot once options.json is readable and expose the user's real
208-
stored value.
182+
_STALE_MIGRATION_MARKER = ".skills_as_tools_default_migration_v1"
183+
184+
185+
def cleanup_stale_migration_marker(data_dir: Path) -> None:
186+
"""Remove the one-time enable_skills_as_tools migration marker.
187+
188+
The marker was created by the previous version's
189+
``migrate_skills_as_tools_default`` (removed in #1133). It is now
190+
unused on every install; cleaning it up prevents permanent ``/data``
191+
litter for users who upgraded across the toggle removal. ``unlink``
192+
is best-effort — a stale dotfile is harmless if removal fails.
209193
"""
210-
marker = data_dir / SKILLS_AS_TOOLS_MIGRATION_MARKER
211-
if marker.exists():
212-
return stored_value
213-
214-
# First run after this update. Force-on + persist only if the user is
215-
# currently on False, then create the marker so the migration does
216-
# not loop — but skip marker creation when the caller could not
217-
# verify the stored value (see config_read_ok in the docstring).
218-
if not stored_value:
219-
log_info(
220-
"One-time migration: forcing enable_skills_as_tools=true. "
221-
"The Pydantic default was set to True in #806 but the add-on's "
222-
"config.yaml was not updated alongside it, so this value stayed "
223-
"False for existing add-on installs. Future user-initiated "
224-
"changes to this setting will be respected."
194+
marker = data_dir / _STALE_MIGRATION_MARKER
195+
try:
196+
marker.unlink(missing_ok=True)
197+
except OSError as e:
198+
log_error(
199+
f"Failed to remove stale migration marker {marker}: {e}. "
200+
"Safe to ignore — the file is unused."
225201
)
226-
if config_file.exists():
227-
try:
228-
with open(config_file, encoding="utf-8") as f:
229-
opts = json.load(f)
230-
if isinstance(opts, dict):
231-
opts["enable_skills_as_tools"] = True
232-
with open(config_file, "w", encoding="utf-8") as f:
233-
json.dump(opts, f, indent=2)
234-
f.write("\n")
235-
log_info("Persisted enable_skills_as_tools=true to options.json")
236-
else:
237-
log_error(
238-
"Cannot persist migration to options.json: top-level "
239-
f"is {type(opts).__name__}, expected dict. Runtime "
240-
"override still applied for this session."
241-
)
242-
except (OSError, json.JSONDecodeError) as e:
243-
log_error(
244-
f"Failed to persist migration to options.json "
245-
f"(operation: persist_skills_as_tools_migration): {e}. "
246-
"Runtime override still applied for this session."
247-
)
248-
stored_value = True
249-
250-
if config_read_ok:
251-
try:
252-
marker.touch()
253-
except OSError as e:
254-
log_error(
255-
f"Failed to create migration marker "
256-
f"(operation: create_skills_as_tools_marker): {e}"
257-
)
258-
259-
return stored_value
260202

261203

262204
def main() -> int:
@@ -266,11 +208,10 @@ def main() -> int:
266208
# Read configuration from Supervisor
267209
config_file = Path("/data/options.json")
268210
data_dir = Path("/data")
211+
cleanup_stale_migration_marker(data_dir)
269212
config: dict[str, Any] = {}
270213
backup_hint = "normal" # default
271214
custom_secret_path = "" # default
272-
enable_skills = True # default
273-
enable_skills_as_tools = True # default
274215
enable_tool_search = False # default
275216
enable_yaml_config_editing = False # default
276217
enable_filesystem_tools = False # default
@@ -280,18 +221,13 @@ def main() -> int:
280221
pinned_tools_raw = "" # default
281222
verify_ssl = True # default
282223
advanced_debug_logging = False # default
283-
config_read_ok = True
284224

285225
if config_file.exists():
286226
try:
287227
with open(config_file) as f:
288228
config = json.load(f)
289229
backup_hint = config.get("backup_hint", "normal")
290230
custom_secret_path = config.get("secret_path", "")
291-
raw_skills = config.get("enable_skills", True)
292-
enable_skills = raw_skills if isinstance(raw_skills, bool) else True
293-
raw_skills_as_tools = config.get("enable_skills_as_tools", True)
294-
enable_skills_as_tools = raw_skills_as_tools if isinstance(raw_skills_as_tools, bool) else True
295231
raw_tool_search = config.get("enable_tool_search", False)
296232
enable_tool_search = raw_tool_search if isinstance(raw_tool_search, bool) else False
297233
raw_yaml_config = config.get("enable_yaml_config_editing", False)
@@ -310,16 +246,6 @@ def main() -> int:
310246
advanced_debug_logging = resolve_bool_option(config, "advanced_debug_logging", False)
311247
except Exception as e:
312248
log_error(f"Failed to read config: {e}, using defaults")
313-
config_read_ok = False
314-
315-
# One-time migration: add-on users whose stored value is False predate
316-
# this release's config.yaml default flip. See migrate_skills_as_tools_default.
317-
enable_skills_as_tools = migrate_skills_as_tools_default(
318-
data_dir=data_dir,
319-
config_file=config_file,
320-
stored_value=enable_skills_as_tools,
321-
config_read_ok=config_read_ok,
322-
)
323249

324250
# Validate Supervisor token (needed for both ha-mcp auth below and the
325251
# options-persist call right after secret path resolution)
@@ -344,8 +270,6 @@ def main() -> int:
344270
# Set up environment for ha-mcp
345271
os.environ["HOMEASSISTANT_URL"] = "http://supervisor/core"
346272
os.environ["BACKUP_HINT"] = backup_hint
347-
os.environ["ENABLE_SKILLS"] = str(enable_skills).lower()
348-
os.environ["ENABLE_SKILLS_AS_TOOLS"] = str(enable_skills_as_tools).lower()
349273
os.environ["ENABLE_TOOL_SEARCH"] = str(enable_tool_search).lower()
350274
os.environ["ENABLE_YAML_CONFIG_EDITING"] = str(enable_yaml_config_editing).lower()
351275
os.environ["HAMCP_ENABLE_FILESYSTEM_TOOLS"] = str(enable_filesystem_tools).lower()

src/ha_mcp/config.py

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from pathlib import Path
1010

1111
from dotenv import load_dotenv
12-
from pydantic import Field, field_validator, model_validator
12+
from pydantic import Field, field_validator
1313
from pydantic_settings import BaseSettings, SettingsConfigDict
1414

1515
from ha_mcp._version import get_version
@@ -92,17 +92,6 @@ class Settings(BaseSettings):
9292
True, alias="ENABLE_DASHBOARD_PARTIAL_TOOLS"
9393
)
9494

95-
# Skills configuration
96-
# Serve bundled HA best-practice skills as MCP resources (skill:// URIs).
97-
# Resources are not auto-injected — clients must explicitly request them.
98-
enable_skills: bool = Field(True, alias="ENABLE_SKILLS")
99-
100-
# Expose skills and doc resources as tools (list_resources/read_resource)
101-
# for clients that don't support MCP resources natively.
102-
# Defaults to True so all clients can access documentation and skills.
103-
# Resource-capable clients can set to False to reduce tool count.
104-
enable_skills_as_tools: bool = Field(True, alias="ENABLE_SKILLS_AS_TOOLS")
105-
10695
# Tool search transform — replaces the full tool catalog with a unified
10796
# BM25 search tool and categorized call proxies (read/write/delete).
10897
# Dramatically reduces idle context token usage for LLMs.
@@ -124,18 +113,6 @@ class Settings(BaseSettings):
124113
# supervisor UI rejects out-of-range values before they reach env vars.
125114
tool_search_max_results: int = Field(5, ge=2, le=10, alias="TOOL_SEARCH_MAX_RESULTS")
126115

127-
@model_validator(mode="after")
128-
def _skills_dependency(self) -> "Settings":
129-
"""Auto-enable skills (resources) when skills-as-tools is on.
130-
131-
skills_as_tools wraps ResourcesAsTools which requires skills to be
132-
registered as MCP resources first. Without this, enabling
133-
skills_as_tools alone would produce empty list_resources results.
134-
"""
135-
if self.enable_skills_as_tools and not self.enable_skills:
136-
self.enable_skills = True
137-
return self
138-
139116
@property
140117
def env_file_name(self) -> str:
141118
"""Get the current environment file name."""

0 commit comments

Comments
 (0)