fix(inverter): don't overwrite a genuinely configured time entity with a dummy sensor

[chalfontchubby]

Fixes #4738

Problem

For any charge_time_format other than "HH:MM:SS", Predbat unconditionally creates and assigns a self-owned sensor.predbat_<type>_<id>_<field> for charge_start_time / charge_end_time / discharge_start_time / discharge_end_time, discarding whatever the user configured - real entity or not.

The reporter's custom inverter definition (GROWATTSPH) combines a non-HH:MM:SS format ("S") with a real time window and a directly configured time.growatt_battery_grid_first_time_period_9_start entity. Confirmed against their log and predbat_debug.yaml:

• args_from_apps_yaml.discharge_start_time = their real time.* entity
• args.discharge_start_time (what is actually used) = sensor.predbat_GROWATTSPH_0_discharge_start_time, created at startup
• Their "successfully wrote" log line is a real, successful write - to Predbat's own placeholder sensor, so the Growatt integration never sees it.

Fix

The dummy is created only when apps.yaml did not give this inverter a real entity for the slot, as suggested on review:

• "Set in apps.yaml" is judged from args_from_apps_yaml, the snapshot taken before Predbat's own defaulting, the same way set_arg_auto() tells user config from defaults. apps.yaml must name an entity (or a re: pattern) for this inverter: a list is read per index, and a single value counts for inverter 0.
• A placeholder such as the Sofar templates' "00:00:00" does not count, so it still gets the dummy - and that dummy, once it is in the live slot, never reads back as configuration. Since fix(inverter): Persist inverters across planning cycles #5126 inverters persist and this runs in refresh_config() every plan cycle, where a lost dummy is re-created and an inverter type change is followed.
• The entity itself is taken from the live args, where any re: pattern is already resolved, and is never Predbat's own dummy for the slot. A pattern that matched nothing leaves the slot empty for the dummy to fill, so that matters from the second cycle on. A malformed value also gets the dummy.
• A single value is put back after create_missing_arg() turns it into a list of defaults.

Scope: only non-HH:MM:SS types reach this. Every component that sets these time entities (fox/FoxCloud, gecloud/GEC/GEE, alphaess, deye, enphase, gateway) also sets an HH:MM:SS type, so component-discovered entities are unaffected. No shipped template names an entity here for a built-in type, so shipped configs behave as before.

One behaviour to be aware of: on an "H M" type (GS, GS_fb00) where a user has hand-configured a real entity for these slots, that entity now receives the HH:MM:SS write each cycle alongside the hour/minute writes, where before the write went to the dummy. No shipped template does this.

Testing

test_custom_type_respects_configured_time_entity, on a custom "S" type:

• list value and single value from apps.yaml → kept, on construction and on refresh
• nothing set → dummy
• bare placeholder → dummy, and after HA loses the dummy, refresh re-creates it
• persisted object: refresh keeps its own dummy and re-creates a lost state
• inverter type change arriving through refresh_config() → the new type's dummy
• an entity in args that apps.yaml never set → dummy
• two inverters, apps.yaml sets inverter 0 only (list, and single value) → inverter 1 gets and keeps its own dummy, re-created after HA loses it
• malformed (nested list) value → dummy, no crash
• a re: pattern that matched nothing (single and list) → dummy, re-created after HA loses it and following a type change; a matched pattern → the resolved entity, kept on refresh

It restores args in place and the HA dummy state afterwards. Mutation-checked: reverting to main, ignoring the snapshot, dropping the per-inverter read, reading after create_missing_arg(), accepting any non-None apps.yaml value, and dropping the own-dummy guard each fail it. ./run_all --quick and pre-commit run --all-files pass (exit 0).

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Address the dummy-entity rebuild risk and test fixture state leakage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes #4738 by preserving genuinely configured Home Assistant time entities for nonstandard inverter time formats.

Changes:

• Adds entity-preservation logic for configured, missing, and placeholder values.
• Adds regression tests covering all three cases.

File summaries

File	Summary	Review findings
apps/predbat/inverter.py	Preserves configured time entities.	Moderate: Exclude Predbat-created dummy IDs during rebuilds. Nit: Reuse utils.is_entity_id().
apps/predbat/tests/test_inverter.py	Adds regression coverage.	Moderate: Snapshot and restore dummy_items to prevent test-state leakage.

Review details

Suppressed comments (1)

apps/predbat/tests/test_inverter.py:60

• Inverter construction calls create_entity, which writes the generated dummy sensors into my_predbat.ha_interface.dummy_items. The finally block restores args and INVERTER_DEF but not those HA states, so this test leaves several TEST_CUSTOM_TIME_ENTITY sensors in the fixture reused by the remaining inverter tests; snapshot and restore dummy_items as well (the nearby reserve test does this for its HA state).

    saved_args = copy.deepcopy(my_predbat.args)

• Files reviewed: 2/2 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.

[springfall2008]

Automated comment from the triage bot — nothing merged or pushed; this is a patch for you to apply if you agree with it.

What this covers

Automated cleanup run on #4745. Two things were outstanding: the pre-commit CI failure and the two Copilot review comments.

• CI failure — already fixed by you in ddabc2fc while this run was in progress. It was a SyntaxError, not a style problem: the 604e43a4 autofix left return is_entity_id(...) at column 0. Nothing further needed.
• Copilot on is_entity_id reuse (inverter.py:317) — correct and already applied. utils.is_entity_id() is isinstance(value, str) and "." in value, exactly the predicate it replaced.
• Copilot on Predbat's own dummy ids being read back as user config (inverter.py:654) — confirmed as a real bug against current main, with a reproduction. Details and the reasoning are in the thread reply; the patch is below.

origin/main was merged into the branch cleanly as part of this run (no conflicts).

Why I could not push

ddabc2fc landed on the branch while this run was working, so git push was rejected as non-fast-forward. Integrating it requires git merge/git pull of the branch's own remote head, and this session's permission allowlist only permits merging origin/main. Rather than route around that — or force-push, which is prohibited — the patch is here for you to apply. It applies on top of ddabc2fc: the only overlap is is_real_entity_configured(), and this version already incorporates both your indentation fix and the is_entity_id() reuse.

Verification

• coverage/run_pre_commit — all hooks pass, including the ruff and black that CI failed on.
• inverter test module — passes.
• Mutation check: with is_own_dummy_entity() stubbed to always return False, the two new cases fail, so they are genuinely exercising the fix rather than passing vacuously.

Patch

--- a/apps/predbat/inverter.py
+++ b/apps/predbat/inverter.py
@@ -301,20 +301,42 @@ class Inverter:
             # then looked up as one.
             self.base.args[arg] = self.base.args[arg] + [None] * (self.id + 1 - len(self.base.args[arg]))
 
+    def is_own_dummy_entity(self, value, entity_name):
+        """
+        Whether value is one of Predbat's own dummy entity ids for this inverter and field, i.e.
+        something an earlier construction of this inverter left behind in args via create_entity()
+        rather than anything a user or a component's automatic_config() deliberately pointed at.
+
+        create_entity() builds 'sensor.{prefix}_{inverter_type}_{id}_{entity_name}' and writes it
+        back into self.base.args, which lives for the whole process and is never re-read from
+        apps.yaml. Inverters are rebuilt from scratch on the balance path (execute.py) as well as at
+        startup, so from the second construction onwards args already holds that dummy - which has a
+        domain and would otherwise read back as a genuinely configured entity, skipping the
+        recreation that re-registers created_attributes and restores the state if it has gone away.
+        The inverter_type sits in the middle and is matched loosely, so a dummy left over from a
+        previous type is still recognised as ours rather than stranding writes on the stale sensor.
+        """
+        if not isinstance(value, str):
+            return False
+        return value.startswith("sensor.{}_".format(self.base.prefix)) and value.endswith("_{}_{}".format(self.id, entity_name))
+
     def is_real_entity_configured(self, arg):
         """
         True if arg already resolves to a real HA entity id for this inverter (contains a domain,
-        e.g. 'time.foo'), as opposed to being unset or a bare placeholder value such as '23:59:00'
-        or '00:00:00' with no domain. Used to tell "the user configured this themselves" apart from
-        "nothing is there yet" before create_missing_arg's own value-vs-list check would conflate the
-        two - a config item that's present but not a real entity is exactly the case a dummy entity
-        still needs to be created for.
+        e.g. 'time.foo'), as opposed to being unset, a bare placeholder value such as '23:59:00' or
+        '00:00:00' with no domain, or a dummy this inverter created for itself on an earlier
+        construction. Used to tell "this was configured deliberately" apart from "nothing real is
+        there yet" before create_missing_arg's own value-vs-list check would conflate the two - a
+        config item that's present but not a real entity is exactly the case a dummy entity still
+        needs to be created for.
         """
         values = self.base.args.get(arg)
         if not isinstance(values, list) or self.id >= len(values):
             return False
         value = values[self.id]
