trevato/forge

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

More
Forks1g fMapg aRuns15g rEventsFlows

Retirement issues are checked against main before they are handed out: one whose class or rule is already gone is closed with the commit that removed it #5985

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/5983-retirement-issues-are-checked-against-ma → main 1 commit fork

  • Merged. scout/5983-retirement-issues-are-checked-against-ma was merged into main as b69536c.

Review

Sign in to review this change.
3 files changed +217 −2
src/crew/autonomy.ts +38 −2
@@ -99,8 +99,10 @@ import { given, pendingReplans } from "../core/limb_replan.ts";
9999 import { handedOverPulls, SKIP_SUMMARY, skipsReplan } from "./guard.ts";
100100 import { atRunCap, type Held, heldBy, heldWords, replannable } from "./held.ts";
101101 import { NO_PULL, shunnedBy, stallReason, unstickOwed } from "./stalls.ts";
102 import { BY_CREW, DONE_LIKE } from "../core/done_on_main.ts";
102 import { alreadyDone, BY_CREW, DONE_LIKE } from "../core/done_on_main.ts";
103 import { repoDir } from "../git/repos.ts";
103104 import { ALREADY_NOTE, alreadyDoneOids } from "./already.ts";
105 import { removedBy, retiredNames } from "./retired.ts";
104106 import { interruptionOf } from "./interrupted.ts";
105107 import {
106108 PARK_NOTE,
@@ -388,7 +390,13 @@ export async function driveRepo(
388390 const shunned = shunnedBy(app.db, repo, issue.id, now);
389391 return crew.filter((c) => c.role !== "builder" || !shunned.has(c.agent.user.id));
390392 };
391 const pickable = ready.filter((r) => crewFor(r.issue).some((c) => c.role === "builder"));
393 const pickable = (
394 await withoutRetired(
395 app,
396 repo,
397 ready.filter((r) => crewFor(r.issue).some((c) => c.role === "builder")),
398 )
399 );
392400 const next = await nextOf(app, repo, pickable);
393401 if (next !== undefined) {
394402 const routed = await routeIssue(app, repo, next.issue, crewFor(next.issue), inFlight);
@@ -1367,6 +1375,34 @@ export function readyState(
13671375 );
13681376 }
13691377
1378 /**
1379 * The ready issues, less those that retire names already gone from the base
1380 * (`retired.ts`): each gets the already-done note, once, and is not handed out.
1381 */
1382 async function withoutRetired(
1383 app: App,
1384 repo: Repo,
1385 ready: readonly Ranked[],
1386 ): Promise<Ranked[]> {
1387 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
1388 const kept: Ranked[] = [];
1389 for (const r of ready) {
1390 const names = retiredNames(r.issue);
1391 if (names.length === 0) {
1392 kept.push(r);
1393 continue;
1394 }
1395 const oids = await removedBy(dir, repo.defaultBranch, names);
1396 if (oids.length === 0) {
1397 kept.push(r);
1398 continue;
1399 }
1400 if (!alreadyDone(app.db, [r.issue.id]).has(r.issue.id))
1401 noteAlreadyDone(app, repo, r.issue, oids);
1402 }
1403 return kept;
1404 }
1405
13701406 async function nextOf(
13711407 app: App,
13721408 repo: Repo,
src/crew/retired.ts added +60
@@ -0,0 +1,60 @@
1 /**
2 * A retirement issue already carried out. An issue whose title says a class
3 * or rule "leaves the stylesheet" or "leaves §5d" can be handed out after
4 * the work landed (#4691, #4616): the run then closes with nothing to
5 * change. Before the picker hands one out, the names in it are looked for
6 * on the base branch; when none is left, the crew writes the already-done
7 * note with the commit that last touched them instead.
8 *
9 * Names are the backticked `.class` tokens of the title and body, nothing
10 * else; an issue without them is never judged. Docs and tests may keep
11 * saying a name after it is gone from the code, so they are not searched.
12 */
13
14 import { runGit } from "../git/env.ts";
15
16 const RETIRES = /\bleaves?\s+(?:the\s+stylesheet|§\s*5d)/i;
17 const NAME = /`(\.[A-Za-z_][\w-]*)`/g;
18
19 /** The most names one issue is checked for, and commits it is answered with. */
20 const MOST = 20;
21 const COMMITS = 5;
22
23 /** The `.class` names a retirement issue lists; empty when it is not one. */
24 export function retiredNames(issue: { title: string; body: string }): string[] {
25 if (!RETIRES.test(issue.title)) return [];
26 const found = new Set<string>();
27 for (const m of `${issue.title}\n${issue.body}`.matchAll(NAME)) found.add(m[1]!);
28 return [...found].slice(0, MOST);
29 }
30
31 const escape = (s: string): string => s.replace(/[.\\^$*+?()[\]{}|]/g, "\\$&");
32
33 /**
34 * The commits that last touched these names, when none of them is left on
35 * `branch` in the bare repository at `dir`. Empty when any name remains,
36 * when git cannot say, or when no commit is found to name.
37 */
38 export async function removedBy(
39 dir: string,
40 branch: string,
41 names: readonly string[],
42 ): Promise<string[]> {
43 if (names.length === 0) return [];
44 const ref = `refs/heads/${branch}`;
45 for (const name of names) {
46 const left = await runGit(
47 ["grep", "-q", "-E", "-e", `${escape(name)}([^A-Za-z0-9_-]|$)`, ref, "--", ".", ":!docs", ":!test"],
48 { cwd: dir },
49 );
50 // Exit 1 is "no match"; any other failure is "cannot say".
51 if (left.ok || left.error.code !== 1) return [];
52 }
53 const oids: string[] = [];
54 for (const name of names) {
55 const last = await runGit(["log", "-n", "1", "--format=%H", `-S${name}`, ref], { cwd: dir });
56 if (last.ok && /^[0-9a-f]{40}$/.test(last.value) && !oids.includes(last.value))
57 oids.push(last.value);
58 }
59 return oids.slice(0, COMMITS);
60 }
test/crew-retired.test.ts added +119
@@ -0,0 +1,119 @@
1 /**
2 * A retirement issue is checked against the base before it is handed out:
3 * one whose classes are gone gets the already-done note and is skipped, one
4 * whose classes remain is handed out, and one naming none is handed out.
5 */
6 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
7 import { mkdtemp, rm } from "node:fs/promises";
8 import { tmpdir } from "node:os";
9 import { join } from "node:path";
10 import { createApp, type App } from "../src/app.ts";
11 import { addToCrew, createAgent } from "../src/actions/crew.ts";
12 import { openIssue, setIssueState } from "../src/actions/issues.ts";
13 import { commitToBranch, createRepo, updateRepoSettings } from "../src/actions/repos.ts";
14 import { alreadyDone } from "../src/core/done_on_main.ts";
15 import { getIssueById, type Issue } from "../src/core/issues.ts";
16 import { getRepoById, type Repo } from "../src/core/repos.ts";
17 import { createUser, type User } from "../src/core/users.ts";
18 import { driveRepo } from "../src/crew/autonomy.ts";
19 import { retiredNames } from "../src/crew/retired.ts";
20 import { resolveOid } from "../src/git/merge.ts";
21 import { repoDir } from "../src/git/repos.ts";
22
23 describe("retiredNames", () => {
24 test("only backticked .class tokens of a retirement title", () => {
25 const body = "Done when `.a` and `.b-c` go; not `plain` or `x.y`.";
26 expect(retiredNames({ title: "`.a` leaves the stylesheet", body })).toEqual([".a", ".b-c"]);
27 expect(retiredNames({ title: "`.a` leaves §5d", body: "" })).toEqual([".a"]);
28 expect(retiredNames({ title: "Add `.a`", body })).toEqual([]);
29 expect(retiredNames({ title: "Rules leave the stylesheet", body: "none" })).toEqual([]);
30 });
31 });
32
33 describe("the picker and retirement issues", () => {
34 let dataDir: string;
35 let app: App;
36 let alice: User;
37 let repo: Repo;
38 const fresh = () => getRepoById(app.db, repo.id)!;
39 let removal = "";
40
41 const ask = (title: string, body = ""): Issue => {
42 const o = openIssue(app, alice, fresh(), { title, body });
43 if (!o.ok) throw new Error(String(o.error));
44 return o.value;
45 };
46 const drop = (issue: Issue) =>
47 setIssueState(app, alice, fresh(), getIssueById(app.db, issue.id)!, "closed", { reason: "dropped" });
48
49 beforeAll(async () => {
50 dataDir = await mkdtemp(join(tmpdir(), "forge-retired-"));
51 app = createApp({ dataDir, port: 0 });
52 const a = await createUser(app.db, {
53 origin: "seed", trusted: true, username: "alice", email: "[email protected]",
54 password: "a-good-passphrase", adminIfFirst: true,
55 });
56 if (!a.ok) throw new Error(a.error);
57 alice = a.value;
58 const made = await createRepo(app, alice, { name: "styles", visibility: "private" });
59 if (!made.ok) throw new Error(String(made.error));
60 repo = made.value;
61 const first = await commitToBranch(app, alice, repo, {
62 message: "Start",
63 files: [{ path: "src/app.css", content: ".old { color: red }\n.keep { color: blue }\n" }],
64 });
65 if (!first.ok) throw new Error(JSON.stringify(first.error));
66 const second = await commitToBranch(app, alice, repo, {
67 message: "Retire .old",
68 files: [{ path: "src/app.css", content: ".keep { color: blue }\n" }],
69 });
70 if (!second.ok) throw new Error(JSON.stringify(second.error));
71 removal = (await resolveOid(repoDir(app.reposRoot, repo.ownerName, repo.name), `refs/heads/${repo.defaultBranch}`))!;
72 const agent = await createAgent(app, alice, { username: "mason" });
73 if (!agent.ok) throw new Error(String(agent.error));
74 expect(addToCrew(app, alice, repo, "mason").ok).toBe(true);
75 const on = await updateRepoSettings(app, alice, repo, {
76 autonomy: "autonomous",
77 autonomyDirection: "Retire the old rules.",
78 autonomyCeilingUsd: 10,
79 autonomySlots: 1,
80 autonomyBy: alice.id,
81 });
82 expect(on.ok).toBe(true);
83 });
84
85 afterAll(async () => {
86 app.close();
87 await rm(dataDir, { recursive: true, force: true });
88 });
89
90 test("classes already gone: the note names the removing commit and nothing is handed out", async () => {
91 const gone = ask("`.old` leaves the stylesheet", "Done when `.old` is gone.");
92 expect(await driveRepo(app, fresh())).not.toBe("picked");
93 expect(getIssueById(app.db, gone.id)!.assigneeId).toBeNull();
94 expect(alreadyDone(app.db, [gone.id]).get(gone.id)).toEqual([removal]);
95 // Said once, however many ticks follow.
96 await driveRepo(app, fresh());
97 const notes = app.db
98 .query<{ n: number }, [string]>("SELECT COUNT(*) AS n FROM comment WHERE issue_id = ?1 AND body LIKE 'Done on main already%'")
99 .get(gone.id)!;
100 expect(notes.n).toBe(1);
101 drop(gone);
102 });
103
104 test("classes that remain: the issue is handed out", async () => {
105 const kept = ask("`.keep` leaves the stylesheet", "Done when `.keep` is gone.");
106 expect(await driveRepo(app, fresh())).toBe("picked");
107 expect(getIssueById(app.db, kept.id)!.assigneeId).not.toBeNull();
108 expect(alreadyDone(app.db, [kept.id]).size).toBe(0);
109 });
110
111 test("no names: the issue is handed out as before", async () => {
112 const kept = getIssueById(app.db, (await Promise.resolve(ask("Old rules leave the stylesheet", "No names here."))).id)!;
113 // The previous test's issue holds the one slot; free it first.
114 app.db.query("UPDATE issue SET assignee_id = NULL WHERE assignee_id IS NOT NULL").run();
115 app.db.query("UPDATE issue SET state = 'closed' WHERE id != ?1").run(kept.id);
116 expect(await driveRepo(app, fresh())).toBe("picked");
117 expect(getIssueById(app.db, kept.id)!.assigneeId).not.toBeNull();
118 });
119 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs