feat(server): improve pagination stop

mcp_server.py

@@ -357,7 +357,24 @@ async def fetch_pr_comments(
     max_comments: int | None = None,
     max_retries: int | None = None,
 ) -> list[CommentResult] | None:
-    """Fetches all review comments for a given pull request with pagination support."""
+    """
+    Fetch review comments for a pull request and combine them across
+    paginated responses.
+
+    Parameters:
+        per_page (int | None): Override for number of comments to request
+            per page.
+        max_pages (int | None): Override for maximum number of pages to fetch.
+        max_comments (int | None): Override for a hard limit on total comments
+            to collect.
+        max_retries (int | None): Override for maximum retry attempts on
+            transient errors.
+
+    Returns:
+        list[CommentResult] | None: A list of fetched review comments combined
+        from all pages, or `None` when fetching fails due to timeouts,
+        unrecoverable server errors, or other network-related failures.
+    """
     print(f"Fetching comments for {owner}/{repo}#{pull_number}", file=sys.stderr)
     token = os.getenv("GITHUB_TOKEN")
     headers: dict[str, str] = {
@@ -453,7 +470,7 @@ async def fetch_pr_comments(
 
                                     now = int(time.time())
                                     retry_after = max(int(reset) - now, 1)
-                            except (TypeError, ValueError):
+                            except (ValueError, TypeError):
                                 retry_after = 60
 
                             print(
@@ -528,7 +545,10 @@ async def fetch_pr_comments(
                     match = re.search(r"<([^>]+)>;\s*rel=\"next\"", link_header)
                     next_url = match.group(1) if match else None
                 print(f"DEBUG: next_url={next_url}", file=sys.stderr)
-                url = next_url
+                if next_url:
+                    url = next_url
+                else:
+                    break
 
         total_comments = len(all_comments)
         print(
@@ -758,8 +778,26 @@ async def handle_call_tool(
         self, name: str, arguments: dict[str, Any]
     ) -> Sequence[TextContent]:
         """
-        Handle tool calls.
-        Each tool call is routed to the appropriate method based on the tool name.
+        Dispatch a tool invocation by name and return its textual outputs.
+
+        Parameters:
+            name (str): The tool identifier to invoke (e.g.,
+                "fetch_pr_review_comments", "resolve_open_pr_url").
+            arguments (dict[str, Any]): Tool-specific arguments; expected keys
+                depend on `name` (for example, "pr_url", "per_page",
+                "max_pages", "max_comments", "max_retries",
+                "select_strategy", "owner", "repo", "branch", and "output" for
+                "fetch_pr_review_comments").
+
+        Returns:
+            Sequence[TextContent]: One or more text outputs produced by the
+            invoked tool (e.g., markdown and/or JSON).
+
+        Raises:
+            ValueError: If `name` is unknown or if provided arguments fail
+                validation.
+            RuntimeError: If an underlying HTTP or OS error occurs while
+                executing the requested tool.
         """
 
         async def _run_with_handling(operation: Callable[[], Awaitable[T]]) -> T:
@@ -778,16 +816,56 @@ async def _run_with_handling(operation: Callable[[], Awaitable[T]]) -> T:
             def _validate_int(
                 arg_name: str, value: Any, min_v: int, max_v: int
             ) -> int | None:
+                """
+                Validate and coerce a value to an integer within specified
+                bounds.
+
+                Parameters:
+                    arg_name (str): Name of the argument (used in error
+                        messages).
+                    value (Any): The value to validate; accepts integers or
+                        numeric strings. A value of None is allowed and
+                        returns None.
+                    min_v (int): Minimum allowed value (inclusive).
+                    max_v (int): Maximum allowed value (inclusive).
+
+                Returns:
+                    int | None: The coerced integer when a valid value is
+                    provided, or None if `value` is None.
+
+                Raises:
+                    ValueError: If `value` is a boolean, not an integer or
+                    numeric string, or if the coerced integer is outside the
+                    [min_v, max_v] range.
+                """
                 if value is None:
                     return None
-                if isinstance(value, bool) or not isinstance(value, int):
-                    raise ValueError(f"Invalid type for {arg_name}: expected integer")
-                if not (min_v <= value <= max_v):
+
+                type_error = f"Invalid type for {arg_name}: expected integer"
+
+                # Reject bools explicitly (they're a subclass of int in Python)
+                if isinstance(value, bool):
+                    raise ValueError(type_error)
+
+                # Coerce to int: accept int directly or parse numeric string
+                if isinstance(value, int):
+                    result = value
+                elif isinstance(value, str):
+                    try:
+                        result = int(value, 10)
+                    except ValueError:
+                        raise ValueError(type_error) from None
+                else:
+                    raise ValueError(type_error)
+
+                # Validate range
+                if not (min_v <= result <= max_v):
                     raise ValueError(
-                        f"Invalid value for {arg_name}: must be between {min_v} "
-                        f"and {max_v}"
+                        f"Invalid value for {arg_name}: must be between "
+                        f"{min_v} and {max_v}"
                     )
-                return cast(int, value)
+
+                return result
 
             per_page = _validate_int(
                 "per_page", arguments.get("per_page"), PER_PAGE_MIN, PER_PAGE_MAX

tests/test_integration.py

@@ -146,34 +146,41 @@ class TestRealGitHubIntegration:
     @pytest.mark.asyncio
     async def test_real_github_pr_fetch(self) -> None:
         """
-        Test fetching from a real GitHub PR.
-
-        This test requires GITHUB_TOKEN and uses a known public repository.
-        Marked as integration test - can be skipped in CI if token not available.
+        Verify fetching pull request comments from GitHub when a valid
+        GITHUB_TOKEN is present.
+
+        Skips the test if no suitable token is available. Attempts to fetch
+        comments for a known stable PR (microsoft/TypeScript#27353) with
+        limits on comments and pages, asserts the result is a non-empty list,
+        and checks that the first comment contains the `id`, `body`, and
+        `user` fields. If an HTTP error prevents access to the PR, the test
+        is skipped.
         """
-        # Use a stable public PR for testing (e.g., a closed PR that won't change)
-        # This should be a PR known to exist with comments
         token = os.getenv("GITHUB_TOKEN")
         if not token or token.startswith("test-token") or len(token) < 30:
             pytest.skip("Skipping real GitHub test: no valid GITHUB_TOKEN")
         try:
             comments = await fetch_pr_comments(
-                "octocat",  # GitHub's demo user
-                "Hello-World",  # GitHub's demo repo
-                1,  # First PR (likely to exist and be stable)
-                max_comments=5,  # Limit to avoid large response
+                "microsoft",  # Microsoft organization
+                "TypeScript",  # TypeScript repository
+                27353,  # Stable closed PR with review comments
+                max_comments=10,  # Limit to avoid large response
+                max_pages=2,  # Only fetch first 2 pages to avoid timeout
             )
 
             # Basic validation - real PR should have some structure
             assert isinstance(comments, list)
+            # This PR should have comments, so we can be more assertive
+            assert len(comments) > 0, "Expected PR #27353 to have review comments"
+
             # Real comments should have standard GitHub API fields
-            if comments:  # Only check if comments exist
-                assert "id" in comments[0]
-                assert "body" in comments[0]
+            assert "id" in comments[0]
+            assert "body" in comments[0]
+            assert "user" in comments[0]
 
         except httpx.HTTPError as e:
             # If we can't access the test PR, skip rather than fail
-            pytest.skip(f"Could not access test PR for integration test: {e}")
+            pytest.skip(f"Could not access test PR microsoft/TypeScript#27353: {e}")
 
     @pytest.mark.integration
     @pytest.mark.asyncio
@@ -183,21 +190,38 @@ async def test_real_pr_resolution(self) -> None:
         if not token or token.startswith("test-token") or len(token) < 30:
             pytest.skip("Skipping real GitHub test: no valid GITHUB_TOKEN")
         try:
-            # Try to resolve PRs for a known active repository
-            pr_url = await git_pr_resolver.resolve_pr_url(
-                "octocat", "Hello-World", select_strategy="first"
-            )
-
-            # Should return a valid GitHub PR URL
-            assert pr_url.startswith("https://github.com/")
-            assert "/pull/" in pr_url
+            # Try to resolve PRs for repositories that are more likely to have open PRs
+            test_repos = [
+                ("microsoft", "TypeScript"),  # Very active repo
+                ("facebook", "react"),  # Very active repo
+                ("octocat", "Hello-World"),  # Original test case
+            ]
 
-        except ValueError as e:
-            if "No open PRs found" in str(e):
-                # This is fine - the test repo might not have open PRs
-                pytest.skip("Test repository has no open PRs")
-            else:
-                raise
+            success = False
+            for owner, repo in test_repos:
+                try:
+                    pr_url = await git_pr_resolver.resolve_pr_url(
+                        owner, repo, select_strategy="first"
+                    )
+
+                    # Should return a valid GitHub PR URL
+                    assert pr_url.startswith("https://github.com/")
+                    assert "/pull/" in pr_url
+                    success = True
+                    break
+
+                except ValueError as e:
+                    if "No open PRs found" in str(e):
+                        continue  # Try next repo
+                    else:
+                        raise
+
+            if not success:
+                pytest.skip("No test repositories have open PRs available")
+
+        except (OSError, RuntimeError) as e:
+            # Catch system and runtime errors and skip rather than fail
+            pytest.skip(f"Could not test PR resolution: {e}")
 
 
 class TestErrorRecoveryAndResilience:

tests/test_mcp_server_tools.py

@@ -64,8 +64,34 @@ async def test_handle_call_tool_invalid_type(mcp_server: ReviewSpecGenerator) ->
         )
 
 
+@pytest.mark.asyncio
+async def test_handle_call_tool_rejects_bool(mcp_server: ReviewSpecGenerator) -> None:
+    """Test that boolean values are rejected for integer parameters."""
+    with pytest.raises(ValueError, match="Invalid type for per_page: expected integer"):
+        await mcp_server.handle_call_tool(
+            "fetch_pr_review_comments",
+            {"pr_url": "https://github.com/o/r/pull/1", "per_page": True},
+        )
+
+
+@pytest.mark.asyncio
+async def test_handle_call_tool_rejects_float(mcp_server: ReviewSpecGenerator) -> None:
+    """Test that float values are rejected to prevent silent truncation."""
+    with pytest.raises(ValueError, match="Invalid type for per_page: expected integer"):
+        await mcp_server.handle_call_tool(
+            "fetch_pr_review_comments",
+            {"pr_url": "https://github.com/o/r/pull/1", "per_page": 50.7},
+        )
+
+
 @pytest.mark.asyncio
 async def test_handle_call_tool_invalid_output(mcp_server: ReviewSpecGenerator) -> None:
+    """
+    Validates that handle_call_tool rejects unsupported output formats.
+
+    Asserts that calling handle_call_tool with an invalid `output` value
+    raises a ValueError whose message contains "Invalid output".
+    """
     with pytest.raises(ValueError, match="Invalid output"):
         await mcp_server.handle_call_tool(
             "fetch_pr_review_comments",

tests/test_rest_error_handling.py

@@ -195,6 +195,30 @@ async def test_fetch_pr_comments_returns_none_for_invalid_payload() -> None:
     assert result is None
 
 
+@pytest.mark.asyncio
+async def test_fetch_pr_comments_raises_4xx_client_errors() -> None:
+    """Should raise HTTPStatusError for 4xx client errors without retrying."""
+    request = httpx.Request("GET", "https://api.github.com")
+    http_error = httpx.HTTPStatusError(
+        "Not found",
+        request=request,
+        response=httpx.Response(404, request=request),
+    )
+    error_response = _make_response(status=404, raise_error=http_error)
+
+    mock_client = AsyncMock()
+    mock_client.get.return_value = error_response
+    mock_client.__aenter__.return_value = mock_client
+    mock_client.__aexit__.return_value = None
+
+    with patch("mcp_server.httpx.AsyncClient", return_value=mock_client):
+        with pytest.raises(httpx.HTTPStatusError, match="Not found"):
+            await fetch_pr_comments("owner", "repo", 1, max_retries=3)
+
+    # Should only make one request (no retries for 4xx)
+    assert mock_client.get.call_count == 1
+
+
 @pytest.mark.asyncio
 async def test_fetch_pr_comments_handles_timeout_exception() -> None:
     """Should return None when httpx raises a TimeoutException."""