fix(delivery): improve Telegram failure diagnostics

[IceCodeNew]

Summary

• classify Telegram API failures into safe structured reasons without logging response bodies, Bot Tokens, or Chat IDs
• let the Telegram adapter own the single WARNING for handled API rejections while the shared HTTP client records only INFO context
• skip a guaranteed-to-fail operations alert when the original failure made the shared delivery channel unavailable
• validate Telegram error metadata and channel availability reasons as packaged reference data

Root cause

A Telegram rejection produced generic HTTP logging and then attempted to send the task-failure alert through the same unavailable Telegram destination. Operators could see a 400 response but not a safe actionable reason such as chat-not-found, and the redundant alert attempt added noise.

Behavior

Known Telegram descriptions, parameters.migrate_to_chat_id, and HTTP statuses map to safe kebab-case reasons. Channel-level failures such as chat-not-found, bot-token-rejected, and insufficient-rights suppress only the redundant alert when operations delivery is the same object. Transient or message-specific errors still attempt the alert. Telegram reference data is validated when a Telegram publisher is constructed, so non-Telegram runs do not depend on it.

Privacy

Normal logs continue to omit message bodies, credentials, Chat IDs, URLs, and raw Telegram error descriptions. Unknown responses fall back to api-error.

Review fixes

• validate response-error ownership and channel availability as real booleans
• keep parameter reasons and channel availability classifications in validated reference data
• scope Telegram reference-data validation to Telegram publisher construction

Validation

• 867 passed
• line coverage: 99.85% (master: 99.85%)
• branch coverage: 99.54% (master: 99.52%)
• prek run --all-files
• latest local CodeRabbit review of the published full diff: 0 issues
• Qodo review: 0 bugs, prior finding resolved
• commit hooks passed

The GitHub CodeRabbit status is rate limited; no online review ran for that status.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 35 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: 12bbbbbd-5736-4c56-924a-4d0c7d12aab3

📥 Commits

Reviewing files that changed from the base of the PR and between ed4363e and 2cd6927.

📒 Files selected for processing (10)

• docs/design.md
• tests/test_api_client.py
• tests/test_publishers.py
• tests/test_reference_data.py
• tests/test_service.py
• weather_briefing/api_client.py
• weather_briefing/data/telegram_error_classification.json
• weather_briefing/publishers.py
• weather_briefing/reference_data.py
• weather_briefing/service.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/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.}

[codecov]

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #88   +/-   ##
=======================================
  Coverage   99.79%   99.79%           
=======================================
  Files          45       45           
  Lines        8810     8908   +98     
  Branches      526      538   +12     
=======================================
+ Hits         8792     8890   +98     
  Misses         13       13           
  Partials        5        5           

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

[qodo-code-review]

PR Summary by Qodo

Fix Telegram delivery diagnostics and suppress redundant ops alerts

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Classify Telegram API rejections into safe structured reasons without leaking tokens, chat IDs, or
 bodies.
• Move handled Telegram rejection WARNINGs into the Telegram adapter; keep shared HTTP logging
 INFO-only.
• Skip failure-alert delivery when the same shared channel is definitively unavailable.

Diagram

graph TD
  SVC["BriefingService"] --> DP["DeliveryProvider"] --> TP["TelegramPublisher"] --> HC["LoggedAsyncClient"] --> TG{{"Telegram Bot API"}}
  TP --> RD["Telegram classification"]
  TP --> DE["DeliveryError (reason)"] --> SVC

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Typed exception hierarchy instead of string reasons

• ➕ Compile-time discoverability of failure categories (e.g., ChannelUnavailableError vs MessageRejectedError)
• ➕ Avoids regex validation and string comparisons throughout the codebase
• ➖ More classes/boilerplate and migration churn for existing tests/handlers
• ➖ Still needs stable, privacy-safe identifiers for logging and metrics

2. Centralize response-error classification in LoggedAsyncClient

• ➕ Single place to interpret provider HTTP errors
• ➕ Potentially reusable across other adapters
• ➖ Client layer lacks provider-specific payload knowledge (e.g., Telegram parameters.migrate_to_chat_id)
• ➖ Increases risk of leaking response details; blurs adapter ownership of warnings

Recommendation: Current approach is the best fit: keep provider-specific parsing/classification inside the Telegram adapter (where payload knowledge lives), while giving the shared HTTP client a minimal, privacy-preserving logging contract. The added channel_unavailable metadata enables the service to make a clear operational decision (skip redundant alerts only when the shared channel is known-bad) without weakening privacy guarantees.

Files changed (10) +291 / -51

Enhancement (1) +36 / -2

reference_data.pyValidate expanded Telegram error classification reference data +36/-2

Validate expanded Telegram error classification reference data

• Expands TelegramErrorClassification to include parameter-based reasons and a validated set of channel-unavailable reasons. Adds strict schema validation ensuring fields exist, reasons are safe, and channel-unavailable reasons are unique and drawn from known classifications.

weather_briefing/reference_data.py

Bug fix (3) +80 / -20

api_client.pyAllow adapters to own response-error warnings via request extensions +23/-3

Allow adapters to own response-error warnings via request extensions

• Extends 'api_call_extensions' with a validated 'response_error_handled' boolean and uses it to downgrade handled HTTP error responses from WARNING to INFO in LoggedAsyncClient logging.

weather_briefing/api_client.py

publishers.pyEmit single adapter WARNING and raise structured DeliveryError for Telegram failures +41/-11

