From 2fbda99136c4fa51546afc4ecc0936bede009d3d Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Wed, 22 Oct 2025 17:16:45 -0300 Subject: [PATCH] fix(config): improve configuration merges The library we were using does not provide a good way to handle exceptions et al. Even if it did, we probably need to handle this in a case-by-case basis, as the correct thing to do for each property is different. This removes the library, introducing a manual merge process, and starts adding tests to verify it all. Ideally, we should also document what happens for each option. closes #932 closes #241 Signed-off-by: Carlos Alexandro Becker --- crush.json | 11 ++- go.mod | 1 - go.sum | 2 - internal/config/config.go | 125 ++++++++++++++++++++++++++++++ internal/config/load.go | 46 ++++++----- internal/config/merge.go | 16 ---- internal/config/merge_test.go | 140 ++++++++++++++++++++++++++++++---- 7 files changed, 286 insertions(+), 55 deletions(-) delete mode 100644 internal/config/merge.go diff --git a/crush.json b/crush.json index f5daef89add28ad4924c2bb87ca70020af005d67..3d416dcb01160ef1aea332c02bfee7e3a2f8ca3c 100644 --- a/crush.json +++ b/crush.json @@ -1,6 +1,15 @@ { "$schema": "https://charm.land/crush.json", + "gopls": { + "args": [ + "mcp" + ] + }, "lsp": { - "gopls": {} + "gopls": { + "args": [ + "server" + ] + } } } diff --git a/go.mod b/go.mod index 7c1cf2b1b5033a3e4fb404fa0a09c83f77db8367..a531f830019afa745253d0040587a7602add44f8 100644 --- a/go.mod +++ b/go.mod @@ -34,7 +34,6 @@ require ( github.com/nxadm/tail v1.4.11 github.com/openai/openai-go v1.12.0 github.com/pressly/goose/v3 v3.26.0 - github.com/qjebbs/go-jsons v1.0.0-alpha.4 github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06 github.com/sahilm/fuzzy v0.1.1 github.com/spf13/cobra v1.10.1 diff --git a/go.sum b/go.sum index bbb776c23ee5a38737e960af5f17764ea977aaab..5ef15266d79980db098c258b808e724f2a2b7f42 100644 --- a/go.sum +++ b/go.sum @@ -243,8 +243,6 @@ github.com/posthog/posthog-go v1.6.11 h1:5G8Y3pxnOpc3S4+PK1z1dCmZRuldiWxBsqqvvSf github.com/posthog/posthog-go v1.6.11/go.mod h1:LcC1Nu4AgvV22EndTtrMXTy+7RGVC0MhChSw7Qk5XkY= github.com/pressly/goose/v3 v3.26.0 h1:KJakav68jdH0WDvoAcj8+n61WqOIaPGgH0bJWS6jpmM= github.com/pressly/goose/v3 v3.26.0/go.mod h1:4hC1KrritdCxtuFsqgs1R4AU5bWtTAf+cnWvfhf2DNY= -github.com/qjebbs/go-jsons v1.0.0-alpha.4 h1:Qsb4ohRUHQODIUAsJKdKJ/SIDbsO7oGOzsfy+h1yQZs= -github.com/qjebbs/go-jsons v1.0.0-alpha.4/go.mod h1:wNJrtinHyC3YSf6giEh4FJN8+yZV7nXBjvmfjhBIcw4= github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE= github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= diff --git a/internal/config/config.go b/internal/config/config.go index ff948b874ea1613ca126053547dcf9b7d4cc3297..c7587508facf766674cf0abf1e1282f9c27a1f2c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,9 +1,11 @@ package config import ( + "cmp" "context" "fmt" "log/slog" + "maps" "net/http" "net/url" "os" @@ -116,6 +118,28 @@ type MCPConfig struct { Headers map[string]string `json:"headers,omitempty" jsonschema:"description=HTTP headers for HTTP/SSE MCP servers"` } +func (m MCPConfig) merge(o MCPConfig) MCPConfig { + // headers and env gets merged, new replacing existing values + maps.Copy(m.Env, o.Env) + maps.Copy(m.Headers, o.Headers) + + // bools are true if any is true + m.Disabled = o.Disabled || m.Disabled + + // max timeout + m.Timeout = max(o.Timeout, m.Timeout) + + // everything else is replaced if non-zero + m.Command = cmp.Or(o.Command, m.Command) + if len(o.Args) > 0 { + m.Args = o.Args + } + m.Type = cmp.Or(o.Type, m.Type) + m.URL = cmp.Or(o.URL, m.URL) + + return m +} + type LSPConfig struct { Disabled bool `json:"disabled,omitempty" jsonschema:"description=Whether this LSP server is disabled,default=false"` Command string `json:"command,omitempty" jsonschema:"required,description=Command to execute for the LSP server,example=gopls"` @@ -127,6 +151,43 @@ type LSPConfig struct { Options map[string]any `json:"options,omitempty" jsonschema:"description=LSP server-specific settings passed during initialization"` } +func (l LSPConfig) merge(o LSPConfig) LSPConfig { + // all maps gets merged, new replacing existing values + if l.Env == nil { + l.Env = make(map[string]string) + } + maps.Copy(l.Env, o.Env) + if l.InitOptions == nil { + l.InitOptions = make(map[string]any) + } + maps.Copy(l.InitOptions, o.InitOptions) + if l.Options == nil { + l.Options = make(map[string]any) + } + maps.Copy(l.Options, o.Options) + + // filetypes and rootmarkers get merged + l.RootMarkers = append(l.RootMarkers, o.RootMarkers...) + l.FileTypes = append(l.FileTypes, o.FileTypes...) + slices.Sort(l.RootMarkers) + slices.Sort(l.FileTypes) + l.RootMarkers = slices.Compact(l.RootMarkers) + l.FileTypes = slices.Compact(l.FileTypes) + + // disabled if any disabled + l.Disabled = l.Disabled || o.Disabled + + // args get replaced if non-empty + if len(o.Args) > 0 { + l.Args = o.Args + } + + // command takes precedence: + l.Command = cmp.Or(o.Command, l.Command) + + return l +} + type TUIOptions struct { CompactMode bool `json:"compact_mode,omitempty" jsonschema:"description=Enable compact mode for the TUI interface,default=false"` DiffMode string `json:"diff_mode,omitempty" jsonschema:"description=Diff mode for the TUI interface,enum=unified,enum=split"` @@ -136,6 +197,13 @@ type TUIOptions struct { Completions Completions `json:"completions,omitzero" jsonschema:"description=Completions UI options"` } +func (o *TUIOptions) merge(t TUIOptions) { + o.CompactMode = o.CompactMode || t.CompactMode + o.DiffMode = cmp.Or(t.DiffMode, o.DiffMode) + o.Completions.MaxDepth = cmp.Or(t.Completions.MaxDepth, o.Completions.MaxDepth) + o.Completions.MaxItems = cmp.Or(t.Completions.MaxDepth, o.Completions.MaxDepth) +} + // Completions defines options for the completions UI. type Completions struct { MaxDepth *int `json:"max_depth,omitempty" jsonschema:"description=Maximum depth for the ls tool,default=0,example=10"` @@ -169,6 +237,22 @@ type Options struct { DisableMetrics bool `json:"disable_metrics,omitempty" jsonschema:"description=Disable sending metrics,default=false"` } +func (o *Options) merge(t Options) { + o.ContextPaths = append(o.ContextPaths, t.ContextPaths...) + o.Debug = o.Debug || t.Debug + o.DebugLSP = o.DebugLSP || t.DebugLSP + o.DisableProviderAutoUpdate = o.DisableProviderAutoUpdate || t.DisableProviderAutoUpdate + o.DisableMetrics = o.DisableMetrics || t.DisableMetrics + o.DataDirectory = cmp.Or(t.DataDirectory, o.DataDirectory) + o.DisabledTools = append(o.DisabledTools, t.DisabledTools...) + o.TUI.merge(*t.TUI) + if t.Attribution != nil { + o.Attribution = &Attribution{} + o.Attribution.CoAuthoredBy = o.Attribution.CoAuthoredBy || t.Attribution.CoAuthoredBy + o.Attribution.GeneratedWith = o.Attribution.GeneratedWith || t.Attribution.GeneratedWith + } +} + type MCPs map[string]MCPConfig type MCP struct { @@ -263,6 +347,11 @@ type Tools struct { Ls ToolLs `json:"ls,omitzero"` } +func (o *Tools) merge(t Tools) { + o.Ls.MaxDepth = cmp.Or(t.Ls.MaxDepth, o.Ls.MaxDepth) + o.Ls.MaxItems = cmp.Or(t.Ls.MaxDepth, o.Ls.MaxDepth) +} + type ToolLs struct { MaxDepth *int `json:"max_depth,omitempty" jsonschema:"description=Maximum depth for the ls tool,default=0,example=10"` MaxItems *int `json:"max_items,omitempty" jsonschema:"description=Maximum number of items to return for the ls tool,default=1000,example=100"` @@ -302,6 +391,42 @@ type Config struct { knownProviders []catwalk.Provider `json:"-"` } +func (c *Config) merge(t Config) { + for name, mcp := range t.MCP { + existing, ok := c.MCP[name] + if !ok { + c.MCP[name] = mcp + continue + } + c.MCP[name] = existing.merge(mcp) + } + for name, lsp := range t.LSP { + existing, ok := c.LSP[name] + if !ok { + c.LSP[name] = lsp + continue + } + c.LSP[name] = existing.merge(lsp) + } + // simple override + maps.Copy(c.Models, t.Models) + c.Schema = cmp.Or(c.Schema, t.Schema) + if t.Options != nil { + c.Options.merge(*t.Options) + } + if t.Permissions != nil { + c.Permissions.AllowedTools = append(c.Permissions.AllowedTools, t.Permissions.AllowedTools...) + } + if c.Providers != nil { + for key, value := range t.Providers.Seq2() { + c.Providers.Set(key, value) + } + } + tools := &c.Tools + tools.merge(t.Tools) + c.Tools = *tools +} + func (c *Config) WorkingDir() string { return c.workingDir } diff --git a/internal/config/load.go b/internal/config/load.go index a219b7d1c848b3eea76809fca96e8e4049838365..7541b370a4a953421f6a63248f48b9327092ecaa 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -26,21 +26,6 @@ import ( const defaultCatwalkURL = "https://catwalk.charm.sh" -// LoadReader config via io.Reader. -func LoadReader(fd io.Reader) (*Config, error) { - data, err := io.ReadAll(fd) - if err != nil { - return nil, err - } - - var config Config - err = json.Unmarshal(data, &config) - if err != nil { - return nil, err - } - return &config, err -} - // Load loads the configuration from the default paths. func Load(workingDir, dataDir string, debug bool) (*Config, error) { configPaths := lookupConfigs(workingDir) @@ -574,12 +559,19 @@ func loadFromReaders(readers []io.Reader) (*Config, error) { return &Config{}, nil } - merged, err := Merge(readers) - if err != nil { - return nil, fmt.Errorf("failed to merge configuration readers: %w", err) + result := newConfig() + for _, r := range readers { + bts, err := io.ReadAll(r) + if err != nil { + return nil, fmt.Errorf("could not read: %w", err) + } + config := newConfig() + if err := json.Unmarshal(bts, &config); err != nil { + return nil, err + } + result.merge(*config) } - - return LoadReader(merged) + return result, nil } func hasVertexCredentials(env env.Env) bool { @@ -674,3 +666,17 @@ func isInsideWorktree() bool { ).CombinedOutput() return err == nil && strings.TrimSpace(string(bts)) == "true" } + +func newConfig() *Config { + return &Config{ + Agents: map[string]Agent{}, + MCP: map[string]MCPConfig{}, + LSP: map[string]LSPConfig{}, + Models: map[SelectedModelType]SelectedModel{}, + Options: &Options{ + TUI: &TUIOptions{}, + }, + Permissions: &Permissions{}, + Providers: csync.NewMap[string, ProviderConfig](), + } +} diff --git a/internal/config/merge.go b/internal/config/merge.go deleted file mode 100644 index 3c9b7d6283a193166ad50730b28853a909f5158a..0000000000000000000000000000000000000000 --- a/internal/config/merge.go +++ /dev/null @@ -1,16 +0,0 @@ -package config - -import ( - "bytes" - "io" - - "github.com/qjebbs/go-jsons" -) - -func Merge(data []io.Reader) (io.Reader, error) { - got, err := jsons.Merge(data) - if err != nil { - return nil, err - } - return bytes.NewReader(got), nil -} diff --git a/internal/config/merge_test.go b/internal/config/merge_test.go index 1b721bf2e8e4b4596025c2c773bec0093778f430..0c732f5f1f6655a7c766082b741631104c7a2b1c 100644 --- a/internal/config/merge_test.go +++ b/internal/config/merge_test.go @@ -1,27 +1,137 @@ package config import ( + "bytes" + "encoding/json" "io" - "strings" + "maps" + "slices" "testing" + + "github.com/stretchr/testify/require" ) -func TestMerge(t *testing.T) { - data1 := strings.NewReader(`{"foo": "bar"}`) - data2 := strings.NewReader(`{"baz": "qux"}`) +// TestConfigMerging defines the rules on how configuration merging works. +// Generally, things are either appended to or replaced by the later configuration. +// Whether one or the other happen depends on effects its effects. +func TestConfigMerging(t *testing.T) { + t.Run("empty", func(t *testing.T) { + c := exerciseMerge(t, Config{}, Config{}) + require.NotNil(t, c) + }) - merged, err := Merge([]io.Reader{data1, data2}) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } + t.Run("mcps", func(t *testing.T) { + c := exerciseMerge(t, Config{ + MCP: MCPs{ + "foo": { + Command: "foo-mcp", + Args: []string{"serve"}, + Type: MCPSSE, + Timeout: 10, + }, + "zaz": { + Disabled: true, + Env: map[string]string{"FOO": "bar"}, + Headers: map[string]string{"api-key": "exposed"}, + URL: "nope", + }, + }, + }, Config{ + MCP: MCPs{ + "foo": { + Args: []string{"serve", "--stdio"}, + Type: MCPStdio, + Timeout: 7, + }, + "bar": { + Command: "bar", + }, + "zaz": { + Env: map[string]string{"FOO": "foo", "BAR": "bar"}, + Headers: map[string]string{"api-key": "$API"}, + URL: "http://bar", + }, + }, + }) + require.NotNil(t, c) + require.Len(t, slices.Collect(maps.Keys(c.MCP)), 3) + require.Equal(t, MCPConfig{ + Command: "foo-mcp", + Args: []string{"serve", "--stdio"}, + Type: MCPStdio, + Timeout: 10, + }, c.MCP["foo"]) + require.Equal(t, MCPConfig{ + Command: "bar", + }, c.MCP["bar"]) + require.Equal(t, MCPConfig{ + Disabled: true, + URL: "http://bar", + Env: map[string]string{"FOO": "foo", "BAR": "bar"}, + Headers: map[string]string{"api-key": "$API"}, + }, c.MCP["zaz"]) + }) - expected := `{"foo":"bar","baz":"qux"}` - got, err := io.ReadAll(merged) - if err != nil { - t.Fatalf("expected no error reading merged data, got %v", err) - } + t.Run("lsps", func(t *testing.T) { + result := exerciseMerge(t, Config{ + LSP: LSPs{ + "gopls": LSPConfig{ + Env: map[string]string{"FOO": "bar"}, + RootMarkers: []string{"go.sum"}, + FileTypes: []string{"go"}, + }, + }, + }, Config{ + LSP: LSPs{ + "gopls": LSPConfig{ + Command: "gopls", + InitOptions: map[string]any{"a": 10}, + RootMarkers: []string{"go.sum"}, + }, + }, + }, Config{ + LSP: LSPs{ + "gopls": LSPConfig{ + Args: []string{"serve", "--stdio"}, + InitOptions: map[string]any{"a": 12, "b": 18}, + RootMarkers: []string{"go.sum", "go.mod"}, + FileTypes: []string{"go"}, + Disabled: true, + }, + }, + }, + Config{ + LSP: LSPs{ + "gopls": LSPConfig{ + Options: map[string]any{"opt1": "10"}, + RootMarkers: []string{"go.work"}, + }, + }, + }, + ) + require.NotNil(t, result) + require.Equal(t, LSPConfig{ + Disabled: true, + Command: "gopls", + Args: []string{"serve", "--stdio"}, + Env: map[string]string{"FOO": "bar"}, + FileTypes: []string{"go"}, + RootMarkers: []string{"go.mod", "go.sum", "go.work"}, + InitOptions: map[string]any{"a": 12.0, "b": 18.0}, + Options: map[string]any{"opt1": "10"}, + }, result.LSP["gopls"]) + }) +} - if string(got) != expected { - t.Errorf("expected %s, got %s", expected, string(got)) +func exerciseMerge(tb testing.TB, confs ...Config) *Config { + tb.Helper() + readers := make([]io.Reader, 0, len(confs)) + for _, c := range confs { + bts, err := json.Marshal(c) + require.NoError(tb, err) + readers = append(readers, bytes.NewReader(bts)) } + result, err := loadFromReaders(readers) + require.NoError(tb, err) + return result }