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.

That's correct for the two existing cases:

• "H M" format (GS/GS_fb00) never expects these fields to be user-configured at all — the real writes go via separate hour/minute entities (confirmed against templates/ginlong_solis.yaml, which never sets discharge_start_time).
• "S" format with no time window (SF/SE/etc) ships a bare placeholder string, not a real entity (confirmed against templates/sofar.yaml, which sets it to the literal "00:00:00"). has_time_window turns out to be read nowhere else in the codebase, so this placeholder is genuinely inert either way — it only ever talks to itself.

The reporter's custom inverter definition (GROWATTSPH) combines a non-HH:MM:SS format ("S") with a real time window (has_time_window: true, has_discharge_enable_time: true) and a real, directly user-configured time.growatt_battery_grid_first_time_period_9_start entity — a combination the existing check never anticipated. For this combination the write path already does a plain write straight to whatever discharge_start_time resolves to; the dummy creation is the only thing standing in the way of it actually working.

Confirmed directly against the reporter's log and predbat_debug.yaml:

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

Fix

is_real_entity_configured() distinguishes "the user pointed this at a real HA entity" (has a domain, e.g. time.foo) from "this is unset or a bare placeholder" (no dot, e.g. "23:59:00") — preserving both existing legitimate cases exactly while respecting a genuinely configured one.

Testing

New regression test covering all three cases:

• real entity configured → kept, not overwritten (fails without the fix, reproducing the exact sensor.predbat_<type>_<id>_discharge_start_time naming pattern from the report)
• nothing configured → dummy still created (GS_fb00-style, unaffected)
• bare placeholder string, no domain → still replaced by a dummy (SF-style, unaffected)

Full --quick suite green.

🤖 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…