Round-trip a scalar wait_until as a scalar

[bdraco]

What does this implement/fix?

A scalar wait_until was rewritten on every save as wait_until: {id: ...}. The parser stores a bare scalar under id when the catalog shorthand is a gate key, but the emitter kept the catalog key and never collapsed it back. One shared shorthand_key helper in helpers/automation_keys.py now decides the key for both sides; a catalog wide test pins the emit and re-parse round trip.

A bare string in a condition position is the condition id with no config, as esphome reads it, so wait_until: api.connected decomposes to its condition and if: {condition: api.connected} no longer loses the gate on save; a non-string scalar under wait_until still round-trips as written.

Related issue or feature (if applicable):

• fixes Scalar wait_until round trips to wait_until: {id: ...} #2800

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.

[bdraco]

@esphbot review

[codspeed]

Merging this PR will not alter performance

✅ 28 untouched benchmarks
⏩ 1 skipped benchmark¹

---

_{Comparing wait-until-scalar-shorthand (ae0de0c) with main (d9afbf3)}

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 Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.75%. Comparing base (6783330) to head (ae0de0c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2825   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files         294      294           
  Lines       25384    25421   +37     
=======================================
+ Hits        25321    25359   +38     
+ Misses         63       62    -1     

Flag	Coverage Δ
py3.12	99.72% <100.00%> (+<0.01%)	⬆️
py3.14	99.60% <100.00%> (+<0.01%)	⬆️

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

Files with missing lines	Coverage Δ
...vice_builder/controllers/automations/_decompose.py	100.00% <100.00%> (ø)
..._device_builder/controllers/automations/catalog.py	100.00% <100.00%> (ø)
..._device_builder/controllers/automations/emitter.py	100.00% <100.00%> (+0.59%)	⬆️
..._device_builder/controllers/automations/parsing.py	100.00% <ø> (ø)
esphome_device_builder/helpers/automation_keys.py	100.00% <100.00%> (ø)
esphome_device_builder/models/automations.py	100.00% <ø> (ø)

... and 4 files with indirect coverage changes

🚀 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.

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified, and targeted round-trip tests are included.

Review effort: Lite
Findings: None

What changed in this PR

Fixes scalar wait_until round-tripping by sharing shorthand-key resolution between parsing and emitting.

Changes:

• Adds gate/action-list-aware shorthand resolution.
• Reuses the helper in parsing and emission.
• Adds scalar and lambda round-trip tests.

File	Summary
tests/​test_automations_writer.py	Tests wait_until lambda round-tripping.
tests/​test_automations_branches.py	Adds catalog-wide scalar shorthand coverage.
esphome_device_builder/​controllers/​automations/​parsing.py	Re-exports the shared helper.
esphome_device_builder/​controllers/​automations/​emitter.py	Uses shared shorthand resolution.
esphome_device_builder/​controllers/​automations/​_decompose.py	Implements shared shorthand-key logic.

---

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

[bdraco]

@esphbot review

[esphbot]

Previous review — superseded by a newer review below.

[bdraco]

@esphbot review

[esphbot]

Tip

No blocking issues found — ready to merge.

[bdraco]

@esphbot review

[bdraco]

@esphbot review

[bdraco]

The review ran against 4873c27; the docstring and catalog-test findings were already addressed in b9f8323 (the suggested fixpoint assertion and the assert ... is not None are what the branch carries).

Finding 3 taken in afff943: a bare string in a condition position now decomposes to that condition id with no config, which is how esphome's registry validator reads it, so wait_until: api.connected becomes its condition node and if: {condition: api.connected} keeps its gate on save. String items under and / or / not go through the same path, which covers the pre-existing drop you noted. A non-string scalar under wait_until (a tagged lambda, which esphome rejects) still round-trips as written.

Finding 2 declined: the id fallback for a mapping-only entry predates this PR, the {id: v} mapping is the valid form for such entries, and the parser must not refuse user content; esphome reports the real error on validate.

@esphbot review

[esphbot]

@bdraco: The review ran against 4873c27; the docstring and catalog-test findings were already addressed in b9...

Confirmed against afff943f. Docstring and fixpoint test both landed. Finding 2 dismissal accepted — noted as your call, not re-raised.

String path covers strings only. A !lambda condition loads as TaggedScalar, not str, so _decompose_condition_list still returns [] and drops that gate on save. Filed as pre-existing follow-up.

Two new suggestions: unknown bare string under wait_until now faults whole automation to read-only (previously lossless {id: ...}); and wait_until: api.connected re-emits expanded, not as a scalar.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Tip

No blocking issues found — ready to merge.

[bdraco]

Suggestion 1 taken in 452cd19: the string path is gated on catalog.condition_by_id, so a substitution or a typo under wait_until stays the lossless id param and the automation stays editable; if: {condition: <unknown>} still faults loud.

Finding 3 taken in the same commit: a condition body that is not a mapping, a condition id, or a list of those (an !include, a tagged scalar) now raises INVALID_ARGS, so the entry parses read-only instead of decomposing to no gate that the next save would delete. Pinned at the parser and through parse_device_yaml with an !include gate.

Finding 4 resolves with suggestion 1: an unknown string re-parses to the same id param, so the catalog-wide fixpoint test covers wait_until again instead of skipping gate-shorthand actions. A string naming a known condition emits identical YAML either way.

Suggestion 2 declined: collapsing a lone param-less condition back to the string spelling would also rewrite the documented {api.connected:} form into an undocumented one on save, so the emitter keeps canonicalising to the full condition: form.

@esphbot review

[esphbot]

@bdraco: Suggestion 1 taken in 452cd19: the string path is gated on catalog.condition_by_id, so a substit...

Verified 452cd19. Gate checks catalog.condition_by_id first, so unknown strings keep id param and stay editable; fixpoint test covers wait_until again. Unsupported condition body raises at _decompose_condition:240. Findings 1, 3, 4 closed; suggestion 2 decline accepted — canonicalising to condition: reaches a fixpoint after one save. One nit left: tagged body raises plain CommandError, so _safe_tree sets error without unsupported and frontend shows red alert on valid !include YAML. UnsupportedActionError fits better.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Tip

No blocking issues found — ready to merge.

[bdraco]

Taken in 5547721: a tagged scalar in a condition position raises UnsupportedActionError, so the entry carries unsupported and the editor shows the edit-in-YAML hint; a malformed body keeps the plain INVALID_ARGS. The message no longer names the ruamel type and the test matches on the wording, with the parse_device_yaml case asserting unsupported.

@esphbot review

[esphbot]

@bdraco: Taken in 5547721: a tagged scalar in a condition position raises UnsupportedActionError, so the ...

Confirmed at 5547721. TaggedScalar check precedes dict guard, so tagged gates take UnsupportedActionError and carry unsupported. Malformed bodies keep INVALID_ARGS — pinned by _decompose_condition_list(5). Ordering stays correct: TaggedScalar subclasses nothing string-like, quoted scalars do, so condition: "api.connected" still reads as condition id. Remaining notes non-blocking: three docstrings still describe unsupported as action-only, plus two pre-existing action-side drops for follow-up.

[esphbot]

Previous review — superseded by a newer review below.

[bdraco]

Suggestion 1 taken in 7f7f817: the class, _safe_tree, ParsedAutomation.unsupported and the automations/parse row in docs/API.md now describe the flag as a body the tree cannot represent, naming both the formless LVGL update and a tagged condition gate. The rename stays out of this PR.

Suggestion 2 is the pre-existing action-side drop and stays a follow-up, as you note. On the id fallback: the Visual Editor round-trips every parsed key through automations/upsert, including params without a config entry, so the fallback is lossless there.

@esphbot review

[esphbot]

Tip

No blocking issues found — ready to merge.

[esphbot]

@bdraco: Suggestion 1 taken in 7f7f817: the class, _safe_tree, ParsedAutomation.unsupported and the `au...

Confirmed resolved. Checked all four surfaces: UnsupportedActionError, _safe_tree, ParsedAutomation.unsupported, docs/API.md. No stale "known action with no structured form" phrasing left anywhere. Rename deferral agreed.

Deferral of suggestion 2 accepted. Filed as [Deferred] for traceability only.

id fallback answered — dropped.

One new note: line 199 gates on condition_by_id(...) is not None, which hydrates a body off disk. A missing body file returns None too, so a catalog-packaging regression would silently flip every wait_until: <condition> to the id shape. LazyBodyStore.is_known already exists — an is_known_condition twin of is_known_action makes it a frozenset probe. Non-blocking; the id fallback still round-trips losslessly.

[esphbot]

Previous review — superseded by a newer review below.

[esphbot]

Tip

No blocking issues found — ready to merge.

[bdraco]

Suggestion 1 taken in ae0de0c: catalog.is_known_condition answers from the condition id set, so the string branch never loads a body and a missing body cannot be mistaken for an unknown id.

Finding 3 declined: wait_until: {id: api.connected} is not a config esphome accepts ("Unable to find condition with the name 'id'"), while wait_until: api.connected is, and it means the condition gate. Emitting the valid form and reading it back as the gate keeps the YAML the source of truth; keeping the mapping would write an invalid file to preserve tree symmetry. Pinned in test_emit_wait_until_id_param_naming_a_condition_reparses_as_the_gate.

Finding 2 remains the deferred action-side follow-up.

@esphbot review

[esphbot]

@bdraco: Suggestion 1 taken in ae0de0c: catalog.is_known_condition answers from the condition id set, so ...

Verified ae0de0c. is_known_condition reads _CONDITION_IDS — same frozenset backing _CONDITION_STORE.is_known. No disk read on miss path, no packaging failure mistaken for unknown id.

Finding 3 decline accepted. wait_until.json carries only timeout, so {id: ...} names no validated key; bare scalar emits valid gate. YAML stays source of truth.

Finding 2 carried as [Deferred] suggestion, non-blocking.

[esphbot]

PR Review — Round-trip a scalar wait_until as a scalar

Merge-ready. The last open suggestion is fixed in ae0de0c, and the one remaining finding is the author's own deferred follow-up.

Verified statically against the head checkout (ae0de0c4; no interpreter in the review shell, so this is code and catalog reading, not a green test run):

• Suggestion Add section config API and user preferences #1 is resolved correctly. catalog.is_known_condition (catalog.py:320-322) answers from _CONDITION_IDS — the exact frozenset already bound as _CONDITION_STORE.is_known (catalog.py:167) — so the string branch is a frozenset probe with no disk read, and a missing body can no longer read as "not a condition". The new test_emit_wait_until_id_param_naming_a_condition_reparses_as_the_gate pins the emit/re-parse pair that the change turns on.

• Finding 3's decline holds up. definitions/automations/actions/wait_until.json lists only timeout in config_entries, so a {id: ...} body names no key ESPHome validates in that position, while the bare scalar is the documented gate form. Emitting the valid YAML and reading it back as the gate is the right trade; keeping the mapping would write a file ESPHome rejects. Not re-raised, per @bdraco: "keeping the mapping would write an invalid file to preserve tree symmetry."

• The shared helper stays behaviour-preserving outside wait_until. wait_until.json is still the only definition in definitions/automations/ with a gate-key scalar_shorthand_key, and no action has a shorthand equal to one of its accepts_action_list keys, so the only catalog entry whose emit shape moves is the one the bug is about.

• No cycle, no stale importers. helpers/automation_keys.py imports models/automations.py, which imports only models/common.py; DEFAULT_SHORTHAND_KEY and _shorthand_key have no surviving references outside the new helper.

• Every _decompose_* path that can now raise is parse-side and wrapped by _safe_tree, so the new faults surface as a flagged read-only automation rather than an unhandled error on the write path.

• 🟢 [Deferred] _decompose_action_list (_decompose.py:132-133) still drops a non-mapping action item silently — the action-side twin of the condition drop this PR fixes. Carried as a follow-up per @bdraco.

---

✅ Resolved since last review (1)

Previously-flagged issues verified fixed

• esphome_device_builder/controllers/automations/_decompose.py:195 Gate the string-as-condition branch on id membership, not on a successful body load

---

🟢 Suggestions

1. [Deferred] Action-list entries that are not mappings are still dropped silently
esphome_device_builder/controllers/automations/_decompose.py:132-133

Deferred per @bdraco: "Finding 2 remains the deferred action-side follow-up." Recorded here as a non-blocking follow-up, not a merge blocker.

This is the action-side twin of the condition-list drop this PR just fixed. _decompose_condition_list now raises UnsupportedActionError for a TaggedScalar and CommandError for any other non-mapping body, so the automation comes back read-only instead of losing the gate. _decompose_action_list still continues past a non-mapping / empty item, so an !included action item or a bare scalar under then: never reaches the tree and the next automations/upsert re-renders the handler without it.

Why it matters: that is silent YAML loss on save — the same failure class as the wait_until bug this PR closes — and it is currently pinned by test_decompose_action_list_skips_empty_and_non_dict_items, so the behaviour is locked in until that test is changed.

Suggested shape when you pick it up: mirror the new _decompose_condition ladder — UnsupportedActionError for a tagged item, CommandError for any other non-mapping item — so _safe_tree flags the automation instead of handing back a tree that is missing an action.

    for item in items:
        if not isinstance(item, dict) or not item:
            continue

---

Checklist

• Fix addresses the reported bug (scalar wait_until round-trip)
• Catalog lookups on the parse hot path avoid unnecessary disk reads
• Shared helper is behaviour-preserving outside the intended wait_until change
• Parse failures fail loud rather than losing user YAML — suggestion ＃1
• Error surfaced at the right severity for the user (unsupported vs invalid)
• Emit/parse fixpoint covered for every catalog entry, gate shorthand included
• Docstrings and docs match the contract the code now implements
• No new import cycles / correct layering for the shared helper
• No stale importers after moving DEFAULT_SHORTHAND_KEY
• Diff matches the PR description's stated approach; no scope creep

ℹ️ Triage summary

1 finding(s) deferred at a reviewer's request.

---

Silent Failure Analysis

🟠 **2. HIGH** — silent tag stripping on the fallback path
esphome_device_builder/controllers/automations/_decompose.py:195-208

Risk: The PR makes a tagged condition gate unsupported in mapping form (condition: !include x), but the shorthand scalar form of the same gate misses it: wait_until: !include gate.yaml is a TaggedScalar, not a str, so it falls to the else, and _render_value without preserve_tags returns bare str(value) (_decompose.py:346-348) — the tag is dropped with no error and no unsupported flag, then emit_action_node collapses {id: "gate.yaml"} back to wait_until: gate.yaml, silently writing away the !include.

elif (
    isinstance(raw_params, str)
    and action.scalar_shorthand_key in CONDITION_GATE_KEYS
    and catalog.is_known_condition(raw_params)
):
    ...
else:
    params = {scalar_param_key(action): _render_value(raw_params)}

Fix: Raise UnsupportedActionError for a TaggedScalar raw body whose scalar_shorthand_key is a gate key (mirroring the new _decompose_condition check), or route it through preserve_tags=True so the tag survives the round trip.

🟠 **3. HIGH** — silent drop of unrepresentable list entries
esphome_device_builder/controllers/automations/_decompose.py:131-141

Risk: Pre-existing issue, now the direct twin of what this PR fixes: _decompose_condition_list no longer drops non-mapping bodies, but the action list still silently skips them, so then: !include acts.yaml (or a - !include act.yaml entry in an action list) parses to an empty/short action list with error unset, and a subsequent upsert re-emits the automation without those actions — silent data loss rather than a read-only "edit in YAML" automation.

for item in items:
    if not isinstance(item, dict) or not item:
        continue

Fix: Apply the same treatment as the condition path: raise UnsupportedActionError for a tagged action-list body/entry and CommandError for other non-mapping entries, instead of continue.

🟡 **4. MEDIUM** — fallback value hides an unrecognised id
esphome_device_builder/controllers/automations/_decompose.py:195-208

Risk: Gating the string path on is_known_condition means a string in a condition position that is not catalogued (wait_until: api.conected, a typo) is silently reinterpreted as an id param and presented as a gate-less action, whereas the identical string reached via condition: api.conected raises Unknown condition id and flags the automation — the same YAML gets two different treatments and the typo never surfaces.

and catalog.is_known_condition(raw_params)
...
else:
    params = {scalar_param_key(action): _render_value(raw_params)}

Fix: Keep the lossless id fallback only for values that cannot be a condition id (a substitution like ${cond}), and fault with Unknown condition id for a plain unqualified/dotted string in a gate position.

---

Automated review by Kōan (Claude) HEAD=ae0de0c 6 min 1s

[esphbot]

Tip

No blocking issues found — ready to merge.