Claude/analyze repo issues p vm5g - #1
Closed
kingpanther13 wants to merge 2 commits into
Closed
Conversation
The area_filter parameter in ha_search_entities returned entities from unrelated areas because get_entities_by_area relied on fuzzy matching entity friendly names to infer area membership. This caused false positives (e.g., searching for "salon" could match entities in area "abc" if partial string ratios exceeded the threshold). Replace the fuzzy name-matching approach with proper Home Assistant registry lookups: - Entity registry: direct entity -> area_id assignments - Device registry: device -> area_id (inherited by entities) - Area registry: area_id -> area name (for fuzzy query matching) Entity area resolution priority: entity direct area > device area. Fixes homeassistant-ai#504 https://claude.ai/code/session_01Pv5Wp2wZSvfJEbVF4Zxyy9
kingpanther13
marked this pull request as ready for review
January 29, 2026 20:55
kingpanther13
pushed a commit
that referenced
this pull request
Feb 2, 2026
…coverage Addresses all critical, high, and medium issues from code review: - CRITICAL #1: Track per-assistant success/failure in exposure loop. On partial failure, response now includes exposure_succeeded and exposure_failed dicts showing exactly which assistants were updated. - HIGH #2/#7: Handle fetch failure on expose-only path. If entity registry get fails after exposure, return error with exposure_applied instead of silently returning empty entity_entry. - HIGH #3: Wrap coerce_bool_param calls for enabled/hidden in try/except ValueError, returning VALIDATION_INVALID_PARAMETER instead of falling through to generic Exception handler. - MEDIUM #6: Replace inline json.loads with parse_json_param utility, removing the import json dependency. - MEDIUM #8: Standardize no-updates error to use "suggestions" (plural list) matching all other error responses. - MEDIUM #9: Track actual server-confirmed exposures via succeeded dict instead of echoing back user input. - MEDIUM #10: Extract _format_entity_entry helper to eliminate duplicated entity_entry dict construction. Adds 8 new unit tests covering: - Expose-only failure (no partial flag) - Mixed partial failure with succeeded tracking - Expose-only entity not found - Invalid enabled/hidden values ("maybe") - All 3 assistants in single call - List type for expose_to (rejected) - Registry failure with labels https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV
kingpanther13
added a commit
that referenced
this pull request
Feb 4, 2026
* feat: merge labels and voice assistant exposure into ha_set_entity Extend ha_set_entity with two new optional parameters: - labels: list of label IDs (replace/set semantics, consistent with aliases) - expose_to: dict mapping assistant IDs to booleans for voice assistant exposure This enables single-call updates for all entity metadata including area, name, icon, enabled, hidden, aliases, labels, and voice assistant exposure. Add deprecation notices to ha_manage_entity_labels and ha_expose_entity docstrings, pointing users to ha_set_entity for single-entity operations. Includes 14 unit tests covering labels parameter, expose_to parameter, combined operations, partial failure handling, and JSON string parsing. Closes homeassistant-ai#481 https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * Update src/ha_mcp/tools/tools_entities.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/ha_mcp/tools/tools_entities.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * feat: merge labels and voice assistant exposure into ha_set_entity Extend ha_set_entity with two new optional parameters: - labels: list of label IDs (replace/set semantics, consistent with aliases) - expose_to: dict mapping assistant IDs to booleans for voice assistant exposure control Remove ha_manage_entity_labels and ha_expose_entity tools since their functionality is now covered by ha_set_entity. This reduces tool count and cognitive load for AI agents. Update all E2E and unit tests to use ha_set_entity instead of the removed tools. Add 14 new unit tests for the labels and expose_to parameters. Add "Tool Consolidation" principle to CLAUDE.md: remove redundant tools rather than deprecating them. Closes homeassistant-ai#481 https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * chore: sync AGENTS.md with CLAUDE.md tool consolidation principle https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * fix: resolve E2E test double-parsing bug in test_label_operations Tests were calling parse_mcp_result() then passing the already-parsed dict to assert_mcp_success(), which internally calls parse_mcp_result() again. A plain dict lacks the .content attribute, causing the second parse to return {"error": "No content in result"}. Fixed by passing raw MCP results directly to assert_mcp_success(), matching the pattern used in other working E2E tests. https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * fix: use assert_mcp_success for ha_get_state check in integrity test ha_get_state returns {"data": {...}} without a top-level "success" key, so parse_mcp_result + .get("success") always evaluates to None. Use assert_mcp_success which properly validates the response envelope. https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * fix: move json import to top of file per PEP 8 Move local `import json as _json` to the module level, addressing PR review feedback about import placement. https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * fix: address PR review — exposure tracking, error handling, and test coverage Addresses all critical, high, and medium issues from code review: - CRITICAL #1: Track per-assistant success/failure in exposure loop. On partial failure, response now includes exposure_succeeded and exposure_failed dicts showing exactly which assistants were updated. - HIGH #2/#7: Handle fetch failure on expose-only path. If entity registry get fails after exposure, return error with exposure_applied instead of silently returning empty entity_entry. - HIGH #3: Wrap coerce_bool_param calls for enabled/hidden in try/except ValueError, returning VALIDATION_INVALID_PARAMETER instead of falling through to generic Exception handler. - MEDIUM #6: Replace inline json.loads with parse_json_param utility, removing the import json dependency. - MEDIUM #8: Standardize no-updates error to use "suggestions" (plural list) matching all other error responses. - MEDIUM #9: Track actual server-confirmed exposures via succeeded dict instead of echoing back user input. - MEDIUM #10: Extract _format_entity_entry helper to eliminate duplicated entity_entry dict construction. Adds 8 new unit tests covering: - Expose-only failure (no partial flag) - Mixed partial failure with succeeded tracking - Expose-only entity not found - Invalid enabled/hidden values ("maybe") - All 3 assistants in single call - List type for expose_to (rejected) - Registry failure with labels https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * fix: rename exposure_applied to exposure_succeeded for consistency Addresses Gemini code review feedback: standardize response key names for exposure tracking across success and failure paths. https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * feat: add label_operation and bulk entity support to ha_set_entity Add full feature parity with removed ha_manage_entity_labels tool: - Add label_operation parameter: "set" (default), "add", "remove" - "set": replaces all labels (existing behavior) - "add": adds labels to existing without duplicates - "remove": removes specified labels from existing - Add bulk entity_id support (str | list[str]) - Bulk operations support labels and expose_to parameters only - Single-entity parameters (area_id, name, etc.) blocked for bulk - Parallel processing with asyncio.gather - Aggregated results with success/failure counts - Add _get_entity_labels() helper for fetching current labels - Refactor into _update_single_entity() for cleaner bulk support - Add comprehensive unit tests (31 total, 9 new tests) https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * test(e2e): add label_operation tests for add and remove Add E2E tests for new label_operation parameter in ha_set_entity: - test_add_labels_to_existing: verify 'add' preserves existing labels - test_remove_specific_labels: verify 'remove' only removes specified - test_add_prevents_duplicates: verify no duplicate labels when adding https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * chore: update uv.lock for version 6.5.0 https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV * fix: use set for O(1) label removal membership check Convert parsed_labels to set before list comprehension for improved performance when removing multiple labels: O(M+N) vs O(M*N). https://claude.ai/code/session_01LL5wZ2K7KQUU2AmMyKzNWV --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
kingpanther13
added a commit
that referenced
this pull request
Feb 14, 2026
Resolve all merge conflicts from upstream/master (Parts 1-4 merged). Address Gemini review comments: - Context propagation now handled by upstream's context=context params (fixes comments #1, #2, #3 on helpers.py) - Replace asyncio.sleep(1.0) with polling loop in test_entity_rename.py (fixes comment #4) Fix callers that assign exception_to_structured_error() result to add explicit raise_error=False (tools_config_dashboards, tools_search) since the default is now True. Fix duplicate ToolError imports in tools_config_helpers and tools_entities. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kingpanther13
added a commit
that referenced
this pull request
Feb 23, 2026
- Added dual src/ directories section (the #1 gotcha) - FORK-DEV.md itself is backed up to ~/.ha-mcp-fork-dev.md - Deploy workflow now includes restoring FORK-DEV.md after reset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kingpanther13
pushed a commit
that referenced
this pull request
Apr 13, 2026
…meassistant-ai#976) * fix(history): add query_params echo to _fetch_statistics response _fetch_history echoes its effective parameters in a query_params block (noted as desirable by maintainers on homeassistant-ai#964). _fetch_statistics had no equivalent, creating an asymmetry between the two source branches of ha_get_history. Adds query_params to the statistics response: - statistic_types: raw caller-supplied value (not resolved default), so the block reflects what was requested, not internal expansion. period is intentionally excluded — period_type at the top level already carries this value; duplication would contradict the stated design principle (time_range excluded for the same reason). Known edge case: statistic_types=[] is falsy, so all_stat_types falls back to the full six-type default while query_params.statistic_types would show []. Pre-existing behavior, not introduced here. Tracked as a follow-up item. Test: test_get_statistics_query_params_in_response asserts query_params present with statistic_types key and period absent. * fix(history): add limit and offset to _fetch_statistics query_params echo Extends the query_params block to include all three caller-controllable parameters, matching _fetch_history symmetry: - statistic_types: raw caller input (not normalized stat_types_list) - limit: effective_limit after coerce_int_param - offset: effective_offset after coerce_int_param Addresses KP13 review Critical #1. * test(history): remove false-green E2E statistics query_params test The test used sun.sun as entity — it has no state_class, so HA recorder generates no long-term statistics. On a stock fixture, inner.get("success") is falsy, the early-exit fires, and assertions never execute (false-green). Also fixes: "year" re-added to valid_periods list in period-iteration test (was incorrectly dropped — production code at L583 already includes it). Unit coverage for query_params echo is added in test_history_pagination.py. Addresses KP13 review Critical #2, #3, Important #6. * test(history): add unit tests for _fetch_statistics query_params echo Adds two tests to TestStatisticsPagination following the established pattern in this file (HistoryTools mock + patch get_connected_ws_client): - test_statistics_query_params_default: asserts statistic_types=None, limit=_DEFAULT_HISTORY_LIMIT, offset=0 on a default call - test_statistics_query_params_roundtrip: asserts exact values when statistic_types=["mean"], limit=10, offset=5 are passed explicitly Both tests call ha_get_history (public tool layer), not _fetch_statistics directly, matching the sibling test_history_pagination.py pattern. Addresses KP13 review Critical #2, #3, Important #4. --------- Co-authored-by: LB-Agent <lb@local>
kingpanther13
added a commit
that referenced
this pull request
May 4, 2026
Addresses Patch76's CHANGES_REQUESTED review on PR homeassistant-ai#1120 plus the audit-table walk-through showed several legit nuggets I'd skipped on the first pass. Adding all of them now. Patch76 #1 — Cloudflared HA-addon `additional_hosts` block: Added a <details> "Running on Home Assistant OS? Use the Cloudflared add-on" section to the existing Cloudflare Tunnel block, with the brenner-tobias add-on badge link and the `additional_hosts:` YAML pointing at port 9583. Patch76 #2 + #3 — github-copilot-agents org-deployment notes: New "Org & Repository Deployment" instruction-block (any transport) documenting: - Repository-wide config via Settings → Copilot → Coding agent → MCP configuration on github.com (applies to all users with repo access, alternative to per-user .vscode/mcp.json) - Operational prerequisite: the "MCP servers" policy must be enabled for the org/enterprise — admins disable it by default. JetBrains — extended clientNote with the "Import from Claude" button tip (Settings → Tools → AI Assistant → MCP Servers) for users migrating from Claude Desktop. Audit-table gaps closed: - Antigravity: new instruction-block with the UI nav steps to the raw config editor (... menu → MCP Servers → Manage MCP Servers → View raw config) plus an HTTP-transport caveat (gated to !isStdio) about "connection closed" / "SSE stream failed to reconnect" errors with a recommendation to switch to stdio. - Codex: extended Management Commands block with a Codex Desktop walkthrough (Settings → MCP → Add Server with field names) and an OAuth-2.0-for-remote-servers note (gated to !isStdio). - VS Code: secure-input-prompts block now also mentions the vscode:mcp/install?<config> deep-link install pattern. - Copilot CLI: Notes & extras section after both the stdio and HTTP step blocks — Server Type legend (1/2/3/4 for Local/STDIO/HTTP/SSE), KEY=VALUE env-var format, * vs comma-list Tools format, COPILOT_HOME override, /mcp interactive command, "GitHub MCP server included by default" reminder. - OpenCode: Management Commands block extended with OPENCODE_CONFIG env-var path override, project-vs-global config-merge precedence (with link to opencode.ai/docs), {env:VAR} headers interpolation pattern for Bearer auth, oauth: false opt-out flag, home-assistant_* tool namespacing, and a 92+-tools context-size warning recommending a dedicated OpenCode agent for HA-heavy workflows. - Webhook Proxy: "How It Works" section before the install steps with the routing chain (AI client → HTTPS → reverse proxy → HA :8123 → webhook /api/webhook/<id> → MCP add-on). Plus a comparison table vs Cloudflare Tunnel (setup/cost/routing/best-for) after the steps. - Continue: clientNote with the Agent-Mode-required gotcha — MCP only works when Continue is in Agent Mode (use the agent selector near the chat input). - Claude.ai: clientNote with the Pro/Max/Team/Enterprise subscription requirement and the "Search and tools" button tip for per-conversation tool toggling. - Claude Code: clientNote noting config changes take effect immediately (no restart needed) — useful contrast with the restart-required clients. - Linux quick-test: new FAQ item ("Test ha-mcp without configuring a client") with the public-demo-server one-liner from the deleted linux.md body. TOC entry added. - AGENTS.md: appended a sentence to the "adding a new entry" recipe noting that arrays should be kept ordered by `order` (the wizard renders in array order without re-sorting). Intentional skips (with reason, in case anyone re-audits): - JetBrains Node 18+ requirement: only relevant for npm-based MCP servers; ha-mcp uses uvx, so this is a non-applicable constraint. - JetBrains 2025.2+ built-in MCP server: about the IDE itself acting as an MCP server, not relevant to ha-mcp client setup. - Zed Bearer-header HTTP shape: Zed is in stdioOnlyClients, so the wizard routes Zed users through mcp-proxy for HTTP — the isZed HTTP branch in the JSON builder is dead code for ha-mcp users going through the wizard. Adding a Bearer-auth alternative would contradict the httpNote. - uvx Python 3.10+ claim: source body was wrong (faq says 3.13+, matches pyproject.toml requires-python = "==3.13.*"). Body deletion auto-resolved. Build verified clean (npm run build, 7 pages, 18.4s).
kingpanther13
added a commit
that referenced
this pull request
May 5, 2026
…s, drop content collections (homeassistant-ai#1120) * feat(site): add transport-agnostic clientNote field, fix multi-line configLocation rendering Two foundational wizard fixes prerequisite for the content-collection cleanup: 1. clientNote (homeassistant-ai#1106 / Patch76 S2): new optional frontmatter field on the clients schema, rendered unconditionally in the wizard's notes pane. Distinct from httpNote, which only renders on non-stdio paths and so misses dependencies that apply across all transports (e.g. github-copilot agents requires the Copilot extension regardless of stdio vs sse). 2. configLocation multi-line rendering (homeassistant-ai#1106 / Patch76 S1): inline <code> collapsed YAML newlines into one unreadable string. Now splits at newline boundaries, rendering as a bulleted list when multi-line and keeping the previous single-<code> form when single-line. Affects github-copilot-agents and claude-desktop frontmatter today. * feat(site): migrate transport-agnostic notes to clientNote, add troubleshooting nuggets Migrates ~20 nuggets from unrendered content-collection bodies into surfaces users actually see (homeassistant-ai#1097, homeassistant-ai#1106). clients/*.md frontmatter — add clientNote (or rename httpNote → clientNote for cases where the dependency applies regardless of transport): - cursor.md: restart-after-config - windsurf.md: restart-after-config - jetbrains.md: IDE 2025.1+ + AI Assistant plugin + restart - vscode.md: GitHub Copilot extension prereq + reload - github-copilot-agents.md: extension + coding-agent-mode (was httpNote, but applies to stdio path too) - open-webui.md: v0.6.31+ MCP support - raycast.md: v1.98.0+/1.100.0+ MCP+HTTP version reqs, Pro/BYOK faq.astro: - new "Antigravity client troubleshooting" item (5 bullets) - new "Claude.ai connection issues" item - new "Keep ha-mcp-web running in the background" item (nohup pattern) - new "Docker port configuration" item (dual-port caveat: -p second number must equal MCP_PORT) - extended existing "uvx not found" item with Claude Desktop PATH not-inherited workaround guide-macos.astro: - new optional Quick Test step using public demo server * feat(site): migrate UI/CLI client wizard nuggets Extends existing wizard branches with content from unrendered bodies (homeassistant-ai#1097, homeassistant-ai#1106). All additions follow the existing structural patterns of the affected branch (extending <ol> for UI clients, pushing instruction-block divs for CLI clients). ChatGPT (UI block, configFormat=ui): - Add 4th step with the per-conversation activation flow: + → More → Developer Mode → enable connector each conversation, root-path "/" requirement, UUID client ID requirement. Raycast (UI block, configFormat=ui): - Add @home-assistant @-mention usage (Quick AI / AI Chat) - Note deeplink-install alternative - Add HTTP-transport block (gated on !isStdio): enable HTTP in AI Settings, httpHeaders Bearer auth shape - Link to manual.raycast.com/model-context-protocol Open WebUI (UI block, configFormat=ui): - Correct nav path: Admin Panel → Settings → Tools → Manage Tool Servers (was Admin Settings → External Tools — body now reflects the current v0.6.31+ flow); call out the user-side "External Tools" so users don't pick the wrong setting - Drop the now-stale "Set Type: MCP (Streamable HTTP)" step (the current flow no longer asks for it per the body) - Add URL discovery patterns block (host.docker.internal, local network, cloudflared) and mcpo proxy note for stdio servers - Link to docs.openwebui.com/features/mcp/ Claude Code (CLI, configFormat=cli): - Push a Management Commands instruction-block: - SSE alternative `claude mcp add --transport sse ...` (gated on !isStdio — primary stays http to match the JSON path's intent) - claude mcp list / claude mcp remove - --scope user note for cross-project sharing Gemini CLI (CLI, configFormat=cli): - Push a Management Commands instruction-block: - SSE alternative gated on !isStdio (primary stays --transport http to match the wizard's JSON config which uses httpUrl / streamableHttp — keeping primary consistent with the other path) - gemini mcp list / remove - /mcp and /mcp refresh slash commands inside the chat session Codex (CLI, configFormat=cli): - Push a Management Commands instruction-block: codex mcp list / get / remove Antigravity (JSON, configFormat=json, isStdio): - New stdio branch in the JSON builder (placed before the generic mcpServers fallback). Sets FASTMCP_SHOW_SERVER_BANNER=false in env to suppress the startup banner that triggers Antigravity's "Unexpected server output" errors. Handles both uvx (env object) and Docker (-e flag in args) shapes. Build verified clean (npm run build, 7 pages, 4.84s). * feat(site): migrate VS Code/github-copilot-agents secure-inputs + OpenCode mgmt cmds Adds 3 alternative-instruction blocks to the JSON-config branch of the wizard (homeassistant-ai#1097, homeassistant-ai#1106). All three follow the existing instruction-block pattern (<div class="instruction-block"> + <h4 class="instruction-title">) used by the CLI clients' Management Commands blocks. VS Code (json/stdio): - "Alternative: Secure Input Prompts" block. Shows the mcp.inputs[] + ${input:ha-url} / ${input:ha-token} variant from vscode.md so users can opt into VS Code's prompt-on-first-use credential UX instead of putting the URL+token in the config file. GitHub Copilot Agents (json): - "Alternative: Secure Input Prompts" block (gated on the client id, any transport). Shows the bare top-level inputs[] + servers{} shape from github-copilot-agents.md. - Includes an inline note explaining the wizard emits the .vscode/mcp.json (repository) shape, and the personal settings.json form just wraps the same content under an extra mcp: { ... } key. OpenCode (json): - "Management Commands & Quirks" block: opencode mcp list / auth / logout / debug, plus the corrective trio for users coming from other clients (top-level mcp not mcpServers, single command array no args field, environment not env). Build verified clean (npm run build, 7 pages, 24.26s). * feat(site): migrate platform/deployment nuggets into wizard Targeted edits to existing wizard branches in setup.astro for nuggets that fit cleanly into the current structure (homeassistant-ai#1097, homeassistant-ai#1106). Linux uvInstall block (platformId === 'linux'): - Add an "Or via your package manager" line citing pacman -S uv (Arch) and apk add uv (Alpine), matching the macOS branch's existing one-liner that lists curl as a fallback. HA Add-on deployment block (derivedDeployment === 'ha-addon'): - Add an intro line stating the auto-discovery / auto-secret / auto-token behaviour so users understand why this path is the easiest. - Refine the "check the logs" step to specify the exact nav path (Settings → Add-ons → Home Assistant MCP Server → Logs) and clarify these are the add-on logs, not main HA logs. - Show an example log line with the actual URL shape so users know what to look for. - Add a tiny footer note: port 9583 default, 128-bit secret path, persisted across restarts. Docker deployment block (derivedDeployment === 'docker'): - Add a "Container management" details block alongside the existing "Production hardening (docker compose)" details: docker logs -f, stop, rm, pull. Same details/summary pattern as the hardening one. Cloudflared persistent-tunnel block (state.proxy === 'cloudflared', inside the existing "Want a permanent URL?" callout): - Append the ~/.cloudflared/config.yml YAML so users have the actual ingress mapping needed to wire the named tunnel to localhost:8086. Keeps the existing 4-command sequence and adds the config file next to it (the body's order: create → config → route → run). Build verified clean (npm run build, 7 pages, 17.18s). * refactor(site): convert content collections from .md (frontmatter-only) to .yaml Closes the loop on homeassistant-ai#1097 / homeassistant-ai#1106. The bodies under site/src/content/{clients,platforms,connections,deployment}/*.md were unrendered (the wizard's only consumer reads c.data via getCollection and never invokes .render() / .body). Once the body-content migration landed in setup.astro, faq.astro, and guide-macos.astro, the .md shell stopped serving any purpose. Converting to .yaml is the honest representation of what these files have always been since the wizard's introduction: pure metadata. No fake markdown wrapper, no dangling --- markers, the file is the data. Astro's content-layer glob loader supports YAML natively, so this is a transparent swap from the consumer's perspective — state.client.id, state.client.configLocation, etc. all resolve the same way. Changes: - 31 .md files → 31 .yaml files (19 clients/, 4 platforms/, 3 connections/, 5 deployment/). Each .yaml is the unwrapped frontmatter content. - content.config.ts: 4 loader patterns updated **/*.md → **/*.yaml (one per collection). - setup.astro: cross-ref comment updated opencode.md → opencode.yaml. Net deletion: ~1900 lines (the unrendered body content) across the two commits that delivered the migration sweep. Build verified clean (npm run build, 7 pages, 4.78s). * docs: add Site Content Collections section to AGENTS.md Documents the post-homeassistant-ai#1097/homeassistant-ai#1106 contract of site/src/content/*/*.yaml so future contributors don't reintroduce unrendered body prose into the metadata files. Names the rendered surfaces where setup content actually lives (setup.astro, faq.astro, guide-*.astro), explains the "don't author body content here" rule, and points back to the issue trail for context. * refactor(site): inline wizard data into setup.astro, drop content collections Per-issue resolution of homeassistant-ai#1097 / homeassistant-ai#1106: the four Astro content collections (clients, platforms, connections, deployment) under site/src/content/ existed solely to feed metadata to setup.astro via getCollection. Body content was unrendered (verified in homeassistant-ai#1097), and after the migration sweep that put the ~110 unique setup-instruction nuggets into setup.astro, faq.astro, and guide-macos.astro (commits 70361d1..1043e3a), the collection files held only frontmatter — pure metadata duplicating what the wizard already needed at hand. This commit collapses the indirection: - All 31 entries (19 clients + 4 platforms + 3 connections + 5 deployments) inlined directly into setup.astro as four pre-sorted JS arrays at the top of the frontmatter block. Same field names, same order, no shape change visible to the wizard's downstream code beyond dropping the intermediate `c.data` accessor (the markup blocks for the picker tiles are updated to use `client.transports` / `client.logo` / etc. instead of `client.data.transports`). - Logos pre-baked through `withBase()` via a single `.map()` so the template doesn't need the helper anymore. - `import { getCollection } from 'astro:content'` removed from setup.astro. - Entire `site/src/content/` directory deleted (4 subdirs, 31 yaml files). - `site/src/content.config.ts` deleted (no collections to define). - AGENTS.md "Site Content Collections" section rewritten as "Setup Wizard" — single source of truth, no separate content-collection layer. Net effect: contributors edit one file (setup.astro) instead of needing to coordinate edits across a metadata yaml + a wizard branch. The unrendered-body trap that homeassistant-ai#1097 was filed about can no longer occur, since there is no body surface to author into. Build verified clean (npm run build, 7 pages, 3.46s). Closes homeassistant-ai#1097. Closes homeassistant-ai#1106. * feat(site): migrate remaining legitimate nuggets into wizard Follow-up to the body-content sweep — items previously marked deferred that on closer review represent real user-facing setup needs (homeassistant-ai#1097, homeassistant-ai#1106). Zed inline data: - Multi-line configLocation (macOS/Linux ~/.config path, Windows %APPDATA%\\Zed\\settings.json, project-specific .zed/settings.json override). Now renders as a 3-bullet list via the Patch76 S1 fix. - New clientNote: "Settings file supports JSON with // comments" (a Zed-specific quirk users coming from strict JSON tools hit). Cloudflare Tunnel proxy block: - Add a one-line intro callout naming the three concrete advantages the body authored: no port forwarding, free tier, works behind CGNAT. Plus the Cloudflare Access-on-top option for SSO/policy. Custom Reverse Proxy block: - Add Caddy-specific note (automatic HTTPS via Let's Encrypt, needs public IP / dynamic DNS for ACME) and Nginx note pointing at certbot for cert issuance. The existing block listed Caddy / Nginx / Traefik in passing but didn't surface their setup differences. Continue (configFormat=yaml, !isStdio): - "Alternative: With Authentication Headers" instruction-block with the requestOptions.headers shape. Continue's primary YAML has no auth slot today, so users with Bearer-protected MCP servers need this variant explicitly. Gemini CLI (configFormat=cli, !isStdio): - "Alternative: With Authentication Headers" instruction-block. The `gemini mcp add --transport http` command has no flag for custom headers, so authenticated remote servers require manual ~/.gemini/settings.json (or .gemini/settings.json project-level) edits. Show the httpUrl + headers JSON shape. Codex (configFormat=cli, !isStdio): - "Alternative: With Authentication Headers" instruction-block with the TOML [mcp_servers.<name>.headers] shape — same rationale as Gemini, codex mcp add has no header flag. - Existing Codex Management Commands block extended with a one- line note that the TOML config file is shared between the CLI and the Codex Desktop app (Settings → MCP). Build verified clean (npm run build, 7 pages, 3.73s). * docs(agents): hoist deprecated content-collection warning to top of section The Setup Wizard section's history line buried the warning that the deprecated site/src/content/*.md path is gone. Future contributors who encounter references to that path in old issues, PRs, comments, or blog posts need to know explicitly that the files no longer exist — that was the original purpose of touching AGENTS.md per homeassistant-ai#1097's resolution discussion. Replaces the trailing one-line history with a callout block at the top of the section: states the path is gone, explains what was deprecated and why, and forbids re-creating the content collection (which is the exact regression mode homeassistant-ai#1097 was filed to prevent). * docs(agents): drop AGENTS.md additions Master AGENTS.md never mentioned site/src/content/*.md files, so the "Setup Wizard" section I'd been editing was net-new content (a warning against a baseline AGENTS.md never documented). The right move is to leave AGENTS.md untouched — anyone investigating an old reference to the deprecated path can read git history or this PR's commit log. Reverts c6957d4 + 480c62c (the AGENTS.md hunk only) + 6f1e4ce in their entirety. * docs(agents): add Setup Wizard section as a pointer for future site work Short navigational note: where the wizard data lives (4 inline arrays at the top of setup.astro), where instruction templates live (JS template literals keyed off state.client.id / etc. in the <script> block), where cross-cutting troubleshooting content goes (faq.astro, guide-*.astro), and the 3-step recipe for adding a new client / platform / connection / deployment. Informational only — no warnings, no deprecation callouts (master never documented the deprecated content-collection path, so flagging its absence here would be inventing context). Future contributors get the breadcrumbs they need without AGENTS.md trying to retroactively police a path that was never documented in the first place. * fix(site): address Gemini review + comment-analyzer findings Six small follow-ups from the review pass on PR homeassistant-ai#1120: 1. Drop the stale `site/src/content/clients/opencode.yaml — keep aligned` cross-reference comment on the isOpenCode stdio branch (Gemini flagged this; the file no longer exists). The remaining comment about the deliberate Docker-vs-uvx asymmetry stays. 2. Gate the github-copilot-agents secure-input-prompts alternative to `&& isStdio`. The block was firing on any transport, but the inputs[]/command/args/env example is stdio-shaped — for HTTP/SSE users the wizard's primary already emits the correct {url, transport:"sse"} form, so the alternative was incoherent. 3. Replace internal "Task 1/2/3" comment prefixes (migration scratchpad numbering) with intent-only descriptions: "VS Code: alternative secure-input-prompts config (stdio path only…)" / "GitHub Copilot Agents: alternative secure-input-prompts config — only the stdio shape uses inputs[]…" / "OpenCode: management commands + corrective notes for users coming from other clients". 4. Soften two negative-existence claims about third-party CLIs (`gemini mcp add` / `codex mcp add` "does not accept custom headers") — these were comment-rot vectors. Now just present the manual config-file path as the route for Bearer-token auth without explicitly comparing it to the CLI flag set. 5. Drop two tautological sub-comments inside the antigravity stdio branch ("Docker: add env var as an additional -e flag" / "uvx: merge into env object"). The code below each was self-evidently doing exactly that. 6. Trim the Antigravity "EOF errors" FAQ bullet from "Use absolute paths… does not resolve relative paths in the same working directory as your shell" to just "Use absolute paths… not relative paths" — matches the source body's wording. The working-directory specifics were authored copy not in the source. Build verified clean (npm run build, 7 pages, 19.09s). * fix(site): docker run command was malformed when secret path enabled The docker-deployment branch built a one-line `docker run` command but constructed the secret-path fragment with a trailing `\<newline>` (intended as a shell-line-continuation) and then `.trim()`-ed only the newline before splicing it back into the same line. The trailing backslash survived into the rendered command: ... -e MCP_SECRET_PATH=/private_xxx \ ghcr.io/... Bash parses `\<space>` as an escaped space, joining the leading whitespace onto the next token — so docker was invoked with an image argument of " ghcr.io/homeassistant-ai/ha-mcp:latest" (note the leading space), which fails with `invalid reference format`. Users who enabled the secret path on the Docker deployment got a broken copy-paste command. Drop the trailing `\\\n` from secretPathEnv (no continuation needed inline) and remove the matching .trim() at the splice site. The emitted command is now: docker run -d --name ha-mcp -p 8086:8086 -e ... -e ... -e MCP_SECRET_PATH=/private_xxx ghcr.io/... Pre-existing bug in master (predates homeassistant-ai#1106). Boy Scout fix bundled here since the surrounding deployment block is being heavily touched by this PR. Build verified clean (npm run build, 7 pages, 41s). * feat(site): close audit-table gaps + Patch76 review Addresses Patch76's CHANGES_REQUESTED review on PR homeassistant-ai#1120 plus the audit-table walk-through showed several legit nuggets I'd skipped on the first pass. Adding all of them now. Patch76 #1 — Cloudflared HA-addon `additional_hosts` block: Added a <details> "Running on Home Assistant OS? Use the Cloudflared add-on" section to the existing Cloudflare Tunnel block, with the brenner-tobias add-on badge link and the `additional_hosts:` YAML pointing at port 9583. Patch76 #2 + #3 — github-copilot-agents org-deployment notes: New "Org & Repository Deployment" instruction-block (any transport) documenting: - Repository-wide config via Settings → Copilot → Coding agent → MCP configuration on github.com (applies to all users with repo access, alternative to per-user .vscode/mcp.json) - Operational prerequisite: the "MCP servers" policy must be enabled for the org/enterprise — admins disable it by default. JetBrains — extended clientNote with the "Import from Claude" button tip (Settings → Tools → AI Assistant → MCP Servers) for users migrating from Claude Desktop. Audit-table gaps closed: - Antigravity: new instruction-block with the UI nav steps to the raw config editor (... menu → MCP Servers → Manage MCP Servers → View raw config) plus an HTTP-transport caveat (gated to !isStdio) about "connection closed" / "SSE stream failed to reconnect" errors with a recommendation to switch to stdio. - Codex: extended Management Commands block with a Codex Desktop walkthrough (Settings → MCP → Add Server with field names) and an OAuth-2.0-for-remote-servers note (gated to !isStdio). - VS Code: secure-input-prompts block now also mentions the vscode:mcp/install?<config> deep-link install pattern. - Copilot CLI: Notes & extras section after both the stdio and HTTP step blocks — Server Type legend (1/2/3/4 for Local/STDIO/HTTP/SSE), KEY=VALUE env-var format, * vs comma-list Tools format, COPILOT_HOME override, /mcp interactive command, "GitHub MCP server included by default" reminder. - OpenCode: Management Commands block extended with OPENCODE_CONFIG env-var path override, project-vs-global config-merge precedence (with link to opencode.ai/docs), {env:VAR} headers interpolation pattern for Bearer auth, oauth: false opt-out flag, home-assistant_* tool namespacing, and a 92+-tools context-size warning recommending a dedicated OpenCode agent for HA-heavy workflows. - Webhook Proxy: "How It Works" section before the install steps with the routing chain (AI client → HTTPS → reverse proxy → HA :8123 → webhook /api/webhook/<id> → MCP add-on). Plus a comparison table vs Cloudflare Tunnel (setup/cost/routing/best-for) after the steps. - Continue: clientNote with the Agent-Mode-required gotcha — MCP only works when Continue is in Agent Mode (use the agent selector near the chat input). - Claude.ai: clientNote with the Pro/Max/Team/Enterprise subscription requirement and the "Search and tools" button tip for per-conversation tool toggling. - Claude Code: clientNote noting config changes take effect immediately (no restart needed) — useful contrast with the restart-required clients. - Linux quick-test: new FAQ item ("Test ha-mcp without configuring a client") with the public-demo-server one-liner from the deleted linux.md body. TOC entry added. - AGENTS.md: appended a sentence to the "adding a new entry" recipe noting that arrays should be kept ordered by `order` (the wizard renders in array order without re-sorting). Intentional skips (with reason, in case anyone re-audits): - JetBrains Node 18+ requirement: only relevant for npm-based MCP servers; ha-mcp uses uvx, so this is a non-applicable constraint. - JetBrains 2025.2+ built-in MCP server: about the IDE itself acting as an MCP server, not relevant to ha-mcp client setup. - Zed Bearer-header HTTP shape: Zed is in stdioOnlyClients, so the wizard routes Zed users through mcp-proxy for HTTP — the isZed HTTP branch in the JSON builder is dead code for ha-mcp users going through the wizard. Adding a Bearer-auth alternative would contradict the httpNote. - uvx Python 3.10+ claim: source body was wrong (faq says 3.13+, matches pyproject.toml requires-python = "==3.13.*"). Body deletion auto-resolved. Build verified clean (npm run build, 7 pages, 18.4s). * fix(site): address idiot-check findings on third-party UI/path claims Five accuracy fixes flagged by a fresh-eyes pass on the audit-table-gap commit (11d15d2). All are doc-text corrections, no logic changes. 1. Cloudflared HA-addon `additional_hosts.service` — `localhost` was wrong. The Cloudflared add-on tunnels from inside its own container, so `localhost` doesn't reach the MCP add-on. Switched the example to `homeassistant.local:9583` and the footnote to spell out that users should match whatever IP/hostname they see in the MCP add-on logs (since the working value depends on their HA networking). 2. github-copilot-agents Repository-wide config menu path — added a parenthetical noting the node is sometimes labeled "Cloud agent" depending on UI version (GitHub renamed it; both are in the wild). 3. github-copilot-agents org policy name — was `"MCP servers"`, the canonical full label is `"MCP servers in Copilot"`. Also softened the unsourced "admins disable it by default" assertion to "your admin may have it scoped or disabled." 4. JetBrains AI Assistant menu label — official path is `Settings → Tools → AI Assistant → Model Context Protocol (MCP)`, not "MCP Servers". Updated both configLocation and the clientNote "Import from Claude" reference. 5. Claude Code clientNote — "no restart needed" overstated it. New servers added via `claude mcp add` may not appear in an active session until /mcp reconnect or a new session. Reworded to reflect actual behaviour. Build verified clean (npm run build, 7 pages, 18.8s). --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
kingpanther13
pushed a commit
that referenced
this pull request
May 8, 2026
…s 1/4/5 + Boy-Scout e/f/g) Addresses kingpanther13's PR homeassistant-ai#1168 review test-coverage findings: Blocker #1 — DEEP_SEARCH_KEYS scenes: - Added 'scenes' to the test-helper tuple at tests/src/e2e/tools/test_deep_search.py:14; every default-call deep_search test had been silently skipping the scenes bucket because the iteration source didn't include it. - Updated test_deep_search_all_types to count scenes in the result tally. - Generalised test_deep_search_limit's count to iterate DEEP_SEARCH_KEYS so any future bucket addition flows through automatically. - Added test_deep_search_default_includes_scenes locking the contract that a default-call response must include the scenes bucket. Blocker #4 — DELETE blocklist coverage: - Added test_api_post_blocks_scene_config_delete next to test_api_post_blocks_scene_config_write. Sandbox exposes only api_post/api_get (no api_delete), so the test exercises the delete- flavored attack pattern (empty-body POST) and asserts the path-prefix blocklist still rejects it. Blocker #5 — 404 → ENTITY_NOT_FOUND: - ha_config_get_scene's exception catch now passes entity_id alongside scene_id so the helper's 404 classifier picks ENTITY_NOT_FOUND instead of the generic RESOURCE_NOT_FOUND fallback. Scenes are entities, and agents branching on the not-found code lose the scenes-are-entities signal otherwise. - Added TestSceneRestClientErrorMapping with two unit tests: test_get_scene_404_surfaces_as_entity_not_found locks the 404 path, test_set_scene_400_surfaces_with_scene_id_context covers Boy-Scout g (upsert 400 path — highest-likelihood scene failure when an LLM submits malformed entity state). Boy-Scout — additional test gaps closed: - (e) E2E rename scenario: test_scene_rename_decouples_entity_id_from_storage_key in tests/src/e2e/workflows/scenes/test_lifecycle.py exercises the full create → get → python_transform → remove path against a scene whose storage key and entity_id slug diverge. Locks the _resolve_scene_entity_id contract end-to-end against the no-warning invariant the BAT validation surfaced as broken before it landed. - (f) Phase 2.5 registry-augmentation: TestSceneRegistryAugmentation in tests/src/unit/test_smart_search_scene_phase25.py with two tests — one locking the alias-on-divergence behaviour, one verifying that a failing entity-registry list does not break the scene branch and falls through to storage-id keyed lookup. - (g) upsert 400 path: covered alongside Blocker #5 in TestSceneRestClientErrorMapping (one test class for both). Plus partial-failure coverage for Boy-Scout d (silent failures surfacing, landed in the previous commit): TestSceneFetchPartialFailure with test_per_id_failures_surface_partial and a test_no_partial_flag_when_everything_clean negative case so the 'partial' field doesn't accidentally always-be-set. ruff + mypy clean; full unit suite passes 1982 tests (was 1976; +6 new unit tests across the new test classes). E2E tests run in CI only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kingpanther13
added a commit
that referenced
this pull request
May 18, 2026
…n deferral gate (homeassistant-ai#1359) * docs: merge Boy Scout Rule + Handling Discovered Improvements; tighten deferral gate Merge AGENTS.md's "Handling Discovered Improvements" and "Boy Scout Rule" sections into a single "Boy Scout Rule — Handling Discovered Improvements" section that defaults to fix-in-place and tightens the bar for follow-up issues/PRs. Key changes: - One consolidated size-scale table (folds the test-quality gradient into examples on the Small/Mid/Large rows) - Explicit ~100-line size threshold: under = bundle; over = ask author - Anti-noise gate with three sub-tests on point #1 (sibling-pattern, named alternatives, review-surface) - Weasel-phrase list ("post-merge follow-up", "nice to have", etc.) as a re-check cue, with explicit "scope is the author's call, not yours" - Non-follow-up list (line-count items, sweeps, drift fixes, parity) - Code-review bot suggestions clarified: apply inline or dismiss unless the author confirms it's a large, out-of-scope change - One-line cross-reference added in §"PR Execution Philosophy" PR template: - Strict gate comment on "Future improvements" — leave empty unless all three criteria met; explicit list of what NOT to defer .gemini/styleguide.md: - New "Non-blocking suggestions and scope" section: scope is the author's call; never request follow-ups unless grossly unrelated; surface legitimate findings; don't bucket as "post-merge follow-up" * docs: refine Boy Scout Rule + reframe PR template; sync skill files Address review feedback from PR homeassistant-ai#1359 self-review: AGENTS.md merged section: - 200-line heuristic reframed as "should-I-ask" trigger, not a bundling cap (bundling at any size is fine if not grossly out of scope) - Restored explicit "Never open a follow-up PR or issue without user approval" - Qualified weasel-phrase "Out of scope for this PR" to distinguish from the prescribed ask-the-user template (substring overlap) - Mid-sized example changed from "Refactor of sibling function" to "Adding a new helper module that doesn't exist yet" (avoid contradiction with anti-noise gate sub-test 1(a)) - Added "without refactoring surrounding code" qualifier to test bullet - Added regression-risk caveat for fix-in-place sweeps - "Author" → "user" throughout; one-line blurb defining user = PR author - Softened gate sub-test 1(b) with mechanical-migration carve-out - Folded NOT-follow-up list into Small row examples (design cleanup) - "Scope is user's call" consolidated to one place with "same rule for issues" paragraph folded in - Shrunk "Code-review bot suggestions" paragraph to one sentence pointing to the styleguide section (single source of truth for bot-side rule) - Cross-reference at end of PR Execution Philosophy converted to Markdown anchor link PR template: - Reframed from 3-condition self-checklist to user-confirmed-deferral framing (removed all line-count language) - Direct AI agents to ASK the user when uncertain rather than self-bucketing - Kept the do-not-list bullets (point-of-use reminder) .gemini/styleguide.md: - Title-cased H2 to match siblings - Collapsed two-bar conflict ("grossly unrelated" vs "likely out of scope") into a single "never unilaterally file a follow-up" rule - Canonicalized the ask-the-user template wording with AGENTS.md - "Author" → "user" Skill files (.claude/skills/): - issue-to-pr-resolver: removed `## Future improvements` from PR body template; rewrote "Discovered improvements" rule to defer to AGENTS.md - my-pr-checker: rewrote Step 6 "Final Report" to default to fix-in-place and only populate `## Future improvements` with explicit user confirmation * docs: trim Boy Scout section bloat; fix stale Test Coverage line; restructure table Addresses second idiot-check pass on PR homeassistant-ai#1359: HIGH — fix stale "open an issue instead" line: - Test Coverage Requirements section had a leftover that directly authorized the AI to open follow-up issues whenever refactoring "would significantly expand PR scope," contradicting the new Boy Scout Rule. Replaced with a cross-reference to the gate. Bloat cuts (~70 words, ~8% reduction): - Removed user-definition blockquote (terminology drift mitigated by the "end-user-facing" rewording below) - Removed "Sometimes a follow-up is genuinely necessary" preamble — the anti-noise gate above already implies legitimate follow-ups exist - Removed restating sentence "The same rule applies to filing follow-up issues..." after the verification template — already explicit at top of the section Table restructure: - Pulled the 13-item "Small" examples list out of the table cell into a bulleted list below the table (was 695 chars in one comma-separated cell) - Mid-sized and Large rows keep their shorter examples inline as parentheticals Terminology fix: - "user-facing or maintainer benefit" → "end-user-facing or maintainer benefit" in anti-noise gate condition 2 (only place "user" was used in the end-user sense within this section) .gemini/styleguide.md: - Added reciprocal cross-reference back to AGENTS.md § Boy Scout Rule — Handling Discovered Improvements for the author/agent-side rule --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
kingpanther13
added a commit
that referenced
this pull request
May 23, 2026
…t infrastructure Outcome of going through all 60 findings from Gemini Code Assist + pr-review-toolkit (code-reviewer, pr-test-analyzer, silent-failure- hunter, comment-analyzer). 3 wrong (skipped: PATH-resolved node binary mis-flagged as hardcoded; project-relative esbuild path mis-flagged as hardcoded; theoretical FakeBroadcastChannel constructor-throw). Remaining real items addressed: Harness: - vm.runInContext replaces window.eval / indirect eval to clear the "no eval()" style-guide flag (Gemini #1, #2). - Timer-callback and broadcast-listener throws now record into the errors list instead of being silently swallowed (homeassistant-ai#30, homeassistant-ai#32). - SAFETY_CAP exhaustion records a clear "runaway setInterval" error instead of breaking silently (#4, homeassistant-ai#31). - Non-navigation jsdomErrors route to errors (not console) so tests asserting `not result.errors` catch them (homeassistant-ai#38). - Transpile failure short-circuits init eval to avoid cascading syntax errors from un-transpiled TS (homeassistant-ai#34). - FakeBroadcastChannel.postMessage now delivers to peer same-name channels in the same context per spec (#5). - Time-faked surface documented accurately (Date.now / setTimeout / setInterval only; new Date / performance.now still wall-time) (homeassistant-ai#46). - New broadcastChannelUnavailable param simulates the `typeof BroadcastChannel === 'undefined'` browsing context so the production null-guard branch is exercised (homeassistant-ai#15). - Dead comments and rot-prone duplications removed (homeassistant-ai#47, homeassistant-ai#49, homeassistant-ai#51, homeassistant-ai#56, homeassistant-ai#57, homeassistant-ai#58, homeassistant-ai#59, homeassistant-ai#66). extract_astro_vars.mjs: - vm.runInContext replaces (0, eval) (Gemini #2). - Multi-line `import { a, b } from 'x';` now stripped robustly (#7). - Eval errors wrapped with the source path for actionable failures (homeassistant-ai#35). _js_harness.py: - Wrong test file name and workflow path in docstring fixed (homeassistant-ai#41, homeassistant-ai#42). - _strip_astro_frontmatter raises ValueError when frontmatter opens but never closes (homeassistant-ai#36). - discover_script_surfaces raises when site/src/ is missing instead of silently producing partial results (homeassistant-ai#37). - extract_script_body accepts source_label for actionable errors (homeassistant-ai#40). - Astro `<script lang="js">` is no longer mis-tagged as TypeScript (#9). - Inert chr(92) Windows backslash replace removed (homeassistant-ai#14). - Field docstrings on ScriptSurface trimmed to the one that earns its keep (homeassistant-ai#52). - _PY_RENDERERS registry refactor + accurate enumeration comment (homeassistant-ai#45). test_settings_ui_js_behavior.py: - Rot-bait PR/issue numbers removed from module docstring (homeassistant-ai#43). - _TOP_LEVEL_ELEMENT_IDS + import-time drift check replaces the "refresh this manually" comment (homeassistant-ai#55). - _assert_clean_init helper called at the top of every test so init failures surface as init errors, not as misleading "side effect didn't fire" failures (homeassistant-ai#33). - 4xx restartBtn assertion now reads disabled state via JS and snaps to body.dataset instead of OR-shortcircuiting against a wiped DOM (homeassistant-ai#27). - New test_script_boots_without_broadcastchannel_global covers the null-guard branch (homeassistant-ai#15). - Assertion-restating comments removed (homeassistant-ai#60). test_astro_setup_js_behavior.py: - Rot-bait homeassistant-ai#1422 reference removed from module docstring (homeassistant-ai#44). - _section_has_hidden_class replaces fragile substring slicing (#6). - test_initial_state_only_client_section_visible now asserts on the promised visibility, not just absence of errors (homeassistant-ai#26). - Per-client smoke now captures config-output text AND instructions HTML into body.dataset and asserts on non-empty content, catching a typo that drops the whole per-client branch (homeassistant-ai#21). test_astro_tools_js_behavior.py: - _card_class helper replaces ±200-char substring slicing (#12). - test_design_mode_toggle now asserts design-only elements lose 'hidden' class, not just the button label flip (homeassistant-ai#25). - New tests cover .filter-btn / .cat-btn / .size-filter-btn / group-category|file|none / sort-alpha / expand-all wiring (homeassistant-ai#22, homeassistant-ai#23, homeassistant-ai#24) — the adjacent coverage gaps issue homeassistant-ai#1422 didn't name but that fit the harness's same regression-class. test_consent_form_js_behavior.py: - _build_form_dom docstring fixed (said "three", listed four) (homeassistant-ai#54). test_rendered_scripts_parse.py: - Missing-dependency skip flips to fail when CI=true so a workflow drift that drops the install step doesn't silently lose parse coverage (homeassistant-ai#29). - Subsumed-test-class reference removed from module docstring. AGENTS.md: - "60s probe windows take milliseconds" wording fixed; time-faked surface documented (homeassistant-ai#13). - Per-surface module naming guidance updated; reflects actual files (#10). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kingpanther13
added a commit
that referenced
this pull request
May 24, 2026
…ge for every rendered <script> (homeassistant-ai#1425) * test(internal): JSDOM behaviour harness + auto-discovery parse coverage for every rendered <script> Closes homeassistant-ai#1422. Adds a JSDOM harness (tests/js/harness.mjs + tests/src/unit/_js_harness.py) that drives real rendered <script> bodies through stubbed fetch / BroadcastChannel / virtual timers / DOM and reports observed side effects. A discovery walker auto-picks-up every <script> surface in the repo (src/ha_mcp/settings_ui.py, src/ha_mcp/auth/consent_form.py, every site/src/**/*.astro) so parse coverage extends as new UI surfaces ship — no registration needed. Behavioural coverage landed for the surfaces named in homeassistant-ai#1422: * settings_ui — restartInProgress concurrency guard, 4xx-suppress-reload branch, 5xx fall-through, instance_id-flip probe, BroadcastChannel restart-required + restart-initiated listeners, saveFeatureFlag JSON-parse fallback. * setup.astro — state-machine progression (local / network / remote), plus a parametrised per-client smoke that drives the wizard to config generation for every id in the real clientsData array. * tools.astro — search/filter pipeline + design-mode toggle (TypeScript; esbuild strips types in the harness before eval). * Layout.astro — copy-button idempotency across re-init. * consent_form — submit handler disable + spinner state. The legacy TestRenderedHTMLJsSyntax in test_settings_ui.py is removed — the auto-discovery parse test in test_rendered_scripts_parse.py subsumes it (and extends to the four other surfaces it never covered). CI: unit-tests job in pr.yml installs nodejs + jsdom + esbuild via apt-get / npm ci. Local devs without tests/js/node_modules/ get clean skips, matching the original parse guard's behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): skip Astro frontmatter in script extraction; drain microtasks before clock advance CI surfaced two harness bugs the local smoke-tests didn't catch: 1. `extract_script_body` and the discovery walker greedily matched the first `<script>` substring in the source, which in setup.astro is actually a frontmatter comment: `// below in the <script> block keyed off the entry's id.` That made the "script body" start mid-frontmatter and the extracted text wasn't valid JS — esbuild and JSDOM both rejected it with "Unexpected identifier 'keyed'". Fix: strip the `--- ... ---` Astro frontmatter block before searching for `<script>` tags. Plain .py and .html sources have no frontmatter and pass through unchanged. 2. `clock.advance(settleMs)` returned immediately when no timers were yet scheduled, but the script under test often awaits a chain of stubbed-fetch promises BEFORE hitting its first `setTimeout`. With only one microtask drain between eval and advance, those promises hadn't resolved yet, so no timers existed, advance was a no-op, and the script stayed suspended — `restartAddon`'s POST to /api/settings/restart never fired and the `alert(msg)` in the 4xx branch never ran. Fix: drain microtasks aggressively at the start of advance() so pending promises get to schedule their timers, and drain again when the timer queue temporarily empties (a promise resolution may queue new timers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(internal): expand initial DOM fixtures to cover every top-level addEventListener target CI surfaced this via the test_5xx test (the only one whose assertion included the harness errors list): the settings_ui script aborts during init at `document.getElementById('backupRefresh').addEventListener(...)` because the test DOM is missing the backup table / modal markup. With init aborted, the invoke step never runs — `restartAddon` is never called, POSTs never fire, `alert()` never runs, and all three restart- flow tests silently fail. The setup.astro tests had the same shape: `generateConfig` queries `config-section` (distinct from `section-config`) to show/hide the inner code block. Without it, the proxy click handler in the remote- flow test threw and the `document.body.dataset.beforeProxy` assignment never landed. Fixes: - settings_ui MIN_DOM now includes backupBulkDelete, backupDomain, backupEntity, backupList, backupRefresh, backupState, featuresBody, modalBackdrop / modalBody / modalClose / modalTitle. Set built from `grep -h "document.getElementById" settings_ui.py` so future top-level handlers will surface as the same pattern. - setup.astro DOM now includes config-section alongside section-config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): run JSDOM eval at global scope; capture body attrs in dom snapshot Two harness bugs the prior CI rounds didn't surface until init-stage crashes were resolved: 1. Wrapping the rendered script in an `async () => { ... }()` IIFE confined top-level `function` declarations to the IIFE scope. `function restartAddon() {...}` never landed on `window`, so `invoke: "window.restartAddon();"` threw `is not a function`. A real browser hoists inline-script function decls to the global window — match that by running prelude + script body at global scope and keeping the IIFE for `invoke` alone (so awaits inside invoke still work). 2. `document.body.innerHTML` returns body's children but not body's own attrs, so tests that wrote `document.body.dataset.foo = 'bar'` as a side-channel for in-page state had no way to assert on it — `result.dom` came back without the attr. Serialise `document.documentElement.outerHTML` instead so html/head/body tags and their own attributes round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(internal): sequence /api/settings/info responses for 5xx restart probe The test_5xx flow hits the info endpoint three times — loadTools init, restartAddon's pre-POST baseline capture, and _probeAddonRestarted after the POST. The old single-response fixture returned the SAME instance_id every time, so the probe never saw the flip and looped until timeout, leaving reloads=0. Adds a `responses: [...]` shape to the harness fetch_map: each match on a URL pattern advances a per-pattern counter; the last entry sticks after exhaustion (matches "the addon came back online and stays online"). Test now provides baseline → baseline → flipped so the probe terminates with restarted=true and the reload fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(test): cache apt downloads and node_modules for the unit-tests job The unit-tests "Install git and Node.js" step was 44 s — almost all of it network download of the nodejs / npm .deb. The "Install JS test dependencies" step is 1 s when node_modules is fresh but can grow as deps change. - Cache /var/cache/apt/archives keyed on a stable string (apt package set rarely changes). Disable docker-clean and set Keep-Downloaded- Packages so the cached .debs survive install for the next run. apt install still runs (unpacks from local cache, ~3-5 s) but skips the network leg. - Cache tests/js/node_modules keyed on package-lock.json so dep bumps invalidate cleanly. `npm ci` short-circuits when the tree matches. Expected first-cold-cache run: unchanged (~45 s install). Cache hits: ~5 s for both steps combined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(internal): address Gemini + pr-review-toolkit findings on JS test infrastructure Outcome of going through all 60 findings from Gemini Code Assist + pr-review-toolkit (code-reviewer, pr-test-analyzer, silent-failure- hunter, comment-analyzer). 3 wrong (skipped: PATH-resolved node binary mis-flagged as hardcoded; project-relative esbuild path mis-flagged as hardcoded; theoretical FakeBroadcastChannel constructor-throw). Remaining real items addressed: Harness: - vm.runInContext replaces window.eval / indirect eval to clear the "no eval()" style-guide flag (Gemini #1, #2). - Timer-callback and broadcast-listener throws now record into the errors list instead of being silently swallowed (homeassistant-ai#30, homeassistant-ai#32). - SAFETY_CAP exhaustion records a clear "runaway setInterval" error instead of breaking silently (#4, homeassistant-ai#31). - Non-navigation jsdomErrors route to errors (not console) so tests asserting `not result.errors` catch them (homeassistant-ai#38). - Transpile failure short-circuits init eval to avoid cascading syntax errors from un-transpiled TS (homeassistant-ai#34). - FakeBroadcastChannel.postMessage now delivers to peer same-name channels in the same context per spec (#5). - Time-faked surface documented accurately (Date.now / setTimeout / setInterval only; new Date / performance.now still wall-time) (homeassistant-ai#46). - New broadcastChannelUnavailable param simulates the `typeof BroadcastChannel === 'undefined'` browsing context so the production null-guard branch is exercised (homeassistant-ai#15). - Dead comments and rot-prone duplications removed (homeassistant-ai#47, homeassistant-ai#49, homeassistant-ai#51, homeassistant-ai#56, homeassistant-ai#57, homeassistant-ai#58, homeassistant-ai#59, homeassistant-ai#66). extract_astro_vars.mjs: - vm.runInContext replaces (0, eval) (Gemini #2). - Multi-line `import { a, b } from 'x';` now stripped robustly (#7). - Eval errors wrapped with the source path for actionable failures (homeassistant-ai#35). _js_harness.py: - Wrong test file name and workflow path in docstring fixed (homeassistant-ai#41, homeassistant-ai#42). - _strip_astro_frontmatter raises ValueError when frontmatter opens but never closes (homeassistant-ai#36). - discover_script_surfaces raises when site/src/ is missing instead of silently producing partial results (homeassistant-ai#37). - extract_script_body accepts source_label for actionable errors (homeassistant-ai#40). - Astro `<script lang="js">` is no longer mis-tagged as TypeScript (#9). - Inert chr(92) Windows backslash replace removed (homeassistant-ai#14). - Field docstrings on ScriptSurface trimmed to the one that earns its keep (homeassistant-ai#52). - _PY_RENDERERS registry refactor + accurate enumeration comment (homeassistant-ai#45). test_settings_ui_js_behavior.py: - Rot-bait PR/issue numbers removed from module docstring (homeassistant-ai#43). - _TOP_LEVEL_ELEMENT_IDS + import-time drift check replaces the "refresh this manually" comment (homeassistant-ai#55). - _assert_clean_init helper called at the top of every test so init failures surface as init errors, not as misleading "side effect didn't fire" failures (homeassistant-ai#33). - 4xx restartBtn assertion now reads disabled state via JS and snaps to body.dataset instead of OR-shortcircuiting against a wiped DOM (homeassistant-ai#27). - New test_script_boots_without_broadcastchannel_global covers the null-guard branch (homeassistant-ai#15). - Assertion-restating comments removed (homeassistant-ai#60). test_astro_setup_js_behavior.py: - Rot-bait homeassistant-ai#1422 reference removed from module docstring (homeassistant-ai#44). - _section_has_hidden_class replaces fragile substring slicing (#6). - test_initial_state_only_client_section_visible now asserts on the promised visibility, not just absence of errors (homeassistant-ai#26). - Per-client smoke now captures config-output text AND instructions HTML into body.dataset and asserts on non-empty content, catching a typo that drops the whole per-client branch (homeassistant-ai#21). test_astro_tools_js_behavior.py: - _card_class helper replaces ±200-char substring slicing (#12). - test_design_mode_toggle now asserts design-only elements lose 'hidden' class, not just the button label flip (homeassistant-ai#25). - New tests cover .filter-btn / .cat-btn / .size-filter-btn / group-category|file|none / sort-alpha / expand-all wiring (homeassistant-ai#22, homeassistant-ai#23, homeassistant-ai#24) — the adjacent coverage gaps issue homeassistant-ai#1422 didn't name but that fit the harness's same regression-class. test_consent_form_js_behavior.py: - _build_form_dom docstring fixed (said "three", listed four) (homeassistant-ai#54). test_rendered_scripts_parse.py: - Missing-dependency skip flips to fail when CI=true so a workflow drift that drops the install step doesn't silently lose parse coverage (homeassistant-ai#29). - Subsumed-test-class reference removed from module docstring. AGENTS.md: - "60s probe windows take milliseconds" wording fixed; time-faked surface documented (homeassistant-ai#13). - Per-surface module naming guidance updated; reflects actual files (#10). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): seed data-transports on wizard tiles; add NODE/ESBUILD env overrides; FakeBroadcastChannel ctor guard CI surfaced a real test-fixture bug exposed by the new jsdomError → errors routing: setup.astro's connection-click handler reads `card.dataset.transports` via JSON.parse, but the wizard DOM stubs were emitting `<button data-client="...">` without the matching `data-transports` attribute. JSON.parse(undefined) threw "undefined is not valid JSON" on the jsdomError channel, which the previous silent-handling code dropped — now correctly surfaced as a test failure. Fix: serialise the real `transports` array from the clientsData entry onto each tile. Also addressing the items previously marked deferred / skipped during the Gemini + pr-review-toolkit triage: - NODE_BINARY env override (Gemini #3): _node_binary() helper checks the env var before falling back to PATH-resolved `node`. Default unchanged. - ESBUILD_BINARY env override (Gemini #8): _esbuild_binary() returns the env-var path when set, else the project-local install. Default unchanged so the lockfile-pinned install stays the reproducible default. - FakeBroadcastChannel constructor guard (sf-hunter #L1): wraps the `new` in try/catch and records construction failures into errors before re-raising. - Trim TestWizardStateMachine class docstring (comment-analyzer homeassistant-ai#50). - Tighten the info-call enumeration comment in the 5xx test to describe the harness's "last entry sticks" semantics rather than pinning a specific call count (comment-analyzer homeassistant-ai#48). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): stub layout-dependent JSDOM APIs (scrollIntoView, scrollTo, matchMedia) The new timer-callback-error routing surfaced a real JSDOM limitation: `section.scrollIntoView()` (called from the wizard's `scrollToSection` helper inside a setTimeout) is not implemented in JSDOM. Every per-client setup-flow test failed with ``timer callback: TypeError: section.scrollIntoView is not a function`` — production behaviour is fine, but the harness's noise filter wasn't distinguishing real script bugs from JSDOM-missing-API noise. Adds a defensive no-op stub for scrollIntoView (Element + HTMLElement prototypes), scrollTo on window, and matchMedia — the three most common layout-dependent APIs production UI scripts touch. Future rendered scripts that lean on other layout APIs (IntersectionObserver, etc.) can extend the list when needed. Also relaxes the per-client smoke's bare `assert not result.errors` to rely on `_assert_clean_init` (init/transpile/invoke/jsdom errors) plus the content-shape assertion. Timer-callback errors from missing JSDOM APIs are noise; the content-shape check still catches the regression class the test is named for. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): seed .tool-chevron on tool-card fixture for tools.astro expand-all test The expand-all handler queries `card.querySelector('.tool-chevron')!` (TypeScript non-null assertion). The runtime `!` doesn't actually check; chevron is null in the test DOM and `chevron.classList.add(...)` throws. Production cards include the chevron; our fixture didn't. Add it alongside `.tool-details` in `_build_tools_dom`. 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.7 (1M context) <noreply@anthropic.com>
kingpanther13
added a commit
that referenced
this pull request
May 24, 2026
…snapshot (homeassistant-ai#1428) * ci(haos-e2e): drop redundant cache save in inaddon lane homeassistant-ai#1407 made both HAOS lanes share the same actions/cache key for the 12 GB qcow2. Both jobs start in parallel on the same PR push, both try to actions/cache/save@v5 with the same key, and the loser hits the cache service's reservation guard: Failed to save: Unable to reserve cache with key haos-image-XXXXXXXXXXXXXXXX, another job may be creating this cache. But not before the loser spends ~25s zstdmt-tar'ing the 12 GB qcow2 just to be told no. Remove the save step from the inaddon lane and let the external lane own the save — both lanes still restore from the shared key on the next run. Observed in run homeassistant-ai#283 (job 77593061250): 11:26:17 cache save started 11:26:42 Failed to save: ... another job may be creating this cache 11:26:43 Cache save failed. Wall-time win in steady state: ~25s on every cache-miss run for the inaddon lane. No change to cache contents or hit rates. * perf(haos-e2e): compress qcow2 in-format before oras push + add tag_suffix dispatch input The publish workflow currently pushes the post-bake qcow2 raw via ORAS (``haos-test-image.qcow2:application/octet-stream``), which is ~12 GB on disk and downloads at GHCR's ~32 MB/s single-stream cap → 6m 24s on every cache-miss e2e run (run homeassistant-ai#283 job 77593061250). The image is dominated by sparse-zeroed space from ``qemu-img resize 32G`` plus addon Docker layers; ``qemu-img convert -c -O qcow2`` re-packs the file with the format's native zlib compression and typically shrinks it ~3–5x. The result stays a standard qcow2 (same OCI artifact-type, same consumer code path). qemu decompresses sectors lazily during VM I/O when the e2e workflow boots it — no upfront xz-style decompress step on the pull side. Workflow changes ---------------- * New ``Compress qcow2 in-format (qemu-img convert -c)`` step before the artifact upload + GHCR push. Logs ``before / after / ratio`` so the size win is visible in the workflow output. * ``workflow_dispatch`` gains a ``tag_suffix`` input (default ``latest``) so perf-iteration branches can publish to a non-``latest`` moving tag (e.g. ``:17.3-haose2eefficiency``) without disturbing the master-served image other PRs pull from. Pull-side validation -------------------- This commit only changes how the image is *published*; the e2e workflows still pull ``:HAOS_VERSION-latest`` (which is still the old uncompressed image) until commit 3 in this series flips them to the new tag. The e2e test workflows are not in this workflow file's trigger ``paths`` and are not in this PR's commit yet either, so this push does not auto-trigger an e2e run that would pull a not-yet-existing image. The next step is a manual ``gh workflow run build-haos-test-image.yml --ref perf/haos-e2e-improvements -f tag_suffix=haose2eefficiency`` to publish the compressed image at ``:17.3-haose2eefficiency``. * test(haos-e2e): TEMP point both lanes at :17.3-haose2eefficiency (perf measurement) Temporary scaffolding for PR homeassistant-ai#1428 only. The publish workflow on this branch (dispatched with tag_suffix=haose2eefficiency) just published the in-format compressed qcow2 to ``ghcr.io/homeassistant-ai/haos-test-image:17.3-haose2eefficiency`` — size 5.1 GB down from 12 GB (2.3x ratio, see run 26361263298 step 11). Pointing both test lanes at the new tag lets this PR's e2e CI exercise the compressed pull path so we can measure GHCR-pull wall-time end-to-end. The shared actions/cache entry for the existing key was deleted before this push so both lanes miss the restore step and fall through to the GHCR fetch. Final cleanup commit before this PR is marked ready will revert both ``tag=`` lines back to ``-latest``. Do not merge in this state. * revert: restore :17.3-latest tag pointer in HAOS e2e workflows Reverts the temporary tag pointer added in commit 0132186 ("test(haos-e2e): TEMP point both lanes at :17.3-haose2eefficiency"). The compressed-qcow2 measurement is done — both lanes pulled :17.3-haose2eefficiency on commit 2b's run with GHCR pull dropping from 5m 39s (commit #1, uncompressed) to 47-58s (compressed). See runs 26361656754 + 26361656780. After merge, ``build-haos-test-image.yml``'s next master-push run republishes ``:17.3-latest`` through the compression step (added in commit ba97395), so the master-served image gets the same treatment and every future PR's GHCR-pull path benefits. The ``:17.3-haose2eefficiency`` GHCR tag itself can be deleted post-merge (or left as a perf-iteration artifact — it doesn't hurt anything). This commit's CI run is expected to be SLOWER than commit 2b's because it pulls the old uncompressed ``:17.3-latest`` — that's the validation that the master-served path still works end to end, not a regression. * test(haos-e2e): address PR review feedback (correctness + comment accuracy) Aggregated fixes from three PR review agents on PR homeassistant-ai#1428 (comment-analyzer, silent-failure-hunter, code-reviewer). Correctness / safety: * ``build-haos-test-image.yml`` compression step now runs ``qemu-img check`` on the compressed file before consumers (artifact upload + oras push) see it. Catches torn writes and refcount-table corruption that ``set -e`` alone misses — a ``qemu-img convert`` killed mid-write can produce an exit-0 truncated file via the shell wrapper. * Added a 1 GiB sanity floor on the compressed file size: the baked HAOS image is gigabytes; anything smaller is corruption ``qemu-img check`` didn't catch, and we fail before the moving tag points at bad bytes. * Added a ``TAG_SUFFIX`` regex guard (``^[A-Za-z0-9._-]+$``, the OCI tag character set) in the ``Push image to GHCR`` step. A typo like ``foo/bar``, ``:evil``, or a trailing space would otherwise surface as a confused ``oras`` error several lines deep. * Dropped ``-p`` from ``qemu-img convert``: the progress bar renders as one very long line of carriage-return-overwrites in non-TTY GHA logs without adding signal the ``before/after/ratio`` echoes don't already give. Comment accuracy: * ``build_image.py`` had a stale comment claiming ``qemu-img convert -c`` only shrinks ``~7 GB → ~7 GB`` and adds 9 min — that was from a smaller pre-homeassistant-ai#1379 addon set on a non-resized qcow2. Replaced with the current state: workflow step does the compress at publish time, measured at 12 GB → 5.1 GB (2.3x) in 6m 15s on publish run 26361263298 step 11. * ``build-haos-test-image.yml`` compression step comment: fixed "sectors" → "clusters" (qcow2 unit is the 64 KiB cluster); reframed the "sparse-zeroed space" line (sparse means *absent*, not zero-filled — the wire-bytes win comes from ORAS serialising sparse holes as actual zero bytes, which the dense compressed output skips); added the "writes re-allocate uncompressed" trade-off; cited the specific publish + e2e runs that produced the measured numbers. * ``haos-e2e-inaddon-tests.yml`` no-cache-save comment: added one paragraph making the byte-identical-qcow2 invariant explicit (both lanes consume the same publish-time bake; any PR-level overrides happen in-VM on the per-worker overlay, never on the cached base). * ``tag_suffix`` dispatch input description: previously said *what* the override is for, now also says *how* — the consumer edit (``tag=`` lines in the two e2e workflow files) is a separate manual step, and the allowed character set is named. * Consolidated the duplicate ``TAG_SUFFIX`` env-var explanation in the ``Push image to GHCR`` step down to a pointer back at the input description. No changes to the productive perf wins from this PR series (commits 8655bd8 + ba97395): cache-save still removed from inaddon lane, qcow2 still compressed before oras push, tag_suffix input still wired through. * revert: don't touch build_image.py in this PR (would force local build) Reverts the build_image.py stale-comment fix from 897dfd1. That change invalidated the e2e workflows' cache key (``git ls-tree`` over ``tests/haos_image_build/``) AND tripped the ``Detect PR-modified bake inputs`` gate that skips GHCR pull and forces a local image build on cache miss. Local build then filled the runner's 14 GB SSD before the test suite could run — the exact failure mode homeassistant-ai#1407 was originally designed to mitigate. External lane run 26362625939 hit: System.IO.IOException: No space left on device The stale comment in build_image.py:1400 about ``qemu-img convert -c`` is still wrong, but addressing it in the same PR as a cache-key-invalidating side effect isn't worth it. Will follow up separately if needed; the workflow step's own comment is now the source of truth for the compression numbers. * fix(haos-e2e): free disk space before local-build path + re-apply build_image.py comment fix Re-applies the build_image.py stale-comment fix that was reverted in 65cba9c, paired with a workflow-level fix for the disk-fill the reverted attempt exposed. What hit: * The original review-fix commit (897dfd1) modified ``tests/haos_image_build/build_image.py`` for a docstring update. * That path is in the cache-key hash (``git ls-tree -r HEAD tests/haos_image_build``) AND in the ``Detect PR-modified bake inputs`` gate's regex, so the change correctly invalidated the cache key AND triggered ``bake_inputs_changed=true``. * With GHCR fallback intentionally suppressed on bake-input changes (would otherwise serve master's stale image), the workflow fell through to the local-build path. * Local build then tripped ``No space left on device`` mid-bake (external lane run 26362625939) — the qcow2 download + decompress + boot + addon-install + ``cp`` peaks against the ~14 GB usable SSD. Two fixes, one commit: 1. **Workflow-level disk prune** before the local-build path on both e2e lanes (``haos-e2e-tests.yml`` and ``haos-e2e-inaddon-tests.yml``). Removes the unused ``/usr/share/dotnet``, ``/usr/share/swift``, ``/opt/ghc``, ``/usr/local/lib/android``, ``/usr/local/.ghcup`` trees (~20 GB combined on a standard ubuntu-22.04 runner) plus ``docker system prune`` plus ``apt-get clean``. Only runs on the conditions that lead into local build — ``cache-hit != 'true' && ghcr-pull.outcome != 'success'`` — so cache-hit and GHCR-pull paths are unaffected. Without this, ANY PR that legitimately changes bake inputs (the point of the cache-invalidation logic) would trip the same disk-fill that just took out PR homeassistant-ai#1428. 2. **Re-apply the stale-comment fix** in ``build_image.py`` with a shorter, less narrative version: drops the old incorrect numbers (``~7 GB → ~7 GB``, ``+9 min``) and replaces them with a pointer to the workflow step that does the actual compression (homeassistant-ai#1428, measured ``12 GB → 5.1 GB`` / ``2.3x``). The build_image.py change still invalidates the cache key, forcing this commit's CI runs through the local-build path again — which now has the disk prune to survive. * ci(haos-e2e): note that external lane is sole cache writer (sanity-check + nav aid) Adds a three-line comment on the external lane's ``Save image to cache`` step cross-referencing the inaddon lane's no-save block. Together with the matching comment in haos-e2e-inaddon-tests.yml (also homeassistant-ai#1428), a future maintainer reading either side now sees the full design: shared cache key (homeassistant-ai#1407), single writer (this PR). Also serves as the cache-hit-path sanity-check for homeassistant-ai#1428: this commit doesn't touch any bake-input path, so cache key ``b11cff145b55bc4b``-or-equivalent stays valid and the e2e workflows will restore from cache rather than re-running the local build that the previous commit (2772942) exercised. CI should land at ~6-7 min per lane vs the local-build path's ~11-12 min, confirming the common cache-hit path is unaffected by this PR. --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
kingpanther13
added a commit
that referenced
this pull request
May 25, 2026
…d findings (homeassistant-ai#1164) Round-3 review pass landed 13 verified findings; the cosmetic "persisted-value visible in UI after F5" item was explicitly skipped per user direction (UI shows post-gate value when master off; user accepts this since beta tools are actually disabled at runtime — the preserve-across-master-cycle UX is at the data layer, not the visual). Source fixes: - **Save button copy is now source-blind** — the previous "your feature-flag toggles already saved on click" claim only held when ``saveFeatureFlag`` raised ``restartNotice``; tool-config pin saves, backup-config saves, and cross-tab ``restart-required`` broadcasts also raise it. New copy: "a restart is pending. Click Restart above to apply your prior changes." (#2) - **Stale F.37 test docstring** describing the deleted server-side cascade rewritten. (#3) - **Beta-gate INFO log noise** — cascade-clear removal meant the gate could fire its "forcing %s=False" line every Settings rebuild, spamming addon logs once a user had truthy sub-flags persisted. Dedup via ``_BETA_GATE_LOGGED`` set per process, cleared on ``_reset_global_settings``. (#9) - **Lazy-lock docstring** updated to reflect Python 3.13 semantics (``asyncio.Lock()`` no longer takes a loop arg; the lazy pattern still serves test fixtures and single-loop deployment, with the invariant documented). (#10) - **Addon-mode carve-out comment** clarified to distinguish dev (master in schema) from stable (master web-UI-only). (homeassistant-ai#14) - **probe-div null branch** in F.37 now writes ``data-error`` so a failing test points at "selector missed" vs "value flipped" unambiguously. (homeassistant-ai#17) Tests added: - ``test_translations_cover_every_schema_key`` — parity check that every ``schema:`` key has a non-empty translation ``name`` and ``description``. Parameterised across stable + dev addons. Pins the class of silent gap that this PR's ``advanced_debug_logging`` fix addressed. (#4) - ``test_save_features_acquires_override_file_lock`` + ``test_save_advanced_acquires_override_file_lock`` — counting-lock wrapper asserts ``async with _get_override_file_lock()`` runs exactly once in each file-mode write path. Pin against a regression that silently bypasses concurrent-save serialisation. (#5) - ``test_dual_save_buttons_mirror_disabled_and_status_on_post_failure`` — exercises the 500-response branch of the dual-save mirror so a regression that broke ``_setAdvSaveStatus``/``_setAdvSaveDisabled`` for error paths only would still fail. (#6) - ``test_save_features_master_on_restores_subflag_values_in_addon_mode`` — addon-mode round-trip mirror of the existing standalone restore test; asserts the Supervisor merge-and-post call carries only the master flip-on and never zeroes out sub-flag values. (#7) - ``test_save_button_nothing_to_save_when_no_dirty_and_no_restart`` + ``test_save_button_restart_pending_hint_when_dirty_empty_but_restart_showing`` — both branches of the empty-dirty Save click are exercised; the restart-pending branch asserts the copy is source-blind. (#8) Deferred per user direction: - #1 (file-vs-Settings visual after F5): user accepts the current behavior (UI shows post-gate value; runtime tools actually disabled when master off; data-layer preserve still works end-to-end). - #11/#12/homeassistant-ai#13 (code-simplifier helper extractions): skipped as complicated to implement without behavior risk. - homeassistant-ai#15 (pre-homeassistant-ai#1164 users with already-cleared sub-flags): release-note concern, not a code change. - homeassistant-ai#16 (lock fragility under future thread-pool dispatch): speculative future-risk; not actionable today.
kingpanther13
added a commit
that referenced
this pull request
May 28, 2026
…jects Folds together three small things that share the same custom- component file: * Manifest bump 0.5.0 → 0.5.1 (was 0.6.0 in the earlier push; this is a small patch-shape change, not new capability — the PACKAGES_ONLY_YAML_KEYS branch is a routing addition, no new surface). Pattern mirrors homeassistant-ai#1459's bump on the same file. * Patch76 follow-up #1 (PR comment 2026-05-28): spell each storage-mode tool out individually in the rejection guidance instead of the compact slash-form ``ha_config_set_automation/script/scene``. An agent reading the rejection at call time would otherwise parse that as one malformed tool name and fail to route. Locations: * ``__init__.py`` reject message * ``tools_yaml_config.py`` ``yaml_path`` parameter description * Patch76 follow-up #2: ``TestHandleEditYamlConfigPathTraversal`` pins the layering of ``os.path.normpath`` before the ``fnmatch`` package check so a crafted ``packages/../configuration.yaml`` cannot smuggle a PACKAGES_ONLY key (``automation``) into ``configuration.yaml``. Belt-and-suspenders against a future refactor reordering those two steps; defense is already correct by construction. Updated tests for the spell-out: * ``test_yaml_config.py`` E2E rejection asserts each tool name individually instead of the combined slash-string. * ``test_yaml_dashboards.py::test_rejects_packages_only_key_in_configuration_yaml`` does the same at the unit layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kingpanther13
added a commit
that referenced
this pull request
May 29, 2026
…ant-ai#1164) (homeassistant-ai#1431) * feat(policy): scaffold policy package for per-tool approval (#966) * feat(policy): add Predicate/Rule/Policy data models (#966) * feat(addon): expose enable_per_tool_approval option (#966) * feat(config): add enable_per_tool_approval setting (#966) * feat(policy): atomic load/save for tool_policy.json (#966) * feat(addon): wire enable_per_tool_approval through start.py + docs (#966) * feat(policy): args-hash + remember-cache for approval queue (#966) * feat(policy): predicate evaluator (eq/in/regex/exists/...) (#966) * feat(policy): pending entries with TTL, decisions, and event signalling (#966) * feat(policy): PolicyMiddleware happy-path branches (#966) * test(policy): cover block/deny/timeout/recall/remember branches (#966) * feat(policy): /api/policy/* Starlette handlers (#966) * fix(policy): wrap ValidationError, scope contains op, regex doc, test bind (#966) * feat(policy): Policies tab in web UI + sidecar route wiring (#966) * feat(policy): register PolicyMiddleware on the FastMCP server (#966) * fix(policy): return 400 on malformed approve/deny bodies (#966) * feat(toolsearch): unpin yaml-edit and code-mode tools, gated by approval middleware (#966) * chore: ruff format + lint cleanup for policy package (#966) - ruff format reflow on PR-touched files (case statements split to two lines, function signatures, line continuations). - UP042: Verdict now inherits from StrEnum instead of (str, Enum). - E402: hoist `import anyio` to the top of test_approval_queue.py. - I001: sort imports in test_evaluator.py and test_model.py. No behavior change. * fix(policy): mypy narrowing for evaluator comparisons (#966) `Predicate.value` is `Any | None` and `extract_path` returns `Any`, so `val == pv`, `val > pv`, etc. inherit `Any` and trip the project's `warn_return_any` mypy setting on functions declared `-> bool`. Wrap the comparison branches in `bool(...)` to make the narrowing explicit. Also guard the `regex` branch with `isinstance(pv, str)` so `re.search` receives a definite `str` instead of `Any | None`; a non-string regex value now returns False instead of raising TypeError at evaluation time, which is the only sensible behavior for a malformed pattern. No change to any test's expected outcome. * docs: credit @L1AD and PolicyLayer for #966 inspiration * docs(addon): fix wrong YAML example in enable_per_tool_approval section (#966) * fix(policy): CI green + critical bugs from reviewer cycle (#966) - expires_in_seconds: use time-remaining not total TTL - middleware: fail-closed on corrupt policy load (was crashing all gated calls) - handlers: 500-with-corrupt-flag on get_config when policy invalid - approval URL: wire secret_prefix via lazy getattr so HTTP-standalone emits a usable absolute path - approve/deny: return bool, 409 on already-decided - Predicate: field validators for op/value compatibility (Gemini) - persistence: explicit UTF-8 encoding (Gemini) - middleware: reuse tools/helpers safe_progress - handlers comment: fix wrong "next call" claim - test_middleware: unwrap ExceptionGroup for pytest.raises (CI fix) - test_stdio_settings_sidecar: include new policy_* handler keys (CI fix) * refactor(policy): drop default_action + tighten Rule.tool_name (#966) - Policy schema simplified: no default_action field. System is always "allow unless a rule matches; rule = require approval". The previous default_action='require_approval' option was a bricked-config trap since rules can't grant allow-overrides. - Rule.tool_name now rejects empty string; wildcard '*' documented in the docstring. - Evaluator simplified to match. - Tests updated/added. * refactor(policy): encapsulate decision state + clean naming (#966) - PendingApproval.decide() encapsulates the decision/event coupling; property guards read-only access to decision. - __post_init__ validates expires_at > created_at. - ApprovalQueue docstring spells out single-process scope and restart-loses-tokens semantics. - Rename args_preview -> args throughout (it was always the full unmodified args; "preview" was misleading). - Remove on_policy_change dead parameter from build_policy_handlers. * fix(toolsearch): default-pinned tools should be user-unpinnable (#966) - Computed pinned set now respects tool_config.json — explicit "enabled" state removes from defaults. - Remove ha_restart / ha_reload_core from DEFAULT_PINNED_TOOLS (recovery actions, low frequency, low value in default LLM tool surface). - Add ha_manage_backup to MANDATORY_TOOLS (operational essential). - Server now declares _settings_secret_prefix on __init__ for pyright. * refactor(policy): rename feature to "Tool Security Policies" (#966) User-facing rename: addon config option, env var, Settings attribute, UI tab label, addon DOCS sections, translations, server method. Internal Python naming (policy/ package, tool_policy.json, /api/policy/* routes, class names like PolicyMiddleware/ApprovalQueue) unchanged for less churn. * docs: small comment polish for policy review nits (#966) * feat(ui): per-tool security-gated toggle in Tools tab (#966) * test(policy): integration + timing-isolation coverage gaps (#966) - test_server_policy_wiring: assert _apply_tool_security_policies attaches middleware + approval_queue when enabled, neither when disabled. - test_settings_ui_handler_selection: parametrize the live-vs-stub branch in build_settings_handlers (sidecar / no server / no queue / live). - test_middleware wait-loop timing: assert event-wake exit, not polling. - test_middleware multi-rule precedence: first-match wins for remember_minutes. * feat(ui): rewrite Tool Security Policies tab — per-tool cards + predicate editor (#966) * feat(config): expose enable_tool_security_policies as a feature flag (#966) Wires the new addon-config toggle into FEATURE_FLAG_FIELDS so it appears in the Server Settings tab and rides PR #1420's _save_feature_flags + _supervisor_merge_and_post_options + _schedule_supervisor_self_restart flow when toggled in addon mode. * fix(policy): address all verified review findings + CI failures (#966) CI fixes: - ruff: drop dead noqa: SLF001 suppression - unit test: test_missing_path_never_matches_except_exists uses op-compatible values so Predicate field_validator doesn't reject Review findings: - FEATURE_META entry for enable_tool_security_policies (toggle now renders in Server Settings tab) - Approval URL points to /settings?tab=tool-security-policies (was POST-only /api/policy/approve which 405'd on browser open) - policyDecide surfaces network errors + 409 current_decision - Policy gains version field for optimistic concurrency; PUT 409s on version mismatch; client surfaces 'reload before saving' - _apply_tool_security_policies failure logs spell out security impact (TOOL SECURITY GATING IS NOT ACTIVE) and include data_dir/env-var context - Validator rejects value on op='exists'; gt/lt TypeError degrades to False - ToolVisibilityResult -> UserToolStateOverrides, fields are frozenset, disjointness asserted - PendingApproval.event private; expose async wait() - _SupervisorOptionsError gains transport()/validation() classmethods encoding kind->status_code pairing - Wiring test binds queue identity; handler-selection covers all 3 live routes - Comment + doc polish (audit-trail claim, e2e docstring path, internal Task references) * fix(policy): CI green + real e2e test for the approval flow (#966) - ruff format: tests/src/unit/test_settings_ui.py - test_save_and_roundtrip: account for save_policy version bump - test_serialized_shape_is_stable: include 'version' in expected keys - test_addon_save_returns_500_when_server_is_none: guard server._settings_secret_prefix assignment with None check (regression from #4's secret-prefix wiring) - tests/src/e2e/policy/test_approval_flow.py: real e2e exercising block -> approve -> re-call cycle with strict args-binding rejection on mutated args. Skip-stub replaced with real test driving the live middleware via mcp_client + /api/policy/* HTTP. * fix(ui): broken quote escaping in predicate-form placeholder breaks JS parse (#966) The Python source `'placeholder=\\'\"lock\"...\\\\'>'` rendered as JS `'placeholder='\"lock\"..'>'` — the single quote inside the HTML attribute value closed the outer JS string literal, and subsequent tokens (\"lock\", 'or', '[', ...) broke parsing. With a syntax error in the inline <script>, the browser stopped executing — Tools tab stuck on 'Loading...', tabs unclickable. Switched to a JS-safe double-quoted attribute with " for the embedded double quotes in the placeholder hint. * fix(ui): gated toggle reads addon-config flag, not Policy.enabled (#966) The per-tool 'security gated' toggle was grayed out even when the user had enable_tool_security_policies turned ON in the addon config + the Server Settings tab toggle, because the JS was reading Policy.enabled (the file field) instead of the addon-config feature flag — which is the single source of truth for whether the middleware is active. loadPolicyState now reads enable_tool_security_policies from /api/settings/features (same place renderFeatureFlags consumes from). * fix(policy): Policy.extra=ignore so old persisted files load cleanly (#966) Persisted tool_policy.json files from an earlier revision of this PR carry default_action (since dropped) and rejected with ValidationError on load — surfacing as 'Could not load policy: 500' when the user clicked the per-tool gated toggle. Predicate/Rule keep extra=forbid (typo catching at construction). * fix(policy): drop Policy.enabled — addon-config flag is the sole switch (#966) The middleware's server-side gate was checking `policy.enabled` (a file field with no UI surface), so it returned ALLOW on every call regardless of rules. The addon-config flag (`enable_tool_security_policies`) was supposed to be the only switch — and the middleware is only registered when that flag is true — so the inner `policy.enabled` check was both redundant and broken. Remove the field, remove both server-side checks (middleware + evaluator), update tests, and refresh the JS comment that referred to it. * fix(policy): drop approve_url, instruct LLM to send user to settings page (#966) The relative-path approve_url doesn't resolve cleanly through cloudflared or other reverse-proxy deployment modes — the LLM can't safely hand it to the user. The user already knows where the Tool Security Policies tab is (they set the rule from it), and that page lists all pending approvals, so a per-request URL is unnecessary noise. - Drop approve_url from USER_APPROVAL_REQUIRED context; keep `token` so a caller could correlate but the user doesn't need to act on it. - Update message + progress text to instruct the LLM to tell the user to open the settings UI Tool Security Policies tab. - Drop the now-unused approval_url_builder param + the _settings_secret_prefix plumbing in server.py / settings_ui.py. Also fix the failing test_defaults (asserted dropped Policy.enabled field) and the e2e test PUT body that still carried `"enabled": True`. * feat(policy): schema-driven condition builder for write/destructive tools (#966) The previous "Add predicate" UX required users to type both the dotted arg path (e.g. `args.domain`) and the value as JSON. Two problems: 1. They need to know what fields each tool takes. 2. They need to know what values are legal (which HA domains exist, which entities, etc.). Replace the free-text path input with a dropdown sourced from the tool's JSON schema, and replace the free-text value input with a (multi-)select sourced from HA when the path has a known value source (domain, service, entity_id today; trivially extensible). Free-text is still available via an "(other — type a path)" escape hatch and as the automatic fallback for ops that don't pair with a registry (regex / contains / gt / lt). Server: - New `/api/policy/tool-schema?name=...` returns `{paths: [...], value_sources: {path: source_key}}`. Read-only tools return empty paths so the UI falls back to free-text (gating those is low-value but still permitted manually). - New `/api/policy/value-source?source=...` resolves a source key to a live list of choices. In-process 30s TTL cache avoids hammering HA when the user explores paths. - value_sources.py registry maps (tool_name, arg_path) → source_key for the common write/destructive surface (call_service, set_entity, set_integration_enabled, get_history, etc.). New mappings are one dict entry plus, if a new source key, one fetcher. - Both endpoints mount in addon + secret-prefix routes. Sidecar serves 503 stubs (no FastMCP registry / HA client in that process). UI: rename user-facing "predicate" → "condition" (CS jargon → SQL/JIRA terminology users actually recognise; internal Pydantic class stays `Predicate` so the wire format is unchanged). Form fetches the schema lazily on first open, caches it on the card, refetches value choices when path/op changes. Includes test_schema_handlers.py covering: missing-name 400, sidecar 503, unknown-tool 404, read-only empty-paths, write-tool paths + registry, JSON-schema enum passthrough, value-source 400 paths, both HA-services payload shapes, domain filtering for entities/services, and upstream-fetch 502 mapping. * test: include new policy handler keys in sidecar all-keys assertion (#966) * fix(ui): clearer condition-builder labels, optional value, bareword input (#966) User feedback on the new form was: 1. "args.foo" path placeholder is gibberish; no real label on path/value 2. value box should not be mandatory for ops where backend allows None 3. typing `lock` into the value box errored with "Invalid JSON" — every normal-looking input has to be quoted 4. for ha_call_service `data` was the only arg without an obvious meaning Changes: - Real `<label>`s on the form rows: "Argument:", "Match when:", "Value:". - Op dropdown shows friendly text ("is present (any value)", "equals", "is one of", "matches regex", etc.); wire values unchanged. - Hint line under the value row reflects the current op so users know whether a value is required and roughly what shape it should take. - Value is now OPTIONAL for ops where the backend accepts a missing field (exists, eq, neq, contains). Submitting an empty value omits the `value` key from the predicate entirely. - Bareword inputs auto-coerce: `lock` → `"lock"`, `lock,alarm` → list, `42` → number, `true` → bool. Falls back to a clearer error if even the smart-coercion can't make JSON. - Path dropdown options now carry the schema `description` as a `title` tooltip, so `data` reads as "Service data dict" on hover instead of being a mystery. - Schema-declared enums render as a value dropdown automatically (no registry entry needed) when the path's JSON-schema has `enum`. Also fix /tmp/extract_js.py — naive paren-counter broke once form strings started containing parens; switch to ast.parse so future edits don't silently break the harness. * feat(policy): wildcard path "args.*" + clearer empty-value semantics (#966) User asked for a catch-all "any argument equals X" condition and called out that the previous "Leave blank to gate on null" hint was nonsensical — a blank value should mean "any value" to a normal user, not "match the null literal". Backend: - Refactor evaluator: `extract_path` → `iter_path_values`, which yields every value the dotted path resolves to. A `*` segment fans out across the current node (dict values for dicts, items for lists). A path like `args.*` thus yields every top-level arg; `args.config.*` yields every leaf of the config sub-dict. - `match_predicate` rewrites to "ANY matching value satisfies the op", which collapses to the previous single-value semantics for non-wildcard paths. So `path=args.*, op=eq, value="lock"` gates whenever any arg of the tool call equals "lock". UI: - New "(any argument)" option at the top of the path dropdown, fills `args.*` and carries a tooltip explaining the semantic. - VALUE_OPTIONAL_OPS shrinks to just `exists` — blank value is no longer silently accepted for eq/neq/contains. Instead, the value-required error fires, and the hint text under the field tells the user to switch op to `is present` if they wanted "any value". - Hint copy revised across all ops so the "what happens with this op + blank value" question has a clear answer at each step. Tests: - New TestIterPathValues covering top-level, nested, missing, and the three wildcard shapes (dict values, list items, empty). - New TestWildcardPredicate covering eq/in/exists/regex matching via `args.*` plus an end-to-end evaluate() test. - Existing tests still pass with the refactored matcher; signatures of the public functions are unchanged. * fix(ui): default condition path to '(any argument)'; relabel error (#966) - Drop the '(pick an argument)' placeholder; default the path dropdown to '(any argument)' so the form is immediately submittable. - 'path is required' error reads 'argument is required' if it ever fires (it won't on the happy path now). * feat(policy): auto-save conditions + surface matched_rule in approval error (#966) UI: - Drop the manual "Save changes" button on each rule card. Conditions now PUT to disk the moment the user clicks "Save condition", clicks the × on a condition row, or edits the remember-minutes field (debounced 500ms). The only feedback is a small "Saving…" / "Saved." status line next to the card. - Removed the now-dead .policy-save-rule CSS and the markDirty helper. Server: - USER_APPROVAL_REQUIRED error context now carries `matched_rule` with the rule's tool_name + when[]. Lets the user (and the LLM) tell at a glance which rule fired, instead of guessing whether their condition saved correctly. * feat(policy): case-insensitive string comparison in all ops (#966) Security gates shouldn't fire differently based on whether the LLM capitalised its argument — 'Lock' and 'LOCK' and 'lock' are the same operationally. eq/neq/in/not_in/contains lower-case both sides before comparing when both are strings; regex uses re.IGNORECASE. Non-string types pass through unchanged so int(1) != str('1') still holds. * fix(policy): mypy bool cast + broaden e2e coverage (#966) mypy: bool(_ci(val) == _ci(pv)) — _ci returns Any (passes non-strings through unchanged), so eq/neq comparisons need an explicit bool wrap. Tests: previous e2e only covered the happy block→approve→re-call path. Add four more cases against the live testcontainer: - wildcard `args.*` gates when any arg matches the value - wildcard `args.*` passes through when no arg matches - case-insensitive matching (rule 'lock' gates caller 'LOCK') - deny → middleware raises USER_DENIED, tool never runs - remember_minutes>0: second call within the window skips the queue * refactor(policy): address review-cycle findings (#966) Gemini (6 unresolved threads): - Migrate POLICY_LOAD_FAILED / USER_DENIED / USER_APPROVAL_REQUIRED off manual `ToolError(json.dumps(...))` onto the canonical `raise_tool_error(create_error_response(...))` pattern. Added the three error codes to ErrorCode enum. - Hoist sync `load_policy()` off the event loop via `anyio.to_thread.run_sync` in the middleware's policy provider call. - Add justification comment on `local_provider._list_tools()` (same rationale that's already documented in `settings_ui.py`'s tool enumerator: public `list_tools()` filters disabled tools but operators may still want to author gating rules for them). Code-reviewer findings: - ApprovalQueue TOCTOU: two concurrent `on_call_tool` coroutines with identical (tool, args_hash) could both miss `find()` and create duplicate pending entries; approving one would leave the other waiter blocked. Introduce `find_or_create(...)` serialised behind an `anyio.Lock`; middleware now uses it. - ApprovalQueue had no pending-entries cap → memory exhaustion under an LLM retry-loop with mutated args. Add `PENDING_CAP = 1000` with FIFO eviction of oldest entries when the cap is hit. Silent-failure-hunter findings: - handlers.py: `get_tool_schema` and `get_value_source` now `logger.exception` before returning 500/502 so FastMCP version bumps or HA outages leave a traceable signal instead of opaque client errors. - value_sources fetchers `logger.warning` on unexpected HA response shapes (would otherwise silently return empty dropdowns). - value_sources cache no longer stores empty results — a transient HA glitch returning [] would otherwise pin the dropdown blank for 30s. PR-test-analyzer findings (the critical one): - test_persistence.py's `test_save_and_roundtrip` passed `Policy(enabled=True, ...)` for a field that no longer exists; `extra="ignore"` silently dropped it so the test was a no-op assertion. Replace with real round-tripped fields (wait_seconds / approval_ttl_minutes / remember_minutes) and add an explicit `test_load_drops_unknown_fields` exercising the extra="ignore" back-compat contract with a JSON file carrying `default_action` + `enabled`. - Add wildcard scalar/None tests (`args.x.*` against scalar yields nothing; doesn't crash). - Add ApprovalQueue tests: concurrent `find_or_create` shares one pending entry; `create` evicts oldest at PENDING_CAP. - Add handler tests: sidecar value-source returns 503, tool-schema 500 on `_list_tools` exception, value-source cache key separates per params, `_extract_arg_paths` skips malformed property entries. Comment-analyzer findings: - Grammar fix in handlers.py `_is_write_or_destructive` docstring. - model.py docstring "older version of this PR" → "older builds". - Strip the `(#966)` / `(issue #966)` parentheticals from module docstrings, settings_ui CSS/HTML/comments — git blame and the commit message carry the link. * fix(policy): UI surface fetch failures + middleware reissues swept pending (#966) - Middleware: after _wait_for_decision returns without a verdict, check whether the pending entry was swept (TTL elapsed during the wait). If so, create a fresh entry before raising USER_APPROVAL_REQUIRED so the LLM isn't told to re-call against a dead token. - UI: policyLoadConfig now surfaces fetch failures in a visible error banner instead of silently rendering blank — picks up the policy_file_corrupt:true repair hint from the server's 500 response. - UI: loadValueChoices records the failure (lastValueSourceError) so renderHint can show it under the value row. The dropdown still downgrades to free-text, but the user can now tell a transient HA outage from "no value source registered for this path". - UI: renderValueControl uses an autoincrement seq so rapid path/op edits don't let an earlier slow fetch's DOM mutation land after a newer one's (similar to the autoSave pattern). * fix(policy): logger.info on silent decide-False; debug log on gt/lt type-mismatch; strengthen event-wake test (#966) - ApprovalQueue.approve/deny: emit logger.info when the call returns False (unknown token or already decided) — was silent. Helps debug the case where the middleware's consume_and_maybe_remember races with an out-of-band decide. - Evaluator gt/lt TypeError fallback now logs at debug so a user whose 'battery_level < 20' rule never fires can see that the arg came in as a string and tighten the rule. - test_event_wakes_waiter now measures elapsed wait time and asserts < 200ms, ruling out a hidden poll-loop impl that would still pass the previous decision-only check. * style: ruff format evaluator.py for CI's 0.15.13 (#966) Local ruff 0.15.7 didn't wrap the multi-arg logger.debug call; CI's ruff 0.15.13 does. Upgrading local toolchain to match. * feat(policy): clear remember-cache on save, clearer disabled-state UX, mirror master toggle (#966) Three things: 1. Remember-cache invalidation on policy save (B2). ApprovalQueue.clear_remember_cache() drops every remembered approval; put_config calls it after a successful save. Without this, tightening a rule was silently bypassed by any in-flight remembered approvals until their window expired. 2. Better 503 / "unavailable" messaging (B8 + the broader issue). The stub handler's 503 message used to read "Live approvals unavailable in this mode (sidecar)" even when the real cause was the feature being turned off in addon config — the user had no way to tell from the UI. Updated to call out all three causes (feature off, sidecar, ImportError) and point at the addon log. The pending-list JS now checks policyState.enabled first and shows "Tool Security Policies is turned off" when that's the actual reason, falling back to the server's 503 message otherwise. 3. Mirror the master toggle onto the Tool Security Policies tab. Was only exposed in Server Settings before — users on the Policies tab had to navigate away to find the on/off switch. New checkbox at the top of the tab posts to the same /api/settings/features endpoint, so the two surfaces are live mirrors of the same addon-config flag. * test(policy): fix JS-harness drift guard + lock policy-tab behaviour (#966) The merged-in JSDOM behaviour test (#1425) failed collection because its hardcoded _TOP_LEVEL_ELEMENT_IDS list didn't yet know about the policy-tab handlers this PR adds (policy-master-toggle, policy-save-global-btn). Add them, plus matching DOM stubs in _build_min_dom so the init pass doesn't throw on the addEventListener calls. While the file is open, add three behavioural tests that pin the new condition-builder UX wiring: - Master toggle change POSTs to /api/settings/features with the enable_tool_security_policies flag (so the on-tab toggle stays a true mirror of the Server-Settings checkbox). - /api/policy/pending 503 renders "Tool Security Policies is turned off" when the addon flag is off (avoids the old misleading "sidecar / unavailable" copy). - /api/policy/pending 503 propagates the server's addon-log message verbatim when the flag IS on but the queue is unreachable (so users know where to look for ImportError details). The parse-coverage path catches syntax breaks already; these tests catch behavioural regressions on top of it. * refactor(policy): address 2nd-round review findings (#966) Verified all 23 findings from the 2nd pr-review-toolkit pass against the code; fixed 22 (skipping #13 — the JSDOM seq-cancel race test is high-effort to author reliably and the production guard is small enough that bench-level review catches regressions). ## Correctness / silent-failure - ApprovalQueue PENDING_CAP eviction now sorts by `(decision == "pending", created_at)` so resolved entries evict first. When a still-pending entry MUST be evicted (cap full, no resolved to drop), `.set()` its event so any waiter in `_wait_for_decision` wakes immediately instead of blocking the full wait_seconds against a row that no longer exists. - Middleware: log INFO with old + new token on the reissue-after-sweep branch so operators can correlate "approval row keeps reappearing" with the actual cause. - Middleware: scope `clear_remember_cache` to "rules actually changed" — editing only wait_seconds / approval_ttl_minutes no longer blows away in-flight remembered approvals. - Policy: model_validator requires `wait_seconds < approval_ttl_minutes * 60` so the middleware can't repeatedly issue fresh pending entries because the wait outlasted the TTL. - value_sources: cache key uses `urllib.parse.urlencode` so a future param value containing `=`/`&` can't collide with another key. - ApprovalQueue: `approve`/`deny` on unknown token now logs WARNING (security-gating endpoint, suggests UI bug or token probing). Already-decided stays INFO (legitimate race). ## UI - settings_ui.policyState gains an `enabledKnown` tri-state bit so downstream branches (`policyLoadPending`'s "feature off" copy, master-toggle revert) don't false-confidently route to the "disabled" message when the features fetch actually failed. - Master-toggle change handler reverts the checkbox on save failure AND syncs from `policyState.enabled` after a successful load — no more "UI says on, server says off" drift. - `policyLoadConfig` appends "(response body unparseable)" when the 500 body isn't JSON (e.g. HTML error page from a misrouted sidecar), so the operator sees more than just "HTTP 500". - `policyLoadPending` surfaces fetch errors inline ("Lost contact with server, retrying") instead of silently freezing the list. - `fetchToolSchema` records `lastValueSourceError` on failure so the hint banner explains why the value dropdown silently downgraded to free text. ## Tests - test_handlers: `test_put_config_clears_remember_cache_when_rules_change` + `test_put_config_preserves_remember_cache_when_only_timing_changes` lock in the scoped invalidation. - test_approval_queue: `test_create_evicts_resolved_entries_before_pending`, `test_evicting_pending_wakes_its_waiter`, `test_create_after_sweep_still_evicts_when_pending_fills_cap` cover the new eviction rules. The strengthened `test_find_or_create_lock_blocks_concurrent_create_under_real_race` inserts a yield point inside the lock body so the lock actually matters to the assertion (the previous test would pass even without the lock under anyio's cooperative scheduler). - test_middleware: `test_swept_pending_during_wait_is_reissued_with_fresh_token` exercises the previously-untested reissue branch. - test_schema_handlers: `test_value_source_empty_result_not_cached` proves the empty-result no-cache guard actually triggers a refetch. - test_settings_ui_js_behavior: master-toggle test now JSON-parses the POST body and structurally asserts `flags.enable_tool_security_policies is True` instead of loose substring matching. ## Comments - approval_queue.py PENDING_CAP docstring now matches implementation (was promising "resolved first" before the implementation actually did it). - evaluator.py: gt-branch comment example uses ">" not "<". - handlers.py: dropped cross-reference to settings_ui that would rot. - middleware.py: trimmed "fast on warm disk" speculation. - value_sources.py: trimmed "(WebSocket reconnect, auth lapse)" speculation in the empty-cache comment. - settings_ui.py: four comments still saying "predicates" updated to "conditions" to match user-facing terminology. * fix(ui): blank value on eq/in/etc coerces to op=exists (#966) User expects 'leave value blank to gate on the argument's mere presence regardless of value' to work across the equality-ish ops, not just op=exists. Earlier I had the form reject blank value for anything other than exists with a 'value is required' error. Now: for eq / neq / in / not_in / contains / exists, leaving the value blank silently coerces the predicate to op=exists on save. The condition row then reads as 'args.* exists' which is the right description of what's stored. Ops that genuinely need a value (regex / gt / lt) still raise 'value is required for op=...'. The hint text under each op updated to call out 'Leave blank to gate on any value' where it applies. * docs(addon): drop beta tag from Tool Security Policies (#966) The feature is stable enough to ship as a default-supported addon config option, not a beta. Also corrects two pieces of doc drift that landed here originally: - 'approval URL' wording → 'tell the user to open the Tool Security Policies tab' (the URL field was dropped earlier in this PR) - 'predicates' → 'conditions' (matches the user-facing terminology the UI now uses) Touches both prod and dev addon directories (config.yaml-driven UI text + the rendered DOCS.md). * docs(beta): describe 3-path enabling (dev addon, stable + web UI, env vars) (#1164) * feat(config): advanced settings registry + beta master toggle field (#1164) - Add ``ADVANCED_SETTINGS_FIELDS`` registry (21 fields across connection, search, operations, diagnostics, tools_surface, beta_codemode sections) - Add ``_ADVANCED_SETTINGS_BOUNDS`` and ``_ADVANCED_SETTINGS_CHOICES`` dicts for UI/POST validation - Add ``BETA_FEATURE_FIELDS`` tuple for master-gate enforcement - Add ``enable_beta_features`` Settings field (alias ENABLE_BETA_FEATURES, default False) as the master beta toggle - Update ``FEATURE_FLAG_FIELDS``: add ``enable_beta_features`` at front, ``enable_code_mode`` at end; reorder for UI grouping - Extend ``BACKUP_OVERRIDE_FIELDS`` from 3 to 5 entries (add ``auto_backup_dir`` and ``auto_backup_calendar_lookahead_days``) - Extend ``_apply_backup_overrides`` to handle ``str`` type; widen ``coerced`` annotation to ``bool | int | str``; add bounds check for ``auto_backup_calendar_lookahead_days`` (1..365) - Add coverage gate test asserting every Settings env alias is registered in one of the three panel registries (or in the explicit ALLOWLIST) - Add ``test_enable_beta_features_default_false`` * refactor(config): code-review fixups — docstring clarity, stricter bool reject, null-byte guard (#1164) * feat(settings-ui): per-tool env-pin for DISABLED_TOOLS / PINNED_TOOLS (#1164 addendum) Add env_pinned_tools() and effective_tool_config() helpers so tools listed in DISABLED_TOOLS / PINNED_TOOLS env vars stay read-only at runtime even after tool_config.json has been written. The _get_tools GET handler now includes env_pinned metadata per tool entry; _save_tools rejects incoming flips of env-pinned tools with HTTP 409. Server.py startup path updated to use effective_tool_config() so env pins apply at boot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(config): master beta gate + advanced overrides apply (#1164) - Rewrite _apply_feature_flag_overrides: lift addon-mode short-circuit for beta fields (enable_beta_features + BETA_FEATURE_FIELDS), add master gate that forces all 5 beta sub-flags to False when master is off regardless of env/file state - Update get_feature_flag_origin: beta fields never return "addon" — they follow standalone precedence in either mode - Add _apply_advanced_overrides: reads feature_flags.json for all editable ADVANCED_SETTINGS_FIELDS entries; skips display-only fields; validates types, bounds, and choices before setattr - Wire _apply_advanced_overrides into get_global_settings (runs after feature-flag + backup passes) - Add 18 unit tests covering master gate semantics, addon-mode carve-out, advanced-override int/str/float/display-only/out-of-bounds/invalid- choice cases, and backward-compat (pre-master override files) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(settings-ui): code-review fixups — drop dup env_pinned, settings param, msg + tests (#1164) Address code-quality review feedback on 32d67b4d: - Drop the redundant per-tool-entry `env_pinned` field from the GET /api/settings/tools response. The top-level `env_pinned` map is the single source of truth; UI does O(1) lookups against it. - `effective_tool_config()` now accepts an optional `settings` parameter (mirrors `load_tool_config()`); restores dependency injection at the server.py startup callsite (`self.settings`). - 409 rejection message uses comma-joined names instead of Python list repr for better human readability; structured `context.rejected` remains for programmatic access. - Add `test_get_tools_includes_env_pinned_map` test covering the new GET response field. - Symmetrize `get_data_dir.cache_clear()` calls at both ends of each tmp_path test so cross-test cache pollution can't leak in. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(config): code-review fixups — docstring accuracy, top-level import, null-byte test (#1164) - Correct _apply_advanced_overrides docstring: most advanced fields ARE in the addon config.yaml schema (backup_hint, verify_ssl, enabled_tool_modules, etc.) and are handled correctly via the env- var-wins check because start.py exports them. Only code_mode_* and mcp_server_version are file-only in either mode. - Move `from typing import Any` from function body to module-level imports (stdlib typing — no need to defer). - Add test_advanced_override_str_field_with_null_byte_rejected to cover the previously-unexercised null-byte reject branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(settings-ui): render env-pinned tool rows as read-only + update addon translations (#1164) - Add `toolEnvPinned` module-level map; populate from `data.env_pinned` in `loadTools()` - Tool rows for env-pinned tools get `.env-pinned` class, all inputs disabled, and a `feature-locked-note` banner naming the env var (DISABLED_TOOLS or PINNED_TOOLS) - Group master toggle excludes env-pinned tools from bulk enable/disable - Update `pinNotice` copy to describe the env-pinned lock-until-unset semantics - Update `homeassistant-addon-dev/translations/en.yaml` descriptions for `disabled_tools` and `pinned_tools` to reflect that they are operator-level locks, not seed-only values - Add JSDOM behavioural tests for env-pinned disabled and pinned tool rows * feat(settings-ui): /api/settings/advanced GET+POST handlers (#1164) * feat(settings-ui): render advanced settings sections in Server Settings tab (#1164) * feat(settings-ui): beta master toggle + nested sub-row gating + 409 rejection (#1164) * feat(settings-ui): nest code-mode sub-numerics under enable_code_mode (#1164) * test(addon): assert start.py auto-enables master beta in dev addon mode + stable schema absence (#1164) * feat(addon): start.py auto-enables ENABLE_BETA_FEATURES=true when dev addon options carry beta keys (#1164) * fix(settings-ui): post-CI/post-review fixes — backup-config str+range, addon-mode gate skip, e2e env, JSDOM ids, stale comments (#1164) * fix(config): beta-sub-flag origin returns addon in dev mode (env var presence as signal) (#1164) * fix(tests): addon-mode save tests use non-beta flag matching new origin semantics (#1164) * refactor(settings-ui): loadAdvancedSettings error parity + atomic-write helper reuse + beta_sub_flags via API (#1164) * refactor(config): narrower setattr errors + hasattr precheck + Gemini coerced-decl nit (#1164) * refactor(settings-ui): hoist override-file read + display-only warning + missing test coverage (#1164) * test(settings-ui): section-uniqueness + registry-disjoint + tighter env-pin assertions + accurate docstring (#1164) * refactor(addon): extract maybe_auto_enable_beta_master helper + real unit tests (#1164) * test(settings-ui): JSDOM coverage for advanced sections + master live-render; verify file untouched on 400 (#1164) * refactor(config): NamedTuple registries + import-time validator; finish silent-failure + JSDOM nesting tests (#1164) * fix(tests): restore standalone-mode assertion + add adv section containers (#1164) `replace_all` from an earlier sweep had pivoted the standalone-mode save assertion onto a beta sub-flag, so the master-gate guard now rejected the request and the test failed. Restore the non-beta `enable_tool_search` flag here — the assertion is about the unified save-contract shape, not the beta path. JSDOM `TestAdvancedSectionRender` tests were failing because MIN_DOM lacked the five `adv*` section containers; `renderAdvancedSection` would silently no-op (getElementById returned null) and the assertions fired against an empty body. Add `advConnection`, `advSearch`, `advOperations`, `advToolsSurface`, `advDiagnostics` to `_TOP_LEVEL_ELEMENT_IDS` so `_build_min_dom` emits `<div>` containers for them. * fix(tests): adopt master beta gate + new advanced handler keys (#1164) Three failures surfaced after rebasing onto upstream/master: 1. ``test_returns_all_handler_keys`` expected the pre-#1164 handler set. Add ``get_advanced_settings`` / ``save_advanced_settings`` to the expected keys. 2. ``test_tools_filesystem.TestFeatureFlag::test_enabled_with_*`` broke because the master beta gate now forces every beta sub-flag False at runtime when ``ENABLE_BETA_FEATURES`` is unset. Set both env vars together in the enabling tests so they exercise the sub-flag bool parsing in isolation, and add an explicit test for the gated behavior so a future regression in ``_apply_feature_flag_overrides`` surfaces here too. 3. ``test_yaml_config_tool.enable_flag`` fixture sets ``ENABLE_YAML_CONFIG_EDITING`` but didn't set the master, so the cached settings landed with the sub-flag forced False — would have broken next-up after the filesystem tests. Set ``ENABLE_BETA_FEATURES`` alongside. * fix(policy): contains operator case-insensitive on list-membership branch Pre-fix: case "contains": if isinstance(val, str) and isinstance(pv, str): return pv.lower() in val.lower() # CI return isinstance(val, (list, tuple, set)) and pv in val # case-SENSITIVE The string-in-string branch was already case-insensitive (matching the ``_ci``-equivalent treatment that ``eq`` / ``in`` / ``not_in`` apply), but the list-membership branch fell through to Python's default ``in`` operator. A rule listing ``["light.kitchen"]`` would not fire on an LLM passing ``["Light.Kitchen"]`` — silent gate failure. Bring it in line with the other string-op branches via per-element ``_ci``, which passes non-string entries through unchanged so mixed-type collections still get natural equality semantics. Caught by Gemini Code Assist on #1431; addressing inline rather than opening a follow-up because the policy module is now in master (#1421) and any reviewer running the suite would see the case-sensitivity asymmetry in the existing TestCaseInsensitive class. * feat(settings-ui): addon-aware locked banner, beta-at-bottom, danger warning, dual save buttons, fork-dev stable copy (#1164) Five user-feedback fixes against the Server Settings UI: 1. Locked-banner copy adapts in addon mode. The standalone "Set via env var X — unset it to edit here." copy is misleading in HA addon mode where the operator has no env-var surface (start.py writes the env vars from /data/options.json; Supervisor writes the rest). Endpoints now return is_addon; the JS helper envLockedNoteHtml swaps in addon-aware copy that points users at the addon Configuration tab. Master beta gets an extra hint explaining the auto-enable rule. 2. Beta block rendered into a dedicated bottom-of-panel betaBody container instead of featuresBody. The dangerous block sits last so users see safer settings first; a "Beta features (dangerous)" header in warning color marks the boundary. 3. enable_beta_features help-text rewritten to lead with an explicit danger warning (permanent damage to HA, no warranty, take a backup, own risk). Mirrored as a blockquote at the top of the dev addon's beta-options section in DOCS.md so the addon UI also surfaces the risk. 4. Save button redesigned — primary-CTA styling (bigger, accent background, hover state), duplicated at the top of the panel so a user scrolling either end can hit save, and paired with a prominent two-step note explaining that Save → Restart are both required for changes to take effect. 5. New scripts/fork-dev/copy-stable.sh + restore-dev.sh let maintainers whose only HA test path is the fork-dev addon flip homeassistant-addon-dev/ between dev-flavor and a stable-mirroring "stable test" flavor. Round-trip clean — copy → restore restores the index exactly. JSDOM behavioural coverage added for each of (1)-(4); MIN_DOM updated with the new top-row + beta-section element ids. * fix(tests): JSDOM beta-block tests assert against production HTML / fixed regex (#1164) Three CI failures from the previous commit: - ``test_beta_section_header_present_with_danger_styling`` and ``test_two_step_save_note_present`` asserted on static ``panel-server`` markup that lives in the rendered HTML template, not in any JS-populated container. MIN_DOM doesn't replicate the full panel-server children (by design — it's a minimal handler stub), so the assertions never found their target strings. Switch both tests to assert directly against ``_SETTINGS_HTML``; the presence of the markup at the template level is the property we actually want to lock down. - ``test_beta_rows_render_into_betaBody`` used a regex ``<div id="betaBody">(.*?)</div>\s*<div`` whose ``</div>\s*<div`` boundary matched the very first nested ``</div><div`` *inside* the master row (after the ``.feature-name`` close tag), so ``bb_content`` only captured the header of the master row. Anchor the boundary on ``</div>\s*</body>`` instead — non-greedy capture between the ``betaBody`` open tag and the body close still gives the full container content. Added a fallback path that asserts on class markers + non-leakage to featuresBody if the regex still misses. * feat(settings): fix stuck-master bug, master in dev schema, cascade-clear, drop connection panel, sync backup_hint+verify_ssl (#1164) Six fixes against the Server Settings + addon Configuration surfaces: 1. ``maybe_auto_enable_beta_master`` now requires ``config.get(key) is True`` instead of ``key in config``. The bare presence check fired the moment HA Supervisor merged the dev addon's schema defaults into options.json, locking the master "on" with origin=env on every fresh dev install even when all 5 sub-flags were False. New unit tests cover the truthy / all-false / one-of-many / non-bool-truthy permutations so a future regression on the semantic fails loudly. 2. Master ``enable_beta_features`` moved into the dev addon Configuration tab (schema + options + translations + DOCS.md). Defaults ON in dev — beta tools are the channel's purpose, so a fresh install lights them up without the user round-tripping to the web UI. Stable's schema is unchanged; the standalone web UI master path remains the gate there. start.py writes ENABLE_BETA_FEATURES from options.json only when the key is present; ``get_feature_flag_origin`` now treats the master like the sub-flags (env-var presence in addon mode → origin=addon). ``maybe_auto_enable_beta_master`` is kept as a one-cycle legacy bridge for installs whose options.json pre-dates the master key. 3. Beta sub-flags also default ON in the dev addon options block. 4. Master-off cascade clear: flipping ``enable_beta_features=False`` in ``_save_feature_flags`` now also writes False for every truthy beta sub-flag in the same save. Without this, sub-flags stayed True in the override file and resumed the moment the master was flipped back on — UX bug the user reported as "having to turn off every toggle individually." JS mirrors the cascade so sub-rows visually de-toggle on master-off without a page reload. 5. "Connection (display only)" section removed from the Server Settings panel. The read-only HOMEASSISTANT_URL / TOKEN / SUPERVISOR_TOKEN fields just wasted space — operators already see them in addon logs and configuration. Registry entries kept in ADVANCED_SETTINGS_FIELDS so the API still returns them (env-pin debugging, future surfaces). ``verify_ssl`` moved from ``connection`` to ``operations`` so it still renders in the panel. 6. ``backup_hint`` and ``verify_ssl`` now sync between addon Configuration and the web UI like feature flags do. New ``ADDON_SYNCED_ADVANCED_FIELDS`` set drives the origin helper (returns ``'addon'`` in addon mode for these) and the save handler (routes their writes through Supervisor ``/addons/self/options`` instead of the override file). Cross-surface gate visibility note appended to every beta sub-flag description in ``translations/en.yaml`` (and a top-of-options note on the master) so addon Configuration users know the web UI master gates everything. Locked-banner addon-mode copy from the earlier commit was already in place; tests in this commit re-target fixtures from the removed connection section to ``search``. * fix(addon-dev): beta sub-flags default OFF — only the master defaults ON (#1164) Mis-read of the user's intent in the previous commit. The intended shape for the dev addon's fresh-install defaults is: enable_beta_features: true ← gate unlocked enable_yaml_config_editing: false ← user opts in enable_filesystem_tools: false ← user opts in enable_custom_component_integration: false ← user opts in enable_code_mode: false ← user opts in enable_lite_docstrings: false ← user opts in The previous commit defaulted every sub-flag to true alongside the master, which would have shipped every beta tool live on a fresh dev install — including filesystem writes and the YAML config editor. Each sub-flag mutates the user's HA system, so they remain opt-in even on the dev channel; the master being on just means the gate is open. * revert(scope): drop scripts/fork-dev/ — maintainer tooling, wrong repo (#1164) Pushed these in 69b7a235 as part of task #17. They're personal-fork test tooling for the fork-dev addon workflow — they have no business on master. Removing from the PR; they're still in this branch's git history if anyone needs to fish them back out. * fix(addon+ui): #1431 review pass — restore MCP_HOST, gate sub-flag env writes, sane save (#1164) Round-2 review pass addressed 14 verified findings: **Bugs**: - **MCP_HOST regression** restored. The PR's earlier merge of upstream/master dropped the `bind_host = os.getenv("MCP_HOST", "0.0.0.0")` block introduced by #1434/#1436. `mcp.run(host=...)` now goes through `bind_host` again. - **Beta sub-flag env vars** are now written only when the matching key is present in `/data/options.json`. start.py was writing ENABLE_YAML_CONFIG_EDITING=false (etc.) unconditionally on stable addon, marking those fields origin='addon' in the web UI; the user's save then POSTed to Supervisor which rejected because the keys are not in stable's schema. - **Env-pinned tool save 409**: `_save_tools` now accepts re-sends whose state matches the env-pinned value, rejecting only true mismatches. The JS `saveConfig` POSTs the entire `toolStates` map including env-pinned rows; without this fix every save with `DISABLED_TOOLS` / `PINNED_TOOLS` non-empty would 409. - **Cascade-clear** now reads the persisted override file directly via `_read_feature_flag_override_file()` instead of `get_global_settings()` (whose master gate had already forced sub-flags to False, hiding stale-true overrides). Also force-False sub-flags that are explicitly True in the same payload as master=false, so `{master:false, sub:true}` no longer lands an inconsistent persisted state. - **Master beta-gate check** is now applied uniformly (no more "skip in addon mode" carve-out). The skip existed because the legacy auto-enable wrote ENABLE_BETA_FEATURES from sub-flag presence; now start.py writes the master from its own options key, so the gate is sound to apply in both modes. - **Dev-upgrade silent-disable warning**: start.py logs when master=false but a sub-flag is true in options.json, so an operator who toggled the master off in Configuration after a pre-#1164 dev install sees why their previously-enabled beta tools went away. - **Mixed-batch advanced save** is now split client-side. The server-side guard that returned 500 stays as a defense, but the UI no longer triggers it — `saveAdvancedSettings` partitions `_advancedDirty` into addon-routed and file-routed batches. **Logging / defensive code**: - Supervisor failure in `_save_advanced_settings` now logs before returning, matching the sibling `_save_feature_flags` / `_save_backup_config` handlers. - The three `assert sup_err is not None` sites that would crash under `python -O` now explicitly return INTERNAL_ERROR (covers the addon-route paths in feature flags and advanced settings). - `_apply_advanced_overrides` narrows `except Exception` to `(ValueError, TypeError)`, matching the parallel `_apply_feature_flag_overrides` exception shape. - `maybe_auto_enable_beta_master` now logs which sub-flag(s) triggered the legacy bridge when it fires, with a removal- candidate note in the docstring. **Docs / UX**: - Docstring drift fixed in `get_feature_flag_origin`, `_get_advanced_settings`, `_save_advanced_settings`. - `envLockedNoteHtml` master copy rewritten — origin='env' on the master is now only the legacy-bridge path, not the default dev-addon path. - "Bottom" save button now actually at the bottom of `panel-server` (below the beta block and its code-mode sub-numerics). Second two-step save note duplicated near the bottom row so users editing dangerous beta toggles also see it. Findings deferred to follow-up (legit but bigger than this pass): - A.2 concurrent-save read-modify-write race (needs an `asyncio.Lock` around the override-file path; functionality is safe today because the runtime gate hides the persisted-state inconsistency). - A.4 master-flip via addon Configuration tab → no cascade (the runtime gate + new log warning cover the observable surface). - F.* missing tests for new behaviours — adding in a follow-up commit. * test(settings): cover #1431 review pass fixes (#1164) - ``test_env_pinned_noop_resend_does_not_409`` — JS saveConfig POSTs the entire toolStates map; env-pinned no-op resend must be accepted. - ``test_env_pinned_value_mismatch_still_409s`` — pin true flips still rejected. - ``test_save_features_cascade_clears_subflag_even_when_payload_says_true`` — in-payload {master:false, sub:true} → 409 instead of inconsistent persisted state. - ``test_save_features_cascade_reads_override_file_not_post_gate_settings`` — cascade reads the file directly so it catches stale-true sub-flag overrides hidden by the master gate on the resolved Settings. - ``test_stable_addon_does_not_declare_enable_beta_features`` and ``test_dev_addon_declares_enable_beta_features_master_in_schema`` — lock the schema asymmetry that makes the dev/stable channel distinction work. - ``test_dev_addon_defaults_every_beta_subflag_to_false`` — sub-flags remain opt-in even on dev. * fix(settings): serialise override-file RMW + cover addon-synced advanced save (#1164) A.2 (concurrent-save race): both ``_save_feature_flags`` and ``_save_advanced_settings`` touch the same ``feature_flags.json`` override file. Two near-simultaneous requests could interleave their read/merge/write and clobber each other's persisted state. The runtime master gate hid the inconsistency for beta sub-flags, but other field combinations (advanced + feature-flag in the same window) would have lost state silently. Wrap the RMW window in an ``asyncio.Lock`` (lazy-initialised under the live event loop so module import doesn't bind to no loop). Both handlers acquire the same lock, so saves serialise correctly. F.35 + F.40 (test coverage): - ``test_save_advanced_addon_synced_routes_through_supervisor`` — ``backup_hint`` / ``verify_ssl`` saves in addon mode call ``_supervisor_merge_and_post_options``, return ``mode='addon'``, do NOT write the override file. - ``test_save_advanced_addon_synced_supervisor_4xx_surfaces_validation_failed`` — Supervisor schema rejection surfaces as ``CONFIG_VALIDATION_FAILED`` with the Supervisor status code preserved, not a generic 502. - ``test_origin_for_addon_synced_field_is_addon_in_addon_mode`` — pins the origin matrix: ``backup_hint`` / ``verify_ssl`` come back ``origin='addon, editable'`` in addon mode; non-synced env-pinned fields stay ``origin='env, locked'``. * fix(settings): A.8 preserve sub-flag visual + cover remaining deferred test gaps (#1164) A.8 — JS master-off no longer flips sub-flag checkboxes visually. Previously the cascade-clear set ``_lastFeatureFlags[sub].value = false`` on master-off so the re-render painted sub-rows unchecked. That fought the user's mental model — they expressed intent on individual sub-flags, master-off shouldn't visually wipe that context. Now sub-rows stay checked + dimmed + disabled after the master flips off; the server-side cascade still clears the values on disk, so a refresh shows the cleared state, and a failed save leaves the visible checked state matching the actual on-disk state. F.37 — ``test_master_off_click_dims_subrow_live_without_clobbering_value`` dispatches a real change event on the master input and asserts the sub-row goes dimmed + disabled but keeps its checked attribute. F.38 — three cross-mode origin permutations for the master: - dev-addon env-set → 'addon' - standalone env-set → 'env' - addon mode + file override (no env) → 'file' F.42 — ``test_dual_save_buttons_mirror_disabled_and_status_state`` probes both ``advSaveStatus`` / ``advSaveStatusTop`` text and both buttons' disabled state via a hidden probe div, asserts the mirror holds at save completion. * fix(tests): probe JSDOM properties via probe div; assert mid-save mirror (#1164) Two test bugs in 25b45ab7's new assertions: 1. ``test_master_off_click_dims_subrow_live_without_clobbering_value`` asserted on the literal string ``"checked"`` in the serialised DOM. ``input.checked`` is a DOM property (not an HTML attribute), so JSDOM's serialiser doesn't emit it. The .checked state IS true at the property level — just invisible to the regex. Probe via a hidden ``__sub_state_probe`` div that reads the .checked / .disabled properties and writes them to data-* attributes. 2. ``test_dual_save_buttons_mirror_disabled_and_status_state`` probed AFTER the full save+reload chain, by which point loadAdvancedSettings() had blanked both status text els. Restructure the test to probe SYNCHRONOUSLY after ``click()``, while saveAdvancedSettings is mid-flight (status="Saving…", both buttons disabled). That's the actual mirror invariant we wanted to lock — the helpers ``_setAdvSaveDisabled(true)`` and ``_setAdvSaveStatus('Saving…')`` run synchronously before the first await. * feat(settings): drop sub-flag cascade-clear; restore advanced_debug_logging translation; clearer Save-button copy (#1164) Three user-reported fixes from stable-addon testing: 1. Stable add-on's ``translations/en.yaml`` was missing the ``advanced_debug_logging`` description — the schema declares the toggle in ``config.yaml:49`` but no translation ever shipped, so the addon Configuration UI showed an unlabelled checkbox. Add the missing entry (same wording as dev's translation). 2. Big "Save advanced settings" button used to say "Nothing to save." after the user toggled a feature flag (beta master, Tool Search, etc.). Feature-flag toggles auto-save on click via ``saveFeatureFlag`` — they never enter ``_advancedDirty``, so the advanced-save button sees nothing to do. When the restart banner is already showing (recent feature-flag save), surface that explicitly: "No advanced changes to save — your feature-flag toggles already saved on click. Click Restart above to apply them." Falls back to the original "Nothing to save." copy when there's no pending restart. 3. Drop the master-off cascade-clear behaviour entirely. The runtime master gate in ``_apply_feature_flag_overrides`` already forces every beta sub-flag to False whenever the master is off, so the tools stay disabled at runtime regardless of file state. Leaving the sub-flag values in the override file means toggling the master off → on restores the user's prior sub-flag selections automatically; the previous cascade-clear forced users to re-check each sub-flag after every master cycle, which was the wrong UX trade for an opt-in beta surface. The master-gate check is unchanged — it still rejects payloads that try to enable a sub-flag while the effective master is off, so the "sub true while master false in same payload" inconsistency still can't land. The cascade-clear was a separate (now-removed) defence. Tests updated: - ``test_save_features_master_off_preserves_subflag_values`` — was ``test_save_features_cascade_clears_subflags_when_master_off``; asserts the new "preserve" semantics. - ``test_save_features_master_on_restores_runtime_subflag_values`` — new round-trip test for master off → on restoring sub-flags. - ``test_save_features_payload_master_false_sub_true_rejected_by_gate`` — renamed; asserts gate rejection still covers the inconsistent payload. - ``test_save_features_cascade_reads_override_file_not_post_gate_settings`` — deleted (no cascade, no reason to test cascade's read path). - ``test_save_features_master_off_applied_dict_contains_only_master`` — new no-cascade pin; ``applied`` carries only the master flip. * fix(settings): #1431 review pass — address 13 verified findings (#1164) Round-3 review pass landed 13 verified findings; the cosmetic "persisted-value visible in UI after F5" item was explicitly skipped per user direction (UI shows post-gate value when master off; user accepts this since beta tools are actually disabled at runtime — the preserve-across-master-cycle UX is at the data layer, not the visual). Source fixes: - **Save button copy is now source-blind** — the previous "your feature-flag toggles already saved on click" claim only held when ``saveFeatureFlag`` raised ``restartNotice``; tool-config pin saves, backup-config saves, and cross-tab ``restart-required`` broadcasts also raise it. New copy: "a restart is pending. Click Restart above to apply your prior changes." (#2) - **Stale F.37 test docstring** describing the deleted server-side cascade rewritten. (#3) - **Beta-gate INFO log noise** — cascade-clear removal meant the gate could fire its "forcing %s=False" line every Settings rebuild, spamming addon logs once a user had truthy sub-flags persisted. Dedup via ``_BETA_GATE_LOGGED`` set per process, cleared on ``_reset_global_settings``. (#9) - **Lazy-lock docstring** updated to reflect Python 3.13 semantics (``asyncio.Lock()`` no longer takes a loop arg; the lazy pattern still serves test fixtures and single-loop deployment, with the invariant documented). (#10) - **Addon-mode carve-out comment** clarified to distinguish dev (master in schema) from stable (master web-UI-only). (#14) - **probe-div null branch** in F.37 now writes ``data-error`` so a failing test points at "selector missed" vs "value flipped" unambiguously. (#17) Tests added: - ``test_translations_cover_every_schema_key`` — parity check that every ``schema:`` key has a non-empty translation ``name`` and ``description``. Parameterised across stable + dev addons. Pins the class of silent gap that this PR's ``advanced_debug_logging`` fix addressed. (#4) - ``test_save_features_acquires_override_file_lock`` + ``test_save_advanced_acquires_override_file_lock`` — counting-lock wrapper asserts ``async with _get_override_file_lock()`` runs exactly once in each file-mode write path. Pin against a regression that silently bypasses concurrent-save serialisation. (#5) - ``test_dual_save_buttons_mirror_disabled_and_status_on_post_failure`` — exercises the 500-response branch of the dual-save mirror so a regression that broke ``_setAdvSaveStatus``/``_setAdvSaveDisabled`` for error paths only would still fail. (#6) - ``test_save_features_master_on_restores_subflag_values_in_addon_mode`` — addon-mode round-trip mirror of the existing standalone restore test; asserts the Supervisor merge-and-post call carries only the master flip-on and never zeroes out sub-flag values. (#7) - ``test_save_button_nothing_to_save_when_no_dirty_and_no_restart`` + ``test_save_button_restart_pending_hint_when_dirty_empty_but_restart_showing`` — both branches of the empty-dirty Save click are exercised; the restart-pending branch asserts the copy is source-blind. (#8) Deferred per user direction: - #1 (file-vs-Settings visual after F5): user accepts the current behavior (UI shows post-gate value; runtime tools actually disabled when master off; data-layer preserve still works end-to-end). - #11/#12/#13 (code-simplifier helper extractions): skipped as complicated to implement without behavior risk. - #15 (pre-#1164 users with already-cleared sub-flags): release-note concern, not a code change. - #16 (lock fragility under future thread-pool dispatch): speculative future-risk; not actionable today. * feat(settings): version footer + Patch76 review feedback Adds the running ha-mcp version to the settings UI footer (issue #1466). ``info.version`` flows from ``/api/settings/info`` → ``HA_MCP_BUILD_VERSION`` env var on addon builds (set by both stable and dev Dockerfiles) → package metadata fallback. Empty on older deployments without the field. Addresses Patch76's review (no blockers, all bundleable): - ``OverrideField`` folds the structurally-identical ``FeatureFlagField`` and ``BackupOverrideField`` into one NamedTuple; aliases preserve readable construction sites. ``AdvancedField`` keeps its own type since it c…
kingpanther13
added a commit
that referenced
this pull request
May 29, 2026
…xcept, 6-tool e2e, abs-path test) Patch76's homeassistant-ai#1431-aware re-review (verdict: mechanism "solid... ready"). Three non-blocking items addressed: 1. Narrow the settings-lookup except (util_helpers.py). build_skill_content and attach_skill_content wrapped get_global_settings() in a bare `except Exception`, masking programming bugs (AttributeError/ImportError) the same as a genuine config issue. Narrowed both to `except ValidationError` (the realistic Settings() config-load failure named in the existing comment) so real bugs now surface, per the repo's narrow-except convention. Also flipped the attach_skill_content fallback from `master_on = True` to `False`: on a settings-fetch failure we can't know the master state, so suppress the vendor-missing warning rather than emit a misleading one whose true cause was the settings fetch. 2. e2e skill_content delivery now covers all six write tools: - test_skill_content_delivery.py: parametrized on/off coverage for script / scene / helper / dashboard (hint-is-first-key + canonical files on default; suppression on MandatoryBPS=False). Automation keeps its explicit tests incl. the BP-warning section-slice. - test_yaml_config.py: TestYamlConfigSkillContentDelivery (on/off) using the module's mcp_client_with_yaml_config fixture (ha_config_set_yaml is feature-flag + component gated, can't share the automation-dir fixtures). The six tools have distinct success-return shapes where an ordering or wrong-dict bug would slip past the structural AST test. 3. test_skill_loader.py: test_resolve_skill_files_rejects_absolute_path (`/etc/passwd` passed directly) — guards the traversal check against a future refactor to a naive prefix match. Test bookkeeping for #1: test_settings_load_raises_returns_empty now feeds a real pydantic.ValidationError (via _make_validation_error()) instead of a plain ValueError, and test_settings_load_propagates_unexpected_error pins that a non-config error (AttributeError) is NOT swallowed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kingpanther13
added a commit
that referenced
this pull request
May 30, 2026
…e checker warnings with embedded skills responses (homeassistant-ai#1182) (homeassistant-ai#1448) * feat(internal): add skill_loader utility with path-traversal guards Shared helper for resolving (skill, file) pairs from the bundled skills-vendor directory. Mirrors the symlink + path-traversal guards in ha_get_skill_guide's file read path, but silently skips bad files instead of raising so write tools can use it for response embedding without failing the operation. Refs #1182 * refactor(internal): server._get_skills_dir delegates to skill_loader Single source of truth for the skills-vendor path lookup. The new delegate matches the existing exists()-only check exactly — no behaviour change for ha_get_skill_guide or any other caller — and gives the write-tool include_skill parameter the same path resolver without holding a server reference. Also simplifies skill_loader._skills_dir_at to a bare existence check so the two paths stay byte-equivalent. Refs #1182 * refactor: best_practice_checker returns dataclass; warning text names 3 access routes check_automation_config / check_script_config now return BestPracticeCheckResult — a list[str] subclass that also exposes a .referenced_files set of skill file paths each warning points to. Existing call sites (and tests) that treat the return as a plain list keep working unchanged; new callers use .referenced_files to fetch file bodies via skill_loader and embed them in responses. Each warning's ' See ...' suffix now names all three skill-access routes when skills are enabled: See skill://... | call ha_get_skill_guide(skill=..., file=...) | or pass include_skill=True on this tool to receive the file in the next response automatically When skill_prefix=None (skills feature off server-wide), the suffix is suppressed entirely — matches historical behaviour because none of the three routes resolve when skills are off. Internal sweep: every warnings.append(... + _ref(...)) site replaced with _emit(warnings, ..., file_ref) so the referenced_files set stays in sync with the warning strings. Refs #1182 * feat: include_skill=True default on ha_config_set_automation Adds include_skill: bool = True parameter. When True (default), the response carries skill_content: {path: body} populated from the canonical mapping (automation-patterns.md + template-guidelines.md). Independent of include_skill, best-practice warnings auto-populate the same skill_content field with whichever reference files those warnings cited — so the LLM always gets the relevant guidance inline on the first wrong attempt, no follow-up call needed. bp_warnings storage typed as BestPracticeCheckResult so the referenced_files set is accessible to the response builder. Refs #1182 * refactor(internal): hoist build_skill_content to util_helpers Shared helper now lives in util_helpers.build_skill_content so each of the six write tools getting include_skill (automation, script, scene, helper, dashboard, yaml) imports the same implementation. Per-tool canonical mapping stays local to each module. Refs #1182 * feat: include_skill=True default on ha_config_set_script Mirrors the ha_config_set_automation pattern: response carries skill_content with automation-patterns.md + template-guidelines.md by default, auto-embeds referenced files on BP-checker warnings (works in both python_transform and full-replace modes). Refs #1182 * feat: include_skill=True default on ha_config_set_scene No scene-specific reference file exists. Returns the top-level home-assistant-best-practices SKILL.md doc by default, which links out to the relevant references for action/condition design (scenes share action syntax with automations/scripts). Refs #1182 * feat: include_skill=True default on ha_config_set_helper Adds include_skill: bool = True parameter. _attach_helper_skill() post-processes each of the 4 success return sites (simple-create, simple-update, config-store-update, and the flow-helper branch) to attach skill_content with references/helper-selection.md — the decision matrix for picking the right helper type (input_*, counter, timer, template, group, utility_meter, etc.). Refs #1182 * feat: include_skill=True default on ha_config_set_dashboard Returns dashboard-guide.md + dashboard-cards.md by default — layout patterns and card-type taxonomy. Single success-return site wraps via _attach_dashboard_skill helper. Refs #1182 * feat: include_skill=True default on ha_config_set_yaml Returns template-guidelines.md by default. YAML packages frequently include template sensors, command_line entities, and mqtt templates — exactly where template misuse causes the most subtle bugs. Refs #1182 * test: cover build_skill_content + per-tool canonical mappings Unit tests pin the shared assembly contract: canonical files attached when include_skill=True, suppressed when False; BP-warning referenced files always attach; canonical and referenced dedupe; missing canonical files silently skipped; missing skills-vendor degrades to no-op. Plus a pin per tool's canonical mapping so any future change to one of the six write tools' skill assignment is a deliberate edit caught by these assertions. Refs #1182 * feat: section-slice reactive auto-embed via #anchor (issue #1182 Q3) Best-practice warnings already point at specific markdown anchors (e.g. automation-patterns.md#native-conditions). Previously the auto-embed path shipped the whole 20 KB reference file. Now it ships just the matching section — typically 1-5 KB. Reactive content cost drops 5-15x with no information loss. Implementation: - skill_loader gets extract_section(body, anchor) — GH-style slugifier, fence-aware (a ``# yaml-comment`` inside ```yaml ... ``` no longer false-closes the surrounding section). - resolve_skill_files accepts "path#anchor" entries; reads each file at most once even when multiple sections are requested from it. - best_practice_checker._emit preserves the anchor in referenced_files instead of stripping to bare path. - util_helpers.build_skill_content dedupes bare-vs-anchored for the same file (full file in canonical supersedes a sliced section ref). Tests cover: anchor extraction, fence handling, ifthen-style slash slugs, missing anchor silently skipped, file-read dedup across multiple sections, bare-vs-anchored supersession at the right granularity. Refs #1182 * docs(internal): trim ha_config_set_automation docstring (-6.5 KB) Removed schema enumerations (REQUIRED FIELDS / OPTIONAL CONFIG FIELDS, TRIGGER/CONDITION/ACTION TYPES, blueprint vs regular type breakdown), extended worked examples (motion light, blueprint create/update), and the embedded PREFER NATIVE OVER TEMPLATES cheat-sheet. All of that now ships in the response under skill_content via automation-patterns.md + template-guidelines.md by default; the reactive checker additionally embeds the relevant section on warnings. Kept the action-verb summary, the when-NOT-to-use routing (scene/helper alternatives), the two-mode contract (config vs python_transform with config_hash), and the pointer to where the templating guidance lives now (skill_content / best_practice_warnings). 7,453 → 979 chars (-6,474). Catalog cost down by ~1.5K tokens for this single tool. The guidance is not lost — it arrives in the response on every write call (and just the relevant section on warnings) instead of riding along in the catalog forever. Refs #1182 * docs(internal): trim ha_config_set_script docstring (-3.9 KB) Mirrors the ha_config_set_automation trim. Removed schema field lists, 6 extended worked examples (delay, blink, parameters, blueprint create/update), and the embedded PREFER NATIVE OVER TEMPLATES cheat-sheet. All of that arrives in the response via skill_content (automation-patterns.md + template-guidelines.md by default; relevant section on BP warnings). Kept the action-verb summary, the when-NOT-to-use routing (use ha_config_set_automation for trigger-based work), the two-mode contract, the sequence-vs-use_blueprint requirement, and the pointer to where the templating guidance lives now. 4,762 → 869 chars (-3,893). Refs #1182 * docs(internal): trim ha_config_set_dashboard docstring (-3.0 KB) Removed python_transform worked examples, MODERN BEST PRACTICES list, DISCOVERING ENTITY IDs preamble, DOCUMENTATION cross-refs, and 4 verbose dashboard config examples (empty, sections, strategy, update). All of that ships via skill_content (dashboard-guide.md + dashboard-cards.md by default). Kept the two-mode contract, the index-shift caveat for chained python_transforms, the strategy-vs-custom note, the url_path naming rules, and pointers to entity-discovery tools. 4,058 → 1,047 chars (-3,011). Refs #1182 * docs(internal): trim ha_config_set_helper docstring (-2.0 KB) Removed the full SIMPLE/FLOW type enumerations (12 simple + 15 flow + config_subentry), the verbose Behavior notes preamble, and 4 worked examples (template sensor, group, tod, config_subentry). The helper-type decision matrix and worked examples now ship in skill_content via helper-selection.md by default. Kept the param-required-by-mode table, the action= disambiguation contract, the schema-discovery-on-first-error pattern, and the update-field-preservation note (all behavior contracts the LLM needs at call time, not reference docs). 3,282 → 1,225 chars (-2,057). Refs #1182 * docs(internal): trim ha_config_get_automation docstring (-0.3 KB) Examples and "use ha_get_skill_guide" pointer removed — single required parameter makes the call self-evident, and the matching set_automation call now ships skill_content automatically. Kept the return-shape contract (config_hash + automation_id resolution) the LLM needs at call time. 708 → 425 chars (-283). Refs #1182 * docs(internal): trim ha_config_get_script docstring (-0.4 KB) Examples and behavioral-parity-with-automations note removed; the prefix-strip and bare-key contract is now stated once, concisely. Refs #1182 * docs(internal): trim ha_config_get_scene + ha_config_set_scene docstrings Both shorted: examples removed, schema-shape reduced to one-liner, ha_get_skill_guide pointers dropped (skill_content ships in set_scene responses by default). get_scene: 455 → 220 (-235); set_scene: 1,322 → 850 (-472). Total -707. Refs #1182 * docs(internal): trim ha_config_list_helpers docstring (-0.8 KB) Removed the per-helper-type one-line descriptions (the same information is already enumerated by the Literal[...] on helper_type, visible to the LLM as the JSON-schema enum), the examples, and the skill_guide pointer. Kept the storage-vs-YAML scope note and the simple-vs-flow routing (use ha_search_entities for flow-based types). 1,318 → 538 chars (-780). Refs #1182 * docs(internal): trim ha_config_get_dashboard docstring (-0.9 KB) Examples (8 of them) and the search-workflow walk-through removed. Kept the three-mode contract (list/search/get) and the config_hash return-shape distinction (present in get/search, absent in list). 1,978 → 1,071 chars (-907). Refs #1182 * docs(internal): trim ha_call_service docstring (-0.8 KB) Removed 4 worked examples, the per-parameter Markdown bullet list (parameter descriptions are already on the Annotated[Field()] entries in the signature), and the skill_guide pointer. Kept the domain.service pattern, the omitted-entity_id targeting note, the return_response / wait contract, and the discovery-tool pointers. 1,593 → 711 chars (-882). Refs #1182 * docs(internal): trim ha_config_set_yaml docstring (-0.3 KB) Collapsed the per-tool routing bullet list into one sentence (the LLM already knows the alternatives from the catalog). Replaced the skill_guide pointer with a note that template-guidelines.md ships in this response by default. Kept the LAST RESORT warning, the YAML-only scope (allowed keys), the post_action reload-vs-restart note, and the comment/tag preservation guarantee. 1,240 → 1,036 chars (-204). Refs #1182 * fix: attach skill_content on ha_config_set_dashboard python_transform path The python_transform branch built and returned transform_result without calling _attach_dashboard_skill, so include_skill=True was a silent no-op on the recommended edit mode. Only the create/update branch wrapped. Refs #1182 * fix: attach skill_content on ha_config_set_helper config_subentry path The config_subentry branch returned set_config_subentry's response directly without calling _attach_helper_skill, so include_skill=True was a silent no-op on this fifth return site. The other four return sites (flow, simple-create, simple-update, config-update) already wrap. Refs #1182 * fix: attach skill_content on ha_config_set_scene python_transform path The python_transform branch built and returned response without ever calling build_skill_content, so include_skill=True was a silent no-op when editing an existing scene. Only the config-replacement branch wrapped. Refs #1182 * docs(internal): correct scene docstring + module comment (D1, D3) Three sites claimed scenes 'share action syntax with automations/scripts' or that SKILL.md links out to 'action/condition design' references. Factually wrong: scenes are pure state snapshots — only an entities dict, never triggers/conditions/actions. The validator at _validate_scene_config rejects list shape with exactly this distinction. Rewrote the module comment and include_skill Field description to state honestly what SKILL.md actually covers (entity-naming, safe-refactoring, helper-vs-template trade-offs) and why it's still relevant for scene authoring. Re-added the entities-dict shape with a concrete example to the public docstring — the prior trim dropped the only example, and no shipped skill file carries scene examples, so the LLM had no on-call-site reference for the first-write payload. Refs #1182 * docs(internal): drop stale helper count from ha_config_set_yaml (D2) Said '27 helper types' but Literal on set_helper.helper_type lists 28 (includes config_subentry, which the prior count missed). Switched to an enumeration of representative types instead of a count so future helper additions don't introduce drift again. Refs #1182 * docs(internal): align checker docstrings with skill_prefix=None impl (D4) Module docstring and check_automation_config arg doc both promised that when skill_prefix=None, the URI route would be omitted but the ha_get_skill_guide tool route and the include_skill parameter route would still be mentioned. The implementation (_three_route_suffix returns '' when skill_prefix falsy) suppresses the entire suffix — correctly, because skill_prefix=None signals skills are disabled server-wide, in which case none of the three routes can resolve. Updated both docstring sites to match the implementation rather than the other way around (the implementation is the right behaviour; the docstrings were aspirational). Refs #1182 * fix(internal): preserve referenced_files on copy/deepcopy (L1) BestPracticeCheckResult.__init__ resets referenced_files to an empty set, so the default list-subclass copy protocol (which re-enters __init__ with self as items) silently dropped the auto-embed payload. Added __copy__ and __deepcopy__ overrides that explicitly carry the set across the copy. No current caller uses copy/deepcopy on the result, so this is latent; fixing now avoids a debugging trap for any future consumer (e.g. a Transform layer that wants to forward the result). Refs #1182 * fix: slugify both sides in extract_section + cover edge cases (L2) The slugifier was applied to the heading from the file but NOT to the caller-provided anchor, so an asymmetric comparison silently missed on trailing whitespace, double-hash typos, mixed-case anchors, etc. All current _emit() sites pre-slugify, so the bug is latent — but the asymmetry was a footgun for any future change touching anchor strings. Now both sides are slugified before comparison. Tests added: - trailing/leading whitespace tolerance - mixed-case tolerance - ## / # typo absorption - last-heading section runs to EOF - first-match wins on heading-slug collision Refs #1182 * fix: surface skills-vendor-missing as a top-level warning (L3) Previously: include_skill=True on any of the 6 write tools silently omitted the skill_content field when the bundled skills-vendor submodule wasn't initialised. Asymmetric vs the read-side ha_get_skill_guide tool (server.py:200-208), which surfaces a structured degraded:True payload for the same condition. Operators on Docker / source installs who skipped --recurse-submodules got a silently degraded server. New shared helper util_helpers.attach_skill_content: attaches skill_content as before AND appends a top-level warnings[] entry when the caller requested skill content (include_skill=True OR referenced_files non-empty) AND the vendor is missing. Swept all six write tools to delegate through this helper: - automation/script/scene/yaml: inline `if skill_content: result[...]=` blocks replaced with attach_skill_content() calls. - helpers / dashboards: the per-tool _attach_*_skill wrappers now delegate to the shared helper (same single point for the degraded-warning behaviour). User-opted-out path (include_skill=False, no BP-warnings) stays silent — only requests-that-can't-be-fulfilled surface the warning. Refs #1182 * fix: escalate skill_loader log levels (O1) Every failure mode in _read_file_safely logged at DEBUG, which is below the default LOG_LEVEL=INFO — symlink rejects, path-traversal rejects, and missing files were all invisible in production. Combined with the write tools' silent-degrade contract, the operator had zero feedback on these conditions. Now WARNING for: - symlink rejection (security event) - path-traversal rejection (security event) - missing file / not-regular-file (caller bug or submodule drift) - OS errors during resolve/read Plus: missing anchor in extract_section was silently dropped with no log at all — now logs WARNING in resolve_skill_files since a missing anchor means a _emit() site typo or vendor submodule heading rename, both of which are real bugs. Refs #1182 * test: structural attach coverage + real-skill anchor resolution (T1, T2, T3) Two parametrized test classes that would have caught the bugs fixed earlier in this PR (the three missing attach_skill_content calls on python_transform / config_subentry branches) without needing the full fastmcp stack: * TestWriteToolAttachCoverage — AST-scans each of the six write tools, counts success-return paths, and asserts an attach-helper call exists for each. Pins the wrap-against-every-return-site contract structurally. * TestEveryEmittedAnchorResolves — extracts every literal anchor passed to _emit() in best_practice_checker, plus every per-tool canonical file mapping, and resolves each against the real bundled skills-vendor submodule. Catches submodule heading renames and _emit() typos that would otherwise produce silent empty skill_content with no test signal. Real-skill tests skip cleanly when the vendor submodule isn't initialised so a fresh clone doesn't fail collection. Refs #1182 * style: ruff SIM114 + PERF401 cleanup in skill_content_wiring test Combined the dual isinstance branches in _count_attach_calls into a single or-chained condition, and switched the inner-loop append in _canonical_files_mappings to list.extend with a generator. Behaviour-equivalent. Refs #1182 * docs(internal): restore full ha_call_service docstring from upstream ha_call_service has no skill_content, no best-practice checker, and no include_skill parameter — the original trim (commit 5694b3b9) plus the merge-conflict resolution dropped Basic Usage examples that nothing else replaces for this tool. Restored the full upstream post-#1447 docstring (Basic Usage + Key behavior + skill_guide pointer + Common patterns trailer) verbatim. Comment-analyzer agent flagged this as finding #4 earlier and I wrongly demoted it to LP4. Fixing now: this trim was asymmetric with the rest of the PR and the tool's high call frequency makes losing the docstring guidance especially costly. Refs #1182 * docs(internal): restore read-only tool docstrings — no skill_content path Five GET/LIST tools were trimmed under the same trim philosophy as the write tools, but they don't ship skill_content (only write tools do) and they're not best-practice-checker-gated either. The LLM has no alternate channel for the dropped guidance on these tools. Reverted the docstrings to their pre-trim verbatim shape: - ha_config_get_automation (was commit da9bc359) - ha_config_get_script (was commit b23b23e0) - ha_config_get_scene (was part of commit 46d49aa1; set_scene trim kept) - ha_config_list_helpers (was commit 639f8f0d) + flow-helper routing kept - ha_config_get_dashboard (was commit 7163e0c8) Addresses inline review comments on #1448 noting these trims were inappropriate for read-only tools that have no skill-fallback path. Refs #1182 * docs(internal): restore ha_config_set_automation docstring (move PREFER NATIVE to top) Addresses inline review on PR #1448. Per the reviewer's notes: - PREFER NATIVE OVER TEMPLATES block restored AND moved to the TOP of the docstring (was buried; a hallucinating LLM that's ignoring the skill content needs this front-and-centre to avoid reaching for Jinja first). - AUTOMATION TYPES + REQUIRED FIELDS (regular vs blueprint) restored — blueprint workflow has zero coverage in skill files. - OPTIONAL CONFIG FIELDS list restored — category / initial_state / variables aren't in skill content. - BASIC EXAMPLES + BLUEPRINT EXAMPLES restored — full worked configs for time-trigger, motion-light, update, blueprint create/update. - TRIGGER/CONDITION/ACTION TYPES enumeration restored — fills the zone-trigger / template-trigger / device-condition / parallel / delay gaps in automation-patterns.md. - TROUBLESHOOTING restored with the reviewer's requested clarification: ha_eval_template is for "IF you must use Jinja and have no native alternative" — frames it as a fallback, not a default. Kept the skill_content delivery note (auto-embed on warnings + canonical files via include_skill) since that's the new mechanism this PR adds. Refs #1182 * docs(internal): restore ha_config_set_script docstring (PREFER NATIVE at top, fields: + blueprints back) Per inline review: PREFER NATIVE OVER TEMPLATES moved to the top (reviewer marked line 434 'This absolutely needs to be left in, and needs to be at the very top.'). Restored sections: - SCRIPTS vs AUTOMATIONS routing kept high. - python_transform examples (delete/append/replace) restored — no skill file documents the syntax. - Required + optional config fields restored — script 'fields:' has zero skill coverage; its purpose (caller-supplied input with selector schema) is script-only. - Worked examples for delay, blink, parameterised backup, update, blueprint create/update restored — blueprint workflow has zero coverage in skill files. Refs #1182 * docs(internal): restore ha_config_set_scene docstring Per inline review (line 520): make sure worked example is in the docstring since SKILL.md (the only file set_scene ships) has no scene example. Restored the upstream WHEN TO USE / WHEN NOT TO USE / SCENE SHAPE / EXAMPLE structure verbatim. Kept the skill_content delivery note for the include_skill mechanism. Refs #1182 * docs(internal): restore ha_config_set_helper docstring Per inline review (line 2397): make sure most important + relevant info isn't excluded. Restored full upstream docstring covering: - SIMPLE vs FLOW vs CONFIG_SUBENTRY dispatch model with full type enumerations (helper-selection.md doesn't have a clean SIMPLE/FLOW table; 'trend' helper is in the FLOW list but missing from the skill file; subentry workflow has zero skill coverage). - All four required-params-by-mode rules. - Behavior notes (UPDATE preservation, action= disambiguation, silent-ignore + data_schema discovery, menu_options for menu-rooted types). - Worked tool-call examples for template / group / tod / config_subentry — these are first-call-payload-non-obvious shapes the LLM needs explicitly. The skill describes the menu flow in prose only; the tool-call shape was gone with no replacement. Refs #1182 * docs(internal): restore ha_config_set_dashboard docstring + clarify yaml-mode distinction Per inline review (lines 932, 1010, 559, 594, 576, 574): - python_transform examples (5 patterns: icon update, append, del, pattern loop, multi-op chain) restored — no skill file documents python_transform syntax for dashboards. - Strategy-based dashboard example + 'Take Control' caveat restored — strategy dashboards have zero skill coverage. - MODERN DASHBOARD BEST PRACTICES list restored. Dropped the stale '2024+' qualifier (it's 2026 now; this section is the current guidance, not a recent-add note). Reviewer flagged this should also be kept up to date in the skills repo separately. - title/icon/require_admin/show_in_sidebar update-alongside-config behavior note restored. - DISCOVERING ENTITY IDs section restored verbatim. - All 4 example shapes restored (empty / sections-tile / strategy / update). - NEW: STORAGE-MODE vs YAML-MODE DASHBOARDS section disambiguates the two YAML cases per reviewer's clarification at line 594 — dedicated .yaml file referenced from configuration.yaml, vs directly inlined under configuration.yaml's lovelace: key. Tool covers neither; pointer to ha_config_set_yaml for the latter. Refs #1182 * docs(internal): restore ha_config_set_yaml docstring (intact + skill_content note) Per inline review (line 182): leave docstring intact except the skill-access blurb. Restored full upstream LAST RESORT structure + routing bullets + intended-use scope + post_action / comment-tag / replace semantics. Helper-count corrected to 28 (was 27). Added the include_skill / skill_content delivery note for the new mechanism; kept the ha_get_skill_guide pointer for deeper guidance. Refs #1182 * docs(internal): minor wording revert to upstream verbatim on 3 docstrings Trimmed wording drift introduced during the partial restorations back to upstream-verbatim: - set_automation, set_script: 'before writing' (was 'BEFORE writing' — cosmetic case change reverted). - set_automation, set_script: integrated the skill_content auto-embed note as an additional sentence after the original 'will surface anything in a logic position' line rather than replacing that line — keeps the original wording intact. - set_script: restored 'Creates a new script or updates...' paragraph + 'fields:' wording + 'Create script with parameters:' header to upstream. SCRIPTS vs AUTOMATIONS moved back to its original position after Optional fields. - set_scene: SCENE SHAPE sentence reverted to upstream order ('Automations use a list of actions; scenes capture a snapshot of states as a dict'). Set_automation's PREFER NATIVE placement at the top, the ha_eval_template clarification, and the skill_content delivery notes are kept — those are the user-requested additions. Refs #1182 * feat: hide include_skill param from schema, teach opt-out via response hint BAT-confirmed regression: every LLM (Claude/GPT/Gemini variants) saw the default-on include_skill bool in the tool catalog and reflexively set it to False, defeating the proactive-skill-delivery design from PR #1448. Fix via FastMCP `exclude_args=["include_skill"]` on all six write-tool decorators. The JSON schema published to clients no longer lists the parameter, so LLMs can't pre-emptively disable what they can't see. The runtime still accepts include_skill=False when passed explicitly (Pydantic's default `additionalProperties: true`), so the opt-out path remains functional for callers who know about it. How the LLM learns about the (now-hidden) opt-out: when skill_content is actually delivered, `attach_skill_content` injects a sibling `skill_content_hint` field that names the parameter and tells the LLM to pass `include_skill=false` on subsequent calls if the content has already been received. The hint only ships alongside delivered content, so it can't be acted on before the model has seen what it's opting out of — eliminating the BAT-observed reflex-disable failure mode. Best-practice-checker route suffix becomes 2-route (skill:// URI + ha_get_skill_guide) — the previous 3rd route, "pass include_skill=True", is dropped because it pointed at a parameter no longer in the catalog. The auto-embed on warnings is unchanged; only the advertised routes are reduced. Tool docstrings drop the now-misleading "(see include_skill)" mentions on all six write tools — the param is hidden, so referring the LLM to it from the docstring was self-contradictory. Renamed `_three_route_suffix` → `_skill_route_suffix` and updated the module-level docs to describe the 2-route shape. Tests: - New `test_include_skill_is_hidden_from_tool_catalog` in test_skill_content_wiring.py — AST-pins exclude_args=["include_skill"] on every write tool's @tool / @mcp.tool decorator. - `test_happy_path_attaches_skill_content` now asserts the new `skill_content_hint` ships with delivered content. - `test_nothing_requested_is_silent` asserts the hint is absent when no content is delivered. - `TestThreeRouteWarningSuffix` updated: the include_skill-route test is replaced with `test_warning_does_not_mention_include_skill_param` (positive assertion that the hidden param is never named in warnings). Closes the BAT regression on PR #1448. Feature flag for the design is still issue #1182. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: hoist skill_content_hint to top of response, imperative wording BAT regression on the hidden-include_skill change: even Opus needed five tries to find and act on the opt-out hint when it trailed the ~25KB skill_content body; Sonnet/Haiku never found it. Cause is two-part — placement (LLMs process top-down and the hint was the last key after a giant payload) and voice (conditional "if ... then" reads as advisory, not actionable). Fix: reorder the response so skill_content_hint is the FIRST key and skill_content is the LAST, with the operation result fields (success, data, entity_id) sandwiched between. Reword the hint to imperative voice: "Pass `include_skill=false` on subsequent calls to this tool in this session to skip this content." Mutation is in place via response.clear() + re-insertion because the write-tool callers pass the dict by reference and expect their handle to keep pointing at the same response object. Test: new test_hint_appears_first_and_content_last in test_build_skill_content.py pins the key ordering contract and asserts the other response fields are preserved between hint and content. Existing test_happy_path_attaches_skill_content already asserts the hint value via the _SKILL_CONTENT_OPTOUT_HINT constant so the wording change is picked up there automatically. If smaller models still miss the hint after this, the fallback is to re-expose the param under an obscure name (visible-but-undescribed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: re-expose include_skill as opaque attach_skill_payload param BAT regression on the hidden-param + top-of-response-hint approach: no model ever opted out when the hint was visible at the top of the response, even after multiple calls in a session. Hidden visibility was working, but the LLM could not act on the hint because it couldn't see a callable param in the schema to set. Switching to the visible-but-opaque strategy: 1. Drop ``exclude_args=["attach_skill_payload"]`` from every write-tool decorator so the param appears in the published MCP schema again. 2. Rename ``include_skill`` → ``attach_skill_payload`` everywhere. The new name was chosen so the schema shows a default-True boolean with no obvious "this is the skill toggle, flip me to disable" semantic — "attach" reads as an internal data-shaping flag rather than a user-facing feature, "payload" suggests internal/advanced framing. 3. Strip the Pydantic Field description on the param across all 6 write tools (Field(default=True) only, no description=). The schema now publishes only the bare type + default, with no prose telling the model what the param does. 4. Remove the parameter's mention from every tool docstring (already done previously when the (see include_skill) parentheticals were dropped; the docstrings now describe only the by-default behaviour of skill_content, never the param). The opt-out hint (still imperative, still first key in response) becomes the ONLY surface that names the param and explains when to set it. A model that reflexively toggles unexplained schema parameters has nothing semantic to pull on; a model that needs to opt out reads the hint and now has a callable name plus a usage rule in the same sentence. Tests: - test_attach_skill_payload_is_visible_in_tool_catalog (NEW) — asserts the decorator does NOT carry exclude_args=["attach_skill_payload"], pinning the visibility decision so a future PR can't quietly re-hide the param and revert to the broken state. - test_attach_skill_payload_has_no_field_description (NEW) — asserts the Field on the param carries no description kwarg, pinning the opacity decision so a well-meaning future PR can't re-add a description that defeats the opacity. - All existing tests updated for the new param name (find/replace across test_build_skill_content.py, test_best_practice_checker.py, test_skill_content_wiring.py). Module docstrings (best_practice_checker.py, skill_loader.py, server.py) updated to reference the new name and the visible-opaque design rationale. If this fallback also fails BAT (small models don't pick up the hint), the next option is to leave behaviour alone but also embed the opt-out instruction inline at the START of the skill_content body itself so the LLM cannot avoid reading it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: rename attach_skill_payload to opaque enabled (BAT round 4) BAT round 3: Opus could still see ``attach_skill_payload`` had skill semantics in the name and reflex-disabled every try. The word "skill" in the param name was the giveaway — even with no description, the name itself told the model what it controlled. Renamed to ``enabled`` — completely generic, no domain semantic. A model scanning the schema sees a default-True boolean named "enabled" on a write tool and the natural read is "this gates whether the tool runs at all, don't touch it" — gatekeeper framing prevents the reflex-disable. The hint shipped inside the response when content is delivered clarifies that ``enabled=false`` only skips the skill_content payload, not the write itself. Hint text updated: "Pass ``enabled=false`` on subsequent calls to this tool in this session to skip this content." Still imperative, still first key in the response. Mechanical rename across 14 files (6 write tools + 2 internal helpers + util_helpers + best_practice_checker + skill_loader + server + 4 test files). No collisions with existing ``enabled`` usages (other occurrences are docstring text about automation enabled state, unrelated). If THIS still BATs poorly: - Opus picks up that "enabled=false" gates the write itself and stops using it → revisit with prior_skill_delivery (inverted polarity) reframe. - Models still don't opt out → fallback is to inline the directive at the top of the skill_content body itself (user's earlier ask), forcing the LLM to read it as part of the content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: rename enabled to MandatoryBPS, make REQUIRED (BAT round 5) BAT round 4: ``enabled`` worked as opacity for some models but Haiku just OMITTED the param entirely on every call (no default fallback to opt out, but also no signal it should opt in to skip content). Two changes this round: 1. Rename ``enabled`` → ``MandatoryBPS`` (Mandatory Best-Practice Skills). The capital-cased ``Mandatory`` prefix nudges models away from reflex-disabling — flipping a thing labelled "mandatory" to false reads as actively breaking something. Capital-cased to make the wordplay legible (Python convention is snake_case but the user's directive was explicit on the name). 2. Make the param REQUIRED at the schema level. Drop ``default=True`` from the Field, drop ``= True`` from the Python signature, and put a ``*,`` kwarg-only separator before it so Python's "non-default follows default" syntax rule doesn't fire. The MCP runtime always uses kwargs so no caller is broken. This forces the model to pass either true or false on every call — the previous "Haiku omits the param" failure mode is now impossible because FastMCP's Pydantic validator rejects the call without it. The default semantic remains "pass true if you have no opinion"; the response-side hint teaches when to pass false. Hint text auto-updated via the constant rename: "Pass ``MandatoryBPS=false`` on subsequent calls to this tool in this session to skip this content." Still imperative, still first key in the response. New structural test test_MandatoryBPS_is_required pins: * Field has no ``default`` / ``default_factory`` kwarg * Python signature has no default for MandatoryBPS (either no entry in kw_defaults for kwarg-only params, or non-positional placement) If Opus picks up that MandatoryBPS=false breaks something and refuses to ever set it false → revisit with reframed semantic (e.g. ``priorSkillReceived=false`` default which inverts polarity). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: revert MandatoryBPS to default=True (round 5 corrected) Misread the previous directive — user clarified the param should default to True (skill content delivered when omitted), not be required-without-default. With ``default=True``: - LLM passes true → content delivered - LLM passes false → content skipped - LLM omits → default kicks in, content delivered This matches the original safety semantic from when the param was ``include_skill``: omission is safe (content ships) rather than fail-loud. The opt-out hint in the response still teaches the LLM to pass ``MandatoryBPS=false`` when content is redundant. Changes: - Restored ``Field(default=True)`` on all 6 tool signatures. - Restored ``= True`` Python default on all 6 signatures. - Removed ``*,`` kwarg-only separator (no longer needed without the required-after-defaulted ordering problem). - Removed ``test_MandatoryBPS_is_required`` and its ``_MandatoryBPS_arg`` AST helper from test_skill_content_wiring.py — the required-contract no longer holds. - ``test_MandatoryBPS_has_no_field_description`` still pins the opacity contract (Field carries no description kwarg). - ``test_MandatoryBPS_is_visible_in_tool_catalog`` still pins visibility (no exclude_args). Hint constant unchanged: "Pass ``MandatoryBPS=false`` on subsequent calls to this tool in this session to skip this content." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(config): add ENABLE_MANDATORY_BPS master switch for skill_content delivery Adds an operator-controlled toggle for the write-tool skill_content feature (#1182) across all three configuration surfaces — env var, addon config, web UI — mirroring the enable_tool_search pattern. Setting sits ABOVE the per-call MandatoryBPS parameter as a server- wide master switch. When false, no skill_content goes out from any write tool regardless of the per-call param or BP-warning auto-embed. Default on — preserves the round-5 behaviour for existing operators. Wiring: - src/ha_mcp/config.py: ``enable_mandatory_bps`` Pydantic field with ``ENABLE_MANDATORY_BPS`` env alias, default True. Added to FEATURE_FLAG_FIELDS so the /api/settings/features endpoint advertises origin (env / addon / file / default) and validates writes the same way every other feature flag does. - src/ha_mcp/tools/util_helpers.py: master-switch check at the top of build_skill_content — returns empty dict when the setting is off, short-circuiting before the per-call canonical/referenced union and the I/O. - src/ha_mcp/settings_ui.py: FEATURE_META entry so the web UI surfaces label + help text matching the pattern of every other enable_* toggle. Add-on: - homeassistant-addon/config.yaml: ``enable_mandatory_bps: true`` in options + ``enable_mandatory_bps: bool?`` in schema. Visible in the stable addon Configuration tab. - homeassistant-addon-dev/config.yaml: same. - homeassistant-addon/translations/en.yaml: addon-tab name + description (kept in sync with web UI's help text per the comment above FEATURE_META). - homeassistant-addon-dev/translations/en.yaml: same. - homeassistant-addon/start.py: read raw value from /data/options.json with bool coercion (default True), emit ``ENABLE_MANDATORY_BPS`` env var alongside the other flags. Tests: - New ``test_master_switch_off_short_circuits`` in test_build_skill_content.py — patches get_global_settings to return a settings instance with enable_mandatory_bps=False and asserts build_skill_content returns empty even when the per-call MandatoryBPS=True or when referenced_files (BP-warning auto-embed) are supplied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: PR-toolkit findings — hint position, settings safety, UTF-8, stale docs Addresses real findings from the pr-review-toolkit batch run. Bug fixes: - build_skill_content + attach_skill_content now wrap get_global_settings in try/except → silently degrade to no skill_content. A cold-cache settings-load exception would otherwise bubble up, get re-mapped to INTERNAL_ERROR by the outer except-block of the write tool, and lead the agent to retry an already-committed mutation. - attach_skill_content's vendor-missing warning is suppressed when the master switch is off — the suppression cause is the operator config, not a missing submodule, so telling them to run `git submodule update --init` was misleading. - skill_loader._read_file_safely now catches UnicodeDecodeError separately from OSError. Invalid UTF-8 in a vendored skill file would otherwise propagate and fail a write the agent just committed. - ha_config_set_automation / _script / _scene config-update paths now call attach_skill_content AFTER building the outer return dict, so the hint-first response ordering survives. Previously the dict-spread ({"success": True, ..., **result}) pushed skill_content_hint to position 2-3 — the exact placement BAT showed small models can't find. dashboards / helpers / yaml / python_transform paths were already operating on the returned dict so they were unaffected. - homeassistant-addon/start.py now log_error's on an invalid (non-bool) enable_mandatory_bps value before falling back to default True, so the defensive coercion isn't silent. Stale comment/text fixes: - TestThreeRouteWarningSuffix → TestTwoRouteWarningSuffix; class docstring + inline comment + test docstring all stop claiming the param is "hidden via exclude_args" (it's visible). - test_build_skill_content's "hidden opt-out path" comment updated to "schema-visible-but-undescribed". - best_practice_checker.py module-docstring + _skill_route_suffix docstring no longer recite the param-design rationale verbatim; cross-reference util_helpers._SKILL_CONTENT_OPTOUT_HINT instead (eliminates the drift hazard between two copies). - tools_config_dashboards docstring's ha_config_set_yaml parenthetical now explicit that it only updates the registration entry, not the dashboard body in the referenced .yaml file. - util_helpers._SKILL_CONTENT_OPTOUT_HINT comment trimmed: no more five-round BAT history block in production code (per CLAUDE.md "don't reference fix history in code" — it rots and belongs in the PR description). One-paragraph "design rationale settled by BAT, don't tune casually" warning replaces the bullet list. Dead-code removal: - BestPracticeCheckResult.__copy__ / __deepcopy__ removed — zero callers in src or tests do copy.copy / copy.deepcopy on a result instance. Docstring updated to be honest that slicing / list() / copy.copy all drop the attribute and no call site exercises any of those paths. New tests: - test_master_off_with_vendor_missing_does_not_emit_warning — pins the suppression-cause-attribution fix above. - test_trailing_hash_resolves_to_whole_file — pins the (previously undefined) "references/foo.md#" trailing-empty-anchor behaviour so a refactor can't silently flip it. - test_enable_mandatory_bps_default_on + two parametrized coercion tests in tests/src/unit/test_config.py — pin Pydantic's bool accept/reject contract on the new env var. Skipped from the toolkit batch: - Loose structural attach-coverage inequality (would require fastmcp env to tighten with per-branch instance tests). - BestPracticeCheckResult dataclass refactor (works as-is; flagged as follow-up not blocker by type-design agent). - Duplicate import block in automations/scripts (isort-style nit; ruff passes; merging may not survive autoformat). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: auto-embed BP sections on errors + generic hint everywhere + skill_guide opt-out hint Closes the three design gaps surfaced by live BAT against the deployed PR. Before this commit, write-tool error responses dropped all skill_content — only success-path returns carried it, leaving the LLM without inline fix material on the exact code path where it needs it most (a write that just failed). Changes: 1. New helpers in util_helpers.py: - augment_error_dict_with_skill_content(error_dict, bp_warnings) mutates an error response in place to append the generic ha_get_skill_guide pointer to suggestions (idempotent) and, when bp_warnings has referenced_files, attach the matching section bodies under skill_content with skill_content_hint at the top. Canonical files NOT attached on errors (targeted section bodies are 1-5 KB; 25-37 KB canonical bundle would bloat errors without matching benefit). - augment_tool_error_with_skill_content(te, bp_warnings) wraps the dict mutator around a ToolError — used by each write tool's outer except handler. 2. Each of the 6 write tools' outer @tool method now wraps its body in: try: ... return response except ToolError as te: raise augment_tool_error_with_skill_content(te, bp_warnings) from None except Exception as e: error = exception_to_structured_error(..., raise_error=False) augment_error_dict_with_skill_content(error, bp_warnings) raise_tool_error(error) Six modifications instead of touching all 145 raise sites. The wrap captures every ToolError that bubbles to the outer handler, adds the generic hint, and embeds BP sections where bp_warnings has referenced_files. 3. ha_get_skill_guide Tier 3 (file content fetch) on the home-assistant-best-practices skill now prepends a skill_content_hint at the top of the response telling the LLM to pass MandatoryBPS=false on subsequent write-tool calls (avoids duplicate canonical delivery for smart clients that fetch skills proactively). Other skills (if any) unchanged. Tests added: - test_augment_error_adds_generic_hint_without_bp pins the no-BP-context case: every error gets the generic pointer. - test_augment_error_idempotent_on_re_raise pins that nested re-raise paths don't double-append the hint. - test_augment_error_embeds_bp_sections_when_referenced pins the with-BP case: section body inlined under skill_content with hint at top. Closes tasks #14, #15, #16. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(internal): prepend "MUST call ha_get_skill_guide first" on 6 write tools Front-loads the read-skills-then-write pattern on every write tool's docstring so an LLM scanning the tool catalog sees the directive before any of the substantive tool-specific guidance. Pairs with the existing ha_get_skill_guide Tier 3 response (commit 5bc31982) which prepends skill_content_hint telling the LLM to pass MandatoryBPS=false on subsequent write-tool calls — closing the read-then-write loop: 1. LLM sees write tool docstring → "MUST call ha_get_skill_guide first" 2. LLM calls ha_get_skill_guide → response top key is the opt-out hint 3. LLM calls write tool with MandatoryBPS=false → no duplicate content The single-line bare directive is deliberate (per maintainer request). The skill argument and which specific file to fetch are deferred to ha_get_skill_guide's own discovery flow (Tier 1 lists skills, Tier 2 lists files in a skill, Tier 3 reads content). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test+docs: address Patch76 PR review (P1-P3, P4 Option A, P6-P8) Test coverage additions: - test_augment_tool_error_wraps_dict_augmentation: pins the ToolError wrapper used by all 6 write tools' outer except handler. Decodes the JSON body, runs the dict augmentation (generic hint + section embed), re-encodes into a new ToolError. - test_augment_tool_error_falls_through_on_non_json_body: pins the defensive fall-through for non-JSON ToolError bodies. - test_settings_load_raises_returns_empty in test_build_skill_content: pins the broad-except graceful-degrade — get_global_settings() raising must short-circuit to {} so a settings-validation regression doesn't fail a write the agent already committed. - test_resolve_skill_files_oserror_during_resolve_returns_skipped in test_skill_loader: patches Path.resolve to raise OSError (e.g. ELOOP from a circular symlink), asserts silent-skip. - test_resolve_skill_files_invalid_utf8_returns_skipped in test_skill_loader: writes 0xff 0xfe bytes to a .md file, asserts the UnicodeDecodeError (subclass of ValueError, not OSError) is caught by the dedicated except clause and doesn't propagate. E2E test (tests/src/e2e/workflows/automation/test_skill_content_delivery.py): - test_default_mandatorybps_attaches_canonical_skill_content: real HA container + real FastMCP, asserts skill_content_hint is the FIRST key and skill_content contains the canonical files. - test_mandatorybps_false_suppresses_skill_content: explicit opt-out ships no skill_content / no hint. - test_bp_warning_auto_embeds_only_relevant_section: BP-checker fires on template-in-condition input + MandatoryBPS=False, response carries ONLY the section-anchored body (not the whole canonical file) — proves section-slicing works end-to-end through real FastMCP. Docstring tweak (P4 Option A, action-verb-first preserved): - All 6 write tools' first docstring line now reads "Create/Update <thing>. MUST call ha_get_skill_guide first." — satisfies the styleguide action-verb-first convention while keeping the BAT-tuned MUST directive on the same first line where catalog scanners see it. Previous structure put the MUST on its own first line above the action verb. Nits: - best_practice_checker.BestPracticeCheckResult: __slots__ added as one-line typo guard (prevents accidental attribute writes beyond referenced_files). - skill_loader.py: contract-stated comment replaces the "current _emit sites all pre-slugify" snapshot that would rot. - skill_loader.py module docstring: added caveat that top-level or near-EOF anchors return most of the file (the section runs to the next same/higher-level heading). Rejected: - P5 narrowing of `except Exception` around get_global_settings(). The broad except is deliberate graceful-degrade — narrowing to (OSError, ValidationError) would let a future AttributeError from Settings schema drift propagate and crash the write, which is the exact failure mode the broad except is designed to prevent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): patch get_global_settings at source + scope Path.resolve patch Two unit-test failures from 593bbefa, both caused by patch-target scope mistakes: 1. test_settings_load_raises_returns_empty was patching `ha_mcp.tools.util_helpers.get_global_settings` but the symbol isn't bound in that module's namespace — `build_skill_content` imports it function-locally via `from ..config import get_global_settings`. Patched at the source module instead. 2. test_resolve_skill_files_oserror_during_resolve_returns_skipped patched Path.resolve globally, which fired on the `skill_dir.resolve()` call in `resolve_skill_files` (line ~178) BEFORE reaching `_read_file_safely`. The outer call has no try/except so the OSError propagated and failed the test. Scoped the patch to only fire on `.md` suffixes, which is the only path that goes through `_read_file_safely`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test+fix: address Patch76 post-#1431 review (narrow except, 6-tool e2e, abs-path test) Patch76's #1431-aware re-review (verdict: mechanism "solid... ready"). Three non-blocking items addressed: 1. Narrow the settings-lookup except (util_helpers.py). build_skill_content and attach_skill_content wrapped get_global_settings() in a bare `except Exception`, masking programming bugs (AttributeError/ImportError) the same as a genuine config issue. Narrowed both to `except ValidationError` (the realistic Settings() config-load failure named in the existing comment) so real bugs now surface, per the repo's narrow-except convention. Also flipped the attach_skill_content fallback from `master_on = True` to `False`: on a settings-fetch failure we can't know the master state, so suppress the vendor-missing warning rather than emit a misleading one whose true cause was the settings fetch. 2. e2e skill_content delivery now covers all six write tools: - test_skill_content_delivery.py: parametrized on/off coverage for script / scene / helper / dashboard (hint-is-first-key + canonical files on default; suppression on MandatoryBPS=False). Automation keeps its explicit tests incl. the BP-warning section-slice. - test_yaml_config.py: TestYamlConfigSkillContentDelivery (on/off) using the module's mcp_client_with_yaml_config fixture (ha_config_set_yaml is feature-flag + component gated, can't share the automation-dir fixtures). The six tools have distinct success-return shapes where an ordering or wrong-dict bug would slip past the structural AST test. 3. test_skill_loader.py: test_resolve_skill_files_rejects_absolute_path (`/etc/passwd` passed directly) — guards the traversal check against a future refactor to a naive prefix match. Test bookkeeping for #1: test_settings_load_raises_returns_empty now feeds a real pydantic.ValidationError (via _make_validation_error()) instead of a plain ValueError, and test_settings_load_propagates_unexpected_error pins that a non-config error (AttributeError) is NOT swallowed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(addon): restore dev addon version to upstream dev374 The merge left the dev addon config.yaml version at dev373 while upstream/master is at dev374, making the PR diff show a backwards version bump. The version line is release-pipeline-owned; restoring it to upstream's value zeroes the spurious diff so the PR only changes enable_mandatory_bps. No stable-version change (stable matches upstream at 7.6.0). Audit note (re #1486, "addon config isn't auto-synced between flavors"): verified enable_mandatory_bps is present on BOTH stable and dev addon config.yaml (options + schema) and translations, is NOT in BETA_FEATURE_FIELDS (so not beta-gated), and is written unconditionally in start.py — i.e. the skill_content feature is available on stable, not accidentally dev-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes homeassistant-ai#504 — ha_search_entities(area_filter="salon") incorrectly returned entities from unrelated areas (e.g., climate.abc from area "abc").
Root cause: get_entities_by_area() used fuzzy matching on entity friendly names to guess area membership. calculate_partial_ratio("salon", "climatisation abc") returned 60 (meeting the default threshold), causing false positives.
Fix: Rewrote get_entities_by_area() to use the three HA registries (entity, device, area) for accurate area resolution instead of fuzzy name inference. Area query matching uses exact match first, then fuzzy match at threshold 80 against actual area names/IDs. Entity area resolution priority: entity direct area_id > device area_id.
Type of change
Testing
uv run pytest)uv run ruff check)Checklist