Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an MCP Architecture Review verification plan and implements structured error handling, cursor-based pagination, new Pydantic models, two GitHub PR review tools (list_pr_review_comments, resolve_open_pr_url), pagination helpers, server-side fetch logic, and tests validating schemas and error normalization (≤50 words). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as PRReviewServer
participant GitHub as GitHub API
Client->>Server: handle_call_tool(github_list_pr_review_comments, args)
Server->>Server: validate & normalize args (limit, author, path_prefix, cursor)
alt invalid args
Server->>Client: ToolErrorEnvelopeModel (code, message, next_steps)
else
Server->>Server: if PR URL needed -> call github_resolve_open_pr_url
Server->>GitHub: fetch_pr_comments_page(owner, repo, pull_number, page_size)
GitHub-->>Server: comments + Link header (next URL)
Server->>Server: parse Link header -> encode_cursor(next_url)
Server->>Client: PaginatedReviewCommentsResult(items, nextCursor, total)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/mcp_github_pr_review/models.py (1)
301-305: Redundantserialization_aliaswhenaliasis already set.In Pydantic v2,
aliasapplies to both validation and serialization by default. Since both are set to"nextCursor", theserialization_aliasis redundant.♻️ Simplified field definition
next_cursor: str | None = Field( default=None, alias="nextCursor", - serialization_alias="nextCursor", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_github_pr_review/models.py` around lines 301 - 305, The Field for next_cursor (next_cursor: str | None = Field(..., alias="nextCursor", serialization_alias="nextCursor")) redundantly sets serialization_alias identical to alias; remove the serialization_alias argument and rely on alias (Pydantic v2 uses alias for both validation and serialization) so update the Field call for next_cursor to only include alias="nextCursor".src/mcp_github_pr_review/errors.py (1)
14-14: Consider typingcodeas aLiteralto matchToolErrorDetailsModel.The
codefield is typed asstr, butToolErrorDetailsModel.codeonly accepts specific literal values ("invalid_arguments","auth","not_found","rate_limited","upstream","internal_error"). Passing an invalid code will cause a PydanticValidationErrorat runtime into_model(). Aligning the type here would catch mismatches statically.♻️ Suggested improvement
+from typing import Literal + +ErrorCode = Literal[ + "invalid_arguments", + "auth", + "not_found", + "rate_limited", + "upstream", + "internal_error", +] + `@dataclass`(frozen=True) class ToolExecutionError(Exception): """Actionable error information for canonical MCP tool responses.""" - code: str + code: ErrorCode message: str next_steps: list[str] = field(default_factory=list) def to_model(self) -> ToolErrorDetailsModel: """Convert error details to the normalized response model.""" return ToolErrorDetailsModel( - code=self.code, # type: ignore[arg-type] + code=self.code, message=self.message, next_steps=self.next_steps, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_github_pr_review/errors.py` at line 14, The field "code" is currently annotated as str but must be restricted to the exact set of values used by ToolErrorDetailsModel to avoid runtime Pydantic ValidationError; change the annotation for "code" to a Literal containing "invalid_arguments", "auth", "not_found", "rate_limited", "upstream", and "internal_error" (import Literal from typing), update any constructors/usages that build this error to use those literal values, and ensure the to_model() conversion still maps directly to ToolErrorDetailsModel without casting.tests/test_mcp_arch_review.py (1)
129-129: Misleading return type annotation on test helper.The
failfunction declares-> PaginatedReviewCommentsResultbut always raisesToolExecutionError. While this works becauseAsyncMockuses it correctly, consider usingNoReturnfor clarity.♻️ Suggested fix
+from typing import Any, NoReturn ... - async def fail(*args: Any, **kwargs: Any) -> PaginatedReviewCommentsResult: # noqa: ARG001 + async def fail(*args: Any, **kwargs: Any) -> NoReturn: # noqa: ARG001 raise ToolExecutionError(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_arch_review.py` at line 129, The test helper function fail currently declares a misleading return type PaginatedReviewCommentsResult but always raises ToolExecutionError; update the signature of fail (the async def fail helper) to return typing.NoReturn instead, add an import for NoReturn from typing at the top of the test module, and keep the body raising ToolExecutionError unchanged so static type checkers and readers correctly understand it never returns.src/mcp_github_pr_review/server.py (1)
1922-1930: Placeholder values used whencursoris provided.When a cursor is supplied, the code passes placeholder strings (
"placeholder-owner","placeholder-repo",pull_number=0) tofetch_pr_comments_page. While these are ignored whencursoris valid (since the URL is decoded from the cursor), they would appear in error messages or logs if the cursor is malformed.Consider preserving actual values from the initial request or extracting them from the cursor URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_github_pr_review/server.py` around lines 1922 - 1930, The code calls fetch_pr_comments_page with placeholder values ("placeholder-owner", "placeholder-repo", pull_number=0) when a cursor is provided; replace those placeholders so real context is preserved by passing request_owner, request_repo, and pull_number (the original request values) instead of placeholders, and add a small fallback that attempts to extract owner/repo/pull_number from the decoded cursor URL only if request_* values are empty or None and handle malformed/invalid cursor parsing errors so they don't produce misleading logs; update the call site for fetch_pr_comments_page and the cursor-parsing logic accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/MCP-ARCH-REVIEW.md`:
- Around line 155-166: The spec's module layout (client/, tools/, schemas/,
errors/, pagination/, types/) does not match the actual package layout (e.g.,
modules errors.py, pagination.py, models.py, server.py under
src/mcp_github_pr_review); update the MCP-ARCH-REVIEW.md to either reflect the
current flat structure (listing errors.py, pagination.py, models.py, server.py
and their responsibilities) or explicitly mark the spec as a future-refactor
target and state the intended target layout; while editing, call out two
concrete expectations to preserve: ensure runtime config validation happens once
at startup (reference server.py bootstrap/startup path) and remove/centralize
any duplicate HTTP request logic (search for duplicated request wrappers across
modules such as client-like logic in server.py or models.py) so verifiers know
what to check.
In `@src/mcp_github_pr_review/pagination.py`:
- Around line 16-19: The exception handler around base64.urlsafe_b64decode
should also catch binascii.Error so invalid base64 input is normalized to
ValueError("Invalid pagination cursor"); update the except tuple to include
binascii.Error (e.g., except (ValueError, UnicodeDecodeError, binascii.Error) as
exc) and add an import for binascii at the top of the module so the symbol is
available; keep the raised ValueError from the same block and use the existing
exc variable for exception chaining.
In `@src/mcp_github_pr_review/server.py`:
- Around line 1943-1969: The current code applies
include_resolved/author/path_prefix filtering after a single fetch which breaks
pagination guarantees (variables: comments, filtered_comments, include_resolved,
author, path_prefix, next_cursor, limit, ReviewCommentModel,
PaginatedReviewCommentsResult); change the logic to perform server-side
accumulation: keep fetching subsequent GitHub pages (using the same fetch loop
that produced comments and next_cursor) and apply the filters as you accumulate
until you have limit matching items or a max_fetch_pages cap to avoid unbounded
fetching, then convert those accumulated filtered items via
ReviewCommentModel.model_validate and return them with next_cursor set to the
actual next page token and total reflecting the count of matching items (or a
consistent sentinel if you cannot compute full total). Ensure the fetch loop and
pagination token handling are updated to reference the same next_cursor used in
the external API call.
---
Nitpick comments:
In `@src/mcp_github_pr_review/errors.py`:
- Line 14: The field "code" is currently annotated as str but must be restricted
to the exact set of values used by ToolErrorDetailsModel to avoid runtime
Pydantic ValidationError; change the annotation for "code" to a Literal
containing "invalid_arguments", "auth", "not_found", "rate_limited", "upstream",
and "internal_error" (import Literal from typing), update any
constructors/usages that build this error to use those literal values, and
ensure the to_model() conversion still maps directly to ToolErrorDetailsModel
without casting.
In `@src/mcp_github_pr_review/models.py`:
- Around line 301-305: The Field for next_cursor (next_cursor: str | None =
Field(..., alias="nextCursor", serialization_alias="nextCursor")) redundantly
sets serialization_alias identical to alias; remove the serialization_alias
argument and rely on alias (Pydantic v2 uses alias for both validation and
serialization) so update the Field call for next_cursor to only include
alias="nextCursor".
In `@src/mcp_github_pr_review/server.py`:
- Around line 1922-1930: The code calls fetch_pr_comments_page with placeholder
values ("placeholder-owner", "placeholder-repo", pull_number=0) when a cursor is
provided; replace those placeholders so real context is preserved by passing
request_owner, request_repo, and pull_number (the original request values)
instead of placeholders, and add a small fallback that attempts to extract
owner/repo/pull_number from the decoded cursor URL only if request_* values are
empty or None and handle malformed/invalid cursor parsing errors so they don't
produce misleading logs; update the call site for fetch_pr_comments_page and the
cursor-parsing logic accordingly.
In `@tests/test_mcp_arch_review.py`:
- Line 129: The test helper function fail currently declares a misleading return
type PaginatedReviewCommentsResult but always raises ToolExecutionError; update
the signature of fail (the async def fail helper) to return typing.NoReturn
instead, add an import for NoReturn from typing at the top of the test module,
and keep the body raising ToolExecutionError unchanged so static type checkers
and readers correctly understand it never returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86456c05-736e-4278-a94b-81ae36f8eb79
📒 Files selected for processing (6)
specs/MCP-ARCH-REVIEW.mdsrc/mcp_github_pr_review/errors.pysrc/mcp_github_pr_review/models.pysrc/mcp_github_pr_review/pagination.pysrc/mcp_github_pr_review/server.pytests/test_mcp_arch_review.py
* docs(readme): document pre-commit hook install with global core.hooksPath * docs(claude): add pre-commit hook installation notes * chore: normalize formatting across docs workflows and tests * docs(readme): replace absolute hook paths with repo-relative examples * docs(claude): replace absolute hook paths with placeholders * style(tests): apply ruff formatting in git PR resolver tests * style(tests): apply ruff formatting in MCP tool schema tests * style(tests): apply ruff formatting in pagination limit tests * style(tests): apply ruff formatting in performance tests
…ms/mcp-arch-audit # Conflicts: # specs/MCP-ARCH-REVIEW.md # src/mcp_github_pr_review/errors.py # src/mcp_github_pr_review/models.py # src/mcp_github_pr_review/pagination.py # src/mcp_github_pr_review/server.py # tests/test_mcp_arch_review.py
Contributor
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)
src/mcp_github_pr_review/server.py (1)
1655-1701:⚠️ Potential issue | 🟠 MajorDo not force branch/git detection for
latestorfirststrategies.Current logic requires
branch(or git context) even when strategy does not need it. This makesgithub_resolve_open_pr_url/list flows fail unnecessarily outside a local git checkout whenowner+repoare already provided.✅ Strategy-aware fix pattern
- if not (owner and repo and branch): + needs_branch = select_strategy in {"branch", "error"} + if not (owner and repo and (branch or not needs_branch)): ctx = git_detect_repo_branch() owner = owner or ctx.owner repo = repo or ctx.repo - branch = branch or ctx.branch + if needs_branch: + branch = branch or ctx.branch host = host or ctx.hostAlso applies to: 1894-1912
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_github_pr_review/server.py` around lines 1655 - 1701, The code forces git_detect_repo_branch() to fill owner/repo/branch even when select_strategy does not require branch (e.g., "latest" or "first"), causing github_resolve_open_pr_url/list flows to fail outside a repo; update the logic after ResolveOpenPrUrlArgs.model_validate so that you only call git_detect_repo_branch() or require branch when select_strategy is one that needs it (reference the select_strategy variable and ResolveOpenPrUrlArgs validation), e.g., check select_strategy value before invoking git_detect_repo_branch() and only populate branch from git context when necessary; apply the same conditional fix in the analogous block around the code handling select_strategy later (the other resolve/list handler).
🧹 Nitpick comments (1)
tests/test_mcp_arch_review.py (1)
208-260: Add a hostile-cursor regression test.Please add a case where
cursordecodes to a non-GitHub/non-API URL and assert the tool returnsinvalid_arguments(and does not perform fetch). This will lock in cursor safety behavior.As per coding guidelines, "Test error conditions and edge cases in addition to happy path scenarios".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_arch_review.py` around lines 208 - 260, Add a hostile-cursor regression test alongside test_list_pr_review_comments_uses_cursor_context_without_placeholders that calls mcp_server.list_pr_review_comments with a cursor which _extract_cursor_pr_context is monkeypatched to decode into a non-GitHub host (e.g., "evil.com", owner/repo/pull_number values). Also monkeypatch fetch_pr_comments_page to a function that raises if invoked to ensure no fetch is attempted; then assert the RPC result indicates invalid_arguments (or equivalent error) and that fetch_pr_comments_page was not called. Use the same symbols: mcp_server.list_pr_review_comments, _extract_cursor_pr_context, and fetch_pr_comments_page to locate and implement the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp_github_pr_review/server.py`:
- Around line 1019-1038: The code trusts decode_cursor(cursor) and then issues
an authenticated fetch; instead validate any decoded URL before using it in
fetch_pr_comments_page: after calling decode_cursor (and in the similar decode
usage at the other spot), parse the URL and ensure its scheme is https, the
hostname equals api_base_for_host(host) (or the expected host derived from
host), and the path matches the expected prefix for this endpoint (e.g., starts
with /repos/{owner}/{repo}/pulls/{pull_number}/comments) and contains the
per_page parameter; if validation fails, raise the same ToolExecutionError for
invalid_arguments. Use decode_cursor, fetch_pr_comments_page and
api_base_for_host as anchors to find where to add the checks.
---
Outside diff comments:
In `@src/mcp_github_pr_review/server.py`:
- Around line 1655-1701: The code forces git_detect_repo_branch() to fill
owner/repo/branch even when select_strategy does not require branch (e.g.,
"latest" or "first"), causing github_resolve_open_pr_url/list flows to fail
outside a repo; update the logic after ResolveOpenPrUrlArgs.model_validate so
that you only call git_detect_repo_branch() or require branch when
select_strategy is one that needs it (reference the select_strategy variable and
ResolveOpenPrUrlArgs validation), e.g., check select_strategy value before
invoking git_detect_repo_branch() and only populate branch from git context when
necessary; apply the same conditional fix in the analogous block around the code
handling select_strategy later (the other resolve/list handler).
---
Nitpick comments:
In `@tests/test_mcp_arch_review.py`:
- Around line 208-260: Add a hostile-cursor regression test alongside
test_list_pr_review_comments_uses_cursor_context_without_placeholders that calls
mcp_server.list_pr_review_comments with a cursor which
_extract_cursor_pr_context is monkeypatched to decode into a non-GitHub host
(e.g., "evil.com", owner/repo/pull_number values). Also monkeypatch
fetch_pr_comments_page to a function that raises if invoked to ensure no fetch
is attempted; then assert the RPC result indicates invalid_arguments (or
equivalent error) and that fetch_pr_comments_page was not called. Use the same
symbols: mcp_server.list_pr_review_comments, _extract_cursor_pr_context, and
fetch_pr_comments_page to locate and implement the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82809ae0-5600-4cb6-a268-81ffc802d9c4
📒 Files selected for processing (17)
.github/dependabot.yml.github/workflows/lint.yml.github/workflows/tests.yml.gitignoreCLAUDE.mdMakefileREADME.mdSECURITY.mdTEST_COVERAGE_SUMMARY.mdUV_COMMANDS.mdspecs/MCP-ARCH-REVIEW.mdspecs/PYDANTIC_MODEL_INTEGRATION.mdsrc/mcp_github_pr_review/errors.pysrc/mcp_github_pr_review/models.pysrc/mcp_github_pr_review/pagination.pysrc/mcp_github_pr_review/server.pytests/test_mcp_arch_review.py
💤 Files with no reviewable changes (2)
- Makefile
- specs/PYDANTIC_MODEL_INTEGRATION.md
✅ Files skipped from review due to trivial changes (8)
- README.md
- .github/workflows/tests.yml
- TEST_COVERAGE_SUMMARY.md
- .gitignore
- .github/dependabot.yml
- SECURITY.md
- UV_COMMANDS.md
- .github/workflows/lint.yml
Comment on lines
+1019
to
+1038
| if cursor: | ||
| try: | ||
| url = decode_cursor(cursor) | ||
| except ValueError as exc: | ||
| raise ToolExecutionError( | ||
| code="invalid_arguments", | ||
| message="The provided cursor is invalid.", | ||
| next_steps=[ | ||
| "Use the nextCursor value returned by a previous response.", | ||
| "Restart pagination with cursor omitted.", | ||
| ], | ||
| ) from exc | ||
| else: | ||
| safe_owner = quote(owner, safe="") | ||
| safe_repo = quote(repo, safe="") | ||
| api_base = api_base_for_host(host) | ||
| url = ( | ||
| f"{api_base}/repos/{safe_owner}/{safe_repo}/pulls/{pull_number}" | ||
| f"/comments?per_page={per_page}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Validate decoded cursor URLs before authenticated fetches.
fetch_pr_comments_page currently trusts decode_cursor(cursor) and then sends an authenticated request to that URL. A forged cursor can redirect requests (with Authorization) to attacker-controlled hosts.
🔒 Suggested hardening
@@
if cursor:
try:
- url = decode_cursor(cursor)
+ decoded_url = decode_cursor(cursor)
except ValueError as exc:
raise ToolExecutionError(
code="invalid_arguments",
message="The provided cursor is invalid.",
@@
- ) from exc
+ ) from exc
+
+ parsed = urlparse(decoded_url)
+ expected_host = (urlparse(api_base_for_host(host)).netloc or "").lower()
+ path_match = re.match(
+ r"^/repos/([^/]+)/([^/]+)/pulls/(\d+)/comments$",
+ parsed.path,
+ )
+ if (
+ parsed.scheme != "https"
+ or parsed.netloc.lower() != expected_host
+ or not path_match
+ ):
+ raise ToolExecutionError(
+ code="invalid_arguments",
+ message="The provided cursor is invalid.",
+ next_steps=[
+ "Use only nextCursor values returned by this tool.",
+ "Restart pagination with cursor omitted.",
+ ],
+ )
+ url = decoded_url
else:Also applies to: 1065-1067
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp_github_pr_review/server.py` around lines 1019 - 1038, The code trusts
decode_cursor(cursor) and then issues an authenticated fetch; instead validate
any decoded URL before using it in fetch_pr_comments_page: after calling
decode_cursor (and in the similar decode usage at the other spot), parse the URL
and ensure its scheme is https, the hostname equals api_base_for_host(host) (or
the expected host derived from host), and the path matches the expected prefix
for this endpoint (e.g., starts with
/repos/{owner}/{repo}/pulls/{pull_number}/comments) and contains the per_page
parameter; if validation fails, raise the same ToolExecutionError for
invalid_arguments. Use decode_cursor, fetch_pr_comments_page and
api_base_for_host as anchors to find where to add the checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the MCP architecture review spec by adding canonical read-only MCP tools with strict schemas, annotations, structured outputs, and normalized actionable error envelopes while preserving legacy tool compatibility.
The server now supports cursor-based single-page comment listing (
github_list_pr_review_comments) and structured PR URL resolution (github_resolve_open_pr_url), backed by new pagination and error helper modules.The changes also add strict Pydantic models for tool I/O, list response envelope fields (
items,nextCursor,total), and filtering support (author,path_prefix,include_resolved).Coverage includes a new contract-focused test suite plus full quality validation (
ruff format,ruff check --fix,mypy,compile-check,pytest) passing in this branch.Summary by CodeRabbit
New Features
Documentation
Tests