Public: anyone can read this; the crew can change it.
Reassigning an issue does not release the builder that held it: a person who takes over, or a second agent, races the first #419
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/413-reassigning-an-issue-does-not-release-th → main 1 commit fork
2 files
changed +148
−7
src/crew/dispatch.ts +53 −6
| @@ -16,6 +16,7 @@ import type { ForgeEvent } from "../core/events.ts"; | ||
| 16 | 16 | import { |
| 17 | 17 | getIssue, |
| 18 | 18 | getIssueById, |
| 19 | addComment as insertComment, | |
| 19 | 20 | listComments, |
| 20 | 21 | type Issue, |
| 21 | 22 | } from "../core/issues.ts"; |
| @@ -254,6 +255,49 @@ export function queueAgentRun( | ||
| 254 | 255 | return run; |
| 255 | 256 | } |
| 256 | 257 | |
| 258 | /** | |
| 259 | * An issue belongs to one builder: when it is handed to a person, to | |
| 260 | * nobody, or to another agent, the runs of everyone else on it end — a | |
| 261 | * queued one is cancelled, a running one is aborted — and one line on the | |
| 262 | * issue says who took it. | |
| 263 | */ | |
| 264 | function releaseOtherRuns( | |
| 265 | app: App, | |
| 266 | repo: Repo, | |
| 267 | issue: Issue, | |
| 268 | actorId: string | null, | |
| 269 | ): void { | |
| 270 | const released: Run[] = []; | |
| 271 | for (const live of listActive(app.db, "agent")) { | |
| 272 | if (live.issueId !== issue.id || live.actorId === issue.assigneeId) | |
| 273 | continue; | |
| 274 | const cancelled = cancelRun( | |
| 275 | app.db, | |
| 276 | live.id, | |
| 277 | "the issue was reassigned; this run no longer holds it", | |
| 278 | ); | |
| 279 | if (!cancelled) continue; | |
| 280 | announceRun(app, cancelled); | |
| 281 | if (live.state === "running") abortAgentRun(live.id); | |
| 282 | released.push(cancelled); | |
| 283 | } | |
| 284 | if (released.length === 0 || actorId === null) return; | |
| 285 | const holder = | |
| 286 | issue.assigneeId === null | |
| 287 | ? undefined | |
| 288 | : getUserById(app.db, issue.assigneeId); | |
| 289 | const who = holder ? `@${holder.username}` : "nobody"; | |
| 290 | const stopped = released | |
| 291 | .map((r) => (r.actorId ? getUserById(app.db, r.actorId) : undefined)) | |
| 292 | .filter((u) => u !== undefined) | |
| 293 | .map((u) => `@${u.username}`); | |
| 294 | insertComment(app.db, { | |
| 295 | issueId: issue.id, | |
| 296 | authorId: actorId, | |
| 297 | body: `Taken over by ${who}; stopped ${[...new Set(stopped)].join(", ") || "the earlier run"}.`, | |
| 298 | }); | |
| 299 | } | |
| 300 | ||
| 257 | 301 | function route(app: App, event: ForgeEvent): void { |
| 258 | 302 | if (event.repoId === null) return; |
| 259 | 303 | const number = event.payload["number"]; |
| @@ -263,12 +307,15 @@ function route(app: App, event: ForgeEvent): void { | ||
| 263 | 307 | switch (event.kind) { |
| 264 | 308 | case "issue.assigned": |
| 265 | 309 | case "pull.assigned": { |
| 266 | if (event.payload["agent"] !== true || typeof number !== "number") return; | |
| 310 | if (typeof number !== "number") return; | |
| 267 | 311 | const issue = getIssue(app.db, repo.id, number); |
| 268 | const agent = issue?.assigneeId | |
| 312 | if (!issue) return; | |
| 313 | releaseOtherRuns(app, repo, issue, event.actorId); | |
| 314 | if (event.payload["agent"] !== true) return; | |
| 315 | const agent = issue.assigneeId | |
| 269 | 316 | ? getAgent(app.db, issue.assigneeId) |
| 270 | 317 | : undefined; |
| 271 | if (!issue || !agent || issue.state !== "open") return; | |
| 318 | if (!agent || issue.state !== "open") return; | |
| 272 | 319 | queueAgentRun( |
| 273 | 320 | app, |
| 274 | 321 | repo, |
| @@ -562,9 +609,7 @@ function parkedReason( | ||
| 562 | 609 | const what = kind === "deploy" ? "the preview's deploy" : "the check"; |
| 563 | 610 | if (judged.cause !== "environment") { |
| 564 | 611 | const named = |
| 565 | judged.failingTest === undefined | |
| 566 | ? "" | |
| 567 | : ` ("${judged.failingTest}")`; | |
| 612 | judged.failingTest === undefined ? "" : ` ("${judged.failingTest}")`; | |
| 568 | 613 | return `${what} failed${named} and the decider saw no point in another try`; |
| 569 | 614 | } |
| 570 | 615 | return retriedChecks.has(head) |
| @@ -576,6 +621,8 @@ export function agentFor(app: App, issue: Issue): Agent | undefined { | ||
| 576 | 621 | if (issue.assigneeId !== null) { |
| 577 | 622 | const assigned = getAgent(app.db, issue.assigneeId); |
| 578 | 623 | if (assigned) return assigned; |
| 624 | // A person holds it: nothing the agent's name is on starts a run. | |
| 625 | return undefined; | |
| 579 | 626 | } |
| 580 | 627 | return issue.kind === "pull" ? getAgent(app.db, issue.authorId) : undefined; |
| 581 | 628 | } |
test/crew-interrupted.test.ts +95 −1
| @@ -4,7 +4,8 @@ import { mkdtemp, rm } from "node:fs/promises"; | ||
| 4 | 4 | import { tmpdir } from "node:os"; |
| 5 | 5 | import { join } from "node:path"; |
| 6 | 6 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; |
| 7 | import { openIssue } from "../src/actions/issues.ts"; | |
| 7 | import { addComment, assignIssue, openIssue } from "../src/actions/issues.ts"; | |
| 8 | import { listComments } from "../src/core/issues.ts"; | |
| 8 | 9 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| 9 | 10 | import { getRepo } from "../src/core/repos.ts"; |
| 10 | 11 | import { createApp, type App } from "../src/app.ts"; |
| @@ -294,3 +295,96 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Done while | ||
| 294 | 295 | ).toHaveLength(0); |
| 295 | 296 | }, 40_000); |
| 296 | 297 | }); |
| 298 | ||
| 299 | describe("an issue handed to someone else", () => { | |
| 300 | let dataDir: string; | |
| 301 | let app: App; | |
| 302 | let servers: Servers; | |
| 303 | let crew: CrewHandle; | |
| 304 | let alice: User; | |
| 305 | let bot: User; | |
| 306 | let other: User; | |
| 307 | ||
| 308 | beforeAll(async () => { | |
| 309 | dataDir = await mkdtemp(join(tmpdir(), "forge-reassign-")); | |
| 310 | app = createApp({ dataDir, port: 0 }); | |
| 311 | servers = await serve(app); | |
| 312 | const a = await createUser(app.db, { | |
| 313 | username: "alice", | |
| 314 | email: "[email protected]", | |
| 315 | password: "a-good-passphrase", | |
| 316 | adminIfFirst: true, | |
| 317 | }); | |
| 318 | if (!a.ok) throw new Error(a.error); | |
| 319 | alice = a.value; | |
| 320 | const one = await createAgent(app, alice, { username: "bot" }); | |
| 321 | const two = await createAgent(app, alice, { username: "other" }); | |
| 322 | if (!one.ok || !two.ok) throw new Error("agents"); | |
| 323 | bot = one.value.user; | |
| 324 | other = two.value.user; | |
| 325 | crew = await startCrew(app, { | |
| 326 | forgeUrl: servers.url, | |
| 327 | localUrl: servers.url, | |
| 328 | binary: join(dataDir, "no-such-claude"), | |
| 329 | concurrency: 0, | |
| 330 | budgetUsd: 1, | |
| 331 | runTimeoutMs: 60_000, | |
| 332 | }); | |
| 333 | }); | |
| 334 | ||
| 335 | afterAll(async () => { | |
| 336 | await crew.stop(); | |
| 337 | servers.stop(); | |
| 338 | app.close(); | |
| 339 | await rm(dataDir, { recursive: true, force: true }); | |
| 340 | }); | |
| 341 | ||
| 342 | async function setup(name: string) { | |
| 343 | const made = await createRepo(app, alice, { name, visibility: "private" }); | |
| 344 | if (!made.ok) throw new Error(String(made.error)); | |
| 345 | const repo = made.value; | |
| 346 | addToCrew(app, alice, repo, "bot"); | |
| 347 | addToCrew(app, alice, repo, "other"); | |
| 348 | const issued = openIssue(app, alice, repo, { title: "Work", body: "" }); | |
| 349 | if (!issued.ok) throw new Error(String(issued.error)); | |
| 350 | return { repo, issue: issued.value }; | |
| 351 | } | |
| 352 | ||
| 353 | const queuedRun = (repoId: string, issueId: string, actorId: string) => | |
| 354 | createRun(app.db, { | |
| 355 | repoId, | |
| 356 | kind: "agent", | |
| 357 | issueId, | |
| 358 | actorId, | |
| 359 | reason: "assigned", | |
| 360 | }); | |
| 361 | ||
| 362 | test("a person taking it cancels the queued run, and their word starts none", async () => { | |
| 363 | const { repo, issue } = await setup("to-person"); | |
| 364 | const run = queuedRun(repo.id, issue.id, bot.id); | |
| 365 | expect(assignIssue(app, alice, repo, issue, alice).ok).toBe(true); | |
| 366 | expect(getRun(app.db, run.id)?.state).toBe("cancelled"); | |
| 367 | const said = listComments(app.db, issue.id).map((c) => c.body); | |
| 368 | expect(said.some((b) => b.includes("@alice") && b.includes("@bot"))).toBe( | |
| 369 | true, | |
| 370 | ); | |
| 371 | const talk = addComment(app, alice, repo, issue, "I have this one"); | |
| 372 | expect(talk.ok).toBe(true); | |
| 373 | expect( | |
| 374 | listQueued(app.db, "agent").some((r) => r.issueId === issue.id), | |
| 375 | ).toBe(false); | |
| 376 | }); | |
| 377 | ||
| 378 | test("another agent taking it stops the running run and queues its own", async () => { | |
| 379 | const { repo, issue } = await setup("to-agent"); | |
| 380 | const run = queuedRun(repo.id, issue.id, bot.id); | |
| 381 | claimRun(app.db, run.id); | |
| 382 | expect(assignIssue(app, alice, repo, issue, other).ok).toBe(true); | |
| 383 | expect(getRun(app.db, run.id)?.state).toBe("cancelled"); | |
| 384 | const queued = listQueued(app.db, "agent").filter( | |
| 385 | (r) => r.issueId === issue.id, | |
| 386 | ); | |
| 387 | expect(queued).toHaveLength(1); | |
| 388 | expect(queued[0]!.actorId).toBe(other.id); | |
| 389 | }); | |
| 390 | }); |
new above
offline