[07/10] refactor: introduce delivery package

[IceCodeNew]

Summary

• separate delivery contracts, renderers, stdout transport, and Telegram adapter
• move Telegram reference validation beside its consumer
• keep thin compatibility exports for existing application imports
• reject invalid Telegram message split limits

Dependency

Based on #94. Merge as step 07 after #94.

Verification

• prek run --all-files
• 889 tests passed
• line coverage: 99.85%
• branch coverage: 99.55%

Summary by CodeRabbit

• New Features

  • Added consistent weather briefing delivery for standard output and Telegram.
  • Added plain-text and Telegram HTML formatting for briefings, articles, warnings, disasters, advice, and operational alerts.
  • Long Telegram messages are automatically split while preserving formatting and readability.
  • Added improved Telegram delivery error reporting and notification controls.

• Bug Fixes

  • Improved handling of message limits, line breaks, markup, and invalid message-size settings.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e8201df5-fb37-4fc4-ac49-cf7f2d972495

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR introduces a dedicated delivery package containing rendering, publishing, diagnostics, Telegram error classification, and message chunking. Legacy publisher and renderer modules become compatibility re-exports, while reference-data loading and related tests move to their new module boundaries.

Changes

Delivery layer extraction

Layer / File(s)	Summary
Delivery contracts weather_briefing/delivery/base.py	Adds publisher and diagnostics protocols, DeliveryProvider, structured DeliveryError, and guarded rendered-text logging.
Rendering implementations weather_briefing/delivery/renderers.py	Adds localized Telegram HTML and plain-text renderers for briefings, articles, and alerts.
Telegram delivery and chunking weather_briefing/delivery/telegram.py, weather_briefing/delivery/telegram_reference.py, tests/test_publishers.py	Adds Telegram publishing, error classification, validated HTML splitting, and expanded chunking/error-metadata tests.
Reference-data migration weather_briefing/reference_data.py, tests/test_reference_data.py	Moves Telegram classification loading and adds validated Open-Meteo weather-code descriptions.
Package and compatibility exports weather_briefing/delivery/__init__.py, weather_briefing/delivery/stdout.py, weather_briefing/publishers.py, weather_briefing/render.py, tests/test_render.py	Adds package exports and stdout publishing while preserving legacy publisher and renderer import paths.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DeliveryProvider
  participant TelegramPublisher
  participant TelegramBotAPI
  DeliveryProvider->>TelegramPublisher: Publish rendered message
  TelegramPublisher->>TelegramPublisher: Split HTML when required
  TelegramPublisher->>TelegramBotAPI: Send HTML chunks
  TelegramBotAPI-->>TelegramPublisher: Return response
  TelegramPublisher-->>DeliveryProvider: Report success or DeliveryError

Loading

Possibly related PRs

• IceCodeNew/weather-briefing#42: Updates Telegram HTML chunking behavior and its publisher test coverage.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: introducing a new delivery package.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/weather-refactor-03-delivery

---

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.}

[IceCodeNew]

/agentic_review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.81%. Comparing base (90a3ccd) to head (ef9da17).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #95   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          70       76    +6     
  Lines        9323     9358   +35     
  Branches      553      554    +1     
=======================================
+ Hits         9306     9341   +35     
  Misses         12       12           
  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): 46 rules

1. Patching telegram.telegram_error_classification ✓ Resolved 📘 Rule violation ▣ Testability

Description

The tests patch telegram_error_classification and load_reference_data at their import sites
(weather_briefing.delivery.telegram and weather_briefing.delivery.telegram_reference) instead of
patching the functions in their defining modules, making the patches brittle to import/refactor
changes. This violates the requirement to patch behavior at the defining module rather than where it
is imported.

Code

tests/test_publishers.py[76]

+    monkeypatch.setattr("weather_briefing.delivery.telegram.telegram_error_classification", fail_validation)

Relevance

⭐⭐⭐ High

Team has accepted test-hardening changes to reduce brittleness/flakiness; patch-path cleanup likely
welcomed.

PR-#11
PR-#92

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

tests/test_publishers.py patches
weather_briefing.delivery.telegram.telegram_error_classification, but that symbol is imported into
weather_briefing.delivery.telegram from its defining module
weather_briefing.delivery.telegram_reference, so the patch is applied to an imported binding
rather than the definition. Similarly, tests/test_reference_data.py patches
weather_briefing.delivery.telegram_reference.load_reference_data, yet load_reference_data is
defined in weather_briefing.data.resources and only imported into
weather_briefing.delivery.telegram_reference, so the checklist requirement is met only by patching
the defining module paths directly.

Rule 2274647: Patch behavior at its defining module, not where it is imported
tests/test_publishers.py[72-80]
weather_briefing/delivery/telegram_reference.py[26-33]
tests/test_reference_data.py[216-221]
weather_briefing/data/resources.py[19-28]

Agent prompt

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

## Issue description
Some tests patch functions via import-site paths (imported bindings) instead of patching those functions in their defining modules: `tests/test_publishers.py` patches `weather_briefing.delivery.telegram.telegram_error_classification` even though it is defined in `weather_briefing.delivery.telegram_reference`, and `tests/test_reference_data.py` patches `weather_briefing.delivery.telegram_reference.load_reference_data` even though it is defined in `weather_briefing.data.resources`. This violates the patch-targeting rule and makes tests fragile to refactors/import rewiring.

## Issue Context
- `weather_briefing.delivery.telegram` imports `telegram_error_classification` from `weather_briefing.delivery.telegram_reference`, so patching `weather_briefing.delivery.telegram.telegram_error_classification` targets an imported name.
- `weather_briefing.delivery.telegram_reference` imports `load_reference_data` from `weather_briefing.data.resources`, so patching `weather_briefing.delivery.telegram_reference.load_reference_data` targets an imported name.
Update the tests to patch the functions at their defining module paths instead of the modules that merely import them.

## Fix Focus Areas
- tests/test_publishers.py[72-80]
- weather_briefing/delivery/telegram.py[10-35]
- weather_briefing/delivery/telegram_reference.py[26-33]
- tests/test_reference_data.py[216-221]
- weather_briefing/delivery/telegram_reference.py[11-33]
- weather_briefing/data/resources.py[19-28]

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

---

2. Empty split drops message ✓ Resolved 🐞 Bug ≡ Correctness

Description

split_message() can return an empty tuple for oversized HTML that contains only tags (no
text/entities), because _finish_chunk() refuses to flush chunks when _visible_length==0. When
TelegramPublisher.publish() is called with single_message=False, it then iterates zero chunks and
sends no request, silently dropping the message.

Code

weather_briefing/delivery/telegram.py[R212-225]

+    def finish(self) -> tuple[str, ...]:
+        """Flush and return all accumulated chunks."""
+        self._finish_chunk()
+        return tuple(self._chunks)
+
+    def _append_entity(self, value: str) -> None:
+        if self._visible_length == self._limit:
+            self._finish_chunk()
+        self._parts.append(value)
+        self._visible_length += 1
+
+    def _finish_chunk(self) -> None:
+        if self._visible_length == 0:
+            return

Relevance

⭐⭐⭐ High

Silently dropping a message on edge-case input is a concrete correctness bug; similar defensive
fixes are usually accepted.

PR-#84
PR-#98

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The chunker only appends a chunk when _visible_length > 0, but HTML tags don’t increase
_visible_length, so an oversized body with only tags can produce zero chunks; the publisher then
iterates over that empty tuple and performs no HTTP calls.

weather_briefing/delivery/telegram.py[150-159]
weather_briefing/delivery/telegram.py[173-184]
weather_briefing/delivery/telegram.py[212-229]
weather_briefing/delivery/telegram.py[37-62]

Agent prompt

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

### Issue description
`split_message()` can return `()` (no chunks) when the body is larger than `limit` but contains no visible characters (e.g., lots of HTML tags with empty text). `TelegramPublisher.publish()` then loops over zero chunks and returns without calling the Telegram API, silently dropping the message.

### Issue Context
- This only affects the splitting path (`single_message=False`).
- Root cause is that `_TelegramHTMLChunker._finish_chunk()` is a no-op when `_visible_length == 0`, even if markup has been accumulated.

### Fix Focus Areas
- weather_briefing/delivery/telegram.py[150-159]
- weather_briefing/delivery/telegram.py[212-229]
- tests/test_publishers.py[95-103]

### Suggested fix
After `chunker.finish()`, if the returned tuple is empty but `body` is non-empty, return `(body,)` (or alternatively raise a `ValueError` to avoid silent success). Add a regression test that passes an oversized markup-only body and asserts a non-empty result.

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

---

3. Zero-limit split hangs ✓ Resolved 🐞 Bug ☼ Reliability

Description

weather_briefing.delivery.telegram.split_message() does not validate limit; when limit <= 0 and body
is non-empty, _TelegramHTMLChunker.handle_data() never consumes input (slicing by 0/negative) and
can loop forever, hanging the caller. This is currently avoided by TelegramPublisher (uses 4096) but
is reachable by direct callers and future misconfiguration/refactors.

Code

weather_briefing/delivery/telegram.py[R150-202]

+def split_message(body: str, limit: int) -> tuple[str, ...]:
+    """Split Telegram HTML into independently valid chunks."""
+    if len(body) <= limit:
+        return (body,)
+    chunker = _TelegramHTMLChunker(limit)
+    chunker.feed(body)
+    chunker.close()
+    return chunker.finish()
+
+
+class _TelegramHTMLChunker(HTMLParser):
+    """Split Telegram HTML while making every chunk independently valid."""
+
+    def __init__(self, limit: int) -> None:
+        super().__init__(convert_charrefs=False)
+        self._limit = limit
+        self._chunks: list[str] = []
+        self._parts: list[str] = []
+        self._open_tags: list[tuple[str, str]] = []
+        self._visible_length = 0
+
+    def handle_starttag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None:
+        if self._visible_length == self._limit:
+            self._finish_chunk()
+        start_tag = self.get_starttag_text()
+        assert start_tag is not None
+        self._parts.append(start_tag)
+        self._open_tags.append((tag, start_tag))
+
+    def handle_endtag(self, tag: str) -> None:
+        self._parts.append(f"</{tag}>")
+        self._open_tags.pop()
+
+    def handle_data(self, data: str) -> None:
+        while True:
+            available = self._limit - self._visible_length
+            if available == 0:
+                self._finish_chunk()
+                available = self._limit
+            if len(data) <= available:
+                self._parts.append(data)
+                self._visible_length += len(data)
+                return
+            split_at = data.rfind("\n", 0, available + 1)
+            if split_at > 0:
+                self._parts.append(data[:split_at])
+                self._visible_length += split_at
+                data = data[split_at:]
+            else:
+                self._parts.append(data[:available])
+                self._visible_length += available
+                data = data[available:]
+            self._finish_chunk()

Relevance

⭐⭐⭐ High

Team often accepts input validation preventing hangs/indefinite stalls; add non-positive limit guard
to avoid infinite loop.

PR-#92

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation accepts any integer limit and passes it into _TelegramHTMLChunker; when limit is
0, handle_data sets available to 0 and then slices with data[:available] / data[available:], which
does not reduce the string, so the loop cannot terminate.

weather_briefing/delivery/telegram.py[150-202]

Agent prompt

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

### Issue description
`split_message(body, limit)` can hang indefinitely when `limit <= 0` and `body` is non-empty because `_TelegramHTMLChunker.handle_data()` never makes progress (the `data = data[available:]` step doesn’t shrink `data` when `available` is 0, and negative limits can also produce non-progressing slices).

### Issue Context
`TelegramPublisher` currently passes `MAX_MESSAGE_LENGTH = 4096`, so this is primarily an API-contract/robustness gap for direct callers (and a footgun if a future refactor passes through a configured limit).

