review(#135): unbounded by default + eager validation in 7 wrapper methods

Per bot review on PR #135:

### Behavior change: max_pages=None is now truly unbounded

Dropped the silent 1000-page cap. None iterates until the server
returns no cursor; the existing cursor-repeat guard remains the
safety net against infinite loops (cap was never part of the public
contract). Bot's recommended path: most users expect None = unbounded
and a silent truncation post-release would be a harder break to fix.

### Fix: 7 wrapper methods had lazy validation

Methods using `yield from self._list_all(...)` (5 sync) or
`async for item in ... yield item` (2 async) were silently generators
— their body, including _validate_max_pages, did not execute until
the caller advanced the iterator. milestones.list_all(max_pages=0)
returned a generator without raising; the ValueError only fired on
the first next().

Fixed by converting to `return self._list_all(...)`:
- communications.{Sync,Async}.list_all_rfqs
- incentive_programs.list_all
- milestones.list_all
- structured_targets.list_all
- subaccounts.{Sync,Async}.list_all_transfers

Async ones additionally need `def` (not `async def`) so they're plain
functions returning the AsyncIterator, not coroutines.

### Async drift FQNs

Added all 19 async resource FQNs to _MAX_PAGES_FQNS in
tests/_contract_support.py so the drift framework recognizes the
kwarg if coverage is ever extended to async classes.

### Regression tests

- TestMaxPagesEagerValidation × 4 (sync + async, both styles) — asserts
  ValueError fires at call time, not on first __next__.
- TestMaxPagesNoneIsUnbounded × 1 — server returns 1100 pages, suite
  iterates all 1100 (proves the 1000 cap is gone).

Verify: 1577 passed (+5). ruff + mypy --strict clean.

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

Author: TexasCoding

kalshi/resources/_base.py

@@ -173,24 +173,24 @@ def _list_all(
         convention). ``cursor_key`` only affects how the response envelope
         is parsed.
 
-        ``max_pages`` caps the number of pages fetched as a safety net for
-        servers that never repeat a cursor but never return empty either.
-        ``None`` (default) means the built-in ceiling of 1000. Callers that
-        need a tighter cap (e.g. a quick preview) pass it explicitly.
+        ``max_pages`` is an optional hard cap on pages fetched. ``None``
+        (default) iterates until the server returns an empty cursor; the
+        cursor-repeat guard below provides the safety net against
+        infinite loops.
 
         Raises ``KalshiError`` if a cursor value repeats, which indicates
-        a server-side pagination bug that would otherwise cause the safety
-        cap to silently issue ``max_pages`` duplicate requests.
+        a server-side pagination bug.
         """
-        page_cap = 1000 if max_pages is None else max_pages
         current_params = dict(params) if params else {}
         seen_cursors: set[str] = set()
-        for _ in range(page_cap):
+        pages_fetched = 0
+        while max_pages is None or pages_fetched < max_pages:
             page = self._list(
                 path, model_cls, items_key,
                 params=current_params, cursor_key=cursor_key,
             )
             yield from page.items
+            pages_fetched += 1
             if not page.cursor:
                 break
             if page.cursor in seen_cursors:
@@ -286,20 +286,18 @@ async def _list_all(
         max_pages: int | None = None,
         cursor_key: str = "cursor",
     ) -> AsyncIterator[T]:
-        """Async counterpart of ``SyncResource._list_all``. ``max_pages``
-        semantics mirror the sync version (``None`` -> 1000 default).
-        Raises ``KalshiError`` on repeated cursor; see sync docstring.
-        """
-        page_cap = 1000 if max_pages is None else max_pages
+        """Async counterpart of :meth:`SyncResource._list_all`."""
         current_params = dict(params) if params else {}
         seen_cursors: set[str] = set()
-        for _ in range(page_cap):
+        pages_fetched = 0
+        while max_pages is None or pages_fetched < max_pages:
             page = await self._list(
                 path, model_cls, items_key,
                 params=current_params, cursor_key=cursor_key,
             )
             for item in page.items:
                 yield item
+            pages_fetched += 1
             if not page.cursor:
                 break
             if page.cursor in seen_cursors:

kalshi/resources/communications.py

@@ -222,7 +222,7 @@ def list_all_rfqs(
             market_ticker=market_ticker, subaccount=subaccount,
             status=status, creator_user_id=creator_user_id,
         )
-        yield from self._list_all(
+        return self._list_all(
             "/communications/rfqs", RFQ, "rfqs",
             params=params, max_pages=max_pages,
         )
@@ -418,7 +418,7 @@ async def list_rfqs(
         )
         return await self._list("/communications/rfqs", RFQ, "rfqs", params=params)
 
-    async def list_all_rfqs(
+    def list_all_rfqs(
         self,
         *,
         limit: int | None = None,
@@ -429,18 +429,18 @@ async def list_all_rfqs(
         creator_user_id: str | None = None,
         max_pages: int | None = None,
     ) -> AsyncIterator[RFQ]:
+        # Plain `def` so _require_auth + _validate_max_pages run at call time.
         self._require_auth()
         _validate_max_pages(max_pages)
         params = _list_rfqs_params(
             cursor=None, limit=limit, event_ticker=event_ticker,
             market_ticker=market_ticker, subaccount=subaccount,
             status=status, creator_user_id=creator_user_id,
         )
-        async for item in self._list_all(
+        return self._list_all(
             "/communications/rfqs", RFQ, "rfqs",
             params=params, max_pages=max_pages,
-        ):
-            yield item
+        )
 
     async def get_rfq(self, rfq_id: str) -> GetRFQResponse:
         self._require_auth()

kalshi/resources/incentive_programs.py

@@ -62,7 +62,7 @@ def list_all(
     ) -> Iterator[IncentiveProgram]:
         _validate_max_pages(max_pages)
         params = _params(status=status, type=incentive_type, limit=limit)
-        yield from self._list_all(
+        return self._list_all(
             "/incentive_programs",
             IncentiveProgram,
             "incentive_programs",

kalshi/resources/milestones.py

@@ -119,7 +119,7 @@ def list_all(
             related_event_ticker=related_event_ticker,
             cursor=None, min_updated_ts=min_updated_ts,
         )
-        yield from self._list_all(
+        return self._list_all(
             "/milestones", Milestone, "milestones",
             params=params, max_pages=max_pages,
         )

kalshi/resources/structured_targets.py

@@ -66,7 +66,7 @@ def list_all(
             competition=competition,
             page_size=page_size,
         )
-        yield from self._list_all(
+        return self._list_all(
             "/structured_targets",
             StructuredTarget,
             "structured_targets",

kalshi/resources/subaccounts.py

@@ -160,7 +160,7 @@ def list_all_transfers(
         self._require_auth()
         _validate_max_pages(max_pages)
         params = _params(limit=limit)
-        yield from self._list_all(
+        return self._list_all(
             "/portfolio/subaccounts/transfers",
             SubaccountTransfer,
             "transfers",
@@ -254,23 +254,24 @@ async def list_transfers(
             params=params,
         )
 
-    async def list_all_transfers(
+    def list_all_transfers(
         self,
         *,
         limit: int | None = None,
         max_pages: int | None = None,
     ) -> AsyncIterator[SubaccountTransfer]:
+        # Plain `def` (not `async def`) so _require_auth and _validate_max_pages
+        # run at call time, not when the returned AsyncIterator is awaited.
         self._require_auth()
         _validate_max_pages(max_pages)
         params = _params(limit=limit)
-        async for item in self._list_all(
+        return self._list_all(
             "/portfolio/subaccounts/transfers",
             SubaccountTransfer,
             "transfers",
             params=params,
             max_pages=max_pages,
-        ):
-            yield item
+        )
 
     @overload
     async def update_netting(

tests/_contract_support.py

@@ -1061,6 +1061,26 @@ class Exclusion:
     "kalshi.resources.incentive_programs.IncentiveProgramsResource.list_all",
     "kalshi.resources.structured_targets.StructuredTargetsResource.list_all",
     "kalshi.resources.multivariate.MultivariateCollectionsResource.list_all",
+    # Async counterparts — same kwarg, same client-only semantics.
+    "kalshi.resources.markets.AsyncMarketsResource.list_all",
+    "kalshi.resources.markets.AsyncMarketsResource.list_trades_all",
+    "kalshi.resources.milestones.AsyncMilestonesResource.list_all",
+    "kalshi.resources.events.AsyncEventsResource.list_all",
+    "kalshi.resources.events.AsyncEventsResource.list_all_multivariate",
+    "kalshi.resources.historical.AsyncHistoricalResource.markets_all",
+    "kalshi.resources.historical.AsyncHistoricalResource.fills_all",
+    "kalshi.resources.historical.AsyncHistoricalResource.orders_all",
+    "kalshi.resources.historical.AsyncHistoricalResource.trades_all",
+    "kalshi.resources.orders.AsyncOrdersResource.list_all",
+    "kalshi.resources.orders.AsyncOrdersResource.fills_all",
+    "kalshi.resources.communications.AsyncCommunicationsResource.list_all_rfqs",
+    "kalshi.resources.communications.AsyncCommunicationsResource.list_all_quotes",
+    "kalshi.resources.subaccounts.AsyncSubaccountsResource.list_all_transfers",
+    "kalshi.resources.portfolio.AsyncPortfolioResource.settlements_all",
+    "kalshi.resources.fcm.AsyncFcmResource.orders_all",
+    "kalshi.resources.incentive_programs.AsyncIncentiveProgramsResource.list_all",
+    "kalshi.resources.structured_targets.AsyncStructuredTargetsResource.list_all",
+    "kalshi.resources.multivariate.AsyncMultivariateCollectionsResource.list_all",
 )
 for _fqn in _MAX_PAGES_FQNS:
     EXCLUSIONS[(_fqn, "max_pages")] = Exclusion(

tests/test_base_helpers.py

@@ -144,7 +144,7 @@ class TestSyncListAllCursorLoopDetection:
     def test_repeated_cursor_raises(
         self, test_auth: KalshiAuth, test_config: KalshiConfig
     ) -> None:
-        """Server that returns the same cursor twice must bail fast, not retry 1000x."""
+        """Server that returns the same cursor twice must bail fast on the 2nd request."""
         route = respx.get("https://test.kalshi.com/trade-api/v2/things").mock(
             return_value=httpx.Response(
                 200, json={"items": [{"id": "x"}], "cursor": "loop"}
@@ -156,8 +156,7 @@ def test_repeated_cursor_raises(
             list(resource._list_all("/things", _Item, "items"))
 
         # First call (no cursor) fetches cursor="loop". Second call (cursor=loop) returns
-        # cursor="loop" again → loop detected before a third request. Total: 2 requests,
-        # not 1000.
+        # cursor="loop" again → loop detected before a third request.
         assert route.call_count == 2
 
     @respx.mock
@@ -366,3 +365,74 @@ def test_zero_rejected(self) -> None:
     def test_negative_rejected(self) -> None:
         with pytest.raises(ValueError, match=r"max_pages must be positive"):
             _validate_max_pages(-3)
+
+
+class TestMaxPagesEagerValidation:
+    """Regression: ``_validate_max_pages`` must fire at call time, not on first iteration.
+
+    Methods that were previously written with ``yield from``/``async for ... yield``
+    silently became generators — their body didn't execute until the caller advanced
+    the iterator, so `max_pages=0` got deferred. All `*_all()` methods must now use
+    `return self._list_all(...)` so the validator runs eagerly.
+    """
+
+    def test_sync_milestones_list_all_validates_eagerly(
+        self, test_auth: KalshiAuth, test_config: KalshiConfig,
+    ) -> None:
+        from kalshi.resources.milestones import MilestonesResource
+        resource = MilestonesResource(SyncTransport(test_auth, test_config))
+        with pytest.raises(ValueError, match=r"max_pages must be positive"):
+            resource.list_all(limit=10, max_pages=0)
+
+    def test_sync_subaccounts_list_all_transfers_validates_eagerly(
+        self, test_auth: KalshiAuth, test_config: KalshiConfig,
+    ) -> None:
+        from kalshi.resources.subaccounts import SubaccountsResource
+        resource = SubaccountsResource(SyncTransport(test_auth, test_config))
+        with pytest.raises(ValueError, match=r"max_pages must be positive"):
+            resource.list_all_transfers(max_pages=0)
+
+    @pytest.mark.asyncio
+    async def test_async_subaccounts_list_all_transfers_validates_eagerly(
+        self, test_auth: KalshiAuth, test_config: KalshiConfig,
+    ) -> None:
+        from kalshi.resources.subaccounts import AsyncSubaccountsResource
+        resource = AsyncSubaccountsResource(AsyncTransport(test_auth, test_config))
+        with pytest.raises(ValueError, match=r"max_pages must be positive"):
+            resource.list_all_transfers(max_pages=0)
+
+    @pytest.mark.asyncio
+    async def test_async_communications_list_all_rfqs_validates_eagerly(
+        self, test_auth: KalshiAuth, test_config: KalshiConfig,
+    ) -> None:
+        from kalshi.resources.communications import AsyncCommunicationsResource
+        resource = AsyncCommunicationsResource(AsyncTransport(test_auth, test_config))
+        with pytest.raises(ValueError, match=r"max_pages must be positive"):
+            resource.list_all_rfqs(max_pages=0)
+
+
+class TestMaxPagesNoneIsUnbounded:
+    """``max_pages=None`` must iterate until the server returns no cursor.
+
+    The 1000-page default was removed: cursor-repeat guard is the real safety net.
+    """
+
+    @respx.mock
+    def test_sync_iterates_past_1000_pages(
+        self, test_auth: KalshiAuth, test_config: KalshiConfig,
+    ) -> None:
+        # Page N returns cursor str(N+1) for the first 1100 pages, then empty.
+        call_counter = {"n": 0}
+
+        def responder(request: httpx.Request) -> httpx.Response:
+            call_counter["n"] += 1
+            n = call_counter["n"]
+            cursor = str(n) if n < 1100 else ""
+            return httpx.Response(
+                200, json={"items": [{"id": f"i{n}"}], "cursor": cursor},
+            )
+
+        respx.get("https://test.kalshi.com/trade-api/v2/things").mock(side_effect=responder)
+        resource = SyncResource(SyncTransport(test_auth, test_config))
+        items = list(resource._list_all("/things", _Item, "items"))
+        assert len(items) == 1100, f"Expected 1100 items, got {len(items)} (cap leaked?)"