trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

Take it back: the studio's verb on a shipped change #247

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/190-take-it-back-the-studio-s-verb-on-a-ship → main 2 commits fork

  • Merged. scout/190-take-it-back-the-studio-s-verb-on-a-ship was merged into main as 71295b4.

Review

Sign in to review this change.
5 files changed +350 −7
src/http/server.ts +8 −1
@@ -82,6 +82,7 @@ import {
8282 modeSubmit,
8383 saySubmit,
8484 shipSubmit,
85 takeBackSubmit,
8586 tellSubmit,
8687 } from "../web/studio/actions.ts";
8788 import { appStateFragment } from "../web/studio/previews.ts";
@@ -567,10 +568,16 @@ export function buildRouter(): Router<Handler> {
567568 router.add("POST", "/:owner/:name/issues/:number/assignee", (ctx) =>
568569 assigneeSubmit(ctx, "issue"),
569570 );
570 // The studio's three verbs on an app: tell, ship, say.
571 // The studio's verbs on an app: tell, ship, say, and — on a shipped
572 // change — take it back.
571573 router.add("POST", "/:owner/:name/tell", tellSubmit);
572574 router.add("POST", "/:owner/:name/pulls/:number/ship", shipSubmit);
573575 router.add("POST", "/:owner/:name/pulls/:number/say", saySubmit);
576 router.add(
577 "POST",
578 "/:owner/:name/pulls/:number/take-back",
579 takeBackSubmit,
580 );
574581 // What an issue waits for: link and unlink, from the issue page.
575582 router.add(
576583 "POST",
src/web/pages/search.ts +27 −4
@@ -10,6 +10,8 @@
1010 */
1111
1212 import { search } from "../../actions/search.ts";
13 import { getPull } from "../../core/pulls.ts";
14 import { canWrite, getRepo } from "../../core/repos.ts";
1315 import {
1416 searchKind,
1517 type CodeHit,
@@ -379,7 +381,7 @@ interface Destructive {
379381 */
380382 const DESTRUCTIVE: {
381383 pattern: RegExp;
382 describe: (m: RegExpMatchArray) => Destructive;
384 describe: (m: RegExpMatchArray, ctx: Ctx) => Destructive;
383385 }[] = [
384386 {
385387 pattern: /^\/([^/]+)\/([^/]+)\/settings\/delete$/,
@@ -476,12 +478,33 @@ const DESTRUCTIVE: {
476478 phrase: "",
477479 }),
478480 },
481 {
482 pattern: /^\/([^/]+)\/([^/]+)\/pulls\/(\d+)\/take-back$/,
483 describe: (m, ctx) => {
484 // The number in the URL is what the route itself will act on; the
485 // title shown here is looked up fresh rather than trusted from a
486 // link, so a crafted URL cannot put someone else's words on it — and
487 // only shown to someone who could take the change back anyway, so
488 // this page never becomes a way to read a private pull's title.
489 const repo = getRepo(ctx.app.db, m[1]!, m[2]!);
490 const writer = repo && canWrite(ctx.app.db, repo, ctx.auth.user);
491 const pull = writer ? getPull(ctx.app.db, repo.id, Number(m[3])) : undefined;
492 const name = pull ? `"${pull.title}"` : `change #${m[3]}`;
493 return {
494 verb: "Take it back",
495 title: `Take back ${name}?`,
496 detail:
497 "This opens a new change that undoes exactly what it shipped — nothing changes until a person ships that new change, same as any other.",
498 phrase: "",
499 };
500 },
501 },
479502 ];
480503
481 function describeTarget(to: string): Destructive | undefined {
504 function describeTarget(to: string, ctx: Ctx): Destructive | undefined {
482505 for (const entry of DESTRUCTIVE) {
483506 const match = to.match(entry.pattern);
484 if (match) return entry.describe(match);
507 if (match) return entry.describe(match, ctx);
485508 }
486509 return undefined;
487510 }
@@ -506,7 +529,7 @@ export function confirmPage(ctx: Ctx): Response {
506529 ) {
507530 return notFoundPage(ctx);
508531 }
509 const known = describeTarget(to);
532 const known = describeTarget(to, ctx);
510533 // Only routes this file knows are destructive can be confirmed. A crafted
511534 // link therefore cannot invent reassuring words for a dangerous button, and
512535 // cannot aim the form at an endpoint that was never meant for it.
src/web/studio/actions.ts +66
@@ -11,6 +11,7 @@
1111 import { addComment, setIssueStateThrough } from "../../actions/issues.ts";
1212 import { tellCrew } from "../../actions/crew.ts";
1313 import { mergePull } from "../../actions/pulls.ts";
14 import { revertPull, type RevertPullError } from "../../actions/revert.ts";
1415 import { getPull } from "../../core/pulls.ts";
1516 import { bringUpToDate } from "../../crew/autonomy.ts";
1617 import type { Ctx } from "../../http/context.ts";
@@ -107,6 +108,71 @@ export async function shipSubmit(ctx: Ctx): Promise<Response> {
107108 });
108109 }
109110
111 /** A take-back refusal in the studio's words. */
112 function takeBackWords(error: RevertPullError): string {
113 if (typeof error === "string") {
114 switch (error) {
115 case "not_shipped":
116 return "it has not shipped";
117 case "mirrored":
118 return "the origin owns this history";
119 case "already_reverted":
120 return "it has already been taken back";
121 case "unauthorized":
122 return "sign in first";
123 case "forbidden":
124 return "only a person can take a change back";
125 case "not_found":
126 return "that change was not found";
127 }
128 }
129 switch (error.kind) {
130 case "conflict":
131 return `it conflicts with the current code in ${error.files.join(", ")}`;
132 case "unsafe":
133 return "git refused it";
134 case "base_moved":
135 return "the base moved under it";
136 case "git":
137 return `git said ${error.message}`;
138 case "octopus":
139 return "it has more than two parents";
140 case "branch_failed":
141 return `it could not be made ready (${error.reason})`;
142 case "pull_failed":
143 return `the change could not be opened (${error.reason})`;
144 }
145 }
146
147 /**
148 * POST /:owner/:name/pulls/:number/take-back — open a new change that
149 * undoes exactly what a shipped one merged. Never touches the default
150 * branch itself (`revertPull` does the computing); this only calls it and
151 * says the result in the studio's words.
152 */
153 export async function takeBackSubmit(ctx: Ctx): Promise<Response> {
154 const repo = loadRepo(ctx);
155 if (!repo) return notFoundPage(ctx);
156 if (!ctx.auth.user) return signInFirst(ctx);
157 const form = await readForm(ctx, "rw");
158 if (form instanceof Response) return form;
159 const base = repoBase(repo);
160 const number = Number(ctx.params.number);
161 const pull = Number.isInteger(number)
162 ? getPull(ctx.app.db, repo.id, number)
163 : undefined;
164 if (!pull) return notFoundPage(ctx);
165 const reverted = await revertPull(ctx.app, ctx.auth.user, repo, pull);
166 if (!reverted.ok)
167 return flashOrJson(ctx, `${base}/pulls/${pull.number}`, {
168 tone: "danger",
169 message: `That could not be taken back: ${takeBackWords(reverted.error)}.`,
170 });
171 return flashOrJson(ctx, `${base}/pulls/${reverted.value.number}`, {
172 message: `Opened "${reverted.value.title}" — ship it to take "${pull.title}" back.`,
173 });
174 }
175
110176 /** POST /:owner/:name/pulls/:number/say { text } — feedback to the crew. */
111177 export async function saySubmit(ctx: Ctx): Promise<Response> {
112178 const repo = loadRepo(ctx);
src/web/studio/change.ts +30 −2
@@ -9,7 +9,7 @@
99
1010 import { appsRuntime } from "../../apps/runtime.ts";
1111 import type { AppRecord } from "../../core/apps.ts";
12 import { pullStatus, type Pull } from "../../core/pulls.ts";
12 import { pullStatus, revertOf, type Pull } from "../../core/pulls.ts";
1313 import { canWrite, type Repo } from "../../core/repos.ts";
1414 import { checkSummary } from "../../core/runs.ts";
1515 import { repoDir } from "../../git/repos.ts";
@@ -21,6 +21,7 @@ import { liveClientJs, liveMount } from "../live.ts";
2121 import {
2222 btn,
2323 card,
24 confirmAction,
2425 icon,
2526 originTag,
2627 stateBadge,
@@ -207,13 +208,40 @@ export async function studioPull(
207208 }),
208209 );
209210
211 // A shipped change offers one more verb: open a new change that undoes
212 // exactly what it shipped, for someone to ship. Never on a change still
213 // open (nothing shipped to take back yet), never on a mirror (the origin
214 // owns that history), never twice. The action re-checks all of this and
215 // does the git work; this only decides whether to offer the button.
216 const takeBackSection = when(
217 pull.mergedAt !== null &&
218 writer &&
219 repo.origin === null &&
220 pull.originHeadRepo === null &&
221 revertOf(db, repo.id, pull.id) === undefined,
222 () =>
223 card(
224 html`<p class="muted small">
225 Take it back — opens a new change that undoes exactly what this
226 one shipped, for a person to ship.
227 </p>
228 <div class="bar mt">
229 ${confirmAction({
230 action: `${thread}/take-back`,
231 verb: "Take it back",
232 size: "sm",
233 })}
234 </div>`,
235 ),
236 );
237
210238 const talk = conversation(ctx, repo, pull, { openVerb: "opened this" });
211239
212240 const body = html`${repoHeader(ctx, repo, "pulls", {
213241 compact: true,
214242 ownTitle: true,
215243 })}
216 ${head} ${checkLine} ${decisionSection}
244 ${head} ${checkLine} ${decisionSection} ${takeBackSection}
217245 ${lookCard(ctx, repo, record, pull, pull.headOid)}
218246 <div class="split">
219247 <div>
test/studio.test.ts +219
@@ -5085,6 +5085,225 @@ describe("a person's own change ships from its own studio page", () => {
50855085 });
50865086 });
50875087
5088 /**
5089 * Issue #190: a shipped change's studio page offers Take it back to a
5090 * writer, behind the studio's existing confirm, and pressing it opens a new
5091 * change that undoes it.
5092 */
5093 describe("take it back: the studio's verb on a shipped change", () => {
5094 let dataDir: string;
5095 let app: App;
5096 let servers: Servers;
5097 let base: string;
5098 let alice: User;
5099 let repo: Repo;
5100 let ownerCookie: string;
5101 let readerCookie: string;
5102
5103 const signIn = async (username: string): Promise<string> =>
5104 (
5105 (
5106 await fetch(`${base}/signin`, {
5107 method: "POST",
5108 body: new URLSearchParams({
5109 username,
5110 password: "a-good-passphrase",
5111 }),
5112 redirect: "manual",
5113 })
5114 ).headers.get("set-cookie") ?? ""
5115 ).split(";")[0]!;
5116
5117 const get = (path: string, cookie: string) =>
5118 fetch(`${base}${path}`, {
5119 headers: { cookie: `${cookie}; forge_mode=studio` },
5120 redirect: "manual",
5121 });
5122 const post = (path: string, cookie: string, origin = base) =>
5123 fetch(`${base}${path}`, {
5124 method: "POST",
5125 headers: { cookie, origin },
5126 redirect: "manual",
5127 });
5128
5129 const shipChange = async (
5130 branch: string,
5131 title: string,
5132 content: string,
5133 ): Promise<Pull> => {
5134 const committed = await commitToBranch(app, alice, repo, {
5135 branch,
5136 base: "main",
5137 message: title,
5138 files: [{ path: "code.txt", content }],
5139 });
5140 if (!committed.ok) throw new Error(JSON.stringify(committed.error));
5141 const opened = await openPull(app, alice, repo, {
5142 title,
5143 baseBranch: "main",
5144 headBranch: branch,
5145 });
5146 if (!opened.ok) throw new Error(String(opened.error));
5147 const merged = await mergePull(app, alice, repo, opened.value, "squash");
5148 if (!merged.ok) throw new Error(JSON.stringify(merged.error));
5149 return getPull(app.db, repo.id, opened.value.number)!;
5150 };
5151
5152 beforeAll(async () => {
5153 dataDir = await mkdtemp(join(tmpdir(), "forge-take-back-"));
5154 app = createApp({ dataDir, port: 0 });
5155 servers = await serve(app);
5156 base = servers.url;
5157 const a = await createUser(app.db, {
5158 username: "alice",
5159 email: "[email protected]",
5160 password: "a-good-passphrase",
5161 adminIfFirst: true,
5162 });
5163 if (!a.ok) throw new Error(a.error);
5164 alice = a.value;
5165 const r = await createUser(app.db, {
5166 username: "eve",
5167 email: "[email protected]",
5168 password: "a-good-passphrase",
5169 });
5170 if (!r.ok) throw new Error(r.error);
5171 ownerCookie = await signIn("alice");
5172 readerCookie = await signIn("eve");
5173 const made = await createRepo(app, alice, {
5174 name: "orchard",
5175 visibility: "public",
5176 });
5177 if (!made.ok) throw new Error(String(made.error));
5178 repo = made.value;
5179 const seeded = await commitToBranch(app, alice, repo, {
5180 message: "Start",
5181 files: [
5182 {
5183 path: "package.json",
5184 content:
5185 '{"name":"orchard","scripts":{"start":"bun run server.ts"}}\n',
5186 },
5187 {
5188 path: "server.ts",
5189 content:
5190 "Bun.serve({ port: Number(process.env.PORT), fetch: () => new Response('ok') });\n",
5191 },
5192 { path: "code.txt", content: "alpha\n" },
5193 ],
5194 });
5195 if (!seeded.ok) throw new Error(JSON.stringify(seeded.error));
5196 const enabled = await enableApp(app, alice, repo, { slug: "orchard" });
5197 if (!enabled.ok) throw new Error(String(enabled.error));
5198 });
5199
5200 afterAll(async () => {
5201 servers.stop();
5202 app.close();
5203 await rm(dataDir, { recursive: true, force: true });
5204 });
5205
5206 test("a shipped change offers Take it back to a writer, not to a reader, and not while it is still open", async () => {
5207 const branch = await commitToBranch(app, alice, repo, {
5208 branch: "feature/loud",
5209 base: "main",
5210 message: "Make it loud",
5211 files: [{ path: "code.txt", content: "alpha\nBETA\n" }],
5212 });
5213 if (!branch.ok) throw new Error(JSON.stringify(branch.error));
5214 const opened = await openPull(app, alice, repo, {
5215 title: "Make it loud",
5216 baseBranch: "main",
5217 headBranch: "feature/loud",
5218 });
5219 if (!opened.ok) throw new Error(String(opened.error));
5220 const pull = opened.value;
5221
5222 const whileOpen = await (
5223 await get(`/alice/orchard/pulls/${pull.number}`, ownerCookie)
5224 ).text();
5225 expect(whileOpen).not.toContain("Take it back");
5226
5227 const merged = await mergePull(app, alice, repo, pull, "squash");
5228 if (!merged.ok) throw new Error(JSON.stringify(merged.error));
5229
5230 const ownerPage = await (
5231 await get(`/alice/orchard/pulls/${pull.number}`, ownerCookie)
5232 ).text();
5233 expect(ownerPage).toContain("Take it back");
5234 expect(ownerPage).toContain(
5235 `/-/confirm?to=%2Falice%2Forchard%2Fpulls%2F${pull.number}%2Ftake-back`,
5236 );
5237
5238 const readerPage = await (
5239 await get(`/alice/orchard/pulls/${pull.number}`, readerCookie)
5240 ).text();
5241 expect(readerPage).not.toContain("Take it back");
5242
5243 const confirm = await (
5244 await get(
5245 `/-/confirm?to=${encodeURIComponent(`/alice/orchard/pulls/${pull.number}/take-back`)}`,
5246 ownerCookie,
5247 )
5248 ).text();
5249 expect(confirm).toContain("Take back");
5250 expect(confirm).toContain("Make it loud");
5251 expect(confirm).toContain("opens a new change that undoes exactly what it shipped");
5252 });
5253
5254 test("pressing Take it back opens a new change and lands on its studio page", async () => {
5255 const shipped = await shipChange("feature/gamma", "Add gamma", "alpha\nGAMMA\n");
5256
5257 const taken = await post(
5258 `/alice/orchard/pulls/${shipped.number}/take-back`,
5259 ownerCookie,
5260 );
5261 expect(taken.status).toBe(303);
5262 const location = taken.headers.get("location")!;
5263 expect(location).toBe(`/alice/orchard/pulls/${shipped.number + 1}`);
5264
5265 const revertPage = await (await get(location, ownerCookie)).text();
5266 expect(revertPage).toContain(`Revert &quot;Add gamma&quot;`);
5267 });
5268
5269 test("the POST is refused from another origin, and nothing is created", async () => {
5270 const shipped = await shipChange("feature/delta", "Add delta", "alpha\nDELTA\n");
5271
5272 const forged = await post(
5273 `/alice/orchard/pulls/${shipped.number}/take-back`,
5274 ownerCookie,
5275 "https://evil.example",
5276 );
5277 expect(forged.status).toBe(403);
5278 expect(getPull(app.db, repo.id, shipped.number + 1)).toBeUndefined();
5279 });
5280
5281 test("a refusal is said in the studio's own words, and takes nothing back twice", async () => {
5282 const shipped = await shipChange("feature/epsilon", "Add epsilon", "alpha\nEPSILON\n");
5283 const first = await post(
5284 `/alice/orchard/pulls/${shipped.number}/take-back`,
5285 ownerCookie,
5286 );
5287 expect(first.status).toBe(303);
5288
5289 const again = await post(
5290 `/alice/orchard/pulls/${shipped.number}/take-back`,
5291 ownerCookie,
5292 );
5293 expect(again.status).toBe(303);
5294 const location = again.headers.get("location")!;
5295 expect(location).toBe(`/alice/orchard/pulls/${shipped.number}`);
5296 const flash = decodeURIComponent(again.headers.get("set-cookie") ?? "");
5297 expect(flash).toContain("already been taken back");
5298
5299 // Only one revert pull exists — the second attempt created nothing.
5300 expect(
5301 getPull(app.db, repo.id, shipped.number + 1)?.title,
5302 ).toBe('Revert "Add epsilon"');
5303 expect(getPull(app.db, repo.id, shipped.number + 2)).toBeUndefined();
5304 });
5305 });
5306
50885307 describe("the studio's crew panel on a forge with no agents yet", () => {
50895308 let dataDir: string;
50905309 let app: App;

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs