crush.json 🔗
@@ -1,6 +1,15 @@
{
"$schema": "https://charm.land/crush.json",
+ "gopls": {
+ "args": [
+ "mcp"
+ ]
+ },
"lsp": {
- "gopls": {}
+ "gopls": {
+ "args": [
+ "server"
+ ]
+ }
}
}
fix(config): improve configuration merges
Carlos Alexandro Becker created
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 <caarlos0@users.noreply.github.com>Change summary
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(-)
Detailed changes
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
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=
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
}
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](),
+ }
+}
internal/config/merge.go 🔗
@@ -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
-}
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
}