48668e2
@@ -1299,7 +1299,9 @@
runs on main's tip with that pull merged, which is the proof a re-check of
its own would give, and a red candidate bisects as above. So a landing that
moves main does not send every other approved pull off for a check of its
own first; the batch lands in one gate. A pull's own check runs again only
when its head changes. A person's merge, which no candidate gated, still
when its head changes: an approval checks a head that was never checked,
and a green one only when main moved since and the repository is not
autonomous, whose land gates on today's main anyway (`src/checks/approval.ts`). A person's merge, which no candidate gated, still
refuses a stale green. Each sweep reads every open pull, oldest first, not
only the newest page, so an older approved pull is batched with the rest
rather than joining alone on its own event. The autonomy tick line counts the approved pulls the
@@ -0,0 +1,35 @@
/**
 * Whether an approval sends a pull's head through the lane again (#4391).
 *
 * An approval is what lets a reader's pull run, so a head that has never
 * been checked is checked when it is approved. But a crew reviewer approves
 * nearly every pull minutes after its push check passed, and checking that
 * head again cost the lane a full run per pull: 21 of 52 heads in one hour
 * were checked twice, and 27 checks queued behind them.
 *
 * A head already green in its environment is checked again only when that
 * says something new: its base has moved since the green check merged with
 * it, and nothing else will judge the pull on today's base. An autonomous
 * repository's land gates the merge on today's base anyway
 * (`checks/landing.ts`), so there the approval adds nothing.
 */
import type { Run } from "../core/runs.ts";
export function checkOnApproval(
  /** The runs at this head (`checksForHead`). */
  runs: readonly Run[],
  /** The environment the pull's checks run in, `pull-<n>`. */
  env: string,
  /** The base branch's tip now; undefined when it could not be read. */
  baseTip: string | undefined,
  /** Whether the pull lands through the merge queue's full gate. */
  landGated: boolean,
): boolean {
  const green = runs.filter(
    (r) => r.kind === "check" && r.env === env && r.state === "ok",
  );
  if (green.length === 0) return true;
  if (landGated || baseTip === undefined) return false;
  return !green.some((r) => r.baseOid === baseTip);
}
@@ -45,6 +45,7 @@
import {
import { Gate, runGit } from "../git/env.ts";
import { resolveOid } from "../git/merge.ts";
import { mergeForCheck } from "./merged.ts";
import { checkOnApproval } from "./approval.ts";
import { checkKey, provenBy, recordProven } from "./proven.ts";
import { giveHistory } from "./history.ts";
import { readLearned } from "./learned.ts";
@@ -1093,15 +1094,16 @@
export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
        const pull = getPull(app.db, repo.id, number);
        if (!pull || pull.state !== "open") return;
        const head = await headOf(repo, pull);
        if (head !== undefined)
          await request(
            repo,
            `pull-${pull.number}`,
            head,
            "approved",
            pull.id,
            actor,
          );
        if (head === undefined) return;
        // A head already green is not checked again for nothing (#4391).
        const env = `pull-${pull.number}`;
        const dir = await pullDir(app.db, app.reposRoot, repo, pull);
        const tip = await resolveOid(dir, `refs/heads/${pull.baseBranch}`);
        // The merge queue gates only an autonomous repository's lands (`crew/autonomy.ts` admit).
        const landGated = repo.autonomy === "autonomous" && repo.autonomyBy !== null;
        if (!checkOnApproval(checksForHead(app.db, pull.id, head), env, tip, landGated))
          return;
        await request(repo, env, head, "approved", pull.id, actor);
        return;
      }
      // Landed or closed: nothing left to check, so nothing waits in the lane.
@@ -0,0 +1,47 @@
import { describe, expect, test } from "bun:test";
import { checkOnApproval } from "../src/checks/approval.ts";
import type { Run } from "../src/core/runs.ts";
const run = (over: Partial<Run>): Run =>
  ({
    id: "r",
    kind: "check",
    env: "pull-7",
    state: "ok",
    baseOid: "base1",
    ...over,
  }) as Run;
describe("checkOnApproval: an approval checks a head again only when it says something new (#4391)", () => {
  test("a head never checked is checked when approved", () => {
    expect(checkOnApproval([], "pull-7", "base1", true)).toBe(true);
    expect(checkOnApproval([], "pull-7", "base1", false)).toBe(true);
  });
  test("a head with only a red, a cancelled or another environment's green is checked", () => {
    const runs = [
      run({ state: "failed" }),
      run({ state: "cancelled" }),
      run({ env: "pull-8" }),
      run({ kind: "deploy" }),
    ];
    expect(checkOnApproval(runs, "pull-7", "base1", true)).toBe(true);
  });
  test("a green head on an unmoved base is not checked again", () => {
    expect(checkOnApproval([run({})], "pull-7", "base1", false)).toBe(false);
  });
  test("a green head on a moved base is checked again where nothing gates the land", () => {
    expect(checkOnApproval([run({})], "pull-7", "base2", false)).toBe(true);
    expect(checkOnApproval([run({ baseOid: "" })], "pull-7", "base2", false)).toBe(true);
  });
  test("a green head on a moved base is not checked again where the land gates on today's base", () => {
    expect(checkOnApproval([run({})], "pull-7", "base2", true)).toBe(false);
  });
  test("an unreadable base is no reason to check a green head again", () => {
    expect(checkOnApproval([run({})], "pull-7", undefined, false)).toBe(false);
  });
});
@@ -44,7 +44,7 @@
import { repoDir } from "../src/git/repos.ts";
import { STALE_AFTER_MS } from "../src/runs/lease.ts";
import { pullWord, servingHistory } from "../src/web/studio/words.ts";
import { createUser, type User } from "../src/core/users.ts";
import type { Repo } from "../src/core/repos.ts";
import { getRepo, type Repo } from "../src/core/repos.ts";
import { serve, type Servers } from "../src/http/server.ts";
import { listEvents } from "../src/core/events.ts";
import { readRunLog } from "../src/runs/log.ts";
@@ -608,6 +608,50 @@
describe("checks", () => {
    expect(second.state).toBe("ok");
  }, 60_000);
  test("approving a head whose check is already green queues no second check (#4391)", async () => {
    const based = await commitToBranch(app, alice, repo, {
      message: "The base's check passes",
      files: [{ path: "forge.json", content: checkJson("true") }],
    });
    if (!based.ok) throw new Error(JSON.stringify(based.error));
    // Bob's fork, made by the fork test above when it ran first.
    const fork =
      getRepo(app.db, "bob", repo.name) ??
      (await forkRepo(app, bob, repo).then((f) => (f.ok ? f.value : undefined)));
    if (fork === undefined) throw new Error("no fork");
    const committed = await commitToBranch(app, bob, fork, {
      branch: "green-then-approved",
      message: "Green before the second look",
      files: [{ path: "GREEN.md", content: "x\n" }],
    });
    if (!committed.ok) throw new Error(JSON.stringify(committed.error));
    const opened = await openPull(app, bob, repo, {
      title: "Green before the second look",
      baseBranch: "main",
      headBranch: "green-then-approved",
      headRepo: fork,
    });
    if (!opened.ok) throw new Error(String(opened.error));
    const pull = opened.value;
    const mine = (): Run[] =>
      listRuns(app.db, repo.id, { kind: "check" }).filter(
        (r) => r.issueId === pull.id && r.commitOid === committed.value.commitOid,
      );
    // The first approval is what lets a reader's head run at all.
    const approved = await submitReview(app, alice, repo, pull, { state: "approved" });
    if (!approved.ok) throw new Error(String(approved.error));
    const first = await settled((r) => r.issueId === pull.id);
    expect(first.state).toBe("ok");
    // A second approval of the same green head on the same base is nothing new.
    const again = await submitReview(app, alice, repo, pull, {
      state: "approved",
      body: "Still good.",
    });
    if (!again.ok) throw new Error(String(again.error));
    await Bun.sleep(300);
    expect(mine().map((r) => r.id)).toEqual([first.id]);
  }, 60_000);
  test("a commit already proven green is not run again; the re-request answers with the earlier verdict (#1795)", async () => {
    const made = await commitToBranch(app, alice, repo, {
      message: "Proven once",
		@@ -1299,7 +1299,9 @@ runs on main's tip with that pull merged, which is the proof a re-check of
1299	1299	its own would give, and a red candidate bisects as above. So a landing that
1300	1300	moves main does not send every other approved pull off for a check of its
1301	1301	own first; the batch lands in one gate. A pull's own check runs again only
1302		when its head changes. A person's merge, which no candidate gated, still
	1302	when its head changes: an approval checks a head that was never checked,
	1303	and a green one only when main moved since and the repository is not
	1304	autonomous, whose land gates on today's main anyway (`src/checks/approval.ts`). A person's merge, which no candidate gated, still
1303	1305	refuses a stale green. Each sweep reads every open pull, oldest first, not
1304	1306	only the newest page, so an older approved pull is batched with the rest
1305	1307	rather than joining alone on its own event. The autonomy tick line counts the approved pulls the
		@@ -0,0 +1,35 @@
	1	/**
	2	* Whether an approval sends a pull's head through the lane again (#4391).
	3	*
	4	* An approval is what lets a reader's pull run, so a head that has never
	5	* been checked is checked when it is approved. But a crew reviewer approves
	6	* nearly every pull minutes after its push check passed, and checking that
	7	* head again cost the lane a full run per pull: 21 of 52 heads in one hour
	8	* were checked twice, and 27 checks queued behind them.
	9	*
	10	* A head already green in its environment is checked again only when that
	11	* says something new: its base has moved since the green check merged with
	12	* it, and nothing else will judge the pull on today's base. An autonomous
	13	* repository's land gates the merge on today's base anyway
	14	* (`checks/landing.ts`), so there the approval adds nothing.
	15	*/
	16
	17	import type { Run } from "../core/runs.ts";
	18
	19	export function checkOnApproval(
	20	/** The runs at this head (`checksForHead`). */
	21	runs: readonly Run[],
	22	/** The environment the pull's checks run in, `pull-<n>`. */
	23	env: string,
	24	/** The base branch's tip now; undefined when it could not be read. */
	25	baseTip: string \| undefined,
	26	/** Whether the pull lands through the merge queue's full gate. */
	27	landGated: boolean,
	28	): boolean {
	29	const green = runs.filter(
	30	(r) => r.kind === "check" && r.env === env && r.state === "ok",
	31	);
	32	if (green.length === 0) return true;
	33	if (landGated \|\| baseTip === undefined) return false;
	34	return !green.some((r) => r.baseOid === baseTip);
	35	}
		@@ -45,6 +45,7 @@ import {
45	45	import { Gate, runGit } from "../git/env.ts";
46	46	import { resolveOid } from "../git/merge.ts";
47	47	import { mergeForCheck } from "./merged.ts";
	48	import { checkOnApproval } from "./approval.ts";
48	49	import { checkKey, provenBy, recordProven } from "./proven.ts";
49	50	import { giveHistory } from "./history.ts";
50	51	import { readLearned } from "./learned.ts";
		@@ -1093,15 +1094,16 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
1093	1094	const pull = getPull(app.db, repo.id, number);
1094	1095	if (!pull \|\| pull.state !== "open") return;
1095	1096	const head = await headOf(repo, pull);
1096		if (head !== undefined)
1097		await request(
1098		repo,
1099		`pull-${pull.number}`,
1100		head,
1101		"approved",
1102		pull.id,
1103		actor,
1104		);
	1097	if (head === undefined) return;
	1098	// A head already green is not checked again for nothing (#4391).
	1099	const env = `pull-${pull.number}`;
	1100	const dir = await pullDir(app.db, app.reposRoot, repo, pull);
	1101	const tip = await resolveOid(dir, `refs/heads/${pull.baseBranch}`);
	1102	// The merge queue gates only an autonomous repository's lands (`crew/autonomy.ts` admit).
	1103	const landGated = repo.autonomy === "autonomous" && repo.autonomyBy !== null;
	1104	if (!checkOnApproval(checksForHead(app.db, pull.id, head), env, tip, landGated))
	1105	return;
	1106	await request(repo, env, head, "approved", pull.id, actor);
1105	1107	return;
1106	1108	}
1107	1109	// Landed or closed: nothing left to check, so nothing waits in the lane.
		@@ -0,0 +1,47 @@
	1	import { describe, expect, test } from "bun:test";
	2	import { checkOnApproval } from "../src/checks/approval.ts";
	3	import type { Run } from "../src/core/runs.ts";
	4
	5	const run = (over: Partial<Run>): Run =>
	6	({
	7	id: "r",
	8	kind: "check",
	9	env: "pull-7",
	10	state: "ok",
	11	baseOid: "base1",
	12	...over,
	13	}) as Run;
	14
	15	describe("checkOnApproval: an approval checks a head again only when it says something new (#4391)", () => {
	16	test("a head never checked is checked when approved", () => {
	17	expect(checkOnApproval([], "pull-7", "base1", true)).toBe(true);
	18	expect(checkOnApproval([], "pull-7", "base1", false)).toBe(true);
	19	});
	20
	21	test("a head with only a red, a cancelled or another environment's green is checked", () => {
	22	const runs = [
	23	run({ state: "failed" }),
	24	run({ state: "cancelled" }),
	25	run({ env: "pull-8" }),
	26	run({ kind: "deploy" }),
	27	];
	28	expect(checkOnApproval(runs, "pull-7", "base1", true)).toBe(true);
	29	});
	30
	31	test("a green head on an unmoved base is not checked again", () => {
	32	expect(checkOnApproval([run({})], "pull-7", "base1", false)).toBe(false);
	33	});
	34
	35	test("a green head on a moved base is checked again where nothing gates the land", () => {
	36	expect(checkOnApproval([run({})], "pull-7", "base2", false)).toBe(true);
	37	expect(checkOnApproval([run({ baseOid: "" })], "pull-7", "base2", false)).toBe(true);
	38	});
	39
	40	test("a green head on a moved base is not checked again where the land gates on today's base", () => {
	41	expect(checkOnApproval([run({})], "pull-7", "base2", true)).toBe(false);
	42	});
	43
	44	test("an unreadable base is no reason to check a green head again", () => {
	45	expect(checkOnApproval([run({})], "pull-7", undefined, false)).toBe(false);
	46	});
	47	});
		@@ -44,7 +44,7 @@ import { repoDir } from "../src/git/repos.ts";
44	44	import { STALE_AFTER_MS } from "../src/runs/lease.ts";
45	45	import { pullWord, servingHistory } from "../src/web/studio/words.ts";
46	46	import { createUser, type User } from "../src/core/users.ts";
47		import type { Repo } from "../src/core/repos.ts";
	47	import { getRepo, type Repo } from "../src/core/repos.ts";
48	48	import { serve, type Servers } from "../src/http/server.ts";
49	49	import { listEvents } from "../src/core/events.ts";
50	50	import { readRunLog } from "../src/runs/log.ts";
		@@ -608,6 +608,50 @@ describe("checks", () => {
608	608	expect(second.state).toBe("ok");
609	609	}, 60_000);
610	610
	611	test("approving a head whose check is already green queues no second check (#4391)", async () => {
	612	const based = await commitToBranch(app, alice, repo, {
	613	message: "The base's check passes",
	614	files: [{ path: "forge.json", content: checkJson("true") }],
	615	});
	616	if (!based.ok) throw new Error(JSON.stringify(based.error));
	617	// Bob's fork, made by the fork test above when it ran first.
	618	const fork =
	619	getRepo(app.db, "bob", repo.name) ??
	620	(await forkRepo(app, bob, repo).then((f) => (f.ok ? f.value : undefined)));
	621	if (fork === undefined) throw new Error("no fork");
	622	const committed = await commitToBranch(app, bob, fork, {
	623	branch: "green-then-approved",
	624	message: "Green before the second look",
	625	files: [{ path: "GREEN.md", content: "x\n" }],
	626	});
	627	if (!committed.ok) throw new Error(JSON.stringify(committed.error));
	628	const opened = await openPull(app, bob, repo, {
	629	title: "Green before the second look",
	630	baseBranch: "main",
	631	headBranch: "green-then-approved",
	632	headRepo: fork,
	633	});
	634	if (!opened.ok) throw new Error(String(opened.error));
	635	const pull = opened.value;
	636	const mine = (): Run[] =>
	637	listRuns(app.db, repo.id, { kind: "check" }).filter(
	638	(r) => r.issueId === pull.id && r.commitOid === committed.value.commitOid,
	639	);
	640	// The first approval is what lets a reader's head run at all.
	641	const approved = await submitReview(app, alice, repo, pull, { state: "approved" });
	642	if (!approved.ok) throw new Error(String(approved.error));
	643	const first = await settled((r) => r.issueId === pull.id);
	644	expect(first.state).toBe("ok");
	645	// A second approval of the same green head on the same base is nothing new.
	646	const again = await submitReview(app, alice, repo, pull, {
	647	state: "approved",
	648	body: "Still good.",
	649	});
	650	if (!again.ok) throw new Error(String(again.error));
	651	await Bun.sleep(300);
	652	expect(mine().map((r) => r.id)).toEqual([first.id]);
	653	}, 60_000);
	654
611	655	test("a commit already proven green is not run again; the re-request answers with the earlier verdict (#1795)", async () => {
612	656	const made = await commitToBranch(app, alice, repo, {
613	657	message: "Proven once",