fix(github-actions): harden google-internal-tests against SHA status overwrite and 3000-file truncation

github-actions/google-internal-tests/BUILD.bazel

@@ -1,13 +1,16 @@
 load("@devinfra_npm//:defs.bzl", "npm_link_all_packages")
-load("//tools:defaults.bzl", "esbuild_checked_in", "ts_project")
+load("//tools:defaults.bzl", "esbuild_checked_in", "jasmine_test", "ts_project")
 
 package(default_visibility = ["//github-actions/google-internal-tests:__subpackages__"])
 
 npm_link_all_packages()
 
 ts_project(
     name = "lib",
-    srcs = glob(["lib/*.ts"]),
+    srcs = glob(
+        ["lib/*.ts"],
+        exclude = ["lib/*.spec.ts"],
+    ),
     tsconfig = "//github-actions:tsconfig",
     deps = [
         ":node_modules/@actions/core",
@@ -18,6 +21,32 @@ ts_project(
     ],
 )
 
+ts_project(
+    name = "test_lib",
+    testonly = True,
+    srcs = glob(["lib/*.spec.ts"]),
+    tsconfig = "//github-actions:tsconfig_test",
+    deps = [
+        ":lib",
+        ":node_modules/@actions/core",
+        ":node_modules/@actions/github",
+        ":node_modules/@octokit/rest",
+        ":node_modules/@types/node",
+        "//github-actions:node_modules/@types/jasmine",
+        "//ng-dev/utils:g3_sync_config",
+    ],
+)
+
+jasmine_test(
+    name = "test",
+    data = [
+        ":test_lib",
+    ],
+    env = {
+        "GITHUB_REPOSITORY": "angular/angular",
+    },
+)
+
 esbuild_checked_in(
     name = "main",
     srcs = [

github-actions/google-internal-tests/lib/main.spec.ts

@@ -0,0 +1,194 @@
+import {context} from '@actions/github';
+import {Octokit} from '@octokit/rest';
+import {main} from './main.js';
+
+describe('google-internal-tests action', () => {
+  let mockGit: jasmine.SpyObj<Octokit>;
+  let existingStatuses: Array<{context: string; state: string; target_url?: string | null}>;
+  let prFiles: Array<{filename: string; status?: string; previous_filename?: string}>;
+  const mockSyncConfig = {
+    config: {
+      syncedFilePatterns: ['packages/**'],
+      alwaysExternalFilePatterns: [],
+      separateFilePatterns: ['goldens/**'],
+    },
+    ngMatchFn: (p: string) => p.startsWith('packages/'),
+    separateMatchFn: (p: string) => p.startsWith('goldens/'),
+  };
+
+  beforeEach(() => {
+    process.env['INPUT_RUN-TESTS-GUIDE-URL'] = 'https://goo.gle/angular-internal-presubmit';
+    existingStatuses = [];
+    prFiles = [];
+
+    context.eventName = 'pull_request_target';
+    context.payload = {
+      pull_request: {
+        number: 100,
+        head: {sha: 'deadbeef1234567890'},
+        base: {ref: 'main'},
+      },
+    };
+    spyOnProperty(context, 'repo', 'get').and.returnValue({
+      owner: 'angular',
+      repo: 'angular',
+    });
+
+    mockGit = jasmine.createSpyObj('Octokit', ['paginate', 'repos', 'pulls']);
+    mockGit.repos = jasmine.createSpyObj('repos', [
+      'createCommitStatus',
+      'getCombinedStatusForRef',
+    ]);
+    mockGit.pulls = jasmine.createSpyObj('pulls', ['listFiles']);
+
+    (mockGit.repos.createCommitStatus as unknown as jasmine.Spy).and.resolveTo({});
+    (mockGit.paginate as jasmine.Spy).and.callFake((fn: unknown) => {
+      if (fn === mockGit.repos.getCombinedStatusForRef) {
+        return Promise.resolve(existingStatuses);
+      }
+      if (fn === mockGit.pulls.listFiles) {
+        return Promise.resolve(prFiles);
+      }
+      return Promise.resolve([]);
+    });
+  });
+
+  afterEach(() => {
+    delete process.env['INPUT_RUN-TESTS-GUIDE-URL'];
+  });
+
+  it('should post pending status when a PR targeting main touches synced files', async () => {
+    prFiles = [{filename: 'packages/core/src/index.ts'}];
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).toHaveBeenCalledOnceWith({
+      owner: 'angular',
+      repo: 'angular',
+      state: 'pending',
+      description: 'Waiting for tests to start. @Googlers: Initiate a presubmit. See -->',
+      target_url: 'https://goo.gle/angular-internal-presubmit',
+      context: 'google-internal-tests',
+      sha: 'deadbeef1234567890',
+    });
+  });
+
+  it('should post success status when a PR targeting main does not touch synced files', async () => {
+    prFiles = [{filename: 'docs/README.md'}];
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).toHaveBeenCalledOnceWith({
+      owner: 'angular',
+      repo: 'angular',
+      state: 'success',
+      description: 'Does not affect Google.',
+      context: 'google-internal-tests',
+      sha: 'deadbeef1234567890',
+    });
+  });
+
+  it('should post skipped success status for PR targeting non-main branch when no status exists', async () => {
+    context.payload.pull_request!.base.ref = '18.2.x';
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).toHaveBeenCalledOnceWith({
+      owner: 'angular',
+      repo: 'angular',
+      state: 'success',
+      description: 'Skipped. PR does not target `main` branch',
+      context: 'google-internal-tests',
+      sha: 'deadbeef1234567890',
+    });
+  });
+
+  it('should not overwrite an existing status pointing to an internal CL', async () => {
+    context.payload.pull_request!.base.ref = '18.2.x';
+    existingStatuses = [
+      {
+        context: 'google-internal-tests',
+        state: 'success',
+        target_url: 'http://cl/123456789',
+      },
+    ];
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).not.toHaveBeenCalled();
+  });
+
+  it('should not overwrite an existing pending status when triggered on a non-main branch with the same head SHA', async () => {
+    context.payload.pull_request!.base.ref = '18.2.x';
+    existingStatuses = [
+      {
+        context: 'google-internal-tests',
+        state: 'pending',
+        target_url: 'https://goo.gle/angular-internal-presubmit',
+      },
+    ];
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).not.toHaveBeenCalled();
+  });
+
+  it('should overwrite a non-CL skipped success status when a PR targeting main with the same SHA touches synced files', async () => {
+    context.payload.pull_request!.base.ref = 'main';
+    existingStatuses = [
+      {
+        context: 'google-internal-tests',
+        state: 'success',
+        target_url: null,
+      },
+    ];
+    prFiles = [{filename: 'packages/core/src/index.ts'}];
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).toHaveBeenCalledOnceWith({
+      owner: 'angular',
+      repo: 'angular',
+      state: 'pending',
+      description: 'Waiting for tests to start. @Googlers: Initiate a presubmit. See -->',
+      target_url: 'https://goo.gle/angular-internal-presubmit',
+      context: 'google-internal-tests',
+      sha: 'deadbeef1234567890',
+    });
+  });
+
+  it('should fail closed with pending status when returned file list reaches 3000-file pagination limit', async () => {
+    prFiles = Array.from({length: 3000}, (_, i) => ({
+      filename: `docs/dummy-${i}.md`,
+    }));
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).toHaveBeenCalledOnceWith({
+      owner: 'angular',
+      repo: 'angular',
+      state: 'pending',
+      description: 'Waiting for tests to start. @Googlers: Initiate a presubmit. See -->',
+      target_url: 'https://goo.gle/angular-internal-presubmit',
+      context: 'google-internal-tests',
+      sha: 'deadbeef1234567890',
+    });
+  });
+
+  it('should fail closed with pending status when returned file count is less than pull_request.changed_files', async () => {
+    context.payload.pull_request!['changed_files'] = 3005;
+    prFiles = [{filename: 'docs/README.md'}];
+
+    await main(mockGit as unknown as Octokit, mockSyncConfig);
+
+    expect(mockGit.repos.createCommitStatus).toHaveBeenCalledOnceWith({
+      owner: 'angular',
+      repo: 'angular',
+      state: 'pending',
+      description: 'Waiting for tests to start. @Googlers: Initiate a presubmit. See -->',
+      target_url: 'https://goo.gle/angular-internal-presubmit',
+      context: 'google-internal-tests',
+      sha: 'deadbeef1234567890',
+    });
+  });
+});

