feat(validator): add a GitHub validator and select each queue's validator in the orchestrator profiles (#878)

## Why?

SubmitQueue only rejects a GitHub pull request at validation for what
the merger's mergeability check catches: closed, draft, conflicting,
wrong stack shape or target. A pull request that is missing a required
review or has a failing or still-running required check is accepted,
batched and built. It then fails at land time, when GitHub refuses the
merge, after holding up the queue.

Every queue in the example orchestrator also gets the always-passing
fake validator, with no way to choose a real one per queue.

## What?

**`submitqueue/extension/validator/github`**: a `validator.Validator`
that asks GitHub whether each pull request could merge under the
repository's own branch protection or rulesets. Nothing is restated in
SubmitQueue config. For each `github://` URI, bottom of the stack first,
it reads the pull request in one GraphQL call and rejects at the first
rule it breaks:

| Rule | Reason |
|---|---|
| URI names the configured host | `host` |
| Pull request head is still the URI's commit | `stale` |
| Review decision is not `CHANGES_REQUESTED` / `REVIEW_REQUIRED` |
`not_approved` |
| Every required check passed (success, neutral and skipped count as
passing; a check name reported more than once must pass every time) |
`check_failed` / `check_pending` |
| Merge state is not `BLOCKED` (catches what the rules above can't see,
e.g. a required check that hasn't reported yet) | `blocked` |

- **Pass when undecided:** anything GitHub hasn't decided (`UNKNOWN`
merge state, no review rule) passes. GitHub enforces the same rules
again when it merges.
- **No duplicated checks:** open, draft, conflict and stack shape stay
with the merger's mergeability check. A missing pull request stays with
the change provider.
- **Composes with other providers:** URIs that aren't GitHub change IDs
are skipped, so it combines with other change providers' validators via
`composite`.
- **Credential and base URL come from the caller:** they arrive on the
injected `*http.Client`, the same contract as the GitHub change provider
and merger.
- **Errors:** a rejection is an error naming the pull request and rule.
A failure to read GitHub is returned wrapped, keeping
`platform/http.StatusError` in the chain for `platform/errs/http`.
Neither is classified here, as for any extension.
- **Metrics:** tagged with the queue. `validate.finish` counts only read
failures as errors. `validate.rejections` is also tagged by reason.

**Orchestrator wiring** (`service/submitqueue/orchestrator/server`):

- Profiles gain a per-queue `validator:` block, `{type: fake|github}`
with `tokenEnv`, `baseUrl`, `host` and `timeout`. These default to
`GITHUB_TOKEN`, `https://api.github.com` and `github.com`.
- The default is `fake`, so existing queues and the built-in example
setup are unchanged.
- This replaces the hard-coded `validatorFactory` in `main.go`.
- A `github` validator whose token is unset fails at startup.

## Test Plan

```sh
bazel test //submitqueue/extension/validator/... //service/submitqueue/orchestrator/server:all
make lint check-gazelle check-mocks check-tidy
```



## Issue
<!-- 
Link the issue here. 
- Use 'Closes #123' if this is the final fix. 
- Use 'Part of #123' or just '#123' if the feature is still in progress.
-->

Author: roychying

service/submitqueue/demo/provider/README.md

@@ -4,7 +4,7 @@ Each directory here is one **provider** — a code-hosting system SubmitQueue la
 
 | File | Selects |
 |---|---|
-| `profiles.yaml` | the change provider, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
+| `profiles.yaml` | the change provider, validator, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
 | `merge.yaml` | the merge target each queue lands on (read by Runway) |
 
 Neither holds a secret. Each integration names the *environment variable* carrying its credential, so these files stay committable and rotating a token needs no edit.

service/submitqueue/orchestrator/server/BUILD.bazel

