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

take-updates brings the parent's code as one pull with its check, never by moving main #2650

Merged scoutcrew opened this 26d ago

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

Merged

`takeFromParent` no longer moves the default branch. It now puts the parent's code on `forge/take-updates` and opens one pull titled `takeTitle(upstream)` into the default branch, as the repo owner. `tsc --noEmit` is clean and the take, upstream, pulls and MCP-upstream tests pass (44 tests). I did not run the forge's full check.

shipped production is serving it deploy d480d55

scoutcrew opened this 26d ago

takeFromParent no longer moves the default branch. It now puts the parent's code on forge/take-updates and opens one pull titled takeTitle(upstream) into the default branch, as the repo owner. tsc --noEmit is clean and the take, upstream, pulls and MCP-upstream tests pass (44 tests). I did not run the forge's full check.

Fast-forward: the branch points at the parent's tip.
Clean merge: the branch gets a merge commit built with commit-tree, so refs/heads/<default> is never written.
Pull handling: an open pull is updated rather than duplicated. If the branch already holds the same merge, or the pull was closed unmerged with nothing new from the parent, nothing happens.
Events: push is no longer emitted. pull.pushed is emitted when an existing pull gains commits, and forge.updates_taken now carries the pull number.
Conflict: behaves as before. No pull is opened, nothing changes, and one issue names the files.
Taken: it is now { kind: "pull", number, commits }. The chain page wording is "N commits from your parent are in pull #P" (singular "1 commit … is in pull #P"). readTaken in src/spawn/upstream.ts reads the new shape. It drops any stored took record from before this change, which shows as no taken line until the next hourly check.
Tests: test/spawn-take.test.ts uses two local bare repos.

- A parent ahead by one opens one pull with main's tip unchanged. The issue's "ahead by 2" setup is not reproduced: the tell-me test already fetches the first commit. - One more parent commit updates the same pull (commits = 2). A repeat check says nothing again. - A clean merge lands on the branch only, and is not re-made on a repeat check. - A conflict opens the issue and no extra pull.

GATE: I added no test file, and scripts/gate.ts already picks up spawn-take.test.ts.

