feat: add Singapore NEA nowcast capability

[IceCodeNew]

What changed

Add Singapore NEA's two-hour forecast as a supplementary regional capability provider. The adapter supports optional API-key authentication, normalizes timestamps, validates response shapes, and preserves the fixed English source language.

Open-Meteo remains the primary weather provider; NEA supplements it with local nowcast detail.

Validation

• Full test suite and coverage passed
• Ruff, formatting, ty, secret, and workflow checks passed

Stacked on codex/pr3-location-language.

[coderabbitai]

Warning

Review limit reached

@IceCodeNew, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 25355111-68e7-44f8-a5db-883887c95f02

📥 Commits

Reviewing files that changed from the base of the PR and between 0f2daa4 and 32194ec.

📒 Files selected for processing (16)

• README.md
• docs/design.md
• docs/notes.md
• env.example
• tests/test_capabilities.py
• tests/test_cli.py
• tests/test_config.py
• tests/test_regional_weather.py
• tests/test_service.py
• weather_briefing/capabilities.py
• weather_briefing/cli.py
• weather_briefing/config.py
• weather_briefing/data/provider_defaults.json
• weather_briefing/regional_weather.py
• weather_briefing/registries.py
• weather_briefing/service.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/pr4-singapore

---

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.77%. Comparing base (0f2daa4) to head (32194ec).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master      #74    +/-   ##
========================================
  Coverage   99.77%   99.77%            
========================================
  Files          43       45     +2     
  Lines        7918     8123   +205     
  Branches      441      456    +15     
========================================
+ Hits         7900     8105   +205     
  Misses         13       13            
  Partials        5        5            

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

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 37 rules

1. NEA used outside Singapore 🐞 Bug ≡ Correctness ⭐ New

Description

When WEATHER_PROVIDERS explicitly includes nea-sg, weather_providers_for() returns it for
every location and _weather_context_provider() builds it as a supplement (or even as the primary
when it's the only provider) without checking location.country_code. Since
NEASingaporeNowcastProvider.fetch() ignores the requested latitude/longitude, non-SG briefings can
receive Singapore nowcast context, producing misleading summaries.

Code

weather_briefing/cli.py[R476-487]

    names = weather_providers_for(location, settings.weather_providers)
+    local_capability_names = {WeatherProviderName.NEA_SINGAPORE}
+    main_names = [name for name in names if name not in local_capability_names]
+    supplement_names = [name for name in names if name in local_capability_names]
+    if not main_names:
+        main_names, supplement_names = supplement_names, []
    providers: list[WeatherContextProvider] = []
    active_names: list[str] = []
-    for name in names:
+    for name in main_names:
        if name == WeatherProviderName.QWEATHER and not _qweather_is_configured(settings):
            if settings.weather_providers is not None:
                raise ValueError("Explicit QWeather provider is missing JWT configuration")

Relevance

⭐⭐⭐ High

PR #74 design notes scope NEA to SG nowcast supplement; gating by country_code matches stated
contract.

PR-#74

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The configuration path returns explicit provider tuples for all locations; the CLI then always
constructs nea-sg supplements based only on provider name, and the NEA adapter itself does not use
the requested coordinates, so it cannot be location-correct when invoked for non-SG locations.

weather_briefing/config.py[224-236]
weather_briefing/cli.py[471-504]
weather_briefing/regional_weather.py[42-77]

Agent prompt

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

### Issue description
`nea-sg` is Singapore-only, but the current provider selection logic applies it to *any* location whenever it appears in explicit `WEATHER_PROVIDERS`. The NEA adapter also ignores the input coordinates, so calling it for non-SG locations returns Singapore nowcast and pollutes the context.

### Issue Context
- Explicit `WEATHER_PROVIDERS` applies globally across all configured locations.
- `_weather_context_provider()` partitions `nea-sg` into supplements purely by provider name, not by `location.country_code`.
- `NEASingaporeNowcastProvider.fetch()` does not use `latitude`/`longitude`, so it cannot self-correct without an explicit guard.

### Fix Focus Areas
- weather_briefing/cli.py[476-532]
- weather_briefing/config.py[224-236]
- weather_briefing/regional_weather.py[42-77]

### Implementation guidance
Pick one (or combine for defense-in-depth):
1) **Selection-time guard (recommended):** In `_weather_context_provider()` (or in `weather_providers_for()`), drop `nea-sg` unless `location.country_code == "SG"`. If that removal would leave no providers (e.g. explicit `WEATHER_PROVIDERS=("nea-sg",)` on a non-SG location), raise a clear `ValueError/ConfigurationError`.
2) **Provider-level guard:** In `NEASingaporeNowcastProvider.fetch()`, validate the coordinates are within a Singapore bounding box and raise `RegionalWeatherProviderError` when outside; this prevents accidental misuse even if selection logic regresses.

Add/adjust tests to cover multi-location behavior with explicit `WEATHER_PROVIDERS=open-meteo,nea-sg` to ensure non-SG locations do not call NEA.

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

---

2. Supplements not best-effort ✗ Dismissed 🐞 Bug ☼ Reliability

Description

CapabilityProviderSet.fetch_all awaits supplemental providers sequentially and only suppresses
WeatherContextError/ValueError, so a slow supplement can delay the whole run and an unexpected
exception type can still abort the briefing. This contradicts the documented intent that NEA
supplement failures should not block the primary weather context.

Code

weather_briefing/capabilities.py[R101-119]

