trevato/forge

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

More
Forks1g fMapg aRuns16g rEventsFlows

A big pull from a fork lands in reviewed slices and still ends as a merge of the author's head #9028

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/9010-a-big-pull-from-a-fork-lands-in-reviewed → main 5 commits fork 2 threads · 2 unresolved

  • Merged. scout/9010-a-big-pull-from-a-fork-lands-in-reviewed was merged into main as 8fba7d1.

Review

Sign in to review this change.
11 files changed +868 −0
docs/features/pull-slices.md added +30
@@ -0,0 +1,30 @@
1 # Landing a fork's pull in slices
2
3 A pull from a fork too long to review in one piece lands in slices (#9010).
4 Each slice is a range of the author's own commits, from what the base already
5 holds up to one commit you pick, merged with its own merge commit. Nothing is
6 rebased or squashed, so every commit of theirs becomes an ancestor of the base
7 and their next `git merge` of it is clean or a fast-forward.
8
9 - **On the page**: the pull's Files tab has a Slices card; a person who may
10 write lands from it (an agent proposes and does not land). Pick a commit
11 (the oldest are listed first) and press *Land slice*; in the terminal, `S`
12 on the pull does the same. The card lists the slices that landed and how
13 many commits are left.
14 - **The last slice** is the author's head: picking it merges the pull through
15 the ordinary merge, with its review and checks gate, and closes it.
16 - **Through the API**: `POST /api/v1/repos/{owner}/{name}/pulls/{number}/slices`
17 with `{"upTo": "<commit oid>"}` answers `{mergeOid, last, left}`; a commit
18 that is not one of the pull's the base lacks answers 422 `bad_slice`.
19
20 A slice's merge commit carries a `Slice-of: #N` trailer; the card reads the
21 landed slices back from the base's history, so there is no separate record to
22 drift.
23
24 Every slice passes the gate the whole pull would: a standing hold refuses it
25 (409 `held`), and so do a head review that does not let the pull land and a
26 head check that is red — or, where the repository requires checks, missing or
27 run against an older base (409 `review_required` / `checks_required`). The
28 head is what was reviewed and checked, and a slice is an ancestor of it. Only
29 a pull from a fork lands in slices; a branch of the repository itself answers
30 422 `not_fork`.
src/actions/pull_slices.ts added +143
@@ -0,0 +1,143 @@
1 /**
2 * Landing a pull in slices (#9010). A writer reads a range of the author's
3 * commits and lands it as its own merge commit of the author's commit as it
4 * is — nothing rebased or squashed, so the pull-back stays clean. The slice
5 * at the author's head is the pull's own merge, through `mergePull` and its
6 * whole gate; the slices before it stay on the pull's trail as a note.
7 *
8 * A slice before the head passes the gate the whole pull would: no hold
9 * stands, the head's review verdict lets it land, and the head's check is
10 * not red — nor, where the repository requires checks, missing or run
11 * against an older base. The head is what was reviewed and checked, and a
12 * slice is an ancestor of it: it lands only what that verdict covered.
13 */
14
15 import type { App } from "../app.ts";
16 import { addComment as addIssueComment } from "../core/issues.ts";
17 import { getHold } from "../core/holds.ts";
18 import { checkSummary } from "../core/runs.ts";
19 import { checkStale, checksHold } from "../checks/stale.ts";
20 import { getPull, pullDir, pullStatus, reviewGate, landingTrailers, type MergePullError, type Pull } from "../core/pulls.ts";
21 import type { Repo } from "../core/repos.ts";
22 import { commitIdentity } from "../core/commit_identity.ts";
23 import { refreshPullHead } from "../core/following.ts";
24 import { aheadCount, merge } from "../git/merge.ts";
25 import { repoDir } from "../git/repos.ts";
26 import { isSafeOid } from "../git/guard.ts";
27 import { sliceEndOk, sliceEnds, sliceTrailer } from "../git/slices.ts";
28 import { err, ok, type Result } from "../kernel/result.ts";
29 import type { User } from "../core/users.ts";
30 import { requireWriter, type AccessError } from "./access.ts";
31 import { mergePull } from "./pulls.ts";
32
33 export type SliceError =
34 | AccessError
35 | MergePullError
36 /** `upTo` is not one of this pull's commits the base lacks. */
37 | "bad_slice"
38 /** A sealed pull's work is apart from the base; it lands whole. */
39 | "sealed"
40 /** Slices are for a pull from a fork; a branch here lands whole. */
41 | "not_fork"
42 /** A hold stands on the pull: nothing of it lands until it is released. */
43 | "held";
44
45 export interface SliceLanded {
46 readonly mergeOid: string;
47 /** This slice was the author's head: the pull is merged. */
48 readonly last: boolean;
49 /** The author's commits the base still lacks. */
50 readonly left: number;
51 }
52
53 /** Where a slice may end, oldest first, for the page and the terminal. */
54 export async function sliceChoices(
55 app: App,
56 repo: Repo,
57 pull: Pull,
58 ): Promise<{ oid: string; subject: string }[]> {
59 const baseDir = repoDir(app.reposRoot, repo.ownerName, repo.name);
60 const status = await pullStatus(baseDir, pull);
61 return sliceEnds(baseDir, status.baseOid, status.headOid);
62 }
63
64 export async function landSlice(
65 app: App,
sentinelcrew outdated

HIGH: from here a non-last slice merges onto the base with no hold, review, checks or stale-base gate. Gate it as mergePull does, per slice.
66 actor: User | undefined,
67 repo: Repo,
68 pull: Pull,
69 upTo: string,
70 ): Promise<Result<SliceLanded, SliceError>> {
71 const writer = requireWriter(app, actor, repo);
72 if (!writer.ok) return writer;
73 if (pull.repoId !== repo.id) return err("not_found");
74 // The crew proposes, a person lands: a slice is a landing.
75 if (writer.value.kind === "agent") return err("forbidden");
76 if (pull.originHeadRepo !== null || repo.origin !== null) return err("mirrored");
77 if (pull.mergedAt !== null) return err("already_merged");
78 if (pull.state === "closed") return err("closed");
79 if (!pull.crossRepo) return err("not_fork");
80 if (getHold(app.db, pull.id) !== undefined) return err("held");
81 if (!isSafeOid(upTo)) return err("bad_slice");
82 await refreshPullHead(app, repo, pull);
83 const live = getPull(app.db, repo.id, pull.number) ?? pull;
84 const baseDir = repoDir(app.reposRoot, repo.ownerName, repo.name);
85 if ((await pullDir(app.db, app.reposRoot, repo, live)) !== baseDir) return err("sealed");
86 const status = await pullStatus(baseDir, live);
87 if (upTo === status.headOid) {
88 const whole = await mergePull(app, actor, repo, live, "merge");
89 return whole.ok ? ok({ mergeOid: whole.value.mergeOid, last: true, left: 0 }) : whole;
90 }
91 if (!(await sliceEndOk(baseDir, status.baseOid, status.headOid, upTo))) return err("bad_slice");
92 const verdict = reviewGate({ db: app.db, requireReview: repo.requireReview }, live, status.headOid);
93 if (!verdict.mergeable) return err("review_required");
94 const check = checkSummary(app.db, live.id, status.headOid);
95 if (
96 check.state === "failed" ||
97 checksHold(repo.requireChecks, check.state, checkStale(check.run, status.baseOid))
98 )
99 return err("checks_required");
100 const taking = await aheadCount(baseDir, status.baseOid, upTo);
101 const lander = commitIdentity(app.config, writer.value);
102 const landed = await merge(baseDir, {
103 baseBranch: live.baseBranch,
104 baseOid: status.baseOid,
105 headOid: upTo,
106 strategy: "merge",
107 message: [
108 `Merge slice of pull request #${live.number} from ${live.headBranch}`,
109 `${live.title}\n\n${taking} commit${taking === 1 ? "" : "s"}, up to ${upTo.slice(0, 12)}.`,
110 `${sliceTrailer(live.number)}\n${landingTrailers(null, lander)}`,
111 ].join("\n\n"),
112 author: lander,
113 });
114 if (!landed.ok) return err(landed.error);
115 // `update-ref` runs no hooks: the base's move is announced here, so
116 // webhooks, checks and production follow it as they would a push.
117 app.bus.append({
118 kind: "push",
119 repoId: repo.id,
120 actorId: writer.value.id,
121 payload: {
122 updates: [
123 {
124 ref: `refs/heads/${live.baseBranch}`,
125 oldOid: landed.value.previousOid,
126 newOid: landed.value.mergeOid,
127 },
128 ],
129 viaApi: true,
130 },
131 });
132 const left = await aheadCount(baseDir, landed.value.mergeOid, status.headOid);
133 const body = `Landed a slice: ${taking} commit${taking === 1 ? "" : "s"} up to ${upTo.slice(0, 12)}, merged as they are in ${landed.value.mergeOid.slice(0, 12)}. ${left} left.`;
134 const noted = addIssueComment(app.db, { issueId: live.id, authorId: writer.value.id, body });
135 if (noted.ok)
136 app.bus.append({
137 kind: "pull.commented",
138 repoId: repo.id,
139 actorId: writer.value.id,
140 payload: { number: live.number, body, commentId: noted.value.id },
141 });
142 return ok({ mergeOid: landed.value.mergeOid, last: false, left });
143 }
src/contract/openapi/routes/index.ts +1
@@ -283,6 +283,7 @@ export { route as postApiV1ReposOwnerNamePullsNumberLooks } from "./postApiV1Rep
283283 export { route as postApiV1ReposOwnerNamePullsNumberMerge } from "./postApiV1ReposOwnerNamePullsNumberMerge.ts";
284284 export { route as postApiV1ReposOwnerNamePullsNumberReviewStates } from "./postApiV1ReposOwnerNamePullsNumberReviewStates.ts";
285285 export { route as postApiV1ReposOwnerNamePullsNumberReviews } from "./postApiV1ReposOwnerNamePullsNumberReviews.ts";
286 export { route as postApiV1ReposOwnerNamePullsNumberSlices } from "./postApiV1ReposOwnerNamePullsNumberSlices.ts";
286287 export { route as postApiV1ReposOwnerNamePullsNumberTakeBack } from "./postApiV1ReposOwnerNamePullsNumberTakeBack.ts";
287288 export { route as postApiV1ReposOwnerNamePullsNumberThreadsAnchorResolve } from "./postApiV1ReposOwnerNamePullsNumberThreadsAnchorResolve.ts";
288289 export { route as postApiV1ReposOwnerNamePullsNumberVariant } from "./postApiV1ReposOwnerNamePullsNumberVariant.ts";
src/contract/openapi/routes/postApiV1ReposOwnerNamePullsNumberSlices.ts added +3
@@ -0,0 +1,3 @@
1 import { REPO, type Route } from "../shared.ts";
2
3 export const route: Route = { method: "post", path: `${REPO}/pulls/{number}/slices`, summary: "Land a pull's commits up to one as their own merge (a person's)", auth: true };
src/git/slices.ts added +99
@@ -0,0 +1,99 @@
1 /**
2 * Slices of a pull (#9010): a long pull lands as several true merges, each
3 * of one of the author's own commits — never rewritten, so every commit of
4 * theirs becomes an ancestor of the base and their next `git merge` of it is
5 * clean. A slice's merge commit carries a `Slice-of: #N` trailer; which
6 * slices landed is read back from the base's history, not stored apart.
7 */
8
9 import { runGit } from "./env.ts";
10 import { isSafeOid, isSafeRef } from "./guard.ts";
11
12 /** The trailer a slice's merge commit carries. */
13 export function sliceTrailer(number: number): string {
14 return `Slice-of: #${number}`;
15 }
16
17 /** Where a slice may end: a commit of the pull's that the base does not hold yet. */
18 export async function sliceEndOk(
19 dir: string,
20 baseOid: string,
21 headOid: string,
22 upTo: string,
23 ): Promise<boolean> {
24 if (!isSafeOid(baseOid) || !isSafeOid(headOid) || !isSafeOid(upTo)) return false;
25 const inHead = await runGit(["merge-base", "--is-ancestor", upTo, headOid], { cwd: dir });
26 if (!inHead.ok) return false;
27 const onBase = await runGit(["merge-base", "--is-ancestor", upTo, baseOid], { cwd: dir });
28 return !onBase.ok;
29 }
30
31 export interface LandedSlice {
32 /** The merge commit on the base. */
33 readonly mergeOid: string;
34 /** The author's commit it merged, as it was. */
35 readonly upTo: string;
36 }
37
38 /**
39 * The slices of pull `number` on the base, oldest first: merge commits on
40 * its first-parent line since the pull opened that carry its trailer.
41 */
42 export async function landedSlices(
43 dir: string,
44 baseRef: string,
45 number: number,
46 since: string,
47 ): Promise<LandedSlice[]> {
48 if (!Number.isInteger(number) || number < 1) return [];
49 if (!isSafeOid(baseRef) && !isSafeRef(baseRef)) return [];
50 const at = Date.parse(since);
51 const out = await runGit(
52 [
53 "log",
54 "--first-parent",
55 "--merges",
56 "--max-count=100",
57 ...(Number.isFinite(at) ? [`--since=${Math.floor(at / 1000) - 60}`] : []),
58 `--grep=^${sliceTrailer(number)}$`,
59 "--format=%H %P",
60 baseRef,
61 "--",
62 ],
63 { cwd: dir },
64 );
65 if (!out.ok) return [];
66 const slices: LandedSlice[] = [];
67 for (const line of out.value.split("\n")) {
68 const [mergeOid, , upTo] = line.trim().split(" ");
69 if (mergeOid !== undefined && upTo !== undefined && isSafeOid(mergeOid) && isSafeOid(upTo))
70 slices.push({ mergeOid, upTo });
71 }
72 return slices.reverse();
73 }
74
75 /**
76 * Where a slice may end, oldest first: the author's line of commits
77 * (first parent) the base lacks — the oldest `limit - 1` of them, then the
78 * head, which is the last slice.
79 */
80 export async function sliceEnds(
81 dir: string,
82 baseOid: string,
83 headOid: string,
84 limit = 100,
85 ): Promise<{ oid: string; subject: string }[]> {
86 if (!isSafeOid(baseOid) || !isSafeOid(headOid)) return [];
87 const out = await runGit(
88 ["log", "--first-parent", "--max-count=2000", "--format=%H %s", `${baseOid}..${headOid}`, "--"],
89 { cwd: dir },
90 );
91 if (!out.ok) return [];
92 const all = out.value
93 .split("\n")
94 .map((line) => ({ oid: line.slice(0, line.indexOf(" ")), subject: line.slice(line.indexOf(" ") + 1) }))
95 .filter((c) => isSafeOid(c.oid));
96 if (all.length === 0) return [];
97 const [head, ...older] = all;
98 return [...older.reverse().slice(0, limit - 1), head!];
99 }
src/http/api.ts +30
@@ -95,6 +95,7 @@ import {
9595 MAX_REVIEW_LINES,
9696 type ReviewInput,
9797 } from "../actions/pulls.ts";
98 import { landSlice } from "../actions/pull_slices.ts";
9899 import { attachLook } from "../actions/attach_look.ts";
99100 import { lookUrl } from "../core/looks.ts";
100101 import { pullLooks } from "../looks/readable.ts";
@@ -2633,6 +2634,35 @@ export function apiRoutes(router: Router<Handler>): void {
26332634 },
26342635 );
26352636
2637 // Land the author's commits up to `upTo` as their own merge commit (#9010).
2638 router.add(
2639 "POST",
2640 "/api/v1/repos/:owner/:name/pulls/:number/slices",
2641 async (ctx) => {
2642 const denied = requireScope(ctx, "rw");
2643 if (denied) return denied;
2644 const found = loadPull(ctx);
2645 if ("error" in found) return found.error;
2646 const input = await body(ctx.request);
2647 const landed = await landSlice(
2648 ctx.app,
2649 ctx.auth.user,
2650 found.repo,
2651 found.pull,
2652 str(input.upTo),
2653 );
2654 if (!landed.ok) {
2655 if (typeof landed.error !== "string")
2656 return json({ error: landed.error.kind }, 409);
2657 if (landed.error === "bad_slice" || landed.error === "sealed" || landed.error === "not_fork")
2658 return json({ error: landed.error }, 422);
2659 if (landed.error === "held") return json({ error: "held" }, 409);
2660 return originAware(landed.error, 409);
2661 }
2662 return json(landed.value);
2663 },
2664 );
2665
26362666 router.add(
26372667 "POST",
26382668 "/api/v1/repos/:owner/:name/pulls/:number/comments",
src/http/server.ts +2
@@ -334,6 +334,7 @@ import {
334334 pullFilesPage,
335335 pullChecksFragment,
336336 pullMergeSubmit,
337 pullSliceSubmit,
337338 pullLandPastSubmit,
338339 pullChecksSubmit,
339340 pullReviewStateSubmit,
@@ -1156,6 +1157,7 @@ export function buildRouter(): Router<Handler> {
11561157 threadResolveSubmit(false),
11571158 );
11581159 router.add("POST", "/:owner/:name/pulls/:number/merge", pullMergeSubmit);
1160 router.add("POST", "/:owner/:name/pulls/:number/slice", pullSliceSubmit);
11591161 router.add("POST", "/:owner/:name/pulls/:number/land-past", pullLandPastSubmit);
11601162 router.add("POST", "/:owner/:name/pulls/:number/state", (ctx) =>
11611163 stateSubmit(ctx, "pull"),
src/tui/screens/pull.ts +61
@@ -44,6 +44,7 @@ import { holdPull, releasePull } from "../../actions/holds.ts";
4444 import { getHold, type Hold } from "../../core/holds.ts";
4545 import { bypassOffered, cleanReason, landPastGate } from "../../checks/bypass.ts";
4646 import { revertPull } from "../../actions/revert.ts";
47 import { landSlice, sliceChoices } from "../../actions/pull_slices.ts";
4748 import { takeBackWords } from "../../actions/revert_words.ts";
4849 import { openEnvUrl } from "../../actions/apps.ts";
4950 import { listRepoRuns, mayReadRunLog } from "../../actions/runs.ts";
@@ -115,6 +116,7 @@ import {
115116 drawLines,
116117 hitScroll,
117118 errorText,
119 pickerScreen,
118120 live,
119121 openLink,
120122 plural,
@@ -768,6 +770,60 @@ export function pullScreen(
768770 });
769771 };
770772
773 /**
774 * Whether `S` is on offer: an open, unmerged pull from a fork on a local
775 * repository, for a person who may write — the page's slices form, by the
776 * same rule (#9010). `landSlice` holds the gate; this only shows the key.
777 */
778 const mayLandSlice = (): boolean => {
779 if (repo === undefined || repo.origin !== null || !mayWrite()) return false;
780 if (f.user === undefined || f.user.kind === "agent") return false;
781 const row = getPull(f.db, repo.id, number);
782 return row !== undefined && row.crossRepo && row.originHeadRepo === null && row.mergedAt === null && row.state === "open";
783 };
784
785 /** `S` — pick where the slice ends, then land the author's commits up to it as they are. */
786 const landSlices = (): void => {
787 if (repo === undefined) return;
788 const target = repo;
789 const row = getPull(f.db, target.id, number);
790 if (row === undefined) return;
791 runAsync(ctx, async () => {
792 const ends = await sliceChoices(f.app, target, row);
793 if (ends.length === 0) {
794 ctx.toast("nothing left to land");
795 return;
796 }
797 ctx.push(
798 pickerScreen(ctx, screen, {
799 title: `Land #${number} up to`,
800 items: ends.map((c, i) => ({
801 id: c.oid,
802 label: `${c.oid.slice(0, 7)} ${c.subject}`,
803 ...(i === ends.length - 1 ? { hint: "head: the last slice" } : {}),
804 })),
805 onPick: ([upTo]) => {
806 if (upTo === undefined) return;
807 runAsync(ctx, async () => {
808 const live = getPull(f.db, target.id, number);
809 if (live === undefined) return;
810 const landed = await landSlice(f.app, f.user, target, live, upTo);
811 if (!landed.ok) {
812 ctx.toast(`not landed: ${errorText(String(typeof landed.error === "string" ? landed.error : landed.error.kind))}`, "danger");
813 return;
814 }
815 await reload();
816 ctx.toast(
817 landed.value.last ? "last slice landed: the pull is merged" : `slice landed, ${landed.value.left} left`,
818 "success",
819 );
820 });
821 },
822 }),
823 );
824 });
825 };
826
771827 /**
772828 * Whether `L` is on offer: an open pull, an owner who is a person, and
773829 * landing stopped (`checks/bypass.ts`). The same rule the page's form follows.
@@ -1037,6 +1093,7 @@ export function pullScreen(
10371093 binding(["m"], "merge", { disabled: state() !== "ready" }),
10381094 ...(wall !== undefined && mayWrite() ? [binding(["w"], "the access wall: allow or send back")] : []),
10391095 ...(mayLandPast() ? [binding(["L"], "land past the gate")] : []),
1096 ...(mayLandSlice() ? [binding(["S"], "land a slice")] : []),
10401097 ...(tab === 1 && view.threadHere() !== null
10411098 ? [
10421099 binding(
@@ -1241,6 +1298,10 @@ export function pullScreen(
12411298 );
12421299 return true;
12431300 }
1301 if (e.key === "S" && mayLandSlice()) {
1302 landSlices();
1303 return true;
1304 }
12441305 if (e.key === "L" && mayLandPast()) {
12451306 landPast();
12461307 return true;
src/web/pages/pull.ts +113
@@ -76,6 +76,8 @@ import { notePhase, takeSessionTime } from "../../core/route_timing.ts";
7676 import type { DiffLaps } from "../../git/diff.ts";
7777 import { diffCacheCounts, diffKey, keptDiff, keptRender } from "../diff-cache.ts";
7878 import { keptRead } from "../head-reads.ts";
79 import { landedSlices, sliceEnds, type LandedSlice } from "../../git/slices.ts";
80 import { landSlice } from "../../actions/pull_slices.ts";
7981 import { diffSyntax } from "../diff-syntax.ts";
8082 import {
8183 renderDiff,
@@ -99,6 +101,7 @@ import {
99101 labelCss,
100102 queuedPhrase,
101103 row,
104 selectField,
102105 sha,
103106 time,
104107 type Tone,
@@ -966,6 +969,24 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
966969 ),
967970 );
968971 const commits = log.ok ? log.value : [];
972 // A fork's pull may land in slices; which did is read from the base (#9010).
973 // A person lands them: the crew proposes (rule 1 in docs/APPS.md).
974 const mayLand = mayMerge && ctx.auth.user?.kind !== "agent";
975 const slices = pull.crossRepo
976 ? await lap("git", () =>
977 keptRead(`slices:${repo.id}:${pull.number}:${status.baseOid}`, () =>
978 landedSlices(dir, status.baseOid, pull.number, pull.createdAt),
979 ),
980 )
981 : [];
982 const ends =
983 pull.crossRepo && mayLand && pull.state === "open" && pull.mergedAt === null && status.ahead > 1
984 ? await lap("git", () =>
985 keptRead(`slice-ends:${repo.id}:${status.baseOid}:${status.headOid}`, () =>
986 sliceEnds(dir, status.baseOid, status.headOid),
987 ),
988 )
989 : [];
969990 const commitsCard = card(
970991 commits.length === 0
971992 ? html`<p class="muted small">No commits to show.</p>`
@@ -1024,6 +1045,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
10241045 ${holdCard(ctx, repo, pull, { act: false })}
10251046 ${teamsCard(teamVerdicts(ctx.app.db, { ...pull, headOid: status.headOid }))}
10261047 ${mergeBox(ctx, repo, pull, status, mayMerge, by, check, isApp, tier, stale)}
1048 ${slicesCard(base, thread, slices, status.ahead, ends, pull.state === "open" && pull.mergedAt === null, mayLand)}
10271049 ${await combinedCard(dir, pull, status.headOid, status.baseOid)}
10281050 <div class="split">
10291051 <div>
@@ -1442,6 +1464,97 @@ export async function pullMergeSubmit(ctx: Ctx): Promise<Response> {
14421464 return flashTo(ctx, dest, { message: "Pull request merged." });
14431465 }
14441466
1467 // POST /:owner/:name/pulls/:number/slice — land the author's commits up to one (#9010).
1468 export async function pullSliceSubmit(ctx: Ctx): Promise<Response> {
1469 const found = pullContext(ctx);
1470 if (found instanceof Response) return found;
1471 if (!ctx.auth.user || !canWrite(ctx.app.db, found.repo, ctx.auth.user))
1472 return forbiddenPage(ctx, "Only someone who can write to this repository can do this.");
1473 const form = await readForm(ctx, "rw");
1474 if (form instanceof Response) return form;
1475 const dest = `${repoBase(found.repo)}/pulls/${found.pull.number}`;
1476 const landed = await landSlice(ctx.app, ctx.auth.user, found.repo, found.pull, field(form, "upTo") ?? "");
1477 if (!landed.ok)
1478 return flashTo(ctx, `${dest}/files`, {
1479 tone: "danger",
1480 message:
1481 landed.error === "bad_slice"
1482 ? "That commit is not one of this pull's that the base still lacks. Nothing landed."
1483 : landed.error === "sealed"
1484 ? "This pull's work is sealed apart from the base; it lands whole."
1485 : landed.error === "not_fork"
1486 ? "Only a pull from a fork lands in slices; this one lands whole."
1487 : landed.error === "held"
1488 ? "A hold stands on this pull: nothing of it lands until it is released."
1489 : landed.error === "forbidden" && ctx.auth.user.kind === "agent"
1490 ? "The crew proposes and a person lands: an agent cannot land a slice."
1491 : mergeFailure(landed.error),
1492 });
1493 return flashTo(ctx, landed.value.last ? dest : `${dest}/files`, {
1494 message: landed.value.last
1495 ? "Last slice landed: the pull is merged."
1496 : `Slice landed. ${landed.value.left} commit${landed.value.left === 1 ? "" : "s"} left.`,
1497 });
1498 }
1499
1500 /**
1501 * The slices card (#9010): which slices of a fork's pull landed, what is
1502 * left, and — for a writer — a form to land the author's commits up to one
1503 * of them as their own merge commit.
1504 */
1505 function slicesCard(
1506 base: string,
1507 thread: string,
1508 slices: readonly LandedSlice[],
1509 left: number,
1510 commits: readonly { oid: string; subject: string }[],
1511 open: boolean,
1512 mayMerge: boolean,
1513 ): Html {
1514 if (slices.length === 0 && !(open && mayMerge && left > 1)) return html``;
1515 return card(
1516 html`${when(
1517 slices.length > 0,
1518 () => html`<ul class="list">
1519 ${slices.map(
1520 (s, i) => html`<li>
1521 <a class="row" href="${base}/commit/${s.mergeOid}">
1522 <span class="row-main">
1523 <span class="row-title">Slice ${i + 1}: up to ${sha(s.upTo)}</span>
1524 <span class="row-meta">merged as they are</span>
1525 </span>
1526 <span class="row-trail">${sha(s.mergeOid)}</span>
1527 </a>
1528 </li>`,
1529 )}
1530 </ul>`,
1531 )}
1532 <p class="muted small">
1533 ${open ? html`${left} commit${left === 1 ? "" : "s"} left to land.` : html`Nothing left: the last slice was the author's head.`}
1534 </p>
1535 ${when(
1536 open && mayMerge && commits.length > 1,
1537 () => html`<form method="post" action="${thread}/slice">
1538 ${selectField({
1539 label: "Land the author's commits up to",
1540 name: "upTo",
1541 id: "slice-up-to",
1542 hint: "Each slice is its own merge of the author's commits as they are. It lands only while the head's review and check would let the whole pull land.",
1543 options: commits.map((c, i) => ({
1544 value: c.oid,
1545 label: `${c.oid.slice(0, 7)} ${c.subject}${i === commits.length - 1 ? " (head: the last slice)" : ""}`,
1546 })),
1547 })}
1548 ${btn("Land slice", { icon: "merge" })}
1549 </form>`,
1550 )}`,
1551 {
1552 head: html`${icon("merge")}
1553 <h2>Slices</h2>`,
1554 },
1555 );
1556 }
1557
14451558 // POST /:owner/:name/pulls/:number/land-past — the owner's break-glass (#7790).
14461559 export async function pullLandPastSubmit(ctx: Ctx): Promise<Response> {
14471560 const found = pullContext(ctx);
test/pull-slices.test.ts added +238
@@ -0,0 +1,238 @@
1 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
2 import { mkdtemp, rm } from "node:fs/promises";
3 import { tmpdir } from "node:os";
4 import { join } from "node:path";
5 import { createApp, type App } from "../src/app.ts";
6 import { serve, type Servers } from "../src/http/server.ts";
7 import { gitClientEnv } from "./helpers/git-client.ts";
8 import { proveAll } from "./helpers/verified.ts";
9 import { promote } from "./helpers/members.ts";
10 import { tokenFor } from "./helpers/tokens.ts";
11 import { setHold, releaseHold } from "../src/core/holds.ts";
12 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
13 import { getPull } from "../src/core/pulls.ts";
14 import { getUserByName } from "../src/core/users.ts";
15 import { getRepo } from "../src/core/repos.ts";
16
17 const gitEnv = gitClientEnv({
18 GIT_AUTHOR_NAME: "Jay Moran",
19 GIT_AUTHOR_EMAIL: "[email protected]",
20 GIT_COMMITTER_NAME: "Jay Moran",
21 GIT_COMMITTER_EMAIL: "[email protected]",
22 });
23
24 describe("a fork's pull lands in slices (#9010)", () => {
25 let dataDir: string;
26 let work: string;
27 let app: App;
28 let servers: Servers;
29 let base: string;
30 let alice = "";
31 let bob = "";
32
33 async function git(args: string[], cwd: string): Promise<string> {
34 const p = Bun.spawn(["git", ...args], { cwd, env: gitEnv, stdout: "pipe", stderr: "pipe" });
35 const [out, err, code] = await Promise.all([
36 new Response(p.stdout).text(),
37 new Response(p.stderr).text(),
38 p.exited,
39 ]);
40 if (code !== 0) throw new Error(`git ${args.join(" ")}: ${err}`);
41 return out.trim();
42 }
43 const call = (path: string, token: string, body?: unknown): Promise<Response> =>
44 fetch(`${base}${path}`, {
45 method: body === undefined ? "GET" : "POST",
46 headers: { authorization: `Bearer ${token}`, "content-type": "application/json" },
47 ...(body === undefined ? {} : { body: JSON.stringify(body) }),
48 });
49 const authed = (user: string, token: string) =>
50 `${base.replace("http://", `http://${user}:${token}@`)}`;
51
52 let cookie = "";
53 async function signup(username: string): Promise<string> {
54 const signed = await fetch(`${base}/signup`, {
55 method: "POST",
56 body: new URLSearchParams({ username, email: `${username}@f.dev`, password: "kept-fork-pass" }),
57 redirect: "manual",
58 });
59 if (username === "alice") cookie = (signed.headers.get("set-cookie") ?? "").split(";")[0]!;
60 return tokenFor(app, username, "admin", "k");
61 }
62
63 beforeAll(async () => {
64 dataDir = await mkdtemp(join(tmpdir(), "forge-sl-"));
65 work = await mkdtemp(join(tmpdir(), "forge-sl-work-"));
66 app = createApp({ dataDir, port: 0 });
67 servers = await serve(app);
68 base = servers.url;
69 alice = await signup("alice");
70 bob = await signup("bob");
71 proveAll(app);
72 promote(app.db);
73 await call("/api/v1/repos", alice, { name: "app", visibility: "public" });
74 await git(["clone", `${authed("alice", alice)}/alice/app.git`, "a"], work);
75 await Bun.write(join(work, "a/one.txt"), "one\n");
76 await git(["add", "."], join(work, "a"));
77 await git(["commit", "-m", "seed"], join(work, "a"));
78 await git(["push", "origin", "main"], join(work, "a"));
79 await call("/api/v1/repos/alice/app/forks", bob, {});
80 });
81
82 afterAll(async () => {
83 servers.stop();
84 app.close();
85 await rm(dataDir, { recursive: true, force: true });
86 await rm(work, { recursive: true, force: true });
87 });
88
89 test("each slice merges the author's commits as they are, the last merges the head, and the author's merge of main is a fast-forward", async () => {
90 const b = join(work, "b");
91 await git(["clone", `${authed("bob", bob)}/bob/app.git`, "b"], work);
92 const mine: string[] = [];
93 for (const n of ["x", "y", "z"]) {
94 await Bun.write(join(b, `${n}.txt`), `${n}\n`);
95 await git(["add", "."], b);
96 await git(["commit", "-m", `add ${n}`], b);
97 mine.push(await git(["rev-parse", "HEAD"], b));
98 }
99 await git(["push", "origin", "main"], b);
100 const opened = await call("/api/v1/repos/alice/app/pulls", bob, {
101 title: "Jay's long work",
102 base: "main",
103 head: "main",
104 headRepo: "bob/app",
105 });
106 expect(opened.status).toBe(201);
107 const { number } = (await opened.json()) as { number: number };
108
109 const a = join(work, "a");
110 await Bun.write(join(a, "two.txt"), "two\n");
111 await git(["add", "."], a);
112 await git(["commit", "-m", "alice moves on"], a);
113 await git(["push", "origin", "main"], a);
114
115 const slice = (token: string, upTo: string) =>
116 call(`/api/v1/repos/alice/app/pulls/${number}/slices`, token, { upTo });
117
118 // Someone who cannot write cannot land a slice; nor can anything not his.
119 expect((await slice(bob, mine[0]!)).status).toBeGreaterThanOrEqual(403);
120 expect((await slice(alice, "not-an-oid")).status).toBe(422);
121 const alicesTip = await git(["rev-parse", "HEAD"], a);
122 expect((await slice(alice, alicesTip)).status).toBe(422);
123
124 const first = await slice(alice, mine[0]!);
125 expect(first.status).toBe(200);
126 expect(await first.json()).toMatchObject({ last: false, left: 2 });
127 await git(["pull", "--no-rebase", "origin", "main"], a);
128 expect((await git(["log", "-1", "--format=%P"], a)).split(" ")[1]).toBe(mine[0]!);
129 // Landed already: the same end again is refused.
130 expect((await slice(alice, mine[0]!)).status).toBe(422);
131
132 const page = async () =>
133 (await fetch(`${base}/alice/app/pulls/${number}/files`, { headers: { cookie } })).text();
134 const shown = await page();
135 expect(shown).toContain("Slice 1: up to");
136 expect(shown).toContain("2 commits left to land.");
137 expect(shown).toContain(`value="${mine[1]}"`);
138
139 expect(await (await slice(alice, mine[1]!)).json()).toMatchObject({ last: false, left: 1 });
140 const last = await slice(alice, mine[2]!);
141 expect(last.status).toBe(200);
142 expect(await last.json()).toMatchObject({ last: true, left: 0 });
143
144 const after = await page();
145 expect(after).toContain("Slice 2: up to");
146 expect(after).toContain("Nothing left");
147 const pull = (await (await call(`/api/v1/repos/alice/app/pulls/${number}`, alice)).json()) as { merged?: boolean; state?: string; mergedAt?: string | null };
148 expect(pull.mergedAt ?? (pull.merged ? "yes" : null)).not.toBeNull();
149
150 // Every commit of the author's is an ancestor of main, unrewritten.
151 await git(["pull", "--no-rebase", "origin", "main"], a);
152 for (const oid of mine) await git(["merge-base", "--is-ancestor", oid, "HEAD"], a);
153 // The last merge takes the head itself, or the combined branch built on it (#9027).
154 const lastSecond = (await git(["log", "-1", "--format=%P"], a)).split(" ")[1]!;
155 await git(["merge-base", "--is-ancestor", mine[2]!, lastSecond], a);
156
157 // The author's side: main comes back as a fast-forward.
158 await git(["fetch", `${authed("alice", alice)}/alice/app.git`, "main"], b);
159 await git(["merge", "--ff-only", "FETCH_HEAD"], b);
160 expect(await git(["rev-parse", "HEAD"], b)).toBe(await git(["rev-parse", "HEAD"], a));
161 });
162
163 test("a slice before the head passes the whole pull's gate: no hold, no red check, a fork only", async () => {
164 const b = join(work, "b");
165 await git(["checkout", "-b", "more"], b);
166 const mine: string[] = [];
167 for (const n of ["p", "q"]) {
168 await Bun.write(join(b, `${n}.txt`), `${n}\n`);
169 await git(["add", "."], b);
170 await git(["commit", "-m", `add ${n}`], b);
171 mine.push(await git(["rev-parse", "HEAD"], b));
172 }
173 await git(["push", "origin", "more"], b);
174 const opened = await call("/api/v1/repos/alice/app/pulls", bob, {
175 title: "More",
176 base: "main",
177 head: "more",
178 headRepo: "bob/app",
179 });
180 expect(opened.status).toBe(201);
181 const { number } = (await opened.json()) as { number: number };
182 const repo = getRepo(app.db, "alice", "app")!;
183 const pull = getPull(app.db, repo.id, number)!;
184 const slice = (upTo: string) =>
185 call(`/api/v1/repos/alice/app/pulls/${number}/slices`, alice, { upTo });
186
187 setHold(app.db, pull.id, getUserByName(app.db, "alice")!.id, "wait for the release");
188 const held = await slice(mine[0]!);
189 expect(held.status).toBe(409);
190 expect(await held.json()).toEqual({ error: "held" });
191 // The page's form says so in its flash, and lands nothing.
192 const form = async (upTo: string): Promise<string> => {
193 const posted = await fetch(`${base}/alice/app/pulls/${number}/slice`, {
194 method: "POST",
195 headers: { cookie, origin: base, "content-type": "application/x-www-form-urlencoded" },
196 body: new URLSearchParams({ upTo }),
197 redirect: "manual",
198 });
199 expect(posted.status).toBe(303);
200 expect(posted.headers.get("location")).toBe(`/alice/app/pulls/${number}/files`);
201 const flash = /forge_flash=([^;]*)/.exec(posted.headers.get("set-cookie") ?? "")?.[1] ?? "";
202 return (JSON.parse(decodeURIComponent(flash)) as { message: string }).message;
203 };
204 expect(await form(mine[0]!)).toContain("A hold stands on this pull");
205 releaseHold(app.db, pull.id, getUserByName(app.db, "alice")!.id);
206 expect(await form("0".repeat(40))).toContain("not one of this pull's");
207
208 // A head the review gate stops stops every slice of it too.
209 app.db.query("UPDATE repo SET require_review = 1 WHERE id = ?1").run(repo.id);
210 const unreviewed = await slice(mine[0]!);
211 expect(unreviewed.status).toBe(409);
212 expect(await unreviewed.json()).toEqual({ error: "review_required" });
213 app.db.query("UPDATE repo SET require_review = 0 WHERE id = ?1").run(repo.id);
214
215 const run = createRun(app.db, { repoId: repo.id, kind: "check", issueId: pull.id, env: `pull-${number}`, commitOid: mine[1]! });
216 claimRun(app.db, run.id);
217 finishRun(app.db, run.id, { state: "failed", summary: "red" });
218 const red = await slice(mine[0]!);
219 expect(red.status).toBe(409);
220 expect(await red.json()).toEqual({ error: "checks_required" });
221
222 // A branch of the repository itself lands whole.
223 const a = join(work, "a");
224 await git(["checkout", "-b", "local"], a);
225 for (const n of ["r", "s"]) {
226 await Bun.write(join(a, `${n}.txt`), `${n}\n`);
227 await git(["add", "."], a);
228 await git(["commit", "-m", `add ${n}`], a);
229 }
230 const first = await git(["rev-parse", "HEAD~1"], a);
231 await git(["push", "origin", "local"], a);
232 const own = await call("/api/v1/repos/alice/app/pulls", alice, { title: "Own", base: "main", head: "local" });
233 const ownNumber = ((await own.json()) as { number: number }).number;
234 const notFork = await call(`/api/v1/repos/alice/app/pulls/${ownNumber}/slices`, alice, { upTo: first });
235 expect(notFork.status).toBe(422);
236 expect(await notFork.json()).toEqual({ error: "not_fork" });
237 });
238 });
test/tui-pull-slice.test.ts added +148
@@ -0,0 +1,148 @@
1 /**
2 * The terminal's pull screen lands a fork's pull in slices with `S`: offered
3 * to a person who may write on an open pull from a fork, and to no one else;
4 * picking a commit lands the author's commits up to it (#9010).
5 */
6 import { afterAll, beforeAll, 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 { createAgent } from "../src/actions/crew.ts";
11 import { openPull } from "../src/actions/pulls.ts";
12 import { commitToBranch, createRepo, forkRepo } from "../src/actions/repos.ts";
13 import { createApp, type App } from "../src/app.ts";
14 import { getPull } from "../src/core/pulls.ts";
15 import { getRepoById, type Repo } from "../src/core/repos.ts";
16 import { createUser, getUserByName, type User } from "../src/core/users.ts";
17 import { resolveOid } from "../src/git/merge.ts";
18 import { repoDir } from "../src/git/repos.ts";
19 import { createForgeApp } from "../src/tui/forge.ts";
20 import { pullScreen } from "../src/tui/screens/pull.ts";
21 import { local } from "../src/tui/screens/where.ts";
22 import { headlessTerminal } from "../src/tui/term/terminal.ts";
23 import { quietLogs } from "./helpers/globals.ts";
24 import { settleApp } from "./helpers/tui.ts";
25 import { promote } from "./helpers/members.ts";
26
27 quietLogs();
28
29 let dataDir: string;
30 let app: App;
31
32 beforeAll(async () => {
33 dataDir = await mkdtemp(join(tmpdir(), "forge-tui-slice-"));
34 app = createApp({ dataDir, port: 0, host: "forge.test" });
35 });
36
37 afterAll(async () => {
38 app.close();
39 await rm(dataDir, { recursive: true, force: true });
40 });
41
42 async function person(username: string, adminIfFirst = false): Promise<User> {
43 const made = await createUser(app.db, {
44 origin: "seed",
45 trusted: true,
46 username,
47 email: `${username}@forge.local`,
48 password: "a-good-passphrase",
49 adminIfFirst,
50 });
51 if (!made.ok) throw new Error(String(made.error));
52 return made.value;
53 }
54
55 async function commit(user: User, repo: Repo, branch: string, path: string): Promise<string> {
56 const c = await commitToBranch(app, user, repo, {
57 branch,
58 message: `add ${path}`,
59 files: [{ path, content: `${path}\n` }],
60 });
61 if (!c.ok) throw new Error(JSON.stringify(c.error));
62 return (await resolveOid(repoDir(app.reposRoot, repo.ownerName, repo.name), `refs/heads/${branch}`))!;
63 }
64
65 test("S is offered to a writer on an open fork pull, to no agent, reader or branch pull; a pick lands the slice", async () => {
66 const alice = await person("alice", true);
67 const bob = await person("bob");
68 promote(app.db, bob);
69 const r = await createRepo(app, alice, { name: "lib", visibility: "public" });
70 if (!r.ok) throw new Error(String(r.error));
71 let repo = r.value;
72 await commit(alice, repo, "main", "seed.md");
73 const forked = await forkRepo(app, bob, repo);
74 if (!forked.ok) throw new Error(String(forked.error));
75 const fork = forked.value;
76 const mine = [await commit(bob, fork, "main", "x.md"), await commit(bob, fork, "main", "y.md")];
77 const fromFork = await openPull(app, bob, repo, {
78 title: "Bob's work",
79 body: "",
80 baseBranch: "main",
81 headBranch: "main",
82 headRepo: fork,
83 });
84 if (!fromFork.ok) throw new Error(String(fromFork.error));
85 await commit(alice, repo, "feat", "feat.md");
86 const own = await openPull(app, alice, repo, {
87 title: "Alice's branch",
88 body: "",
89 baseBranch: "main",
90 headBranch: "feat",
91 });
92 if (!own.ok) throw new Error(String(own.error));
93 const agent = await createAgent(app, alice, { username: "bot" });
94 if (!agent.ok) throw new Error(String(agent.error));
95 repo = getRepoById(app.db, repo.id)!;
96
97 const open = async (user: User, number: number) => {
98 const { app: tui, session } = createForgeApp(
99 app,
100 { sshUsername: user.username, remoteAddress: "127.0.0.1", fingerprint: "SHA256:t", user },
101 { terminal: headlessTerminal(160, 44), exitHooks: false },
102 );
103 tui.start();
104 tui.ctx.push(pullScreen(session, tui.ctx, local(session, repo), number));
105 await settleApp(tui);
106 const press = async (...keys: string[]) => {
107 for (const key of keys)
108 tui.dispatch({ type: "key", key, ctrl: false, alt: false, shift: false });
109 await settleApp(tui);
110 };
111 return { tui, press, text: () => tui.screen.toText() };
112 };
113 const picker = (n: number) => `Land #${n} up to`;
114 const number = fromFork.value.number;
115
116 // Not an agent, not the author who cannot write here, not a branch of the repository.
117 for (const [who, n] of [
118 [getUserByName(app.db, "bot")!, number],
119 [bob, number],
120 [alice, own.value.number],
121 ] as const) {
122 const s = await open(who, n);
123 try {
124 await s.press("S");
125 expect(s.text()).not.toContain(picker(n));
126 } finally {
127 s.tui.stop();
128 }
129 }
130
131 const s = await open(alice, number);
132 try {
133 await s.press("S");
134 expect(s.text()).toContain(picker(number));
135 expect(s.text()).toContain(mine[0]!.slice(0, 7));
136 expect(s.text()).toContain("head: the last slice");
137 // Oldest first: enter takes the first of the author's commits.
138 await s.press("enter");
139 expect(s.text()).toContain("slice landed, 1 left");
140 } finally {
141 s.tui.stop();
142 }
143 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
144 const tip = (await resolveOid(dir, "refs/heads/main"))!;
145 const parents = (await Bun.$`git -C ${dir} log -1 --format=%P ${tip}`.text()).trim().split(" ");
146 expect(parents[1]).toBe(mine[0]!);
147 expect(getPull(app.db, repo.id, number)!.mergedAt).toBeNull();
148 }, 60_000);

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs