PR Review: Telegram Interface + Chat/Task Modes

Thanks for this substantial addition. The Telegram integration is well-structured and the Chat/Task mode abstraction across all interfaces is a solid design. Here is the detailed feedback, organized by category.

Bugs / Correctness Issues

1. hmac.new should be hmac.new → hmac.new is not a thing — it must be hmac.HMAC or the module-level shortcut hmac.new.
Actually, looking closely at slack.py:187 and whatsapp.py:204:

hmac.new(...)   # This is wrong — the correct call is hmac.new() in Python 2 only.

In Python 3, hmac does not export a new function at the module level in the way it is being called here. The correct call is:

hmac.new(key, msg, digestmod)  # actually this IS valid in Python 3 as well

Correction: hmac.new is valid in Python 3. Disregard — but double-check that tests actually exercise this path, because hmac.new is not the idiomatic form (hmac.HMAC or the recommended hmac.new are both fine, but the Slack pre-existing code should be verified with a test).

2. _process_text_message silently drops unknown bot commands (telegram.py:462-465)
When a user sends /somecommand, the code falls through the if text.startswith("/") check (which only logs), and then still proceeds to _process_task_mode or _process_chat_mode with the raw /somecommand text. The agent will receive /help or /start etc. as a task. This is likely unintentional — either return early for unrecognized commands, or explicitly document that all commands are forwarded to the agent.

3. _process_callback_query task mode does not extract model_response.text (telegram.py:1048-1055)
The task-mode path in _process_callback_query only checks run_result.output, while every other task-mode handler (e.g., _process_task_mode at line 492-498) also checks run_result.get_last_model_response().text. This means callback query responses in task mode will silently drop the model's text reply if it is not in output.

4. Chat session keyed on user_id but group messages use chat_id for replies (telegram.py:519)
In CHAT mode, the session is looked up by str(user_id) (the Telegram user), but the response is sent to chat_id. In group chats, multiple users share a chat_id but each has their own session. This is semantically correct per-user, but means that in a group, User A's session context is invisible to User B even though replies appear in the same chat. This is probably fine but worth documenting, since it may surprise users expecting group-scoped sessions.

Security Concerns

5. set-webhook and delete-webhook endpoints are unauthenticated (telegram.py:269-287)
The /telegram/set-webhook and /telegram/delete-webhook endpoints accept arbitrary URLs with no authentication. An attacker who can reach the FastAPI server can redirect the bot's webhook to their own server, receiving all user messages. These endpoints should require authentication — at minimum, check the webhook_secret header, or gate them behind the same auth mechanism used elsewhere in the project (see interfaces/auth.py).

6. Bot token is included in health check response (telegram.py:190)
health_check exposes "bot_token_configured": bool(self.telegram_tools.bot_token) — this is fine (just a boolean). But the TelegramTools.api_url property (telegram.py tool line 135) formats the token directly into the URL string. If this URL ever surfaces in a log or error response that bubbles up to the health endpoint, the token leaks. The _api_request error logging at tool line 194 logs the full URL indirectly via httpx error messages, which include the request URL. Consider masking the token in the URL for log purposes.

7. Webhook secret comparison timing (telegram.py:249)

if secret != self.webhook_secret:

This is a plain string comparison, which is vulnerable to timing attacks. Use hmac.compare_digest(secret, self.webhook_secret) instead, consistent with how Slack and WhatsApp do their signature checks.

Code Quality / Design

8. Massive code duplication across _get_media_category and _get_format_error_message
These two methods are copy-pasted verbatim between TelegramInterface (telegram.py:854-915) and WhatsAppInterface (whatsapp.py:342-415), including the identical class-level IMAGE_TYPES, AUDIO_TYPES, VIDEO_TYPES, DOCUMENT_TYPES sets. These should live in the Interface base class or a shared utility. This will bite on maintenance — a bug fix in one won't propagate to the other.

9. __getattr__-based lazy loading in __init__.py re-imports on every access (interfaces/init.py:155-170)
Each attribute lookup calls _get_interface_classes(), _get_auth_functions(), and _get_schema_classes() — which each do a fresh import round. The returned dicts are never cached. At minimum, cache the dicts at module level after first construction. This is a minor performance issue on repeated imports but is a pattern that will confuse contributors.

10. InterfaceResetCommand is a plain class, not a dataclass or Pydantic model (schemas.py:29-47)
Every other schema in schemas.py is a Pydantic BaseModel. InterfaceResetCommand is a plain class with class-level attributes that are shared across all instances (the command and case_sensitive attributes are class variables, not instance variables). This means:

cmd1 = InterfaceResetCommand()
cmd1.command = "/clear"
cmd2 = InterfaceResetCommand()
print(cmd2.command)  # Still "/reset" — but only because Python creates an instance attr on cmd1

This works by accident. If someone does InterfaceResetCommand.command = "/clear", it mutates the class default for all future instances. Convert to a dataclass or Pydantic model.

11. get_poll_tools returns only create_poll, but the docstring says (create_poll, stop_poll) (telegram tool line 1087)
The stop_poll function referenced in the docstring is never defined or returned. Either implement it or fix the docstring.

Performance

12. No file size limits on media downloads (telegram.py:556, tool line 842)
download_file downloads the entire file into memory with no size cap. Telegram allows files up to 20 MB. For a production system, consider adding a max_file_size option and streaming/chunked downloads, especially since the bytes are then written to a temp file anyway.

13. Temp files in _process_media_with_agent use delete=False but cleanup is in a finally block — race condition if the process crashes
This is the standard pattern but worth noting: if the process is killed (OOM, SIGKILL), temp files in /tmp with prefix telegram_media_ will accumulate. A periodic cleanup or use of a dedicated temp directory with TTL would be more robust in production.

Test Coverage

14. Zero tests for any of the new code
There are no tests for:

• TelegramInterface webhook handling, message routing, whitelist, or mode switching
• TelegramTools API request construction or message splitting
• The new InterfaceMode / InterfaceResetCommand abstractions in the base class
• The Chat/Task mode logic in SlackInterface, WhatsAppInterface, or GmailInterface

Given that the project has a tests/ directory with pytest configured, even basic unit tests mocking the HTTP layer would significantly increase confidence. The Pydantic schemas (TelegramMessage, TelegramWebhookPayload, etc.) are particularly easy to test with sample payloads.

Minor / Nits

• telegram.py:355: import traceback inside an except block — move to module level.
• telegram.py:1019: import os inside a finally block — already imported at module level (line 18).
• telegram.py:937: import mimetypes as mimetypes_module — just import mimetypes is fine since there is no name collision in scope.
• telegram tool line 771: send_chat_action returns result is True but the Telegram API actually returns True as the result value (not the usual {"ok": true, "result": ...} structure). Verify this works correctly — if _api_request returns the result field, and that field is literally True, then result is True is correct. But if it returns e.g. {"ok": true}, this will always be False.
• The on_event("startup") hook on the router (telegram.py:300-303) is deprecated in FastAPI. Use lifespan context managers instead.

Summary

The overall structure is good and the Chat/Task mode abstraction is a worthwhile addition. The main blockers before merge are the unauthenticated webhook management endpoints (#5), the timing-attack-vulnerable secret comparison (#7), and the missing tests (#14). The code duplication (#8) and the InterfaceResetCommand class variable bug (#10) are strong candidates for follow-up.