fix: address phase1 cli review feedback

Author: keithah

cmd/collect/README.md

@@ -51,15 +51,19 @@ only `api_url` + `api_key` into `~/.wakatime.cfg`, preserving existing plugin
 settings such as `debug`, `include`, and `exclude`. It also honors
 `STINT_API_URL` and `STINT_API_KEY` for scripted installs.
 
+For WakaTime-compatible `stint` commands such as heartbeats, stats, and editor
+plugin calls, credentials are resolved as: explicit flags, `STINT_*` env,
+`~/.stint.cfg`, `~/.wakatime.cfg`, then built-in defaults. `WAKATIME_API_KEY`
+is accepted as a later API-key fallback for compatibility.
+
 ## Configuration
 
 Collector settings are resolved from these layers, **highest precedence first**:
 
 1. **Explicit command-line flags** (e.g. `--api-url`)
 2. **Environment variables** (`STINT_API_URL`, ...)
 3. **Collector config file** (`~/.stint/collect.json`, override with `--config PATH`)
-4. **Native Stint config** (`~/.stint.cfg`) and WakaTime plugin config (`~/.wakatime.cfg`) for API credentials
-5. **Built-in defaults**
+4. **Built-in defaults**
 
 A flag only overrides lower layers when you actually pass it; a flag left at its
 default does not clobber an env var or config file value.

internal/stintcli/cli.go

