feat(server): improve pagination stop

[petems]

• accept numeric strings for tool args, and harden markdown user parsing

Summary by CodeRabbit

• New Features

  • Improved numeric input validation: accepts numeric strings, rejects booleans/floats, enforces min/max bounds with clearer messages.
  • Enhanced user-facing documentation for tool invocation and comment-fetching behaviors.

• Bug Fixes

  • Safer pagination that stops when no next page is present.
  • Reordered error handling for more robust retry timing.
  • Localized time usage for more precise backoff computations.

• Tests

  • Expanded integration tests across real repositories; HTTP errors now skip with repo-specific messages.
  • New unit tests for numeric input rejection and 4xx error propagation (no retries).

[gemini-code-assist]

Summary of Changes

Hello @petems, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the server's stability and flexibility by refining its data handling mechanisms. It addresses potential infinite loops in pagination, strengthens the parsing of user data for markdown generation, and broadens the accepted input types for integer validation, making the system more resilient to varied inputs.

Highlights

• Pagination Logic Improvement: The server's pagination loop in _int_conf now includes a break condition if next_url is not found, preventing potential infinite loops when fetching paginated data.
• Hardened Markdown User Parsing: The fence_for function now safely retrieves user data by providing a default empty dictionary for comment.get("user"), making the markdown generation more robust against missing user information.
• Flexible Integer Validation: The _validate_int function has been updated to accept numeric strings as valid input, converting them to integers. It also explicitly rejects boolean values, improving the flexibility and safety of tool argument parsing.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[gemini-code-assist]

Code Review

This pull request introduces several improvements, including more robust pagination handling, safer user data parsing from comments, and flexible type handling for tool arguments by accepting numeric strings. The changes are positive. I've added a couple of suggestions to further refine the code by simplifying a redundant check and using more specific exception handling for better robustness.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds an internal integer validation helper, tightens Retry-After parsing, refines pagination termination for GitHub comment fetching, localizes time import, enriches docstrings, and expands tests to cover pagination, 4xx/5xx error behaviors, malformed data, and stricter tool-input validation.

Changes

