fix(telegram): log document/video send failures instead of printing to stdout (#13356)#13387
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Telegram gateway observability by logging native send_document / send_video failures via the module logger (with stack traces) instead of printing to stdout, while preserving the existing base-adapter fallback behavior.
Changes:
- Replace
print(...)inTelegramAdapter.send_documentandsend_videoexception handlers withlogger.error(..., exc_info=True). - Add regression tests asserting failures are logged (with
exc_info) and not emitted to stdout. - Add a canary test ensuring the base-adapter fallback path still executes on
send_documentfailures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gateway/platforms/telegram.py |
Switch error reporting from stdout print to structured logging with stack traces for document/video sends. |
tests/gateway/test_telegram_documents.py |
Add caplog/capsys regression coverage to ensure failures are logged (not printed) and fallback behavior remains intact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --- Regression coverage for #13356 ----------------------------------- | ||
| # Before the fix, ``send_document`` wrote the exception to stdout via | ||
| # ``print(...)`` instead of ``logger.error``. On systemd/Docker gateway | ||
| # deployments the diagnostic was invisible, so operators seeing | ||
| # "Hermes says success, Telegram received nothing" had no way to | ||
| # see the underlying error. These tests pin that every failure | ||
| # routes through the standard logger with a full stack trace (matches | ||
| # the pattern established by ``send_voice`` and ``send_image_file``). | ||
|
|
There was a problem hiding this comment.
The PR description says this file adds “4 new cases (2 per method)”, but this diff adds 5 new tests (3 under TestSendDocument plus 2 under TestSendVideo). Please update the PR description to match the actual test changes (or drop the extra test if the intent really was 4).
|
Thanks @copilot-pull-request-reviewer — caught pattern #7 (docstring / PR-description accuracy) cleanly. You're right: the prose said "4 new cases (2 per method)" but the actual diff adds 5 (3 under The 3 No code change needed. |
|
Supply-chain Quick version: Was masked pre- |
…o stdout (NousResearch#13356) ``TelegramAdapter.send_document`` and ``send_video`` wrote native-send exceptions to ``stdout`` via ``print(...)`` before falling back to the base adapter's text-fallback. On systemd / Docker / journald-captured gateway deployments ``stdout`` from the Python process isn't captured in the gateway log stream — so operators who reported "Hermes says success, Telegram received nothing" had no visible error to diagnose. The adjacent paths in the same file already use the right pattern: * ``send_voice`` (line 1701) → ``logger.error(..., exc_info=True)`` * ``send_image_file`` (line 1738) → ``logger.error(..., exc_info=True)`` Only ``send_document`` and ``send_video`` had drifted to ``print``. Fix --- Replace ``print`` with ``logger.error`` and attach ``exc_info=True`` so the full stack + exception chain lands in the gateway log. Matches the sibling-method pattern exactly. Narrow scope — explicitly not changed ------------------------------------- * **Fallback routing.** Still falls through to ``super().send_document(...)`` / ``super().send_video(...)`` on native-send failure, same as ``send_voice`` / ``send_image_file``. If Teknium wants to change that semantic for Telegram (where native support exists so the fallback masks real failures), that's a separate, larger-scope PR. * **Return value.** ``SendResult`` still reflects the text fallback's success, unchanged. * **Other adapters.** Only Telegram had this ``print`` pattern; other platforms already use ``logger``. Regression coverage ------------------- ``tests/gateway/test_telegram_documents.py`` gets 4 new cases (2 per method): * ``test_send_document_api_error_is_logged_with_exc_info`` — asserts an ``ERROR``-level record lands in the ``gateway.platforms.telegram`` logger with ``exc_info`` attached, the exception text present, and the adapter name in the message. * ``test_send_document_logs_do_not_hit_stdout`` — structural pin: the old ``"[Telegram] Failed to send document"`` string must not appear on ``stdout`` (would re-introduce the systemd/Docker-invisible regression). * ``test_send_document_still_invokes_base_fallback_on_error`` — preserved-behaviour canary: this PR only changes observability, not routing. * Matching pair for ``send_video``. All 4 new tests fail on clean ``origin/main`` (``6fb69229``) with:: AssertionError: '[Telegram] Failed to send document' is contained here: '[Telegram] Failed to send document: boom\\n' AssertionError: Expected an ERROR-level log from gateway.platforms.telegram on send_document failure Validation ---------- ``source venv/bin/activate && python -m pytest tests/gateway/test_telegram_documents.py -q`` → **36 passed**. Broader Telegram suite (``test_telegram_documents.py`` + ``test_telegram_approval_buttons.py`` + ``test_telegram_caption_merge.py`` + ``test_telegram_conflict.py``) → **68 passed, 0 regressions**. Related ------- This PR addresses the observability portion of NousResearch#13356. The reporter also describes "reports success, Telegram receives nothing" — the deeper question of whether the base-adapter text fallback itself should surface a more distinct failure signal (or whether Telegram native send silently succeeds for files that later server-side drop) is out of scope for this PR; with proper logging in place, operators can now diagnose the actual upstream error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a656776 to
adc2b02
Compare
|
Rebased onto current Re-ran focused tests on rebased head ( The two new regression tests ( Still applicable: |
|
Closing to keep the queue clean — happy to reopen if this is still useful. |
Addresses the observability portion of #13356.
TL;DR
TelegramAdapter.send_documentandsend_videowrote native-send exceptions tostdoutviaprint(...)before falling back to the base adapter. On systemd / Docker / journald-captured gateway deploymentsstdoutisn't captured in the gateway log stream — so operators reporting "Hermes says success, Telegram received nothing" had no visible error to diagnose.Fix: replace
printwithlogger.error(..., exc_info=True), matching the established pattern insend_voiceandsend_image_filein the same file.Root cause
gateway/platforms/telegram.py:The two adjacent native-send methods in the same file already use the correct pattern:
Only
send_documentandsend_videodrifted toprint.Fix
Identical change for
send_video. 4 production-line delta total.Behaviour matrix
printto stdout (invisible in journald), fallback returns successlogger.error(exc_info=True)to gateway log, fallback returns successprintto stdout, exception propagateslogger.error(exc_info=True), exception propagatesNarrow scope — explicitly not changed
super().send_document(...)/super().send_video(...), matchingsend_voice/send_image_file. This PR only fixes observability.SendResultstill reflects the text fallback's success, same as the sibling methods.printpattern; other platforms already uselogger.Messageobject (withmessage_id) for a subsequently-dropped file is also a bug — is out of scope here. With proper logging in place operators can now diagnose the actual upstream error and decide if that semantic needs changing.Regression coverage
tests/gateway/test_telegram_documents.pygets 5 new cases (3 for send_document, 2 for send_video), leveragingcaplog+capsys:TestSendDocument:test_send_document_api_error_is_logged_with_exc_info— asserts an ERROR record lands ingateway.platforms.telegramlogger with (1)exc_infoattached, (2) exception text present, (3) adapter name in message.test_send_document_logs_do_not_hit_stdout— structural pin: the old"[Telegram] Failed to send document"string must not appear on stdout. Prevents regression back toprint.test_send_document_still_invokes_base_fallback_on_error— preserved-behaviour canary: this PR only changes observability, not routing.TestSendVideo: matching pair (..._is_logged_with_exc_info,..._logs_do_not_hit_stdout).4 of 4 stdout-and-logger tests fail on clean
origin/main(6fb69229) with the exact symptom:Validation
Broader Telegram suite (4 test files) → 68 passed, 0 regressions.
Pre-empted review questions
Q. Why not also remove the
super()text fallback — isn't returningsuccess=Truemisleading when the file didn't actually send?Maybe, but that's a deliberate scope-narrowing choice. The same "fall through to base on native failure" pattern exists in
send_voiceandsend_image_file— changing it for Telegram specifically would break adapter symmetry and risk regressions in callers that rely on the fallback. Happy to follow up with a separate PR if the maintainer wants that behavior for Telegram.Q. Why test both
caplogandcapsys?caplogpins the positive invariant (error IS logged correctly).capsyspins the negative invariant (error is NOT leaked to stdout). Together they're a two-sided guard — a future refactor that addsprintback in addition tologger.errorwould still be caught bycapsys.Q. Is this a complete fix for #13356?
For the observability portion, yes. For the reporter's deeper "success reported, file never arrives" concern, the fix here at minimum makes any raised exception diagnosable. If Telegram is returning
message_idfor a file that's subsequently server-side dropped, that's either a python-telegram-bot semantics issue or amessage_thread_id/ path-access issue that needs separate investigation — with proper logging in place operators can now surface the actual upstream error.Co-authored via LLM assistance; I've reviewed every line and am responsible for correctness.