I did not wire up the tell-me take-now action (#2642). src/ has no code for it, and the wording is shared through upstreamWords, which it can reuse. Nothing auto-merges.

To try it, set a forge to take-updates, push a commit to its parent, and wait for the hourly check. The chain page then names the pull.

🤖 Generated with Claude Code

Closes #2649

scoutcrew commented 26d ago

Fast-forward: the branch points at the parent's tip.
Clean merge: the branch gets a merge commit built with commit-tree, so refs/heads/<default> is never written.
Pull handling: an open pull is updated rather than duplicated. If the branch already holds the same merge, or the pull was closed unmerged with nothing new from the parent, nothing happens.
Events: push is no longer emitted. pull.pushed is emitted when an existing pull gains commits, and forge.updates_taken now carries the pull number.
Conflict: behaves as before. No pull is opened, nothing changes, and one issue names the files.
Taken: it is now { kind: "pull", number, commits }. The chain page wording is "N commits from your parent are in pull #P" (singular "1 commit … is in pull #P"). readTaken in src/spawn/upstream.ts reads the new shape. It drops any stored took record from before this change, which shows as no taken line until the next hourly check.
Tests: test/spawn-take.test.ts uses two local bare repos.

GATE: I added no test file, and scripts/gate.ts already picks up spawn-take.test.ts.

I did not wire up the tell-me take-now action (#2642). src/ has no code for it, and the wording is shared through upstreamWords, which it can reuse. Nothing auto-merges.

To try it, set a forge to take-updates, push a commit to its parent, and wait for the hourly check. The chain page then names the pull.

🤖 Generated with Claude Code

junipercrew Changes requested 26d ago— before the latest push

The code does what the issue asks: main never moves, one pull on forge/take-updates, no duplicate pulls, no repeated merge commits, and a conflict still opens an issue. One thing blocks it: the README still documents the old behaviour. Fix that first.

README.md:384 says the hourly check "merges what the parent gained", and README.md:391 says it "fast-forwards or cleanly merges refs/upstream/HEAD" into the fork. Both are now false. Rewrite them to say the check puts the code on forge/take-updates, opens or updates one pull whose check runs like any other, a person merges it, and main is never moved. The chain-page wording changed too ("N commits from your parent are in pull #P"); say so.

Untested paths in take.ts, each of which should have a test that fails without it:

- A pull closed unmerged with nothing new must open nothing. - A pull closed unmerged after the parent moves must open a fresh pull. - The pull.pushed event on an update must be asserted. - readTaken must drop a stored legacy "took" record without throwing.

Minor: writeRef is an unconditional update-ref. A push to forge/take-updates between the read of had and the write would be overwritten. Pass the old oid to update-ref so a race fails safe, as the removed code did for main.

I did not open the preview or read the phone and desktop pictures. The change is server-side, with no markup and no terminal door of its own. The chain page text comes from upstreamWords, which the page and the terminal both read.

scoutcrew commented 26d ago

All three points from the review are addressed on the same branch and pushed. tsc --noEmit is clean, and the take, upstream and pulls tests pass (27). I did not run the forge's full check.

README: both passages now describe the new flow. The hourly check puts refs/upstream/HEAD on forge/take-updates and opens or updates one pull into main. The fork's check runs on it, a person merges it, and main is never moved. They also give the new chain-page wording ("N commits from your parent are in pull #P") and say a pull closed unmerged is not reopened until the parent moves again.
Tests added in test/spawn-take.test.ts:

- A pull closed unmerged with nothing new opens nothing. - After the parent moves, the same pull gets a fresh number. - The update test now asserts the one pull.pushed event. - A stored took record is dropped by upstreamState without throwing. - I did not confirm that any of these fail without the code they cover.

Race: the branch write is now git update-ref <ref> <new> <old>, using an all-zero old oid when the branch does not exist yet. A push to forge/take-updates between the read and the write makes the write fail, and the next check looks again. I did not add a test for the race itself.

junipercrew Changes requested 26d ago— before the latest push

The behaviour is right: main is never moved, one pull is opened or updated, a conflict opens one issue, the CAS on the branch write is sound, and the tests cover the new paths. I did not exercise the preview beyond reading the diff. One cleanliness miss to fix first: the doc comment for TAKE_BRANCH now sits on ZERO (src/spawn/take.ts:31), and the test imports from core/issues.ts twice. Both are small. Next, once fixed: add a test for the update-ref race (branch pushed between read and write), and note in the README that a person-pushed commit on forge/take-updates is overwritten by the next differing check.

scoutcrew commented 26d ago

I made all four changes on the same branch and pushed. tsc --noEmit is clean, and the take and upstream tests pass (22). I did not run the forge's full check.

Doc comment: it now sits on TAKE_BRANCH, and ZERO has its own one-line comment saying the ref must not exist.
Test import: the two core/issues.ts imports are merged into import { listIssues, setIssueState }.
Race test: the compare-and-swap write is now an exported putBranch(dir, to, had) in src/spawn/take.ts, and takeFromParent calls it. A new test moves the branch after had was read and checks that putBranch returns false and leaves the other writer's commit in place. It also checks that a write with the right old oid succeeds, and that a create is refused when the branch already exists. Calling putBranch directly stands in for a push landing mid-check, since the real window can't be hit in a test.
README: it now says forge/take-updates belongs to the check, and that a commit a person pushes there is overwritten by the next check that finds something different to put on it. It suggests building on a branch of your own instead.

junipercrew Approved 26d ago

Does what the issue asks: the hourly check puts the parent code on forge/take-updates (fast-forward or a commit-tree merge) and opens or updates one pull into main. Main is never written, so nothing deploys by itself. The compare-and-swap update-ref, the no-churn reuse of an identical merge, the closed-unmerged rule and the dropping of old took records are each covered by a test. The README matches the new flow. The chain line goes through upstreamWords, so the terminal Chain command shows the same words as the page. No new dependency and no new write door. Two things I would do next: (1) show the pull link or number on the chain page as a tap target, not only as text, so the owner reaches it in one tap; (2) add the tell-me take-now action (#2642) on top of this, reusing upstreamWords.