test: address Patch76 review findings on PR #1406

Three findings from Patch76's review of #1406:

1. **Discriminator test wasn't actually discriminating** —
   ``test_event_capture_short_circuits_rest_scan`` previously seeded
   ``get_states()`` to return ``automation.from_rest`` on every call and
   asserted ``result in {"automation.from_event", "automation.from_rest"}``.
   That accepts both outcomes, so a regression that broke the closure-cell
   short-circuit (forcing ``sample()`` to always fall through to
   ``get_states()``) would silently pass — REST would win every time. Now
   seeds ``get_states()`` with a side_effect that returns empty on the
   first call (post-subscribe sample) and the REST entity on subsequent
   calls, then asserts ``result == "automation.from_event"`` (deterministic
   discriminator: cell wins → event entity, regression → REST entity) and
   ``call_count == 1`` (the cell short-circuit means the post-event
   re-sample never touches REST).

2. **Duplicate-unique-id collision branch untested** — Patch76 noted the
   first-wins guard in ``event_filter`` (``util_helpers.py:1197``) had no
   test pinning the contract. Added
   ``test_duplicate_unique_id_collision_first_wins`` which fires two
   matching state_changed events back-to-back and asserts both
   "automation.first wins" and "warning logged naming both entity_ids."

3. **``last_api_error`` wedged-channel branch untested** — Patch76 noted
   the conditional warning at ``util_helpers.py:1222`` had no test pinning
   the "REST channel down" vs "automation never published" discrimination
   the code exists to make. Added
   ``test_wedged_rest_channel_surfaces_last_api_error`` which seeds
   ``get_states()`` to raise ``HomeAssistantAPIError`` on every call and
   asserts the timeout warning includes "every REST sample failing" and
   the last error message string.