+    async def fetch_all(
+        self,
+        latitude: float,
+        longitude: float,
+        *,
+        forecast_date: pendulum.Date | None = None,
+    ) -> tuple[WeatherContextSnapshot, ...]:
+        """Fetch the primary context and best-effort supplementary capabilities."""
+        snapshots = [await self.fetch(latitude, longitude, forecast_date=forecast_date)]
+        if forecast_date is not None:
+            return tuple(snapshots)
+        from .weather_context import WeatherContextError
+
+        for provider in self.supplements:
+            try:
+                snapshots.append(await _fetch_context(provider, latitude, longitude, None))
+            except (WeatherContextError, ValueError):
+                continue
+        return tuple(snapshots)

Relevance

⭐⭐ Medium

No clear historical precedent on running “best-effort supplements” concurrently/broader exception
suppression; best-effort semantics appear but details vary.

PR-#19
PR-#54

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The design doc says NEA supplement failures must not block the primary context, but fetch_all
currently awaits each supplement inline (serially) and only suppresses two exception classes.
Because the CLI uses a shared AsyncClient with a global timeout, a slow supplement can delay the
entire run by up to that timeout, and any exception outside the caught set will still propagate and
fail the run.

docs/design.md[51-58]
weather_briefing/capabilities.py[101-119]
weather_briefing/cli.py[255-280]

Agent prompt

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

## Issue description
`CapabilityProviderSet.fetch_all()` fetches supplements sequentially and only catches `WeatherContextError`/`ValueError`. This means optional supplements can (a) add up to the full configured HTTP timeout to every run when the supplement endpoint is slow, and (b) still crash the run if the supplement raises an unexpected exception type.

## Issue Context
Design explicitly states Singapore NEA is a supplement and should not block the main Open-Meteo context when it fails. The CLI’s shared `httpx` client is configured with a global timeout (`settings.http_timeout_seconds`), so any slow supplement call becomes user-visible latency.

## Fix Focus Areas
- Run supplements concurrently and never block the main snapshot longer than necessary.
- Treat supplements as "best-effort": suppress (and optionally log) all non-cancellation exceptions from supplements.
- Consider applying a shorter per-supplement timeout (separate from the main provider timeout), so an optional nowcast cannot consume the full global timeout budget.

### Code pointers
- weather_briefing/capabilities.py[101-119]

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

---

3. NEA errors too generic ✓ Resolved 🐞 Bug ◔ Observability

Description

NEASingaporeNowcastProvider wraps HTTP failures into RegionalWeatherProviderError using only the
exception type name (e.g. “HTTPStatusError”), dropping safe details like the HTTP status code and
reducing diagnosability.

Code

weather_briefing/regional_weather.py[R68-69]

+        except (httpx.HTTPError, KeyError, TypeError, ValueError) as exc:
+            raise RegionalWeatherProviderError(f"NEA nowcast failed: {type(exc).__name__}") from None

Relevance

⭐⭐⭐ High

Repo prioritizes actionable but safe error details (diagnostic stage/status) over generic type names
(PRs 19,35).

PR-#19
PR-#35

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The NEA provider currently formats failures with type(exc).__name__, while the repo already has a
sanitization helper that preserves HTTP status codes for HTTPStatusError.

weather_briefing/regional_weather.py[66-69]
weather_briefing/weather_context.py[715-718]

Agent prompt

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

## Issue description
When NEA requests fail with `httpx.HTTPStatusError`, the provider raises `RegionalWeatherProviderError("NEA nowcast failed: HTTPStatusError")`, which omits the HTTP status code (e.g. 401/403/429/500). This makes logs and alerts less actionable.

## Issue Context
Other providers already sanitize errors but keep the HTTP status code (via `_safe_provider_error`). NEA nowcast should follow the same pattern.

## Fix Focus Areas
- weather_briefing/regional_weather.py[66-69]
- weather_briefing/weather_context.py[715-718]

## Suggested fix
In `regional_weather.py`, format errors similarly to `_safe_provider_error`, e.g.:
- If `isinstance(exc, httpx.HTTPStatusError)`, emit `HTTP {exc.response.status_code}`.
- Otherwise emit `{type(exc).__name__}`.
Optionally, reuse a shared helper (import `_safe_provider_error` or create a small local helper) to keep behavior consistent.

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

---

4. Nowcast labeled as daily ✓ Resolved 🐞 Bug ≡ Correctness

Description

Supplementary NEA nowcast snapshots are converted via snapshot_to_documents(), which hardcodes the
weather section label as “今明天气预报”, so the two-hour nowcast is presented to the LLM as a day-level
forecast and can mislead summaries.

Code

weather_briefing/service.py[R333-349]

+            if isinstance(self._weather_context_provider, CapabilityProviderSet):
+                weather_contexts = await self._weather_context_provider.fetch_all(
+                    self._location.latitude,
+                    self._location.longitude,
+                    forecast_date=forecast_date,
+                )
+            else:
+                weather_contexts = (
+                    await fetch_weather_context(
+                        self._weather_context_provider,
+                        self._location.latitude,
+                        self._location.longitude,
+                        forecast_date,
+                    ),
+                )
+            for weather_context in weather_contexts:
+                context_items.extend(snapshot_to_documents(weather_context))

Relevance

⭐⭐⭐ High

Team has accepted fixes correcting misleading labeling/semantics in context sent to LLM (PR 59;
boundary clarity PR 76).

PR-#59
PR-#76

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The service now converts every snapshot returned by CapabilityProviderSet.fetch_all() into
documents, but the document formatter uses a fixed “今明天气预报” label while the NEA provider explicitly
returns a two-hour nowcast string.

weather_briefing/service.py[328-350]
weather_briefing/weather_context.py[777-807]
weather_briefing/regional_weather.py[70-77]

Agent prompt

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

## Issue description
NEA nowcast snapshots ("Next two hours") are rendered into `SourceDocument.content` using `snapshot_to_documents()`, which labels the weather section as `今明天气预报`. This makes the nowcast appear like a today/tomorrow forecast in LLM context.

## Issue Context
The PR introduces supplementary weather contexts and iterates over all snapshots to convert them into documents. The existing formatter assumes all `WeatherContextSnapshot.weather_forecast` items are day-level forecasts.

## Fix Focus Areas
- weather_briefing/weather_context.py[777-807]
- weather_briefing/regional_weather.py[70-77]
- weather_briefing/service.py[332-349]

## Suggested fix
Introduce a way to distinguish nowcast vs daily forecast when rendering, e.g.:
- Add an optional field to `WeatherContextSnapshot` (e.g. `forecast_kind: Literal["daily","nowcast"]` or `forecast_horizon_label`) and have `snapshot_to_documents()` choose `两小时短时预报` / `短时预报` vs `今明天气预报` accordingly; or
- Change `snapshot_to_documents()` to use a neutral label like `天气预报` / `天气信息` (less invasive but affects all providers).

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

---

5. NEA supplement decision undocumented ✓ Resolved 📘 Rule violation ⌂ Architecture

Description

The PR introduces a best-effort supplementary weather capability (nea-sg) and new multi-snapshot
fetch behavior, which is a non-obvious architectural decision. There is no corresponding
rationale/trade-offs/boundaries entry for this decision in docs/notes.md, which violates the
documentation requirement.

Code

weather_briefing/capabilities.py[R101-119]

+    async def fetch_all(
+        self,
+        latitude: float,
+        longitude: float,
+        *,
+        forecast_date: pendulum.Date | None = None,
+    ) -> tuple[WeatherContextSnapshot, ...]:
+        """Fetch the primary context and best-effort supplementary capabilities."""
+        snapshots = [await self.fetch(latitude, longitude, forecast_date=forecast_date)]
+        if forecast_date is not None:
+            return tuple(snapshots)
+        from .weather_context import WeatherContextError
+
+        for provider in self.supplements:
+            try:
+                snapshots.append(await _fetch_context(provider, latitude, longitude, None))
+            except (WeatherContextError, ValueError):
+                continue
+        return tuple(snapshots)

Relevance

⭐⭐⭐ High

Team frequently documents non-obvious boundaries in docs/notes.md (see added “Accepted operating
boundaries” in PR #54, #58).

PR-#54
PR-#58

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2141673 requires non-obvious architectural decisions to be recorded in
docs/notes.md with rationale/trade-offs/boundaries. This PR adds a new best-effort supplement
execution path (fetch_all() iterates self.supplements and skips failures), but docs/notes.md
(which defines itself as the place for such decision rationale) has no entry documenting this new
behavior.

Rule 2141673: Document non-obvious architectural decisions in docs/notes.md
weather_briefing/capabilities.py[101-119]
weather_briefing/cli.py[416-493]
docs/notes.md[1-4]

Agent prompt

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

## Issue description
This PR adds a new architectural decision: Singapore NEA nowcast is treated as a *supplementary*, best-effort capability (not blocking the primary provider) and `CapabilityProviderSet.fetch_all()` returns multiple context snapshots when no `forecast_date` is provided. Per the checklist, this kind of non-obvious decision must be documented in `docs/notes.md` with rationale, trade-offs, and operating boundaries/assumptions.

## Issue Context
The implementation introduces a new supplement path (`supplements` + `fetch_all`) and CLI wiring that classifies `nea-sg` as a local supplement by default. `docs/notes.md` currently describes its purpose as capturing key architectural choices and their boundaries, but it does not document this new supplement decision.

## Fix Focus Areas
- docs/notes.md[1-4]
- weather_briefing/capabilities.py[101-119]
- weather_briefing/cli.py[416-493]

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

---

View more (1)

6. Configured order silently rewritten 🐞 Bug ≡ Correctness

Description

_weather_context_provider() always moves nea-sg into supplements when any non-local provider
is present, so an explicit order like WEATHER_PROVIDERS=nea-sg,open-meteo will still run
Open-Meteo as the primary provider. This silently diverges from the documented semantics that the
first configured provider is the primary source and can lead to unexpected provider
selection/attribution during ops and debugging.

Code

weather_briefing/cli.py[R476-482]

    names = weather_providers_for(location, settings.weather_providers)
+    local_capability_names = {WeatherProviderName.NEA_SINGAPORE}
+    main_names = [name for name in names if name not in local_capability_names]
+    supplement_names = [name for name in names if name in local_capability_names]
+    if not main_names:
+        main_names, supplement_names = supplement_names, []
    providers: list[WeatherContextProvider] = []

Relevance

⭐⭐ Medium

PR71 docs say NEA is supplement and Open‑Meteo remains primary; may intentionally override
configured order.

PR-#71
PR-#19
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Docs describe WEATHER_PROVIDERS as an ordered primary+fallback list (first is primary), but the
CLI implementation splits out nea-sg into a supplement list that is always executed after
non-local providers, changing the meaning of the configured order when nea-sg is present.

weather_briefing/cli.py[471-532]
README.md[17-20]
README.md[58-60]
docs/design.md[57-64]

Agent prompt

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

## Issue description
When `WEATHER_PROVIDERS` is explicitly configured, the code currently partitions providers into `main_names` and `supplement_names` and always runs local providers (currently `nea-sg`) after the main provider(s). This means an explicitly configured ordering is not honored if `nea-sg` appears before another provider, despite docs stating that the first configured provider is the primary source.

## Issue Context
Design docs say NEA should behave as a supplement and not replace Open-Meteo, but the current behavior is silent normalization that can surprise users who explicitly set provider order.

## Fix Focus Areas
- weather_briefing/cli.py[471-532]
- README.md[17-20]
- README.md[58-60]
- docs/design.md[57-64]

## Proposed fix
Choose one consistent contract and enforce it:
1) **If explicit ordering must be respected:** only apply the `nea-sg` supplement split when `settings.weather_providers is None` (region defaults), and otherwise preserve the configured list as the fallback order.

