Public: anyone can read this; the crew can change it.
A 'Red after landing' issue closes itself when main's next green verification passes the file it names #11400
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/11395-a-red-after-landing-issue-closes-itself → main 1 commit fork
3 files
changed +115
−12
src/checks/culprit.ts +57 −3
| @@ -8,13 +8,16 @@ | ||
| 8 | 8 | * a push no pull carried gets one issue. A red whose files were already red |
| 9 | 9 | * at the verification before it was named there and starts nothing. |
| 10 | 10 | * |
| 11 | * A later green verification that ran every file such an issue names | |
| 12 | * closes it as done (#11395), unless a person has commented on it. | |
| 13 | * | |
| 11 | 14 | * Called by the checks service with the way it asks for a run; the answer |
| 12 | 15 | * is the lines for the finished run's log. |
| 13 | 16 | */ |
| 14 | 17 | |
| 15 | 18 | import type { App } from "../app.ts"; |
| 16 | 19 | import { revertPull } from "../actions/revert.ts"; |
| 17 | import { createIssue } from "../core/issues.ts"; | |
| 20 | import { addComment, createIssue, setIssueState } from "../core/issues.ts"; | |
| 18 | 21 | import { getPull, type Pull } from "../core/pulls.ts"; |
| 19 | 22 | import type { Repo } from "../core/repos.ts"; |
| 20 | 23 | import { getRun, type Run } from "../core/runs.ts"; |
| @@ -41,6 +44,9 @@ const FILED_BY = "_Filed by the forge's verification after landing, not written | ||
| 41 | 44 | |
| 42 | 45 | const short = (oid: string): string => oid.slice(0, 7); |
| 43 | 46 | |
| 47 | /** How every issue this module files is titled. */ | |
| 48 | const TITLE_PREFIX = "Red after landing: "; | |
| 49 | ||
| 44 | 50 | |
| 45 | 51 | /** The last green verification before run `below`. */ |
| 46 | 52 | function greenBefore(app: App, repoId: string, below: string): Run | undefined { |
| @@ -153,7 +159,7 @@ async function name(app: App, repo: Repo, red: Run, culprit: string, green: stri | ||
| 153 | 159 | ? `culprit #${pull.number} (${short(culprit)}): proposed its revert, #${made.value.number}` |
| 154 | 160 | : `culprit #${pull.number} (${short(culprit)}): the revert could not be proposed (${made.error})`; |
| 155 | 161 | } |
| 156 | const title = `Red after landing: ${short(culprit)} broke ${files[0] ?? "main"}`; | |
| 162 | const title = `${TITLE_PREFIX}${short(culprit)} broke ${files[0] ?? "main"}`; | |
| 157 | 163 | return `culprit ${short(culprit)}, landed by no pull: ${fileOnce( |
| 158 | 164 | app, |
| 159 | 165 | repo, |
| @@ -168,7 +174,7 @@ function unbisected(app: App, repo: Repo, red: Run, why: string): string { | ||
| 168 | 174 | return `not bisected (${why}): ${fileOnce( |
| 169 | 175 | app, |
| 170 | 176 | repo, |
| 171 | `Red after landing: ${files[0] ?? "main"} fails on ${repo.defaultBranch}`, | |
| 177 | `${TITLE_PREFIX}${files[0] ?? "main"} fails on ${repo.defaultBranch}`, | |
| 172 | 178 | `${FILED_BY}\n\nMain's verification ${red.id} is red at ${short(red.commitOid)}: ${redSays(red)}, failing alone too. The forge did not name the landing that broke it: ${why}.\n\nFiles: ${files.join(", ")}`, |
| 173 | 179 | )}`; |
| 174 | 180 | } |
| @@ -209,3 +215,51 @@ export async function afterVerify(app: App, repo: Repo, run: Run, ask: Ask): Pro | ||
| 209 | 215 | `at most ${maxSteps(range.length)} step(s); run ${asked.id}`, |
| 210 | 216 | ]; |
| 211 | 217 | } |
| 218 | ||
| 219 | /** The files an issue's closing `Files:` line names; empty when it has none. */ | |
| 220 | function filesNamed(body: string): string[] { | |
| 221 | const line = /\nFiles: ([^\n]+)$/.exec(body); | |
| 222 | return line === null ? [] : line[1]!.split(",").map((f) => f.trim()).filter(Boolean); | |
| 223 | } | |
| 224 | ||
| 225 | /** | |
| 226 | * The lines for a green verification's log: one per open 'Red after | |
| 227 | * landing' issue this module filed before `run` was asked for, whose every | |
| 228 | * named file `run` printed passing (`verify.ts` `passedFiles`), commented on | |
| 229 | * and closed as done (#11395). An issue a person (any account not an agent) | |
| 230 | * has commented on stays open: the comment is their ask. Only an issue the | |
| 231 | * forge filed is closed: its title, the owner as author and its opening line. | |
| 232 | */ | |
| 233 | export function closeOnGreen(app: App, repo: Repo, run: Run, passed: readonly string[]): string[] { | |
| 234 | if (run.env !== VERIFY_ENV || run.state !== "ok" || passed.length === 0) return []; | |
| 235 | const open = app.db | |
| 236 | .query<{ id: string; number: number; title: string; body: string }, [string, string, string, string]>( | |
| 237 | `SELECT issue.id, issue.number, issue.title, issue.body FROM issue | |
| 238 | WHERE issue.repo_id = ?1 AND issue.kind = 'issue' AND issue.state = 'open' | |
| 239 | AND issue.author_id = ?2 AND issue.title LIKE ?3 AND issue.created_at < ?4 | |
| 240 | AND NOT EXISTS (SELECT 1 FROM comment JOIN user ON user.id = comment.author_id | |
| 241 | WHERE comment.issue_id = issue.id AND user.is_agent = 0) | |
| 242 | ORDER BY issue.number`, | |
| 243 | ) | |
| 244 | .all(repo.id, repo.ownerId, `${TITLE_PREFIX}%`, run.createdAt); | |
| 245 | const where = `verification ${run.id} at ${short(run.commitOid)}`; | |
| 246 | const lines: string[] = []; | |
| 247 | for (const issue of open) { | |
| 248 | if (!issue.title.startsWith(TITLE_PREFIX) || !issue.body.startsWith(FILED_BY)) continue; | |
| 249 | const files = filesNamed(issue.body); | |
| 250 | if (files.length === 0 || !files.every((f) => passed.includes(f))) continue; | |
| 251 | const body = `Green again: main's ${where} ran and passed ${files.map((f) => `\`${f}\``).join(", ")}.`; | |
| 252 | const noted = addComment(app.db, { issueId: issue.id, authorId: repo.ownerId, body }); | |
| 253 | if (noted.ok) | |
| 254 | app.bus.append({ | |
| 255 | kind: "issue.commented", | |
| 256 | repoId: repo.id, | |
| 257 | actorId: repo.ownerId, | |
| 258 | payload: { number: issue.number, body, commentId: noted.value.id }, | |
| 259 | }); | |
| 260 | if (setIssueState(app.db, issue.id, "closed", "done") === undefined) continue; | |
| 261 | app.bus.append({ kind: "issue.closed", repoId: repo.id, actorId: repo.ownerId, payload: { number: issue.number } }); | |
| 262 | lines.push(`culprit: closed #${issue.number}, ${files.join(", ")} green again`); | |
| 263 | } | |
| 264 | return lines; | |
| 265 | } |
src/checks/service.ts +3 −1
| @@ -68,7 +68,7 @@ import { ASKED, redMain, unheld } from "./red_main.ts"; | ||
| 68 | 68 | import { proposeRevert } from "./revert_red.ts"; |
| 69 | 69 | import { rebasesFirst } from "./finishing_first.ts"; |
| 70 | 70 | import { IMPACT_FILE, keepImpact, seedImpact } from "./impact-store.ts"; |
| 71 | import { afterVerify } from "./culprit.ts"; | |
| 71 | import { afterVerify, closeOnGreen } from "./culprit.ts"; | |
| 72 | 72 | import { redDetail, reportGreen, reportRed } from "./report.ts"; |
| 73 | 73 | import { |
| 74 | 74 | bisectOrigin, |
| @@ -886,6 +886,8 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 886 | 886 | reportRed(app, repo, done).forEach(write); |
| 887 | 887 | // A file the run printed passing closes its open report issue (#9524). |
| 888 | 888 | reportGreen(app, repo, done, outcome.passed ?? []).forEach(write); |
| 889 | // A green that ran every file a culprit's issue names closes it (#11395). | |
| 890 | closeOnGreen(app, repo, done, outcome.passed ?? []).forEach(write); | |
| 889 | 891 | } |
| 890 | 892 | if (done && inVerifyLane(run.env)) |
| 891 | 893 | await afterVerify(app, repo, done, (r, env, oid, reason) => request(r, env, oid, reason)) |
test/check-verify.test.ts +55 −8
| @@ -11,7 +11,9 @@ import { join } from "node:path"; | ||
| 11 | 11 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| 12 | 12 | import { createApp, type App } from "../src/app.ts"; |
| 13 | 13 | import { startChecks, type ChecksHandle } from "../src/checks/service.ts"; |
| 14 | import { closeOnGreen } from "../src/checks/culprit.ts"; | |
| 14 | 15 | import { VERIFY_ENV } from "../src/checks/verify.ts"; |
| 16 | import { addComment } from "../src/core/issues.ts"; | |
| 15 | 17 | import type { Repo } from "../src/core/repos.ts"; |
| 16 | 18 | import { getRun, listRuns, type Run } from "../src/core/runs.ts"; |
| 17 | 19 | import { createUser, type User } from "../src/core/users.ts"; |
| @@ -31,11 +33,14 @@ const report = (fail: boolean): string => | ||
| 31 | 33 | ? `printf 't/a.test.ts:\\n(fail) grp > works\\n 0 pass\\n 1 fail\\n'; exit 1` |
| 32 | 34 | : `printf 't/a.test.ts:\\n(pass) grp > works\\n 1 pass\\n 0 fail\\n'`; |
| 33 | 35 | |
| 34 | async function push(fail: boolean, note = ""): Promise<string> { | |
| 36 | /** A green run that printed only another file: it says nothing of t/a.test.ts. */ | |
| 37 | const otherGreen = `printf 't/b.test.ts:\\n(pass) other > works\\n 1 pass\\n 0 fail\\n'`; | |
| 38 | ||
| 39 | async function push(fail: boolean, note = "", verify = report(fail)): Promise<string> { | |
| 35 | 40 | const pushed = await commitToBranch(app, alice, repo, { |
| 36 | 41 | message: fail ? "breaks it" : "works", |
| 37 | 42 | files: [ |
| 38 | { path: "forge.json", content: JSON.stringify({ check: { run: "true", verify: report(fail), install: "" } }) }, | |
| 43 | { path: "forge.json", content: JSON.stringify({ check: { run: "true", verify, install: "" } }) }, | |
| 39 | 44 | { |
| 40 | 45 | path: "t/a.test.ts", |
| 41 | 46 | content: `${note}import { test, expect, describe } from "bun:test";\ndescribe("grp", () => { test("works", () => { expect(${fail ? 2 : 1}).toBe(1); }); });\n`, |
| @@ -117,6 +122,23 @@ test("a red verification that fails alone too names the push that broke it in on | ||
| 117 | 122 | expect(log).toContain("landed by no pull"); |
| 118 | 123 | }, 60_000); |
| 119 | 124 | |
| 125 | const culpritIssue = (): { id: string; state: string } | undefined => | |
| 126 | app.db | |
| 127 | .query<{ id: string; state: string }, [string]>( | |
| 128 | "SELECT id, state FROM issue WHERE repo_id = ?1 AND kind = 'issue' AND title LIKE 'Red after landing:%' ORDER BY number LIMIT 1", | |
| 129 | ) | |
| 130 | .get(repo.id) ?? undefined; | |
| 131 | ||
| 132 | test("a green verification that did not run the culprit issue's file leaves it open (#11395)", async () => { | |
| 133 | const oid = await push(false, "// elsewhere\n", otherGreen); | |
| 134 | await until(() => finished(repo.defaultBranch, oid)); | |
| 135 | expect(await checks.verifyDue(Date.now() + 31 * 60_000)).toHaveLength(1); | |
| 136 | const run = await until(() => finished(VERIFY_ENV, oid)); | |
| 137 | expect(run.state).toBe("ok"); | |
| 138 | expect(culpritIssue()?.state).toBe("open"); | |
| 139 | expect(readRunLog(app.config.dataDir, run.id).lines.join("\n")).not.toContain("culprit: closed"); | |
| 140 | }, 60_000); | |
| 141 | ||
| 120 | 142 | test("a later green verification closes the report's issue for the file it passed (#9524)", async () => { |
| 121 | 143 | // A tree never proven green before: a proven one is not run again, and prints no file. |
| 122 | 144 | const oid = await push(false, "// fixed\n"); |
| @@ -137,10 +159,35 @@ test("a later green verification closes the report's issue for the file it passe | ||
| 137 | 159 | .all(closed.id) |
| 138 | 160 | .map((c) => c.body); |
| 139 | 161 | expect(said).toContain(`Green again in the full suite (check run ${run.id}, ${oid.slice(0, 7)}).`); |
| 140 | // The culprit's issue is not the report's: it stays open. | |
| 141 | const culprit = app.db | |
| 142 | .query<{ state: string }, [string]>("SELECT state FROM issue WHERE repo_id = ?1 AND title LIKE 'Red after landing:%'") | |
| 143 | .get(repo.id); | |
| 144 | expect(culprit?.state).toBe("open"); | |
| 145 | expect(readRunLog(app.config.dataDir, run.id).lines.join("\n")).toContain("t/a.test.ts green again"); | |
| 162 | // The culprit's issue names the same file: the green that ran it closes it too (#11395). | |
| 163 | const culprit = await until(() => { | |
| 164 | const got = culpritIssue(); | |
| 165 | return got?.state === "closed" ? got : undefined; | |
| 166 | }); | |
| 167 | expect(app.db.query<{ close_reason: string }, [string]>("SELECT close_reason FROM issue WHERE id = ?1").get(culprit.id)?.close_reason).toBe("done"); | |
| 168 | const told = app.db | |
| 169 | .query<{ body: string }, [string]>("SELECT body FROM comment WHERE issue_id = ?1") | |
| 170 | .all(culprit.id) | |
| 171 | .map((c) => c.body); | |
| 172 | expect(told).toEqual([`Green again: main's verification ${run.id} at ${oid.slice(0, 7)} ran and passed \`t/a.test.ts\`.`]); | |
| 173 | const log = readRunLog(app.config.dataDir, run.id).lines.join("\n"); | |
| 174 | expect(log).toContain("t/a.test.ts green again"); | |
| 175 | expect(log).toContain("culprit: closed"); | |
| 146 | 176 | }, 60_000); |
| 177 | ||
| 178 | test("a culprit issue a person has commented on stays open through a green (#11395)", async () => { | |
| 179 | const run = listRuns(app.db, repo.id, { kind: "check", limit: 50 }).find((r) => r.env === VERIFY_ENV && r.state === "ok")!; | |
| 180 | const made = app.db | |
| 181 | .query<{ id: string; number: number }, [string]>("SELECT id, number FROM issue WHERE repo_id = ?1 AND title LIKE 'Red after landing:%'") | |
| 182 | .get(repo.id)!; | |
| 183 | // Reopened as the forge filed it, then a person asks something on it. | |
| 184 | app.db.query("UPDATE issue SET state = 'open', close_reason = NULL, closed_at = NULL WHERE id = ?1").run(made.id); | |
| 185 | app.db.query("DELETE FROM comment WHERE issue_id = ?1").run(made.id); | |
| 186 | const later = { ...run, createdAt: new Date(Date.now() + 60_000).toISOString() }; | |
| 187 | expect(addComment(app.db, { issueId: made.id, authorId: alice.id, body: "Still seeing this locally?" }).ok).toBe(true); | |
| 188 | expect(closeOnGreen(app, repo, later, ["t/a.test.ts"])).toEqual([]); | |
| 189 | expect(culpritIssue()?.state).toBe("open"); | |
| 190 | // Without the person's comment, the same green closes it. | |
| 191 | app.db.query("DELETE FROM comment WHERE issue_id = ?1").run(made.id); | |
| 192 | expect(closeOnGreen(app, repo, later, ["t/a.test.ts"])).toEqual([`culprit: closed #${made.number}, t/a.test.ts green again`]); | |
| 193 | }); |