-        return is_entity_id(values[self.id])
+        if self.is_own_dummy_entity(value, arg):
+            return False
+        return is_entity_id(value)
 
     def __init__(self, base, id=0, quiet=False):
         """

--- a/apps/predbat/tests/test_inverter.py
+++ b/apps/predbat/tests/test_inverter.py
@@ -53,6 +53,10 @@ def test_custom_type_respects_configured_time_entity(my_predbat):
         writes go via separate hour/minute entities - so the dummy must still be created.
       - SF style (has_time_window False): ships a bare placeholder string, not a real entity - the
         dummy must still replace it, since there is nothing real to preserve.
+
+    And a dummy Predbat wrote into args itself must not read back as user configuration on a later
+    construction, since args is process-lifetime state and inverters are rebuilt from scratch on the
+    balance path: the dummy has a domain, so a plain "has a dot" test would skip recreating it.
     """
     failed = False
     print("Test: test_custom_type_respects_configured_time_entity")
@@ -110,6 +114,42 @@ def test_custom_type_respects_configured_time_entity(my_predbat):
         if not (isinstance(got, str) and got.startswith("sensor.")):
             print(f"ERROR: test_custom_type_respects_configured_time_entity: SF-style bare placeholder should still be replaced by a dummy sensor, got {got}")
             failed = True
+
+        # Case 4: rebuild. self.base.args lives for the whole process and is never re-read from
+        # apps.yaml, and inverters are rebuilt from scratch on the balance path (execute.py), so by
+        # the second construction args already holds the dummy written by the first. That dummy has
+        # a domain, so a plain "has a dot" test reads it back as user-configured and skips creation
+        # - leaving created_attributes unpopulated on the new object and the state unrestored if it
+        # has gone away.
+        my_predbat.args["inverter_type"] = ["TEST_CUSTOM_TIME_ENTITY_REBUILD"]
+        my_predbat.args["inverter"] = {
+            "charge_time_format": "S",
+            "has_time_window": True,
+            "has_charge_enable_time": True,
+            "has_discharge_enable_time": True,
+        }
+        my_predbat.args.pop("discharge_start_time", None)
+
+        Inverter(my_predbat, 0, quiet=True)
+        dummy_id = my_predbat.args["discharge_start_time"][0]
+        rebuilt = Inverter(my_predbat, 0, quiet=True)
+
+        if my_predbat.args["discharge_start_time"][0] != dummy_id:
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: rebuild should keep the same dummy id {dummy_id}, got {my_predbat.args['discharge_start_time'][0]}")
+            failed = True
+        if dummy_id not in rebuilt.created_attributes:
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: rebuild should re-register {dummy_id} in created_attributes, got {sorted(rebuilt.created_attributes)}")
+            failed = True
+
+        # Case 5: the inverter type changes between constructions (discovery or an apps.yaml edit).
+        # The dummy id embeds the type, so the one left in args names the old type - it must be
+        # recognised as Predbat's own and replaced, not preserved as though the user had chosen it.
+        my_predbat.args["inverter_type"] = ["TEST_CUSTOM_TIME_ENTITY_REBUILD2"]
+        Inverter(my_predbat, 0, quiet=True)
+        got = my_predbat.args["discharge_start_time"][0]
+        if got != "sensor.{}_TEST_CUSTOM_TIME_ENTITY_REBUILD2_0_discharge_start_time".format(my_predbat.prefix):
+            print(f"ERROR: test_custom_type_respects_configured_time_entity: a stale dummy from a previous inverter type should be replaced, got {got}")
+            failed = True
     finally:
         my_predbat.args = saved_args
         if saved_def is None:
@@ -117,6 +157,8 @@ def test_custom_type_respects_configured_time_entity(my_predbat):
         else:
             INVERTER_DEF["TEST_CUSTOM_TIME_ENTITY"] = saved_def
         INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY_SF", None)
+        INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY_REBUILD", None)
+        INVERTER_DEF.pop("TEST_CUSTOM_TIME_ENTITY_REBUILD2", None)
 
     return failed

[Copilot]

Copilot review overview

🔵 Needs a closer look

Test fixture state can leak between subtests unless the registry and shared arguments are restored correctly.

Review effort: Lite
Findings: None

Resolved since last review (2)

• create_entity() produces Predbat-owned IDs such as sensor.<prefix>_<type>_<id>_<field>, so a…
• This predicate duplicates utils.is_entity_id() exactly, and inverter.py already imports that…

[springfall2008]

Approved. The check now reads args_from_apps_yaml, as asked for in the earlier review, and the registry is gone. Rebased onto current main; the inverter test module passed on a local merge with main before the rebase. Entities set by components and the shipped templates behave as before.

(Review written by Claude on behalf of @springfall2008.)