Public: anyone can read this; the crew can change it.
A conflict run that finds the work already done closes the pull: no conflict loops #2125
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2121-a-conflict-run-that-finds-the-work-alrea → main 1 commit fork 2 threads · 2 unresolved
2 files
changed +148
−5
src/crew/autonomy.ts +78 −2
| @@ -26,6 +26,7 @@ import { addLearned } from "../checks/learned.ts"; | ||
| 26 | 26 | import { enqueue, type GateCandidate, type Queued } from "../checks/queue.ts"; |
| 27 | 27 | import { mergeable, resolveOid, treeOf } from "../git/merge.ts"; |
| 28 | 28 | import { repoDir } from "../git/repos.ts"; |
| 29 | import { CLOSED_NOTE } from "../core/done_on_main.ts"; | |
| 29 | 30 | import { mergePull } from "../actions/pulls.ts"; |
| 30 | 31 | import { |
| 31 | 32 | listCrewRoles, |
| @@ -870,6 +871,13 @@ export function settleAgentRun( | ||
| 870 | 871 | if (/opened pull #|updated pull #/.test(run.summary)) return false; |
| 871 | 872 | if (interruptionOf(run.summary) !== undefined) return false; |
| 872 | 873 | if (answeredWithoutChange(app, repo, run)) return true; |
| 874 | const carried = alreadyDoneOids(run.summary); | |
| 875 | if (run.state === "ok" && carried.length > 0) { | |
| 876 | const asked = getIssueById(app.db, run.issueId); | |
| 877 | const own = asked?.kind === "pull" ? getPull(app.db, repo.id, asked.number) : undefined; | |
| 878 | if (own && own.mergedAt === null) | |
| 879 | return closeSuperseded(app, repo, own, carried); | |
| 880 | } | |
| 873 | 881 | const issue = getIssueById(app.db, run.issueId); |
| 874 | 882 | if (!issue || issue.kind !== "issue" || issue.state !== "open") return false; |
| 875 | 883 | if (issue.id === repo.directionIssueId) return false; |
| @@ -1240,7 +1248,7 @@ function mergeWhy(error: AccessError | MergePullError): string { | ||
| 1240 | 1248 | /** Pull id → the tips git last read, and the files in conflict (null: clean). */ |
| 1241 | 1249 | const checked = new Map< |
| 1242 | 1250 | string, |
| 1243 | { head: string; key: string; files: string[] | null } | |
| 1251 | { head: string; key: string; files: string[] | null; tree: string | null } | |
| 1244 | 1252 | >(); |
| 1245 | 1253 | /** Pull id → when the sweep last saw it merge cleanly. */ |
| 1246 | 1254 | const cleanAt = new Map<string, string>(); |
| @@ -1270,6 +1278,52 @@ function conflictEpisodeStart(app: App, issue: Issue, pullId: string): string { | ||
| 1270 | 1278 | return start; |
| 1271 | 1279 | } |
| 1272 | 1280 | |
| 1281 | /** | |
| 1282 | * Close a pull whose change is already on the base, naming the commits that | |
| 1283 | * carry it, and close the issues it said it closes as done (#2121). Not a | |
| 1284 | * park and no strike: nothing failed, the work landed another way. | |
| 1285 | */ | |
| 1286 | export function closeSuperseded( | |
| 1287 | app: App, | |
| 1288 | repo: Repo, | |
| 1289 | pull: Pull, | |
| 1290 | oids: readonly string[], | |
| 1291 | ): boolean { | |
| 1292 | if (repo.autonomyBy === null) return false; | |
| 1293 | const by = getUserById(app.db, repo.autonomyBy); | |
| 1294 | const issue = getIssueById(app.db, pull.id); | |
| 1295 | if (!by || !issue || issue.state !== "open") return false; | |
| 1296 | const named = oids.map((o) => o.slice(0, 8)).join(" "); | |
| 1297 | addComment( | |
| 1298 | app, | |
| 1299 | by, | |
| 1300 | repo, | |
| 1301 | issue, | |
| 1302 | `Closed by the crew: superseded, its change is already on ${pull.baseBranch} in ${named}.`, | |
| 1303 | ); | |
| 1304 | const closed = setIssueState(app, by, repo, issue, "closed"); | |
| 1305 | if (!closed.ok) return false; | |
| 1306 | for (const number of closesFromBody(pull.body)) { | |
| 1307 | const about = getIssue(app.db, repo.id, number); | |
| 1308 | if (!about || about.state !== "open") continue; | |
| 1309 | if (about.assigneeId !== null) assignIssue(app, by, repo, about, null); | |
| 1310 | addComment(app, by, repo, about, `${CLOSED_NOTE}${named} (pull #${pull.number}).`); | |
| 1311 | setIssueState(app, by, repo, about, "closed", { reason: "done" }); | |
| 1312 | } | |
| 1313 | log.info("superseded pull closed", { | |
| 1314 | repo: `${repo.ownerName}/${repo.name}`, | |
| 1315 | pull: pull.number, | |
| 1316 | commits: oids.length, | |
| 1317 | }); | |
| 1318 | return true; | |
| 1319 | } | |
| 1320 | ||
| 1321 | /** Conflict runs the crew may spend on one pull for one main tip. */ | |
| 1322 | const RUNS_PER_TIP = 2; | |
| 1323 | ||
| 1324 | /** Pull id → the main tip it was last asked about, and how many times. */ | |
| 1325 | const asked = new Map<string, { tip: string; n: number }>(); | |
| 1326 | ||
| 1273 | 1327 | /** How long a conflicted pull may wait to be brought up to date. */ |
| 1274 | 1328 | const CONFLICT_LIMIT_MS = 24 * 3_600_000; |
| 1275 | 1329 | |
| @@ -1331,6 +1385,7 @@ export async function sweepConflicts(app: App, repo: Repo): Promise<number> { | ||
| 1331 | 1385 | if (getIssueById(app.db, id)?.state !== "open") { |
| 1332 | 1386 | checked.delete(id); |
| 1333 | 1387 | cleanAt.delete(id); |
| 1388 | asked.delete(id); | |
| 1334 | 1389 | } |
| 1335 | 1390 | for (const pull of listPulls(app.db, repo.id, { sealed: true, state: "open", limit: 50 })) { |
| 1336 | 1391 | if (!pull.authorIsAgent || pull.mergedAt !== null) continue; |
| @@ -1350,6 +1405,7 @@ export async function sweepConflicts(app: App, repo: Repo): Promise<number> { | ||
| 1350 | 1405 | seen = { |
| 1351 | 1406 | head: pull.headOid, |
| 1352 | 1407 | key, |
| 1408 | tree: reading.ok ? reading.value.treeOid : null, | |
| 1353 | 1409 | files: reading.ok |
| 1354 | 1410 | ? null |
| 1355 | 1411 | : reading.error.kind === "conflict" |
| @@ -1358,6 +1414,16 @@ export async function sweepConflicts(app: App, repo: Repo): Promise<number> { | ||
| 1358 | 1414 | }; |
| 1359 | 1415 | checked.set(pull.id, seen); |
| 1360 | 1416 | } |
| 1417 | // Merging main leaves main's own tree: the pull adds nothing, so there | |
| 1418 | // is no work to bring up to date and no run to spend on it. | |
treeOf(dir, baseTip) is awaited for every pull on every tick. Resolve it once per sweep, outside the loop. |
||
| 1419 | if (seen.tree !== null && seen.tree === (await treeOf(dir, baseTip))) { | |
| 1420 | if (closeSuperseded(app, repo, pull, [baseTip])) { | |
| 1421 | acted++; | |
| 1422 | checked.delete(pull.id); | |
| 1423 | cleanAt.delete(pull.id); | |
| 1424 | } | |
| 1425 | continue; | |
| 1426 | } | |
| 1361 | 1427 | if (seen.files === null) { |
| 1362 | 1428 | // Merges cleanly: whatever came before was a different episode. |
| 1363 | 1429 | cleanAt.set(pull.id, new Date().toISOString()); |
| @@ -1389,7 +1455,17 @@ export async function sweepConflicts(app: App, repo: Repo): Promise<number> { | ||
| 1389 | 1455 | } |
| 1390 | 1456 | continue; |
| 1391 | 1457 | } |
| 1392 | if (bringUpToDate(app, repo, pull, files, since) !== undefined) acted++; | |
| 1458 | // Main's tip asks no more than twice: a run that ends "ok" without a | |
| 1459 | // push must not be queued again on every tick. | |
| 1460 | const tried = asked.get(pull.id); | |
| 1461 | if (tried?.tip === baseTip && tried.n >= RUNS_PER_TIP) continue; | |
| 1462 | if (bringUpToDate(app, repo, pull, files, since) !== undefined) { | |
| 1463 | acted++; | |
| 1464 | asked.set(pull.id, { | |
| 1465 | tip: baseTip, | |
| 1466 | n: tried?.tip === baseTip ? tried.n + 1 : 1, | |
| 1467 | }); | |
| 1468 | } | |
| 1393 | 1469 | } |
| 1394 | 1470 | return acted; |
| 1395 | 1471 | } |
test/conflict_sweep.test.ts +70 −3
| @@ -20,7 +20,7 @@ import { createUser, getUserByName, type User } from "../src/core/users.ts"; | ||
| 20 | 20 | import { parkedCount } from "../src/crew/parks.ts"; |
| 21 | 21 | import { createToken } from "../src/core/tokens.ts"; |
| 22 | 22 | import { serve, type Servers } from "../src/http/server.ts"; |
| 23 | import { sweepConflicts } from "../src/crew/autonomy.ts"; | |
| 23 | import { settleAgentRun, sweepConflicts } from "../src/crew/autonomy.ts"; | |
| 24 | 24 | import { conflictedPulls, conflictedSays } from "../src/flow/conflicted.ts"; |
| 25 | 25 | import { flowLines } from "../src/tui/screens/flow.ts"; |
| 26 | 26 | |
| @@ -188,7 +188,11 @@ test("a pull brought up to date before is not closed for it: only the current co | ||
| 188 | 188 | branch, |
| 189 | 189 | ...(base ? { base } : {}), |
| 190 | 190 | message: `other ${value}`, |
| 191 | files: [{ path: "other.ts", content: `export const other = '${value}';\n` }], | |
| 191 | files: [ | |
| 192 | { path: "other.ts", content: `export const other = '${value}';\n` }, | |
| 193 | // What only the branch adds, so it never just equals main. | |
| 194 | ...(branch === "main" ? [] : [{ path: "own-other.ts", content: "own\n" }]), | |
| 195 | ], | |
| 192 | 196 | }); |
| 193 | 197 | if (!r.ok) throw new Error(JSON.stringify(r.error)); |
| 194 | 198 | }; |
| @@ -230,7 +234,11 @@ async function pullOn(file: string) { | ||
| 230 | 234 | branch, |
| 231 | 235 | ...(base ? { base } : {}), |
| 232 | 236 | message: `${file} ${value}`, |
| 233 | files: [{ path: file, content: `export const v = '${value}';\n` }], | |
| 237 | files: [ | |
| 238 | { path: file, content: `export const v = '${value}';\n` }, | |
| 239 | // What only the branch adds, so it never just equals main. | |
| 240 | ...(branch === "main" ? [] : [{ path: `own-${file}`, content: "own\n" }]), | |
| 241 | ], | |
| 234 | 242 | }); |
| 235 | 243 | if (!r.ok) throw new Error(JSON.stringify(r.error)); |
| 236 | 244 | }; |
| @@ -288,3 +296,62 @@ test("a pull still conflicted 24 hours after the first attempt is closed, and it | ||
| 288 | 296 | const said = listComments(app.db, pull.id).map((c) => c.body).join("\n"); |
| 289 | 297 | expect(said).toContain("within 24 hours"); |
| 290 | 298 | }); |
| 299 | ||
| 300 | test("a pull whose change is already on main is closed by the first pass with no run, and its issue closes as done", async () => { | |
| 301 | const { pull, ask, runs } = await pullOn("carried.ts"); | |
| 302 | // Main lands the very same change another way, after the branch was cut. | |
| 303 | const landed = await commitToBranch(app, alice, repo, { | |
| 304 | branch: "main", | |
| 305 | message: "carried.ts, landed another way", | |
| 306 | files: [ | |
| 307 | { path: "carried.ts", content: "export const v = 'branch';\n" }, | |
| 308 | { path: "own-carried.ts", content: "own\n" }, | |
| 309 | ], | |
| 310 | }); | |
| 311 | if (!landed.ok) throw new Error(JSON.stringify(landed.error)); | |
| 312 | expect(await sweepConflicts(app, fresh())).toBe(1); | |
| 313 | expect(runs().length).toBe(0); | |
| 314 | expect(getIssueById(app.db, pull.id)!.state).toBe("closed"); | |
| 315 | expect(getIssueById(app.db, ask.id)!.state).toBe("closed"); | |
| 316 | const { listComments } = await import("../src/core/issues.ts"); | |
| 317 | const said = listComments(app.db, pull.id).map((c) => c.body).join("\n"); | |
| 318 | expect(said).toContain("already on main"); | |
| 319 | }); | |
| 320 | ||
| 321 | test("a run answering already done closes the pull as superseded and its issue as done", async () => { | |
| 322 | const { pull, ask, write, runs } = await pullOn("answered.ts"); | |
| 323 | await write("main", "main 1"); | |
| 324 | expect(await sweepConflicts(app, fresh())).toBe(1); | |
| 325 | const run = runs()[0]!; | |
| 326 | claimRun(app.db, run.id); | |
| 327 | const oid = "a".repeat(40); | |
| 328 | finishRun(app.db, run.id, { state: "ok", summary: `already done: ${oid}` }); | |
| 329 | const mason = getUserByName(app.db, "mason")!; | |
| 330 | settleAgentRun(app, fresh(), { | |
| 331 | issueId: pull.id, | |
| 332 | state: "ok", | |
| 333 | summary: `already done: ${oid}`, | |
| 334 | reason: "conflict", | |
| 335 | actorId: mason.id, | |
| 336 | }); | |
| 337 | expect(getIssueById(app.db, pull.id)!.state).toBe("closed"); | |
| 338 | expect(getIssueById(app.db, ask.id)!.state).toBe("closed"); | |
| 339 | const { listComments } = await import("../src/core/issues.ts"); | |
| 340 | const said = listComments(app.db, pull.id).map((c) => c.body).join("\n"); | |
| 341 | expect(said).toContain("aaaaaaaa"); | |
| 342 | }); | |
| 343 | ||
| 344 | test("the same main tip asks the crew at most twice", async () => { | |
| 345 | const { pull, write, runs } = await pullOn("capped.ts"); | |
| 346 | await write("main", "main 1"); | |
| 347 | for (let round = 1; round <= 2; round++) { | |
| 348 | expect(await sweepConflicts(app, fresh())).toBe(1); | |
| 349 | const run = runs().find((r) => r.state === "queued")!; | |
| 350 | claimRun(app.db, run.id); | |
| 351 | finishRun(app.db, run.id, { state: "ok", summary: "nothing to push" }); | |
| 352 | } | |
| 353 | expect(runs().length).toBe(2); | |
| 354 | expect(await sweepConflicts(app, fresh())).toBe(0); | |
| 355 | expect(runs().length).toBe(2); | |
| 356 | expect(getIssueById(app.db, pull.id)!.state).toBe("open"); | |
| 357 | }); |
new above
offline
Add the same guard the park path has: return false unless repo.autonomy === "autonomous", and respect getHold.