fix(github): check push watch paths against the net change of the push

Closes #10937

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Andras Bacsai
2026-10-15 22:06:30 +02:00
co-authored by Claude Opus 5.5
parent 2db26e88ab
commit 19430b461a
3 changed files with 207 additions and 1 deletions
+32
View File
@@ -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<int, string>|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.
*
+6 -1
View File
@@ -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;
}
@@ -0,0 +1,169 @@
<?php
use App\Models\Application;
use App\Models\ApplicationDeploymentQueue;
use App\Models\Environment;
use App\Models\GithubApp;
use App\Models\PrivateKey;
use App\Models\Project;
use App\Models\Server;
use App\Models\Team;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Http\Client\Request;
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Queue;
use Illuminate\Testing\TestResponse;
use Tests\TestCase;
uses(RefreshDatabase::class);
const PUSH_WATCH_PATHS_BEFORE_SHA = '1111111111111111111111111111111111111111';
const PUSH_WATCH_PATHS_AFTER_SHA = '2222222222222222222222222222222222222222';
const PUSH_WATCH_PATHS_COMPARE_URL = 'https://api.github.com/repos/test-org/test-repo/compare/'.PUSH_WATCH_PATHS_BEFORE_SHA.'...'.PUSH_WATCH_PATHS_AFTER_SHA;
beforeEach(function () {
Server::flushIdentityMap();
Queue::fake();
$team = Team::factory()->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();
});