fix(channels): persist proactive messages to channel session history (#1600)

[dolho]

Summary

A proactive send_message (#321) was delivered and written nowhere. The next inbound turn builds context from build_public_chat_context, which only ever contained turns the message router persisted — so the agent had no record of its own outreach. It would repeat itself, contradict the message, or fail to parse the reply ("yes, sounds good" — to what?).

Now fixed for all three DM channels (Telegram, WhatsApp, and Slack).

⚠️ The issue's suggested fix cannot work — worth reading before reviewing

The issue proposes keying off the chat link's session_id, and states the seam "already exists for Telegram and WhatsApp". It doesn't.

telegram_chat_links.session_id and whatsapp_chat_links.session_id are never written by any code path. Across the whole repo they appear in exactly three places, all SELECTs:

db/telegram_channels.py:273   telegram_chat_links.c.session_id,     ← select
db/telegram_channels.py:411   telegram_chat_links.c.session_id,     ← select
db/whatsapp_channels.py:370   whatsapp_chat_links.c.session_id,     ← select

record_inbound (the only writer of those tables) inserts telegram_user_id + message_count and never a session. They're vestigial columns.

Implemented as suggested, the fix would have persisted nothing — silently — and looked done. AC-6 ("recipient with no session yet") isn't an edge case under that design; it's every recipient.

Approach

Resolve the session by identifier via get_or_create_public_chat_session, and make the identifier provably equal to the inbound one.

That equality is the whole fix. Persisting into a session is worthless — it has to be the session the recipient's next message resolves to, or the write succeeds and the bug survives invisibly. So each _deliver_* sets DeliveryResult.session_identifier by driving its own adapter's get_session_identifier() with a synthetic DM, never by re-deriving the format in the service:

Channel	Key	Derived from
Telegram	{bot_id}:{user}:{chat}	binding bot_id + chat link's telegram_user_id (a DM's chat_id IS the user id — the existing send already relies on this)
WhatsApp	{binding_id}:{phone}	binding id + chat link's wa_user_phone
Slack	{team}:{user}:{dm_channel}	workspace team_id + looked-up user + opened DM channel

Why not just format the string in the service? The formats have subtle fallbacks that would drift — Telegram's bot_id defaults to "" in the webhook but "unknown" in the adapter. A mismatch mints a second session and reintroduces the bug in a place nothing checks.

Slack is fixed, not documented as a limitation (AC-4 allowed either). It has no chat-link table, but _deliver_slack already resolves team, user, and DM channel — everything its DM key needs.

AC coverage

• AC-1/2/3 — Telegram/WhatsApp persist; the next turn's context includes the message (identifier equality is what guarantees it)
• AC-4 — Slack persisted, not deferred
• AC-5 — persist only on confirmed delivery; a failed send writes no phantom turn
• AC-6 — moot under this design (the column was never the key); the session is created when absent, and that's documented
• AC-7 — sender_email = recipient. A proactive send targets ONE verified email — a DM, i.e. single-participant — so _assistant_sender_email's rule (Slack: scope chat session to thread, not channel #903) resolves to the recipient, keeping sender-filtered MEM-001 in the right user's memory

Scope: persistence hangs off _send_message_inner, not the shared _deliver_* helpers, so send_access_grant_notification (out of scope, #951) is excluded structurally rather than by a flag. Fail-soft throughout: the message is already delivered when persistence runs, so a DB error is logged loudly and never surfaces as a delivery failure.

send_group_message → #1649

Confirmed the same gap (0 persistence calls in both send_telegram_group_message and send_agent_slack_channel_message), but it's a different problem, not a port: Telegram group sessions are per-(sender, chat) and a broadcast has no sender; Slack channels are thread-scoped on a ts only knowable after the send. That's a semantics decision — filed as #1649 with the analysis.

Test Plan

• pytest tests/unit/test_1600_proactive_history.py — 12 passed
• pytest tests/test_proactive_audit_unit.py — existing suite green (16 total)
• Reverting the _persist_outbound call fails test_successful_delivery_persists (the end-to-end regression). Honest note: only that 1 of 12 flips — the parity tests drive the adapters, which this PR doesn't change; they exist to catch future drift, not to prove today's fix.
• Not exercised against live Telegram/Slack/WhatsApp — needs real channel credentials

Related to #1600

Fixes #1600

[dolho]

Test-coverage correction (c51f08e9)

Asked directly whether the tests really cover this, I checked instead of asserting — and they didn't.

I sabotaged the service two ways, each of which silently derives a different session key and therefore leaves the bug fully intact:

• Telegram metadata key typo: bot_id → botid_TYPO (yields unknown:123:123)
• Slack probe dropping is_dm (takes the thread branch, not the DM branch)

All 12 tests passed against both. The suite would have shipped green over a non-fix.

The hole: TestSessionIdentifierParity compares two probes constructed in the test file. It proves the adapters are self-consistent — never in doubt — and never touches the probe the service builds. It was testing the wrong side of the seam.

c51f08e9 adds TestDeliveryPathProducesTheInboundKey, which drives the real _deliver_telegram / _deliver_slack with mocked channel APIs and asserts the returned session_identifier equals the key an inbound DM resolves to. Both sabotages now fail their test and nothing else. Also asserts a failed send carries no session key (nothing to persist against). 15 tests.

Still not covered — stating it rather than implying it

AC-2's end-to-end round-trip is not tested: "proactive send → inbound reply → context contains the proactive message". Every db call in this suite is mocked, so nothing proves add_public_chat_message and build_public_chat_context actually round-trip through a real store. What's verified is that the right call is made with the right arguments against the right session key — not that the agent genuinely sees the message on its next turn.

Closing that needs a real DB (the sqlite harness) or a live channel. Worth doing before this is trusted in prod; happy to add it if a reviewer wants it in this PR rather than a follow-up.

[dolho]

Validated via /validate-pr at a8b4131b.

Disclosure: I wrote every commit here. The mechanical checks below are facts; the judgement that the fix is right is not independent. The two real findings this pass produced both came from CI and from adversarially attacking my own tests — not from reading them approvingly.

Findings this pass

1. CI caught a real regression — my tests poisoned another suite. The base-vs-head diff flagged two EXISTING tests failing under HEAD:

test_1609_proactive_rate_limits::test_limit_is_sourced_from_settings_not_the_constant
test_1609_proactive_rate_limits::test_zero_limit_is_unlimited_and_skips_redis

My fixture stubbed sys.modules and forced a re-import so the stubs bound at import time. Re-importing creates a new module object and rebinds the proactive_message_service attribute on the services package; monkeypatch restores sys.modules but not that attribute. #1609 reaches the module via import services.proactive_message_service as pms (package attribute → my re-imported object) while its svc fixture goes through sys.modules (→ the original). It patched one object and exercised another, so its patch silently did nothing and the real limit answered.

Fixed in a8b4131b by dropping sys.modules manipulation entirely and patching the bound names — which is the pattern #1609 itself documents in a comment ("patch the name as bound in the CONSUMER module ... robust to sys.modules identity quirks"). Strictly less machinery, nothing to leak. Both files now pass in both orders and under random seeds. Local runs never showed it — it needs 1600-then-1609 in one session; the seed matrix is what found it.

2. The tests didn't cover the fix (c51f08e9, detailed in the comment above). Sabotaging the service two ways — telegram bot_id→botid_TYPO, slack probe dropping is_dm, each silently sending the turn to a different session and leaving the bug intact — passed all 12 tests. The parity tests compared two probes written in the test file and never touched the service's probe. Now covered by TestDeliveryPathProducesTheInboundKey; both sabotages fail it.

Validation summary

Category	Status	Notes
Base branch	✅	dev
PR size	✅	4 files (+566/−1)
Issue link	✅	Related to #1600 — resolves, P2/type-bug/theme-channels
Commit messages	✅	conventional; each explains why, incl. the two self-corrections
Requirements	➖	bug fix (tier = commit message only). Note: #321 proactive messaging has no requirements section at all — a pre-existing gap, not introduced here
Architecture	➖	no endpoint/schema/component change
Feature flow	✅	proactive-messaging.md — new §5 documents the key derivation, the vestigial-column finding, #903 attribution, and the #951/#1649 scope lines
Security sweep	✅	no keys, tokens, real emails, public IPs, .env, credential files
Infrastructure	➖	none touched
Build/config packaging	➖	no new top-level module, no new os.getenv()
Code quality	✅	scoped to the stated purpose; matches router step 11's persistence shape
Tests	✅	15 + existing suites green; lint OK
CI	⚠️	re-running after a8b4131b — the regression diff must go green before merge

Known gaps (stated, not hidden)

• AC-2 is not tested end-to-end. "proactive send → inbound reply → context contains the proactive message" — every db call is mocked, so nothing proves add_public_chat_message + build_public_chat_context round-trip through a real store. Needs the sqlite harness or a live channel.
• Not exercised against live Telegram/Slack/WhatsApp. A live Slack test was set up on a dev instance but is blocked on a workspace bot token.
• test_no_session_identifier_skips_persistence_without_crashing covers a defensive branch that can't currently fire: _deliver_web always raises, so no channel returns success without a key today.

Recommendation

APPROVE once CI is green — pending the regression diff clearing on a8b4131b.

The fix itself is sound and the reasoning is documented where the next reader will hit it. But I'd weight an independent review heavily here: across three passes this PR produced a test suite that passed over a sabotaged fix and a fixture that broke a neighbouring suite. Both were found by adversarial checks and CI, not by me reading my own work. The AC-2 round-trip is the gap I'd most want closed before this is trusted in production.

[vybe]

Validated via /validate-pr: session-key parity is asserted through the adapters' own get_session_identifier (both probe-parity and service-probe tests — the latter catch a sabotaged probe), persist-only-on-confirmed-delivery, fail-soft persistence, #903 attribution verified (sender_email=recipient for single-participant DM). Confirmed the db facade call shape matches the inbound router's (agent_name as link_id is the router's own convention, message_router.py:455). Security battery clean. Approving.