fix(customer): never send an unsubstituted placeholder or a fake customer id
With DISPATCH_AGENT autonomous=true in the agent registry, the assignment-failure
path sends a real customer message. It would have sent
"Delay alert: Order 36 may arrive later than expected. New ETA: {new_eta}"
with customer_id "unknown": the DELAYED template names {new_eta} but DispatchAgent
supplied only order_id, and it has no customer id to give.
- _send_via_channel refuses to send when any {placeholder} survives substitution
(or the template is missing), logs which variables the caller omitted, and
records a failed notification. Silence beats a broken message.
- customer_id is omitted from the payload when it is absent or "unknown" rather
than sent literally; the backend resolves the recipient from order_id.
- DispatchAgent supplies new_eta "being confirmed" — honest, since no ETA exists
at assignment-failure time.
- Tests: the old bug is refused, a complete payload renders clean, the id is
omitted/passed correctly, and DispatchAgent's vars satisfy the template.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012AJLYcbTHCe45fyFnMfEin
This commit is contained in:
@@ -1,4 +1,5 @@
|
|||||||
"""Customer Agent - Handles notifications, tracking, and customer communication."""
|
"""Customer Agent - Handles notifications, tracking, and customer communication."""
|
||||||
|
import re
|
||||||
import uuid
|
import uuid
|
||||||
from datetime import datetime, timedelta
|
from datetime import datetime, timedelta
|
||||||
from typing import Dict, List, Any, Optional
|
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 core.registry import registry
|
||||||
from config.system_config import GO_API_BASE_URL, INTERNAL_API_KEY
|
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):
|
class NotificationChannel(str, Enum):
|
||||||
SMS = "sms"
|
SMS = "sms"
|
||||||
@@ -360,6 +364,30 @@ class CustomerAgent(SpecializedAgent):
|
|||||||
"action": "provide_options",
|
"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(
|
async def _send_via_channel(
|
||||||
self,
|
self,
|
||||||
order_id: str,
|
order_id: str,
|
||||||
@@ -368,24 +396,51 @@ class CustomerAgent(SpecializedAgent):
|
|||||||
notification_type: NotificationType,
|
notification_type: NotificationType,
|
||||||
template_vars: Dict[str, Any],
|
template_vars: Dict[str, Any],
|
||||||
) -> 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, "")
|
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
|
message = template
|
||||||
for key, value in template_vars.items():
|
for key, value in template_vars.items():
|
||||||
message = message.replace(f"{{{key}}}", str(value))
|
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()}"
|
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(
|
result = await api_post(
|
||||||
f"{GO_API_BASE_URL}/api/v1/internal/notify",
|
f"{GO_API_BASE_URL}/api/v1/internal/notify",
|
||||||
json={
|
json=payload,
|
||||||
"notification_id": notification_id,
|
|
||||||
"order_id": order_id,
|
|
||||||
"customer_id": customer_id,
|
|
||||||
"channel": channel.value,
|
|
||||||
"notification_type": notification_type.value,
|
|
||||||
"message": message,
|
|
||||||
},
|
|
||||||
headers={"X-Internal-Key": INTERNAL_API_KEY},
|
headers={"X-Internal-Key": INTERNAL_API_KEY},
|
||||||
timeout=aiohttp.ClientTimeout(total=10),
|
timeout=aiohttp.ClientTimeout(total=10),
|
||||||
)
|
)
|
||||||
@@ -407,11 +462,13 @@ class CustomerAgent(SpecializedAgent):
|
|||||||
delivered_at=None,
|
delivered_at=None,
|
||||||
status=status,
|
status=status,
|
||||||
)
|
)
|
||||||
if customer_id not in self._notifications:
|
history_key = customer_id or "unknown"
|
||||||
self._notifications[customer_id] = []
|
self._notifications.setdefault(history_key, []).append(notification)
|
||||||
self._notifications[customer_id].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 {
|
return {
|
||||||
"notification_id": notification_id,
|
"notification_id": notification_id,
|
||||||
|
|||||||
@@ -474,11 +474,15 @@ class DispatchAgent(SpecializedAgent):
|
|||||||
recipient="CUSTOMER_AGENT",
|
recipient="CUSTOMER_AGENT",
|
||||||
message_type=MessageType.AGENT_TASK,
|
message_type=MessageType.AGENT_TASK,
|
||||||
payload={
|
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",
|
"task_type": "send_notification",
|
||||||
"order_id": booking_id,
|
"order_id": booking_id,
|
||||||
"notification_type": "delayed",
|
"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),
|
correlation_id=str(booking_id),
|
||||||
)
|
)
|
||||||
|
|||||||
117
tests/test_customer_notify.py
Normal file
117
tests/test_customer_notify.py
Normal file
@@ -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'])}"
|
||||||
|
)
|
||||||
Reference in New Issue
Block a user