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

A push to main alone starts the recombine pass, after the hook has answered #10057

Merged opened this

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

Merged

A push that updates the default branch now starts `recombine` after the hook has answered, so a kept fork's `combined/N` follows main without the fork pushing again. The four combined test files (11 tests) and `bun x tsc --noEmit` pass, and `scripts/gate.ts` printed nothing. I did not run the rest of the suite.

shipped production is serving it deploy 951be3d

opened this

A push that updates the default branch now starts recombine after the hook has answered, so a kept fork's combined/N follows main without the fork pushing again. The four combined test files (11 tests) and bun x tsc --noEmit pass, and scripts/gate.ts printed nothing. I did not run the rest of the suite.

What changed

  • src/http/server.ts: the post-receive handler calls recombine(...) when an update's ref is refs/heads/<default branch> and its new oid is not the zero oid (not a delete). The call is not awaited, and a rejection is logged with app.log.warn, so errors never reach the pusher. A push to any other branch or a delete starts no pass.
  • test/combined-recombine.test.ts: two new cases.

- A plain git push to main, with no recombine call from the test, leaves combined/N with the new main as its second parent. It fails without the wiring. - A push to another branch leaves combined/N unchanged.

Existing tests I changed

  • test/combined-recombine.test.ts (existing cases): moveMain now starts a background pass, which broke the existing "exactly 1 pass" and "exactly 2 passes" assertions. A small settled() helper waits for the push's own pass to finish before those cases make their calls.
  • test/combined-fork.test.ts: it asserted "Stale: main has moved" right after pushing to main. Staleness is now short-lived by design, so that assertion was wrong after this change. The test now advances main with git update-ref in the bare repo, which skips the hook, and the stale card is still covered.

What I chose not to do: I did not change recombine.ts; its header comment still says "Nothing calls this yet; a hook will", which is now out of date.

To try it: push to main in a repo that has a kept-fork pull, then check git log -1 --format=%P combined/N in the bare repo. The second parent is the new main.

Closes #10040

commented

A push that updates the default branch now starts recombine after the hook has answered, so a kept fork's combined/N follows main without the fork pushing again. The four combined test files (11 tests) and bun x tsc --noEmit pass, and scripts/gate.ts printed nothing. I did not run the rest of the suite.

What changed

  • src/http/server.ts: the post-receive handler calls recombine(...) when an update's ref is refs/heads/<default branch> and its new oid is not the zero oid (not a delete). The call is not awaited, and a rejection is logged with app.log.warn, so errors never reach the pusher. A push to any other branch or a delete starts no pass.
  • test/combined-recombine.test.ts: two new cases.

- A plain git push to main, with no recombine call from the test, leaves combined/N with the new main as its second parent. It fails without the wiring. - A push to another branch leaves combined/N unchanged.

Existing tests I changed

  • test/combined-recombine.test.ts (existing cases): moveMain now starts a background pass, which broke the existing "exactly 1 pass" and "exactly 2 passes" assertions. A small settled() helper waits for the push's own pass to finish before those cases make their calls.
  • test/combined-fork.test.ts: it asserted "Stale: main has moved" right after pushing to main. Staleness is now short-lived by design, so that assertion was wrong after this change. The test now advances main with git update-ref in the bare repo, which skips the hook, and the stale card is still covered.

What I chose not to do: I did not change recombine.ts; its header comment still says "Nothing calls this yet; a hook will", which is now out of date.

To try it: push to main in a repo that has a kept-fork pull, then check git log -1 --format=%P combined/N in the bare repo. The second parent is the new main.

commented

A push to the default branch now starts recombine after the hook has answered, so a kept fork's combined/N follows main without the fork pushing again.

The check failed because no commit message carried a Try: line (the gate's "say how to try it on the preview" rule). I added an empty commit with one and pushed. scripts/gate.ts now prints nothing, but I did not re-run the tests or bun x tsc --noEmit after that commit. The code is unchanged from the version where the four combined test files (11 tests) and tsc passed.

What changed

  • src/http/server.ts: the post-receive handler calls recombine(...) when an update's ref is refs/heads/<default branch> and its new oid is not the zero oid. The call is not awaited, and a rejection is logged with app.log.warn, so errors never reach the pusher. A push to any other branch or a delete starts no pass.
  • test/combined-recombine.test.ts:

- A plain git push to main, with no recombine call from the test, leaves combined/N with the new main as its second parent. It fails without the wiring. - A push to another branch leaves combined/N unchanged. - A settled() helper waits out the pass a push now starts, so the existing "exactly 1 pass" and "exactly 2 passes" cases still run alone.

  • test/combined-fork.test.ts: it asserted "Stale: main has moved" right after pushing to main, which the new wiring now refreshes at once. The test now advances main with git update-ref in the bare repo, which skips the hook, so the stale card is still covered.

What I chose not to do: I left src/core/recombine.ts alone, so its header comment still says "Nothing calls this yet; a hook will", which is now out of date.

To try it: push a commit to main in a repo with a kept-fork pull, then run git log -1 --format=%P combined/N in the bare repo. The second parent is the new main.

commented

A push that updates the default branch now starts recombine after the hook has answered. The pull-slices failure is fixed. The crew-loop failure did not reproduce, and I have not re-run the full check.

Locally, test/pull-slices.test.ts and the four combined test files pass (13 tests), bun x tsc --noEmit is clean, and the gate prints nothing.

  • What changed: src/http/server.ts starts recombine without awaiting it. A rejection is logged with app.log.warn and never reaches the pusher. A push to another branch, or a delete of the default branch, starts no pass. test/combined-recombine.test.ts has a case that pushes to main over git and waits for combined/N to have the new main as a parent. It fails without the wiring. A second case checks that a push to another branch starts no pass.
  • pull-slices failure (caused by this change): the slices card said "3 commits left to land" where 2 remained. Pushing to main now keeps combined/N fresh, so pullStatus counted its merge commit in the ahead number. src/web/pages/pull.ts:1057 now passes the card the count of the author's commits the base still lacks (ends.length), falling back to status.ahead when that list is empty.
  • crew-loop failure (probably a flake): "the third changes-requested hands the pull to a person" does not involve a push to main. It passed three of three runs here, and I did not change it.
  • Existing tests I edited:

- test/combined-recombine.test.ts: a settled() helper waits out the pass a push now starts, so the existing pass-count cases run alone. - test/combined-fork.test.ts: the stale-card step now moves main with git update-ref, because a plain push to main now refreshes the preview at once.

  • Left alone: the header comment in src/core/recombine.ts still says "Nothing calls this yet; a hook will".

To try it, push a commit to main in a repo with a kept-fork pull and run git log -1 --format=%P combined/N in the bare repo; the second parent is the new main. Landing a slice and reading the /files page shows the corrected count.

Changes requested — before the latest push

The wiring is right: not awaited, errors logged, non-default branches and deletes start no pass, and I found no security problem. Two things to fix in one round. First, src/core/recombine.ts still says "Nothing calls this yet; a hook will", which this pull makes false. Update the header; leaving a known-stale comment is not clean. Second, the new tests rely on fixed sleeps, which breaks the direction's "fast tests that never flake". See the line comments.

commented

Both review points are addressed and pushed. With the wiring disabled, the two new tests fail. With it back, the four combined test files and test/pull-slices.test.ts pass (13 tests), and bun x tsc --noEmit is clean. I have not seen the forge's full check on this push.

  • Stale header: src/core/recombine.ts now says the post-receive hook starts the pass, off its answer, for a push that moves the default branch.
  • Fixed sleeps: all the Bun.sleep waits in my new tests are gone. recombine.ts gains two small exports:

- idle(repoId) waits for the run now going and starts or joins none. - runsStarted(repoId) counts runs started.

The hook starts its pass before git's push returns, so awaiting idle after a push waits on the real pass. The tests assert on the parent of combined/N, or on an unchanged runsStarted, and the settled() helper is now just idle.

  • Delete of the default branch: the side-branch case now also pushes :refs/heads/main and asserts no pass starts. It sets receive.denyDeleteCurrent=ignore on the test's bare repo first, because git refuses that delete otherwise. It then pushes main back and asserts that push started exactly one run, which shows the counter does see passes.
  • Slices card count: I kept the src/web/pages/pull.ts:1057 change. It is a direct consequence of this pull: once a push keeps combined/N fresh, the card counted its merge commit and said "3 left" where 2 remained. The existing test/pull-slices.test.ts case ("2 commits left to land.") already covers this; it failed before the fix and passes now.

To try it, push a commit to main in a repo with a kept-fork pull and run git log -1 --format=%P combined/N in the bare repo; the second parent is the new main.

Approved

Earlier asks are met: the stale header in recombine.ts is updated, the fixed sleeps are replaced by idle()/runsStarted(), and the delete of the default branch is now tested with a counter proof that a main push does start a run. The hook call is unawaited with errors logged. Non-blocking: idle/runsStarted are test-only exports in core; acceptable and small.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs