trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

A shipped change can be reverted: the revert commit, headless #198

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/189-a-shipped-change-can-be-reverted-the-rev → main 1 commit fork

  • Merged. scout/189-a-shipped-change-can-be-reverted-the-rev was merged into main as 54da230.

Review

Sign in to review this change.
7 files changed +429 −10
src/actions/pulls.ts +4
@@ -74,6 +74,9 @@ export interface OpenPullInput {
7474 headBranch: string;
7575 /** For a fork proposing upstream: the fork the head branch lives in. */
7676 headRepo?: Repo;
77 /** The issue id of the pull this one reverts ("Take it back"). Native
78 * repos only — a mirror's pull is the origin's, which has no such field. */
79 revertsIssueId?: string;
7780 }
7881
7982 export async function openPull(
@@ -171,6 +174,7 @@ export async function openPull(
171174 headDir: repoDir(app.reposRoot, head.ownerName, head.name),
172175 }
173176 : {}),
177 ...(input.revertsIssueId ? { revertsIssueId: input.revertsIssueId } : {}),
174178 },
175179 );
176180 if (!created.ok) return created;
src/actions/revert.ts added +96
@@ -0,0 +1,96 @@
1 /**
2 * Take it back: land a change that undoes exactly what a shipped pull
3 * merged. It never writes to the default branch itself — a branch, a
4 * revert commit built as plumbing (`git/revert.ts`), and a pull whose title
5 * says what it undoes, exactly like any other change a person opens. Only
6 * a fresh branch and a fresh pull commit if the revert applies cleanly;
7 * anything that does not land cleanly leaves nothing behind.
8 */
9
10 import type { App } from "../app.ts";
11 import { revertOf, type Pull } from "../core/pulls.ts";
12 import type { Repo } from "../core/repos.ts";
13 import type { User } from "../core/users.ts";
14 import { resolveOid } from "../git/merge.ts";
15 import { repoDir } from "../git/repos.ts";
16 import { revertCommit, type RevertError } from "../git/revert.ts";
17 import { err, ok, type Result } from "../kernel/result.ts";
18 import { requireWriter, type AccessError } from "./access.ts";
19 import { createBranch, deleteBranch } from "./branches.ts";
20 import { openPull } from "./pulls.ts";
21
22 export type RevertPullError =
23 | AccessError
24 | RevertError
25 | "not_shipped"
26 | "mirrored"
27 | "already_reverted"
28 | { kind: "branch_failed"; reason: string }
29 | { kind: "pull_failed"; reason: string };
30
31 export async function revertPull(
32 app: App,
33 actor: User | undefined,
34 repo: Repo,
35 pull: Pull,
36 ): Promise<Result<Pull, RevertPullError>> {
37 const writer = requireWriter(app, actor, repo);
38 if (!writer.ok) return writer;
39 if (pull.repoId !== repo.id) return err("not_found");
40 // Rule 1 in docs/APPS.md: the crew proposes, a person lands. Taking a
41 // change back is a landing verb, not a proposing one.
42 if (writer.value.kind === "agent") return err("forbidden");
43 if (pull.mergedAt === null || pull.mergedOid === null)
44 return err("not_shipped");
45 // On a mirror the origin owns history; a local revert here would be
46 // overwritten by the next sync, exactly as a local merge would.
47 if (repo.origin !== null || pull.originHeadRepo !== null)
48 return err("mirrored");
49 if (revertOf(app.db, repo.id, pull.id) !== undefined)
50 return err("already_reverted");
51
52 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
53 const onto = await resolveOid(dir, `refs/heads/${repo.defaultBranch}`);
54 if (onto === undefined)
55 return err({ kind: "git", message: "the default branch has no tip" });
56
57 const built = await revertCommit(dir, {
58 commitOid: pull.mergedOid,
59 ontoOid: onto,
60 message: `Revert "${pull.title}" (#${pull.number})`,
61 author: {
62 name: writer.value.displayName || writer.value.username,
63 email: writer.value.email,
64 },
65 });
66 if (!built.ok) return built;
67
68 // The branch name is the server's own — never taken from the request —
69 // so it always says exactly what it reverts.
70 const branchName = `revert-${pull.number}`;
71 const branch = await createBranch(app, writer.value, repo, {
72 name: branchName,
73 from: built.value.commitOid,
74 });
75 if (!branch.ok) {
76 const reason = typeof branch.error === "string" ? branch.error : "git";
77 return err({ kind: "branch_failed", reason });
78 }
79
80 const opened = await openPull(app, writer.value, repo, {
81 title: `Revert "${pull.title}"`,
82 body: `Reverts #${pull.number}.`,
83 baseBranch: repo.defaultBranch,
84 headBranch: branchName,
85 revertsIssueId: pull.id,
86 });
87 if (!opened.ok) {
88 // The branch was made for this pull and the pull did not open: take it
89 // back so a refused revert leaves nothing half-made behind, the same
90 // rule `editFiles` (`actions/contents.ts`) follows for the same reason.
91 await deleteBranch(app, writer.value, repo, branchName);
92 const reason = typeof opened.error === "string" ? opened.error : "git";
93 return err({ kind: "pull_failed", reason });
94 }
95 return ok(opened.value);
96 }
src/core/pulls.ts +32 −2
@@ -51,6 +51,8 @@ export interface Pull extends Issue {
5151 * not hold): `owner/name` there. Null for a native pull.
5252 */
5353 readonly originHeadRepo: string | null;
54 /** The pull this one reverts, when it was opened by "Take it back". */
55 readonly revertsPullId: string | null;
5456 }
5557
5658 interface PullRow {
@@ -62,6 +64,7 @@ interface PullRow {
6264 merged_oid: string | null;
6365 head_repo_id: string | null;
6466 origin_head_repo: string | null;
67 reverts_issue_id: string | null;
6568 }
6669
6770 function fromRow(issue: Issue, row: PullRow): Pull {
@@ -77,6 +80,7 @@ function fromRow(issue: Issue, row: PullRow): Pull {
7780 headRepoId,
7881 crossRepo: headRepoId !== issue.repoId,
7982 originHeadRepo: row.origin_head_repo,
83 revertsPullId: row.reverts_issue_id,
8084 };
8185 }
8286
@@ -110,6 +114,8 @@ export interface CreatePullInput {
110114 headRepoId?: string;
111115 /** The fork's on-disk dir, required when headRepoId differs from the base. */
112116 headDir?: string;
117 /** The issue id of the pull this one reverts ("Take it back"). */
118 revertsIssueId?: string;
113119 }
114120
115121 export async function createPull(
@@ -148,8 +154,8 @@ export async function createPull(
148154
149155 const headRepoId = input.headRepoId ?? input.repoId;
150156 db.query(
151 `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id)
152 VALUES (?1, ?2, ?3, ?4, ?5, ?6)`,
157 `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id, reverts_issue_id)
158 VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)`,
153159 ).run(
154160 issue.value.id,
155161 input.baseBranch,
@@ -157,6 +163,7 @@ export async function createPull(
157163 baseOid,
158164 headOid,
159165 headRepoId,
166 input.revertsIssueId ?? null,
160167 );
161168
162169 // A durable, fetchable ref for the PR head — the way tooling addresses it,
@@ -199,6 +206,29 @@ export function pullForBranch(
199206 return row ? getPull(db, repoId, row.number) : undefined;
200207 }
201208
209 /**
210 * The pull that already reverts this one, if taking it back has already
211 * been done — open, or merged, either one counts as "already taken back".
212 * A revert pull that was closed *without* merging is an abandoned attempt,
213 * not a standing answer, so it does not block a second try.
214 */
215 export function revertOf(
216 db: Database,
217 repoId: string,
218 pullId: string,
219 ): Pull | undefined {
220 const row = db
221 .query<
222 { number: number },
223 [string, string]
224 >(`SELECT issue.number AS number FROM pull JOIN issue ON issue.id = pull.issue_id
225 WHERE issue.repo_id = ?1 AND pull.reverts_issue_id = ?2
226 AND (issue.state = 'open' OR pull.merged_at IS NOT NULL)
227 ORDER BY issue.created_at DESC LIMIT 1`)
228 .get(repoId, pullId);
229 return row ? getPull(db, repoId, row.number) : undefined;
230 }
231
202232 /**
203233 * Open pulls, in any repository, whose head is this branch of this one:
204234 * what a push to a fork moves.
src/core/schema.ts +9
@@ -783,4 +783,13 @@ export const migrations: readonly string[] = [
783783 DROP TABLE look;
784784 ALTER TABLE look_new RENAME TO look;
785785 `,
786
787 // 27 — take it back: a pull that reverts another, named explicitly rather
788 // than sniffed from its title. `reverts_issue_id` points at the issue the
789 // shipped pull was (a pull is its own issue), so "was this already taken
790 // back" is one lookup rather than a guess from wording.
791 `
792 ALTER TABLE pull ADD COLUMN reverts_issue_id TEXT REFERENCES issue(id) ON DELETE SET NULL;
793 CREATE INDEX pull_reverts ON pull(reverts_issue_id) WHERE reverts_issue_id IS NOT NULL;
794 `,
786795 ];
src/git/merge.ts +36 −8
@@ -30,23 +30,38 @@ export interface MergeResult {
3030 }
3131
3232 /**
33 * Can head be merged into base, and if so what would the tree be? Pure
34 * computation — writes nothing. Returns the merged tree oid or the conflicted
35 * paths. Used both to show mergeability and as the first half of a merge.
33 * The plumbing both `mergeable` and a revert's three-way merge share: run
34 * `merge-tree --write-tree` over two sides and report the merged tree oid or
35 * the conflicted paths. `base` overrides the merge base git would otherwise
36 * compute from history — a revert needs the commit being undone as the base,
37 * not whatever `ours`/`theirs` share an ancestor with.
3638 */
37 export async function mergeable(
39 export async function writeMergeTree(
3840 dir: string,
39 baseOid: string,
40 headOid: string,
41 ours: string,
42 theirs: string,
43 base?: string,
4144 ): Promise<Result<{ treeOid: string }, MergeError>> {
42 if (!isSafeOid(baseOid) || !isSafeOid(headOid))
45 if (
46 !isSafeOid(ours) ||
47 !isSafeOid(theirs) ||
48 (base !== undefined && !isSafeOid(base))
49 ) {
4350 return err({ kind: "unsafe" });
51 }
4452
4553 // merge-tree exits 0 on a clean merge (stdout = tree oid) and 1 on conflict
4654 // (stdout = tree oid, blank line, then conflicted file names). Both outcomes
4755 // are data on stdout, so capture regardless of exit code.
4856 const out = await runGitCapture(
49 ["merge-tree", "--write-tree", "--name-only", baseOid, headOid],
57 [
58 "merge-tree",
59 "--write-tree",
60 "--name-only",
61 ...(base === undefined ? [] : [`--merge-base=${base}`]),
62 ours,
63 theirs,
64 ],
5065 { cwd: dir, timeoutMs: 60_000 },
5166 );
5267
@@ -70,6 +85,19 @@ export async function mergeable(
7085 });
7186 }
7287
88 /**
89 * Can head be merged into base, and if so what would the tree be? Pure
90 * computation — writes nothing. Returns the merged tree oid or the conflicted
91 * paths. Used both to show mergeability and as the first half of a merge.
92 */
93 export async function mergeable(
94 dir: string,
95 baseOid: string,
96 headOid: string,
97 ): Promise<Result<{ treeOid: string }, MergeError>> {
98 return writeMergeTree(dir, baseOid, headOid);
99 }
100
73101 /** Land head onto base with the chosen strategy, only if base is unmoved. */
74102 export async function merge(
75103 dir: string,
src/git/revert.ts added +79
@@ -0,0 +1,79 @@
1 /**
2 * Reverting a shipped commit as plumbing — the same three-way merge `git
3 * revert` runs, computed with `merge-tree` instead of a checkout: the commit
4 * being undone is the merge base, the branch it landed on is one side, and
5 * the commit's own parent (the state right before it) is the other. Any
6 * change made since keeps its place; only what this one commit did is
7 * undone, and a real conflict is reported rather than guessed at.
8 */
9
10 import { err, ok, type Result } from "../kernel/result.ts";
11 import { runGit } from "./env.ts";
12 import { isSafeOid } from "./guard.ts";
13 import { writeMergeTree, type MergeError, type MergeIdentity } from "./merge.ts";
14
15 export type RevertError =
16 | MergeError
17 /** More than two parents: an octopus merge the forge never makes itself,
18 * so reverting one would be a guess about which side is "the" undo. */
19 | { kind: "octopus"; parents: number };
20
21 export interface RevertResult {
22 readonly commitOid: string;
23 }
24
25 /** A commit's parent oids, in order; empty for a root commit. */
26 export async function commitParents(
27 dir: string,
28 oid: string,
29 ): Promise<string[] | undefined> {
30 if (!isSafeOid(oid)) return undefined;
31 const result = await runGit(["rev-parse", `${oid}^@`], { cwd: dir });
32 if (!result.ok) return undefined;
33 return result.value
34 .split("\n")
35 .map((line) => line.trim())
36 .filter((line) => line !== "");
37 }
38
39 /**
40 * Build the commit that undoes `commitOid`, staged on top of `ontoOid`. A
41 * squash merge (the forge's own default) has one parent, which is exactly
42 * the state to restore; an ordinary merge's first parent is the base
43 * branch's own side — `git revert -m 1`'s rule, and the only mainline the
44 * forge's own merges ever produce, so that is the one used without asking.
45 */
46 export async function revertCommit(
47 dir: string,
48 input: {
49 commitOid: string;
50 ontoOid: string;
51 message: string;
52 author: MergeIdentity;
53 },
54 ): Promise<Result<RevertResult, RevertError>> {
55 if (!isSafeOid(input.commitOid) || !isSafeOid(input.ontoOid))
56 return err({ kind: "unsafe" });
57
58 const parents = await commitParents(dir, input.commitOid);
59 if (parents === undefined || parents.length === 0)
60 return err({ kind: "git", message: "could not read the change's parent" });
61 if (parents.length > 2) return err({ kind: "octopus", parents: parents.length });
62 const mainline = parents[0]!;
63
64 const tree = await writeMergeTree(dir, input.ontoOid, mainline, input.commitOid);
65 if (!tree.ok) return tree;
66
67 const env = {
68 GIT_AUTHOR_NAME: input.author.name,
69 GIT_AUTHOR_EMAIL: input.author.email,
70 GIT_COMMITTER_NAME: input.author.name,
71 GIT_COMMITTER_EMAIL: input.author.email,
72 };
73 const commit = await runGit(
74 ["commit-tree", tree.value.treeOid, "-p", input.ontoOid, "-m", input.message],
75 { cwd: dir, env },
76 );
77 if (!commit.ok) return err({ kind: "git", message: commit.error.stderr });
78 return ok({ commitOid: commit.value.trim() });
79 }
test/revert.test.ts added +173
@@ -0,0 +1,173 @@
1 /**
2 * Take it back, headless: `revertPull` turns a shipped change into a new
3 * branch and pull that undoes it, without touching the default branch and
4 * without a page in front of it. Everything here is driven through actions
5 * directly, the way `test/crew.test.ts` builds its repositories — no
6 * `git clone`/`git push` over HTTP, which does not run in this sandbox.
7 */
8
9 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
10 import { mkdtemp, rm } from "node:fs/promises";
11 import { tmpdir } from "node:os";
12 import { join } from "node:path";
13 import { createApp, type App } from "../src/app.ts";
14 import { mergePull, openPull } from "../src/actions/pulls.ts";
15 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
16 import { revertPull } from "../src/actions/revert.ts";
17 import { getPull, revertOf, type Pull } from "../src/core/pulls.ts";
18 import type { Repo } from "../src/core/repos.ts";
19 import { addToCrew, createAgent } from "../src/actions/crew.ts";
20 import { createUser, type User } from "../src/core/users.ts";
21 import { resolveOid } from "../src/git/merge.ts";
22 import { repoDir } from "../src/git/repos.ts";
23
24 describe("take it back: reverting a shipped change, headless", () => {
25 let dataDir: string;
26 let app: App;
27 let alice: User;
28 let repo: Repo;
29 let dir: string;
30
31 const contentAt = async (ref: string, path: string): Promise<string> => {
32 const p = Bun.spawn(["git", "show", `${ref}:${path}`], {
33 cwd: dir,
34 stdout: "pipe",
35 stderr: "ignore",
36 });
37 return new Response(p.stdout).text();
38 };
39
40 const branchExists = async (name: string): Promise<boolean> =>
41 (await resolveOid(dir, `refs/heads/${name}`)) !== undefined;
42
43 const shipChange = async (
44 branch: string,
45 title: string,
46 content: string,
47 ): Promise<Pull> => {
48 await commitToBranch(app, alice, repo, {
49 branch,
50 base: "main",
51 message: title,
52 files: [{ path: "code.txt", content }],
53 });
54 const opened = await openPull(app, alice, repo, {
55 title,
56 baseBranch: "main",
57 headBranch: branch,
58 });
59 if (!opened.ok) throw new Error(JSON.stringify(opened.error));
60 const merged = await mergePull(app, alice, repo, opened.value, "squash");
61 if (!merged.ok) throw new Error(JSON.stringify(merged.error));
62 return getPull(app.db, repo.id, opened.value.number)!;
63 };
64
65 beforeAll(async () => {
66 dataDir = await mkdtemp(join(tmpdir(), "forge-revert-"));
67 app = createApp({ dataDir, port: 0 });
68
69 const created = await createUser(app.db, {
70 username: "alice",
71 email: "[email protected]",
72 password: "a-good-passphrase",
73 adminIfFirst: true,
74 });
75 if (!created.ok) throw new Error(created.error);
76 alice = created.value;
77
78 const madeRepo = await createRepo(app, alice, {
79 name: "app",
80 visibility: "public",
81 });
82 if (!madeRepo.ok) throw new Error(String(madeRepo.error));
83 repo = madeRepo.value;
84 dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
85
86 await commitToBranch(app, alice, repo, {
87 branch: "main",
88 message: "base",
89 files: [{ path: "code.txt", content: "alpha\n" }],
90 });
91 });
92
93 afterAll(async () => {
94 app.close();
95 await rm(dataDir, { recursive: true, force: true });
96 });
97
98 test("a test fails on today's code: ship it, take it back, main only changes once the revert lands", async () => {
99 const shipped = await shipChange("loud", "Make it loud", "alpha\nBETA\n");
100 expect(await contentAt("main", "code.txt")).toBe("alpha\nBETA\n");
101
102 const reverted = await revertPull(app, alice, repo, shipped);
103 expect(reverted.ok).toBe(true);
104 if (!reverted.ok) return;
105 const revertPullRow = reverted.value;
106 expect(revertPullRow.title).toBe('Revert "Make it loud"');
107 expect(revertPullRow.body).toContain(`Reverts #${shipped.number}`);
108 expect(revertPullRow.revertsPullId).toBe(shipped.id);
109 expect(await branchExists(`revert-${shipped.number}`)).toBe(true);
110
111 // main is untouched until a person merges the revert pull.
112 expect(await contentAt("main", "code.txt")).toBe("alpha\nBETA\n");
113 expect(
114 await contentAt(`revert-${shipped.number}`, "code.txt"),
115 ).toBe("alpha\n");
116
117 const landed = await mergePull(app, alice, repo, revertPullRow, "squash");
118 expect(landed.ok).toBe(true);
119 expect(await contentAt("main", "code.txt")).toBe("alpha\n");
120 });
121
122 test("a change already taken back is refused rather than reverted twice", async () => {
123 const shipped = getPull(app.db, repo.id, 1)!;
124 const again = await revertPull(app, alice, repo, shipped);
125 expect(again).toEqual({ ok: false, error: "already_reverted" });
126 expect(revertOf(app.db, repo.id, shipped.id)).toBeDefined();
127 });
128
129 test("a revert that would conflict changes nothing and leaves no branch behind", async () => {
130 const gamma = await shipChange("gamma", "Add gamma", "alpha\nGAMMA\n");
131 // Change the very same line again, so undoing "Add gamma" can no
132 // longer apply without a conflict.
133 await shipChange("delta", "Change it again", "alpha\nDELTA\n");
134 expect(await contentAt("main", "code.txt")).toBe("alpha\nDELTA\n");
135
136 const refused = await revertPull(app, alice, repo, gamma);
137 expect(refused).toMatchObject({ ok: false, error: { kind: "conflict" } });
138 expect(await contentAt("main", "code.txt")).toBe("alpha\nDELTA\n");
139 expect(await branchExists(`revert-${gamma.number}`)).toBe(false);
140 expect(revertOf(app.db, repo.id, gamma.id)).toBeUndefined();
141 });
142
143 test("an agent may not take a change back — landing is a person's verb", async () => {
144 const agent = await createAgent(app, alice, { username: "bot" });
145 if (!agent.ok) throw new Error(String(agent.error));
146 const added = addToCrew(app, alice, repo, "bot");
147 if (!added.ok) throw new Error(String(added.error));
148 const shipped = await shipChange(
149 "epsilon",
150 "Add epsilon",
151 "alpha\nDELTA\nEPSILON\n",
152 );
153 const refused = await revertPull(app, agent.value.user, repo, shipped);
154 expect(refused).toEqual({ ok: false, error: "forbidden" });
155 });
156
157 test("reverting a pull that never shipped is refused", async () => {
158 await commitToBranch(app, alice, repo, {
159 branch: "zeta",
160 base: "main",
161 message: "not merged",
162 files: [{ path: "code.txt", content: "alpha\nDELTA\nZETA\n" }],
163 });
164 const opened = await openPull(app, alice, repo, {
165 title: "Add zeta",
166 baseBranch: "main",
167 headBranch: "zeta",
168 });
169 if (!opened.ok) throw new Error(JSON.stringify(opened.error));
170 const refused = await revertPull(app, alice, repo, opened.value);
171 expect(refused).toEqual({ ok: false, error: "not_shipped" });
172 });
173 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs