Skip to content

fix: do not treat every GitHub message as a missing pull request - #3194

Open
SebTardif wants to merge 1 commit into
sherlock-project:masterfrom
SebTardif:fix/pull-request-manifest-status
Open

SebTardif wants to merge 1 commit into
sherlock-project:masterfrom
SebTardif:fix/pull-request-manifest-status

Conversation

@SebTardif

Copy link
Copy Markdown

Summary

--json with a pull number no longer treats every GitHub message field as a missing pull request.

Problem

The pull-request fetch ignored the HTTP status. Any JSON object with a message key exited as "Pull request #N not found." GitHub uses that field for rate limits and authentication failures, so a pull request that exists was reported as missing.

Change

pull_request_manifest_url() in sherlock_project/sherlock.py uses the status code. HTTP 404 is not found. Any other non-200 is a fetch failure, and the printed error includes GitHub's message. A 200 response still returns the raw data.json URL from head.sha, including when the body also has a message field.

Validation

tests/test_pull_request_manifest.py failed on the old check: a 403 rate-limit body and a 200 body that contained message were both treated as missing. After the change those four tests pass.

--json with a pull number treated any JSON message field as
"pull request not found". A rate-limit or authentication response
then hid a pull request that exists. HTTP 404 is the not-found
case. Any other non-200 status is a fetch failure.

Tests cover 404, a 403 rate-limit body, a normal 200, and a 200
body that also contains a message field.
@SebTardif
SebTardif requested a review from ppfeister as a code owner November 8, 2026 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant