From 80a785bce788447add66753948e54a933689af1e Mon Sep 17 00:00:00 2001 From: Chenghan Ying <125171961+roychying@users.noreply.github.com> Date: Fri, 16 Oct 2026 11:21:41 -0700 Subject: [PATCH] feat(submitqueue): add a GitHub validator and select each queue's validator in the orchestrator profiles --- service/submitqueue/demo/provider/README.md | 2 +- .../orchestrator/server/BUILD.bazel | 2 + .../submitqueue/orchestrator/server/config.go | 56 +++ .../orchestrator/server/config_test.go | 81 +++- .../submitqueue/orchestrator/server/main.go | 15 +- .../orchestrator/server/profiles.go | 63 ++- .../orchestrator/server/profiles_test.go | 9 + .../extension/validator/github/BUILD.bazel | 41 ++ .../extension/validator/github/README.md | 44 +++ .../extension/validator/github/evaluate.go | 139 +++++++ .../validator/github/evaluate_test.go | 280 ++++++++++++++ .../extension/validator/github/graphql.go | 202 ++++++++++ .../extension/validator/github/validator.go | 132 +++++++ .../validator/github/validator_test.go | 363 ++++++++++++++++++ 14 files changed, 1405 insertions(+), 24 deletions(-) create mode 100644 submitqueue/extension/validator/github/BUILD.bazel create mode 100644 submitqueue/extension/validator/github/README.md create mode 100644 submitqueue/extension/validator/github/evaluate.go create mode 100644 submitqueue/extension/validator/github/evaluate_test.go create mode 100644 submitqueue/extension/validator/github/graphql.go create mode 100644 submitqueue/extension/validator/github/validator.go create mode 100644 submitqueue/extension/validator/github/validator_test.go diff --git a/service/submitqueue/demo/provider/README.md b/service/submitqueue/demo/provider/README.md index 19a039068..cbfab70b8 100644 --- a/service/submitqueue/demo/provider/README.md +++ b/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. diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 751d844e7..f6a1cc452 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/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", diff --git a/service/submitqueue/orchestrator/server/config.go b/service/submitqueue/orchestrator/server/config.go index af891cfe8..5b95832c6 100644 --- a/service/submitqueue/orchestrator/server/config.go +++ b/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 diff --git a/service/submitqueue/orchestrator/server/config_test.go b/service/submitqueue/orchestrator/server/config_test.go index 45b923306..64ca8b487 100644 --- a/service/submitqueue/orchestrator/server/config_test.go +++ b/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,6 +123,7 @@ 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) } @@ -129,6 +131,7 @@ 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) { diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index 8203de747..3d3df9ebe 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/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 -} diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index 83c1ec3a5..24cfd4903 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -56,6 +56,9 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer/heuristic" "github.com/uber/submitqueue/submitqueue/extension/speculation/speculator" specstandard "github.com/uber/submitqueue/submitqueue/extension/speculation/speculator/standard" + "github.com/uber/submitqueue/submitqueue/extension/validator" + validatorfake "github.com/uber/submitqueue/submitqueue/extension/validator/fake" + githubvalidator "github.com/uber/submitqueue/submitqueue/extension/validator/github" ) // Profile holds the per-queue extension implementations. Grouping them per @@ -66,6 +69,10 @@ type Profile struct { // ChangeProvider resolves change metadata for requests in this queue. ChangeProvider changeprovider.Factory + // Validator runs custom validation on requests in this queue before they + // are batched. + Validator validator.Factory + // BuildRunner triggers and polls builds for batches in this queue. BuildRunner buildrunner.Factory @@ -112,6 +119,14 @@ func (p Profiles) ChangeProviderFactory() changeprovider.Factory { }) } +// ValidatorFactory returns a validator.Factory that resolves the Validator for +// each queue from the profile registry. +func (p Profiles) ValidatorFactory() validator.Factory { + return validatorFunc(func(c validator.Config) (validator.Validator, error) { + return p.For(c.QueueName).Validator.For(c) + }) +} + // BuildRunnerFactory returns a buildrunner.Factory that resolves the // BuildRunner for each queue from the profile registry. func (p Profiles) BuildRunnerFactory() buildrunner.Factory { @@ -162,6 +177,10 @@ func (f changeProviderFunc) For(c changeprovider.Config) (changeprovider.ChangeP return f(c) } +type validatorFunc func(validator.Config) (validator.Validator, error) + +func (f validatorFunc) For(c validator.Config) (validator.Validator, error) { return f(c) } + type buildRunnerFunc func(buildrunner.Config) (buildrunner.BuildRunner, error) func (f buildRunnerFunc) For(c buildrunner.Config) (buildrunner.BuildRunner, error) { return f(c) } @@ -221,6 +240,7 @@ func newProfiles( logger.Info("extension profiles built", zap.String("default_change_provider", cfg.Defaults.ChangeProvider.Type), + zap.String("default_validator", cfg.Defaults.Validator.Type), zap.String("default_build_runner", cfg.Defaults.BuildRunner.Type), zap.String("default_analyzer", cfg.Defaults.Analyzer.Type), zap.String("default_scorer", cfg.Defaults.Scorer.Type), @@ -255,6 +275,10 @@ func (b *profileBuilder) build(cfg queueProfileConfig, where string) (Profile, e if err != nil { return Profile{}, err } + val, err := reuse(b, cfg.Validator, where, b.newValidatorFactory) + if err != nil { + return Profile{}, err + } runner, err := reuse(b, cfg.BuildRunner, where, b.newBuildRunnerFactory) if err != nil { return Profile{}, err @@ -269,6 +293,7 @@ func (b *profileBuilder) build(cfg queueProfileConfig, where string) (Profile, e } return withSpeculator(Profile{ ChangeProvider: provider, + Validator: val, BuildRunner: runner, Analyzer: analyzer, Storage: b.stores, @@ -554,6 +579,40 @@ func (b *profileBuilder) newPhabChangeProvider(cfg phabProviderConfig, where str }, nil } +// newValidatorFactory builds the configured validator's factory. The HTTP client +// is built once here; only the per-queue validator is built per call. +func (b *profileBuilder) newValidatorFactory(cfg validatorConfig, where string) (validator.Factory, error) { + switch cfg.Type { + case validatorTypeFake: + return validatorFunc(func(c validator.Config) (validator.Validator, error) { + return validatorfake.New(c), nil + }), nil + + case validatorTypeGitHub: + token, ok := tokenFrom(cfg.TokenEnv) + if !ok { + return nil, fmt.Errorf("%s: github validator needs %s to be set", where, cfg.TokenEnv) + } + client, err := b.githubClient(cfg.BaseURL, cfg.Timeout, token, where) + if err != nil { + return nil, err + } + scope := b.scope.SubScope("validator.github") + return validatorFunc(func(c validator.Config) (validator.Validator, error) { + return githubvalidator.New(githubvalidator.Params{ + Config: c, + HTTPClient: client, + Host: cfg.Host, + Logger: b.logger.Sugar(), + MetricsScope: scope, + }) + }), nil + + default: + return nil, fmt.Errorf("%s: unknown validator type %q", where, cfg.Type) + } +} + // newBuildRunnerFactory builds the configured build runner's factory. The HTTP // client is built once here; only the per-queue runner is built per call. func (b *profileBuilder) newBuildRunnerFactory(cfg buildRunnerConfig, where string) (buildrunner.Factory, error) { @@ -658,8 +717,8 @@ func pathOverlapKey(by string) pathoverlap.PathKey { } // githubClient builds an HTTP client rooted at a GitHub API and authenticated -// with the given token. Shared by the change provider and the Actions runner, -// which differ only in which endpoints they call. +// with the given token. Shared by the change provider, the validator and the +// Actions runner, which differ only in which endpoints they call. func (b *profileBuilder) githubClient(baseURL, timeout, token, where string) (*nethttp.Client, error) { client, err := http.NewClient(baseURL) if err != nil { diff --git a/service/submitqueue/orchestrator/server/profiles_test.go b/service/submitqueue/orchestrator/server/profiles_test.go index 1e83b93c0..f1c7da48b 100644 --- a/service/submitqueue/orchestrator/server/profiles_test.go +++ b/service/submitqueue/orchestrator/server/profiles_test.go @@ -30,12 +30,14 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/conflict" "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer" "github.com/uber/submitqueue/submitqueue/extension/speculation/speculator" + "github.com/uber/submitqueue/submitqueue/extension/validator" ) // recorder captures the queue name each seam's factory was handed, so a test // can assert the Config survived the profile lookup instead of stopping at it. type recorder struct { changeProvider string + validator string buildRunner string analyzer string storage string @@ -59,6 +61,10 @@ func profileRecording(rec *recorder) Profile { rec.changeProvider = c.QueueName return nil, nil }), + Validator: validatorFunc(func(c validator.Config) (validator.Validator, error) { + rec.validator = c.QueueName + return nil, nil + }), BuildRunner: buildRunnerFunc(func(c buildrunner.Config) (buildrunner.BuildRunner, error) { rec.buildRunner = c.QueueName return nil, nil @@ -106,6 +112,8 @@ func TestProfilesForwardQueueNameToFactories(t *testing.T) { _, err := profiles.ChangeProviderFactory().For(changeprovider.Config{QueueName: tt.queue}) require.NoError(t, err) + _, err = profiles.ValidatorFactory().For(validator.Config{QueueName: tt.queue}) + require.NoError(t, err) _, err = profiles.BuildRunnerFactory().For(buildrunner.Config{QueueName: tt.queue}) require.NoError(t, err) _, err = profiles.AnalyzerFactory().For(conflict.Config{QueueName: tt.queue}) @@ -116,6 +124,7 @@ func TestProfilesForwardQueueNameToFactories(t *testing.T) { require.NoError(t, err) assert.Equal(t, tt.queue, rec.changeProvider) + assert.Equal(t, tt.queue, rec.validator) assert.Equal(t, tt.queue, rec.buildRunner) assert.Equal(t, tt.queue, rec.analyzer) assert.Equal(t, tt.queue, rec.storage) diff --git a/submitqueue/extension/validator/github/BUILD.bazel b/submitqueue/extension/validator/github/BUILD.bazel new file mode 100644 index 000000000..aee86fcef --- /dev/null +++ b/submitqueue/extension/validator/github/BUILD.bazel @@ -0,0 +1,41 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "evaluate.go", + "graphql.go", + "validator.go", + ], + importpath = "github.com/uber/submitqueue/submitqueue/extension/validator/github", + visibility = ["//visibility:public"], + deps = [ + "//platform/base/change/github:go_default_library", + "//platform/http:go_default_library", + "//platform/metrics:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_zap//:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = [ + "evaluate_test.go", + "validator_test.go", + ], + embed = [":go_default_library"], + deps = [ + "//platform/base/change:go_default_library", + "//platform/base/change/github:go_default_library", + "//platform/http:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_zap//zaptest:go_default_library", + ], +) diff --git a/submitqueue/extension/validator/github/README.md b/submitqueue/extension/validator/github/README.md new file mode 100644 index 000000000..cca9df8fc --- /dev/null +++ b/submitqueue/extension/validator/github/README.md @@ -0,0 +1,44 @@ +# github validator + +A `validator.Validator` that rejects a request whose GitHub pull requests the repository would not let merge yet. It asks GitHub rather than restating policy: the rules come from the repository's branch protection or rulesets, so a repository changes what SubmitQueue enforces by changing its own settings. It is constructed by the wiring layer with an HTTP client and the GitHub host the client talks to; the example orchestrator selects it per queue with `validator: {type: github}` in its extension profiles (see [`service/submitqueue/orchestrator/server`](../../../../service/submitqueue/orchestrator/server)). + +## Rules + +For each `github://` URI in the request, bottom of the stack first, the validator reads the pull request from GitHub's GraphQL API and rejects at the first rule it breaks: + +| Rule | Rejection reason | +|---|---| +| The URI names the configured host | `host` | +| The pull request's head is still the commit the URI names | `stale` | +| GitHub's review decision is not `CHANGES_REQUESTED` or `REVIEW_REQUIRED` | `not_approved` | +| Every required check on the head commit passed; success, neutral and skipped count as passing | `check_failed`, `check_pending` | +| GitHub's merge state is not `BLOCKED` | `blocked` | + +A check is required when GitHub reports it as required for that pull request, which covers both branch protection and rulesets. One check name can report more than once, for example from two workflows; every report of it must pass, so a failing one names the check rather than surfacing only as `blocked`. + +The `blocked` rule comes last because it explains nothing. It catches what the rules above cannot see: a required check that has not reported yet, a required conversation resolution, required signatures, or a required check past the first 100 the validator reads. + +Whatever GitHub has not decided passes. A merge state GitHub is still computing (`UNKNOWN`) or a repository with no review rule rejects nothing. GitHub enforces the same rules again when the pull request is merged, so a miss here fails later at the merge rather than landing. + +## What it leaves to other stages + +- **Open, draft, conflicting, stack shape, target branch**: the merger's mergeability check rejects these before batching, and runs again before merging. +- **The pull request exists and its head has not moved**: the change provider fails the request before validators run. The validator still compares heads, because its own verdict must be about the commit the URI names. + +Each pull request is judged by GitHub's verdict for that pull request, which applies the rules of its own base branch. In a stack, that base is the branch below it rather than the trunk. + +## Composition + +URIs that are not GitHub change IDs are skipped, so the validator composes with validators for other change providers (see [`composite`](../composite)). + +## Failures + +A rejection is an error naming the pull request and the rule it breaks. A failure to read GitHub — a transport error, an HTTP status, a GraphQL error such as a pull request that does not exist — is returned wrapped, and an HTTP status stays in the chain as `platform/http.StatusError` so `platform/errs/http` can classify it. Neither is classified here, as for any extension, and the validate stage currently rejects the request on either. + +## Auth and hosts + +The validator adds no credential and no base URL. Its HTTP client's transport roots relative paths at the API (for example `platform/http.NewClient("https://api.github.com")`) and authenticates them. Reading a public repository needs only an authenticated request; a private one needs read access to pull requests, checks and commit statuses. GitHub Enterprise Server works by pointing the client at its API and setting the host. + +## Metrics + +Under the `github_validator` scope, tagged with the `queue`: `validate.start` and the `validate.finish` latency histogram, whose `result` tag is `error` only for a failure to read GitHub, and `validate.rejections` tagged with the rejection `reason`. diff --git a/submitqueue/extension/validator/github/evaluate.go b/submitqueue/extension/validator/github/evaluate.go new file mode 100644 index 000000000..427d95a3d --- /dev/null +++ b/submitqueue/extension/validator/github/evaluate.go @@ -0,0 +1,139 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package github + +import ( + "fmt" + "sort" + "strings" + + entitygithub "github.com/uber/submitqueue/platform/base/change/github" +) + +// rejectionReason names why a pull request was rejected. The values are metric +// tag values, so they stay stable. +type rejectionReason string + +const ( + reasonHost rejectionReason = "host" + reasonStale rejectionReason = "stale" + reasonNotApproved rejectionReason = "not_approved" + reasonCheckFailed rejectionReason = "check_failed" + reasonCheckPending rejectionReason = "check_pending" + reasonBlocked rejectionReason = "blocked" +) + +// GitHub's reviewDecision and mergeStateStatus values the rules act on. +const ( + reviewDecisionChangesRequested = "CHANGES_REQUESTED" + reviewDecisionReviewRequired = "REVIEW_REQUIRED" + mergeStateStatusBlocked = "BLOCKED" +) + +// rejection is why one pull request in a request cannot land. +type rejection struct { + uri string + reason rejectionReason + detail string +} + +func (r rejection) Error() string { + return fmt.Sprintf("%s: %s", r.uri, r.detail) +} + +// checkOutcome is ordered worst to best; requiredChecksNotPassed relies on it. +type checkOutcome int + +const ( + checkFailed checkOutcome = iota + checkPending + checkPassed +) + +// check is one status check reported on a pull request's head commit. +type check struct { + name string + required bool + outcome checkOutcome +} + +// pullRequestState is what GitHub reports about a pull request, reduced to +// what evaluatePullRequest reads. +type pullRequestState struct { + headSHA string + reviewDecision string + mergeStateStatus string + checks []check +} + +// evaluatePullRequest reports the first rule pr breaks. Whatever GitHub has not +// decided passes: an UNKNOWN merge state or an absent review rule rejects nothing. +func evaluatePullRequest(cid entitygithub.ChangeID, pr pullRequestState) (rejection, bool) { + reject := func(reason rejectionReason, format string, args ...any) (rejection, bool) { + return rejection{uri: cid.String(), reason: reason, detail: fmt.Sprintf(format, args...)}, true + } + + if pr.headSHA != cid.HeadCommitSHA { + return reject(reasonStale, "head moved from %s to %s", cid.HeadCommitSHA, pr.headSHA) + } + + switch pr.reviewDecision { + case reviewDecisionChangesRequested: + return reject(reasonNotApproved, "changes requested") + case reviewDecisionReviewRequired: + return reject(reasonNotApproved, "approving review required") + } + + failed, pending := requiredChecksNotPassed(pr.checks) + if len(failed) > 0 { + return reject(reasonCheckFailed, "required checks failed: %s", strings.Join(failed, ", ")) + } + if len(pending) > 0 { + return reject(reasonCheckPending, "required checks not finished: %s", strings.Join(pending, ", ")) + } + + // Last, because it explains nothing: it catches what the rules above + // cannot see, such as a required check that has not reported yet. + if pr.mergeStateStatus == mergeStateStatusBlocked { + return reject(reasonBlocked, "GitHub reports merging as blocked") + } + + return rejection{}, false +} + +// requiredChecksNotPassed returns sorted names. A name reported more than once, +// such as by two workflows, takes its worst outcome: every report must pass. +func requiredChecksNotPassed(checks []check) (failed, pending []string) { + worst := make(map[string]checkOutcome) + for _, c := range checks { + if !c.required { + continue + } + if outcome, seen := worst[c.name]; !seen || c.outcome < outcome { + worst[c.name] = c.outcome + } + } + for name, outcome := range worst { + switch outcome { + case checkFailed: + failed = append(failed, name) + case checkPending: + pending = append(pending, name) + } + } + sort.Strings(failed) + sort.Strings(pending) + return failed, pending +} diff --git a/submitqueue/extension/validator/github/evaluate_test.go b/submitqueue/extension/validator/github/evaluate_test.go new file mode 100644 index 000000000..8816531bf --- /dev/null +++ b/submitqueue/extension/validator/github/evaluate_test.go @@ -0,0 +1,280 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package github + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + entitygithub "github.com/uber/submitqueue/platform/base/change/github" +) + +var ( + headSHA = strings.Repeat("a", 40) + movedSHA = strings.Repeat("b", 40) +) + +func TestEvaluatePullRequest(t *testing.T) { + cid := entitygithub.ChangeID{Scheme: "github", Host: "github.com", Org: "acme", Repo: "widgets", PRNumber: 7, HeadCommitSHA: headSHA} + + tests := []struct { + name string + pr pullRequestState + wantReason rejectionReason + wantDetail string + }{ + { + name: "clean pull request passes", + pr: pullRequestState{headSHA: headSHA, reviewDecision: "APPROVED", mergeStateStatus: "CLEAN"}, + }, + { + name: "no review rule and no checks passes", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "CLEAN"}, + }, + { + name: "merge state still being computed passes", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "UNKNOWN"}, + }, + { + name: "failing check that is not required passes", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "UNSTABLE", checks: []check{ + {name: "lint", outcome: checkFailed}, + }}, + }, + { + name: "required check that passed", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "CLEAN", checks: []check{ + {name: "ci", required: true, outcome: checkPassed}, + }}, + }, + { + name: "merge states other stages own pass", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "DIRTY"}, + }, + { + name: "moved head is stale", + pr: pullRequestState{headSHA: movedSHA, reviewDecision: "APPROVED", mergeStateStatus: "CLEAN"}, + wantReason: reasonStale, + wantDetail: "head moved from " + headSHA + " to " + movedSHA, + }, + { + name: "changes requested", + pr: pullRequestState{headSHA: headSHA, reviewDecision: "CHANGES_REQUESTED", mergeStateStatus: "BLOCKED"}, + wantReason: reasonNotApproved, + }, + { + name: "required review missing", + pr: pullRequestState{headSHA: headSHA, reviewDecision: "REVIEW_REQUIRED", mergeStateStatus: "BLOCKED"}, + wantReason: reasonNotApproved, + }, + { + name: "required check failed", + pr: pullRequestState{headSHA: headSHA, reviewDecision: "APPROVED", mergeStateStatus: "BLOCKED", checks: []check{ + {name: "ci", required: true, outcome: checkFailed}, + {name: "build", required: true, outcome: checkPassed}, + }}, + wantReason: reasonCheckFailed, + wantDetail: "required checks failed: ci", + }, + { + name: "failed check outranks a pending one", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "BLOCKED", checks: []check{ + {name: "slow", required: true, outcome: checkPending}, + {name: "ci", required: true, outcome: checkFailed}, + }}, + wantReason: reasonCheckFailed, + }, + { + name: "required check still running", + pr: pullRequestState{headSHA: headSHA, mergeStateStatus: "UNKNOWN", checks: []check{ + {name: "ci", required: true, outcome: checkPending}, + }}, + wantReason: reasonCheckPending, + wantDetail: "required checks not finished: ci", + }, + { + name: "blocked for a reason no rule sees", + pr: pullRequestState{headSHA: headSHA, reviewDecision: "APPROVED", mergeStateStatus: "BLOCKED"}, + wantReason: reasonBlocked, + }, + { + name: "stale outranks every other rule", + pr: pullRequestState{headSHA: movedSHA, reviewDecision: "CHANGES_REQUESTED", mergeStateStatus: "BLOCKED", checks: []check{ + {name: "ci", required: true, outcome: checkFailed}, + }}, + wantReason: reasonStale, + }, + { + name: "review outranks checks", + pr: pullRequestState{headSHA: headSHA, reviewDecision: "REVIEW_REQUIRED", mergeStateStatus: "BLOCKED", checks: []check{ + {name: "ci", required: true, outcome: checkFailed}, + }}, + wantReason: reasonNotApproved, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rej, rejected := evaluatePullRequest(cid, tt.pr) + if tt.wantReason == "" { + assert.False(t, rejected, "unexpected rejection: %v", rej) + return + } + require.True(t, rejected) + assert.Equal(t, tt.wantReason, rej.reason) + assert.Equal(t, cid.String(), rej.uri) + if tt.wantDetail != "" { + assert.Equal(t, tt.wantDetail, rej.detail) + } + }) + } +} + +func TestRequiredChecksNotPassed(t *testing.T) { + tests := []struct { + name string + checks []check + wantFailed []string + wantPending []string + }{ + { + name: "no checks", + }, + { + name: "checks that are not required are ignored", + checks: []check{ + {name: "lint", outcome: checkFailed}, + {name: "docs", outcome: checkPending}, + }, + }, + { + name: "a name reported twice takes its worst outcome", + checks: []check{ + {name: "ci", required: true, outcome: checkPassed}, + {name: "ci", required: true, outcome: checkFailed}, + {name: "e2e", required: true, outcome: checkPassed}, + {name: "e2e", required: true, outcome: checkPending}, + {name: "lint", required: true, outcome: checkPending}, + {name: "lint", required: true, outcome: checkFailed}, + }, + wantFailed: []string{"ci", "lint"}, + wantPending: []string{"e2e"}, + }, + { + name: "a name whose every report passed passes", + checks: []check{ + {name: "ci", required: true, outcome: checkPassed}, + {name: "ci", required: true, outcome: checkPassed}, + }, + }, + { + name: "names are sorted", + checks: []check{ + {name: "unit", required: true, outcome: checkFailed}, + {name: "build", required: true, outcome: checkFailed}, + {name: "slow", required: true, outcome: checkPending}, + {name: "e2e", required: true, outcome: checkPending}, + }, + wantFailed: []string{"build", "unit"}, + wantPending: []string{"e2e", "slow"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + failed, pending := requiredChecksNotPassed(tt.checks) + assert.Equal(t, tt.wantFailed, failed) + assert.Equal(t, tt.wantPending, pending) + }) + } +} + +func TestToCheck(t *testing.T) { + tests := []struct { + name string + node checkContextNode + want check + }{ + { + name: "successful check run", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "COMPLETED", Conclusion: "SUCCESS", IsRequired: true}, + want: check{name: "ci", required: true, outcome: checkPassed}, + }, + { + name: "neutral check run passes", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "COMPLETED", Conclusion: "NEUTRAL"}, + want: check{name: "ci", outcome: checkPassed}, + }, + { + name: "skipped check run passes", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "COMPLETED", Conclusion: "SKIPPED"}, + want: check{name: "ci", outcome: checkPassed}, + }, + { + name: "check run in progress is pending", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "IN_PROGRESS"}, + want: check{name: "ci", outcome: checkPending}, + }, + { + name: "queued check run is pending", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "QUEUED"}, + want: check{name: "ci", outcome: checkPending}, + }, + { + name: "failed check run", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "COMPLETED", Conclusion: "FAILURE"}, + want: check{name: "ci", outcome: checkFailed}, + }, + { + name: "cancelled check run fails", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "COMPLETED", Conclusion: "CANCELLED"}, + want: check{name: "ci", outcome: checkFailed}, + }, + { + name: "check run needing action fails", + node: checkContextNode{Typename: "CheckRun", Name: "ci", Status: "COMPLETED", Conclusion: "ACTION_REQUIRED"}, + want: check{name: "ci", outcome: checkFailed}, + }, + { + name: "successful commit status", + node: checkContextNode{Typename: "StatusContext", Context: "ci/legacy", State: "SUCCESS", IsRequired: true}, + want: check{name: "ci/legacy", required: true, outcome: checkPassed}, + }, + { + name: "pending commit status", + node: checkContextNode{Typename: "StatusContext", Context: "ci/legacy", State: "PENDING"}, + want: check{name: "ci/legacy", outcome: checkPending}, + }, + { + name: "expected commit status is pending", + node: checkContextNode{Typename: "StatusContext", Context: "ci/legacy", State: "EXPECTED"}, + want: check{name: "ci/legacy", outcome: checkPending}, + }, + { + name: "errored commit status fails", + node: checkContextNode{Typename: "StatusContext", Context: "ci/legacy", State: "ERROR"}, + want: check{name: "ci/legacy", outcome: checkFailed}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, toCheck(tt.node)) + }) + } +} diff --git a/submitqueue/extension/validator/github/graphql.go b/submitqueue/extension/validator/github/graphql.go new file mode 100644 index 000000000..3067c3f3a --- /dev/null +++ b/submitqueue/extension/validator/github/graphql.go @@ -0,0 +1,202 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package github + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + + entitygithub "github.com/uber/submitqueue/platform/base/change/github" + phttp "github.com/uber/submitqueue/platform/http" +) + +// maxCheckContexts bounds the checks read; one past it still surfaces through +// GitHub's BLOCKED merge state, only without its name. +const maxCheckContexts = 100 + +const pullRequestStateQuery = ` +query($owner: String!, $repo: String!, $number: Int!, $maxChecks: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + headRefOid + reviewDecision + mergeStateStatus + commits(last: 1) { + nodes { + commit { + statusCheckRollup { + contexts(first: $maxChecks) { + nodes { + __typename + ... on CheckRun { + name + status + conclusion + isRequired(pullRequestNumber: $number) + } + ... on StatusContext { + context + state + isRequired(pullRequestNumber: $number) + } + } + } + } + } + } + } + } + } +} +` + +type graphqlRequest struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` +} + +type graphqlResponse struct { + Data struct { + Repository struct { + PullRequest pullRequestNode `json:"pullRequest"` + } `json:"repository"` + } `json:"data"` + Errors []graphqlError `json:"errors"` +} + +type graphqlError struct { + Type string `json:"type"` + Message string `json:"message"` +} + +type pullRequestNode struct { + HeadRefOid string `json:"headRefOid"` + ReviewDecision string `json:"reviewDecision"` + MergeStateStatus string `json:"mergeStateStatus"` + Commits struct { + Nodes []struct { + Commit struct { + StatusCheckRollup struct { + Contexts struct { + Nodes []checkContextNode `json:"nodes"` + } `json:"contexts"` + } `json:"statusCheckRollup"` + } `json:"commit"` + } `json:"nodes"` + } `json:"commits"` +} + +// checkContextNode is one member of a commit's status check rollup: either a +// check run (Name, Status, Conclusion) or a commit status (Context, State). +type checkContextNode struct { + Typename string `json:"__typename"` + Name string `json:"name"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + Context string `json:"context"` + State string `json:"state"` + IsRequired bool `json:"isRequired"` +} + +// fetchPullRequestState fails, rather than rejects, on a missing pull request: +// the change provider resolved the URI before validators run. +func fetchPullRequestState(ctx context.Context, client *http.Client, cid entitygithub.ChangeID) (pullRequestState, error) { + body, err := json.Marshal(graphqlRequest{ + Query: pullRequestStateQuery, + Variables: map[string]any{ + "owner": cid.Org, + "repo": cid.Repo, + "number": cid.PRNumber, + "maxChecks": maxCheckContexts, + }, + }) + if err != nil { + return pullRequestState{}, fmt.Errorf("marshal GraphQL request: %w", err) + } + + // The path is relative: the client's transport roots it at the API. + status, respBody, err := phttp.SendRequest(ctx, client, http.MethodPost, "/graphql", body, setGraphQLHeaders) + if err != nil { + return pullRequestState{}, err + } + if status != http.StatusOK { + return pullRequestState{}, phttp.NewStatusError(status, respBody) + } + + var resp graphqlResponse + if err := json.Unmarshal(respBody, &resp); err != nil { + return pullRequestState{}, fmt.Errorf("decode GraphQL response: %w", err) + } + if len(resp.Errors) > 0 { + return pullRequestState{}, fmt.Errorf("GraphQL errors: %+v", resp.Errors) + } + pr := resp.Data.Repository.PullRequest + if pr.HeadRefOid == "" { + return pullRequestState{}, fmt.Errorf("GraphQL response has no pull request") + } + return toPullRequestState(pr), nil +} + +func setGraphQLHeaders(req *http.Request) { + req.Header.Set("Content-Type", "application/json") +} + +func toPullRequestState(pr pullRequestNode) pullRequestState { + state := pullRequestState{ + headSHA: pr.HeadRefOid, + reviewDecision: pr.ReviewDecision, + mergeStateStatus: pr.MergeStateStatus, + } + for _, c := range pr.Commits.Nodes { + for _, node := range c.Commit.StatusCheckRollup.Contexts.Nodes { + state.checks = append(state.checks, toCheck(node)) + } + } + return state +} + +func toCheck(node checkContextNode) check { + if node.Typename == "StatusContext" { + return check{name: node.Context, required: node.IsRequired, outcome: statusContextOutcome(node.State)} + } + return check{name: node.Name, required: node.IsRequired, outcome: checkRunOutcome(node.Status, node.Conclusion)} +} + +// checkRunOutcome maps a check run onto an outcome. Neutral and skipped runs +// satisfy a required check on GitHub, so they pass here too. +func checkRunOutcome(status, conclusion string) checkOutcome { + if status != "COMPLETED" { + return checkPending + } + switch conclusion { + case "SUCCESS", "NEUTRAL", "SKIPPED": + return checkPassed + default: + return checkFailed + } +} + +func statusContextOutcome(state string) checkOutcome { + switch state { + case "SUCCESS": + return checkPassed + case "PENDING", "EXPECTED": + return checkPending + default: + return checkFailed + } +} diff --git a/submitqueue/extension/validator/github/validator.go b/submitqueue/extension/validator/github/validator.go new file mode 100644 index 000000000..4537d586b --- /dev/null +++ b/submitqueue/extension/validator/github/validator.go @@ -0,0 +1,132 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package github provides a validator.Validator that rejects a request whose +// GitHub pull requests the repository's own rules would not let merge yet: a +// review is missing or requests changes, a required check failed or is still +// running, or GitHub otherwise reports merging as blocked. It reads that state +// from GitHub's GraphQL API, so the rules are whatever branch protection or +// rulesets the repository configures; nothing is restated in SubmitQueue +// configuration. See README.md for what it leaves to other stages. +package github + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/uber-go/tally" + "go.uber.org/zap" + + entitygithub "github.com/uber/submitqueue/platform/base/change/github" + coremetrics "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" +) + +// Params holds the dependencies for the GitHub validator. +type Params struct { + // Config is the per-queue identity handed to the Factory that built this + // validator. + Config validator.Config + // HTTPClient roots relative paths at the GitHub API (for example + // platform/http.NewClient("https://api.github.com")) and authenticates + // them. The caller owns the base URL and the credential. + HTTPClient *http.Client + // Host is the GitHub instance change URIs must name (e.g. "github.com"): + // the one HTTPClient talks to. + Host string + // Logger is the structured logger. + Logger *zap.SugaredLogger + // MetricsScope is the metrics scope for instrumentation. + MetricsScope tally.Scope +} + +type githubValidator struct { + cfg validator.Config + httpClient *http.Client + host string + logger *zap.SugaredLogger + metricsScope tally.Scope +} + +// New returns a validator bound to the queue named in params.Config. It errors +// when HTTPClient or Host is missing. +func New(params Params) (validator.Validator, error) { + if params.HTTPClient == nil { + return nil, errors.New("github validator: HTTPClient is required") + } + if params.Host == "" { + return nil, errors.New("github validator: Host is required") + } + return &githubValidator{ + cfg: params.Config, + httpClient: params.HTTPClient, + host: params.Host, + logger: params.Logger.Named("github_validator"), + metricsScope: params.MetricsScope.SubScope("github_validator").Tagged(map[string]string{"queue": params.Config.QueueName}), + }, nil +} + +// Validate checks the request's GitHub pull requests bottom first and returns +// the first rule one breaks. A failure to read GitHub is returned wrapped, +// keeping an HTTP status code in the chain for platform/errs/http. URIs that +// are not GitHub change IDs are skipped, so this validator composes with +// validators for other change providers. +func (v *githubValidator) Validate(ctx context.Context, request entity.Request) error { + op := coremetrics.Begin(v.metricsScope, "validate", coremetrics.LongLatencyBuckets) + rej, rejected, err := v.findRejection(ctx, request) + // A rejection is an expected outcome, so only a failure to read GitHub + // counts against the operation's result. + op.Complete(err) + if err != nil { + return err + } + if !rejected { + return nil + } + + coremetrics.NamedCounter(v.metricsScope, "validate", "rejections", 1, coremetrics.NewTag("reason", string(rej.reason))) + v.logger.Infow("rejecting request", + "queue", v.cfg.QueueName, + "request_id", request.ID, + "uri", rej.uri, + "reason", string(rej.reason), + "detail", rej.detail, + ) + return rej +} + +func (v *githubValidator) findRejection(ctx context.Context, request entity.Request) (rejection, bool, error) { + for _, uri := range request.Change.URIs { + cid, err := entitygithub.ParseChangeID(uri) + if err != nil { + v.logger.Debugw("skipping non-GitHub URI", "uri", uri) + continue + } + if cid.Host != v.host { + return rejection{uri: cid.String(), reason: reasonHost, detail: fmt.Sprintf("names host %q, not %q", cid.Host, v.host)}, true, nil + } + + pr, err := fetchPullRequestState(ctx, v.httpClient, cid) + if err != nil { + return rejection{}, false, fmt.Errorf("read pull request %s: %w", uri, err) + } + if rej, rejected := evaluatePullRequest(cid, pr); rejected { + return rej, true, nil + } + } + return rejection{}, false, nil +} diff --git a/submitqueue/extension/validator/github/validator_test.go b/submitqueue/extension/validator/github/validator_test.go new file mode 100644 index 000000000..c314becf0 --- /dev/null +++ b/submitqueue/extension/validator/github/validator_test.go @@ -0,0 +1,363 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "go.uber.org/zap/zaptest" + + "github.com/uber/submitqueue/platform/base/change" + phttp "github.com/uber/submitqueue/platform/http" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" +) + +const testHost = "github.com" + +func testURI(number int) string { + return fmt.Sprintf("github://%s/acme/widgets/pull/%d/%s", testHost, number, headSHA) +} + +// prBody is a GraphQL response body for one pull request at headSHA. +func prBody(reviewDecision, mergeStateStatus string, contexts ...map[string]any) string { + pr := map[string]any{ + "headRefOid": headSHA, + "reviewDecision": reviewDecision, + "mergeStateStatus": mergeStateStatus, + "commits": map[string]any{"nodes": []any{ + map[string]any{"commit": map[string]any{ + "statusCheckRollup": map[string]any{"contexts": map[string]any{"nodes": contexts}}, + }}, + }}, + } + body, _ := json.Marshal(map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": pr}}}) + return string(body) +} + +func checkRun(name, status, conclusion string, required bool) map[string]any { + return map[string]any{"__typename": "CheckRun", "name": name, "status": status, "conclusion": conclusion, "isRequired": required} +} + +type graphqlCall struct { + method string + path string + auth string + variables map[string]any +} + +// fakeGitHub answers GraphQL requests with the body configured for the pull +// request number in the request's variables and records each call. +type fakeGitHub struct { + mu sync.Mutex + status int + bodies map[int]string + calls []graphqlCall +} + +func (f *fakeGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { + raw, _ := io.ReadAll(r.Body) + var req graphqlRequest + _ = json.Unmarshal(raw, &req) + + f.mu.Lock() + defer f.mu.Unlock() + f.calls = append(f.calls, graphqlCall{method: r.Method, path: r.URL.Path, auth: r.Header.Get("Authorization"), variables: req.Variables}) + if f.status != 0 { + w.WriteHeader(f.status) + _, _ = io.WriteString(w, "upstream unavailable") + return + } + number, _ := req.Variables["number"].(float64) + _, _ = io.WriteString(w, f.bodies[int(number)]) +} + +// authTransport stands in for the credential the wiring layer configures. +type authTransport struct{ base http.RoundTripper } + +func (t authTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req = req.Clone(req.Context()) + req.Header.Set("Authorization", "Bearer test-token") + return t.base.RoundTrip(req) +} + +func newTestValidator(t *testing.T, gh *fakeGitHub, scope tally.Scope) validator.Validator { + t.Helper() + server := httptest.NewServer(gh) + t.Cleanup(server.Close) + client, err := phttp.NewClient(server.URL) + require.NoError(t, err) + client.Transport = authTransport{base: client.Transport} + + v, err := New(Params{ + Config: validator.Config{QueueName: "acme/widgets"}, + HTTPClient: client, + Host: testHost, + Logger: zaptest.NewLogger(t).Sugar(), + MetricsScope: scope, + }) + require.NoError(t, err) + return v +} + +func request(uris ...string) entity.Request { + return entity.Request{ID: "acme/widgets/1", Queue: "acme/widgets", Change: change.Change{URIs: uris}} +} + +func TestValidate(t *testing.T) { + approved := prBody("APPROVED", "CLEAN", checkRun("ci", "COMPLETED", "SUCCESS", true)) + failing := prBody("APPROVED", "BLOCKED", checkRun("ci", "COMPLETED", "FAILURE", true)) + + tests := []struct { + name string + gh *fakeGitHub + uris []string + // wantReason is the rejection's reason; empty means the request passes. + wantReason rejectionReason + wantURI string + // wantFetched is the pull request numbers read from GitHub, in order. + wantFetched []int + }{ + { + name: "passing pull request", + gh: &fakeGitHub{bodies: map[int]string{1: approved}}, + uris: []string{testURI(1)}, + wantFetched: []int{1}, + }, + { + name: "failing required check rejects", + gh: &fakeGitHub{bodies: map[int]string{1: failing}}, + uris: []string{testURI(1)}, + wantReason: reasonCheckFailed, + wantURI: testURI(1), + wantFetched: []int{1}, + }, + { + name: "stack is read bottom first and rejected at the pull request that fails", + gh: &fakeGitHub{bodies: map[int]string{1: approved, 2: failing}}, + uris: []string{testURI(1), testURI(2)}, + wantReason: reasonCheckFailed, + wantURI: testURI(2), + wantFetched: []int{1, 2}, + }, + { + name: "pull requests above a rejected one are not read", + gh: &fakeGitHub{bodies: map[int]string{1: failing, 2: approved}}, + uris: []string{testURI(1), testURI(2)}, + wantReason: reasonCheckFailed, + wantURI: testURI(1), + wantFetched: []int{1}, + }, + { + name: "URI of another change provider is skipped", + gh: &fakeGitHub{bodies: map[int]string{1: approved}}, + uris: []string{"phab://D123/diff/456", testURI(1)}, + wantFetched: []int{1}, + }, + { + name: "URI on another GitHub host is rejected without reading it", + gh: &fakeGitHub{}, + uris: []string{"github://github.example.com/acme/widgets/pull/1/" + headSHA}, + wantReason: reasonHost, + wantURI: "github://github.example.com/acme/widgets/pull/1/" + headSHA, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := newTestValidator(t, tt.gh, tally.NoopScope) + + err := v.Validate(context.Background(), request(tt.uris...)) + + var fetched []int + for _, call := range tt.gh.calls { + assert.Equal(t, http.MethodPost, call.method) + assert.Equal(t, "/graphql", call.path) + assert.Equal(t, "Bearer test-token", call.auth) + assert.Equal(t, "acme", call.variables["owner"]) + assert.Equal(t, "widgets", call.variables["repo"]) + number, _ := call.variables["number"].(float64) + fetched = append(fetched, int(number)) + } + assert.Equal(t, tt.wantFetched, fetched) + + if tt.wantReason == "" { + assert.NoError(t, err) + return + } + var rej rejection + require.True(t, errors.As(err, &rej), "want a rejection, got %v", err) + assert.Equal(t, tt.wantReason, rej.reason) + assert.Equal(t, tt.wantURI, rej.uri) + }) + } +} + +func TestValidateReadFailures(t *testing.T) { + tests := []struct { + name string + gh *fakeGitHub + wantStatus int + }{ + { + name: "HTTP status keeps its code for classification", + gh: &fakeGitHub{status: http.StatusBadGateway}, + wantStatus: http.StatusBadGateway, + }, + { + name: "GraphQL error", + gh: &fakeGitHub{bodies: map[int]string{ + 1: `{"data":{"repository":{"pullRequest":null}},"errors":[{"type":"NOT_FOUND","message":"Could not resolve to a PullRequest"}]}`, + }}, + }, + { + name: "response without a pull request", + gh: &fakeGitHub{bodies: map[int]string{1: `{"data":{"repository":{"pullRequest":null}}}`}}, + }, + { + name: "undecodable response", + gh: &fakeGitHub{bodies: map[int]string{1: `not json`}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := newTestValidator(t, tt.gh, tally.NoopScope) + + err := v.Validate(context.Background(), request(testURI(1))) + + require.Error(t, err) + assert.False(t, errors.As(err, &rejection{}), "a failure to read GitHub is not a rejection") + var statusErr *phttp.StatusError + if tt.wantStatus != 0 { + require.True(t, errors.As(err, &statusErr)) + assert.Equal(t, tt.wantStatus, statusErr.StatusCode) + } else { + assert.False(t, errors.As(err, &statusErr)) + } + }) + } +} + +func TestValidateMetrics(t *testing.T) { + tests := []struct { + name string + gh *fakeGitHub + wantResult string + wantRejections map[string]int64 + }{ + { + name: "pass", + gh: &fakeGitHub{bodies: map[int]string{1: prBody("APPROVED", "CLEAN")}}, + wantResult: "success", + }, + { + name: "rejection is counted by reason and is not an error", + gh: &fakeGitHub{bodies: map[int]string{1: prBody("REVIEW_REQUIRED", "BLOCKED")}}, + wantResult: "success", + wantRejections: map[string]int64{"not_approved": 1}, + }, + { + name: "read failure is an error", + gh: &fakeGitHub{status: http.StatusInternalServerError}, + wantResult: "error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scope := tally.NewTestScope("", nil) + v := newTestValidator(t, tt.gh, scope) + + _ = v.Validate(context.Background(), request(testURI(1))) + + snapshot := scope.Snapshot() + results := map[string]bool{} + for _, h := range snapshot.Histograms() { + if h.Name() == "github_validator.validate.finish" { + assert.Equal(t, "acme/widgets", h.Tags()["queue"]) + results[h.Tags()["result"]] = true + } + } + assert.Equal(t, map[string]bool{tt.wantResult: true}, results) + + rejections := map[string]int64{} + for _, c := range snapshot.Counters() { + if c.Name() == "github_validator.validate.rejections" { + assert.Equal(t, "acme/widgets", c.Tags()["queue"]) + rejections[c.Tags()["reason"]] += c.Value() + } + } + if tt.wantRejections == nil { + assert.Empty(t, rejections) + } else { + assert.Equal(t, tt.wantRejections, rejections) + } + }) + } +} + +func TestNew(t *testing.T) { + client := &http.Client{} + + tests := []struct { + name string + params Params + wantErr bool + }{ + { + name: "complete params", + params: Params{HTTPClient: client, Host: testHost}, + }, + { + name: "missing HTTP client", + params: Params{Host: testHost}, + wantErr: true, + }, + { + name: "missing host", + params: Params{HTTPClient: client}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.params.Logger = zaptest.NewLogger(t).Sugar() + tt.params.MetricsScope = tally.NoopScope + + v, err := New(tt.params) + + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, v) + return + } + require.NoError(t, err) + assert.NotNil(t, v) + }) + } +}