Skip to content

feat(telegram): interactive /sessions picker with IDOR-safe state - #89356

Open
nothingness0db wants to merge 1 commit into
NousResearch:mainfrom
nothingness0db:feat/telegram-sessions-picker
Open

feat(telegram): interactive /sessions picker with IDOR-safe state#89356
nothingness0db wants to merge 1 commit into
NousResearch:mainfrom
nothingness0db:feat/telegram-sessions-picker

Conversation

@nothingness0db

@nothingness0db nothingness0db commented Aug 18, 2026

Copy link
Copy Markdown

Summary

Replace the numbered text reply for /sessions\ on Telegram with an inline-keyboard picker. Each session is a button; tapping it resumes that session directly. The currently-active session is filtered out so the list only shows resumable targets. Pickers paginate when there are more than 8 sessions (\◀ Prev\ / \1/N\ / \Next ▶), and a \✗ Cancel\ button dismisses the picker. Buttons render the title on its own line and the first-message preview on a second line (truncated to 40 chars, 64-char Telegram cap).
图片

Built on the same picker state pattern as the existing model picker / choice picker / approval picker — but with the security-aware fixes that the blocker sweep on #43695 and #49038 flagged:

  1. IDOR guard at the runner boundary. The picker's \on_session_selected\ callback re-runs _resume_target_allowed\ with the captured \SessionSource\ before delegating to the shared _resume_session_by_id\ helper. A co-member in a shared group cannot tap a button to bind to another user's persisted session — same gate the text /resume \ path uses. The adapter also re-checks via _is_callback_user_authorized\ for a cheap fail-closed layer.

  2. Collision-safe state key. State is keyed by (chat_id, msg_id, thread_id)\ instead of \chat_id\ alone. A second /sessions\ opened in the same chat (forum threads, /sessions called twice in a row) cannot overwrite the first picker's state, and a stale click on the old keyboard after a new /sessions\ has replaced it is rejected at the adapter before the runner is invoked.

  3. Session-switch via the funnel. _resume_session_by_id\ uses \�sync_session_store.switch_session\ and calls _release_running_agent_state\ + _clear_conversation_scope\ + _evict_cached_agent\ — the same funnel that fixed the bug-class regressions Fix gateway /resume leaking cached agent state across session switches #10702, /new doesn't reset model config — old session keeps using stale model after config change #58403, feat(memory): notify memory providers on in-chat /resume and /branch session switches #6672. The text /resume\ path still uses the inline switch logic (kept distinct to preserve the Matrix --cross-room\ branch which needs source-object-aware title substitution).

  4. Origin-scoped listing. The picker receives only the rows the runner already filtered through _resume_row_visible\ + _resume_target_allowed\ — same scope the text list uses, so the picker cannot bypass the IDOR guard that the listing already enforces. The picker branch lifts the legacy 10-cap (text fallback still caps at 10) so the picker can paginate through the full origin-scoped list (up to 50).

  5. Authorization gate at the adapter. Mirrors the approval / choice picker pattern: a co-member tap is rejected at the Telegram adapter before the runner callback runs.

Tests

\
tests/gateway/test_telegram_sessions_picker.py — 16 tests (adapter state, IDOR, pagination, collision, current-session filter)
tests/gateway/test_sessions_command_picker_integration.py — 6 tests (runner ↔ adapter wiring, fallback, full-list cap)
\\

Regression-safe: the existing 27 tests in \ est_resume_command.py\ (incl. \TestHandleSessionsCommand\ and \TestSameOriginChatGroupScoping) all still pass.

\uv run --no-sync python -m pytest tests/gateway/test_telegram_sessions_picker.py tests/gateway/test_sessions_command_picker_integration.py tests/gateway/test_resume_command.py\ → 49 passed.

Why not just merge #43695 / #49038

Both upstream PRs were kept open by the hermes-sweeper with the same blockers:

  • IDOR guard bypassed at the callback → fixed here via the closure-captured _resume_target_allowed\
  • Synchronous \session_store.switch_session, missing conversation-scope clear → fixed here via the funnel
  • Picker state keyed by chat_id alone → fixed here via (chat_id, msg_id, thread_id)\
  • No tests → fixed here with 22 new tests

Salvage credit: the pagination shape, cancel button, and adapter scaffold follow the pattern eltecnicowd opened in #49038. Rebased on top of the current \plugins/platforms/telegram/adapter.py\ (the path #43695 conflicted on).

