refactor: validate and parse the endpoint and proxy at program load by peterguy · Pull Request #1267 · sourcegraph/src-cli

peterguy · 2026-03-07T02:37:45Z

Summary

Parse the endpoint URL at program load instead of passing around an unparsed string and needing to check/parse it in multiple places.

Parse the proxy url at program load, supporting HTTP_PROXY, HTTPS_PROXY and NO_PROXY in addition to SRC_PROXY. SRC_PROXY takes precedence.

Changes

`config` struct (`cmd/src/main.go`)

Split out a configFromFile struct containing the JSON elements
Un-export fields (all consumers are in package main)
Use endpointURL - Endpoint and Proxy are now in configFromFile
Parse endpointURL in readConfig - consumers use the parsed URL

`api.ClientOpts` (`internal/api/api.go`)

Endpoint string → EndpointURL *url.URL
Use JoinPath for URL construction instead of string concatenation with TrimRight

`login` command (`cmd/src/login.go`)

loginCmd now uses the config's endpointURL
Endpoint conflict detection moved from loginCmd into the handler, where the CLI arg is parsed and compared before entering the command

`code_intel_upload` (`cmd/src/code_intel_upload.go`)

makeCodeIntelUploadURL uses a stack copy (u := *cfg.endpointURL) instead of re-parsing the URL
Error return removed since there's nothing that can fail

Tests

Updated all test call sites to pass *url.URL

Testing

All affected packages pass:

ok  github.com/sourcegraph/src-cli/cmd/src
ok  github.com/sourcegraph/src-cli/internal/api
ok  github.com/sourcegraph/src-cli/internal/batches/repozip
ok  github.com/sourcegraph/src-cli/internal/batches/executor

cmd/src/code_intel_upload.go

cmd/src/login.go

peterguy · 2026-03-07T23:44:48Z

cmd/src/login.go

+	if cfg.configFilePath != "" {
 		fmt.Fprintln(out)
-		fmt.Fprintf(out, "⚠️  Warning: Configuring src with a JSON file is deprecated. Please migrate to using the env vars SRC_ENDPOINT, SRC_ACCESS_TOKEN, and SRC_PROXY instead, and then remove %s. See https://github.com/sourcegraph/src-cli#readme for more information.\n", cfg.ConfigFilePath)
+		fmt.Fprintf(out, "⚠️  Warning: Configuring src with a JSON file is deprecated. Please migrate to using the env vars SRC_ENDPOINT, SRC_ACCESS_TOKEN, and SRC_PROXY instead, and then remove %s. See https://github.com/sourcegraph/src-cli#readme for more information.\n", cfg.configFilePath)


The config file has been deprecated for a long time, but if it ever is finally removed, the ability to set additional headers will go also. Do we need to keep the ability to set additional headers around? If so, we should probably introduce yet another environment variable for those and delay the config file removal. If not, let's complete the config file deprecation!

See https://github.com/sourcegraph/src-cli/pull/1267/changes#r2903847768

peterguy · 2026-03-08T03:32:27Z

This change is part of the following stack:

refactor: validate and parse the endpoint and proxy at program load #1267 ◀
- Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY #1260

_{Change managed by git-spice.}

internal/api/api.go

bahrmichael

I think there's a bug in https://github.com/sourcegraph/src-cli/pull/1267/changes#r2903861064. But apart from that good to go, so I'll approve to unblock.

cmd/src/batch_remote.go

keegancsmith

approving, but agree with Michael's catch on the breaking change.

internal/api/api.go

cmd/src/login.go

Amp-Thread-ID: https://ampcode.com/threads/T-019cdb3f-f7de-750b-b4c3-13762c7dfc11 Co-authored-by: Amp <amp@ampcode.com>

burmudar · 2026-03-11T07:57:43Z

cmd/src/login.go

+			out:              os.Stdout,
+			apiFlags:         apiFlags,
+			oauthClient:      oauth.NewClient(oauth.DefaultClientID),
+			loginEndpointURL: loginEndpointURL,


Suggested change

loginEndpointURL: loginEndpointURL,

endpointURL: loginEndpointURL,

burmudar · 2026-03-11T07:57:57Z

cmd/src/login.go

+	out              io.Writer
+	apiFlags         *api.Flags
+	oauthClient      oauth.Client
+	loginEndpointURL *url.URL


Suggested change

loginEndpointURL *url.URL

endpointURL *url.URL

cmd/src/login.go

internal/oauth/flow.go

burmudar · 2026-03-11T08:10:41Z

cmd/src/main.go

+	proxyURL          *url.URL
+	proxyPath         string
+	configFilePath    string
+	endpointURL       *url.URL // always non-nil; defaults to https://sourcegraph.com via readConfig


if we are sure it will never be nil, then maybe we should just make it url.URL and not a pointer?

Yeah, I considered that. I do like the signal it sends: using a value says it's a constant, do not modify, owned by the config/program, immutable, non-optional, no nil dereferencing panics.

Propagating it through the code introduces enough cognitive friction that I have hesitated to do it:

.String() usage scattered everywhere that are currently implicit via %s on *url.URL

The url parse functions all deal in pointers, so there is inevitable dereferencing friction

I just don't think we gain enough from making it a value to warrant the widespread semantic awkwardness we'd have to deal with afterward.

diff

WDYT?

burmudar · 2026-03-11T08:13:02Z

cmd/src/login.go

 func selectLoginFlow(p loginParams) (loginFlowKind, loginFlow) {
-	endpointArg := cleanEndpoint(p.endpoint)
-
+	if p.loginEndpointURL != nil && p.loginEndpointURL.String() != p.cfg.endpointURL.String() {


if we just make cfg the source of truth and the cli endpoint arg just overrides it we don't have to do this check at all.

Yes, but that would break current behavior. As confusing as the current behavior is, I was trying to keep that behavior, changing it in a followup PR.

burmudar · 2026-03-11T08:18:23Z

cmd/src/login_oauth.go

 	if err != nil {
 		printLoginProblem(p.out, fmt.Sprintf("OAuth Device flow authentication failed: %s", err))
-		fmt.Fprintln(p.out, loginAccessTokenMessage(endpointArg))
+		fmt.Fprintln(p.out, loginAccessTokenMessage(p.cfg.endpointURL))


Note that we are preferring the configured endpointURL over the provided argument endpoint here

burmudar · 2026-03-11T08:40:04Z

cmd/src/login.go

 			return err
 		}
-		endpoint := cfg.Endpoint
+


I think after successful parse, we should override whatever is configured in cfg for the endpoint.

So cfg should be source of truth hence forth.

Indeed it should, and after a followup PR, it will be!

burmudar · 2026-03-11T08:40:42Z

cmd/src/login.go

+	out              io.Writer
+	apiFlags         *api.Flags
+	oauthClient      oauth.Client
+	loginEndpointURL *url.URL


We're using cfg.EndpointURL and never use this endpointURL anymore - so we can remove it

That'll be the intent of a follow up PR

Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>

peterguy changed the title ~~refactor: use parsed *url.URL types instead of string endpoints~~ refactor: validate and parse the endpoint up front instead of keeping it as a string Mar 7, 2026

peterguy commented Mar 7, 2026

View reviewed changes

cmd/src/code_intel_upload.go Outdated Show resolved Hide resolved

peterguy commented Mar 7, 2026

View reviewed changes

cmd/src/login.go Outdated Show resolved Hide resolved

peterguy commented Mar 7, 2026

View reviewed changes

peterguy changed the title ~~refactor: validate and parse the endpoint up front instead of keeping it as a string~~ refactor: validate and parse the endpoint and proxy at program load Mar 8, 2026

peterguy self-assigned this Mar 8, 2026

peterguy marked this pull request as ready for review March 8, 2026 01:34

peterguy requested a review from a team March 8, 2026 01:34

peterguy mentioned this pull request Mar 8, 2026

Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY #1260

Open

bahrmichael reviewed Mar 9, 2026

View reviewed changes

internal/api/api.go Outdated Show resolved Hide resolved

bahrmichael approved these changes Mar 9, 2026

View reviewed changes

keegancsmith self-requested a review March 9, 2026 09:50

keegancsmith reviewed Mar 9, 2026

View reviewed changes

cmd/src/batch_remote.go Outdated Show resolved Hide resolved

keegancsmith approved these changes Mar 9, 2026

View reviewed changes

internal/api/api.go Outdated Show resolved Hide resolved

peterguy force-pushed the peterguy/clean-up-config branch from cab95e8 to 97c1b0a Compare March 10, 2026 01:53

peterguy commented Mar 11, 2026

View reviewed changes

cmd/src/login.go Show resolved Hide resolved

refactor: validate and parse the endpoint and proxy at program load

9ad25e5

Amp-Thread-ID: https://ampcode.com/threads/T-019cdb3f-f7de-750b-b4c3-13762c7dfc11 Co-authored-by: Amp <amp@ampcode.com>

peterguy force-pushed the peterguy/clean-up-config branch from 5ccd030 to 9ad25e5 Compare March 11, 2026 06:01

burmudar reviewed Mar 11, 2026

View reviewed changes

Update internal/oauth/flow.go

93dc82a

Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>

	loginEndpointURL: loginEndpointURL,
	endpointURL: loginEndpointURL,

Conversation

peterguy commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

config struct (cmd/src/main.go)

api.ClientOpts (internal/api/api.go)

login command (cmd/src/login.go)

code_intel_upload (cmd/src/code_intel_upload.go)

Tests

Testing

Uh oh!

Uh oh!

Uh oh!

peterguy Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterguy commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bahrmichael left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keegancsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peterguy commented Mar 7, 2026 •

edited

Loading

`config` struct (`cmd/src/main.go`)

`api.ClientOpts` (`internal/api/api.go`)

`login` command (`cmd/src/login.go`)

`code_intel_upload` (`cmd/src/code_intel_upload.go`)

peterguy Mar 7, 2026 •

edited

Loading

peterguy commented Mar 8, 2026 •

edited

Loading