Redesign forecast and briefing runs - #22
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe CLI and service migrate from daily/hourly modes to forecast/briefing modes. Forecast dates now flow through weather providers and LLM payloads, briefing delivery uses persisted state and silent notifications, and tests and documentation cover the updated scheduling and content rules. ChangesForecast and briefing execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant CLI
participant BriefingService
participant WeatherProviders
participant SQLiteStateStore
participant Publisher
Scheduler->>CLI: start forecast or briefing run
CLI->>SQLiteStateStore: evaluate briefing delivery policy
CLI->>BriefingService: run with forecast date and delivery flags
BriefingService->>WeatherProviders: fetch dated or current context
BriefingService->>SQLiteStateStore: read and update briefing state
BriefingService->>Publisher: publish rendered message
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 99.37% 99.43% +0.05%
==========================================
Files 36 36
Lines 4636 4928 +292
Branches 265 282 +17
==========================================
+ Hits 4607 4900 +293
+ Misses 18 17 -1
Partials 11 11 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
This PR redesigns the execution model from daily/hourly to forecast/briefing, adding a forecast target-date path (--date) that does not alter the run clock/state timestamps, plus one-shot --run-now behavior and a “silent final-window flush” policy for deferred briefing delivery (Telegram disable_notification).
Changes:
- Plumb
forecast_datethrough weather providers, fallback logic, and the LLM payload to support future-date forecasts without time-traveling state. - Update CLI modes/scheduling to
run forecast|briefing, add--run-now, and implement final-window forced delivery + silent Telegram delivery when appropriate. - Upgrade persisted “recent briefings” from plain bodies to structured records (kind/body/published_at) for better LLM context.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| weather_briefing/weather_context.py | Adds forecast_date support to providers and introduces a shared fetch helper for dated vs non-dated calls. |
| weather_briefing/state.py | Returns structured BriefingRecord objects from recent_briefings. |
| weather_briefing/service.py | Adds forecast_date, forced publish, and silent publish controls; expands LLM payload with forecast date + structured recent briefings. |
| weather_briefing/publishers.py | Extends publisher interface with silent and maps it to Telegram disable_notification. |
| weather_briefing/prompts.py | Updates system prompt contract for forecast/briefing modes and forecast-date constraints. |
| weather_briefing/models.py | Introduces BriefingRecord dataclass. |
| weather_briefing/cli.py | Renames run modes, adds --date/--run-now, and implements final-window delivery policy. |
| README.md | Updates usage docs to new modes and flags. |
| docs/requirements.md | Updates requirements to new CLI/mode semantics and final-window behavior. |
| docs/design.md | Documents the redesigned scheduling, one-shot runs, and forecast-date plumbing. |
| tests/test_weather_context.py | Adds coverage for date-targeted provider selection and fallback behavior. |
| tests/test_state.py | Updates expectations for structured recent briefings. |
| tests/test_service.py | Adds/updates coverage for forecast-date isolation, forced briefing publish, and silent policy behavior. |
| tests/test_publishers.py | Adds coverage for Telegram silent delivery flag. |
| tests/test_cli.py | Adds/updates coverage for parser changes, date parsing, and delivery policy logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bdff5c9 to
6a7a8dd
Compare
6a7a8dd to
ac2393a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/service.py (1)
242-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate silent delivery to verbatim messages.
Line 242 silences only the summary. Lines 244-252 publish deferred verbatim articles without
silent, so final-window flushes still trigger notifications.Proposed fix
- await self._delivery.publish_verbatim(article) + await self._delivery.publish_verbatim(article, silent=publish_silently)- async def publish_verbatim(self, article: Article) -> None: + async def publish_verbatim(self, article: Article, *, silent: bool = False) -> None: message = self.renderer.render_verbatim(article) ... - await self.publisher.publish(message) + await self.publisher.publish(message, silent=silent)Add a forced, suppressed briefing test containing a verbatim article.
🤖 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/service.py` around lines 242 - 252, Propagate the computed publish_silently value from the summary delivery in the verbatim-article loop within the surrounding service method by passing it to _delivery.publish_verbatim for each article. Add a test covering a forced, suppressed briefing containing a verbatim article and verify the verbatim delivery is silent.
🤖 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 `@docs/requirements.md`:
- Line 15: Clarify requirement 8 so the Telegram Bot API silent-message
parameter applies only to the unconditional final configured sending-hour
fallback when no briefing has succeeded that day. Explicitly preserve audible
delivery for manual --run-now executions and normally publishable briefings,
consistent with the existing design and README behavior.
In `@README.md`:
- Line 72: Update the --date documentation to state that it accepts today’s
local date or a future date, replacing the future-only wording in README.md
(72-72), docs/design.md (25-25), and docs/requirements.md (14-14). Keep the
existing descriptions of runtime, status, history-window, and --at behavior
unchanged.
---
Outside diff comments:
In `@weather_briefing/service.py`:
- Around line 242-252: Propagate the computed publish_silently value from the
summary delivery in the verbatim-article loop within the surrounding service
method by passing it to _delivery.publish_verbatim for each article. Add a test
covering a forced, suppressed briefing containing a verbatim article and verify
the verbatim delivery is silent.
🪄 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
Run ID: 7d3ea0c4-de2f-4a23-a3db-87299ad364ba
📒 Files selected for processing (15)
README.mddocs/design.mddocs/requirements.mdtests/test_cli.pytests/test_publishers.pytests/test_service.pytests/test_state.pytests/test_weather_context.pyweather_briefing/cli.pyweather_briefing/models.pyweather_briefing/prompts.pyweather_briefing/publishers.pyweather_briefing/service.pyweather_briefing/state.pyweather_briefing/weather_context.py
ac2393a to
c90b296
Compare
Review fixes appliedAddressed all three CodeRabbit findings in |
c90b296 to
d7e1edb
Compare
Problem
A long-running daemon may already own the scheduler and state database, while an operator still needs to start a separate process for one immediate one-shot task. Starting daemon --run-now attempts to create another daemon and conflicts with the existing process, so immediate execution belongs on run forecast and run briefing instead.
The one-shot briefing must include information that earlier briefing runs deferred as not worth interrupting the user. Because those items may sit for hours, the LLM must discard time-sensitive facts that newer weather snapshots have replaced. If no briefing has been sent by the final configured window of the day, the service must flush the otherwise-suppressed result through the Telegram API as a silent message.
Forecast execution has a different time concern: an operator may request a future date such as the day after tomorrow. That target date must affect provider queries and the LLM summary without changing the real run time, SQLite state timestamps, or history windows. The existing --at behavior remains reserved for historical test replay.
Changes
Why this is one pull request
These changes are one concern: the execution and delivery contract for forecast and briefing runs. The command names, daemon ownership, one-shot semantics, deferred-state handling, freshness rules, final-window policy, and forecast target date all meet at the same CLI-to-service-to-provider boundary.
Splitting them would leave invalid intermediate behavior. Renaming commands without moving --run-now would preserve the second-daemon conflict; forcing deferred delivery without freshness rules could publish stale weather; adding --date without separating it from the run clock could corrupt state and history semantics; and the final-window rule depends on the same briefing force-publish path. This PR changes that contract atomically and does not include unrelated coverage work or other features.
Summary by CodeRabbit
New Features
forecastandbriefingrun modes.--run-nowand date-specific forecasts with--date.Documentation
Bug Fixes