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

[bdraco]

What does this implement/fix?

The parser lists an id-less top-level script: item under a synthetic script_<index> location, but the writer only matched a literal id. A replace aimed at that row appended a second script on the editor's draft path, and a delete answered not_found; the guarded path from #2788 caught the append only after the fact.

Both writers now find the row through the parser's own identity rule (instance_id), so the replace and the delete land on the row the listing showed. A replace writes the listed id onto the item; esphome requires a script id, so the row had never loaded before.

Related issue or feature (if applicable):

• fixes Replacing an id-less script by its synthetic location appends instead of replacing on the draft path #2799

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 script-synthetic-id-replace (241cbb7) with main (b15a0fd)}

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 (b15a0fd) to head (241cbb7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2821      +/-   ##
==========================================
- Coverage   99.75%   99.75%   -0.01%     
==========================================
  Files         294      294              
  Lines       25364    25363       -1     
==========================================
- Hits        25301    25300       -1     
  Misses         63       63              

Flag	Coverage Δ
py3.12	99.71% <100.00%> (-0.01%)	⬇️
py3.14	99.59% <100.00%> (-0.01%)	⬇️

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

Files with missing lines	Coverage Δ
..._device_builder/controllers/automations/parsing.py	100.00% <100.00%> (ø)
..._device_builder/controllers/automations/writing.py	100.00% <100.00%> (ø)
esphome_device_builder/models/automations.py	100.00% <ø> (ø)

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

Previous review — superseded by a newer review below.

[esphbot]

Tip

No blocking issues found — ready to merge.

[bdraco]

241cbb7 takes all three:

• The lookup now runs two passes, declared ids first, then the positional label for id-less rows; a declared script_0 behind an id-less row takes the write again. Test added at the writer level.
• The new writer tests apply the diff the way the editor does and compare it against the returned text.
• The guard's replace arm keeps a case: the collision above, where the write lands on the declared row while the listing's first script_0 is the id-less one, so the guard answers precondition_failed with nothing written.

@esphbot review

[esphbot]

@bdraco: 241cbb7 takes all three:

Confirmed all three at 241cbb7.

Two-pass lookup correct. declares_id(raw) matches instance_id's falsy-id rule, so no divergence on id: 0. Both passes share the is_mapping_entry filter with original indices — same basis as _parse_automation_list.

Writer tests now apply diffs via apply_yaml_diff_like_frontend. Guard test asserts the message and empty devices.saved.

Two nits left, both non-blocking: _render_delete_if_unchanged (controller.py:565) has no post-write check, so the same collision deletes the declared row after validating the id-less one; and docs/API.md's automations/delete row still omits the new addressing rule.

[esphbot]

PR Review — Resolve an id-less script by the id its listing shows

All three findings from the prior review are genuinely resolved in 241cbb7 — merge-ready, with two non-blocking notes.

Specific strengths of the follow-up commit:

• _top_level_item_index (writing.py:489-502) now runs declared-id first, then the positional label, so a declared script_0 sitting behind an id-less row takes the write again. The declared pass uses declares_id(raw) rather than a bare raw.get("id"), so it agrees exactly with instance_id's falsy-id rule (id: 0 / id: "" stay id-less) instead of re-implementing it.

• Both passes iterate the same is_mapping_entry-filtered entries list while keeping the original enumerate index, which is precisely what _parse_automation_list (parsing.py:198-200) does — !included or scalar entries cannot shift the synthetic numbering in one place and not the other.

• Every new writer test now asserts _apply_diff(text, diff) == new_text, and _apply_diff routes through apply_yaml_diff_like_frontend — the editor-draft path the reported bug actually lives on is now covered, not just new_text.

• The rewritten guard test (test_upsert_with_expected_refuses_a_replace_that_lands_on_another_row) restores coverage of the expected is not None arm and asserts the exact message, so the ternary arm is no longer silently untested; it also pins that nothing is written.

• test_upsert_script_prefers_the_row_declaring_the_id_over_an_idless_one asserts full output text rather than substrings, which is the right strength for a two-row collision.

• _render_delete_if_unchanged still has no post-write check, so on the same colliding-id config the guard validates the id-less row while the writer removes the declared one — pre-existing, invalid-ESPHome-only, but now the asymmetric half of a behaviour the upsert side tests explicitly.

• docs/API.md's automations/delete row was left unchanged even though the delete path stopped answering not_found for an id-less script.

Note: I could not execute the test suite (the review shell is read-only, no interpreter), so the pass/fail claim is not independently verified here — the reasoning above is from reading the code and tests.

---

✅ Resolved since last review (2)

Previously-flagged issues verified fixed

• tests/test_automations_writer.py:2264 New writer tests discard the YamlDiff they are meant to prove
• esphome_device_builder/controllers/automations/writing.py:488 First-match lookup can resolve a declared script_N id to an earlier id-less row

---

---

Checklist

• Writer identity matches the parser's listing rule (same filter, same index basis)
• No silently-wrong row resolution for colliding ids on the upsert path
• Tests verify observable behaviour including the frontend-applied diff
• Existing Let automations/upsert save under the lock and prove what it replaces #2788 guard coverage preserved
• No injection / unsafe deserialization introduced (YAML loads via make_yaml)
• Error paths return the right codes; message change carries no test dependency
• Docs match behaviour for every command the change touches
• No scope creep; diff matches the PR description
• File-size cap respected (writing.py 854 lines, grandfathered, not made worse)

ℹ️ Triage summary

1 pre-existing finding(s) on unchanged code suppressed (freeze).

---

Automated review by Kōan (Claude) HEAD=241cbb7 4 min 2s

[esphbot]

Tip

No blocking issues found — ready to merge.

[Copilot]

Copilot review overview

🟡 Changes recommended

Guarded deletion can validate one script row but delete another when an id-less row precedes a declared script_0.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Guarded delete validates one script row but deletes another · New

What changed in this PR

Fixes automation writes so id-less top-level scripts can be replaced or deleted using their parser-generated script_<index> identity.

Changes:

• Aligns script lookup with parser identity resolution.
• Assigns IDs when replacing id-less scripts.
• Adds regression tests and API/model documentation.

File	Summary
tests/​test_automations_writer.py	Tests synthetic-ID replacement and deletion.
tests/​test_automations_delete_save.py	Tests guarded saves for id-less scripts.
esphome_device_builder/​models/​automations.py	Documents synthetic script IDs.
esphome_device_builder/​controllers/​automations/​writing.py	Resolves scripts by declared or synthetic identity.
esphome_device_builder/​controllers/​automations/​parsing.py	Shares the parser identity rule.
docs/​API.md	Documents id-less script replacement behavior.

---

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

[bdraco]

This looks right from the code: the check takes the first row at the location and the writer prefers the item that declares the id. It came in after the merge, so it is tracked in #2851 with the YAML that would show it. Not reproduced yet.