feat: Enhanced GitHub API rate limit handling with structured logging

[petems]

Summary

This PR implements comprehensive GitHub API rate limit handling with differentiation between primary and secondary rate limits, along with production-grade structured logging for observability.

Key Features

• 🛡️ Robust Rate Limit Handling: Distinguish between secondary (abuse) and primary rate limits
• 🔄 Smart Retry Logic: One-retry policy for secondary limits, header-based backoff for primary limits
• 📊 Structured Logging: Machine-parseable logs with contextual metadata for monitoring
• 🏗️ Clean Architecture: Extracted rate limit logic into dedicated RateLimitHandler class
• ✅ Comprehensive Testing: 350 tests passing (12 new unit tests added)

Changes

1. Rate Limit Detection & Retry (644bf4f)

New Components:

• SecondaryRateLimitError exception for clean abort behavior
• Helper functions for rate limit detection and delay calculation
• Support for both REST and GraphQL API endpoints

Rate Limit Handling:

# Secondary (abuse detection) - 60s backoff, one retry
if "secondary rate limit" in response.message:
    - Retry once with 60s backoff
    - Abort if persists

# Primary rate limits - respect GitHub headers
if Retry-After or X-RateLimit-Reset:
    - Extract delay from headers
    - Backoff and retry

Improvements:

• Log GitHub Request IDs for support escalation
• Increase exponential backoff cap: 5s → 15s
• Unified handling across REST and GraphQL endpoints

2. RateLimitHandler Class Extraction (be8ccd3)

Refactoring Benefits:

• -97 lines of duplication removed between REST and GraphQL handlers
• Stateful tracking of retry attempts per handler instance
• Easier testing with dependency injection pattern
• Clear separation of concerns

New Class Structure:

class RateLimitHandler:
    def __init__(self, context: str, secondary_backoff: float = 60.0)
    async def handle_rate_limit(self, response: httpx.Response) -> str | None
    # + private methods for detection and delay calculation

Test Coverage:

• 6 new unit tests for RateLimitHandler class
• Tests verify: success responses, secondary limits, primary limits, custom backoff

3. Structured Logging (64c9bdc)

Logging Infrastructure:

logger = logging.getLogger(__name__)

# Example structured log
logger.warning(
    "Secondary rate limit detected; backing off and retrying",
    extra={
        "context": "fetch_pr_comments_graphql",
        "status_code": 403,
        "backoff_seconds": 60,
        "rate_limit_type": "secondary",
    },
)

Log Levels:

• DEBUG: Page fetching progress
• INFO: Request IDs, max_comments limit reached
• WARNING: Rate limit detection, auth fallback
• ERROR: Rate limit exhaustion, GraphQL errors

Updated Logs:

• ✅ Rate limit events with type, backoff duration, context
• ✅ GitHub Request IDs for support escalation
• ✅ GraphQL errors with full error details
• ✅ Authentication fallback with metadata
• ✅ Pagination limits with fetched counts

Test Updates:

• Configured caplog to capture logger output at INFO level
• Updated 6 tests to use caplog.text instead of captured.err

Test Results

✅ 350 tests passing (6 new unit tests added)
✅ All linting checks pass
✅ Type checking passes (mypy)
✅ Format checks pass (ruff)

New Tests:

• test_rate_limit_handler_ignores_success_response
• test_rate_limit_handler_secondary_limit_retry
• test_rate_limit_handler_secondary_limit_exhausted
• test_rate_limit_handler_primary_limit_retry_after
• test_rate_limit_handler_primary_limit_reset_header
• test_rate_limit_handler_custom_backoff

Benefits

Operational

• 🔍 Better Observability: Structured logs integrate with ELK, Datadog, Splunk
• 🚨 Easier Debugging: Rich context in every log message
• 📈 Monitoring: Filter/group by rate_limit_type, context, status_code
• 🎯 Support Escalation: GitHub Request IDs captured automatically

Code Quality

• 🧹 Reduced Duplication: DRY principle applied to rate limit handling
• 🧪 Better Testability: Unit tests for rate limit logic in isolation
• 📚 Documentation: Comprehensive docstrings on all new methods
• 🏗️ Maintainability: Clear separation of concerns

Risk Assessment

Risk Level: ✅ Low

• No breaking changes to public API
• Existing error handling patterns preserved
• All tests pass including integration tests
• Backward compatible with existing callers

Future Enhancements

Follow-up opportunities identified in code review:

• Make SECONDARY_RATE_LIMIT_BACKOFF configurable via environment variable
• Add metrics/telemetry for rate limit frequency
• Implement circuit breaker for repeated secondary limits
• Add token-gated integration test with live rate limiting

---

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Summary by CodeRabbit

• Improvements

  • Smarter GitHub API rate‑limit handling with exponential backoff capped at 15s, clearer abort behavior on persistent secondary limits, and unified handling across REST/GraphQL flows.
  • Replaced ad‑hoc prints with structured logging for API errors, auth issues, pagination progress, and request IDs.

• Tests

  • Added comprehensive rate‑limit/retry tests and updated existing tests to assert on logs via logging capture.

• Documentation

  • Guidance added recommending log capture (caplog) and preferring logging over prints.

[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

Introduces structured logging, a centralized RateLimitHandler and SecondaryRateLimitError for primary/secondary rate-limit detection and retry control, raises exponential backoff cap to 15.0s, and wires rate-limit-aware status handlers into GraphQL and REST comment-fetching flows while replacing print diagnostics with logger calls.

Changes

Cohort / File(s)	Summary
Core server implementation src/mcp_github_pr_review/server.py	Added module logger and replaced prints with structured logging. Added SECONDARY_RATE_LIMIT_BACKOFF and PRIMARY_RATE_LIMIT_MAX_RETRIES constants, SecondaryRateLimitError, and RateLimitHandler to encapsulate rate-limit detection, request-id logging, backoff and retry decisions. Increased _calculate_backoff_delay cap to 15.0s and extended _retry_http_request to accept an optional status_handler. Integrated RateLimitHandler into fetch_pr_comments_graphql and fetch_pr_comments with per-call status handling and updated error/pagination logging.
Tests — logging capture updates tests/test_graphql_error_handling.py	Replaced stderr capture (capsys) with logging capture (caplog), set logging level in tests, and updated assertions to check caplog.text. Test fixtures updated to accept caplog: pytest.LogCaptureFixture.
Tests — new rate-limit coverage tests/test_rate_limit_handling.py	Added comprehensive unit tests for rate-limit handling and backoff behavior, including REST/GraphQL mock responses, SleepRecorder, AsyncClient mocks, and scenarios for secondary/primary limits, Retry-After/X-RateLimit-Reset handling, retry exhaustion, and backoff cap verification.
Docs / guidance CLAUDE.md	Added guidance recommending use of caplog for tests and preferring logging over print statements.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller as Client
    participant Retry as _retry_http_request
    participant Status as status_handler (callback)
    participant RHandler as RateLimitHandler
    participant API as GitHub API

    Caller->>Retry: call request_fn(status_handler)
    loop attempts
        Retry->>API: perform HTTP request
        API-->>Retry: httpx.Response
        alt status_handler provided
            Retry->>Status: await status_handler(response, attempt)
            Status->>RHandler: delegate handle_rate_limit(response)
            alt Secondary rate limit
                RHandler->>RHandler: sleep(SECONDARY_RATE_LIMIT_BACKOFF)
                RHandler-->>Status: return retry or raise SecondaryRateLimitError
                Status-->>Retry: retry or abort
            else Primary rate limit
                RHandler->>RHandler: compute delay (Retry-After / X-RateLimit-Reset)
                RHandler-->>Status: return delay directive
                Status-->>Retry: retry after delay
            else No rate limit / success
                RHandler-->>Status: return None
                Status-->>Retry: return response
            end
        else
            alt transient failure
                Retry->>Retry: compute exponential backoff (cap 15s)
                Retry->>Retry: sleep(delay)
            else success
                Retry-->>Caller: return response
            end
        end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Areas to focus on:

• RateLimitHandler logic for detecting primary vs secondary limits and parsing Retry-After / X-RateLimit-Reset.
• Interaction between _retry_http_request and the new status_handler callback (async flow and return semantics).
• Handling of SecondaryRateLimitError in GraphQL and REST flows and pagination boundary logging.
• New tests in tests/test_rate_limit_handling.py for correctness of mocks and sleep/backoff assertions.

Possibly related PRs

• refactor: rename package and remove legacy 'spec-maker' branding #43 — touches the same server rate-limit, retry and fetch comment paths and likely overlaps the refactor and retry logic.

Poem

🐰 I hopped through logs and waited, calm and steady,
Counting Retry-After ticks with backoff paws ready.
I log each request-id, then rest a little while,
Two strikes for secondary, primary waits with style.
A rabbit's cheer — retries tamed and tidy!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.05% 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 PR title "feat: Enhanced GitHub API rate limit handling with structured logging" directly corresponds to the primary changes in the changeset. The two key components mentioned—enhanced rate limit handling and structured logging—are accurately reflected throughout the modifications: the server.py file introduces the RateLimitHandler class, SecondaryRateLimitError, rate limit constants, and replaces print statements with logger calls across both GraphQL and REST paths; the tests are updated to use caplog for log verification; and documentation reflects logging best practices. The title is specific and clear, avoiding vague terminology, and appropriately captures the main refactoring effort without being either too narrow or 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 feat/enhanced-rate-limit-handling

---

📜 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 50bb3eb and e48aa7e.

📒 Files selected for processing (4)

• CLAUDE.md (1 hunks)
• src/mcp_github_pr_review/server.py (14 hunks)
• tests/test_graphql_error_handling.py (10 hunks)
• tests/test_rate_limit_handling.py (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• CLAUDE.md

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use Ruff for formatting and linting; code must be formatted with Ruff and pass Ruff checks
Adhere to line length of 88 characters (Ruff configuration)
Target Python 3.10+ syntax and features
Maintain static typing that passes mypy
Keep imports sorted per Ruff’s import sorting rules
Address security findings covered by Ruff’s bandit rules
Code must compile without SyntaxError (compile check)

**/*.py: Use async/await patterns for all I/O operations (GitHub API calls, file operations)
Use type hints for all function parameters and return values
Handle errors gracefully with proper logging (log to stderr)
Pass ruff linting with zero violations
Format code with ruff format
Code must pass static type checks with mypy
Implement retry logic for transient failures (e.g., GitHub API)
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (exclusive create, path validation)
Enforce pagination safety caps and rate limiting when calling GitHub APIs
Ensure code is compatible with Python 3.10+
Run make compile-check to catch import/syntax errors early; code must be syntactically valid
Return meaningful error messages to MCP clients

Files:

• src/mcp_github_pr_review/server.py
• tests/test_graphql_error_handling.py
• tests/test_rate_limit_handling.py

src/mcp_github_pr_review/server.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/mcp_github_pr_review/server.py: All GitHub API interactions must be async/await based
Implement robust error handling for timeouts, API errors, and rate limiting
Enforce pagination safety limits (max pages and max comments)
generate_markdown() must use dynamic markdown fencing to handle nested backticks
get_pr_info() must correctly parse owner, repo, and PR number from GitHub URLs

Files:

• src/mcp_github_pr_review/server.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest as the single test runner with async tests supported by shared fixtures
Avoid live GitHub calls in tests by default; use mocks/fixtures

tests/**/*.py: Name test functions with the test_ prefix
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests

Files:

• tests/test_graphql_error_handling.py
• tests/test_rate_limit_handling.py

tests/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Place test files under tests/ with filename pattern tests/test_*.py

Files:

• tests/test_graphql_error_handling.py
• tests/test_rate_limit_handling.py

🧠 Learnings (1)

📚 Learning: 2025-10-22T20:35:36.888Z

Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T20:35:36.888Z
Learning: Applies to src/mcp_github_pr_review/server.py : Implement robust error handling for timeouts, API errors, and rate limiting

Applied to files:

• src/mcp_github_pr_review/server.py

🧬 Code graph analysis (3)

src/mcp_github_pr_review/server.py (1)

tests/conftest.py (3)

• get (128-136)
• get (430-438)
• json (408-409)

tests/test_graphql_error_handling.py (1)

tests/conftest.py (1)

• github_token (213-218)

tests/test_rate_limit_handling.py (1)

src/mcp_github_pr_review/server.py (6)

• RateLimitHandler (160-334)
• SecondaryRateLimitError (152-157)
• _calculate_backoff_delay (337-348)
• fetch_pr_comments (717-929)
• fetch_pr_comments_graphql (459-714)
• handle_rate_limit (256-334)

🔇 Additional comments (11)

tests/test_graphql_error_handling.py (1)

3-3: LGTM! Logging-based test migration is correct.

The migration from stderr capture (capsys) to logging verification (caplog) is consistent and correct across all test functions. Each test properly:

• Imports the logging module
• Updates function signatures with caplog: pytest.LogCaptureFixture
• Sets appropriate log levels with caplog.set_level(logging.INFO)
• Asserts on caplog.text instead of captured stderr

This aligns with the PR's objective to implement structured logging.

Also applies to: 546-1100

src/mcp_github_pr_review/server.py (6)

4-4: LGTM! Logger initialization follows best practices.

The addition of logging and time modules is appropriate for the new functionality. Logger initialization using logging.getLogger(__name__) follows Python best practices for module-level logging.

Also applies to: 9-9, 47-48

---

140-149: LGTM! Rate limit constants are well-defined.

The constants are appropriately defined:

• SECONDARY_RATE_LIMIT_BACKOFF (60s) is reasonable for abuse detection
• PRIMARY_RATE_LIMIT_MAX_RETRIES (3) provides bounded retry behavior
• SECONDARY_RATE_LIMIT_INDICATORS consolidates magic strings into a reusable constant, improving maintainability

---

152-157: LGTM! Exception class is well-designed.

The SecondaryRateLimitError exception:

• Appropriately inherits from RuntimeError
• Stores the response for debugging
• Has a clear docstring and error message
• Follows Python exception naming conventions

---

160-334: LGTM! RateLimitHandler class is well-implemented.

The RateLimitHandler class effectively encapsulates rate limit logic:

• Clean separation of concerns with private detection/delay methods
• Proper state tracking for secondary and primary retry attempts
• Handles both secondary (abuse) and primary rate limits correctly
• Uses structured logging with contextual metadata
• Bounded retry behavior prevents infinite loops
• Secondary limits: one retry then abort
• Primary limits: respects Retry-After and X-RateLimit-Reset headers with max retry cap

The implementation addresses the previous review concerns about unbounded retries.

---

337-348: LGTM! Backoff cap increase is appropriate.

The exponential backoff cap increased from 5.0s to 15.0s provides better handling for high retry counts while still preventing excessive delays. The function maintains jitter for distributed retry behavior.

---

384-928: LGTM! Comprehensive logging integration.

The structured logging implementation is thorough and well-designed:

• Appropriate log levels (DEBUG/INFO/WARNING/ERROR) for different scenarios
• Structured logging with contextual metadata via extra parameter
• RateLimitHandler properly instantiated and integrated in both GraphQL and REST paths
• SecondaryRateLimitError correctly caught and handled by returning None
• Existing print statements to stderr are acceptable for diagnostic purposes

The integration successfully replaces ad-hoc error reporting with production-grade structured logging.

tests/test_rate_limit_handling.py (4)

1-63: LGTM! Test utilities are well-designed.

The test infrastructure is excellent:

• SleepRecorder captures sleep durations without delays, speeding up tests
• Helper functions (_make_rest_response, _make_graphql_response, _mock_async_client) reduce boilerplate
• Proper use of httpx.Request and httpx.Response for realistic mocking
• Clean separation of concerns between setup utilities and test logic

---

66-261: LGTM! Comprehensive integration test coverage.

The integration tests thoroughly validate rate limit handling:

• REST and GraphQL paths both tested for secondary and primary limits
• Correct backoff delays verified (60s for secondary, respects Retry-After for primary)
• One-retry policy for secondary limits confirmed
• Abort behavior after exhaustion verified
• Backoff cap of 15 seconds validated

Tests use proper mocking, environment setup, and assertions. The SleepRecorder pattern ensures tests run quickly while still verifying timing behavior.

---

266-491: LGTM! Thorough RateLimitHandler unit test coverage.

The unit tests comprehensively verify RateLimitHandler behavior:

• Correctly ignores successful responses
• Detects and handles secondary rate limits (one retry, then exception)
• Handles primary rate limits with header-based backoff (Retry-After, X-RateLimit-Reset)
• Enforces max retry limit for primary rate limits (3 retries)
• Tracks retry counts independently for primary and secondary limits
• Supports custom backoff configuration

Tests use proper mocking (time, sleep) and verify both return values and internal state. This provides confidence in the handler's correctness.

---

493-523: LGTM! Persistent rate limit abort test validates bounded retry behavior.

This test confirms that persistent primary rate limits are handled correctly:

• After PRIMARY_RATE_LIMIT_MAX_RETRIES (3) attempts, the handler stops retrying
• HTTPStatusError is raised to propagate the error to the caller
• Call counts (4 requests, 3 sleeps) match the expected retry logic

This test ensures the system doesn't loop indefinitely when rate limits persist, addressing the concern from previous reviews.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[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 significantly upgrades the application's interaction with the GitHub API by introducing sophisticated rate limit management and structured logging. The core change involves refactoring rate limit detection and retry mechanisms into a dedicated class, allowing for differentiated handling of primary and secondary rate limits. This, combined with detailed, machine-parseable logs, aims to enhance the system's resilience, debuggability, and operational visibility when dealing with GitHub's API constraints.

Highlights

• Enhanced Rate Limit Handling: Implemented distinct logic for GitHub's primary and secondary rate limits, ensuring more robust and compliant API interaction by differentiating between abuse detection and standard rate limits.
• Structured Logging: Introduced production-grade structured logging with contextual metadata for improved observability, debugging, and monitoring of API calls and rate limit events, including GitHub Request IDs for support escalation.
• Refactored Rate Limit Logic: Extracted all rate limit handling into a dedicated RateLimitHandler class, significantly reducing code duplication (by 97 lines) and improving maintainability and testability across both REST and GraphQL API clients.
• Smart Retry Mechanism: Configured a one-retry policy with a 60-second backoff for secondary rate limits and header-based backoff for primary limits, with the exponential backoff cap increased from 5 seconds to 15 seconds.
• Comprehensive Testing: Added 12 new unit tests specifically for the new rate limit handling logic and updated 6 existing tests to leverage structured logging assertions instead of capsys.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

❌ Patch coverage is 95.00000% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/mcp_github_pr_review/server.py	95.00%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[gemini-code-assist]

Code Review

This is an excellent pull request that significantly improves the robustness and observability of GitHub API interactions. The introduction of the RateLimitHandler class is a great architectural change, cleanly separating concerns and removing duplicated code. The distinction between primary and secondary rate limits is handled well, and the move to structured logging is a huge step up from print statements. The new tests are comprehensive and provide good coverage for the new logic. I have a few minor suggestions to further improve the readability and maintainability of the new rate limit handling code, but overall this is a high-quality contribution.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

tests/test_graphql_error_handling.py (1)

545-557: Use caplog.at_level or module-level import to reduce repetition.

You can avoid repeating import logging + caplog.set_level(logging.INFO) in each test by either:

• Using with caplog.at_level(logging.INFO): ... around the assertions, or
• Importing logging once at module scope and calling caplog.set_level(logging.INFO) per test only when necessary.

Assertions against "Reached max_comments limit" look good and align with the server log message.

Also applies to: 631-642, 711-722, 781-791, 861-872, 971-972, 976-977, 1042-1053, 1104-1105

src/mcp_github_pr_review/server.py (2)

329-391: Replace print-based diagnostics with structured logging in retry helper.

Adopt the module logger for consistency and machine-parseable logs.

         except httpx.RequestError as e:
             if attempt < max_retries:
                 delay = _calculate_backoff_delay(attempt)
-                print(
-                    f"Request error: {e}. Retrying in {delay:.2f}s...",
-                    file=sys.stderr,
-                )
+                logger.warning(
+                    "Request error; retrying with backoff",
+                    extra={"error": str(e), "attempt": attempt, "delay_sec": round(delay, 2)},
+                )
                 await asyncio.sleep(delay)
                 attempt += 1
                 continue
             raise
@@
         if 500 <= response.status_code < 600 and attempt < max_retries:
             delay = _calculate_backoff_delay(attempt)
-            print(
-                f"Server error {response.status_code}. Retrying in {delay:.2f}s...",
-                file=sys.stderr,
-            )
+            logger.warning(
+                "Server error; retrying with backoff",
+                extra={
+                    "status_code": response.status_code,
+                    "attempt": attempt,
+                    "delay_sec": round(delay, 2),
+                },
+            )
             await asyncio.sleep(delay)
             attempt += 1
             continue

---

817-838: Unify remaining prints under structured logging.

Prefer logger.debug/info/warning/error with extra=... to complete the migration to structured logs.

Example conversions:

- print(
-     f"Fetched {len(threads)} threads, "
-     f"total comments: {len(all_comments)}",
-     file=sys.stderr,
- )
+ logger.debug(
+     "GraphQL page fetched",
+     extra={"threads": len(threads), "total_comments": len(all_comments)},
+ )

- print(
-    "DEBUG: page_count="
-    f"{page_count}, MAX_PAGES={max_pages_v}, "
-    f"comments_len={len(all_comments)}",
-    file=sys.stderr,
-)
+ logger.debug(
+    "REST pagination progress",
+    extra={
+        "page_count": page_count,
+        "max_pages": max_pages_v,
+        "total_comments": len(all_comments),
+    },
+ )

- print(f"DEBUG: next_url={next_url}", file=sys.stderr)
+ logger.debug("REST next page", extra={"next_url": next_url})

- print(f"Timeout error fetching PR comments: {str(e)}", file=sys.stderr)
+ logger.error("Timeout fetching PR comments", extra={"error": str(e)})

- print(error_msg, file=sys.stderr)
+ logger.error(error_msg, extra={"tool": name})

Also applies to: 642-647, 648-652, 654-661, 851-857, 1122-1125

tests/test_rate_limit_handling.py (1)

1-392: Solid coverage for secondary/primary RL and backoff. Add a persistent-primary case.

Consider adding a test where primary RL repeats N>cap times to assert we stop retrying (after implementing the primary cap).

Example test sketch:

+@pytest.mark.asyncio
+async def test_rest_primary_rate_limit_persistent_aborts(monkeypatch: pytest.MonkeyPatch) -> None:
+    primary = _make_rest_response(
+        403,
+        {"message": "API rate limit exceeded"},
+        headers={"Retry-After": "1", "X-GitHub-Request-Id": "ghi999"},
+    )
+    client = _mock_async_client("get", [primary, primary, primary, primary])
+    with patch("httpx.AsyncClient", return_value=client):
+        recorder = SleepRecorder()
+        monkeypatch.setattr("mcp_github_pr_review.server.asyncio.sleep", recorder)
+        # With PRIMARY_RATE_LIMIT_MAX_RETRIES=3, we should attempt 4 requests total (initial + 3 retries)
+        with pytest.raises(httpx.HTTPStatusError):
+            await fetch_pr_comments("owner", "repo", 123)
+    assert client.get.call_count >= 4
+    assert len(recorder.calls) == 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 fbf8f13 and 64c9bdc.

📒 Files selected for processing (3)

• src/mcp_github_pr_review/server.py (10 hunks)
• tests/test_graphql_error_handling.py (9 hunks)
• tests/test_rate_limit_handling.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use Ruff for formatting and linting; code must be formatted with Ruff and pass Ruff checks
Adhere to line length of 88 characters (Ruff configuration)
Target Python 3.10+ syntax and features
Maintain static typing that passes mypy
Keep imports sorted per Ruff’s import sorting rules
Address security findings covered by Ruff’s bandit rules
Code must compile without SyntaxError (compile check)

**/*.py: Use async/await patterns for all I/O operations (GitHub API calls, file operations)
Use type hints for all function parameters and return values
Handle errors gracefully with proper logging (log to stderr)
Pass ruff linting with zero violations
Format code with ruff format
Code must pass static type checks with mypy
Implement retry logic for transient failures (e.g., GitHub API)
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (exclusive create, path validation)
Enforce pagination safety caps and rate limiting when calling GitHub APIs
Ensure code is compatible with Python 3.10+
Run make compile-check to catch import/syntax errors early; code must be syntactically valid
Return meaningful error messages to MCP clients

Files:

• tests/test_graphql_error_handling.py
• tests/test_rate_limit_handling.py
• src/mcp_github_pr_review/server.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest as the single test runner with async tests supported by shared fixtures
Avoid live GitHub calls in tests by default; use mocks/fixtures

tests/**/*.py: Name test functions with the test_ prefix
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests

Files:

• tests/test_graphql_error_handling.py
• tests/test_rate_limit_handling.py

tests/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Place test files under tests/ with filename pattern tests/test_*.py

Files:

• tests/test_graphql_error_handling.py
• tests/test_rate_limit_handling.py

src/mcp_github_pr_review/server.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/mcp_github_pr_review/server.py: All GitHub API interactions must be async/await based
Implement robust error handling for timeouts, API errors, and rate limiting
Enforce pagination safety limits (max pages and max comments)
generate_markdown() must use dynamic markdown fencing to handle nested backticks
get_pr_info() must correctly parse owner, repo, and PR number from GitHub URLs

Files:

• src/mcp_github_pr_review/server.py

🧠 Learnings (3)

📓 Common learnings

Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T20:35:36.888Z
Learning: Applies to src/mcp_github_pr_review/server.py : Implement robust error handling for timeouts, API errors, and rate limiting

📚 Learning: 2025-10-22T20:35:36.888Z

Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T20:35:36.888Z
Learning: Applies to src/mcp_github_pr_review/server.py : Implement robust error handling for timeouts, API errors, and rate limiting

Applied to files:

• src/mcp_github_pr_review/server.py

📚 Learning: 2025-10-22T20:35:36.888Z

Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T20:35:36.888Z
Learning: Applies to src/mcp_github_pr_review/server.py : Enforce pagination safety limits (max pages and max comments)

Applied to files:

• src/mcp_github_pr_review/server.py

🧬 Code graph analysis (3)

tests/test_graphql_error_handling.py (1)

tests/conftest.py (1)

• github_token (213-218)

tests/test_rate_limit_handling.py (1)

src/mcp_github_pr_review/server.py (6)

• RateLimitHandler (151-312)
• SecondaryRateLimitError (143-148)
• _calculate_backoff_delay (315-326)
• fetch_pr_comments (664-857)
• fetch_pr_comments_graphql (429-661)
• handle_rate_limit (249-312)

src/mcp_github_pr_review/server.py (1)

tests/conftest.py (3)

• get (128-136)
• get (430-438)
• json (408-409)

🔇 Additional comments (5)

src/mcp_github_pr_review/server.py (5)

466-470: LGTM: token requirement now logged.

Clear, structured error on missing GITHUB_TOKEN for GraphQL path.

---

315-327: Backoff cap raised to 15s looks good.

Matches tests; jitter and cap remain predictable and safe.

---

628-635: LGTM: limit-reached info log matches tests.

Message prefix is stable and includes structured fields.

---

736-774: LGTM: REST status handler and auth fallback logging.

Good structured warnings and delegating RL handling to RateLimitHandler.

---

798-802: Verify product intent for conservative 5xx→None behavior and add explicit test coverage.

The concern is valid: the test mocks [Response(500), Response(200, json=[])], demonstrating a scenario where a 5xx error followed by a successful retry should succeed. However, the code at lines 798-802 returns None regardless, discarding any accumulated comments.

The had_server_error flag is set when any 5xx occurs (line 751), persists across pagination, and triggers unconditional None return even after successful retry. This is marked "Conservative behavior" but:

• The specific case (5xx→retry success→return None) lacks explicit test validation
• The docstring mentions "unrecoverable" errors, but successfully retried errors are arguably recoverable
• The test at line 182-188 mocks this exact scenario but doesn't assert the return value

Either add an explicit test validating the None return for this case to confirm intent, or reconsider whether returning successful data with a warning (including retry counts) better serves product expectations.