Skip to content

fix(git): correct fork→parent push target and PR base#296

Closed
e-jung wants to merge 1 commit into
kunchenguid:mainfrom
e-jung:fix/fork-push-target-pr-base
Closed

fix(git): correct fork→parent push target and PR base#296
e-jung wants to merge 1 commit into
kunchenguid:mainfrom
e-jung:fix/fork-push-target-pr-base

Conversation

@e-jung

@e-jung e-jung commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #293.

no-mistakes v1.29.1 cannot ship a fork-based contribution: the push step and the PR step both derived their target from a single DB field (repos.upstream_url). There was no way to push to a contributor's fork while opening the PR against the parent. Worse, the failure mode is silent — a fork self-PR returns HTTP 200 and looks like success.

This PR adds a separate push target so push→fork and PR→parent route correctly.

The bug

One URL field served two purposes:

  • Push step used sctx.Repo.UpstreamURL directly as the push URL (internal/pipeline/steps/push.go, internal/pipeline/steps/ci_fix.go).
  • PR step used the same URL for both provider detection and gh --repo, and --head was always the bare branch name — never the <fork_owner>:<branch> form gh pr create requires for a cross-repo PR.
  • no-mistakes init read only origin, so whatever origin pointed at became both the push target and the PR base.

Two failure modes depending on what origin was at init time:

  • origin = fork: push lands in the fork, then pr opens a self-PR inside the fork (base = fork's main). CI watches checks on the fork. HTTP 200 → looks like success.
  • origin = parent: push fails with HTTP 403 (contributor can't push to a repo they don't own); run aborts before PR creation.

Manually editing the bare gate's remotes doesn't help: the steps read the DB field, not the worktree remotes, and init is idempotently self-healing (git remote set-url overwrites manual changes back to the stored value).

Repro

(any repo where you lack push access to the parent)

  1. Fork kunchenguid/no-mistakes<you>/no-mistakes; clone the fork (origin = fork).
  2. go build -o ./bin/no-mistakes ./cmd/no-mistakes && ./bin/no-mistakes init
  3. git switch -c scout/repro-fork && git commit -am "scout: fork repro"
  4. git push no-mistakes HEAD

Before: push + PR both target the single upstream_url → self-PR in the fork (or push 403).
After: --fork-url <you>/no-mistakes.git at init (or auto-detected); push lands on the fork and the PR opens against the parent with head = <you>:scout/repro-fork.

The fix

Add a nullable repos.fork_url column. When set, it is the push target and upstream_url remains the PR base (parent); when empty, behavior is unchanged (no regression for non-fork repos).

A single decision point routes the two paths:

  • Repo.PushURL() (internal/db/repo.go) → fork when set, else upstream. The push step and the CI auto-fix push path both call it. No inline re-derivation.
  • GitHub PR creation emits --head "<fork_owner>:<branch>" against the parent (--repo), via github.NewWithFork + Host.headRef. Bitbucket sources the branch from the fork in the POST body; GitLab carries the fork identity on the host.

Surfacing / plumbing:

  • Schema (internal/db/schema.go): ALTER TABLE repos ADD COLUMN fork_url TEXT + idempotent migration (NULL = no fork). COALESCE(fork_url, '') on read; nullableString writes genuine NULL for non-fork repos.
  • db (internal/db/repo.go): Repo.ForkURL + Repo.PushURL(); UpdateRepoMetadata folds in fork_url.
  • cli (internal/cli/init.go): no-mistakes init --fork-url <url> (recommended setup: origin = parent read-only, --fork-url = your fork). status and eject show both URLs when set.
  • gate (internal/gate/gate.go): InitWithFork records fork_url; Init delegates so no caller churn.
  • pipeline: push step + CI auto-fix push use Repo.PushURL().
  • scm: github.NewWithFork emits cross-repo --head; bitbucket and gitlab mirror it.

Verification

gofmt -l . clean · go vet ./... clean · go test -race ./... green (Go 1.26.4) · go build ./cmd/no-mistakes OK.

Two guard tests pin the corrected behavior (the bug's defining symptom is a silent success, so these exist to catch regressions):

  • TestPushStep_TargetsForkWhenForkURLSet (internal/pipeline/steps/push_test.go) — push resolves to the fork when ForkURL is set.
  • TestPRStep_ForkCreatesCrossRepoPR (internal/pipeline/steps/pr_test.go) — PR --repo is the parent and --head is <fork_owner>:<branch>; explicitly asserts the PR does not target the fork (no self-PR).

Plus: TestPRStep_BitbucketForkSourcesBranchFromFork (fork as POST source.repository on parent), a GitHub headRef unit test, and DB round-trip + migration tests (internal/db/repo_test.go).

Step Before After
Push single upstream_url → fork or parent, can't split Repo.PushURL()fork when set
PR --repo <same URL> --head <branch> (self-PR in fork, or push 403) --repo <parent> --head <fork_owner>:<branch>
CI watch polls fork self-PR (or never runs) polls parent PR

No behavior change for non-fork repos: empty fork_url preserves today's single-URL behavior exactly.


AI disclosure: Human-reviewed. The change was produced with AI assistance and reviewed by a human contributor before submission.

no-mistakes used one DB field (repos.upstream_url) for both the push target
and the PR base repo, so it could not push to a contributor's fork while
opening a PR against the parent. The failure mode was silent: a fork self-PR
returns HTTP 200 and looks like success.

Add a nullable repos.fork_url column. When set, it is the push target and
upstream_url remains the PR base (parent); when empty, behavior is unchanged.
A single decision point (Repo.PushURL) routes pushes to the fork, and GitHub
PR creation emits --head <fork_owner>:<branch> against the parent (--repo).
Bitbucket sources the branch from the fork; GitLab carries the fork identity
on the host.

- schema: add fork_url column + idempotent migration (NULL = no fork)
- db: Repo.ForkURL + Repo.PushURL(); UpdateRepoMetadata folds in fork_url
- cli: 'no-mistakes init --fork-url <url>'; status/eject show the fork
- gate: InitWithFork records fork_url (Init delegates; no caller churn)
- pipeline: push step + CI auto-fix push target Repo.PushURL()
- scm: github.NewWithFork emits cross-repo --head; bitbucket/glab mirrors
- tests: push-to-fork guard, cross-repo PR --head guard, db round-trip +
  migration, bitbucket fork source, github headRef unit test

Resolves kunchenguid#293.
@kunchenguid

Copy link
Copy Markdown
Owner

First off, thanks - and the #293 writeup is one of the better bug reports I've gotten. Root cause is exactly right: one upstream_url driving both push and PR with no fork/parent split, and a fork_url + a centralized push-target accessor is the right direction.

Here's where I've landed though. This is core SCM routing - push, PR create, existing-PR lookup, init, migrations, all three providers at once - so I'd rather do it as one clean-slate pass than land it incrementally. A few things from your own analysis I want to get exactly right in that pass:

  • existing-PR lookup: gh pr list --head <owner>:<branch> isn't supported, so the fork PR-lookup needs a different mechanism
  • a plain no-mistakes init must not clobber a stored fork config
  • GitLab: finish the MR routing or explicitly scope it out, not leave it half-wired
  • an e2e fork fixture that actually fails on the silent self-PR (the dangerous case)
  • sequencing with the URL-redaction work (fix(security): redact embedded credentials from upstream URLs in logs and errors #299)

So I'm going to supersede this with my own PR and close this one out once that's up. Your analysis shaped it directly - appreciated. I'll link the replacement here when it lands.

@kunchenguid

Copy link
Copy Markdown
Owner

Superseded by #306, which lands a clean-slate implementation of the fork-routing fix (push routes to the fork, PR opens against the parent with <fork_owner>:<branch>, a supported gh pr list lookup, idempotent --fork-url that survives re-init, GitLab/Bitbucket explicitly scoped out, credential redaction, and an e2e fork fixture). Your analysis here shaped it directly - thank you for the writeup and the root-cause work. Closing in favor of #306.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(scm): no-mistakes cannot ship fork-based contributions — push and PR share one upstream_url, no fork→parent routing

2 participants