Public: anyone can read this; the crew can change it.
A pull with changes requested always moves: a follow-up on the reviewed head, a fresh review on a newer one #2180
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/2172-a-pull-with-changes-requested-always-mov → main 5 commits fork 1 thread · 1 unresolved
11 files
changed +694
−6
README.md +10
| @@ -541,6 +541,16 @@ card ends when the pull is approved, closed or merged. Hand-overs can be | ||
| 541 | 541 | closed in bulk like the other pull asks. The terminal's On you card lists |
| 542 | 542 | them as "handed over" rows: `enter` opens the pull, `x` closes it. |
| 543 | 543 | |
| 544 | **A request for changes always moves.** Every tick, a crew pull whose | |
| 545 | reviewer last asked for changes is moved on. If the builder has pushed since | |
| 546 | and the new head is green, the reviewer is asked again: a verdict on old | |
| 547 | code never blocks. If the verdict is on the current head and no run is in | |
| 548 | flight, one feedback run is queued to the pull's builder, and a builder is | |
| 549 | assigned when the pull has none. That run takes a builder slot like any | |
| 550 | other. The tick log names each pull it moved, or why it did not, and | |
| 551 | `/flow`, the terminal's Flow dialog and the MCP `flow` tool list the pulls | |
| 552 | waiting on a builder and those waiting on a reviewer, oldest first. | |
| 553 | ||
| 544 | 554 | **Already done.** A builder that finds its issue already built on the base |
| 545 | 555 | branch declines with `DECLINED: already done on main in <commit>`, naming the |
| 546 | 556 | full or abbreviated id of each commit that did it (at most five). The forge |
src/core/requested.ts added +56
| @@ -0,0 +1,56 @@ | ||
| 1 | /** | |
| 2 | * Open pulls whose crew reviewer's latest word is "changes requested" | |
| 3 | * (#2172): one read, for the tick that moves them (`crew/requested.ts`) | |
| 4 | * and for /flow that shows them waiting (`flow/requested.ts`), so the two | |
| 5 | * can never disagree about which pulls these are. | |
| 6 | * | |
| 7 | * "Latest" is the newest verdict any reviewer on the pull's crew passed on | |
| 8 | * it, whatever head it was on: a verdict on an older head is the one a | |
| 9 | * push has not been read against yet. An imported review is an origin's, | |
| 10 | * never the crew's. Pure data: no authz, no events. | |
| 11 | */ | |
| 12 | ||
| 13 | import type { Database } from "bun:sqlite"; | |
| 14 | ||
| 15 | export interface RequestedPull { | |
| 16 | readonly repoId: string; | |
| 17 | /** `owner/name`. */ | |
| 18 | readonly repo: string; | |
| 19 | /** Whether the repository is public: a reader kept from private names sees only these. */ | |
| 20 | readonly isPublic: number; | |
| 21 | readonly pullId: string; | |
| 22 | readonly number: number; | |
| 23 | readonly headOid: string; | |
| 24 | /** The head the verdict was passed on. */ | |
| 25 | readonly reviewedOid: string; | |
| 26 | readonly reviewerId: string; | |
| 27 | /** When the verdict was passed. */ | |
| 28 | readonly at: string; | |
| 29 | } | |
| 30 | ||
| 31 | /** Oldest verdict first; one repository's when `repoId` is given, else every one's. */ | |
| 32 | export function requestedPulls(db: Database, repoId?: string): RequestedPull[] { | |
| 33 | return db | |
| 34 | .query<RequestedPull, [string | null]>( | |
| 35 | `SELECT i.repo_id AS repoId, o.username || '/' || r.name AS repo, | |
| 36 | r.visibility = 'public' AS isPublic, | |
| 37 | i.id AS pullId, i.number AS number, p.head_oid AS headOid, | |
| 38 | v.commit_oid AS reviewedOid, v.author_id AS reviewerId, v.created_at AS at | |
| 39 | FROM issue i JOIN pull p ON p.issue_id = i.id | |
| 40 | JOIN repo r ON r.id = i.repo_id JOIN user o ON o.id = r.owner_id | |
| 41 | JOIN review v ON v.id = ( | |
| 42 | SELECT w.id FROM review w | |
| 43 | JOIN repo_crew c ON c.repo_id = i.repo_id AND c.user_id = w.author_id | |
| 44 | WHERE w.issue_id = i.id AND c.role = 'reviewer' AND w.origin_author IS NULL | |
| 45 | ORDER BY w.id DESC LIMIT 1) | |
| 46 | WHERE i.kind = 'pull' AND i.state = 'open' AND p.merged_at IS NULL | |
| 47 | AND v.state = 'changes_requested' | |
| 48 | AND (?1 IS NULL OR i.repo_id = ?1) | |
| 49 | ORDER BY v.created_at, i.number`, | |
| 50 | ) | |
| 51 | .all(repoId ?? null); | |
| 52 | } | |
| 53 | ||
| 54 | /** Whether a push has come since the verdict: the reviewer owes the next word, not the builder. */ | |
| 55 | export const verdictIsStale = (p: RequestedPull): boolean => | |
| 56 | p.reviewedOid !== p.headOid; |
src/crew/autonomy.ts +16
| @@ -119,6 +119,7 @@ import { | ||
| 119 | 119 | speaksForCrew, |
| 120 | 120 | } from "./direct.ts"; |
| 121 | 121 | import { crewOffInPreview } from "./preview.ts"; |
| 122 | import { moveRequested } from "./requested.ts"; | |
| 122 | 123 | import { reviewersOf, sendToReview, sweepReviews } from "./review.ts"; |
| 123 | 124 | |
| 124 | 125 | const log = createLog("autonomy"); |
| @@ -330,6 +331,13 @@ export function repoPace(app: App, repo: Repo, now = new Date()): Pace { | ||
| 330 | 331 | return pace(accounts, repo.autonomySlots, meanRunUsd(app.db, repo.id), now); |
| 331 | 332 | } |
| 332 | 333 | |
| 334 | /** The builder slots free now: none while paused, else the paced slots less what is slotted. */ | |
| 335 | function freeSlots(app: App, repo: Repo, now: Date): number { | |
| 336 | if (autonomyStatus(app, repo, now).paused !== null) return 0; | |
| 337 | const slotted = slottedRuns(activeRuns(app, repo.id), listCrewRoles(app.db, repo.id)); | |
| 338 | return Math.max(0, repoPace(app, repo, now).slots - slotted.length); | |
| 339 | } | |
| 340 | ||
| 333 | 341 | export type Drove = "off" | "paused" | "waiting" | "full" | "picked" | "planned" | "idle"; |
| 334 | 342 | |
| 335 | 343 | /** |
| @@ -1131,6 +1139,9 @@ export async function autonomyTick( | ||
| 1131 | 1139 | }), |
| 1132 | 1140 | ); |
| 1133 | 1141 | const current = getRepoById(app.db, repo.id) ?? repo; |
| 1142 | // Before the picks: a pull a reviewer answered is finished before new | |
| 1143 | // work takes the slots it needs (#2172). | |
| 1144 | const requested = await moveRequested(app, current, freeSlots(app, current, now), now); | |
| 1134 | 1145 | const did = await driveRepo(app, current, now); |
| 1135 | 1146 | // Every tick says what it did and why, so an app that sits still is |
| 1136 | 1147 | // never a mystery: the numbers behind "idle" are on the line. |
| @@ -1151,6 +1162,11 @@ export async function autonomyTick( | ||
| 1151 | 1162 | ).length, |
| 1152 | 1163 | active: status.active, |
| 1153 | 1164 | slots: status.slots, |
| 1165 | ...(requested.length === 0 | |
| 1166 | ? {} | |
| 1167 | : { | |
| 1168 | requested: `${requested.filter((m) => m.did !== "not moved").length}/${requested.length} moved`, | |
| 1169 | }), | |
| 1154 | 1170 | spent: `${status.spentTodayUsd.toFixed(2)}/${status.ceilingUsd}`, |
| 1155 | 1171 | ...(status.paused === null ? {} : { paused: status.paused }), |
| 1156 | 1172 | }); |
src/crew/requested.ts added +204
| @@ -0,0 +1,204 @@ | ||
| 1 | /** | |
| 2 | * A pull with changes requested always moves (#2172). | |
| 3 | * | |
| 4 | * Every tick, for each open crew pull whose reviewer's latest verdict is | |
| 5 | * "changes requested" (`core/requested.ts`): | |
| 6 | * | |
| 7 | * - On an older head than the pull's, with that head green: the reviewer | |
| 8 | * is asked again. The verdict was about code that is no longer there, | |
| 9 | * so it never blocks; a head that was never sent is sent however long | |
| 10 | * it has been green (`sendToReview`'s `stale`). | |
| 11 | * - On the current head, with no run in flight on the pull and none | |
| 12 | * started since the verdict: a feedback run is queued to the pull's | |
| 13 | * builder, one being assigned when the pull has none. One run per pull, | |
| 14 | * and each takes a builder slot — `free` is how many the tick has. | |
| 15 | * | |
| 16 | * Every pull it passes over gets one line saying why, said once until the | |
| 17 | * reason changes, so a stall is never silent and never a flood. | |
| 18 | */ | |
| 19 | ||
| 20 | import type { App } from "../app.ts"; | |
| 21 | import { assignIssue } from "../actions/issues.ts"; | |
| 22 | import { getAgent, listCrewRoles, type Agent } from "../core/agents.ts"; | |
| 23 | import { getHold } from "../core/holds.ts"; | |
| 24 | import { getIssueById } from "../core/issues.ts"; | |
| 25 | import { getPull } from "../core/pulls.ts"; | |
| 26 | import type { Repo } from "../core/repos.ts"; | |
| 27 | import { requestedPulls, verdictIsStale, type RequestedPull } from "../core/requested.ts"; | |
| 28 | import { reviewSummary } from "../core/reviews.ts"; | |
| 29 | import { activeRunFor } from "../core/runs.ts"; | |
| 30 | import { getUserById } from "../core/users.ts"; | |
| 31 | import { createLog } from "../kernel/log.ts"; | |
| 32 | import { queueAgentRun } from "./dispatch.ts"; | |
| 33 | import { handoverOf } from "./guard.ts"; | |
| 34 | import { greenHead, sendToReview } from "./review.ts"; | |
| 35 | import { pickBuilder } from "./route.ts"; | |
| 36 | ||
| 37 | const log = createLog("crew"); | |
| 38 | ||
| 39 | /** What the tick did with one pull: moved it, or why not. */ | |
| 40 | export type RequestedMove = | |
| 41 | | { readonly pull: number; readonly did: "asked reviewer" } | |
| 42 | | { readonly pull: number; readonly did: "queued feedback"; readonly builder: string; readonly assigned: boolean } | |
| 43 | | { readonly pull: number; readonly did: "not moved"; readonly why: string }; | |
| 44 | ||
| 45 | /** | |
| 46 | * The last why-not said for a pull, so it is said once: a log dedupe, in | |
| 47 | * memory, keyed `repo:pull` across every repository. Each tick drops its | |
| 48 | * repository's keys for pulls no longer waiting; a repository that stops | |
| 49 | * ticking keeps its few until the process restarts, which only means one | |
| 50 | * line is said again. | |
| 51 | */ | |
| 52 | const said = new Map<string, string>(); | |
| 53 | ||
| 54 | /** | |
| 55 | * Move every pull with changes requested in `repo` that can move, taking | |
| 56 | * at most `free` builder slots. Returns what happened to each, oldest | |
| 57 | * verdict first. | |
| 58 | */ | |
| 59 | export async function moveRequested( | |
| 60 | app: App, | |
| 61 | repo: Repo, | |
| 62 | free: number, | |
| 63 | now = new Date(), | |
| 64 | ): Promise<RequestedMove[]> { | |
| 65 | const moves: RequestedMove[] = []; | |
| 66 | const crew = listCrewRoles(app.db, repo.id); | |
| 67 | const builders = crew.filter((c) => c.role === "builder").map((c) => c.agent); | |
| 68 | let slots = free; | |
| 69 | for (const requested of requestedPulls(app.db, repo.id)) { | |
| 70 | const move = verdictIsStale(requested) | |
| 71 | ? await askAgain(app, repo, requested, now) | |
| 72 | : slots <= 0 | |
| 73 | ? notMoved(requested, "waits for a builder slot") | |
| 74 | : followUp(app, repo, requested, builders); | |
| 75 | if (move.did === "queued feedback") slots--; | |
| 76 | moves.push(move); | |
| 77 | say(repo, requested, move); | |
| 78 | } | |
| 79 | const open = new Set(moves.map((m) => `${repo.id}:${m.pull}`)); | |
| 80 | for (const key of said.keys()) | |
| 81 | if (key.startsWith(`${repo.id}:`) && !open.has(key)) said.delete(key); | |
| 82 | return moves; | |
| 83 | } | |
| 84 | ||
| 85 | const notMoved = (p: RequestedPull, why: string): RequestedMove => ({ | |
| 86 | pull: p.number, | |
| 87 | did: "not moved", | |
| 88 | why, | |
| 89 | }); | |
| 90 | ||
| 91 | /** A push has come since the verdict: the reviewer reads the new head. */ | |
| 92 | async function askAgain( | |
| 93 | app: App, | |
| 94 | repo: Repo, | |
| 95 | requested: RequestedPull, | |
| 96 | now: Date, | |
| 97 | ): Promise<RequestedMove> { | |
| 98 | const pull = getPull(app.db, repo.id, requested.number); | |
| 99 | if (!pull) return notMoved(requested, "no pull row"); | |
| 100 | if (!greenHead(app.db, pull)) return notMoved(requested, "the new head is not green yet"); | |
| 101 | const result = await sendToReview(app, repo, pull, now, { stale: true }); | |
| 102 | if (result === "sent") return { pull: requested.number, did: "asked reviewer" }; | |
| 103 | return notMoved( | |
| 104 | requested, | |
| 105 | result === "wait" | |
| 106 | ? "a review of the new head is out or could not be queued" | |
| 107 | : result === "lapsed" | |
| 108 | ? "the new head's review lapsed; waits for a person" | |
| 109 | : result === "held" | |
| 110 | ? "held, or handed to a person" | |
| 111 | : result === "done" | |
| 112 | ? "a verdict is already on the new head" | |
| 113 | : "no reviewer on the crew", | |
| 114 | ); | |
| 115 | } | |
| 116 | ||
| 117 | /** The verdict is on the current head: the builder answers it. */ | |
| 118 | function followUp( | |
| 119 | app: App, | |
| 120 | repo: Repo, | |
| 121 | requested: RequestedPull, | |
| 122 | builders: readonly Agent[], | |
| 123 | ): RequestedMove { | |
| 124 | const pull = getPull(app.db, repo.id, requested.number); | |
| 125 | const issue = getIssueById(app.db, requested.pullId); | |
| 126 | if (!pull || !issue) return notMoved(requested, "no pull row"); | |
| 127 | if (getHold(app.db, pull.id) !== undefined) return notMoved(requested, "a person holds it"); | |
| 128 | if (handoverOf(app.db, repo, pull.id) !== undefined) | |
| 129 | return notMoved(requested, "handed to a person after the review rounds"); | |
| 130 | // Past the rounds the verdict is advice, and the pull lands as it is. | |
| 131 | if (reviewSummary(app.db, pull.id, pull.headOid).changesRequested === 0) | |
| 132 | return notMoved(requested, "past the review rounds; the verdict is advice"); | |
| 133 | if (activeRunFor(app.db, "agent", pull.id) !== undefined) | |
| 134 | return notMoved(requested, "a run is in flight"); | |
| 135 | if (startedSince(app, pull.id, requested.at)) | |
| 136 | return notMoved(requested, "a run since the verdict ended without a push"); | |
| 137 | if (builders.length === 0) return notMoved(requested, "no builder on the crew"); | |
| 138 | const assignee = issue.assigneeId; | |
| 139 | let builder = builders.find((b) => b.user.id === assignee); | |
| 140 | let assigned = false; | |
| 141 | if (builder === undefined) { | |
| 142 | // A person's name on it is a person holding it; an agent that does not | |
| 143 | // build here, or no name at all, is a pull without a builder. | |
| 144 | if (assignee !== null && getAgent(app.db, assignee) === undefined) | |
| 145 | return notMoved(requested, "assigned to a person"); | |
| 146 | builder = | |
| 147 | builders.find((b) => b.user.id === issue.authorId) ?? | |
| 148 | pickBuilder(issue, builders, new Map()); | |
| 149 | const by = repo.autonomyBy === null ? undefined : getUserById(app.db, repo.autonomyBy); | |
| 150 | if (builder === undefined || by === undefined) | |
| 151 | return notMoved(requested, "nobody to assign a builder as"); | |
| 152 | const done = assignIssue(app, by, repo, issue, builder.user); | |
| 153 | if (!done.ok) return notMoved(requested, `could not assign ${builder.user.username}: ${done.error}`); | |
| 154 | assigned = true; | |
| 155 | } | |
| 156 | const current = getIssueById(app.db, issue.id) ?? issue; | |
| 157 | // The assignment may already have queued the builder's run; either is the one. | |
| 158 | const run = | |
| 159 | queueAgentRun(app, repo, current, builder, "feedback") ?? | |
| 160 | activeRunFor(app.db, "agent", pull.id, builder.user.id); | |
| 161 | if (run === undefined) | |
| 162 | return notMoved(requested, `${builder.user.username}'s run was not queued (ceiling, cap or breaker)`); | |
| 163 | return { | |
| 164 | pull: requested.number, | |
| 165 | did: "queued feedback", | |
| 166 | builder: builder.user.username, | |
| 167 | assigned, | |
| 168 | }; | |
| 169 | } | |
| 170 | ||
| 171 | /** Whether a run other than a review started on the pull after the verdict. */ | |
| 172 | function startedSince(app: App, pullId: string, at: string): boolean { | |
| 173 | return ( | |
| 174 | app.db | |
| 175 | .query<{ n: number }, [string, string]>( | |
| 176 | `SELECT COUNT(*) AS n FROM run WHERE issue_id = ?1 AND kind = 'agent' | |
| 177 | AND reason != 'review' AND created_at > ?2 | |
| 178 | AND summary NOT LIKE 'skipped: %'`, | |
| 179 | ) | |
| 180 | .get(pullId, at)!.n > 0 | |
| 181 | ); | |
| 182 | } | |
| 183 | ||
| 184 | /** A move is always said; a why-not once, until it changes. */ | |
| 185 | function say(repo: Repo, requested: RequestedPull, move: RequestedMove): void { | |
| 186 | const where = { repo: `${repo.ownerName}/${repo.name}`, pull: requested.number }; | |
| 187 | const key = `${repo.id}:${requested.number}`; | |
| 188 | if (move.did === "not moved") { | |
| 189 | const line = `${requested.headOid}:${move.why}`; | |
| 190 | if (said.get(key) === line) return; | |
| 191 | said.set(key, line); | |
| 192 | log.info(`changes requested: not moved, ${move.why}`, where); | |
| 193 | return; | |
| 194 | } | |
| 195 | said.delete(key); | |
| 196 | log.info( | |
| 197 | move.did === "asked reviewer" | |
| 198 | ? "changes requested on an older head: reviewer asked again" | |
| 199 | : "changes requested on the head: feedback queued", | |
| 200 | move.did === "queued feedback" | |
| 201 | ? { ...where, builder: move.builder, ...(move.assigned ? { assigned: true } : {}) } | |
| 202 | : where, | |
| 203 | ); | |
| 204 | } |
src/crew/review.ts +11 −3
| @@ -284,13 +284,17 @@ export function waitsForReviewer( | ||
| 284 | 284 | * there is nothing to wait for — no reviewer on the crew — and `done` |
| 285 | 285 | * when a verdict is on the head. A run the pull's builder still has open |
| 286 | 286 | * does not stop a send — the review queues behind it. `held` when a person |
| 287 | * holds the pull: nothing is sent, whoever asks. | |
| 287 | * holds the pull: nothing is sent, whoever asks. A head pushed over a | |
| 288 | * stale "changes requested" (`stale`) is owed its review however long it | |
| 289 | * has been green: one that was never sent is sent, as in an autonomous | |
| 290 | * repository (#2172). | |
| 288 | 291 | */ |
| 289 | 292 | export async function sendToReview( |
| 290 | 293 | app: App, |
| 291 | 294 | repo: Repo, |
| 292 | 295 | pull: Pull, |
| 293 | 296 | now = new Date(), |
| 297 | { stale = false }: { readonly stale?: boolean } = {}, | |
| 294 | 298 | ): Promise<"none" | "done" | "sent" | "wait" | "lapsed" | "held"> { |
| 295 | 299 | if (getHold(app.db, pull.id) !== undefined) return "held"; |
| 296 | 300 | const reviewers = reviewersOf(app.db, repo.id); |
| @@ -301,8 +305,12 @@ export async function sendToReview( | ||
| 301 | 305 | const history = reviewAttempts(app.db, pull); |
| 302 | 306 | if (history === undefined) return "wait"; |
| 303 | 307 | const stage = stageOf(history, now); |
| 304 | // An autonomous repository sends a pull nobody reviewed whatever its age. | |
| 305 | const unreviewed = stage === "lapsed" && history.attempts.length === 0 && repo.autonomy === "autonomous"; | |
| 308 | // An autonomous repository sends a pull nobody reviewed whatever its age, | |
| 309 | // and so does any repository whose reviewer's verdict this head answers. | |
| 310 | const unreviewed = | |
| 311 | stage === "lapsed" && | |
| 312 | history.attempts.length === 0 && | |
| 313 | (repo.autonomy === "autonomous" || stale); | |
| 306 | 314 | if (stage === "wait" || (stage === "lapsed" && !unreviewed)) return stage; |
| 307 | 315 | const late = stage === "late" || unreviewed; |
| 308 | 316 | const issue = getIssueById(app.db, pull.id); |
src/flow/requested.ts added +64
| @@ -0,0 +1,64 @@ | ||
| 1 | /** | |
| 2 | * Pulls with changes requested, as /flow shows them (#2172): the ones | |
| 3 | * waiting on a builder (the verdict is on the head) apart from the ones | |
| 4 | * waiting on a reviewer (a push came since), each oldest first. Read from | |
| 5 | * `core/requested.ts`, the same rows the tick moves. | |
| 6 | * | |
| 7 | * A builder's wait starts at the verdict; a reviewer's at the head it has | |
| 8 | * not read yet, which is when that head's first run was queued (the | |
| 9 | * verdict's time when it has none). | |
| 10 | */ | |
| 11 | ||
| 12 | import type { Database } from "bun:sqlite"; | |
| 13 | import { requestedPulls, verdictIsStale } from "../core/requested.ts"; | |
| 14 | import { span } from "./pipeline.ts"; | |
| 15 | ||
| 16 | export interface WaitingPull { | |
| 17 | /** `owner/name`. */ | |
| 18 | readonly repo: string; | |
| 19 | readonly number: number; | |
| 20 | readonly since: string; | |
| 21 | } | |
| 22 | ||
| 23 | export interface RequestedWaits { | |
| 24 | readonly builder: readonly WaitingPull[]; | |
| 25 | readonly reviewer: readonly WaitingPull[]; | |
| 26 | } | |
| 27 | ||
| 28 | /** `publicOnly` leaves out private repositories, for a reader who must not learn their names. */ | |
| 29 | export function requestedWaits(db: Database, publicOnly = false): RequestedWaits { | |
| 30 | const builder: WaitingPull[] = []; | |
| 31 | const reviewer: WaitingPull[] = []; | |
| 32 | const headSeen = db.query<{ at: string | null }, [string, string]>( | |
| 33 | `SELECT MIN(created_at) AS at FROM run WHERE issue_id = ?1 AND commit_oid = ?2`, | |
| 34 | ); | |
| 35 | for (const p of requestedPulls(db)) { | |
| 36 | if (publicOnly && !p.isPublic) continue; | |
| 37 | if (!verdictIsStale(p)) { | |
| 38 | builder.push({ repo: p.repo, number: p.number, since: p.at }); | |
| 39 | continue; | |
| 40 | } | |
| 41 | const at = headSeen.get(p.pullId, p.headOid)?.at ?? null; | |
| 42 | reviewer.push({ repo: p.repo, number: p.number, since: at !== null && at > p.at ? at : p.at }); | |
| 43 | } | |
| 44 | const oldest = (a: WaitingPull, b: WaitingPull) => | |
| 45 | a.since.localeCompare(b.since) || a.number - b.number; | |
| 46 | return { builder: builder.sort(oldest), reviewer: reviewer.sort(oldest) }; | |
| 47 | } | |
| 48 | ||
| 49 | /** One sentence per kind of wait for /flow, or null when nothing waits on it. */ | |
| 50 | export function requestedSays( | |
| 51 | pulls: readonly WaitingPull[], | |
| 52 | on: "builder" | "reviewer", | |
| 53 | now = Date.now(), | |
| 54 | ): string | null { | |
| 55 | if (pulls.length === 0) return null; | |
| 56 | const list = pulls | |
| 57 | .slice(0, 5) | |
| 58 | .map((p) => `${p.repo}#${p.number} ${span(now - Date.parse(p.since))}`) | |
| 59 | .join(", "); | |
| 60 | const more = pulls.length > 5 ? `, and ${pulls.length - 5} more` : ""; | |
| 61 | const what = pulls.length === 1 ? "pull" : "pulls"; | |
| 62 | const verb = pulls.length === 1 ? "waits" : "wait"; | |
| 63 | return `${pulls.length} ${what} with changes requested ${verb} on a ${on}, oldest first: ${list}${more}.`; | |
| 64 | } |
src/http/mcp.ts +1 −1
| @@ -2073,7 +2073,7 @@ const TOOLS: readonly Tool[] = [ | ||
| 2073 | 2073 | { |
| 2074 | 2074 | name: "flow", |
| 2075 | 2075 | description: |
| 2076 | "The forge's pipeline, for a forge admin: ready, building, check queue, checking, review, approved, landed and live, each with how many wait, how many are being worked, when the oldest waiting one entered (oldestWaiting) and what limits it (capacity), and one sentence (says) naming the stage whose oldest has waited longest — the bottleneck. Beside it, throughput: per stage how many left it in the last hour (lastHour), in 24 hours (day) and in each of the last 24 hours (hours, 0 the last), the median ask→live in ms over the last 24 hours, and its own sentence (says). Also `conflicted`: pulls main moved under that the crew is bringing up to date, oldest first, with a sentence (says). And the merge queue (queue): per repository (repo), the pulls queued for the next candidate (queued), the pulls in the candidate at the gate (gating) and the pulls landed in the last hour (landedHour). Read-only; the knobs are the `capacity` tool. The same as /flow.", | |
| 2076 | "The forge's pipeline, for a forge admin: ready, building, check queue, checking, review, approved, landed and live, each with how many wait, how many are being worked, when the oldest waiting one entered (oldestWaiting) and what limits it (capacity), and one sentence (says) naming the stage whose oldest has waited longest — the bottleneck. Beside it, throughput: per stage how many left it in the last hour (lastHour), in 24 hours (day) and in each of the last 24 hours (hours, 0 the last), the median ask→live in ms over the last 24 hours, and its own sentence (says). Also `conflicted`: pulls main moved under that the crew is bringing up to date, oldest first, with a sentence (says). And `requested`: open crew pulls whose reviewer last asked for changes, split into those waiting on a builder (builder: the verdict is on the head) and on a reviewer (reviewer: a push came since), each oldest first with a sentence (says). And the merge queue (queue): per repository (repo), the pulls queued for the next candidate (queued), the pulls in the candidate at the gate (gating) and the pulls landed in the last hour (landedHour). Read-only; the knobs are the `capacity` tool. The same as /flow.", | |
| 2077 | 2077 | inputSchema: { type: "object", properties: {} }, |
| 2078 | 2078 | writes: false, |
| 2079 | 2079 | run(ctx, user) { |
src/system/telemetry.ts +7
| @@ -10,6 +10,7 @@ import { SOURCES } from "../boards/sources.ts"; | ||
| 10 | 10 | import { conflictedPulls, conflictedSays } from "../flow/conflicted.ts"; |
| 11 | 11 | import { readPipeline, verdict } from "../flow/pipeline.ts"; |
| 12 | 12 | import { readQueues } from "../flow/queue.ts"; |
| 13 | import { requestedSays, requestedWaits } from "../flow/requested.ts"; | |
| 13 | 14 | import { readThroughput, stageOut, throughputSays } from "../flow/throughput.ts"; |
| 14 | 15 | import type { Reader } from "../actions/telemetry.ts"; |
| 15 | 16 | import { agentView, live } from "./live.ts"; |
| @@ -25,9 +26,15 @@ export function flowFor(app: App, reader: Reader, now = new Date()) { | ||
| 25 | 26 | const flow = readThroughput(app.db, stages.map((s) => s.key), now); |
| 26 | 27 | const dup = systemFor(app, reader).duplicates; |
| 27 | 28 | const conflicted = conflictedPulls(app.db, reader.kind === "agent"); |
| 29 | const requested = requestedWaits(app.db, reader.kind === "agent"); | |
| 28 | 30 | return { |
| 29 | 31 | says: verdict(stages, now.getTime()), |
| 30 | 32 | conflicted: { says: conflictedSays(conflicted, now.getTime()), pulls: conflicted }, |
| 33 | // Pulls with changes requested, by whom they wait on (#2172). | |
| 34 | requested: { | |
| 35 | builder: { says: requestedSays(requested.builder, "builder", now.getTime()), pulls: requested.builder }, | |
| 36 | reviewer: { says: requestedSays(requested.reviewer, "reviewer", now.getTime()), pulls: requested.reviewer }, | |
| 37 | }, | |
| 31 | 38 | stages, |
| 32 | 39 | throughput: { |
| 33 | 40 | says: throughputSays(flow), |
src/tui/screens/flow.ts +11 −2
| @@ -4,7 +4,9 @@ | ||
| 4 | 4 | * stage of what left it in the last hour and in 24 hours — the numbers of |
| 5 | 5 | * `flow/pipeline.ts` and `flow/throughput.ts`, the same the page and the |
| 6 | 6 | * MCP `flow` tool read. Under them, each repository's merge queue (#2036): |
| 7 | * queued, gating, landed in the last hour (`flow/queue.ts`). Read-only; | |
| 7 | * queued, gating, landed in the last hour (`flow/queue.ts`); then the | |
| 8 | * conflicted pulls, and the pulls with changes requested waiting on a | |
| 9 | * builder and on a reviewer (`flow/requested.ts`, #2172). Read-only; | |
| 8 | 10 | * Enter, Esc or q closes it. |
| 9 | 11 | */ |
| 10 | 12 | |
| @@ -12,6 +14,7 @@ import type { Database } from "bun:sqlite"; | ||
| 12 | 14 | import { conflictedPulls, conflictedSays } from "../../flow/conflicted.ts"; |
| 13 | 15 | import { readPipeline, STAGE_LABEL, verdict } from "../../flow/pipeline.ts"; |
| 14 | 16 | import { readQueues } from "../../flow/queue.ts"; |
| 17 | import { requestedSays, requestedWaits } from "../../flow/requested.ts"; | |
| 15 | 18 | import { readThroughput, stageOut, throughputSays } from "../../flow/throughput.ts"; |
| 16 | 19 | import type { Ctx, Screen } from "../app/app.ts"; |
| 17 | 20 | import { binding } from "../app/keys.ts"; |
| @@ -27,7 +30,7 @@ export function flowLines( | ||
| 27 | 30 | db: Database, |
| 28 | 31 | now = new Date(), |
| 29 | 32 | dups = live().read().duplicates, |
| 30 | /** What an agent reader may see: its queue lines and conflicted pulls come already cut to public. */ | |
| 33 | /** What an agent reader may see: its queue lines, conflicted pulls and changes-requested waits come already cut to public. */ | |
| 31 | 34 | agent?: { queues: ReturnType<typeof readQueues> }, |
| 32 | 35 | ): string[] { |
| 33 | 36 | const stages = readPipeline(db, now); |
| @@ -35,6 +38,11 @@ export function flowLines( | ||
| 35 | 38 | const inner = WIDTH - 6; |
| 36 | 39 | const queues = agent?.queues ?? readQueues(db, now); |
| 37 | 40 | const conflicted = conflictedSays(conflictedPulls(db, agent !== undefined), now.getTime()); |
| 41 | const requested = requestedWaits(db, agent !== undefined); | |
| 42 | const waits = [ | |
| 43 | requestedSays(requested.builder, "builder", now.getTime()), | |
| 44 | requestedSays(requested.reviewer, "reviewer", now.getTime()), | |
| 45 | ].flatMap((says) => (says === null ? [] : ["", ...wrap(says, inner)])); | |
| 38 | 46 | return [ |
| 39 | 47 | ...wrap(verdict(stages, now.getTime()), inner), |
| 40 | 48 | "", |
| @@ -54,6 +62,7 @@ export function flowLines( | ||
| 54 | 62 | `${pad(truncate(q.repo, 30), 30)} ${pad(String(q.queued), 6, "right")} ${pad(String(q.gating), 6, "right")} ${pad(String(q.landedHour), 9, "right")}`, |
| 55 | 63 | )), |
| 56 | 64 | ...(conflicted === null ? [] : ["", ...wrap(conflicted, inner)]), |
| 65 | ...waits, | |
| 57 | 66 | "", |
| 58 | 67 | ...duplicateLines(dups, inner), |
| 59 | 68 | ]; |
src/web/pages/flow.ts +10
| @@ -17,11 +17,16 @@ | ||
| 17 | 17 | * next candidate, what the candidate at the gate carries, and what landed |
| 18 | 18 | * in the last hour — `flow/queue.ts`'s numbers, which the terminal's Flow |
| 19 | 19 | * command and the MCP `flow` tool read too. |
| 20 | * | |
| 21 | * Beside the conflicted pulls, the pulls with changes requested (#2172): | |
| 22 | * those waiting on a builder apart from those waiting on a reviewer, by | |
| 23 | * age — `flow/requested.ts`'s, the rows the tick moves. | |
| 20 | 24 | */ |
| 21 | 25 | |
| 22 | 26 | import { noteRead, telemetryReader } from "../../actions/telemetry.ts"; |
| 23 | 27 | import { queuesFor, systemFor } from "../../system/telemetry.ts"; |
| 24 | 28 | import { conflictedPulls, conflictedSays } from "../../flow/conflicted.ts"; |
| 29 | import { requestedSays, requestedWaits } from "../../flow/requested.ts"; | |
| 25 | 30 | import { |
| 26 | 31 | bottleneck, |
| 27 | 32 | readPipeline, |
| @@ -113,6 +118,9 @@ export function flowPage(ctx: Ctx): Response { | ||
| 113 | 118 | const landed = out.get("approved"); |
| 114 | 119 | const dups = systemFor(ctx.app, reader).duplicates; |
| 115 | 120 | const conflicted = conflictedSays(conflictedPulls(ctx.app.db, reader.kind === "agent"), now); |
| 121 | const requested = requestedWaits(ctx.app.db, reader.kind === "agent"); | |
| 122 | const onBuilder = requestedSays(requested.builder, "builder", now); | |
| 123 | const onReviewer = requestedSays(requested.reviewer, "reviewer", now); | |
| 116 | 124 | const listed = asked(ctx.url); |
| 117 | 125 | const shown = listed === null ? undefined : out.get(listed.key); |
| 118 | 126 | const body = html`<div class="page-head"> |
| @@ -130,6 +138,8 @@ export function flowPage(ctx: Ctx): Response { | ||
| 130 | 138 | <strong>${verdict(stages, now)}</strong> |
| 131 | 139 | </p> |
| 132 | 140 | ${conflicted === null ? "" : html`<p id="conflicted">${conflicted}</p>`} |
| 141 | ${onBuilder === null ? "" : html`<p id="requested-builder">${onBuilder}</p>`} | |
| 142 | ${onReviewer === null ? "" : html`<p id="requested-reviewer">${onReviewer}</p>`} | |
| 133 | 143 | <p id="throughput"> |
| 134 | 144 | landed |
| 135 | 145 | <a href="${outHref("approved", 0)}" data-landed-hour>${String(landed?.hours[0] ?? 0)}</a> |
test/requested.test.ts added +304
| @@ -0,0 +1,304 @@ | ||
| 1 | /** | |
| 2 | * A pull with changes requested always moves (#2172): a verdict on an | |
| 3 | * older head sends the reviewer back to the new green head; a verdict on | |
| 4 | * the current head queues one feedback run to the pull's builder, | |
| 5 | * assigning one when it has none. Both within one tick, and /flow says | |
| 6 | * which pulls wait on a builder and which on a reviewer. | |
| 7 | */ | |
| 8 | ||
| 9 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 10 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 11 | import { tmpdir } from "node:os"; | |
| 12 | import { join } from "node:path"; | |
| 13 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 14 | import { openPull } from "../src/actions/pulls.ts"; | |
| 15 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 16 | import { createApp, type App } from "../src/app.ts"; | |
| 17 | import { setCrewRole } from "../src/core/agents.ts"; | |
| 18 | import { getIssueById } from "../src/core/issues.ts"; | |
| 19 | import { getPull, type Pull } from "../src/core/pulls.ts"; | |
| 20 | import { getRepoById, type Repo } from "../src/core/repos.ts"; | |
| 21 | import { submitReview } from "../src/core/reviews.ts"; | |
| 22 | import { claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts"; | |
| 23 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; | |
| 24 | import { autonomyTick } from "../src/crew/autonomy.ts"; | |
| 25 | import { moveRequested } from "../src/crew/requested.ts"; | |
| 26 | import { requestedSays, requestedWaits } from "../src/flow/requested.ts"; | |
| 27 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 28 | import { createToken } from "../src/core/tokens.ts"; | |
| 29 | import { flowLines } from "../src/tui/screens/flow.ts"; | |
| 30 | import { flowFor } from "../src/system/telemetry.ts"; | |
| 31 | import { quietLogs } from "./helpers/globals.ts"; | |
| 32 | ||
| 33 | quietLogs(); | |
| 34 | ||
| 35 | const HOUR = 60 * 60_000; | |
| 36 | ||
| 37 | describe("a pull with changes requested always moves", () => { | |
| 38 | let dataDir = ""; | |
| 39 | let app: App; | |
| 40 | let servers: Servers; | |
| 41 | let alice: User; | |
| 42 | let builder: User; | |
| 43 | let reviewer: User; | |
| 44 | let repo: Repo; | |
| 45 | let n = 0; | |
| 46 | ||
| 47 | const fresh = () => getRepoById(app.db, repo.id)!; | |
| 48 | const reload = (pull: Pull) => getPull(app.db, repo.id, pull.number)!; | |
| 49 | const runs = (pull: Pull, reason: string) => | |
| 50 | listRuns(app.db, repo.id, { issueId: pull.id }).filter( | |
| 51 | (r) => r.kind === "agent" && r.reason === reason, | |
| 52 | ); | |
| 53 | const push = async (branch: string, base?: string) => { | |
| 54 | const c = await commitToBranch(app, alice, repo, { | |
| 55 | branch, | |
| 56 | ...(base === undefined ? {} : { base }), | |
| 57 | message: `${branch} ${++n}`, | |
| 58 | files: [{ path: `${branch}.md`, content: `${n}\n` }], | |
| 59 | }); | |
| 60 | if (!c.ok) throw new Error(JSON.stringify(c.error)); | |
| 61 | }; | |
| 62 | /** A builder's pull, opened on a branch of its own. */ | |
| 63 | const builderPull = async (): Promise<Pull> => { | |
| 64 | const branch = `b${++n}`; | |
| 65 | await push(branch, "main"); | |
| 66 | const o = await openPull(app, builder, repo, { | |
| 67 | title: branch, | |
| 68 | body: "", | |
| 69 | baseBranch: "main", | |
| 70 | headBranch: branch, | |
| 71 | }); | |
| 72 | if (!o.ok) throw new Error(String(o.error)); | |
| 73 | return o.value; | |
| 74 | }; | |
| 75 | /** The head's check, green `ago` milliseconds back. */ | |
| 76 | const green = (pull: Pull, ago: number) => { | |
| 77 | const run = createRun(app.db, { | |
| 78 | repoId: repo.id, | |
| 79 | kind: "check", | |
| 80 | issueId: pull.id, | |
| 81 | env: `pull-${pull.number}`, | |
| 82 | commitOid: pull.headOid, | |
| 83 | reason: "pull", | |
| 84 | }); | |
| 85 | claimRun(app.db, run.id); | |
| 86 | finishRun(app.db, run.id, { state: "ok", summary: "ok" }); | |
| 87 | const at = new Date(Date.now() - ago).toISOString(); | |
| 88 | app.db | |
| 89 | .query("UPDATE run SET created_at = ?2, finished_at = ?2 WHERE id = ?1") | |
| 90 | .run(run.id, at); | |
| 91 | }; | |
| 92 | const changesRequested = (pull: Pull) => { | |
| 93 | const r = submitReview(app.db, { | |
| 94 | issueId: pull.id, | |
| 95 | authorId: reviewer.id, | |
| 96 | state: "changes_requested", | |
| 97 | body: "Fix the one thing.", | |
| 98 | commitOid: pull.headOid, | |
| 99 | }); | |
| 100 | if (!r.ok) throw new Error(r.error); | |
| 101 | }; | |
| 102 | /** Shape 1: changes requested, then a push the reviewer was never asked about, green long ago. */ | |
| 103 | const staleShape = async () => { | |
| 104 | const pull = await builderPull(); | |
| 105 | green(pull, 4 * HOUR); | |
| 106 | changesRequested(pull); | |
| 107 | await push(pull.headBranch); | |
| 108 | const pushed = reload(pull); | |
| 109 | expect(pushed.headOid).not.toBe(pull.headOid); | |
| 110 | green(pushed, 3 * HOUR); | |
| 111 | return pushed; | |
| 112 | }; | |
| 113 | /** Shape 2: changes requested on the current head, no assignee, no run since. */ | |
| 114 | const currentShape = async () => { | |
| 115 | const pull = await builderPull(); | |
| 116 | green(pull, HOUR); | |
| 117 | changesRequested(pull); | |
| 118 | // An agent's pull starts assigned to its author; this one was let go. | |
| 119 | app.db.query("UPDATE issue SET assignee_id = NULL WHERE id = ?1").run(pull.id); | |
| 120 | return pull; | |
| 121 | }; | |
| 122 | const clearRuns = () => | |
| 123 | app.db.query("UPDATE run SET state = 'ok' WHERE kind = 'agent' AND state IN ('queued','running')").run(); | |
| 124 | ||
| 125 | beforeAll(async () => { | |
| 126 | dataDir = await mkdtemp(join(tmpdir(), "forge-requested-")); | |
| 127 | app = createApp({ dataDir, port: 0 }); | |
| 128 | servers = await serve(app); | |
| 129 | const made = await createUser(app.db, { | |
| 130 | trusted: true, | |
| 131 | username: "alice", | |
| 132 | email: "[email protected]", | |
| 133 | password: "a-good-passphrase", | |
| 134 | adminIfFirst: true, | |
| 135 | }); | |
| 136 | if (!made.ok) throw new Error(made.error); | |
| 137 | alice = made.value; | |
| 138 | const r = await createRepo(app, alice, { name: "garden", visibility: "private" }); | |
| 139 | if (!r.ok) throw new Error(String(r.error)); | |
| 140 | repo = r.value; | |
| 141 | await push("main"); | |
| 142 | for (const name of ["mason", "quill"]) { | |
| 143 | const agent = await createAgent(app, alice, { username: name }); | |
| 144 | if (!agent.ok) throw new Error(String(agent.error)); | |
| 145 | expect(addToCrew(app, alice, repo, name).ok).toBe(true); | |
| 146 | } | |
| 147 | builder = getUserByName(app.db, "mason")!; | |
| 148 | reviewer = getUserByName(app.db, "quill")!; | |
| 149 | setCrewRole(app.db, repo.id, reviewer.id, "reviewer"); | |
| 150 | // Assisted: a lapsed head is not sent for being autonomous, only for answering a verdict. | |
| 151 | app.db | |
| 152 | .query("UPDATE repo SET autonomy = 'assisted', autonomy_by = ?2, autonomy_slots = 3 WHERE id = ?1") | |
| 153 | .run(repo.id, alice.id); | |
| 154 | }, 30_000); | |
| 155 | ||
| 156 | afterAll(async () => { | |
| 157 | servers.stop(); | |
| 158 | app.close(); | |
| 159 | await rm(dataDir, { recursive: true, force: true }); | |
| 160 | }); | |
| 161 | ||
| 162 | test("a verdict on an older head: the reviewer is asked about the new green head", async () => { | |
| 163 | const pull = await staleShape(); | |
| 164 | const moves = await moveRequested(app, fresh(), 0); | |
| 165 | expect(moves).toContainEqual({ pull: pull.number, did: "asked reviewer" }); | |
| 166 | const sent = runs(pull, "review"); | |
| 167 | expect(sent).toHaveLength(1); | |
| 168 | expect(sent[0]!.actorId).toBe(reviewer.id); | |
| 169 | // Asked once: the review in flight is waited for, not sent again. | |
| 170 | const again = await moveRequested(app, fresh(), 0); | |
| 171 | expect(again.find((m) => m.pull === pull.number)?.did).toBe("not moved"); | |
| 172 | expect(runs(pull, "review")).toHaveLength(1); | |
| 173 | clearRuns(); | |
| 174 | }); | |
| 175 | ||
| 176 | test("a verdict on an older head whose new head is not green waits for the check", async () => { | |
| 177 | const pull = await builderPull(); | |
| 178 | changesRequested(pull); | |
| 179 | await push(pull.headBranch); | |
| 180 | const moves = await moveRequested(app, fresh(), 3); | |
| 181 | expect(moves.find((m) => m.pull === pull.number)).toEqual({ | |
| 182 | pull: pull.number, | |
| 183 | did: "not moved", | |
| 184 | why: "the new head is not green yet", | |
| 185 | }); | |
| 186 | app.db.query("UPDATE issue SET state = 'closed' WHERE id = ?1").run(pull.id); | |
| 187 | }); | |
| 188 | ||
| 189 | test("a verdict on the current head: one feedback run to the builder, assigned when the pull had none", async () => { | |
| 190 | const pull = await currentShape(); | |
| 191 | expect(getIssueById(app.db, pull.id)!.assigneeId).toBeNull(); | |
| 192 | // No slot, no run. | |
| 193 | const full = await moveRequested(app, fresh(), 0); | |
| 194 | expect(full.find((m) => m.pull === pull.number)).toEqual({ | |
| 195 | pull: pull.number, | |
| 196 | did: "not moved", | |
| 197 | why: "waits for a builder slot", | |
| 198 | }); | |
| 199 | expect(runs(pull, "feedback")).toHaveLength(0); | |
| 200 | ||
| 201 | const moves = await moveRequested(app, fresh(), 1); | |
| 202 | expect(moves).toContainEqual({ | |
| 203 | pull: pull.number, | |
| 204 | did: "queued feedback", | |
| 205 | builder: "mason", | |
| 206 | assigned: true, | |
| 207 | }); | |
| 208 | expect(getIssueById(app.db, pull.id)!.assigneeId).toBe(builder.id); | |
| 209 | expect(runs(pull, "feedback")).toHaveLength(1); | |
| 210 | // One in flight per pull. | |
| 211 | const again = await moveRequested(app, fresh(), 1); | |
| 212 | expect(again.find((m) => m.pull === pull.number)).toEqual({ | |
| 213 | pull: pull.number, | |
| 214 | did: "not moved", | |
| 215 | why: "a run is in flight", | |
| 216 | }); | |
| 217 | // A run that answered nothing is not asked again. | |
| 218 | clearRuns(); | |
| 219 | const after = await moveRequested(app, fresh(), 1); | |
| 220 | expect(after.find((m) => m.pull === pull.number)?.did).toBe("not moved"); | |
| 221 | expect(runs(pull, "feedback")).toHaveLength(1); | |
| 222 | app.db.query("UPDATE issue SET state = 'closed' WHERE id = ?1").run(pull.id); | |
| 223 | }); | |
| 224 | ||
| 225 | test("an approval after the request: nothing to move", async () => { | |
| 226 | const pull = await currentShape(); | |
| 227 | submitReview(app.db, { | |
| 228 | issueId: pull.id, | |
| 229 | authorId: reviewer.id, | |
| 230 | state: "approved", | |
| 231 | commitOid: pull.headOid, | |
| 232 | }); | |
| 233 | const moves = await moveRequested(app, fresh(), 3); | |
| 234 | expect(moves.find((m) => m.pull === pull.number)).toBeUndefined(); | |
| 235 | app.db.query("UPDATE issue SET state = 'closed' WHERE id = ?1").run(pull.id); | |
| 236 | }); | |
| 237 | ||
| 238 | test("both shapes move within one tick, and /flow tells them apart", async () => { | |
| 239 | // Every earlier pull is settled: only these two are in play. | |
| 240 | app.db.query("UPDATE issue SET state = 'closed' WHERE repo_id = ?1 AND kind = 'pull'").run(repo.id); | |
| 241 | const stale = await staleShape(); | |
| 242 | const current = await currentShape(); | |
| 243 | ||
| 244 | const waits = requestedWaits(app.db); | |
| 245 | expect(waits.reviewer.map((p) => p.number)).toEqual([stale.number]); | |
| 246 | expect(waits.builder.map((p) => p.number)).toEqual([current.number]); | |
| 247 | // A reader kept from private names (an agent on /flow) sees neither. | |
| 248 | const outside = requestedWaits(app.db, true); | |
| 249 | expect([...outside.builder, ...outside.reviewer]).toEqual([]); | |
| 250 | expect(requestedSays(waits.builder, "builder")).toStartWith( | |
| 251 | `1 pull with changes requested waits on a builder, oldest first: alice/garden#${current.number} `, | |
| 252 | ); | |
| 253 | ||
| 254 | const signin = await fetch(`${servers.url}/signin`, { | |
| 255 | method: "POST", | |
| 256 | redirect: "manual", | |
| 257 | headers: { "content-type": "application/x-www-form-urlencoded", origin: servers.url }, | |
| 258 | body: new URLSearchParams({ username: "alice", password: "a-good-passphrase" }), | |
| 259 | }); | |
| 260 | const cookie = (signin.headers.get("set-cookie") ?? "").split(";")[0] ?? ""; | |
| 261 | const page = await (await fetch(`${servers.url}/flow`, { headers: { cookie } })).text(); | |
| 262 | expect(page).toContain(`id="requested-builder"`); | |
| 263 | expect(page).toContain(`waits on a reviewer, oldest first: alice/garden#${stale.number} `); | |
| 264 | ||
| 265 | // The terminal's Flow dialog says the same; an agent's is cut to public. | |
| 266 | const dialog = flowLines(app.db).join(" "); | |
| 267 | expect(dialog).toContain("waits on a builder"); | |
| 268 | expect(dialog).toContain("waits on a reviewer"); | |
| 269 | const agentDialog = flowLines(app.db, new Date(), undefined, { queues: [] }).join(" "); | |
| 270 | expect(agentDialog).not.toContain("changes requested"); | |
| 271 | ||
| 272 | // So does the MCP flow tool. | |
| 273 | const res = await fetch(`${servers.url}/mcp`, { | |
| 274 | method: "POST", | |
| 275 | headers: { | |
| 276 | authorization: `Bearer ${createToken(app.db, alice.id, "t", "ro").token}`, | |
| 277 | "content-type": "application/json", | |
| 278 | accept: "application/json, text/event-stream", | |
| 279 | }, | |
| 280 | body: JSON.stringify({ | |
| 281 | jsonrpc: "2.0", | |
| 282 | id: 1, | |
| 283 | method: "tools/call", | |
| 284 | params: { name: "flow", arguments: {} }, | |
| 285 | }), | |
| 286 | }); | |
| 287 | const tool = (await res.json()) as { result: { content: { text: string }[] } }; | |
| 288 | const read = JSON.parse(tool.result.content[0]!.text) as { | |
| 289 | requested: Record<"builder" | "reviewer", { says: string | null; pulls: { number: number }[] }>; | |
| 290 | }; | |
| 291 | expect(read.requested.builder.pulls.map((p) => p.number)).toEqual([current.number]); | |
| 292 | expect(read.requested.reviewer.pulls.map((p) => p.number)).toEqual([stale.number]); | |
| 293 | expect(read.requested.reviewer.says).toContain(`garden#${stale.number}`); | |
| 294 | // An agent reader's telemetry has the private repository's pulls cut out. | |
| 295 | const agentFlow = flowFor(app, { kind: "agent" }).requested; | |
| 296 | expect([...agentFlow.builder.pulls, ...agentFlow.reviewer.pulls]).toEqual([]); | |
| 297 | expect(agentFlow.builder.says).toBeNull(); | |
| 298 | ||
| 299 | await autonomyTick(app, new Date(), async () => 0); | |
| 300 | expect(runs(stale, "review")).toHaveLength(1); | |
| 301 | expect(runs(current, "feedback")).toHaveLength(1); | |
| 302 | expect(runs(current, "feedback")[0]!.actorId).toBe(builder.id); | |
| 303 | }); | |
| 304 | }); |
new above
offline
Nit: nothing reads
reviewerId; the tick and /flow only use the head, the verdict oid and its time. Drop the column and the field, or use it, for example by naming the reviewer in the tick's log line.