Apply review feedback

- Harden SpawnSendTelemetry against relative executable paths
- Use io.Copy for telemetry subprocess stdin write
- Clean up GH_TELEMETRY/DO_NOT_TRACK help text
- Fall back to built-in defaults (NoOp telemetry) on config load failure

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

internal/ghcmd/cmd.go

@@ -52,13 +52,18 @@ func Main() exitCode {
 	buildVersion := build.Version
 	hasDebug, _ := utils.IsDebugEnabled()
 
-	cfg, err := config.NewConfig()
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "failed to load config: %s\n", err)
-		return exitError
+	cfg, cfgErr := config.NewConfig()
+	if cfgErr != nil {
+		fmt.Fprintf(os.Stderr, "warning: failed to load config: %s\n", cfgErr)
 	}
+	cfgFunc := func() (gh.Config, error) { return cfg, cfgErr }
 
-	ioStreams := newIOStreams(cfg)
+	var ioStreams *iostreams.IOStreams
+	if cfgErr == nil {
+		ioStreams = newIOStreams(cfg)
+	} else {
+		ioStreams = iostreams.System()
+	}
 	stderr := ioStreams.ErrOut
 
 	ghExecutablePath := executablePath("gh")
@@ -70,9 +75,13 @@ func Main() exitCode {
 	}
 
 	var telemetryService ghtelemetry.Service
-	if os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg) {
+	switch {
+	case cfgErr != nil:
+		// Without a valid on-disk config we can't honour user telemetry preferences, so disable it to be safe.
 		telemetryService = &telemetry.NoOpService{}
-	} else {
+	case os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg):
+		telemetryService = &telemetry.NoOpService{}
+	default:
 		telemetryState := telemetry.ParseTelemetryState(cfg.Telemetry().Value)
 		switch telemetryState {
 		case telemetry.Disabled:
@@ -100,12 +109,14 @@ func Main() exitCode {
 	}
 	defer telemetryService.Flush()
 
-	cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfg, ioStreams, ghExecutablePath, telemetryService)
+	cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfgFunc, ioStreams, ghExecutablePath, telemetryService)
 
-	var m migration.MultiAccount
-	if err := cfg.Migrate(m); err != nil {
-		fmt.Fprintln(stderr, err)
-		return exitError
+	if cfgErr == nil {
+		var m migration.MultiAccount
+		if err := cfg.Migrate(m); err != nil {
+			fmt.Fprintln(stderr, err)
+			return exitError
+		}
 	}
 
 	ctx := context.Background()

internal/telemetry/telemetry.go

@@ -354,6 +354,13 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) {
 		return
 	}
 
+	// Resolve the executable to an absolute path before changing the child's
+	// working directory. Without this, a relative path (e.g. from GH_PATH) would
+	// be resolved against cmd.Dir at Start time and fail to spawn.
+	if abs, err := filepath.Abs(executable); err == nil {
+		executable = abs
+	}
+
 	cmd := exec.Command(executable, "send-telemetry")
 
 	cmd.Stdout = io.Discard
@@ -362,7 +369,10 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) {
 	// Set the working directory to a stable directory elsewhere so that the subprocess doesn't
 	// hold a reference to the parent's current working directory, avoiding any weirdness around
 	// deleting the parent process's current working directory while the child is still running.
-	cmd.Dir = os.TempDir()
+	// Only do this when we have an absolute executable path so that the child can still be found.
+	if filepath.IsAbs(executable) {
+		cmd.Dir = os.TempDir()
+	}
 
 	// Configure the child process to be detached from the parent so that it can continue running
 	// after the parent exits, and so that it doesn't receive any signals sent to the parent.
@@ -381,7 +391,8 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) {
 
 	// Write the payload synchronously into the kernel pipe buffer, then close
 	// the pipe to signal EOF. The child reads the complete payload from stdin.
-	_, _ = stdin.Write(payloadBytes)
+	// io.Copy loops until all bytes are written, avoiding any risk of a short write.
+	_, _ = io.Copy(stdin, bytes.NewReader(payloadBytes))
 	_ = stdin.Close()
 
 	// Release resources associated with the child process since we will never Wait for it.

pkg/cmd/attestation/verify/verify_integration_test.go

@@ -30,7 +30,7 @@ func TestVerifyIntegration(t *testing.T) {
 
 	ios, _, _, _ := iostreams.Test()
 	hc, err := factory.HttpClientFunc(
-		&config.AuthConfig{},
+		func() (gh.Config, error) { return config.NewBlankConfig(), nil },
 		ios,
 		"test",
 		"",
@@ -150,7 +150,7 @@ func TestVerifyIntegrationCustomIssuer(t *testing.T) {
 
 	ios, _, _, _ := iostreams.Test()
 	hc, err := factory.HttpClientFunc(
-		&config.AuthConfig{},
+		func() (gh.Config, error) { return config.NewBlankConfig(), nil },
 		ios,
 		"test",
 		"",
@@ -228,7 +228,7 @@ func TestVerifyIntegrationReusableWorkflow(t *testing.T) {
 	cfg := config.NewBlankConfig()
 	ios, _, _, _ := iostreams.Test()
 	hc, err := factory.HttpClientFunc(
-		cfg.Authentication(),
+		func() (gh.Config, error) { return cfg, nil },
 		ios,
 		"test",
 		"",
@@ -325,7 +325,7 @@ func TestVerifyIntegrationReusableWorkflowSignerWorkflow(t *testing.T) {
 	cfg := config.NewBlankConfig()
 	ios, _, _, _ := iostreams.Test()
 	hc, err := factory.HttpClientFunc(
-		cfg.Authentication(),
+		func() (gh.Config, error) { return cfg, nil },
 		ios,
 		"test",
 		"",

pkg/cmd/factory/default.go

@@ -23,19 +23,16 @@ import (
 var ssoHeader string
 var ssoURLRE = regexp.MustCompile(`\burl=([^;]+)`)
 
-func New(appVersion string, invokingAgent string, cfg gh.Config, ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory {
+func New(appVersion string, invokingAgent string, cfgFunc func() (gh.Config, error), ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory {
 	f := &cmdutil.Factory{
-		AppVersion:    appVersion,
-		InvokingAgent: invokingAgent,
-		Cfg:           cfg,
-		Config: func() (gh.Config, error) {
-			return cfg, nil
-		}, // No factory dependencies
+		AppVersion:     appVersion,
+		InvokingAgent:  invokingAgent,
+		Config:         cfgFunc,
 		ExecutablePath: executablePath,
 	}
 
 	f.IOStreams = ios
-	f.HttpClient = HttpClientFunc(cfg.Authentication(), ios, appVersion, invokingAgent, telemetryDisabler)
+	f.HttpClient = HttpClientFunc(cfgFunc, ios, appVersion, invokingAgent, telemetryDisabler)
 	f.PlainHttpClient = plainHttpClientFunc(ios, appVersion, invokingAgent, telemetryDisabler)
 	f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable
 	f.Remotes = remotesFunc(f)    // Depends on Config, and GitClient
@@ -187,10 +184,14 @@ func remotesFunc(f *cmdutil.Factory) func() (ghContext.Remotes, error) {
 	return rr.Resolver()
 }
 
-func HttpClientFunc(authCfg gh.AuthConfig, ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) {
+func HttpClientFunc(cfgFunc func() (gh.Config, error), ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) {
 	return func() (*http.Client, error) {
+		cfg, err := cfgFunc()
+		if err != nil {
+			return nil, err
+		}
 		opts := api.HTTPClientOptions{
-			Config:            authCfg,
+			Config:            cfg.Authentication(),
 			Log:               ios.ErrOut,
 			LogColorize:       ios.ColorEnabled(),
 			AppVersion:        appVersion,

pkg/cmd/factory/default_test.go

@@ -353,7 +353,7 @@ func TestSSOURL(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			cfg := config.NewBlankConfig()
 			ios, _, _, stderr := iostreams.Test()
-			client, err := HttpClientFunc(cfg.Authentication(), ios, "v1.2.3", "", &telemetry.NoOpService{})()
+			client, err := HttpClientFunc(func() (gh.Config, error) { return cfg, nil }, ios, "v1.2.3", "", &telemetry.NoOpService{})()
 			require.NoError(t, err)
 			req, err := http.NewRequest("GET", ts.URL, nil)
 			if tt.sso != "" {

pkg/cmd/root/help_topic.go

@@ -118,10 +118,10 @@ var HelpTopics = []helpTopic{
 			more compatible with speech synthesis and braille screen readers.
 
 			%[1]sGH_TELEMETRY%[1]s: set to %[1]slog%[1]s to print telemetry data to standard error instead of sending it.
-			Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry that would have been printed when set to %[1]slog%[1]s. 
-			
-			%[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry that would have been printed 
-			when %[1]sGH_TELEMETRY%[1]s is set to  %[1]slog%[1]s. %[1]sGH_TELEMETRY%[1]s takes precedence if both are set.
+			Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry. Takes precedence over %[1]sDO_NOT_TRACK%[1]s.
+
+			%[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry. Ignored when
+			%[1]sGH_TELEMETRY%[1]s is set, which takes precedence.
 
 			%[1]sGH_SPINNER_DISABLED%[1]s: set to a truthy value to replace the spinner animation with
 			a textual progress indicator.

pkg/cmdutil/factory.go

@@ -26,9 +26,13 @@ type Factory struct {
 
 	BaseRepo func() (ghrepo.Interface, error)
 	Branch   func() (string, error)
-	Cfg      gh.Config
-	// TODO: Config should be removed in favour of cfg being passed to the right place,
-	// but this is going to be very invasive and shouldn't be done as part of a feature change.
+	// It would be nice if Config were just loaded once at startup and an error
+	// were returned, but this would prevent commands like "gh version" from running.
+	// So for now, we eagerly load the config and don't fail if there is an error,
+	// and defer the error handling to commands that need it.
+	// HOWEVER, as an additional point, the root command setup currently DOES call
+	// this and errors, so we never get to "gh version" anyway.
+	// We need to revisit that, but I don't want to make it worse.
 	Config     func() (gh.Config, error)
 	HttpClient func() (*http.Client, error)
 	// PlainHttpClient is a special HTTP client that does not automatically set