fix(provider): fail closed on ambiguous identities

Amp-Thread-ID: https://ampcode.com/threads/T-01a0246b-e61a-70e8-aa71-24f1ea7804c8
Co-authored-by: Amp <amp@ampcode.com>

Author: PierrunoYT

internal/cli/auth_test.go

@@ -851,12 +851,15 @@ func TestRunAuthRefreshRejectsEmptyCredentialCandidates(t *testing.T) {
 	if err := os.WriteFile(configPath, []byte(`{"providers":[]}`), 0o600); err != nil {
 		t.Fatal(err)
 	}
-	var stdout, stderr bytes.Buffer
-	code := runWithDeps([]string{"auth", "refresh", "   "}, &stdout, &stderr, appDeps{
-		userConfigPath: func() (string, error) { return configPath, nil },
-	})
-	if code != exitCrash || !strings.Contains(stderr.String(), "no credential candidates") {
-		t.Fatalf("exit = %d stderr = %q, want empty-candidate error", code, stderr.String())
+	for _, extra := range [][]string{nil, {"--watch"}} {
+		args := append([]string{"auth", "refresh", "   "}, extra...)
+		var stdout, stderr bytes.Buffer
+		code := runWithDeps(args, &stdout, &stderr, appDeps{
+			userConfigPath: func() (string, error) { return configPath, nil },
+		})
+		if code != exitCrash || stdout.Len() != 0 || !strings.Contains(stderr.String(), "no credential candidates") {
+			t.Fatalf("args = %q exit = %d stdout = %q stderr = %q, want explicit empty-candidate app error", args, code, stdout.String(), stderr.String())
+		}
 	}
 }
 

internal/cli/provider_onboarding_test.go

@@ -105,6 +105,10 @@ func TestProviderRemoveRejectsAmbiguousFoldedName(t *testing.T) {
 			{Name: "WORK", ProviderKind: config.ProviderKindOpenAICompatible, BaseURL: "https://upper.example/v1", Model: "m2", APIKeyStored: true},
 		},
 	})
