fix(delivery): diagnose Telegram API failures

[IceCodeNew]

Summary

• log safe Telegram delivery metadata at INFO level before sending
• classify Telegram HTTP failures into actionable reasons without exposing response bodies, tokens, chat IDs, or message text
• distinguish Telegram API rejections from transport failures

Root cause

Telegram HTTP 400 responses were reduced to a generic delivery error. Operators could not distinguish invalid chat configuration, permission failures, malformed HTML, message size limits, or other API rejections from network failures.

Impact

Production logs now show the message sizing and chunking mode plus a safe failure category. This makes configuration and API failures diagnosable while preserving sensitive values.

Verification

• prek run --all-files
• uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml
• 827 tests passed
• line coverage: 99.85%
• branch coverage: 99.51%

[coderabbitai]

Warning

Review limit reached

@IceCodeNew, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c06a2ea3-1d25-4145-b760-6f2a35047522

📥 Commits

Reviewing files that changed from the base of the PR and between f9062e6 and 60160a7.

📒 Files selected for processing (7)

• docs/design.md
• docs/notes.md
• tests/test_publishers.py
• tests/test_reference_data.py
• weather_briefing/data/telegram_error_classification.json
• weather_briefing/publishers.py
• weather_briefing/reference_data.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-telegram-delivery-diagnostics

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Diagnose Telegram delivery failures with safe logging and error classification

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Log safe Telegram delivery metadata (length, chunk count, options) before sending.
• Classify Telegram HTTP failures into actionable, non-sensitive failure reasons.
• Add coverage to ensure logs never leak tokens, chat IDs, or response bodies.

Diagram

graph TD
  A["Briefing runner"] --> B["TelegramPublisher"] --> C["httpx AsyncClient"] --> D{{"Telegram Bot API"}}
  B --> E["INFO/WARN logs"]
  B --> F["_telegram_error_reason"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Expose structured failure reason upstream (not only in message/log)

• ➕ Callers could route/alert differently (e.g., config error vs transient network) without parsing strings
• ➕ Easier to keep stable contracts and avoid log-coupled automation
• ➖ Requires API/exception shape changes (DeliveryError fields or new exception types)
• ➖ Potentially larger refactor across all publishers/call sites

2. Emit metrics for failure categories (in addition to logs)

• ➕ Better for alerting and trend analysis; avoids log scraping
• ➕ Can keep logs minimal while still being diagnosable
• ➖ Requires metrics plumbing and backend support
• ➖ Still needs careful cardinality control for reasons

3. Use a maintained Telegram error taxonomy/library

• ➕ Less bespoke string matching; easier updates as Telegram messages evolve
• ➕ Potentially more complete coverage of edge cases
• ➖ Adds dependency or a larger internal taxonomy surface
• ➖ May still rely on Telegram description strings; not always stable

Recommendation: The PR’s approach is appropriate for the stated goal: it improves operator diagnosability while explicitly preventing sensitive leakage by avoiding response-body logging and only emitting derived categories. If future use-cases need automated remediation/alerting, consider evolving DeliveryError to carry a structured reason (and/or emitting metrics) so callers don’t need to rely on log text or exception message parsing.

Files changed (3) +136 / -10

Bug fix (1) +63 / -3

publishers.pySplit Telegram transport vs API failures and log safe rejection reasons +63/-3

Split Telegram transport vs API failures and log safe rejection reasons

• Promotes the pre-send Telegram diagnostic log from DEBUG to INFO and enriches it with safe metadata. Splits exception handling between HTTPStatusError (Telegram API rejection) and RequestError (transport issues), logging chunk context and emitting a categorized failure reason. Introduces _telegram_error_reason() to classify known Telegram error descriptions/parameters and fall back to status-based categories without logging response bodies.

weather_briefing/publishers.py

Tests (1) +68 / -6

test_publishers.pyAdd tests for Telegram safe logging and error reason classification +68/-6

Add tests for Telegram safe logging and error reason classification

• Reworks the Telegram failure test to assert INFO logs contain only safe metadata and that API failures produce a categorized reason without leaking token/chat/response details. Adds parametrized coverage for multiple Telegram rejection patterns and validates transport errors are logged with chunk context but without private exception messages.

tests/test_publishers.py

Documentation (1) +5 / -1

design.mdDocument safe Telegram delivery diagnostics and failure categorization +5/-1

Document safe Telegram delivery diagnostics and failure categorization

• Updates the design docs to describe INFO-level logging of safe delivery metadata (length/chunking/options) and the new categorized handling of Telegram API rejections. Explicitly notes that message bodies, bot tokens, chat IDs, and unknown response text are not logged.

docs/design.md

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.79%. Comparing base (f9062e6) to head (60160a7).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master      #84    +/-   ##
========================================
  Coverage   99.79%   99.79%            
========================================
  Files          45       45            
  Lines        8654     8755   +101     
  Branches      505      520    +15     
========================================
+ Hits         8636     8737   +101     
  Misses         13       13            
  Partials        5        5            

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 39 rules

1. Duplicate failure warning logs ✓ Resolved 🐞 Bug ◔ Observability

Description

In the CLI path, TelegramPublisher now emits its own WARNING logs for HTTP rejections/transport
failures while the shared LoggedAsyncClient already emits WARNING logs for the same failed request,
producing duplicated warning records for a single failed Telegram call. This increases log
noise/cost and can skew warning-based dashboards/alert thresholds.

Code

weather_briefing/publishers.py[R170-193]

+            except httpx.HTTPStatusError as exc:
+                reason = _telegram_error_reason(exc.response)
+                _LOGGER.warning(
+                    "Telegram delivery rejected index=%d/%d message_visible_characters=%d payload_characters=%d "
+                    "status_code=%d reason=%s",
+                    index,
+                    len(chunks),
+                    message.visible_length,
+                    len(chunk),
+                    exc.response.status_code,
+                    reason,
+                )
+                raise DeliveryError(f"Telegram delivery failed ({reason})") from None
+            except httpx.RequestError as exc:
+                _LOGGER.warning(
+                    "Telegram delivery request failed index=%d/%d message_visible_characters=%d payload_characters=%d "
+                    "reason=%s",
+                    index,
+                    len(chunks),
+                    message.visible_length,
+                    len(chunk),
+                    type(exc).__name__,
+                )
+                raise DeliveryError("Telegram delivery failed (request-error)") from None

Relevance

⭐⭐⭐ High

Team has reduced noisy WARNINGs before (e.g., downgrade expected failures/logging) and added
centralized HTTP warning logs (PR #21).

PR-#21
PR-#79

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI wires TelegramPublisher to LoggedAsyncClient; LoggedAsyncClient emits WARNING logs for
failed responses/exceptions, and the new TelegramPublisher code also emits WARNING logs for the same
failure conditions, creating duplicate warning entries for one failed Telegram request.

weather_briefing/cli.py[283-288]
weather_briefing/cli.py[394-411]
weather_briefing/api_client.py[42-77]
weather_briefing/publishers.py[170-193]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Telegram delivery failures are now logged twice at WARNING in the default CLI runtime: once by `LoggedAsyncClient` (generic API failure) and again by `TelegramPublisher` (Telegram-specific context). This duplication is new with this PR and will increase warning volume.

### Issue Context
- The CLI constructs a `LoggedAsyncClient` and passes it down to `TelegramPublisher`.
- `LoggedAsyncClient` logs `WARNING` for `response.is_error` and for request exceptions.
- `TelegramPublisher` now logs `WARNING` in both `HTTPStatusError` and `RequestError` handlers.

### Fix Focus Areas
- weather_briefing/publishers.py[170-193]
- weather_briefing/api_client.py[42-87]
- weather_briefing/cli.py[283-288]

### Suggested fix
Choose a single layer to emit WARNING for Telegram delivery failures:
- Option A (localized): downgrade `TelegramPublisher` failure logs to `INFO` (still visible with default app logging), relying on `LoggedAsyncClient` for WARNING severity; keep the publisher log for the *classification* field.
- Option B (more general): adjust `LoggedAsyncClient` to log `response.is_error` at `INFO` (or `DEBUG`) and reserve `WARNING` for exceptions only, so callers like `TelegramPublisher` can own failure severity.
- Option C (targeted): in `LoggedAsyncClient`, special-case the `telegram/send-message` extension and log non-2xx at INFO so `TelegramPublisher`’s WARNING is the single warning line.

Update/extend tests if log level expectations change.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Telegram client rationale undocumented ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

TelegramPublisher implements a custom Telegram Bot API client using httpx and custom error
handling, but there is no corresponding justification in docs/notes.md explaining why a maintained
Telegram SDK is not used. This violates the requirement to document justified custom
external-service client implementations.

Code

weather_briefing/publishers.py[R141-144]

+        _LOGGER.info(
            "Telegram delivery prepared: visible_characters=%d payload_characters=%d chunks=%d single_message=%s",
            message.visible_length,
            len(message.body),

Relevance

⭐⭐⭐ High

Team previously documented “don’t reimplement maintained SDKs” rationale in notes for any-llm
migration (PR #41).

PR-#41

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires documenting justification in docs/notes.md whenever a custom HTTP client is
used for an external service instead of a maintained SDK. The PR changes expand the custom Telegram
handling in TelegramPublisher (manual HTTP calls and bespoke classification) while docs/notes.md
contains no entry describing this client choice or rationale.

Rule 2141694: Prefer well-maintained official SDKs over custom external-service clients, and document justified custom implementations
weather_briefing/publishers.py[112-193]
docs/notes.md[1-6]
docs/notes.md[91-119]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The code uses a custom `httpx`-based Telegram Bot API client, but `docs/notes.md` does not document why a maintained Telegram SDK/library is not used and what the custom client is responsible for.

## Issue Context
Compliance requires that custom external-service clients/wrappers be justified in `docs/notes.md` (service name, SDK not used, and rationale/limitations).

## Fix Focus Areas
- docs/notes.md[1-6]
- docs/notes.md[91-119]
- weather_briefing/publishers.py[112-193]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Telegram error markers hard-coded ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

_telegram_error_reason() hard-codes a pattern/lookup table of Telegram error-description markers
and reason mappings directly in code. The rule requires such reference data (matching patterns /
lookup tables) to be externalized into validated configuration sources.

Code

weather_briefing/publishers.py[R222-257]

+def _telegram_error_reason(response: httpx.Response) -> str:
+    """Classify a Telegram API error without logging its response body."""
+    try:
+        payload = response.json()
+    except ValueError:
+        payload = None
+
+    if isinstance(payload, dict):
+        parameters = payload.get("parameters")
+        if isinstance(parameters, dict) and type(parameters.get("migrate_to_chat_id")) is int:
+            return "chat-migrated"
+
+        description = payload.get("description")
+        if isinstance(description, str):
+            normalized = description.casefold()
+            markers = (
+                ("chat not found", "chat-not-found"),
+                ("bot was blocked by the user", "bot-blocked"),
+                ("user is deactivated", "user-deactivated"),
+                ("not enough rights", "insufficient-rights"),
+                ("have no rights to send a message", "insufficient-rights"),
+                ("can't parse entities", "invalid-html"),
+                ("message is too long", "message-too-long"),
+                ("message text is empty", "empty-message"),
+                ("too many requests", "rate-limited"),
+            )
+            for marker, reason in markers:
+                if marker in normalized:
+                    return reason
+
+    return {
+        401: "bot-token-rejected",
+        403: "forbidden",
+        404: "bot-token-rejected",
+        429: "rate-limited",
+    }.get(response.status_code, "api-error")

Relevance

⭐⭐ Medium

Some reference data is externalized (e.g., JSON via reference_value), but no precedent forcing small
marker tables out of code.

PR-#23

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rule forbids embedding lookup tables and matching-pattern lists directly in implementation code
and requires moving them to validated config/data sources. The added _telegram_error_reason()
function introduces an inline markers table and status-code mapping dict used for classification.

Rule 2141692: Externalize domain reference data into validated configuration sources
weather_briefing/publishers.py[222-257]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_telegram_error_reason()` embeds a non-trivial list of matching patterns and reason mappings directly in code. Compliance requires domain/reference lookup data and matching-pattern tables to live in a dedicated configuration/data file with explicit validation.

## Issue Context
The repository already has a validated reference-data mechanism (`weather_briefing/reference_data.py`) and packaged JSON files under `weather_briefing/data/`, which can be used to store and validate the Telegram error classification mapping.

## Fix Focus Areas
- weather_briefing/publishers.py[222-257]
- weather_briefing/reference_data.py[89-132]
- weather_briefing/data[1-1]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Silent option not logged ✓ Resolved 🐞 Bug ◔ Observability

Description

The new INFO-level “Telegram delivery prepared” log omits the silent/disable_notification
delivery option even though the design doc states INFO logs record delivery options. This reduces
the usefulness of the new diagnostics when verifying whether a message was intentionally delivered
silently.

Code

weather_briefing/publishers.py[R141-144]

+        _LOGGER.info(
            "Telegram delivery prepared: visible_characters=%d payload_characters=%d chunks=%d single_message=%s",
            message.visible_length,
            len(message.body),

Relevance

⭐⭐ Medium

Repo values design/log contract alignment, but no clear prior reviews requiring logging every
delivery option.

PR-#55
PR-#21

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The design doc says INFO logs record delivery options, and the code uses silent to set Telegram’s
disable_notification, but the new INFO log does not include silent/disable_notification.

docs/design.md[167-171]
weather_briefing/publishers.py[130-147]
weather_briefing/publishers.py[158-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new Telegram preparation log line is intended to include delivery options, but it currently logs only `single_message` (and sizing/chunking) and does not log the `silent` flag that is sent as `disable_notification`.

### Issue Context
Operators troubleshooting Telegram delivery may need to distinguish “silent by policy” deliveries from normal deliveries; the design doc also claims delivery options are included.

### Fix Focus Areas
- weather_briefing/publishers.py[141-147]
- weather_briefing/publishers.py[158-166]
- docs/design.md[167-171]

### Suggested fix
Add a safe boolean field to the INFO log message, e.g. `silent=%s` or `disable_notification=%s`, and pass the `silent` argument value. If needed, update the design doc wording and any log-asserting tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---