From 0b0fcf999eff45cf95d8862d8c837f1372ec79fe Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Mon, 17 Aug 2026 08:50:36 +0530 Subject: [PATCH 1/2] fix: report a provider failure when reading pull request files getPullRequestFiles() assumed every response body was a list of file records. GitHub checked no status at all, so an authorization, rate limit or gateway response reached array_merge() and raised a TypeError; Gitea and Forgejo checked the status but not the body, and raised the same TypeError on a non-JSON body. Both surfaced as an unexpected 500 rather than a controlled failure. Reading an error payload as a file list is the quieter half of this. GitLab turned one into a single diff with an empty path, and Bitbucket returned no files at all, so a failed fetch was indistinguishable from a pull request that changed nothing. Check the status before the body on GitHub, and require a list of file records everywhere before merging one. Pagination is unchanged. Base now drives these paths from canned responses, since a live forge cannot be made to page or fail on demand. Adapters describe their own page shape; Gogs opts out through the pull request lookup flag it already sets. --- src/VCS/Adapter/Git/Bitbucket.php | 4 +- src/VCS/Adapter/Git/GitHub.php | 10 ++ src/VCS/Adapter/Git/GitLab.php | 6 +- src/VCS/Adapter/Git/Gitea.php | 4 + tests/VCS/Adapter/BitbucketTest.php | 26 +++++ tests/VCS/Adapter/GitHubTest.php | 2 + tests/VCS/Adapter/GitLabTest.php | 24 +++++ tests/VCS/Base.php | 157 ++++++++++++++++++++++++++++ 8 files changed, 230 insertions(+), 3 deletions(-) diff --git a/src/VCS/Adapter/Git/Bitbucket.php b/src/VCS/Adapter/Git/Bitbucket.php index 95f2eff8..6e28ad09 100644 --- a/src/VCS/Adapter/Git/Bitbucket.php +++ b/src/VCS/Adapter/Git/Bitbucket.php @@ -1154,12 +1154,12 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $ $responseBody = $response['body'] ?? []; if (!is_array($responseBody)) { - break; + throw new Exception('Pull request files response is not an object.'); } $values = $responseBody['values'] ?? []; if (!is_array($values)) { - break; + throw new Exception('Pull request files response is not a list of diffs.'); } foreach ($values as $diff) { diff --git a/src/VCS/Adapter/Git/GitHub.php b/src/VCS/Adapter/Git/GitHub.php index 826b92ee..c19e772d 100644 --- a/src/VCS/Adapter/Git/GitHub.php +++ b/src/VCS/Adapter/Git/GitHub.php @@ -765,7 +765,17 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $ 'page' => $currentPage, ]); + $responseHeaders = $response['headers'] ?? []; + $statusCode = $responseHeaders['status-code'] ?? 0; + if ($statusCode >= 400) { + throw new Exception("Failed to get pull request files: HTTP {$statusCode}", $statusCode); + } + $files = $response['body'] ?? []; + if (!\is_array($files) || !\array_is_list($files)) { + throw new Exception('Pull request files response is not a list of files.'); + } + $allFiles = array_merge($allFiles, $files); if (\count($files) < $perPage) { diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 6d25871b..f6649087 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -774,7 +774,11 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $ } $files = $response['body'] ?? []; - if (!is_array($files) || empty($files)) { + if (!is_array($files) || !\array_is_list($files)) { + throw new Exception('Merge request files response is not a list of diffs.'); + } + + if (empty($files)) { break; } diff --git a/src/VCS/Adapter/Git/Gitea.php b/src/VCS/Adapter/Git/Gitea.php index d5d7ab18..719ef2db 100644 --- a/src/VCS/Adapter/Git/Gitea.php +++ b/src/VCS/Adapter/Git/Gitea.php @@ -796,6 +796,10 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $ } $files = $response['body'] ?? []; + if (!\is_array($files) || !\array_is_list($files)) { + throw new Exception('Pull request files response is not a list of files.'); + } + $allFiles = array_merge($allFiles, $files); if (\count($files) < $limit) { diff --git a/tests/VCS/Adapter/BitbucketTest.php b/tests/VCS/Adapter/BitbucketTest.php index 3bb9cffb..e8fe7451 100644 --- a/tests/VCS/Adapter/BitbucketTest.php +++ b/tests/VCS/Adapter/BitbucketTest.php @@ -50,6 +50,32 @@ protected function signWebhookPayload(string $payload, string $secret): string return 'sha256=' . hash_hmac('sha256', $payload, $secret); } + /** + * Bitbucket reports a pull request's files as a diffstat page, cursored by + * a 'next' link rather than counted against the page size. + * + * @param array $filenames + * @return array|string + */ + protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string + { + return [ + 'values' => \array_map(fn (string $filename) => ['new' => ['path' => $filename]], $filenames), + 'next' => $last ? null : 'https://api.bitbucket.org/2.0/next', + ]; + } + + /** + * A Bitbucket page is an object, so an error payload carrying no 'values' + * is indistinguishable from a page listing no files. + * + * @return array|string> + */ + protected function malformedPullRequestFilesBodies(): array + { + return ['an HTML error page' => '502 Bad Gateway']; + } + protected function setupAdapter(): void { if (empty(static::$accessToken)) { diff --git a/tests/VCS/Adapter/GitHubTest.php b/tests/VCS/Adapter/GitHubTest.php index a31f5851..9d12acb0 100644 --- a/tests/VCS/Adapter/GitHubTest.php +++ b/tests/VCS/Adapter/GitHubTest.php @@ -30,8 +30,10 @@ protected function signWebhookPayload(string $payload, string $secret): string { return 'sha256=' . hash_hmac('sha256', $payload, $secret); } + protected static string $eventHeader = 'x-github-event'; protected static string $signatureHeader = 'x-hub-signature-256'; + protected static bool $replayAdapterNeedsToken = false; protected function setupAdapter(): void { diff --git a/tests/VCS/Adapter/GitLabTest.php b/tests/VCS/Adapter/GitLabTest.php index 13c4704b..6a19f87e 100644 --- a/tests/VCS/Adapter/GitLabTest.php +++ b/tests/VCS/Adapter/GitLabTest.php @@ -6,6 +6,7 @@ use Utopia\Cache\Cache; use Utopia\System\System; use Utopia\Tests\Base; +use Utopia\VCS\Adapter\Git; use Utopia\VCS\Adapter\Git\GitLab; class GitLabTest extends Base @@ -18,6 +19,7 @@ class GitLabTest extends Base protected static string $signatureHeader = 'x-gitlab-token'; protected static string $pushEventName = 'Push Hook'; protected static string $pullRequestEventName = 'Merge Request Hook'; + protected static int $pullRequestFilesPageSize = 100; /** @var array */ protected static array $pullRequestOpenedActions = ['opened', 'synchronize']; @@ -36,6 +38,28 @@ protected function signWebhookPayload(string $payload, string $secret): string return $secret; } + /** + * GitLab reports a merge request's files as diffs keyed by path. + * + * @param array $filenames + * @return array|string + */ + protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string + { + return \array_map(fn (string $filename) => ['new_path' => $filename], $filenames); + } + + /** + * @param array> $responses + */ + protected function replayAdapter(array $responses): Git + { + // GitLab waits for the merge request's diff to be ready before paging. + \array_unshift($responses, $this->providerResponse(['patch_id_sha' => 'abc123'])); + + return parent::replayAdapter($responses); + } + protected function setupAdapter(): void { if (empty(static::$accessToken)) { diff --git a/tests/VCS/Base.php b/tests/VCS/Base.php index ab571542..f2d9c63a 100644 --- a/tests/VCS/Base.php +++ b/tests/VCS/Base.php @@ -4,6 +4,8 @@ use Exception; use PHPUnit\Framework\TestCase; +use Utopia\Cache\Adapter\None; +use Utopia\Cache\Cache; use Utopia\Fetch\Client; use Utopia\System\System; use Utopia\VCS\Adapter\Git; @@ -113,6 +115,18 @@ abstract class Base extends TestCase protected static bool $supportsPullRequestLookup = true; + /** + * Files per page the adapter asks this provider for, and so the count that + * tells a full page apart from the last one. + */ + protected static int $pullRequestFilesPageSize = 30; + + /** + * Whether a replay adapter has to be handed a token. GitHub mints its own + * from an app, which cannot be done offline, and defaults it instead. + */ + protected static bool $replayAdapterNeedsToken = true; + protected static bool $supportsCommitStatuses = true; protected static bool $supportsCommitStatusLookup = true; @@ -204,6 +218,33 @@ abstract protected function pushPayload(string $branch, array $added = [], array */ abstract protected function pullRequestPayload(bool $external = false): string; + /** + * Build one page of a pull request's files, shaped the way this provider + * returns it. $last tells providers that page by cursor rather than by + * count that no page follows. + * + * @param array $filenames + * @return array|string + */ + protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string + { + return \array_map(fn (string $filename) => ['filename' => $filename, 'status' => 'added'], $filenames); + } + + /** + * Bodies this provider, or something in front of it, can return with a + * success status where a page of files was expected. + * + * @return array|string> + */ + protected function malformedPullRequestFilesBodies(): array + { + return [ + 'an error payload' => ['message' => 'Bad credentials'], + 'an HTML error page' => '502 Bad Gateway', + ]; + } + protected function setUp(): void { $this->setupAdapter(); @@ -1305,6 +1346,122 @@ public function testGetPullRequestFiles(): void } } + /** + * The adapter under test, replying from $responses in order instead of + * calling out. Paging and provider failures are impractical to provoke on + * a live forge, and a failure has to surface as an exception rather than + * as a file list built out of an error payload: a webhook that reads an + * empty diff silently skips the deployments the push should have made. + * + * @param array> $responses + */ + protected function replayAdapter(array $responses): Git + { + $adapter = $this->getMockBuilder($this->vcsAdapter::class) + ->setConstructorArgs([new Cache(new None())]) + ->onlyMethods(['call']) + ->getMock(); + + $adapter->method('call')->willReturnCallback( + function () use (&$responses): array { + return \array_shift($responses) ?? $this->providerResponse([]); + } + ); + + if (static::$replayAdapterNeedsToken) { + $adapter->initializeVariables('1', '', null, 'token', null); + } + + return $adapter; + } + + /** + * @param array|string $body + * @return array + */ + protected function providerResponse(array|string $body, int $statusCode = 200): array + { + return [ + 'headers' => ['status-code' => $statusCode], + 'body' => $body, + ]; + } + + public function testGetPullRequestFilesPaginated(): void + { + $this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests'); + + $pageSize = static::$pullRequestFilesPageSize; + $full = \array_map(fn (int $i) => "first-{$i}.txt", \range(0, $pageSize - 1)); + $adapter = $this->replayAdapter([ + $this->providerResponse($this->pullRequestFilesPage($full, false)), + $this->providerResponse($this->pullRequestFilesPage(['last.txt'])), + ]); + + $result = $adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1); + + $filenames = array_column($result, 'filename'); + $this->assertCount($pageSize + 1, $filenames); + $this->assertSame('last.txt', $filenames[$pageSize]); + } + + public function testGetPullRequestFilesProviderFailure(): void + { + $this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests'); + + foreach ([401, 403, 404, 500] as $statusCode) { + $adapter = $this->replayAdapter([ + $this->providerResponse(['message' => 'Bad credentials'], $statusCode), + ]); + + $thrown = null; + + try { + $adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1); + } catch (Exception $e) { + $thrown = $e; + } + + $this->assertNotNull($thrown, "HTTP {$statusCode} was not reported as a failure"); + $this->assertSame($statusCode, $thrown->getCode()); + } + } + + public function testGetPullRequestFilesProviderFailureOnLaterPage(): void + { + $this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests'); + + $full = \array_map(fn (int $i) => "first-{$i}.txt", \range(0, static::$pullRequestFilesPageSize - 1)); + $adapter = $this->replayAdapter([ + $this->providerResponse($this->pullRequestFilesPage($full, false)), + $this->providerResponse(['message' => 'API rate limit exceeded'], 403), + ]); + + $this->expectException(Exception::class); + $this->expectExceptionCode(403); + + $adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1); + } + + public function testGetPullRequestFilesMalformedBody(): void + { + $this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests'); + + foreach ($this->malformedPullRequestFilesBodies() as $description => $body) { + $adapter = $this->replayAdapter([$this->providerResponse($body)]); + + $thrown = null; + + try { + $adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1); + } catch (Exception $e) { + $thrown = $e; + } + + $this->assertNotNull($thrown, "{$description} was not reported as a failure"); + } + } + public function testGetPullRequestWithInvalidNumber(): void { $this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests'); From 61b92d16bf00daa0321061a772db12c56e8e5ad2 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Mon, 17 Aug 2026 08:58:04 +0530 Subject: [PATCH 2/2] style: keep the pull request files comments to what the code cannot say --- tests/VCS/Adapter/BitbucketTest.php | 7 ++----- tests/VCS/Adapter/GitLabTest.php | 2 -- tests/VCS/Base.php | 22 ++++++++-------------- 3 files changed, 10 insertions(+), 21 deletions(-) diff --git a/tests/VCS/Adapter/BitbucketTest.php b/tests/VCS/Adapter/BitbucketTest.php index e8fe7451..cbb2e236 100644 --- a/tests/VCS/Adapter/BitbucketTest.php +++ b/tests/VCS/Adapter/BitbucketTest.php @@ -51,9 +51,6 @@ protected function signWebhookPayload(string $payload, string $secret): string } /** - * Bitbucket reports a pull request's files as a diffstat page, cursored by - * a 'next' link rather than counted against the page size. - * * @param array $filenames * @return array|string */ @@ -66,8 +63,8 @@ protected function pullRequestFilesPage(array $filenames, bool $last = true): ar } /** - * A Bitbucket page is an object, so an error payload carrying no 'values' - * is indistinguishable from a page listing no files. + * A page is an object here, so an error payload carrying no 'values' is + * indistinguishable from a page listing no files. * * @return array|string> */ diff --git a/tests/VCS/Adapter/GitLabTest.php b/tests/VCS/Adapter/GitLabTest.php index 6a19f87e..4c1a9ac3 100644 --- a/tests/VCS/Adapter/GitLabTest.php +++ b/tests/VCS/Adapter/GitLabTest.php @@ -39,8 +39,6 @@ protected function signWebhookPayload(string $payload, string $secret): string } /** - * GitLab reports a merge request's files as diffs keyed by path. - * * @param array $filenames * @return array|string */ diff --git a/tests/VCS/Base.php b/tests/VCS/Base.php index f2d9c63a..c68ffb45 100644 --- a/tests/VCS/Base.php +++ b/tests/VCS/Base.php @@ -116,14 +116,12 @@ abstract class Base extends TestCase protected static bool $supportsPullRequestLookup = true; /** - * Files per page the adapter asks this provider for, and so the count that - * tells a full page apart from the last one. + * Files per page the adapter asks this provider for. */ protected static int $pullRequestFilesPageSize = 30; /** - * Whether a replay adapter has to be handed a token. GitHub mints its own - * from an app, which cannot be done offline, and defaults it instead. + * GitHub mints its own token from an app, which cannot be done offline. */ protected static bool $replayAdapterNeedsToken = true; @@ -219,9 +217,8 @@ abstract protected function pushPayload(string $branch, array $added = [], array abstract protected function pullRequestPayload(bool $external = false): string; /** - * Build one page of a pull request's files, shaped the way this provider - * returns it. $last tells providers that page by cursor rather than by - * count that no page follows. + * One page of a pull request's files. $last is only read by providers that + * page by cursor rather than by count. * * @param array $filenames * @return array|string @@ -232,8 +229,8 @@ protected function pullRequestFilesPage(array $filenames, bool $last = true): ar } /** - * Bodies this provider, or something in front of it, can return with a - * success status where a page of files was expected. + * Bodies this provider can return with a success status where a page of + * files was expected. * * @return array|string> */ @@ -1347,11 +1344,8 @@ public function testGetPullRequestFiles(): void } /** - * The adapter under test, replying from $responses in order instead of - * calling out. Paging and provider failures are impractical to provoke on - * a live forge, and a failure has to surface as an exception rather than - * as a file list built out of an error payload: a webhook that reads an - * empty diff silently skips the deployments the push should have made. + * The adapter under test, replying from $responses in order. A live forge + * cannot be made to page or fail on demand. * * @param array> $responses */