trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

A fix the owner or steward writes lands through the queue as is: an in-repo pull is gated and landed, not closed for being off a fork #7575

Merged torquecrew opened this in answer to trevatoowner

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

ConversationFiles
Merged

torque/forge:torque/7572-a-fix-the-owner-or-steward-writes-lands → main 2 commits fork

  • Merged. torque/7572-a-fix-the-owner-or-steward-writes-lands was merged into main as 8626b33.

Review

Sign in to review this change.
10 files changed +267 −6
docs/APPS.md +6
@@ -308,6 +308,12 @@ around it. None is advisory.
308308 create a repository (it owns nothing it was not given; it may fork to
309309 contribute). A reviewer on the crew may still pass a verdict — request
310310 changes, approve — since a verdict gates nothing an agent could do.
311 A person's pull from a branch of the repository itself — the owner's
312 or a steward's fix — is a normal pull: its pushes move its head and run
313 its check, review and gate again, and it lands through the queue. No
314 builder is sent to it, since none can push that branch; changes asked
315 for on it go back to its author, and the crew never parks it
316 (`crew/authors_pull.ts`, #7572).
311317 2. **A preview lives exactly as long as its pull is open.** Opened: deployed.
312318 Pushed: redeployed at the new head. Closed or merged: stopped, and its
313319 data deleted. There is no orphan state; the sweep on boot rebuilds the
src/crew/authors_pull.ts added +22
@@ -0,0 +1,22 @@
1 /**
2 * A person's pull from a branch of the repository itself (#7572): the
3 * owner's or a steward's fix, opened by hand or with an admin token. It is
4 * a normal pull — a push to its branch moves its head (`trackPullHead`) and
5 * runs its check, review and gate again, and green it lands through the
6 * queue — but no agent can push its branch: the crew works from forks.
7 *
8 * So the crew never sends a builder to it and never parks it. Changes asked
9 * for on it go back to its author (`requested.ts`), and closing it is only
10 * theirs: a closed pull no longer follows its branch, and every later push
11 * would be lost to it.
12 */
13
14 import type { Pull } from "../core/pulls.ts";
15
16 /** Whether the pull is a person's, from a branch of the repository it proposes to. */
17 export function authorsInRepoPull(pull: Pick<Pull, "authorIsAgent" | "crossRepo">): boolean {
18 return !pull.authorIsAgent && !pull.crossRepo;
19 }
20
21 /** What the changes-requested pass says of one, once it is back with its author. */
22 export const BACK_WITH_AUTHOR = "back with its author, a person: the branch lives in the repository";
src/crew/autonomy.ts +6 −1
@@ -13,6 +13,7 @@
1313 */
1414
1515 import { closedToday, closeStale } from "./stale_pulls.ts";
16 import { authorsInRepoPull } from "./authors_pull.ts";
1617 import type { Database } from "bun:sqlite";
1718 import type { App } from "../app.ts";
1819 import { ceilingReached, forgeCeiling } from "../flow/ceiling.ts";
@@ -1082,7 +1083,10 @@ function answeredWithoutChange(
10821083 * planner's to split or drop on its next pass, and one parked twice is
10831084 * handed to the planner with every reason (`strikeNote`). A pull a person
10841085 * holds is not parked: closing it would hand its issue back to the crew, to
1085 * build the same change again beside the one that was told to wait.
1086 * build the same change again beside the one that was told to wait. A
1087 * person's pull from a branch of the repository itself is not parked
1088 * either: closing it is its author's, and closed it would stop following
1089 * the pushes that answer its review (#7572).
10861090 */
10871091 export function parkPull(
10881092 app: App,
@@ -1092,6 +1096,7 @@ export function parkPull(
10921096 ): boolean {
10931097 if (repo.autonomy !== "autonomous" || repo.autonomyBy === null) return false;
10941098 if (getHold(app.db, pull.id) !== undefined) return false;
1099 if (authorsInRepoPull(pull)) return false;
10951100 const by = getUserById(app.db, repo.autonomyBy);
10961101 if (!by) return false;
10971102 const issue = getIssueById(app.db, pull.id);
src/crew/requested.ts +19
@@ -32,6 +32,7 @@ import { reviewSummary } from "../core/reviews.ts";
3232 import { activeRunFor } from "../core/runs.ts";
3333 import { getUserById } from "../core/users.ts";
3434 import { createLog } from "../kernel/log.ts";
35 import { authorsInRepoPull, BACK_WITH_AUTHOR } from "./authors_pull.ts";
3536 import { queueAgentRun } from "./dispatch.ts";
3637 import { flowOnlyHolds } from "./flow_only.ts";
3738 import { HANDOVER_NOTE, handoverOf } from "./guard.ts";
@@ -207,6 +208,8 @@ function followUp(
207208 // Past the rounds the verdict is advice, and the pull lands as it is.
208209 if (reviewSummary(app.db, pull.id, pull.headOid).changesRequested === 0)
209210 return notMoved(requested, "past the review rounds; the verdict is advice");
211 // No builder can push a branch of the repository itself (#7572).
212 if (authorsInRepoPull(pull)) return handBack(app, repo, requested, issue);
210213 if (activeRunFor(app.db, "agent", pull.id) !== undefined)
211214 return notMoved(requested, "a run is in flight");
212215 // A push ends the verdict's turn (the reviewer reads the new head), so every
@@ -247,6 +250,22 @@ function followUp(
247250 };
248251 }
249252
253 /**
254 * A person's in-repo pull with changes asked for is theirs to answer: it is
255 * assigned back to its author, once, and no builder is sent — its run could
256 * only fail, and the failure would park the pull.
257 */
258 function handBack(app: App, repo: Repo, requested: RequestedPull, issue: Issue): RequestedMove {
259 if (issue.assigneeId !== issue.authorId) {
260 const author = getUserById(app.db, issue.authorId);
261 const by = (repo.autonomyBy === null ? undefined : getUserById(app.db, repo.autonomyBy)) ?? author;
262 if (author === undefined || by === undefined) return notMoved(requested, "nobody to hand it back to");
263 const done = assignIssue(app, by, repo, issue, author);
264 if (!done.ok) return notMoved(requested, `could not hand it back to ${author.username}: ${done.error}`);
265 }
266 return notMoved(requested, BACK_WITH_AUTHOR);
267 }
268
250269 /** How many runs other than a review started on the pull after the verdict. */
251270 function startedSince(app: App, pullId: string, at: string): number {
252271 return app.db
src/crew/runner.ts +5 −1
@@ -33,6 +33,7 @@ import { Stopwatch } from "../apps/timing.ts";
3333 import { previewEnv } from "../apps/data.ts";
3434 import { appsRuntime } from "../apps/runtime.ts";
3535 import { ownApp, ownAppUrls } from "./app_hosts.ts";
36 import { authorsInRepoPull } from "./authors_pull.ts";
3637 import { crewRole, getAgent, type Agent } from "../core/agents.ts";
3738 import { getAppByRepo, type AppRecord } from "../core/apps.ts";
3839 import {
@@ -887,10 +888,13 @@ async function work(
887888 // brief, its clone is its own fork at a scratch branch, and it pushes
888889 // nothing. The pull's branch is not its to move, and it never tries.
889890 const reviewing = reason === "review";
891 // A person's such pull is theirs to push, and stays open (#7572).
890892 if (pull !== undefined && !reviewing && pull.headRepoId === repo.id)
891893 return {
892894 state: "failed",
893 summary: `pull #${pull.number}'s branch lives in the repository itself; the crew works from forks now — close it and assign the issue again`,
895 summary: authorsInRepoPull(pull)
896 ? `pull #${pull.number}'s branch lives in the repository itself and only its author, a person, can push it; the crew leaves it with them`
897 : `pull #${pull.number}'s branch lives in the repository itself; the crew works from forks now — close it and assign the issue again`,
894898 };
895899 // A pull whose branch lives in someone else's fork is not the agent's to
896900 // push to, and the run no longer fails for it: like a merged pull's
test/authors-pull.test.ts added +203
@@ -0,0 +1,203 @@
1 /**
2 * A fix the owner or a steward writes lands through the queue as it is
3 * (#7572): a person's pull from a branch of the repository itself is a
4 * normal pull. A push to its branch moves its head and starts its check;
5 * changes asked for on it go back to its author, never to a builder who
6 * cannot push that branch; and the crew never parks it, so it never stops
7 * following its branch.
8 */
9
10 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
11 import { mkdtemp, rm } from "node:fs/promises";
12 import { tmpdir } from "node:os";
13 import { join } from "node:path";
14 import { addToCrew, createAgent } from "../src/actions/crew.ts";
15 import { openPull } from "../src/actions/pulls.ts";
16 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
17 import { createApp, type App } from "../src/app.ts";
18 import { startChecks, type ChecksHandle } from "../src/checks/service.ts";
19 import { setCrewRole } from "../src/core/agents.ts";
20 import { getIssueById } from "../src/core/issues.ts";
21 import { getPull, type Pull } from "../src/core/pulls.ts";
22 import { getRepoById, type Repo } from "../src/core/repos.ts";
23 import { submitReview } from "../src/core/reviews.ts";
24 import { listRuns } from "../src/core/runs.ts";
25 import { createUser, getUserByName, type User } from "../src/core/users.ts";
26 import { authorsInRepoPull, BACK_WITH_AUTHOR } from "../src/crew/authors_pull.ts";
27 import { parkPull, settleAgentRun } from "../src/crew/autonomy.ts";
28 import { finishApproved } from "../src/crew/finishing.ts";
29 import { moveRequested } from "../src/crew/requested.ts";
30 import { serve, type Servers } from "../src/http/server.ts";
31 import { quietLogs } from "./helpers/globals.ts";
32
33 quietLogs();
34
35 describe("a person's in-repo pull is a normal pull the crew leaves to its author", () => {
36 let dataDir = "";
37 let app: App;
38 let servers: Servers;
39 let checks: ChecksHandle;
40 let alice: User;
41 let builder: User;
42 let reviewer: User;
43 let repo: Repo;
44 let n = 0;
45
46 const fresh = () => getRepoById(app.db, repo.id)!;
47 const reload = (pull: Pull) => getPull(app.db, repo.id, pull.number)!;
48 const push = async (by: User, branch: string, base?: string): Promise<string> => {
49 const c = await commitToBranch(app, by, repo, {
50 branch,
51 ...(base === undefined ? {} : { base }),
52 message: `${branch} ${++n}`,
53 files: [{ path: `${branch.replace("/", "-")}.md`, content: `${n}\n` }],
54 });
55 if (!c.ok) throw new Error(JSON.stringify(c.error));
56 return c.value.commitOid;
57 };
58 /** A pull `by` opens on a branch of the repository, pushed by alice: an agent writes only its fork. */
59 const pullBy = async (by: User, branch: string): Promise<Pull> => {
60 await push(alice, branch, "main");
61 const o = await openPull(app, by, repo, { title: branch, body: "", baseBranch: "main", headBranch: branch });
62 if (!o.ok) throw new Error(String(o.error));
63 return o.value;
64 };
65 const changesRequested = (pull: Pull) => {
66 const r = submitReview(app.db, {
67 issueId: pull.id,
68 authorId: reviewer.id,
69 state: "changes_requested",
70 body: "Fix the one thing.",
71 commitOid: pull.headOid,
72 });
73 if (!r.ok) throw new Error(r.error);
74 };
75 const agentRuns = (pull: Pull) =>
76 listRuns(app.db, repo.id, { issueId: pull.id }).filter((r) => r.kind === "agent" && r.reason !== "review");
77
78 beforeAll(async () => {
79 dataDir = await mkdtemp(join(tmpdir(), "forge-authors-pull-"));
80 app = createApp({ dataDir, port: 0 });
81 servers = await serve(app);
82 checks = startChecks(app, { forgeUrl: servers.url, concurrency: 1 });
83 const made = await createUser(app.db, {
84 origin: "seed",
85 trusted: true,
86 username: "alice",
87 email: "[email protected]",
88 password: "a-good-passphrase",
89 adminIfFirst: true,
90 });
91 if (!made.ok) throw new Error(made.error);
92 alice = made.value;
93 const r = await createRepo(app, alice, { name: "garden", visibility: "private" });
94 if (!r.ok) throw new Error(String(r.error));
95 repo = r.value;
96 const gate = await commitToBranch(app, alice, repo, {
97 message: "The check",
98 files: [{ path: "forge.json", content: `{"check": {"run": "true", "install": ""}}\n` }],
99 });
100 if (!gate.ok) throw new Error(JSON.stringify(gate.error));
101 for (const name of ["mason", "quill"]) {
102 const agent = await createAgent(app, alice, { username: name });
103 if (!agent.ok) throw new Error(String(agent.error));
104 expect(addToCrew(app, alice, repo, name).ok).toBe(true);
105 }
106 builder = getUserByName(app.db, "mason")!;
107 reviewer = getUserByName(app.db, "quill")!;
108 setCrewRole(app.db, repo.id, reviewer.id, "reviewer");
109 app.db
110 .query("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?2, autonomy_slots = 3 WHERE id = ?1")
111 .run(repo.id, alice.id);
112 }, 30_000);
113
114 afterAll(async () => {
115 await checks.stop();
116 servers.stop();
117 app.close();
118 await rm(dataDir, { recursive: true, force: true });
119 });
120
121 test("only a person's pull from a branch of the repository itself is one", () => {
122 expect(authorsInRepoPull({ authorIsAgent: false, crossRepo: false })).toBe(true);
123 expect(authorsInRepoPull({ authorIsAgent: true, crossRepo: false })).toBe(false);
124 expect(authorsInRepoPull({ authorIsAgent: false, crossRepo: true })).toBe(false);
125 });
126
127 test("a push to its branch moves the pull's head and starts a check of the new head", async () => {
128 const pull = await pullBy(alice, "steward/one");
129 const tip = await push(alice, pull.headBranch);
130 expect(reload(pull).headOid).toBe(tip);
131 const deadline = Date.now() + 10_000;
132 let checked = false;
133 while (!checked && Date.now() < deadline) {
134 checked = listRuns(app.db, repo.id, { kind: "check", limit: 100 }).some(
135 (r) => r.issueId === pull.id && r.commitOid === tip && r.env === `pull-${pull.number}`,
136 );
137 if (!checked) await Bun.sleep(50);
138 }
139 expect(checked).toBe(true);
140 });
141
142 test("changes asked for on it go back to its author: no builder is sent, even one already on it", async () => {
143 const pull = await pullBy(alice, "steward/two");
144 // A builder's name on it, as #7436 had.
145 app.db.query("UPDATE issue SET assignee_id = ?2 WHERE id = ?1").run(pull.id, builder.id);
146 changesRequested(pull);
147 const moves = await moveRequested(app, fresh(), 3);
148 expect(moves.find((m) => m.pull === pull.number)).toEqual({
149 pull: pull.number,
150 did: "not moved",
151 why: BACK_WITH_AUTHOR,
152 });
153 expect(getIssueById(app.db, pull.id)!.assigneeId).toBe(alice.id);
154 expect(agentRuns(pull)).toHaveLength(0);
155 // Said again on a later tick, and still nothing queued.
156 await moveRequested(app, fresh(), 3);
157 expect(agentRuns(pull)).toHaveLength(0);
158 });
159
160 test("a builder's run on it that pushed nothing does not park it, and its next push still moves its head", async () => {
161 const pull = await pullBy(alice, "steward/three");
162 changesRequested(pull);
163 const parked = settleAgentRun(app, fresh(), {
164 issueId: pull.id,
165 state: "failed",
166 summary: `pull #${pull.number}'s branch lives in the repository itself`,
167 reason: "feedback",
168 actorId: builder.id,
169 });
170 expect(parked).toBe(false);
171 expect(parkPull(app, fresh(), pull, "the check stayed red")).toBe(false);
172 expect(getIssueById(app.db, pull.id)!.state).toBe("open");
173 const tip = await push(alice, pull.headBranch);
174 expect(reload(pull).headOid).toBe(tip);
175 });
176
177 test("a builder's own in-repo pull is still parked as before", async () => {
178 const pull = await pullBy(builder, "b-legacy");
179 expect(parkPull(app, fresh(), pull, "the check stayed red")).toBe(true);
180 expect(getIssueById(app.db, pull.id)!.state).toBe("closed");
181 });
182
183 test("the finishing pass leaves it alone: not a candidate, and still open", async () => {
184 const pull = await pullBy(alice, "steward/four");
185 const r = submitReview(app.db, {
186 issueId: pull.id,
187 authorId: reviewer.id,
188 state: "approved",
189 body: "",
190 commitOid: pull.headOid,
191 });
192 if (!r.ok) throw new Error(r.error);
193 // Main moves under it on the same file: approved and conflicted.
194 await commitToBranch(app, alice, repo, {
195 message: "main moves",
196 files: [{ path: "steward-four.md", content: "main\n" }],
197 });
198 const pass = await finishApproved(app, fresh(), 3);
199 expect(pass.moves.find((m) => m.pull === pull.number)).toBeUndefined();
200 expect(agentRuns(pull)).toHaveLength(0);
201 expect(getIssueById(app.db, pull.id)!.state).toBe("open");
202 });
203 });
test/checks-disk-red.test.ts +1 −1
@@ -54,7 +54,7 @@ describe("a red on a full check machine", () => {
5454 files: [{ path: `${branch}.md`, content: "x\n" }],
5555 });
5656 if (!c.ok) throw new Error(JSON.stringify(c.error));
57 const o = await openPull(app, alice, repo, { title: branch, baseBranch: "main", headBranch: branch });
57 const o = await openPull(app, bot, repo, { title: branch, baseBranch: "main", headBranch: branch });
5858 if (!o.ok) throw new Error(String(o.error));
5959 app.db.run("UPDATE issue SET assignee_id = ?1 WHERE id = ?2", [bot.id, o.value.id]);
6060 for (let i = 0; i < CHECK_FAILED_LIMIT; i++) {
test/crew-failed-while-building.test.ts +1 −1
@@ -43,7 +43,7 @@ describe("a failure during the builder's run", () => {
4343 files: [{ path: `${branch}.md`, content: "x\n" }],
4444 });
4545 if (!c.ok) throw new Error(JSON.stringify(c.error));
46 const o = await openPull(app, alice, repo, { title: branch, baseBranch: "main", headBranch: branch, ...(body === undefined ? {} : { body }) });
46 const o = await openPull(app, bot, repo, { title: branch, baseBranch: "main", headBranch: branch, ...(body === undefined ? {} : { body }) });
4747 if (!o.ok) throw new Error(String(o.error));
4848 app.db.run("UPDATE issue SET assignee_id = ?1 WHERE id = ?2", [bot.id, o.value.id]);
4949 for (let i = 0; i < CHECK_FAILED_LIMIT; i++) {
test/crew-shared-red.test.ts +1 −1
@@ -52,7 +52,7 @@ describe("the crew, with a red many pulls share", () => {
5252 files: [{ path: `${branch}.md`, content: "x\n" }],
5353 });
5454 if (!c.ok) throw new Error(JSON.stringify(c.error));
55 const o = await openPull(app, alice, repo, { title: branch, baseBranch: "main", headBranch: branch });
55 const o = await openPull(app, bot, repo, { title: branch, baseBranch: "main", headBranch: branch });
5656 if (!o.ok) throw new Error(String(o.error));
5757 app.db.run("UPDATE issue SET assignee_id = ?1 WHERE id = ?2", [bot.id, o.value.id]);
5858 for (let i = 0; i < CHECK_FAILED_LIMIT; i++) {
test/holds.test.ts +3 −1
@@ -404,7 +404,9 @@ describe("a held pull and the rules", () => {
404404 f.app.db
405405 .query("UPDATE issue SET author_id = ?2 WHERE id = ?1")
406406 .run(pull.id, author.id);
407 return f.passCheck(pull, minutesAgo);
407 f.passCheck(pull, minutesAgo);
408 // Read again: the author decides whose pull it is (#7572).
409 return getPull(f.app.db, f.repo.id, pull.number)!;
408410 };
409411 const approve = (pull: Pull, by: User) =>
410412 submitReview(f.app.db, {

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs