Skip to content

Resolve user-keyed map bases through extends-only wrappers - #2655

Merged
bdraco merged 5 commits into
mainfrom
sync-map-key-type-extends
Sep 3, 2026
Merged

Resolve user-keyed map bases through extends-only wrappers#2655
bdraco merged 5 commits into
mainfrom
sync-map-key-type-extends

Conversation

@bdraco

@bdraco bdraco commented Sep 3, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

The 2026.9.0b1 schema moved homeassistant.action's variables behind an extends-only wrapper (api.VARIABLES_SCHEMA), and the map collapse only looks for key_type on the raw entry; the nightly sync (#2653) therefore degrades the field from the map editor to a nested group holding a bogus literal string field. The same wrapper shape (api.KEY_VALUE_SCHEMA) has always degraded data and data_template on the homeassistant action and event actions.

Resolve key_type through the extends chain, so an extends-only wrapper whose base carries key_type collapses to map with the base's value template. A bundle-wide sweep shows exactly six entries change, all of them genuine user-keyed maps: data, data_template and variables on the two homeassistant actions. The catalog is regenerated here via script/sync_components.py --version 2026.9.0b1 against esphome 2026.9.0b1; the artifact churn is exactly the nine map repairs across the three homeassistant action bodies, plus esp8266.json picking up the d1_wroom_02 board option that landed in boards.index.json after the nightly components run.

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed
  • Companion frontend PR: esphome/device-builder-frontend#

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.
  • components.index.json / definitions/components/*.json have not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

@github-actions github-actions Bot added the bugfix Bug fix label Sep 3, 2026
@bdraco

bdraco commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

@codspeed-hq

codspeed-hq Bot commented Sep 3, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 28 untouched benchmarks
⏩ 1 skipped benchmark1


Comparing sync-map-key-type-extends (8f98e1e) with main (5a59109)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.72%. Comparing base (5a59109) to head (8f98e1e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2655   +/-   ##
=======================================
  Coverage   99.72%   99.72%           
=======================================
  Files         281      281           
  Lines       23911    23911           
=======================================
  Hits        23846    23846           
  Misses         65       65           
Flag Coverage Δ
py3.12 99.69% <ø> (ø)
py3.14 99.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@esphbot

esphbot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

@bdraco: @esphbot review

Reviewed. Fix correct, scope tight. Verified degraded shape in shipped homeassistant.action.jsondata/data_template nested with bogus string child, variables still map. Two nits, neither blocking: _extends_map_schema returns on first key_type base, so a multi-base wrapper would drop sibling fields silently; recursion lacks _seen_refs guard other extends walkers thread. Approving.

@esphbot

esphbot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco

bdraco commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

@bdraco
bdraco marked this pull request as ready for review September 3, 2026 08:49
Copilot AI lite review requested due to automatic review settings September 3, 2026 08:49
@bdraco

bdraco commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

@esphbot

esphbot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The PR includes an additional unrelated generated-catalog change (esp8266) that contradicts the PR description’s stated scope, and the new helper’s docstring violates the repo’s docstring style guidance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the component-catalog sync logic to correctly detect and collapse user-keyed map fields when the key_type marker is present only in an extends base (i.e., the field entry is an extends-only wrapper), preventing schema regressions that incorrectly render these fields as nested groups.

Changes:

  • Add _extends_map_schema() and extend _convert_field() to resolve key_type through extends-only wrapper chains and collapse to map using the base schema’s value template.
  • Add focused regression tests covering direct key_type, extends-only wrappers, multi-hop extends, and “don’t collapse” cases.
  • Update generated catalog entries so homeassistant.* action/event data, data_template, and variables render as map (plus an additional unrelated generated change in esp8266.json).
File summaries
File Description
script/sync_components.py Resolves map schemas through extends-only wrappers and uses the resolved base to build the map value template.
tests/test_sync_components_map_extends.py Adds regression coverage for extends-only wrapper map collapse behavior.
esphome_device_builder/definitions/automations/actions/homeassistant.action.json Generated catalog now renders data, data_template, and variables as map.
esphome_device_builder/definitions/automations/actions/homeassistant.event.json Generated catalog now renders data, data_template, and variables as map.
esphome_device_builder/definitions/automations/actions/homeassistant.service.json Generated catalog now renders data, data_template, and variables as map.
esphome_device_builder/definitions/components/esp8266.json Generated definition changed (includes d1_wroom_02 in board options), which appears unrelated to the PR’s stated scope.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread esphome_device_builder/definitions/components/esp8266.json
Comment thread script/sync_components.py Outdated
@bdraco

bdraco commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

@esphbot

esphbot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco

bdraco commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

@esphbot

esphbot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

PR Review — Resolve user-keyed map bases through extends-only wrappers

Merge-ready. All three findings from the prior round are fixed in f057902 + 8f98e1e, and I re-verified the regenerated artifacts against git history rather than taking the PR body's word for it.

What holds up under verification:

  • The bug and the fix are real, and the artifacts prove it. git show 225ac41a4^:.../homeassistant.action.json shows variables was "type":"map" with a {"key":"value","label":"Value"} template before the 2026.9.0b1 sync, and "type":"nested" with a bogus literal string child after it. This PR restores the map shape on all three homeassistant action bodies and additionally repairs data / data_template, which were degraded long before Sync component catalog from schema 2026.9.0b1 #2653. The emitted shape matches _build_map_value_template (sync_components.py:4388) exactly.
  • Churn is confined. git diff --stat main...HEAD is 6 files. A bundle-wide sweep for the degraded shape finds no remaining field-level occurrence; the only "key":"string" leftovers are substitutions.json / packages.json, which are component-root maps on a different code path — untouched and unworsened here (pre-existing, out of scope).
  • Prior findings closed. The sibling-base _resolve_extends sweep is now deferred behind if map_schema is None: return None (line 3528), so the ancestry flatten no longer runs for ordinary nested fields; an unlocatable base returns None instead of falling through as "resolved, no fields" (line 3513-3515); competing map bases with differing templates return None instead of first-wins (line 3522-3524). Each is pinned by a negative test, including both extends orderings.
  • No consumer breakage. _coerce_string_map_values (helpers/yaml/component.py:197) iterates component.config_entries only; _apply_preset_value's NESTED branch (controllers/components/_resolve.py:263) only sees component entries. controllers/automations/_decompose.py does not branch on ConfigEntryType at all, so the nested→map flip is a rendering change on a path the frontend already exercised for variables.
  • Recursion is safe. _extends_map_schema follows only extends edges and bails on any node with config_vars, so the documented lvgl WIDGET_TYPES cycle (a config_vars cycle, guarded by _seen_refs in _merge_extends_config_vars) cannot reach it; _lookup_schema_ref is @cached, so the recursive walk is cheap.

On the two Copilot comments, @bdraco is right on both and I verified independently: board_bodies/d1_wroom_02.json and its boards.index.json row landed in #2653 while esp8266.json's option list stayed stale, so the regeneration is deterministic catch-up (hand-reverting it would violate the no-hand-editing rule); the docstring at line 3507 is now a single-line contract under the 100-char limit.

  • No blocking issues.
  • No new findings this round.

✅ Resolved since last review (1)

Previously-flagged issues verified fixed
  • script/sync_components.py:3513 Sibling-base guard flattens the whole ancestry even when no map base exists (carried over, still open)


Checklist

  • Prior findings resolved (deferred sibling scan, unresolvable base, competing bases)
  • Fix addresses the described bug (extends-only wrapper degrades map to nested)
  • Regenerated artifacts match the canonical map shape and the pre-Sync component catalog from schema 2026.9.0b1 #2653 baseline
  • Artifact churn confined to the described scope; esp8266 drift explained and deterministic
  • No hand-edited catalog artifacts; automations.index.json needs no companion update
  • Existing direct key_type path unchanged and pinned by test
  • Edge cases covered (nested hop, non-map base, own config_vars, sibling base both orderings, unresolvable ref, competing bases, duplicate base)
  • No backward-incompatible change to consumers (decompose does not branch on entry type)
  • No unguarded recursion or sync-runtime regression
  • Line length / docstring style within repo conventions

Silent Failure Analysis

🟡 **1. MEDIUM** — guard relying on a helper that silently returns empty on lookup failure
script/sync_components.py:3529-3532

Risk: The top-level loop refuses to collapse when a ref is unlocatable, but this sibling guard uses _resolve_extends, which returns {} for a ref it cannot resolve, so a sibling base that is itself extends-only and points at a missing base passes the guard and its inherited fields are dropped from the catalog with no diagnostic.

# A sibling base contributing real fields would be silently dropped
# by the collapse — keep the nested group.
if any(_resolve_extends(ref, schema_dir) for ref in other_refs):
    return None

Fix: Make the sibling check resolution-aware — have it fail closed (return None, and log via _LOGGER) when any transitive extends ref under other_refs cannot be located, mirroring the _lookup_schema_ref is None guard above.

🟡 **2. MEDIUM** — unchecked None assigned into emitted output
script/sync_components.py:3913-3918

Risk: _build_map_value_template returns None when the resolved body has no config_vars or the sample field fails conversion — reachable now that map_schema may be a base body whose own schema node only carries extends — so the sync silently emits a type: "map" entry with config_entries: null and the frontend gets a row editor with no value shape.

if isinstance(map_schema, dict):
    entry["type"] = "map"
    entry["config_entries"] = _build_map_value_template(map_schema, schema_dir)
    return entry

Fix: Check the template before committing to the map shape: if it is None, log the component/key via _LOGGER and fall through to the nested-group path instead of emitting a value-less map.

🟡 **3. MEDIUM** — malformed input silently reclassified instead of rejected
script/sync_components.py:3517-3521

Risk: A base that declares key_type but whose schema body is missing or not a dict falls into the else branch and is treated as an ordinary sibling ref; _resolve_extends then yields {} for it, so it neither triggers the map collapse nor blocks a competing one — a real map base disappears without any signal.

schema_node = target.get("schema")
if "key_type" in target and isinstance(schema_node, dict):
    found = schema_node
else:
    found = _extends_map_schema(schema_node, schema_dir)

Fix: Split the condition so a key_type target with a non-dict body returns None explicitly (and logs), rather than falling through to the generic sibling path.


Automated review by Kōan (Claude) HEAD=8f98e1e 3 min 23s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco
bdraco merged commit 1a51dc0 into main Sep 3, 2026
22 checks passed
@bdraco
bdraco deleted the sync-map-key-type-extends branch September 3, 2026 09:16
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants