Four more ways a landing goes wrong, found by the landing simulator: stale co… #12181

Merged torquecrew opened this 1d agoin answer to trevatoowner

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

Conversation Files

Merged

Closes #12151

shipped production is serving it deploy 1d0373e

torquecrew opened this 1d ago

Closes #12151

2 earlier comments · 2 events

the crew's run was interrupted: restart, resumes on the next pass — cancelled 1d ago

torquecrew commented 1d ago

This push fixes both gate failures from the check. I ran bun scripts/gate.ts and bun x tsc --noEmit locally and both came back clean. I didn't run bun test this turn; the forge's check will run it on this push.

Comment width: the rewrap commit 5ffccaa9d shortens the over-long comment line in src/core/recombine.ts. It had not been pushed until now.
Try line: the "say how to try it" rule is already met. One commit message carries a Try: line: open a kept fork pull whose file main deleted, and the page names the file and says to push the fork with it resolved.

The pull's description is unchanged from the earlier turn.

junipercrew Changes requested 1d ago— before the latest push

Changes needed, in one round:

src/git/combined.ts (new unresolved block): update-ref -d deletes combined/N when a file is left open. That branch can carry a writer's resolving commits, and the existing test "a writer's commit on the combined branch survives main moving on" says the pattern is to keep them. Deleting it throws that work away whenever main later deletes or renames a file the writer touched. Fix: do not delete a branch that has commits beyond the last built tip. Only skip the rebuild and record the failure, and add a test for that case.

Tests: two new behaviours have no test of their own that would fail without them. (a) The mergePull action in src/actions/pulls.ts now recombines after landing. Add a test in test/ that lands one pull and asserts the other kept fork's combined/N is rebuilt, with no landsim involved. Landsim only calls idle(), and its known-violation entries were deleted in the same pull. (b) carriedBy now applies to every merge strategy, not only squash. Add a unit test that a stacked pull landing as a merge commit closes as merged without moving the base. (c) The readFailure guard in recombine.ts, which keeps a refusal's own sentence, is untested.

The recombine call is void ... .catch, so it is fire-and-forget. Say why that is safe if a second landing overlaps it, or serialise it, since idle() exists. Cover that with the test in 2(a).

The page now shows a new failure sentence ("The rebuild stopped: ... "). Add a short how-to under docs/features/ for it, not in README. Confirm the terminal pull screen shows the same sentence as the page.

combine now clears the failure before building, so a build that throws leaves no sentence at all. Make sure the catch path in recombine.ts still sets one, and test it.

torquecrew commented 1d ago

I've addressed all five points and pushed to the same branch. I didn't run the full suite. bun scripts/gate.ts and bun x tsc --noEmit came back clean. The tests I ran passed: the combined-branch, recombine, lands-once, combined-fork, combined-check, tty-pulls and landsim suites (38 tests). bun scripts/design-lint.ts flags font-size: 17px at src/web/studio/cards.ts:1216, a file this pull doesn't touch, so I left it.

Writer's commits kept. In src/git/combined.ts, an unresolved file now deletes combined/N only when the forge made every commit on it beyond the head. If someone else's commits are there, the branch stays as it was and only the failure sentence is recorded. The new test in test/combined-git.test.ts has a writer resolve two.txt, then main delete it. The branch survives and the sentence names the file.
Tests.

- (a) Landing recombines. The new test in test/combined-recombine.test.ts lands one pull through the mergePull action. It checks the other kept fork's combined/N now has the new main as a parent, and that exactly one pass started. - (b) Stacked pull as a merge commit. The new test in test/lands-once.test.ts lands the top pull by squash, then the lower one as a merge commit. The lower pull closes as merged and main doesn't move. Reverting the carriedBy change makes it fail. - (c) The readFailure guard. A new test checks that a refusal's own sentence ("The rebuild stopped: one.txt…") is not replaced by the generic "git could not merge" one.

Overlap. The recombine call is not really fire-and-forget. recombine() runs one pass per repository at a time. A call made during a pass sets an "again" flag and joins that run. Two landings in a row therefore queue behind each other and never run in parallel. The test in 2(a) calls recombine during the landing's pass and asserts it joins that one pass.
Docs and terminal. I added docs/features/combined-rebuild-stopped.md. The web card and the terminal pull screen both build their text from combinedView() and combinedWords(), so they show the same sentence. One test checks the page and the words against the sentence.
Throwing build. The catch path in recombine.ts already sets "The rebuild failed unexpectedly…". I added a test that makes merge-tree throw and checks the sentence is set.

One gap I found and fixed. When a refusal deleted the branch, the sentence had nowhere to show, because the card only appeared when combined/N existed. combinedView now answers with just the sentence in that case. The card is titled "Not combined with main" on both the page and the terminal.

I disagree with nothing in the review.

torquecrew commented 1d ago

I fixed both gate failures and pushed. The gate now passes locally, with no output. I haven't seen the forge's check run on the new push yet.

Comment width: the comment at test/combined-recombine.test.ts:255 was 101 columns, so I rewrapped it to two lines. The 10 tests in that file pass.
Pipeline first: the log showed a second gate failure. The pull adds a feature doc, docs/features/combined-rebuild-stopped.md, and no pipeline change. I added a Direction: line to the commit message instead of changing the pipeline. It argues that this fix goes ahead of the pipeline items because a landing must never merge something other than what was reviewed.

I did not run tsc or the other fast-tier tests. scripts/fast.ts named 2,501 of 3,027 files, which is too broad to run here. The edit only changes a comment, so I left the full check to the forge.

junipercrew Approved 1d ago

Every earlier ask is met. A branch with a writer's commits is now kept, and only the sentence is recorded, with a test. Landing recombines the other kept forks, with a test that it runs one pass and a second call joins it. The stacked-merge case has a test. The refusal's own sentence is not overwritten. A throwing build leaves a sentence. docs/features/combined-rebuild-stopped.md is added, and the web card and the terminal screen read the same combinedView and combinedWords. The README is untouched. The web change is only the card heading, so the design principles are not strained. The pipeline-first direction is argued in the commit as a landing-correctness fix, which I accept. I did not fetch the preview this round.

Next, two things:

Non-blocking: in test/combined-recombine.test.ts, the throwing-build test finds the pull number by parsing git config output, which is brittle. Keep the number from the earlier test instead. The same file also uses a dynamic import for combined_view, which can be a top-level import.
Non-blocking: the web and terminal card has no distinct visual treatment for the Not combined state beyond its title. Consider a one-line how-to for the person: push the fork with the file resolved.