Resolve an id-less script by the id its listing shows

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 `<domain>.<platform>` 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, `<domain>_<idx>` positional, `<parent_instance_id>_<sub_key>` 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 `<parent_instance_id>_<sub_key>` 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_<index>` 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

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,

esphome_device_builder/controllers/automations/writing.py

@@ -67,6 +67,8 @@
 from .parsing import (
     ComponentTarget,
     component_action_field_paths,
+    declares_id,
+    instance_id,
     is_mapping_entry,
     make_yaml,
     resolve_action_field_target,
@@ -156,7 +158,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 +478,30 @@ 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 ``<domain>:`` 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
+    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
+
+
 @in_list_form
 def _upsert_top_level_list_indexed(
     yaml_text: str,
@@ -596,7 +605,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 +624,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 ``<domain>:`` 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 ``<domain>:`` 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

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_<index>`` when id-less."""
 
     id: str
     kind: Literal["script"] = "script"

tests/test_automations_delete_save.py

@@ -519,13 +519,33 @@ 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]
 
+    await controller.upsert(
+        configuration="d.yaml",
+        automation=_AUTOMATION | {"trigger_id": None},
+        location=shown.location.to_dict(),
+        save=True,
+        expected=shown.raw_yaml,
+    )
+
+    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_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",

tests/test_automations_writer.py

@@ -2241,6 +2241,73 @@ 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_<index>`` 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 _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(
+        _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"))
+    assert _apply_diff(_IDLESS_SCRIPTS, diff) == new_text
+    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"