fix(ng-dev): allow for a retry during the validation that the commit sha being released is the expected commit

ng-dev/release/publish/actions.ts

@@ -43,6 +43,7 @@ import {promptToInitiatePullRequestMerge} from './prompt-merge.js';
 import {Prompt} from '../../utils/prompt.js';
 import {glob} from 'fast-glob';
 import {PnpmVersioning} from './pnpm-versioning.js';
+import {Commit} from '../../utils/git/octokit-types.js';
 
 /** Interface describing a Github repository. */
 export interface GithubRepo {
@@ -150,28 +151,11 @@ export abstract class ReleaseAction {
   }
 
   /** Gets the most recent commit of a specified branch. */
-  protected async getLatestCommitOfBranch(branchName: string): Promise<string> {
+  protected async getLatestCommitOfBranch(branchName: string): Promise<Commit> {
     const {
       data: {commit},
     } = await this.git.github.repos.getBranch({...this.git.remoteParams, branch: branchName});
-    return commit.sha;
-  }
-
-  /** Checks whether the given revision is ahead to the base by the specified amount. */
-  private async _isRevisionAheadOfBase(
-    baseRevision: string,
-    targetRevision: string,
-    expectedAheadCount: number,
-  ) {
-    const {
-      data: {ahead_by, status},
-    } = await this.git.github.repos.compareCommits({
-      ...this.git.remoteParams,
-      base: baseRevision,
-      head: targetRevision,
-    });
-
-    return status === 'ahead' && ahead_by === expectedAheadCount;
+    return commit;
   }
 
   /**
@@ -560,7 +544,7 @@ export abstract class ReleaseAction {
     // Keep track of the commit where we started the staging process on. This will be used
     // later to ensure that no changes, except for the version bump have landed as part
     // of the staging time window (where the caretaker could accidentally land other stuff).
-    const beforeStagingSha = await this.getLatestCommitOfBranch(stagingBranch);
+    const {sha: beforeStagingSha} = await this.getLatestCommitOfBranch(stagingBranch);
 
     await this.assertPassingGithubStatus(beforeStagingSha, stagingBranch);
     await this.checkoutUpstreamBranch(stagingBranch);
@@ -703,24 +687,11 @@ export abstract class ReleaseAction {
     npmDistTag: NpmDistTag,
     additionalOptions: {showAsLatestOnGitHub: boolean},
   ) {
-    const versionBumpCommitSha = await this.getLatestCommitOfBranch(publishBranch);
-
-    // Ensure the latest commit in the publish branch is the bump commit.
-    if (!(await this._isCommitForVersionStaging(releaseNotes.version, versionBumpCommitSha))) {
-      Log.error(`  ✘   Latest commit in "${publishBranch}" branch is not a staging commit.`);
-      Log.error('      Please make sure the staging pull request has been merged.');
-      throw new FatalReleaseActionError();
-    }
-
-    // Ensure no commits have landed since we started the staging process. This would signify
-    // that the locally-built release packages are not matching with the release commit on GitHub.
-    // Note: We expect the version bump commit to be ahead by **one** commit. This means it's
-    // the direct parent of the commit that was latest when we started the staging.
-    if (!(await this._isRevisionAheadOfBase(beforeStagingSha, versionBumpCommitSha, 1))) {
-      Log.error(`  ✘   Unexpected additional commits have landed while staging the release.`);
-      Log.error('      Please revert the bump commit and retry, or cut a new version on top.');
-      throw new FatalReleaseActionError();
-    }
+    const releaseSha = await this._getAndValidateLatestCommitForPublishing(
+      publishBranch,
+      releaseNotes.version,
+      beforeStagingSha,
+    );
 
     // Before publishing, we want to ensure that the locally-built packages we
     // built in the staging phase have not been modified accidentally.
@@ -729,7 +700,7 @@ export abstract class ReleaseAction {
     // Create a Github release for the new version.
     await this._createGithubReleaseForVersion(
       releaseNotes,
-      versionBumpCommitSha,
+      releaseSha,
       npmDistTag === 'next',
       additionalOptions.showAsLatestOnGitHub,
     );
@@ -759,13 +730,45 @@ export abstract class ReleaseAction {
     }
   }
 
-  /** Checks whether the given commit represents a staging commit for the specified version. */
-  private async _isCommitForVersionStaging(version: semver.SemVer, commitSha: string) {
-    const {data} = await this.git.github.repos.getCommit({
-      ...this.git.remoteParams,
-      ref: commitSha,
-    });
-    return data.commit.message.startsWith(getCommitMessageForRelease(version));
+  /**
+   * Retreive the latest commit from the provided branch, and verify that it is the expected
+   * release commit and is the direct child of the previous sha provided.
+   *
+   * The method will make one recursive attempt to check again before throwing an error if
+   * any error occurs during this validation.
+   */
+  private async _getAndValidateLatestCommitForPublishing(
+    branch: string,
+    version: semver.SemVer,
+    previousSha: string,
+    isRetry = false,
+  ): Promise<string> {
+    try {
+      const commit = await this.getLatestCommitOfBranch(branch);
+      // Ensure the latest commit in the publish branch is the bump commit.
+      if (!commit.commit.message.startsWith(getCommitMessageForRelease(version))) {
+        /** The shortened sha of the commit for usage in the error message. */
+        const sha = commit.sha.slice(0, 8);
+        Log.error(`  ✘   Latest commit (${sha}) in "${branch}" branch is not a staging commit.`);
+        Log.error('      Please make sure the staging pull request has been merged.');
+        throw new FatalReleaseActionError();
+      }
+
+      // We only inspect the first parent as we enforce that no merge commits are used in our
+      // repos, so all commits have exactly one parent.
+      if (commit.parents[0].sha !== previousSha) {
+        Log.error(`  ✘   Unexpected additional commits have landed while staging the release.`);
+        Log.error('      Please revert the bump commit and retry, or cut a new version on top.');
+        throw new FatalReleaseActionError();
+      }
+
+      return commit.sha;
+    } catch (e: unknown) {
+      if (isRetry) {
+        throw e;
+      }
+      return this._getAndValidateLatestCommitForPublishing(branch, version, previousSha, true);
+    }
   }
 
   // TODO: Remove this check and run it as part of common release validation.

ng-dev/release/publish/actions/configure-next-as-major.ts

@@ -33,7 +33,7 @@ export class ConfigureNextAsMajorAction extends ReleaseAction {
   override async perform() {
     const {branchName} = this.active.next;
     const newVersion = this._newVersion;
-    const beforeStagingSha = await this.getLatestCommitOfBranch(branchName);
+    const {sha: beforeStagingSha} = await this.getLatestCommitOfBranch(branchName);
 
     await this.assertPassingGithubStatus(beforeStagingSha, branchName);
     await this.checkoutUpstreamBranch(branchName);

ng-dev/release/publish/actions/exceptional-minor/prepare-exceptional-minor.ts

@@ -39,7 +39,7 @@ export class PrepareExceptionalMinorAction extends ReleaseAction {
   }
 
   async perform(): Promise<void> {
-    const latestBaseBranchSha = await this.getLatestCommitOfBranch(this._baseBranch);
+    const {sha: latestBaseBranchSha} = await this.getLatestCommitOfBranch(this._baseBranch);
 
     await this.assertPassingGithubStatus(latestBaseBranchSha, this._baseBranch);
 

ng-dev/release/publish/actions/shared/branch-off-next-branch.ts

@@ -59,7 +59,7 @@ export abstract class BranchOffNextBranchBaseAction extends CutNpmNextPrerelease
     const compareVersionForReleaseNotes = await this._computeReleaseNoteCompareVersion();
     const newVersion = await this._computeNewVersion();
     const newBranch = `${newVersion.major}.${newVersion.minor}.x`;
-    const beforeStagingSha = await this.getLatestCommitOfBranch(nextBranchName);
+    const {sha: beforeStagingSha} = await this.getLatestCommitOfBranch(nextBranchName);
 
     // Verify the current next branch has a passing status, before we branch off.
     await this.assertPassingGithubStatus(beforeStagingSha, nextBranchName);

ng-dev/release/publish/test/branch-off-next-branch-testing.ts

@@ -36,20 +36,16 @@
   // We first mock the commit status check for the next branch, then expect two pull
   // requests from a fork that are targeting next and the new feature-freeze branch.
   repo
-    .expectBranchRequest('master', 'PRE_STAGING_SHA')
+    .expectBranchRequest('master', {sha: 'PRE_STAGING_SHA'})
     .expectCommitStatusCheck('PRE_STAGING_SHA', 'success')
     .expectFindForkRequest(fork)
     .expectPullRequestToBeCreated(expectedNewBranch, fork, expectedStagingForkBranch, 200)
     .expectPullRequestMergeCheck(200, false)
     .expectPullRequestMerge(200)
-    .expectBranchRequest(expectedNewBranch, 'STAGING_COMMIT_SHA')
-    .expectCommitRequest(
-      'STAGING_COMMIT_SHA',
-      `release: cut the v${expectedVersion} release\n\nPR Close #200.`,
-    )
-    .expectCommitCompareRequest('PRE_STAGING_SHA', 'STAGING_COMMIT_SHA', {
-      status: 'ahead',
-      ahead_by: 1,
+    .expectBranchRequest(expectedNewBranch, {
+      sha: 'STAGING_COMMIT_SHA',
+      parents: [{sha: 'PRE_STAGING_SHA'}],
+      commit: {message: `release: cut the v${expectedVersion} release\n\nPR Close #200.`},
     })
     .expectTagToBeCreated(expectedTagName, 'STAGING_COMMIT_SHA')
     .expectReleaseToBeCreated(
@@ -64,9 +60,7 @@
 
   // In the fork, we make the following branches appear as non-existent,
   // so that the PRs can be created properly without collisions.
-  fork
-    .expectBranchRequest(expectedStagingForkBranch, null)
-    .expectBranchRequest(expectedNextUpdateBranch, null);
+  fork.expectBranchRequest(expectedStagingForkBranch).expectBranchRequest(expectedNextUpdateBranch);
 
   return {expectedNextUpdateBranch, expectedStagingForkBranch, expectedTagName};
 }

ng-dev/release/publish/test/common.spec.ts

@@ -185,11 +185,10 @@ describe('common release action logic', () => {
       const customRegistryUrl = 'https://custom-npm-registry.google.com';
 
       repo
-        .expectBranchRequest(branchName, 'STAGING_SHA')
-        .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
-        .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
-          status: 'ahead',
-          ahead_by: 1,
+        .expectBranchRequest(branchName, {
+          sha: 'STAGING_SHA',
+          parents: [{sha: 'BEFORE_STAGING_SHA'}],
+          commit: {message: `release: cut the v${version} release`},
         })
         .expectTagToBeCreated(tagName, 'STAGING_SHA')
         .expectReleaseToBeCreated(version.toString(), tagName, true);
@@ -231,11 +230,10 @@ describe('common release action logic', () => {
         .commit('feat(test): second commit');
 
       repo
-        .expectBranchRequest(branchName, 'STAGING_SHA')
-        .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
-        .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
-          status: 'ahead',
-          ahead_by: 1,
+        .expectBranchRequest(branchName, {
+          sha: 'STAGING_SHA',
+          parents: [{sha: 'BEFORE_STAGING_SHA'}],
+          commit: {message: `release: cut the v${version} release`},
         })
         .expectTagToBeCreated(tagName, 'STAGING_SHA')
         .expectReleaseToBeCreated(
@@ -285,13 +283,15 @@ describe('common release action logic', () => {
       git.commit('feat(test): first commit');
 
       repo
-        .expectBranchRequest(branchName, 'STAGING_SHA')
-        .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
+        .expectBranchRequest(branchName, {
+          sha: 'STAGING_SHA',
+          commit: {message: `release: cut the v${version} release`},
+        })
         .expectCommitStatusCheck('STAGING_SHA', 'success')
         .expectFindForkRequest(fork)
         .expectPullRequestToBeCreated(branchName, fork, 'release-stage-10.1.0-next.0', 10);
 
-      fork.expectBranchRequest('release-stage-10.1.0-next.0', null);
+      fork.expectBranchRequest('release-stage-10.1.0-next.0');
 
       const stagingPromise = instance.testStagingWithBuild(
         version,
@@ -334,11 +334,10 @@ describe('common release action logic', () => {
       );
 
       repo
-        .expectBranchRequest(branchName, 'STAGING_SHA')
-        .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
-        .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
-          status: 'ahead',
-          ahead_by: 1,
+        .expectBranchRequest(branchName, {
+          sha: 'STAGING_SHA',
+          parents: [{sha: 'BEFORE_STAGING_SHA'}],
+          commit: {message: `release: cut the v${version} release`},
         })
         .expectTagToBeCreated(tagName, 'STAGING_SHA')
         .expectReleaseToBeCreated(
@@ -366,13 +365,11 @@ describe('common release action logic', () => {
       );
       const {version, branchName} = baseReleaseTrains.latest;
 
-      repo
-        .expectBranchRequest(branchName, 'STAGING_SHA')
-        .expectCommitRequest('STAGING_SHA', `release: cut the v${version} release`)
-        .expectCommitCompareRequest('BEFORE_STAGING_SHA', 'STAGING_SHA', {
-          status: 'ahead',
-          ahead_by: 2, // this implies that another unknown/new commit is in between.
-        });
+      repo.expectBranchRequest(branchName, {
+        sha: 'STAGING_SHA',
+        parents: [{sha: 'THE_ONE_BEFORE_STAGING_SHA'}],
+        commit: {message: `release: cut the v${version} release`},
+      });
 
       spyOn(Log, 'error');
 
@@ -458,7 +455,7 @@ describe('common release action logic', () => {
         .expectPullRequestMerge(200);
 
       // Simulate that the fork branch name is available.
-      fork.expectBranchRequest(forkBranchName, null);
+      fork.expectBranchRequest(forkBranchName);
 
       await instance.testCherryPickWithPullRequest(version, branchName);
 
@@ -488,7 +485,7 @@ describe('common release action logic', () => {
         .expectPullRequestMergeCheck(200, true);
 
       // Simulate that the fork branch name is available.
-      fork.expectBranchRequest(forkBranchName, null);
+      fork.expectBranchRequest(forkBranchName);
 
       await instance.testCherryPickWithPullRequest(version, branchName);
 
@@ -510,7 +507,7 @@ describe('common release action logic', () => {
         .expectPullRequestMerge(200);
 
       // Simulate that the fork branch name is available.
-      fork.expectBranchRequest(forkBranchName, null);
+      fork.expectBranchRequest(forkBranchName);
 
       await instance.testCherryPickWithPullRequest(version, branchName);
 

ng-dev/release/publish/test/configure-next-as-major.spec.ts

@@ -97,7 +97,7 @@
     // We first mock the commit status check for the next branch, then expect two pull
     // requests from a fork that are targeting next and the new feature-freeze branch.
     repo
-      .expectBranchRequest('master', 'MASTER_COMMIT_SHA')
+      .expectBranchRequest('master', {sha: 'MASTER_COMMIT_SHA'})
       .expectCommitStatusCheck('MASTER_COMMIT_SHA', 'success')
       .expectFindForkRequest(fork)
       .expectPullRequestToBeCreated('master', fork, expectedForkBranch, 200)
@@ -106,7 +106,7 @@
 
     // In the fork, we make the staging branch appear as non-existent,
     // so that the PR can be created properly without collisions.
-    fork.expectBranchRequest(expectedForkBranch, null);
+    fork.expectBranchRequest(expectedForkBranch);
 
     await action.instance.perform();
 

ng-dev/release/publish/test/exceptional-minor/prepare-exceptional-minor.spec.ts

@@ -86,7 +86,7 @@ describe('cut exceptional minor next release candidate action', () => {
       );
 
       repo
-        .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
+        .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
         .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
 
       await instance.perform();
@@ -119,7 +119,7 @@ describe('cut exceptional minor next release candidate action', () => {
       );
 
       repo
-        .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
+        .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
         .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
 
       await instance.perform();
@@ -152,7 +152,7 @@ describe('cut exceptional minor next release candidate action', () => {
       );
 
       repo
-        .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
+        .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
         .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
 
       await instance.perform();
@@ -185,7 +185,7 @@ describe('cut exceptional minor next release candidate action', () => {
       );
 
       repo
-        .expectBranchRequest('14.2.x', 'PATCH_LATEST_SHA')
+        .expectBranchRequest('14.2.x', {sha: 'PATCH_LATEST_SHA'})
         .expectCommitStatusCheck('PATCH_LATEST_SHA', 'success');
 
       await instance.perform();