Skip to content

Commit 249bcd1

Browse files
kingpanther13claude
andcommitted
fix(settings-ui): unblock tool list rendering + always-emit settings_url
Two regressions surfaced while testing PR #1381 against Claude Desktop. 1. Settings page stuck on "Loading...". stopSidecar()'s JS confirm() prompt used a single-quoted string 'Stop the settings server?\n\n' inside the Python triple-quoted _SETTINGS_HTML. Python consumed the \n\n as literal newlines, so the rendered <script> contained a JS string spanning two physical lines — an unrecoverable SyntaxError that aborted the entire script before loadTools() could run. The in-page error handler (window.addEventListener('error', ...)) cannot catch parse-time errors, so the user saw only the initial "Loading..." indicator with no diagnostic. Escape the backslashes in the Python source so the JS engine sees the intended \n\n escape sequence. 2. settings_url invisible to fields=-projecting callers. An LLM that minimized payload via fields=["system_info"] (or any narrow projection) would lose the settings_url field, since the projection ran *after* settings_url was added to the result. With the field absent the LLM cannot hand the URL to the user even when it knows the user is asking for it. Move the settings_url emission to *after* project_fields so it survives every projection, and surface it in the main tool docstring (which LLMs read first) instead of relying on the fields= enum description that less-attentive LLMs may skip. Regression tests: - test_rendered_script_parses_as_javascript shells out to `node --check` against the rendered <script> body so any future raw-newline-in-JS-string regression fails fast with a precise parser diagnostic. Skipped when node is not on PATH (the test matrix installs node already). - test_settings_url_survives_fields_projection pins the new always-emit-regardless-of-projection contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2ad5114 commit 249bcd1

4 files changed

Lines changed: 109 additions & 10 deletions

File tree

