Address PR review feedback: config lockout, OAuth logging, temp cleanup

- Move HTTPS enforcement from config.Load() to PersistentPreRunE, skip
  for "config" subcommands so users can fix a bad base_url without lockout
- Route discoverOAuth messages through LoginOptions.Logger instead of
  writing directly to os.Stderr
- Clean up stale temp files on non-Windows rename failure in both
  atomicWriteFile (config.go) and saveAllToFile (keyring.go)

Author: jeremy

internal/auth/auth.go

@@ -241,7 +241,7 @@ func (m *Manager) Login(ctx context.Context, opts LoginOptions) error {
 	credKey := m.credentialKey()
 
 	// Discover OAuth config
-	oauthCfg, oauthType, err := m.discoverOAuth(ctx)
+	oauthCfg, oauthType, err := m.discoverOAuth(ctx, opts.log)
 	if err != nil {
 		return err
 	}
@@ -293,11 +293,11 @@ func (m *Manager) Logout() error {
 	return m.store.Delete(credKey)
 }
 
-func (m *Manager) discoverOAuth(ctx context.Context) (*oauth.Config, string, error) {
+func (m *Manager) discoverOAuth(ctx context.Context, log func(string)) (*oauth.Config, string, error) {
 	discoverer := oauth.NewDiscoverer(m.httpClient)
 	cfg, err := discoverer.Discover(ctx, m.cfg.BaseURL)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "warning: OAuth discovery failed for %s, using Launchpad fallback\n", m.cfg.BaseURL)
+		log(fmt.Sprintf("warning: OAuth discovery failed for %s, using Launchpad fallback", m.cfg.BaseURL))
 		// Fallback to Launchpad
 		lpURL, lpErr := m.launchpadURL()
 		if lpErr != nil {
@@ -307,10 +307,10 @@ func (m *Manager) discoverOAuth(ctx context.Context) (*oauth.Config, string, err
 			AuthorizationEndpoint: lpURL + "/authorization/new",
 			TokenEndpoint:         lpURL + "/authorization/token",
 		}
-		fmt.Fprintf(os.Stderr, "Authenticating via launchpad (%s)\n", fallbackCfg.AuthorizationEndpoint)
+		log(fmt.Sprintf("Authenticating via launchpad (%s)", fallbackCfg.AuthorizationEndpoint))
 		return fallbackCfg, "launchpad", nil
 	}
-	fmt.Fprintf(os.Stderr, "Authenticating via bc3 (%s)\n", cfg.AuthorizationEndpoint)
+	log(fmt.Sprintf("Authenticating via bc3 (%s)", cfg.AuthorizationEndpoint))
 	return cfg, "bc3", nil
 }
 

internal/auth/auth_test.go

@@ -443,7 +443,8 @@ func TestDiscoverOAuth_PropagatesInsecureLaunchpadError(t *testing.T) {
 	// Set insecure Launchpad URL — should cause hard error, not silent fallback.
 	t.Setenv("BASECAMP_LAUNCHPAD_URL", "http://evil.example.com")
 
-	_, _, err := m.discoverOAuth(context.Background())
+	noop := func(string) {}
+	_, _, err := m.discoverOAuth(context.Background(), noop)
 	require.Error(t, err, "insecure launchpad URL error must propagate through discoverOAuth")
 	assert.Contains(t, err.Error(), "BASECAMP_LAUNCHPAD_URL")
 }

internal/auth/keyring.go

@@ -159,12 +159,15 @@ func (s *Store) saveAllToFile(all map[string]*Credentials) error {
 	// Windows: rename fails when destination exists. Try rename first to
 	// preserve the old file on unrelated errors; only remove+retry on failure.
 	destPath := s.credentialsPath()
-	if err := os.Rename(tmpPath, destPath); err != nil && runtime.GOOS == "windows" {
-		_ = os.Remove(destPath)
-		return os.Rename(tmpPath, destPath)
-	} else { //nolint:revive // else-with-return kept for clarity of the two-branch pattern
+	if err := os.Rename(tmpPath, destPath); err != nil {
+		if runtime.GOOS == "windows" {
+			_ = os.Remove(destPath)
+			return os.Rename(tmpPath, destPath)
+		}
+		os.Remove(tmpPath) // Clean up stale temp on failure
 		return err
 	}
+	return nil
 }
 
 func (s *Store) loadFromFile(origin string) (*Credentials, error) {

internal/cli/root.go

@@ -67,16 +67,31 @@ func NewRootCmd() *cobra.Command {
 					Todolist: flags.Todolist,
 					CacheDir: flags.CacheDir,
 				})
-				// Re-validate: profile may have changed base_url
-				if err := hostutil.RequireSecureURL(cfg.BaseURL); err != nil {
-					return fmt.Errorf("base_url (from profile %q): %w", profileName, err)
-				}
 				// Profile-scoped cache (only if cache dir was not explicitly set via flag or env)
 				if flags.CacheDir == "" && os.Getenv("BASECAMP_CACHE_DIR") == "" {
 					cfg.CacheDir = filepath.Join(cfg.CacheDir, "profiles", profileName)
 				}
 			}
 
+			// Enforce HTTPS for non-localhost base_url.
+			// Skip for "config" subcommands so users can fix a bad base_url
+			// without being locked out by the validation they need to repair.
+			if !isConfigCmd(cmd) {
+				if err := hostutil.RequireSecureURL(cfg.BaseURL); err != nil {
+					source := cfg.Sources["base_url"]
+					if source == "" {
+						source = "unknown"
+					}
+					return fmt.Errorf("base_url (%s): %w\nFix with: basecamp config unset base_url", source, err)
+				}
+				if profileName != "" {
+					// Re-validate: profile may have changed base_url
+					if err := hostutil.RequireSecureURL(cfg.BaseURL); err != nil {
+						return fmt.Errorf("base_url (from profile %q): %w\nFix with: basecamp config unset base_url", profileName, err)
+					}
+				}
+			}
+
 			// Resolve behavior preferences: explicit flag > config > version.IsDev()
 			resolvePreferences(cmd, cfg, &flags)
 
@@ -364,6 +379,17 @@ func promptForProfile(cfg *config.Config) (string, error) {
 
 // transformCobraError transforms Cobra's default error messages to match the
 // Bash CLI format for consistency with existing tests and user expectations.
+// isConfigCmd returns true if cmd is "config" or any of its subcommands.
+// Used to skip HTTPS enforcement so users can repair a bad base_url.
+func isConfigCmd(cmd *cobra.Command) bool {
+	for c := cmd; c != nil; c = c.Parent() {
+		if c.Name() == "config" {
+			return true
+		}
+	}
+	return false
+}
+
 func transformCobraError(err error) error {
 	msg := err.Error()
 

internal/commands/config.go

@@ -417,12 +417,15 @@ func atomicWriteFile(path string, data []byte) error {
 	// Unix: rename atomically replaces the destination.
 	// Windows: rename fails when destination exists. Try rename first to
 	// preserve the old file on unrelated errors; only remove+retry on failure.
-	if err := os.Rename(tmpPath, path); err != nil && runtime.GOOS == "windows" {
-		_ = os.Remove(path)
-		return os.Rename(tmpPath, path)
-	} else { //nolint:revive // else-with-return kept for clarity of the two-branch pattern
+	if err := os.Rename(tmpPath, path); err != nil {
+		if runtime.GOOS == "windows" {
+			_ = os.Remove(path)
+			return os.Rename(tmpPath, path)
+		}
+		os.Remove(tmpPath) // Clean up stale temp on failure
 		return err
 	}
+	return nil
 }
 
 func newConfigProjectCmd() *cobra.Command {

internal/config/config.go

@@ -7,8 +7,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-
-	"github.com/basecamp/basecamp-cli/internal/hostutil"
 )
 
 // Config holds the resolved configuration.
@@ -122,11 +120,6 @@ func Load(overrides FlagOverrides) (*Config, error) {
 	// Apply flag overrides
 	ApplyOverrides(cfg, overrides)
 
-	// Enforce HTTPS for non-localhost base_url
-	if err := hostutil.RequireSecureURL(cfg.BaseURL); err != nil {
-		return nil, fmt.Errorf("base_url: %w", err)
-	}
-
 	return cfg, nil
 }