feat(runway): reject changes that disagree on provider

## Summary

### Why?

The merger decides which provider a change came from by its URI scheme, and rejects a scheme it has no parser for. That part works, and it happens before any git command runs.

What it does not do is check that the changes in one request agree with each other. `resolveChange` determines the provider per URI and then discards it, so a request whose steps are addressed through different providers is resolved by different parsers and applied as though nothing were unusual. SubmitQueue already refuses that within a single change, but a Runway request carries one step per SubmitQueue request, so nothing covers the request as a whole.

### What?

Keeps `Provider` on `changeRef` — the scheme the change was addressed through — rather than parsing it and throwing it away.

`resolveAndValidate` now compares every change against the first and rejects a request that mixes providers, naming both and the steps they came from. It already walked every URI to validate it, and it runs before the mutex and before any git command, so an incoherent request costs nothing and leaves the checkout untouched.

This cannot refuse a legitimate request: there is no way to address one merge through two providers, and the apply paths would otherwise have to reason about changes resolved by different parsers.

Also names the change, not just the commit, in the unavailable-commit error, so the reader is not sent looking for a deleted commit when the likelier cause is a change this remote was never going to serve.

Whether a change belongs to the repository this merger serves is deliberately not checked. The merger is already constrained to its checkout and remote by configuration, and a change it cannot fetch is refused on those grounds.

## Test Plan

✅ `bazel test //runway/...` — all targets pass (git suite 70s)
✅ `make lint`, `make check-tidy`, `make check-gazelle`, `make test`

New cases: two steps using different providers, one change spanning two providers, and an unsupported provider — each asserted terminal and not a conflict. A multi-step multi-URI request through one provider is asserted to still succeed, guarding against over-rejecting.

The rejection cases run against a Merger whose git executable does not exist, so any git invocation would fail as an exec error. Getting `ErrInvalidRequest` back proves the request was refused before the merger reached for git.

Author: behinddwalls

runway/extension/merger/git/README.md

@@ -19,6 +19,14 @@ Every URI is reduced to three things: the commit to apply, the ref the provider
 
 An unrecognized scheme is a terminal invalid request.
 
+## What a request must agree on
+
+The supported providers are a property of this merger, not of the queue or of the wire contract: the URI scheme selects the parser, and a scheme with no case is a terminal invalid request. Nothing upstream filters on it, so an unsupported provider is first refused here.
+
+Beyond the scheme, every change in one request must come from the same provider. There is no sense in one merge being addressed through two of them, and the check runs before any git command, so an incoherent request costs nothing and leaves the checkout untouched.
+
+Whether a change actually belongs to the repository this merger serves is not checked here — the merger is constrained to its checkout and remote by configuration, and a change it cannot fetch is refused on those grounds.
+
 ## Object availability
 
 The default fetch refspec is `+refs/heads/*`, which does not cover a provider's change refs — a pull request head never also pushed as a branch, the normal case for a fork, is simply absent locally. Every referenced commit is therefore fetched and verified before any step is applied, so a request naming an unreachable commit fails without having touched the checkout.

runway/extension/merger/git/changeref.go

