Merge pull request #858 from github/use-better-base-sha

Declare the merge base as base for code scanning comparisons

Author: cannist

Diff for: lib/actions-util.js

@@ -90,6 +90,66 @@ export const getCommitOid = async function (ref = "HEAD"): Promise<string> {
   }
 };
 
+/**
+ * If the action was triggered by a pull request, determine the commit sha of the merge base.
+ * Returns undefined if run by other triggers or the merge base cannot be determined.
+ */
+export const determineMergeBaseCommitOid = async function (): Promise<
+  string | undefined
+> {
+  if (process.env.GITHUB_EVENT_NAME !== "pull_request") {
+    return undefined;
+  }
+
+  const mergeSha = getRequiredEnvParam("GITHUB_SHA");
+
+  try {
+    let commitOid = "";
+    let baseOid = "";
+    let headOid = "";
+
+    await new toolrunner.ToolRunner(
+      await safeWhich.safeWhich("git"),
+      ["show", "-s", "--format=raw", mergeSha],
+      {
+        silent: true,
+        listeners: {
+          stdline: (data) => {
+            if (data.startsWith("commit ") && commitOid === "") {
+              commitOid = data.substring(7);
+            } else if (data.startsWith("parent ")) {
+              if (baseOid === "") {
+                baseOid = data.substring(7);
+              } else if (headOid === "") {
+                headOid = data.substring(7);
+              }
+            }
+          },
+          stderr: (data) => {
+            process.stderr.write(data);
+          },
+        },
+      }
+    ).exec();
+
+    // Let's confirm our assumptions: We had a merge commit and the parsed parent data looks correct
+    if (
+      commitOid === mergeSha &&
+      headOid.length === 40 &&
+      baseOid.length === 40
+    ) {
+      return baseOid;
+    }
+    return undefined;
+  } catch (e) {
+    core.info(
+      `Failed to call git to determine merge base. Continuing with data from environment: ${e}`
+    );
+    core.info((e as Error).stack || "NO STACK");
+    return undefined;
+  }
+};
+
 interface WorkflowJobStep {
   run: any;
 }

Diff for: lib/actions-util.js.map

@@ -57,14 +57,17 @@ test("validate correct payload used per version", async (t) => {
       "/opt/src",
       undefined,
       ["CodeQL", "eslint"],
-      version
+      version,
+      "mergeBaseCommit"
     );
     // Not triggered by a pull request
     t.falsy(payload.base_ref);
     t.falsy(payload.base_sha);
   }
 
   process.env["GITHUB_EVENT_NAME"] = "pull_request";
+  process.env["GITHUB_SHA"] = "commit";
+  process.env["GITHUB_BASE_REF"] = "master";
   process.env[
     "GITHUB_EVENT_PATH"
   ] = `${__dirname}/../src/testdata/pull_request.json`;
@@ -79,8 +82,29 @@ test("validate correct payload used per version", async (t) => {
       "/opt/src",
       undefined,
       ["CodeQL", "eslint"],
-      version
+      version,
+      "mergeBaseCommit"
     );
+    // Uploads for a merge commit use the merge base
+    t.deepEqual(payload.base_ref, "refs/heads/master");
+    t.deepEqual(payload.base_sha, "mergeBaseCommit");
+  }
+
+  for (const version of newVersions) {
+    const payload: any = uploadLib.buildPayload(
+      "headCommit",
+      "refs/pull/123/head",
+      "key",
+      undefined,
+      "",
+      undefined,
+      "/opt/src",
+      undefined,
+      ["CodeQL", "eslint"],
+      version,
+      "mergeBaseCommit"
+    );
+    // Uploads for the head use the PR base
     t.deepEqual(payload.base_ref, "refs/heads/master");
     t.deepEqual(payload.base_sha, "f95f852bd8fca8fcc58a9a2d6c842781e32a215e");
   }
@@ -96,7 +120,8 @@ test("validate correct payload used per version", async (t) => {
       "/opt/src",
       undefined,
       ["CodeQL", "eslint"],
-      version
+      version,
+      "mergeBaseCommit"
     );
     // These older versions won't expect these values
     t.falsy(payload.base_ref);

Diff for: lib/upload-lib.js

@@ -291,7 +291,8 @@ export function buildPayload(
   checkoutURI: string,
   environment: string | undefined,
   toolNames: string[],
-  gitHubVersion: util.GitHubVersion
+  gitHubVersion: util.GitHubVersion,
+  mergeBaseCommitOid: string | undefined
 ) {
   if (util.isActions()) {
     const payloadObj = {
@@ -314,15 +315,28 @@ export function buildPayload(
       gitHubVersion.type !== util.GitHubVariant.GHES ||
       semver.satisfies(gitHubVersion.version, `>=3.1`)
     ) {
-      if (
-        process.env.GITHUB_EVENT_NAME === "pull_request" &&
-        process.env.GITHUB_EVENT_PATH
-      ) {
-        const githubEvent = JSON.parse(
-          fs.readFileSync(process.env.GITHUB_EVENT_PATH, "utf8")
-        );
-        payloadObj.base_ref = `refs/heads/${githubEvent.pull_request.base.ref}`;
-        payloadObj.base_sha = githubEvent.pull_request.base.sha;
+      if (process.env.GITHUB_EVENT_NAME === "pull_request") {
+        if (
+          commitOid === util.getRequiredEnvParam("GITHUB_SHA") &&
+          mergeBaseCommitOid
+        ) {
+          // We're uploading results for the merge commit
+          // and were able to determine the merge base.
+          // So we use that as the most accurate base.
+          payloadObj.base_ref = `refs/heads/${util.getRequiredEnvParam(
+            "GITHUB_BASE_REF"
+          )}`;
+          payloadObj.base_sha = mergeBaseCommitOid;
+        } else if (process.env.GITHUB_EVENT_PATH) {
+          // Either we're not uploading results for the merge commit
+          // or we could not determine the merge base.
+          // Using the PR base is the only option here
+          const githubEvent = JSON.parse(
+            fs.readFileSync(process.env.GITHUB_EVENT_PATH, "utf8")
+          );
+          payloadObj.base_ref = `refs/heads/${githubEvent.pull_request.base.ref}`;
+          payloadObj.base_sha = githubEvent.pull_request.base.sha;
+        }
       }
     }
     return payloadObj;
@@ -389,7 +403,8 @@ async function uploadFiles(
     checkoutURI,
     environment,
     toolNames,
-    gitHubVersion
+    gitHubVersion,
+    await actionsUtil.determineMergeBaseCommitOid()
   );
 
   // Log some useful debug info about the info