fix(ci): stop the i18n ratchet failing PRs on files they never touched (#2165)

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

.github/workflows/frontend-tests.yml

@@ -104,8 +104,16 @@ jobs:
         run: |
           set -euo pipefail
 
+          # On `pull_request` the checkout is the MERGE REF, whose first parent is
+          # the base branch tip. `pull_request.base.sha` is the base tip from when
+          # the PR event fired, so it is an ANCESTOR of that merge ref — which
+          # makes `base...HEAD` collapse to a two-dot diff and list everything
+          # main landed since the PR opened. Over-broad is harmless here (the
+          # extra files are already formatted), but it is the same trap the i18n
+          # ratchet below hit for real, so do not copy the old idiom. The fork
+          # point is what "changed in this PR" actually means.
           if [[ "${{ github.event_name }}" == "pull_request" ]]; then
-            BASE_SHA="${{ github.event.pull_request.base.sha }}"
+            BASE_SHA="$(git merge-base HEAD "origin/${{ github.base_ref }}")"
           else
             BASE_SHA="${{ github.event.before }}"
           fi
@@ -115,7 +123,7 @@ jobs:
           fi
 
           CHANGED_FILES="$(
-            git diff --name-only --diff-filter=ACMRT "${BASE_SHA}"..."${{ github.sha }}" \
+            git diff --name-only --diff-filter=ACMRT "${BASE_SHA}" HEAD \
             | grep -E '^apps/(web|cli|packages)/.*\.(ts|tsx|js|jsx|mjs|cjs|json|md|mdx|yml|yaml|css|scss)$' \
             || true
           )"
@@ -160,8 +168,7 @@ jobs:
       # These two judge the CHANGE instead of the FILE: an added file must be
       # clean outright, a modified file only on the lines it touched, and the
       # guard allowlist may not shrink. Same model as
-      # `golangci-lint --new-from-rev` for Go. Base SHA is resolved the same way
-      # as the Prettier step above.
+      # `golangci-lint --new-from-rev` for Go.
       - name: Run i18n new-code ratchet
         shell: bash
         # The job defaults to `apps`; these scripts live one level deeper.
@@ -169,19 +176,28 @@ jobs:
         run: |
           set -euo pipefail
 
+          # DO NOT pass --base on pull_request. The checkout is the merge ref
+          # `refs/pull/N/merge`, and `pull_request.base.sha` is the base branch tip
+          # from when the PR event fired — an ancestor of that merge ref. Handing
+          # it in made the ratchet judge every commit main landed since the PR
+          # opened as this PR's new code, failing PRs on files they never touched
+          # (#2160). The scripts' own default, `merge-base HEAD origin/main`,
+          # resolves to the merge ref's base-side parent and is correct here;
+          # origin/main is present because the checkout is fetch-depth: 0.
           if [[ "${{ github.event_name }}" == "pull_request" ]]; then
-            BASE_SHA="${{ github.event.pull_request.base.sha }}"
-          else
-            BASE_SHA="${{ github.event.before }}"
+            node scripts/run-i18n-ratchet.mjs
+            exit 0
           fi
 
+          # push: HEAD *is* the new base branch tip, so there is no fork point to
+          # find and the previous tip is the only meaningful base.
+          BASE_SHA="${{ github.event.before }}"
           if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then
             echo "No usable base SHA; skipping the new-code ratchet."
             exit 0
           fi
 
-          node scripts/check-new-code-i18n.mjs --base "${BASE_SHA}"
-          node scripts/check-guard-allowlist.mjs --base "${BASE_SHA}"
+          node scripts/run-i18n-ratchet.mjs --base "${BASE_SHA}"
 
       - name: Run tests
         run: pnpm --filter @kandev/web test

apps/web/scripts/check-new-code-i18n.mjs

@@ -26,37 +26,19 @@
  * Usage:
  *   node scripts/check-new-code-i18n.mjs [--base <ref-or-sha>]
  *
