Public: anyone can read this; the crew can change it.
An approved pull red for 24 h gets one re-check, then goes back to its builder with the failing test #6358
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6349-an-approved-pull-red-for-24-h-gets-one-r → main 4 commits fork 4 threads · 4 unresolved
3 files
changed +280
−0
src/crew/autonomy.ts +8
| @@ -125,6 +125,7 @@ import { | ||
| 125 | 125 | } from "./direct.ts"; |
| 126 | 126 | import { crewOffInPreview } from "./preview.ts"; |
| 127 | 127 | import { approvedOnHead, finishApproved, finishFields } from "./finishing.ts"; |
| 128 | import { recheckRedApproved } from "./red_approved.ts"; | |
| 128 | 129 | import { readVitals, vitalsMisses, vitalsWords } from "../flow/vitals.ts"; |
| 129 | 130 | import { moveRequested, tallyRequested, tallyWords } from "./requested.ts"; |
| 130 | 131 | import { reviewersOf, sendToReview, sweepReviews } from "./review.ts"; |
| @@ -1234,6 +1235,13 @@ export async function autonomyTick( | ||
| 1234 | 1235 | // First of all: a slot freed goes to an approved pull that no longer |
| 1235 | 1236 | // merges, before a changes-requested one or a new issue (#3682). |
| 1236 | 1237 | const finishing = await finishApproved(app, repo, freeSlots(app, repo, now)); |
| 1238 | // An approved pull red for a day gets one re-check, then goes back (#6349). | |
| 1239 | await recheckRedApproved(app, repo, now).catch((error) => | |
| 1240 | log.error("red approved re-check failed", { | |
| 1241 | repo: `${repo.ownerName}/${repo.name}`, | |
| 1242 | error: String(error), | |
| 1243 | }), | |
| 1244 | ); | |
| 1237 | 1245 | // Quick, and before the picks: a pull main moved under is brought up |
| 1238 | 1246 | // to date within this tick, not after a landing gate (#2039). |
| 1239 | 1247 | await sweepConflicts(app, repo).catch((error) => |
src/crew/red_approved.ts added +127
| @@ -0,0 +1,127 @@ | ||
| 1 | /** | |
| 2 | * An approved pull that stays red gets one more look, then goes back (#6349). | |
| 3 | * | |
| 4 | * #3885 sat approved and red for 5.5 days: nothing moved a pull whose | |
| 5 | * check failed after its approval. Every tick, for each open crew pull | |
| 6 | * approved on its head whose latest check of that head failed | |
| 7 | * {@link RED_AFTER_MS} ago or more: | |
| 8 | * | |
| 9 | * - No re-check was ever queued for the head: one is, reason `recheck`, | |
| 10 | * and a comment says so. One per head, never two; a push is a new head. | |
| 11 | * - The re-check ended red: the crew reviewer who approved the head | |
| 12 | * requests changes on it, in one line naming the first failing test | |
| 13 | * from the check's summary. That verdict is what `requested.ts` hands | |
| 14 | * to the pull's builder, so the pass needs no run of its own. | |
| 15 | * - The re-check ended green, or is still running: nothing. | |
| 16 | */ | |
| 17 | ||
| 18 | import type { App } from "../app.ts"; | |
| 19 | import { addComment } from "../actions/issues.ts"; | |
| 20 | import { checksFor } from "../checks/service.ts"; | |
| 21 | import { listCrewRoles } from "../core/agents.ts"; | |
| 22 | import { getIssueById } from "../core/issues.ts"; | |
| 23 | import { listPulls, pullDir, type Pull } from "../core/pulls.ts"; | |
| 24 | import type { Repo } from "../core/repos.ts"; | |
| 25 | import { listReviews, submitReview } from "../core/reviews.ts"; | |
| 26 | import { getRun, INFRA_SUMMARY, type Run } from "../core/runs.ts"; | |
| 27 | import { getUserById } from "../core/users.ts"; | |
| 28 | import { resolveOid } from "../git/merge.ts"; | |
| 29 | import { createLog } from "../kernel/log.ts"; | |
| 30 | import { approvedOnHead } from "./finishing.ts"; | |
| 31 | ||
| 32 | const log = createLog("crew"); | |
| 33 | ||
| 34 | /** How long a head stays red after its approval before it is checked once more. */ | |
| 35 | export const RED_AFTER_MS = 24 * 3_600_000; | |
| 36 | ||
| 37 | /** The reason the re-check carries. */ | |
| 38 | export const RECHECK = "recheck"; | |
| 39 | ||
| 40 | /** What the pass did with one pull. */ | |
| 41 | export interface RedMove { | |
| 42 | readonly pull: number; | |
| 43 | readonly did: "re-check queued" | "handed back"; | |
| 44 | } | |
| 45 | ||
| 46 | /** The check runs of this head of the pull, newest first. */ | |
| 47 | function checksOfHead(app: App, pull: Pull): Run[] { | |
| 48 | return app.db | |
| 49 | .query<{ id: string }, [string, string]>( | |
| 50 | `SELECT id FROM run WHERE kind = 'check' AND issue_id = ?1 AND commit_oid = ?2 ORDER BY id DESC`, | |
| 51 | ) | |
| 52 | .all(pull.id, pull.headOid) | |
| 53 | .map((r) => getRun(app.db, r.id)!); | |
| 54 | } | |
| 55 | ||
| 56 | /** The line a hand-back says: the first failing test, else the check's own summary. */ | |
| 57 | export function failingLine(run: Run): string { | |
| 58 | const name = run.checkDetail?.names[0]; | |
| 59 | const said = name !== undefined && name !== "" ? name : run.summary.trim().split("\n")[0] ?? ""; | |
| 60 | return said.slice(0, 200) || "the check failed"; | |
| 61 | } | |
| 62 | ||
| 63 | export async function recheckRedApproved(app: App, repo: Repo, now: Date): Promise<RedMove[]> { | |
| 64 | if (repo.autonomy !== "autonomous") return []; | |
| 65 | const moves: RedMove[] = []; | |
| 66 | const reviewers = listCrewRoles(app.db, repo.id) | |
| 67 | .filter((c) => c.role === "reviewer") | |
| 68 | .map((c) => c.agent.user); | |
| 69 | if (reviewers.length === 0) return moves; | |
| 70 | for (const pull of listPulls(app.db, repo.id, { sealed: true, state: "open", limit: 50 })) { | |
| 71 | if (!pull.authorIsAgent || pull.mergedAt !== null || pull.headOid === "") continue; | |
| 72 | if (!approvedOnHead(app.db, pull)) continue; | |
| 73 | const issue = getIssueById(app.db, pull.id); | |
| 74 | if (!issue || issue.state !== "open") continue; | |
| 75 | const runs = checksOfHead(app, pull); | |
| 76 | // A check in flight is the head's answer to come: nothing to say yet. | |
| 77 | if (runs.some((r) => r.state === "queued" || r.state === "running") && !runs.some((r) => r.reason === RECHECK && r.state === "failed")) continue; | |
| 78 | const again = runs.find((r) => r.reason === RECHECK); | |
| 79 | const by = | |
| 80 | reviewers.find((u) => | |
| 81 | listReviews(app.db, pull.id).some( | |
| 82 | (r) => r.authorId === u.id && r.state === "approved" && r.commitOid === pull.headOid, | |
| 83 | ), | |
| 84 | ) ?? reviewers[0]!; | |
| 85 | if (again !== undefined) { | |
| 86 | if (again.state !== "failed") continue; | |
| 87 | // A machine failure is no verdict on the code (#6263): the pull stays approved. | |
| 88 | if (again.summary.startsWith(INFRA_SUMMARY)) continue; | |
| 89 | const line = `Red again after approval: ${failingLine(again)}`; | |
| 90 | const sent = submitReview(app.db, { | |
| 91 | issueId: pull.id, | |
| 92 | authorId: by.id, | |
| 93 | state: "changes_requested", | |
| 94 | body: line, | |
| 95 | commitOid: pull.headOid, | |
| 96 | }); | |
| 97 | if (!sent.ok) continue; | |
| 98 | log.info("approved pull red after its re-check: handed back", { | |
| 99 | repo: `${repo.ownerName}/${repo.name}`, | |
| 100 | pull: pull.number, | |
| 101 | }); | |
| 102 | moves.push({ pull: pull.number, did: "handed back" }); | |
outdated
This ignores queued and running checks, so a head with a check already in flight still looks red for 24 h. Return early here when |
||
| 103 | continue; | |
| 104 | } | |
| 105 | const last = runs.find((r) => r.state === "ok" || r.state === "failed"); | |
| 106 | if (last?.state !== "failed" || last.finishedAt === null) continue; | |
| 107 | if (now.getTime() - Date.parse(last.finishedAt) < RED_AFTER_MS) continue; | |
| 108 | const checks = checksFor(app); | |
| 109 | if (checks === undefined) continue; | |
| 110 | // The head as it is now, as `askCheck` reads it. | |
| 111 | const dir = await pullDir(app.db, app.reposRoot, repo, pull); | |
| 112 | const ref = pull.crossRepo ? `refs/pull/${pull.number}/head` : `refs/heads/${pull.headBranch}`; | |
outdated
|
||
| 113 | if ((await resolveOid(dir, ref)) !== pull.headOid) continue; | |
| 114 | const queued = await checks.request(repo, `pull-${pull.number}`, pull.headOid, RECHECK, pull.id); | |
| 115 | // `request` hands back a check already in flight, which is not ours. | |
| 116 | if (queued?.reason !== RECHECK) continue; | |
| 117 | addComment( | |
| 118 | app, | |
| 119 | getUserById(app.db, by.id), | |
| 120 | repo, | |
| 121 | issue, | |
| 122 | `This pull is approved but its check has been red for 24 h (${failingLine(last)}). One re-check is queued; if it is red too, the pull goes back to its builder.`, | |
| 123 | ); | |
| 124 | moves.push({ pull: pull.number, did: "re-check queued" }); | |
| 125 | } | |
| 126 | return moves; | |
| 127 | } |
test/red-approved.test.ts added +145
| @@ -0,0 +1,145 @@ | ||
| 1 | /** | |
| 2 | * An approved pull red for 24 h gets one re-check per head, then goes back | |
| 3 | * to its builder naming the failing test (#6349). `now` is passed in. | |
| 4 | */ | |
| 5 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 6 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 7 | import { tmpdir } from "node:os"; | |
| 8 | import { join } from "node:path"; | |
| 9 | import { createApp, type App } from "../src/app.ts"; | |
| 10 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 11 | import { openPull } from "../src/actions/pulls.ts"; | |
| 12 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 13 | import { provideChecksForTest, type ChecksHandle } from "../src/checks/service.ts"; | |
| 14 | import { setCrewRole } from "../src/core/agents.ts"; | |
| 15 | import { listComments } from "../src/core/issues.ts"; | |
| 16 | import type { Pull } from "../src/core/pulls.ts"; | |
| 17 | import type { Repo } from "../src/core/repos.ts"; | |
| 18 | import { listReviews, submitReview } from "../src/core/reviews.ts"; | |
| 19 | import { claimRun, createRun, finishRun, INFRA_SUMMARY, listRuns } from "../src/core/runs.ts"; | |
| 20 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; | |
| 21 | import { recheckRedApproved } from "../src/crew/red_approved.ts"; | |
| 22 | ||
| 23 | let dataDir: string; | |
| 24 | let app: App; | |
| 25 | let repo: Repo; | |
| 26 | let pull: Pull; | |
| 27 | let quill: User; | |
| 28 | ||
| 29 | const HOUR = 3_600_000; | |
| 30 | const t0 = Date.now(); | |
| 31 | const rechecks = () => | |
| 32 | listRuns(app.db, repo.id, { kind: "check" }).filter((r) => r.reason === "recheck"); | |
| 33 | ||
| 34 | beforeAll(async () => { | |
| 35 | dataDir = await mkdtemp(join(tmpdir(), "forge-red-approved-")); | |
| 36 | app = createApp({ dataDir, port: 0 }); | |
| 37 | const a = await createUser(app.db, { | |
| 38 | origin: "seed", | |
| 39 | trusted: true, | |
| 40 | username: "alice", | |
| 41 | email: "[email protected]", | |
| 42 | password: "a-good-passphrase", | |
| 43 | adminIfFirst: true, | |
| 44 | }); | |
| 45 | if (!a.ok) throw new Error(a.error); | |
| 46 | const alice = a.value; | |
| 47 | const made = await createRepo(app, alice, { name: "engine", visibility: "private" }); | |
| 48 | if (!made.ok) throw new Error(String(made.error)); | |
| 49 | repo = made.value; | |
| 50 | await commitToBranch(app, alice, repo, { message: "Start", files: [{ path: "a.ts", content: "1\n" }] }); | |
| 51 | for (const name of ["mason", "quill"]) { | |
| 52 | await createAgent(app, alice, { username: name }); | |
| 53 | addToCrew(app, alice, repo, name); | |
| 54 | } | |
| 55 | const mason = getUserByName(app.db, "mason")!; | |
| 56 | quill = getUserByName(app.db, "quill")!; | |
| 57 | setCrewRole(app.db, repo.id, quill.id, "reviewer"); | |
| 58 | await commitToBranch(app, alice, repo, { | |
| 59 | branch: "feature/x", | |
| 60 | base: "main", | |
| 61 | message: "X", | |
| 62 | files: [{ path: "x.ts", content: "2\n" }], | |
| 63 | }); | |
| 64 | const opened = await openPull(app, mason, repo, { | |
| 65 | title: "X", | |
| 66 | body: "", | |
| 67 | baseBranch: "main", | |
| 68 | headBranch: "feature/x", | |
| 69 | }); | |
| 70 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 71 | pull = opened.value; | |
| 72 | submitReview(app.db, { issueId: pull.id, authorId: quill.id, state: "approved", commitOid: pull.headOid }); | |
| 73 | app.db.query("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2 WHERE id = ?1").run(repo.id, alice.id); | |
| 74 | repo = { ...repo, autonomy: "autonomous", autonomyBy: alice.id }; | |
| 75 | const request: ChecksHandle["request"] = async (r, env, oid, reason, issueId) => | |
| 76 | createRun(app.db, { repoId: r.id, kind: "check", env, commitOid: oid, reason, issueId }); | |
| 77 | provideChecksForTest(app, { request } as unknown as ChecksHandle); | |
| 78 | // The pull's own check, red since 25 h before t0. | |
| 79 | const run = createRun(app.db, { repoId: repo.id, kind: "check", env: `pull-${pull.number}`, commitOid: pull.headOid, issueId: pull.id }); | |
| 80 | claimRun(app.db, run.id); | |
| 81 | finishRun(app.db, run.id, { state: "failed", summary: "check failed", checkDetail: { passed: 3, failed: 1, names: ["parses the flag"] } }); | |
| 82 | app.db.query("UPDATE run SET finished_at = ?1 WHERE id = ?2").run(new Date(t0 - 25 * HOUR).toISOString(), run.id); | |
| 83 | }); | |
| 84 | ||
| 85 | afterAll(async () => { | |
| 86 | app.close(); | |
| 87 | await rm(dataDir, { recursive: true, force: true }); | |
| 88 | }); | |
| 89 | ||
| 90 | test("red for 23 h is left alone", async () => { | |
| 91 | expect(await recheckRedApproved(app, repo, new Date(t0 - 2 * HOUR))).toEqual([]); | |
| 92 | expect(rechecks()).toHaveLength(0); | |
| 93 | }); | |
| 94 | ||
| 95 | test("a check of another reason already in flight on the head: no comment, no move, no re-check", async () => { | |
| 96 | const other = createRun(app.db, { repoId: repo.id, kind: "check", env: `pull-${pull.number}`, commitOid: pull.headOid, issueId: pull.id, reason: "push" }); | |
| 97 | for (const n of [0, 1]) expect(await recheckRedApproved(app, repo, new Date(t0 + n * HOUR))).toEqual([]); | |
| 98 | expect(rechecks()).toHaveLength(0); | |
| 99 | expect(listComments(app.db, pull.id).filter((c) => c.body.includes("re-check"))).toHaveLength(0); | |
| 100 | app.db.query("DELETE FROM run WHERE id = ?1").run(other.id); | |
| 101 | }); | |
| 102 | ||
| 103 | test("25 h red gets one re-check, and the second tick queues none", async () => { | |
| 104 | const first = await recheckRedApproved(app, repo, new Date(t0)); | |
| 105 | expect(first).toEqual([{ pull: pull.number, did: "re-check queued" }]); | |
| 106 | expect(rechecks()).toHaveLength(1); | |
| 107 | expect(listComments(app.db, pull.id).some((c) => c.body.includes("One re-check is queued"))).toBe(true); | |
| 108 | expect(await recheckRedApproved(app, repo, new Date(t0 + HOUR))).toEqual([]); | |
| 109 | expect(rechecks()).toHaveLength(1); | |
| 110 | }); | |
| 111 | ||
| 112 | test("a re-check still running or ended green is left alone", async () => { | |
| 113 | const run = rechecks()[0]!; | |
| 114 | const reviews = listReviews(app.db, pull.id).length; | |
| 115 | for (const state of ["queued", "running", "ok"]) { | |
outdated
|
||
| 116 | app.db.query("UPDATE run SET state = ?1 WHERE id = ?2").run(state, run.id); | |
| 117 | expect(await recheckRedApproved(app, repo, new Date(t0 + 2 * HOUR))).toEqual([]); | |
| 118 | } | |
| 119 | expect(listReviews(app.db, pull.id)).toHaveLength(reviews); | |
| 120 | app.db.query("UPDATE run SET state = 'queued' WHERE id = ?1").run(run.id); | |
| 121 | }); | |
| 122 | ||
| 123 | test("a re-check that failed on infrastructure does not hand the pull back", async () => { | |
| 124 | const run = rechecks()[0]!; | |
| 125 | const reviews = listReviews(app.db, pull.id).length; | |
| 126 | app.db.query("UPDATE run SET state = 'failed', summary = ?1 WHERE id = ?2").run(INFRA_SUMMARY, run.id); | |
| 127 | expect(await recheckRedApproved(app, repo, new Date(t0 + 2 * HOUR))).toEqual([]); | |
| 128 | expect(listReviews(app.db, pull.id)).toHaveLength(reviews); | |
| 129 | expect(rechecks()).toHaveLength(1); | |
| 130 | app.db.query("UPDATE run SET state = 'queued', summary = '' WHERE id = ?1").run(run.id); | |
| 131 | }); | |
| 132 | ||
| 133 | test("red after the re-check hands the pull back, naming the test", async () => { | |
| 134 | const run = rechecks()[0]!; | |
| 135 | claimRun(app.db, run.id); | |
| 136 | finishRun(app.db, run.id, { state: "failed", summary: "check failed", checkDetail: { passed: 3, failed: 1, names: ["parses the flag"] } }); | |
| 137 | expect(await recheckRedApproved(app, repo, new Date(t0 + 2 * HOUR))).toEqual([{ pull: pull.number, did: "handed back" }]); | |
| 138 | const last = listReviews(app.db, pull.id).at(-1)!; | |
| 139 | expect(last.state).toBe("changes_requested"); | |
| 140 | expect(last.authorName).toBe("quill"); | |
| 141 | expect(last.body).toContain("parses the flag"); | |
| 142 | // Changes requested: no longer approved, so nothing more is done. | |
| 143 | expect(await recheckRedApproved(app, repo, new Date(t0 + 3 * HOUR))).toEqual([]); | |
| 144 | expect(rechecks()).toHaveLength(1); | |
| 145 | }); |
new above
offline
Skip here when
again.summarystarts withINFRA_SUMMARY. A machine failure must not send the pull back (#6263).