refactor: extract notification decisions and split core modules

[IceCodeNew]

Summary

• extract a portable, typed notification_decision package with explicit message kinds, policy registration, message-specific assessment builders, and separate prompts
• preserve platform-neutral notification baselines and service-status impact surfaces so later decisions compare complete prior facts instead of renderer-specific output
• split oversized weather parsing, delivery rendering, AnyLLM transport, persistence, CLI, scheduling, diagnostics, and composition modules by reason to change
• preserve forecast delivery, manual forced delivery, final-window silent delivery, transaction boundaries, handled revisions, and per-publisher retry semantics

Review guide

The first 9 dependency-ordered commits build the refactor from parser and renderer splits through the reusable decision core, message-specific adapters, persistence, composition, and design contract. Twenty-one focused follow-up commits address independently validated review findings without rewriting reviewed history, including optional weather enrichment, localization, assessment immutability, skipped rendering, prompt and transaction ownership, timestamp validation, diagnostics boundaries, forced delivery, durable notification comparison state, service-status surfaces, policy registration, strict stored-state validation, and policy result validation.

Validation

• prek run --all-files
• uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml
• 1337 tests passed
• line coverage 99.91% (baseline 99.90%)
• branch coverage 99.68% (baseline 99.66%)
• exact-SHA local reviews clean: GPT-5.6 sol max and OpenCode/Zhipu GLM-5.2 max
• no added Any, cast(), or type-checker suppression

Summary by CodeRabbit

• New Features

  • Refined notification decisions for weather and service-status, including cumulative change tracking and tighter delivery exceptions.
  • Added support for notification prompt policies per notification type and enhanced service-status monitoring context (including handled surfaces).
  • Improved briefing output rendering options (plain-text and compact Bark), plus streamlined delivery composition.
  • Added a richer command-line parser with validated diagnostic duration inputs.

• Bug Fixes

  • Hardened persistence/SQLite validation, legacy migrations, and rollback safety; improved timestamp and JSON payload validation.

• Documentation

  • Updated design and prompt/system-rule documentation to reflect the new notification gating behavior and output schema.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR separates notification decisions from briefing generation, centralizes LLM transport, splits persistence and rendering responsibilities, extracts weather parsing modules, and reorganizes CLI composition, scheduling, diagnostics, prompts, and tests.

Changes

Application and notification flow

Layer / File(s)	Summary
Notification contracts and orchestration weather_briefing/notification_decision/*, weather_briefing/application/*, weather_briefing/service.py, weather_briefing/service_status/*	Adds validated notification assessments and policies, separates briefing validation from summarization, applies notification gating after candidate generation, and persists service-status surfaces.
Notification integration tests tests/test_notification_decision.py, tests/test_service.py, tests/test_service_status.py	Covers policy validation, bounded assessment payloads, forced and forecast bypasses, candidate baselines, and corrupted service-status state.

LLM transport and delivery

Layer / File(s)	Summary
Structured LLM transport weather_briefing/llm/*	Moves structured request construction, error normalization, resource cleanup, fallback delegation, and notification decision calls into dedicated contracts and transport helpers.
Rendering and provider composition weather_briefing/delivery/*, weather_briefing/composition/*, weather_briefing/cli.py	Adds shared rendering primitives, platform-specific plain-text/HTML/Bark renderers, provider factories, notification-policy composition, and runtime wiring.

Persistence and weather parsing

Layer / File(s)	Summary
SQLite state and migrations weather_briefing/persistence/*, weather_briefing/models.py, weather_briefing/state.py	Adds notification payload and handled-surface storage, schema migration, validated JSON decoding, transactional rollback behavior, and timezone-aware persistence checks.
Weather parsing and localization weather_briefing/weather/*, weather_briefing/localization.py, weather_briefing/data/localization.json	Extracts Open-Meteo and QWeather parsing, validation, formatting, air-quality/allergen handling, numeric checks, and localized AQI labels.

CLI, prompts, and documentation

Layer / File(s)	Summary
CLI and runtime support weather_briefing/command_parser.py, weather_briefing/scheduling.py, weather_briefing/runtime_diagnostics.py	Adds focused command parsing, Git-aware version diagnostics, scheduling helpers, UTC logging, and validated rendered-text diagnostics commands.
Prompt and design updates weather_briefing/data/*, weather_briefing/notification_decision/*.txt, docs/design.md, tests/test_prompts.py	Separates generation and notification prompts, removes should_publish from structured briefing output, and documents notification strategy, candidate persistence, service-status revision inputs, and delivery rules.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• IceCodeNew/weather-briefing#96: Overlaps the structured-output and LLM result contract changes.
• IceCodeNew/weather-briefing#114: Relates to service-status notification decision and monitoring flow changes.
• IceCodeNew/weather-briefing#121: Overlaps the AnyLLM structured request and JSON-object transport implementation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.69% 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
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main refactor: extracting notification decisions and splitting core modules.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/extract-notification-decision

---

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.87%. Comparing base (ebe0431) to head (afabe77).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #125    +/-   ##
========================================
  Coverage   99.86%   99.87%            
========================================
  Files         117      140    +23     
  Lines       12435    13241   +806     
  Branches      742      790    +48     
========================================
+ Hits        12418    13224   +806     
  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. SQLiteRuntimeDiagnostics patched at import-site ✓ Resolved 📘 Rule violation ▣ Testability ⭐ New

Description

The tests patch SQLiteRuntimeDiagnostics and FallbackLLMProvider via
weather_briefing.runtime_diagnostics and weather_briefing.composition.llm, respectively, which
are import sites rather than the symbols’ defining modules. This violates the compliance rule
requiring patches to target the defining module path, and the pattern occurs multiple times in
tests/test_cli.py.

Code

tests/test_cli.py[R968-971]

+    monkeypatch.setattr(
+        "weather_briefing.runtime_diagnostics.SQLiteRuntimeDiagnostics",
+        unavailable_diagnostics,
+    )

Relevance

●●● Strong

Team previously accepted “patch defining module, not import site” fixes (PR #101, #102; also
#99/#107).

PR-#101
PR-#102
PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited test lines patch weather_briefing.runtime_diagnostics.SQLiteRuntimeDiagnostics even
though SQLiteRuntimeDiagnostics is defined in weather_briefing/persistence/diagnostics.py, and
they patch weather_briefing.composition.llm.FallbackLLMProvider even though FallbackLLMProvider
is defined in weather_briefing/llm/fallback.py. Because these patch targets point at
import/re-export locations instead of the original defining modules, they conflict with the
compliance checklist requirement that mocks/patches must reference the defining module path rather
than an import site.

Rule 2274647: Patch behavior at its defining module, not where it is imported
tests/test_cli.py[968-971]
tests/test_cli.py[1070-1073]
tests/test_cli.py[1144-1147]
weather_briefing/persistence/diagnostics.py[19-24]
tests/test_cli.py[1291-1298]
weather_briefing/llm/fallback.py[46-57]

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 patching `SQLiteRuntimeDiagnostics` and `FallbackLLMProvider` at import sites (`weather_briefing.runtime_diagnostics` and `weather_briefing.composition.llm`) instead of patching them at their defining modules, which violates the compliance rule that patches must target the symbol’s defining module path.

## Issue Context
- `SQLiteRuntimeDiagnostics` is defined in `weather_briefing/persistence/diagnostics.py`, but tests currently patch it via `weather_briefing.runtime_diagnostics.SQLiteRuntimeDiagnostics`.
- `FallbackLLMProvider` is defined in `weather_briefing/llm/fallback.py`, but tests currently patch it via `weather_briefing.composition.llm.FallbackLLMProvider`.
- The compliance checklist explicitly disallows patching at import/re-export sites; update patch targets to use the defining module paths.

## Fix Focus Areas
- tests/test_cli.py[968-971]
- tests/test_cli.py[1070-1073]
- tests/test_cli.py[1144-1147]
- tests/test_cli.py[1295-1298]

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

---

2. AQI dropped when PM2.5 missing ✓ Resolved 🐞 Bug ≡ Correctness

Description

weather_briefing.weather.qweather_parsing.air_quality_snapshot unconditionally indexes
concentration["value"] and concentration["unit"], so responses missing PM2.5 concentration metadata
raise KeyError and the caller drops the entire air-quality snapshot. This is inconsistent with
AirQualitySnapshot where pm25_concentration/pm25_unit are optional and can be None.

Code

weather_briefing/weather/qweather_parsing.py[R65-66]

+        pm25_concentration=_float_value(concentration["value"]),
+        pm25_unit=str(concentration["unit"]),

Relevance

●●● Strong

Team often accepts defensive weather parsing to avoid optional enrichment crashing snapshots
(accepted in PRs #99, #124).

PR-#99
PR-#124

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The parser currently raises on missing optional PM2.5 keys; the provider catches KeyError and
returns None for the whole snapshot, even though the model allows those fields to be None.

weather_briefing/weather/qweather_parsing.py[48-70]
weather_briefing/models.py[135-152]
weather_briefing/weather/qweather.py[258-306]

Agent prompt

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

### Issue description
`air_quality_snapshot()` treats PM2.5 concentration fields as mandatory by subscripting `concentration["value"]` and `concentration["unit"]`. When QWeather returns an AQI index but omits (or partially omits) the PM2.5 concentration object, this raises `KeyError` and `_fetch_air_quality()` returns `None`, discarding otherwise useful AQI data.

### Issue Context
`AirQualitySnapshot.pm25_concentration` and `pm25_unit` are explicitly optional (`float | None`, `str | None`). The parser already uses `_mapping_or_empty(..., "concentration")`, which implies the field may be absent, but then immediately requires keys.

### Fix Focus Areas
- weather_briefing/weather/qweather_parsing.py[48-70]

### How to fix
- Parse PM2.5 concentration defensively:
 - Use `.get("value")` / `.get("unit")` (or a small `try/except KeyError`) and set `pm25_concentration=None` / `pm25_unit=None` when absent.
 - If present, still validate with `_float_value` and cast unit to `str`.
- Keep the rest of the snapshot (AQI/category/guidance) intact even when PM2.5 concentration is missing.

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

---

3. Warnings not compacted ✓ Resolved 🐞 Bug ➹ Performance

Description

weather_notification_assessment forwards currently_active_warnings verbatim into
previous_active_warnings, including verbose fields like detail and source_ids. This weakens the
intended bounded/low-noise decision payload and can unnecessarily bloat the LLM
notification-decision input.

Code

weather_briefing/application/notification.py[56]

+    policy_input["previous_active_warnings"] = briefing_payload.get("currently_active_warnings", [])

Relevance

●●● Strong

Team accepts bloat/limit reductions (avoid chunking, overflow budgets); likely to compact warnings
in policy input.

PR-#107
PR-#110
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new assessment builder claims to build a bounded policy input, but it passes the warnings list
through unchanged. The upstream payload builder includes verbose warning fields (notably detail),
meaning the decision payload can carry large warning bodies even though other parts (articles) are
compacted.

weather_briefing/application/notification.py[47-57]
weather_briefing/application/payloads.py[46-88]

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_notification_assessment()` is intended to build a bounded weather notification policy payload, but it currently forwards `currently_active_warnings` verbatim as `previous_active_warnings`, including potentially long `detail` text and other fields.

### Issue Context
`build_briefing_payload()` serializes warnings with fields including `detail` and `source_ids`. Passing these through unchanged increases the decision payload size and may add noise for the notification policy when only identity/status/timestamps are typically needed.

### Fix Focus Areas
- weather_briefing/application/notification.py[47-61]
- weather_briefing/application/payloads.py[46-88]

### Suggested fix
- Add a `_compact_warnings()` helper (parallel to `_compact_articles()`), and store only a minimal schema such as: `id`, `title`, `status`, and `last_confirmed_at` (optionally: truncate `detail` to a small max length if it’s still needed for “material change” judgments).
- Replace the direct assignment with:
 - `policy_input["previous_active_warnings"] = _compact_warnings(briefing_payload.get("currently_active_warnings"))`
- Keep behavior deterministic (preserve ordering) and safe for non-list / malformed values (return `[]`).

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

---

View more (5)

4. Unvalidated assessment payload ✓ Resolved 🐞 Bug ☼ Reliability

Description

NotificationAssessment stores payload without runtime validation, so a non-Mapping payload can be
constructed successfully and then crash later when LLMPromptNotificationPolicy calls dict(payload).
This turns a configuration/integration mistake into a less actionable TypeError at decision time
instead of a clear ValueError at the API boundary.

Code

weather_briefing/notification_decision/core.py[R30-33]

+    def __init__(self, kind: object, payload: Mapping[str, object]) -> None:
+        """Validate and retain one application-owned policy identifier."""
+        object.__setattr__(self, "kind", _validated_kind(kind, context="Notification kind"))
+        object.__setattr__(self, "payload", payload)

Relevance

●●● Strong

Team often adds runtime shape checks to fail fast on invalid payloads (accepted PRs #98, #99, #114).

PR-#98
PR-#99
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor stores payload as-is without checking its runtime type, while the LLM-backed policy
later converts payload via dict(payload), which will raise TypeError for invalid payload inputs.

weather_briefing/notification_decision/core.py[23-34]
weather_briefing/notification_decision/core.py[85-91]

Agent prompt

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

## Issue description
`NotificationAssessment.__init__` validates `kind` but does not validate `payload` at runtime. This allows invalid payload types (e.g., `None`, `int`, list) to propagate until `LLMPromptNotificationPolicy.assess_notification()` calls `dict(payload)`, where it can raise `TypeError`.

## Issue Context
This module is a portable policy core, so it should fail fast with a clear, actionable error message at the boundary where assessments are constructed.

## Fix Focus Areas
- weather_briefing/notification_decision/core.py[30-33]

## Suggested fix
- Change the constructor signature to accept `payload: object` (or keep the type but still validate) and validate:
 - `isinstance(payload, Mapping)`
 - optionally: all keys are `str` (since the rest of the code assumes `Mapping[str, object]`)
- On failure, raise `ValueError` with an actionable message (e.g., `"Notification payload must be a mapping with string keys"`).
- Optionally, store `dict(payload)` to both validate-copy and decouple from later mutation.

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

---

5. Silent git subprocess failures ✓ Resolved 📘 Rule violation ☼ Reliability

Description

_display_version() invokes the git CLI via subprocess.run() but swallows spawn/non-zero
failures by returning __version__ without logging or otherwise surfacing the failure. This
violates the requirement that CLI invocations validate availability and handle failures in an
observable way.

Code

weather_briefing/command_parser.py[R72-99]

+    repository_root = Path(__file__).resolve().parents[1]
+    try:
+        git_metadata = subprocess.run(
+            (
+                "git",
+                "-C",
+                str(repository_root),
+                "rev-parse",
+                "--show-toplevel",
+                "--short=7",
+                "HEAD",
+            ),
+            check=True,
+            capture_output=True,
+            text=True,
+        ).stdout.splitlines()
+        if len(git_metadata) != 2 or Path(git_metadata[0]).resolve() != repository_root:
+            return __version__
+        revision = git_metadata[1]
+        status = subprocess.run(
+            ("git", "-C", str(repository_root), "status", "--porcelain"),
+            check=True,
+            capture_output=True,
+            text=True,
+        ).stdout
+    except (OSError, subprocess.CalledProcessError):
+        return __version__
+

Relevance

●●● Strong

Team frequently adds observable logging for fallback/failures; similar observability fixes accepted
(e.g., PR #115).

PR-#115
PR-#19
PR-#62

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2225218 requires code that invokes external CLI tools to validate availability and
handle execution failures observably. The new _display_version() runs git via
subprocess.run(check=True, capture_output=True) and catches OSError/CalledProcessError by
returning __version__ without any logging or surfaced error, making failures silent.

Rule 2225218: CLI tools invoked in code must validate availability and handle execution failures
weather_briefing/command_parser.py[67-99]

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/command_parser.py:_display_version()` calls the external `git` CLI but silently ignores failures (missing executable, non-zero exit) by returning `__version__` without any observable signal.

## Issue Context
Compliance requires external CLI invocations to validate availability and handle execution failures (including non-zero exit codes) in a way callers can observe (e.g., logging or explicit error propagation).

## Fix Focus Areas
- weather_briefing/command_parser.py[67-102]

## Suggested approach
- Add a preflight `shutil.which("git")` check before calling `subprocess.run`.
- On `OSError`/`CalledProcessError`, log a short message (e.g., `logging.getLogger(__name__).debug(...)` or a one-line `print(..., file=sys.stderr)`), then fall back to `__version__` as today.
- Consider including the `returncode` and a truncated `stderr` when available.

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

---

6. Prompt type validation missing ✓ Resolved 🐞 Bug ☼ Reliability

Description

LLMPromptNotificationPolicy.__post_init__ calls self.system_prompt.strip() without first
verifying system_prompt is a str, so a non-string value will raise AttributeError and bypass
the intended ValueError validation. This makes policy composition brittle to malformed direct
callers (despite the type annotation).

Code

weather_briefing/notification_decision/core.py[R70-71]

+        if not self.system_prompt.strip():
+            raise ValueError("Notification policy prompt must not be empty")

Relevance

●●● Strong

Repo often adds defensive type/shape validation to avoid AttributeError and raise controlled errors
(accepted in PR #98/#99).

PR-#98
PR-#99
PR-#100

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly type-checks kind before using .strip(), but uses .strip() on
system_prompt without a corresponding type check, so non-string prompts will raise at .strip()
instead of producing a controlled validation error.

weather_briefing/notification_decision/core.py[66-71]

Agent prompt

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

## Issue description
`LLMPromptNotificationPolicy.__post_init__` calls `.strip()` on `system_prompt` without ensuring it is a string, which can raise `AttributeError` and produce an unclear failure mode.

## Issue Context
`kind` is already validated with an explicit `isinstance(..., str)` check, but `system_prompt` is not.

## Fix Focus Areas
- weather_briefing/notification_decision/core.py[66-71]

## Suggested change
Update the prompt validation to also check type, e.g.:

```py
if not isinstance(self.system_prompt, str) or not self.system_prompt.strip():
   raise ValueError("Notification policy prompt must be a non-empty string")
```

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

---

7. Kind strip AttributeError ✓ Resolved 🐞 Bug ☼ Reliability

Description

NotificationAssessment.__post_init__ (and LLMPromptNotificationPolicy.__post_init__) call .strip()
on kind without first verifying it is a string, so a truthy non-string kind will raise
AttributeError and crash assessment construction. This defeats the intended “actionable ValueError”
validation behavior and makes the API boundary brittle to misconfiguration or external callers.

Code

weather_briefing/notification_decision/core.py[R24-27]

+    def __post_init__(self) -> None:
+        """Reject invalid application-owned policy identifiers."""
+        if not self.kind or self.kind != self.kind.strip():
+            raise ValueError("Notification kind must be a non-empty normalized string")

Relevance

●●● Strong

Team previously added strict non-string validation at boundaries; likely to add isinstance(str)
before strip().

PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new validation logic calls .strip() on kind in both NotificationAssessment and
LLMPromptNotificationPolicy without checking isinstance(kind, str), so non-string truthy values
will raise AttributeError before the intended ValueError is raised.

weather_briefing/notification_decision/core.py[17-71]

Agent prompt

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

## Issue description
`NotificationAssessment.__post_init__` and `LLMPromptNotificationPolicy.__post_init__` assume `kind` is a `str` and immediately call `.strip()`. If a caller passes a truthy non-string (e.g., `kind=1` from config parsing or a plugin), this throws `AttributeError` instead of the intended explicit validation error.

## Issue Context
This code is meant to reject invalid policy identifiers with a clear, application-owned error. Right now it only reliably handles `None`/empty strings; it doesn’t guard type.

## Fix Focus Areas
- weather_briefing/notification_decision/core.py[17-71]

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

---

8. Duplicate large LLM payload ✓ Resolved 🐞 Bug ➹ Performance

Description

BriefingService now performs an additional LLM call for notification decisions and passes a payload
that includes large fields like full context_documents, recent_context_documents, and
recent_briefings, duplicating much of the already-large summarization input. This increases token
usage/latency and raises the likelihood of context-limit request failures; the decision payload
should be trimmed/compacted to only the facts the notification policy needs (plus
candidate_message).

Code

weather_briefing/service.py[R317-322]

+        notification = (
+            NotificationDecision(should_notify=True)
+            if kind == "forecast"
+            else await self._notification_decisions.assess_notification(
+                weather_notification_assessment(payload, result)
+            )

Relevance

●●● Strong

Team has repeatedly trimmed/bounded LLM payload/history to avoid oversize failures; will likely
compact notification payload too.

PR-#58
PR-#33
PR-#68

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The service now calls the notification decision provider after summarization, and the assessment
payload explicitly includes large context/history fields copied from the main briefing payload,
which itself embeds full document content bodies.

weather_briefing/service.py[317-323]
weather_briefing/application/notification.py[11-35]
weather_briefing/application/payloads.py[46-87]

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 notification-decision request sends a large assessment payload (including full context document bodies) in addition to the summarization request. This can materially increase cost/latency and raise the chance of request failures for large histories.

## Issue Context
`weather_notification_assessment` currently copies multiple high-volume fields from the main briefing payload, and `build_briefing_payload` includes full `content` for `context_documents`.

## Fix Focus Areas
- weather_briefing/service.py[317-323]
- weather_briefing/application/notification.py[11-35]
- weather_briefing/application/payloads.py[46-87]

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

---

9. Prompt package name brittle ✓ Resolved 🐞 Bug ☼ Reliability

Description

notification_decision/policies.py derives the resource package name via
__name__.rpartition(".")[0], which becomes an empty string when the module is executed as
__main__, breaking prompt loading at import time. Using __package__ (with a safe fallback) or an
explicit package anchor avoids this fragile behavior.

Code

weather_briefing/notification_decision/policies.py[R7-15]

+_PACKAGE = __name__.rpartition(".")[0]
+
+
+def _load_notification_prompt(filename: str) -> str:
+    """Load one notification policy with an actionable failure."""
+    try:
+        return resources.files(_PACKAGE).joinpath(filename).read_text(encoding="utf-8")
+    except (OSError, UnicodeDecodeError) as exc:
+        raise RuntimeError(f"Unable to load notification policy: {filename}") from exc

Relevance

●●● Strong

Repo hardens packaged prompt loading and avoids fragile import-time/resource failures;
__package__/explicit anchor aligns with prior fixes.

PR-#101
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The module computes _PACKAGE from __name__ and loads prompt files immediately at import time; if
_PACKAGE is empty/incorrect, resources.files(_PACKAGE) will fail and prevent prompts from
loading.

weather_briefing/notification_decision/policies.py[1-19]

Agent prompt

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

## Issue description
Prompt loading uses `_PACKAGE = __name__.rpartition(".")[0]` and then `resources.files(_PACKAGE)`. If the module is executed as `__main__` (or otherwise has an unexpected `__name__`), `_PACKAGE` can be invalid and prompt loading fails at import time.

## Issue Context
This is low-likelihood in normal CLI usage (where the module is imported), but it’s easy to harden by anchoring resource lookup to `__package__` or to a known package string.

## Fix Focus Areas
- weather_briefing/notification_decision/policies.py[1-19]

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing

[IceCodeNew]

/agentic_review

[IceCodeNew]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Decouple notification decisions via typed policy core and per-kind prompts

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add typed notification-decision core with explicit per-kind policy dispatch.
• Split weather vs service-status inputs/prompts; remove decisions from content schema.
• Refactor CLI, composition, persistence, renderers, and weather parsing; update tests/docs.

Diagram

graph TD
  cli["CLI"] --> comp["composition/*"] --> briefing["BriefingService"]
  comp --> status["ServiceStatusMonitor"] --> decision["DecisionService"] --> llm{{"Any-LLM"}}
  briefing --> decision --> llm
  briefing --> delivery["delivery/*"]
  status --> delivery
  briefing --> db[("SQLiteStateStore")]
  status --> db

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

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Single shared decision prompt with a `kind` selector

• ➕ Fewer prompt resources to ship and version
• ➕ One prompt to tune
• ➖ Re-couples weather vs service-status semantics and inputs
• ➖ Harder to test and evolve policies independently
• ➖ Encourages the model boundary to understand routing

2. Keep `should_publish` inside the main structured briefing schema

• ➕ Single LLM call for content + decision (lower latency/cost)
• ➕ Simpler pipeline wiring
• ➖ Decision becomes coupled to content prompt/schema changes
• ➖ Harder to reuse for non-briefing message types
• ➖ More complex to validate/repair when only the decision is wrong

3. Rule-based (non-LLM) decisions for some kinds

• ➕ Deterministic and cheap for well-defined signals
• ➕ Reduces reliance on LLM availability
• ➖ Requires substantial domain logic to match current nuance
• ➖ May regress judgment quality on ambiguous textual changes

Recommendation: The PR’s approach is the best long-term boundary: callers produce NotificationAssessment(kind, payload), NotificationDecisionService routes to explicitly registered policies, and the LLM adapter only executes a given prompt+payload without knowing message types. This keeps weather vs service-status independently evolvable and allows future non-LLM policy implementations.

Files changed (57) +3348 / -1706

Enhancement (9) +629 / -114

notification.pyAdd bounded weather notification assessment builder +74/-0

Add bounded weather notification assessment builder

• Adds 'weather_notification_assessment()' that compacts briefing history/articles/warnings into metadata-only payloads and attaches the candidate message for decisioning. Avoids resending full bodies while preserving decision-relevant identity/freshness signals.

weather_briefing/application/notification.py

notifications.pyCompose notification decision service by required kinds +62/-0

Compose notification decision service by required kinds

• Adds composition for 'NotificationDecisionService' using explicit kind-to-policy builders so workflows can register only needed policies (weather and/or service_status).

weather_briefing/composition/notifications.py

bark_renderer.pyAdd Bark renderer module +94/-0

Add Bark renderer module

• Introduces a Bark-specific renderer that uses compact formatting and numbered sources, built on shared rendering primitives.

weather_briefing/delivery/bark_renderer.py

plain_renderer.pyAdd plain-text renderer module +139/-0

Add plain-text renderer module

• Adds a dedicated plain-text renderer with configurable source attribution options, replacing the implementation previously embedded in the monolithic renderers module.

weather_briefing/delivery/plain_renderer.py

__init__.pyExpose notification-decision public API +21/-0

Expose notification-decision public API

• Adds package exports for notification decision types and services, establishing a stable import surface for orchestration and composition.

weather_briefing/notification_decision/init.py

core.pyAdd portable typed notification decision core and dispatch service +136/-0

Add portable typed notification decision core and dispatch service

• Introduces validated 'NotificationAssessment', policy/model protocols, 'LLMPromptNotificationPolicy', and 'NotificationDecisionService' dispatch by kind with duplicate/empty registry safeguards. Keeps model adapters unaware of kind routing and prompt selection.

weather_briefing/notification_decision/core.py

policies.pyPackage per-kind notification prompts and constants +24/-0

Package per-kind notification prompts and constants

• Defines stable kind constants and loads per-kind prompt resources via 'importlib.resources' with actionable failures. Moves prompt ownership out of generic data prompts and into 'notification_decision'.

weather_briefing/notification_decision/policies.py

service.pyIntegrate NotificationDecisionProvider into BriefingService +36/-114

Integrate NotificationDecisionProvider into BriefingService

• Wires 'BriefingService' to build weather assessments and call 'NotificationDecisionProvider' for briefings, while forcing forecasts to always notify. Extracts result validation and settings surfaces into application modules and updates raw payload adjustments for filtered warning IDs.

weather_briefing/service.py

notification.pyAdd service-status notification assessment builder +43/-0

Add service-status notification assessment builder

• Adds 'service_status_notification_assessment()' to construct a type-specific decision payload (source + previous/current message fields) for the service-status policy.

weather_briefing/service_status/notification.py

Refactor (33) +1960 / -1414

briefing_settings.pyExtract typed briefing settings surface +68/-0

Extract typed briefing settings surface

• Introduces a dedicated settings façade used by briefing orchestration instead of a local protocol, centralizing the subset of settings required by 'BriefingService'.

weather_briefing/application/briefing_settings.py

briefing_validation.pyExtract briefing output validation into a reusable validator +62/-0

Extract briefing output validation into a reusable validator

• Moves output checks (required advice topics, renderer length constraints, kind-specific invariants) into an application-level validator builder. Keeps orchestration code focused on sequencing rather than validation details.

weather_briefing/application/briefing_validation.py

summarization.pyMake summarization content-only (no notification decision) +5/-6

Make summarization content-only (no notification decision)

• Updates the summarization pipeline to return a validated 'BriefingResult' only, aligning with the new two-step flow: generate candidate content, then assess notification worthiness separately.

weather_briefing/application/summarization.py

cli.pySlim CLI and delegate parsing/scheduling/diagnostics +27/-241

Slim CLI and delegate parsing/scheduling/diagnostics

• Removes the large inline argparse/scheduling/diagnostics helpers from 'cli.py' and delegates to 'command_parser', 'scheduling', and 'runtime_diagnostics'. Wires notification decision service composition alongside existing delivery/LLM/weather composition.

weather_briefing/cli.py

command_parser.pyExtract CLI parser construction +128/-0

Extract CLI parser construction

• Introduces a standalone command parser module to own subcommands and argument validation, reducing CLI coupling to runtime orchestration and scheduling rules.

weather_briefing/command_parser.py

delivery.pyIsolate delivery provider composition +104/-0

Isolate delivery provider composition

• Adds a focused module for building delivery providers/renderers after renderer code is split, keeping CLI orchestration thinner.

weather_briefing/composition/delivery.py

llm.pyIsolate LLM provider composition +44/-0

Isolate LLM provider composition

• Moves LLM provider wiring out of the CLI into its own composition module, aligning with the extracted transport and narrower structured calls.

weather_briefing/composition/llm.py

weather.pyIsolate weather provider composition +3/-135

Isolate weather provider composition

• Moves weather context provider wiring into a dedicated composition module, consistent with the broader refactor that extracts provider parsing into separate modules.

weather_briefing/composition/weather.py

__init__.pyUpdate delivery exports after renderer split +3/-1

Update delivery exports after renderer split

• Adjusts delivery package exports/import paths to reflect the new renderer modules and shared rendering utilities while keeping the delivery surface coherent.

weather_briefing/delivery/init.py

base.pyAlign delivery base types with refactored renderer layout +1/-1

Align delivery base types with refactored renderer layout

• Minor adjustments to delivery base definitions/imports to support the split renderer module structure.

weather_briefing/delivery/base.py

rendering.pyExtract shared rendering primitives +82/-0

Extract shared rendering primitives

• Adds shared utilities for attribution, formatting, and message shaping used by Telegram/plain/Bark renderers to reduce duplication and keep platform renderers focused.

weather_briefing/delivery/rendering.py

telegram_renderer.pyMove Telegram HTML renderer into its own module +103/-0

Move Telegram HTML renderer into its own module

• Extracts Telegram HTML rendering into a dedicated renderer module that reuses shared rendering primitives while preserving Telegram-specific escaping/formatting rules.

weather_briefing/delivery/telegram_renderer.py

any_llm.pyRefactor any-llm adapter to use transport and per-policy decision prompts +27/-125

Refactor any-llm adapter to use transport and per-policy decision prompts

• Delegates structured JSON-object request building/error normalization to 'any_llm_transport' and changes the decision API to 'decide_notification(system_prompt, payload)'. Removes the prior coupling to a shared notification policy prompt inside the model adapter.

weather_briefing/llm/any_llm.py

any_llm_transport.pyAdd structured completion transport and safe resource closing +139/-0

Add structured completion transport and safe resource closing

• Introduces a standalone transport module that injects the active Pydantic JSON schema into the final user message, normalizes any-llm errors, and safely closes SDK resources. Centralizes structured-IO behavior shared across structured calls.

weather_briefing/llm/any_llm_transport.py

fallback.pyUpdate fallback provider wiring for new transport/API +15/-7

Update fallback provider wiring for new transport/API

• Adjusts fallback logic to align with the refactored any-llm adapter/transport boundary, keeping retry/error behavior consistent.

weather_briefing/llm/fallback.py

lazy.pyUpdate lazy LLM wrappers for revised provider contracts +13/-5

Update lazy LLM wrappers for revised provider contracts

• Aligns lazy-loaded LLM wrappers with the new any-llm adapter surface and separated decision call path.

weather_briefing/llm/lazy.py

result.pyRemove notification decision from LLM result parsing +11/-15

Remove notification decision from LLM result parsing

• Changes parsing to return only 'BriefingResult' and removes extraction of notification decision from the content payload. Notification worthiness is now assessed via the decision service using per-kind prompts.

weather_briefing/llm/result.py

schema.pyRemove 'should_publish' from content structured output schema +0/-1

Remove 'should_publish' from content structured output schema

• Drops the decision field from the main structured output model so summarization outputs are content-only. Decisioning uses a separate narrow schema for 'should_notify'.

weather_briefing/llm/schema.py

__init__.pyUpdate persistence exports after splitting operations +4/-1

Update persistence exports after splitting operations

• Adjusts persistence package exports/import paths to reflect new domain-specific operation modules while keeping the transactional store concept intact.

weather_briefing/persistence/init.py

content.pyExtract article/briefing content persistence operations +193/-0

Extract article/briefing content persistence operations

• Moves article, pending-article, and briefing content persistence into a dedicated operations module to reduce the monolithic state store surface.

weather_briefing/persistence/content.py

context.pyExtract weather context persistence operations +105/-0

Extract weather context persistence operations

• Splits context document persistence/query operations into a separate module, keeping context history storage isolated from other domains.

weather_briefing/persistence/context.py

service_status.pyExtract service-status persistence store +167/-0

Extract service-status persistence store

• Moves service-status incident/message tracking and handled/decided revision recording into a dedicated 'SQLiteServiceStatusStore', preserving retry semantics and per-platform handling behavior.

weather_briefing/persistence/service_status.py

store.pyCompose SQLiteStateStore from domain operation modules +15/-496

Compose SQLiteStateStore from domain operation modules

• Refactors 'SQLiteStateStore' into a composition of content/context/warning/health operations over a single transactional SQLite connection and attaches a dedicated 'service_status' store. Removes large blocks of inline SQL methods while preserving transaction boundaries.

weather_briefing/persistence/store.py

warnings.pyExtract warning persistence operations +88/-0

Extract warning persistence operations

• Moves warning lifecycle persistence and queries into a dedicated module, isolating warning storage from content and context state concerns.

weather_briefing/persistence/warnings.py

runtime_diagnostics.pyCentralize runtime diagnostics and sensitive logging controls +89/-0

Centralize runtime diagnostics and sensitive logging controls

• Adds/expands a dedicated diagnostics module for logging configuration and temporary sensitive rendered-text diagnostics management, reducing CLI responsibilities.

weather_briefing/runtime_diagnostics.py

scheduling.pyExtract scheduling window and briefing delivery policy logic +66/-0

Extract scheduling window and briefing delivery policy logic

• Moves schedule-window checks and last-briefing-window silent delivery policy into a dedicated module used by CLI/runtime orchestration.

weather_briefing/scheduling.py

monitor.pyRefactor service-status monitor around extracted decision/persistence helpers +4/-35

Refactor service-status monitor around extracted decision/persistence helpers

• Updates service-status monitoring to use the new assessment builder and split persistence store access, preserving handled/decided revision boundaries so delivery failures can retry safely.

weather_briefing/service_status/monitor.py

state.pyUpdate state wiring for composed persistence store +3/-3

Update state wiring for composed persistence store

• Adjusts state façade usage to match the composed 'SQLiteStateStore' and its attached 'service_status' store, keeping external state-store usage consistent after internal splits.

weather_briefing/state.py

base.pyAlign provider base utilities for parsing extraction +10/-0

Align provider base utilities for parsing extraction

• Minor updates to weather provider base utilities/types to support extracted parsing modules and more consistent response contract validation.

weather_briefing/weather/base.py

open_meteo.pyDelegate Open-Meteo response parsing to a dedicated module +16/-206

Delegate Open-Meteo response parsing to a dedicated module

• Refactors the provider to use 'open_meteo_parsing' for daily value extraction, peak computations, and allergen parsing. Standardizes safe parsing errors and keeps IO logic separate from payload interpretation.

weather_briefing/weather/open_meteo.py

open_meteo_parsing.pyAdd Open-Meteo parsing helpers with explicit contract errors +200/-0

Add Open-Meteo parsing helpers with explicit contract errors

• Introduces standalone parsing helpers and a dedicated response-contract error type to validate/transform Open-Meteo payloads previously parsed inline in the provider.

weather_briefing/weather/open_meteo_parsing.py

qweather.pyDelegate QWeather response parsing to a dedicated module +21/-136

Delegate QWeather response parsing to a dedicated module

• Refactors the provider to use 'qweather_parsing', tightening payload validation and isolating parsing/localized formatting from network/IO concerns.

weather_briefing/weather/qweather.py

qweather_parsing.pyAdd QWeather parsing helpers and validation +144/-0

Add QWeather parsing helpers and validation

• Adds a standalone parsing module to validate QWeather payload shapes and convert them into domain snapshots, replacing inline parsing logic in the provider module.

weather_briefing/weather/qweather_parsing.py

Tests (10) +716 / -156

test_any_llm_provider.pyUpdate any-llm provider tests for new decision API +9/-14

Update any-llm provider tests for new decision API

• Aligns tests with the extracted 'any_llm_transport' and the updated decision method name/shape. Removes coupling to the prior shared notification policy prompt wiring.

tests/test_any_llm_provider.py

test_cli.pyAdjust CLI tests after parser and runtime extraction +34/-16

Adjust CLI tests after parser and runtime extraction

• Updates CLI tests for the extracted 'command_parser' module and slimmer 'cli.py' responsibilities. Keeps coverage of command behavior after scheduling/diagnostics logic moves out.

tests/test_cli.py

test_llm.pyAlign LLM tests with content-only structured schema +6/-7

Align LLM tests with content-only structured schema

• Updates LLM tests after removing 'should_publish' from the structured content schema and making result parsing return only 'BriefingResult'. Ensures validation expectations match the new contract.

tests/test_llm.py

test_llm_fallback.pyUpdate fallback tests for transport and schema changes +16/-6

Update fallback tests for transport and schema changes

• Adjusts fallback scenarios to match the refactored any-llm transport boundary and revised structured output/decision contracts. Keeps coverage for error normalization and retry behavior.

tests/test_llm_fallback.py

test_notification_decision.pyAdd comprehensive tests for typed notification decision policies +291/-0

Add comprehensive tests for typed notification decision policies

• Adds extensive coverage for 'NotificationAssessment' validation, policy registration/dispatch, and prompt-driven policy behavior. Includes error cases like duplicate kinds, unsupported kinds, and invalid payload shapes.

tests/test_notification_decision.py

test_prompts.pyValidate shipping and loading of notification decision prompts +47/-9

Validate shipping and loading of notification decision prompts

• Updates prompt tests to ensure per-kind notification decision prompt resources are present in wheel/sdist and loadable via 'importlib.resources'. Confirms separation from content-generation prompts.

tests/test_prompts.py

test_service.pyRewire briefing service tests for decoupled decisions +149/-58

Rewire briefing service tests for decoupled decisions

• Updates 'BriefingService' tests to use 'NotificationDecisionProvider' and to validate the new weather assessment payload shaping. Confirms forecasts bypass decisioning (always notify) and that validation/persistence behavior remains intact.

tests/test_service.py

test_service_status.pyUpdate service-status tests for new assessment and persistence split +33/-30

Update service-status tests for new assessment and persistence split

• Adjusts service-status notification tests to use 'service_status_notification_assessment' and the split persistence APIs. Verifies handled/decided revision semantics still enable retries without duplicate sends.

tests/test_service_status.py

test_summarization.pyUpdate summarization tests for separated decision flow +4/-8

Update summarization tests for separated decision flow

• Updates tests so summarization returns content only, with notification worthiness determined separately. Keeps validation coverage for generated payload structure.

tests/test_summarization.py

test_weather_context.pyUpdate weather-context tests for extracted parsing modules +127/-8

Update weather-context tests for extracted parsing modules

• Adjusts provider tests to match the new 'open_meteo_parsing'/'qweather_parsing' modules and stricter payload validation/localization rules. Ensures behavior remains stable after parsing extraction.

tests/test_weather_context.py

Documentation (1) +16 / -12

design.mdDocument the new notification-decision boundary and module split +16/-12

Document the new notification-decision boundary and module split

• Updates the design doc to introduce 'NotificationDecisionProvider'/'NotificationDecisionService', clarify that prompts/inputs are per message type, and describe the new package responsibilities (notification_decision, llm transport, split persistence ops, split weather parsing). Also clarifies forecast bypasses decisioning and how silent delivery interacts with weather decisions.

docs/design.md

Other (4) +27 / -10

prompts.pyRemove shared notification policy prompt from data prompts +2/-8

Remove shared notification policy prompt from data prompts

• Updates prompt exports to focus on content-generation prompts only, removing the previous shared notification policy resource now owned by 'notification_decision'.

weather_briefing/data/prompts.py

system_prompt.txtAdjust packaged system prompt for revised LLM contract +1/-2

Adjust packaged system prompt for revised LLM contract

• Updates the shipped system prompt content to reflect that content generation no longer includes notification decisions.

weather_briefing/data/system_prompt.txt

service_status.txtAdd dedicated service-status decision prompt +11/-0

Add dedicated service-status decision prompt

• Adds a JSON-only prompt to judge whether a service-status message is worth notifying, based on previous/current official message facts, without translation or generation.

weather_briefing/notification_decision/service_status.txt

weather.txtAdd dedicated weather decision prompt +13/-0

Add dedicated weather decision prompt

• Adds a JSON-only prompt to judge whether a generated weather change message warrants notification, emphasizing cumulative change since last successful publish and timeliness constraints.

weather_briefing/notification_decision/weather.txt

[qodo-code-review]

Code review by qodo was updated up to the latest commit 22a3ed0

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@weather_briefing/notification_decision/core.py`:
- Around line 66-71: Update NotificationPolicy.__post_init__ to validate that
system_prompt is a string before calling strip(), raising the same intended
ValueError for non-string or empty prompts. Preserve the existing non-empty
prompt validation for valid string values.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bef2c793-8ba7-4910-aacb-29947815fed7

📥 Commits

Reviewing files that changed from the base of the PR and between ebe0431 and 22a3ed0.

📒 Files selected for processing (59)

• docs/design.md
• tests/test_any_llm_provider.py
• tests/test_cli.py
• tests/test_llm.py
• tests/test_llm_fallback.py
• tests/test_notification_decision.py
• tests/test_prompts.py
• tests/test_service.py
• tests/test_service_status.py
• tests/test_summarization.py
• tests/test_weather_context.py
• weather_briefing/application/briefing_settings.py
• weather_briefing/application/briefing_validation.py
• weather_briefing/application/notification.py
• weather_briefing/application/summarization.py
• weather_briefing/cli.py
• weather_briefing/command_parser.py
• weather_briefing/composition/delivery.py
• weather_briefing/composition/llm.py
• weather_briefing/composition/notifications.py
• weather_briefing/composition/weather.py
• weather_briefing/data/notification_policy.txt
• weather_briefing/data/prompts.py
• weather_briefing/data/system_prompt.txt
• weather_briefing/delivery/__init__.py
• weather_briefing/delivery/bark_renderer.py
• weather_briefing/delivery/base.py
• weather_briefing/delivery/plain_renderer.py
• weather_briefing/delivery/renderers.py
• weather_briefing/delivery/rendering.py
• weather_briefing/delivery/telegram_renderer.py
• weather_briefing/llm/any_llm.py
• weather_briefing/llm/any_llm_transport.py
• weather_briefing/llm/fallback.py
• weather_briefing/llm/lazy.py
• weather_briefing/llm/result.py
• weather_briefing/llm/schema.py
• weather_briefing/notification_decision/__init__.py
• weather_briefing/notification_decision/core.py
• weather_briefing/notification_decision/policies.py
• weather_briefing/notification_decision/service_status.txt
• weather_briefing/notification_decision/weather.txt
• weather_briefing/notifications.py
• weather_briefing/persistence/__init__.py
• weather_briefing/persistence/content.py
• weather_briefing/persistence/context.py
• weather_briefing/persistence/service_status.py
• weather_briefing/persistence/store.py
• weather_briefing/persistence/warnings.py
• weather_briefing/runtime_diagnostics.py
• weather_briefing/scheduling.py
• weather_briefing/service.py
• weather_briefing/service_status/monitor.py
• weather_briefing/service_status/notification.py
• weather_briefing/state.py
• weather_briefing/weather/open_meteo.py
• weather_briefing/weather/open_meteo_parsing.py
• weather_briefing/weather/qweather.py
• weather_briefing/weather/qweather_parsing.py

💤 Files with no reviewable changes (4)

• weather_briefing/data/notification_policy.txt
• weather_briefing/notifications.py
• weather_briefing/delivery/renderers.py
• weather_briefing/llm/schema.py

[qodo-code-review]

Code review by qodo was updated up to the latest commit 2a68af6

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4b2d299

[qodo-code-review]

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

[IceCodeNew]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 33 minutes.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

weather_briefing/notification_decision/core.py (1)

121-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate policy kinds at the registry boundary.

NotificationDecisionService trusts policy.kind; another NotificationPolicy implementation can register a non-string or unnormalized value, producing invalid configuration that later fails dispatch. Apply the same non-empty, trimmed-string validation before duplicate detection.

As per coding guidelines, validate configuration at its input boundary and reject invalid types and unknown application-owned choices instead of coercing them.

Proposed fix

         registered: dict[str, NotificationPolicy] = {}
         for policy in policies:
-            if policy.kind in registered:
-                raise ValueError(f"Duplicate notification policy: {policy.kind}")
-            registered[policy.kind] = policy
+            kind = policy.kind
+            if not isinstance(kind, str) or not kind or kind != kind.strip():
+                raise ValueError("Notification policy kind must be a non-empty normalized string")
+            if kind in registered:
+                raise ValueError(f"Duplicate notification policy: {kind}")
+            registered[kind] = policy

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@weather_briefing/notification_decision/core.py` around lines 121 - 124,
Update the policy registration loop in NotificationDecisionService to validate
policy.kind before duplicate detection: require a non-empty string after
trimming and reject unknown application-owned kinds without coercing the value.
Use the normalized kind consistently for duplicate checks and registry storage,
while preserving the existing duplicate-policy error behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@weather_briefing/persistence/content.py`:
- Around line 57-59: Update the shared require_aware_datetime helper to reject
ambiguous DST-overlap timestamps, including aware values whose fold/offset
choices are not uniquely resolvable, while preserving valid aware timestamps.
This validation must cover the persistence boundaries at
weather_briefing/persistence/content.py:57-59, 84-86, 116-122, 136-138, and
146-149; weather_briefing/persistence/context.py:19-21 and 49-51;
weather_briefing/persistence/health.py:93-99 and 176-182;
weather_briefing/persistence/store.py:61-66 and 85-93; and
weather_briefing/persistence/warnings.py:21-23 and 44-52, which require no
direct changes if they already call the helper. Add coverage for parser-produced
timestamps and IANA-timezone paths feeding these boundaries.

In `@weather_briefing/persistence/service_status.py`:
- Around line 29-33: Update _stored_surfaces so persisted bytes or bytearray
values are passed directly to json.loads without str coercion, preserving valid
JSON decoding; if those types are not supported, explicitly validate and reject
non-text values before parsing. Keep the existing optional None handling and
downstream surface validation unchanged.

---

Outside diff comments:
In `@weather_briefing/notification_decision/core.py`:
- Around line 121-124: Update the policy registration loop in
NotificationDecisionService to validate policy.kind before duplicate detection:
require a non-empty string after trimming and reject unknown application-owned
kinds without coercing the value. Use the normalized kind consistently for
duplicate checks and registry storage, while preserving the existing
duplicate-policy error behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9dd8f839-1c98-4b30-84f2-a5f4bcf64956

📥 Commits

Reviewing files that changed from the base of the PR and between 78e32e5 and e35ef75.

📒 Files selected for processing (28)

• docs/design.md
• tests/test_cli.py
• tests/test_notification_decision.py
• tests/test_prompts.py
• tests/test_service.py
• tests/test_service_status.py
• tests/test_state.py
• tests/test_weather_context.py
• weather_briefing/application/notification.py
• weather_briefing/data/localization.json
• weather_briefing/data/system_prompt.txt
• weather_briefing/localization.py
• weather_briefing/models.py
• weather_briefing/notification_decision/core.py
• weather_briefing/persistence/content.py
• weather_briefing/persistence/context.py
• weather_briefing/persistence/health.py
• weather_briefing/persistence/schema.py
• weather_briefing/persistence/service_status.py
• weather_briefing/persistence/store.py
• weather_briefing/persistence/warnings.py
• weather_briefing/runtime_diagnostics.py
• weather_briefing/service.py
• weather_briefing/service_status/models.py
• weather_briefing/service_status/monitor.py
• weather_briefing/service_status/notification.py
• weather_briefing/weather/open_meteo_parsing.py
• weather_briefing/weather/qweather_parsing.py

🚧 Files skipped from review as they are similar to previous changes (3)

• weather_briefing/data/system_prompt.txt
• docs/design.md
• tests/test_prompts.py

[IceCodeNew]

Addressed the outside-diff CodeRabbit body finding in 9a3042f: policy registry construction now validates every declared kind before duplicate detection and storage. The analogous persistence audit continued in b7d1cdf, 5d30583, and e194dd3, which enforce JSON-text boundaries, warning field shape, and row/payload identity consistency. The two inline threads have been replied to with either the fixing commits or the DST-fold rationale and marked resolved.

[IceCodeNew]

The CodeRabbit CLI fallback for the rate-limited e194dd3 page review found one valid boundary issue. Fixed in afabe77: NotificationDecision now rejects non-boolean should_notify values, and NotificationDecisionService rejects invalid custom-policy result objects. Focused and full verification passed, followed by clean exact-SHA GPT-5.6 sol max and Zhipu GLM-5.2 max local reviews before this push.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

weather_briefing/persistence/service_status.py (1)

158-185: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Validate handled_at before persisting it.

handled_at reaches storage_time() on Line 185 without require_aware_datetime, so a naive timestamp can enter service-status state. Validate it at this boundary, consistent with the other persistence operations.

Proposed fix

     ) -> None:
         """Mark one observed message as delivered or intentionally skipped."""
+        handled_at = require_aware_datetime(
+            handled_at,
+            context="Service-status handling time",
+        )
         cursor = self._connection.execute(

Based on learnings, persistence must reject naive timestamps while retaining explicitly resolved DST-fold instants.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@weather_briefing/persistence/service_status.py` around lines 158 - 185,
Update mark_service_status_message_handled to validate handled_at with
require_aware_datetime before passing it to storage_time. Preserve explicitly
resolved DST-fold instants while rejecting naive timestamps, consistent with the
other persistence operations.

Sources: Coding guidelines, Learnings

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@weather_briefing/persistence/service_status.py`:
- Around line 158-185: Update mark_service_status_message_handled to validate
handled_at with require_aware_datetime before passing it to storage_time.
Preserve explicitly resolved DST-fold instants while rejecting naive timestamps,
consistent with the other persistence operations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4364164c-2b3a-4f75-a3f3-9ed97b8f26f8

📥 Commits

Reviewing files that changed from the base of the PR and between e35ef75 and afabe77.

📒 Files selected for processing (7)

• tests/test_notification_decision.py
• tests/test_service_status.py
• tests/test_state.py
• weather_briefing/notification_decision/core.py
• weather_briefing/persistence/content.py
• weather_briefing/persistence/service_status.py
• weather_briefing/persistence/warnings.py

[IceCodeNew]

Not changing the latest outside-diff CodeRabbit suggestion about handled_at. mark_service_status_message_handled already passes handled_at to the shared persistence serialization.storage_time helper, and that helper calls require_aware_datetime before converting to UTC. A naive value therefore raises ValueError before the UPDATE executes; adding the same validation immediately before storage_time would duplicate the existing boundary without changing behavior. Explicitly resolved DST-fold instants remain valid, as intended.