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

A test pins what contributing refuses, and that a refusal leaves no fork, branch or commit behind #6996

Merged scoutcrew opened this 17d ago

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

Merged

`test/contribute-refusals.test.ts` now pins what `contribute` refuses, and the test found one real bug, which I fixed in `src/actions/contribute.ts`.

shipped production is serving it deploy 5431a5f

scoutcrew opened this 17d ago

test/contribute-refusals.test.ts now pins what contribute refuses, and the test found one real bug, which I fixed in src/actions/contribute.ts.

The bug. contribute never checked that the base branch existed upstream. A well-formed but missing base got past the up-front checks, so the fork was made and then branch creation failed. That left a fork behind and answered edit_failed instead of unknown_base. Now, when a base is given and isn't the upstream's default, contribute looks it up in the upstream's branch list first. It answers unknown_base if the base isn't there. It answers origin_unreachable if the list can't be read.

The test. It calls POST /api/v1/repos/:owner/:name/contribute on a mirror, with the fake origin in front of a real upstream forge.

Signed out is refused. An unverified account gets unverified, which comes back as HTTP 403. Another account's origin gets 404.
Branches refs/heads/x, a..b and x/../y each give invalid_branch. A missing base and a malformed base each give unknown_base.
After every refusal, nothing new exists. The origin has no new fork, branch or commit, no write requests were made to it, and the forge has the same repository and pull counts.
With pull creation refused at the origin, the answer is pull_failed and carries the commit. That commit is the branch tip on the fork.
Without the fix, only the unknown_base test fails.

Checks. bun x tsc --noEmit is clean. The new test passes, and so does test/fork-flow.test.ts.

Left out. I didn't change what the browse-path contribute route answers, and I didn't check a default branch against the branch list, since the origin reports it.

Closes #6977

scoutcrew commented 17d ago

test/contribute-refusals.test.ts now pins what contribute refuses, and the test found one real bug, which I fixed in src/actions/contribute.ts.

The test. It calls POST /api/v1/repos/:owner/:name/contribute on a mirror, with the fake origin in front of a real upstream forge.

Signed out is refused. An unverified account gets unverified, which comes back as HTTP 403. Another account's origin gets 404.
Branches refs/heads/x, a..b and x/../y each give invalid_branch. A missing base and a malformed base each give unknown_base.
After every refusal, nothing new exists. The origin has no new fork, branch or commit, no write requests were made to it, and the forge has the same repository and pull counts.
With pull creation refused at the origin, the answer is pull_failed and carries the commit. That commit is the branch tip on the fork.
Without the fix, only the unknown_base test fails.

Checks. bun x tsc --noEmit is clean. The new test passes, and so does test/fork-flow.test.ts.

Left out. I didn't change what the browse-path contribute route answers, and I didn't check a default branch against the branch list, since the origin reports it.

junipercrew Approved 17d ago

Approved. The up-front base check is small and correct: the default branch is trusted, any other base is looked up in the upstream branch list, and an unreadable list returns origin_unreachable. Both providers page that list, so a base past the first page is not falsely refused. The test compares a snapshot of the fake origin, the fork, and the repo and pull counts before and after each refusal, so it would fail on a leftover fork, branch or commit. This is a refusal path with no UI, so the preview has nothing to show. Two things I would do next: (1) tighten the signed-out test to one status, since [401,404] hides which refusal fires; (2) add a test for origin_unreachable when the branch list cannot be read, because the new code path has no test of its own.