From 19430b461a28bfea49cc5f278a59ffd3e4930771 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 15 Oct 2026 22:06:30 +0200 Subject: [PATCH] fix(github): check push watch paths against the net change of the push Closes #10937 Co-Authored-By: Claude Opus 5.5 --- app/Http/Controllers/Webhook/Github.php | 32 ++++ bootstrap/helpers/github.php | 7 +- tests/Feature/GithubAppPushWatchPathsTest.php | 169 ++++++++++++++++++ 3 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/GithubAppPushWatchPathsTest.php diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index ba270cf688..cecd4186d9 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -484,6 +484,9 @@ class Github extends Controller if ($applications->isEmpty()) { return response("Nothing to do. No applications found with branch '$branch'."); } + if ($applications->contains(fn (Application $application): bool => filled($application->watch_paths))) { + $changed_files = $this->githubPushComparedFiles($github_app, $payload) ?? $changed_files; + } } if ($x_github_event === 'pull_request') { if ($action !== 'closed') { @@ -620,6 +623,35 @@ class Github extends Controller } } + /** + * Files that the push changes between its before and after commits, from + * the GitHub compare API. + * + * The file lists of the pushed commits are not the net change: a merge + * commit names the files that it changes relative to its first parent, + * also when the push does not change them. Returns null for a new branch, + * a missing repository name or an API failure; the caller then uses the + * file lists of the pushed commits. + * + * @return Collection|null + * + * @throws InvalidWebhookPayloadException When the before or after value is not a commit SHA. + */ + private function githubPushComparedFiles(GithubApp $githubApp, mixed $payload): ?Collection + { + $before = $this->webhookCommitSha($payload, 'before'); + $after = $this->webhookCommitSha($payload, 'after'); + $fullName = $this->manualWebhookRepositoryFullName(data_get($payload, 'repository.full_name')); + if ($before === null || $after === null || $fullName === null || preg_match('/\A0+\z/', $before) === 1) { + return null; + } + + [$owner, $repo] = explode('/', $fullName, 2); + $files = getGithubCommitRangeFiles($githubApp, $owner, $repo, $before, $after); + + return $files === null ? null : collect($files); + } + /** * Read and validate the pull_request payload fields that the handlers use. * diff --git a/bootstrap/helpers/github.php b/bootstrap/helpers/github.php index 46ab731e94..48498c0135 100644 --- a/bootstrap/helpers/github.php +++ b/bootstrap/helpers/github.php @@ -417,7 +417,12 @@ function getGithubCommitRangeFiles(?GithubApp $source, string $owner, string $re return null; } - return $files->pluck('filename')->filter()->values()->toArray(); + // A renamed file also changes its previous path. + return $files->flatMap(fn (mixed $file): array => [data_get($file, 'filename'), data_get($file, 'previous_filename')]) + ->filter(fn (mixed $filename): bool => is_string($filename) && $filename !== '') + ->unique() + ->values() + ->toArray(); } catch (Exception $e) { return null; } diff --git a/tests/Feature/GithubAppPushWatchPathsTest.php b/tests/Feature/GithubAppPushWatchPathsTest.php new file mode 100644 index 0000000000..ddc0985c8a --- /dev/null +++ b/tests/Feature/GithubAppPushWatchPathsTest.php @@ -0,0 +1,169 @@ +create(); + + openssl_pkey_export(openssl_pkey_new([ + 'private_key_bits' => 2048, + 'private_key_type' => OPENSSL_KEYTYPE_RSA, + ]), $pemKey); + + $this->githubApp = GithubApp::create([ + 'name' => 'github-app-push-watch-paths', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'app_id' => 1234567890, + 'installation_id' => 67890, + 'webhook_secret' => 'test-secret', + 'private_key_id' => PrivateKey::create([ + 'name' => 'GitHub App Key', + 'private_key' => $pemKey, + 'team_id' => $team->id, + ])->id, + 'is_public' => false, + 'team_id' => $team->id, + ]); + + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $server = Server::factory()->create(['team_id' => $team->id]); + $server->settings->update([ + 'is_reachable' => true, + 'is_usable' => true, + 'force_disabled' => false, + ]); + $destination = $server->standaloneDockers()->firstOrFail(); + + $createApplication = fn (string $name, string $watchPaths): Application => Application::create([ + 'name' => $name, + 'git_repository' => 'test-org/test-repo', + 'git_branch' => 'main', + 'build_pack' => 'nixpacks', + 'ports_exposes' => '3000', + 'watch_paths' => $watchPaths, + 'repository_project_id' => 987654321, + 'source_id' => $this->githubApp->id, + 'source_type' => GithubApp::class, + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + ]); + + $this->payments = $createApplication('payments', 'packages/payments/**'); + $this->dashboard = $createApplication('dashboard', 'apps/dashboard/**'); +}); + +/** + * A push of a merge commit that names packages/payments/config.yml relative to + * its first parent, while the net change of the push is only in apps/dashboard. + */ +function sendGithubAppMergePush(TestCase $test, string $before = PUSH_WATCH_PATHS_BEFORE_SHA): TestResponse +{ + $body = json_encode([ + 'ref' => 'refs/heads/main', + 'before' => $before, + 'after' => PUSH_WATCH_PATHS_AFTER_SHA, + 'repository' => ['id' => 987654321, 'full_name' => 'test-org/test-repo'], + 'commits' => [ + ['message' => 'Merge integration into feature', 'added' => [], 'removed' => [], 'modified' => ['packages/payments/config.yml']], + ['message' => 'Update dashboard', 'added' => [], 'removed' => [], 'modified' => ['apps/dashboard/page.tsx']], + ], + ], JSON_THROW_ON_ERROR); + + return $test->call('POST', '/webhooks/source/github/events', [], [], [], [ + 'HTTP_X-GitHub-Event' => 'push', + 'HTTP_X-GitHub-Hook-Installation-Target-Id' => '1234567890', + 'HTTP_X-Hub-Signature-256' => 'sha256='.hash_hmac('sha256', $body, 'test-secret'), + 'CONTENT_TYPE' => 'application/json', + ], $body); +} + +function fakeGithubAppPushApi(array $compareResponse): void +{ + Http::fake([ + 'https://api.github.com/zen' => Http::response('Keep it logically awesome.', 200, [ + 'Date' => now()->toRfc7231String(), + ]), + 'https://api.github.com/app/installations/67890/access_tokens' => Http::response([ + 'token' => 'fake-installation-token', + ], 201), + PUSH_WATCH_PATHS_COMPARE_URL => $compareResponse === [] + ? Http::response(['message' => 'Server Error'], 500) + : Http::response($compareResponse), + ]); +} + +function deployedApplicationIds(): array +{ + return ApplicationDeploymentQueue::query()->pluck('application_id')->map(fn (mixed $id): int => (int) $id)->sort()->values()->all(); +} + +it('checks watch paths against the net files of the push, not the files of each commit', function () { + fakeGithubAppPushApi(['status' => 'ahead', 'files' => [['filename' => 'apps/dashboard/page.tsx']]]); + + sendGithubAppMergePush($this)->assertOk(); + + expect(deployedApplicationIds())->toBe([$this->dashboard->id]); + Http::assertSent(fn (Request $request): bool => $request->url() === PUSH_WATCH_PATHS_COMPARE_URL); +}); + +it('triggers watch paths for the previous name of a renamed file', function () { + fakeGithubAppPushApi(['status' => 'ahead', 'files' => [ + ['filename' => 'packages/legacy/config.yml', 'previous_filename' => 'packages/payments/config.yml', 'status' => 'renamed'], + ]]); + + sendGithubAppMergePush($this)->assertOk(); + + expect(deployedApplicationIds())->toBe([$this->payments->id]); +}); + +it('uses the files of the pushed commits when the compare API fails', function () { + fakeGithubAppPushApi([]); + + sendGithubAppMergePush($this)->assertOk(); + + expect(deployedApplicationIds())->toBe([$this->payments->id, $this->dashboard->id]); +}); + +it('uses the files of the pushed commits for a new branch', function () { + fakeGithubAppPushApi(['status' => 'ahead', 'files' => []]); + + sendGithubAppMergePush($this, before: str_repeat('0', 40))->assertOk(); + + expect(deployedApplicationIds())->toBe([$this->payments->id, $this->dashboard->id]); + Http::assertNotSent(fn (Request $request): bool => str_contains($request->url(), '/compare/')); +}); + +it('does not call the compare API when no application has watch paths', function () { + Application::query()->update(['watch_paths' => null]); + fakeGithubAppPushApi(['status' => 'ahead', 'files' => []]); + + sendGithubAppMergePush($this)->assertOk(); + + expect(deployedApplicationIds())->toBe([$this->payments->id, $this->dashboard->id]); + Http::assertNothingSent(); +});