src/ha_mcp/settings_ui.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ def apply_tool_visibility(
610610
async function stopSidecar() {
611611
const btn = document.getElementById('stopSidecarBtn');
612612
if (!confirm(
613-
'Stop the settings server?\n\n' +
613+
'Stop the settings server?\\n\\n' +
614614
'A "disabled" sentinel file will be written so the server does not ' +
615615
'respawn the next time ha-mcp starts. To re-enable, delete the file ' +
616616
'at ~/.ha-mcp/settings_ui_disabled (or unset HA_MCP_DISABLE_SETTINGS_UI).'

src/ha_mcp/tools/tools_search.py

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,7 +1231,10 @@ async def ha_get_overview(
12311231
"area_analysis, ai_insights, pagination, partial, warnings, "
12321232
"device_types, service_availability, system_info, "
12331233
"notification_count, notifications, repair_count, repairs, "
1234-
"repairs_error, tool_discovery."
1234+
"repairs_error, tool_discovery. The ``settings_url`` "
1235+
"field (stdio mode only, see tool description) is not "
1236+
"subject to this projection — it is always included "
1237+
"when the settings-UI sidecar is running."
12351238
),
12361239
),
12371240
] = None,
@@ -1249,6 +1252,14 @@ async def ha_get_overview(
12491252
Use fields= to project the response to only the keys you need — a
12501253
significantly smaller payload when fetching a single sub-section (e.g.
12511254
fields=["system_info"] returns just that section instead of the full overview).
1255+
1256+
When the ha-mcp settings-UI sidecar is running (stdio mode, e.g.
1257+
Claude Desktop / Claude Code), the response always includes a
1258+
``settings_url`` field — the local URL to the tool-configuration
1259+
page. Hand this URL to the user when they ask how to enable or
1260+
disable tools or change server settings. ``settings_url`` is
1261+
emitted regardless of ``fields=`` projection so it stays
1262+
discoverable even when callers minimize the response.
12521263
"""
12531264
# Validate fields= early so a malformed value returns VALIDATION_FAILED
12541265
# with parameter="fields" (ha_get_overview has no outer try/except, so
@@ -1441,16 +1452,22 @@ async def ha_get_overview(
14411452
# the FastMCP server directly. A leftover URL file from a prior
14421453
# stdio run on the same machine could in principle be surfaced
14431454
# by an HTTP-mode process — acceptable because the URL itself
1444-
# is gated by the random secret path either way. Added before
1445-
# the fields= projection so callers can request just
1446-
# ``settings_url`` via ``fields=["settings_url"]``.
1455+
# is gated by the random secret path either way.
1456+
#
1457+
# Added *after* ``project_fields`` so it survives every
1458+
# ``fields=`` projection — even an LLM that calls
1459+
# ``fields=["system_info"]`` (to minimize payload) still sees
1460+
# the URL and can hand it to the user. Hiding it behind the
1461+
# projection made it effectively invisible to less-attentive
1462+
# LLMs that scanned only the documented ``fields=`` enum.
14471463
from ..stdio_settings_sidecar import read_sidecar_url
14481464

1465+
projected = project_fields(result, parsed_fields)
14491466
sidecar_url = read_sidecar_url()
14501467
if sidecar_url:
1451-
result["settings_url"] = sidecar_url
1468+
projected["settings_url"] = sidecar_url
14521469

1453-
return project_fields(result, parsed_fields)
1470+
return projected
14541471

14551472
@mcp.tool(
14561473
tags={"Search & Discovery"},

tests/src/unit/test_overview_system_info.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,3 +372,28 @@ async def test_settings_url_omitted_when_no_sidecar(
372372
)
373373
result = await overview_tool(detail_level="minimal")
374374
assert "settings_url" not in result
375+
376+
@pytest.mark.asyncio
377+
async def test_settings_url_survives_fields_projection(
378+
self, overview_tool, monkeypatch
379+
):
380+
"""``settings_url`` MUST be returned even when ``fields=`` filters
381+
the rest of the payload.
382+
383+
A less-attentive LLM that minimizes payload via
384+
``fields=["system_info"]`` (or any narrow projection) would
385+
otherwise lose the URL silently — and the LLM cannot hand the
386+
user a URL it never receives. Pinning the post-projection
387+
emission keeps ``settings_url`` discoverable regardless of how
388+
the caller scopes the overview response.
389+
"""
390+
url = "http://127.0.0.1:8099/private_abc/settings"
391+
monkeypatch.setattr(
392+
"ha_mcp.stdio_settings_sidecar.read_sidecar_url",
393+
lambda: url,
394+
)
395+
result = await overview_tool(fields=["system_info"])
396+
assert result.get("settings_url") == url
397+
# system_info is still projected; settings_url is the only
398+
# extra survivor (plus the always-retained success/warnings).
399+
assert "system_info" in result

tests/src/unit/test_settings_ui.py

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -505,9 +505,7 @@ def _patch_supervisor_client(
505505
cm.__aenter__ = AsyncMock(return_value=mock_client)
506506
cm.__aexit__ = AsyncMock(return_value=None)
507507
factory = MagicMock(return_value=cm)
508-
patcher = patch(
509-
"ha_mcp.settings_ui.make_supervisor_httpx_client", factory
510-
)
508+
patcher = patch("ha_mcp.settings_ui.make_supervisor_httpx_client", factory)
511509
return patcher, mock_client
512510

513511
@pytest.mark.asyncio
@@ -707,3 +705,62 @@ async def test_invalid_slug_in_body_falls_back_to_self(self, monkeypatch, body):
707705
await restart(request)
708706

709707
mock_client.post.assert_awaited_once_with("/addons/self/restart")
708+
709+
710+
class TestRenderedHTMLJsSyntax:
711+
"""The settings page HTML is built from a Python triple-quoted string.
712+
713+
Python consumes ``\\n`` inside that string as a real newline. If the
714+
author writes ``'foo\\n\\nbar'`` intending the JavaScript escape
715+
sequence, the rendered ``<script>`` contains a raw newline inside a
716+
single-quoted JS string literal — an unrecoverable SyntaxError that
717+
aborts the *entire* script before any handler runs, leaving the
718+
page stuck on its initial ``Loading...`` indicator forever. The
719+
page-level ``window.addEventListener('error', ...)`` cannot catch
720+
parse-time SyntaxErrors, so the user sees no diagnostic at all.
721+
"""
722+
723+
def _extract_script(self) -> str:
724+
from ha_mcp.settings_ui import _SETTINGS_HTML
725+
726+
start = _SETTINGS_HTML.find("<script>")
727+
end = _SETTINGS_HTML.find("</script>")
728+
assert start != -1 and end != -1 and end > start
729+
return _SETTINGS_HTML[start + len("<script>") : end]
730+
731+
def test_rendered_script_parses_as_javascript(self, tmp_path: Path):
732+
"""The rendered ``<script>`` body must parse as valid JavaScript.
733+
734+
A parse-time SyntaxError aborts the entire script before any
735+
handler runs, leaving the page stuck on its initial
736+
``Loading...`` indicator with no in-page diagnostic (the
737+
page-level ``window.addEventListener('error', ...)`` cannot
738+
catch parse-time errors). The canonical trigger is a
739+
Python-consumed ``\\n`` escape leaving a raw newline inside a
740+
single-quoted JS string — but any other ill-formed JS would
741+
be just as fatal.
742+
743+
Shells out to ``node --check`` for a faithful parse. Skipped
744+
when node isn't available so a missing dev dependency doesn't
745+
block local runs; CI installs node (see ``test.yml``).
746+
"""
747+
import shutil
748+
import subprocess
749+
750+
if shutil.which("node") is None:
751+
pytest.skip("node not installed — install Node.js to run this test")
752+
753+
script = self._extract_script()
754+
js_file = tmp_path / "settings_ui_script.js"
755+
js_file.write_text(script, encoding="utf-8")
756+
result = subprocess.run(
757+
["node", "--check", str(js_file)],
758+
capture_output=True,
759+
text=True,
760+
check=False,
761+
)
762+
if result.returncode != 0:
763+
raise AssertionError(
764+
"Settings UI <script> body failed node --check:\n"
765+
f"stdout: {result.stdout}\nstderr: {result.stderr}"
766+
)

0 commit comments

Comments
 (0)