trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlows

A merge conflict is not a failed try: the pull is brought up to date and its tries are kept #8022

Merged masoncrew opened this in answer to trevatoowner

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

ConversationFiles
Merged

mason/forge:mason/7909-a-merge-conflict-is-not-a-failed-try-the → main 5 commits fork

  • Merged. mason/7909-a-merge-conflict-is-not-a-failed-try-the was merged into main as 8526368.

Review

Sign in to review this change.
7 files changed +367 −5
src/actions/list-conflicts.ts added +77
@@ -0,0 +1,77 @@
1 /**
2 * A pull that conflicts only in list-like docs (`LIST_DOCS`) is brought up to
3 * date by the forge: the base merged into its head with both sides' lines
4 * kept, one commit on the head branch. No clone, no run, no model — the
5 * conflicts every landing on docs/MCP.md makes are not worth one.
6 */
7
8 import type { App } from "../app.ts";
9 import { forgeHost, noreplyAddress } from "../core/commit_identity.ts";
10 import { trackPullHead, type Pull } from "../core/pulls.ts";
11 import type { Repo } from "../core/repos.ts";
12 import { conflictedTree } from "../git/conflict-file.ts";
13 import { runGit } from "../git/env.ts";
14 import { mergeable, mergeBase, resolveOid } from "../git/merge.ts";
15 import { repoDir } from "../git/repos.ts";
16 import { isListDoc, unionMergeFile } from "../git/union-merge.ts";
17 import { commitFiles } from "../git/write.ts";
18
19 /** Whether every conflicted path is one a list-merge settles. */
20 export const onlyListDocs = (files: readonly string[]): boolean =>
21 files.length > 0 && files.every(isListDoc);
22
23 /**
24 * Merge the base into the pull's head, keeping both sides of every list-doc
25 * clash, when those are the only files that conflict. The commit id when the
26 * head moved; undefined when this is not a pull the forge can settle, or the
27 * branch moved meanwhile (the next look decides).
28 */
29 export async function resolveListConflicts(
30 app: App,
31 repo: Repo,
32 pull: Pull,
33 ): Promise<string | undefined> {
34 if (pull.state !== "open" || pull.mergedAt !== null) return undefined;
35 if (pull.crossRepo || pull.originHeadRepo !== null || repo.origin !== null) return undefined;
36 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
37 const head = await resolveOid(dir, `refs/heads/${pull.headBranch}`);
38 const tip = await resolveOid(dir, `refs/heads/${pull.baseBranch}`);
39 if (head === undefined || tip === undefined) return undefined;
40 // Asked of git now, not of the red that sent us: the base may have moved.
41 const now = await mergeable(dir, tip, head);
42 if (now.ok || now.error.kind !== "conflict" || !onlyListDocs(now.error.files)) return undefined;
43 const files = now.error.files;
44 const base = await mergeBase(dir, tip, head);
45 const tree = await conflictedTree(dir, tip, head);
46 if (base === undefined || tree === undefined) return undefined;
47 const written: { path: string; content: string; mode: "100644" | "100755" }[] = [];
48 for (const path of files) {
49 const content = await unionMergeFile(dir, base, head, tip, path);
50 const entry = await runGit(["ls-tree", tree, "--", path], { cwd: dir });
51 const mode = entry.ok ? entry.value.split(" ")[0] : undefined;
52 if (content === undefined || (mode !== "100644" && mode !== "100755")) return undefined;
53 written.push({ path, content, mode });
54 }
55 const committed = await commitFiles(dir, {
56 branch: pull.headBranch,
57 message: `Merge ${pull.baseBranch} into ${pull.headBranch}\n\nKept both sides in ${files.join(", ")}.`,
58 files: written,
59 seedTree: tree,
60 mergeParents: [tip],
61 expectTip: head,
62 author: { name: "forge", email: noreplyAddress("forge", forgeHost(app.config)) },
63 });
64 if (!committed.ok) return undefined;
65 const commitOid = committed.value.commitOid;
66 trackPullHead(app.db, repo.id, pull.headBranch, commitOid);
67 // `update-ref` runs no hooks: announce the push as a git push would be.
68 app.bus.append({
69 kind: "push",
70 repoId: repo.id,
71 payload: {
72 updates: [{ ref: `refs/heads/${pull.headBranch}`, newOid: commitOid }],
73 viaApi: true,
74 },
75 });
76 return commitOid;
77 }
src/core/runs.ts +6 −4
@@ -1004,8 +1004,10 @@ export function latestRunsForIssues(
10041004 }
10051005
10061006 /**
1007 * How many automatic follow-ups an agent has had on a pull since a person
1008 * last spoke: the bound behind rule 7 in docs/APPS.md. `since` is the
1007 * How many automatic follow-ups for a failed check an agent has had on a
1008 * pull since a person last spoke: the bound behind rule 7 in docs/APPS.md.
1009 * A bring-up-to-date run (`conflict`) is no try: main moving is not the
1010 * change's failing, and those runs are bounded where they are queued. `since` is the
10091011 * moment of the last human comment or review, or the pull's creation.
10101012 */
10111013 export function automaticRunsSince(
@@ -1017,7 +1019,7 @@ export function automaticRunsSince(
10171019 .query<{ n: number }, [string, string]>(
10181020 `SELECT COUNT(*) AS n FROM run
10191021 WHERE kind = 'agent' AND issue_id = ?1 AND created_at > ?2
1020 AND reason IN ('check_failed','conflict')`,
1022 AND reason = 'check_failed'`,
10211023 )
10221024 .get(issueId, since);
10231025 return row?.n ?? 0;
@@ -1041,7 +1043,7 @@ export function automaticRunsSinceFor(
10411043 `WITH since (issue_id, at) AS (VALUES ${batch.map((_, n) => `(?${2 * n + 1}, ?${2 * n + 2})`).join(",")})
10421044 SELECT run.issue_id, COUNT(*) AS n FROM run JOIN since ON since.issue_id = run.issue_id
10431045 WHERE run.kind = 'agent' AND run.created_at > since.at
1044 AND run.reason IN ('check_failed','conflict')
1046 AND run.reason = 'check_failed'
10451047 GROUP BY run.issue_id`,
10461048 )
10471049 .all(...batch.flat());
src/crew/autonomy.ts +9
@@ -12,6 +12,7 @@
1212 * repository's origin.
1313 */
1414
15 import { onlyListDocs, resolveListConflicts } from "../actions/list-conflicts.ts";
1516 import { closedToday, closeStale } from "./stale_pulls.ts";
1617 import { authorsInRepoPull } from "./authors_pull.ts";
1718 import type { Database } from "bun:sqlite";
@@ -1821,6 +1822,14 @@ export async function sweepConflicts(
18211822 // push must not be queued again on every tick.
18221823 const tried = asked.get(pull.id);
18231824 if (tried?.tip === baseTip && tried.n >= RUNS_PER_TIP) continue;
1825 // A clash only in list docs is settled by the forge, with no run: its
1826 // push is a check, and the next tick sees the pull clean (#7909).
1827 if (onlyListDocs(files) && room > 0 && (await resolveListConflicts(app, repo, pull)) !== undefined) {
1828 room--;
1829 acted++;
1830 checked.delete(pull.id);
1831 continue;
1832 }
18241833 // Its push is a check: none starts that the lane has no slot for (#4459).
18251834 if (room <= 0) continue;
18261835 if (bringUpToDate(app, repo, pull, files, since) !== undefined) {
src/crew/dispatch.ts +37
@@ -10,6 +10,7 @@
1010
1111 import { twinNote, twinOf, type Twin } from "./twin.ts";
1212 import { NO_MERGE } from "../checks/merged.ts";
13 import { resolveListConflicts } from "../actions/list-conflicts.ts";
1314 import { markHeard } from "../core/issues.ts";
1415 import { capacityCount, noteBooted } from "../core/capacity.ts";
1516 import { tmpdir } from "node:os";
@@ -115,6 +116,30 @@ const log = createLog("crew");
115116 export const CHECK_FAILED_LIMIT = 2;
116117 const TICK_MS = 5_000;
117118
119 /** Whether the check run's red is a pull that does not merge into its base. */
120 function conflictSummary(app: App, runId: unknown): boolean {
121 if (typeof runId !== "string") return false;
122 return getRun(app.db, runId)?.summary.includes(NO_MERGE) === true;
123 }
124
125 /**
126 * A pull's check could not start for a conflict: the forge keeps both sides
127 * of a list-doc clash itself, else queues one bring-up-to-date run. Neither
128 * is a try (`automaticRunsSince` counts failed checks only); one in flight is
129 * enough.
130 */
131 async function bringUpAfterNoMerge(app: App, repo: Repo, issue: Issue, agent: Agent): Promise<void> {
132 const pull = getPull(app.db, repo.id, issue.number);
133 if (!pull) return;
134 if (await resolveListConflicts(app, repo, pull)) return;
135 if (activeRunFor(app.db, "agent", issue.id) !== undefined) return;
136 queueAgentRun(app, repo, issue, agent, "conflict", undefined, {
137 notBefore: new Date().toISOString(),
138 attempt: 0,
139 summary: "main moved: the pull does not merge into its base",
140 });
141 }
142
118143 export interface CrewHandle {
119144 /** Whether a usable Claude Code was found at start. */
120145 readonly ready: boolean;
@@ -732,6 +757,18 @@ function route(app: App, event: ForgeEvent): void {
732757 });
733758 return;
734759 }
760 // A pull that does not merge is no failed try: the forge settles a
761 // list-doc clash itself, else a bring-up-to-date run is queued. It
762 // counts towards no limit and parks nothing (#7909).
763 if (conflictSummary(app, event.payload["runId"])) {
764 void bringUpAfterNoMerge(app, repo, issue, agent).catch((error) =>
765 log.error("bringing a conflicted pull up to date failed", {
766 issue: issue.number,
767 error: String(error),
768 }),
769 );
770 return;
771 }
735772 // A red that main shares (#1971), or that three pulls share within
736773 // the hour (#2266), is not the pull's: no rework, no park, no
737774 // strike; it waits and is checked again when main is green.
src/git/union-merge.ts added +67
@@ -0,0 +1,67 @@
1 /**
2 * Three versions of one text file merged with `git merge-file --union`: a
3 * clash keeps both sides' lines instead of leaving markers. For a file whose
4 * content is a list, where two pulls each add an entry at the same place.
5 */
6
7 import { mkdtemp, rm, writeFile } from "node:fs/promises";
8 import { tmpdir } from "node:os";
9 import { join } from "node:path";
10 import { runGitCapture } from "./env.ts";
11 import { isSafeOid, isSafeTreePath } from "./guard.ts";
12
13 const MAX_BYTES = 1_000_000;
14
15 /**
16 * The files a pull's conflict is settled in without a run. Only files that
17 * are line-lists, where each pull adds its own entry: `--union` keeps both
18 * sides' lines unreviewed, which is right for two table rows or bullets and
19 * wrong for prose, where it would duplicate or interleave paragraphs.
20 * docs/MCP.md is its tool table and bullets; README.md and CHANGELOG.md hold
21 * sections of prose and stay with a run.
22 */
23 export const LIST_DOCS: readonly string[] = ["docs/MCP.md"];
24
25 export const isListDoc = (path: string): boolean => LIST_DOCS.includes(path);
26
27 async function blobAt(dir: string, oid: string, path: string): Promise<string | undefined> {
28 const out = await runGitCapture(["cat-file", "blob", `${oid}:${path}`], { cwd: dir });
29 return out.code === 0 && out.stdout.length <= MAX_BYTES ? out.stdout : undefined;
30 }
31
32 /**
33 * `path` at `ours` and `theirs`, merged over what they share at `base`, both
34 * sides kept. Undefined when the file is missing on any side or too large.
35 */
36 export async function unionMergeFile(
37 dir: string,
38 base: string,
39 ours: string,
40 theirs: string,
41 path: string,
42 ): Promise<string | undefined> {
43 if (!isSafeOid(base) || !isSafeOid(ours) || !isSafeOid(theirs) || !isSafeTreePath(path))
44 return undefined;
45 const [b, o, t] = await Promise.all([
46 blobAt(dir, base, path),
47 blobAt(dir, ours, path),
48 blobAt(dir, theirs, path),
49 ]);
50 if (b === undefined || o === undefined || t === undefined) return undefined;
51 const work = await mkdtemp(join(tmpdir(), "forge-union-"));
52 try {
53 await Promise.all([
54 writeFile(join(work, "ours"), o),
55 writeFile(join(work, "base"), b),
56 writeFile(join(work, "theirs"), t),
57 ]);
58 const merged = await runGitCapture(
59 ["merge-file", "-p", "--union", join(work, "ours"), join(work, "base"), join(work, "theirs")],
60 { cwd: dir },
61 );
62 // Exit 0 is clean; a positive count is the clashes kept as both sides.
63 return merged.code >= 0 && merged.code < 128 ? merged.stdout : undefined;
64 } finally {
65 await rm(work, { recursive: true, force: true });
66 }
67 }
test/asks-counts.test.ts +1 −1
@@ -254,7 +254,7 @@ describe("home's asks are read grouped, and once", () => {
254254 const handovers = sql.filter((s) => s.startsWith("SELECT r.issue_id AS issueId, COUNT(*)"));
255255 const rounds = sql.filter((s) => s.startsWith("SELECT r.issue_id AS issueId, r.id AS id"));
256256 const verdicts = sql.filter((s) => s.startsWith("SELECT review.issue_id, review.id"));
257 const tries = sql.filter((s) => s.includes("reason IN ('check_failed','conflict')"));
257 const tries = sql.filter((s) => s.includes("run.reason = 'check_failed'"));
258258 const reviewers = sql.filter((s) => s.includes("WHERE repo_crew.role = ?1"));
259259 expect(active.length).toBe(1);
260260 expect(handovers.length).toBe(1);
test/crew-merge-conflict.test.ts added +170
@@ -0,0 +1,170 @@
1 /**
2 * A merge conflict is not a failed try (#7909): a check that could not start
3 * because the pull "does not merge into main" never counts towards
4 * CHECK_FAILED_LIMIT and never parks it. A clash only in a list doc is
5 * settled by the forge keeping both sides; any other queues one
6 * bring-up-to-date run.
7 */
8
9 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
10 import type { App } from "../src/app.ts";
11 import { announceRun } from "../src/actions/runs.ts";
12 import { commitToBranch } from "../src/actions/repos.ts";
13 import { openPull } from "../src/actions/pulls.ts";
14 import { NO_MERGE } from "../src/checks/merged.ts";
15 import { CHECK_FAILED_LIMIT } from "../src/crew/dispatch.ts";
16 import { getIssueById } from "../src/core/issues.ts";
17 import { getPull, type Pull } from "../src/core/pulls.ts";
18 import type { Repo } from "../src/core/repos.ts";
19 import { automaticRunsSince, claimRun, createRun, finishRun, listRuns } from "../src/core/runs.ts";
20 import type { User } from "../src/core/users.ts";
21 import { onlyListDocs } from "../src/actions/list-conflicts.ts";
22 import { sweepConflicts } from "../src/crew/autonomy.ts";
23 import { getRepoById } from "../src/core/repos.ts";
24 import { runGitCapture } from "../src/git/env.ts";
25 import { unionMergeFile } from "../src/git/union-merge.ts";
26 import { repoDir } from "../src/git/repos.ts";
27 import { startStandInCrew, stopStandInCrew, type StandInCrew } from "./helpers/crew-standin.ts";
28
29 describe("the crew, with a pull that does not merge", () => {
30 let standIn: StandInCrew;
31 let app: App;
32 let alice: User;
33 let bot: User;
34 let repo: Repo;
35 let n = 0;
36
37 beforeAll(async () => {
38 standIn = await startStandInCrew();
39 ({ app, alice, bot, repo } = standIn);
40 standIn.checks.request = async () => undefined;
41 app.db.run("UPDATE repo SET autonomy = 'autonomous', autonomy_by = ?1 WHERE id = ?2", [alice.id, repo.id]);
42 }, 30_000);
43 afterAll(() => stopStandInCrew(standIn));
44
45 const commit = async (branch: string, path: string, content: string) => {
46 const c = await commitToBranch(app, alice, repo, {
47 message: `${branch} ${path}`,
48 branch,
49 ...(branch === "main" ? {} : { base: "main" }),
50 files: [{ path, content }],
51 });
52 if (!c.ok) throw new Error(JSON.stringify(c.error));
53 };
54
55 /** A pull whose automatic tries are spent, conflicting with main in `path`. */
56 const conflicted = async (path: string): Promise<Pull> => {
57 const branch = `clash-${++n}`;
58 await commit("main", path, "- base\n");
59 await commit(branch, path, "- base\n- theirs\n");
60 const o = await openPull(app, bot, repo, { title: branch, baseBranch: "main", headBranch: branch });
61 if (!o.ok) throw new Error(String(o.error));
62 app.db.run("UPDATE issue SET assignee_id = ?1 WHERE id = ?2", [bot.id, o.value.id]);
63 for (let i = 0; i < CHECK_FAILED_LIMIT; i++) {
64 const tried = createRun(app.db, { repoId: repo.id, kind: "agent", issueId: o.value.id, reason: "check_failed" });
65 claimRun(app.db, tried.id);
66 finishRun(app.db, tried.id, { state: "ok", summary: "reworked" });
67 }
68 await commit("main", path, "- base\n- ours\n");
69 return getPull(app.db, repo.id, o.value.number)!;
70 };
71 const redForConflict = (pull: Pull) => {
72 const run = createRun(app.db, {
73 repoId: repo.id,
74 kind: "check",
75 env: `pull-${pull.number}`,
76 commitOid: pull.headOid,
77 reason: "push",
78 issueId: pull.id,
79 });
80 claimRun(app.db, run.id);
81 announceRun(
82 app,
83 finishRun(app.db, run.id, { state: "failed", summary: `${NO_MERGE} main: docs/x.ts` })!,
84 );
85 };
86 const conflictRuns = (pull: Pull) =>
87 listRuns(app.db, repo.id, { kind: "agent" }).filter((r) => r.issueId === pull.id && r.reason === "conflict");
88 /** Poll `done` until true, at most `ms`; the test's own expects then say what failed. */
89 const until = async (done: () => boolean, ms = 10_000) => {
90 const end = Date.now() + ms;
91 while (!done() && Date.now() < end) await new Promise((r) => setTimeout(r, 10));
92 };
93 const headOf = (pull: Pull) => getPull(app.db, repo.id, pull.number)!.headOid;
94 const fileAt = async (oid: string, path: string) =>
95 (await runGitCapture(["cat-file", "blob", `${oid}:${path}`], { cwd: repoDir(app.reposRoot, alice.username, repo.name) })).stdout;
96
97 test("a clash outside the list docs queues a bring-up-to-date run, is no try and parks nothing", async () => {
98 const pull = await conflicted("src/shared.ts");
99 // Two reds: one in flight is enough.
100 redForConflict(pull);
101 redForConflict(pull);
102 await until(() => conflictRuns(pull).length > 0);
103 // The list-doc pull below is handled after both: its head moving means the reds were seen.
104 const listed = await conflicted("docs/MCP.md");
105 redForConflict(listed);
106 await until(() => headOf(listed) !== listed.headOid);
107 expect(getIssueById(app.db, pull.id)?.state).toBe("open");
108 expect(conflictRuns(pull)).toHaveLength(1);
109 expect(automaticRunsSince(app.db, pull.id, "1970-01-01T00:00:00.000Z")).toBe(CHECK_FAILED_LIMIT);
110 });
111
112 test("a clash only in docs/MCP.md keeps both sides with no run", async () => {
113 const pull = await conflicted("docs/MCP.md");
114 redForConflict(pull);
115 await until(() => headOf(pull) !== pull.headOid);
116 expect(headOf(pull)).not.toBe(pull.headOid);
117 const doc = await fileAt(headOf(pull), "docs/MCP.md");
118 expect(doc).toContain("- ours");
119 expect(doc).toContain("- theirs");
120 expect(doc).not.toContain("<<<<<<<");
121 expect(conflictRuns(pull)).toHaveLength(0);
122 expect(getIssueById(app.db, pull.id)?.state).toBe("open");
123 });
124
125 test("the sweep settles a list-doc clash itself, with no run", async () => {
126 const pull = await conflicted("docs/MCP.md");
127 app.db.run("UPDATE repo SET autonomy = 'autonomous' WHERE id = ?1", [repo.id]);
128 // The sweep starts work only when the lane has room: clear the runs earlier tests queued.
129 for (const r of listRuns(app.db, repo.id, { kind: "agent" }))
130 if (r.state === "queued" || r.state === "running") {
131 claimRun(app.db, r.id);
132 finishRun(app.db, r.id, { state: "ok", summary: "cleared" });
133 }
134 // And the earlier tests' pulls: each run the sweep starts takes a slot of the lane's one.
135 app.db.run("UPDATE issue SET state = 'closed' WHERE repo_id = ?1 AND id != ?2 AND state = 'open'", [repo.id, pull.id]);
136 await sweepConflicts(app, getRepoById(app.db, repo.id)!);
137 expect(headOf(pull)).not.toBe(pull.headOid);
138 const doc = await fileAt(headOf(pull), "docs/MCP.md");
139 expect(doc).toContain("- ours");
140 expect(doc).toContain("- theirs");
141 expect(conflictRuns(pull)).toHaveLength(0);
142 });
143
144 test("prose docs are left to a run", async () => {
145 expect(onlyListDocs(["README.md"])).toBe(false);
146 expect(onlyListDocs(["CHANGELOG.md"])).toBe(false);
147 });
148
149 test("a real union merge keeps both sides' lines", async () => {
150 const dir = repoDir(app.reposRoot, alice.username, repo.name);
151 await commit("main", "docs/MCP.md", "- base\n");
152 const base = (await runGitCapture(["rev-parse", "main"], { cwd: dir })).stdout.trim();
153 await commit("main", "docs/MCP.md", "- base\n- ours\n");
154 const ours = (await runGitCapture(["rev-parse", "main"], { cwd: dir })).stdout.trim();
155 await commit(`union-${++n}`, "docs/MCP.md", "- base\n- theirs\n");
156 const theirs = (await runGitCapture(["rev-parse", `union-${n}`], { cwd: dir })).stdout.trim();
157 const merged = await unionMergeFile(dir, base, ours, theirs, "docs/MCP.md");
158 expect(merged).toContain("- base");
159 expect(merged).toContain("- ours");
160 expect(merged).toContain("- theirs");
161 expect(merged).not.toContain("<<<<<<<");
162 });
163
164 test("union merge: only list docs qualify", async () => {
165 expect(onlyListDocs(["docs/MCP.md"])).toBe(true);
166 expect(onlyListDocs(["docs/MCP.md", "src/a.ts"])).toBe(false);
167 expect(onlyListDocs([])).toBe(false);
168 expect(await unionMergeFile(repoDir(app.reposRoot, alice.username, repo.name), "x", "y", "z", "docs/MCP.md")).toBeUndefined();
169 });
170 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs