diff --git a/agents/customer_agent.py b/agents/customer_agent.py index 325d7de..1c4e8ae 100644 --- a/agents/customer_agent.py +++ b/agents/customer_agent.py @@ -1,4 +1,5 @@ """Customer Agent - Handles notifications, tracking, and customer communication.""" +import re import uuid from datetime import datetime, timedelta from typing import Dict, List, Any, Optional @@ -14,6 +15,9 @@ from core.http_client import api_post, api_get from core.registry import registry from config.system_config import GO_API_BASE_URL, INTERNAL_API_KEY +# Any {placeholder} left after template substitution — a caller forgot a variable. +_UNFILLED_PLACEHOLDER = re.compile(r"\{([a-zA-Z_][a-zA-Z0-9_]*)\}") + class NotificationChannel(str, Enum): SMS = "sms" @@ -360,6 +364,30 @@ class CustomerAgent(SpecializedAgent): "action": "provide_options", } + def _failed_notification(self, order_id, customer_id, channel, notification_type, + reason: str, message: str) -> Dict[str, Any]: + """A not-sent result in the same shape as a sent one, recorded for audit.""" + notification_id = f"NOTIF-{uuid.uuid4().hex[:8].upper()}" + key = customer_id or "unknown" + self._notifications.setdefault(key, []).append(Notification( + notification_id=notification_id, + order_id=order_id, + customer_id=key, + channel=channel, + notification_type=notification_type, + message=message, + sent_at=datetime.now(), + delivered_at=None, + status="failed", + )) + return { + "notification_id": notification_id, + "channel": channel.value, + "status": "failed", + "reason": reason, + "message": message, + } + async def _send_via_channel( self, order_id: str, @@ -368,24 +396,51 @@ class CustomerAgent(SpecializedAgent): notification_type: NotificationType, template_vars: Dict[str, Any], ) -> Dict[str, Any]: - """Render template and call Go backend (POST /api/v1/internal/notify).""" + """Render template and call Go backend (POST /api/v1/internal/notify). + + Refuses to send a message that still contains an unsubstituted + {placeholder}: a caller that omits a template variable would otherwise + put the literal text in front of a customer. Silence is better than a + broken message, and the ERROR names the caller's missing variables.""" template = self._templates.get(notification_type, {}).get(channel.value, "") + if not template: + logger.error( + f"Customer Agent: no {channel.value} template for {notification_type.value}; not sending" + ) + return self._failed_notification(order_id, customer_id, channel, notification_type, + "no template", "") + message = template for key, value in template_vars.items(): message = message.replace(f"{{{key}}}", str(value)) + missing = _UNFILLED_PLACEHOLDER.findall(message) + if missing: + logger.error( + f"Customer Agent: refusing to send {notification_type.value} for order {order_id} — " + f"template variables not supplied: {missing}" + ) + return self._failed_notification(order_id, customer_id, channel, notification_type, + f"unfilled template variables: {missing}", message) + notification_id = f"NOTIF-{uuid.uuid4().hex[:8].upper()}" + payload = { + "notification_id": notification_id, + "order_id": order_id, + "channel": channel.value, + "notification_type": notification_type.value, + "message": message, + } + # Omit customer_id rather than sending a placeholder: callers that only + # know the booking (e.g. DispatchAgent on an assignment failure) leave the + # backend to resolve the recipient from order_id, which it can already do. + if customer_id and str(customer_id) != "unknown": + payload["customer_id"] = customer_id + result = await api_post( f"{GO_API_BASE_URL}/api/v1/internal/notify", - json={ - "notification_id": notification_id, - "order_id": order_id, - "customer_id": customer_id, - "channel": channel.value, - "notification_type": notification_type.value, - "message": message, - }, + json=payload, headers={"X-Internal-Key": INTERNAL_API_KEY}, timeout=aiohttp.ClientTimeout(total=10), ) @@ -407,11 +462,13 @@ class CustomerAgent(SpecializedAgent): delivered_at=None, status=status, ) - if customer_id not in self._notifications: - self._notifications[customer_id] = [] - self._notifications[customer_id].append(notification) + history_key = customer_id or "unknown" + self._notifications.setdefault(history_key, []).append(notification) - logger.info(f"[{channel.value.upper()}] {notification_type.value} -> {customer_id[:20]} [{status}]") + logger.info( + f"[{channel.value.upper()}] {notification_type.value} -> " + f"{str(history_key)[:20]} [{status}]" + ) return { "notification_id": notification_id, diff --git a/agents/dispatch_agent.py b/agents/dispatch_agent.py index 7cad049..fde7ae1 100644 --- a/agents/dispatch_agent.py +++ b/agents/dispatch_agent.py @@ -474,11 +474,15 @@ class DispatchAgent(SpecializedAgent): recipient="CUSTOMER_AGENT", message_type=MessageType.AGENT_TASK, payload={ - # CUSTOMER_AGENT's real task contract (send_notification + DELAYED template) + # CUSTOMER_AGENT's real task contract (send_notification + DELAYED + # template). Every variable the template names must be supplied or + # the send is refused; we have no ETA at assignment-failure time, + # so say so rather than inventing one. No customer_id: the event + # carries none, and the backend resolves the recipient from order_id. "task_type": "send_notification", "order_id": booking_id, "notification_type": "delayed", - "template_vars": {"order_id": booking_id, "reason": "finding the right rider"}, + "template_vars": {"order_id": booking_id, "new_eta": "being confirmed"}, }, correlation_id=str(booking_id), ) diff --git a/tests/test_customer_notify.py b/tests/test_customer_notify.py new file mode 100644 index 0000000..09774af --- /dev/null +++ b/tests/test_customer_notify.py @@ -0,0 +1,117 @@ +"""CUSTOMER_AGENT must never put an unsubstituted {placeholder} in front of a +customer, and must not send a literal "unknown" customer id. + +The DispatchAgent assignment-failure path knows only a booking id, so this is +the realistic caller: it supplies order_id and new_eta and no customer. +""" +import pytest + +from agents.customer_agent import ( + CustomerAgent, NotificationChannel, NotificationType, _UNFILLED_PLACEHOLDER, +) + + +def make_agent(captured): + agent = CustomerAgent.__new__(CustomerAgent) + agent.agent_id = "CUSTOMER_AGENT" + agent._notifications = {} + agent._templates = CustomerAgent._init_templates(agent) + + async def fake_post(url, json=None, headers=None, timeout=None): + captured.append(json) + return {"ok": True} + + import agents.customer_agent as mod + mod.api_post = fake_post + return agent + + +@pytest.fixture(autouse=True) +def restore_api_post(): + import agents.customer_agent as mod + original = mod.api_post + yield + mod.api_post = original + + +@pytest.mark.asyncio +async def test_missing_template_var_is_refused_not_sent(): + sent = [] + agent = make_agent(sent) + result = await agent._send_via_channel( + order_id="36", customer_id="unknown", + channel=NotificationChannel.SMS, + notification_type=NotificationType.DELAYED, + template_vars={"order_id": "36"}, # new_eta omitted — the old bug + ) + assert result["status"] == "failed" + assert "new_eta" in result["reason"] + assert sent == [] # nothing reached the backend + + +@pytest.mark.asyncio +async def test_all_vars_supplied_sends_clean_message(): + sent = [] + agent = make_agent(sent) + result = await agent._send_via_channel( + order_id="36", customer_id="unknown", + channel=NotificationChannel.SMS, + notification_type=NotificationType.DELAYED, + template_vars={"order_id": "36", "new_eta": "being confirmed"}, + ) + assert result["status"] == "sent" + assert len(sent) == 1 + assert not _UNFILLED_PLACEHOLDER.findall(sent[0]["message"]) + assert sent[0]["message"] == ( + "Delay alert: Order 36 may arrive later than expected. New ETA: being confirmed" + ) + + +@pytest.mark.asyncio +async def test_unknown_customer_id_is_omitted_from_payload(): + sent = [] + agent = make_agent(sent) + await agent._send_via_channel( + order_id="36", customer_id="unknown", + channel=NotificationChannel.SMS, + notification_type=NotificationType.DELAYED, + template_vars={"order_id": "36", "new_eta": "being confirmed"}, + ) + assert "customer_id" not in sent[0] # backend resolves from order_id + assert sent[0]["order_id"] == "36" + + +@pytest.mark.asyncio +async def test_real_customer_id_is_sent(): + sent = [] + agent = make_agent(sent) + await agent._send_via_channel( + order_id="36", customer_id="CUST-9", + channel=NotificationChannel.SMS, + notification_type=NotificationType.DELAYED, + template_vars={"order_id": "36", "new_eta": "being confirmed"}, + ) + assert sent[0]["customer_id"] == "CUST-9" + + +@pytest.mark.asyncio +async def test_dispatch_supplies_every_template_var(): + """The DispatchAgent payload must satisfy the DELAYED template exactly.""" + from agents.dispatch_agent import DispatchAgent + sent_msgs = [] + + async def capture(recipient, message_type, payload, correlation_id=None): + sent_msgs.append(payload) + + agent = DispatchAgent.__new__(DispatchAgent) + agent.send_message = capture + await agent._notify_customer_delay("36") + + payload = sent_msgs[0] + assert payload["task_type"] == "send_notification" + template = CustomerAgent._init_templates(CustomerAgent.__new__(CustomerAgent))[ + NotificationType.DELAYED]["sms"] + required = set(_UNFILLED_PLACEHOLDER.findall(template)) + assert required <= set(payload["template_vars"]), ( + f"template needs {required}, dispatch supplies {set(payload['template_vars'])}" + )