Skip to content

fix(channels): count inbound client messages on the Sharing-tab roster (#1533)#1554

Merged
vybe merged 2 commits into
devfrom
feature/1533-client-message-count
Jul 9, 2026
Merged

fix(channels): count inbound client messages on the Sharing-tab roster (#1533)#1554
vybe merged 2 commits into
devfrom
feature/1533-client-message-count

Conversation

@webmixgamer

Copy link
Copy Markdown
Contributor

Summary

The agent Sharing tab's client roster showed 0 messages for every external client. Investigating it turned up a wider fault than the issue describes — one dead call site causing three symptoms:

  1. message_count was always 0 — the reported bug.
  2. last_active was also stale. It was the /login timestamp, not last activity.
  3. Clients who never ran /login never appeared in the roster at all.

Root cause: get_or_create_chat_link and increment_message_count (Telegram + WhatsApp) had zero callers outside the database.py facade. The shared inbound path (message_router._handle_message_inner) writes public_chat_sessions — a different table — so *_chat_links rows were only ever created by set_*_verified_email. The read path faithfully echoed a column nobody wrote, and client_roster_service's own docstring asserted the data existed.

⚠️ Correction to the issue body. Issue #1533's Context says "last_active … is touched on inbound traffic via get_or_create_chat_link", and its Technical Notes say to wire the increment "adjacent to the existing get_or_create_chat_link / last_active touch". That call site does not exist. Implementing the issue literally would have shipped an increment that never fires for unverified users and left last_active wrong. The scope widening here was reviewed and approved before implementation.

Changes

  • adapters/base.py — new default-no-op record_inbound_activity(message, agent_name) hook on the ChannelAdapter ABC, alongside the existing enrich_message / on_response_sent defaults. Slack and VoIP inherit the no-op (Invariant Fix git pushing bug #9).
  • adapters/message_router.py — calls it once per delivered DM at step 5c, after _enforce_access_policy (so an unauthenticated stranger who messages the bot cannot create unbounded chat-link rows) and skipped for groups (a chat link is DM-keyed; counting group traffic would list members who never DM'd the agent). Wrapped in try/except — a counter write must never block message processing.
  • adapters/telegram_adapter.py, adapters/whatsapp_adapter.py — override the hook.
  • db/telegram_channels.py, db/whatsapp_channels.py — new record_inbound(): one atomic INSERT … ON CONFLICT DO UPDATE replacing get_or_create + increment. This removes a cross-worker SELECT-then-INSERT race (--workers 2; the old get_or_create_chat_link had no IntegrityError catch), halves the writes per message, and refreshes a stale display name via COALESCE(excluded, existing). Because increment_message_count was last_active's only live writer, one call fixes the count and the timestamp together.
  • Dead-code removal — the four now-unused methods, their facade delegations, and the _row_to_chat_link helper only they called. Verified zero callers across src/, tests/, and the private trinity-enterprise submodule.
  • SharingPanel.vue — the Messages header now states counts are not backfilled.
  • Docsarchitecture.md, telegram-integration.md (which documented the deleted methods and claimed message_count was "Incremented per message"), whatsapp-integration.md, plus a learnings.md entry.

No schema change, so no migration on either track.

Test Plan

  • Dual-backend DB tests through the real db_backend harness — roster read-back 0→1→2, last_active advances, username backfills, /login interplay, per-client isolation. Passes on SQLite and real PostgreSQL (on_conflict_do_update portability is the main technical risk; SQLite alone would not have proven it).
  • Real router pipeline (_handle_message_inner) — DM counts; group, access-denied do not; a raising hook still delivers the reply.
  • Adapter override bodies executed for real, and mutation-verified: typo the metadata key to sender_username and the suite goes red. Without this test, that typo ships green.
  • Facade delegation guarded — a wholesale database = MagicMock() stub cannot see a missing/renamed facade method (learnings.md, 2026-07-06).
  • Full tests/unit: 3648 passed. Channel/router suites vs a live backend: 97 passed. The 5 failures reproduce identically on a pristine origin/dev worktree (macOS-Docker pgroup ×2, a Europe/Kiev tz case, 2 live-API anon-session tests). Zero regressions.
  • Verified against a live instance. Real Telegram Update payloads POSTed to the real webhook endpoint with a valid secret token, through the real transport → parse_message → router → hook → adapter → production SQLite:
    • require_email=true, unverified sender → Access denied at the router, no row created (proves the hook sits after the gate)
    • DM Fix: Add missing Docker labels to system agent container #1('alice', 1, …032Z); DM Feature/gemini runtime support #2('alice', 2, …822Z) — one row, no duplicate, last_active advanced
    • Group message (valid @mention) → reaches the router (START channel=-100123456, DONE) and is not counted
    • GET /api/agents/{name}/clients[{"identity":"@alice","message_count":2,…}]

Notes

Fixes #1533

🤖 Generated with Claude Code

webmixgamer and others added 2 commits July 9, 2026 13:32
#1533)

The roster showed `message_count = 0` for every external client, `last_active`
was frozen at `/login` time, and clients who never ran `/login` never appeared
at all. All three share one cause: `get_or_create_chat_link` and
`increment_message_count` (Telegram + WhatsApp) had zero callers outside the
`database.py` facade. The shared inbound path writes `public_chat_sessions`, a
different table, so `*_chat_links` rows were only ever created by
`set_*_verified_email` — the `/login` flow.

Note: the issue's Context paragraph states that `last_active` "is touched on
inbound traffic via `get_or_create_chat_link`". It is not — that function was
dead, and returns early for an existing row without writing anything.

Revive the write path behind a default-no-op `ChannelAdapter.record_inbound_activity`
hook, called once per delivered DM from `ChannelMessageRouter._handle_message_inner`
at step 5c: after the access gate, so an unauthenticated stranger who messages the
bot cannot create unbounded chat-link rows, and skipped for groups, since a chat
link is keyed by (binding, user) and counting group traffic would list members who
never DM'd the agent. Telegram and WhatsApp override it; Slack and VoIP inherit
the no-op (Invariant #9). The call is best-effort — a counter write never blocks
message processing.

Replace `get_or_create` + `increment` with one atomic `INSERT … ON CONFLICT DO
UPDATE` (`record_inbound`), which removes a cross-worker SELECT-then-INSERT race
under `--workers 2`, halves the writes per message, and refreshes a stale display
name via `COALESCE(excluded, existing)`. Since `increment_message_count` was
`last_active`'s only live writer, one call fixes the count and the timestamp
together. Delete the now-dead methods, their facade delegations, and the
`_row_to_chat_link` helper they alone called.

Historical counts are not backfilled; the roster's "Messages" header says so.

Tests: dual-backend (SQLite + PostgreSQL) roster read-back 0->1->2, `last_active`
advance, username backfill, and `/login` interplay through the real `db_backend`
harness; the real `_handle_message_inner` for the DM, group, access-denied and
counter-failure paths; and the adapter override bodies executed for real —
mutation-verified, a typo'd metadata key turns the suite red. A facade-delegation
guard covers the wholesale-mock blindness recorded in docs/memory/learnings.md.

Verified against a live instance: real Telegram webhook payloads through the real
transport drive the roster 0->1->2; a group message reaches the router and is not
counted; an access-denied message creates no row.

Follow-up #1552 records the read-time-derivation reframe (deriving the roster from
public_chat_messages) that this tactical fix deliberately defers.

Fixes #1533

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ess-denied case

The public-repo rule in CLAUDE.md calls for `user@example.com`-style
placeholders. `a@b.com` was copied from the neighbouring access-gate test and
was also inconsistent with the rest of this file, which already uses
`alice@example.com`. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@webmixgamer webmixgamer added the ui PR touches the frontend UI — triggers Playwright e2e tests label Jul 9, 2026

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated via /validate-pr: Fixes #1533. record_inbound() atomic upsert removes a cross-worker SELECT-then-INSERT race; regression test test_1533_client_message_count.py present. All required checks green (incl. schema-parity, pg-migrations). Security scan clean. Approving.

@vybe
vybe merged commit 7174427 into dev Jul 9, 2026
29 of 30 checks passed
@webmixgamer
webmixgamer deleted the feature/1533-client-message-count branch July 10, 2026 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants