trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRunsg rEventsFlows

The pull page's merge box and the terminal's m say which team a pull waits on, and OPERATING.md stops saying it is not required #10141

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/10081-the-pull-page-s-merge-box-and-the-termin → main 2 commits fork

  • Merged. scout/10081-the-pull-page-s-merge-box-and-the-termin was merged into main as d0282d5.

Review

Sign in to review this change.
4 files changed +120 −9
docs/OPERATING.md +1 −1

Diff not shown — a line 2,708 characters wide (limit 2,000). +1 −1

View file
src/tui/screens/pull.ts +34 −5
@@ -26,7 +26,7 @@
2626 * `n`/`p` walk the unsettled ones while the cursor is inside a thread.
2727 */
2828
29 import { teamHoldRefusal } from "../../core/teams_words.ts";
29 import { teamHoldRefusal, waitingOnTeams } from "../../core/teams_words.ts";
3030 import { crewFilters, guardedRefusal, isStrangerPull, strangerGuard, strangerNext } from "../../crew/stranger.ts";
3131 import { RED_ALONE_WAIT, redAloneAt, redMark } from "../../checks/queue.ts";
3232 import {
@@ -60,7 +60,7 @@ import {
6060 type EnvStatus, envsFor } from "../../apps/runtime.ts";
6161 import { proposedByText } from "../../core/proposal_token.ts";
6262 import { getAppByRepo } from "../../core/apps.ts";
63 import { teamVerdicts, type TeamVerdict } from "../../core/issue_teams.ts";
63 import { teamsHolding, teamVerdicts, type TeamVerdict } from "../../core/issue_teams.ts";
6464 import { getIssue, type Issue } from "../../core/issues.ts";
6565 import { isProposed, pullMayRun } from "../../core/pull_trust.ts";
6666 import { guestWaits, mayApproveGuest } from "../../origins/guest.ts";
@@ -259,6 +259,8 @@ export function pullScreen(
259259 let step: PathStep | null = null;
260260 /** Where each team the issue names stands; empty when it names none. */
261261 let teams: TeamVerdict[] = [];
262 /** The named teams `mergePull` would refuse for (`team_review_required`), by name. */
263 let holding: string[] = [];
262264 /** Settled threads the reader has opened back up, for this visit only. */
263265 const expanded = new Set<string>();
264266 let lines: Span[][] = [];
@@ -433,6 +435,13 @@ export function pullScreen(
433435 ...row,
434436 headOid: row.headOid || (facts?.headOid ?? ""),
435437 });
438 holding =
439 row === undefined
440 ? []
441 : teamsHolding(f.db, {
442 ...row,
443 headOid: row.headOid || (facts?.headOid ?? ""),
444 }).map((v) => v.name);
436445 checkOld =
437446 repo !== undefined &&
438447 row !== undefined &&
@@ -1061,7 +1070,7 @@ export function pullScreen(
10611070
10621071 const mirrored = (): boolean => repo === undefined || repo.origin !== null;
10631072 const state = (): MergeState =>
1064 mergeState(detail?.issue, facts, mirrored(), gated(), checksGated());
1073 mergeState(detail?.issue, facts, mirrored(), gated(), checksGated(), holding.length > 0);
10651074
10661075 const screen: Screen = {
10671076 title: `#${number}`,
@@ -1213,6 +1222,7 @@ export function pullScreen(
12131222 drawMergeBox(s, boxRow!, c, issue, facts, view.summary(), mirrored(), {
12141223 gated: gated(),
12151224 checksGated: checksGated(),
1225 holding,
12161226 checkOld,
12171227 check,
12181228 checksLine,
@@ -1521,6 +1531,11 @@ export function pullScreen(
15211531 });
15221532 return true;
15231533 }
1534 // A named team holds it (#10081): the merge's own sentence, and no merge.
1535 if (e.key === "m" && mayWrite() && state() === "team_required") {
1536 ctx.toast(teamHoldRefusal(holding), "attn");
1537 return true;
1538 }
15241539 if (e.key === "m" && mayWrite() && state() === "checks_required") {
15251540 ctx.toast(checksRefusal(check, checkOld), "attn");
15261541 return true;
@@ -1630,6 +1645,8 @@ export type MergeState =
16301645 | "empty"
16311646 /** The verdicts do not let this land yet — `mergePull` refuses it too. */
16321647 | "review_required"
1648 /** A team the closed issue names has not approved this head — `mergePull` refuses it too. */
1649 | "team_required"
16331650 /** The head's deploy is not green yet — `mergePull` refuses it too. */
16341651 | "checks_required"
16351652 | "ready"
@@ -1818,6 +1835,7 @@ export function mergeState(
18181835 mirrored = false,
18191836 gated = false,
18201837 checksGated = false,
1838 teamsHeld = false,
18211839 ): MergeState {
18221840 if (issue === undefined || pull === null || pull === undefined)
18231841 return "unknown";
@@ -1833,7 +1851,11 @@ export function mergeState(
18331851 // because a conflict is the more urgent of the two and only one line fits.
18341852 // The checks come after the reviewers for the same reason: a person's
18351853 // objection outranks a machine's.
1836 return gated ? "review_required" : checksGated ? "checks_required" : "ready";
1854 // A named team's review is a person's objection too, so it comes before
1855 // the machine's, in the order `mergePull` asks them.
1856 if (gated) return "review_required";
1857 if (teamsHeld) return "team_required";
1858 return checksGated ? "checks_required" : "ready";
18371859 }
18381860
18391861 function drawMergeBox(
@@ -1847,6 +1869,8 @@ function drawMergeBox(
18471869 o: {
18481870 gated: boolean;
18491871 checksGated: boolean;
1872 /** The named teams holding the merge, by name; empty when none does. */
1873 holding: readonly string[];
18501874 /** The green was checked against an older base. */
18511875 checkOld: boolean;
18521876 check: CheckSummary;
@@ -1859,7 +1883,7 @@ function drawMergeBox(
18591883 teams: readonly TeamVerdict[];
18601884 },
18611885 ): void {
1862 const state = mergeState(issue, pull, mirrored, o.gated, o.checksGated);
1886 const state = mergeState(issue, pull, mirrored, o.gated, o.checksGated, o.holding.length > 0);
18631887 const tone =
18641888 state === "merged"
18651889 ? "merged"
@@ -1869,6 +1893,7 @@ function drawMergeBox(
18691893 ? "danger"
18701894 : state === "mirrored" ||
18711895 state === "review_required" ||
1896 state === "team_required" ||
18721897 state === "checks_required"
18731898 ? "attn"
18741899 : "default";
@@ -1900,6 +1925,8 @@ function drawMergeBox(
19001925 ? o.standing.blocking.length > 0
19011926 ? `Changes requested by ${o.standing.blocking.join(", ")}`
19021927 : "Needs an approval of this head before it can merge"
1928 : state === "team_required"
1929 ? `${waitingOnTeams(o.holding)}; it cannot merge until ${o.holding.length === 1 ? "it approves" : "each approves"} this head`
19031930 : state === "checks_required"
19041931 ? o.check.state === "ok"
19051932 ? "Waiting on checks: checked against an older base; press C to check again"
@@ -1913,6 +1940,8 @@ function drawMergeBox(
19131940 text:
19141941 state === "review_required"
19151942 ? "review"
1943 : state === "team_required"
1944 ? "team"
19161945 : state === "checks_required"
19171946 ? "checks"
19181947 : state,
src/web/pages/pull.ts +29 −3
@@ -85,8 +85,8 @@ import {
8585 type ThreadView,
8686 } from "../diff-view.ts";
8787 import { field, readForm } from "../form.ts";
88 import { teamHoldRefusal } from "../../core/teams_words.ts";
89 import { teamVerdicts, type TeamVerdict } from "../../core/issue_teams.ts";
88 import { teamHoldRefusal, waitingOnTeams } from "../../core/teams_words.ts";
89 import { teamsHolding, teamVerdicts, type TeamVerdict } from "../../core/issue_teams.ts";
9090 import { html, raw, when, type Html } from "../html.ts";
9191 import { flashTo, fragment, page } from "../layout.ts";
9292 import { liveRegionsJs } from "../client.ts";
@@ -476,6 +476,7 @@ function mergeBox(
476476 isApp: boolean,
477477 tier = "",
478478 stale = false,
479 holding: readonly string[] = [],
479480 ): Html {
480481 const thread = `${repoBase(repo)}/pulls/${pull.number}`;
481482 // A reader's head runs nothing until a writer approves it; say so rather
@@ -593,6 +594,30 @@ function mergeBox(
593594 );
594595 }
595596
597 // The team gate, `mergePull`'s `team_review_required` (#10080): a team the
598 // closed issue names, with a reviewer here, has not approved this head.
599 // Asked here with the same `teamsHolding` so the button is honest.
600 if (holding.length > 0) {
601 return box(
602 "attn",
603 icon("users"),
604 html`${waitingOnTeams(holding)}.`,
605 html`The issue this pull closes names
606 ${holding.length === 1 ? "a team" : "teams"} whose reviewer has not
607 approved the commit that is head right now. Nothing merges until
608 ${holding.length === 1 ? "it does" : "each does"}.
609 ${reviewsChip(status.reviews, by)} ${checks}`,
610 when(mayMerge, () =>
611 btn("Merge pull request", {
612 variant: "primary",
613 icon: "merge",
614 disabled: true,
615 title: waitingOnTeams(holding),
616 }),
617 ),
618 );
619 }
620
596621 // The check gate, `mergePull`'s `checks_required`: the deploy of the
597622 // commit that is head right now must have passed. Read live here so the
598623 // button is honest — the refusal itself happens in the merge.
@@ -1054,7 +1079,8 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
10541079 ${when(wall, (w) => wallCard(w, mayMerge ? thread : undefined))}
10551080 ${holdCard(ctx, repo, pull, { act: false })}
10561081 ${teamsCard(teamVerdicts(ctx.app.db, { ...pull, headOid: status.headOid }))}
1057 ${mergeBox(ctx, repo, pull, status, mayMerge, by, check, isApp, tier, stale)}
1082 ${mergeBox(ctx, repo, pull, status, mayMerge, by, check, isApp, tier, stale,
1083 teamsHolding(ctx.app.db, { ...pull, headOid: status.headOid }).map((v) => v.name))}
10581084 ${slicesCard(base, thread, slices, ends.length > 0 ? ends.length : status.ahead, ends, pull.state === "open" && pull.mergedAt === null, mayLand)}
10591085 ${await combinedCard(dir, pull, status.headOid, status.baseOid)}
10601086 <div class="split">
test/team-merge-gate.test.ts +56
@@ -2,6 +2,7 @@
22 * A named team's review gates the merge (#10080, #9930): `mergePull` refuses
33 * `team_review_required`, with the one sentence, while a team with a crew
44 * reviewer here has not approved the head; the owner's break-glass passes.
5 * The pull page's merge box and the terminal's `m` say which team (#10081).
56 */
67
78 import { seatAs } from "./helpers/seat.ts";
@@ -22,6 +23,12 @@ import { saveTeam } from "../src/core/teams.ts";
2223 import { createUser, getUserByName, type User } from "../src/core/users.ts";
2324 import { serve, type Servers } from "../src/http/server.ts";
2425 import { mergeFailure } from "../src/web/pages/pull.ts";
26 import { createSession } from "../src/core/sessions.ts";
27 import { createForgeApp } from "../src/tui/forge.ts";
28 import { mergeState, pullScreen } from "../src/tui/screens/pull.ts";
29 import { local } from "../src/tui/screens/where.ts";
30 import { headlessTerminal } from "../src/tui/term/terminal.ts";
31 import { settleApp } from "./helpers/tui.ts";
2532 import { quietLogs } from "./helpers/globals.ts";
2633 import { tokenFor } from "./helpers/tokens.ts";
2734
@@ -167,4 +174,53 @@ describe("a named team's review gates the merge", () => {
167174 expect(landed.ok).toBe(true);
168175 expect(fresh(pull).mergedAt).not.toBeNull();
169176 });
177
178 /** The pull's Files step, as alice reads it: the page that draws the merge box. */
179 const page = async (pull: Pull): Promise<string> => {
180 const cookie = `forge_session=${createSession(app.db, alice.id).token}`;
181 const res = await fetch(`${servers.url}/alice/garden/pulls/${pull.number}/files`, { headers: { cookie } });
182 expect(res.status).toBe(200);
183 return res.text();
184 };
185 const mergeButton = (body: string): string =>
186 body.match(/<button[^>]*>(?:(?!<\/button>)[\s\S])*Merge pull request/)?.[0] ?? "";
187
188 test("the page's merge box names the team and disables merge while it holds", async () => {
189 const pull = await pullNaming(["design"]);
190 const held = await page(pull);
191 expect(held).toContain("Waiting on Design&#39;s review.");
192 expect(mergeButton(held)).toMatch(/\bdisabled\b/);
193 approve(pull, "quill");
194 const free = await page(pull);
195 expect(free).not.toContain("Waiting on Design&#39;s review.");
196 });
197
198 test("the terminal's m says the sentence and does not merge", async () => {
199 const pull = await pullNaming(["design"]);
200 const { app: tui, session } = createForgeApp(
201 app,
202 { sshUsername: "alice", remoteAddress: "127.0.0.1", fingerprint: "SHA256:t", user: alice },
203 { terminal: headlessTerminal(160, 44), exitHooks: false },
204 );
205 tui.start();
206 try {
207 tui.ctx.push(pullScreen(session, tui.ctx, local(session, repo), pull.number));
208 await settleApp(tui);
209 expect(tui.screen.toText()).toContain("Waiting on Design's review");
210 tui.dispatch({ type: "key", key: "m", ctrl: false, alt: false, shift: false });
211 await settleApp(tui);
212 expect(tui.screen.toText()).toContain(SENTENCE);
213 expect(fresh(pull).mergedAt).toBeNull();
214 } finally {
215 tui.stop();
216 }
217 });
218
219 test("the terminal's merge state: a held team after the reviewers, before the checks", () => {
220 const issue = { state: "open" } as Parameters<typeof mergeState>[0];
221 const facts = { mergedAt: null, mergeable: true, diff: null } as unknown as Parameters<typeof mergeState>[1];
222 expect(mergeState(issue, facts, false, false, true, true)).toBe("team_required");
223 expect(mergeState(issue, facts, false, true, false, true)).toBe("review_required");
224 expect(mergeState(issue, facts, false, false, false, false)).toBe("ready");
225 });
170226 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs