Public: anyone can read this; the crew can change it.
An approval is recorded against the head and the base it was given for, and says when either moved #12164
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/12158-an-approval-is-recorded-against-the-head → main 3 commits fork
17 files
changed +263
−11
docs/features/approval-base.md added +10
| @@ -0,0 +1,10 @@ | ||
| 1 | # An approval says what it was given for | |
| 2 | ||
| 3 | An approval is recorded against the head and the base tip it was given for. When the base moves after it (another pull landed), the approval is still counted, and every door says the base moved. | |
| 4 | ||
| 5 | - **Page:** the reviews chip in the merge box on the pull's Files step reads "the base moved since the approval" in red. Nothing is added while the approval is current. | |
| 6 | - **API:** `GET /api/v1/repos/:owner/:name/pulls/:number` carries `approvedAt: { at, head, base, current }`, or `null` when no one's latest verdict approves. `base` is `null` for an approval given before the base was recorded; `current` is `false` once the head or the base moved. | |
| 7 | - **MCP:** `pulls` with a `repo` carries `movedSinceApproval` on each row; the card says "moved since approval". | |
| 8 | - **Terminal:** the pull screen's merge box adds "the base moved since the approval" after the approval count. | |
| 9 | ||
| 10 | It is information, not a gate: the base moves with every landing, so refusing a merge on it would ask for a new approval after every unrelated merge. A moved head still dismisses the approval, as before. |
src/actions/pulls.ts +2
| @@ -854,6 +854,7 @@ export async function submitReview( | ||
| 854 | 854 | state: input.state, |
| 855 | 855 | body: input.body ?? "", |
| 856 | 856 | commitOid: status.headOid, |
| 857 | baseOid: status.baseOid, | |
| 857 | 858 | }); |
| 858 | 859 | if (!recorded.ok) return recorded; |
| 859 | 860 | app.bus.append({ |
| @@ -1034,6 +1035,7 @@ export async function approveHere( | ||
| 1034 | 1035 | state: "approved", |
| 1035 | 1036 | body: "", |
| 1036 | 1037 | commitOid: status.headOid, |
| 1038 | baseOid: status.baseOid, | |
| 1037 | 1039 | }); |
| 1038 | 1040 | if (!recorded.ok) return err("not_found"); |
| 1039 | 1041 | app.bus.append({ |
src/core/migration-pins/20261114070000-review-base.pin added +1
| @@ -0,0 +1,1 @@ | ||
| 1 | 30e84ed139e687b40c02ab983058eb9539453e650e97244b9c7e7fcaef8ae41e |
src/core/migrations/20261114070000-review-base.ts added +6
| @@ -0,0 +1,6 @@ | ||
| 1 | // An approval is given against a head and the base tip it was given for. `base_oid` is that base | |
| 2 | // tip, written where approvals are written; '' is a review made before the column, whose base is | |
| 3 | // not known (it stays current by its head alone). Additive. | |
| 4 | export default ` | |
| 5 | ALTER TABLE review ADD COLUMN base_oid TEXT NOT NULL DEFAULT ''; | |
| 6 | `; |
src/core/pulls.ts +12 −2
| @@ -41,7 +41,7 @@ import { | ||
| 41 | 41 | type Issue, |
| 42 | 42 | type ListIssuesOptions, |
| 43 | 43 | } from "./issues.ts"; |
| 44 | import { reviewSummary } from "./reviews.ts"; | |
| 44 | import { approvalRecord, listReviews, reviewSummary, type ApprovalRecord } from "./reviews.ts"; | |
| 45 | 45 | import { checkStale, checksHold } from "../checks/stale.ts"; |
| 46 | 46 | import { checkSummary } from "./runs.ts"; |
| 47 | 47 | import { writeStamp } from "./write_clock.ts"; |
| @@ -781,6 +781,11 @@ export interface PullReviewStatus { | ||
| 781 | 781 | /** Whether this repo demands an approval at all (`repo.requireReview`). */ |
| 782 | 782 | readonly required: boolean; |
| 783 | 783 | readonly mergeable: boolean; |
| 784 | /** | |
| 785 | * The approval the pull stands on, with the head and base it was given for; null when none, | |
| 786 | * or when the caller named no base. | |
| 787 | */ | |
| 788 | readonly approval: ApprovalRecord | null; | |
| 784 | 789 | } |
| 785 | 790 | |
| 786 | 791 | /** |
| @@ -830,9 +835,14 @@ export function reviewGate( | ||
| 830 | 835 | gate: ReviewGate, |
| 831 | 836 | pull: Pull, |
| 832 | 837 | headOid: string, |
| 838 | baseOid?: string, | |
| 833 | 839 | ): PullReviewStatus { |
| 834 | 840 | const summary = reviewSummary(gate.db, pull.id, headOid); |
| 835 | 841 | return { |
| 842 | approval: | |
| 843 | baseOid === undefined | |
| 844 | ? null | |
| 845 | : approvalRecord(listReviews(gate.db, pull.id), headOid, baseOid), | |
| 836 | 846 | approved: summary.approvals, |
| 837 | 847 | changesRequested: summary.changesRequested, |
| 838 | 848 | required: gate.requireReview, |
| @@ -864,7 +874,7 @@ export async function pullStatus( | ||
| 864 | 874 | ]); |
| 865 | 875 | const baseOid = liveBase ?? pull.baseOid; |
| 866 | 876 | const headOid = liveHead ?? pull.headOid; |
| 867 | const reviews = gate === undefined ? null : reviewGate(gate, pull, headOid); | |
| 877 | const reviews = gate === undefined ? null : reviewGate(gate, pull, headOid, baseOid); | |
| 868 | 878 | // A fork's pull combined on our side merges as its combined branch (#9012). |
| 869 | 879 | const lands = pull.crossRepo |
| 870 | 880 | ? ((await combinedTip(dir, pull.number, headOid)) ?? headOid) |
src/core/reviews.ts +43 −3
| @@ -210,6 +210,8 @@ export interface Review { | ||
| 210 | 210 | readonly body: string; |
| 211 | 211 | /** The head the verdict was passed on. A verdict on an older head is stale. */ |
| 212 | 212 | readonly commitOid: string; |
| 213 | /** The base tip the verdict was given against; '' when it was not recorded. */ | |
| 214 | readonly baseOid: string; | |
| 213 | 215 | readonly createdAt: string; |
| 214 | 216 | } |
| 215 | 217 | |
| @@ -231,6 +233,8 @@ export interface SubmitReviewInput { | ||
| 231 | 233 | /** May be empty: an approval needs no words. */ |
| 232 | 234 | body?: string; |
| 233 | 235 | commitOid: string; |
| 236 | /** The base tip the verdict is given against. */ | |
| 237 | baseOid?: string; | |
| 234 | 238 | /** Set when the review came from an origin rather than from here. */ |
| 235 | 239 | originRemoteId?: string; |
| 236 | 240 | originAuthor?: string; |
| @@ -254,8 +258,8 @@ export function submitReview( | ||
| 254 | 258 | const write = db.transaction(() => { |
| 255 | 259 | db.query( |
| 256 | 260 | `INSERT INTO review (id, issue_id, author_id, state, body, commit_oid, |
| 257 | origin_remote_id, origin_author, created_at) | |
| 258 | VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)`, | |
| 261 | origin_remote_id, origin_author, created_at, base_oid) | |
| 262 | VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)`, | |
| 259 | 263 | ).run( |
| 260 | 264 | reviewId, |
| 261 | 265 | input.issueId, |
| @@ -266,6 +270,7 @@ export function submitReview( | ||
| 266 | 270 | input.originRemoteId ?? null, |
| 267 | 271 | input.originAuthor ?? null, |
| 268 | 272 | now, |
| 273 | input.baseOid ?? "", | |
| 269 | 274 | ); |
| 270 | 275 | db.query("UPDATE issue SET updated_at = ?1 WHERE id = ?2").run( |
| 271 | 276 | new Date().toISOString(), |
| @@ -284,13 +289,14 @@ interface ReviewRowRaw { | ||
| 284 | 289 | state: ReviewState; |
| 285 | 290 | body: string; |
| 286 | 291 | commit_oid: string; |
| 292 | base_oid: string; | |
| 287 | 293 | created_at: string; |
| 288 | 294 | } |
| 289 | 295 | |
| 290 | 296 | const REVIEW_SELECT = ` |
| 291 | 297 | SELECT review.id, review.author_id, user.username AS author_name, |
| 292 | 298 | review.origin_author, review.state, review.body, review.commit_oid, |
| 293 | review.created_at | |
| 299 | review.base_oid, review.created_at | |
| 294 | 300 | FROM review JOIN user ON user.id = review.author_id`; |
| 295 | 301 | |
| 296 | 302 | const toReview = (row: ReviewRowRaw): Review => ({ |
| @@ -301,6 +307,7 @@ const toReview = (row: ReviewRowRaw): Review => ({ | ||
| 301 | 307 | state: row.state, |
| 302 | 308 | body: row.body, |
| 303 | 309 | commitOid: row.commit_oid, |
| 310 | baseOid: row.base_oid, | |
| 304 | 311 | createdAt: row.created_at, |
| 305 | 312 | }); |
| 306 | 313 | |
| @@ -617,3 +624,36 @@ export function getReviewByRemoteId( | ||
| 617 | 624 | .get(issueId, remoteId); |
| 618 | 625 | return row ? toReview(row) : undefined; |
| 619 | 626 | } |
| 627 | ||
| 628 | /** | |
| 629 | * The approval a pull stands on, said with what it was given for: the head | |
| 630 | * and the base tip. `current` is false once either moved. Null when nobody's | |
| 631 | * latest verdict approves. A review made before the base was recorded | |
| 632 | * (`base` null) is judged by its head alone. | |
| 633 | */ | |
| 634 | export interface ApprovalRecord { | |
| 635 | readonly at: string; | |
| 636 | readonly head: string; | |
| 637 | readonly base: string | null; | |
| 638 | readonly current: boolean; | |
| 639 | } | |
| 640 | ||
| 641 | export function approvalRecord( | |
| 642 | reviews: readonly Review[], | |
| 643 | headOid: string, | |
| 644 | baseOid: string, | |
| 645 | ): ApprovalRecord | null { | |
| 646 | let latest: Review | undefined; | |
| 647 | for (const review of latestByAuthor(reviews).values()) | |
| 648 | if (review.state === "approved" && (latest === undefined || review.id > latest.id)) | |
| 649 | latest = review; | |
| 650 | if (latest === undefined) return null; | |
| 651 | return { | |
| 652 | at: latest.createdAt, | |
| 653 | head: latest.commitOid, | |
| 654 | base: latest.baseOid === "" ? null : latest.baseOid, | |
| 655 | current: | |
| 656 | latest.commitOid === headOid && | |
| 657 | (latest.baseOid === "" || latest.baseOid === baseOid), | |
| 658 | }; | |
| 659 | } |
src/http/api.ts +3
| @@ -2783,6 +2783,9 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 2783 | 2783 | ...withLabels(ctx, found.pull), |
| 2784 | 2784 | check: checkWord(checkSummary(ctx.app.db, found.pull.id, status.headOid).run ?? undefined, status.headOid), |
| 2785 | 2785 | checkStale: checkStaleNow, |
| 2786 | // The approval and what it was given for; `current` is false once the head or the base | |
| 2787 | // moved. | |
| 2788 | approvedAt: status.reviews?.approval ?? null, | |
| 2786 | 2789 | variantSiblings: variantSiblings(ctx.app.db, [found.pull])[0] ?? [], |
| 2787 | 2790 | ...(live ? { mergeable: status.mergeable, conflicts: status.conflicts } : {}), |
| 2788 | 2791 | proposal: pullProposalOf(ctx.app.db, found.pull.id) ?? null, |
src/http/mcp/tools/pulls.ts +18 −2
| @@ -1,6 +1,7 @@ | ||
| 1 | import { refTip } from "../../../git/read.ts"; | |
| 1 | 2 | import { canRead, getReposByIds } from "../../../core/repos.ts"; |
| 2 | 3 | import { mayView, sealedScope } from "../../../core/sealed.ts"; |
| 3 | import { reviewSummariesFor } from "../../../core/reviews.ts"; | |
| 4 | import { approvalRecord, listReviewsFor, reviewSummariesFor } from "../../../core/reviews.ts"; | |
| 4 | 5 | import { listPulls, openPullPileRows, pullDir, pullMergeStates, pullsByIssueIds } from "../../../core/pulls.ts"; |
| 5 | 6 | import { checkSummary } from "../../../core/runs.ts"; |
| 6 | 7 | import type { Ctx } from "../../context.ts"; |
| @@ -11,7 +12,7 @@ import { good, bad, type Args, type Answer, type Tool, REPO_ARG, repoArg, isAnsw | ||
| 11 | 12 | |
| 12 | 13 | export const tool: Tool = { |
| 13 | 14 | name: "pulls", |
| 14 | doc: "`{ repo?, state?, limit?, cursor? }`: without `repo`, every open pull you may read, oldest first, as `{ pulls, next_cursor }` (limit 1-200, default 50): `repo`, `number`, `title`, `check`, `verdict` (approved, changes_requested or none) and `gate` (what holds it, or null; the `why` module's cause); with `repo`, a repository's pulls, newest first, one brief line each: `number`, `title`, `author`, `head`, `check` (state), `approvals`, `changesRequested`, `mergeable` (null once closed); `state` open (default) or closed, `limit` 1-50 (default 20); no bodies, no diffs", | |
| 15 | doc: "`{ repo?, state?, limit?, cursor? }`: without `repo`, every open pull you may read, oldest first, as `{ pulls, next_cursor }` (limit 1-200, default 50): `repo`, `number`, `title`, `check`, `verdict` (approved, changes_requested or none) and `gate` (what holds it, or null; the `why` module's cause); with `repo`, a repository's pulls, newest first, one brief line each: `number`, `title`, `author`, `head`, `check` (state), `approvals`, `movedSinceApproval` (the approval was given for a head or base that has since moved), `changesRequested`, `mergeable` (null once closed); `state` open (default) or closed, `limit` 1-50 (default 20); no bodies, no diffs", | |
| 15 | 16 | description: |
| 16 | 17 | "A repository's pulls, newest first; no `repo`: all open pulls you may read, oldest first.", |
| 17 | 18 | inputSchema: { |
| @@ -41,6 +42,10 @@ export const tool: Tool = { | ||
| 41 | 42 | pulls.map((p) => pullDir(db, ctx.app.reposRoot, repo, p)), |
| 42 | 43 | ); |
| 43 | 44 | const merge = await pullMergeStates(pulls.map((pull, i) => ({ dir: dirs[i]!, pull }))); |
| 45 | const approvals = listReviewsFor(db, pulls.map((p) => p.id)); | |
| 46 | const bases = await Promise.all( | |
| 47 | pulls.map((p, i) => (p.state === "open" ? refTip(dirs[i]!, `refs/heads/${p.baseBranch}`) : undefined)), | |
| 48 | ); | |
| 44 | 49 | const answer = good({ |
| 45 | 50 | pulls: pulls.map((p, i) => ({ |
| 46 | 51 | number: p.number, |
| @@ -49,6 +54,7 @@ export const tool: Tool = { | ||
| 49 | 54 | head: p.headBranch, |
| 50 | 55 | check: checkSummary(db, p.id, p.headOid).state, |
| 51 | 56 | approvals: reviews.get(p.id)?.approvals ?? 0, |
| 57 | movedSinceApproval: movedSince(approvals.get(p.id) ?? [], p.headOid, bases[i]), | |
| 52 | 58 | changesRequested: reviews.get(p.id)?.changesRequested ?? 0, |
| 53 | 59 | mergeable: merge[i]?.mergeable ?? null, |
| 54 | 60 | })), |
| @@ -97,3 +103,13 @@ function pile(ctx: Ctx, user: User, args: Args): Answer { | ||
| 97 | 103 | next_cursor, |
| 98 | 104 | }); |
| 99 | 105 | } |
| 106 | ||
| 107 | /** | |
| 108 | * An approval stands for a head and a base: true when either moved since. An unread base is | |
| 109 | * not a move. | |
| 110 | */ | |
| 111 | function movedSince(reviews: Parameters<typeof approvalRecord>[0], head: string, base: string | undefined): boolean { | |
| 112 | const approval = approvalRecord(reviews, head, base ?? ""); | |
| 113 | if (approval === null) return false; | |
| 114 | return base === undefined ? approval.head !== head : !approval.current; | |
| 115 | } |
src/tui/screens/pull.ts +9 −3
| @@ -168,6 +168,8 @@ export interface Standing { | ||
| 168 | 168 | readonly blocking: readonly string[]; |
| 169 | 169 | /** Verdicts a push has dismissed. */ |
| 170 | 170 | readonly stale: number; |
| 171 | /** Approvals given for a base that has since moved. */ | |
| 172 | readonly baseMoved?: number; | |
| 171 | 173 | } |
| 172 | 174 | |
| 173 | 175 | export function standingOf(reviews: readonly ReviewFact[]): Standing { |
| @@ -175,16 +177,19 @@ export function standingOf(reviews: readonly ReviewFact[]): Standing { | ||
| 175 | 177 | for (const review of reviews) latest.set(review.author, review); |
| 176 | 178 | let approvals = 0; |
| 177 | 179 | let stale = 0; |
| 180 | let baseMoved = 0; | |
| 178 | 181 | const blocking: string[] = []; |
| 179 | 182 | for (const review of latest.values()) { |
| 180 | 183 | if (!review.current) { |
| 181 | 184 | if (review.state !== "commented") stale++; |
| 182 | 185 | continue; |
| 183 | 186 | } |
| 184 | if (review.state === "approved") approvals++; | |
| 185 | else if (review.state === "changes_requested") blocking.push(review.author); | |
| 187 | if (review.state === "approved") { | |
| 188 | approvals++; | |
| 189 | if (review.baseMoved === true) baseMoved++; | |
| 190 | } else if (review.state === "changes_requested") blocking.push(review.author); | |
| 186 | 191 | } |
| 187 | return { approvals, blocking, stale }; | |
| 192 | return { approvals, blocking, stale, baseMoved }; | |
| 188 | 193 | } |
| 189 | 194 | |
| 190 | 195 | /** `2 approvals · changes requested by bob` — the merge box's second line. */ |
| @@ -199,6 +204,7 @@ export function standingText(standing: Standing, required: boolean): string { | ||
| 199 | 204 | ? "" |
| 200 | 205 | : `changes requested by ${standing.blocking.join(", ")}`, |
| 201 | 206 | standing.stale === 0 ? "" : `${standing.stale} dismissed by a push`, |
| 207 | (standing.baseMoved ?? 0) === 0 ? "" : "the base moved since the approval", | |
| 202 | 208 | ].filter((part) => part !== ""); |
| 203 | 209 | return parts.join(" · "); |
| 204 | 210 | } |
src/view/local.ts +4
| @@ -322,6 +322,10 @@ export function localSource( | ||
| 322 | 322 | commitOid: review.commitOid, |
| 323 | 323 | createdAt: review.createdAt, |
| 324 | 324 | current: review.commitOid === status.headOid, |
| 325 | baseMoved: | |
| 326 | review.commitOid === status.headOid && | |
| 327 | review.baseOid !== "" && | |
| 328 | review.baseOid !== status.baseOid, | |
| 325 | 329 | })), |
| 326 | 330 | }; |
| 327 | 331 | } |
src/view/source.ts +2
| @@ -172,6 +172,8 @@ export interface ReviewFact { | ||
| 172 | 172 | readonly createdAt: string; |
| 173 | 173 | /** False when the verdict was passed on an older head — dismissed. */ |
| 174 | 174 | readonly current: boolean; |
| 175 | /** True when this verdict is current but was given for a base that has since moved. */ | |
| 176 | readonly baseMoved?: boolean; | |
| 175 | 177 | } |
| 176 | 178 | |
| 177 | 179 | /** What a pull adds to a conversation. */ |
src/web/pages/pull.ts +2
| @@ -260,6 +260,8 @@ function reviewsChip( | ||
| 260 | 260 | ${blocking.map((s) => s.who).join(", ")}</span |
| 261 | 261 | >`, |
| 262 | 262 | ); |
| 263 | if (reviews.approved > 0 && reviews.approval !== null && !reviews.approval.current) | |
| 264 | parts.push(html`<span class="bad">${icon("alert")}the base moved since the approval</span>`); | |
| 263 | 265 | if (parts.length === 0) { |
| 264 | 266 | if (!reviews.required) return html``; |
| 265 | 267 | parts.push(html`<span class="muted">No reviews yet</span>`); |
src/web/studio/cards.ts +1 −1
| @@ -777,7 +777,7 @@ export function cardFor( | ||
| 777 | 777 | const slug = text(p["repo"]) || text(extra["repo"]); |
| 778 | 778 | const path = `/${slug}/pulls/${number}`; |
| 779 | 779 | const verdict = text(p["verdict"]); |
| 780 | const review = reviewWord(verdict, Number(p["approvals"]), Number(p["changesRequested"])); | |
| 780 | const review = reviewWord(verdict, Number(p["approvals"]), Number(p["changesRequested"])) + (p["movedSinceApproval"] === true ? " · moved since approval" : ""); | |
| 781 | 781 | rows.push({ |
| 782 | 782 | label: `#${number} ${clip(p["title"])}`.trim(), |
| 783 | 783 | value: `${checkWord(check) === "none" ? "no check" : `check ${checkWord(check)}`} · ${review}`, |
test/approval-base.test.ts added +114
| @@ -0,0 +1,114 @@ | ||
| 1 | /** | |
| 2 | * An approval is recorded against the head and the base tip it was given | |
| 3 | * for, and stops being current when either moves: the API carries | |
| 4 | * `approvedAt.base`, the pull page's reviews chip says when the base moved. | |
| 5 | */ | |
| 6 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 7 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 8 | import { tmpdir } from "node:os"; | |
| 9 | import { join } from "node:path"; | |
| 10 | import { openPull, submitReview } from "../src/actions/pulls.ts"; | |
| 11 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 12 | import { createApp, type App } from "../src/app.ts"; | |
| 13 | import { getPull } from "../src/core/pulls.ts"; | |
| 14 | import type { Repo } from "../src/core/repos.ts"; | |
| 15 | import { approvalRecord, type Review } from "../src/core/reviews.ts"; | |
| 16 | import { createToken } from "../src/core/tokens.ts"; | |
| 17 | import { createUser, type User } from "../src/core/users.ts"; | |
| 18 | import { forgeHttp, type ForgeHttp } from "../src/http/server.ts"; | |
| 19 | import { quietLogs } from "./helpers/globals.ts"; | |
| 20 | import { promote } from "./helpers/members.ts"; | |
| 21 | ||
| 22 | quietLogs(); | |
| 23 | ||
| 24 | describe("approvalRecord", () => { | |
| 25 | const review = (over: Partial<Review>): Review => ({ | |
| 26 | id: "1", authorId: "a", authorName: "a", originAuthor: null, state: "approved", | |
| 27 | body: "", commitOid: "h1", baseOid: "b1", createdAt: "t", ...over, | |
| 28 | }); | |
| 29 | test("current only while the head and the base are those it was given for", () => { | |
| 30 | const r = [review({})]; | |
| 31 | expect(approvalRecord(r, "h1", "b1")?.current).toBe(true); | |
| 32 | expect(approvalRecord(r, "h1", "b2")?.current).toBe(false); | |
| 33 | expect(approvalRecord(r, "h2", "b1")?.current).toBe(false); | |
| 34 | }); | |
| 35 | test("an approval from before the base was recorded is judged by its head", () => { | |
| 36 | const r = [review({ baseOid: "" })]; | |
| 37 | expect(approvalRecord(r, "h1", "b9")).toEqual({ at: "t", head: "h1", base: null, current: true }); | |
| 38 | }); | |
| 39 | test("a later request for changes by the same person withdraws it", () => { | |
| 40 | expect(approvalRecord([review({}), review({ id: "2", state: "changes_requested" })], "h1", "b1")).toBeNull(); | |
| 41 | }); | |
| 42 | }); | |
| 43 | ||
| 44 | describe("approvedAt over the API and the page", () => { | |
| 45 | let dataDir: string; | |
| 46 | let app: App; | |
| 47 | let http: ForgeHttp; | |
| 48 | let alice: User; | |
| 49 | let bob: User; | |
| 50 | let repo: Repo; | |
| 51 | let token = ""; | |
| 52 | const url = "http://forge.test"; | |
| 53 | ||
| 54 | const get = (path: string, auth = true) => | |
| 55 | http.handle(new Request(`${url}${path}`, { headers: auth ? { authorization: `Bearer ${token}` } : {} })); | |
| 56 | ||
| 57 | beforeAll(async () => { | |
| 58 | dataDir = await mkdtemp(join(tmpdir(), "forge-approval-base-")); | |
| 59 | app = createApp({ dataDir, port: 0, host: "forge.test" }); | |
| 60 | http = forgeHttp(app); | |
| 61 | const mk = async (username: string, first: boolean) => { | |
| 62 | const made = await createUser(app.db, { | |
| 63 | origin: "seed", trusted: true, username, email: `${username}@forge.local`, | |
| 64 | password: "a-good-passphrase", adminIfFirst: first, | |
| 65 | }); | |
| 66 | if (!made.ok) throw new Error(made.error); | |
| 67 | return made.value; | |
| 68 | }; | |
| 69 | alice = await mk("alice", true); | |
| 70 | bob = await mk("bob", false); | |
| 71 | const made = await createRepo(app, alice, { name: "lib", visibility: "public" }); | |
| 72 | if (!made.ok) throw new Error(String(made.error)); | |
| 73 | repo = made.value; | |
| 74 | promote(app.db, bob); | |
| 75 | app.db.run("UPDATE user SET is_admin = 1 WHERE id = ?1", [bob.id]); | |
| 76 | bob = { ...bob, isAdmin: true }; | |
| 77 | token = createToken(app.db, alice.id, "t", "rw").token; | |
| 78 | }, 60_000); | |
| 79 | ||
| 80 | afterAll(async () => { | |
| 81 | app.close(); | |
| 82 | await rm(dataDir, { recursive: true, force: true }); | |
| 83 | }); | |
| 84 | ||
| 85 | test("the approval names its head and base, and goes stale when the base moves", async () => { | |
| 86 | const first = await commitToBranch(app, alice, repo, { message: "one", files: [{ path: "a.md", content: "a\n" }] }); | |
| 87 | if (!first.ok) throw new Error(JSON.stringify(first.error)); | |
| 88 | const side = await commitToBranch(app, alice, repo, { branch: "feat", base: "main", message: "feat", files: [{ path: "f.md", content: "f\n" }] }); | |
| 89 | if (!side.ok) throw new Error(JSON.stringify(side.error)); | |
| 90 | const opened = await openPull(app, alice, repo, { title: "Feat", baseBranch: "main", headBranch: "feat" }); | |
| 91 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 92 | const number = opened.value.number; | |
| 93 | const path = `/api/v1/repos/alice/lib/pulls/${number}?diff=0`; | |
| 94 | ||
| 95 | expect((await (await get(path)).json() as { approvedAt: unknown }).approvedAt).toBeNull(); | |
| 96 | ||
| 97 | const pull = getPull(app.db, repo.id, number)!; | |
| 98 | const approved = await submitReview(app, bob, repo, pull, { state: "approved" }); | |
| 99 | if (!approved.ok) throw new Error(String(approved.error)); | |
| 100 | const body = (await (await get(path)).json()) as { approvedAt: { head: string; base: string; current: boolean; at: string } }; | |
| 101 | expect(body.approvedAt.head).toBe(side.value.commitOid); | |
| 102 | expect(body.approvedAt.base).toBe(first.value.commitOid); | |
| 103 | expect(body.approvedAt.current).toBe(true); | |
| 104 | ||
| 105 | expect(await (await get(`/alice/lib/pulls/${number}/files`)).text()).not.toContain("the base moved since the approval"); | |
| 106 | ||
| 107 | const moved = await commitToBranch(app, alice, repo, { message: "two", files: [{ path: "b.md", content: "b\n" }] }); | |
| 108 | if (!moved.ok) throw new Error(JSON.stringify(moved.error)); | |
| 109 | const after = (await (await get(path)).json()) as { approvedAt: { head: string; base: string; current: boolean } }; | |
| 110 | expect(after.approvedAt.base).toBe(first.value.commitOid); | |
| 111 | expect(after.approvedAt.current).toBe(false); | |
| 112 | expect(await (await get(`/alice/lib/pulls/${number}/files`)).text()).toContain("the base moved since the approval"); | |
| 113 | }, 60_000); | |
| 114 | }); |
test/mcp-cards.test.ts +6
| @@ -80,6 +80,12 @@ describe("explore, pulls and feed cards", () => { | ||
| 80 | 80 | await rm(dataDir, { recursive: true, force: true }); |
| 81 | 81 | }); |
| 82 | 82 | |
| 83 | test("a pull row says when its approval moved", () => { | |
| 84 | const rows = cardFor("pulls", { pulls: [{ repo: "a/b", number: 1, title: "t", check: "ok", verdict: "approved", approvals: 1, movedSinceApproval: true }, { repo: "a/b", number: 2, title: "t", check: "ok", verdict: "approved", approvals: 1, movedSinceApproval: false }] }, "https://forge.test", {})!.rows; | |
| 85 | expect(rows[0]!.value).toContain(" · moved since approval"); | |
| 86 | expect(rows[1]!.value).not.toContain("moved since approval"); | |
| 87 | }); | |
| 88 | ||
| 83 | 89 | test("explore draws its card for a stranger: the ranked public app, linked, and not the private one", async () => { |
| 84 | 90 | const c = await card("explore", {}); |
| 85 | 91 | expect(c.kind).toBe("explore"); |
test/mcp-pulls.test.ts +11
| @@ -117,6 +117,7 @@ test("lists open pulls in the brief shape, newest first, limited", async () => { | ||
| 117 | 117 | head: "two", |
| 118 | 118 | check: "none", |
| 119 | 119 | approvals: 0, |
| 120 | movedSinceApproval: false, | |
| 120 | 121 | changesRequested: 0, |
| 121 | 122 | mergeable: true, |
| 122 | 123 | }); |
| @@ -178,3 +179,13 @@ test("a sealed pull is seen by a writer and its reporter, not by a plain reader" | ||
| 178 | 179 | expect(await heads("sam")).toContain("sealed"); |
| 179 | 180 | expect(await heads("rita")).not.toContain("sealed"); |
| 180 | 181 | }); |
| 182 | ||
| 183 | test("an approval given for a base that has since moved says so", async () => { | |
| 184 | const before = (await labPulls("rita")).find((p) => p.head === "reviewed"); | |
| 185 | expect(before.movedSinceApproval).toBe(false); | |
| 186 | // The approval was given against a base tip that is not the base's tip now. | |
| 187 | app.db.run("UPDATE review SET base_oid = ?1 WHERE commit_oid != ''", ["0".repeat(40)]); | |
| 188 | const after = (await labPulls("rita")).find((p) => p.head === "reviewed"); | |
| 189 | expect(after.movedSinceApproval).toBe(true); | |
| 190 | expect((await labPulls("rita")).find((p) => p.head === "asked").movedSinceApproval).toBe(false); | |
| 191 | }); |
test/pull-next-tui.test.ts +19
| @@ -6,6 +6,8 @@ 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 | 8 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; |
| 9 | import { repoDir } from "../src/git/repos.ts"; | |
| 10 | import { submitReview } from "../src/core/reviews.ts"; | |
| 9 | 11 | import { createUser, type User } from "../src/core/users.ts"; |
| 10 | 12 | import { createForgeApp } from "../src/tui/forge.ts"; |
| 11 | 13 | import { pullScreen } from "../src/tui/screens/pull.ts"; |
| @@ -99,3 +101,20 @@ test("with reviews off, a green pull's screen says it lands by itself, as the pa | ||
| 99 | 101 | const text = await screenSaying(topic, "next: queue, green, reviews are off: it lands"); |
| 100 | 102 | if (text.includes("next: reviewer")) throw new Error(text); |
| 101 | 103 | }); |
| 104 | ||
| 105 | test("the terminal's pull screen says when an approval was given for a base that has moved", async () => { | |
| 106 | const topic = await openTopic("lib-moved", true); | |
| 107 | const bob = await createUser(app.db, { | |
| 108 | origin: "seed", trusted: true, username: "bob", email: "[email protected]", password: "a-good-passphrase", | |
| 109 | }); | |
| 110 | if (!bob.ok) throw new Error(String(bob.error)); | |
| 111 | const head = (await Bun.$`git --git-dir=${repoDir(app.reposRoot, alice.username, topic.repo.name)} rev-parse refs/heads/topic`.text()).trim(); | |
| 112 | const base = (await Bun.$`git --git-dir=${repoDir(app.reposRoot, alice.username, topic.repo.name)} rev-parse refs/heads/main`.text()).trim(); | |
| 113 | const given = submitReview(app.db, { issueId: topic.pull.id, authorId: bob.value.id, state: "approved", commitOid: head, baseOid: base }); | |
| 114 | if (!given.ok) throw new Error(String(given.error)); | |
| 115 | const quiet = await screenSaying(topic, "1 approval"); | |
| 116 | if (quiet.includes("the base moved since the approval")) throw new Error(quiet); | |
| 117 | const moved = await commitToBranch(app, alice, topic.repo, { branch: "main", message: "more", files: [{ path: "more.txt", content: "m\n" }] }); | |
| 118 | if (!moved.ok) throw new Error(JSON.stringify(moved.error)); | |
| 119 | await screenSaying(topic, "the base moved since the approval"); | |
| 120 | }); |
new above
offline