Public: anyone can read this; the crew can change it.
Everything a merge sets off runs with the merger's rights or as the forge: a test merges as a plain writer and finds no write made as another account #5842
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/5837-everything-a-merge-sets-off-runs-with-th → main 2 commits fork 3 threads · 3 unresolved
3 files
changed +244
−47
src/crew/converge.ts +17 −8
| @@ -33,7 +33,7 @@ import { | ||
| 33 | 33 | type Issue, |
| 34 | 34 | } from "../core/issues.ts"; |
| 35 | 35 | import { closesFromBody, listPulls, type Pull } from "../core/pulls.ts"; |
| 36 | import { getRepoById, listAllForks, type Repo } from "../core/repos.ts"; | |
| 36 | import { canWrite, getRepoById, listAllForks, type Repo } from "../core/repos.ts"; | |
| 37 | 37 | import { activeRunFor, cancelRun, listRuns } from "../core/runs.ts"; |
| 38 | 38 | import { commitIdentity } from "../core/commit_identity.ts"; |
| 39 | 39 | import { getUserById, type User } from "../core/users.ts"; |
| @@ -276,6 +276,8 @@ export async function settleConverge( | ||
| 276 | 276 | app: App, |
| 277 | 277 | fork: Repo, |
| 278 | 278 | parent: Repo, |
| 279 | /** Whoever moved the fork's branch (a merge, a push): the only name the close may be said under. */ | |
| 280 | by?: User, | |
| 279 | 281 | ): Promise<boolean> { |
| 280 | 282 | const issue = convergeIssue(app, fork, parent); |
| 281 | 283 | if (issue === undefined) return false; |
| @@ -294,20 +296,23 @@ export async function settleConverge( | ||
| 294 | 296 | cancelRun(app.db, run.id, "parent's tip already in the fork"); |
| 295 | 297 | const closed = setIssueState(app.db, issue.id, "closed", "done"); |
| 296 | 298 | if (closed === undefined) return false; |
| 297 | const said = insertComment(app.db, { | |
| 298 | issueId: issue.id, | |
| 299 | authorId: issue.authorId, | |
| 300 | body, | |
| 301 | }); | |
| 299 | // Said as whoever moved the branch, and only if they may write here; with | |
| 300 | // none (the tick) nothing is said as the issue's author, who did not act. | |
| 301 | const said = | |
| 302 | by !== undefined && canWrite(app.db, fork, by) | |
| 303 | ? insertComment(app.db, { issueId: issue.id, authorId: by.id, body }) | |
| 304 | : undefined; | |
| 302 | 305 | app.bus.append({ |
| 303 | 306 | kind: "issue.closed", |
| 304 | 307 | repoId: fork.id, |
| 308 | ...(by === undefined ? {} : { actorId: by.id }), | |
| 305 | 309 | payload: { number: issue.number, reason: "done" }, |
| 306 | 310 | }); |
| 307 | if (said.ok) | |
| 311 | if (said?.ok) | |
| 308 | 312 | app.bus.append({ |
| 309 | 313 | kind: "issue.commented", |
| 310 | 314 | repoId: fork.id, |
| 315 | actorId: said.value.authorId, | |
| 311 | 316 | payload: { number: issue.number, body, commentId: said.value.id }, |
| 312 | 317 | }); |
| 313 | 318 | log.info("converge already done", { |
| @@ -348,7 +353,11 @@ export function settleOnPush(app: App): () => void { | ||
| 348 | 353 | if (fork === undefined || fork.forkParentId === null) return; |
This actor lookup from event.actorId is the part no test reaches. Add a test that merges on a fork and checks the close names the merger. |
||
| 349 | 354 | const parent = getRepoById(app.db, fork.forkParentId); |
| 350 | 355 | if (parent === undefined) return; |
| 351 | settleConverge(app, fork, parent).catch((error) => | |
| 356 | const by = | |
| 357 | event.actorId === null || event.actorId === undefined | |
| 358 | ? undefined | |
| 359 | : getUserById(app.db, event.actorId); | |
| 360 | settleConverge(app, fork, parent, by).catch((error) => | |
| 352 | 361 | log.error("could not settle a converge issue", { |
| 353 | 362 | fork: `${fork.ownerName}/${fork.name}`, |
| 354 | 363 | error: String(error), |
test/converge-settled.test.ts +92 −39
| @@ -15,8 +15,10 @@ import { | ||
| 15 | 15 | convergeFork, |
| 16 | 16 | convergeIssue, |
| 17 | 17 | settleAll, |
| 18 | settleConverge, | |
| 18 | 19 | } from "../src/crew/converge.ts"; |
| 19 | 20 | import { getIssueById, listComments } from "../src/core/issues.ts"; |
| 21 | import { listEvents } from "../src/core/events.ts"; | |
| 20 | 22 | import { forkRelation } from "../src/core/following.ts"; |
| 21 | 23 | import { getRepoById, type Repo } from "../src/core/repos.ts"; |
| 22 | 24 | import { listRuns } from "../src/core/runs.ts"; |
| @@ -30,9 +32,8 @@ describe("a converge issue whose parent tip the fork already holds", () => { | ||
| 30 | 32 | let app: App; |
| 31 | 33 | let alice: User; |
| 32 | 34 | let bob: User; |
| 33 | let parent: Repo; | |
| 34 | let fork: Repo; | |
| 35 | let issueId: string; | |
| 35 | let carol: User; | |
| 36 | let n = 0; | |
| 36 | 37 | |
| 37 | 38 | const fresh = (r: Repo): Repo => getRepoById(app.db, r.id)!; |
| 38 | 39 | const commit = (who: User, repo: Repo, path: string, content: string) => |
| @@ -59,26 +60,20 @@ describe("a converge issue whose parent tip the fork already holds", () => { | ||
| 59 | 60 | email: "[email protected]", |
| 60 | 61 | password: "a-good-passphrase", |
| 61 | 62 | }); |
| 62 | if (!a.ok || !b.ok) throw new Error("users"); | |
| 63 | const c = await createUser(app.db, { | |
| 64 | origin: "seed", | |
| 65 | trusted: true, | |
| 66 | username: "carol", | |
| 67 | email: "[email protected]", | |
| 68 | password: "a-good-passphrase", | |
| 69 | }); | |
| 70 | if (!a.ok || !b.ok || !c.ok) throw new Error("users"); | |
| 63 | 71 | alice = a.value; |
| 64 | 72 | bob = b.value; |
| 73 | carol = c.value; | |
| 65 | 74 | promote(app.db, bob); |
| 66 | const made = await createRepo(app, alice, { name: "game", visibility: "public" }); | |
| 67 | if (!made.ok) throw new Error(String(made.error)); | |
| 68 | parent = made.value; | |
| 69 | await commit(alice, parent, "README.md", "# game\n"); | |
| 70 | const forked = await forkRepo(app, bob, parent); | |
| 71 | if (!forked.ok) throw new Error(String(forked.error)); | |
| 72 | fork = forked.value; | |
| 73 | 75 | const bot = await createAgent(app, alice, { username: "bot", brief: "Be brief." }); |
| 74 | 76 | if (!bot.ok) throw new Error(String(bot.error)); |
| 75 | expect(addToCrew(app, bob, fresh(fork), "bot").ok).toBe(true); | |
| 76 | // Both sides change the same file: a decision, so the crew gets an issue. | |
| 77 | await commit(bob, fork, "README.md", "# game — by bob\n"); | |
| 78 | await commit(alice, parent, "README.md", "# game — by alice\n"); | |
| 79 | const out = await convergeFork(app, fresh(fork), fresh(parent), bob); | |
| 80 | expect(out.kind).toBe("queued"); | |
| 81 | issueId = out.kind === "queued" ? out.issue.id : ""; | |
| 82 | 77 | }, 30_000); |
| 83 | 78 | |
| 84 | 79 | afterAll(async () => { |
| @@ -86,20 +81,36 @@ describe("a converge issue whose parent tip the fork already holds", () => { | ||
| 86 | 81 | await rm(dataDir, { recursive: true, force: true }); |
| 87 | 82 | }); |
| 88 | 83 | |
| 89 | const runsFor = () => | |
| 90 | listRuns(app.db, fork.id, { kind: "agent", limit: 20 }).filter( | |
| 91 | (r) => r.issueId === issueId, | |
| 92 | ); | |
| 84 | /** A parent and bob's fork that both changed README.md, with the crew's converge issue open and a run queued. */ | |
| 85 | async function diverged() { | |
| 86 | const name = `game${++n}`; | |
| 87 | const made = await createRepo(app, alice, { name, visibility: "public" }); | |
| 88 | if (!made.ok) throw new Error(String(made.error)); | |
| 89 | const parent = made.value; | |
| 90 | await commit(alice, parent, "README.md", "# game\n"); | |
| 91 | const forked = await forkRepo(app, bob, parent); | |
| 92 | if (!forked.ok) throw new Error(String(forked.error)); | |
| 93 | const fork = forked.value; | |
| 94 | expect(addToCrew(app, bob, fresh(fork), "bot").ok).toBe(true); | |
| 95 | // Both sides change the same file: a decision, so the crew gets an issue. | |
| 96 | await commit(bob, fork, "README.md", "# game — by bob\n"); | |
| 97 | await commit(alice, parent, "README.md", "# game — by alice\n"); | |
| 98 | const out = await convergeFork(app, fresh(fork), fresh(parent), bob); | |
| 99 | expect(out.kind).toBe("queued"); | |
| 100 | const issueId = out.kind === "queued" ? out.issue.id : ""; | |
| 101 | // The issue was opened by alice, so a comment as bob is not the author's. | |
| 102 | app.db.query("UPDATE issue SET author_id = ?1 WHERE id = ?2").run(alice.id, issueId); | |
| 103 | const runs = () => | |
| 104 | listRuns(app.db, fork.id, { kind: "agent", limit: 20 }).filter( | |
| 105 | (r) => r.issueId === issueId, | |
| 106 | ); | |
| 107 | return { parent, fork, issueId, runs, name }; | |
| 108 | } | |
| 93 | 109 | |
| 94 | test("a fork still diverged keeps its issue open and its run queued", async () => { | |
| 95 | expect(await settleAll(app)).toBe(0); | |
| 96 | expect(getIssueById(app.db, issueId)?.state).toBe("open"); | |
| 97 | expect(runsFor().map((r) => r.state)).toEqual(["queued"]); | |
| 98 | }); | |
| 99 | ||
| 100 | test("merging the parent by hand closes the issue, names both commits, starts no run", async () => { | |
| 101 | const parentDir = repoDir(app.reposRoot, "alice", "game"); | |
| 102 | const forkDir = repoDir(app.reposRoot, "bob", "game"); | |
| 110 | /** Merge the parent's tip into the fork's default branch by hand; returns both commits. */ | |
| 111 | async function mergeByHand(name: string) { | |
| 112 | const parentDir = repoDir(app.reposRoot, "alice", name); | |
| 113 | const forkDir = repoDir(app.reposRoot, "bob", name); | |
| 103 | 114 | const parentTip = (await resolveOid(parentDir, "refs/heads/main"))!; |
| 104 | 115 | const forkTip = (await resolveOid(forkDir, "refs/heads/main"))!; |
| 105 | 116 | const env = { |
| @@ -119,23 +130,65 @@ describe("a converge issue whose parent tip the fork already holds", () => { | ||
| 119 | 130 | "commit-tree", `${forkTip}^{tree}`, "-p", forkTip, "-p", parentTip, "-m", "merge alice by hand", |
| 120 | 131 | ); |
| 121 | 132 | await git("update-ref", "refs/heads/main", merge); |
| 133 | return { parentTip, merge }; | |
| 134 | } | |
| 135 | ||
| 136 | const closedBy = (forkId: string) => | |
| 137 | listEvents(app.db, { repoId: forkId, kind: "issue.closed" })[0]; | |
| 138 | ||
| 139 | test("a fork still diverged keeps its issue open and its run queued", async () => { | |
| 140 | const { issueId, runs } = await diverged(); | |
| 141 | expect(await settleAll(app)).toBe(0); | |
| 142 | expect(getIssueById(app.db, issueId)?.state).toBe("open"); | |
| 143 | expect(runs().map((r) => r.state)).toEqual(["queued"]); | |
| 144 | }); | |
| 145 | ||
| 146 | test("a writer moving the branch closes the issue as the writer, naming both commits, starting no run", async () => { | |
| 147 | const { parent, fork, issueId, runs, name } = await diverged(); | |
| 148 | const { parentTip, merge } = await mergeByHand(name); | |
| 122 | 149 | expect((await forkRelation(app, fresh(fork), fresh(parent)))?.state).toBe("ahead"); |
| 123 | 150 | |
| 124 | expect(await settleAll(app)).toBe(1); | |
| 125 | const issue = getIssueById(app.db, issueId)!; | |
| 126 | expect(issue.state).toBe("closed"); | |
| 151 | expect(await settleConverge(app, fresh(fork), fresh(parent), bob)).toBe(true); | |
| 152 | expect(getIssueById(app.db, issueId)?.state).toBe("closed"); | |
| 127 | 153 | expect(convergeIssue(app, fresh(fork), fresh(parent))).toBeUndefined(); |
| 128 | const said = listComments(app.db, issueId).map((c) => c.body).join("\n"); | |
| 129 | expect(said).toContain(parentTip.slice(0, 7)); | |
| 130 | expect(said).toContain(merge.slice(0, 7)); | |
| 131 | expect(runsFor().map((r) => r.state)).toEqual(["cancelled"]); | |
| 154 | const comments = listComments(app.db, issueId); | |
| 155 | expect(comments.map((c) => c.authorId)).toEqual([bob.id]); | |
| 156 | expect(comments[0]!.body).toContain(parentTip.slice(0, 7)); | |
| 157 | expect(comments[0]!.body).toContain(merge.slice(0, 7)); | |
| 158 | expect(closedBy(fork.id)?.actorId).toBe(bob.id); | |
| 159 | const said = listEvents(app.db, { repoId: fork.id, kind: "issue.commented" })[0]; | |
| 160 | expect(said?.actorId).toBe(bob.id); | |
| 161 | expect(runs().map((r) => r.state)).toEqual(["cancelled"]); | |
| 132 | 162 | // Nothing is left to do twice. |
| 133 | 163 | expect(await settleAll(app)).toBe(0); |
| 134 | 164 | }); |
| 135 | 165 | |
| 166 | test("someone who cannot write the fork moving the branch closes the issue and says nothing", async () => { | |
| 167 | const { parent, fork, issueId } = await diverged(); | |
| 168 | await mergeByHand(`game${n}`); | |
| 169 | expect(await settleConverge(app, fresh(fork), fresh(parent), carol)).toBe(true); | |
| 170 | expect(getIssueById(app.db, issueId)?.state).toBe("closed"); | |
| 171 | expect(listComments(app.db, issueId)).toEqual([]); | |
| 172 | expect(closedBy(fork.id)?.actorId).toBe(carol.id); | |
| 173 | }); | |
| 174 | ||
| 175 | test("the tick, with nobody who moved the branch, closes the issue and writes no comment", async () => { | |
| 176 | const { fork, issueId, runs } = await diverged(); | |
| 177 | await mergeByHand(`game${n}`); | |
| 178 | expect(await settleAll(app)).toBe(1); | |
| 179 | expect(getIssueById(app.db, issueId)?.state).toBe("closed"); | |
| 180 | expect(listComments(app.db, issueId)).toEqual([]); | |
| 181 | expect(closedBy(fork.id)?.actorId).toBeNull(); | |
| 182 | expect(runs().map((r) => r.state)).toEqual(["cancelled"]); | |
| 183 | }); | |
| 184 | ||
| 136 | 185 | test("asking the crew to converge now opens nothing and queues nothing", async () => { |
| 186 | const { parent, fork, issueId, runs } = await diverged(); | |
| 187 | await mergeByHand(`game${n}`); | |
| 188 | expect(await settleAll(app)).toBe(1); | |
| 137 | 189 | const out = await convergeFork(app, fresh(fork), fresh(parent), bob); |
| 138 | 190 | expect(out.kind).toBe("nothing"); |
| 139 | expect(runsFor().length).toBe(1); | |
| 191 | expect(runs().length).toBe(1); | |
| 192 | expect(getIssueById(app.db, issueId)?.state).toBe("closed"); | |
| 140 | 193 | }); |
| 141 | 194 | }); |
test/merge-actors.test.ts added +135
| @@ -0,0 +1,135 @@ | ||
| 1 | /** | |
| 2 | * Everything a merge sets off writes as the merger or as nobody (#5837): a | |
| 3 | * writer who is no admin merges a pull of someone else's in an organization's | |
| 4 | * repository whose autonomy is run by a third account, and every row the | |
| 5 | * merge wrote names only the merger — never the pull's author, the owner or | |
| 6 | * `autonomyBy`. | |
| 7 | * | |
| 8 | * What `mergePull` starts, and as whom: | |
| 9 | * - the pull row closes as merged merged_by = the merger | |
| 10 | * - the decision to land resolves no actor column | |
| 11 | * - `pull.merged` on the outbox actor = the merger | |
| 12 | * - each issue the body closes closes state only; `issue.closed` actor = the merger | |
| 13 | * - subscribers of `pull.merged` (checks, previews, looks, crew, follow) | |
| 14 | * read the event's actor and write as it, or write no row; the fork's | |
| 15 | * converge close is its own test (`converge-settled.test.ts`), not run here | |
| 16 | * - no comment, release or setting is written by a merge | |
| 17 | */ | |
| 18 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 19 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 20 | import { tmpdir } from "node:os"; | |
| 21 | import { join } from "node:path"; | |
| 22 | import { createApp, type App } from "../src/app.ts"; | |
| 23 | import { addTeamMember, createOrg, createTeam } from "../src/actions/orgs.ts"; | |
| 24 | import { openIssue } from "../src/actions/issues.ts"; | |
| 25 | import { mergePull, openPull } from "../src/actions/pulls.ts"; | |
| 26 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 27 | import { getIssue } from "../src/core/issues.ts"; | |
| 28 | import { getOrg } from "../src/core/orgs.ts"; | |
| 29 | import { getRepoById, updateRepoSettings, type Repo } from "../src/core/repos.ts"; | |
| 30 | import type { User } from "../src/core/users.ts"; | |
| 31 | import { promote } from "./helpers/members.ts"; | |
| 32 | import { signUpProven as signUp } from "./helpers/verified.ts"; | |
| 33 | ||
| 34 | describe("a writer's merge writes nothing as anyone else", () => { | |
| 35 | let dataDir: string; | |
| 36 | let app: App; | |
| 37 | let owner: User; | |
| 38 | let bob: User; | |
| 39 | let carol: User; | |
| 40 | let dave: User; | |
| 41 | let repo: Repo; | |
| 42 | ||
| 43 | beforeAll(async () => { | |
| 44 | dataDir = await mkdtemp(join(tmpdir(), "forge-merge-actors-")); | |
| 45 | app = createApp({ dataDir, port: 0 }); | |
| 46 | const made = await Promise.all( | |
| 47 | ["alice", "bob", "carol", "dave"].map((username) => | |
| 48 | signUp(app, { | |
| 49 | username, | |
| 50 | email: `${username}@forge.test`, | |
| 51 | password: "a-long-passphrase", | |
| 52 | }), | |
| 53 | ), | |
| 54 | ); | |
| 55 | if (!made.every((m) => m.ok)) throw new Error("sign-up failed"); | |
| 56 | [owner, bob, carol, dave] = made.map((m) => (m.ok ? m.value : undefined!)) as [User, User, User, User]; | |
| 57 | promote(app.db, owner, bob, carol, dave); | |
| 58 | expect(createOrg(app, owner, "acme").ok).toBe(true); | |
| 59 | const acme = getOrg(app.db, "acme")!; | |
| 60 | const devs = createTeam(app, owner, acme, "devs", "write"); | |
| 61 | if (!devs.ok) throw new Error("team"); | |
| 62 | for (const name of ["bob", "carol"]) | |
| 63 | expect(addTeamMember(app, owner, acme, devs.value, name).ok).toBe(true); | |
| 64 | const r = await createRepo(app, owner, { owner: "acme", name: "site", visibility: "public" }); | |
| 65 | if (!r.ok) throw new Error(String(r.error)); | |
| 66 | repo = r.value; | |
| 67 | // The forge acts as dave when it runs on its own: he is never the merger. | |
| 68 | updateRepoSettings(app.db, repo, { autonomy: "autonomous", autonomyBy: dave.id }); | |
| 69 | repo = getRepoById(app.db, repo.id)!; | |
| 70 | }, 30_000); | |
| 71 | ||
| 72 | afterAll(async () => { | |
| 73 | app.close(); | |
| 74 | await rm(dataDir, { recursive: true, force: true }); | |
| 75 | }); | |
| 76 | ||
| 77 | const rows = <T>(sql: string): T[] => app.db.query<T, []>(sql).all(); | |
| 78 | ||
| 79 | test("every row the merge wrote names the merger or the forge's system actor only", async () => { | |
| 80 | expect( | |
| 81 | (await commitToBranch(app, owner, repo, { | |
| 82 | message: "seed", | |
| 83 | files: [{ path: "README.md", content: "hello\n" }], | |
| 84 | })).ok, | |
| 85 | ).toBe(true); | |
| 86 | expect( | |
| 87 | (await commitToBranch(app, carol, repo, { | |
| 88 | branch: "feature", | |
| 89 | message: "feature", | |
| 90 | files: [{ path: "feature.txt", content: "yes\n" }], | |
| 91 | })).ok, | |
| 92 | ).toBe(true); | |
| 93 | const asked = openIssue(app, dave, repo, { title: "Add the feature" }); | |
| 94 | if (!asked.ok) throw new Error("issue"); | |
| 95 | const pull = await openPull(app, carol, repo, { | |
| 96 | title: "Feature", | |
| 97 | body: `Closes #${asked.value.number}`, | |
| 98 | baseBranch: "main", | |
| 99 | headBranch: "feature", | |
| 100 | }); | |
| 101 | if (!pull.ok) throw new Error(String(pull.error)); | |
| 102 | expect(bob.isAdmin).toBe(false); | |
| 103 | ||
| 104 | const settingsBefore = JSON.stringify(getRepoById(app.db, repo.id)); | |
| 105 | const commentsBefore = rows<{ id: string }>("SELECT id FROM comment").length; | |
| 106 | const eventsBefore = rows<{ id: string }>("SELECT id FROM event").map((e) => e.id); | |
| 107 | ||
| 108 | const merged = await mergePull(app, bob, repo, pull.value, "squash"); | |
| 109 | expect(merged.ok).toBe(true); | |
| 110 | // The issue it names is closed: the follow-up ran. | |
| 111 | expect(getIssue(app.db, repo.id, asked.value.number)?.state).toBe("closed"); | |
| 112 | ||
| 113 | expect( | |
| 114 | rows<{ merged_by: string }>("SELECT merged_by FROM pull").map((r) => r.merged_by), | |
| 115 | ).toEqual([bob.id]); | |
| 116 | ||
| 117 | // Events: every one the merge wrote, actors only bob (or nobody). | |
| 118 | const wrote = rows<{ id: string; kind: string; actor_id: string | null }>( | |
| 119 | "SELECT id, kind, actor_id FROM event ORDER BY id", | |
| 120 | ).filter((e) => !eventsBefore.includes(e.id)); | |
| 121 | expect(wrote.map((e) => e.kind)).toContain("pull.merged"); | |
| 122 | expect(wrote.map((e) => e.kind)).toContain("issue.closed"); | |
| 123 | for (const e of wrote) | |
| 124 | expect([bob.id, null], `${e.kind} was written as another account`).toContain(e.actor_id); | |
| 125 | ||
| 126 | // Comments: a merge writes none; and none under another name. | |
| 127 | const comments = rows<{ author_id: string }>("SELECT author_id FROM comment"); | |
| 128 | expect(comments.length).toBe(commentsBefore); | |
| 129 | for (const c of comments) expect(c.author_id).not.toBe(dave.id); | |
| 130 | ||
| 131 | // Settings: untouched by a merge, `autonomyBy` included. | |
| 132 | expect(JSON.stringify(getRepoById(app.db, repo.id))).toBe(settingsBefore); | |
| 133 | expect(getRepoById(app.db, repo.id)?.autonomyBy).toBe(dave.id); | |
| 134 | }); | |
| 135 | }); |
new above
offline
The branch
by !== undefined && canWrite(...)is untested in its true arm.