Public: anyone can read this; the crew can change it.
A test pins which pulls wait on "changes requested": the newest crew verdict on any head, never an origin's imported review #6723
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6714-a-test-pins-which-pulls-wait-on-changes → main 1 commit fork 1 thread · 1 unresolved
1 file
changed +179
−0
test/requested-pulls.test.ts added +179
| @@ -0,0 +1,179 @@ | ||
| 1 | /** | |
| 2 | * Which pulls wait on "changes requested" (#6714): the newest verdict a | |
| 3 | * reviewer on the crew passed, on any head; an origin's imported review is | |
| 4 | * never the crew's word. `requestedPulls` is the one read the tick and /flow | |
| 5 | * share, so these pin it. | |
| 6 | */ | |
| 7 | ||
| 8 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 9 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 10 | import { tmpdir } from "node:os"; | |
| 11 | import { join } from "node:path"; | |
| 12 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 13 | import { openPull } from "../src/actions/pulls.ts"; | |
| 14 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 15 | import { createApp, type App } from "../src/app.ts"; | |
| 16 | import { setCrewRole } from "../src/core/agents.ts"; | |
| 17 | import type { Pull } from "../src/core/pulls.ts"; | |
| 18 | import { getPull } from "../src/core/pulls.ts"; | |
| 19 | import { requestedPulls, verdictIsStale } from "../src/core/requested.ts"; | |
| 20 | import type { Repo } from "../src/core/repos.ts"; | |
| 21 | import { submitReview, type ReviewState } from "../src/core/reviews.ts"; | |
| 22 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; | |
| 23 | import { quietLogs } from "./helpers/globals.ts"; | |
| 24 | ||
| 25 | quietLogs(); | |
| 26 | ||
| 27 | describe("requestedPulls", () => { | |
| 28 | let dataDir = ""; | |
| 29 | let app: App; | |
| 30 | let alice: User; | |
| 31 | let builder: User; | |
| 32 | let reviewer: User; | |
| 33 | let garden: Repo; | |
| 34 | let orchard: Repo; | |
| 35 | let n = 0; | |
| 36 | ||
| 37 | const push = async (repo: Repo, branch: string, base?: string) => { | |
| 38 | const c = await commitToBranch(app, alice, repo, { | |
| 39 | branch, | |
| 40 | ...(base === undefined ? {} : { base }), | |
| 41 | message: `${branch} ${++n}`, | |
| 42 | files: [{ path: `${branch}.md`, content: `${n}\n` }], | |
| 43 | }); | |
| 44 | if (!c.ok) throw new Error(JSON.stringify(c.error)); | |
| 45 | }; | |
| 46 | const pullIn = async (repo: Repo): Promise<Pull> => { | |
| 47 | const branch = `b${++n}`; | |
| 48 | await push(repo, branch, "main"); | |
| 49 | const o = await openPull(app, builder, repo, { | |
| 50 | title: branch, | |
| 51 | body: "", | |
| 52 | baseBranch: "main", | |
| 53 | headBranch: branch, | |
| 54 | }); | |
| 55 | if (!o.ok) throw new Error(String(o.error)); | |
| 56 | return o.value; | |
| 57 | }; | |
| 58 | const say = ( | |
| 59 | pull: Pull, | |
| 60 | state: ReviewState, | |
| 61 | at: string, | |
| 62 | opts: { imported?: boolean; oid?: string } = {}, | |
| 63 | ) => { | |
| 64 | const r = submitReview(app.db, { | |
| 65 | issueId: pull.id, | |
| 66 | authorId: reviewer.id, | |
| 67 | state, | |
| 68 | body: "", | |
| 69 | commitOid: opts.oid ?? pull.headOid, | |
| 70 | createdAt: at, | |
| 71 | ...(opts.imported ? { originRemoteId: `r${++n}`, originAuthor: "octocat" } : {}), | |
| 72 | }); | |
| 73 | if (!r.ok) throw new Error(r.error); | |
| 74 | }; | |
| 75 | const numbers = (repoId?: string) => requestedPulls(app.db, repoId).map((p) => p.pullId); | |
| 76 | const day = (d: number) => `2026-01-0${d}T00:00:00.000Z`; | |
| 77 | ||
| 78 | beforeAll(async () => { | |
| 79 | dataDir = await mkdtemp(join(tmpdir(), "forge-requested-pulls-")); | |
| 80 | app = createApp({ dataDir, port: 0 }); | |
| 81 | const made = await createUser(app.db, { | |
| 82 | origin: "seed", | |
| 83 | trusted: true, | |
| 84 | username: "alice", | |
| 85 | email: "[email protected]", | |
| 86 | password: "a-good-passphrase", | |
| 87 | adminIfFirst: true, | |
| 88 | }); | |
| 89 | if (!made.ok) throw new Error(made.error); | |
| 90 | alice = made.value; | |
| 91 | for (const name of ["mason", "quill"]) { | |
| 92 | const agent = await createAgent(app, alice, { username: name }); | |
| 93 | if (!agent.ok) throw new Error(String(agent.error)); | |
| 94 | } | |
| 95 | builder = getUserByName(app.db, "mason")!; | |
| 96 | reviewer = getUserByName(app.db, "quill")!; | |
| 97 | const repos: Repo[] = []; | |
| 98 | for (const name of ["garden", "orchard"]) { | |
| 99 | const r = await createRepo(app, alice, { name, visibility: "private" }); | |
| 100 | if (!r.ok) throw new Error(String(r.error)); | |
| 101 | await push(r.value, "main"); | |
| 102 | for (const who of ["mason", "quill"]) expect(addToCrew(app, alice, r.value, who).ok).toBe(true); | |
| 103 | setCrewRole(app.db, r.value.id, reviewer.id, "reviewer"); | |
| 104 | repos.push(r.value); | |
| 105 | } | |
| 106 | [garden, orchard] = repos as [Repo, Repo]; | |
| 107 | }, 30_000); | |
| 108 | ||
| 109 | afterAll(async () => { | |
| 110 | app.close(); | |
| 111 | await rm(dataDir, { recursive: true, force: true }); | |
| 112 | }); | |
| 113 | ||
| 114 | test("the newest crew verdict decides: changes requested lists the pull, a later approval takes it off", async () => { | |
| 115 | const pull = await pullIn(garden); | |
| 116 | expect(numbers(garden.id)).not.toContain(pull.id); | |
| 117 | say(pull, "changes_requested", day(1)); | |
| 118 | const [listed] = requestedPulls(app.db, garden.id).filter((p) => p.pullId === pull.id); | |
| 119 | expect(listed).toMatchObject({ number: pull.number, repo: "alice/garden", reviewerId: reviewer.id }); | |
| 120 | expect(verdictIsStale(listed!)).toBe(false); | |
| 121 | say(pull, "approved", day(2)); | |
| 122 | expect(numbers(garden.id)).not.toContain(pull.id); | |
| 123 | }); | |
| 124 | ||
| 125 | test("a verdict on an older head, with a newer push, is listed and stale", async () => { | |
| 126 | const pull = await pullIn(garden); | |
| 127 | say(pull, "changes_requested", day(1)); | |
| 128 | await push(garden, pull.headBranch); | |
| 129 | const pushed = getPull(app.db, garden.id, pull.number)!; | |
| 130 | expect(pushed.headOid).not.toBe(pull.headOid); | |
| 131 | const listed = requestedPulls(app.db, garden.id).find((p) => p.pullId === pull.id)!; | |
| 132 | expect(listed.reviewedOid).toBe(pull.headOid); | |
| 133 | expect(listed.headOid).toBe(pushed.headOid); | |
| 134 | expect(verdictIsStale(listed)).toBe(true); | |
| 135 | }); | |
| 136 | ||
| 137 | test("a review imported from an origin never lists a pull", async () => { | |
| 138 | const only = await pullIn(garden); | |
| 139 | say(only, "changes_requested", day(1), { imported: true }); | |
| 140 | expect(numbers(garden.id)).not.toContain(only.id); | |
| 141 | // Nor does it hide the crew's own word: the crew approved, the import is newer. | |
| 142 | const both = await pullIn(garden); | |
| 143 | say(both, "approved", day(1)); | |
| 144 | say(both, "changes_requested", day(2), { imported: true }); | |
| 145 | expect(numbers(garden.id)).not.toContain(both.id); | |
| 146 | // And an import after the crew's changes-requested leaves the pull listed. | |
| 147 | const kept = await pullIn(garden); | |
| 148 | say(kept, "changes_requested", day(1)); | |
| 149 | say(kept, "approved", day(2), { imported: true }); | |
| 150 | expect(numbers(garden.id)).toContain(kept.id); | |
| 151 | }); | |
| 152 | ||
| 153 | test("closed and merged pulls are never listed", async () => { | |
| 154 | const closed = await pullIn(garden); | |
| 155 | const merged = await pullIn(garden); | |
| 156 | say(closed, "changes_requested", day(1)); | |
| 157 | say(merged, "changes_requested", day(1)); | |
| 158 | expect(numbers(garden.id)).toEqual(expect.arrayContaining([closed.id, merged.id])); | |
| 159 | app.db.query("UPDATE issue SET state = 'closed' WHERE id = ?1").run(closed.id); | |
| 160 | app.db | |
| 161 | .query("UPDATE pull SET merged_at = ?2 WHERE issue_id = ?1") | |
| 162 | .run(merged.id, day(3)); | |
| 163 | expect(numbers(garden.id)).not.toContain(closed.id); | |
| 164 | expect(numbers(garden.id)).not.toContain(merged.id); | |
| 165 | }); | |
| 166 | ||
| 167 | test("oldest verdict first, and repoId narrows to one repository", async () => { | |
| 168 | app.db.query("UPDATE issue SET state = 'closed'").run(); | |
| 169 | const newer = await pullIn(garden); | |
| 170 | const older = await pullIn(orchard); | |
| 171 | const oldest = await pullIn(garden); | |
| 172 | say(newer, "changes_requested", day(5)); | |
| 173 | say(older, "changes_requested", day(4)); | |
| 174 | say(oldest, "changes_requested", day(3)); | |
| 175 | expect(numbers()).toEqual([oldest.id, older.id, newer.id]); | |
| 176 | expect(numbers(garden.id)).toEqual([oldest.id, newer.id]); | |
| 177 | expect(numbers(orchard.id)).toEqual([older.id]); | |
| 178 | }); | |
| 179 | }); |
new above
offline
Not covered: whether the verdict comes from a crew reviewer. Every
sayposts asquill, so removingc.role = 'reviewer'(or therepo_crewjoin) fromrequestedPullsleaves all five tests green. The fix: add anauthorIdoption here, and a case wheremason(crew, not a reviewer) requests changes and the pull is not listed. That would pin the "reviewer on the crew" claim made in this file's header.