fix(cmd): Fixed various bugs in connect.go by junaid2005p · Pull Request #393 · SurgeDM/Surge

junaid2005p · 2026-04-20T05:14:18Z

Greptile Summary

This PR fixes several bugs in the remote-connect flow: the most impactful is that StreamEvents previously returned an empty func(){} cleanup, causing the SSE goroutine to be unrecoverable after use. The PR also extracts TLS/CA configuration into a dedicated http_client.go + remote_client.go layer, renames resolveConnectBaseURL → parseConnectTarget (returning a struct for extensibility), adds IPv6 address support, and wires --insecure-http/--insecure-tls/--tls-ca-file flags end-to-end. Tests are comprehensive and the new mergeContexts helper is clean.

Confidence Score: 5/5

Safe to merge — all findings are P2 style suggestions with no blocking defects.

The primary bugs (no-op StreamEvents cleanup, missing TLS support, missing IPv6 handling) are correctly fixed with well-covered tests. The two remaining comments are minor style suggestions that do not affect correctness at the current call sites.

No files require special attention for merge safety.

Important Files Changed

Filename	Overview
cmd/connect.go	Refactored: `resolveConnectBaseURL` → `parseConnectTarget` returning a `connectTarget` struct; added IPv6 and scheme-validation logic, `isLocalHost` for auto-token resolution. Direct `service.Client.Timeout` mutation during probe is a minor fragility.
cmd/remote_client.go	New file: centralises TLS/CA/insecure globals and the `remoteClientConfig` struct; provides `newRemoteDownloadService` and `newRemoteAPIHTTPClient` factory helpers.
cmd/utils.go	Added `resolveHostTarget`, `resolveAPIConnection`, and updated `doAPIRequest` to use a TLS-aware client. `doAPIRequest` creates a new `http.Client` per call and uses `http.NewRequest` without context.
internal/core/http_client.go	New file: `HTTPClientOptions`, `NewHTTPClient`, `NewStreamingHTTPClient`, `NewHTTPTransport`, and `newTLSConfig` with custom CA/`InsecureSkipVerify` support. Implementation is clean and correct.
internal/core/remote_service.go	Key bug fix: `StreamEvents` previously returned `func(){}` as cleanup; now returns a real cancel via `mergeContexts`. Added `mergeContexts` helper, removed duplicate `s.ctx` checks from `streamWithReconnect`.
cmd/cli_test.go	New `TestConnectCmd_HostSourcesBypassLocalAutodetect` verifies `--host` and `SURGE_HOST` bypass local autodetection. Good integration coverage.
internal/core/remote_service_test.go	New file: tests SSE stream cleanup, `mergeContexts` behaviour, and service shutdown. Good coverage of the new context-merging logic.

Sequence Diagram

sequenceDiagram
    participant CLI as surge connect
    participant Utils as cmd/utils.go
    participant RC as cmd/remote_client.go
    participant Core as core/remote_service.go
    participant HTTP as core/http_client.go
    participant SSE as Remote SSE /events

    CLI->>Utils: resolveHostTarget() / parseConnectTarget()
    Utils->>RC: currentRemoteClientConfig()
    RC-->>Utils: remoteClientConfig (TLS opts)
    Utils->>RC: newRemoteDownloadService(baseURL, token)
    RC->>HTTP: NewHTTPClient(opts)
    RC->>HTTP: NewStreamingHTTPClient(opts)
    HTTP-->>RC: *http.Client (with TLS/CA)
    RC->>Core: NewRemoteDownloadService(baseURL, token, opts)
    Core-->>CLI: *RemoteDownloadService
    CLI->>Core: service.List() [probe with ConnectTimeout]
    Core-->>CLI: ok
    CLI->>Core: service.StreamEvents(streamCtx)
    Core->>Core: mergeContexts(s.ctx, streamCtx)
    Core-->>CLI: ch, cancelFn
    loop SSE reconnect loop
        Core->>SSE: GET /events (Bearer token)
        SSE-->>Core: event stream
        Core-->>CLI: ch <- decoded event
    end
    CLI->>Core: cancelFn() / service.Shutdown()
    Core->>Core: merged context cancelled, goroutine exits, ch closed

Comments Outside Diff (1)

internal/core/remote_service.go, line 248-258 (link)

cancel called twice on normal stream exit

StreamEvents returns cancel as the cleanup function and calls defer cancel() inside the goroutine. When the stream ends naturally (goroutine exits), it cancels the merged context; if the caller then calls the returned cleanup, cancel runs a second time. context.WithCancel cancels are idempotent and the mergeContexts stop-funcs are also safe to call repeatedly, so this is not a correctness bug — but it can confuse readers. Consider returning a one-shot wrapper that no-ops after the first call, or document that the returned cleanup is idempotent.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/core/remote_service.go
Line: 248-258

Comment:
**`cancel` called twice on normal stream exit**

`StreamEvents` returns `cancel` as the cleanup function _and_ calls `defer cancel()` inside the goroutine. When the stream ends naturally (goroutine exits), it cancels the merged context; if the caller then calls the returned cleanup, `cancel` runs a second time. `context.WithCancel` cancels are idempotent and the `mergeContexts` stop-funcs are also safe to call repeatedly, so this is not a correctness bug — but it can confuse readers. Consider returning a one-shot wrapper that no-ops after the first call, or document that the returned cleanup is idempotent.

How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: cmd/utils.go
Line: 110

Comment:
**Missing context in `doAPIRequest` request creation**

`http.NewRequest` is used here instead of `http.NewRequestWithContext`, so callers cannot propagate cancellation or deadlines into the outgoing request. While the `http.Client` timeout (15 s via `newRemoteAPIHTTPClient`) bounds the call, a proper context enables future callers to cancel mid-flight (e.g. SIGINT propagation for long uploads).

```suggestion
	req, err := http.NewRequestWithContext(context.Background(), method, reqURL, body)
```

If you want cancellation to propagate from callers, consider adding a `ctx context.Context` parameter to `doAPIRequest` and threading it through.

**Rule Used:** What: Prioritize readable, idiomatic Go code over ... ([source](https://app.greptile.com/review/custom-context?memory=a792d438-3101-4554-a566-e21a5e164b1f))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: cmd/connect.go
Line: 70-73

Comment:
**Direct `Timeout` mutation on a shared `*http.Client`**

`service.Client.Timeout` is overwritten directly to perform a short connectivity probe, then restored. While safe today (no goroutines have started at this point), this pattern is fragile — if a refactor starts any goroutines before this block the mutation becomes a data race on the shared `*http.Client`. Consider wrapping the probe in a dedicated helper that constructs a single-use client with the connect timeout instead of mutating the shared one:

```go
func probeConnection(service *core.RemoteDownloadService, timeout time.Duration) error {
    probeClient := *service.Client  // shallow copy — shares transport
    probeClient.Timeout = timeout
    // use probeClient to call List
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (8): Last reviewed commit: "Merge branch 'main' into connect-fix" | Re-trigger Greptile}

…t:port does not work and neither does SURGE_HOST

… command

…n to clarify the priority order

… targets is broken

…r handling

greptile-apps · 2026-04-20T05:24:32Z

+func NewRemoteDownloadService(baseURL string, token string, opts HTTPClientOptions) (*RemoteDownloadService, error) {
 	ctx, cancel := context.WithCancel(context.Background())
+	client, err := NewHTTPClient(opts)
+	if err != nil {
+		cancel()
+		return nil, err
+	}
+	sseClient, err := NewStreamingHTTPClient(opts)
+	if err != nil {
+		cancel()
+		return nil, err
+	}
 	return &RemoteDownloadService{
 		BaseURL:   baseURL,
 		Token:     token,
-		Client:    &http.Client{Timeout: 30 * time.Second},
-		SSEClient: &http.Client{},
+		Client:    client,
+		SSEClient: sseClient,
 		ctx:       ctx,
 		cancel:    cancel,
-	}
+	}, nil
 }


Missing type and function definitions — code will not compile

NewHTTPClient, NewStreamingHTTPClient, and HTTPClientOptions are called/used in NewRemoteDownloadService but are not defined anywhere in the internal/core package or any other package in the repository (confirmed via exhaustive search). Similarly, in cmd/, currentRemoteClientConfig, newRemoteAPIHTTPClient, and newRemoteDownloadService are invoked from connect.go and utils.go without a corresponding definition. The variables globalInsecureHTTP, globalInsecureTLS, and globalTLSCAFile referenced in root.go are also undeclared.

The PR appears to be missing a file (e.g., cmd/remote_client.go and/or internal/core/http_client.go) that should contain these declarations. As submitted, the package will not compile.

Prompt To Fix With AI

This is a comment left during a code review. Path: internal/core/remote_service.go Line: 31-51 Comment: **Missing type and function definitions — code will not compile** `NewHTTPClient`, `NewStreamingHTTPClient`, and `HTTPClientOptions` are called/used in `NewRemoteDownloadService` but are not defined anywhere in the `internal/core` package or any other package in the repository (confirmed via exhaustive search). Similarly, in `cmd/`, `currentRemoteClientConfig`, `newRemoteAPIHTTPClient`, and `newRemoteDownloadService` are invoked from `connect.go` and `utils.go` without a corresponding definition. The variables `globalInsecureHTTP`, `globalInsecureTLS`, and `globalTLSCAFile` referenced in `root.go` are also undeclared. The PR appears to be missing a file (e.g., `cmd/remote_client.go` and/or `internal/core/http_client.go`) that should contain these declarations. As submitted, the package will not compile. How can I resolve this? If you propose a fix, please make it concise.

github-actions · 2026-04-20T06:33:14Z

Binary Size Analysis

✅ Size Decreased or Stable

Version	Human Readable	Raw Bytes
Main	18.85 MB	`19767588`
PR	18.85 MB	`19763492`
Difference	-4.00 KB	`-4096`

…rrors

…onnect-fix

junaid2005p and others added 8 commits April 18, 2026 01:42

test(cli): Added a failing case to show that surge connect --host hos…

17f9b50

…t:port does not work and neither does SURGE_HOST

fix(connect): Added a fix to consier --host and SURGE_HOST in connect…

13fb48e

… command

docs(utils): Added a small comment on the resolveHostTarget() functio…

f1769fa

…n to clarify the priority order

test(cmd): Added a failing testcase to show hosname parsing from ipv6…

7011aab

… targets is broken

refactor(connect): centralize target parsing and tighten connect erro…

6a55644

…r handling

fix(connect): shut down remote SSE stream on TUI exit

d0f65ab

fix(cmd): fixed insecureHTTP issue added no TLS

ea30bb1

Merge branch 'main' into connect-fix

2f562fc

greptile-apps Bot reviewed Apr 20, 2026

View reviewed changes

junaid2005p added 2 commits April 20, 2026 11:58

feat(cmd): unify cmd remote calling

bef3fa7

feat(internal): Added remaining helpers

9a0f7d5

junaid2005p and others added 7 commits April 20, 2026 12:11

fix(connect): send pre-TUI status to stderr and clarify remote auth e…

bb1a377

…rrors

test(tui): Fixed a failing test

d93f93b

Merge branch 'main' into connect-fix

9610daa

Merge branch 'main' into connect-fix

5f293b7

test: fixed failing tests

3078c4e

Merge branch 'connect-fix' of github.com:JunaidIslam2105/surge into c…

4d86378

…onnect-fix

Merge branch 'main' into connect-fix

8d4c8f9

junaid2005p merged commit 40bb4ea into main Apr 20, 2026
16 checks passed

junaid2005p deleted the connect-fix branch April 20, 2026 07:23

junaid2005p requested a review from SuperCoolPencil April 20, 2026 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cmd): Fixed various bugs in connect.go#393

fix(cmd): Fixed various bugs in connect.go#393
junaid2005p merged 17 commits intomainfrom
connect-fix

junaid2005p commented Apr 20, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

greptile-apps Bot Apr 20, 2026

Uh oh!

github-actions Bot commented Apr 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

junaid2005p commented Apr 20, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Analysis

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

junaid2005p commented Apr 20, 2026 •

edited by greptile-apps Bot

Loading

github-actions Bot commented Apr 20, 2026 •

edited

Loading