@@ -57,10 +57,6 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) error {
 			return runHeartbeatsList(args[1:], stdout)
 		case "config":
 			return runConfig(args[1:], stdout)
-		case "setup":
-			return runSetup(args[1:], stdout)
-		case "cli":
-			return runCLICommand(args[1:], stdout)
 		case "today":
 			return runToday(args[1:], stdout)
 		case "today-goal":

internal/stintcli/cobra.go

@@ -28,14 +28,20 @@ func newCobraRoot(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
 		Use:   "setup",
 		Short: "Write Stint and WakaTime-compatible config",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runSetup(append(cobraFlags(cmd), args...), stdout)
+			if commandWantsHelp(args) {
+				return cmd.Help()
+			}
+			return runSetup(args, stdout)
 		},
 		DisableFlagParsing: true,
 	})
 	root.AddCommand(&cobra.Command{
 		Use:   "collect",
 		Short: "Scan local AI agent data and post usage events",
 		RunE: func(cmd *cobra.Command, args []string) error {
+			if commandWantsHelp(args) {
+				return cmd.Help()
+			}
 			return runCollect(args, stdin, stdout, stderr)
 		},
 		DisableFlagParsing: true,
@@ -45,6 +51,9 @@ func newCobraRoot(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
 		Use:   "install",
 		Short: "Install the pinned upstream wakatime-cli",
 		RunE: func(cmd *cobra.Command, args []string) error {
+			if commandWantsHelp(args) {
+				return cmd.Help()
+			}
 			return runWakaTimeCLIInstall(args, stdout)
 		},
 		DisableFlagParsing: true,
@@ -62,9 +71,11 @@ func cobraManagedCommand(command string) bool {
 	}
 }
 
-func cobraFlags(cmd *cobra.Command) []string {
-	if cmd == nil {
-		return nil
+func commandWantsHelp(args []string) bool {
+	for _, arg := range args {
+		if arg == "--help" || arg == "-h" {
+			return true
+		}
 	}
-	return cmd.Flags().Args()
+	return false
 }

internal/stintcli/cobra_test.go

@@ -0,0 +1,27 @@
+package stintcli
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	_ "modernc.org/sqlite"
+)
+
+func TestCobraManagedCommandsShowHelp(t *testing.T) {
+	for _, args := range [][]string{
+		{"setup", "--help"},
+		{"collect", "--help"},
+		{"cli", "install", "--help"},
+	} {
+		t.Run(strings.Join(args, " "), func(t *testing.T) {
+			var out bytes.Buffer
+			if err := Run(args, nil, &out, &bytes.Buffer{}); err != nil {
+				t.Fatal(err)
+			}
+			if !strings.Contains(out.String(), "Usage:") {
+				t.Fatalf("expected help output, got %q", out.String())
+			}
+		})
+	}
+}

internal/stintcli/options.go

@@ -356,7 +356,10 @@ func parseCommonWithFlagSet(fs *flag.FlagSet, args []string) (Options, error) {
 	if err != nil {
 		return o, err
 	}
-	nativeCfg := loadNativeConfig()
+	nativeCfg, err := loadNativeConfig()
+	if err != nil {
+		return o, fmt.Errorf("load native config: %w", err)
+	}
 	o.Config = cfg
 	internalCfg, _ := LoadConfig(o.InternalConfigPath)
 	o.InternalConfig = internalCfg
@@ -724,16 +727,24 @@ func resolveAPIKeyFromConfigs(key string, configs []Config, fallbacks ...string)
 	if key != "" {
 		return key, nil
 	}
+	var vaultErr error
 	for _, cfg := range configs {
 		apiKey, err := resolveAPIKeyFromConfig(cfg)
 		if err != nil {
-			return "", err
+			vaultErr = err
+			continue
 		}
 		if apiKey != "" {
 			return apiKey, nil
 		}
 	}
-	return first(fallbacks...), nil
+	if fallback := first(fallbacks...); fallback != "" {
+		return fallback, nil
+	}
+	if vaultErr != nil {
+		return "", vaultErr
+	}
+	return "", nil
 }
 
 func resolveAPIKeyFromConfig(cfg Config, fallbacks ...string) (string, error) {

internal/stintcli/setup_test.go

@@ -101,6 +101,87 @@ func TestParseCommonUsesEnvBeforeStintConfig(t *testing.T) {
 	}
 }
 
+func TestParseCommonErrorsWhenNativeConfigCannotBeRead(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("permission fixture is not meaningful as root")
+	}
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	t.Setenv("WAKATIME_HOME", home)
+	if err := os.WriteFile(DefaultStintConfigPath(), []byte("[settings]\napi_key = waka_native\n"), 0o000); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(DefaultStintConfigPath(), 0o600) })
+
+	_, err := parseCommon(nil)
+	if err == nil || !strings.Contains(err.Error(), "load native config") {
+		t.Fatalf("expected native config load error, got %v", err)
+	}
+}
+
+func TestParseCommonFallsBackToWakaTimeConfigWhenNativeVaultFails(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	t.Setenv("WAKATIME_HOME", home)
+
+	native := Config{Sections: map[string]map[string]string{}}
+	native.Set("settings", "api_key_vault_cmd", "exit 9")
+	if err := native.Write(DefaultStintConfigPath()); err != nil {
+		t.Fatal(err)
+	}
+	waka := Config{Sections: map[string]map[string]string{}}
+	waka.Set("settings", "api_key", "waka_fallback")
+	if err := waka.Write(DefaultWakaTimeConfigPath()); err != nil {
+		t.Fatal(err)
+	}
+
+	opts, err := parseCommon(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if opts.APIKey != "waka_fallback" {
+		t.Fatalf("expected wakatime fallback key, got %q", opts.APIKey)
+	}
+}
+
+func TestSetupDoesNotUpdateNativeConfigWhenWakaTimeWriteCannotBePrepared(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("permission fixture is not meaningful as root")
+	}
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	t.Setenv("WAKATIME_HOME", home)
+
+	original := Config{Sections: map[string]map[string]string{}}
+	original.Set("settings", "api_url", "https://old.example.com/api/v1")
+	original.Set("settings", "api_key", "waka_old")
+	if err := original.Write(DefaultStintConfigPath()); err != nil {
+		t.Fatal(err)
+	}
+	blockedDir := filepath.Join(home, "blocked")
+	if err := os.Mkdir(blockedDir, 0o500); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(blockedDir, 0o700) })
+
+	err := Run([]string{
+		"setup",
+		"--server", "https://new.example.com/api/v1",
+		"--key", "waka_new",
+		"--wakatime-config", filepath.Join(blockedDir, ".wakatime.cfg"),
+	}, nil, &bytes.Buffer{}, &bytes.Buffer{})
+	if err == nil {
+		t.Fatal("expected wakatime config write error")
+	}
+	cfg, loadErr := LoadConfig(DefaultStintConfigPath())
+	if loadErr != nil {
+		t.Fatal(loadErr)
+	}
+	if cfg.Get("settings", "api_url") != "https://old.example.com/api/v1" || cfg.Get("settings", "api_key") != "waka_old" {
+		t.Fatalf("native config was partially updated: %#v", cfg.Section("settings"))
+	}
+}
+
 func TestSetupHonorsEnvironmentFallbacks(t *testing.T) {
 	home := t.TempDir()
 	t.Setenv("HOME", home)

internal/stintcli/stint_config.go

@@ -27,34 +27,115 @@ func runSetup(args []string, stdout io.Writer) error {
 	if apiURL == "" || apiKey == "" {
 		return fmt.Errorf("server and key are required (use --server/--key or STINT_API_URL/STINT_API_KEY)")
 	}
-	if err := writeSetupConfig(*stintConfig, apiURL, apiKey, true); err != nil {
-		return fmt.Errorf("write stint config: %w", err)
+	stintWrite, err := prepareSetupConfig(*stintConfig, apiURL, apiKey, true)
+	if err != nil {
+		return fmt.Errorf("prepare stint config: %w", err)
 	}
-	if err := writeSetupConfig(*wakaConfig, apiURL, apiKey, false); err != nil {
-		return fmt.Errorf("write wakatime config: %w", err)
+	defer stintWrite.cleanup()
+	wakaWrite, err := prepareSetupConfig(*wakaConfig, apiURL, apiKey, false)
+	if err != nil {
+		return fmt.Errorf("prepare wakatime config: %w", err)
+	}
+	defer wakaWrite.cleanup()
+	if err := commitSetupConfigs(stintWrite, wakaWrite); err != nil {
+		return err
 	}
 	fmt.Fprintf(stdout, "wrote %s and %s\n", expandHome(*stintConfig), expandHome(*wakaConfig))
 	return nil
 }
 
 func writeSetupConfig(path, apiURL, apiKey string, native bool) error {
+	write, err := prepareSetupConfig(path, apiURL, apiKey, native)
+	if err != nil {
+		return err
+	}
+	defer write.cleanup()
+	return write.commit()
+}
+
+type preparedSetupConfig struct {
+	path      string
+	tmpPath   string
+	oldBytes  []byte
+	hadOld    bool
+	committed bool
+}
+
+func prepareSetupConfig(path, apiURL, apiKey string, native bool) (preparedSetupConfig, error) {
 	path = expandHome(path)
 	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
-		return err
+		return preparedSetupConfig{}, err
 	}
 	cfg, err := LoadConfig(path)
 	if err != nil {
-		return err
+		return preparedSetupConfig{}, err
 	}
 	cfg.Set("settings", "api_url", apiURL)
 	cfg.Set("settings", "api_key", apiKey)
 	if native {
 		cfg.Set("settings", "offline", "true")
 	}
-	return cfg.Write(path)
+	prepared := preparedSetupConfig{path: path}
+	if oldBytes, err := os.ReadFile(path); err == nil {
+		prepared.oldBytes = oldBytes
+		prepared.hadOld = true
+	} else if !os.IsNotExist(err) {
+		return preparedSetupConfig{}, err
+	}
+	tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".tmp-*")
+	if err != nil {
+		return preparedSetupConfig{}, err
+	}
+	prepared.tmpPath = tmp.Name()
+	if err := tmp.Close(); err != nil {
+		prepared.cleanup()
+		return preparedSetupConfig{}, err
+	}
+	if err := cfg.Write(prepared.tmpPath); err != nil {
+		prepared.cleanup()
+		return preparedSetupConfig{}, err
+	}
+	return prepared, nil
+}
+
+func commitSetupConfigs(configs ...preparedSetupConfig) error {
+	var committed []preparedSetupConfig
+	for _, config := range configs {
+		if err := config.commit(); err != nil {
+			for i := len(committed) - 1; i >= 0; i-- {
+				_ = committed[i].restore()
+			}
+			return err
+		}
+		committed = append(committed, config)
+	}
+	return nil
+}
+
+func (p *preparedSetupConfig) commit() error {
+	if err := os.Rename(p.tmpPath, p.path); err != nil {
+		return err
+	}
+	p.committed = true
+	return nil
+}
+
+func (p preparedSetupConfig) restore() error {
+	if p.hadOld {
+		return os.WriteFile(p.path, p.oldBytes, 0o600)
+	}
+	if err := os.Remove(p.path); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	return nil
+}
+
+func (p preparedSetupConfig) cleanup() {
+	if !p.committed && p.tmpPath != "" {
+		_ = os.Remove(p.tmpPath)
+	}
 }
 
-func loadNativeConfig() Config {
-	cfg, _ := LoadConfig(DefaultStintConfigPath())
-	return cfg
+func loadNativeConfig() (Config, error) {
+	return LoadConfig(DefaultStintConfigPath())
 }