Reach one peer's DM by every name it answers to - #76
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edf88e6562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| UPDATE consumer_cursors SET delivered_through_seq = MAX( | ||
| delivered_through_seq, | ||
| COALESCE(( | ||
| SELECT losing.delivered_through_seq | ||
| FROM consumer_cursors AS losing | ||
| JOIN pairs ON pairs.loser_thread_id = losing.thread_id | ||
| WHERE pairs.winner_thread_id = consumer_cursors.thread_id | ||
| ), 0) |
There was a problem hiding this comment.
When the losing thread has a higher global delivered_through_seq than the winner while the winner still contains an unread lower-sequence message, taking MAX advances the winner past that unread message. After the threads are merged, list_unread_messages filters on message.seq > delivered_through_seq, so that message is silently considered delivered and never reaches the runtime; the migration must avoid advancing either thread over messages that were unread in that thread.
Useful? React with 👍 / 👎.
| if stored_session is None: | ||
| stored_session = ChannelSession( | ||
| id=address.channel_session_id, | ||
| channel=known.channel, | ||
| provider_thread_id=address.provider_thread_id, | ||
| provider_thread_id=address.provider_thread_ids[0], |
There was a problem hiding this comment.
For a Telegram bot, provider_thread_ids contains the username route first and its stable numeric ID second, but a newly minted row persists only the username. If that username changes, even only in case, before the first private reply, the inbound candidates contain the new username and the same numeric ID, neither of which matches the stored old username; _settle_provider_thread_id therefore misses the row and record_inbound creates the same split conversation this change is intended to prevent. Persist the stable alternate association rather than discarding every identity after index zero.
Useful? React with 👍 / 👎.
| AND winner.target_handle_key = loser.target_handle_key | ||
| AND winner.id <> loser.id | ||
| AND LOWER(winner.provider_thread_id) | ||
| NOT LIKE '%:@' || winner.target_handle_key || ':%' |
There was a problem hiding this comment.
Matching two rows by target_handle_key does not prove that they belong to the same Telegram peer because usernames can be renamed and reassigned. If a handle-keyed conversation belongs to the former owner and a numeric conversation records the new owner under that same handle, these predicates classify them as a pair; the later statements then combine their private message histories and reminders and delete one conversation. Require evidence involving the stable numeric peer identity before performing this destructive merge.
Useful? React with 👍 / 👎.
| candidates = tuple( | ||
| dict.fromkeys((message.provider_thread_id, *address.provider_thread_ids)) | ||
| ) | ||
| if len(candidates) == 1: | ||
| return message | ||
| stored = await self._storage.find_channel_session( | ||
| channel=message.channel, | ||
| provider_thread_ids=candidates, |
There was a problem hiding this comment.
When the database contains both the exact numeric session for this inbound message and another stale session under one of the supplied aliases, querying all candidates together makes _fetch_one_or_conflict raise instead of selecting the exact route. This can occur with an unmerged multi-row split or a reassigned username, and every subsequent inbound message for the otherwise unambiguous numeric chat is rejected; look up message.provider_thread_id first and consult aliases only when that exact identity is absent.
Useful? React with 👍 / 👎.
| if stored is None or stored.provider_thread_id == message.provider_thread_id: | ||
| return message | ||
| return replace(message, provider_thread_id=stored.provider_thread_id) |
There was a problem hiding this comment.
When a numeric Telegram inbound is settled onto a conversation opened under @username, this replacement makes the durable turn use the handle-derived thread ID. The wrapper consequently maps runtime events to the handle-derived provider session, while TelegramChannel._stream_routes registered only the numeric session seen in _handle_message; accept_turn_event cannot find the route and silently suppresses typing activity for every turn in the newly unified conversation. Register the numeric inbound route for the settled thread or otherwise preserve that route when forwarding turn events.
Useful? React with 👍 / 👎.
| assert [ | ||
| (row["thread_id"], row["delivered_through_seq"]) for row in cursors | ||
| ] == [("thread-kana-chat", 1)] | ||
| assert reminder is not None | ||
| assert reminder["owner_thread_id"] == "thread-kana-chat" |
There was a problem hiding this comment.
The new migration test locks exact thread IDs, provider identity text, cursor tuples, and reminder ownership values rather than expressing only the business behavior under test, so harmless representation changes will fail it even when the merge remains correct. Replace these exact equality checks with behavioral verification of conversation consolidation, unread preservation, and reminder reachability as required by the repository testing rule.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
1 participant
A Telegram peer that holds a username is addressable two ways, and the channel session id is a uuid5 over the identity string that names it. A DM opened by
@usernameand the same DM the peer speaks in under its numeric chat id therefore hashed to two different conversations: outbound landed in one, the reply in the other, and a seconddm:@handlesend failed as ambiguous because both rows answered to the handle.A channel now offers every identity one peer answers to, and the stored row settles which one this conversation kept. The candidate set stays local to the two places that resolve it - the inbound path in the orchestrator and minting in the command service - so a single provider thread id travels from there on.
The second commit merges the conversations an earlier release split. Its pairing condition is narrow: exactly two DM rows under one handle, one keyed by that very handle and one not. Two different peers who share a display name are left alone, since neither of their rows is keyed by that name. Messages, thread, reminder and cursor move onto the surviving row; a message keeps the provider thread id it was addressed under, because that is a fact about the message rather than about the conversation.