Skip to content

fix(telegram): preserve forum topic thread in last-route delivery#53052

Merged
obviyus merged 5 commits intoopenclaw:mainfrom
VACInc:vacinc
Mar 25, 2026
Merged

fix(telegram): preserve forum topic thread in last-route delivery#53052
obviyus merged 5 commits intoopenclaw:mainfrom
VACInc:vacinc

Conversation

@VACInc
Copy link
Contributor

@VACInc VACInc commented Mar 23, 2026

Summary

Fix Telegram forum-topic routing for async and session follow-up replies.

Previously, inbound Telegram messages only updated last-route delivery metadata for non-group chats. Forum-topic messages in Telegram supergroups therefore never persisted their topic threadId in the delivery context. When a later async completion or yielded follow-up replied, it had the group chat id but not the topic id, so the reply fell back to General instead of the originating topic.

Root Cause

extensions/telegram/src/bot-message-context.session.ts only updated updateLastRoute under !isGroup, which meant forum-topic messages in group chats skipped last-route thread tracking entirely.

What Changed

  • derive updateLastRouteThreadId from resolvedThreadId for group/forum messages
  • keep using dmThreadId for DM topics
  • allow updateLastRoute for:
    • non-group chats, as before
    • group/forum chats when a thread id is present
  • keep mainDmOwnerPin DM-only

Why This Fix Works

The async reply path already knows how to send Telegram replies with a thread id. The missing piece was persisting the forum-topic thread id into the session last-route delivery context on inbound messages. With this change, later follow-up replies retain topic affinity instead of dropping into General.

Tests

  • pnpm test -- extensions/telegram/src/bot-message-context.dm-topic-threadid.test.ts
  • pnpm test -- extensions/telegram/src/bot-message-context.dm-threads.test.ts

Notes

This is a narrow routing fix, not a broader Telegram delivery refactor.

@openclaw-barnacle openclaw-barnacle bot added channel: telegram Channel integration: telegram size: S labels Mar 23, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 23, 2026

Greptile Summary

This PR fixes a Telegram forum-topic routing bug where async and session follow-up replies were not preserving the originating forum topic threadId. Previously, updateLastRoute was only set for non-group chats, so forum-topic messages in supergroups never persisted their thread ID into the last-route delivery context — causing later replies to fall back to General instead of the originating topic.

Changes:

  • bot-message-context.session.ts: Introduces updateLastRouteThreadId, derived from resolvedThreadId (forum-scoped, only set when isForum === true) for groups, and from dmThreadId for DMs. The updateLastRoute guard is widened from !isGroup to !isGroup || updateLastRouteThreadId != null, enabling last-route persistence for forum topics while leaving non-forum group messages (and mainDmOwnerPin) unchanged.
  • bot-message-context.dm-topic-threadid.test.ts: Updates the existing group-message test to use is_forum: true and asserts that updateLastRoute is populated with the correct threadId.

The implementation is correct. resolvedThreadId is only ever non-null for forum groups (gated by isForum), so non-forum supergroups are unaffected. The General topic edge case (where resolvedThreadId = 1) is handled safely downstream: buildTelegramThreadParams explicitly strips message_thread_id=1 before any Telegram API call. A minor test coverage gap exists for the General topic (forum group with no message_thread_id) — worth adding a case to confirm the stored threadId: "1" and API-level omission behavior.

Confidence Score: 4/5

  • Safe to merge; the fix is targeted and correct, with one minor P2 test coverage gap for the General forum topic edge case.
  • The logic change is provably correct: resolvedThreadId is only set for isForum === true groups (enforced by resolveTelegramForumThreadId), the mainDmOwnerPin guard correctly stays DM-only, and the General topic id=1 is safely stripped at the API layer by buildTelegramThreadParams. The only gap is that the General forum topic scenario (no message_thread_id in a forum group) is untested in the updated test file, which is a non-blocking P2 suggestion.
  • No files require special attention beyond the optional test coverage addition in bot-message-context.dm-topic-threadid.test.ts.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/telegram/src/bot-message-context.dm-topic-threadid.test.ts
Line: 75-93

Comment:
**Missing test coverage for General forum topic (id=1)**

The updated test verifies the forum-topic path when `message_thread_id` is present (id=99), but there is no test for a forum group message that arrives in the **General topic** (i.e., `is_forum: true` with no `message_thread_id`).

In that case, `resolveTelegramForumThreadId` returns `TELEGRAM_GENERAL_TOPIC_ID = 1`, so `resolvedThreadId = 1` and the new logic sets `updateLastRoute` with `threadId: "1"`. This is a new behavior — previously `updateLastRoute` was `undefined` for all group messages. While the API layer correctly strips `message_thread_id=1` before sending (via `buildTelegramThreadParams`), the effect of persisting `threadId: "1"` in the delivery context is now untested.

Consider adding a test case like:

```ts
it("sets updateLastRoute with threadId '1' for forum General topic (no message_thread_id)", async () => {
  const ctx = await buildCtx({
    message: {
      chat: { id: -1001234567890, type: "supergroup", title: "Test Forum", is_forum: true },
      text: "@bot hello",
      // no message_thread_id → General topic
    },
    options: { forceWasMentioned: true },
    resolveGroupActivation: () => true,
  });
  expect(ctx).not.toBeNull();
  const updateLastRoute = getUpdateLastRoute() as { threadId?: string; to?: string } | undefined;
  expect(updateLastRoute).toBeDefined();
  expect(updateLastRoute?.to).toBe("telegram:-1001234567890");
  expect(updateLastRoute?.threadId).toBe("1"); // General topic stored but stripped at API layer
});
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "style(telegram): format last-route updat..." | Re-trigger Greptile

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a35753dc9a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +279 to +283
sessionKey: updateLastRouteSessionKey,
channel: "telegram",
to: `telegram:${chatId}`,
accountId: route.accountId,
threadId: updateLastRouteThreadId,

Choose a reason for hiding this comment

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

P2 Badge Skip topic last-route writes for sessions shared by multiple topics

When more than one Telegram topic is bound to the same targetSessionKey via configured ACP bindings or runtime thread bindings, this now stores each inbound topic's threadId on that shared session. Later implicit sends resolve from the mutable session delivery context (src/infra/outbound/targets.ts:109-118), so a completion started from topic 42 can be delivered into topic 99 after any message there updates the route. That shared-session setup is supported (src/channels/plugins/binding-routing.ts:60-80, extensions/telegram/src/thread-bindings.ts:606-612), so forum-topic traffic can now overwrite each other's destination instead of staying pinned to the originating topic.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think this applies to the code path this PR changes.

This PR is about async/session follow-up replies. That path does not resolve from mutable session deliveryContext at send time...it captures the originating thread when the follow-up is queued in src/auto-reply/reply/get-reply-run.ts:519 and then routes with queued.originatingThreadId in src/auto-reply/reply/followup-runner.ts:101. The embedded follow-up run is also seeded from the same queued origin in src/auto-reply/reply/followup-runner.ts:182. The specific scenario described here would not affect the follow-up path this PR is fixing.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93b7cd6c1b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +276 to +283
updateLastRoute:
!isGroup || updateLastRouteThreadId != null
? {
sessionKey: updateLastRouteSessionKey,
channel: "telegram",
to: `telegram:${chatId}`,
accountId: route.accountId,
threadId: updateLastRouteThreadId,

Choose a reason for hiding this comment

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

P2 Badge Avoid persisting topic thread IDs onto shared session routes

Fresh evidence beyond the earlier async-followup concern: even though queued followups carry ctx.MessageThreadId, session-scoped senders like cron and exec approvals still resolve from persisted deliveryContext (src/cron/isolated-agent/delivery-target.ts:66-99, src/infra/exec-approval-session-target.ts:51-68). Telegram forum topics can legitimately share one target session via the parent-peer/binding flow in extensions/telegram/src/conversation-route.ts:46-60, so this new branch will let any inbound topic overwrite that shared session’s stored threadId. In that setup, a later cron run or approval prompt for the same session can be delivered into whichever topic most recently had traffic, instead of the topic the session owner expects.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a broader pre-existing shared-session routing issue, not the bug this PR is targeting right now. I'm happy to address it if the maintainers will have it though.

@obviyus obviyus merged commit 89b7fee into openclaw:main Mar 25, 2026
37 of 40 checks passed
@obviyus
Copy link
Contributor

obviyus commented Mar 25, 2026

Landed on main.

Thanks @VACInc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: telegram Channel integration: telegram size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants