Repository navigation
Conversation
houko
left a comment
There was a problem hiding this comment.
Solid feature — the three-way routing (stranger→agent, agent→owner notification, owner→stranger relay) is well-structured with good safeguards (rate limiting, dedup, escalation debouncing, audit logging).
A few things to address:
[P1] Hand-rolled TOML parser is fragile
The regex-based TOML parsing doesn't handle comments, escaping, multi-line values, or quoted strings with = inside. Consider using a proper TOML library (npm install toml) instead — the fallback-to-defaults behavior silently masks config mistakes.
[P1] Memory growth at scale
activeConversations, rateLimitMap, and lastEscalationTime are unbounded Maps. The cleanup intervals help, but expired rate limit entries for inactive JIDs won't self-clean. Add max size limits with LRU eviction, or periodic full-scan cleanup.
[P2] loggedOut disconnect reason removed
The change removes DisconnectReason.loggedOut from the reconnection guard. Was this intentional? Without it, a WhatsApp logout will trigger reconnection attempts instead of requiring manual re-login.
[P2] Prompt injection edge cases
The [NOTIFY_OWNER]...[/NOTIFY_OWNER] delimiters work for normal cases, but nested brackets in user messages could confuse the regex. Worth documenting the assumptions.
Minor:
- Owner number validation (7-15 digits) only warns but doesn't reject — could be surprising if misconfigured
- No length limits on relay messages
- Consider what happens if agent response contains both
[NOTIFY_OWNER]AND[RELAY_TO_STRANGER]tags simultaneously
Overall the code quality is good and the feature addresses a real need. Happy to approve once P1 items are addressed.
…g#937) Complete rewrite of WhatsApp gateway message routing: - Conversation Tracker: in-memory Map with TTL (24h default), tracks active stranger sessions with message history, escalation state - Stranger ↔ Agent direct conversation: strangers talk directly with the agent instead of being forwarded to owner. Removed generateSenderAck - Owner context injection: [ACTIVE_STRANGER_CONVERSATIONS] block injected when owner messages agent, with [ESCALATED] markers - Owner → Stranger relay: [RELAY_TO_STRANGER] tag parsing with JID validation against active conversations - [NOTIFY_OWNER] tag: agent can selectively notify owner with structured JSON payload (reason + summary) - Rate limiting: per-JID (3 msg/min) for strangers with debounce - Escalation deduplication: 5-minute cooldown per stranger - Multi-owner fix: replies go to sender's JID, not always OWNER_JID[0] - Prompt injection mitigation: system instructions use structured delimiters instead of raw concatenation - Non-text media: descriptors for photos, videos, voice messages, stickers, locations, contacts, documents (no silent drop) - Audit logging for all relayed messages Closes librefang#937
212fb62 to
3ab7b35
Compare
|
Hey @f-liva — nice work on this feature overall. A couple things before we can merge:
Please address these and we can merge. Thanks! |
…, memory leaks, crash - Replace hand-rolled regex TOML parser with proper `toml` npm library (P1) - Add periodic cleanup for lastEscalationTime map to prevent unbounded growth (P1) - Fix /conversations endpoint crash: missing `req` param in jsonResponse call - Remove duplicate OWNER_JID declaration (dead code from old routing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, memory leaks, crash - Replace hand-rolled regex TOML parser with proper `toml` npm library (P1) - Add periodic cleanup for lastEscalationTime map to prevent unbounded growth (P1) - Fix /conversations endpoint crash: missing `req` param in jsonResponse call - Remove duplicate OWNER_JID declaration (dead code from old routing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
So, I've made the fixes for this PR, but please don't merge it yet, because over the weekend I rewrote the entire WhatsApp system regarding this issue—and not just that. So, let's hold off on this specific PR for a moment |
|
Superseded by a new PR from a clean branch rebased on current upstream/main. The old branch had a stale base with 3.7MB of unrelated diff noise. |
…, memory leaks, crash - Replace hand-rolled regex TOML parser with proper `toml` npm library (P1) - Add periodic cleanup for lastEscalationTime map to prevent unbounded growth (P1) - Fix /conversations endpoint crash: missing `req` param in jsonResponse call - Remove duplicate OWNER_JID declaration (dead code from old routing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Implements full bidirectional message routing for the WhatsApp gateway, resolving the core issues reported in #937:
generateSenderAck— the agent's response IS the reply[ACTIVE_STRANGER_CONVERSATIONS]block injected into owner messages so the agent knows who's waiting[RELAY_TO_STRANGER]tag with JID validation — owner tells agent what to relay, agent reformulates and sends[NOTIFY_OWNER]selective escalation: agent decides when to notify the owner (structured JSON with reason + summary)Bug fixes included
OWNER_JID[0]Design principle
No personality, escalation rules, or behavioral logic in the gateway — it provides routing mechanisms (
[NOTIFY_OWNER],[RELAY_TO_STRANGER], context injection). All behavior is defined by the agent's identity files (AGENTS.md / SOUL.md).Configuration
Only one new config field:
Test plan
[NOTIFY_OWNER]→ owner receives notification, stranger gets clean response[RELAY_TO_STRANGER]→ message delivered to strangerCloses #937