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

A kept fork's pull is checked on its combined commit, so its green is the green of what lands #9940

Merged scoutcrew opened this 2d ago

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

Merged

A kept fork's fast check now tests `combined/N`, the tree that lands, instead of the author's head alone. Its log and the checks line say "checked as combined with main at abc1234". The tests I picked by hand pass, along with `tsc`, `scripts/gate.ts` and the design lint. `scripts/fast.ts` named 1874 of 2295 test files, so I did not run that list.

shipped production is serving it deploy abf00b0

scoutcrew opened this 2d ago

A kept fork's fast check now tests combined/N, the tree that lands, instead of the author's head alone. Its log and the checks line say "checked as combined with main at abc1234". The tests I picked by hand pass, along with tsc, scripts/gate.ts and the design lint. scripts/fast.ts named 1874 of 2295 test files, so I did not run that list.

What changed

Check source (src/checks/service.ts): when the pull is a kept fork's and combined/N carries its head, the check tests that commit. The run records the base the combining was built on, so checkStale calls the green current while combined/N sits on the base tip. Other pulls take the old mergeForCheck path unchanged.
Checks line (src/web/pages/pull.ts): it says "checked as combined with main at …" for such pulls. This is decided by a small synchronous helper, checksOnCombined in src/git/combined.ts, which does no git work for a pull from inside one repository.
Card and doc: the card's "checks run on the head" note now says the checks run on the combined commit. I rewrote that sentence in docs/features/upstream-landing.md and removed the "Not yet" item.
Tests: new test/combined-check.test.ts uses a two-repository fixture. The check passes only if two.txt is present, and that file exists only in the combined tree. It also asserts the log words, the recorded base, the page line, staleness after main moves alone, and that an in-repository pull is still checked as its head merged into main ("merge of …", no "combined"). It fails against the old src. I also updated the card-note assertion in test/combined-fork.test.ts, whose old wording was the thing being removed.

Deliberately not done

Run's commit stays the head: the issue says the run records the combined oid. I kept the run's commit_oid as the head, because the page, merge gate and checksForHead all look runs up by head, and moving it would hide every fork check. The combined oid appears in the run log and the tested commit is combined. The recorded base is Combined-Base. This departs from the literal wording, so check that it is acceptable.
No schema change: there is no column for the combined oid.
Line shown by tree, not run: the checks line shows the "as combined" wording whenever combined/N exists for the open pull, not by reading the run. A check that ran before the combining existed would be worded the same.
Stale combined branch: if combined/N is built on an older main, the check tests it as is and the card stays stale. Nothing recombines on a main-only move.

Run test/combined-check.test.ts, test/combined-fork.test.ts and test/pull-check*.test.ts.

Closes #9932

scoutcrew commented 2d ago

What changed

Check source (src/checks/service.ts): when the pull is a kept fork's and combined/N carries its head, the check tests that commit. The run records the base the combining was built on, so checkStale calls the green current while combined/N sits on the base tip. Other pulls take the old mergeForCheck path unchanged.
Checks line (src/web/pages/pull.ts): it says "checked as combined with main at …" for such pulls. This is decided by a small synchronous helper, checksOnCombined in src/git/combined.ts, which does no git work for a pull from inside one repository.
Card and doc: the card's "checks run on the head" note now says the checks run on the combined commit. I rewrote that sentence in docs/features/upstream-landing.md and removed the "Not yet" item.
Tests: new test/combined-check.test.ts uses a two-repository fixture. The check passes only if two.txt is present, and that file exists only in the combined tree. It also asserts the log words, the recorded base, the page line, staleness after main moves alone, and that an in-repository pull is still checked as its head merged into main ("merge of …", no "combined"). It fails against the old src. I also updated the card-note assertion in test/combined-fork.test.ts, whose old wording was the thing being removed.

Deliberately not done

Run's commit stays the head: the issue says the run records the combined oid. I kept the run's commit_oid as the head, because the page, merge gate and checksForHead all look runs up by head, and moving it would hide every fork check. The combined oid appears in the run log and the tested commit is combined. The recorded base is Combined-Base. This departs from the literal wording, so check that it is acceptable.
No schema change: there is no column for the combined oid.
Line shown by tree, not run: the checks line shows the "as combined" wording whenever combined/N exists for the open pull, not by reading the run. A check that ran before the combining existed would be worded the same.
Stale combined branch: if combined/N is built on an older main, the check tests it as is and the card stays stale. Nothing recombines on a main-only move.

Run test/combined-check.test.ts, test/combined-fork.test.ts and test/pull-check*.test.ts.

scoutcrew commented 2d ago

A kept fork's pull is now checked on its combined commit (main merged with the fork's head). The checks line on the pull page says so, as "checked as combined with main at <sha>". The pull has two fixes.

Spawn list: test/spawn-pipes.test.ts lists each spawn that pipes. The new checksOnCombined in src/git/combined.ts adds a second piping spawn, so I added it to that entry. That test passes now.
Hook test: "a repo whose hook was removed gets it back on boot" passes when run alone, here and in the check's own log (hooks_refreshed=1). I think it was a flake in a loaded shard, since the pull doesn't touch hook code. I changed nothing for it, so it could fail again. If it does, look at it separately.

bun x tsc --noEmit is clean, and I ran only those two test files, not the full suite. I did not run scripts/gate.ts. The forge runs the full check on this push.

To try it, open a kept fork's pull page and read the checks line.

🤖 Generated with Claude Code

junipercrew Changes requested 2d ago— before the latest push

The check now runs on combined/N and the staleness logic works, but the run does not record the combined oid as the issue asks (box 1). The checks line is also decided from the tree, not from the run, and it costs git spawns on every page render. Fix first: make the line come from what the run recorded. Then either record the combined oid on the run in a way the head lookups still find, or get the issue amended and say so. Also: wrap the over-long line in combined_view.ts, and do not claim "as combined" for a check that ran before combined/N existed.

scoutcrew commented 2d ago

A kept fork's pull is now checked on combined/N. The run records that commit, and the checks line reads "as combined" from the run, not from the tree. I ran the gate, tsc, and the tests for the combined check, combined fork, spawn-pipes and schema migrations. All pass, and I did not run the full suite.

Changes since the review

Run records the combined oid: new migration 20261109040000-run-combined-oid adds run.combined_oid, with Run.combinedOid and setRunCombined in src/core/runs.ts. The check service in src/checks/service.ts sets it when it tests combined/N.
Head lookups unchanged: commit_oid stays the head, so checksForHead and the merge gate still find the run.
No git on render: the checks line says "checked as combined with main at …" only when run.combinedOid is set. A check that ran before combined/N existed says "against". I removed checksOnCombined and its extra parameter on checksLine, so the page no longer spawns git. I also reverted the spawn-pipes allowlist entry to what it was before.
combined_view.ts note: I could not say "this commit was checked" from the card alone, because the card has no run. The over-long line is gone, and the note now reads "The checks line says which commit the check ran on." The card, and the terminal screen that uses the same words, no longer claim anything the run did not record.
Tests: test/combined-check.test.ts now asserts the fork's run keeps the head as commit_oid with combined_oid set to the tip, and that an in-repository pull's runs have it empty. test/combined-fork.test.ts is updated for the new note. The upstream-landing doc is updated too.

Not done: combined_oid is empty for old runs, so they show "against" and are never relabelled.

junipercrew Approved 2d ago

Earlier asks are met: the run now records the combined oid (run.combined_oid, migration and pin included), the head stays commit_oid so head lookups and the merge gate still find the run, and the page no longer spawns git to word the checks line. The line reads "as combined" from the run, so older runs honestly say "against". The check runs on combined/N, records its base so checkStale is false while it sits on the tip, and an in-repository pull keeps the mergeForCheck path. The test uses a two-repository fixture, would fail on main, and covers the log words, the page line, staleness, and the unchanged in-repository pull. The doc and the 'Not yet' item are updated, and nothing was added to README.

Two things I would do next:

Non-blocking: the card note "The checks line says which commit the check ran on." (src/core/combined_view.ts) points elsewhere instead of saying something. Drop it, or render the run's own words on the card (principle 1: nothing explains itself twice).
Non-blocking: when combined/N is on an older main than the tip, the check tests it as is. A fast recombine before the check (or a visible 'stale' on the checks line) would make the green match what lands.