fix(configsvc): deep-copy tool-filter slices in Snapshot.Clone (+ regression guard) (#479)

* fix(configsvc): deep-copy tool-filter slices in Snapshot.Clone

Investigation of the reported 'config-file disabled_tools does not reach
runtime' concern found NO reproducible code bug: every boundary (file
parse, storage Save/Get/List, and the full path
parse->New->LoadConfiguredServers->SaveConfiguration->IsToolConfigDenied)
provably preserves enabled_tools/disabled_tools. The original manual
observation was a test-environment artifact (reused /tmp data-dir while
SaveConfiguration rewrites the config file + hot-reload watcher).

The investigation did surface one real latent defect: Snapshot.Clone()
deep-copied Args/Headers/Env/OAuth/Isolation but shallow-aliased the
spec-049 EnabledTools/DisabledTools slices via 'clonedSrv := *srv'. Not
data loss (reads see the aliased value) but an immutability violation:
mutating a cloned config's filter would corrupt the shared backing array.

- Deep-copy EnabledTools/DisabledTools in Snapshot.Clone
- TestSnapshotClone_DeepCopiesToolFilters (aliasing regression)
- TestConfigFileToolFilter_ReachesRuntime (permanent full-path guard)
- correct the spec-049 quickstart note (no #468 gap exists)

Related #468

* test(049): make TestConfigFileToolFilter_ReachesRuntime cross-platform

The first revision string-interpolated t.TempDir() into a JSON literal;
on windows-amd64 the backslash path produced invalid JSON, failing the
test (caught by the Build Binaries windows job). Marshal the config via
encoding/json so the path is escaped on every OS.

Related #468

Author: Dumbris

internal/runtime/config_tool_filter_e2e_test.go

@@ -0,0 +1,74 @@
+package runtime
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+// TestConfigFileToolFilter_ReachesRuntime is a regression guard for the
+// concern raised after spec 049: that config-file enabled_tools/disabled_tools
+// on a stdio server might not reach IsToolConfigDenied at runtime.
+//
+// It exercises the full production path — file → LoadFromFile → runtime.New
+// (ConfigService) → LoadConfiguredServers (config→storage sync) →
+// SaveConfiguration (storage→ConfigService + file rewrite) — and asserts the
+// filter is honored at every stage and survives the config-file rewrite.
+func TestConfigFileToolFilter_ReachesRuntime(t *testing.T) {
+	dir := t.TempDir()
+	p := filepath.Join(dir, "mcp_config.json")
+	// json.Marshal so the data_dir path is correctly escaped on every OS
+	// (Windows temp paths contain backslashes — raw string interpolation into
+	// a JSON literal produces invalid JSON).
+	cfgJSON, err := json.Marshal(map[string]any{
+		"listen":   "127.0.0.1:0",
+		"data_dir": dir,
+		"api_key":  "k",
+		"mcpServers": []map[string]any{{
+			"name": "everything", "command": "npx", "args": []string{"-y", "x"},
+			"protocol": "stdio", "enabled": true, "disabled_tools": []string{"echo"},
+		}},
+	})
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(p, cfgJSON, 0600))
+
+	cfg, err := config.LoadFromFile(p)
+	require.NoError(t, err)
+	require.Equal(t, []string{"echo"}, cfg.Servers[0].DisabledTools, "parse boundary")
+
+	rt, err := New(cfg, p, zap.NewNop())
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = rt.Close() })
+
+	assert.True(t, rt.IsToolConfigDenied("everything", "echo"), "after New")
+	assert.False(t, rt.IsToolConfigDenied("everything", "get-tiny-image"), "non-listed tool allowed")
+
+	// Sync config → storage (async upstream connect will fail for the fake
+	// npx binary; the storage save is synchronous and is what we assert on).
+	_ = rt.LoadConfiguredServers(cfg)
+	time.Sleep(300 * time.Millisecond)
+
+	stored, err := rt.storageManager.ListUpstreamServers()
+	require.NoError(t, err)
+	require.Len(t, stored, 1)
+	assert.Equal(t, []string{"echo"}, stored[0].DisabledTools, "storage after LoadConfiguredServers")
+	assert.True(t, rt.IsToolConfigDenied("everything", "echo"), "runtime after LoadConfiguredServers")
+
+	// Round-trip through storage → ConfigService → config file.
+	require.NoError(t, rt.SaveConfiguration())
+	assert.True(t, rt.IsToolConfigDenied("everything", "echo"), "runtime after SaveConfiguration")
+
+	rewritten, err := config.LoadFromFile(p)
+	require.NoError(t, err)
+	require.Len(t, rewritten.Servers, 1)
+	assert.Equal(t, []string{"echo"}, rewritten.Servers[0].DisabledTools,
+		"disabled_tools must survive the SaveConfiguration file rewrite")
+}

