Add PR annotations for validate, format, and types (#2345)

Lint errors already show up in the GitHub changes view. This makes
errors from the validate, format, and a new type checking step also show
up inline when applicable. Type checking has hundreds of false positives
so it's not required to pass. It'll also only show warnings from files
modified by the PR.

GitHub's annotation feature is very poorly designed and implemented, so
this has a lot of restrictions, such as 10 annotation limit per job.
Anything beyond that is silently ignored because of course. (This also
isn't documented in GitHub's user-facing docs anywhere, because again of
course)

Author: GarboMuffin

.github/workflows/validate.yml

@@ -35,7 +35,7 @@ jobs:
           cache: npm
       - name: Install dependencies
         run: npm ci
-      - name: Validate
+      - name: Lint
         run: npm run lint
 
   format:
@@ -52,5 +52,33 @@ jobs:
           cache: npm
       - name: Install dependencies
         run: npm ci
-      - name: Validate
-        run: npm run check-format
+      - name: Check format
+        shell: bash
+        run: |
+          # Rewrite the prettier output format into the GitHub Actions annotation format
+          # This is a hack, but this is easier than reimplementing the prettier CLI's ignore
+          # rules ourselves.
+          NO_COLOR=1 npm run check-format 2> >(tee >( \
+            grep -v 'Run Prettier with --write to fix' | \
+            sed -e 's/^\[warn] \(.*\)$/::warning file=\1,line=1::File was not formatted with prettier. Usually, commenting !format will fix this./' \
+          ))
+
+  type-warnings:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          persist-credentials: false
+      - name: Install Node.js
+        uses: actions/setup-node@v4
+        with:
+          node-version: 22
+          cache: npm
+      - name: Install dependencies
+        run: npm ci
+      - name: Generate type warnings
+        run: node development/ci-generate-type-warnings.js
+        env:
+          PR_NUMBER: "${{ github.event.number }}"
+          GH_REPO: "${{ github.repository }}"

development/builder.js

@@ -960,12 +960,13 @@ class Builder {
   async validate() {
     const errors = [];
     const build = await this.build();
-    for (const [fileName, file] of Object.entries(build.files)) {
+    for (const [outputFileName, file] of Object.entries(build.files)) {
       try {
         await file.validate();
       } catch (e) {
         errors.push({
-          fileName,
+          outputFileName,
+          inputFilePath: file.sourcePath,
           error: e,
         });
       }

development/ci-generate-type-warnings.js

@@ -0,0 +1,88 @@
+import pathUtil from "node:path";
+import ts from "typescript";
+import { createAnnotation, getChangedFiles, isCI } from "./ci-interop.js";
+
+/**
+ * @fileoverview Generates CI annotations for type warnings in files modified by the
+ * pull request. Standard TypeScript CLI will include all files and its output is
+ * detected as errors, so not usable for us.
+ */
+
+const check = async () => {
+  const rootDir = pathUtil.join(import.meta.dirname, "..");
+
+  const changedFiles = Array.from(await getChangedFiles()).sort();
+  const changedFilesAbsolute = changedFiles.map((f) =>
+    pathUtil.join(rootDir, f)
+  );
+
+  console.log(`${changedFiles.size} changed files:`);
+  console.log(Array.from(changedFiles).sort().join("\n"));
+  console.log("");
+
+  const tsconfigPath = pathUtil.join(rootDir, "tsconfig.json");
+  const commandLine = ts.getParsedCommandLineOfConfigFile(
+    tsconfigPath,
+    {},
+    ts.sys
+  );
+
+  const program = ts.createProgram({
+    rootNames: commandLine.fileNames,
+    options: commandLine.options,
+  });
+  const emitted = program.emit();
+
+  const diagnostics = [
+    ...ts.getPreEmitDiagnostics(program),
+    ...emitted.diagnostics,
+  ];
+
+  let numWarnings = 0;
+
+  for (const diagnostic of diagnostics) {
+    if (!changedFilesAbsolute.includes(diagnostic.file.fileName)) {
+      continue;
+    }
+
+    const startPosition = ts.getLineAndCharacterOfPosition(
+      diagnostic.file,
+      diagnostic.start
+    );
+    const endPosition = ts.getLineAndCharacterOfPosition(
+      diagnostic.file,
+      diagnostic.start
+    );
+    // Won't be the most readable, but it's probably fine.
+    const flattened = ts.flattenDiagnosticMessageText(
+      diagnostic.messageText,
+      " ",
+      0
+    );
+
+    numWarnings++;
+    createAnnotation({
+      type: "warning",
+      file: diagnostic.file.fileName,
+      title: "Type warning - may indicate a bug - ignore if no bug",
+      onlyIfChanged: true,
+      message: flattened,
+      line: startPosition.line + 1,
+      col: startPosition.character + 1,
+      endLine: endPosition.line + 1,
+      endCol: endPosition.character + 1,
+    });
+  }
+
+  console.log(`Warnings in changed files: ${numWarnings}`);
+  console.log(`Warnings in all files: ${diagnostics.length}`);
+};
+
+if (isCI()) {
+  check();
+} else {
+  console.error(
+    "This script is only intended to be used in CI. For development, use normal TypeScript CLI instead."
+  );
+  process.exit(1);
+}

development/ci-interop.js

@@ -0,0 +1,95 @@
+import pathUtil from "node:path";
+
+/**
+ * @fileoverview Various utilities related to integrating with CI.
+ */
+
+// https://github.com/actions/toolkit/blob/main/docs/problem-matchers.md#limitations
+export const MAX_ANNOTATIONS_PER_TYPE_PER_STEP = 10;
+export const MAX_ANNOTATIONS_PER_JOB = 50;
+
+/**
+ * @returns {boolean} true if running in CI (GitHub Actions, etc.)
+ */
+export const isCI = () => !!process.env.CI;
+
+/**
+ * Note: PR may have changed between when this CI job was queued and when this job runs.
+ * So, don't use this for security-critical things where accuracy is a must.
+ * @returns {Promise<Set<string>>} List of paths (relative to root without leading / or ./) changed by this PR.
+ */
+export const getChangedFiles = async () => {
+  if (!isCI()) {
+    return [];
+  }
+
+  const prNumber = +process.env.PR_NUMBER;
+  if (!prNumber) {
+    throw new Error("Missing PR_NUMBER");
+  }
+
+  const repo = process.env.GH_REPO;
+  if (typeof repo !== "string" || !repo.includes("/")) {
+    throw new Error("Missing GH_REPO");
+  }
+
+  const diffResponse = await fetch(
+    `https://patch-diff.githubusercontent.com/raw/${repo}/pull/${prNumber}.diff`
+  );
+  const diffText = await diffResponse.text();
+  const fileMatches = [
+    ...diffText.matchAll(/^(?:---|\+\+\+) [ab]\/(.+)$/gm),
+  ].map((match) => match[1]);
+
+  return new Set(fileMatches);
+};
+
+/**
+ * @typedef Annotation
+ * @property {'notice'|'warning'|'error'} type
+ * @property {string} file Absolute path to file or relative from repository root
+ * @property {string} title
+ * @property {string} message
+ * @property {number} [line] 1-indexed
+ * @property {number} [col] 1-indexed
+ * @property {number} [endLine] 1-indexed
+ * @property {number} [endCol] 1-indexed
+ */
+
+/**
+ * @param {Annotation} annotation
+ */
+export const createAnnotation = (annotation) => {
+  const rootDir = pathUtil.join(import.meta.dirname, "..");
+  const relativeFileName = pathUtil.relative(rootDir, annotation.file);
+
+  // https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands
+
+  let output = "";
+  output += `::${annotation.type} `;
+  output += `file=${relativeFileName}`;
+
+  // Documentation says line number is not required, but in practice that gets interpreted as
+  // line 0 and ends up not showing up. So, we'll default to line 1.
+  if (typeof annotation.line === "number") {
+    output += `,line=${annotation.line}`;
+  } else {
+    output += ",line=1";
+  }
+
+  // These are all actually optional.
+  if (typeof annotation.col === "number") {
+    output += `,col=${annotation.col}`;
+  }
+  if (typeof annotation.endLine === "number") {
+    output += `,endLine=${annotation.endLine}`;
+  }
+  if (typeof annotation.endCol === "number") {
+    output += `,endCol=${annotation.endCol}`;
+  }
+
+  output += `,title=${annotation.title}::`;
+  output += annotation.message;
+
+  console.log(output);
+};

development/validate.js

@@ -1,9 +1,13 @@
+import pathUtil from "node:path";
 import Builder from "./builder.js";
+import { createAnnotation, isCI } from "./ci-interop.js";
 import * as Colors from "./colors.js";
 
 const builder = new Builder("production");
 const errors = await builder.validate();
 
+const rootDir = pathUtil.join(import.meta.dirname, "..");
+
 if (errors.length === 0) {
   console.log(
     `${Colors.GREEN}${Colors.BOLD}Validation checks passed.${Colors.RESET}`
@@ -15,10 +19,25 @@ if (errors.length === 0) {
       errors.length === 1 ? "file" : "files"
     } failed validation.${Colors.RESET}`
   );
+
   console.error("");
-  for (const { fileName, error } of errors) {
-    console.error(`${Colors.BOLD}${fileName}${Colors.RESET}: ${error}`);
+
+  for (const { outputFileName, inputFilePath, error } of errors) {
+    const displayName = inputFilePath
+      ? pathUtil.relative(rootDir, inputFilePath)
+      : outputFileName;
+    console.error(`${Colors.BOLD}${displayName}${Colors.RESET}: ${error}`);
+
+    if (isCI() && inputFilePath) {
+      createAnnotation({
+        type: "error",
+        file: inputFilePath,
+        title: "Validation error",
+        message: error,
+      });
+    }
   }
-  console.error(``);
+
+  console.error("");
   process.exit(1);
 }

package-lock.json

@@ -43,7 +43,8 @@
   "devDependencies": {
     "@transifex/api": "^7.1.5",
     "prettier": "^3.6.0",
-    "spdx-expression-parse": "^4.0.0"
+    "spdx-expression-parse": "^4.0.0",
+    "typescript": "^5.9.3"
   },
   "private": true
 }