diff --git a/internal/config/merge.go b/internal/config/merge.go index 10fed0ab..39a193e7 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -528,18 +528,21 @@ func CopyServerConfig(src *ServerConfig) *ServerConfig { } dst := &ServerConfig{ - Name: src.Name, - URL: src.URL, - Protocol: src.Protocol, - Command: src.Command, - WorkingDir: src.WorkingDir, - Enabled: src.Enabled, - Quarantined: src.Quarantined, - SkipQuarantine: src.SkipQuarantine, - Shared: src.Shared, - Created: src.Created, - Updated: src.Updated, - LauncherWaitTimeout: src.LauncherWaitTimeout, + Name: src.Name, + URL: src.URL, + Protocol: src.Protocol, + Command: src.Command, + WorkingDir: src.WorkingDir, + Enabled: src.Enabled, + Quarantined: src.Quarantined, + SkipQuarantine: src.SkipQuarantine, + Shared: src.Shared, + Created: src.Created, + Updated: src.Updated, + LauncherWaitTimeout: src.LauncherWaitTimeout, + ReconnectOnUse: src.ReconnectOnUse, + SourceRegistryID: src.SourceRegistryID, + SourceRegistryProvenance: src.SourceRegistryProvenance, } // Copy slices @@ -547,6 +550,14 @@ func CopyServerConfig(src *ServerConfig) *ServerConfig { dst.Args = make([]string, len(src.Args)) copy(dst.Args, src.Args) } + if src.EnabledTools != nil { + dst.EnabledTools = make([]string, len(src.EnabledTools)) + copy(dst.EnabledTools, src.EnabledTools) + } + if src.DisabledTools != nil { + dst.DisabledTools = make([]string, len(src.DisabledTools)) + copy(dst.DisabledTools, src.DisabledTools) + } // Copy maps if src.Env != nil { @@ -568,6 +579,24 @@ func CopyServerConfig(src *ServerConfig) *ServerConfig { dst.AutoApproveToolChanges = &autoApprove } + // Copy *Duration overrides by value to avoid shared pointer state (spec 074) + if src.HealthCheckInterval != nil { + hc := *src.HealthCheckInterval + dst.HealthCheckInterval = &hc + } + if src.ToolDiscoveryInterval != nil { + td := *src.ToolDiscoveryInterval + dst.ToolDiscoveryInterval = &td + } + + // Copy the per-upstream auth-broker block by value (spec 074, server edition). + // In the personal edition AuthBrokerConfig is an empty stub struct, so this is + // a no-op there; copying by value keeps the pointer from being shared. + if src.AuthBroker != nil { + broker := *src.AuthBroker + dst.AuthBroker = &broker + } + // Copy nested structs dst.Isolation = copyIsolationConfig(src.Isolation) dst.OAuth = copyOAuthConfig(src.OAuth) diff --git a/internal/config/merge_test.go b/internal/config/merge_test.go index 4eed210f..0dcc8236 100644 --- a/internal/config/merge_test.go +++ b/internal/config/merge_test.go @@ -997,3 +997,74 @@ func TestCopyServerConfig_AllFields(t *testing.T) { t.Error("CopyServerConfig(nil) should return nil") } } + +// TestCopyServerConfig_PreservesAllowlistAndOverrides guards the fields that +// were silently dropped by CopyServerConfig before this fix: the per-tool +// allowlist/denylist, ReconnectOnUse, the spec-074 *Duration overrides, and the +// source-registry provenance. A drop here loses a server's disabled_tools and +// per-server cadence overrides on any config merge/patch round-trip. +func TestCopyServerConfig_PreservesAllowlistAndOverrides(t *testing.T) { + hc := Duration(30 * time.Second) + td := Duration(5 * time.Minute) + src := &ServerConfig{ + Name: "srv", + ReconnectOnUse: true, + EnabledTools: []string{"a", "b"}, + DisabledTools: []string{"c"}, + HealthCheckInterval: &hc, + ToolDiscoveryInterval: &td, + SourceRegistryID: "reg-1", + SourceRegistryProvenance: "official", + } + + dst := CopyServerConfig(src) + + if !dst.ReconnectOnUse { + t.Error("ReconnectOnUse was dropped") + } + if dst.SourceRegistryID != "reg-1" { + t.Errorf("SourceRegistryID: got %q, want %q", dst.SourceRegistryID, "reg-1") + } + if dst.SourceRegistryProvenance != "official" { + t.Errorf("SourceRegistryProvenance: got %q, want %q", dst.SourceRegistryProvenance, "official") + } + if !reflect.DeepEqual(dst.EnabledTools, src.EnabledTools) { + t.Errorf("EnabledTools: got %v, want %v", dst.EnabledTools, src.EnabledTools) + } + if !reflect.DeepEqual(dst.DisabledTools, src.DisabledTools) { + t.Errorf("DisabledTools: got %v, want %v", dst.DisabledTools, src.DisabledTools) + } + if dst.HealthCheckInterval == nil || *dst.HealthCheckInterval != hc { + t.Errorf("HealthCheckInterval: got %v, want %v", dst.HealthCheckInterval, hc) + } + if dst.ToolDiscoveryInterval == nil || *dst.ToolDiscoveryInterval != td { + t.Errorf("ToolDiscoveryInterval: got %v, want %v", dst.ToolDiscoveryInterval, td) + } + + // Deep copy: mutating the source must not affect the copy. + src.DisabledTools[0] = "mutated" + if dst.DisabledTools[0] == "mutated" { + t.Error("DisabledTools is aliased, not deep-copied") + } + *src.HealthCheckInterval = Duration(time.Hour) + if *dst.HealthCheckInterval == Duration(time.Hour) { + t.Error("HealthCheckInterval pointer is shared, not copied by value") + } +} + +// TestMergeServerConfig_PreservesDisabledToolsOnUnrelatedPatch is the +// end-to-end regression: patching an unrelated field on a server that has a +// disabled_tools denylist must not wipe the denylist (it previously did, +// because MergeServerConfig starts from CopyServerConfig(base)). +func TestMergeServerConfig_PreservesDisabledToolsOnUnrelatedPatch(t *testing.T) { + base := &ServerConfig{Name: "srv", DisabledTools: []string{"danger"}} + patch := &ServerConfig{Env: map[string]string{"K": "V"}} + + merged, _, err := MergeServerConfig(base, patch, MergeOptions{}) + if err != nil { + t.Fatalf("MergeServerConfig: %v", err) + } + if len(merged.DisabledTools) != 1 || merged.DisabledTools[0] != "danger" { + t.Errorf("DisabledTools dropped on unrelated patch: got %v, want [danger]", merged.DisabledTools) + } +}