fix(actions): handle merge commits in verify-signed-commit-authors

.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.mjs

@@ -8,6 +8,15 @@ const DEFAULT_ALLOWED_SIGNERS_RELATIVE_PATH = '../../signing/allowed_signers';
 const FULL_SHA_PATTERN = /^[0-9a-fA-F]{40}$/;
 const INTEGER_PATTERN = /^[0-9]+$/;
 const LINE_SPLIT_PATTERN = /\r?\n/;
+// A merge commit in the PR branch (e.g. from merging the base branch in)
+// can put the true merge-base much further back on the merged-in side than
+// the initial shallow fetch's depth budget reaches, since that budget is
+// sized for a linear history of prCommitCount commits. When that happens,
+// rev-list can't prove shared ancestry across the shallow boundary and
+// overcounts. Retry with a deepened fetch a few times before paying for a
+// full unshallow fetch, so the common (linear-history) case stays cheap.
+const MAX_DEEPEN_ATTEMPTS = 4;
+const DEEPEN_MULTIPLIER = 4;
 
 const enforce = process.env.SIGNED_COMMIT_ENFORCE !== 'false';
 
@@ -63,6 +72,7 @@ function main() {
       expectedCommitCount: prCommitCount,
       prBaseSha,
       prHeadSha,
+      prNumber,
       workspace,
     });
     return;
@@ -140,18 +150,22 @@ function ensureGitRepository(workspace) {
   }
 }
 
