feat: Add a skill that shows PRs that need to be reviewed#54
feat: Add a skill that shows PRs that need to be reviewed#54
Conversation
The skill will run a script that outputs all the PRs that the user should review for their team. It can then mark those notifications as read.
| if result.returncode != 0: | ||
| print(f"Error fetching team members: {result.stderr}", flush=True) | ||
| sys.exit(1) | ||
| members = [m['login'] for m in json.loads(result.stdout)] |
There was a problem hiding this comment.
Bug: The script incorrectly parses paginated GitHub API responses from gh api --paginate, which will cause a JSONDecodeError when multiple pages of data are returned.
Severity: HIGH
Suggested Fix
Add the --slurp flag to the gh api --paginate calls. This will instruct the GitHub CLI to collect all results from all pages and output them as a single, valid JSON array, which can be correctly parsed by json.loads().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
plugins/sentry-skills/skills/pr-review-notifications/fetch_notifications.py#L25
Potential issue: The code calls the GitHub CLI with `gh api --paginate` to fetch team
members and notifications. The output of this command is then parsed directly with
`json.loads()`. However, when the API returns multiple pages of data, the `gh` CLI
outputs newline-delimited JSON (NDJSON), not a single valid JSON array. This will cause
`json.loads()` to raise a `JSONDecodeError`, crashing the script. This bug will manifest
in common scenarios, such as when an organization has more team members or a user has
more notifications than fit on a single API page (typically 30 items).
Did we get this right? 👍 / 👎 to inform future reviews.
| ['gh', 'api', 'notifications', '--paginate'], | ||
| capture_output=True, text=True | ||
| ) | ||
| all_notifs = json.loads(result.stdout) |
There was a problem hiding this comment.
Bug: The GitHub API call to fetch notifications lacks error handling, which will cause a crash if the API request fails.
Severity: HIGH
Suggested Fix
Check the result.returncode after the subprocess.run call. If it is not 0, handle the error gracefully (e.g., log the error from result.stderr and exit or continue) instead of attempting to parse the output as JSON.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
plugins/sentry-skills/skills/pr-review-notifications/fetch_notifications.py#L32-L35
Potential issue: The `subprocess.run` call to fetch notifications via `gh api
notifications` does not have any error handling. If the command fails for any reason
(e.g., network issue, rate limiting, authentication error), the `returncode` will be
non-zero and `stdout` will contain an error message instead of JSON. The code proceeds
to call `json.loads(result.stdout)` without checking the return code, which will raise a
`JSONDecodeError` and crash the script. This is inconsistent with other API calls in the
same file which do check the return code before parsing the output.
Did we get this right? 👍 / 👎 to inform future reviews.
The skill will run a script that outputs all the PRs that the user should review for their team. It
can then mark those notifications as read.