Merge pull request #3 from nitzpo/e2e-run

Fix: E2E bugs from real-world MCP testing

nadlan_mcp/fastmcp_server.py

@@ -22,6 +22,28 @@ mcp = FastMCP("nadlan-mcp")
 # Initialize the Govmap client
 client = GovmapClient()
 
+def strip_bloat_fields(deals: List[Dict]) -> List[Dict]:
+    """
+    Remove bloat fields from deal objects to reduce token usage in MCP responses.
+
+    Removes:
+    - shape: Large MULTIPOLYGON coordinate data (~40-50% of tokens, not useful for LLM analysis)
+    - sourceorder: Internal ordering field
+    - source_polygon_id: Internal reference field
+
+    Args:
+        deals: List of deal dictionaries
+
+    Returns:
+        List of deals with bloat fields removed
+    """
+    bloat_fields = {'shape', 'sourceorder', 'source_polygon_id'}
+
+    return [
+        {k: v for k, v in deal.items() if k not in bloat_fields}
+        for deal in deals
+    ]
+
 @mcp.tool()
 def autocomplete_address(search_text: str) -> str:
     """Search and autocomplete Israeli addresses.
@@ -77,7 +99,7 @@ def get_deals_by_radius(latitude: float, longitude: float, radius_meters: int =
             "total_deals": len(deals),
             "search_radius_meters": radius_meters,
             "center_coordinates": {"latitude": latitude, "longitude": longitude},
-            "deals": deals
+            "deals": strip_bloat_fields(deals)
         }, ensure_ascii=False, indent=2)
         
     except Exception as e:
@@ -134,15 +156,15 @@ def get_street_deals(polygon_id: str, limit: int = 100, deal_type: int = 2) -> s
             "deal_type": deal_type,
             "deal_type_description": deal_type_desc,
             "market_statistics": stats,
-            "deals": deals
+            "deals": strip_bloat_fields(deals)
         }, ensure_ascii=False, indent=2)
-        
+
     except Exception as e:
         logger.error(f"Error in get_street_deals: {e}")
         return f"Error fetching street deals: {str(e)}"
 
 @mcp.tool()
-def find_recent_deals_for_address(address: str, years_back: int = 2, radius_meters: int = 30, max_deals: int = 50, deal_type: int = 2) -> str:
+def find_recent_deals_for_address(address: str, years_back: int = 2, radius_meters: int = 30, max_deals: int = 100, deal_type: int = 2) -> str:
     """Find recent real estate deals for a specific address.
     
     Args:
@@ -150,7 +172,7 @@ def find_recent_deals_for_address(address: str, years_back: int = 2, radius_mete
         years_back: How many years back to search (default: 2)
         radius_meters: Search radius in meters from the address (default: 30)
                       Small radius since street deals cover the entire street anyway
-        max_deals: Maximum number of deals to return (default: 50, optimized for LLM token limits)
+        max_deals: Maximum number of deals to return (default: 100, provides good context for LLM analysis)
         deal_type: Deal type filter (1=first hand/new, 2=second hand/used, default: 2)
         
     Returns:
@@ -221,7 +243,7 @@ def find_recent_deals_for_address(address: str, years_back: int = 2, radius_mete
                 "deal_type_description": deal_type_desc
             },
             "market_statistics": stats,
-            "deals": deals
+            "deals": strip_bloat_fields(deals)
         }, ensure_ascii=False, indent=2)
         
     except Exception as e:
@@ -278,9 +300,9 @@ def get_neighborhood_deals(polygon_id: str, limit: int = 100, deal_type: int = 2
             "deal_type": deal_type,
             "deal_type_description": deal_type_desc,
             "market_statistics": stats,
-            "deals": deals
+            "deals": strip_bloat_fields(deals)
         }, ensure_ascii=False, indent=2)
-        
+
     except Exception as e:
         logger.error(f"Error in get_neighborhood_deals: {e}")
         return f"Error fetching neighborhood deals: {str(e)}"
@@ -544,13 +566,16 @@ def get_valuation_comparables(
     min_area: Optional[float] = None,
     max_area: Optional[float] = None,
     min_floor: Optional[int] = None,
-    max_floor: Optional[int] = None
+    max_floor: Optional[int] = None,
+    radius_meters: int = 100,
+    max_comparables: int = 50
 ) -> str:
     """Get comparable properties for valuation analysis.
-    
+
     This tool provides detailed comparable deals filtered by your criteria.
-    The LLM can then analyze these comparables and estimate property values.
+    Returns a generous number of comparables by default - the LLM analyzing
-    
+    the results can determine which are most similar based on the full details.
+
     Args:
         address: The address to find comparables for (in Hebrew or English)
         years_back: How many years back to search (default: 2)
@@ -563,13 +588,21 @@ def get_valuation_comparables(
         max_area: Maximum asset area (square meters)
         min_floor: Minimum floor number
         max_floor: Maximum floor number
-        
+        radius_meters: Search radius in meters (default: 100, larger than find_recent_deals to get more comparables)
+        max_comparables: Maximum number of deals to return (default: 50, optimized for MCP token limits)
+
     Returns:
-        JSON string containing filtered comparable deals with full details
+        JSON string containing filtered comparable deals with full details.
+        Returns many comparables so LLM can assess similarity and relevance.
     """
     try:
-        # Get all deals for the address
+        # Get all deals for the address with higher limits for valuation
-        deals = client.find_recent_deals_for_address(address, years_back)
+        deals = client.find_recent_deals_for_address(
+            address,
+            years_back,
+            radius=radius_meters,
+            max_deals=max_comparables
+        )
         
         if not deals:
             return json.dumps({
@@ -608,9 +641,9 @@ def get_valuation_comparables(
             },
             "total_comparables": len(filtered_deals),
             "statistics": stats,
-            "comparables": filtered_deals
+            "comparables": strip_bloat_fields(filtered_deals)
         }, ensure_ascii=False, indent=2)
-        
+
     except Exception as e:
         logger.error(f"Error in get_valuation_comparables: {e}")
         return f"Error getting valuation comparables: {str(e)}"

nadlan_mcp/govmap.py

@@ -510,7 +510,7 @@ class GovmapClient:
         address: str,
         years_back: int = 2,
         radius: int = 30,
-        max_deals: int = 50,
+        max_deals: int = 100,
         deal_type: int = 2,
     ) -> List[Dict[str, Any]]:
         """
@@ -524,7 +524,7 @@ class GovmapClient:
             years_back: How many years back to search (default: 2)
             radius: Search radius in meters for initial coordinate search (default: 30)
                    Small radius since street deals cover the entire street anyway
-            max_deals: Maximum number of deals to return (default: 50)
+            max_deals: Maximum number of deals to return (default: 100)
             deal_type: Deal type filter (1=first hand/new, 2=second hand/used, default: 2)
 
         Returns:
@@ -622,14 +622,18 @@ class GovmapClient:
 
                     # Process street deals and separate building deals
                     for deal in current_street_deals:
-                        deal_id = f"{deal.get('dealId', '')}{deal.get('address', '')}{deal.get('dealDate', '')}"
+                        # Create unique deal ID for deduplication
+                        deal_id = f"{deal.get('dealId', '')}{deal.get('dealDate', '')}"
                         if deal_id not in seen_deals:
                             seen_deals.add(deal_id)
                             deal["source_polygon_id"] = polygon_id
                             deal["deal_source"] = "street"
 
                             # Check if this is from the same building
-                            deal_address = deal.get("address", "").lower().strip()
+                            # Construct address from API fields (API doesn't have single "address" field)
+                            street = deal.get("streetNameHeb", "")
+                            house_num = str(deal.get("houseNum", ""))
+                            deal_address = f"{street} {house_num}".lower().strip()
                             if self._is_same_building(
                                 search_address_normalized, deal_address
                             ):
@@ -642,7 +646,8 @@ class GovmapClient:
 
                     # Add neighborhood deals with lowest priority
                     for deal in current_neighborhood_deals:
-                        deal_id = f"{deal.get('dealId', '')}{deal.get('address', '')}{deal.get('dealDate', '')}"
+                        # Create unique deal ID for deduplication
+                        deal_id = f"{deal.get('dealId', '')}{deal.get('dealDate', '')}"
                         if deal_id not in seen_deals:
                             seen_deals.add(deal_id)
                             deal["source_polygon_id"] = polygon_id
@@ -700,6 +705,24 @@ class GovmapClient:
             logger.error(f"Error in find_recent_deals_for_address: {e}")
             raise
 
+    def _calculate_distance(self, point1: Tuple[float, float], point2: Tuple[float, float]) -> float:
+        """
+        Calculate Euclidean distance between two points in ITM coordinates.
+
+        ITM (Israeli Transverse Mercator) uses meters as units, so Euclidean
+        distance provides accurate results for distances within Israel.
+
+        Args:
+            point1: (longitude, latitude) in ITM
+            point2: (longitude, latitude) in ITM
+
+        Returns:
+            Distance in meters
+        """
+        dx = point2[0] - point1[0]
+        dy = point2[1] - point1[1]
+        return (dx * dx + dy * dy) ** 0.5
+
     def _is_same_building(self, search_address: str, deal_address: str) -> bool:
         """
         Check if a deal is from the same building as the search address.
@@ -819,12 +842,24 @@ class GovmapClient:
                 deal_type = deal.get(
                     "propertyTypeDescription", deal.get("assetTypeHeb", "")
                 )
-                if property_type.lower() not in deal_type.lower():
+                # Skip deals with missing property type data when filter is active
+                if not deal_type:
+                    continue
+
+                # Normalize both strings for flexible matching
+                property_type_normalized = property_type.lower().strip()
+                deal_type_normalized = deal_type.lower().strip()
+
+                # Check if the filter term appears in the deal type
+                # This allows "דירה" to match "דירת גג", "דירה בבניין", etc.
+                if property_type_normalized not in deal_type_normalized:
                     continue
 
             # Room count filter
-            rooms = deal.get("assetRoomNum")
+            if min_rooms is not None or max_rooms is not None:
-            if rooms is not None:
+                rooms = deal.get("assetRoomNum")
+                if rooms is None:
+                    continue  # Skip deals with missing room data when filter is active
                 try:
                     rooms = float(rooms)
                     if min_rooms is not None and rooms < min_rooms:
@@ -832,11 +867,13 @@ class GovmapClient:
                     if max_rooms is not None and rooms > max_rooms:
                         continue
                 except (TypeError, ValueError):
-                    pass  # Skip deals with invalid room data
+                    continue  # Skip deals with invalid room data when filter is active
 
             # Price filter
-            price = deal.get("dealAmount")
+            if min_price is not None or max_price is not None:
-            if price is not None:
+                price = deal.get("dealAmount")
+                if price is None:
+                    continue  # Skip deals with missing price data when filter is active
                 try:
                     price = float(price)
                     if min_price is not None and price < min_price:
@@ -844,11 +881,13 @@ class GovmapClient:
                     if max_price is not None and price > max_price:
                         continue
                 except (TypeError, ValueError):
-                    pass  # Skip deals with invalid price data
+                    continue  # Skip deals with invalid price data when filter is active
 
             # Area filter
-            area = deal.get("assetArea")
+            if min_area is not None or max_area is not None:
-            if area is not None:
+                area = deal.get("assetArea")
+                if area is None:
+                    continue  # Skip deals with missing area data when filter is active
                 try:
                     area = float(area)
                     if min_area is not None and area < min_area:
@@ -856,7 +895,7 @@ class GovmapClient:
                     if max_area is not None and area > max_area:
                         continue
                 except (TypeError, ValueError):
-                    pass  # Skip deals with invalid area data
+                    continue  # Skip deals with invalid area data when filter is active
 
             # Floor filter
             floor_str = deal.get("floorNo", "")

tests/test_govmap_client.py

@@ -6,6 +6,7 @@ import pytest
 import requests
 from unittest.mock import Mock, patch
 from nadlan_mcp.govmap import GovmapClient
+from nadlan_mcp.config import GovmapConfig
 
 
 class TestGovmapClient:
@@ -22,7 +23,8 @@ class TestGovmapClient:
     def test_client_initialization_with_custom_url(self):
         """Test that GovmapClient can be initialized with custom URL."""
         custom_url = "https://custom-api.example.com/api/"
-        client = GovmapClient(custom_url)
+        custom_config = GovmapConfig(base_url=custom_url)
+        client = GovmapClient(custom_config)
         assert client.base_url == "https://custom-api.example.com/api"
     
     @patch('requests.Session')
@@ -226,26 +228,31 @@ class TestGovmapClient:
                 "dealId": "deal1",
                 "dealAmount": 1000000,
                 "dealDate": "2025-01-01T00:00:00.000Z",
-                "address": "Test Street 1"
+                "address": "Test Street 1",
+                "priority": 1
             }
         ]
-        
+
         # Mock neighborhood deals response
         mock_neighborhood.return_value = [
             {
                 "dealId": "deal2",
                 "dealAmount": 2000000,
                 "dealDate": "2025-01-15T00:00:00.000Z",
-                "address": "Test Street 2"
+                "address": "Test Street 2",
+                "priority": 2
             }
         ]
         
         client = GovmapClient()
         result = client.find_recent_deals_for_address("test address", years_back=1)
-        
+
         assert len(result) == 2
-        assert result[0]["dealDate"] == "2025-01-15T00:00:00.000Z"  # Should be sorted by date
+        # Should be sorted by priority first (street=1 before neighborhood=2), then by date
-        assert result[1]["dealDate"] == "2025-01-01T00:00:00.000Z"
+        assert result[0]["priority"] == 1  # Street deal comes first
+        assert result[0]["dealDate"] == "2025-01-01T00:00:00.000Z"
+        assert result[1]["priority"] == 2  # Neighborhood deal comes second
+        assert result[1]["dealDate"] == "2025-01-15T00:00:00.000Z"
     
     @patch('requests.Session')
     def test_http_error_handling(self, mock_session_class):
