Skip to content

Conversation

@mcw0933
Copy link

@mcw0933 mcw0933 commented May 19, 2025

Description of issue

The Github API requires start_side on multi-line review comments: https://docs.github.com/en/rest/pulls/comments#create-a-review-comment-for-a-pull-request

If you do not pass this param, and you comment on a multi-line range of deleted code (LEFT, n, n+1) you get a 422 validation error with a misleading error message:

{
    "message": "Validation Failed",
    "errors": [
        {
            "resource": "PullRequestReviewComment",
            "code": "custom",
            "field": "pull_request_review_thread.start_line",
            "message": "pull_request_review_thread.start_line must precede the end line."
        }
    ],
    "documentation_url": "https://docs.github.com/rest/pulls/comments#create-a-review-comment-for-a-pull-request",
    "status": "422"
}

This misleads the user into thinking the problem is with the start_line and line params when it is in fact the absent start_side that is the issue.

Demonstration of issue:

Script to test issue against this PR
#!/usr/bin/env -S uv run --script
# /// script
# requires-python = ">=3.8"
# dependencies = [
#   "PyGithub",
# ]
# ///
import sys
import subprocess
import github
from github.GithubException import GithubException
import argparse

#Get token from GitHub CLI
def get_github_token():
    try:
        token = subprocess.check_output(["gh", "auth", "token"]).decode("utf-8").strip()
        if not token:
            raise RuntimeError("No token returned from 'gh auth token'")
        return token
    except Exception as e:
        print("Error getting GitHub token via 'gh auth token':", e)
        sys.exit(1)

def main():
    parser = argparse.ArgumentParser(description="Reproduce GitHub 422 error for multiline review comment without start_side.")
    parser.add_argument("--repo", required=True, help="Repository in the form owner/repo")
    parser.add_argument("--pr", type=int, required=True, help="Pull request number")
    parser.add_argument("--commit", required=True, help="Commit SHA")
    parser.add_argument("--file", required=True, help="File path in the repo")
    parser.add_argument("--side", required=True, choices=["LEFT", "RIGHT"], help="Side for the comment")
    parser.add_argument("--start-line", type=int, required=True, help="Start line for the multiline comment")
    parser.add_argument("--line", type=int, required=True, help="End line for the multiline comment")
    args = parser.parse_args()

    token = get_github_token()
    g = github.Github(token)
    repo = g.get_repo(args.repo)
    pr = repo.get_pull(args.pr)
    commit = repo.get_commit(args.commit)

    try:
        pr.create_review_comment(
            body="This should fail with 422 if start_side is not set for a multi-line comment.",
            commit=commit,
            path=args.file,
            side=args.side,
            start_line=args.start_line,
            line=args.line,
            # start_side is intentionally omitted
        )
        raise AssertionError("Expected a 422 Unprocessable Entity error from GitHub, but comment was created successfully!")
    except GithubException as e:
        print(f"Status: {e.status}")
        print(f"Data: {e.data}")
        assert e.status == 422, f"Expected 422, got {e.status}"
        print("Received expected 422 error from GitHub when start_side is not set for a multi-line comment.")

if __name__ == "__main__":
    main()

Invoke with:

./failing_multiline_review_comment.py \
  --repo PyGithub/PyGithub \
  --pr 3294 \
  --commit 158652830bbcd4096ea011651a818242bf558de8 \
  --file github/PullRequest.py \
  --side LEFT \
  --start-line 131 \
  --line 132

Proposed fix

Add a validation method on review comment fields to check for the proper combination of fields for multi-line comments.
Optimistically provide start_side when possible to help things succeed.
Assert with a better failure message when not possible to succeed.

@mcw0933 mcw0933 marked this pull request as ready for review May 19, 2025 18:41
@mcw0933
Copy link
Author

mcw0933 commented May 19, 2025

Hey - I'm having trouble locally getting tests to run. Trying to follow the instructions in CONTRIBUTING.md but after installing all of the stuff in /requirements I'm still getting:

~> /Users/matt/.local/share/uv/tools/pip/bin/pytest
ImportError while loading conftest '/Users/matt/src/pygithub/tests/conftest.py'.
tests/conftest.py:36: in <module>
    from . import Framework
tests/Framework.py:73: in <module>
    import github
github/__init__.py:54: in <module>
    from . import Auth
github/Auth.py:42: in <module>
    from github.InstallationAuthorization import InstallationAuthorization
github/InstallationAuthorization.py:47: in <module>
    import github.NamedUser
github/NamedUser.py:62: in <module>
    import github.Event
github/Event.py:48: in <module>
    import github.Organization
github/Organization.py:93: in <module>
    import github.CodeSecurityConfigRepository
github/CodeSecurityConfigRepository.py:26: in <module>
    import github.Repository
github/Repository.py:189: in <module>
    import github.Environment
github/Environment.py:52: in <module>
    from github.PublicKey import PublicKey
github/PublicKey.py:48: in <module>
    from nacl import encoding, public
E   ModuleNotFoundError: No module named 'nacl'

and trying

~> uv run pip install nacl
ERROR: Could not find a version that satisfies the requirement nacl (from versions: none)
ERROR: No matching distribution found for nacl

isn't working either.

@mcw0933
Copy link
Author

mcw0933 commented May 20, 2025

Also, if anyone from this project has pro-tips for getting the upstream issue fixed in Github API (probably lolwutno after the layoffs 😢), that feedback is welcome. Looks like they have a mistaken early exit in their review comment validation routine.

@mcw0933
Copy link
Author

mcw0933 commented May 20, 2025

@github-actions github-actions bot added the ⭐ top pull request Top pull request. label May 21, 2025

def test_validate_review_comment_start_side_start_side_already_defined(self):
# start_side already defined
result = self.pull.validate_review_comment_start_side("LEFT", "LEFT", 1, 1, github.GithubObject.NotSet)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a different value for start_side here so we see that start_side does not get modified if given?

comment.get("start_side", NotSet),
comment.get("line", NotSet),
comment.get("start_line", NotSet),
comment.get("in_reply_to", NotSet),
Copy link
Collaborator

@EnricoMi EnricoMi Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments when creating a review do not contain the in_reply_to attribute: https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#create-a-review-for-a-pull-request

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
comment.get("in_reply_to", NotSet),
NotSet,

@EnricoMi EnricoMi changed the title fix(ReviewComment): add validation to avoid misleading error message from Github API Add validation to creating review with comments to avoid misleading Github API error message Aug 13, 2025
@github-actions
Copy link

github-actions bot commented Aug 13, 2025

Test Results

     8 files  ± 0       8 suites  ±0   4m 50s ⏱️ +7s
 1 030 tests + 5   1 030 ✅ + 5  0 💤 ±0  0 ❌ ±0 
11 315 runs  +40  11 314 ✅ +40  1 💤 ±0  0 ❌ ±0 

Results for commit ed9b72b. ± Comparison against base commit 17bc4df.

♻️ This comment has been updated with latest results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants