fix(portal+data): Secure Hub DB accessor, durable chat leads, booking race (P0b/P1b)

appointment_manager.py

@@ -136,7 +136,11 @@ def txn_create(transaction):
                 .where("time_slot", "==", appt.time_slot)
                 .where("status", "==", "confirmed")
             )
-            existing = list(query.stream())
+            # Read INSIDE the transaction (transaction=transaction) so the slot
+            # count joins the transaction's read-set / optimistic lock. Without
+            # it the read is non-transactional and two concurrent bookings can
+            # both see < MAX_PER_SLOT and both commit -> the slot is oversold.
+            existing = list(query.stream(transaction=transaction))
             if len(existing) >= MAX_PER_SLOT:
                 raise ValueError(f"Time slot {appt.time_slot} on {appt.date} is fully booked.")
             transaction.set(doc_ref, appt.to_dict())

main.py

@@ -6771,7 +6771,7 @@ async def get_secure_hub_deal(deal_id: str, request: Request, phone: str = ""):
     this is not a deal-existence oracle.
     """
     try:
-        db = get_database()
+        db = get_database().db  # THODatabase wrapper -> raw Firestore client
         deal = db.collection("deals").document(deal_id).get()
         deal_data = deal.to_dict() if deal.exists else None
         if not deal_data or not _deal_phone_ok(deal_data, phone):
@@ -6820,7 +6820,7 @@ async def download_secure_document(deal_id: str, note_id: str, request: Request,
     anonymously.
     """
     try:
-        db = get_database()
+        db = get_database().db  # THODatabase wrapper -> raw Firestore client
         deal = db.collection("deals").document(deal_id).get()
         deal_data = deal.to_dict() if deal.exists else None
         if not deal_data or not _deal_phone_ok(deal_data, phone):

tests/test_crm.py

@@ -12,15 +12,33 @@
 
 
 @pytest.fixture(autouse=True)
-def _no_disk_writes(monkeypatch):
-    """Keep save_lead's success path from appending to the repo's data/leads.json.
+def _no_external_writes(monkeypatch):
+    """Keep save_lead hermetic — no local file write AND no real Firestore.
 
     save_lead guards its file write behind ``os.access(..., os.W_OK)``; forcing
