trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns14g rEventsFlows

A failed check says what failed, on the change's page #143

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/125-a-failed-check-says-what-failed-on-the-c → main 1 commit fork

  • Merged. scout/125-a-failed-check-says-what-failed-on-the-c was merged into main as e8be50b.

Review

Sign in to review this change.
7 files changed +262 −8
src/checks/bun-test.ts added +45
@@ -0,0 +1,45 @@
1 /**
2 * `bun test`'s own tail, recognised once as a check finishes — a convenience
3 * over one known runner, not a general test protocol. Anything that is not
4 * exactly this shape (another runner, a build failure, `tsc`'s own errors)
5 * returns undefined, and the caller keeps saying what it always said.
6 */
7
8 import type { CheckDetail } from "../core/runs.ts";
9
10 const FAIL_LINE = /^\(fail\) (.+) \[[0-9.]+m?s\]$/;
11 const PASS_COUNT = /^\s*(\d+) pass$/;
12 const FAIL_COUNT = /^\s*(\d+) fail$/;
13
14 /** How many failing names are kept — a run with forty failures still fits on one line. */
15 export const MAX_FAILING_NAMES = 3;
16
17 /**
18 * The counts and the first few failing names, or undefined when the lines
19 * never carried `bun test`'s own summary (no `N pass` / `M fail`, or `M`
20 * came to zero — a failure from something other than a test).
21 */
22 export function parseBunTestFailure(
23 lines: readonly string[],
24 ): CheckDetail | undefined {
25 const names: string[] = [];
26 let passed: number | undefined;
27 let failed: number | undefined;
28 for (const line of lines) {
29 const fail = FAIL_LINE.exec(line);
30 if (fail) {
31 if (names.length < MAX_FAILING_NAMES) names.push(fail[1]!);
32 continue;
33 }
34 const pass = PASS_COUNT.exec(line);
35 if (pass) {
36 passed = Number(pass[1]);
37 continue;
38 }
39 const failCount = FAIL_COUNT.exec(line);
40 if (failCount) failed = Number(failCount[1]);
41 }
42 if (passed === undefined || failed === undefined || failed === 0)
43 return undefined;
44 return { passed, failed, names };
45 }
src/checks/service.ts +23 −3
@@ -36,6 +36,8 @@ import { appendRunLog } from "../runs/log.ts";
3636 import { exportTree, readFileAt } from "../apps/checkout.ts";
3737 import { parseCheck } from "../apps/manifest.ts";
3838 import { childEnv, runStep } from "../apps/process.ts";
39 import { parseBunTestFailure } from "./bun-test.ts";
40 import type { CheckDetail } from "../core/runs.ts";
3941
4042 const log = createLog("checks");
4143
@@ -284,6 +286,9 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
284286 summary: outcome.ok
285287 ? `check passed in ${seconds}s`
286288 : `check failed: ${outcome.error}`,
289 ...(outcome.ok || outcome.detail === undefined
290 ? {}
291 : { checkDetail: outcome.detail }),
287292 });
288293 write(outcome.ok ? `passed in ${seconds}s` : `failed: ${outcome.error}`);
289294 if (done) announceRun(app, done);
@@ -297,7 +302,10 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
297302 repo: Repo,
298303 write: (line: string) => void,
299304 signal: AbortSignal,
300 ): Promise<{ ok: true } | { ok: false; error: string }> {
305 ): Promise<
306 | { ok: true }
307 | { ok: false; error: string; detail?: CheckDetail | undefined }
308 > {
301309 const bare = repoDir(app.reposRoot, repo.ownerName, repo.name);
302310 const [forgeJson, packageJson] = await Promise.all([
303311 readFileAt(bare, run.commitOid, "forge.json"),
@@ -337,14 +345,26 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
337345 });
338346 if (!installed.ok) return { ok: false, error: installed.error };
339347 }
348 // The check step's own lines are kept beside what `write` already
349 // logs, so a failure can be summarised once here — never by reading
350 // the log back, on this run or any later render of it.
351 const checkLines: string[] = [];
340352 const ran = await runStep("check", manifest.run, {
341353 cwd: dir,
342354 env,
343 log: write,
355 log: (line) => {
356 checkLines.push(line);
357 write(line);
358 },
344359 signal,
345360 timeoutMs: manifest.timeout * 1000,
346361 });
347 if (!ran.ok) return { ok: false, error: ran.error };
362 if (!ran.ok)
363 return {
364 ok: false,
365 error: ran.error,
366 detail: parseBunTestFailure(checkLines),
367 };
348368 return { ok: true };
349369 } finally {
350370 if (existsSync(dir)) rmSync(dir, { recursive: true, force: true });
src/core/runs.ts +20 −1
@@ -21,6 +21,18 @@ export const RUN_TERMINAL: readonly RunState[] = ["ok", "failed", "cancelled"];
2121 export const isTerminal = (state: RunState): boolean =>
2222 RUN_TERMINAL.includes(state);
2323
24 /**
25 * What a failed check's own output said, recognised once as the run
26 * finishes (`checks/bun-test.ts`) and never re-parsed: the pass/fail
27 * counts, and the first few failing names — already bounded, so a run with
28 * forty failures still fits on one line wherever this is shown.
29 */
30 export interface CheckDetail {
31 readonly passed: number;
32 readonly failed: number;
33 readonly names: readonly string[];
34 }
35
2436 export interface Run {
2537 readonly id: string;
2638 readonly repoId: string;
@@ -50,6 +62,8 @@ export interface Run {
5062 readonly heartbeatAt: string | null;
5163 /** For an agent run: how many turns the model took. */
5264 readonly turns: number;
65 /** A failed check's own detail, when its output was recognised; null otherwise. */
66 readonly checkDetail: CheckDetail | null;
5367 readonly createdAt: string;
5468 readonly startedAt: string | null;
5569 /** When this deploy's environment started serving — set once, at the swap-in. */
@@ -75,6 +89,7 @@ interface RunRow {
7589 attempt: number;
7690 heartbeat_at: string | null;
7791 turns: number;
92 check_detail: string;
7893 created_at: string;
7994 started_at: string | null;
8095 serving_at: string | null;
@@ -99,6 +114,7 @@ const toRun = (r: RunRow): Run => ({
99114 attempt: r.attempt,
100115 heartbeatAt: r.heartbeat_at,
101116 turns: r.turns,
117 checkDetail: r.check_detail ? (JSON.parse(r.check_detail) as CheckDetail) : null,
102118 createdAt: r.created_at,
103119 startedAt: r.started_at,
104120 servingAt: r.serving_at,
@@ -196,13 +212,15 @@ export function finishRun(
196212 sessionId?: string | undefined;
197213 costUsd?: number | undefined;
198214 turns?: number | undefined;
215 /** A failed check's own detail, when its output was recognised. */
216 checkDetail?: CheckDetail | undefined;
199217 },
200218 ): Run | undefined {
201219 const changes = db
202220 .query(
203221 `UPDATE run SET state = ?2, summary = ?3,
204222 session_id = COALESCE(?4, session_id), cost_usd = COALESCE(?5, cost_usd),
205 turns = COALESCE(?6, turns),
223 turns = COALESCE(?6, turns), check_detail = ?7,
206224 finished_at = strftime('%Y-%m-%dT%H:%M:%fZ','now')
207225 WHERE id = ?1 AND state = 'running'`,
208226 )
@@ -213,6 +231,7 @@ export function finishRun(
213231 outcome.sessionId ?? null,
214232 outcome.costUsd ?? null,
215233 outcome.turns ?? null,
234 outcome.checkDetail ? JSON.stringify(outcome.checkDetail) : "",
216235 ).changes;
217236 return changes > 0 ? getRun(db, runId) : undefined;
218237 }
src/core/schema.ts +10
@@ -752,4 +752,14 @@ export const migrations: readonly string[] = [
752752 UNIQUE (app_id, env, commit_oid, kind)
753753 );
754754 `,
755
756 // 25 — a failed check's own detail: the pass/fail counts and the first
757 // few failing names, recognised once from a `bun test` tail as the run
758 // finishes (`checks/bun-test.ts`) and stored as JSON beside the run, so a
759 // page that shows it never re-parses a log. Empty when the run passed or
760 // its output was not recognised — the studio then says exactly what it
761 // said before this column existed.
762 `
763 ALTER TABLE run ADD COLUMN check_detail TEXT NOT NULL DEFAULT '';
764 `,
755765 ];
src/web/studio/words.ts +24 −4
@@ -15,6 +15,7 @@ import { getIssueById } from "../../core/issues.ts";
1515 import { getPull, pullForBranch, type Pull } from "../../core/pulls.ts";
1616 import {
1717 checkSummary,
18 type CheckDetail,
1819 type CheckSummary,
1920 type Run,
2021 } from "../../core/runs.ts";
@@ -86,15 +87,34 @@ function mergedPullWord(
8687 return { word: "waiting", note: "merged; waiting for production to redeploy" };
8788 }
8889
90 /**
91 * What failed, in a sentence: the counts `parseBunTestFailure` already
92 * recognised, stored with the run — never re-read from its log here — and
93 * the first few names it kept, escaped as text like any other value
94 * (`html`, `web/html.ts`) wherever this lands on a page.
95 */
96 function detailNote(detail: CheckDetail): string {
97 const more = detail.failed - detail.names.length;
98 const names = detail.names.join(", ") + (more > 0 ? `, and ${more} more` : "");
99 return `${detail.passed} passed, ${detail.failed} failed: ${names}`;
100 }
101
89102 /** A single open pull's check, in the studio's own words — never dev's
90103 * "Checks passed/failed": the same five words as the rest of the page. */
91104 function checkRunWord(check: CheckSummary): { word: Word; note: string } {
92105 if (check.state === "ok")
93106 return { word: "look", note: "green — ready for a look" };
94 if (check.state === "failed")
95 return check.run?.servingAt != null
96 ? { word: "stopped", note: "serving, but the check failed" }
97 : { word: "stopped", note: "failed before it answered" };
107 if (check.state === "failed") {
108 const base =
109 check.run?.servingAt != null
110 ? "serving, but the check failed"
111 : "failed before it answered";
112 const detail = check.run?.checkDetail;
113 return {
114 word: "stopped",
115 note: detail ? `${base} — ${detailNote(detail)}` : base,
116 };
117 }
98118 if (check.state === "running")
99119 return {
100120 word: "working",
test/checks.test.ts +82
@@ -11,6 +11,7 @@ import { join } from "node:path";
1111 import { createApp, type App } from "../src/app.ts";
1212 import { openPull } from "../src/actions/pulls.ts";
1313 import { commitToBranch, createRepo, forkRepo } from "../src/actions/repos.ts";
14 import { parseBunTestFailure } from "../src/checks/bun-test.ts";
1415 import {
1516 pickNext,
1617 startChecks,
@@ -33,6 +34,47 @@ import { readRunLog } from "../src/runs/log.ts";
3334 const checkJson = (run: string, extra = ""): string =>
3435 `{"check": {"run": ${JSON.stringify(run)}, "install": ""${extra}}}\n`;
3536
37 describe("parseBunTestFailure: a convenience over one known runner, not a general protocol", () => {
38 test("bun test's own tail: the counts, and the failing names bounded to a few", () => {
39 const lines = [
40 "bun test v1.3.14 (0d9b296a)",
41 "",
42 "check.test.ts:",
43 "(fail) first [0.16ms]",
44 "(fail) second [0.07ms]",
45 "(fail) third [0.05ms]",
46 "(fail) fourth [0.04ms]",
47 "",
48 " 3 pass",
49 " 4 fail",
50 " 7 expect() calls",
51 "Ran 7 tests across 1 file. [51.00ms]",
52 ];
53 expect(parseBunTestFailure(lines)).toEqual({
54 passed: 3,
55 failed: 4,
56 names: ["first", "second", "third"],
57 });
58 });
59
60 test("a name arbitrary text from a subprocess, kept verbatim — escaping is the page's job, not the parser's", () => {
61 const lines = ["(fail) <script>alert(1)</script> [1.00ms]", " 0 pass", " 1 fail", "Ran 1 test across 1 file. [1.00ms]"];
62 expect(parseBunTestFailure(lines)?.names).toEqual([
63 "<script>alert(1)</script>",
64 ]);
65 });
66
67 test("output that is not bun test's own shape is not guessed at", () => {
68 expect(
69 parseBunTestFailure(["$ test -f NOPE.md", "test: NOPE.md: No such file or directory"]),
70 ).toBeUndefined();
71 // A `pass` line with no `fail` line beside it is not this shape either.
72 expect(parseBunTestFailure([" 5 pass", "Ran 5 tests across 1 file."])).toBeUndefined();
73 // All green: nothing failed, so there is nothing to summarise.
74 expect(parseBunTestFailure([" 5 pass", " 0 fail", "Ran 5 tests across 1 file."])).toBeUndefined();
75 });
76 });
77
3678 describe("checks", () => {
3779 let dataDir: string;
3880 let app: App;
@@ -148,6 +190,9 @@ describe("checks", () => {
148190 expect(failed.env).toBe(`pull-${pull.number}`);
149191 expect(failed.reason).toBe("pull");
150192 expect(failed.summary).toBe("check failed: check exited with code 1");
193 // `test -f` is not `bun test`: its output is not this shape, so nothing
194 // is guessed at it.
195 expect(failed.checkDetail).toBeNull();
151196 expect(
152197 checksForHead(app.db, pull.id, branched.value.commitOid)[0]?.id,
153198 ).toBe(failed.id);
@@ -165,6 +210,43 @@ describe("checks", () => {
165210 expect(passed.reason).toBe("push");
166211 }, 60_000);
167212
213 test("a bun test failure in the check step is summarised: how many passed, how many failed, and their names", async () => {
214 const branched = await commitToBranch(app, alice, repo, {
215 branch: "red-tests",
216 message: "A check that runs bun test, with two failures",
217 files: [
218 { path: "forge.json", content: checkJson("bun test check.test.ts") },
219 {
220 path: "check.test.ts",
221 content: [
222 'import { test, expect } from "bun:test";',
223 'test("a passing test", () => { expect(1).toBe(1); });',
224 'test("first failing test", () => { expect(1).toBe(2); });',
225 'test("second failing test", () => { expect(1).toBe(2); });',
226 "",
227 ].join("\n"),
228 },
229 ],
230 });
231 if (!branched.ok) throw new Error(JSON.stringify(branched.error));
232 const opened = await openPull(app, alice, repo, {
233 title: "Red tests",
234 baseBranch: "main",
235 headBranch: "red-tests",
236 });
237 if (!opened.ok) throw new Error(String(opened.error));
238 const pull = opened.value;
239 const failed = await settled(
240 (r) => r.issueId === pull.id && r.commitOid === branched.value.commitOid,
241 );
242 expect(failed.state).toBe("failed");
243 expect(failed.checkDetail).toEqual({
244 passed: 1,
245 failed: 2,
246 names: ["first failing test", "second failing test"],
247 });
248 }, 30_000);
249
168250 test("a fork's pull is checked at the base's snapshot, and follows the fork's pushes", async () => {
169251 const forked = await forkRepo(app, bob, repo);
170252 if (!forked.ok) throw new Error(String(forked.error));
test/studio.test.ts +58
@@ -354,6 +354,64 @@ describe("the studio", () => {
354354 expect(merged.ok).toBe(true);
355355 });
356356
357 test("a failed check's own page says what failed — the counts and the first names, bounded, and escaped as text", async () => {
358 const branch = await commitToBranch(app, alice, repo, {
359 branch: "feature/red-check",
360 base: "main",
361 message: "A change whose check goes red",
362 files: [{ path: "RED.md", content: "red\n" }],
363 });
364 if (!branch.ok) throw new Error(JSON.stringify(branch.error));
365 const opened = await openPull(app, alice, repo, {
366 title: "A change whose check goes red",
367 body: "",
368 baseBranch: "main",
369 headBranch: "feature/red-check",
370 });
371 if (!opened.ok) throw new Error(String(opened.error));
372 const pull = opened.value;
373 const run = createRun(app.db, {
374 repoId: repo.id,
375 kind: "check",
376 issueId: pull.id,
377 env: `pull-${pull.number}`,
378 commitOid: pull.headOid,
379 reason: "pull",
380 });
381 claimRun(app.db, run.id);
382 finishRun(app.db, run.id, {
383 state: "failed",
384 summary: "check failed: check exited with code 1",
385 checkDetail: {
386 passed: 274,
387 failed: 4,
388 names: [
389 "a pull closed under a running agent > the clone goes after it",
390 "<script>alert(1)</script>",
391 "a third one",
392 ],
393 },
394 });
395
396 const studio = await (
397 await get(`/alice/notes/pulls/${pull.number}`)
398 ).text();
399 expect(studio).toContain("data-word=\"stopped\"");
400 expect(studio).toContain("274 passed, 4 failed");
401 expect(studio).toContain(
402 "a pull closed under a running agent &gt; the clone goes after it",
403 );
404 // A fourth failing test beyond the bound is only counted, not named.
405 expect(studio).toContain("and 1 more");
406 // A test name is arbitrary text from a subprocess: it reaches the page
407 // escaped, never as markup a browser would run.
408 expect(studio).not.toContain("<script>alert(1)</script>");
409 expect(studio).toContain("&lt;script&gt;alert(1)&lt;/script&gt;");
410 // The link to the run's full log is unchanged: a summary, not a
411 // replacement for it.
412 expect(studio).toContain(`href="/alice/notes/runs/${run.id}"`);
413 });
414
357415 test("a change that no longer merges into its base says so, and offers no Ship — a mergeable green pull is unchanged", async () => {
358416 const scout = getUserByName(app.db, "scout")!;
359417 const base1 = await commitToBranch(app, alice, repo, {

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs