Public: anyone can read this; the crew can change it.
On a new forge a finished change goes live by itself: today it waits forever… #11668
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/11666-on-a-new-forge-a-finished-change-goes-li → main 8 commits fork 6 threads · 6 unresolved
26 files
changed +249
−46
docs/features/reviews-off-lands-on-green.md added +24
| @@ -0,0 +1,24 @@ | ||
| 1 | # Reviews are something an owner turns on (#11666) | |
| 2 | ||
| 3 | A green pull in an app whose owner has **not** asked for reviews, and where no reviewer is seated, lands by itself. No review is sent and nothing waits for one. | |
| 4 | ||
| 5 | **This changes the default.** Before, a green agent pull with no reviewer seated waited for a person forever. Now, with **Require review** off (the default) and no reviewer on the crew, it lands once its check passes. A new forge's first app has reviews off, so its first change goes live on green. An owner who wants a second look before anything lands turns reviews on, as below. | |
| 6 | ||
| 7 | ## How to turn reviews on | |
| 8 | ||
| 9 | Either one keeps every agent pull waiting for an approving review: | |
| 10 | ||
| 11 | - In the app's settings, turn on **Require review**. Agent pulls then wait for an approving review, from a person or a seated reviewer. | |
| 12 | - Or seat a reviewer on the crew. Reviews stay on wherever a reviewer is seated, whatever the setting says. | |
| 13 | ||
| 14 | ## What still stops a pull with reviews off | |
| 15 | ||
| 16 | A conflict, a red check, a hold, the access wall, and a `changes requested` verdict on the head. Only the wait for a review is gone. The pull's page, the terminal and the MCP `pull` tool read "Next: queue, green, reviews are off: it lands". | |
| 17 | ||
| 18 | ## One rule | |
| 19 | ||
| 20 | `reviewsOff` in `src/crew/review.ts`: `requireReview` is off **and** no reviewer is seated. The gate (`reviewed`), the review waits (`reviewWaitsFor`) and the next-actor line (`nextAsking`, used by `nextOfPull` and `stepsOfStored`) all ask it, so they cannot drift. | |
| 21 | ||
| 22 | ## Cost | |
| 23 | ||
| 24 | The seated reviewers are read only when the answer would otherwise be "waiting for the reviewer", and only when **Require review** is off: at most once per pull page, at most once per batch of pulls. The pull page loads no crew otherwise, so its budget in `test/pages-budget.test.ts` is one higher for that read; the batch's is unchanged. |
src/crew/autonomy.ts +8 −3
| @@ -152,7 +152,7 @@ import { recordQueueFlakes } from "../checks/flake.ts"; | ||
| 152 | 152 | import { readVitals, vitalsMisses, vitalsWords } from "../flow/vitals.ts"; |
| 153 | 153 | import { moveRequested, tallyRequested, tallyWords } from "./requested.ts"; |
| 154 | 154 | import { passLanded } from "./box_pass.ts"; |
| 155 | import { reviewersOf, sendToReview, sweepReviews } from "./review.ts"; | |
| 155 | import { reviewersForRepos, reviewsOff, sendToReview, sweepReviews } from "./review.ts"; | |
| 156 | 156 | import { lastWaits, noteWaits, tallyWaits, type Wait, waitWords } from "./waits.ts"; |
| 157 | 157 | import { bootCounts } from "../kernel/boots.ts"; |
| 158 | 158 | |
| @@ -1951,12 +1951,17 @@ export async function reviewed( | ||
| 1951 | 1951 | // An agent's pull ships only on an approving review — the crew's reviewer |
| 1952 | 1952 | // or a person's. With no reviewer, or a review that lapsed, it waits for |
| 1953 | 1953 | // a person (`waitsForPerson` puts it where a person looks); never by lapse. |
| 1954 | // An app whose owner has not asked for reviews lands a green pull without | |
| 1955 | // one (#11666); a verdict already on the head was handled above. | |
| 1956 | const seated = reviewersForRepos(app.db, [repo.id]); | |
outdated
Duplicates the condition in review.ts and next_actor.ts; extract one named predicate.
outdated
High: unsafe default. This passes every green agent pull, including from an agent that was prompt-injected, to production unreviewed, for every app with default settings. Gate it behind an explicit owner opt-in (a separate setting), not the absence of |
||
| 1957 | if (reviewsOff(app.db, repo, seated)) return "pass"; | |
| 1958 | const reviewers = seated.get(repo.id)?.length ?? 0; | |
| 1954 | 1959 | if (pull.authorIsAgent) { |
| 1955 | if (reviewersOf(app.db, repo.id).length > 0) await sendToReview(app, repo, pull); | |
| 1960 | if (reviewers > 0) await sendToReview(app, repo, pull); | |
| 1956 | 1961 | return "hold"; |
| 1957 | 1962 | } |
| 1958 | 1963 | // A person's own pull is not held for a review that never comes. |
| 1959 | if (reviewersOf(app.db, repo.id).length === 0) return "pass"; | |
| 1964 | if (reviewers === 0) return "pass"; | |
| 1960 | 1965 | const stage = await sendToReview(app, repo, pull); |
| 1961 | 1966 | return stage === "lapsed" ? "lapsed" : stage === "none" ? "pass" : "hold"; |
| 1962 | 1967 | } |
src/crew/review.ts +20 −2
| @@ -133,6 +133,20 @@ export function pickReviewer( | ||
| 133 | 133 | export const reviewersOf = (db: Database, repoId: string): Agent[] => |
| 134 | 134 | reviewersForRepos(db, [repoId]).get(repoId) ?? []; |
| 135 | 135 | |
| 136 | /** | |
| 137 | * Whether reviews are off for a repository (#11666): its owner has not asked | |
| 138 | * for them (`requireReview`) and no reviewer is seated. A green pull then | |
| 139 | * lands without one, and nothing waits for one. The one rule: the gate, the | |
| 140 | * waits and the next-actor line all ask it here. The seated reviewers are | |
| 141 | * read only when the owner has not asked; a caller that already read them | |
| 142 | * (`reviewersForRepos`) passes them in. | |
| 143 | */ | |
| 144 | export const reviewsOff = ( | |
| 145 | db: Database, | |
| 146 | repo: Pick<Repo, "id" | "requireReview">, | |
outdated
Clean: |
||
| 147 | seated?: ReadonlyMap<string, readonly Agent[]>, | |
| 148 | ): boolean => !repo.requireReview && ((seated ?? reviewersForRepos(db, [repo.id])).get(repo.id)?.length ?? 0) === 0; | |
| 149 | ||
| 136 | 150 | /** `reviewersOf` for many repositories at once, by repo; one with none is absent. */ |
| 137 | 151 | export const reviewersForRepos = (db: Database, repoIds: readonly string[]): Map<string, Agent[]> => |
| 138 | 152 | crewWithRole(db, repoIds, "reviewer"); |
| @@ -374,7 +388,11 @@ export function reviewWaitsFor( | ||
| 374 | 388 | handovers?: ReadonlyMap<string, Handover>, |
| 375 | 389 | ): Map<string, ReviewWaits> { |
| 376 | 390 | const out = new Map<string, ReviewWaits>(); |
| 377 | const agents = pulls.filter((p) => p.pull.authorIsAgent); | |
| 391 | // An app with reviews off waits on none (#11666). | |
| 392 | const seated = reviewersForRepos(db, pulls.map((p) => p.repo.id)); | |
| 393 | const agents = pulls.filter( | |
| 394 | (p) => p.pull.authorIsAgent && !reviewsOff(db, p.repo, seated), | |
| 395 | ); | |
| 378 | 396 | const held = holdsFor(db, agents.map((p) => p.pull.id)); |
| 379 | 397 | const unheld = agents.filter((p) => !held.has(p.pull.id)); |
| 380 | 398 | const seen = reviewSummariesFor(db, unheld.map((p) => p.pull)); |
| @@ -388,7 +406,7 @@ export function reviewWaitsFor( | ||
| 388 | 406 | // answer differently. |
| 389 | 407 | const over = handovers ?? handoversOf(db, open.map((p) => ({ id: p.pull.id, repo: p.repo }))); |
| 390 | 408 | const histories = reviewAttemptsFor(db, open.map((p) => p.pull)); |
| 391 | const reviewers = reviewersForRepos(db, open.map((p) => p.repo.id)); | |
| 409 | const reviewers = seated; | |
| 392 | 410 | for (const { repo, pull } of open) { |
| 393 | 411 | const handover = over.get(pull.id); |
| 394 | 412 | const history = histories.get(pull.id); |
src/flow/next_actor.ts +32 −9
| @@ -9,6 +9,8 @@ import { getHold } from "../core/holds.ts"; | ||
| 9 | 9 | import type { Pull } from "../core/pulls.ts"; |
| 10 | 10 | import { checkSummary, type RunState } from "../core/runs.ts"; |
| 11 | 11 | import { reviewSummary } from "../core/reviews.ts"; |
| 12 | import type { Repo } from "../core/repos.ts"; | |
| 13 | import { reviewsOff } from "../crew/review.ts"; | |
| 12 | 14 | import { wallAsk, wallWords } from "./wall_wait.ts"; |
| 13 | 15 | |
| 14 | 16 | export type Actor = "builder" | "reviewer" | "queue" | "person"; |
| @@ -30,6 +32,8 @@ export interface NextInput { | ||
| 30 | 32 | readonly pushedSince: number; |
| 31 | 33 | /** What the access wall holds this head for, in plain words (#7847): a person decides it. */ |
| 32 | 34 | readonly wall?: readonly string[] | null; |
| 35 | /** Reviews are off for the app (`reviewsOff`): a green pull lands without one. */ | |
| 36 | readonly reviewOff?: boolean; | |
| 33 | 37 | } |
| 34 | 38 | |
| 35 | 39 | const WAITING = { |
| @@ -51,28 +55,47 @@ export function nextActor(i: NextInput): Next { | ||
| 51 | 55 | if (i.check === "ok") return { who: "queue", why: "approved and green" }; |
| 52 | 56 | return { who: "queue", why: `approved, ${WAITING[i.check]}` }; |
outdated
Wording: the line says the pull lands by itself even when no one has read it. Once the default is opt-in, make this read the same way. Keep it plain (docs/WRITING.md). |
||
| 53 | 57 | } |
| 58 | if (i.reviewOff === true) { | |
| 59 | if (i.check === "ok") return { who: "queue", why: "green, reviews are off: it lands" }; | |
| 60 | return { who: "queue", why: i.check === "none" ? "no check yet" : WAITING[i.check] }; | |
| 61 | } | |
| 54 | 62 | return { |
| 55 | 63 | who: "reviewer", |
| 56 | 64 | why: i.pushedSince > 0 ? "pushed since the last review" : "no review yet", |
| 57 | 65 | }; |
| 58 | 66 | } |
| 59 | 67 | |
| 68 | /** | |
| 69 | * `nextActor`, asking whether reviews are off only when the answer would be | |
| 70 | * a wait on the reviewer: that is the one answer reviews being off changes, | |
| 71 | * so the seated reviewers are read then and only then (#11666). | |
| 72 | */ | |
| 73 | export function nextAsking(i: NextInput, off: () => boolean): Next { | |
| 74 | const next = nextActor(i); | |
| 75 | return next.who === "reviewer" && off() ? nextActor({ ...i, reviewOff: true }) : next; | |
| 76 | } | |
| 77 | ||
| 60 | 78 | /** The next actor of an open pull at `headOid`; null once it is merged or closed. */ |
| 61 | 79 | export function nextOfPull( |
| 62 | 80 | db: Database, |
| 63 | 81 | pull: Pull, |
| 64 | 82 | live: { readonly headOid: string; readonly conflicts: number }, |
| 83 | /** The pull's repository; without one, reviews count as asked for. */ | |
| 84 | repo: Pick<Repo, "id" | "requireReview"> | undefined, | |
| 65 | 85 | ): Next | null { |
outdated
Two extra queries per pull page render (budget raised to cover it) and a third spelling of the rule. Take the repo and reviewer count from the caller. |
||
| 66 | 86 | if (pull.state !== "open" || pull.mergedAt !== null) return null; |
| 67 | 87 | const reviews = reviewSummary(db, pull.id, live.headOid); |
| 68 | 88 | const hold = getHold(db, pull.id); |
| 69 | return nextActor({ | |
| 70 | held: hold === undefined ? null : { by: hold.heldByName, why: hold.why }, | |
| 71 | conflicted: live.conflicts > 0, | |
| 72 | check: checkSummary(db, pull.id, live.headOid).state, | |
| 73 | changesRequested: reviews.changesRequested, | |
| 74 | approvals: reviews.approvals, | |
| 75 | pushedSince: reviews.stale, | |
| 76 | wall: wallWords(db, pull.id, live.headOid), | |
| 77 | }); | |
| 89 | return nextAsking( | |
| 90 | { | |
| 91 | held: hold === undefined ? null : { by: hold.heldByName, why: hold.why }, | |
| 92 | conflicted: live.conflicts > 0, | |
| 93 | check: checkSummary(db, pull.id, live.headOid).state, | |
| 94 | changesRequested: reviews.changesRequested, | |
| 95 | approvals: reviews.approvals, | |
| 96 | pushedSince: reviews.stale, | |
| 97 | wall: wallWords(db, pull.id, live.headOid), | |
| 98 | }, | |
| 99 | () => repo !== undefined && reviewsOff(db, repo), | |
| 100 | ); | |
| 78 | 101 | } |
src/flow/path.ts +18 −11
| @@ -19,7 +19,8 @@ import { getPull, type Pull } from "../core/pulls.ts"; | ||
| 19 | 19 | import type { Repo } from "../core/repos.ts"; |
| 20 | 20 | import { reviewSummariesFor, reviewSummary } from "../core/reviews.ts"; |
| 21 | 21 | import { checksForHeads, checkSummary, type RunState } from "../core/runs.ts"; |
| 22 | import { nextActor, nextOfPull, type Next } from "./next_actor.ts"; | |
| 22 | import { reviewsOff } from "../crew/review.ts"; | |
| 23 | import { nextAsking, nextOfPull, type Next } from "./next_actor.ts"; | |
| 23 | 24 | import { wallWordsOf } from "./wall_wait.ts"; |
| 24 | 25 | |
| 25 | 26 | export const STEPS = ["check", "review", "land", "live"] as const; |
| @@ -107,7 +108,7 @@ export function stepOfPull( | ||
| 107 | 108 | repo: Repo, |
| 108 | 109 | pull: Pull, |
| 109 | 110 | live: { readonly headOid: string; readonly conflicts: number }, |
| 110 | next: Next | null = pull.state === "open" && pull.mergedAt === null ? nextOfPull(db, pull, live) : null, | |
| 111 | next: Next | null = pull.state === "open" && pull.mergedAt === null ? nextOfPull(db, pull, live, repo) : null, | |
| 111 | 112 | ): PathStep | null { |
| 112 | 113 | if (pull.mergedAt !== null) { |
| 113 | 114 | const deploys = pull.baseBranch === repo.defaultBranch && getAppByRepo(db, repo.id) !== undefined; |
| @@ -171,19 +172,25 @@ export function stepsOfStored(db: Database, repo: Repo, pulls: readonly Pull[]): | ||
| 171 | 172 | const checks = checksForHeads(db, new Map(open.map((p) => [p.id, p.headOid]))); |
| 172 | 173 | const holds = holdsFor(db, ids); |
| 173 | 174 | const walls = latestDecisionsFor(db, ids, "land"); |
| 175 | // Asked at most once for the batch, and only if a pull would wait on a review (#11666). | |
| 176 | let off: boolean | undefined; | |
| 177 | const reviewOff = (): boolean => (off ??= reviewsOff(db, repo)); | |
| 174 | 178 | for (const pull of open) { |
| 175 | 179 | const r = reviews.get(pull.id)!; |
| 176 | 180 | const check = checks.get(pull.id)?.[0]?.state ?? "none"; |
| 177 | 181 | const hold = holds.get(pull.id); |
| 178 | const next = nextActor({ | |
| 179 | held: hold === undefined ? null : { by: hold.heldByName, why: hold.why }, | |
| 180 | conflicted: false, | |
| 181 | check, | |
| 182 | changesRequested: r.changesRequested, | |
| 183 | approvals: r.approvals, | |
| 184 | pushedSince: r.stale, | |
| 185 | wall: wallWordsOf(walls.get(pull.id), pull.headOid), | |
| 186 | }); | |
| 182 | const next = nextAsking( | |
| 183 | { | |
| 184 | held: hold === undefined ? null : { by: hold.heldByName, why: hold.why }, | |
| 185 | conflicted: false, | |
| 186 | check, | |
| 187 | changesRequested: r.changesRequested, | |
| 188 | approvals: r.approvals, | |
| 189 | pushedSince: r.stale, | |
| 190 | wall: wallWordsOf(walls.get(pull.id), pull.headOid), | |
| 191 | }, | |
| 192 | reviewOff, | |
| 193 | ); | |
| 187 | 194 | out.set(pull.id, pathStep({ state: "open", check, approvals: r.approvals, next })); |
| 188 | 195 | } |
| 189 | 196 | const merged = pulls.filter((p) => p.mergedAt !== null); |
src/flow/why.ts +1 −1
| @@ -213,7 +213,7 @@ export function whyPull( | ||
| 213 | 213 | ): Why { |
| 214 | 214 | if (pull.mergedAt !== null) return { cause: null, settled: "merged" }; |
| 215 | 215 | if (pull.state !== "open") return { cause: null, settled: "closed" }; |
| 216 | const next = nextOfPull(app.db, pull, live); | |
| 216 | const next = nextOfPull(app.db, pull, live, repo); | |
| 217 | 217 | if (next === null) return { cause: null }; |
| 218 | 218 | const check = checkSummary(app.db, pull.id, live.headOid).state; |
| 219 | 219 | const wall = next.who === "person" ? wallWords(app.db, pull.id, live.headOid) : null; |
src/http/mcp/tools/pull.ts +1 −1
| @@ -74,7 +74,7 @@ export const tool: Tool = { | ||
| 74 | 74 | next: nextOfPull(ctx.app.db, pull, { |
| 75 | 75 | headOid: pull.headOid, |
| 76 | 76 | conflicts: state?.conflicts?.length ?? 0, |
| 77 | }), | |
| 77 | }, repo), | |
| 78 | 78 | step: stepOfPull(ctx.app.db, repo, pull, { |
| 79 | 79 | headOid: pull.headOid, |
| 80 | 80 | conflicts: state?.conflicts?.length ?? 0, |
src/tui/screens/pull.ts +1 −1
| @@ -399,7 +399,7 @@ export function pullScreen( | ||
| 399 | 399 | : nextOfPull(f.db, row, { |
| 400 | 400 | headOid: row.headOid || (facts?.headOid ?? ""), |
| 401 | 401 | conflicts: facts?.conflicts.length ?? 0, |
| 402 | }); | |
| 402 | }, settings); | |
| 403 | 403 | step = |
| 404 | 404 | row === undefined || settings === undefined |
| 405 | 405 | ? null |
src/web/studio/change.ts +1 −1
| @@ -226,7 +226,7 @@ export function changeHead( | ||
| 226 | 226 | pull: Pull, |
| 227 | 227 | live: { readonly headOid: string; readonly conflicts: number } | null = null, |
| 228 | 228 | ): Html { |
| 229 | const next = live === null ? null : nextOfPull(ctx.app.db, pull, live); | |
| 229 | const next = live === null ? null : nextOfPull(ctx.app.db, pull, live, repo); | |
| 230 | 230 | const step = |
| 231 | 231 | live === null && pull.mergedAt === null |
| 232 | 232 | ? null |
test/asks-counts.test.ts +2
| @@ -39,6 +39,7 @@ import { handoverOf, handoversOf } from "../src/crew/guard.ts"; | ||
| 39 | 39 | import { serve, type Servers } from "../src/http/server.ts"; |
| 40 | 40 | import { id } from "../src/kernel/id.ts"; |
| 41 | 41 | import { quiet, statements } from "./helpers/statements.ts"; |
| 42 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 42 | 43 | |
| 43 | 44 | const ZERO = "0".repeat(40); |
| 44 | 45 | |
| @@ -94,6 +95,7 @@ describe("home's asks are read grouped, and once", () => { | ||
| 94 | 95 | description: name, |
| 95 | 96 | }); |
| 96 | 97 | if (!made.ok) throw new Error(`seed repo ${name}: ${made.error}`); |
| 98 | askForReviews(app.db, made.value.id); | |
| 97 | 99 | for (const agent of [builder, reviewer]) addCrew(app.db, made.value.id, agent.id, owner.id); |
| 98 | 100 | seatAs(app.db, made.value.id, reviewer.id, "reviewer"); |
| 99 | 101 | app.db.query("UPDATE repo SET autonomy = ?2 WHERE id = ?1").run(made.value.id, autonomy); |
test/crew-now-review-waits.test.ts +27 −5
| @@ -9,6 +9,10 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | ||
| 9 | 9 | import { mkdtemp, rm } from "node:fs/promises"; |
| 10 | 10 | import { tmpdir } from "node:os"; |
| 11 | 11 | import { join } from "node:path"; |
| 12 | import { reviewed } from "../src/crew/autonomy.ts"; | |
| 13 | import { reviewWaitsFor } from "../src/crew/review.ts"; | |
| 14 | import { getRepoById } from "../src/core/repos.ts"; | |
| 15 | import { getPull } from "../src/core/pulls.ts"; | |
| 12 | 16 | import { crewNow } from "../src/actions/crew_now.ts"; |
| 13 | 17 | import { createAgent } from "../src/actions/crew.ts"; |
| 14 | 18 | import { openPull } from "../src/actions/pulls.ts"; |
| @@ -20,6 +24,7 @@ import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | ||
| 20 | 24 | import { getUserByName, type User } from "../src/core/users.ts"; |
| 21 | 25 | import { seedAdmin } from "./helpers/admin.ts"; |
| 22 | 26 | import { quietLogs } from "./helpers/globals.ts"; |
| 27 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 23 | 28 | |
| 24 | 29 | quietLogs(); |
| 25 | 30 | |
| @@ -32,20 +37,20 @@ describe("crewNow's review waits", () => { | ||
| 32 | 37 | let n = 0; |
| 33 | 38 | |
| 34 | 39 | /** An agent's pull whose check went green two hours ago, with no reviewer on the crew. */ |
| 35 | const greenAgentPull = async () => { | |
| 40 | const greenAgentPull = async (into: Repo = repo) => { | |
| 36 | 41 | const branch = `b${++n}`; |
| 37 | const c = await commitToBranch(app, ada, repo, { | |
| 42 | const c = await commitToBranch(app, ada, into, { | |
| 38 | 43 | branch, |
| 39 | 44 | base: "main", |
| 40 | 45 | message: branch, |
| 41 | 46 | files: [{ path: `${branch}.md`, content: "x\n" }], |
| 42 | 47 | }); |
| 43 | 48 | if (!c.ok) throw new Error(JSON.stringify(c.error)); |
| 44 | const o = await openPull(app, ada, repo, { title: `pull ${branch}`, body: "", baseBranch: "main", headBranch: branch }); | |
| 49 | const o = await openPull(app, ada, into, { title: `pull ${branch}`, body: "", baseBranch: "main", headBranch: branch }); | |
| 45 | 50 | if (!o.ok) throw new Error(String(o.error)); |
| 46 | 51 | app.db.query("UPDATE issue SET author_id = ?2 WHERE id = ?1").run(o.value.id, agentId); |
| 47 | 52 | const run = createRun(app.db, { |
| 48 | repoId: repo.id, | |
| 53 | repoId: into.id, | |
| 49 | 54 | kind: "check", |
| 50 | 55 | issueId: o.value.id, |
| 51 | 56 | env: `pull-${o.value.number}`, |
| @@ -82,7 +87,9 @@ describe("crewNow's review waits", () => { | ||
| 82 | 87 | ada = made.value; |
| 83 | 88 | const r = await createRepo(app, ada, { name: "garden", visibility: "private" }); |
| 84 | 89 | if (!r.ok) throw new Error(String(r.error)); |
| 85 | repo = r.value; | |
| 90 | // Reviews are something an owner turns on (#11666). | |
| 91 | askForReviews(app.db, r.value.id); | |
| 92 | repo = getRepoById(app.db, r.value.id)!; | |
| 86 | 93 | createAppRecord(app.db, { repoId: repo.id, slug: "garden", access: "private" }); |
| 87 | 94 | const seeded = await commitToBranch(app, ada, repo, { |
| 88 | 95 | message: "seed", |
| @@ -113,4 +120,19 @@ describe("crewNow's review waits", () => { | ||
| 113 | 120 | expect(nine.rows.filter((r) => r.why === "review")).toHaveLength(9); |
| 114 | 121 | expect(nine.statements).toBe(one.statements); |
| 115 | 122 | }, 30_000); |
| 123 | ||
| 124 | test("in a repository with reviews off, a green agent pull waits on nobody and the gate passes it (#11666)", async () => { | |
| 125 | const made = await createRepo(app, ada, { name: "meadow", visibility: "private" }); | |
| 126 | if (!made.ok) throw new Error(String(made.error)); | |
| 127 | const meadow = made.value; | |
| 128 | const seeded = await commitToBranch(app, ada, meadow, { message: "seed", files: [{ path: "README.md", content: "# meadow\n" }] }); | |
| 129 | if (!seeded.ok) throw new Error(JSON.stringify(seeded.error)); | |
| 130 | const pull = getPull(app.db, meadow.id, (await greenAgentPull(meadow)).number)!; | |
| 131 | expect(await reviewed(app, meadow, pull)).toBe("pass"); | |
| 132 | expect(reviewWaitsFor(app.db, [{ repo: meadow, pull }]).get(pull.id)).toBeUndefined(); | |
| 133 | // The same pull's twin where reviews are asked for still waits, as before. | |
| 134 | const asked = getPull(app.db, repo.id, (await greenAgentPull()).number)!; | |
| 135 | expect(await reviewed(app, repo, asked)).toBe("hold"); | |
| 136 | expect(reviewWaitsFor(app.db, [{ repo, pull: asked }]).get(asked.id)?.person?.why).toBe("no reviewer"); | |
| 137 | }, 30_000); | |
| 116 | 138 | }); |
test/crew-red-outside-diff.test.ts +2
| @@ -30,6 +30,7 @@ import type { User } from "../src/core/users.ts"; | ||
| 30 | 30 | import type { Decider } from "../src/crew/decide.ts"; |
| 31 | 31 | import { appendRunLog, readRunLog } from "../src/runs/log.ts"; |
| 32 | 32 | import { startStandInCrew, stopStandInCrew, type StandInCrew } from "./helpers/crew-standin.ts"; |
| 33 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 33 | 34 | |
| 34 | 35 | const IMPACT = new Map([ |
| 35 | 36 | ["test/people.test.ts", ["src/people.ts"]], |
| @@ -83,6 +84,7 @@ describe("the crew, with a red outside the diff", () => { | ||
| 83 | 84 | (app.config as { decider?: Decider }).decider = decider; |
| 84 | 85 | // Parking is autonomy's: the repository runs on its own, on alice's word. |
| 85 | 86 | app.db.run("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?1 WHERE id = ?2", [alice.id, repo.id]); |
| 87 | askForReviews(app.db, repo.id); | |
| 86 | 88 | const checkout = mkdtempSync(join(tmpdir(), "forge-outside-diff-")); |
| 87 | 89 | writeFileSync(join(checkout, IMPACT_FILE), formatImpact(IMPACT)); |
| 88 | 90 | expect(keepImpact(app.config.dataDir, repo.id, checkout)).toBe("impact map kept: 3 tests"); |
test/follow-sources.test.ts +2
| @@ -42,6 +42,7 @@ import { runGit } from "../src/git/env.ts"; | ||
| 42 | 42 | import { repoDir } from "../src/git/repos.ts"; |
| 43 | 43 | import { serve } from "../src/http/server.ts"; |
| 44 | 44 | import { mirrorOutbound, quietLogs } from "./helpers/globals.ts"; |
| 45 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 45 | 46 | |
| 46 | 47 | quietLogs(); |
| 47 | 48 | mirrorOutbound({ allow: ["127.0.0.1"] }); |
| @@ -163,6 +164,7 @@ describe("following several sources", () => { | ||
| 163 | 164 | /** A repository whose main holds f.txt = `mainText`, and a commit tree builder. */ |
| 164 | 165 | async function repoWith(name: string, mainText: string) { |
| 165 | 166 | const r = ok(await createRepo(app, alice, { name, visibility: "public" })); |
| 167 | askForReviews(app.db, r.id); | |
| 166 | 168 | const d = repoDir(app.reposRoot, r.ownerName, r.name); |
| 167 | 169 | const git = async (args: string[], input?: string) => |
| 168 | 170 | ok(await runGit(args, { cwd: d, env: ident, ...(input === undefined ? {} : { stdin: input }) })).trim(); |
test/helpers/reviews.ts added +13
| @@ -0,0 +1,13 @@ | ||
| 1 | /** | |
| 2 | * Reviews are something an owner turns on (#11666): with them off and no | |
| 3 | * reviewer seated, a green pull lands by itself. A test whose expectation is | |
| 4 | * a wait on a review asks for reviews here, in one place, rather than | |
| 5 | * writing the column itself. | |
| 6 | */ | |
| 7 | ||
| 8 | import type { Database } from "bun:sqlite"; | |
| 9 | ||
| 10 | /** Turn `requireReview` on (or, with `on` false, off) for a repository. */ | |
| 11 | export function askForReviews(db: Database, repoId: string, on = true): void { | |
| 12 | db.query("UPDATE repo SET require_review = ?2 WHERE id = ?1").run(repoId, on ? 1 : 0); | |
| 13 | } |
test/mcp-flow-why.test.ts +2
| @@ -19,6 +19,7 @@ import { movingRow } from "../src/tui/screens/about.ts"; | ||
| 19 | 19 | import { CAUSES, firstCause, type WhyFacts } from "../src/flow/why.ts"; |
| 20 | 20 | import { serve, type Servers } from "../src/http/server.ts"; |
| 21 | 21 | import { promote } from "./helpers/members.ts"; |
| 22 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 22 | 23 | |
| 23 | 24 | describe("the list of causes", () => { |
| 24 | 25 | const cases: [string, WhyFacts][] = [ |
| @@ -238,6 +239,7 @@ describe("MCP flow about a pull", () => { | ||
| 238 | 239 | if (!made.ok) throw new Error(String(made.error)); |
| 239 | 240 | repo = made.value; |
| 240 | 241 | await put("main", "top\nbase\n"); |
| 242 | askForReviews(app.db, repo.id); | |
| 241 | 243 | const bot = await createAgent(app, alice, { username: "bot" }); |
| 242 | 244 | if (!bot.ok) throw new Error(String(bot.error)); |
| 243 | 245 | expect(addToCrew(app, alice, repo, "bot").ok).toBe(true); |
test/mcp-pulls-pile.test.ts +3
| @@ -17,6 +17,7 @@ import { createUser, getUserByName } from "../src/core/users.ts"; | ||
| 17 | 17 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; |
| 18 | 18 | import { serve, type Servers } from "../src/http/server.ts"; |
| 19 | 19 | import { quietLogs } from "./helpers/globals.ts"; |
| 20 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 20 | 21 | |
| 21 | 22 | quietLogs(); |
| 22 | 23 | |
| @@ -65,6 +66,8 @@ beforeAll(async () => { | ||
| 65 | 66 | })); |
| 66 | 67 | }; |
| 67 | 68 | const pub = ok(await createRepo(app.db, app.reposRoot, { ownerId: ada.id, name: "svc", visibility: "public" })); |
| 69 | // Reviews are something an owner turns on (#11666): with them off these pulls would land. | |
| 70 | askForReviews(app.db, pub.id); | |
| 68 | 71 | const priv = ok(await createRepo(app.db, app.reposRoot, { ownerId: ada.id, name: "vault", visibility: "private" })); |
| 69 | 72 | for (const repo of [pub, priv]) |
| 70 | 73 | ok(await commitToBranch(app, ada, repo, { message: "seed", files: [{ path: "a.txt", content: "a\n" }] })); |
test/mcp.test.ts +2
| @@ -35,6 +35,7 @@ import { showBoard } from "../src/actions/boards.ts"; | ||
| 35 | 35 | import { panelData } from "../src/boards/panel_data.ts"; |
| 36 | 36 | import { listVia } from "./mcp-list.ts"; |
| 37 | 37 | import { adminOpenapi } from "./helpers/openapi-doc.ts"; |
| 38 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 38 | 39 | |
| 39 | 40 | /** tools/list's order: the tools by name, then `report`. */ |
| 40 | 41 | const sortedThenReport = (names: string[]): string[] => [ |
| @@ -120,6 +121,7 @@ describe("the forge over MCP", () => { | ||
| 120 | 121 | }); |
| 121 | 122 | if (!made.ok) throw new Error(String(made.error)); |
| 122 | 123 | game = made.value; |
| 124 | askForReviews(app.db, game.id); | |
| 123 | 125 | await commitToBranch(app, alice, game, { |
| 124 | 126 | message: "seed", |
| 125 | 127 | files: [{ path: "README.md", content: "# game\n" }], |
test/next_actor.test.ts +23 −1
| @@ -1,5 +1,5 @@ | ||
| 1 | 1 | import { describe, expect, test } from "bun:test"; |
| 2 | import { nextActor, type NextInput } from "../src/flow/next_actor.ts"; | |
| 2 | import { nextActor, nextAsking, type NextInput } from "../src/flow/next_actor.ts"; | |
| 3 | 3 | |
| 4 | 4 | const base: NextInput = { |
| 5 | 5 | held: null, |
| @@ -25,9 +25,31 @@ const cases: [string, Partial<NextInput>, string, string][] = [ | ||
| 25 | 25 | // The access wall (#7847): a person decides, however approved and green. |
| 26 | 26 | ["walled, approved and green", { approvals: 1, check: "ok", wall: ["anyone signed out could read pack records"] }, "person", "the access wall holds it for a person to decide: anyone signed out could read pack records. Allow it (merge) or send it back"], |
| 27 | 27 | ["walled, sent back", { approvals: 1, check: "ok", changesRequested: 1, wall: ["anyone signed out could read pack records"] }, "builder", "changes were asked on this head"], |
| 28 | // Reviews off (#11666): nobody waits for a review, but a verdict on the head still counts. | |
| 29 | ["reviews off, green", { check: "ok", reviewOff: true }, "queue", "green, reviews are off: it lands"], | |
| 30 | ["reviews off, no check", { reviewOff: true }, "queue", "no check yet"], | |
| 31 | ["reviews off, running", { check: "running", reviewOff: true }, "queue", "its check is running"], | |
| 32 | ["reviews off, changes asked", { changesRequested: 1, check: "ok", reviewOff: true }, "builder", "changes were asked on this head"], | |
| 33 | ["reviews off, red", { check: "failed", reviewOff: true }, "builder", "its check is red"], | |
| 28 | 34 | ]; |
| 29 | 35 | |
outdated
Mid-file aliased imports; merge into the existing imports and the cases table. |
||
| 30 | 36 | describe("nextActor", () => { |
| 31 | 37 | for (const [name, input, who, why] of cases) |
| 32 | 38 | test(name, () => expect(nextActor({ ...base, ...input })).toEqual({ who: who as never, why })); |
| 33 | 39 | }); |
| 40 | ||
| 41 | describe("nextAsking (#11666)", () => { | |
| 42 | test("asks whether reviews are off only when the pull would wait on the reviewer", () => { | |
| 43 | let asked = 0; | |
| 44 | const off = () => (asked++, true); | |
| 45 | expect(nextAsking({ ...base, check: "ok", approvals: 1 }, off)).toEqual({ who: "queue", why: "approved and green" }); | |
| 46 | expect(nextAsking({ ...base, check: "failed" }, off).who).toBe("builder"); | |
| 47 | expect(asked).toBe(0); | |
| 48 | expect(nextAsking({ ...base, check: "ok" }, off)).toEqual({ who: "queue", why: "green, reviews are off: it lands" }); | |
| 49 | expect(asked).toBe(1); | |
| 50 | }); | |
| 51 | ||
| 52 | test("with reviews asked for, the reviewer still acts next", () => { | |
| 53 | expect(nextAsking({ ...base, check: "ok" }, () => false)).toEqual({ who: "reviewer", why: "no review yet" }); | |
| 54 | }); | |
| 55 | }); |
test/pages-budget.test.ts +1 −1
| @@ -37,7 +37,7 @@ const DEFAULT = { statements: 60, git: 8 }; | ||
| 37 | 37 | const HEAVIER: Record<string, { statements?: number; git?: number }> = { |
| 38 | 38 | "/:owner/:name/issues/:number": { statements: 70 }, |
| 39 | 39 | "/:owner/:name/next": { statements: 64 }, |
| 40 | "/:owner/:name/pulls/:number": { statements: 110 }, // +1: the header Talk count (#10327); +1: the landing line (#10860) | |
| 40 | "/:owner/:name/pulls/:number": { statements: 111 }, // +1: the header Talk count (#10327); +1: the landing line (#10860); +1: whether a reviewer is seated, read only when Require review is off and the pull would otherwise wait on one; the page loads no crew besides (#11666) | |
| 41 | 41 | "/board": { statements: 70 }, // +6: the money line (#510) — one settings read, then the month's and the 30 days' meter, storage and ledger reads; constant in the number of apps |
| 42 | 42 | "/settings/capacity": { statements: 71 }, |
| 43 | 43 | }; |
test/path-steps-batch.test.ts +4 −1
| @@ -18,6 +18,7 @@ import { stepOfStored, stepsOfStored } from "../src/flow/path.ts"; | ||
| 18 | 18 | import { quietLogs } from "./helpers/globals.ts"; |
| 19 | 19 | import { statements } from "./helpers/statements.ts"; |
| 20 | 20 | import { expectWork } from "./helpers/work.ts"; |
| 21 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 21 | 22 | |
| 22 | 23 | quietLogs(); |
| 23 | 24 | |
| @@ -67,7 +68,9 @@ beforeAll(async () => { | ||
| 67 | 68 | bob = await user("bob"); |
| 68 | 69 | const made = await createRepo(app, alice, { name: "steps", visibility: "public" }); |
| 69 | 70 | if (!made.ok) throw new Error(String(made.error)); |
| 70 | repo = made.value; | |
| 71 | // These pulls wait on a review, as reviews asked for say (#11666). | |
| 72 | askForReviews(app.db, made.value.id); | |
| 73 | repo = { ...made.value, requireReview: true }; | |
| 71 | 74 | const record = createAppRecord(app.db, { repoId: repo.id, slug: "steps", access: "public" }); |
| 72 | 75 | if (!record.ok) throw new Error(record.error); |
| 73 | 76 | }); |
test/pipeline-path.test.ts +2
| @@ -15,6 +15,7 @@ import { getRepoById } from "../src/core/repos.ts"; | ||
| 15 | 15 | import { pathStep, stepOfPull, STEPS } from "../src/flow/path.ts"; |
| 16 | 16 | import { serve, type Servers } from "../src/http/server.ts"; |
| 17 | 17 | import { quietLogs } from "./helpers/globals.ts"; |
| 18 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 18 | 19 | |
| 19 | 20 | quietLogs(); |
| 20 | 21 | |
| @@ -90,6 +91,7 @@ beforeAll(async () => { | ||
| 90 | 91 | for (const name of ["open", "merged"]) { |
| 91 | 92 | const repo = await createRepo(app, alice, { name, visibility: "public" }); |
| 92 | 93 | if (!repo.ok) throw new Error(String(repo.error)); |
| 94 | askForReviews(app.db, repo.value.id); | |
| 93 | 95 | for (const [branch, file] of [[undefined, "forge.json"], ["topic", "t.txt"]] as const) { |
| 94 | 96 | const done = await commitToBranch(app, alice, repo.value, { |
| 95 | 97 | ...(branch === undefined ? {} : { branch }), |
test/pull-next-line.test.ts +2
| @@ -10,6 +10,7 @@ import { createSession } from "../src/core/sessions.ts"; | ||
| 10 | 10 | import { createUser, type User } from "../src/core/users.ts"; |
| 11 | 11 | import { serve, type Servers } from "../src/http/server.ts"; |
| 12 | 12 | import { quietLogs } from "./helpers/globals.ts"; |
| 13 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 13 | 14 | |
| 14 | 15 | quietLogs(); |
| 15 | 16 | |
| @@ -45,6 +46,7 @@ beforeAll(async () => { | ||
| 45 | 46 | for (const name of ["open", "merged"]) { |
| 46 | 47 | const repo = await createRepo(app, alice, { name, visibility: "public" }); |
| 47 | 48 | if (!repo.ok) throw new Error(String(repo.error)); |
| 49 | askForReviews(app.db, repo.value.id); | |
| 48 | 50 | for (const [branch, file] of [[undefined, "forge.json"], ["topic", "t.txt"]] as const) { |
| 49 | 51 | const done = await commitToBranch(app, alice, repo.value, { |
| 50 | 52 | ...(branch === undefined ? {} : { branch }), |
test/pull-next-tui.test.ts +29 −6
| @@ -5,6 +5,7 @@ import { join } from "node:path"; | ||
| 5 | 5 | import { openPull } from "../src/actions/pulls.ts"; |
| 6 | 6 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| 7 | 7 | import { createApp, type App } from "../src/app.ts"; |
| 8 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 8 | 9 | import { createUser, type User } from "../src/core/users.ts"; |
| 9 | 10 | import { createForgeApp } from "../src/tui/forge.ts"; |
| 10 | 11 | import { pullScreen } from "../src/tui/screens/pull.ts"; |
| @@ -12,6 +13,7 @@ import { local } from "../src/tui/screens/where.ts"; | ||
| 12 | 13 | import { headlessTerminal } from "../src/tui/term/terminal.ts"; |
| 13 | 14 | import { quietLogs } from "./helpers/globals.ts"; |
| 14 | 15 | import { settleApp } from "./helpers/tui.ts"; |
| 16 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 15 | 17 | |
| 16 | 18 | quietLogs(); |
| 17 | 19 | |
| @@ -39,9 +41,11 @@ afterAll(async () => { | ||
| 39 | 41 | await rm(dataDir, { recursive: true, force: true }); |
| 40 | 42 | }); |
| 41 | 43 | |
| 42 | test("the terminal's pull screen says who acts next, as the page does", async () => { | |
| 43 | const repo = await createRepo(app, alice, { name: "lib", visibility: "public" }); | |
| 44 | /** A repository with a `topic` pull open; reviews are asked for only when `reviews` says so (#11666). */ | |
| 45 | async function openTopic(name: string, reviews: boolean) { | |
| 46 | const repo = await createRepo(app, alice, { name, visibility: "public" }); | |
| 44 | 47 | if (!repo.ok) throw new Error(String(repo.error)); |
| 48 | askForReviews(app.db, repo.value.id, reviews); | |
| 45 | 49 | for (const branch of ["main", "topic"]) { |
| 46 | 50 | const done = await commitToBranch(app, alice, repo.value, { |
| 47 | 51 | branch, |
| @@ -56,6 +60,11 @@ test("the terminal's pull screen says who acts next, as the page does", async () | ||
| 56 | 60 | headBranch: "topic", |
| 57 | 61 | }); |
| 58 | 62 | if (!opened.ok) throw new Error(String(opened.error)); |
| 63 | return { repo: repo.value, pull: opened.value }; | |
| 64 | } | |
| 65 | ||
| 66 | /** The pull screen's text once it says `wanted`, then the screen is stopped. */ | |
| 67 | async function screenSaying(repo: Awaited<ReturnType<typeof openTopic>>, wanted: string): Promise<string> { | |
| 59 | 68 | const { app: tui, session } = createForgeApp( |
| 60 | 69 | app, |
| 61 | 70 | { sshUsername: "alice", remoteAddress: "127.0.0.1", fingerprint: "SHA256:alice", user: alice }, |
| @@ -63,16 +72,30 @@ test("the terminal's pull screen says who acts next, as the page does", async () | ||
| 63 | 72 | ); |
| 64 | 73 | tui.start(); |
| 65 | 74 | try { |
| 66 | tui.ctx.push(pullScreen(session, tui.ctx, local(session, repo.value), opened.value.number)); | |
| 75 | tui.ctx.push(pullScreen(session, tui.ctx, local(session, repo.repo), repo.pull.number)); | |
| 67 | 76 | const end = Date.now() + 20_000; |
| 68 | while (!tui.screen.toText().includes("next: reviewer, no review yet")) { | |
| 77 | while (!tui.screen.toText().includes(wanted)) { | |
| 69 | 78 | if (Date.now() > end) throw new Error(tui.screen.toText()); |
| 70 | 79 | await settleApp(tui); |
| 71 | 80 | await Bun.sleep(20); |
| 72 | 81 | } |
| 73 | // The page's step line, too: this pull is at review. | |
| 74 | if (!tui.screen.toText().includes("step: Review — ")) throw new Error(tui.screen.toText()); | |
| 82 | return tui.screen.toText(); | |
| 75 | 83 | } finally { |
| 76 | 84 | tui.stop(); |
| 77 | 85 | } |
| 86 | } | |
| 87 | ||
| 88 | test("the terminal's pull screen says who acts next, as the page does", async () => { | |
| 89 | const text = await screenSaying(await openTopic("lib", true), "next: reviewer, no review yet"); | |
| 90 | // The page's step line, too: this pull is at review. | |
| 91 | if (!text.includes("step: Review — ")) throw new Error(text); | |
| 92 | }); | |
| 93 | ||
| 94 | test("with reviews off, a green pull's screen says it lands by itself, as the page does (#11666)", async () => { | |
| 95 | const topic = await openTopic("lib-off", false); | |
| 96 | const run = createRun(app.db, { repoId: topic.repo.id, kind: "check", issueId: topic.pull.id, commitOid: topic.pull.headOid }); | |
| 97 | claimRun(app.db, run.id); | |
| 98 | finishRun(app.db, run.id, { state: "ok", summary: "green" }); | |
| 99 | const text = await screenSaying(topic, "next: queue, green, reviews are off: it lands"); | |
| 100 | if (text.includes("next: reviewer")) throw new Error(text); | |
| 78 | 101 | }); |
test/review-gate.test.ts +17 −1
| @@ -6,7 +6,7 @@ | ||
| 6 | 6 | * before. |
| 7 | 7 | */ |
| 8 | 8 | import { seatAs } from "./helpers/seat.ts"; |
| 9 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 9 | import { afterAll, afterEach, beforeAll, describe, expect, test } from "bun:test"; | |
| 10 | 10 | import { mkdtemp, rm } from "node:fs/promises"; |
| 11 | 11 | import { tmpdir } from "node:os"; |
| 12 | 12 | import { join } from "node:path"; |
| @@ -28,6 +28,7 @@ import { createUser, getUserByName, type User } from "../src/core/users.ts"; | ||
| 28 | 28 | import { landIfGreen, settleAgentRun } from "../src/crew/autonomy.ts"; |
| 29 | 29 | import { sendToReview, waitsForPerson } from "../src/crew/review.ts"; |
| 30 | 30 | import { askLinesFor } from "../src/web/studio/asks.ts"; |
| 31 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 31 | 32 | |
| 32 | 33 | describe("the review gate", () => { |
| 33 | 34 | let dataDir: string; |
| @@ -37,6 +38,10 @@ describe("the review gate", () => { | ||
| 37 | 38 | let quill: User; |
| 38 | 39 | let repo: Repo; |
| 39 | 40 | const fresh = () => getRepoById(app.db, repo.id)!; |
| 41 | /** Whether the owner asks for reviews (#11666); each test starts with them off. */ | |
| 42 | const askReviews = (on: boolean) => | |
| 43 | askForReviews(app.db, repo.id, on); | |
| 44 | afterEach(() => askReviews(false)); | |
| 40 | 45 | const reviewRuns = (pull: Pull) => |
| 41 | 46 | listRuns(app.db, repo.id, { issueId: pull.id }).filter( |
| 42 | 47 | (r) => r.reason === "review", |
| @@ -203,6 +208,7 @@ describe("the review gate", () => { | ||
| 203 | 208 | test("with no reviewer on the crew an agent's pull waits for a person and says why", async () => { |
| 204 | 209 | seatAs(app.db, repo.id, quill.id, "builder"); |
| 205 | 210 | const pull = await greenPull(mason, "no-reviewer"); |
| 211 | askReviews(true); // with them off this pull would land (#11666) | |
| 206 | 212 | expect(await landIfGreen(app, fresh(), pull)).toBe(false); |
| 207 | 213 | expect(reviewRuns(pull)).toHaveLength(0); |
| 208 | 214 | expect(waitsForPerson(app.db, repo.id, getPull(app.db, repo.id, pull.number)!)?.why).toBe("no reviewer"); |
| @@ -210,6 +216,16 @@ describe("the review gate", () => { | ||
| 210 | 216 | expect(await landIfGreen(app, fresh(), pull)).toBe(true); |
| 211 | 217 | }); |
| 212 | 218 | |
| 219 | test("with reviews off, a green agent pull lands; changes asked on its head still hold it (#11666)", async () => { | |
| 220 | seatAs(app.db, repo.id, quill.id, "builder"); | |
| 221 | const landing = await greenPull(mason, "reviews-off"); | |
| 222 | expect(await landIfGreen(app, fresh(), landing)).toBe(true); | |
| 223 | const asked = await greenPull(mason, "reviews-off-changes"); | |
| 224 | submitReview(app.db, { issueId: asked.id, authorId: alice.id, state: "changes_requested", commitOid: asked.headOid }); | |
| 225 | expect(await landIfGreen(app, fresh(), asked)).toBe(false); | |
| 226 | expect(getPull(app.db, repo.id, asked.number)?.mergedAt).toBeNull(); | |
| 227 | }); | |
| 228 | ||
| 213 | 229 | test("a person's own pull is unchanged: a review that lapses does not hold it; it lands, and says so", async () => { |
| 214 | 230 | seatAs(app.db, repo.id, quill.id, "reviewer"); |
| 215 | 231 | const pull = await greenPull(alice, "person-unanswered"); |
test/review-waits.test.ts +11 −1
| @@ -6,7 +6,7 @@ | ||
| 6 | 6 | * who only reads. |
| 7 | 7 | */ |
| 8 | 8 | |
| 9 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 9 | import { afterAll, afterEach, beforeAll, describe, expect, test } from "bun:test"; | |
| 10 | 10 | import { mkdtemp, rm } from "node:fs/promises"; |
| 11 | 11 | import { tmpdir } from "node:os"; |
| 12 | 12 | import { join } from "node:path"; |
| @@ -23,6 +23,7 @@ import { landIfGreen } from "../src/crew/autonomy.ts"; | ||
| 23 | 23 | import { serve, type Servers } from "../src/http/server.ts"; |
| 24 | 24 | import { askLinesFor } from "../src/web/studio/asks.ts"; |
| 25 | 25 | import { quietLogs } from "./helpers/globals.ts"; |
| 26 | import { askForReviews } from "./helpers/reviews.ts"; | |
| 26 | 27 | |
| 27 | 28 | quietLogs(); |
| 28 | 29 | |
| @@ -35,6 +36,12 @@ describe("an agent pull that waits for a person's review says so", () => { | ||
| 35 | 36 | let alice: User; |
| 36 | 37 | let bob: User; |
| 37 | 38 | let repo: Repo; |
| 39 | /** Whether the owner asks for reviews (#11666), in the row and in the copy the tests pass on. */ | |
| 40 | const setReviews = (on: boolean) => { | |
| 41 | askForReviews(app.db, repo.id, on); | |
| 42 | repo = { ...repo, requireReview: on }; | |
| 43 | }; | |
| 44 | afterEach(() => setReviews(true)); | |
| 38 | 45 | let aliceCookie = ""; |
| 39 | 46 | let bobCookie = ""; |
| 40 | 47 | let aliceRo = ""; |
| @@ -141,6 +148,7 @@ describe("an agent pull that waits for a person's review says so", () => { | ||
| 141 | 148 | }); |
| 142 | 149 | if (!made.ok) throw new Error(String(made.error)); |
| 143 | 150 | repo = made.value; |
| 151 | setReviews(true); | |
| 144 | 152 | await commitToBranch(app, alice, repo, { |
| 145 | 153 | message: "seed", |
| 146 | 154 | files: [{ path: "README.md", content: "# engine\n" }], |
| @@ -180,6 +188,8 @@ describe("an agent pull that waits for a person's review says so", () => { | ||
| 180 | 188 | |
| 181 | 189 | test("with no reviewer the agent's pull does not land, and a person's own does", async () => { |
| 182 | 190 | expect(await landIfGreen(app, repo, waiting)).toBe(false); |
| 191 | // A person's own pull lands on green in an app whose owner has reviews off (#11666). | |
| 192 | setReviews(false); | |
| 183 | 193 | expect(await landIfGreen(app, repo, own)).toBe(true); |
| 184 | 194 | }); |
| 185 | 195 |
test/seat-slots.test.ts +1 −1
| @@ -80,7 +80,7 @@ beforeAll(async () => { | ||
| 80 | 80 | const id = (n: string) => getUserByName(app.db, n)!.id; |
| 81 | 81 | seatAs(app.db, repo.id, id("atlas"), "planner"); |
| 82 | 82 | seatAs(app.db, repo.id, id("judge"), "reviewer"); |
| 83 | ok(await updateRepoSettings(app, alice, repo, { autonomy: "autonomous", autonomyDirection: "Make the yard rounder.", autonomyCeilingUsd: 10, autonomySlots: 2, autonomyBy: alice.id })); | |
| 83 | ok(await updateRepoSettings(app, alice, repo, { autonomy: "autonomous", requireReview: true, autonomyDirection: "Make the yard rounder.", autonomyCeilingUsd: 10, autonomySlots: 2, autonomyBy: alice.id })); | |
| 84 | 84 | const res = await fetch(`${servers.url}/signin`, { method: "POST", body: new URLSearchParams({ username: "alice", password: PASSWORD }), redirect: "manual" }); |
| 85 | 85 | cookie = (res.headers.get("set-cookie") ?? "").split(";")[0]!; |
| 86 | 86 | admin = createToken(app.db, alice.id, "admin", "admin").token; |
new above
offline
The doc confirms the default flips for every app with reviews unasked. Make landing-by-itself an explicit setting, and say in this doc how to turn it on.