[BUG] Data race in Config.initSensitiveCache can return a nil replacer and panic FilterSensitiveData

Quick Summary

Config.sensitiveCache is created lazily without any synchronization (pkg/config/security.go:221-222), so the sync.Once it contains is defeated. Two goroutines can each allocate their own SensitiveDataCache, and SensitiveDataReplacer can then return a nil *strings.Replacer. FilterSensitiveData calls .Replace() on that result (pkg/config/config.go:209), which panics with a nil pointer dereference.

Triggered whenever more than one turn filters tool output through the same *Config, i.e. more than one turn in flight.

Environment & Tools

• PicoClaw Version: bbf6893c (current main)
• Go Version: go1.25.13
• AI Model & Provider: N/A — reproduces at the config layer, no provider involved
• Operating System: macOS 15.7.4 (darwin/arm64); not OS-specific
• Channels: N/A

Steps to Reproduce

1. Add this test to pkg/config/security_test.go:

func TestSensitiveDataReplacer_ConcurrentFirstUse(t *testing.T) {
	const goroutines = 8

	// A fresh *Config per iteration: the cache is initialized once per Config,
	// so the only interesting window is the first concurrent use.
	for i := 0; i < 200; i++ {
		cfg := &Config{
			ModelList: SecureModelList{
				&ModelConfig{
					ModelName: "m",
					Model:     "openai/m",
					APIKeys:   SimpleSecureStrings("sk-long-key-12345"),
				},
			},
		}

		var wg sync.WaitGroup
		start := make(chan struct{})
		got := make([]*strings.Replacer, goroutines)
		for g := 0; g < goroutines; g++ {
			wg.Add(1)
			go func(g int) {
				defer wg.Done()
				<-start
				got[g] = cfg.SensitiveDataReplacer()
			}(g)
		}
		close(start)
		wg.Wait()

		for g, r := range got {
			require.NotNil(t, r, "iteration %d, goroutine %d: nil replacer", i, g)
		}
	}
}

1. go test ./pkg/config/ -run TestSensitiveDataReplacer_ConcurrentFirstUse -race -count=1

❌ Actual Behavior

WARNING: DATA RACE
Read at 0x00c000123130 by goroutine 11:
  github.com/sipeed/picoclaw/pkg/config.(*Config).initSensitiveCache()
      pkg/config/security.go:221 +0x34
  github.com/sipeed/picoclaw/pkg/config.(*Config).SensitiveDataReplacer()
      pkg/config/security.go:215 +0x90

Previous write at 0x00c000123130 by goroutine 12:
  github.com/sipeed/picoclaw/pkg/config.(*Config).initSensitiveCache()
      pkg/config/security.go:222 +0x88
  github.com/sipeed/picoclaw/pkg/config.(*Config).SensitiveDataReplacer()
      pkg/config/security.go:215 +0x90

This is not only a race-detector warning. The corrupted outcome is observable in a normal build, without -race. Stressing the same call (32 goroutines × 200000 fresh *Config, 6.4M calls) on bbf6893c returned a nil replacer once:

STRESS RESULT: nil replacers = 1, wrong output = 0

A nil *strings.Replacer reaching FilterSensitiveData panics:

// pkg/config/config.go:209
return c.SensitiveDataReplacer().Replace(content)

panic: runtime error: invalid memory address or nil pointer dereference

That call site sits in the tool-result path (pkg/agent/pipeline_execute.go:295, 303, 388, 527, 690, 776 and pkg/agent/turn_coord.go:148), so the crash lands mid-turn while filtering tool output.

✅ Expected Behavior

SensitiveDataReplacer() returns a usable replacer for every caller, the cache is built once, and go test -race is clean.

Additional Context

Mechanism

// pkg/config/security.go:219-224
func (sec *Config) initSensitiveCache() {
	if sec.sensitiveCache == nil {          // unsynchronized read
		sec.sensitiveCache = &SensitiveDataCache{}   // unsynchronized write
	}
	sec.sensitiveCache.once.Do(func() { ... })
}

// pkg/config/security.go:214-217
func (sec *Config) SensitiveDataReplacer() *strings.Replacer {
	sec.initSensitiveCache()
	return sec.sensitiveCache.replacer      // re-reads the field
}

The sync.Once is inside the object being created, so it cannot protect its own creation. Two goroutines that both observe nil each allocate a cache, and the second write wins. Interleaving that produces a nil replacer:

1. G1 and G2 both read sensitiveCache == nil.
2. G1 writes C1; C1.once runs and sets C1.replacer.
3. Before G1 reaches line 216, G2 writes C2 (fresh, replacer still nil, once not run).
4. G1's line 216 reads the field again, now C2, and returns C2.replacer — nil.

A milder outcome is that both goroutines run the full reflection walk in collectSensitiveValues, so the "computed once" promise in the doc comment does not hold.

Trigger and blast radius

It needs two turns filtering tool output through the same *Config at the same time, i.e. more than one turn in flight (agents.defaults.max_parallel_turns > 1; the worker pool defaults to 1 in pkg/agent/agent_init.go:60-64, so a single-turn setup is not exposed).

Note on the obvious fix

Moving the cache into Config as a value (sensitiveCache SensitiveDataCache) and dropping the nil check is the smallest change on paper, but it does not compile cleanly: Config is shallow-copied by value in cmd/picoclaw/internal/mcp/helpers.go:130 (clone := *cfg), so embedding anything containing sync.Once fails go vet:

cmd/picoclaw/internal/mcp/helpers.go:130:11: assignment copies lock value to clone:
  config.Config contains config.SensitiveDataCache contains sync.Once contains sync.noCopy

The same applies to a sync.Mutex or atomic.Pointer field. Keeping Config copyable therefore means the synchronization has to live outside the struct. I have a patch that guards the pointer creation with a package-level mutex and returns the cache from initSensitiveCache so the caller always uses the cache its Once ran on, with the reflection walk still outside the lock. Happy to open it as a PR if that direction is acceptable — or to redo it as a value field plus an explicit clone helper in mcp/helpers.go if you would rather remove the shallow copy.