diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index f43f169a0f..02f5b2e554 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.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 diff --git a/apps/web/scripts/check-new-code-i18n.mjs b/apps/web/scripts/check-new-code-i18n.mjs index f9d66c4e85..983f258e41 100644 --- a/apps/web/scripts/check-new-code-i18n.mjs +++ b/apps/web/scripts/check-new-code-i18n.mjs @@ -26,37 +26,19 @@ * Usage: * node scripts/check-new-code-i18n.mjs [--base ] * - * 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, diff --git a/apps/web/scripts/lib/changed-files.mjs b/apps/web/scripts/lib/changed-files.mjs new file mode 100644 index 0000000000..d7dd024c38 --- /dev/null +++ b/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 }); +} diff --git a/apps/web/scripts/lib/git-base.mjs b/apps/web/scripts/lib/git-base.mjs index d059077f22..7731ee6e73 100644 --- a/apps/web/scripts/lib/git-base.mjs +++ b/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 to check explicitly` }; } + + // Only an explicit base can be stale; the default IS the fork point already. + return { base: explicit ? forkPointFloor(base, cwd) : base }; } diff --git a/apps/web/scripts/lib/git-base.test.ts b/apps/web/scripts/lib/git-base.test.ts new file mode 100644 index 0000000000..575ccb5422 --- /dev/null +++ b/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 = () => Agent-generated', + "titles;\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 = () =>

Hardcoded

;\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 :`. + * 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/**", + ); + }); +});