trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlows

Builders push migrations without their pin and sink merge batches: the crew's pre-push runs pin-migrations automatically when a migration changes #6939

Merged masoncrew opened this in answer to trevatoowner

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

ConversationFiles
Merged

mason/forge:mason/6938-builders-push-migrations-without-their-p → main 1 commit fork

  • Merged. mason/6938-builders-push-migrations-without-their-p was merged into main as e522d43.

Review

Sign in to review this change.
7 files changed +212 −4
CLAUDE.md +3 −1
@@ -286,7 +286,9 @@ flag exists only in the stdio dev runner and is never reachable over SSH.
286286 pinned the schema test fails on an edit: the pull that adds a migration
287287 runs `bun scripts/pin-migrations.ts` and commits the one file it writes,
288288 `core/migration-pins/<name>.pin` (`core/migration-pins.json` is the frozen
289 older pins); the release never touches pins. A migration must only add
289 older pins); the release never touches pins. A crew clone's `pre-push`
290 hook (`crew/pin_hook.ts`) runs it and commits the pin when a push touches
291 a migration, and the gate names a migration added without its pin. A migration must only add
290292 (`test/schema-additive.test.ts`). A release is an annotated tag whose
291293 message is its record (`bun scripts/cut-release.ts <version> --tag`,
292294 read by `release/shipped.ts`); no pull and no release touches
scripts/gate.ts +10 −3
@@ -20,7 +20,7 @@
2020 * runs `outsideFiles` nightly and files a `Red:` issue for each that fails (#812).
2121 */
2222
23 import { readdirSync } from "node:fs";
23 import { existsSync, readdirSync } from "node:fs";
2424 import { join } from "node:path";
2525
2626 export type Exclusion = {
@@ -193,7 +193,7 @@ function git(args: string[], cwd: string = process.cwd()): { ok: boolean; out: s
193193 */
194194 // Imported on use: `treeGate` loads this file alone, outside the repository.
195195 export async function pinRefusal(cwd: string = process.cwd()): Promise<string[]> {
196 const { parseNameStatus, pinEdits } = await import("../src/kernel/pin_edits.ts");
196 const { parseNameStatus, pinEdits, unpinnedAdds } = await import("../src/kernel/pin_edits.ts");
197197 const env = process.env.FORGE_CHECK_BASE;
198198 const base =
199199 env || git(["merge-base", "origin/main", "HEAD"], cwd).out || git(["merge-base", "main", "HEAD"], cwd).out;
@@ -207,7 +207,7 @@ export async function pinRefusal(cwd: string = process.cwd()): Promise<string[]>
207207 return [];
208208 }
209209 const changes = parseNameStatus(diff.out);
210 return pinEdits(changes).map((path) => {
210 const edits = pinEdits(changes).map((path) => {
211211 const name = path.split("/").pop()?.replace(/\.pin$/, "") ?? "";
212212 const deleted = changes.some((c) => c.path === path && c.status.startsWith("D"));
213213 const onBase = git(["cat-file", "-e", `${base}:src/core/migrations/${name}.ts`], cwd).ok;
@@ -217,6 +217,13 @@ export async function pinRefusal(cwd: string = process.cwd()): Promise<string[]>
217217 ? `${path} was deleted: a migration on main is history, so restore it`
218218 : `${path} was changed: a migration on main is history, so add a new migration instead`;
219219 });
220 // Last, so the check's summary — its last line — names the one-step fix.
221 const pinned = (name: string) => existsSync(join(cwd, "src/core/migration-pins", `${name}.pin`));
222 const missing = unpinnedAdds(changes, pinned).map(
223 (name) =>
224 `this pull adds migration ${name} without its pin: run bun scripts/pin-migrations.ts and commit src/core/migration-pins/${name}.pin`,
225 );
226 return [...edits, ...missing];
220227 }
221228
222229 // `bun scripts/gate.ts`: the audit alone, first in `forge.json`'s check, so
src/crew/pin_hook.ts added +53
@@ -0,0 +1,53 @@
1 /**
2 * The crew clone's `pre-push` hook (#6938): a push whose commits add or
3 * change a file under `src/core/migrations/` runs
4 * `bun scripts/pin-migrations.ts` first and commits the pin it writes, so a
5 * builder cannot push a migration without its pin by forgetting a step.
6 *
7 * A hook cannot add a commit to the push already in flight, so when it
8 * commits a pin it refuses this push and says to push again; the forge's
9 * own push at the end of a run carries the pin commit if the agent stops
10 * there. The hook runs inside the agent's sandbox, where `.git/hooks` is
11 * read-only — the forge's own pushes skip it (`--no-verify`), so the
12 * repository's script never runs outside the sandbox.
13 *
14 * A repository without `scripts/pin-migrations.ts` is not touched; a pin
15 * script that fails (a pinned migration was edited) lets the push through,
16 * and the check's gate names the fix.
17 */
18
19 import { chmodSync, mkdirSync, writeFileSync } from "node:fs";
20 import { join } from "node:path";
21
22 export const PIN_HOOK = `#!/bin/sh
23 # Written by the forge for every crew clone (src/crew/pin_hook.ts, #6938).
24 [ -f scripts/pin-migrations.ts ] || exit 0
25 changed=
26 while read -r local_ref local_oid remote_ref remote_oid; do
27 case "$local_oid" in *[!0]*) ;; *) continue ;; esac
28 case "$remote_oid" in
29 *[!0]*) from=$remote_oid ;;
30 *) from=$(git merge-base origin/HEAD "$local_oid" 2>/dev/null) ;;
31 esac
32 if [ -z "$from" ]; then changed=1; continue; fi
33 git diff --quiet "$from" "$local_oid" -- src/core/migrations/ 2>/dev/null || changed=1
34 done
35 [ -n "$changed" ] || exit 0
36 bun scripts/pin-migrations.ts >&2 || exit 0
37 git add -- src/core/migration-pins/ || exit 0
38 git diff --cached --quiet -- src/core/migration-pins/ && exit 0
39 git commit --quiet --no-verify -m "Pin the migration this pull adds" \\
40 -m "bun scripts/pin-migrations.ts, run by the crew's pre-push hook." \\
41 -- src/core/migration-pins/ || exit 0
42 echo "pre-push: this push adds a migration without its pin; the pin is committed now — run git push again" >&2
43 exit 1
44 `;
45
46 /** Install the hook in the clone at `dir`, replacing any earlier copy. */
47 export function installPinHook(dir: string): void {
48 const hooks = join(dir, ".git", "hooks");
49 mkdirSync(hooks, { recursive: true });
50 const path = join(hooks, "pre-push");
51 writeFileSync(path, PIN_HOOK);
52 chmodSync(path, 0o755);
53 }
src/crew/runner.ts +5
@@ -73,6 +73,7 @@ import {
7373 import { mintRunToken } from "../core/run_token.ts";
7474 import { deleteToken } from "../core/tokens.ts";
7575 import { runGit } from "../git/env.ts";
76 import { installPinHook } from "./pin_hook.ts";
7677 import { cloneWithMirror } from "./mirror.ts";
7778 import { isSafeRef } from "../git/guard.ts";
7879 import { runGitCapture } from "../git/env.ts";
@@ -1929,6 +1930,8 @@ async function prepareClone(
19291930 // own exclude file — the repository's `.gitignore` is the repository's.
19301931 mkdirSync(join(dir, ".git", "info"), { recursive: true });
19311932 writeFileSync(join(dir, ".git", "info", "exclude"), CLONE_EXCLUDES);
1933 // A push that adds a migration commits its pin first (#6938).
1934 installPinHook(dir);
19321935 write(
19331936 remote === undefined
19341937 ? `branch ${branch} from ${repo.defaultBranch}`
@@ -2047,6 +2050,8 @@ async function landWork(
20472050 [
20482051 "push",
20492052 "--quiet",
2053 // The clone's hooks are the agent's sandbox's, never the forge's.
2054 "--no-verify",
20502055 `${options.localUrl}/${fork.ownerName}/${fork.name}.git`,
20512056 `${branch}:${branch}`,
20522057 ],
src/kernel/pin_edits.ts +20
@@ -39,3 +39,23 @@ export function parseNameStatus(out: string): Change[] {
3939 }
4040 return changes;
4141 }
42
43 const MIGRATIONS = "src/core/migrations/";
44
45 /**
46 * The migrations a pull adds whose pin is missing (#6938): each added
47 * `src/core/migrations/<name>.ts` for which `pinned(name)` is false.
48 */
49 export function unpinnedAdds(
50 changes: readonly Change[],
51 pinned: (name: string) => boolean,
52 ): string[] {
53 const names: string[] = [];
54 for (const { status, path } of changes) {
55 if (!path.startsWith(MIGRATIONS) || !path.endsWith(".ts")) continue;
56 if (!status.startsWith("A") && !status.startsWith("C")) continue;
57 const name = path.slice(MIGRATIONS.length, -".ts".length);
58 if (!name.includes("/") && !pinned(name)) names.push(name);
59 }
60 return names;
61 }
test/crew-pin-hook.test.ts added +103
@@ -0,0 +1,103 @@
1 /**
2 * The crew clone's pre-push hook (#6938): a fake builder run that commits a
3 * migration and pushes ends with the pin committed and on the remote.
4 */
5 import { afterAll, expect, test } from "bun:test";
6 import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
7 import { join } from "node:path";
8 import { tmpdir } from "node:os";
9 import { installPinHook } from "../src/crew/pin_hook.ts";
10
11 const root = mkdtempSync(join(tmpdir(), "pin-hook-"));
12 afterAll(() => rmSync(root, { recursive: true, force: true }));
13
14 const git = (cwd: string, ...args: string[]) =>
15 Bun.spawnSync(["git", "-c", "user.name=t", "-c", "user.email=t@t", ...args], {
16 cwd,
17 env: { ...process.env, GIT_AUTHOR_NAME: "t", GIT_AUTHOR_EMAIL: "t@t", GIT_COMMITTER_NAME: "t", GIT_COMMITTER_EMAIL: "t@t" },
18 });
19 const ok = (cwd: string, ...args: string[]) => {
20 const r = git(cwd, ...args);
21 if (r.exitCode !== 0) throw new Error(r.stderr.toString());
22 return r.stdout.toString().trim();
23 };
24 const put = (dir: string, file: string, text: string) => {
25 mkdirSync(join(dir, file, ".."), { recursive: true });
26 writeFileSync(join(dir, file), text);
27 };
28
29 // Stands in for the repository's own script: a pin for each unpinned migration.
30 const FAKE_PIN_SCRIPT = `
31 import { existsSync, readdirSync, writeFileSync } from "node:fs";
32 for (const f of readdirSync("src/core/migrations")) {
33 const pin = "src/core/migration-pins/" + f.replace(/\\.ts$/, ".pin");
34 if (!existsSync(pin)) writeFileSync(pin, "id\\n");
35 }
36 `;
37
38 /** A remote with one pinned migration on main, and a builder's clone of it with the hook. */
39 function clone(name: string, withScript = true) {
40 const seed = join(root, `${name}-seed`);
41 const bare = join(root, `${name}.git`);
42 const dir = join(root, name);
43 mkdirSync(seed);
44 ok(seed, "init", "-q", "-b", "main");
45 put(seed, "src/core/migrations/0001-a.ts", "a");
46 put(seed, "src/core/migration-pins/0001-a.pin", "id\n");
47 if (withScript) put(seed, "scripts/pin-migrations.ts", FAKE_PIN_SCRIPT);
48 put(seed, "README.md", "r");
49 ok(seed, "add", "-A");
50 ok(seed, "commit", "-qm", "base");
51 ok(root, "clone", "-q", "--bare", seed, bare);
52 ok(root, "clone", "-q", bare, dir);
53 ok(dir, "checkout", "-qb", "work");
54 installPinHook(dir);
55 return { dir, bare };
56 }
57
58 test("a builder push that adds a migration commits its pin, and the next push carries both", () => {
59 const { dir, bare } = clone("adds");
60 put(dir, "src/core/migrations/0002-b.ts", "b");
61 ok(dir, "add", "-A");
62 ok(dir, "commit", "-qm", "add a migration");
63 const first = git(dir, "push", "-q", "origin", "work");
64 expect(first.exitCode).not.toBe(0);
65 expect(first.stderr.toString()).toContain("run git push again");
66 expect(existsSync(join(dir, "src/core/migration-pins/0002-b.pin"))).toBe(true);
67 expect(ok(dir, "status", "--porcelain")).toBe("");
68 expect(ok(dir, "show", "--name-only", "--format=", "HEAD")).toBe("src/core/migration-pins/0002-b.pin");
69 ok(dir, "push", "-q", "origin", "work");
70 expect(ok(bare, "show", "work:src/core/migration-pins/0002-b.pin")).toBe("id");
71 expect(ok(bare, "show", "work:src/core/migrations/0002-b.ts")).toBe("b");
72 });
73
74 test("a push that touches no migration goes through untouched", () => {
75 const { dir, bare } = clone("plain");
76 put(dir, "README.md", "changed");
77 ok(dir, "commit", "-qam", "words");
78 const head = ok(dir, "rev-parse", "HEAD");
79 ok(dir, "push", "-q", "origin", "work");
80 expect(ok(bare, "rev-parse", "work")).toBe(head);
81 });
82
83 test("a push whose migration already has its pin goes through with no extra commit", () => {
84 const { dir, bare } = clone("pinned");
85 put(dir, "src/core/migrations/0002-b.ts", "b");
86 put(dir, "src/core/migration-pins/0002-b.pin", "id\n");
87 ok(dir, "add", "-A");
88 ok(dir, "commit", "-qm", "add with pin");
89 const head = ok(dir, "rev-parse", "HEAD");
90 ok(dir, "push", "-q", "origin", "work");
91 expect(ok(bare, "rev-parse", "work")).toBe(head);
92 });
93
94 test("a repository without the pin script is not touched", () => {
95 const { dir, bare } = clone("noscript", false);
96 put(dir, "src/core/migrations/0002-b.ts", "b");
97 ok(dir, "add", "-A");
98 ok(dir, "commit", "-qm", "add a migration");
99 const head = ok(dir, "rev-parse", "HEAD");
100 ok(dir, "push", "-q", "origin", "work");
101 expect(ok(bare, "rev-parse", "work")).toBe(head);
102 expect(readFileSync(join(dir, ".git/hooks/pre-push"), "utf8")).toContain("pin-migrations");
103 });
test/pin-refusal.test.ts +18
@@ -3,6 +3,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
33 import { join } from "node:path";
44 import { tmpdir } from "node:os";
55 import { pinRefusal } from "../scripts/gate.ts";
6 import { summariseCheckFailure } from "../src/checks/summary.ts";
67
78 const root = mkdtempSync(join(tmpdir(), "pin-refusal-"));
89 afterAll(() => rmSync(root, { recursive: true, force: true }));
@@ -104,3 +105,20 @@ test("with no base it warns on stderr instead of passing silently", async () =>
104105 }
105106 expect(seen.join("\n")).toContain("pins not checked");
106107 });
108
109 test("a migration added without its pin is named with the one-step fix, last, as the check's summary", async () => {
110 const { dir, base } = repo("unpinned");
111 put(dir, "src/core/migrations/0003-c.ts", "c");
112 put(dir, "src/core/migrations/0004-d.ts", "d");
113 put(dir, "src/core/migration-pins/0004-d.pin", "x");
114 run(dir, "add", "-A");
115 run(dir, "commit", "-qm", "add");
116 const out = await withBase(base, () => pinRefusal(dir));
117 const named =
118 "this pull adds migration 0003-c without its pin: run bun scripts/pin-migrations.ts and commit src/core/migration-pins/0003-c.pin";
119 expect(out).toEqual([named]);
120 const r = Bun.spawnSync(["bun", gateScript], { cwd: dir, env: { ...process.env, FORGE_CHECK_BASE: base } });
121 expect(r.exitCode).toBe(2);
122 const lines = r.stderr.toString().split("\n");
123 expect(summariseCheckFailure(lines, "check exited with code 2").summary).toBe(`gate: ${named}`);
124 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs