feat(api): add enterprise GitHub support via env vars (Phase 2)#38
feat(api): add enterprise GitHub support via env vars (Phase 2)#38
Conversation
Enable GitHub Enterprise Server (GHES) support by leveraging existing URL construction utilities from git_pr_resolver.py. The server now dynamically constructs API endpoints based on GH_HOST or explicit URL overrides. Changes: - Make graphql_url_for_host() public in git_pr_resolver.py - Update get_pr_info() to extract host from PR URLs (returns 4-tuple) - Thread host parameter through API call chain - Add GITHUB_API_URL and GITHUB_GRAPHQL_URL env var support - Add comprehensive tests for enterprise URL construction - Document enterprise GitHub configuration in README and CLAUDE.md Tests: - 13 new tests in test_enterprise_url_support.py - Updated existing tests for 4-tuple return value - All 179 tests passing Backward compatible: All changes use smart defaults for github.com. Addresses issue #35 Phase 2 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughPR adds GitHub Enterprise host-awareness: PR URL parsing now returns host; git_pr_resolver exposes host-aware REST/GraphQL URL helpers and host-matching logic; mcp_server threads host through PR comment fetching; .env.example, README, CLAUDE, specs, and tests updated. Defaults preserve github.com behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MCP as mcp_server
participant Resolver as git_pr_resolver
participant Env as Environment
participant GH as GitHub API
User->>MCP: fetch_pr_review_comments(pr_url)
MCP->>MCP: get_pr_info(pr_url)\n=> (host, owner, repo, pr_number)
note right of MCP #EEF5FF: Host threaded into REST & GraphQL flows
MCP->>Resolver: api_base_for_host(host)
Resolver->>Env: check GITHUB_API_URL?
Env-->>Resolver: override or default REST base
Resolver-->>MCP: REST base
MCP->>GH: GET {REST base}/repos/{owner}/{repo}/pulls/{pr}/comments
MCP->>Resolver: graphql_url_for_host(host)
Resolver->>Env: check GITHUB_GRAPHQL_URL / GITHUB_API_URL?
Env-->>Resolver: override or default GraphQL endpoint
Resolver-->>MCP: GraphQL endpoint
MCP->>GH: POST {GraphQL endpoint} query
GH-->>MCP: responses
MCP-->>User: aggregated comments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (6)
Comment |
Summary of ChangesHello @petems, I'm Gemini Code Assist1! 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 enhances the MCP server's flexibility by introducing robust support for GitHub Enterprise Server (GHES). It achieves this by implementing dynamic construction of GitHub API URLs, allowing the server to interact with various GitHub instances beyond github.com. This is facilitated through improved PR URL parsing to extract host information and new environment variables for explicit API endpoint configuration, ensuring seamless operation across different GitHub environments without impacting existing functionality. Highlights
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that thoughtfully implements support for GitHub Enterprise. The changes are well-structured, backward-compatible, and supported by comprehensive documentation and a solid new test suite. Making the URL construction dynamic and configurable through environment variables is a robust solution. I've added one suggestion to improve the maintainability of the new tests by using fixtures to reduce code duplication. Overall, great work on this feature.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @petems. * #38 (comment) The following files were modified: * `git_pr_resolver.py` * `mcp_server.py`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_server.py (1)
169-173: Critical: Prevent SSRF/token exfiltration by restricting PR hosts to GH_HOST (or an allowlist)Current flow extracts host from the PR URL and uses it to build API endpoints, sending Authorization headers. A crafted PR URL like https://evil.example.com/... would send GITHUB_TOKEN to that domain.
Enforce an allowlist:
- Only permit GH_HOST (default github.com). Optionally support a comma-separated GH_ALLOWED_HOSTS.
- Validate early in fetch_pr_review_comments and the API functions.
- Update tests to set GH_HOST (and optionally GH_ALLOWED_HOSTS) when using enterprise hosts.
Apply these diffs:
def get_pr_info(pr_url: str) -> tuple[str, str, str, str]: ... return host, owner, repo, numAdd helpers (outside changed ranges, near other helpers):
# Host allowlist helpers def _allowed_hosts() -> set[str]: env = os.getenv("GH_ALLOWED_HOSTS") base = {os.getenv("GH_HOST", "github.com").lower()} if env: base |= {h.strip().lower() for h in env.split(",") if h.strip()} # Treat api.github.com as equivalent to github.com if "github.com" in base: base.add("api.github.com") return base def _ensure_allowed_host(host: str) -> str: h = host.lower() if h not in _allowed_hosts(): raise ValueError(f"PR host '{host}' is not allowed. Set GH_HOST or GH_ALLOWED_HOSTS.") return hValidate in GraphQL function:
async def fetch_pr_comments_graphql( owner: str, repo: str, pull_number: int, *, - host: str = "github.com", + host: str = "github.com", max_comments: int | None = None, max_retries: int | None = None, ) -> list[CommentResult] | None: + host = _ensure_allowed_host(host)Validate in REST function:
async def fetch_pr_comments( owner: str, repo: str, pull_number: int, *, - host: str = "github.com", + host: str = "github.com", per_page: int | None = None, max_pages: int | None = None, max_comments: int | None = None, max_retries: int | None = None, ) -> list[CommentResult] | None: + host = _ensure_allowed_host(host)Validate in the tool entrypoint:
- host, owner, repo, pull_number_str = get_pr_info(pr_url) + host, owner, repo, pull_number_str = get_pr_info(pr_url) + host = _ensure_allowed_host(host)This blocks token leakage and SSRF while still supporting GHES via GH_HOST/GH_ALLOWED_HOSTS.
Also applies to: 251-256, 1005-1013
🧹 Nitpick comments (4)
specs/PHASE2_ENTERPRISE_GITHUB_URLS.md (2)
232-250: Document an explicit host allowlist to prevent token exfiltrationAccepting any host from a PR URL can cause Authorization headers to be sent to attacker-controlled domains. Add a security note and requirement: only allow requests to GH_HOST (default github.com) or a configured allowlist, and reject PR URLs whose host doesn’t match. Include a brief rationale and a test plan update.
Also applies to: 323-336
329-370: Add language specifiers to fenced code blocks (markdownlint MD040)Several code fences lack language hints. Suggest:
- Shell/env snippets: use bash
- Python code: use python
- Plain URL examples: use text
- Markdown examples: use markdown
Apply edits like:
-```markdown +```markdown ...-
+text
https://{host}/owner/repo/pull/123-``` +```text - https://github.com/owner/repo/pull/123 (GitHub.com) - https://github.enterprise.com/owner/repo/pull/456 (GHES) +https://github.com/owner/repo/pull/123 (GitHub.com) +https://github.enterprise.com/owner/repo/pull/456 (GHES)-
+text
feat(api): add enterprise GitHub support via env vars
...Also applies to: 439-458
mcp_server.py (2)
141-162: Solid host-aware parser; remove assert and consider urlparseParser looks good. Two nits:
- Replace assert with direct unpack (asserts can be stripped with -O).
- Optional: consider urlparse for scheme/host validation or support http(s) if needed later.
Apply:
- groups = match.groups() - assert len(groups) == 4 - host, owner, repo, num = groups[0], groups[1], groups[2], groups[3] - return host, owner, repo, num + g1, g2, g3, g4 = match.groups() + return g1, g2, g3, g4
250-256: Micro-optimization: compute graphql_url once per request, not per pagegraphql_url_for_host(host) doesn’t change across pages. Move it above the pagination loop to avoid recomputing.
- async with httpx.AsyncClient(timeout=timeout, follow_redirects=True) as client: - while has_next_page and len(all_comments) < max_comments_v: + async with httpx.AsyncClient(timeout=timeout, follow_redirects=True) as client: + graphql_url = graphql_url_for_host(host) + while has_next_page and len(all_comments) < max_comments_v: variables = { "owner": owner, "repo": repo, "prNumber": pull_number, "cursor": cursor, } - attempt = 0 - graphql_url = graphql_url_for_host(host) + attempt = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.env.example(1 hunks)CLAUDE.md(1 hunks)README.md(1 hunks)git_pr_resolver.py(2 hunks)mcp_server.py(6 hunks)specs/PHASE2_ENTERPRISE_GITHUB_URLS.md(1 hunks)tests/test_enterprise_url_support.py(1 hunks)tests/test_get_pr_info.py(2 hunks)tests/test_git_pr_resolver.py(3 hunks)tests/test_integration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_get_pr_info.pytests/test_git_pr_resolver.pytests/test_integration.pytests/test_enterprise_url_support.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_get_pr_info.pygit_pr_resolver.pytests/test_git_pr_resolver.pymcp_server.pytests/test_integration.pytests/test_enterprise_url_support.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_get_pr_info.pytests/test_git_pr_resolver.pytests/test_integration.pytests/test_enterprise_url_support.py
mcp_server.py
📄 CodeRabbit inference engine (CLAUDE.md)
mcp_server.py: Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Implement robust error handling for network timeouts, API errors, and rate limiting in GitHub integrations
Enforce pagination safety limits (max pages/comments) when fetching PR comments
Load configuration from environment (.env) with sensible defaults (GITHUB_TOKEN required; PR_FETCH_MAX_PAGES, PR_FETCH_MAX_COMMENTS, HTTP_PER_PAGE, HTTP_MAX_RETRIES optional)
Generate markdown with dynamic code fencing to handle nested backticks in generate_markdown
Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Files:
mcp_server.py
tests/test_integration.py
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests live in tests/test_integration.py and should skip when GITHUB_TOKEN is not set
Files:
tests/test_integration.py
🧠 Learnings (2)
📚 Learning: 2025-10-05T19:32:12.941Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.941Z
Learning: Applies to mcp_server.py : Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Applied to files:
mcp_server.pytests/test_integration.py
📚 Learning: 2025-10-05T19:32:12.941Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.941Z
Learning: Applies to mcp_server.py : Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Applied to files:
mcp_server.py
🧬 Code graph analysis (5)
tests/test_get_pr_info.py (1)
mcp_server.py (1)
get_pr_info(141-161)
tests/test_git_pr_resolver.py (1)
git_pr_resolver.py (1)
graphql_url_for_host(234-269)
mcp_server.py (1)
git_pr_resolver.py (4)
api_base_for_host(109-117)git_detect_repo_branch(61-106)graphql_url_for_host(234-269)resolve_pr_url(120-231)
tests/test_integration.py (1)
mcp_server.py (2)
get_pr_info(141-161)fetch_pr_comments(364-584)
tests/test_enterprise_url_support.py (2)
git_pr_resolver.py (1)
graphql_url_for_host(234-269)mcp_server.py (3)
fetch_pr_comments(364-584)fetch_pr_comments_graphql(164-361)get_pr_info(141-161)
🪛 GitHub Actions: Tests
tests/test_enterprise_url_support.py
[error] 1-1: GraphQL enterprise URL test failing: expected to use enterprise host 'github.enterprise.com' but code called 'https://api.github.com/graphql'.
[error] 1-1: REST enterprise URL test failing: expected 'https://github.enterprise.com/api/v3/repos/...' but code called 'https://api.github.com/repos/owner/repo/pulls/123/comments?per_page=100'.
🪛 LanguageTool
README.md
[grammar] ~98-~98: There might be a mistake here.
Context: ...tructs API endpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - G...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...dpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - GraphQL API: `https://{GH_HOST}/api/grap...
(QB_NEW_EN)
CLAUDE.md
[uncategorized] ~78-~78: The official name of this software platform is spelled with a capital “H”.
Context: ...tHub Variables**: - GH_HOST (default "github.com"): GitHub hostname for enterprise i...
(GITHUB)
specs/PHASE2_ENTERPRISE_GITHUB_URLS.md
[uncategorized] ~14-~14: The official name of this software platform is spelled with a capital “H”.
Context: ... - Returns https://api.github.com for github.com - Returns https://{host}/api/v3...
(GITHUB)
[uncategorized] ~19-~19: The official name of this software platform is spelled with a capital “H”.
Context: ...constructs GraphQL API URLs - Handles github.com → https://api.github.com/graphql ...
(GITHUB)
[grammar] ~53-~53: There might be a mistake here.
Context: ...→ graphql_url_for_host Rationale: - This function is already well-tested thr...
(QB_NEW_EN)
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...ment variable precedence - Handles both github.com and enterprise cases correctly **C...
(GITHUB)
[grammar] ~260-~260: There might be a mistake here.
Context: ...resolver - Verify it's not prefixed with underscore 2. **test_get_pr_info_retu...
(QB_NEW_EN)
[uncategorized] ~263-~263: The official name of this software platform is spelled with a capital “H”.
Context: ...t_get_pr_info_returns_host** - Test github.com URL: https://github.com/owner/repo...
(GITHUB)
[grammar] ~263-~263: There might be a mistake here.
Context: ...turns_host** - Test github.com URL: https://github.com/owner/repo/pull/123` - Test enterprise URL: `https://github.ent...
(QB_NEW_EN)
[grammar] ~264-~264: There might be a mistake here.
Context: ...epo/pull/123 - Test enterprise URL:https://github.enterprise.com/owner/repo/pull/456` - Verify returns 4-tuple: `(host, owner, r...
(QB_NEW_EN)
[grammar] ~272-~272: There might be a mistake here.
Context: ...enterprise_url** - Mock httpx client - Call with host="github.enterprise.com"`...
(QB_NEW_EN)
[grammar] ~273-~273: There might be a mistake here.
Context: ...* - Mock httpx client - Call with host="github.enterprise.com" - Verify POST request goes to `https://git...
(QB_NEW_EN)
[grammar] ~277-~277: There might be a mistake here.
Context: ...enterprise_url** - Mock httpx client - Call with host="github.enterprise.com"`...
(QB_NEW_EN)
[grammar] ~278-~278: There might be a mistake here.
Context: ...* - Mock httpx client - Call with host="github.enterprise.com" - Verify GET request goes to `https://gith...
(QB_NEW_EN)
[uncategorized] ~318-~318: The official name of this software platform is spelled with a capital “H”.
Context: ...es to handle 4-tuple return - Verify github.com still works (regression protection)...
(GITHUB)
[grammar] ~347-~347: There might be a mistake here.
Context: ...tructs API endpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - G...
(QB_NEW_EN)
[grammar] ~348-~348: There might be a mistake here.
Context: ...dpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - GraphQL API: `https://{GH_HOST}/api/grap...
(QB_NEW_EN)
[grammar] ~384-~384: There might be a mistake here.
Context: ...patibility ✅ Zero Breaking Changes: - All new parameters have defaults (`host=...
(QB_NEW_EN)
[uncategorized] ~385-~385: The official name of this software platform is spelled with a capital “H”.
Context: ...**: - All new parameters have defaults (host="github.com") - Existing github.com users requ...
(GITHUB)
[uncategorized] ~386-~386: The official name of this software platform is spelled with a capital “H”.
Context: ...faults (host="github.com") - Existing github.com users require no configuration chan...
(GITHUB)
[grammar] ~395-~395: There might be a mistake here.
Context: ... host from URLs - [ ] GraphQL calls use graphql_url_for_host() - [ ] REST calls use api_base_for_host()...
(QB_NEW_EN)
[grammar] ~396-~396: There might be a mistake here.
Context: ...ql_url_for_host()- [ ] REST calls useapi_base_for_host()` - [ ] Environment variable overrides work ...
(QB_NEW_EN)
[grammar] ~405-~405: There might be a mistake here.
Context: ...isk Level**: Low-Medium Mitigation: - Reusing battle-tested functions from `gi...
(QB_NEW_EN)
[grammar] ~407-~407: There might be a mistake here.
Context: ...rehensive test coverage for new behavior - Defaults ensure backward compatibility -...
(QB_NEW_EN)
[grammar] ~408-~408: There might be a mistake here.
Context: ...- Defaults ensure backward compatibility - Gradual rollout via feature branch ## E...
(QB_NEW_EN)
[grammar] ~413-~413: There might be a mistake here.
Context: ...ementation Time - Code changes: 2 hours - Test development: 2 hours - Documentatio...
(QB_NEW_EN)
[grammar] ~414-~414: There might be a mistake here.
Context: ...ges: 2 hours - Test development: 2 hours - Documentation: 30 minutes - Testing and ...
(QB_NEW_EN)
[grammar] ~415-~415: There might be a mistake here.
Context: ...ent: 2 hours - Documentation: 30 minutes - Testing and validation: 30 minutes - **T...
(QB_NEW_EN)
[grammar] ~416-~416: There might be a mistake here.
Context: ...tes - Testing and validation: 30 minutes - Total: 5 hours ## Implementation Orde...
(QB_NEW_EN)
[grammar] ~424-~424: There might be a mistake here.
Context: ...l_for_hostpublic 4. Update imports inmcp_server.py5. Modifyget_pr_info()` to extract host 6...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
README.md
102-102: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
specs/PHASE2_ENTERPRISE_GITHUB_URLS.md
351-351: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
362-362: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
439-439: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
git_pr_resolver.py (1)
234-234: LGTM: Public API exposure is correct.The rename from
_graphql_url_for_hosttographql_url_for_hostcorrectly exposes this function as part of the public API. The function signature and implementation remain unchanged, maintaining backward compatibility while enabling external modules to construct host-aware GraphQL URLs..env.example (1)
8-20: LGTM: Clear enterprise configuration examples.The added configuration examples provide clear guidance for both GitHub.com (default) and GitHub Enterprise Server deployments. The commented format is appropriate for an example file.
CLAUDE.md (1)
77-86: LGTM: Clear documentation of enterprise configuration.The documentation clearly explains the new enterprise GitHub environment variables and tuning parameters, maintaining consistency with the configuration additions in .env.example.
tests/test_git_pr_resolver.py (1)
533-591: LGTM: Test updates correctly reflect public API.The test updates consistently use the renamed public
graphql_url_for_hostfunction. The test logic remains unchanged, maintaining coverage for enterprise URL construction and environment variable handling.tests/test_get_pr_info.py (2)
15-16: LGTM: Test correctly validates 4-tuple return.The assertion correctly validates the updated
get_pr_infosignature that now returns(host, owner, repo, num)instead of the previous 3-tuple.
37-44: LGTM: Good test coverage for enterprise hosts.The test correctly validates that
get_pr_infoextracts and returns the host from enterprise GitHub URLs, ensuring host-aware URL parsing works for non-GitHub.com domains.tests/test_integration.py (1)
65-66: LGTM: Integration test correctly uses 4-tuple and threads host parameter.The test correctly destructures the 4-tuple from
get_pr_infoand passes the extractedhosttofetch_pr_comments, validating the end-to-end host-aware workflow.tests/test_enterprise_url_support.py (1)
1-298: No changes needed. Implementation correctly uses thehostparameter for enterprise URLs viagraphql_url_for_hostandapi_base_for_host.Likely an incorrect or invalid review comment.
mcp_server.py (2)
23-28: Good reuse of shared helpersImporting api_base_for_host and graphql_url_for_host centralizes URL logic and keeps env precedence consistent.
369-374: LGTM: Host-aware REST endpoint constructionThreading host and using api_base_for_host(host) correctly replaces hardcoded dotcom endpoints and respects env overrides.
Also applies to: 415-419
Docstrings generation was requested by @petems. * #38 (comment) The following files were modified: * `git_pr_resolver.py` * `mcp_server.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Broke up long docstring lines in git_pr_resolver.py - Fixed docstring formatting in mcp_server.py - All ruff checks now pass - All 181 tests passing
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
mcp_server.py (4)
296-318: Increase 5xx retry backoff ceiling to 15s (spec requirement)Spec calls for 15s ceiling; current cap is 5s. Raise cap to 15s for both RequestError and 5xx retries.
Apply this diff:
- delay = min( - 5.0, + delay = min( + 15.0, (0.5 * (2**attempt)) + random.uniform(0, 0.25), # noqa: S311 )- delay = min( - 5.0, + delay = min( + 15.0, (0.5 * (2**attempt)) + random.uniform(0, 0.25), # noqa: S311 )Based on PR objectives
331-334: Handle GraphQL secondary rate limits (60s wait, single retry, log request-id)GraphQL often returns 200 with errors for rate limits. Detect “secondary rate limit” messages, log X‑GitHub‑Request‑Id, wait 60s, then retry once.
Example change within this block:
- if "errors" in data: - print(f"GraphQL errors: {data['errors']}", file=sys.stderr) - return None + if "errors" in data: + # Detect secondary/abuse rate limiting patterns in GraphQL errors + errors = data["errors"] or [] + messages = " ".join(str(e.get("message", "")) for e in errors) + req_id = response.headers.get("X-GitHub-Request-Id", "unknown") + if re.search(r"secondary rate limit|abuse", messages, re.I): + print( + f"GraphQL secondary rate limit detected (req {req_id}); " + "sleeping 60s and retrying once...", + file=sys.stderr, + ) + await asyncio.sleep(60) + # retry once by continuing the outer pagination loop iteration + continue + print(f"GraphQL errors: {errors} (req {req_id})", file=sys.stderr) + return NoneAlso consider factoring a small helper to avoid duplication. Based on PR objectives
510-535: Implement secondary (abuse) rate-limit handling with request-id loggingSpec requires differentiating primary vs secondary rate limits: detect via body message, log X‑GitHub‑Request‑Id, wait 60s, single retry. Current code treats 429/403 uniformly.
Apply this diff to enhance the 429/403 branch:
if response.status_code in (429, 403): - retry_after_header = response.headers.get("Retry-After") + retry_after_header = response.headers.get("Retry-After") remaining = response.headers.get("X-RateLimit-Remaining") reset = response.headers.get("X-RateLimit-Reset") + req_id = response.headers.get("X-GitHub-Request-Id", "unknown") + + # Try to parse a JSON body message; fallback to text + msg = "" + try: + body = response.json() + msg = str(body.get("message") or "") + except Exception: + try: + msg = response.text + except Exception: + msg = "" + + # Secondary/abuse limit: explicit 60s wait, single retry + if re.search(r"secondary rate limit|abuse", msg, re.I): + print( + f"Secondary rate limit detected (req {req_id}); " + "sleeping 60s then retrying once...", + file=sys.stderr, + ) + await asyncio.sleep(60) + # After the sleep, retry current URL immediately + continue if retry_after_header or remaining == "0": retry_after = 60 try: if retry_after_header: retry_after = int(retry_after_header) elif reset: import time now = int(time.time()) retry_after = max(int(reset) - now, 1) except (ValueError, TypeError): retry_after = 60 print( - f"Rate limited. Backing off for {retry_after}s...", + f"Primary rate limit (req {req_id}). " + f"Backing off for {retry_after}s...", file=sys.stderr, ) await asyncio.sleep(retry_after) continueIf you want, I can extract helpers _is_secondary_rate_limit and _log_request_id for reuse across REST/GraphQL. Based on PR objectives
539-551: Raise 5xx backoff ceiling to 15s (spec requirement)Mirror GraphQL’s change here; increase cap from 5s to 15s.
Apply this diff:
- delay = min( - 5.0, + delay = min( + 15.0, (0.5 * (2**attempt)) + random.uniform(0, 0.25), # noqa: S311 )Based on PR objectives
♻️ Duplicate comments (2)
tests/test_enterprise_url_support.py (1)
86-298: Consider refactoring to reduce mock setup duplication.As noted in the past review comment, there's significant duplication in the mock setup across these tests. Consider introducing pytest fixtures for the GraphQL and REST clients to improve maintainability.
This is not blocking but would improve code quality.
README.md (1)
81-120: Duplicate: Formatting issues remain unaddressed.The formatting issues previously flagged in the past review comment have not been resolved:
- Line 102:
**Advanced: Custom API URLs**should be a proper heading (### Advanced: Custom API URLs)- Line 113: Code block missing language identifier (should be
```bash)
🧹 Nitpick comments (3)
specs/PHASE2_ENTERPRISE_GITHUB_URLS.md (3)
1-75: Minor: capitalize “GitHub” consistentlyReplace lowercase “github.com” occurrences in prose with “GitHub.com” and “GitHub” to match style.
329-370: Add languages to fenced code blocks (markdownlint MD040)Specify bash or markdown where appropriate for fenced blocks.
Apply these edits:
- The configuration examples: use ```bash
- The URL format example: use ```text
- The README snippet: use ```markdown
439-458: Add language to commit message fence (MD040)Use ```text for the commit message block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.env.example(1 hunks)CLAUDE.md(1 hunks)README.md(1 hunks)git_pr_resolver.py(3 hunks)mcp_server.py(7 hunks)specs/PHASE2_ENTERPRISE_GITHUB_URLS.md(1 hunks)tests/test_enterprise_url_support.py(1 hunks)tests/test_get_pr_info.py(2 hunks)tests/test_git_pr_resolver.py(3 hunks)tests/test_integration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_integration.pytests/test_get_pr_info.pytests/test_git_pr_resolver.pytests/test_enterprise_url_support.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_integration.pytests/test_get_pr_info.pytests/test_git_pr_resolver.pytests/test_enterprise_url_support.pymcp_server.pygit_pr_resolver.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_integration.pytests/test_get_pr_info.pytests/test_git_pr_resolver.pytests/test_enterprise_url_support.py
tests/test_integration.py
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests live in tests/test_integration.py and should skip when GITHUB_TOKEN is not set
Files:
tests/test_integration.py
mcp_server.py
📄 CodeRabbit inference engine (CLAUDE.md)
mcp_server.py: Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Implement robust error handling for network timeouts, API errors, and rate limiting in GitHub integrations
Enforce pagination safety limits (max pages/comments) when fetching PR comments
Load configuration from environment (.env) with sensible defaults (GITHUB_TOKEN required; PR_FETCH_MAX_PAGES, PR_FETCH_MAX_COMMENTS, HTTP_PER_PAGE, HTTP_MAX_RETRIES optional)
Generate markdown with dynamic code fencing to handle nested backticks in generate_markdown
Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Files:
mcp_server.py
🧠 Learnings (2)
📚 Learning: 2025-10-05T19:32:12.951Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.951Z
Learning: Applies to mcp_server.py : Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Applied to files:
tests/test_integration.pymcp_server.py
📚 Learning: 2025-10-05T19:32:12.951Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.951Z
Learning: Applies to mcp_server.py : Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Applied to files:
mcp_server.py
🧬 Code graph analysis (6)
tests/test_integration.py (1)
mcp_server.py (2)
get_pr_info(141-168)fetch_pr_comments(386-600)
tests/test_get_pr_info.py (1)
mcp_server.py (1)
get_pr_info(141-168)
tests/test_git_pr_resolver.py (1)
git_pr_resolver.py (1)
graphql_url_for_host(250-296)
tests/test_enterprise_url_support.py (2)
git_pr_resolver.py (1)
graphql_url_for_host(250-296)mcp_server.py (3)
fetch_pr_comments(386-600)fetch_pr_comments_graphql(171-383)get_pr_info(141-168)
mcp_server.py (1)
git_pr_resolver.py (4)
api_base_for_host(109-117)git_detect_repo_branch(61-106)graphql_url_for_host(250-296)resolve_pr_url(120-247)
git_pr_resolver.py (2)
tests/test_git_pr_resolver.py (4)
post(475-477)post(518-519)post(625-627)post(654-656)tests/conftest.py (4)
post(134-144)post(436-446)json(404-405)raise_for_status(407-411)
🪛 GitHub Actions: Tests
tests/test_enterprise_url_support.py
[error] 1-1: Pytest failure: fetch_pr_comments_graphql uses enterprise URL but code calls https://api.github.com/graphql instead of https://<enterprise_host>/api/graphql. Update enterprise URL handling to respect host parameter.
[error] 1-1: Pytest failure: fetch_pr_comments_rest uses enterprise URL but REST endpoint constructed as https://api.github.com/repos/... instead of https://<enterprise_host>/api/v3/repos/... . Update enterprise URL handling to respect host parameter.
🪛 LanguageTool
README.md
[grammar] ~98-~98: There might be a mistake here.
Context: ...tructs API endpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - G...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...dpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - GraphQL API: `https://{GH_HOST}/api/grap...
(QB_NEW_EN)
CLAUDE.md
[uncategorized] ~78-~78: The official name of this software platform is spelled with a capital “H”.
Context: ...tHub Variables**: - GH_HOST (default "github.com"): GitHub hostname for enterprise i...
(GITHUB)
specs/PHASE2_ENTERPRISE_GITHUB_URLS.md
[uncategorized] ~14-~14: The official name of this software platform is spelled with a capital “H”.
Context: ... - Returns https://api.github.com for github.com - Returns https://{host}/api/v3...
(GITHUB)
[uncategorized] ~19-~19: The official name of this software platform is spelled with a capital “H”.
Context: ...constructs GraphQL API URLs - Handles github.com → https://api.github.com/graphql ...
(GITHUB)
[grammar] ~53-~53: There might be a mistake here.
Context: ...→ graphql_url_for_host Rationale: - This function is already well-tested thr...
(QB_NEW_EN)
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...ment variable precedence - Handles both github.com and enterprise cases correctly **C...
(GITHUB)
[grammar] ~260-~260: There might be a mistake here.
Context: ...resolver - Verify it's not prefixed with underscore 2. **test_get_pr_info_retu...
(QB_NEW_EN)
[uncategorized] ~263-~263: The official name of this software platform is spelled with a capital “H”.
Context: ...t_get_pr_info_returns_host** - Test github.com URL: https://github.com/owner/repo...
(GITHUB)
[grammar] ~263-~263: There might be a mistake here.
Context: ...turns_host** - Test github.com URL: https://github.com/owner/repo/pull/123` - Test enterprise URL: `https://github.ent...
(QB_NEW_EN)
[grammar] ~264-~264: There might be a mistake here.
Context: ...epo/pull/123 - Test enterprise URL:https://github.enterprise.com/owner/repo/pull/456` - Verify returns 4-tuple: `(host, owner, r...
(QB_NEW_EN)
[grammar] ~272-~272: There might be a mistake here.
Context: ...enterprise_url** - Mock httpx client - Call with host="github.enterprise.com"`...
(QB_NEW_EN)
[grammar] ~273-~273: There might be a mistake here.
Context: ...* - Mock httpx client - Call with host="github.enterprise.com" - Verify POST request goes to `https://git...
(QB_NEW_EN)
[grammar] ~277-~277: There might be a mistake here.
Context: ...enterprise_url** - Mock httpx client - Call with host="github.enterprise.com"`...
(QB_NEW_EN)
[grammar] ~278-~278: There might be a mistake here.
Context: ...* - Mock httpx client - Call with host="github.enterprise.com" - Verify GET request goes to `https://gith...
(QB_NEW_EN)
[uncategorized] ~318-~318: The official name of this software platform is spelled with a capital “H”.
Context: ...es to handle 4-tuple return - Verify github.com still works (regression protection)...
(GITHUB)
[grammar] ~347-~347: There might be a mistake here.
Context: ...tructs API endpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - G...
(QB_NEW_EN)
[grammar] ~348-~348: There might be a mistake here.
Context: ...dpoints based on GH_HOST: - REST API: https://{GH_HOST}/api/v3 - GraphQL API: `https://{GH_HOST}/api/grap...
(QB_NEW_EN)
[grammar] ~384-~384: There might be a mistake here.
Context: ...patibility ✅ Zero Breaking Changes: - All new parameters have defaults (`host=...
(QB_NEW_EN)
[uncategorized] ~385-~385: The official name of this software platform is spelled with a capital “H”.
Context: ...**: - All new parameters have defaults (host="github.com") - Existing github.com users requ...
(GITHUB)
[uncategorized] ~386-~386: The official name of this software platform is spelled with a capital “H”.
Context: ...faults (host="github.com") - Existing github.com users require no configuration chan...
(GITHUB)
[grammar] ~395-~395: There might be a mistake here.
Context: ... host from URLs - [ ] GraphQL calls use graphql_url_for_host() - [ ] REST calls use api_base_for_host()...
(QB_NEW_EN)
[grammar] ~396-~396: There might be a mistake here.
Context: ...ql_url_for_host()- [ ] REST calls useapi_base_for_host()` - [ ] Environment variable overrides work ...
(QB_NEW_EN)
[grammar] ~405-~405: There might be a mistake here.
Context: ...isk Level**: Low-Medium Mitigation: - Reusing battle-tested functions from `gi...
(QB_NEW_EN)
[grammar] ~407-~407: There might be a mistake here.
Context: ...rehensive test coverage for new behavior - Defaults ensure backward compatibility -...
(QB_NEW_EN)
[grammar] ~408-~408: There might be a mistake here.
Context: ...- Defaults ensure backward compatibility - Gradual rollout via feature branch ## E...
(QB_NEW_EN)
[grammar] ~413-~413: There might be a mistake here.
Context: ...ementation Time - Code changes: 2 hours - Test development: 2 hours - Documentatio...
(QB_NEW_EN)
[grammar] ~414-~414: There might be a mistake here.
Context: ...ges: 2 hours - Test development: 2 hours - Documentation: 30 minutes - Testing and ...
(QB_NEW_EN)
[grammar] ~415-~415: There might be a mistake here.
Context: ...ent: 2 hours - Documentation: 30 minutes - Testing and validation: 30 minutes - **T...
(QB_NEW_EN)
[grammar] ~416-~416: There might be a mistake here.
Context: ...tes - Testing and validation: 30 minutes - Total: 5 hours ## Implementation Orde...
(QB_NEW_EN)
[grammar] ~421-~421: There might be a mistake here.
Context: ...tation Order 1. ✅ Create feature branch 2. ✅ Create spec file (this document) 3. Ma...
(QB_NEW_EN)
[grammar] ~422-~422: There might be a mistake here.
Context: ...ch 2. ✅ Create spec file (this document) 3. Make _graphql_url_for_host public 4. U...
(QB_NEW_EN)
[grammar] ~423-~423: There might be a mistake here.
Context: ...) 3. Make _graphql_url_for_host public 4. Update imports in mcp_server.py 5. Mod...
(QB_NEW_EN)
[grammar] ~424-~424: There might be a mistake here.
Context: ...l_for_hostpublic 4. Update imports inmcp_server.py5. Modifyget_pr_info()` to extract host 6...
(QB_NEW_EN)
[grammar] ~425-~425: There might be a mistake here.
Context: .... Modify get_pr_info() to extract host 6. Update fetch_pr_comments_graphql() to ...
(QB_NEW_EN)
[grammar] ~426-~426: There might be a mistake here.
Context: ...r_comments_graphql()to use dynamic URL 7. Updatefetch_pr_comments()` to use dyna...
(QB_NEW_EN)
[grammar] ~427-~427: There might be a mistake here.
Context: ...fetch_pr_comments() to use dynamic URL 8. Thread host parameter through call chain...
(QB_NEW_EN)
[grammar] ~428-~428: There might be a mistake here.
Context: ...Thread host parameter through call chain 9. Write unit tests 10. Update integration ...
(QB_NEW_EN)
[grammar] ~429-~429: There might be a mistake here.
Context: ...r through call chain 9. Write unit tests 10. Update integration tests 11. Update docu...
(QB_NEW_EN)
[grammar] ~430-~430: There might be a mistake here.
Context: ... unit tests 10. Update integration tests 11. Update documentation 12. Run full test s...
(QB_NEW_EN)
[grammar] ~431-~431: There might be a mistake here.
Context: ...tegration tests 11. Update documentation 12. Run full test suite 13. Manual validatio...
(QB_NEW_EN)
[grammar] ~432-~432: There might be a mistake here.
Context: ...te documentation 12. Run full test suite 13. Manual validation with mock enterprise U...
(QB_NEW_EN)
[grammar] ~433-~433: There might be a mistake here.
Context: ...ual validation with mock enterprise URLs 14. Create commit with descriptive message 1...
(QB_NEW_EN)
[grammar] ~434-~434: There might be a mistake here.
Context: .... Create commit with descriptive message 15. Open PR for review ## Commit Message `...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
README.md
102-102: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
specs/PHASE2_ENTERPRISE_GITHUB_URLS.md
351-351: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
362-362: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
439-439: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (17)
.env.example (1)
8-20: LGTM!The enterprise GitHub configuration documentation is clear and comprehensive. The examples correctly demonstrate GitHub.com defaults and GitHub Enterprise Server configuration patterns.
CLAUDE.md (1)
77-81: LGTM!The enterprise GitHub variables documentation correctly describes the new environment configuration options and their defaults.
tests/test_get_pr_info.py (2)
16-16: LGTM!Test correctly updated to validate the new 4-tuple return signature including host extraction.
37-44: LGTM!Good test coverage for enterprise host acceptance. The test validates that arbitrary hosts (like gitlab.com) are accepted, which is essential for GitHub Enterprise Server support.
tests/test_git_pr_resolver.py (1)
533-592: LGTM!Test updates correctly reflect the promotion of
graphql_url_for_hostto a public function. All call sites consistently use the new public name, and the test coverage validates enterprise URL construction patterns.tests/test_integration.py (1)
65-66: LGTM!The integration test correctly demonstrates the end-to-end flow: extracting host from PR URL and propagating it through to the comment-fetching layer.
git_pr_resolver.py (3)
129-161: LGTM!Excellent documentation of the resolve_pr_url function. The docstring clearly describes all parameters, selection strategies, return values, and error conditions.
259-312: LGTM!The graphql_url_for_host function is now properly public with comprehensive documentation. The implementation correctly handles:
- Explicit GITHUB_GRAPHQL_URL override with host matching
- Inference from GITHUB_API_URL
- GitHub.com and GHES defaults
- The api.github.com/github.com equivalence for dotcom
328-344: LGTM!Good documentation of the GraphQL PR lookup helper. The function correctly uses the public graphql_url_for_host for host-aware URL resolution.
tests/test_enterprise_url_support.py (1)
1-298: Implementation correctly uses host parameter in both GraphQL and REST URL construction. Verified thatfetch_pr_comments_graphqlcallsgraphql_url_for_host(host)(yieldinghttps://<enterprise_host>/api/graphql) andfetch_pr_commentscallsapi_base_for_host(host)(yieldinghttps://<enterprise_host>/api/v3), matching the test expectations.Likely an incorrect or invalid review comment.
mcp_server.py (7)
23-28: Correct: importing host-aware URL helpersUsing api_base_for_host and graphql_url_for_host is the right abstraction for GHES support. LGTM.
141-174: get_pr_info now returns host; regex looks solidHost-first 4‑tuple and permissive tail match are correct for GHES URLs. LGTM.
181-209: Good: host threaded into GraphQL fetch APINew host parameter and doc updates align with enterprise support. LGTM.
286-291: Good: dynamic GraphQL endpointSwitch to graphql_url_for_host(host) removes hardcoding and respects env overrides. LGTM.
404-456: Good: REST fetch now host-awareAdding host param and api_base_for_host(host) correctly removes hardcoded dotcom URLs. LGTM.
1017-1057: Docstring updates: clear and accurateDocs now explain host parsing and behavior differences (GraphQL primary path). LGTM.
1077-1085: Correct: parse host and thread through to GraphQLHost extraction and propagation complete the GHES support path. LGTM.
- Add pytest fixtures (mock_graphql_client, mock_rest_client) to eliminate code duplication in tests/test_enterprise_url_support.py - Refactor 6 tests to use fixtures for cleaner, more maintainable test code - Fix README.md formatting: convert bold text to proper heading (line 102) - Add language identifiers to code blocks for proper syntax highlighting - Improve API URL formatting to resolve grammar checker warnings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)tests/test_enterprise_url_support.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_enterprise_url_support.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_enterprise_url_support.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_enterprise_url_support.py
🧬 Code graph analysis (1)
tests/test_enterprise_url_support.py (2)
git_pr_resolver.py (1)
graphql_url_for_host(259-312)mcp_server.py (3)
fetch_pr_comments(399-620)fetch_pr_comments_graphql(176-396)get_pr_info(141-173)
🪛 GitHub Actions: Tests
tests/test_enterprise_url_support.py
[error] 144-144: test_fetch_pr_comments_graphql_uses_enterprise_url failed: GraphQL request URL used the default api.github.com instead of enterprise host; expected https://github.enterprise.com/api/graphql but got https://api.github.com/graphql.
[error] 176-176: test_fetch_pr_comments_rest_uses_enterprise_url failed: REST request URL did not switch to enterprise host; expected URL starting with https://github.enterprise.com/api/v3/repos/ but got https://api.github.com/repos/owner/repo/pulls/123/comments?per_page=100.
🔇 Additional comments (6)
tests/test_enterprise_url_support.py (6)
1-10: LGTM!The imports are well-organized and include all necessary modules for testing enterprise URL support.
12-54: Excellent implementation of the suggested fixtures.The
mock_graphql_clientandmock_rest_clientfixtures effectively address the previous review comment about reducing duplication in test setup. The fixtures properly mockhttpx.AsyncClientfor both GraphQL POST and REST GET scenarios, making the test suite cleaner and more maintainable.
56-61: LGTM!This test appropriately verifies the public API contract, ensuring
graphql_url_for_hostremains accessible and not prefixed with an underscore.
63-128: Comprehensive coverage of URL parsing scenarios.These tests thoroughly validate
get_pr_infobehavior across multiple URL formats (github.com, enterprise, with query parameters, fragments, and trailing paths) and properly test error handling for invalid formats.
219-240: LGTM!The environment variable override tests appropriately verify that
GITHUB_API_URLandGITHUB_GRAPHQL_URLare respected when set, ensuring the precedence rules are correctly implemented.
130-217: fetch_pr_comments_graphql correctly uses the host parameterThe implementation already imports
graphql_url_for_hostand callsgraphql_url_for_host(host)to buildgraphql_url, which is then passed toclient.post. The existing tests will pass as written; no changes required.Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_enterprise_url_support.py (1)
12-53: Consider moving fixtures to conftest.py for broader reuse.While the fixtures work well in this file, consider moving them to
tests/conftest.pyif other test files need to mock httpx.AsyncClient behavior for GraphQL or REST calls. This would make the fixtures available across the entire test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_enterprise_url_support.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_enterprise_url_support.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_enterprise_url_support.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_enterprise_url_support.py
🧬 Code graph analysis (1)
tests/test_enterprise_url_support.py (2)
git_pr_resolver.py (1)
graphql_url_for_host(259-312)mcp_server.py (3)
fetch_pr_comments(399-620)fetch_pr_comments_graphql(176-396)get_pr_info(141-173)
🔇 Additional comments (7)
tests/test_enterprise_url_support.py (7)
12-36: Well-designed fixture reduces test duplication.The
mock_graphql_clientfixture successfully addresses the previous feedback about reducing duplication in mock setup. The fixture properly mocks the async context manager protocol and provides a consistent GraphQL response structure for all tests.
39-53: REST client fixture complements GraphQL fixture well.The
mock_rest_clientfixture mirrors the structure of the GraphQL fixture and properly mocks the REST API client behavior.
56-60: LGTM! Validates public API requirement.The test correctly verifies that
graphql_url_for_hostis publicly accessible as required by Phase 2 specifications.
63-127: Comprehensive coverage of URL parsing scenarios.The test suite thoroughly validates
get_pr_infobehavior across multiple scenarios:
- Standard github.com and enterprise URLs
- URL variations (query parameters, fragments, trailing paths)
- Invalid format error handling
All tests correctly verify the 4-tuple return format
(host, owner, repo, pull_number).
130-158: Environment isolation correctly implemented.These tests properly use
clear=Trueinpatch.dict, addressing the previous feedback about environment variable interference. The tests correctly verify GraphQL URL construction for both enterprise and default github.com hosts.
162-193: REST URL tests correctly isolated and verified.The REST API tests properly use
clear=Truefor environment isolation and correctly verify URL construction using appropriate assertions (startswithandinchecks account for query parameters).
196-240: Environment override tests correctly validate precedence.These tests intentionally use
clear=Falseto verify environment variable override behavior. This is correct because:
- The tests explicitly set the override variables they need to test
- The override variables should take precedence according to the documented precedence rules
- The test assertions correctly verify that custom URLs are used when overrides are set
- Add 'host' parameter to tool schema with comprehensive description - Add descriptions for all parameters (owner, repo, branch, host) - Update tool description to mention host parameter support - Add 5 comprehensive tests for host parameter functionality: * Schema validation test * Parameter descriptions verification test * Explicit host parameter usage test * Git context fallback test * Explicit override test Closes schema-implementation mismatch identified in code review. Test coverage: 187 tests passing (6 new tests added) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/test_enterprise_url_support.py(1 hunks)tests/test_get_pr_info.py(2 hunks)tests/test_integration.py(2 hunks)tests/test_mcp_server_tools.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_mcp_server_tools.pytests/test_get_pr_info.pytests/test_enterprise_url_support.pytests/test_integration.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_mcp_server_tools.pytests/test_get_pr_info.pytests/test_enterprise_url_support.pytests/test_integration.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_mcp_server_tools.pytests/test_get_pr_info.pytests/test_enterprise_url_support.pytests/test_integration.py
tests/test_integration.py
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests live in tests/test_integration.py and should skip when GITHUB_TOKEN is not set
Files:
tests/test_integration.py
🧠 Learnings (2)
📚 Learning: 2025-10-05T19:32:12.951Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.951Z
Learning: Applies to mcp_server.py : Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Applied to files:
tests/test_mcp_server_tools.py
📚 Learning: 2025-10-05T19:32:12.951Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.951Z
Learning: Applies to mcp_server.py : Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Applied to files:
tests/test_get_pr_info.pytests/test_integration.py
🧬 Code graph analysis (4)
tests/test_mcp_server_tools.py (2)
tests/conftest.py (1)
mcp_server(261-265)mcp_server.py (4)
ReviewSpecGenerator(706-1139)fetch_pr_review_comments(1022-1112)handle_list_tools(721-845)handle_call_tool(847-1020)
tests/test_get_pr_info.py (1)
mcp_server.py (1)
get_pr_info(141-173)
tests/test_enterprise_url_support.py (2)
git_pr_resolver.py (1)
graphql_url_for_host(259-312)mcp_server.py (3)
fetch_pr_comments(399-620)fetch_pr_comments_graphql(176-396)get_pr_info(141-173)
tests/test_integration.py (2)
mcp_server.py (2)
get_pr_info(141-173)fetch_pr_comments(399-620)tests/conftest.py (5)
mock_http_client(196-205)sample_pr_comments(272-308)create_mock_response(153-181)add_get_response(106-108)get_calls(115-117)
🪛 GitHub Actions: Tests
tests/test_integration.py
[error] 124-124: End-to-end integration test 'test_custom_host_support' failed: GitHub Enterprise host API base not used; request URL starts with 'https://api.github.com/...' instead of the expected 'https://{enterprise_host}/api/v3'.
🔇 Additional comments (14)
tests/test_get_pr_info.py (2)
16-16: LGTM!The test assertion correctly validates the 4-tuple return value with host extraction for github.com URLs.
37-45: LGTM!The new test effectively validates host-agnostic PR URL parsing for enterprise/alternative GitHub hosts. Individual assertions provide clear failure diagnostics.
tests/test_mcp_server_tools.py (5)
240-289: LGTM!This test comprehensively validates that the git-detected host is correctly propagated through the auto-resolve flow to both
resolve_pr_urlandfetch_pr_comments_graphql.
401-418: LGTM!The test correctly validates that the git-detected host from context is propagated to
resolve_pr_url.
448-486: LGTM!These tests provide valuable validation of the MCP tool schema, ensuring the host parameter is properly documented with meaningful descriptions for API consumers.
488-523: LGTM!The test effectively validates that an explicit host parameter is correctly passed through to
resolve_pr_urlalong with other parameters.
525-591: LGTM!These tests provide critical coverage of host resolution precedence, validating both fallback to git context and explicit override behavior.
tests/test_enterprise_url_support.py (6)
13-40: LGTM!The
create_mock_async_clienthelper effectively reduces test duplication and provides a clean, reusable way to mock HTTP interactions.
42-71: LGTM!The fixtures effectively address the previous review feedback by reducing duplication and providing clean, maintainable test setup.
Based on learnings
73-145: LGTM!These tests provide comprehensive coverage of the public API and URL parsing logic, including edge cases with query parameters, fragments, and trailing paths.
147-182: LGTM!The GraphQL tests correctly validate URL construction for both enterprise hosts and github.com defaults, with proper environment isolation using
clear=True.Based on learnings
185-223: LGTM!The REST API tests correctly validate URL construction for both enterprise hosts and github.com defaults, with proper environment isolation.
225-269: LGTM!These tests effectively validate that environment variable overrides (
GITHUB_GRAPHQL_URLandGITHUB_API_URL) take precedence over host-based URL construction.tests/test_integration.py (1)
65-66: LGTM!The workflow test correctly adapts to the 4-tuple return from
get_pr_infoand properly threads the host parameter through tofetch_pr_comments.
Use collections.abc.Generator instead of typing.Generator following Python 3.9+ best practices (PEP 585). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Eliminate ~40 lines of duplicated retry logic between fetch_pr_comments_graphql() and fetch_pr_comments() by introducing reusable helper functions. Changes: - Add _calculate_backoff_delay(): Extract exponential backoff with jitter - Add _retry_http_request(): Generic async retry wrapper handling RequestError and 5xx errors with optional custom status handling - Refactor fetch_pr_comments_graphql(): Reduce retry code from ~42 lines to ~13 lines using new helper - Refactor fetch_pr_comments(): Extract complex REST-specific logic (401 fallback, rate limiting) into clean status handler callback Benefits: - Single source of truth for retry logic - Easier to maintain and test - Improved code readability - Preserved all existing behavior including conservative server error handling All tests passing (188 passed). Addresses top priority code review finding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_enterprise_url_support.py (1)
227-247: Clarify host matching behavior in test comment.The test comment states "Use api.github.com which is treated as equivalent to github.com" (line 229), but the test passes
host="github.com"(line 239), nothost="api.github.com". Consider updating the comment to clarify that the GITHUB_GRAPHQL_URL usesapi.github.comand the function call usesgithub.com, demonstrating the equivalence.Apply this diff to improve the comment clarity:
- """Test GraphQL function respects GITHUB_GRAPHQL_URL override when hosts match.""" - # Use api.github.com which is treated as equivalent to github.com + """Test GraphQL function respects GITHUB_GRAPHQL_URL override when hosts match. + + GITHUB_GRAPHQL_URL uses api.github.com host, which is treated as + equivalent to github.com when matching. + """ env = { "GITHUB_TOKEN": "test_token", "GITHUB_GRAPHQL_URL": "https://api.github.com/custom/graphql",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcp_server.py(13 hunks)tests/test_enterprise_url_support.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_enterprise_url_support.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_enterprise_url_support.pymcp_server.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_enterprise_url_support.py
mcp_server.py
📄 CodeRabbit inference engine (CLAUDE.md)
mcp_server.py: Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Implement robust error handling for network timeouts, API errors, and rate limiting in GitHub integrations
Enforce pagination safety limits (max pages/comments) when fetching PR comments
Load configuration from environment (.env) with sensible defaults (GITHUB_TOKEN required; PR_FETCH_MAX_PAGES, PR_FETCH_MAX_COMMENTS, HTTP_PER_PAGE, HTTP_MAX_RETRIES optional)
Generate markdown with dynamic code fencing to handle nested backticks in generate_markdown
Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Files:
mcp_server.py
🧠 Learnings (1)
📚 Learning: 2025-10-05T19:32:12.951Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.951Z
Learning: Applies to mcp_server.py : Use async/await for all GitHub API calls in the MCP server (e.g., fetch_pr_comments is async)
Applied to files:
mcp_server.py
🧬 Code graph analysis (2)
tests/test_enterprise_url_support.py (2)
git_pr_resolver.py (1)
graphql_url_for_host(259-312)mcp_server.py (3)
fetch_pr_comments(448-651)fetch_pr_comments_graphql(254-445)get_pr_info(219-251)
mcp_server.py (1)
git_pr_resolver.py (4)
api_base_for_host(109-117)git_detect_repo_branch(61-106)graphql_url_for_host(259-312)resolve_pr_url(120-256)
🔇 Additional comments (14)
tests/test_enterprise_url_support.py (3)
14-41: LGTM! Well-structured test helper.The
create_mock_async_clienthelper provides a clean abstraction for creating mocked async clients with consistent setup. Good use ofsetattrfor dynamic method assignment and proper async context manager protocol.
43-72: LGTM! Fixtures address previous review feedback.The
mock_graphql_clientandmock_rest_clientfixtures effectively reduce test duplication as suggested in the previous review. The fixtures properly mock thehttpx.AsyncClientand set up appropriate responses for GraphQL (POST) and REST (GET) operations.
186-206: No issues found. The test correctly validates REST API base URL construction for enterprise hosts.The
api_base_for_hostimplementation in git_pr_resolver.py (lines 109–117) correctly returns"https://github.enterprise.com/api/v3"for enterprise hosts. Combined with the URL construction in mcp_server.py (lines 500–504), the resulting URL matches the test assertions. Environment isolation withclear=Trueis properly applied.mcp_server.py (11)
23-28: LGTM! Imports correctly expose enterprise URL support.The new imports from
git_pr_resolverproperly expose the public API functions needed for host-aware URL construction and PR resolution. The imports align with the Phase 2 objectives.
140-216: LGTM! Well-structured retry logic with proper backoff.The
_calculate_backoff_delayand_retry_http_requestfunctions implement solid retry patterns:
- Exponential backoff with jitter to prevent thundering herd
- Configurable max_retries parameter
- Support for custom status handlers (rate limiting, auth fallback)
- Proper 5xx retry handling
- Clear error propagation
The 5.0-second cap on backoff is appropriate for client-side retries.
219-251: LGTM! Host extraction properly implemented.The
get_pr_infofunction correctly:
- Extracts host from the PR URL as the first element of the returned tuple
- Updates the error message to reflect the host-aware format
- Maintains backward compatibility by preserving the order (host added as first element)
- Properly validates the URL format with regex
The 4-tuple return
(host, owner, repo, pull_number)aligns with the Phase 2 specification.
254-286: LGTM! GraphQL function signature properly extended.The
fetch_pr_comments_graphqlfunction signature correctly:
- Adds
host: str = "github.com"as a keyword-only parameter- Defaults to github.com for backward compatibility
- Updates docstring to document the new parameter
- Properly threads the host through to URL construction
363-377: LGTM! GraphQL URL construction uses host parameter.The code correctly:
- Uses
graphql_url_for_host(host)to construct the GraphQL endpoint URL (line 363)- Wraps the request in a closure to capture loop variables (lines 366-373)
- Passes the constructed URL to the retry helper (lines 375-377)
This properly implements host-aware GraphQL requests.
448-477: LGTM! REST fetch function signature properly extended.The
fetch_pr_commentsfunction correctly:
- Adds
host: str = "github.com"as a keyword-only parameter- Updates docstring to reflect the REST pagination behavior
- Maintains backward compatibility with default github.com host
500-504: LGTM! REST API base URL uses host parameter.The code correctly:
- Uses
api_base_for_host(host)to construct the REST API base URL (line 500)- Constructs the full URL with the API base and URL-encoded owner/repo (lines 501-504)
This properly implements host-aware REST requests.
523-600: LGTM! REST status handler implements robust error handling.The
handle_rest_statuscallback correctly handles:
- 5xx error tracking for conservative failure behavior (lines 529-531)
- 401 Bearer token fallback to legacy 'token' scheme (lines 533-547)
- Rate limiting with Retry-After and X-RateLimit-Reset headers (lines 549-573)
- Proper async sleep for rate limit backoff
The conservative behavior of returning None if any server error occurred (lines 597-600) ensures reliability.
840-872: LGTM! Tool description accurately reflects host support.The
resolve_open_pr_urltool description correctly:
- Documents the new
hostparameter (lines 865-872)- Explains the fallback behavior (detected from git context or defaults to github.com)
- Maintains clarity in the description
1031-1047: LGTM! Host parameter properly threaded through PR resolution.The code correctly:
- Extracts host from arguments (line 1031)
- Falls back to git context host if not provided (line 1038)
- Passes host to
resolve_pr_url(line 1046)This properly implements host-aware PR URL resolution.
1127-1137: LGTM! 4-tuple unpacking properly implemented.The code correctly:
- Unpacks the 4-tuple from
get_pr_info(pr_url)including host (line 1127)- Passes host to
fetch_pr_comments_graphql(line 1134)- Maintains proper error handling for ValueError
This properly propagates host information through the comment fetching flow.
Wrap fetch_pr_comments call in patch.dict(os.environ, {}, clear=True)
to prevent environment variables like GITHUB_API_URL or
GITHUB_GRAPHQL_URL from overriding the host parameter during test
execution.
This ensures that api_base_for_host(host) correctly uses the provided
enterprise host instead of being overridden by environment configuration.
Fixes flaky test behavior when environment variables are present.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Expand docstring to explicitly explain that GITHUB_GRAPHQL_URL uses api.github.com host while the function call uses github.com host, demonstrating that these two hosts are treated as equivalent during matching. This clarifies the test's behavior where environment variable with api.github.com is correctly matched against a function call with github.com as the host parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Reformat function call to single line per ruff's style preferences. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_integration.py (1)
90-127: Well-structured enterprise host test.The test comprehensively validates GitHub Enterprise support: host parsing, environment isolation, and API URL construction. The environment clearing on line 113 effectively prevents
GITHUB_API_URL/GITHUB_GRAPHQL_URLoverrides.Optional: Consider preserving GITHUB_TOKEN for explicitness.
While the current approach (clearing all env vars) works for mock tests, the past review suggested keeping
GITHUB_TOKENto make it explicit that we're testing host parameter override, not tokenless operation:- with patch.dict(os.environ, {}, clear=True): + with patch.dict(os.environ, {"GITHUB_TOKEN": os.getenv("GITHUB_TOKEN", "test-token")}, clear=True): comments = await fetch_pr_comments(owner, repo, int(pr_number), host=host)This minor change clarifies test intent and aligns with the previous suggestion, though functionally both approaches work for mocked HTTP clients.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_enterprise_url_support.py(1 hunks)tests/test_integration.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_enterprise_url_support.py
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest for all tests, including async tests with shared fixtures in tests/conftest.py; avoid live GitHub calls by default (integration tests are token-gated)
tests/**/*.py: Name test functions test_*
Use pytest-asyncio for async test functions
Mock external dependencies (GitHub API, filesystem) in tests
Files:
tests/test_integration.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
tests/test_integration.py
tests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Place test files under tests/ with filenames matching tests/test_*.py
Files:
tests/test_integration.py
tests/test_integration.py
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests live in tests/test_integration.py and should skip when GITHUB_TOKEN is not set
Files:
tests/test_integration.py
🧠 Learnings (1)
📚 Learning: 2025-10-05T19:32:12.951Z
Learnt from: CR
PR: cool-kids-inc/github-pr-review-mcp-server#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-05T19:32:12.951Z
Learning: Applies to mcp_server.py : Provide a URL parser (get_pr_info) that extracts owner/repo/PR number accurately from GitHub URLs
Applied to files:
tests/test_integration.py
🧬 Code graph analysis (1)
tests/test_integration.py (2)
mcp_server.py (2)
get_pr_info(219-251)fetch_pr_comments(448-651)tests/conftest.py (5)
mock_http_client(196-205)sample_pr_comments(272-308)create_mock_response(153-181)add_get_response(106-108)get_calls(115-117)
🔇 Additional comments (1)
tests/test_integration.py (1)
65-66: LGTM! API update correctly implemented.The test correctly destructures the new 4-tuple return from
get_pr_info()and passes thehostparameter tofetch_pr_comments(), aligning with the Phase 2 enterprise support changes.
|
@gemini-code-assist review |
Update environment isolation to keep GITHUB_TOKEN while clearing other variables. This clarifies test intent: we're testing host parameter override behavior, not tokenless operation. Changes: - Preserve GITHUB_TOKEN from environment or use "test-token" default - Clear all other environment variables to prevent GITHUB_API_URL or GITHUB_GRAPHQL_URL from overriding the host parameter - Add comment explaining the test intent This makes the test more realistic (token is typically present in real usage) and aligns with previous code review feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This is an excellent pull request that thoughtfully implements support for GitHub Enterprise. The refactoring of the HTTP retry logic into a shared, robust helper is a significant improvement to the codebase's maintainability. The changes are well-supported by comprehensive new tests and excellent documentation, including the detailed implementation spec. I have one high-severity finding regarding an inconsistency in how environment variable overrides are handled for REST and GraphQL APIs, which could lead to incorrect behavior in multi-host environments. I've detailed this in a comment on mcp_server.py.
Apply same host-matching logic from graphql_url_for_host to api_base_for_host to ensure GITHUB_API_URL overrides only apply when the environment variable's host matches the target host. Problem: api_base_for_host unconditionally used GITHUB_API_URL if set, regardless of the target host. This caused incorrect routing in multi-host environments (e.g., when GITHUB_API_URL is set for a GHES instance, calls to github.com would be incorrectly routed to GHES). Solution: - Add host-matching logic to api_base_for_host that mirrors graphql_url_for_host behavior - Parse hostname from GITHUB_API_URL and compare to target host - Treat api.github.com and github.com as equivalent for dotcom - Only apply override when hosts match - Add comprehensive docstring explaining the behavior Test Coverage: - Update existing test to use matching hosts - Add new test verifying env override applies when hosts match - Add new test verifying env override is ignored when hosts don't match - Add comprehensive host-matching test with 5 scenarios including github.com/api.github.com equivalence This ensures consistent behavior between REST and GraphQL API base URL resolution and prevents routing bugs in multi-host environments. Fixes #gemini-code-assist-suggestion 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
git_pr_resolver.py (2)
128-142: Moveurlparseimport to module level.The
urlparseimport is used inside this function and again ingraphql_url_for_host(line 313). Moving it to the top of the file alongside other imports improves readability and follows Python conventions.Apply this diff to move the import:
import os import re import sys from dataclasses import dataclass from typing import Any -from urllib.parse import quote +from urllib.parse import quote, urlparse import httpxThen remove the local imports from lines 128 and 313:
- from urllib.parse import urlparse - parsed = urlparse(explicit)
318-322: Consider extracting common host-matching logic.The host-matching logic (treating
api.github.comandgithub.comas equivalent) appears in bothapi_base_for_host(lines 133-142) and here in the nested_hosts_matchfunction. Extracting this to a module-level helper function would reduce duplication.Example extraction:
def _normalize_github_hosts_match(target_host: str, env_api_host: str) -> bool: """Check if target_host and env_api_host are equivalent, treating api.github.com and github.com as the same for dotcom.""" target_lower = target_host.lower() env_lower = env_api_host.lower() if target_lower == "github.com": return env_lower in {"api.github.com", "github.com"} return env_lower == target_lowerThen use it in both functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
git_pr_resolver.py(4 hunks)tests/test_enterprise_url_support.py(1 hunks)tests/test_git_pr_resolver.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_git_pr_resolver.py
- tests/test_enterprise_url_support.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format Python code with Ruff (ruff format) using a line length of 88 characters
Fix lint issues with Ruff (ruff check --fix), including security (bandit) and import sorting rules
Ensure static typing passes with mypy for Python 3.10+
**/*.py: Code must pass ruff linting with zero violations
Code must be formatted with ruff format
Use type hints for all function parameters and return values
Follow async/await patterns for I/O operations (GitHub API calls, file ops)
Handle errors gracefully with proper logging to stderr
Implement retry logic for transient failures honoring HTTP_MAX_RETRIES
Validate all input parameters (URLs, filenames, numeric limits)
Use safe file operations (e.g., exclusive create, path validation) for writing files
Return meaningful error messages to MCP clients
Files:
git_pr_resolver.py
🔇 Additional comments (2)
git_pr_resolver.py (2)
159-191: LGTM!The expanded docstring clearly documents the function's behavior, parameters, return value, and exceptions, including the new
hostparameter for enterprise support.
358-374: LGTM!The docstring clearly documents the function's behavior and parameters, and the call to
graphql_url_for_hostcorrectly uses the now-public API.
Move urlparse import to module level and extract duplicate host-matching logic into _normalize_github_hosts_match helper function. Also fix docstring placement in graphql_url_for_host to comply with Python conventions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
* feat(api): add enterprise GitHub support via env vars Enable GitHub Enterprise Server (GHES) support by leveraging existing URL construction utilities from git_pr_resolver.py. The server now dynamically constructs API endpoints based on GH_HOST or explicit URL overrides. Changes: - Make graphql_url_for_host() public in git_pr_resolver.py - Update get_pr_info() to extract host from PR URLs (returns 4-tuple) - Thread host parameter through API call chain - Add GITHUB_API_URL and GITHUB_GRAPHQL_URL env var support - Add comprehensive tests for enterprise URL construction - Document enterprise GitHub configuration in README and CLAUDE.md Tests: - 13 new tests in test_enterprise_url_support.py - Updated existing tests for 4-tuple return value - All 179 tests passing Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Overview
Implements Phase 2 of the GitHub API Robustness specification (#35) by adding GitHub Enterprise Server (GHES) support through dynamic URL construction and environment variable configuration.
Closes #35 (Phase 2)
What Changed
Core Implementation
Dynamic URL Construction:
graphql_url_for_host()public API ingit_pr_resolver.pyget_pr_info()to extract host from PR URLs (now returns 4-tuple:host, owner, repo, number)hostparameter through entire API call chain (fetch_pr_comments_graphql,fetch_pr_comments,fetch_pr_review_comments)https://api.github.comURLs with dynamic construction viaapi_base_for_host()andgraphql_url_for_host()Environment Variable Support:
GH_HOST- GitHub hostname (default:github.com)GITHUB_API_URL- Explicit REST API base URL overrideGITHUB_GRAPHQL_URL- Explicit GraphQL API URL overrideURL Format Support
The server now accepts PR URLs from any GitHub host:
Examples:
https://github.com/owner/repo/pull/123(GitHub.com)https://github.enterprise.com/owner/repo/pull/456(GHES)Testing
New Tests (13):
test_graphql_url_for_host_is_public- Verify function is public APItest_get_pr_info_returns_host_*- Host extraction from various URL formatstest_fetch_pr_comments_*_uses_enterprise_url- Enterprise URL constructiontest_fetch_pr_comments_*_respects_env_override- Environment variable precedencetest_get_pr_info_accepts_enterprise_hosts- Multi-host supportUpdated Tests:
get_pr_info()test_git_pr_resolver.pyfor renamed public functionTest Results: ✅ All 179 tests passing (0 regressions)
Documentation
Backward Compatibility
✅ 100% backward compatible - All changes use smart defaults:
get_pr_info()extracts host from URL (works with existing github.com URLs)hostparameter defaults to"github.com"in all functionsQuality Checks
All pre-commit checks passing:
Files Changed (10)
git_pr_resolver.py- Public GraphQL URL helpermcp_server.py- Dynamic URL construction and host threading.env.example- Enterprise configuration examplesREADME.md- Enterprise GitHub Support sectionCLAUDE.md- Updated environment configuration docsspecs/PHASE2_ENTERPRISE_GITHUB_URLS.md- Implementation specificationtests/test_enterprise_url_support.py- New test suite (13 tests)tests/test_get_pr_info.py- Updated for 4-tupletests/test_git_pr_resolver.py- Updated function referencestests/test_integration.py- Updated for 4-tupleExample Usage
GitHub.com (no config change)
# Existing configuration works as-is GITHUB_TOKEN=ghp_xxxGitHub Enterprise Server
# Simple configuration GH_HOST=github.enterprise.com GITHUB_TOKEN=your_enterprise_tokenCustom Enterprise Setup
# Advanced configuration for non-standard setups GITHUB_API_URL=https://custom.github.company.com/api GITHUB_GRAPHQL_URL=https://custom.github.company.com/graphql GITHUB_TOKEN=your_tokenNext Steps
This completes Phase 2 of issue #35. Remaining phases:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests