Public: anyone can read this; the crew can change it.
An until-green hold is released through the checks service, and a hold set on an already-green head releases at once #6863
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6546-an-until-green-hold-is-released-through → main 1 commit fork 1 thread · 1 unresolved
2 files
changed +78
−1
src/actions/holds.ts +9 −1
| @@ -22,7 +22,7 @@ import { | ||
| 22 | 22 | } from "../core/holds.ts"; |
| 23 | 23 | import { getIssueById } from "../core/issues.ts"; |
| 24 | 24 | import { getPull, type Pull } from "../core/pulls.ts"; |
| 25 | import type { Run } from "../core/runs.ts"; | |
| 25 | import { checksForHead, type Run } from "../core/runs.ts"; | |
| 26 | 26 | import { canRead, canWrite, type Repo } from "../core/repos.ts"; |
| 27 | 27 | import type { User } from "../core/users.ts"; |
| 28 | 28 | import { err, ok, type Result } from "../kernel/result.ts"; |
| @@ -80,6 +80,14 @@ export function holdPull( | ||
| 80 | 80 | actorId: holder.value.id, |
| 81 | 81 | payload: { number: pull.number, why: hold.why }, |
| 82 | 82 | }); |
| 83 | // Held on a head whose latest check is already green: nothing is left to | |
| 84 | // wait for, so it is let go at once, with the same line a later check gives. | |
| 85 | if (until === "green") { | |
| 86 | const latest = checksForHead(app.db, pull.id, pull.headOid).find( | |
| 87 | (r) => r.kind === "check", | |
| 88 | ); | |
| 89 | if (latest) releaseOnGreen(app, latest); | |
| 90 | } | |
| 83 | 91 | return ok(hold); |
| 84 | 92 | } |
| 85 | 93 |
test/holds.test.ts +69
| @@ -33,6 +33,7 @@ import { createToken } from "../src/core/tokens.ts"; | ||
| 33 | 33 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; |
| 34 | 34 | import { landIfGreen, parkPull } from "../src/crew/autonomy.ts"; |
| 35 | 35 | import { sendToReview, sweepReviews } from "../src/crew/review.ts"; |
| 36 | import { startChecks } from "../src/checks/service.ts"; | |
| 36 | 37 | import { serve, type Servers } from "../src/http/server.ts"; |
| 37 | 38 | import { recordAskLedger } from "../src/web/studio/asks.ts"; |
| 38 | 39 | |
| @@ -975,3 +976,71 @@ describe("a hold set until green", () => { | ||
| 975 | 976 | expect(releaseOnGreen(app, verdict(pull, "ok"))).toBe(true); |
| 976 | 977 | }); |
| 977 | 978 | }); |
| 979 | ||
| 980 | describe("an until-green hold, through the checks service", () => { | |
| 981 | let f: Awaited<ReturnType<typeof forge>>; | |
| 982 | let servers: Servers; | |
| 983 | let checks: ReturnType<typeof startChecks>; | |
| 984 | ||
| 985 | beforeAll(async () => { | |
| 986 | f = await forge("forge-holds-service-", "private"); | |
| 987 | servers = await serve(f.app); | |
| 988 | checks = startChecks(f.app, { forgeUrl: servers.url, concurrency: 1 }); | |
| 989 | const declared = await commitToBranch(f.app, f.alice, f.repo, { | |
| 990 | message: "Declare a check", | |
| 991 | files: [ | |
| 992 | { | |
| 993 | path: "forge.json", | |
| 994 | content: '{"check": {"run": "true", "install": ""}}\n', | |
| 995 | }, | |
| 996 | ], | |
| 997 | }); | |
| 998 | if (!declared.ok) throw new Error(JSON.stringify(declared.error)); | |
| 999 | }, 30_000); | |
| 1000 | ||
| 1001 | afterAll(async () => { | |
| 1002 | await checks.stop(); | |
| 1003 | servers.stop(); | |
| 1004 | await f.close(); | |
| 1005 | }); | |
| 1006 | ||
| 1007 | const notes = (pull: Pull) => | |
| 1008 | ( | |
| 1009 | f.app.db | |
| 1010 | .query("SELECT payload FROM event WHERE kind = 'pull.released'") | |
| 1011 | .all() as { payload: string }[] | |
| 1012 | ) | |
| 1013 | .map((r) => JSON.parse(r.payload)) | |
| 1014 | .filter((p) => p.number === pull.number) | |
| 1015 | .map((p) => p.note); | |
| 1016 | ||
| 1017 | test("a passing check run by the service releases the hold", async () => { | |
| 1018 | const pull = await f.pullOn("svc-one"); | |
| 1019 | setHold(f.app.db, pull.id, f.alice.id, "red alone", "green"); | |
| 1020 | const run = await checks.request(f.repo, `pull-${pull.number}`, pull.headOid, "pull", pull.id); | |
| 1021 | expect(run).toBeDefined(); | |
| 1022 | const deadline = Date.now() + 30_000; | |
| 1023 | while (getHold(f.app.db, pull.id) && Date.now() < deadline) await Bun.sleep(50); | |
| 1024 | expect(getHold(f.app.db, pull.id)).toBeUndefined(); | |
| 1025 | expect(notes(pull)).toEqual([ | |
| 1026 | `hold released: green on ${pull.headOid.slice(0, 7)}`, | |
| 1027 | ]); | |
| 1028 | }, 60_000); | |
| 1029 | ||
| 1030 | test("holding a head that is already green releases at once", async () => { | |
| 1031 | const pull = f.passCheck(await f.pullOn("svc-two")); | |
| 1032 | const held = holdPull(f.app, f.alice, f.repo, pull, "red alone", false, "green"); | |
| 1033 | expect(held.ok).toBe(true); | |
| 1034 | expect(getHold(f.app.db, pull.id)).toBeUndefined(); | |
| 1035 | expect(notes(pull)).toEqual([ | |
| 1036 | `hold released: green on ${pull.headOid.slice(0, 7)}`, | |
| 1037 | ]); | |
| 1038 | // A plain hold on a green head stays; so does one on a head with no green. | |
| 1039 | const plain = f.passCheck(await f.pullOn("svc-three")); | |
| 1040 | holdPull(f.app, f.alice, f.repo, plain, "waiting"); | |
| 1041 | expect(getHold(f.app.db, plain.id)).toBeDefined(); | |
| 1042 | const bare = await f.pullOn("svc-four"); | |
| 1043 | holdPull(f.app, f.alice, f.repo, bare, "red alone", false, "green"); | |
| 1044 | expect(getHold(f.app.db, bare.id)).toBeDefined(); | |
| 1045 | }); | |
| 1046 | }); |
new above
offline
Only the newest check is consulted, which is right. A test with a latest red run would pin that a red head stays held.