From 97d3e6398eca2c17f7243575972b96ee92dd834d Mon Sep 17 00:00:00 2001 From: Tim Walsh Date: Sat, 22 Aug 2026 00:28:25 -0700 Subject: [PATCH] fix(config): layer project .cu.yml correctly and stop Save leaking it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Project config was merged with viper.Set, which writes viper's override slot — outranking flags and environment variables and inverting the documented precedence. It now merges via MergeConfigMap into the config layer, so the chain is flags > env > project .cu.yml > global config > defaults. Save serialized the whole merged viper state into the global file, so `cu config set` inside any project baked that project's values into ~/.config/cu/config.yaml. Save now starts from the file on disk and applies only values written through Set. Together these were worse than a precedence bug: with the released binary, a project .cu.yml containing api_token overwrote the real token in the global config. Credential keys from a project file are now dropped with a warning, per the context-layer design's credential blocklist. Config loading also moves entirely into config.Init. It previously ran in both cobra.OnInitialize and PersistentPreRunE, in that order, so any re-read could clobber the merged layer. Fixes #37 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014aqbmccWm1tqttmBUCR5rv ClickUp: 86dxbeqyt --- internal/cmd/root.go | 34 ++--------- internal/config/config.go | 104 +++++++++++++++++++++++++++++---- internal/config/config_test.go | 102 ++++++++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 39 deletions(-) diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 3594f0e..6c769f4 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -26,10 +26,14 @@ functionality for managing tasks, lists, spaces, and other ClickUp resources. It allows developers and teams to interact with ClickUp directly from the terminal, enabling efficient task management and seamless integration with development workflows.`, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - // Initialize configuration + // Initialize configuration. Everything — global file, project .cu.yml + // overlay, env, flags — is layered here, in one place and one order. if err := config.Init(cfgFile); err != nil { return fmt.Errorf("failed to initialize config: %w", err) } + if debug && viper.ConfigFileUsed() != "" { + fmt.Fprintln(os.Stderr, "Using config file:", viper.ConfigFileUsed()) + } return nil }, } @@ -40,8 +44,6 @@ func Execute() error { } func init() { - cobra.OnInitialize(initConfig) - // Global flags rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.config/cu/config.yml)") rootCmd.PersistentFlags().BoolVar(&debug, "debug", false, "enable debug mode") @@ -76,29 +78,3 @@ func init() { rootCmd.AddCommand(bulkCmd) rootCmd.AddCommand(exportCmd) } - -func initConfig() { - if cfgFile != "" { - // Use config file from the flag - viper.SetConfigFile(cfgFile) - } else { - // Find home directory - home, err := os.UserHomeDir() - cobra.CheckErr(err) - - // Search config in home directory with name ".cu" (without extension) - viper.AddConfigPath(home + "/.config/cu") - viper.AddConfigPath(".") - viper.SetConfigType("yaml") - viper.SetConfigName("config") - } - - // Read in environment variables that match - viper.SetEnvPrefix("CU") - viper.AutomaticEnv() - - // If a config file is found, read it in - if err := viper.ReadInConfig(); err == nil && debug { - fmt.Fprintln(os.Stderr, "Using config file:", viper.ConfigFileUsed()) - } -} diff --git a/internal/config/config.go b/internal/config/config.go index b1e70ab..3b60fdf 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -34,8 +34,37 @@ var ( // Track if we're in a project with config hasProjectConfig bool projectConfigPath string + + // globalConfigPath is the global config file discovered by Init, if any. + globalConfigPath string + // explicitConfigFile records a --config path, which always wins. + explicitConfigFile string + // staged holds values written through Set. Save applies these on top of + // whatever is already on disk, so project .cu.yml values, environment + // variables and flags can never be baked into ~/.config/cu/config.yaml. + staged = map[string]interface{}{} ) +// credentialKeys are never accepted from a project .cu.yml. That file is +// committed and reviewed like code, so honouring a token there would let any +// repository you clone substitute the credential used for API calls. +var credentialKeys = []string{"api_token"} + +// globalPath returns the global config file to write. An explicit --config +// always wins; otherwise a discovered file is used only while it still lives +// under the configured directory, since DefaultConfigDir is a variable that +// tests and tooling repoint. +func globalPath() string { + if explicitConfigFile != "" { + return explicitConfigFile + } + fallback := filepath.Join(DefaultConfigDir, ConfigFileName+"."+ConfigType) + if globalConfigPath != "" && filepath.Dir(globalConfigPath) == filepath.Clean(DefaultConfigDir) { + return globalConfigPath + } + return fallback +} + // Init initializes the configuration func Init(cfgFile string) error { // Create config directory if it doesn't exist @@ -47,6 +76,27 @@ func Init(cfgFile string) error { viper.SetDefault("output", "table") viper.SetDefault("debug", false) + // Environment variables outrank both config layers below. + viper.SetEnvPrefix("CU") + viper.AutomaticEnv() + + // --- global config layer ------------------------------------------------- + staged = map[string]interface{}{} + explicitConfigFile = cfgFile + if cfgFile != "" { + viper.SetConfigFile(cfgFile) + } else { + viper.AddConfigPath(DefaultConfigDir) + viper.AddConfigPath(".") + viper.SetConfigType(ConfigType) + viper.SetConfigName(ConfigFileName) + } + // A missing global config is normal on a fresh machine. + _ = viper.ReadInConfig() + + globalConfigPath = viper.ConfigFileUsed() + + // --- project overlay ----------------------------------------------------- // Look for project config file in current directory and parent directories projectConfigPath = findProjectConfig() if projectConfigPath != "" { @@ -56,10 +106,22 @@ func Init(cfgFile string) error { // Read project config if err := projectViper.ReadInConfig(); err == nil { - // Merge project config with main config - // Project config takes precedence - for k, v := range projectViper.AllSettings() { - viper.Set(k, v) + settings := projectViper.AllSettings() + for _, k := range credentialKeys { + if _, present := settings[k]; present { + delete(settings, k) + fmt.Fprintf(os.Stderr, + "cu: ignoring %q in %s — credentials come from the keyring, environment, or your global config\n", + k, projectConfigPath) + } + } + // MergeConfigMap merges into viper's *config* layer, so project + // values override the global file while still losing to + // environment variables and command-line flags. Using viper.Set + // here would place them in the override slot, which outranks + // everything — the inversion this replaces. + if err := viper.MergeConfigMap(settings); err != nil { + return fmt.Errorf("failed to merge project config %s: %w", projectConfigPath, err) } } } @@ -76,10 +138,27 @@ func Load() (*Config, error) { return &cfg, nil } -// Save saves the current configuration to file +// Save writes the global config file. Only values that came from that file or +// were written through Set are persisted — project .cu.yml values, environment +// variables and flags are deliberately excluded, so running `cu config set` +// inside a project can no longer bake that project's settings into the global +// config. func Save() error { - configPath := filepath.Join(DefaultConfigDir, ConfigFileName+"."+ConfigType) - return viper.WriteConfigAs(configPath) + path := globalPath() + + // Start from what is already on disk so a write can never truncate + // settings this process did not load, then apply only explicit Sets. + gv := viper.New() + gv.SetConfigFile(path) + _ = gv.ReadInConfig() + for k, v := range staged { + gv.Set(k, v) + } + + if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil { + return fmt.Errorf("failed to create config directory: %w", err) + } + return gv.WriteConfigAs(path) } // Get returns a configuration value @@ -87,9 +166,11 @@ func Get(key string) interface{} { return viper.Get(key) } -// Set sets a configuration value +// Set sets a configuration value for this process and stages it for the global +// config file, so a following Save persists it there. func Set(key string, value interface{}) { viper.Set(key, value) + staged[key] = value } // GetString returns a string configuration value @@ -209,8 +290,11 @@ func SaveProjectConfig(settings map[string]interface{}) error { // Update with new settings for k, v := range settings { projectViper.Set(k, v) - // Also update main viper - viper.Set(k, v) + } + // Reflect them in the running process at project precedence — below env and + // flags, above the global file — matching how Init loads them. + if err := viper.MergeConfigMap(settings); err != nil { + return fmt.Errorf("failed to apply project config: %w", err) } // Write the file diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d33375b..66fa243 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/spf13/pflag" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -480,3 +481,104 @@ func TestConfigSafetyChecks(t *testing.T) { } }) } + +// --- regression tests for the layering rework ------------------------------- +// +// Two bugs are covered here, both reproducible before the fix: +// 1. project .cu.yml merged via viper.Set landed in viper's override slot, +// so it beat environment variables and command-line flags. +// 2. Save serialized the whole merged state into the global file, so running +// `cu config set` inside any project baked that project's values into +// ~/.config/cu/config.yaml. + +// newLayeredFixture writes a global config and a project .cu.yml, points +// DefaultConfigDir at the former and chdirs into the latter. +func newLayeredFixture(t *testing.T, global, project string) (cfgDir, projDir string) { + t.Helper() + viper.Reset() + + tmp := t.TempDir() + cfgDir = filepath.Join(tmp, ".config", "cu") + require.NoError(t, os.MkdirAll(cfgDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(cfgDir, ConfigFileName+"."+ConfigType), []byte(global), 0o600)) + + projDir = filepath.Join(tmp, "proj") + require.NoError(t, os.MkdirAll(projDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(projDir, ProjectConfigFileName), []byte(project), 0o600)) + + old := DefaultConfigDir + DefaultConfigDir = cfgDir + t.Cleanup(func() { DefaultConfigDir = old; viper.Reset() }) + t.Chdir(projDir) + + return cfgDir, projDir +} + +func TestProjectConfigPrecedence(t *testing.T) { + t.Run("project overrides global", func(t *testing.T) { + newLayeredFixture(t, + "output: table\ndefault_list: from-global\ndefault_space: global-space\n", + "output: yaml\ndefault_list: from-project\n") + + require.NoError(t, Init("")) + + assert.Equal(t, "from-project", GetString("default_list"), "project should override global") + assert.Equal(t, "yaml", GetString("output")) + assert.Equal(t, "global-space", GetString("default_space"), "keys absent from the project file keep the global value") + }) + + t.Run("environment outranks project", func(t *testing.T) { + newLayeredFixture(t, + "output: table\n", + "output: yaml\n") + t.Setenv("CU_OUTPUT", "csv") + + require.NoError(t, Init("")) + + assert.Equal(t, "csv", GetString("output"), "env must beat project config") + }) + + t.Run("flags outrank project", func(t *testing.T) { + newLayeredFixture(t, + "output: table\n", + "output: yaml\n") + + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + fs.String("output", "table", "") + require.NoError(t, fs.Set("output", "json")) + require.NoError(t, viper.BindPFlag("output", fs.Lookup("output"))) + + require.NoError(t, Init("")) + + assert.Equal(t, "json", GetString("output"), "an explicit flag must beat project config") + }) + + t.Run("credentials in a project file are ignored", func(t *testing.T) { + newLayeredFixture(t, + "api_token: global-token\n", + "api_token: project-token\ndefault_list: from-project\n") + + require.NoError(t, Init("")) + + assert.Equal(t, "global-token", GetString("api_token"), "a committed project file must not substitute the token") + assert.Equal(t, "from-project", GetString("default_list"), "non-credential keys still apply") + }) +} + +func TestSaveDoesNotLeakProjectConfig(t *testing.T) { + cfgDir, _ := newLayeredFixture(t, + "default_space: global-space\n", + "default_list: from-project\noutput: yaml\n") + + require.NoError(t, Init("")) + Set("default_workspace", "ws-1") + require.NoError(t, Save()) + + written, err := os.ReadFile(filepath.Join(cfgDir, ConfigFileName+"."+ConfigType)) + require.NoError(t, err) + got := string(written) + + assert.NotContains(t, got, "from-project", "project values must not be written to the global config") + assert.Contains(t, got, "ws-1", "explicitly set values are persisted") + assert.Contains(t, got, "global-space", "pre-existing global values are preserved") +}