- * With no --base it uses `git merge-base HEAD origin/main`. CI passes the PR's
- * base SHA explicitly, matching the Prettier step in frontend-tests.yml.
+ * With no --base it uses `git merge-base HEAD origin/main`, which is the right
+ * answer on every checkout shape — including CI's `refs/pull/N/merge`, where it
+ * resolves to the merge ref's base-side parent. Prefer passing no --base. An
+ * explicit one is floored at that same fork point (see lib/git-base.mjs), because
+ * a base sha captured when a PR opened goes stale the moment main moves.
  */
 import path from "node:path";
 
 import { ESLint } from "eslint";
 
+import { changedFiles, webDiff } from "./lib/changed-files.mjs";
 import { lineInRanges, parseAddedLineRanges } from "./lib/diff-ranges.mjs";
-import { git, REPO_ROOT, resolveBase, toPosixPath, WEB_DIR } from "./lib/git-base.mjs";
-
-const WEB_PREFIX = "apps/web/";
-
-/** Only UI source. Tests build fixtures out of literals on purpose. */
-function isCandidate(repoPath) {
-  if (!repoPath.startsWith(WEB_PREFIX)) return false;
-  const rel = repoPath.slice(WEB_PREFIX.length);
-  if (!/\.tsx?$/.test(rel)) return false;
-  if (/\.(test|spec)\.tsx?$/.test(rel)) return false;
-  if (rel.startsWith("e2e/") || rel.startsWith("scripts/")) return false;
-  return /^(components|app|hooks|lib|src)\//.test(rel);
-}
-
-function changedFiles(base, filter) {
-  // --find-renames explicitly: rename detection defaults on since Git 2.9, but a
-  // repo with diff.renames=false would report a moved file as ADDED, and an
-  // added file is judged whole — demanding a full migration for a plain git mv.
-  return git(["diff", "--name-only", "--find-renames", `--diff-filter=${filter}`, base])
-    .split("\n")
-    .map((line) => line.trim())
-    .filter(isCandidate);
-}
+import { REPO_ROOT, resolveBase, toPosixPath, WEB_DIR } from "./lib/git-base.mjs";
 
 const resolved = resolveBase();
 if (resolved.skip) {
@@ -79,16 +61,7 @@ if (targets.length === 0) {
 }
 
 // Line attribution only matters for modified files; added files are judged whole.
-//
-// Scoped to apps/web rather than to the individual files, because narrowing the
-// pathspec to only the NEW path of a rename hides the matching deletion and git
-// then reports the whole file as added — which would demand a full migration for
-// a pure `git mv`. Keeping both sides in scope lets rename detection pair them,
-// so a pure rename yields no hunks and reports nothing.
-const addedRanges =
-  modified.length === 0
-    ? new Map()
-    : parseAddedLineRanges(git(["diff", "--unified=0", "--find-renames", base, "--", WEB_PREFIX]));
+const addedRanges = modified.length === 0 ? new Map() : parseAddedLineRanges(webDiff(base));
 
 const eslint = new ESLint({
   cwd: WEB_DIR,

apps/web/scripts/lib/changed-files.mjs

@@ -0,0 +1,49 @@
+/**
+ * Which files a change added or modified, and the raw diff its added-line ranges
+ * are parsed from.
+ *
+ * Split out of `check-new-code-i18n.mjs` so the selection can be tested against
+ * real git topologies — a merge ref, a rebased branch, a push — without needing
+ * ESLint or this repository's own history.
+ */
+import { git } from "./git-base.mjs";
+
+export const WEB_PREFIX = "apps/web/";
+
+/** Only UI source. Tests build fixtures out of literals on purpose. */
+export function isCandidate(repoPath) {
+  if (!repoPath.startsWith(WEB_PREFIX)) return false;
+  const rel = repoPath.slice(WEB_PREFIX.length);
+  if (!/\.tsx?$/.test(rel)) return false;
+  if (/\.(test|spec)\.tsx?$/.test(rel)) return false;
+  if (rel.startsWith("e2e/") || rel.startsWith("scripts/")) return false;
+  return /^(components|app|hooks|lib|src)\//.test(rel);
+}
+
+/**
+ * Candidate paths matching `filter` (a `--diff-filter` value) between `base` and
+ * the working tree.
+ *
+ * `--find-renames` explicitly: rename detection defaults on since Git 2.9, but a
+ * repo with diff.renames=false would report a moved file as ADDED, and an added
+ * file is judged whole — demanding a full migration for a plain `git mv`.
+ */
+export function changedFiles(base, filter, cwd) {
+  return git(["diff", "--name-only", "--find-renames", `--diff-filter=${filter}`, base], { cwd })
+    .split("\n")
+    .map((line) => line.trim())
+    .filter(isCandidate);
+}
+
+/**
+ * The zero-context diff that added-line attribution is parsed from.
+ *
+ * Scoped to apps/web rather than to the individual files, because narrowing the
+ * pathspec to only the NEW path of a rename hides the matching deletion and git
+ * then reports the whole file as added — which would demand a full migration for
+ * a pure `git mv`. Keeping both sides in scope lets rename detection pair them,
+ * so a pure rename yields no hunks and reports nothing.
+ */
+export function webDiff(base, cwd) {
+  return git(["diff", "--unified=0", "--find-renames", base, "--", WEB_PREFIX], { cwd });
+}

apps/web/scripts/lib/git-base.mjs

@@ -11,9 +11,12 @@ import path from "node:path";
 export const WEB_DIR = path.resolve(import.meta.dirname, "..", "..");
 export const REPO_ROOT = path.resolve(WEB_DIR, "..", "..");
 
-export function git(args, { quiet = false } = {}) {
+/** The branch every PR is judged against. */
+export const BASE_BRANCH = "origin/main";
+
+export function git(args, { quiet = false, cwd = REPO_ROOT } = {}) {
   return execFileSync("git", args, {
-    cwd: REPO_ROOT,
+    cwd,
     encoding: "utf8",
     maxBuffer: 64 * 1024 * 1024,
     stdio: ["ignore", "pipe", quiet ? "ignore" : "inherit"],
@@ -30,6 +33,60 @@ export function toPosixPath(value) {
   return value.split(path.sep).join("/");
 }
 
+function mergeBase(ref, cwd) {
+  return git(["merge-base", ref, "HEAD"], { quiet: true, cwd }).trim();
+}
+
+function isAncestor(maybeAncestor, descendant, cwd) {
+  try {
+    git(["merge-base", "--is-ancestor", maybeAncestor, descendant], { quiet: true, cwd });
+    return true;
+  } catch {
+    return false;
+  }
+}
+
+/**
+ * Raise a caller-supplied base to the point HEAD forked from the base branch.
+ *
+ * An explicit `--base` is routinely STALE. On a `pull_request` event CI checks
+ * out the MERGE REF (`refs/pull/N/merge`), whose first parent is the base branch
+ * tip at check time — but `github.event.pull_request.base.sha` is the base tip
+ * from when the PR event fired. The stale sha is therefore an *ancestor* of
+ * HEAD, so `merge-base` hands it straight back, and every commit the base branch
+ * landed in between reads as "new code in this PR". That fails the ratchet on
+ * files the change never touched, and it is not fixed by a three-dot diff:
+ * `base...HEAD` collapses to `base..HEAD` precisely because base is an ancestor.
+ * Rebasing a branch onto a newer base produces the same shape with no merge
+ * commit at all.
+ *
+ * The ratchet's contract is that it never asks you to migrate code you did not
+ * write, so the base must never sit older than the fork point.
+ */
+function forkPointFloor(base, cwd) {
+  // HEAD is contained in the base branch — a `push` build of that branch. There
+  // is no fork point (the merge base IS HEAD), and advancing to it would compare
+  // HEAD against itself and wave every commit through. Trust the caller's base.
+  if (isAncestor("HEAD", BASE_BRANCH, cwd)) return base;
+
+  let forkPoint;
+  try {
+    forkPoint = mergeBase(BASE_BRANCH, cwd);
+  } catch {
+    // No base branch in this checkout, so there is nothing better to offer.
+    return base;
+  }
+
+  if (forkPoint === base || !isAncestor(base, forkPoint, cwd)) return base;
+
+  console.log(
+    `ℹ base advanced to the ${BASE_BRANCH} fork point ${forkPoint.slice(0, 9)} — ` +
+      `the --base given was older, and everything ${BASE_BRANCH} landed in between\n` +
+      `  is not this change's code.`,
+  );
+  return forkPoint;
+}
+
 /**
  * The fork point to judge against, or `null` when the caller explicitly allows
  * running without one.
@@ -43,13 +100,14 @@ export function toPosixPath(value) {
  *
  * @returns {{ base: string } | { skip: string }}
  */
-export function resolveBase(argv = process.argv) {
+export function resolveBase(argv = process.argv, cwd = REPO_ROOT) {
   const flag = argv.indexOf("--base");
   const explicit = flag !== -1 && argv[flag + 1] ? argv[flag + 1] : null;
-  const requested = explicit ?? "origin/main";
+  const requested = explicit ?? BASE_BRANCH;
 
+  let base;
   try {
-    return { base: git(["merge-base", requested, "HEAD"], { quiet: true }).trim() };
+    base = mergeBase(requested, cwd);
   } catch {
     if (explicit || process.env.CI) {
       console.error(
@@ -62,4 +120,7 @@ export function resolveBase(argv = process.argv) {
     }
     return { skip: `no base ref (tried "${requested}"); pass --base <sha> to check explicitly` };
   }
+
+  // Only an explicit base can be stale; the default IS the fork point already.
+  return { base: explicit ? forkPointFloor(base, cwd) : base };
 }

apps/web/scripts/lib/git-base.test.ts

@@ -0,0 +1,238 @@
+/**
+ * Base resolution against real git topologies.
+ *
+ * The bug these pin down (#2160): CI passed `--base
+ * ${{ github.event.pull_request.base.sha }}` while checking out the MERGE REF.
+ * That sha is an ancestor of the merge ref, so `merge-base` returned it verbatim
+ * and every commit main landed since the PR opened was judged as the PR's own
+ * new code — failing PRs on files not in their diff. A three-dot diff does not
+ * help: `base...HEAD` collapses to `base..HEAD` exactly when base is an ancestor.
+ *
+ * Each scenario is built as a real repository, and every "fixed" assertion is
+ * paired with one showing the raw stale base still reproduces the failure — a
+ * test that only exercised the happy path would pass against the broken code.
+ */
+import { execFileSync } from "node:child_process";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import { afterAll, describe, expect, it } from "vitest";
+
+import { changedFiles } from "./changed-files.mjs";
+import { resolveBase } from "./git-base.mjs";
+
+/** A file `main` lands while the PR is open. Not in the PR's diff. */
+const FOREIGN = "apps/web/components/settings/agent-generated-task-title-settings.tsx";
+/** The PR's own new file. */
+const OWNED = "apps/web/components/gitlab/watch-table.tsx";
+/** Clean contents for OWNED: no user-facing literal for the ratchet to find. */
+const CLEAN_SOURCE = "export const Watch = () => null;\n";
+
+const tmpRoots: string[] = [];
+
+afterAll(() => {
+  for (const dir of tmpRoots) fs.rmSync(dir, { recursive: true, force: true });
+});
+
+function git(cwd: string, args: string[], { quiet = false }: { quiet?: boolean } = {}) {
+  return execFileSync(
+    "git",
+    [
+      "-c",
+      "user.email=ratchet@example.test",
+      "-c",
+      "user.name=Ratchet Test",
+      "-c",
+      "commit.gpgsign=false",
+      // A developer machine may set core.hooksPath globally; an inherited
+      // pre-commit hook would fail these fixture commits for unrelated reasons.
+      "-c",
+      "core.hooksPath=/nonexistent",
+      ...args,
+    ],
+    { cwd, encoding: "utf8", stdio: ["ignore", "pipe", quiet ? "ignore" : "inherit"] },
+  );
+}
+
+function write(dir: string, rel: string, body: string) {
+  const file = path.join(dir, rel);
+  fs.mkdirSync(path.dirname(file), { recursive: true });
+  fs.writeFileSync(file, body);
+}
+
+function commit(dir: string, message: string) {
+  git(dir, ["add", "-A"]);
+  git(dir, ["commit", "-q", "-m", message]);
+  return git(dir, ["rev-parse", "HEAD"]).trim();
+}
+
+function initRepo() {
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-ratchet-"));
+  tmpRoots.push(dir);
+  git(dir, ["init", "-q", "-b", "main"]);
+  write(dir, "apps/web/components/base/base.tsx", "export const Base = () => null;\n");
+  return { dir, root: commit(dir, "base") };
+}
+
+/** Stands in for `origin/main` without needing an actual remote. */
+function setOriginMain(dir: string, sha: string) {
+  git(dir, ["update-ref", "refs/remotes/origin/main", sha]);
+}
+
+function landOnMain(dir: string) {
+  write(
+    dir,
+    FOREIGN,
+    [
+      'export const Card = () => <CardTitle className="text-base">Agent-generated',
+      "titles</CardTitle>;\n",
+    ].join(" "),
+  );
+  return commit(dir, "feat: agent-generated task titles");
+}
+
+/**
+ * CI's `pull_request` checkout shape:
+ *
+ *   root ── main ─────────────── mainTip ──┐
+ *      └── pr (clean file) ── prHead ──────┴── mergeRef   (first parent = mainTip)
+ */
+function prMergeRef() {
+  const { dir, root } = initRepo();
+
+  git(dir, ["checkout", "-q", "-b", "pr"]);
+  write(dir, OWNED, CLEAN_SOURCE);
+  const prHead = commit(dir, "feat(i18n): localize GitLab");
+
+  git(dir, ["checkout", "-q", "main"]);
+  const mainTip = landOnMain(dir);
+  setOriginMain(dir, mainTip);
+
+  // Detach at the base tip and merge the PR in, so the first parent is the base
+  // side — exactly how GitHub builds refs/pull/N/merge.
+  git(dir, ["checkout", "-q", mainTip]);
+  git(dir, ["merge", "-q", "--no-ff", "-m", `Merge ${prHead} into ${mainTip}`, prHead]);
+
+  return { dir, root, prHead, mainTip, mergeRef: git(dir, ["rev-parse", "HEAD"]).trim() };
+}
+
+function baseOf(dir: string, argv: string[]) {
+  const resolved = resolveBase(argv, dir);
+  if ("skip" in resolved) throw new Error(`unexpected skip: ${resolved.skip}`);
+  return resolved.base;
+}
+
+describe("resolveBase on a pull_request merge ref", () => {
+  it("reproduces the failure when the stale base is used verbatim", () => {
+    const { dir, root } = prMergeRef();
+
+    // Guards the scenario itself: without the floor, main's file reads as added.
+    expect(changedFiles(root, "A", dir)).toContain(FOREIGN);
+  });
+
+  it("floors a stale --base at the fork point, so main's file is not reported", () => {
+    const { dir, root, mainTip } = prMergeRef();
+
+    expect(baseOf(dir, ["--base", root])).toBe(mainTip);
+
+    const added = changedFiles(baseOf(dir, ["--base", root]), "A", dir);
+    expect(added).not.toContain(FOREIGN);
+    expect(added).toEqual([OWNED]);
+  });
+
+  it("resolves the same base with no --base at all", () => {
+    const { dir, mainTip } = prMergeRef();
+
+    expect(baseOf(dir, [])).toBe(mainTip);
+    expect(changedFiles(baseOf(dir, []), "A", dir)).toEqual([OWNED]);
+  });
+
+  it("leaves an already-current --base alone", () => {
+    const { dir, mainTip } = prMergeRef();
+
+    expect(baseOf(dir, ["--base", mainTip])).toBe(mainTip);
+  });
+
+  // Selection only — whether the literal inside is then flagged is ESLint's job,
+  // and check-new-code-i18n.mjs resolves its config from this repo's own root, so
+  // it cannot be pointed at a fixture. What matters here is that raising the base
+  // does not narrow the file set past the PR's own work: a file the change added
+  // must still reach the linter.
+  it("still selects the PR's own added file, literal and all", () => {
+    const { dir, root, mainTip } = prMergeRef();
+    git(dir, ["checkout", "-q", "-B", "pr2", mainTip]);
+    write(dir, OWNED, "export const W = () => <p>Hardcoded</p>;\n");
+    commit(dir, "feat: add a literal");
+    setOriginMain(dir, mainTip);
+
+    expect(changedFiles(baseOf(dir, ["--base", root]), "A", dir)).toEqual([OWNED]);
+  });
+});
+
+describe("resolveBase on other checkout shapes", () => {
+  it("floors a rebased branch, which has no merge commit but the same defect", () => {
+    const { dir, root } = initRepo();
+    const mainTip = landOnMain(dir);
+    setOriginMain(dir, mainTip);
+
+    // Rebased onto the new tip: linear history that still CONTAINS main's commit.
+    git(dir, ["checkout", "-q", "-b", "pr"]);
+    write(dir, OWNED, CLEAN_SOURCE);
+    commit(dir, "feat(i18n): localize GitLab");
+
+    expect(changedFiles(root, "A", dir)).toContain(FOREIGN);
+    expect(baseOf(dir, ["--base", root])).toBe(mainTip);
+    expect(changedFiles(baseOf(dir, ["--base", root]), "A", dir)).toEqual([OWNED]);
+  });
+
+  it("does not advance a push build, where HEAD is the base branch tip", () => {
+    const { dir, root } = initRepo();
+    const mainTip = landOnMain(dir);
+    setOriginMain(dir, mainTip);
+
+    // `push` to main passes github.event.before. Advancing to the fork point
+    // would compare HEAD with itself and wave the pushed commit through.
+    expect(baseOf(dir, ["--base", root])).toBe(root);
+    expect(changedFiles(baseOf(dir, ["--base", root]), "A", dir)).toContain(FOREIGN);
+  });
+
+  it("keeps an explicit base when there is no origin/main to floor against", () => {
+    const { dir, root } = initRepo();
+    git(dir, ["checkout", "-q", "-b", "pr"]);
+    write(dir, OWNED, CLEAN_SOURCE);
+    commit(dir, "feat: add");
+
+    expect(baseOf(dir, ["--base", root])).toBe(root);
+  });
+});
+
+describe("the guard-allowlist check inherits the same base", () => {
+  /**
+   * check-guard-allowlist.mjs reads the baseline with `git show <base>:<path>`.
+   * With a stale base that baseline predates main's entries, so a PR deleting one
+   * of them is not flagged — the removal-guard's whole purpose, silently lost.
+   */
+  it("reads a baseline that includes what main landed", () => {
+    const { dir, root } = initRepo();
+    const allowlist = "apps/web/eslint.i18n.options.mjs";
+
+    git(dir, ["checkout", "-q", "-b", "pr"]);
+    write(dir, OWNED, CLEAN_SOURCE);
+    commit(dir, "feat: pr work");
+    const prHead = git(dir, ["rev-parse", "HEAD"]).trim();
+
+    git(dir, ["checkout", "-q", "main"]);
+    write(dir, allowlist, 'export const i18nGuardFiles = ["components/settings/**"];\n');
+    const mainTip = commit(dir, "feat: migrate settings");
+    setOriginMain(dir, mainTip);
+    git(dir, ["checkout", "-q", mainTip]);
+    git(dir, ["merge", "-q", "--no-ff", "-m", "merge", prHead]);
+
+    const stale = () => git(dir, ["show", `${root}:${allowlist}`], { quiet: true });
+    expect(stale).toThrow(); // the baseline does not exist at the stale base
+
+    expect(git(dir, ["show", `${baseOf(dir, ["--base", root])}:${allowlist}`])).toContain(
+      "components/settings/**",
+    );
+  });
+});