From e791d3c1eda5bd6db03039dc7c405fbb4e24c9d9 Mon Sep 17 00:00:00 2001 From: Nitzan P <9297302+nitzpo@users.noreply.github.com> Date: Sun, 23 Nov 2025 09:49:52 +0200 Subject: [PATCH] Normalize MCP response structures across all tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement changes from MCP_NORMALIZATION_FIX.md to provide consistent response structure across all MCP tools. This fixes the bot integration issue where different tools returned data in different structures. Changes: - get_valuation_comparables: - Rename "comparables" → "deals" - Move "total_comparables" → "market_statistics.deal_breakdown.total_deals" - Add "search_parameters" section with "filters_applied" - Move "statistics" → "market_statistics" - find_recent_deals_for_address: - Rename "price_stats.average_price" → "price_statistics.mean" - Rename "price_stats.median_price" → "price_statistics.median" - Rename "area_stats" → "area_statistics" - Rename "price_per_sqm_stats" → "price_per_sqm_statistics" - get_deal_statistics: - Add "search_parameters" section - Move "statistics" → "market_statistics" - Add "market_statistics.deal_breakdown.total_deals" - analyze_market_trends: - Add "market_statistics.deal_breakdown.total_deals" - Keep existing tool-specific fields (yearly_trends, etc.) - get_market_activity_metrics: - Add "market_statistics.deal_breakdown.total_deals" - Keep existing tool-specific metrics All tools now follow standard structure: { "search_parameters" or "analysis_parameters": {...}, "market_statistics": { "deal_breakdown": {"total_deals": N}, "price_statistics": {"mean": ..., "median": ...}, "area_statistics": {...}, "price_per_sqm_statistics": {...} }, "deals": [...] } Updated tests to match new normalized structure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- MCP_NORMALIZATION_FIX.md | 327 ++++++++++++++++++++++ nadlan_mcp/fastmcp_server.py | 163 +++++++---- tests/e2e/test_mcp_tools_comprehensive.py | 39 +-- tests/test_fastmcp_tools.py | 23 +- 4 files changed, 476 insertions(+), 76 deletions(-) create mode 100644 MCP_NORMALIZATION_FIX.md diff --git a/MCP_NORMALIZATION_FIX.md b/MCP_NORMALIZATION_FIX.md new file mode 100644 index 0000000..0002865 --- /dev/null +++ b/MCP_NORMALIZATION_FIX.md @@ -0,0 +1,327 @@ +# Instructions for nadlan-mcp Project: Normalize Response Structure + +## Problem + +Different MCP tools return data in **inconsistent structures**, causing the bot to fail when interpreting results. + +### Current Inconsistent Structures + +**`get_valuation_comparables`** returns: +```json +{ + "total_comparables": 3, + "statistics": { + "total_deals": 3, + "price_statistics": { "mean": ..., "median": ... } + }, + "comparables": [...] // ← Array of deals +} +``` + +**`find_recent_deals_for_address`** returns: +```json +{ + "market_statistics": { + "deal_breakdown": { + "total_deals": 4 + }, + "price_stats": { "average_price": ..., "median_price": ... } + }, + "deals": [...] // ← Different field name! +} +``` + +### The Impact + +The bot code looks for: +- Deal count in: `market_statistics.deal_breakdown.total_deals` +- Deal array in: `deals` + +When `get_valuation_comparables` is called: +- Bot looks for `deals` array → finds nothing (field is called `comparables`) +- Bot looks for `market_statistics.deal_breakdown.total_deals` → finds nothing (field is `total_comparables` or `statistics.total_deals`) +- Bot thinks there are **0 deals** even when MCP returned **20 deals** +- User gets: "לא מצאתי עסקאות ספציפיות" (I didn't find specific deals) + +--- + +## Solution: Normalize All MCP Tool Responses + +All MCP tools should return data in a **consistent structure**. I recommend standardizing on this format: + +### Recommended Standard Structure + +```json +{ + "search_parameters": { + // Tool-specific search parameters that were used + "address": "...", + "years_back": 2, + // ... other params + }, + "market_statistics": { + "deal_breakdown": { + "total_deals": N, // ← Always here + "same_building_deals": 0, // Optional: only if relevant + "street_deals": 0, // Optional: only if relevant + "neighborhood_deals": 0 // Optional: only if relevant + }, + "price_statistics": { // ← Standardize field name + "mean": 1750000.0, // Use "mean" not "average_price" + "median": 1800000.0, // Use "median" not "median_price" + "min": 1443000.0, + "max": 2100000.0, + "p25": 1500000.0, + "p75": 2000000.0, + "std_dev": 250000.0, + "total": 7000000.0 + }, + "area_statistics": { // ← Standardize field name + "mean": 68.8, // Use "mean" not "average_area" + "median": 76.0, + "min": 42.0, + "max": 106.0, + "p25": 50.0, + "p75": 90.0 + }, + "price_per_sqm_statistics": { // ← Standardize field name + "mean": 27892.0, // Use "mean" not "average_price_per_sqm" + "median": 34357.0, + "min": 19811.0, + "max": 35294.0, + "p25": 25000.0, + "p75": 32000.0 + }, + "property_type_distribution": { // Optional + "דירה": 20, + "בית": 5 + }, + "date_range": { // Optional + "earliest": "2024-01-31", + "latest": "2025-11-05" + } + }, + "deals": [ // ← Always "deals", never "comparables" + { + "objectid": 1975198, + "deal_amount": 1800000.0, + "deal_date": "2025-08-21", + "asset_area": 51.0, + "settlement_name_heb": "חולון", + "property_type_description": "דירה", + "neighborhood": "קרית עבודה", + "rooms": 3.0, + "streetNameHeb": "חנקין", + "streetNameEng": "Hankin", + "houseNum": 62, + "floorNo": "שניה", + "price_per_sqm": 35294.12, + "deal_source": "street", // Optional: same_building, street, neighborhood + "priority": 1, // Optional: relevance ranking + // ... other fields + } + ] +} +``` + +--- + +## Specific Changes Needed + +### 1. `get_valuation_comparables` Tool + +**Current response**: +```json +{ + "total_comparables": 3, + "statistics": { "total_deals": 3, "price_statistics": {...} }, + "comparables": [...] +} +``` + +**Should be changed to**: +```json +{ + "search_parameters": { + "address": "חנקין 62 חולון", + "years_back": 2, + "filters_applied": { + "property_type": null, + "rooms": "2.5-3.5", + "price": null, + "area": null, + "floor": null + }, + "radius_meters": 100, + "max_comparables": 50 + }, + "market_statistics": { + "deal_breakdown": { + "total_deals": 3 // ← Move from "total_comparables" + }, + "price_statistics": { // ← Keep, matches standard + "mean": 1743333.33, + "median": 1750000.0, + "min": 1680000.0, + "max": 1800000.0, + "p25": 1680000.0, + "p75": 1800000.0, + "std_dev": 60277.14 + }, + "area_statistics": {...}, // ← Keep + "price_per_sqm_statistics": {...} // ← Keep + }, + "deals": [...] // ← Rename from "comparables" +} +``` + +**Changes**: +1. Remove `total_comparables` field +2. Add `market_statistics.deal_breakdown.total_deals` instead +3. Rename `comparables` → `deals` +4. Move `statistics` → `market_statistics` +5. Add `search_parameters` section with `filters_applied` + +### 2. `find_recent_deals_for_address` Tool + +**Current response**: ✅ Already follows the standard! + +Keep as-is, except: +- Rename `price_stats.average_price` → `price_statistics.mean` +- Rename `price_stats.median_price` → `price_statistics.median` +- Rename `area_stats` → `area_statistics` +- Rename `price_per_sqm_stats` → `price_per_sqm_statistics` + +### 3. `analyze_market_trends` Tool + +**Should return**: +```json +{ + "analysis_parameters": { + "address": "...", + "years_back": 3 + }, + "market_statistics": { // ← Add this + "deal_breakdown": { + "total_deals": 50 // ← Move from market_summary + }, + "price_statistics": {...}, // ← From trend analysis + "area_statistics": {...} + }, + "yearly_trends": {...}, // Keep tool-specific data + "trend_analysis": {...}, // Keep tool-specific data + "deals": [...] // Optional: sample deals +} +``` + +### 4. `get_deal_statistics` Tool + +**Should return**: +```json +{ + "search_parameters": {...}, + "market_statistics": { // ← Rename from "statistics" + "deal_breakdown": { + "total_deals": 25 + }, + "price_statistics": {...}, + "area_statistics": {...}, + "price_per_sqm_statistics": {...} + }, + "deals": [] // Can be empty for statistics-only queries +} +``` + +### 5. `get_market_activity_metrics` Tool + +**Should return**: +```json +{ + "analysis_parameters": {...}, + "market_statistics": { + "deal_breakdown": { + "total_deals": 20 // ← Move from total_deals_analyzed + } + }, + "market_activity": { // Keep tool-specific metrics + "activity_score": 75.0, + "liquidity_score": 80.0, + ... + }, + "investment_potential": {...}, // Keep tool-specific metrics + "deals": [] // Optional +} +``` + +--- + +## Benefits of Normalization + +1. **Bot compatibility**: Bot can use the same code path for all tools +2. **Consistency**: Developers always know where to find deal count and deal array +3. **Maintainability**: Adding new tools is easier with a standard structure +4. **Testing**: Can write generic tests that work for all tools +5. **Documentation**: Single structure to document instead of 5+ different formats + +--- + +## Implementation Checklist + +For each MCP tool that returns deals: + +- [ ] Add `market_statistics.deal_breakdown.total_deals` field +- [ ] Ensure deals array is named `deals` (not `comparables`, etc.) +- [ ] Standardize statistics field names: + - [ ] `price_statistics` with `mean`, `median`, `min`, `max`, `p25`, `p75` + - [ ] `area_statistics` with same structure + - [ ] `price_per_sqm_statistics` with same structure +- [ ] Add `search_parameters` or `analysis_parameters` section +- [ ] Keep tool-specific fields (like `yearly_trends`, `market_activity`, etc.) but always include the standard structure +- [ ] Test with bot to verify it correctly interprets results + +--- + +## Migration Strategy + +**Option 1: Breaking change (recommended)** +- Update all tools at once to use new structure +- Update bot to expect new structure +- Deploy both together + +**Option 2: Backward compatible** +- Return BOTH old and new fields temporarily: + ```json + { + "total_comparables": 3, // Old (deprecated) + "comparables": [...], // Old (deprecated) + "market_statistics": { // New (standard) + "deal_breakdown": { + "total_deals": 3 + } + }, + "deals": [...] // New (standard) + } + ``` +- Update bot to prefer new fields, fall back to old +- Remove old fields after bot is updated + +I recommend **Option 1** since this is an internal MCP server with a single client (the bot). + +--- + +## Testing After Changes + +Once normalized, test with the bot: + +``` +User: "כמה עולה דירת 3 חדרים בחנקין 62 חולון?" +Expected: Bot should return actual deals and prices, NOT "לא מצאתי עסקאות" +``` + +Verify in bot logs: +``` +Total deals found: 20 // Should show actual count +Sample deals (most recent 10): + 1. חנקין 62 חולון - ₪1,800,000 - 3 rooms - 51 sqm - 2025-08-21 + ... +``` diff --git a/nadlan_mcp/fastmcp_server.py b/nadlan_mcp/fastmcp_server.py index c3de75f..22e1b7c 100644 --- a/nadlan_mcp/fastmcp_server.py +++ b/nadlan_mcp/fastmcp_server.py @@ -258,33 +258,30 @@ def find_recent_deals_for_address( } } + # Standardize field names to match other tools if prices: - stats["price_stats"] = { - "average_price": round(sum(prices) / len(prices), 0), - "min_price": min(prices), - "max_price": max(prices), - "median_price": sorted(prices)[len(prices) // 2] if prices else 0, - "total_volume": sum(prices), + stats["price_statistics"] = { + "mean": round(sum(prices) / len(prices), 0), + "min": min(prices), + "max": max(prices), + "median": sorted(prices)[len(prices) // 2] if prices else 0, + "total": sum(prices), } if areas: - stats["area_stats"] = { - "average_area": round(sum(areas) / len(areas), 1), - "min_area": min(areas), - "max_area": max(areas), - "median_area": sorted(areas)[len(areas) // 2] if areas else 0, + stats["area_statistics"] = { + "mean": round(sum(areas) / len(areas), 1), + "min": min(areas), + "max": max(areas), + "median": sorted(areas)[len(areas) // 2] if areas else 0, } if price_per_sqm_values: - stats["price_per_sqm_stats"] = { - "average_price_per_sqm": round( - sum(price_per_sqm_values) / len(price_per_sqm_values), 0 - ), - "min_price_per_sqm": round(min(price_per_sqm_values), 0), - "max_price_per_sqm": round(max(price_per_sqm_values), 0), - "median_price_per_sqm": round( - sorted(price_per_sqm_values)[len(price_per_sqm_values) // 2], 0 - ) + stats["price_per_sqm_statistics"] = { + "mean": round(sum(price_per_sqm_values) / len(price_per_sqm_values), 0), + "min": round(min(price_per_sqm_values), 0), + "max": round(max(price_per_sqm_values), 0), + "median": round(sorted(price_per_sqm_values)[len(price_per_sqm_values) // 2], 0) if price_per_sqm_values else 0, } @@ -531,6 +528,7 @@ def analyze_market_trends( deal_type_desc = "first hand (new)" if deal_type == 1 else "second hand (used)" # Return summarized analysis (NO raw deals to save tokens) + # Normalize structure with standard market_statistics while keeping tool-specific analysis return json.dumps( { "analysis_parameters": { @@ -541,8 +539,12 @@ def analyze_market_trends( "deal_type": deal_type, "deal_type_description": deal_type_desc, }, + "market_statistics": { + "deal_breakdown": { + "total_deals": len(deals), + }, + }, "market_summary": { - "total_deals": len(deals), "years_with_data": len(yearly_trends), "unique_property_types": len(property_type_analysis), "unique_neighborhoods": len(neighborhood_analysis), @@ -564,6 +566,7 @@ def analyze_market_trends( else None, "deal_source_summary": f"Building: {len([d for d in deals if getattr(d, 'deal_source', None) == 'same_building'])}, Street: {len([d for d in deals if getattr(d, 'deal_source', None) == 'street'])}, Neighborhood: {len([d for d in deals if getattr(d, 'deal_source', None) == 'neighborhood'])}", }, + "deals": [], # Trend analysis doesn't return raw deals to save tokens }, ensure_ascii=False, indent=2, @@ -756,9 +759,18 @@ def get_valuation_comparables( if not deals: return json.dumps( { - "address": address, - "years_back": years_back, - "comparables": [], + "search_parameters": { + "address": address, + "years_back": years_back, + "radius_meters": radius_meters, + "max_comparables": max_comparables, + }, + "market_statistics": { + "deal_breakdown": { + "total_deals": 0, + }, + }, + "deals": [], "message": "No deals found for this address", }, ensure_ascii=False, @@ -782,20 +794,33 @@ def get_valuation_comparables( # Calculate statistics on filtered comparables stats = client.calculate_deal_statistics(filtered_deals) + # Normalize response structure to match other tools return json.dumps( { - "address": address, - "years_back": years_back, - "filters_applied": { - "property_type": property_type, - "rooms": f"{min_rooms}-{max_rooms}" if min_rooms or max_rooms else None, - "price": f"{min_price}-{max_price}" if min_price or max_price else None, - "area": f"{min_area}-{max_area}" if min_area or max_area else None, - "floor": f"{min_floor}-{max_floor}" if min_floor or max_floor else None, + "search_parameters": { + "address": address, + "years_back": years_back, + "radius_meters": radius_meters, + "max_comparables": max_comparables, + "filters_applied": { + "property_type": property_type, + "rooms": f"{min_rooms}-{max_rooms}" if min_rooms or max_rooms else None, + "price": f"{min_price}-{max_price}" if min_price or max_price else None, + "area": f"{min_area}-{max_area}" if min_area or max_area else None, + "floor": f"{min_floor}-{max_floor}" if min_floor or max_floor else None, + }, }, - "total_comparables": len(filtered_deals), - "statistics": stats.model_dump(exclude_none=True), # Serialize DealStatistics model - "comparables": strip_bloat_fields(filtered_deals), + "market_statistics": { + "deal_breakdown": { + "total_deals": len(filtered_deals), + }, + "price_statistics": stats.price_statistics, + "area_statistics": stats.area_statistics, + "price_per_sqm_statistics": stats.price_per_sqm_statistics, + "property_type_distribution": stats.property_type_distribution, + "date_range": stats.date_range, + }, + "deals": strip_bloat_fields(filtered_deals), }, ensure_ascii=False, indent=2, @@ -836,9 +861,15 @@ def get_deal_statistics( if not deals: return json.dumps( { - "address": address, - "years_back": years_back, - "statistics": {"count": 0, "message": "No deals found for this address"}, + "search_parameters": { + "address": address, + "years_back": years_back, + }, + "market_statistics": { + "deal_breakdown": {"total_deals": 0}, + "message": "No deals found for this address", + }, + "deals": [], }, ensure_ascii=False, indent=2, @@ -853,15 +884,28 @@ def get_deal_statistics( # Calculate statistics stats = client.calculate_deal_statistics(deals) + # Normalize response structure to match other tools return json.dumps( { - "address": address, - "years_back": years_back, - "filters_applied": { - "property_type": property_type, - "rooms": f"{min_rooms}-{max_rooms}" if min_rooms or max_rooms else None, + "search_parameters": { + "address": address, + "years_back": years_back, + "filters_applied": { + "property_type": property_type, + "rooms": f"{min_rooms}-{max_rooms}" if min_rooms or max_rooms else None, + }, }, - "statistics": stats.model_dump(exclude_none=True), # Serialize DealStatistics model + "market_statistics": { + "deal_breakdown": { + "total_deals": stats.total_deals, + }, + "price_statistics": stats.price_statistics, + "area_statistics": stats.area_statistics, + "price_per_sqm_statistics": stats.price_per_sqm_statistics, + "property_type_distribution": stats.property_type_distribution, + "date_range": stats.date_range, + }, + "deals": [], # Statistics-only query, no full deals returned }, ensure_ascii=False, indent=2, @@ -924,10 +968,18 @@ def get_market_activity_metrics(address: str, years_back: int = 2, radius_meters if not deals: return json.dumps( { - "address": address, + "analysis_parameters": { + "address": address, + "years_back": years_back, + "radius_meters": radius_meters, + }, + "market_statistics": { + "deal_breakdown": { + "total_deals": 0, + }, + }, + "deals": [], "error": "No deals found for analysis", - "years_back": years_back, - "radius_meters": radius_meters, }, ensure_ascii=False, indent=2, @@ -938,13 +990,19 @@ def get_market_activity_metrics(address: str, years_back: int = 2, radius_meters liquidity_metrics = _safe_calculate_metric(client.get_market_liquidity, deals) investment_metrics = _safe_calculate_metric(client.analyze_investment_potential, deals) - # Combine all metrics + # Combine all metrics with normalized structure return json.dumps( { - "address": address, - "years_back": years_back, - "radius_meters": radius_meters, - "total_deals_analyzed": len(deals), + "analysis_parameters": { + "address": address, + "years_back": years_back, + "radius_meters": radius_meters, + }, + "market_statistics": { + "deal_breakdown": { + "total_deals": len(deals), + }, + }, "market_activity": activity_metrics, "market_liquidity": liquidity_metrics, "investment_potential": investment_metrics, @@ -957,6 +1015,7 @@ def get_market_activity_metrics(address: str, years_back: int = 2, radius_meters "price_trend": investment_metrics.get("price_trend"), "market_stability": investment_metrics.get("market_stability"), }, + "deals": [], # Activity metrics don't return raw deals }, ensure_ascii=False, indent=2, diff --git a/tests/e2e/test_mcp_tools_comprehensive.py b/tests/e2e/test_mcp_tools_comprehensive.py index c447bfa..4e9ebea 100644 --- a/tests/e2e/test_mcp_tools_comprehensive.py +++ b/tests/e2e/test_mcp_tools_comprehensive.py @@ -75,11 +75,12 @@ class TestMCPToolsE2E: result = analyze_market_trends(self.TEST_ADDRESS_1, years_back=3, radius_meters=100) data = json.loads(result) - # Check response structure - assert "market_summary" in data - assert "total_deals" in data["market_summary"] - assert isinstance(data["market_summary"]["total_deals"], int) - assert data["market_summary"]["total_deals"] >= 0 + # Check response structure (normalized in MCP_NORMALIZATION_FIX) + assert "market_statistics" in data + assert "deal_breakdown" in data["market_statistics"] + assert "total_deals" in data["market_statistics"]["deal_breakdown"] + assert isinstance(data["market_statistics"]["deal_breakdown"]["total_deals"], int) + assert data["market_statistics"]["deal_breakdown"]["total_deals"] >= 0 def test_get_valuation_comparables(self): """Test getting valuation comparables.""" @@ -88,13 +89,17 @@ class TestMCPToolsE2E: ) data = json.loads(result) - assert "total_comparables" in data - assert isinstance(data["total_comparables"], int) - assert data["total_comparables"] >= 0 + # Normalized structure: total_comparables -> market_statistics.deal_breakdown.total_deals + assert "market_statistics" in data + assert "deal_breakdown" in data["market_statistics"] + assert "total_deals" in data["market_statistics"]["deal_breakdown"] + assert isinstance(data["market_statistics"]["deal_breakdown"]["total_deals"], int) + assert data["market_statistics"]["deal_breakdown"]["total_deals"] >= 0 - if data["total_comparables"] > 0: - assert "comparables" in data - comp = data["comparables"][0] + # Normalized structure: comparables -> deals + if data["market_statistics"]["deal_breakdown"]["total_deals"] > 0: + assert "deals" in data + comp = data["deals"][0] assert "deal_amount" in comp # rooms field is optional and excluded when None # Just verify the comparable has basic required fields @@ -105,11 +110,13 @@ class TestMCPToolsE2E: result = get_deal_statistics(self.TEST_ADDRESS_1, years_back=3) data = json.loads(result) - # Check response structure - assert "statistics" in data - if "sample_size" in data["statistics"]: - assert isinstance(data["statistics"]["sample_size"], int) - assert data["statistics"]["sample_size"] >= 0 + # Check response structure (normalized: statistics -> market_statistics) + assert "market_statistics" in data + # Check for total_deals in normalized location + if "deal_breakdown" in data["market_statistics"]: + assert "total_deals" in data["market_statistics"]["deal_breakdown"] + assert isinstance(data["market_statistics"]["deal_breakdown"]["total_deals"], int) + assert data["market_statistics"]["deal_breakdown"]["total_deals"] >= 0 def test_get_market_activity_metrics(self): """Test market activity metrics.""" diff --git a/tests/test_fastmcp_tools.py b/tests/test_fastmcp_tools.py index 6064b46..a6025eb 100644 --- a/tests/test_fastmcp_tools.py +++ b/tests/test_fastmcp_tools.py @@ -364,10 +364,13 @@ class TestGetValuationComparables: ) parsed = json.loads(result) - assert "filters_applied" in parsed - assert "statistics" in parsed - assert "comparables" in parsed - assert parsed["filters_applied"]["property_type"] == "דירה" + # Normalized structure: filters_applied is now in search_parameters + assert "search_parameters" in parsed + assert "filters_applied" in parsed["search_parameters"] + assert parsed["search_parameters"]["filters_applied"]["property_type"] == "דירה" + # Normalized structure: comparables -> deals + assert "deals" in parsed + assert "market_statistics" in parsed @patch("nadlan_mcp.fastmcp_server.client") def test_comparables_strips_bloat(self, mock_client): @@ -392,7 +395,8 @@ class TestGetValuationComparables: result = fastmcp_server.get_valuation_comparables("test address") parsed = json.loads(result) - comparable = parsed["comparables"][0] + # Normalized structure: comparables -> deals + comparable = parsed["deals"][0] assert "shape" not in comparable assert "sourceorder" not in comparable # source_polygon_id is kept when added by processing @@ -421,9 +425,12 @@ class TestGetDealStatistics: result = fastmcp_server.get_deal_statistics("test address") parsed = json.loads(result) - assert "address" in parsed - assert "statistics" in parsed - assert parsed["statistics"]["total_deals"] == 2 # Field name is total_deals in model + # Normalized structure: address is now in search_parameters, statistics -> market_statistics + assert "search_parameters" in parsed + assert "address" in parsed["search_parameters"] + assert "market_statistics" in parsed + assert "deal_breakdown" in parsed["market_statistics"] + assert parsed["market_statistics"]["deal_breakdown"]["total_deals"] == 2 class TestGetMarketActivityMetrics: