From 6beed8945319027eaa658a634442263dd2548ce1 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Sep 2026 12:32:51 +0100 Subject: [PATCH 1/2] Resolve an id-less script by the id its listing shows The parser lists an id-less top-level script under script_, but the writer matched only a literal id, so a replace aimed at that row appended a duplicate on the draft path and a delete answered not found. Both writers now resolve the row through the parser's own identity rule, and a replace writes the listed id onto the item, which esphome requires anyway. --- docs/API.md | 2 +- .../controllers/automations/parsing.py | 4 +- .../controllers/automations/writing.py | 47 +++++++++--------- esphome_device_builder/models/automations.py | 2 +- tests/test_automations_delete_save.py | 24 +++++---- tests/test_automations_writer.py | 49 +++++++++++++++++++ 6 files changed, 86 insertions(+), 42 deletions(-) diff --git a/docs/API.md b/docs/API.md index d5234b122..15408e2bc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -330,7 +330,7 @@ Parsing and writing live on the backend: the frontend exchanges structured `Auto | `automations/get_filters` | `{}` | `[Filter]` | Full sensor / binary_sensor / text_sensor filter catalog. Each entry's `applies_to` lists the component domains the filter is valid on; the REGISTRY_LIST renderer uses it to scope the per-row picker. | | `automations/get_available` | `{configuration, yaml?}` | `{triggers, actions, conditions, scripts, devices}` | Scoped catalog for a single device. Optional `yaml` scopes off the editor's unsaved draft instead of disk, so a component just added through the wizard exposes its triggers before the global save. `triggers` / `actions` / `conditions` filtered to the components present in the YAML, matched by the catalog's canonical `.` form (e.g. an action with `domain == "switch.template"` only surfaces when a switch with `platform: template` is configured); `core` items (control flow, lambda, combinators) and device-level triggers are always included. `scripts` lists declared `script: id`s with their `parameters:` map. `devices` lists every configured component instance with its `id` / `name` for id-picker dropdowns, plus `title` — the component's catalog display name (e.g. `wifi` → "WiFi Component"), shown when `name` is unset so a nameless singleton reads as a human name instead of the raw key. Each instance carries `has_explicit_id`: false means `id` is the parser/writer round-trip identity synthesized for an id-less block (the domain for a singleton, `_` positional, `_` for sub-entities), not a YAML-referenceable id — the frontend must never write it into an action's reference param (#2208). A multi-entity platform component (e.g. `sensor: - platform: aht10`) also lists each configured nested sub-entity (`temperature` / `humidity`) as its own instance keyed on the bare sub-domain (`component_id: "sensor"`) with `parent_id` set to the container — on its declared `id:`, or on the synthetic `_` id when the sub-block has none — and always marks the platform item `is_entity_container: true` (even with no sub-blocks configured) so the frontend offers entity triggers on the sub-entities and never on the container. | | `automations/parse` | `{configuration, yaml?}` | `[ParsedAutomation]` | Walk the device YAML and return every recognised automation (top-level `script:` / `interval:`, a mapping-form block counting as its one entry at index 0 or the script's id, `api.actions:`, device-level `esphome.on_*`, inline component `on_*:`, light `effects:` entries). An unknown *condition* id (or a misrouted / malformed body) flags only its own automation (`error` set, empty tree); siblings still parse. An uncatalogued single-key **action** (from an `external_components` source, or a typo) instead decomposes to an opaque passthrough `ActionNode` — `unknown: true` with the body on `raw_body` (tags carried via the passthrough tag sentinel above) — so its catalogued siblings stay editable; the frontend renders that one node read-only. A *known* action with no structured form (an oversized LVGL `*.update`) additionally sets `unsupported: true`, so the editor shows the neutral "edit in YAML" hint instead of an error alert. A YAML that won't load at all raises `INVALID_ARGS`. Optional `yaml` parses the unsaved draft instead of disk. | -| `automations/upsert` | `{configuration, automation, location, yaml?, save?, expected?}` | `{yaml_diff: YamlDiff}` | Insert or replace one automation at `location`; returns the splice the frontend applies in place. Optional `yaml` splices into the unsaved draft instead of disk. `save` and `expected` behave as for `automations/delete`; with either, the write is guarded: a replace needs `expected` to still match, and an insert must leave every existing automation intact and put the new one at `location` (an occupied index, or a location with no index aimed at a list-shaped handler, is refused; an append at the end is not, even when it turns a bare action list into `then:` entries); otherwise `precondition_failed` with nothing written. An index past the end, or one holding a list entry the parser never lists (an `!include`d item, a multi-key `effects:` entry), is `invalid_args`; the latter names the index to append at. A top-level `interval:` or `script:` written as a single block mapping, or as a one-line flow mapping or list, is rewritten as a block list by any write (upsert or delete) before the write lands; a value the line splicers cannot see is `invalid_args` naming its shape: a flow value spanning several lines, an inline scalar, a tagged value such as `script: !include scripts.yaml` (edit the included file instead), an alias as the whole value or inside a flow value, or a block whose anchor is aliased (listifying it drops or reshapes the anchored value; a flow block is refused for any aliased anchor under it, since re-dumping drops them all). | +| `automations/upsert` | `{configuration, automation, location, yaml?, save?, expected?}` | `{yaml_diff: YamlDiff}` | Insert or replace one automation at `location`; returns the splice the frontend applies in place. An id-less `script:` item is addressed by the `script_` id `automations/parse` lists it under; replacing it writes that id onto the item. Optional `yaml` splices into the unsaved draft instead of disk. `save` and `expected` behave as for `automations/delete`; with either, the write is guarded: a replace needs `expected` to still match, and an insert must leave every existing automation intact and put the new one at `location` (an occupied index, or a location with no index aimed at a list-shaped handler, is refused; an append at the end is not, even when it turns a bare action list into `then:` entries); otherwise `precondition_failed` with nothing written. An index past the end, or one holding a list entry the parser never lists (an `!include`d item, a multi-key `effects:` entry), is `invalid_args`; the latter names the index to append at. A top-level `interval:` or `script:` written as a single block mapping, or as a one-line flow mapping or list, is rewritten as a block list by any write (upsert or delete) before the write lands; a value the line splicers cannot see is `invalid_args` naming its shape: a flow value spanning several lines, an inline scalar, a tagged value such as `script: !include scripts.yaml` (edit the included file instead), an alias as the whole value or inside a flow value, or a block whose anchor is aliased (listifying it drops or reshapes the anchored value; a flow block is refused for any aliased anchor under it, since re-dumping drops them all). | | `automations/delete` | `{configuration, location, yaml?, save?, expected?}` | `{yaml_diff: YamlDiff}` | Remove the automation at `location`. `expected` is the `raw_yaml` `automations/parse` returned for it (trailing newlines are not compared): the delete then happens only while the automation at that location still reads that way, and answers `precondition_failed` with nothing written if it changed or moved (a location is positional) or the file no longer loads; a changed automation's message carries the first lines of a diff from `expected` to its current text. Optional `yaml` splices into the unsaved draft instead of disk. `save: true` also writes the result to the on-disk config, for callers with no draft buffer: the read, the splice and the atomic write run as one job under the file's write lock, followed by the same history commit and rescan as `devices/update_config`. It is refused with `INVALID_ARGS` alongside `yaml`, and when the result would be an empty file, as `devices/update_config` refuses one. | ### Editor diff --git a/esphome_device_builder/controllers/automations/parsing.py b/esphome_device_builder/controllers/automations/parsing.py index 77b4f3c89..55bd9890f 100644 --- a/esphome_device_builder/controllers/automations/parsing.py +++ b/esphome_device_builder/controllers/automations/parsing.py @@ -230,8 +230,8 @@ def _parse_top_level_scripts(root: Any) -> list[ParsedAutomation]: scripts, range_of = listed def _describe(item: dict[str, Any], idx: int) -> tuple[AutomationLocation, str]: - script_id = item.get("id") or f"script_{idx}" - return ScriptLocation(id=str(script_id)), f"Script: {script_id}" + script_id = instance_id("script", item, idx, is_list=True) + return ScriptLocation(id=script_id), f"Script: {script_id}" return _parse_automation_list( scripts, diff --git a/esphome_device_builder/controllers/automations/writing.py b/esphome_device_builder/controllers/automations/writing.py index dbf56679e..c26c3727d 100644 --- a/esphome_device_builder/controllers/automations/writing.py +++ b/esphome_device_builder/controllers/automations/writing.py @@ -67,6 +67,7 @@ from .parsing import ( ComponentTarget, component_action_field_paths, + instance_id, is_mapping_entry, make_yaml, resolve_action_field_target, @@ -156,7 +157,7 @@ def _upsert_script( ) -> tuple[str, YamlDiff]: """Splice or replace a top-level ``script:`` list item.""" rendered = render_script_item(tree, location.id) - return _upsert_top_level_list(yaml_text, "script", rendered, location.id, "id") + return _upsert_top_level_list(yaml_text, "script", rendered, location.id) def _upsert_interval( @@ -476,23 +477,26 @@ def _upsert_top_level_list( domain: str, rendered_item: str, item_id: str, - id_key: str, ) -> tuple[str, YamlDiff]: - """Insert / replace a list item identified by a string id field.""" - yaml = make_yaml() - data = yaml.load(yaml_text) or {} - items = data.get(domain) if isinstance(data, dict) else None - existing_idx: int | None = None - if isinstance(items, list): - for idx, raw in enumerate(items): - if isinstance(raw, dict) and str(raw.get(id_key, "")) == item_id: - existing_idx = idx - break + """Insert / replace the list item the parser lists as *item_id*.""" + existing_idx = _top_level_item_index(yaml_text, domain, item_id) if existing_idx is None: return _append_top_level_list(yaml_text, domain, rendered_item) return _replace_top_level_list_item(yaml_text, domain, existing_idx, rendered_item) +def _top_level_item_index(yaml_text: str, domain: str, item_id: str) -> int | None: + """Index of the first ``:`` list item the parser lists as *item_id*.""" + data = make_yaml().load(yaml_text) or {} + items = data.get(domain) if isinstance(data, dict) else None + if not isinstance(items, list): + return None + for idx, raw in enumerate(items): + if is_mapping_entry(raw) and instance_id(domain, raw, idx, is_list=True) == item_id: + return idx + return None + + @in_list_form def _upsert_top_level_list_indexed( yaml_text: str, @@ -596,7 +600,7 @@ def _delete_top_level( ) -> tuple[str, YamlDiff]: """Drop a top-level script / interval / device-on block.""" if isinstance(location, ScriptLocation): - return _delete_top_level_list_by_id(yaml_text, "script", "id", location.id) + return _delete_top_level_list_by_id(yaml_text, "script", location.id) if isinstance(location, IntervalLocation): return _delete_top_level_list_by_index(yaml_text, "interval", location.index) if isinstance(location, DeviceOnLocation): @@ -615,21 +619,14 @@ def _delete_top_level( def _delete_top_level_list_by_id( yaml_text: str, domain: str, - id_key: str, item_id: str, ) -> tuple[str, YamlDiff]: - """Remove the list item under ``:`` whose ``id`` matches.""" - yaml = make_yaml() - data = yaml.load(yaml_text) or {} - items = data.get(domain) if isinstance(data, dict) else None - if not isinstance(items, list): - msg = f"Block {domain!r} not present; nothing to delete" + """Remove the list item under ``:`` the parser lists as *item_id*.""" + idx = _top_level_item_index(yaml_text, domain, item_id) + if idx is None: + msg = f"{domain}:[id={item_id!r}] not present" raise CommandError(ErrorCode.NOT_FOUND, msg) - for idx, raw in enumerate(items): - if isinstance(raw, dict) and str(raw.get(id_key, "")) == item_id: - return _delete_list_item_lines(yaml_text, domain, idx) - msg = f"{domain}:[{id_key}={item_id!r}] not present" - raise CommandError(ErrorCode.NOT_FOUND, msg) + return _delete_list_item_lines(yaml_text, domain, idx) @in_list_form diff --git a/esphome_device_builder/models/automations.py b/esphome_device_builder/models/automations.py index 2d9b73ad1..210b53991 100644 --- a/esphome_device_builder/models/automations.py +++ b/esphome_device_builder/models/automations.py @@ -285,7 +285,7 @@ class AutomationCatalogIndex(DashboardModel): @dataclass class ScriptLocation(DashboardModel): - """A top-level ``script:`` list item, keyed by the script's ``id``.""" + """A top-level ``script:`` list item, keyed by its ``id``; ``script_`` when id-less.""" id: str kind: Literal["script"] = "script" diff --git a/tests/test_automations_delete_save.py b/tests/test_automations_delete_save.py index e6744d04d..7f53cfa99 100644 --- a/tests/test_automations_delete_save.py +++ b/tests/test_automations_delete_save.py @@ -519,25 +519,23 @@ async def test_upsert_with_save_refuses_an_insert_that_is_not_clean( assert devices.saved == [] -async def test_upsert_with_expected_refuses_when_the_writer_would_append_instead( +async def test_upsert_with_expected_replaces_an_idless_script_under_its_listed_id( tmp_path: Path, ) -> None: - idless = "script:\n - then:\n - delay: 1s\n" + idless = "script:\n - then:\n - delay: 2s\n" controller, devices = _setup(tmp_path, idless) shown = (await asyncio.to_thread(parsing.parse_device_yaml, idless))[0] - with pytest.raises(CommandError) as excinfo: - await controller.upsert( - configuration="d.yaml", - automation=_AUTOMATION | {"trigger_id": None}, - location=shown.location.to_dict(), - save=True, - expected=shown.raw_yaml, - ) + await controller.upsert( + configuration="d.yaml", + automation=_AUTOMATION | {"trigger_id": None}, + location=shown.location.to_dict(), + save=True, + expected=shown.raw_yaml, + ) - assert excinfo.value.code is ErrorCode.PRECONDITION_FAILED - assert "could not be replaced in place" in excinfo.value.message - assert devices.saved == [] + saved = "script:\n - id: script_0\n then:\n - delay: 1s\n" + assert devices.saved == [("d.yaml", saved, "Save an automation to d.yaml")] async def test_upsert_with_save_refuses_a_file_that_no_longer_loads(tmp_path: Path) -> None: diff --git a/tests/test_automations_writer.py b/tests/test_automations_writer.py index 597610a53..52f40235f 100644 --- a/tests/test_automations_writer.py +++ b/tests/test_automations_writer.py @@ -2241,6 +2241,55 @@ def test_upsert_script_with_same_id_replaces_existing_item() -> None: assert "delay: 1s" not in new_text +_IDLESS_SCRIPTS = ( + "esphome:\n name: x\nscript:\n" + " - then:\n - delay: 1s\n" + " - id: keep\n then:\n - delay: 2s\n" + " - then:\n - delay: 3s\n" +) + + +def _script_ids(text: str) -> list[str]: + rows = parse_device_yaml(text) + return [p.location.id for p in rows if isinstance(p.location, ScriptLocation)] + + +@pytest.mark.parametrize( + ("script_id", "gone"), [("script_0", "delay: 1s"), ("script_2", "delay: 3s")] +) +def test_upsert_idless_script_replaces_the_row_listed_under_its_synthetic_id( + script_id: str, gone: str +) -> None: + """A ``script_`` id lands on that id-less row and writes the id onto it.""" + new_text, _diff = render_upsert( + _IDLESS_SCRIPTS, + tree=AutomationTree( + trigger_id=None, + actions=[ActionNode(action_id="logger.log", params={"id": "wake"})], + ), + location=ScriptLocation(id=script_id), + ) + assert gone not in new_text + assert f"- id: {script_id}" in new_text + assert _script_ids(new_text) == ["script_0", "keep", "script_2"] + + +def test_upsert_idless_script_past_the_end_appends() -> None: + """A synthetic id with no row behind it is a new script.""" + new_text, _diff = render_upsert( + _IDLESS_SCRIPTS, + tree=AutomationTree(actions=[ActionNode(action_id="delay", params={"id": "4s"})]), + location=ScriptLocation(id="script_7"), + ) + assert _script_ids(new_text) == ["script_0", "keep", "script_2", "script_7"] + + +def test_delete_idless_script_removes_the_row_listed_under_its_synthetic_id() -> None: + new_text, _diff = render_delete(_IDLESS_SCRIPTS, location=ScriptLocation(id="script_2")) + assert "delay: 3s" not in new_text + assert _script_ids(new_text) == ["script_0", "keep"] + + def test_upsert_interval_at_existing_index_replaces_in_place() -> None: """An indexed interval upsert at a populated index replaces the item.""" text = "esphome:\n name: x\ninterval:\n - interval: 60s\n then:\n - delay: 1s\n" From 241cbb759c75fb551792b1360a48c7194d5b500b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Sep 2026 14:03:37 +0100 Subject: [PATCH 2/2] Let a declared script id win over the id-less row listed as it A declared id is the user's stated name; the positional label is only a fallback for a row without one. The new writer tests apply the diff the editor applies, and the guard's replace arm keeps a case: the collision above, where the write lands on the declared row and the guard fails closed. --- .../controllers/automations/writing.py | 11 ++++++--- tests/test_automations_delete_save.py | 22 +++++++++++++++++ tests/test_automations_writer.py | 24 ++++++++++++++++--- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/esphome_device_builder/controllers/automations/writing.py b/esphome_device_builder/controllers/automations/writing.py index c26c3727d..169d1758a 100644 --- a/esphome_device_builder/controllers/automations/writing.py +++ b/esphome_device_builder/controllers/automations/writing.py @@ -67,6 +67,7 @@ from .parsing import ( ComponentTarget, component_action_field_paths, + declares_id, instance_id, is_mapping_entry, make_yaml, @@ -486,13 +487,17 @@ def _upsert_top_level_list( def _top_level_item_index(yaml_text: str, domain: str, item_id: str) -> int | None: - """Index of the first ``:`` list item the parser lists as *item_id*.""" + """Index of the ``:`` item declaring *item_id*, else the id-less one listed as it.""" data = make_yaml().load(yaml_text) or {} items = data.get(domain) if isinstance(data, dict) else None if not isinstance(items, list): return None - for idx, raw in enumerate(items): - if is_mapping_entry(raw) and instance_id(domain, raw, idx, is_list=True) == item_id: + entries = [(idx, raw) for idx, raw in enumerate(items) if is_mapping_entry(raw)] + for idx, raw in entries: + if declares_id(raw) and str(raw["id"]) == item_id: + return idx + for idx, raw in entries: + if instance_id(domain, raw, idx, is_list=True) == item_id: return idx return None diff --git a/tests/test_automations_delete_save.py b/tests/test_automations_delete_save.py index 7f53cfa99..a3a205440 100644 --- a/tests/test_automations_delete_save.py +++ b/tests/test_automations_delete_save.py @@ -538,6 +538,28 @@ async def test_upsert_with_expected_replaces_an_idless_script_under_its_listed_i assert devices.saved == [("d.yaml", saved, "Save an automation to d.yaml")] +async def test_upsert_with_expected_refuses_a_replace_that_lands_on_another_row( + tmp_path: Path, +) -> None: + """A declared ``script_0`` behind an id-less row takes the write; the guard fails closed.""" + text = "script:\n - then:\n - delay: 1s\n - id: script_0\n then:\n - delay: 2s\n" + controller, devices = _setup(tmp_path, text) + shown = (await asyncio.to_thread(parsing.parse_device_yaml, text))[0] + + with pytest.raises(CommandError) as excinfo: + await controller.upsert( + configuration="d.yaml", + automation=_AUTOMATION | {"trigger_id": None}, + location=shown.location.to_dict(), + save=True, + expected=shown.raw_yaml, + ) + + assert excinfo.value.code is ErrorCode.PRECONDITION_FAILED + assert "could not be replaced in place" in excinfo.value.message + assert devices.saved == [] + + async def test_upsert_with_save_refuses_a_file_that_no_longer_loads(tmp_path: Path) -> None: controller, devices = _setup(tmp_path, "esphome: [\n") diff --git a/tests/test_automations_writer.py b/tests/test_automations_writer.py index 52f40235f..e9f252e0c 100644 --- a/tests/test_automations_writer.py +++ b/tests/test_automations_writer.py @@ -2261,7 +2261,7 @@ def test_upsert_idless_script_replaces_the_row_listed_under_its_synthetic_id( script_id: str, gone: str ) -> None: """A ``script_`` id lands on that id-less row and writes the id onto it.""" - new_text, _diff = render_upsert( + new_text, diff = render_upsert( _IDLESS_SCRIPTS, tree=AutomationTree( trigger_id=None, @@ -2269,23 +2269,41 @@ def test_upsert_idless_script_replaces_the_row_listed_under_its_synthetic_id( ), location=ScriptLocation(id=script_id), ) + assert _apply_diff(_IDLESS_SCRIPTS, diff) == new_text assert gone not in new_text assert f"- id: {script_id}" in new_text assert _script_ids(new_text) == ["script_0", "keep", "script_2"] +def test_upsert_script_prefers_the_row_declaring_the_id_over_an_idless_one() -> None: + """A declared ``script_0`` wins over the id-less row the parser also lists as ``script_0``.""" + text = "script:\n - then:\n - delay: 1s\n - id: script_0\n then:\n - delay: 2s\n" + new_text, diff = render_upsert( + text, + tree=AutomationTree(actions=[ActionNode(action_id="delay", params={"id": "9s"})]), + location=ScriptLocation(id="script_0"), + ) + assert _apply_diff(text, diff) == new_text + assert ( + new_text + == "script:\n - then:\n - delay: 1s\n - id: script_0\n then:\n - delay: 9s\n" + ) + + def test_upsert_idless_script_past_the_end_appends() -> None: """A synthetic id with no row behind it is a new script.""" - new_text, _diff = render_upsert( + new_text, diff = render_upsert( _IDLESS_SCRIPTS, tree=AutomationTree(actions=[ActionNode(action_id="delay", params={"id": "4s"})]), location=ScriptLocation(id="script_7"), ) + assert _apply_diff(_IDLESS_SCRIPTS, diff) == new_text assert _script_ids(new_text) == ["script_0", "keep", "script_2", "script_7"] def test_delete_idless_script_removes_the_row_listed_under_its_synthetic_id() -> None: - new_text, _diff = render_delete(_IDLESS_SCRIPTS, location=ScriptLocation(id="script_2")) + new_text, diff = render_delete(_IDLESS_SCRIPTS, location=ScriptLocation(id="script_2")) + assert _apply_diff(_IDLESS_SCRIPTS, diff) == new_text assert "delay: 3s" not in new_text assert _script_ids(new_text) == ["script_0", "keep"]