refactor(buildrunner): trigger on batches, resolve changes internally

Change BuildRunner.Trigger to take batch identity — base []entity.Batch (the dependency batches) and head entity.Batch (the batch under test) — instead of controller-pre-resolved base/head []entity.Change, per the extension contract. Each implementation (buildkite, githubactions, fake) gains an injected changeset.Resolver and resolves the base and head batches' changes itself; the build controller drops its private collectChanges walk and loads the dependency batches as identity.

Status, Cancel, and the build id/status outputs are unchanged. The wiring injects the resolver into the fake build runner; the buildkite/githubactions Params gain a Resolver field.

Revises build-runner.md, which had deliberately kept batches out of the boundary — the base/head split survives, expressed as batch identity.

Author: behinddwalls

doc/rfc/submitqueue/build-runner.md

@@ -46,23 +46,23 @@ See `extension/buildrunner/build_runner.go` for the exact Go signatures. The sec
 
 ### Trigger: base + head
 
-`Trigger` takes two ordered lists of changes and a free-form metadata map:
+> **Revised by [extension-contract.md](extension-contract.md).** `Trigger` now takes identity at the batch granularity — `base []entity.Batch` (the dependency batches) and `head entity.Batch` (the batch under test) — and the runner resolves each batch's changes itself through an injected `changeset.Resolver`. The base/head split below still holds; only the boundary type changed from resolved `[]entity.Change` to batch identity. The "rejected: list-of-lists of changes" note is superseded by that RFC's "identity in, resolve internally" principle.
 
-- **`base`** — changes from the dependency batches (assumed-good prefix). Ordered.
-- **`head`** — changes from the batch being verified. Ordered.
+`Trigger` takes the base dependency batches, the head batch, and a free-form metadata map:
+
+- **`base`** — the dependency batches (assumed-good prefix), ordered. The runner resolves their changes.
+- **`head`** — the batch being verified. The runner resolves its changes.
 - **`metadata`** — caller-supplied attributes (requester, ticket ID, trace ID, etc.) the provider MAY persist or echo back via `Status`. Schema is caller/provider-defined; the interface treats it as opaque. `nil` is equivalent to an empty map.
 
-The provider applies `base` then `head` in order on top of the queue's target branch and validates the resulting tree. Validation is **implicit and holistic**: it is not a per-change action, it is what the provider does after applying everything.
+The provider resolves and applies `base` then `head` in order on top of the queue's target branch and validates the resulting tree. Validation is **implicit and holistic**: it is not a per-change action, it is what the provider does after applying everything.
 
 Why split base and head:
 
 - The orchestrator's internal model already distinguishes them — a speculation path has a head batch and a list of base batches assumed to pass.
 - Lets a provider cache or short-circuit the base when it has validated the same prefix before — a hot path for stacked-batch speculation.
 - Lets the provider attribute terminal failure to base vs head in `BuildMetadata` without the orchestrator having to round-trip the split itself.
 
-Rejected: a single flat `changes []entity.Change`. Provider would have to deduce base via prefix matching and could not distinguish "base broke" from "head broke" without out-of-band hints. Loses the orchestrator's clearest piece of structural information at the boundary for no gain.
-
-Rejected: list-of-lists, one slice per batch. Pushes batch structure across the boundary, which the provider does not care about. The provider thinks in terms of "stuff to apply before" and "stuff to validate" — base / head matches that mental model. Batches are an orchestrator concept.
+Rejected: a single flat input with no base/head split. Provider would have to deduce base via prefix matching and could not distinguish "base broke" from "head broke" without out-of-band hints. Loses the orchestrator's clearest piece of structural information at the boundary for no gain.
 
 ### Async vs sync contract
 

example/submitqueue/orchestrator/server/main.go

@@ -833,7 +833,7 @@ func newQueueRegistry(logger *zap.Logger, scope tally.Scope, resolver changeset.
 		mergeChecker:   mc,
 		changeProvider: cp,
 		pusher:         psh,
-		buildRunner:    buildfake.New(),
+		buildRunner:    buildfake.New(resolver),
 		scorer: scorerfake.New(resolver, heuristic.New(
 			resolver,
 			[]heuristic.Bucket{{Min: 0, Max: 1<<31 - 1, Score: 0.5}},

submitqueue/extension/buildrunner/build_runner.go

@@ -40,10 +40,11 @@ type BuildRunner interface {
 	// Validation is implicit and holistic — it is what the runner does
 	// after applying everything, not a per-change action.
 	//
-	// base contains changes from the dependency batches (an assumed-good
-	// prefix). head contains changes from the batch being verified.
-	// Splitting them lets a runner cache or short-circuit the base when
-	// it has validated the same prefix before, and lets it attribute
+	// base is the dependency batches (an assumed-good prefix); head is the
+	// batch being verified. The runner resolves each batch's changes itself
+	// through an injected changeset resolver. Keeping base and head as
+	// separate batch inputs lets a runner cache or short-circuit the base
+	// when it has validated the same prefix before, and lets it attribute
 	// terminal failure to base vs head in BuildMetadata.
 	//
 	// metadata carries free-form caller-supplied attributes (e.g. requester,
@@ -59,8 +60,8 @@ type BuildRunner interface {
 	// Factory that built it. Returns an error if the request is invalid.
 	Trigger(
 		ctx context.Context,
-		base []entity.Change,
-		head []entity.Change,
+		base []entity.Batch,
+		head entity.Batch,
 		metadata entity.BuildMetadata,
 	) (buildID entity.BuildID, err error)
 

submitqueue/extension/buildrunner/buildkite/BUILD.bazel

@@ -9,6 +9,7 @@ go_library(
     importpath = "github.com/uber/submitqueue/submitqueue/extension/buildrunner/buildkite",
     visibility = ["//visibility:public"],
     deps = [
+        "//submitqueue/core/changeset",
         "//submitqueue/entity",
         "//submitqueue/extension/buildrunner",
         "@org_uber_go_zap//:zap",
@@ -21,6 +22,8 @@ go_test(
     embed = [":buildkite"],
     deps = [
         "//core/httpclient",
+        "//submitqueue/core/changeset",
+        "//submitqueue/core/changeset/fake",
         "//submitqueue/entity",
         "//submitqueue/extension/buildrunner",
         "@com_github_stretchr_testify//assert",

submitqueue/extension/buildrunner/buildkite/buildkite.go

@@ -38,6 +38,7 @@ import (
 
 	"go.uber.org/zap"
 
+	"github.com/uber/submitqueue/submitqueue/core/changeset"
 	"github.com/uber/submitqueue/submitqueue/entity"
 	"github.com/uber/submitqueue/submitqueue/extension/buildrunner"
 )
@@ -66,9 +67,10 @@ const (
 
 // runner implements buildrunner.BuildRunner.
 type runner struct {
-	cfg    buildrunner.Config
-	client *client
-	logger *zap.SugaredLogger
+	cfg      buildrunner.Config
+	client   *client
+	resolver changeset.Resolver
+	logger   *zap.SugaredLogger
 }
 
 var _ buildrunner.BuildRunner = (*runner)(nil)
@@ -83,6 +85,8 @@ type Params struct {
 	// for the base URL (via httpclient.BaseURLTransport) and auth (via a
 	// transport layer). If nil, http.DefaultClient is used.
 	HTTPClient *http.Client
+	// Resolver resolves a batch's changes (base and head batches).
+	Resolver changeset.Resolver
 	// Logger is the structured logger.
 	Logger *zap.SugaredLogger
 }
@@ -100,24 +104,34 @@ func NewBuildRunner(params Params) (buildrunner.BuildRunner, error) {
 	if params.Logger == nil {
 		return nil, fmt.Errorf("logger is required")
 	}
-	return newRunner(params.Config, &client{httpClient: params.HTTPClient}, params.Logger.Named("buildkite_buildrunner")), nil
+	return newRunner(params.Config, &client{httpClient: params.HTTPClient}, params.Resolver, params.Logger.Named("buildkite_buildrunner")), nil
 }
 
 // newRunner constructs a runner. Used by NewBuildRunner and by tests.
-func newRunner(cfg buildrunner.Config, c *client, logger *zap.SugaredLogger) *runner {
+func newRunner(cfg buildrunner.Config, c *client, resolver changeset.Resolver, logger *zap.SugaredLogger) *runner {
 	return &runner{
-		cfg:    cfg,
-		client: c,
-		logger: logger,
+		cfg:      cfg,
+		client:   c,
+		resolver: resolver,
+		logger:   logger,
 	}
 }
 
 // Trigger calls the Buildkite API to create the build and returns the Buildkite
 // build number as the build ID. Errors are propagated to the caller so the
 // queue consumer can nack and retry.
-func (r *runner) Trigger(ctx context.Context, base, head []entity.Change, metadata entity.BuildMetadata) (entity.BuildID, error) {
-	baseJSON, _ := json.Marshal(flattenURIs(base))
-	headJSON, _ := json.Marshal(flattenURIs(head))
+func (r *runner) Trigger(ctx context.Context, base []entity.Batch, head entity.Batch, metadata entity.BuildMetadata) (entity.BuildID, error) {
+	baseChanges, err := resolveBatches(ctx, r.resolver, base)
+	if err != nil {
+		return entity.BuildID{}, fmt.Errorf("buildkite: resolve base: %w", err)
+	}
+	headChanges, err := r.resolver.ChangesForBatch(ctx, head)
+	if err != nil {
+		return entity.BuildID{}, fmt.Errorf("buildkite: resolve head: %w", err)
+	}
+
+	baseJSON, _ := json.Marshal(flattenURIs(baseChanges))
+	headJSON, _ := json.Marshal(flattenURIs(headChanges))
 
 	env := map[string]string{
 		EnvKeyBaseURIs: string(baseJSON),
@@ -189,6 +203,19 @@ func flattenURIs(changes []entity.Change) []string {
 	return uris
 }
 
+// resolveBatches resolves each batch's changes and concatenates them in order.
+func resolveBatches(ctx context.Context, resolver changeset.Resolver, batches []entity.Batch) ([]entity.Change, error) {
+	var changes []entity.Change
+	for _, b := range batches {
+		cs, err := resolver.ChangesForBatch(ctx, b)
+		if err != nil {
+			return nil, err
+		}
+		changes = append(changes, cs...)
+	}
+	return changes, nil
+}
+
 // encodeBuildNumber encodes a Buildkite build number as the SQ build ID.
 func encodeBuildNumber(number int) string {
 	return strconv.Itoa(number)

submitqueue/extension/buildrunner/buildkite/buildkite_test.go

@@ -27,20 +27,29 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/uber/submitqueue/core/httpclient"
+	"github.com/uber/submitqueue/submitqueue/core/changeset"
+	changesetfake "github.com/uber/submitqueue/submitqueue/core/changeset/fake"
 	"github.com/uber/submitqueue/submitqueue/entity"
 	"github.com/uber/submitqueue/submitqueue/extension/buildrunner"
 )
 
-// newTestRunner creates a runner backed by a test HTTP server.
-func newTestRunner(t *testing.T, handler http.Handler) *runner {
+// newTestRunner creates a runner backed by a test HTTP server. An optional
+// resolver seeds the batch changes the runner resolves; omit it for tests that
+// do not trigger builds (Status/Cancel).
+func newTestRunner(t *testing.T, handler http.Handler, resolver ...changeset.Resolver) *runner {
 	t.Helper()
 	srv := httptest.NewServer(handler)
 	t.Cleanup(srv.Close)
 	c, err := httpclient.NewClient(srv.URL)
 	require.NoError(t, err)
+	r := changeset.Resolver(changesetfake.New())
+	if len(resolver) > 0 {
+		r = resolver[0]
+	}
 	return newRunner(
 		buildrunner.Config{QueueName: "my-queue"},
 		&client{httpClient: c},
+		r,
 		zap.NewNop().Sugar(),
 	)
 }
@@ -70,17 +79,18 @@ func TestTrigger_SubmitsCorrectPayloadAndReturnsBuildkiteNumber(t *testing.T) {
 	var capturedMethod string
 	var capturedBody []byte
 
+	resolver := changesetfake.New().
+		Set("base-batch", entity.Change{URIs: []string{"github://org/repo/pull/1/aaa111"}}).
+		Set("head-batch", entity.Change{URIs: []string{"github://org/repo/pull/2/bbb222"}})
+
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		capturedMethod = req.Method
 		capturedBody, _ = io.ReadAll(req.Body)
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write(buildJSON(42, "scheduled", "https://buildkite.com/test-org/my-pipeline/builds/42"))
-	}))
+	}), resolver)
 
-	base := []entity.Change{{URIs: []string{"github://org/repo/pull/1/aaa111"}}}
-	head := []entity.Change{{URIs: []string{"github://org/repo/pull/2/bbb222"}}}
-
-	id, err := r.Trigger(context.Background(), base, head, nil)
+	id, err := r.Trigger(context.Background(), []entity.Batch{{ID: "base-batch"}}, entity.Batch{ID: "head-batch"}, nil)
 	require.NoError(t, err)
 	assert.Equal(t, encodeBuildNumber(42), id.ID)
 
@@ -95,13 +105,14 @@ func TestTrigger_SubmitsCorrectPayloadAndReturnsBuildkiteNumber(t *testing.T) {
 
 func TestTrigger_EmptyBase_ProducesJSONArray(t *testing.T) {
 	var capturedBody []byte
+	resolver := changesetfake.New().Set("head-batch", entity.Change{URIs: []string{"u"}})
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		capturedBody, _ = io.ReadAll(req.Body)
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write(buildJSON(1, "scheduled", ""))
-	}))
+	}), resolver)
 
-	_, err := r.Trigger(context.Background(), nil, []entity.Change{{URIs: []string{"u"}}}, nil)
+	_, err := r.Trigger(context.Background(), nil, entity.Batch{ID: "head-batch"}, nil)
 	require.NoError(t, err)
 
 	var req createBuildRequest
@@ -112,17 +123,17 @@ func TestTrigger_EmptyBase_ProducesJSONArray(t *testing.T) {
 
 func TestTrigger_MultipleChangesFlattened(t *testing.T) {
 	var capturedBody []byte
+	resolver := changesetfake.New().Set("head-batch",
+		entity.Change{URIs: []string{"github://org/repo/pull/1/aaa"}},
+		entity.Change{URIs: []string{"github://org/repo/pull/2/bbb", "github://org/repo/pull/3/ccc"}},
+	)
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		capturedBody, _ = io.ReadAll(req.Body)
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write(buildJSON(2, "scheduled", ""))
-	}))
+	}), resolver)
 
-	head := []entity.Change{
-		{URIs: []string{"github://org/repo/pull/1/aaa"}},
-		{URIs: []string{"github://org/repo/pull/2/bbb", "github://org/repo/pull/3/ccc"}},
-	}
-	_, err := r.Trigger(context.Background(), nil, head, nil)
+	_, err := r.Trigger(context.Background(), nil, entity.Batch{ID: "head-batch"}, nil)
 	require.NoError(t, err)
 
 	var req createBuildRequest
@@ -138,20 +149,21 @@ func TestTrigger_BuildkiteError_ReturnsError(t *testing.T) {
 		w.WriteHeader(http.StatusInternalServerError)
 	}))
 
