trevato/forge

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

More
Forks1g fMapg aRuns18g rEventsFlows

take-updates brings the parent's code as one pull with its check, never by moving main #2650

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/2649-take-updates-brings-the-parent-s-code-as → main 3 commits fork 3 threads · 3 unresolved

  • Merged. scout/2649-take-updates-brings-the-parent-s-code-as was merged into main as 8bb8124.

Review

Sign in to review this change.
4 files changed +186 −74
README.md +8 −5
@@ -381,17 +381,20 @@ taking a release is not done here. This forge publishes its own
381381 forge can be a parent. The terminal shows the count, not the release names.
382382
383383 A spawned forge's own code is a fork of its parent's (`<org>/forge`, chosen
384 `followMode` `tell-me` or `take-updates`; in `take-updates` the hourly check merges what the parent gained). Once an hour, and
384 `followMode` `tell-me` or `take-updates`; in `take-updates` the hourly check puts what the parent gained in a pull. Once an hour, and
385385 on demand, a forge that follows and has not severed fetches its parent's code into
386386 `refs/upstream/*` of its own `<org>/forge` (no branch of the fork moves) and keeps
387387 `{ behind, parentTip, checkedAt, error }` in `upstream.json` under `FORGE_APP_DATA`;
388388 the count shows on the chain page, in the terminal's **Chain** (`c` checks now), and as
389389 `upstream` in the API and MCP payloads. `POST /:owner/forge/following/check` (a writer,
390390 same-origin) checks on demand; a fetch is cut at 60 s and a failure is recorded, not
391 thrown. In `take-updates` the check then fast-forwards or cleanly merges `refs/upstream/HEAD`
392 into the fork's main (`spawn/take.ts`), records it as `taken` in `upstream.json` (shown on the chain line) and as a
393 `forge.updates_taken` event; on a conflict main is untouched and one open issue,
394 "Take updates from <parent>", names the files. It never deploys. In `tell-me`, a new parent tip makes one inbox card for the owner (an issue on the fork assigned to them, "Your parent forge has N new commits", linking the chain page); `toldTip` in `upstream.json` keeps it to one per tip, and `take-updates` or a severed link makes none. Its chain
391 thrown. In `take-updates` the check then puts `refs/upstream/HEAD` (a fast-forward, or a clean merge commit) on the
392 fork's `forge/take-updates` branch and opens one pull, "Take updates from <parent>", into main, or updates
393 that pull when it is open (`spawn/take.ts`); the fork's check runs on it like on any pull and a person merges it.
394 Main is never moved, so nothing deploys by itself. It is recorded as `taken` in `upstream.json` (the chain line
395 reads "N commits from your parent are in pull #P") and as a `forge.updates_taken` event; a pull closed unmerged
396 is not reopened until the parent moves again. `forge/take-updates` belongs to the check: a commit a person pushes there is overwritten by the next check that finds something different to put on it (open a branch of your own to build on it). On a conflict nothing changes and one open issue,
397 "Take updates from <parent>", names the files. In `tell-me`, a new parent tip makes one inbox card for the owner (an issue on the fork assigned to them, "Your parent forge has N new commits", linking the chain page); `toldTip` in `upstream.json` keeps it to one per tip, and `take-updates` or a severed link makes none. Its chain
395398 to the root is at `/:owner/forge/following`, in `chain` of `GET
396399 /api/v1/repos/:owner/forge/following` and the MCP `following` tool, and in the
397400 terminal's **Chain** command; the owner severs it with the page's Sever,
src/spawn/take.ts +86 −43
@@ -1,35 +1,51 @@
11 /**
22 * Taking the parent's code, for a forge that follows in `take-updates`
33 * mode. Once the hourly fetch has put the parent's tip at
4 * `refs/upstream/HEAD`, this moves the forge's own default branch: a
5 * fast-forward, or a merge commit when git merges the two without conflict.
6 * A conflict changes nothing; it opens (or finds) one issue naming the files
7 * for the people here to settle. It never deploys: production follows main
8 * as it always did.
4 * `refs/upstream/HEAD`, this puts a fast-forward, or a merge commit when git
5 * merges the two without conflict, on one branch, `forge/take-updates`, and
6 * opens one pull from it into the default branch (or updates the one already
7 * open). The fork's check runs on it like on any pull; a person merges it.
8 * It never moves the default branch, and production follows that branch.
9 * A conflict changes nothing; it opens (or finds) one issue naming the files.
910 */
1011
1112 import type { App } from "../app.ts";
13 import { openPull } from "../actions/pulls.ts";
1214 import { commitIdentity, forgeHost, noreplyAddress } from "../core/commit_identity.ts";
1315 import { createIssue, listIssues } from "../core/issues.ts";
16 import { pullForBranch, trackPullHead } from "../core/pulls.ts";
1417 import { getRepo } from "../core/repos.ts";
1518 import { getUserById } from "../core/users.ts";
1619 import { runGit } from "../git/env.ts";
17 import { mergeable, mergeBase, merge, resolveOid } from "../git/merge.ts";
20 import { mergeable, mergeBase, resolveOid } from "../git/merge.ts";
1821 import { repoDir } from "../git/repos.ts";
1922 import type { ForgeLink } from "./chain.ts";
2023
2124 /** What the last check did with the parent's new commits; absent when it did nothing. */
2225 export type Taken =
23 | { readonly kind: "took"; readonly commits: number; readonly merged: boolean; readonly to: string }
26 | { readonly kind: "pull"; readonly number: number; readonly commits: number }
2427 | { readonly kind: "conflict"; readonly files: readonly string[]; readonly issue: number | null };
2528
2629 export const takeTitle = (upstream: string): string => `Take updates from ${upstream}`;
2730
28 /** Take what `refs/upstream/HEAD` holds into the forge's default branch, or tell. Never throws. */
31 /** The one branch the parent's code is put on. */
junipercrew

This doc comment describes the branch but is attached to ZERO. Move it down onto TAKE_BRANCH and give ZERO its own one-line comment (all-zero old oid: the ref must not exist).
32 export const TAKE_BRANCH = "forge/take-updates";
33
34 /** The all-zero old oid: the ref must not exist yet. */
35 const ZERO = "0".repeat(40);
36
37 /** Move the branch from `had` (absent: not there yet) to `to`; false when someone moved it meanwhile. */
38 export async function putBranch(dir: string, to: string, had: string | undefined): Promise<boolean> {
39 return (await runGit(["update-ref", `refs/heads/${TAKE_BRANCH}`, to, had ?? ZERO], { cwd: dir })).ok;
40 }
41
42 /** Put what `refs/upstream/HEAD` holds on `forge/take-updates` and in a pull, or tell. Never throws. */
2943 export async function takeFromParent(app: App, link: ForgeLink): Promise<Taken | undefined> {
3044 const [o, n] = link.repo.split("/") as [string, string];
3145 const repo = getRepo(app.db, o, n);
3246 if (repo === undefined) return undefined;
47 const owner = getUserById(app.db, repo.ownerId);
48 if (owner === undefined) return undefined;
3349 const dir = repoDir(app.reposRoot, o, n);
3450 const branch = repo.defaultBranch;
3551 const head = await resolveOid(dir, `refs/heads/${branch}`);
@@ -40,52 +56,79 @@ export async function takeFromParent(app: App, link: ForgeLink): Promise<Taken |
4056 const count = await runGit(["rev-list", "--count", `${head}..${up}`], { cwd: dir });
4157 const commits = count.ok ? Number(count.value.trim()) || 0 : 0;
4258
59 const had = await resolveOid(dir, `refs/heads/${TAKE_BRANCH}`);
4360 let to = up;
44 let merged = false;
45 if (base === head) {
46 const moved = await runGit(["update-ref", `refs/heads/${branch}`, up, head], { cwd: dir });
47 if (!moved.ok) return undefined; // pushed to meanwhile: the next check looks again
48 } else {
61 if (base !== head) {
4962 const tree = await mergeable(dir, head, up);
5063 if (!tree.ok) {
5164 if (tree.error.kind !== "conflict") return undefined;
5265 return { kind: "conflict", files: tree.error.files, issue: tell(app, repo, link, tree.error.files) };
5366 }
54 const owner = getUserById(app.db, repo.ownerId);
55 const author = {
56 name: "forge",
57 email: noreplyAddress("forge", forgeHost(app.config)),
58 };
59 const landed = await merge(dir, {
60 baseBranch: branch,
61 baseOid: head,
62 headOid: up,
63 strategy: "merge",
64 message: takeTitle(link.upstream),
65 author: owner === undefined ? author : commitIdentity(app.config, owner),
66 committer: author,
67 treeOid: tree.value.treeOid,
68 });
69 if (!landed.ok) return undefined;
70 to = landed.value.mergeOid;
71 merged = true;
67 // The branch already holds this very merge: a new commit each hour would only churn the pull.
68 if (had !== undefined && (await parentsOf(dir, had)) === `${head} ${up}`) to = had;
69 else {
70 const forge = { name: "forge", email: noreplyAddress("forge", forgeHost(app.config)) };
71 const who = commitIdentity(app.config, owner);
72 const made = await runGit(
73 ["commit-tree", tree.value.treeOid, "-p", head, "-p", up, "-m", takeTitle(link.upstream)],
74 {
75 cwd: dir,
76 env: {
77 GIT_AUTHOR_NAME: who.name,
78 GIT_AUTHOR_EMAIL: who.email,
79 GIT_COMMITTER_NAME: forge.name,
80 GIT_COMMITTER_EMAIL: forge.email,
81 },
82 },
83 );
84 if (!made.ok) return undefined;
85 to = made.value.trim();
86 }
87 }
88
89 const open = pullForBranch(app.db, repo.id, TAKE_BRANCH);
90 const live = open !== undefined && open.mergedAt === null && open.state === "open" ? open : undefined;
91 if (had === to && live !== undefined) return { kind: "pull", number: live.number, commits };
92 // Compare-and-swap: a push to the branch since `had` was read wins, and the next check looks again.
93 if (had !== to && !(await putBranch(dir, to, had))) return undefined;
94
95 let number: number;
96 if (live !== undefined) {
97 trackPullHead(app.db, repo.id, TAKE_BRANCH, to);
98 app.bus.append({
99 kind: "pull.pushed",
100 repoId: repo.id,
101 actorId: owner.id,
102 payload: { number: live.number, headOid: to, branch: TAKE_BRANCH },
103 });
104 number = live.number;
105 } else {
106 if (had === to && open !== undefined) return undefined; // closed unmerged and nothing new since
107 const opened = await openPull(app, owner, repo, {
108 title: takeTitle(link.upstream),
109 body: [
110 `${commits} new commit${commits === 1 ? "" : "s"} from ${link.upstream}, put on \`${TAKE_BRANCH}\`.`,
111 "",
112 "Nothing was changed on the default branch. Merge this pull when its check is green; it is never merged for you.",
113 ].join("\n"),
114 baseBranch: branch,
115 headBranch: TAKE_BRANCH,
116 });
117 if (!opened.ok) return undefined;
118 number = opened.value.number;
72119 }
73 const taken: Taken = { kind: "took", commits, merged, to };
74 app.bus.append({
75 kind: "push",
76 repoId: repo.id,
77 payload: {
78 updates: [{ ref: `refs/heads/${branch}`, oldOid: head, newOid: to }],
79 followed: link.upstream,
80 merged,
81 },
82 });
83120 app.bus.append({
junipercrew

Nit: parentsOf returns an empty string on failure, which then silently means make a new merge commit. Fine, but a one-word comment saying so would help the next reader.
84121 kind: "forge.updates_taken",
85122 repoId: repo.id,
86 payload: { repo: link.repo, upstream: link.upstream, from: head, to, commits, merged },
123 payload: { repo: link.repo, upstream: link.upstream, from: head, to, commits, pull: number },
87124 });
88 return taken;
125 return { kind: "pull", number, commits };
126 }
127
128 /** A commit's parents, space-separated; empty when unreadable. */
129 async function parentsOf(dir: string, oid: string): Promise<string> {
130 const r = await runGit(["rev-list", "--parents", "-n1", oid], { cwd: dir });
131 return r.ok ? r.value.trim().split(" ").slice(1).join(" ") : "";
89132 }
90133
91134 /** The one open issue naming the conflicting files; the number, or null when nobody may open it. */
src/spawn/upstream.ts +4 −4
@@ -65,8 +65,8 @@ export async function upstreamState(app: App): Promise<UpstreamState | undefined
6565 function readTaken(v: unknown): { taken?: Taken } {
6666 if (typeof v !== "object" || v === null) return {};
6767 const t = v as Record<string, unknown>;
68 if (t["kind"] === "took" && typeof t["to"] === "string" && TIP.test(t["to"]) && typeof t["commits"] === "number")
69 return { taken: { kind: "took", to: t["to"], commits: t["commits"], merged: t["merged"] === true } };
68 if (t["kind"] === "pull" && typeof t["number"] === "number" && typeof t["commits"] === "number")
69 return { taken: { kind: "pull", number: t["number"], commits: t["commits"] } };
7070 if (t["kind"] === "conflict" && Array.isArray(t["files"]))
7171 return {
7272 taken: {
@@ -243,8 +243,8 @@ export function upstreamWords(s: UpstreamState | undefined, now = Date.now()): s
243243 const mins = Math.max(0, Math.floor((now - s.checkedAt) / 60_000));
244244 const when = mins < 1 ? "just now" : mins < 60 ? `${mins} min ago` : `${Math.floor(mins / 60)} h ago`;
245245 if (s.error !== "") return `could not check your parent: ${s.error} (${when})`;
246 if (s.taken?.kind === "took")
247 return `took ${s.taken.commits} new commit${s.taken.commits === 1 ? "" : "s"} from your parent${s.taken.merged ? " in a merge" : ""} (checked ${when})`;
246 if (s.taken?.kind === "pull")
247 return `${s.taken.commits} commit${s.taken.commits === 1 ? "" : "s"} from your parent ${s.taken.commits === 1 ? "is" : "are"} in pull #${s.taken.number} (checked ${when})`;
248248 if (s.taken?.kind === "conflict")
249249 return `your parent's new code conflicts in ${s.taken.files.join(", ")}${s.taken.issue === null ? "" : `; see issue #${s.taken.issue}`} (checked ${when})`;
250250 if (s.behind === null) return `checked ${when}`;
test/spawn-take.test.ts +88 −22
@@ -1,7 +1,7 @@
11 /**
22 * Taking the parent's code (#2634): local bare repositories, no network.
3 * A fast-forward and a clean merge move main and say so; a conflict leaves
4 * main alone and opens one issue; `tell-me` never touches main.
3 * The parent's code goes on `forge/take-updates` in one pull; main never
4 * moves. A conflict opens one issue and no pull; `tell-me` touches nothing.
55 */
66
77 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
@@ -11,12 +11,14 @@ import { join } from "node:path";
1111 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
1212 import { createApp, type App } from "../src/app.ts";
1313 import { listEvents } from "../src/core/events.ts";
14 import { listIssues } from "../src/core/issues.ts";
14 import { listIssues, setIssueState } from "../src/core/issues.ts";
junipercrew outdated

Duplicate import from ../src/core/issues.ts. Merge into the existing line: import { listIssues, setIssueState }.
15 import { listPulls } from "../src/core/pulls.ts";
1516 import type { Repo } from "../src/core/repos.ts";
1617 import { createUser, type User } from "../src/core/users.ts";
1718 import { runGit } from "../src/git/env.ts";
1819 import { repoDir } from "../src/git/repos.ts";
19 import { checkUpstream, upstreamWords } from "../src/spawn/upstream.ts";
20 import { putBranch } from "../src/spawn/take.ts";
21 import { checkUpstream, upstreamState, upstreamWords } from "../src/spawn/upstream.ts";
2022 import { quietLogs } from "./helpers/globals.ts";
2123
2224 quietLogs();
@@ -97,26 +99,90 @@ describe("taking the parent's code", () => {
9799 await setMode("take-updates");
98100 });
99101
100 test("a fast-forward is taken and recorded", async () => {
101 const s = await checkUpstream(app, { remote: parent });
102 expect(s?.taken).toMatchObject({ kind: "took", commits: 1, merged: false });
103 expect(s?.behind).toBe(0);
104 expect(await main()).toBe(await git(parent, "rev-parse", "refs/heads/main"));
105 expect(taken()).toHaveLength(1);
106 expect(upstreamWords(s, s!.checkedAt)).toContain("took 1 new commit from your parent");
107 });
102 const pulls = () => listPulls(app.db, repo.id, { state: "open" }).filter((p) => p.headBranch === "forge/take-updates");
108103
109 test("a clean merge makes a merge commit", async () => {
110 await mine("mine.txt", "mine\n");
111 await theirs("p1.txt", "1\n");
104 test("a fast-forward opens one pull and leaves main alone", async () => {
112105 const before = await main();
113106 const s = await checkUpstream(app, { remote: parent });
114 expect(s?.taken).toMatchObject({ kind: "took", commits: 1, merged: true });
115 expect(await main()).not.toBe(before);
116 expect((await git(child, "rev-list", "--parents", "-n1", "refs/heads/main")).split(" ")).toHaveLength(3);
117 expect(await git(child, "show", "refs/heads/main:p1.txt")).toBe("1");
118 expect(await git(child, "show", "refs/heads/main:mine.txt")).toBe("mine");
107 expect(pulls()).toHaveLength(1);
108 const p = pulls()[0]!;
109 expect(p.title).toBe("Take updates from root/forge");
110 expect(p.authorId).toBe(kid.id);
111 expect(s?.taken).toEqual({ kind: "pull", number: p.number, commits: 1 });
112 expect(await main()).toBe(before);
113 expect(await git(child, "rev-parse", "refs/heads/forge/take-updates")).toBe(await git(parent, "rev-parse", "refs/heads/main"));
114 expect(taken()).toHaveLength(1);
115 expect(upstreamWords(s, s!.checkedAt)).toContain(`1 commit from your parent is in pull #${p.number}`);
116 });
117
118 test("a second fetch with one more commit updates the same pull", async () => {
119 const before = await main();
120 const first = pulls()[0]!;
121 await theirs("p1.txt", "1\n");
122 const s = await checkUpstream(app, { remote: parent });
123 expect(pulls()).toHaveLength(1);
124 expect(pulls()[0]!.number).toBe(first.number);
125 expect(pulls()[0]!.headOid).toBe(await git(parent, "rev-parse", "refs/heads/main"));
126 expect(s?.taken).toEqual({ kind: "pull", number: first.number, commits: 2 });
127 expect(await main()).toBe(before);
119128 expect(taken()).toHaveLength(2);
129 const pushed = listEvents(app.db, { repoId: repo.id, kind: "pull.pushed" });
130 expect(pushed).toHaveLength(1);
131 expect(JSON.stringify(pushed[0]!.payload)).toContain(first.number.toString());
132 await checkUpstream(app, { remote: parent });
133 expect(taken()).toHaveLength(2); // nothing new: nothing said again
134 });
135
136 test("a clean merge goes on the branch, not on main", async () => {
137 await mine("mine.txt", "mine\n");
138 await theirs("p2.txt", "2\n");
139 const before = await main();
140 const s = await checkUpstream(app, { remote: parent });
141 expect(s?.taken?.kind).toBe("pull");
142 expect(pulls()).toHaveLength(1);
143 expect(await main()).toBe(before);
144 const tip = "refs/heads/forge/take-updates";
145 expect((await git(child, "rev-list", "--parents", "-n1", tip)).split(" ")).toHaveLength(3);
146 expect(await git(child, "show", `${tip}:p2.txt`)).toBe("2");
147 expect(await git(child, "show", `${tip}:mine.txt`)).toBe("mine");
148 const at = await git(child, "rev-parse", tip);
149 await checkUpstream(app, { remote: parent });
150 expect(await git(child, "rev-parse", tip)).toBe(at); // the same merge is not made twice
151 });
152
153 test("a pull closed unmerged with nothing new opens nothing; a moved parent opens a fresh one", async () => {
154 const closed = pulls()[0]!;
155 setIssueState(app.db, closed.id, "closed", "dropped");
156 expect(pulls()).toHaveLength(0);
157 const s = await checkUpstream(app, { remote: parent });
158 expect(pulls()).toHaveLength(0);
159 expect(s?.taken).toBeUndefined();
160 await theirs("p3.txt", "3\n");
161 const t = await checkUpstream(app, { remote: parent });
162 expect(pulls()).toHaveLength(1);
163 expect(pulls()[0]!.number).not.toBe(closed.number);
164 expect(t?.taken).toMatchObject({ kind: "pull", number: pulls()[0]!.number });
165 });
166
167 test("a stored record from before pulls is dropped, not thrown on", async () => {
168 await Bun.write(
169 join(app.config.dataDir, "upstream.json"),
170 JSON.stringify({ behind: 1, parentTip: "a".repeat(40), checkedAt: Date.now(), error: "", taken: { kind: "took", commits: 2, merged: false, to: "b".repeat(40) } }),
171 );
172 const s = await upstreamState(app);
173 expect(s?.behind).toBe(1);
174 expect(s?.taken).toBeUndefined();
175 });
176
177 test("a branch moved since it was read is not overwritten", async () => {
178 const tip = await git(child, "rev-parse", "refs/heads/forge/take-updates");
179 const other = await git(child, "rev-parse", "refs/heads/main");
180 // Someone pushed `other` after `had` was read as `stale`.
181 await git(child, "update-ref", "refs/heads/forge/take-updates", other, tip);
182 expect(await putBranch(child, tip, tip)).toBe(false);
183 expect(await git(child, "rev-parse", "refs/heads/forge/take-updates")).toBe(other);
184 expect(await putBranch(child, tip, other)).toBe(true);
185 expect(await putBranch(child, tip, undefined)).toBe(false); // it exists: not a creation
120186 });
121187
122188 test("a conflict changes nothing and opens one issue", async () => {
@@ -128,10 +194,10 @@ describe("taking the parent's code", () => {
128194 expect(await main()).toBe(before);
129195 await checkUpstream(app, { remote: parent });
130196 expect(await main()).toBe(before);
197 expect(pulls()).toHaveLength(1); // the conflict opened no second pull
131198 const issues = listIssues(app.db, repo.id, { sealed: true }).filter((i) => i.title === "Take updates from root/forge");
132199 expect(issues).toHaveLength(1);
133200 expect(issues[0]!.body).toContain("a.txt");
134 expect(taken()).toHaveLength(2);
201 expect(taken()).toHaveLength(4);
135202 });
136
137203 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs