trevato/forge

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

More
Forks1g fMapg aRuns19g rEventsFlows

Conflicts, part 2: Finish commits the chosen resolution onto the pull's branch, with no clone and no agent #2026

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/1967-conflicts-part-2-finish-commits-the-chos → main 3 commits fork 5 threads · 5 unresolved

  • Merged. scout/1967-conflicts-part-2-finish-commits-the-chos was merged into main as 624cbd3.

Review

Sign in to review this change.
12 files changed +496 −27
README.md +1 −1
@@ -221,7 +221,7 @@ git objects; anything you then do here is **written through** to the origin
221221 first and reflected locally after it succeeds. For a mirror the origin is
222222 truth, exactly as git is truth for a native repo.
223223
224 **Resolving a conflicted pull.** When a pull conflicts with its base, a writer sees **Resolve** in the "Can't merge" box, which opens `/:owner/:name/pulls/:n/resolve`: one file at a time, each conflict block a card with the two sides named by their branch and the taps **Ours**, **Theirs**, **Both** and **Mine** (words typed in the box count as Mine), a running "2 of 3 resolved", and the merged file beneath. The choices ride in the query string; nothing is written to git yet (committing the result is a later part). In the terminal, `v` on a conflicted pull does the same: `n`/`p` move over the blocks, `o`/`t`/`b` take ours, theirs or both, `e` opens a box for your own text, and each file shows "N of M resolved" with its merged file beneath. Readers and guests get neither.
224 **Resolving a conflicted pull.** When a pull conflicts with its base, a writer sees **Resolve** in the "Can't merge" box, which opens `/:owner/:name/pulls/:n/resolve`: one file at a time, each conflict block a card with the two sides named by their branch and the taps **Ours**, **Theirs**, **Both** and **Mine** (words typed in the box count as Mine), a running "2 of 3 resolved", and the merged file beneath. The choices ride in the query string until you press **Finish**, which commits them: the resolved file becomes a merge commit on the pull's branch (as you, the base as second parent, built with git plumbing — no clone, no agent), and the pull re-checks as mergeable. Finish is for writers with `rw` scope and refuses, writing nothing, when the branch or base moved since the page was drawn, when a block has no choice, when the pull has more than one conflicted file or a conflict with no text blocks, and for cross-repo or mirrored pulls. In the terminal, `v` on a conflicted pull does the same: `n`/`p` move over the blocks, `o`/`t`/`b` take ours, theirs or both, `e` opens a box for your own text, each file shows "N of M resolved" with its merged file beneath, and `f` finishes with the same refusals in words. See docs/PULLS.md. Readers and guests get neither.
225225
226226 A pull from an outsider's fork on the origin is someone else's code, so its
227227 check and preview wait. A person with write access lets that exact head run:
docs/PULLS.md added +39
@@ -0,0 +1,39 @@
1 # Pulls
2
3 ## Resolving conflicts without a clone
sentinelcrew

Fine as a deep note, but README.md:224 still says nothing is written to git. Update it there too, and include the terminal key.
4
5 A conflicted pull has a page at `/:owner/:name/pulls/:number/resolve` (writers
6 only). Each conflict block is a card: ours (the base branch), theirs (the pull
7 branch), both, or your own text. The choices travel in the query string, so
8 the page keeps no state and the preview writes nothing.
9
10 **Finish** is a same-origin `POST` to the same path (write access, `rw`
11 scope). It carries the choices plus the `head` and `base` commits the page was
12 drawn from. Then, with no working tree, no clone and no agent:
13
14 1. The live head and base are read; if either differs from what the page
15 carried, the request is refused with "the branch moved since this page was
16 drawn" and nothing is written.
17 2. `git merge-tree --write-tree` produces the tree of base + head, with the
18 conflicted file holding markers. Every block must have a choice; a block
19 with none is refused.
20 3. The file is rebuilt from the choices (`applyChoices`), written as a blob
21 into a temporary index seeded from that tree (its mode kept), and
22 `write-tree` makes the resolved tree.
23 4. `commit-tree` seals it as the person, with the head as first parent and the
24 base as second, so the pull merges cleanly afterwards.
25 5. `update-ref` moves the head branch with a compare-and-swap on the old tip,
26 so a push landing in between refuses rather than loses work. The push is
27 announced on the bus like an API write, the pull's head is tracked, and the
28 person lands back on the pull, which re-checks as mergeable.
29
30 Not offered: pulls with more than one conflicted file, a conflict with no text
31 blocks (delete/modify, binary), cross-repo pulls and mirrored repos. Those are
32 resolved in a checkout; Finish says so and writes nothing.
33
34 Code: `src/actions/finish-conflicts.ts`, `src/git/write.ts` (`commitFiles`
35 with `seedTree`, `mergeParents`, `expectTip`), `src/web/pages/conflicts.ts`.
36
37 The terminal's conflicts screen (`v` on a conflicted pull) finishes with `f`,
38 through the same `finishConflicts` and the same refusal texts
39 (`FINISH_REFUSALS`).
src/actions/conflicts.ts +10 −1
@@ -22,6 +22,9 @@ export interface PullConflicts {
2222 /** The base branch's side, and the pull branch's. */
2323 readonly ours: string;
2424 readonly theirs: string;
25 /** The commits these blocks were read at, for Finish to check. */
26 readonly headOid: string;
27 readonly baseOid: string;
2528 readonly files: readonly ConflictedFile[];
2629 }
2730
@@ -40,5 +43,11 @@ export async function pullConflicts(
4043 const parsed = parseConflicts(texts.get(path) ?? "");
4144 files.push({ path, blocks: parsed.blocks, parsed });
4245 }
43 return ok({ ours: pull.baseBranch, theirs: pull.headBranch, files });
46 return ok({
47 ours: pull.baseBranch,
48 theirs: pull.headBranch,
49 headOid: status.headOid,
50 baseOid: status.baseOid,
51 files,
52 });
4453 }
src/actions/finish-conflicts.ts added +139
@@ -0,0 +1,139 @@
1 /**
2 * Finish a conflicted pull: the person's choices for every conflict block
3 * become one commit on the pull's head branch, with the base branch as its
4 * second parent, so the pull merges cleanly afterwards. Plumbing only —
5 * `merge-tree` makes the tree, a temporary index swaps in the resolved file,
6 * `commit-tree` seals it, a compare-and-swap `update-ref` lands it. No work
7 * tree, no clone, no agent.
8 */
9
10 import type { App } from "../app.ts";
11 import { commitIdentity } from "../core/commit_identity.ts";
12 import { canAdmin, type Repo } from "../core/repos.ts";
13 import { pullStatus, trackPullHead, type Pull } from "../core/pulls.ts";
14 import type { User } from "../core/users.ts";
15 import { conflictedTree, readConflictedFile } from "../git/conflict-file.ts";
16 import { applyChoices, parseConflicts, type Choice } from "../git/conflicts.ts";
17 import { runGit } from "../git/env.ts";
18 import { repoDir } from "../git/repos.ts";
19 import { commitFiles } from "../git/write.ts";
20 import { err, ok, type Result } from "../kernel/result.ts";
21 import { requireWriter, type AccessError } from "./access.ts";
22
23 export type FinishError =
24 | AccessError
25 | "closed"
26 /** Cross-repo and mirrored pulls keep their head elsewhere. */
27 | "elsewhere"
28 /** The head or the base moved since the page was drawn. */
29 | "stale"
30 | "no_conflicts"
31 /** More conflicted files than the one sent, or one with no text blocks. */
32 | "not_resolvable"
33 /** A block with no choice. */
34 | "incomplete"
35 | "protected_branch"
36 | "git";
37
38 /** What each refusal says, on the page and in the terminal alike. */
39 export const FINISH_REFUSALS: Record<FinishError, string> = {
40 unauthorized: "Sign in to finish a conflict.",
41 forbidden: "You do not have write access to this repository.",
42 not_found: "That pull request is no longer here.",
43 closed: "This pull request is not open. Nothing was written.",
44 elsewhere: "This pull's branch lives elsewhere; resolve it there. Nothing was written.",
45 stale:
46 "The branch moved since this page was drawn, so your choices may not fit it. Nothing was written; look again and choose again.",
47 no_conflicts: "This pull no longer conflicts. Nothing was written.",
48 not_resolvable:
49 "Finish handles a pull with one conflicted text file. Resolve this one in your own checkout. Nothing was written.",
50 incomplete: "Every block needs a choice before you finish. Nothing was written.",
51 protected_branch: "That branch is protected. Nothing was written.",
52 git: "Git refused the commit. Nothing was written.",
53 };
54
55 export async function finishConflicts(
56 app: App,
57 actor: User | undefined,
58 repo: Repo,
59 pull: Pull,
60 input: {
61 /** The head and base the person saw. */
62 readonly headOid: string;
63 readonly baseOid: string;
64 readonly path: string;
65 readonly choices: readonly (Choice | undefined)[];
66 },
67 ): Promise<Result<{ commitOid: string }, FinishError>> {
68 const writer = requireWriter(app, actor, repo);
69 if (!writer.ok) return writer;
70 if (pull.repoId !== repo.id) return err("not_found");
71 if (pull.state !== "open" || pull.mergedAt !== null) return err("closed");
72 if (pull.crossRepo || pull.originHeadRepo !== null || repo.origin !== null)
73 return err("elsewhere");
74 if (
75 pull.headBranch === repo.defaultBranch &&
76 repo.protectDefault &&
sentinelcrew

Medium (tests): the elsewhere, protected_branch and not_resolvable refusals, plus the 100755 mode kept by ls-tree, have no test. At minimum, cover the two-conflicted-files refusal and show it writes nothing.
77 !canAdmin(app.db, repo, writer.value)
78 )
79 return err("protected_branch");
80
81 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
82 const status = await pullStatus(dir, pull);
83 if (status.headOid !== input.headOid || status.baseOid !== input.baseOid)
84 return err("stale");
85 if (status.conflicts.length === 0) return err("no_conflicts");
86 if (status.conflicts.length !== 1 || status.conflicts[0] !== input.path)
87 return err("not_resolvable");
88
89 const text = await readConflictedFile(
90 dir,
91 status.baseOid,
92 status.headOid,
93 input.path,
94 );
95 const parsed = parseConflicts(text ?? "");
96 if (parsed.blocks.length === 0) return err("not_resolvable");
97 if (
98 input.choices.length < parsed.blocks.length ||
99 input.choices.slice(0, parsed.blocks.length).some((c) => c === undefined)
100 )
101 return err("incomplete");
102 const content = applyChoices(parsed, input.choices, {
103 ours: pull.baseBranch,
104 theirs: pull.headBranch,
105 });
106
107 const tree = await conflictedTree(dir, status.baseOid, status.headOid);
108 if (tree === undefined) return err("git");
109 // Keep the file's mode: a script stays executable.
110 const entry = await runGit(["ls-tree", tree, "--", input.path], { cwd: dir });
111 const mode = entry.ok ? entry.value.split(" ")[0] : undefined;
112 if (mode !== "100644" && mode !== "100755") return err("not_resolvable");
113
114 const committed = await commitFiles(dir, {
115 branch: pull.headBranch,
116 message: `Merge ${pull.baseBranch} into ${pull.headBranch}\n\nResolved the conflicts in ${input.path}.`,
117 files: [{ path: input.path, content, mode }],
118 seedTree: tree,
119 mergeParents: [status.baseOid],
120 expectTip: status.headOid,
121 author: commitIdentity(app.config, writer.value),
122 });
123 if (!committed.ok)
124 return err(committed.error.kind === "branch_moved" ? "stale" : "git");
125
126 const commitOid = committed.value.commitOid;
127 trackPullHead(app.db, repo.id, pull.headBranch, commitOid);
128 // `update-ref` runs no hooks: announce the push as a git push would be.
129 app.bus.append({
130 kind: "push",
131 repoId: repo.id,
132 actorId: writer.value.id,
133 payload: {
134 updates: [{ ref: `refs/heads/${pull.headBranch}`, newOid: commitOid }],
135 viaApi: true,
136 },
137 });
138 return ok({ commitOid });
139 }
src/git/conflict-file.ts +22 −11
@@ -26,7 +26,26 @@ export async function readConflictedFiles(
2626 paths: readonly string[],
2727 ): Promise<Map<string, string>> {
2828 const out = new Map<string, string>();
29 if (!isSafeOid(ours) || !isSafeOid(theirs) || paths.length === 0) return out;
29 if (paths.length === 0) return out;
30 const tree = await conflictedTree(dir, ours, theirs);
31 if (tree === undefined) return out;
32 for (const path of paths) {
33 if (!isSafeTreePath(path)) continue;
34 const blob = await runGitCapture(["cat-file", "blob", `${tree}:${path}`], {
35 cwd: dir,
36 });
37 if (blob.code === 0 && blob.stdout.length <= MAX_BYTES) out.set(path, blob.stdout);
38 }
39 return out;
40 }
41
42 /** The tree `merge-tree` writes for a conflicted merge, markers and all. */
43 export async function conflictedTree(
44 dir: string,
45 ours: string,
46 theirs: string,
47 ): Promise<string | undefined> {
48 if (!isSafeOid(ours) || !isSafeOid(theirs)) return undefined;
3049 const merged = await runGitCapture(
3150 [
3251 "-c",
@@ -39,15 +58,7 @@ export async function readConflictedFiles(
3958 ],
4059 { cwd: dir, timeoutMs: 60_000 },
4160 );
42 if (merged.code !== 1) return out;
61 if (merged.code !== 1) return undefined;
4362 const tree = merged.stdout.split("\n")[0]!.trim();
44 if (!isSafeOid(tree)) return out;
45 for (const path of paths) {
46 if (!isSafeTreePath(path)) continue;
47 const blob = await runGitCapture(["cat-file", "blob", `${tree}:${path}`], {
48 cwd: dir,
49 });
50 if (blob.code === 0 && blob.stdout.length <= MAX_BYTES) out.set(path, blob.stdout);
51 }
52 return out;
63 return isSafeOid(tree) ? tree : undefined;
5364 }
src/git/write.ts +20 −4
@@ -18,6 +18,8 @@ export interface FileWrite {
1818 readonly path: string;
1919 /** The new bytes, or `null` to delete the path from the tree. */
2020 readonly content: string | Uint8Array | null;
21 /** The file mode. @default "100644" */
22 readonly mode?: "100644" | "100755";
2123 }
2224
2325 export type CommitFilesError =
@@ -49,6 +51,14 @@ export async function commitFiles(
4951 startPoint?: string;
5052 message: string;
5153 files: readonly FileWrite[];
54 /**
55 * Start from this tree instead of the parent's (a merge's result), and
56 * name the parents after the tip: the commit is then a merge.
57 */
58 seedTree?: string;
59 mergeParents?: readonly string[];
60 /** Refuse, writing nothing, unless the branch is at this commit now. */
61 expectTip?: string;
5262 author: { name: string; email: string };
5363 /**
5464 * Called with the sealed commit *before* the local ref moves. An error
@@ -87,9 +97,12 @@ export async function commitFiles(
8797 ? await resolveOid(dir, `refs/heads/${input.startPoint}`)
8898 : undefined);
8999
100 if (input.expectTip !== undefined && tip !== input.expectTip)
101 return err({ kind: "branch_moved" });
102
90103 // Seed the index from the parent's tree, or empty for a first commit.
91 const seed = parent
92 ? await runGit(["read-tree", `${parent}^{tree}`], { cwd: dir, env })
104 const seed = parent || input.seedTree
105 ? await runGit(["read-tree", input.seedTree ?? `${parent}^{tree}`], { cwd: dir, env })
93106 : await runGit(["read-tree", "--empty"], { cwd: dir, env });
94107 if (!seed.ok) return err({ kind: "git", message: seed.error.stderr });
95108
@@ -119,7 +132,7 @@ export async function commitFiles(
119132 "update-index",
120133 "--add",
121134 "--cacheinfo",
122 `100644,${hashed.value.trim()},${file.path}`,
135 `${file.mode ?? "100644"},${hashed.value.trim()},${file.path}`,
123136 ],
124137 { cwd: dir, env },
125138 );
@@ -146,7 +159,10 @@ export async function commitFiles(
146159 GIT_COMMITTER_NAME: input.author.name,
147160 GIT_COMMITTER_EMAIL: input.author.email,
148161 };
149 const parents = parent ? ["-p", parent] : [];
162 const parents = [
163 ...(parent ? ["-p", parent] : []),
164 ...(input.mergeParents ?? []).flatMap((p) => ["-p", p]),
165 ];
150166 const commit = await runGit(
151167 ["commit-tree", tree.value.trim(), ...parents, "-m", input.message],
152168 { cwd: dir, env: commitEnv },
src/http/server.ts +2 −1
@@ -300,7 +300,7 @@ import {
300300 pullThreadFragment,
301301 threadResolveSubmit,
302302 } from "../web/pages/pull.ts";
303 import { conflictsPage } from "../web/pages/conflicts.ts";
303 import { conflictsFinish, conflictsPage } from "../web/pages/conflicts.ts";
304304 import {
305305 appDisable,
306306 appMembersUpdate,
@@ -1002,6 +1002,7 @@ export function buildRouter(): Router<Handler> {
10021002 // box, one tab under the change itself.
10031003 router.add("GET", "/:owner/:name/pulls/:number/files", pullFilesPage);
10041004 router.add("GET", "/:owner/:name/pulls/:number/resolve", conflictsPage);
1005 router.add("POST", "/:owner/:name/pulls/:number/resolve", conflictsFinish);
10051006 router.add("GET", "/:owner/:name/pulls/:number/thread", pullThreadFragment);
10061007 router.add("GET", "/:owner/:name/pulls/:number/state", pullStateFragment);
10071008 router.add("POST", "/:owner/:name/pulls/:number/comments", pullCommentSubmit);
src/tui/screens/conflicts.ts +23 −4
@@ -1,8 +1,8 @@
11 /**
22 * A conflicted pull's blocks in the terminal, the page's twin: a cursor over
33 * the blocks, `o`/`t`/`b` to take ours, theirs or both, `e` for your own
4 * text, and per file "N of M resolved" with the merged file beneath. Nothing
5 * is written to git; the choices live in this screen only.
4 * text, and per file "N of M resolved" with the merged file beneath. The choices
5 * live in this screen only until `f` finishes them: the caller commits them.
66 */
77
88 import type { PullConflicts } from "../../actions/conflicts.ts";
@@ -22,7 +22,17 @@ type Line = { text: string; style: ReturnType<typeof style> }[];
2222
2323 const NAMES = { ours: "Ours", theirs: "Theirs", both: "Both", text: "Mine" };
2424
25 export function conflictScreen(ctx: Ctx, read: PullConflicts): Screen {
25 /** Commit one file's choices; the answer is what to tell the person. */
26 export type Finish = (
27 path: string,
28 choices: readonly (Choice | undefined)[],
29 ) => Promise<{ ok: boolean; message: string }>;
30
31 export function conflictScreen(
32 ctx: Ctx,
33 read: PullConflicts,
34 finish?: Finish,
35 ): Screen {
2636 const scroll = new Scroll();
2737 const composer = new Composer();
2838 /** choices[file][block] */
@@ -48,7 +58,7 @@ export function conflictScreen(ctx: Ctx, read: PullConflicts): Screen {
4858 };
4959 const total = flat.length;
5060 say(`${plural(total, "conflict block")} in ${plural(read.files.length, "file")}`);
51 say("n/p block · o ours · t theirs · b both · e your own text · nothing is written to git", muted);
61 say("n/p block · o ours · t theirs · b both · e your own text · f finish (commit to the branch)", muted);
5262 let n = 0;
5363 read.files.forEach((file, fi) => {
5464 const mine = choices[fi]!;
@@ -104,6 +114,7 @@ export function conflictScreen(ctx: Ctx, read: PullConflicts): Screen {
104114 binding(["n", "p"], "block"),
105115 binding(["o", "t", "b"], "ours / theirs / both"),
106116 binding(["e"], "your text"),
117 binding(["f"], "finish"),
107118 binding(["j", "k"], "scroll"),
108119 binding(["esc", "q"], "back"),
109120 ],
@@ -130,6 +141,14 @@ export function conflictScreen(ctx: Ctx, read: PullConflicts): Screen {
130141 ctx.pop();
131142 return true;
132143 }
144 if (!e.ctrl && !e.alt && e.key === "f" && finish && read.files[0]) {
145 const path = read.files[0].path;
146 void finish(path, choices[0]!).then((done) => {
147 ctx.toast(done.message, done.ok ? "success" : "danger");
148 if (done.ok) ctx.pop();
149 });
150 return true;
151 }
133152 if (!e.ctrl && !e.alt && flat.length > 0) {
134153 if (e.key === "n") return move(1), true;
135154 if (e.key === "p") return move(-1), true;
src/tui/screens/pull.ts +16 −1
@@ -103,6 +103,7 @@ import { assignPicker, assignedText } from "./assign.ts";
103103 import { DiffView, type DiffAnchor, type DiffNote } from "./commits.ts";
104104 import { liveBindings, liveKey } from "./mirror.ts";
105105 import { pullConflicts } from "../../actions/conflicts.ts";
106 import { FINISH_REFUSALS, finishConflicts } from "../../actions/finish-conflicts.ts";
106107 import { conflictScreen } from "./conflicts.ts";
107108 import { reviewScreen } from "./review.ts";
108109 import { RunList, openRunLog, runTone } from "./runlist.ts";
@@ -649,7 +650,21 @@ export function pullScreen(
649650 ctx.toast(errorText(read.error), "danger");
650651 return;
651652 }
652 ctx.push(conflictScreen(ctx, read.value));
653 const seen = read.value;
654 ctx.push(
655 conflictScreen(ctx, seen, async (path, choices) => {
656 const fresh = getPull(f.db, target.id, number) ?? row;
657 const done = await finishConflicts(f.app, f.user, target, fresh, {
658 headOid: seen.headOid,
659 baseOid: seen.baseOid,
660 path,
661 choices,
662 });
663 return done.ok
664 ? { ok: true, message: "Conflicts resolved: committed to the branch." }
665 : { ok: false, message: FINISH_REFUSALS[done.error] };
666 }),
667 );
653668 });
654669 };
655670
src/web/pages/conflicts.ts +50 −3
@@ -18,8 +18,10 @@ import {
1818 } from "../../git/conflicts.ts";
1919 import type { Ctx } from "../../http/context.ts";
2020 import { crumbsFor, notFoundPage, repoBase, repoPalette, repoHeader } from "../chrome.ts";
21 import { FINISH_REFUSALS, finishConflicts } from "../../actions/finish-conflicts.ts";
22 import { field, readForm } from "../form.ts";
2123 import { html, type Html } from "../html.ts";
22 import { page } from "../layout.ts";
24 import { flashTo, page } from "../layout.ts";
2325 import { btn, card, icon } from "../ui.ts";
2426 import { changeHead, changeTabs, pullContext } from "../studio/change.ts";
2527
@@ -148,7 +150,7 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
148150 <p class="muted small mt">
149151 <a href="${back}">← Files</a> · <span class="mono">${file}</span> ·
150152 <strong id="rs-count">${done} of ${total} resolved</strong>
151 · nothing is written to git
153 · nothing is written until you finish
152154 </p>
153155 ${files.length > 1
154156 ? html`<p class="small">
@@ -166,8 +168,13 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
166168 )
167169 : html`<form method="get" action="${here}">
168170 <input type="hidden" name="file" value="${file}" />
171 <input type="hidden" name="head" value="${status.headOid}" />
172 <input type="hidden" name="base" value="${status.baseOid}" />
169173 <div class="rs-list">${cards}</div>
170 <p class="mt">${btn("Update the merged file", { icon: "check" })}</p>
174 <p class="mt">${btn("Update the merged file", { icon: "check", variant: "ghost" })}
175 ${files.length === 1
176 ? html`<button type="submit" formmethod="post" class="btn" data-variant="primary">Finish: commit this to ${pull.headBranch}</button>`
177 : html`<span class="muted small">Finish needs every conflicted file on this page; this pull has ${files.length}. Resolve it in your own checkout.</span>`}</p>
171178 </form>`}
172179 <h2 class="mt">Merged file</h2>
173180 <pre class="rs-merged" aria-label="Merged ${file}">${merged}</pre>`;
@@ -181,3 +188,43 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
181188 styleCss: CSS,
182189 });
183190 }
191
192 // POST /:owner/:name/pulls/:number/resolve
193 export async function conflictsFinish(ctx: Ctx): Promise<Response> {
194 const found = pullContext(ctx);
195 if (found instanceof Response) return found;
196 const { repo, pull } = found;
197 if (!ctx.auth.user || !canWrite(ctx.app.db, repo, ctx.auth.user))
sentinelcrew

High (tests): this gate, and the rw scope and origin check in readForm just below, are the only things between a reader or a read-scoped token and a commit on someone's branch. No test fails if any of them is removed. Add a test where a reader, a stranger, a read-scope token and a cross-origin POST are each refused, and the head branch tip stays where it was.
198 return new Response("forbidden", { status: 403 });
199 const form = await readForm(ctx, "rw");
200 if (form instanceof Response) return form;
201 const dest = `${repoBase(repo)}/pulls/${pull.number}`;
202 const params = new URLSearchParams();
203 for (const [k, v] of form.entries())
204 if (typeof v === "string" && /^[bt]\d{1,4}$/.test(k)) params.set(k, v);
205 const head = field(form, "head");
206 const baseOid = field(form, "base");
207 const path = field(form, "file");
208 const dir = await pullDir(ctx.app.db, ctx.app.reposRoot, repo, pull);
sentinelcrew outdated

Only the page can do this. The terminal's conflicts screen picks the same choices but can't finish them. Wire finishConflicts into src/tui/screens/conflicts.ts so both doors can commit.
209 // Count the blocks the same way the page did, so choices line up.
210 const status = await pullStatus(dir, pull);
211 const text =
212 status.conflicts.includes(path) && head === status.headOid
213 ? await readConflictedFile(dir, status.baseOid, status.headOid, path)
214 : undefined;
215 const total = parseConflicts(text ?? "").blocks.length;
216 const done = await finishConflicts(ctx.app, ctx.auth.user, repo, pull, {
217 headOid: head,
218 baseOid,
219 path,
220 choices: choicesFrom(params, total),
221 });
222 if (!done.ok)
223 return flashTo(ctx, `${dest}/resolve`, {
224 tone: "danger",
225 message: FINISH_REFUSALS[done.error],
226 });
227 return flashTo(ctx, dest, {
228 message: "Conflicts resolved: the choices are committed to the branch.",
229 });
230 }
test/conflicts-tui.test.ts +29 −1
@@ -6,8 +6,10 @@ import { openPull } from "../src/actions/pulls.ts";
66 import { pullConflicts } from "../src/actions/conflicts.ts";
77 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
88 import { createApp, type App } from "../src/app.ts";
9 import { getPull, type Pull } from "../src/core/pulls.ts";
9 import { getPull, pullStatus, type Pull } from "../src/core/pulls.ts";
1010 import type { Repo } from "../src/core/repos.ts";
11 import { runGit } from "../src/git/env.ts";
12 import { repoDir } from "../src/git/repos.ts";
1113 import { createUser, type User } from "../src/core/users.ts";
1214 import { createForgeApp } from "../src/tui/forge.ts";
1315 import { pullScreen } from "../src/tui/screens/pull.ts";
@@ -135,4 +137,30 @@ describe("the terminal reads a conflicted pull's blocks", () => {
135137 expect(after.indexOf("quiet line", after.indexOf("Merged file"))).toBeGreaterThan(-1);
136138 expect(after.indexOf("loud line", after.indexOf("Merged file"))).toBeGreaterThan(-1);
137139 }, 60_000);
140
141 test("f finishes: the head branch gets a merge commit with both lines; a stale head is refused", async () => {
142 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
143 const tip = async (ref: string) =>
144 (await runGit(["rev-parse", ref], { cwd: dir })).ok
145 ? ((await runGit(["rev-parse", ref], { cwd: dir })) as { ok: true; value: string }).value.trim()
146 : "";
147 const was = await tip("refs/heads/loud");
148 const out = await pressV(alice, ["b", "f"]);
149 expect(out).not.toContain("<<<<<<<");
150 // The commit lands off the key's own turn: wait for the branch to move.
151 for (let i = 0; i < 100 && (await tip("refs/heads/loud")) === was; i++) await Bun.sleep(50);
152 const now = await tip("refs/heads/loud");
153 expect(now).not.toBe(was);
154 const parents = await runGit(["rev-list", "--parents", "-n", "1", now], { cwd: dir });
155 expect(parents.ok && parents.value.trim().split(" ")).toEqual([
156 now,
157 was,
158 await tip("refs/heads/main"),
159 ]);
160 const file = await runGit(["show", `${now}:code.txt`], { cwd: dir });
161 expect(file.ok && file.value).toBe("top\nquiet line\nloud line\nend");
162 // The pull no longer conflicts, so there is nothing left to finish.
163 const fresh = getPull(app.db, repo.id, pull.number)!;
164 expect((await pullStatus(dir, fresh)).mergeable).toBe(true);
165 }, 60_000);
138166 });
test/conflicts.test.ts +145
@@ -6,6 +6,8 @@ import { createApp, type App } from "../src/app.ts";
66 import { serve, type Servers } from "../src/http/server.ts";
77 import { applyChoices, parseConflicts } from "../src/git/conflicts.ts";
88 import { choicesFrom } from "../src/web/pages/conflicts.ts";
9 import { runGit } from "../src/git/env.ts";
10 import { repoDir } from "../src/git/repos.ts";
911 import { gitClientEnv } from "./helpers/git-client.ts";
1012
1113 const labels = { ours: "main", theirs: "loud" };
@@ -178,4 +180,147 @@ describe("the Resolve page", () => {
178180 const files = await (await fetch(`${base}/dev/app/pulls/1/files`, { headers })).text();
179181 expect(files).toContain('href="/dev/app/pulls/1/resolve"');
180182 });
sentinelcrew outdated

Good happy path, stale case and incomplete case. Put the non-writer, wrong-scope and cross-origin refusals next to this test.
183
184 const tipOf = async (branch: string): Promise<string> => {
185 const out = await runGit(["rev-parse", `refs/heads/${branch}`], {
186 cwd: repoDir(app.reposRoot, "dev", "app"),
187 });
188 return out.ok ? out.value.trim() : "";
189 };
190 const drawnFields = async (n: number, file: string) => {
191 const drawn = await (
192 await fetch(`${base}/dev/app/pulls/${n}/resolve?file=${file}`, { headers })
193 ).text();
194 const oid = (name: string) =>
195 new RegExp(`name="${name}" value="([0-9a-f]{40})"`).exec(drawn)![1]!;
196 return { file, head: oid("head"), base: oid("base") };
197 };
198 const finishPost = (n: number, auth: Record<string, string>, origin: string | null, body: Record<string, string>) =>
199 fetch(`${base}/dev/app/pulls/${n}/resolve`, {
200 method: "POST",
201 headers: {
202 ...auth,
203 "content-type": "application/x-www-form-urlencoded",
204 ...(origin === null ? {} : { origin }),
205 },
206 body: new URLSearchParams(body),
207 redirect: "manual",
208 });
209
210 test("Finish refuses a reader, a stranger, a read token and a foreign origin, and writes nothing", async () => {
211 const form = { ...(await drawnFields(1, "code.txt")), b0: "both", b1: "ours" };
212 const tip = await tipOf("loud");
213 const mint = async (user: string, scope: string) => {
214 await fetch(`${base}/signup`, {
215 method: "POST",
216 body: new URLSearchParams({ username: user, email: `${user}@f.dev`, password: "conflicts-pass-123" }),
217 redirect: "manual",
218 });
219 const j = (await (
220 await fetch(`${base}/api/v1/tokens`, {
221 method: "POST",
222 headers: { "content-type": "application/json" },
223 body: JSON.stringify({ adminConfirm: true, username: user, password: "conflicts-pass-123", name: "t", scope }),
224 })
225 ).json()) as { token: string };
226 return { authorization: `Bearer ${j.token}` };
227 };
228 const reader = await mint("eve", "rw");
229 const attempts: [string, Response][] = [
230 ["reader", await finishPost(1, reader, base, form)],
231 ["stranger", await finishPost(1, {}, base, form)],
232 ["read token", await finishPost(1, await mint("dev2", "ro"), base, form)],
233 ["foreign origin", await finishPost(1, { authorization: headers["authorization"]! }, "http://evil.example", form)],
234 ];
235 for (const [who, res] of attempts) {
236 expect([who, [403, 404].includes(res.status)]).toEqual([who, true]);
237 }
238 // dev's own read-scoped token is the real case: same user, wrong scope.
239 const devRo = await (
240 await fetch(`${base}/api/v1/tokens`, {
241 method: "POST",
242 headers: { "content-type": "application/json" },
243 body: JSON.stringify({ adminConfirm: true, username: "dev", password: "conflicts-pass-123", name: "ro", scope: "ro" }),
244 })
245 ).json() as { token: string };
246 const ro = await finishPost(1, { authorization: `Bearer ${devRo.token}` }, base, form);
247 expect(ro.status).toBe(403);
248 expect(await tipOf("loud")).toBe(tip);
249 });
250
251 test("Finish refuses a pull with two conflicted files and writes nothing", async () => {
252 await git(["checkout", "main"]);
253 await Bun.write(join(dir, "a.txt"), "a\n");
254 await Bun.write(join(dir, "b.txt"), "b\n");
255 await git(["add", "."]);
256 await git(["commit", "-m", "two files"]);
257 await git(["push", "origin", "main"]);
258 await git(["checkout", "-b", "two"]);
259 await Bun.write(join(dir, "a.txt"), "a-two\n");
260 await Bun.write(join(dir, "b.txt"), "b-two\n");
261 await git(["commit", "-am", "two"]);
262 await git(["push", "origin", "two"]);
263 await fetch(`${base}/api/v1/repos/dev/app/pulls`, {
264 method: "POST",
265 headers,
266 body: JSON.stringify({ base: "main", head: "two", title: "Two" }),
267 });
268 await git(["checkout", "main"]);
269 await Bun.write(join(dir, "a.txt"), "a-main\n");
270 await Bun.write(join(dir, "b.txt"), "b-main\n");
271 await git(["commit", "-am", "main again"]);
272 await git(["push", "origin", "main"]);
273
274 const form = { ...(await drawnFields(2, "a.txt")), b0: "both" };
275 const tip = await tipOf("two");
276 const res = await finishPost(2, { authorization: headers["authorization"]! }, base, form);
277 expect(res.status).toBe(303);
278 const cookie = res.headers.getSetCookie().map((c) => c.split(";")[0]).join("; ");
279 const back = await (
280 await fetch(new URL(res.headers.get("location")!, base), { headers: { ...headers, cookie } })
281 ).text();
282 expect(back).toContain("Finish handles a pull with one conflicted text file");
283 expect(await tipOf("two")).toBe(tip);
284 });
285
286 test("Finish commits the choices onto the head branch and the pull merges", async () => {
287 const url = `${base}/dev/app/pulls/1/resolve`;
288 const drawn = await (await fetch(`${url}?file=code.txt`, { headers })).text();
289 const hidden = (name: string) =>
290 new RegExp(`name="${name}" value="([0-9a-f]{40})"`).exec(drawn)![1]!;
291 const post = (fields: Record<string, string>) =>
292 fetch(url, {
293 method: "POST",
294 headers: {
295 authorization: headers["authorization"]!,
296 "content-type": "application/x-www-form-urlencoded",
297 origin: base,
298 },
299 body: new URLSearchParams({ file: "code.txt", head: hidden("head"), base: hidden("base"), ...fields }),
300 redirect: "manual",
301 });
302 const flash = async (res: Response) => (
303 await fetch(new URL(res.headers.get("location")!, base), {
304 headers: { ...headers, cookie: res.headers.getSetCookie().map((c) => c.split(";")[0]).join("; ") },
305 })
306 ).text();
307
308 // A head that moved since the page was drawn is refused; nothing is written.
309 const stale = await post({ head: "0".repeat(40), b0: "both", b1: "ours" });
310 expect(stale.status).toBe(303);
311 expect(await flash(stale)).toContain("branch moved since this page was drawn");
312
313 // A block with no choice is refused.
314 const open = await post({ b0: "both" });
315 expect(await flash(open)).toContain("Every block needs a choice");
316
317 const done = await post({ b0: "both", b1: "text", t1: "mine\n" });
318 expect(done.headers.get("location")).toBe("/dev/app/pulls/1");
319 const pull = await (await fetch(`${base}/api/v1/repos/dev/app/pulls/1`, { headers })).json() as { mergeable?: boolean };
320 expect(pull.mergeable).toBe(true);
321 const raw = await (await fetch(`${base}/dev/app/raw/loud/code.txt`, { headers })).text();
322 expect(raw).toBe("x\nA-main\nA-loud\nm1\nm2\nm3\nm4\nm5\nm6\nmine\ny\n");
323 const log = await (await fetch(`${base}/api/v1/repos/dev/app/pulls/1`, { headers })).text();
324 expect(log).not.toContain("<<<<<<<");
325 });
181326 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs