Public: anyone can read this; the crew can change it.
While "flow only" is on, a crew pull for a non-flow issue gets no feedback run; it is held with one line saying it waits for the switch #7393
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6446-while-flow-only-is-on-a-crew-pull-for-a → main 2 commits fork 3 threads · 3 unresolved
3 files
changed +93
−9
src/crew/requested.ts +40 −8
| @@ -24,8 +24,8 @@ import type { App } from "../app.ts"; | ||
| 24 | 24 | import { addComment, assignIssue } from "../actions/issues.ts"; |
| 25 | 25 | import { getAgent, listCrewRoles, type Agent } from "../core/agents.ts"; |
| 26 | 26 | import { getHold } from "../core/holds.ts"; |
| 27 | import { getIssueById, listComments, type Issue } from "../core/issues.ts"; | |
| 28 | import { getPull } from "../core/pulls.ts"; | |
| 27 | import { getIssue, getIssueById, listComments, type Issue } from "../core/issues.ts"; | |
| 28 | import { closesFromBody, getPull } from "../core/pulls.ts"; | |
| 29 | 29 | import type { Repo } from "../core/repos.ts"; |
| 30 | 30 | import { requestedPulls, verdictIsStale, type RequestedPull } from "../core/requested.ts"; |
| 31 | 31 | import { reviewSummary } from "../core/reviews.ts"; |
| @@ -33,10 +33,11 @@ import { activeRunFor } from "../core/runs.ts"; | ||
| 33 | 33 | import { getUserById } from "../core/users.ts"; |
| 34 | 34 | import { createLog } from "../kernel/log.ts"; |
| 35 | 35 | import { queueAgentRun } from "./dispatch.ts"; |
| 36 | import { flowOnlyHolds } from "./flow_only.ts"; | |
| 36 | 37 | import { HANDOVER_NOTE, handoverOf } from "./guard.ts"; |
| 37 | 38 | import { greenHead, sendToReview } from "./review.ts"; |
| 38 | 39 | import { pickBuilder } from "./route.ts"; |
| 39 | import { NO_BUILDER_ON_CREW, WAITS_FOR_BUILDER_SLOT } from "./words.ts"; | |
| 40 | import { NO_BUILDER_ON_CREW, WAITS_FOR_BUILDER_SLOT, WAITS_FOR_FLOW_ONLY } from "./words.ts"; | |
| 40 | 41 | |
| 41 | 42 | const log = createLog("crew"); |
| 42 | 43 | |
| @@ -62,6 +63,7 @@ function reasonOf(why: string): string { | ||
| 62 | 63 | if (why === "a person holds it" || why.startsWith("held,")) return "held"; |
| 63 | 64 | if (why.includes("a person")) return "handed to a person"; |
| 64 | 65 | if (why === WAITS_FOR_BUILDER_SLOT) return "waits for a slot"; |
| 66 | if (why === WAITS_FOR_FLOW_ONLY) return "waits for flow only"; | |
| 65 | 67 | if (why === "the new head is not green yet") return "waits for a check"; |
| 66 | 68 | if (why.startsWith("past the review rounds") || why.startsWith("a verdict is already") || why.startsWith("a run since")) |
| 67 | 69 | return "settled"; |
| @@ -112,11 +114,11 @@ export async function moveRequested( | ||
| 112 | 114 | const builders = crew.filter((c) => c.role === "builder").map((c) => c.agent); |
| 113 | 115 | let slots = free; |
| 114 | 116 | for (const requested of requestedPulls(app.db, repo.id)) { |
| 115 | const move = verdictIsStale(requested) | |
| 116 | ? await askAgain(app, repo, requested, now) | |
| 117 | : slots <= 0 | |
| 118 | ? notMoved(requested, WAITS_FOR_BUILDER_SLOT) | |
| 119 | : followUp(app, repo, requested, builders); | |
| 117 | let move: RequestedMove; | |
| 118 | if (verdictIsStale(requested)) move = await askAgain(app, repo, requested, now); | |
| 119 | else if (flowOnlyKeeps(app, repo, requested)) move = holdForFlowOnly(app, repo, requested); | |
| 120 | else if (slots <= 0) move = notMoved(requested, WAITS_FOR_BUILDER_SLOT); | |
| 121 | else move = followUp(app, repo, requested, builders); | |
| 120 | 122 | if (move.did === "queued feedback") slots--; |
| 121 | 123 | moves.push(move); |
| 122 | 124 | say(repo, requested, move); |
| @@ -133,6 +135,36 @@ const notMoved = (p: RequestedPull, why: string): RequestedMove => ({ | ||
| 133 | 135 | why, |
| 134 | 136 | }); |
| 135 | 137 | |
| 138 | /** | |
| 139 | * Whether FLOW ONLY keeps the pull's builder off: it closes issues, and none | |
| 140 | * of them is flow work (`flowOnlyHolds`). A pull that closes nothing is not held. | |
| 141 | */ | |
| 142 | function flowOnlyKeeps(app: App, repo: Repo, requested: RequestedPull): boolean { | |
| 143 | if (!repo.flowOnly) return false; | |
| 144 | const pull = getPull(app.db, repo.id, requested.number); | |
| 145 | if (!pull) return false; | |
| 146 | const closed = closesFromBody(pull.body) | |
| 147 | .map((n) => getIssue(app.db, repo.id, n)) | |
| 148 | .filter((i) => i !== undefined); | |
| 149 | return closed.length > 0 && closed.every((i) => flowOnlyHolds(app.db, repo, i)); | |
| 150 | } | |
| 151 | ||
| 152 | /** No feedback run while the switch is on: one comment, said once, and the pull waits. */ | |
| 153 | function holdForFlowOnly(app: App, repo: Repo, requested: RequestedPull): RequestedMove { | |
| 154 | const issue = getIssueById(app.db, requested.pullId); | |
| 155 | const reviewer = getUserById(app.db, requested.reviewerId); | |
| 156 | const note = `${WAITS_FOR_FLOW_ONLY}. (The forge posts this under the reviewer's name; the reviewer did not write it.)`; | |
| 157 | if ( | |
| 158 | issue !== undefined && | |
| 159 | reviewer !== undefined && | |
| 160 | !listComments(app.db, issue.id).some( | |
| 161 | (c) => c.body.startsWith(WAITS_FOR_FLOW_ONLY) && c.createdAt >= requested.at, | |
| 162 | ) | |
| 163 | ) | |
| 164 | addComment(app, reviewer, repo, issue, note); | |
| 165 | return notMoved(requested, WAITS_FOR_FLOW_ONLY); | |
| 166 | } | |
| 167 | ||
| 136 | 168 | /** A push has come since the verdict: the reviewer reads the new head. */ |
outdated
Over-long line; split the message into a const. |
||
| 137 | 169 | async function askAgain( |
| 138 | 170 | app: App, |
src/crew/words.ts +3
| @@ -10,3 +10,6 @@ export const WAITS_FOR_BUILDER_SLOT = "waits for a builder slot"; | ||
| 10 | 10 | |
| 11 | 11 | /** The repository's crew has no agent in the builder role to run it. */ |
| 12 | 12 | export const NO_BUILDER_ON_CREW = "no builder on the crew"; |
| 13 | ||
| 14 | /** FLOW ONLY is on and the pull's issue is not flow work: no feedback run until it is off. */ | |
| 15 | export const WAITS_FOR_FLOW_ONLY = "Waits for flow only to be turned off"; |
test/requested.test.ts +50 −1
| @@ -15,7 +15,7 @@ import { openPull } from "../src/actions/pulls.ts"; | ||
| 15 | 15 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| 16 | 16 | import { createApp, type App } from "../src/app.ts"; |
| 17 | 17 | import { setCrewRole } from "../src/core/agents.ts"; |
| 18 | import { getIssueById, listComments } from "../src/core/issues.ts"; | |
| 18 | import { createIssue, createLabel, getIssueById, getLabel, listComments, setIssueLabels } from "../src/core/issues.ts"; | |
| 19 | 19 | import { getPull, type Pull } from "../src/core/pulls.ts"; |
| 20 | 20 | import { getRepoById, type Repo } from "../src/core/repos.ts"; |
| 21 | 21 | import { submitReview } from "../src/core/reviews.ts"; |
| @@ -24,6 +24,7 @@ import { createUser, getUserByName, type User } from "../src/core/users.ts"; | ||
| 24 | 24 | import { autonomyTick } from "../src/crew/autonomy.ts"; |
| 25 | 25 | import { HANDOVER_NOTE } from "../src/crew/guard.ts"; |
| 26 | 26 | import { moveRequested, tallyRequested, tallyWords } from "../src/crew/requested.ts"; |
| 27 | import { WAITS_FOR_FLOW_ONLY } from "../src/crew/words.ts"; | |
| 27 | 28 | import { requestedSays, requestedWaits } from "../src/flow/requested.ts"; |
| 28 | 29 | import { serve, type Servers } from "../src/http/server.ts"; |
| 29 | 30 | import { createToken } from "../src/core/tokens.ts"; |
| @@ -359,4 +360,52 @@ describe("a pull with changes requested always moves", () => { | ||
| 359 | 360 | expect(next).toEqual({ moved: 0, total: 3, waiting: { "a run is in flight": 2, "handed to a person": 1 } }); |
| 360 | 361 | expect(tallyWords(next)).toBe("0/3 moved; 2 a run is in flight, 1 handed to a person"); |
| 361 | 362 | }); |
| 363 | ||
| 364 | test("while flow only is on, a pull for a non-flow issue gets no feedback run and one comment; a flow pull gets a run; off, a run", async () => { | |
| 365 | app.db.query("UPDATE issue SET state = 'closed' WHERE repo_id = ?1 AND kind = 'pull'").run(repo.id); | |
| 366 | clearRuns(); | |
| 367 | const flowLabel = (): string => { | |
| 368 | const have = getLabel(app.db, repo.id, "flow"); | |
| 369 | if (have) return have.id; | |
| 370 | const made = createLabel(app.db, repo.id, "flow", "1d76db"); | |
| 371 | if (!made.ok) throw new Error(made.error); | |
| 372 | return made.value.id; | |
| 373 | }; | |
| 374 | const closing = async (labels: boolean) => { | |
| 375 | const made = createIssue(app.db, { repoId: repo.id, title: "Work", body: "", authorId: alice.id }); | |
| 376 | if (!made.ok) throw new Error(made.error); | |
| 377 | if (labels) setIssueLabels(app.db, made.value.id, [flowLabel()]); | |
| 378 | const pull = await currentShape(); | |
| 379 | app.db.query("UPDATE issue SET body = ?2 WHERE id = ?1").run(pull.id, `Closes #${made.value.number}`); | |
| 380 | return pull; | |
| 381 | }; | |
| 382 | const said = (pull: Pull) => | |
| 383 | listComments(app.db, pull.id).filter((c) => c.body.startsWith(WAITS_FOR_FLOW_ONLY)); | |
| 384 | app.db.query("UPDATE repo SET flow_only = 1 WHERE id = ?1").run(repo.id); | |
| 385 | const feature = await closing(false); | |
| 386 | const flow = await closing(true); | |
| 387 | const moves = await moveRequested(app, fresh(), 5); | |
| 388 | expect(moves.find((m) => m.pull === feature.number)).toEqual({ | |
| 389 | pull: feature.number, | |
| 390 | did: "not moved", | |
| 391 | why: WAITS_FOR_FLOW_ONLY, | |
| 392 | }); | |
| 393 | const tally = tallyRequested(moves); | |
| 394 | expect(tally.waiting["waits for flow only"]).toBe(1); | |
| 395 | expect(tallyWords(tally)).toContain("1 waits for flow only"); | |
| 396 | expect(runs(feature, "feedback")).toHaveLength(0); | |
| 397 | expect(said(feature)).toHaveLength(1); | |
| 398 | expect(moves.find((m) => m.pull === flow.number)?.did).toBe("queued feedback"); | |
| 399 | expect(runs(flow, "feedback")).toHaveLength(1); | |
| 400 | expect(said(flow)).toHaveLength(0); | |
| 401 | // Said once. | |
| 402 | await moveRequested(app, fresh(), 5); | |
| 403 | expect(said(feature)).toHaveLength(1); | |
| 404 | expect(runs(feature, "feedback")).toHaveLength(0); | |
| 405 | // Switch off: the next tick handles it as ever. | |
| 406 | app.db.query("UPDATE repo SET flow_only = 0 WHERE id = ?1").run(repo.id); | |
| 407 | const off = await moveRequested(app, fresh(), 5); | |
| 408 | expect(off.find((m) => m.pull === feature.number)?.did).toBe("queued feedback"); | |
| 409 | expect(runs(feature, "feedback")).toHaveLength(1); | |
| 410 | }); | |
| 362 | 411 | }); |
new above
offline
Nested ternary is mis-indented; use an if chain. Also classify the hold's reason as its own tally word (see line 62) rather than 'held'.