@@ -27,6 +27,9 @@ import (
 // form that does not depend on which provider minted it. Adding a provider
 // means adding one case to resolveChange, not touching the apply paths.
 type changeRef struct {
+	// Provider is the URI scheme the change was addressed through ("github",
+	// "git"). Every change in one request must agree on it.
+	Provider string
 	// SHA is the full commit hash the URI pins the change to. This is the
 	// commit that gets fetched and applied.
 	SHA string
@@ -55,7 +58,8 @@ func resolveChange(uri string) (changeRef, error) {
 			return changeRef{}, fmt.Errorf("%w: invalid change URI %q: %v", merger.ErrInvalidRequest, uri, err)
 		}
 		return changeRef{
-			SHA: cid.HeadCommitSHA,
+			Provider: scheme,
+			SHA:      cid.HeadCommitSHA,
 			// GitHub publishes every PR's head under refs/pull/<n>/head in the
 			// base repository, including PRs opened from a fork.
 			Ref:   fmt.Sprintf("refs/pull/%d/head", cid.PRNumber),
@@ -70,9 +74,10 @@ func resolveChange(uri string) (changeRef, error) {
 		// A git:// URI already names its own fully-qualified ref, so the
 		// staleness check reads exactly the ref the caller pinned.
 		return changeRef{
-			SHA:   cid.CommitSHA,
-			Ref:   cid.Ref,
-			Label: fmt.Sprintf("%s@%s", cid.Repo, cid.Ref),
+			Provider: scheme,
+			SHA:      cid.CommitSHA,
+			Ref:      cid.Ref,
+			Label:    fmt.Sprintf("%s@%s", cid.Repo, cid.Ref),
 		}, nil
 
 	default:

runway/extension/merger/git/git_merger.go

@@ -294,16 +294,18 @@ func (m *gitMerger) process(ctx context.Context, req *runwaymq.MergeRequest, com
 }
 
 // resolveAndValidate normalizes DEFAULT strategies to the configured default,
-// resolves every change URI once for the apply paths to work from, and enforces
-// the PROMOTE composition rule. All failures here are terminal
-// (merger.ErrInvalidRequest): retrying never succeeds, so the controller
-// publishes a FAILED result rather than nacking.
+// resolves every change URI once for the apply paths to work from, checks that
+// the request's changes agree on a provider, and enforces the PROMOTE
+// composition rule. All failures here are terminal (merger.ErrInvalidRequest):
+// retrying never succeeds, so the controller publishes a FAILED result rather
+// than nacking.
 func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedStep, error) {
 	if len(req.GetSteps()) == 0 {
 		return nil, fmt.Errorf("%w: request has no steps", merger.ErrInvalidRequest)
 	}
 
 	resolved := make([]resolvedStep, 0, len(req.GetSteps()))
+	var first providerCheck
 	promoteSeen := false
 	for _, step := range req.GetSteps() {
 		strategy := step.GetStrategy()
@@ -327,6 +329,9 @@ func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedSt
 			if err != nil {
 				return nil, err
 			}
+			if err := first.check(ref, step.GetStepId()); err != nil {
+				return nil, err
+			}
 			refs = append(refs, ref)
 		}
 		resolved = append(resolved, resolvedStep{step: step, strategy: strategy, refs: refs})
@@ -346,6 +351,32 @@ func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedSt
 	return resolved, nil
 }
 
+// providerCheck holds the provider the first change in a request established,
+// and rejects any later change addressed through a different one.
+//
+// There is no sense in one merge being addressed through two providers, and
+// SubmitQueue already refuses it within a single change. Catching it here costs
+// nothing and keeps the apply paths from having to reason about a request whose
+// changes were resolved by different parsers.
+type providerCheck struct {
+	provider string
+	stepID   string
+	set      bool
+}
+
+// check records the first change's provider and compares every later one to it.
+func (o *providerCheck) check(ref changeRef, stepID string) error {
+	if !o.set {
+		o.provider, o.stepID, o.set = ref.Provider, stepID, true
+		return nil
+	}
+	if ref.Provider != o.provider {
+		return fmt.Errorf("%w: request mixes change providers: step %q uses %q, step %q uses %q",
+			merger.ErrInvalidRequest, o.stepID, o.provider, stepID, ref.Provider)
+	}
+	return nil
+}
+
 // applyTransforming runs the reset/apply/push cycle for the transforming
 // strategies (REBASE, SQUASH_REBASE, MERGE), retrying on remote contention when
 // committing. For a dry run it applies the steps locally then discards them.

runway/extension/merger/git/git_merger_test.go

@@ -630,6 +630,78 @@ func TestClassifyMergeFailure(t *testing.T) {
 	}
 }
 
+// --- change provider consistency ---
+
+// newUnrunnableMerger builds a Merger whose git executable does not exist, so
+// any git invocation fails loudly. A request rejected by this Merger with
+// ErrInvalidRequest was therefore rejected before it reached for git — a
+// stronger claim than observing that the remote did not move.
+func (f gitFixture) newUnrunnableMerger(t *testing.T) merger.Merger {
+	t.Helper()
+	return f.newMergerWith(t, func(p *Params) {
+		p.Runtime.Executable = filepath.Join(t.TempDir(), "no-such-git")
+	})
+}
+
+func TestMerge_RejectsInconsistentProvider(t *testing.T) {
+	const otherSHA = "89abcdef0123456789abcdef0123456789abcdef"
+
+	tests := []struct {
+		name string
+		req  *runwaymq.MergeRequest
+	}{
+		{
+			name: "two steps using different providers",
+			req: req("b",
+				stepOf(mergestrategypb.Strategy_REBASE, "s1", "github://github.example.com/uber/one/pull/1/"+fakeSHA),
+				stepOf(mergestrategypb.Strategy_REBASE, "s2", "git://git.example.com/uber/one/refs%2Fheads%2Fmain/"+otherSHA),
+			),
+		},
+		{
+			name: "one change spanning two providers",
+			req: req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1",
+				"github://github.example.com/uber/one/pull/1/"+fakeSHA,
+				"git://git.example.com/uber/one/refs%2Fheads%2Fmain/"+otherSHA,
+			)),
+		},
+		{
+			name: "unsupported provider",
+			req:  req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", "phab://phab.example.com/D123/456")),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := setupGitFixture(t)
+			m := f.newUnrunnableMerger(t)
+
+			_, err := m.Merge(context.Background(), tt.req)
+			require.Error(t, err)
+			assert.True(t, errors.Is(err, merger.ErrInvalidRequest),
+				"want ErrInvalidRequest before any git runs, got %v", err)
+			assert.False(t, errors.Is(err, merger.ErrConflict))
+		})
+	}
+}
+
+func TestMerge_AcceptsMultipleChangesFromOneProvider(t *testing.T) {
+	// Guard against over-rejecting: several steps and several URIs are normal,
+	// so long as they are all addressed through one provider.
+	f := setupGitFixture(t)
+	a := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a")
+	b := f.pushPRCommit(t, "feature/b", "b.txt", "b\n", "add b")
+	c := f.pushPRCommit(t, "feature/c", "c.txt", "c\n", "add c")
+
+	m := f.newMerger(t, mergestrategypb.Strategy_REBASE)
+	res, err := m.Merge(context.Background(), req("b",
+		stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(a), uri(b)),
+		stepOf(mergestrategypb.Strategy_REBASE, "s2", uri(c)),
+	))
+	require.NoError(t, err)
+	assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome())
+	assert.Len(t, f.remoteCommitsSinceSeed(t), 3)
+}
+
 // --- repo migration (unrelated histories) ---
 //
 // A repository migration reaches Runway as an ordinary change in the target
@@ -991,26 +1063,29 @@ func TestMerge_StalenessCheckOffByDefault(t *testing.T) {
 
 func TestResolveChange(t *testing.T) {
 	tests := []struct {
-		name      string
-		uri       string
-		wantSHA   string
-		wantRef   string
-		wantLabel string
-		wantErr   bool
+		name         string
+		uri          string
+		wantProvider string
+		wantSHA      string
+		wantRef      string
+		wantLabel    string
+		wantErr      bool
 	}{
 		{
-			name:      "github pull request",
-			uri:       "github://github.example.com/uber/submitqueue/pull/42/" + fakeSHA,
-			wantSHA:   fakeSHA,
-			wantRef:   "refs/pull/42/head",
-			wantLabel: "uber/submitqueue#42",
+			name:         "github pull request",
+			uri:          "github://github.example.com/uber/submitqueue/pull/42/" + fakeSHA,
+			wantProvider: "github",
+			wantSHA:      fakeSHA,
+			wantRef:      "refs/pull/42/head",
+			wantLabel:    "uber/submitqueue#42",
 		},
 		{
-			name:      "git ref",
-			uri:       "git://git.example.com/uber/monorepo/refs%2Fheads%2Fmain/" + fakeSHA,
-			wantSHA:   fakeSHA,
-			wantRef:   "refs/heads/main",
-			wantLabel: "uber/monorepo@refs/heads/main",
+			name:         "git ref",
+			uri:          "git://git.example.com/uber/monorepo/refs%2Fheads%2Fmain/" + fakeSHA,
+			wantProvider: "git",
+			wantSHA:      fakeSHA,
+			wantRef:      "refs/heads/main",
+			wantLabel:    "uber/monorepo@refs/heads/main",
 		},
 		{name: "unsupported scheme", uri: "phab://phab.example.com/D123/456", wantErr: true},
 		{name: "no scheme", uri: "not-a-uri", wantErr: true},
@@ -1026,6 +1101,7 @@ func TestResolveChange(t *testing.T) {
 				return
 			}
 			require.NoError(t, err)
+			assert.Equal(t, tt.wantProvider, got.Provider)
 			assert.Equal(t, tt.wantSHA, got.SHA)
 			assert.Equal(t, tt.wantRef, got.Ref)
 			assert.Equal(t, tt.wantLabel, got.Label)

runway/extension/merger/git/objects.go

@@ -70,8 +70,11 @@ func (m *gitMerger) ensureObject(ctx context.Context, ref changeRef) error {
 	}
 
 	coremetrics.NamedCounter(m.metricsScope, "merge", "object_unavailable", 1)
-	return fmt.Errorf("%w: commit %s is not available from remote %s (tried by SHA and via %q)",
-		merger.ErrInvalidRequest, ref.SHA, m.remote, ref.Ref)
+	// Name the change, not just the commit. A bare "commit not available"
+	// sends the reader looking for a deleted commit, when the likelier cause is
+	// a change this remote was never going to be able to serve.
+	return fmt.Errorf("%w: commit %s of %s is not available from remote %s (tried by SHA and via %q)",
+		merger.ErrInvalidRequest, ref.SHA, ref.Label, m.remote, ref.Ref)
 }
 
 // hasCommit reports whether sha names a commit object in the local checkout.