-    that False exercises the full structure-and-return logic while skipping the
-    side effect, so collecting/running tests never pollutes local lead data.
+    that False skips the data/leads.json side effect. We also stub the lead
+    manager with an in-memory fake so the durable-persist path succeeds without a
+    real Firestore client (which needs ADC and could write to a live project —
+    that exact gap let test data leak into prod before this fixture existed).
     """
     monkeypatch.setattr(crm_tools.os, "access", lambda *a, **k: False)
 
+    class _FakeDocRef:
+        def set(self, data):
+            pass
+
+    class _FakeColl:
+        def document(self, _id):
+            return _FakeDocRef()
+
+    class _FakeDB:
+        def collection(self, name):
+            return _FakeColl()
+
+    monkeypatch.setattr(
+        crm_tools, "_get_lead_manager", lambda: type("M", (), {"db": _FakeDB()})()
+    )
+
 
 def test_save_lead_valid_returns_success():
     result = crm_tools.save_lead(

tests/test_lead_capture_and_booking.py

@@ -0,0 +1,132 @@
+"""P1b regression guards (holistic review): durable lead capture + booking race.
+
+1. The chat agent's save_lead tool must persist the customer's name+phone to
+   Firestore (durable), not only to stdout + an ephemeral local file — otherwise
+   high-intent chat leads are silently lost on Cloud Run while the customer is
+   told "saved". It must also report failure truthfully.
+2. The appointment booking transaction must READ the slot count inside the
+   transaction (stream(transaction=...)), else two concurrent bookings can both
+   pass the MAX_PER_SLOT check and oversell a slot.
+"""
+
+from __future__ import annotations
+
+import os
+import sys
+from unittest.mock import patch
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+
+# ─── chat lead persistence ──────────────────────────────────────────────────
+
+
+class _FakeDocRef:
+    def __init__(self, sink):
+        self._sink = sink
+
+    def set(self, data):
+        self._sink["data"] = data
+
+
+class _FakeColl:
+    def __init__(self, sink):
+        self._sink = sink
+
+    def document(self, doc_id):
+        self._sink["doc_id"] = doc_id
+        return _FakeDocRef(self._sink)
+
+
+class _FakeDB:
+    def __init__(self, sink):
+        self._sink = sink
+
+    def collection(self, name):
+        self._sink["collection"] = name
+        return _FakeColl(self._sink)
+
+
+def test_save_lead_persists_to_firestore(monkeypatch):
+    import tools.crm_tools as crm
+
+    sink = {}
+    monkeypatch.setattr(crm, "_get_lead_manager", lambda: type("M", (), {"db": _FakeDB(sink)})())
+
+    res = crm.save_lead("Jordan Brooks", "512-555-0123", "wants a 3/2 doublewide under 100k")
+
+    assert res["success"] is True
+    assert sink["collection"] == "leads"
+    assert sink["data"]["name"] == "Jordan Brooks"
+    assert "5125550123" in sink["data"]["phone"].replace("+1", "")  # normalized + stored
+    assert sink["data"]["source"] == "chat"
+    assert sink["data"]["triage_notes"] == "wants a 3/2 doublewide under 100k"
+
+
+def test_save_lead_reports_failure_when_firestore_down(monkeypatch):
+    import tools.crm_tools as crm
+
+    def boom():
+        raise RuntimeError("firestore unreachable")
+
+    monkeypatch.setattr(crm, "_get_lead_manager", boom)
+
+    res = crm.save_lead("Jordan Brooks", "512-555-0123", "interested")
+    # Must NOT falsely confirm a lead that wasn't durably saved.
+    assert res["success"] is False
+
+
+def test_save_lead_still_validates_input():
+    import tools.crm_tools as crm
+
+    assert crm.save_lead("", "512-555-0123", "x")["success"] is False  # no name
+    assert crm.save_lead("Jordan", "123", "x")["success"] is False  # too short
+
+
+# ─── appointment double-booking race ────────────────────────────────────────
+
+
+def test_create_appointment_reads_inside_transaction():
+    import appointment_manager as am
+    from appointment_manager import Appointment, AppointmentManager
+
+    captured = {}
+
+    class FakeQuery:
+        def where(self, *a, **k):
+            return self
+
+        def stream(self, transaction=None):
+            captured["transaction"] = transaction
+            return iter([])  # no existing bookings
+
+    class FakeColl:
+        def where(self, *a, **k):
+            return FakeQuery()
+
+        def document(self, _id):
+            return object()
+
+    class FakeTxn:
+        def set(self, *a, **k):
+            pass
+
+    mgr = AppointmentManager.__new__(AppointmentManager)
+    mgr._collection = lambda: FakeColl()
+    sentinel_txn = FakeTxn()
+    mgr.db = type("DB", (), {"transaction": lambda self: sentinel_txn})()
+
+    appt = Appointment(
+        appointment_id="a1",
+        name="X",
+        phone="5125550000",
+        date="2031-07-01",  # future weekday-agnostic; passes the past/today checks
+        time_slot="10:00 AM",
+        status="confirmed",
+    )
+
+    # Make @firestore.transactional a pass-through so txn_create runs directly.
+    with patch.object(am.firestore, "transactional", lambda f: f):
+        mgr.create_appointment_sync(appt)
+
+    assert captured.get("transaction") is sentinel_txn, "slot read not bound to the transaction"

tests/test_pii_endpoint_hardening.py

@@ -79,6 +79,10 @@ def __init__(self, deal_doc, note_doc=None, notes=()):
             "deals": _Coll(deal_doc, notes=notes),
             "deal_notes": _Coll(note_doc, notes=notes),
         }
+        # The endpoints use get_database().db (THODatabase wrapper -> raw client).
+        # Mirror that here so the test exercises the real accessor — its absence
+        # is why the original wrong-accessor 500 shipped undetected.
+        self.db = self
 
     def collection(self, name):
         return self._c[name]

tools/crm_tools.py

@@ -9,6 +9,7 @@
 import logging
 import os
 import re
+import uuid
 from datetime import date, datetime, timedelta
 from zoneinfo import ZoneInfo
 
@@ -20,6 +21,20 @@
 
 TIMEZONE = ZoneInfo("America/Chicago")
 
+# Lazily-created so importing this module (and unit tests) never spins up a real
+# Firestore client. Tests monkeypatch _get_lead_manager / _lead_manager.
+_lead_manager = None
+
+
+def _get_lead_manager():
+    """Return a cached LeadManager (durable Firestore-backed lead store)."""
+    global _lead_manager
+    if _lead_manager is None:
+        from lead_management import LeadManager
+
+        _lead_manager = LeadManager()
+    return _lead_manager
+
 
 def save_lead(
     user_name: str, phone_number: str, interest_notes: str, tool_context: ToolContext = None
@@ -66,6 +81,30 @@ def save_lead(
     # 3. Log to stdout (Cloud Logging)
     logger.info(json.dumps(lead_data))
 
+    # 3.5 Persist DURABLY to Firestore — the ONLY storage that survives on Cloud
+    # Run (the container filesystem is ephemeral, so the local-file write below
+    # is a dev convenience only). Without this the chat agent told customers
+    # "saved" while their name+phone existed only in Cloud Logging — invisible to
+    # the CRM (high-intent leads silently lost). A lead is reported saved ONLY if
+    # this write succeeds.
+    persisted = False
+    try:
+        from lead_management import Lead, normalize_phone
+
+        lead = Lead(
+            lead_id=f"chat_{uuid.uuid4().hex[:12]}",
+            user_id="chat",
+            session_id="",
+            name=user_name,
+            phone=normalize_phone(phone_number),
+            source="chat",
+            triage_notes=interest_notes,
+        )
+        _get_lead_manager().db.collection("leads").document(lead.lead_id).set(lead.to_dict())
+        persisted = True
+    except Exception as e:
+        logger.error(f"Lead Firestore persist FAILED — lead at risk: {e}")
+
     # 4. Dev/Fallback: Append to local JSON file if possible
     try:
         data_dir = os.path.join(os.path.dirname(__file__), "..", "data")
@@ -91,9 +130,19 @@ def save_lead(
     except Exception as e:
         logger.warning(f"Could not write to local file: {e}")
 
+    if persisted:
+        return {
+            "success": True,
+            "message": f"Thanks {user_name}! I've saved your info. A sales representative will call you at {phone_number} shortly.",
+        }
+    # Firestore write failed — be truthful so the agent routes the customer to a
+    # human instead of falsely confirming a lead that wasn't durably saved.
     return {
-        "success": True,
-        "message": f"Thanks {user_name}! I've saved your info. A sales representative will call you at {phone_number} shortly.",
+        "success": False,
+        "message": (
+            f"Thanks {user_name} — I had trouble saving your details just now. "
+            "Please call our showroom directly and we'll make sure a representative reaches you."
+        ),
     }