Cohort / File(s)	Summary
Server logic mcp_server.py	Introduced _validate_int to coerce numeric strings and reject booleans with min/max enforcement; updated uses for per_page, max_pages, max_comments, max_retries; swapped exception order when parsing Retry-After to catch ValueError before TypeError; pagination loop now breaks when next_url is absent rather than assigning None; localized import time and expanded docstrings for fetch_pr_comments and handle_call_tool.
Integration tests tests/test_integration.py	Updated live-PR targets (e.g., microsoft/TypeScript#27353), increased max_comments/max_pages, added assertions on comment fields and pagination behavior, broadened PR-resolution to try multiple repos with first-success strategy, and added server/malformed-data handling checks; network/token HTTP errors now skip tests.
Tool validation tests tests/test_mcp_server_tools.py	Added async tests asserting ValueError when per_page is a boolean or a float; clarified test_handle_call_tool_invalid_output with an explanatory docstring.
REST error handling tests tests/test_rest_error_handling.py	Added test_fetch_pr_comments_raises_4xx_client_errors to ensure 4xx HTTPStatusError from fetch_pr_comments is propagated without retries and that the client made a single GET call.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant S as MCP Server
  participant GH as GitHub API

  C->>S: fetch_pr_comments(repo, pr, per_page?, max_pages?, max_comments?)
  rect rgb(245,250,245)
    loop paginate (until no next_url or max_pages reached)
      S->>GH: GET /comments?page=n
      alt 2xx
        GH-->>S: 200 + comments + Link/Rate headers
        S->>S: accumulate comments (cap at max_comments)
        alt Link has next_url and pages < max_pages
          S->>S: set next_url, continue
        else
          S->>S: break loop (no next_url)
        end
      else 429/403 (rate-limited)
        S->>S: compute retry_after (catch ValueError then TypeError)
        S-->>C: backoff / return None
      else 4xx
        GH-->>S: 4xx error
        S-->>C: raise HTTPStatusError (no retries)
      else 5xx
        S-->>C: return None
      end
    end
  end
  S-->>C: aggregated comments list or None / raise

Loading

sequenceDiagram
  autonumber
  participant S as MCP Server
  participant V as _validate_int

  S->>V: _validate_int(value, name, min?, max?)
  alt value is bool
    V-->>S: raise ValueError("Invalid type for {name}: expected integer")
  else value is int or numeric string
    V->>V: coerced = int(value)
    alt parse succeeds
      V->>V: enforce min/max bounds
      V-->>S: return coerced int
    else parse fails
      V-->>S: raise ValueError("Invalid integer for {name}")
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I nibbled links from page to page,
Counting numbers, strict and sage.
No booleans hiding in my stash,
I turned strings to ints in a flash.
Tests hopped by — a tidy bash. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title concisely describes the enhanced pagination stopping logic, which is a real and significant part of the changeset, and is clear and specific without being overly broad.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch split/server-fixes

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eccfeed and 1e77d29.

📒 Files selected for processing (1)

• mcp_server.py (5 hunks)

🔇 Additional comments (4)

mcp_server.py (4)

360-377: Well-documented pagination and parameter handling.

The expanded docstring clearly describes the pagination aggregation behavior, parameter overrides, and failure semantics. The documentation accurately reflects the function's behavior and will help users understand the pagination limits and error handling.

---

548-551: Clearer pagination termination.

The explicit break when next_url is absent makes the pagination stopping condition immediately obvious, compared to the implicit behavior of assigning None and relying on the while url: check. This improves readability and intent.

---

781-801: Comprehensive tool handler documentation.

The expanded docstring provides clear guidance on the dispatcher's interface, documenting all parameters, return types, and exceptions. The examples of tool names and argument keys are particularly helpful for understanding the expected inputs.

---

816-868: Robust integer validation with proper type handling.

The validation logic correctly addresses the previous concern about float truncation by explicitly rejecting floats while accepting numeric strings:

• Booleans rejected upfront (line 847)
• Integers accepted directly (line 851)
• Strings parsed with explicit base-10 conversion (line 855), rejecting decimal strings like "1.5"
• All other types (including floats) rejected (line 858)
• Range validation applied consistently (line 862)

The use of int(value, 10) prevents ambiguity with octal or hex representations, and the comprehensive docstring documents all validation rules clearly.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1baae2e and 5055960.

📒 Files selected for processing (2)

• mcp_server.py (3 hunks)
• tests/test_integration.py (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_integration.py (2)

mcp_server.py (1)

• fetch_pr_comments (350-550)

git_pr_resolver.py (1)

• resolve_pr_url (114-224)

[petems]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[petems]

@gemini review

[petems]

@coderabbitai generate docstrings

[coderabbitai]

✅ Actions performed

Docstrings generation started.

[coderabbitai]

Note

Generated docstrings for this pull request at #34

[petems]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several valuable improvements, enhancing robustness and test coverage. The changes to accept numeric strings for tool arguments and fix the pagination logic are well-implemented. My review includes a couple of suggestions to further improve code style and maintainability, specifically by adhering to PEP 8 for imports and refactoring a validation function to be more concise.

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb6ef9e and 056d5d5.

📒 Files selected for processing (3)

• mcp_server.py (6 hunks)
• tests/test_integration.py (3 hunks)
• tests/test_mcp_server_tools.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_mcp_server_tools.py
• tests/test_integration.py

🧰 Additional context used

🪛 GitHub Actions: Lint

mcp_server.py

[error] 361-361: E501 Line too long (89 > 88).

---

[error] 366-366: E501 Line too long (90 > 88).

---

[error] 367-367: E501 Line too long (90 > 88).

---

[error] 370-370: E501 Line too long (94 > 88).

---

[error] 371-371: E501 Line too long (92 > 88).

---

[error] 780-780: E501 Line too long (112 > 88).

---

[error] 781-781: E501 Line too long (100 > 88).

---

[error] 782-782: E501 Line too long (109 > 88).

---

[error] 783-783: E501 Line too long (118 > 88).

---

[error] 786-786: E501 Line too long (137 > 88).

---

[error] 790-790: E501 Line too long (102 > 88).

---

[error] 814-814: E501 Line too long (137 > 88).

---

[error] 819-819: E501 Line too long (111 > 88).

---

[error] 822-822: E501 Line too long (153 > 88).

🔇 Additional comments (2)

mcp_server.py (2)

544-547: LGTM! Pagination stop logic is now correct.

The new logic properly terminates the pagination loop when no next_url is available, preventing attempts to fetch with url=None. This is an important correctness fix.

---

824-846: LGTM! Integer validation now correctly rejects floats.

The implementation properly addresses the previous concern about silent float truncation. The logic correctly:

• Rejects booleans explicitly (necessary since isinstance(True, int) is True)
• Accepts actual integers as-is
• Accepts and validates numeric strings using int(value, 10)
• Rejects floats and other types

This ensures that only true integers and their string representations are accepted.

Based on past review comments.

[petems]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several valuable improvements. The pagination logic is now more robust, correctly handling the end of comment pages. The argument parsing for tools is hardened to accept numeric strings while rejecting invalid types like booleans, which is a great enhancement for usability. The integration tests have been significantly improved by using more reliable public repositories and handling flakiness, which will improve CI stability. I have one minor suggestion in mcp_server.py to improve code clarity in the validation logic.