Skip to content

Commit 50475cf

Browse files
authored
feat(change): require host[:port] authority in change URIs (#329)
## Summary ### Why? The Change URIs RFC (doc/rfc/change-uri.md, previous PR in this stack) makes every change URI carry the provider instance's `host[:port]` as its RFC 3986 authority, so a URI is globally unambiguous on its own: GitHub and Phabricator URIs previously carried no host (the same URI could name two different changes on two provider instances), and the `ghe`/`ghes` schemes stood in for an instance discriminator without actually being one. ### What? - `platform/base/change/github`: parser rewritten on `net/url`; format is now `github://{host[:port]}/{org}/{repo}/pull/{pr}/{sha}`; single `github` scheme — `ghe`/`ghes` are rejected; parsed form gains a `Host` field. - `platform/base/change/phabricator`: same treatment; format is now `phab://{host[:port]}/D{revision}/{diff}`; the host moves in-band from queue config into the URI authority. - `platform/base/change/git`: unchanged format (it already carried the authority); gains only the new lowercase-host validation. - `platform/base/change/changeutil`: new `IsLowercaseASCII` predicate — canonical form is validated, never normalized (uppercase hosts are rejected, not folded), matching the existing full-40-hex SHA rule. - Contract docs updated in `api/base/change` proto (comment-only, regenerated via `make proto`), `platform/base/change`, and entity comments; all test fixtures across gateway, orchestrator, extensions, runway, integration, and e2e suites moved to authority-bearing URIs (`github.example.com` / `phab.example.com`). No wire change: `Change.uris` stays a repeated string; only the accepted grammar of its contents changes. Pre-production, so this is a hard cutover with no dual-format window (per the RFC's rejected alternatives). ## Test Plan ✅ `make test` (65/65), ✅ `make build`, ✅ `make proto` / `make fmt` / `make gazelle` idempotent. Parser suites cover the new grammar: host and host:port round-trips, nested orgs, and rejections for missing host, uppercase host, host-less legacy URIs, and `ghe://`/`ghes://` schemes. ## Stack 1. #328 1. @ #329
1 parent 48de2e7 commit 50475cf

45 files changed

Lines changed: 420 additions & 269 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

api/base/change/proto/change.proto

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,15 @@ option java_package = "com.uber.submitqueue.base.change";
2929
// Stovepipe, and other repo-local domains — the proto-level analog of the
3030
// platform/base/change Go entity. Domains import it rather than redefining their own.
3131
message Change {
32-
// URIs identifying the change(s) (RFC 3986 compliant). The scheme identifies the
33-
// change provider, and the path contains provider-specific resource identifiers.
32+
// URIs identifying the change(s) (RFC 3986 compliant): scheme://<host[:port]>/<path>.
33+
// The scheme identifies the change provider, the authority is the provider instance
34+
// the change lives on, and the path contains provider-specific resource identifiers.
3435
//
3536
// Supported by default (other providers can be added):
36-
// GitHub PR: "github://<org>/<repo>/pull/<pr>/<head_commit_sha>"
37-
// ("ghe"/"ghes" schemes for GitHub Enterprise)
38-
// git commit: "git://<remote>/<repo>/<ref>/<commit_sha>"
39-
// (<ref> is a fully-qualified, percent-encoded git ref)
37+
// GitHub PR: "github://<host[:port]>/<org>/<repo>/pull/<pr>/<head_commit_sha>"
38+
// Phabricator Diff: "phab://<host[:port]>/D<revision>/<diff>"
39+
// git commit: "git://<host[:port]>/<repo>/<ref>/<commit_sha>"
40+
// (<ref> is a fully-qualified, percent-encoded git ref)
4041
//
4142
// The commit SHA must be the full 40-character lowercase hex SHA; abbreviated
4243
// SHAs are rejected because downstream staleness checks compare by strict equality.

api/base/change/protopb/change.pb.go

Lines changed: 7 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

api/runway/messagequeue/merge_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ func TestMergeRequestRoundTrip(t *testing.T) {
3333
Steps: []*MergeStep{
3434
{
3535
StepId: "queue-a/1",
36-
Changes: []*changepb.Change{{Uris: []string{"github://uber/repo/pull/1/0123456789abcdef0123456789abcdef01234567"}}},
36+
Changes: []*changepb.Change{{Uris: []string{"github://github.example.com/uber/repo/pull/1/0123456789abcdef0123456789abcdef01234567"}}},
3737
Strategy: strategypb.Strategy_REBASE,
3838
},
3939
{
4040
StepId: "queue-a/2",
41-
Changes: []*changepb.Change{{Uris: []string{"github://uber/repo/pull/2/89abcdef0123456789abcdef0123456789abcdef"}}},
41+
Changes: []*changepb.Change{{Uris: []string{"github://github.example.com/uber/repo/pull/2/89abcdef0123456789abcdef0123456789abcdef"}}},
4242
Strategy: strategypb.Strategy_MERGE,
4343
},
4444
},

platform/base/change/change.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,17 @@ package change
2020
// Change represents a code change identified by URIs from a code change provider (e.g., GitHub Pull Request, Phabricator Diff).
2121
// The provider is extracted from the URI scheme. The object is immutable after creation.
2222
type Change struct {
23-
// URIs identifies the change(s) to land (RFC 3986 compliant).
24-
// The scheme identifies the change provider, and the path contains provider-specific resource identifiers.
23+
// URIs identifies the change(s) to land (RFC 3986 compliant): scheme://<host[:port]>/<path>.
24+
// The scheme identifies the change provider, the authority is the provider instance the
25+
// change lives on, and the path contains provider-specific resource identifiers.
2526
//
26-
// GitHub is supported by default (though other providers can be added):
27-
// Template: "<scheme>://<org>/<repo>/pull/<pr>/<head_commit_sha>"
28-
// Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"
29-
// Schemes: "github", "ghe", "ghes". Head commit SHA must be full 40-char lowercase hex.
27+
// Supported formats:
28+
// GitHub PR: "github://<host[:port]>/<org>/<repo>/pull/<pr>/<head_commit_sha>"
29+
// Phabricator Diff: "phab://<host[:port]>/D<revision>/<diff>"
30+
// git ref/commit: "git://<host[:port]>/<repo>/<ref>/<sha>"
31+
// Example: "github://github.example.com/uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"
32+
//
33+
// Head/commit SHAs must be the full 40-char lowercase hex form.
3034
//
3135
URIs []string `json:"uris"`
3236
}

platform/base/change/changeutil/BUILD.bazel

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,20 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")
22

33
go_library(
44
name = "go_default_library",
5-
srcs = ["hex.go"],
5+
srcs = [
6+
"hex.go",
7+
"host.go",
8+
],
69
importpath = "github.com/uber/submitqueue/platform/base/change/changeutil",
710
visibility = ["//visibility:public"],
811
)
912

1013
go_test(
1114
name = "go_default_test",
12-
srcs = ["hex_test.go"],
15+
srcs = [
16+
"hex_test.go",
17+
"host_test.go",
18+
],
1319
embed = [":go_default_library"],
1420
deps = ["@com_github_stretchr_testify//assert:go_default_library"],
1521
)
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package changeutil
16+
17+
// IsLowercaseASCII reports whether s contains no uppercase ASCII letters (A-Z).
18+
// Providers validate the host segment of a change URI with this: DNS is
19+
// case-insensitive, so an uppercase variant of a host would alias one
20+
// provider instance into many identities if let through. Canonical form
21+
// rejects uppercase hosts rather than folding them.
22+
func IsLowercaseASCII(s string) bool {
23+
for i := 0; i < len(s); i++ {
24+
c := s[i]
25+
if c >= 'A' && c <= 'Z' {
26+
return false
27+
}
28+
}
29+
return true
30+
}
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package changeutil
16+
17+
import (
18+
"testing"
19+
20+
"github.com/stretchr/testify/assert"
21+
)
22+
23+
func TestIsLowercaseASCII(t *testing.T) {
24+
tests := []struct {
25+
name string
26+
s string
27+
want bool
28+
}{
29+
{name: "plain lowercase host", s: "github.example.com", want: true},
30+
{name: "lowercase host with port", s: "github.example.com:8443", want: true},
31+
{name: "empty string", s: "", want: true},
32+
{name: "digits and hyphens", s: "git-01.example-corp.com", want: true},
33+
{name: "uppercase letter rejected", s: "GitHub.example.com", want: false},
34+
{name: "all uppercase rejected", s: "GITHUB.EXAMPLE.COM", want: false},
35+
}
36+
37+
for _, tt := range tests {
38+
t.Run(tt.name, func(t *testing.T) {
39+
assert.Equal(t, tt.want, IsLowercaseASCII(tt.s))
40+
})
41+
}
42+
}

platform/base/change/git/change_id.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,9 @@ func ParseChangeID(raw string) (ChangeID, error) {
7272
if u.Host == "" {
7373
return ChangeID{}, fmt.Errorf("invalid change ID %q: missing remote (expected format: %s)", raw, changeIDFormat)
7474
}
75+
if !changeutil.IsLowercaseASCII(u.Hostname()) {
76+
return ChangeID{}, fmt.Errorf("invalid change ID %q: remote %q must be lowercase (expected format: %s)", raw, u.Hostname(), changeIDFormat)
77+
}
7578

7679
// Split on the escaped path so the percent-encoded ref stays a single
7780
// segment (url.URL.Path decodes %2F to "/", which would split it apart).

platform/base/change/git/change_id_test.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,11 @@ func TestParseChangeID(t *testing.T) {
106106
raw: "git:///uber/monorepo/refs%2Fheads%2Fmain/" + sha,
107107
wantErr: true,
108108
},
109+
{
110+
name: "uppercase host",
111+
raw: "git://Git.example.com/uber/monorepo/refs%2Fheads%2Fmain/" + sha,
112+
wantErr: true,
113+
},
109114
{
110115
name: "missing commit SHA",
111116
raw: "git://git.example.com/uber/monorepo/refs%2Fheads%2Fmain",

platform/base/change/github/change_id.go

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,21 @@ package github
1616

1717
import (
1818
"fmt"
19+
"net/url"
1920
"strconv"
2021
"strings"
2122

2223
"github.com/uber/submitqueue/platform/base/change/changeutil"
2324
)
2425

26+
// scheme is the canonical URI scheme for GitHub change identifiers. Deployment
27+
// flavors (github.com, GitHub Enterprise, GitHub Enterprise Server) share this
28+
// one scheme; the flavor is derivable from the host in the authority, so it
29+
// does not get its own scheme.
30+
const scheme = "github"
31+
2532
// changeIDFormat is the expected format for change IDs, included in error messages.
26-
const changeIDFormat = "{scheme}://{owner}/{repo}/pull/{pr_number}/{head_commit_sha}"
33+
const changeIDFormat = "github://{host[:port]}/{owner}/{repo}/pull/{pr_number}/{head_commit_sha}"
2734

2835
// pullSegment is the literal segment separating the repo path from the pull
2936
// request number. Mirrors the path layout of an actual GitHub PR URL
@@ -38,13 +45,16 @@ const pullSegment = "pull"
3845
// Validate up-front to fail fast at the gateway with a clearer error.
3946
const shaLength = 40
4047

41-
// ChangeID represents a parsed GitHub-family change identifier.
42-
// Covers GitHub.com, GitHub Enterprise (GHE), and GitHub Enterprise Server (GHES)
43-
// since they share the same pull request model.
44-
// Format: {scheme}://{owner}/{repo}/pull/{pr_number}/{head_commit_sha}
48+
// ChangeID represents a parsed GitHub change identifier. The authority names
49+
// the GitHub instance the change lives on, so github.com, GitHub Enterprise,
50+
// and GitHub Enterprise Server all parse under the single "github" scheme.
51+
// Format: github://{host[:port]}/{owner}/{repo}/pull/{pr_number}/{head_commit_sha}
4552
type ChangeID struct {
46-
// Scheme captures the source variant (e.g., "github", "ghe", "ghes").
53+
// Scheme captures the URI scheme (always "github" in current implementation).
4754
Scheme string
55+
// Host is the host or host:port of the GitHub instance the change lives
56+
// on, e.g. "github.example.com" or "github.example.com:8443".
57+
Host string
4858
// Org is the organization or owner of the repository.
4959
Org string
5060
// Repo is the repository name.
@@ -56,29 +66,30 @@ type ChangeID struct {
5666
}
5767

5868
// ParseChangeID parses a raw change ID string into a ChangeID.
59-
// Expected format: {scheme}://{owner}/{repo}/pull/{pr_number}/{head_commit_sha}
69+
// Expected format: github://{host[:port]}/{owner}/{repo}/pull/{pr_number}/{head_commit_sha}
6070
// The parser works from the end: SHA (last), PR number (second-to-last),
6171
// the literal "pull" segment (third-to-last), and everything before is the
6272
// repo path (split into owner and repo).
6373
func ParseChangeID(raw string) (ChangeID, error) {
64-
// Split on "://" to get scheme and path
65-
schemeSplit := strings.SplitN(raw, "://", 2)
66-
if len(schemeSplit) != 2 {
67-
return ChangeID{}, fmt.Errorf("invalid change ID %q: missing '://' separator (expected format: %s)", raw, changeIDFormat)
74+
u, err := url.Parse(raw)
75+
if err != nil {
76+
return ChangeID{}, fmt.Errorf("invalid change ID %q: %w (expected format: %s)", raw, err, changeIDFormat)
6877
}
69-
70-
scheme := schemeSplit[0]
71-
if scheme == "" {
72-
return ChangeID{}, fmt.Errorf("invalid change ID %q: empty scheme (expected format: %s)", raw, changeIDFormat)
78+
if u.Scheme != scheme {
79+
return ChangeID{}, fmt.Errorf("invalid change ID %q: scheme must be %q, got %q (expected format: %s)", raw, scheme, u.Scheme, changeIDFormat)
80+
}
81+
if u.Host == "" {
82+
return ChangeID{}, fmt.Errorf("invalid change ID %q: missing host (expected format: %s)", raw, changeIDFormat)
83+
}
84+
if !changeutil.IsLowercaseASCII(u.Hostname()) {
85+
return ChangeID{}, fmt.Errorf("invalid change ID %q: host %q must be lowercase (expected format: %s)", raw, u.Hostname(), changeIDFormat)
7386
}
7487

75-
path := schemeSplit[1]
76-
77-
// Split the path into segments and parse from the end.
78-
segments := strings.Split(path, "/")
79-
// Need at least 5 segments: {owner}/{repo}/pull/{pr_number}/{sha}
88+
// Split on the escaped path so any percent-encoded segments stay intact.
89+
segments := strings.Split(strings.TrimPrefix(u.EscapedPath(), "/"), "/")
90+
// Need at least 5 segments: {owner}/{repo}/pull/{pr_number}/{sha}.
8091
if len(segments) < 5 {
81-
return ChangeID{}, fmt.Errorf("invalid change ID %q: need at least owner/repo/pull/pr/sha, got %d segments (expected format: %s)", raw, len(segments), changeIDFormat)
92+
return ChangeID{}, fmt.Errorf("invalid change ID %q: need at least owner/repo/pull/pr/sha, got %d path segments (expected format: %s)", raw, len(segments), changeIDFormat)
8293
}
8394

8495
sha := segments[len(segments)-1]
@@ -118,7 +129,8 @@ func ParseChangeID(raw string) (ChangeID, error) {
118129
}
119130

120131
return ChangeID{
121-
Scheme: scheme,
132+
Scheme: u.Scheme,
133+
Host: u.Host,
122134
Org: org,
123135
Repo: repo,
124136
PRNumber: prNumber,
@@ -128,7 +140,7 @@ func ParseChangeID(raw string) (ChangeID, error) {
128140

129141
// String returns the string representation of the change ID.
130142
func (c ChangeID) String() string {
131-
return fmt.Sprintf("%s://%s/%s/%s/%d/%s", c.Scheme, c.Org, c.Repo, pullSegment, c.PRNumber, c.HeadCommitSHA)
143+
return fmt.Sprintf("%s://%s/%s/%s/%s/%d/%s", c.Scheme, c.Host, c.Org, c.Repo, pullSegment, c.PRNumber, c.HeadCommitSHA)
132144
}
133145

134146
// OwnerRepo returns the "{org}/{repo}" string.

0 commit comments

Comments
 (0)