From 50295914c16dcb3711d5c7e7694cb8473bdd6cf2 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Mon, 8 Jun 2026 03:48:33 +0100 Subject: [PATCH] Remove hidden discovery provider fallback --- .../v6/internal/subsystems/ai-runtime.md | 2 +- .../src/content/help/__tests__/ai.test.ts | 12 ++++ frontend-modern/src/content/help/ai.ts | 4 +- internal/ai/service.go | 59 ++-------------- internal/ai/service_coverage_imp_test.go | 67 +++++++++++++++++++ 5 files changed, 88 insertions(+), 56 deletions(-) create mode 100644 frontend-modern/src/content/help/__tests__/ai.test.ts diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 719e32acd..15a879f53 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -1763,7 +1763,7 @@ deriving an older display status from `workflowStatusHistory`. divergence must be sourced from canonical `ReadState.DockerHosts()` views, with model-shaped data limited to the watcher adapter input rather than direct `StateSnapshot.DockerHosts` reads in the Patrol run loop. -4. Keep discovery scheduling authoritative through `internal/config/ai.go`: `discovery_enabled` and `discovery_interval_hours` must govern both lightweight infrastructure discovery and deep service-discovery background loops. `internal/api/ai_handlers.go` must preserve an explicitly supplied `discovery_interval_hours: 0` as the manual-only setting and may only apply the 24-hour default when discovery is enabled without an explicit interval payload. Discovery analysis remains a Pulse tool-led model workflow: Pulse supplies agent/API/metrics evidence and cache orchestration, while the selected model provides the intelligence. Background, settings-triggered, or drawer-triggered discovery progress must describe that discovery analysis directly and must not imply a live Pulse Assistant chat transcript unless the run is actually executing inside the chat surface. Settings-triggered manual discovery is an explicit operator refresh and must not open, append to, or masquerade as a Pulse Assistant session. Assistant and Patrol access to discovery must stay behind the governed `pulse_discovery` tool, including an explicit forced refresh action for known resources, while settings-level manual runs must use the canonical `/api/discovery/run` new/changed/stale/repairable sweep rather than a frontend-only shortcut. Fresh-but-unidentified service-discovery records are not complete when canonical resource metadata, stored facts, or safe command evidence can deterministically identify a known workload and endpoint; cached reads and the canonical sweep must repair those records instead of presenting `Unknown Service` as fresh. +4. Keep discovery scheduling authoritative through `internal/config/ai.go`: `discovery_enabled` and `discovery_interval_hours` must govern both lightweight infrastructure discovery and deep service-discovery background loops. `internal/api/ai_handlers.go` must preserve an explicitly supplied `discovery_interval_hours: 0` as the manual-only setting and may only apply the 24-hour default when discovery is enabled without an explicit interval payload. Discovery analysis remains a Pulse tool-led model workflow: Pulse supplies agent/API/metrics evidence and cache orchestration, while the selected model provides the intelligence. Background, settings-triggered, or drawer-triggered discovery progress must describe that discovery analysis directly and must not imply a live Pulse Assistant chat transcript unless the run is actually executing inside the chat surface. Settings-triggered manual discovery is an explicit operator refresh and must not open, append to, or masquerade as a Pulse Assistant session. Assistant and Patrol access to discovery must stay behind the governed `pulse_discovery` tool, including an explicit forced refresh action for known resources, while settings-level manual runs must use the canonical `/api/discovery/run` new/changed/stale/repairable sweep rather than a frontend-only shortcut. Discovery analysis must use the selected discovery route only: provider-owned default resolution may choose a model before the request when no explicit route exists, but `AnalyzeForDiscovery` must not fall back to a stale default provider or another configured provider after selected-route provider construction or execution fails. Fresh-but-unidentified service-discovery records are not complete when canonical resource metadata, stored facts, or safe command evidence can deterministically identify a known workload and endpoint; cached reads and the canonical sweep must repair those records instead of presenting `Unknown Service` as fresh. 5. Preserve auditability for outbound model-bound context exports and keep the export record aligned with the prompt boundary that actually reaches the provider External provider-bound unified-resource context must enforce the same data-handling policy the export audit records: `local-only` resources are diff --git a/frontend-modern/src/content/help/__tests__/ai.test.ts b/frontend-modern/src/content/help/__tests__/ai.test.ts new file mode 100644 index 000000000..6904dd2e1 --- /dev/null +++ b/frontend-modern/src/content/help/__tests__/ai.test.ts @@ -0,0 +1,12 @@ +import { describe, expect, it } from 'vitest'; +import { aiHelpContent } from '../ai'; + +describe('AI help content', () => { + it('describes provider route recovery as explicit instead of automatic fallback', () => { + const providerHelp = aiHelpContent.find((item) => item.id === 'ai.providers.overview'); + + expect(providerHelp?.description).toContain('choose the route'); + expect(providerHelp?.description).toContain('switch routes explicitly'); + expect(providerHelp?.description).not.toMatch(/fallback to others|automatic provider fallback/i); + }); +}); diff --git a/frontend-modern/src/content/help/ai.ts b/frontend-modern/src/content/help/ai.ts index d5d19dc68..ffaca79b0 100644 --- a/frontend-modern/src/content/help/ai.ts +++ b/frontend-modern/src/content/help/ai.ts @@ -45,8 +45,8 @@ export const aiHelpContent: HelpContent[] = [ '- Natural language infrastructure queries\n' + '- Automated troubleshooting suggestions\n' + '- Alert investigation assistance\n\n' + - 'You can configure multiple providers and Pulse will use the primary provider ' + - 'with fallback to others if unavailable.', + 'You can configure multiple providers and choose the route for Assistant, Patrol, and discovery. ' + + 'Pulse retries the selected route when appropriate and asks you to switch routes explicitly if that provider is unavailable.', related: ['ai.openai.baseUrl', 'ai.ollama.baseUrl', 'ai.ollama.keepAlive'], addedInVersion: 'v4.0.0', }, diff --git a/internal/ai/service.go b/internal/ai/service.go index d8961197c..4d8a33749 100644 --- a/internal/ai/service.go +++ b/internal/ai/service.go @@ -3525,12 +3525,13 @@ func (s *Service) GetKnowledgeStore() *knowledge.Store { // It sends a prompt to the AI using the discovery model (optimized for cost). // Dynamically creates a provider matching the discovery model to avoid using a // stale or mismatched global provider (e.g., when the user selects a different -// model per-session in chat). Falls back to other configured providers on failure. +// model per-session in chat). Discovery uses the selected route only: provider +// failures fail closed instead of silently trying another configured provider. func (s *Service) AnalyzeForDiscovery(ctx context.Context, prompt string) (string, error) { s.mu.RLock() cfg := s.cfg costStore := s.costStore - fallbackProvider := s.provider // keep as last resort + defaultProvider := s.provider s.mu.RUnlock() if cfg == nil || !cfg.Enabled { @@ -3560,11 +3561,10 @@ func (s *Service) AnalyzeForDiscovery(ctx context.Context, prompt string) (strin var providerErr error provider, providerErr = providers.NewForModel(cfg, model) if providerErr != nil { - log.Debug().Err(providerErr).Str("model", model).Msg("[Discovery] Could not create provider for discovery model, using default") - provider = fallbackProvider + return "", fmt.Errorf("discovery provider for selected model %q is not configured: %w", model, providerErr) } } else { - provider = fallbackProvider + provider = defaultProvider } if provider == nil { @@ -3583,55 +3583,8 @@ func (s *Service) AnalyzeForDiscovery(ctx context.Context, prompt string) (strin discoveryReq = sanitizer(discoveryReq) } resp, err := provider.Chat(ctx, discoveryReq) - - // If the primary provider fails (e.g., rate limited), try other configured providers if err != nil { - primaryErr := err - primaryProvider, _ := config.ParseModelString(model) - - for _, altProviderName := range cfg.GetConfiguredProviders() { - if altProviderName == primaryProvider { - continue // skip the one that just failed - } - if err := s.enforceBudget("discovery"); err != nil { - return "", err - } - - altModel, resolveErr := ResolveConfiguredProviderModel(ctx, cfg, altProviderName) - if resolveErr != nil || altModel == "" { - continue - } - - altProvider, createErr := providers.NewForModel(cfg, altModel) - if createErr != nil { - continue - } - - log.Info(). - Str("failed_provider", primaryProvider). - Str("fallback_provider", altProviderName). - Str("fallback_model", altModel). - Msg("[Discovery] Primary provider failed, trying fallback") - - altReq := providers.ChatRequest{ - Messages: messages, - Model: altModel, - MaxTokens: discoveryResponseTokenBudget, - } - if sanitizer := s.requestSanitizerForModel(altModel); sanitizer != nil { - altReq = sanitizer(altReq) - } - resp, err = altProvider.Chat(ctx, altReq) - if err == nil { - model = altModel - provider = altProvider - break - } - } - - if err != nil { - return "", fmt.Errorf("discovery analysis failed: %w (primary error: %v)", err, primaryErr) - } + return "", fmt.Errorf("discovery analysis failed on selected provider route: %w", err) } // Track cost if cost store is available diff --git a/internal/ai/service_coverage_imp_test.go b/internal/ai/service_coverage_imp_test.go index a9a5f53b1..29348e9be 100644 --- a/internal/ai/service_coverage_imp_test.go +++ b/internal/ai/service_coverage_imp_test.go @@ -2,6 +2,9 @@ package ai import ( "context" + "errors" + "net/http" + "net/http/httptest" "strings" "testing" "time" @@ -206,6 +209,70 @@ func TestService_AnalyzeForDiscovery(t *testing.T) { } } +func TestService_AnalyzeForDiscoveryDoesNotFallbackToAnotherConfiguredProvider(t *testing.T) { + var alternateProviderCalls int + alternateProviderServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + alternateProviderCalls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "id":"chatcmpl-fallback", + "object":"chat.completion", + "choices":[{"message":{"role":"assistant","content":"fallback discovery response"},"finish_reason":"stop"}], + "usage":{"prompt_tokens":1,"completion_tokens":1} + }`)) + })) + defer alternateProviderServer.Close() + + selectedProviderCalls := 0 + svc := NewService(nil, nil) + svc.provider = &mockProvider{ + chatFunc: func(ctx context.Context, req providers.ChatRequest) (*providers.ChatResponse, error) { + selectedProviderCalls++ + return nil, errors.New("selected discovery route unavailable") + }, + nameFunc: func() string { return "selected-provider" }, + } + svc.cfg = &config.AIConfig{ + Enabled: true, + OpenAIAPIKey: "openai-test", + OpenAIBaseURL: alternateProviderServer.URL, + } + + _, err := svc.AnalyzeForDiscovery(context.Background(), "test discovery") + if err == nil || !strings.Contains(err.Error(), "selected discovery route unavailable") { + t.Fatalf("expected selected route failure, got %v", err) + } + if selectedProviderCalls != 1 { + t.Fatalf("selected provider calls = %d, want 1", selectedProviderCalls) + } + if alternateProviderCalls != 0 { + t.Fatalf("alternate provider calls = %d, want no hidden provider fallback", alternateProviderCalls) + } +} + +func TestService_AnalyzeForDiscoveryExplicitModelProviderCreationFailsClosed(t *testing.T) { + defaultProviderCalls := 0 + svc := NewService(nil, nil) + svc.provider = &mockProvider{ + chatFunc: func(ctx context.Context, req providers.ChatRequest) (*providers.ChatResponse, error) { + defaultProviderCalls++ + return &providers.ChatResponse{Content: "default provider response"}, nil + }, + } + svc.cfg = &config.AIConfig{ + Enabled: true, + DiscoveryModel: config.FormatModelString(config.AIProviderDeepSeek, config.DeepSeekModelV4Pro), + } + + _, err := svc.AnalyzeForDiscovery(context.Background(), "test discovery") + if err == nil || !strings.Contains(err.Error(), "discovery provider for selected model") { + t.Fatalf("expected selected discovery provider configuration error, got %v", err) + } + if defaultProviderCalls != 0 { + t.Fatalf("default provider calls = %d, want no stale provider fallback", defaultProviderCalls) + } +} + func TestService_RecordIncidentRunbook(t *testing.T) { svc := NewService(nil, nil) canonicalStore := unifiedresources.NewMemoryStore()