@@ -525,3 +532,121 @@ class TestMarketAnalysisFunctions:
         assert stats["count"] == 3
         assert stats["price_stats"]["mean"] > 0
         assert stats["area_stats"]["mean"] == pytest.approx(80.0)
+
+    def test_is_same_building_comparisons(self):
+        """Test `_is_same_building` correctly compares address strings."""
+        client = GovmapClient()
+
+        # Test that _is_same_building works with addresses constructed from API fields
+        search_address = "חנקין 62"
+
+        # Deal from same building (should match)
+        deal_address_same = "חנקין 62"
+        assert client._is_same_building(search_address, deal_address_same) is True
+
+        # Deal from different building on same street (should not match)
+        deal_address_different = "חנקין 50"
+        assert client._is_same_building(search_address, deal_address_different) is False
+
+        # Deal from different street (should not match)
+        deal_address_other_street = "בילינסון 6"
+        assert client._is_same_building(search_address, deal_address_other_street) is False
+
+    def test_filter_excludes_missing_property_type(self):
+        """Test that deals with missing property type are excluded when filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "propertyTypeDescription": "דירה"},
+            {"dealId": "2", "propertyTypeDescription": None},
+            {"dealId": "3", "propertyTypeDescription": "בית"},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, property_type="דירה")
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_missing_area(self):
+        """Test that deals with missing area are excluded when area filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "assetArea": 65},
+            {"dealId": "2", "assetArea": None},
+            {"dealId": "3", "assetArea": 50},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, min_area=60, max_area=70)
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_missing_rooms(self):
+        """Test that deals with missing room count are excluded when room filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "assetRoomNum": 3},
+            {"dealId": "2", "assetRoomNum": None},
+            {"dealId": "3", "assetRoomNum": 2},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, min_rooms=2.5, max_rooms=4)
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_missing_price(self):
+        """Test that deals with missing price are excluded when price filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "dealAmount": 2000000},
+            {"dealId": "2", "dealAmount": None},
+            {"dealId": "3", "dealAmount": 1500000},
+            {"dealId": "4"},  # Missing key entirely
+        ]
+
+        filtered = client.filter_deals_by_criteria(deals, min_price=1800000, max_price=2200000)
+
+        assert len(filtered) == 1
+        assert filtered[0]["dealId"] == "1"
+
+    def test_filter_excludes_invalid_numeric_data(self):
+        """Test that deals with invalid numeric data are excluded when filter is active."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "assetArea": 65, "assetRoomNum": 3, "dealAmount": 2000000},
+            {"dealId": "2", "assetArea": "invalid", "assetRoomNum": 3, "dealAmount": 2000000},
+            {"dealId": "3", "assetArea": 65, "assetRoomNum": "bad", "dealAmount": 2000000},
+            {"dealId": "4", "assetArea": 65, "assetRoomNum": 3, "dealAmount": "wrong"},
+        ]
+
+        # Area filter should exclude deal 2
+        filtered_area = client.filter_deals_by_criteria(deals, min_area=60, max_area=70)
+        assert len(filtered_area) == 3
+        assert all(d["dealId"] in ["1", "3", "4"] for d in filtered_area)
+
+        # Room filter should exclude deal 3
+        filtered_rooms = client.filter_deals_by_criteria(deals, min_rooms=2, max_rooms=4)
+        assert len(filtered_rooms) == 3
+        assert all(d["dealId"] in ["1", "2", "4"] for d in filtered_rooms)
+
+        # Price filter should exclude deal 4
+        filtered_price = client.filter_deals_by_criteria(deals, min_price=1500000, max_price=2500000)
+        assert len(filtered_price) == 3
+        assert all(d["dealId"] in ["1", "2", "3"] for d in filtered_price)
+
+    def test_filter_allows_missing_data_when_no_filter(self):
+        """Test that deals with missing data pass through when no filter is active for that field."""
+        client = GovmapClient()
+        deals = [
+            {"dealId": "1", "propertyTypeDescription": "דירה", "assetArea": 65},
+            {"dealId": "2", "propertyTypeDescription": "דירה", "assetArea": None},
+            {"dealId": "3", "propertyTypeDescription": "דירה"},  # Missing assetArea entirely
+        ]
+
+        # Filter by property type only - missing area should pass through
+        filtered = client.filter_deals_by_criteria(deals, property_type="דירה")
+
+        assert len(filtered) == 3  # All should pass since we're not filtering by area