From 035a532c0aee254f5685d18e0415eaa3c276431e Mon Sep 17 00:00:00 2001 From: Andy Anderson Date: Mon, 29 Jun 2026 12:28:44 -0400 Subject: [PATCH 1/2] [quality] add extended registry + provider_prompts tests Adds 15+ test cases covering previously untested paths in pkg/agent: Registry (registry_extended_test.go): - Nil receiver safety for all public methods - RemoveSelectedAgent cleanup verification - promoteExecutingDefault (3 scenarios: promotion, no-change, no-better-option) - LRU eviction when selectedAgent map exceeds 10000 cap - Concurrent access safety (race detector coverage) - GetDefault with missing provider in map - SetDefault with nil provider entry - HasAvailableProviders with nil map entry Provider prompts (provider_prompts_test.go): - OSCommandHint returns non-empty, contains shell reference - OSContext matches runtime.GOOS - DefaultSystemPrompt and ChatOnlySystemPrompt non-empty and differ Partially addresses #19907. Signed-off-by: quality-agent --- pkg/agent/provider_prompts_test.go | 62 +++++++ pkg/agent/registry_extended_test.go | 258 ++++++++++++++++++++++++++++ 2 files changed, 320 insertions(+) create mode 100644 pkg/agent/provider_prompts_test.go create mode 100644 pkg/agent/registry_extended_test.go diff --git a/pkg/agent/provider_prompts_test.go b/pkg/agent/provider_prompts_test.go new file mode 100644 index 0000000000..12dd4aab07 --- /dev/null +++ b/pkg/agent/provider_prompts_test.go @@ -0,0 +1,62 @@ +package agent + +import ( + "runtime" + "strings" + "testing" +) + +func TestOSCommandHint_NotEmpty(t *testing.T) { + hint := OSCommandHint() + if hint == "" { + t.Error("OSCommandHint() should not return empty string") + } +} + +func TestOSCommandHint_ContainsShellReference(t *testing.T) { + hint := OSCommandHint() + // On Linux/macOS the hint should mention bash or shell conventions. + // On Windows it should mention PowerShell or cmd. + switch runtime.GOOS { + case "windows": + if !strings.Contains(strings.ToLower(hint), "powershell") && + !strings.Contains(strings.ToLower(hint), "cmd") { + t.Errorf("expected Windows hint to mention PowerShell/cmd, got %q", hint) + } + default: + if !strings.Contains(strings.ToLower(hint), "bash") && + !strings.Contains(strings.ToLower(hint), "sh") && + !strings.Contains(strings.ToLower(hint), "shell") { + t.Errorf("expected Unix hint to mention bash/sh/shell, got %q", hint) + } + } +} + +func TestOSContext_MatchesRuntime(t *testing.T) { + ctx := OSContext() + if ctx == "" { + t.Error("OSContext() should not return empty string") + } + // Should contain the current OS + if !strings.Contains(strings.ToLower(ctx), runtime.GOOS) { + t.Errorf("expected OSContext to mention %q, got %q", runtime.GOOS, ctx) + } +} + +func TestDefaultSystemPrompt_NotEmpty(t *testing.T) { + if DefaultSystemPrompt == "" { + t.Error("DefaultSystemPrompt should not be empty") + } +} + +func TestChatOnlySystemPrompt_NotEmpty(t *testing.T) { + if ChatOnlySystemPrompt == "" { + t.Error("ChatOnlySystemPrompt should not be empty") + } +} + +func TestChatOnlySystemPrompt_DiffersFromDefault(t *testing.T) { + if ChatOnlySystemPrompt == DefaultSystemPrompt { + t.Error("ChatOnlySystemPrompt should differ from DefaultSystemPrompt") + } +} diff --git a/pkg/agent/registry_extended_test.go b/pkg/agent/registry_extended_test.go new file mode 100644 index 0000000000..baa33e9e20 --- /dev/null +++ b/pkg/agent/registry_extended_test.go @@ -0,0 +1,258 @@ +package agent + +import ( + "context" + "fmt" + "sync" + "testing" + "time" +) + +// mockToolProvider implements ai.Provider with CapabilityToolExec. +type mockToolProvider struct { + name string + available bool +} + +func (m *mockToolProvider) Name() string { return m.name } +func (m *mockToolProvider) DisplayName() string { return m.name } +func (m *mockToolProvider) Description() string { return m.name } +func (m *mockToolProvider) Provider() string { return "mock" } +func (m *mockToolProvider) IsAvailable() bool { return m.available } +func (m *mockToolProvider) Capabilities() ProviderCapability { return CapabilityToolExec } +func (m *mockToolProvider) Chat(_ context.Context, _ *ChatRequest) (*ChatResponse, error) { + return nil, nil +} +func (m *mockToolProvider) StreamChat(_ context.Context, _ *ChatRequest, _ func(string)) (*ChatResponse, error) { + return nil, nil +} + +func newTestRegistry() *Registry { + return &Registry{ + providers: make(map[string]AIProvider), + selectedAgent: make(map[string]string), + selectedAgentLRU: make(map[string]time.Time), + } +} + +func TestRegistry_NilReceiver(t *testing.T) { + var r *Registry + + if err := r.Register(&MockProvider{name: "x", available: true}); err == nil { + t.Error("expected error from nil registry Register") + } + if _, err := r.Get("x"); err == nil { + t.Error("expected error from nil registry Get") + } + if _, err := r.GetDefault(); err == nil { + t.Error("expected error from nil registry GetDefault") + } + if r.GetDefaultName() != "" { + t.Error("expected empty string from nil registry GetDefaultName") + } + if err := r.SetDefault("x"); err == nil { + t.Error("expected error from nil registry SetDefault") + } + if r.GetSelectedAgent("s") != "" { + t.Error("expected empty string from nil registry GetSelectedAgent") + } + if err := r.SetSelectedAgent("s", "x"); err == nil { + t.Error("expected error from nil registry SetSelectedAgent") + } + // RemoveSelectedAgent on nil should not panic + r.RemoveSelectedAgent("s") + if r.List() != nil { + t.Error("expected nil from nil registry List") + } + if r.ListAvailable() != nil { + t.Error("expected nil from nil registry ListAvailable") + } + if r.HasAvailableProviders() { + t.Error("expected false from nil registry HasAvailableProviders") + } + // promoteExecutingDefault should not panic on nil + r.promoteExecutingDefault() +} + +func TestRegistry_RemoveSelectedAgent(t *testing.T) { + r := newTestRegistry() + p := &MockProvider{name: "a", available: true} + r.Register(p) + + // Set a session agent + if err := r.SetSelectedAgent("sess-1", "a"); err != nil { + t.Fatalf("SetSelectedAgent: %v", err) + } + if r.GetSelectedAgent("sess-1") != "a" { + t.Fatalf("expected 'a', got %q", r.GetSelectedAgent("sess-1")) + } + + // Remove it + r.RemoveSelectedAgent("sess-1") + + // Should fall back to default + if got := r.GetSelectedAgent("sess-1"); got != "a" { + t.Errorf("expected default 'a' after removal, got %q", got) + } + + // Verify internal maps are clean + r.mu.RLock() + _, inAgent := r.selectedAgent["sess-1"] + _, inLRU := r.selectedAgentLRU["sess-1"] + r.mu.RUnlock() + if inAgent || inLRU { + t.Error("expected session to be removed from internal maps") + } + + // Remove with empty string should not panic + r.RemoveSelectedAgent("") +} + +func TestRegistry_PromoteExecutingDefault(t *testing.T) { + r := newTestRegistry() + + // Register copilot-cli (suggest-only) first so it becomes default + cli := &MockProvider{name: "copilot-cli", available: true} + r.Register(cli) + if r.GetDefaultName() != "copilot-cli" { + t.Fatalf("expected copilot-cli as default, got %q", r.GetDefaultName()) + } + + // Register a tool-capable provider + tool := &mockToolProvider{name: "claude-code", available: true} + r.Register(tool) + + // Promote should switch default to the tool-capable agent + r.promoteExecutingDefault() + if r.GetDefaultName() != "claude-code" { + t.Errorf("expected default promoted to claude-code, got %q", r.GetDefaultName()) + } +} + +func TestRegistry_PromoteExecutingDefault_NoChange(t *testing.T) { + r := newTestRegistry() + + // Register a tool-capable provider as default (not in suggestOnlyAgents) + tool := &mockToolProvider{name: "claude-code", available: true} + r.Register(tool) + + // promoteExecutingDefault should not change anything + r.promoteExecutingDefault() + if r.GetDefaultName() != "claude-code" { + t.Errorf("expected default unchanged, got %q", r.GetDefaultName()) + } +} + +func TestRegistry_PromoteExecutingDefault_NoBetterOption(t *testing.T) { + r := newTestRegistry() + + // Only copilot-cli registered, no tool-capable alternative + cli := &MockProvider{name: "copilot-cli", available: true} + r.Register(cli) + + r.promoteExecutingDefault() + // Should keep copilot-cli as fallback + if r.GetDefaultName() != "copilot-cli" { + t.Errorf("expected copilot-cli retained as fallback, got %q", r.GetDefaultName()) + } +} + +func TestRegistry_SetSelectedAgent_LRUEviction(t *testing.T) { + r := newTestRegistry() + p := &MockProvider{name: "a", available: true} + r.Register(p) + + // Fill the registry with sessions just under the cap (we use a smaller + // batch to make the test tractable — the real cap is 10000). + // We directly manipulate the maps to simulate historical sessions. + r.mu.Lock() + for i := 0; i < 10000; i++ { + sid := fmt.Sprintf("old-session-%d", i) + r.selectedAgent[sid] = "a" + // All old sessions have old timestamps + r.selectedAgentLRU[sid] = time.Now().Add(-time.Hour) + } + r.mu.Unlock() + + // Adding one more should trigger eviction + if err := r.SetSelectedAgent("new-session", "a"); err != nil { + t.Fatalf("SetSelectedAgent failed: %v", err) + } + + r.mu.RLock() + count := len(r.selectedAgent) + r.mu.RUnlock() + + // After eviction of 5% (500), we should have ~9501 entries + if count > 9600 { + t.Errorf("expected eviction to reduce count, got %d", count) + } + if count < 9400 { + t.Errorf("eviction removed too many entries, got %d", count) + } + + // The new session should still be present + if r.GetSelectedAgent("new-session") != "a" { + t.Error("new session should still be present after eviction") + } +} + +func TestRegistry_ConcurrentAccess(t *testing.T) { + r := newTestRegistry() + p := &MockProvider{name: "concurrent", available: true} + r.Register(p) + + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + sid := fmt.Sprintf("session-%d", id) + _ = r.SetSelectedAgent(sid, "concurrent") + _ = r.GetSelectedAgent(sid) + r.RemoveSelectedAgent(sid) + _ = r.List() + _ = r.ListAvailable() + _ = r.HasAvailableProviders() + }(i) + } + wg.Wait() +} + +func TestRegistry_GetDefault_NotFound(t *testing.T) { + r := newTestRegistry() + // Set a default name manually but don't register the provider + r.mu.Lock() + r.defaultAgent = "phantom" + r.mu.Unlock() + + _, err := r.GetDefault() + if err == nil { + t.Error("expected error when default provider not found in map") + } +} + +func TestRegistry_SetDefault_NilProvider(t *testing.T) { + r := newTestRegistry() + // Inject nil provider directly into map + r.mu.Lock() + r.providers["nil-provider"] = nil + r.mu.Unlock() + + err := r.SetDefault("nil-provider") + if err == nil { + t.Error("expected error when setting nil provider as default") + } +} + +func TestRegistry_HasAvailableProviders_WithNilEntry(t *testing.T) { + r := newTestRegistry() + // Inject nil provider + r.mu.Lock() + r.providers["nil-entry"] = nil + r.mu.Unlock() + + if r.HasAvailableProviders() { + t.Error("nil provider should not count as available") + } +} From 3ef59826fd2db2f2d5147f510fa591e5bcef9a9b Mon Sep 17 00:00:00 2001 From: Andy Anderson Date: Mon, 29 Jun 2026 13:39:42 -0400 Subject: [PATCH 2/2] fix: rename mockToolProvider to mockRegToolProvider to avoid redeclaration Signed-off-by: clubanderson --- pkg/agent/registry_extended_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/agent/registry_extended_test.go b/pkg/agent/registry_extended_test.go index baa33e9e20..c004ef9f57 100644 --- a/pkg/agent/registry_extended_test.go +++ b/pkg/agent/registry_extended_test.go @@ -8,22 +8,22 @@ import ( "time" ) -// mockToolProvider implements ai.Provider with CapabilityToolExec. -type mockToolProvider struct { +// mockRegToolProvider implements ai.Provider with CapabilityToolExec. +type mockRegToolProvider struct { name string available bool } -func (m *mockToolProvider) Name() string { return m.name } -func (m *mockToolProvider) DisplayName() string { return m.name } -func (m *mockToolProvider) Description() string { return m.name } -func (m *mockToolProvider) Provider() string { return "mock" } -func (m *mockToolProvider) IsAvailable() bool { return m.available } -func (m *mockToolProvider) Capabilities() ProviderCapability { return CapabilityToolExec } -func (m *mockToolProvider) Chat(_ context.Context, _ *ChatRequest) (*ChatResponse, error) { +func (m *mockRegToolProvider) Name() string { return m.name } +func (m *mockRegToolProvider) DisplayName() string { return m.name } +func (m *mockRegToolProvider) Description() string { return m.name } +func (m *mockRegToolProvider) Provider() string { return "mock" } +func (m *mockRegToolProvider) IsAvailable() bool { return m.available } +func (m *mockRegToolProvider) Capabilities() ProviderCapability { return CapabilityToolExec } +func (m *mockRegToolProvider) Chat(_ context.Context, _ *ChatRequest) (*ChatResponse, error) { return nil, nil } -func (m *mockToolProvider) StreamChat(_ context.Context, _ *ChatRequest, _ func(string)) (*ChatResponse, error) { +func (m *mockRegToolProvider) StreamChat(_ context.Context, _ *ChatRequest, _ func(string)) (*ChatResponse, error) { return nil, nil } @@ -119,7 +119,7 @@ func TestRegistry_PromoteExecutingDefault(t *testing.T) { } // Register a tool-capable provider - tool := &mockToolProvider{name: "claude-code", available: true} + tool := &mockRegToolProvider{name: "claude-code", available: true} r.Register(tool) // Promote should switch default to the tool-capable agent @@ -133,7 +133,7 @@ func TestRegistry_PromoteExecutingDefault_NoChange(t *testing.T) { r := newTestRegistry() // Register a tool-capable provider as default (not in suggestOnlyAgents) - tool := &mockToolProvider{name: "claude-code", available: true} + tool := &mockRegToolProvider{name: "claude-code", available: true} r.Register(tool) // promoteExecutingDefault should not change anything