fix: validate external response contracts

[IceCodeNew]

Summary

• convert missing or invalid required LLM text fields into LLMError so repair retries can recover
• validate Open-Meteo daily forecast field presence, container shape, and per-day values with precise contract errors
• document the synchronous local-persistence architecture choice, its operating assumptions, and reevaluation triggers
• require future architecture notes to explain key choices and record accepted unresolved design concerns
• add unit, service retry, and provider boundary coverage

Validation

• mise exec -- uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml (529 passed)
• weather_briefing line coverage: 100%
• weather_briefing branch coverage: 100%
• prek run

Scope

The synchronous SQLite observation is documented rather than refactored: under the current low-frequency, sequential workload, a threading or async database boundary would add connection-lifecycle and serialization complexity without a demonstrated latency problem.

Summary by CodeRabbit

• Bug Fixes

  • Improved validation of generated briefing results, including headlines and active warning details.
  • Improved handling of malformed or incomplete weather forecast data with clearer parsing errors.
  • Ensured retry behavior covers both failed requests and invalid generated results.

• Documentation

  • Added architecture notes documenting synchronous local persistence decisions, assumptions, and reevaluation triggers.

• Tests

  • Expanded coverage for invalid briefing and weather forecast responses.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds explicit validation for LLM and Open-Meteo payload shapes, expands retry and parsing tests, and documents the boundaries and reevaluation criteria for synchronous local persistence in asynchronous orchestration.

Changes

Response Validation

Layer / File(s)	Summary
LLM result validation weather_briefing/llm.py, tests/test_llm.py, tests/test_service.py	LLM parsing now requires non-empty string warning fields and headlines, with tests covering validation errors and retry behavior for missing headlines.
Open-Meteo shape validation weather_briefing/weather_context.py, tests/test_weather_context.py	Open-Meteo daily fields and arrays receive explicit shape and bounds checks, with parsing failures mapped to WeatherContextError and covered by parametrized tests.

Architecture Documentation

Layer / File(s)	Summary
Persistence architecture note AGENTS.md, docs/notes.md	Documentation now records the rationale, assumptions, reevaluation triggers, and migration constraints for synchronous SQLite and filesystem operations in asynchronous orchestration.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• IceCodeNew/weather-briefing#23: Overlaps with the Open-Meteo fetch and response parsing flow.
• IceCodeNew/weather-briefing#24: Also strengthens Open-Meteo response shape and type handling.
• IceCodeNew/weather-briefing#36: Overlaps with expanded LLM retry coverage using FailingOnceLLM.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 accurately summarizes the main change: stricter validation of external response contracts.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-response-contract-validation

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (f047228) to head (dd3a13c).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #37   +/-   ##
=======================================
  Coverage   99.61%   99.62%           
=======================================
  Files          39       39           
  Lines        5787     5851   +64     
  Branches      330      337    +7     
=======================================
+ Hits         5765     5829   +64     
  Misses         15       15           
  Partials        7        7           

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[IceCodeNew]

Copilot review was requested twice during the draft phase, but GitHub reported that the requester had reached the Copilot review quota. No Copilot review threads or code findings were produced. Local verification, full diff review, and CI are clean; proceeding to the ready-state reviewer loop with this limitation recorded.

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_weather_context.py (1)

29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse _is_string_keyed_dict from weather_context.

Since _is_string_keyed_dict is already defined in weather_briefing.weather_context, consider importing it directly to avoid duplicating the logic.

♻️ Proposed refactor

-def _is_string_keyed_dict(value: object) -> TypeGuard[dict[str, object]]:
-    return isinstance(value, dict) and all(isinstance(key, str) for key in value)

(You can update the existing imports from weather_briefing.weather_context to include _is_string_keyed_dict, and also remove the TypeGuard import on line 2 if it is no longer used.)

🤖 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 `@tests/test_weather_context.py` around lines 29 - 32, Remove the duplicate
_is_string_keyed_dict definition from the test module and import
_is_string_keyed_dict from weather_briefing.weather_context alongside the
existing imports. Remove the TypeGuard import if it is no longer used.

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

Nitpick comments:
In `@tests/test_weather_context.py`:
- Around line 29-32: Remove the duplicate _is_string_keyed_dict definition from
the test module and import _is_string_keyed_dict from
weather_briefing.weather_context alongside the existing imports. Remove the
TypeGuard import if it is no longer used.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5f53d1fa-8411-4fe9-8b51-b2779bf2c323

📥 Commits

Reviewing files that changed from the base of the PR and between f047228 and dd3a13c.

📒 Files selected for processing (7)

• AGENTS.md
• docs/notes.md
• tests/test_llm.py
• tests/test_service.py
• tests/test_weather_context.py
• weather_briefing/llm.py
• weather_briefing/weather_context.py

[IceCodeNew]

Review-autofix complete

No unresolved, current CodeRabbit review threads were found. One trivial review-summary suggestion was independently reviewed and intentionally not applied: importing a production-private TypeGuard into the test module would couple test fixture typing to an implementation detail, while the local one-line helper is test-specific. The repository does not enforce the unrelated docstring-coverage advisory.

Copilot produced no findings because the requester review quota was exhausted, as recorded above. All CI checks, local pre-commit checks, 529 tests, and source line/branch coverage are clean.