Name a DM by the conversation the provider delivered into - #77
Conversation
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: ec92bb52e6
ℹ️ 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".
| WHERE opened.target_kind = 'dm' | ||
| AND opened.target_handle IS NOT NULL | ||
| AND INSTR(opened.provider_thread_id, ':@' || opened.target_handle || ':') > 0 | ||
| GROUP BY opened.agent_id, opened.id |
There was a problem hiding this comment.
When a Telegram username has appeared with more than one sender_id, this join returns multiple candidates but the GROUP BY selects an arbitrary identity. Since _resolve_or_mint uses the newest matching sender, an @kana session minted for the newer account can be migrated to an older account, causing its messages and reminders to be merged into the wrong peer thread and the original thread to be deleted. Select the applicable sender deterministically, such as by ranking matching messages by descending sequence before constructing peer_thread_id.
Useful? React with 👍 / 👎.
| await self._storage.rebind_channel_session( | ||
| channel_session.id, | ||
| provider_thread_id=delivered, | ||
| updated_at_ms=self._clock(), |
There was a problem hiding this comment.
If an inbound DM under the numeric provider ID is persisted while the outbound Telegram request is still awaiting its acknowledgement, record_inbound creates the numeric session first and this blind rebind then gives the handle-based session the same provider identity. The provider-identity index is non-unique, so the duplicate persists; subsequent find_channel_session calls reject the multiple rows and the conversation remains split. The rebind operation must atomically detect and merge/adopt an existing destination session rather than only updating this row.
Useful? React with 👍 / 👎.
| CASE | ||
| WHEN peer.target_handle IS NOT NULL THEN 'dm:@' || peer.target_handle | ||
| ELSE 'dm:' || peer.id | ||
| END AS peer_target |
There was a problem hiding this comment.
When two halves are merged and the retained peer has a handle, this stores moved messages under dm:@<handle> instead of the retained channel session's canonical dm:<session-id> target. read_message_history resolves the handle and then filters messages by target.canonical_target, so every moved outbound message is omitted from the merged conversation's history even though its thread_id was updated. Set peer_target to the retained session's canonical target; the handle should remain only a presentation alias.
Useful? React with 👍 / 👎.
| JOIN channel_sessions AS peer | ||
| ON peer.agent_id = renamed.agent_id | ||
| AND peer.provider_thread_id = renamed.peer_thread_id |
There was a problem hiding this comment.
If another channel has the same opaque provider_thread_id, this join can select that channel's session because it matches only agent_id and provider identity, even though the repository defines provider identity using (agent_id, channel, provider_thread_id). The migration then moves messages into the other channel's thread while leaving their channel column unchanged and deletes the original DM session. Add a same-channel predicate when selecting peer.
Useful? React with 👍 / 👎.
| return ChannelDeliveryReceipt( | ||
| provider_message_id=confirmed_ids[0], | ||
| provider_receipt_ref=( | ||
| confirmed_ids[-1] if len(confirmed_ids) > 1 else None | ||
| ), | ||
| provider_thread_id=provider_thread_id, |
There was a problem hiding this comment.
After a first send by @username, _outbound_route has cached the handle-based identity under the local session ID, but the newly extracted numeric identity is only returned in the receipt and never replaces that cache entry. A later numeric inbound update is cached under its numeric-derived session ID, while orchestration continues emitting runtime events with the retained handle-derived local session ID, so typing actions keep targeting the old username. If that username changes or is reassigned, typing fails or is sent to the wrong chat; update _stream_routes for the request's session when the acknowledgement supplies the authoritative identity.
Useful? React with 👍 / 👎.
| try: | ||
| # This peer has only spoken in a group, so its DM does not exist yet and | ||
| # can only be opened by the name it answers to. | ||
| channel.delivered_thread_ids["test:dm:@kana"] = "test:dm:peer-1" |
There was a problem hiding this comment.
This test makes the required provider behavior true by programming TestChannel.delivered_thread_ids, so it cannot detect the Telegram adapter failing to extract chat.id from an actual send acknowledgement, including the rich-message and document paths changed here. Since the behavior depends on an external provider response, cover it through the real Telegram e2e instead of treating a configurable fake receipt as verification.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
ec92bb5 to
05b040f
Compare
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b040f8a9
ℹ️ 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".
| if request.delivery_handle is not None: | ||
| identity = replace(identity, chat_id=f"@{request.delivery_handle}") |
There was a problem hiding this comment.
After a newly minted bot DM succeeds, the session still retains delivery_handle, and _adopt_delivered_thread_id returns without changing anything because the acknowledgement's numeric ID already equals the stored numeric ID. Consequently every later send enters this branch and replaces the stable numeric chat ID with @username; if the bot renames that username, the conversation starts failing, and if the name is reassigned, messages can be delivered to the new owner. Clear the one-time handle after acknowledgement or use it only until the numeric route has been confirmed.
Useful? React with 👍 / 👎.
| UPDATE channel_sessions SET provider_thread_id = ( | ||
| SELECT peer_thread_id FROM bcn_renamed_dms | ||
| WHERE bcn_renamed_dms.opened_id = channel_sessions.id | ||
| ) | ||
| WHERE id IN (SELECT opened_id FROM bcn_renamed_dms) |
There was a problem hiding this comment.
For a pre-v27 handle session left behind by a failed or unconfirmed first send, this migration can infer the peer's numeric ID from a group message and rename the session even though the private chat was never opened. Because it does not also preserve the old username as delivery_handle, the next send uses the numeric route rather than the username required to open that chat and can no longer retry successfully. Record the delivery handle during this rename or restrict it to sessions whose private delivery was confirmed.
Useful? React with 👍 / 👎.
| def _delivery_handle(channel_session: ChannelSession) -> str | None: | ||
| """The name that opens this chat, for as long as its id cannot.""" | ||
|
|
||
| handle = channel_session.metadata.get("delivery_handle") | ||
| return handle if isinstance(handle, str) and handle else None |
There was a problem hiding this comment.
_delivery_handle has exactly one call site and only unwraps one metadata value with a type/emptiness check, so it adds an extra indirection without defining a reusable abstraction. Inline this lookup where ChannelSendRequest is constructed, as the repository explicitly prohibits meaningless helper fragments used only once.
AGENTS.md reference: AGENTS.md:L2-L2
Useful? React with 👍 / 👎.
05b040f to
4dfbf46
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dfbf464b0
ℹ️ 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".
| await self._storage.save_channel_session( | ||
| replace( | ||
| channel_session, |
There was a problem hiding this comment.
When a handle resolves to a different provider thread ID and no destination session exists, the rebind at line 468 changes the stored identity, but this subsequent save still passes the pre-rebind channel_session. Both the SQLite and in-memory repositories reject that as an attempt to change an immutable provider identity, so the command raises after Telegram has already delivered the message and the outbound remains unfinalized. Reload the rebound session, or combine the rebind and metadata removal into one storage operation.
Useful? React with 👍 / 👎.
| if ( | ||
| delivery_result.state is not OutboundDeliveryState.SENT | ||
| or "delivery_handle" not in channel_session.metadata |
There was a problem hiding this comment.
For a multipart Telegram send where at least one part is confirmed before a later part fails, the PARTIAL result already contains the acknowledged numeric thread ID, but this SENT-only condition retains delivery_handle. The current code fixes confirmed sends, but every later message after this partial delivery is still routed through @username; if that username changes or is reassigned, delivery can fail or reach another peer even though the stable numeric route is known.
Useful? React with 👍 / 👎.
4dfbf46 to
008d542
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 008d542670
ℹ️ 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".
| def delivered_thread_id(self, provider_thread_id: str) -> str: | ||
| """Say which conversation a send landed in, the way a provider does.""" | ||
|
|
||
| return self.delivered_thread_ids.get(provider_thread_id, provider_thread_id) |
There was a problem hiding this comment.
delivered_thread_id has exactly one call site and merely forwards to dict.get, so it adds an unnecessary indirection without defining a reusable abstraction. Inline this lookup when constructing ChannelDeliveryReceipt, as the repository explicitly prohibits meaningless helper fragments used only once.
AGENTS.md reference: AGENTS.md:L2-L2
Useful? React with 👍 / 👎.
| # the conversation the provider says this landed in, which is the only | ||
| # authority on a chat that was addressed by a name rather than by its id | ||
| provider_thread_id: str | None = None |
There was a problem hiding this comment.
The relevant plans/2026-09-05-dm-address-resolution.md still specifies @username as the Telegram bot DM identity and does not describe acknowledgement-derived provider identities, storage rebinding, or the v27 data migration introduced here. Because this is a material new identity flow rather than an implementation detail from that plan, it needs the required researched implementation plan and review sequence before landing.
AGENTS.md reference: AGENTS.md:L4-L6
Useful? React with 👍 / 👎.
| assert [(row["id"], row["provider_thread_id"]) for row in sessions] == [ | ||
| ("channel-group", "telegram:1:-100:0"), | ||
| ("channel-kana-chat", "telegram:1:7:0"), | ||
| ("channel-mika-handle", "telegram:1:8:0"), | ||
| ] |
There was a problem hiding this comment.
This assertion requires the complete ordered set of session IDs and provider identities to match an exact snapshot, rather than checking only the migration invariants under test. That makes unrelated fixture additions or harmless ordering changes fail this test, and the repository explicitly prohibits exact assertions; assert the required retained, removed, and rebound properties independently instead.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
d455d89 to
a8792e0
Compare
|
@codex review |
a8792e0 to
3a613ba
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8792e0dc9
ℹ️ 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".
| '$.delivery_handle', | ||
| target_handle |
There was a problem hiding this comment.
When an unmerged legacy handle session already contains a sent or partial outbound but has never received a DM reply, it still lands in bcn_renamed_dms; this unconditional JSON_SET reinstates delivery_handle even though a confirmed part already opened the numeric chat. If that username has since changed or been reassigned, the next outbound overrides the safe numeric ID with the stale handle and can fail or reach the wrong peer. The fresh issue is that the newly added assignment should be conditional on the session having no confirmed delivery.
Useful? React with 👍 / 👎.
| await self._storage.save_channel_session( | ||
| replace( | ||
| channel_session, |
There was a problem hiding this comment.
If an inbound reply is persisted while the provider send is awaiting acknowledgement, record_inbound updates this same numeric session before execution resumes here. Saving the pre-send channel_session then overwrites the reply's last_inbound_at_ms and target presentation with stale values while merely trying to remove one metadata key, which can leave an obsolete handle bound to the conversation. Reload the row or perform a narrow atomic metadata update before saving.
Useful? React with 👍 / 👎.
| delivery_result.state | ||
| in (OutboundDeliveryState.SENT, OutboundDeliveryState.PARTIAL) |
There was a problem hiding this comment.
For a multipart handle-routed send, an earlier part can be confirmed before a later part raises TelegramTransportError; _unknown retains the confirmed receipt entry but returns no typed provider_thread_id. Because this condition excludes UNKNOWN, the handle remains even though the confirmed part proves the numeric chat is open, so later sends continue through a renameable or reassigned username. Treat a confirmed receipt prefix as sufficient to clear the handle and adopt its acknowledged thread ID even when the overall delivery outcome is unknown.
Useful? React with 👍 / 👎.
| UPDATE channel_sessions SET | ||
| provider_thread_id = ( | ||
| SELECT peer_thread_id FROM bcn_renamed_dms | ||
| WHERE bcn_renamed_dms.opened_id = channel_sessions.id |
There was a problem hiding this comment.
For an unmerged legacy DM, this changes the provider identity in place but leaves its handle-derived local channel-session ID unchanged. If the peer later appears in a group under a new username before replying in the DM, _resolve_or_mint derives the numeric UUID, fails to find the migrated row by that ID, and then attempts to insert a second row with the same provider identity; save_channel_session rejects the duplicate, so dm:@newname cannot be sent. Fall back to locating the existing session by (channel, provider_thread_id) before creating it.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3a613ba to
0db548f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
A chat that does not exist yet can only be opened by the name it answers to, so a DM to a bot went out addressed by username and was stored under that name. The peer then spoke under its own chat id, which matched nothing, and one conversation became two: the outbound in one, the reply in the other, and a later send by handle rejected as ambiguous.
A send acknowledgement always names the conversation it landed in. Telegram now reads that id out of the acknowledgement and reports it on the delivery receipt, and a conversation whose stored name differs adopts it. Nothing distinguishes a bot from a person or a first message from a later one; the provider's own answer is the only authority. Storage gained a narrow rebind operation for this, because changing a conversation's provider identity stays forbidden everywhere else.
The migration names conversations that were stored before this. A peer's id is recoverable from what it said, since an inbound message carries both the handle it spoke under and the id it spoke from: where that peer already has a conversation of its own the two are merged, and where it does not the existing one is renamed. The merged cursor is held below the lowest message either side still had waiting, rather than taking the larger of the two, which would mark those messages delivered.