github-actions/google-internal-tests/lib/main.ts

@@ -11,7 +11,10 @@ type GetCombinedStatusForRefResponse =
   RestEndpointMethodTypes['repos']['getCombinedStatusForRef']['response'];
 type GithubStatus = GetCombinedStatusForRefResponse['data']['statuses'][0];
 
-async function main() {
+export async function main(
+  providedGithub?: Octokit,
+  providedSyncConfig?: Awaited<ReturnType<typeof getGoogleSyncConfig>>,
+) {
   if (context.repo.owner !== 'angular') {
     core.info('Skipping Google Internal Tests action for non-Angular repos.');
     return;
@@ -23,20 +26,30 @@ async function main() {
     );
   }
 
-  const githubToken = core.getInput('github-token', {required: true});
   const runTestGuideURL = core.getInput('run-tests-guide-url', {required: false});
-  const syncConfigPath = path.resolve(core.getInput('sync-config', {required: true}));
-  const syncConfig = await getGoogleSyncConfig(syncConfigPath);
+  const syncConfig =
+    providedSyncConfig ??
+    (await getGoogleSyncConfig(path.resolve(core.getInput('sync-config', {required: true}))));
 
   const prNum = context.payload.pull_request!.number;
   const prHeadSHA = context.payload.pull_request!.head!.sha;
   const prBaseRef = context.payload.pull_request!.base!.ref;
-  const github = new Octokit({auth: githubToken});
+  const github =
+    providedGithub ??
+    new Octokit({
+      auth: core.getInput('github-token', {required: true}),
+    });
   const existingGoogleStatus = await findExistingTestStatus(github, prHeadSHA);
 
-  // If there is an existing status already pointing to an internal CL, we do not override
-  // the status. This can happen when e.g. a presubmit-tested PR is closed and reopened.
-  if (existingGoogleStatus && existingGoogleStatus.target_url?.startsWith('http://cl/')) {
+  // If there is an existing status already pointing to an internal CL or currently pending, we
+  // do not override the status. This can happen when e.g. a presubmit-tested PR is closed and
+  // reopened, or when the same commit SHA is opened against a non-sync branch while a sync
+  // branch PR is pending.
+  if (
+    existingGoogleStatus &&
+    (existingGoogleStatus.state === 'pending' ||
+      existingGoogleStatus.target_url?.startsWith('http://cl/'))
+  ) {
     core.info(`Pull request HEAD commit already has existing test status.`);
     return;
   }
@@ -60,20 +73,35 @@ async function main() {
     pull_number: prNum,
   });
 
-  let affectsGoogle = false;
-  for (const f of files) {
-    const paths = [f.filename];
-    if (f.status === 'renamed' && f.previous_filename) {
-      paths.push(f.previous_filename);
-    }
+  const maxPaginatedFiles = 3000;
+  const changedFilesCount = context.payload.pull_request?.changed_files;
 
-    if (
-      paths.some(
-        (filePath) => syncConfig.ngMatchFn(filePath) || syncConfig.separateMatchFn(filePath),
-      )
-    ) {
-      affectsGoogle = true;
-      break;
+  let affectsGoogle = false;
+  if (
+    files.length >= maxPaginatedFiles ||
+    (typeof changedFilesCount === 'number' && files.length < changedFilesCount)
+  ) {
+    core.warning(
+      `Pull request file list reached the GitHub pagination limit (${files.length} files returned` +
+        `${typeof changedFilesCount === 'number' ? ` of ${changedFilesCount}` : ''}). ` +
+        `Assuming pull request affects Google to fail closed.`,
+    );
+    affectsGoogle = true;
+  } else {
+    for (const f of files) {
+      const paths = [f.filename];
+      if (f.status === 'renamed' && f.previous_filename) {
+        paths.push(f.previous_filename);
+      }
+
+      if (
+        paths.some(
+          (filePath) => syncConfig.ngMatchFn(filePath) || syncConfig.separateMatchFn(filePath),
+        )
+      ) {
+        affectsGoogle = true;
+        break;
+      }
     }
   }
 
@@ -111,7 +139,9 @@ async function findExistingTestStatus(
   return existingStatuses.find((s) => s.context === statusContext) ?? null;
 }
 
-main().catch((e: Error) => {
-  console.error(e);
-  core.setFailed(e.message);
-});
+if (!process.env['TEST_SRCDIR']) {
+  main().catch((e: Error) => {
+    console.error(e);
+    core.setFailed(e.message);
+  });
+}