feat: add Japan JMA forecast capability - #75
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 40 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 (17)
📝 WalkthroughWalkthroughThis change adds JMA Japan forecast support, including office-code validation, location and environment configuration, regional forecast parsing, provider ordering rules, CLI construction, fallback handling, caching behavior, documentation, and test coverage. ChangesJapan weather provider
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant _weather_context_provider
participant _build_jma
participant JMAJapanForecastProvider
participant JMAForecastAPI
participant WeatherContextSnapshot
_weather_context_provider->>_build_jma: pass jma_office_code
_build_jma->>JMAJapanForecastProvider: construct with jma_base_url and office code
JMAJapanForecastProvider->>JMAForecastAPI: request /{office_code}.json
JMAForecastAPI-->>JMAJapanForecastProvider: forecast JSON
JMAJapanForecastProvider->>WeatherContextSnapshot: return normalized forecast snapshot
🚥 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 #75 +/- ##
========================================
Coverage 99.77% 99.78%
========================================
Files 45 45
Lines 8123 8394 +271
Branches 456 488 +32
========================================
+ Hits 8105 8376 +271
Misses 13 13
Partials 5 5 ☔ View full report in Codecov by Harness. |
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 09:59
bd1e647 to
1d2e440
Compare
IceCodeNew
force-pushed
the
codex/pr4-singapore
branch
from
July 20, 2026 09:59
81f0061 to
58653bf
Compare
Owner
Author
|
/agentic_review |
Code Review by Qodo
Context used✅ Compliance rules (platform):
37 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
IceCodeNew
force-pushed
the
codex/pr4-singapore
branch
from
July 20, 2026 13:17
58653bf to
12937fc
Compare
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 13:17
1d2e440 to
6e53a71
Compare
Owner
Author
|
/agentic_review |
IceCodeNew
force-pushed
the
codex/pr4-singapore
branch
from
July 20, 2026 13:25
12937fc to
0966f7a
Compare
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 13:25
6e53a71 to
1ad3af0
Compare
|
Code review by qodo was updated up to the latest commit 6e53a71 |
Owner
Author
|
/agentic_review |
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 13:29
1ad3af0 to
165a42b
Compare
Owner
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 165a42b |
IceCodeNew
force-pushed
the
codex/pr4-singapore
branch
4 times, most recently
from
July 20, 2026 15:06
667144d to
32194ec
Compare
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 15:38
165a42b to
711823b
Compare
Owner
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 711823b |
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 15:48
711823b to
02c0d2d
Compare
Owner
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 02c0d2d |
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 16:02
02c0d2d to
fa77c12
Compare
Owner
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit fa77c12 |
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 16:18
fa77c12 to
dbb155c
Compare
IceCodeNew
marked this pull request as ready for review
July 20, 2026 16:20
PR Summary by QodoAdd JMA Japan forecast provider with per-location office codes
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
|
Code review by qodo was updated up to the latest commit dbb155c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
weather_briefing/cli.py (1)
488-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProvider-specific availability check hardcoded into core orchestration.
jma_availableand itsname != WeatherProviderName.JMA_JAPAN or jma_availablecheck add a second provider-name-keyed conditional to_weather_context_provider(alongside the pre-existing QWeather-JWT check later in the same function). As more local providers gain per-location availability rules, this function will keep growingif name == ...branches instead of composing per-provider checks.♻️ Suggested extraction of a small availability registry
+_LOCAL_PROVIDER_AVAILABILITY: dict[str, Callable[[ResolvedLocation], bool]] = { + WeatherProviderName.JMA_JAPAN: lambda location: ( + location.jma_office_code is not None and location.country_code in {None, "JP"} + ), +} + + +def _local_provider_available(name: str, location: ResolvedLocation) -> bool: + check = _LOCAL_PROVIDER_AVAILABILITY.get(name) + return check is None or check(location) + + def _weather_context_provider( settings: Settings, client: httpx.AsyncClient, location: ResolvedLocation, ) -> CapabilityProviderSet: names = weather_providers_for(location, settings.weather_providers) - jma_available = location.jma_office_code is not None and location.country_code in {None, "JP"} - if settings.weather_providers is not None and WeatherProviderName.JMA_JAPAN in names and not jma_available: - reason = "missing-jma-office-code" if location.jma_office_code is None else "known-non-japan-country" - _LOGGER.warning("Skipping explicit JMA provider reason=%s", reason) + unavailable_local = [ + name for name in names if name in LOCAL_WEATHER_CAPABILITY_PROVIDERS and not _local_provider_available(name, location) + ] + if settings.weather_providers is not None and unavailable_local: + _LOGGER.warning("Skipping unavailable local provider names=%s", ",".join(unavailable_local)) main_names = [name for name in names if name not in LOCAL_WEATHER_CAPABILITY_PROVIDERS] supplement_names = [ name for name in names - if name in LOCAL_WEATHER_CAPABILITY_PROVIDERS and (name != WeatherProviderName.JMA_JAPAN or jma_available) + if name in LOCAL_WEATHER_CAPABILITY_PROVIDERS and _local_provider_available(name, location) ]Note: this changes the warning message format (loses the specific
missing-jma-office-code/known-non-japan-countryreason), so the existing tests (test_unavailable_jma_skips_explicit_supplement) would need updating if adopted — treat as illustrative, not a drop-in fix.As per coding guidelines, "keep core data platform-neutral, place vendor or delivery syntax in adapters ... and prefer composition and thin subclasses over copied request logic or growing conditionals."
🤖 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/cli.py` around lines 488 - 497, Refactor `_weather_context_provider` to remove the JMA-specific `jma_available` calculation and provider-name conditional from core orchestration. Introduce or reuse a composable provider availability registry/check associated with each local provider, and use it when building `supplement_names` while preserving unavailable-provider filtering and warning behavior; update affected tests, including `test_unavailable_jma_skips_explicit_supplement`, for the revised abstraction.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 `@docs/notes.md`:
- Around line 7-9: Clarify the JMA statement so it applies only when JMA is
configured as a supplement alongside a global primary provider, while preserving
the standalone configuration behavior where jma-jp may be the sole or primary
weather provider. Update the JMA rationale in the notes without changing the
provider orchestration rules or configuration semantics.
---
Nitpick comments:
In `@weather_briefing/cli.py`:
- Around line 488-497: Refactor `_weather_context_provider` to remove the
JMA-specific `jma_available` calculation and provider-name conditional from core
orchestration. Introduce or reuse a composable provider availability
registry/check associated with each local provider, and use it when building
`supplement_names` while preserving unavailable-provider filtering and warning
behavior; update affected tests, including
`test_unavailable_jma_skips_explicit_supplement`, for the revised abstraction.
🪄 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: 5f61b173-dd72-4b42-b6bb-166c64a94add
📒 Files selected for processing (15)
README.mddocs/design.mddocs/notes.mdenv.exampletests/test_cli.pytests/test_config.pytests/test_geocoding.pytests/test_regional_weather.pyweather_briefing/cli.pyweather_briefing/config.pyweather_briefing/data/provider_defaults.jsonweather_briefing/geocoding.pyweather_briefing/models.pyweather_briefing/regional_weather.pyweather_briefing/registries.py
IceCodeNew
marked this pull request as draft
July 20, 2026 16:43
IceCodeNew
force-pushed
the
codex/pr5-japan
branch
from
July 20, 2026 16:44
dbb155c to
1338493
Compare
IceCodeNew
marked this pull request as ready for review
July 20, 2026 16:46
| @@ -107,13 +107,11 @@ async def fetch_all( | |||
| ) -> tuple[WeatherContextSnapshot, ...]: | |||
| """Fetch primary context and skip expected supplement failures.""" | |||
| snapshots = [await self.fetch(latitude, longitude, forecast_date=forecast_date)] | |||
There was a problem hiding this comment.
2. Noisy dated supplement warnings 🐞 Bug ◔ Observability
When CapabilityProviderSet.fetch_all() is called with a forecast_date, it now invokes supplements with that date; supplements that don’t support dated fetches raise WeatherContextError and are skipped, but LoggedWeatherContextProvider logs them as WARNING failures. This produces misleading warning noise during otherwise successful dated forecasts (e.g., Singapore with nea-sg configured).
Agent Prompt
### Issue description
Dated forecasts now attempt to call supplement providers with `forecast_date`. For non-dated supplements, `fetch_weather_context()` raises `WeatherContextError` (unsupported target date). Even though `CapabilityProviderSet.fetch_all()` catches and ignores this error, `LoggedWeatherContextProvider` logs it at WARNING first, creating misleading “failed” warnings for an expected/benign skip.
### Issue Context
This is introduced by changing `CapabilityProviderSet.fetch_all()` to pass `forecast_date` through to supplements (needed for dated-capable supplements like JMA). The fix should preserve dated-capable supplement execution while avoiding WARNING logs for the expected “does not support target forecast dates” path.
### Fix Focus Areas
- weather_briefing/capabilities.py[101-117]
- weather_briefing/weather_context.py[162-215]
- weather_briefing/weather_context.py[778-792]
### Suggested implementation direction
- In `LoggedWeatherContextProvider.fetch(...)`, before logging “call started”, detect `forecast_date is not None` AND underlying provider is not a `DatedWeatherContextProvider`.
- Either: raise `WeatherContextError` without emitting a WARNING (optionally log at DEBUG/INFO as a skip), or
- Downgrade logging level specifically for this expected unsupported-date error.
This keeps real provider failures visible while removing expected warning noise on dated forecasts with non-dated supplements (e.g., NEA).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 1338493 |
This was referenced Jul 21, 2026
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Add Japan Meteorological Agency forecast support with validated office codes, response normalization, Japanese source-language metadata, and provider registry/configuration wiring.
Open-Meteo remains the primary weather provider; JMA supplies local forecast context for Japan.
Validation
Stacked on
codex/pr4-singapore.Summary by CodeRabbit
New Features
Documentation
Bug Fixes