fix(config): copy all ServerConfig fields in CopyServerConfig

CopyServerConfig was hand-copying a subset of ServerConfig fields and had
drifted as new fields were added — it silently omitted EnabledTools,
DisabledTools, ReconnectOnUse, HealthCheckInterval, ToolDiscoveryInterval,
SourceRegistryID, SourceRegistryProvenance, and AuthBroker.

Because MergeServerConfig starts from CopyServerConfig(base), any config
merge/patch (e.g. upstream_servers patch, the Web UI server-config edit)
round-trips the server through CopyServerConfig and drops these fields. The
user-visible symptom: editing an unrelated field on a server that has a
disabled_tools / enabled_tools allowlist, a reconnect-on-use flag, or
per-server health/discovery cadence overrides silently wipes those settings.

Copy the missing fields: scalars and slices directly (deep-copying the
allowlist slices like Args), the *Duration overrides by value (matching the
AutoApproveToolChanges pattern), and the AuthBroker block by value (a no-op
empty stub in the personal edition).

Tests: TestCopyServerConfig_PreservesAllowlistAndOverrides asserts each field
is copied and that slices/pointers are independent of the source;
TestMergeServerConfig_PreservesDisabledToolsOnUnrelatedPatch is the
end-to-end regression for the patch path.

Note: the same record<->config field mapping is hand-duplicated in the
storage layer (SaveUpstreamServer / GetUpstreamServer / ListUpstreamServers /
ListQuarantinedUpstreamServers); ListQuarantinedUpstreamServers has drifted
similarly. A follow-up could collapse all of these onto a single helper to
prevent the next field from drifting again.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: Roman Chernyak

internal/config/merge.go

@@ -528,25 +528,36 @@ 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
 	if src.Args != nil {
 		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)

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)
+	}
+}