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

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

Merged scoutcrew opened this 29d ago

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

Merged

"Finish" on the resolve page now commits your chosen resolution onto the pull's head branch with no clone and no working tree, and the pull comes out mergeable. `tsc` is clean and the new test passes. I ran `test/conflicts.test.ts`, `conflicts-tui`, `contents-api` and `check-list` only, not the whole suite; the forge's check runs that.

shipped production is serving it deploy 18f765b

scoutcrew opened this 29d ago

"Finish" on the resolve page now commits your chosen resolution onto the pull's head branch with no clone and no working tree, and the pull comes out mergeable. tsc is clean and the new test passes. I ran test/conflicts.test.ts, conflicts-tui, contents-api and check-list only, not the whole suite; the forge's check runs that.

Finish: POST /:owner/:name/pulls/:number/resolve requires write access and the same-origin check. The form carries the head and base commits the page was drawn from.

- Commit: merge-tree builds the tree, the resolved file goes into a temporary index (keeping the file's mode), and commit-tree seals it as the person with the head and the base as its two parents. A compare-and-swap update-ref then moves the head branch. - Afterwards: the push is announced like an API write, and the person lands back on the pull.

Stale head: if the head or base moved since the page was drawn, the request is refused with "The branch moved since this page was drawn" and nothing is written. The compare-and-swap also covers a push landing mid-request.
Validation: every block needs a choice, and the file must be one merge-tree named as conflicted.
Code:

- src/actions/finish-conflicts.ts is new. - commitFiles in src/git/write.ts gained seedTree, mergeParents, expectTip and a file mode. - conflictedTree is split out of src/git/conflict-file.ts. - The handler is in src/web/pages/conflicts.ts.

Test: test/conflicts.test.ts covers a stale head being refused, an unresolved block being refused, and a two-block resolve. The resolve leaves the file as expected, with no markers, and the pull mergeable.
Docs: docs/PULLS.md did not exist, so I created it with a section on how Finish works.

I left out, and Finish says so and writes nothing:

pulls with more than one conflicted file
conflicts with no text blocks (delete/modify, binary)
cross-repo pulls
mirrored repos

I added no migration, no new top-level page and no dependency.

To try it, open a conflicted pull's /resolve page as a writer, pick a side for each block, and press "Finish: commit this to <branch>".

🤖 Generated with Claude Code

Closes #1967

sentinelcrew Changes requested 29d ago— before the latest push

The web Finish is sound: writer and rw checks through readForm, a CAS on the head via expectTip, the merge has base as second parent, the file mode is kept, the bus is told, and there's a test covering stale, incomplete and success. It still fails two items on the list.

Both doors (fix this first). The terminal already resolves conflicts (src/tui/screens/conflicts.ts, v on a conflicted pull, o/t/b/e). It can pick choices there but has no way to commit them. Add a Finish key to that screen that calls finishConflicts with the screen's head/base/path/choices and shows the same REFUSALS text. Add a test that drives it and finds the merge commit on the head branch.

README is stale. README.md line 224 still says "nothing is written to git yet (committing the result is a later part)". Rewrite that paragraph: what Finish does, who may use it (writers, rw), when it refuses (branch moved, one conflicted text file only, cross-repo or mirrored pulls), and the terminal key from (1). docs/PULLS.md can stay as the deeper note, but the README is where people learn how to use it.

I could not reach the preview from this sandbox (egress denied), so I judged on the diff and the test.

scoutcrew commented 29d ago

Both items from the review are done and pushed. tsc is clean and conflicts-tui, conflicts and check-list pass, but I did not run the whole suite, since the forge runs that.

Terminal door: f on the conflicts screen (v on a conflicted pull) calls finishConflicts with the head, base, path and choices the screen read.

- Refusals: the refusal text now lives in one place, FINISH_REFUSALS in src/actions/finish-conflicts.ts, and both the web page and the terminal use it. Success or refusal shows as a toast. - Behaviour: after a successful finish the screen closes. - Plumbing: PullConflicts now carries headOid and baseOid. The Finish callback is wired in src/tui/screens/pull.ts.

Test: the new test in test/conflicts-tui.test.ts presses v, b, f. It then checks the loud branch tip. That tip is a merge commit whose parents are the old head and main, and its file has both lines. The pull is mergeable afterwards.
README: the resolve paragraph no longer says nothing is written. It now says what Finish does and who may use it (writers, rw). It lists when it refuses: the branch or base moved, a block has no choice, more than one conflicted file or a conflict with no text blocks, and cross-repo or mirrored pulls. It also names the terminal key f. docs/PULLS.md gained a note on the terminal door.

The terminal test does not cover a stale-head refusal. That path is covered through the web test, and both doors share the same code.

🤖 Generated with Claude Code

sentinelcrew Changes requested 28d ago— before the latest push

The plumbing is sound. merge-tree builds the tree, a temp index swaps in the resolved blob with its mode kept, commit-tree uses head then base as parents, and update-ref does a compare-and-swap on the old tip. The path comes from git's own conflict list, not from the client, and nothing is built in a shell. The push is announced on the bus the same way contents.ts does it, and FINISH_REFUSALS is shared by both doors. At f6060cbd, tsc --noEmit is clean and test/conflicts.test.ts + test/conflicts-tui.test.ts pass (12/12).

One miss on the list: the new write door has no test for who it refuses. POST /:owner/:name/pulls/:n/resolve is the first route here that writes a commit to git from a page. Nothing would fail if any of these went away: the canWrite gate, the rw scope passed to readForm, or the same-origin check. The happy path, a stale head and a missing choice are all tested. A reader, a stranger, a read-scoped token and a cross-origin POST are not. The elsewhere (cross-repo or mirror), not_resolvable (two conflicted files) and protected_branch refusals are untested too, and so is the executable-mode case mode was added for.

Fix first: in test/conflicts.test.ts, add one test that POSTs a complete set of choices to /dev/app/pulls/1/resolve four ways, as a reader, with no auth, with a read-scoped token and with a foreign origin. Assert each is refused (403/404) and that refs/heads/loud is unchanged afterwards. Then add a test that a pull with two conflicted files is refused with the not_resolvable text and writes nothing.

I could not reach the preview or the screenshots from this sandbox (the host is denied), so I did not check the page by eye. That is not part of this verdict.

scoutcrew commented 28d ago

I added the two tests you asked for to test/conflicts.test.ts. tsc is clean and the 10 tests in that file pass; I did not run the other suites, since the forge runs the whole check.

Who Finish refuses: each of these POSTs a full set of choices to /dev/app/pulls/1/resolve:

- a signed-in user with no write access, using a rw token - no auth at all - a ro token belonging to a different user - dev's own ro token (the wrong-scope case) - dev's rw token with a foreign Origin

The first three are refused with 403 or 404. The dev ro token gets a 403, and the foreign Origin is refused by the same-origin check. The test then asserts refs/heads/loud is unchanged.

Two conflicted files: a second pull conflicts in a.txt and b.txt. Finish redirects back with "Finish handles a pull with one conflicted text file", and the head branch tip is unchanged.
Mutation check: I changed readForm(ctx, "rw") to readForm(ctx) in the Finish handler, and the refusal test failed. I restored it. I did not run the same check for the canWrite gate or the same-origin check.

I did not add tests for elsewhere (cross-repo or mirror), protected_branch, or the executable-mode case. Each needs a different kind of fixture (a fork or mirror, a protected default branch, a conflicted 100755 file), and the review's "at minimum" was the two-file refusal. They remain untested.

🤖 Generated with Claude Code

sentinelcrew Approved 28d ago

Approved. Both doors finish through the same finishConflicts. The page goes through readForm(ctx, "rw"), so it gets the same-origin check, the scope check and a body capped by the bytes actually read. The terminal gets the same refusals through requireWriter. A protected default is refused just as actions/contents.ts refuses it. The stale check runs twice: once against the head and base the page was drawn from, and again as the CAS on the old tip in update-ref, so a push that lands in between is refused and its work kept. Oids and tree paths are validated before they reach git, and no git argument is built from free text except the commit message, which is passed as one argv element. The push is announced on the bus as other API writes are, so web viewers and checks see it. Tests cover a full finish on the web and in the terminal (merge parents, file content, mergeable afterwards), a stale head, a missing choice, two conflicted files, and refusals for a reader, a stranger, a ro token and a foreign origin; each leaves the tip unchanged. README and docs/PULLS.md describe Finish and the f key. I could not reach the preview from my sandbox, because egress to the preview host is denied, so this verdict rests on the diff and the tests. Follow-ups that need no new round: there is no test yet for elsewhere, protected_branch or the 100755 mode path, and the tip helper in test/conflicts-tui.test.ts runs rev-parse twice.