### Fix Focus Areas
- weather_briefing/delivery/telegram.py[150-202]

### Proposed fix
- Add an explicit guard at the start of `split_message` (and/or `_TelegramHTMLChunker.__init__`) such as:
 - `if limit <= 0: raise ValueError("limit must be a positive integer")`
- Add a small unit test to assert `split_message("x", 0)` and `split_message("x", -1)` raise `ValueError` (or your preferred error type).

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

---

View more (1)

4. Tests patch private loader ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new tests monkeypatch the private function
weather_briefing.data.resources._load_reference_data(), coupling them to an internal implementation
detail rather than the dependency actually consumed by telegram_error_classification(). This makes
the tests brittle to internal refactors of the reference-data loader even when public behavior is
unchanged.

Code

tests/test_publishers.py[R74-75]

+    monkeypatch.setattr("weather_briefing.data.resources._load_reference_data", lambda filename: {})
+    telegram_error_classification.cache_clear()

Relevance

⭐⭐ Medium

Recent merged tests also monkeypatch _load_reference_data; team may view it as acceptable seam for
reference-data caching.

PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both tests patch the private _load_reference_data helper even though the public wrapper
load_reference_data() is the stable API; this creates unnecessary coupling to internal delegation
details.

tests/test_publishers.py[73-80]
tests/test_reference_data.py[216-221]
weather_briefing/data/resources.py[19-33]

Agent prompt

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

### Issue description
Tests are monkeypatching `weather_briefing.data.resources._load_reference_data` (a private, internal helper). This couples tests to the current internal implementation of `load_reference_data()` and can break tests during internal refactors that do not change the public behavior under test.

### Issue Context
`telegram_error_classification()` consumes `load_reference_data()` (imported into `weather_briefing.delivery.telegram_reference`), not `_load_reference_data()` directly.

### Fix
Patch the dependency at the consumption site instead:
- In `tests/test_publishers.py`, patch `weather_briefing.delivery.telegram_reference.load_reference_data` to return `{}` (or the desired dict) and keep `telegram_error_classification.cache_clear()`.
- In `tests/test_reference_data.py`, similarly patch `weather_briefing.delivery.telegram_reference.load_reference_data` to return the parametrized `value`.

### Fix Focus Areas
- tests/test_publishers.py[73-80]
- tests/test_reference_data.py[216-221]

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

---

Previous review results

Review updated until commit ef9da17 ⚖️ Balanced

Results up to commit b3fa6f7 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Zero-limit split hangs ✓ Resolved 🐞 Bug ☼ Reliability

Description

weather_briefing.delivery.telegram.split_message() does not validate limit; when limit <= 0 and body
is non-empty, _TelegramHTMLChunker.handle_data() never consumes input (slicing by 0/negative) and
can loop forever, hanging the caller. This is currently avoided by TelegramPublisher (uses 4096) but
is reachable by direct callers and future misconfiguration/refactors.

Code

weather_briefing/delivery/telegram.py[R150-202]

+def split_message(body: str, limit: int) -> tuple[str, ...]:
+    """Split Telegram HTML into independently valid chunks."""
+    if len(body) <= limit:
+        return (body,)
+    chunker = _TelegramHTMLChunker(limit)
+    chunker.feed(body)
+    chunker.close()
+    return chunker.finish()
+
+
+class _TelegramHTMLChunker(HTMLParser):
+    """Split Telegram HTML while making every chunk independently valid."""
+
+    def __init__(self, limit: int) -> None:
+        super().__init__(convert_charrefs=False)
+        self._limit = limit
+        self._chunks: list[str] = []
+        self._parts: list[str] = []
+        self._open_tags: list[tuple[str, str]] = []
+        self._visible_length = 0
+
+    def handle_starttag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None:
+        if self._visible_length == self._limit:
+            self._finish_chunk()
+        start_tag = self.get_starttag_text()
+        assert start_tag is not None
+        self._parts.append(start_tag)
+        self._open_tags.append((tag, start_tag))
+
+    def handle_endtag(self, tag: str) -> None:
+        self._parts.append(f"</{tag}>")
+        self._open_tags.pop()
+
+    def handle_data(self, data: str) -> None:
+        while True:
+            available = self._limit - self._visible_length
+            if available == 0:
+                self._finish_chunk()
+                available = self._limit
+            if len(data) <= available:
+                self._parts.append(data)
+                self._visible_length += len(data)
+                return
+            split_at = data.rfind("\n", 0, available + 1)
+            if split_at > 0:
+                self._parts.append(data[:split_at])
+                self._visible_length += split_at
+                data = data[split_at:]
+            else:
+                self._parts.append(data[:available])
+                self._visible_length += available
+                data = data[available:]
+            self._finish_chunk()