Emit single adapter WARNING and raise structured DeliveryError for Telegram failures

• Introduces a structured DeliveryError carrying a safe reason and 'channel_unavailable' flag with strict validation. TelegramPublisher now marks Telegram HTTP errors as handled for the HTTP client, emits the sole WARNING on rejection, classifies failures using reference data (description markers, migrate_to_chat_id, status), and consistently raises DeliveryError without leaking sensitive details.

weather_briefing/publishers.py

service.pySkip redundant failure alert when shared channel is unavailable +16/-6

Skip redundant failure alert when shared channel is unavailable

• When a run fails due to a DeliveryError marked 'channel_unavailable' and the ops delivery provider is the same instance as the primary delivery, the service logs an INFO skip reason and avoids attempting to send an alert through the same unavailable channel.

weather_briefing/service.py

Tests (4) +160 / -27

test_api_client.pyTest handled-response error logging downgrade in LoggedAsyncClient +24/-0

Test handled-response error logging downgrade in LoggedAsyncClient

• Adds coverage ensuring that when an adapter claims ownership of a response error, the HTTP client logs only INFO (no WARNING). Also validates type-checking for the new 'response_error_handled' extension flag.

tests/test_api_client.py

test_publishers.pyTest DeliveryError safety and Telegram channel-unavailability metadata +40/-15

Test DeliveryError safety and Telegram channel-unavailability metadata

• Adds validation tests for safe kebab-case reasons and boolean channel availability. Updates Telegram publisher tests to assert adapter-owned WARNING logging, structured reason propagation, and correct channel_unavailable classification across scenarios.

tests/test_publishers.py

test_reference_data.pyExpand reference-data validation coverage for Telegram metadata fields +43/-11

Expand reference-data validation coverage for Telegram metadata fields

• Introduces helper fixtures for Telegram classification reference data and extends tests to validate parameter reasons and channel_unavailable_reasons constraints (presence, uniqueness, and known-reason membership).

tests/test_reference_data.py

test_service.pyTest skipping ops alert when shared delivery channel is unavailable +53/-1

Test skipping ops alert when shared delivery channel is unavailable

• Adds a publisher stub that raises a channel-unavailable DeliveryError and a service test asserting the failure alert is not re-attempted through the same delivery provider, while still preserving the original exception.

tests/test_service.py

Documentation (1) +1 / -1

design.mdDocument Telegram rejection logging ownership and alert-skip behavior +1/-1

Document Telegram rejection logging ownership and alert-skip behavior

• Updates the delivery design notes to reflect that Telegram handled rejections produce a single WARNING from the publisher, classification also considers migrate_to_chat_id/status mappings, and ops alerts are skipped when the shared channel is unavailable.

docs/design.md

Other (1) +14 / -1

telegram_error_classification.jsonAdd Telegram migrate_to_chat_id and channel-unavailability reason metadata +14/-1

Add Telegram migrate_to_chat_id and channel-unavailability reason metadata

• Extends Telegram classification reference data with 'parameter_reasons' for migrate_to_chat_id and a curated list of reasons that indicate the channel is not usable (token/chat/permission-level failures).

weather_briefing/data/telegram_error_classification.json

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 39 rules

1. Import-time data load ✓ Resolved 🐞 Bug ☼ Reliability

Description

weather_briefing.publishers now calls telegram_error_classification() at module import, forcing
packaged JSON I/O and validation on every startup even when Telegram delivery is not selected. This
unnecessarily broadens the failure surface (a missing/malformed packaged resource would prevent
stdout-only runs from starting) and adds avoidable startup work.

Code

weather_briefing/publishers.py[R19-20]

+_SAFE_DELIVERY_REASON = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*")
+telegram_error_classification()

Relevance

⭐ Low

Team favors import-time reference-data validation/startup-fail-fast (PR #86, #77); similar
import-time calls were accepted.

PR-#86
PR-#77
PR-#84

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The publishers module performs a module-scope call to telegram_error_classification(), and that
function reads packaged JSON via importlib.resources (I/O) and can raise ReferenceDataError if
the resource is missing/malformed. Because the CLI imports from weather_briefing.publishers
unconditionally, the import-time I/O/validation happens even when using non-Telegram publishers.

weather_briefing/publishers.py[18-21]
weather_briefing/reference_data.py[102-114]
weather_briefing/reference_data.py[173-187]
weather_briefing/cli.py[39-42]
PR-#86

Agent prompt

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

### Issue description
`weather_briefing.publishers` executes `telegram_error_classification()` at import time, which triggers reference-data file reads and validation. This adds startup I/O and makes all code paths importing `publishers` depend on the Telegram classification resource being present/valid, even when Telegram is not configured.

### Issue Context
The CLI imports `DeliveryProvider`/`StdoutPublisher`/`TelegramPublisher` from `weather_briefing.publishers`, so the module-level call runs regardless of which publisher is configured.

### Fix Focus Areas
- weather_briefing/publishers.py[18-21]
- weather_briefing/cli.py[394-411]

### Suggested fix
Remove the module-level `telegram_error_classification()` call.

If you still want early validation when Telegram is configured, trigger it in a Telegram-only path, e.g.:
- in `TelegramPublisher.__init__` (call `telegram_error_classification()` once), or
- in `_build_telegram_publisher()` in `cli.py` before constructing the publisher.

This preserves fail-fast behavior for Telegram deployments without impacting stdout-only runs.

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

---