From 169eecf5f53f5ef0c1af28132e49541df412563d Mon Sep 17 00:00:00 2001 From: Bruno Perez Date: Mon, 3 Aug 2026 11:47:22 +0200 Subject: [PATCH 1/2] fix: compare the param guard against the merge base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard diffed the checked-out tree against the tip of the base branch. On a pull request that tree is GitHub's refs/pull/N/merge commit, which is only recomputed when the PR or its base is pushed to — so a re-run, or a `labeled` event, compares a merge built on an older base against a base tip that has moved on. Every parameter the base gained in between reads as a removal. That is how run 26753498917 blocked PR #37: re-run four days after it was queued, it reported mistral/magistral-* losing `prompt_mode`, a file the PR never touched. Diffing against the merge base puts both sides of the comparison on the same snapshot. Removals the PR really does make are still caught: PR #132 still reports dropping `reasoning_effort` from z-ai/glm-5.2. --- CONTRIBUTING.md | 5 +++-- src/data/check-removals.ts | 34 +++++++++++++++++++++++++--------- src/data/git-baseline.ts | 19 +++++++++++++++++++ tests/git-baseline.test.ts | 25 +++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 tests/git-baseline.test.ts 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..9377304 --- /dev/null +++ b/tests/git-baseline.test.ts @@ -0,0 +1,25 @@ +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); + }); + + it("returns a commit both sides share", () => { + const parent = git(["rev-parse", "HEAD^"]).trim(); + expect(mergeBase(parent)).toBe(parent); + }); + + 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); + }); +}); From bd7b94dcd9bf84cd85620d2620bca9c6c55222fa Mon Sep 17 00:00:00 2001 From: Bruno Perez Date: Mon, 3 Aug 2026 11:49:04 +0200 Subject: [PATCH 2/2] test: stop assuming HEAD has a parent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI clones shallow, so `git rev-parse HEAD^` fails there. The remaining cases cover the contract that matters — a resolvable ref narrows to a shared commit, an unresolvable one yields null and the guard falls back. --- tests/git-baseline.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/git-baseline.test.ts b/tests/git-baseline.test.ts index 9377304..68eb59e 100644 --- a/tests/git-baseline.test.ts +++ b/tests/git-baseline.test.ts @@ -7,11 +7,8 @@ describe("mergeBase", () => { expect(mergeBase("HEAD")).toBe(head); }); - it("returns a commit both sides share", () => { - const parent = git(["rev-parse", "HEAD^"]).trim(); - expect(mergeBase(parent)).toBe(parent); - }); - + // 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(); });