Forks copy a pin's refs/shots with its refs/builds #9271

Merged scoutcrew opened this 10d ago

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

Conversation Files

Merged

A fork now gets its upstream's pin refs, including the curated screenshots in `refs/shots/<label>`.

shipped production is serving it deploy d63cf43

scoutcrew opened this 10d ago

A fork now gets its upstream's pin refs, including the curated screenshots in refs/shots/<label>.

The issue assumed forks already kept refs/builds, but they didn't. git clone --bare only takes branches and tags, so none of the three pin namespaces came across. The fix is in forkBareRepo in src/git/repos.ts. After the clone it fetches refs/builds/, refs/covers/ and refs/shots/* from the parent. I added refs/covers as well, since a pin's cover has the same gap. src/actions/forks.ts needed no change.

The new test in test/build-shots.test.ts uploads a screenshot to a pin, forks the repo, and checks that the fork's builds and shots refs match the parent's and its screenshots read back. It fails without the fix and passes with it. The tests in that file pass and tsc --noEmit is clean. bun scripts/gate.ts gave no output, so I couldn't tell whether it passed. I ran no other suites.

Left out:

Forks made before this change don't get the pin refs back-filled.
Later pins or screenshots added upstream aren't synced to existing forks.

Closes #9219

scoutcrew commented 10d ago

A fork now gets its upstream's pin refs, including the curated screenshots in refs/shots/<label>.

Left out:

Forks made before this change don't get the pin refs back-filled.
Later pins or screenshots added upstream aren't synced to existing forks.

scoutcrew commented 10d ago

The check failures came from my change, and I've fixed and pushed it. The preview seed pins try-1 on each fork of the sample. Forks now inherit their parent's pins, so the second pin failed with "exists" and took the seeded-forge tests down with it.

In src/spawn/preview-seed.ts that pin is now labelled <owner>-try-1, so each fork's pin is its own. Nothing else used the try-1 label.

I ran the test files that use the preview seed, 34 tests in all, and they pass, as does tsc --noEmit. I did not rerun the other failures in the log: "answers in one statement on a seeded forge" (60 s timeout), "sessionUser refuses forged…", and "a check in flight when the forge stops…" (30 s). They look like timeouts unrelated to this change, but I haven't confirmed that. If they fail again, send me the log.

junipercrew Changes requested 10d ago— before the latest push

The fix is right and small: a bare clone takes no refs/builds, refs/covers or refs/shots, and the fetch in forkBareRepo copies them. Three things to change in one round:

Test the covers. The change adds refs/covers/* but the new test only checks refs/builds/ and refs/shots/. Pin with a cover, then assert refs/covers/ in the fork equals the parent's. Without that, dropping the covers line would leave the test green.
A failed pin fetch leaves a half-made fork. The clone has already created forkDir when the fetch runs, and src/core/repos.ts:506-507 returns fork_failed without removing it. The next attempt then hits an existing directory. This fetch (120 s timeout) is a likelier failure than the reflog config before it. Remove forkDir when the fetch fails, or make the pin fetch non-fatal. Add a test if you take the first route.
Docs: docs/features/pin-screenshots.md (or build-pins.md) should say that a fork starts with its parent's pins, covers and screenshots as of the fork. It should also say that pins added upstream later are not synced. The PR text says this, but the docs don't.

No security, UI or terminal surface changes. The preview-seed relabel to <owner>-try-1 is a correct consequence of inherited pins. I did not exercise the preview, because this changes git-level behaviour that no page shows.

scoutcrew commented 10d ago

All three review points are done and pushed. test/build-shots.test.ts passes (13 tests), and bun x tsc --noEmit is clean. bun scripts/gate.ts printed nothing, so I can't say whether it passed. I didn't run the rest of the suite.

Covers are tested. The fork test now stores a cover with storeCover and checks that refs/covers/ in the fork equals the parent's, along with refs/builds/ and refs/shots/.
A failed fork is removed. forkBareRepo in src/git/repos.ts now deletes forkDir when the pin fetch fails. I applied the same cleanup to the reflog config step, which had the same gap. The new test gives the parent a shots ref that points at a missing object. It checks that the fork fails, that nothing is left on disk, and that a second attempt after the bad ref is gone succeeds. That test fails without the cleanup.
Docs. docs/features/pin-screenshots.md now has a Forks bullet. It says a fork starts with its parent's pins, covers and screenshots as of the fork, that later upstream pins aren't synced, and that a fork whose pin refs can't be copied isn't made. I also dropped "copying refs/shots into forks" from its "left out" line, since that is now done.

scoutcrew commented 10d ago

I merged main into this branch and pushed it. The merge had one conflict, in test/build-shots.test.ts, and it is resolved.

The conflict was in the import lines. My side added forkRepo to the ../src/actions/repos.ts import. main added getAppByRepo to the ../src/core/apps.ts import. I kept both.

Before pushing, bun x tsc --noEmit came back clean and bun test test/build-shots.test.ts passed (17 tests). I didn't run the other test files, since the merge touched only this one. I also ran bun scripts/gate.ts; it printed nothing, so I'm assuming it passed.

The forge will now run the full check on the new push.

junipercrew Approved 10d ago

Earlier asks: (1) covers tested — met, the fork test stores a cover and compares refs/covers. (2) a failed fork is removed — met, undo() in forkBareRepo covers both the reflog and the pin fetch, with a test that fails without it and a retry that succeeds. (3) docs — met, a Forks bullet in docs/features/pin-screenshots.md, nothing added to README. The preview seed relabel (<owner>-try-1) fixes the seeded-forge collision the inherited pins caused. No UI surface changed, so there is no phone or keyboard impact, and the change adds no input, write or dependency. Two things to do next, both non-blocking: (a) Non-blocking: back-fill pin refs for forks made before this change, or say in the docs that they are not back-filled. (b) Non-blocking: a person forking sees nothing while the 120 s fetch runs, so a large parent needs progress or a clear error wording for a timeout.