Rebased onto current upstream/master. Mechanical conflict resolution per
Patch76: the ``_POLL_CADENCE`` deletion wins, taking #1389's measurement
instrumentation with it. ``_poll_for_automation_entity`` no longer needs
the cadence loop or the elapsed-time instrumentation because the WS
event-driven path resolves sub-second on the happy path and the REST
fallback uses uniform polling. The redundant local ``import time`` inside
``upsert_automation_config`` is also gone now that the top-level
``import time`` (added by #1389) remains for the unique_id-generation use
at ``time.time() * 1000``.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: kingpanther13

src/ha_mcp/client/rest_client.py

@@ -895,8 +895,6 @@ async def upsert_automation_config(
         Raises:
             HomeAssistantAPIError: If configuration invalid or API error
         """
-        import time
-
         # Generate unique_id for new automation if not provided
         if identifier is None:
             unique_id = str(int(time.time() * 1000))

tests/src/unit/test_wait_helpers.py

@@ -979,23 +979,49 @@ async def fire_noise():
     async def test_event_capture_short_circuits_rest_scan(self, ws_client, mock_client):
         """When the matching event arrives, the discovered entity_id is
         captured in the closure cell so the post-nudge ``sample()`` returns
-        the captured value WITHOUT re-scanning ``get_states()``. Pin this
-        explicitly by seeding ``get_states()`` with a *different* matching
-        automation: cell-wins → result is the event's entity_id, scan-wins
-        → result is the REST entity_id. Different outcomes give the test
-        a discriminator the empty-list version couldn't provide."""
-        # REST scan would resolve to ``automation.from_rest``; the event
-        # carries ``automation.from_event``. Only one can win.
-        mock_client.get_states = AsyncMock(
-            return_value=[
+        the captured value WITHOUT re-scanning ``get_states()``.
+
+        Deterministic discriminator (Patch76 #1406 review): seeding
+        ``get_states()`` to return a *different* matching automation on
+        every call — but ONLY after the post-subscribe sample has run with
+        empty results — produces different outcomes for the two paths:
+
+        - Working (cell short-circuits): post-subscribe sample returns
+          empty → loop enters → event fires + filter captures
+          ``automation.from_event`` → re-sample sees ``captured["entity_id"]``
+          is set → returns ``automation.from_event`` without calling
+          ``get_states()`` again.
+        - Regression (cell check removed from ``sample()``): re-sample
+          falls through to ``get_states()`` → finds ``automation.from_rest``
+          → returns ``automation.from_rest``.
+
+        Asserting equality (not ``in``) is what makes this a real
+        discriminator — the previous ``in {...}`` assertion accepted both
+        outcomes and would silently pass a short-circuit regression.
+        """
+        call_count = {"n": 0}
+
+        async def get_states_seq():
+            call_count["n"] += 1
+            if call_count["n"] == 1:
+                # Post-subscribe sample: empty so the loop must enter.
+                return []
+            # Any subsequent call (which the working short-circuit makes
+            # us skip): returns the REST entity that would override the
+            # captured cell if the short-circuit were broken.
+            return [
                 {
                     "entity_id": "automation.from_rest",
                     "attributes": {"id": "uid_dup"},
                 }
             ]
-        )
+
+        mock_client.get_states = AsyncMock(side_effect=get_states_seq)
 
         async def fire_after_subscribe():
+            # Small delay so the post-subscribe sample completes and the
+            # wait loop is parked on ``nudge.wait()`` before the event
+            # arrives — guarantees the event-path wins.
             await asyncio.sleep(0.05)
             await ws_client.fire_state_changed(
                 "automation.from_event",
@@ -1008,16 +1034,13 @@ async def fire_after_subscribe():
         )
         await fire_task
 
-        # In practice the post-subscribe REST sample races against the
-        # event arrival, so the result may be either value depending on
-        # timing — but ``automation.from_event`` proves the capture
-        # short-circuit took the value from the event payload (the cell
-        # short-circuit at the top of ``sample()`` returns early before
-        # re-calling ``get_states()``).
-        assert result in {"automation.from_event", "automation.from_rest"}
-        # Whichever path won, the captured cell or the REST sample must
-        # have produced a real entity_id (i.e. the wait did not time out).
-        assert result is not None
+        # Working short-circuit: event payload's entity_id wins.
+        # Regression (no short-circuit): REST entity would override.
+        assert result == "automation.from_event"
+        # Tighter pin: ``get_states()`` was called exactly once (the
+        # post-subscribe sample). The post-event re-sample short-circuits
+        # via the captured cell and never touches REST.
+        assert call_count["n"] == 1
 
     async def test_connection_drop_during_discovery_falls_back_to_rest(
         self, ws_client, mock_client
@@ -1061,3 +1084,78 @@ async def get_states_dropping_ws():
         assert call_count["n"] >= 2
         # Cleanup of the subscription we did establish still ran.
         assert len(ws_client.unsubscribed) == 1
+
+    async def test_duplicate_unique_id_collision_first_wins(
+        self, ws_client, mock_client, caplog
+    ):
+        """If two matching events arrive for the same unique_id (HA
+        storage forbids it, but the filter's first-wins guard exists for
+        the "if it ever happens" path), the FIRST observed entity_id is
+        captured and a warning is logged on the collision. Pins the
+        first-wins contract against a regression that flips back to
+        last-writer-wins. Patch76 #1406 review."""
+        import logging
+
+        # Post-subscribe sample finds nothing; both events arrive after
+        # subscribe.
+        mock_client.get_states = AsyncMock(return_value=[])
+
+        async def fire_two_matches():
+            await asyncio.sleep(0.05)
+            # Both events carry the same unique_id but different
+            # entity_ids — only the first should win.
+            await ws_client.fire_state_changed(
+                "automation.first",
+                new_state={"attributes": {"id": "uid_dup"}},
+            )
+            await ws_client.fire_state_changed(
+                "automation.second",
+                new_state={"attributes": {"id": "uid_dup"}},
+            )
+
+        with caplog.at_level(logging.WARNING, logger="ha_mcp.tools.util_helpers"):
+            fire_task = asyncio.create_task(fire_two_matches())
+            result = await wait_for_automation_entity_by_unique_id(
+                mock_client, "uid_dup", timeout=2.0
+            )
+            await fire_task
+
+        assert result == "automation.first"
+        # Collision warning emitted naming both entity_ids.
+        collision_logs = [
+            r for r in caplog.records if "Duplicate automation match" in r.getMessage()
+        ]
+        assert len(collision_logs) == 1
+        assert "automation.first" in collision_logs[0].getMessage()
+        assert "automation.second" in collision_logs[0].getMessage()
+
+    async def test_wedged_rest_channel_surfaces_last_api_error(
+        self, mock_client, caplog
+    ):
+        """When every REST sample raises ``HomeAssistantAPIError`` across
+        the timeout budget, the final timeout warning surfaces the last
+        error so operators can distinguish "automation truly not
+        published" from "REST channel down." Pins the
+        ``captured["last_api_error"]`` discrimination branch. Patch76
+        #1406 review. Uses the autouse ``force_rest_fallback`` fixture so
+        no WS subscription is created — the REST sample callback is the
+        only path."""
+        import logging
+
+        mock_client.get_states = AsyncMock(
+            side_effect=HomeAssistantAPIError("HA REST 503 transient")
+        )
+
+        with caplog.at_level(logging.WARNING, logger="ha_mcp.tools.util_helpers"):
+            result = await wait_for_automation_entity_by_unique_id(
+                mock_client, "uid_wedged", timeout=0.2, poll_interval=0.05
+            )
+
+        assert result is None
+        # The conditional wedged-channel warning fires with the last
+        # error string embedded.
+        wedged_logs = [
+            r for r in caplog.records if "every REST sample failing" in r.getMessage()
+        ]
+        assert len(wedged_logs) == 1
+        assert "HA REST 503 transient" in wedged_logs[0].getMessage()