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

[dolho]

Summary

A proactive group/channel broadcast (send_group_message, #349/#350) was delivered and written nowhere, so the agent had no record of its own outreach. This is the group counterpart of #1600 (DMs).

The two channels do not come out equivalent, and that's the most important thing to know before reviewing.

Channel	Outcome	Why
Slack	✅ real recall fix	Channel sessions are thread-scoped (team:channel:thread, #903) and an in-thread reply carries thread_ts = the parent's ts. A top-level post filed at its own ts lands in exactly the session its replies resolve to. Verified against the adapter: both sides derive T1:C1:1720000000.111222
Telegram	⚠️ bookkeeping only	Group sessions are keyed per (sender, chat) — get_session_identifier has no group branch — and a broadcast has no human sender. It's filed under a synthetic agent-sender key ({bot_id}:{agent}:{chat_id}) that nothing else writes to

The Telegram limitation, stated plainly

The message becomes recorded, auditable and attributable — but the agent still will NOT recall it. No participant's inbound session contains it, so when someone replies in the group, the broadcast isn't in context. Verified: bot-7:atlas:-100999 (ours) vs bot-7:55555:-100999 (a participant's).

Real recall needs a per-chat group session, which changes behaviour for every existing inbound group conversation — a migration and a separate decision, not something to smuggle into a bug fix. test_telegram_key_deliberately_does_not_match_a_participant pins the non-match so it can't be mistaken for a bug, and fails if the adapter later grows a group branch, forcing a re-decision.

Changes

• services/channel_history.py (new) — shared persistence + key derivation. Lives in a service, not the routers (Invariant Fix: Add missing Docker labels to system agent container #1), and both endpoints need identical semantics. Key derivation drives each adapter's own get_session_identifier() rather than re-implementing the format — the bug: proactive messages (send_message) never persisted to channel session history — agent unaware of its own outbound messages #1600 drift trap.
• services/slack_service.py — send_message_detailed() → (ok, error, ts). send_message() keeps its (ok, error) contract and delegates; ~7 call sites unpack the 2-tuple. Without the ts there's no way to file a top-level post in the session its replies use.
• routers/slack.py, routers/telegram.py — persist on confirmed delivery.

Attribution (#903): assistant role, agent sender_label, sender_email=None. This is the deliberate difference from #1600, which stamps the recipient's email: a DM is single-participant, a broadcast is a shared thread and must never be folded into one participant's MEM-001 memory.

Safety: persisted only on confirmed delivery (a failed send writes no phantom turn), and fail-soft — the message is already delivered when persistence runs, so a DB error is logged loudly, never surfaced as a send failure.

Test plan — and how I know the tests are worth anything

• pytest tests/unit/test_1649_group_message_history.py — 13 passed
• lint (sys.modules) OK; enterprise-docs guard passes; 403 neighbouring channel/proactive tests green

The tests drive the real endpoints, not just the helpers — the lesson from #1600, where a probe-vs-probe suite passed over a sabotaged fix. I checked the same way here, and it mattered: with helper-only tests, deleting the Slack router's persistence call outright left all 9 green. The router tests catch:

Sabotage	Result
delete the Slack router's persist call	2 fail ✅
swap team_id/channel_id in the key call	2 fail ✅
Telegram router files under chat_id instead of the agent	1 fail ✅

Known gaps

• Not exercised against live Slack/Telegram — needs real channel credentials. A live Slack test is set up on a dev instance but blocked on a workspace bot token.
• No end-to-end round-trip (persist → build_public_chat_context contains it): every db call is mocked. Same gap as bug: proactive messages (send_message) never persisted to channel session history — agent unaware of its own outbound messages #1600; worth closing for both in one pass with the sqlite harness.
• A Slack reply posted top-level (not in-thread) starts its own session and won't see the broadcast — inherent to thread-scoping (Slack: scope chat session to thread, not channel #903).

Relationship to #1600

Independent: different layer (routers vs proactive_message_service), different session model, no shared code path today. This branches from dev, so it doesn't contain #1600. Once both land, making #1600's _persist_outbound delegate to channel_history would remove the near-duplicate — worth a small follow-up, deliberately not done here to keep the PRs reviewable apart.

Related to #1649

Fixes #1649

[dolho]

Validated via /validate-pr at fa8ce1e1. CI fully green, including the regression diff (all 6 pytest jobs — base and head × 3 seeds).

Disclosure: I wrote this PR. The table below is mechanical fact; the judgement that the design is right is not independent. Requesting @vybe for that.

Summary

Category	Status	Notes
Base branch	✅	dev
PR size	✅	9 files (+642/−7)
Issue link	✅	Related to #1649 — resolves, P2/type-bug/theme-channels/complexity-medium
Commit message	✅	conventional; states the Slack/Telegram asymmetry rather than implying parity
Requirements	➖	bug fix (tier = commit message). No requirements section exists for #321/#349/#350 proactive messaging — pre-existing gap, not introduced here
Architecture	✅	channel_history.py added to the services catalog with the recall asymmetry recorded
Feature flows	✅	telegram-integration.md (limitation) + slack-channel-routing.md (recall fix + ts capture)
Security sweep	✅	no keys, tokens, real emails, public IPs, .env, credential files
Infrastructure	➖	none touched
Build packaging	✅	channel_history.py is under services/, which the Dockerfile COPYs wholesale — no #1033-class gap
Config packaging	➖	no new os.getenv()
Code quality	✅	Invariant #1 respected (logic in a service, not the routers); send_message()'s 2-tuple contract preserved for its ~7 callers
CI	✅	pytest base+head × 3 seeds, regression diff, CodeQL, lint, prod-image-smoke — all pass

What I attacked, and what it caught

The #1600 lesson was that a green suite can sit on top of a non-fix. So I sabotaged this one before trusting it:

Sabotage	Helper-only tests	Router tests
delete the Slack router's persist call entirely	9/9 passed ❌	2 fail ✅
swap team_id/channel_id in the key call	—	2 fail ✅
Telegram router files under chat_id not the agent	—	1 fail ✅

The first row is the point: with helper-only coverage, removing the fix outright left everything green. That's why TestRoutersPersist drives the real endpoint functions.

The asymmetry a reviewer should focus on

Slack is a genuine recall fix — verified against the adapter, both sides derive T1:C1:1720000000.111222, so an in-thread reply carries the broadcast into context.

Telegram is bookkeeping only. bot-7:atlas:-100999 (broadcast) vs bot-7:55555:-100999 (a participant) — the agent still won't recall it. That's the accepted trade-off (least risk, no change to existing group sessions), documented in code/flow/architecture and pinned by a test that fails if the adapter ever grows a group branch, forcing the decision rather than silently changing behaviour.

If a reviewer thinks recall matters more than the migration cost, the per-chat group branch is the alternative — and that's the one call worth re-litigating here.

Known gaps

• No live channel test. Needs real Slack/Telegram credentials; a live Slack test is set up on a dev instance but blocked on a workspace bot token.
• No end-to-end round-trip (persist → build_public_chat_context contains it) — every db call is mocked. Same gap as fix(channels): persist proactive messages to channel session history (#1600) #1650; worth closing for both in one pass with the sqlite harness.
• A Slack reply posted top-level rather than in-thread starts its own session and won't see the broadcast — inherent to Slack: scope chat session to thread, not channel #903 thread-scoping.

Recommendation

APPROVE (pending independent review).

Mechanically clean and CI-green, with the one design decision (Telegram bookkeeping vs recall) surfaced rather than buried. @vybe — the Telegram session-model trade-off is the part I'd most like a second opinion on; everything else is mechanical.

[vybe]

Validated via /validate-pr: the Slack recall fix is asserted end-to-end at router level (posted-ts session key = the key an in-thread reply resolves to), the Telegram bookkeeping-only trade-off is pinned by a test that forces a re-decision if the adapter grows a group branch, send_message keeps its 2-tuple contract for existing callers, #903 sender_email=None attribution verified. Resolved the registry.json append conflict with #1650 (both entries kept, JSON validated). Security battery clean. Approving.