fix: prevent overlapping overview pages (#2009)

* fix: make overview pagination stable

Use one round-robin entity ordering for every page so the requested limit is honored and next_offset cannot repeat prior entities.

Add a regression covering the reported two-page overlap.

Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>

* test: cover multi-domain overview pagination

Walk every page of an equal multi-domain result and assert the round-robin pages are complete and disjoint. Document the allocator mutation contract and stream the ordering so pagination only retains the requested page.

Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>

---------

Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>

Author: Sanjays2402

src/ha_mcp/tools/smart_search/_overview.py

@@ -3,6 +3,7 @@
 import asyncio
 import logging
 import random
+from itertools import islice
 from typing import Any
 
 from ...visibility.resolver import load_hidden_set
@@ -433,65 +434,41 @@ def _format_domain_stats(
         return formatted_domain_stats
 
     @staticmethod
-    def _allocate_page_one(
+    def _allocate_page(
         formatted_domain_stats: dict[str, dict[str, Any]],
         effective_limit: int,
-        total_entity_count: int,
+        offset: int,
     ) -> int:
-        """Distribute the page-1 budget: a min allocation per domain, rest proportional.
+        """Take one stable round-robin page across domains.
+
+        A single ordering must drive every page. Otherwise a specially balanced
+        first page followed by sequential offset pages can repeat entities.
 
-        Gives each domain a minimum slice so the LLM sees entities from every
-        domain, then distributes the remaining budget proportionally. Mutates
-        ``formatted_domain_stats`` in place; returns the count included.
+        Mutates ``formatted_domain_stats`` in place; returns the count included.
         """
-        min_per_domain = 3
-        num_domains = len(formatted_domain_stats)
-        reserved = min(min_per_domain * num_domains, effective_limit)
-        remaining_budget = effective_limit - reserved
-
-        entities_included = 0
-        for domain_data in formatted_domain_stats.values():
-            domain_entities = domain_data["entities"]
-            domain_len = len(domain_entities)
-            base = min(min_per_domain, domain_len)
-            if total_entity_count > 0 and remaining_budget > 0:
-                extra = int(remaining_budget * domain_len / total_entity_count)
-            else:
-                extra = 0
-            take = min(base + extra, domain_len)
-            if take < domain_len:
-                domain_data["entities"] = domain_entities[:take]
-                domain_data["truncated"] = True
-            entities_included += len(domain_data["entities"])
-        return entities_included
+        max_domain_size = max(
+            (len(data["entities"]) for data in formatted_domain_stats.values()),
+            default=0,
+        )
+        ordered = (
+            (domain, data["entities"][index])
+            for index in range(max_domain_size)
+            for domain, data in formatted_domain_stats.items()
+            if index < len(data["entities"])
+        )
 
-    @staticmethod
-    def _allocate_subsequent_pages(
-        formatted_domain_stats: dict[str, dict[str, Any]],
-        effective_limit: int,
-        offset: int,
-    ) -> int:
-        """Apply pages-2+ sequential skip/take across domains. Mutates in place."""
-        entities_skipped = 0
-        entities_included = 0
-        for domain_data in formatted_domain_stats.values():
-            domain_entities = domain_data["entities"]
-            domain_len = len(domain_entities)
-
-            skip_from_domain = max(0, min(domain_len, offset - entities_skipped))
-            budget_left = effective_limit - entities_included
-            take_from_domain = max(0, min(domain_len - skip_from_domain, budget_left))
-
-            if skip_from_domain > 0 or take_from_domain < domain_len:
-                domain_data["entities"] = domain_entities[
-                    skip_from_domain : skip_from_domain + take_from_domain
-                ]
-                if take_from_domain < domain_len:
-                    domain_data["truncated"] = True
+        selected: dict[str, list[dict[str, Any]]] = {
+            domain: [] for domain in formatted_domain_stats
+        }
+        for domain, entity in islice(ordered, offset, offset + effective_limit):
+            selected[domain].append(entity)
 
-            entities_skipped += skip_from_domain
-            entities_included += take_from_domain
-        return entities_included
+        for domain, data in formatted_domain_stats.items():
+            original_count = len(data["entities"])
+            data["entities"] = selected[domain]
+            if len(data["entities"]) < original_count:
+                data["truncated"] = True
+        return sum(len(entities) for entities in selected.values())
 
     def _paginate_overview_entities(
         self,
@@ -514,14 +491,9 @@ def _paginate_overview_entities(
         total_entity_count = sum(
             len(ds["entities"]) for ds in formatted_domain_stats.values()
         )
-        if offset == 0:
-            entities_included = self._allocate_page_one(
-                formatted_domain_stats, effective_limit, total_entity_count
-            )
-        else:
-            entities_included = self._allocate_subsequent_pages(
-                formatted_domain_stats, effective_limit, offset
-            )
+        entities_included = self._allocate_page(
+            formatted_domain_stats, effective_limit, offset
+        )
 
         has_more = (offset + entities_included) < total_entity_count
         return {

tests/src/unit/test_performance_parallelization.py

@@ -539,6 +539,40 @@ async def test_pagination_offset_skips_entities(self):
         assert result["pagination"]["has_more"] is False
         assert result["pagination"]["next_offset"] is None
 
+    @pytest.mark.asyncio
+    async def test_pagination_pages_respect_limit_without_overlap(self):
+        """Following next_offset returns disjoint pages of the requested size."""
+        entities = [
+            {
+                "entity_id": f"light.l{i}",
+                "attributes": {"friendly_name": f"Light {i}"},
+                "state": "on",
+            }
+            for i in range(4)
+        ]
+        tools = _make_tools(MockClient(entities=entities))
+
+        first = await tools.get_system_overview(
+            detail_level="standard", limit=2, offset=0
+        )
+        second = await tools.get_system_overview(
+            detail_level="standard",
+            limit=2,
+            offset=first["pagination"]["next_offset"],
+        )
+
+        first_names = {
+            entity["friendly_name"]
+            for entity in first["domain_stats"]["light"]["entities"]
+        }
+        second_names = {
+            entity["friendly_name"]
+            for entity in second["domain_stats"]["light"]["entities"]
+        }
+        assert first["pagination"]["entities_returned"] == 2
+        assert second["pagination"]["entities_returned"] == 2
+        assert first_names.isdisjoint(second_names)
+
     @pytest.mark.asyncio
     async def test_pagination_not_applied_to_minimal(self):
         """Minimal mode does not apply global pagination (already capped per-domain)."""
@@ -561,7 +595,7 @@ async def test_pagination_not_applied_to_minimal(self):
 
     @pytest.mark.asyncio
     async def test_pagination_across_multiple_domains(self):
-        """Pagination distributes budget fairly across domains on page 1."""
+        """Pagination interleaves entities across domains in round-robin order."""
         entities = (
             [
                 {
@@ -597,7 +631,7 @@ async def test_pagination_across_multiple_domains(self):
         assert result["domain_stats"]["sensor"]["count"] == 150
         assert result["domain_stats"]["light"]["count"] == 50
         assert result["domain_stats"]["switch"]["count"] == 50
-        # Every domain gets at least some entities (fair distribution)
+        # Round-robin interleaving includes entities from every domain
         assert len(result["domain_stats"]["sensor"]["entities"]) >= 3
         assert len(result["domain_stats"]["light"]["entities"]) >= 3
         assert len(result["domain_stats"]["switch"]["entities"]) >= 3
@@ -608,6 +642,50 @@ async def test_pagination_across_multiple_domains(self):
         assert total_returned <= 100
         assert result["pagination"]["has_more"] is True
 
+    @pytest.mark.asyncio
+    async def test_pagination_walks_multiple_domains_without_overlap(self):
+        """Following next_offset tiles a multi-domain round-robin ordering."""
+        entities = [
+            {
+                "entity_id": f"{domain}.{domain[0]}{index}",
+                "attributes": {"friendly_name": f"{domain.title()} {index}"},
+                "state": "on",
+            }
+            for domain in ("sensor", "light", "switch")
+            for index in range(3)
+        ]
+        tools = _make_tools(MockClient(entities=entities))
+
+        pages = []
+        offset = 0
+        while True:
+            page = await tools.get_system_overview(
+                detail_level="standard", limit=3, offset=offset
+            )
+            pages.append(page)
+            next_offset = page["pagination"]["next_offset"]
+            if next_offset is None:
+                break
+            offset = next_offset
+
+        returned_names = [
+            entity["friendly_name"]
+            for page in pages
+            for domain in page["domain_stats"].values()
+            for entity in domain["entities"]
+        ]
+        expected_names = {entity["attributes"]["friendly_name"] for entity in entities}
+
+        assert len(pages) == 3
+        assert all(page["pagination"]["entities_returned"] == 3 for page in pages)
+        assert all(
+            len(domain["entities"]) == 1
+            for page in pages
+            for domain in page["domain_stats"].values()
+        )
+        assert len(returned_names) == len(set(returned_names))
+        assert set(returned_names) == expected_names
+
     @pytest.mark.asyncio
     async def test_pagination_explicit_limit_overrides_default(self):
         """Explicit limit=50 overrides the 200 default."""