fix: resolve hub endpoint from --base-url flag

[meatballs]

Summary

Fixes #104 — Hub endpoint ignores --base-url flag, falls back to localhost:8080.

resolveHubEndpoint() checks cfg.Hub.Endpoint and SCION_SERVER_BASE_URL env but ignores the --base-url CLI flag. When neither config nor env is set, it falls back to http://localhost:<webPort>, causing containers to receive the wrong hub endpoint.

Fix: Check webBaseURL (the --base-url flag) first, before the env var and localhost fallback. One line added.

Test plan

• go build / go vet clean
• Start hub with --base-url https://example.com — verify SCION_HUB_ENDPOINT in containers matches
• Start hub without --base-url — verify fallback to localhost still works
• Start hub with SCION_SERVER_BASE_URL env — verify env var still works

[mfreeman451]

should use guard clauses here and eliminate inline if/else

[ptone]

PR #106 Review: fix: resolve hub endpoint from --base-url flag

PR: #106
Files changed: cmd/server_foreground.go (+32 / -19)
Reviewed: 2026-04-11

---

Executive Summary

This is a low-risk, well-scoped bug fix that refactors resolveHubEndpoint() into an early-return cascade, adding webBaseURL (the --base-url CLI flag) as a resolution source. The refactor is semantically faithful to the original logic for all existing code paths while correctly inserting the missing flag check.

---

Critical Issues

None.

---

Observations

1. Subtle behavioral change in the !enableHub path (low risk)

Old code: When cfg.Hub.Endpoint == "" and enableHub == false, the old code fell into the else if branch and called brokerSettings.GetHubEndpoint().

New code: When cfg.Hub.Endpoint == "" and !enableHub, the function returns brokerSettings.GetHubEndpoint() immediately — same behavior.

However, in the old code, when enableHub == true and cfg.Hub.Endpoint != "", the function returned cfg.Hub.Endpoint directly (the first assignment hubEndpoint := cfg.Hub.Endpoint was non-empty, so neither if nor else if branch executed). The new code preserves this: cfg.Hub.Endpoint != "" returns early on line 624.

Verdict: Behavioral equivalence confirmed for all pre-existing paths. No issue.

2. Priority ordering is correct and well-documented

The new resolution order is:

1. cfg.Hub.Endpoint (explicit config)
2. brokerSettings.GetHubEndpoint() (when hub is not enabled — grove-local mode)
3. webBaseURL / --base-url flag (new)
4. SCION_SERVER_BASE_URL env var
5. http://localhost:<port> fallback

The --base-url flag taking precedence over the env var is the correct design: CLI flags should override environment variables in standard 12-factor precedence.

3. Consider trimming trailing slash on cfg.Hub.Endpoint (minor)

The webBaseURL and SCION_SERVER_BASE_URL paths both call strings.TrimRight(…, "/"), but the cfg.Hub.Endpoint early return on line 624-625 does not. If a user sets hub.endpoint: "https://example.com/" in config, it will propagate with the trailing slash, which could cause double-slash issues downstream (e.g., https://example.com//api/v1/...).

This is pre-existing behavior and not introduced by this PR, so it is non-blocking, but worth noting as a follow-up.

4. No unit test coverage for the new webBaseURL path

The test plan mentions manual verification, but there is no automated test for the new branch. Since webBaseURL is a package-level var (set via cobra flags), it could be set in a test and the function invoked directly. This would prevent regressions.

Suggested follow-up (non-blocking):

func TestResolveHubEndpoint_BaseURLFlag(t *testing.T) {
    oldWebBaseURL := webBaseURL
    oldEnableHub := enableHub
    defer func() {
        webBaseURL = oldWebBaseURL
        enableHub = oldEnableHub
    }()

    enableHub = true
    webBaseURL = "https://example.com/"

    cfg := &config.GlobalConfig{}
    result := resolveHubEndpoint(cfg, &config.Settings{})
    if result != "https://example.com" {
        t.Errorf("expected trimmed base URL, got %q", result)
    }
}

---

Positive Feedback

• Excellent refactor. The early-return style is significantly more readable than the nested if/else if it replaces. Each resolution step is self-contained and easy to reason about.
• Correct precedence. CLI flag > env var > computed default follows Go CLI conventions.
• Minimal blast radius. Single function, single file, no interface changes. The fix is surgical.
• Good PR description. Clear problem statement, root cause, and manual test plan.

---

Final Verdict

Approve. The change is correct, minimal, and improves readability. The lack of a unit test for the new path is a minor gap but not blocking given the straightforward nature of the fix.