@@ -68,6 +68,7 @@ go_library(
         "//submitqueue/extension/speculation/speculator/standard:go_default_library",
         "//submitqueue/extension/validator:go_default_library",
         "//submitqueue/extension/validator/fake:go_default_library",
+        "//submitqueue/extension/validator/github:go_default_library",
         "//submitqueue/orchestrator:go_default_library",
         "//submitqueue/orchestrator/extension/storage:go_default_library",
         "//submitqueue/orchestrator/extension/storage/mysql:go_default_library",
@@ -144,6 +145,7 @@ go_test(
         "//submitqueue/extension/queuepause/fake:go_default_library",
         "//submitqueue/extension/speculation/scorer:go_default_library",
         "//submitqueue/extension/speculation/speculator:go_default_library",
+        "//submitqueue/extension/validator:go_default_library",
         "//submitqueue/orchestrator/extension/storage:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",

service/submitqueue/orchestrator/server/config.go

@@ -40,6 +40,12 @@ const (
 	buildRunnerTypeBuildkite     = "buildkite"
 )
 
+// Validator types selectable from configuration.
+const (
+	validatorTypeFake   = "fake"
+	validatorTypeGitHub = "github"
+)
+
 // Conflict analyzer types selectable from configuration.
 const (
 	analyzerTypeAll         = "all"
@@ -97,6 +103,7 @@ const (
 	defaultGitTokenUser   = "x-access-token"
 	defaultGitHubTokenEnv = "GITHUB_TOKEN"
 	defaultGitHubBaseURL  = "https://api.github.com"
+	defaultGitHubHost     = "github.com"
 	defaultPhabTokenEnv   = "PHAB_API_TOKEN"
 	defaultHTTPTimeout    = 30 * time.Second
 )
@@ -122,6 +129,7 @@ type profilesConfig struct {
 type namedQueueProfileConfig struct {
 	Name           string                `yaml:"name"`
 	ChangeProvider *changeProviderConfig `yaml:"changeProvider"`
+	Validator      *validatorConfig      `yaml:"validator"`
 	BuildRunner    *buildRunnerConfig    `yaml:"buildRunner"`
 	Analyzer       *analyzerConfig       `yaml:"analyzer"`
 	Scorer         *scorerConfig         `yaml:"scorer"`
@@ -131,6 +139,7 @@ type namedQueueProfileConfig struct {
 // queueProfileConfig is the full set of extensions a queue resolves to.
 type queueProfileConfig struct {
 	ChangeProvider changeProviderConfig `yaml:"changeProvider"`
+	Validator      validatorConfig      `yaml:"validator"`
 	BuildRunner    buildRunnerConfig    `yaml:"buildRunner"`
 	Analyzer       analyzerConfig       `yaml:"analyzer"`
 	Scorer         scorerConfig         `yaml:"scorer"`
@@ -191,6 +200,19 @@ type phabProviderConfig struct {
 	Timeout  string `yaml:"timeout"`
 }
 
+// validatorConfig selects the custom validation a request passes before it is
+// batched.
+type validatorConfig struct {
+	Type string `yaml:"type"`
+	// TokenEnv names the environment variable holding the API token (github only).
+	TokenEnv string `yaml:"tokenEnv"`
+	// BaseURL is the GitHub API root, for GitHub Enterprise (github only).
+	BaseURL string `yaml:"baseUrl"`
+	// Host is the GitHub instance change URIs must name (github only).
+	Host    string `yaml:"host"`
+	Timeout string `yaml:"timeout"`
+}
+
 // buildRunnerConfig selects how a batch's build is triggered and polled.
 type buildRunnerConfig struct {
 	Type string `yaml:"type"`
@@ -304,6 +326,11 @@ func (c *profilesConfig) normalizeAndValidate() error {
 				return err
 			}
 		}
+		if q.Validator != nil {
+			if err := q.Validator.normalizeAndValidate(where); err != nil {
+				return err
+			}
+		}
 		if q.BuildRunner != nil {
 			if err := q.BuildRunner.normalizeAndValidate(where); err != nil {
 				return err
@@ -370,6 +397,9 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig {
 	if q.ChangeProvider != nil {
 		profile.ChangeProvider = *q.ChangeProvider
 	}
+	if q.Validator != nil {
+		profile.Validator = *q.Validator
+	}
 	if q.BuildRunner != nil {
 		profile.BuildRunner = *q.BuildRunner
 	}
@@ -411,6 +441,9 @@ func (p *queueProfileConfig) normalizeAndValidate(where string) error {
 	if err := p.ChangeProvider.normalizeAndValidate(where); err != nil {
 		return err
 	}
+	if err := p.Validator.normalizeAndValidate(where); err != nil {
+		return err
+	}
 	if err := p.BuildRunner.normalizeAndValidate(where); err != nil {
 		return err
 	}
@@ -511,6 +544,29 @@ func (c *changeProviderConfig) ensurePhabricator(where string) error {
 	return nil
 }
 
+func (v *validatorConfig) normalizeAndValidate(where string) error {
+	if v.Type == "" {
+		v.Type = validatorTypeFake
+	}
+	switch v.Type {
+	case validatorTypeFake:
+		return nil
+	case validatorTypeGitHub:
+		if v.TokenEnv == "" {
+			v.TokenEnv = defaultGitHubTokenEnv
+		}
+		if v.BaseURL == "" {
+			v.BaseURL = defaultGitHubBaseURL
+		}
+		if v.Host == "" {
+			v.Host = defaultGitHubHost
+		}
+		return nil
+	default:
+		return fmt.Errorf("%s: unknown validator type %q", where, v.Type)
+	}
+}
+
 func (b *buildRunnerConfig) normalizeAndValidate(where string) error {
 	if b.Type == "" {
 		b.Type = buildRunnerTypeFake

service/submitqueue/orchestrator/server/config_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
 	"github.com/uber/submitqueue/submitqueue/extension/conflict"
 	"github.com/uber/submitqueue/submitqueue/extension/speculation/speculator"
+	"github.com/uber/submitqueue/submitqueue/extension/validator"
 )
 
 func TestValidateProfileQueueTenants(t *testing.T) {
@@ -122,13 +123,15 @@ queues:
 	resolved := cfg.resolve(cfg.Queues[0])
 	assert.Equal(t, analyzerTypeNone, resolved.Analyzer.Type)
 	assert.Equal(t, changeProviderTypeFake, resolved.ChangeProvider.Type)
+	assert.Equal(t, validatorTypeFake, resolved.Validator.Type)
 	assert.Equal(t, buildRunnerTypeFake, resolved.BuildRunner.Type)
 }
 
 func TestLoadProfilesConfig_AppliesForgeDefaults(t *testing.T) {
 	path := writeProfiles(t, `
 defaults:
   changeProvider: {type: github}
+  validator: {type: github}
   buildRunner: {type: githubactions, owner: uber, repo: sq-sandbox, workflow: validate.yml}
 `)
 
@@ -138,6 +141,9 @@ defaults:
 	require.NotNil(t, cfg.Defaults.ChangeProvider.GitHub)
 	assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.ChangeProvider.GitHub.TokenEnv)
 	assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.ChangeProvider.GitHub.BaseURL)
+	assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.Validator.TokenEnv)
+	assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.Validator.BaseURL)
+	assert.Equal(t, defaultGitHubHost, cfg.Defaults.Validator.Host)
 	assert.Equal(t, defaultGitHubTokenEnv, cfg.Defaults.BuildRunner.TokenEnv)
 	assert.Equal(t, defaultGitHubBaseURL, cfg.Defaults.BuildRunner.BaseURL)
 }
@@ -146,6 +152,7 @@ func TestLoadProfilesConfig_EmptyFileIsAllFakes(t *testing.T) {
 	cfg, err := loadProfilesConfig(writeProfiles(t, ""))
 	require.NoError(t, err)
 	assert.Equal(t, changeProviderTypeFake, cfg.Defaults.ChangeProvider.Type)
+	assert.Equal(t, validatorTypeFake, cfg.Defaults.Validator.Type)
 	assert.Equal(t, buildRunnerTypeFake, cfg.Defaults.BuildRunner.Type)
 	assert.Equal(t, analyzerTypeAll, cfg.Defaults.Analyzer.Type)
 }
@@ -170,6 +177,14 @@ func TestLoadProfilesConfig_Rejects(t *testing.T) {
 			name:     "unknown change provider",
 			contents: "defaults:\n  changeProvider: {type: gitlab}\n",
 		},
+		{
+			name:     "unknown validator",
+			contents: "defaults:\n  validator: {type: lint}\n",
+		},
+		{
+			name:     "unknown validator on a queue",
+			contents: "queues:\n  - name: a\n    validator: {type: lint}\n",
+		},
 		{
 			name:     "unknown build runner",
 			contents: "defaults:\n  buildRunner: {type: jenkins}\n",
@@ -308,18 +323,70 @@ func TestNewProfiles_ResolvesPerQueueAnalyzers(t *testing.T) {
 func TestNewProfiles_FailsWhenAForgeTokenIsMissing(t *testing.T) {
 	// Failing at startup beats accepting land requests and discovering the
 	// credential gap one API call into the first merge.
-	t.Setenv("TEST_GH_TOKEN_UNSET", "")
+	tests := []struct {
+		name     string
+		contents string
+	}{
+		{
+			name:     "github change provider",
+			contents: "defaults:\n  changeProvider:\n    type: github\n    github: {tokenEnv: TEST_GH_TOKEN_UNSET}\n",
+		},
+		{
+			name:     "github validator",
+			contents: "queues:\n  - name: gh\n    validator: {type: github, tokenEnv: TEST_GH_TOKEN_UNSET}\n",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv("TEST_GH_TOKEN_UNSET", "")
+			cfg, err := loadProfilesConfig(writeProfiles(t, tt.contents))
+			require.NoError(t, err)
+
+			_, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
+			require.Error(t, err)
+		})
+	}
+}
+
+func TestNewProfiles_ResolvesPerQueueValidators(t *testing.T) {
+	// A change URI on another GitHub host is one the GitHub validator rejects
+	// without calling GitHub, so the two types differ observably offline.
+	t.Setenv("TEST_GH_TOKEN", "token")
 	path := writeProfiles(t, `
-defaults:
-  changeProvider:
-    type: github
-    github: {tokenEnv: TEST_GH_TOKEN_UNSET}
+queues:
+  - name: gh
+    validator: {type: github, tokenEnv: TEST_GH_TOKEN}
 `)
 	cfg, err := loadProfilesConfig(path)
 	require.NoError(t, err)
+	profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
+	require.NoError(t, err)
 
-	_, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
-	require.Error(t, err)
+	foreignHost := entity.Request{Change: change.Change{URIs: []string{
+		"github://github.example.com/acme/widgets/pull/1/" + strings.Repeat("a", 40),
+	}}}
+
+	tests := []struct {
+		queue      string
+		wantReject bool
+	}{
+		{queue: "gh", wantReject: true},
+		{queue: "unlisted-falls-back-to-fake"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.queue, func(t *testing.T) {
+			v, err := profiles.ValidatorFactory().For(validator.Config{QueueName: tt.queue})
+			require.NoError(t, err)
+
+			err = v.Validate(context.Background(), foreignHost)
+			if tt.wantReject {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
 }
 
 func TestNewProfiles_RoutingFallsBackToFakeWithoutTokens(t *testing.T) {

service/submitqueue/orchestrator/server/main.go

@@ -50,8 +50,6 @@ import (
 	servicemq "github.com/uber/submitqueue/service/messagequeue"
 	"github.com/uber/submitqueue/service/submitqueue/core"
 	"github.com/uber/submitqueue/submitqueue/core/changeset"
-	"github.com/uber/submitqueue/submitqueue/extension/validator"
-	validatorfake "github.com/uber/submitqueue/submitqueue/extension/validator/fake"
 	"github.com/uber/submitqueue/submitqueue/orchestrator"
 	mysqlstorage "github.com/uber/submitqueue/submitqueue/orchestrator/extension/storage/mysql"
 	"go.uber.org/zap"
@@ -220,7 +218,7 @@ func run() error {
 		ChangeProvider: profiles.ChangeProviderFactory(),
 		Analyzer:       profiles.AnalyzerFactory(),
 		Speculator:     profiles.SpeculatorFactory(),
-		Validator:      validatorFactory{},
+		Validator:      profiles.ValidatorFactory(),
 		Hooks:          hookResolver{},
 	}
 
@@ -492,14 +490,3 @@ type hookResolver struct{}
 func (hookResolver) For(*basehook.HookEvent) []hookext.Hook {
 	return []hookext.Hook{hooknoop.New()}
 }
-
-// validatorFactory routes every queue to the always-passing fake validator.
-// Choosing an impl per queue is host policy, so the adapter lives here rather
-// than in the extension package. A deployment with real validation swaps this
-// for a routing adapter.
-type validatorFactory struct{}
-
-// For returns the Validator bound to the queue named in config.
-func (validatorFactory) For(config validator.Config) (validator.Validator, error) {
-	return validatorfake.New(config), nil
-}