Public: anyone can read this; the crew can change it.
A resolve run that pushed nothing is never shown as the crew's commit #2296
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2276-a-resolve-run-that-pushed-nothing-is-nev → main 1 commit fork 1 thread · 1 unresolved
4 files
changed +92
−4
src/crew/proposal.ts +10 −1
| @@ -217,6 +217,8 @@ export const PROPOSE_WHY: Record<ProposeError, string> = { | ||
| 217 | 217 | not_queued: "The crew could not take it on right now (its ceiling or breaker); try again later.", |
| 218 | 218 | }; |
| 219 | 219 | |
| 220 | const NOT_PUSHED = "the crew did not push a resolution"; | |
| 221 | ||
| 220 | 222 | /** |
| 221 | 223 | * The latest proposal. A landed one is shown only while the pull's head is |
| 222 | 224 | * still the commit that run left: after another push, it is not the crew's. |
| @@ -239,7 +241,14 @@ export async function proposalOf( | ||
| 239 | 241 | ? { kind: "none" } |
| 240 | 242 | : { kind: "failed", state: last.state, summary: last.summary }; |
| 241 | 243 | } |
| 242 | if (last.commitOid === "" || last.commitOid !== pull.headOid) return { kind: "none" }; | |
| 244 | if (last.commitOid === "") { | |
| 245 | // Ended ok but left no commit: news only while the pull still conflicts. | |
| 246 | const status = await pullStatus(dir, pull); | |
| 247 | return status.conflicts.length === 0 | |
| 248 | ? { kind: "none" } | |
| 249 | : { kind: "failed", state: last.state, summary: NOT_PUSHED }; | |
| 250 | } | |
| 251 | if (last.commitOid !== pull.headOid) return { kind: "none" }; | |
| 243 | 252 | const agent = proposerOf(app, pull); |
| 244 | 253 | return { |
| 245 | 254 | kind: "landed", |
src/crew/runner.ts +23 −3
| @@ -62,6 +62,7 @@ import { | ||
| 62 | 62 | finishRun, |
| 63 | 63 | lastAgentRun, |
| 64 | 64 | noteRunProgress, |
| 65 | setRunBase, | |
| 65 | 66 | overranBefore, |
| 66 | 67 | type Run, |
| 67 | 68 | } from "../core/runs.ts"; |
| @@ -294,6 +295,10 @@ export async function executeAgentRun( | ||
| 294 | 295 | const claimed = claimRun(app.db, run.id); |
| 295 | 296 | if (!claimed) return undefined; |
| 296 | 297 | const lease = holdLease(app.db, run.id); |
| 298 | // A resolve run keeps the head it started from, to tell a push from none. | |
| 299 | const startedAt = | |
| 300 | run.reason === "resolve" ? await pullHeadOf(app, run, false) : undefined; | |
| 301 | if (startedAt !== undefined) setRunBase(app.db, run.id, startedAt); | |
| 297 | 302 | const controller = new AbortController(); |
| 298 | 303 | trackAgentRun(run.id, controller); |
| 299 | 304 | const write = (line: string): void => |
| @@ -387,7 +392,7 @@ export async function executeAgentRun( | ||
| 387 | 392 | // never calls a later push the crew's commit. |
| 388 | 393 | const left = |
| 389 | 394 | run.reason === "resolve" && outcome.state === "ok" |
| 390 | ? await headLeftBy(app, run) | |
| 395 | ? await headLeftBy(app, run, startedAt) | |
| 391 | 396 | : undefined; |
| 392 | 397 | const done = finishRun(app.db, run.id, { |
| 393 | 398 | state: outcome.state, |
| @@ -401,14 +406,29 @@ export async function executeAgentRun( | ||
| 401 | 406 | return undefined; |
| 402 | 407 | } |
| 403 | 408 | |
| 404 | /** The head of the run's pull as the run left it. */ | |
| 405 | async function headLeftBy(app: App, run: Run): Promise<string | undefined> { | |
| 409 | /** The head the run left, if it moved since `startedAt`; none if it pushed nothing. */ | |
| 410 | async function headLeftBy( | |
| 411 | app: App, | |
| 412 | run: Run, | |
| 413 | startedAt: string | undefined, | |
| 414 | ): Promise<string | undefined> { | |
| 415 | const head = await pullHeadOf(app, run, true); | |
| 416 | return head === startedAt ? undefined : head; | |
| 417 | } | |
| 418 | ||
| 419 | /** The head of the run's pull, fetched fresh when `refresh` is set. */ | |
| 420 | async function pullHeadOf( | |
| 421 | app: App, | |
| 422 | run: Run, | |
| 423 | refresh: boolean, | |
| 424 | ): Promise<string | undefined> { | |
| 406 | 425 | const repo = getRepoById(app.db, run.repoId); |
| 407 | 426 | const issue = |
| 408 | 427 | run.issueId === null ? undefined : getIssueById(app.db, run.issueId); |
| 409 | 428 | if (repo === undefined || issue === undefined || issue.kind !== "pull") return undefined; |
| 410 | 429 | const pull = getPull(app.db, repo.id, issue.number); |
| 411 | 430 | if (pull === undefined) return undefined; |
| 431 | if (!refresh) return pull.headOid; | |
| 412 | 432 | // A fork's push reaches the pull's row by a fetch; take it now, not later. |
| 413 | 433 | // A push's own follow-up may have taken the head already, after this row |
| 414 | 434 | // was read: ask the row again rather than trust the copy in hand. |
test/crew-proposal.test.ts +25
| @@ -431,4 +431,29 @@ echo '{"type":"result","subtype":"success","is_error":false,"result":"Wrote note | ||
| 431 | 431 | expect(text).toContain("with `trunk`"); |
| 432 | 432 | expect(text).not.toContain("`main`"); |
| 433 | 433 | }); |
| 434 | ||
| 435 | test("a resolve run that ends ok without pushing records no commit and is not the crew's", async () => { | |
| 436 | // The agent now reports success and pushes nothing; the pull still conflicts. | |
| 437 | const fake = join(dataDir, "fake-claude"); | |
| 438 | writeFileSync( | |
| 439 | fake, | |
| 440 | `#!/bin/sh | |
| 441 | if [ "$1" = "--version" ]; then echo "2.1.0 (fake)"; exit 0; fi | |
| 442 | IFS= read -r first | |
| 443 | echo '{"type":"system","subtype":"init","session_id":"fake-2","model":"fake"}' | |
| 444 | echo '{"type":"result","subtype":"success","is_error":false,"result":"Looked, changed nothing.","total_cost_usd":0.01,"num_turns":1,"session_id":"fake-2","terminal_reason":"completed"}' | |
| 445 | `, | |
| 446 | ); | |
| 447 | const known = new Set(listRuns(app.db, repo.id, { kind: "agent" }).map((r) => r.id)); | |
| 448 | const head = current().headOid; | |
| 449 | const res = await tap(); | |
| 450 | expect(res.status).toBe(303); | |
| 451 | const run = await settled((r) => r.reason === "resolve" && !known.has(r.id)); | |
| 452 | expect(run.state).toBe("ok"); | |
| 453 | expect(run.commitOid).toBe(""); | |
| 454 | expect(current().headOid).toBe(head); | |
| 455 | const page = await resolvePage(); | |
| 456 | expect(page).not.toContain("committed as"); | |
| 457 | expect(page).toContain("the crew did not push a resolution"); | |
| 458 | }, 60_000); | |
| 434 | 459 | }); |
test/proposal-run.test.ts +34
| @@ -18,6 +18,8 @@ import type { Repo } from "../src/core/repos.ts"; | ||
| 18 | 18 | import { submitReview as record } from "../src/core/reviews.ts"; |
| 19 | 19 | import { listRuns } from "../src/core/runs.ts"; |
| 20 | 20 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; |
| 21 | import { proposalOf } from "../src/crew/proposal.ts"; | |
| 22 | import { pullDir } from "../src/core/pulls.ts"; | |
| 21 | 23 | import { landIfGreen } from "../src/crew/autonomy.ts"; |
| 22 | 24 | import { landsOnItsOwn } from "../src/crew/direct.ts"; |
| 23 | 25 | import { createForgeApp } from "../src/tui/forge.ts"; |
| @@ -236,4 +238,36 @@ describe("letting a proposed pull run", () => { | ||
| 236 | 238 | await approveToRun(app, alice, lib, pull()); |
| 237 | 239 | expect(pullMayRun(app.db, lib, pull(), head)).toBe(true); |
| 238 | 240 | }); |
| 241 | ||
| 242 | test("an ok resolve run with no commit on a conflicting pull reads failed; one that pushed reads landed", async () => { | |
| 243 | const theirs = await commitToBranch(app, alice, lib, { | |
| 244 | branch: "feature/clash", | |
| 245 | message: "clash", | |
| 246 | files: [{ path: "clash.md", content: "other\n" }], | |
| 247 | }); | |
| 248 | if (!theirs.ok) throw new Error(JSON.stringify(theirs.error)); | |
| 249 | await push("main", "clash.md"); | |
| 250 | const opened = await openPull(app, alice, lib, { | |
| 251 | title: "Clash", | |
| 252 | baseBranch: "main", | |
| 253 | headBranch: "feature/clash", | |
| 254 | }); | |
| 255 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 256 | const pull = () => getPull(app.db, lib.id, opened.value.number)!; | |
| 257 | const dir = await pullDir(app.db, app.reposRoot, lib, pull()); | |
| 258 | const ran = (commitOid?: string) => { | |
| 259 | const run = createRun(app.db, { | |
| 260 | repoId: lib.id, kind: "agent", issueId: pull().id, reason: "resolve", | |
| 261 | }); | |
| 262 | claimRun(app.db, run.id); | |
| 263 | finishRun(app.db, run.id, { state: "ok", summary: "done", commitOid }); | |
| 264 | return run; | |
| 265 | }; | |
| 266 | ran(); | |
| 267 | const seen = await proposalOf(app, lib, pull(), dir); | |
| 268 | expect(seen).toEqual({ kind: "failed", state: "ok", summary: "the crew did not push a resolution" }); | |
| 269 | ||
| 270 | ran(pull().headOid); | |
| 271 | expect((await proposalOf(app, lib, pull(), dir)).kind).toBe("landed"); | |
| 272 | }); | |
| 239 | 273 | }); |
new above
offline
The summary is shown as "The last proposal did not land (ok): …". Showing "ok" beside a failure is confusing; for this case, render the summary without the state.