Relevance

⭐⭐⭐ High

Team often accepts input validation preventing hangs/indefinite stalls; add non-positive limit guard
to avoid infinite loop.

PR-#92

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation accepts any integer limit and passes it into _TelegramHTMLChunker; when limit is
0, handle_data sets available to 0 and then slices with data[:available] / data[available:], which
does not reduce the string, so the loop cannot terminate.

weather_briefing/delivery/telegram.py[150-202]

Agent prompt

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

### Issue description
`split_message(body, limit)` can hang indefinitely when `limit <= 0` and `body` is non-empty because `_TelegramHTMLChunker.handle_data()` never makes progress (the `data = data[available:]` step doesn’t shrink `data` when `available` is 0, and negative limits can also produce non-progressing slices).

### Issue Context
`TelegramPublisher` currently passes `MAX_MESSAGE_LENGTH = 4096`, so this is primarily an API-contract/robustness gap for direct callers (and a footgun if a future refactor passes through a configured limit).

### Fix Focus Areas
- weather_briefing/delivery/telegram.py[150-202]

### Proposed fix
- Add an explicit guard at the start of `split_message` (and/or `_TelegramHTMLChunker.__init__`) such as:
 - `if limit <= 0: raise ValueError("limit must be a positive integer")`
- Add a small unit test to assert `split_message("x", 0)` and `split_message("x", -1)` raise `ValueError` (or your preferred error type).

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

---

Results up to commit a5dbf7c ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Empty split drops message ✓ Resolved 🐞 Bug ≡ Correctness

Description

split_message() can return an empty tuple for oversized HTML that contains only tags (no
text/entities), because _finish_chunk() refuses to flush chunks when _visible_length==0. When
TelegramPublisher.publish() is called with single_message=False, it then iterates zero chunks and
sends no request, silently dropping the message.

Code

weather_briefing/delivery/telegram.py[R212-225]

+    def finish(self) -> tuple[str, ...]:
+        """Flush and return all accumulated chunks."""
+        self._finish_chunk()
+        return tuple(self._chunks)
+
+    def _append_entity(self, value: str) -> None:
+        if self._visible_length == self._limit:
+            self._finish_chunk()
+        self._parts.append(value)
+        self._visible_length += 1
+
+    def _finish_chunk(self) -> None:
+        if self._visible_length == 0:
+            return

Relevance

⭐⭐⭐ High

Silently dropping a message on edge-case input is a concrete correctness bug; similar defensive
fixes are usually accepted.

PR-#84
PR-#98

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The chunker only appends a chunk when _visible_length > 0, but HTML tags don’t increase
_visible_length, so an oversized body with only tags can produce zero chunks; the publisher then
iterates over that empty tuple and performs no HTTP calls.

weather_briefing/delivery/telegram.py[150-159]
weather_briefing/delivery/telegram.py[173-184]
weather_briefing/delivery/telegram.py[212-229]
weather_briefing/delivery/telegram.py[37-62]

Agent prompt

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

### Issue description
`split_message()` can return `()` (no chunks) when the body is larger than `limit` but contains no visible characters (e.g., lots of HTML tags with empty text). `TelegramPublisher.publish()` then loops over zero chunks and returns without calling the Telegram API, silently dropping the message.

