diff --git a/.github/workflows/param-guard.yml b/.github/workflows/param-guard.yml new file mode 100644 index 0000000..23871ad --- /dev/null +++ b/.github/workflows/param-guard.yml @@ -0,0 +1,47 @@ +name: Param guard + +# Blocks pull requests that remove a parameter from a model that still exists. +# Removing a parameter is breaking: every Manifest user who already configured it +# loses the ability to see or change that setting. +# +# Override: a maintainer applies the `allow-param-removal` label to the PR. The +# label change re-triggers this workflow (see `types` below) and the guard step +# is skipped, leaving an auditable warning in the checks output. + +on: + pull_request: + branches: [main] + types: [opened, synchronize, reopened, labeled, unlabeled] + +jobs: + no-param-removals: + name: No parameter removals + runs-on: ubuntu-latest + steps: + - name: Check out PR + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Fetch base branch + run: git fetch --no-tags origin "${{ github.base_ref }}" + + - name: Set up Node + uses: actions/setup-node@v4 + with: + node-version: "20" + cache: "npm" + + - name: Install dependencies + run: npm ci + + - name: Guard against parameter removals + if: ${{ !contains(github.event.pull_request.labels.*.name, 'allow-param-removal') }} + run: npm run guard:params + env: + BASE_REF: "origin/${{ github.base_ref }}" + + - name: Removal override acknowledged + if: ${{ contains(github.event.pull_request.labels.*.name, 'allow-param-removal') }} + run: | + echo "::warning::'allow-param-removal' label present — parameter-removal guard intentionally skipped for this PR." diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b7ecfd6..4eacf46 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -103,6 +103,32 @@ params: thinking.type: enabled ``` +## Removing parameters is blocked + +Once a parameter is published for a model, **it cannot be removed**. People using +that model in [Manifest](https://manifest.build/) may already have the parameter +configured; dropping it from the catalog takes away their ability to see or change +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: + +```bash +npm run guard:params # compares against origin/main +npm run guard:params -- --base # compare against a specific ref +``` + +What is _not_ blocked: adding new parameters, editing a parameter's metadata +(label, description, default, range, values, applicability), and removing a whole +model file. Only the disappearance of a `path` from a model that still exists is +treated as a breaking removal. + +If a removal is genuinely necessary (e.g. a parameter was added by mistake), a +maintainer must add the **`allow-param-removal`** label to the PR. The label +re-runs the check and skips the guard, leaving a visible warning on the run. + ## Site changes The website code lives under `src/`: diff --git a/package.json b/package.json index af08806..cc6c5a5 100644 --- a/package.json +++ b/package.json @@ -20,6 +20,7 @@ "build": "tsx src/build/build.ts", "dev": "tsx watch src/server/dev.ts", "validate": "tsx src/data/validate.ts", + "guard:params": "tsx src/data/check-removals.ts", "typecheck": "tsc --noEmit", "lint": "eslint . --ext .ts,.tsx", "format": "prettier --write .", diff --git a/src/data/check-removals.ts b/src/data/check-removals.ts new file mode 100644 index 0000000..3cbbeb1 --- /dev/null +++ b/src/data/check-removals.ts @@ -0,0 +1,116 @@ +import { execFileSync } from "node:child_process"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import type { Model } from "../schema/model.js"; +import { loadAllModels } from "./load.js"; +import { REPO_ROOT } from "./paths.js"; +import { findRemovedParams, type ParamRemoval } from "./removals.js"; + +const OVERRIDE_LABEL = "allow-param-removal"; + +function git(args: string[]): string { + return execFileSync("git", args, { + cwd: REPO_ROOT, + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + }); +} + +function refExists(ref: string): boolean { + try { + git(["rev-parse", "--verify", "--quiet", `${ref}^{commit}`]); + return true; + } catch { + return false; + } +} + +function argBase(): string | undefined { + const eq = process.argv.find((a) => a.startsWith("--base=")); + if (eq) return eq.slice("--base=".length); + const i = process.argv.indexOf("--base"); + if (i >= 0 && process.argv[i + 1]) return process.argv[i + 1]; + return undefined; +} + +/** Resolve the base ref to diff against, trying the most specific source first. */ +function resolveBaseRef(): string | 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; + } + return null; +} + +/** Materialize the `models/` tree at `ref` into a temp dir and load it. */ +async function loadModelsAtRef(ref: string): Promise { + const listing = git(["ls-tree", "-r", "--name-only", ref, "--", "models"]).trim(); + const files = listing ? listing.split("\n").filter((f) => /\.ya?ml$/i.test(f)) : []; + if (files.length === 0) return []; + + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "mp-baseline-")); + try { + for (const file of files) { + const content = git(["show", `${ref}:${file}`]); + const dest = path.join(tmp, ...file.split("/")); + await fs.mkdir(path.dirname(dest), { recursive: true }); + await fs.writeFile(dest, content, "utf8"); + } + const { models } = await loadAllModels(path.join(tmp, "models")); + return models; + } finally { + await fs.rm(tmp, { recursive: true, force: true }); + } +} + +function reportRemovals(removals: ParamRemoval[], baseRef: string): void { + console.error(`\n✖ Parameter removal detected vs ${baseRef} — blocked.\n`); + console.error( + "Removing a parameter breaks every Manifest user who already has it\n" + + "configured: they lose the ability to see or change that setting.\n", + ); + console.error("Parameters present on the base branch but missing in this change:\n"); + let lastModel = ""; + for (const removal of removals) { + if (removal.modelId !== lastModel) { + console.error(` ${removal.modelId}`); + lastModel = removal.modelId; + } + console.error(` - ${removal.path}`); + } + console.error( + `\nIf this removal is genuinely intended, a maintainer must add the` + + `\n\`${OVERRIDE_LABEL}\` label to the PR, then re-run this check.\n`, + ); +} + +async function main(): Promise { + const baseRef = resolveBaseRef(); + if (!baseRef) { + console.log("No base ref available to compare against — skipping removal guard."); + return; + } + + const [{ models: current }, base] = await Promise.all([ + loadAllModels(), + loadModelsAtRef(baseRef), + ]); + + const removals = findRemovedParams(base, current); + if (removals.length === 0) { + console.log(`OK — no parameters removed vs ${baseRef}.`); + return; + } + + reportRemovals(removals, baseRef); + process.exit(1); +} + +main().catch((err) => { + console.error("Removal guard crashed:", err); + process.exit(2); +}); diff --git a/src/data/removals.ts b/src/data/removals.ts new file mode 100644 index 0000000..b7451f4 --- /dev/null +++ b/src/data/removals.ts @@ -0,0 +1,51 @@ +import { modelId, type Model } from "../schema/model.js"; + +export interface ParamRemoval { + modelId: string; + path: string; +} + +/** + * Build an index of `modelId -> set of parameter paths` declared for that model. + * Paths from duplicate model ids (should not happen in a valid catalog) are + * unioned so we never under-count what a model exposes. + */ +export function buildParamIndex(models: Model[]): Map> { + const index = new Map>(); + for (const model of models) { + const id = modelId(model); + const paths = index.get(id) ?? new Set(); + for (const param of model.params) { + paths.add(param.path); + } + index.set(id, paths); + } + return index; +} + +/** + * Find parameters that exist on `base` but are gone on `current`. + * + * Scope (intentional): only models that STILL EXIST in `current` are checked. + * Dropping a whole model (its id no longer appears) is not reported here — that + * is governed by a separate policy decision. The harm we guard against is a live + * model silently losing a knob that consumers already have configured. + */ +export function findRemovedParams(base: Model[], current: Model[]): ParamRemoval[] { + const baseIndex = buildParamIndex(base); + const currentIndex = buildParamIndex(current); + const removals: ParamRemoval[] = []; + + for (const [id, basePaths] of baseIndex) { + const currentPaths = currentIndex.get(id); + if (!currentPaths) continue; // whole model gone — out of scope by policy + for (const path of basePaths) { + if (!currentPaths.has(path)) { + removals.push({ modelId: id, path }); + } + } + } + + removals.sort((a, b) => a.modelId.localeCompare(b.modelId) || a.path.localeCompare(b.path)); + return removals; +} diff --git a/tests/removals.test.ts b/tests/removals.test.ts new file mode 100644 index 0000000..df99b04 --- /dev/null +++ b/tests/removals.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from "vitest"; +import { buildParamIndex, findRemovedParams } from "../src/data/removals.js"; +import type { AuthType, Model, Parameter } from "../src/schema/model.js"; + +function param(path: string): Parameter { + return { path, type: "number", label: "L", description: "d", group: "sampling" }; +} + +function model(provider: string, authType: AuthType, name: string, paths: string[]): Model { + return { provider, authType, model: name, params: paths.map(param) }; +} + +describe("findRemovedParams", () => { + it("flags a parameter removed from a model that still exists", () => { + const base = [model("anthropic", "api_key", "opus", ["max_tokens", "temperature"])]; + const current = [model("anthropic", "api_key", "opus", ["max_tokens"])]; + + expect(findRemovedParams(base, current)).toEqual([ + { modelId: "anthropic/opus", path: "temperature" }, + ]); + }); + + it("treats a renamed parameter path as a removal", () => { + const base = [model("openai", "api_key", "gpt", ["max_tokens"])]; + const current = [model("openai", "api_key", "gpt", ["maxTokens"])]; + + expect(findRemovedParams(base, current)).toEqual([ + { modelId: "openai/gpt", path: "max_tokens" }, + ]); + }); + + it("does not flag newly added parameters", () => { + const base = [model("openai", "api_key", "gpt", ["max_tokens"])]; + const current = [model("openai", "api_key", "gpt", ["max_tokens", "temperature"])]; + + expect(findRemovedParams(base, current)).toEqual([]); + }); + + it("does not flag a whole model that was removed (out of scope by policy)", () => { + const base = [ + model("anthropic", "api_key", "opus", ["max_tokens"]), + model("anthropic", "api_key", "haiku", ["temperature"]), + ]; + const current = [model("anthropic", "api_key", "opus", ["max_tokens"])]; + + expect(findRemovedParams(base, current)).toEqual([]); + }); + + it("does not flag a brand-new model", () => { + const base = [model("openai", "api_key", "gpt", ["max_tokens"])]; + const current = [ + model("openai", "api_key", "gpt", ["max_tokens"]), + model("openai", "api_key", "gpt-mini", ["temperature"]), + ]; + + expect(findRemovedParams(base, current)).toEqual([]); + }); + + it("does not care about parameter ordering", () => { + const base = [model("openai", "api_key", "gpt", ["a", "b", "c"])]; + const current = [model("openai", "api_key", "gpt", ["c", "a", "b"])]; + + expect(findRemovedParams(base, current)).toEqual([]); + }); + + it("treats api_key and subscription as separate models", () => { + const base = [ + model("anthropic", "api_key", "opus", ["temperature"]), + model("anthropic", "subscription", "opus", ["temperature"]), + ]; + const current = [ + model("anthropic", "api_key", "opus", ["temperature"]), + model("anthropic", "subscription", "opus", []), + ]; + + expect(findRemovedParams(base, current)).toEqual([ + { modelId: "anthropic/opus-subscription", path: "temperature" }, + ]); + }); + + it("reports multiple removals sorted by model id then path", () => { + const base = [ + model("openai", "api_key", "gpt", ["a", "z"]), + model("anthropic", "api_key", "opus", ["m", "n"]), + ]; + const current = [ + model("openai", "api_key", "gpt", ["a"]), + model("anthropic", "api_key", "opus", ["m"]), + ]; + + expect(findRemovedParams(base, current)).toEqual([ + { modelId: "anthropic/opus", path: "n" }, + { modelId: "openai/gpt", path: "z" }, + ]); + }); +}); + +describe("buildParamIndex", () => { + it("collects parameter paths per model id", () => { + const index = buildParamIndex([model("openai", "api_key", "gpt", ["a", "b"])]); + expect(index.get("openai/gpt")).toEqual(new Set(["a", "b"])); + }); +});