fix: name a representative error in the deep-search non-404 failed fragment#1930
Conversation
…agment The generic 'per-id fetch raised a non-404 error' bucket collapses every non-404, non-timeout exception into one opaque partial_reason fragment, with the real exception visible only at debug logging. Follow-up report on homeassistant-ai#1784: a box where every per-id script fetch returns a fast HTTP 500 because scripts.yaml contains a !secret reference read identically to the pre-homeassistant-ai#1810 timeout case. Capture one representative summary per type in the Attempt-C fetch closures (new summarize_fetch_error in _fetch.py: 'HTTP <code>: <first line>' for HomeAssistantAPIError, '<Type>: <first line>' otherwise, 160-char cap), return it as a trailing tuple slot from the automation/ script/scene helpers, and append it to the failed fragment as an 'e.g.'. No sample -> fragment wording byte-identical to today. 404/yaml_skipped and timeout classes keep their dedicated fragments and never sample.
|
@codex review — apply the review criteria in .gemini/styleguide.md in addition to AGENTS.md guidance |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
kingpanther13
left a comment
There was a problem hiding this comment.
Thanks — the mechanism is right, and I verified the plumbing end to end (classification isolation, byte-identical no-sample wording, all callers updated). Requesting changes on three items, all within files the PR already touches:
1. The headline example can't actually happen — re-mock the automation/script tests with a realistic 500 body. HA core's BaseEditConfigView.get() file-loads the YAML with no try/except before the id lookup, and the request handler only catches vol.Invalid/ServiceNotFound/Unauthorized — so the YAMLException escapes to aiohttp, whose production 500 body is the fixed 500 Internal Server Error text. "Secrets not supported in this YAML file" goes to the HA log, never the HTTP body. The real rendered sample for your scenario is HTTP 500: 500 Internal Server Error, not HTTP 500: Secrets not supported in this YAML file. Your own scene test already mocks the realistic form — but the automation/script tests in test_deep_search_tier3_parallel.py / test_ha_search_merge.py mock API error: 500 - Secrets not supported in this YAML file, pinning output real HA can't produce. Please re-mock those realistically, update the asserted samples, and correct the description's example. (The fix is still worthwhile — the sample names the 500 and sends the user to the logs.)
2. Capture only the first error instead of summarizing all N. Each closure runs summarize_fetch_error() and appends on every failure, but only failed_errors[0] is read — in exactly the scenario motivating this PR, that's N summarizations with N−1 discarded. Guard the append (if not failed_errors:) or use a scalar; three sites (_deep.py automation + script closures, _scenes.py). The "List membership tracks failed_count" comments describe an invariant nothing relies on — drop them with the change. While in there: the script generic except Exception branch (_deep.py:593) has no test through it; the guard change is a natural moment to add one.
3. Two test docstrings went stale. The 5→6-tuple widening makes test_deep_search_tier3_parallel.py:389 and :721 ("…returned 5-tuple") wrong — one-word fixes (5→6).
Minor, take or leave:
- Scene 404s can produce an
e.g. HTTP 404: …sample on the registry-failed fallback (the scene closure has no yaml_skipped branch), contradicting the description's "404s never produce a sample" — either exclude 404s from the scene sample or scope the claim to automations/scripts. - Since the real sample can't name the cause (see 1), consider a static hint when the sample is an HTTP 500: "check HA logs; a common cause is a
!secretin scripts.yaml/automations.yaml, which the per-id config endpoint rejects." That would deliver the five-minute diagnosis your report asked for.
Addresses the maintainer review on homeassistant-ai#1930. - The motivating sample can't be "HTTP 500: Secrets not supported...": HA core's config view file-loads the YAML with no try/except and the handler only catches vol.Invalid/ServiceNotFound/Unauthorized, so the YAMLException escapes to aiohttp and the 500 body is the generic "500 Internal Server Error" placeholder. Re-mocked the automation/script tests + summarizer units to that realistic body and updated the asserted samples. - Guard the failed-sample capture to the FIRST failure (new record_first_failure in _fetch.py): N failures now do 1 summarize_fetch_error call, not N. All failures still counted. Dropped the stale "List membership tracks failed_count" comments. Added a test through the script generic except branch and a spy test pinning exactly-once summarization. - Since an HTTP 500 body can't name the cause, append a static HA-log hint (http_500_diagnosis_hint) pointing at the log + the common !secret cause. Kept endpoint-agnostic (names no specific YAML file) so it reads correctly on the shared scene path too. Scoped to HTTP-500 samples only; non-500 fragments stay byte-identical. - Fixed two stale 5-tuple -> 6-tuple docstrings.
|
@kingpanther13 thanks for the thorough pass, all three are fixed and pushed. Walking through them: The headline example. You're right, and this was the important one. I traced it back through the client and the sample my own PR text showed can't actually render. Capture only the first error. Good catch on the N summarizations. Pulled the guarded append into a Stale docstrings. Fixed both, 5-tuple to 6-tuple at On the two minor ones: I took the scene-404 point by scoping the "404s never produce a sample" claim to automations/scripts in the description rather than adding a scene 404 branch, since a scene per-id 404 falling into the generic failed class is a faithful "this fetch 404'd" signal, not a misclassification (scenes pre-filter integration-managed ones upstream via the registry walk instead). And I took the static-500-hint suggestion. Worth flagging: my first cut of that hint named Full unit suite is green here apart from the same 3 |
kingpanther13
left a comment
There was a problem hiding this comment.
Round 2 verified — all three requested items addressed cleanly (realistic 500 mocks, guarded first-capture with the spy test, docstring fixes), plus both minors taken with the endpoint-agnostic hint done right. Remaining minor polish (a scene-404 regression pin and hardening the 500-hint selection) will land in a follow-up. Thanks for the thorough response — great first contribution.
🧪 Your changes are now in the dev channel!Your PR has been merged to master and is available for testing in the dev channel. Test your changes before the next stable release (biweekly Wednesday): Quick start# Run dev version
uvx ha-mcp-dev
# Check version
uvx ha-mcp-dev --versionDocker: docker pull ghcr.io/homeassistant-ai/ha-mcp:dev
docker run --rm -i \
-e HOMEASSISTANT_URL=http://your-ha:8123 \
-e HOMEASSISTANT_TOKEN=your_token \
ghcr.io/homeassistant-ai/ha-mcp:devFound an issue? Please open a new bug report and mention this PR for context. |
) * fix: prefer an HTTP-500 sample for the deep-search failed fragment and pin the scene-404 asymmetry Post-merge polish on #1930 (the #1784 follow-up): - record_first_failure now upgrades a non-500 sample once when the first HTTP 500 arrives: failure order under asyncio.gather tracks latency, so a fast-failing outlier (e.g. a connection blip) could previously claim the sample slot and suppress the http_500_diagnosis_hint diagnosis in exactly the mixed-failure runs the hint exists for. Still at most two summarize_fetch_error calls per pass. - The 500-hint predicate is a shared regex (HTTP 500 followed by ':' or end-of-string) instead of a startswith on the rendered string, so the hint and the upgrade can never disagree and a bare "HTTP 500" (empty error body) no longer silently drops the hint. - The three call-site comments no longer claim the closure holds the guard (it lives in record_first_failure). - New tests: scene per-id 404 surfacing as an e.g. HTTP 404 sample with no 500 hint (pins the asymmetry the #1930 description disclosed), an automation seam test through public deep_search, record_first_failure selection semantics incl. an at-most-two-summaries spy, bare/near-miss hint predicate cases, the both-types-500 double-hint composition, and failed_sample is None asserts on the existing timeout/yaml unpacks. * test: pin the bare HTTP 500 render for empty-bodied 500s Review finding: the hint-side test hard-codes the "HTTP 500" literal, but the renderer side of the chain — summarize_fetch_error emitting the bare colon-less form the _HTTP_500_SAMPLE '$' alternative exists for — was unpinned; a rendering change would have silently re-dropped the hint with a green suite. --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
What does this PR do?
Follow-up to my comment on #1784. That fix split the timeout class out of the generic "per-id fetch raised a non-404 error" bucket, but every other non-404 exception still collapses into the same opaque string. On my box that bucket was hiding a fast HTTP 500 on every per-id script fetch: HA core's script config view file-loads scripts.yaml on each request and dies on a
!secretreference (annotatedyaml.YAMLException: Secrets not supported in this YAML file) before it ever gets to the id lookup. Nothing timed out, but the warning read identically to the pre-#1810 timeout case, and the actual error was only visible after a debug-log dive. I know ha_search is reworked in the custom component, but the container/addon/uvx server path insrc/still carries this exact bucket, so filing the fix there.This PR makes the failed fragment name one representative error inline:
Correction from the first revision (thanks @kingpanther13 for catching it): the rendered sample is
HTTP 500: 500 Internal Server Error, notHTTP 500: Secrets not supported in this YAML file. ThatYAMLExceptionmessage never reaches the HTTP body — core'sBaseEditConfigView.get()file-loads the YAML with no try/except and the request handler only catchesvol.Invalid/ServiceNotFound/Unauthorized, so the exception escapes to aiohttp, whose production 500 body is the fixed500 Internal Server Errortext. The!secretdetail goes to the HA log only. So the sample can name the status but not the cause — which is why the fragment now also appends a static hint pointing at the HA log and the single most common cause (a!secretthe per-id config endpoint rejects). The hint is deliberately endpoint-agnostic — it names no specific YAML file, since the same string rides the automation, script, and scene fragments and a scene 500 has nothing to do with scripts.yaml. That delivers the five-minute diagnosis the report asked for without the response claiming a cause it can't actually confirm. My test mocks now use the realistic500 Internal Server Errorbody throughout.Implementation: a new
summarize_fetch_error()in_fetch.pyrenders an exception asHTTP <code>: <first line of the message>forHomeAssistantAPIError(stripping the client's ownAPI error: <code> -prefix so the code isn't stated twice) or<ExceptionType>: <first line>otherwise, capped at 160 chars. A companionhttp_500_diagnosis_hint()returns the static HA-log hint for anHTTP 500:sample and""for anything else (so non-500 fragments stay byte-identical). The Attempt-C fetch closures for automations, scripts, and scenes capture a summary on the generic failed class only, and only the first one — the append is now guarded (if not failed_errors:), so the motivating "every per-id fetch 500s" case does N−1 fewersummarize_fetch_errorcalls; every failure still counts. The per-type helpers return the sample as a new trailing tuple slot, and_apply_per_type_partial_flag/_apply_scene_partial_flagappend it as ane.g.(plus the 500 hint) when present. With no sample the fragment wording is byte-identical to today, so anything matching on the current string is unaffected.One asymmetry worth flagging: automations and scripts have a dedicated
yaml_skippedbranch that classifies a per-id 404 out of the generic bucket, so their 404s never produce a sample. Scenes have no such branch (their integration-managed scenes are pre-filtered upstream via the registry walk instead), so a scene per-id 404 does fall through to the genericfailedclass and can surface ane.g. HTTP 404: …sample. I left that as-is rather than adding a scene 404 branch — it's a faithful "this scene fetch failed with a 404" signal, not a misclassification — but calling it out so the "404s never produce a sample" statement is scoped to automations/scripts, not scenes.Tests were written first per the TDD guideline: unit tests for the summarizer and the 500-hint scoping, fragment-assembly tests for both flag helpers with and without a sample (including that the hint rides only HTTP-500 samples), component tests pinning the new tuple slot (including that 404s and timeouts leave it
Noneon the automation/script path, and a new test through the script closure's genericexcept Exceptionbranch), a test proving the first-error guard summarizes exactly once for N failures, and seam tests driving my exact scenario end to end through publicdeep_searchfor scripts and scenes. One existing scene test (test_partial_reason_distinguishes_integration_from_failure) pinned the exact old fragment while its fixture drives a realRuntimeError; updated it to pin the new sample-carrying wording. Full unit suite is green locally apart from 3 failures intest_helper_response_shape.pythat fail identically on a clean master checkout here, so I'm reading those as environment-specific; I didn't run the Docker e2e suite locally and am counting on CI for that.Type of change
Testing
uv run pytest)uv run ruff check)Checklist