Public: anyone can read this; the crew can change it.
A failed recombine leaves the combined card stale with the failure's sentence, and the docs stop saying main alone does not recombine #10069
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9995-a-failed-recombine-leaves-the-combined-c → main 3 commits fork 3 threads · 3 unresolved
5 files
changed +85
−11
docs/features/upstream-landing.md +2 −2
| @@ -24,7 +24,7 @@ The rule lives in `landingKind` (`src/core/landing_kind.ts`) and `mergePull` app | ||
| 24 | 24 | |
| 25 | 25 | When main has moved since a kept fork's head, the forge merges main into that head in a branch of the base repository, `combined/N` (`src/git/combined.ts`), on opening the pull, on every push to the fork, and again just before landing. The author's branch is never written. Each conflicting block keeps main's side, with no review: the author's side of that block is dropped from the landed tree, and the note says so and how to get it back (merge main into their branch, resolve, push: a head that already has main is its own combined build). A conflict git cannot pick a side of (modify/delete, rename) is left open. The combined commit's message is the note, and its `Combined-Base` and `Combined-Kept` trailers are the facts the pull renders from (`src/core/combined_view.ts`): a "Combined with main" card on the page and the same section on the terminal's pull screen. The preview builds `combined/N`, under the approval of the head it carries (read from git, `headUnder`), and landing merges it, so the author's head stays an ancestor of main. Commits pushed on top of `combined/N` by a writer are kept by the next combining. |
| 26 | 26 | |
| 27 | A push to main alone does not recombine: the card marks itself stale, naming the main commit it was built on, until the next push to the fork or the landing recombines. The pull's fast check runs on the combined commit, not the author's head alone (`checks/service.ts`): its log and the checks line say "checked as combined with main at abc1234", the run records that base, and the green is stale only when `combined/N` is built on an older main than the tip. A pull from inside one repository is still checked as its head merged into main. The run stays keyed to the head (`commit_oid`), so head lookups and the merge gate find it; the combined commit it tested is recorded on the run (`combined_oid`), and the checks line reads "as combined" from that, never from the tree, so a check that ran before `combined/N` existed says "against". | |
| 27 | A push to main alone recombines every open kept-fork pull in the background (`core/recombine.ts`, started by the post-receive hook after it answers). When a rebuild fails the card stays stale, naming the main commit it was built on, and says what to do in one fixed sentence (the detail of an unexpected error goes to the log, never the page); any successful `combine()` (a push to main or to the fork, or the landing) clears the line. Main is merged into the head with its side winning every block, so a conflict never fails a rebuild; only a missing head or a git failure does, and the sentence then says to push to the fork to rebuild. The pull's fast check runs on the combined commit, not the author's head alone (`checks/service.ts`): its log and the checks line say "checked as combined with main at abc1234", the run records that base, and the green is stale only when `combined/N` is built on an older main than the tip. A pull from inside one repository is still checked as its head merged into main. The run stays keyed to the head (`commit_oid`), so head lookups and the merge gate find it; the combined commit it tested is recorded on the run (`combined_oid`), and the checks line reads "as combined" from that, never from the tree, so a check that ran before `combined/N` existed says "against". | |
| 28 | 28 | |
| 29 | 29 | Test: `test/combined-check.test.ts` (the check's source, its words, stale, an in-repository pull unchanged), `test/combined-fork.test.ts` (open, page, terminal, a push recombining, stale, landing) and `test/combined-git.test.ts` (a writer's commit surviving, modify/delete left open, the head a preview runs under). |
| 30 | 30 | |
| @@ -38,4 +38,4 @@ Test: `test/source-merge-held.test.ts` (two forges, an `-s ours` merge held, a m | ||
| 38 | 38 | |
| 39 | 39 | ## Not yet |
| 40 | 40 | |
| 41 | Landing a very large fork in reviewed slices that still end as a merge of the author's head; routing a pull opened on the Forgejo mirror to trunk; a crew run sent to refine `combined/N` beyond main's side; recombining when main alone moves and running his code under the guest rule until a person approves (#8903 is not in this repository). Resolving conflicts on our side is today's resolve page (`docs/PULLS.md`). | |
| 41 | Landing a very large fork in reviewed slices that still end as a merge of the author's head; routing a pull opened on the Forgejo mirror to trunk; a crew run sent to refine `combined/N` beyond main's side; running his code under the guest rule until a person approves (#8903 is not in this repository). Resolving conflicts on our side is today's resolve page (`docs/PULLS.md`). |
src/core/combined_view.ts +7 −2
| @@ -6,7 +6,7 @@ | ||
| 6 | 6 | */ |
| 7 | 7 | |
| 8 | 8 | import type { Pull } from "./pulls.ts"; |
| 9 | import { keptLine, readCombined, TAKE_BACK, type Kept } from "../git/combined.ts"; | |
| 9 | import { keptLine, readCombined, readFailure, TAKE_BACK, type Kept } from "../git/combined.ts"; | |
| 10 | 10 | |
| 11 | 11 | export interface CombinedView { |
| 12 | 12 | readonly branch: string; |
| @@ -16,6 +16,8 @@ export interface CombinedView { | ||
| 16 | 16 | /** Main has moved past `builtOn`: the preview is not what lands until it recombines. */ |
| 17 | 17 | readonly stale: boolean; |
| 18 | 18 | readonly files: readonly Kept[]; |
| 19 | /** Why the last rebuild after a push to main failed; "" when it did not, or a later one succeeded. */ | |
| 20 | readonly failure: string; | |
| 19 | 21 | } |
| 20 | 22 | |
| 21 | 23 | /** The combined branch of an open fork's pull, when it differs from the head. */ |
| @@ -29,11 +31,13 @@ export async function combinedView( | ||
| 29 | 31 | const read = await readCombined(dir, pull.number, headOid); |
| 30 | 32 | // Main already in the head: the preview is the head itself, nothing to say. |
| 31 | 33 | if (read === undefined || read.oid === headOid) return undefined; |
| 34 | const stale = read.baseOid !== "" && read.baseOid !== baseOid; | |
| 32 | 35 | return { |
| 36 | failure: stale ? ((await readFailure(dir, pull.number)) ?? "") : "", | |
| 33 | 37 | branch: read.branch, |
| 34 | 38 | oid: read.oid, |
| 35 | 39 | builtOn: read.baseOid, |
| 36 | stale: read.baseOid !== "" && read.baseOid !== baseOid, | |
| 40 | stale, | |
| 37 | 41 | files: read.files, |
| 38 | 42 | }; |
| 39 | 43 | } |
| @@ -48,6 +52,7 @@ export function combinedWords( | ||
| 48 | 52 | notes.push( |
| 49 | 53 | `Stale: main has moved since ${view.builtOn.slice(0, 10)}, which this was built on. It recombines on the next push to the fork, and merging recombines before it lands, so what lands may differ from the preview.`, |
| 50 | 54 | ); |
| 55 | if (view.failure !== "") notes.push(`${view.failure} The preview stays on the older main until a rebuild succeeds.`); | |
| 51 | 56 | notes.push("The checks line says which commit the check ran on."); |
| 52 | 57 | if (view.files.some((f) => f.kept === "main")) notes.push(TAKE_BACK); |
| 53 | 58 | return { |
src/core/recombine.ts +16 −7
| @@ -7,7 +7,7 @@ | ||
| 7 | 7 | */ |
| 8 | 8 | |
| 9 | 9 | import type { Database } from "bun:sqlite"; |
| 10 | import { combine } from "../git/combined.ts"; | |
| 10 | import { combine, setFailure } from "../git/combined.ts"; | |
| 11 | 11 | import { repoDir } from "../git/repos.ts"; |
| 12 | 12 | import { resolveOid } from "../git/merge.ts"; |
| 13 | 13 | import type { Log } from "../kernel/log.ts"; |
| @@ -35,6 +35,9 @@ export async function idle(repoId: string): Promise<void> { | ||
| 35 | 35 | await running.get(repoId)?.done; |
| 36 | 36 | } |
| 37 | 37 | |
| 38 | /** What the page says of an error nobody expected: the detail stays in the log. */ | |
| 39 | const UNEXPECTED = "The rebuild failed unexpectedly; the forge's log has the detail. Push to the fork to rebuild."; | |
| 40 | ||
| 38 | 41 | /** One pass: the tip is read once, each pull combined against it. */ |
| 39 | 42 | async function pass(forge: RecombineForge, repoId: string): Promise<void> { |
| 40 | 43 | const repo = getRepoById(forge.db, repoId); |
| @@ -50,14 +53,20 @@ async function pass(forge: RecombineForge, repoId: string): Promise<void> { | ||
| 50 | 53 | const headOid = await resolveOid(dir, `refs/pull/${pull.number}/head`); |
| 51 | 54 | const made = |
| 52 | 55 | headOid === undefined ? undefined : await combine(dir, { number: pull.number, baseOid, headOid }); |
| 53 | if (made === undefined) | |
| 56 | if (made === undefined) { | |
| 54 | 57 | forge.log.warn("recombine skipped a pull", { repo: repoId, pull: pull.number }); |
| 58 | await setFailure( | |
| 59 | dir, | |
| 60 | pull.number, | |
| 61 | headOid === undefined | |
| 62 | ? "The rebuild failed: the pull's head is missing from the repository. Push to the fork again to rebuild." | |
| 63 | : "The rebuild failed: git could not merge main into the pull's head. Push to the fork to rebuild it.", | |
| 64 | ); | |
| 65 | } | |
| 55 | 66 | } catch (e) { |
outdated
Do not put the raw error message on a page; use a fixed sentence and keep the detail in the log. |
||
| 56 | forge.log.warn("recombine failed on a pull", { | |
| 57 | repo: repoId, | |
| 58 | pull: pull.number, | |
| 59 | error: e instanceof Error ? e.message : String(e), | |
| 60 | }); | |
| 67 | const error = e instanceof Error ? e.message : String(e); | |
| 68 | forge.log.warn("recombine failed on a pull", { repo: repoId, pull: pull.number, error }); | |
| 69 | await setFailure(dir, pull.number, UNEXPECTED).catch(() => undefined); | |
| 61 | 70 | } |
| 62 | 71 | } |
| 63 | 72 | } |
src/git/combined.ts +30
| @@ -119,6 +119,16 @@ async function mergeTree( | ||
| 119 | 119 | export async function combine( |
| 120 | 120 | dir: string, |
| 121 | 121 | input: { readonly number: number; readonly baseOid: string; readonly headOid: string }, |
| 122 | ): Promise<{ oid: string; files: Kept[] | null } | undefined> { | |
| 123 | const made = await build(dir, input); | |
| 124 | // A rebuild that worked ends any earlier failure's sentence, whoever asked for it. | |
| 125 | if (made !== undefined) await setFailure(dir, input.number, undefined); | |
| 126 | return made; | |
| 127 | } | |
| 128 | ||
| 129 | async function build( | |
| 130 | dir: string, | |
| 131 | input: { readonly number: number; readonly baseOid: string; readonly headOid: string }, | |
| 122 | 132 | ): Promise<{ oid: string; files: Kept[] | null } | undefined> { |
| 123 | 133 | const { number, baseOid, headOid } = input; |
| 124 | 134 | if (!Number.isInteger(number) || number < 1 || !isSafeOid(baseOid) || !isSafeOid(headOid)) |
| @@ -227,3 +237,23 @@ export async function previewHead( | ||
| 227 | 237 | const combined = pull.crossRepo ? await combinedTip(dir, pull.number, head) : undefined; |
| 228 | 238 | return { commit: combined ?? head, head }; |
| 229 | 239 | } |
| 240 | ||
| 241 | const failureKey = (number: number): string => `recombine.failed${number}`; | |
| 242 | ||
| 243 | /** Remember (or, with undefined, forget) why the last rebuild of this pull failed: one line, in the repository's own config. */ | |
| 244 | export async function setFailure(dir: string, number: number, sentence: string | undefined): Promise<void> { | |
| 245 | if (!Number.isInteger(number) || number < 1) return; | |
| 246 | if (sentence === undefined) await runGit(["config", "--local", "--unset-all", failureKey(number)], { cwd: dir }); | |
| 247 | else { | |
| 248 | const one = sentence.replace(/\s+/g, " ").trim().slice(0, 300); | |
| 249 | await runGit(["config", "--local", failureKey(number), one], { cwd: dir }); | |
| 250 | } | |
| 251 | } | |
| 252 | ||
| 253 | /** Why the last rebuild of this pull failed, when it did and nothing has rebuilt since. */ | |
| 254 | export async function readFailure(dir: string, number: number): Promise<string | undefined> { | |
| 255 | if (!Number.isInteger(number) || number < 1) return undefined; | |
| 256 | const out = await runGit(["config", "--local", "--get", failureKey(number)], { cwd: dir }); | |
| 257 | const line = out.ok ? out.value.trim() : ""; | |
| 258 | return line === "" ? undefined : line; | |
| 259 | } |
test/combined-recombine.test.ts +30
| @@ -8,6 +8,7 @@ import { serve, type Servers } from "../src/http/server.ts"; | ||
| 8 | 8 | import { gitClientEnv } from "./helpers/git-client.ts"; |
| 9 | 9 | import { proveAll } from "./helpers/verified.ts"; |
| 10 | 10 | import { promote } from "./helpers/members.ts"; |
| 11 | import { combine, readFailure } from "../src/git/combined.ts"; | |
| 11 | 12 | import { idle, recombine, runsStarted } from "../src/core/recombine.ts"; |
| 12 | 13 | import { getRepo } from "../src/core/repos.ts"; |
| 13 | 14 | import { repoDir } from "../src/git/repos.ts"; |
| @@ -179,3 +180,32 @@ test("a push to another branch, or a delete of the default branch, starts no pas | ||
| 179 | 180 | await idle(repoId); |
| 180 | 181 | expect(runsStarted(repoId)).toBe(runs + 1); |
| 181 | 182 | }); |
| 183 | ||
| 184 | test("a failed rebuild is worded on the pull's combined card, and any good rebuild clears it", async () => { | |
| 185 | const card = async (): Promise<string> => | |
| 186 | (await (await call(`/alice/app/pulls/${first}`, alice)).text()).replace(/'/g, "'"); | |
| 187 | const sentence = "The rebuild failed: the pull's head is missing from the repository."; | |
outdated
Relies on an earlier test having deleted the head ref; set that up here. |
||
| 188 | const head = await git(["rev-parse", "refs/heads/one"], b); | |
| 189 | // This test makes its own state: a combined branch built on one main, the head ref then gone. | |
| 190 | await git(["update-ref", `refs/pull/${first}/head`, head], bare); | |
| 191 | await settled(await moveMain("f1.txt")); | |
| 192 | await git(["update-ref", "-d", `refs/pull/${first}/head`], bare); | |
| 193 | await moveMain("m5.txt"); | |
| 194 | await idle(repoId); | |
| 195 | const failed = await card(); | |
| 196 | expect(failed).toContain("Combined with main"); | |
| 197 | expect(failed).toContain("stale"); | |
| 198 | expect(failed).toContain(sentence); | |
| 199 | expect(failed).toContain("Push to the fork again to rebuild."); | |
| 200 | // A push to the fork rebuilds through combine() alone, not the recombine pass. | |
| 201 | await git(["update-ref", `refs/pull/${first}/head`, head], bare); | |
| 202 | const main = await git(["rev-parse", "refs/heads/main"], bare); | |
| 203 | expect(await combine(bare, { number: first, baseOid: main, headOid: head })).toBeDefined(); | |
| 204 | expect(await readFailure(bare, first)).toBeUndefined(); | |
| 205 | // Main moves again: the old sentence does not come back. | |
| 206 | await moveMain("m6.txt"); | |
| 207 | await idle(repoId); | |
| 208 | const fixed = await card(); | |
| 209 | expect(fixed).not.toContain(sentence); | |
| 210 | expect(fixed).not.toContain("stale"); | |
| 211 | }); |
Clearing belongs in combine() so that fork pushes and landing clear it too; otherwise a stale old sentence reappears on the next main push.