2) **If `nea-sg` must always be a supplement:** validate `settings.weather_providers` so that if it contains `nea-sg` alongside other providers, either:
  - require `nea-sg` to be last (raise a clear `ValueError` if not), or
  - normalize with an explicit warning log explaining the reorder.

Also update README/design notes to explicitly state the special-case semantics for `nea-sg` under explicit `WEATHER_PROVIDERS`.

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

---

7. Area default can show None ✓ Resolved 🐞 Bug ≡ Correctness

Description

NEA forecast line formatting uses entry.get('area', 'Singapore'), so explicit null/blank “area”
values render as “None: …” or “: …” instead of falling back to “Singapore”.

Code

weather_briefing/regional_weather.py[R57-61]

+            forecast_lines = tuple(
+                f"{entry.get('area', 'Singapore')}: {entry['forecast']}"
+                for entry in forecasts
+                if isinstance(entry, dict) and isinstance(entry.get("forecast"), str)
+            )

Relevance

⭐⭐⭐ High

Team frequently fixes small correctness/formatting edge-cases; similar contract/format hardening
accepted (PRs 35,59).

PR-#35
PR-#59

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tuple comprehension directly interpolates entry.get('area', 'Singapore') into the output line,
which will stringify None or empty strings when present.

weather_briefing/regional_weather.py[55-61]

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 nowcast formatter uses `entry.get('area', 'Singapore')`, which only applies the default when the key is absent; it does not handle `{"area": None}` or `{"area": ""}`.

## Issue Context
This is defensive formatting for unexpected API shapes; it prevents awkward user-visible strings like `None: Fair`.

## Fix Focus Areas
- weather_briefing/regional_weather.py[57-61]

## Suggested fix
Normalize `area` before formatting, e.g.:
```py
area = entry.get("area")
area = area.strip() if isinstance(area, str) else ""
area = area or "Singapore"
```
Then format using the normalized `area`.
Add a small unit test covering `area=None` and `area=""`.

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

---

Previous review results

Review updated until commit 32194ec ⚖️ Balanced

Results up to commit 58653bf ⚖️ Balanced

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

1. Supplements not best-effort ✗ Dismissed 🐞 Bug ☼ Reliability

Description

CapabilityProviderSet.fetch_all awaits supplemental providers sequentially and only suppresses
WeatherContextError/ValueError, so a slow supplement can delay the whole run and an unexpected
exception type can still abort the briefing. This contradicts the documented intent that NEA
supplement failures should not block the primary weather context.

Code

weather_briefing/capabilities.py[R101-119]

+    async def fetch_all(
+        self,
+        latitude: float,
+        longitude: float,
+        *,
+        forecast_date: pendulum.Date | None = None,
+    ) -> tuple[WeatherContextSnapshot, ...]:
+        """Fetch the primary context and best-effort supplementary capabilities."""
+        snapshots = [await self.fetch(latitude, longitude, forecast_date=forecast_date)]
+        if forecast_date is not None:
+            return tuple(snapshots)
+        from .weather_context import WeatherContextError
+
+        for provider in self.supplements:
+            try:
+                snapshots.append(await _fetch_context(provider, latitude, longitude, None))
+            except (WeatherContextError, ValueError):
+                continue
+        return tuple(snapshots)

Relevance

⭐⭐ Medium

No clear historical precedent on running “best-effort supplements” concurrently/broader exception
suppression; best-effort semantics appear but details vary.

PR-#19
PR-#54

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The design doc says NEA supplement failures must not block the primary context, but fetch_all
currently awaits each supplement inline (serially) and only suppresses two exception classes.
Because the CLI uses a shared AsyncClient with a global timeout, a slow supplement can delay the
entire run by up to that timeout, and any exception outside the caught set will still propagate and
fail the run.

docs/design.md[51-58]
weather_briefing/capabilities.py[101-119]
weather_briefing/cli.py[255-280]

Agent prompt

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

## Issue description
`CapabilityProviderSet.fetch_all()` fetches supplements sequentially and only catches `WeatherContextError`/`ValueError`. This means optional supplements can (a) add up to the full configured HTTP timeout to every run when the supplement endpoint is slow, and (b) still crash the run if the supplement raises an unexpected exception type.

## Issue Context
Design explicitly states Singapore NEA is a supplement and should not block the main Open-Meteo context when it fails. The CLI’s shared `httpx` client is configured with a global timeout (`settings.http_timeout_seconds`), so any slow supplement call becomes user-visible latency.

## Fix Focus Areas
- Run supplements concurrently and never block the main snapshot longer than necessary.
- Treat supplements as "best-effort": suppress (and optionally log) all non-cancellation exceptions from supplements.
- Consider applying a shorter per-supplement timeout (separate from the main provider timeout), so an optional nowcast cannot consume the full global timeout budget.