+	configBefore, err := os.ReadFile(configPath)
+	if err != nil {
+		t.Fatal(err)
+	}
 	store, err := config.ProviderKeyStoreAt(filepath.Dir(configPath))
 	if err != nil {
 		t.Fatal(err)
@@ -121,13 +125,13 @@ func TestProviderRemoveRejectsAmbiguousFoldedName(t *testing.T) {
 		t.Fatalf("stderr = %q, want an ambiguous-provider-name error", stderr.String())
 	}
 
-	cfg := readFileConfig(t, configPath)
-	if len(cfg.Providers) != 2 || cfg.Providers[0].Name != "work" || cfg.Providers[1].Name != "WORK" {
-		t.Fatalf("providers = %+v, want both rows untouched", cfg.Providers)
+	configAfter, err := os.ReadFile(configPath)
+	if err != nil || !bytes.Equal(configAfter, configBefore) {
+		t.Fatalf("config changed after rejected removal: err=%v\nbefore=%s\nafter=%s", err, configBefore, configAfter)
 	}
 	key, ok, err := store.Get("work")
 	if err != nil || !ok || key != "sk-lower" {
-		t.Fatalf("stored key = %q ok=%v err=%v, want the credential preserved", key, ok, err)
+		t.Fatalf("stored key = %q ok=%v err=%v, want the credential state preserved", key, ok, err)
 	}
 
 	// The exact spelling still works, so a legacy config remains repairable.
@@ -136,7 +140,7 @@ func TestProviderRemoveRejectsAmbiguousFoldedName(t *testing.T) {
 	if code := runWithDeps([]string{"providers", "remove", "WORK"}, &stdout, &stderr, providerSetupDeps(configPath)); code != exitSuccess {
 		t.Fatalf("exact-name removal exit = %d stderr = %q", code, stderr.String())
 	}
-	cfg = readFileConfig(t, configPath)
+	cfg := readFileConfig(t, configPath)
 	if len(cfg.Providers) != 1 || cfg.Providers[0].Name != "work" {
 		t.Fatalf("providers = %+v, want only the exact row removed", cfg.Providers)
 	}

internal/config/writer.go

@@ -2,7 +2,6 @@ package config
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -220,20 +219,19 @@ func PersistedProviderIdentity(path, identity string) (string, bool, error) {
 type PersistedIdentityMatch uint8
 
 const (
-	// PersistedIdentityNone means no row owns the identity, or a catalog id was
-	// given that more than one row claims (an ambiguous request this package
-	// refuses to guess at).
+	// PersistedIdentityNone means no row owns the identity.
 	PersistedIdentityNone PersistedIdentityMatch = iota
 	// PersistedIdentityName means a row's own name matched, exactly or as a
 	// case variant.
 	PersistedIdentityName
 	// PersistedIdentityCatalog means the identity matched only the catalog id of
 	// exactly one row.
 	PersistedIdentityCatalog
+	// PersistedIdentityAmbiguous means multiple rows own the folded name or
+	// catalog id. No caller may select a row or credential from this result.
+	PersistedIdentityAmbiguous
 )
 
-var errAmbiguousPersistedProviderIdentity = errors.New("ambiguous persisted provider identity")
-
 // ResolvePersistedProviderIdentity finds the persisted user-config row that
 // owns identity and reports how it was addressed.
 //
@@ -292,11 +290,14 @@ func ResolvePersistedProviderIdentity(path, identity string) (ProviderProfile, P
 		return *foldedName, PersistedIdentityName, nil
 	}
 	if foldedMatches > 1 {
-		return ProviderProfile{}, PersistedIdentityNone, fmt.Errorf("%w: ambiguous provider name %q matches multiple persisted rows that differ only by case; use the exact spelling from config.json", errAmbiguousPersistedProviderIdentity, identity)
+		return ProviderProfile{}, PersistedIdentityAmbiguous, fmt.Errorf("ambiguous provider name %q matches multiple persisted rows that differ only by case; use the exact spelling from config.json", identity)
 	}
 	if catalogMatches == 1 {
 		return *catalogRow, PersistedIdentityCatalog, nil
 	}
+	if catalogMatches > 1 {
+		return ProviderProfile{}, PersistedIdentityAmbiguous, fmt.Errorf("provider identity %q is ambiguous: %d saved profiles use it as a catalog id", identity, catalogMatches)
+	}
 	return ProviderProfile{}, PersistedIdentityNone, nil
 }
 
@@ -353,25 +354,12 @@ func ProviderCredentialCandidates(path, addressedName string) (candidates []stri
 	add(canonicalName)
 	row, match, err := ResolvePersistedProviderIdentity(path, addressedName)
 	if err != nil {
-		if errors.Is(err, errAmbiguousPersistedProviderIdentity) {
+		if match == PersistedIdentityAmbiguous {
 			return nil, canonicalName, err
 		}
 		return candidates, canonicalName, err
 	}
 	if match == PersistedIdentityNone {
-		providers, err := persistedProviders(path)
-		if err != nil {
-			return candidates, canonicalName, err
-		}
-		catalogMatches := 0
-		for _, provider := range providers {
-			if sameProviderIdentity(provider.CatalogID, addressedName) {
-				catalogMatches++
-			}
-		}
-		if catalogMatches > 1 {
-			return nil, canonicalName, fmt.Errorf("provider identity %q is ambiguous: %d saved profiles use it as a catalog id", addressedName, catalogMatches)
-		}
 		return candidates, canonicalName, nil
 	}
 	canonicalName = strings.TrimSpace(row.Name)

internal/config/writer_test.go

@@ -1414,8 +1414,8 @@ func TestResolvePersistedProviderIdentityPrefersNames(t *testing.T) {
 		writeConfigFixture(t, variants, FileConfig{
 			Providers: []ProviderProfile{{Name: "work"}, {Name: "WORK"}},
 		}, 0o600)
-		if _, _, err := ResolvePersistedProviderIdentity(variants, "wOrK"); err == nil {
-			t.Fatal("resolve succeeded for a name matching two case-variant rows, want an ambiguity error")
+		if _, match, err := ResolvePersistedProviderIdentity(variants, "wOrK"); err == nil || match != PersistedIdentityAmbiguous {
+			t.Fatalf("match = %v err = %v, want a distinct ambiguity result", match, err)
 		}
 		// An exact spelling still addresses one row, so a legacy config with such
 		// a pair stays repairable.
@@ -1450,8 +1450,8 @@ func TestResolvePersistedProviderIdentityPrefersNames(t *testing.T) {
 				{Name: "personal-xai", CatalogID: "xai"},
 			},
 		}, 0o600)
-		if _, match, err := ResolvePersistedProviderIdentity(shared, "xai"); err != nil || match != PersistedIdentityNone {
-			t.Fatalf("match = %v err = %v, want no guess at a shared catalog id", match, err)
+		if _, match, err := ResolvePersistedProviderIdentity(shared, "xai"); err == nil || match != PersistedIdentityAmbiguous {
+			t.Fatalf("match = %v err = %v, want distinct ambiguity for a shared catalog id", match, err)
 		}
 	})
 }