-function fetchFromOrigin(ref, workspace) {
-  const token = process.env.GITHUB_TOKEN;
-  if (!token) {
-    return;
-  }
+function authConfigArgs(token) {
   const authHeader = Buffer.from(`x-access-token:${token}`, 'utf8').toString(
     'base64',
   );
-  const configArgs = [
+  return [
     '-c',
     `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`,
   ];
+}
+
+function fetchFromOrigin(ref, workspace) {
+  const token = process.env.GITHUB_TOKEN;
+  if (!token) {
+    return;
+  }
+  const configArgs = authConfigArgs(token);
 
   if (FULL_SHA_PATTERN.test(ref)) {
     git([...configArgs, 'fetch', '--no-tags', 'origin', ref], workspace);
@@ -184,18 +198,8 @@ function unshallowFromOrigin(workspace) {
   if (!token) {
     return;
   }
-  const authHeader = Buffer.from(`x-access-token:${token}`, 'utf8').toString(
-    'base64',
-  );
   git(
-    [
-      '-c',
-      `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`,
-      'fetch',
-      '--unshallow',
-      '--no-tags',
-      'origin',
-    ],
+    [...authConfigArgs(token), 'fetch', '--unshallow', '--no-tags', 'origin'],
     workspace,
   );
 }
@@ -239,21 +243,11 @@ function fetchPullRequestCommits({
   workspace,
 }) {
   const token = requiredEnv('GITHUB_TOKEN');
-  const authHeader = Buffer.from(`x-access-token:${token}`, 'utf8').toString(
-    'base64',
-  );
 
   ensureGitRepository(workspace);
 
   const fetchBase = git(
-    [
-      '-c',
-      `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`,
-      'fetch',
-      '--no-tags',
-      'origin',
-      prBaseSha,
-    ],
+    [...authConfigArgs(token), 'fetch', '--no-tags', 'origin', prBaseSha],
     workspace,
   );
   if (!fetchBase.ok) {
@@ -262,8 +256,7 @@ function fetchPullRequestCommits({
 
   const fetch = git(
     [
-      '-c',
-      `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`,
+      ...authConfigArgs(token),
       'fetch',
       '--no-tags',
       `--depth=${prCommitCount + 1}`,
@@ -290,32 +283,92 @@ function verifyPullRequestCommits({
   expectedCommitCount,
   prBaseSha,
   prHeadSha,
+  prNumber,
   workspace,
 }) {
+  const commits = resolvePullRequestCommits({
+    expectedCommitCount,
+    prBaseSha,
+    prHeadSha,
+    prNumber,
+    workspace,
+  });
+
+  verifyCommits(commits, allowedSignersPath, workspace);
+
+  workflowCommand(
+    'notice',
+    `All ${commits.length} PR commit(s) are signed by allowed SSH keys.`,
+  );
+}
+
+function listCommitsInRange(baseSha, headSha, workspace) {
   const revList = git(
-    ['rev-list', '--reverse', `${prBaseSha}..${prHeadSha}`],
+    ['rev-list', '--reverse', `${baseSha}..${headSha}`],
     workspace,
   );
   if (!revList.ok) {
     fail(`Could not list PR commits locally: ${commandDetails(revList)}`);
   }
+  return revList.stdout.trim().split(LINE_SPLIT_PATTERN).filter(Boolean);
+}
+
+function deepenPullRequestFetch({deepenBy, prNumber, workspace}) {
+  const token = requiredEnv('GITHUB_TOKEN');
+  const deepen = git(
+    [
+      ...authConfigArgs(token),
+      'fetch',
+      '--no-tags',
+      `--deepen=${deepenBy}`,
+      'origin',
+      `+refs/pull/${prNumber}/head:refs/remotes/pull/${prNumber}/head`,
+    ],
+    workspace,
+  );
+  if (!deepen.ok) {
+    fail(`Could not deepen PR history: ${commandDetails(deepen)}`);
+  }
+}
+
+function resolvePullRequestCommits({
+  expectedCommitCount,
+  prBaseSha,
+  prHeadSha,
+  prNumber,
+  workspace,
+}) {
+  let commits = listCommitsInRange(prBaseSha, prHeadSha, workspace);
+
+  let deepenBy = Math.max(expectedCommitCount, 1) * DEEPEN_MULTIPLIER;
+  for (
+    let attempt = 0;
+    commits.length !== expectedCommitCount && attempt < MAX_DEEPEN_ATTEMPTS;
+    attempt++
+  ) {
+    deepenPullRequestFetch({deepenBy, prNumber, workspace});
+    commits = listCommitsInRange(prBaseSha, prHeadSha, workspace);
+    deepenBy *= DEEPEN_MULTIPLIER;
+  }
+
+  // Guaranteed correct (no shallow boundary left to hide shared ancestry),
+  // but expensive, so it's only a last resort after deepening fails to
+  // converge.
+  if (
+    commits.length !== expectedCommitCount &&
+    isShallowRepository(workspace)
+  ) {
+    unshallowFromOrigin(workspace);
+    commits = listCommitsInRange(prBaseSha, prHeadSha, workspace);
+  }
 
-  const commits = revList.stdout
-    .trim()
-    .split(LINE_SPLIT_PATTERN)
-    .filter(Boolean);
   if (commits.length !== expectedCommitCount) {
     fail(
       `Expected ${expectedCommitCount} PR commit(s) from the pull_request payload, but git rev-list found ${commits.length}.`,
     );
   }
 
-  verifyCommits(commits, allowedSignersPath, workspace);
-
-  workflowCommand(
-    'notice',
-    `All ${commits.length} PR commit(s) are signed by allowed SSH keys.`,
-  );
+  return commits;
 }
 
 function verifyCommitRange({

.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.test.mjs

@@ -18,6 +18,7 @@ const scriptPath = fileURLToPath(
 const baseSha = '1111111111111111111111111111111111111111';
 const firstCommit = '2222222222222222222222222222222222222222';
 const secondCommit = '3333333333333333333333333333333333333333';
+const phantomCommit = '4444444444444444444444444444444444444444';
 const headSha = secondCommit;
 const activeAllowedSigners =
   'alice@example.com namespaces="git" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAITestKey\n';
@@ -73,6 +74,78 @@ test('accepts pull request commits signed by allowed SSH keys', () => {
   );
 });
 
+test('deepens the shallow fetch and retries when a merge commit undercounts the initial depth', () => {
+  const result = runVerifier({
+    env: {
+      FAKE_GIT_REQUIRE_DEEPENS: '1',
+    },
+  });
+
+  assert.equal(result.status, 0, result.stderr || result.stdout);
+  assert.match(
+    result.stdout,
+    /::notice::All 2 PR commit\(s\) are signed by allowed SSH keys\./,
+  );
+
+  const calls = readToolCalls(result.dir);
+  const deepenCalls = calls.filter(
+    call =>
+      call.command === 'git' &&
+      call.args.some(arg => arg.startsWith('--deepen=')),
+  );
+  assert.equal(
+    deepenCalls.length,
+    1,
+    'expected exactly one deepen fetch before the count converged',
+  );
+});
+
+test('falls back to a full unshallow fetch when deepening does not converge, and still succeeds if that fixes it', () => {
+  const result = runVerifier({
+    env: {
+      FAKE_GIT_REQUIRE_DEEPENS: '100',
+      FAKE_GIT_SHALLOW: '1',
+    },
+  });
+
+  assert.equal(result.status, 0, result.stderr || result.stdout);
+  assert.match(
+    result.stdout,
+    /::notice::All 2 PR commit\(s\) are signed by allowed SSH keys\./,
+  );
+
+  const calls = readToolCalls(result.dir);
+  const deepenCalls = calls.filter(
+    call =>
+      call.command === 'git' &&
+      call.args.some(arg => arg.startsWith('--deepen=')),
+  );
+  assert.equal(
+    deepenCalls.length,
+    4,
+    'expected all deepen attempts to be exhausted before falling back',
+  );
+  const unshallowCall = calls.find(
+    call => call.command === 'git' && call.args.includes('--unshallow'),
+  );
+  assert.ok(unshallowCall, 'expected git fetch --unshallow as a last resort');
+});
+
+test('fails closed when the commit count still does not match after deepening and unshallowing', () => {
+  const result = runVerifier({
+    env: {
+      FAKE_GIT_ALWAYS_MISMATCH: '1',
+      FAKE_GIT_SHALLOW: '1',
+    },
+  });
+
+  assert.equal(result.status, 1);
+  assert.match(
+    result.stdout,
+    /::error::Expected 2 PR commit\(s\) from the pull_request payload, but git rev-list found 3\./,
+  );
+});
+
 test('rejects a commit whose signature is not made by an allowed key', () => {
   const result = runVerifier({
     env: {
@@ -432,7 +505,7 @@ function writeToolStubs(binDir, dir) {
   writeExecutable(
     join(binDir, 'git'),
     `#!/usr/bin/env node
-import {appendFileSync, existsSync} from 'node:fs';
+import {appendFileSync, existsSync, readFileSync} from 'node:fs';
 
 const allArgs = process.argv.slice(2);
 appendFileSync(
@@ -459,6 +532,9 @@ if (command === 'fetch') {
     }
     appendFileSync(${JSON.stringify(join(dir, 'unshallow-called'))}, '1');
   }
+  if (args.some(arg => arg.startsWith('--deepen='))) {
+    appendFileSync(${JSON.stringify(join(dir, 'deepen-called'))}, '1');
+  }
   process.exit(0);
 }
 
@@ -507,7 +583,25 @@ if (command === 'rev-list') {
   if (process.env.FAKE_GIT_EMPTY_REV_LIST) {
     process.exit(0);
   }
-  process.stdout.write(${JSON.stringify(`${firstCommit}\n${secondCommit}\n`)});
+  const normal = ${JSON.stringify(`${firstCommit}\n${secondCommit}\n`)};
+  const withPhantom = ${JSON.stringify(`${phantomCommit}\n${firstCommit}\n${secondCommit}\n`)};
+  if (process.env.FAKE_GIT_ALWAYS_MISMATCH) {
+    process.stdout.write(withPhantom);
+    process.exit(0);
+  }
+  const requiredDeepens = Number(process.env.FAKE_GIT_REQUIRE_DEEPENS || 0);
+  if (requiredDeepens > 0) {
+    const deepenCallsPath = ${JSON.stringify(join(dir, 'deepen-called'))};
+    const deepenCalls = existsSync(deepenCallsPath)
+      ? readFileSync(deepenCallsPath, 'utf8').length
+      : 0;
+    const unshallowed = existsSync(${JSON.stringify(join(dir, 'unshallow-called'))});
+    if (!unshallowed && deepenCalls < requiredDeepens) {
+      process.stdout.write(withPhantom);
+      process.exit(0);
+    }
+  }
+  process.stdout.write(normal);
   process.exit(0);
 }