fix: enable per-call tool discovery for shared oauth/headers/none MCP clients and rename UpdateClientConnection to UpdateClientCredentials

core/bifrost.go

@@ -4204,12 +4204,12 @@ func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPCli
 	return bifrost.MCPManager.UpdateClient(id, updatedConfig)
 }
 
-// UpdateMCPClientConnection reconnects an existing MCP client using updated headers
-func (bifrost *Bifrost) UpdateMCPClientConnection(id string, newConfig *schemas.MCPClientConfig) error {
+// UpdateMCPClientCredentials reconnects an existing MCP client using updated headers
+func (bifrost *Bifrost) UpdateMCPClientCredentials(id string, newConfig *schemas.MCPClientConfig) error {
 	if bifrost.MCPManager == nil {
 		return fmt.Errorf("mcp is not configured in this bifrost instance")
 	}
-	return bifrost.MCPManager.UpdateClientConnection(id, newConfig)
+	return bifrost.MCPManager.UpdateClientCredentials(id, newConfig)
 }
 
 // ReconnectMCPClient attempts to reconnect an MCP client if it is disconnected.

core/mcp/addclient_discoveredtools_test.go

@@ -98,3 +98,69 @@ func TestAddClient_PerUserHeaders_DiscoveredToolsStateSelection(t *testing.T) {
 		})
 	}
 }
+
+// TestAddClient_PerCallConnection_StartsConnectionChecker pins the fix for
+// the shared-per-call tool-discovery bug: AddClient's per-call branch (any
+// auth type RequiresPerCallConnection resolves true for — per_user_headers
+// here) must start a connection checker, exactly like the sticky success
+// path (connectToMCPClient) already does. Without one, nothing ever
+// revisits a client whose DiscoveredTools was empty/nil at add time — it
+// would sit at Healthy/0-tools forever.
+func TestAddClient_PerCallConnection_StartsConnectionChecker(t *testing.T) {
+	m := NewMCPManager(context.Background(), schemas.MCPConfig{}, nil, &MockLogger{}, nil)
+
+	config := &schemas.MCPClientConfig{
+		ID:                "per-call-checker-client",
+		Name:              "per_call_checker_client",
+		ConnectionType:    schemas.MCPConnectionTypeHTTP,
+		ConnectionString:  schemas.NewSecretVar("https://example.invalid/mcp"),
+		AuthType:          schemas.MCPAuthTypePerUserHeaders,
+		PerUserHeaderKeys: []string{"X-User-Token"},
+		// Non-nil (even empty) DiscoveredTools: a nil map means admin
+		// verification never ran, parking in pending_verification BEFORE
+		// ever reaching the RequiresPerCallConnection branch this test
+		// targets — see TestAddClient_PerUserHeaders_DiscoveredToolsStateSelection.
+		DiscoveredTools: map[string]schemas.ChatTool{},
+	}
+
+	require.NoError(t, m.AddClient(context.Background(), config))
+
+	m.checkerManager.mu.RLock()
+	_, hasChecker := m.checkerManager.checkers[config.ID]
+	m.checkerManager.mu.RUnlock()
+	assert.True(t, hasChecker, "AddClient's per-call branch must start a connection checker, or this client's tools are never (re)discovered")
+}
+
+// TestAddClient_PerCallConnection_SharedType_DiscoversToolsSynchronously pins
+// the second half of the same bug report: a connection checker alone is not
+// enough, because ClientConnectionChecker.Start uses the SLOW healthyInterval
+// (not the fast Unstable one) for a client that already starts Healthy — so
+// without a synchronous first discovery pass, a shared oauth/headers/none
+// per-call client would sit at Healthy/0-tools for up to healthyInterval
+// before its first real check ever ran. AddClient must discover tools
+// synchronously, mirroring what per_user_headers/token_exchange already get
+// from their own admin-verify-at-setup-time step.
+func TestAddClient_PerCallConnection_SharedType_DiscoversToolsSynchronously(t *testing.T) {
+	ts, _ := buildAdminDiscoveryHTTPServer(t)
+
+	m := NewMCPManager(context.Background(), schemas.MCPConfig{}, nil, &MockLogger{}, nil)
+
+	config := &schemas.MCPClientConfig{
+		ID:               "shared-sync-discovery-client",
+		Name:             "shared_sync_discovery_client",
+		ConnectionType:   schemas.MCPConnectionTypeHTTP,
+		ConnectionString: schemas.NewSecretVar(ts.URL),
+		AuthType:         schemas.MCPAuthTypeHeaders,
+		Headers:          map[string]schemas.SecretVar{"Authorization": *schemas.NewSecretVar("Bearer shared-add-token")},
+		// NeedsSessionStickiness left nil: the default per-call value.
+		// DiscoveredTools left nil: nothing to restore, forcing the new
+		// synchronous-discovery path this test targets.
+	}
+
+	require.NoError(t, m.AddClient(context.Background(), config))
+
+	state, ok := snapshotAddClientState(m, config.ID)
+	require.True(t, ok)
+	assert.Equal(t, schemas.MCPConnectionStateHealthy, state.State)
+	assert.Contains(t, state.ToolMap, "shared_sync_discovery_client-echo", "tools must be discovered synchronously during AddClient, not deferred entirely to the periodic checker")
+}

core/mcp/admin_tool_discovery_test.go

@@ -234,39 +234,119 @@ func TestPerformAdminToolDiscovery_PerUserHeaders_ConvertsHeadersAndDispatches(t
 	}
 }
 
-// TestPerformAdminToolDiscovery_UnsupportedAuthType_ReturnsError confirms
-// every non-per-user auth type falls into the default branch and errors
-// instead of attempting discovery — shared auth types have no distinct
-// "admin" credential to sync with.
+// TestPerformAdminToolDiscovery_UnsupportedAuthType_ReturnsError confirms a
+// genuinely unknown auth type falls into the default branch and errors
+// instead of attempting discovery.
 func TestPerformAdminToolDiscovery_UnsupportedAuthType_ReturnsError(t *testing.T) {
-	authTypes := []schemas.MCPAuthType{
-		schemas.MCPAuthTypeNone,
-		schemas.MCPAuthTypeHeaders,
-		schemas.MCPAuthTypeOauth,
-		schemas.MCPAuthType("something_unknown"),
+	cred := &fakeAdminCredStore{headers: http.Header{"Authorization": []string{"Bearer x"}}}
+	m := &MCPManager{credStore: cred, logger: &MockLogger{}}
+
+	config := &schemas.MCPClientConfig{
+		ID:       "client-3",
+		Name:     "shared-client",
+		AuthType: schemas.MCPAuthType("something_unknown"),
 	}
 
-	for _, authType := range authTypes {
+	tools, mapping, err := m.performAdminToolDiscovery(context.Background(), config)
+	require.Error(t, err)
+	require.Nil(t, tools)
+	require.Nil(t, mapping)
+	require.True(t, strings.Contains(err.Error(), "admin tool discovery not supported for auth_type"))
+}
+
+// TestPerformAdminToolDiscovery_SharedOAuth_PerCall_ExtractsBearerTokenAndDispatches
+// pins the fix for the shared-OAuth-per-call tool-discovery bug: auth_type
+// "oauth" now routes through the same bearer-based dispatch as
+// per_user_oauth/token_exchange, since sharedOAuthResolver.AdminConnectionHeaders
+// delegates to ConnectionHeaders for a per-call client (see credstore's
+// shared_oauth.go) instead of erroring.
+func TestPerformAdminToolDiscovery_SharedOAuth_PerCall_ExtractsBearerTokenAndDispatches(t *testing.T) {
+	ts, rec := buildAdminDiscoveryHTTPServer(t)
+
+	cred := &fakeAdminCredStore{headers: http.Header{"Authorization": []string{"Bearer shared-secret-token"}}}
+	m := &MCPManager{credStore: cred, logger: &MockLogger{}}
+
+	config := &schemas.MCPClientConfig{
+		ID:               "client-5",
+		Name:             "shared-oauth-client",
+		AuthType:         schemas.MCPAuthTypeOauth,
+		ConnectionType:   schemas.MCPConnectionTypeHTTP,
+		ConnectionString: schemas.NewSecretVar(ts.URL),
+		// NeedsSessionStickiness left nil: the default per-call value.
+	}
+
+	tools, mapping, err := m.performAdminToolDiscovery(context.Background(), config)
+	require.NoError(t, err)
+	require.Contains(t, tools, "shared-oauth-client-echo")
+	require.Equal(t, "echo", mapping["echo"])
+	require.Equal(t, 1, cred.callCount())
+
+	authVals := rec.headerValues("Authorization")
+	require.NotEmpty(t, authVals)
+	for _, v := range authVals {
+		require.Equal(t, "Bearer shared-secret-token", v)
+	}
+}
+
+// TestPerformAdminToolDiscovery_SharedHeadersAndNone_PerCall_ConvertsHeadersAndDispatches
+// is the shared headers/none counterpart, table-driven since both route
+// through the same headers-map dispatch branch as per_user_headers.
+func TestPerformAdminToolDiscovery_SharedHeadersAndNone_PerCall_ConvertsHeadersAndDispatches(t *testing.T) {
+	for _, authType := range []schemas.MCPAuthType{schemas.MCPAuthTypeHeaders, schemas.MCPAuthTypeNone} {
 		t.Run(string(authType), func(t *testing.T) {
-			cred := &fakeAdminCredStore{headers: http.Header{"Authorization": []string{"Bearer x"}}}
+			ts, rec := buildAdminDiscoveryHTTPServer(t)
+
+			cred := &fakeAdminCredStore{headers: http.Header{"X-Api-Key": []string{"shared-key-1"}}}
 			m := &MCPManager{credStore: cred, logger: &MockLogger{}}
 
 			config := &schemas.MCPClientConfig{
-				ID:       "client-3",
-				Name:     "shared-client",
-				AuthType: authType,
+				ID:               "client-6",
+				Name:             "shared-headers-client",
+				AuthType:         authType,
+				ConnectionType:   schemas.MCPConnectionTypeHTTP,
+				ConnectionString: schemas.NewSecretVar(ts.URL),
 			}
 
 			tools, mapping, err := m.performAdminToolDiscovery(context.Background(), config)
-			require.Error(t, err)
-			require.Nil(t, tools)
-			require.Nil(t, mapping)
-			require.True(t, strings.Contains(err.Error(), "admin tool discovery not supported for auth_type"))
-			require.Zero(t, cred.callCount(), "unsupported auth types must not resolve admin credentials")
+			require.NoError(t, err)
+			require.Contains(t, tools, "shared-headers-client-echo")
+			require.Equal(t, "echo", mapping["echo"])
+			require.Equal(t, 1, cred.callCount())
+
+			apiKeyVals := rec.headerValues("X-Api-Key")
+			require.NotEmpty(t, apiKeyVals)
+			for _, v := range apiKeyVals {
+				require.Equal(t, "shared-key-1", v)
+			}
 		})
 	}
 }
 
+// TestPerformAdminToolDiscovery_SharedHeaders_PerCall_EmptyHeadersStillDispatches
+// pins the VerifyHeadersConnection guard relaxation: a shared headers/none
+// client with NO resolved headers at all (auth carried entirely by other
+// static config headers, or none) must still attempt discovery rather than
+// erroring on "user headers are required" — that guard is per_user_headers
+// specific.
+func TestPerformAdminToolDiscovery_SharedHeaders_PerCall_EmptyHeadersStillDispatches(t *testing.T) {
+	ts, _ := buildAdminDiscoveryHTTPServer(t)
+
+	cred := &fakeAdminCredStore{headers: http.Header{}} // resolves to nothing
+	m := &MCPManager{credStore: cred, logger: &MockLogger{}}
+
+	config := &schemas.MCPClientConfig{
+		ID:               "client-7",
+		Name:             "shared-none-client",
+		AuthType:         schemas.MCPAuthTypeNone,
+		ConnectionType:   schemas.MCPConnectionTypeHTTP,
+		ConnectionString: schemas.NewSecretVar(ts.URL),
+	}
+
+	tools, _, err := m.performAdminToolDiscovery(context.Background(), config)
+	require.NoError(t, err)
+	require.Contains(t, tools, "shared-none-client-echo")
+}
+
 // TestPerformAdminToolDiscovery_CredStoreError_Propagates confirms a
 // credStore.AdminConnectionHeaders failure is wrapped and returned as-is,
 // without attempting any connection.