🤖 Generated with opencode

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 18, 2026
@nothingness0db
nothingness0db force-pushed the feat/telegram-sessions-picker branch from 63464cc to 12232b1 Compare August 18, 2026 19:33
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review - automated review for reference; please use your judgment.

Reviewed the diff. This is the right architecture for an interactive picker over an authorization-sensitive action: the runner hands the adapter the FULL origin-scoped row list (picker paginates internally instead of the text 10-cap silently hiding sessions), the adapter stays transport-only with composite (chat_id, msg_id, thread_id) state keys so forum topics and double-/sessions cannot collide, stale keyboards are rejected by msg_id before anything runs, taps get a cheap adapter-level authorization gate mirroring the approval/choice pickers, and the runner callback re-runs _resume_target_allowed as the authoritative IDOR check before invoking the extracted _resume_session_by_id funnel. The NOTE explaining why the Matrix cross-room branch must stay separate from the shared helper is exactly the kind of comment that prevents a bad DRY cleanup later. Tests cover collision safety, stale rejection, unauthorized taps, pagination rendering, current-session filtering with index integrity, and the beyond-10 list handoff.

  • State lifetime: _sessions_picker_state entries are removed on select/cancel but otherwise live until process exit, and each holds a closure over the runner (on_session_selected). The PR text references a sweeper flagging this bug class for other pickers - please confirm whatever sweeper exists covers the new dictionary too (or add a TTL like the model picker), since a busy group could accumulate dozens of dead pickers holding runner references.

  • Nit: sx:noop as a page-indicator no-op works, but a dedicated prefix (sp:) would keep the sx cancel namespace unambiguous if more nav verbs ever appear.

No blocking issues found.

@nothingness0db

Copy link
Copy Markdown
Author

Indeed

Address reviewer feedback from the AI code review:

1. Picker UX redesign — preview moved to message body (every Telegram
   client renders the same), buttons show just page-relative numbers.
   Single-line body format keeps title + preview visible without the
   "stacked clutter" the original two-line layout produced on mobile.

2. Memory leak fix — _sessions_picker_state entries now carry a
   created_at timestamp and are swept on every send_sessions_picker
   call (lazy TTL, 1h default). Without this, busy groups accumulate
   dead pickers holding closures over the runner callback.

3. State key cleanup — nav-row page-indicator uses the dedicated "sp:"
   prefix (was "sx:noop" co-habiting with the cancel "sx:" namespace),
   so future nav verbs don't collide with cancel handling.

4. MarkdownV2 escaping — title and preview are now escaped for the
   parse_mode on every body line (was missing for the new body format).

Tests: 51 pass. Deployed + verified on Telegram.
@nothingness0db
nothingness0db force-pushed the feat/telegram-sessions-picker branch from 13d6959 to 8747a3a Compare August 22, 2026 00:34
@nothingness0db

nothingness0db commented Aug 22, 2026

Copy link
Copy Markdown
Author

Pushed an update addressing the three blockers raised in the AI review + manual UX testing on a live Telegram deployment.

Memory leak (the main concern): _sessions_picker_state\ entries now carry a \created_at\ timestamp and are evicted lazily on every \send_sessions_picker\ call. _SESSIONS_PICKER_TTL_SECONDS = 3600\ default — a picker that isn't tapped in an hour is abandoned by definition, and the dict holds closures over the runner callback. Without this, busy groups accumulate dead pickers. Covers the same defect class for the model picker and choice picker in a follow-up if you want symmetry.
图片

State key namespace: the nav-row page-indicator moved from \sx:noop\ to a dedicated \sp:noop\ prefix, so future nav verbs don't have to dodge the cancel handler.

UX rework on the same surface (from live Telegram testing): preview moved from button to message body so every client (mobile / desktop / web) renders the disambiguation hint identically. Body is now \1. Title — preview\ single-line so title stays visible while 8 rows stay scannable. Buttons are just page-relative numbers in a 4-up grid. MarkdownV2 escaping applied to title and preview. Page info now reflects the picker page count, not the legacy 10-cap.

Live-verified on the user's Telegram bot (tested 8 mock sessions: pagination works, IDOR gate fires on co-member taps, cancel pops state, switched session re-delivered to the running agent). 51 tests pass.

Updated screenshots / mockup: see inline

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants