trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns12g rEventsFlows

The gate refuses an edit to a migration pin that is already on main, even when the pull's own earlier commit landed it #8366

Merged masoncrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

mason/forge:mason/8352-the-gate-refuses-an-edit-to-a-migration → main 1 commit fork

  • Merged. mason/8352-the-gate-refuses-an-edit-to-a-migration was merged into main as 0baf7ca.

Review

Sign in to review this change.
3 files changed +139 −18
scripts/gate.ts +35 −11
@@ -194,23 +194,37 @@ function git(args: string[], cwd: string = process.cwd()): { ok: boolean; out: s
194194 * Pins the pull edited, against its base: `$FORGE_CHECK_BASE`, else the
195195 * merge base with main. A pin whose migration is not on the base was never
196196 * history, so it is deleted and regenerated rather than left to a new migration.
197 * With no base to compare to it says so on stderr and names nothing.
197 * It also compares HEAD to main's tip (#8352): a pin there that the pull's
198 * own commits changed or deleted is refused, though a squash landing left
199 * the merge base before it. A tree with no git history (the forge's full
200 * check exports one) has nothing to diff and says so on stderr; a repository
201 * where no base resolves is refused, never passed unchecked.
198202 */
199203 // Imported on use: `treeGate` loads this file alone, outside the repository.
200204 export async function pinRefusal(cwd: string = process.cwd()): Promise<string[]> {
201 const { parseNameStatus, pinEdits, unpinnedAdds } = await import("../src/kernel/pin_edits.ts");
202 const env = process.env.FORGE_CHECK_BASE;
203 const base =
204 env || git(["merge-base", "origin/main", "HEAD"], cwd).out || git(["merge-base", "main", "HEAD"], cwd).out;
205 if (!base) {
205 const { parseNameStatus, pinEdits, tipEdits, unpinnedAdds } = await import("../src/kernel/pin_edits.ts");
206 if (!git(["rev-parse", "--git-dir"], cwd).ok) {
206207 console.error("gate: no base to compare to (set FORGE_CHECK_BASE): migration pins not checked");
207208 return [];
208209 }
210 const env = process.env.FORGE_CHECK_BASE;
211 const base =
212 env || git(["merge-base", "origin/main", "HEAD"], cwd).out || git(["merge-base", "main", "HEAD"], cwd).out;
213 const tip =
214 env ||
215 git(["rev-parse", "--verify", "--quiet", "origin/main^{commit}"], cwd).out ||
216 git(["rev-parse", "--verify", "--quiet", "main^{commit}"], cwd).out;
217 const unchecked = (why: string) => [
218 `${why}: a migration on main is history, so its pins must be checked; set FORGE_CHECK_BASE or fetch origin/main`,
219 ];
220 if (!base || !tip) return unchecked("no base to compare to");
209221 const diff = git(["diff", "--name-status", "-M", `${base}..HEAD`], cwd);
210 if (!diff.ok) {
211 console.error(`gate: git diff against ${base} failed: migration pins not checked`);
212 return [];
213 }
222 if (!diff.ok) return unchecked(`git diff against ${base} failed`);
223 const fromTip = git(
224 ["diff", "--name-status", "--no-renames", tip, "HEAD", "--", "src/core/migration-pins", "src/core/migration-pins.json"],
225 cwd,
226 );
227 if (!fromTip.ok) return unchecked(`git diff against ${tip} failed`);
214228 const changes = parseNameStatus(diff.out);
215229 const edits = pinEdits(changes).map((path) => {
216230 const name = path.split("/").pop()?.replace(/\.pin$/, "") ?? "";
@@ -222,13 +236,23 @@ export async function pinRefusal(cwd: string = process.cwd()): Promise<string[]>
222236 ? `${path} was deleted: a migration on main is history, so restore it`
223237 : `${path} was changed: a migration on main is history, so add a new migration instead`;
224238 });
239 // Pins on main's tip the pull's commits wrote: by content, so a rebase passes.
240 const named = new Set(pinEdits(changes));
241 const touched = (path: string) => git(["log", "-1", "--format=%H", `${base}..HEAD`, "--", path], cwd).out !== "";
242 const landed = tipEdits(parseNameStatus(fromTip.out), touched)
243 .filter(({ path }) => !named.has(path))
244 .map(({ status, path }) =>
245 status.startsWith("D")
246 ? `${path} was deleted: a migration on main is history, so restore it`
247 : `${path} was changed: a migration on main is history, so add a new migration instead`,
248 );
225249 // Last, so the check's summary — its last line — names the one-step fix.
226250 const pinned = (name: string) => existsSync(join(cwd, "src/core/migration-pins", `${name}.pin`));
227251 const missing = unpinnedAdds(changes, pinned).map(
228252 (name) =>
229253 `this pull adds migration ${name} without its pin: run bun scripts/pin-migrations.ts and commit src/core/migration-pins/${name}.pin`,
230254 );
231 return [...edits, ...missing];
255 return [...edits, ...landed, ...missing];
232256 }
233257
234258 // `bun scripts/gate.ts`: the audit alone, first in `forge.json`'s check, so
src/kernel/pin_edits.ts +20
@@ -59,3 +59,23 @@ export function unpinnedAdds(
5959 }
6060 return names;
6161 }
62
63 /**
64 * Pins already on main that the pull changed or deleted (#8352). `changes`
65 * is main's tip diffed to HEAD; `touched(path)` says the pull's own commits
66 * wrote that path. A squash-landed commit is not an ancestor of the pull,
67 * so its merge base predates the pin and the diff to it reads an edit as an
68 * add; the tip does not. A pin that only differs because the pull is
69 * behind main was never touched, and a rebased pull matches the tip.
70 */
71 export function tipEdits(
72 changes: readonly Change[],
73 touched: (path: string) => boolean,
74 ): Change[] {
75 return changes.filter(
76 ({ status, path }) =>
77 (path.startsWith(PINS) || path === FROZEN) &&
78 (status.startsWith("M") || status.startsWith("D") || status.startsWith("T")) &&
79 touched(path),
80 );
81 }
test/pin-refusal.test.ts +84 −7
@@ -32,9 +32,10 @@ function repo(name: string) {
3232 return { dir, base: run(dir, "rev-parse", "HEAD") };
3333 }
3434
35 const withBase = async (base: string, f: () => Promise<string[]>) => {
35 const withBase = async (base: string | undefined, f: () => Promise<string[]>) => {
3636 const old = process.env.FORGE_CHECK_BASE;
37 process.env.FORGE_CHECK_BASE = base;
37 if (base === undefined) delete process.env.FORGE_CHECK_BASE;
38 else process.env.FORGE_CHECK_BASE = base;
3839 try {
3940 return await f();
4041 } finally {
@@ -90,22 +91,98 @@ test("`bun scripts/gate.ts` exits 2 and names the edited pin", () => {
9091 expect(r.stderr.toString()).toContain("src/core/migration-pins/0001-a.pin");
9192 });
9293
93 test("with no base it warns on stderr instead of passing silently", async () => {
94 test("in a repository with no base it refuses instead of passing unchecked", async () => {
9495 const { dir } = repo("nobase");
96 const out = await withBase(undefined, () => pinRefusal(dir));
97 expect(out).toHaveLength(1);
98 expect(out[0]).toStartWith("no base to compare to: a migration on main is history");
99 });
100
101 test("a tree with no git history (the full check's export) says so on stderr and passes", async () => {
102 const dir = join(root, "export");
103 put(dir, "src/core/migration-pins/0001-a.pin", "x");
95104 const seen: string[] = [];
96105 const orig = console.error;
97106 console.error = (...a: unknown[]) => void seen.push(a.join(" "));
98 const old = process.env.FORGE_CHECK_BASE;
99 delete process.env.FORGE_CHECK_BASE;
100107 try {
101 expect(await pinRefusal(dir)).toEqual([]);
108 expect(await withBase(undefined, () => pinRefusal(dir))).toEqual([]);
102109 } finally {
103110 console.error = orig;
104 if (old !== undefined) process.env.FORGE_CHECK_BASE = old;
105111 }
106112 expect(seen.join("\n")).toContain("pins not checked");
107113 });
108114
115 /**
116 * #8103's history (#8352): the branch lands a migration and its pin by
117 * squash, then, still holding its pre-squash commit, edits both. The merge
118 * base predates the squash, so against it the pin reads as added.
119 */
120 function squashed(name: string) {
121 const { dir, base } = repo(name);
122 const migration = "src/core/migrations/0005-e.ts";
123 const pin = "src/core/migration-pins/0005-e.pin";
124 put(dir, migration, "create index e");
125 put(dir, pin, "v1");
126 run(dir, "add", "-A");
127 run(dir, "commit", "-qm", "add e");
128 run(dir, "checkout", "-q", "-b", "main", base);
129 put(dir, migration, "create index e");
130 put(dir, pin, "v1");
131 run(dir, "add", "-A");
132 run(dir, "commit", "-qm", "add e (#1) squashed");
133 run(dir, "update-ref", "refs/remotes/origin/main", "HEAD");
134 run(dir, "checkout", "-q", "work");
135 return { dir, migration, pin };
136 }
137
138 test("an edit to a pin a squash already landed on main is refused", async () => {
139 const { dir, migration, pin } = squashed("squash-edit");
140 put(dir, migration, "create index e2 where x");
141 put(dir, pin, "v2");
142 run(dir, "commit", "-qam", "edit e");
143 expect(await withBase(undefined, () => pinRefusal(dir))).toEqual([
144 `${pin} was changed: a migration on main is history, so add a new migration instead`,
145 ]);
146 const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => k !== "FORGE_CHECK_BASE"));
147 const r = Bun.spawnSync(["bun", gateScript], { cwd: dir, env });
148 expect(r.exitCode).toBe(2);
149 expect(r.stderr.toString()).toContain(`gate: ${pin} was changed: a migration on main is history`);
150 });
151
152 test("deleting a pin a squash already landed on main is refused", async () => {
153 const { dir, pin } = squashed("squash-delete");
154 run(dir, "rm", "-q", pin);
155 run(dir, "commit", "-qm", "rm e");
156 expect(await withBase(undefined, () => pinRefusal(dir))).toContain(
157 `${pin} was deleted: a migration on main is history, so restore it`,
158 );
159 });
160
161 test("a pull that keeps the squashed pin as landed passes", async () => {
162 const { dir } = squashed("squash-kept");
163 put(dir, "src/other.ts", "more work");
164 run(dir, "add", "-A");
165 run(dir, "commit", "-qm", "more");
166 expect(await withBase(undefined, () => pinRefusal(dir))).toEqual([]);
167 });
168
169 test("a pull rebased onto main passes, and so does one behind main's newer pins", async () => {
170 const { dir, base } = repo("rebased");
171 run(dir, "checkout", "-q", "-b", "main");
172 put(dir, "src/core/migrations/0006-f.ts", "f");
173 put(dir, "src/core/migration-pins/0006-f.pin", "x");
174 run(dir, "add", "-A");
175 run(dir, "commit", "-qm", "f on main");
176 run(dir, "update-ref", "refs/remotes/origin/main", "HEAD");
177 run(dir, "checkout", "-q", "-b", "behind", base);
178 put(dir, "src/other.ts", "work");
179 run(dir, "add", "-A");
180 run(dir, "commit", "-qm", "work");
181 expect(await withBase(undefined, () => pinRefusal(dir))).toEqual([]);
182 run(dir, "rebase", "-q", "main");
183 expect(await withBase(undefined, () => pinRefusal(dir))).toEqual([]);
184 });
185
109186 test("a migration added without its pin is named with the one-step fix, last, as the check's summary", async () => {
110187 const { dir, base } = repo("unpinned");
111188 put(dir, "src/core/migrations/0003-c.ts", "c");

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs