trevato/forge

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

More
Forks1g fMapg aRunsg rEventsFlows

A park note says what each failed try of the pull changed #10883

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/10863-a-park-note-says-what-each-failed-try-of → main 3 commits fork 1 thread · 1 unresolved

  • Merged. scout/10863-a-park-note-says-what-each-failed-try-of was merged into main as d0a6480.

Review

Sign in to review this change.
12 files changed +207 −42
docs/features/park-note-tries.md added +14
@@ -0,0 +1,14 @@
1 # A park note tells what each failed try changed
2
3 When the crew parks a pull because its check stayed red, the "Parked by the crew" comment on the issue the pull closes adds one line per failed try, oldest first:
4
5 ```
6 try 1: src/a.ts, src/b.ts
7 try 2: src/b.ts (+3 more)
8 ```
9
10 - Each line is the head the check went red on and the files it changed since the try before it (the first, since the base): at most 5 tries, at most 10 files each.
11 - Only tries after the last human word on the pull count, the same ones that counted toward the park.
12 - A pull with fewer than two failed tries adds nothing.
13 - Code: `triesChanged` in `src/crew/tries.ts`, told by `parkPullTold` in `src/crew/park_pull.ts`.
14 - Try it: let an autonomous repo's agent pull go red twice and read the issue's comments.
scripts/unreached-allow.json −1
@@ -2,7 +2,6 @@
22 "src/apps/cloud.ts",
33 "src/boards/machine.ts",
44 "src/crew/governor_log.ts",
5 "src/crew/tries.ts",
65 "src/flow/alarm_drift.ts",
76 "src/flow/alarm_unproposed.ts",
87 "src/flow/approved_wait.ts",
src/crew/autonomy.ts +2 −2
@@ -142,7 +142,7 @@ import { approvedOnHead, finishApproved, finishFields } from "./finishing.ts";
142142 import { recheckRedApproved } from "./red_approved.ts";
143143 import { sweepStaleReds } from "./stale_reds.ts";
144144 import { sweepStranded } from "./stranded.ts";
145 import { parkPull, strikeNote } from "./park_pull.ts";
145 import { parkPull, parkPullTold, strikeNote } from "./park_pull.ts";
146146 import { keepVerdict } from "../flow/culprits.ts";
147147 import { sweepRepeats } from "./repeat_checks.ts";
148148 import { breathe } from "../kernel/breathe.ts";
@@ -157,7 +157,7 @@ import { bootCounts } from "../kernel/boots.ts";
157157
158158 const log = createLog("autonomy");
159159
160 export { parkPull };
160 export { parkPull, parkPullTold };
161161
162162 export const AUTONOMY_TICK_MS = 60_000;
163163 /**
src/crew/dispatch.ts +21 −13
@@ -103,7 +103,7 @@ import {
103103 autonomyTick,
104104 landIfGreen,
105105 parkForBreaker,
106 parkPull,
106 parkPullTold,
107107 settleAgentRun,
108108 spentToday,
109109 } from "./autonomy.ts";
@@ -940,19 +940,27 @@ function answerRed(
940940 const since = lastHumanWord(app, issue, agent);
941941 if (automaticRunsSince(app.db, issue.id, since) >= CHECK_FAILED_LIMIT) {
942942 const pull = getPull(app.db, repo.id, issue.number);
943 if (
944 pull &&
945 parkPull(
946 app,
947 repo,
948 pull,
949 `the check stayed red through ${CHECK_FAILED_LIMIT} automatic tries`,
950 )
943 const waiting = () =>
944 log.info("check failed again; waiting for a person", {
945 issue: issue.number,
946 });
947 if (pull === undefined) return waiting();
948 parkPullTold(
949 app,
950 repo,
951 pull,
952 `the check stayed red through ${CHECK_FAILED_LIMIT} automatic tries`,
953 since,
951954 )
952 return;
953 log.info("check failed again; waiting for a person", {
954 issue: issue.number,
955 });
955 .then((parked) => {
956 if (!parked) waiting();
957 })
958 .catch((error) =>
959 log.error("parking a red pull failed", {
960 issue: issue.number,
961 error: String(error),
962 }),
963 );
956964 return;
957965 }
958966 const runId = payload["runId"];
src/crew/park_pull.ts +35 −6
@@ -13,6 +13,7 @@ import { closesFromBody, type Pull } from "../core/pulls.ts";
1313 import type { Repo } from "../core/repos.ts";
1414 import { getUserById } from "../core/users.ts";
1515 import { createLog } from "../kernel/log.ts";
16 import { triesChanged } from "./tries.ts";
1617 import { authorsInRepoPull } from "./authors_pull.ts";
1718 import {
1819 PARK_NOTE,
@@ -74,6 +75,7 @@ export function parkPull(
7475 repo: Repo,
7576 pull: Pull,
7677 why: string,
78 tries: readonly string[] = [],
7779 ): boolean {
7880 if (repo.autonomy !== "autonomous" || repo.autonomyBy === null) return false;
7981 if (getHold(app.db, pull.id) !== undefined) return false;
@@ -96,12 +98,15 @@ export function parkPull(
9698 by,
9799 repo,
98100 about,
99 strikeNote(
100 app.db,
101 repo,
102 about,
103 `${why} (pull #${pull.number} closed; its branch \`${pull.headBranch}\` at ${pull.headOid.slice(0, 12)} keeps the work so far).`,
104 PLANNER_HINT,
101 withTries(
102 strikeNote(
103 app.db,
104 repo,
105 about,
106 `${why} (pull #${pull.number} closed; its branch \`${pull.headBranch}\` at ${pull.headOid.slice(0, 12)} keeps the work so far).`,
107 PLANNER_HINT,
108 ),
109 tries,
105110 ),
106111 );
107112 }
@@ -112,3 +117,27 @@ export function parkPull(
112117 });
113118 return true;
114119 }
120
121 /**
122 * A note with a line per failed try after its reason's paragraph, so
123 * `parkReason` still reads only the reason. Only a pull that went red more
124 * than once has anything to tell.
125 */
126 function withTries(note: string, tries: readonly string[]): string {
127 return tries.length < 2 ? note : `${note}\n\n${tries.join("\n")}`;
128 }
129
130 /**
131 * `parkPull`, with the files each red head told in the issue's note: the
132 * heads after `since` (the last human word), the tries that counted.
133 */
134 export async function parkPullTold(
135 app: App,
136 repo: Repo,
137 pull: Pull,
138 why: string,
junipercrew outdated

since is "" here: pass the caller's lastHumanWord so tries before a human reset are not told.
139 since: string,
140 ): Promise<boolean> {
141 const tries = await triesChanged(app, repo, pull, since).catch(() => []);
142 return parkPull(app, repo, pull, why, tries);
143 }
src/crew/stale_reds.ts +2 −2
@@ -34,7 +34,7 @@ import { createLog } from "../kernel/log.ts";
3434 import { authorsInRepoPull } from "./authors_pull.ts";
3535 import { agentFor, CHECK_FAILED_LIMIT, lastHumanWord, queueAgentRun } from "./dispatch.ts";
3636 import { approvedOnHead } from "./finishing.ts";
37 import { parkPull } from "./park_pull.ts";
37 import { parkPullTold } from "./park_pull.ts";
3838 import { failingLine } from "./red_approved.ts";
3939
4040 const log = createLog("stale-reds");
@@ -161,7 +161,7 @@ export async function sweepStaleReds(
161161 const since = lastHumanWord(app, issue, agent);
162162 if (automaticRunsSince(app.db, issue.id, since) >= CHECK_FAILED_LIMIT) {
163163 const why = `the check stayed red through ${CHECK_FAILED_LIMIT} automatic tries: ${failingLine(red.run)}`;
164 if (parkPull(app, repo, pull, why)) moves.push({ pull: pull.number, did: "parked" });
164 if (await parkPullTold(app, repo, pull, why, since)) moves.push({ pull: pull.number, did: "parked" });
165165 continue;
166166 }
167167
src/crew/tries.ts +11 −5
@@ -14,9 +14,9 @@ import { listRuns } from "../core/runs.ts";
1414 import { runGitCapture } from "../git/env.ts";
1515
1616 const OID = /^[0-9a-f]{40,64}$/;
17 /** Tries told, newest kept: the reason is three lines with its first. */
18 const TRIES_TOLD = 2;
19 const FILES_PER_TRY = 4;
17 /** Tries told, newest kept. */
18 const TRIES_TOLD = 5;
19 const FILES_PER_TRY = 10;
2020 const LINE_MAX = 160;
2121
2222 /** `try 2: src/a.ts, src/b.ts (+3 more)`, one per line, oldest first; empty when none can be read. */
@@ -27,17 +27,23 @@ export async function triesChanged(
2727 since: string,
2828 ): Promise<string[]> {
2929 const heads: string[] = [];
30 let earlier: string | undefined;
3031 for (const run of listRuns(app.db, repo.id, { issueId: pull.id, limit: 50 })
3132 .reverse()) {
3233 if (run.kind !== "check" && run.kind !== "deploy") continue;
33 if (run.state !== "failed" || run.createdAt <= since) continue;
34 if (run.state !== "failed") continue;
3435 if (!OID.test(run.commitOid) || heads.includes(run.commitOid)) continue;
36 // A try a person has answered is not told, but the first told one changed only what came after it.
37 if (run.createdAt <= since) {
38 earlier = run.commitOid;
39 continue;
40 }
3541 heads.push(run.commitOid);
3642 }
3743 if (heads.length === 0) return [];
3844 const dir = await pullDir(app.db, app.reposRoot, repo, pull);
3945 const lines: string[] = [];
40 let before = pull.baseOid;
46 let before = earlier ?? pull.baseOid;
4147 for (const [n, head] of heads.entries()) {
4248 const out = OID.test(before)
4349 ? await runGitCapture(
test/checks-disk-red.test.ts +2 −1
@@ -5,6 +5,7 @@
55 * disk reading is plain data, injected.
66 */
77
8 import { pullCloses } from "./helpers/pull-closes.ts";
89 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
910 import type { App } from "../src/app.ts";
1011 import { announceRun } from "../src/actions/runs.ts";
@@ -149,7 +150,7 @@ describe("a red on a full check machine", () => {
149150 const before = cards()[0]!.sentence;
150151 const own = await spentPull();
151152 announceRun(app, failed(own, 300));
152 expect(open(own)).toBe(false);
153 await pullCloses(app, own);
153154 expect(cards()[0]!.sentence).toBe(before);
154155 });
155156 });
test/crew-failed-while-building.test.ts +6 −4
@@ -4,6 +4,7 @@
44 * decided again when the run ends.
55 */
66
7 import { pullCloses } from "./helpers/pull-closes.ts";
78 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
89 import type { App } from "../src/app.ts";
910 import { announceRun } from "../src/actions/runs.ts";
@@ -66,6 +67,7 @@ describe("a failure during the builder's run", () => {
6667 return finishRun(app.db, run.id, { state: "failed", summary: `${kind} failed` })!;
6768 };
6869 const open = (pull: Pull) => getIssueById(app.db, pull.id)?.state === "open";
70 const closes = (pull: Pull) => pullCloses(app, pull);
6971
7072 const building = (issueId: string): Run => {
7173 const run = createRun(app.db, { repoId: repo.id, kind: "agent", issueId, reason: "feedback" });
@@ -89,7 +91,7 @@ describe("a failure during the builder's run", () => {
8991 announceRun(app, failure(pull, "deploy"));
9092 expect(open(pull)).toBe(true);
9193 ends(run);
92 expect(open(pull)).toBe(false);
94 await closes(pull);
9395 });
9496
9597 test("a queued run holds it too, and the end of that run decides it", async () => {
@@ -99,7 +101,7 @@ describe("a failure during the builder's run", () => {
99101 expect(open(pull)).toBe(true);
100102 claimRun(app.db, queued.id);
101103 ends(queued);
102 expect(open(pull)).toBe(false);
104 await closes(pull);
103105 });
104106
105107 test("a run on the issue the pull closes holds it as well", async () => {
@@ -110,7 +112,7 @@ describe("a failure during the builder's run", () => {
110112 announceRun(app, failure(pull, "deploy"));
111113 expect(open(pull)).toBe(true);
112114 ends(run);
113 expect(open(pull)).toBe(false);
115 await closes(pull);
114116 });
115117
116118 test("a push during the run makes the failure history: the run's end decides nothing", async () => {
@@ -138,6 +140,6 @@ describe("a failure during the builder's run", () => {
138140 test("the same failure after the run ended closes it as before", async () => {
139141 const pull = await spentPull();
140142 announceRun(app, failure(pull, "deploy"));
141 expect(open(pull)).toBe(false);
143 await closes(pull);
142144 });
143145 });
test/crew-shared-red.test.ts +5 −3
@@ -8,6 +8,7 @@
88 * already spent its automatic tries, so any red that is its own parks it.
99 */
1010
11 import { pullCloses } from "./helpers/pull-closes.ts";
1112 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
1213 import type { App } from "../src/app.ts";
1314 import { announceRun } from "../src/actions/runs.ts";
@@ -92,6 +93,7 @@ describe("the crew, with a red many pulls share", () => {
9293 [`flake-${test}`, repo.id, test, new Date().toISOString()],
9394 );
9495 const open = (pull: Pull) => getIssueById(app.db, pull.id)?.state === "open";
96 const closes = (pull: Pull) => pullCloses(app, pull);
9597 const sharedCards = () =>
9698 vitals({
9799 db: app.db,
@@ -114,11 +116,11 @@ describe("the crew, with a red many pulls share", () => {
114116
115117 const own = await spentPull();
116118 announceRun(app, failed(own, ["Y"]));
117 expect(open(own)).toBe(false);
119 await closes(own);
118120 // X and Y together is not only the lane's red: Y is the pull's own.
119121 const mixed = await spentPull();
120122 announceRun(app, failed(mixed, ["X", "Y"]));
121 expect(open(mixed)).toBe(false);
123 await closes(mixed);
122124 expect(sharedCards()).toHaveLength(1);
123125
124126 // Main's next green check sends the three back through the lane, once.
@@ -133,7 +135,7 @@ describe("the crew, with a red many pulls share", () => {
133135 mainGreen();
134136 const pulls = [await spentPull(), await spentPull(), await spentPull()];
135137 for (const p of pulls) announceRun(app, failed(p, ["Z"]));
136 for (const p of pulls) expect(open(p)).toBe(false);
138 for (const p of pulls) await closes(p);
137139 expect(sharedCards().map((c) => c.sentence).filter((s) => s.includes('"Z"'))).toEqual([]);
138140 });
139141
test/crew-tries.test.ts +99 −5
@@ -16,6 +16,9 @@ import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
1616 import { createUser, type User } from "../src/core/users.ts";
1717 import { resolveOid } from "../src/git/merge.ts";
1818 import { repoDir } from "../src/git/repos.ts";
19 import { openIssue } from "../src/actions/issues.ts";
20 import { listComments } from "../src/core/issues.ts";
21 import { parkPullTold } from "../src/crew/park_pull.ts";
1922 import { triesChanged } from "../src/crew/tries.ts";
2023
2124 const at = (n: number) => `2026-01-01T00:00:${String(n).padStart(2, "0")}.000Z`;
@@ -141,8 +144,7 @@ describe("triesChanged", () => {
141144 run(p, h2, "failed", 2, "deploy");
142145 run(p, h3, "failed", 3);
143146 const all = await triesChanged(app, fresh(), p, EPOCH);
144 // Only the last TRIES_TOLD (2) lines come back, numbered by their place.
145 expect(all).toEqual(["try 2: src/b.ts", "try 3: src/a.ts, src/c.ts"]);
147 expect(all).toEqual(["try 1: first-" + branchN + ".txt, src/a.ts", "try 2: src/b.ts", "try 3: src/a.ts, src/c.ts"]);
146148
147149 // With one try the first line says the files since the base.
148150 const { pull: q, branch: qb } = await newPull();
@@ -164,7 +166,7 @@ describe("triesChanged", () => {
164166 run(p, red, "failed", 7);
165167 run(p, "not-an-oid", "failed", 8);
166168 run(p, "", "failed", 9);
167 expect(await triesChanged(app, fresh(), p, at(2))).toEqual([`try 1: first-${branchN}.txt, green.ts, old.ts, red.ts`]);
169 expect(await triesChanged(app, fresh(), p, at(2))).toEqual(["try 1: green.ts, red.ts"]);
168170 expect(await triesChanged(app, fresh(), p, at(9))).toEqual([]);
169171 });
170172
@@ -186,11 +188,11 @@ describe("triesChanged", () => {
186188 const { pull: p, branch } = await newPull();
187189 const many = await commit(
188190 branch,
189 ["a", "b", "c", "d", "e", "f"].map((n) => ({ path: `m/${n}.ts`, content: `${n}\n` })),
191 ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k"].map((n) => ({ path: `m/${n}.ts`, content: `${n}\n` })),
190192 );
191193 run(p, many, "failed", 1);
192194 expect(await triesChanged(app, fresh(), p, EPOCH)).toEqual([
193 `try 1: first-${branchN}.txt, m/a.ts, m/b.ts, m/c.ts (+3 more)`,
195 `try 1: first-${branchN}.txt, m/a.ts, m/b.ts, m/c.ts, m/d.ts, m/e.ts, m/f.ts, m/g.ts, m/h.ts, m/i.ts (+2 more)`,
194196 ]);
195197
196198 const long = "d".repeat(100);
@@ -216,4 +218,96 @@ describe("triesChanged", () => {
216218 expect(lines[0]).toContain("<img src=x onerror=alert(1)>");
217219 expect(lines[0]).not.toContain("&lt;");
218220 });
221
222 test("a park note on the issue tells each failed try's files when the pull went red twice", async () => {
223 app.db
224 .query("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2 WHERE id = ?1")
225 .run(repo.id, alice.id);
226 const issue = openIssue(app, alice, fresh(), { title: "Do it" });
227 if (!issue.ok) throw new Error(String(issue.error));
228 const branch = `feature/park-${++branchN}`;
229 const c = await commitToBranch(app, alice, fresh(), {
230 branch,
231 base: "main",
232 message: "First",
233 files: [{ path: "first.txt", content: "1\n" }],
234 });
235 if (!c.ok) throw new Error(JSON.stringify(c.error));
236 const opened = await openPull(app, alice, fresh(), {
237 title: "Park",
238 body: `Closes #${issue.value.number}`,
239 baseBranch: "main",
240 headBranch: branch,
241 });
242 if (!opened.ok) throw new Error(String(opened.error));
243 const p = opened.value;
244 const h1 = await commit(branch, [{ path: "src/a.ts", content: "a\n" }]);
245 const h2 = await commit(branch, [{ path: "src/b.ts", content: "b\n" }]);
246 run(p, h1, "failed", 1);
247 run(p, h2, "failed", 2);
248 // An agent's pull, so the crew may park it.
249 expect(await parkPullTold(app, fresh(), { ...p, authorIsAgent: true }, "the check stayed red", "")).toBe(true);
250 const note = listComments(app.db, issue.value.id).at(-1)!.body;
251 expect(note).toContain("try 1: first.txt, src/a.ts");
252 expect(note).toContain("try 2: src/b.ts");
253 });
254
255 test("tries from before the last human word are left out of the note", async () => {
256 app.db
257 .query("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2 WHERE id = ?1")
258 .run(repo.id, alice.id);
259 const issue = openIssue(app, alice, fresh(), { title: "Again" });
260 if (!issue.ok) throw new Error(String(issue.error));
261 const branch = `feature/since-${++branchN}`;
262 const c = await commitToBranch(app, alice, fresh(), {
263 branch, base: "main", message: "First", files: [{ path: "first.txt", content: "1\n" }],
264 });
265 if (!c.ok) throw new Error(JSON.stringify(c.error));
266 const opened = await openPull(app, alice, fresh(), {
267 title: "Since", body: `Closes #${issue.value.number}`, baseBranch: "main", headBranch: branch,
268 });
269 if (!opened.ok) throw new Error(String(opened.error));
270 const p = opened.value;
271 const old = await commit(branch, [{ path: "src/old.ts", content: "o\n" }]);
272 const h1 = await commit(branch, [{ path: "src/new1.ts", content: "a\n" }]);
273 const h2 = await commit(branch, [{ path: "src/new2.ts", content: "b\n" }]);
274 run(p, old, "failed", 1);
275 run(p, h1, "failed", 20);
276 run(p, h2, "failed", 21);
277 const told = await parkPullTold(app, fresh(), { ...p, authorIsAgent: true }, "red", at(10));
278 expect(told).toBe(true);
279 const note = listComments(app.db, issue.value.id).at(-1)!.body;
280 expect(note).toContain("src/new1.ts");
281 expect(note).toContain("src/new2.ts");
282 expect(note).not.toContain("src/old.ts");
283 });
284
285 test("two parks racing write one note", async () => {
286 app.db
287 .query("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2 WHERE id = ?1")
288 .run(repo.id, alice.id);
289 const issue = openIssue(app, alice, fresh(), { title: "Race" });
290 if (!issue.ok) throw new Error(String(issue.error));
291 const branch = `feature/race-${++branchN}`;
292 const c = await commitToBranch(app, alice, fresh(), {
293 branch, base: "main", message: "First", files: [{ path: "first.txt", content: "1\n" }],
294 });
295 if (!c.ok) throw new Error(JSON.stringify(c.error));
296 const opened = await openPull(app, alice, fresh(), {
297 title: "Race", body: `Closes #${issue.value.number}`, baseBranch: "main", headBranch: branch,
298 });
299 if (!opened.ok) throw new Error(String(opened.error));
300 const p = { ...opened.value, authorIsAgent: true };
301 const h1 = await commit(branch, [{ path: "src/a.ts", content: "a\n" }]);
302 const h2 = await commit(branch, [{ path: "src/b.ts", content: "b\n" }]);
303 run(p, h1, "failed", 1);
304 run(p, h2, "failed", 2);
305 const results = await Promise.all([
306 parkPullTold(app, fresh(), p, "red", ""),
307 parkPullTold(app, fresh(), p, "red", ""),
308 ]);
309 expect(results.filter(Boolean)).toHaveLength(1);
310 const notes = listComments(app.db, issue.value.id).filter((x) => x.body.includes("try 1:"));
311 expect(notes).toHaveLength(1);
312 });
219313 });
test/helpers/pull-closes.ts added +10
@@ -0,0 +1,10 @@
1 /** Waits for a pull's issue to close: parking reads git history, so it lands just after the announcement. */
2 import { expect } from "bun:test";
3 import type { App } from "../../src/app.ts";
4 import { getIssueById } from "../../src/core/issues.ts";
5
6 export async function pullCloses(app: App, pull: { id: string }): Promise<void> {
7 const open = () => getIssueById(app.db, pull.id)?.state === "open";
8 for (let i = 0; i < 1000 && open(); i++) await Bun.sleep(2);
9 expect(open()).toBe(false);
10 }

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs