diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 32bec9d..1eb248a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -145,8 +145,9 @@ that setting and breaks their setup. CI enforces this. The `Param guard` workflow (`npm run guard:params`) compares your PR against `main` and **fails if any parameter `path` that exists on a model is gone** -— this includes renaming a `path` (the old name counts as removed). You can run the -same check locally before opening a PR: +— this includes renaming a `path` (the old name counts as removed). The comparison +runs against the merge base, so parameters `main` gained after you branched are not +counted against you. You can run the same check locally before opening a PR: ```bash npm run guard:params # compares against origin/main diff --git a/src/data/check-removals.ts b/src/data/check-removals.ts index 801c8a8..707b009 100644 --- a/src/data/check-removals.ts +++ b/src/data/check-removals.ts @@ -1,5 +1,5 @@ import { loadAllModels } from "./load.js"; -import { loadModelsAtRef, refExists } from "./git-baseline.js"; +import { loadModelsAtRef, mergeBase, refExists } from "./git-baseline.js"; import { findRemovedParams, type ParamRemoval } from "./removals.js"; const OVERRIDE_LABEL = "allow-param-removal"; @@ -12,14 +12,30 @@ function argBase(): string | undefined { return undefined; } -/** Resolve the base ref to diff against, trying the most specific source first. */ -function resolveBaseRef(): string | null { +interface Baseline { + /** Commit the current catalog is compared against. */ + commit: string; + /** How to name that commit in output. */ + label: string; +} + +/** + * Resolve the baseline to diff against, trying the most specific source first. + * + * The catalog is compared against the merge base rather than the tip of the + * base branch: the tip may have moved on since this checkout was built, and + * anything it gained in the meantime is not something this change removed. + */ +function resolveBaseline(): Baseline | null { const githubBase = process.env.GITHUB_BASE_REF ? `origin/${process.env.GITHUB_BASE_REF}` : undefined; const candidates = [argBase(), process.env.BASE_REF, githubBase, "origin/main", "main"]; for (const ref of candidates) { - if (ref && refExists(ref)) return ref; + if (!ref || !refExists(ref)) continue; + const base = mergeBase(ref); + if (!base) return { commit: ref, label: ref }; + return { commit: base, label: `${ref} (merge base ${base.slice(0, 7)})` }; } return null; } @@ -46,24 +62,24 @@ function reportRemovals(removals: ParamRemoval[], baseRef: string): void { } async function main(): Promise { - const baseRef = resolveBaseRef(); - if (!baseRef) { + const baseline = resolveBaseline(); + if (!baseline) { console.log("No base ref available to compare against — skipping removal guard."); return; } const [{ models: current }, base] = await Promise.all([ loadAllModels(), - loadModelsAtRef(baseRef), + loadModelsAtRef(baseline.commit), ]); const removals = findRemovedParams(base, current); if (removals.length === 0) { - console.log(`OK — no parameters removed vs ${baseRef}.`); + console.log(`OK — no parameters removed vs ${baseline.label}.`); return; } - reportRemovals(removals, baseRef); + reportRemovals(removals, baseline.label); process.exit(1); } diff --git a/src/data/git-baseline.ts b/src/data/git-baseline.ts index bbd5a8e..6590df7 100644 --- a/src/data/git-baseline.ts +++ b/src/data/git-baseline.ts @@ -25,6 +25,25 @@ export function refExists(ref: string): boolean { } } +/** + * Return the commit `ref` and `HEAD` share, or null when git cannot find one + * (unrelated histories, or a clone shallow enough to hide it). + * + * On a pull request the checkout is GitHub's `refs/pull/N/merge` commit, which + * is only recomputed when the PR or its base is pushed to. A re-run — or a + * `labeled` event — therefore compares a merge built against an older base with + * a freshly fetched base tip, and every parameter the base gained in between + * reads as a removal. The merge base is the commit the tree in hand was + * actually built on, so both sides of the diff come from the same snapshot. + */ +export function mergeBase(ref: string): string | null { + try { + return git(["merge-base", ref, "HEAD"]).trim() || null; + } catch { + return null; + } +} + /** * Materialize the `models/` tree at `ref` into a temp dir and load it. Returns * an empty array when `ref` has no catalog (e.g. before the catalog existed). diff --git a/tests/git-baseline.test.ts b/tests/git-baseline.test.ts new file mode 100644 index 0000000..68eb59e --- /dev/null +++ b/tests/git-baseline.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from "vitest"; +import { git, mergeBase, refExists } from "../src/data/git-baseline.js"; + +describe("mergeBase", () => { + it("resolves a ref that is already an ancestor of HEAD to that ref", () => { + const head = git(["rev-parse", "HEAD"]).trim(); + expect(mergeBase("HEAD")).toBe(head); + }); + + // Nothing here may assume HEAD has a parent: CI clones shallow, and the + // guard falls back to the ref itself when git cannot find a merge base. + it("returns null for a ref git cannot resolve", () => { + expect(mergeBase("no-such-ref-for-tests")).toBeNull(); + }); +}); + +describe("refExists", () => { + it("is true for HEAD and false for a made-up ref", () => { + expect(refExists("HEAD")).toBe(true); + expect(refExists("no-such-ref-for-tests")).toBe(false); + }); +});