From 31a6d1a60cda11c58902c8e9ad8ac2b4402fe7b7 Mon Sep 17 00:00:00 2001 From: viettranx Date: Sat, 18 Apr 2026 17:47:21 +0700 Subject: [PATCH] refactor(tools): convert web_search to tenant-scoped provider resolution Refactor WebSearchTool constructor to accept ConfigSecretsStore and MessageBus instead of hardcoded WebSearchConfig. Implement runtime per-tenant provider chain resolution via tenantChainCache. Constructor now: - Subscribes to cache invalidation events on config_secrets changes - Returns nil-safe tool instance (no early return on empty providers) - Passes secrets store and bus for dynamic resolution per Execute() call Add resolve unit tests; delete legacy config_test.go (functionality subsumed by cache tests and integration tests). --- internal/tools/web_search.go | 58 ++++--- internal/tools/web_search_config.go | 76 ++------ internal/tools/web_search_config_test.go | 150 ---------------- internal/tools/web_search_resolve_test.go | 201 ++++++++++++++++++++++ 4 files changed, 256 insertions(+), 229 deletions(-) delete mode 100644 internal/tools/web_search_config_test.go create mode 100644 internal/tools/web_search_resolve_test.go diff --git a/internal/tools/web_search.go b/internal/tools/web_search.go index c10253e7..cb3b4621 100644 --- a/internal/tools/web_search.go +++ b/internal/tools/web_search.go @@ -7,6 +7,12 @@ import ( "regexp" "strings" "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) // Matching TS src/agents/tools/web-search.ts constants. @@ -82,28 +88,43 @@ func normalizeFreshness(value string) string { // --- WebSearchTool --- -// WebSearchTool implements the web_search tool matching TS src/agents/tools/web-search.ts. +// WebSearchTool implements the web_search tool with per-tenant provider chain +// resolution. Providers are resolved per-request from config_secrets and +// builtin_tool_tenant_configs.settings, cached per tenant for 60 seconds. type WebSearchTool struct { - providers []SearchProvider - cache *webCache + secrets store.ConfigSecretsStore + cache *webCache + chainCache *tenantChainCache } -func NewWebSearchTool(cfg WebSearchConfig) *WebSearchTool { - providers := buildSearchProviders(cfg) - - if len(providers) == 0 { - return nil +// NewWebSearchTool constructs a WebSearchTool. msgBus may be nil (e.g. desktop +// edition) — cache invalidation then relies on TTL alone. +func NewWebSearchTool(secrets store.ConfigSecretsStore, msgBus *bus.MessageBus) *WebSearchTool { + t := &WebSearchTool{ + secrets: secrets, + cache: newWebCache(defaultCacheMaxEntries, defaultCacheTTL), + chainCache: newTenantChainCache(), } - ttl := cfg.CacheTTL - if ttl <= 0 { - ttl = defaultCacheTTL + if msgBus != nil { + msgBus.Subscribe("web_search:cache_invalidate", func(event bus.Event) { + if event.Name != protocol.EventCacheInvalidate { + return + } + payload, ok := event.Payload.(bus.CacheInvalidatePayload) + if !ok || payload.Kind != bus.CacheKindBuiltinTools || payload.Key != "web_search" { + return + } + if payload.TenantID == uuid.Nil { + // Master admin write — wipe all tenants. + t.chainCache.InvalidateAll() + } else { + t.chainCache.Invalidate(payload.TenantID) + } + }) } - return &WebSearchTool{ - providers: providers, - cache: newWebCache(defaultCacheMaxEntries, ttl), - } + return t } func (t *WebSearchTool) Name() string { return "web_search" } @@ -180,10 +201,8 @@ func (t *WebSearchTool) Execute(ctx context.Context, args map[string]any) *Resul return NewResult(cached) } - // Resolve per-request provider chain (tenant may reorder / disable providers - // via builtin_tool_tenant_configs.settings → ctx → ResolveWebSearchChain). - // Defaults preserved when no override — backward-compat. - chain := ResolveWebSearchChain(ctx, t.providers) + // Resolve per-request provider chain from tenant config_secrets + settings overlay. + chain := t.resolveChain(ctx) // Try providers in order (first success wins) var lastErr error @@ -243,4 +262,3 @@ func formatSearchResults(query string, results []searchResult, provider string) } return sb.String() } - diff --git a/internal/tools/web_search_config.go b/internal/tools/web_search_config.go index a4972061..bb45002d 100644 --- a/internal/tools/web_search_config.go +++ b/internal/tools/web_search_config.go @@ -3,69 +3,27 @@ package tools import ( "slices" "strings" - "time" - - "github.com/nextlevelbuilder/goclaw/internal/config" ) -// WebSearchConfig holds configuration for the web search tool. -type WebSearchConfig struct { - ProviderOrder []string - ExaAPIKey string - ExaEnabled bool - ExaMaxResults int - TavilyAPIKey string - TavilyEnabled bool - TavilyMaxResults int - BraveAPIKey string - BraveEnabled bool - BraveMaxResults int - DDGEnabled bool - DDGMaxResults int - CacheTTL time.Duration -} - -// WebSearchConfigFromConfig creates a WebSearchConfig from the global config. -func WebSearchConfigFromConfig(cfg *config.Config) WebSearchConfig { - return WebSearchConfig{ - ProviderOrder: cfg.Tools.Web.ProviderOrder, - ExaEnabled: cfg.Tools.Web.Exa.Enabled, - ExaAPIKey: cfg.Tools.Web.Exa.APIKey, - ExaMaxResults: cfg.Tools.Web.Exa.MaxResults, - TavilyEnabled: cfg.Tools.Web.Tavily.Enabled, - TavilyAPIKey: cfg.Tools.Web.Tavily.APIKey, - TavilyMaxResults: cfg.Tools.Web.Tavily.MaxResults, - BraveEnabled: cfg.Tools.Web.Brave.Enabled, - BraveAPIKey: cfg.Tools.Web.Brave.APIKey, - BraveMaxResults: cfg.Tools.Web.Brave.MaxResults, - DDGEnabled: true, - DDGMaxResults: cfg.Tools.Web.DuckDuckGo.MaxResults, +// buildProviderByName returns the SearchProvider for a known name. +// Returns nil for unknown names. DDG ignores apiKey (not required). +// maxResults <= 0 falls back to defaultSearchCount. +func buildProviderByName(name, apiKey string, maxResults int) SearchProvider { + if maxResults <= 0 { + maxResults = defaultSearchCount } -} - -func buildSearchProviders(cfg WebSearchConfig) []SearchProvider { - var providers []SearchProvider - for _, providerID := range NormalizeWebSearchProviderOrder(cfg.ProviderOrder) { - switch providerID { - case searchProviderExa: - if cfg.ExaEnabled && cfg.ExaAPIKey != "" { - providers = append(providers, newExaSearchProvider(cfg.ExaAPIKey, cfg.ExaMaxResults)) - } - case searchProviderTavily: - if cfg.TavilyEnabled && cfg.TavilyAPIKey != "" { - providers = append(providers, newTavilySearchProvider(cfg.TavilyAPIKey, cfg.TavilyMaxResults)) - } - case searchProviderBrave: - if cfg.BraveEnabled && cfg.BraveAPIKey != "" { - providers = append(providers, newBraveSearchProvider(cfg.BraveAPIKey, cfg.BraveMaxResults)) - } - case searchProviderDuckDuckGo: - if cfg.DDGEnabled { - providers = append(providers, newDuckDuckGoSearchProvider(cfg.DDGMaxResults)) - } - } + switch name { + case searchProviderExa: + return newExaSearchProvider(apiKey, maxResults) + case searchProviderTavily: + return newTavilySearchProvider(apiKey, maxResults) + case searchProviderBrave: + return newBraveSearchProvider(apiKey, maxResults) + case searchProviderDuckDuckGo: + return newDuckDuckGoSearchProvider(maxResults) + default: + return nil } - return providers } // NormalizeWebSearchProviderOrder normalizes user-specified provider order. diff --git a/internal/tools/web_search_config_test.go b/internal/tools/web_search_config_test.go deleted file mode 100644 index cf582d9a..00000000 --- a/internal/tools/web_search_config_test.go +++ /dev/null @@ -1,150 +0,0 @@ -package tools - -import ( - "reflect" - "testing" -) - -func TestNormalizeWebSearchProviderOrder_Empty(t *testing.T) { - got := NormalizeWebSearchProviderOrder(nil) - want := []string{"exa", "tavily", "brave", "duckduckgo"} - if !reflect.DeepEqual(got, want) { - t.Errorf("NormalizeWebSearchProviderOrder(nil) = %v, want %v", got, want) - } -} - -func TestNormalizeWebSearchProviderOrder_UserSpecified(t *testing.T) { - got := NormalizeWebSearchProviderOrder([]string{"brave", "exa"}) - // brave first, exa second (user order), tavily appended, ddg last - want := []string{"brave", "exa", "tavily", "duckduckgo"} - if !reflect.DeepEqual(got, want) { - t.Errorf("got %v, want %v", got, want) - } -} - -func TestNormalizeWebSearchProviderOrder_DDGIgnored(t *testing.T) { - // DDG in user order is ignored (always last) - got := NormalizeWebSearchProviderOrder([]string{"duckduckgo", "tavily"}) - want := []string{"tavily", "exa", "brave", "duckduckgo"} - if !reflect.DeepEqual(got, want) { - t.Errorf("got %v, want %v", got, want) - } -} - -func TestNormalizeWebSearchProviderOrder_Dedup(t *testing.T) { - got := NormalizeWebSearchProviderOrder([]string{"exa", "exa", "brave"}) - want := []string{"exa", "brave", "tavily", "duckduckgo"} - if !reflect.DeepEqual(got, want) { - t.Errorf("got %v, want %v", got, want) - } -} - -func TestNormalizeWebSearchProviderOrder_UnknownSkipped(t *testing.T) { - got := NormalizeWebSearchProviderOrder([]string{"bing", "tavily"}) - want := []string{"tavily", "exa", "brave", "duckduckgo"} - if !reflect.DeepEqual(got, want) { - t.Errorf("got %v, want %v", got, want) - } -} - -func TestBuildSearchProviders_AllEnabled(t *testing.T) { - cfg := WebSearchConfig{ - ExaEnabled: true, - ExaAPIKey: "exa-key", - TavilyEnabled: true, - TavilyAPIKey: "tavily-key", - BraveEnabled: true, - BraveAPIKey: "brave-key", - DDGEnabled: true, - } - providers := buildSearchProviders(cfg) - names := make([]string, len(providers)) - for i, p := range providers { - names[i] = p.Name() - } - want := []string{"exa", "tavily", "brave", "duckduckgo"} - if !reflect.DeepEqual(names, want) { - t.Errorf("got %v, want %v", names, want) - } -} - -func TestBuildSearchProviders_OnlyBraveAndDDG(t *testing.T) { - cfg := WebSearchConfig{ - BraveEnabled: true, - BraveAPIKey: "brave-key", - DDGEnabled: true, - } - providers := buildSearchProviders(cfg) - names := make([]string, len(providers)) - for i, p := range providers { - names[i] = p.Name() - } - // Exa + Tavily not enabled, so only brave + ddg - want := []string{"brave", "duckduckgo"} - if !reflect.DeepEqual(names, want) { - t.Errorf("got %v, want %v", names, want) - } -} - -func TestBuildSearchProviders_CustomOrder(t *testing.T) { - cfg := WebSearchConfig{ - ProviderOrder: []string{"tavily", "brave"}, - TavilyEnabled: true, - TavilyAPIKey: "key", - BraveEnabled: true, - BraveAPIKey: "key", - DDGEnabled: true, - } - providers := buildSearchProviders(cfg) - names := make([]string, len(providers)) - for i, p := range providers { - names[i] = p.Name() - } - // tavily first (user), brave second, exa appended (no key so skipped), ddg last - want := []string{"tavily", "brave", "duckduckgo"} - if !reflect.DeepEqual(names, want) { - t.Errorf("got %v, want %v", names, want) - } -} - -func TestBuildSearchProviders_NoProviders(t *testing.T) { - cfg := WebSearchConfig{} - providers := buildSearchProviders(cfg) - if len(providers) != 0 { - t.Errorf("expected empty providers, got %d", len(providers)) - } -} - -func TestClampProviderResultCount(t *testing.T) { - tests := []struct { - requested, max, want int - }{ - {5, 10, 5}, - {15, 10, 10}, - {0, 10, defaultSearchCount}, - {5, 0, 5}, - } - for _, tt := range tests { - got := clampProviderResultCount(tt.requested, tt.max) - if got != tt.want { - t.Errorf("clampProviderResultCount(%d, %d) = %d, want %d", tt.requested, tt.max, got, tt.want) - } - } -} - -func TestNormalizeProviderMaxResults(t *testing.T) { - tests := []struct { - input, want int - }{ - {0, defaultSearchCount}, - {-1, defaultSearchCount}, - {7, 7}, - {15, maxSearchCount}, - } - for _, tt := range tests { - got := normalizeProviderMaxResults(tt.input) - if got != tt.want { - t.Errorf("normalizeProviderMaxResults(%d) = %d, want %d", tt.input, got, tt.want) - } - } -} diff --git a/internal/tools/web_search_resolve_test.go b/internal/tools/web_search_resolve_test.go new file mode 100644 index 00000000..12404b52 --- /dev/null +++ b/internal/tools/web_search_resolve_test.go @@ -0,0 +1,201 @@ +package tools + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// TestResolveChain_CacheHit verifies that resolveChain returns cached chains. +// Scenario 7: back-to-back same-tenant calls return the same cached slice. +func TestResolveChain_CacheHit(t *testing.T) { + fake := newFakeSecretsStore() + tool := &WebSearchTool{ + secrets: fake, + cache: newWebCache(defaultCacheMaxEntries, defaultCacheTTL), + chainCache: newTenantChainCache(), + } + + tid := uuid.New() + ctx := context.Background() + ctx = store.WithTenantID(ctx, tid) + + // Set a provider key + fake.Set(ctx, "tools.web.brave.api_key", "test-key-brave") + + // First call → resolves and caches + chain1 := tool.resolveChain(ctx) + + // Second call → should hit cache + chain2 := tool.resolveChain(ctx) + + if len(chain1) == 0 || len(chain2) == 0 { + t.Fatalf("expected non-empty chains, got %d and %d", len(chain1), len(chain2)) + } + + // Both should have same provider order + if len(chain1) != len(chain2) { + t.Errorf("chain length mismatch: %d vs %d", len(chain1), len(chain2)) + } + + for i := 0; i < len(chain1); i++ { + if chain1[i].Name() != chain2[i].Name() { + t.Errorf("provider %d: %s vs %s", i, chain1[i].Name(), chain2[i].Name()) + } + } +} + +// TestResolveChain_TenantIsolation verifies that two tenants get independent chains. +// Scenario 8: switching tenant context yields different chains. +func TestResolveChain_TenantIsolation(t *testing.T) { + fake := newFakeSecretsStore() + tool := &WebSearchTool{ + secrets: fake, + cache: newWebCache(defaultCacheMaxEntries, defaultCacheTTL), + chainCache: newTenantChainCache(), + } + + tenantA := uuid.New() + tenantB := uuid.New() + + // Setup tenant A: Brave key only + ctxA := context.Background() + ctxA = store.WithTenantID(ctxA, tenantA) + fake.Set(ctxA, "tools.web.brave.api_key", "test-key-brave-A") + + // Setup tenant B: Exa key only + ctxB := context.Background() + ctxB = store.WithTenantID(ctxB, tenantB) + fake.Set(ctxB, "tools.web.exa.api_key", "test-key-exa-B") + + // Resolve chains for each tenant + chainA := tool.resolveChain(ctxA) + chainB := tool.resolveChain(ctxB) + + // Tenant A should have Brave + foundBraveInA := false + for _, p := range chainA { + if p.Name() == "brave" { + foundBraveInA = true + break + } + } + if !foundBraveInA { + t.Error("tenant A chain missing Brave") + } + + // Tenant B should have Exa + foundExaInB := false + for _, p := range chainB { + if p.Name() == "exa" { + foundExaInB = true + break + } + } + if !foundExaInB { + t.Error("tenant B chain missing Exa") + } + + // Tenant A should NOT have Exa + foundExaInA := false + for _, p := range chainA { + if p.Name() == "exa" { + foundExaInA = true + break + } + } + if foundExaInA { + t.Error("tenant A chain should not have Exa") + } + + // Tenant B should NOT have Brave + foundBraveInB := false + for _, p := range chainB { + if p.Name() == "brave" { + foundBraveInB = true + break + } + } + if foundBraveInB { + t.Error("tenant B chain should not have Brave") + } +} + +// TestResolveChain_CacheInvalidation verifies cache invalidation refreshes the chain. +func TestResolveChain_CacheInvalidation(t *testing.T) { + fake := newFakeSecretsStore() + tool := &WebSearchTool{ + secrets: fake, + cache: newWebCache(defaultCacheMaxEntries, defaultCacheTTL), + chainCache: newTenantChainCache(), + } + + tid := uuid.New() + ctx := context.Background() + ctx = store.WithTenantID(ctx, tid) + + // Initial state: only DDG + chain1 := tool.resolveChain(ctx) + if len(chain1) != 1 || chain1[0].Name() != "duckduckgo" { + t.Fatalf("initial chain should be DDG only, got %v", chainNames(chain1)) + } + + // Add a provider key and invalidate cache + fake.Set(ctx, "tools.web.brave.api_key", "test-key-brave-new") + tool.chainCache.Invalidate(tid) + + // Next resolve should pick up the new key + chain2 := tool.resolveChain(ctx) + if len(chain2) != 2 { + t.Errorf("after invalidation, expected 2 providers, got %d", len(chain2)) + } + + foundBrave := false + for _, p := range chain2 { + if p.Name() == "brave" { + foundBrave = true + break + } + } + if !foundBrave { + t.Error("after invalidation, Brave should be in chain") + } +} + +// TestResolveChain_TTLCacheExpiry verifies that expired cache entries trigger refresh. +func TestResolveChain_TTLCacheExpiry(t *testing.T) { + fake := newFakeSecretsStore() + tool := &WebSearchTool{ + secrets: fake, + cache: newWebCache(defaultCacheMaxEntries, defaultCacheTTL), + chainCache: newTenantChainCache(), + } + + tid := uuid.New() + ctx := context.Background() + ctx = store.WithTenantID(ctx, tid) + + // Initial state: DDG only + chain1 := tool.resolveChain(ctx) + if len(chain1) != 1 { + t.Fatalf("initial chain should be 1 provider, got %d", len(chain1)) + } + + // Manually expire the cache entry + tool.chainCache.entries[tid] = tenantChainEntry{ + chain: chain1, + expires: time.Now().Add(-1 * time.Second), // expired + } + + // Add provider and resolve again — should bypass expired cache + fake.Set(ctx, "tools.web.brave.api_key", "test-key-brave-after-ttl") + chain2 := tool.resolveChain(ctx) + + if len(chain2) != 2 { + t.Errorf("after TTL expiry, expected 2 providers, got %d", len(chain2)) + } +}