revert: 4 Telegram salvages flagged as broken or risky by post-merge audit - #28575
Merged
Conversation
Contributor
🔎 Lint report:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-merge audit (parallel subagent fan-out across 9 clusters) flagged 4 of the 44 Telegram salvages as broken or unsafe. Reverting them.
Reverts (newest-first to minimize conflicts)
1. Revert #26636 "pin incoming user message for duration of agent turn" (a724c3b)
Issue: Pin/unpin enabled UNCONDITIONALLY with no opt-out flag.
Pin Messagesadmin permission; without it every message logs a debug-level failure (silent noise).session_context.pyandrun.pyedits, but the actual diff only touchestelegram.py.Unpin DOES fire reliably on cancel/exception (
on_processing_completeinvoked in both paths), so that aspect was correct — the issue is the default-on policy without an opt-in flag.Path forward: Re-merge gated behind
extra.pin_user_messages: truewith tests.2. Revert #28015 "support quick-command-only menus" (b1acf80)
Issue: Feature is non-functional. The salvage references
self._runner_ref, but no code in the repo ever assigns_runner_refto aTelegramAdapterinstance. Verified:The fallback path
getattr(self, '_runner_ref', None)returns None, the top-levelquick_commandsis never reached, andset_my_commands([])registers an empty menu when users settelegram.command_menu: quick_commands_onlywith their quick_commands defined at the top level.Nothing populates
PlatformConfig.extra.quick_commandsanywhere in the codebase, so the only working code path requires users to manually duplicatequick_commandsintoplatforms.telegram.extra.quick_commands— undocumented.Path forward: Re-merge after either (a) wiring an actual runner ref on the adapter at attach-time, or (b) bridging
gateway.quick_commands→platforms.telegram.extra.quick_commandsat config-load time.3. Revert #27865 "auto-detect @username mentions and create Telegram entities" (cf814c9)
Issue: Three bugs, no tests:
r'@([a-zA-Z][a-zA-Z0-9_]{4,31})'matches@exampleinsideuser@example.com:parse_mode+entitiesare mutually exclusive per Telegram Bot API. Bot API docs: "can be specified instead of parse_mode". Passing both alongsideParseMode.MARKDOWN_V2may cause Bot API to reject or silently ignore parse_mode.Offset miscalculation. Offsets are computed against the MarkdownV2-escaped
formattedstring.\@escape bytes shift offsets relative to plain text; entity boundaries may mis-align in rendered output.Path forward: Re-merge with (a) stricter regex (negative-lookbehind for
[^a-zA-Z0-9._-]), (b) dropentities=whenparse_modeis set, (c) compute offsets against unescaped text.Conflict resolution: kept #27098's
**text_kwargssplit (thread-not-found retry) while removing only theentities=_entitiesargument.4. Revert #23795 "enforce TELEGRAM_ALLOWED_USERS allowlist on inbound messages" (db50af9)
Issue: P0 claim is incorrect, and salvage introduces three concrete regressions.
The PR claims
TELEGRAM_ALLOWED_USERS was only checked for callback/inline-button actions but not for inbound messages. False — the gateway runner's_is_user_authorizedatgateway/run.py:6025already fail-closed inbound messages BEFORE this salvage. Default is deny.Regressions introduced by adding the adapter-level user_id check:
Channel posts dropped.
message.from_userisNonefor channel broadcasts, so_user_id=""short-circuits to deny BEFORE the runner sees the message. This required a follow-up test fixture stub (f1cefad8c test+release: stub auth in channel_posts fixture) just to keep the fix(telegram): handle channel post updates #25327 channel-post tests green — proving the salvage broke real channel post routing.Anonymous-admin / sender_chat traffic dropped. PR fix(gateway): allow chat-scoped telegram auth without sender user_id #27806 (later-merged
fix(gateway): allow chat-scoped telegram auth without sender user_id) explicitly aims to support this — but the adapter-level user_id requirement neuters it because_should_process_messagereturns False BEFORE the runner-level chat-scoped fallback can authorize.First-time DM users see silence. The runner's pairing-flow onboarding UX in
_handle_messageis skipped because the adapter drops the message first.The salvage ships zero tests despite the P0 label, and its docstring claim "Empty TELEGRAM_ALLOWED_USERS continues to allow all users" contradicts the actual code path it goes through after #24468 made the env fallback fail-closed.
Net: only real gain over pre-existing behavior is an earlier log line, paid for with three concrete user-facing regressions.
#24468 (the fail-closed env fallback, separate PR) is correctly KEPT — it only changes a rarely-reached fallback path. #27806's chat-scoped auth is correctly KEPT (the runner-level part), and reverting #23795 unblocks it functionally.
Path forward: If we want defense-in-depth at the adapter level, do it as a parity check (only refuse when runner would also refuse), not as a hard pre-runner gate.
Audit methodology
Parallel subagent fan-out across 9 clusters (44 PRs total):
Each PR audited on: bug reality (does parent code have the bug?), fix correctness, test coverage (would new tests fail against parent?), side effects (silent default changes?), interaction with other recent salvages.
Verdict counts: KEEP 33 · KEEP-WITH-FOLLOWUP 7 · REVERT 4 (this PR).
Follow-up issues to file (not in this PR):
*_ALLOW_ALL_USERSenv or runner-level fix to extend chat-scoped auth tochat_type == 'channel'._GATEWAY_PROVIDER_ERROR_REover-broad — could rewrite legitimate assistant prose mentioning HTTP status codes. Telegram-only scope limits blast radius; tightening regex is the follow-up.direct_messages_topic_id, no retry path; broaden_should_retry_without_dm_topic_reply_anchor.bd0c54d17 fix: route Telegram image documents through photo handling.Validation
scripts/run_tests.sh tests/gateway/test_telegram_group_gating.py tests/gateway/test_telegram_mention_boundaries.py tests/gateway/test_telegram_callback_auth_fail_closed.py tests/gateway/test_telegram_channel_posts.py tests/gateway/test_telegram_documents.py tests/gateway/test_unauthorized_dm_behavior.py tests/tools/test_send_message_tool.py tests/tools/test_send_message_telegram_proxy.py -q→ 245/246 passing (1 pre-existing slack-parse flake unrelated)scripts/run_tests.sh tests/gateway/test_telegram_thread_fallback.py -q→ 41/41 passing in isolationThe 3 test-pollution failures observed when running the full telegram test set together also reproduce on plain main without the reverts — pre-existing issue not introduced here.