From f38b4b1734c4fbf6066641c7453220d68387ff9d Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Tue, 2 Jun 2026 13:42:55 +0300 Subject: [PATCH] fix(kit): correct config loading and isolate ACP sessions - Isolate each ACP session's config store instead of sharing the global viper, preventing per-session SetModel/SetThinkingLevel races; seed the root-command flag values (model, thinking-level, provider URL/key) so `kit acp -m ` is still honored - Run initConfig for isolated SDK stores by gating on opts.CLI instead of v.GetString("model"), which setSDKDefaults always populates and thus skipped .kit.yml / KIT_* loading for SDK callers - Configure KIT_* env overrides unconditionally in initConfig so passing an explicit config file no longer disables environment variable support - Wrap config unmarshal/validate errors with %w to preserve the error chain --- internal/acpserver/session.go | 21 ++++++++++++++------- internal/config/merger.go | 4 ++-- pkg/kit/config.go | 17 +++++++++++------ pkg/kit/kit.go | 9 ++++++--- 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/internal/acpserver/session.go b/internal/acpserver/session.go index 435417a0..2b45111a 100644 --- a/internal/acpserver/session.go +++ b/internal/acpserver/session.go @@ -7,6 +7,7 @@ import ( "sync" "github.com/charmbracelet/log" + "github.com/spf13/viper" "github.com/mark3labs/kit/internal/extbridge" "github.com/mark3labs/kit/internal/extensions" @@ -38,14 +39,20 @@ func newSessionRegistry() *sessionRegistry { // given working directory. The Kit-generated session ID is used as the ACP // session ID so the mapping is 1:1. func (r *sessionRegistry) create(ctx context.Context, cwd string) (*acpSession, error) { + // Each ACP session gets its own isolated config store (CLI is left nil) so + // per-session SetModel / SetThinkingLevel calls cannot race or bleed across + // the sessionRegistry. We seed the relevant root-command flag values from + // the process-global store (which cobra populated from flags) so launching + // `kit acp -m [--thinking-level ...] [--provider-url ...]` is still + // honored; .kit.yml and KIT_* env vars are loaded per session by kit.New. kitInstance, err := kit.New(ctx, &kit.Options{ - SessionDir: cwd, - Quiet: true, - Streaming: true, - // Share the process-global config store so cobra flag bindings and - // config loaded by the CLI's cobra.OnInitialize remain in effect for - // the ACP server. SDK consumers leave CLI nil to get isolated stores. - CLI: &kit.CLIOptions{}, + SessionDir: cwd, + Quiet: true, + Streaming: true, + Model: viper.GetString("model"), + ThinkingLevel: viper.GetString("thinking-level"), + ProviderURL: viper.GetString("provider-url"), + ProviderAPIKey: viper.GetString("provider-api-key"), }) if err != nil { // Provide actionable guidance for provider auth errors, which are diff --git a/internal/config/merger.go b/internal/config/merger.go index bd542a16..d64eab58 100644 --- a/internal/config/merger.go +++ b/internal/config/merger.go @@ -31,7 +31,7 @@ func LoadAndValidateConfigFrom(v *viper.Viper) (*Config, error) { MCPServers: make(map[string]MCPServerConfig), } if err := v.Unmarshal(config); err != nil { - return nil, fmt.Errorf("failed to unmarshal config: %v", err) + return nil, fmt.Errorf("failed to unmarshal config: %w", err) } // Fix environment variable case sensitivity issue @@ -39,7 +39,7 @@ func LoadAndValidateConfigFrom(v *viper.Viper) (*Config, error) { fixEnvironmentCase(v, config) if err := config.Validate(); err != nil { - return nil, fmt.Errorf("invalid config: %v", err) + return nil, fmt.Errorf("invalid config: %w", err) } return config, nil diff --git a/pkg/kit/config.go b/pkg/kit/config.go index f04d99f1..e6f35718 100644 --- a/pkg/kit/config.go +++ b/pkg/kit/config.go @@ -92,6 +92,17 @@ func initConfig(v *viper.Viper, configFile string, debug bool) error { if v == nil { v = viper.GetViper() } + + // Configure KIT_* environment overrides unconditionally, before any file + // is loaded, so that an explicit config file does not disable env support. + // Map hyphenated config keys (e.g. "max-tokens") to underscored env var + // names (e.g. KIT_MAX_TOKENS); without this AutomaticEnv looks for + // KIT_MAX-TOKENS and silently misses valid overrides. Precedence is + // resolved at read time, so calling these before ReadConfig is fine. + v.SetEnvPrefix("KIT") + v.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) + v.AutomaticEnv() + if configFile != "" { return loadConfigWithEnvSubstitution(v, configFile) } @@ -130,12 +141,6 @@ func initConfig(v *viper.Viper, configFile string, debug bool) error { fmt.Fprintf(os.Stderr, "No config file found in current directory or home directory\n") } - v.SetEnvPrefix("KIT") - // Map hyphenated config keys (e.g. "max-tokens") to underscored env - // var names (e.g. KIT_MAX_TOKENS). Without this, AutomaticEnv looks - // for KIT_MAX-TOKENS and silently misses valid env overrides. - v.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) - v.AutomaticEnv() return nil } diff --git a/pkg/kit/kit.go b/pkg/kit/kit.go index 9fec0715..06fb4839 100644 --- a/pkg/kit/kit.go +++ b/pkg/kit/kit.go @@ -1223,10 +1223,13 @@ func New(ctx context.Context, opts *Options) (*Kit, error) { setSDKDefaults(v) // Initialize config (loads config files and env vars) into the instance - // store. Only initialize if not already done (e.g., by CLI's - // cobra.OnInitialize, which populates the shared global store). + // store. The CLI shares the process-global store, which cobra.OnInitialize + // has already populated, so re-running initConfig there is unnecessary; + // SDK callers get a fresh isolated store that must be loaded here. + // We key off opts.CLI (not a config value) because setSDKDefaults always + // seeds "model", which would otherwise mask an empty store. // SkipConfig bypasses .kit.yml file loading (viper defaults and env vars still apply). - if !opts.SkipConfig && v.GetString("model") == "" { + if !opts.SkipConfig && opts.CLI == nil { if err := initConfig(v, opts.ConfigFile, false); err != nil { return fmt.Errorf("failed to initialize config: %w", err) }