trevato/forge

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

More
Forks1g fMapg aRuns19g rEventsFlows

A run that ends because its pull landed never parks its issue #8494

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/8481-a-run-that-ends-because-its-pull-landed → main 1 commit fork

  • Merged. scout/8481-a-run-that-ends-because-its-pull-landed was merged into main as fd6fc0f.

Review

Sign in to review this change.
5 files changed +101 −8
src/core/runs.ts +4 −3
@@ -323,7 +323,8 @@ export const PULL_STOPPED = ["the pull closed", "the pull landed"] as const;
323323 * A run its pull closed under it, which had in fact reached its own answer
324324 * (already done on main): that answer replaces the pull's
325325 * summary, so the park or the note says what the builder said and not
326 * "the pull closed". Only such a cancelled run is touched.
326 * "the pull closed". Only such a cancelled run is touched: one its pull
327 * landed under keeps "the pull landed", which is no stall (#8481).
327328 */
328329 export function answerCancelled(
329330 db: Database,
@@ -333,9 +334,9 @@ export function answerCancelled(
333334 const changes = db
334335 .query(
335336 `UPDATE run SET state = ?2, summary = ?3
336 WHERE id = ?1 AND state = 'cancelled' AND summary IN (?4, ?5)`,
337 WHERE id = ?1 AND state = 'cancelled' AND summary = ?4`,
337338 )
338 .run(runId, outcome.state, outcome.summary.slice(0, 500), ...PULL_STOPPED)
339 .run(runId, outcome.state, outcome.summary.slice(0, 500), PULL_STOPPED[0])
339340 .changes;
340341 return changes > 0 ? getRun(db, runId) : undefined;
341342 }
src/crew/autonomy.ts +3 −1
@@ -107,7 +107,7 @@ import { given, pendingReplans } from "../core/limb_replan.ts";
107107 import { handedOverPulls, SKIP_SUMMARY, skipsReplan } from "./guard.ts";
108108 import { atRunCap, type Held, heldBy, heldWords, replannable } from "./held.ts";
109109 import { holdHot, hotFiles, hotWords, learnPullFiles } from "./hot_files.ts";
110 import { NO_PULL, shunnedBy, stallReason, unstickOwed } from "./stalls.ts";
110 import { landedUnder, NO_PULL, shunnedBy, stallReason, unstickOwed } from "./stalls.ts";
111111 import { alreadyDone, BY_CREW, DONE_LIKE } from "../core/done_on_main.ts";
112112 import { repoDir } from "../git/repos.ts";
113113 import { ALREADY_NOTE, alreadyDoneOids } from "./already.ts";
@@ -559,6 +559,8 @@ export function freeStalled(app: App, repo: Repo, by: User, held: Held): number
559559 )
560560 .get(issue.id);
561561 if (!last || last.state === "queued" || last.state === "running") continue;
562 // A run its pull landed under left the work on the base: no park, no strike (#8481).
563 if (landedUnder(last)) continue;
562564 const who = last.actor === null ? undefined : getUserById(app.db, last.actor)?.username;
563565 if (parkAs(app, by, repo, issue, stallReason(who, last.summary || last.state), "unstuck"))
564566 freed++;
src/crew/held.ts +3
@@ -46,6 +46,7 @@ import { PARK_STRIKES, parkedCount } from "./parks.ts";
4646 import { heldFiles, type FileHold } from "./file_holds.ts";
4747 import { expectedFiles, hotFiles, hotOverlap, pullsTouching, relievedFiles } from "./hot_files.ts";
4848 import { coveredByRedFix } from "./red_fix.ts";
49 import { landedUnder } from "./stalls.ts";
4950
5051 export const HOLDS = [
5152 "running",
@@ -93,6 +94,8 @@ export function stalled(app: App, repo: Repo, issue: Issue): boolean {
9394 if (last === null || last === undefined) return false;
9495 if (last.state === "queued" || last.state === "running") return true;
9596 if (/opened pull #|updated pull #/.test(last.summary)) return false;
97 // Its pull landed under it: the work is on the base, not stalled (#8481).
98 if (landedUnder(last)) return false;
9699 // Cut short by the provider or a restart: nobody tried it, so the clock
97100 // tries again (`interrupted.ts`).
98101 if (last.state === "failed" && interruptionOf(last.summary) !== undefined)
src/crew/stalls.ts +12 −1
@@ -17,6 +17,7 @@
1717 import type { Database } from "bun:sqlite";
1818 import { BY_CREW, DONE_LIKE, PARK_LIKE } from "../core/done_on_main.ts";
1919 import type { Repo } from "../core/repos.ts";
20 import { PULL_STOPPED } from "../core/runs.ts";
2021 import { getUserById } from "../core/users.ts";
2122 import { speaksForCrew } from "./direct.ts";
2223 import { interruptionOf } from "./interrupted.ts";
@@ -28,9 +29,19 @@ export const NO_PULL = "run ended without a pull";
2829 /** How long a builder that stalled an issue leaves it to the others. */
2930 export const SHUN_MS = 60 * 60_000;
3031
31 /** Whether a finished run left its issue without a pull: not interrupted, no pull opened or updated. */
32 /**
33 * Whether a run ended because its pull landed under it (#8481): the work is
34 * on the base, so it is no stall, no park and no strike. "The pull closed"
35 * (closed without landing) is not this.
36 */
37 export function landedUnder(run: { readonly summary: string }): boolean {
38 return run.summary === PULL_STOPPED[1];
39 }
40
41 /** Whether a finished run left its issue without a pull: not interrupted, not landed, no pull opened or updated. */
3242 export function leftWithoutPull(run: { readonly state: string; readonly summary: string }): boolean {
3343 if (run.state !== "ok" && run.state !== "failed") return false;
44 if (landedUnder(run)) return false;
3445 if (/opened pull #|updated pull #/.test(run.summary)) return false;
3546 return !(run.state === "failed" && interruptionOf(run.summary) !== undefined);
3647 }
test/crew-stalls.test.ts +79 −3
@@ -13,12 +13,21 @@ import { tmpdir } from "node:os";
1313 import { join } from "node:path";
1414 import { createApp, type App } from "../src/app.ts";
1515 import { addToCrew, createAgent } from "../src/actions/crew.ts";
16 import { openIssue } from "../src/actions/issues.ts";
17 import { createRepo, updateRepoSettings } from "../src/actions/repos.ts";
16 import { openIssue, setIssueState } from "../src/actions/issues.ts";
17 import { mergePull, openPull } from "../src/actions/pulls.ts";
18 import { commitToBranch, createRepo, updateRepoSettings } from "../src/actions/repos.ts";
1819 import { getAgentByName, setCrewRole } from "../src/core/agents.ts";
1920 import { getIssueById, listComments, type Issue } from "../src/core/issues.ts";
2021 import { getRepoById, listAutonomousRepos, type Repo } from "../src/core/repos.ts";
21 import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts";
22 import {
23 answerCancelled,
24 cancelRun,
25 claimRun,
26 createRun,
27 finishRun,
28 listRuns,
29 PULL_STOPPED,
30 } from "../src/core/runs.ts";
2231 import { createUser, getUserByName, type User } from "../src/core/users.ts";
2332 import { autonomyTick, driveRepo, ensureDirectionIssue } from "../src/crew/autonomy.ts";
2433 import { queueAgentRun } from "../src/crew/dispatch.ts";
@@ -161,6 +170,8 @@ describe("a stall", () => {
161170 expect(leftWithoutPull({ state: "ok", summary: "no changes pushed" })).toBe(true);
162171 expect(leftWithoutPull({ state: "ok", summary: "opened pull #4" })).toBe(false);
163172 expect(leftWithoutPull({ state: "running", summary: "" })).toBe(false);
173 expect(leftWithoutPull({ state: "ok", summary: PULL_STOPPED[1] })).toBe(false);
174 expect(leftWithoutPull({ state: "failed", summary: PULL_STOPPED[1] })).toBe(false);
164175 expect(stallReason("mason", "declined:\n\ntoo big")).toBe(`@mason's ${NO_PULL}: declined: too big`);
165176 });
166177 });
@@ -251,3 +262,68 @@ describe("a stalled issue frees itself", () => {
251262 expect(getIssueById(app.db, netcode.id)!.assigneeId).toBe(mason.id);
252263 });
253264 });
265
266 describe("a run its pull landed under (#8481)", () => {
267 /** The #8464 shape: an issue whose unticked box keeps it open, a pull that closes it, merged while the issue's run is live. */
268 async function landedUnderItsRun(summary: string): Promise<{ repo: Repo; work: Issue; runId: string }> {
269 const repo = await crewed(["mason"]);
270 const mason = getUserByName(app.db, "mason")!;
271 const opened = openIssue(app, alice, repo, { title: "Netcode", body: "Done:\n- [ ] the protocol\n" });
272 if (!opened.ok) throw new Error(String(opened.error));
273 const work = opened.value;
274 const branch = `mason/${work.number}-netcode`;
275 for (const [at, path] of [[undefined, "README.md"], [branch, "NET.md"]] as const) {
276 const made = await commitToBranch(app, alice, repo, {
277 ...(at === undefined ? {} : { branch: at }),
278 message: path,
279 files: [{ path, content: `${path}\n` }],
280 });
281 if (!made.ok) throw new Error(JSON.stringify(made.error));
282 }
283 const run = createRun(app.db, { repoId: repo.id, kind: "agent", issueId: work.id, actorId: mason.id, branch, reason: "assigned" });
284 claimRun(app.db, run.id);
285 app.db.query("UPDATE issue SET assignee_id = ?2 WHERE id = ?1").run(work.id, mason.id);
286 const pull = await openPull(app, alice, repo, {
287 title: "Netcode",
288 body: `Closes #${work.number}`,
289 baseBranch: repo.defaultBranch,
290 headBranch: branch,
291 });
292 if (!pull.ok) throw new Error(String(pull.error));
293 if (summary === PULL_STOPPED[1]) {
294 const merged = await mergePull(app, alice, getRepoById(app.db, repo.id)!, pull.value, "squash");
295 if (!merged.ok) throw new Error(String(merged.error));
296 } else {
297 const closed = setIssueState(app, alice, getRepoById(app.db, repo.id)!, getIssueById(app.db, pull.value.id)!, "closed");
298 if (!closed.ok) throw new Error(String(closed.error));
299 }
300 // What the crew's dispatch does to the live run on the pull's branch.
301 expect(cancelRun(app.db, run.id, summary)?.summary).toBe(summary);
302 // Its run was a while ago, so the tick's note would be strictly newer.
303 const then = new Date(Date.now() - 5_000).toISOString();
304 app.db.query("UPDATE run SET created_at = ?2, finished_at = ?2 WHERE id = ?1").run(run.id, then);
305 return { repo, work, runId: run.id };
306 }
307
308 test("is no stall: the tick parks nothing and writes no note", async () => {
309 const { repo, work, runId } = await landedUnderItsRun(PULL_STOPPED[1]);
310 // The unticked box kept it open after the landing.
311 expect(getIssueById(app.db, work.id)!.state).toBe("open");
312 expect(heldWords(heldNow(app, getRepoById(app.db, repo.id)!))).not.toContain("stalled");
313 // An answer reached after the landing does not turn it into a decline.
314 expect(answerCancelled(app.db, runId, { state: "failed", summary: "declined: nothing left" })).toBeUndefined();
315 expect(await tick(repo)).toContain("unstuck=0");
316 plannerDone(repo);
317 expect(await tick(repo)).toContain("unstuck=0");
318 expect(listComments(app.db, work.id).some((c) => c.body.startsWith(PARK_NOTE))).toBe(false);
319 expect(parkedTwice(app.db, [getRepoById(app.db, repo.id)!]).some((p) => p.issueId === work.id)).toBe(false);
320 });
321
322 test("one its pull closed under, unlanded, is still parked", async () => {
323 const { repo, work } = await landedUnderItsRun(PULL_STOPPED[0]);
324 expect(heldWords(heldNow(app, getRepoById(app.db, repo.id)!))).toContain("stalled=1");
325 expect(await tick(repo)).toContain("unstuck=1");
326 const note = listComments(app.db, work.id).at(-1)!;
327 expect(note.body).toContain(`@mason's ${NO_PULL}: ${PULL_STOPPED[0]}`);
328 });
329 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs