fix(extension): forward the per-queue Config down to every impl (#553)

## Summary

### Why?

Per-queue extensions are resolved through a `Factory` contract —
`For(cfg Config) (Impl, error)`, where `Config` carries the queue name.
The controllers hold up their end: `build.go:233` and
`buildsignal.go:183` both call
`c.buildRunners.For(buildrunner.Config{QueueName: batch.Queue})`.

The wiring layer dropped it. In
`service/submitqueue/orchestrator/server/profiles.go`, `Profile` held
built extension **instances** for ChangeProvider, BuildRunner, Analyzer,
Scorer and Speculator. Only `Storage` held a `Factory`. So the two ends
of the seam looked like this:

```go
return p.For(c.QueueName).Storage.For(c)   // forwards cfg onward
return p.For(c.QueueName).BuildRunner, nil // cfg is a map key, then dropped
```

An implementation's queue name was therefore fixed at construction. For
queues listed in `byQueue` you could hand-copy the name into each
profile. For `defaultProfile` you could not: one instance serves every
queue without an explicit entry, so it can only ever carry one queue
name — or none.

The visible symptom is the Buildkite and GitHub Actions build runners,
which read `cfg.QueueName` to emit `SQ_QUEUE` / `sq_queue` for the
pipeline script. Both have zero non-test callers, so nothing ever
exercised the path and nothing caught the gap.

### What?

**The seam.** Every `Profile` field is now a `Factory`, and each
`XFactory()` resolves the profile by name and then forwards the whole
`Config` into it — the same second `For(c)` that `Storage` already used.
Implementations are built per resolution, which is what
`counterFactory.For` already does for the MySQL counter; anything
expensive and queue-independent (the resolver, the metrics scope, the
change provider's HTTP clients) is still built once in `newProfiles`.
`newChangeProvider` becomes `newChangeProviderFactory`, splitting HTTP
client construction from the per-queue provider that wraps it.

`withSpeculator` becomes a lazy closure that resolves the profile's
scorer at the queue the speculator itself was asked for, so the fix
reaches one level down. That introduces a real error path where a struct
field read could not fail; it is propagated rather than yielding a
speculator over a nil scorer. `ScorerFactory()` is added for symmetry —
the scorer was previously reachable only through `withSpeculator`.

**The implementations.** Every impl belonging to an extension that has a
`Config`+`Factory` pair now receives it: the conflict analyzers, the
scorers, the validators, the change providers, the merge checkers, the
fake build runners, the standard speculator, Stovepipe's two fakes, and
Runway's `noop` and `fake` mergers. Impls with an existing `Params`
struct gain a `Config` field, matching the Buildkite precedent; the rest
take `cfg` as a leading positional parameter.

Three decisions worth a reviewer's attention:

- **`bestfirst` and `sticky` are untouched.** Only `speculator` declares
a `Config` and a `Factory`; `generator` and `allocator` declare neither.
They are composition internals rather than per-queue seams, so giving
them a `Config` would have meant inventing two types that nothing reads.
- **Runway's mergers are stateful by design.** `fakeMergerFactory`
shared one instance so its `atomic.Uint64` revision-id counter stayed
unique, and `gitMergerFactory` shares one checkout. Building one per
queue would have broken both. The counter moves to the host factory and
is injected, which keeps ids unique process-wide while letting each
merger carry its own queue identity. This also fixes an existing quirk
in `noopMergerFactory`, which already built a fresh merger — and
therefore a fresh counter — on every resolution. `gitMergerFactory`
still shares one instance and is now the only merger factory that does
not forward its `Config`, with a comment recording why.
- **`validatorfake.NewFactory` moves into the wiring.** `CLAUDE.md`
reserves extension packages for contracts and implementations — deciding
which impl serves which queue is host policy — so it is replaced by a
`validatorFactory` adapter in the orchestrator wiring.

Four implementations deliberately still take no `Config`: `submitqueue`,
`stovepipe` and `platform`'s MySQL storage/counter backends already take
the queue name as a plain string and the host adapters bridge to it, and
`merger/git` is the shared-checkout exception above.

The change is split into five commits — four mechanical per-family ones,
then the wiring rewrite that actually fixes the bug — so it can be
reviewed and bisected a family at a time.

## Test Plan

✅ `make test` — 96/96, including three new cases in `profiles_test.go`
✅ `make build` — all four service binaries
✅ `bazel test //test/integration/...` — 8/8
✅ `bazel test //test/e2e/...` — 3/3
✅ `make lint`, `make check-gazelle`, `make check-tidy`, `make
check-mocks` — all clean

Mutation-checked the regression guard: reverting a single `For(c)` back
to `For(Config{})` fails `TestProfilesForwardQueueNameToFactories` for
both the listed-queue and default-profile cases. That test is the thing
that was missing — it asserts the `Config` survives the profile lookup,
which is precisely what no test covered before.

The implementation half was already covered: `buildkite_test.go:109` and
`githubactions_test.go:123` assert the queue name reaches the wire.
Together the two halves cover the full path from controller to CI
request.

Two pre-existing issues left untouched, both unrelated to this change:
`go test ./runway/extension/merger/git/...` fails outside Bazel on a
machine without a hermetic git toolchain (it passes under `bazel test`),
and `go vet` reports `copylocks` on protobuf `MessageState` in the
mergesignal tests.

Author: behinddwalls

runway/extension/merger/fake/fake.go

@@ -51,12 +51,21 @@ var _ merger.Merger = (*Merger)(nil)
 // Merger is a Merger that succeeds unless a marker token in a change URI
 // requests otherwise.
 type Merger struct {
-	seq atomic.Uint64
+	// cfg is the per-queue identity this merger was built for.
+	cfg merger.Config
+	// seq mints the synthetic revision ids returned by Merge. It is supplied by
+	// the caller so a factory that builds one Merger per queue can still hand
+	// out ids that are unique across every queue in the process.
+	seq *atomic.Uint64
 }
 
-// New returns a Merger that defaults to success and honors marker tokens
-// embedded in change URIs.
-func New() *Merger { return &Merger{} }
+// New returns a Merger bound to the queue named in cfg that defaults to success
+// and honors marker tokens embedded in change URIs. seq must be non-nil and is
+// shared, not owned: callers minting ids from more than one Merger must pass the
+// same counter to each.
+func New(cfg merger.Config, seq *atomic.Uint64) *Merger {
+	return &Merger{cfg: cfg, seq: seq}
+}
 
 // CheckMergeability reports the request as mergeable unless a recognized marker
 // token asks for a failure. Outputs are empty, as for any dry run.

runway/extension/merger/fake/fake_test.go

@@ -16,6 +16,7 @@ package fake
 
 import (
 	"context"
+	"sync/atomic"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -27,6 +28,9 @@ import (
 	"github.com/uber/submitqueue/runway/extension/merger"
 )
 
+// testCfg is the per-queue identity used by every case in this file.
+var testCfg = merger.Config{QueueName: "test-queue"}
+
 const baseURI = "github://github.example.com/uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"
 
 // requestWith builds a two-step request whose second step carries the given
@@ -54,7 +58,7 @@ func TestUnmarkedRequestSucceeds(t *testing.T) {
 	req := requestWith(baseURI)
 
 	t.Run("check mergeability reports no outputs", func(t *testing.T) {
-		res, err := New().CheckMergeability(context.Background(), req)
+		res, err := New(testCfg, new(atomic.Uint64)).CheckMergeability(context.Background(), req)
 		require.NoError(t, err)
 
 		assert.Equal(t, req.GetId(), res.GetId())
@@ -67,7 +71,7 @@ func TestUnmarkedRequestSucceeds(t *testing.T) {
 	})
 
 	t.Run("merge reports one output per step", func(t *testing.T) {
-		res, err := New().Merge(context.Background(), req)
+		res, err := New(testCfg, new(atomic.Uint64)).Merge(context.Background(), req)
 		require.NoError(t, err)
 
 		assert.Equal(t, req.GetId(), res.GetId())
@@ -102,10 +106,10 @@ func TestMarkedRequestFails(t *testing.T) {
 				fn   func(*runwaymq.MergeRequest) (*runwaymq.MergeResult, error)
 			}{
 				{"CheckMergeability", func(r *runwaymq.MergeRequest) (*runwaymq.MergeResult, error) {
-					return New().CheckMergeability(context.Background(), r)
+					return New(testCfg, new(atomic.Uint64)).CheckMergeability(context.Background(), r)
 				}},
 				{"Merge", func(r *runwaymq.MergeRequest) (*runwaymq.MergeResult, error) {
-					return New().Merge(context.Background(), r)
+					return New(testCfg, new(atomic.Uint64)).Merge(context.Background(), r)
 				}},
 			} {
 				t.Run(call.name, func(t *testing.T) {
@@ -129,7 +133,7 @@ func TestMarkedRequestFails(t *testing.T) {
 func TestUnrecognizedTokenSucceeds(t *testing.T) {
 	req := requestWith(baseURI + "?sq-fake=some-other-fakes-token")
 
-	res, err := New().Merge(context.Background(), req)
+	res, err := New(testCfg, new(atomic.Uint64)).Merge(context.Background(), req)
 	require.NoError(t, err)
 	assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome())
 }
@@ -144,7 +148,7 @@ func TestFirstRecognizedTokenWins(t *testing.T) {
 		},
 	}
 
-	_, err := New().Merge(context.Background(), req)
+	_, err := New(testCfg, new(atomic.Uint64)).Merge(context.Background(), req)
 	require.Error(t, err)
 	assert.ErrorIs(t, err, merger.ErrConflict)
 	assert.NotErrorIs(t, err, merger.ErrInvalidRequest)

runway/extension/merger/noop/BUILD.bazel

@@ -21,6 +21,7 @@ go_test(
         "//api/base/mergestrategy/protopb:go_default_library",
         "//api/runway/messagequeue:go_default_library",
         "//api/runway/messagequeue/protopb:go_default_library",
+        "//runway/extension/merger:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",
     ],

runway/extension/merger/noop/noop.go

@@ -31,11 +31,20 @@ var _ merger.Merger = (*Merger)(nil)
 
 // Merger is a no-op implementation that always succeeds.
 type Merger struct {
-	seq atomic.Uint64
+	// cfg is the per-queue identity this merger was built for.
+	cfg merger.Config
+	// seq mints the synthetic revision ids returned by Merge. It is supplied by
+	// the caller so a factory that builds one Merger per queue can still hand
+	// out ids that are unique across every queue in the process.
+	seq *atomic.Uint64
 }
 
-// New returns a new no-op Merger instance.
-func New() *Merger { return &Merger{} }
+// New returns a no-op Merger bound to the queue named in cfg. seq must be
+// non-nil and is shared, not owned: callers minting ids from more than one
+// Merger must pass the same counter to each.
+func New(cfg merger.Config, seq *atomic.Uint64) *Merger {
+	return &Merger{cfg: cfg, seq: seq}
+}
 
 func (v *Merger) CheckMergeability(_ context.Context, req *runwaymq.MergeRequest) (*runwaymq.MergeResult, error) {
 	steps := make([]*runwaymq.StepResult, len(req.GetSteps()))

runway/extension/merger/noop/noop_test.go

@@ -16,8 +16,11 @@ package noop
 
 import (
 	"context"
+	"sync/atomic"
 	"testing"
 
+	"github.com/uber/submitqueue/runway/extension/merger"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	changepb "github.com/uber/submitqueue/api/base/change/protopb"
@@ -26,6 +29,9 @@ import (
 	runwaypb "github.com/uber/submitqueue/api/runway/messagequeue/protopb"
 )
 
+// testCfg is the per-queue identity used by every case in this file.
+var testCfg = merger.Config{QueueName: "test-queue"}
+
 func testRequest() *runwaymq.MergeRequest {
 	return &runwaymq.MergeRequest{
 		Id:        "queue-a/42",
@@ -46,7 +52,7 @@ func testRequest() *runwaymq.MergeRequest {
 }
 
 func TestCheckMergeability(t *testing.T) {
-	v := New()
+	v := New(testCfg, new(atomic.Uint64))
 	req := testRequest()
 
 	res, err := v.CheckMergeability(context.Background(), req)
@@ -62,7 +68,7 @@ func TestCheckMergeability(t *testing.T) {
 }
 
 func TestMerge(t *testing.T) {
-	v := New()
+	v := New(testCfg, new(atomic.Uint64))
 	req := testRequest()
 
 	res, err := v.Merge(context.Background(), req)
@@ -80,7 +86,7 @@ func TestMerge(t *testing.T) {
 }
 
 func TestMerge_UniqueOutputIDs(t *testing.T) {
-	v := New()
+	v := New(testCfg, new(atomic.Uint64))
 	req := testRequest()
 
 	res1, err := v.Merge(context.Background(), req)

service/runway/server/main.go

@@ -25,6 +25,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"syscall"
 	"time"
 
@@ -314,10 +315,10 @@ func newMergerFactory(logger *zap.Logger, scope tally.Scope) (merger.Factory, er
 		// Marker-driven outcomes, for e2e tests that need Runway to fail on
 		// demand without a git checkout. Never production.
 		logger.Info("MERGER=fake; using marker-driven fake merger")
-		return &fakeMergerFactory{merger: fake.New()}, nil
+		return &fakeMergerFactory{seq: new(atomic.Uint64)}, nil
 	case "noop":
 		logger.Info("MERGER=noop; using noop merger")
-		return &noopMergerFactory{}, nil
+		return &noopMergerFactory{seq: new(atomic.Uint64)}, nil
 	case "", "git":
 		// Fall through to the merge-environment default below.
 	default:
@@ -327,7 +328,7 @@ func newMergerFactory(logger *zap.Logger, scope tally.Scope) (merger.Factory, er
 	checkoutPath := os.Getenv("MERGE_CHECKOUT_PATH")
 	if checkoutPath == "" {
 		logger.Info("MERGE_CHECKOUT_PATH not set; using noop merger")
-		return &noopMergerFactory{}, nil
+		return &noopMergerFactory{seq: new(atomic.Uint64)}, nil
 	}
 
 	defaultStrategy, err := parseStrategy(os.Getenv("MERGE_DEFAULT_STRATEGY"))
@@ -370,7 +371,8 @@ func newMergerFactory(logger *zap.Logger, scope tally.Scope) (merger.Factory, er
 
 // gitMergerFactory returns a single git-backed merger for every queue. The
 // merger owns one checkout and serializes its own operations, so one instance
-// is shared across queues. A deployment that lands multiple targets wires a
+// is shared across queues — which is why this is the one merger factory that
+// does not forward its Config. A deployment that lands multiple targets wires a
 // factory with a per-queue map instead.
 type gitMergerFactory struct {
 	merger merger.Merger
@@ -380,20 +382,26 @@ func (f *gitMergerFactory) For(_ merger.Config) (merger.Merger, error) {
 	return f.merger, nil
 }
 
-type noopMergerFactory struct{}
+// noopMergerFactory builds a noop merger per queue, bound to that queue's
+// config. The synthetic revision-id counter is held here rather than on the
+// merger so ids stay unique across every queue in the process.
+type noopMergerFactory struct {
+	seq *atomic.Uint64
+}
 
-func (f *noopMergerFactory) For(_ merger.Config) (merger.Merger, error) {
-	return noop.New(), nil
+func (f *noopMergerFactory) For(cfg merger.Config) (merger.Merger, error) {
+	return noop.New(cfg, f.seq), nil
 }
 
-// fakeMergerFactory shares one fake merger across queues so the synthetic
-// revision ids it mints stay unique for the lifetime of the process.
+// fakeMergerFactory builds a fake merger per queue, bound to that queue's
+// config. As with noop, the revision-id counter lives on the factory so ids
+// stay unique for the lifetime of the process.
 type fakeMergerFactory struct {
-	merger merger.Merger
+	seq *atomic.Uint64
 }
 
-func (f *fakeMergerFactory) For(_ merger.Config) (merger.Merger, error) {
-	return f.merger, nil
+func (f *fakeMergerFactory) For(cfg merger.Config) (merger.Merger, error) {
+	return fake.New(cfg, f.seq), nil
 }
 
 // parseStrategy maps the MERGE_DEFAULT_STRATEGY env value to a concrete merge

service/stovepipe/server/main.go

@@ -133,16 +133,16 @@ func (f *inMemoryCounterFactory) For(config counter.Config) (counter.Counter, er
 type fakeSourceControlFactory struct{}
 
 func (fakeSourceControlFactory) For(cfg sourcecontrol.Config) (sourcecontrol.SourceControl, error) {
-	return sourcecontrolfake.New([]string{fmt.Sprintf("git://%s/HEAD", cfg.QueueName)}), nil
+	return sourcecontrolfake.New(cfg, []string{fmt.Sprintf("git://%s/HEAD", cfg.QueueName)}), nil
 }
 
-// fakeBuildRunnerFactory is the example BuildRunner factory: every queue shares the same
-// stateless fake runner, which succeeds unless a caller embeds a failure marker in the head
-// URI. A real deployment supplies a backend-specific factory (e.g. Buildkite, per queue).
+// fakeBuildRunnerFactory is the example BuildRunner factory: every queue gets a stateless fake
+// runner bound to its own config, which succeeds unless a caller embeds a failure marker in the
+// head URI. A real deployment supplies a backend-specific factory (e.g. Buildkite, per queue).
 type fakeBuildRunnerFactory struct{}
 
-func (fakeBuildRunnerFactory) For(_ buildrunner.Config) (buildrunner.BuildRunner, error) {
-	return buildrunnerfake.New(), nil
+func (fakeBuildRunnerFactory) For(cfg buildrunner.Config) (buildrunner.BuildRunner, error) {
+	return buildrunnerfake.New(cfg), nil
 }
 
 func main() {

service/submitqueue/orchestrator/server/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@rules_go//go:def.bzl", "go_binary", "go_cross_binary", "go_library")
+load("@rules_go//go:def.bzl", "go_binary", "go_cross_binary", "go_library", "go_test")
 
 exports_files(
     ["docker-compose.yml"],
@@ -49,6 +49,7 @@ go_library(
         "//submitqueue/extension/speculation/speculator/standard:go_default_library",
         "//submitqueue/extension/storage:go_default_library",
         "//submitqueue/extension/storage/mysql:go_default_library",
+        "//submitqueue/extension/validator:go_default_library",
         "//submitqueue/extension/validator/fake:go_default_library",
         "//submitqueue/orchestrator:go_default_library",
         "@com_github_go_sql_driver_mysql//:go_default_library",
@@ -87,3 +88,19 @@ filegroup(
     ],
     visibility = ["//test:__subpackages__"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["profiles_test.go"],
+    embed = [":orchestrator_lib"],  # keep
+    deps = [
+        "//submitqueue/extension/buildrunner:go_default_library",
+        "//submitqueue/extension/changeprovider:go_default_library",
+        "//submitqueue/extension/conflict:go_default_library",
+        "//submitqueue/extension/scorer:go_default_library",
+        "//submitqueue/extension/speculation/speculator:go_default_library",
+        "//submitqueue/extension/storage:go_default_library",
+        "@com_github_stretchr_testify//assert:go_default_library",
+        "@com_github_stretchr_testify//require:go_default_library",
+    ],
+)