fix: Go-side Lows batched on #399

internal/cli/client.go

@@ -66,10 +66,23 @@ type Client struct {
 // web UI's address (only `whr github app create` uses it): the API is on a unix
 // socket in the state directory (D29, §7.5).
 type ClientConfig struct {
-	Listen       string `json:"listen"`
-	APITokenFile string `json:"api_token_file"`
-	StateDir     string `json:"state_dir"`
-	PublicURL    string `json:"public_url"`
+	Listen       string        `json:"listen"`
+	APITokenFile string        `json:"api_token_file"`
+	StateDir     string        `json:"state_dir"`
+	PublicURL    lenientString `json:"public_url"`
+}
+
+// lenientString decodes a JSON string and leaves any other value empty, so a
+// malformed public_url cannot make every client command fail; `whr serve`
+// validates the whole file.
+type lenientString string
+
+func (l *lenientString) UnmarshalJSON(b []byte) error {
+	var s string
+	if json.Unmarshal(b, &s) == nil {
+		*l = lenientString(s)
+	}
+	return nil
 }
 
 // ReadClientConfig reads listen, api_token_file and state_dir from the

internal/cli/doctor.go

@@ -104,6 +104,9 @@ func newDoctor(st *state) *cobra.Command {
 			}
 			rs := doctor.Run(cmd.Context(), checks, skipped)
 			repair := repairContext{Account: whrUser}
+			if cmd.Flags().Changed("prefix") {
+				repair.Prefix = prefix
+			}
 			for i, r := range rs {
 				context := repair
 				if other { // command adds it to the fixes that are for whr's account only

internal/cli/github.go

@@ -86,16 +86,17 @@ It listens on the configuration's "listen" address while it waits, so stop
 			if ip := net.ParseIP(host); err != nil || ip == nil || !ip.IsLoopback() {
 				return usageError{fmt.Sprintf("--listen %q is not a loopback address: the forwarder reaches whr there (D29)", listen)}
 			}
+			fromFlag := publicURL != ""
 			switch {
 			case local:
 				publicURL = "http://" + listen
 			case publicURL == "" && ccErr == nil:
-				publicURL = cc.PublicURL
+				publicURL = string(cc.PublicURL)
 			}
 			if publicURL == "" {
 				return usageError{"no public name: set public_url in the configuration (whr setup asks for it) or pass --public-url whr.example.ts.net; to try it on the host itself, pass --local and open the link in a browser on this Mac"}
 			}
-			if !local && !loopbackHTTP(publicURL) {
+			if !local && (!fromFlag || !loopbackHTTP(publicURL)) {
 				n, err := config.NormalizePublicURL(publicURL)
 				if err != nil {
 					return usageError{"public name: " + err.Error()}
@@ -145,7 +146,9 @@ It listens on the configuration's "listen" address while it waits, so stop
 				ui := render.Writer{W: st.env.Stderr, S: st.style(st.env.Stderr, false)}
 				ui.Command("tailscale serve status")
 				fmt.Fprintln(st.env.Stderr, "To create the mapping (never use a public funnel):")
-				ui.Command("tailscale serve --bg " + port)
+				if isDigits(port) {
+					ui.Command("tailscale serve --bg " + port)
+				}
 				fmt.Fprintln(st.env.Stderr, "Or use --local on this Mac.")
 			}
 
@@ -185,8 +188,21 @@ It listens on the configuration's "listen" address while it waits, so stop
 	return cmd
 }
 
+func isDigits(s string) bool {
+	if s == "" {
+		return false
+	}
+	for _, r := range s {
+		if r < '0' || r > '9' {
+			return false
+		}
+	}
+	return true
+}
+
 // loopbackHTTP is an explicit http:// address on a loopback host: a test double
-// or a local trial, which githubapp.CheckBaseURL still checks.
+// or a local trial named with --public-url only (a configuration value is normalised
+// like config.Load and the doctor do), which githubapp.CheckBaseURL still checks.
 func loopbackHTTP(raw string) bool {
 	u, err := url.Parse(raw)
 	if err != nil || u.Scheme != "http" {

internal/cli/github_test.go

@@ -195,3 +195,31 @@ func TestGitHubAppCreateWithoutAPublicNameSaysWhatToDo(t *testing.T) {
 		t.Errorf("--local: %q", stderr.String())
 	}
 }
+
+// A loopback http:// public name is a test double the flag may name; a
+// configuration value goes through the same normalisation as config.Load and
+// the doctor (issue #399).
+func TestGitHubAppCreateRefusesAnHTTPPublicURLFromTheConfiguration(t *testing.T) {
+	addr := freeAddr(t)
+	dir := t.TempDir()
+	cfg := filepath.Join(dir, "config.json")
+	body := fmt.Sprintf(`{"listen": %q, "api_token_file": "/x", "public_url": "http://127.0.0.1:9"}`, addr)
+	if err := os.WriteFile(cfg, []byte(body), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	done, _, stderr := runCLIAsync("github", "app", "create", "--config", cfg, "--ttl", "1s", "--key-dir", dir)
+	if code := <-done; code != exitcode.Usage || !strings.Contains(stderr.String(), "public name") {
+		t.Errorf("exit %d, stderr %q", code, stderr.String())
+	}
+}
+
+func TestReadClientConfigIgnoresANonStringPublicURL(t *testing.T) {
+	cfg := filepath.Join(t.TempDir(), "config.json")
+	if err := os.WriteFile(cfg, []byte(`{"api_token_file": "/x", "public_url": 5}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	cc, err := ReadClientConfig(cfg)
+	if err != nil || cc.PublicURL != "" {
+		t.Fatalf("got %+v, %v", cc, err)
+	}
+}

internal/cli/help.go

@@ -12,7 +12,7 @@ import (
 )
 
 // descColumn finds where a flag or command description starts on a line like
-// "      --dev   use a ...": text, two or more spaces, then more text.
+// "      --listen   use a ...": text, two or more spaces, then more text.
 var descColumn = regexp.MustCompile(`^( *\S.*?\S {2,})\S`)
 
 // hangWrap wraps every line over 80 columns at spaces, except a command line

internal/cli/prefix_setup_test.go

@@ -28,7 +28,6 @@ func userPrefixRig(t *testing.T) (*setupRig, string) {
 	}
 	r.env.Executable = func() (string, error) { return r.exe, nil }
 	r.env.UID = os.Getuid() // the checks compare owners with the running account
-	r.host.outputs["/usr/bin/dscl . -read /Users/werner NFSHomeDirectory"] = "NFSHomeDirectory: " + home + "\n"
 	return r, home
 }
 
@@ -268,4 +267,11 @@ func TestAUserOwnedPrefixWorksWithAWarning(t *testing.T) {
 	if !strings.Contains(out, "warn\tprefix\t") || strings.Contains(out, "fail\tprefix\t") && !strings.Contains(out, "does not exist") {
 		t.Fatalf("stdout %q, stderr %q", out, errOut)
 	}
+	// a fix line that names whr setup keeps the prefix the doctor was given
+	// (issue #399)
+	for _, line := range strings.Split(out, "\n") {
+		if strings.Contains(line, "whr setup") && !strings.Contains(line, "--prefix") {
+			t.Errorf("a fix line lacks the prefix: %q", line)
+		}
+	}
 }

internal/cli/setup_test.go

@@ -297,11 +297,12 @@ func TestDoctorRunsEveryCheckReadOnlyAndNamesTheFix(t *testing.T) {
 	if idx("power") >= idx("config-dir") || idx("config-dir") >= idx("service-install") {
 		t.Errorf("host steps, then user steps, in the wizard's order: %v", order)
 	}
-	if got := lines["power"][3]; got != "whr setup host --only power" {
+	pfx := " --prefix " + shellArgument(filepath.Dir(filepath.Dir(r.exe)))
+	if got := lines["power"][3]; got != "whr setup host --only power"+pfx {
 		t.Errorf("power fix %q", got)
 	}
 	// run as werner, not whr: the user phase says so, and says to run as workharbor
-	if f := lines["config-base"]; f[0] != "not_verified" || !strings.Contains(f[2], "check it as workharbor") || f[3] != "whr setup --only config-base (run as workharbor)" {
+	if f := lines["config-base"]; f[0] != "not_verified" || !strings.Contains(f[2], "check it as workharbor") || f[3] != "whr setup --only config-base"+pfx+" (run as workharbor)" {
 		t.Errorf("config-base: %q", f)
 	}
 	if f := lines["config"]; f[0] != "fail" || !strings.Contains(f[3], "whr setup") {
@@ -448,9 +449,10 @@ func TestDoctorRepairsKeepSelectedAccount(t *testing.T) {
 	for _, account := range []string{"workharbor", "whr", "operator", "operator's"} {
 		r := newSetupRig(t)
 		_, out, _ := r.run("doctor", "--user", account)
-		suffix := ""
+		// the doctor keeps the prefix it was given (issue #399)
+		suffix := " --prefix " + shellArgument(filepath.Dir(filepath.Dir(r.exe)))
 		if account != "workharbor" {
-			suffix = " --user " + shellArgument(account)
+			suffix += " --user " + shellArgument(account)
 		}
 		want := "whr setup host --only workharbor-user" + suffix
 		if !strings.Contains(out, want) {
@@ -565,7 +567,7 @@ func TestTheHostPartWarnsWhenTheAccountCannotSudo(t *testing.T) {
 	if code == exitcode.OK || code == exitcode.Usage {
 		t.Errorf("non-admin: exit %d, stderr %q", code, errOut)
 	}
-	for _, want := range []string{"cannot sudo", "sudo pmset", "an administrator"} {
+	for _, want := range []string{"cannot sudo", "sudo pmset", "an administrator", "need an administrator (sudo)", "this run is not complete"} {
 		if !strings.Contains(errOut, want) {
 			t.Errorf("non-admin: stderr lacks %q: %q", want, errOut)
 		}