Skip to content

feat: add affectedFiles to parsed pull_request event#79

Open
hmacr wants to merge 3 commits intomainfrom
ser-401
Open

feat: add affectedFiles to parsed pull_request event#79
hmacr wants to merge 3 commits intomainfrom
ser-401

Conversation

@hmacr
Copy link
Contributor

@hmacr hmacr commented Mar 23, 2026

Related SER-401

@greptile-apps
Copy link

greptile-apps bot commented Mar 23, 2026

Greptile Summary

This PR adds an affectedFiles field to the parsed pull_request webhook event for both GitHub and Gitea adapters, fetching the list of changed files via the respective REST APIs using a new getPullRequestFiles method on each adapter. The feature is well-tested with both unit and integration tests.

  • getPullRequestFiles in GitHub.php uses an unbounded while (true) pagination loop with no maximum page guard, unlike the Gitea implementation which caps at $maxPages = 100. A bug or edge case causing the API to consistently return $perPage items could result in an infinite loop.
  • The GitHub implementation also lacks HTTP status code checking: if the API returns a 4xx/5xx error, the error body is silently merged into the file list and affectedFiles degrades to [] without any signal to the caller — inconsistent with the Gitea implementation which explicitly throws.
  • Both adapters call getPullRequestFiles unconditionally inside getEvent with no try/catch. Since Gitea's implementation throws on HTTP errors, a malformed payload (e.g. missing number field causing (int)''0) will surface as an uncaught exception and crash event parsing entirely. The same risk exists in GitHub if curl-level errors occur.

Confidence Score: 2/5

  • The PR introduces real reliability risks: an infinite loop potential in GitHub pagination and uncaught exceptions that could crash all pull_request webhook processing in both adapters.
  • Two P1 issues — a missing max-page guard in the GitHub pagination loop and unguarded exception propagation in both adapters' getEvent — mean the changes could break webhook handling for all pull request events under certain error conditions. The Gitea implementation is more robust but still shares the getEvent exception risk.
  • src/VCS/Adapter/Git/GitHub.php (infinite loop risk + missing error handling in getPullRequestFiles) and both adapters' getEvent call sites (unguarded getPullRequestFiles invocation).

Important Files Changed

Filename Overview
src/VCS/Adapter/Git/GitHub.php Adds getPullRequestFiles (paginated, but no max-page guard and no HTTP error check) and calls it unconditionally in getEvent for pull_request events — a thrown exception or infinite loop would break all PR webhook processing.
src/VCS/Adapter/Git/Gitea.php Adds getPullRequestFiles with proper HTTP status checking and a $maxPages guard; however, calling it unconditionally in getEvent without a try/catch means an exception on a malformed payload (e.g. missing number) will crash the entire event parse.
src/VCS/Adapter.php Adds the abstract getPullRequestFiles method signature to the base adapter — straightforward addition with no issues.
tests/VCS/Adapter/GitHubTest.php Adds testGetPullRequestFiles and updates testGetEventPullRequest to assert affectedFiles; test assertions look correct and well-targeted.
tests/VCS/Adapter/GiteaTest.php Rewrites testGetEventPullRequest and testGetEventPullRequestExternal to use real API resources and adds testGetPullRequestFiles; tests are more realistic and now assert affectedFiles correctly.
tests/VCS/Base.php Adds testGetPullRequestFiles as a required abstract test method — no issues.

Reviews (1): Last reviewed commit: "feat: add affectedFiles to parsed pull..." | Re-trigger Greptile

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