### Issue Context
- This only affects the splitting path (`single_message=False`).
- Root cause is that `_TelegramHTMLChunker._finish_chunk()` is a no-op when `_visible_length == 0`, even if markup has been accumulated.

### Fix Focus Areas
- weather_briefing/delivery/telegram.py[150-159]
- weather_briefing/delivery/telegram.py[212-229]
- tests/test_publishers.py[95-103]

### Suggested fix
After `chunker.finish()`, if the returned tuple is empty but `body` is non-empty, return `(body,)` (or alternatively raise a `ValueError` to avoid silent success). Add a regression test that passes an oversized markup-only body and asserts a non-empty result.

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

---

2. Patching telegram.telegram_error_classification ✓ Resolved 📘 Rule violation ▣ Testability

Description

The tests patch telegram_error_classification and load_reference_data at their import sites
(weather_briefing.delivery.telegram and weather_briefing.delivery.telegram_reference) instead of
patching the functions in their defining modules, making the patches brittle to import/refactor
changes. This violates the requirement to patch behavior at the defining module rather than where it
is imported.

Code

tests/test_publishers.py[76]

+    monkeypatch.setattr("weather_briefing.delivery.telegram.telegram_error_classification", fail_validation)

Relevance

⭐⭐⭐ High

Team has accepted test-hardening changes to reduce brittleness/flakiness; patch-path cleanup likely
welcomed.

PR-#11
PR-#92

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

tests/test_publishers.py patches
weather_briefing.delivery.telegram.telegram_error_classification, but that symbol is imported into
weather_briefing.delivery.telegram from its defining module
weather_briefing.delivery.telegram_reference, so the patch is applied to an imported binding
rather than the definition. Similarly, tests/test_reference_data.py patches
weather_briefing.delivery.telegram_reference.load_reference_data, yet load_reference_data is
defined in weather_briefing.data.resources and only imported into
weather_briefing.delivery.telegram_reference, so the checklist requirement is met only by patching
the defining module paths directly.

Rule 2274647: Patch behavior at its defining module, not where it is imported
tests/test_publishers.py[72-80]
weather_briefing/delivery/telegram_reference.py[26-33]
tests/test_reference_data.py[216-221]
weather_briefing/data/resources.py[19-28]

Agent prompt

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

## Issue description
Some tests patch functions via import-site paths (imported bindings) instead of patching those functions in their defining modules: `tests/test_publishers.py` patches `weather_briefing.delivery.telegram.telegram_error_classification` even though it is defined in `weather_briefing.delivery.telegram_reference`, and `tests/test_reference_data.py` patches `weather_briefing.delivery.telegram_reference.load_reference_data` even though it is defined in `weather_briefing.data.resources`. This violates the patch-targeting rule and makes tests fragile to refactors/import rewiring.

## Issue Context
- `weather_briefing.delivery.telegram` imports `telegram_error_classification` from `weather_briefing.delivery.telegram_reference`, so patching `weather_briefing.delivery.telegram.telegram_error_classification` targets an imported name.
- `weather_briefing.delivery.telegram_reference` imports `load_reference_data` from `weather_briefing.data.resources`, so patching `weather_briefing.delivery.telegram_reference.load_reference_data` targets an imported name.
Update the tests to patch the functions at their defining module paths instead of the modules that merely import them.

## Fix Focus Areas
- tests/test_publishers.py[72-80]
- weather_briefing/delivery/telegram.py[10-35]
- weather_briefing/delivery/telegram_reference.py[26-33]
- tests/test_reference_data.py[216-221]
- weather_briefing/delivery/telegram_reference.py[11-33]
- weather_briefing/data/resources.py[19-28]

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

---

Results up to commit 8af30ee ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Tests patch private loader ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new tests monkeypatch the private function
weather_briefing.data.resources._load_reference_data(), coupling them to an internal implementation
detail rather than the dependency actually consumed by telegram_error_classification(). This makes
the tests brittle to internal refactors of the reference-data loader even when public behavior is
unchanged.

Code

tests/test_publishers.py[R74-75]

+    monkeypatch.setattr("weather_briefing.data.resources._load_reference_data", lambda filename: {})
+    telegram_error_classification.cache_clear()

Relevance

⭐⭐ Medium

Recent merged tests also monkeypatch _load_reference_data; team may view it as acceptable seam for
reference-data caching.

PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both tests patch the private _load_reference_data helper even though the public wrapper
load_reference_data() is the stable API; this creates unnecessary coupling to internal delegation
details.

tests/test_publishers.py[73-80]
tests/test_reference_data.py[216-221]
weather_briefing/data/resources.py[19-33]

Agent prompt

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

### Issue description
Tests are monkeypatching `weather_briefing.data.resources._load_reference_data` (a private, internal helper). This couples tests to the current internal implementation of `load_reference_data()` and can break tests during internal refactors that do not change the public behavior under test.

### Issue Context
`telegram_error_classification()` consumes `load_reference_data()` (imported into `weather_briefing.delivery.telegram_reference`), not `_load_reference_data()` directly.

### Fix
Patch the dependency at the consumption site instead:
- In `tests/test_publishers.py`, patch `weather_briefing.delivery.telegram_reference.load_reference_data` to return `{}` (or the desired dict) and keep `telegram_error_classification.cache_clear()`.
- In `tests/test_reference_data.py`, similarly patch `weather_briefing.delivery.telegram_reference.load_reference_data` to return the parametrized `value`.

### Fix Focus Areas
- tests/test_publishers.py[73-80]
- tests/test_reference_data.py[216-221]

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

---

Results up to commit c8b323b ⚖️ Balanced

No changes from previous review

Results up to commit ef9da17 ⚖️ Balanced

No changes from previous review

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit a5dbf7c

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8af30ee

[qodo-code-review]

PR Summary by Qodo

Refactor delivery into dedicated package; harden Telegram chunking and metadata validation

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract delivery contracts, renderers, and transports into a new weather_briefing.delivery
 package.
• Keep compatibility re-exports for existing imports while migrating internals.
• Harden Telegram delivery: validated error classification, safe message splitting, and limit
 checks.

Diagram

graph TD
  App["Application"] --> Provider["DeliveryProvider"] --> Renderer["Renderers"] --> Msg["RenderedMessage"]
  Provider --> Stdout["StdoutPublisher"]
  Provider --> Telegram["TelegramPublisher"] --> TGAPI{{"Telegram Bot API"}}
  Telegram --> Ref[("Telegram refs")]

  subgraph Legend
    direction LR
    _mod["Module"] ~~~ _db[("Reference data")] ~~~ _ext{{"External API"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep delivery co-located in `publishers.py` and `render.py`

• ➕ Fewer new modules and import-path changes
• ➕ Less churn for readers familiar with the old layout
• ➖ Single file grows into mixed concerns (composition, transport, HTML chunking, reference validation)
• ➖ Harder to test/patch ownership boundaries cleanly (as seen in updated monkeypatch targets)

2. Use a dedicated HTML tokenizer/parser library for Telegram chunking

• ➕ Potentially more robust handling of malformed HTML and edge-cases
• ➕ May simplify state management vs a custom HTMLParser subclass
• ➖ Adds dependency/complexity for a narrowly-scoped use-case
• ➖ Still requires Telegram-specific constraints (visible-length semantics, tag re-opening/closing)

Recommendation: The PR’s approach (new delivery/ package + compatibility re-exports) is a good balance: it reduces module coupling while avoiding a flag-day migration. The custom HTMLParser-based chunking is appropriate given Telegram’s constraints and is now hardened with positive-limit validation and a safe fallback for markup-only content.

Files changed (12) +753 / -676

Bug fix (1) +229 / -0

telegram.pyExtract Telegram publisher with validated errors and robust HTML chunking +229/-0

Extract Telegram publisher with validated errors and robust HTML chunking

• Moves Telegram Bot API publishing into 'delivery/telegram.py', including safe error classification, optional sensitive-text diagnostics, and HTML message splitting. Adds validation for non-positive split limits and a fallback to avoid dropping markup-only messages.

weather_briefing/delivery/telegram.py

Refactor (8) +498 / -661

__init__.pyIntroduce delivery package public API +16/-0

Introduce delivery package public API

• Defines the 'weather_briefing.delivery' package and re-exports the core contracts, renderers, and publishers as the new primary import surface.

weather_briefing/delivery/init.py

base.pyAdd platform-neutral delivery composition and safe error contract +121/-0

Add platform-neutral delivery composition and safe error contract

• Introduces 'DeliveryProvider' to compose a 'MessageRenderer' with a transport 'Publisher', including limit clamping and optional sensitive rendered-text diagnostics. Defines 'DeliveryError' with validated kebab-case reasons and shared rendered-text logging helpers.

weather_briefing/delivery/base.py

renderers.pyMove and formalize PlainText and Telegram HTML renderers +219/-0

Move and formalize PlainText and Telegram HTML renderers

• Implements 'PlainTextRenderer' and 'TelegramHTMLRenderer' plus a 'MessageRenderer' protocol inside the delivery package. Preserves previous formatting behavior while centralizing localization and visible-length calculation.

weather_briefing/delivery/renderers.py

stdout.pyExtract stdout publisher adapter +19/-0

Extract stdout publisher adapter

• Moves stdout publishing into a dedicated adapter that prints rendered bodies and ignores delivery hints.

weather_briefing/delivery/stdout.py

telegram_reference.pyAdd validated Telegram error classification reference loader +99/-0

Add validated Telegram error classification reference loader

• Relocates Telegram error classification validation next to its consumer and enforces strict schema/format checks for markers, parameters, status mappings, and channel-unavailable reason sets.

weather_briefing/delivery/telegram_reference.py

publishers.pyReplace publishers implementation with compatibility re-exports +19/-352

Replace publishers implementation with compatibility re-exports

• Removes the previous monolithic delivery/publisher implementation and re-exports the new delivery primitives to preserve existing import paths ('weather_briefing.publishers.*').

weather_briefing/publishers.py

reference_data.pyRemove Telegram classification from legacy reference exports +1/-92

Remove Telegram classification from legacy reference exports

• Drops Telegram error classification types/functions from the legacy 'reference_data' compatibility module, leaving only weather-related reference helpers and exports.

weather_briefing/reference_data.py

render.pyReplace render implementation with compatibility re-exports +4/-217

Replace render implementation with compatibility re-exports

• Removes the prior renderer implementations and re-exports delivery renderers and the 'MessageRenderer' protocol to keep existing application imports working.

weather_briefing/render.py

Tests (3) +26 / -15

test_publishers.pyUpdate publisher tests to use new delivery modules and Telegram splitter +22/-12

Update publisher tests to use new delivery modules and Telegram splitter

• Switches imports from legacy 'publishers'/'render' modules to 'weather_briefing.delivery'. Updates monkeypatching to patch reference-data loading at its owning module and adds new coverage for Telegram split edge cases (markup-only bodies and non-positive limits).

tests/test_publishers.py

test_reference_data.pyMove Telegram classification tests to delivery reference module +3/-2

Move Telegram classification tests to delivery reference module

• Redirects 'telegram_error_classification' imports to 'weather_briefing.delivery.telegram_reference' and patches 'data.resources.load_reference_data' at the resource owner for validation tests.

tests/test_reference_data.py

test_render.pyAdopt delivery renderers in render tests +1/-1

Adopt delivery renderers in render tests

• Updates renderer imports to come from the new 'weather_briefing.delivery' package rather than the legacy 'render' module.

tests/test_render.py

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8af30ee

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c8b323b

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c8b323b

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit ef9da17

[qodo-code-review]

Code review by qodo was updated up to the latest commit ef9da17