Record an outbound once the provider has answered - #78
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: 13b0c72bf9
ℹ️ 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".
| provider_attempted_at_ms=attempted_at_ms, | ||
| reply_to_message_id=reply_to_message_id, | ||
| ) | ||
| outcome = Message( |
There was a problem hiding this comment.
When the same command invocation is retried with an already-used command_id, this in-memory message is not discoverable as the previously persisted outbound because _insert_outbound replaces its deterministic ID with a UUID. The retry therefore reaches _transmit and sends a duplicate to the provider before SQLite rejects the final insert via the unique (agent_id, command_id) index; previously, saving the pending row caused that conflict before delivery. Reserve or check the command ID before making the provider call so request retries cannot duplicate user-visible messages.
Useful? React with 👍 / 👎.
| provider_thread_id=( | ||
| delivery_result.provider_thread_id or address.provider_thread_id | ||
| ), |
There was a problem hiding this comment.
When the initial handle-addressed delivery returns FAILED or UNKNOWN, it has no authoritative provider_thread_id, yet this fallback persists the unopened numeric address as an established session. A later dm:@handle send resolves that session and follows the normal path, which omits delivery_handle; for a Telegram bot chat that can only be opened by username, every retry then uses the unusable numeric route even after a transient outage clears. Avoid treating an unconfirmed opening as established, or retain enough routing state to retry it by handle.
Useful? React with 👍 / 👎.
| delivery_result = await self._delivery.deliver( | ||
| ChannelSendRequest( | ||
| session_id=address.thread_id, | ||
| body=body, | ||
| attachments=attachments, | ||
| target_kind=ChannelTargetKind.DM, | ||
| ) | ||
| # The name this conversation already answers to comes first, then what | ||
| # the provider calls the peer, and only then the token that found it. | ||
| handle = stored_session.target_handle or known.sender.name or raw_target[4:] | ||
| await self._storage.save_channel_session( | ||
| replace( | ||
| stored_session, | ||
| updated_at_ms=now, | ||
| target_handle=handle, | ||
| target_handle_key=handle.casefold(), | ||
| provider_thread_id=address.provider_thread_id, | ||
| delivery_handle=address.delivery_handle, | ||
| ) | ||
| ) | ||
| stored_thread = await self._storage.get_thread(address.thread_id) | ||
| if stored_thread is None: | ||
| stored_thread = ConversationRow( | ||
| now = self._clock() |
There was a problem hiding this comment.
For a slow cold-DM provider call, now is sampled only after _delivery.deliver returns and is then stored as provider_attempted_at_ms, unlike the ordinary delivery path which captures the timestamp before transmission. The durable message therefore records the attempt as starting at completion time, erasing the provider-call duration and producing incorrect delivery history; capture the timestamp immediately before the await.
Useful? React with 👍 / 👎.
| await self._storage.save_channel_session(session) | ||
| await self._storage.save_thread( |
There was a problem hiding this comment.
If another cold send or an inbound DM establishes this conversation while the provider call is in flight, this unconditional save uses newly generated created_at_ms values against the already stored session/thread. The storage validators reject those identity changes, so the command raises after its message was successfully delivered and never records that outbound. Re-resolve or atomically upsert the delivered conversation before persisting the message so concurrent establishment cannot turn a successful send into an untracked failure.
Useful? React with 👍 / 👎.
| AND spoken.seq = ( | ||
| SELECT MAX(latest.seq) FROM messages AS latest | ||
| WHERE latest.agent_id = opened.agent_id | ||
| AND latest.channel = opened.channel | ||
| AND latest.direction = 'inbound' | ||
| AND latest.sender_id IS NOT NULL | ||
| AND LOWER(latest.sender) = opened.target_handle_key | ||
| ) |
There was a problem hiding this comment.
When a handle has been worn by multiple peers, choosing the globally latest inbound speaker does not identify who owned the handle when the historical DM was opened. For example, if peer A received messages through @kana and peer B later acquires that handle and speaks in a group, this migration attributes A's stored private outbound history to B and may merge it into B's numeric conversation. Do not migrate ambiguous reused-handle histories without evidence tying the opened session to the selected sender ID.
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 handle-based session with no numeric counterpart, this updates only provider_thread_id while retaining channel-session and thread IDs derived from the old @handle identity. After restart, a numeric inbound causes Channel.receive to register its provider-session mapping under the numeric-derived local thread ID, but storage resolves the message to the retained handle-derived thread ID; subsequent accept_turn_event calls cannot translate that ID and Telegram's _stream_routes lookup misses, so typing actions disappear for turns triggered by these migrated DMs. Preserve an alias for the retained ID or migrate the local identity consistently.
Useful? React with 👍 / 👎.
13b0c72 to
6684363
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6684363bbd
ℹ️ 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".
| outcome = Message( | ||
| direction=MessageDirection.OUTBOUND, | ||
| seq=0, | ||
| message_id=f"outbound-{target_id}-{command_id}", |
There was a problem hiding this comment.
For every ordinary resolved-target send, this assigns a transient deterministic message_id, and _deliver builds audit_context from it before finalization; SQLite's _insert_outbound then replaces that ID with a UUID. Consequently the pending and terminal audit records point to an outbound message that does not exist in storage, breaking message-to-audit correlation. Preserve this ID during insertion or rebuild the terminal correlation from the finalized outbound.
Useful? React with 👍 / 👎.
| if delivery_state not in _DELIVERED_STATES: | ||
| return MessageSendSuccess(message=outbound, target=raw_target) |
There was a problem hiding this comment.
When an unopened dm:@handle attempt returns FAILED or UNKNOWN, this early return bypasses _record_delivery, so neither the channel outcome nor the bcc.message.send tool event is written to the audit sink. This makes precisely the transient failures that callers may retry invisible to operational history; retain the no-conversation persistence behavior, but record the terminal attempt before returning.
Useful? React with 👍 / 👎.
| try: | ||
| # the peer answers under an id of its own choosing, which is not the one | ||
| # this node would have guessed | ||
| channel.delivered_thread_ids["test:dm:peer-1"] = "test:dm:chat-9" |
There was a problem hiding this comment.
This test manufactures the provider acknowledgement by assigning TestChannel.delivered_thread_ids and then runs against MemoryStorage, so it can pass without exercising the external channel behavior that supplies the authoritative thread ID. Repository guidance prohibits fake/mock coverage for external dependencies and requires those cases to be real e2e tests; move this scenario to the real Telegram e2e suite rather than simulating the acknowledgement.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| assert ( | ||
| TelegramOutboundChannel._acknowledged_thread_id( | ||
| {"message_id": 5, "chat": {"id": 7, "type": "private"}}, opened_by_name | ||
| ) | ||
| == "telegram:1:7:0" |
There was a problem hiding this comment.
This newly added unit test asserts the private helper's complete serialized thread ID exactly, coupling the test to representation details rather than validating the observable delivery-routing behavior. Replace it with permitted behavioral coverage that demonstrates the acknowledged numeric chat is subsequently used; the repository explicitly prohibits exact assertions.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
6684363 to
d51af58
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d51af58b2d
ℹ️ 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 ( | ||
| command_id in self._sending | ||
| or await self._storage.has_delivered_outbound_for_command(command_id) | ||
| ): | ||
| raise ValueError(f"command was already sent: {command_id}") | ||
| self._sending.add(command_id) |
There was a problem hiding this comment.
When two sends with the same command_id enter concurrently, both can observe that _sending does not contain the ID and then suspend in has_delivered_outbound_for_command before either reaches _sending.add; both subsequently transmit, and only the final database uniqueness check rejects one after duplicate delivery. The fresh evidence is that the new in-memory guard performs its reservation after an await, so make the check-and-add atomic before the first suspension point.
Useful? React with 👍 / 👎.
| # a chat that was never opened is not a conversation, and writing one | ||
| # down would leave `dm:@name` resolving to something that cannot be | ||
| # spoken to; what became of the attempt is still worth recording | ||
| if delivery_state not in _DELIVERED_STATES: | ||
| await self._record_delivery( |
There was a problem hiding this comment.
When a cold multipart Telegram send confirms an early part and a later part raises TelegramTransportError, _unknown reports UNKNOWN even though its receipt contains confirmed parts. This branch consequently stores neither the opened session nor the outbound, and the same command can be retried, duplicating those confirmed parts while permanently omitting them from history; treat receipt-confirmed unknown outcomes as opened/partial or persist sufficient delivery and routing evidence.
Useful? React with 👍 / 👎.
| assert named["channel-kana-chat"] == "telegram:1:7:0" | ||
| assert named["channel-mika-handle"] == "telegram:1:8:0" |
There was a problem hiding this comment.
This migration test directly asserts exact serialized provider identities and internal row IDs, coupling coverage to storage representation rather than observable migration behavior. Replace these exact comparisons with permitted behavioral coverage of the migrated conversation and routing outcome.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| await self._storage.save_channel_session(session) | ||
| await self._storage.save_thread( |
There was a problem hiding this comment.
If the process exits or save_thread fails after save_channel_session commits, the database retains a channel session with no thread. A later dm:@handle retry finds that row through find_channel_session, skips _open_dm, and then resolve_inbox_target(held.canonical_target) fails permanently because nothing recreates the missing thread; save the session, thread, and outbound in one storage transaction so no unrecoverable intermediate mapping is visible.
Useful? React with 👍 / 👎.
d51af58 to
42498aa
Compare
|
@codex review |
42498aa to
0d4f9f1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
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>
0d4f9f1 to
e6e5ebe
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6e5ebea38
ℹ️ 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".
|
|
||
| address = opening.address | ||
| attempted_at_ms = self._clock() | ||
| delivery_result = await self._delivery.deliver( |
There was a problem hiding this comment.
When a client supplies a created_at_ms later than the node clock, this provider call completes before the outbound Message is constructed; that constructor then rejects provider_attempted_at_ms < created_at_ms. A successful cold DM is therefore delivered but neither persisted nor reserved by command_id, so retrying the reported failure can duplicate the message. Validate this client field at the Pydantic command boundary before delivery, as the repository requires for input validation.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
| DELETE FROM threads WHERE id IN (SELECT opened_thread_id FROM bcn_merged_dms) | ||
| """, | ||
| """ | ||
| DELETE FROM channel_sessions WHERE id IN (SELECT opened_id FROM bcn_merged_dms) |
There was a problem hiding this comment.
When the numeric DM predates migration v22, its target_handle is null; if the later handle-addressed implementation created a second row for that peer, this merge deletes the only row carrying the handle without copying it to the retained numeric session. After migration, direct operations such as message read --target dm:@name and thread unfollow --target dm:@name no longer resolve until another inbound message refreshes the numeric session. Copy the opened row's handle fields when the retained row has none before deleting it.
Useful? React with 👍 / 👎.
| return ( | ||
| delivery_result.state in _DELIVERED_STATES | ||
| or delivery_result.provider_message_id is not None | ||
| or delivery_result.provider_thread_id is not None |
There was a problem hiding this comment.
When a multipart Lark or WeCom send confirms an early part and a later transport attempt becomes UNKNOWN, this predicate returns false because those adapters expose the evidence as receipt["confirmed_parts"] and nested part receipts rather than a top-level provider_message_id or provider_thread_id. The outbound is consequently not persisted, and retrying the same command_id duplicates the already confirmed parts. Fresh evidence beyond the earlier Telegram case is that contrib/lark/outbound.py::_delivery_receipt and WeCom's _Delivery.receipt retain confirmed-part counts while this provider-neutral check ignores them.
Useful? React with 👍 / 👎.
e6e5ebe to
732dbce
Compare
|
@codex review |
732dbce to
e6e5ebe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 732dbce0e7
ℹ️ 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 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.
When one peer was contacted under two successive handles and never created a numeric session, both handle-backed rows remain in bcn_renamed_dms because _MERGED only detects an already-existing numeric row. This update then assigns both rows the same numeric provider_thread_id; subsequent find_channel_session calls see multiple matches and raise multiple rows violate channel provider identity, breaking inbound processing and further sends for that DM. Coalesce rows that resolve to the same peer identity before applying the rename.
Useful? React with 👍 / 👎.
1 participant
An outbound message was written before it was attempted and corrected afterwards. It is now written once, after the provider answers, carrying whatever became of it. A process that dies between the send and the write leaves a message the peer has and this node does not, which is cheaper than recording something that never happened.
That order is what a DM to a peer this node has never spoken to needs. Such a chat cannot be reached by the id it will answer to until it exists, only by a name, and a name is not an identity: storing one splits the conversation in half, with what was sent under one row and what came back under another. So the message goes out first, addressed by the name the channel says opens it, and the conversation is written down afterwards under the id the delivery reports. Nothing about delivery is kept on the row, so there is no name to retire later.
Telegram names a DM by the peer's own id for people and bots alike and offers a username only as what opens an unopened chat. The migration renames the conversations stored under a handle before this: the peer's id comes from what it said, since an inbound message carries both the handle it spoke under and the id it spoke from, and where the peer already has a conversation of its own the two are merged. A merged cursor is held below the lowest message either side still had waiting.