### Code pointers
- weather_briefing/capabilities.py[101-119]

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

---

2. NEA supplement decision undocumented ✓ Resolved 📘 Rule violation ⌂ Architecture

Description

The PR introduces a best-effort supplementary weather capability (nea-sg) and new multi-snapshot
fetch behavior, which is a non-obvious architectural decision. There is no corresponding
rationale/trade-offs/boundaries entry for this decision in docs/notes.md, which violates the
documentation requirement.

Code

weather_briefing/capabilities.py[R101-119]

+    async def fetch_all(
+        self,
+        latitude: float,
+        longitude: float,
+        *,
+        forecast_date: pendulum.Date | None = None,
+    ) -> tuple[WeatherContextSnapshot, ...]:
+        """Fetch the primary context and best-effort supplementary capabilities."""
+        snapshots = [await self.fetch(latitude, longitude, forecast_date=forecast_date)]
+        if forecast_date is not None:
+            return tuple(snapshots)
+        from .weather_context import WeatherContextError
+
+        for provider in self.supplements:
+            try:
+                snapshots.append(await _fetch_context(provider, latitude, longitude, None))
+            except (WeatherContextError, ValueError):
+                continue
+        return tuple(snapshots)

Relevance

⭐⭐⭐ High

Team frequently documents non-obvious boundaries in docs/notes.md (see added “Accepted operating
boundaries” in PR #54, #58).

PR-#54
PR-#58

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2141673 requires non-obvious architectural decisions to be recorded in
docs/notes.md with rationale/trade-offs/boundaries. This PR adds a new best-effort supplement
execution path (fetch_all() iterates self.supplements and skips failures), but docs/notes.md
(which defines itself as the place for such decision rationale) has no entry documenting this new
behavior.

Rule 2141673: Document non-obvious architectural decisions in docs/notes.md
weather_briefing/capabilities.py[101-119]
weather_briefing/cli.py[416-493]
docs/notes.md[1-4]

Agent prompt

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

## Issue description
This PR adds a new architectural decision: Singapore NEA nowcast is treated as a *supplementary*, best-effort capability (not blocking the primary provider) and `CapabilityProviderSet.fetch_all()` returns multiple context snapshots when no `forecast_date` is provided. Per the checklist, this kind of non-obvious decision must be documented in `docs/notes.md` with rationale, trade-offs, and operating boundaries/assumptions.

## Issue Context
The implementation introduces a new supplement path (`supplements` + `fetch_all`) and CLI wiring that classifies `nea-sg` as a local supplement by default. `docs/notes.md` currently describes its purpose as capturing key architectural choices and their boundaries, but it does not document this new supplement decision.

## Fix Focus Areas
- docs/notes.md[1-4]
- weather_briefing/capabilities.py[101-119]
- weather_briefing/cli.py[416-493]

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

---

Results up to commit 0966f7a ⚖️ Balanced

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

1. NEA errors too generic ✓ Resolved 🐞 Bug ◔ Observability

Description

NEASingaporeNowcastProvider wraps HTTP failures into RegionalWeatherProviderError using only the
exception type name (e.g. “HTTPStatusError”), dropping safe details like the HTTP status code and
reducing diagnosability.

Code

weather_briefing/regional_weather.py[R68-69]

+        except (httpx.HTTPError, KeyError, TypeError, ValueError) as exc:
+            raise RegionalWeatherProviderError(f"NEA nowcast failed: {type(exc).__name__}") from None

Relevance

⭐⭐⭐ High

Repo prioritizes actionable but safe error details (diagnostic stage/status) over generic type names
(PRs 19,35).

PR-#19
PR-#35

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The NEA provider currently formats failures with type(exc).__name__, while the repo already has a
sanitization helper that preserves HTTP status codes for HTTPStatusError.

weather_briefing/regional_weather.py[66-69]
weather_briefing/weather_context.py[715-718]

Agent prompt

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

## Issue description
When NEA requests fail with `httpx.HTTPStatusError`, the provider raises `RegionalWeatherProviderError("NEA nowcast failed: HTTPStatusError")`, which omits the HTTP status code (e.g. 401/403/429/500). This makes logs and alerts less actionable.

## Issue Context
Other providers already sanitize errors but keep the HTTP status code (via `_safe_provider_error`). NEA nowcast should follow the same pattern.

## Fix Focus Areas
- weather_briefing/regional_weather.py[66-69]
- weather_briefing/weather_context.py[715-718]

## Suggested fix
In `regional_weather.py`, format errors similarly to `_safe_provider_error`, e.g.:
- If `isinstance(exc, httpx.HTTPStatusError)`, emit `HTTP {exc.response.status_code}`.
- Otherwise emit `{type(exc).__name__}`.
Optionally, reuse a shared helper (import `_safe_provider_error` or create a small local helper) to keep behavior consistent.

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

---

2. Nowcast labeled as daily ✓ Resolved 🐞 Bug ≡ Correctness

Description

Supplementary NEA nowcast snapshots are converted via snapshot_to_documents(), which hardcodes the
weather section label as “今明天气预报”, so the two-hour nowcast is presented to the LLM as a day-level
forecast and can mislead summaries.

Code

weather_briefing/service.py[R333-349]

+            if isinstance(self._weather_context_provider, CapabilityProviderSet):
+                weather_contexts = await self._weather_context_provider.fetch_all(
+                    self._location.latitude,
+                    self._location.longitude,
+                    forecast_date=forecast_date,
+                )
+            else:
+                weather_contexts = (
+                    await fetch_weather_context(
+                        self._weather_context_provider,
+                        self._location.latitude,
+                        self._location.longitude,
+                        forecast_date,
+                    ),
+                )
+            for weather_context in weather_contexts:
+                context_items.extend(snapshot_to_documents(weather_context))

Relevance

⭐⭐⭐ High

Team has accepted fixes correcting misleading labeling/semantics in context sent to LLM (PR 59;
boundary clarity PR 76).

PR-#59
PR-#76

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The service now converts every snapshot returned by CapabilityProviderSet.fetch_all() into
documents, but the document formatter uses a fixed “今明天气预报” label while the NEA provider explicitly
returns a two-hour nowcast string.

weather_briefing/service.py[328-350]
weather_briefing/weather_context.py[777-807]
weather_briefing/regional_weather.py[70-77]

Agent prompt

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

## Issue description
NEA nowcast snapshots ("Next two hours") are rendered into `SourceDocument.content` using `snapshot_to_documents()`, which labels the weather section as `今明天气预报`. This makes the nowcast appear like a today/tomorrow forecast in LLM context.

## Issue Context
The PR introduces supplementary weather contexts and iterates over all snapshots to convert them into documents. The existing formatter assumes all `WeatherContextSnapshot.weather_forecast` items are day-level forecasts.

## Fix Focus Areas
- weather_briefing/weather_context.py[777-807]
- weather_briefing/regional_weather.py[70-77]
- weather_briefing/service.py[332-349]

## Suggested fix
Introduce a way to distinguish nowcast vs daily forecast when rendering, e.g.:
- Add an optional field to `WeatherContextSnapshot` (e.g. `forecast_kind: Literal["daily","nowcast"]` or `forecast_horizon_label`) and have `snapshot_to_documents()` choose `两小时短时预报` / `短时预报` vs `今明天气预报` accordingly; or
- Change `snapshot_to_documents()` to use a neutral label like `天气预报` / `天气信息` (less invasive but affects all providers).

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

---

3. Area default can show None ✓ Resolved 🐞 Bug ≡ Correctness

Description

NEA forecast line formatting uses entry.get('area', 'Singapore'), so explicit null/blank “area”
values render as “None: …” or “: …” instead of falling back to “Singapore”.

Code

weather_briefing/regional_weather.py[R57-61]

+            forecast_lines = tuple(
+                f"{entry.get('area', 'Singapore')}: {entry['forecast']}"
+                for entry in forecasts
+                if isinstance(entry, dict) and isinstance(entry.get("forecast"), str)
+            )

Relevance

⭐⭐⭐ High

Team frequently fixes small correctness/formatting edge-cases; similar contract/format hardening
accepted (PRs 35,59).

PR-#35
PR-#59

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tuple comprehension directly interpolates entry.get('area', 'Singapore') into the output line,
which will stringify None or empty strings when present.

weather_briefing/regional_weather.py[55-61]

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 nowcast formatter uses `entry.get('area', 'Singapore')`, which only applies the default when the key is absent; it does not handle `{"area": None}` or `{"area": ""}`.

## Issue Context
This is defensive formatting for unexpected API shapes; it prevents awkward user-visible strings like `None: Fair`.

## Fix Focus Areas
- weather_briefing/regional_weather.py[57-61]

## Suggested fix
Normalize `area` before formatting, e.g.:
```py
area = entry.get("area")
area = area.strip() if isinstance(area, str) else ""
area = area or "Singapore"
```
Then format using the normalized `area`.
Add a small unit test covering `area=None` and `area=""`.

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

---

Results up to commit 5d07391 ⚖️ Balanced

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

1. Configured order silently rewritten 🐞 Bug ≡ Correctness

Description

_weather_context_provider() always moves nea-sg into supplements when any non-local provider
is present, so an explicit order like WEATHER_PROVIDERS=nea-sg,open-meteo will still run
Open-Meteo as the primary provider. This silently diverges from the documented semantics that the
first configured provider is the primary source and can lead to unexpected provider
selection/attribution during ops and debugging.

Code

weather_briefing/cli.py[R476-482]

    names = weather_providers_for(location, settings.weather_providers)
+    local_capability_names = {WeatherProviderName.NEA_SINGAPORE}
+    main_names = [name for name in names if name not in local_capability_names]
+    supplement_names = [name for name in names if name in local_capability_names]
+    if not main_names:
+        main_names, supplement_names = supplement_names, []
    providers: list[WeatherContextProvider] = []

Relevance

⭐⭐ Medium

PR71 docs say NEA is supplement and Open‑Meteo remains primary; may intentionally override
configured order.

PR-#71
PR-#19
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Docs describe WEATHER_PROVIDERS as an ordered primary+fallback list (first is primary), but the
CLI implementation splits out nea-sg into a supplement list that is always executed after
non-local providers, changing the meaning of the configured order when nea-sg is present.

weather_briefing/cli.py[471-532]
README.md[17-20]
README.md[58-60]
docs/design.md[57-64]

Agent prompt

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

## Issue description
When `WEATHER_PROVIDERS` is explicitly configured, the code currently partitions providers into `main_names` and `supplement_names` and always runs local providers (currently `nea-sg`) after the main provider(s). This means an explicitly configured ordering is not honored if `nea-sg` appears before another provider, despite docs stating that the first configured provider is the primary source.

## Issue Context
Design docs say NEA should behave as a supplement and not replace Open-Meteo, but the current behavior is silent normalization that can surprise users who explicitly set provider order.

## Fix Focus Areas
- weather_briefing/cli.py[471-532]
- README.md[17-20]
- README.md[58-60]
- docs/design.md[57-64]

## Proposed fix
Choose one consistent contract and enforce it:
1) **If explicit ordering must be respected:** only apply the `nea-sg` supplement split when `settings.weather_providers is None` (region defaults), and otherwise preserve the configured list as the fallback order.

2) **If `nea-sg` must always be a supplement:** validate `settings.weather_providers` so that if it contains `nea-sg` alongside other providers, either:
  - require `nea-sg` to be last (raise a clear `ValueError` if not), or
  - normalize with an explicit warning log explaining the reorder.

Also update README/design notes to explicitly state the special-case semantics for `nea-sg` under explicit `WEATHER_PROVIDERS`.

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

---

[IceCodeNew]

/agentic_review

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0966f7a

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5d07391

[IceCodeNew]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add Singapore NEA two-hour nowcast as best-effort weather supplement

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 40+ Minutes

AI Description

• Add nea-sg provider for Singapore NEA two-hour nowcast with optional API-key auth.
• Treat nea-sg as a best-effort supplement (must be last) and default it for SG.
• Extend context assembly to include supplementary snapshots and add coverage for ordering/error
 handling.

Diagram

graph TD
  svc(["BriefingService"]) --> set(["CapabilityProviderSet"]) --> main[["Primary weather"]] --> open{{"Open-Meteo API"}}
  set(["CapabilityProviderSet"]) --> sup[["NEA nowcast"]] --> nea{{"data.gov.sg NEA API"}}
  cfg["Settings + defaults"] --> set(["CapabilityProviderSet"])
  subgraph Legend
    direction LR
    _svc(["Service"]) ~~~ _adp[["Adapter"]] ~~~ _ext{{"External API"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Merge supplement into a single composite snapshot

• ➕ Consumers always see one WeatherContextSnapshot, reducing downstream branching
• ➕ Avoids multiple documents that may repeat headings/attribution
• ➖ Requires extending WeatherContextSnapshot schema to carry supplemental sections
• ➖ Harder to preserve clear provenance boundaries per provider

2. Run supplements concurrently with independent timeout budgets

• ➕ Better latency isolation when supplements are slow or flaky
• ➕ Allows future growth to multiple regional supplements without serial latency
• ➖ More complexity: task orchestration, cancellation semantics, structured logging per task
• ➖ Requires defining clearer failure isolation rules (what gets swallowed vs surfaced)

3. Model NEA as a separate capability pipeline instead of provider ordering constraints

• ➕ Eliminates the “must be last” ordering rule by construction
• ➕ Makes capability composition explicit (NOWCAST vs WEATHER)
• ➖ Larger refactor across provider selection and metadata handling
• ➖ More moving parts than needed for a single regional supplement today

Recommendation: Keep the PR’s current approach: treat nea-sg as an explicit best-effort supplement, enforce “last or only” ordering, and skip only expected failures (WeatherContextError/ValueError). It is a pragmatic minimal extension that preserves the existing primary-provider semantics while adding local detail. If supplement latency becomes a real issue, consider introducing concurrent supplement fetching with a well-defined cancellation/timeout boundary as a follow-up.

Files changed (16) +449 / -12

Enhancement (6) +204 / -10

capabilities.pyAdd supplement support and 'fetch_all()' aggregation +22/-0

Add supplement support and 'fetch_all()' aggregation

• Extends 'CapabilityProviderSet' with optional 'supplements' and their metadata. Adds 'fetch_all()' to fetch the primary snapshot and then best-effort supplements (skipping WeatherContextError/ValueError) while disabling supplements for dated forecasts.

weather_briefing/capabilities.py

cli.pyWire 'nea-sg' into provider registry and split main vs supplement providers +38/-2

Wire 'nea-sg' into provider registry and split main vs supplement providers

• Registers NEA provider metadata and builder, and adds NEA construction using Settings-provided base URL and API key. Updates provider resolution to treat NEA as a supplement unless it’s the only configured provider, and passes supplement providers/metadata into CapabilityProviderSet.

weather_briefing/cli.py

config.pyAdd NEA settings, SG defaults, and ordering validation +18/-1

Add NEA settings, SG defaults, and ordering validation

• Adds 'nea_base_url'/'nea_api_key' Settings fields and env parsing defaults. Introduces validation that 'nea-sg' must be last when combined with other providers, and extends region default selection to treat country code 'SG' specially.

weather_briefing/config.py

regional_weather.pyImplement Singapore NEA two-hour nowcast adapter +107/-0

Implement Singapore NEA two-hour nowcast adapter

• Adds 'NEASingaporeNowcastProvider' with optional 'x-api-key' auth, v2 and legacy response normalization, strict shape validation, and Singapore timezone timestamp parsing. Wraps transport/parsing failures as 'RegionalWeatherProviderError' while sanitizing sensitive error details.

weather_briefing/regional_weather.py

registries.pyAdd 'nea-sg' to WeatherProviderName registry +1/-0

Add 'nea-sg' to WeatherProviderName registry

• Introduces 'WeatherProviderName.NEA_SINGAPORE = "nea-sg"' for consistent configuration and metadata mapping.

weather_briefing/registries.py

service.pyAllow multiple weather context snapshots in prompt assembly +18/-7

Allow multiple weather context snapshots in prompt assembly

• Updates the service execution path to detect 'CapabilityProviderSet' and include documents for all fetched snapshots (primary + supplements). Preserves the previous single-snapshot path for non-capability providers.

weather_briefing/service.py

Tests (5) +233 / -1

test_capabilities.pyTest supplement snapshot fetching and failure skipping +51/-0

Test supplement snapshot fetching and failure skipping

• Adds coverage for 'CapabilityProviderSet.fetch_all()' returning primary + supplement contexts. Verifies supplements are skipped on expected errors and are not used for dated forecasts.

tests/test_capabilities.py

test_cli.pyTest NEA builder registration and primary-provider selection +19/-0

Test NEA builder registration and primary-provider selection

• Ensures the NEA provider builder exists in the local registry and can be selected. Verifies an explicit 'weather_providers=("nea-sg",)' configuration makes NEA the primary provider with fixed English language support.

tests/test_cli.py

test_config.pyValidate 'nea-sg' ordering constraints and SG defaults +29/-0

Validate 'nea-sg' ordering constraints and SG defaults

• Adds tests enforcing that 'nea-sg' must be last when combined with other providers, both via env and programmatic resolution. Verifies SG defaults resolve to '(open-meteo, nea-sg)' when not explicitly configured.

tests/test_config.py

test_regional_weather.pyAdd contract tests for NEA nowcast adapter +124/-0

Add contract tests for NEA nowcast adapter

• Introduces a full mock-transport test suite for NEA normalization, optional API-key header, legacy timestamp handling, and strict shape validation. Verifies error sanitization for non-status HTTP errors and timestamp fallback behavior.

tests/test_regional_weather.py

test_service.pyAdjust service test to pass CapabilityProviderSet for weather context +10/-1

Adjust service test to pass CapabilityProviderSet for weather context

• Updates forecast service wiring in tests to use 'CapabilityProviderSet' (with metadata) instead of a raw context provider, matching the new multi-snapshot path.

tests/test_service.py

Documentation (3) +5 / -1

README.mdDocument 'nea-sg' as Singapore-specific nowcast supplement +1/-1

Document 'nea-sg' as Singapore-specific nowcast supplement

• Updates the provider-selection description to include Singapore’s default NEA two-hour nowcast. Clarifies that 'nea-sg' must be last when combined, but can be used alone.

README.md

design.mdDescribe supplement semantics and SG default provider behavior +2/-0

Describe supplement semantics and SG default provider behavior

• Adds design notes for NEA integration as a supplement-only provider, including error-handling boundaries and serial execution. Documents the SG default append behavior and the “must be last” explicit-order constraint.

docs/design.md

notes.mdSpell out explicit boundaries for best-effort nowcast supplements +2/-0

Spell out explicit boundaries for best-effort nowcast supplements

• Captures review-time guardrails: primary context must succeed independently; expected supplement failures are dropped; no blanket exception swallowing or parallelism guarantees. Reinforces the ordering rule and the lack of independent timeout isolation.

docs/notes.md

Other (2) +7 / -0

env.exampleAdd NEA base URL and optional API key settings +3/-0

Add NEA base URL and optional API key settings

• Introduces 'NEA_BASE_URL' (default data.gov.sg) and optional 'NEA_API_KEY' placeholders to configure the Singapore NEA adapter.

env.example

provider_defaults.jsonDefault Singapore provider order to include NEA supplement +4/-0

Default Singapore provider order to include NEA supplement

• Adds an 'SG' region entry mapping to '["open-meteo", "nea-sg"]', enabling automatic NEA supplementation for Singapore when WEATHER_PROVIDERS is not explicitly set.

weather_briefing/data/provider_defaults.json

[qodo-code-review]

1. Nea used outside singapore 🐞 Bug ≡ Correctness

When WEATHER_PROVIDERS explicitly includes nea-sg, weather_providers_for() returns it for
every location and _weather_context_provider() builds it as a supplement (or even as the primary
when it's the only provider) without checking location.country_code. Since
NEASingaporeNowcastProvider.fetch() ignores the requested latitude/longitude, non-SG briefings can
receive Singapore nowcast context, producing misleading summaries.

Agent Prompt

### Issue description
`nea-sg` is Singapore-only, but the current provider selection logic applies it to *any* location whenever it appears in explicit `WEATHER_PROVIDERS`. The NEA adapter also ignores the input coordinates, so calling it for non-SG locations returns Singapore nowcast and pollutes the context.

### Issue Context
- Explicit `WEATHER_PROVIDERS` applies globally across all configured locations.
- `_weather_context_provider()` partitions `nea-sg` into supplements purely by provider name, not by `location.country_code`.
- `NEASingaporeNowcastProvider.fetch()` does not use `latitude`/`longitude`, so it cannot self-correct without an explicit guard.

### Fix Focus Areas
- weather_briefing/cli.py[476-532]
- weather_briefing/config.py[224-236]
- weather_briefing/regional_weather.py[42-77]

### Implementation guidance
Pick one (or combine for defense-in-depth):
1) **Selection-time guard (recommended):** In `_weather_context_provider()` (or in `weather_providers_for()`), drop `nea-sg` unless `location.country_code == "SG"`. If that removal would leave no providers (e.g. explicit `WEATHER_PROVIDERS=("nea-sg",)` on a non-SG location), raise a clear `ValueError/ConfigurationError`.
2) **Provider-level guard:** In `NEASingaporeNowcastProvider.fetch()`, validate the coordinates are within a Singapore bounding box and raise `RegionalWeatherProviderError` when outside; this prevents accidental misuse even if selection logic regresses.

Add/adjust tests to cover multi-location behavior with explicit `WEATHER_PROVIDERS=open-meteo,nea-sg` to ensure non-SG locations do not call NEA.

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 32194ec