-	_, err := r.Trigger(context.Background(), nil, nil, nil)
+	_, err := r.Trigger(context.Background(), nil, entity.Batch{ID: "head-batch"}, nil)
 	require.Error(t, err)
 }
 
 func TestTrigger_WithMetadata_SetsEnvVar(t *testing.T) {
 	var capturedBody []byte
+	resolver := changesetfake.New().Set("head-batch", entity.Change{URIs: []string{"u"}})
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		capturedBody, _ = io.ReadAll(req.Body)
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write(buildJSON(10, "scheduled", ""))
-	}))
+	}), resolver)
 
 	metadata := entity.BuildMetadata{"requester": "alice", "ticket": "SQ-42"}
-	_, err := r.Trigger(context.Background(), nil, []entity.Change{{URIs: []string{"u"}}}, metadata)
+	_, err := r.Trigger(context.Background(), nil, entity.Batch{ID: "head-batch"}, metadata)
 	require.NoError(t, err)
 
 	var req createBuildRequest
@@ -165,13 +177,14 @@ func TestTrigger_WithMetadata_SetsEnvVar(t *testing.T) {
 
 func TestTrigger_NilMetadata_NoMetadataEnvVar(t *testing.T) {
 	var capturedBody []byte
+	resolver := changesetfake.New().Set("head-batch", entity.Change{URIs: []string{"u"}})
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		capturedBody, _ = io.ReadAll(req.Body)
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write(buildJSON(11, "scheduled", ""))
-	}))
+	}), resolver)
 
-	_, err := r.Trigger(context.Background(), nil, []entity.Change{{URIs: []string{"u"}}}, nil)
+	_, err := r.Trigger(context.Background(), nil, entity.Batch{ID: "head-batch"}, nil)
 	require.NoError(t, err)
 
 	var req createBuildRequest

submitqueue/extension/buildrunner/fake/BUILD.bazel

@@ -6,6 +6,7 @@ go_library(
     importpath = "github.com/uber/submitqueue/submitqueue/extension/buildrunner/fake",
     visibility = ["//visibility:public"],
     deps = [
+        "//submitqueue/core/changeset",
         "//submitqueue/core/fakemarker",
         "//submitqueue/entity",
         "//submitqueue/extension/buildrunner",
@@ -17,6 +18,7 @@ go_test(
     srcs = ["fake_test.go"],
     embed = [":fake"],
     deps = [
+        "//submitqueue/core/changeset/fake",
         "//submitqueue/entity",
         "//submitqueue/extension/buildrunner",
         "@com_github_stretchr_testify//assert",

submitqueue/extension/buildrunner/fake/fake.go

@@ -37,6 +37,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/uber/submitqueue/submitqueue/core/changeset"
 	"github.com/uber/submitqueue/submitqueue/core/fakemarker"
 	"github.com/uber/submitqueue/submitqueue/entity"
 	"github.com/uber/submitqueue/submitqueue/extension/buildrunner"
@@ -57,21 +58,28 @@ const outcomeOK = "ok"
 // per-build state: the outcome is encoded in the BuildID at Trigger and read
 // back out at Status. Uniqueness comes from a random suffix per ID, so it needs
 // no shared counter and never collides across instances or processes.
-type runner struct{}
+type runner struct {
+	resolver changeset.Resolver
+}
 
 // New returns a buildrunner.BuildRunner that defaults to succeeding and honors
-// marker tokens embedded in head change URIs.
-func New() buildrunner.BuildRunner {
-	return &runner{}
+// marker tokens embedded in head change URIs. The resolver resolves the head
+// batch's changes so the marker can be inspected.
+func New(resolver changeset.Resolver) buildrunner.BuildRunner {
+	return &runner{resolver: resolver}
 }
 
 // Trigger fails when a head change URI carries the trigger-error marker;
 // otherwise it returns a unique BuildID that encodes the terminal outcome the
 // build should report at Status time (decided from the head marker). The base
-// changes and metadata are ignored.
-func (r *runner) Trigger(_ context.Context, _ []entity.Change, head []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) {
+// batches and metadata are ignored.
+func (r *runner) Trigger(ctx context.Context, _ []entity.Batch, head entity.Batch, _ entity.BuildMetadata) (entity.BuildID, error) {
+	headChanges, err := r.resolver.ChangesForBatch(ctx, head)
+	if err != nil {
+		return entity.BuildID{}, fmt.Errorf("fake: resolve head: %w", err)
+	}
 	outcome := outcomeOK
-	switch fakemarker.TokenInChanges(head) {
+	switch fakemarker.TokenInChanges(headChanges) {
 	case tokenTriggerError:
 		return entity.BuildID{}, fmt.Errorf("fake: marked trigger error")
 	case tokenFail: