Public: anyone can read this; the crew can change it.
A pull lands once: no empty re-landings on main #2018
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/2017-a-pull-lands-once-no-empty-re-landings-o → main 2 commits fork 1 thread · 1 unresolved
7 files
changed +306
−19
docs/PLANNING.md +4
| @@ -524,6 +524,10 @@ Where it decides today: | ||
| 524 | 524 | lands by rule, once its base merges cleanly and it does not cross the |
| 525 | 525 | wall (`docs/CONTRACT.md`). Nobody is asked; the ledger keeps the landing |
| 526 | 526 | as a `ship` by the rule, once per head, so the week's numbers add up. |
| 527 | A pull lands once: the merge reads the pull's row, not the caller's | |
| 528 | copy, and a squash whose tree is the base's own commits nothing — the | |
| 529 | pull closes as merged into the commit that carried it, and main does | |
| 530 | not move (#2017). | |
| 527 | 531 | - **Whose fault, and try again or stop** (`crew/dispatch.ts`, |
| 528 | 532 | `judgeFailedCheck`): a failed check gets another automatic run by rule, |
| 529 | 533 | up to the limit. The decider reads the failure, the log's tail and the |
scripts/gate.ts +1
| @@ -29,6 +29,7 @@ export type Exclusion = { | ||
| 29 | 29 | export const GATE: readonly string[] = [ |
| 30 | 30 | "test/domain-drill.test.ts", |
| 31 | 31 | "test/conflicts.test.ts", |
| 32 | "test/lands-once.test.ts", | |
| 32 | 33 | "test/conflicts-tui.test.ts", |
| 33 | 34 | "test/accounts-retire.test.ts", |
| 34 | 35 | "test/client-home.test.ts", |
src/actions/pulls.ts +3 −1
| @@ -358,7 +358,9 @@ export async function mergePull( | ||
| 358 | 358 | repo: Repo, |
| 359 | 359 | pull: Pull, |
| 360 | 360 | strategy: MergeStrategy, |
| 361 | ): Promise<Result<{ mergeOid: string }, AccessError | MergePullError>> { | |
| 361 | ): Promise< | |
| 362 | Result<{ mergeOid: string; carried: boolean }, AccessError | MergePullError> | |
| 363 | > { | |
| 362 | 364 | const writer = requireWriter(app, actor, repo); |
| 363 | 365 | if (!writer.ok) return writer; |
| 364 | 366 | if (pull.repoId !== repo.id) return err("not_found"); |
src/core/pulls.ts +79 −17
| @@ -20,6 +20,7 @@ import { | ||
| 20 | 20 | mergeBase, |
| 21 | 21 | mergeable, |
| 22 | 22 | resolveOid, |
| 23 | treeOf, | |
| 23 | 24 | writeRef, |
| 24 | 25 | type MergeError, |
| 25 | 26 | type MergeStrategy, |
| @@ -827,9 +828,14 @@ export async function mergePull( | ||
| 827 | 828 | */ |
| 828 | 829 | sealedFrom?: string; |
| 829 | 830 | }, |
| 830 | ): Promise<Result<{ mergeOid: string }, MergePullError>> { | |
| 831 | if (pull.mergedAt !== null) return err("already_merged"); | |
| 832 | if (pull.state === "closed") return err("closed"); | |
| 831 | ): Promise<Result<{ mergeOid: string; carried: boolean }, MergePullError>> { | |
| 832 | // The row, not the caller's copy: a landing reads its pull, runs a gate | |
| 833 | // for minutes, then merges — and the pull may have landed meanwhile. A | |
| 834 | // stale copy merged again is an empty commit on the base (#2017). | |
| 835 | const issue = getIssueById(db, pull.id); | |
| 836 | const live = (issue?.kind === "pull" ? join(db, issue) : undefined) ?? pull; | |
| 837 | if (live.mergedAt !== null) return err("already_merged"); | |
| 838 | if (live.state === "closed") return err("closed"); | |
| 833 | 839 | |
| 834 | 840 | if (input.sealedFrom !== undefined) { |
| 835 | 841 | const there = await pullStatus(input.sealedFrom, pull, input.gate); |
| @@ -860,6 +866,19 @@ export async function mergePull( | ||
| 860 | 866 | // caller: a check computed a request earlier and handed down is a check |
| 861 | 867 | // that can be skipped. |
| 862 | 868 | const status = await pullStatus(dir, pull, input.gate); |
| 869 | // What the pull proposes is already on the base: there is nothing to | |
| 870 | // land, so nothing is committed — the pull closes as merged into the | |
| 871 | // commit that carried it, and the base does not move. | |
| 872 | const carrier = await carriedBy(dir, pull, status, input.strategy); | |
| 873 | if (carrier !== undefined) { | |
| 874 | closeAsMerged(db, pull.id, { | |
| 875 | mergeOid: carrier, | |
| 876 | actorId: input.actorId, | |
| 877 | baseOid: status.baseOid, | |
| 878 | headOid: status.headOid, | |
| 879 | }); | |
| 880 | return ok({ mergeOid: carrier, carried: true }); | |
| 881 | } | |
| 863 | 882 | if (status.reviews !== null && !status.reviews.mergeable) |
| 864 | 883 | return err("review_required"); |
| 865 | 884 | // The check is the latest deploy run of *this* head. Read here, not |
| @@ -880,24 +899,67 @@ export async function mergePull( | ||
| 880 | 899 | }); |
| 881 | 900 | if (!landed.ok) return landed; |
| 882 | 901 | |
| 883 | const now = new Date().toISOString(); | |
| 884 | transact(db, () => { | |
| 885 | db.query( | |
| 886 | "UPDATE pull SET merged_at = ?1, merged_oid = ?2, merged_by = ?3, base_oid = ?4, head_oid = ?5 WHERE issue_id = ?6", | |
| 887 | ).run( | |
| 888 | now, | |
| 889 | landed.value.mergeOid, | |
| 890 | input.actorId, | |
| 891 | status.baseOid, | |
| 892 | status.headOid, | |
| 893 | pull.id, | |
| 894 | ); | |
| 895 | setIssueState(db, pull.id, "closed"); | |
| 902 | closeAsMerged(db, pull.id, { | |
| 903 | mergeOid: landed.value.mergeOid, | |
| 904 | actorId: input.actorId, | |
| 905 | baseOid: status.baseOid, | |
| 906 | headOid: status.headOid, | |
| 896 | 907 | }); |
| 897 | 908 | |
| 898 | 909 | // The merge commit is kept reachable forever, independent of the base branch. |
| 899 | 910 | await writeRef(dir, `refs/pull/${pull.number}/merged`, landed.value.mergeOid); |
| 900 | return ok({ mergeOid: landed.value.mergeOid }); | |
| 911 | return ok({ mergeOid: landed.value.mergeOid, carried: false }); | |
| 912 | } | |
| 913 | ||
| 914 | /** | |
| 915 | * The commit on the base that already carries a squash of this pull, when | |
| 916 | * squashing it again would commit the base's own tree: the pull's own | |
| 917 | * landing if the base still holds it, else the base's tip. Undefined when | |
| 918 | * the squash would change something — or is not a squash, or does not | |
| 919 | * merge cleanly, which the merge itself reports. | |
| 920 | */ | |
| 921 | async function carriedBy( | |
| 922 | dir: string, | |
| 923 | pull: Pull, | |
| 924 | status: { baseOid: string; headOid: string; ahead: number; mergeable: boolean }, | |
| 925 | strategy: MergeStrategy, | |
| 926 | ): Promise<string | undefined> { | |
| 927 | if (strategy !== "squash" || status.ahead === 0 || !status.mergeable) | |
| 928 | return undefined; | |
| 929 | const merged = await mergeable(dir, status.baseOid, status.headOid); | |
| 930 | if (!merged.ok) return undefined; | |
| 931 | if (merged.value.treeOid !== (await treeOf(dir, status.baseOid))) | |
| 932 | return undefined; | |
| 933 | const landing = await resolveOid(dir, `refs/pull/${pull.number}/merged`); | |
| 934 | if ( | |
| 935 | landing !== undefined && | |
| 936 | (await mergeBase(dir, landing, status.baseOid)) === landing | |
| 937 | ) | |
| 938 | return landing; | |
| 939 | return status.baseOid; | |
| 940 | } | |
| 941 | ||
| 942 | /** Record the landing on the row and close the pull — once: a pull already | |
| 943 | * merged keeps the commit it first landed as. */ | |
| 944 | function closeAsMerged( | |
| 945 | db: Database, | |
| 946 | pullId: string, | |
| 947 | landed: { mergeOid: string; actorId: string; baseOid: string; headOid: string }, | |
| 948 | ): void { | |
| 949 | const now = new Date().toISOString(); | |
| 950 | transact(db, () => { | |
| 951 | db.query( | |
| 952 | "UPDATE pull SET merged_at = ?1, merged_oid = ?2, merged_by = ?3, base_oid = ?4, head_oid = ?5 WHERE issue_id = ?6 AND merged_at IS NULL", | |
| 953 | ).run( | |
| 954 | now, | |
| 955 | landed.mergeOid, | |
| 956 | landed.actorId, | |
| 957 | landed.baseOid, | |
| 958 | landed.headOid, | |
| 959 | pullId, | |
| 960 | ); | |
| 961 | setIssueState(db, pullId, "closed"); | |
| 962 | }); | |
| 901 | 963 | } |
| 902 | 964 | |
| 903 | 965 | export { getIssueById }; |
src/crew/autonomy.ts +32 −1
| @@ -22,7 +22,7 @@ import { | ||
| 22 | 22 | } from "../actions/issues.ts"; |
| 23 | 23 | import type { AccessError } from "../actions/access.ts"; |
| 24 | 24 | import { gateOnLand, releaseOnLand } from "../checks/landing.ts"; |
| 25 | import { mergeable, resolveOid } from "../git/merge.ts"; | |
| 25 | import { mergeable, resolveOid, treeOf } from "../git/merge.ts"; | |
| 26 | 26 | import { repoDir } from "../git/repos.ts"; |
| 27 | 27 | import { mergePull } from "../actions/pulls.ts"; |
| 28 | 28 | import { |
| @@ -1268,6 +1268,11 @@ export async function landIfGreen( | ||
| 1268 | 1268 | ): Promise<boolean> { |
| 1269 | 1269 | if (repo.autonomy !== "autonomous" || repo.autonomyBy === null) return false; |
| 1270 | 1270 | if (pull.state !== "open" || pull.mergedAt !== null) return false; |
| 1271 | // The caller's copy may predate a landing: the row says whether it is | |
| 1272 | // still open (#2017). | |
| 1273 | const row = getPull(app.db, repo.id, pull.number); | |
| 1274 | if (row === undefined || row.state !== "open" || row.mergedAt !== null) | |
| 1275 | return false; | |
| 1271 | 1276 | // A person said "not this one yet": nothing below happens until it is |
| 1272 | 1277 | // released — no landing, no review sent, no run to bring the branch up |
| 1273 | 1278 | // to date (`core/holds.ts`). |
| @@ -1309,6 +1314,18 @@ export async function landIfGreen( | ||
| 1309 | 1314 | } |
| 1310 | 1315 | return false; |
| 1311 | 1316 | } |
| 1317 | // Nothing the base does not already hold: no gate and no review, for | |
| 1318 | // there is nothing to land. The merge closes the pull as merged into | |
| 1319 | // the commit that carried it, and main does not move (#2017). | |
| 1320 | if (clean.value.treeOid === (await treeOf(dir, baseTip))) { | |
| 1321 | const closed = await mergePull(app, by, repo, pull, "squash"); | |
| 1322 | log.info("already on the base, not landed again", { | |
| 1323 | repo: `${repo.ownerName}/${repo.name}`, | |
| 1324 | pull: pull.number, | |
| 1325 | closed: closed.ok, | |
| 1326 | }); | |
| 1327 | return false; | |
| 1328 | } | |
| 1312 | 1329 | } |
| 1313 | 1330 | // A pull that loosens the contract is a change of policy, which is a |
| 1314 | 1331 | // wall: decided by a person, never crossed by the crew. The pull stays |
| @@ -1386,6 +1403,17 @@ export async function landIfGreen( | ||
| 1386 | 1403 | addComment(app, by, repo, getIssueById(app.db, pull.id) ?? pull, verdict.comment); |
| 1387 | 1404 | return false; |
| 1388 | 1405 | } |
| 1406 | // The gate took minutes: the pull it was run on must still be the one | |
| 1407 | // here — open, unmerged, at the same head — or another landing, or a | |
| 1408 | // push, came first and this verdict is about nothing. | |
| 1409 | const still = getPull(app.db, repo.id, pull.number); | |
| 1410 | if ( | |
| 1411 | still === undefined || | |
| 1412 | still.state !== "open" || | |
| 1413 | still.mergedAt !== null || | |
| 1414 | still.headOid !== pull.headOid | |
| 1415 | ) | |
| 1416 | return false; | |
| 1389 | 1417 | // Green and clear of the wall lands. An autonomous repository does not |
| 1390 | 1418 | // wait on a person; the ledger keeps the landing as the rule it is, once |
| 1391 | 1419 | // per head, so the week's numbers still add up. |
| @@ -1428,6 +1456,9 @@ export async function landIfGreen( | ||
| 1428 | 1456 | return false; |
| 1429 | 1457 | } |
| 1430 | 1458 | conflicted.delete(pull.id); |
| 1459 | // Closed as merged into what already carried it: main did not move, so | |
| 1460 | // there is no release to cut and nothing new was landed. | |
| 1461 | if (merged.value.carried) return false; | |
| 1431 | 1462 | resolveOpen(app.db, pull.id, "land", "shipped"); |
| 1432 | 1463 | // On the forge's own code, the landed tree is checked out and preflighted |
| 1433 | 1464 | // off to the side; the landing does not wait on it (#1662). |
src/git/merge.ts +12
| @@ -268,6 +268,18 @@ export async function resolveOid( | ||
| 268 | 268 | return result.ok ? result.value.trim() : undefined; |
| 269 | 269 | } |
| 270 | 270 | |
| 271 | /** The tree a commit records: two commits with one tree change nothing. */ | |
| 272 | export async function treeOf( | |
| 273 | dir: string, | |
| 274 | oid: string, | |
| 275 | ): Promise<string | undefined> { | |
| 276 | if (!isSafeOid(oid)) return undefined; | |
| 277 | const result = await runGit(["rev-parse", "--verify", `${oid}^{tree}`], { | |
| 278 | cwd: dir, | |
| 279 | }); | |
| 280 | return result.ok ? result.value.trim() : undefined; | |
| 281 | } | |
| 282 | ||
| 271 | 283 | /** The merge base of two commits — the fork point a PR diff is measured from. */ |
| 272 | 284 | export async function mergeBase( |
| 273 | 285 | dir: string, |
test/lands-once.test.ts added +175
| @@ -0,0 +1,175 @@ | ||
| 1 | /** | |
| 2 | * A pull lands once (#2017). A landing reads its pull, runs a gate for | |
| 3 | * minutes, then merges; a copy of the pull read before an earlier landing | |
| 4 | * must not squash it onto main a second time — that is an empty commit | |
| 5 | * with the pull's title, a moved main, and a release. And a squash whose | |
| 6 | * tree is the base's own commits nothing: the pull closes as merged into | |
| 7 | * the commit that already carried it. | |
| 8 | */ | |
| 9 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 10 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 11 | import { tmpdir } from "node:os"; | |
| 12 | import { join } from "node:path"; | |
| 13 | import { createApp, type App } from "../src/app.ts"; | |
| 14 | import { mergePull, openPull } from "../src/actions/pulls.ts"; | |
| 15 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 16 | import { getPull, type Pull } from "../src/core/pulls.ts"; | |
| 17 | import { getRepoById, type Repo } from "../src/core/repos.ts"; | |
| 18 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 19 | import { createUser, type User } from "../src/core/users.ts"; | |
| 20 | import { landIfGreen, sweepGreen } from "../src/crew/autonomy.ts"; | |
| 21 | import { repoDir } from "../src/git/repos.ts"; | |
| 22 | ||
| 23 | describe("a pull lands once", () => { | |
| 24 | let dataDir: string; | |
| 25 | let app: App; | |
| 26 | let alice: User; | |
| 27 | let repo: Repo; | |
| 28 | const fresh = () => getRepoById(app.db, repo.id)!; | |
| 29 | const git = (...args: string[]) => | |
| 30 | Bun.spawnSync(["git", ...args], { | |
| 31 | cwd: repoDir(app.reposRoot, repo.ownerName, repo.name), | |
| 32 | }) | |
| 33 | .stdout.toString() | |
| 34 | .trim(); | |
| 35 | const mainTip = () => git("rev-parse", "refs/heads/main"); | |
| 36 | const mainCount = () => Number(git("rev-list", "--count", "refs/heads/main")); | |
| 37 | ||
| 38 | beforeAll(async () => { | |
| 39 | dataDir = await mkdtemp(join(tmpdir(), "forge-lands-once-")); | |
| 40 | app = createApp({ dataDir, port: 0 }); | |
| 41 | const a = await createUser(app.db, { | |
| 42 | trusted: true, | |
| 43 | username: "alice", | |
| 44 | email: "[email protected]", | |
| 45 | password: "a-good-passphrase", | |
| 46 | adminIfFirst: true, | |
| 47 | }); | |
| 48 | if (!a.ok) throw new Error(a.error); | |
| 49 | alice = a.value; | |
| 50 | const made = await createRepo(app, alice, { | |
| 51 | name: "engine", | |
| 52 | visibility: "private", | |
| 53 | }); | |
| 54 | if (!made.ok) throw new Error(String(made.error)); | |
| 55 | repo = made.value; | |
| 56 | const seeded = await commitToBranch(app, alice, repo, { | |
| 57 | message: "Start", | |
| 58 | files: [{ path: "README.md", content: "# engine\n" }], | |
| 59 | }); | |
| 60 | if (!seeded.ok) throw new Error(JSON.stringify(seeded.error)); | |
| 61 | app.db | |
| 62 | .query( | |
| 63 | "UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2 WHERE id = ?1", | |
| 64 | ) | |
| 65 | .run(repo.id, alice.id); | |
| 66 | }); | |
| 67 | ||
| 68 | afterAll(async () => { | |
| 69 | await app.close?.(); | |
| 70 | await rm(dataDir, { recursive: true, force: true }); | |
| 71 | }); | |
| 72 | ||
| 73 | function passCheck(pull: Pull): Pull { | |
| 74 | const check = createRun(app.db, { | |
| 75 | repoId: repo.id, | |
| 76 | kind: "check", | |
| 77 | issueId: pull.id, | |
| 78 | env: `pull-${pull.number}`, | |
| 79 | commitOid: pull.headOid, | |
| 80 | reason: "pull", | |
| 81 | }); | |
| 82 | claimRun(app.db, check.id); | |
| 83 | finishRun(app.db, check.id, { state: "ok", summary: "check passed" }); | |
| 84 | return pull; | |
| 85 | } | |
| 86 | ||
| 87 | async function push(branch: string, path: string, content: string) { | |
| 88 | const pushed = await commitToBranch(app, alice, repo, { | |
| 89 | branch, | |
| 90 | base: "main", | |
| 91 | message: path, | |
| 92 | files: [{ path, content }], | |
| 93 | }); | |
| 94 | if (!pushed.ok) throw new Error(JSON.stringify(pushed.error)); | |
| 95 | } | |
| 96 | ||
| 97 | async function greenPull(name: string, content?: string): Promise<Pull> { | |
| 98 | await push(`feature/${name}`, `${name}.ts`, content ?? `export const n = "${name}";\n`); | |
| 99 | const opened = await openPull(app, alice, repo, { | |
| 100 | title: name, | |
| 101 | baseBranch: "main", | |
| 102 | headBranch: `feature/${name}`, | |
| 103 | }); | |
| 104 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 105 | return passCheck(getPull(app.db, repo.id, opened.value.number)!); | |
| 106 | } | |
| 107 | ||
| 108 | test("landing the same copy of a pull twice makes one commit", async () => { | |
| 109 | const pull = await greenPull("twice"); | |
| 110 | const before = mainCount(); | |
| 111 | expect(await landIfGreen(app, fresh(), pull)).toBe(true); | |
| 112 | const landed = mainTip(); | |
| 113 | // The copy read before the landing, as a sweep that started earlier holds. | |
| 114 | expect(await landIfGreen(app, fresh(), pull)).toBe(false); | |
| 115 | expect(mainCount()).toBe(before + 1); | |
| 116 | expect(mainTip()).toBe(landed); | |
| 117 | const after = getPull(app.db, repo.id, pull.number)!; | |
| 118 | expect(after.mergedOid).toBe(landed); | |
| 119 | expect(after.state).toBe("closed"); | |
| 120 | }); | |
| 121 | ||
| 122 | test("a push after the landing and a sweep do not move main", async () => { | |
| 123 | const pull = await greenPull("pushed"); | |
| 124 | expect(await landIfGreen(app, fresh(), pull)).toBe(true); | |
| 125 | const landed = mainTip(); | |
| 126 | await push("feature/pushed", "pushed-again.ts", "export const m = 1;\n"); | |
| 127 | expect(await sweepGreen(app, fresh())).toBe(0); | |
| 128 | // Even a caller still holding the pull as it was before it landed. | |
| 129 | expect(await landIfGreen(app, fresh(), passCheck(pull))).toBe(false); | |
| 130 | expect(mainTip()).toBe(landed); | |
| 131 | expect(getPull(app.db, repo.id, pull.number)!.mergedOid).toBe(landed); | |
| 132 | }); | |
| 133 | ||
| 134 | test("two landings at once make one commit", async () => { | |
| 135 | const pull = await greenPull("racing"); | |
| 136 | const before = mainCount(); | |
| 137 | const results = await Promise.all([ | |
| 138 | landIfGreen(app, fresh(), pull), | |
| 139 | landIfGreen(app, fresh(), pull), | |
| 140 | ]); | |
| 141 | expect(results.filter(Boolean)).toHaveLength(1); | |
| 142 | expect(mainCount()).toBe(before + 1); | |
| 143 | expect(getPull(app.db, repo.id, pull.number)!.mergedOid).toBe(mainTip()); | |
| 144 | }); | |
| 145 | ||
| 146 | test("a pull whose change main already holds closes as merged, and main does not move", async () => { | |
| 147 | const first = await greenPull("same-a", "export const same = 1;\n"); | |
| 148 | expect(await landIfGreen(app, fresh(), first)).toBe(true); | |
| 149 | // The same file, the same content, from another branch. | |
| 150 | await push("feature/same-b", "same-a.ts", "export const same = 1;\n"); | |
| 151 | const opened = await openPull(app, alice, repo, { | |
| 152 | title: "same-b", | |
| 153 | baseBranch: "main", | |
| 154 | headBranch: "feature/same-b", | |
| 155 | }); | |
| 156 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 157 | const second = passCheck(getPull(app.db, repo.id, opened.value.number)!); | |
| 158 | const tip = mainTip(); | |
| 159 | expect(await landIfGreen(app, fresh(), second)).toBe(false); | |
| 160 | expect(mainTip()).toBe(tip); | |
| 161 | const closed = getPull(app.db, repo.id, second.number)!; | |
| 162 | expect(closed.state).toBe("closed"); | |
| 163 | expect(closed.mergedOid).toBe(tip); | |
| 164 | }); | |
| 165 | ||
| 166 | test("a person pressing merge on a pull that already landed is told so", async () => { | |
| 167 | const pull = await greenPull("pressed"); | |
| 168 | expect(await landIfGreen(app, fresh(), pull)).toBe(true); | |
| 169 | const tip = mainTip(); | |
| 170 | const again = await mergePull(app, alice, fresh(), pull, "squash"); | |
| 171 | expect(again.ok).toBe(false); | |
| 172 | if (!again.ok) expect(again.error).toBe("already_merged"); | |
| 173 | expect(mainTip()).toBe(tip); | |
| 174 | }); | |
| 175 | }); |
new above
offline
The fallback returns status.baseOid, which can be an unrelated commit when the landing ref is missing. Prefer finding the commit that introduced the tree, or mark the pull as carried by the base tip in its history.