internal/runtime/configsvc/snapshot.go

@@ -50,6 +50,19 @@ func (s *Snapshot) Clone() *config.Config {
 					copy(clonedSrv.Args, srv.Args)
 				}
 
+				// Spec 049: deep-copy the config tool-filter slices. Without
+				// this, `clonedSrv := *srv` aliases the original backing array,
+				// so a mutation of a cloned snapshot's filter would corrupt the
+				// shared config (immutability violation).
+				if srv.EnabledTools != nil {
+					clonedSrv.EnabledTools = make([]string, len(srv.EnabledTools))
+					copy(clonedSrv.EnabledTools, srv.EnabledTools)
+				}
+				if srv.DisabledTools != nil {
+					clonedSrv.DisabledTools = make([]string, len(srv.DisabledTools))
+					copy(clonedSrv.DisabledTools, srv.DisabledTools)
+				}
+
 				// Clone OAuth config if present
 				if srv.OAuth != nil {
 					oauthClone := *srv.OAuth

internal/runtime/configsvc/snapshot_filter_clone_test.go

@@ -0,0 +1,36 @@
+package configsvc
+
+import (
+	"testing"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+// Spec 049: Clone must deep-copy EnabledTools/DisabledTools so a mutation of
+// the cloned config cannot corrupt the original snapshot's backing array.
+func TestSnapshotClone_DeepCopiesToolFilters(t *testing.T) {
+	orig := &config.ServerConfig{
+		Name:          "s",
+		EnabledTools:  []string{"a", "b"},
+		DisabledTools: []string{"x", "y"},
+	}
+	s := &Snapshot{Config: &config.Config{Servers: []*config.ServerConfig{orig}}}
+
+	cloned := s.Clone()
+	cs := cloned.Servers[0]
+
+	// Distinct backing arrays.
+	cs.DisabledTools[0] = "MUTATED"
+	cs.EnabledTools[0] = "MUTATED"
+	if orig.DisabledTools[0] != "x" {
+		t.Fatalf("DisabledTools aliased: original corrupted to %v", orig.DisabledTools)
+	}
+	if orig.EnabledTools[0] != "a" {
+		t.Fatalf("EnabledTools aliased: original corrupted to %v", orig.EnabledTools)
+	}
+	// Values still copied correctly (no data loss).
+	cloned2 := s.Clone()
+	if len(cloned2.Servers[0].DisabledTools) != 2 || cloned2.Servers[0].DisabledTools[1] != "y" {
+		t.Fatalf("DisabledTools not copied: %v", cloned2.Servers[0].DisabledTools)
+	}
+}

specs/049-agent-discoverable-disabled-tools/quickstart.md

@@ -128,10 +128,16 @@ pkill -f 'mcpproxy serve.*18049' 2>/dev/null
 | §1 `upstream_servers` → `{callable:12,disabled_by_user:1}`, zero reasons omitted; all-callable server emits no block | ✅ PASS |
 | §5 zero-result nudge | ✅ PASS (unit: `TestDisabledDiscovery_ZeroResultNudge`) |
 
-**Known limitation (pre-existing, NOT spec 049):** config-file `disabled_tools`
-on a stdio server did not reach `IsToolConfigDenied` at runtime in this manual
-run (`disabled_tools` showed null in the live server view; an `echo` call
-succeeded). The 049 classifier maps `disabled_by_config` correctly whenever
-`IsToolConfigDenied` fires (proven by `TestClassifyDisabledTool_Precedence` and
-the §4 call-path message). This config→runtime load gap belongs to #468 and is
-filed separately; it does not affect 049 correctness.
+**Investigated and resolved — there is no config→runtime gap.** The manual
+run that appeared to show config-file `disabled_tools` not reaching
+`IsToolConfigDenied` was a test-environment artifact (a `/tmp` data-dir reused
+across runs while mcpproxy's own `SaveConfiguration()` rewrites the config
+file, plus the hot-reload watcher — the effective config at the `echo` call
+was indeterminate). Systematic debugging proved every boundary preserves the
+filter: file parse, storage `Save→Get/List`, and the full path
+parse→`New`→`LoadConfiguredServers`→`SaveConfiguration`→`IsToolConfigDenied`.
+This is now permanently guarded by
+`TestConfigFileToolFilter_ReachesRuntime` (internal/runtime). The investigation
+did surface and fix one real latent defect — `configsvc.Snapshot.Clone()`
+shallow-aliased the `EnabledTools`/`DisabledTools` slices (immutability
+violation, not data loss) — fixed with `TestSnapshotClone_DeepCopiesToolFilters`.